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

["Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com>]

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.  

Also my plan was to make 1 patch but i got review comments from Andrew to separate it out 
and hence two patches with 2 different functions and removing the usage of phy_set_max_speed




> Thanks,
> 
> Maxime