| Message ID | 20200909162552.11032-7-marek.behun@nic.cz |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | PLEASE REVIEW: Add support for LEDs on Marvell PHYs | expand |
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
On Thu, 10 Sep 2020 15:15:41 +0200 Andrew Lunn <andrew@lunn.ch> 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. > > 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
> 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 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. But perhaps driver can set reasonable defaults without DT input? Best regards, Pavel
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
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.
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
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.)
> 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 22:23:45 +0200 Pavel Machek <pavel@ucw.cz> 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, 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.
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
> - 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 Fri, 11 Sep 2020 09:12:01 +0200 Matthias Schiffer <matthias.schiffer@ew.tq-group.com> 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 >
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
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,
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> --- drivers/net/phy/marvell.c | 314 +++++++++++++++++++++++++++++++++++++- 1 file changed, 312 insertions(+), 2 deletions(-)