Re: [PATCH net-next v5 4/7] net: phy: aquantia: add essential functions to aqr105 driver

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

On Sun, 23 Feb 2025 23:26:49 +0100
Hans-Frieder Vogt <hfdevel@gmx.net> wrote:

> Hi Maxime,
> 
> On 23.02.2025 11.32, Maxime Chevallier wrote:
> > Hi,
> >
> > On Sat, 22 Feb 2025 10:49:31 +0100
> > Hans-Frieder Vogt via B4 Relay <devnull+hfdevel.gmx.net@kernel.org>
> > wrote:
> >  
> >> From: Hans-Frieder Vogt <hfdevel@gmx.net>
> >>
> >> This patch makes functions that were provided for aqr107 applicable to
> >> aqr105, or replaces generic functions with specific ones. Since the aqr105
> >> was introduced before NBASE-T was defined (or 802.3bz), there are a number
> >> of vendor specific registers involved in the definition of the
> >> advertisement, in auto-negotiation and in the setting of the speed. The
> >> functions have been written following the downstream driver for TN4010
> >> cards with aqr105 PHY, and use code from aqr107 functions wherever it
> >> seemed to make sense.
> >>
> >> Signed-off-by: Hans-Frieder Vogt <hfdevel@gmx.net>
> >> ---
> >>   drivers/net/phy/aquantia/aquantia_main.c | 242 ++++++++++++++++++++++++++++++-
> >>   1 file changed, 240 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
> >> index 86b0e63de5d88fa1050919a8826bdbec4bbcf8ba..38c6cf7814da1fb9a4e715f242249eee15a3cc85 100644
> >> --- a/drivers/net/phy/aquantia/aquantia_main.c
> >> +++ b/drivers/net/phy/aquantia/aquantia_main.c
> >> @@ -33,6 +33,9 @@
> >>   #define PHY_ID_AQR115C	0x31c31c33
> >>   #define PHY_ID_AQR813	0x31c31cb2
> >>
> >> +#define MDIO_AN_10GBT_CTRL_ADV_LTIM		BIT(0)  
> > This is a standard C45 definition, from :
> > 45.2.7.10.15 10GBASE-T LD loop timing ability (7.32.0)
> >
> > So if you need this advertising capability, you should add that in the
> > generic definitions for C45 registers in include/uapi/linux/mdio.h  
> Thanks. Wasn't aware this being a standard definition.
> 
> Wouldn't the definition
> #define ADVERTISE_XNP                           BIT(12)
> then need to go to include/uapi/linux/mii.h accordingly?

Looks like this is indeed part of the standard now, in 

28.2.4.1.3 Auto-Negotiation advertisement register (Register 4), so it seems
to be the right move to modify ADVERTISE_RESV into  ADVERTISE_XNP.

> There, bit 12 is currently named ADVERTISE_RESV and commented as unused
> (which it obviously is not, because it is used in
> drivers/net/ethernet/sfc/falcon/mdio_10g.c

One note is that this driver uses the C45 MMD 7 AN register layout :

45.2.7.1 AN control register (Register 7.0)

in which the eXtended Next Page bit is BIT(13).

That actually leads to an interesting point, as it appears the at803x.c
driver mixes both, which looks incorrect to me :

	/* Ar803x extended next page bit is enabled by default. Cisco
	 * multigig switches read this bit and attempt to negotiate 10Gbps
	 * rates even if the next page bit is disabled. This is incorrect
	 * behaviour but we still need to accommodate it. XNP is only needed
	 * for 10Gbps support, so disable XNP.
	 */
	return phy_modify(phydev, MII_ADVERTISE, MDIO_AN_CTRL1_XNP, 0);

In such case, BIT(13) fot MII_ADVERTISE is ADVERTISE_RFAULT, if my
understanding of the spec is correct.

> I think, for now, I will just do the same as in the falcon driver and
> use ADVERTISE_RESV instead. Then it may be renamed later in all places.

Make sure you use the BIT(12) in your case indeed, looks to be the
right way in that case. 

> >
> > That being said, as it looks this is the first driver using this
> > feature, do you actually need to advertise Loop Timing ability here ?
> > I guess it comes from the vendor driver ?  
> you are right. The code just tries to replicate the vendor code.
> However, I have now tested the driver without this flag and haven't
> noticed any unusual behavior. So, I guess, it works indeed without.
> I'll remove the flag in the next revision of the patch.

So in that case, no need to define MDIO_AN_10GBT_CTRL_ADV_LTIM at all :)

Thanks,

Maxime