[1/2] dt-bindings: net: Add TI DP83640

  The TI DP83640 is a PTP PHY. Some of his features can be enabled by
hardware straps or by registers configuration. Add a device tree
binding for configuration through registers

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 .../devicetree/bindings/net/ti,dp83640.yaml   | 113 ++++++++++++++++++
 include/dt-bindings/net/ti-dp83640.h          |  18 +++
 2 files changed, 131 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ti,dp83640.yaml
 create mode 100644 include/dt-bindings/net/ti-dp83640.h
  

> +  ti,led-config:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 3]
> +    description: |
> +      If present, configures the LED Mode (values defined in
> +      dt-bindings/net/ti-dp83640.h).
> +      LED configuration can also be strapped. If the strap pin is not set
> +      correctly or not set at all then this can be used to configure it.
> +       - 1     = Mode 1
> +        LED_LINK = ON for Good Link, OFF for No Link
> +        LED_SPEED = ON in 100 Mb/s, OFF in 10 Mb/s
> +        LED_ACT = ON for Activity, OFF for No Activity
> +       - 2     = Mode 2
> +        LED_LINK = ON for Good Link, BLINK for Activity
> +        LED_SPEED = ON in 100 Mb/s, OFF in 10 Mb/s
> +        LED_ACT = ON for Collision, OFF for No Collision
> +       - 3     = Mode 3
> +        LED_LINK = ON for Good Link, BLINK for Activity
> +        LED_SPEED = ON in 100 Mb/s, OFF in 10 Mb/s
> +        LED_ACT = ON for Full Duplex, OFF for Half Duplex
> +       - unset = Configured by straps

Please look at have the Marvell PHY driver supports LEDs via
/sys/class/leds. Now we have a generic way to supports LEDs, DT
properties like this will not be accepted.

> +
> +  ti,phy-control-frames:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]
> +    description: |
> +      If present, enables or disables the PHY control frames.
> +      PHY Control Frames support can also be strapped. If the strap pin is not
> +      set correctly or not set at all then this can be used to configure it.
> +       - 0     = PHY Control Frames disabled
> +       - 1     = PHY Control Frames enabled
> +       - unset = Configured by straps

What is a control frame?

> +
> +  ti,energy-detect-en:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: |
> +      If present, Energy Detect Mode is enabled. If not present, Energy Detect
> +      Mode is disabled. This feature can not be strapped.

Please use the phy tunable ETHTOOL_PHY_EDPD. There are a few examples
you can copy.

    Andrew
  

Hey,

On Tue, Jan 30, 2024 at 09:59:34AM +0100, Bastien Curutchet wrote:
> +description: |
> +  The DP83640 Precision PHYTER device delivers the highest level of precision

This is not a marketing document.

> +  clock synchronization for real time industrial connectivity based on the
> +  IEEE 1588 standard. The DP83640 has deterministic, low latency and allows
> +  choice of microcontroller with no hardware customization required
> +
> +  This device interfaces directly to the MAC layer through the
> +  IEEE 802.3 Standard Media Independent Interface (MII), or Reduced MII (RMII).
> +
> +  Specifications about the Ethernet PHY can be found at:
> +    https://www.ti.com/lit/gpn/dp83640
> +
> +properties:
> +  reg:
> +    maxItems: 1
> +
> +  ti,clk-output:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]
> +    description: |
> +      If present, enables or disables the CLK_OUT pin.
> +      CLK_OUT pin disabling can also be strapped. If the strap pin is not set
> +      correctly or not set at all then this can be used to configure it.
> +       - 0     = CLK_OUT pin disabled
> +       - 1     = CLK_OUT pin enabled
> +       - unset = Configured by straps

If you are providing a clock, why is there no clock-controller property
here? I don't think the 3-way nature of this property is needed, if you
make this a "real" clock controller.

> +  ti,fiber-mode:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]
> +    description: |
> +      If present, enables or disables the FX Fiber Mode.
> +      Fiber mode support can also be strapped. If the strap pin is not set
> +      correctly or not set at all then this can be used to configure it.
> +       - 0     = FX Fiber Mode disabled
> +       - 1     = FX Fiber Mode enabled
> +       - unset = Configured by straps

I don't like these properties that map meanings onto numbers. We can
have enums of strings in bindings that allow you to use something more
meaningful than "0" or "1".

Cheers,
Conor.
  

On Tue, Jan 30, 2024 at 05:56:37PM +0000, Conor Dooley wrote:
> Hey,
> 
> On Tue, Jan 30, 2024 at 09:59:34AM +0100, Bastien Curutchet wrote:
> > +description: |
> > +  The DP83640 Precision PHYTER device delivers the highest level of precision
> 
> This is not a marketing document.
> 
> > +  clock synchronization for real time industrial connectivity based on the
> > +  IEEE 1588 standard. The DP83640 has deterministic, low latency and allows
> > +  choice of microcontroller with no hardware customization required
> > +
> > +  This device interfaces directly to the MAC layer through the
> > +  IEEE 802.3 Standard Media Independent Interface (MII), or Reduced MII (RMII).
> > +
> > +  Specifications about the Ethernet PHY can be found at:
> > +    https://www.ti.com/lit/gpn/dp83640
> > +
> > +properties:
> > +  reg:
> > +    maxItems: 1
> > +
> > +  ti,clk-output:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1]
> > +    description: |
> > +      If present, enables or disables the CLK_OUT pin.
> > +      CLK_OUT pin disabling can also be strapped. If the strap pin is not set
> > +      correctly or not set at all then this can be used to configure it.
> > +       - 0     = CLK_OUT pin disabled
> > +       - 1     = CLK_OUT pin enabled
> > +       - unset = Configured by straps
> 
> If you are providing a clock, why is there no clock-controller property
> here? I don't think the 3-way nature of this property is needed, if you
> make this a "real" clock controller.
> 
> > +  ti,fiber-mode:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1]
> > +    description: |
> > +      If present, enables or disables the FX Fiber Mode.
> > +      Fiber mode support can also be strapped. If the strap pin is not set
> > +      correctly or not set at all then this can be used to configure it.
> > +       - 0     = FX Fiber Mode disabled
> > +       - 1     = FX Fiber Mode enabled
> > +       - unset = Configured by straps
> 
> I don't like these properties that map meanings onto numbers. We can
> have enums of strings in bindings that allow you to use something more
> meaningful than "0" or "1".

Tristate properties are fairly common pattern where we need 
on/off/default. I've thought about making it a type. I don't think we 
need defines for it.

Rob
  

On Wed, Jan 31, 2024 at 03:05:21PM -0600, Rob Herring wrote:
> On Tue, Jan 30, 2024 at 05:56:37PM +0000, Conor Dooley wrote:
> > On Tue, Jan 30, 2024 at 09:59:34AM +0100, Bastien Curutchet wrote:

> > > +  ti,fiber-mode:
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [0, 1]
> > > +    description: |
> > > +      If present, enables or disables the FX Fiber Mode.
> > > +      Fiber mode support can also be strapped. If the strap pin is not set
> > > +      correctly or not set at all then this can be used to configure it.
> > > +       - 0     = FX Fiber Mode disabled
> > > +       - 1     = FX Fiber Mode enabled
> > > +       - unset = Configured by straps
> > 
> > I don't like these properties that map meanings onto numbers. We can
> > have enums of strings in bindings that allow you to use something more
> > meaningful than "0" or "1".
> 
> Tristate properties are fairly common pattern where we need 
> on/off/default. I've thought about making it a type. I don't think we 
> need defines for it.

I think a type would be a good idea. I am not at all a fan of any of the
properties people introduce along these lines.
  

On Wed, Jan 31, 2024 at 09:18:39PM +0000, Conor Dooley wrote:
> On Wed, Jan 31, 2024 at 03:05:21PM -0600, Rob Herring wrote:
> > On Tue, Jan 30, 2024 at 05:56:37PM +0000, Conor Dooley wrote:
> > > On Tue, Jan 30, 2024 at 09:59:34AM +0100, Bastien Curutchet wrote:
> 
> > > > +  ti,fiber-mode:
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    enum: [0, 1]
> > > > +    description: |
> > > > +      If present, enables or disables the FX Fiber Mode.
> > > > +      Fiber mode support can also be strapped. If the strap pin is not set
> > > > +      correctly or not set at all then this can be used to configure it.
> > > > +       - 0     = FX Fiber Mode disabled
> > > > +       - 1     = FX Fiber Mode enabled
> > > > +       - unset = Configured by straps
> > > 
> > > I don't like these properties that map meanings onto numbers. We can
> > > have enums of strings in bindings that allow you to use something more
> > > meaningful than "0" or "1".
> > 
> > Tristate properties are fairly common pattern where we need 
> > on/off/default. I've thought about making it a type. I don't think we 
> > need defines for it.
> 
> I think a type would be a good idea. I am not at all a fan of any of the
> properties people introduce along these lines.

Before going too far with that, i'm not actually sure it is required
here. I've not looked at the PHY driver itself, but i expect there is
some indication somewhere that the network stack expects a fibre link
is to be used. We probably can determine at runtime if fibre should be
used.

	Andrew
  

Hi Andrew,


Thank you for your feedback.

On 1/30/24 14:34, Andrew Lunn wrote:
>> +  ti,led-config:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [1, 2, 3]
>> +    description: |
>> +      If present, configures the LED Mode (values defined in
>> +      dt-bindings/net/ti-dp83640.h).
>> +      LED configuration can also be strapped. If the strap pin is not set
>> +      correctly or not set at all then this can be used to configure it.
>> +       - 1     = Mode 1
>> +        LED_LINK = ON for Good Link, OFF for No Link
>> +        LED_SPEED = ON in 100 Mb/s, OFF in 10 Mb/s
>> +        LED_ACT = ON for Activity, OFF for No Activity
>> +       - 2     = Mode 2
>> +        LED_LINK = ON for Good Link, BLINK for Activity
>> +        LED_SPEED = ON in 100 Mb/s, OFF in 10 Mb/s
>> +        LED_ACT = ON for Collision, OFF for No Collision
>> +       - 3     = Mode 3
>> +        LED_LINK = ON for Good Link, BLINK for Activity
>> +        LED_SPEED = ON in 100 Mb/s, OFF in 10 Mb/s
>> +        LED_ACT = ON for Full Duplex, OFF for Half Duplex
>> +       - unset = Configured by straps
> Please look at have the Marvell PHY driver supports LEDs via
> /sys/class/leds. Now we have a generic way to supports LEDs, DT
> properties like this will not be accepted.
Ok I'll use /sys/class/leds
>> +
>> +  ti,phy-control-frames:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [0, 1]
>> +    description: |
>> +      If present, enables or disables the PHY control frames.
>> +      PHY Control Frames support can also be strapped. If the strap pin is not
>> +      set correctly or not set at all then this can be used to configure it.
>> +       - 0     = PHY Control Frames disabled
>> +       - 1     = PHY Control Frames enabled
>> +       - unset = Configured by straps
> What is a control frame?
I'm not an expert on this but it seems that if the PHY's Serial Management
interface is not available, it is possible to build PCF (PHY Control Frame)
packets that will be passed to PHY through the MAC Transmit Data 
interface. The
PHY is then able to intercept and interpret these packets. Enabling it 
increases
the MII Transmit packet latency.
You'll find details in §5.4.6 of datasheet 
[https://www.ti.com/lit/gpn/dp83640]
>> +
>> +  ti,energy-detect-en:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description: |
>> +      If present, Energy Detect Mode is enabled. If not present, Energy Detect
>> +      Mode is disabled. This feature can not be strapped.
> Please use the phy tunable ETHTOOL_PHY_EDPD. There are a few examples
> you can copy.

Ok I'll do that also, thank you.


Best regards,

Bastien
  

> > > +  ti,phy-control-frames:
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [0, 1]
> > > +    description: |
> > > +      If present, enables or disables the PHY control frames.
> > > +      PHY Control Frames support can also be strapped. If the strap pin is not
> > > +      set correctly or not set at all then this can be used to configure it.
> > > +       - 0     = PHY Control Frames disabled
> > > +       - 1     = PHY Control Frames enabled
> > > +       - unset = Configured by straps
> > What is a control frame?
> I'm not an expert on this but it seems that if the PHY's Serial Management
> interface is not available, it is possible to build PCF (PHY Control Frame)
> packets that will be passed to PHY through the MAC Transmit Data interface.
> The
> PHY is then able to intercept and interpret these packets. Enabling it
> increases
> the MII Transmit packet latency.
> You'll find details in §5.4.6 of datasheet
> [https://www.ti.com/lit/gpn/dp83640]

Do you actually need this feature?

[Looks at data sheet]

Ah, so it allows you to access PHY registers by sending it commands in
Ethernet frames. That should in theory be faster than MDIO. However,
my experience with Ethernet switches which offer similar capabilities,
it is often not faster, because of interrupt coalescing. 

Anyway, the serial management interface is the MDIO bus. You know this
is available, because that is how the PHY driver it talking to the
PHY! Also, i've not seen any code which implements sending commands to
the PHY using Ethernet frames.

So why not just hard code it in the driver to disable this feature?

	Andrew
  

Hi Andrew, Bastien,

On Wed, 31 Jan 2024 23:40:45 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Wed, Jan 31, 2024 at 09:18:39PM +0000, Conor Dooley wrote:
> > On Wed, Jan 31, 2024 at 03:05:21PM -0600, Rob Herring wrote:  
> > > On Tue, Jan 30, 2024 at 05:56:37PM +0000, Conor Dooley wrote:  
> > > > On Tue, Jan 30, 2024 at 09:59:34AM +0100, Bastien Curutchet wrote:  
> >   
> > > > > +  ti,fiber-mode:
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +    enum: [0, 1]
> > > > > +    description: |
> > > > > +      If present, enables or disables the FX Fiber Mode.
> > > > > +      Fiber mode support can also be strapped. If the strap pin is not set
> > > > > +      correctly or not set at all then this can be used to configure it.
> > > > > +       - 0     = FX Fiber Mode disabled
> > > > > +       - 1     = FX Fiber Mode enabled
> > > > > +       - unset = Configured by straps  
> > > > 
> > > > I don't like these properties that map meanings onto numbers. We can
> > > > have enums of strings in bindings that allow you to use something more
> > > > meaningful than "0" or "1".  
> > > 
> > > Tristate properties are fairly common pattern where we need 
> > > on/off/default. I've thought about making it a type. I don't think we 
> > > need defines for it.  
> > 
> > I think a type would be a good idea. I am not at all a fan of any of the
> > properties people introduce along these lines.  
> 
> Before going too far with that, i'm not actually sure it is required
> here. I've not looked at the PHY driver itself, but i expect there is
> some indication somewhere that the network stack expects a fibre link
> is to be used. We probably can determine at runtime if fibre should be
> used.

I've missed that thread initially. I guess that if Fiber is to be used,
this would be done through sfp, then we have all the regular interfaces
to configure the phy_interface_mode, in that case that would be
100BaseX.

So, a sane behaviour could simply be to configure the PHY in copper
mode by default, without relying on any DT property ? If anyone wants to
use fiber mode, then they would have to implement the
sfp_upstreamp_ops, which would take care of reconfiguring the MDI
interface of the PHY in the correct mode.

Maxime
  

> 
> I've missed that thread initially. I guess that if Fiber is to be used,
> this would be done through sfp, then we have all the regular interfaces
> to configure the phy_interface_mode, in that case that would be
> 100BaseX.
> 
> So, a sane behaviour could simply be to configure the PHY in copper
> mode by default, without relying on any DT property ? If anyone wants to
> use fiber mode, then they would have to implement the
> sfp_upstreamp_ops, which would take care of reconfiguring the MDI
> interface of the PHY in the correct mode.

I missed the fact that this isn't a new driver... So this would indeed
break existing setups who would have fiber-mode strapped-in but don't
use SFP for some reason :(

Maxime

> Maxime
>