Message-ID: <LV8PR11MB8700A34520EA2521BC5F59EF9F112@LV8PR11MB8700.namprd11.prod.outlook.com>
Date: Tue, 23 Apr 2024 11:05:44 +0000
From: <Raju.Lakkaraju@...rochip.com>
To: <andrew@...n.ch>
CC: <netdev@...r.kernel.org>, <davem@...emloft.net>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <edumazet@...gle.com>, <linux-kernel@...r.kernel.org>,
	<Bryan.Whitehead@...rochip.com>, <UNGLinuxDriver@...rochip.com>
Subject: RE: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY
 does not

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Friday, April 5, 2024 10:42 PM
> To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@...rochip.com>
> Cc: netdev@...r.kernel.org; davem@...emloft.net; kuba@...nel.org;
> pabeni@...hat.com; edumazet@...gle.com; linux-kernel@...r.kernel.org;
> Bryan Whitehead - C21958 <Bryan.Whitehead@...rochip.com>;
> UNGLinuxDriver <UNGLinuxDriver@...rochip.com>
> 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
> 
> On Fri, Apr 05, 2024 at 08:17:22AM +0000, Raju.Lakkaraju@...rochip.com
> wrote:
> > Hi Andrew,
> >
> > Sorry for delayed response.
> >
> > > -----Original Message-----
> > > From: Andrew Lunn <andrew@...n.ch>
> > > Sent: Thursday, March 21, 2024 4:23 AM
> > > To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@...rochip.com>
> > > Cc: netdev@...r.kernel.org; davem@...emloft.net; kuba@...nel.org;
> > > pabeni@...hat.com; edumazet@...gle.com;
> > > linux-kernel@...r.kernel.org; Bryan Whitehead - C21958
> > > <Bryan.Whitehead@...rochip.com>; UNGLinuxDriver
> > > <UNGLinuxDriver@...rochip.com>
> > > 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 (netdev->phydev) {
> > > > +             ret = phy_ethtool_set_wol(netdev->phydev, wol);
> > > > +             if (ret != -EOPNOTSUPP && ret != 0)
> > > > +                     return ret;
> > >
> > > I'm not sure this condition is correct.
> > >
> > > If there is an error, and the error is not EOPNOTSUPP, you want to
> > > report that error. However, if the PHY can support the WoL
> > > configuration, it will return 0, and this function should exit, WoL
> > > in the MAC is not needed. And doing WoL in the PHY consumes less
> power since you can suspend the MAC.
> > >
> > > So i think it should simply be:
> > >
> > > > +             if (ret != -EOPNOTSUPP)
> > > > +                     return ret;
> > >
> > > Do you have a board with this MAC with a PHY which does have some
> > > WoL support. Could you test PHY WoL is used when appropriate?
> >
> > Yes.
> > We have external PHY (Max Linear GPY211C) attach to MAC of PCI11x1x
> > (PCIe Ethernet chip) If I don't register the Ethernet module in wakeup
> source, WOL is not working. Ethernet device power state shows as disable.
> 
> So i'm talking about the case where the PHY is doing the wakeup, without
> help from the MAC. In that case, "Ethernet device power state shows as
> disable." is sensible.
> 

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.
When I refer the other vendor pcie's ethernet drivers, " device_set_wakeup_enable( )" function called in ethernet driver Power Management's suspend and resume functions.
But LAN743x driver call this function when Ethtool configure the WOL.

> > i.e.
> >
> /sys/devices/pci0000:00/0000:00:1c.4/0000:05:00.0/0000:06:03.0/0000:09:00.
> 0/power/wakeup   <--  disabled

When I call the device_set_wakeup_enable( ) with enable = 1, wakeup (/sys/devices/pci0000:00/0000:00:1c.4/0000:05:00.0/0000:06:03.0/0000:09:00.0/power/wakeup) shows enable.
Also observe that device (PCI11x1x's ethernet device : 0000:09:00.0) add in wake list.
i.e.
device: 'wakeup34': device_add

> >
> > PCI11x1x is PCIe bridge device between PCIe and Ethernet along with
> > other peripherals (i.e. UART, SPI, I2C, USB and PCIe devices)
> > 0000:09:00.0 - Ethernet device
> > 0000:05:00.0 - PCIe Bridge Up link
> 
> How does the PHY indicate wake up? It could be you can power off the MAC,
> but you need to keep parts of the PCIe bridge up, in order the wake up gets
> delivered?
> 

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.

> > When I test the WOL_PHY option on setup (PCI11x1x MAC + GPY211C PHY),
> > observe the following:
> 
> > 1. When enable WOL_PHY by using ethtool (i.e. ethtool -s enp9s0 wol
> > p), GPY211 PHY configure the WOL. After resume from sleep, GPY211 WOL
> > configuration vanish. Observed that gpy_config_init( ) function reset.
> > Is it expected behaviour ? In other mail thread, we discussed that
> > Ethtool configuration should retain after resume from sleep.
> 
> The whole point of suspend/resume is that the configuration is retained. So i
> would expect from the users perspective WoL is still enabled.
> 
> As you point out, we might have a logic issue here, that on resume we hit the
> PHY with a hardware reset. That could be clearing out WoL?  We might need
> to cache the PHY WoL configuration in phydev, and on resume re-apply it.
> WoL is complex so we need to be careful who is actually managing it. But this
> seems like something which could be done in the phylib core.
> 

Yes. You are correct.  On resume we hit the PHY with a hardware reset. That could be clearing out WoL.

On resume, phy_init_hw( ) calls the PHY's configuration and interrupt functions to resets.
As you suggested, I add the wolopts flag in phydev and update this when set_wol function execute. This change fix the issue.
i.e.
Re-apply WOL configuration on PHY in phy_suspend( ) function.

Please find the attached prototype code change (Temporary patch)for reference. 
I will submit this patch separately.

> > 2. when WOL configure with ethtool, Either Link-down and Link-up on
> > CLI, WOL configuration vanish. Is it expected behaviour ? Due to this,
> > every time we have to configure WOL through Ethtool.
> 
> This is unexpected. I would expect WoL to remain configured until the
> configuration is changed.
> 

Observe that magic packet configuration(ethtool's wol g option) did not vanish.
But when Link flap occur for WAKE_PHY case, this issue observes. This might be recent "netlink" changes issue. 
I will debug and confirm.

> > Based on above information, We need to check for return < 0 condition
> > and return the error. Else enable the wakeup by calling
> > "device_set_wakeup_enable( )" function.
> 
> Once it is clear how the PHY does the wakeup, we can look at this. However, if
> the PHY can support the WoL, you should not be configuring the MAC as well
> to do the same WoL, you should be putting the MAC into a low power mode.
> 

When PHY support's the WOL (WAKE_PHY, WAKE_MAGIC), it's generated only MDINT interrupt which can handle by MAC.
But we must configure the wake phy interrupt bit in MAC's Power Management register.

PCI11x1x's ethernet device MAC supports "Secure-ON" features, but GPY211 does not support "Secure-ON".
So, we have to use MAC WOL for secure-on feature.

I tested this part. 
When enable the MAC's WAKE_PHY_INT bit, when PHY generate the interrupt, System resumes from sleep.

>     Andrew

Thanks,
Raju

Download attachment "0001-Wake-ON-LAN-PHY-reconfiguration.patch" of type "application/octet-stream" (2015 bytes)

Powered by blists - more mailing lists