diff mbox

[RFC,v2,2/2] NET: PHY: lantiq: add LED configuration support

Message ID 1464454741-18557-2-git-send-email-hauke@hauke-m.de
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Hauke Mehrtens May 28, 2016, 4:59 p.m. UTC 
This makes it possible to configure the behavior of the LEDs connected
to a PHY. The LEDs are controlled by the chip, this makes it possible
to configure the behavior when the hardware should activate and
deactivate the LEDs.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 Documentation/devicetree/bindings/phy/phy-leds.txt |  45 ++++++
 drivers/net/phy/lantiq.c                           | 152 +++++++++++++++++++++
 include/dt-bindings/phy/phy-leds.h                 |  27 ++++
 3 files changed, 224 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/phy-leds.txt
 create mode 100644 include/dt-bindings/phy/phy-leds.h

Comments

Andrew Lunn May 28, 2016, 6:27 p.m. UTC | #1 
On Sat, May 28, 2016 at 06:59:01PM +0200, Hauke Mehrtens wrote:
> This makes it possible to configure the behavior of the LEDs connected
> to a PHY. The LEDs are controlled by the chip, this makes it possible
> to configure the behavior when the hardware should activate and
> deactivate the LEDs.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

Hi Hauke

This is interesting. But you definitely needs to Cc: the device tree
list.

It would also be good to see basic implementations for other PHYs, so
we know the binding is generic enough.

> +LED configuration for Ethernet phys
> +
> +Property names:
> +	led-const-on: conditions the LED should be constant on
> +	led-pules: condition the LED should be pulsed on
> +	led-blink-slow: condition the LED should slowly blink
> +	led-blink-fast: condition the LED should fast blink

A binding should make it clear if properties are required or optional.

led-pulse, not led-pules.

These properties also seem to be mutually exclusive. How can it be
constantly on, yet blink? You need better descriptions.

> +property values:
> +	PHY_LED_OFF:		LED is off
> +	PHY_LED_LINK10:		link is 10MBit/s
> +	PHY_LED_LINK100:	link is 100MBit/s
> +	PHY_LED_LINK1000:	link is 1000MBit/s
> +	PHY_LED_PDOWN:		link is powered down
> +	PHY_LED_EEE:		link is in EEE mode
> +	PHY_LED_ANEG:		auto negotiation is running
> +	PHY_LED_ABIST:		analog self testing is running
> +	PHY_LED_CDIAG:		cable diagnostics is running
> +	PHY_LED_COPPER:		copper interface detected
> +	PHY_LED_FIBER:		fiber interface detected
> +	PHY_LED_TXACT:		Transmit activity
> +	PHY_LED_RXACT:		Receive activity
> +	PHY_LED_COL:		Collision

There should be a comment that not all PHYs implement all values, or
even all properties. And some PHYS will accept an ORed list of
properties, where as other will only accept a single value.

And there should be a comment to look at the binding document for the
specific PHY for what it supports. And you need such a document for
the lantiq.

Maybe some of the implementation should be pushed into the phylib.
phy_probe() already looks for the max-speed property, so it could also
parse these properties and call a function in the phy_driver
structure.

	Andrew
Hauke Mehrtens May 28, 2016, 9:23 p.m. UTC | #2 
On 05/28/2016 08:27 PM, Andrew Lunn wrote:
> On Sat, May 28, 2016 at 06:59:01PM +0200, Hauke Mehrtens wrote:
>> This makes it possible to configure the behavior of the LEDs connected
>> to a PHY. The LEDs are controlled by the chip, this makes it possible
>> to configure the behavior when the hardware should activate and
>> deactivate the LEDs.
>>
>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> 
> Hi Hauke
> 
> This is interesting. But you definitely needs to Cc: the device tree
> list.

Ah yes I forgot that, will fix the typos and resend it.

> It would also be good to see basic implementations for other PHYs, so
> we know the binding is generic enough.

Do you know for which PHY I could implement this? I only have access to
the documentation for the Lantiq / Intel PHYs.

>> +LED configuration for Ethernet phys
>> +
>> +Property names:
>> +	led-const-on: conditions the LED should be constant on
>> +	led-pules: condition the LED should be pulsed on
>> +	led-blink-slow: condition the LED should slowly blink
>> +	led-blink-fast: condition the LED should fast blink
> 
> A binding should make it clear if properties are required or optional.

OK, I will make the all optional. Currently my intention was to make
this a generic binding, but different hardware probably has different
capabilities.  Should this documentation file be specific for this PHY
driver or should I add two files for documentation one describing the
generic interface and then an additional one describing the concrete
capabilities of this PHY?

> 
> led-pulse, not led-pules.

will fix that.

> These properties also seem to be mutually exclusive. How can it be
> constantly on, yet blink? You need better descriptions.

This specific phy has an order in which the attributes are applied, I
will document that.

>> +property values:
>> +	PHY_LED_OFF:		LED is off
>> +	PHY_LED_LINK10:		link is 10MBit/s
>> +	PHY_LED_LINK100:	link is 100MBit/s
>> +	PHY_LED_LINK1000:	link is 1000MBit/s
>> +	PHY_LED_PDOWN:		link is powered down
>> +	PHY_LED_EEE:		link is in EEE mode
>> +	PHY_LED_ANEG:		auto negotiation is running
>> +	PHY_LED_ABIST:		analog self testing is running
>> +	PHY_LED_CDIAG:		cable diagnostics is running
>> +	PHY_LED_COPPER:		copper interface detected
>> +	PHY_LED_FIBER:		fiber interface detected
>> +	PHY_LED_TXACT:		Transmit activity
>> +	PHY_LED_RXACT:		Receive activity
>> +	PHY_LED_COL:		Collision
> 
> There should be a comment that not all PHYs implement all values, or
> even all properties. And some PHYS will accept an ORed list of
> properties, where as other will only accept a single value.
> 
> And there should be a comment to look at the binding document for the
> specific PHY for what it supports. And you need such a document for
> the lantiq.
> 
> Maybe some of the implementation should be pushed into the phylib.
> phy_probe() already looks for the max-speed property, so it could also
> parse these properties and call a function in the phy_driver
> structure.

I also thought about this, but as far as I know there is no standard
interface for these properties. I think setting the max speed is
standardized by the ieee. Should there be a callback with the
information needed for the PHY driver? I think having the parsing in the
generic code will not make the PHY driver much simpler, but we could
later provide these settings for different places like change the
configuration from user space.

Hauke
Andrew Lunn May 28, 2016, 10:03 p.m. UTC | #3 
> Do you know for which PHY I could implement this? I only have access to
> the documentation for the Lantiq / Intel PHYs.

Maybe actually implementing them is too far. But you could take a look
at available datasheets and thing about how you would implement the
binding. A quick search found:

http://www.davicom.com.tw/userfile/24247/DM9161-DS-F05_091008.pdf
http://www.o2xygen.com/photo/VSC8201XVZ/VSC8201XVZ_001.pdf
http://www.ti.com.cn/cn/lit/ds/symlink/dp83640.pdf
http://www.ti.com/lit/ds/symlink/dp83848-ep.pdf
http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8081MNX-RNB.pdf

Is what you have defined flexible enough?
 
> Should this documentation file be specific for this PHY driver or
> should I add two files for documentation one describing the generic
> interface and then an additional one describing the concrete
> capabilities of this PHY?

Yes, have a generic document which any PHY should be able to use, and
a specific one per PHY.

> I also thought about this, but as far as I know there is no standard
> interface for these properties. I think setting the max speed is
> standardized by the ieee. Should there be a callback with the
> information needed for the PHY driver? I think having the parsing in the
> generic code will not make the PHY driver much simpler, but we could
> later provide these settings for different places like change the
> configuration from user space.

What we don't want is X PHYs with Y different LED bindings. We should
be aiming to describe a binding which fits the majority of PHYs. And
since it should fit the majority of PHYs we can pull parts of it into
the infrastructure. That itself will prevent people doing something
different.

	Andrew
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/phy/phy-leds.txt b/Documentation/devicetree/bindings/phy/phy-leds.txt
new file mode 100644
index 0000000..6d9f5f3
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/phy-leds.txt
@@ -0,0 +1,45 @@ 
+LED configuration for Ethernet phys
+
+Property names:
+	led-const-on: conditions the LED should be constant on
+	led-pules: condition the LED should be pulsed on
+	led-blink-slow: condition the LED should slowly blink
+	led-blink-fast: condition the LED should fast blink
+
+property values:
+	PHY_LED_OFF:		LED is off
+	PHY_LED_LINK10:		link is 10MBit/s
+	PHY_LED_LINK100:	link is 100MBit/s
+	PHY_LED_LINK1000:	link is 1000MBit/s
+	PHY_LED_PDOWN:		link is powered down
+	PHY_LED_EEE:		link is in EEE mode
+	PHY_LED_ANEG:		auto negotiation is running
+	PHY_LED_ABIST:		analog self testing is running
+	PHY_LED_CDIAG:		cable diagnostics is running
+	PHY_LED_COPPER:		copper interface detected
+	PHY_LED_FIBER:		fiber interface detected
+	PHY_LED_TXACT:		Transmit activity
+	PHY_LED_RXACT:		Receive activity
+	PHY_LED_COL:		Collision
+
+Example:
+
+#include <dt-bindings/phy/phy-leds.h>
+phy@0 {
+	compatible = "ethernet-phy-ieee802.3-c22";
+	reg = <0x0>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	led@0 {
+		compatible = "phy,led";
+		reg = <0>;
+		led-const-on = <(PHY_LED_LINK10 | PHY_LED_LINK100 | PHY_LED_LINK1000)>;
+		led-pules = <(PHY_LED_TXACT | PHY_LED_RXACT)>;
+	};
+	led@2 {
+		compatible = "phy,led";
+		reg = <2>;
+		led-blink-slow = <PHY_LED_EEE>;
+		led-blink-fast = <PHY_LED_PDOWN>;
+	};
+};
diff --git a/drivers/net/phy/lantiq.c b/drivers/net/phy/lantiq.c
index a2adc4b..2d00b31 100644
--- a/drivers/net/phy/lantiq.c
+++ b/drivers/net/phy/lantiq.c
@@ -17,6 +17,7 @@ 
 #include <linux/module.h>
 #include <linux/phy.h>
 #include <linux/of.h>
+#include <dt-bindings/phy/phy-leds.h>
 
 #define LANTIQ_MDIO_IMASK		0x19	/* interrupt mask */
 #define LANTIQ_MDIO_ISTAT		0x1A	/* interrupt status */
@@ -152,11 +153,158 @@ 
 #define PHY_ID_PHY11G_VR9			0xD565A409
 #define PHY_ID_PHY22F_VR9			0xD565A419
 
+static void lantiq_gphy_config_led(struct phy_device *phydev,
+				   struct device_node *led_np)
+{
+	const __be32 *addr, *blink_fast_p, *const_on_p, *pules_p, *blink_slow_p;
+	u32 num, blink_fast, const_on, pules, blink_slow;
+	u32 ledxl;
+	u32 ledxh;
+
+	addr = of_get_property(led_np, "reg", NULL);
+	if (!addr)
+		return;
+	num = be32_to_cpu(*addr);
+
+	if (num < 0 || num > 3)
+		return;
+
+	ledxh = LANTIQ_MMD_LEDxH_BLINKF_NONE | LANTIQ_MMD_LEDxH_CON_LINK10XX;
+	blink_fast_p = of_get_property(led_np, "led-blink-fast", NULL);
+	if (blink_fast_p) {
+		ledxh &= ~LANTIQ_MMD_LEDxH_BLINKF_MASK;
+		blink_fast = be32_to_cpu(*blink_fast_p);
+		if ((blink_fast & PHY_LED_LINK10) &&
+		    (blink_fast & PHY_LED_LINK100) &&
+		    (blink_fast & PHY_LED_LINK1000)) {
+			ledxh |= LANTIQ_MMD_LEDxH_BLINKF_LINK10XX;
+		} else if ((blink_fast & PHY_LED_LINK10) &&
+			   (blink_fast & PHY_LED_LINK1000)) {
+			ledxh |= LANTIQ_MMD_LEDxH_BLINKF_LINK10_0;
+		} else if ((blink_fast & PHY_LED_LINK10) &&
+			   (blink_fast & PHY_LED_LINK100)) {
+			ledxh |= LANTIQ_MMD_LEDxH_BLINKF_LINK10X;
+		} else if ((blink_fast & PHY_LED_LINK100) &&
+			   (blink_fast & PHY_LED_LINK1000)) {
+			ledxh |= LANTIQ_MMD_LEDxH_BLINKF_LINK100X;
+		} else if (blink_fast & PHY_LED_LINK10) {
+			ledxh |= LANTIQ_MMD_LEDxH_BLINKF_LINK10;
+		} else if (blink_fast & PHY_LED_LINK100) {
+			ledxh |= LANTIQ_MMD_LEDxH_BLINKF_LINK100;
+		} else if (blink_fast & PHY_LED_LINK1000) {
+			ledxh |= LANTIQ_MMD_LEDxH_BLINKF_LINK1000;
+		} else if (blink_fast & PHY_LED_PDOWN) {
+			ledxh |= LANTIQ_MMD_LEDxH_BLINKF_PDOWN;
+		} else if (blink_fast & PHY_LED_EEE) {
+			ledxh |= LANTIQ_MMD_LEDxH_BLINKF_EEE;
+		} else if (blink_fast & PHY_LED_ANEG) {
+			ledxh |= LANTIQ_MMD_LEDxH_BLINKF_ANEG;
+		} else if (blink_fast & PHY_LED_ABIST) {
+			ledxh |= LANTIQ_MMD_LEDxH_BLINKF_ABIST;
+		} else if (blink_fast & PHY_LED_CDIAG) {
+			ledxh |= LANTIQ_MMD_LEDxH_BLINKF_CDIAG;
+		}
+	}
+	const_on_p = of_get_property(led_np, "led-const-on", NULL);
+	if (const_on_p) {
+		ledxh &= ~LANTIQ_MMD_LEDxH_CON_MASK;
+		const_on = be32_to_cpu(*const_on_p);
+		if ((const_on & PHY_LED_LINK10) &&
+		    (const_on & PHY_LED_LINK100) &&
+		    (const_on & PHY_LED_LINK1000)) {
+			ledxh |= LANTIQ_MMD_LEDxH_CON_LINK10XX;
+		} else if ((const_on & PHY_LED_LINK10) &&
+			   (const_on & PHY_LED_LINK1000)) {
+			ledxh |= LANTIQ_MMD_LEDxH_CON_LINK10_0;
+		} else if ((const_on & PHY_LED_LINK10) &&
+			   (const_on & PHY_LED_LINK100)) {
+			ledxh |= LANTIQ_MMD_LEDxH_CON_LINK10X;
+		} else if ((const_on & PHY_LED_LINK100) &&
+			   (const_on & PHY_LED_LINK1000)) {
+			ledxh |= LANTIQ_MMD_LEDxH_CON_LINK100X;
+		} else if (const_on & PHY_LED_LINK10) {
+			ledxh |= LANTIQ_MMD_LEDxH_CON_LINK10;
+		} else if (const_on & PHY_LED_LINK100) {
+			ledxh |= LANTIQ_MMD_LEDxH_CON_LINK100;
+		} else if (const_on & PHY_LED_LINK1000) {
+			ledxh |= LANTIQ_MMD_LEDxH_CON_LINK1000;
+		} else if (const_on & PHY_LED_PDOWN) {
+			ledxh |= LANTIQ_MMD_LEDxH_CON_PDOWN;
+		} else if (const_on & PHY_LED_EEE) {
+			ledxh |= LANTIQ_MMD_LEDxH_CON_EEE;
+		} else if (const_on & PHY_LED_ANEG) {
+			ledxh |= LANTIQ_MMD_LEDxH_CON_ANEG;
+		} else if (const_on & PHY_LED_ABIST) {
+			ledxh |= LANTIQ_MMD_LEDxH_CON_ABIST;
+		} else if (const_on & PHY_LED_CDIAG) {
+			ledxh |= LANTIQ_MMD_LEDxH_CON_CDIAG;
+		} else if (const_on & PHY_LED_COPPER) {
+			ledxh |= LANTIQ_MMD_LEDxH_CON_COPPER;
+		} else if (const_on & PHY_LED_FIBER) {
+			ledxh |= LANTIQ_MMD_LEDxH_CON_FIBER;
+		}
+	}
+	phy_write_mmd_indirect(phydev, LANTIQ_MMD_LED0H + (num * 2),
+			       MDIO_MMD_VEND2, ledxh);
+
+	ledxl = LANTIQ_MMD_LEDxL_PULSE_TXACT | LANTIQ_MMD_LEDxL_PULSE_RXACT |
+		LANTIQ_MMD_LEDxL_BLINKS_NONE;
+	pules_p = of_get_property(led_np, "led-pules", NULL);
+	if (pules_p) {
+		ledxl &= ~LANTIQ_MMD_LEDxL_PULSE_MASK;
+		pules = be32_to_cpu(*pules_p);
+		if (pules & PHY_LED_TXACT)
+			ledxl |= LANTIQ_MMD_LEDxL_PULSE_TXACT;
+		if (pules & PHY_LED_RXACT)
+			ledxl |= LANTIQ_MMD_LEDxL_PULSE_RXACT;
+		if (pules & PHY_LED_COL)
+			ledxl |= LANTIQ_MMD_LEDxL_PULSE_COL;
+	}
+	blink_slow_p = of_get_property(led_np, "led-blink-slow", NULL);
+	if (blink_slow_p) {
+		ledxl &= ~LANTIQ_MMD_LEDxL_BLINKS_MASK;
+		blink_slow = be32_to_cpu(*blink_slow_p);
+		if ((blink_slow & PHY_LED_LINK10) &&
+		    (blink_slow & PHY_LED_LINK100) &&
+		    (blink_slow & PHY_LED_LINK1000)) {
+			ledxl |= LANTIQ_MMD_LEDxL_BLINKS_LINK10XX;
+		} else if ((blink_slow & PHY_LED_LINK10) &&
+			   (blink_slow & PHY_LED_LINK1000)) {
+			ledxl |= LANTIQ_MMD_LEDxL_BLINKS_LINK10_0;
+		} else if ((blink_slow & PHY_LED_LINK10) &&
+			   (blink_slow & PHY_LED_LINK100)) {
+			ledxl |= LANTIQ_MMD_LEDxL_BLINKS_LINK10X;
+		} else if ((blink_slow & PHY_LED_LINK100) &&
+			   (blink_slow & PHY_LED_LINK1000)) {
+			ledxl |= LANTIQ_MMD_LEDxL_BLINKS_LINK100X;
+		} else if (blink_slow & PHY_LED_LINK10) {
+			ledxl |= LANTIQ_MMD_LEDxL_BLINKS_LINK10;
+		} else if (blink_slow & PHY_LED_LINK100) {
+			ledxl |= LANTIQ_MMD_LEDxL_BLINKS_LINK100;
+		} else if (blink_slow & PHY_LED_LINK1000) {
+			ledxl |= LANTIQ_MMD_LEDxL_BLINKS_LINK1000;
+		} else if (blink_slow & PHY_LED_PDOWN) {
+			ledxl |= LANTIQ_MMD_LEDxL_BLINKS_PDOWN;
+		} else if (blink_slow & PHY_LED_EEE) {
+			ledxl |= LANTIQ_MMD_LEDxL_BLINKS_EEE;
+		} else if (blink_slow & PHY_LED_ANEG) {
+			ledxl |= LANTIQ_MMD_LEDxL_BLINKS_ANEG;
+		} else if (blink_slow & PHY_LED_ABIST) {
+			ledxl |= LANTIQ_MMD_LEDxL_BLINKS_ABIST;
+		} else if (blink_slow & PHY_LED_CDIAG) {
+			ledxl |= LANTIQ_MMD_LEDxL_BLINKS_CDIAG;
+		}
+	}
+	phy_write_mmd_indirect(phydev, LANTIQ_MMD_LED0L + (num * 2),
+			       MDIO_MMD_VEND2, ledxl);
+}
+
 static int lantiq_gphy_config_init(struct phy_device *phydev)
 {
 	int err;
 	u32 ledxh;
 	u32 ledxl;
+	struct device_node *led_np;
 
 	/* Mask all interrupts */
 	err = phy_write(phydev, LANTIQ_MDIO_IMASK, 0);
@@ -190,6 +338,10 @@  static int lantiq_gphy_config_init(struct phy_device *phydev)
 	phy_write_mmd_indirect(phydev, LANTIQ_MMD_LED2H, MDIO_MMD_VEND2, ledxh);
 	phy_write_mmd_indirect(phydev, LANTIQ_MMD_LED2L, MDIO_MMD_VEND2, ledxl);
 
+	for_each_child_of_node(phydev->mdio.dev.of_node, led_np)
+		if (of_device_is_compatible(led_np, "phy,led"))
+			lantiq_gphy_config_led(phydev, led_np);
+
 	return 0;
 }
 
diff --git a/include/dt-bindings/phy/phy-leds.h b/include/dt-bindings/phy/phy-leds.h
new file mode 100644
index 0000000..6e43245
--- /dev/null
+++ b/include/dt-bindings/phy/phy-leds.h
@@ -0,0 +1,27 @@ 
+/*
+ * Copyright (C) 2016 Hauke Mehrtens <hauke@hauke-m.de>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ */
+
+#ifndef _DT_BINDINGS_PHY_LEDS
+#define _DT_BINDINGS_PHY_LEDS
+
+#define PHY_LED_OFF		(1 << 0)	/* is off */
+#define PHY_LED_LINK10		(1 << 1)	/* link is 10MBit/s */
+#define PHY_LED_LINK100		(1 << 2)	/* link is 100MBit/s */
+#define PHY_LED_LINK1000	(1 << 3)	/* link is 1000MBit/s */
+#define PHY_LED_PDOWN		(1 << 4)	/* link is powered down */
+#define PHY_LED_EEE		(1 << 5)	/* link is in EEE mode */
+#define PHY_LED_ANEG		(1 << 6)	/* auto negotiation is running */
+#define PHY_LED_ABIST		(1 << 7)	/* analog self testing is running */
+#define PHY_LED_CDIAG		(1 << 8)	/* cable diagnostics is running */
+#define PHY_LED_COPPER		(1 << 9)	/* copper interface detected */
+#define PHY_LED_FIBER		(1 << 10)	/* fiber interface detected */
+#define PHY_LED_TXACT		(1 << 11)	/* Transmit activity */
+#define PHY_LED_RXACT		(1 << 12)	/* Receive activity */
+#define PHY_LED_COL		(1 << 13)	/* Collision */
+
+#endif /* _DT_BINDINGS_PHY_LEDS */