[PATCH 0/2] Add device tree binding support to TI's DP83640

[PATCH 0/2] Add device tree binding support to TI's DP83640

[Bastien Curutchet]

Hi everyone,

This short patch series adds a device-tree binding support to the TI's PHY
DP83640. The goal is to be able to enable or disable the following features
through the device tree:
   - Energy Detect Mode
   - PHY Control Frames
   - LED Configuration
   - Fiber Mode

Bastien Curutchet (2):
  dt-bindings: net: Add TI DP83640
  net: phy: Add some configuration from device-tree

 .../devicetree/bindings/net/ti,dp83640.yaml   | 113 +++++++++++++++
 drivers/net/phy/dp83640.c                     | 131 +++++++++++++++++-
 drivers/net/phy/dp83640_reg.h                 |  21 ++-
 include/dt-bindings/net/ti-dp83640.h          |  18 +++
 4 files changed, 281 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/ti,dp83640.yaml
 create mode 100644 include/dt-bindings/net/ti-dp83640.h

-- 
2.43.0

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

[Bastien Curutchet]

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

diff --git a/Documentation/devicetree/bindings/net/ti,dp83640.yaml b/Documentation/devicetree/bindings/net/ti,dp83640.yaml
new file mode 100644
index 000000000000..b0f389122934
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ti,dp83640.yaml
@@ -0,0 +1,113 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2024 Nanometrics Inc
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/ti,dp83640.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI DP83640 ethernet PHY
+
+allOf:
+  - $ref: ethernet-controller.yaml#
+
+maintainers:
+  - Bastien Curutchet <bastien.curutchet@bootlin.com>
+
+description: |
+  The DP83640 Precision PHYTER device delivers the highest level of precision
+  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
+
+  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
+
+  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
+
+  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
+
+  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.
+
+required:
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/net/ti-dp83640.h>
+
+    mdio0 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      ethphy0: ethernet-phy@0 {
+        reg = <0>;
+        ti,clk-output = <0>;
+        ti,energy-detect-en;
+        ti,led-config = <DP83640_PHYCR_LED_CNFG_MODE_3>;
+        ti,phy-control-frames = <1>;
+        ti,fiber-mode = <1>;
+      };
+    };
diff --git a/include/dt-bindings/net/ti-dp83640.h b/include/dt-bindings/net/ti-dp83640.h
new file mode 100644
index 000000000000..5f44f8eeb666
--- /dev/null
+++ b/include/dt-bindings/net/ti-dp83640.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+/*
+ * Device Tree constants for the Texas Instruments DP83640 PHY
+ *
+ * Author: Bastien Curutchet bastien.curutchet@bootlin.com>
+ *
+ * Copyright: 2024 Nanometrics Inc.
+ */
+
+#ifndef _DT_BINDINGS_TI_DP83640_H
+#define _DT_BINDINGS_TI_DP83640_H
+
+/* PHY CTRL bits */
+#define DP83640_PHYCR_LED_CNFG_MODE_1 1
+#define DP83640_PHYCR_LED_CNFG_MODE_2 2
+#define DP83640_PHYCR_LED_CNFG_MODE_3 3
+
+#endif
-- 
2.43.0

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

[Andrew Lunn]

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

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

[Bastien Curutchet]

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

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

[Andrew Lunn]

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

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2023 bytes --]

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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Rob Herring]

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

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1185 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Andrew Lunn]

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

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

[Maxime Chevallier]

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

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

[Maxime Chevallier]

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

[PATCH 2/2] net: phy: Add some configuration from device-tree

[Bastien Curutchet]

Some features can now be enabled or disabled from device tree.
If attributes are present in device-tree, features are enabled
or disabled via MDIO registers. Else, hardware configuration is
left as is.
These features are : Energy Detect Mode, PHY Control Frames,
LED configuration and Fiber Mode.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 drivers/net/phy/dp83640.c     | 131 +++++++++++++++++++++++++++++++++-
 drivers/net/phy/dp83640_reg.h |  21 +++++-
 2 files changed, 150 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 5c42c47dc564..f5770002b849 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -7,6 +7,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <dt-bindings/net/ti-dp83640.h>
 #include <linux/crc32.h>
 #include <linux/ethtool.h>
 #include <linux/kernel.h>
@@ -16,6 +17,7 @@
 #include <linux/net_tstamp.h>
 #include <linux/netdevice.h>
 #include <linux/if_vlan.h>
+#include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/ptp_classify.h>
 #include <linux/ptp_clock_kernel.h>
@@ -1418,15 +1420,142 @@ static int dp83640_ts_info(struct mii_timestamper *mii_ts,
 	return 0;
 }
 
+#ifdef CONFIG_OF_MDIO
+static int dp83640_of_init(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct device_node *of_node = dev->of_node;
+	int reg_val;
+	u32 of_val;
+	int ret;
+
+	if (!of_node)
+		return 0;
+
+	/* All configured features reside in PAGE 0 */
+	phy_write(phydev, PAGESEL, 0);
+
+	/* Energy detect mode */
+	reg_val = phy_read(phydev, EDCR);
+	if (of_property_present(of_node, "ti,energy-detect-en"))
+		reg_val |= ED_EN;
+	else
+		reg_val &= ~ED_EN;
+	phy_write(phydev, EDCR, reg_val);
+
+	/* CLK_OUTPUT Pin */
+	if (of_property_present(of_node, "ti,clk-output")) {
+		ret = of_property_read_u32(of_node, "ti,clk-output", &of_val);
+		if (ret)
+			return ret;
+
+		reg_val = phy_read(phydev, PHYCR2);
+		switch (of_val) {
+		case 0:
+			reg_val |= CLK_OUT_DIS;
+			break;
+		case 1:
+			reg_val &= ~CLK_OUT_DIS;
+			break;
+		default:
+			phydev_err(phydev, "Invalid value for ti,clk-output property (%d)"
+					, of_val);
+			return -EINVAL;
+		}
+		phy_write(phydev, PHYCR2, reg_val);
+	}
+
+	/* LED configuration */
+	if (of_property_present(of_node, "ti,led-config"))  {
+		ret = of_property_read_u32(of_node, "ti,led-config", &of_val);
+		if (ret)
+			return ret;
+
+		reg_val = phy_read(phydev, PHYCR) & ~(LED_CNFG_1 | LED_CNFG_0);
+		switch (of_val) {
+		case DP83640_PHYCR_LED_CNFG_MODE_1:
+			reg_val |= LED_CNFG_0;
+			break;
+		case DP83640_PHYCR_LED_CNFG_MODE_2:
+			/* Keeping LED_CNFG_1 and LED_CNFG_0 unset */
+			break;
+		case DP83640_PHYCR_LED_CNFG_MODE_3:
+			reg_val |= LED_CNFG_1;
+			break;
+		default:
+			phydev_err(phydev, "Invalid value for ti,led-config property (%d)"
+					, of_val);
+			return -EINVAL;
+		}
+		phy_write(phydev, PHYCR, reg_val);
+	}
+	if (of_property_present(of_node, "ti,phy-control-frames")) {
+		of_property_read_u32(of_node, "ti,phy-control-frames", &of_val);
+		if (ret)
+			return ret;
+
+		reg_val = phy_read(phydev, PCFCR);
+		switch (of_val) {
+		case 0:
+			reg_val &= ~PCF_EN;
+			break;
+		case 1:
+			reg_val |= PCF_EN;
+			break;
+		default:
+			phydev_err(phydev, "Invalid value for ti,phy-control-frames property (%d)"
+					, of_val);
+			return -EINVAL;
+		}
+		phy_write(phydev, PCFCR, reg_val);
+	}
+	if (of_property_present(of_node, "ti,fiber-mode")) {
+		ret = of_property_read_u32(of_node, "ti,fiber-mode", &of_val);
+		if (ret)
+			return ret;
+
+		reg_val = phy_read(phydev, PCSR);
+		switch (of_val) {
+		case 0:
+			reg_val &= ~FX_EN;
+			break;
+		case 1:
+			reg_val |= FX_EN;
+			break;
+		default:
+			phydev_err(phydev, "Invalid value for ti,fiber-mode property (%d)"
+					, of_val);
+			return -EINVAL;
+		}
+		phy_write(phydev, PCSR, reg_val);
+		/* Write SOFT_RESET bit to ensure configuration */
+		reg_val = phy_read(phydev, PHYCR2) | SOFT_RESET;
+		phy_write(phydev, PHYCR2, reg_val);
+	}
+
+	return 0;
+}
+#else
+static int dp83640_of_init(struct phy_device *phydev)
+{
+	return 0;
+}
+#endif /* CONFIG_OF_MDIO */
+
 static int dp83640_probe(struct phy_device *phydev)
 {
 	struct dp83640_clock *clock;
 	struct dp83640_private *dp83640;
-	int err = -ENOMEM, i;
+	int err, i;
 
 	if (phydev->mdio.addr == BROADCAST_ADDR)
 		return 0;
 
+	err = dp83640_of_init(phydev);
+	if (err < 0)
+		return err;
+
+	err = -ENOMEM;
 	clock = dp83640_clock_get_bus(phydev->mdio.bus);
 	if (!clock)
 		goto no_clock;
diff --git a/drivers/net/phy/dp83640_reg.h b/drivers/net/phy/dp83640_reg.h
index daae7fa58fb8..8877ba560406 100644
--- a/drivers/net/phy/dp83640_reg.h
+++ b/drivers/net/phy/dp83640_reg.h
@@ -6,7 +6,11 @@
 #define HAVE_DP83640_REGISTERS
 
 /* #define PAGE0                  0x0000 */
+#define PCSR                      0x0016 /* PCS Configuration and Status Register */
+#define PHYCR                     0x0019 /* PHY Control Register */
 #define PHYCR2                    0x001c /* PHY Control Register 2 */
+#define EDCR                      0x001D /* Energy Detect Control Register */
+#define PCFCR                     0x001F /* PHY Control Frames Control Register */
 
 #define PAGE4                     0x0004
 #define PTP_CTL                   0x0014 /* PTP Control Register */
@@ -50,8 +54,23 @@
 #define PTP_GPIOMON               0x001e /* PTP GPIO Monitor Register */
 #define PTP_RXHASH                0x001f /* PTP Receive Hash Register */
 
+/* Bit definitions for the PCSR register */
+#define FX_EN		          BIT(6)  /* Enable FX Fiber Mode */
+
+/* Bit definitions for the PHYCR register */
+#define LED_CNFG_0	          BIT(5)  /* LED configuration, bit 0 */
+#define LED_CNFG_1	          BIT(6)  /* LED configuration, bit 1 */
+
 /* Bit definitions for the PHYCR2 register */
-#define BC_WRITE                  (1<<11) /* Broadcast Write Enable */
+#define CLK_OUT_DIS	          BIT(1)  /* Disable CLK_OUT pin */
+#define SOFT_RESET		  BIT(9)  /* Soft Reset */
+#define BC_WRITE                  BIT(11) /* Broadcast Write Enable */
+
+/* Bit definitions for the EDCR register */
+#define ED_EN		          BIT(15) /* Enable Energy Detect Mode */
+
+/* Bit definitions for the PCFCR register */
+#define PCF_EN	                  BIT(0)  /* Enable PHY Control Frames */
 
 /* Bit definitions for the PTP_CTL register */
 #define TRIG_SEL_SHIFT            (10)    /* PTP Trigger Select */
-- 
2.43.0