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 v4
1. Forget about asking hardware for PMA capabilites.
2. Just set the Gbits features along with 2.5Gbps support 
   for AQR115c Phy chip. 

Changes since v3
1. remove setting of 2500baseX bit introduced as
   part of previous patches.
2. follow reverse xmas declaration of variables.
3. remove local mask introduced as part of
   previous patch and optimize the logic.

Changes since v2
1. seperate out the changes into two different patches. 
2. use phy_gbit_features instead of initializing each and 
   every link mode bits. 
3. write seperate functions for 2.5Gbps supported phy.
4. 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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index e982e9ce44a5..bd543cdc8c1d 100644
--- a/drivers/net/phy/aquantia/aquantia_main.c
+++ b/drivers/net/phy/aquantia/aquantia_main.c
@@ -721,6 +721,16 @@ static int aqr113c_fill_interface_modes(struct phy_device *phydev)
 	return aqr107_fill_interface_modes(phydev);
 }
 
+static int aqr115c_get_features(struct phy_device *phydev)
+{
+	/* PHY FIXUP */
+	/* Phy supports Speeds up to 2.5G with Autoneg though the phy PMA says otherwise */
+	linkmode_or(phydev->supported, phydev->supported, phy_gbit_features);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->supported);
+
+	return 0;
+}
+
 static int aqr113c_config_init(struct phy_device *phydev)
 {
 	int ret;
@@ -1036,6 +1046,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

On Mon, Sep 30, 2024 at 03:33:40PM -0700, Abhishek Chauhan 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.

Thanks for persisting with this. Just one further item:

> +static int aqr115c_get_features(struct phy_device *phydev)
> +{
> +	/* PHY FIXUP */
> +	/* Phy supports Speeds up to 2.5G with Autoneg though the phy PMA says otherwise */
> +	linkmode_or(phydev->supported, phydev->supported, phy_gbit_features);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->supported);

I'd still prefer to see:

	unsigned long *supported = phydev->supported;

	/* PHY supports speeds up to 2.5G with autoneg. PMA capabilities
	 * are not useful.
	 */
	linkmode_or(supported, supported, phy_gbit_features);
	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);

because that avoids going over column 80, and networking prefers it that
way.

Other than that, the patch looks the best solution.

Thanks.

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

On 10/1/2024 4:59 AM, Russell King (Oracle) wrote:
> On Mon, Sep 30, 2024 at 03:33:40PM -0700, Abhishek Chauhan 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.
> 
> Thanks for persisting with this. Just one further item:
> 
No worries, Russell. Thank you for your guidance. 

>> +static int aqr115c_get_features(struct phy_device *phydev)
>> +{
>> +	/* PHY FIXUP */
>> +	/* Phy supports Speeds up to 2.5G with Autoneg though the phy PMA says otherwise */
>> +	linkmode_or(phydev->supported, phydev->supported, phy_gbit_features);
>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->supported);
> 
> I'd still prefer to see:
> 
> 	unsigned long *supported = phydev->supported;
> 
> 	/* PHY supports speeds up to 2.5G with autoneg. PMA capabilities
> 	 * are not useful.
> 	 */
> 	linkmode_or(supported, supported, phy_gbit_features);
> 	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> 
> because that avoids going over column 80, and networking prefers it that
> way.
> 
Noted!

> Other than that, the patch looks the best solution.
> 
> Thanks.
>