| Message ID | 20200909162552.11032-1-marek.behun@nic.cz |
|---|---|
| Headers | show |
| Series | PLEASE REVIEW: Add support for LEDs on Marvell PHYs | expand |
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 <marek.behun@nic.cz> > --- > .../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 <marek.behun@nic.cz> > + */ > +#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.
On Wed, 9 Sep 2020 11:20:00 -0700 Randy Dunlap <rdunlap@infradead.org> 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 <marek.behun@nic.cz> > > --- > > .../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 <marek.behun@nic.cz> > > + */ > > +#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 <rdunlap@infradead.org> 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 <marek.behun@nic.cz> >>> --- >>> .../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.
On Wed, 9 Sep 2020 22:48:15 +0200 Pavel Machek <pavel@ucw.cz> 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 <marek.behun@nic.cz> > > --- > > .../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 <marek.behun@nic.cz> > > + linux-leds@vger.kernel.org > > +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 <marek.behun@nic.cz> > > > + linux-leds@vger.kernel.org > > > +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 <andrew@lunn.ch> 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 <pavel@ucw.cz> 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 <pavel@ucw.cz> 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
On Wed 2020-09-09 18:25:50, Marek Behún wrote: > This patch uses the new API for HW controlled LEDs to add support for > probing and control of LEDs connected to an ethernet PHY chip. > > A PHY driver wishing to utilize this API needs to implement the methods > in struct hw_controlled_led_ops and set the member led_ops in struct > phy_driver to point to that structure. > > Signed-off-by: Marek Behún <marek.behun@nic.cz> Acked-by: Pavel Machek <pavel@ucw.cz>
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 <marek.behun@nic.cz> I suggest limiting to "useful" hardware modes, and documenting what those modes do somewhere. Best regards, Pavel
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 <marek.behun@nic.cz> > > 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
> 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
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 <marek.behun@nic.cz> > > > > > > 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
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, 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 Thu, 10 Sep 2020 22:44:54 +0100 Russell King - ARM Linux admin <linux@armlinux.org.uk> 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 <linux@armlinux.org.uk> 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
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