[PATCH RFC leds + net-next v2 0/1] Add support for LEDs on Marvell PHYs

[PATCH RFC leds + net-next v2 0/1] Add support for LEDs on Marvell PHYs

[Marek Behún]

Hi,

this is v2 of my RFC adding support for LEDs connected to Marvell PHYs.

The LED subsystem patches are not contained:
- the patch adding support for LED private triggers is already accepted
  in Pavel Machek's for-next tree.
  If you want to try this patch on top of net-next, please also apply
  https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next&id=93690cdf3060c61dfce813121d0bfc055e7fa30d
- the other led-trigger patch is not needed in this version of the RFC

The main difference from v1 is that only one trigger, named
"hw-control", is added to the /sys/class/leds/<LED>/trigger file.

When this trigger is activated, another file called "hw_control" is
created in the /sys/class/leds/<LED>/ directory. This file lists
available HW control modes for this LED in the same way the trigger
file does for triggers.

Example:

  # cd /sys/class/leds/eth0\:green\:link/
  # cat trigger
  [none] hw-control timer oneshot heartbeat ...
  # echo hw-control >trigger
  # cat trigger
  none [hw-control] timer oneshot heartbeat ...
  # cat hw_control
  link/nolink link/act/nolink 1000/100/10/nolink act/noact
  blink-act/noact transmit/notransmit copperlink/else [1000/else]
  force-hi-z force-blink
  # echo 1000/100/10/nolink >hw_control
  # cat hw_control
  link/nolink link/act/nolink [1000/100/10/nolink] act/noact
  blink-act/noact transmit/notransmit copperlink/else 1000/else
  force-hi-z force-blink

The benefit here is that only one trigger is registered via LED API.
I guess there are other PHY drivers which too support HW controlled
blinking modes. So of this way of controlling PHY LED HW controlled
modes is accepted, the code creating the hw-control trigger and
hw_control file should be made into library code so that it can be
reused.

What do you think?

Marek

Marek Behún (1):
  net: phy: marvell: add support for PHY LEDs via LED class

 drivers/net/phy/Kconfig   |   7 +
 drivers/net/phy/marvell.c | 423 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 429 insertions(+), 1 deletion(-)

-- 
2.26.2

[PATCH RFC leds + net-next v2 1/1] net: phy: marvell: add support for PHY LEDs via LED class

[Marek Behún]

This patch adds support for controlling the LEDs connected to several
families of Marvell PHYs via Linux' LED API. These families are:
88E1112, 88E1121R, 88E1240, 88E1340S, 88E1510 and 88E1545. More can be
added.

The code reads LEDs definitions from the device-tree node of the PHY.

Since the LEDs can be controlled by hardware, we add one LED-private LED
trigger named "hw-control". This trigger is private and displayed only
for Marvell PHY LEDs.

When this driver is activated, another sysfs file is created in that
LEDs sysfs directory, names "hw_control". This file contains space
separated list of possible HW controlled modes for this LED. The one
which is selected is enclosed by square brackets. To change HW control
mode the user has to write the name of desired mode to this "hw_control"
file.

This patch does not yet add support for compound LED modes. This could
be achieved via the LED multicolor framework (which is not yet in
upstream).

Settings such as HW blink rate or pulse stretch duration are not yet
supported, nor are LED polarity settings.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/net/phy/Kconfig   |   7 +
 drivers/net/phy/marvell.c | 423 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 429 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index dd20c2c27c2f..fb75abdb9f24 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -456,6 +456,13 @@ config MARVELL_PHY
 	help
 	  Currently has a driver for the 88E1011S
 
+config MARVELL_PHY_LEDS
+	bool "Support LEDs on Marvell PHYs"
+	depends on MARVELL_PHY && LEDS_TRIGGERS
+	help
+	  This option enables support for controlling LEDs connected to Marvell
+	  PHYs.
+
 config MARVELL_10G_PHY
 	tristate "Marvell Alaska 10Gbit PHYs"
 	help
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index bb86ac0bd092..e2bb2cc889ef 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -23,6 +23,7 @@
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
 #include <linux/spinlock.h>
+#include <linux/leds.h>
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/mii.h>
@@ -148,6 +149,11 @@
 #define MII_88E1510_PHY_LED_DEF		0x1177
 #define MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE	0x1040
 
+#define MII_PHY_LED45_CTRL		19
+
+#define MII_PHY_LED_CTRL_FORCE_ON	0x9
+#define MII_PHY_LED_CTRL_FORCE_OFF	0x8
+
 #define MII_M1011_PHY_STATUS		0x11
 #define MII_M1011_PHY_STATUS_1000	0x8000
 #define MII_M1011_PHY_STATUS_100	0x4000
@@ -252,6 +258,8 @@
 #define LPA_PAUSE_FIBER		0x180
 #define LPA_PAUSE_ASYM_FIBER	0x100
 
+#define MARVELL_PHY_MAX_LEDS	6
+
 #define NB_FIBER_STATS	1
 
 MODULE_DESCRIPTION("Marvell PHY driver");
@@ -271,10 +279,23 @@ static struct marvell_hw_stat marvell_hw_stats[] = {
 	{ "phy_receive_errors_fiber", 1, 21, 16},
 };
 
+struct marvell_phy_led {
+	struct led_classdev cdev;
+	u8 idx;
+	/* register value when in HW ctrl mode */
+	u8 regval;
+};
+
+#define to_marvell_phy_led(l)	container_of(l, struct marvell_phy_led, cdev)
+
 struct marvell_priv {
 	u64 stats[ARRAY_SIZE(marvell_hw_stats)];
 	char *hwmon_name;
 	struct device *hwmon_dev;
+#if IS_ENABLED(CONFIG_MARVELL_PHY_LEDS)
+	struct marvell_phy_led leds[MARVELL_PHY_MAX_LEDS];
+	u32 led_flags;
+#endif
 	bool cable_test_tdr;
 	u32 first;
 	u32 last;
@@ -662,6 +683,379 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
 	return err;
 }
 
+#if IS_ENABLED(CONFIG_MARVELL_PHY_LEDS)
+
+static struct led_hw_trigger_type marvell_led_trigger_type;
+
+enum {
+	L1V0_RECV		= BIT(0),
+	L1V0_COPPER		= BIT(1),
+	L1V5_100_FIBER		= BIT(2),
+	L1V5_100_10		= BIT(3),
+	L2V2_INIT		= BIT(4),
+	L2V2_PTP		= BIT(5),
+	L2V2_DUPLEX		= BIT(6),
+	L3V0_FIBER		= BIT(7),
+	L3V0_LOS		= BIT(8),
+	L3V5_TRANS		= BIT(9),
+	L3V7_FIBER		= BIT(10),
+	L3V7_DUPLEX		= BIT(11),
+};
+
+struct marvell_led_ctrl_info {
+	const char *name;
+	s8 regval[MARVELL_PHY_MAX_LEDS];
+	u32 flags;
+};
+
+static const struct marvell_led_ctrl_info marvell_led_ctrl_info[] = {
+	{ "link/nolink",		{ 0x0,  -1, 0x0,  -1,  -1,  -1, }, 0 },
+	{ "link/act/nolink",		{ 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, 0 },
+	{ "1000/100/10/nolink",		{ 0x2,  -1,  -1,  -1,  -1,  -1, }, 0 },
+	{ "act/noact",			{ 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, 0 },
+	{ "blink-act/noact",		{ 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, 0 },
+	{ "transmit/notransmit",	{ 0x5,  -1, 0x5,  -1, 0x5, 0x5, }, 0 },
+	{ "transmit/notransmit",	{  -1,  -1,  -1, 0x5,  -1,  -1, }, L3V5_TRANS },
+	{ "recv/norecv",		{  -1,  -1,  -1,  -1, 0x0, 0x0, }, 0 },
+	{ "recv/norecv",		{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_RECV },
+	{ "copperlink/else",		{ 0x6,  -1,  -1,  -1,  -1,  -1, }, 0 },
+	{ "copperlink/else",		{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_COPPER },
+	{ "1000/else",			{ 0x7,  -1,  -1,  -1,  -1,  -1, }, 0 },
+	{ "link/recv/nolink",		{  -1, 0x2,  -1, 0x2, 0x2, 0x2, }, 0 },
+	{ "100-fiber/else",		{  -1, 0x5,  -1,  -1,  -1,  -1, }, L1V5_100_FIBER },
+	{ "100-10/else",		{  -1, 0x5,  -1,  -1,  -1,  -1, }, L1V5_100_10 },
+	{ "1000-100/else",		{  -1, 0x6,  -1,  -1,  -1,  -1, }, 0 },
+	{ "1000-10/else",		{  -1,  -1, 0x6, 0x6,  -1,  -1, }, 0 },
+	{ "100/else",			{  -1, 0x7,  -1,  -1,  -1,  -1, }, 0 },
+	{ "10/else",			{  -1,  -1, 0x7,  -1,  -1,  -1, }, 0 },
+	{ "fiber/else",			{  -1,  -1,  -1, 0x0,  -1,  -1, }, L3V0_FIBER },
+	{ "fiber/else",			{  -1,  -1,  -1, 0x7,  -1,  -1, }, L3V7_FIBER },
+	{ "full/half",			{  -1,  -1,  -1, 0x7,  -1,  -1, }, L3V7_DUPLEX },
+	{ "full/half",			{  -1,  -1,  -1,  -1, 0x6, 0x6, }, 0 },
+	{ "full/collision/half",	{  -1,  -1,  -1,  -1, 0x7, 0x7, }, 0 },
+	{ "full/collision/half",	{  -1,  -1, 0x2,  -1,  -1,  -1, }, L2V2_DUPLEX },
+	{ "ptp",			{  -1,  -1, 0x2,  -1,  -1,  -1, }, L2V2_PTP },
+	{ "normal-init",		{  -1,  -1, 0x2,  -1,  -1,  -1, }, L2V2_INIT },
+	{ "normal-los",			{  -1,  -1,  -1, 0x0,  -1,  -1, }, L3V0_LOS },
+	{ "force-hi-z",			{ 0xa, 0xa, 0xa, 0xa, 0xa, 0xa, }, 0 },
+	{ "force-blink",		{ 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, 0 },
+};
+
+struct marvell_leds_info {
+	u32 family;
+	int nleds;
+	u32 flags;
+};
+
+#define LED(fam,n,flg)								\
+	{									\
+		.family = MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E##fam),	\
+		.nleds = (n),							\
+		.flags = (flg),							\
+	}									\
+
+static const struct marvell_leds_info marvell_leds_info[] = {
+	LED(1112,  4, L1V0_COPPER | L1V5_100_FIBER | L2V2_INIT | L3V0_LOS | L3V5_TRANS | L3V7_FIBER),
+	LED(1121R, 3, L1V5_100_10),
+	LED(1240,  6, L3V5_TRANS),
+	LED(1340S, 6, L1V0_COPPER | L1V5_100_FIBER | L2V2_PTP | L3V0_FIBER | L3V7_DUPLEX),
+	LED(1510,  3, L1V0_RECV | L1V5_100_FIBER | L2V2_DUPLEX),
+	LED(1545,  6, L1V0_COPPER | L1V5_100_FIBER | L3V0_FIBER | L3V7_DUPLEX),
+};
+
+static inline int marvell_led_reg(int led)
+{
+	switch (led) {
+	case 0 ... 3:
+		return MII_PHY_LED_CTRL;
+	case 4 ... 5:
+		return MII_PHY_LED45_CTRL;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int marvell_led_set_regval(struct phy_device *phydev, int led, u16 val)
+{
+	u16 mask;
+	int reg;
+
+	reg = marvell_led_reg(led);
+	if (reg < 0)
+		return reg;
+
+	val <<= (led % 4) * 4;
+	mask = 0xf << ((led % 4) * 4);
+
+	return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, reg, mask, val);
+}
+
+static int marvell_led_get_regval(struct phy_device *phydev, int led)
+{
+	int reg, val;
+
+	reg = marvell_led_reg(led);
+	if (reg < 0)
+		return reg;
+
+	val = phy_read_paged(phydev, MII_MARVELL_LED_PAGE, reg);
+	if (val < 0)
+		return val;
+
+	val >>= (led % 4) * 4;
+	val &= 0xf;
+
+	return val;
+}
+
+static struct led_trigger marvell_hw_led_trigger;
+
+static int _marvell_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness,
+				       bool check_trigger)
+{
+	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
+	struct marvell_phy_led *led = to_marvell_phy_led(cdev);
+	u8 val;
+
+	/* don't do anything if HW control is enabled */
+	if (check_trigger && cdev->trigger == &marvell_hw_led_trigger)
+		return 0;
+
+	val = brightness ? MII_PHY_LED_CTRL_FORCE_ON : MII_PHY_LED_CTRL_FORCE_OFF;
+
+	return marvell_led_set_regval(phydev, led->idx, val);
+}
+
+static int marvell_led_brightness_set_blocking(struct led_classdev *cdev,
+					       enum led_brightness brightness)
+{
+	return _marvell_led_brightness_set(cdev, brightness, true);
+}
+
+static int marvell_led_trigger_activate(struct led_classdev *cdev)
+{
+	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
+	struct marvell_phy_led *led = to_marvell_phy_led(cdev);
+
+	return marvell_led_set_regval(phydev, led->idx, led->regval);
+}
+
+static void marvell_led_trigger_deactivate(struct led_classdev *cdev)
+{
+	_marvell_led_brightness_set(cdev, cdev->brightness, false);
+}
+
+static ssize_t hw_control_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct marvell_phy_led *led = to_marvell_phy_led(led_trigger_get_led(dev));
+	struct phy_device *phydev = to_phy_device(led->cdev.dev->parent);
+	const struct marvell_led_ctrl_info *info;
+	struct marvell_priv *priv = phydev->priv;
+	int i, len = 0;
+
+	for (i = 0; i < ARRAY_SIZE(marvell_led_ctrl_info); ++i) {
+		bool sel;
+
+		info = &marvell_led_ctrl_info[i];
+
+		/* skip HW triggers unsupported by this LED */
+		if (info->regval[led->idx] == -1)
+			continue;
+
+		if (info->flags && !(priv->led_flags & info->flags))
+			continue;
+
+		sel = led->regval == info->regval[led->idx];
+
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%s%s%s%c", sel ? "[" : "", info->name,
+				 sel ? "]" : "",
+				 i == ARRAY_SIZE(marvell_led_ctrl_info) - 1 ? '\n' : ' ');
+	}
+
+	return len;
+}
+
+static ssize_t hw_control_store(struct device *dev, struct device_attribute *attr, const char *buf,
+				size_t count)
+{
+	struct marvell_phy_led *led = to_marvell_phy_led(led_trigger_get_led(dev));
+	struct phy_device *phydev = to_phy_device(led->cdev.dev->parent);
+	const struct marvell_led_ctrl_info *info;
+	struct marvell_priv *priv = phydev->priv;
+	int i, ret = -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(marvell_led_ctrl_info); ++i) {
+		info = &marvell_led_ctrl_info[i];
+
+		/* skip HW triggers unsupported by this LED */
+		if (info->regval[led->idx] == -1)
+			continue;
+
+		if (info->flags && !(priv->led_flags & info->flags))
+			continue;
+
+		if (sysfs_streq(buf, info->name)) {
+			ret = marvell_led_set_regval(phydev, led->idx, info->regval[led->idx]);
+			if (!ret)
+				led->regval = info->regval[led->idx];
+			break;
+		}
+	}
+
+	return ret < 0 ? ret : count;
+}
+
+static DEVICE_ATTR_RW(hw_control);
+
+static struct attribute *marvell_led_trig_attrs[] = {
+	&dev_attr_hw_control.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(marvell_led_trig);
+
+static struct led_trigger marvell_hw_led_trigger = {
+	.name		= "hw-control",
+	.activate	= marvell_led_trigger_activate,
+	.deactivate	= marvell_led_trigger_deactivate,
+	.trigger_type	= &marvell_led_trigger_type,
+	.groups		= marvell_led_trig_groups,
+};
+
+static int marvell_register_led(struct phy_device *phydev, struct device_node *np, int nleds)
+{
+	struct marvell_priv *priv = phydev->priv;
+	struct led_init_data init_data = {};
+	struct marvell_phy_led *led;
+	u32 reg, color;
+	int err;
+
+	err = of_property_read_u32(np, "reg", &reg);
+	if (err < 0)
+		return err;
+
+	/*
+	 * Maybe we should check here if reg >= nleds, where nleds is number of LEDs of this specific
+	 * PHY.
+	 */
+	if (reg >= nleds) {
+		phydev_err(phydev,
+			   "LED node %pOF 'reg' property too large (%u, PHY supports max %u)\n",
+			   np, reg, nleds - 1);
+		return -EINVAL;
+	}
+
+	led = &priv->leds[reg];
+
+	err = of_property_read_u32(np, "color", &color);
+	if (err < 0) {
+		phydev_err(phydev, "LED node %pOF does not specify color\n", np);
+		return -EINVAL;
+	}
+
+#if 0
+	/* LED_COLOR_ID_MULTI is not yet merged in Linus' tree */
+	/* TODO: Support DUAL MODE */
+	if (color == LED_COLOR_ID_MULTI) {
+		phydev_warn(phydev, "node %pOF: This driver does not yet support multicolor LEDs\n",
+			    np);
+		return -ENOTSUPP;
+	}
+#endif
+
+	init_data.fwnode = &np->fwnode;
+	init_data.devname_mandatory = true;
+	init_data.devicename = phydev->attached_dev ? netdev_name(phydev->attached_dev) : "";
+
+	if (led->cdev.max_brightness) {
+		phydev_err(phydev, "LED node %pOF 'reg' property collision with another LED\n", np);
+		return -EEXIST;
+	}
+
+	led->cdev.max_brightness = 1;
+	led->cdev.brightness_set_blocking = marvell_led_brightness_set_blocking;
+	led->cdev.trigger_type = &marvell_led_trigger_type;
+	led->idx = reg;
+	led->regval = marvell_led_get_regval(phydev, reg);
+
+	of_property_read_string(np, "linux,default-trigger", &led->cdev.default_trigger);
+
+	err = devm_led_classdev_register_ext(&phydev->mdio.dev, &led->cdev, &init_data);
+	if (err < 0) {
+		phydev_err(phydev, "Cannot register LED %pOF: %i\n", np, err);
+		return err;
+	}
+
+	return 0;
+}
+
+static void marvell_register_leds(struct phy_device *phydev)
+{
+	struct marvell_priv *priv = phydev->priv;
+	struct device_node *node = phydev->mdio.dev.of_node;
+	struct device_node *leds, *led;
+	const struct marvell_leds_info *info = NULL;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(marvell_leds_info); ++i) {
+		if (MARVELL_PHY_FAMILY_ID(phydev->phy_id) == marvell_leds_info[i].family) {
+			info = &marvell_leds_info[i];
+			break;
+		}
+	}
+
+	if (!info)
+		return;
+
+	priv->led_flags = info->flags;
+
+#if 0
+	/*
+	 * TODO: here priv->led_flags should be changed so that hw_control values
+	 * for unsupported modes won't be shown. This cannot be deduced from
+	 * family only: for example the 88E1510 family contains 88E1510 which
+	 * does not support fiber, but also 88E1512, which supports fiber.
+	 */
+	switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) {
+	}
+#endif
+
+	leds = of_get_child_by_name(node, "leds");
+	if (!leds)
+		return;
+
+	for_each_available_child_of_node(leds, led) {
+		/* Should this check if some LED registration failed? */
+		marvell_register_led(phydev, led, info->nleds);
+	}
+}
+
+static inline int marvell_leds_init(void)
+{
+	return led_trigger_register(&marvell_hw_led_trigger);
+}
+
+static inline void marvell_leds_exit(void)
+{
+	led_trigger_unregister(&marvell_hw_led_trigger);
+}
+
+#else /* !IS_ENABLED(CONFIG_MARVELL_PHY_LEDS) */
+
+static inline void marvell_register_leds(struct phy_device *phydev)
+{
+}
+
+static inline int marvell_leds_init(void)
+{
+	return 0;
+}
+
+static inline void marvell_leds_exit(void)
+{
+}
+
+#endif /* !IS_ENABLED(CONFIG_MARVELL_PHY_LEDS) */
+
 static void marvell_config_led(struct phy_device *phydev)
 {
 	u16 def_config;
@@ -692,6 +1086,8 @@ static void marvell_config_led(struct phy_device *phydev)
 			      def_config);
 	if (err < 0)
 		phydev_warn(phydev, "Fail to config marvell phy LED.\n");
+
+	marvell_register_leds(phydev);
 }
 
 static int marvell_config_init(struct phy_device *phydev)
@@ -2989,7 +3385,32 @@ static struct phy_driver marvell_drivers[] = {
 	},
 };
 
-module_phy_driver(marvell_drivers);
+static int __init phy_module_init(void)
+{
+	int ret;
+
+	ret = marvell_leds_init();
+	if (ret < 0)
+		return ret;
+
+	ret = phy_drivers_register(marvell_drivers, ARRAY_SIZE(marvell_drivers), THIS_MODULE);
+	if (ret < 0) {
+		marvell_leds_exit();
+		return ret;
+	}
+
+	return 0;
+}
+
+module_init(phy_module_init);
+
+static void __exit phy_module_exit(void)
+{
+	phy_drivers_unregister(marvell_drivers, ARRAY_SIZE(marvell_drivers));
+	marvell_leds_exit();
+}
+
+module_exit(phy_module_exit);
 
 static struct mdio_device_id __maybe_unused marvell_tbl[] = {
 	{ MARVELL_PHY_ID_88E1101, MARVELL_PHY_ID_MASK },
-- 
2.26.2

Re: [PATCH RFC leds + net-next v2 1/1] net: phy: marvell: add support for PHY LEDs via LED class

[Andrew Lunn]

On Thu, Jul 23, 2020 at 08:13:19PM +0200, Marek Behún wrote:
> This patch adds support for controlling the LEDs connected to several
> families of Marvell PHYs via Linux' LED API. These families are:
> 88E1112, 88E1121R, 88E1240, 88E1340S, 88E1510 and 88E1545. More can be
> added.
> 
> The code reads LEDs definitions from the device-tree node of the PHY.
> 
> Since the LEDs can be controlled by hardware, we add one LED-private LED
> trigger named "hw-control". This trigger is private and displayed only
> for Marvell PHY LEDs.
> 
> When this driver is activated, another sysfs file is created in that
> LEDs sysfs directory, names "hw_control". This file contains space
> separated list of possible HW controlled modes for this LED. The one
> which is selected is enclosed by square brackets. To change HW control
> mode the user has to write the name of desired mode to this "hw_control"
> file.
> 
> This patch does not yet add support for compound LED modes. This could
> be achieved via the LED multicolor framework (which is not yet in
> upstream).
> 
> Settings such as HW blink rate or pulse stretch duration are not yet
> supported, nor are LED polarity settings.

Hi Marek

I expect some of this should be moved into the phylib core. We don't
want each PHY inventing its own way to do this. The core should
provide a framework and the PHY driver fills in the gaps.

Take a look at for example mscc_main.c and its LED information. It has
pretty similar hardware to the Marvell. And microchip.c also has LED
handling, etc.

> +static int _marvell_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness,
> +				       bool check_trigger)

Please avoid _ functions. 

> +{
> +	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
> +	struct marvell_phy_led *led = to_marvell_phy_led(cdev);
> +	u8 val;
> +
> +	/* don't do anything if HW control is enabled */
> +	if (check_trigger && cdev->trigger == &marvell_hw_led_trigger)
> +		return 0;

I thought the brightness file disappeared when a trigger takes
over. So is this possible?

      Andrew

Re: [PATCH RFC leds + net-next v2 1/1] net: phy: marvell: add support for PHY LEDs via LED class

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 745 bytes --]

Hi!

> > +{
> > +	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
> > +	struct marvell_phy_led *led = to_marvell_phy_led(cdev);
> > +	u8 val;
> > +
> > +	/* don't do anything if HW control is enabled */
> > +	if (check_trigger && cdev->trigger == &marvell_hw_led_trigger)
> > +		return 0;
> 
> I thought the brightness file disappeared when a trigger takes
> over. So is this possible?

No.

When trigger is set, brightness controls "maximum" brightness LED can
have (and can turn trigger off by setting brightness to 0). Interface
is ... not quite nice.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH RFC leds + net-next v2 1/1] net: phy: marvell: add support for PHY LEDs via LED class

[Marek Behun]

On Thu, 23 Jul 2020 23:35:31 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> I thought the brightness file disappeared when a trigger takes
> over. So is this possible?
> 
>       Andrew

It does not disappear nor should it. When you have a LED with 10 levels
of brightness, you want to be able to configure with which brightness
it blinks when controlled by a trigger. SW triggers use that brightness
which was stored into the brightness file when blinking the LED.

Marek

Re: [PATCH RFC leds + net-next v2 1/1] net: phy: marvell: add support for PHY LEDs via LED class

[Marek Behun]

On Thu, 23 Jul 2020 23:35:31 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> Hi Marek
> 
> I expect some of this should be moved into the phylib core. We don't
> want each PHY inventing its own way to do this. The core should
> provide a framework and the PHY driver fills in the gaps.
> 
> Take a look at for example mscc_main.c and its LED information. It has
> pretty similar hardware to the Marvell. And microchip.c also has LED
> handling, etc.

OK, this makes sense. I will have to think about this a little.

My main issue though is whether one "hw-control" trigger should be
registered via LED API and the specific mode should be chosen via
another sysfs file as in this RFC, or whether each HW control mode
should have its own trigger. The second solution would either result in
a lot of registered triggers or complicate LED API, though...

Marek

Re: [PATCH RFC leds + net-next v2 1/1] net: phy: marvell: add support for PHY LEDs via LED class

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

Hi!

> > I expect some of this should be moved into the phylib core. We don't
> > want each PHY inventing its own way to do this. The core should
> > provide a framework and the PHY driver fills in the gaps.
> > 
> > Take a look at for example mscc_main.c and its LED information. It has
> > pretty similar hardware to the Marvell. And microchip.c also has LED
> > handling, etc.
> 
> OK, this makes sense. I will have to think about this a little.
> 
> My main issue though is whether one "hw-control" trigger should be
> registered via LED API and the specific mode should be chosen via
> another sysfs file as in this RFC, or whether each HW control mode
> should have its own trigger. The second solution would either result in
> a lot of registered triggers or complicate LED API, though...

If you register say 5 triggers.... that's okay. If you do like 1024
additional triggers (it happened before!)... well please don't.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH RFC leds + net-next v2 1/1] net: phy: marvell: add support for PHY LEDs via LED class

[Andrew Lunn]

On Fri, Jul 24, 2020 at 12:24:03PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > I expect some of this should be moved into the phylib core. We don't
> > > want each PHY inventing its own way to do this. The core should
> > > provide a framework and the PHY driver fills in the gaps.
> > > 
> > > Take a look at for example mscc_main.c and its LED information. It has
> > > pretty similar hardware to the Marvell. And microchip.c also has LED
> > > handling, etc.
> > 
> > OK, this makes sense. I will have to think about this a little.
> > 
> > My main issue though is whether one "hw-control" trigger should be
> > registered via LED API and the specific mode should be chosen via
> > another sysfs file as in this RFC, or whether each HW control mode
> > should have its own trigger. The second solution would either result in
> > a lot of registered triggers or complicate LED API, though...
> 
> If you register say 5 triggers.... that's okay. If you do like 1024
> additional triggers (it happened before!)... well please don't.

Hi Pavel

There tends to be around 15 different blink patterns per LED. And
there can be 2 to 3 LEDs per PHY. The blink patterns can be different
per PHY, or they can be the same. For the Marvell PHY we are looking
at around 45. Most of the others PHYs tend to have the same patterns
for all LEDs, so 15 triggers could be shared.

But if you then think of a 10 port Ethernet switch, there could be 450
triggers, if the triggers are not shared at all.

So to some extent, it is a question of how much effort should be put
in to sharing triggers.

   Andrew

Re: [PATCH RFC leds + net-next v2 1/1] net: phy: marvell: add support for PHY LEDs via LED class

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1367 bytes --]

Hi!

> > > My main issue though is whether one "hw-control" trigger should be
> > > registered via LED API and the specific mode should be chosen via
> > > another sysfs file as in this RFC, or whether each HW control mode
> > > should have its own trigger. The second solution would either result in
> > > a lot of registered triggers or complicate LED API, though...
> > 
> > If you register say 5 triggers.... that's okay. If you do like 1024
> > additional triggers (it happened before!)... well please don't.
> 
> Hi Pavel
> 
> There tends to be around 15 different blink patterns per LED. And
> there can be 2 to 3 LEDs per PHY. The blink patterns can be different
> per PHY, or they can be the same. For the Marvell PHY we are looking
> at around 45. Most of the others PHYs tend to have the same patterns
> for all LEDs, so 15 triggers could be shared.
> 
> But if you then think of a 10 port Ethernet switch, there could be 450
> triggers, if the triggers are not shared at all.
> 
> So to some extent, it is a question of how much effort should be put
> in to sharing triggers.

It sounds to me ... lot of effort should be put to sharing triggers
there :-).

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH RFC leds + net-next v2 0/1] Add support for LEDs on Marvell PHYs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2541 bytes --]

On Thu 2020-07-23 20:13:18, Marek Behún wrote:
> Hi,
> 
> this is v2 of my RFC adding support for LEDs connected to Marvell PHYs.
> 
> The LED subsystem patches are not contained:
> - the patch adding support for LED private triggers is already accepted
>   in Pavel Machek's for-next tree.
>   If you want to try this patch on top of net-next, please also apply
>   https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next&id=93690cdf3060c61dfce813121d0bfc055e7fa30d
> - the other led-trigger patch is not needed in this version of the RFC
> 
> The main difference from v1 is that only one trigger, named
> "hw-control", is added to the /sys/class/leds/<LED>/trigger file.
> 
> When this trigger is activated, another file called "hw_control" is
> created in the /sys/class/leds/<LED>/ directory. This file lists
> available HW control modes for this LED in the same way the trigger
> file does for triggers.
> 
> Example:
> 
>   # cd /sys/class/leds/eth0\:green\:link/
>   # cat trigger
>   [none] hw-control timer oneshot heartbeat ...
>   # echo hw-control >trigger

Make this "hw-phy-mode" or something. hw-control is a bit too generic.

>   # cat trigger
>   none [hw-control] timer oneshot heartbeat ...
>   # cat hw_control
>   link/nolink link/act/nolink 1000/100/10/nolink act/noact
>   blink-act/noact transmit/notransmit copperlink/else [1000/else]
>   force-hi-z force-blink
>   # echo 1000/100/10/nolink >hw_control
>   # cat hw_control
>   link/nolink link/act/nolink [1000/100/10/nolink] act/noact
>   blink-act/noact transmit/notransmit copperlink/else 1000/else
>   force-hi-z force-blink
> 
> The benefit here is that only one trigger is registered via LED API.
> I guess there are other PHY drivers which too support HW controlled
> blinking modes. So of this way of controlling PHY LED HW controlled
> modes is accepted, the code creating the hw-control trigger and
> hw_control file should be made into library code so that it can be
> reused.
> 
> What do you think?

So.. you have 10 of them right now. I guess both hw_control and making
it into the trigger directly is acceptable here.

In future, would you expect having software "1000/100/10/nolink"
triggers I could activate on my scrollock LED (or on GPIO controlled
LEDs) to indicate network activity?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH RFC leds + net-next v2 0/1] Add support for LEDs on Marvell PHYs

[Marek Behún]

[-- Attachment #1: Type: text/plain, Size: 1569 bytes --]

On Fri, 24 Jul 2020 12:29:01 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> In future, would you expect having software "1000/100/10/nolink"
> triggers I could activate on my scrollock LED (or on GPIO controlled
> LEDs) to indicate network activity?

Look at drivers/net/phy/phy_led_triggers.c, something like that could
be actually implemented there.

Some of the modes are useful, like the "1000/100/10/nolink". But some
of them are pretty weird, and I don't think anyone actually uses it
("1000-10/else", which is on if the device is linked at 1000mbps ar
10mbps, and else off? who would sacrifies a LED for this?).

I actually wanted to talk about the phy_led_triggers.c code. It
registers several trigger for each PHY, with the name in form:
  phy-device-name:mode
where
  phy-device-name is derived from OF
    - sometimes it is in the form
      d0032004.mdio-mii:01
    - but sometimes in the form of whole OF path followed by ":" and
      the PHY address:
      /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08
  mode is "link", "1Gbps", "100Mbps", "10Mbps" and so on"

So I have a GPIO LED, and I can set it to sw trigger so that it is on
when a specific PHY is linked on 1Gbps.

The problem is that on Turris Mox I can connect up to three 8-port
switches, which yields in 25 network PHYs overall. So reading the
trigger file results in 4290 bytes (look at attachment cat_trigger.txt).
I think the phy_led_triggers should have gone this way of having just
one trigger (like netdev has), and specifying phy device via and mode
via another file.

Marek


[-- Attachment #2: cat_trigger.txt --]
[-- Type: text/plain, Size: 4298 bytes --]

none timer oneshot heartbeat default-on [mmc0] mmc1 /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:01:link /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:01:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:01:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:01:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:02:link /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:02:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:02:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:02:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:03:link /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:03:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:03:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:03:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:04:link /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:04:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch
 1@11/mdio:04:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:04:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:05:link /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:05:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:05:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:05:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:06:link /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:06:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:06:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:06:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:07:link /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:07:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:07:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:07:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:08:link /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:08:1Gbps /soc/inter
 nal-regs@d0000000/mdio@32004/switch1@11/mdio:08:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:08:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:01:link /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:01:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:01:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:01:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:02:link /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:02:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:02:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:02:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:03:link /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:03:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:03:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:03:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:04:link /soc/internal-regs@d0000000/mdio@32004/
 switch0@10/mdio:04:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:04:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:04:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:05:link /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:05:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:05:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:05:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:06:link /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:06:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:06:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:06:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:07:link /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:07:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:07:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:07:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08:link /soc
 /internal-regs@d0000000/mdio@32004/switch0@10/mdio:08:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08:10Mbps d0032004.mdio-mii:01:link d0032004.mdio-mii:01:1Gbps d0032004.mdio-mii:01:100Mbps d0032004.mdio-mii:01:10Mbps

Re: [PATCH RFC leds + net-next v2 0/1] Add support for LEDs on Marvell PHYs

[Marek Behún]

On Fri, 24 Jul 2020 15:12:33 +0200
Marek Behún <marek.behun@nic.cz> wrote:

> On Fri, 24 Jul 2020 12:29:01 +0200
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > In future, would you expect having software "1000/100/10/nolink"
> > triggers I could activate on my scrollock LED (or on GPIO controlled
> > LEDs) to indicate network activity?  
> 
> Look at drivers/net/phy/phy_led_triggers.c, something like that could
> be actually implemented there.
> 
> Some of the modes are useful, like the "1000/100/10/nolink". But some
> of them are pretty weird, and I don't think anyone actually uses it
> ("1000-10/else", which is on if the device is linked at 1000mbps ar
> 10mbps, and else off? who would sacrifies a LED for this?).
> 
> I actually wanted to talk about the phy_led_triggers.c code. It
> registers several trigger for each PHY, with the name in form:
>   phy-device-name:mode
> where
>   phy-device-name is derived from OF
>     - sometimes it is in the form
>       d0032004.mdio-mii:01
>     - but sometimes in the form of whole OF path followed by ":" and
>       the PHY address:
>       /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08
>   mode is "link", "1Gbps", "100Mbps", "10Mbps" and so on"
> 
> So I have a GPIO LED, and I can set it to sw trigger so that it is on
> when a specific PHY is linked on 1Gbps.
> 
> The problem is that on Turris Mox I can connect up to three 8-port
> switches, which yields in 25 network PHYs overall. So reading the
> trigger file results in 4290 bytes (look at attachment
> cat_trigger.txt). I think the phy_led_triggers should have gone this
> way of having just one trigger (like netdev has), and specifying phy
> device via and mode via another file.
> 
> Marek
> 

In fact I think the way the phy_led_triggers does this should be
deprecated. I new kernel config options should be create, something
like "new user API for PHY LED trigger", which would create just one
trigger, with name phydev, like we have netdev, and like in the netdev
trigger, the mode and device should be configured via other files.

This phydev trigger could then be made similar to phy-hw-mode trigger...

Marek

Re: [PATCH RFC leds + net-next v2 0/1] Add support for LEDs on Marvell PHYs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]

On Fri 2020-07-24 15:12:33, Marek Behún wrote:
> On Fri, 24 Jul 2020 12:29:01 +0200
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > In future, would you expect having software "1000/100/10/nolink"
> > triggers I could activate on my scrollock LED (or on GPIO controlled
> > LEDs) to indicate network activity?
> 
> Look at drivers/net/phy/phy_led_triggers.c, something like that could
> be actually implemented there.
> 
> Some of the modes are useful, like the "1000/100/10/nolink". But some
> of them are pretty weird, and I don't think anyone actually uses it
> ("1000-10/else", which is on if the device is linked at 1000mbps ar
> 10mbps, and else off? who would sacrifies a LED for this?).
> 
> I actually wanted to talk about the phy_led_triggers.c code. It
> registers several trigger for each PHY, with the name in form:
>   phy-device-name:mode
> where
>   phy-device-name is derived from OF
>     - sometimes it is in the form
>       d0032004.mdio-mii:01
>     - but sometimes in the form of whole OF path followed by ":" and
>       the PHY address:
>       /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08
>   mode is "link", "1Gbps", "100Mbps", "10Mbps" and so on"
> 
> So I have a GPIO LED, and I can set it to sw trigger so that it is on
> when a specific PHY is linked on 1Gbps.
> 
> The problem is that on Turris Mox I can connect up to three 8-port
> switches, which yields in 25 network PHYs overall. So reading the
> trigger file results in 4290 bytes (look at attachment cat_trigger.txt).
> I think the phy_led_triggers should have gone this way of having just
> one trigger (like netdev has), and specifying phy device via and mode
> via another file.

I agree with you. This is ... not pretty ... and it would be nice to
get it fixed.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]