[PATCH 1/1 RFC] net/phy: Add Lantiq PHY driver

This currently only supports PEF7071 and allows to specify max-speed and
is able to read the LED configuration from device-tree.

Signed-off-by: Alexander Stein <redacted>
---
The main purpose for now is to set a LED configuration from device tree and
to limit the maximum speed. The latter one in my case hardware limited.
As MAC and it's link partner support 1000MBit/s they would try to use that
but will eventually fail due to magnetics only supporting 100MBit/s. So
limit the maximum link speed supported directly from the start.

As this is a RFC I skipped the device tree binding doc.

 drivers/net/phy/Kconfig  |   5 ++
 drivers/net/phy/Makefile |   1 +
 drivers/net/phy/lantiq.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 173 insertions(+)
 create mode 100644 drivers/net/phy/lantiq.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 3e28f7a..c004885 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig

@@ -119,6 +119,11 @@ config STE10XP
 	---help---
 	  This is the driver for the STe100p and STe101p PHYs.
 
+config LANTIQ_PHY
+	tristate "Driver for Lantiq PHYs"
+	---help---
+	  Supports the PEF7071 PHYs.
+
 config LSI_ET1011C_PHY
 	tristate "Driver for LSI ET1011C PHY"
 	---help---

diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 8ad4ac6..e886549 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile

@@ -38,3 +38,4 @@ obj-$(CONFIG_MDIO_SUN4I)	+= mdio-sun4i.o
 obj-$(CONFIG_MDIO_MOXART)	+= mdio-moxart.o
 obj-$(CONFIG_AMD_XGBE_PHY)	+= amd-xgbe-phy.o
 obj-$(CONFIG_MDIO_BCM_UNIMAC)	+= mdio-bcm-unimac.o
+obj-$(CONFIG_LANTIQ_PHY)	+= lantiq.o

diff --git a/drivers/net/phy/lantiq.c b/drivers/net/phy/lantiq.c
new file mode 100644
index 0000000..876a7d1
--- /dev/null
+++ b/drivers/net/phy/lantiq.c

@@ -0,0 +1,167 @@
+/*
+ * Driver for Lantiq PHYs
+ *
+ * Author: Alexander Stein <alexander.stein@systec-electronic.com>
+ *
+ * Copyright (c) 2015-2016 SYS TEC electronic GmbH
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/phy.h>
+#include <linux/of.h>
+
+#define PHY_ID_PEF7071	0xd565a401
+
+#define MII_LANTIQ_MMD_CTRL_REG		0x0d
+#define MII_LANTIQ_MMD_REGDATA_REG	0x0e
+#define OP_DATA				1
+
+struct lantiqphy_led_ctrl {
+	const char *property;
+	u32 regnum;
+};
+
+static int lantiq_extended_write(struct phy_device *phydev,
+				 u8 mode, u32 dev_addr, u32 regnum, u16 val)
+{
+	phy_write(phydev, MII_LANTIQ_MMD_CTRL_REG, dev_addr);
+	phy_write(phydev, MII_LANTIQ_MMD_REGDATA_REG, regnum);
+	phy_write(phydev, MII_LANTIQ_MMD_CTRL_REG, (mode << 14) | dev_addr);
+	return phy_write(phydev, MII_LANTIQ_MMD_REGDATA_REG, val);
+}
+
+static int lantiq_of_load_led_config(struct phy_device *phydev,
+				     struct device_node *of_node,
+				     const struct lantiqphy_led_ctrl *leds,
+				     u8 entries)
+{
+	u16 val;
+	int i;
+	int ret = 0;
+
+	for (i = 0; i < entries; i++) {
+		if (!of_property_read_u16(of_node, leds[i].property, &val)) {
+			ret = lantiq_extended_write(phydev, OP_DATA, 0x1f,
+						    leds[i].regnum, val);
+			if (ret) {
+				dev_err(&phydev->dev, "Error writing register 0x1f.%04x (%d)\n",
+					leds[i].regnum, ret);
+				break;
+			}
+		}
+	}
+
+	return ret;
+}
+
+static const struct lantiqphy_led_ctrl leds[] = {
+	{
+		.property = "led0h",
+		.regnum = 0x01e2,
+	},
+	{
+		.property = "led0l",
+		.regnum = 0x01e3,
+	},
+	{
+		.property = "led1h",
+		.regnum = 0x01e4,
+	},
+	{
+		.property = "led1l",
+		.regnum = 0x01e5,
+	},
+	{
+		.property = "led2h",
+		.regnum = 0x01e6,
+	},
+	{
+		.property = "led2l",
+		.regnum = 0x01e7,
+	},
+};
+
+static int lantiqphy_config_init(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->dev;
+	struct device_node *of_node = dev->of_node;
+	u32 max_speed;
+
+	if (!of_node && dev->parent->of_node)
+		of_node = dev->parent->of_node;
+
+	if (of_node) {
+		lantiq_of_load_led_config(phydev, of_node, leds,
+					  ARRAY_SIZE(leds));
+
+		if (!of_property_read_u32(of_node, "max-speed",
+					  &max_speed)) {
+			/* The default values for phydev->supported are
+			 * provided by the PHY driver "features" member,
+			 * we want to reset to sane defaults fist before
+			 * supporting higher speeds.
+			 */
+			phydev->supported &= PHY_DEFAULT_FEATURES;
+
+			switch (max_speed) {
+			default:
+				break;
+
+			case SPEED_1000:
+				phydev->supported |= PHY_1000BT_FEATURES;
+			case SPEED_100:
+				phydev->supported |= PHY_100BT_FEATURES;
+			case SPEED_10:
+				phydev->supported |= PHY_10BT_FEATURES;
+			}
+		}
+	}
+	return 0;
+}
+
+static struct phy_driver lantiqphy_driver[] = {
+{
+	.phy_id		= PHY_ID_PEF7071,
+	.phy_id_mask	= 0x00fffffe,
+	.name		= "Lantiq PEF7071",
+	.features	= (PHY_GBIT_FEATURES | SUPPORTED_Pause),
+	.flags		= PHY_HAS_MAGICANEG,
+	.config_init	= lantiqphy_config_init,
+	.config_aneg	= genphy_config_aneg,
+	.read_status	= genphy_read_status,
+	.suspend	= genphy_suspend,
+	.resume		= genphy_resume,
+	.driver		= { .owner = THIS_MODULE,},
+} };
+
+static int __init lantiqphy_init(void)
+{
+	return phy_drivers_register(lantiqphy_driver,
+				    ARRAY_SIZE(lantiqphy_driver));
+}
+
+static void __exit lantiqphy_exit(void)
+{
+	phy_drivers_unregister(lantiqphy_driver,
+			       ARRAY_SIZE(lantiqphy_driver));
+}
+
+module_init(lantiqphy_init);
+module_exit(lantiqphy_exit);
+
+MODULE_DESCRIPTION("Lantiq PHY driver");
+MODULE_AUTHOR("Alexander Stein <alexander.stein@systec-electronic.com>");
+MODULE_LICENSE("GPL");
+
+static struct mdio_device_id __maybe_unused lantiq_tbl[] = {
+	{ PHY_ID_PEF7071, 0x00fffffe },
+	{ }
+};
+MODULE_DEVICE_TABLE(mdio, lantiq_tbl);

-- 
2.7.3

CC'ing Andrew, John,

On 05/18/2016 09:03 AM, Alexander Stein wrote:

This currently only supports PEF7071 and allows to specify max-speed and
is able to read the LED configuration from device-tree.

Signed-off-by: Alexander Stein <redacted>
---
The main purpose for now is to set a LED configuration from device tree and
to limit the maximum speed. The latter one in my case hardware limited.
As MAC and it's link partner support 1000MBit/s they would try to use that
but will eventually fail due to magnetics only supporting 100MBit/s. So
limit the maximum link speed supported directly from the start.

The 'max-speed' parsing that you do in the driver should not be needed,
PHYLIB takes care of that already see
drivers/net/phy/phy_device.c::of_set_phy_supported

For LEDs, we had a patch series floating around adding LED triggers [1],
and it seems to me like the LEDs class subsystem would be a good fit for
controlling PHY LEDs, possibly with the help of PHYLIB when it comes to
doing the low-level work of registering LEDs and their names with the
LEDS subsystem.

[1]: http://lists.openwall.net/netdev/2016/03/23/61

As this is a RFC I skipped the device tree binding doc.

Too bad, that's probably what needs to be discussed here, because the
driver looks pretty reasonable otherwise.

-- 
Florian

For LEDs, we had a patch series floating around adding LED triggers [1],
and it seems to me like the LEDs class subsystem would be a good fit for
controlling PHY LEDs, possibly with the help of PHYLIB when it comes to
doing the low-level work of registering LEDs and their names with the
LEDS subsystem.

[1]: http://lists.openwall.net/netdev/2016/03/23/61

That patch fizzled out. I got the feeling it was pushing the
capabilities of the coder. I do however think it is a reasonable path
to follow for PHY LEDs.

I took a quick look at the datasheet and the controlling of the LEDs
is very flexible. It should not be a problem to expose some of that
functionality via LED triggers.

      Andrew

Hi,

On 18/05/2016 18:24, Florian Fainelli wrote:

CC'ing Andrew, John,

also CC'ing Matthias and Hauke. we have had a driver in OpenWrt/LEDE for
several years that seems a little more complete than this one.

https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/lantiq/patches-4.4/0023-NET-PHY-adds-driver-for-lantiq-PHY11G.patch;h=93bb4275ec1d261f398afb8fdc879c1dd973f997;hb=HEAD

	John

On 05/18/2016 09:03 AM, Alexander Stein wrote:

quoted

This currently only supports PEF7071 and allows to specify max-speed and
is able to read the LED configuration from device-tree.

Signed-off-by: Alexander Stein <redacted>
---
The main purpose for now is to set a LED configuration from device tree and
to limit the maximum speed. The latter one in my case hardware limited.
As MAC and it's link partner support 1000MBit/s they would try to use that
but will eventually fail due to magnetics only supporting 100MBit/s. So
limit the maximum link speed supported directly from the start.

The 'max-speed' parsing that you do in the driver should not be needed,
PHYLIB takes care of that already see
drivers/net/phy/phy_device.c::of_set_phy_supported

For LEDs, we had a patch series floating around adding LED triggers [1],
and it seems to me like the LEDs class subsystem would be a good fit for
controlling PHY LEDs, possibly with the help of PHYLIB when it comes to
doing the low-level work of registering LEDs and their names with the
LEDS subsystem.

[1]: http://lists.openwall.net/netdev/2016/03/23/61

quoted

As this is a RFC I skipped the device tree binding doc.

Too bad, that's probably what needs to be discussed here, because the
driver looks pretty reasonable otherwise.

quoted

 drivers/net/phy/Kconfig  |   5 ++
 drivers/net/phy/Makefile |   1 +
 drivers/net/phy/lantiq.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 173 insertions(+)
 create mode 100644 drivers/net/phy/lantiq.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 3e28f7a..c004885 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig

@@ -119,6 +119,11 @@ config STE10XP
 	---help---
 	  This is the driver for the STe100p and STe101p PHYs.
 
+config LANTIQ_PHY
+	tristate "Driver for Lantiq PHYs"
+	---help---
+	  Supports the PEF7071 PHYs.
+
 config LSI_ET1011C_PHY
 	tristate "Driver for LSI ET1011C PHY"
 	---help---

diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 8ad4ac6..e886549 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile

@@ -38,3 +38,4 @@ obj-$(CONFIG_MDIO_SUN4I)	+= mdio-sun4i.o
 obj-$(CONFIG_MDIO_MOXART)	+= mdio-moxart.o
 obj-$(CONFIG_AMD_XGBE_PHY)	+= amd-xgbe-phy.o
 obj-$(CONFIG_MDIO_BCM_UNIMAC)	+= mdio-bcm-unimac.o
+obj-$(CONFIG_LANTIQ_PHY)	+= lantiq.o

diff --git a/drivers/net/phy/lantiq.c b/drivers/net/phy/lantiq.c
new file mode 100644
index 0000000..876a7d1
--- /dev/null
+++ b/drivers/net/phy/lantiq.c

@@ -0,0 +1,167 @@
+/*
+ * Driver for Lantiq PHYs
+ *
+ * Author: Alexander Stein <alexander.stein@systec-electronic.com>
+ *
+ * Copyright (c) 2015-2016 SYS TEC electronic GmbH
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/phy.h>
+#include <linux/of.h>
+
+#define PHY_ID_PEF7071	0xd565a401
+
+#define MII_LANTIQ_MMD_CTRL_REG		0x0d
+#define MII_LANTIQ_MMD_REGDATA_REG	0x0e
+#define OP_DATA				1
+
+struct lantiqphy_led_ctrl {
+	const char *property;
+	u32 regnum;
+};
+
+static int lantiq_extended_write(struct phy_device *phydev,
+				 u8 mode, u32 dev_addr, u32 regnum, u16 val)
+{
+	phy_write(phydev, MII_LANTIQ_MMD_CTRL_REG, dev_addr);
+	phy_write(phydev, MII_LANTIQ_MMD_REGDATA_REG, regnum);
+	phy_write(phydev, MII_LANTIQ_MMD_CTRL_REG, (mode << 14) | dev_addr);
+	return phy_write(phydev, MII_LANTIQ_MMD_REGDATA_REG, val);
+}
+
+static int lantiq_of_load_led_config(struct phy_device *phydev,
+				     struct device_node *of_node,
+				     const struct lantiqphy_led_ctrl *leds,
+				     u8 entries)
+{
+	u16 val;
+	int i;
+	int ret = 0;
+
+	for (i = 0; i < entries; i++) {
+		if (!of_property_read_u16(of_node, leds[i].property, &val)) {
+			ret = lantiq_extended_write(phydev, OP_DATA, 0x1f,
+						    leds[i].regnum, val);
+			if (ret) {
+				dev_err(&phydev->dev, "Error writing register 0x1f.%04x (%d)\n",
+					leds[i].regnum, ret);
+				break;
+			}
+		}
+	}
+
+	return ret;
+}
+
+static const struct lantiqphy_led_ctrl leds[] = {
+	{
+		.property = "led0h",
+		.regnum = 0x01e2,
+	},
+	{
+		.property = "led0l",
+		.regnum = 0x01e3,
+	},
+	{
+		.property = "led1h",
+		.regnum = 0x01e4,
+	},
+	{
+		.property = "led1l",
+		.regnum = 0x01e5,
+	},
+	{
+		.property = "led2h",
+		.regnum = 0x01e6,
+	},
+	{
+		.property = "led2l",
+		.regnum = 0x01e7,
+	},
+};
+
+static int lantiqphy_config_init(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->dev;
+	struct device_node *of_node = dev->of_node;
+	u32 max_speed;
+
+	if (!of_node && dev->parent->of_node)
+		of_node = dev->parent->of_node;
+
+	if (of_node) {
+		lantiq_of_load_led_config(phydev, of_node, leds,
+					  ARRAY_SIZE(leds));
+
+		if (!of_property_read_u32(of_node, "max-speed",
+					  &max_speed)) {
+			/* The default values for phydev->supported are
+			 * provided by the PHY driver "features" member,
+			 * we want to reset to sane defaults fist before
+			 * supporting higher speeds.
+			 */
+			phydev->supported &= PHY_DEFAULT_FEATURES;
+
+			switch (max_speed) {
+			default:
+				break;
+
+			case SPEED_1000:
+				phydev->supported |= PHY_1000BT_FEATURES;
+			case SPEED_100:
+				phydev->supported |= PHY_100BT_FEATURES;
+			case SPEED_10:
+				phydev->supported |= PHY_10BT_FEATURES;
+			}
+		}
+	}
+	return 0;
+}
+
+static struct phy_driver lantiqphy_driver[] = {
+{
+	.phy_id		= PHY_ID_PEF7071,
+	.phy_id_mask	= 0x00fffffe,
+	.name		= "Lantiq PEF7071",
+	.features	= (PHY_GBIT_FEATURES | SUPPORTED_Pause),
+	.flags		= PHY_HAS_MAGICANEG,
+	.config_init	= lantiqphy_config_init,
+	.config_aneg	= genphy_config_aneg,
+	.read_status	= genphy_read_status,
+	.suspend	= genphy_suspend,
+	.resume		= genphy_resume,
+	.driver		= { .owner = THIS_MODULE,},
+} };
+
+static int __init lantiqphy_init(void)
+{
+	return phy_drivers_register(lantiqphy_driver,
+				    ARRAY_SIZE(lantiqphy_driver));
+}
+
+static void __exit lantiqphy_exit(void)
+{
+	phy_drivers_unregister(lantiqphy_driver,
+			       ARRAY_SIZE(lantiqphy_driver));
+}
+
+module_init(lantiqphy_init);
+module_exit(lantiqphy_exit);
+
+MODULE_DESCRIPTION("Lantiq PHY driver");
+MODULE_AUTHOR("Alexander Stein <alexander.stein@systec-electronic.com>");
+MODULE_LICENSE("GPL");
+
+static struct mdio_device_id __maybe_unused lantiq_tbl[] = {
+	{ PHY_ID_PEF7071, 0x00fffffe },
+	{ }
+};
+MODULE_DEVICE_TABLE(mdio, lantiq_tbl);

Hi John,

On Thursday 19 May 2016 06:50:56, John Crispin wrote:

On 18/05/2016 18:24, Florian Fainelli wrote:

quoted

CC'ing Andrew, John,

also CC'ing Matthias and Hauke. we have had a driver in OpenWrt/LEDE for
several years that seems a little more complete than this one.

https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/lantiq/patc
hes-4.4/0023-NET-PHY-adds-driver-for-lantiq-PHY11G.patch;h=93bb4275ec1d261f3
98afb8fdc879c1dd973f997;hb=HEAD

Thanks for the link, I wasn't aware of that patch. I like it in general, but 
there are some things I'd like to get addressed first:
* vr9_gphy_of_reg_init() writes uncoditionally to led3h and led3l even on
  PEf7071 which does not have this register at all
* Why is PHY_HAS_INTERRUPT commented out everywhere?
* ltq_phy_init and ltq_phy_exit can be simplified using phy_drivers_register
  and phy_drivers_unregister
* A mdio_device_id table is missing

Best regards,
Alexander

[ changing haukes mail addr to the intel one ]

On 19/05/2016 08:57, Alexander Stein wrote:

Hi John,

On Thursday 19 May 2016 06:50:56, John Crispin wrote:

quoted

On 18/05/2016 18:24, Florian Fainelli wrote:

quoted

CC'ing Andrew, John,

also CC'ing Matthias and Hauke. we have had a driver in OpenWrt/LEDE for
several years that seems a little more complete than this one.

https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/lantiq/patc
hes-4.4/0023-NET-PHY-adds-driver-for-lantiq-PHY11G.patch;h=93bb4275ec1d261f3
98afb8fdc879c1dd973f997;hb=HEAD

Thanks for the link, I wasn't aware of that patch. I like it in general, but 
there are some things I'd like to get addressed first:
* vr9_gphy_of_reg_init() writes uncoditionally to led3h and led3l even on
  PEf7071 which does not have this register at all

we use this driver mainly on the 11g and 22f version. mathias recently
added the led3 handling.

@Mathias, can you have a look at this and fix it inside the lede tree ?

* Why is PHY_HAS_INTERRUPT commented out everywhere?

legacy code, the old mips silicon had a bug and the internal phys irq
lines worked unreliably so we used polling instead. rather than remove
the code i just disabled that part. code is not cleaned up yet for
upstream submission as you can tell :-)

* ltq_phy_init and ltq_phy_exit can be simplified using phy_drivers_register
  and phy_drivers_unregister

yes, this driver is based on a version made by daniel from spharion
about 5 years ago. that api probably did not exist at the time

* A mdio_device_id table is missing

same as previous answer, old api version

	John

On Wednesday 18 May 2016 19:01:09, Andrew Lunn wrote:

quoted

For LEDs, we had a patch series floating around adding LED triggers [1],
and it seems to me like the LEDs class subsystem would be a good fit for
controlling PHY LEDs, possibly with the help of PHYLIB when it comes to
doing the low-level work of registering LEDs and their names with the
LEDS subsystem.

[1]: http://lists.openwall.net/netdev/2016/03/23/61

That patch fizzled out. I got the feeling it was pushing the
capabilities of the coder. I do however think it is a reasonable path
to follow for PHY LEDs.

I took a quick look at the datasheet and the controlling of the LEDs
is very flexible. It should not be a problem to expose some of that
functionality via LED triggers.

To be honest I don't know how the PHY LEDs could be set by LED triggers. 
Wouldn't that require to create triggers for each value which can be written 
to LEDxH and LEDxL? In my case the hardware requires some specific setting due 
to LED connections.
Of course making the LED configuration changeable at runtime would be awesome.

Best regards,
Alexander

On Thursday 19 May 2016 09:03:26, John Crispin wrote:

[ changing haukes mail addr to the intel one ]

On 19/05/2016 08:57, Alexander Stein wrote:

quoted

Hi John,

On Thursday 19 May 2016 06:50:56, John Crispin wrote:

quoted

On 18/05/2016 18:24, Florian Fainelli wrote:

quoted

CC'ing Andrew, John,

also CC'ing Matthias and Hauke. we have had a driver in OpenWrt/LEDE for
several years that seems a little more complete than this one.

https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/lantiq/p
atc
hes-4.4/0023-NET-PHY-adds-driver-for-lantiq-PHY11G.patch;h=93bb4275ec1d2
61f3 98afb8fdc879c1dd973f997;hb=HEAD

Thanks for the link, I wasn't aware of that patch. I like it in general,
but there are some things I'd like to get addressed first:
* vr9_gphy_of_reg_init() writes uncoditionally to led3h and led3l even on

  PEf7071 which does not have this register at all

we use this driver mainly on the 11g and 22f version. mathias recently
added the led3 handling.

@Mathias, can you have a look at this and fix it inside the lede tree ?

quoted

* Why is PHY_HAS_INTERRUPT commented out everywhere?

legacy code, the old mips silicon had a bug and the internal phys irq
lines worked unreliably so we used polling instead. rather than remove
the code i just disabled that part. code is not cleaned up yet for
upstream submission as you can tell :-)

Would you or Mathias mind dropping a cleaned up patch to netdev ml, cc'ing me? 
I can try it on our hardware using the 11g. Maybe I can even test the IRQ 
feature.

Regards,
Alexander

2016-05-19 9:03 GMT+02:00 John Crispin [off-list ref]:

On 19/05/2016 08:57, Alexander Stein wrote:

quoted

Thanks for the link, I wasn't aware of that patch. I like it in general, but
there are some things I'd like to get addressed first:
* vr9_gphy_of_reg_init() writes uncoditionally to led3h and led3l even on
  PEf7071 which does not have this register at all

we use this driver mainly on the 11g and 22f version. mathias recently
added the led3 handling.

@Mathias, can you have a look at this and fix it inside the lede tree ?

Well, I haven't added the led3 handling, I've only changed the initial
value (function) of led3.

Maybe it's cleaner to not use a default value for the led function and
completely rely on the device tree bindings. But by adjusting the
initial values, I had to change only the led function of one board in
the openwrt xrx200 subtarget instead of touching all dts files.

I know that the LTQ Datasheet for the PEF 7071 Version 1.5 mentions
the led3 control register albeit there is no pin for a forth led. So I
guess it's safe to write to the led3 register even for the PEF 7071.

Mathias

On Thursday 19 May 2016 12:03:10, Mathias Kresin wrote:

2016-05-19 9:03 GMT+02:00 John Crispin [off-list ref]:

quoted

On 19/05/2016 08:57, Alexander Stein wrote:

quoted

Thanks for the link, I wasn't aware of that patch. I like it in general,
but there are some things I'd like to get addressed first:
* vr9_gphy_of_reg_init() writes uncoditionally to led3h and led3l even on

  PEf7071 which does not have this register at all

we use this driver mainly on the 11g and 22f version. mathias recently
added the led3 handling.

@Mathias, can you have a look at this and fix it inside the lede tree ?

Well, I haven't added the led3 handling, I've only changed the initial
value (function) of led3.

Maybe it's cleaner to not use a default value for the led function and
completely rely on the device tree bindings. But by adjusting the
initial values, I had to change only the led function of one board in
the openwrt xrx200 subtarget instead of touching all dts files.

I think setting default values is good.

I know that the LTQ Datasheet for the PEF 7071 Version 1.5 mentions
the led3 control register albeit there is no pin for a forth led. So I
guess it's safe to write to the led3 register even for the PEF 7071.

Mh, my PEF 7071 User Manual (Version 2.0, 2012-10-17) doesn't mention LED3x 
registers. There is LED3DA and LED3EN in PHY_LED but was removed in 1.6 
manual.
I think, some flag if the PHY supports LED3 and depend on that is just fine.

Best regards,
Alexander

To be honest I don't know how the PHY LEDs could be set by LED triggers. 
Wouldn't that require to create triggers for each value which can be written 
to LEDxH and LEDxL? In my case the hardware requires some specific setting due 
to LED connections.

Supporting all possibilities is not possible. However you could
picking out a subset which other PHYs could also implement. Think
about the typical patterns you see if you have two LEDs, which seems
to be the most common setup.

We would have generic PHY trigger code, and a driver specific part
which configures the PHY to that configuration, if the trigger is
activated.

	Andrew

Hi Alexander,

-----Original Message-----
From: Alexander Stein [mailto:alexander.stein@systec-electronic.com]
Sent: Thursday, May 19, 2016 12:22 PM
To: Mathias Kresin <redacted>
Cc: John Crispin <john@phrozen.org>; Florian Fainelli <f.fainelli@gmail.com>;
netdev@vger.kernel.org; linux-kernel@vger.kernel.org; andrew@lunn.ch;
Mehrtens, Hauke [off-list ref]
Subject: Re: [PATCH 1/1 RFC] net/phy: Add Lantiq PHY driver

On Thursday 19 May 2016 12:03:10, Mathias Kresin wrote:

quoted

2016-05-19 9:03 GMT+02:00 John Crispin [off-list ref]:

quoted

On 19/05/2016 08:57, Alexander Stein wrote:

quoted

Thanks for the link, I wasn't aware of that patch. I like it in
general, but there are some things I'd like to get addressed first:
* vr9_gphy_of_reg_init() writes uncoditionally to led3h and led3l
even on

  PEf7071 which does not have this register at all

we use this driver mainly on the 11g and 22f version. mathias
recently added the led3 handling.

@Mathias, can you have a look at this and fix it inside the lede tree ?

Well, I haven't added the led3 handling, I've only changed the initial
value (function) of led3.

Maybe it's cleaner to not use a default value for the led function and
completely rely on the device tree bindings. But by adjusting the
initial values, I had to change only the led function of one board in
the openwrt xrx200 subtarget instead of touching all dts files.

I think setting default values is good.

The registers are set to some reset values after the chip is coming out of reset, but we should set  them all to the same value, Mathias said that all except for one board he knows are using only one LED per port, but they are often using different LED pins, I will change my patch.

quoted

I know that the LTQ Datasheet for the PEF 7071 Version 1.5 mentions
the led3 control register albeit there is no pin for a forth led. So I
guess it's safe to write to the led3 register even for the PEF 7071.

Mh, my PEF 7071 User Manual (Version 2.0, 2012-10-17) doesn't mention
LED3x registers. There is LED3DA and LED3EN in PHY_LED but was removed in
1.6 manual.

LED3x is only available in PEF 7072 which is a different package with more pins for the LED3 and some other interfaces.

I think, some flag if the PHY supports LED3 and depend on that is just fine.

I do not know how to distinguish between PEF 7071 and PEF 7072.

Hauke

Hi Hauke,

On Monday 23 May 2016 09:12:54, Mehrtens, Hauke wrote:

quoted

On Thursday 19 May 2016 12:03:10, Mathias Kresin wrote:

quoted

2016-05-19 9:03 GMT+02:00 John Crispin [off-list ref]:

quoted

On 19/05/2016 08:57, Alexander Stein wrote:

quoted

Thanks for the link, I wasn't aware of that patch. I like it in
general, but there are some things I'd like to get addressed first:
* vr9_gphy_of_reg_init() writes uncoditionally to led3h and led3l
even on

  PEf7071 which does not have this register at all

we use this driver mainly on the 11g and 22f version. mathias
recently added the led3 handling.

@Mathias, can you have a look at this and fix it inside the lede tree
?

Well, I haven't added the led3 handling, I've only changed the initial
value (function) of led3.

Maybe it's cleaner to not use a default value for the led function and
completely rely on the device tree bindings. But by adjusting the
initial values, I had to change only the led function of one board in
the openwrt xrx200 subtarget instead of touching all dts files.

I think setting default values is good.

The registers are set to some reset values after the chip is coming out of
reset, but we should set  them all to the same value, Mathias said that all
except for one board he knows are using only one LED per port, but they are
often using different LED pins, I will change my patch.

One LED per port? I would think of using one RJ45 socket per port which 
usually have 2 LEDs.

quoted

quoted

I know that the LTQ Datasheet for the PEF 7071 Version 1.5 mentions
the led3 control register albeit there is no pin for a forth led. So I
guess it's safe to write to the led3 register even for the PEF 7071.

Mh, my PEF 7071 User Manual (Version 2.0, 2012-10-17) doesn't mention
LED3x registers. There is LED3DA and LED3EN in PHY_LED but was removed in
1.6 manual.

LED3x is only available in PEF 7072 which is a different package with more
pins for the LED3 and some other interfaces.

quoted

I think, some flag if the PHY supports LED3 and depend on that is just
fine.

I do not know how to distinguish between PEF 7071 and PEF 7072.

I expected that PEF 7072 would have a different PHY ID, but apparently this is 
not the case, though I don't have a datasheet for 7072. Is there really no way 
to distinguish those two?

Alexander

2016-05-23 11:49 GMT+02:00 Alexander Stein
[off-list ref]:

quoted

The registers are set to some reset values after the chip is coming out of
reset, but we should set  them all to the same value, Mathias said that all
except for one board he knows are using only one LED per port, but they are
often using different LED pins, I will change my patch.

One LED per port? I would think of using one RJ45 socket per port which
usually have 2 LEDs.

The majority of the CPEs I'm talking about, do not have any leds at
the back side RJ45 socket. They are using a single led at the front
for status indication.

I'm only aware of one CPE that has two LEDs at the RJ45 socket.

Mathias