[PATCH v2 0/6] net: phy: Add TI's DP83640 device tree binding

[PATCH v2 0/6] net: phy: Add TI's DP83640 device tree binding

[Bastien Curutchet]

Hi everyone,

This patch series adds small features to the TI PHY DP83640 :
  - Add LED handling through /sys/class/leds to use the three
    configuration modes offered by PHY.
  - Add Energy Detect Mode handling through ethtool's
    {get/set}_phy_tunable()
  - Add a device tree attribute to be able to override the hardware
    strap configuration if needed. This attribute allows to
    enable/disable fiber mode.

Change log v1 -> v2 :
  - LED Configuration is now done through /sys/class/leds insted of DT
  - Energy Detect Mode is done from {set/get}_phy_tunable() instead of DT
  - The DT property to enable/disable PHY control frames is dropped,
    the feature is disabled directly in driver as it is not handled
    anyway.
  - The CLK_OUT pin handling has been dropped.

Bastien Curutchet (6):
  dt-bindings: net: Add bindings for PHY DP83640
  leds: trigger: Create a new LED netdev trigger for collision
  net: phy: DP83640: Add LED handling
  net: phy: DP83640: Add EDPD management
  net: phy: DP83640: Explicitly disabling PHY Control Frames
  net: phy: DP83640: Add fiber mode enabling/disabling from device tree

 .../testing/sysfs-class-led-trigger-netdev    |  11 +
 .../devicetree/bindings/net/ti,dp83640.yaml   |  77 +++++
 drivers/leds/trigger/ledtrig-netdev.c         |   4 +
 drivers/net/phy/dp83640.c                     | 299 ++++++++++++++++++
 drivers/net/phy/dp83640_reg.h                 |  24 ++
 include/linux/leds.h                          |   1 +
 6 files changed, 416 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ti,dp83640.yaml

-- 
2.43.0

[PATCH v2 1/6] dt-bindings: net: Add bindings for PHY DP83640

[Bastien Curutchet]

The DP83640 is a PTP PHY. Some of his features can be enabled by
hardware straps. There is not binding yet.

Add a device tree binding to be able to override the hardware strap
configuration when needed.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 .../devicetree/bindings/net/ti,dp83640.yaml   | 77 +++++++++++++++++++
 1 file changed, 77 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ti,dp83640.yaml

diff --git a/Documentation/devicetree/bindings/net/ti,dp83640.yaml b/Documentation/devicetree/bindings/net/ti,dp83640.yaml
new file mode 100644
index 000000000000..db1dc909d5cb
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ti,dp83640.yaml
@@ -0,0 +1,77 @@
+# 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 is an Ethernet PHY providing PTP
+  capabilities based on IEEE 1588 standard.
+
+  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,fiber-mode:
+    $ref: /schemas/types.yaml#/definitions/string
+    enum: [ disable, enable ]
+    description: |
+      If present, enables or disables the FX Fiber Mode.
+       - disable  = FX Fiber Mode disabled
+       - enable   = FX Fiber Mode enabled
+      Fiber mode enabling 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.
+
+  leds:
+    type: object
+    $ref: /schemas/leds/common.yaml#
+    description: |
+      Describes the three LEDs of the PHY.
+
+required:
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    mdio0 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      ethphy0: ethernet-phy@0 {
+        reg = <0>;
+        ti,fiber-mode = "disable";
+        leds {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          led@0 {
+            reg = <0>;
+            label = "activity";
+          };
+          led@1 {
+            reg = <1>;
+            label = "link";
+          };
+          led@2 {
+            reg = <2>;
+            label = "speed";
+          };
+        };
+      };
+    };
-- 
2.43.0

Re: [PATCH v2 1/6] dt-bindings: net: Add bindings for PHY DP83640

[Conor Dooley]

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

On Tue, Feb 27, 2024 at 10:39:40AM +0100, Bastien Curutchet wrote:
> The DP83640 is a PTP PHY. Some of his features can be enabled by
> hardware straps. There is not binding yet.
> 
> Add a device tree binding to be able to override the hardware strap
> configuration when needed.
> 
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

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

[PATCH v2 2/6] leds: trigger: Create a new LED netdev trigger for collision

[Bastien Curutchet]

Collisions on link does not fit into one of the existing netdev triggers.

Add TRIGGER_NETDEV_COLLISION in the enum led_trigger_netdev_modes.
Add its definition in Documentation.
Add its handling in ledtrig-netdev, it can only be supported by hardware
so no software fallback is implemented.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 .../ABI/testing/sysfs-class-led-trigger-netdev        | 11 +++++++++++
 drivers/leds/trigger/ledtrig-netdev.c                 |  4 ++++
 include/linux/leds.h                                  |  1 +
 3 files changed, 16 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-netdev b/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
index a6c307c4befa..fbb2bc1d6108 100644
--- a/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
@@ -178,3 +178,14 @@ Description:
 		If set to 1, the LED's normal state reflects the link full
 		duplex state of the named network device.
 		Setting this value also immediately changes the LED state.
+
+What:		/sys/class/leds/<led>/collision
+Date:		Feb 2024
+KernelVersion:	6.8
+Contact:	linux-leds@vger.kernel.org
+Description:
+		Signal collision of the named network device.
+
+		If set to 0 (default), the LED's normal state is off.
+
+		If set to 1, the LED's normal state reflects collisions.
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 8e5475819590..5c17b8e27d5c 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -318,6 +318,7 @@ static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
 	case TRIGGER_NETDEV_FULL_DUPLEX:
 	case TRIGGER_NETDEV_TX:
 	case TRIGGER_NETDEV_RX:
+	case TRIGGER_NETDEV_COLLISION:
 		bit = attr;
 		break;
 	default:
@@ -352,6 +353,7 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 	case TRIGGER_NETDEV_FULL_DUPLEX:
 	case TRIGGER_NETDEV_TX:
 	case TRIGGER_NETDEV_RX:
+	case TRIGGER_NETDEV_COLLISION:
 		bit = attr;
 		break;
 	default:
@@ -410,6 +412,7 @@ DEFINE_NETDEV_TRIGGER(half_duplex, TRIGGER_NETDEV_HALF_DUPLEX);
 DEFINE_NETDEV_TRIGGER(full_duplex, TRIGGER_NETDEV_FULL_DUPLEX);
 DEFINE_NETDEV_TRIGGER(tx, TRIGGER_NETDEV_TX);
 DEFINE_NETDEV_TRIGGER(rx, TRIGGER_NETDEV_RX);
+DEFINE_NETDEV_TRIGGER(collision, TRIGGER_NETDEV_COLLISION);
 
 static ssize_t interval_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
@@ -473,6 +476,7 @@ static struct attribute *netdev_trig_attrs[] = {
 	&dev_attr_tx.attr,
 	&dev_attr_interval.attr,
 	&dev_attr_offloaded.attr,
+	&dev_attr_collision.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(netdev_trig);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 4754b02d3a2c..8864d6ce8185 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -578,6 +578,7 @@ enum led_trigger_netdev_modes {
 	TRIGGER_NETDEV_FULL_DUPLEX,
 	TRIGGER_NETDEV_TX,
 	TRIGGER_NETDEV_RX,
+	TRIGGER_NETDEV_COLLISION,
 
 	/* Keep last */
 	__TRIGGER_NETDEV_MAX,
-- 
2.43.0

Re: [PATCH v2 2/6] leds: trigger: Create a new LED netdev trigger for collision

[Andrew Lunn]

On Tue, Feb 27, 2024 at 10:39:41AM +0100, Bastien Curutchet wrote:
> Collisions on link does not fit into one of the existing netdev triggers.
> 
> Add TRIGGER_NETDEV_COLLISION in the enum led_trigger_netdev_modes.
> Add its definition in Documentation.
> Add its handling in ledtrig-netdev, it can only be supported by hardware
> so no software fallback is implemented.

How useful is collision? How did you test this? How did you cause
collisions to see if the LED actually worked?

As far as i can see, this is just a normal 100Base-T PHY. Everybody
uses that point-to-point nowadays. If it was an 100Base-T1, with a
shared medium, good old CSMA/CD then collision might actually be
useful.

I also disagree with not having software fallback:

ip -s link show eth0
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DEFAULT group default qlen 1000
    link/ether 80:ee:73:83:60:27 brd ff:ff:ff:ff:ff:ff
    RX:     bytes    packets errors dropped  missed   mcast           
    4382213540983 2947876747      0       0       0  154890 
    TX:     bytes    packets errors dropped carrier collsns           
      18742773651  197507119      0       0       0       0

collsns = 0. The information is there in a standard format. However,
when did you last see it not 0?

	Andrew

Re: [PATCH v2 2/6] leds: trigger: Create a new LED netdev trigger for collision

[Russell King (Oracle)]

On Tue, Feb 27, 2024 at 05:03:14PM +0100, Andrew Lunn wrote:
> On Tue, Feb 27, 2024 at 10:39:41AM +0100, Bastien Curutchet wrote:
> > Collisions on link does not fit into one of the existing netdev triggers.
> > 
> > Add TRIGGER_NETDEV_COLLISION in the enum led_trigger_netdev_modes.
> > Add its definition in Documentation.
> > Add its handling in ledtrig-netdev, it can only be supported by hardware
> > so no software fallback is implemented.
> 
> How useful is collision? How did you test this? How did you cause
> collisions to see if the LED actually worked?
> 
> As far as i can see, this is just a normal 100Base-T PHY. Everybody
> uses that point-to-point nowadays.

That's largely irrelevant when it comes to collisions. If the link has
negotiated half-duplex mode (which we still support) then even on
twisted pair, there can be collisions, even though TX and RX are using
separate pairs. It's a quirk of 802.3 that this is the case.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 2/6] leds: trigger: Create a new LED netdev trigger for collision

[Bastien Curutchet]

Hi Andrew,


On 2/27/24 17:03, Andrew Lunn wrote:
> On Tue, Feb 27, 2024 at 10:39:41AM +0100, Bastien Curutchet wrote:
>> Collisions on link does not fit into one of the existing netdev triggers.
>>
>> Add TRIGGER_NETDEV_COLLISION in the enum led_trigger_netdev_modes.
>> Add its definition in Documentation.
>> Add its handling in ledtrig-netdev, it can only be supported by hardware
>> so no software fallback is implemented.
> How useful is collision? How did you test this? How did you cause
> collisions to see if the LED actually worked?
Indeed I am not able to generate collision on my setup so I did not test 
this
collision part.
My use case is that the hardware strap configuration that selects the 
LED output mode
can not be trusted so I have to force configuration with software. I 
added this collision
part because I wanted to cover all the LED configuration modes offered 
by the PHY.
> As far as i can see, this is just a normal 100Base-T PHY. Everybody
> uses that point-to-point nowadays. If it was an 100Base-T1, with a
> shared medium, good old CSMA/CD then collision might actually be
> useful.
>
> I also disagree with not having software fallback:
>
> ip -s link show eth0
> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DEFAULT group default qlen 1000
>      link/ether 80:ee:73:83:60:27 brd ff:ff:ff:ff:ff:ff
>      RX:     bytes    packets errors dropped  missed   mcast
>      4382213540983 2947876747      0       0       0  154890
>      TX:     bytes    packets errors dropped carrier collsns
>        18742773651  197507119      0       0       0       0
>
> collsns = 0. The information is there in a standard format. However,
> when did you last see it not 0?

Ok, I could add the software callback but I will not be able to test it ...


Best regards,
Bastien

Re: [PATCH v2 2/6] leds: trigger: Create a new LED netdev trigger for collision

[Andrew Lunn]

> > How useful is collision? How did you test this? How did you cause
> > collisions to see if the LED actually worked?
> Indeed I am not able to generate collision on my setup so I did not test
> this
> collision part.
> My use case is that the hardware strap configuration that selects the LED
> output mode
> can not be trusted so I have to force configuration with software. I added
> this collision
> part because I wanted to cover all the LED configuration modes offered by
> the PHY.

There are a few things i want to avoid here:

1) Vendor SDK mentality. The hardware can do this, lets add a knob to
make use of it. We end up with 100 of configuration knobs which nobody
ever uses. Do you actually have a board where the strapping is wrong?
Are you going to submit a .dts file making use of this option?

2) LEDs are the wild west, because it is not part of 802.3. Every
vendor does it differently, and has their own special blinking
patterns. My preference is to keep it simple to what people actually
use. You cannot actually generate a collision, the developer who wants
to add support for collision. I have to ask, is collision actually
useful?

> > As far as i can see, this is just a normal 100Base-T PHY. Everybody
> > uses that point-to-point nowadays. If it was an 100Base-T1, with a
> > shared medium, good old CSMA/CD then collision might actually be
> > useful.
> > 
> > I also disagree with not having software fallback:
> > 
> > ip -s link show eth0
> > 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DEFAULT group default qlen 1000
> >      link/ether 80:ee:73:83:60:27 brd ff:ff:ff:ff:ff:ff
> >      RX:     bytes    packets errors dropped  missed   mcast
> >      4382213540983 2947876747      0       0       0  154890
> >      TX:     bytes    packets errors dropped carrier collsns
> >        18742773651  197507119      0       0       0       0
> > 
> > collsns = 0. The information is there in a standard format. However,
> > when did you last see it not 0?
> 
> Ok, I could add the software callback but I will not be able to test it ...

My personal experience is, anything not tested is broken...

Think about what Russell actually said. That should give you a clue
how to cause collisions. If not, go study history books about CSMA/CD.

   Andrew

Re: [PATCH v2 2/6] leds: trigger: Create a new LED netdev trigger for collision

[Russell King (Oracle)]

On Thu, Feb 29, 2024 at 04:17:47PM +0100, Andrew Lunn wrote:
> 2) LEDs are the wild west, because it is not part of 802.3. Every
> vendor does it differently, and has their own special blinking
> patterns. My preference is to keep it simple to what people actually
> use. You cannot actually generate a collision, the developer who wants
> to add support for collision. I have to ask, is collision actually
> useful?

I'm not sure when you say "You cannot actually generate a collision"
whether you're referring to the link or the LEDs here? Obviously, we
can't detect a collision unless the hardware tells us that it happened.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH v2 3/6] net: phy: DP83640: Add LED handling

[Bastien Curutchet]

The PHY have three LED : Activity LED, Speed LED and Link LED. The PHY
offers three configuration modes for them. The driver does not handle them.

Add LED handling through the /sys/class/leds interface.
On every mode the Speed LED indicates the speed (10Mbps or 100 Mbps) and
the Link LED indicates whether the link is good or not. Link LED will also
reflects link activity in mode 2 and 3. The Activity LED can reflect the
link activity (mode 1), the link's duplex (mode 2) or collisions on the
link (mode 3).
Only the Activity LED can have its hardware configuration updated, it
has an impact on Link LED as activity reflexion is added on it on modes
2 and 3

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

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 5c42c47dc564..c46c81ef0ad0 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -59,6 +59,22 @@
 				   MII_DP83640_MISR_SPD_INT |\
 				   MII_DP83640_MISR_LINK_INT)
 
+#define DP83640_ACTLED_IDX	0
+#define DP83640_LNKLED_IDX	1
+#define DP83640_SPDLED_IDX	2
+/* LNKLED = ON for Good Link, OFF for No Link */
+/* SPDLED = ON in 100 Mb/s, OFF in 10 Mb/s */
+/* ACTLED = ON for Activity, OFF for No Activity */
+#define DP83640_LED_MODE_1	1
+/* LNKLED = ON for Good Link, Blink for Activity */
+/* SPDLED = ON in 100 Mb/s, OFF in 10 Mb/s */
+/* ACTLED = ON for Collision, OFF for No Collision */
+#define DP83640_LED_MODE_2	2
+/* LNKLED = ON for Good Link, Blink for Activity */
+/* SPDLED = ON in 100 Mb/s, OFF in 10 Mb/s */
+/* ACTLED = ON for Full-Duplex, OFF for Half-Duplex */
+#define DP83640_LED_MODE_3	3
+
 /* phyter seems to miss the mark by 16 ns */
 #define ADJTIME_FIX	16
 
@@ -1515,6 +1531,161 @@ static void dp83640_remove(struct phy_device *phydev)
 	kfree(dp83640);
 }
 
+static int dp83640_led_brightness_set(struct phy_device *phydev, u8 index,
+				      enum led_brightness brightness)
+{
+	int val;
+
+	if (index > DP83640_SPDLED_IDX)
+		return -EINVAL;
+
+	phy_write(phydev, PAGESEL, 0);
+
+	val = phy_read(phydev, LEDCR) & ~DP83640_LED_DIS(index);
+	val |= DP83640_LED_DRV(index);
+	if (brightness)
+		val |= DP83640_LED_VAL(index);
+	else
+		val &= ~DP83640_LED_VAL(index);
+	phy_write(phydev, LEDCR, val);
+
+	return 0;
+}
+
+/**
+ * dp83640_led_mode - Check the trigger netdev rules and compute the associated
+ *                    configuration mode
+ * @index: The targeted LED
+ * @rules: Rules to be checked
+ *
+ * Returns the mode that is to be set in LED_CFNG. If the rules are not
+ * supported by the PHY, returns -ENOTSUPP. If the rules are supported but don't
+ * impact the LED configuration, returns 0
+ */
+static int dp83640_led_mode(u8 index, unsigned long rules)
+{
+	/* Only changes on ACTLED have an impact on LED Mode configuration */
+	switch (index) {
+	case DP83640_ACTLED_IDX:
+		switch (rules) {
+		case BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX):
+			return DP83640_LED_MODE_1;
+		case BIT(TRIGGER_NETDEV_COLLISION):
+			return DP83640_LED_MODE_2;
+		case BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
+		     BIT(TRIGGER_NETDEV_HALF_DUPLEX):
+			return DP83640_LED_MODE_3;
+		default:
+			return -EOPNOTSUPP;
+		}
+
+	case DP83640_SPDLED_IDX:
+		/* SPDLED has the same function in every mode */
+		switch (rules) {
+		case BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK_100):
+			return 0;
+		default:
+			return -EOPNOTSUPP;
+		}
+
+	case DP83640_LNKLED_IDX:
+		/* LNKLED has the same function in every mode */
+		switch (rules) {
+		case BIT(TRIGGER_NETDEV_LINK):
+			return 0;
+		default:
+			return -EOPNOTSUPP;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int dp83640_led_hw_is_supported(struct phy_device *phydev, u8 index,
+				       unsigned long rules)
+{
+	int ret;
+
+	ret = dp83640_led_mode(index, rules);
+
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int dp83640_led_hw_control_set(struct phy_device *phydev, u8 index,
+				      unsigned long rules)
+{
+	int mode, val;
+
+	mode = dp83640_led_mode(index, rules);
+	if (mode < 0)
+		return mode;
+
+	if (mode) {
+		phy_write(phydev, PAGESEL, 0);
+		val = phy_read(phydev, PHYCR) & ~(LED_CNFG_1 | LED_CNFG_0);
+		switch (mode) {
+		case DP83640_LED_MODE_1:
+			val |= LED_CNFG_0;
+		break;
+		case DP83640_LED_MODE_2:
+			/* Keeping LED_CNFG_1 and LED_CNFG_0 unset */
+			break;
+		case DP83640_LED_MODE_3:
+			val |= LED_CNFG_1;
+			break;
+		default:
+			return -EINVAL;
+		}
+		phy_write(phydev, PHYCR, val);
+	}
+
+	val = phy_read(phydev, LEDCR);
+	val &= ~(DP83640_LED_DIS(index) | DP83640_LED_DRV(index));
+	phy_write(phydev, LEDCR, val);
+
+	return 0;
+}
+
+static int dp83640_led_hw_control_get(struct phy_device *phydev, u8 index,
+				      unsigned long *rules)
+{
+	int val;
+
+	switch (index) {
+	case DP83640_ACTLED_IDX:
+		phy_write(phydev, PAGESEL, 0);
+		val = phy_read(phydev, PHYCR);
+		if (val & LED_CNFG_0) {
+			/* Mode 1 */
+			*rules = BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX);
+		} else if (val & LED_CNFG_1) {
+			/* Mode 3 */
+			*rules = BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
+				 BIT(TRIGGER_NETDEV_HALF_DUPLEX);
+		} else {
+			/* Mode 2 */
+			*rules = BIT(TRIGGER_NETDEV_COLLISION);
+		}
+		break;
+
+	case DP83640_LNKLED_IDX:
+		*rules = BIT(TRIGGER_NETDEV_LINK);
+		break;
+
+	case DP83640_SPDLED_IDX:
+		*rules = BIT(TRIGGER_NETDEV_LINK_10) |
+			 BIT(TRIGGER_NETDEV_LINK_100);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static struct phy_driver dp83640_driver = {
 	.phy_id		= DP83640_PHY_ID,
 	.phy_id_mask	= 0xfffffff0,
@@ -1526,6 +1697,11 @@ static struct phy_driver dp83640_driver = {
 	.config_init	= dp83640_config_init,
 	.config_intr    = dp83640_config_intr,
 	.handle_interrupt = dp83640_handle_interrupt,
+
+	.led_brightness_set = dp83640_led_brightness_set,
+	.led_hw_is_supported = dp83640_led_hw_is_supported,
+	.led_hw_control_set = dp83640_led_hw_control_set,
+	.led_hw_control_get = dp83640_led_hw_control_get,
 };
 
 static int __init dp83640_init(void)
diff --git a/drivers/net/phy/dp83640_reg.h b/drivers/net/phy/dp83640_reg.h
index daae7fa58fb8..09dd2d2527c7 100644
--- a/drivers/net/phy/dp83640_reg.h
+++ b/drivers/net/phy/dp83640_reg.h
@@ -6,6 +6,8 @@
 #define HAVE_DP83640_REGISTERS
 
 /* #define PAGE0                  0x0000 */
+#define LEDCR                     0x0018 /* PHY Control Register */
+#define PHYCR                     0x0019 /* PHY Control Register */
 #define PHYCR2                    0x001c /* PHY Control Register 2 */
 
 #define PAGE4                     0x0004
@@ -50,6 +52,15 @@
 #define PTP_GPIOMON               0x001e /* PTP GPIO Monitor Register */
 #define PTP_RXHASH                0x001f /* PTP Receive Hash Register */
 
+/* Bit definitions for the LEDCR register */
+#define DP83640_LED_DIS(x)        BIT((x) + 9) /* Disable LED */
+#define DP83640_LED_DRV(x)        BIT((x) + 3) /* Force LED val to output */
+#define DP83640_LED_VAL(x)        BIT((x))     /* LED val */
+
+/* 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 */
 
-- 
2.43.0

Re: [PATCH v2 3/6] net: phy: DP83640: Add LED handling

[Maxime Chevallier]

Hello Bastien,

On Tue, 27 Feb 2024 10:39:42 +0100
Bastien Curutchet <bastien.curutchet@bootlin.com> wrote:

> The PHY have three LED : Activity LED, Speed LED and Link LED. The PHY
> offers three configuration modes for them. The driver does not handle them.
> 
> Add LED handling through the /sys/class/leds interface.
> On every mode the Speed LED indicates the speed (10Mbps or 100 Mbps) and
> the Link LED indicates whether the link is good or not. Link LED will also
> reflects link activity in mode 2 and 3. The Activity LED can reflect the
> link activity (mode 1), the link's duplex (mode 2) or collisions on the
> link (mode 3).
> Only the Activity LED can have its hardware configuration updated, it
> has an impact on Link LED as activity reflexion is added on it on modes
> 2 and 3
> 
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>

Regarding the LED management I don't have much knowledge on it,
however I do have comments on the code itself :)

> ---
>  drivers/net/phy/dp83640.c     | 176 ++++++++++++++++++++++++++++++++++
>  drivers/net/phy/dp83640_reg.h |  11 +++
>  2 files changed, 187 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
> index 5c42c47dc564..c46c81ef0ad0 100644
> --- a/drivers/net/phy/dp83640.c
> +++ b/drivers/net/phy/dp83640.c
> @@ -59,6 +59,22 @@
>  				   MII_DP83640_MISR_SPD_INT |\
>  				   MII_DP83640_MISR_LINK_INT)
>  
> +#define DP83640_ACTLED_IDX	0
> +#define DP83640_LNKLED_IDX	1
> +#define DP83640_SPDLED_IDX	2
> +/* LNKLED = ON for Good Link, OFF for No Link */
> +/* SPDLED = ON in 100 Mb/s, OFF in 10 Mb/s */
> +/* ACTLED = ON for Activity, OFF for No Activity */
> +#define DP83640_LED_MODE_1	1
> +/* LNKLED = ON for Good Link, Blink for Activity */
> +/* SPDLED = ON in 100 Mb/s, OFF in 10 Mb/s */
> +/* ACTLED = ON for Collision, OFF for No Collision */
> +#define DP83640_LED_MODE_2	2
> +/* LNKLED = ON for Good Link, Blink for Activity */
> +/* SPDLED = ON in 100 Mb/s, OFF in 10 Mb/s */
> +/* ACTLED = ON for Full-Duplex, OFF for Half-Duplex */
> +#define DP83640_LED_MODE_3	3
> +
>  /* phyter seems to miss the mark by 16 ns */
>  #define ADJTIME_FIX	16
>  
> @@ -1515,6 +1531,161 @@ static void dp83640_remove(struct phy_device *phydev)
>  	kfree(dp83640);
>  }
>  
> +static int dp83640_led_brightness_set(struct phy_device *phydev, u8 index,
> +				      enum led_brightness brightness)
> +{
> +	int val;
> +
> +	if (index > DP83640_SPDLED_IDX)
> +		return -EINVAL;
> +
> +	phy_write(phydev, PAGESEL, 0);
> +
> +	val = phy_read(phydev, LEDCR) & ~DP83640_LED_DIS(index);
> +	val |= DP83640_LED_DRV(index);
> +	if (brightness)
> +		val |= DP83640_LED_VAL(index);
> +	else
> +		val &= ~DP83640_LED_VAL(index);
> +	phy_write(phydev, LEDCR, val);

Looks like you can use phy_modify here

> +
> +	return 0;
> +}
> +
> +/**
> + * dp83640_led_mode - Check the trigger netdev rules and compute the associated
> + *                    configuration mode
> + * @index: The targeted LED
> + * @rules: Rules to be checked
> + *
> + * Returns the mode that is to be set in LED_CFNG. If the rules are not
> + * supported by the PHY, returns -ENOTSUPP. If the rules are supported but don't
> + * impact the LED configuration, returns 0
> + */
> +static int dp83640_led_mode(u8 index, unsigned long rules)
> +{
> +	/* Only changes on ACTLED have an impact on LED Mode configuration */
> +	switch (index) {
> +	case DP83640_ACTLED_IDX:
> +		switch (rules) {
> +		case BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX):
> +			return DP83640_LED_MODE_1;
> +		case BIT(TRIGGER_NETDEV_COLLISION):
> +			return DP83640_LED_MODE_2;
> +		case BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
> +		     BIT(TRIGGER_NETDEV_HALF_DUPLEX):
> +			return DP83640_LED_MODE_3;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +
> +	case DP83640_SPDLED_IDX:
> +		/* SPDLED has the same function in every mode */
> +		switch (rules) {
> +		case BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK_100):
> +			return 0;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +
> +	case DP83640_LNKLED_IDX:
> +		/* LNKLED has the same function in every mode */
> +		switch (rules) {
> +		case BIT(TRIGGER_NETDEV_LINK):
> +			return 0;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int dp83640_led_hw_is_supported(struct phy_device *phydev, u8 index,
> +				       unsigned long rules)
> +{
> +	int ret;
> +
> +	ret = dp83640_led_mode(index, rules);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}

It looks like you can just return whatever dp83640_led_mode() returns
directly ?

> +
> +static int dp83640_led_hw_control_set(struct phy_device *phydev, u8 index,
> +				      unsigned long rules)
> +{
> +	int mode, val;
> +
> +	mode = dp83640_led_mode(index, rules);
> +	if (mode < 0)
> +		return mode;
> +
> +	if (mode) {
> +		phy_write(phydev, PAGESEL, 0);

Here the current page is written to 0, don't you need to restore the
page to the original one afterwards ? the broadcast_* functions in the
same driver are always restoring the page to the initial one, I think
you also need that, or unrelated register accesses in the same PHY
driver might write to the wrong page.

If you want that to be done automatically for you, you can implement the
.read_page and .write_page in the driver, then use the
phy_read/write/modify_paged accessors.

> +		val = phy_read(phydev, PHYCR) & ~(LED_CNFG_1 | LED_CNFG_0);
> +		switch (mode) {
> +		case DP83640_LED_MODE_1:
> +			val |= LED_CNFG_0;
> +		break;
> +		case DP83640_LED_MODE_2:
> +			/* Keeping LED_CNFG_1 and LED_CNFG_0 unset */
> +			break;
> +		case DP83640_LED_MODE_3:
> +			val |= LED_CNFG_1;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		phy_write(phydev, PHYCR, val);
> +	}
> +
> +	val = phy_read(phydev, LEDCR);
> +	val &= ~(DP83640_LED_DIS(index) | DP83640_LED_DRV(index));
> +	phy_write(phydev, LEDCR, val);

You can use phy_modify here aswell

> +
> +	return 0;
> +}
> +
> +static int dp83640_led_hw_control_get(struct phy_device *phydev, u8 index,
> +				      unsigned long *rules)
> +{
> +	int val;
> +
> +	switch (index) {
> +	case DP83640_ACTLED_IDX:
> +		phy_write(phydev, PAGESEL, 0);

Same comment on the page selection here.

> +		val = phy_read(phydev, PHYCR);
> +		if (val & LED_CNFG_0) {
> +			/* Mode 1 */
> +			*rules = BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX);
> +		} else if (val & LED_CNFG_1) {
> +			/* Mode 3 */
> +			*rules = BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
> +				 BIT(TRIGGER_NETDEV_HALF_DUPLEX);
> +		} else {
> +			/* Mode 2 */
> +			*rules = BIT(TRIGGER_NETDEV_COLLISION);
> +		}
> +		break;
> +
> +	case DP83640_LNKLED_IDX:
> +		*rules = BIT(TRIGGER_NETDEV_LINK);
> +		break;
> +
> +	case DP83640_SPDLED_IDX:
> +		*rules = BIT(TRIGGER_NETDEV_LINK_10) |
> +			 BIT(TRIGGER_NETDEV_LINK_100);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static struct phy_driver dp83640_driver = {
>  	.phy_id		= DP83640_PHY_ID,
>  	.phy_id_mask	= 0xfffffff0,
> @@ -1526,6 +1697,11 @@ static struct phy_driver dp83640_driver = {
>  	.config_init	= dp83640_config_init,
>  	.config_intr    = dp83640_config_intr,
>  	.handle_interrupt = dp83640_handle_interrupt,
> +
> +	.led_brightness_set = dp83640_led_brightness_set,
> +	.led_hw_is_supported = dp83640_led_hw_is_supported,
> +	.led_hw_control_set = dp83640_led_hw_control_set,
> +	.led_hw_control_get = dp83640_led_hw_control_get,
>  };
>  
>  static int __init dp83640_init(void)
> diff --git a/drivers/net/phy/dp83640_reg.h b/drivers/net/phy/dp83640_reg.h
> index daae7fa58fb8..09dd2d2527c7 100644
> --- a/drivers/net/phy/dp83640_reg.h
> +++ b/drivers/net/phy/dp83640_reg.h
> @@ -6,6 +6,8 @@
>  #define HAVE_DP83640_REGISTERS
>  
>  /* #define PAGE0                  0x0000 */
> +#define LEDCR                     0x0018 /* PHY Control Register */
> +#define PHYCR                     0x0019 /* PHY Control Register */
>  #define PHYCR2                    0x001c /* PHY Control Register 2 */
>  
>  #define PAGE4                     0x0004
> @@ -50,6 +52,15 @@
>  #define PTP_GPIOMON               0x001e /* PTP GPIO Monitor Register */
>  #define PTP_RXHASH                0x001f /* PTP Receive Hash Register */
>  
> +/* Bit definitions for the LEDCR register */
> +#define DP83640_LED_DIS(x)        BIT((x) + 9) /* Disable LED */
> +#define DP83640_LED_DRV(x)        BIT((x) + 3) /* Force LED val to output */
> +#define DP83640_LED_VAL(x)        BIT((x))     /* LED val */
> +
> +/* 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 */
>  

Thanks,

Maxime

Re: [PATCH v2 3/6] net: phy: DP83640: Add LED handling

[Russell King (Oracle)]

On Tue, Feb 27, 2024 at 10:58:06AM +0100, Maxime Chevallier wrote:
> > +		val = phy_read(phydev, PHYCR) & ~(LED_CNFG_1 | LED_CNFG_0);
> > +		switch (mode) {
> > +		case DP83640_LED_MODE_1:
> > +			val |= LED_CNFG_0;
> > +		break;
> > +		case DP83640_LED_MODE_2:
> > +			/* Keeping LED_CNFG_1 and LED_CNFG_0 unset */
> > +			break;
> > +		case DP83640_LED_MODE_3:
> > +			val |= LED_CNFG_1;
> > +			break;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +		phy_write(phydev, PHYCR, val);

This should also be phy_modify() as well. Any read-modify-write sequence
is open to race conditions if it is open coded because the bus lock will
be dropped after the read and regained on the write.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 3/6] net: phy: DP83640: Add LED handling

[Andrew Lunn]

> +static int dp83640_led_brightness_set(struct phy_device *phydev, u8 index,
> +				      enum led_brightness brightness)
> +{
> +	int val;
> +
> +	if (index > DP83640_SPDLED_IDX)
> +		return -EINVAL;
> +
> +	phy_write(phydev, PAGESEL, 0);
> +
> +	val = phy_read(phydev, LEDCR) & ~DP83640_LED_DIS(index);
> +	val |= DP83640_LED_DRV(index);
> +	if (brightness)
> +		val |= DP83640_LED_VAL(index);
> +	else
> +		val &= ~DP83640_LED_VAL(index);
> +	phy_write(phydev, LEDCR, val);

I don't understand this driver too well, but should this be using
ext_read() and ext_write().

I'm also woundering if it would be good to implement the .read_page
and .write_page members in phy_driver and then use phy_write_paged()
and phy_write_paged() and phy_modify_paged(), which should do better
locking.

	Andrew

Re: [PATCH v2 3/6] net: phy: DP83640: Add LED handling

[Bastien Curutchet]

Hi

On 2/28/24 16:04, Andrew Lunn wrote:
>> +static int dp83640_led_brightness_set(struct phy_device *phydev, u8 index,
>> +				      enum led_brightness brightness)
>> +{
>> +	int val;
>> +
>> +	if (index > DP83640_SPDLED_IDX)
>> +		return -EINVAL;
>> +
>> +	phy_write(phydev, PAGESEL, 0);
>> +
>> +	val = phy_read(phydev, LEDCR) & ~DP83640_LED_DIS(index);
>> +	val |= DP83640_LED_DRV(index);
>> +	if (brightness)
>> +		val |= DP83640_LED_VAL(index);
>> +	else
>> +		val &= ~DP83640_LED_VAL(index);
>> +	phy_write(phydev, LEDCR, val);
> I don't understand this driver too well, but should this be using
> ext_read() and ext_write().
>
> I'm also woundering if it would be good to implement the .read_page
> and .write_page members in phy_driver and then use phy_write_paged()
> and phy_write_paged() and phy_modify_paged(), which should do better
> locking.
I don't feel comfortable implementing .read_page and write_page members 
as I have
only one PHY on my board so I'll not be able to test all the broadcast 
thing.

If that's OK with you, I'll use the ext_read() and ext_write()


Best regards,

Bastien

[PATCH v2 4/6] net: phy: DP83640: Add EDPD management

[Bastien Curutchet]

The driver does not support enabling/disabling Energy Detect Power Down
(EDPD).
The PHY itself support EDPD.

Add missing part in the driver in order to have this support based on
ethtool {set,get}_phy_tunable functions.

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

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index c46c81ef0ad0..16c9fda50b19 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -1531,6 +1531,66 @@ static void dp83640_remove(struct phy_device *phydev)
 	kfree(dp83640);
 }
 
+static int dp83640_get_edpd(struct phy_device *phydev, u16 *edpd)
+{
+	int val;
+
+	phy_write(phydev, PAGESEL, 0);
+	val = phy_read(phydev, EDCR);
+	if (val & ED_EN)
+		*edpd = 2000; /* 2 seconds */
+	else
+		*edpd = ETHTOOL_PHY_EDPD_DISABLE;
+
+	return 0;
+}
+
+static int dp83640_set_edpd(struct phy_device *phydev, u16 edpd)
+{
+	int val;
+
+	phy_write(phydev, PAGESEL, 0);
+	val = phy_read(phydev, EDCR);
+
+	switch (edpd) {
+	case ETHTOOL_PHY_EDPD_DFLT_TX_MSECS:
+	case 2000: /* 2 seconds */
+		val |= ED_EN;
+		break;
+	case ETHTOOL_PHY_EDPD_DISABLE:
+		val &= ~ED_EN;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	phy_write(phydev, EDCR, val);
+
+	return 0;
+}
+
+static int dp83640_get_tunable(struct phy_device *phydev,
+			       struct ethtool_tunable *tuna, void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_EDPD:
+		return dp83640_get_edpd(phydev, data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int dp83640_set_tunable(struct phy_device *phydev,
+			       struct ethtool_tunable *tuna, const void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_EDPD:
+		return dp83640_set_edpd(phydev, *(u16 *)data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static int dp83640_led_brightness_set(struct phy_device *phydev, u8 index,
 				      enum led_brightness brightness)
 {
@@ -1692,6 +1752,8 @@ static struct phy_driver dp83640_driver = {
 	.name		= "NatSemi DP83640",
 	/* PHY_BASIC_FEATURES */
 	.probe		= dp83640_probe,
+	.get_tunable    = dp83640_get_tunable,
+	.set_tunable    = dp83640_set_tunable,
 	.remove		= dp83640_remove,
 	.soft_reset	= dp83640_soft_reset,
 	.config_init	= dp83640_config_init,
diff --git a/drivers/net/phy/dp83640_reg.h b/drivers/net/phy/dp83640_reg.h
index 09dd2d2527c7..bf34d422d91e 100644
--- a/drivers/net/phy/dp83640_reg.h
+++ b/drivers/net/phy/dp83640_reg.h
@@ -9,6 +9,7 @@
 #define LEDCR                     0x0018 /* PHY Control Register */
 #define PHYCR                     0x0019 /* PHY Control Register */
 #define PHYCR2                    0x001c /* PHY Control Register 2 */
+#define EDCR                      0x001D /* Energy Detect Control Register */
 
 #define PAGE4                     0x0004
 #define PTP_CTL                   0x0014 /* PTP Control Register */
@@ -64,6 +65,9 @@
 /* Bit definitions for the PHYCR2 register */
 #define BC_WRITE                  (1<<11) /* Broadcast Write Enable */
 
+/* Bit definitions for the EDCR register */
+#define ED_EN		          BIT(15) /* Enable Energy Detect Mode */
+
 /* Bit definitions for the PTP_CTL register */
 #define TRIG_SEL_SHIFT            (10)    /* PTP Trigger Select */
 #define TRIG_SEL_MASK             (0x7)
-- 
2.43.0

Re: [PATCH v2 4/6] net: phy: DP83640: Add EDPD management

[Maxime Chevallier]

Hello Bastien,

On Tue, 27 Feb 2024 10:39:43 +0100
Bastien Curutchet <bastien.curutchet@bootlin.com> wrote:

> The driver does not support enabling/disabling Energy Detect Power Down
> (EDPD).
> The PHY itself support EDPD.
> 
> Add missing part in the driver in order to have this support based on
> ethtool {set,get}_phy_tunable functions.
> 
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---
>  drivers/net/phy/dp83640.c     | 62 +++++++++++++++++++++++++++++++++++
>  drivers/net/phy/dp83640_reg.h |  4 +++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
> index c46c81ef0ad0..16c9fda50b19 100644
> --- a/drivers/net/phy/dp83640.c
> +++ b/drivers/net/phy/dp83640.c
> @@ -1531,6 +1531,66 @@ static void dp83640_remove(struct phy_device *phydev)
>  	kfree(dp83640);
>  }
>  
> +static int dp83640_get_edpd(struct phy_device *phydev, u16 *edpd)
> +{
> +	int val;
> +
> +	phy_write(phydev, PAGESEL, 0);

Same comment as the previous patch on paged accesses

> +	val = phy_read(phydev, EDCR);
> +	if (val & ED_EN)
> +		*edpd = 2000; /* 2 seconds */
> +	else
> +		*edpd = ETHTOOL_PHY_EDPD_DISABLE;
> +
> +	return 0;
> +}
> +
> +static int dp83640_set_edpd(struct phy_device *phydev, u16 edpd)
> +{
> +	int val;
> +
> +	phy_write(phydev, PAGESEL, 0);

ditto

> +	val = phy_read(phydev, EDCR);
> +
> +	switch (edpd) {
> +	case ETHTOOL_PHY_EDPD_DFLT_TX_MSECS:
> +	case 2000: /* 2 seconds */
> +		val |= ED_EN;
> +		break;
> +	case ETHTOOL_PHY_EDPD_DISABLE:
> +		val &= ~ED_EN;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	phy_write(phydev, EDCR, val);

Here you could use phy_clear_bits / phy_set_bits, which are flavors of
the phy_modify accessors.

> +
> +	return 0;
> +}
> +
> +static int dp83640_get_tunable(struct phy_device *phydev,
> +			       struct ethtool_tunable *tuna, void *data)
> +{
> +	switch (tuna->id) {
> +	case ETHTOOL_PHY_EDPD:
> +		return dp83640_get_edpd(phydev, data);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int dp83640_set_tunable(struct phy_device *phydev,
> +			       struct ethtool_tunable *tuna, const void *data)
> +{
> +	switch (tuna->id) {
> +	case ETHTOOL_PHY_EDPD:
> +		return dp83640_set_edpd(phydev, *(u16 *)data);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
>  static int dp83640_led_brightness_set(struct phy_device *phydev, u8 index,
>  				      enum led_brightness brightness)
>  {
> @@ -1692,6 +1752,8 @@ static struct phy_driver dp83640_driver = {
>  	.name		= "NatSemi DP83640",
>  	/* PHY_BASIC_FEATURES */
>  	.probe		= dp83640_probe,
> +	.get_tunable    = dp83640_get_tunable,
> +	.set_tunable    = dp83640_set_tunable,
>  	.remove		= dp83640_remove,
>  	.soft_reset	= dp83640_soft_reset,
>  	.config_init	= dp83640_config_init,
> diff --git a/drivers/net/phy/dp83640_reg.h b/drivers/net/phy/dp83640_reg.h
> index 09dd2d2527c7..bf34d422d91e 100644
> --- a/drivers/net/phy/dp83640_reg.h
> +++ b/drivers/net/phy/dp83640_reg.h
> @@ -9,6 +9,7 @@
>  #define LEDCR                     0x0018 /* PHY Control Register */
>  #define PHYCR                     0x0019 /* PHY Control Register */
>  #define PHYCR2                    0x001c /* PHY Control Register 2 */
> +#define EDCR                      0x001D /* Energy Detect Control Register */
>  
>  #define PAGE4                     0x0004
>  #define PTP_CTL                   0x0014 /* PTP Control Register */
> @@ -64,6 +65,9 @@
>  /* Bit definitions for the PHYCR2 register */
>  #define BC_WRITE                  (1<<11) /* Broadcast Write Enable */
>  
> +/* Bit definitions for the EDCR register */
> +#define ED_EN		          BIT(15) /* Enable Energy Detect Mode */
> +
>  /* Bit definitions for the PTP_CTL register */
>  #define TRIG_SEL_SHIFT            (10)    /* PTP Trigger Select */
>  #define TRIG_SEL_MASK             (0x7)

Thanks,

Maxime

[PATCH v2 5/6] net: phy: DP83640: Explicitly disabling PHY Control Frames

[Bastien Curutchet]

The PHY offers a PHY control frame feature that allows to access PHY
registers through the MAC transmit data interface. This functionality
is not handled by the driver but can be enabled via hardware strap or
register access.

Disable the feature in config_init() to save some latency on MII packets.

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

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 16c9fda50b19..b371dea23937 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -1120,6 +1120,7 @@ static int dp83640_config_init(struct phy_device *phydev)
 {
 	struct dp83640_private *dp83640 = phydev->priv;
 	struct dp83640_clock *clock = dp83640->clock;
+	int val;
 
 	if (clock->chosen && !list_empty(&clock->phylist))
 		recalibrate(clock);
@@ -1135,6 +1136,11 @@ static int dp83640_config_init(struct phy_device *phydev)
 	ext_write(0, phydev, PAGE4, PTP_CTL, PTP_ENABLE);
 	mutex_unlock(&clock->extreg_lock);
 
+	/* Disable unused PHY control frames */
+	phy_write(phydev, PAGESEL, 0);
+	val = phy_read(phydev, PCFCR) & ~PCF_EN;
+	phy_write(phydev, PCFCR, val);
+
 	return 0;
 }
 
diff --git a/drivers/net/phy/dp83640_reg.h b/drivers/net/phy/dp83640_reg.h
index bf34d422d91e..b5adb8958c08 100644
--- a/drivers/net/phy/dp83640_reg.h
+++ b/drivers/net/phy/dp83640_reg.h
@@ -10,6 +10,7 @@
 #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 */
@@ -68,6 +69,9 @@
 /* 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 */
 #define TRIG_SEL_MASK             (0x7)
-- 
2.43.0

Re: [PATCH v2 5/6] net: phy: DP83640: Explicitly disabling PHY Control Frames

[Maxime Chevallier]

Hi Bastien,

On Tue, 27 Feb 2024 10:39:44 +0100
Bastien Curutchet <bastien.curutchet@bootlin.com> wrote:

> The PHY offers a PHY control frame feature that allows to access PHY
> registers through the MAC transmit data interface. This functionality
> is not handled by the driver but can be enabled via hardware strap or
> register access.
> 
> Disable the feature in config_init() to save some latency on MII packets.
> 
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---
>  drivers/net/phy/dp83640.c     | 6 ++++++
>  drivers/net/phy/dp83640_reg.h | 4 ++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
> index 16c9fda50b19..b371dea23937 100644
> --- a/drivers/net/phy/dp83640.c
> +++ b/drivers/net/phy/dp83640.c
> @@ -1120,6 +1120,7 @@ static int dp83640_config_init(struct phy_device *phydev)
>  {
>  	struct dp83640_private *dp83640 = phydev->priv;
>  	struct dp83640_clock *clock = dp83640->clock;
> +	int val;
>  
>  	if (clock->chosen && !list_empty(&clock->phylist))
>  		recalibrate(clock);
> @@ -1135,6 +1136,11 @@ static int dp83640_config_init(struct phy_device *phydev)
>  	ext_write(0, phydev, PAGE4, PTP_CTL, PTP_ENABLE);
>  	mutex_unlock(&clock->extreg_lock);
>  
> +	/* Disable unused PHY control frames */
> +	phy_write(phydev, PAGESEL, 0);
> +	val = phy_read(phydev, PCFCR) & ~PCF_EN;
> +	phy_write(phydev, PCFCR, val);

Use phy_modify instead, and you might also want to look at the paging.
The ext_write before apparently does some page-management itself through
the clock struct (?).

> +
>  	return 0;
>  }
>  
> diff --git a/drivers/net/phy/dp83640_reg.h b/drivers/net/phy/dp83640_reg.h
> index bf34d422d91e..b5adb8958c08 100644
> --- a/drivers/net/phy/dp83640_reg.h
> +++ b/drivers/net/phy/dp83640_reg.h
> @@ -10,6 +10,7 @@
>  #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 */
> @@ -68,6 +69,9 @@
>  /* 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 */
>  #define TRIG_SEL_MASK             (0x7)

Thanks,

Maxime

[PATCH v2 6/6] net: phy: DP83640: Add fiber mode enabling/disabling from device tree

[Bastien Curutchet]

The PHY is able to use copper or fiber. The fiber mode can be enabled or
disabled by hardware strap. If hardware strap is incorrect, PHY can't
establish link.

Add a DT attribute 'ti,fiber-mode' that can be use to override the
hardware strap configuration. If the property is not present, hardware
strap configuration is left as is.

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

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index b371dea23937..886f2bc3710d 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -16,6 +16,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>
@@ -141,6 +142,11 @@ struct dp83640_private {
 	/* queues of incoming and outgoing packets */
 	struct sk_buff_head rx_queue;
 	struct sk_buff_head tx_queue;
+
+#define FIBER_MODE_DEFAULT	0
+#define FIBER_MODE_ENABLE	1
+#define FIBER_MODE_DISABLE	2
+	int fiber;
 };
 
 struct dp83640_clock {
@@ -1141,6 +1147,17 @@ static int dp83640_config_init(struct phy_device *phydev)
 	val = phy_read(phydev, PCFCR) & ~PCF_EN;
 	phy_write(phydev, PCFCR, val);
 
+	if (dp83640->fiber != FIBER_MODE_DEFAULT) {
+		val = phy_read(phydev, PCSR) & ~FX_EN;
+		if (dp83640->fiber == FIBER_MODE_ENABLE)
+			val |= FX_EN;
+		phy_write(phydev, PCSR, val);
+
+		/* Write SOFT_RESET bit to ensure configuration */
+		val = phy_read(phydev, PHYCR2) | SOFT_RESET;
+		phy_write(phydev, PHYCR2, val);
+	}
+
 	return 0;
 }
 
@@ -1440,6 +1457,39 @@ 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 dp83640_private *dp83640 = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+	struct device_node *of_node = dev->of_node;
+	const char *fiber;
+	int ret;
+
+	if (of_property_present(of_node, "ti,fiber-mode")) {
+		ret = of_property_read_string(of_node, "ti,fiber-mode", &fiber);
+		if (ret)
+			return ret;
+
+		dp83640->fiber = FIBER_MODE_DEFAULT;
+		if (!strncmp(fiber, "enable", 6))
+			dp83640->fiber = FIBER_MODE_ENABLE;
+		else if (!strncmp(fiber, "disable", 7))
+			dp83640->fiber = FIBER_MODE_DISABLE;
+		else
+			return -EINVAL;
+	}
+
+	return 0;
+}
+#else
+static int dp83640_of_init(struct phy_device *phydev)
+{
+	dp83640->fiber = FIBER_MODE_DEFAULT;
+	return 0;
+}
+#endif /* CONFIG_OF_MDIO */
+
 static int dp83640_probe(struct phy_device *phydev)
 {
 	struct dp83640_clock *clock;
@@ -1472,6 +1522,10 @@ static int dp83640_probe(struct phy_device *phydev)
 	phydev->mii_ts = &dp83640->mii_ts;
 	phydev->priv = dp83640;
 
+	err = dp83640_of_init(phydev);
+	if (err < 0)
+		goto of_failed;
+
 	spin_lock_init(&dp83640->rx_lock);
 	skb_queue_head_init(&dp83640->rx_queue);
 	skb_queue_head_init(&dp83640->tx_queue);
@@ -1494,6 +1548,7 @@ static int dp83640_probe(struct phy_device *phydev)
 
 no_register:
 	clock->chosen = NULL;
+of_failed:
 	kfree(dp83640);
 no_memory:
 	dp83640_clock_put(clock);
diff --git a/drivers/net/phy/dp83640_reg.h b/drivers/net/phy/dp83640_reg.h
index b5adb8958c08..cbecf04da5a5 100644
--- a/drivers/net/phy/dp83640_reg.h
+++ b/drivers/net/phy/dp83640_reg.h
@@ -6,6 +6,7 @@
 #define HAVE_DP83640_REGISTERS
 
 /* #define PAGE0                  0x0000 */
+#define PCSR                      0x0016 /* PCS Configuration and Status Register */
 #define LEDCR                     0x0018 /* PHY Control Register */
 #define PHYCR                     0x0019 /* PHY Control Register */
 #define PHYCR2                    0x001c /* PHY Control Register 2 */
@@ -54,6 +55,9 @@
 #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 LEDCR register */
 #define DP83640_LED_DIS(x)        BIT((x) + 9) /* Disable LED */
 #define DP83640_LED_DRV(x)        BIT((x) + 3) /* Force LED val to output */
@@ -64,6 +68,7 @@
 #define LED_CNFG_1	          BIT(6)  /* LED configuration, bit 1 */
 
 /* Bit definitions for the PHYCR2 register */
+#define SOFT_RESET		  BIT(9)  /* Soft Reset */
 #define BC_WRITE                  (1<<11) /* Broadcast Write Enable */
 
 /* Bit definitions for the EDCR register */
-- 
2.43.0

Re: [PATCH v2 6/6] net: phy: DP83640: Add fiber mode enabling/disabling from device tree

[Russell King (Oracle)]

On Tue, Feb 27, 2024 at 10:39:45AM +0100, Bastien Curutchet wrote:
> @@ -1141,6 +1147,17 @@ static int dp83640_config_init(struct phy_device *phydev)
>  	val = phy_read(phydev, PCFCR) & ~PCF_EN;
>  	phy_write(phydev, PCFCR, val);
>  
> +	if (dp83640->fiber != FIBER_MODE_DEFAULT) {
> +		val = phy_read(phydev, PCSR) & ~FX_EN;
> +		if (dp83640->fiber == FIBER_MODE_ENABLE)
> +			val |= FX_EN;
> +		phy_write(phydev, PCSR, val);

		val = 0;
		if (dp83640->fiber == FIBER_MODE_ENABLE)
			val = FX_EN;

		phy_modify(phydev, PCSR, FX_EN, val);

> +
> +		/* Write SOFT_RESET bit to ensure configuration */
> +		val = phy_read(phydev, PHYCR2) | SOFT_RESET;
> +		phy_write(phydev, PHYCR2, val);

		phy_set_bits(phydev, PHYCR2, SOFT_RESET);

...
> +#else
> +static int dp83640_of_init(struct phy_device *phydev)
> +{
> +	dp83640->fiber = FIBER_MODE_DEFAULT;

This hasn't been build tested - dp83640 won't exist here.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 6/6] net: phy: DP83640: Add fiber mode enabling/disabling from device tree

[Andrew Lunn]

On Tue, Feb 27, 2024 at 10:39:45AM +0100, Bastien Curutchet wrote:
> The PHY is able to use copper or fiber. The fiber mode can be enabled or
> disabled by hardware strap. If hardware strap is incorrect, PHY can't
> establish link.
> 
> Add a DT attribute 'ti,fiber-mode' that can be use to override the
> hardware strap configuration. If the property is not present, hardware
> strap configuration is left as is.

How have you tested this? Do you have a RDK with it connected to an
SFP cage?

    Andrew

Re: [PATCH v2 6/6] net: phy: DP83640: Add fiber mode enabling/disabling from device tree

[Bastien Curutchet]

Hi Andrew,

On 2/27/24 17:18, Andrew Lunn wrote:
> On Tue, Feb 27, 2024 at 10:39:45AM +0100, Bastien Curutchet wrote:
>> The PHY is able to use copper or fiber. The fiber mode can be enabled or
>> disabled by hardware strap. If hardware strap is incorrect, PHY can't
>> establish link.
>>
>> Add a DT attribute 'ti,fiber-mode' that can be use to override the
>> hardware strap configuration. If the property is not present, hardware
>> strap configuration is left as is.
> How have you tested this? Do you have a RDK with it connected to an
> SFP cage?

I did not test fiber mode as my board uses copper.

My use case is that I need to explicitly disable the fiber mode because 
the strap hardware is
misconfigured and could possibly enable fiber mode from time to time.


Best regards,

Bastien

Re: [PATCH v2 6/6] net: phy: DP83640: Add fiber mode enabling/disabling from device tree

[Andrew Lunn]

On Thu, Feb 29, 2024 at 08:31:55AM +0100, Bastien Curutchet wrote:
> Hi Andrew,
> 
> On 2/27/24 17:18, Andrew Lunn wrote:
> > On Tue, Feb 27, 2024 at 10:39:45AM +0100, Bastien Curutchet wrote:
> > > The PHY is able to use copper or fiber. The fiber mode can be enabled or
> > > disabled by hardware strap. If hardware strap is incorrect, PHY can't
> > > establish link.
> > > 
> > > Add a DT attribute 'ti,fiber-mode' that can be use to override the
> > > hardware strap configuration. If the property is not present, hardware
> > > strap configuration is left as is.
> > How have you tested this? Do you have a RDK with it connected to an
> > SFP cage?
> 
> I did not test fiber mode as my board uses copper.
> 
> My use case is that I need to explicitly disable the fiber mode because the
> strap hardware is
> misconfigured and could possibly enable fiber mode from time to time.

O.K. So lets refocus this is little. Rather than support fibre mode,
just support disabling fibre mode. But leave a clear path for somebody
to add fibre support sometime in the future.

Looking at the current code, do you think fibre mode actually works
today? If you think it cannot actually work today in fibre mode, one
option would be to hard code it to copper mode. Leave the
configuration between fibre and copper mode to the future when
somebody actually implements fibre mode.

   Andrew

Re: [PATCH v2 6/6] net: phy: DP83640: Add fiber mode enabling/disabling from device tree

[Maxime Chevallier]

Hi Bastien, Andrew,

On Thu, 29 Feb 2024 16:23:59 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, Feb 29, 2024 at 08:31:55AM +0100, Bastien Curutchet wrote:
> > Hi Andrew,
> > 
> > On 2/27/24 17:18, Andrew Lunn wrote:  
> > > On Tue, Feb 27, 2024 at 10:39:45AM +0100, Bastien Curutchet wrote:  
> > > > The PHY is able to use copper or fiber. The fiber mode can be enabled or
> > > > disabled by hardware strap. If hardware strap is incorrect, PHY can't
> > > > establish link.
> > > > 
> > > > Add a DT attribute 'ti,fiber-mode' that can be use to override the
> > > > hardware strap configuration. If the property is not present, hardware
> > > > strap configuration is left as is.  
> > > How have you tested this? Do you have a RDK with it connected to an
> > > SFP cage?  
> > 
> > I did not test fiber mode as my board uses copper.
> > 
> > My use case is that I need to explicitly disable the fiber mode because the
> > strap hardware is
> > misconfigured and could possibly enable fiber mode from time to time.  
> 
> O.K. So lets refocus this is little. Rather than support fibre mode,
> just support disabling fibre mode. But leave a clear path for somebody
> to add fibre support sometime in the future.
> 
> Looking at the current code, do you think fibre mode actually works
> today? If you think it cannot actually work today in fibre mode, one
> option would be to hard code it to copper mode. Leave the
> configuration between fibre and copper mode to the future when
> somebody actually implements fibre mode.

Looking at the driver and the datasheet, it's hard to say that the
fiber mode can't work in the current state. It's configured either
through straps or an overriding register, and it's enough to get the
scrambler/descrambler automatically setup according to that single
strap. 

So it's hard to say that defaulting to copper won't break anything :(

OTOH there's no SFP support in this PHY, in terms of register config,
some aneg modes won't work in 100BaseFX, which the driver doesn't account for,
So nothing would indicate that the fiber mode was ever used.

There's the DP83822 driver that can accept the "ti,fiber-mode"
property, so adding that would at least be coherent with other DP83xxx
PHYs but it has the opposite logic we want, so doesn't prevent any
possible regression for existing fiber users.

All in all, do you think that defaulting to copper and leaving users an
option to implement "ti,fiber-mode" is an acceptable risk to take ?

Maxime

Re: [PATCH v2 6/6] net: phy: DP83640: Add fiber mode enabling/disabling from device tree

[Andrew Lunn]

> All in all, do you think that defaulting to copper and leaving users an
> option to implement "ti,fiber-mode" is an acceptable risk to take ?

Yes, lets do that. But try to make it easy to revert the change if
anybody complains about a regression.

	Andrew