RE: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not

[Ronnie.Kunin]

Thanks very much for your feedback and suggestions Andrew, some comments and clarifications below.

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Tuesday, April 23, 2024 3:11 PM
> To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@xxxxxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> edumazet@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Bryan Whitehead - C21958
> <Bryan.Whitehead@xxxxxxxxxxxxx>; UNGLinuxDriver <UNGLinuxDriver@microchipcom>
> Subject: Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > If we don't enable/register the PCI11x1x's ethernet device in wake
> > list by calling " device_set_wakeup_enable( )" function, device power
> > state shows as disable.
> 
> > PHY (GPY211C)'s interrupt pin (MDINT) connect to PCI11x1x's ethernet device.
> 
> > When I configure the WAKE_PHY or WAKE_MAGIC on GPY211C PHY, Interrupt
> > generation observed when magic packet or link activity (link down or
> > link up).  If wakeup enable in PCI11x1x's ethernet device, System
> > resumes from sleep.
> 
> This is the bit that is missing from your commit message, and maybe it should be in a comment. The
> interrupt controller is part of the MAC. So you need to leave MAC burning power, maybe even
> processing packets, because you cannot disable the MAC but leave the interrupt controller functioning,
> so that it can trigger a wake up via PCIe.

I think there is a terminology problem here. Within MCHP we sometimes call "the MAC" to the whole ethernet controller chip that has everything (i.e. actual MAC, FIFOs, filtering engines, offloads, interrupt controller, bus interface, etc) except the PHY.
That is what Raju probably means when he says that the interrupt is handled by "the MAC". While the registers that control enabling/disabling processing of the phy interrupt do reside within the MAC block's register set in the ethernet controller, it neither means that the extensive parts of the actual MAC block need to be kept enabled nor that processing packets has to occur in the MAC in order for the PCI11x1x chip to detect an event coming from the PHY that should be processed as a wake event over PCIe  

> 
> There are a few things you should consider:
> 
> Call phy_speed_down().  This will renegotiate the link, dropping it to the slowest speed both link
> partners support. So hopefully down to 10Mbps. Your MAC will then only need to pointlessly process
> 10Mbps of packets, rather than 1Gbps.p

I am Windows driver expert, not Linux so I may be wrong for Linux, but with the advent of dynamic PM in modern OSs (connected and then modern standby in Windows, I remember also autosuspend - at least in USB, maybe not applicable to PCIe  - in Linux ) we have stayed away from renegotiating the link to down speed during suspend - resume events since those interfere with / delay connectivity significantly.

> 
> See if you can disable parts of the MAC, in particularly the receive engine, in order to save power.

This is already supported by the hardware and I had told Raju to do it as soon as he successfully got the host to wake over PCIe from events signaled from the PHY 

> 
> Talk to your hardware engineer and see if the next generation of the hardware can separate the
> interrupt controller from the MAC, so you can disable the MAC and leave the interrupt controller
> functioning.

As mentioned above, for all intents and purposes, that is already the case.

> 
> > Please find the attached prototype code change (Temporary patch)for reference.
> > I will submit this patch separately.
> 
> Please just submit it in the normal way for review.
> 
>        Andrew