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

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

[Abhishek Chauhan]

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(-)

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,
-- 
2.25.1

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

[Maxime Chevallier]

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

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

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Przemek Kitszel]

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);
> +}

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

[Andrew Lunn]

> +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

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

[Abhishek Chauhan (ABC)]

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

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

[Abhishek Chauhan (ABC)]

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. 
> 

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

[Abhishek Chauhan (ABC)]

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);
>> +}

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

[Abhishek Chauhan (ABC)]

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

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

[Abhishek Chauhan (ABC)]

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