RE: [PATCH net-next 2/2] net: phy: microchip_t1: Autonegotiaion support for LAN887x T1 phy

[Tarun.Alle]

Hi Andrew,

Thanks for the review comments.

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Monday, December 9, 2024 10:41 PM
> To: Tarun Alle - I68638 <Tarun.Alle@xxxxxxxxxxxxx>
> Cc: Arun Ramadoss - I17769 <Arun.Ramadoss@xxxxxxxxxxxxx>;
> UNGLinuxDriver <UNGLinuxDriver@xxxxxxxxxxxxx>;
> hkallweit1@xxxxxxxxx; linux@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next 2/2] net: phy: microchip_t1: Autonegotiaion
> support for LAN887x T1 phy
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> the content is safe
> 
> > -     /* First patch only supports 100Mbps and 1000Mbps force-mode.
> > -      * T1 Auto-Negotiation (Clause 98 of IEEE 802.3) will be added later.
> > -      */
> > -     linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev-
> >supported);
> > -
> 
> What are the backwards compatibility issues here? I would at least expect
> some comments in the commit message. As far as i understand, up until
> this patch, it always required forced configuration. With this patch, it
> suddenly will auto-neg by default? If the link partner is not expecting auto-
> neg, that will fail.
> 

I will update the commit message.

> > +/* LAN887X Errata: 100M master issue. Dual speed in Aneg is not
> > +supported. */
> 
> Please could you expand on this. We are now doing auto-neg by default, and
> auto-neg is somewhat broken?
> 

LAN887X Errata: The device may not link in auto-neg when both 100BASE-T1 and 1000BASE-T1 are advertised.
Hence advertising only one speed. In this case auto-neg to determine Leader/Follower.

I will update comment as mentioned above.

>         Andrew

Thanks,
Tarun Alle.