Re: [PATCH v3 1/3] net: phy: bcm54811: New link mode for BroadR-Reach

["Kamil Horák, 2N" <kamilh@axis.com>]

On 5/23/24 16:27, Andrew Lunn wrote:
>>> 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.
> Great.
>
>>> 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!
> It also seems quite a generic feature. e.g. to select between
> 1000BaseT_FULL and 1000BaseT1_FULL. So it should get reused for other
> use cases.
>
>>> 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.
> That was what i did not know. Does 802.3 allow auto-neg for these
> BroadR-Reach modes at the same time as 'normal' modes. And it seems
> like the ..810 supports is, and i assume it conforms to 802.3? So we
> cannot globally return an error in ethtool_ksetting_set() with a mix
> or modes, it needs to be the driver which decides.

As far I understand it, the chip is not capable of attempting IEEE and 
BroadR-Reach modes at the same time, not even the BCM54810, which is 
capable of autoneg in BRR. One has to choose IEEE or BRR first then 
start the auto-negotiation (or attempt the link with forced 
master-slave/speed setting for BRR). There are two separate "link is up" 
bits, one if the IEEE registers, second in the BRR one. Theoretically, 
it should be possible to have kind of auto-detection of hardware - for 
example start with IEEE, if there is no link after a timeout, try BRR as 
well. But as for the circuitry necessary to do that, there would have to 
be something like hardware plug-in, as I was told by our HW team. In 
other words, it is not probable to have a device capable of both 
(IEEE+BRR) modes at once. Thus, having the driver to choose from a set 
containing IEEE and BRR modes makes little sense.
Our use case is fixed master/slave and fixed speed (10/100), and BRR on 
1 pair only with BCM54811.  I can imagine autoneg master/slave and 
10/100 in the same physical media (one pair) but that would require 
BCM54810.

Back to the ethtool_ksetting_set(), both BCM54810 and 54811 sure support 
the IEEE modes and this would function even with a generic driver. With 
BRR and no autoneg, it seems that if there is one of 10, 100, 1000 
speeds and half or full duplex, it would be sufficient to have 
config_aneg method of the phy driver implemented correctly to do the 
thing (start IEEE (generic) or BRR auto-negotiation, which would include 
set up the PHY for selected link mode and wait for the link to appear). 
Not sure about how many other drivers regularly used fit this scheme, it 
seems that vast majority prefers auto-negotiation... However, it could 
be even made so that direct linkmode selection would work everywhere, 
leaving to the phy driver the choice of whether start autoneg with only 
one option or force that option directly when there is no aneg at all 
(BCM54811 in BRR mode).

>
>     Andrew
Kamil