Message-ID: <b9ce037f-8720-4a6c-8cfe-01bffee230c1@axis.com>
Date: Mon, 27 May 2024 13:37:05 +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/25/24 19:12, Andrew Lunn wrote:
>> 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.
> So IEEE and BRR autoneg are mutually exclusive. It would be good to
> see if 802.3 actually says or implies that. Generic functions like
> ksetting_set/get should be based on 802.3, so when designing the API
> we should focus on that, not what the particular devices you are
> interested in support.
I am not sure about how to determine whether IEEE 802.3 says anything 
about the IEEE and BRR modes auto-negotiation mutual exclusivity - it is 
purely question of the implementation, in our case in the Broadcom PHYs. 
One of the BRR modes (1BR100) is direct equivalent of 100Base-T1 as 
specified in IEEE 802.3bw. As it requests different hardware to be 
connected, I doubt there is any (even theoretical) possibility to 
negotiate with a set of supported modes including let's say 100Base-T1 
and 100Base-T.
>
> We probably want phydev->supports listing all modes, IEEE and BRR. Is
> there a bit equivalent to BMSR_ANEGCAPABLE indicating the hardware can
> do BRR autoneg? If there is, we probably want to add a
> ETHTOOL_LINK_MODE_Autoneg_BRR_BIT.
There is "LDS Ability" (LRESR_LDSABILITY) bit in the LRE registers set 
of BCM54810, which is equivalent to BMSR_ANEGCAPABLE and it is at same 
position (bit 3 of the status register), so that just this could work.

But just in our case, the LDS Ability bit is "reserved" and "reads as 1" 
(BCM54811, BCM54501). So at least for these two it cannot be used as an 
indication of aneg capability.

LDS is "long-distance signaling" int he Broadcom's terminology, "a 
special new type of auto-negotiation"....

>
> ksetting_set should enforce this mutual exclusion. So
> phydev->advertise should never be set containing invalid combination,
> ksetting_set() should return an error.
>
> I guess we need to initialize phydev->advertise to IEEE link modes in
> order to not cause regressions. However, if the PHY does not support
> any IEEE modes, it can then default to BRR link modes. It would also
> make sense to have a standardized DT property to indicate BRR should
> be used by default.

With device tree property it would be about the same situation as with 
phy tunable, wouldn't? The tunable was already in the first version of 
this patch and it (or DT property) is same type of solution, one knows 
in advance which set of link modes to use. I personally feel the DT as 
better method, because the IEEE/BRR selection is of hardware nature and 
cannot be easily auto-detected - exactly what the DT is for.

There is description of the LDS negotioation in BCM54810 datasheet 
saying that if the PHY detects standard Ethernet link pulses on a wire 
pair, it transitions automatically from BRR-LDS to Clause 28 
auto-negotioation mode. Thus, at least the 54810 can be set so that it 
starts in BRR mode and if there is no BRR PHY at the other end and the 
other end is also set to auto-negotiate (Clause-28), the 
auto-negotiation continues in IEEE mode and potentially results in the 
PHY in IEEE mode. In this case, it would make sense to have both BRR and 
IEEE link modes in same list and just start with BRR, leaving on the PHY 
itself the decision to fall back to IEEE. The process would be 
sub-optimal in most use cases - who would use BRR PHY in hardwired IEEE 
circuit..?

However, I cannot promise to do such a driver because I do not have the 
BCM54810 available nor it is my task here.

>
>> 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.
>   
>
>
>> 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).
> No, this is not correct. There is a difference between autoneg with a
> single mode, and forced. forced does not attempt to perform autoneg,
> the hardware is just configured to a particular link mode. autoneg
> with a single link mode does perform autoneg, you get to see what the
> link partner is advertising etc. Either you can resolve to a common
> link code and autoneg is successful, or there are no common modes and
> autoneg fails.
>
> A driver does not have a choice here, it need to do what it is told to
> do.

OK so back to the proposed new parameter for ethtool, the "linkmode" 
would mean forced setting of  given link mode - so use the 
link_mode_masks as 1 of N or just pass the link mode number as another 
parameter?

For the BRR, this forced setting includes the duplex option (always 
full) but still requires additional parameter to determine master/slave 
(likely using the command - eg. "master-slave forced-slave")

For IEEE modes (or any other supported modes on other PHYs) this would 
set speed and duplex as requested.

>
> 	Andrew
Kamil

Powered by blists - more mailing lists