1. Patchew
  2. linux
  3. [PATCH net v3 0/2] Fix AQR PMA capabilities
  4. View patch

[PATCH net v3 1/2] net: phy: aquantia: AQR115c fix up PMA capabilities

Abhishek Chauhan posted 2 patches 2 months ago
There is a newer version of this series
[PATCH net v3 1/2] net: phy: aquantia: AQR115c fix up PMA capabilities
Posted by Abhishek Chauhan 2 months ago 
AQR115c reports incorrect PMA capabilities which includes
10G/5G and also incorrectly disables capabilities like autoneg
and 10Mbps support.

AQR115c as per the Marvell databook supports speeds up to 2.5Gbps
with autonegotiation.

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 v2
1. seperate out the changes into two different patches. 
3. use phy_gbit_features instead of initializing each and 
every link mode bits. 
4. write seperate functions for 2.5Gbps supported phy 
5. remove FIBRE bit which was introduced in patch 1

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. 

 drivers/net/phy/aquantia/aquantia_main.c | 28 ++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index e982e9ce44a5..88ba12aaf6e0 100644
--- a/drivers/net/phy/aquantia/aquantia_main.c
+++ b/drivers/net/phy/aquantia/aquantia_main.c
@@ -767,6 +767,33 @@ static int aqr111_config_init(struct phy_device *phydev)
 	return aqr107_config_init(phydev);
 }
 
+static int aqr115c_get_features(struct phy_device *phydev)
+{
+	int ret;
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };
+
+	/* Normal feature discovery */
+	ret = genphy_c45_pma_read_abilities(phydev);
+	if (ret)
+		return ret;
+
+	/* PHY FIXUP */
+	/* Although the PHY sets bit 12.18.19.48, it does not support 5G/10G modes */
+	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, phydev->supported);
+	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, phydev->supported);
+	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, phydev->supported);
+	linkmode_clear_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, phydev->supported);
+
+	/* Phy supports Speeds up to 2.5G with Autoneg though the phy PMA says otherwise */
+	linkmode_copy(supported, phy_gbit_features);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
+
+	linkmode_or(phydev->supported, supported, phydev->supported);
+
+	return 0;
+}
+
 static struct phy_driver aqr_driver[] = {
 {
 	PHY_ID_MATCH_MODEL(PHY_ID_AQ1202),
@@ -1036,6 +1063,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   = aqr115c_get_features,
 	.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 v3 1/2] net: phy: aquantia: AQR115c fix up PMA capabilities
Posted by Russell King (Oracle) 2 months ago 
On Wed, Sep 25, 2024 at 04:01:28PM -0700, Abhishek Chauhan wrote:
> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
> index e982e9ce44a5..88ba12aaf6e0 100644
> --- a/drivers/net/phy/aquantia/aquantia_main.c
> +++ b/drivers/net/phy/aquantia/aquantia_main.c
> @@ -767,6 +767,33 @@ static int aqr111_config_init(struct phy_device *phydev)
>  	return aqr107_config_init(phydev);
>  }
>  
> +static int aqr115c_get_features(struct phy_device *phydev)
> +{
> +	int ret;
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };

Networking uses reverse-christmas tree ordering for variables. Please
swap the order of these.

Also, I think this would be better:

	unsigned long *supported = phydev->supported;

You don't actually need a separate mask.

> +
> +	/* Normal feature discovery */
> +	ret = genphy_c45_pma_read_abilities(phydev);
> +	if (ret)
> +		return ret;
> +
> +	/* PHY FIXUP */
> +	/* Although the PHY sets bit 12.18.19.48, it does not support 5G/10G modes */
> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, phydev->supported);
> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, phydev->supported);
> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, phydev->supported);
> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, phydev->supported);

For the above four, s/phydev->supported/supported/

> +
> +	/* Phy supports Speeds up to 2.5G with Autoneg though the phy PMA says otherwise */
> +	linkmode_copy(supported, phy_gbit_features);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> +
> +	linkmode_or(phydev->supported, supported, phydev->supported);

Drop this linkmode_or().

You'll then be modifying phydev->supported directly, which is totally
fine.

-- 
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 v3 1/2] net: phy: aquantia: AQR115c fix up PMA capabilities
Posted by Abhishek Chauhan (ABC) 2 months ago 

On 9/26/2024 4:45 AM, Russell King (Oracle) wrote:
> On Wed, Sep 25, 2024 at 04:01:28PM -0700, Abhishek Chauhan wrote:
>> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
>> index e982e9ce44a5..88ba12aaf6e0 100644
>> --- a/drivers/net/phy/aquantia/aquantia_main.c
>> +++ b/drivers/net/phy/aquantia/aquantia_main.c
>> @@ -767,6 +767,33 @@ static int aqr111_config_init(struct phy_device *phydev)
>>  	return aqr107_config_init(phydev);
>>  }
>>  
>> +static int aqr115c_get_features(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };
> 
> Networking uses reverse-christmas tree ordering for variables. Please
> swap the order of these.
> 
> Also, I think this would be better:
> 
> 	unsigned long *supported = phydev->supported;
> 
> You don't actually need a separate mask.
> 
>> +
>> +	/* Normal feature discovery */
>> +	ret = genphy_c45_pma_read_abilities(phydev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* PHY FIXUP */
>> +	/* Although the PHY sets bit 12.18.19.48, it does not support 5G/10G modes */
>> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, phydev->supported);
>> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, phydev->supported);
>> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, phydev->supported);
>> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, phydev->supported);
> 
> For the above four, s/phydev->supported/supported/
> 
>> +
>> +	/* Phy supports Speeds up to 2.5G with Autoneg though the phy PMA says otherwise */
>> +	linkmode_copy(supported, phy_gbit_features);
>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, supported);
>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
>> +
>> +	linkmode_or(phydev->supported, supported, phydev->supported);
> 
> Drop this linkmode_or().
> 
> You'll then be modifying phydev->supported directly, which is totally
> fine.
Noted!.

Thanks Russell. Appreciate all the reviews. 


>
Re: [PATCH net v3 1/2] net: phy: aquantia: AQR115c fix up PMA capabilities
Posted by Maxime Chevallier 2 months ago 
Hello,

On Wed, 25 Sep 2024 16:01:28 -0700
Abhishek Chauhan <quic_abchauha@quicinc.com> wrote:

> AQR115c reports incorrect PMA capabilities which includes
> 10G/5G and also incorrectly disables capabilities like autoneg
> and 10Mbps support.
> 
> AQR115c as per the Marvell databook supports speeds up to 2.5Gbps
> with autonegotiation.
> 
> 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>
> ---

[...]

>  
> +static int aqr115c_get_features(struct phy_device *phydev)
> +{
> +	int ret;
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };
> +
> +	/* Normal feature discovery */
> +	ret = genphy_c45_pma_read_abilities(phydev);
> +	if (ret)
> +		return ret;
> +
> +	/* PHY FIXUP */
> +	/* Although the PHY sets bit 12.18.19.48, it does not support 5G/10G modes */
> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, phydev->supported);
> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, phydev->supported);
> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, phydev->supported);
> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, phydev->supported);
> +
> +	/* Phy supports Speeds up to 2.5G with Autoneg though the phy PMA says otherwise */
> +	linkmode_copy(supported, phy_gbit_features);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, supported);

I still think you shouldn't report 2500BaseX, as you mentionned
it's a BaseT PHY. This is independent of the modes that the PHY uses to
connect to the MAC. Although the PHY can talk to the MAC using
2500BaseX on its MII interface, it looks like it can't use
2500BaseX on its MDI (the LP side of the PHY). There's the same issue in
patch 2.

Thanks,

Maxime
Re: [PATCH net v3 1/2] net: phy: aquantia: AQR115c fix up PMA capabilities
Posted by Abhishek Chauhan (ABC) 2 months ago 

On 9/25/2024 11:42 PM, Maxime Chevallier wrote:
> Hello,
> 
> On Wed, 25 Sep 2024 16:01:28 -0700
> Abhishek Chauhan <quic_abchauha@quicinc.com> wrote:
> 
>> AQR115c reports incorrect PMA capabilities which includes
>> 10G/5G and also incorrectly disables capabilities like autoneg
>> and 10Mbps support.
>>
>> AQR115c as per the Marvell databook supports speeds up to 2.5Gbps
>> with autonegotiation.
>>
>> 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>
>> ---
> 
> [...]
> 
>>  
>> +static int aqr115c_get_features(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };
>> +
>> +	/* Normal feature discovery */
>> +	ret = genphy_c45_pma_read_abilities(phydev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* PHY FIXUP */
>> +	/* Although the PHY sets bit 12.18.19.48, it does not support 5G/10G modes */
>> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, phydev->supported);
>> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, phydev->supported);
>> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, phydev->supported);
>> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, phydev->supported);
>> +
>> +	/* Phy supports Speeds up to 2.5G with Autoneg though the phy PMA says otherwise */
>> +	linkmode_copy(supported, phy_gbit_features);
>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, supported);
> 
> I still think you shouldn't report 2500BaseX, as you mentionned
> it's a BaseT PHY. This is independent of the modes that the PHY uses to
> connect to the MAC. Although the PHY can talk to the MAC using
> 2500BaseX on its MII interface, it looks like it can't use
> 2500BaseX on its MDI (the LP side of the PHY). There's the same issue in
> patch 2.
> 
I Agree with you. I did some experiments today without setting the 2500BaseX bit and i see
the link still comes up with 2.5Gbps with autoneg on. 

I will rectify this as part of the next patch 

Thanks Maxime 


> Thanks,
> 
> Maxime

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.