Re: [PATCH net v2] net: phy: aquantia: Introduce custom get_features

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

Hi,

On Mon, 23 Sep 2024 22:52:51 -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.
> 
> Introduce custom get_features for AQR family of chipsets
> 
> 1. such as AQR111/B0/114c which supports speeds up to 5Gbps
> 2. such as AQR115c/AQCS109 which supports speeds up to 2.5Gbps
> 
> 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")
> Fixes: 0ebc581f8a4b ("net: phy: aquantia: add support for aqr115c")
> Link: https://lore.kernel.org/all/20240913011635.1286027-1-quic_abchauha@quicinc.com/T/
> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
> ---
> Changes since v1 
> 1. remove usage of phy_set_max_speed in the aquantia driver code.
> 2. Introduce aqr_custom_get_feature which checks for the phy id and
>    takes necessary actions based on max_speed supported by the phy
> 3. remove aqr111_config_init as it is just a wrapper function. 
> 
> output from my device looks like :- 
> 1. Link is up with 2.5Gbps with 2500BaseX with autoneg on.
> 
> 
> Settings for eth0:
>         Supported ports: [ TP    FIBRE ]
>         Supported link modes:   10baseT/Full
>                                 100baseT/Full
>                                 1000baseT/Full
>                                 2500baseX/Full
>                                 2500baseT/Full
>         Supported pause frame use: Symmetric Receive-only
>         Supports auto-negotiation: Yes
>         Supported FEC modes: Not reported
>         Advertised link modes:  10baseT/Full
>                                 100baseT/Full
>                                 1000baseT/Full
>                                 2500baseX/Full
>                                 2500baseT/Full
> 

 [...]

> +static void aqr_supported_speed(struct phy_device *phydev, u32 max_speed)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };
> +
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, supported);

Can this PHY actually support FIBRE ports ? What you must list here are
the modes that the PHY can support on the LP side. I'm not familiar
with this PHY, but from what I can see from the current driver, there's
no such support yet in the driver.

> +	linkmode_set_bit(ETHTOOL_LINK_MODE_TP_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> +
> +	if (max_speed == SPEED_2500) {
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, supported);

If the PHY is strictly BaseT, then you shouldn't specify 2500BaseX as
supported

> +		linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> +	} else if (max_speed == SPEED_5000) {
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, supported);

Same here

> +		linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> +	}
> +
> +	linkmode_copy(phydev->supported, supported);
> +}
> +
> +static int aqr_custom_get_feature(struct phy_device *phydev)
> +{
> +	switch (phydev->drv->phy_id) {
> +	case PHY_ID_AQR115C:
> +	case PHY_ID_AQCS109:
> +		aqr_supported_speed(phydev, SPEED_2500);
> +	break;
> +	case PHY_ID_AQR111:
> +	case PHY_ID_AQR111B0:
> +	case PHY_ID_AQR114C:
> +		aqr_supported_speed(phydev, SPEED_5000);
> +	break;
> +	}
> +	return 0;
> +}

You could define one .get_feature for the 2.5G PHYs and another for the
5G phys, that would avoid having to modify this single helper for each
new PHY.

Thanks,

Maxime