Re: [RFC PATCH net v1] net: phy: aquantia: Set phy speed to 2.5gbps for AQR115c

[Andrew Lunn]

On Fri, Sep 13, 2024 at 09:12:13AM -0700, Abhishek Chauhan (ABC) wrote:
> 
> 
> On 9/13/2024 1:01 AM, Maxime Chevallier wrote:
> > Hi,
> > 
> > On Thu, 12 Sep 2024 18:16:35 -0700
> > Abhishek Chauhan <quic_abchauha@xxxxxxxxxxx> wrote:
> > 
> >> Recently we observed that aquantia AQR115c always comes up in
> >> 100Mbps mode. AQR115c aquantia chip supports max speed up to
> >> 2.5Gbps. Today the AQR115c configuration is done through
> >> aqr113c_config_init which internally calls aqr107_config_init.
> >> aqr113c and aqr107 are both capable of 10Gbps. Whereas AQR115c
> >> supprts max speed of 2.5Gbps only.
> >>
> >> Fixes: 0ebc581f8a4b ("net: phy: aquantia: add support for aqr115c")
> >> Signed-off-by: Abhishek Chauhan <quic_abchauha@xxxxxxxxxxx>
> >> ---
> >>  drivers/net/phy/aquantia/aquantia_main.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
> >> index e982e9ce44a5..9afc041dbb64 100644
> >> --- a/drivers/net/phy/aquantia/aquantia_main.c
> >> +++ b/drivers/net/phy/aquantia/aquantia_main.c
> >> @@ -499,6 +499,12 @@ static int aqr107_config_init(struct phy_device *phydev)
> >>  	if (!ret)
> >>  		aqr107_chip_info(phydev);
> >>  
> >> +	/* AQR115c supports speed up to 2.5Gbps */
> >> +	if (phydev->interface == PHY_INTERFACE_MODE_2500BASEX) {
> >> +		phy_set_max_speed(phydev, SPEED_2500);
> >> +		phydev->autoneg = AUTONEG_ENABLE;
> >> +	}
> >> +
> > 
> > If I get your commit log right, the code above will also apply for
> > ASQR107, AQR113 and so on, don't you risk breaking these PHYs if they
> > are in 2500BASEX mode at boot?
> > 
> 
> I was thinking of the same. That this might break something here for other Phy chip. 
> As every phy shares the same config init. Hence the reason for RFC. 
> 
> > Besides that, if the PHY switches between SGMII and 2500BASEX
> > dynamically depending on the link speed, it could be that it's
> > configured by default in SGMII, hence this check will be missed.
> > 
> > 
> I think the better way is to have AQR115c its own config_init which sets 
> the max speed to 2.5Gbps and then call aqr113c_config_init . 

phy_set_max_speed(phydev, SPEED_2500) is something a MAC does, not a
PHY. It is a way for the MAC to say is supports less than the PHY. I
would say the current aqcs109_config_init() is doing this wrong.

> > Is the AQR115c in the same situation as AQR111 for example, where the
> > PMA capabilities reported are incorrect ?

This is the approach to follow. The PHY registers should report what
it is actually capable of. But some aquantia PHYs get this
wrong. Please check if this is also true for your PHY. Look at what
genphy_c45_pma_read_abilities() is doing.

You might need to implement a .get_feature callback for this PHY which
first calls genphy_c45_pma_read_abilities() and then manually fixes up
what the PHY gets wrong.

	Andrew