[RFC PATCH net v1] net: phy: aquantia: Set phy speed to 2.5gbps for AQR115c

[RFC PATCH net v1] net: phy: aquantia: Set phy speed to 2.5gbps for AQR115c

[Abhishek Chauhan]

Recently we observed that aquantia AQR115c always comes up in
100Mbps mode. AQR115c aquantia chip supports max speed up to
2.5Gbps. Today the AQR115c configuration is done through
aqr113c_config_init which internally calls aqr107_config_init.
aqr113c and aqr107 are both capable of 10Gbps. Whereas AQR115c
supprts max speed of 2.5Gbps only.

Fixes: 0ebc581f8a4b ("net: phy: aquantia: add support for aqr115c")
Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
---
 drivers/net/phy/aquantia/aquantia_main.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index e982e9ce44a5..9afc041dbb64 100644
--- a/drivers/net/phy/aquantia/aquantia_main.c
+++ b/drivers/net/phy/aquantia/aquantia_main.c
@@ -499,6 +499,12 @@ static int aqr107_config_init(struct phy_device *phydev)
 	if (!ret)
 		aqr107_chip_info(phydev);
 
+	/* AQR115c supports speed up to 2.5Gbps */
+	if (phydev->interface == PHY_INTERFACE_MODE_2500BASEX) {
+		phy_set_max_speed(phydev, SPEED_2500);
+		phydev->autoneg = AUTONEG_ENABLE;
+	}
+
 	ret = aqr107_set_downshift(phydev, MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT);
 	if (ret)
 		return ret;
@@ -1036,6 +1042,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	= genphy_c45_pma_read_abilities,
 	.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: [RFC PATCH net v1] net: phy: aquantia: Set phy speed to 2.5gbps for AQR115c

[Maxime Chevallier]

Hi,

On Thu, 12 Sep 2024 18:16:35 -0700
Abhishek Chauhan <quic_abchauha@quicinc.com> wrote:

> Recently we observed that aquantia AQR115c always comes up in
> 100Mbps mode. AQR115c aquantia chip supports max speed up to
> 2.5Gbps. Today the AQR115c configuration is done through
> aqr113c_config_init which internally calls aqr107_config_init.
> aqr113c and aqr107 are both capable of 10Gbps. Whereas AQR115c
> supprts max speed of 2.5Gbps only.
> 
> Fixes: 0ebc581f8a4b ("net: phy: aquantia: add support for aqr115c")
> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
> ---
>  drivers/net/phy/aquantia/aquantia_main.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
> index e982e9ce44a5..9afc041dbb64 100644
> --- a/drivers/net/phy/aquantia/aquantia_main.c
> +++ b/drivers/net/phy/aquantia/aquantia_main.c
> @@ -499,6 +499,12 @@ static int aqr107_config_init(struct phy_device *phydev)
>  	if (!ret)
>  		aqr107_chip_info(phydev);
>  
> +	/* AQR115c supports speed up to 2.5Gbps */
> +	if (phydev->interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		phy_set_max_speed(phydev, SPEED_2500);
> +		phydev->autoneg = AUTONEG_ENABLE;
> +	}
> +

If I get your commit log right, the code above will also apply for
ASQR107, AQR113 and so on, don't you risk breaking these PHYs if they
are in 2500BASEX mode at boot?

Besides that, if the PHY switches between SGMII and 2500BASEX
dynamically depending on the link speed, it could be that it's
configured by default in SGMII, hence this check will be missed.

Is the AQR115c in the same situation as AQR111 for example, where the
PMA capabilities reported are incorrect ? If so, you can take the same
approach as aqr111, which is to create a dedicated .config_init()
callback for the AQR115c, which sets the max speed, then call
aqr113c_config_init() from there ?

Maxime

Re: [RFC PATCH net v1] net: phy: aquantia: Set phy speed to 2.5gbps for AQR115c

[Abhishek Chauhan (ABC)]

On 9/13/2024 1:01 AM, Maxime Chevallier wrote:
> Hi,
> 
> On Thu, 12 Sep 2024 18:16:35 -0700
> Abhishek Chauhan <quic_abchauha@quicinc.com> wrote:
> 
>> Recently we observed that aquantia AQR115c always comes up in
>> 100Mbps mode. AQR115c aquantia chip supports max speed up to
>> 2.5Gbps. Today the AQR115c configuration is done through
>> aqr113c_config_init which internally calls aqr107_config_init.
>> aqr113c and aqr107 are both capable of 10Gbps. Whereas AQR115c
>> supprts max speed of 2.5Gbps only.
>>
>> Fixes: 0ebc581f8a4b ("net: phy: aquantia: add support for aqr115c")
>> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
>> ---
>>  drivers/net/phy/aquantia/aquantia_main.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
>> index e982e9ce44a5..9afc041dbb64 100644
>> --- a/drivers/net/phy/aquantia/aquantia_main.c
>> +++ b/drivers/net/phy/aquantia/aquantia_main.c
>> @@ -499,6 +499,12 @@ static int aqr107_config_init(struct phy_device *phydev)
>>  	if (!ret)
>>  		aqr107_chip_info(phydev);
>>  
>> +	/* AQR115c supports speed up to 2.5Gbps */
>> +	if (phydev->interface == PHY_INTERFACE_MODE_2500BASEX) {
>> +		phy_set_max_speed(phydev, SPEED_2500);
>> +		phydev->autoneg = AUTONEG_ENABLE;
>> +	}
>> +
> 
> If I get your commit log right, the code above will also apply for
> ASQR107, AQR113 and so on, don't you risk breaking these PHYs if they
> are in 2500BASEX mode at boot?
> 

I was thinking of the same. That this might break something here for other Phy chip. 
As every phy shares the same config init. Hence the reason for RFC. 

> Besides that, if the PHY switches between SGMII and 2500BASEX
> dynamically depending on the link speed, it could be that it's
> configured by default in SGMII, hence this check will be missed.
> 
> Is the AQR115c in the same situation as AQR111 for example, where the
> PMA capabilities reported are incorrect ? If so, you can take the same
> approach as aqr111, which is to create a dedicated .config_init()
> callback for the AQR115c, which sets the max speed, then call
> aqr113c_config_init() from there ?
> 
I think the better way is to have AQR115c its own config_init which sets 
the max speed to 2.5Gbps and then call aqr113c_config_init . 
I will clean up the config_init and 

Thanks for the review comments.

> Maxime

Re: [RFC PATCH net v1] net: phy: aquantia: Set phy speed to 2.5gbps for AQR115c

[Andrew Lunn]

On Fri, Sep 13, 2024 at 09:12:13AM -0700, Abhishek Chauhan (ABC) wrote:
> 
> 
> On 9/13/2024 1:01 AM, Maxime Chevallier wrote:
> > Hi,
> > 
> > On Thu, 12 Sep 2024 18:16:35 -0700
> > Abhishek Chauhan <quic_abchauha@quicinc.com> wrote:
> > 
> >> Recently we observed that aquantia AQR115c always comes up in
> >> 100Mbps mode. AQR115c aquantia chip supports max speed up to
> >> 2.5Gbps. Today the AQR115c configuration is done through
> >> aqr113c_config_init which internally calls aqr107_config_init.
> >> aqr113c and aqr107 are both capable of 10Gbps. Whereas AQR115c
> >> supprts max speed of 2.5Gbps only.
> >>
> >> Fixes: 0ebc581f8a4b ("net: phy: aquantia: add support for aqr115c")
> >> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
> >> ---
> >>  drivers/net/phy/aquantia/aquantia_main.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
> >> index e982e9ce44a5..9afc041dbb64 100644
> >> --- a/drivers/net/phy/aquantia/aquantia_main.c
> >> +++ b/drivers/net/phy/aquantia/aquantia_main.c
> >> @@ -499,6 +499,12 @@ static int aqr107_config_init(struct phy_device *phydev)
> >>  	if (!ret)
> >>  		aqr107_chip_info(phydev);
> >>  
> >> +	/* AQR115c supports speed up to 2.5Gbps */
> >> +	if (phydev->interface == PHY_INTERFACE_MODE_2500BASEX) {
> >> +		phy_set_max_speed(phydev, SPEED_2500);
> >> +		phydev->autoneg = AUTONEG_ENABLE;
> >> +	}
> >> +
> > 
> > If I get your commit log right, the code above will also apply for
> > ASQR107, AQR113 and so on, don't you risk breaking these PHYs if they
> > are in 2500BASEX mode at boot?
> > 
> 
> I was thinking of the same. That this might break something here for other Phy chip. 
> As every phy shares the same config init. Hence the reason for RFC. 
> 
> > Besides that, if the PHY switches between SGMII and 2500BASEX
> > dynamically depending on the link speed, it could be that it's
> > configured by default in SGMII, hence this check will be missed.
> > 
> > 
> I think the better way is to have AQR115c its own config_init which sets 
> the max speed to 2.5Gbps and then call aqr113c_config_init . 

phy_set_max_speed(phydev, SPEED_2500) is something a MAC does, not a
PHY. It is a way for the MAC to say is supports less than the PHY. I
would say the current aqcs109_config_init() is doing this wrong.

> > Is the AQR115c in the same situation as AQR111 for example, where the
> > PMA capabilities reported are incorrect ?

This is the approach to follow. The PHY registers should report what
it is actually capable of. But some aquantia PHYs get this
wrong. Please check if this is also true for your PHY. Look at what
genphy_c45_pma_read_abilities() is doing.

You might need to implement a .get_feature callback for this PHY which
first calls genphy_c45_pma_read_abilities() and then manually fixes up
what the PHY gets wrong.

	Andrew     	

Re: [RFC PATCH net v1] net: phy: aquantia: Set phy speed to 2.5gbps for AQR115c

[Abhishek Chauhan (ABC)]

On 9/13/2024 9:35 AM, Andrew Lunn wrote:
> On Fri, Sep 13, 2024 at 09:12:13AM -0700, Abhishek Chauhan (ABC) wrote:
>>
>>
>> On 9/13/2024 1:01 AM, Maxime Chevallier wrote:
>>> Hi,
>>>
>>> On Thu, 12 Sep 2024 18:16:35 -0700
>>> Abhishek Chauhan <quic_abchauha@quicinc.com> wrote:
>>>
>>>> Recently we observed that aquantia AQR115c always comes up in
>>>> 100Mbps mode. AQR115c aquantia chip supports max speed up to
>>>> 2.5Gbps. Today the AQR115c configuration is done through
>>>> aqr113c_config_init which internally calls aqr107_config_init.
>>>> aqr113c and aqr107 are both capable of 10Gbps. Whereas AQR115c
>>>> supprts max speed of 2.5Gbps only.
>>>>
>>>> Fixes: 0ebc581f8a4b ("net: phy: aquantia: add support for aqr115c")
>>>> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
>>>> ---
>>>>  drivers/net/phy/aquantia/aquantia_main.c | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
>>>> index e982e9ce44a5..9afc041dbb64 100644
>>>> --- a/drivers/net/phy/aquantia/aquantia_main.c
>>>> +++ b/drivers/net/phy/aquantia/aquantia_main.c
>>>> @@ -499,6 +499,12 @@ static int aqr107_config_init(struct phy_device *phydev)
>>>>  	if (!ret)
>>>>  		aqr107_chip_info(phydev);
>>>>  
>>>> +	/* AQR115c supports speed up to 2.5Gbps */
>>>> +	if (phydev->interface == PHY_INTERFACE_MODE_2500BASEX) {
>>>> +		phy_set_max_speed(phydev, SPEED_2500);
>>>> +		phydev->autoneg = AUTONEG_ENABLE;
>>>> +	}
>>>> +
>>>
>>> If I get your commit log right, the code above will also apply for
>>> ASQR107, AQR113 and so on, don't you risk breaking these PHYs if they
>>> are in 2500BASEX mode at boot?
>>>
>>
>> I was thinking of the same. That this might break something here for other Phy chip. 
>> As every phy shares the same config init. Hence the reason for RFC. 
>>
>>> Besides that, if the PHY switches between SGMII and 2500BASEX
>>> dynamically depending on the link speed, it could be that it's
>>> configured by default in SGMII, hence this check will be missed.
>>>
>>>
>> I think the better way is to have AQR115c its own config_init which sets 
>> the max speed to 2.5Gbps and then call aqr113c_config_init . 
> 

Noted!

> phy_set_max_speed(phydev, SPEED_2500) is something a MAC does, not a
> PHY. It is a way for the MAC to say is supports less than the PHY. I
> would say the current aqcs109_config_init() is doing this wrong.
> 
>>> Is the AQR115c in the same situation as AQR111 for example, where the
>>> PMA capabilities reported are incorrect ?
> 
> This is the approach to follow. The PHY registers should report what
> it is actually capable of. But some aquantia PHYs get this
> wrong. Please check if this is also true for your PHY. Look at what
> genphy_c45_pma_read_abilities() is doing.
> 
> You might need to implement a .get_feature callback for this PHY which
> first calls genphy_c45_pma_read_abilities() and then manually fixes up
> what the PHY gets wrong.
> 

Thanks alot Andrew. I will get back to you with my analysis on AQR115c 
phy. Most likely the pma abilities are reported incorrectly. 


> 	Andrew     	

Re: [RFC PATCH net v1] net: phy: aquantia: Set phy speed to 2.5gbps for AQR115c

[Russell King (Oracle)]

On Fri, Sep 13, 2024 at 06:35:17PM +0200, Andrew Lunn wrote:
> On Fri, Sep 13, 2024 at 09:12:13AM -0700, Abhishek Chauhan (ABC) wrote:
> > On 9/13/2024 1:01 AM, Maxime Chevallier wrote:
> > > Hi,
> > > 
> > > On Thu, 12 Sep 2024 18:16:35 -0700
> > > Abhishek Chauhan <quic_abchauha@quicinc.com> wrote:
> > > 
> > >> Recently we observed that aquantia AQR115c always comes up in
> > >> 100Mbps mode. AQR115c aquantia chip supports max speed up to
> > >> 2.5Gbps. Today the AQR115c configuration is done through
> > >> aqr113c_config_init which internally calls aqr107_config_init.
> > >> aqr113c and aqr107 are both capable of 10Gbps. Whereas AQR115c
> > >> supprts max speed of 2.5Gbps only.
> > >>
> > >> Fixes: 0ebc581f8a4b ("net: phy: aquantia: add support for aqr115c")
> > >> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
> > >> ---
> > >>  drivers/net/phy/aquantia/aquantia_main.c | 7 +++++++
> > >>  1 file changed, 7 insertions(+)
> > >>
> > >> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
> > >> index e982e9ce44a5..9afc041dbb64 100644
> > >> --- a/drivers/net/phy/aquantia/aquantia_main.c
> > >> +++ b/drivers/net/phy/aquantia/aquantia_main.c
> > >> @@ -499,6 +499,12 @@ static int aqr107_config_init(struct phy_device *phydev)
> > >>  	if (!ret)
> > >>  		aqr107_chip_info(phydev);
> > >>  
> > >> +	/* AQR115c supports speed up to 2.5Gbps */
> > >> +	if (phydev->interface == PHY_INTERFACE_MODE_2500BASEX) {
> > >> +		phy_set_max_speed(phydev, SPEED_2500);
> > >> +		phydev->autoneg = AUTONEG_ENABLE;
> > >> +	}
> > >> +
> > > 
> > > If I get your commit log right, the code above will also apply for
> > > ASQR107, AQR113 and so on, don't you risk breaking these PHYs if they
> > > are in 2500BASEX mode at boot?
> > > 
> > 
> > I was thinking of the same. That this might break something here for other Phy chip. 
> > As every phy shares the same config init. Hence the reason for RFC. 
> > 
> > > Besides that, if the PHY switches between SGMII and 2500BASEX
> > > dynamically depending on the link speed, it could be that it's
> > > configured by default in SGMII, hence this check will be missed.
> > > 
> > > 
> > I think the better way is to have AQR115c its own config_init which sets 
> > the max speed to 2.5Gbps and then call aqr113c_config_init . 
> 
> phy_set_max_speed(phydev, SPEED_2500) is something a MAC does, not a
> PHY. It is a way for the MAC to say is supports less than the PHY. I
> would say the current aqcs109_config_init() is doing this wrong.

Agreed on two points:

1) phy_set_max_speed() is documented as a function that the MAC will
call.

2) calling phy_set_max_speed() in .config_init() is way too late for
phylink. .config_init() is called from phy_init_hw(), which happens
after the PHY has been attached. However, phylink needs to know what
the PHY supports _before_ that, especially for any PHY that is on a
SFP, so it can determine what interface to use for the PHY.

So, as Andrew says, the current aqcs109_config_init(), and it seems
aqr111_config_init() are both broken.

The PHY driver needs to indicate to phylib what is supported by the
PHY no later than the .get_features() method.

Thanks.

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

Re: [RFC PATCH net v1] net: phy: aquantia: Set phy speed to 2.5gbps for AQR115c

[Abhishek Chauhan (ABC)]

On 9/17/2024 2:31 AM, Russell King (Oracle) wrote:
> On Fri, Sep 13, 2024 at 06:35:17PM +0200, Andrew Lunn wrote:
>> On Fri, Sep 13, 2024 at 09:12:13AM -0700, Abhishek Chauhan (ABC) wrote:
>>> On 9/13/2024 1:01 AM, Maxime Chevallier wrote:
>>>> Hi,
>>>>
>>>> On Thu, 12 Sep 2024 18:16:35 -0700
>>>> Abhishek Chauhan <quic_abchauha@quicinc.com> wrote:
>>>>
>>>>> Recently we observed that aquantia AQR115c always comes up in
>>>>> 100Mbps mode. AQR115c aquantia chip supports max speed up to
>>>>> 2.5Gbps. Today the AQR115c configuration is done through
>>>>> aqr113c_config_init which internally calls aqr107_config_init.
>>>>> aqr113c and aqr107 are both capable of 10Gbps. Whereas AQR115c
>>>>> supprts max speed of 2.5Gbps only.
>>>>>
>>>>> Fixes: 0ebc581f8a4b ("net: phy: aquantia: add support for aqr115c")
>>>>> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
>>>>> ---
>>>>>  drivers/net/phy/aquantia/aquantia_main.c | 7 +++++++
>>>>>  1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
>>>>> index e982e9ce44a5..9afc041dbb64 100644
>>>>> --- a/drivers/net/phy/aquantia/aquantia_main.c
>>>>> +++ b/drivers/net/phy/aquantia/aquantia_main.c
>>>>> @@ -499,6 +499,12 @@ static int aqr107_config_init(struct phy_device *phydev)
>>>>>  	if (!ret)
>>>>>  		aqr107_chip_info(phydev);
>>>>>  
>>>>> +	/* AQR115c supports speed up to 2.5Gbps */
>>>>> +	if (phydev->interface == PHY_INTERFACE_MODE_2500BASEX) {
>>>>> +		phy_set_max_speed(phydev, SPEED_2500);
>>>>> +		phydev->autoneg = AUTONEG_ENABLE;
>>>>> +	}
>>>>> +
>>>>
>>>> If I get your commit log right, the code above will also apply for
>>>> ASQR107, AQR113 and so on, don't you risk breaking these PHYs if they
>>>> are in 2500BASEX mode at boot?
>>>>
>>>
>>> I was thinking of the same. That this might break something here for other Phy chip. 
>>> As every phy shares the same config init. Hence the reason for RFC. 
>>>
>>>> Besides that, if the PHY switches between SGMII and 2500BASEX
>>>> dynamically depending on the link speed, it could be that it's
>>>> configured by default in SGMII, hence this check will be missed.
>>>>
>>>>
>>> I think the better way is to have AQR115c its own config_init which sets 
>>> the max speed to 2.5Gbps and then call aqr113c_config_init . 
>>
>> phy_set_max_speed(phydev, SPEED_2500) is something a MAC does, not a
>> PHY. It is a way for the MAC to say is supports less than the PHY. I
>> would say the current aqcs109_config_init() is doing this wrong.
> 
> Agreed on two points:
> 
> 1) phy_set_max_speed() is documented as a function that the MAC will
> call.
> 
> 2) calling phy_set_max_speed() in .config_init() is way too late for
> phylink. .config_init() is called from phy_init_hw(), which happens
> after the PHY has been attached. However, phylink needs to know what
> the PHY supports _before_ that, especially for any PHY that is on a
> SFP, so it can determine what interface to use for the PHY.
> 
> So, as Andrew says, the current aqcs109_config_init(), and it seems
> aqr111_config_init() are both broken.
> 
> The PHY driver needs to indicate to phylib what is supported by the
> PHY no later than the .get_features() method.
> 

Noted!. Makes sense. thanks for your review, Russell. 
We are in the process of figuring out what the phy chip is reporting as 
its features. Once done i will raise a clean patch for upstream review. 

> Thanks.
> 

Re: [RFC PATCH net v1] net: phy: aquantia: Set phy speed to 2.5gbps for AQR115c

[Abhishek Chauhan (ABC)]

On 9/17/2024 1:57 PM, Abhishek Chauhan (ABC) wrote:
> 
> 
> On 9/17/2024 2:31 AM, Russell King (Oracle) wrote:
>> On Fri, Sep 13, 2024 at 06:35:17PM +0200, Andrew Lunn wrote:
>>> On Fri, Sep 13, 2024 at 09:12:13AM -0700, Abhishek Chauhan (ABC) wrote:
>>>> On 9/13/2024 1:01 AM, Maxime Chevallier wrote:
>>>>> Hi,
>>>>>
>>>>> On Thu, 12 Sep 2024 18:16:35 -0700
>>>>> Abhishek Chauhan <quic_abchauha@quicinc.com> wrote:
>>>>>
>>>>>> Recently we observed that aquantia AQR115c always comes up in
>>>>>> 100Mbps mode. AQR115c aquantia chip supports max speed up to
>>>>>> 2.5Gbps. Today the AQR115c configuration is done through
>>>>>> aqr113c_config_init which internally calls aqr107_config_init.
>>>>>> aqr113c and aqr107 are both capable of 10Gbps. Whereas AQR115c
>>>>>> supprts max speed of 2.5Gbps only.
>>>>>>
>>>>>> Fixes: 0ebc581f8a4b ("net: phy: aquantia: add support for aqr115c")
>>>>>> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
>>>>>> ---
>>>>>>  drivers/net/phy/aquantia/aquantia_main.c | 7 +++++++
>>>>>>  1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
>>>>>> index e982e9ce44a5..9afc041dbb64 100644
>>>>>> --- a/drivers/net/phy/aquantia/aquantia_main.c
>>>>>> +++ b/drivers/net/phy/aquantia/aquantia_main.c
>>>>>> @@ -499,6 +499,12 @@ static int aqr107_config_init(struct phy_device *phydev)
>>>>>>  	if (!ret)
>>>>>>  		aqr107_chip_info(phydev);
>>>>>>  
>>>>>> +	/* AQR115c supports speed up to 2.5Gbps */
>>>>>> +	if (phydev->interface == PHY_INTERFACE_MODE_2500BASEX) {
>>>>>> +		phy_set_max_speed(phydev, SPEED_2500);
>>>>>> +		phydev->autoneg = AUTONEG_ENABLE;
>>>>>> +	}
>>>>>> +
>>>>>
>>>>> If I get your commit log right, the code above will also apply for
>>>>> ASQR107, AQR113 and so on, don't you risk breaking these PHYs if they
>>>>> are in 2500BASEX mode at boot?
>>>>>
>>>>
>>>> I was thinking of the same. That this might break something here for other Phy chip. 
>>>> As every phy shares the same config init. Hence the reason for RFC. 
>>>>
>>>>> Besides that, if the PHY switches between SGMII and 2500BASEX
>>>>> dynamically depending on the link speed, it could be that it's
>>>>> configured by default in SGMII, hence this check will be missed.
>>>>>
>>>>>
>>>> I think the better way is to have AQR115c its own config_init which sets 
>>>> the max speed to 2.5Gbps and then call aqr113c_config_init . 
>>>
>>> phy_set_max_speed(phydev, SPEED_2500) is something a MAC does, not a
>>> PHY. It is a way for the MAC to say is supports less than the PHY. I
>>> would say the current aqcs109_config_init() is doing this wrong.
>>
>> Agreed on two points:
>>
>> 1) phy_set_max_speed() is documented as a function that the MAC will
>> call.
>>
>> 2) calling phy_set_max_speed() in .config_init() is way too late for
>> phylink. .config_init() is called from phy_init_hw(), which happens
>> after the PHY has been attached. However, phylink needs to know what
>> the PHY supports _before_ that, especially for any PHY that is on a
>> SFP, so it can determine what interface to use for the PHY.
>>
>> So, as Andrew says, the current aqcs109_config_init(), and it seems
>> aqr111_config_init() are both broken.
>>
>> The PHY driver needs to indicate to phylib what is supported by the
>> PHY no later than the .get_features() method.
>>
> 
> Noted!. Makes sense. thanks for your review, Russell. 
> We are in the process of figuring out what the phy chip is reporting as 
> its features. Once done i will raise a clean patch for upstream review. 
> 

Russell and Andrew 

we added prints and understood what the phy is reporting as part of the 
genphy_c45_pma_read_abilities 

[   12.041576] MDIO_STAT2: 0xb301


[   12.050722] MDIO_PMA_EXTABLE: 0x40fc

From the PMA extensible register we see that the phy is reporting that it supports

#define MDIO_PMA_EXTABLE_10GBT		0x0004	/* 10GBASE-T ability */
#define MDIO_PMA_EXTABLE_10GBKX4	0x0008	/* 10GBASE-KX4 ability */
#define MDIO_PMA_EXTABLE_10GBKR		0x0010	/* 10GBASE-KR ability */
#define MDIO_PMA_EXTABLE_1000BT		0x0020	/* 1000BASE-T ability */
#define MDIO_PMA_EXTABLE_1000BKX	0x0040	/* 1000BASE-KX ability */
#define MDIO_PMA_EXTABLE_100BTX		0x0080	/* 100BASE-TX ability */
#define MDIO_PMA_EXTABLE_NBT		0x4000  /* 2.5/5GBASE-T ability */

[   12.060265] MDIO_PMA_NG_EXTABLE: 0x3

/* 2.5G/5G Extended abilities register. */
#define MDIO_PMA_NG_EXTABLE_2_5GBT	0x0001	/* 2.5GBASET ability */
#define MDIO_PMA_NG_EXTABLE_5GBT	0x0002	/* 5GBASET ability */

I feel that the phy here is incorrectly reporting all these abilities as 
AQR115c supports speeds only upto 2.5Gbps 
https://www.marvell.com/content/dam/marvell/en/public-collateral/transceivers/marvell-phys-transceivers-aqrate-gen4-product-brief.pdf

AQR115C / AQR115 Single port, 2.5Gbps / 1Gbps / 100Mbps / 10Mbps 7 x 7 mm / 7 x 11 mm

I feel like get_features for AQR115c is reporting wrong modes and hence the 
link is coming up and is negotiated at 100Mbps. (Misbehavior from AQR115c)

From the MAC perspective we have ensure our max capabilities are 2.5 Gbps 
by setting the below from stmmac driver 
phylink_limit_mac_speed(config, 2500);

I am thinking of solving this problem by having 
custom .get_features in the AQR115c driver to only set supported speeds 
upto 2.5gbps 

let me know what you think ? 
 

>> Thanks.
>>

Re: [RFC PATCH net v1] net: phy: aquantia: Set phy speed to 2.5gbps for AQR115c

[Andrew Lunn]

> Russell and Andrew 
> 
> we added prints and understood what the phy is reporting as part of the 
> genphy_c45_pma_read_abilities 
> 
> [   12.041576] MDIO_STAT2: 0xb301
> 
> 
> [   12.050722] MDIO_PMA_EXTABLE: 0x40fc
> 
> >From the PMA extensible register we see that the phy is reporting that it supports
> 
> #define MDIO_PMA_EXTABLE_10GBT		0x0004	/* 10GBASE-T ability */
> #define MDIO_PMA_EXTABLE_10GBKX4	0x0008	/* 10GBASE-KX4 ability */
> #define MDIO_PMA_EXTABLE_10GBKR		0x0010	/* 10GBASE-KR ability */
> #define MDIO_PMA_EXTABLE_1000BT		0x0020	/* 1000BASE-T ability */
> #define MDIO_PMA_EXTABLE_1000BKX	0x0040	/* 1000BASE-KX ability */
> #define MDIO_PMA_EXTABLE_100BTX		0x0080	/* 100BASE-TX ability */
> #define MDIO_PMA_EXTABLE_NBT		0x4000  /* 2.5/5GBASE-T ability */
> 
> [   12.060265] MDIO_PMA_NG_EXTABLE: 0x3
> 
> /* 2.5G/5G Extended abilities register. */
> #define MDIO_PMA_NG_EXTABLE_2_5GBT	0x0001	/* 2.5GBASET ability */
> #define MDIO_PMA_NG_EXTABLE_5GBT	0x0002	/* 5GBASET ability */
> 
> I feel that the phy here is incorrectly reporting all these abilities as 
> AQR115c supports speeds only upto 2.5Gbps 
> https://www.marvell.com/content/dam/marvell/en/public-collateral/transceivers/marvell-phys-transceivers-aqrate-gen4-product-brief.pdf
> 
> AQR115C / AQR115 Single port, 2.5Gbps / 1Gbps / 100Mbps / 10Mbps 7 x 7 mm / 7 x 11 mm

One things to check. Are you sure you have the correct firmware? Many
of the registers which the standards say should be Read Only can be
influenced by the firmware. So the wrong firmware, or provisioning
taken from another device could result in the wrong capabilities being
set.

You might want to report this issue to Marvell, but my guess would be,
they don't care. I would guess the vendor driver ignores these
registers and simply uses the product ID to determine what the device
actually supports.

> I am thinking of solving this problem by having 
> custom .get_features in the AQR115c driver to only set supported speeds 
> upto 2.5gbps 

Yes, that is the correct solution.

It would also be good if you could, in a separate patch, change the
aqcs109_config_init() to not call phy_set_max_speed() and add a custom
.get_features.

	Andrew

Re: [RFC PATCH net v1] net: phy: aquantia: Set phy speed to 2.5gbps for AQR115c

[Abhishek Chauhan (ABC)]

On 9/18/2024 2:45 PM, Andrew Lunn wrote:
>> Russell and Andrew 
>>
>> we added prints and understood what the phy is reporting as part of the 
>> genphy_c45_pma_read_abilities 
>>
>> [   12.041576] MDIO_STAT2: 0xb301
>>
>>
>> [   12.050722] MDIO_PMA_EXTABLE: 0x40fc
>>
>> >From the PMA extensible register we see that the phy is reporting that it supports
>>
>> #define MDIO_PMA_EXTABLE_10GBT		0x0004	/* 10GBASE-T ability */
>> #define MDIO_PMA_EXTABLE_10GBKX4	0x0008	/* 10GBASE-KX4 ability */
>> #define MDIO_PMA_EXTABLE_10GBKR		0x0010	/* 10GBASE-KR ability */
>> #define MDIO_PMA_EXTABLE_1000BT		0x0020	/* 1000BASE-T ability */
>> #define MDIO_PMA_EXTABLE_1000BKX	0x0040	/* 1000BASE-KX ability */
>> #define MDIO_PMA_EXTABLE_100BTX		0x0080	/* 100BASE-TX ability */
>> #define MDIO_PMA_EXTABLE_NBT		0x4000  /* 2.5/5GBASE-T ability */
>>
>> [   12.060265] MDIO_PMA_NG_EXTABLE: 0x3
>>
>> /* 2.5G/5G Extended abilities register. */
>> #define MDIO_PMA_NG_EXTABLE_2_5GBT	0x0001	/* 2.5GBASET ability */
>> #define MDIO_PMA_NG_EXTABLE_5GBT	0x0002	/* 5GBASET ability */
>>
>> I feel that the phy here is incorrectly reporting all these abilities as 
>> AQR115c supports speeds only upto 2.5Gbps 
>> https://www.marvell.com/content/dam/marvell/en/public-collateral/transceivers/marvell-phys-transceivers-aqrate-gen4-product-brief.pdf
>>
>> AQR115C / AQR115 Single port, 2.5Gbps / 1Gbps / 100Mbps / 10Mbps 7 x 7 mm / 7 x 11 mm
> 
> One things to check. Are you sure you have the correct firmware? Many
> of the registers which the standards say should be Read Only can be
> influenced by the firmware. So the wrong firmware, or provisioning
> taken from another device could result in the wrong capabilities being
> set.
> 

I did check with the hardware team and the firmware loaded is 
AQR-G4_v5.6.7-AQR_Marvell_NoSwap_XFI2500SGMII_ID44842_VER1922.cld
Only Marvell folks can tell me what is inside the FW. 
Let me double check with Marvell on this and ask them why is the phy 
reporting all these PMA capabilities. 

> You might want to report this issue to Marvell, but my guess would be,
> they don't care. I would guess the vendor driver ignores these
> registers and simply uses the product ID to determine what the device
> actually supports.
> 
>> I am thinking of solving this problem by having 
>> custom .get_features in the AQR115c driver to only set supported speeds 
>> upto 2.5gbps 
> 
> Yes, that is the correct solution.

> It would also be good if you could, in a separate patch, change the
> aqcs109_config_init() to not call phy_set_max_speed() and add a custom
> .get_features.
Let me raise this patch in a day or two for upstream review after 
testing it out locally on AQR115c 
> 
> 	Andrew