[PATCH 1/3] net: phy: mscc: Add possibilty to disable combined LED mode

[PATCH 1/3] net: phy: mscc: Add possibilty to disable combined LED mode

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

By default the LED modes offer to combine two indicators like speed/link
and activity in one LED. In order to use a LED only for the first of the
two modes, the combined feature needs to be disabled.

In order to do this we introduce a boolean devicetree property
'vsc8531,led-[N]-combine-disable' and wire it up to the matching
bits in the LED behavior register.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/net/phy/mscc/mscc.h      |  5 ++++
 drivers/net/phy/mscc/mscc_main.c | 47 ++++++++++++++++++++++++++++----
 2 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index a50235fdf7d9..114b087fc89a 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h
@@ -85,6 +85,10 @@ enum rgmii_clock_delay {
 #define LED_MODE_SEL_MASK(x)		  (GENMASK(3, 0) << LED_MODE_SEL_POS(x))
 #define LED_MODE_SEL(x, mode)		  (((mode) << LED_MODE_SEL_POS(x)) & LED_MODE_SEL_MASK(x))
 
+#define MSCC_PHY_LED_BEHAVIOR		  30
+#define LED_COMBINE_DIS_MASK(x)		  (1 << (x))
+#define LED_COMBINE_DIS(x, dis)		  (((dis) ? 1 : 0) << (x))
+
 #define MSCC_EXT_PAGE_CSR_CNTL_17	  17
 #define MSCC_EXT_PAGE_CSR_CNTL_18	  18
 
@@ -363,6 +367,7 @@ struct vsc8531_private {
 	int rate_magic;
 	u16 supp_led_modes;
 	u32 leds_mode[MAX_LEDS];
+	bool leds_combine[MAX_LEDS];
 	u8 nleds;
 	const struct vsc85xx_hw_stat *hw_stats;
 	u64 *stats;
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 6e32da28e138..d42723e04c98 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -171,7 +171,8 @@ static void vsc85xx_get_stats(struct phy_device *phydev,
 
 static int vsc85xx_led_cntl_set(struct phy_device *phydev,
 				u8 led_num,
-				u8 mode)
+				u8 mode,
+				bool combine_disable)
 {
 	int rc;
 	u16 reg_val;
@@ -181,6 +182,10 @@ static int vsc85xx_led_cntl_set(struct phy_device *phydev,
 	reg_val &= ~LED_MODE_SEL_MASK(led_num);
 	reg_val |= LED_MODE_SEL(led_num, (u16)mode);
 	rc = phy_write(phydev, MSCC_PHY_LED_MODE_SEL, reg_val);
+	reg_val = phy_read(phydev, MSCC_PHY_LED_BEHAVIOR);
+	reg_val &= ~LED_COMBINE_DIS_MASK(led_num);
+	reg_val |= LED_COMBINE_DIS(led_num, combine_disable);
+	rc = phy_write(phydev, MSCC_PHY_LED_BEHAVIOR, reg_val);
 	mutex_unlock(&phydev->lock);
 
 	return rc;
@@ -432,6 +437,21 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev,
 	return led_mode;
 }
 
+static bool vsc85xx_dt_led_combine_get(struct phy_device *phydev,
+				       char *led)
+{
+	struct vsc8531_private *priv = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+	struct device_node *of_node = dev->of_node;
+	bool led_combine = false;
+	int err;
+
+	if (!of_node)
+		return false;
+
+	return of_property_read_bool(of_node, led);
+}
+
 #else
 static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
 {
@@ -444,13 +464,19 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev,
 {
 	return default_mode;
 }
+
+static bool vsc85xx_dt_led_combine_get(struct phy_device *phydev,
+				       char *led)
+{
+	return false;
+}
 #endif /* CONFIG_OF_MDIO */
 
 static int vsc85xx_dt_led_modes_get(struct phy_device *phydev,
 				    u32 *default_mode)
 {
 	struct vsc8531_private *priv = phydev->priv;
-	char led_dt_prop[28];
+	char led_dt_prop[32];
 	int i, ret;
 
 	for (i = 0; i < priv->nleds; i++) {
@@ -463,6 +489,14 @@ static int vsc85xx_dt_led_modes_get(struct phy_device *phydev,
 		if (ret < 0)
 			return ret;
 		priv->leds_mode[i] = ret;
+
+		ret = sprintf(led_dt_prop,
+			      "vsc8531,led-%d-combine-disable", i);
+		if (ret < 0)
+			return ret;
+
+		priv->leds_combine[i] =
+			vsc85xx_dt_led_combine_get(phydev, led_dt_prop);
 	}
 
 	return 0;
@@ -1779,7 +1813,8 @@ static int vsc8584_config_init(struct phy_device *phydev)
 		return ret;
 
 	for (i = 0; i < vsc8531->nleds; i++) {
-		ret = vsc85xx_led_cntl_set(phydev, i, vsc8531->leds_mode[i]);
+		ret = vsc85xx_led_cntl_set(phydev, i, vsc8531->leds_mode[i],
+					   vsc8531->leds_combine[i]);
 		if (ret)
 			return ret;
 	}
@@ -1846,7 +1881,8 @@ static int vsc85xx_config_init(struct phy_device *phydev)
 		return rc;
 
 	for (i = 0; i < vsc8531->nleds; i++) {
-		rc = vsc85xx_led_cntl_set(phydev, i, vsc8531->leds_mode[i]);
+		rc = vsc85xx_led_cntl_set(phydev, i, vsc8531->leds_mode[i],
+					  vsc8531->leds_combine[i]);
 		if (rc)
 			return rc;
 	}
@@ -2099,7 +2135,8 @@ static int vsc8514_config_init(struct phy_device *phydev)
 		return ret;
 
 	for (i = 0; i < vsc8531->nleds; i++) {
-		ret = vsc85xx_led_cntl_set(phydev, i, vsc8531->leds_mode[i]);
+		ret = vsc85xx_led_cntl_set(phydev, i, vsc8531->leds_mode[i],
+					   vsc8531->leds_combine[i]);
 		if (ret)
 			return ret;
 	}
-- 
2.33.0

[PATCH 2/3] dt-bindings: net: phy: mscc: vsc8531: Add LED mode combine disable property

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

Add the new property to disable the combined LED modes to the bindings
documentation.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
index 0a3647fe331b..1ca11ab4011b 100644
--- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
+++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
@@ -31,10 +31,13 @@ Optional properties:
 			  VSC8531_LINK_100_ACTIVITY (2),
 			  VSC8531_LINK_ACTIVITY (0) and
 			  VSC8531_DUPLEX_COLLISION (8).
+- vsc8531,led-[N]-combine-disable	: Disable the combined mode for LED[N].
+			  This disables the second mode if a combined mode is selected.
 - load-save-gpios	: GPIO used for the load/save operation of the PTP
 			  hardware clock (PHC).
 
 
+
 Table: 1 - Edge rate change
 ----------------------------------------------------------------|
 | 		Edge Rate Change (VDDMAC)			|
-- 
2.33.0

[PATCH 3/3] arm64: dts: imx8mm-kontron: Add ethernet PHY LED modes

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

This sets the LED modes for the gigabit ethernet PHY to match the ones
of the 100M USB ethernet.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 arch/arm64/boot/dts/freescale/imx8mm-kontron-n801x-s.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-n801x-s.dts b/arch/arm64/boot/dts/freescale/imx8mm-kontron-n801x-s.dts
index d17abb515835..9650bc378ba9 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-n801x-s.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-n801x-s.dts
@@ -6,6 +6,7 @@
 /dts-v1/;
 
 #include "imx8mm-kontron-n801x-som.dtsi"
+#include <dt-bindings/net/mscc-phy-vsc8531.h>
 
 / {
 	model = "Kontron i.MX8MM N801X S";
@@ -120,10 +121,14 @@ mdio {
 		#size-cells = <0>;
 
 		ethphy: ethernet-phy@0 {
+			compatible = "ethernet-phy-id0007.0570";
 			reg = <0>;
 			reset-assert-us = <100>;
 			reset-deassert-us = <100>;
 			reset-gpios = <&gpio4 27 GPIO_ACTIVE_LOW>;
+			vsc8531,led-0-mode = <VSC8531_LINK_100_1000_ACTIVITY>;
+			vsc8531,led-1-mode = <VSC8531_LINK_ACTIVITY>;
+			vsc8531,led-0-combine-disable;
 		};
 	};
 };
-- 
2.33.0

Re: [PATCH 2/3] dt-bindings: net: phy: mscc: vsc8531: Add LED mode combine disable property

[Frieder Schrempf]

On 30.09.21 14:57, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> Add the new property to disable the combined LED modes to the bindings
> documentation.
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
>  Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> index 0a3647fe331b..1ca11ab4011b 100644
> --- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> +++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> @@ -31,10 +31,13 @@ Optional properties:
>  			  VSC8531_LINK_100_ACTIVITY (2),
>  			  VSC8531_LINK_ACTIVITY (0) and
>  			  VSC8531_DUPLEX_COLLISION (8).
> +- vsc8531,led-[N]-combine-disable	: Disable the combined mode for LED[N].
> +			  This disables the second mode if a combined mode is selected.
>  - load-save-gpios	: GPIO used for the load/save operation of the PTP
>  			  hardware clock (PHC).
>  
>  
> +

Sorry, this extra newline shouldn't be here of course.

>  Table: 1 - Edge rate change
>  ----------------------------------------------------------------|
>  | 		Edge Rate Change (VDDMAC)			|
> 

Re: [PATCH 1/3] net: phy: mscc: Add possibilty to disable combined LED mode

[Andrew Lunn]

On Thu, Sep 30, 2021 at 02:57:43PM +0200, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> By default the LED modes offer to combine two indicators like speed/link
> and activity in one LED. In order to use a LED only for the first of the
> two modes, the combined feature needs to be disabled.
> 
> In order to do this we introduce a boolean devicetree property
> 'vsc8531,led-[N]-combine-disable' and wire it up to the matching
> bits in the LED behavior register.

Sorry, but no DT property. Each PHY has its own magic combination of
DT properties, nothing shared, nothing common. This does not scale.

Please look at the work being done to control PHY LEDs using the Linux
LED infrastructure. That should give us one uniform interface for all
PHY LEDs.

    Andrew

Re: [PATCH 1/3] net: phy: mscc: Add possibilty to disable combined LED mode

[Frieder Schrempf]

On 01.10.21 02:05, Andrew Lunn wrote:
> On Thu, Sep 30, 2021 at 02:57:43PM +0200, Frieder Schrempf wrote:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> By default the LED modes offer to combine two indicators like speed/link
>> and activity in one LED. In order to use a LED only for the first of the
>> two modes, the combined feature needs to be disabled.
>>
>> In order to do this we introduce a boolean devicetree property
>> 'vsc8531,led-[N]-combine-disable' and wire it up to the matching
>> bits in the LED behavior register.
> 
> Sorry, but no DT property. Each PHY has its own magic combination of
> DT properties, nothing shared, nothing common. This does not scale.
> 
> Please look at the work being done to control PHY LEDs using the Linux
> LED infrastructure. That should give us one uniform interface for all
> PHY LEDs.

+Cc: Marek

I guess you are referring to this: [1]?

If so, the last version I could find is a year old now. Is anyone still
working on this?

I understand, that the generic approach is the one we want to have, but
does this really mean adding PHY led configuration via DT to existing
drivers (that already use DT properties for LED modes) is not accepted
anymore, even if the new API is not yet in place?

If anyone (Marek?) would be willing to revive this series, I could maybe
try to find some spare time to rework the mscc PHY LED configuration on
top of this and do some tests.

[1] https://patches.linaro.org/patch/255422/

Re: [PATCH 1/3] net: phy: mscc: Add possibilty to disable combined LED mode

[Marek Behún]

On Fri, 1 Oct 2021 11:20:36 +0200
Frieder Schrempf <frieder.schrempf@kontron.de> wrote:

> On 01.10.21 02:05, Andrew Lunn wrote:
> > On Thu, Sep 30, 2021 at 02:57:43PM +0200, Frieder Schrempf wrote:  
> >> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>
> >> By default the LED modes offer to combine two indicators like speed/link
> >> and activity in one LED. In order to use a LED only for the first of the
> >> two modes, the combined feature needs to be disabled.
> >>
> >> In order to do this we introduce a boolean devicetree property
> >> 'vsc8531,led-[N]-combine-disable' and wire it up to the matching
> >> bits in the LED behavior register.  
> > 
> > Sorry, but no DT property. Each PHY has its own magic combination of
> > DT properties, nothing shared, nothing common. This does not scale.
> > 
> > Please look at the work being done to control PHY LEDs using the Linux
> > LED infrastructure. That should give us one uniform interface for all
> > PHY LEDs.  
> 
> +Cc: Marek
> 
> I guess you are referring to this: [1]?
> 
> If so, the last version I could find is a year old now. Is anyone still
> working on this?

Yes, I am still working on this.

Anyway the last version is not one year old, the last version to add
this support is 4 months old:
https://lore.kernel.org/netdev/20210602144439.4d20b295@dellmb/T/

This version does not add the code for ethernet PHYs, instead it just
tries to touch only the LED subsystem by adding the API for offloading
LED triggers and an example implementation for Turris Omnia LED
controller.

I will probably send another version this weekend. Sorry this takes
this long.


> I understand, that the generic approach is the one we want to have, but
> does this really mean adding PHY led configuration via DT to existing
> drivers (that already use DT properties for LED modes) is not accepted
> anymore, even if the new API is not yet in place?

I don't know about Rob, but I would be against it.

But if you need to have your PHY LED configured with via devicetree
ASAP, instead of proposing the vendor specific property, you can
propose LED subnodes and properties that will be generic and compatible
with the LED subsystem API, i.e. something like:

  ethernet-phy@1 {
    .... eth phy properties;

    leds {
      led@0 {
        reg = <0>;
        color = <LED_COLOR_ID_GREEN>;
        /* this LED should indicate link/speed */
        function = LED_FUNCTION_LINK; 
      };
    };
  }

Then make your PHY driver parse this, and make it so that if
function is LED_FUNCTION_LINK or LED_FUNCTION_ACTIVITY, the driver will
disable combined mode.

Afterwards, when LED subsystem has support for trigger offloading, you
can update mscc driver so that instead of just disabling combined mode,
it will register the LEDs via LED subsystem...

Marek

Re: [PATCH 1/3] net: phy: mscc: Add possibilty to disable combined LED mode

[Frieder Schrempf]

Hi Marek,

On 01.10.21 12:09, Marek Behún wrote:
> On Fri, 1 Oct 2021 11:20:36 +0200
> Frieder Schrempf <frieder.schrempf@kontron.de> wrote:
> 
>> On 01.10.21 02:05, Andrew Lunn wrote:
>>> On Thu, Sep 30, 2021 at 02:57:43PM +0200, Frieder Schrempf wrote:
>>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>>
>>>> By default the LED modes offer to combine two indicators like speed/link
>>>> and activity in one LED. In order to use a LED only for the first of the
>>>> two modes, the combined feature needs to be disabled.
>>>>
>>>> In order to do this we introduce a boolean devicetree property
>>>> 'vsc8531,led-[N]-combine-disable' and wire it up to the matching
>>>> bits in the LED behavior register.
>>>
>>> Sorry, but no DT property. Each PHY has its own magic combination of
>>> DT properties, nothing shared, nothing common. This does not scale.
>>>
>>> Please look at the work being done to control PHY LEDs using the Linux
>>> LED infrastructure. That should give us one uniform interface for all
>>> PHY LEDs.
>>
>> +Cc: Marek
>>
>> I guess you are referring to this: [1]?
>>
>> If so, the last version I could find is a year old now. Is anyone still
>> working on this?
> 
> Yes, I am still working on this.
> 
> Anyway the last version is not one year old, the last version to add
> this support is 4 months old:
> https://lore.kernel.org/netdev/20210602144439.4d20b295@dellmb/T/

Thanks for pointing out the latest patches. Good to know that you are 
still working on this.

> 
> This version does not add the code for ethernet PHYs, instead it just
> tries to touch only the LED subsystem by adding the API for offloading
> LED triggers and an example implementation for Turris Omnia LED
> controller.
> 
> I will probably send another version this weekend. Sorry this takes
> this long.

No worries, and thanks for the work!

> 
> 
>> I understand, that the generic approach is the one we want to have, but
>> does this really mean adding PHY led configuration via DT to existing
>> drivers (that already use DT properties for LED modes) is not accepted
>> anymore, even if the new API is not yet in place?
> 
> I don't know about Rob, but I would be against it.
> 
> But if you need to have your PHY LED configured with via devicetree
> ASAP, instead of proposing the vendor specific property, you can
> propose LED subnodes and properties that will be generic and compatible
> with the LED subsystem API, i.e. something like:
> 
>    ethernet-phy@1 {
>      .... eth phy properties;
> 
>      leds {
>        led@0 {
>          reg = <0>;
>          color = <LED_COLOR_ID_GREEN>;
>          /* this LED should indicate link/speed */
>          function = LED_FUNCTION_LINK;
>        };
>      };
>    }
> 
> Then make your PHY driver parse this, and make it so that if
> function is LED_FUNCTION_LINK or LED_FUNCTION_ACTIVITY, the driver will
> disable combined mode.
> 
> Afterwards, when LED subsystem has support for trigger offloading, you
> can update mscc driver so that instead of just disabling combined mode,
> it will register the LEDs via LED subsystem...

Good idea, but I'm not really in a hurry. Now knowing that work on the 
trigger offloading is still active, I guess I will just wait a bit until 
the dust has settled and maybe the bindings have been defined. Then I 
can try to implement this in the PHY driver.

Thanks
Frieder