Date: Mon, 27 May 2024 15:20:39 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Kamil Horák, 2N <kamilh@...s.com>
Cc: netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v3 1/3] net: phy: bcm54811: New link mode for BroadR-Reach

> > 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.

> 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.

> > 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.

> 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.

	Andrew

Powered by blists - more mailing lists