[PATCH 0/4] lan78xx: Read configuration from Device Tree

[PATCH 0/4] lan78xx: Read configuration from Device Tree

[Phil Elwell]

The Microchip LAN78XX family of devices are Ethernet controllers with
a USB interface. Despite being discoverable devices it can be useful to
be able to configure them from Device Tree, particularly in low-cost
applications without an EEPROM or programmed OTP.

This patch set adds support for reading the MAC address, EEE setting
and LED modes from Device Tree.

Phil Elwell (4):
  lan78xx: Read MAC address from DT if present
  lan78xx: Read initial EEE setting from Device Tree
  lan78xx: Read LED modes from Device Tree
  dt-bindings: Document the DT bindings for lan78xx

 .../devicetree/bindings/net/microchip,lan78xx.txt  | 44 ++++++++++++
 MAINTAINERS                                        |  1 +
 drivers/net/usb/lan78xx.c                          | 81 ++++++++++++++++------
 3 files changed, 105 insertions(+), 21 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/microchip,lan78xx.txt

-- 
2.7.4

[PATCH 1/4] lan78xx: Read MAC address from DT if present

[Phil Elwell]

There is a standard mechanism for locating and using a MAC address from
the Device Tree. Use this facility in the lan78xx driver to support
applications without programmed EEPROM or OTP. At the same time,
regularise the handling of the different address sources.

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
 drivers/net/usb/lan78xx.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 55a78eb..d2727b5 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -37,6 +37,7 @@
 #include <linux/irqchip/chained_irq.h>
 #include <linux/microchipphy.h>
 #include <linux/phy.h>
+#include <linux/of_net.h>
 #include "lan78xx.h"
 
 #define DRIVER_AUTHOR	"WOOJUNG HUH <woojung.huh@microchip.com>"
@@ -1651,34 +1652,35 @@ static void lan78xx_init_mac_address(struct lan78xx_net *dev)
 	addr[5] = (addr_hi >> 8) & 0xFF;
 
 	if (!is_valid_ether_addr(addr)) {
-		/* reading mac address from EEPROM or OTP */
-		if ((lan78xx_read_eeprom(dev, EEPROM_MAC_OFFSET, ETH_ALEN,
-					 addr) == 0) ||
-		    (lan78xx_read_otp(dev, EEPROM_MAC_OFFSET, ETH_ALEN,
-				      addr) == 0)) {
-			if (is_valid_ether_addr(addr)) {
-				/* eeprom values are valid so use them */
-				netif_dbg(dev, ifup, dev->net,
-					  "MAC address read from EEPROM");
-			} else {
-				/* generate random MAC */
-				random_ether_addr(addr);
-				netif_dbg(dev, ifup, dev->net,
-					  "MAC address set to random addr");
-			}
+		const u8 *mac_addr;
 
-			addr_lo = addr[0] | (addr[1] << 8) |
-				  (addr[2] << 16) | (addr[3] << 24);
-			addr_hi = addr[4] | (addr[5] << 8);
-
-			ret = lan78xx_write_reg(dev, RX_ADDRL, addr_lo);
-			ret = lan78xx_write_reg(dev, RX_ADDRH, addr_hi);
+		mac_addr = of_get_mac_address(dev->udev->dev.of_node);
+		if (mac_addr) {
+			/* valid address present in Device Tree */
+			ether_addr_copy(addr, mac_addr);
+			netif_dbg(dev, ifup, dev->net,
+				  "MAC address read from Device Tree");
+		} else if (((lan78xx_read_eeprom(dev, EEPROM_MAC_OFFSET,
+						 ETH_ALEN, addr) == 0) ||
+			    (lan78xx_read_otp(dev, EEPROM_MAC_OFFSET,
+					      ETH_ALEN, addr) == 0)) &&
+			   is_valid_ether_addr(addr)) {
+			/* eeprom values are valid so use them */
+			netif_dbg(dev, ifup, dev->net,
+				  "MAC address read from EEPROM");
 		} else {
 			/* generate random MAC */
 			random_ether_addr(addr);
 			netif_dbg(dev, ifup, dev->net,
 				  "MAC address set to random addr");
 		}
+
+		addr_lo = addr[0] | (addr[1] << 8) |
+			  (addr[2] << 16) | (addr[3] << 24);
+		addr_hi = addr[4] | (addr[5] << 8);
+
+		ret = lan78xx_write_reg(dev, RX_ADDRL, addr_lo);
+		ret = lan78xx_write_reg(dev, RX_ADDRH, addr_hi);
 	}
 
 	ret = lan78xx_write_reg(dev, MAF_LO(0), addr_lo);
-- 
2.7.4

[1/4] lan78xx: Read MAC address from DT if present

[Phil Elwell]

There is a standard mechanism for locating and using a MAC address from
the Device Tree. Use this facility in the lan78xx driver to support
applications without programmed EEPROM or OTP. At the same time,
regularise the handling of the different address sources.

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
 drivers/net/usb/lan78xx.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 55a78eb..d2727b5 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -37,6 +37,7 @@
 #include <linux/irqchip/chained_irq.h>
 #include <linux/microchipphy.h>
 #include <linux/phy.h>
+#include <linux/of_net.h>
 #include "lan78xx.h"
 
 #define DRIVER_AUTHOR	"WOOJUNG HUH <woojung.huh@microchip.com>"
@@ -1651,34 +1652,35 @@ static void lan78xx_init_mac_address(struct lan78xx_net *dev)
 	addr[5] = (addr_hi >> 8) & 0xFF;
 
 	if (!is_valid_ether_addr(addr)) {
-		/* reading mac address from EEPROM or OTP */
-		if ((lan78xx_read_eeprom(dev, EEPROM_MAC_OFFSET, ETH_ALEN,
-					 addr) == 0) ||
-		    (lan78xx_read_otp(dev, EEPROM_MAC_OFFSET, ETH_ALEN,
-				      addr) == 0)) {
-			if (is_valid_ether_addr(addr)) {
-				/* eeprom values are valid so use them */
-				netif_dbg(dev, ifup, dev->net,
-					  "MAC address read from EEPROM");
-			} else {
-				/* generate random MAC */
-				random_ether_addr(addr);
-				netif_dbg(dev, ifup, dev->net,
-					  "MAC address set to random addr");
-			}
+		const u8 *mac_addr;
 
-			addr_lo = addr[0] | (addr[1] << 8) |
-				  (addr[2] << 16) | (addr[3] << 24);
-			addr_hi = addr[4] | (addr[5] << 8);
-
-			ret = lan78xx_write_reg(dev, RX_ADDRL, addr_lo);
-			ret = lan78xx_write_reg(dev, RX_ADDRH, addr_hi);
+		mac_addr = of_get_mac_address(dev->udev->dev.of_node);
+		if (mac_addr) {
+			/* valid address present in Device Tree */
+			ether_addr_copy(addr, mac_addr);
+			netif_dbg(dev, ifup, dev->net,
+				  "MAC address read from Device Tree");
+		} else if (((lan78xx_read_eeprom(dev, EEPROM_MAC_OFFSET,
+						 ETH_ALEN, addr) == 0) ||
+			    (lan78xx_read_otp(dev, EEPROM_MAC_OFFSET,
+					      ETH_ALEN, addr) == 0)) &&
+			   is_valid_ether_addr(addr)) {
+			/* eeprom values are valid so use them */
+			netif_dbg(dev, ifup, dev->net,
+				  "MAC address read from EEPROM");
 		} else {
 			/* generate random MAC */
 			random_ether_addr(addr);
 			netif_dbg(dev, ifup, dev->net,
 				  "MAC address set to random addr");
 		}
+
+		addr_lo = addr[0] | (addr[1] << 8) |
+			  (addr[2] << 16) | (addr[3] << 24);
+		addr_hi = addr[4] | (addr[5] << 8);
+
+		ret = lan78xx_write_reg(dev, RX_ADDRL, addr_lo);
+		ret = lan78xx_write_reg(dev, RX_ADDRH, addr_hi);
 	}
 
 	ret = lan78xx_write_reg(dev, MAF_LO(0), addr_lo);

[PATCH 2/4] lan78xx: Read initial EEE setting from Device Tree

[Phil Elwell]

Add two new Device Tree properties:
* microchip,eee-enabled  - a boolean to enable EEE
* microchip,tx-lpi-timer - time in microseconds to wait after TX goes
                           idle before entering the low power state
                           (default 600)

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
 drivers/net/usb/lan78xx.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index d2727b5..d98397b 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2080,6 +2080,23 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 	mii_adv = (u32)mii_advertise_flowctrl(dev->fc_request_control);
 	phydev->advertising |= mii_adv_to_ethtool_adv_t(mii_adv);
 
+	if (of_property_read_bool(dev->udev->dev.of_node,
+				  "microchip,eee-enabled")) {
+		struct ethtool_eee edata;
+
+		memset(&edata, 0, sizeof(edata));
+		edata.cmd = ETHTOOL_SEEE;
+		edata.advertised = ADVERTISED_1000baseT_Full |
+				   ADVERTISED_100baseT_Full;
+		edata.eee_enabled = true;
+		edata.tx_lpi_enabled = true;
+		if (of_property_read_u32(dev->udev->dev.of_node,
+					 "microchip,tx-lpi-timer",
+					 &edata.tx_lpi_timer))
+			edata.tx_lpi_timer = 600; /* non-aggressive */
+		(void)lan78xx_set_eee(dev->net, &edata);
+	}
+
 	genphy_config_aneg(phydev);
 
 	dev->fc_autoneg = phydev->autoneg;
-- 
2.7.4

[2/4] lan78xx: Read initial EEE setting from Device Tree

[Phil Elwell]

Add two new Device Tree properties:
* microchip,eee-enabled  - a boolean to enable EEE
* microchip,tx-lpi-timer - time in microseconds to wait after TX goes
                           idle before entering the low power state
                           (default 600)

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
 drivers/net/usb/lan78xx.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index d2727b5..d98397b 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2080,6 +2080,23 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 	mii_adv = (u32)mii_advertise_flowctrl(dev->fc_request_control);
 	phydev->advertising |= mii_adv_to_ethtool_adv_t(mii_adv);
 
+	if (of_property_read_bool(dev->udev->dev.of_node,
+				  "microchip,eee-enabled")) {
+		struct ethtool_eee edata;
+
+		memset(&edata, 0, sizeof(edata));
+		edata.cmd = ETHTOOL_SEEE;
+		edata.advertised = ADVERTISED_1000baseT_Full |
+				   ADVERTISED_100baseT_Full;
+		edata.eee_enabled = true;
+		edata.tx_lpi_enabled = true;
+		if (of_property_read_u32(dev->udev->dev.of_node,
+					 "microchip,tx-lpi-timer",
+					 &edata.tx_lpi_timer))
+			edata.tx_lpi_timer = 600; /* non-aggressive */
+		(void)lan78xx_set_eee(dev->net, &edata);
+	}
+
 	genphy_config_aneg(phydev);
 
 	dev->fc_autoneg = phydev->autoneg;

[PATCH 3/4] lan78xx: Read LED modes from Device Tree

[Phil Elwell]

Add support for DT property "microchip,led-modes", a vector of two
cells (u32s) in the range 0-15, each of which sets the mode for one
of the two LEDs. Some possible values are:

    0=link/activity          1=link1000/activity
    2=link100/activity       3=link10/activity
    4=link100/1000/activity  5=link10/1000/activity
    6=link10/100/activity    14=off    15=on

Also use the presence of the DT property to indicate that the
LEDs should be enabled - necessary in the event that no valid OTP
or EEPROM is available.

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
 drivers/net/usb/lan78xx.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index d98397b..ffb483d 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2008,6 +2008,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 {
 	int ret;
 	u32 mii_adv;
+	u32 led_modes[2];
 	struct phy_device *phydev;
 
 	phydev = phy_find_first(dev->mdiobus);
@@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 		(void)lan78xx_set_eee(dev->net, &edata);
 	}
 
+	if (!of_property_read_u32_array(dev->udev->dev.of_node,
+					"microchip,led-modes",
+					led_modes, ARRAY_SIZE(led_modes))) {
+		u32 reg;
+		int i;
+
+		reg = phy_read(phydev, 0x1d);
+		for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
+			reg &= ~(0xf << (i * 4));
+			reg |= (led_modes[i] & 0xf) << (i * 4);
+		}
+		(void)phy_write(phydev, 0x1d, reg);
+
+		/* Ensure the LEDs are enabled */
+		lan78xx_read_reg(dev, HW_CFG, &reg);
+		reg |= HW_CFG_LED0_EN_ | HW_CFG_LED1_EN_;
+		lan78xx_write_reg(dev, HW_CFG, reg);
+	}
+
 	genphy_config_aneg(phydev);
 
 	dev->fc_autoneg = phydev->autoneg;
-- 
2.7.4

[3/4] lan78xx: Read LED modes from Device Tree

[Phil Elwell]

Add support for DT property "microchip,led-modes", a vector of two
cells (u32s) in the range 0-15, each of which sets the mode for one
of the two LEDs. Some possible values are:

    0=link/activity          1=link1000/activity
    2=link100/activity       3=link10/activity
    4=link100/1000/activity  5=link10/1000/activity
    6=link10/100/activity    14=off    15=on

Also use the presence of the DT property to indicate that the
LEDs should be enabled - necessary in the event that no valid OTP
or EEPROM is available.

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
 drivers/net/usb/lan78xx.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index d98397b..ffb483d 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2008,6 +2008,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 {
 	int ret;
 	u32 mii_adv;
+	u32 led_modes[2];
 	struct phy_device *phydev;
 
 	phydev = phy_find_first(dev->mdiobus);
@@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 		(void)lan78xx_set_eee(dev->net, &edata);
 	}
 
+	if (!of_property_read_u32_array(dev->udev->dev.of_node,
+					"microchip,led-modes",
+					led_modes, ARRAY_SIZE(led_modes))) {
+		u32 reg;
+		int i;
+
+		reg = phy_read(phydev, 0x1d);
+		for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
+			reg &= ~(0xf << (i * 4));
+			reg |= (led_modes[i] & 0xf) << (i * 4);
+		}
+		(void)phy_write(phydev, 0x1d, reg);
+
+		/* Ensure the LEDs are enabled */
+		lan78xx_read_reg(dev, HW_CFG, &reg);
+		reg |= HW_CFG_LED0_EN_ | HW_CFG_LED1_EN_;
+		lan78xx_write_reg(dev, HW_CFG, reg);
+	}
+
 	genphy_config_aneg(phydev);
 
 	dev->fc_autoneg = phydev->autoneg;

[PATCH 4/4] dt-bindings: Document the DT bindings for lan78xx

[Phil Elwell]

The Microchip LAN78XX family of devices are Ethernet controllers with
a USB interface. Despite being discoverable devices it can be useful to
be able to configure them from Device Tree, particularly in low-cost
applications without an EEPROM or programmed OTP.

Document the supported properties in a bindings file, adding it to
MAINTAINERS at the same time.

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
 .../devicetree/bindings/net/microchip,lan78xx.txt  | 44 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/microchip,lan78xx.txt

diff --git a/Documentation/devicetree/bindings/net/microchip,lan78xx.txt b/Documentation/devicetree/bindings/net/microchip,lan78xx.txt
new file mode 100644
index 0000000..e7d7850
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/microchip,lan78xx.txt
@@ -0,0 +1,44 @@
+Microchip LAN78xx Gigabit Ethernet controller
+
+The LAN78XX devices are usually configured by programming their OTP or with
+an external EEPROM, but some platforms (e.g. Raspberry Pi 3 B+) have neither.
+
+Please refer to ethernet.txt for a description of common Ethernet bindings.
+
+Optional properties:
+- microchip,eee-enabled: if present, enable Energy Efficient Ethernet support;
+- microchip,led-modes: a two-element vector, with each element configuring
+  the operating mode of an LED. The values supported by the device are;
+  0: Link/Activity
+  1: Link1000/Activity
+  2: Link100/Activity
+  3: Link10/Activity
+  4: Link100/1000/Activity
+  5: Link10/1000/Activity
+  6: Link10/100/Activity
+  7: RESERVED
+  8: Duplex/Collision
+  9: Collision
+  10: Activity
+  11: RESERVED
+  12: Auto-negotiation Fault
+  13: RESERVED
+  14: Off
+  15: On
+- microchip,tx-lpi-timer: the delay (in microseconds) between the TX fifo
+  becoming empty and invoking Low Power Idles (default 600).
+
+Example:
+
+	/* Standard configuration for a Raspberry Pi 3 B+ */
+	ethernet: usbether@1 {
+		compatible = "usb424,7800";
+		reg = <1>;
+		microchip,eee-enabled;
+		microchip,tx-lpi-timer = <600>;
+		/*
+		 * led0 = 1:link1000/activity
+		 * led1 = 6:link10/100/activity
+		 */
+		microchip,led-modes = <1 6>;
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index 2328eed..b637aad 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14482,6 +14482,7 @@ M:	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/usb/lan78xx.*
+F:	Documentation/devicetree/bindings/net/microchip,lan78xx.txt
 
 USB MASS STORAGE DRIVER
 M:	Alan Stern <stern@rowland.harvard.edu>
-- 
2.7.4

[4/4] dt-bindings: Document the DT bindings for lan78xx

[Phil Elwell]

The Microchip LAN78XX family of devices are Ethernet controllers with
a USB interface. Despite being discoverable devices it can be useful to
be able to configure them from Device Tree, particularly in low-cost
applications without an EEPROM or programmed OTP.

Document the supported properties in a bindings file, adding it to
MAINTAINERS at the same time.

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
 .../devicetree/bindings/net/microchip,lan78xx.txt  | 44 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/microchip,lan78xx.txt

diff --git a/Documentation/devicetree/bindings/net/microchip,lan78xx.txt b/Documentation/devicetree/bindings/net/microchip,lan78xx.txt
new file mode 100644
index 0000000..e7d7850
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/microchip,lan78xx.txt
@@ -0,0 +1,44 @@
+Microchip LAN78xx Gigabit Ethernet controller
+
+The LAN78XX devices are usually configured by programming their OTP or with
+an external EEPROM, but some platforms (e.g. Raspberry Pi 3 B+) have neither.
+
+Please refer to ethernet.txt for a description of common Ethernet bindings.
+
+Optional properties:
+- microchip,eee-enabled: if present, enable Energy Efficient Ethernet support;
+- microchip,led-modes: a two-element vector, with each element configuring
+  the operating mode of an LED. The values supported by the device are;
+  0: Link/Activity
+  1: Link1000/Activity
+  2: Link100/Activity
+  3: Link10/Activity
+  4: Link100/1000/Activity
+  5: Link10/1000/Activity
+  6: Link10/100/Activity
+  7: RESERVED
+  8: Duplex/Collision
+  9: Collision
+  10: Activity
+  11: RESERVED
+  12: Auto-negotiation Fault
+  13: RESERVED
+  14: Off
+  15: On
+- microchip,tx-lpi-timer: the delay (in microseconds) between the TX fifo
+  becoming empty and invoking Low Power Idles (default 600).
+
+Example:
+
+	/* Standard configuration for a Raspberry Pi 3 B+ */
+	ethernet: usbether@1 {
+		compatible = "usb424,7800";
+		reg = <1>;
+		microchip,eee-enabled;
+		microchip,tx-lpi-timer = <600>;
+		/*
+		 * led0 = 1:link1000/activity
+		 * led1 = 6:link10/100/activity
+		 */
+		microchip,led-modes = <1 6>;
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index 2328eed..b637aad 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14482,6 +14482,7 @@ M:	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/usb/lan78xx.*
+F:	Documentation/devicetree/bindings/net/microchip,lan78xx.txt
 
 USB MASS STORAGE DRIVER
 M:	Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH 4/4] dt-bindings: Document the DT bindings for lan78xx

[Andrew Lunn]

On Thu, Apr 12, 2018 at 02:55:36PM +0100, Phil Elwell wrote:
> The Microchip LAN78XX family of devices are Ethernet controllers with
> a USB interface. Despite being discoverable devices it can be useful to
> be able to configure them from Device Tree, particularly in low-cost
> applications without an EEPROM or programmed OTP.
> 
> Document the supported properties in a bindings file, adding it to
> MAINTAINERS at the same time.

Hi Phil

How you link an OF node to a USB device is not obvious. Could you
please include either a pointer to some binding documentation, or make
your example show it.

Thanks
	Andrew

[4/4] dt-bindings: Document the DT bindings for lan78xx

[Andrew Lunn]

On Thu, Apr 12, 2018 at 02:55:36PM +0100, Phil Elwell wrote:
> The Microchip LAN78XX family of devices are Ethernet controllers with
> a USB interface. Despite being discoverable devices it can be useful to
> be able to configure them from Device Tree, particularly in low-cost
> applications without an EEPROM or programmed OTP.
> 
> Document the supported properties in a bindings file, adding it to
> MAINTAINERS at the same time.

Hi Phil

How you link an OF node to a USB device is not obvious. Could you
please include either a pointer to some binding documentation, or make
your example show it.

Thanks
	Andrew
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] lan78xx: Read MAC address from DT if present

[Andrew Lunn]

Hi Phil

> -			ret = lan78xx_write_reg(dev, RX_ADDRL, addr_lo);
> -			ret = lan78xx_write_reg(dev, RX_ADDRH, addr_hi);
> +		mac_addr = of_get_mac_address(dev->udev->dev.of_node);

It might be better to use the higher level eth_platform_get_mac_address().

   Andrew

[1/4] lan78xx: Read MAC address from DT if present

[Andrew Lunn]

Hi Phil

> -			ret = lan78xx_write_reg(dev, RX_ADDRL, addr_lo);
> -			ret = lan78xx_write_reg(dev, RX_ADDRH, addr_hi);
> +		mac_addr = of_get_mac_address(dev->udev->dev.of_node);

It might be better to use the higher level eth_platform_get_mac_address().

   Andrew
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] dt-bindings: Document the DT bindings for lan78xx

[Phil Elwell]

Hi Andrew,

On 12/04/2018 15:04, Andrew Lunn wrote:
> On Thu, Apr 12, 2018 at 02:55:36PM +0100, Phil Elwell wrote:
>> The Microchip LAN78XX family of devices are Ethernet controllers with
>> a USB interface. Despite being discoverable devices it can be useful to
>> be able to configure them from Device Tree, particularly in low-cost
>> applications without an EEPROM or programmed OTP.
>>
>> Document the supported properties in a bindings file, adding it to
>> MAINTAINERS at the same time.
> 
> Hi Phil
> 
> How you link an OF node to a USB device is not obvious. Could you
> please include either a pointer to some binding documentation, or make
> your example show it.

Thanks for the feedback. Would you consider this (lifted from the Pi 3B+ Device Tree)
a sufficient example?

&usb {
	usb1@1 {
		compatible = "usb424,2514";
		reg = <1>;
		#address-cells = <1>;
		#size-cells = <0>;

		usb1_1@1 {
			compatible = "usb424,2514";
			reg = <1>;
			#address-cells = <1>;
			#size-cells = <0>;

			ethernet: usbether@1 {
				compatible = "usb424,7800";
				reg = <1>;
				microchip,eee-enabled;
				microchip,tx-lpi-timer = <600>; /* non-aggressive*/
				/*
				 * led0 = 1:link1000/activity
				 * led1 = 6:link10/100/activity
				 */
				microchip,led-modes = <1 6>;
			};
		};
	};
};

Phil

[4/4] dt-bindings: Document the DT bindings for lan78xx

[Phil Elwell]

Hi Andrew,

On 12/04/2018 15:04, Andrew Lunn wrote:
> On Thu, Apr 12, 2018 at 02:55:36PM +0100, Phil Elwell wrote:
>> The Microchip LAN78XX family of devices are Ethernet controllers with
>> a USB interface. Despite being discoverable devices it can be useful to
>> be able to configure them from Device Tree, particularly in low-cost
>> applications without an EEPROM or programmed OTP.
>>
>> Document the supported properties in a bindings file, adding it to
>> MAINTAINERS at the same time.
> 
> Hi Phil
> 
> How you link an OF node to a USB device is not obvious. Could you
> please include either a pointer to some binding documentation, or make
> your example show it.

Thanks for the feedback. Would you consider this (lifted from the Pi 3B+ Device Tree)
a sufficient example?

&usb {
	usb1@1 {
		compatible = "usb424,2514";
		reg = <1>;
		#address-cells = <1>;
		#size-cells = <0>;

		usb1_1@1 {
			compatible = "usb424,2514";
			reg = <1>;
			#address-cells = <1>;
			#size-cells = <0>;

			ethernet: usbether@1 {
				compatible = "usb424,7800";
				reg = <1>;
				microchip,eee-enabled;
				microchip,tx-lpi-timer = <600>; /* non-aggressive*/
				/*
				 * led0 = 1:link1000/activity
				 * led1 = 6:link10/100/activity
				 */
				microchip,led-modes = <1 6>;
			};
		};
	};
};

Phil
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] lan78xx: Read initial EEE setting from Device Tree

[Andrew Lunn]

On Thu, Apr 12, 2018 at 02:55:34PM +0100, Phil Elwell wrote:
> Add two new Device Tree properties:
> * microchip,eee-enabled  - a boolean to enable EEE
> * microchip,tx-lpi-timer - time in microseconds to wait after TX goes
>                            idle before entering the low power state
>                            (default 600)

Hi Phil

This looks wrong.

What should happen is that the MAC driver calls phy_init_eee() to find
out if the PHY supports EEE. There should be no need to look in device
tree.

	Andrew

[2/4] lan78xx: Read initial EEE setting from Device Tree

[Andrew Lunn]

On Thu, Apr 12, 2018 at 02:55:34PM +0100, Phil Elwell wrote:
> Add two new Device Tree properties:
> * microchip,eee-enabled  - a boolean to enable EEE
> * microchip,tx-lpi-timer - time in microseconds to wait after TX goes
>                            idle before entering the low power state
>                            (default 600)

Hi Phil

This looks wrong.

What should happen is that the MAC driver calls phy_init_eee() to find
out if the PHY supports EEE. There should be no need to look in device
tree.

	Andrew
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] dt-bindings: Document the DT bindings for lan78xx

[Andrew Lunn]

On Thu, Apr 12, 2018 at 03:10:57PM +0100, Phil Elwell wrote:
> Hi Andrew,
> 
> On 12/04/2018 15:04, Andrew Lunn wrote:
> > On Thu, Apr 12, 2018 at 02:55:36PM +0100, Phil Elwell wrote:
> >> The Microchip LAN78XX family of devices are Ethernet controllers with
> >> a USB interface. Despite being discoverable devices it can be useful to
> >> be able to configure them from Device Tree, particularly in low-cost
> >> applications without an EEPROM or programmed OTP.
> >>
> >> Document the supported properties in a bindings file, adding it to
> >> MAINTAINERS at the same time.
> > 
> > Hi Phil
> > 
> > How you link an OF node to a USB device is not obvious. Could you
> > please include either a pointer to some binding documentation, or make
> > your example show it.
> 
> Thanks for the feedback. Would you consider this (lifted from the Pi 3B+ Device Tree)
> a sufficient example?

Yes, this is good.

Thanks
     Andrew

[4/4] dt-bindings: Document the DT bindings for lan78xx

[Andrew Lunn]

On Thu, Apr 12, 2018 at 03:10:57PM +0100, Phil Elwell wrote:
> Hi Andrew,
> 
> On 12/04/2018 15:04, Andrew Lunn wrote:
> > On Thu, Apr 12, 2018 at 02:55:36PM +0100, Phil Elwell wrote:
> >> The Microchip LAN78XX family of devices are Ethernet controllers with
> >> a USB interface. Despite being discoverable devices it can be useful to
> >> be able to configure them from Device Tree, particularly in low-cost
> >> applications without an EEPROM or programmed OTP.
> >>
> >> Document the supported properties in a bindings file, adding it to
> >> MAINTAINERS at the same time.
> > 
> > Hi Phil
> > 
> > How you link an OF node to a USB device is not obvious. Could you
> > please include either a pointer to some binding documentation, or make
> > your example show it.
> 
> Thanks for the feedback. Would you consider this (lifted from the Pi 3B+ Device Tree)
> a sufficient example?

Yes, this is good.

Thanks
     Andrew
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] lan78xx: Read MAC address from DT if present

[Phil Elwell]

Hi Andrew,

On 12/04/2018 15:06, Andrew Lunn wrote:
> Hi Phil
> 
>> -			ret = lan78xx_write_reg(dev, RX_ADDRL, addr_lo);
>> -			ret = lan78xx_write_reg(dev, RX_ADDRH, addr_hi);
>> +		mac_addr = of_get_mac_address(dev->udev->dev.of_node);
> 
> It might be better to use the higher level eth_platform_get_mac_address().

OK - I'll take your word for it.

Phil

[1/4] lan78xx: Read MAC address from DT if present

[Phil Elwell]

Hi Andrew,

On 12/04/2018 15:06, Andrew Lunn wrote:
> Hi Phil
> 
>> -			ret = lan78xx_write_reg(dev, RX_ADDRL, addr_lo);
>> -			ret = lan78xx_write_reg(dev, RX_ADDRH, addr_hi);
>> +		mac_addr = of_get_mac_address(dev->udev->dev.of_node);
> 
> It might be better to use the higher level eth_platform_get_mac_address().

OK - I'll take your word for it.

Phil
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] lan78xx: Read LED modes from Device Tree

[Andrew Lunn]

On Thu, Apr 12, 2018 at 02:55:35PM +0100, Phil Elwell wrote:
> Add support for DT property "microchip,led-modes", a vector of two
> cells (u32s) in the range 0-15, each of which sets the mode for one
> of the two LEDs. Some possible values are:
> 
>     0=link/activity          1=link1000/activity
>     2=link100/activity       3=link10/activity
>     4=link100/1000/activity  5=link10/1000/activity
>     6=link10/100/activity    14=off    15=on
> 
> Also use the presence of the DT property to indicate that the
> LEDs should be enabled - necessary in the event that no valid OTP
> or EEPROM is available.

I'm not a fan of this, but at the moment, we don't have anything
better.

Please follow what mscc does, add a header file for the LED settings.

       Andrew

> 
> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
> ---
>  drivers/net/usb/lan78xx.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index d98397b..ffb483d 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -2008,6 +2008,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  {
>  	int ret;
>  	u32 mii_adv;
> +	u32 led_modes[2];
>  	struct phy_device *phydev;
>  
>  	phydev = phy_find_first(dev->mdiobus);
> @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  		(void)lan78xx_set_eee(dev->net, &edata);
>  	}
>  
> +	if (!of_property_read_u32_array(dev->udev->dev.of_node,
> +					"microchip,led-modes",
> +					led_modes, ARRAY_SIZE(led_modes))) {
> +		u32 reg;
> +		int i;
> +
> +		reg = phy_read(phydev, 0x1d);
> +		for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
> +			reg &= ~(0xf << (i * 4));
> +			reg |= (led_modes[i] & 0xf) << (i * 4);
> +		}

Please add range checks for led_modes[i] and return -EINVAL if the
check fails.

      Andrew

[3/4] lan78xx: Read LED modes from Device Tree

[Andrew Lunn]

On Thu, Apr 12, 2018 at 02:55:35PM +0100, Phil Elwell wrote:
> Add support for DT property "microchip,led-modes", a vector of two
> cells (u32s) in the range 0-15, each of which sets the mode for one
> of the two LEDs. Some possible values are:
> 
>     0=link/activity          1=link1000/activity
>     2=link100/activity       3=link10/activity
>     4=link100/1000/activity  5=link10/1000/activity
>     6=link10/100/activity    14=off    15=on
> 
> Also use the presence of the DT property to indicate that the
> LEDs should be enabled - necessary in the event that no valid OTP
> or EEPROM is available.

I'm not a fan of this, but at the moment, we don't have anything
better.

Please follow what mscc does, add a header file for the LED settings.

       Andrew

> 
> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
> ---
>  drivers/net/usb/lan78xx.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index d98397b..ffb483d 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -2008,6 +2008,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  {
>  	int ret;
>  	u32 mii_adv;
> +	u32 led_modes[2];
>  	struct phy_device *phydev;
>  
>  	phydev = phy_find_first(dev->mdiobus);
> @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  		(void)lan78xx_set_eee(dev->net, &edata);
>  	}
>  
> +	if (!of_property_read_u32_array(dev->udev->dev.of_node,
> +					"microchip,led-modes",
> +					led_modes, ARRAY_SIZE(led_modes))) {
> +		u32 reg;
> +		int i;
> +
> +		reg = phy_read(phydev, 0x1d);
> +		for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
> +			reg &= ~(0xf << (i * 4));
> +			reg |= (led_modes[i] & 0xf) << (i * 4);
> +		}

Please add range checks for led_modes[i] and return -EINVAL if the
check fails.

      Andrew
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] dt-bindings: Document the DT bindings for lan78xx

[Andrew Lunn]

On Thu, Apr 12, 2018 at 02:55:36PM +0100, Phil Elwell wrote:
> The Microchip LAN78XX family of devices are Ethernet controllers with
> a USB interface. Despite being discoverable devices it can be useful to
> be able to configure them from Device Tree, particularly in low-cost
> applications without an EEPROM or programmed OTP.

It would be good to document what happens when there is an EEPROM. Is
OF used in preference to the EEPROM?
   
Andrew

[4/4] dt-bindings: Document the DT bindings for lan78xx

[Andrew Lunn]

On Thu, Apr 12, 2018 at 02:55:36PM +0100, Phil Elwell wrote:
> The Microchip LAN78XX family of devices are Ethernet controllers with
> a USB interface. Despite being discoverable devices it can be useful to
> be able to configure them from Device Tree, particularly in low-cost
> applications without an EEPROM or programmed OTP.

It would be good to document what happens when there is an EEPROM. Is
OF used in preference to the EEPROM?
   
Andrew
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] lan78xx: Read LED modes from Device Tree

[Phil Elwell]

Hi Andrew,

On 12/04/2018 15:26, Andrew Lunn wrote:
> On Thu, Apr 12, 2018 at 02:55:35PM +0100, Phil Elwell wrote:
>> Add support for DT property "microchip,led-modes", a vector of two
>> cells (u32s) in the range 0-15, each of which sets the mode for one
>> of the two LEDs. Some possible values are:
>>
>>     0=link/activity          1=link1000/activity
>>     2=link100/activity       3=link10/activity
>>     4=link100/1000/activity  5=link10/1000/activity
>>     6=link10/100/activity    14=off    15=on
>>
>> Also use the presence of the DT property to indicate that the
>> LEDs should be enabled - necessary in the event that no valid OTP
>> or EEPROM is available.
> 
> I'm not a fan of this, but at the moment, we don't have anything
> better.
> 
> Please follow what mscc does, add a header file for the LED settings.

Good idea.

> 
>>
>> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
>> ---
>>  drivers/net/usb/lan78xx.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
>> index d98397b..ffb483d 100644
>> --- a/drivers/net/usb/lan78xx.c
>> +++ b/drivers/net/usb/lan78xx.c
>> @@ -2008,6 +2008,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>>  {
>>  	int ret;
>>  	u32 mii_adv;
>> +	u32 led_modes[2];
>>  	struct phy_device *phydev;
>>  
>>  	phydev = phy_find_first(dev->mdiobus);
>> @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>>  		(void)lan78xx_set_eee(dev->net, &edata);
>>  	}
>>  
>> +	if (!of_property_read_u32_array(dev->udev->dev.of_node,
>> +					"microchip,led-modes",
>> +					led_modes, ARRAY_SIZE(led_modes))) {
>> +		u32 reg;
>> +		int i;
>> +
>> +		reg = phy_read(phydev, 0x1d);
>> +		for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
>> +			reg &= ~(0xf << (i * 4));
>> +			reg |= (led_modes[i] & 0xf) << (i * 4);
>> +		}
> 
> Please add range checks for led_modes[i] and return -EINVAL if the
> check fails.

Will do.

Thanks,

Phil

[3/4] lan78xx: Read LED modes from Device Tree

[Phil Elwell]

Hi Andrew,

On 12/04/2018 15:26, Andrew Lunn wrote:
> On Thu, Apr 12, 2018 at 02:55:35PM +0100, Phil Elwell wrote:
>> Add support for DT property "microchip,led-modes", a vector of two
>> cells (u32s) in the range 0-15, each of which sets the mode for one
>> of the two LEDs. Some possible values are:
>>
>>     0=link/activity          1=link1000/activity
>>     2=link100/activity       3=link10/activity
>>     4=link100/1000/activity  5=link10/1000/activity
>>     6=link10/100/activity    14=off    15=on
>>
>> Also use the presence of the DT property to indicate that the
>> LEDs should be enabled - necessary in the event that no valid OTP
>> or EEPROM is available.
> 
> I'm not a fan of this, but at the moment, we don't have anything
> better.
> 
> Please follow what mscc does, add a header file for the LED settings.

Good idea.

> 
>>
>> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
>> ---
>>  drivers/net/usb/lan78xx.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
>> index d98397b..ffb483d 100644
>> --- a/drivers/net/usb/lan78xx.c
>> +++ b/drivers/net/usb/lan78xx.c
>> @@ -2008,6 +2008,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>>  {
>>  	int ret;
>>  	u32 mii_adv;
>> +	u32 led_modes[2];
>>  	struct phy_device *phydev;
>>  
>>  	phydev = phy_find_first(dev->mdiobus);
>> @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>>  		(void)lan78xx_set_eee(dev->net, &edata);
>>  	}
>>  
>> +	if (!of_property_read_u32_array(dev->udev->dev.of_node,
>> +					"microchip,led-modes",
>> +					led_modes, ARRAY_SIZE(led_modes))) {
>> +		u32 reg;
>> +		int i;
>> +
>> +		reg = phy_read(phydev, 0x1d);
>> +		for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
>> +			reg &= ~(0xf << (i * 4));
>> +			reg |= (led_modes[i] & 0xf) << (i * 4);
>> +		}
> 
> Please add range checks for led_modes[i] and return -EINVAL if the
> check fails.

Will do.

Thanks,

Phil
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] dt-bindings: Document the DT bindings for lan78xx

[Phil Elwell]

Hi Andrew,

On 12/04/2018 15:30, Andrew Lunn wrote:
> On Thu, Apr 12, 2018 at 02:55:36PM +0100, Phil Elwell wrote:
>> The Microchip LAN78XX family of devices are Ethernet controllers with
>> a USB interface. Despite being discoverable devices it can be useful to
>> be able to configure them from Device Tree, particularly in low-cost
>> applications without an EEPROM or programmed OTP.
> 
> It would be good to document what happens when there is an EEPROM. Is
> OF used in preference to the EEPROM?

Yes it is. I'll mention it in V2.

Phil

[4/4] dt-bindings: Document the DT bindings for lan78xx

[Phil Elwell]

Hi Andrew,

On 12/04/2018 15:30, Andrew Lunn wrote:
> On Thu, Apr 12, 2018 at 02:55:36PM +0100, Phil Elwell wrote:
>> The Microchip LAN78XX family of devices are Ethernet controllers with
>> a USB interface. Despite being discoverable devices it can be useful to
>> be able to configure them from Device Tree, particularly in low-cost
>> applications without an EEPROM or programmed OTP.
> 
> It would be good to document what happens when there is an EEPROM. Is
> OF used in preference to the EEPROM?

Yes it is. I'll mention it in V2.

Phil
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] lan78xx: Read LED modes from Device Tree

[Andrew Lunn]

> @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  		(void)lan78xx_set_eee(dev->net, &edata);
>  	}
>  
> +	if (!of_property_read_u32_array(dev->udev->dev.of_node,
> +					"microchip,led-modes",
> +					led_modes, ARRAY_SIZE(led_modes))) {
> +		u32 reg;
> +		int i;
> +
> +		reg = phy_read(phydev, 0x1d);
> +		for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
> +			reg &= ~(0xf << (i * 4));
> +			reg |= (led_modes[i] & 0xf) << (i * 4);
> +		}
> +		(void)phy_write(phydev, 0x1d, reg);

Poking PHY registers directly from the MAC driver is not always a good
idea. This MAC driver does that in a few places :-(

What do we know about the PHY? It is built into the device or is it
external? If it is external, how do you know the LED register is at
0x1d?

The safest place to do this is in the PHY driver, and place these OF
properties into the PHY node.

	   Andrew

[3/4] lan78xx: Read LED modes from Device Tree

[Andrew Lunn]

> @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  		(void)lan78xx_set_eee(dev->net, &edata);
>  	}
>  
> +	if (!of_property_read_u32_array(dev->udev->dev.of_node,
> +					"microchip,led-modes",
> +					led_modes, ARRAY_SIZE(led_modes))) {
> +		u32 reg;
> +		int i;
> +
> +		reg = phy_read(phydev, 0x1d);
> +		for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
> +			reg &= ~(0xf << (i * 4));
> +			reg |= (led_modes[i] & 0xf) << (i * 4);
> +		}
> +		(void)phy_write(phydev, 0x1d, reg);

Poking PHY registers directly from the MAC driver is not always a good
idea. This MAC driver does that in a few places :-(

What do we know about the PHY? It is built into the device or is it
external? If it is external, how do you know the LED register is at
0x1d?

The safest place to do this is in the PHY driver, and place these OF
properties into the PHY node.

	   Andrew
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] lan78xx: Read initial EEE setting from Device Tree

[Phil Elwell]

Andrew,

On 12/04/2018 15:16, Andrew Lunn wrote:
> On Thu, Apr 12, 2018 at 02:55:34PM +0100, Phil Elwell wrote:
>> Add two new Device Tree properties:
>> * microchip,eee-enabled  - a boolean to enable EEE
>> * microchip,tx-lpi-timer - time in microseconds to wait after TX goes
>>                            idle before entering the low power state
>>                            (default 600)
> 
> Hi Phil
> 
> This looks wrong.
> 
> What should happen is that the MAC driver calls phy_init_eee() to find
> out if the PHY supports EEE. There should be no need to look in device
> tree.

If the driver should be calling phy_init_eee to initialise EEE operation then I'm fine
with that (although I notice that the TI cpsw calls phy_ethtool_set_eee but I don't see
it calling phy_init_eee). However, it sounds like I need to keep my DT toggle of the
EEE enablement and parameters downstream.

Phil

[2/4] lan78xx: Read initial EEE setting from Device Tree

[Phil Elwell]

Andrew,

On 12/04/2018 15:16, Andrew Lunn wrote:
> On Thu, Apr 12, 2018 at 02:55:34PM +0100, Phil Elwell wrote:
>> Add two new Device Tree properties:
>> * microchip,eee-enabled  - a boolean to enable EEE
>> * microchip,tx-lpi-timer - time in microseconds to wait after TX goes
>>                            idle before entering the low power state
>>                            (default 600)
> 
> Hi Phil
> 
> This looks wrong.
> 
> What should happen is that the MAC driver calls phy_init_eee() to find
> out if the PHY supports EEE. There should be no need to look in device
> tree.

If the driver should be calling phy_init_eee to initialise EEE operation then I'm fine
with that (although I notice that the TI cpsw calls phy_ethtool_set_eee but I don't see
it calling phy_init_eee). However, it sounds like I need to keep my DT toggle of the
EEE enablement and parameters downstream.

Phil
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/4] lan78xx: Read LED modes from Device Tree

[Woojung.Huh]

> > @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> >  		(void)lan78xx_set_eee(dev->net, &edata);
> >  	}
> >
> > +	if (!of_property_read_u32_array(dev->udev->dev.of_node,
> > +					"microchip,led-modes",
> > +					led_modes, ARRAY_SIZE(led_modes))) {
> > +		u32 reg;
> > +		int i;
> > +
> > +		reg = phy_read(phydev, 0x1d);
> > +		for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
> > +			reg &= ~(0xf << (i * 4));
> > +			reg |= (led_modes[i] & 0xf) << (i * 4);
> > +		}
> > +		(void)phy_write(phydev, 0x1d, reg);
> 
> Poking PHY registers directly from the MAC driver is not always a good
> idea. This MAC driver does that in a few places :-(
Agree but, some are for workaround unfortunately.

> What do we know about the PHY? It is built into the device or is it
> external? If it is external, how do you know the LED register is at
> 0x1d?
This register is not defined in include/linux/microchipphy.h. :(
Also agree that there parts should be applied to internal PHY only.

RE: [PATCH 3/4] lan78xx: Read LED modes from Device Tree

[Woojung.Huh]

> > @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> >  		(void)lan78xx_set_eee(dev->net, &edata);
> >  	}
> >
> > +	if (!of_property_read_u32_array(dev->udev->dev.of_node,
> > +					"microchip,led-modes",
> > +					led_modes, ARRAY_SIZE(led_modes))) {
> > +		u32 reg;
> > +		int i;
> > +
> > +		reg = phy_read(phydev, 0x1d);
> > +		for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
> > +			reg &= ~(0xf << (i * 4));
> > +			reg |= (led_modes[i] & 0xf) << (i * 4);
> > +		}
> > +		(void)phy_write(phydev, 0x1d, reg);
> 
> Poking PHY registers directly from the MAC driver is not always a good
> idea. This MAC driver does that in a few places :-(
Agree but, some are for workaround unfortunately.

> What do we know about the PHY? It is built into the device or is it
> external? If it is external, how do you know the LED register is at
> 0x1d?
This register is not defined in include/linux/microchipphy.h. :(
Also agree that there parts should be applied to internal PHY only.

[3/4] lan78xx: Read LED modes from Device Tree

[Woojung.Huh]

> > @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> >  		(void)lan78xx_set_eee(dev->net, &edata);
> >  	}
> >
> > +	if (!of_property_read_u32_array(dev->udev->dev.of_node,
> > +					"microchip,led-modes",
> > +					led_modes, ARRAY_SIZE(led_modes))) {
> > +		u32 reg;
> > +		int i;
> > +
> > +		reg = phy_read(phydev, 0x1d);
> > +		for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
> > +			reg &= ~(0xf << (i * 4));
> > +			reg |= (led_modes[i] & 0xf) << (i * 4);
> > +		}
> > +		(void)phy_write(phydev, 0x1d, reg);
> 
> Poking PHY registers directly from the MAC driver is not always a good
> idea. This MAC driver does that in a few places :-(
Agree but, some are for workaround unfortunately.

> What do we know about the PHY? It is built into the device or is it
> external? If it is external, how do you know the LED register is at
> 0x1d?
This register is not defined in include/linux/microchipphy.h. :(
Also agree that there parts should be applied to internal PHY only.
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] dt-bindings: Document the DT bindings for lan78xx

[Rob Herring]

On Thu, Apr 12, 2018 at 02:55:36PM +0100, Phil Elwell wrote:
> The Microchip LAN78XX family of devices are Ethernet controllers with
> a USB interface. Despite being discoverable devices it can be useful to
> be able to configure them from Device Tree, particularly in low-cost
> applications without an EEPROM or programmed OTP.
> 
> Document the supported properties in a bindings file, adding it to
> MAINTAINERS at the same time.
> 
> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
> ---
>  .../devicetree/bindings/net/microchip,lan78xx.txt  | 44 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/microchip,lan78xx.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/microchip,lan78xx.txt b/Documentation/devicetree/bindings/net/microchip,lan78xx.txt
> new file mode 100644
> index 0000000..e7d7850
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/microchip,lan78xx.txt
> @@ -0,0 +1,44 @@
> +Microchip LAN78xx Gigabit Ethernet controller
> +
> +The LAN78XX devices are usually configured by programming their OTP or with
> +an external EEPROM, but some platforms (e.g. Raspberry Pi 3 B+) have neither.
> +
> +Please refer to ethernet.txt for a description of common Ethernet bindings.
> +
> +Optional properties:
> +- microchip,eee-enabled: if present, enable Energy Efficient Ethernet support;

I see we have some flags for broken EEE, but nothing already defined to 
enable EEE. Seems like this should either be a user option (therefore 
not in DT) or we should use the broken EEE properties if this is h/w 
dependent.

> +- microchip,led-modes: a two-element vector, with each element configuring
> +  the operating mode of an LED. The values supported by the device are;
> +  0: Link/Activity
> +  1: Link1000/Activity
> +  2: Link100/Activity
> +  3: Link10/Activity
> +  4: Link100/1000/Activity
> +  5: Link10/1000/Activity
> +  6: Link10/100/Activity
> +  7: RESERVED
> +  8: Duplex/Collision
> +  9: Collision
> +  10: Activity
> +  11: RESERVED
> +  12: Auto-negotiation Fault
> +  13: RESERVED
> +  14: Off
> +  15: On
> +- microchip,tx-lpi-timer: the delay (in microseconds) between the TX fifo
> +  becoming empty and invoking Low Power Idles (default 600).

Needs a unit suffix as defined in property-units.txt.

> +
> +Example:
> +
> +	/* Standard configuration for a Raspberry Pi 3 B+ */
> +	ethernet: usbether@1 {
> +		compatible = "usb424,7800";
> +		reg = <1>;
> +		microchip,eee-enabled;
> +		microchip,tx-lpi-timer = <600>;
> +		/*
> +		 * led0 = 1:link1000/activity
> +		 * led1 = 6:link10/100/activity
> +		 */
> +		microchip,led-modes = <1 6>;
> +	};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2328eed..b637aad 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14482,6 +14482,7 @@ M:	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
>  L:	netdev@vger.kernel.org
>  S:	Maintained
>  F:	drivers/net/usb/lan78xx.*
> +F:	Documentation/devicetree/bindings/net/microchip,lan78xx.txt
>  
>  USB MASS STORAGE DRIVER
>  M:	Alan Stern <stern@rowland.harvard.edu>
> -- 
> 2.7.4
> 

[4/4] dt-bindings: Document the DT bindings for lan78xx

[Rob Herring]

On Thu, Apr 12, 2018 at 02:55:36PM +0100, Phil Elwell wrote:
> The Microchip LAN78XX family of devices are Ethernet controllers with
> a USB interface. Despite being discoverable devices it can be useful to
> be able to configure them from Device Tree, particularly in low-cost
> applications without an EEPROM or programmed OTP.
> 
> Document the supported properties in a bindings file, adding it to
> MAINTAINERS at the same time.
> 
> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
> ---
>  .../devicetree/bindings/net/microchip,lan78xx.txt  | 44 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/microchip,lan78xx.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/microchip,lan78xx.txt b/Documentation/devicetree/bindings/net/microchip,lan78xx.txt
> new file mode 100644
> index 0000000..e7d7850
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/microchip,lan78xx.txt
> @@ -0,0 +1,44 @@
> +Microchip LAN78xx Gigabit Ethernet controller
> +
> +The LAN78XX devices are usually configured by programming their OTP or with
> +an external EEPROM, but some platforms (e.g. Raspberry Pi 3 B+) have neither.
> +
> +Please refer to ethernet.txt for a description of common Ethernet bindings.
> +
> +Optional properties:
> +- microchip,eee-enabled: if present, enable Energy Efficient Ethernet support;

I see we have some flags for broken EEE, but nothing already defined to 
enable EEE. Seems like this should either be a user option (therefore 
not in DT) or we should use the broken EEE properties if this is h/w 
dependent.

> +- microchip,led-modes: a two-element vector, with each element configuring
> +  the operating mode of an LED. The values supported by the device are;
> +  0: Link/Activity
> +  1: Link1000/Activity
> +  2: Link100/Activity
> +  3: Link10/Activity
> +  4: Link100/1000/Activity
> +  5: Link10/1000/Activity
> +  6: Link10/100/Activity
> +  7: RESERVED
> +  8: Duplex/Collision
> +  9: Collision
> +  10: Activity
> +  11: RESERVED
> +  12: Auto-negotiation Fault
> +  13: RESERVED
> +  14: Off
> +  15: On
> +- microchip,tx-lpi-timer: the delay (in microseconds) between the TX fifo
> +  becoming empty and invoking Low Power Idles (default 600).

Needs a unit suffix as defined in property-units.txt.

> +
> +Example:
> +
> +	/* Standard configuration for a Raspberry Pi 3 B+ */
> +	ethernet: usbether@1 {
> +		compatible = "usb424,7800";
> +		reg = <1>;
> +		microchip,eee-enabled;
> +		microchip,tx-lpi-timer = <600>;
> +		/*
> +		 * led0 = 1:link1000/activity
> +		 * led1 = 6:link10/100/activity
> +		 */
> +		microchip,led-modes = <1 6>;
> +	};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2328eed..b637aad 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14482,6 +14482,7 @@ M:	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
>  L:	netdev@vger.kernel.org
>  S:	Maintained
>  F:	drivers/net/usb/lan78xx.*
> +F:	Documentation/devicetree/bindings/net/microchip,lan78xx.txt
>  
>  USB MASS STORAGE DRIVER
>  M:	Alan Stern <stern@rowland.harvard.edu>
> -- 
> 2.7.4
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] dt-bindings: Document the DT bindings for lan78xx

[Phil Elwell]

On 16/04/2018 20:22, Rob Herring wrote:
> On Thu, Apr 12, 2018 at 02:55:36PM +0100, Phil Elwell wrote:
>> The Microchip LAN78XX family of devices are Ethernet controllers with
>> a USB interface. Despite being discoverable devices it can be useful to
>> be able to configure them from Device Tree, particularly in low-cost
>> applications without an EEPROM or programmed OTP.
>>
>> Document the supported properties in a bindings file, adding it to
>> MAINTAINERS at the same time.
>>
>> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
>> ---
>>  .../devicetree/bindings/net/microchip,lan78xx.txt  | 44 ++++++++++++++++++++++
>>  MAINTAINERS                                        |  1 +
>>  2 files changed, 45 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/net/microchip,lan78xx.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/microchip,lan78xx.txt b/Documentation/devicetree/bindings/net/microchip,lan78xx.txt
>> new file mode 100644
>> index 0000000..e7d7850
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/microchip,lan78xx.txt
>> @@ -0,0 +1,44 @@
>> +Microchip LAN78xx Gigabit Ethernet controller
>> +
>> +The LAN78XX devices are usually configured by programming their OTP or with
>> +an external EEPROM, but some platforms (e.g. Raspberry Pi 3 B+) have neither.
>> +
>> +Please refer to ethernet.txt for a description of common Ethernet bindings.
>> +
>> +Optional properties:
>> +- microchip,eee-enabled: if present, enable Energy Efficient Ethernet support;
> 
> I see we have some flags for broken EEE, but nothing already defined to 
> enable EEE. Seems like this should either be a user option (therefore 
> not in DT) or we should use the broken EEE properties if this is h/w 
> dependent.

In the downstream Raspberry Pi kernel we use DT as a way of passing user settings
to drivers - it's more powerful than the command line. I understand that this is
not the done thing here so I'm withdrawing this element of the patch series.

Apologies for the noise.

>> +- microchip,led-modes: a two-element vector, with each element configuring
>> +  the operating mode of an LED. The values supported by the device are;
>> +  0: Link/Activity
>> +  1: Link1000/Activity
>> +  2: Link100/Activity
>> +  3: Link10/Activity
>> +  4: Link100/1000/Activity
>> +  5: Link10/1000/Activity
>> +  6: Link10/100/Activity
>> +  7: RESERVED
>> +  8: Duplex/Collision
>> +  9: Collision
>> +  10: Activity
>> +  11: RESERVED
>> +  12: Auto-negotiation Fault
>> +  13: RESERVED
>> +  14: Off
>> +  15: On
>> +- microchip,tx-lpi-timer: the delay (in microseconds) between the TX fifo
>> +  becoming empty and invoking Low Power Idles (default 600).
> 
> Needs a unit suffix as defined in property-units.txt.
> 
>> +
>> +Example:
>> +
>> +	/* Standard configuration for a Raspberry Pi 3 B+ */
>> +	ethernet: usbether@1 {
>> +		compatible = "usb424,7800";
>> +		reg = <1>;
>> +		microchip,eee-enabled;
>> +		microchip,tx-lpi-timer = <600>;
>> +		/*
>> +		 * led0 = 1:link1000/activity
>> +		 * led1 = 6:link10/100/activity
>> +		 */
>> +		microchip,led-modes = <1 6>;
>> +	};
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 2328eed..b637aad 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -14482,6 +14482,7 @@ M:	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
>>  L:	netdev@vger.kernel.org
>>  S:	Maintained
>>  F:	drivers/net/usb/lan78xx.*
>> +F:	Documentation/devicetree/bindings/net/microchip,lan78xx.txt
>>  
>>  USB MASS STORAGE DRIVER
>>  M:	Alan Stern <stern@rowland.harvard.edu>
>> -- 
>> 2.7.4
>>

[4/4] dt-bindings: Document the DT bindings for lan78xx

[Phil Elwell]

On 16/04/2018 20:22, Rob Herring wrote:
> On Thu, Apr 12, 2018 at 02:55:36PM +0100, Phil Elwell wrote:
>> The Microchip LAN78XX family of devices are Ethernet controllers with
>> a USB interface. Despite being discoverable devices it can be useful to
>> be able to configure them from Device Tree, particularly in low-cost
>> applications without an EEPROM or programmed OTP.
>>
>> Document the supported properties in a bindings file, adding it to
>> MAINTAINERS at the same time.
>>
>> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
>> ---
>>  .../devicetree/bindings/net/microchip,lan78xx.txt  | 44 ++++++++++++++++++++++
>>  MAINTAINERS                                        |  1 +
>>  2 files changed, 45 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/net/microchip,lan78xx.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/microchip,lan78xx.txt b/Documentation/devicetree/bindings/net/microchip,lan78xx.txt
>> new file mode 100644
>> index 0000000..e7d7850
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/microchip,lan78xx.txt
>> @@ -0,0 +1,44 @@
>> +Microchip LAN78xx Gigabit Ethernet controller
>> +
>> +The LAN78XX devices are usually configured by programming their OTP or with
>> +an external EEPROM, but some platforms (e.g. Raspberry Pi 3 B+) have neither.
>> +
>> +Please refer to ethernet.txt for a description of common Ethernet bindings.
>> +
>> +Optional properties:
>> +- microchip,eee-enabled: if present, enable Energy Efficient Ethernet support;
> 
> I see we have some flags for broken EEE, but nothing already defined to 
> enable EEE. Seems like this should either be a user option (therefore 
> not in DT) or we should use the broken EEE properties if this is h/w 
> dependent.

In the downstream Raspberry Pi kernel we use DT as a way of passing user settings
to drivers - it's more powerful than the command line. I understand that this is
not the done thing here so I'm withdrawing this element of the patch series.

Apologies for the noise.

>> +- microchip,led-modes: a two-element vector, with each element configuring
>> +  the operating mode of an LED. The values supported by the device are;
>> +  0: Link/Activity
>> +  1: Link1000/Activity
>> +  2: Link100/Activity
>> +  3: Link10/Activity
>> +  4: Link100/1000/Activity
>> +  5: Link10/1000/Activity
>> +  6: Link10/100/Activity
>> +  7: RESERVED
>> +  8: Duplex/Collision
>> +  9: Collision
>> +  10: Activity
>> +  11: RESERVED
>> +  12: Auto-negotiation Fault
>> +  13: RESERVED
>> +  14: Off
>> +  15: On
>> +- microchip,tx-lpi-timer: the delay (in microseconds) between the TX fifo
>> +  becoming empty and invoking Low Power Idles (default 600).
> 
> Needs a unit suffix as defined in property-units.txt.
> 
>> +
>> +Example:
>> +
>> +	/* Standard configuration for a Raspberry Pi 3 B+ */
>> +	ethernet: usbether@1 {
>> +		compatible = "usb424,7800";
>> +		reg = <1>;
>> +		microchip,eee-enabled;
>> +		microchip,tx-lpi-timer = <600>;
>> +		/*
>> +		 * led0 = 1:link1000/activity
>> +		 * led1 = 6:link10/100/activity
>> +		 */
>> +		microchip,led-modes = <1 6>;
>> +	};
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 2328eed..b637aad 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -14482,6 +14482,7 @@ M:	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
>>  L:	netdev@vger.kernel.org
>>  S:	Maintained
>>  F:	drivers/net/usb/lan78xx.*
>> +F:	Documentation/devicetree/bindings/net/microchip,lan78xx.txt
>>  
>>  USB MASS STORAGE DRIVER
>>  M:	Alan Stern <stern@rowland.harvard.edu>
>> -- 
>> 2.7.4
>>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html