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

[Hans-Frieder Vogt <hfdevel@gmx.net>]

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?
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
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.
>
> 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.
> Thanks,
>
> Maxime
Thanks as well,
Hans-Frieder