Message-ID: <c39dd894-bd63-430b-a60c-402c04f5dbf7@axis.com>
Date: Thu, 23 May 2024 12:23:58 +0200
From: Kamil Horák, 2N <kamilh@...s.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v3 1/3] net: phy: bcm54811: New link mode for BroadR-Reach

On 5/22/24 16:05, Andrew Lunn wrote:
> On Wed, May 22, 2024 at 09:34:05AM +0200, Kamil Horák, 2N wrote:
>> On 5/6/24 21:14, Andrew Lunn wrote:
>>> On Mon, May 06, 2024 at 04:40:13PM +0200, Kamil Horák - 2N wrote:
>>>> Introduce new link modes necessary for the BroadR-Reach mode on
>>>> bcm5481x PHY by Broadcom and new PHY tunable to choose between
>>>> normal (IEEE) ethernet and BroadR-Reach modes of the PHY.
>>> I would of split this into two patches. The reason being, we need the
>>> new link mode. But do we need the tunable? Why don't i just use the
>>> link mode to select it?
>>>
>>> ethtool -s eth42 advertise 1BR10
>> Tried to find a way to do the link mode selection this way but the
>> advertised modes are only applicable when there is auto-negotiation, 
>> which
>> is only partially the case of BCM54811: it only has auto-negotiation 
>> in IEEE
>> mode.
>> Thus, to avoid choosing between BroadR-Reach and IEEE mode using the PHY
>> Tunable, we would need something else and I am already running out of
>> ideas...
>> Is there any other possibility?
>>
>> In addition, we would have to check for incompatible link modes 
>> selected to
>> advertise (cannot choose one BRR and one IEEE mode to advertise), or 
>> perhaps
>> the BRR modes would take precedence, if there is any BRR mode 
>> selected to
>> advertise, IEEE modes would be ignored.
> So it sounds like multiple problems.
>
> 1) Passing a specific link mode when not using auto-neg. The current
> API is:
>
> ethtool -s eth42 autoneg off speed 10 duplex full
>
> Maybe we want to extend that with
>
> ethtool -s eth42 autoneg off speed 10 duplex full linkmode 1BR10
>
> or just
>
> ethtool -s eth42 autoneg off linkmode 1BR10

This sounds perfect to me. The second (shorter) way is better because, 
at least with BCM54811, given the link mode, the duplex and speed are 
also determined. All BroadR-Reach link modes are full duplex, anyway.

>
> You can probably add a new member to ethtool_link_ksettings to pass it
> to phylib. From there, it probably needs putting into a phy_device
> member, next to speed and duplex. The PHY driver can then use it to
> configure the hardware.
I did not dare to cut this deep so far, but as I see there is a demand, 
let's go for it!
>
> 2) Invalid combinations of link modes when auto-neg is
> enabled. Probably the first question to answer is, is this specific to
> this PHY, or generic across all PHYs which support BR and IEEE
> modes. If it is generic, we can add tests in
> phy_ethtool_ksettings_set() to return EINVAL. If this is specific to
> this PHY, it gets messy. When phylib call phy_start_aneg() to
> configure the hardware, it does not expect it to return an error. We
> might need to add an additional op to phy_driver to allow the PHY
> driver to validate settings when phy_ethtool_ksettings_set() is
> called. This would help solve a similar problem with a new mediatek
> PHY which is broken with forced modes.
Regarding the specificity, it definitely touches the BCM54811 and even 
more BCM54810, because the ...810 supports autoneg  in BroadR-Reach mode 
too.
I unfortunately do not have any device using BCM54810 (not even a 
devkit) available to test it, thus I only can do the BCM54811 driver.

With BCM54811 and considering no explicit BRR / IEEE switching (as it 
was originally intended by adding a tunable), we definitely have to 
check the set of selected link modes for applicability and return (and 
handle)  the error caused by impossible combination. Originally, I 
thought about abusing the autoneg with only one mode to select that mode 
without negotiation but that would apply only to BCM54811.
Thus, for BCM54811 I suggest to ignore BRR modes if there is at least 
one IEEE one in the set and report an error for not applicable set (BRR 
modes only).  For BCM54810 we should accept only sets consisting of link 
modes of same type (IEEE or BRR) and switch between IEEE and BRR as 
needed, but this would be likely a task for someone else. I do not have 
the hardware at hand. I'll do it with anticipation of such possibility, 
right?

>
>      Andrew


Kamil

Powered by blists - more mailing lists