Message ID | 20200724164603.29148-3-marek.behun@nic.cz |
---|---|
State | RFC |
Delegated to: | David Miller |
Headers | show |
Series | Add support for LEDs on Marvell PHYs | expand |
Hi! > +static const struct marvell_led_mode_info marvell_led_mode_info[] = { > + { "link", { 0x0, -1, 0x0, -1, -1, -1, }, 0 }, > + { "link/act", { 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, 0 }, > + { "1Gbps/100Mbps/10Mbps", { 0x2, -1, -1, -1, -1, -1, }, 0 }, is this "1Gbps-10Mbps"? > + { "act", { 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, 0 }, > + { "blink-act", { 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, 0 }, > + { "tx", { 0x5, -1, 0x5, -1, 0x5, 0x5, }, 0 }, > + { "tx", { -1, -1, -1, 0x5, -1, -1, }, L3V5_TRANS }, > + { "rx", { -1, -1, -1, -1, 0x0, 0x0, }, 0 }, > + { "rx", { -1, 0x0, -1, -1, -1, -1, }, L1V0_RECV }, > + { "copper", { 0x6, -1, -1, -1, -1, -1, }, 0 }, > + { "copper", { -1, 0x0, -1, -1, -1, -1, }, L1V0_COPPER }, > + { "1Gbps", { 0x7, -1, -1, -1, -1, -1, }, 0 }, > + { "link/rx", { -1, 0x2, -1, 0x2, 0x2, 0x2, }, 0 }, > + { "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, }, 0 }, > + { "1Gbps-10Mbps", { -1, -1, 0x6, 0x6, -1, -1, }, 0 }, > + { "100Mbps", { -1, 0x7, -1, -1, -1, -1, }, 0 }, > + { "10Mbps", { -1, -1, 0x7, -1, -1, -1, }, 0 }, > + { "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, }, 0 }, > + { "FullDuplex/collision", { -1, -1, -1, -1, 0x7, 0x7, }, 0 }, > + { "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 }, > + { "hi-z", { 0xa, 0xa, 0xa, 0xa, 0xa, 0xa, }, 0 }, > + { "blink", { 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, 0 }, > +}; Certainly more documentation will be required here, what "ptp" setting does, for example, is not very obvious to me. Best regards, Pavel
On Sat, 25 Jul 2020 11:23:39 +0200 Pavel Machek <pavel@ucw.cz> wrote: > Hi! > > > +static const struct marvell_led_mode_info marvell_led_mode_info[] = { > > + { "link", { 0x0, -1, 0x0, -1, -1, -1, }, 0 }, > > + { "link/act", { 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, 0 }, > > + { "1Gbps/100Mbps/10Mbps", { 0x2, -1, -1, -1, -1, -1, }, 0 }, > > is this "1Gbps-10Mbps"? Most of these modes mean "ON on event", eg "link" means ON when link up, else OFF. " "tx" means ON on when transmitting, else OFF "act" means ON when activity, else OFF "copper" means ON when copper link up, else OFF but some are blinking modes "blink-act" means BLINK when activity, else OFF Some modes can do ON and BLINK, these have one '/' in their name "link/act" means ON when link up, BLINK on activity, else OFF "link/rx" means ON when link up, BLINK on receive, else OFF there is one mode, "1Gbps/100Mbps/10Mbps", which behaves differently: blinks 3 times when linked on 1Gbps blinks 2 times when linked on 100Mbps blinks 1 time when linked on 10Mbps (and this blinking is repeating, ie blinks 3 times, pause, blinks 3 times, pause) Some modes are disjunctive: "100Mbps-fiber" means ON when linked on 100Mbps or via fiber, else OFF > > > + { "act", { 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, 0 }, > > + { "blink-act", { 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, 0 }, > > + { "tx", { 0x5, -1, 0x5, -1, 0x5, 0x5, }, 0 }, > > + { "tx", { -1, -1, -1, 0x5, -1, -1, }, L3V5_TRANS }, > > + { "rx", { -1, -1, -1, -1, 0x0, 0x0, }, 0 }, > > + { "rx", { -1, 0x0, -1, -1, -1, -1, }, L1V0_RECV }, > > + { "copper", { 0x6, -1, -1, -1, -1, -1, }, 0 }, > > + { "copper", { -1, 0x0, -1, -1, -1, -1, }, L1V0_COPPER }, > > + { "1Gbps", { 0x7, -1, -1, -1, -1, -1, }, 0 }, > > + { "link/rx", { -1, 0x2, -1, 0x2, 0x2, 0x2, }, 0 }, > > + { "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, }, 0 }, > > + { "1Gbps-10Mbps", { -1, -1, 0x6, 0x6, -1, -1, }, 0 }, > > + { "100Mbps", { -1, 0x7, -1, -1, -1, -1, }, 0 }, > > + { "10Mbps", { -1, -1, 0x7, -1, -1, -1, }, 0 }, > > + { "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, }, 0 }, > > + { "FullDuplex/collision", { -1, -1, -1, -1, 0x7, 0x7, }, 0 }, > > + { "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 }, > > + { "hi-z", { 0xa, 0xa, 0xa, 0xa, 0xa, 0xa, }, 0 }, > > + { "blink", { 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, 0 }, > > +}; > > Certainly more documentation will be required here, what "ptp" setting > does, for example, is not very obvious to me. "ptp" means it will light up when the PTP functionality is enabled on the PHY and a PTP packet is received. > Best regards, > Pavel
On Sat, Jul 25, 2020 at 11:34:50AM +0200, Marek Behun wrote: > On Sat, 25 Jul 2020 11:23:39 +0200 > Pavel Machek <pavel@ucw.cz> wrote: > > > Hi! > > > > > +static const struct marvell_led_mode_info marvell_led_mode_info[] = { > > > + { "link", { 0x0, -1, 0x0, -1, -1, -1, }, 0 }, > > > + { "link/act", { 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, 0 }, > > > + { "1Gbps/100Mbps/10Mbps", { 0x2, -1, -1, -1, -1, -1, }, 0 }, > > > > is this "1Gbps-10Mbps"? > > Most of these modes mean "ON on event", eg > "link" means ON when link up, else OFF. " > "tx" means ON on when transmitting, else OFF > "act" means ON when activity, else OFF > "copper" means ON when copper link up, else OFF > but some are blinking modes > "blink-act" means BLINK when activity, else OFF > > Some modes can do ON and BLINK, these have one '/' in their name > "link/act" means ON when link up, BLINK on activity, else OFF > "link/rx" means ON when link up, BLINK on receive, else OFF > > there is one mode, "1Gbps/100Mbps/10Mbps", which behaves differently: > blinks 3 times when linked on 1Gbps > blinks 2 times when linked on 100Mbps > blinks 1 time when linked on 10Mbps > (and this blinking is repeating, ie blinks 3 times, pause, blinks 3 times, > pause) > > Some modes are disjunctive: > "100Mbps-fiber" means ON when linked on 100Mbps or via fiber, else OFF Hi Marek I would be good to added this to the sysfs documentation. > > > + { "act", { 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, 0 }, > > > + { "blink-act", { 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, 0 }, > > > + { "tx", { 0x5, -1, 0x5, -1, 0x5, 0x5, }, 0 }, > > > + { "tx", { -1, -1, -1, 0x5, -1, -1, }, L3V5_TRANS }, > > > + { "rx", { -1, -1, -1, -1, 0x0, 0x0, }, 0 }, > > > + { "rx", { -1, 0x0, -1, -1, -1, -1, }, L1V0_RECV }, To be consistent these four probably should have the blink- prefix. Or it could be /tx, /rx ? > > > + { "copper", { 0x6, -1, -1, -1, -1, -1, }, 0 }, > > > + { "copper", { -1, 0x0, -1, -1, -1, -1, }, L1V0_COPPER }, > > > + { "1Gbps", { 0x7, -1, -1, -1, -1, -1, }, 0 }, > > > + { "link/rx", { -1, 0x2, -1, 0x2, 0x2, 0x2, }, 0 }, > > > + { "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, }, 0 }, > > > + { "1Gbps-10Mbps", { -1, -1, 0x6, 0x6, -1, -1, }, 0 }, > > > + { "100Mbps", { -1, 0x7, -1, -1, -1, -1, }, 0 }, > > > + { "10Mbps", { -1, -1, 0x7, -1, -1, -1, }, 0 }, > > > + { "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, }, 0 }, > > > + { "FullDuplex/collision", { -1, -1, -1, -1, 0x7, 0x7, }, 0 }, > > > + { "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 }, > > > + { "hi-z", { 0xa, 0xa, 0xa, 0xa, 0xa, 0xa, }, 0 }, > > > + { "blink", { 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, 0 }, > > > +}; > > > > Certainly more documentation will be required here, what "ptp" setting > > does, for example, is not very obvious to me. > > "ptp" means it will light up when the PTP functionality is enabled on > the PHY and a PTP packet is received. Does hi-z mean off? In the implementation i did, i did not list off and on as triggers. I instead used them for untriggered brightness. That allowed the software triggers to work, so i had the PHY blinking the heartbeat etc. But i had to make it optional, since a quick survey of datasheets suggested not all PHYs support simple on/off control. Something beyond the scope of this patchset is implementing etHool -p -p --identify Initiates adapter-specific action intended to enable an operator to easily identify the adapter by sight. Typically this involves blink‐ ing one or more LEDs on the specific network port. If we have software controlled on/off, then a software trigger seems like i good way to do this. Andrew
On Fri, Jul 24, 2020 at 06:46:03PM +0200, 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. > > The code reads LEDs definitions from the device-tree node of the PHY. > > This patch does not yet add support for compound LED modes. This could > be achieved via the LED multicolor framework (which is not yet in > upstream). > > Settings such as HW blink rate or pulse stretch duration are not yet > supported, nor are LED polarity settings. > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > --- > drivers/net/phy/Kconfig | 1 + > drivers/net/phy/marvell.c | 364 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 365 insertions(+) > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index ffea11f73acd..5428a8af26d2 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -462,6 +462,7 @@ config LXT_PHY > > config MARVELL_PHY > tristate "Marvell PHYs" > + depends on LED_TRIGGER_PHY_HW Does it really depend on it? I think the driver will work fine without it, just the LED control will be missing. It is really a policy question. Cable test is always available, there is no Kconfig'ury to stop it getting built. Is LED support really big so that somebody might want to disable it? I think not. So lets just enable it all the time. > help > Currently has a driver for the 88E1011S > > +enum { > + L1V0_RECV = BIT(0), > + L1V0_COPPER = BIT(1), > + L1V5_100_FIBER = BIT(2), > + L1V5_100_10 = BIT(3), > + L2V2_INIT = BIT(4), > + L2V2_PTP = BIT(5), > + L2V2_DUPLEX = BIT(6), > + L3V0_FIBER = BIT(7), > + L3V0_LOS = BIT(8), > + L3V5_TRANS = BIT(9), > + L3V7_FIBER = BIT(10), > + L3V7_DUPLEX = BIT(11), Maybe also add COMMON? > +}; > + > +struct marvell_led_mode_info { > + const char *name; > + s8 regval[MARVELL_PHY_MAX_LEDS]; > + u32 flags; Maybe give the enum a name, and use it here? It can be quite hard tracking the meaning of flags in this code. Here, it is an indication of an individual feature. > +}; > + > +static const struct marvell_led_mode_info marvell_led_mode_info[] = { > + { "link", { 0x0, -1, 0x0, -1, -1, -1, }, 0 }, Replace flags 0 with COMMON? > + { "link/act", { 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, 0 }, > + { "1Gbps/100Mbps/10Mbps", { 0x2, -1, -1, -1, -1, -1, }, 0 }, > + { "act", { 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, 0 }, > + { "blink-act", { 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, 0 }, > + { "tx", { 0x5, -1, 0x5, -1, 0x5, 0x5, }, 0 }, > + { "tx", { -1, -1, -1, 0x5, -1, -1, }, L3V5_TRANS }, > + { "rx", { -1, -1, -1, -1, 0x0, 0x0, }, 0 }, > + { "rx", { -1, 0x0, -1, -1, -1, -1, }, L1V0_RECV }, > + { "copper", { 0x6, -1, -1, -1, -1, -1, }, 0 }, > + { "copper", { -1, 0x0, -1, -1, -1, -1, }, L1V0_COPPER }, > + { "1Gbps", { 0x7, -1, -1, -1, -1, -1, }, 0 }, > + { "link/rx", { -1, 0x2, -1, 0x2, 0x2, 0x2, }, 0 }, > + { "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, }, 0 }, > + { "1Gbps-10Mbps", { -1, -1, 0x6, 0x6, -1, -1, }, 0 }, > + { "100Mbps", { -1, 0x7, -1, -1, -1, -1, }, 0 }, > + { "10Mbps", { -1, -1, 0x7, -1, -1, -1, }, 0 }, > + { "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, }, 0 }, > + { "FullDuplex/collision", { -1, -1, -1, -1, 0x7, 0x7, }, 0 }, > + { "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 }, > + { "hi-z", { 0xa, 0xa, 0xa, 0xa, 0xa, 0xa, }, 0 }, > + { "blink", { 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, 0 }, > +}; > + > +struct marvell_leds_info { > + u32 family; > + int nleds; > + u32 flags; Here flags is a combination of all the features this specific PHY supports. > +}; > + > +#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, L1V0_COPPER | L1V5_100_FIBER | L2V2_INIT | L3V0_LOS | L3V5_TRANS | L3V7_FIBER), > + LED(1121R, 3, L1V5_100_10), > + LED(1240, 6, L3V5_TRANS), > + LED(1340S, 6, L1V0_COPPER | L1V5_100_FIBER | L2V2_PTP | L3V0_FIBER | L3V7_DUPLEX), > + LED(1510, 3, L1V0_RECV | L1V5_100_FIBER | L2V2_DUPLEX), > + LED(1545, 6, L1V0_COPPER | L1V5_100_FIBER | L3V0_FIBER | L3V7_DUPLEX), > +}; > + > +static inline int marvell_led_reg(int led) No inline functions in C code. Let the compiler decide. > +{ > + switch (led) { > + case 0 ... 3: > + return MII_PHY_LED_CTRL; > + case 4 ... 5: > + return MII_PHY_LED45_CTRL; > + default: > + return -EINVAL; > + } > +} > +static inline bool is_valid_led_mode(struct marvell_priv *priv, struct marvell_phy_led *led, > + const struct marvell_led_mode_info *mode) Same here, and anywhere else you might of used inline. > +{ > + return mode->regval[led->idx] != -1 && (!mode->flags || (priv->led_flags & mode->flags)); If you have COMMON, this gets simpler. > +} > + > +static int marvell_register_led(struct phy_device *phydev, struct device_node *np, int nleds) > +{ > + struct marvell_priv *priv = phydev->priv; > + struct led_init_data init_data = {}; > + struct marvell_phy_led *led; > + u32 reg, color; > + int err; > + > + err = of_property_read_u32(np, "reg", ®); > + if (err < 0) > + return err; > + > + /* > + * Maybe we should check here if reg >= nleds, where nleds is number of LEDs of this specific > + * PHY. > + */ > + if (reg >= nleds) { > + phydev_err(phydev, > + "LED node %pOF 'reg' property too large (%u, PHY supports max %u)\n", > + np, reg, nleds - 1); > + return -EINVAL; > + } > + > + led = &priv->leds[reg]; > + > + err = of_property_read_u32(np, "color", &color); > + if (err < 0) { > + phydev_err(phydev, "LED node %pOF does not specify color\n", np); > + return -EINVAL; > + } > + > +#if 0 > + /* LED_COLOR_ID_MULTI is not yet merged in Linus' tree */ > + /* TODO: Support DUAL MODE */ > + if (color == LED_COLOR_ID_MULTI) { > + phydev_warn(phydev, "node %pOF: This driver does not yet support multicolor LEDs\n", > + np); > + return -ENOTSUPP; > + } > +#endif Code getting committed should not be using #if 0. Is the needed code in the LED tree? Do we want to consider a stable branch of the LED tree which DaveM can pull into net-next? Or do you want to wait until the next merge cycle? > + > + init_data.fwnode = &np->fwnode; > + init_data.devname_mandatory = true; > + init_data.devicename = phydev->attached_dev ? netdev_name(phydev->attached_dev) : ""; This we need to think about. Are you running this on a system with systemd? Does the interface have a name like enp2s0? Does the LED get registered before or after systemd renames it from eth0 to enp2s0? > + > + if (led->cdev.max_brightness) { > + phydev_err(phydev, "LED node %pOF 'reg' property collision with another LED\n", np); > + return -EEXIST; > + } > + > + led->cdev.max_brightness = 1; > + led->cdev.brightness_set_blocking = marvell_led_brightness_set; > + led->cdev.trigger_type = &phy_hw_led_trig_type; > + led->idx = reg; > + > + of_property_read_string(np, "linux,default-trigger", &led->cdev.default_trigger); > + > + err = devm_led_classdev_register_ext(&phydev->mdio.dev, &led->cdev, &init_data); > + if (err < 0) { > + phydev_err(phydev, "Cannot register LED %pOF: %i\n", np, err); > + return err; > + } I don't like having the DT binding in the driver. We want all PHY drivers to use the same binding, and not feel free to implement whatever they want in their own driver. These are all standard properties which all PHY drivers should be using. So lets move it into phylib core. That also allows us to specify the properties in DT, maybe just pulling in the core LED yaml stuff. > + > + return 0; > +} > + > +static void marvell_register_leds(struct phy_device *phydev) > +{ > + struct marvell_priv *priv = phydev->priv; > + struct device_node *node = phydev->mdio.dev.of_node; > + struct device_node *leds, *led; > + const struct marvell_leds_info *info = NULL; > + int i; Reverse Christmas tree. > + > + /* some families don't support LED control in this driver yet */ > + if (!phydev->drv->led_set_hw_mode) > + return; > + > + 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; > + > + priv->led_flags = info->flags; > + > +#if 0 > + /* > + * TODO: here priv->led_flags should be changed so that hw_control values > + * for unsupported modes won't be shown. This cannot be deduced from > + * family only: for example the 88E1510 family contains 88E1510 which > + * does not support fiber, but also 88E1512, which supports fiber. > + */ > + switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) { > + } > +#endif > + > + leds = of_get_child_by_name(node, "leds"); > + if (!leds) > + return; > + > + for_each_available_child_of_node(leds, led) { > + /* Should this check if some LED registration failed? */ > + marvell_register_led(phydev, led, info->nleds); > + } > +} > + Andrew
On Sat, 25 Jul 2020 17:03:42 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > Does hi-z mean off? In the implementation i did, i did not list off > and on as triggers. I instead used them for untriggered > brightness. That allowed the software triggers to work, so i had the > PHY blinking the heartbeat etc. But i had to make it optional, since a > quick survey of datasheets suggested not all PHYs support simple > on/off control. I don't actually know what hi-z means, but enabling it disabled the LED. But there is another register value for OFF... > Something beyond the scope of this patchset is implementing etHool -p > > -p --identify > Initiates adapter-specific action intended to enable an operator to > easily identify the adapter by sight. Typically this involves blink‐ > ing one or more LEDs on the specific network port. > > If we have software controlled on/off, then a software trigger seems > like i good way to do this. I'll look into this. Marek
On Sat, Jul 25, 2020 at 07:39:50PM +0200, Marek Behun wrote: > On Sat, 25 Jul 2020 17:03:42 +0200 > Andrew Lunn <andrew@lunn.ch> wrote: > > > Does hi-z mean off? In the implementation i did, i did not list off > > and on as triggers. I instead used them for untriggered > > brightness. That allowed the software triggers to work, so i had the > > PHY blinking the heartbeat etc. But i had to make it optional, since a > > quick survey of datasheets suggested not all PHYs support simple > > on/off control. > > I don't actually know what hi-z means, but enabling it disabled the LED. > But there is another register value for OFF... Can the pin be used for other things? Maybe it needs to be configured for high impedance when it is used as a shared interrupt? If it does not seem like a useful LED setting, i would drop it for the moment. > > Something beyond the scope of this patchset is implementing etHool -p > > > > -p --identify > > Initiates adapter-specific action intended to enable an operator to > > easily identify the adapter by sight. Typically this involves blink‐ > > ing one or more LEDs on the specific network port. > > > > If we have software controlled on/off, then a software trigger seems > > like i good way to do this. > > I'll look into this. No rush. As i said, out of scope for this patchset. Just keep it in mind. It should be something we can implement once in phylib, and then all phy drivers which have LED support get it for free. Andrew
On Sat, 25 Jul 2020 19:23:18 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > On Fri, Jul 24, 2020 at 06:46:03PM +0200, 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. > > > > The code reads LEDs definitions from the device-tree node of the PHY. > > > > This patch does not yet add support for compound LED modes. This could > > be achieved via the LED multicolor framework (which is not yet in > > upstream). > > > > Settings such as HW blink rate or pulse stretch duration are not yet > > supported, nor are LED polarity settings. > > > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > > --- > > drivers/net/phy/Kconfig | 1 + > > drivers/net/phy/marvell.c | 364 ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 365 insertions(+) > > > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > > index ffea11f73acd..5428a8af26d2 100644 > > --- a/drivers/net/phy/Kconfig > > +++ b/drivers/net/phy/Kconfig > > @@ -462,6 +462,7 @@ config LXT_PHY > > > > config MARVELL_PHY > > tristate "Marvell PHYs" > > + depends on LED_TRIGGER_PHY_HW > > Does it really depend on it? I think the driver will work fine without > it, just the LED control will be missing. > > It is really a policy question. Cable test is always available, there > is no Kconfig'ury to stop it getting built. Is LED support really big > so that somebody might want to disable it? I think not. So lets just > enable it all the time. OK > > help > > Currently has a driver for the 88E1011S > > > > > +enum { > > + L1V0_RECV = BIT(0), > > + L1V0_COPPER = BIT(1), > > + L1V5_100_FIBER = BIT(2), > > + L1V5_100_10 = BIT(3), > > + L2V2_INIT = BIT(4), > > + L2V2_PTP = BIT(5), > > + L2V2_DUPLEX = BIT(6), > > + L3V0_FIBER = BIT(7), > > + L3V0_LOS = BIT(8), > > + L3V5_TRANS = BIT(9), > > + L3V7_FIBER = BIT(10), > > + L3V7_DUPLEX = BIT(11), > > Maybe also add COMMON? These are meant to be interpreted as flag bits, one LED can have multiple flags. Most time in kernel bit fields use 0, not a constant, to express no flag. > > +}; > > + > > +struct marvell_led_mode_info { > > + const char *name; > > + s8 regval[MARVELL_PHY_MAX_LEDS]; > > + u32 flags; > > Maybe give the enum a name, and use it here? It can be quite hard > tracking the meaning of flags in this code. Here, it is an indication > of an individual feature. led_flags? > > > +}; > > + > > +static const struct marvell_led_mode_info marvell_led_mode_info[] = { > > + { "link", { 0x0, -1, 0x0, -1, -1, -1, }, 0 }, > > Replace flags 0 with COMMON? > > > + { "link/act", { 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, 0 }, > > + { "1Gbps/100Mbps/10Mbps", { 0x2, -1, -1, -1, -1, -1, }, 0 }, > > + { "act", { 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, 0 }, > > + { "blink-act", { 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, 0 }, > > + { "tx", { 0x5, -1, 0x5, -1, 0x5, 0x5, }, 0 }, > > + { "tx", { -1, -1, -1, 0x5, -1, -1, }, L3V5_TRANS }, > > + { "rx", { -1, -1, -1, -1, 0x0, 0x0, }, 0 }, > > + { "rx", { -1, 0x0, -1, -1, -1, -1, }, L1V0_RECV }, > > + { "copper", { 0x6, -1, -1, -1, -1, -1, }, 0 }, > > + { "copper", { -1, 0x0, -1, -1, -1, -1, }, L1V0_COPPER }, > > + { "1Gbps", { 0x7, -1, -1, -1, -1, -1, }, 0 }, > > + { "link/rx", { -1, 0x2, -1, 0x2, 0x2, 0x2, }, 0 }, > > + { "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, }, 0 }, > > + { "1Gbps-10Mbps", { -1, -1, 0x6, 0x6, -1, -1, }, 0 }, > > + { "100Mbps", { -1, 0x7, -1, -1, -1, -1, }, 0 }, > > + { "10Mbps", { -1, -1, 0x7, -1, -1, -1, }, 0 }, > > + { "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, }, 0 }, > > + { "FullDuplex/collision", { -1, -1, -1, -1, 0x7, 0x7, }, 0 }, > > + { "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 }, > > + { "hi-z", { 0xa, 0xa, 0xa, 0xa, 0xa, 0xa, }, 0 }, > > + { "blink", { 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, 0 }, > > +}; > > + > > +struct marvell_leds_info { > > + u32 family; > > + int nleds; > > + u32 flags; > > Here flags is a combination of all the features this specific PHY > supports. > > > +}; > > + > > +#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, L1V0_COPPER | L1V5_100_FIBER | L2V2_INIT | L3V0_LOS | L3V5_TRANS | L3V7_FIBER), > > + LED(1121R, 3, L1V5_100_10), > > + LED(1240, 6, L3V5_TRANS), > > + LED(1340S, 6, L1V0_COPPER | L1V5_100_FIBER | L2V2_PTP | L3V0_FIBER | L3V7_DUPLEX), > > + LED(1510, 3, L1V0_RECV | L1V5_100_FIBER | L2V2_DUPLEX), > > + LED(1545, 6, L1V0_COPPER | L1V5_100_FIBER | L3V0_FIBER | L3V7_DUPLEX), > > +}; > > + > > +static inline int marvell_led_reg(int led) > > No inline functions in C code. Let the compiler decide. OK > > > +{ > > + switch (led) { > > + case 0 ... 3: > > + return MII_PHY_LED_CTRL; > > + case 4 ... 5: > > + return MII_PHY_LED45_CTRL; > > + default: > > + return -EINVAL; > > + } > > +} > > > +static inline bool is_valid_led_mode(struct marvell_priv *priv, struct marvell_phy_led *led, > > + const struct marvell_led_mode_info *mode) > > Same here, and anywhere else you might of used inline. > > > +{ > > + return mode->regval[led->idx] != -1 && (!mode->flags || (priv->led_flags & mode->flags)); > > If you have COMMON, this gets simpler. How? I need to check whether this specific LED mode has that specific flag bit... > > +} > > + > > > +static int marvell_register_led(struct phy_device *phydev, struct device_node *np, int nleds) > > +{ > > + struct marvell_priv *priv = phydev->priv; > > + struct led_init_data init_data = {}; > > + struct marvell_phy_led *led; > > + u32 reg, color; > > + int err; > > + > > + err = of_property_read_u32(np, "reg", ®); > > + if (err < 0) > > + return err; > > + > > + /* > > + * Maybe we should check here if reg >= nleds, where nleds is number of LEDs of this specific > > + * PHY. > > + */ > > + if (reg >= nleds) { > > + phydev_err(phydev, > > + "LED node %pOF 'reg' property too large (%u, PHY supports max %u)\n", > > + np, reg, nleds - 1); > > + return -EINVAL; > > + } > > + > > + led = &priv->leds[reg]; > > + > > + err = of_property_read_u32(np, "color", &color); > > + if (err < 0) { > > + phydev_err(phydev, "LED node %pOF does not specify color\n", np); > > + return -EINVAL; > > + } > > + > > +#if 0 > > + /* LED_COLOR_ID_MULTI is not yet merged in Linus' tree */ > > + /* TODO: Support DUAL MODE */ > > + if (color == LED_COLOR_ID_MULTI) { > > + phydev_warn(phydev, "node %pOF: This driver does not yet support multicolor LEDs\n", > > + np); > > + return -ENOTSUPP; > > + } > > +#endif > > Code getting committed should not be using #if 0. Is the needed code > in the LED tree? Do we want to consider a stable branch of the LED > tree which DaveM can pull into net-next? Or do you want to wait until > the next merge cycle? That's why this is RFC. But yes, I would like to have this merged for 5.9, so maybe we should ask Dave. Is this common? Do we also need to tell Pavel or how does this work? > > + > > + init_data.fwnode = &np->fwnode; > > + init_data.devname_mandatory = true; > > + init_data.devicename = phydev->attached_dev ? netdev_name(phydev->attached_dev) : ""; > > This we need to think about. Are you running this on a system with > systemd? Does the interface have a name like enp2s0? Does the LED get > registered before or after systemd renames it from eth0 to enp2s0? Yes, well, this should be discussed also with LED guys. I don't suppose that renaming the sysfs symlink on interface rename event is appropriate, but who knows? The interfaces are platform specific, on mvebu. They aren't connected via PCI, so their names remain eth0, eth1 ... > > + > > + if (led->cdev.max_brightness) { > > + phydev_err(phydev, "LED node %pOF 'reg' property collision with another LED\n", np); > > + return -EEXIST; > > + } > > + > > + led->cdev.max_brightness = 1; > > + led->cdev.brightness_set_blocking = marvell_led_brightness_set; > > + led->cdev.trigger_type = &phy_hw_led_trig_type; > > + led->idx = reg; > > + > > + of_property_read_string(np, "linux,default-trigger", &led->cdev.default_trigger); > > + > > + err = devm_led_classdev_register_ext(&phydev->mdio.dev, &led->cdev, &init_data); > > + if (err < 0) { > > + phydev_err(phydev, "Cannot register LED %pOF: %i\n", np, err); > > + return err; > > + } > > I don't like having the DT binding in the driver. We want all PHY > drivers to use the same binding, and not feel free to implement > whatever they want in their own driver. These are all standard > properties which all PHY drivers should be using. So lets move it into > phylib core. That also allows us to specify the properties in DT, > maybe just pulling in the core LED yaml stuff. > I also want this code to be generalized somehow so that it can be reused. The problem is that I want to have support for DUAL mode, which is Marvell specific, and a DUAL LED needs to be defined in device tree. > > + > > + return 0; > > +} > > + > > +static void marvell_register_leds(struct phy_device *phydev) > > +{ > > + struct marvell_priv *priv = phydev->priv; > > + struct device_node *node = phydev->mdio.dev.of_node; > > + struct device_node *leds, *led; > > + const struct marvell_leds_info *info = NULL; > > + int i; > > Reverse Christmas tree. > > > + > > + /* some families don't support LED control in this driver yet */ > > + if (!phydev->drv->led_set_hw_mode) > > + return; > > + > > + 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; > > + > > + priv->led_flags = info->flags; > > + > > +#if 0 > > + /* > > + * TODO: here priv->led_flags should be changed so that hw_control values > > + * for unsupported modes won't be shown. This cannot be deduced from > > + * family only: for example the 88E1510 family contains 88E1510 which > > + * does not support fiber, but also 88E1512, which supports fiber. > > + */ > > + switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) { > > + } > > +#endif > > + > > + leds = of_get_child_by_name(node, "leds"); > > + if (!leds) > > + return; > > + > > + for_each_available_child_of_node(leds, led) { > > + /* Should this check if some LED registration failed? */ > > + marvell_register_led(phydev, led, info->nleds); > > + } > > +} > > + > > Andrew
On Sat, Jul 25, 2020 at 08:02:24PM +0200, Marek Behun wrote: > On Sat, 25 Jul 2020 19:23:18 +0200 > Andrew Lunn <andrew@lunn.ch> wrote: > > > On Fri, Jul 24, 2020 at 06:46:03PM +0200, 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. > > > > > > The code reads LEDs definitions from the device-tree node of the PHY. > > > > > > This patch does not yet add support for compound LED modes. This could > > > be achieved via the LED multicolor framework (which is not yet in > > > upstream). > > > > > > Settings such as HW blink rate or pulse stretch duration are not yet > > > supported, nor are LED polarity settings. > > > > > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > > > --- > > > drivers/net/phy/Kconfig | 1 + > > > drivers/net/phy/marvell.c | 364 ++++++++++++++++++++++++++++++++++++++ > > > 2 files changed, 365 insertions(+) > > > > > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > > > index ffea11f73acd..5428a8af26d2 100644 > > > --- a/drivers/net/phy/Kconfig > > > +++ b/drivers/net/phy/Kconfig > > > @@ -462,6 +462,7 @@ config LXT_PHY > > > > > > config MARVELL_PHY > > > tristate "Marvell PHYs" > > > + depends on LED_TRIGGER_PHY_HW > > > > Does it really depend on it? I think the driver will work fine without > > it, just the LED control will be missing. > > > > It is really a policy question. Cable test is always available, there > > is no Kconfig'ury to stop it getting built. Is LED support really big > > so that somebody might want to disable it? I think not. So lets just > > enable it all the time. > > OK > > > > help > > > Currently has a driver for the 88E1011S > > > > > > > > +enum { > > > + L1V0_RECV = BIT(0), > > > + L1V0_COPPER = BIT(1), > > > + L1V5_100_FIBER = BIT(2), > > > + L1V5_100_10 = BIT(3), > > > + L2V2_INIT = BIT(4), > > > + L2V2_PTP = BIT(5), > > > + L2V2_DUPLEX = BIT(6), > > > + L3V0_FIBER = BIT(7), > > > + L3V0_LOS = BIT(8), > > > + L3V5_TRANS = BIT(9), > > > + L3V7_FIBER = BIT(10), > > > + L3V7_DUPLEX = BIT(11), COMMON = BIT(32), > > > +static const struct marvell_led_mode_info marvell_led_mode_info[] = { > > > + { "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 }, > > > + { "hi-z", { 0xa, 0xa, 0xa, 0xa, 0xa, 0xa, }, COMMON }, > > > + { "blink", { 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, COMMON }, > > > +}; > > > + > > > +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 | L1V5_100_FIBER | L3V0_FIBER | L3V7_DUPLEX), > > > +}; > > > + > > > +{ > > > + return mode->regval[led->idx] != -1 && (!mode->flags || (priv->led_flags & mode->flags)); This then becomes return mode->regval[led->idx] != -1 && (priv->led_flags & mode->flags)); Andrew
> > > +#if 0 > > > + /* LED_COLOR_ID_MULTI is not yet merged in Linus' tree */ > > > + /* TODO: Support DUAL MODE */ > > > + if (color == LED_COLOR_ID_MULTI) { > > > + phydev_warn(phydev, "node %pOF: This driver does not yet support multicolor LEDs\n", > > > + np); > > > + return -ENOTSUPP; > > > + } > > > +#endif > > > > Code getting committed should not be using #if 0. Is the needed code > > in the LED tree? Do we want to consider a stable branch of the LED > > tree which DaveM can pull into net-next? Or do you want to wait until > > the next merge cycle? > > That's why this is RFC. But yes, I would like to have this merged for > 5.9, so maybe we should ask Dave. Is this common? Do we also need to > tell Pavel or how does this work? The Pavel needs to create a stable branch. DaveM then merges that branch into net-next. Your patches can then be merged. When Linus pulls the two branches, led and net-next, git sees the exact same patches twice, and simply drops them from the second pull request. So you need to ask Pavel and DaveM if they are willing to do this. > > > + init_data.fwnode = &np->fwnode; > > > + init_data.devname_mandatory = true; > > > + init_data.devicename = phydev->attached_dev ? netdev_name(phydev->attached_dev) : ""; > > > > This we need to think about. Are you running this on a system with > > systemd? Does the interface have a name like enp2s0? Does the LED get > > registered before or after systemd renames it from eth0 to enp2s0? > > Yes, well, this should be discussed also with LED guys. I don't suppose > that renaming the sysfs symlink on interface rename event is > appropriate, but who knows? > The interfaces are platform specific, on mvebu. They aren't connected > via PCI, so their names remain eth0, eth1 ... But the Marvell driver is used with more than just mvebu. And we need this generic. There are USB Ethernet dongles which used phylib. They will get their interfaces renamed to include the MAC address, etc. It is possible to hook the notifier so we know when an interface is renamed. We can then either destroy and re-create the LED, or if the LED framework allows it, rename it. Or we avoid interface names all together and stick with the phy name, which is stable. To make it more user friendly, you could create additional symlinks. We already have /sys/class/net/ethX/phydev linking into sys/bus/mdio_bus/devices/.. . We could add /sys/class/net/ethX/ledY linking into /sys/class/led/... It would also be possible to teach ethtool about LEDs, so that it follows the symbolic links, and manipulates the LED class files. > I also want this code to be generalized somehow so that it can be > reused. The problem is that I want to have support for DUAL mode, which > is Marvell specific, and a DUAL LED needs to be defined in device tree. It sounds like you first need to teach the LED core about dual LEDs and triggers which affect two LEDs.. Andrew
On Sat, 25 Jul 2020 20:48:46 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > > > > +#if 0 > > > > + /* LED_COLOR_ID_MULTI is not yet merged in Linus' tree */ > > > > + /* TODO: Support DUAL MODE */ > > > > + if (color == LED_COLOR_ID_MULTI) { > > > > + phydev_warn(phydev, "node %pOF: This driver does not yet support multicolor LEDs\n", > > > > + np); > > > > + return -ENOTSUPP; > > > > + } > > > > +#endif > > > > > > Code getting committed should not be using #if 0. Is the needed code > > > in the LED tree? Do we want to consider a stable branch of the LED > > > tree which DaveM can pull into net-next? Or do you want to wait until > > > the next merge cycle? > > > > That's why this is RFC. But yes, I would like to have this merged for > > 5.9, so maybe we should ask Dave. Is this common? Do we also need to > > tell Pavel or how does this work? > > The Pavel needs to create a stable branch. DaveM then merges that > branch into net-next. Your patches can then be merged. When Linus > pulls the two branches, led and net-next, git sees the exact same > patches twice, and simply drops them from the second pull request. > > So you need to ask Pavel and DaveM if they are willing to do this. > > > > > + init_data.fwnode = &np->fwnode; > > > > + init_data.devname_mandatory = true; > > > > + init_data.devicename = phydev->attached_dev ? netdev_name(phydev->attached_dev) : ""; > > > > > > This we need to think about. Are you running this on a system with > > > systemd? Does the interface have a name like enp2s0? Does the LED get > > > registered before or after systemd renames it from eth0 to enp2s0? > > > > Yes, well, this should be discussed also with LED guys. I don't suppose > > that renaming the sysfs symlink on interface rename event is > > appropriate, but who knows? > > The interfaces are platform specific, on mvebu. They aren't connected > > via PCI, so their names remain eth0, eth1 ... > > But the Marvell driver is used with more than just mvebu. And we need > this generic. There are USB Ethernet dongles which used phylib. They > will get their interfaces renamed to include the MAC address, etc. > > It is possible to hook the notifier so we know when an interface is > renamed. We can then either destroy and re-create the LED, or if the > LED framework allows it, rename it. > > Or we avoid interface names all together and stick with the phy name, > which is stable. To make it more user friendly, you could create > additional symlinks. We already have /sys/class/net/ethX/phydev > linking into sys/bus/mdio_bus/devices/.. . We could add > /sys/class/net/ethX/ledY linking into /sys/class/led/... > > It would also be possible to teach ethtool about LEDs, so that it > follows the symbolic links, and manipulates the LED class files. I will propose rename of the LED name on interface rename event. > > I also want this code to be generalized somehow so that it can be > > reused. The problem is that I want to have support for DUAL mode, which > > is Marvell specific, and a DUAL LED needs to be defined in device tree. > > It sounds like you first need to teach the LED core about dual LEDs > and triggers which affect two LEDs.. This is already done. DUAL LEDs will be handled by the multicolor LED framework which also is already in Pavel's tree. The problem is that if we make PHY LEDs OF parsing code generic in phy-core, it will also need to be robust enough to take care of DUAL LEDs OF parsing. That is something which is Marvell specific. It is possible that some other vendor also manufactures PHYs with something like that, but the LEDs on other PHYs may have different maximum value of brightness, or additional configurations which will need to be in device tree... But I will try to make it generic and then we will see where it goes... Marek
On Sat 2020-07-25 20:48:46, Andrew Lunn wrote: > > > > +#if 0 > > > > + /* LED_COLOR_ID_MULTI is not yet merged in Linus' tree */ > > > > + /* TODO: Support DUAL MODE */ > > > > + if (color == LED_COLOR_ID_MULTI) { > > > > + phydev_warn(phydev, "node %pOF: This driver does not yet support multicolor LEDs\n", > > > > + np); > > > > + return -ENOTSUPP; > > > > + } > > > > +#endif > > > > > > Code getting committed should not be using #if 0. Is the needed code > > > in the LED tree? Do we want to consider a stable branch of the LED > > > tree which DaveM can pull into net-next? Or do you want to wait until > > > the next merge cycle? > > > > That's why this is RFC. But yes, I would like to have this merged for > > 5.9, so maybe we should ask Dave. Is this common? Do we also need to > > tell Pavel or how does this work? > > The Pavel needs to create a stable branch. DaveM then merges that > branch into net-next. Your patches can then be merged. When Linus > pulls the two branches, led and net-next, git sees the exact same > patches twice, and simply drops them from the second pull request. > > So you need to ask Pavel and DaveM if they are willing to do this. Multicolor should be upstream now, so I believe this is no longer required? > > I also want this code to be generalized somehow so that it can be > > reused. The problem is that I want to have support for DUAL mode, which > > is Marvell specific, and a DUAL LED needs to be defined in device tree. > > It sounds like you first need to teach the LED core about dual LEDs > and triggers which affect two LEDs.. Umm. Yes, triggers for controlling both intensity and hue will be interesting. Suggestions welcome. Best regards, Pavel
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index ffea11f73acd..5428a8af26d2 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -462,6 +462,7 @@ config LXT_PHY config MARVELL_PHY tristate "Marvell PHYs" + depends on LED_TRIGGER_PHY_HW help Currently has a driver for the 88E1011S diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index bb86ac0bd092..08cd79a1aa8d 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -23,6 +23,7 @@ #include <linux/etherdevice.h> #include <linux/skbuff.h> #include <linux/spinlock.h> +#include <linux/leds.h> #include <linux/mm.h> #include <linux/module.h> #include <linux/mii.h> @@ -148,6 +149,11 @@ #define MII_88E1510_PHY_LED_DEF 0x1177 #define MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE 0x1040 +#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 +258,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"); @@ -271,10 +279,19 @@ static struct marvell_hw_stat marvell_hw_stats[] = { { "phy_receive_errors_fiber", 1, 21, 16}, }; +struct marvell_phy_led { + struct led_classdev cdev; + u8 idx; +}; + +#define to_marvell_phy_led(l) container_of(l, struct marvell_phy_led, cdev) + struct marvell_priv { u64 stats[ARRAY_SIZE(marvell_hw_stats)]; char *hwmon_name; struct device *hwmon_dev; + struct marvell_phy_led leds[MARVELL_PHY_MAX_LEDS]; + u32 led_flags; bool cable_test_tdr; u32 first; u32 last; @@ -662,6 +679,333 @@ static int m88e1510_config_aneg(struct phy_device *phydev) return err; } +enum { + L1V0_RECV = BIT(0), + L1V0_COPPER = BIT(1), + L1V5_100_FIBER = BIT(2), + L1V5_100_10 = BIT(3), + L2V2_INIT = BIT(4), + L2V2_PTP = BIT(5), + L2V2_DUPLEX = BIT(6), + L3V0_FIBER = BIT(7), + L3V0_LOS = BIT(8), + L3V5_TRANS = BIT(9), + L3V7_FIBER = BIT(10), + L3V7_DUPLEX = BIT(11), +}; + +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, }, 0 }, + { "link/act", { 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, 0 }, + { "1Gbps/100Mbps/10Mbps", { 0x2, -1, -1, -1, -1, -1, }, 0 }, + { "act", { 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, 0 }, + { "blink-act", { 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, 0 }, + { "tx", { 0x5, -1, 0x5, -1, 0x5, 0x5, }, 0 }, + { "tx", { -1, -1, -1, 0x5, -1, -1, }, L3V5_TRANS }, + { "rx", { -1, -1, -1, -1, 0x0, 0x0, }, 0 }, + { "rx", { -1, 0x0, -1, -1, -1, -1, }, L1V0_RECV }, + { "copper", { 0x6, -1, -1, -1, -1, -1, }, 0 }, + { "copper", { -1, 0x0, -1, -1, -1, -1, }, L1V0_COPPER }, + { "1Gbps", { 0x7, -1, -1, -1, -1, -1, }, 0 }, + { "link/rx", { -1, 0x2, -1, 0x2, 0x2, 0x2, }, 0 }, + { "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, }, 0 }, + { "1Gbps-10Mbps", { -1, -1, 0x6, 0x6, -1, -1, }, 0 }, + { "100Mbps", { -1, 0x7, -1, -1, -1, -1, }, 0 }, + { "10Mbps", { -1, -1, 0x7, -1, -1, -1, }, 0 }, + { "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, }, 0 }, + { "FullDuplex/collision", { -1, -1, -1, -1, 0x7, 0x7, }, 0 }, + { "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 }, + { "hi-z", { 0xa, 0xa, 0xa, 0xa, 0xa, 0xa, }, 0 }, + { "blink", { 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, 0 }, +}; + +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, L1V0_COPPER | L1V5_100_FIBER | L2V2_INIT | L3V0_LOS | L3V5_TRANS | L3V7_FIBER), + LED(1121R, 3, L1V5_100_10), + LED(1240, 6, L3V5_TRANS), + LED(1340S, 6, L1V0_COPPER | L1V5_100_FIBER | L2V2_PTP | L3V0_FIBER | L3V7_DUPLEX), + LED(1510, 3, L1V0_RECV | L1V5_100_FIBER | L2V2_DUPLEX), + LED(1545, 6, 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_brightness_set(struct led_classdev *cdev, enum led_brightness brightness) +{ + struct phy_device *phydev = to_phy_device(cdev->dev->parent); + struct marvell_phy_led *led = to_marvell_phy_led(cdev); + u8 val; + + /* don't do anything if HW control is enabled */ + if (cdev->trigger == &phy_hw_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->idx, val); +} + +static inline bool is_valid_led_mode(struct marvell_priv *priv, struct marvell_phy_led *led, + const struct marvell_led_mode_info *mode) +{ + return mode->regval[led->idx] != -1 && (!mode->flags || (priv->led_flags & mode->flags)); +} + +static const char *marvell_led_iter_hw_mode(struct phy_device *phydev, struct led_classdev *cdev, + void **iter) +{ + struct marvell_phy_led *led = to_marvell_phy_led(cdev); + const struct marvell_led_mode_info *mode = *iter; + struct marvell_priv *priv = phydev->priv; + + 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(priv, 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 phy_device *phydev, struct led_classdev *cdev, + const char *name) +{ + struct marvell_phy_led *led = to_marvell_phy_led(cdev); + struct marvell_priv *priv = phydev->priv; + 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(priv, led, mode)) + continue; + + if (sysfs_streq(name, mode->name)) + return marvell_led_set_regval(phydev, led->idx, mode->regval[led->idx]); + } + + return -EINVAL; +} + +static const char *marvell_led_get_hw_mode(struct phy_device *phydev, struct led_classdev *cdev) +{ + struct marvell_phy_led *led = to_marvell_phy_led(cdev); + struct marvell_priv *priv = phydev->priv; + const struct marvell_led_mode_info *mode; + int i, regval; + + regval = marvell_led_get_regval(phydev, led->idx); + 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(priv, led, mode)) + continue; + + if (mode->regval[led->idx] == regval) + return mode->name; + } + + return NULL; +} + +static int marvell_register_led(struct phy_device *phydev, struct device_node *np, int nleds) +{ + struct marvell_priv *priv = phydev->priv; + struct led_init_data init_data = {}; + struct marvell_phy_led *led; + u32 reg, color; + int err; + + err = of_property_read_u32(np, "reg", ®); + if (err < 0) + return err; + + /* + * Maybe we should check here if reg >= nleds, where nleds is number of LEDs of this specific + * PHY. + */ + if (reg >= nleds) { + phydev_err(phydev, + "LED node %pOF 'reg' property too large (%u, PHY supports max %u)\n", + np, reg, nleds - 1); + return -EINVAL; + } + + led = &priv->leds[reg]; + + err = of_property_read_u32(np, "color", &color); + if (err < 0) { + phydev_err(phydev, "LED node %pOF does not specify color\n", np); + return -EINVAL; + } + +#if 0 + /* LED_COLOR_ID_MULTI is not yet merged in Linus' tree */ + /* TODO: Support DUAL MODE */ + if (color == LED_COLOR_ID_MULTI) { + phydev_warn(phydev, "node %pOF: This driver does not yet support multicolor LEDs\n", + np); + return -ENOTSUPP; + } +#endif + + init_data.fwnode = &np->fwnode; + init_data.devname_mandatory = true; + init_data.devicename = phydev->attached_dev ? netdev_name(phydev->attached_dev) : ""; + + if (led->cdev.max_brightness) { + phydev_err(phydev, "LED node %pOF 'reg' property collision with another LED\n", np); + return -EEXIST; + } + + led->cdev.max_brightness = 1; + led->cdev.brightness_set_blocking = marvell_led_brightness_set; + led->cdev.trigger_type = &phy_hw_led_trig_type; + led->idx = reg; + + of_property_read_string(np, "linux,default-trigger", &led->cdev.default_trigger); + + err = devm_led_classdev_register_ext(&phydev->mdio.dev, &led->cdev, &init_data); + if (err < 0) { + phydev_err(phydev, "Cannot register LED %pOF: %i\n", np, err); + return err; + } + + return 0; +} + +static void marvell_register_leds(struct phy_device *phydev) +{ + struct marvell_priv *priv = phydev->priv; + struct device_node *node = phydev->mdio.dev.of_node; + struct device_node *leds, *led; + const struct marvell_leds_info *info = NULL; + int i; + + /* some families don't support LED control in this driver yet */ + if (!phydev->drv->led_set_hw_mode) + return; + + 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; + + priv->led_flags = info->flags; + +#if 0 + /* + * TODO: here priv->led_flags should be changed so that hw_control values + * for unsupported modes won't be shown. This cannot be deduced from + * family only: for example the 88E1510 family contains 88E1510 which + * does not support fiber, but also 88E1512, which supports fiber. + */ + switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) { + } +#endif + + leds = of_get_child_by_name(node, "leds"); + if (!leds) + return; + + for_each_available_child_of_node(leds, led) { + /* Should this check if some LED registration failed? */ + marvell_register_led(phydev, led, info->nleds); + } +} + static void marvell_config_led(struct phy_device *phydev) { u16 def_config; @@ -692,6 +1036,8 @@ static void marvell_config_led(struct phy_device *phydev) def_config); if (err < 0) phydev_warn(phydev, "Fail to config marvell phy LED.\n"); + + marvell_register_leds(phydev); } static int marvell_config_init(struct phy_device *phydev) @@ -2656,6 +3002,9 @@ static struct phy_driver marvell_drivers[] = { .get_stats = marvell_get_stats, .get_tunable = m88e1011_get_tunable, .set_tunable = m88e1011_set_tunable, + .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, }, { .phy_id = MARVELL_PHY_ID_88E1111, @@ -2717,6 +3066,9 @@ static struct phy_driver marvell_drivers[] = { .get_stats = marvell_get_stats, .get_tunable = m88e1011_get_tunable, .set_tunable = m88e1011_set_tunable, + .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, }, { .phy_id = MARVELL_PHY_ID_88E1318S, @@ -2796,6 +3148,9 @@ static struct phy_driver marvell_drivers[] = { .get_sset_count = marvell_get_sset_count, .get_strings = marvell_get_strings, .get_stats = marvell_get_stats, + .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, }, { .phy_id = MARVELL_PHY_ID_88E1116R, @@ -2844,6 +3199,9 @@ 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_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, }, { .phy_id = MARVELL_PHY_ID_88E1540, @@ -2896,6 +3254,9 @@ 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_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, }, { .phy_id = MARVELL_PHY_ID_88E3016, @@ -2964,6 +3325,9 @@ static struct phy_driver marvell_drivers[] = { .get_stats = marvell_get_stats, .get_tunable = m88e1540_get_tunable, .set_tunable = m88e1540_set_tunable, + .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, }, { .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. The code reads LEDs definitions from the device-tree node of the PHY. This patch does not yet add support for compound LED modes. This could be achieved via the LED multicolor framework (which is not yet in upstream). Settings such as HW blink rate or pulse stretch duration are not yet supported, nor are LED polarity settings. Signed-off-by: Marek Behún <marek.behun@nic.cz> --- drivers/net/phy/Kconfig | 1 + drivers/net/phy/marvell.c | 364 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 365 insertions(+)