| Message ID | 20220610084037.7625-1-claudiu.manoil@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9b7fd1670a94a57d974795acebde843a5c1a354e |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] phy: aquantia: Fix AN when higher speeds than 1G are not advertised | expand |
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
> -----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
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
> -----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.
> 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
> -----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.
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!
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)
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(-)