Re: [PATCH net-next v2 1/2] dt-bindings: net: ethernet-phy: Add master-slave role property for SPE PHYs

[Rob Herring]

On Mon, Sep 9, 2024 at 12:00 PM Andrew Lunn <andrew@xxxxxxx> wrote:
>
> On Mon, Sep 09, 2024 at 11:20:09AM -0500, Rob Herring wrote:
> > On Mon, Sep 09, 2024 at 02:43:40PM +0200, Oleksij Rempel wrote:
> > > Introduce a new `master-slave` string property in the ethernet-phy
> > > binding to specify the link role for Single Pair Ethernet
> > > (1000/100/10Base-T1) PHYs. This property supports the values
> > > `forced-master` and `forced-slave`, which allow the PHY to operate in a
> > > predefined role, necessary when hardware strap pins are unavailable or
> > > wrongly set.
> > >
> > > Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> > > ---
> > > changes v2:
> > > - use string property instead of multiple flags
> > > ---
> > >  .../devicetree/bindings/net/ethernet-phy.yaml      | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > > index d9b62741a2259..025e59f6be6f3 100644
> > > --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > > +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > > @@ -158,6 +158,20 @@ properties:
> > >        Mark the corresponding energy efficient ethernet mode as
> > >        broken and request the ethernet to stop advertising it.
> > >
> > > +  master-slave:
> >
> > Outdated terminology and kind of vague what it is for...
> >
> > The usual transformation to 'controller-device' would not make much
> > sense though. I think a better name would be "spe-link-role" or
> > "spe-link-mode".
>
> This applies to more than Single Pair Ethernet. This property could
> also be used for 2 and 4 pair cables. So spe-link-mode would be wrong.

I kind of figured that... Propose something that's not just
duplicating possible values.

>
> Also:
>
> https://grouper.ieee.org/groups/802/3/dc/comments/P8023_D2p0_comments_final_by_cls.pdf
>
> On 3 December 2020, the IEEE SA Standard Board passed the following resolution. (See
> <https://standards.ieee.org/about/sasb/resolutions.html>.)
>
>   "IEEE standards (including recommended practices and guides) shall
>   be written in such a way as to unambiguously communicate the
>   technical necessities, preferences, and options of the standard to
>   best enable market adoption, conformity assessment,
>   interoperability, and other technical aspirations of the developing
>   standards committee. IEEE standards should be written in such a way
>   as to avoid non-inclusive and insensitive terminology (see IEEE
>   Policy 9.27) and other deprecated terminology (see clause 10 of the
>   IEEE SA Style Manual) except when required by safety, legal,
>   regulatory, and other similar considerations.  Terms such as
>   master/slave, blacklist, and whitelist should be avoided."
>
>   In IEEE Std 802.3, 1000BASE-T, 10BASE-T1L, 100BASE-T1, 1000BASE-T1,
>   and MultiGBASE-T PHYs use the terms "master" and "slave" to indicate
>   whether the clock is derived from an external source or from the
>   received signal. In these cases, the terms appear in the text,
>   figures, state names, variable names, register/bit names, etc. A
>   direct substitution of terms will create disconnects between the
>   standard and the documentation for devices in the field (e.g., the
>   register interface) and also risks the introduction of technical
>   errors. Note that "master" and "slave" are also occasionally used to
>   describe the relationship between an ONT and an ONU for EPON and
>   between a CNT and a CNU for EPoC.
>
>   The approach that other IEEE standards are taking to address this
>   issue have been considered. For example, IEEE P1588g proposes to
>   define "optional alternative suitable and inclusive terminology" but
>   not replace the original terms. (See
>   <https://development.standards.ieee.org/myproject-web/public/view.html#pardetail/8858>.)
>   It is understood that an annex to the IEEE 1588 standard has been
>   proposed that defines the inclusive terminology. It is also
>   understood that the inclusive terminology has been chosen to be
>   "leader" and "follower".
>
>   The IEEE P802.1ASdr project proposes to align to the IEEE P1588g
>   inclusive terminology.  (See
>   <https://development.standards.ieee.org/myprojectweb/public/view.html#pardetail/9009>.)
>   Based on this, it seems reasonable to include an annex that defines
>   optional alternative inclusive terminology and, for consistency, to
>   use the terms "leader" and "follower" as the inclusive terminology
>
> The 2022 revision of 802.3 still has master/slave when describing the
> registers, but it does have Annex K as described above saying "leader"
> and "follower" are optional substitutions.
>
> The Linux code has not changed, and the uAPI has not changed. It seems
> like the best compromise would be to allow both 'force-master' and
> 'force-leader', as well as 'force-slave' and 'force-follower', and a
> reference to 802.3 Annex K.

It seems silly to maintain both forever. I'd rather have one or the
other than both.

> As to you comment about it being unclear what it means i would suggest
> a reference to 802.3 section 1.4.389:
>
>   1.4.389 master Physical Layer device (PHY): Within IEEE 802.3, in a
>   100BASE-T2, 1000BASE-T, 10BASE-T1L, 100BASE-T1, 1000BASE-T1, or any
>   MultiGBASE-T link containing a pair of PHYs, the PHY that uses an
>   external clock for generating its clock signals to determine the
>   timing of transmitter and receiver operations. It also uses the
>   master transmit scrambler generator polynomial for side-stream
>   scrambling. Master and slave PHY status is determined during the
>   Auto-Negotiation process that takes place prior to establishing the
>   transmission link, or in the case of a PHY where Auto-Negotiation is
>   optional and not used, master and slave PHY status

phy-status? Shrug.

Another thought. Is it possible that h/w strapping disables auto-neg,
but you actually want to override that and force auto-neg?

Rob