On 9/24/2024 1:24 AM, Maxime Chevallier wrote: > Hi, > > On Mon, 23 Sep 2024 22:52:51 -0700 > Abhishek Chauhan <quic_abchauha@xxxxxxxxxxx> 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@xxxxxxxxxxx/T/ >> Signed-off-by: Abhishek Chauhan <quic_abchauha@xxxxxxxxxxx> >> --- >> 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. I will update this . I have seen the databook and i dont think the Phy supports FIBRE. > >> + 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 Noted! > >> + 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. > Noted! > Thanks, > > Maxime