* [PATCH v2 net-next 0/3] Support offload LED blinking to PHY. @ 2023-06-24 20:56 Andrew Lunn 2023-06-24 20:56 ` [PATCH v2 net-next 1/3] led: trig: netdev: Fix requesting offload device Andrew Lunn ` (3 more replies) 0 siblings, 4 replies; 11+ messages in thread From: Andrew Lunn @ 2023-06-24 20:56 UTC (permalink / raw) To: netdev Cc: Heiner Kallweit, Russell King, Simon Horman, Christian Marangi, Andrew Lunn Allow offloading of the LED trigger netdev to PHY drivers and implement it for the Marvell PHY driver. Additionally, correct the handling of when the initial state of the LED cannot be represented by the trigger, and so an error is returned. Since v1: Add true kerneldoc for the new entries in struct phy_driver Add received Reviewed-by: tags Since v0: Make comments in struct phy_driver look more like kerneldoc Add cover letter Andrew Lunn (3): led: trig: netdev: Fix requesting offload device net: phy: phy_device: Call into the PHY driver to set LED offload net: phy: marvell: Add support for offloading LED blinking drivers/leds/trigger/ledtrig-netdev.c | 8 +- drivers/net/phy/marvell.c | 243 ++++++++++++++++++++++++++ drivers/net/phy/phy_device.c | 68 +++++++ include/linux/phy.h | 33 ++++ 4 files changed, 349 insertions(+), 3 deletions(-) -- 2.40.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 net-next 1/3] led: trig: netdev: Fix requesting offload device 2023-06-24 20:56 [PATCH v2 net-next 0/3] Support offload LED blinking to PHY Andrew Lunn @ 2023-06-24 20:56 ` Andrew Lunn 2023-07-18 19:34 ` Daniel Golle 2023-06-24 20:56 ` [PATCH v2 net-next 2/3] net: phy: phy_device: Call into the PHY driver to set LED offload Andrew Lunn ` (2 subsequent siblings) 3 siblings, 1 reply; 11+ messages in thread From: Andrew Lunn @ 2023-06-24 20:56 UTC (permalink / raw) To: netdev Cc: Heiner Kallweit, Russell King, Simon Horman, Christian Marangi, Andrew Lunn When the netdev trigger is activates, it tries to determine what device the LED blinks for, and what the current blink mode is. The documentation for hw_control_get() says: * Return 0 on success, a negative error number on failing parsing the * initial mode. Error from this function is NOT FATAL as the device * may be in a not supported initial state by the attached LED trigger. */ For the Marvell PHY and the Armada 370-rd board, the initial LED blink mode is not supported by the trigger, so it returns an error. This resulted in not getting the device the LED is blinking for. As a result, the device is unknown and offloaded is never performed. Change to condition to always get the device if offloading is supported, and reduce the scope of testing for an error from hw_control_get() to skip setting trigger internal state if there is an error. Reviewed-by: Simon Horman <simon.horman@corigine.com> Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/leds/trigger/ledtrig-netdev.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c index 32b66703068a..247b100ee1d3 100644 --- a/drivers/leds/trigger/ledtrig-netdev.c +++ b/drivers/leds/trigger/ledtrig-netdev.c @@ -565,15 +565,17 @@ static int netdev_trig_activate(struct led_classdev *led_cdev) /* Check if hw control is active by default on the LED. * Init already enabled mode in hw control. */ - if (supports_hw_control(led_cdev) && - !led_cdev->hw_control_get(led_cdev, &mode)) { + if (supports_hw_control(led_cdev)) { dev = led_cdev->hw_control_get_device(led_cdev); if (dev) { const char *name = dev_name(dev); set_device_name(trigger_data, name, strlen(name)); trigger_data->hw_control = true; - trigger_data->mode = mode; + + rc = led_cdev->hw_control_get(led_cdev, &mode); + if (!rc) + trigger_data->mode = mode; } } -- 2.40.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 net-next 1/3] led: trig: netdev: Fix requesting offload device 2023-06-24 20:56 ` [PATCH v2 net-next 1/3] led: trig: netdev: Fix requesting offload device Andrew Lunn @ 2023-07-18 19:34 ` Daniel Golle 2023-08-07 22:27 ` Andrew Lunn 0 siblings, 1 reply; 11+ messages in thread From: Daniel Golle @ 2023-07-18 19:34 UTC (permalink / raw) To: Andrew Lunn Cc: netdev, Heiner Kallweit, Russell King, Simon Horman, Christian Marangi On Sat, Jun 24, 2023 at 10:56:27PM +0200, Andrew Lunn wrote: > When the netdev trigger is activates, it tries to determine what > device the LED blinks for, and what the current blink mode is. > > The documentation for hw_control_get() says: > > * Return 0 on success, a negative error number on failing parsing the > * initial mode. Error from this function is NOT FATAL as the device > * may be in a not supported initial state by the attached LED trigger. > */ > > For the Marvell PHY and the Armada 370-rd board, the initial LED blink > mode is not supported by the trigger, so it returns an error. This > resulted in not getting the device the LED is blinking for. As a > result, the device is unknown and offloaded is never performed. > > Change to condition to always get the device if offloading is > supported, and reduce the scope of testing for an error from > hw_control_get() to skip setting trigger internal state if there is an > error. > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > drivers/leds/trigger/ledtrig-netdev.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c > index 32b66703068a..247b100ee1d3 100644 > --- a/drivers/leds/trigger/ledtrig-netdev.c > +++ b/drivers/leds/trigger/ledtrig-netdev.c > @@ -565,15 +565,17 @@ static int netdev_trig_activate(struct led_classdev *led_cdev) > /* Check if hw control is active by default on the LED. > * Init already enabled mode in hw control. > */ > - if (supports_hw_control(led_cdev) && > - !led_cdev->hw_control_get(led_cdev, &mode)) { > + if (supports_hw_control(led_cdev)) { > dev = led_cdev->hw_control_get_device(led_cdev); > if (dev) { > const char *name = dev_name(dev); > > set_device_name(trigger_data, name, strlen(name)); > trigger_data->hw_control = true; > - trigger_data->mode = mode; > + > + rc = led_cdev->hw_control_get(led_cdev, &mode); Shouldn't there also be something like led_cdev->hw_control_get(led_cdev, 0); in netdev_trig_deactivate then? Because somehow we need to tell the hardware to no longer perform an offloading operation. > + if (!rc) > + trigger_data->mode = mode; > } > } > > -- > 2.40.1 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 net-next 1/3] led: trig: netdev: Fix requesting offload device 2023-07-18 19:34 ` Daniel Golle @ 2023-08-07 22:27 ` Andrew Lunn 2023-08-07 22:49 ` Daniel Golle 0 siblings, 1 reply; 11+ messages in thread From: Andrew Lunn @ 2023-08-07 22:27 UTC (permalink / raw) To: Daniel Golle Cc: netdev, Heiner Kallweit, Russell King, Simon Horman, Christian Marangi > > + if (supports_hw_control(led_cdev)) { > > dev = led_cdev->hw_control_get_device(led_cdev); > > if (dev) { > > const char *name = dev_name(dev); > > > > set_device_name(trigger_data, name, strlen(name)); > > trigger_data->hw_control = true; > > - trigger_data->mode = mode; > > + > > + rc = led_cdev->hw_control_get(led_cdev, &mode); > > Shouldn't there also be something like > led_cdev->hw_control_get(led_cdev, 0); > in netdev_trig_deactivate then? > Because somehow we need to tell the hardware to no longer perform an > offloading operation. Hi Daniel Back from vacation, so getting around to this now. Interesting question. I would actually expect the trigger that takes its place will set the brightness to what it wants it to default it. It is documented that setting the brightness disables any offload. Have you seen a real problem with changing triggers? Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 net-next 1/3] led: trig: netdev: Fix requesting offload device 2023-08-07 22:27 ` Andrew Lunn @ 2023-08-07 22:49 ` Daniel Golle 2023-08-07 23:48 ` Andrew Lunn 0 siblings, 1 reply; 11+ messages in thread From: Daniel Golle @ 2023-08-07 22:49 UTC (permalink / raw) To: Andrew Lunn Cc: netdev, Heiner Kallweit, Russell King, Simon Horman, Christian Marangi Hi Andrew, On Tue, Aug 08, 2023 at 12:27:10AM +0200, Andrew Lunn wrote: > > > + if (supports_hw_control(led_cdev)) { > > > dev = led_cdev->hw_control_get_device(led_cdev); > > > if (dev) { > > > const char *name = dev_name(dev); > > > > > > set_device_name(trigger_data, name, strlen(name)); > > > trigger_data->hw_control = true; > > > - trigger_data->mode = mode; > > > + > > > + rc = led_cdev->hw_control_get(led_cdev, &mode); > > > > Shouldn't there also be something like > > led_cdev->hw_control_get(led_cdev, 0); > > in netdev_trig_deactivate then? > > Because somehow we need to tell the hardware to no longer perform an > > offloading operation. > > Hi Daniel > > Back from vacation, so getting around to this now. > > Interesting question. I would actually expect the trigger that takes > its place will set the brightness to what it wants it to default > it. It is documented that setting the brightness disables any offload. So setting the brigthness should result in the trigger to be cleared back to 'none' then, and that would result in calling netdev_trig_deactivate if it was previously active. Because otherwise, even if I take care of truning off all hardware triggers in the led_set_brightness call, the netdev trigger would still be selected. > > Have you seen a real problem with changing triggers? Yes, when manually switching from the netdev trigger to none (or any other trigger), hardware offloading would remain active with my implementation of the PHY LED driver[1] (which doesn't clear any offloading related things but only sets/clears a FORCE_ON bit in its led_set_brightness function). [1]: https://github.com/dangowrt/linux/commit/439d52d7b80c97ff0c682ec68a70812030c3d79e Cheers Daniel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 net-next 1/3] led: trig: netdev: Fix requesting offload device 2023-08-07 22:49 ` Daniel Golle @ 2023-08-07 23:48 ` Andrew Lunn 0 siblings, 0 replies; 11+ messages in thread From: Andrew Lunn @ 2023-08-07 23:48 UTC (permalink / raw) To: Daniel Golle Cc: netdev, Heiner Kallweit, Russell King, Simon Horman, Christian Marangi > So setting the brigthness should result in the trigger to be cleared > back to 'none' then, and that would result in calling > netdev_trig_deactivate if it was previously active. > > Because otherwise, even if I take care of truning off all hardware > triggers in the led_set_brightness call, the netdev trigger would > still be selected. I looked at edtrig-timer.c, which can offload the blinking to hardware if it supports the needed op. Its deactivate function is: static void timer_trig_deactivate(struct led_classdev *led_cdev) { /* Stop blinking */ led_set_brightness(led_cdev, LED_OFF); } So this does suggest the trigger should disable offload. v3 will do similar. Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 net-next 2/3] net: phy: phy_device: Call into the PHY driver to set LED offload 2023-06-24 20:56 [PATCH v2 net-next 0/3] Support offload LED blinking to PHY Andrew Lunn 2023-06-24 20:56 ` [PATCH v2 net-next 1/3] led: trig: netdev: Fix requesting offload device Andrew Lunn @ 2023-06-24 20:56 ` Andrew Lunn 2023-06-24 20:56 ` [PATCH v2 net-next 3/3] net: phy: marvell: Add support for offloading LED blinking Andrew Lunn 2023-07-29 12:38 ` [PATCH v2 net-next 0/3] Support offload LED blinking to PHY Daniel Golle 3 siblings, 0 replies; 11+ messages in thread From: Andrew Lunn @ 2023-06-24 20:56 UTC (permalink / raw) To: netdev Cc: Heiner Kallweit, Russell King, Simon Horman, Christian Marangi, Andrew Lunn Linux LEDs can be requested to perform hardware accelerated blinking to indicate link, RX, TX etc. Pass the rules for blinking to the PHY driver, if it implements the ops needed to determine if a given pattern can be offloaded, to offload it, and what the current offload is. Additionally implement the op needed to get what device the LED is for. Reviewed-by: Simon Horman <simon.horman@corigine.com> Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/net/phy/phy_device.c | 68 ++++++++++++++++++++++++++++++++++++ include/linux/phy.h | 33 +++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 0c2014accba7..618c587144fd 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -3020,6 +3020,61 @@ static int phy_led_blink_set(struct led_classdev *led_cdev, return err; } +static __maybe_unused struct device * +phy_led_hw_control_get_device(struct led_classdev *led_cdev) +{ + struct phy_led *phyled = to_phy_led(led_cdev); + struct phy_device *phydev = phyled->phydev; + + if (phydev->attached_dev) + return &phydev->attached_dev->dev; + return NULL; +} + +static int __maybe_unused +phy_led_hw_control_get(struct led_classdev *led_cdev, + unsigned long *rules) +{ + struct phy_led *phyled = to_phy_led(led_cdev); + struct phy_device *phydev = phyled->phydev; + int err; + + mutex_lock(&phydev->lock); + err = phydev->drv->led_hw_control_get(phydev, phyled->index, rules); + mutex_unlock(&phydev->lock); + + return err; +} + +static int __maybe_unused +phy_led_hw_control_set(struct led_classdev *led_cdev, + unsigned long rules) +{ + struct phy_led *phyled = to_phy_led(led_cdev); + struct phy_device *phydev = phyled->phydev; + int err; + + mutex_lock(&phydev->lock); + err = phydev->drv->led_hw_control_set(phydev, phyled->index, rules); + mutex_unlock(&phydev->lock); + + return err; +} + +static __maybe_unused int phy_led_hw_is_supported(struct led_classdev *led_cdev, + unsigned long rules) +{ + struct phy_led *phyled = to_phy_led(led_cdev); + struct phy_device *phydev = phyled->phydev; + int err; + + mutex_lock(&phydev->lock); + err = phydev->drv->led_hw_is_supported(phydev, phyled->index, rules); + mutex_unlock(&phydev->lock); + + return err; +} + static void phy_leds_unregister(struct phy_device *phydev) { struct phy_led *phyled; @@ -3057,6 +3112,19 @@ static int of_phy_led(struct phy_device *phydev, cdev->brightness_set_blocking = phy_led_set_brightness; if (phydev->drv->led_blink_set) cdev->blink_set = phy_led_blink_set; + +#ifdef CONFIG_LEDS_TRIGGERS + if (phydev->drv->led_hw_is_supported && + phydev->drv->led_hw_control_set && + phydev->drv->led_hw_control_get) { + cdev->hw_control_is_supported = phy_led_hw_is_supported; + cdev->hw_control_set = phy_led_hw_control_set; + cdev->hw_control_get = phy_led_hw_control_get; + cdev->hw_control_trigger = "netdev"; + } + + cdev->hw_control_get_device = phy_led_hw_control_get_device; +#endif cdev->max_brightness = 1; init_data.devicename = dev_name(&phydev->mdio.dev); init_data.fwnode = of_fwnode_handle(led); diff --git a/include/linux/phy.h b/include/linux/phy.h index 11c1e91563d4..0f9e4598c596 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -1104,6 +1104,39 @@ struct phy_driver { int (*led_blink_set)(struct phy_device *dev, u8 index, unsigned long *delay_on, unsigned long *delay_off); + /** + * @led_hw_is_supported: Can the HW support the given rules. + * @dev: PHY device which has the LED + * @index: Which LED of the PHY device + * @rules The core is interested in these rules + * + * Return 0 if yes, -EOPNOTSUPP if not, or an error code. + */ + int (*led_hw_is_supported)(struct phy_device *dev, u8 index, + unsigned long rules); + /** + * @led_hw_control_set: Set the HW to control the LED + * @dev: PHY device which has the LED + * @index: Which LED of the PHY device + * @rules The rules used to control the LED + * + * Returns 0, or a an error code. + */ + int (*led_hw_control_set)(struct phy_device *dev, u8 index, + unsigned long rules); + /** + * @led_hw_control_get: Get how the HW is controlling the LED + * @dev: PHY device which has the LED + * @index: Which LED of the PHY device + * @rules Pointer to the rules used to control the LED + * + * Set *@rules to how the HW is currently blinking. Returns 0 + * on success, or a error code if the current blinking cannot + * be represented in rules, or some other error happens. + */ + int (*led_hw_control_get)(struct phy_device *dev, u8 index, + unsigned long *rules); + }; #define to_phy_driver(d) container_of(to_mdio_common_driver(d), \ struct phy_driver, mdiodrv) -- 2.40.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 net-next 3/3] net: phy: marvell: Add support for offloading LED blinking 2023-06-24 20:56 [PATCH v2 net-next 0/3] Support offload LED blinking to PHY Andrew Lunn 2023-06-24 20:56 ` [PATCH v2 net-next 1/3] led: trig: netdev: Fix requesting offload device Andrew Lunn 2023-06-24 20:56 ` [PATCH v2 net-next 2/3] net: phy: phy_device: Call into the PHY driver to set LED offload Andrew Lunn @ 2023-06-24 20:56 ` Andrew Lunn 2023-06-26 3:33 ` Kalesh Anakkur Purayil 2023-07-29 12:38 ` [PATCH v2 net-next 0/3] Support offload LED blinking to PHY Daniel Golle 3 siblings, 1 reply; 11+ messages in thread From: Andrew Lunn @ 2023-06-24 20:56 UTC (permalink / raw) To: netdev Cc: Heiner Kallweit, Russell King, Simon Horman, Christian Marangi, Andrew Lunn Add the code needed to indicate if a given blinking pattern can be offloaded, to offload a pattern and to try to return the current pattern. It is expected that ledtrig-netdev will gain support for other patterns, such as different link speeds etc. So the code is over-engineers to make adding such additional patterns easy. Reviewed-by: Simon Horman <simon.horman@corigine.com> Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/net/phy/marvell.c | 243 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 243 insertions(+) diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index 43b6cb725551..a443df3034f3 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -2893,6 +2893,234 @@ static int m88e1318_led_blink_set(struct phy_device *phydev, u8 index, MII_88E1318S_PHY_LED_FUNC, reg); } +struct marvell_led_rules { + int mode; + unsigned long rules; +}; + +static const struct marvell_led_rules marvell_led0[] = { + { + .mode = 0, + .rules = BIT(TRIGGER_NETDEV_LINK), + }, + { + .mode = 3, + .rules = (BIT(TRIGGER_NETDEV_RX) | + BIT(TRIGGER_NETDEV_TX)), + }, + { + .mode = 4, + .rules = (BIT(TRIGGER_NETDEV_RX) | + BIT(TRIGGER_NETDEV_TX)), + }, + { + .mode = 5, + .rules = BIT(TRIGGER_NETDEV_TX), + }, + { + .mode = 8, + .rules = 0, + }, +}; + +static const struct marvell_led_rules marvell_led1[] = { + { + .mode = 1, + .rules = (BIT(TRIGGER_NETDEV_LINK) | + BIT(TRIGGER_NETDEV_RX) | + BIT(TRIGGER_NETDEV_TX)), + }, + { + .mode = 2, + .rules = (BIT(TRIGGER_NETDEV_LINK) | + BIT(TRIGGER_NETDEV_RX)), + }, + { + .mode = 3, + .rules = (BIT(TRIGGER_NETDEV_RX) | + BIT(TRIGGER_NETDEV_TX)), + }, + { + .mode = 4, + .rules = (BIT(TRIGGER_NETDEV_RX) | + BIT(TRIGGER_NETDEV_TX)), + }, + { + .mode = 8, + .rules = 0, + }, +}; + +static const struct marvell_led_rules marvell_led2[] = { + { + .mode = 0, + .rules = BIT(TRIGGER_NETDEV_LINK), + }, + { + .mode = 1, + .rules = (BIT(TRIGGER_NETDEV_LINK) | + BIT(TRIGGER_NETDEV_RX) | + BIT(TRIGGER_NETDEV_TX)), + }, + { + .mode = 3, + .rules = (BIT(TRIGGER_NETDEV_RX) | + BIT(TRIGGER_NETDEV_TX)), + }, + { + .mode = 4, + .rules = (BIT(TRIGGER_NETDEV_RX) | + BIT(TRIGGER_NETDEV_TX)), + }, + { + .mode = 5, + .rules = BIT(TRIGGER_NETDEV_TX), + }, + { + .mode = 8, + .rules = 0, + }, +}; + +static int marvell_find_led_mode(unsigned long rules, + const struct marvell_led_rules *marvell_rules, + int count, + int *mode) +{ + int i; + + for (i = 0; i < count; i++) { + if (marvell_rules[i].rules == rules) { + *mode = marvell_rules[i].mode; + return 0; + } + } + return -EOPNOTSUPP; +} + +static int marvell_get_led_mode(u8 index, unsigned long rules, int *mode) +{ + int ret; + + switch (index) { + case 0: + ret = marvell_find_led_mode(rules, marvell_led0, + ARRAY_SIZE(marvell_led0), mode); + break; + case 1: + ret = marvell_find_led_mode(rules, marvell_led1, + ARRAY_SIZE(marvell_led1), mode); + break; + case 2: + ret = marvell_find_led_mode(rules, marvell_led2, + ARRAY_SIZE(marvell_led2), mode); + break; + default: + return -EINVAL; + } + + return ret; +} + +static int marvell_find_led_rules(unsigned long *rules, + const struct marvell_led_rules *marvell_rules, + int count, + int mode) +{ + int i; + + for (i = 0; i < count; i++) { + if (marvell_rules[i].mode == mode) { + *rules = marvell_rules[i].rules; + return 0; + } + } + return -EOPNOTSUPP; +} + +static int marvell_get_led_rules(u8 index, unsigned long *rules, int mode) +{ + int ret; + + switch (index) { + case 0: + ret = marvell_find_led_rules(rules, marvell_led0, + ARRAY_SIZE(marvell_led0), mode); + break; + case 1: + ret = marvell_find_led_rules(rules, marvell_led1, + ARRAY_SIZE(marvell_led1), mode); + break; + case 2: + ret = marvell_find_led_rules(rules, marvell_led2, + ARRAY_SIZE(marvell_led2), mode); + break; + default: + return -EOPNOTSUPP; + } + + return ret; +} + +static int m88e1318_led_hw_is_supported(struct phy_device *phydev, u8 index, + unsigned long rules) +{ + int mode, ret; + + switch (index) { + case 0: + case 1: + case 2: + ret = marvell_get_led_mode(index, rules, &mode); + break; + default: + ret = -EINVAL; + } + + return ret; +} + +static int m88e1318_led_hw_control_set(struct phy_device *phydev, u8 index, + unsigned long rules) +{ + int mode, ret, reg; + + switch (index) { + case 0: + case 1: + case 2: + ret = marvell_get_led_mode(index, rules, &mode); + break; + default: + ret = -EINVAL; + } + + if (ret < 0) + return ret; + + reg = phy_read_paged(phydev, MII_MARVELL_LED_PAGE, + MII_88E1318S_PHY_LED_FUNC); + reg &= ~(0xf << (4 * index)); + reg |= mode << (4 * index); + return phy_write_paged(phydev, MII_MARVELL_LED_PAGE, + MII_88E1318S_PHY_LED_FUNC, reg); +} + +static int m88e1318_led_hw_control_get(struct phy_device *phydev, u8 index, + unsigned long *rules) +{ + int mode, reg; + + if (index > 2) + return -EINVAL; + + reg = phy_read_paged(phydev, MII_MARVELL_LED_PAGE, + MII_88E1318S_PHY_LED_FUNC); + mode = (reg >> (4 * index)) & 0xf; + + return marvell_get_led_rules(index, rules, mode); +} + static int marvell_probe(struct phy_device *phydev) { struct marvell_priv *priv; @@ -3144,6 +3372,9 @@ static struct phy_driver marvell_drivers[] = { .get_stats = marvell_get_stats, .led_brightness_set = m88e1318_led_brightness_set, .led_blink_set = m88e1318_led_blink_set, + .led_hw_is_supported = m88e1318_led_hw_is_supported, + .led_hw_control_set = m88e1318_led_hw_control_set, + .led_hw_control_get = m88e1318_led_hw_control_get, }, { .phy_id = MARVELL_PHY_ID_88E1145, @@ -3252,6 +3483,9 @@ static struct phy_driver marvell_drivers[] = { .cable_test_get_status = marvell_vct7_cable_test_get_status, .led_brightness_set = m88e1318_led_brightness_set, .led_blink_set = m88e1318_led_blink_set, + .led_hw_is_supported = m88e1318_led_hw_is_supported, + .led_hw_control_set = m88e1318_led_hw_control_set, + .led_hw_control_get = m88e1318_led_hw_control_get, }, { .phy_id = MARVELL_PHY_ID_88E1540, @@ -3280,6 +3514,9 @@ static struct phy_driver marvell_drivers[] = { .cable_test_get_status = marvell_vct7_cable_test_get_status, .led_brightness_set = m88e1318_led_brightness_set, .led_blink_set = m88e1318_led_blink_set, + .led_hw_is_supported = m88e1318_led_hw_is_supported, + .led_hw_control_set = m88e1318_led_hw_control_set, + .led_hw_control_get = m88e1318_led_hw_control_get, }, { .phy_id = MARVELL_PHY_ID_88E1545, @@ -3308,6 +3545,9 @@ static struct phy_driver marvell_drivers[] = { .cable_test_get_status = marvell_vct7_cable_test_get_status, .led_brightness_set = m88e1318_led_brightness_set, .led_blink_set = m88e1318_led_blink_set, + .led_hw_is_supported = m88e1318_led_hw_is_supported, + .led_hw_control_set = m88e1318_led_hw_control_set, + .led_hw_control_get = m88e1318_led_hw_control_get, }, { .phy_id = MARVELL_PHY_ID_88E3016, @@ -3451,6 +3691,9 @@ static struct phy_driver marvell_drivers[] = { .set_tunable = m88e1540_set_tunable, .led_brightness_set = m88e1318_led_brightness_set, .led_blink_set = m88e1318_led_blink_set, + .led_hw_is_supported = m88e1318_led_hw_is_supported, + .led_hw_control_set = m88e1318_led_hw_control_set, + .led_hw_control_get = m88e1318_led_hw_control_get, }, }; -- 2.40.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 net-next 3/3] net: phy: marvell: Add support for offloading LED blinking 2023-06-24 20:56 ` [PATCH v2 net-next 3/3] net: phy: marvell: Add support for offloading LED blinking Andrew Lunn @ 2023-06-26 3:33 ` Kalesh Anakkur Purayil 0 siblings, 0 replies; 11+ messages in thread From: Kalesh Anakkur Purayil @ 2023-06-26 3:33 UTC (permalink / raw) To: Andrew Lunn Cc: netdev, Heiner Kallweit, Russell King, Simon Horman, Christian Marangi [-- Attachment #1.1: Type: text/plain, Size: 11340 bytes --] On Sun, Jun 25, 2023 at 2:27 AM Andrew Lunn <andrew@lunn.ch> wrote: > Add the code needed to indicate if a given blinking pattern can be > offloaded, to offload a pattern and to try to return the current > pattern. It is expected that ledtrig-netdev will gain support for > other patterns, such as different link speeds etc. So the code is > over-engineers to make adding such additional patterns easy. > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > drivers/net/phy/marvell.c | 243 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 243 insertions(+) > > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c > index 43b6cb725551..a443df3034f3 100644 > --- a/drivers/net/phy/marvell.c > +++ b/drivers/net/phy/marvell.c > @@ -2893,6 +2893,234 @@ static int m88e1318_led_blink_set(struct > phy_device *phydev, u8 index, > MII_88E1318S_PHY_LED_FUNC, reg); > } > > +struct marvell_led_rules { > + int mode; > + unsigned long rules; > +}; > + > +static const struct marvell_led_rules marvell_led0[] = { > + { > + .mode = 0, > + .rules = BIT(TRIGGER_NETDEV_LINK), > + }, > + { > + .mode = 3, > + .rules = (BIT(TRIGGER_NETDEV_RX) | > + BIT(TRIGGER_NETDEV_TX)), > + }, > + { > + .mode = 4, > + .rules = (BIT(TRIGGER_NETDEV_RX) | > + BIT(TRIGGER_NETDEV_TX)), > + }, > + { > + .mode = 5, > + .rules = BIT(TRIGGER_NETDEV_TX), > + }, > + { > + .mode = 8, > + .rules = 0, > + }, > +}; > + > +static const struct marvell_led_rules marvell_led1[] = { > + { > + .mode = 1, > + .rules = (BIT(TRIGGER_NETDEV_LINK) | > + BIT(TRIGGER_NETDEV_RX) | > + BIT(TRIGGER_NETDEV_TX)), > + }, > + { > + .mode = 2, > + .rules = (BIT(TRIGGER_NETDEV_LINK) | > + BIT(TRIGGER_NETDEV_RX)), > + }, > + { > + .mode = 3, > + .rules = (BIT(TRIGGER_NETDEV_RX) | > + BIT(TRIGGER_NETDEV_TX)), > + }, > + { > + .mode = 4, > + .rules = (BIT(TRIGGER_NETDEV_RX) | > + BIT(TRIGGER_NETDEV_TX)), > + }, > + { > + .mode = 8, > + .rules = 0, > + }, > +}; > + > +static const struct marvell_led_rules marvell_led2[] = { > + { > + .mode = 0, > + .rules = BIT(TRIGGER_NETDEV_LINK), > + }, > + { > + .mode = 1, > + .rules = (BIT(TRIGGER_NETDEV_LINK) | > + BIT(TRIGGER_NETDEV_RX) | > + BIT(TRIGGER_NETDEV_TX)), > + }, > + { > + .mode = 3, > + .rules = (BIT(TRIGGER_NETDEV_RX) | > + BIT(TRIGGER_NETDEV_TX)), > + }, > + { > + .mode = 4, > + .rules = (BIT(TRIGGER_NETDEV_RX) | > + BIT(TRIGGER_NETDEV_TX)), > + }, > + { > + .mode = 5, > + .rules = BIT(TRIGGER_NETDEV_TX), > + }, > + { > + .mode = 8, > + .rules = 0, > + }, > +}; > + > +static int marvell_find_led_mode(unsigned long rules, > + const struct marvell_led_rules > *marvell_rules, > + int count, > + int *mode) > +{ > + int i; > + > + for (i = 0; i < count; i++) { > + if (marvell_rules[i].rules == rules) { > + *mode = marvell_rules[i].mode; > + return 0; > + } > + } > + return -EOPNOTSUPP; > +} > + > +static int marvell_get_led_mode(u8 index, unsigned long rules, int *mode) > +{ > + int ret; > + > + switch (index) { > + case 0: > + ret = marvell_find_led_mode(rules, marvell_led0, > + ARRAY_SIZE(marvell_led0), > mode); > + break; > [Kalesh]: If you return directly from here, you can avoid the "return ret;" at the end of this function. Also, it will make other cases in sync with the default one. > + case 1: > + ret = marvell_find_led_mode(rules, marvell_led1, > + ARRAY_SIZE(marvell_led1), > mode); > + break; > + case 2: > + ret = marvell_find_led_mode(rules, marvell_led2, > + ARRAY_SIZE(marvell_led2), > mode); > + break; > + default: > + return -EINVAL; > + } > + > + return ret; > +} > + > +static int marvell_find_led_rules(unsigned long *rules, > + const struct marvell_led_rules > *marvell_rules, > + int count, > + int mode) > +{ > + int i; > + > + for (i = 0; i < count; i++) { > + if (marvell_rules[i].mode == mode) { > + *rules = marvell_rules[i].rules; > + return 0; > + } > + } > + return -EOPNOTSUPP; > +} > + > +static int marvell_get_led_rules(u8 index, unsigned long *rules, int mode) > +{ > + int ret; > + > + switch (index) { > + case 0: > + ret = marvell_find_led_rules(rules, marvell_led0, > + ARRAY_SIZE(marvell_led0), > mode); > + break; > + case 1: > + ret = marvell_find_led_rules(rules, marvell_led1, > + ARRAY_SIZE(marvell_led1), > mode); > + break; > + case 2: > + ret = marvell_find_led_rules(rules, marvell_led2, > + ARRAY_SIZE(marvell_led2), > mode); > + break; > + default: > + return -EOPNOTSUPP; > + } > + > + return ret; > +} > + > +static int m88e1318_led_hw_is_supported(struct phy_device *phydev, u8 > index, > + unsigned long rules) > +{ > + int mode, ret; > + > + switch (index) { > + case 0: > + case 1: > + case 2: > + ret = marvell_get_led_mode(index, rules, &mode); > + break; > + default: > + ret = -EINVAL; > + } > + > + return ret; > +} > + > +static int m88e1318_led_hw_control_set(struct phy_device *phydev, u8 > index, > + unsigned long rules) > +{ > + int mode, ret, reg; > + > + switch (index) { > + case 0: > + case 1: > + case 2: > + ret = marvell_get_led_mode(index, rules, &mode); > + break; > + default: > + ret = -EINVAL; > + } > + > + if (ret < 0) > + return ret; > + > + reg = phy_read_paged(phydev, MII_MARVELL_LED_PAGE, > + MII_88E1318S_PHY_LED_FUNC); > [Kalesh]: Don't you need a check here for "reg < 0"? > + reg &= ~(0xf << (4 * index)); > + reg |= mode << (4 * index); > + return phy_write_paged(phydev, MII_MARVELL_LED_PAGE, > + MII_88E1318S_PHY_LED_FUNC, reg); > +} > + > +static int m88e1318_led_hw_control_get(struct phy_device *phydev, u8 > index, > + unsigned long *rules) > +{ > + int mode, reg; > + > + if (index > 2) > + return -EINVAL; > + > + reg = phy_read_paged(phydev, MII_MARVELL_LED_PAGE, > + MII_88E1318S_PHY_LED_FUNC); > + mode = (reg >> (4 * index)) & 0xf; > + > + return marvell_get_led_rules(index, rules, mode); > +} > + > static int marvell_probe(struct phy_device *phydev) > { > struct marvell_priv *priv; > @@ -3144,6 +3372,9 @@ static struct phy_driver marvell_drivers[] = { > .get_stats = marvell_get_stats, > .led_brightness_set = m88e1318_led_brightness_set, > .led_blink_set = m88e1318_led_blink_set, > + .led_hw_is_supported = m88e1318_led_hw_is_supported, > + .led_hw_control_set = m88e1318_led_hw_control_set, > + .led_hw_control_get = m88e1318_led_hw_control_get, > }, > { > .phy_id = MARVELL_PHY_ID_88E1145, > @@ -3252,6 +3483,9 @@ static struct phy_driver marvell_drivers[] = { > .cable_test_get_status = > marvell_vct7_cable_test_get_status, > .led_brightness_set = m88e1318_led_brightness_set, > .led_blink_set = m88e1318_led_blink_set, > + .led_hw_is_supported = m88e1318_led_hw_is_supported, > + .led_hw_control_set = m88e1318_led_hw_control_set, > + .led_hw_control_get = m88e1318_led_hw_control_get, > }, > { > .phy_id = MARVELL_PHY_ID_88E1540, > @@ -3280,6 +3514,9 @@ static struct phy_driver marvell_drivers[] = { > .cable_test_get_status = > marvell_vct7_cable_test_get_status, > .led_brightness_set = m88e1318_led_brightness_set, > .led_blink_set = m88e1318_led_blink_set, > + .led_hw_is_supported = m88e1318_led_hw_is_supported, > + .led_hw_control_set = m88e1318_led_hw_control_set, > + .led_hw_control_get = m88e1318_led_hw_control_get, > }, > { > .phy_id = MARVELL_PHY_ID_88E1545, > @@ -3308,6 +3545,9 @@ static struct phy_driver marvell_drivers[] = { > .cable_test_get_status = > marvell_vct7_cable_test_get_status, > .led_brightness_set = m88e1318_led_brightness_set, > .led_blink_set = m88e1318_led_blink_set, > + .led_hw_is_supported = m88e1318_led_hw_is_supported, > + .led_hw_control_set = m88e1318_led_hw_control_set, > + .led_hw_control_get = m88e1318_led_hw_control_get, > }, > { > .phy_id = MARVELL_PHY_ID_88E3016, > @@ -3451,6 +3691,9 @@ static struct phy_driver marvell_drivers[] = { > .set_tunable = m88e1540_set_tunable, > .led_brightness_set = m88e1318_led_brightness_set, > .led_blink_set = m88e1318_led_blink_set, > + .led_hw_is_supported = m88e1318_led_hw_is_supported, > + .led_hw_control_set = m88e1318_led_hw_control_set, > + .led_hw_control_get = m88e1318_led_hw_control_get, > }, > }; > > -- > 2.40.1 > > > -- Regards, Kalesh A P [-- Attachment #1.2: Type: text/html, Size: 14484 bytes --] [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4239 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 net-next 0/3] Support offload LED blinking to PHY. 2023-06-24 20:56 [PATCH v2 net-next 0/3] Support offload LED blinking to PHY Andrew Lunn ` (2 preceding siblings ...) 2023-06-24 20:56 ` [PATCH v2 net-next 3/3] net: phy: marvell: Add support for offloading LED blinking Andrew Lunn @ 2023-07-29 12:38 ` Daniel Golle 2023-07-29 17:19 ` Andrew Lunn 3 siblings, 1 reply; 11+ messages in thread From: Daniel Golle @ 2023-07-29 12:38 UTC (permalink / raw) To: Andrew Lunn Cc: netdev, Heiner Kallweit, Russell King, Simon Horman, Christian Marangi, SkyLake Huang Hi Andrew, On Sat, Jun 24, 2023 at 10:56:26PM +0200, Andrew Lunn wrote: > > Allow offloading of the LED trigger netdev to PHY drivers and > implement it for the Marvell PHY driver. Additionally, correct the > handling of when the initial state of the LED cannot be represented by > the trigger, and so an error is returned. I've used this series in my tree and implemented support for all LED- related features in the mediatek-ge-soc PHY driver: https://github.com/dangowrt/linux/commit/3b9b30a9a6699d290964fc76c56cfcd1dadf5651 I've noticed there is a problem when setting up the netdev trigger using the 'linux,default-trigger' property in device tree. In this case led_classdev_register_ext is called *before* register_netdevice has completed. Hence supports_hw_control returns false when the 'netdev' trigger is initially setup as default trigger. To resolve this, I've tried wrapping led_classdev_register_ext() and introducing led_classdev_register_ext_nodefault() which doesn't call out to led_trigger_set_default, so that can be done later by the caller. However, there isn't any good existing call from netdev to phy informing the phy that the netdev has been registered, so the phy_leds would have to register a netdevice_notifier and wait for the NETDEV_REGISTER events, matching against the netdev's PHY... And that seems a bit overkill, just to support netdev trigger offloading to work when used as a default trigger... Any better ideas anyone? > > Since v1: > > Add true kerneldoc for the new entries in struct phy_driver > Add received Reviewed-by: tags > > Since v0: > > Make comments in struct phy_driver look more like kerneldoc > Add cover letter > > Andrew Lunn (3): > led: trig: netdev: Fix requesting offload device > net: phy: phy_device: Call into the PHY driver to set LED offload > net: phy: marvell: Add support for offloading LED blinking > > drivers/leds/trigger/ledtrig-netdev.c | 8 +- > drivers/net/phy/marvell.c | 243 ++++++++++++++++++++++++++ > drivers/net/phy/phy_device.c | 68 +++++++ > include/linux/phy.h | 33 ++++ > 4 files changed, 349 insertions(+), 3 deletions(-) > > -- > 2.40.1 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 net-next 0/3] Support offload LED blinking to PHY. 2023-07-29 12:38 ` [PATCH v2 net-next 0/3] Support offload LED blinking to PHY Daniel Golle @ 2023-07-29 17:19 ` Andrew Lunn 0 siblings, 0 replies; 11+ messages in thread From: Andrew Lunn @ 2023-07-29 17:19 UTC (permalink / raw) To: Daniel Golle Cc: netdev, Heiner Kallweit, Russell King, Simon Horman, Christian Marangi, SkyLake Huang On Sat, Jul 29, 2023 at 01:38:13PM +0100, Daniel Golle wrote: > Hi Andrew, > > On Sat, Jun 24, 2023 at 10:56:26PM +0200, Andrew Lunn wrote: > > > > Allow offloading of the LED trigger netdev to PHY drivers and > > implement it for the Marvell PHY driver. Additionally, correct the > > handling of when the initial state of the LED cannot be represented by > > the trigger, and so an error is returned. > > I've used this series in my tree and implemented support for all LED- > related features in the mediatek-ge-soc PHY driver: > > https://github.com/dangowrt/linux/commit/3b9b30a9a6699d290964fc76c56cfcd1dadf5651 > > I've noticed there is a problem when setting up the netdev trigger using > the 'linux,default-trigger' property in device tree. In this case > led_classdev_register_ext is called *before* register_netdevice has > completed. > Hence supports_hw_control returns false when the 'netdev' trigger is > initially setup as default trigger. > > To resolve this, I've tried wrapping led_classdev_register_ext() and > introducing led_classdev_register_ext_nodefault() which doesn't call > out to led_trigger_set_default, so that can be done later by the > caller. However, there isn't any good existing call from netdev to phy > informing the phy that the netdev has been registered, so the phy_leds > would have to register a netdevice_notifier and wait for the > NETDEV_REGISTER events, matching against the netdev's PHY... And that > seems a bit overkill, just to support netdev trigger offloading to work > when used as a default trigger... > > Any better ideas anyone? Hi Daniel I've not had chance to look at this yet. I really need to stop being lazy and get my Marvell patches merged, the DT binding for defaults, and then look at this ordering issue. I do however agree, it is going to be messy, since as you say, the PHY does not know what MAC it is bound to until quite late. Maybe we can use the netdev_trig_notify() to update the value from supports_hw_control()? Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-08-07 23:48 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-24 20:56 [PATCH v2 net-next 0/3] Support offload LED blinking to PHY Andrew Lunn 2023-06-24 20:56 ` [PATCH v2 net-next 1/3] led: trig: netdev: Fix requesting offload device Andrew Lunn 2023-07-18 19:34 ` Daniel Golle 2023-08-07 22:27 ` Andrew Lunn 2023-08-07 22:49 ` Daniel Golle 2023-08-07 23:48 ` Andrew Lunn 2023-06-24 20:56 ` [PATCH v2 net-next 2/3] net: phy: phy_device: Call into the PHY driver to set LED offload Andrew Lunn 2023-06-24 20:56 ` [PATCH v2 net-next 3/3] net: phy: marvell: Add support for offloading LED blinking Andrew Lunn 2023-06-26 3:33 ` Kalesh Anakkur Purayil 2023-07-29 12:38 ` [PATCH v2 net-next 0/3] Support offload LED blinking to PHY Daniel Golle 2023-07-29 17:19 ` Andrew Lunn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).