Hello Andrew and Pavel,
please review these patches adding support for HW controlled LEDs.
The main difference from previous version is that the API is now generalized
and lives in drivers/leds, so that part needs to be reviewed (and maybe even
applied) by Pavel.
As discussed previously between you two, I made it so that the devicename
part of the LED is now in the form `ethernet-phy%i` when the LED is probed
for an ethernet PHY. Userspace utility wanting to control LEDs for a specific
network interface can access them via /sys/class/net/eth0/phydev/leds:
mox ~ # ls /sys/class/net/eth0/phydev/leds
ethernet-phy0:green:status ethernet-phy0:yellow:activity
mox ~ # ls /sys/class/net/eth0/phydev/leds/ethernet-phy0:green:status
brightness device hw_mode max_brightness power subsystem trigger uevent
mox ~ # cat /sys/class/net/eth0/phydev/leds/ethernet-phy0:green:status/trigger
none [dev-hw-mode] timer oneshot heartbeat default-on mmc0 mmc1
mox ~ # cat /sys/class/net/eth0/phydev/leds/ethernet-phy0:green:status/hw_mode
link link/act [1Gbps/100Mbps/10Mbps] act blink-act tx copper 1Gbps blink
Thank you.
Marek
PS: After this series is applied, we can update some device trees of various
devices which use the `marvell,reg-init` property to initialize LEDs into
specific modes so that instead of using `marvell,reg-init` they can register
LEDs via this subsystem. The `marvell,reg-init` property does not comply with
the idea that the device tree should only describe how devices are connected
to each other on the board. Maybe this property could then be proclaimed as
legacy and a warning could be printed if it is used.
Changes since v1:
- the HW controlled LEDs API is now generalized (so not only for ethernet
PHYs), and lives in drivers/leds/leds-hw-controlled.c.
I did this because I am thinking forward for when I'll be adding support
for LEDs connected to Marvell ethernet switches (mv88e6xxx driver).
The LEDs there should be described as descendants of the `port` nodes, not
`phy` nodes, because:
- some ports don't have PHYs and yet can have LEDs
- some ports have SERDES PHYs which are currently not described in any
way in device-tree
- some LEDs can be made to blink on activity/other event on multiple
ports at once
- hence the private LED trigger was renamed from `phydev-hw-mode` to
`dev-hw-mode`
- the `led-open-drain` DT property was renamed to `led-tristate` property,
because I learned that the two things mean something different and in
Marvell PHYs the polarity on off state can be put into tristate mode, not
open drain mode
- the devicename part of PHY LEDs is now in the format `ethernet-phy%i`,
instead of `phy%i`
- the code adding `phyindex` member to struct phy_device is now in separate
patch
- YAML device-tree binding schema for HW controlled LEDs now lives in it's
own file and the ethernet-phy.yaml file now contains a reference to the
this schema
- added a patch adding nodes for PHY controlled LEDs for Turris MOX' device
tree
Changes since RFC v4:
- added device-tree binding documentation.
- the OF code now checks for linux,default-hw-mode property so that
default HW mode can be set in device tree (like linux,default-trigger)
(this was suggested by Andrew)
- the OF code also checks for enable-active-high and led-open-drain
properties, and the marvell PHY driver now understands uses these
settings when initializing the LEDs
- the LED operations were moved to their own struct phy_device_led_ops
- a new member was added into struct phy_device: phyindex. This is an
incrementing integer, new for each registered phy_device. This is used
for a simple naming scheme for the devicename part of a LED, as was
suggested in a discussion by Andrew and Pavel. A PHY controlled LED
now has a name in form:
phy%i:color:function
When a PHY is attached to a netdevice, userspace can control available
PHY controlled LEDs via /sys/class/net/<ifname>/phydev/leds/
- legacy LED configuration in Marvell PHY driver (in function
marvell_config_led) now writes only to registers which do not
correspond to any registered LED
Changes since RFC v3:
- addressed some of Andrew's suggestions
- phy_hw_led_mode.c renamed to phy_led.c
- the DT reading code is now also generic, moved to phy_led.c and called
from phy_probe
- the function registering the phydev-hw-mode trigger is now called from
phy_device.c function phy_init before registering genphy drivers
- PHY LED functionality now depends on CONFIG_LEDS_TRIGGERS
Changes since RFC v2:
- to share code with other drivers which may want to also offer PHY HW
control of LEDs some of the code was refactored and now resides in
phy_hw_led_mode.c. This code is compiled in when config option
LED_TRIGGER_PHY_HW is enabled. Drivers wanting to offer PHY HW control
of LEDs should depend on this option.
- the "hw-control" trigger is renamed to "phydev-hw-mode" and is
registered by the code in phy_hw_led_mode.c
- the "hw_control" sysfs file is renamed to "hw_mode"
- struct phy_driver is extended by three methods to support PHY HW LED
control
- I renamed the various HW control modes offeret by Marvell PHYs to
conform to other Linux mode names, for example the "1000/100/10/else"
mode was renamed to "1Gbps/100Mbps/10Mbps", or "recv/else" was renamed
to "rx" (this is the name of the mode in netdev trigger).
Marek Behún (7):
dt-bindings: leds: document binding for HW controlled LEDs
leds: add generic API for LEDs that can be controlled by hardware
net: phy: add simple incrementing phyindex member to phy_device struct
dt-bindings: net: ethernet-phy: add description for PHY LEDs
net: phy: add support for LEDs controlled by ethernet PHY chips
net: phy: marvell: add support for LEDs controlled by Marvell PHYs
arm64: dts: armada-3720-turris-mox: add nodes for ethernet PHY LEDs
.../sysfs-class-led-trigger-dev-hw-mode | 8 +
.../leds/linux,hw-controlled-leds.yaml | 99 ++++++
.../devicetree/bindings/net/ethernet-phy.yaml | 8 +
.../dts/marvell/armada-3720-turris-mox.dts | 23 ++
drivers/leds/Kconfig | 10 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-hw-controlled.c | 227 +++++++++++++
drivers/net/phy/marvell.c | 314 +++++++++++++++++-
drivers/net/phy/phy_device.c | 106 ++++++
include/linux/leds-hw-controlled.h | 74 +++++
include/linux/phy.h | 7 +
11 files changed, 875 insertions(+), 2 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode
create mode 100644 Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
create mode 100644 drivers/leds/leds-hw-controlled.c
create mode 100644 include/linux/leds-hw-controlled.h
--
2.26.2
Add nodes for the green and yellow LEDs that are connected to the
ethernet PHY chip on Turris MOX A.
Signed-off-by: Marek Behún <[email protected]>
---
.../dts/marvell/armada-3720-turris-mox.dts | 23 +++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
index f3a678e0fd99b..6da03b6c69c0a 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
@@ -9,6 +9,7 @@
#include <dt-bindings/bus/moxtet.h>
#include <dt-bindings/gpio/gpio.h>
#include <dt-bindings/input/input.h>
+#include <dt-bindings/leds/common.h>
#include "armada-372x.dtsi"
/ {
@@ -273,6 +274,28 @@ &mdio {
phy1: ethernet-phy@1 {
reg = <1>;
+
+ leds {
+ compatible = "linux,hw-controlled-leds";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_STATUS;
+ linux,default-trigger = "dev-hw-mode";
+ linux,default-hw-mode = "1Gbps/100Mbps/10Mbps";
+ };
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_YELLOW>;
+ function = LED_FUNCTION_ACTIVITY;
+ linux,default-trigger = "dev-hw-mode";
+ linux,default-hw-mode = "blink-act";
+ };
+ };
};
/* switch nodes are enabled by U-Boot if modules are present */
--
2.26.2
Add a new integer member phyindex to struct phy_device. This member is
unique for every phy_device. Atomic incrementation occurs in
phy_device_register.
This can be used for example in LED sysfs API. The LED subsystem names
each LED in format `device:color:function`, but currently the PHY device
names are not suited for this, since in some situations a PHY device
name can look like this
d0032004.mdio-mii:01
or even like this
/soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08
Clearly this cannot be used as the `device` part of a LED name.
Signed-off-by: Marek Behún <[email protected]>
---
drivers/net/phy/phy_device.c | 3 +++
include/linux/phy.h | 3 +++
2 files changed, 6 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8adfbad0a1e8f..38f56d39f1229 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -9,6 +9,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/atomic.h>
#include <linux/bitmap.h>
#include <linux/delay.h>
#include <linux/errno.h>
@@ -892,6 +893,7 @@ EXPORT_SYMBOL(get_phy_device);
*/
int phy_device_register(struct phy_device *phydev)
{
+ static atomic_t phyindex;
int err;
err = mdiobus_register_device(&phydev->mdio);
@@ -908,6 +910,7 @@ int phy_device_register(struct phy_device *phydev)
goto out;
}
+ phydev->phyindex = atomic_inc_return(&phyindex) - 1;
err = device_add(&phydev->mdio.dev);
if (err) {
phydev_err(phydev, "failed to add\n");
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3a09d2bf69ea4..52881e21ad951 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -406,6 +406,7 @@ struct macsec_ops;
/* phy_device: An instance of a PHY
*
* drv: Pointer to the driver for this PHY instance
+ * phyindex: a simple incrementing PHY index
* phy_id: UID for this device found during discovery
* c45_ids: 802.3-c45 Device Identifers if is_c45.
* is_c45: Set to true if this phy uses clause 45 addressing.
@@ -446,6 +447,8 @@ struct phy_device {
/* And management functions */
struct phy_driver *drv;
+ int phyindex;
+
u32 phy_id;
struct phy_c45_device_ids c45_ids;
--
2.26.2
Many an ethernet PHY (and other chips) supports various HW control modes
for LEDs connected directly to them.
This patch adds a generic API for registering such LEDs when described
in device tree. This API also exposes generic way to select between
these hardware control modes.
This API registers a new private LED trigger called dev-hw-mode. When
this trigger is enabled for a LED, the various HW control modes which
are supported by the device for given LED can be get/set via hw_mode
sysfs file.
Signed-off-by: Marek Behún <[email protected]>
---
.../sysfs-class-led-trigger-dev-hw-mode | 8 +
drivers/leds/Kconfig | 10 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-hw-controlled.c | 227 ++++++++++++++++++
include/linux/leds-hw-controlled.h | 74 ++++++
5 files changed, 320 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode
create mode 100644 drivers/leds/leds-hw-controlled.c
create mode 100644 include/linux/leds-hw-controlled.h
diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode b/Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode
new file mode 100644
index 0000000000000..7bca112e7ff93
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode
@@ -0,0 +1,8 @@
+What: /sys/class/leds/<led>/hw_mode
+Date: September 2020
+KernelVersion: 5.10
+Contact: Marek Behún <[email protected]>
+ [email protected]
+Description: (W) Set the HW control mode of this LED. The various available HW control modes
+ are specific per device to which the LED is connected to and per LED itself.
+ (R) Show the available HW control modes and the currently selected one.
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1c181df24eae4..5e47ab21aafb4 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -49,6 +49,16 @@ config LEDS_BRIGHTNESS_HW_CHANGED
See Documentation/ABI/testing/sysfs-class-led for details.
+config LEDS_HW_CONTROLLED
+ bool "API for LEDs that can be controlled by hardware"
+ depends on LEDS_CLASS
+ select LEDS_TRIGGERS
+ help
+ This option enables support for a generic API via which other drivers
+ can register LEDs that can be put into hardware controlled mode, eg.
+ a LED connected to an ethernet PHY can be configured to blink on
+ network activity.
+
comment "LED drivers"
config LEDS_88PM860X
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index c2c7d7ade0d06..858e468e40df0 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_LEDS_CLASS) += led-class.o
obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o
obj-$(CONFIG_LEDS_CLASS_MULTICOLOR) += led-class-multicolor.o
obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.o
+obj-$(CONFIG_LEDS_HW_CONTROLLED) += leds-hw-controlled.o
# LED Platform Drivers (keep this sorted, M-| sort)
obj-$(CONFIG_LEDS_88PM860X) += leds-88pm860x.o
diff --git a/drivers/leds/leds-hw-controlled.c b/drivers/leds/leds-hw-controlled.c
new file mode 100644
index 0000000000000..9ef58bf275efd
--- /dev/null
+++ b/drivers/leds/leds-hw-controlled.c
@@ -0,0 +1,227 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Generic API for LEDs that can be controlled by hardware (eg. by an ethernet PHY chip)
+ *
+ * Copyright (C) 2020 Marek Behun <[email protected]>
+ */
+#include <linux/leds-hw-controlled.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+int hw_controlled_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness)
+{
+ struct hw_controlled_led *led = led_cdev_to_hw_controlled_led(cdev);
+ int ret;
+
+ mutex_lock(&led->lock);
+ ret = led->ops->led_brightness_set(cdev->dev->parent, led, brightness);
+ mutex_unlock(&led->lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(hw_controlled_led_brightness_set);
+
+static int of_register_hw_controlled_led(struct device *dev, struct device_node *np,
+ const char *devicename,
+ const struct hw_controlled_led_ops *ops)
+{
+ struct led_init_data init_data = {};
+ struct hw_controlled_led *led;
+ u32 reg;
+ int ret;
+
+ ret = of_property_read_u32(np, "reg", ®);
+ if (ret < 0)
+ return ret;
+
+ led = devm_kzalloc(dev, sizeof(struct hw_controlled_led), GFP_KERNEL);
+ if (!led)
+ return -ENOMEM;
+
+ led->ops = ops;
+
+ led->cdev.max_brightness = 1;
+ led->cdev.brightness_set_blocking = hw_controlled_led_brightness_set;
+ led->cdev.trigger_type = &hw_control_led_trig_type;
+ led->addr = reg;
+
+ of_property_read_string(np, "linux,default-trigger", &led->cdev.default_trigger);
+ of_property_read_string(np, "linux,default-hw-mode", &led->hw_mode);
+
+ led->active_low = !of_property_read_bool(np, "enable-active-high");
+ led->tristate = of_property_read_bool(np, "led-tristate");
+
+ init_data.fwnode = &np->fwnode;
+ init_data.devname_mandatory = true;
+ init_data.devicename = devicename;
+
+ ret = led->ops->led_init(dev, led);
+ if (ret < 0)
+ goto err_free;
+
+ ret = devm_led_classdev_register_ext(dev, &led->cdev, &init_data);
+ if (ret < 0)
+ goto err_free;
+
+ return 0;
+err_free:
+ devm_kfree(dev, led);
+ return ret;
+}
+
+int of_register_hw_controlled_leds(struct device *dev, const char *devicename,
+ const struct hw_controlled_led_ops *ops)
+{
+ struct device_node *node = dev->of_node;
+ struct device_node *leds, *led;
+ int ret;
+
+ if (!IS_ENABLED(CONFIG_OF_MDIO))
+ return 0;
+
+ if (!ops)
+ return -EINVAL;
+
+ /* maybe we should have of_get_compatible_available_child as well */
+ leds = of_get_compatible_child(node, "linux,hw-controlled-leds");
+ if (!leds)
+ return 0;
+
+ if (!devicename)
+ devicename = dev_name(dev);
+
+ for_each_available_child_of_node(leds, led) {
+ ret = of_register_hw_controlled_led(dev, led, devicename, ops);
+ if (ret < 0)
+ dev_err(dev, "Nonfatal error: cannot register LED from node %pOFn: %i\n",
+ led, ret);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_register_hw_controlled_leds);
+
+static int hw_control_led_trig_activate(struct led_classdev *cdev)
+{
+ struct hw_controlled_led *led;
+ int ret;
+
+ led = led_cdev_to_hw_controlled_led(cdev);
+
+ if (!led->hw_mode)
+ return 0;
+
+ mutex_lock(&led->lock);
+ ret = led->ops->led_set_hw_mode(cdev->dev->parent, led, led->hw_mode);
+ mutex_unlock(&led->lock);
+
+ if (ret < 0)
+ dev_warn(cdev->dev->parent, "Could not set HW mode %s on LED %s: %i\n",
+ led->hw_mode, cdev->name, ret);
+
+ /* don't fail to activate this trigger so that user can write hw_mode file */
+ return 0;
+}
+
+static void hw_control_led_trig_deactivate(struct led_classdev *cdev)
+{
+ struct hw_controlled_led *led;
+ int ret;
+
+ led = led_cdev_to_hw_controlled_led(cdev);
+
+ mutex_lock(&led->lock);
+ /* store HW mode before deactivation */
+ led->hw_mode = led->ops->led_get_hw_mode(cdev->dev->parent, led);
+ ret = led->ops->led_set_hw_mode(cdev->dev->parent, led, NULL);
+ mutex_unlock(&led->lock);
+
+ if (ret < 0)
+ dev_err(cdev->dev->parent, "Failed deactivating HW mode on LED %s\n", cdev->name);
+}
+
+static ssize_t hw_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct hw_controlled_led *led;
+ const char *mode, *cur_mode;
+ void *iter = NULL;
+ int len = 0;
+
+ led = led_cdev_to_hw_controlled_led(led_trigger_get_led(dev));
+
+ mutex_lock(&led->lock);
+
+ cur_mode = led->ops->led_get_hw_mode(dev->parent, led);
+
+ for (mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter);
+ mode;
+ mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter)) {
+ bool sel;
+
+ sel = cur_mode && !strcmp(mode, cur_mode);
+
+ len += scnprintf(buf + len, PAGE_SIZE - len, "%s%s%s ", sel ? "[" : "", mode,
+ sel ? "]" : "");
+ }
+
+ if (buf[len - 1] == ' ')
+ buf[len - 1] = '\n';
+
+ mutex_unlock(&led->lock);
+
+ return len;
+}
+
+static ssize_t hw_mode_store(struct device *dev, struct device_attribute *attr, const char *buf,
+ size_t count)
+{
+ struct hw_controlled_led *led;
+ int ret;
+
+ led = led_cdev_to_hw_controlled_led(led_trigger_get_led(dev));
+
+ mutex_lock(&led->lock);
+ ret = led->ops->led_set_hw_mode(dev->parent, led, buf);
+ if (ret < 0)
+ return ret;
+ mutex_unlock(&led->lock);
+
+ return count;
+}
+
+static DEVICE_ATTR_RW(hw_mode);
+
+static struct attribute *hw_control_led_trig_attrs[] = {
+ &dev_attr_hw_mode.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(hw_control_led_trig);
+
+struct led_hw_trigger_type hw_control_led_trig_type;
+EXPORT_SYMBOL_GPL(hw_control_led_trig_type);
+
+struct led_trigger hw_control_led_trig = {
+ .name = "dev-hw-mode",
+ .activate = hw_control_led_trig_activate,
+ .deactivate = hw_control_led_trig_deactivate,
+ .trigger_type = &hw_control_led_trig_type,
+ .groups = hw_control_led_trig_groups,
+};
+EXPORT_SYMBOL_GPL(hw_control_led_trig);
+
+static int __init hw_controlled_leds_init(void)
+{
+ return led_trigger_register(&hw_control_led_trig);
+}
+
+static void __exit hw_controlled_leds_exit(void)
+{
+ led_trigger_unregister(&hw_control_led_trig);
+}
+
+subsys_initcall(hw_controlled_leds_init);
+module_exit(hw_controlled_leds_exit);
+
+MODULE_AUTHOR("Marek Behun <[email protected]>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("API for HW controlled LEDs");
diff --git a/include/linux/leds-hw-controlled.h b/include/linux/leds-hw-controlled.h
new file mode 100644
index 0000000000000..2c9b8a06def18
--- /dev/null
+++ b/include/linux/leds-hw-controlled.h
@@ -0,0 +1,74 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Generic API for LEDs that can be controlled by hardware (eg. by an ethernet PHY chip)
+ *
+ * Copyright (C) 2020 Marek Behun <[email protected]>
+ */
+#ifndef _LINUX_LEDS_HW_CONTROLLED_H_
+#define _LINUX_LEDS_HW_CONTROLLED_H_
+
+#include <linux/kernel.h>
+#include <linux/leds.h>
+
+struct hw_controlled_led {
+ struct led_classdev cdev;
+ const struct hw_controlled_led_ops *ops;
+ struct mutex lock;
+
+ /* these members are filled in by OF if OF is enabled */
+ int addr;
+ bool active_low;
+ bool tristate;
+
+ /* also filled in by OF, but changed by led_set_hw_mode operation */
+ const char *hw_mode;
+
+ void *priv;
+};
+#define led_cdev_to_hw_controlled_led(l) container_of(l, struct hw_controlled_led, cdev)
+
+/* struct hw_controlled_led_ops: Operations on LEDs that can be controlled by HW
+ *
+ * All the following operations must be implemented:
+ * @led_init: Should initialize the LED from OF data (and sanity check whether they are correct).
+ * This should also change led->cdev.max_brightness, if the value differs from default,
+ * which is 1.
+ * @led_brightness_set: Sets brightness.
+ * @led_iter_hw_mode: Iterates available HW control mode names for this LED.
+ * @led_set_hw_mode: Sets HW control mode to value specified by given name.
+ * @led_get_hw_mode: Returns current HW control mode name.
+ */
+struct hw_controlled_led_ops {
+ int (*led_init)(struct device *dev, struct hw_controlled_led *led);
+ int (*led_brightness_set)(struct device *dev, struct hw_controlled_led *led,
+ enum led_brightness brightness);
+ const char *(*led_iter_hw_mode)(struct device *dev, struct hw_controlled_led *led,
+ void **iter);
+ int (*led_set_hw_mode)(struct device *dev, struct hw_controlled_led *led,
+ const char *mode);
+ const char *(*led_get_hw_mode)(struct device *dev, struct hw_controlled_led *led);
+};
+
+#if IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED)
+
+#define hw_controlled_led_ops_ptr(s) (s)
+
+int of_register_hw_controlled_leds(struct device *dev, const char *devicename,
+ const struct hw_controlled_led_ops *ops);
+int hw_controlled_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness);
+
+extern struct led_hw_trigger_type hw_control_led_trig_type;
+extern struct led_trigger hw_control_led_trig;
+
+#else /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */
+
+#define hw_controlled_led_ops_ptr(s) NULL
+static inline int of_register_hw_controlled_leds(struct device *dev, const char *devicename,
+ const struct hw_controlled_led_ops *ops)
+{
+ return 0;
+}
+
+#endif /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */
+
+#endif /* _LINUX_LEDS_HW_CONTROLLED_H_ */
--
2.26.2
Document binding for LEDs connected to an ethernet PHY chip.
Signed-off-by: Marek Behún <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: [email protected]
---
Documentation/devicetree/bindings/net/ethernet-phy.yaml | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index a9e547ac79051..f593e8709dd0d 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -174,6 +174,14 @@ properties:
PHY's that have configurable TX internal delays. If this property is
present then the PHY applies the TX delay.
+ leds:
+ type: object
+ description: |
+ This is used to described LEDs that are connected to the PHY chip and
+ their blinking can be controlled by the PHY.
+ allOf:
+ - $ref: /schemas/leds/linux,hw-controlled-leds.yaml#
+
required:
- reg
--
2.26.2
This patch adds support for controlling the LEDs connected to several
families of Marvell PHYs via the PHY HW LED trigger API. These families
are: 88E1112, 88E1121R, 88E1240, 88E1340S, 88E1510 and 88E1545. More can
be added.
This patch does not yet add support for compound LED modes. This could
be achieved via the LED multicolor framework.
Settings such as HW blink rate or pulse stretch duration are not yet
supported.
Signed-off-by: Marek Behún <[email protected]>
---
drivers/net/phy/marvell.c | 314 +++++++++++++++++++++++++++++++++++++-
1 file changed, 312 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index bb86ac0bd0920..7aedb529e1540 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -148,6 +148,13 @@
#define MII_88E1510_PHY_LED_DEF 0x1177
#define MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE 0x1040
+#define MII_PHY_LED_POLARITY_CTRL 17
+#define MII_PHY_LED_TIMER_CTRL 18
+#define MII_PHY_LED45_CTRL 19
+
+#define MII_PHY_LED_CTRL_FORCE_ON 0x9
+#define MII_PHY_LED_CTRL_FORCE_OFF 0x8
+
#define MII_M1011_PHY_STATUS 0x11
#define MII_M1011_PHY_STATUS_1000 0x8000
#define MII_M1011_PHY_STATUS_100 0x4000
@@ -252,6 +259,8 @@
#define LPA_PAUSE_FIBER 0x180
#define LPA_PAUSE_ASYM_FIBER 0x100
+#define MARVELL_PHY_MAX_LEDS 6
+
#define NB_FIBER_STATS 1
MODULE_DESCRIPTION("Marvell PHY driver");
@@ -280,6 +289,7 @@ struct marvell_priv {
u32 last;
u32 step;
s8 pair;
+ u16 legacy_led_config_mask;
};
static int marvell_read_page(struct phy_device *phydev)
@@ -662,8 +672,300 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
return err;
}
+#if IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED)
+
+enum {
+ COMMON = BIT(0),
+ L1V0_RECV = BIT(1),
+ L1V0_COPPER = BIT(2),
+ L1V5_100_FIBER = BIT(3),
+ L1V5_100_10 = BIT(4),
+ L2V2_INIT = BIT(5),
+ L2V2_PTP = BIT(6),
+ L2V2_DUPLEX = BIT(7),
+ L3V0_FIBER = BIT(8),
+ L3V0_LOS = BIT(9),
+ L3V5_TRANS = BIT(10),
+ L3V7_FIBER = BIT(11),
+ L3V7_DUPLEX = BIT(12),
+};
+
+struct marvell_led_mode_info {
+ const char *name;
+ s8 regval[MARVELL_PHY_MAX_LEDS];
+ u32 flags;
+};
+
+static const struct marvell_led_mode_info marvell_led_mode_info[] = {
+ { "link", { 0x0, -1, 0x0, -1, -1, -1, }, COMMON },
+ { "link/act", { 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, COMMON },
+ { "1Gbps/100Mbps/10Mbps", { 0x2, -1, -1, -1, -1, -1, }, COMMON },
+ { "act", { 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, COMMON },
+ { "blink-act", { 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, COMMON },
+ { "tx", { 0x5, -1, 0x5, -1, 0x5, 0x5, }, COMMON },
+ { "tx", { -1, -1, -1, 0x5, -1, -1, }, L3V5_TRANS },
+ { "rx", { -1, -1, -1, -1, 0x0, 0x0, }, COMMON },
+ { "rx", { -1, 0x0, -1, -1, -1, -1, }, L1V0_RECV },
+ { "copper", { 0x6, -1, -1, -1, -1, -1, }, COMMON },
+ { "copper", { -1, 0x0, -1, -1, -1, -1, }, L1V0_COPPER },
+ { "1Gbps", { 0x7, -1, -1, -1, -1, -1, }, COMMON },
+ { "link/rx", { -1, 0x2, -1, 0x2, 0x2, 0x2, }, COMMON },
+ { "100Mbps-fiber", { -1, 0x5, -1, -1, -1, -1, }, L1V5_100_FIBER },
+ { "100Mbps-10Mbps", { -1, 0x5, -1, -1, -1, -1, }, L1V5_100_10 },
+ { "1Gbps-100Mbps", { -1, 0x6, -1, -1, -1, -1, }, COMMON },
+ { "1Gbps-10Mbps", { -1, -1, 0x6, 0x6, -1, -1, }, COMMON },
+ { "100Mbps", { -1, 0x7, -1, -1, -1, -1, }, COMMON },
+ { "10Mbps", { -1, -1, 0x7, -1, -1, -1, }, COMMON },
+ { "fiber", { -1, -1, -1, 0x0, -1, -1, }, L3V0_FIBER },
+ { "fiber", { -1, -1, -1, 0x7, -1, -1, }, L3V7_FIBER },
+ { "FullDuplex", { -1, -1, -1, 0x7, -1, -1, }, L3V7_DUPLEX },
+ { "FullDuplex", { -1, -1, -1, -1, 0x6, 0x6, }, COMMON },
+ { "FullDuplex/collision", { -1, -1, -1, -1, 0x7, 0x7, }, COMMON },
+ { "FullDuplex/collision", { -1, -1, 0x2, -1, -1, -1, }, L2V2_DUPLEX },
+ { "ptp", { -1, -1, 0x2, -1, -1, -1, }, L2V2_PTP },
+ { "init", { -1, -1, 0x2, -1, -1, -1, }, L2V2_INIT },
+ { "los", { -1, -1, -1, 0x0, -1, -1, }, L3V0_LOS },
+ { "blink", { 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, COMMON },
+};
+
+struct marvell_leds_info {
+ u32 family;
+ int nleds;
+ u32 flags;
+};
+
+#define LED(fam, n, flg) \
+ { \
+ .family = MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E##fam), \
+ .nleds = (n), \
+ .flags = (flg), \
+ } \
+
+static const struct marvell_leds_info marvell_leds_info[] = {
+ LED(1112, 4, COMMON | L1V0_COPPER | L1V5_100_FIBER | L2V2_INIT | L3V0_LOS | L3V5_TRANS |
+ L3V7_FIBER),
+ LED(1121R, 3, COMMON | L1V5_100_10),
+ LED(1240, 6, COMMON | L3V5_TRANS),
+ LED(1340S, 6, COMMON | L1V0_COPPER | L1V5_100_FIBER | L2V2_PTP | L3V0_FIBER | L3V7_DUPLEX),
+ LED(1510, 3, COMMON | L1V0_RECV | L1V5_100_FIBER | L2V2_DUPLEX),
+ LED(1545, 6, COMMON | L1V0_COPPER | L1V5_100_FIBER | L3V0_FIBER | L3V7_DUPLEX),
+};
+
+static inline int marvell_led_reg(int led)
+{
+ switch (led) {
+ case 0 ... 3:
+ return MII_PHY_LED_CTRL;
+ case 4 ... 5:
+ return MII_PHY_LED45_CTRL;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int marvell_led_set_regval(struct phy_device *phydev, int led, u16 val)
+{
+ u16 mask;
+ int reg;
+
+ reg = marvell_led_reg(led);
+ if (reg < 0)
+ return reg;
+
+ val <<= (led % 4) * 4;
+ mask = 0xf << ((led % 4) * 4);
+
+ return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, reg, mask, val);
+}
+
+static int marvell_led_get_regval(struct phy_device *phydev, int led)
+{
+ int reg, val;
+
+ reg = marvell_led_reg(led);
+ if (reg < 0)
+ return reg;
+
+ val = phy_read_paged(phydev, MII_MARVELL_LED_PAGE, reg);
+ if (val < 0)
+ return val;
+
+ val >>= (led % 4) * 4;
+ val &= 0xf;
+
+ return val;
+}
+
+static int marvell_led_set_polarity(struct phy_device *phydev, int led, bool active_low,
+ bool tristate)
+{
+ int reg, shift;
+ u16 mask, val;
+
+ switch (led) {
+ case 0 ... 3:
+ reg = MII_PHY_LED_POLARITY_CTRL;
+ break;
+ case 4 ... 5:
+ reg = MII_PHY_LED45_CTRL;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ val = 0;
+ if (!active_low)
+ val |= BIT(0);
+ if (tristate)
+ val |= BIT(1);
+
+ shift = led * 2;
+ val <<= shift;
+ mask = 0x3 << shift;
+
+ return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, reg, mask, val);
+}
+
+static int marvell_led_brightness_set(struct device *dev, struct hw_controlled_led *led,
+ enum led_brightness brightness)
+{
+ struct phy_device *phydev = to_phy_device(dev);
+ u8 val;
+
+ /* don't do anything if HW control is enabled */
+ if (led->cdev.trigger == &hw_control_led_trig)
+ return 0;
+
+ val = brightness ? MII_PHY_LED_CTRL_FORCE_ON : MII_PHY_LED_CTRL_FORCE_OFF;
+
+ return marvell_led_set_regval(phydev, led->addr, val);
+}
+
+static inline bool is_valid_led_mode(struct hw_controlled_led *led,
+ const struct marvell_led_mode_info *mode)
+{
+ const struct marvell_leds_info *info = led->priv;
+
+ return mode->regval[led->addr] != -1 && (info->flags & mode->flags);
+}
+
+static const char *marvell_led_iter_hw_mode(struct device *dev, struct hw_controlled_led *led,
+ void **iter)
+{
+ const struct marvell_led_mode_info *mode = *iter;
+
+ if (!mode)
+ mode = marvell_led_mode_info;
+
+ if (mode - marvell_led_mode_info == ARRAY_SIZE(marvell_led_mode_info))
+ goto end;
+
+ while (!is_valid_led_mode(led, mode)) {
+ ++mode;
+ if (mode - marvell_led_mode_info == ARRAY_SIZE(marvell_led_mode_info))
+ goto end;
+ }
+
+ *iter = (void *)(mode + 1);
+ return mode->name;
+end:
+ *iter = NULL;
+ return NULL;
+}
+
+static int marvell_led_set_hw_mode(struct device *dev, struct hw_controlled_led *led,
+ const char *name)
+{
+ struct phy_device *phydev = to_phy_device(dev);
+ const struct marvell_led_mode_info *mode;
+ int i;
+
+ if (!name)
+ return 0;
+
+ for (i = 0; i < ARRAY_SIZE(marvell_led_mode_info); ++i) {
+ mode = &marvell_led_mode_info[i];
+
+ if (!is_valid_led_mode(led, mode))
+ continue;
+
+ if (sysfs_streq(name, mode->name))
+ return marvell_led_set_regval(phydev, led->addr, mode->regval[led->addr]);
+ }
+
+ return -EINVAL;
+}
+
+static const char *marvell_led_get_hw_mode(struct device *dev, struct hw_controlled_led *led)
+{
+ struct phy_device *phydev = to_phy_device(dev);
+ const struct marvell_led_mode_info *mode;
+ int i, regval;
+
+ regval = marvell_led_get_regval(phydev, led->addr);
+ if (regval < 0)
+ return NULL;
+
+ for (i = 0; i < ARRAY_SIZE(marvell_led_mode_info); ++i) {
+ mode = &marvell_led_mode_info[i];
+
+ if (!is_valid_led_mode(led, mode))
+ continue;
+
+ if (mode->regval[led->addr] == regval)
+ return mode->name;
+ }
+
+ return NULL;
+}
+
+static int marvell_led_init(struct device *dev, struct hw_controlled_led *led)
+{
+ struct phy_device *phydev = to_phy_device(dev);
+ const struct marvell_leds_info *info = NULL;
+ struct marvell_priv *priv = phydev->priv;
+ int ret, i;
+
+ for (i = 0; i < ARRAY_SIZE(marvell_leds_info); ++i) {
+ if (MARVELL_PHY_FAMILY_ID(phydev->phy_id) == marvell_leds_info[i].family) {
+ info = &marvell_leds_info[i];
+ break;
+ }
+ }
+
+ if (!info)
+ return -EOPNOTSUPP;
+
+ if (led->addr >= info->nleds)
+ return -EINVAL;
+
+ led->priv = (void *)info;
+ led->cdev.max_brightness = 1;
+
+ ret = marvell_led_set_polarity(phydev, led->addr, led->active_low, led->tristate);
+ if (ret < 0)
+ return ret;
+
+ /* ensure marvell_config_led below does not change settings we have set for this LED */
+ if (led->addr < 3)
+ priv->legacy_led_config_mask &= ~(0xf << (led->addr * 4));
+
+ return 0;
+}
+
+static const struct hw_controlled_led_ops marvell_led_ops = {
+ .led_init = marvell_led_init,
+ .led_brightness_set = marvell_led_brightness_set,
+ .led_iter_hw_mode = marvell_led_iter_hw_mode,
+ .led_set_hw_mode = marvell_led_set_hw_mode,
+ .led_get_hw_mode = marvell_led_get_hw_mode,
+};
+
+#endif /* IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */
+
static void marvell_config_led(struct phy_device *phydev)
{
+ struct marvell_priv *priv = phydev->priv;
u16 def_config;
int err;
@@ -688,8 +990,9 @@ static void marvell_config_led(struct phy_device *phydev)
return;
}
- err = phy_write_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL,
- def_config);
+ def_config &= priv->legacy_led_config_mask;
+ err = phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL,
+ priv->legacy_led_config_mask, def_config);
if (err < 0)
phydev_warn(phydev, "Fail to config marvell phy LED.\n");
}
@@ -2580,6 +2883,7 @@ static int marvell_probe(struct phy_device *phydev)
if (!priv)
return -ENOMEM;
+ priv->legacy_led_config_mask = 0xffff;
phydev->priv = priv;
return 0;
@@ -2656,6 +2960,7 @@ static struct phy_driver marvell_drivers[] = {
.get_stats = marvell_get_stats,
.get_tunable = m88e1011_get_tunable,
.set_tunable = m88e1011_set_tunable,
+ .led_ops = hw_controlled_led_ops_ptr(&marvell_led_ops),
},
{
.phy_id = MARVELL_PHY_ID_88E1111,
@@ -2717,6 +3022,7 @@ static struct phy_driver marvell_drivers[] = {
.get_stats = marvell_get_stats,
.get_tunable = m88e1011_get_tunable,
.set_tunable = m88e1011_set_tunable,
+ .led_ops = hw_controlled_led_ops_ptr(&marvell_led_ops),
},
{
.phy_id = MARVELL_PHY_ID_88E1318S,
@@ -2796,6 +3102,7 @@ static struct phy_driver marvell_drivers[] = {
.get_sset_count = marvell_get_sset_count,
.get_strings = marvell_get_strings,
.get_stats = marvell_get_stats,
+ .led_ops = hw_controlled_led_ops_ptr(&marvell_led_ops),
},
{
.phy_id = MARVELL_PHY_ID_88E1116R,
@@ -2844,6 +3151,7 @@ static struct phy_driver marvell_drivers[] = {
.cable_test_start = marvell_vct7_cable_test_start,
.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
.cable_test_get_status = marvell_vct7_cable_test_get_status,
+ .led_ops = hw_controlled_led_ops_ptr(&marvell_led_ops),
},
{
.phy_id = MARVELL_PHY_ID_88E1540,
@@ -2896,6 +3204,7 @@ static struct phy_driver marvell_drivers[] = {
.cable_test_start = marvell_vct7_cable_test_start,
.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
.cable_test_get_status = marvell_vct7_cable_test_get_status,
+ .led_ops = hw_controlled_led_ops_ptr(&marvell_led_ops),
},
{
.phy_id = MARVELL_PHY_ID_88E3016,
@@ -2964,6 +3273,7 @@ static struct phy_driver marvell_drivers[] = {
.get_stats = marvell_get_stats,
.get_tunable = m88e1540_get_tunable,
.set_tunable = m88e1540_set_tunable,
+ .led_ops = hw_controlled_led_ops_ptr(&marvell_led_ops),
},
{
.phy_id = MARVELL_PHY_ID_88E1548P,
--
2.26.2
Hi,
On 9/9/20 9:25 AM, Marek Behún wrote:
> Many an ethernet PHY (and other chips) supports various HW control modes
> for LEDs connected directly to them.
>
> This patch adds a generic API for registering such LEDs when described
> in device tree. This API also exposes generic way to select between
> these hardware control modes.
>
> This API registers a new private LED trigger called dev-hw-mode. When
> this trigger is enabled for a LED, the various HW control modes which
> are supported by the device for given LED can be get/set via hw_mode
> sysfs file.
>
> Signed-off-by: Marek Behún <[email protected]>
> ---
> .../sysfs-class-led-trigger-dev-hw-mode | 8 +
> drivers/leds/Kconfig | 10 +
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-hw-controlled.c | 227 ++++++++++++++++++
> include/linux/leds-hw-controlled.h | 74 ++++++
> 5 files changed, 320 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode
> create mode 100644 drivers/leds/leds-hw-controlled.c
> create mode 100644 include/linux/leds-hw-controlled.h
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 1c181df24eae4..5e47ab21aafb4 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -49,6 +49,16 @@ config LEDS_BRIGHTNESS_HW_CHANGED
>
> See Documentation/ABI/testing/sysfs-class-led for details.
>
> +config LEDS_HW_CONTROLLED
> + bool "API for LEDs that can be controlled by hardware"
> + depends on LEDS_CLASS
depends on OF || COMPILE_TEST
?
> + select LEDS_TRIGGERS
> + help
> + This option enables support for a generic API via which other drivers
> + can register LEDs that can be put into hardware controlled mode, eg.
e.g.
> + a LED connected to an ethernet PHY can be configured to blink on
an LED
> + network activity.
> +
> comment "LED drivers"
>
> config LEDS_88PM860X
> diff --git a/include/linux/leds-hw-controlled.h b/include/linux/leds-hw-controlled.h
> new file mode 100644
> index 0000000000000..2c9b8a06def18
> --- /dev/null
> +++ b/include/linux/leds-hw-controlled.h
> @@ -0,0 +1,74 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Generic API for LEDs that can be controlled by hardware (eg. by an ethernet PHY chip)
> + *
> + * Copyright (C) 2020 Marek Behun <[email protected]>
> + */
> +#ifndef _LINUX_LEDS_HW_CONTROLLED_H_
> +#define _LINUX_LEDS_HW_CONTROLLED_H_
> +
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +
> +struct hw_controlled_led {
> + struct led_classdev cdev;
> + const struct hw_controlled_led_ops *ops;
> + struct mutex lock;
> +
> + /* these members are filled in by OF if OF is enabled */
> + int addr;
> + bool active_low;
> + bool tristate;
> +
> + /* also filled in by OF, but changed by led_set_hw_mode operation */
> + const char *hw_mode;
> +
> + void *priv;
> +};
> +#define led_cdev_to_hw_controlled_led(l) container_of(l, struct hw_controlled_led, cdev)
> +
> +/* struct hw_controlled_led_ops: Operations on LEDs that can be controlled by HW
> + *
> + * All the following operations must be implemented:
> + * @led_init: Should initialize the LED from OF data (and sanity check whether they are correct).
> + * This should also change led->cdev.max_brightness, if the value differs from default,
> + * which is 1.
> + * @led_brightness_set: Sets brightness.
> + * @led_iter_hw_mode: Iterates available HW control mode names for this LED.
> + * @led_set_hw_mode: Sets HW control mode to value specified by given name.
> + * @led_get_hw_mode: Returns current HW control mode name.
> + */
Convert that struct info to kernel-doc?
> +struct hw_controlled_led_ops {
> + int (*led_init)(struct device *dev, struct hw_controlled_led *led);
> + int (*led_brightness_set)(struct device *dev, struct hw_controlled_led *led,
> + enum led_brightness brightness);
> + const char *(*led_iter_hw_mode)(struct device *dev, struct hw_controlled_led *led,
> + void **iter);
> + int (*led_set_hw_mode)(struct device *dev, struct hw_controlled_led *led,
> + const char *mode);
> + const char *(*led_get_hw_mode)(struct device *dev, struct hw_controlled_led *led);
> +};
> +
> +
> +#endif /* _LINUX_LEDS_HW_CONTROLLED_H_ */
thanks.
--
~Randy
On Wed, 9 Sep 2020 11:20:00 -0700
Randy Dunlap <[email protected]> wrote:
> Hi,
>
> On 9/9/20 9:25 AM, Marek Behún wrote:
> > Many an ethernet PHY (and other chips) supports various HW control
> > modes for LEDs connected directly to them.
> >
> > This patch adds a generic API for registering such LEDs when
> > described in device tree. This API also exposes generic way to
> > select between these hardware control modes.
> >
> > This API registers a new private LED trigger called dev-hw-mode.
> > When this trigger is enabled for a LED, the various HW control
> > modes which are supported by the device for given LED can be
> > get/set via hw_mode sysfs file.
> >
> > Signed-off-by: Marek Behún <[email protected]>
> > ---
> > .../sysfs-class-led-trigger-dev-hw-mode | 8 +
> > drivers/leds/Kconfig | 10 +
> > drivers/leds/Makefile | 1 +
> > drivers/leds/leds-hw-controlled.c | 227
> > ++++++++++++++++++ include/linux/leds-hw-controlled.h |
> > 74 ++++++ 5 files changed, 320 insertions(+)
> > create mode 100644
> > Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode
> > create mode 100644 drivers/leds/leds-hw-controlled.c create mode
> > 100644 include/linux/leds-hw-controlled.h
>
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 1c181df24eae4..5e47ab21aafb4 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -49,6 +49,16 @@ config LEDS_BRIGHTNESS_HW_CHANGED
> >
> > See Documentation/ABI/testing/sysfs-class-led for
> > details.
> > +config LEDS_HW_CONTROLLED
> > + bool "API for LEDs that can be controlled by hardware"
> > + depends on LEDS_CLASS
>
> depends on OF || COMPILE_TEST
> ?
>
I specifically did not add OF dependency so that this can be also used
on non-OF systems. A device driver may register such LED itself...
That's why hw_controlled_led_brightness_set symbol is exported.
Do you think I shouldn't do it?
> > + select LEDS_TRIGGERS
> > + help
> > + This option enables support for a generic API via which
> > other drivers
> > + can register LEDs that can be put into hardware
> > controlled mode, eg.
>
> e.g.
>
This will need to be changed on multiple places, thanks.
> > + a LED connected to an ethernet PHY can be configured to
> > blink on
>
> an LED
>
This as well
> > + network activity.
> > +
> > comment "LED drivers"
> >
> > config LEDS_88PM860X
>
>
> > diff --git a/include/linux/leds-hw-controlled.h
> > b/include/linux/leds-hw-controlled.h new file mode 100644
> > index 0000000000000..2c9b8a06def18
> > --- /dev/null
> > +++ b/include/linux/leds-hw-controlled.h
> > @@ -0,0 +1,74 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Generic API for LEDs that can be controlled by hardware (eg. by
> > an ethernet PHY chip)
> > + *
> > + * Copyright (C) 2020 Marek Behun <[email protected]>
> > + */
> > +#ifndef _LINUX_LEDS_HW_CONTROLLED_H_
> > +#define _LINUX_LEDS_HW_CONTROLLED_H_
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/leds.h>
> > +
> > +struct hw_controlled_led {
> > + struct led_classdev cdev;
> > + const struct hw_controlled_led_ops *ops;
> > + struct mutex lock;
> > +
> > + /* these members are filled in by OF if OF is enabled */
> > + int addr;
> > + bool active_low;
> > + bool tristate;
> > +
> > + /* also filled in by OF, but changed by led_set_hw_mode
> > operation */
> > + const char *hw_mode;
> > +
> > + void *priv;
> > +};
> > +#define led_cdev_to_hw_controlled_led(l) container_of(l, struct
> > hw_controlled_led, cdev) +
> > +/* struct hw_controlled_led_ops: Operations on LEDs that can be
> > controlled by HW
> > + *
> > + * All the following operations must be implemented:
> > + * @led_init: Should initialize the LED from OF data (and sanity
> > check whether they are correct).
> > + * This should also change led->cdev.max_brightness, if
> > the value differs from default,
> > + * which is 1.
> > + * @led_brightness_set: Sets brightness.
> > + * @led_iter_hw_mode: Iterates available HW control mode names for
> > this LED.
> > + * @led_set_hw_mode: Sets HW control mode to value specified by
> > given name.
> > + * @led_get_hw_mode: Returns current HW control mode name.
> > + */
>
> Convert that struct info to kernel-doc?
>
Will look into this.
Thanks.
> > +struct hw_controlled_led_ops {
> > + int (*led_init)(struct device *dev, struct
> > hw_controlled_led *led);
> > + int (*led_brightness_set)(struct device *dev, struct
> > hw_controlled_led *led,
> > + enum led_brightness brightness);
> > + const char *(*led_iter_hw_mode)(struct device *dev, struct
> > hw_controlled_led *led,
> > + void **iter);
> > + int (*led_set_hw_mode)(struct device *dev, struct
> > hw_controlled_led *led,
> > + const char *mode);
> > + const char *(*led_get_hw_mode)(struct device *dev, struct
> > hw_controlled_led *led); +};
> > +
>
> > +
> > +#endif /* _LINUX_LEDS_HW_CONTROLLED_H_ */
>
> thanks.
On 9/9/20 11:31 AM, Marek Behún wrote:
> On Wed, 9 Sep 2020 11:20:00 -0700
> Randy Dunlap <[email protected]> wrote:
>
>> Hi,
>>
>> On 9/9/20 9:25 AM, Marek Behún wrote:
>>> Many an ethernet PHY (and other chips) supports various HW control
>>> modes for LEDs connected directly to them.
>>>
>>> This patch adds a generic API for registering such LEDs when
>>> described in device tree. This API also exposes generic way to
>>> select between these hardware control modes.
>>>
>>> This API registers a new private LED trigger called dev-hw-mode.
>>> When this trigger is enabled for a LED, the various HW control
>>> modes which are supported by the device for given LED can be
>>> get/set via hw_mode sysfs file.
>>>
>>> Signed-off-by: Marek Behún <[email protected]>
>>> ---
>>> .../sysfs-class-led-trigger-dev-hw-mode | 8 +
>>> drivers/leds/Kconfig | 10 +
>>> drivers/leds/Makefile | 1 +
>>> drivers/leds/leds-hw-controlled.c | 227
>>> ++++++++++++++++++ include/linux/leds-hw-controlled.h |
>>> 74 ++++++ 5 files changed, 320 insertions(+)
>>> create mode 100644
>>> Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode
>>> create mode 100644 drivers/leds/leds-hw-controlled.c create mode
>>> 100644 include/linux/leds-hw-controlled.h
>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index 1c181df24eae4..5e47ab21aafb4 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -49,6 +49,16 @@ config LEDS_BRIGHTNESS_HW_CHANGED
>>>
>>> See Documentation/ABI/testing/sysfs-class-led for
>>> details.
>>> +config LEDS_HW_CONTROLLED
>>> + bool "API for LEDs that can be controlled by hardware"
>>> + depends on LEDS_CLASS
>>
>> depends on OF || COMPILE_TEST
>> ?
>>
>
> I specifically did not add OF dependency so that this can be also used
> on non-OF systems. A device driver may register such LED itself...
> That's why hw_controlled_led_brightness_set symbol is exported.
>
> Do you think I shouldn't do it?
I have no problem with it as it is.
>>> + select LEDS_TRIGGERS
>>> + help
>>> + This option enables support for a generic API via which
>>> other drivers
>>> + can register LEDs that can be put into hardware
>>> controlled mode, eg.
thanks.
--
~Randy
Hi!
> Many an ethernet PHY (and other chips) supports various HW control modes
> for LEDs connected directly to them.
I guess this should be
"Many ethernet PHYs (and other chips) support various HW control modes
for LEDs connected directly to them."
> This API registers a new private LED trigger called dev-hw-mode. When
> this trigger is enabled for a LED, the various HW control modes which
> are supported by the device for given LED can be get/set via hw_mode
> sysfs file.
>
> Signed-off-by: Marek Beh?n <[email protected]>
> ---
> .../sysfs-class-led-trigger-dev-hw-mode | 8 +
> drivers/leds/Kconfig | 10 +
I guess this should live in drivers/leds/trigger/ledtrig-hw.c . I'd
call the trigger just "hw"...
> +Contact: Marek Beh?n <[email protected]>
> + [email protected]
> +Description: (W) Set the HW control mode of this LED. The various available HW control modes
> + are specific per device to which the LED is connected to and per LED itself.
> + (R) Show the available HW control modes and the currently selected one.
80 columns :-) (and please fix that globally, at least at places where
it is easy, like comments).
> + return 0;
> +err_free:
> + devm_kfree(dev, led);
> + return ret;
> +}
No need for explicit free with devm_ infrastructure?
> + cur_mode = led->ops->led_get_hw_mode(dev->parent, led);
> +
> + for (mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter);
> + mode;
> + mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter)) {
> + bool sel;
> +
> + sel = cur_mode && !strcmp(mode, cur_mode);
> +
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%s%s%s ", sel ? "[" : "", mode,
> + sel ? "]" : "");
> + }
> +
> + if (buf[len - 1] == ' ')
> + buf[len - 1] = '\n';
Can this ever be false? Are you accessing buf[-1] in such case?
> +int of_register_hw_controlled_leds(struct device *dev, const char *devicename,
> + const struct hw_controlled_led_ops *ops);
> +int hw_controlled_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness);
> +
Could we do something like hw_controlled_led -> hw_led to keep
verbosity down and line lengths reasonable? Or hwc_led?
> +extern struct led_hw_trigger_type hw_control_led_trig_type;
> +extern struct led_trigger hw_control_led_trig;
> +
> +#else /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */
CONFIG_LEDS_HWC? Or maybe CONFIG_LEDTRIG_HW?
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed, 9 Sep 2020 22:48:15 +0200
Pavel Machek <[email protected]> wrote:
> Hi!
>
> > Many an ethernet PHY (and other chips) supports various HW control modes
> > for LEDs connected directly to them.
>
> I guess this should be
>
> "Many ethernet PHYs (and other chips) support various HW control modes
> for LEDs connected directly to them."
>
I guess it is older English, used mainly in poetry, but I read it in
works of contemporary fiction as well. As far as I could find, it is still
actually gramatically correct.
https://idioms.thefreedictionary.com/many+an
https://en.wiktionary.org/wiki/many_a
But I will change it if you insist on it.
> > This API registers a new private LED trigger called dev-hw-mode. When
> > this trigger is enabled for a LED, the various HW control modes which
> > are supported by the device for given LED can be get/set via hw_mode
> > sysfs file.
> >
> > Signed-off-by: Marek Behún <[email protected]>
> > ---
> > .../sysfs-class-led-trigger-dev-hw-mode | 8 +
> > drivers/leds/Kconfig | 10 +
>
> I guess this should live in drivers/leds/trigger/ledtrig-hw.c . I'd
> call the trigger just "hw"...
>
Will move in next version. Lets see what others think about the trigger
name.
> > +Contact: Marek Behún <[email protected]>
> > + [email protected]
> > +Description: (W) Set the HW control mode of this LED. The various available HW control modes
> > + are specific per device to which the LED is connected to and per LED itself.
> > + (R) Show the available HW control modes and the currently selected one.
>
> 80 columns :-) (and please fix that globally, at least at places where
> it is easy, like comments).
>
Linux is at 100 columns now since commit bdc48fa11e46, commited by
Linus. See
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/scripts/checkpatch.pl?h=v5.9-rc4&id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
There was actually an article about this on Phoronix, I think.
> > + return 0;
> > +err_free:
> > + devm_kfree(dev, led);
> > + return ret;
> > +}
>
> No need for explicit free with devm_ infrastructure?
No, but if the registration failed, I don't see any reason why the
memory should be freed only when the PHY device is destroyed, if the
memory is not used... On failures other code in Linux frees even devm_
allocated resources.
> > + cur_mode = led->ops->led_get_hw_mode(dev->parent, led);
> > +
> > + for (mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter);
> > + mode;
> > + mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter)) {
> > + bool sel;
> > +
> > + sel = cur_mode && !strcmp(mode, cur_mode);
> > +
> > + len += scnprintf(buf + len, PAGE_SIZE - len, "%s%s%s ", sel ? "[" : "", mode,
> > + sel ? "]" : "");
> > + }
> > +
> > + if (buf[len - 1] == ' ')
> > + buf[len - 1] = '\n';
>
> Can this ever be false? Are you accessing buf[-1] in such case?
>
It can be false if whole PAGE_SIZE is written. The code above
using scnprintf only writes the first PAGE_SIZE bytes.
You are right about the buf[-1] case though. len has to be nonzero.
Thanks.
> > +int of_register_hw_controlled_leds(struct device *dev, const char *devicename,
> > + const struct hw_controlled_led_ops *ops);
> > +int hw_controlled_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness);
> > +
>
> Could we do something like hw_controlled_led -> hw_led to keep
> verbosity down and line lengths reasonable? Or hwc_led?
>
I am not opposed, lets see what Andrew / others think.
> > +extern struct led_hw_trigger_type hw_control_led_trig_type;
> > +extern struct led_trigger hw_control_led_trig;
> > +
> > +#else /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */
>
> CONFIG_LEDS_HWC? Or maybe CONFIG_LEDTRIG_HW?
The second option looks more reasonable to me, if we move to
drivers/leds/trigger.
Marek
Hi!
> > > Many an ethernet PHY (and other chips) supports various HW control modes
> > > for LEDs connected directly to them.
> >
> > I guess this should be
> >
> > "Many ethernet PHYs (and other chips) support various HW control modes
> > for LEDs connected directly to them."
> >
>
> I guess it is older English, used mainly in poetry, but I read it in
> works of contemporary fiction as well. As far as I could find, it is still
> actually gramatically correct.
> https://idioms.thefreedictionary.com/many+an
> https://en.wiktionary.org/wiki/many_a
> But I will change it if you insist on it.
Okay, you got me.
> > > +Contact: Marek Beh?n <[email protected]>
> > > + [email protected]
> > > +Description: (W) Set the HW control mode of this LED. The various available HW control modes
> > > + are specific per device to which the LED is connected to and per LED itself.
> > > + (R) Show the available HW control modes and the currently selected one.
> >
> > 80 columns :-) (and please fix that globally, at least at places where
> > it is easy, like comments).
> >
>
> Linux is at 100 columns now since commit bdc48fa11e46, commited by
> Linus. See
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/scripts/checkpatch.pl?h=v5.9-rc4&id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
> There was actually an article about this on Phoronix, I think.
It is not. Checkpatch no longer warns about it, but 80 columns is
still preffered, see Documentation/process/coding-style.rst . Plus,
you want me to take the patch, not Linus.
> > > +extern struct led_hw_trigger_type hw_control_led_trig_type;
> > > +extern struct led_trigger hw_control_led_trig;
> > > +
> > > +#else /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */
> >
> > CONFIG_LEDS_HWC? Or maybe CONFIG_LEDTRIG_HW?
>
> The second option looks more reasonable to me, if we move to
> drivers/leds/trigger.
Ok :-).
Best regards,
Pavel
On Wed, Sep 09, 2020 at 06:25:45PM +0200, Marek Beh?n wrote:
> Hello Andrew and Pavel,
>
> please review these patches adding support for HW controlled LEDs.
> The main difference from previous version is that the API is now generalized
> and lives in drivers/leds, so that part needs to be reviewed (and maybe even
> applied) by Pavel.
>
> As discussed previously between you two, I made it so that the devicename
> part of the LED is now in the form `ethernet-phy%i` when the LED is probed
> for an ethernet PHY. Userspace utility wanting to control LEDs for a specific
> network interface can access them via /sys/class/net/eth0/phydev/leds:
>
> mox ~ # ls /sys/class/net/eth0/phydev/leds
It is nice they are directly in /sys/class/net/eth0/phydev. Do they
also appear in /sys/class/led?
Have you played with network namespaces? What happens with
ip netns add ns1
ip link set eth0 netns ns1
Andrew
On Wed, 9 Sep 2020 23:42:59 +0200
Andrew Lunn <[email protected]> wrote:
> On Wed, Sep 09, 2020 at 06:25:45PM +0200, Marek Behún wrote:
> > Hello Andrew and Pavel,
> >
> > please review these patches adding support for HW controlled LEDs.
> > The main difference from previous version is that the API is now generalized
> > and lives in drivers/leds, so that part needs to be reviewed (and maybe even
> > applied) by Pavel.
> >
> > As discussed previously between you two, I made it so that the devicename
> > part of the LED is now in the form `ethernet-phy%i` when the LED is probed
> > for an ethernet PHY. Userspace utility wanting to control LEDs for a specific
> > network interface can access them via /sys/class/net/eth0/phydev/leds:
> >
> > mox ~ # ls /sys/class/net/eth0/phydev/leds
>
> It is nice they are directly in /sys/class/net/eth0/phydev. Do they
> also appear in /sys/class/led?
They are in /sys/class/net/eth0/phydev/leds by default, because they
are children of the PHY device and are of `leds` class, and the PHY
subsystem creates a symlink `phydev` when PHY is attached to the
interface.
They are in /sys/class/leds/ as symlinks, because AFAIK everything in
/sys/class/<CLASS>/ is a symlink...
> Have you played with network namespaces? What happens with
>
> ip netns add ns1
>
> ip link set eth0 netns ns1
>
> Andrew
If you move eth0 to other network namespace, naturally the
/sys/class/net/eth0 symlink will disappear, as will the directory it
pointed to.
The symlink phydev does will disappear from /sys/class/net/eth0/
directory after eth0 is moved to ns1, and is lost. It does not return
even if eth0 is moved back to root namespace.
The LED will of course stay in ns1 and also in root namespace, as will
the phydev the LED is a child to. But they are no longer accessible via
/sys/class/net/eth0, instead you can access the LEDs either via
/sys/class/leds or /sys/class/mdio_bus/<MDIO_BUS>/<PHY>/leds, or
without symlinks via /sys/devices/ tree.
Marek
On Wed, 9 Sep 2020 23:40:09 +0200
Pavel Machek <[email protected]> wrote:
> > >
> > > 80 columns :-) (and please fix that globally, at least at places where
> > > it is easy, like comments).
> > >
> >
> > Linux is at 100 columns now since commit bdc48fa11e46, commited by
> > Linus. See
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/scripts/checkpatch.pl?h=v5.9-rc4&id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
> > There was actually an article about this on Phoronix, I think.
>
> It is not. Checkpatch no longer warns about it, but 80 columns is
> still preffered, see Documentation/process/coding-style.rst . Plus,
> you want me to take the patch, not Linus.
Very well, I shall rewrap it to 80 columns :)
Marek
On Thu 2020-09-10 00:15:26, Marek Behun wrote:
> On Wed, 9 Sep 2020 23:40:09 +0200
> Pavel Machek <[email protected]> wrote:
>
> > > >
> > > > 80 columns :-) (and please fix that globally, at least at places where
> > > > it is easy, like comments).
> > > >
> > >
> > > Linux is at 100 columns now since commit bdc48fa11e46, commited by
> > > Linus. See
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/scripts/checkpatch.pl?h=v5.9-rc4&id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
> > > There was actually an article about this on Phoronix, I think.
> >
> > It is not. Checkpatch no longer warns about it, but 80 columns is
> > still preffered, see Documentation/process/coding-style.rst . Plus,
> > you want me to take the patch, not Linus.
>
> Very well, I shall rewrap it to 80 columns :)
And thou shalt wrap to 80 columns ever after!
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> They are in /sys/class/net/eth0/phydev/leds by default, because they
> are children of the PHY device and are of `leds` class, and the PHY
> subsystem creates a symlink `phydev` when PHY is attached to the
> interface.
> They are in /sys/class/leds/ as symlinks, because AFAIK everything in
> /sys/class/<CLASS>/ is a symlink...
>
> > Have you played with network namespaces? What happens with
> >
> > ip netns add ns1
> >
> > ip link set eth0 netns ns1
> >
> > Andrew
>
> If you move eth0 to other network namespace, naturally the
> /sys/class/net/eth0 symlink will disappear, as will the directory it
> pointed to.
>
> The symlink phydev does will disappear from /sys/class/net/eth0/
> directory after eth0 is moved to ns1, and is lost. It does not return
> even if eth0 is moved back to root namespace.
Yes, i just played with this. I would say that is an existing bug. The
link would still work in the namespace, since the mdio_bus is not net
namespace aware and remains accessible from all namespaces.
Andrew
Hi!
> names are not suited for this, since in some situations a PHY device
> name can look like this
> d0032004.mdio-mii:01
> or even like this
> /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08
> Clearly this cannot be used as the `device` part of a LED name.
>
> Signed-off-by: Marek Beh?n <[email protected]>
> ---
> drivers/net/phy/phy_device.c | 3 +++
> include/linux/phy.h | 3 +++
> 2 files changed, 6 insertions(+)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 8adfbad0a1e8f..38f56d39f1229 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -9,6 +9,7 @@
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> +#include <linux/atomic.h>
> #include <linux/bitmap.h>
> #include <linux/delay.h>
> #include <linux/errno.h>
> @@ -892,6 +893,7 @@ EXPORT_SYMBOL(get_phy_device);
> */
> int phy_device_register(struct phy_device *phydev)
> {
> + static atomic_t phyindex;
> int err;
>
> err = mdiobus_register_device(&phydev->mdio);
I'd put the static out of the function... for greater visibility.
Otherwise: Reviewed-by: Pavel Machek <[email protected]>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> Add nodes for the green and yellow LEDs that are connected to the
> ethernet PHY chip on Turris MOX A.
>
> Signed-off-by: Marek Beh?n <[email protected]>
> ---
> .../dts/marvell/armada-3720-turris-mox.dts | 23 +++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> index f3a678e0fd99b..6da03b6c69c0a 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> @@ -9,6 +9,7 @@
> #include <dt-bindings/bus/moxtet.h>
> #include <dt-bindings/gpio/gpio.h>
> #include <dt-bindings/input/input.h>
> +#include <dt-bindings/leds/common.h>
> #include "armada-372x.dtsi"
>
> / {
> @@ -273,6 +274,28 @@ &mdio {
>
> phy1: ethernet-phy@1 {
> reg = <1>;
> +
> + leds {
> + compatible = "linux,hw-controlled-leds";
I don't believe this is suitable compatible.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Thu, Sep 10, 2020 at 02:23:41PM +0200, Pavel Machek wrote:
> On Wed 2020-09-09 18:25:51, Marek Beh?n wrote:
> > This patch adds support for controlling the LEDs connected to several
> > families of Marvell PHYs via the PHY HW LED trigger API. These families
> > are: 88E1112, 88E1121R, 88E1240, 88E1340S, 88E1510 and 88E1545. More can
> > be added.
> >
> > This patch does not yet add support for compound LED modes. This could
> > be achieved via the LED multicolor framework.
> >
> > Settings such as HW blink rate or pulse stretch duration are not yet
> > supported.
> >
> > Signed-off-by: Marek Beh?n <[email protected]>
>
> I suggest limiting to "useful" hardware modes, and documenting what
> those modes do somewhere.
I think to keep the YAML DT verification happy, they will need to be
listed in the marvell PHY binding documentation.
Andrew
> Moreover I propose (and am willing to do) this:
> Rewrite phy_led_trigger so that it registers one trigger, `phydev`.
> The identifier of the PHY which should be source of the trigger can be
> set via a separate sysfs file, `device_name`, like in netdev trigger.
> The linked speed on which the trigger should light the LED will be
> selected via sysfs file `mode` (or do you propose another name?
> `trigger_on` or something?)
>
> Example:
> # cd /sys/class/leds/<LED>
> # echo phydev >trigger
> # echo XYZ >device_name
> # cat mode
> 1Gbps 100Mbps 10Mbps
> # echo 1Gbps >mode
> # cat mode
> [1Gbps] 100Mbps 10Mbps
>
> Also the code should be moved from driver/net/phy to
> drivers/leds/trigger.
>
> The old API can be declared deprecated or removed, but outright
> removal may cause some people to complain.
This is ABI, so you cannot remove it, or change it. You can however
add to it, in a backwards compatible way.
Andrew
On Thu 2020-09-10 15:15:41, Andrew Lunn wrote:
> On Thu, Sep 10, 2020 at 02:23:41PM +0200, Pavel Machek wrote:
> > On Wed 2020-09-09 18:25:51, Marek Beh?n wrote:
> > > This patch adds support for controlling the LEDs connected to several
> > > families of Marvell PHYs via the PHY HW LED trigger API. These families
> > > are: 88E1112, 88E1121R, 88E1240, 88E1340S, 88E1510 and 88E1545. More can
> > > be added.
> > >
> > > This patch does not yet add support for compound LED modes. This could
> > > be achieved via the LED multicolor framework.
> > >
> > > Settings such as HW blink rate or pulse stretch duration are not yet
> > > supported.
> > >
> > > Signed-off-by: Marek Beh?n <[email protected]>
> >
> > I suggest limiting to "useful" hardware modes, and documenting what
> > those modes do somewhere.
>
> I think to keep the YAML DT verification happy, they will need to be
> listed in the marvell PHY binding documentation.
Well, this should really go to the sysfs documenation. Not sure what
to do with DT.
But perhaps driver can set reasonable defaults without DT input?
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Thu, Sep 10, 2020 at 08:24:34PM +0200, Pavel Machek wrote:
> On Thu 2020-09-10 15:15:41, Andrew Lunn wrote:
> > On Thu, Sep 10, 2020 at 02:23:41PM +0200, Pavel Machek wrote:
> > > On Wed 2020-09-09 18:25:51, Marek Beh?n wrote:
> > > > This patch adds support for controlling the LEDs connected to several
> > > > families of Marvell PHYs via the PHY HW LED trigger API. These families
> > > > are: 88E1112, 88E1121R, 88E1240, 88E1340S, 88E1510 and 88E1545. More can
> > > > be added.
> > > >
> > > > This patch does not yet add support for compound LED modes. This could
> > > > be achieved via the LED multicolor framework.
> > > >
> > > > Settings such as HW blink rate or pulse stretch duration are not yet
> > > > supported.
> > > >
> > > > Signed-off-by: Marek Beh?n <[email protected]>
> > >
> > > I suggest limiting to "useful" hardware modes, and documenting what
> > > those modes do somewhere.
> >
> > I think to keep the YAML DT verification happy, they will need to be
> > listed in the marvell PHY binding documentation.
>
> Well, this should really go to the sysfs documenation. Not sure what
> to do with DT.
In DT you can set how you want the LED to blink by default. I expect
that will be the most frequent use cases, and nearly nobody will
change it at runtime.
> But perhaps driver can set reasonable defaults without DT input?
Generally the driver will default to the hardware reset blink
pattern. There are a few PHY drivers which change this at probe, but
not many. The silicon defaults are pretty good.
Andrew
On Thu, Sep 10, 2020 at 08:31:54PM +0200, Andrew Lunn wrote:
> Generally the driver will default to the hardware reset blink
> pattern. There are a few PHY drivers which change this at probe, but
> not many. The silicon defaults are pretty good.
The "right" blink pattern can be a matter of how the hardware is
wired. For example, if you have bi-colour LEDs and the PHY supports
special bi-colour mixing modes.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> I propose that at least these HW modes should be available (and
> documented) for ethernet PHY controlled LEDs:
> mode to determine link on:
> - `link`
> mode for activity (these should blink):
> - `activity` (both rx and tx), `rx`, `tx`
> mode for link (on) and activity (blink)
> - `link/activity`, maybe `link/rx` and `link/tx`
> mode for every supported speed:
> - `1Gbps`, `100Mbps`, `10Mbps`, ...
> mode for every supported cable type:
> - `copper`, `fiber`, ... (are there others?)
In theory, there is AUI and BNC, but no modern device will have these.
> mode that allows the user to determine link speed
> - `speed` (or maybe `linkspeed` ?)
> - on some Marvell PHYs the speed can be determined by how fast
> the LED is blinking (ie. 1Gbps blinks with default blinking
> frequency, 100Mbps with half blinking frequeny of 1Gbps, 10Mbps
> of half blinking frequency of 100Mbps)
> - on other Marvell PHYs this is instead:
> 1Gpbs blinks 3 times, pause, 3 times, pause, ...
> 100Mpbs blinks 2 times, pause, 2 times, pause, ...
> 10Mpbs blinks 1 time, pause, 1 time, pause, ...
> - we don't need to differentiate these modes with different names,
> because the important thing is just that this mode allows the
> user to determine the speed from how the LED blinks
> mode to just force blinking
> - `blink`
> The nice thing is that all this can be documented and done in software
> as well.
Have you checked include/dt-bindings/net/microchip-lan78xx.h and
mscc-phy-vsc8531.h ? If you are defining something generic, we need to
make sure the majority of PHYs can actually do it. There is no
standardization in this area. I'm sure there is some similarity, there
is only so many ways you can blink an LED, but i suspect we need a
mixture of standardized modes which we hope most PHYs implement, and
the option to support hardware specific modes.
Andrew
Hi!
> Okay, so the netdev trigger offers modes `link`, `rx`, `tx`.
> You can enable/disable either of these (via separate sysfs files). `rx`
> and `tx` blink the LED, `link` turns the LED on if the interface is
> linked.
I wonder if people really need separate rx and tx, but... this sounds
reasonable.
> The phy_led_trigger subsystem works differently. Instead of registering
> one trigger (like netdev) it registers one trigger per PHY device and
> per speed. So for a PHY with name XYZ and supported speeds 1Gbps,
> 100Mbps, 10Mbps it registers 3 triggers:
> XYZ:1Gbps XYZ:100Mbps XYZ:10Mbps
That is not reasonable.
> I propose that at least these HW modes should be available (and
> documented) for ethernet PHY controlled LEDs:
Ok, and which of these will you actually use?
> mode to determine link on:
> - `link`
> mode for activity (these should blink):
> - `activity` (both rx and tx), `rx`, `tx`
> mode for link (on) and activity (blink)
> - `link/activity`, maybe `link/rx` and `link/tx`
> mode for every supported speed:
> - `1Gbps`, `100Mbps`, `10Mbps`, ...
> mode for every supported cable type:
> - `copper`, `fiber`, ... (are there others?)
That's ... way too many options.
Can we do it like netdev trigger? link? yes/no. rx? yes/no. tx? yes/no.
If displaying link only for certain speeds is useful, have link_min
and link_max, specifying values in Mbps? Default would be link_min ==
0, and link_max = 25000, so it would react on any link speed.
Is mode for cable type really useful? Then we should have link_fiber?
yes/no. link_copper? yes/no.
> mode that allows the user to determine link speed
> - `speed` (or maybe `linkspeed` ?)
> - on some Marvell PHYs the speed can be determined by how fast
> the LED is blinking (ie. 1Gbps blinks with default blinking
> frequency, 100Mbps with half blinking frequeny of 1Gbps, 10Mbps
> of half blinking frequency of 100Mbps)
> - on other Marvell PHYs this is instead:
> 1Gpbs blinks 3 times, pause, 3 times, pause, ...
> 100Mpbs blinks 2 times, pause, 2 times, pause, ...
> 10Mpbs blinks 1 time, pause, 1 time, pause, ...
> - we don't need to differentiate these modes with different names,
> because the important thing is just that this mode allows the
> user to determine the speed from how the LED blinks
I'd be very careful. Userspace should know what they are asking
for. I'd propose simply ignoring this feature.
> mode to just force blinking - `blink`
We already have different support for blinking in LED subsystem. Lets use that.
Best regards,
Pavel
> We already have different support for blinking in LED subsystem. Lets use that.
You are assuming we have full software control of the LED, we can turn
it on and off. That is not always the case. But there is sometimes a
mode which the hardware blinks the LED.
Being able to blink the LED is useful:
ethtool(1):
-p --identify
Initiates adapter-specific action intended to enable an
operator to easily identify the adapter by sight. Typically
this involves blinking one or more LEDs on the specific network
port.
Once we get LED support in, i expect we will make use of this blink
mode for this ethtool option.
Andrew
Hi!
> > We already have different support for blinking in LED subsystem. Lets use that.
>
> You are assuming we have full software control of the LED, we can turn
> it on and off. That is not always the case. But there is sometimes a
> mode which the hardware blinks the LED.
Please see "timer" trigger support in the LED subsystem. We already
have hardware-accelerated blinking for the LEDs, and phy LEDs should
use same mechanism.
> Being able to blink the LED is useful: ethtool(1): -p --identify
...and yes, it should work for ethtool, too.
See leds-ss4200.c: nasgpio_led_set_blink() for an example. (But it may
not be good example).
Best regards,
Pavel
On Thu, 10 Sep 2020 19:34:35 +0100
Russell King - ARM Linux admin <[email protected]> wrote:
> On Thu, Sep 10, 2020 at 08:31:54PM +0200, Andrew Lunn wrote:
> > Generally the driver will default to the hardware reset blink
> > pattern. There are a few PHY drivers which change this at probe, but
> > not many. The silicon defaults are pretty good.
>
> The "right" blink pattern can be a matter of how the hardware is
> wired. For example, if you have bi-colour LEDs and the PHY supports
> special bi-colour mixing modes.
>
Have you seen such, Russell? This could be achieved via the multicolor
LED framework, but I don't have a device which uses such LEDs, so I
did not write support for this in the Marvell PHY driver.
(I guess I could test it though, since on my device LED0 and LED1
are used, and this to can be put into bi-colour LED mode.)
On Thu, 10 Sep 2020 22:23:45 +0200
Pavel Machek <[email protected]> wrote:
> Hi!
>
> > Okay, so the netdev trigger offers modes `link`, `rx`, `tx`.
> > You can enable/disable either of these (via separate sysfs files). `rx`
> > and `tx` blink the LED, `link` turns the LED on if the interface is
> > linked.
>
> I wonder if people really need separate rx and tx, but... this sounds
> reasonable.
>
> > The phy_led_trigger subsystem works differently. Instead of registering
> > one trigger (like netdev) it registers one trigger per PHY device and
> > per speed. So for a PHY with name XYZ and supported speeds 1Gbps,
> > 100Mbps, 10Mbps it registers 3 triggers:
> > XYZ:1Gbps XYZ:100Mbps XYZ:10Mbps
>
> That is not reasonable.
>
> > I propose that at least these HW modes should be available (and
> > documented) for ethernet PHY controlled LEDs:
>
> Ok, and which of these will you actually use?
>
> > mode to determine link on:
> > - `link`
> > mode for activity (these should blink):
> > - `activity` (both rx and tx), `rx`, `tx`
> > mode for link (on) and activity (blink)
> > - `link/activity`, maybe `link/rx` and `link/tx`
> > mode for every supported speed:
> > - `1Gbps`, `100Mbps`, `10Mbps`, ...
> > mode for every supported cable type:
> > - `copper`, `fiber`, ... (are there others?)
>
> That's ... way too many options.
>
> Can we do it like netdev trigger? link? yes/no. rx? yes/no. tx? yes/no.
>
> If displaying link only for certain speeds is useful, have link_min
> and link_max, specifying values in Mbps? Default would be link_min ==
> 0, and link_max = 25000, so it would react on any link speed.
>
> Is mode for cable type really useful? Then we should have link_fiber?
> yes/no. link_copper? yes/no.
>
I want to put the speed differentiating mode by default on MOX on one
LED, and activity on other LED.
I think there are devices which have written on the case next to the
LED that this LED is on for this specific speed, that LED is on for
other specific speed. So modes for speed are reasonable, I think.
In my opinion the disjunctive modes the Marvell PHYs support are useless
(like ON when 1000Mbps or 10Mbps).
You can't have link_min and link_max setting. The hardware does not
support it this way. You can tell the hardware to light the LED when
linked on a specific speed, and this is actually used on some devices
(as I have written above, some devices have this written on the case).
In my opinion the set `link`, `link/activity`, `activity`, `speed`,
and one mode for each supported speed on the PHY is reasonable. This could
be also compatible with software triggering via the proposed phydev
trigger.
> > mode that allows the user to determine link speed
> > - `speed` (or maybe `linkspeed` ?)
> > - on some Marvell PHYs the speed can be determined by how fast
> > the LED is blinking (ie. 1Gbps blinks with default blinking
> > frequency, 100Mbps with half blinking frequeny of 1Gbps, 10Mbps
> > of half blinking frequency of 100Mbps)
> > - on other Marvell PHYs this is instead:
> > 1Gpbs blinks 3 times, pause, 3 times, pause, ...
> > 100Mpbs blinks 2 times, pause, 2 times, pause, ...
> > 10Mpbs blinks 1 time, pause, 1 time, pause, ...
> > - we don't need to differentiate these modes with different names,
> > because the important thing is just that this mode allows the
> > user to determine the speed from how the LED blinks
>
> I'd be very careful. Userspace should know what they are asking
> for. I'd propose simply ignoring this feature.
As I wrote above, I think this mode is rather useful when you have just
two LEDs for a port. You can tell speed by looking on one LED and
activity by looking at the other LED. And I want to set this as default
on Turris MOX.
> > mode to just force blinking - `blink`
>
> We already have different support for blinking in LED subsystem. Lets use that.
>
> Best regards,
> Pavel
On Thu, 10 Sep 2020 15:15:41 +0200
Andrew Lunn <[email protected]> wrote:
> On Thu, Sep 10, 2020 at 02:23:41PM +0200, Pavel Machek wrote:
> > On Wed 2020-09-09 18:25:51, Marek Beh?n wrote:
> > > This patch adds support for controlling the LEDs connected to
> > > several families of Marvell PHYs via the PHY HW LED trigger API.
> > > These families are: 88E1112, 88E1121R, 88E1240, 88E1340S, 88E1510
> > > and 88E1545. More can be added.
> > >
> > > This patch does not yet add support for compound LED modes. This
> > > could be achieved via the LED multicolor framework.
> > >
> > > Settings such as HW blink rate or pulse stretch duration are not
> > > yet supported.
> > >
> > > Signed-off-by: Marek Beh?n <[email protected]>
> >
> > I suggest limiting to "useful" hardware modes, and documenting what
> > those modes do somewhere.
>
> I think to keep the YAML DT verification happy, they will need to be
> listed in the marvell PHY binding documentation.
>
> Andrew
Okay, so the netdev trigger offers modes `link`, `rx`, `tx`.
You can enable/disable either of these (via separate sysfs files). `rx`
and `tx` blink the LED, `link` turns the LED on if the interface is
linked.
The phy_led_trigger subsystem works differently. Instead of registering
one trigger (like netdev) it registers one trigger per PHY device and
per speed. So for a PHY with name XYZ and supported speeds 1Gbps,
100Mbps, 10Mbps it registers 3 triggers:
XYZ:1Gbps XYZ:100Mbps XYZ:10Mbps
This is especially bad on a system where there are many PHYs and they
have long names derived from device tree path.
I propose that at least these HW modes should be available (and
documented) for ethernet PHY controlled LEDs:
mode to determine link on:
- `link`
mode for activity (these should blink):
- `activity` (both rx and tx), `rx`, `tx`
mode for link (on) and activity (blink)
- `link/activity`, maybe `link/rx` and `link/tx`
mode for every supported speed:
- `1Gbps`, `100Mbps`, `10Mbps`, ...
mode for every supported cable type:
- `copper`, `fiber`, ... (are there others?)
mode that allows the user to determine link speed
- `speed` (or maybe `linkspeed` ?)
- on some Marvell PHYs the speed can be determined by how fast
the LED is blinking (ie. 1Gbps blinks with default blinking
frequency, 100Mbps with half blinking frequeny of 1Gbps, 10Mbps
of half blinking frequency of 100Mbps)
- on other Marvell PHYs this is instead:
1Gpbs blinks 3 times, pause, 3 times, pause, ...
100Mpbs blinks 2 times, pause, 2 times, pause, ...
10Mpbs blinks 1 time, pause, 1 time, pause, ...
- we don't need to differentiate these modes with different names,
because the important thing is just that this mode allows the
user to determine the speed from how the LED blinks
mode to just force blinking
- `blink`
The nice thing is that all this can be documented and done in software
as well.
Moreover I propose (and am willing to do) this:
Rewrite phy_led_trigger so that it registers one trigger, `phydev`.
The identifier of the PHY which should be source of the trigger can be
set via a separate sysfs file, `device_name`, like in netdev trigger.
The linked speed on which the trigger should light the LED will be
selected via sysfs file `mode` (or do you propose another name?
`trigger_on` or something?)
Example:
# cd /sys/class/leds/<LED>
# echo phydev >trigger
# echo XYZ >device_name
# cat mode
1Gbps 100Mbps 10Mbps
# echo 1Gbps >mode
# cat mode
[1Gbps] 100Mbps 10Mbps
Also the code should be moved from driver/net/phy to
drivers/leds/trigger.
The old API can be declared deprecated or removed, but outright
removal may cause some people to complain.
What do you think?
Marek
On Wed 2020-09-09 18:25:51, Marek Beh?n wrote:
> This patch adds support for controlling the LEDs connected to several
> families of Marvell PHYs via the PHY HW LED trigger API. These families
> are: 88E1112, 88E1121R, 88E1240, 88E1340S, 88E1510 and 88E1545. More can
> be added.
>
> This patch does not yet add support for compound LED modes. This could
> be achieved via the LED multicolor framework.
>
> Settings such as HW blink rate or pulse stretch duration are not yet
> supported.
>
> Signed-off-by: Marek Beh?n <[email protected]>
I suggest limiting to "useful" hardware modes, and documenting what
those modes do somewhere.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Thu, Sep 10, 2020 at 10:31:12PM +0200, Marek Behun wrote:
> On Thu, 10 Sep 2020 19:34:35 +0100
> Russell King - ARM Linux admin <[email protected]> wrote:
>
> > On Thu, Sep 10, 2020 at 08:31:54PM +0200, Andrew Lunn wrote:
> > > Generally the driver will default to the hardware reset blink
> > > pattern. There are a few PHY drivers which change this at probe, but
> > > not many. The silicon defaults are pretty good.
> >
> > The "right" blink pattern can be a matter of how the hardware is
> > wired. For example, if you have bi-colour LEDs and the PHY supports
> > special bi-colour mixing modes.
> >
>
> Have you seen such, Russell? This could be achieved via the multicolor
> LED framework, but I don't have a device which uses such LEDs, so I
> did not write support for this in the Marvell PHY driver.
>
> (I guess I could test it though, since on my device LED0 and LED1
> are used, and this to can be put into bi-colour LED mode.)
I haven't, much to my dismay. The Macchiatobin would have been ideal -
the 10G RJ45s have bi-colour on one side and green on the other. It
would have been useful if they were wired to support the PHYs bi-
colour mode.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, 2020-09-10 at 17:00 +0200, Andrew Lunn wrote:
> > I propose that at least these HW modes should be available (and
> > documented) for ethernet PHY controlled LEDs:
> > mode to determine link on:
> > - `link`
> > mode for activity (these should blink):
> > - `activity` (both rx and tx), `rx`, `tx`
> > mode for link (on) and activity (blink)
> > - `link/activity`, maybe `link/rx` and `link/tx`
> > mode for every supported speed:
> > - `1Gbps`, `100Mbps`, `10Mbps`, ...
> > mode for every supported cable type:
> > - `copper`, `fiber`, ... (are there others?)
>
> In theory, there is AUI and BNC, but no modern device will have
> these.
>
> > mode that allows the user to determine link speed
> > - `speed` (or maybe `linkspeed` ?)
> > - on some Marvell PHYs the speed can be determined by how fast
> > the LED is blinking (ie. 1Gbps blinks with default blinking
> > frequency, 100Mbps with half blinking frequeny of 1Gbps,
> > 10Mbps
> > of half blinking frequency of 100Mbps)
> > - on other Marvell PHYs this is instead:
> > 1Gpbs blinks 3 times, pause, 3 times, pause, ...
> > 100Mpbs blinks 2 times, pause, 2 times, pause, ...
> > 10Mpbs blinks 1 time, pause, 1 time, pause, ...
> > - we don't need to differentiate these modes with different
> > names,
> > because the important thing is just that this mode allows the
> > user to determine the speed from how the LED blinks
> > mode to just force blinking
> > - `blink`
> > The nice thing is that all this can be documented and done in
> > software
> > as well.
>
> Have you checked include/dt-bindings/net/microchip-lan78xx.h and
> mscc-phy-vsc8531.h ? If you are defining something generic, we need
> to
> make sure the majority of PHYs can actually do it. There is no
> standardization in this area. I'm sure there is some similarity,
> there
> is only so many ways you can blink an LED, but i suspect we need a
> mixture of standardized modes which we hope most PHYs implement, and
> the option to support hardware specific modes.
>
> Andrew
FWIW, these are the LED HW trigger modes supported by the TI DP83867
PHY:
- Receive Error
- Receive Error or Transmit Error
- Link established, blink for transmit or receive activity
- Full duplex
- 100/1000BT link established
- 10/100BT link established
- 10BT link established
- 100BT link established
- 1000BT link established
- Collision detected
- Receive activity
- Transmit activity
- Receive or Transmit activity
- Link established
AFAIK, the "Link established, blink for transmit or receive activity"
is the only trigger that involves blinking; all other modes simply make
the LED light up when the condition is met. Setting the output level in
software is also possible.
Regarding the option to emulate unsupported HW triggers in software,
two questions come to my mind:
- Do all PHYs support manual setting of the LED level, or are the PHYs
that can only work with HW triggers?
- Is setting PHY registers always efficiently possible, or should SW
triggers be avoided in certain cases? I'm thinking about setups like
mdio-gpio. I guess this can only become an issue for triggers that
blink.
Kind regards,
Matthias
On Fri, 11 Sep 2020 09:12:01 +0200
Matthias Schiffer <[email protected]> wrote:
> On Thu, 2020-09-10 at 17:00 +0200, Andrew Lunn wrote:
> > > I propose that at least these HW modes should be available (and
> > > documented) for ethernet PHY controlled LEDs:
> > > mode to determine link on:
> > > - `link`
> > > mode for activity (these should blink):
> > > - `activity` (both rx and tx), `rx`, `tx`
> > > mode for link (on) and activity (blink)
> > > - `link/activity`, maybe `link/rx` and `link/tx`
> > > mode for every supported speed:
> > > - `1Gbps`, `100Mbps`, `10Mbps`, ...
> > > mode for every supported cable type:
> > > - `copper`, `fiber`, ... (are there others?)
> >
> > In theory, there is AUI and BNC, but no modern device will have
> > these.
> >
> > > mode that allows the user to determine link speed
> > > - `speed` (or maybe `linkspeed` ?)
> > > - on some Marvell PHYs the speed can be determined by how fast
> > > the LED is blinking (ie. 1Gbps blinks with default blinking
> > > frequency, 100Mbps with half blinking frequeny of 1Gbps,
> > > 10Mbps
> > > of half blinking frequency of 100Mbps)
> > > - on other Marvell PHYs this is instead:
> > > 1Gpbs blinks 3 times, pause, 3 times, pause, ...
> > > 100Mpbs blinks 2 times, pause, 2 times, pause, ...
> > > 10Mpbs blinks 1 time, pause, 1 time, pause, ...
> > > - we don't need to differentiate these modes with different
> > > names,
> > > because the important thing is just that this mode allows
> > > the user to determine the speed from how the LED blinks
> > > mode to just force blinking
> > > - `blink`
> > > The nice thing is that all this can be documented and done in
> > > software
> > > as well.
> >
> > Have you checked include/dt-bindings/net/microchip-lan78xx.h and
> > mscc-phy-vsc8531.h ? If you are defining something generic, we need
> > to
> > make sure the majority of PHYs can actually do it. There is no
> > standardization in this area. I'm sure there is some similarity,
> > there
> > is only so many ways you can blink an LED, but i suspect we need a
> > mixture of standardized modes which we hope most PHYs implement, and
> > the option to support hardware specific modes.
> >
> > Andrew
>
>
> FWIW, these are the LED HW trigger modes supported by the TI DP83867
> PHY:
>
> - Receive Error
> - Receive Error or Transmit Error
Does somebody use this? I would just omit these.
> - Link established, blink for transmit or receive activity
`link/activity`
> - Full duplex
Not needed for now, I think.
> - 100/1000BT link established
> - 10/100BT link established
Disjunctive modes can go f*** themselves :)
> - 10BT link established
> - 100BT link established
> - 1000BT link established
`10Mbps`, `100Mbps`, `1Gbps`
> - Collision detected
Not needed for now.
> - Receive activity
> - Transmit activity
`rx/tx`
> - Receive or Transmit activity
`activity`
> - Link established
`link`
>
> AFAIK, the "Link established, blink for transmit or receive activity"
> is the only trigger that involves blinking; all other modes simply
> make the LED light up when the condition is met. Setting the output
> level in software is also possible.
>
> Regarding the option to emulate unsupported HW triggers in software,
> two questions come to my mind:
>
> - Do all PHYs support manual setting of the LED level, or are the PHYs
> that can only work with HW triggers?
> - Is setting PHY registers always efficiently possible, or should SW
> triggers be avoided in certain cases? I'm thinking about setups like
> mdio-gpio. I guess this can only become an issue for triggers that
> blink.
The software trigger do not have to work with the LED connected to the
PHY. Any other LED on the system can be used. Only the information
about link and speed must come from the PHY, and kernel does have this
information already, either by polling or from interrupt.
>
>
> Kind regards,
> Matthias
>
> - Do all PHYs support manual setting of the LED level, or are the PHYs
> that can only work with HW triggers?
There are PHYs with do not have simple on/off.
> - Is setting PHY registers always efficiently possible, or should SW
> triggers be avoided in certain cases? I'm thinking about setups like
> mdio-gpio. I guess this can only become an issue for triggers that
> blink.
There are uses cases where not using software frequently writing
registers would be good. PTP time stamping is one, where the extra
jitter can reduce the accuracy of the clock.
I also think activity blinking in software is unlikely to be
accepted. Nothing extra is allowed in the hot path, when you can be
dealing with a million or more packets per second.
So i would say limit software fallback to link and speed, and don't
assume that is even possible depending on the hardware.
Andrew
On Thu, 10 Sep 2020 22:44:54 +0100
Russell King - ARM Linux admin <[email protected]> wrote:
> On Thu, Sep 10, 2020 at 10:31:12PM +0200, Marek Behun wrote:
> > On Thu, 10 Sep 2020 19:34:35 +0100
> > Russell King - ARM Linux admin <[email protected]> wrote:
> >
> > > On Thu, Sep 10, 2020 at 08:31:54PM +0200, Andrew Lunn wrote:
> > > > Generally the driver will default to the hardware reset blink
> > > > pattern. There are a few PHY drivers which change this at
> > > > probe, but not many. The silicon defaults are pretty good.
> > >
> > > The "right" blink pattern can be a matter of how the hardware is
> > > wired. For example, if you have bi-colour LEDs and the PHY
> > > supports special bi-colour mixing modes.
> > >
> >
> > Have you seen such, Russell? This could be achieved via the
> > multicolor LED framework, but I don't have a device which uses such
> > LEDs, so I did not write support for this in the Marvell PHY driver.
> >
> > (I guess I could test it though, since on my device LED0 and LED1
> > are used, and this to can be put into bi-colour LED mode.)
>
> I haven't, much to my dismay. The Macchiatobin would have been ideal -
> the 10G RJ45s have bi-colour on one side and green on the other. It
> would have been useful if they were wired to support the PHYs bi-
> colour mode.
>
I have access to a Macchiatobin here at work. I am willing to add
support for bicolor LEDs, but only after we solve and merge this first
proposal.
Marek