| Message ID | 20200723181319.15988-1-marek.behun@nic.cz |
|---|---|
| Headers | show |
| Series | Add support for LEDs on Marvell PHYs | expand |
On Thu 2020-07-23 20:13:18, Marek Behún wrote: > Hi, > > this is v2 of my RFC adding support for LEDs connected to Marvell PHYs. > > The LED subsystem patches are not contained: > - the patch adding support for LED private triggers is already accepted > in Pavel Machek's for-next tree. > If you want to try this patch on top of net-next, please also apply > https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next&id=93690cdf3060c61dfce813121d0bfc055e7fa30d > - the other led-trigger patch is not needed in this version of the RFC > > The main difference from v1 is that only one trigger, named > "hw-control", is added to the /sys/class/leds/<LED>/trigger file. > > When this trigger is activated, another file called "hw_control" is > created in the /sys/class/leds/<LED>/ directory. This file lists > available HW control modes for this LED in the same way the trigger > file does for triggers. > > Example: > > # cd /sys/class/leds/eth0\:green\:link/ > # cat trigger > [none] hw-control timer oneshot heartbeat ... > # echo hw-control >trigger Make this "hw-phy-mode" or something. hw-control is a bit too generic. > # cat trigger > none [hw-control] timer oneshot heartbeat ... > # cat hw_control > link/nolink link/act/nolink 1000/100/10/nolink act/noact > blink-act/noact transmit/notransmit copperlink/else [1000/else] > force-hi-z force-blink > # echo 1000/100/10/nolink >hw_control > # cat hw_control > link/nolink link/act/nolink [1000/100/10/nolink] act/noact > blink-act/noact transmit/notransmit copperlink/else 1000/else > force-hi-z force-blink > > The benefit here is that only one trigger is registered via LED API. > I guess there are other PHY drivers which too support HW controlled > blinking modes. So of this way of controlling PHY LED HW controlled > modes is accepted, the code creating the hw-control trigger and > hw_control file should be made into library code so that it can be > reused. > > What do you think? So.. you have 10 of them right now. I guess both hw_control and making it into the trigger directly is acceptable here. In future, would you expect having software "1000/100/10/nolink" triggers I could activate on my scrollock LED (or on GPIO controlled LEDs) to indicate network activity? Best regards, Pavel
On Fri, 24 Jul 2020 12:29:01 +0200 Pavel Machek <pavel@ucw.cz> wrote: > In future, would you expect having software "1000/100/10/nolink" > triggers I could activate on my scrollock LED (or on GPIO controlled > LEDs) to indicate network activity? Look at drivers/net/phy/phy_led_triggers.c, something like that could be actually implemented there. Some of the modes are useful, like the "1000/100/10/nolink". But some of them are pretty weird, and I don't think anyone actually uses it ("1000-10/else", which is on if the device is linked at 1000mbps ar 10mbps, and else off? who would sacrifies a LED for this?). I actually wanted to talk about the phy_led_triggers.c code. It registers several trigger for each PHY, with the name in form: phy-device-name:mode where phy-device-name is derived from OF - sometimes it is in the form d0032004.mdio-mii:01 - but sometimes in the form of whole OF path followed by ":" and the PHY address: /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08 mode is "link", "1Gbps", "100Mbps", "10Mbps" and so on" So I have a GPIO LED, and I can set it to sw trigger so that it is on when a specific PHY is linked on 1Gbps. The problem is that on Turris Mox I can connect up to three 8-port switches, which yields in 25 network PHYs overall. So reading the trigger file results in 4290 bytes (look at attachment cat_trigger.txt). I think the phy_led_triggers should have gone this way of having just one trigger (like netdev has), and specifying phy device via and mode via another file. Marek none timer oneshot heartbeat default-on [mmc0] mmc1 /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:01:link /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:01:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:01:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:01:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:02:link /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:02:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:02:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:02:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:03:link /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:03:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:03:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:03:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:04:link /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:04:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch 1@11/mdio:04:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:04:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:05:link /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:05:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:05:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:05:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:06:link /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:06:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:06:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:06:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:07:link /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:07:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:07:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:07:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:08:link /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:08:1Gbps /soc/inter nal-regs@d0000000/mdio@32004/switch1@11/mdio:08:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch1@11/mdio:08:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:01:link /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:01:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:01:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:01:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:02:link /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:02:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:02:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:02:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:03:link /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:03:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:03:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:03:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:04:link /soc/internal-regs@d0000000/mdio@32004/ switch0@10/mdio:04:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:04:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:04:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:05:link /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:05:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:05:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:05:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:06:link /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:06:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:06:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:06:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:07:link /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:07:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:07:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:07:10Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08:link /soc /internal-regs@d0000000/mdio@32004/switch0@10/mdio:08:1Gbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08:100Mbps /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08:10Mbps d0032004.mdio-mii:01:link d0032004.mdio-mii:01:1Gbps d0032004.mdio-mii:01:100Mbps d0032004.mdio-mii:01:10Mbps
On Fri, 24 Jul 2020 15:12:33 +0200 Marek Behún <marek.behun@nic.cz> wrote: > On Fri, 24 Jul 2020 12:29:01 +0200 > Pavel Machek <pavel@ucw.cz> wrote: > > > In future, would you expect having software "1000/100/10/nolink" > > triggers I could activate on my scrollock LED (or on GPIO controlled > > LEDs) to indicate network activity? > > Look at drivers/net/phy/phy_led_triggers.c, something like that could > be actually implemented there. > > Some of the modes are useful, like the "1000/100/10/nolink". But some > of them are pretty weird, and I don't think anyone actually uses it > ("1000-10/else", which is on if the device is linked at 1000mbps ar > 10mbps, and else off? who would sacrifies a LED for this?). > > I actually wanted to talk about the phy_led_triggers.c code. It > registers several trigger for each PHY, with the name in form: > phy-device-name:mode > where > phy-device-name is derived from OF > - sometimes it is in the form > d0032004.mdio-mii:01 > - but sometimes in the form of whole OF path followed by ":" and > the PHY address: > /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08 > mode is "link", "1Gbps", "100Mbps", "10Mbps" and so on" > > So I have a GPIO LED, and I can set it to sw trigger so that it is on > when a specific PHY is linked on 1Gbps. > > The problem is that on Turris Mox I can connect up to three 8-port > switches, which yields in 25 network PHYs overall. So reading the > trigger file results in 4290 bytes (look at attachment > cat_trigger.txt). I think the phy_led_triggers should have gone this > way of having just one trigger (like netdev has), and specifying phy > device via and mode via another file. > > Marek > In fact I think the way the phy_led_triggers does this should be deprecated. I new kernel config options should be create, something like "new user API for PHY LED trigger", which would create just one trigger, with name phydev, like we have netdev, and like in the netdev trigger, the mode and device should be configured via other files. This phydev trigger could then be made similar to phy-hw-mode trigger... Marek
On Fri 2020-07-24 15:12:33, Marek Behún wrote: > On Fri, 24 Jul 2020 12:29:01 +0200 > Pavel Machek <pavel@ucw.cz> wrote: > > > In future, would you expect having software "1000/100/10/nolink" > > triggers I could activate on my scrollock LED (or on GPIO controlled > > LEDs) to indicate network activity? > > Look at drivers/net/phy/phy_led_triggers.c, something like that could > be actually implemented there. > > Some of the modes are useful, like the "1000/100/10/nolink". But some > of them are pretty weird, and I don't think anyone actually uses it > ("1000-10/else", which is on if the device is linked at 1000mbps ar > 10mbps, and else off? who would sacrifies a LED for this?). > > I actually wanted to talk about the phy_led_triggers.c code. It > registers several trigger for each PHY, with the name in form: > phy-device-name:mode > where > phy-device-name is derived from OF > - sometimes it is in the form > d0032004.mdio-mii:01 > - but sometimes in the form of whole OF path followed by ":" and > the PHY address: > /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08 > mode is "link", "1Gbps", "100Mbps", "10Mbps" and so on" > > So I have a GPIO LED, and I can set it to sw trigger so that it is on > when a specific PHY is linked on 1Gbps. > > The problem is that on Turris Mox I can connect up to three 8-port > switches, which yields in 25 network PHYs overall. So reading the > trigger file results in 4290 bytes (look at attachment cat_trigger.txt). > I think the phy_led_triggers should have gone this way of having just > one trigger (like netdev has), and specifying phy device via and mode > via another file. I agree with you. This is ... not pretty ... and it would be nice to get it fixed. Best regards, Pavel