[PATCH, net] phy: aquantia: Fix AN when higher speeds than 1G are not advertised

[PATCH, net] phy: aquantia: Fix AN when higher speeds than 1G are not advertised

[Claudiu Manoil]

Even when the eth port is resticted to work with speeds not higher than 1G,
and so the eth driver is requesting the phy (via phylink) to advertise up
to 1000BASET support, the aquantia phy device is still advertising for 2.5G
and 5G speeds.
Clear these advertising defaults when requested.

Cc: Ondrej Spacek <ondrej.spacek@nxp.com>
Fixes: 09c4c57f7bc41 ("net: phy: aquantia: add support for auto-negotiation configuration")
Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
 drivers/net/phy/aquantia_main.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index a8db1a19011b..c7047f5d7a9b 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -34,6 +34,8 @@
 #define MDIO_AN_VEND_PROV			0xc400
 #define MDIO_AN_VEND_PROV_1000BASET_FULL	BIT(15)
 #define MDIO_AN_VEND_PROV_1000BASET_HALF	BIT(14)
+#define MDIO_AN_VEND_PROV_5000BASET_FULL	BIT(11)
+#define MDIO_AN_VEND_PROV_2500BASET_FULL	BIT(10)
 #define MDIO_AN_VEND_PROV_DOWNSHIFT_EN		BIT(4)
 #define MDIO_AN_VEND_PROV_DOWNSHIFT_MASK	GENMASK(3, 0)
 #define MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT	4
@@ -231,9 +233,20 @@ static int aqr_config_aneg(struct phy_device *phydev)
 			      phydev->advertising))
 		reg |= MDIO_AN_VEND_PROV_1000BASET_HALF;
 
+	/* Handle the case when the 2.5G and 5G speeds are not advertised */
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
+			      phydev->advertising))
+		reg |= MDIO_AN_VEND_PROV_2500BASET_FULL;
+
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+			      phydev->advertising))
+		reg |= MDIO_AN_VEND_PROV_5000BASET_FULL;
+
 	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV,
 				     MDIO_AN_VEND_PROV_1000BASET_HALF |
-				     MDIO_AN_VEND_PROV_1000BASET_FULL, reg);
+				     MDIO_AN_VEND_PROV_1000BASET_FULL |
+				     MDIO_AN_VEND_PROV_2500BASET_FULL |
+				     MDIO_AN_VEND_PROV_5000BASET_FULL, reg);
 	if (ret < 0)
 		return ret;
 	if (ret > 0)
-- 
2.25.1

Re: [PATCH, net] phy: aquantia: Fix AN when higher speeds than 1G are not advertised

[Andrew Lunn]

On Fri, Jun 10, 2022 at 11:40:37AM +0300, Claudiu Manoil wrote:
> Even when the eth port is resticted to work with speeds not higher than 1G,
> and so the eth driver is requesting the phy (via phylink) to advertise up
> to 1000BASET support, the aquantia phy device is still advertising for 2.5G
> and 5G speeds.
> Clear these advertising defaults when requested.

Hi Claudiu

genphy_c45_an_config_aneg(phydev) is called in aqr_config_aneg, which
should set/clear MDIO_AN_10GBT_CTRL_ADV5G and
MDIO_AN_10GBT_CTRL_ADV2_5G. Does the aQuantia PHY not have these bits?

	Andrew

RE: [PATCH, net] phy: aquantia: Fix AN when higher speeds than 1G are not advertised

[Claudiu Manoil]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Saturday, June 11, 2022 6:00 PM
[...]
> Subject: Re: [PATCH, net] phy: aquantia: Fix AN when higher speeds than 1G are
> not advertised
> 
> On Fri, Jun 10, 2022 at 11:40:37AM +0300, Claudiu Manoil wrote:
> > Even when the eth port is resticted to work with speeds not higher than 1G,
> > and so the eth driver is requesting the phy (via phylink) to advertise up
> > to 1000BASET support, the aquantia phy device is still advertising for 2.5G
> > and 5G speeds.
> > Clear these advertising defaults when requested.
> 
> Hi Claudiu
> 
> genphy_c45_an_config_aneg(phydev) is called in aqr_config_aneg, which
> should set/clear MDIO_AN_10GBT_CTRL_ADV5G and
> MDIO_AN_10GBT_CTRL_ADV2_5G. Does the aQuantia PHY not have these bits?
> 

Hi Andrew,

Apparently it's not enough to clear the 7.20h register (aka MDIO_AN_10GBT_CTRL)
to stop AQR advertising for higher speeds, otherwise I wouldn't have bothered with
the patch.

We have AQR113c and AQR107 phys on 3 board types, 2 firmware versions for the phys,
but for every instance the AQR phys auto-negotiate at 2.5G when phydev->advertising
is set to 1G, or even 5G when phydev->advertising is set to 2.5G (for the port that supports
2.5G).

Do you have any board with any AQR phy (I think they are called AQrate Gen3 PHYs)?
Have you tried to restrict phydev->advertising to 1G and connect to a link partner that
has 2.5G capability?
At least in our case the link is always resolved to 2.5G unless we set the 7.C400h register
as shown in this patch.

Thanks.
Claudiu  

Re: [PATCH, net] phy: aquantia: Fix AN when higher speeds than 1G are not advertised

[Andrew Lunn]

On Sat, Jun 11, 2022 at 06:13:12PM +0000, Claudiu Manoil wrote:
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Saturday, June 11, 2022 6:00 PM
> [...]
> > Subject: Re: [PATCH, net] phy: aquantia: Fix AN when higher speeds than 1G are
> > not advertised
> > 
> > On Fri, Jun 10, 2022 at 11:40:37AM +0300, Claudiu Manoil wrote:
> > > Even when the eth port is resticted to work with speeds not higher than 1G,
> > > and so the eth driver is requesting the phy (via phylink) to advertise up
> > > to 1000BASET support, the aquantia phy device is still advertising for 2.5G
> > > and 5G speeds.
> > > Clear these advertising defaults when requested.
> > 
> > Hi Claudiu
> > 
> > genphy_c45_an_config_aneg(phydev) is called in aqr_config_aneg, which
> > should set/clear MDIO_AN_10GBT_CTRL_ADV5G and
> > MDIO_AN_10GBT_CTRL_ADV2_5G. Does the aQuantia PHY not have these bits?
> > 
> 
> Hi Andrew,
> 
> Apparently it's not enough to clear the 7.20h register (aka MDIO_AN_10GBT_CTRL)
> to stop AQR advertising for higher speeds, otherwise I wouldn't have bothered with
> the patch.

I'm just trying to ensure we are not papering over a crack. I wanted
to eliminate the possibility of a bug in that code which is changing
7.20h. If the datasheet for the aquantia states those bits are not
used, then this patch is fine. If on the other hand the datasheet says
7.20 can be used to change the advertisement, we should investigate
further what is really going on.

	Andrew

RE: [PATCH, net] phy: aquantia: Fix AN when higher speeds than 1G are not advertised

[Claudiu Manoil]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Sunday, June 12, 2022 2:21 AM
> To: Claudiu Manoil <claudiu.manoil@nxp.com>
[...]
> Subject: Re: [PATCH, net] phy: aquantia: Fix AN when higher speeds than 1G are
> not advertised
> 
> On Sat, Jun 11, 2022 at 06:13:12PM +0000, Claudiu Manoil wrote:
> > > -----Original Message-----
> > > From: Andrew Lunn <andrew@lunn.ch>
> > > Sent: Saturday, June 11, 2022 6:00 PM
> > [...]
> > > Subject: Re: [PATCH, net] phy: aquantia: Fix AN when higher speeds than 1G are
> > > not advertised
> > >
> > > On Fri, Jun 10, 2022 at 11:40:37AM +0300, Claudiu Manoil wrote:
> > > > Even when the eth port is resticted to work with speeds not higher than 1G,
> > > > and so the eth driver is requesting the phy (via phylink) to advertise up
> > > > to 1000BASET support, the aquantia phy device is still advertising for 2.5G
> > > > and 5G speeds.
> > > > Clear these advertising defaults when requested.
> > >
> > > Hi Claudiu
> > >
> > > genphy_c45_an_config_aneg(phydev) is called in aqr_config_aneg, which
> > > should set/clear MDIO_AN_10GBT_CTRL_ADV5G and
> > > MDIO_AN_10GBT_CTRL_ADV2_5G. Does the aQuantia PHY not have these bits?
> > >
> >
> > Hi Andrew,
> >
> > Apparently it's not enough to clear the 7.20h register (aka MDIO_AN_10GBT_CTRL)
> > to stop AQR advertising for higher speeds, otherwise I wouldn't have bothered with
> > the patch.
> 
> I'm just trying to ensure we are not papering over a crack. I wanted
> to eliminate the possibility of a bug in that code which is changing
> 7.20h. If the datasheet for the aquantia states those bits are not
> used, then this patch is fine. If on the other hand the datasheet says
> 7.20 can be used to change the advertisement, we should investigate
> further what is really going on.
> 

I understand the situation is not ideal. What I could do is to provide some
logs showing that writing the correct configuration to 7.20h does not yield
the desired result, to have some sort of detailed evidence about the issue.
But I cannot do that until Tuesday at the earliest.
As for documentation, there's an App Note about configuring advertising
via the vendor specific 0xC400 reg but I don't think it's public. Not sure if
we can use that.
Meanwhile, it would be great if you or someone else could confirm the
issue on a different platform.

Thanks.

Re: [PATCH, net] phy: aquantia: Fix AN when higher speeds than 1G are not advertised

[Andrew Lunn]

> I understand the situation is not ideal. What I could do is to provide some
> logs showing that writing the correct configuration to 7.20h does not yield
> the desired result, to have some sort of detailed evidence about the issue.
> But I cannot do that until Tuesday at the earliest.
> As for documentation, there's an App Note about configuring advertising
> via the vendor specific 0xC400 reg but I don't think it's public. Not sure if
> we can use that.
> Meanwhile, it would be great if you or someone else could confirm the
> issue on a different platform.

I don't have any boards with these PHYs.

If there is a vendor document saying it has to be configured via
vendor registers, thats enough for me. But should the generic code be
removed, are those bits documented as reserved?

	 Andrew

RE: [PATCH, net] phy: aquantia: Fix AN when higher speeds than 1G are not advertised

[Claudiu Manoil]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Monday, June 13, 2022 12:25 AM
> To: Claudiu Manoil <claudiu.manoil@nxp.com>
[...]
> Subject: Re: [PATCH, net] phy: aquantia: Fix AN when higher speeds than 1G
> are not advertised
> 
> > I understand the situation is not ideal. What I could do is to provide some
> > logs showing that writing the correct configuration to 7.20h does not yield
> > the desired result, to have some sort of detailed evidence about the issue.
> > But I cannot do that until Tuesday at the earliest.
> > As for documentation, there's an App Note about configuring advertising
> > via the vendor specific 0xC400 reg but I don't think it's public. Not sure if
> > we can use that.
> > Meanwhile, it would be great if you or someone else could confirm the
> > issue on a different platform.
> 
> I don't have any boards with these PHYs.
> 
> If there is a vendor document saying it has to be configured via
> vendor registers, thats enough for me. But should the generic code be
> removed, are those bits documented as reserved?
> 

Hi Andrew,

The generic code sets/clears the MDIO_AN_10GBT_CTRL_ADV2_5G,
MDIO_AN_10GBT_CTRL_ADV5G, MDIO_AN_10GBT_CTRL_ADV10G bits
in reg 7.20h correctly, as it should, if you're asking me. The App Note also
mentions that both registers, 7.20 and 7.c400, must be configured.
We're trying to reach the vendor for more details. You can hold off the
patch for now. Thanks.

Re: [PATCH, net] phy: aquantia: Fix AN when higher speeds than 1G are not advertised

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 10 Jun 2022 11:40:37 +0300 you wrote:
> Even when the eth port is resticted to work with speeds not higher than 1G,
> and so the eth driver is requesting the phy (via phylink) to advertise up
> to 1000BASET support, the aquantia phy device is still advertising for 2.5G
> and 5G speeds.
> Clear these advertising defaults when requested.
> 
> Cc: Ondrej Spacek <ondrej.spacek@nxp.com>
> Fixes: 09c4c57f7bc41 ("net: phy: aquantia: add support for auto-negotiation configuration")
> Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
> 
> [...]

Here is the summary with links:
  - [net] phy: aquantia: Fix AN when higher speeds than 1G are not advertised
    https://git.kernel.org/netdev/net/c/9b7fd1670a94

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html