Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY

Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY

[Russell King (Oracle)]

On Wed, Jun 28, 2023 at 06:13:23PM +0530, Revanth Kumar Uppala wrote:
> From: Narayan Reddy <narayanr@nvidia.com>
> 
> Enable flow control support using pause frames in aquantia phy driver.
> 
> Signed-off-by: Narayan Reddy <narayanr@nvidia.com>
> Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com>

I think this is over-complex.

>  #define MDIO_PHYXS_VEND_IF_STATUS		0xe812
>  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK	GENMASK(7, 3)
>  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR	0
> @@ -583,6 +585,17 @@ static int aqr107_config_init(struct phy_device *phydev)
>  	if (!ret)
>  		aqr107_chip_info(phydev);
>  
> +	/* Advertize flow control */
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported);
> +	linkmode_copy(phydev->advertising, phydev->supported);

This is the wrong place to be doing this, since pause support depends
not only on the PHY but also on the MAC. There are phylib interfaces
that MACs should call so that phylib knows that the MAC supports pause
frames.

Secondly, the PHY driver needs to tell phylib that the PHY supports
pause frames, and that's done through either setting the .features
member in the PHY driver, or by providing a .get_features
implementation.

Configuration of the pause advertisement should already be happening
through the core phylib code.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY

[Revanth Kumar Uppala]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, June 28, 2023 7:17 PM
> To: Russell King (Oracle) <linux@armlinux.org.uk>
> Cc: Revanth Kumar Uppala <ruppala@nvidia.com>; hkallweit1@gmail.com;
> netdev@vger.kernel.org; linux-tegra@vger.kernel.org; Narayan Reddy
> <narayanr@nvidia.com>
> Subject: Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support
> in aquantia PHY
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Jun 28, 2023 at 02:30:48PM +0100, Russell King (Oracle) wrote:
> > On Wed, Jun 28, 2023 at 06:13:23PM +0530, Revanth Kumar Uppala wrote:
> > > From: Narayan Reddy <narayanr@nvidia.com>
> > >
> > > Enable flow control support using pause frames in aquantia phy driver.
> > >
> > > Signed-off-by: Narayan Reddy <narayanr@nvidia.com>
> > > Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com>
> >
> > I think this is over-complex.
> >
> > >  #define MDIO_PHYXS_VEND_IF_STATUS          0xe812
> > >  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK        GENMASK(7, 3)
> > >  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR  0 @@ -583,6 +585,17
> @@
> > > static int aqr107_config_init(struct phy_device *phydev)
> > >     if (!ret)
> > >             aqr107_chip_info(phydev);
> > >
> > > +   /* Advertize flow control */
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev-
> >supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev-
> >supported);
> > > +   linkmode_copy(phydev->advertising, phydev->supported);
> >
> > This is the wrong place to be doing this, since pause support depends
> > not only on the PHY but also on the MAC. There are phylib interfaces
> > that MACs should call so that phylib knows that the MAC supports pause
> > frames.
> >
> > Secondly, the PHY driver needs to tell phylib that the PHY supports
> > pause frames, and that's done through either setting the .features
> > member in the PHY driver, or by providing a .get_features
> > implementation.
> >
> > Configuration of the pause advertisement should already be happening
> > through the core phylib code.
> 
> I really should do a LPC netdev talk "Everybody gets pause wrong..."
> 
> genphy_c45_an_config_aneg() will configure pause advertisement. The PHY
> driver does not need to configure it, if the PHY follows the standard and has the
> configuration in the correct place. As Russell said, please check the PHYs ability
> to advertise pause is being reported correctly, by .get_features, of the default
> implementation of .get_features if that is being used. And then check your MAC
> driver is also indicating it supports pause.
From .get_features, it is not possible to check PHY's ability to advertise pause is being reported as there is no such register present for AQR PHY to check capabilities in its datasheet.
Hence, we are directly configuring the pause frames from  aqr107_config_init().
Thanks,
Revanth Uppala
> 
>         Andrew

Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY

[Russell King (Oracle)]

On Mon, Jul 24, 2023 at 11:29:18AM +0000, Revanth Kumar Uppala wrote:
> 
> 
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Wednesday, June 28, 2023 7:17 PM
> > To: Russell King (Oracle) <linux@armlinux.org.uk>
> > Cc: Revanth Kumar Uppala <ruppala@nvidia.com>; hkallweit1@gmail.com;
> > netdev@vger.kernel.org; linux-tegra@vger.kernel.org; Narayan Reddy
> > <narayanr@nvidia.com>
> > Subject: Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support
> > in aquantia PHY
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, Jun 28, 2023 at 02:30:48PM +0100, Russell King (Oracle) wrote:
> > > On Wed, Jun 28, 2023 at 06:13:23PM +0530, Revanth Kumar Uppala wrote:
> > > > From: Narayan Reddy <narayanr@nvidia.com>
> > > >
> > > > Enable flow control support using pause frames in aquantia phy driver.
> > > >
> > > > Signed-off-by: Narayan Reddy <narayanr@nvidia.com>
> > > > Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com>
> > >
> > > I think this is over-complex.
> > >
> > > >  #define MDIO_PHYXS_VEND_IF_STATUS          0xe812
> > > >  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK        GENMASK(7, 3)
> > > >  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR  0 @@ -583,6 +585,17
> > @@
> > > > static int aqr107_config_init(struct phy_device *phydev)
> > > >     if (!ret)
> > > >             aqr107_chip_info(phydev);
> > > >
> > > > +   /* Advertize flow control */
> > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev-
> > >supported);
> > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev-
> > >supported);
> > > > +   linkmode_copy(phydev->advertising, phydev->supported);
> > >
> > > This is the wrong place to be doing this, since pause support depends
> > > not only on the PHY but also on the MAC. There are phylib interfaces
> > > that MACs should call so that phylib knows that the MAC supports pause
> > > frames.
> > >
> > > Secondly, the PHY driver needs to tell phylib that the PHY supports
> > > pause frames, and that's done through either setting the .features
> > > member in the PHY driver, or by providing a .get_features
> > > implementation.
> > >
> > > Configuration of the pause advertisement should already be happening
> > > through the core phylib code.
> > 
> > I really should do a LPC netdev talk "Everybody gets pause wrong..."
> > 
> > genphy_c45_an_config_aneg() will configure pause advertisement. The PHY
> > driver does not need to configure it, if the PHY follows the standard and has the
> > configuration in the correct place. As Russell said, please check the PHYs ability
> > to advertise pause is being reported correctly, by .get_features, of the default
> > implementation of .get_features if that is being used. And then check your MAC
> > driver is also indicating it supports pause.
> From .get_features, it is not possible to check PHY's ability to advertise pause is being reported as there is no such register present for AQR PHY to check capabilities in its datasheet.
> Hence, we are directly configuring the pause frames from  aqr107_config_init().

... and thus creating a trashy implementation... so NAK.

The first thing to get straight is that in "normal" non-rate adapting
setups, pause frames are no different from normal Ethernet frames as
far as a PHY is concerned. The PHY should be doing absolutely nothing
special with pause frames - it should merely forward them to the MAC,
and it's the MAC that deals with pause frames.

The only thing that a non-rate adapting baseT PHY has to deal with is
the media advertisement and nothing else.

So, whether pause frames are supported has more to do with the MAC
than the PHY.

The way phylib works is that when a PHY is probed, phy_probe() will
set the Pause and Asym_Pause bits in the ->supported bitmap. It is
then up to the MAC driver (or phylink) to call phy_support_asym_pause()
or phy_support_sym_pause() to tell phylib what the MAC supports.

PHY drivers must *not* override this in their config_init() function,
and most certainly must not copy the supported bitmap to the advertising
bitmap there either.

If you need pause-mode rate adaption, this has nothing to do with the
media side, and ->supported and ->advertising are not relevant for
that. Non-phylink based MAC drivers have to use phy_get_rate_matching()
to find out whether the PHY will be using rate adaption and act
accordingly. phylink based MAC drivers have this dealt with for them
and as long as they do what phylink tells them to do, everything
should be fine.

So, basically, do not mess with setting bits in the ->supported bitmap
nor the ->advertising bitmap in config_init. It is wrong, and we will
NAK it.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side

[Russell King (Oracle)]

On Wed, Jun 28, 2023 at 06:13:25PM +0530, Revanth Kumar Uppala wrote:
> +	/* Lane bring-up failures are seen during interface up, as interface
> +	 * speed settings are configured while the PHY is still initializing.
> +	 * To resolve this, poll until PHY system side interface gets ready
> +	 * and the interface speed settings are configured.
> +	 */
> +	ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS, MDIO_PHYXS_VEND_IF_STATUS,
> +					val, (val & MDIO_PHYXS_VEND_IF_STATUS_TX_READY),
> +					20000, 2000000, false);

What does this actually mean when the condition succeeds? Does it mean
that the system interface is now fully configured (but may or may not
have link)?

If that's correct, then that's fine. If it doesn't succeed because
the system interface doesn't have link, then that would be very bad,
because _this_ function needs to return so the MAC side can then be
configured to gain link with the PHY with the appropriate link
parameters.

The comment doesn't make it clear which it is.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side

[Revanth Kumar Uppala]

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Wednesday, June 28, 2023 7:04 PM
> To: Revanth Kumar Uppala <ruppala@nvidia.com>
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org; linux-
> tegra@vger.kernel.org
> Subject: Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Jun 28, 2023 at 06:13:25PM +0530, Revanth Kumar Uppala wrote:
> > +     /* Lane bring-up failures are seen during interface up, as interface
> > +      * speed settings are configured while the PHY is still initializing.
> > +      * To resolve this, poll until PHY system side interface gets ready
> > +      * and the interface speed settings are configured.
> > +      */
> > +     ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS,
> MDIO_PHYXS_VEND_IF_STATUS,
> > +                                     val, (val & MDIO_PHYXS_VEND_IF_STATUS_TX_READY),
> > +                                     20000, 2000000, false);
> 
> What does this actually mean when the condition succeeds? Does it mean that
> the system interface is now fully configured (but may or may not have link)?
Yes, your understanding is correct.
It means that the system interface is now fully configured and has the link.
> 
> If that's correct, then that's fine. If it doesn't succeed because the system
> interface doesn't have link, then that would be very bad, because _this_ function
> needs to return so the MAC side can then be configured to gain link with the PHY
> with the appropriate link parameters.
> 
> The comment doesn't make it clear which it is.
I will add the comment more clearly in V2 series.
> 
> Thanks.
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side

[Russell King (Oracle)]

On Mon, Jul 24, 2023 at 11:29:33AM +0000, Revanth Kumar Uppala wrote:
> > -----Original Message-----
> > From: Russell King <linux@armlinux.org.uk>
> > Sent: Wednesday, June 28, 2023 7:04 PM
> > To: Revanth Kumar Uppala <ruppala@nvidia.com>
> > Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org; linux-
> > tegra@vger.kernel.org
> > Subject: Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, Jun 28, 2023 at 06:13:25PM +0530, Revanth Kumar Uppala wrote:
> > > +     /* Lane bring-up failures are seen during interface up, as interface
> > > +      * speed settings are configured while the PHY is still initializing.
> > > +      * To resolve this, poll until PHY system side interface gets ready
> > > +      * and the interface speed settings are configured.
> > > +      */
> > > +     ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS,
> > MDIO_PHYXS_VEND_IF_STATUS,
> > > +                                     val, (val & MDIO_PHYXS_VEND_IF_STATUS_TX_READY),
> > > +                                     20000, 2000000, false);
> > 
> > What does this actually mean when the condition succeeds? Does it mean that
> > the system interface is now fully configured (but may or may not have link)?
> Yes, your understanding is correct.
> It means that the system interface is now fully configured and has the link.

As you indicate that it also indicates that the system interface has
link, then you leave me no option but to NAK this patch, sorry. The
reason is:

> > ... If it doesn't succeed because the system
> > interface doesn't have link, then that would be very bad, because _this_ function
> > needs to return so the MAC side can then be configured to gain link with the PHY
> > with the appropriate link parameters.

Essentially, if the PHY changes its host interface because the media
side has changed, we *need* the read_status() function to succeed, tell
us that the link is up, and what the parameters are for the media side
link _and_ the host side interface.

At this point, if the PHY has changed its host-side interface, then the
link with the host MAC will be _down_ because the MAC driver is not yet
aware of the new parameters for the link. read_status() has to succeed
and report the new parameters to the MAC so that the MAC (or phylink)
can reconfigure the MAC and PCS for the PHY's new operating mode.

Sorry, but NAK.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side

[Jon Hunter]

Hi Russell,

On 24/07/2023 12:57, Russell King (Oracle) wrote:
> On Mon, Jul 24, 2023 at 11:29:33AM +0000, Revanth Kumar Uppala wrote:
>>> -----Original Message-----
>>> From: Russell King <linux@armlinux.org.uk>
>>> Sent: Wednesday, June 28, 2023 7:04 PM
>>> To: Revanth Kumar Uppala <ruppala@nvidia.com>
>>> Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org; linux-
>>> tegra@vger.kernel.org
>>> Subject: Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side
>>>
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Wed, Jun 28, 2023 at 06:13:25PM +0530, Revanth Kumar Uppala wrote:
>>>> +     /* Lane bring-up failures are seen during interface up, as interface
>>>> +      * speed settings are configured while the PHY is still initializing.
>>>> +      * To resolve this, poll until PHY system side interface gets ready
>>>> +      * and the interface speed settings are configured.
>>>> +      */
>>>> +     ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS,
>>> MDIO_PHYXS_VEND_IF_STATUS,
>>>> +                                     val, (val & MDIO_PHYXS_VEND_IF_STATUS_TX_READY),
>>>> +                                     20000, 2000000, false);
>>>
>>> What does this actually mean when the condition succeeds? Does it mean that
>>> the system interface is now fully configured (but may or may not have link)?
>> Yes, your understanding is correct.
>> It means that the system interface is now fully configured and has the link.
> 
> As you indicate that it also indicates that the system interface has
> link, then you leave me no option but to NAK this patch, sorry. The
> reason is:
> 
>>> ... If it doesn't succeed because the system
>>> interface doesn't have link, then that would be very bad, because _this_ function
>>> needs to return so the MAC side can then be configured to gain link with the PHY
>>> with the appropriate link parameters.
> 
> Essentially, if the PHY changes its host interface because the media
> side has changed, we *need* the read_status() function to succeed, tell
> us that the link is up, and what the parameters are for the media side
> link _and_ the host side interface.
> 
> At this point, if the PHY has changed its host-side interface, then the
> link with the host MAC will be _down_ because the MAC driver is not yet
> aware of the new parameters for the link. read_status() has to succeed
> and report the new parameters to the MAC so that the MAC (or phylink)
> can reconfigure the MAC and PCS for the PHY's new operating mode.
> 
> Sorry, but NAK.


Apologies for not following up before on this and now that is has been a 
year I am not sure if it is even appropriate to dig this up as opposed 
to starting a new thread completely.

However, I want to resume this conversation because we have found that 
this change does resolve a long-standing issue where we occasionally see 
our ethernet controller fail to get an IP address.

I understand that your objection to the above change is that (per 
Revanth's feedback) this change assumes interface has the link. However, 
looking at the aqr107_read_status() function where this change is made 
the function has the following ...

static int aqr107_read_status(struct phy_device *phydev)
{
         int val, ret;

         ret = aqr_read_status(phydev);
         if (ret)
                 return ret;

         if (!phydev->link || phydev->autoneg == AUTONEG_DISABLE)
                 return 0;


So my understanding is that if we don't have the link, then the above 
test will return before we attempt to poll the TX ready status. If that 
is the case, then would the change being proposed be OK?

If you prefer we re-send and restart the discussion again, versus 
reviving a dead thread, that's fine and I will do just that.

Thanks
Jon

-- 
nvpublic

Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side

[Russell King (Oracle)]

On Fri, Jul 19, 2024 at 02:27:24PM +0100, Jon Hunter wrote:
> Hi Russell,
> 
> On 24/07/2023 12:57, Russell King (Oracle) wrote:
> > On Mon, Jul 24, 2023 at 11:29:33AM +0000, Revanth Kumar Uppala wrote:
> > > > -----Original Message-----
> > > > From: Russell King <linux@armlinux.org.uk>
> > > > Sent: Wednesday, June 28, 2023 7:04 PM
> > > > To: Revanth Kumar Uppala <ruppala@nvidia.com>
> > > > Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org; linux-
> > > > tegra@vger.kernel.org
> > > > Subject: Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side
> > > > 
> > > > External email: Use caution opening links or attachments
> > > > 
> > > > 
> > > > On Wed, Jun 28, 2023 at 06:13:25PM +0530, Revanth Kumar Uppala wrote:
> > > > > +     /* Lane bring-up failures are seen during interface up, as interface
> > > > > +      * speed settings are configured while the PHY is still initializing.
> > > > > +      * To resolve this, poll until PHY system side interface gets ready
> > > > > +      * and the interface speed settings are configured.
> > > > > +      */
> > > > > +     ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS,
> > > > MDIO_PHYXS_VEND_IF_STATUS,
> > > > > +                                     val, (val & MDIO_PHYXS_VEND_IF_STATUS_TX_READY),
> > > > > +                                     20000, 2000000, false);
> > > > 
> > > > What does this actually mean when the condition succeeds? Does it mean that
> > > > the system interface is now fully configured (but may or may not have link)?
> > > Yes, your understanding is correct.
> > > It means that the system interface is now fully configured and has the link.
> > 
> > As you indicate that it also indicates that the system interface has
> > link, then you leave me no option but to NAK this patch, sorry. The
> > reason is:
> > 
> > > > ... If it doesn't succeed because the system
> > > > interface doesn't have link, then that would be very bad, because _this_ function
> > > > needs to return so the MAC side can then be configured to gain link with the PHY
> > > > with the appropriate link parameters.
> > 
> > Essentially, if the PHY changes its host interface because the media
> > side has changed, we *need* the read_status() function to succeed, tell
> > us that the link is up, and what the parameters are for the media side
> > link _and_ the host side interface.
> > 
> > At this point, if the PHY has changed its host-side interface, then the
> > link with the host MAC will be _down_ because the MAC driver is not yet
> > aware of the new parameters for the link. read_status() has to succeed
> > and report the new parameters to the MAC so that the MAC (or phylink)
> > can reconfigure the MAC and PCS for the PHY's new operating mode.
> > 
> > Sorry, but NAK.
> 
> 
> Apologies for not following up before on this and now that is has been a
> year I am not sure if it is even appropriate to dig this up as opposed to
> starting a new thread completely.
> 
> However, I want to resume this conversation because we have found that this
> change does resolve a long-standing issue where we occasionally see our
> ethernet controller fail to get an IP address.
> 
> I understand that your objection to the above change is that (per Revanth's
> feedback) this change assumes interface has the link. However, looking at
> the aqr107_read_status() function where this change is made the function has
> the following ...
> 
> static int aqr107_read_status(struct phy_device *phydev)
> {
>         int val, ret;
> 
>         ret = aqr_read_status(phydev);
>         if (ret)
>                 return ret;
> 
>         if (!phydev->link || phydev->autoneg == AUTONEG_DISABLE)
>                 return 0;
> 
> 
> So my understanding is that if we don't have the link, then the above test
> will return before we attempt to poll the TX ready status. If that is the
> case, then would the change being proposed be OK?

Here, phydev->link will be the _media_ side link. This is fine - if the
media link is down, there's no point doing anything further. However,
if the link is up, then we need the PHY to update phydev->interface
_and_ report that the link was up (phydev->link is true).

When that happens, the layers above (e.g. phylib, phylink, MAC driver)
then know that the _media_ side interface has come up, and they also
know the parameters that were negotiated. They also know what interface
mode the PHY is wanting to use.

At that point, the MAC driver can then reconfigure its PHY facing
interface according to what the PHY is using. Until that point, there
is a very real chance that the PHY <--> MAC connection will remain
_down_.

The patch adds up to a _two_ _second_ wait for the PHY <--> MAC
connection to come up before aqr107_read_status() will return. This
is total nonsense - because waiting here means that the MAC won't
get the notification of which interface mode the PHY is expecting
to use, therefore the MAC won't configure its PHY facing hardware
for that interface mode, and therefore the PHY <--> MAC connection
will _not_ _come_ _up_.

You can not wait for the PHY <--> MAC connection to come up in the
phylib read_status method. Ever.

This is non-negotiable because it is just totally wrong to do this
and leads to pointless two second delays.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side

[Jon Hunter]

On 29/07/2024 11:47, Russell King (Oracle) wrote:

...

>> Apologies for not following up before on this and now that is has been a
>> year I am not sure if it is even appropriate to dig this up as opposed to
>> starting a new thread completely.
>>
>> However, I want to resume this conversation because we have found that this
>> change does resolve a long-standing issue where we occasionally see our
>> ethernet controller fail to get an IP address.
>>
>> I understand that your objection to the above change is that (per Revanth's
>> feedback) this change assumes interface has the link. However, looking at
>> the aqr107_read_status() function where this change is made the function has
>> the following ...
>>
>> static int aqr107_read_status(struct phy_device *phydev)
>> {
>>          int val, ret;
>>
>>          ret = aqr_read_status(phydev);
>>          if (ret)
>>                  return ret;
>>
>>          if (!phydev->link || phydev->autoneg == AUTONEG_DISABLE)
>>                  return 0;
>>
>>
>> So my understanding is that if we don't have the link, then the above test
>> will return before we attempt to poll the TX ready status. If that is the
>> case, then would the change being proposed be OK?
> 
> Here, phydev->link will be the _media_ side link. This is fine - if the
> media link is down, there's no point doing anything further. However,
> if the link is up, then we need the PHY to update phydev->interface
> _and_ report that the link was up (phydev->link is true).
> 
> When that happens, the layers above (e.g. phylib, phylink, MAC driver)
> then know that the _media_ side interface has come up, and they also
> know the parameters that were negotiated. They also know what interface
> mode the PHY is wanting to use.
> 
> At that point, the MAC driver can then reconfigure its PHY facing
> interface according to what the PHY is using. Until that point, there
> is a very real chance that the PHY <--> MAC connection will remain
> _down_.
> 
> The patch adds up to a _two_ _second_ wait for the PHY <--> MAC
> connection to come up before aqr107_read_status() will return. This
> is total nonsense - because waiting here means that the MAC won't
> get the notification of which interface mode the PHY is expecting
> to use, therefore the MAC won't configure its PHY facing hardware
> for that interface mode, and therefore the PHY <--> MAC connection
> will _not_ _come_ _up_.
> 
> You can not wait for the PHY <--> MAC connection to come up in the
> phylib read_status method. Ever.
> 
> This is non-negotiable because it is just totally wrong to do this
> and leads to pointless two second delays.


Thanks for the feedback! We will go away, review this and see if we can 
figure out a good/correct way to resolve our ethernet issue.

Jon

-- 
nvpublic

Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side

[Russell King (Oracle)]

On Tue, Jul 30, 2024 at 10:36:12AM +0100, Jon Hunter wrote:
> 
> On 29/07/2024 11:47, Russell King (Oracle) wrote:
> 
> ...
> 
> > > Apologies for not following up before on this and now that is has been a
> > > year I am not sure if it is even appropriate to dig this up as opposed to
> > > starting a new thread completely.
> > > 
> > > However, I want to resume this conversation because we have found that this
> > > change does resolve a long-standing issue where we occasionally see our
> > > ethernet controller fail to get an IP address.
> > > 
> > > I understand that your objection to the above change is that (per Revanth's
> > > feedback) this change assumes interface has the link. However, looking at
> > > the aqr107_read_status() function where this change is made the function has
> > > the following ...
> > > 
> > > static int aqr107_read_status(struct phy_device *phydev)
> > > {
> > >          int val, ret;
> > > 
> > >          ret = aqr_read_status(phydev);
> > >          if (ret)
> > >                  return ret;
> > > 
> > >          if (!phydev->link || phydev->autoneg == AUTONEG_DISABLE)
> > >                  return 0;
> > > 
> > > 
> > > So my understanding is that if we don't have the link, then the above test
> > > will return before we attempt to poll the TX ready status. If that is the
> > > case, then would the change being proposed be OK?
> > 
> > Here, phydev->link will be the _media_ side link. This is fine - if the
> > media link is down, there's no point doing anything further. However,
> > if the link is up, then we need the PHY to update phydev->interface
> > _and_ report that the link was up (phydev->link is true).
> > 
> > When that happens, the layers above (e.g. phylib, phylink, MAC driver)
> > then know that the _media_ side interface has come up, and they also
> > know the parameters that were negotiated. They also know what interface
> > mode the PHY is wanting to use.
> > 
> > At that point, the MAC driver can then reconfigure its PHY facing
> > interface according to what the PHY is using. Until that point, there
> > is a very real chance that the PHY <--> MAC connection will remain
> > _down_.
> > 
> > The patch adds up to a _two_ _second_ wait for the PHY <--> MAC
> > connection to come up before aqr107_read_status() will return. This
> > is total nonsense - because waiting here means that the MAC won't
> > get the notification of which interface mode the PHY is expecting
> > to use, therefore the MAC won't configure its PHY facing hardware
> > for that interface mode, and therefore the PHY <--> MAC connection
> > will _not_ _come_ _up_.
> > 
> > You can not wait for the PHY <--> MAC connection to come up in the
> > phylib read_status method. Ever.
> > 
> > This is non-negotiable because it is just totally wrong to do this
> > and leads to pointless two second delays.
> 
> 
> Thanks for the feedback! We will go away, review this and see if we can
> figure out a good/correct way to resolve our ethernet issue.

Which ethernet driver is having a problem?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side

[Jon Hunter]

On 30/07/2024 10:41, Russell King (Oracle) wrote:
> On Tue, Jul 30, 2024 at 10:36:12AM +0100, Jon Hunter wrote:
>>
>> On 29/07/2024 11:47, Russell King (Oracle) wrote:
>>
>> ...
>>
>>>> Apologies for not following up before on this and now that is has been a
>>>> year I am not sure if it is even appropriate to dig this up as opposed to
>>>> starting a new thread completely.
>>>>
>>>> However, I want to resume this conversation because we have found that this
>>>> change does resolve a long-standing issue where we occasionally see our
>>>> ethernet controller fail to get an IP address.
>>>>
>>>> I understand that your objection to the above change is that (per Revanth's
>>>> feedback) this change assumes interface has the link. However, looking at
>>>> the aqr107_read_status() function where this change is made the function has
>>>> the following ...
>>>>
>>>> static int aqr107_read_status(struct phy_device *phydev)
>>>> {
>>>>           int val, ret;
>>>>
>>>>           ret = aqr_read_status(phydev);
>>>>           if (ret)
>>>>                   return ret;
>>>>
>>>>           if (!phydev->link || phydev->autoneg == AUTONEG_DISABLE)
>>>>                   return 0;
>>>>
>>>>
>>>> So my understanding is that if we don't have the link, then the above test
>>>> will return before we attempt to poll the TX ready status. If that is the
>>>> case, then would the change being proposed be OK?
>>>
>>> Here, phydev->link will be the _media_ side link. This is fine - if the
>>> media link is down, there's no point doing anything further. However,
>>> if the link is up, then we need the PHY to update phydev->interface
>>> _and_ report that the link was up (phydev->link is true).
>>>
>>> When that happens, the layers above (e.g. phylib, phylink, MAC driver)
>>> then know that the _media_ side interface has come up, and they also
>>> know the parameters that were negotiated. They also know what interface
>>> mode the PHY is wanting to use.
>>>
>>> At that point, the MAC driver can then reconfigure its PHY facing
>>> interface according to what the PHY is using. Until that point, there
>>> is a very real chance that the PHY <--> MAC connection will remain
>>> _down_.
>>>
>>> The patch adds up to a _two_ _second_ wait for the PHY <--> MAC
>>> connection to come up before aqr107_read_status() will return. This
>>> is total nonsense - because waiting here means that the MAC won't
>>> get the notification of which interface mode the PHY is expecting
>>> to use, therefore the MAC won't configure its PHY facing hardware
>>> for that interface mode, and therefore the PHY <--> MAC connection
>>> will _not_ _come_ _up_.
>>>
>>> You can not wait for the PHY <--> MAC connection to come up in the
>>> phylib read_status method. Ever.
>>>
>>> This is non-negotiable because it is just totally wrong to do this
>>> and leads to pointless two second delays.
>>
>>
>> Thanks for the feedback! We will go away, review this and see if we can
>> figure out a good/correct way to resolve our ethernet issue.
> 
> Which ethernet driver is having a problem?
> 

It is the drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c driver. It 
works most of the time, but on occasion it fails to get a valid IP 
address.

Thanks
Jon
-- 
nvpublic

Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side

[Russell King (Oracle)]

On Tue, Jul 30, 2024 at 11:02:07AM +0100, Jon Hunter wrote:
> 
> On 30/07/2024 10:41, Russell King (Oracle) wrote:
> > On Tue, Jul 30, 2024 at 10:36:12AM +0100, Jon Hunter wrote:
> > > 
> > > On 29/07/2024 11:47, Russell King (Oracle) wrote:
> > > 
> > > ...
> > > 
> > > > > Apologies for not following up before on this and now that is has been a
> > > > > year I am not sure if it is even appropriate to dig this up as opposed to
> > > > > starting a new thread completely.
> > > > > 
> > > > > However, I want to resume this conversation because we have found that this
> > > > > change does resolve a long-standing issue where we occasionally see our
> > > > > ethernet controller fail to get an IP address.
> > > > > 
> > > > > I understand that your objection to the above change is that (per Revanth's
> > > > > feedback) this change assumes interface has the link. However, looking at
> > > > > the aqr107_read_status() function where this change is made the function has
> > > > > the following ...
> > > > > 
> > > > > static int aqr107_read_status(struct phy_device *phydev)
> > > > > {
> > > > >           int val, ret;
> > > > > 
> > > > >           ret = aqr_read_status(phydev);
> > > > >           if (ret)
> > > > >                   return ret;
> > > > > 
> > > > >           if (!phydev->link || phydev->autoneg == AUTONEG_DISABLE)
> > > > >                   return 0;
> > > > > 
> > > > > 
> > > > > So my understanding is that if we don't have the link, then the above test
> > > > > will return before we attempt to poll the TX ready status. If that is the
> > > > > case, then would the change being proposed be OK?
> > > > 
> > > > Here, phydev->link will be the _media_ side link. This is fine - if the
> > > > media link is down, there's no point doing anything further. However,
> > > > if the link is up, then we need the PHY to update phydev->interface
> > > > _and_ report that the link was up (phydev->link is true).
> > > > 
> > > > When that happens, the layers above (e.g. phylib, phylink, MAC driver)
> > > > then know that the _media_ side interface has come up, and they also
> > > > know the parameters that were negotiated. They also know what interface
> > > > mode the PHY is wanting to use.
> > > > 
> > > > At that point, the MAC driver can then reconfigure its PHY facing
> > > > interface according to what the PHY is using. Until that point, there
> > > > is a very real chance that the PHY <--> MAC connection will remain
> > > > _down_.
> > > > 
> > > > The patch adds up to a _two_ _second_ wait for the PHY <--> MAC
> > > > connection to come up before aqr107_read_status() will return. This
> > > > is total nonsense - because waiting here means that the MAC won't
> > > > get the notification of which interface mode the PHY is expecting
> > > > to use, therefore the MAC won't configure its PHY facing hardware
> > > > for that interface mode, and therefore the PHY <--> MAC connection
> > > > will _not_ _come_ _up_.
> > > > 
> > > > You can not wait for the PHY <--> MAC connection to come up in the
> > > > phylib read_status method. Ever.
> > > > 
> > > > This is non-negotiable because it is just totally wrong to do this
> > > > and leads to pointless two second delays.
> > > 
> > > 
> > > Thanks for the feedback! We will go away, review this and see if we can
> > > figure out a good/correct way to resolve our ethernet issue.
> > 
> > Which ethernet driver is having a problem?
> > 
> 
> It is the drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c driver. It works
> most of the time, but on occasion it fails to get a valid IP address.

Hmm. dwmac-tegra.c sets STMMAC_FLAG_SERDES_UP_AFTER_PHY_LINKUP, which
means that the serdes won't be powered up until after the PHY has
indicated that link is up. If the serdes is not powered up, then the
MAC facing interface on the PHY won't come up.

Hence, the code you're adding will, in all probability, merely add a
two second delay to each and every time the PHY is polled for its
status when the PHY indicates that the media link is up until such
time that the stmmac side has processed that the link has come up.

I also note that mgbe_uphy_lane_bringup_serdes_up() polls the link
status on the MAC PCS side, waiting for the link to become ready
on that side.

So, what you have is:

- you bring the interface up. The serdes interface remains powered down.
- phylib starts polling the PHY.
- the PHY indicates the media link is up.
- your new code polls the PHY's MAC facing interface for link up, but
  because the serdes interface is powered down, it ends up timing out
  after two seconds and then proceeds.
- phylib notifies phylink that the PHY has link.
- phylink brings the PCS and MAC side(s) up, calling
  stmmac_mac_link_up().
- stmmac_mac_link_up() calls mgbe_uphy_lane_bringup_serdes_up() which
  then does receive lane calibration (which is likely the reason why
  this is delayed to link-up, so the PHY is giving a valid serdes
  stream for the calibration to use.)
- mgbe_uphy_lane_bringup_serdes_up() enables the data path, and
  clears resets, and then waits for the serdes link with the PHY to
  come up.

While stmmac_mac_link_up() is running, phylib will continue to try to
poll the PHY for its status once every second, and each time it does
if the PHY's MAC facing link reports that it's down, the phylib locks
will be held for _two_ seconds each time. That will mean you won't be
able to bring the interface down until those two seconds time out.

So, I think one needs to go back and properly understand what is going
on to figure out what is going wrong.

You will likely find that inserting a two second delay at the start of
mgbe_uphy_lane_bringup_serdes_up() is just as effective at solving
the issue - although I am not suggesting that would be an acceptable
solution. It would help to confirm that the reasoning is correct.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side

[Jon Hunter]

On 30/07/2024 12:12, Russell King (Oracle) wrote:

...

> Hmm. dwmac-tegra.c sets STMMAC_FLAG_SERDES_UP_AFTER_PHY_LINKUP, which
> means that the serdes won't be powered up until after the PHY has
> indicated that link is up. If the serdes is not powered up, then the
> MAC facing interface on the PHY won't come up.
> 
> Hence, the code you're adding will, in all probability, merely add a
> two second delay to each and every time the PHY is polled for its
> status when the PHY indicates that the media link is up until such
> time that the stmmac side has processed that the link has come up.
> 
> I also note that mgbe_uphy_lane_bringup_serdes_up() polls the link
> status on the MAC PCS side, waiting for the link to become ready
> on that side.
> 
> So, what you have is:
> 
> - you bring the interface up. The serdes interface remains powered down.
> - phylib starts polling the PHY.
> - the PHY indicates the media link is up.
> - your new code polls the PHY's MAC facing interface for link up, but
>    because the serdes interface is powered down, it ends up timing out
>    after two seconds and then proceeds.
> - phylib notifies phylink that the PHY has link.
> - phylink brings the PCS and MAC side(s) up, calling
>    stmmac_mac_link_up().
> - stmmac_mac_link_up() calls mgbe_uphy_lane_bringup_serdes_up() which
>    then does receive lane calibration (which is likely the reason why
>    this is delayed to link-up, so the PHY is giving a valid serdes
>    stream for the calibration to use.)
> - mgbe_uphy_lane_bringup_serdes_up() enables the data path, and
>    clears resets, and then waits for the serdes link with the PHY to
>    come up.
> 
> While stmmac_mac_link_up() is running, phylib will continue to try to
> poll the PHY for its status once every second, and each time it does
> if the PHY's MAC facing link reports that it's down, the phylib locks
> will be held for _two_ seconds each time. That will mean you won't be
> able to bring the interface down until those two seconds time out.
> 
> So, I think one needs to go back and properly understand what is going
> on to figure out what is going wrong.

Yes agree.

> You will likely find that inserting a two second delay at the start of
> mgbe_uphy_lane_bringup_serdes_up() is just as effective at solving
> the issue - although I am not suggesting that would be an acceptable
> solution. It would help to confirm that the reasoning is correct.

I was wondering about something along those lines. We will go back and 
look at this.

Thanks
Jon

-- 
nvpublic

Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side

[Jon Hunter]

On 30/07/2024 13:25, Jon Hunter wrote:
> 
> On 30/07/2024 12:12, Russell King (Oracle) wrote:
> 
> ...
> 
>> Hmm. dwmac-tegra.c sets STMMAC_FLAG_SERDES_UP_AFTER_PHY_LINKUP, which
>> means that the serdes won't be powered up until after the PHY has
>> indicated that link is up. If the serdes is not powered up, then the
>> MAC facing interface on the PHY won't come up.
>>
>> Hence, the code you're adding will, in all probability, merely add a
>> two second delay to each and every time the PHY is polled for its
>> status when the PHY indicates that the media link is up until such
>> time that the stmmac side has processed that the link has come up.
>>
>> I also note that mgbe_uphy_lane_bringup_serdes_up() polls the link
>> status on the MAC PCS side, waiting for the link to become ready
>> on that side.
>>
>> So, what you have is:
>>
>> - you bring the interface up. The serdes interface remains powered down.
>> - phylib starts polling the PHY.
>> - the PHY indicates the media link is up.
>> - your new code polls the PHY's MAC facing interface for link up, but
>>    because the serdes interface is powered down, it ends up timing out
>>    after two seconds and then proceeds.
>> - phylib notifies phylink that the PHY has link.
>> - phylink brings the PCS and MAC side(s) up, calling
>>    stmmac_mac_link_up().
>> - stmmac_mac_link_up() calls mgbe_uphy_lane_bringup_serdes_up() which
>>    then does receive lane calibration (which is likely the reason why
>>    this is delayed to link-up, so the PHY is giving a valid serdes
>>    stream for the calibration to use.)
>> - mgbe_uphy_lane_bringup_serdes_up() enables the data path, and
>>    clears resets, and then waits for the serdes link with the PHY to
>>    come up.
>>
>> While stmmac_mac_link_up() is running, phylib will continue to try to
>> poll the PHY for its status once every second, and each time it does
>> if the PHY's MAC facing link reports that it's down, the phylib locks
>> will be held for _two_ seconds each time. That will mean you won't be
>> able to bring the interface down until those two seconds time out.
>>
>> So, I think one needs to go back and properly understand what is going
>> on to figure out what is going wrong.
> 
> Yes agree.
> 
>> You will likely find that inserting a two second delay at the start of
>> mgbe_uphy_lane_bringup_serdes_up() is just as effective at solving
>> the issue - although I am not suggesting that would be an acceptable
>> solution. It would help to confirm that the reasoning is correct.
> 
> I was wondering about something along those lines. We will go back and 
> look at this.


Just to circle back on this. After reviewing the link bring-up sequence 
in the mgbe_uphy_lane_bringup_serdes_up() function, the Tegra hardware 
team identified some specific sequence and timing requirements we needed 
to correct. Paritosh has posted a fix for this [0] and now we have 
verified that this fixes the issue we were seeing without making this 
change.

Thanks
Jon

[0] 
https://lore.kernel.org/linux-tegra/20240923134410.2111640-1-paritoshd@nvidia.com/

-- 
nvpublic

Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)

[Russell King (Oracle)]

On Wed, Jun 28, 2023 at 06:13:26PM +0530, Revanth Kumar Uppala wrote:
> @@ -109,6 +134,10 @@
>  #define VEND1_GLOBAL_CFG_10M			0x0310
>  #define VEND1_GLOBAL_CFG_100M			0x031b
>  #define VEND1_GLOBAL_CFG_1G			0x031c
> +#define VEND1_GLOBAL_SYS_CONFIG_SGMII   (BIT(0) | BIT(1))
> +#define VEND1_GLOBAL_SYS_CONFIG_AN      BIT(3)
> +#define VEND1_GLOBAL_SYS_CONFIG_XFI     BIT(8)

My understanding is that bits 2:0 are a _bitfield_ and not individual
bits, which contain the following values:

0 - 10GBASE-R (XFI if you really want to call it that)
3 - SGMII
4 - OCSGMII (2.5G)
6 - 5GBASE-R (XFI5G if you really want to call it that)

Bit 3 controls whether the SGMII control word is used, and this is the
only applicable mode.

Bit 8 is already defined - it's part of the rate adaption mode field,
see VEND1_GLOBAL_CFG_RATE_ADAPT and VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE.

These bits apply to all the VEND1_GLOBAL_CFG_* registers, so these
should be defined after the last register (0x031f).

> +static int aqr113c_wol_enable(struct phy_device *phydev)
> +{
> +	struct aqr107_priv *priv = phydev->priv;
> +	u16 val;
> +	int ret;
> +
> +	/* Disables all advertised speeds except for the WoL
> +	 * speed (100BASE-TX FD or 1000BASE-T)
> +	 * This is set as per the APP note from Marvel
> +	 */
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
> +			       MDIO_AN_LD_LOOP_TIMING_ABILITY);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = (ret & MDIO_AN_VEND_MASK) |
> +	      (MDIO_AN_VEND_PROV_AQRATE_DWN_SHFT_CAP | MDIO_AN_VEND_PROV_1000BASET_FULL);
> +	ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Enable the magic frame and wake up frame detection for the PHY */
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_RSI1_CTRL6,
> +			       MDIO_C22EXT_RSI_WAKE_UP_FRAME_DETECTION);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_RSI1_CTRL7,
> +			       MDIO_C22EXT_RSI_MAGIC_PKT_FRAME_DETECTION);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set the WoL enable bit */
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RSVD_VEND_PROV1,
> +			       MDIO_MMD_AN_WOL_ENABLE);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set the WoL INT_N trigger bit */
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_RSI1_CTRL8,
> +			       MDIO_C22EXT_RSI_WOL_FCS_MONITOR_MODE);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Enable Interrupt INT_N Generation at pin level */
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_SGMII_TX_INT_MASK1,
> +			       MDIO_C22EXT_SGMII0_WAKE_UP_FRAME_MASK |
> +			       MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_INT_STD_MASK,
> +			       VEND1_GLOBAL_INT_STD_MASK_ALL);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_INT_VEND_MASK,
> +			       VEND1_GLOBAL_INT_VEND_MASK_GBE);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set the system interface to SGMII */
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> +			    VEND1_GLOBAL_CFG_100M, VEND1_GLOBAL_SYS_CONFIG_SGMII |
> +			    VEND1_GLOBAL_SYS_CONFIG_AN);

How do you know that SGMII should be used for 100M?

> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> +			    VEND1_GLOBAL_CFG_1G, VEND1_GLOBAL_SYS_CONFIG_SGMII |
> +			    VEND1_GLOBAL_SYS_CONFIG_AN);

How do you know that SGMII should be used for 1G?

Doesn't this depend on the configuration of the host MAC and the
capabilities of it? If the host MAC only supports 10G, doesn't this
break stuff?

> +	if (ret < 0)
> +		return ret;
> +
> +	/* restart auto-negotiation */
> +	genphy_c45_restart_aneg(phydev);
> +	priv->wol_status = 1;
> +
> +	return 0;
> +}
> +
> +static int aqr113c_wol_disable(struct phy_device *phydev)
> +{
> +	struct aqr107_priv *priv = phydev->priv;
> +	int ret;
> +
> +	/* Disable the WoL enable bit */
> +	ret = phy_clear_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RSVD_VEND_PROV1,
> +				 MDIO_MMD_AN_WOL_ENABLE);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Restore the SERDES/System Interface back to the XFI mode */
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> +			    VEND1_GLOBAL_CFG_100M, VEND1_GLOBAL_SYS_CONFIG_XFI);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> +			    VEND1_GLOBAL_CFG_1G, VEND1_GLOBAL_SYS_CONFIG_XFI);
> +	if (ret < 0)
> +		return ret;

Conversely, how do you know that configuring 100M/1G to use 10GBASE-R on
the host interface is how the PHY was provisioned in firmware? I think
at the very least, you should be leaving these settings alone until you
know that the system is entering a low power mode, saving the settings,
and restoring them when you wake up.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)

[Revanth Kumar Uppala]

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Wednesday, June 28, 2023 7:13 PM
> To: Revanth Kumar Uppala <ruppala@nvidia.com>
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org; linux-
> tegra@vger.kernel.org; Narayan Reddy <narayanr@nvidia.com>
> Subject: Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Jun 28, 2023 at 06:13:26PM +0530, Revanth Kumar Uppala wrote:
> > @@ -109,6 +134,10 @@
> >  #define VEND1_GLOBAL_CFG_10M                 0x0310
> >  #define VEND1_GLOBAL_CFG_100M                        0x031b
> >  #define VEND1_GLOBAL_CFG_1G                  0x031c
> > +#define VEND1_GLOBAL_SYS_CONFIG_SGMII   (BIT(0) | BIT(1))
> > +#define VEND1_GLOBAL_SYS_CONFIG_AN      BIT(3)
> > +#define VEND1_GLOBAL_SYS_CONFIG_XFI     BIT(8)
> 
> My understanding is that bits 2:0 are a _bitfield_ and not individual bits, which
> contain the following values:
I will define bitfield instead of defining individual bits in V2 series
> 
> 0 - 10GBASE-R (XFI if you really want to call it that)
> 3 - SGMII
> 4 - OCSGMII (2.5G)
> 6 - 5GBASE-R (XFI5G if you really want to call it that)
> 
> Bit 3 controls whether the SGMII control word is used, and this is the only
> applicable mode.
> 
> Bit 8 is already defined - it's part of the rate adaption mode field, see
> VEND1_GLOBAL_CFG_RATE_ADAPT and
> VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE.
Sure, I will use above mentioned macros and will set the register values with help of FIELD_PREP in V2 series
> 
> These bits apply to all the VEND1_GLOBAL_CFG_* registers, so these should be
> defined after the last register (0x031f).
Will take care of this.
> 
> > +static int aqr113c_wol_enable(struct phy_device *phydev) {
> > +     struct aqr107_priv *priv = phydev->priv;
> > +     u16 val;
> > +     int ret;
> > +
> > +     /* Disables all advertised speeds except for the WoL
> > +      * speed (100BASE-TX FD or 1000BASE-T)
> > +      * This is set as per the APP note from Marvel
> > +      */
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN,
> MDIO_AN_10GBT_CTRL,
> > +                            MDIO_AN_LD_LOOP_TIMING_ABILITY);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     val = (ret & MDIO_AN_VEND_MASK) |
> > +           (MDIO_AN_VEND_PROV_AQRATE_DWN_SHFT_CAP |
> MDIO_AN_VEND_PROV_1000BASET_FULL);
> > +     ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV,
> val);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* Enable the magic frame and wake up frame detection for the PHY */
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT,
> MDIO_C22EXT_GBE_PHY_RSI1_CTRL6,
> > +                            MDIO_C22EXT_RSI_WAKE_UP_FRAME_DETECTION);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT,
> MDIO_C22EXT_GBE_PHY_RSI1_CTRL7,
> > +                            MDIO_C22EXT_RSI_MAGIC_PKT_FRAME_DETECTION);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* Set the WoL enable bit */
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN,
> MDIO_AN_RSVD_VEND_PROV1,
> > +                            MDIO_MMD_AN_WOL_ENABLE);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* Set the WoL INT_N trigger bit */
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT,
> MDIO_C22EXT_GBE_PHY_RSI1_CTRL8,
> > +                            MDIO_C22EXT_RSI_WOL_FCS_MONITOR_MODE);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* Enable Interrupt INT_N Generation at pin level */
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT,
> MDIO_C22EXT_GBE_PHY_SGMII_TX_INT_MASK1,
> > +                            MDIO_C22EXT_SGMII0_WAKE_UP_FRAME_MASK |
> > +                            MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
> VEND1_GLOBAL_INT_STD_MASK,
> > +                            VEND1_GLOBAL_INT_STD_MASK_ALL);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
> VEND1_GLOBAL_INT_VEND_MASK,
> > +                            VEND1_GLOBAL_INT_VEND_MASK_GBE);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* Set the system interface to SGMII */
> > +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > +                         VEND1_GLOBAL_CFG_100M,
> VEND1_GLOBAL_SYS_CONFIG_SGMII |
> > +                         VEND1_GLOBAL_SYS_CONFIG_AN);
> 
> How do you know that SGMII should be used for 100M?
> 
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > +                         VEND1_GLOBAL_CFG_1G,
> VEND1_GLOBAL_SYS_CONFIG_SGMII |
> > +                         VEND1_GLOBAL_SYS_CONFIG_AN);
> 
> How do you know that SGMII should be used for 1G?
> 
> Doesn't this depend on the configuration of the host MAC and the capabilities of
> it? If the host MAC only supports 10G, doesn't this break stuff?
> 
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* restart auto-negotiation */
> > +     genphy_c45_restart_aneg(phydev);
> > +     priv->wol_status = 1;
> > +
> > +     return 0;
> > +}
> > +
> > +static int aqr113c_wol_disable(struct phy_device *phydev) {
> > +     struct aqr107_priv *priv = phydev->priv;
> > +     int ret;
> > +
> > +     /* Disable the WoL enable bit */
> > +     ret = phy_clear_bits_mmd(phydev, MDIO_MMD_AN,
> MDIO_AN_RSVD_VEND_PROV1,
> > +                              MDIO_MMD_AN_WOL_ENABLE);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* Restore the SERDES/System Interface back to the XFI mode */
> > +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > +                         VEND1_GLOBAL_CFG_100M,
> VEND1_GLOBAL_SYS_CONFIG_XFI);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > +                         VEND1_GLOBAL_CFG_1G, VEND1_GLOBAL_SYS_CONFIG_XFI);
> > +     if (ret < 0)
> > +             return ret;
> 
> Conversely, how do you know that configuring 100M/1G to use 10GBASE-R on
> the host interface is how the PHY was provisioned in firmware? I think at the
> very least, you should be leaving these settings alone until you know that the
> system is entering a low power mode, saving the settings, and restoring them
> when you wake up.

Regarding all the above comments ,
We are following the app note AN-N4209 by Marvell semiconductors for enabling and disabling of WOL.
Below are the steps in brief as mentioned in app note
For enabling the WOL,
1. Set the MAC address for the PHY. Make sure the MAC address is a legal address 
2. Disables all advertised speeds except for the WoL speed (100BASE-TX FD or 1000BASE-T)
3. Enable the magic frame and wake up frame detection for the PHY
4. Set the WoL enable bit
5. Set the WoL INT_N trigger bit
6. Optional: Enable Interrupt INT_N Generation at pin level
7. Set the system interface to SGMII by writing into below registers
MDIO Write 1e.31b = 0x0b (Sets SGMII for 100M) 
MDIO Write 1e.31c = 0x0b (Sets SGMII for 1G)
8. Perform a link re-negotiation/auto-negotiation
9. The WoL status bit should be 1 which indicates that the WoL is active. The PHY now is in sleep mode

For remote WAKEUP via magic packet,
1.MAC detects INT from PHY and confirm Wake request.
2. Disable the WoL mode by unsetting the WoL enable bit.
3. Restore the SERDES/System Interface back to the original mode before WoL was initialized using SGMII mode i.e; back to XFI mode.
MDIO write 1e.31b = 0x100 (Reverts the 100M setup to original mode)
MDIO write 1e.31c = 0x100 (Reverts the 1G setup to original mode
4. Perform an auto-negotiation for the register values to take effect and establish a proper link

Thanks,
Revanth Uppala 
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)

[Russell King (Oracle)]

On Mon, Jul 24, 2023 at 11:29:39AM +0000, Revanth Kumar Uppala wrote:
> 
> 
> > -----Original Message-----
> > From: Russell King <linux@armlinux.org.uk>
> > Sent: Wednesday, June 28, 2023 7:13 PM
> > To: Revanth Kumar Uppala <ruppala@nvidia.com>
> > Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org; linux-
> > tegra@vger.kernel.org; Narayan Reddy <narayanr@nvidia.com>
> > Subject: Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, Jun 28, 2023 at 06:13:26PM +0530, Revanth Kumar Uppala wrote:
> > > @@ -109,6 +134,10 @@
> > >  #define VEND1_GLOBAL_CFG_10M                 0x0310
> > >  #define VEND1_GLOBAL_CFG_100M                        0x031b
> > >  #define VEND1_GLOBAL_CFG_1G                  0x031c
> > > +#define VEND1_GLOBAL_SYS_CONFIG_SGMII   (BIT(0) | BIT(1))
> > > +#define VEND1_GLOBAL_SYS_CONFIG_AN      BIT(3)
> > > +#define VEND1_GLOBAL_SYS_CONFIG_XFI     BIT(8)
> > 
> > My understanding is that bits 2:0 are a _bitfield_ and not individual bits, which
> > contain the following values:
> I will define bitfield instead of defining individual bits in V2 series
> > 
> > 0 - 10GBASE-R (XFI if you really want to call it that)
> > 3 - SGMII
> > 4 - OCSGMII (2.5G)
> > 6 - 5GBASE-R (XFI5G if you really want to call it that)
> > 
> > Bit 3 controls whether the SGMII control word is used, and this is the only
> > applicable mode.
> > 
> > Bit 8 is already defined - it's part of the rate adaption mode field, see
> > VEND1_GLOBAL_CFG_RATE_ADAPT and
> > VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE.
> Sure, I will use above mentioned macros and will set the register values with help of FIELD_PREP in V2 series
> > 
> > These bits apply to all the VEND1_GLOBAL_CFG_* registers, so these should be
> > defined after the last register (0x031f).
> Will take care of this.
> > 
> > > +static int aqr113c_wol_enable(struct phy_device *phydev) {
> > > +     struct aqr107_priv *priv = phydev->priv;
> > > +     u16 val;
> > > +     int ret;
> > > +
> > > +     /* Disables all advertised speeds except for the WoL
> > > +      * speed (100BASE-TX FD or 1000BASE-T)
> > > +      * This is set as per the APP note from Marvel
> > > +      */
> > > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN,
> > MDIO_AN_10GBT_CTRL,
> > > +                            MDIO_AN_LD_LOOP_TIMING_ABILITY);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     val = (ret & MDIO_AN_VEND_MASK) |
> > > +           (MDIO_AN_VEND_PROV_AQRATE_DWN_SHFT_CAP |
> > MDIO_AN_VEND_PROV_1000BASET_FULL);
> > > +     ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV,
> > val);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     /* Enable the magic frame and wake up frame detection for the PHY */
> > > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT,
> > MDIO_C22EXT_GBE_PHY_RSI1_CTRL6,
> > > +                            MDIO_C22EXT_RSI_WAKE_UP_FRAME_DETECTION);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT,
> > MDIO_C22EXT_GBE_PHY_RSI1_CTRL7,
> > > +                            MDIO_C22EXT_RSI_MAGIC_PKT_FRAME_DETECTION);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     /* Set the WoL enable bit */
> > > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN,
> > MDIO_AN_RSVD_VEND_PROV1,
> > > +                            MDIO_MMD_AN_WOL_ENABLE);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     /* Set the WoL INT_N trigger bit */
> > > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT,
> > MDIO_C22EXT_GBE_PHY_RSI1_CTRL8,
> > > +                            MDIO_C22EXT_RSI_WOL_FCS_MONITOR_MODE);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     /* Enable Interrupt INT_N Generation at pin level */
> > > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT,
> > MDIO_C22EXT_GBE_PHY_SGMII_TX_INT_MASK1,
> > > +                            MDIO_C22EXT_SGMII0_WAKE_UP_FRAME_MASK |
> > > +                            MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
> > VEND1_GLOBAL_INT_STD_MASK,
> > > +                            VEND1_GLOBAL_INT_STD_MASK_ALL);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
> > VEND1_GLOBAL_INT_VEND_MASK,
> > > +                            VEND1_GLOBAL_INT_VEND_MASK_GBE);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     /* Set the system interface to SGMII */
> > > +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > > +                         VEND1_GLOBAL_CFG_100M,
> > VEND1_GLOBAL_SYS_CONFIG_SGMII |
> > > +                         VEND1_GLOBAL_SYS_CONFIG_AN);
> > 
> > How do you know that SGMII should be used for 100M?
> > 
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > > +                         VEND1_GLOBAL_CFG_1G,
> > VEND1_GLOBAL_SYS_CONFIG_SGMII |
> > > +                         VEND1_GLOBAL_SYS_CONFIG_AN);
> > 
> > How do you know that SGMII should be used for 1G?
> > 
> > Doesn't this depend on the configuration of the host MAC and the capabilities of
> > it? If the host MAC only supports 10G, doesn't this break stuff?
> > 
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     /* restart auto-negotiation */
> > > +     genphy_c45_restart_aneg(phydev);
> > > +     priv->wol_status = 1;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int aqr113c_wol_disable(struct phy_device *phydev) {
> > > +     struct aqr107_priv *priv = phydev->priv;
> > > +     int ret;
> > > +
> > > +     /* Disable the WoL enable bit */
> > > +     ret = phy_clear_bits_mmd(phydev, MDIO_MMD_AN,
> > MDIO_AN_RSVD_VEND_PROV1,
> > > +                              MDIO_MMD_AN_WOL_ENABLE);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     /* Restore the SERDES/System Interface back to the XFI mode */
> > > +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > > +                         VEND1_GLOBAL_CFG_100M,
> > VEND1_GLOBAL_SYS_CONFIG_XFI);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > > +                         VEND1_GLOBAL_CFG_1G, VEND1_GLOBAL_SYS_CONFIG_XFI);
> > > +     if (ret < 0)
> > > +             return ret;
> > 
> > Conversely, how do you know that configuring 100M/1G to use 10GBASE-R on
> > the host interface is how the PHY was provisioned in firmware? I think at the
> > very least, you should be leaving these settings alone until you know that the
> > system is entering a low power mode, saving the settings, and restoring them
> > when you wake up.
> 
> Regarding all the above comments ,
> We are following the app note AN-N4209 by Marvell semiconductors for enabling and disabling of WOL.
> Below are the steps in brief as mentioned in app note

So basically what I gather is that the answer to "how do you know that
configuring 100M/1G to use 10GBASE-R the host interface is how the PHY
was provisioned in firmware?" is that you don't know, and you're just
blindly following what someone's thrown into an application note but
haven't thought enough about it.

> For remote WAKEUP via magic packet,
> 1.MAC detects INT from PHY and confirm Wake request.
> 2. Disable the WoL mode by unsetting the WoL enable bit.
> 3. Restore the SERDES/System Interface back to the original mode before WoL was initialized using SGMII mode i.e; back to XFI mode.
> MDIO write 1e.31b = 0x100 (Reverts the 100M setup to original mode)
> MDIO write 1e.31c = 0x100 (Reverts the 1G setup to original mode

I think you have misunderstood step 3. "Restore ... back to the
original mode" when interpreting the application note.

Since these registers are set during the provisioning on the PHY,
there is *no* guarantee that they were originally in XFI mode before
WoL was enabled. Hence, in order to "restore" their state, you need
to "save" their state at some point, and it would probably be a good
idea to do that when:

1) the PHY is probed to get the power-up status.
2) update the saved registers whenever the driver reconfigures the PHY
   for a different interface mode (I don't think it does this.)
3) use this saved information to restore these registers when WoL is
   disabled, _or_ when the PHY device is detached from the PHY driver
   i.o.w. when the ->remove method is called, so that if the driver
   re-probes, it can get at the _original_ information.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)

[Revanth Kumar Uppala]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, June 28, 2023 7:48 PM
> To: Revanth Kumar Uppala <ruppala@nvidia.com>
> Cc: linux@armlinux.org.uk; hkallweit1@gmail.com; netdev@vger.kernel.org;
> linux-tegra@vger.kernel.org; Narayan Reddy <narayanr@nvidia.com>
> Subject: Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)
> 
> External email: Use caution opening links or attachments
> 
> 
> > +static int aqr113c_wol_enable(struct phy_device *phydev) {
> > +     struct aqr107_priv *priv = phydev->priv;
> > +     u16 val;
> > +     int ret;
> > +
> > +     /* Disables all advertised speeds except for the WoL
> > +      * speed (100BASE-TX FD or 1000BASE-T)
> > +      * This is set as per the APP note from Marvel
> > +      */
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN,
> MDIO_AN_10GBT_CTRL,
> > +                            MDIO_AN_LD_LOOP_TIMING_ABILITY);
> > +     if (ret < 0)
> > +             return ret;
> 
> Please take a look at phylink_speed_down() and phylink_speed_up(). Assuming
> the PHY is not reporting it can do 10Full and 10Half, it should end up in
> 100BaseFull. Assuming the link partner can do 100BaseFull....
> 
> Russell points out you are making a lot of assumptions about the system side
> link. Ideally, you want to leave that to the PHY. Once the auto-neg at the lower
> speed has completed, it might change the system side link, e.g. to SGMII and the
> normal machinery should pass that onto the MAC, so it can follow. I would not
> force anything.
As per my reply to Russel's comment, we are following the app note AN-N4209 by Marvell semiconductors for enabling and disabling of WOL and above logic is part of the same app note.
> 
> > @@ -619,6 +784,31 @@ static int aqr107_config_init(struct phy_device
> *phydev)
> >       if (ret < 0)
> >               return ret;
> >
> > +     /* Configure Magic packet frame pattern (MAC address) */
> > +     ret = phy_write_mmd(phydev, MDIO_MMD_C22EXT,
> MDIO_C22EXT_MAGIC_PKT_PATTERN_0_2_15,
> > +                         phydev->attached_dev->dev_addr[0] |
> > +                         (phydev->attached_dev->dev_addr[1] << 8));
> 
> I think most PHY drivers do this as part of enabling WOL. Doing it in
> aqr107_config_init() is early, is the MAC address stable yet? The user could
> change it. It could still be changed after wol is enabled, but at least the user has
> a clear point in time when WoL configuration happens.
Yes, your assumption is correct.
Will move this logic to aqr113c_wol_enable() function to take care of above scenario.
Thanks,
Revanth Uppala

> 
> > +static void aqr113c_get_wol(struct phy_device *phydev, struct
> > +ethtool_wolinfo *wol) {
> > +     int val;
> > +
> > +     val = phy_read_mmd(phydev, MDIO_MMD_AN,
> MDIO_AN_RSVD_VEND_STATUS3);
> > +     if (val < 0)
> > +             return;
> > +
> > +     wol->supported = WAKE_MAGIC;
> > +     if (val & 0x1)
> > +             wol->wolopts = WAKE_MAGIC;
> 
> WoL seems to be tried to interrupts. So maybe you should actually check an
> interrupt is available? This is not going to work if the PHY is being polled. It does
> however get a bit messy, some boards might connect the 'interrupt' pin to PMIC.
> So there is not a true interrupt, but the PMIC can turn the power back on.
> 
>     Andrew

Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY

[Russell King (Oracle)]

Final comments on the series, posted here because there's no cover
message.

1. The series should include a cover message summarising the overall
   purpose of the series is, giving an overview of the changes, and a
   diffstat for the whole series.

2. The subject line should contain the tree that the patches are
   targetting, e.g. "[PATCH net-next 0/4] Add support for WoL to Aquantia
   PHY driver".

3. The tree should be identified in each patch mailed out.

4. The patches should be threaded to the cover message.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE

[Revanth Kumar Uppala]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, June 28, 2023 7:24 PM
> To: Revanth Kumar Uppala <ruppala@nvidia.com>
> Cc: linux@armlinux.org.uk; hkallweit1@gmail.com; netdev@vger.kernel.org;
> linux-tegra@vger.kernel.org; Narayan Reddy <narayanr@nvidia.com>
> Subject: Re: [PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Jun 28, 2023 at 06:13:24PM +0530, Revanth Kumar Uppala wrote:
> > Enable MAC controlled energy efficient ethernet (EEE) so that MAC can
> > keep the PHY in EEE sleep mode when link utilization is low to reduce
> > energy consumption.
> 
> This needs more explanation. Is this 'SmartEEE', in that the PHY is doing EEE
> without the SoC MAC being involved?
No, this is not Smart EEE.
> 
> Ideally, you should only do SmartEEE, if the SoC MAC is dumb and does not have
> EEE itself. I guess if you are doing rate adaptation, or MACSEC in the PHY, then
> you might be forced to use SmartEEE since the SoC MAC is somewhat decoupled
> from the PHY.
> 
> At the moment, we don't have a good story for SmartEEE. It should be
> configured in the same way as normal EEE, ethtool --set-eee etc. I've got a
> rewrite of normal EEE in the works. Once that is merged i hope SmartEEE will be
> next.
"ethtool --set-eee" is a dynamic way of enabling normal EEE and here we are doing the same normal EEE but configuring it by default in aqr107_config_init() instead of doing it dynamically.
So, is there any concern for this?
Thanks,
Revanth Uppala
> 
>     Andrew

Re: [PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE

[Russell King (Oracle)]

On Mon, Jul 24, 2023 at 11:29:28AM +0000, Revanth Kumar Uppala wrote:
> > Ideally, you should only do SmartEEE, if the SoC MAC is dumb and does not have
> > EEE itself. I guess if you are doing rate adaptation, or MACSEC in the PHY, then
> > you might be forced to use SmartEEE since the SoC MAC is somewhat decoupled
> > from the PHY.
> > 
> > At the moment, we don't have a good story for SmartEEE. It should be
> > configured in the same way as normal EEE, ethtool --set-eee etc. I've got a
> > rewrite of normal EEE in the works. Once that is merged i hope SmartEEE will be
> > next.
> "ethtool --set-eee" is a dynamic way of enabling normal EEE and here we are doing the same normal EEE but configuring it by default in aqr107_config_init() instead of doing it dynamically.

So, setting the MAC_CNTRL_EEE bits is just enabling the standard IEEE
paths in the PHY to allow the IEEE defined EEE architecture to work?

If that's all its doing, I wonder why they aren't set by default...
seems rather strange.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!