| Message ID | 20240924055251.3074850-1-quic_abchauha@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] net: phy: aquantia: Introduce custom get_features | expand |
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
On Mon, Sep 23, 2024 at 10:52:51PM -0700, Abhishek Chauhan wrote: > +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); > + 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); Maybe consider using: linkmode_copy(supported, phy_gbit_features); It shouldn't be necessary to set the two pause bits. You also won't need the initialiser.
On 9/24/24 07:52, Abhishek Chauhan 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> I'm not sure if this patch is -net material > +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); > + 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); > + 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); > + linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported); > + linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported); > + } instead of duplicating, just make the lists incremental: if (max_speed >= SPEED_2500) { linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, supported); linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported); } if (max_speed >= SPEED_5000) linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported); > + > + linkmode_copy(phydev->supported, supported); > +}
> +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); > + 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); > + 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); > + 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); > +} So you have got lots of comments.... Please split this into two patches. One patch for the PHY you are interested in, and a second patch to remove phy_set_max_speed() and fix up that PHY. Also, i would prefer you do the normal feature discovery, calling genphy_read_abilities() and/or genphy_c45_pma_read_abilities() and then fixup the results by removing the modes which should not be there. Take a look at bcm84881_get_features() as an example. Andrew
On 9/24/2024 1:24 AM, Maxime Chevallier wrote: > 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. 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
On 9/24/2024 1:36 AM, Russell King (Oracle) wrote: > On Mon, Sep 23, 2024 at 10:52:51PM -0700, Abhishek Chauhan wrote: >> +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); >> + 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); > > Maybe consider using: > > linkmode_copy(supported, phy_gbit_features); > > It shouldn't be necessary to set the two pause bits. You also won't need > the initialiser. Noted ! Thanks Russell. >
On 9/24/2024 1:46 AM, Przemek Kitszel wrote: > On 9/24/24 07:52, Abhishek Chauhan 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> > > I'm not sure if this patch is -net material > >> +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); >> + 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); >> + 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); >> + linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported); >> + linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported); >> + } > > instead of duplicating, just make the lists incremental: > > if (max_speed >= SPEED_2500) { > linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, supported); > linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported); > } > if (max_speed >= SPEED_5000) > linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported); > I will try to optimize this in the upcoming patches based on your suggestion. >> + >> + linkmode_copy(phydev->supported, supported); >> +}
On 9/24/2024 5:04 AM, Andrew Lunn wrote: >> +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); >> + 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); >> + 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); >> + 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); >> +} > > So you have got lots of comments.... > > Please split this into two patches. One patch for the PHY you are > interested in, and a second patch to remove phy_set_max_speed() and > fix up that PHY. > Noted! > Also, i would prefer you do the normal feature discovery, calling > genphy_read_abilities() and/or genphy_c45_pma_read_abilities() and > then fixup the results by removing the modes which should not be > there. > Sounds good! > Take a look at bcm84881_get_features() as an example. > Thanks Andrew! > Andrew
On 9/24/2024 8:19 AM, Abhishek Chauhan (ABC) wrote: > > > On 9/24/2024 5:04 AM, Andrew Lunn wrote: >>> +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); >>> + 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); >>> + 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); >>> + 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); >>> +} >> >> So you have got lots of comments.... >> >> Please split this into two patches. One patch for the PHY you are >> interested in, and a second patch to remove phy_set_max_speed() and >> fix up that PHY. >> > Noted! > >> Also, i would prefer you do the normal feature discovery, calling >> genphy_read_abilities() and/or genphy_c45_pma_read_abilities() and >> then fixup the results by removing the modes which should not be >> there. >> > Sounds good! >> Take a look at bcm84881_get_features() as an example. >> > Thanks Andrew! Andrew, (Let me know if this is okay with you) On doing the normal feature discovery today I observed that AQR115C i want to document this misbehavior here for future reference //Print added by me in the AQR driver [ 5.583440] AQR supported mask=00,00000000,00018000,000e102c key points :- AQR115c supports 10Mbps(F/H) but feature discovery says no AQR115C supports 1Gbps(F/H) but feature discovery says no AQR115C supports 2500BaseX but feature discovery says no AQR115C supports Autoneg but feature discovery says Hell no! AQR115C does not support 10GBaseT/KX4/KR but feature discovery says yes AQR115c does not support 5GbaseT but feature discovery says yes I have got 2 different FW from Marvell but none seem to help. I also got the confirmation from Marvell folks that Autoneg is supported but the feature discovery says otherwise. Here is the thing what i am going to do for now. (Let me know if this is okay with you) 1. Raise FIXUP for AQR115c patch - remove all the features which are not support - Add supported features which we really requires such as Autoneg/phy_gbit_features/2500BaseX/BaseT(Clearly see them supported in the NDA internal docs) Reference for other folks for information which is public : https://www.marvell.com/content/dam/marvell/en/public-collateral/transceivers/marvell-phys-transceivers-aqrate-gen4-product-brief.pdf 2. Raise FIXUP patch by removing phy_set_max_speed and call - Generic function which sets speeds up to 2.5Gbps for AQCS109 (2.5Gbps max speed) - Generic function which sets speeds up to 5 Gbps for AQR111/B0/114c (5Gbps max speed) Both points 1 and 2 will be a patch series with cover letter. >> Andrew
diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c index e982e9ce44a5..53e7e25f3c85 100644 --- a/drivers/net/phy/aquantia/aquantia_main.c +++ b/drivers/net/phy/aquantia/aquantia_main.c @@ -527,12 +527,6 @@ static int aqcs109_config_init(struct phy_device *phydev) if (!ret) aqr107_chip_info(phydev); - /* AQCS109 belongs to a chip family partially supporting 10G and 5G. - * PMA speed ability bits are the same for all members of the family, - * AQCS109 however supports speeds up to 2.5G only. - */ - phy_set_max_speed(phydev, SPEED_2500); - return aqr107_set_downshift(phydev, MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT); } @@ -639,6 +633,50 @@ static int aqr107_resume(struct phy_device *phydev) return aqr107_wait_processor_intensive_op(phydev); } +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); + 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); + 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); + 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; +} + static const u16 aqr_global_cfg_regs[] = { VEND1_GLOBAL_CFG_10M, VEND1_GLOBAL_CFG_100M, @@ -757,16 +795,6 @@ static int aqr107_probe(struct phy_device *phydev) return aqr_hwmon_probe(phydev); } -static int aqr111_config_init(struct phy_device *phydev) -{ - /* AQR111 reports supporting speed up to 10G, - * however only speeds up to 5G are supported. - */ - phy_set_max_speed(phydev, SPEED_5000); - - return aqr107_config_init(phydev); -} - static struct phy_driver aqr_driver[] = { { PHY_ID_MATCH_MODEL(PHY_ID_AQ1202), @@ -843,6 +871,7 @@ static struct phy_driver aqr_driver[] = { .get_sset_count = aqr107_get_sset_count, .get_strings = aqr107_get_strings, .get_stats = aqr107_get_stats, + .get_features = aqr_custom_get_feature, .link_change_notify = aqr107_link_change_notify, .led_brightness_set = aqr_phy_led_brightness_set, .led_hw_is_supported = aqr_phy_led_hw_is_supported, @@ -855,7 +884,7 @@ static struct phy_driver aqr_driver[] = { .name = "Aquantia AQR111", .probe = aqr107_probe, .get_rate_matching = aqr107_get_rate_matching, - .config_init = aqr111_config_init, + .config_init = aqr107_config_init, .config_aneg = aqr_config_aneg, .config_intr = aqr_config_intr, .handle_interrupt = aqr_handle_interrupt, @@ -867,6 +896,7 @@ static struct phy_driver aqr_driver[] = { .get_sset_count = aqr107_get_sset_count, .get_strings = aqr107_get_strings, .get_stats = aqr107_get_stats, + .get_features = aqr_custom_get_feature, .link_change_notify = aqr107_link_change_notify, .led_brightness_set = aqr_phy_led_brightness_set, .led_hw_is_supported = aqr_phy_led_hw_is_supported, @@ -879,7 +909,7 @@ static struct phy_driver aqr_driver[] = { .name = "Aquantia AQR111B0", .probe = aqr107_probe, .get_rate_matching = aqr107_get_rate_matching, - .config_init = aqr111_config_init, + .config_init = aqr107_config_init, .config_aneg = aqr_config_aneg, .config_intr = aqr_config_intr, .handle_interrupt = aqr_handle_interrupt, @@ -891,6 +921,7 @@ static struct phy_driver aqr_driver[] = { .get_sset_count = aqr107_get_sset_count, .get_strings = aqr107_get_strings, .get_stats = aqr107_get_stats, + .get_features = aqr_custom_get_feature, .link_change_notify = aqr107_link_change_notify, .led_brightness_set = aqr_phy_led_brightness_set, .led_hw_is_supported = aqr_phy_led_hw_is_supported, @@ -1000,7 +1031,7 @@ static struct phy_driver aqr_driver[] = { .name = "Aquantia AQR114C", .probe = aqr107_probe, .get_rate_matching = aqr107_get_rate_matching, - .config_init = aqr111_config_init, + .config_init = aqr107_config_init, .config_aneg = aqr_config_aneg, .config_intr = aqr_config_intr, .handle_interrupt = aqr_handle_interrupt, @@ -1012,6 +1043,7 @@ static struct phy_driver aqr_driver[] = { .get_sset_count = aqr107_get_sset_count, .get_strings = aqr107_get_strings, .get_stats = aqr107_get_stats, + .get_features = aqr_custom_get_feature, .link_change_notify = aqr107_link_change_notify, .led_brightness_set = aqr_phy_led_brightness_set, .led_hw_is_supported = aqr_phy_led_hw_is_supported, @@ -1036,6 +1068,7 @@ static struct phy_driver aqr_driver[] = { .get_sset_count = aqr107_get_sset_count, .get_strings = aqr107_get_strings, .get_stats = aqr107_get_stats, + .get_features = aqr_custom_get_feature, .link_change_notify = aqr107_link_change_notify, .led_brightness_set = aqr_phy_led_brightness_set, .led_hw_is_supported = aqr_phy_led_hw_is_supported,
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 drivers/net/phy/aquantia/aquantia_main.c | 71 +++++++++++++++++------- 1 file changed, 52 insertions(+), 19 deletions(-)