Re: [PATCH net v4 2/2] net: phy: aquantia: remove usage of phy_set_max_speed

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

On Fri, 27 Sep 2024 12:42:36 -0700
"Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com> wrote:

> On 9/27/2024 9:37 AM, Maxime Chevallier wrote:
> > Hi,
> > 
> > On Thu, 26 Sep 2024 18:05:53 -0700
> > Abhishek Chauhan <quic_abchauha@quicinc.com> wrote:
> >   
> >> Remove the use of phy_set_max_speed in phy driver as the
> >> function is mainly used in MAC driver to set the max
> >> speed.
> >>
> >> Instead use get_features to fix up Phy PMA capabilities for
> >> AQR111, AQR111B0, AQR114C and AQCS109
> >>
> >> Fixes: 038ba1dc4e54 ("net: phy: aquantia: add AQR111 and AQR111B0 PHY ID")
> >> Fixes: 0974f1f03b07 ("net: phy: aquantia: remove false 5G and 10G speed ability for AQCS109")
> >> Fixes: c278ec644377 ("net: phy: aquantia: add support for AQR114C PHY ID")
> >> Link: https://lore.kernel.org/all/20240913011635.1286027-1-quic_abchauha@quicinc.com/T/
> >> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>  
> > 
> > [...]
> >   
> >> +static int aqr111_get_features(struct phy_device *phydev)
> >> +{
> >> +	unsigned long *supported = phydev->supported;
> >> +	int ret;
> >> +
> >> +	/* Normal feature discovery */
> >> +	ret = genphy_c45_pma_read_abilities(phydev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/* PHY FIXUP */
> >> +	/* Although the PHY sets bit 12.18.19, it does not support 10G modes */
> >> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> >> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, supported);
> >> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, supported);
> >> +
> >> +	/* Phy supports Speeds up to 5G with Autoneg though the phy PMA says otherwise */
> >> +	linkmode_or(supported, supported, phy_gbit_features);
> >> +	/* Set the 5G speed if it wasn't set as part of the PMA feature discovery */
> >> +	linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> >> +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);  
> > 
> > As you are moving away from phy_set_max_speed(phydev, 5000), it should mean
> > that what used to be in the supported bits already contained the
> > 5GBaseT bit, as phy_set_max_speed simply clears the highest speeds.
> > 
> > In such case, calling the newly introduced function from
> > patch 1 should be enough ?
> >   
> 
> Well i am not sure about how other phy(AQR111, AQR111B0, AQR114C and AQCS109) behaved, 
> but based on my testing and observation with AQR115c, it was pretty clear that 
> the phy did not advertise Autoneg capabilities, did not set lower speed such as 10M/100M/1000BaseT
> ,it did set capabilities beyond what is recommended in the data book.
> 
> So the below mentioned phys such as 
> 
> AQR111, AQR111B0, AQR114C = supports speed up to 5Gbps which means i cannot use the function
> defined in the previous patch as that sets speeds up to 2.5Gbps and all lower speeds. 
> 
> AQCS109 = supports speed up to 2.5Gbps and hence i have reused the same function aqr115c_get_features
> as part of this patch.

I understand your point, and it's hard indeed to be sure that no
regression was introduced. It does feel like you're reading the PHY
features, then reconstructing them almost from scratch again, but given
that the PMA report looks totally incorrect, there not much choice
indeed. So that's fine for me then.

Maxime