[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a599d7ac-b712-412c-9034-e0f35d3451d8@quicinc.com>
Date: Tue, 24 Sep 2024 08:17:42 -0700
From: "Abhishek Chauhan (ABC)" <quic_abchauha@...cinc.com>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
CC: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet
<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni
<pabeni@...hat.com>, <netdev@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, Andrew Halaney <ahalaney@...hat.com>,
"Russell King (Oracle)" <linux@...linux.org.uk>,
Andrew Lunn
<andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
Bartosz Golaszewski
<bartosz.golaszewski@...aro.org>,
"linux-tegra@...r.kernel.org"
<linux-tegra@...r.kernel.org>,
Brad Griffis <bgriffis@...dia.com>,
"Vladimir
Oltean" <vladimir.oltean@....com>,
Jon Hunter <jonathanh@...dia.com>, <kernel@...cinc.com>
Subject: Re: [PATCH net v2] net: phy: aquantia: Introduce custom get_features
On 9/24/2024 1:24 AM, Maxime Chevallier wrote:
> Hi,
>
> On Mon, 23 Sep 2024 22:52:51 -0700
> Abhishek Chauhan <quic_abchauha@...cinc.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@...cinc.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
Powered by blists - more mailing lists