[PATCH v1] dt-bindings: net: ethernet-phy: Add forced-master/slave properties for SPE PHYs

[PATCH v1] dt-bindings: net: ethernet-phy: Add forced-master/slave properties for SPE PHYs

[Oleksij Rempel]

Add two new properties, `forced-master` and `forced-slave`, to the
ethernet-phy binding. These properties are intended for Single Pair
Ethernet (1000/100/10Base-T1) PHYs, where each PHY and product may have
a predefined link role (master or slave). Typically, these roles are set
by hardware strap pins, but in some cases, device tree configuration is
necessary.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 .../devicetree/bindings/net/ethernet-phy.yaml | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index d9b62741a2259..af7a1eb6ceff6 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -158,6 +158,28 @@ properties:
       Mark the corresponding energy efficient ethernet mode as
       broken and request the ethernet to stop advertising it.

+  forced-master:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      If set, forces the PHY to operate as a master. This is used in Single Pair
+      Ethernet (1000/100/10Base-T1) where each PHY and product has a predefined
+      link role (master or slave). This property is board-specific, as the role
+      is usually configured by strap pins but can be set through the device tree
+      if needed.
+      This property is mutually exclusive with 'forced-slave'; only one of them
+      should be used.
+
+  forced-slave:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      If set, forces the PHY to operate as a slave. This is used in Single Pair
+      Ethernet (1000/100/10Base-T1) where each PHY and product has a predefined
+      link role (master or slave). This property is board-specific, as the role
+      is usually configured by strap pins but can be set through the device tree
+      if needed.
+      This property is mutually exclusive with 'forced-master'; only one of them
+      should be used.
+
   pses:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     maxItems: 1
--
2.39.2

Re: [PATCH v1] dt-bindings: net: ethernet-phy: Add forced-master/slave properties for SPE PHYs

[Andrew Lunn]

On Fri, Sep 06, 2024 at 04:49:05PM +0200, Oleksij Rempel wrote:
> Add two new properties, `forced-master` and `forced-slave`, to the
> ethernet-phy binding. These properties are intended for Single Pair
> Ethernet (1000/100/10Base-T1) PHYs, where each PHY and product may have
> a predefined link role (master or slave). Typically, these roles are set
> by hardware strap pins, but in some cases, device tree configuration is
> necessary.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  .../devicetree/bindings/net/ethernet-phy.yaml | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> index d9b62741a2259..af7a1eb6ceff6 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> @@ -158,6 +158,28 @@ properties:
>        Mark the corresponding energy efficient ethernet mode as
>        broken and request the ethernet to stop advertising it.
> 
> +  forced-master:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      If set, forces the PHY to operate as a master. This is used in Single Pair
> +      Ethernet (1000/100/10Base-T1) where each PHY and product has a predefined
> +      link role (master or slave). This property is board-specific, as the role
> +      is usually configured by strap pins but can be set through the device tree
> +      if needed.
> +      This property is mutually exclusive with 'forced-slave'; only one of them
> +      should be used.

DT reviewers tend to complain about such mutually exclusive
properties.

What you are effectively adding is support for the ethtool:

ethtool -s [master-slave preferred-master|preferred-slave|forced-master|forced-slave]

10Base-T1 often does not have autoneg, so preferred-master &
preferred-slave make non sense in this context, but i wounder if
somebody will want these later. An Ethernet switch is generally
preferred-master for example, but the client is preferred-slave.

Maybe make the property a string with supported values 'forced-master'
and 'forced-slave', leaving it open for the other two to be added
later.

I've not seen the implementation yet, but i don't think there is much
driver specific here. We already have phydev->master_slave_set, it
just needs to be set from this property. Can it be done in phylib core
somewhere?

	Andrew

Re: [PATCH v1] dt-bindings: net: ethernet-phy: Add forced-master/slave properties for SPE PHYs

[Oleksij Rempel]

On Fri, Sep 06, 2024 at 05:11:54PM +0200, Andrew Lunn wrote:
> On Fri, Sep 06, 2024 at 04:49:05PM +0200, Oleksij Rempel wrote:
> > Add two new properties, `forced-master` and `forced-slave`, to the
> > ethernet-phy binding. These properties are intended for Single Pair
> > Ethernet (1000/100/10Base-T1) PHYs, where each PHY and product may have
> > a predefined link role (master or slave). Typically, these roles are set
> > by hardware strap pins, but in some cases, device tree configuration is
> > necessary.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  .../devicetree/bindings/net/ethernet-phy.yaml | 22 +++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > index d9b62741a2259..af7a1eb6ceff6 100644
> > --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > @@ -158,6 +158,28 @@ properties:
> >        Mark the corresponding energy efficient ethernet mode as
> >        broken and request the ethernet to stop advertising it.
> > 
> > +  forced-master:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      If set, forces the PHY to operate as a master. This is used in Single Pair
> > +      Ethernet (1000/100/10Base-T1) where each PHY and product has a predefined
> > +      link role (master or slave). This property is board-specific, as the role
> > +      is usually configured by strap pins but can be set through the device tree
> > +      if needed.
> > +      This property is mutually exclusive with 'forced-slave'; only one of them
> > +      should be used.
> 
> DT reviewers tend to complain about such mutually exclusive
> properties.

Yes, at this point i was uncertain.

> What you are effectively adding is support for the ethtool:
> 
> ethtool -s [master-slave preferred-master|preferred-slave|forced-master|forced-slave]

ack

> 10Base-T1 often does not have autoneg, so preferred-master &
> preferred-slave make non sense in this context, but i wounder if
> somebody will want these later. An Ethernet switch is generally
> preferred-master for example, but the client is preferred-slave.

Good point.

> Maybe make the property a string with supported values 'forced-master'
> and 'forced-slave', leaving it open for the other two to be added
> later.
> 
> I've not seen the implementation yet, but i don't think there is much
> driver specific here. We already have phydev->master_slave_set, it
> just needs to be set from this property. Can it be done in phylib core
> somewhere?

Yes, this is the idea.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1] dt-bindings: net: ethernet-phy: Add forced-master/slave properties for SPE PHYs

[Maxime Chevallier]

On Fri, 6 Sep 2024 17:11:54 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

[...]

> 10Base-T1 often does not have autoneg, so preferred-master &
> preferred-slave make non sense in this context, but i wounder if
> somebody will want these later. An Ethernet switch is generally
> preferred-master for example, but the client is preferred-slave.
> 
> Maybe make the property a string with supported values 'forced-master'
> and 'forced-slave', leaving it open for the other two to be added
> later.

My two cents, don't take it as a nack or any strong disagreement, my
experience with SPE is still limited. I agree that for SPE, it's
required that PHYs get their role assigned as early as possible,
otherwise the link can't establish. I don't see any other place but DT
to put that info, as this would be required for say, booting over the
network. This to me falls under 'HW representation', as we could do the
same with straps.

However for preferred-master / preferred-slave, wouldn't we be crossing
the blurry line of "HW description => system configuration in the DT" ?

Maxime

Re: [PATCH v1] dt-bindings: net: ethernet-phy: Add forced-master/slave properties for SPE PHYs

[Andrew Lunn]

> > 10Base-T1 often does not have autoneg, so preferred-master &
> > preferred-slave make non sense in this context, but i wounder if
> > somebody will want these later. An Ethernet switch is generally
> > preferred-master for example, but the client is preferred-slave.
> > 
> > Maybe make the property a string with supported values 'forced-master'
> > and 'forced-slave', leaving it open for the other two to be added
> > later.
> 
> My two cents, don't take it as a nack or any strong disagreement, my
> experience with SPE is still limited. I agree that for SPE, it's
> required that PHYs get their role assigned as early as possible,
> otherwise the link can't establish. I don't see any other place but DT
> to put that info, as this would be required for say, booting over the
> network. This to me falls under 'HW representation', as we could do the
> same with straps.
> 
> However for preferred-master / preferred-slave, wouldn't we be crossing
> the blurry line of "HW description => system configuration in the DT" ?

Yes, we are somewhere near the blurry line. This is why i gave the
example of an Ethernet switch, vs a client. Again, it could be done
with straps, so following your argument, it could be considered HW
representation. But if it is set wrong, it probably does not matter,
auto-neg should still work. Except for a very small number of PHYs
whos random numbers are not random...

But this is also something we don't actually need to resolve now. The
design allows for it, but we don't really need to decided if it is
acceptable until somebody actually posts a patch.

	Andrew

Re: [PATCH v1] dt-bindings: net: ethernet-phy: Add forced-master/slave properties for SPE PHYs

[Florian Fainelli]

On 9/6/24 09:11, Andrew Lunn wrote:
>>> 10Base-T1 often does not have autoneg, so preferred-master &
>>> preferred-slave make non sense in this context, but i wounder if
>>> somebody will want these later. An Ethernet switch is generally
>>> preferred-master for example, but the client is preferred-slave.
>>>
>>> Maybe make the property a string with supported values 'forced-master'
>>> and 'forced-slave', leaving it open for the other two to be added
>>> later.
>>
>> My two cents, don't take it as a nack or any strong disagreement, my
>> experience with SPE is still limited. I agree that for SPE, it's
>> required that PHYs get their role assigned as early as possible,
>> otherwise the link can't establish. I don't see any other place but DT
>> to put that info, as this would be required for say, booting over the
>> network. This to me falls under 'HW representation', as we could do the
>> same with straps.
>>
>> However for preferred-master / preferred-slave, wouldn't we be crossing
>> the blurry line of "HW description => system configuration in the DT" ?
> 
> Yes, we are somewhere near the blurry line. This is why i gave the
> example of an Ethernet switch, vs a client. Again, it could be done
> with straps, so following your argument, it could be considered HW
> representation. But if it is set wrong, it probably does not matter,
> auto-neg should still work. Except for a very small number of PHYs
> whos random numbers are not random...

Having had to deal with an Ethernet PHY that requires operating in slave 
mode "preferably" in order to have a correct RXC duty cycle, if you 
force both sides of the link to "slave", auto-negotiation will fail, 
however thanks to auto-negotiation you can tell that there was a 
master/slave resolution failure. (This reminds me I need to send the 
patch for that PHY errata at some point).

In the case that Oleksij seems to be after, there is no auto-negotiation 
(is that correct?), so it seems to me that the Device Tree is coming to 
the rescue of an improperly strapped HW, and is used as a way to change 
the default HW configuration so as to have a fighting chance of having a 
functional link. That is not unprecedented, but it is definitively a bit 
blurry...
-- 
Florian

Re: [PATCH v1] dt-bindings: net: ethernet-phy: Add forced-master/slave properties for SPE PHYs

[Oleksij Rempel]

On Fri, Sep 06, 2024 at 09:22:29AM -0700, Florian Fainelli wrote:
> On 9/6/24 09:11, Andrew Lunn wrote:
> > > > 10Base-T1 often does not have autoneg, so preferred-master &
> > > > preferred-slave make non sense in this context, but i wounder if
> > > > somebody will want these later. An Ethernet switch is generally
> > > > preferred-master for example, but the client is preferred-slave.
> > > > 
> > > > Maybe make the property a string with supported values 'forced-master'
> > > > and 'forced-slave', leaving it open for the other two to be added
> > > > later.
> > > 
> > > My two cents, don't take it as a nack or any strong disagreement, my
> > > experience with SPE is still limited. I agree that for SPE, it's
> > > required that PHYs get their role assigned as early as possible,
> > > otherwise the link can't establish. I don't see any other place but DT
> > > to put that info, as this would be required for say, booting over the
> > > network. This to me falls under 'HW representation', as we could do the
> > > same with straps.
> > > 
> > > However for preferred-master / preferred-slave, wouldn't we be crossing
> > > the blurry line of "HW description => system configuration in the DT" ?
> > 
> > Yes, we are somewhere near the blurry line. This is why i gave the
> > example of an Ethernet switch, vs a client. Again, it could be done
> > with straps, so following your argument, it could be considered HW
> > representation. But if it is set wrong, it probably does not matter,
> > auto-neg should still work. Except for a very small number of PHYs
> > whos random numbers are not random...
> 
> Having had to deal with an Ethernet PHY that requires operating in slave
> mode "preferably" in order to have a correct RXC duty cycle, if you force
> both sides of the link to "slave", auto-negotiation will fail, however
> thanks to auto-negotiation you can tell that there was a master/slave
> resolution failure. (This reminds me I need to send the patch for that PHY
> errata at some point).
> 
> In the case that Oleksij seems to be after, there is no auto-negotiation (is
> that correct?), so it seems to me that the Device Tree is coming to the
> rescue of an improperly strapped HW, and is used as a way to change the
> default HW configuration so as to have a fighting chance of having a
> functional link. That is not unprecedented, but it is definitively a bit
> blurry...

Yes, there is no auto-negotiation on T1 PHY variants, so the DT property is
to fix broken or not existing for some reason HW straps.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |