[PATCH v3 0/9] led: introduce LED boot and activity function
This series is a reworked version of the previous seried: misc: introduce STATUS LED activity function
This series port and expand the legacy concept of LED boot from the legacy Status LED API to new LED API.
One thing that many device need is a way to communicate to the user that the device is actually doing something.
This is especially useful for recovery steps where an user (for example) insert an USB drive, keep a button pressed and the device autorecover.
There is currently no way to signal the user externally that the bootloader is processing/recoverying aside from setting a LED on.
A solid LED on is not enough and won't actually signal any kind of progress. Solution is the good old blinking LED but uboot doesn't suggest (and support) interrupts and almost all the LED are usually GPIO LED that doesn't support HW blink.
Additional Kconfg are also introduced to set the LED boot and activity. Those are referenced by label.
A documentation for old and these new LED API is created.
(world tested with the azure pipeline)
Changes v3: - Switch to /config property Changes v2: - Drop GPIO SW implementation - Add fix for new LED SW BLINK
Christian Marangi (9): led: turn LED ON on initial SW blink led: implement led_set_state/period_by_label led: implement LED boot API common: board_r: rework BOOT LED handling led: implement LED activity API tftp: implement support for LED activity mtd: implement support for LED activity ubi: implement support for LED activity doc: introduce led.rst documentation
cmd/mtd.c | 19 +++ cmd/ubi.c | 17 ++- common/board_r.c | 25 +++- doc/api/index.rst | 1 + doc/api/led.rst | 10 ++ doc/device-tree-bindings/config.txt | 7 ++ drivers/led/Kconfig | 41 +++++++ drivers/led/led-uclass.c | 28 +++++ drivers/led/led_sw_blink.c | 5 +- include/led.h | 184 ++++++++++++++++++++++++++++ net/tftp.c | 7 ++ 11 files changed, 338 insertions(+), 6 deletions(-) create mode 100644 doc/api/led.rst
We currently init the LED OFF when SW blink is triggered when on_state_change() is called. This can be problematic for very short period as the ON/OFF blink might never trigger.
Turn LED ON on initial SW blink to handle this corner case and better display a LED blink from the user.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com --- drivers/led/led_sw_blink.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/led/led_sw_blink.c b/drivers/led/led_sw_blink.c index 9e36edbee47..853278670b9 100644 --- a/drivers/led/led_sw_blink.c +++ b/drivers/led/led_sw_blink.c @@ -103,8 +103,11 @@ bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state) return false;
if (state == LEDST_BLINK) { + struct led_ops *ops = led_get_ops(dev); + + ops->set_state(dev, LEDST_ON); /* start blinking on next led_sw_blink() call */ - sw_blink->state = LED_SW_BLINK_ST_OFF; + sw_blink->state = LED_SW_BLINK_ST_ON; return true; }
Am 12. August 2024 12:32:43 MESZ schrieb Christian Marangi ansuelsmth@gmail.com:
We currently init the LED OFF when SW blink is triggered when on_state_change() is called. This can be problematic for very short period as the ON/OFF blink might never trigger.
Turn LED ON on initial SW blink to handle this corner case and better display a LED blink from the user.
If the the prior state is on, blinking should start with off.
If the prior state is off, blinking should start with on.
Best regards
Heinrich
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
drivers/led/led_sw_blink.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/led/led_sw_blink.c b/drivers/led/led_sw_blink.c index 9e36edbee47..853278670b9 100644 --- a/drivers/led/led_sw_blink.c +++ b/drivers/led/led_sw_blink.c @@ -103,8 +103,11 @@ bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state) return false;
if (state == LEDST_BLINK) {
struct led_ops *ops = led_get_ops(dev);ops->set_state(dev, LEDST_ON);
sw_blink->state = LED_SW_BLINK_ST_OFF;
sw_blink->state = LED_SW_BLINK_ST_ON;
On Tue, Aug 13, 2024 at 12:00:59AM +0200, Heinrich Schuchardt wrote:
Am 12. August 2024 12:32:43 MESZ schrieb Christian Marangi ansuelsmth@gmail.com:
We currently init the LED OFF when SW blink is triggered when on_state_change() is called. This can be problematic for very short period as the ON/OFF blink might never trigger.
Turn LED ON on initial SW blink to handle this corner case and better display a LED blink from the user.
If the the prior state is on, blinking should start with off.
If the prior state is off, blinking should start with on.
A bit confused. You mean I should improve the commit description or the code needs to he changed to reflect this and check the LED status before applying the BLINK?
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
drivers/led/led_sw_blink.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/led/led_sw_blink.c b/drivers/led/led_sw_blink.c index 9e36edbee47..853278670b9 100644 --- a/drivers/led/led_sw_blink.c +++ b/drivers/led/led_sw_blink.c @@ -103,8 +103,11 @@ bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state) return false;
if (state == LEDST_BLINK) {
struct led_ops *ops = led_get_ops(dev);ops->set_state(dev, LEDST_ON);
sw_blink->state = LED_SW_BLINK_ST_OFF;
sw_blink->state = LED_SW_BLINK_ST_ON;
On 22.08.24 12:47, Christian Marangi wrote:
On Tue, Aug 13, 2024 at 12:00:59AM +0200, Heinrich Schuchardt wrote:
Am 12. August 2024 12:32:43 MESZ schrieb Christian Marangi ansuelsmth@gmail.com:
We currently init the LED OFF when SW blink is triggered when on_state_change() is called. This can be problematic for very short period as the ON/OFF blink might never trigger.
Turn LED ON on initial SW blink to handle this corner case and better display a LED blink from the user.
If the the prior state is on, blinking should start with off.
If the prior state is off, blinking should start with on.
A bit confused. You mean I should improve the commit description or the code needs to he changed to reflect this and check the LED status before applying the BLINK?
The code should take the prior state of the LED into account. When enabling blinking the on/off state of the LED should change immediately.
Best regards
Heinrich
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
drivers/led/led_sw_blink.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/led/led_sw_blink.c b/drivers/led/led_sw_blink.c index 9e36edbee47..853278670b9 100644 --- a/drivers/led/led_sw_blink.c +++ b/drivers/led/led_sw_blink.c @@ -103,8 +103,11 @@ bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state) return false;
if (state == LEDST_BLINK) {
struct led_ops *ops = led_get_ops(dev);ops->set_state(dev, LEDST_ON);
sw_blink->state = LED_SW_BLINK_ST_OFF;
sw_blink->state = LED_SW_BLINK_ST_ON;
Hi Christian,
On Mon, 12 Aug 2024 at 12:33, Christian Marangi ansuelsmth@gmail.com wrote:
We currently init the LED OFF when SW blink is triggered when on_state_change() is called. This can be problematic for very short period as the ON/OFF blink might never trigger.
Turn LED ON on initial SW blink to handle this corner case and better display a LED blink from the user.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
drivers/led/led_sw_blink.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
nit below
diff --git a/drivers/led/led_sw_blink.c b/drivers/led/led_sw_blink.c index 9e36edbee47..853278670b9 100644 --- a/drivers/led/led_sw_blink.c +++ b/drivers/led/led_sw_blink.c @@ -103,8 +103,11 @@ bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state) return false;
if (state == LEDST_BLINK) {
struct led_ops *ops = led_get_ops(dev);ops->set_state(dev, LEDST_ON);
Normally in drivers we define functions like led_set_state() in the uclass, rather than calling things directly like this.
/* start blinking on next led_sw_blink() call */
sw_blink->state = LED_SW_BLINK_ST_OFF;
sw_blink->state = LED_SW_BLINK_ST_ON; return true; }-- 2.45.2
Regards, Simon
On Thu, Sep 19, 2024 at 04:13:44PM +0200, Simon Glass wrote:
Hi Christian,
On Mon, 12 Aug 2024 at 12:33, Christian Marangi ansuelsmth@gmail.com wrote:
We currently init the LED OFF when SW blink is triggered when on_state_change() is called. This can be problematic for very short period as the ON/OFF blink might never trigger.
Turn LED ON on initial SW blink to handle this corner case and better display a LED blink from the user.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
drivers/led/led_sw_blink.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
nit below
diff --git a/drivers/led/led_sw_blink.c b/drivers/led/led_sw_blink.c index 9e36edbee47..853278670b9 100644 --- a/drivers/led/led_sw_blink.c +++ b/drivers/led/led_sw_blink.c @@ -103,8 +103,11 @@ bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state) return false;
if (state == LEDST_BLINK) {
struct led_ops *ops = led_get_ops(dev);ops->set_state(dev, LEDST_ON);Normally in drivers we define functions like led_set_state() in the uclass, rather than calling things directly like this.
I used ops directly as I'm following how it's done in led_sw_blink and because the support for these ops is already validated and we don't need to check for the -ENOSYS condition.
Hope it's ok. Also as suggested I changed the function to toggle the LED as suggested. I added the review tag. Tell me if I have to drop it in the next revision.
/* start blinking on next led_sw_blink() call */
sw_blink->state = LED_SW_BLINK_ST_OFF;
sw_blink->state = LED_SW_BLINK_ST_ON; return true; }-- 2.45.2
Regards, Simon
Introduce new API led_set_state/period_by label as a shorthand to set LED state and LED blink period by referencing them by label.
This is needed for the upcoming additional API that will declare LED in .confg and reference them by their LED label name.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com --- drivers/led/led-uclass.c | 28 ++++++++++++++++++++++++++++ include/led.h | 18 ++++++++++++++++++ 2 files changed, 46 insertions(+)
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index 199d68bc25a..b2b96d7d48b 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -65,6 +65,20 @@ int led_set_state(struct udevice *dev, enum led_state_t state) return ops->set_state(dev, state); }
+int led_set_state_by_label(const char *label, enum led_state_t state) +{ + struct udevice *dev; + int ret; + + ret = led_get_by_label(label, &dev); + if (ret) { + printf("Failed to get LED by label %s\n", label); + return ret; + } + + return led_set_state(dev, state); +} + enum led_state_t led_get_state(struct udevice *dev) { struct led_ops *ops = led_get_ops(dev); @@ -94,6 +108,20 @@ int led_set_period(struct udevice *dev, int period_ms) return -ENOSYS; }
+int led_set_period_by_label(const char *label, int period_ms) +{ + struct udevice *dev; + int ret; + + ret = led_get_by_label(label, &dev); + if (ret) { + printf("Failed to get LED by label %s\n", label); + return ret; + } + + return led_set_period(dev, period_ms); +} + static int led_post_bind(struct udevice *dev) { struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev); diff --git a/include/led.h b/include/led.h index 99f93c5ef86..c1f3380f253 100644 --- a/include/led.h +++ b/include/led.h @@ -111,6 +111,15 @@ int led_get_by_label(const char *label, struct udevice **devp); */ int led_set_state(struct udevice *dev, enum led_state_t state);
+/** + * led_set_state_by_label - set the state of an LED referenced by Label + * + * @label: LED label + * @state: LED state to set + * Return: 0 if OK, -ve on error + */ +int led_set_state_by_label(const char *label, enum led_state_t state); + /** * led_get_state() - get the state of an LED * @@ -128,6 +137,15 @@ enum led_state_t led_get_state(struct udevice *dev); */ int led_set_period(struct udevice *dev, int period_ms);
+/** + * led_set_period_by_label - set the blink period of an LED referenced by Label + * + * @label: LED label + * @period_ms: LED blink period in milliseconds + * Return: 0 if OK, -ve on error + */ +int led_set_period_by_label(const char *label, int period_ms); + /** * led_bind_generic() - bind children of parent to given driver *
Hi Christian,
On Mon, 12 Aug 2024 at 12:33, Christian Marangi ansuelsmth@gmail.com wrote:
Introduce new API led_set_state/period_by label as a shorthand to set LED state and LED blink period by referencing them by label.
This is needed for the upcoming additional API that will declare LED in .confg and reference them by their LED label name.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
drivers/led/led-uclass.c | 28 ++++++++++++++++++++++++++++ include/led.h | 18 ++++++++++++++++++ 2 files changed, 46 insertions(+)
Can you please update test/dm/led.c for the new features in this series?
Regards, Simon
Implement LED boot API to signal correct boot of the system.
led_boot_on/off/blink() are introduced to turn ON, OFF and BLINK the designated boot LED.
New Kconfig are introduced, CONFIG_LED_BOOT_ENABLE to enable the feature. This makes use of the /config property "u-boot,boot-led" to the define the boot LED. It's also introduced a new /config property "u-boot,boot-led-period" to define the default period when the LED is set to blink mode.
If "u-boot,boot-led-period" is not defined, the value of 250 (ms) is used by default.
If CONFIG_LED_BLINK or CONFIG_LED_SW_BLINK is not enabled, led_boot_blink call will fallback to simple LED ON.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com --- doc/device-tree-bindings/config.txt | 3 ++ drivers/led/Kconfig | 20 +++++++++ include/led.h | 64 +++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+)
diff --git a/doc/device-tree-bindings/config.txt b/doc/device-tree-bindings/config.txt index f50c68bbdc3..68edd177040 100644 --- a/doc/device-tree-bindings/config.txt +++ b/doc/device-tree-bindings/config.txt @@ -31,6 +31,9 @@ u-boot,error-led (string) This is used to specify the label for an LED to indicate an error and a successful boot, on supported hardware.
+u-boot,boot-led-period (int) + This is used to specify the default period for an LED in blink mode. + bootsecure (int) Indicates that U-Boot should use secure_boot_cmd() to run commands, rather than the normal CLI. This can be used in production images, to diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig index bee74b25751..fd9442edaf3 100644 --- a/drivers/led/Kconfig +++ b/drivers/led/Kconfig @@ -9,6 +9,26 @@ config LED can provide access to board-specific LEDs. Use of the device tree for configuration is encouraged.
+config LED_BOOT_ENABLE + bool "Enable LED boot support" + help + Enable LED boot support. + + LED boot is a specific LED assigned to signal boot operation status. + +config LED_BOOT_LABEL + string "LED boot label" + depends on LED_BOOT_ENABLE + help + LED label defined in DT to assign for LED boot usage. + +config LED_BOOT_PERIOD + int "LED boot period" + depends on LED_BOOT_ENABLE && (LED_BLINK || LED_SW_BLINK) + default 2 + help + LED boot blink period in ms. + config LED_BCM6328 bool "LED Support for BCM6328" depends on LED && ARCH_BMIPS diff --git a/include/led.h b/include/led.h index c1f3380f253..2d3b89674e2 100644 --- a/include/led.h +++ b/include/led.h @@ -9,6 +9,7 @@
#include <stdbool.h> #include <cyclic.h> +#include <dm/ofnode.h>
struct udevice;
@@ -159,4 +160,67 @@ int led_sw_set_period(struct udevice *dev, int period_ms); bool led_sw_is_blinking(struct udevice *dev); bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state);
+#ifdef CONFIG_LED_BOOT_ENABLE + +/** + * led_boot_on() - turn ON the designated LED for booting + * + * Return: 0 if OK, -ve on error + */ +static inline int led_boot_on(void) +{ + const char *led_name; + + led_name = ofnode_conf_read_str("u-boot,boot-led"); + if (!led_name) + return -ENOENT; + + return led_set_state_by_label(led_name, LEDST_ON); +} + +/** + * led_boot_off() - turn OFF the designated LED for booting + * + * Return: 0 if OK, -ve on error + */ +static inline int led_boot_off(void) +{ + const char *led_name; + + led_name = ofnode_conf_read_str("u-boot,boot-led"); + if (!led_name) + return -ENOENT; + + return led_set_state_by_label(CONFIG_LED_BOOT_LABEL, LEDST_OFF); +} + +#if defined(CONFIG_LED_BLINK) || defined(CONFIG_LED_SW_BLINK) +/** + * led_boot_blink() - turn ON the designated LED for booting + * + * Return: 0 if OK, -ve on error + */ +static inline int led_boot_blink(void) +{ + const char *led_name; + int led_period, ret; + + led_name = ofnode_conf_read_str("u-boot,boot-led"); + if (!led_name) + return -ENOENT; + + led_period = ofnode_conf_read_int("u-boot,boot-led-period", 250); + + ret = led_set_period_by_label(led_name, led_period); + if (ret) + return ret; + + return led_set_state_by_label(led_name, LEDST_BLINK); +} +#else +/* If LED BLINK is not supported/enabled, fallback to LED ON */ +#define led_boot_blink led_boot_on +#endif +#endif + #endif
Hi Christian,
On Mon, 12 Aug 2024 at 12:33, Christian Marangi ansuelsmth@gmail.com wrote:
Implement LED boot API to signal correct boot of the system.
led_boot_on/off/blink() are introduced to turn ON, OFF and BLINK the designated boot LED.
New Kconfig are introduced, CONFIG_LED_BOOT_ENABLE to enable the feature. This makes use of the /config property "u-boot,boot-led" to the define the boot LED. It's also introduced a new /config property "u-boot,boot-led-period" to define the default period when the LED is set to blink mode.
If "u-boot,boot-led-period" is not defined, the value of 250 (ms) is used by default.
If CONFIG_LED_BLINK or CONFIG_LED_SW_BLINK is not enabled, led_boot_blink call will fallback to simple LED ON.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
doc/device-tree-bindings/config.txt | 3 ++ drivers/led/Kconfig | 20 +++++++++ include/led.h | 64 +++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+)
diff --git a/doc/device-tree-bindings/config.txt b/doc/device-tree-bindings/config.txt index f50c68bbdc3..68edd177040 100644 --- a/doc/device-tree-bindings/config.txt +++ b/doc/device-tree-bindings/config.txt @@ -31,6 +31,9 @@ u-boot,error-led (string) This is used to specify the label for an LED to indicate an error and a successful boot, on supported hardware.
+u-boot,boot-led-period (int)
This is used to specify the default period for an LED in blink mode.
I'm sorry to say that this is out-of-date.
The new schema is in options/ and is documented at dtschema:
dtschema/schemas/options/u-boot.yaml
You should add new things there. If you have time, it would be great if you could send bindings for the things in config.txt
bootsecure (int) Indicates that U-Boot should use secure_boot_cmd() to run commands, rather than the normal CLI. This can be used in production images, to diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig index bee74b25751..fd9442edaf3 100644 --- a/drivers/led/Kconfig +++ b/drivers/led/Kconfig @@ -9,6 +9,26 @@ config LED can provide access to board-specific LEDs. Use of the device tree for configuration is encouraged.
+config LED_BOOT_ENABLE
bool "Enable LED boot support"helpEnable LED boot support.LED boot is a specific LED assigned to signal boot operation status.+config LED_BOOT_LABEL
string "LED boot label"depends on LED_BOOT_ENABLEhelpLED label defined in DT to assign for LED boot usage.+config LED_BOOT_PERIOD
int "LED boot period"depends on LED_BOOT_ENABLE && (LED_BLINK || LED_SW_BLINK)default 2
Should that be 2000? It is a very fast blink if it is 2ms
helpLED boot blink period in ms.config LED_BCM6328 bool "LED Support for BCM6328" depends on LED && ARCH_BMIPS diff --git a/include/led.h b/include/led.h index c1f3380f253..2d3b89674e2 100644 --- a/include/led.h +++ b/include/led.h @@ -9,6 +9,7 @@
#include <stdbool.h> #include <cyclic.h> +#include <dm/ofnode.h>
struct udevice;
@@ -159,4 +160,67 @@ int led_sw_set_period(struct udevice *dev, int period_ms); bool led_sw_is_blinking(struct udevice *dev); bool led_sw_on_state_change(struct udevice *dev, enum led_state_t state);
+#ifdef CONFIG_LED_BOOT_ENABLE
+/**
- led_boot_on() - turn ON the designated LED for booting
- Return: 0 if OK, -ve on error
- */
+static inline int led_boot_on(void)
I wonder why these are inline functions?
You should be able to put this code in the C file, with an #ifdef in the header to create an empty, static-inline function if needed.
+{
const char *led_name;led_name = ofnode_conf_read_str("u-boot,boot-led");if (!led_name)return -ENOENT;return led_set_state_by_label(led_name, LEDST_ON);+}
+/**
- led_boot_off() - turn OFF the designated LED for booting
- Return: 0 if OK, -ve on error
- */
+static inline int led_boot_off(void) +{
const char *led_name;led_name = ofnode_conf_read_str("u-boot,boot-led");if (!led_name)return -ENOENT;return led_set_state_by_label(CONFIG_LED_BOOT_LABEL, LEDST_OFF);+}
+#if defined(CONFIG_LED_BLINK) || defined(CONFIG_LED_SW_BLINK) +/**
- led_boot_blink() - turn ON the designated LED for booting
- Return: 0 if OK, -ve on error
- */
+static inline int led_boot_blink(void) +{
const char *led_name;int led_period, ret;led_name = ofnode_conf_read_str("u-boot,boot-led");if (!led_name)return -ENOENT;led_period = ofnode_conf_read_int("u-boot,boot-led-period", 250);ret = led_set_period_by_label(led_name, led_period);if (ret)return ret;return led_set_state_by_label(led_name, LEDST_BLINK);+} +#else +/* If LED BLINK is not supported/enabled, fallback to LED ON */ +#define led_boot_blink led_boot_on +#endif +#endif
#endif
2.45.2
Regards, Simon
Rework BOOT LED handling. There is currently one legacy implementation for BOOT LED from Status Led API.
This work on ancient implementation wused by BOOTP by setting the LED to Blink on boot and to turn it OFF when the firmware was correctly received by network.
Now that we new LED implementation have support for LED boot, rework this by also set the new BOOT LED to blink and also set it to ON before entering main loop to confirm successful boot.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com --- common/board_r.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/common/board_r.c b/common/board_r.c index d4ba245ac69..57957b4e99b 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -39,6 +39,7 @@ #include <initcall.h> #include <kgdb.h> #include <irq_func.h> +#include <led.h> #include <malloc.h> #include <mapmem.h> #include <miiphy.h> @@ -462,14 +463,30 @@ static int initr_malloc_bootparams(void) #if defined(CONFIG_LED_STATUS) static int initr_status_led(void) { -#if defined(CONFIG_LED_STATUS_BOOT) - status_led_set(CONFIG_LED_STATUS_BOOT, CONFIG_LED_STATUS_BLINKING); -#else status_led_init(); + + return 0; +} +#endif + +static int initr_boot_led_blink(void) +{ +#ifdef CONFIG_LED_STATUS_BOOT + status_led_set(CONFIG_LED_STATUS_BOOT, CONFIG_LED_STATUS_BLINKING); +#endif +#ifdef CONFIG_LED_BOOT_ENABLE + led_boot_blink(); #endif return 0; } + +static int initr_boot_led_on(void) +{ +#ifdef CONFIG_LED_BOOT_ENABLE + led_boot_on(); #endif + return 0; +}
#ifdef CONFIG_CMD_NET static int initr_net(void) @@ -716,6 +733,7 @@ static init_fnc_t init_sequence_r[] = { #if defined(CONFIG_LED_STATUS) initr_status_led, #endif + initr_boot_led_blink, /* PPC has a udelay(20) here dating from 2002. Why? */ #ifdef CONFIG_BOARD_LATE_INIT board_late_init, @@ -738,6 +756,7 @@ static init_fnc_t init_sequence_r[] = { #if defined(CFG_PRAM) initr_mem, #endif + initr_boot_led_on, run_main_loop, };
Hi Christian,
On Mon, 12 Aug 2024 at 12:33, Christian Marangi ansuelsmth@gmail.com wrote:
Rework BOOT LED handling. There is currently one legacy implementation for BOOT LED from Status Led API.
This work on ancient implementation wused by BOOTP by setting the LED
used
to Blink on boot and to turn it OFF when the firmware was correctly received by network.
Now that we new LED implementation have support for LED boot, rework this by also set the new BOOT LED to blink and also set it to ON before entering main loop to confirm successful boot.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
common/board_r.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/common/board_r.c b/common/board_r.c index d4ba245ac69..57957b4e99b 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -39,6 +39,7 @@ #include <initcall.h> #include <kgdb.h> #include <irq_func.h> +#include <led.h> #include <malloc.h> #include <mapmem.h> #include <miiphy.h> @@ -462,14 +463,30 @@ static int initr_malloc_bootparams(void) #if defined(CONFIG_LED_STATUS) static int initr_status_led(void) { -#if defined(CONFIG_LED_STATUS_BOOT)
status_led_set(CONFIG_LED_STATUS_BOOT, CONFIG_LED_STATUS_BLINKING);-#else status_led_init();
Please can you rework this to avoid the #ifdefs in this C file? You can make empty static inlines if needed... probably here you can move this code to led.h and have the #ifdef there.
return 0;+} +#endif
+static int initr_boot_led_blink(void) +{ +#ifdef CONFIG_LED_STATUS_BOOT
status_led_set(CONFIG_LED_STATUS_BOOT, CONFIG_LED_STATUS_BLINKING);+#endif +#ifdef CONFIG_LED_BOOT_ENABLE
led_boot_blink();#endif
Same here. Note it is OK to use if (IS_ENABLED(...)) - just not the #ifdef
return 0;}
+static int initr_boot_led_on(void) +{ +#ifdef CONFIG_LED_BOOT_ENABLE
led_boot_on();#endif
return 0;+}
#ifdef CONFIG_CMD_NET static int initr_net(void) @@ -716,6 +733,7 @@ static init_fnc_t init_sequence_r[] = { #if defined(CONFIG_LED_STATUS) initr_status_led, #endif
initr_boot_led_blink, /* PPC has a udelay(20) here dating from 2002. Why? */#ifdef CONFIG_BOARD_LATE_INIT board_late_init, @@ -738,6 +756,7 @@ static init_fnc_t init_sequence_r[] = { #if defined(CFG_PRAM) initr_mem, #endif
initr_boot_led_on, run_main_loop,};
-- 2.45.2
Regards, Simon
Implement LED activity API similar to BOOT LED API.
Usual activity might be a file transfer with TFTP, a flash write...
User of this API will call led_activity_on/off/blink() to signal these kind of activity.
New Kconfig are implemented similar to BOOT LED, LED_ACTIVITY_ENABLE to enable support for it.
It's introduced a new /config property "u-boot,activity-led" and "u-boot,activity-led-period" to define the activity LED label and the default period when the activity LED is set to blink mode.
If "u-boot,activity-led-period" is not defined, the value of 250 (ms) is used by default.
If CONFIG_LED_BLINK or CONFIG_LED_SW_BLINK is not enabled, led_boot_blink call will fallback to simple LED ON.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com --- doc/device-tree-bindings/config.txt | 4 ++ drivers/led/Kconfig | 21 ++++++++++ include/led.h | 63 +++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+)
diff --git a/doc/device-tree-bindings/config.txt b/doc/device-tree-bindings/config.txt index 68edd177040..cd9ec88909b 100644 --- a/doc/device-tree-bindings/config.txt +++ b/doc/device-tree-bindings/config.txt @@ -30,8 +30,12 @@ u-boot,boot-led (string) u-boot,error-led (string) This is used to specify the label for an LED to indicate an error and a successful boot, on supported hardware. +u-boot,activity-led (string) + This is used to specify the label for an LED to indicate an activity + if supported by the operation.
u-boot,boot-led-period (int) +u-boot,activity-led-period (int) This is used to specify the default period for an LED in blink mode.
bootsecure (int) diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig index fd9442edaf3..1336f943dab 100644 --- a/drivers/led/Kconfig +++ b/drivers/led/Kconfig @@ -29,6 +29,27 @@ config LED_BOOT_PERIOD help LED boot blink period in ms.
+config LED_ACTIVITY_ENABLE + bool "Enable LED activity support" + help + Enable LED activity support. + + LED activity is a specific LED assigned to signal activity operation + like file trasnfer, flash write/erase... + +config LED_ACTIVITY_LABEL + string "LED activity label" + depends on LED_ACTIVITY_ENABLE + help + LED label defined in DT to assign for LED activity usage. + +config LED_ACTIVITY_PERIOD + int "LED activity period" + depends on LED_ACTIVITY_ENABLE && (LED_BLINK || LED_SW_BLINK) + default 2 + help + LED activity blink period in ms. + config LED_BCM6328 bool "LED Support for BCM6328" depends on LED && ARCH_BMIPS diff --git a/include/led.h b/include/led.h index 2d3b89674e2..6a1471dae85 100644 --- a/include/led.h +++ b/include/led.h @@ -223,4 +223,67 @@ static inline int led_boot_blink(void) #endif #endif
+#ifdef CONFIG_LED_ACTIVITY_ENABLE + +/** + * led_activity_on() - turn ON the designated LED for activity + * + * Return: 0 if OK, -ve on error + */ +static inline int led_activity_on(void) +{ + const char *led_name; + + led_name = ofnode_conf_read_str("u-boot,activity-led"); + if (!led_name) + return -ENOENT; + + return led_set_state_by_label(led_name, LEDST_ON); +} + +/** + * led_activity_off() - turn OFF the designated LED for activity + * + * Return: 0 if OK, -ve on error + */ +static inline int led_activity_off(void) +{ + const char *led_name; + + led_name = ofnode_conf_read_str("u-boot,activity-led"); + if (!led_name) + return -ENOENT; + + return led_set_state_by_label(led_name, LEDST_OFF); +} + +#if defined(CONFIG_LED_BLINK) || defined(CONFIG_LED_SW_BLINK) +/** + * led_activity_blink() - turn ON the designated LED for activity + * + * Return: 0 if OK, -ve on error + */ +static inline int led_activity_blink(void) +{ + const char *led_name; + int led_period, ret; + + led_name = ofnode_conf_read_str("u-boot,activity-led"); + if (!led_name) + return -ENOENT; + + led_period = ofnode_conf_read_int("u-boot,activity-led-period", 250); + + ret = led_set_period_by_label(led_name, led_period); + if (ret) + return ret; + + return led_set_state_by_label(led_name, LEDST_BLINK); +} +#else +/* If LED BLINK is not supported/enabled, fallback to LED ON */ +#define led_activity_blink led_activity_on +#endif +#endif + #endif
Hi Christian,
On Mon, 12 Aug 2024 at 12:33, Christian Marangi ansuelsmth@gmail.com wrote:
Implement LED activity API similar to BOOT LED API.
Usual activity might be a file transfer with TFTP, a flash write...
User of this API will call led_activity_on/off/blink() to signal these kind of activity.
New Kconfig are implemented similar to BOOT LED, LED_ACTIVITY_ENABLE to enable support for it.
It's introduced a new /config property "u-boot,activity-led" and "u-boot,activity-led-period" to define the activity LED label and the default period when the activity LED is set to blink mode.
If "u-boot,activity-led-period" is not defined, the value of 250 (ms) is used by default.
If CONFIG_LED_BLINK or CONFIG_LED_SW_BLINK is not enabled, led_boot_blink call will fallback to simple LED ON.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
doc/device-tree-bindings/config.txt | 4 ++ drivers/led/Kconfig | 21 ++++++++++ include/led.h | 63 +++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+)
Please see comments about /options on the other patch.
This is a nice feature to have!
diff --git a/doc/device-tree-bindings/config.txt b/doc/device-tree-bindings/config.txt index 68edd177040..cd9ec88909b 100644 --- a/doc/device-tree-bindings/config.txt +++ b/doc/device-tree-bindings/config.txt @@ -30,8 +30,12 @@ u-boot,boot-led (string) u-boot,error-led (string) This is used to specify the label for an LED to indicate an error and a successful boot, on supported hardware. +u-boot,activity-led (string)
This is used to specify the label for an LED to indicate an activityif supported by the operation.u-boot,boot-led-period (int) +u-boot,activity-led-period (int) This is used to specify the default period for an LED in blink mode.
bootsecure (int) diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig index fd9442edaf3..1336f943dab 100644 --- a/drivers/led/Kconfig +++ b/drivers/led/Kconfig @@ -29,6 +29,27 @@ config LED_BOOT_PERIOD help LED boot blink period in ms.
+config LED_ACTIVITY_ENABLE
bool "Enable LED activity support"helpEnable LED activity support.LED activity is a specific LED assigned to signal activity operationlike file trasnfer, flash write/erase...+config LED_ACTIVITY_LABEL
string "LED activity label"depends on LED_ACTIVITY_ENABLEhelpLED label defined in DT to assign for LED activity usage.+config LED_ACTIVITY_PERIOD
int "LED activity period"depends on LED_ACTIVITY_ENABLE && (LED_BLINK || LED_SW_BLINK)default 2helpLED activity blink period in ms.config LED_BCM6328 bool "LED Support for BCM6328" depends on LED && ARCH_BMIPS diff --git a/include/led.h b/include/led.h index 2d3b89674e2..6a1471dae85 100644 --- a/include/led.h +++ b/include/led.h @@ -223,4 +223,67 @@ static inline int led_boot_blink(void) #endif #endif
+#ifdef CONFIG_LED_ACTIVITY_ENABLE
+/**
- led_activity_on() - turn ON the designated LED for activity
- Return: 0 if OK, -ve on error
- */
+static inline int led_activity_on(void) +{
const char *led_name;led_name = ofnode_conf_read_str("u-boot,activity-led");if (!led_name)return -ENOENT;return led_set_state_by_label(led_name, LEDST_ON);+}
+/**
- led_activity_off() - turn OFF the designated LED for activity
- Return: 0 if OK, -ve on error
- */
+static inline int led_activity_off(void) +{
const char *led_name;led_name = ofnode_conf_read_str("u-boot,activity-led");if (!led_name)return -ENOENT;
-EINVAL for devicetree things that are missing. Also, please read the config in of_to_plat() rather than doing it when the driver is actually being used.
return led_set_state_by_label(led_name, LEDST_OFF);+}
+#if defined(CONFIG_LED_BLINK) || defined(CONFIG_LED_SW_BLINK) +/**
- led_activity_blink() - turn ON the designated LED for activity
- Return: 0 if OK, -ve on error
- */
+static inline int led_activity_blink(void)
Hmm again this code should be in a C file and in of_to_plat method..
+{
const char *led_name;int led_period, ret;led_name = ofnode_conf_read_str("u-boot,activity-led");if (!led_name)return -ENOENT;led_period = ofnode_conf_read_int("u-boot,activity-led-period", 250);ret = led_set_period_by_label(led_name, led_period);if (ret)return ret;return led_set_state_by_label(led_name, LEDST_BLINK);+} +#else +/* If LED BLINK is not supported/enabled, fallback to LED ON */ +#define led_activity_blink led_activity_on +#endif +#endif
#endif
2.45.2
Regards, Simon
Implement support for LED activity. If the feature is enabled, make the defined ACTIVITY LED to signal traffic.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com --- net/tftp.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/net/tftp.c b/net/tftp.c index 2e073183d5a..45c2455336a 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -10,6 +10,7 @@ #include <efi_loader.h> #include <env.h> #include <image.h> +#include <led.h> #include <lmb.h> #include <log.h> #include <mapmem.h> @@ -192,6 +193,9 @@ static void new_transfer(void) #ifdef CONFIG_CMD_TFTPPUT tftp_put_final_block_sent = 0; #endif +#ifdef CONFIG_LED_ACTIVITY_ENABLE + led_activity_blink(); +#endif }
#ifdef CONFIG_CMD_TFTPPUT @@ -301,6 +305,9 @@ static void tftp_complete(void) time_start * 1000, "/s"); } puts("\ndone\n"); +#ifdef CONFIG_LED_ACTIVITY_ENABLE + led_activity_off(); +#endif if (!tftp_put_active) efi_set_bootdev("Net", "", tftp_filename, map_sysmem(tftp_load_addr, 0),
Hi Christian,
On Mon, 12 Aug 2024 at 12:33, Christian Marangi ansuelsmth@gmail.com wrote:
Implement support for LED activity. If the feature is enabled, make the defined ACTIVITY LED to signal traffic.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
net/tftp.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/net/tftp.c b/net/tftp.c index 2e073183d5a..45c2455336a 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -10,6 +10,7 @@ #include <efi_loader.h> #include <env.h> #include <image.h> +#include <led.h> #include <lmb.h> #include <log.h> #include <mapmem.h> @@ -192,6 +193,9 @@ static void new_transfer(void) #ifdef CONFIG_CMD_TFTPPUT tftp_put_final_block_sent = 0; #endif +#ifdef CONFIG_LED_ACTIVITY_ENABLE
led_activity_blink();
If you make that an empty static inline when !LED_ACTIVITY_ENABLE and a real function when CONFIG_LED_ACTIVITY_ENABLE, then you can drop the #ifdef here.
Also, can you drop the _ENABLE suffix? All CONFIGs enable something.
+#endif }
#ifdef CONFIG_CMD_TFTPPUT @@ -301,6 +305,9 @@ static void tftp_complete(void) time_start * 1000, "/s"); } puts("\ndone\n"); +#ifdef CONFIG_LED_ACTIVITY_ENABLE
led_activity_off();+#endif if (!tftp_put_active) efi_set_bootdev("Net", "", tftp_filename, map_sysmem(tftp_load_addr, 0), -- 2.45.2
Regards, Simon
Implement support for LED activity. If the feature is enabled, make the defined ACTIVITY LED to signal mtd write or erase operations.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com --- cmd/mtd.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/cmd/mtd.c b/cmd/mtd.c index 795aaa2b37d..ba5ee0d4d71 100644 --- a/cmd/mtd.c +++ b/cmd/mtd.c @@ -10,6 +10,7 @@
#include <command.h> #include <console.h> +#include <led.h> #if CONFIG_IS_ENABLED(CMD_MTD_OTP) #include <hexdump.h> #endif @@ -558,6 +559,11 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc, while (mtd_block_isbad(mtd, off)) off += mtd->erasesize;
+#ifdef CONFIG_LED_ACTIVITY_ENABLE + if (!read) + led_activity_blink(); +#endif + /* Loop over the pages to do the actual read/write */ while (remaining) { /* Skip the block if it is bad */ @@ -585,6 +591,11 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc, io_op.oobbuf += io_op.oobretlen; }
+#ifdef CONFIG_LED_ACTIVITY_ENABLE + if (!read) + led_activity_off(); +#endif + if (!ret && dump) mtd_dump_device_buf(mtd, start_off, buf, len, woob);
@@ -652,6 +663,10 @@ static int do_mtd_erase(struct cmd_tbl *cmdtp, int flag, int argc, erase_op.addr = off; erase_op.len = mtd->erasesize;
+#ifdef CONFIG_LED_ACTIVITY_ENABLE + led_activity_blink(); +#endif + while (len) { if (!scrub) { ret = mtd_block_isbad(mtd, erase_op.addr); @@ -680,6 +695,10 @@ static int do_mtd_erase(struct cmd_tbl *cmdtp, int flag, int argc, erase_op.addr += mtd->erasesize; }
+#ifdef CONFIG_LED_ACTIVITY_ENABLE + led_activity_off(); +#endif + if (ret && ret != -EIO) ret = CMD_RET_FAILURE; else
Implement support for LED activity. If the feature is enabled, make the defined ACTIVITY LED to signal ubi write operation.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com --- cmd/ubi.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/cmd/ubi.c b/cmd/ubi.c index 0e62e449327..6f679eae9c3 100644 --- a/cmd/ubi.c +++ b/cmd/ubi.c @@ -14,6 +14,7 @@ #include <command.h> #include <env.h> #include <exports.h> +#include <led.h> #include <malloc.h> #include <memalign.h> #include <mtd.h> @@ -488,10 +489,22 @@ exit:
int ubi_volume_write(char *volume, void *buf, loff_t offset, size_t size) { + int ret; + +#ifdef CONFIG_LED_ACTIVITY_ENABLE + led_activity_blink(); +#endif + if (!offset) - return ubi_volume_begin_write(volume, buf, size, size); + ret = ubi_volume_begin_write(volume, buf, size, size); + else + ret = ubi_volume_offset_write(volume, buf, offset, size);
- return ubi_volume_offset_write(volume, buf, offset, size); +#ifdef CONFIG_LED_ACTIVITY_ENABLE + led_activity_off(); +#endif + + return ret; }
int ubi_volume_read(char *volume, char *buf, loff_t offset, size_t size)
Hello Christian,
On 12.08.24 12:32, Christian Marangi wrote:
Implement support for LED activity. If the feature is enabled, make the defined ACTIVITY LED to signal ubi write operation.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
cmd/ubi.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/cmd/ubi.c b/cmd/ubi.c index 0e62e449327..6f679eae9c3 100644 --- a/cmd/ubi.c +++ b/cmd/ubi.c @@ -14,6 +14,7 @@ #include <command.h> #include <env.h> #include <exports.h> +#include <led.h> #include <malloc.h> #include <memalign.h> #include <mtd.h> @@ -488,10 +489,22 @@ exit:
int ubi_volume_write(char *volume, void *buf, loff_t offset, size_t size) {
- int ret;
+#ifdef CONFIG_LED_ACTIVITY_ENABLE
- led_activity_blink();
+#endif
Do we really need ifdef? May it is possible to declare an empty function when CONFIG_LED_ACTIVITY_ENABLE is not set? May this applies for the whole series?
- if (!offset)
return ubi_volume_begin_write(volume, buf, size, size);
ret = ubi_volume_begin_write(volume, buf, size, size);- else
ret = ubi_volume_offset_write(volume, buf, offset, size);
- return ubi_volume_offset_write(volume, buf, offset, size);
+#ifdef CONFIG_LED_ACTIVITY_ENABLE
- led_activity_off();
+#endif
return ret; }
int ubi_volume_read(char *volume, char *buf, loff_t offset, size_t size)
bye, Heiko
Hi all
On Wed, Aug 14, 2024 at 6:34 AM Heiko Schocher hs@denx.de wrote:
Hello Christian,
On 12.08.24 12:32, Christian Marangi wrote:
Implement support for LED activity. If the feature is enabled, make the defined ACTIVITY LED to signal ubi write operation.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
cmd/ubi.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/cmd/ubi.c b/cmd/ubi.c index 0e62e449327..6f679eae9c3 100644 --- a/cmd/ubi.c +++ b/cmd/ubi.c @@ -14,6 +14,7 @@ #include <command.h> #include <env.h> #include <exports.h> +#include <led.h> #include <malloc.h> #include <memalign.h> #include <mtd.h> @@ -488,10 +489,22 @@ exit:
int ubi_volume_write(char *volume, void *buf, loff_t offset, size_t size) {
int ret;+#ifdef CONFIG_LED_ACTIVITY_ENABLE
led_activity_blink();+#endif
Do we really need ifdef? May it is possible to declare an empty function when CONFIG_LED_ACTIVITY_ENABLE is not set? May this applies for the whole series?
if (!offset)
return ubi_volume_begin_write(volume, buf, size, size);
ret = ubi_volume_begin_write(volume, buf, size, size);elseret = ubi_volume_offset_write(volume, buf, offset, size);
return ubi_volume_offset_write(volume, buf, offset, size);+#ifdef CONFIG_LED_ACTIVITY_ENABLE
led_activity_off();+#endif
return ret;}
int ubi_volume_read(char *volume, char *buf, loff_t offset, size_t size)
I rather prefer to have some registration of events that need to be executed for a particular i/o activity and then a subscription process from led subsystem if that particular event is connected to the dts or just on a board file
Michael
bye, Heiko -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de
On Wed, Aug 14, 2024 at 10:17:18AM +0200, Michael Nazzareno Trimarchi wrote:
Hi all
On Wed, Aug 14, 2024 at 6:34 AM Heiko Schocher hs@denx.de wrote:
Hello Christian,
On 12.08.24 12:32, Christian Marangi wrote:
Implement support for LED activity. If the feature is enabled, make the defined ACTIVITY LED to signal ubi write operation.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
cmd/ubi.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/cmd/ubi.c b/cmd/ubi.c index 0e62e449327..6f679eae9c3 100644 --- a/cmd/ubi.c +++ b/cmd/ubi.c @@ -14,6 +14,7 @@ #include <command.h> #include <env.h> #include <exports.h> +#include <led.h> #include <malloc.h> #include <memalign.h> #include <mtd.h> @@ -488,10 +489,22 @@ exit:
int ubi_volume_write(char *volume, void *buf, loff_t offset, size_t size) {
int ret;+#ifdef CONFIG_LED_ACTIVITY_ENABLE
led_activity_blink();+#endif
Do we really need ifdef? May it is possible to declare an empty function when CONFIG_LED_ACTIVITY_ENABLE is not set? May this applies for the whole series?
if (!offset)
return ubi_volume_begin_write(volume, buf, size, size);
ret = ubi_volume_begin_write(volume, buf, size, size);elseret = ubi_volume_offset_write(volume, buf, offset, size);
return ubi_volume_offset_write(volume, buf, offset, size);+#ifdef CONFIG_LED_ACTIVITY_ENABLE
led_activity_off();+#endif
return ret;}
int ubi_volume_read(char *volume, char *buf, loff_t offset, size_t size)
I rather prefer to have some registration of events that need to be executed for a particular i/o activity and then a subscription process from led subsystem if that particular event is connected to the dts or just on a board file
My concern is that it might become too complex just for the sake of putting a LED intro a state. Do we have other case where such event subsystem might be useful?
Uboot is not really multi thread so we don't expect that much thing to happen at the same time. Do we have case where an i/o might happen in multiple place? Example transfering data and writing them at the same time? The common practice is to first transfer and then handle.
bye, Heiko -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de
-- Michael Nazzareno Trimarchi Co-Founder & Chief Executive Officer M. +39 347 913 2170 michael@amarulasolutions.com __________________________________
Amarula Solutions BV Joop Geesinkweg 125, 1114 AB, Amsterdam, NL T. +31 (0)85 111 9172 info@amarulasolutions.com www.amarulasolutions.com
Hi
On Sun, Aug 18, 2024 at 6:24 PM Christian Marangi ansuelsmth@gmail.com wrote:
On Wed, Aug 14, 2024 at 10:17:18AM +0200, Michael Nazzareno Trimarchi wrote:
Hi all
On Wed, Aug 14, 2024 at 6:34 AM Heiko Schocher hs@denx.de wrote:
Hello Christian,
On 12.08.24 12:32, Christian Marangi wrote:
Implement support for LED activity. If the feature is enabled, make the defined ACTIVITY LED to signal ubi write operation.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
cmd/ubi.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/cmd/ubi.c b/cmd/ubi.c index 0e62e449327..6f679eae9c3 100644 --- a/cmd/ubi.c +++ b/cmd/ubi.c @@ -14,6 +14,7 @@ #include <command.h> #include <env.h> #include <exports.h> +#include <led.h> #include <malloc.h> #include <memalign.h> #include <mtd.h> @@ -488,10 +489,22 @@ exit:
int ubi_volume_write(char *volume, void *buf, loff_t offset, size_t size) {
int ret;+#ifdef CONFIG_LED_ACTIVITY_ENABLE
led_activity_blink();+#endif
Do we really need ifdef? May it is possible to declare an empty function when CONFIG_LED_ACTIVITY_ENABLE is not set? May this applies for the whole series?
if (!offset)
return ubi_volume_begin_write(volume, buf, size, size);
ret = ubi_volume_begin_write(volume, buf, size, size);elseret = ubi_volume_offset_write(volume, buf, offset, size);
return ubi_volume_offset_write(volume, buf, offset, size);+#ifdef CONFIG_LED_ACTIVITY_ENABLE
led_activity_off();+#endif
return ret;}
int ubi_volume_read(char *volume, char *buf, loff_t offset, size_t size)
I rather prefer to have some registration of events that need to be executed for a particular i/o activity and then a subscription process from led subsystem if that particular event is connected to the dts or just on a board file
My concern is that it might become too complex just for the sake of putting a LED intro a state. Do we have other case where such event subsystem might be useful?
I was thinking of reusing the cyclic subsystem that allows you to subscribe to functions that are executed periodically. I mean it's not exciting to have function call everywhere, and anyway I think that
#if defined(CONFIG_FOO) foo_activity #else foo_activity() { }; #endif
This is my preference to not have it ENABLED everywhere. As I mentioned I even not have experience about having such needs in in code. Most can be implemented in a script except blinking like:
led on; ext4load <> ; led off. We can definitely script most of it. The only exception can be led blink; ext4load <>; led off.
Uboot is not really multi thread so we don't expect that much thing to happen at the same time. Do we have case where an i/o might happen in multiple place? Example transfering data and writing them at the same time? The common practice is to first transfer and then handle.
Michael
bye, Heiko -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de
-- Michael Nazzareno Trimarchi Co-Founder & Chief Executive Officer M. +39 347 913 2170 michael@amarulasolutions.com __________________________________
Amarula Solutions BV Joop Geesinkweg 125, 1114 AB, Amsterdam, NL T. +31 (0)85 111 9172 info@amarulasolutions.com www.amarulasolutions.com
-- Ansuel
On Sun, Aug 18, 2024 at 09:32:32PM +0200, Michael Nazzareno Trimarchi wrote:
Hi
On Sun, Aug 18, 2024 at 6:24 PM Christian Marangi ansuelsmth@gmail.com wrote:
On Wed, Aug 14, 2024 at 10:17:18AM +0200, Michael Nazzareno Trimarchi wrote:
Hi all
On Wed, Aug 14, 2024 at 6:34 AM Heiko Schocher hs@denx.de wrote:
Hello Christian,
On 12.08.24 12:32, Christian Marangi wrote:
Implement support for LED activity. If the feature is enabled, make the defined ACTIVITY LED to signal ubi write operation.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
cmd/ubi.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/cmd/ubi.c b/cmd/ubi.c index 0e62e449327..6f679eae9c3 100644 --- a/cmd/ubi.c +++ b/cmd/ubi.c @@ -14,6 +14,7 @@ #include <command.h> #include <env.h> #include <exports.h> +#include <led.h> #include <malloc.h> #include <memalign.h> #include <mtd.h> @@ -488,10 +489,22 @@ exit:
int ubi_volume_write(char *volume, void *buf, loff_t offset, size_t size) {
int ret;+#ifdef CONFIG_LED_ACTIVITY_ENABLE
led_activity_blink();+#endif
Do we really need ifdef? May it is possible to declare an empty function when CONFIG_LED_ACTIVITY_ENABLE is not set? May this applies for the whole series?
if (!offset)
return ubi_volume_begin_write(volume, buf, size, size);
ret = ubi_volume_begin_write(volume, buf, size, size);elseret = ubi_volume_offset_write(volume, buf, offset, size);
return ubi_volume_offset_write(volume, buf, offset, size);+#ifdef CONFIG_LED_ACTIVITY_ENABLE
led_activity_off();+#endif
return ret;}
int ubi_volume_read(char *volume, char *buf, loff_t offset, size_t size)
I rather prefer to have some registration of events that need to be executed for a particular i/o activity and then a subscription process from led subsystem if that particular event is connected to the dts or just on a board file
My concern is that it might become too complex just for the sake of putting a LED intro a state. Do we have other case where such event subsystem might be useful?
I was thinking of reusing the cyclic subsystem that allows you to subscribe to functions that are executed periodically. I mean it's not exciting to have function call everywhere, and anyway I think that
#if defined(CONFIG_FOO) foo_activity #else foo_activity() { }; #endif
Yes that was suggested and I will change code to use this
This is my preference to not have it ENABLED everywhere. As I mentioned I even not have experience about having such needs in in code. Most can be implemented in a script except blinking like:
led on; ext4load <> ; led off. We can definitely script most of it. The only exception can be led blink; ext4load <>; led off.
It's really a choice but currently for the boot led people have to use board code to turn on the LED or use the preboot env to run command... Not very clean. Is it really that bad to have these simple call in these functions?
Uboot is not really multi thread so we don't expect that much thing to happen at the same time. Do we have case where an i/o might happen in multiple place? Example transfering data and writing them at the same time? The common practice is to first transfer and then handle.
Michael
bye, Heiko -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de
-- Michael Nazzareno Trimarchi Co-Founder & Chief Executive Officer M. +39 347 913 2170 michael@amarulasolutions.com __________________________________
Amarula Solutions BV Joop Geesinkweg 125, 1114 AB, Amsterdam, NL T. +31 (0)85 111 9172 info@amarulasolutions.com www.amarulasolutions.com
-- Ansuel
-- Michael Nazzareno Trimarchi Co-Founder & Chief Executive Officer M. +39 347 913 2170 michael@amarulasolutions.com __________________________________
Amarula Solutions BV Joop Geesinkweg 125, 1114 AB, Amsterdam, NL T. +31 (0)85 111 9172 info@amarulasolutions.com www.amarulasolutions.com
On Wed, Aug 14, 2024 at 06:33:12AM +0200, Heiko Schocher wrote:
Hello Christian,
On 12.08.24 12:32, Christian Marangi wrote:
Implement support for LED activity. If the feature is enabled, make the defined ACTIVITY LED to signal ubi write operation.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
cmd/ubi.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/cmd/ubi.c b/cmd/ubi.c index 0e62e449327..6f679eae9c3 100644 --- a/cmd/ubi.c +++ b/cmd/ubi.c @@ -14,6 +14,7 @@ #include <command.h> #include <env.h> #include <exports.h> +#include <led.h> #include <malloc.h> #include <memalign.h> #include <mtd.h> @@ -488,10 +489,22 @@ exit: int ubi_volume_write(char *volume, void *buf, loff_t offset, size_t size) {
- int ret;
+#ifdef CONFIG_LED_ACTIVITY_ENABLE
- led_activity_blink();
+#endif
Do we really need ifdef? May it is possible to declare an empty function when CONFIG_LED_ACTIVITY_ENABLE is not set? May this applies for the whole series?
Yes can be done.
- if (!offset)
return ubi_volume_begin_write(volume, buf, size, size);
ret = ubi_volume_begin_write(volume, buf, size, size);- else
ret = ubi_volume_offset_write(volume, buf, offset, size);
- return ubi_volume_offset_write(volume, buf, offset, size);
+#ifdef CONFIG_LED_ACTIVITY_ENABLE
- led_activity_off();
+#endif
- return ret; } int ubi_volume_read(char *volume, char *buf, loff_t offset, size_t size)
bye, Heiko -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de
Introduce simple led.rst documentation to document all the additional Kconfig and the current limitation of LED_BLINK and GPIO software blink.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com --- doc/api/index.rst | 1 + doc/api/led.rst | 10 ++++++++++ include/led.h | 39 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+) create mode 100644 doc/api/led.rst
diff --git a/doc/api/index.rst b/doc/api/index.rst index ec0b8adb2cf..9f7f23f868f 100644 --- a/doc/api/index.rst +++ b/doc/api/index.rst @@ -14,6 +14,7 @@ U-Boot API documentation event getopt interrupt + led linker_lists lmb logging diff --git a/doc/api/led.rst b/doc/api/led.rst new file mode 100644 index 00000000000..e52e350d1bb --- /dev/null +++ b/doc/api/led.rst @@ -0,0 +1,10 @@ +.. SPDX-License-Identifier: GPL-2.0+ + +LED +=== + +.. kernel-doc:: include/led.h + :doc: Overview + +.. kernel-doc:: include/led.h + :internal: \ No newline at end of file diff --git a/include/led.h b/include/led.h index 6a1471dae85..fe66192eeae 100644 --- a/include/led.h +++ b/include/led.h @@ -11,6 +11,45 @@ #include <cyclic.h> #include <dm/ofnode.h>
+/** + * DOC: Overview + * + * Generic LED API provided when a supported compatible is defined in DeviceTree. + * + * To enable support for LEDs, enable the `CONFIG_LED` Kconfig option. + * + * The most common implementation is for GPIO-connected LEDs. If using GPIO-connected LEDs, + * enable the `LED_GPIO` Kconfig option. + * + * `LED_BLINK` support requires LED driver support and is therefore optional. If LED blink + * functionality is needed, enable the `LED_BLINK` Kconfig option. If LED driver doesn't + * support HW Blink, SW Blink can be used with the Cyclic framework by enabling the + * CONFIG_LED_SW_BLINK. + * + * Boot and Activity LEDs are also supported. These LEDs can signal various system operations + * during runtime, such as boot initialization, file transfers, and flash write/erase operations. + * + * To enable a Boot LED, enable `CONFIG_LED_BOOT_ENABLE` and define in `/config` root node the + * property `u-boot,boot-led`. This will enable the specified LED to blink and turn ON when + * the bootloader initializes correctly. + * + * To enable an Activity LED, enable `CONFIG_LED_ACTIVITY_ENABLE` and define in `/config` root + * node the property `u-boot,activity-led`. + * This will enable the specified LED to blink and turn ON during file transfers or flash + * write/erase operations. + * + * Both Boot and Activity LEDs provide a simple API to turn the LED ON or OFF: + * `led_boot_on()`, `led_boot_off()`, `led_activity_on()`, and `led_activity_off()`. + * + * Both configurations can optionally define a `u-boot,boot/activity-led-period` property + * if `CONFIG_LED_BLINK` or `CONFIG_LED_SW_BLINK` is enabled for LED blink operations, which + * is usually used by the Activity LED. If not defined the default value of 250 (ms) is used. + * + * When `CONFIG_LED_BLINK` or `CONFIG_LED_SW_BLINK` is enabled, additional APIs are exposed: + * `led_boot_blink()` and `led_activity_blink()`. Note that if `CONFIG_LED_BLINK` is disabled, + * these APIs will behave like the `led_boot_on()` and `led_activity_on()` APIs, respectively. + */ + struct udevice;
enum led_state_t {
participants (5)
-
Christian Marangi -
Heiko Schocher -
Heinrich Schuchardt -
Michael Nazzareno Trimarchi -
Simon Glass