[RFC PATCH v2 0/5] Adds support for PHY LEDs with offload triggers

[RFC PATCH v2 0/5] Adds support for PHY LEDs with offload triggers

[Ansuel Smith]

This is another attempt in adding support for PHY LEDs. Most of the
times Switch/PHY have connected multiple LEDs that are controlled by HW
based on some rules/event. Currently we lack any support for a generic
way to control the HW part and normally we either never implement the
feature or only add control for brightness or hw blink.

This is based on Marek idea of providing some API to cled but use a
different implementation that in theory should be more generilized.

The current idea is:
- LED driver implement 2 API (trigger and configure). They are used to
  put the LED in offload mode and to configure the various trigger.
- We have offload triggers that are used to expose to userspace the
  supported offload triggers and set the offload mode on trigger
  activation.
- The LED driver will declare each supported trigger and each supported
  offloaded trigger and communicate it to the trigger that will expose them
  via sysfs.
- Only supported triggers from linux,supported-offload-triggers will be
  exposed by the offload trigger to userspace via sysfs. Other won't be
  configurable as they are not supported by the LED driver.
- The LED driver will set how the Offload mode is activated/disabled and
  will set a configure function to set each supported offload triggers.
  This function provide 3 different mode enable/disable/read that
  will enable or disable the offload trigger or read the current status.
- On offload trigger activation, only the offload mode is triggered but
  the offload triggers are not configured. This means that the LED will
  run in offload mode but will have the default rules/event set by the
  device by default. To change this the user will have to operate via
  userspace (or we can consider adding another binding in the dts to
  declare also a default trigger configuration)

Each LED driver will have to declare explicit support for the offload
trigger (or return not supported error code) as we pass a u32 flag that
the LED driver will elaborate and understand what is referring to (based
on the current active trigger).

I posted a user for this new implementation that will benefit from this
and will add a big feature to it. Currently qca8k can have up to 3 LEDs
connected to each PHY port and we have some device that have only one of
them connected and the default configuration won't work for that.

v2:
- Fix spelling mistake (sorry)
- Drop patch 02 "permit to declare supported offload triggers".
  Change the logic, now the LED driver declare support for them
  using the configure_offload with the cmd TRIGGER_SUPPORTED.
- Rework code to follow this new implementation.
- Update Documentation to better describe how this offload
  implementation work.

Ansuel Smith (4):
  leds: add function to configure offload leds
  leds: trigger: add offload-phy-activity trigger
  net: dsa: qca8k: add LEDs support
  dt-bindings: net: dsa: qca8k: add LEDs definition example

Marek Behún (1):
  leds: trigger: add API for HW offloading of triggers

 .../devicetree/bindings/net/dsa/qca8k.yaml    |  20 +
 Documentation/leds/leds-class.rst             |  44 +++
 drivers/leds/led-triggers.c                   |   1 +
 drivers/leds/trigger/Kconfig                  |  39 ++
 drivers/leds/trigger/Makefile                 |   1 +
 .../trigger/ledtrig-offload-phy-activity.c    | 145 +++++++
 drivers/net/dsa/Kconfig                       |   9 +
 drivers/net/dsa/Makefile                      |   1 +
 drivers/net/dsa/qca8k-leds.c                  | 364 ++++++++++++++++++
 drivers/net/dsa/qca8k.c                       |   8 +-
 drivers/net/dsa/qca8k.h                       |  64 +++
 include/linux/leds.h                          |  85 ++++
 12 files changed, 779 insertions(+), 2 deletions(-)
 create mode 100644 drivers/leds/trigger/ledtrig-offload-phy-activity.c
 create mode 100644 drivers/net/dsa/qca8k-leds.c

-- 
2.32.0

[RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Ansuel Smith]

From: Marek Behún <kabel@kernel.org>

Add method trigger_offload() and member variable `offloaded` to struct
led_classdev. Add helper functions led_trigger_offload() and
led_trigger_offload_stop().

The trigger_offload() method, when implemented by the LED driver, should
be called (via led_trigger_offload() function) from trigger code wanting
to be offloaded at the moment when configuration of the trigger changes.

If the trigger is successfully offloaded, this method returns 0 and the
trigger does not have to blink the LED in software.

If the trigger with given configuration cannot be offloaded, the method
should return -EOPNOTSUPP, in which case the trigger must blink the LED
in SW.

The second argument to trigger_offload() being false means that the
offloading is being disabled. In this case the function must return 0,
errors are not permitted.

An additional config CONFIG_LEDS_OFFLOAD_TRIGGERS is added to add support
for these special trigger offload driven.

Signed-off-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 Documentation/leds/leds-class.rst | 20 +++++++++++++++++++
 drivers/leds/led-triggers.c       |  1 +
 drivers/leds/trigger/Kconfig      | 10 ++++++++++
 include/linux/leds.h              | 33 +++++++++++++++++++++++++++++++
 4 files changed, 64 insertions(+)

diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index cd155ead8703..5bf6e5d471ce 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -169,6 +169,26 @@ Setting the brightness to zero with brightness_set() callback function
 should completely turn off the LED and cancel the previously programmed
 hardware blinking function, if any.
 
+Hardware offloading of LED triggers
+===================================
+
+Some LEDs can be driven by hardware triggers (for example a LED connected to
+an ethernet PHY or an ethernet switch can be configured to blink on activity on
+the network, which in software is done by the netdev trigger).
+
+To do such offloading, LED driver must support this and a deficated offload
+trigger must be used. The LED must implement the trigger_offload() method and
+the trigger code must try to call this method (via led_trigger_offload()
+function) to set the LED to this particular mode. (and disable any software
+blinking)
+
+The implementation of the trigger_offload() method by the LED driver must return
+0 if the offload is successful and -EOPNOTSUPP if the requested trigger
+configuration is not supported.
+
+If the second argument (enable) to the trigger_offload() method is false, any
+active HW offloading must be deactivated. In this case errors are not permitted
+in the trigger_offload() method and the driver will be set to the new trigger.
 
 Known Issues
 ============
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 072491d3e17b..281c52914f42 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -179,6 +179,7 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 
 		cancel_work_sync(&led_cdev->set_brightness_work);
 		led_stop_software_blink(led_cdev);
+		led_trigger_offload_stop(led_cdev);
 		if (led_cdev->trigger->deactivate)
 			led_cdev->trigger->deactivate(led_cdev);
 		device_remove_groups(led_cdev->dev, led_cdev->trigger->groups);
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index dc6816d36d06..33aba8defeab 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -9,6 +9,16 @@ menuconfig LEDS_TRIGGERS
 
 if LEDS_TRIGGERS
 
+config LEDS_OFFLOAD_TRIGGERS
+	bool "LED Offload Trigger support"
+	help
+	  This option enabled offload triggers support used by leds that
+	  can be driven in hardware by declaring some specific triggers.
+	  A offload trigger will expose a sysfs dir to configure the
+	  different blinking trigger and the available hardware triggers.
+
+	  If unsure, say Y.
+
 config LEDS_TRIGGER_TIMER
 	tristate "LED Timer Trigger"
 	help
diff --git a/include/linux/leds.h b/include/linux/leds.h
index ba4861ec73d3..949ab461287f 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -154,6 +154,13 @@ struct led_classdev {
 
 	/* LEDs that have private triggers have this set */
 	struct led_hw_trigger_type	*trigger_type;
+
+#ifdef CONFIG_LEDS_OFFLOAD_TRIGGERS
+	int			offloaded;
+	/* some LEDs cne be driven by HW */
+	int			(*trigger_offload)(struct led_classdev *led_cdev,
+						   bool enable);
+#endif
 #endif
 
 #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
@@ -409,6 +416,32 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
 	return led_cdev->trigger_data;
 }
 
+#ifdef CONFIG_LEDS_OFFLOAD_TRIGGERS
+static inline int led_trigger_offload(struct led_classdev *led_cdev)
+{
+	int ret;
+
+	if (!led_cdev->trigger_offload)
+		return -EOPNOTSUPP;
+
+	ret = led_cdev->trigger_offload(led_cdev, true);
+	led_cdev->offloaded = !ret;
+
+	return ret;
+}
+
+static inline void led_trigger_offload_stop(struct led_classdev *led_cdev)
+{
+	if (!led_cdev->trigger_offload)
+		return;
+
+	if (led_cdev->offloaded) {
+		led_cdev->trigger_offload(led_cdev, false);
+		led_cdev->offloaded = false;
+	}
+}
+#endif
+
 /**
  * led_trigger_rename_static - rename a trigger
  * @name: the new trigger name
-- 
2.32.0

[RFC PATCH v2 2/5] leds: add function to configure offload leds

[Ansuel Smith]

Add configure_offload helper to configure how the LED should work in
offload mode. The function require to support the particular trigger and
will use the passed offload_flags to elaborate the data and apply the
correct configuration. This function will then be used by the offload
trigger to request and update offload configuration.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 Documentation/leds/leds-class.rst | 24 ++++++++++++++++++++++++
 include/linux/leds.h              | 28 ++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index 5bf6e5d471ce..0a3bbe71dac7 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -190,6 +190,30 @@ If the second argument (enable) to the trigger_offload() method is false, any
 active HW offloading must be deactivated. In this case errors are not permitted
 in the trigger_offload() method and the driver will be set to the new trigger.
 
+The offload trigger will use the function configure_offload() provided by the driver
+that will configure the offloaded mode for the LED.
+This function passes as the first argument (offload_flags) a u32 flag.
+The second argument (cmd) of the configure_offload() method can be used to do various
+operations for the specific trigger. We currently support ENABLE, DISABLE, READ and
+SUPPORTED to enable, disable, read the state of the offload trigger and ask the LED
+driver supports the specific offload trigger.
+
+In ENABLE/DISABLE configure_offload() should configure the LED to enable/disable the
+requested trigger (flags).
+In READ configure_offload() should return 0 or 1 based on the status of the requested
+trigger (flags).
+In SUPPORTED configure_offload() should return 0 or 1 if the LED driver supports the
+requested trigger (flags) or not.
+
+The u32 flag is specific to the trigger and change across them. It's in the LED
+driver interest know how to elaborate this flag and to declare support for a
+particular offload trigger. For this exact reason explicit support for the specific
+trigger is mandatory or the driver returns -EOPNOTSUPP if asked to enter offload mode
+with a not supported trigger.
+If the driver returns -EOPNOTSUPP on configure_offload(), the trigger activation will
+fail as the driver doesn't support that specific offload trigger or doesn't know
+how to handle the provided flags.
+
 Known Issues
 ============
 
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 949ab461287f..2a1e60e4d73e 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -67,6 +67,15 @@ struct led_hw_trigger_type {
 	int dummy;
 };
 
+#ifdef CONFIG_LEDS_OFFLOAD_TRIGGERS
+enum offload_trigger_cmd {
+	TRIGGER_ENABLE, /* Enable the offload trigger */
+	TRIGGER_DISABLE, /* Disable the offload trigger */
+	TRIGGER_READ, /* Read the status of the offload trigger */
+	TRIGGER_SUPPORTED, /* Ask the driver if the trigger is supported */
+};
+#endif
+
 struct led_classdev {
 	const char		*name;
 	unsigned int brightness;
@@ -160,6 +169,14 @@ struct led_classdev {
 	/* some LEDs cne be driven by HW */
 	int			(*trigger_offload)(struct led_classdev *led_cdev,
 						   bool enable);
+	/* Function to configure how the LEDs should work in offload mode.
+	 * The function require to support the trigger and will use the
+	 * passed flags to elaborate the trigger requested and apply the
+	 * correct configuration.
+	 */
+	int			(*configure_offload)(struct led_classdev *led_cdev,
+						     u32 offload_flags,
+						     enum offload_trigger_cmd cmd);
 #endif
 #endif
 
@@ -417,6 +434,17 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
 }
 
 #ifdef CONFIG_LEDS_OFFLOAD_TRIGGERS
+bool led_trigger_offload_is_supported(struct led_classdev *led_cdev, u32 flags)
+{
+	int ret;
+
+	ret = led_cdev->configure_offload(led_cdev, flags, TRIGGER_SUPPORTED);
+	if (ret > 0)
+		return true;
+
+	return false;
+}
+
 static inline int led_trigger_offload(struct led_classdev *led_cdev)
 {
 	int ret;
-- 
2.32.0

[RFC PATCH v2 3/5] leds: trigger: add offload-phy-activity trigger

[Ansuel Smith]

Add Offload Trigger for PHY Activity. This special trigger is used to
configure and expose the different HW trigger that are provided by the
PHY. Each offload trigger can be configured by sysfs and on trigger
activation the offload mode is enabled.

This currently implement these hw triggers:
  - blink_tx: Blink LED on tx packet receive
  - blink_rx: Blink LED on rx packet receive
  - blink_collision: Blink LED on collision detection
  - link_10m: Keep LED on with 10m link speed
  - link_100m: Keep LED on with 100m link speed
  - link_1000m: Keep LED on with 1000m link speed
  - half_duplex: Keep LED on with half duplex link
  - full_duplex: Keep LED on with full duplex link
  - linkup_over: Keep LED on with link speed and blink on rx/tx traffic
  - power_on_reset: Keep LED on with switch reset
  - blink_2hz: Set blink speed at 2hz for every blink event
  - blink_4hz: Set blink speed at 4hz for every blink event
  - blink_8hz: Set blink speed at 8hz for every blink event
  - blink_auto: Set blink speed at 2hz for 10m link speed,
      4hz for 100m and 8hz for 1000m

The trigger will check if the LED driver support the offload trigger and
will expose the offload triggers in sysfs.
It will finally enable hw mode for the LED without configuring any rule.
This mean that the LED will blink/follow whatever offload trigger is
active by default and the user needs to manually configure the desired
offload triggers using sysfs.
A flag is passed to configure_offload with the related rule from this
trigger to active or disable.
It's in the led driver interest the detection and knowing how to
elaborate the passed flags and should report -EOPNOTSUPP otherwise.

The different hw triggers are exposed in the led sysfs dir under the
offload-phy-activity subdir.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/leds/trigger/Kconfig                  |  29 ++++
 drivers/leds/trigger/Makefile                 |   1 +
 .../trigger/ledtrig-offload-phy-activity.c    | 145 ++++++++++++++++++
 include/linux/leds.h                          |  24 +++
 4 files changed, 199 insertions(+)
 create mode 100644 drivers/leds/trigger/ledtrig-offload-phy-activity.c

diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 33aba8defeab..14023e160ba1 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -164,4 +164,33 @@ config LEDS_TRIGGER_TTY
 
 	  When build as a module this driver will be called ledtrig-tty.
 
+config LEDS_OFFLOAD_TRIGGER_PHY_ACTIVITY
+	tristate "LED Offload Trigger for PHY Activity"
+	depends on LEDS_OFFLOAD_TRIGGERS
+	help
+	  This allows LEDs to be configured to run by hardware and offloaded
+	  based on some rules. The LED will blink or be on based on the PHY
+	  Activity for example on packet receive or based on the link speed.
+
+	  The current supported offload triggers are:
+	  - blink_tx: Blink LED on tx packet receive
+	  - blink_rx: Blink LED on rx packet receive
+	  - blink_collision: Blink LED on collision detection
+	  - keep_link_10m: Keep LED on with 10m link speed
+	  - keep_link_100m: Keep LED on with 100m link speed
+	  - keep_link_1000m: Keep LED on with 1000m link speed
+	  - keep_half_duplex: Keep LED on with half duplex link
+	  - keep_full_duplex: Keep LED on with full duplex link
+	  - option_linkup_over: Blink rules are ignored with absent link
+	  - option_power_on_reset: Power ON Led on Switch/PHY reset
+	  - option_blink_2hz: Set blink speed at 2hz for every blink event
+	  - option_blink_4hz: Set blink speed at 4hz for every blink event
+	  - option_blink_8hz: Set blink speed at 8hz for every blink event
+	  - option_blink_auto: Set blink speed at 2hz for 10m link speed,
+	                       4hz for 100m and 8hz for 1000m
+
+	  These offload triggers are present in the LED sysfs dir under
+	  offload-phy-activity if supported by the LED driver.
+
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 25c4db97cdd4..392ca8568588 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
 obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
 obj-$(CONFIG_LEDS_TRIGGER_AUDIO)	+= ledtrig-audio.o
 obj-$(CONFIG_LEDS_TRIGGER_TTY)		+= ledtrig-tty.o
+obj-$(CONFIG_LEDS_OFFLOAD_TRIGGER_PHY_ACTIVITY) += ledtrig-offload-phy-activity.o
diff --git a/drivers/leds/trigger/ledtrig-offload-phy-activity.c b/drivers/leds/trigger/ledtrig-offload-phy-activity.c
new file mode 100644
index 000000000000..4fadd7f31d1a
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-offload-phy-activity.c
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+#include <linux/slab.h>
+
+#define PHY_ACTIVITY_MAX_TRIGGERS 14
+
+#define DEFINE_OFFLOAD_TRIGGER(trigger_name, trigger) \
+	static ssize_t trigger_name##_show(struct device *dev, \
+				struct device_attribute *attr, char *buf) \
+	{ \
+		struct led_classdev *led_cdev = led_trigger_get_led(dev); \
+		int val; \
+		val = led_cdev->configure_offload(led_cdev, trigger, TRIGGER_READ); \
+		return sprintf(buf, "%d\n", val ? 1 : 0); \
+	} \
+	static ssize_t trigger_name##_store(struct device *dev, \
+					struct device_attribute *attr, \
+					const char *buf, size_t size) \
+	{ \
+		struct led_classdev *led_cdev = led_trigger_get_led(dev); \
+		unsigned long state; \
+		int cmd, ret; \
+		ret = kstrtoul(buf, 0, &state); \
+		if (ret) \
+			return ret; \
+		cmd = !!state ? TRIGGER_ENABLE : TRIGGER_DISABLE; \
+		/* Update the configuration with every change */ \
+		led_cdev->configure_offload(led_cdev, trigger, cmd); \
+		return size; \
+	} \
+	DEVICE_ATTR_RW(trigger_name)
+
+/* Expose sysfs for every blink to be configurable from userspace */
+DEFINE_OFFLOAD_TRIGGER(blink_tx, BLINK_TX);
+DEFINE_OFFLOAD_TRIGGER(blink_rx, BLINK_RX);
+DEFINE_OFFLOAD_TRIGGER(blink_collision, BLINK_COLLISION);
+DEFINE_OFFLOAD_TRIGGER(keep_link_10m, KEEP_LINK_10M);
+DEFINE_OFFLOAD_TRIGGER(keep_link_100m, KEEP_LINK_100M);
+DEFINE_OFFLOAD_TRIGGER(keep_link_1000m, KEEP_LINK_1000M);
+DEFINE_OFFLOAD_TRIGGER(keep_half_duplex, KEEP_HALF_DUPLEX);
+DEFINE_OFFLOAD_TRIGGER(keep_full_duplex, KEEP_FULL_DUPLEX);
+DEFINE_OFFLOAD_TRIGGER(option_linkup_over, OPTION_LINKUP_OVER);
+DEFINE_OFFLOAD_TRIGGER(option_power_on_reset, OPTION_POWER_ON_RESET);
+DEFINE_OFFLOAD_TRIGGER(option_blink_2hz, OPTION_BLINK_2HZ);
+DEFINE_OFFLOAD_TRIGGER(option_blink_4hz, OPTION_BLINK_4HZ);
+DEFINE_OFFLOAD_TRIGGER(option_blink_8hz, OPTION_BLINK_8HZ);
+DEFINE_OFFLOAD_TRIGGER(option_blink_auto, OPTION_BLINK_AUTO);
+
+/* The attrs will be placed dynamically based on the supported triggers */
+static struct attribute *phy_activity_attrs[PHY_ACTIVITY_MAX_TRIGGERS + 1];
+
+static int offload_phy_activity_activate(struct led_classdev *led_cdev)
+{
+	int i, trigger, ret;
+
+	/* This triggered is supported only by LEDs that can be offload driven */
+	if (!led_cdev->trigger_offload || !led_cdev->trigger_offload)
+		return -EOPNOTSUPP;
+
+	/* Scan the supported offload triggers and expose them in sysfs if supported */
+	for (trigger = 0, i = 0; trigger < PHY_ACTIVITY_MAX_TRIGGERS; trigger++) {
+		if (led_trigger_offload_is_supported(led_cdev, BLINK_TX))
+			phy_activity_attrs[i++] = &dev_attr_blink_tx.attr;
+
+		if (led_trigger_offload_is_supported(led_cdev, BLINK_RX))
+			phy_activity_attrs[i++] = &dev_attr_blink_rx.attr;
+
+		if (led_trigger_offload_is_supported(led_cdev, BLINK_COLLISION))
+			phy_activity_attrs[i++] = &dev_attr_blink_collision.attr;
+
+		if (led_trigger_offload_is_supported(led_cdev, KEEP_LINK_10M))
+			phy_activity_attrs[i++] = &dev_attr_keep_link_10m.attr;
+
+		if (led_trigger_offload_is_supported(led_cdev, KEEP_LINK_100M))
+			phy_activity_attrs[i++] = &dev_attr_keep_link_100m.attr;
+
+		if (led_trigger_offload_is_supported(led_cdev, KEEP_LINK_1000M))
+			phy_activity_attrs[i++] = &dev_attr_keep_link_1000m.attr;
+
+		if (led_trigger_offload_is_supported(led_cdev, KEEP_HALF_DUPLEX))
+			phy_activity_attrs[i++] = &dev_attr_keep_half_duplex.attr;
+
+		if (led_trigger_offload_is_supported(led_cdev, KEEP_FULL_DUPLEX))
+			phy_activity_attrs[i++] = &dev_attr_keep_full_duplex.attr;
+
+		if (led_trigger_offload_is_supported(led_cdev, OPTION_LINKUP_OVER))
+			phy_activity_attrs[i++] = &dev_attr_option_linkup_over.attr;
+
+		if (led_trigger_offload_is_supported(led_cdev, OPTION_POWER_ON_RESET))
+			phy_activity_attrs[i++] = &dev_attr_option_power_on_reset.attr;
+
+		if (led_trigger_offload_is_supported(led_cdev, OPTION_BLINK_2HZ))
+			phy_activity_attrs[i++] = &dev_attr_option_blink_2hz.attr;
+
+		if (led_trigger_offload_is_supported(led_cdev, OPTION_BLINK_4HZ))
+			phy_activity_attrs[i++] = &dev_attr_option_blink_4hz.attr;
+
+		if (led_trigger_offload_is_supported(led_cdev, OPTION_BLINK_8HZ))
+			phy_activity_attrs[i++] = &dev_attr_option_blink_8hz.attr;
+
+		if (led_trigger_offload_is_supported(led_cdev, OPTION_BLINK_AUTO))
+			phy_activity_attrs[i++] = &dev_attr_option_blink_auto.attr;
+	}
+
+	/* Enable offload mode. No custom configuration is applied,
+	 * the LED driver will use whatever default configuration is
+	 * currently configured.
+	 */
+	ret = led_cdev->trigger_offload(led_cdev, true);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void offload_phy_activity_deactivate(struct led_classdev *led_cdev)
+{
+	led_cdev->trigger_offload(led_cdev, false);
+}
+
+static const struct attribute_group phy_activity_group = {
+	.name = "offload-phy-activity",
+	.attrs = phy_activity_attrs,
+};
+
+static const struct attribute_group *phy_activity_groups[] = {
+	&phy_activity_group,
+	NULL,
+};
+
+static struct led_trigger offload_phy_activity_trigger = {
+	.name       = "offload-phy-activity",
+	.activate   = offload_phy_activity_activate,
+	.deactivate = offload_phy_activity_deactivate,
+	.groups     = phy_activity_groups,
+};
+
+static int __init offload_phy_activity_init(void)
+{
+	return led_trigger_register(&offload_phy_activity_trigger);
+}
+device_initcall(offload_phy_activity_init);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 2a1e60e4d73e..fea06ca65da8 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -546,6 +546,30 @@ static inline void ledtrig_flash_ctrl(bool on) {}
 static inline void ledtrig_torch_ctrl(bool on) {}
 #endif
 
+#ifdef CONFIG_LEDS_OFFLOAD_TRIGGER_PHY_ACTIVITY
+/* 3 main category for offload trigger:
+ * - blink: the led will blink on trigger
+ * - keep: the led will be kept on with condition
+ * - option: a configuration value internal to the led on how offload works
+ */
+enum offload_phy_activity {
+	BLINK_TX = BIT(0), /* Blink on TX traffic */
+	BLINK_RX = BIT(1), /* Blink on RX traffic */
+	BLINK_COLLISION = BIT(2), /* Blink on Collision packet */
+	KEEP_LINK_10M = BIT(3), /* LED on with 10M link */
+	KEEP_LINK_100M = BIT(4), /* LED on with 100M link */
+	KEEP_LINK_1000M = BIT(5), /* LED on with 1000M link */
+	KEEP_HALF_DUPLEX = BIT(6), /* LED on with Half-Duplex link */
+	KEEP_FULL_DUPLEX = BIT(7), /* LED on with Full-Duplex link */
+	OPTION_LINKUP_OVER = BIT(8), /* LED will be on with KEEP offload_triggers and blink on BLINK traffic */
+	OPTION_POWER_ON_RESET = BIT(9), /* LED will be on Switch reset */
+	OPTION_BLINK_2HZ = BIT(10), /* LED will blink with any offload_trigger at 2Hz */
+	OPTION_BLINK_4HZ = BIT(11), /* LED will blink with any offload_trigger at 4Hz */
+	OPTION_BLINK_8HZ = BIT(12), /* LED will blink with any offload_trigger at 8Hz */
+	OPTION_BLINK_AUTO = BIT(13), /* LED will blink differently based on the Link Speed */
+};
+#endif
+
 /*
  * Generic LED platform data for describing LED names and default triggers.
  */
-- 
2.32.0

[RFC PATCH v2 4/5] net: dsa: qca8k: add LEDs support

[Ansuel Smith]

Add LEDs support for qca8k Switch Family. This will provide the LEDs
offload API to permit the PHY LED to be driven offloaded.
Each port have at least 3 LEDs and they can HW blink, set on/off or
set to be driven by some rules applied by the offload trigger.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/Kconfig      |   9 +
 drivers/net/dsa/Makefile     |   1 +
 drivers/net/dsa/qca8k-leds.c | 364 +++++++++++++++++++++++++++++++++++
 drivers/net/dsa/qca8k.c      |   8 +-
 drivers/net/dsa/qca8k.h      |  64 ++++++
 5 files changed, 444 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/dsa/qca8k-leds.c

diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index 7b1457a6e327..89e36f3a8195 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -67,6 +67,15 @@ config NET_DSA_QCA8K
 	  This enables support for the Qualcomm Atheros QCA8K Ethernet
 	  switch chips.
 
+config NET_DSA_QCA8K_LEDS_SUPPORT
+	tristate "Qualcomm Atheros QCA8K Ethernet switch family LEDs support"
+	select NET_DSA_QCA8K
+	select LEDS_OFFLOAD_TRIGGERS
+	help
+	  Thsi enabled support for LEDs present on the Qualcomm Atheros
+	  QCA8K Ethernet switch chips. This require the LEDs offload
+	  triggers support as it can run in offload mode.
+
 config NET_DSA_REALTEK_SMI
 	tristate "Realtek SMI Ethernet switch family support"
 	select NET_DSA_TAG_RTL4_A
diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
index 8da1569a34e6..fb1e7d0cf126 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_NET_DSA_LANTIQ_GSWIP) += lantiq_gswip.o
 obj-$(CONFIG_NET_DSA_MT7530)	+= mt7530.o
 obj-$(CONFIG_NET_DSA_MV88E6060) += mv88e6060.o
 obj-$(CONFIG_NET_DSA_QCA8K)	+= qca8k.o
+obj-$(CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT) += qca8k-leds.o
 obj-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o
 realtek-smi-objs		:= realtek-smi-core.o rtl8366.o rtl8366rb.o rtl8365mb.o
 obj-$(CONFIG_NET_DSA_SMSC_LAN9303) += lan9303-core.o
diff --git a/drivers/net/dsa/qca8k-leds.c b/drivers/net/dsa/qca8k-leds.c
new file mode 100644
index 000000000000..5afcdaa0556c
--- /dev/null
+++ b/drivers/net/dsa/qca8k-leds.c
@@ -0,0 +1,364 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <net/dsa.h>
+
+#include "qca8k.h"
+
+static int
+qca8k_get_enable_led_reg(int port_num, int led_num, struct qca8k_led_pattern_en *reg_info)
+{
+	int shift;
+
+	switch (port_num) {
+	case 0:
+		reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+		reg_info->shift = 14;
+		break;
+	case 1:
+	case 2:
+	case 3:
+		reg_info->reg = QCA8K_LED_CTRL_REG(3);
+		shift = 2 * led_num + (6 * (port_num - 1));
+
+		reg_info->shift = 8 + shift;
+
+		break;
+	case 4:
+		reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+		reg_info->shift = 30;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int
+qca8k_get_control_led_reg(int port_num, int led_num, struct qca8k_led_pattern_en *reg_info)
+{
+	reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+
+	/* 6 total control rule:
+	 * 3 control rules for phy0-3 that applies to all their leds
+	 * 3 control rules for phy4
+	 */
+	if (port_num == 4)
+		reg_info->shift = 16;
+	else
+		reg_info->shift = 0;
+
+	return 0;
+}
+
+static int
+qca8k_cled_configure_offload(struct led_classdev *ldev, u32 rules, enum offload_trigger_cmd cmd)
+{
+	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+	struct led_trigger *trigger = ldev->trigger;
+	struct qca8k_led_pattern_en reg_info;
+	struct qca8k_priv *priv = led->priv;
+	u32 offload_trigger = 0, mask, val;
+	int ret;
+
+	/* Check trigger compatibility */
+	if (strcmp(trigger->name, "offload-phy-activity"))
+		return -EOPNOTSUPP;
+
+	switch (rules) {
+	case BLINK_TX:
+		offload_trigger = QCA8K_LED_TX_BLINK_MASK;
+		break;
+	case BLINK_RX:
+		offload_trigger = QCA8K_LED_RX_BLINK_MASK;
+		break;
+	case BLINK_COLLISION:
+		offload_trigger = QCA8K_LED_COL_BLINK_MASK;
+		break;
+	case KEEP_LINK_10M:
+		offload_trigger = QCA8K_LED_LINK_10M_EN_MASK;
+		break;
+	case KEEP_LINK_100M:
+		offload_trigger = QCA8K_LED_LINK_100M_EN_MASK;
+		break;
+	case KEEP_LINK_1000M:
+		offload_trigger = QCA8K_LED_LINK_1000M_EN_MASK;
+		break;
+	case KEEP_HALF_DUPLEX:
+		offload_trigger = QCA8K_LED_HALF_DUPLEX_MASK;
+		break;
+	case KEEP_FULL_DUPLEX:
+		offload_trigger = QCA8K_LED_FULL_DUPLEX_MASK;
+		break;
+	case OPTION_LINKUP_OVER:
+		offload_trigger = QCA8K_LED_LINKUP_OVER_MASK;
+		break;
+	case OPTION_BLINK_2HZ:
+			offload_trigger = QCA8K_LED_BLINK_2HZ;
+		break;
+	case OPTION_BLINK_4HZ:
+			offload_trigger = QCA8K_LED_BLINK_4HZ;
+		break;
+	case OPTION_BLINK_8HZ:
+			offload_trigger = QCA8K_LED_BLINK_8HZ;
+		break;
+	case OPTION_BLINK_AUTO:
+			offload_trigger = QCA8K_LED_BLINK_AUTO;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	if (rules == OPTION_BLINK_2HZ || rules == OPTION_BLINK_4HZ ||
+	    rules == OPTION_BLINK_8HZ || rules == OPTION_BLINK_AUTO) {
+		mask = QCA8K_LED_BLINK_FREQ_MASK;
+	} else {
+		mask = offload_trigger;
+	}
+
+	qca8k_get_control_led_reg(led->port_num, led->led_num, &reg_info);
+
+	switch (cmd) {
+	case TRIGGER_SUPPORTED:
+		/* We reache this point, we are sure the trigger is supported */
+		return 1;
+	case TRIGGER_ENABLE:
+		ret = qca8k_rmw(priv, reg_info.reg,
+				mask << reg_info.shift,
+				offload_trigger << reg_info.shift);
+		break;
+	case TRIGGER_DISABLE:
+		ret = qca8k_rmw(priv, reg_info.reg,
+				mask << reg_info.shift,
+				0);
+		break;
+	case TRIGGER_READ:
+		ret = qca8k_read(priv, reg_info.reg, &val);
+		if (ret)
+			return ret;
+
+		val >>= reg_info.shift;
+		val &= offload_trigger;
+
+		/* Special handling for LED_BLINK_2HZ */
+		if (!val && offload_trigger == QCA8K_LED_BLINK_2HZ)
+			val = 1;
+
+		return val;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return ret;
+}
+
+static void
+qca8k_led_brightness_set(struct qca8k_led *led,
+			 enum led_brightness b)
+{
+	struct qca8k_led_pattern_en reg_info;
+	struct qca8k_priv *priv = led->priv;
+	u32 val = QCA8K_LED_ALWAYS_OFF;
+
+	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+	if (b)
+		val = QCA8K_LED_ALWAYS_ON;
+
+	qca8k_rmw(priv, reg_info.reg,
+		  GENMASK(1, 0) << reg_info.shift,
+		  val << reg_info.shift);
+}
+
+static void
+qca8k_cled_brightness_set(struct led_classdev *ldev,
+			  enum led_brightness b)
+{
+	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+	return qca8k_led_brightness_set(led, b);
+}
+
+static enum led_brightness
+qca8k_led_brightness_get(struct qca8k_led *led)
+{
+	struct qca8k_led_pattern_en reg_info;
+	struct qca8k_priv *priv = led->priv;
+	u32 val;
+	int ret;
+
+	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+	ret = qca8k_read(priv, reg_info.reg, &val);
+	if (ret)
+		return 0;
+
+	val >>= reg_info.shift;
+	val &= GENMASK(1, 0);
+
+	return val > 0 ? 1 : 0;
+}
+
+static enum led_brightness
+qca8k_cled_brightness_get(struct led_classdev *ldev)
+{
+	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+	return qca8k_led_brightness_get(led);
+}
+
+static int
+qca8k_cled_blink_set(struct led_classdev *ldev,
+		     unsigned long *delay_on,
+		     unsigned long *delay_off)
+{
+	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+	struct qca8k_led_pattern_en reg_info;
+	struct qca8k_priv *priv = led->priv;
+
+	if (*delay_on == 0 && *delay_off == 0) {
+		*delay_on = 125;
+		*delay_off = 125;
+	}
+
+	if (*delay_on != 125 || *delay_off != 125) {
+		/* The hardware only supports blinking at 4Hz. Fall back
+		 * to software implementation in other cases.
+		 */
+		return -EINVAL;
+	}
+
+	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+	qca8k_rmw(priv, reg_info.reg,
+		  GENMASK(1, 0) << reg_info.shift,
+		  QCA8K_LED_ALWAYS_BLINK_4HZ << reg_info.shift);
+
+	return 0;
+}
+
+static int
+qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
+{
+	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+	struct qca8k_led_pattern_en reg_info;
+	struct qca8k_priv *priv = led->priv;
+	u32 val = QCA8K_LED_ALWAYS_OFF;
+
+	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+	if (enable)
+		val = QCA8K_LED_RULE_CONTROLLED;
+
+	return qca8k_rmw(priv, reg_info.reg,
+			 GENMASK(1, 0) << reg_info.shift,
+			 val << reg_info.shift);
+}
+
+static int
+qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
+{
+	struct fwnode_handle *led = NULL, *leds = NULL;
+	struct led_init_data init_data = { };
+	struct qca8k_led *port_led;
+	int led_num, port_index;
+	const char *state;
+	int ret;
+
+	leds = fwnode_get_named_child_node(port, "leds");
+	if (!leds) {
+		dev_dbg(priv->dev, "No Leds node specified in device tree for port %d!\n",
+			port_num);
+		return 0;
+	}
+
+	fwnode_for_each_child_node(leds, led) {
+		/* Reg rapresent the led number of the port.
+		 * Each port can have at least 3 leds attached
+		 * Commonly:
+		 * 1. is gigabit led
+		 * 2. is mbit led
+		 * 3. additional status led
+		 */
+		if (fwnode_property_read_u32(led, "reg", &led_num))
+			continue;
+
+		if (led_num >= QCA8K_LED_PORT_COUNT) {
+			dev_warn(priv->dev, "Invalid LED reg defined %d", port_num);
+			continue;
+		}
+
+		port_index = 3 * port_num + led_num;
+
+		port_led = &priv->ports_led[port_index];
+		port_led->port_num = port_num;
+		port_led->led_num = led_num;
+		port_led->priv = priv;
+
+		ret = fwnode_property_read_string(led, "default-state", &state);
+		if (!ret) {
+			if (!strcmp(state, "on")) {
+				port_led->cdev.brightness = 1;
+				qca8k_led_brightness_set(port_led, 1);
+			} else if (!strcmp(state, "off")) {
+				port_led->cdev.brightness = 0;
+				qca8k_led_brightness_set(port_led, 0);
+			} else if (!strcmp(state, "keep")) {
+				port_led->cdev.brightness =
+					qca8k_led_brightness_get(port_led);
+			}
+		}
+
+		/* 3 brightness settings can be applied from Documentation:
+		 * 0 always off
+		 * 1 blink at 4Hz
+		 * 2 always on
+		 * 3 rule controlled
+		 * Suppots only 2 mode: (pcb limitation, with always on and blink
+		 * only the last led is set to this mode)
+		 * 0 always off (sets all leds off)
+		 * 3 rule controlled
+		 */
+		port_led->cdev.max_brightness = 1;
+		port_led->cdev.brightness_set = qca8k_cled_brightness_set;
+		port_led->cdev.brightness_get = qca8k_cled_brightness_get;
+		port_led->cdev.blink_set = qca8k_cled_blink_set;
+		port_led->cdev.configure_offload = qca8k_cled_configure_offload;
+		port_led->cdev.trigger_offload = qca8k_cled_trigger_offload;
+		init_data.default_label = ":port";
+		init_data.devicename = "qca8k";
+		init_data.fwnode = led;
+
+		ret = devm_led_classdev_register_ext(priv->dev, &port_led->cdev, &init_data);
+		if (ret)
+			dev_warn(priv->dev, "Failed to int led");
+	}
+
+	return 0;
+}
+
+int
+qca8k_setup_led_ctrl(struct qca8k_priv *priv)
+{
+	struct fwnode_handle *mdio, *port;
+	int port_num;
+	int ret;
+
+	mdio = device_get_named_child_node(priv->dev, "mdio");
+	if (!mdio) {
+		dev_info(priv->dev, "No MDIO node specified in device tree!\n");
+		return 0;
+	}
+
+	fwnode_for_each_child_node(mdio, port) {
+		if (fwnode_property_read_u32(port, "reg", &port_num))
+			continue;
+
+		/* Each port can have at least 3 different leds attached */
+		ret = qca8k_parse_port_leds(priv, port, port_num);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index a429c9750add..10b2b47b2156 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -148,7 +148,7 @@ qca8k_set_page(struct mii_bus *bus, u16 page)
 	return 0;
 }
 
-static int
+int
 qca8k_read(struct qca8k_priv *priv, u32 reg, u32 *val)
 {
 	struct mii_bus *bus = priv->bus;
@@ -192,7 +192,7 @@ qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
 	return ret;
 }
 
-static int
+int
 qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
 {
 	struct mii_bus *bus = priv->bus;
@@ -1109,6 +1109,10 @@ qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	ret = qca8k_setup_led_ctrl(priv);
+	if (ret)
+		return ret;
+
 	/* Make sure MAC06 is disabled */
 	ret = qca8k_reg_clear(priv, QCA8K_REG_PORT0_PAD_CTRL,
 			      QCA8K_PORT0_PAD_MAC06_EXCHANGE_EN);
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 128b8cf85e08..b11385ff9bce 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -11,6 +11,7 @@
 #include <linux/delay.h>
 #include <linux/regmap.h>
 #include <linux/gpio.h>
+#include <linux/leds.h>
 
 #define QCA8K_NUM_PORTS					7
 #define QCA8K_NUM_CPU_PORTS				2
@@ -74,6 +75,42 @@
 #define   QCA8K_MDIO_MASTER_DATA_MASK			GENMASK(15, 0)
 #define   QCA8K_MDIO_MASTER_MAX_PORTS			5
 #define   QCA8K_MDIO_MASTER_MAX_REG			32
+
+/* LED control register */
+#define QCA8K_LED_COUNT					15
+#define QCA8K_LED_PORT_COUNT				3
+#define QCA8K_LED_RULE_COUNT				6
+#define QCA8K_LED_RULE_MAX				11
+#define QCA8K_LED_CTRL_REG(_i)				(0x050 + (_i) * 4)
+#define QCA8K_LED_CTRL0_REG				0x50
+#define QCA8K_LED_CTRL1_REG				0x54
+#define QCA8K_LED_CTRL2_REG				0x58
+#define QCA8K_LED_CTRL3_REG				0x5C
+#define   QCA8K_LED_CTRL_SHIFT(_i)			(((_i) % 2) * 16)
+#define   QCA8K_LED_CTRL_MASK				GENMASK(15, 0)
+#define QCA8K_LED_BLINK_FREQ_MASK			GENMASK(1, 0)
+#define QCA8K_LED_BLINK_FREQ_SHITF			0
+#define   QCA8K_LED_BLINK_2HZ				0
+#define   QCA8K_LED_BLINK_4HZ				1
+#define   QCA8K_LED_BLINK_8HZ				2
+#define   QCA8K_LED_BLINK_AUTO				3
+#define QCA8K_LED_LINKUP_OVER_MASK			BIT(2)
+#define QCA8K_LED_TX_BLINK_MASK				BIT(4)
+#define QCA8K_LED_RX_BLINK_MASK				BIT(5)
+#define QCA8K_LED_COL_BLINK_MASK			BIT(7)
+#define QCA8K_LED_LINK_10M_EN_MASK			BIT(8)
+#define QCA8K_LED_LINK_100M_EN_MASK			BIT(9)
+#define QCA8K_LED_LINK_1000M_EN_MASK			BIT(10)
+#define QCA8K_LED_POWER_ON_LIGHT_MASK			BIT(11)
+#define QCA8K_LED_HALF_DUPLEX_MASK			BIT(12)
+#define QCA8K_LED_FULL_DUPLEX_MASK			BIT(13)
+#define QCA8K_LED_PATTERN_EN_MASK			GENMASK(15, 14)
+#define QCA8K_LED_PATTERN_EN_SHIFT			14
+#define   QCA8K_LED_ALWAYS_OFF				0
+#define   QCA8K_LED_ALWAYS_BLINK_4HZ			1
+#define   QCA8K_LED_ALWAYS_ON				2
+#define   QCA8K_LED_RULE_CONTROLLED			3
+
 #define QCA8K_GOL_MAC_ADDR0				0x60
 #define QCA8K_GOL_MAC_ADDR1				0x64
 #define QCA8K_MAX_FRAME_SIZE				0x78
@@ -279,6 +316,19 @@ struct qca8k_ports_config {
 	u8 rgmii_tx_delay[QCA8K_NUM_CPU_PORTS]; /* 0: CPU port0, 1: CPU port6 */
 };
 
+struct qca8k_led_pattern_en {
+	u32 reg;
+	u8 shift;
+};
+
+struct qca8k_led {
+	u8 port_num;
+	u8 led_num;
+	u16 old_rule;
+	struct qca8k_priv *priv;
+	struct led_classdev cdev;
+};
+
 struct qca8k_priv {
 	u8 switch_id;
 	u8 switch_revision;
@@ -293,6 +343,7 @@ struct qca8k_priv {
 	struct dsa_switch_ops ops;
 	struct gpio_desc *reset_gpio;
 	unsigned int port_mtu[QCA8K_NUM_PORTS];
+	struct qca8k_led ports_led[QCA8K_LED_COUNT];
 };
 
 struct qca8k_mib_desc {
@@ -308,4 +359,17 @@ struct qca8k_fdb {
 	u8 mac[6];
 };
 
+/* Common function used by the LEDs module */
+int qca8k_read(struct qca8k_priv *priv, u32 reg, u32 *val);
+int qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val);
+
+#ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
+int qca8k_setup_led_ctrl(struct qca8k_priv *priv);
+#else
+static inline int qca8k_setup_led_ctrl(struct qca8k_priv *priv)
+{
+	return 0;
+}
+#endif
+
 #endif /* __QCA8K_H */
-- 
2.32.0

[RFC PATCH v2 5/5] dt-bindings: net: dsa: qca8k: add LEDs definition example

[Ansuel Smith]

Add LEDs definition example for qca8k using the offload trigger as the
default trigger and add all the supported offload triggers by the
switch.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 .../devicetree/bindings/net/dsa/qca8k.yaml    | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
index 48de0ace265d..106d95adc1e8 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
@@ -64,6 +64,8 @@ properties:
                  internal mdio access is used.
                  With the legacy mapping the reg corresponding to the internal
                  mdio is the switch reg with an offset of -1.
+                 Each phy have at least 3 LEDs connected and can be declared
+                 using the standard LEDs structure.
 
     properties:
       '#address-cells':
@@ -340,6 +342,24 @@ examples:
 
                 internal_phy_port1: ethernet-phy@0 {
                     reg = <0>;
+
+                    leds {
+                        led@0 {
+                            reg = <0>;
+                            color = <LED_COLOR_ID_WHITE>;
+                            function = LED_FUNCTION_LAN;
+                            function-enumerator = <1>;
+                            linux,default-trigger = "offload-phy-activity";
+                        };
+
+                        led@1 {
+                            reg = <1>;
+                            color = <LED_COLOR_ID_AMBER>;
+                            function = LED_FUNCTION_LAN;
+                            function-enumerator = <1>;
+                            linux,default-trigger = "offload-phy-activity";
+                        };
+                    };
                 };
 
                 internal_phy_port2: ethernet-phy@1 {
-- 
2.32.0

Re: [RFC PATCH v2 2/5] leds: add function to configure offload leds

[Randy Dunlap]

On 11/7/21 4:24 PM, Ansuel Smith wrote:
> diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
> index 5bf6e5d471ce..0a3bbe71dac7 100644
> --- a/Documentation/leds/leds-class.rst
> +++ b/Documentation/leds/leds-class.rst
> @@ -190,6 +190,30 @@ If the second argument (enable) to the trigger_offload() method is false, any
>   active HW offloading must be deactivated. In this case errors are not permitted
>   in the trigger_offload() method and the driver will be set to the new trigger.
>   
> +The offload trigger will use the function configure_offload() provided by the driver
> +that will configure the offloaded mode for the LED.
> +This function passes as the first argument (offload_flags) a u32 flag.
> +The second argument (cmd) of the configure_offload() method can be used to do various
> +operations for the specific trigger. We currently support ENABLE, DISABLE, READ and
> +SUPPORTED to enable, disable, read the state of the offload trigger and ask the LED
> +driver supports the specific offload trigger.
> +
> +In ENABLE/DISABLE configure_offload() should configure the LED to enable/disable the
> +requested trigger (flags).
> +In READ configure_offload() should return 0 or 1 based on the status of the requested
> +trigger (flags).
> +In SUPPORTED configure_offload() should return 0 or 1 if the LED driver supports the
> +requested trigger (flags) or not.
> +
> +The u32 flag is specific to the trigger and change across them. It's in the LED

                                                changes

> +driver interest know how to elaborate this flag and to declare support for a

    driver's

> +particular offload trigger. For this exact reason explicit support for the specific
> +trigger is mandatory or the driver returns -EOPNOTSUPP if asked to enter offload mode
> +with a not supported trigger.
> +If the driver returns -EOPNOTSUPP on configure_offload(), the trigger activation will
> +fail as the driver doesn't support that specific offload trigger or doesn't know
> +how to handle the provided flags.


-- 
~Randy

Re: [RFC PATCH v2 3/5] leds: trigger: add offload-phy-activity trigger

[Randy Dunlap]

On 11/7/21 4:24 PM, Ansuel Smith wrote:
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index 33aba8defeab..14023e160ba1 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -164,4 +164,33 @@ config LEDS_TRIGGER_TTY
>   
>   	  When build as a module this driver will be called ledtrig-tty.
>   
> +config LEDS_OFFLOAD_TRIGGER_PHY_ACTIVITY
> +	tristate "LED Offload Trigger for PHY Activity"
> +	depends on LEDS_OFFLOAD_TRIGGERS
> +	help
> +	  This allows LEDs to be configured to run by hardware and offloaded

	                                    to be run

> +	  based on some rules. The LED will blink or be on based on the PHY

Could "on" be changed to ON or "on" or 'on' here:    be ON based on the PHY

> +	  Activity for example on packet receive or based on the link speed.


-- 
~Randy

Re: [RFC PATCH v2 4/5] net: dsa: qca8k: add LEDs support

[Randy Dunlap]

On 11/7/21 4:24 PM, Ansuel Smith wrote:
> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index 7b1457a6e327..89e36f3a8195 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -67,6 +67,15 @@ config NET_DSA_QCA8K
>   	  This enables support for the Qualcomm Atheros QCA8K Ethernet
>   	  switch chips.
>   
> +config NET_DSA_QCA8K_LEDS_SUPPORT
> +	tristate "Qualcomm Atheros QCA8K Ethernet switch family LEDs support"
> +	select NET_DSA_QCA8K
> +	select LEDS_OFFLOAD_TRIGGERS
> +	help
> +	  Thsi enabled support for LEDs present on the Qualcomm Atheros

	  This enables

> +	  QCA8K Ethernet switch chips. This require the LEDs offload

	                                    requires

> +	  triggers support as it can run in offload mode.


-- 
~Randy

Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Randy Dunlap]

On 11/7/21 4:24 PM, Ansuel Smith wrote:
> From: Marek Behún <kabel@kernel.org>
> 

> ---
>   Documentation/leds/leds-class.rst | 20 +++++++++++++++++++
>   drivers/leds/led-triggers.c       |  1 +
>   drivers/leds/trigger/Kconfig      | 10 ++++++++++
>   include/linux/leds.h              | 33 +++++++++++++++++++++++++++++++
>   4 files changed, 64 insertions(+)
> 
> diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
> index cd155ead8703..5bf6e5d471ce 100644
> --- a/Documentation/leds/leds-class.rst
> +++ b/Documentation/leds/leds-class.rst
> @@ -169,6 +169,26 @@ Setting the brightness to zero with brightness_set() callback function
>   should completely turn off the LED and cancel the previously programmed
>   hardware blinking function, if any.
>   
> +Hardware offloading of LED triggers
> +===================================
> +
> +Some LEDs can be driven by hardware triggers (for example a LED connected to
> +an ethernet PHY or an ethernet switch can be configured to blink on activity on
> +the network, which in software is done by the netdev trigger).
> +
> +To do such offloading, LED driver must support this and a deficated offload

                                                              dedicated

> +trigger must be used. The LED must implement the trigger_offload() method and
> +the trigger code must try to call this method (via led_trigger_offload()
> +function) to set the LED to this particular mode. (and disable any software
> +blinking)
> +
> +The implementation of the trigger_offload() method by the LED driver must return
> +0 if the offload is successful and -EOPNOTSUPP if the requested trigger
> +configuration is not supported.
> +
> +If the second argument (enable) to the trigger_offload() method is false, any
> +active HW offloading must be deactivated. In this case errors are not permitted

Preferably s/HW/hardware/ and s/SW/software/ throughout documentation and Kconfig
files.

> +in the trigger_offload() method and the driver will be set to the new trigger.
>   
>   Known Issues
>   ============


> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index dc6816d36d06..33aba8defeab 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -9,6 +9,16 @@ menuconfig LEDS_TRIGGERS
>   
>   if LEDS_TRIGGERS
>   
> +config LEDS_OFFLOAD_TRIGGERS
> +	bool "LED Offload Trigger support"
> +	help
> +	  This option enabled offload triggers support used by leds that

	              enables                                  LEDs

> +	  can be driven in hardware by declaring some specific triggers.
> +	  A offload trigger will expose a sysfs dir to configure the
> +	  different blinking trigger and the available hardware triggers.
> +
> +	  If unsure, say Y.


-- 
~Randy

Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Andrew Lunn]

> +static inline int led_trigger_offload(struct led_classdev *led_cdev)
> +{
> +	int ret;
> +
> +	if (!led_cdev->trigger_offload)
> +		return -EOPNOTSUPP;
> +
> +	ret = led_cdev->trigger_offload(led_cdev, true);
> +	led_cdev->offloaded = !ret;
> +
> +	return ret;
> +}
> +
> +static inline void led_trigger_offload_stop(struct led_classdev *led_cdev)
> +{
> +	if (!led_cdev->trigger_offload)
> +		return;
> +
> +	if (led_cdev->offloaded) {
> +		led_cdev->trigger_offload(led_cdev, false);
> +		led_cdev->offloaded = false;
> +	}
> +}
> +#endif

I think there should be two calls into the cdev driver, not this
true/false parameter. trigger_offload_start() and
trigger_offload_stop().

There are also a number of PHYs which don't allow software blinking of
the LED. So for them, trigger_offload_stop() is going to return
-EOPNOTSUPP. And you need to handle that correctly.

It would be go to also document the expectations of
trigger_offload_stop(). Should it leave the LED in whatever state it
was, or force it off? 

     Andrew

Re: [RFC PATCH v2 3/5] leds: trigger: add offload-phy-activity trigger

[Andrew Lunn]

On Mon, Nov 08, 2021 at 01:24:58AM +0100, Ansuel Smith wrote:
> Add Offload Trigger for PHY Activity. This special trigger is used to
> configure and expose the different HW trigger that are provided by the
> PHY. Each offload trigger can be configured by sysfs and on trigger
> activation the offload mode is enabled.
> 
> This currently implement these hw triggers:
>   - blink_tx: Blink LED on tx packet receive
>   - blink_rx: Blink LED on rx packet receive
>   - blink_collision: Blink LED on collision detection

When did you last see a collision? Do you really have a 1/2 duplex
link? Just because the PHY can, does not mean we should support
it. Lets restrict this to the most useful modes.

>   - link_10m: Keep LED on with 10m link speed
>   - link_100m: Keep LED on with 100m link speed
>   - link_1000m: Keep LED on with 1000m link speed
>   - half_duplex: Keep LED on with half duplex link
>   - full_duplex: Keep LED on with full duplex link
>   - linkup_over: Keep LED on with link speed and blink on rx/tx traffic
>   - power_on_reset: Keep LED on with switch reset

>   - blink_2hz: Set blink speed at 2hz for every blink event
>   - blink_4hz: Set blink speed at 4hz for every blink event
>   - blink_8hz: Set blink speed at 8hz for every blink event

These seems like attributes, not blink modes. They need to be
specified somehow differently, or not at all. Do we really need them?

>   - blink_auto: Set blink speed at 2hz for 10m link speed,
>       4hz for 100m and 8hz for 1000m

Another attribute, and one i've not seen any other PHY do.

	Andrew

Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Ansuel Smith]

On Mon, Nov 08, 2021 at 03:04:23PM +0100, Andrew Lunn wrote:
> > +static inline int led_trigger_offload(struct led_classdev *led_cdev)
> > +{
> > +	int ret;
> > +
> > +	if (!led_cdev->trigger_offload)
> > +		return -EOPNOTSUPP;
> > +
> > +	ret = led_cdev->trigger_offload(led_cdev, true);
> > +	led_cdev->offloaded = !ret;
> > +
> > +	return ret;
> > +}
> > +
> > +static inline void led_trigger_offload_stop(struct led_classdev *led_cdev)
> > +{
> > +	if (!led_cdev->trigger_offload)
> > +		return;
> > +
> > +	if (led_cdev->offloaded) {
> > +		led_cdev->trigger_offload(led_cdev, false);
> > +		led_cdev->offloaded = false;
> > +	}
> > +}
> > +#endif
> 
> I think there should be two calls into the cdev driver, not this
> true/false parameter. trigger_offload_start() and
> trigger_offload_stop().
> 

To not add too much function to the struct, can we introduce one
function that both enable and disable the hw mode?

> There are also a number of PHYs which don't allow software blinking of
> the LED. So for them, trigger_offload_stop() is going to return
> -EOPNOTSUPP. And you need to handle that correctly.
> 

So we have PHYs that can only work in offload or off. Correct?

> It would be go to also document the expectations of
> trigger_offload_stop(). Should it leave the LED in whatever state it
> was, or force it off? 
>

I think it should be put off. Do you agree? (also the brightness should
be set to 0 in this case)

>      Andrew

-- 
	Ansuel

Re: [RFC PATCH v2 3/5] leds: trigger: add offload-phy-activity trigger

[Ansuel Smith]

On Mon, Nov 08, 2021 at 03:17:33PM +0100, Andrew Lunn wrote:
> On Mon, Nov 08, 2021 at 01:24:58AM +0100, Ansuel Smith wrote:
> > Add Offload Trigger for PHY Activity. This special trigger is used to
> > configure and expose the different HW trigger that are provided by the
> > PHY. Each offload trigger can be configured by sysfs and on trigger
> > activation the offload mode is enabled.
> > 
> > This currently implement these hw triggers:
> >   - blink_tx: Blink LED on tx packet receive
> >   - blink_rx: Blink LED on rx packet receive
> >   - blink_collision: Blink LED on collision detection
> 
> When did you last see a collision? Do you really have a 1/2 duplex
> link? Just because the PHY can, does not mean we should support
> it. Lets restrict this to the most useful modes.
>

Ok will drop this. In my case (qca8k) I also never see a device using it
so I agree on the fact that should be dropped.

> >   - link_10m: Keep LED on with 10m link speed
> >   - link_100m: Keep LED on with 100m link speed
> >   - link_1000m: Keep LED on with 1000m link speed
> >   - half_duplex: Keep LED on with half duplex link
> >   - full_duplex: Keep LED on with full duplex link
> >   - linkup_over: Keep LED on with link speed and blink on rx/tx traffic
> >   - power_on_reset: Keep LED on with switch reset
> 
> >   - blink_2hz: Set blink speed at 2hz for every blink event
> >   - blink_4hz: Set blink speed at 4hz for every blink event
> >   - blink_8hz: Set blink speed at 8hz for every blink event
> 
> These seems like attributes, not blink modes. They need to be
> specified somehow differently, or not at all. Do we really need them?
> 

Sorry I didn't update the commit. In sysfs they are exposed as option
like the power_on_reset and linkup_over. So they are option on how the
LED behave on the event.

> >   - blink_auto: Set blink speed at 2hz for 10m link speed,
> >       4hz for 100m and 8hz for 1000m
> 
> Another attribute, and one i've not seen any other PHY do.
> 

Yes we can consider dropping this but I think the other 3 should be
keeped.

> 	Andrew

-- 
	Ansuel

Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Marek Behún]

On Mon, 8 Nov 2021 16:16:13 +0100
Ansuel Smith <ansuelsmth@gmail.com> wrote:

> On Mon, Nov 08, 2021 at 03:04:23PM +0100, Andrew Lunn wrote:
> > > +static inline int led_trigger_offload(struct led_classdev *led_cdev)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (!led_cdev->trigger_offload)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	ret = led_cdev->trigger_offload(led_cdev, true);
> > > +	led_cdev->offloaded = !ret;
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static inline void led_trigger_offload_stop(struct led_classdev *led_cdev)
> > > +{
> > > +	if (!led_cdev->trigger_offload)
> > > +		return;
> > > +
> > > +	if (led_cdev->offloaded) {
> > > +		led_cdev->trigger_offload(led_cdev, false);
> > > +		led_cdev->offloaded = false;
> > > +	}
> > > +}
> > > +#endif  
> > 
> > I think there should be two calls into the cdev driver, not this
> > true/false parameter. trigger_offload_start() and
> > trigger_offload_stop().
> >   
> 
> To not add too much function to the struct, can we introduce one
> function that both enable and disable the hw mode?

Dear Ansuel,

what is the purpose of adding trigger_offload() methods to LED, if you
are not going to add support to offload the netdev trigger? That was
the entire purpose when I wrote that patch.

If you just want to create a new trigger that will make the PHY chip do
the blinking, there is no need at all for the offloading patch.

And you will also get a NACK from me and also Pavel (LED subsystem
maintainer).

The current plan is to:
- add support for offloading existing LED triggers to HW (LED
  controllers (PHY chips, for example))
- make netdev trigger try offloading itself to HW via this new API (if
  it fails, netdev trigger will blink the LED in SW as it does now)
- create LED classdevices in a PHY driver that have the offload()
  methods implemented. The offload method looks at what trigger is
  being enabled for the LED, and it if it is a netdev trigger with such
  settings that are possible to offload, it will be offloaded.

  This whole thing makes use of the existing sysfs ABI.
  So for example if I do
    cd /sys/class/net/eth0/phydev/leds/<LED>
    echo netdev >trigger
    echo eth0 >device_name
    echo 1 >rx
    echo 1 >tx
  The netdev trigger is activated, and it calls the offload() method.
  The offload() method is implemented in the PHY driver, and it checks
  that it can offload these settings (blink on rx/tx), and will enable
  this.
- extend netdev trigger to support more settings:
  - indicate link for specific link modes only (for example 1g, 100m)
  - ...
- extend PHY drivers to support offloading of these new settings

Marek

Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Ansuel Smith]

On Mon, Nov 08, 2021 at 05:13:12PM +0100, Marek Behún wrote:
> On Mon, 8 Nov 2021 16:16:13 +0100
> Ansuel Smith <ansuelsmth@gmail.com> wrote:
> 
> > On Mon, Nov 08, 2021 at 03:04:23PM +0100, Andrew Lunn wrote:
> > > > +static inline int led_trigger_offload(struct led_classdev *led_cdev)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	if (!led_cdev->trigger_offload)
> > > > +		return -EOPNOTSUPP;
> > > > +
> > > > +	ret = led_cdev->trigger_offload(led_cdev, true);
> > > > +	led_cdev->offloaded = !ret;
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static inline void led_trigger_offload_stop(struct led_classdev *led_cdev)
> > > > +{
> > > > +	if (!led_cdev->trigger_offload)
> > > > +		return;
> > > > +
> > > > +	if (led_cdev->offloaded) {
> > > > +		led_cdev->trigger_offload(led_cdev, false);
> > > > +		led_cdev->offloaded = false;
> > > > +	}
> > > > +}
> > > > +#endif  
> > > 
> > > I think there should be two calls into the cdev driver, not this
> > > true/false parameter. trigger_offload_start() and
> > > trigger_offload_stop().
> > >   
> > 
> > To not add too much function to the struct, can we introduce one
> > function that both enable and disable the hw mode?
> 
> Dear Ansuel,
>

(just to make sure, I don't want to look rude, I just want this feature
finally introduced and supported since AFAIK many tried to add support
for LEDs in PHY but everyone failed. For a reason or another, too
specific, not generic...)

> what is the purpose of adding trigger_offload() methods to LED, if you
> are not going to add support to offload the netdev trigger? That was
> the entire purpose when I wrote that patch.

But the final step was adding LEDs support for PHY. The idea was to find
a clean way by passing the idea of offloading a trigger... But fact is
that LEDs in PHY operate by themself so it would add overhead selecting
a trigger that will use event/works just to then be ignored as the
trigger is offloaded.
Also I think we are missing the fact that most of PHY can operate in
software or in hardware. NOT BOTH. So the entire concept of offloading
some trigger won't work as we were not able to simulate with SW
unsupported trigger. (not the case with netdev as it does only support
link, rx/tx but that was to explain the concept that SW and HW mode are
mutually exclusive.)

> 
> If you just want to create a new trigger that will make the PHY chip do
> the blinking, there is no need at all for the offloading patch.
> 

Again the idea here is that a LED can offer a way to run by HW and then
a trigger configure them and enables the mode. I see the offload and
configure function needed anyway side from the implementation.

> And you will also get a NACK from me and also Pavel (LED subsystem
> maintainer).
> 

Can we try to find a common way to introduce this?

> The current plan is to:
> - add support for offloading existing LED triggers to HW (LED
>   controllers (PHY chips, for example))
> - make netdev trigger try offloading itself to HW via this new API (if
>   it fails, netdev trigger will blink the LED in SW as it does now)

Can't be done. If in HW mode, we should just declare a trigger not
supported in SW if we really want to follow the netdev expanding path.
But still that would mean filling the netdev trigger with extra code and
condition that will only apply in HW mode. At this point just create a
dedicated trigger.
We can consider introducing the same sysfs used by netdev trigger but
nothing else.

> - create LED classdevices in a PHY driver that have the offload()
>   methods implemented. The offload method looks at what trigger is
>   being enabled for the LED, and it if it is a netdev trigger with such
>   settings that are possible to offload, it will be offloaded.
> 
>   This whole thing makes use of the existing sysfs ABI.
>   So for example if I do
>     cd /sys/class/net/eth0/phydev/leds/<LED>
>     echo netdev >trigger
>     echo eth0 >device_name

How would this work in HW mode? The PHY blink only with packet in his
port. We can't tell the PHY to HW blink based on an interface.
That is the main problem by using netdev for PHYs, netdev is flexible
but an offload trigger is not and would work only based on some
condition. We should hardcode the device_name in HW and again pollute
the netdev trigger more.
An idea would be add tons of check to netdev on every event to check the
interface but that wouldn't go against any offload idea? The system will
be loaded anyway with all these checks.

>     echo 1 >rx
>     echo 1 >tx
>   The netdev trigger is activated, and it calls the offload() method.
>   The offload() method is implemented in the PHY driver, and it checks
>   that it can offload these settings (blink on rx/tx), and will enable
>   this.
> - extend netdev trigger to support more settings:
>   - indicate link for specific link modes only (for example 1g, 100m)
>   - ...
> - extend PHY drivers to support offloading of these new settings
> 
> Marek

The rest of the implementation is very similar. Except we just NOT use
netdev. And to me it does seems the most sane way to handle offload for
a LED. (considering the specific situation of a PHY)

From what I can see we have 2 path:
- Pollute netdev trigger to add entire different function for HW. (that
  will end up in complex condition and more load/overhead)
- Introduce a new way to entirely offload some triggers.

Could be that here I'm just using the wrong word and I should use
hardware instead offload. But considering we are offloading some trigger
(example rx/tx/link) it's not that wrong.

The thing is why trying to expand a trigger that will just remove some
flexibility when we can solve the problem at the source with some
additional API that currently we lack any support (leds can be
configured to run by hw) and some dedicated trigger that will do the
task in a cleaner way (and without adding extra load/overhead by really
offloading the task) ?

Again hope I didn't seem rude in this message but I just want to find a
solution and proposing a new idea/explaining my concern.

-- 
	Ansuel

Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Marek Behún]

Dear Ansuel,

On Mon, 8 Nov 2021 17:46:02 +0100
Ansuel Smith <ansuelsmth@gmail.com> wrote:

> > what is the purpose of adding trigger_offload() methods to LED, if you
> > are not going to add support to offload the netdev trigger? That was
> > the entire purpose when I wrote that patch.  
> 
> But the final step was adding LEDs support for PHY. The idea was to find
> a clean way by passing the idea of offloading a trigger... But fact is
> that LEDs in PHY operate by themself so it would add overhead selecting
> a trigger that will use event/works just to then be ignored as the
> trigger is offloaded.

I don't understand what kind of overhead in events/works are you
talking about. If the trigger is successfully offloaded, there are no
events/works. Everything is done in hardware. If you look at my last
attempt
  https://lore.kernel.org/linux-leds/20210601005155.27997-5-kabel@kernel.org/
which adds the offloading support to netdev trigger, you can see

  if (!led_trigger_offload(led_cdev))
    return;

so if the trigger is succesfully offloaded, the subsequen
schedule_delayed_work() is not called and netdev trigger does not do anything.
The blinking of the LED on rx/tx activity is done purely in hardware.
No software overhead.

> Also I think we are missing the fact that most of PHY can operate in
> software or in hardware. NOT BOTH. So the entire concept of offloading
> some trigger won't work as we were not able to simulate with SW
> unsupported trigger. (not the case with netdev as it does only support
> link, rx/tx but that was to explain the concept that SW and HW mode are
> mutually exclusive.)

I am not missing this fact, I know that there are LEDs that cannot be
set into purely SW mode. Which brings another problem: Pavel currely
isn't convinced that these LEDs should be exported via LED classdev
API. We need to persuade him about this, because otherwise we would
need to create another subsystem for this.

But there are PHYs which do allow purely SW LED control, i.e. I can set
the LED to be just ON or OFF. These are for example marvell PHYs.

The fact that there are LEDs that can't be controlled purely in SW does
not seem a good reason for to not implement it via netdev trigger.

Either these LEDs shouldn't be exported as LED classdevs, or they
should somehow make use of existing trigger API (so netdev).

The fact that the LED cannot be controlled in SW can be simply
implemented by refusing to disable the netdev trigger on the LED.

> > 
> > If you just want to create a new trigger that will make the PHY chip do
> > the blinking, there is no need at all for the offloading patch.
> >   
> 
> Again the idea here is that a LED can offer a way to run by HW and then
> a trigger configure them and enables the mode. I see the offload and
> configure function needed anyway side from the implementation.

There is already LED private trigger API for this. You don't need
offloading of existing trigger if you are going to create a new trigger
anyway.

> > And you will also get a NACK from me and also Pavel (LED subsystem
> > maintainer).
> >   
> 
> Can we try to find a common way to introduce this?
> 
> > The current plan is to:
> > - add support for offloading existing LED triggers to HW (LED
> >   controllers (PHY chips, for example))
> > - make netdev trigger try offloading itself to HW via this new API (if
> >   it fails, netdev trigger will blink the LED in SW as it does now)  
> 
> Can't be done. If in HW mode, we should just declare a trigger not
> supported in SW if we really want to follow the netdev expanding path.
> But still that would mean filling the netdev trigger with extra code and
> condition that will only apply in HW mode. At this point just create a
> dedicated trigger.

The netdev trigger already needs to be expanded with extra code: it
doesn't allow indicating different link types, for example.

For offloading, the extra code can be quite small. So I don't think
this is an argument.

> We can consider introducing the same sysfs used by netdev trigger but
> nothing else.

Not true.

> > - create LED classdevices in a PHY driver that have the offload()
> >   methods implemented. The offload method looks at what trigger is
> >   being enabled for the LED, and it if it is a netdev trigger with such
> >   settings that are possible to offload, it will be offloaded.
> > 
> >   This whole thing makes use of the existing sysfs ABI.
> >   So for example if I do
> >     cd /sys/class/net/eth0/phydev/leds/<LED>
> >     echo netdev >trigger
> >     echo eth0 >device_name  
> 
> How would this work in HW mode? The PHY blink only with packet in his
> port. We can't tell the PHY to HW blink based on an interface.
> That is the main problem by using netdev for PHYs, netdev is flexible
> but an offload trigger is not and would work only based on some
> condition. We should hardcode the device_name in HW and again pollute
> the netdev trigger more.

netdev trigger does not need to be poluted by this much. All this can be
implemented by one callback to offload method, and everything else can
be done in the LED driver itself.

> An idea would be add tons of check to netdev on every event to check the
> interface but that wouldn't go against any offload idea? The system will
> be loaded anyway with all these checks.

The checks are done only at the moment of configuring the trigger.
Surely you don't see that as "loaded system".

> 
> >     echo 1 >rx
> >     echo 1 >tx
> >   The netdev trigger is activated, and it calls the offload() method.
> >   The offload() method is implemented in the PHY driver, and it checks
> >   that it can offload these settings (blink on rx/tx), and will enable
> >   this.
> > - extend netdev trigger to support more settings:
> >   - indicate link for specific link modes only (for example 1g, 100m)
> >   - ...
> > - extend PHY drivers to support offloading of these new settings
> > 
> > Marek  
> 
> The rest of the implementation is very similar. Except we just NOT use
> netdev. And to me it does seems the most sane way to handle offload for
> a LED. (considering the specific situation of a PHY)
> 
> From what I can see we have 2 path:
> - Pollute netdev trigger to add entire different function for HW. (that
>   will end up in complex condition and more load/overhead)
> - Introduce a new way to entirely offload some triggers.

The check conditions would be all in LED driver (PHY driver in this
case). The code could be a little complicated, but it only needs to be
written once, and the PHY drivers can reuse it. netdev trigger is not
polluted, only a few calls to offload() method are done.

> Could be that here I'm just using the wrong word and I should use
> hardware instead offload. But considering we are offloading some trigger
> (example rx/tx/link) it's not that wrong.
> 
> The thing is why trying to expand a trigger that will just remove some
> flexibility when we can solve the problem at the source with some
> additional API that currently we lack any support (leds can be
> configured to run by hw) and some dedicated trigger that will do the
> task in a cleaner way (and without adding extra load/overhead by really
> offloading the task) ?

As I explained above, there is no extra load/overhead. The only thing
that is extra is that netdev trigger gets a little more new code, but
that is already needed anyway.
> 
> Again hope I didn't seem rude in this message but I just want to find a
> solution and proposing a new idea/explaining my concern.
> 

I don't think you're rude, we are just discussing a topic for which we
have different opinions.

Marek

Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Andrew Lunn]

> So we have PHYs that can only work in offload or off. Correct?

No, they can only work in offload. There is no off. You just get to
chose different offload settings.

      Andrew

Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Andrew Lunn]

> Dear Ansuel,
> 
> what is the purpose of adding trigger_offload() methods to LED, if you
> are not going to add support to offload the netdev trigger? That was
> the entire purpose when I wrote that patch.
> 
> If you just want to create a new trigger that will make the PHY chip do
> the blinking, there is no need at all for the offloading patch.
> 
> And you will also get a NACK from me and also Pavel (LED subsystem
> maintainer).
> 
> The current plan is to:
> - add support for offloading existing LED triggers to HW (LED
>   controllers (PHY chips, for example))
> - make netdev trigger try offloading itself to HW via this new API (if
>   it fails, netdev trigger will blink the LED in SW as it does now)
> - create LED classdevices in a PHY driver that have the offload()
>   methods implemented. The offload method looks at what trigger is
>   being enabled for the LED, and it if it is a netdev trigger with such
>   settings that are possible to offload, it will be offloaded.
> 
>   This whole thing makes use of the existing sysfs ABI.
>   So for example if I do
>     cd /sys/class/net/eth0/phydev/leds/<LED>
>     echo netdev >trigger
>     echo eth0 >device_name
>     echo 1 >rx
>     echo 1 >tx
>   The netdev trigger is activated, and it calls the offload() method.
>   The offload() method is implemented in the PHY driver, and it checks
>   that it can offload these settings (blink on rx/tx), and will enable
>   this.
> - extend netdev trigger to support more settings:
>   - indicate link for specific link modes only (for example 1g, 100m)
>   - ...
> - extend PHY drivers to support offloading of these new settings
> 
> Marek

Hi Marek

The problem here is, you are not making much progress. People are
giving up on you ever getting this done, and doing their own
implementation. Ansuel code is not mature enough yet, it has problems,
but he is responsive, he is dealing with comments, progress is being
made. At some point, it is going to be good enough, and it will get
merged, unless you actual get your code to a point it can be merged.

	Andrew

Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Marek Behún]

On Mon, 8 Nov 2021 18:46:26 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > Dear Ansuel,
> > 
> > what is the purpose of adding trigger_offload() methods to LED, if you
> > are not going to add support to offload the netdev trigger? That was
> > the entire purpose when I wrote that patch.
> > 
> > If you just want to create a new trigger that will make the PHY chip do
> > the blinking, there is no need at all for the offloading patch.
> > 
> > And you will also get a NACK from me and also Pavel (LED subsystem
> > maintainer).
> > 
> > The current plan is to:
> > - add support for offloading existing LED triggers to HW (LED
> >   controllers (PHY chips, for example))
> > - make netdev trigger try offloading itself to HW via this new API (if
> >   it fails, netdev trigger will blink the LED in SW as it does now)
> > - create LED classdevices in a PHY driver that have the offload()
> >   methods implemented. The offload method looks at what trigger is
> >   being enabled for the LED, and it if it is a netdev trigger with such
> >   settings that are possible to offload, it will be offloaded.
> > 
> >   This whole thing makes use of the existing sysfs ABI.
> >   So for example if I do
> >     cd /sys/class/net/eth0/phydev/leds/<LED>
> >     echo netdev >trigger
> >     echo eth0 >device_name
> >     echo 1 >rx
> >     echo 1 >tx
> >   The netdev trigger is activated, and it calls the offload() method.
> >   The offload() method is implemented in the PHY driver, and it checks
> >   that it can offload these settings (blink on rx/tx), and will enable
> >   this.
> > - extend netdev trigger to support more settings:
> >   - indicate link for specific link modes only (for example 1g, 100m)
> >   - ...
> > - extend PHY drivers to support offloading of these new settings
> > 
> > Marek  
> 
> Hi Marek
> 
> The problem here is, you are not making much progress. People are
> giving up on you ever getting this done, and doing their own
> implementation. Ansuel code is not mature enough yet, it has problems,
> but he is responsive, he is dealing with comments, progress is being
> made. At some point, it is going to be good enough, and it will get
> merged, unless you actual get your code to a point it can be merged.
> 
> 	Andrew

Hello Andrew,

you are right that this has been taking too long on my side. I am sorry
for that.

I guess I will have to work on this again ASAP or we will end up with
solution that I don't like.

Nonetheless, what is your opinion about offloading netdev trigger vs
introducing another trigger?

Marek

Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Ansuel Smith]

On Mon, Nov 08, 2021 at 06:35:37PM +0100, Marek Behún wrote:
> Dear Ansuel,
> 
> On Mon, 8 Nov 2021 17:46:02 +0100
> Ansuel Smith <ansuelsmth@gmail.com> wrote:
> 
> > > what is the purpose of adding trigger_offload() methods to LED, if you
> > > are not going to add support to offload the netdev trigger? That was
> > > the entire purpose when I wrote that patch.  
> > 
> > But the final step was adding LEDs support for PHY. The idea was to find
> > a clean way by passing the idea of offloading a trigger... But fact is
> > that LEDs in PHY operate by themself so it would add overhead selecting
> > a trigger that will use event/works just to then be ignored as the
> > trigger is offloaded.
> 
> I don't understand what kind of overhead in events/works are you
> talking about. If the trigger is successfully offloaded, there are no
> events/works. Everything is done in hardware. If you look at my last
> attempt
>   https://lore.kernel.org/linux-leds/20210601005155.27997-5-kabel@kernel.org/
> which adds the offloading support to netdev trigger, you can see
> 
>   if (!led_trigger_offload(led_cdev))
>     return;
> 
> so if the trigger is succesfully offloaded, the subsequen
> schedule_delayed_work() is not called and netdev trigger does not do anything.
> The blinking of the LED on rx/tx activity is done purely in hardware.
> No software overhead.
> 
> > Also I think we are missing the fact that most of PHY can operate in
> > software or in hardware. NOT BOTH. So the entire concept of offloading
> > some trigger won't work as we were not able to simulate with SW
> > unsupported trigger. (not the case with netdev as it does only support
> > link, rx/tx but that was to explain the concept that SW and HW mode are
> > mutually exclusive.)
> 
> I am not missing this fact, I know that there are LEDs that cannot be
> set into purely SW mode. Which brings another problem: Pavel currely
> isn't convinced that these LEDs should be exported via LED classdev
> API. We need to persuade him about this, because otherwise we would
> need to create another subsystem for this.
> 
> But there are PHYs which do allow purely SW LED control, i.e. I can set
> the LED to be just ON or OFF. These are for example marvell PHYs.
> 
> The fact that there are LEDs that can't be controlled purely in SW does
> not seem a good reason for to not implement it via netdev trigger.
> 
> Either these LEDs shouldn't be exported as LED classdevs, or they
> should somehow make use of existing trigger API (so netdev).
> 
> The fact that the LED cannot be controlled in SW can be simply
> implemented by refusing to disable the netdev trigger on the LED.
>

Are you aware of any device that can have some trigger offloaded and
still have the led triggered manually? Talking about mixed mode, so HW
and SW. Asking to understand as currently the only way to impement all
of this in netdev trigger is that:
IF any hw offload trigger is supported (and enabled) then the entire
netdev trigger can't work as it won't be able to simulate missing
trigger in SW. And that would leave some flexibility.

We need to understand how to operate in this condition. Should netdev
detect that and ""hide"" the sysfs triggers? Should we report error?

> > > 
> > > If you just want to create a new trigger that will make the PHY chip do
> > > the blinking, there is no need at all for the offloading patch.
> > >   
> > 
> > Again the idea here is that a LED can offer a way to run by HW and then
> > a trigger configure them and enables the mode. I see the offload and
> > configure function needed anyway side from the implementation.
> 
> There is already LED private trigger API for this. You don't need
> offloading of existing trigger if you are going to create a new trigger
> anyway.
> 
> > > And you will also get a NACK from me and also Pavel (LED subsystem
> > > maintainer).
> > >   
> > 
> > Can we try to find a common way to introduce this?
> > 
> > > The current plan is to:
> > > - add support for offloading existing LED triggers to HW (LED
> > >   controllers (PHY chips, for example))
> > > - make netdev trigger try offloading itself to HW via this new API (if
> > >   it fails, netdev trigger will blink the LED in SW as it does now)  
> > 
> > Can't be done. If in HW mode, we should just declare a trigger not
> > supported in SW if we really want to follow the netdev expanding path.
> > But still that would mean filling the netdev trigger with extra code and
> > condition that will only apply in HW mode. At this point just create a
> > dedicated trigger.
> 
> The netdev trigger already needs to be expanded with extra code: it
> doesn't allow indicating different link types, for example.
> 
> For offloading, the extra code can be quite small. So I don't think
> this is an argument.
> 
> > We can consider introducing the same sysfs used by netdev trigger but
> > nothing else.
> 
> Not true.
> 
> > > - create LED classdevices in a PHY driver that have the offload()
> > >   methods implemented. The offload method looks at what trigger is
> > >   being enabled for the LED, and it if it is a netdev trigger with such
> > >   settings that are possible to offload, it will be offloaded.
> > > 
> > >   This whole thing makes use of the existing sysfs ABI.
> > >   So for example if I do
> > >     cd /sys/class/net/eth0/phydev/leds/<LED>
> > >     echo netdev >trigger
> > >     echo eth0 >device_name  
> > 
> > How would this work in HW mode? The PHY blink only with packet in his
> > port. We can't tell the PHY to HW blink based on an interface.
> > That is the main problem by using netdev for PHYs, netdev is flexible
> > but an offload trigger is not and would work only based on some
> > condition. We should hardcode the device_name in HW and again pollute
> > the netdev trigger more.
> 
> netdev trigger does not need to be poluted by this much. All this can be
> implemented by one callback to offload method, and everything else can
> be done in the LED driver itself.
> 
> > An idea would be add tons of check to netdev on every event to check the
> > interface but that wouldn't go against any offload idea? The system will
> > be loaded anyway with all these checks.
> 
> The checks are done only at the moment of configuring the trigger.
> Surely you don't see that as "loaded system".
> 
> > 
> > >     echo 1 >rx
> > >     echo 1 >tx
> > >   The netdev trigger is activated, and it calls the offload() method.
> > >   The offload() method is implemented in the PHY driver, and it checks
> > >   that it can offload these settings (blink on rx/tx), and will enable
> > >   this.
> > > - extend netdev trigger to support more settings:
> > >   - indicate link for specific link modes only (for example 1g, 100m)
> > >   - ...
> > > - extend PHY drivers to support offloading of these new settings
> > > 
> > > Marek  
> > 
> > The rest of the implementation is very similar. Except we just NOT use
> > netdev. And to me it does seems the most sane way to handle offload for
> > a LED. (considering the specific situation of a PHY)
> > 
> > From what I can see we have 2 path:
> > - Pollute netdev trigger to add entire different function for HW. (that
> >   will end up in complex condition and more load/overhead)
> > - Introduce a new way to entirely offload some triggers.
> 
> The check conditions would be all in LED driver (PHY driver in this
> case). The code could be a little complicated, but it only needs to be
> written once, and the PHY drivers can reuse it. netdev trigger is not
> polluted, only a few calls to offload() method are done.
> 
> > Could be that here I'm just using the wrong word and I should use
> > hardware instead offload. But considering we are offloading some trigger
> > (example rx/tx/link) it's not that wrong.
> > 
> > The thing is why trying to expand a trigger that will just remove some
> > flexibility when we can solve the problem at the source with some
> > additional API that currently we lack any support (leds can be
> > configured to run by hw) and some dedicated trigger that will do the
> > task in a cleaner way (and without adding extra load/overhead by really
> > offloading the task) ?
> 
> As I explained above, there is no extra load/overhead. The only thing
> that is extra is that netdev trigger gets a little more new code, but
> that is already needed anyway.
> > 
> > Again hope I didn't seem rude in this message but I just want to find a
> > solution and proposing a new idea/explaining my concern.
> > 
> 
> I don't think you're rude, we are just discussing a topic for which we
> have different opinions.
> 
> Marek

-- 
	Ansuel

Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Marek Behún]

On Mon, 8 Nov 2021 18:58:38 +0100
Ansuel Smith <ansuelsmth@gmail.com> wrote:

> Are you aware of any device that can have some trigger offloaded and
> still have the led triggered manually?

I don't understand why we would need such a thing.

Look, just to make it clear via an example: I have a device with a
Marvell PHY chip inside. There is a LED connected to one of the PHY LED
pins.

Marvell PHY has LED[0] control register, which supports the following
modes:
  LED is OFF
  LED is ON
  LED is ON when Link is up
  LED blinks on RX activity
  LED blinks on TX activity
  LED blinks on RX/TX activity
  LED is ON and blinks on RX/TX activity
  ...

I have code that exports this LED as a LED classdev

When I activate netdev trigger on this LED, the netdev trigger currently
just blinks the LED in software, by calling the .brightness_set()
method, which configures LED[0] control register to one of the first
two modes above (LED is OFF, LED is ON).

But I have also another patch that adds support to offloading netdev
trigger upon offloadable settings. The netdev trigger code calls the
.trigger_offload() method, which is implemented in PHY driver. This
method checks whether it is a netdev trigger that is to be offloaded,
and whether device_name is the name of the device attached to the PHY,
and then chooses one of the modes above, according to netdev trigger
settings.

So when I request netdev trigger for eth0, to indicate link and blink
on activity, the netdev trigger doesn't do anything in software. It
just calls the offload method ONCE (at the moment I am changing netdev
trigger settings). The blinking is then done by the PHY chip. Netdev
trigger doesn't do anything, at least not until I change the settings
again.

> Talking about mixed mode, so HW and SW.

What exactly do you mean by mixed mode? There is no mixed mode.

> Asking to understand as currently the only way to impement all
> of this in netdev trigger is that:
> IF any hw offload trigger is supported (and enabled) then the entire
> netdev trigger can't work as it won't be able to simulate missing
> trigger in SW. And that would leave some flexibility.

What do you mean by missing trigger here? I think we need to clarify
what we mean by the word "trigger". Are you talking about the various
blinking modes that the PHY supports? If so, please let's call them HW
control modes, and not triggers. By "triggers" I understand triggers
that can be enabled on a LED via /sys/class/leds/<LED>/trigger.

> We need to understand how to operate in this condition. Should netdev
> detect that and ""hide"" the sysfs triggers? Should we report error?

So if I understand you correctly, you are asking about what should we
do if user asked for netdev trigger settings (currently only link, rx,
tx, interval) that can't be offloaded to the PHY chip.

Well, if the PHY allows to manipulate the LEDs ON/OFF state (in other
words "full control by SW", or ability to implement brightness_set()
method), then netdev trigger should blink the LED in SW via this
mechanism (which is something it would do now). A new sysfs file,
"offloaded", can indicate whether the trigger is offloaded to HW or not.

If, on the other hand, the LED cannot be controlled by SW, and it only
support some HW control modes, then there are multiple ways how to
implement what should be done, and we need to discuss this.

For example suppose that the PHY LED pin supports indicating LINK,
blinking on activity, or both, but it doesn't support blinking on rx
only, or tx only.

Since the LED is always indicating something about one network device,
the netdev trigger should be always activated for this LED and it
should be impossible to deactivate it. Also, it should be impossible to
change device_name.

  $ cd /sys/class/leds/<LED>
  $ cat device_name
  eth0
  $ echo eth1 >device_name
  Operation not supported.
  $ echo none >trigger
  Operation not supported.

Now suppose that the driver by default enabled link indication, so we
have:
  $ cat link
  1
  $ cat rx
  0
  $ cat tx
  0

We want to enable blink on activity, but the LED supports only blinking
on both rx/tx activity, rx only or tx only is not supported.

Currently the only way to enable this is to do
  $ echo 1 >rx
  $ echo 1 >tx
but the first call asks for (link=1, rx=1, tx=0), which is impossible.

There are multiple things which can be done:
- "echo 1 >rx" indicates error, but remembers the setting
- "echo 1 >rx" quietly fails, without error indication. Something can
  be written to dmesg about nonsupported mode
- "echo 1 >rx" succeeds, but also sets tx=1
- rx and tx are non-writable, writing always fails. Another sysfs file
  is created, which lists modes that are actually supported, and allows
  to select between them. When a mode is selected, link,rx,tx are
  filled automatically, so that user may read them to know what the LED
  is actually doing
- something different?

Marek

Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Ansuel Smith]

On Mon, Nov 08, 2021 at 07:41:42PM +0100, Marek Behún wrote:
> On Mon, 8 Nov 2021 18:58:38 +0100
> Ansuel Smith <ansuelsmth@gmail.com> wrote:
> 
> > Are you aware of any device that can have some trigger offloaded and
> > still have the led triggered manually?
> 
> I don't understand why we would need such a thing.
> 
> Look, just to make it clear via an example: I have a device with a
> Marvell PHY chip inside. There is a LED connected to one of the PHY LED
> pins.
> 
> Marvell PHY has LED[0] control register, which supports the following
> modes:
>   LED is OFF
>   LED is ON
>   LED is ON when Link is up
>   LED blinks on RX activity
>   LED blinks on TX activity
>   LED blinks on RX/TX activity
>   LED is ON and blinks on RX/TX activity
>   ...
> 
> I have code that exports this LED as a LED classdev
> 
> When I activate netdev trigger on this LED, the netdev trigger currently
> just blinks the LED in software, by calling the .brightness_set()
> method, which configures LED[0] control register to one of the first
> two modes above (LED is OFF, LED is ON).
> 
> But I have also another patch that adds support to offloading netdev
> trigger upon offloadable settings. The netdev trigger code calls the
> .trigger_offload() method, which is implemented in PHY driver. This
> method checks whether it is a netdev trigger that is to be offloaded,
> and whether device_name is the name of the device attached to the PHY,
> and then chooses one of the modes above, according to netdev trigger
> settings.
> 
> So when I request netdev trigger for eth0, to indicate link and blink
> on activity, the netdev trigger doesn't do anything in software. It
> just calls the offload method ONCE (at the moment I am changing netdev
> trigger settings). The blinking is then done by the PHY chip. Netdev
> trigger doesn't do anything, at least not until I change the settings
> again.
> 
> > Talking about mixed mode, so HW and SW.
> 
> What exactly do you mean by mixed mode? There is no mixed mode.
>

Ok.

> > Asking to understand as currently the only way to impement all
> > of this in netdev trigger is that:
> > IF any hw offload trigger is supported (and enabled) then the entire
> > netdev trigger can't work as it won't be able to simulate missing
> > trigger in SW. And that would leave some flexibility.
> 
> What do you mean by missing trigger here? I think we need to clarify
> what we mean by the word "trigger". Are you talking about the various
> blinking modes that the PHY supports? If so, please let's call them HW
> control modes, and not triggers. By "triggers" I understand triggers
> that can be enabled on a LED via /sys/class/leds/<LED>/trigger.
> 

offload triggers = blinking modes supported

> > We need to understand how to operate in this condition. Should netdev
> > detect that and ""hide"" the sysfs triggers? Should we report error?
> 
> So if I understand you correctly, you are asking about what should we
> do if user asked for netdev trigger settings (currently only link, rx,
> tx, interval) that can't be offloaded to the PHY chip.
> 
> Well, if the PHY allows to manipulate the LEDs ON/OFF state (in other
> words "full control by SW", or ability to implement brightness_set()
> method), then netdev trigger should blink the LED in SW via this
> mechanism (which is something it would do now). A new sysfs file,
> "offloaded", can indicate whether the trigger is offloaded to HW or not.
> 

Are all these sysfs entry OK? I mean if we want to add support for he
main blinking modes, the number will increase to at least 10 additional
entry. 

> If, on the other hand, the LED cannot be controlled by SW, and it only
> support some HW control modes, then there are multiple ways how to
> implement what should be done, and we need to discuss this.
> 
> For example suppose that the PHY LED pin supports indicating LINK,
> blinking on activity, or both, but it doesn't support blinking on rx
> only, or tx only.
> 
> Since the LED is always indicating something about one network device,
> the netdev trigger should be always activated for this LED and it
> should be impossible to deactivate it. Also, it should be impossible to
> change device_name.
> 
>   $ cd /sys/class/leds/<LED>
>   $ cat device_name
>   eth0
>   $ echo eth1 >device_name
>   Operation not supported.
>   $ echo none >trigger
>   Operation not supported.
> 
> Now suppose that the driver by default enabled link indication, so we
> have:
>   $ cat link
>   1
>   $ cat rx
>   0
>   $ cat tx
>   0
> 
> We want to enable blink on activity, but the LED supports only blinking
> on both rx/tx activity, rx only or tx only is not supported.
> 
> Currently the only way to enable this is to do
>   $ echo 1 >rx
>   $ echo 1 >tx
> but the first call asks for (link=1, rx=1, tx=0), which is impossible.
> 
> There are multiple things which can be done:
> - "echo 1 >rx" indicates error, but remembers the setting
> - "echo 1 >rx" quietly fails, without error indication. Something can
>   be written to dmesg about nonsupported mode
> - "echo 1 >rx" succeeds, but also sets tx=1
> - rx and tx are non-writable, writing always fails. Another sysfs file
>   is created, which lists modes that are actually supported, and allows
>   to select between them. When a mode is selected, link,rx,tx are
>   filled automatically, so that user may read them to know what the LED
>   is actually doing
> - something different?
> 

Expose only the supported blinking modes? (in conjunciong with a generic
traffic blinking mode)

The initial question was Should we support a mixed mode offloaed
blinking modes and blinking modes simulated by sw? I assume no as i
don't think a device that supports that exist.

> Marek

-- 
	Ansuel

Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Marek Behún]

On Mon, 8 Nov 2021 20:08:53 +0100
Ansuel Smith <ansuelsmth@gmail.com> wrote:

> > Well, if the PHY allows to manipulate the LEDs ON/OFF state (in other
> > words "full control by SW", or ability to implement brightness_set()
> > method), then netdev trigger should blink the LED in SW via this
> > mechanism (which is something it would do now). A new sysfs file,
> > "offloaded", can indicate whether the trigger is offloaded to HW or not.
> >   
> 
> Are all these sysfs entry OK? I mean if we want to add support for he
> main blinking modes, the number will increase to at least 10 additional
> entry. 

See below.

> > If, on the other hand, the LED cannot be controlled by SW, and it only
> > support some HW control modes, then there are multiple ways how to
> > implement what should be done, and we need to discuss this.
> > 
> > For example suppose that the PHY LED pin supports indicating LINK,
> > blinking on activity, or both, but it doesn't support blinking on rx
> > only, or tx only.
> > 
> > Since the LED is always indicating something about one network device,
> > the netdev trigger should be always activated for this LED and it
> > should be impossible to deactivate it. Also, it should be impossible to
> > change device_name.
> > 
> >   $ cd /sys/class/leds/<LED>
> >   $ cat device_name
> >   eth0
> >   $ echo eth1 >device_name
> >   Operation not supported.
> >   $ echo none >trigger
> >   Operation not supported.
> > 
> > Now suppose that the driver by default enabled link indication, so we
> > have:
> >   $ cat link
> >   1
> >   $ cat rx
> >   0
> >   $ cat tx
> >   0
> > 
> > We want to enable blink on activity, but the LED supports only blinking
> > on both rx/tx activity, rx only or tx only is not supported.
> > 
> > Currently the only way to enable this is to do
> >   $ echo 1 >rx
> >   $ echo 1 >tx
> > but the first call asks for (link=1, rx=1, tx=0), which is impossible.
> > 
> > There are multiple things which can be done:
> > - "echo 1 >rx" indicates error, but remembers the setting
> > - "echo 1 >rx" quietly fails, without error indication. Something can
> >   be written to dmesg about nonsupported mode
> > - "echo 1 >rx" succeeds, but also sets tx=1
> > - rx and tx are non-writable, writing always fails. Another sysfs file
> >   is created, which lists modes that are actually supported, and allows
> >   to select between them. When a mode is selected, link,rx,tx are
> >   filled automatically, so that user may read them to know what the LED
> >   is actually doing
> > - something different?
> >   
> 
> Expose only the supported blinking modes? (in conjunciong with a generic
> traffic blinking mode)

The problem with exposing only supported blinking modes is that you
can't hide rx and tx files and create a new, rxtx file. That would
break netdev trigger ABI.

> The initial question was Should we support a mixed mode offloaed
> blinking modes and blinking modes simulated by sw? I assume no as i
> don't think a device that supports that exist.

If you mean something like: the PHY can blink on tx, but not on rx, and
I want to blink on rx/tx, do I will enable HW blinking on tx, and on rx
I will blink from software, then no. If I can blink in software, then
this case should be all done in software. If software blinking is not
supported, then this setting is simply unsupported by the LED.


Let's at first implement offloading of only those things that are
supported by netdev trigger already in SW, so link, rx and tx.

When this works OK and gets merged, we can try to extend the netdev
trigger to support more modes in SW, and then implement offloading of
these modes.

Marek

Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Andrew Lunn]

> I guess I will have to work on this again ASAP or we will end up with
> solution that I don't like.
> 
> Nonetheless, what is your opinion about offloading netdev trigger vs
> introducing another trigger?

It is a solution that fits the general pattern, do it in software, and
offload it if possible.

However, i'm not sure the software solution actually works very well.
At least for switches. The two DSA drivers which implement
get_stats64() simply copy the cached statistics. The XRS700X updates
its cached values every 3000ms. The ar9331 is the same. Those are the
only two switch drivers which implement get_stats64 and none implement
get_stats. There was also was an issue that get_stats64() cannot
perform blocking calls. I don't remember if that was fixed, but if
not, get_stats64() is going to be pretty useless on switches.

We also need to handle drivers which don't actually implement
dev_get_stats(). That probably means only supporting offloads, all
modes which cannot be offloaded need to be rejected. This is pretty
much the same case of software control of the LEDs is not possible.
Unfortunately, dev_get_stats() does not return -EOPNOTSUPP, you need
to look at dev->netdev_ops->ndo_get_stats64 and
dev->netdev_ops->ndo_get_stats.

Are you working on Marvell switches? Have you implemented
get_stats64() for mv88e6xxx? How often do you poll the hardware for
the stats?

Given this, i think we need to bias the API so that it very likely
ends up offloading, if offloading is available.

     Andrew

Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Vladimir Oltean]

On Mon, Nov 08, 2021 at 08:53:36PM +0100, Andrew Lunn wrote:
> > I guess I will have to work on this again ASAP or we will end up with
> > solution that I don't like.
> > 
> > Nonetheless, what is your opinion about offloading netdev trigger vs
> > introducing another trigger?
> 
> It is a solution that fits the general pattern, do it in software, and
> offload it if possible.
> 
> However, i'm not sure the software solution actually works very well.
> At least for switches. The two DSA drivers which implement
> get_stats64() simply copy the cached statistics. The XRS700X updates
> its cached values every 3000ms. The ar9331 is the same. Those are the
> only two switch drivers which implement get_stats64 and none implement
> get_stats. There was also was an issue that get_stats64() cannot
> perform blocking calls. I don't remember if that was fixed, but if
> not, get_stats64() is going to be pretty useless on switches.

No it wasn't, I lost the interest.
I feel pretty uneasy hooking up .ndo_get_stats64() to my switches, and
basically opening the flood gates for random processes and kernel
threads to send SPI transactions back and forth like it's nothing.
Latency for programs like ptp4l and phc2sys is actually more important.

> 
> We also need to handle drivers which don't actually implement
> dev_get_stats(). That probably means only supporting offloads, all
> modes which cannot be offloaded need to be rejected. This is pretty
> much the same case of software control of the LEDs is not possible.
> Unfortunately, dev_get_stats() does not return -EOPNOTSUPP, you need
> to look at dev->netdev_ops->ndo_get_stats64 and
> dev->netdev_ops->ndo_get_stats.
> 
> Are you working on Marvell switches? Have you implemented
> get_stats64() for mv88e6xxx? How often do you poll the hardware for
> the stats?
> 
> Given this, i think we need to bias the API so that it very likely
> ends up offloading, if offloading is available.
> 
>      Andrew
> 

We could use some of the newer stats APIs exposed by Jakub if
.ndo_get_stats64 is not implemented, like ethtool_ops->get_eth_mac_stats.
Although.. see above.

Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Marek Behún]

On Mon, 8 Nov 2021 20:53:36 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > I guess I will have to work on this again ASAP or we will end up with
> > solution that I don't like.
> > 
> > Nonetheless, what is your opinion about offloading netdev trigger vs
> > introducing another trigger?  
> 
> It is a solution that fits the general pattern, do it in software, and
> offload it if possible.
> 
> However, i'm not sure the software solution actually works very well.
> At least for switches. The two DSA drivers which implement
> get_stats64() simply copy the cached statistics. The XRS700X updates
> its cached values every 3000ms. The ar9331 is the same. Those are the
> only two switch drivers which implement get_stats64 and none implement
> get_stats. There was also was an issue that get_stats64() cannot
> perform blocking calls. I don't remember if that was fixed, but if
> not, get_stats64() is going to be pretty useless on switches.
> 
> We also need to handle drivers which don't actually implement
> dev_get_stats(). That probably means only supporting offloads, all
> modes which cannot be offloaded need to be rejected. This is pretty
> much the same case of software control of the LEDs is not possible.
> Unfortunately, dev_get_stats() does not return -EOPNOTSUPP, you need
> to look at dev->netdev_ops->ndo_get_stats64 and
> dev->netdev_ops->ndo_get_stats.
> 
> Are you working on Marvell switches? Have you implemented
> get_stats64() for mv88e6xxx? How often do you poll the hardware for
> the stats?
> 
> Given this, i think we need to bias the API so that it very likely
> ends up offloading, if offloading is available.

I am working with Marvell PHYs and Marvell switches. I am aware of the
problem that SW netdev does not work for switches because we don't have
uptodate data.

It seems to me that yes, if the user wants to blink the LEDs on
activity on switch port, the netdev trigger should only work in offload
mode and only on LEDs that are connected to switch pins, unless we
implement a mechanism to get statistics at lest 10 times a second
(which could maybe be done on marvell switches by reading the registers
via ethernet frames instead of MDIO).

Marvell switches don't seem to support rx only / tx only activity
blinking, only rx/tx. I think this could be solved by making rx/tx
sysfs files for netdev trigger behave so that writing to one would also
write to another.

But since currently netdev trigger does not work for these switches, I
think making it so that it works at least to some fashion (in HW
supported modes) would still be better that current situation.

I will try to look into this, maybe work together with Ansuel.

Marek

Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Ansuel Smith]

On Mon, Nov 08, 2021 at 09:11:10PM +0100, Marek Behún wrote:
> On Mon, 8 Nov 2021 20:53:36 +0100
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > > I guess I will have to work on this again ASAP or we will end up with
> > > solution that I don't like.
> > > 
> > > Nonetheless, what is your opinion about offloading netdev trigger vs
> > > introducing another trigger?  
> > 
> > It is a solution that fits the general pattern, do it in software, and
> > offload it if possible.
> > 
> > However, i'm not sure the software solution actually works very well.
> > At least for switches. The two DSA drivers which implement
> > get_stats64() simply copy the cached statistics. The XRS700X updates
> > its cached values every 3000ms. The ar9331 is the same. Those are the
> > only two switch drivers which implement get_stats64 and none implement
> > get_stats. There was also was an issue that get_stats64() cannot
> > perform blocking calls. I don't remember if that was fixed, but if
> > not, get_stats64() is going to be pretty useless on switches.
> > 
> > We also need to handle drivers which don't actually implement
> > dev_get_stats(). That probably means only supporting offloads, all
> > modes which cannot be offloaded need to be rejected. This is pretty
> > much the same case of software control of the LEDs is not possible.
> > Unfortunately, dev_get_stats() does not return -EOPNOTSUPP, you need
> > to look at dev->netdev_ops->ndo_get_stats64 and
> > dev->netdev_ops->ndo_get_stats.
> > 
> > Are you working on Marvell switches? Have you implemented
> > get_stats64() for mv88e6xxx? How often do you poll the hardware for
> > the stats?
> > 
> > Given this, i think we need to bias the API so that it very likely
> > ends up offloading, if offloading is available.
> 
> I am working with Marvell PHYs and Marvell switches. I am aware of the
> problem that SW netdev does not work for switches because we don't have
> uptodate data.
> 
> It seems to me that yes, if the user wants to blink the LEDs on
> activity on switch port, the netdev trigger should only work in offload
> mode and only on LEDs that are connected to switch pins, unless we
> implement a mechanism to get statistics at lest 10 times a second
> (which could maybe be done on marvell switches by reading the registers
> via ethernet frames instead of MDIO).
> 
> Marvell switches don't seem to support rx only / tx only activity
> blinking, only rx/tx. I think this could be solved by making rx/tx
> sysfs files for netdev trigger behave so that writing to one would also
> write to another.
> 
> But since currently netdev trigger does not work for these switches, I
> think making it so that it works at least to some fashion (in HW
> supported modes) would still be better that current situation.
> 
> I will try to look into this, maybe work together with Ansuel.
> 
> Marek

I'm polishing some api with Andrew comments and will try to extend
netdev trigger while still proposing the additional trigger. Better
discuss in v3.
I think the main target here is first have a good generic solution for
offload and then actually thinking about working on the triggers/offload
part.

-- 
	Ansuel