Date: Tue, 28 May 2024 16:47:01 +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/27/24 15:20, Andrew Lunn wrote:
>>> 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.
> CLause 22 and clause 45 might say something. e.g. the documentation
> about BMSR_ANEGCAPABLE might indicate what link modes it covers.
There is nothing new in IEEE 802.3 clause 22, compared to what can be 
found in a datasheet of any PHY complying the standard... As for Clause 
45, I'd say that it does not handle the BRR case, nor the 100Base-T1 aka 
1BR100.
>
>> 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"....
> For generic code, we should go from what 802.3 says. Does clause 22 or
> clause 45 define anything like LDS Ability? If you look at how 802.3
> C22/C45 works, it is mostly self describing. You can read registers to
> determine what the PHY supports. So it is possible to have generic
> genphy_read_abilities() and genphy_c45_pma_read_abilities which does
> most of the work. Ideally we just want to extend them to cover BBR
> link modes.
>
This sounds to me like we should not rely on common properties of IEEE 
and BRR register sets and rather implement it separately for the 
BroadR-Reach mode.

In other words, on initialization, decide whether there will be IEEE or 
BRR mode and behave according to that. The IEEE mode is already 
implemented in current state of Broadcom PHY library and broadcom.c and 
it does not nothing special in addition to make sure that the BRR mode 
is off. The rest is IEEE compatible.

If there were fully separated handling of IEEE and BRR, it would be 
difficult to do IEEE/BRR auto-detection or even try to issue aneg start 
command in both modes at once then check which one succeeds. However, 
this is not I would like to implement anyway (also for lack of hardware 
capable of doing so).

All code in bcm-phy-lib should handle PHY in LRE (or BRR) mode. For 
example, bcm54811_config_aneg in my last patch version basically calls 
bcm_config_aneg or genphy_config_aneg based on whether the PHY is in BRR 
or IEEE mode. The bcm_config_aneg then calls some genphy_... functions 
and thus relies on the fact that the LRE (BRR mode) registers do mostly 
the same as IEEE. This should probably be avoided and all control that 
can be done in the LRE register set only be done there and of course use 
the register definitions from brcmphy.h (LRECR_RESET to reset the chip, 
although it is same bit 15 as BMCR_RESET in Basic mode control register 
etc.). Only this shall be a pure solution.

For example, regarding the auto-negotiation, in BRR mode it shall mean 
LDS, in IEEE mode the usual auto-negotiation as described in IEEE802.3.

>>> 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.
> If we decide IEEE and BRR are mutually exclusive because of the
> coupling, then this is clearly a hardware property. So DT, and maybe
> sometime in the future ACPI, is the correct way to describe this.

yes, see previous paragraph - IEEE and BRR to be mutually exclusive, 
neglect the possibility existing in some chips to do kind of 
super-auto-negotiation and thus make the chip to detect the type of 
connected physical network. I cannot test it and anyway, the BCM54810 
(with BRR aneg) seems to be deprecated in favor of BCM54811 (no BRR 
aneg, or at least not documented).

For our use case this is irrelevant, we have fixed master-slave and 
speed selection.

>
>> 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.
> That is fine. At the moment, we are just trying to explore all the
> corners before we decide how this should work. 802.3 should be our
> main guide, but also look at real hardware.
>
>> 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?
> The autoneg off should be enough to indicate what the passed link mode
> means. However, it could also be placed into a new property it that
> seems more logical for the API. When it comes to the internal API, i
> think it will be a new member anyway.
OK then the only change to ethtool itself would be adding the 
possibility of eg. "linkmode 100BaseT1/Full", let the other end process 
it together with other parameters such as master-slave etc.
>
> 	Andrew

So now, maybe it's time to try to implement the solution discussed above 
and try another patch version...?


Kamil

Powered by blists - more mailing lists