[PATCH v4 0/4] net: phy: realtek: Enable configuration of RTL8211E LEDs

[PATCH v4 0/4] net: phy: realtek: Enable configuration of RTL8211E LEDs

[Matthias Kaehlcke]

The Realtek RTL8211E allows customization of the PHY LED behavior,
like which LEDs are on for certain link speeds and which LEDs blink
when there is traffic. By default EEE LED mode is enabled, in which
a blinking LED is on for 400ms and off for 2s. This series adds a
generic device tree binding for configuring PHY LEDs and adds LED
configuration support for the RTL8211E PHY.

Certain registers of the RTL8211E can only be accessed through
a vendor specific extended page mechanism. Extended pages need
to be accessed for the LED configuration. This series adds helpers
to facilitate accessing extended pages.

Matthias Kaehlcke (4):
  dt-bindings: net: phy: Add subnode for LED configuration
  net: phy: Add function to retrieve LED configuration from the DT
  net: phy: realtek: Add helpers for accessing RTL8211E extension pages
  net: phy: realtek: configure RTL8211E LEDs

 .../devicetree/bindings/net/ethernet-phy.yaml |  47 +++++
 drivers/net/phy/phy_device.c                  |  50 ++++++
 drivers/net/phy/realtek.c                     | 169 ++++++++++++++++--
 include/linux/phy.h                           |  15 ++
 4 files changed, 266 insertions(+), 15 deletions(-)

-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v4 1/4] dt-bindings: net: phy: Add subnode for LED configuration

[Matthias Kaehlcke]

The LED behavior of some Ethernet PHYs is configurable. Add an
optional 'leds' subnode with a child node for each LED to be
configured. The binding aims to be compatible with the common
LED binding (see devicetree/bindings/leds/common.txt).

A LED can be configured to be 'on' when a link with a certain speed
is active, or to blink on RX/TX activity. For the configuration to
be effective it needs to be supported by the hardware and the
corresponding PHY driver.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v4:
- patch added to the series
---
 .../devicetree/bindings/net/ethernet-phy.yaml | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index f70f18ff821f..81c5aacc89a5 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -153,6 +153,38 @@ properties:
       Delay after the reset was deasserted in microseconds. If
       this property is missing the delay will be skipped.
 
+patternProperties:
+  "^leds$":
+    type: object
+    description:
+      Subnode with configuration of the PHY LEDs.
+
+    patternProperties:
+      "^led@[0-9]+$":
+        type: object
+        description:
+          Subnode with the configuration of a single PHY LED.
+
+    properties:
+      reg:
+        description:
+          The ID number of the LED, typically corresponds to a hardware ID.
+        $ref: "/schemas/types.yaml#/definitions/uint32"
+
+      linux,default-trigger:
+        description:
+          This parameter, if present, is a string specifying the trigger
+          assigned to the LED. Supported triggers are:
+            "phy_link_10m_active" - LED will be on when a 10Mb/s link is active
+            "phy_link_100m_active" - LED will be on when a 100Mb/s link is active
+            "phy_link_1g_active" - LED will be on when a 1Gb/s link is active
+            "phy_link_10g_active" - LED will be on when a 10Gb/s link is active
+            "phy_activity" - LED will blink when data is received or transmitted
+        $ref: "/schemas/types.yaml#/definitions/string"
+
+    required:
+      - reg
+
 required:
   - reg
 
@@ -173,5 +205,20 @@ examples:
             reset-gpios = <&gpio1 4 1>;
             reset-assert-us = <1000>;
             reset-deassert-us = <2000>;
+
+            leds {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                led@0 {
+                    reg = <0>;
+                    linux,default-trigger = "phy_link_1g_active";
+                };
+
+                led@1 {
+                    reg = <1>;
+                    linux,default-trigger = "phy_activity";
+                };
+            };
         };
     };
-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v4 2/4] net: phy: Add function to retrieve LED configuration from the DT

[Matthias Kaehlcke]

Add a phylib function for retrieving PHY LED configuration that
is specified in the device tree using the generic binding. LEDs
can be configured to be 'on' for a certain link speed or to blink
when there is TX/RX activity.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v4:
- patch added to the series
---
 drivers/net/phy/phy_device.c | 50 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          | 15 +++++++++++
 2 files changed, 65 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6b5cb87f3866..b4b48de45712 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2188,6 +2188,56 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
 	return phydrv->config_intr && phydrv->ack_interrupt;
 }
 
+int of_get_phy_led_cfg(struct phy_device *phydev, int led,
+		       struct phy_led_config *cfg)
+{
+	struct device_node *np, *child;
+	const char *trigger;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_OF_MDIO))
+		return -ENOENT;
+
+	np = of_find_node_by_name(phydev->mdio.dev.of_node, "leds");
+	if (!np)
+		return -ENOENT;
+
+	for_each_child_of_node(np, child) {
+		u32 val;
+
+		if (!of_property_read_u32(child, "reg", &val)) {
+			if (val == (u32)led)
+				break;
+		}
+	}
+
+	if (!child)
+		return -ENOENT;
+
+	ret = of_property_read_string(child, "linux,default-trigger",
+				      &trigger);
+	if (ret)
+		return ret;
+
+	if (!strcmp(trigger, "phy_link_10m_active")) {
+		cfg->trigger = PHY_LED_LINK_10M;
+	} else if (!strcmp(trigger, "phy_link_100m_active")) {
+		cfg->trigger = PHY_LED_LINK_100M;
+	} else if (!strcmp(trigger, "phy_link_1g_active")) {
+		cfg->trigger = PHY_LED_LINK_1G;
+	} else if (!strcmp(trigger, "phy_link_10g_active")) {
+		cfg->trigger = PHY_LED_LINK_10G;
+	}  else if (!strcmp(trigger, "phy_activity")) {
+		cfg->trigger = PHY_LED_ACTIVITY;
+	} else {
+		phydev_warn(phydev, "trigger '%s' for LED%d is invalid\n",
+			    trigger, led);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /**
  * phy_probe - probe and init a PHY device
  * @dev: device to probe and init
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 462b90b73f93..b4693415be31 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1176,6 +1176,21 @@ int phy_ethtool_set_link_ksettings(struct net_device *ndev,
 				   const struct ethtool_link_ksettings *cmd);
 int phy_ethtool_nway_reset(struct net_device *ndev);
 
+enum phy_led_trigger {
+	PHY_LED_LINK_10M,
+	PHY_LED_LINK_100M,
+	PHY_LED_LINK_1G,
+	PHY_LED_LINK_10G,
+	PHY_LED_ACTIVITY,
+};
+
+struct phy_led_config {
+	enum phy_led_trigger trigger;
+};
+
+int of_get_phy_led_cfg(struct phy_device *phydev, int led,
+		       struct phy_led_config *cfg);
+
 #if IS_ENABLED(CONFIG_PHYLIB)
 int __init mdio_bus_init(void);
 void mdio_bus_exit(void);
-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v4 3/4] net: phy: realtek: Add helpers for accessing RTL8211E extension pages

[Matthias Kaehlcke]

The RTL8211E has extension pages, which can be accessed after
selecting a page through a custom method. Add a function to
modify bits in a register of an extension page and a helper for
selecting an ext page. Use rtl8211e_modify_ext_paged() in
rtl8211e_config_init() instead of doing things 'manually'.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v4:
- don't add constant RTL8211E_EXT_PAGE, it's only used once,
  use a literal instead
- pass 'oldpage' to phy_restore_page() in rtl8211e_select_ext_page(),
  not 'page'
- return 'oldpage' in rtl8211e_select_ext_page()
- use __phy_modify() in rtl8211e_modify_ext_paged() instead of
  reimplementing __phy_modify_changed()
- in rtl8211e_modify_ext_paged() return directly when
  rtl8211e_select_ext_page() fails
---
 drivers/net/phy/realtek.c | 48 +++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index a669945eb829..e09d3b0da2c7 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -53,6 +53,36 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
 	return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
 }
 
+static int rtl8211e_select_ext_page(struct phy_device *phydev, int page)
+{
+	int ret, oldpage;
+
+	oldpage = phy_select_page(phydev, 7);
+	if (oldpage < 0)
+		return oldpage;
+
+	ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, page);
+	if (ret)
+		return phy_restore_page(phydev, oldpage, ret);
+
+	return oldpage;
+}
+
+static int rtl8211e_modify_ext_paged(struct phy_device *phydev, int page,
+				     u32 regnum, u16 mask, u16 set)
+{
+	int ret = 0;
+	int oldpage;
+
+	oldpage = rtl8211e_select_ext_page(phydev, page);
+	if (oldpage < 0)
+		return oldpage;
+
+	ret = __phy_modify(phydev, regnum, mask, set);
+
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
 static int rtl8201_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -184,7 +214,7 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 
 static int rtl8211e_config_init(struct phy_device *phydev)
 {
-	int ret = 0, oldpage;
+	int ret;
 	u16 val;
 
 	/* enable TX/RX delay for rgmii-* modes, and disable them for rgmii. */
@@ -213,19 +243,9 @@ static int rtl8211e_config_init(struct phy_device *phydev)
 	 * 2 = RX Delay, 1 = TX Delay, 0 = SELRGV (see original PHY datasheet
 	 * for details).
 	 */
-	oldpage = phy_select_page(phydev, 0x7);
-	if (oldpage < 0)
-		goto err_restore_page;
-
-	ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
-	if (ret)
-		goto err_restore_page;
-
-	ret = __phy_modify(phydev, 0x1c, RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
-			   val);
-
-err_restore_page:
-	return phy_restore_page(phydev, oldpage, ret);
+	return rtl8211e_modify_ext_paged(phydev, 0xa4, 0x1c,
+					 RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
+					 val);
 }
 
 static int rtl8211b_suspend(struct phy_device *phydev)
-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v4 4/4] net: phy: realtek: configure RTL8211E LEDs

[Matthias Kaehlcke]

Configure the RTL8211E LEDs behavior when the device tree property
'realtek,led-modes' is specified.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v4:
- use the generic PHY LED binding
- keep default/current configuration if none is specified
- added rtl8211e_disable_eee_led_mode()
  - was previously in separate patch, however since we always want to
    disable EEE LED mode when a LED configuration is specified it makes
    sense to just add the function here.
- don't call phy_restore_page() in rtl8211e_config_leds() if
  selection of the extended page failed.
- use phydev_warn() instead of phydev_err() if LED configuration
  fails since we don't bail out
- use hex number to specify page for consistency
- add hex number to comment about ext page 44 to facilitate searching
---
 drivers/net/phy/realtek.c | 121 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 120 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index e09d3b0da2c7..46e3d77d41b6 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -9,8 +9,11 @@
  * Copyright (c) 2004 Freescale Semiconductor, Inc.
  */
 #include <linux/bitops.h>
-#include <linux/phy.h>
+#include <linux/bits.h>
 #include <linux/module.h>
+#include <linux/phy.h>
+
+#define RTL821x_NUM_LEDS			3
 
 #define RTL821x_PHYSR				0x11
 #define RTL821x_PHYSR_DUPLEX			BIT(13)
@@ -26,6 +29,19 @@
 #define RTL821x_EXT_PAGE_SELECT			0x1e
 #define RTL821x_PAGE_SELECT			0x1f
 
+/* RTL8211E page 5 */
+#define RTL8211E_EEE_LED_MODE1			0x05
+#define RTL8211E_EEE_LED_MODE2			0x06
+
+/* RTL8211E extension page 44 (0x2c) */
+#define RTL8211E_LACR				0x1a
+#define RLT8211E_LACR_LEDACTCTRL_SHIFT		4
+#define RLT8211E_LACR_LEDACTCTRL_MASK		GENMASK(6, 4)
+#define RTL8211E_LCR				0x1c
+#define RTL8211E_LCR_LEDCTRL_MASK		(GENMASK(2, 0) | \
+						 GENMASK(6, 4) | \
+						 GENMASK(10, 8))
+
 #define RTL8211F_INSR				0x1d
 
 #define RTL8211F_TX_DELAY			BIT(8)
@@ -83,6 +99,105 @@ static int rtl8211e_modify_ext_paged(struct phy_device *phydev, int page,
 	return phy_restore_page(phydev, oldpage, ret);
 }
 
+static void rtl8211e_disable_eee_led_mode(struct phy_device *phydev)
+{
+	int oldpage;
+	int err = 0;
+
+	oldpage = phy_select_page(phydev, 5);
+	if (oldpage < 0)
+		goto out;
+
+	/* write magic values to disable EEE LED mode */
+	err = __phy_write(phydev, RTL8211E_EEE_LED_MODE1, 0x8b82);
+	if (err)
+		goto out;
+
+	err = __phy_write(phydev, RTL8211E_EEE_LED_MODE2, 0x052b);
+
+out:
+	phy_restore_page(phydev, oldpage, err);
+}
+
+static int rtl8211e_config_leds(struct phy_device *phydev)
+{
+	int i, oldpage, ret;
+	u16 lacr_bits = 0, lcr_bits = 0;
+	u16 lacr_mask = RLT8211E_LACR_LEDACTCTRL_MASK;
+	u16 lcr_mask = RTL8211E_LCR_LEDCTRL_MASK;
+	bool eed_led_mode_disabled = false;
+
+	for (i = 0; i < RTL821x_NUM_LEDS; i++) {
+		struct phy_led_config cfg;
+
+		ret = of_get_phy_led_cfg(phydev, i, &cfg);
+		if (ret) {
+			lacr_mask &= ~BIT(4 + i);
+			lcr_mask &= ~GENMASK((i * 4) + 2, i * 4);
+			continue;
+		}
+
+		if (!eed_led_mode_disabled) {
+			rtl8211e_disable_eee_led_mode(phydev);
+			eed_led_mode_disabled = true;
+		}
+
+		switch (cfg.trigger) {
+		case PHY_LED_LINK_10M:
+			lcr_bits |= 1 << (i * 4);
+			break;
+
+		case PHY_LED_LINK_100M:
+			lcr_bits |= 2 << (i * 4);
+			break;
+
+		case PHY_LED_LINK_1G:
+			lcr_bits |= 4 << (i * 4);
+			break;
+
+		case PHY_LED_ACTIVITY:
+			lacr_bits |= BIT(RLT8211E_LACR_LEDACTCTRL_SHIFT + i);
+			break;
+
+		default:
+			phydev_warn(phydev,
+				    "unknown trigger for LED%d: %d\n",
+				    i, cfg.trigger);
+		}
+	}
+
+	oldpage = rtl8211e_select_ext_page(phydev, 0x2c);
+	if (oldpage < 0) {
+		phydev_err(phydev, "failed to select extended page: %d\n", oldpage);
+		return oldpage;
+	}
+
+	if (lacr_mask == 0)
+		goto skip_lacr;
+
+	ret = __phy_modify(phydev, RTL8211E_LACR,
+			   lacr_mask, lacr_bits);
+	if (ret) {
+		phydev_err(phydev, "failed to write LACR reg: %d\n",
+			   ret);
+		goto err;
+	}
+
+skip_lacr:
+	if (lcr_mask == 0)
+		goto skip_lcr;
+
+	ret = __phy_modify(phydev, RTL8211E_LCR,
+			   lcr_mask, lcr_bits);
+	if (ret)
+		phydev_err(phydev, "failed to write LCR reg: %d\n",
+			   ret);
+
+skip_lcr:
+err:
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
 static int rtl8201_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -217,6 +332,10 @@ static int rtl8211e_config_init(struct phy_device *phydev)
 	int ret;
 	u16 val;
 
+	ret = rtl8211e_config_leds(phydev);
+	if (ret)
+		phydev_warn(phydev, "LED configuration failed: %d\n", ret);
+
 	/* enable TX/RX delay for rgmii-* modes, and disable them for rgmii. */
 	switch (phydev->interface) {
 	case PHY_INTERFACE_MODE_RGMII:
-- 
2.22.0.770.g0f2c4a37fd-goog

Re: [PATCH v4 4/4] net: phy: realtek: configure RTL8211E LEDs

[David Miller]

From: Matthias Kaehlcke <mka@chromium.org>
Date: Thu,  1 Aug 2019 12:07:59 -0700

> +static int rtl8211e_config_leds(struct phy_device *phydev)
> +{
> +	int i, oldpage, ret;
> +	u16 lacr_bits = 0, lcr_bits = 0;
> +	u16 lacr_mask = RLT8211E_LACR_LEDACTCTRL_MASK;
> +	u16 lcr_mask = RTL8211E_LCR_LEDCTRL_MASK;
> +	bool eed_led_mode_disabled = false;

Please use reverse christmas tree ordering here.

Re: [PATCH v4 2/4] net: phy: Add function to retrieve LED configuration from the DT

[Andrew Lunn]

On Thu, Aug 01, 2019 at 12:07:57PM -0700, Matthias Kaehlcke wrote:
> Add a phylib function for retrieving PHY LED configuration that
> is specified in the device tree using the generic binding. LEDs
> can be configured to be 'on' for a certain link speed or to blink
> when there is TX/RX activity.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> Changes in v4:
> - patch added to the series
> ---
>  drivers/net/phy/phy_device.c | 50 ++++++++++++++++++++++++++++++++++++
>  include/linux/phy.h          | 15 +++++++++++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 6b5cb87f3866..b4b48de45712 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2188,6 +2188,56 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
>  	return phydrv->config_intr && phydrv->ack_interrupt;
>  }
>  
> +int of_get_phy_led_cfg(struct phy_device *phydev, int led,
> +		       struct phy_led_config *cfg)
> +{
> +	struct device_node *np, *child;
> +	const char *trigger;
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_OF_MDIO))
> +		return -ENOENT;
> +
> +	np = of_find_node_by_name(phydev->mdio.dev.of_node, "leds");
> +	if (!np)
> +		return -ENOENT;
> +
> +	for_each_child_of_node(np, child) {
> +		u32 val;
> +
> +		if (!of_property_read_u32(child, "reg", &val)) {
> +			if (val == (u32)led)
> +				break;
> +		}
> +	}

Hi Matthias

This is leaking references to np and child. In the past we have not
cared about this too much, but we are now getting patches adding the
missing releases. So it would be good to fix this.

	Andrew

Re: [PATCH v4 1/4] dt-bindings: net: phy: Add subnode for LED configuration

[Andrew Lunn]

On Thu, Aug 01, 2019 at 12:07:56PM -0700, Matthias Kaehlcke wrote:
> The LED behavior of some Ethernet PHYs is configurable. Add an
> optional 'leds' subnode with a child node for each LED to be
> configured. The binding aims to be compatible with the common
> LED binding (see devicetree/bindings/leds/common.txt).
> 
> A LED can be configured to be 'on' when a link with a certain speed
> is active, or to blink on RX/TX activity. For the configuration to
> be effective it needs to be supported by the hardware and the
> corresponding PHY driver.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> Changes in v4:
> - patch added to the series
> ---
>  .../devicetree/bindings/net/ethernet-phy.yaml | 47 +++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> index f70f18ff821f..81c5aacc89a5 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> @@ -153,6 +153,38 @@ properties:
>        Delay after the reset was deasserted in microseconds. If
>        this property is missing the delay will be skipped.
>  
> +patternProperties:
> +  "^leds$":
> +    type: object
> +    description:
> +      Subnode with configuration of the PHY LEDs.
> +
> +    patternProperties:
> +      "^led@[0-9]+$":
> +        type: object
> +        description:
> +          Subnode with the configuration of a single PHY LED.
> +
> +    properties:
> +      reg:
> +        description:
> +          The ID number of the LED, typically corresponds to a hardware ID.
> +        $ref: "/schemas/types.yaml#/definitions/uint32"
> +
> +      linux,default-trigger:
> +        description:
> +          This parameter, if present, is a string specifying the trigger
> +          assigned to the LED. Supported triggers are:
> +            "phy_link_10m_active" - LED will be on when a 10Mb/s link is active
> +            "phy_link_100m_active" - LED will be on when a 100Mb/s link is active
> +            "phy_link_1g_active" - LED will be on when a 1Gb/s link is active
> +            "phy_link_10g_active" - LED will be on when a 10Gb/s link is active
> +            "phy_activity" - LED will blink when data is received or transmitted

Matthias

We should think a bit more about these names.

I can see in future needing 1G link, but it blinks off when there is
active traffic? So phy_link_1g_active could be confusing, and very similar to
phy_link_1g_activity? So maybe 

> +            "phy_link_10m" - LED will be solid on when a 10Mb/s link is active
> +            "phy_link_100m" - LED will be solid on when a 100Mb/s link is active
> +            "phy_link_1g" - LED will be solid on when a 1Gb/s link is active

etc.

And then in the future we can have

               "phy_link_1g_activity' - LED will be on when 1Gbp/s
                                        link is active and blink off
                                        with activity.

What other use cases do we have? I don't want to support everything,
but we should be able to represent the most common modes without the
names getting too confusing.

      Andrew

Re: [PATCH v4 2/4] net: phy: Add function to retrieve LED configuration from the DT

[Matthias Kaehlcke]

On Fri, Aug 02, 2019 at 06:38:10PM +0200, Andrew Lunn wrote:
> On Thu, Aug 01, 2019 at 12:07:57PM -0700, Matthias Kaehlcke wrote:
> > Add a phylib function for retrieving PHY LED configuration that
> > is specified in the device tree using the generic binding. LEDs
> > can be configured to be 'on' for a certain link speed or to blink
> > when there is TX/RX activity.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > Changes in v4:
> > - patch added to the series
> > ---
> >  drivers/net/phy/phy_device.c | 50 ++++++++++++++++++++++++++++++++++++
> >  include/linux/phy.h          | 15 +++++++++++
> >  2 files changed, 65 insertions(+)
> > 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 6b5cb87f3866..b4b48de45712 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -2188,6 +2188,56 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
> >  	return phydrv->config_intr && phydrv->ack_interrupt;
> >  }
> >  
> > +int of_get_phy_led_cfg(struct phy_device *phydev, int led,
> > +		       struct phy_led_config *cfg)
> > +{
> > +	struct device_node *np, *child;
> > +	const char *trigger;
> > +	int ret;
> > +
> > +	if (!IS_ENABLED(CONFIG_OF_MDIO))
> > +		return -ENOENT;
> > +
> > +	np = of_find_node_by_name(phydev->mdio.dev.of_node, "leds");
> > +	if (!np)
> > +		return -ENOENT;
> > +
> > +	for_each_child_of_node(np, child) {
> > +		u32 val;
> > +
> > +		if (!of_property_read_u32(child, "reg", &val)) {
> > +			if (val == (u32)led)
> > +				break;
> > +		}
> > +	}
> 
> Hi Matthias
> 
> This is leaking references to np and child. In the past we have not
> cared about this too much, but we are now getting patches adding the
> missing releases. So it would be good to fix this.

Good point, I'll fix it in the next revision.

Thanks

Matthias

Re: [PATCH v4 4/4] net: phy: realtek: configure RTL8211E LEDs

[Andrew Lunn]

On Thu, Aug 01, 2019 at 12:07:59PM -0700, Matthias Kaehlcke wrote:
> Configure the RTL8211E LEDs behavior when the device tree property
> 'realtek,led-modes' is specified.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

Hi Matthias

I was more thinking of adding a new driver call to the PHY driver API,
to configure an LED. Something like

rtl8211e_config_leds(phydev, int led, struct phy_led_config cfg);

It would be called by the phylib core after config_init(). But also,
thinking ahead to generic linux LED support, it could be called later
to reconfigure the LEDs to use a different trigger. The standard LED
sysfs interface would be used.

      Andrew

Re: [PATCH v4 1/4] dt-bindings: net: phy: Add subnode for LED configuration

[Matthias Kaehlcke]

On Fri, Aug 02, 2019 at 06:57:55PM +0200, Andrew Lunn wrote:
> On Thu, Aug 01, 2019 at 12:07:56PM -0700, Matthias Kaehlcke wrote:
> > The LED behavior of some Ethernet PHYs is configurable. Add an
> > optional 'leds' subnode with a child node for each LED to be
> > configured. The binding aims to be compatible with the common
> > LED binding (see devicetree/bindings/leds/common.txt).
> > 
> > A LED can be configured to be 'on' when a link with a certain speed
> > is active, or to blink on RX/TX activity. For the configuration to
> > be effective it needs to be supported by the hardware and the
> > corresponding PHY driver.
> > 
> > Suggested-by: Andrew Lunn <andrew@lunn.ch>
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > Changes in v4:
> > - patch added to the series
> > ---
> >  .../devicetree/bindings/net/ethernet-phy.yaml | 47 +++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > index f70f18ff821f..81c5aacc89a5 100644
> > --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > @@ -153,6 +153,38 @@ properties:
> >        Delay after the reset was deasserted in microseconds. If
> >        this property is missing the delay will be skipped.
> >  
> > +patternProperties:
> > +  "^leds$":
> > +    type: object
> > +    description:
> > +      Subnode with configuration of the PHY LEDs.
> > +
> > +    patternProperties:
> > +      "^led@[0-9]+$":
> > +        type: object
> > +        description:
> > +          Subnode with the configuration of a single PHY LED.
> > +
> > +    properties:
> > +      reg:
> > +        description:
> > +          The ID number of the LED, typically corresponds to a hardware ID.
> > +        $ref: "/schemas/types.yaml#/definitions/uint32"
> > +
> > +      linux,default-trigger:
> > +        description:
> > +          This parameter, if present, is a string specifying the trigger
> > +          assigned to the LED. Supported triggers are:
> > +            "phy_link_10m_active" - LED will be on when a 10Mb/s link is active
> > +            "phy_link_100m_active" - LED will be on when a 100Mb/s link is active
> > +            "phy_link_1g_active" - LED will be on when a 1Gb/s link is active
> > +            "phy_link_10g_active" - LED will be on when a 10Gb/s link is active
> > +            "phy_activity" - LED will blink when data is received or transmitted
> 
> Matthias
> 
> We should think a bit more about these names.
> 
> I can see in future needing 1G link, but it blinks off when there is
> active traffic? So phy_link_1g_active could be confusing, and very similar to
> phy_link_1g_activity?

agreed, the 'active' vs' 'activity' can be confusing, let's avoid that.

> So maybe 

> > +            "phy_link_10m" - LED will be solid on when a 10Mb/s link is active
> > +            "phy_link_100m" - LED will be solid on when a 100Mb/s link is active
> > +            "phy_link_1g" - LED will be solid on when a 1Gb/s link is active
> 
> etc.
>
> And then in the future we can have
> 
>                "phy_link_1g_activity' - LED will be on when 1Gbp/s
>                                         link is active and blink off
>                                         with activity.

sounds good to me

> What other use cases do we have? I don't want to support everything,
> but we should be able to represent the most common modes without the
> names getting too confusing.

Initially I planned to support to configure a LED to be solid for
multiple link speeds, however that could become a bit messy with the
string based triggers, unless we limit the possible combinations. My
expertise in network land is limited, so I'm not sure if that's an
important/realistic use case.

Re: [PATCH v4 4/4] net: phy: realtek: configure RTL8211E LEDs

[Matthias Kaehlcke]

Hi Andrew,

On Fri, Aug 02, 2019 at 08:18:40PM +0200, Andrew Lunn wrote:
> On Thu, Aug 01, 2019 at 12:07:59PM -0700, Matthias Kaehlcke wrote:
> > Configure the RTL8211E LEDs behavior when the device tree property
> > 'realtek,led-modes' is specified.

note to self: update commit message

> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> 
> Hi Matthias
> 
> I was more thinking of adding a new driver call to the PHY driver API,
> to configure an LED. Something like
> 
> rtl8211e_config_leds(phydev, int led, struct phy_led_config cfg);

I guess it sould be singular ('_config_led') if it configures a single
LED.

> It would be called by the phylib core after config_init(). But also,
> thinking ahead to generic linux LED support, it could be called later
> to reconfigure the LEDs to use a different trigger. The standard LED
> sysfs interface would be used.

I'll look into the phylib part.

Thanks

Matthias

Re: [PATCH v4 3/4] net: phy: realtek: Add helpers for accessing RTL8211E extension pages

[Heiner Kallweit]

On 01.08.2019 21:07, Matthias Kaehlcke wrote:
> The RTL8211E has extension pages, which can be accessed after
> selecting a page through a custom method. Add a function to
> modify bits in a register of an extension page and a helper for
> selecting an ext page. Use rtl8211e_modify_ext_paged() in
> rtl8211e_config_init() instead of doing things 'manually'.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> Changes in v4:
> - don't add constant RTL8211E_EXT_PAGE, it's only used once,
>   use a literal instead
> - pass 'oldpage' to phy_restore_page() in rtl8211e_select_ext_page(),
>   not 'page'
> - return 'oldpage' in rtl8211e_select_ext_page()
> - use __phy_modify() in rtl8211e_modify_ext_paged() instead of
>   reimplementing __phy_modify_changed()
> - in rtl8211e_modify_ext_paged() return directly when
>   rtl8211e_select_ext_page() fails
> ---
>  drivers/net/phy/realtek.c | 48 +++++++++++++++++++++++++++------------
>  1 file changed, 34 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index a669945eb829..e09d3b0da2c7 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -53,6 +53,36 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
>  	return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
>  }
>  
> +static int rtl8211e_select_ext_page(struct phy_device *phydev, int page)

The "extended page" mechanism doesn't exist on RTL8211E only. A prefix
rtl821x like in other functions may be better therefore.

> +{
> +	int ret, oldpage;
> +
> +	oldpage = phy_select_page(phydev, 7);
> +	if (oldpage < 0)
> +		return oldpage;
> +
> +	ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, page);
> +	if (ret)
> +		return phy_restore_page(phydev, oldpage, ret);
> +
> +	return oldpage;
> +}
> +
> +static int rtl8211e_modify_ext_paged(struct phy_device *phydev, int page,
> +				     u32 regnum, u16 mask, u16 set)
> +{
> +	int ret = 0;
> +	int oldpage;
> +
> +	oldpage = rtl8211e_select_ext_page(phydev, page);
> +	if (oldpage < 0)
> +		return oldpage;
> +
> +	ret = __phy_modify(phydev, regnum, mask, set);
> +
> +	return phy_restore_page(phydev, oldpage, ret);
> +}
> +
>  static int rtl8201_ack_interrupt(struct phy_device *phydev)
>  {
>  	int err;
> @@ -184,7 +214,7 @@ static int rtl8211f_config_init(struct phy_device *phydev)
>  
>  static int rtl8211e_config_init(struct phy_device *phydev)
>  {
> -	int ret = 0, oldpage;
> +	int ret;
>  	u16 val;
>  
>  	/* enable TX/RX delay for rgmii-* modes, and disable them for rgmii. */
> @@ -213,19 +243,9 @@ static int rtl8211e_config_init(struct phy_device *phydev)
>  	 * 2 = RX Delay, 1 = TX Delay, 0 = SELRGV (see original PHY datasheet
>  	 * for details).
>  	 */
> -	oldpage = phy_select_page(phydev, 0x7);
> -	if (oldpage < 0)
> -		goto err_restore_page;
> -
> -	ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
> -	if (ret)
> -		goto err_restore_page;
> -
> -	ret = __phy_modify(phydev, 0x1c, RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
> -			   val);
> -
> -err_restore_page:
> -	return phy_restore_page(phydev, oldpage, ret);
> +	return rtl8211e_modify_ext_paged(phydev, 0xa4, 0x1c,
> +					 RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
> +					 val);
>  }
>  
>  static int rtl8211b_suspend(struct phy_device *phydev)
> 

Re: [PATCH v4 3/4] net: phy: realtek: Add helpers for accessing RTL8211E extension pages

[Matthias Kaehlcke]

On Sun, Aug 04, 2019 at 10:33:30AM +0200, Heiner Kallweit wrote:
> On 01.08.2019 21:07, Matthias Kaehlcke wrote:
> > The RTL8211E has extension pages, which can be accessed after
> > selecting a page through a custom method. Add a function to
> > modify bits in a register of an extension page and a helper for
> > selecting an ext page. Use rtl8211e_modify_ext_paged() in
> > rtl8211e_config_init() instead of doing things 'manually'.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > Changes in v4:
> > - don't add constant RTL8211E_EXT_PAGE, it's only used once,
> >   use a literal instead
> > - pass 'oldpage' to phy_restore_page() in rtl8211e_select_ext_page(),
> >   not 'page'
> > - return 'oldpage' in rtl8211e_select_ext_page()
> > - use __phy_modify() in rtl8211e_modify_ext_paged() instead of
> >   reimplementing __phy_modify_changed()
> > - in rtl8211e_modify_ext_paged() return directly when
> >   rtl8211e_select_ext_page() fails
> > ---
> >  drivers/net/phy/realtek.c | 48 +++++++++++++++++++++++++++------------
> >  1 file changed, 34 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index a669945eb829..e09d3b0da2c7 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -53,6 +53,36 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
> >  	return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
> >  }
> >  
> > +static int rtl8211e_select_ext_page(struct phy_device *phydev, int page)
> 
> The "extended page" mechanism doesn't exist on RTL8211E only. A prefix
> rtl821x like in other functions may be better therefore.

Sounds good, I'll change it in the next revision

Thanks

Matthias