[PATCH 0/7] misc: introduce STATUS LED activity function
This series expand the STATUS LED framework with a new color and a big new feature. 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.
To fix this and handle the problem of device not supporting HW blink, expand the STATUS LED framework with new API.
We introduce a new config LED_STATUS_ACTIVITY, that similar to the RED, GREEN and others, takes the LED ID set in the LED_STATUS config and is used as the global LED for activity operations.
We add status_led_activity() that simulate software blink. Any function that signal activity will call this function. At each call a counter is increased. When the counter reach the freq value, the LED is toggled simulating a blink and the counter is zeroed. When the counter reach the freq value again, the LED is toggled again and so on...
Call to this function is added to the usual operation for recovery. Currently added to tftp traffic and mtd spi and nand write and erase operation.
This also contains a big fixup for the gpio_led driver that currently deviates from the Documentation and make the coloured status led feature unusable.
Christian Marangi (7): misc: gpio_led: fix broken coloured LED status functions led: status_led: add support for white LED colour led: status_led: add function to toggle a status LED led: status_led: add new activity LED config and functions tftp: implement support for LED status activity mtd: implement support for LED status activity ubi: implement support for LED status activity
cmd/legacy_led.c | 6 ++++ cmd/mtd.c | 23 +++++++++++++++ cmd/ubi.c | 17 ++++++++++- common/board_f.c | 2 ++ drivers/led/Kconfig | 29 +++++++++++++++++++ drivers/misc/gpio_led.c | 41 +++++++++++++++++++------- drivers/misc/status_led.c | 53 +++++++++++++++++++++++++++------- drivers/mtd/nand/core.c | 4 +++ drivers/mtd/nand/spi/core.c | 5 ++++ drivers/mtd/spi/spi-nor-core.c | 9 ++++++ include/status_led.h | 6 ++++ net/tftp.c | 16 ++++++++++ 12 files changed, 189 insertions(+), 22 deletions(-)
The GPIO LED driver is a backend to provide LED status functions via the GPIO common functions.
The coloured LED functions are currently broken and deviates from what is written in README.LED
Quoting the README.LED:
CONFIG_STATUS_LED_RED is the red LED. It is used to signal errors. This must be a valid LED number (0-5). Other similar color LED's macros are CONFIG_STATUS_LED_GREEN, CONFIG_STATUS_LED_YELLOW and CONFIG_STATUS_LED_BLUE.
Hence the value of the config must refer to the ID.
Currently this is not the case and the driver expect the GPIO ID to be put in these config. On top of this to actually have these functions, a never define in Kconfig config must be declared to actually compile them. (CONFIG_GPIO_LED_STUBS)
To fix this and the GPIO problem, introduce some define that reference the LED_STATUS_BIT<n> config to have the ID->BIT connection (as described in Docs) and drop the never defined config.
The gpio_led already provide a wrapper to the functions and should not be enabled if the board provide their custom function hence it's ok to also provide the wrapper for the colour functions.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com --- drivers/misc/gpio_led.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/misc/gpio_led.c b/drivers/misc/gpio_led.c index 30679f80cf1..0f3682e1465 100644 --- a/drivers/misc/gpio_led.c +++ b/drivers/misc/gpio_led.c @@ -52,56 +52,63 @@ void __led_toggle(led_id_t mask) gpio_set_value(mask, !gpio_get_value(mask)); }
-#ifdef CONFIG_GPIO_LED_STUBS - /* 'generic' override of colored LED stubs, to use GPIO functions instead */
+/* We support up to 6 LEDs, LED 0 STATUS BIT doesn't have the number suffix */ +#define GPIO_BIT0 CONFIG_LED_STATUS_BIT +#define GPIO_BIT1 CONFIG_LED_STATUS_BIT1 +#define GPIO_BIT2 CONFIG_LED_STATUS_BIT2 +#define GPIO_BIT3 CONFIG_LED_STATUS_BIT3 +#define GPIO_BIT4 CONFIG_LED_STATUS_BIT4 +#define GPIO_BIT5 CONFIG_LED_STATUS_BIT5 +/* C preprocessor magic way to generate a GPIO_LED<id> reference */ +#define GPIO_BIT(id) ___PASTE(GPIO_BIT, id) + #ifdef CONFIG_LED_STATUS_RED + void red_led_on(void) { - __led_set(CONFIG_LED_STATUS_RED, CONFIG_LED_STATUS_ON); + __led_set(GPIO_BIT(CONFIG_LED_STATUS_RED), CONFIG_LED_STATUS_ON); }
void red_led_off(void) { - __led_set(CONFIG_LED_STATUS_RED, CONFIG_LED_STATUS_OFF); + __led_set(GPIO_BIT(CONFIG_LED_STATUS_RED), CONFIG_LED_STATUS_OFF); } #endif
#ifdef CONFIG_LED_STATUS_GREEN void green_led_on(void) { - __led_set(CONFIG_LED_STATUS_GREEN, CONFIG_LED_STATUS_ON); + __led_set(GPIO_BIT(CONFIG_LED_STATUS_GREEN), CONFIG_LED_STATUS_ON); }
void green_led_off(void) { - __led_set(CONFIG_LED_STATUS_GREEN, CONFIG_LED_STATUS_OFF); + __led_set(GPIO_BIT(CONFIG_LED_STATUS_GREEN), CONFIG_LED_STATUS_OFF); } #endif
#ifdef CONFIG_LED_STATUS_YELLOW void yellow_led_on(void) { - __led_set(CONFIG_LED_STATUS_YELLOW, CONFIG_LED_STATUS_ON); + __led_set(GPIO_BIT(CONFIG_LED_STATUS_YELLOW), CONFIG_LED_STATUS_ON); }
void yellow_led_off(void) { - __led_set(CONFIG_LED_STATUS_YELLOW, CONFIG_LED_STATUS_OFF); + __led_set(GPIO_BIT(CONFIG_LED_STATUS_YELLOW), CONFIG_LED_STATUS_OFF); } #endif
#ifdef CONFIG_LED_STATUS_BLUE void blue_led_on(void) { - __led_set(CONFIG_LED_STATUS_BLUE, CONFIG_LED_STATUS_ON); + __led_set(GPIO_BIT(CONFIG_LED_STATUS_BLUE), CONFIG_LED_STATUS_ON); }
void blue_led_off(void) { - __led_set(CONFIG_LED_STATUS_BLUE, CONFIG_LED_STATUS_OFF); + __led_set(GPIO_BIT(CONFIG_LED_STATUS_BLUE), CONFIG_LED_STATUS_OFF); } #endif - -#endif /* CONFIG_GPIO_LED_STUBS */
Add support for white LED colour present on many devices.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com --- cmd/legacy_led.c | 6 ++++++ common/board_f.c | 2 ++ drivers/led/Kconfig | 14 ++++++++++++++ drivers/misc/gpio_led.c | 12 ++++++++++++ include/status_led.h | 4 ++++ 5 files changed, 38 insertions(+)
diff --git a/cmd/legacy_led.c b/cmd/legacy_led.c index 5256255f052..40dbc05a651 100644 --- a/cmd/legacy_led.c +++ b/cmd/legacy_led.c @@ -57,6 +57,9 @@ static const led_tbl_t led_commands[] = { #endif #ifdef CONFIG_LED_STATUS_BLUE { "blue", CONFIG_LED_STATUS_BLUE, blue_led_off, blue_led_on, NULL }, +#endif +#ifdef CONFIG_LED_STATUS_WHITE + { "white", CONFIG_LED_STATUS_WHITE, white_led_off, white_led_on, NULL }, #endif { NULL, 0, NULL, NULL, NULL } }; @@ -180,6 +183,9 @@ U_BOOT_CMD( #endif #ifdef CONFIG_LED_STATUS_BLUE "blue|" +#endif +#ifdef CONFIG_LED_STATUS_WHITE + "white|" #endif "all] [on|off|toggle|blink] [blink-freq in ms]", "[led_name] [on|off|toggle|blink] sets or clears led(s)" diff --git a/common/board_f.c b/common/board_f.c index 039d6d712d0..54e2009339e 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -72,6 +72,8 @@ __weak void yellow_led_on(void) {} __weak void yellow_led_off(void) {} __weak void blue_led_on(void) {} __weak void blue_led_off(void) {} +__weak void white_led_on(void) {} +__weak void white_led_off(void) {}
/* * Why is gd allocated a register? Prior to reloc it might be better to diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig index 9837960198d..6c4f02d71f2 100644 --- a/drivers/led/Kconfig +++ b/drivers/led/Kconfig @@ -415,6 +415,20 @@ config LED_STATUS_GREEN
endif # LED_STATUS_GREEN_ENABLE
+config LED_STATUS_WHITE_ENABLE + bool "Enable white LED" + help + Enable white status LED. + +if LED_STATUS_WHITE_ENABLE + +config LED_STATUS_WHITE + int "White LED identification" + help + Valid enabled LED device number (0-5). + +endif # LED_STATUS_WHITE_ENABLE + config LED_STATUS_CMD bool "Enable status LED commands"
diff --git a/drivers/misc/gpio_led.c b/drivers/misc/gpio_led.c index 0f3682e1465..de84c206b6b 100644 --- a/drivers/misc/gpio_led.c +++ b/drivers/misc/gpio_led.c @@ -112,3 +112,15 @@ void blue_led_off(void) __led_set(GPIO_BIT(CONFIG_LED_STATUS_BLUE), CONFIG_LED_STATUS_OFF); } #endif + +#ifdef CONFIG_LED_STATUS_WHITE +void white_led_on(void) +{ + __led_set(GPIO_BIT(CONFIG_LED_STATUS_WHITE), CONFIG_LED_STATUS_ON); +} + +void white_led_off(void) +{ + __led_set(GPIO_BIT(CONFIG_LED_STATUS_WHITE), CONFIG_LED_STATUS_OFF); +} +#endif diff --git a/include/status_led.h b/include/status_led.h index 6707ab1d29d..5ce4522b029 100644 --- a/include/status_led.h +++ b/include/status_led.h @@ -87,6 +87,8 @@ void yellow_led_on(void); void yellow_led_off(void); void blue_led_on(void); void blue_led_off(void); +void white_led_on(void); +void white_led_off(void); #else .extern LED_init .extern red_led_on @@ -97,6 +99,8 @@ void blue_led_off(void); .extern green_led_off .extern blue_led_on .extern blue_led_off + .extern white_led_on + .extern white_led_off #endif
#endif /* _STATUS_LED_H_ */
Add function to toggle a status LED by using the status LED ID reference configs.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com --- drivers/misc/status_led.c | 28 +++++++++++++++++++++++----- include/status_led.h | 1 + 2 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/drivers/misc/status_led.c b/drivers/misc/status_led.c index a6e9c03a02e..93bfb410662 100644 --- a/drivers/misc/status_led.c +++ b/drivers/misc/status_led.c @@ -103,17 +103,24 @@ void status_led_tick(ulong timestamp) } }
-void status_led_set(int led, int state) +static led_dev_t *status_get_led_dev(int led) { - led_dev_t *ld; - if (led < 0 || led >= MAX_LED_DEV) - return; + return NULL;
if (!status_led_init_done) status_led_init();
- ld = &led_dev[led]; + return &led_dev[led]; +} + +void status_led_set(int led, int state) +{ + led_dev_t *ld; + + ld = status_get_led_dev(led); + if (!ld) + return;
ld->state = state; if (state == CONFIG_LED_STATUS_BLINKING) { @@ -122,3 +129,14 @@ void status_led_set(int led, int state) } __led_set (ld->mask, state); } + +void status_led_toggle(int led) +{ + led_dev_t *ld; + + ld = status_get_led_dev(led); + if (!ld) + return; + + __led_toggle(ld->mask); +} diff --git a/include/status_led.h b/include/status_led.h index 5ce4522b029..fe0c84fb4b4 100644 --- a/include/status_led.h +++ b/include/status_led.h @@ -38,6 +38,7 @@ void status_led_init(void); void status_led_tick(unsigned long timestamp); void status_led_set(int led, int state); +void status_led_toggle(int led);
/***** MVS v1 **********************************************************/ #if (defined(CONFIG_MVS) && CONFIG_MVS < 2)
Add a new activity LED config and additional functions to implement a simple software blink feature to signal activity of any kind.
Usual activity might be a file transfer with TFTP, a flash write...
Driver will call status_led_activity on each activity and LED will be toggled based on the defined FREQ config value.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com --- drivers/led/Kconfig | 15 +++++++++++++++ drivers/misc/status_led.c | 25 ++++++++++++++++++++----- include/status_led.h | 1 + 3 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig index 6c4f02d71f2..8eaa74bdd27 100644 --- a/drivers/led/Kconfig +++ b/drivers/led/Kconfig @@ -359,6 +359,21 @@ config LED_STATUS_BOOT
endif # LED_STATUS_BOOT_ENABLE
+config LED_STATUS_ACTIVITY_ENABLE + bool "Enable BOOT LED" + help + Enable to turn an LED on when the board is doing some + activity (flash write, file download). + +if LED_STATUS_ACTIVITY_ENABLE + +config LED_STATUS_ACTIVITY + int "LED to light when the board is doing some activity" + help + Valid enabled LED device number. + +endif # LED_STATUS_ACTIVITY_ENABLE + config LED_STATUS_RED_ENABLE bool "Enable red LED" help diff --git a/drivers/misc/status_led.c b/drivers/misc/status_led.c index 93bfb410662..9490e1d7341 100644 --- a/drivers/misc/status_led.c +++ b/drivers/misc/status_led.c @@ -82,6 +82,14 @@ void status_led_init(void) status_led_init_done = 1; }
+static void status_led_sw_blink(led_dev_t *ld) +{ + if (++ld->cnt >= ld->period) { + __led_toggle(ld->mask); + ld->cnt -= ld->period; + } +} + void status_led_tick(ulong timestamp) { led_dev_t *ld; @@ -95,11 +103,7 @@ void status_led_tick(ulong timestamp) if (ld->state != CONFIG_LED_STATUS_BLINKING) continue;
- if (++ld->cnt >= ld->period) { - __led_toggle (ld->mask); - ld->cnt -= ld->period; - } - + status_led_sw_blink(ld); } }
@@ -140,3 +144,14 @@ void status_led_toggle(int led)
__led_toggle(ld->mask); } + +void status_led_activity(int led) +{ + led_dev_t *ld; + + ld = status_get_led_dev(led); + if (!ld) + return; + + status_led_sw_blink(ld); +} diff --git a/include/status_led.h b/include/status_led.h index fe0c84fb4b4..037bad159c2 100644 --- a/include/status_led.h +++ b/include/status_led.h @@ -39,6 +39,7 @@ void status_led_init(void); void status_led_tick(unsigned long timestamp); void status_led_set(int led, int state); void status_led_toggle(int led); +void status_led_activity(int led);
/***** MVS v1 **********************************************************/ #if (defined(CONFIG_MVS) && CONFIG_MVS < 2)
On 05/06/2024 21:21, Christian Marangi wrote:
Add a new activity LED config and additional functions to implement a simple software blink feature to signal activity of any kind.
Usual activity might be a file transfer with TFTP, a flash write...
Driver will call status_led_activity on each activity and LED will be toggled based on the defined FREQ config value.
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
drivers/led/Kconfig | 15 +++++++++++++++ drivers/misc/status_led.c | 25 ++++++++++++++++++++----- include/status_led.h | 1 + 3 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig index 6c4f02d71f2..8eaa74bdd27 100644 --- a/drivers/led/Kconfig +++ b/drivers/led/Kconfig @@ -359,6 +359,21 @@ config LED_STATUS_BOOT
endif # LED_STATUS_BOOT_ENABLE
+config LED_STATUS_ACTIVITY_ENABLE
- bool "Enable BOOT LED"
- help
Enable to turn an LED on when the board is doing someactivity (flash write, file download).+if LED_STATUS_ACTIVITY_ENABLE
+config LED_STATUS_ACTIVITY
- int "LED to light when the board is doing some activity"
- help
Valid enabled LED device number.
I would add multiple entries here, like: LED_STATUS_ACTIVITY_STORAGE LED_STATUS_ACTIVITY_NETWORK ...
so we can enable either ones and specify different leds for each applications.
Neil
+endif # LED_STATUS_ACTIVITY_ENABLE
- config LED_STATUS_RED_ENABLE bool "Enable red LED" help
diff --git a/drivers/misc/status_led.c b/drivers/misc/status_led.c index 93bfb410662..9490e1d7341 100644 --- a/drivers/misc/status_led.c +++ b/drivers/misc/status_led.c @@ -82,6 +82,14 @@ void status_led_init(void) status_led_init_done = 1; }
+static void status_led_sw_blink(led_dev_t *ld) +{
- if (++ld->cnt >= ld->period) {
__led_toggle(ld->mask);ld->cnt -= ld->period;- }
+}
- void status_led_tick(ulong timestamp) { led_dev_t *ld;
@@ -95,11 +103,7 @@ void status_led_tick(ulong timestamp) if (ld->state != CONFIG_LED_STATUS_BLINKING) continue;
if (++ld->cnt >= ld->period) {__led_toggle (ld->mask);ld->cnt -= ld->period;}
status_led_sw_blink(ld);@@ -140,3 +144,14 @@ void status_led_toggle(int led)
__led_toggle(ld->mask); }
+void status_led_activity(int led) +{
- led_dev_t *ld;
- ld = status_get_led_dev(led);
- if (!ld)
return;- status_led_sw_blink(ld);
+} diff --git a/include/status_led.h b/include/status_led.h index fe0c84fb4b4..037bad159c2 100644 --- a/include/status_led.h +++ b/include/status_led.h @@ -39,6 +39,7 @@ void status_led_init(void); void status_led_tick(unsigned long timestamp); void status_led_set(int led, int state); void status_led_toggle(int led); +void status_led_activity(int led);
/***** MVS v1 **********************************************************/ #if (defined(CONFIG_MVS) && CONFIG_MVS < 2)
Implement support for LED status 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 | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/net/tftp.c b/net/tftp.c index 2e335413492..07dea321bb4 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -19,6 +19,7 @@ #include <asm/global_data.h> #include <net/tftp.h> #include "bootp.h" +#include <status_led.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -193,6 +194,10 @@ static void new_transfer(void) #ifdef CONFIG_CMD_TFTPPUT tftp_put_final_block_sent = 0; #endif +#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE + status_led_set(CONFIG_LED_STATUS_ACTIVITY, + CONFIG_LED_STATUS_BLINKING); +#endif }
#ifdef CONFIG_CMD_TFTPPUT @@ -228,6 +233,10 @@ static void show_block_marker(void) { ulong pos;
+#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE + status_led_activity(CONFIG_LED_STATUS_ACTIVITY); +#endif + #ifdef CONFIG_TFTP_TSIZE if (tftp_tsize) { pos = tftp_cur_block * tftp_block_size + @@ -290,6 +299,9 @@ static void tftp_complete(void) /* Print hash marks for the last packet received */ while (tftp_tsize && tftp_tsize_num_hash < 49) { putc('#'); +#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE + status_led_activity(CONFIG_LED_STATUS_ACTIVITY); +#endif tftp_tsize_num_hash++; } puts(" "); @@ -302,6 +314,10 @@ static void tftp_complete(void) time_start * 1000, "/s"); } puts("\ndone\n"); +#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE + status_led_set(CONFIG_LED_STATUS_ACTIVITY, + CONFIG_LED_STATUS_OFF); +#endif if (!tftp_put_active) efi_set_bootdev("Net", "", tftp_filename, map_sysmem(tftp_load_addr, 0),
On Wed, 5 Jun 2024 at 20:51, Christian Marangi ansuelsmth@gmail.com wrote:
Implement support for LED status activity. If the feature is enabled, make the defined ACTIVITY LED to signal traffic.
Would this not just duplicate the activity on the NIC LED?
Signed-off-by: Christian Marangi ansuelsmth@gmail.com
net/tftp.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/net/tftp.c b/net/tftp.c index 2e335413492..07dea321bb4 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -19,6 +19,7 @@ #include <asm/global_data.h> #include <net/tftp.h> #include "bootp.h" +#include <status_led.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -193,6 +194,10 @@ static void new_transfer(void) #ifdef CONFIG_CMD_TFTPPUT tftp_put_final_block_sent = 0; #endif +#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE
status_led_set(CONFIG_LED_STATUS_ACTIVITY,CONFIG_LED_STATUS_BLINKING);+#endif }
#ifdef CONFIG_CMD_TFTPPUT @@ -228,6 +233,10 @@ static void show_block_marker(void) { ulong pos;
+#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE
status_led_activity(CONFIG_LED_STATUS_ACTIVITY);+#endif
#ifdef CONFIG_TFTP_TSIZE if (tftp_tsize) { pos = tftp_cur_block * tftp_block_size + @@ -290,6 +299,9 @@ static void tftp_complete(void) /* Print hash marks for the last packet received */ while (tftp_tsize && tftp_tsize_num_hash < 49) { putc('#'); +#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE
status_led_activity(CONFIG_LED_STATUS_ACTIVITY);+#endif tftp_tsize_num_hash++; } puts(" "); @@ -302,6 +314,10 @@ static void tftp_complete(void) time_start * 1000, "/s"); } puts("\ndone\n"); +#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE
status_led_set(CONFIG_LED_STATUS_ACTIVITY,CONFIG_LED_STATUS_OFF);+#endif if (!tftp_put_active) efi_set_bootdev("Net", "", tftp_filename, map_sysmem(tftp_load_addr, 0), -- 2.43.0
On Thu, Jun 06, 2024 at 09:22:05AM +0100, Peter Robinson wrote:
On Wed, 5 Jun 2024 at 20:51, Christian Marangi ansuelsmth@gmail.com wrote:
Implement support for LED status activity. If the feature is enabled, make the defined ACTIVITY LED to signal traffic.
Would this not just duplicate the activity on the NIC LED?
Yes but most of the time Vendor doesn't ship NIC port with LED attached or even some PHY require additional setup to make the NIC LED blink on traffic.
Implement support for LED status 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 | 23 +++++++++++++++++++++++ drivers/mtd/nand/core.c | 4 ++++ drivers/mtd/nand/spi/core.c | 5 +++++ drivers/mtd/spi/spi-nor-core.c | 9 +++++++++ 4 files changed, 41 insertions(+)
diff --git a/cmd/mtd.c b/cmd/mtd.c index 9189f45cabd..aa0a41ac3bb 100644 --- a/cmd/mtd.c +++ b/cmd/mtd.c @@ -11,6 +11,7 @@ #include <command.h> #include <common.h> #include <console.h> +#include <status_led.h> #if CONFIG_IS_ENABLED(CMD_MTD_OTP) #include <hexdump.h> #endif @@ -559,6 +560,12 @@ 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_STATUS_ACTIVITY_ENABLE + if (!read) + status_led_set(CONFIG_LED_STATUS_ACTIVITY, + CONFIG_LED_STATUS_BLINKING); +#endif + /* Loop over the pages to do the actual read/write */ while (remaining) { /* Skip the block if it is bad */ @@ -586,6 +593,12 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc, io_op.oobbuf += io_op.oobretlen; }
+#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE + if (!read) + status_led_set(CONFIG_LED_STATUS_ACTIVITY, + CONFIG_LED_STATUS_OFF); +#endif + if (!ret && dump) mtd_dump_device_buf(mtd, start_off, buf, len, woob);
@@ -653,6 +666,11 @@ 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_STATUS_ACTIVITY_ENABLE + status_led_set(CONFIG_LED_STATUS_ACTIVITY, + CONFIG_LED_STATUS_ON); +#endif + while (len) { if (!scrub) { ret = mtd_block_isbad(mtd, erase_op.addr); @@ -681,6 +699,11 @@ static int do_mtd_erase(struct cmd_tbl *cmdtp, int flag, int argc, erase_op.addr += mtd->erasesize; }
+#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE + status_led_set(CONFIG_LED_STATUS_ACTIVITY, + CONFIG_LED_STATUS_OFF); +#endif + if (ret && ret != -EIO) ret = CMD_RET_FAILURE; else diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c index f6d9c584f78..631a4c83e04 100644 --- a/drivers/mtd/nand/core.c +++ b/drivers/mtd/nand/core.c @@ -18,6 +18,7 @@ #include <linux/bitops.h> #include <linux/mtd/nand.h> #include <linux/printk.h> +#include <status_led.h>
/** * nanddev_isbad() - Check if a block is bad @@ -182,6 +183,9 @@ int nanddev_mtd_erase(struct mtd_info *mtd, struct erase_info *einfo) }
nanddev_pos_next_eraseblock(nand, &pos); +#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE + status_led_activity(CONFIG_LED_STATUS_ACTIVITY); +#endif }
return 0; diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c index 62c28aa422d..86d7ed9e9d0 100644 --- a/drivers/mtd/nand/spi/core.c +++ b/drivers/mtd/nand/spi/core.c @@ -32,6 +32,7 @@ #include <linux/bug.h> #include <linux/mtd/spinand.h> #include <linux/printk.h> +#include <status_led.h> #endif
/* SPI NAND index visible in MTD names */ @@ -657,6 +658,10 @@ static int spinand_mtd_write(struct mtd_info *mtd, loff_t to,
ops->retlen += iter.req.datalen; ops->oobretlen += iter.req.ooblen; + +#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE + status_led_activity(CONFIG_LED_STATUS_ACTIVITY); +#endif }
#ifndef __UBOOT__ diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index f86003ca8c0..1e7436079e9 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -32,6 +32,8 @@ #include <spi-mem.h> #include <spi.h>
+#include <status_led.h> + #include "sf_internal.h"
/* Define max times to check status register before we give up. */ @@ -1039,6 +1041,10 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) ret = spi_nor_wait_till_ready(nor); if (ret) goto erase_err; + +#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE + status_led_activity(CONFIG_LED_STATUS_ACTIVITY); +#endif }
addr_known = false; @@ -1816,6 +1822,9 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, goto write_err; *retlen += written; i += written; +#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE + status_led_activity(CONFIG_LED_STATUS_ACTIVITY); +#endif }
write_err:
Implement support for LED status 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, 16 insertions(+), 1 deletion(-)
diff --git a/cmd/ubi.c b/cmd/ubi.c index 0a6a80bdd10..c300f1dc853 100644 --- a/cmd/ubi.c +++ b/cmd/ubi.c @@ -20,6 +20,7 @@ #include <mtd.h> #include <nand.h> #include <onenand_uboot.h> +#include <status_led.h> #include <dm/devres.h> #include <linux/mtd/mtd.h> #include <linux/mtd/partitions.h> @@ -417,7 +418,21 @@ int ubi_volume_begin_write(char *volume, void *buf, size_t size,
int ubi_volume_write(char *volume, void *buf, size_t size) { - return ubi_volume_begin_write(volume, buf, size, size); + int ret; + +#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE + status_led_set(CONFIG_LED_STATUS_ACTIVITY, + CONFIG_LED_STATUS_BLINKING); +#endif + + ret = ubi_volume_begin_write(volume, buf, size, size); + +#ifdef CONFIG_LED_STATUS_ACTIVITY_ENABLE + status_led_set(CONFIG_LED_STATUS_ACTIVITY, + CONFIG_LED_STATUS_OFF); +#endif + + return ret; }
int ubi_volume_read(char *volume, char *buf, size_t size)
On Wed, Jun 05, 2024 at 09:21:32PM +0200, Christian Marangi wrote:
This series expand the STATUS LED framework with a new color and a big new feature. 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.
To fix this and handle the problem of device not supporting HW blink, expand the STATUS LED framework with new API.
We introduce a new config LED_STATUS_ACTIVITY, that similar to the RED, GREEN and others, takes the LED ID set in the LED_STATUS config and is used as the global LED for activity operations.
We add status_led_activity() that simulate software blink. Any function that signal activity will call this function. At each call a counter is increased. When the counter reach the freq value, the LED is toggled simulating a blink and the counter is zeroed. When the counter reach the freq value again, the LED is toggled again and so on...
Call to this function is added to the usual operation for recovery. Currently added to tftp traffic and mtd spi and nand write and erase operation.
This also contains a big fixup for the gpio_led driver that currently deviates from the Documentation and make the coloured status led feature unusable.
Thanks for the work here, this is quite interesting. My only real feedback is, can you please rewrite doc/README.LED as doc/api/led.rst (and add it to the index), and then document your new functionality there as well?
On Wed, Jun 05, 2024 at 02:23:21PM -0600, Tom Rini wrote:
On Wed, Jun 05, 2024 at 09:21:32PM +0200, Christian Marangi wrote:
This series expand the STATUS LED framework with a new color and a big new feature. 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.
To fix this and handle the problem of device not supporting HW blink, expand the STATUS LED framework with new API.
We introduce a new config LED_STATUS_ACTIVITY, that similar to the RED, GREEN and others, takes the LED ID set in the LED_STATUS config and is used as the global LED for activity operations.
We add status_led_activity() that simulate software blink. Any function that signal activity will call this function. At each call a counter is increased. When the counter reach the freq value, the LED is toggled simulating a blink and the counter is zeroed. When the counter reach the freq value again, the LED is toggled again and so on...
Call to this function is added to the usual operation for recovery. Currently added to tftp traffic and mtd spi and nand write and erase operation.
This also contains a big fixup for the gpio_led driver that currently deviates from the Documentation and make the coloured status led feature unusable.
Thanks for the work here, this is quite interesting. My only real feedback is, can you please rewrite doc/README.LED as doc/api/led.rst (and add it to the index), and then document your new functionality there as well?
Yes totally, actually my bad for not updating the Documentation. Honestly I really wanted an early feedback on this for the implementation. Also I love how it's very easy to add this activity thing also in other cmd or other task.
Hi Christian,
On 6/5/24 9:21 PM, Christian Marangi wrote:
This series expand the STATUS LED framework with a new color and a big new feature. 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.
I haven't used it yet but we do have a cyclic framework now for things happening in the background. I think this is a good use-case for this framework? Something would set the blinking frequency (could be from CLI directly, or as part of board files, or architecture, etc...) and the LED would just blink then. This would allow to highlight stages in the boot process, though that is not like an activity LED so if you're stuck in a stage, you wouldn't know if something is still happening or if you're really stuck (e.g. no packet on TFTP or TFTP very slow). The benefit is that it would be way less intrusive than patching all commands that could make use of that LED. Right now, this only adds support to MTD, SPI and TFTP, but what about MMC, NVMe, USB, other net stuff (e.g. wget), etc...
Cheers, Quentin
On Thu, Jun 06, 2024 at 11:12:11AM +0200, Quentin Schulz wrote:
Hi Christian,
On 6/5/24 9:21 PM, Christian Marangi wrote:
This series expand the STATUS LED framework with a new color and a big new feature. 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.
I haven't used it yet but we do have a cyclic framework now for things happening in the background. I think this is a good use-case for this framework? Something would set the blinking frequency (could be from CLI directly, or as part of board files, or architecture, etc...) and the LED would just blink then. This would allow to highlight stages in the boot process, though that is not like an activity LED so if you're stuck in a stage, you wouldn't know if something is still happening or if you're really stuck (e.g. no packet on TFTP or TFTP very slow). The benefit is that it would be way less intrusive than patching all commands that could make use of that LED. Right now, this only adds support to MTD, SPI and TFTP, but what about MMC, NVMe, USB, other net stuff (e.g. wget), etc...
Can you hint me on where is this framework? Checking the tftp code i couldn't find extra call to it. Maybe it's attached to the schedule() function?
Also notice that it's really not a one setting since almost all device have GPIO LEDs and doesn't have a way to support HW Blink so the "activity" function needs to be called multiple time to increase the counter and toggle the LED.
Also this have the extra feature that you can check if something gets stuck in the process. The lifecycle is: - Turn on the ACTIVITY LED at the start of the thing - Blink once in a while (for very little task this might not happen) - Turn off the ACTIVITY LED at the end of the thing
Soo if something goes wrong the LED would never turn OFF but would stay solid ON.
More than happy to rework this to a less intrusive implementation.
Maybe this can be generalized to some generic API like task_start(), task_processing() and task_end()? Might make more sense than having to add specific LED function to each function?
(AFAIK linux kernel have something similar used for all the trace framework so having something in uboot to trace these kind of operation might be interesting)
Hi Christian,
On 6/6/24 11:52 AM, Christian Marangi wrote:
On Thu, Jun 06, 2024 at 11:12:11AM +0200, Quentin Schulz wrote:
Hi Christian,
On 6/5/24 9:21 PM, Christian Marangi wrote:
This series expand the STATUS LED framework with a new color and a big new feature. 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.
I haven't used it yet but we do have a cyclic framework now for things happening in the background. I think this is a good use-case for this framework? Something would set the blinking frequency (could be from CLI directly, or as part of board files, or architecture, etc...) and the LED would just blink then. This would allow to highlight stages in the boot process, though that is not like an activity LED so if you're stuck in a stage, you wouldn't know if something is still happening or if you're really stuck (e.g. no packet on TFTP or TFTP very slow). The benefit is that it would be way less intrusive than patching all commands that could make use of that LED. Right now, this only adds support to MTD, SPI and TFTP, but what about MMC, NVMe, USB, other net stuff (e.g. wget), etc...
Can you hint me on where is this framework? Checking the tftp code i couldn't find extra call to it. Maybe it's attached to the schedule() function?
https://docs.u-boot.org/en/latest/develop/cyclic.html
Also notice that it's really not a one setting since almost all device have GPIO LEDs and doesn't have a way to support HW Blink so the "activity" function needs to be called multiple time to increase the counter and toggle the LED.
Cyclic callback would be called twice per expected blink period, where you would toggle the GPIO (essentially making it 50% duty cycle, but could be more fine-grained if you want a different duty cycle).
Also this have the extra feature that you can check if something gets stuck in the process. The lifecycle is:
- Turn on the ACTIVITY LED at the start of the thing
- Blink once in a while (for very little task this might not happen)
- Turn off the ACTIVITY LED at the end of the thing
Soo if something goes wrong the LED would never turn OFF but would stay solid ON.
Yes, that's something that wouldn't be covered by cyclic framework here. It all depends what you want to do, if it's an activity LED, then we need to hook ourselves deep into frameworks where stuff is actually happening. If it's just to specify which stage of the boot we reached, then cyclic would be good enough probably (register for stage 1, unregister stage1+register for stage2 for different frequency, etc...).
More than happy to rework this to a less intrusive implementation.
Maybe this can be generalized to some generic API like task_start(), task_processing() and task_end()? Might make more sense than having to add specific LED function to each function?
This also likely would introduce a hit in performance if we need to toggle the GPIO in the same thread that we do TFTP/storage medium reading/writing? I assume we could still adapt cyclic to make it spawn a one time event instead of looping (e.g. by unregistering itself at the end of its own callback?).
(AFAIK linux kernel have something similar used for all the trace framework so having something in uboot to trace these kind of operation might be interesting)
Indeed, that's what's being done with ledtrig_.* functions, they are however scheduled on a workqueue and called from the subsystem directly.
I'm a bit confused also as to why we control the LED blinking from the cmd/ ? E.g. for cmd/mtd.c I would assume that the changes made to the mtd subsystem should be enough to handle those? Similarly, since UBI is for use over NAND/MTD, shouldn't that already be handled by the MTD subsystem, and if not, why not in the UBI subsystem instead of the CMD_UBI? One of the issues is that you may not necessarily go through the cmd/ to do stuff with storage medium or network (e.g. directly from board files).
For the net protocols, why not hook this to the net_[sg]et_*_handler for example so it's protocol-agnostic? No clue how difficult this would be, or if you'd rather have something like per-protocol activity?
Finally, maybe we also want to have a Kconfig symbol per "type" of activity to control what should be "monitored", and I would also suggest if we go this route to have a Kconfig symbol for the frequency per "type" of activity as well, so that one can know which activity is happening right now.
Nothing against this patch series or its current implementation, just throwing ideas :)
Cheers, Quentin
On Thu, Jun 06, 2024 at 12:55:37PM +0200, Quentin Schulz wrote:
Hi Christian,
On 6/6/24 11:52 AM, Christian Marangi wrote:
On Thu, Jun 06, 2024 at 11:12:11AM +0200, Quentin Schulz wrote:
Hi Christian,
On 6/5/24 9:21 PM, Christian Marangi wrote:
This series expand the STATUS LED framework with a new color and a big new feature. 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.
I haven't used it yet but we do have a cyclic framework now for things happening in the background. I think this is a good use-case for this framework? Something would set the blinking frequency (could be from CLI directly, or as part of board files, or architecture, etc...) and the LED would just blink then. This would allow to highlight stages in the boot process, though that is not like an activity LED so if you're stuck in a stage, you wouldn't know if something is still happening or if you're really stuck (e.g. no packet on TFTP or TFTP very slow). The benefit is that it would be way less intrusive than patching all commands that could make use of that LED. Right now, this only adds support to MTD, SPI and TFTP, but what about MMC, NVMe, USB, other net stuff (e.g. wget), etc...
Can you hint me on where is this framework? Checking the tftp code i couldn't find extra call to it. Maybe it's attached to the schedule() function?
Thanks looks very interesting and looks handy to make use of the watchdog for it. I will try now to rework the implementation for the sw blink to make use of cyclic thing.
Also notice that it's really not a one setting since almost all device have GPIO LEDs and doesn't have a way to support HW Blink so the "activity" function needs to be called multiple time to increase the counter and toggle the LED.
Cyclic callback would be called twice per expected blink period, where you would toggle the GPIO (essentially making it 50% duty cycle, but could be more fine-grained if you want a different duty cycle).
Well status LED already have CONFIG_STATUS_LED_FREQ where you can set a value. I will just use this.
Also this have the extra feature that you can check if something gets stuck in the process. The lifecycle is:
- Turn on the ACTIVITY LED at the start of the thing
- Blink once in a while (for very little task this might not happen)
- Turn off the ACTIVITY LED at the end of the thing
Soo if something goes wrong the LED would never turn OFF but would stay solid ON.
Yes, that's something that wouldn't be covered by cyclic framework here. It all depends what you want to do, if it's an activity LED, then we need to hook ourselves deep into frameworks where stuff is actually happening. If it's just to specify which stage of the boot we reached, then cyclic would be good enough probably (register for stage 1, unregister stage1+register for stage2 for different frequency, etc...).
The cyclic framework can reduce the implementation to just START and STOP. We would lose the ability to know if there is an actual progress or not tho... So maybe that is bad but honestly a TFTP transfer can be tracked by the other machine and MTD write/erase won't magically stop and get stalled... (and even with that they will timeout and the status LED stop will be called anyway)
So a dumb blinking with the watchdog is O.K. This is really a simple thing to show that something is happening (use case of recovering the device without actually using serial)
More than happy to rework this to a less intrusive implementation.
Maybe this can be generalized to some generic API like task_start(), task_processing() and task_end()? Might make more sense than having to add specific LED function to each function?
This also likely would introduce a hit in performance if we need to toggle the GPIO in the same thread that we do TFTP/storage medium reading/writing? I assume we could still adapt cyclic to make it spawn a one time event instead of looping (e.g. by unregistering itself at the end of its own callback?).
But the penality is that bad? Unless you have the crazy idea of an absurt low value for the freq, it would be triggered once every 200 iteration. If you are transfering MB of data you are probably on x86 or high end device where a GPIO bit set doesn't really affect anything.
My current idea is that start() will register a cyclic and LED will blink with watchdog. Stop will deregister it.
(AFAIK linux kernel have something similar used for all the trace framework so having something in uboot to trace these kind of operation might be interesting)
Indeed, that's what's being done with ledtrig_.* functions, they are however scheduled on a workqueue and called from the subsystem directly.
I'm a bit confused also as to why we control the LED blinking from the cmd/ ? E.g. for cmd/mtd.c I would assume that the changes made to the mtd subsystem should be enough to handle those? Similarly, since UBI is for use over NAND/MTD, shouldn't that already be handled by the MTD subsystem, and if not, why not in the UBI subsystem instead of the CMD_UBI? One of the issues is that you may not necessarily go through the cmd/ to do stuff with storage medium or network (e.g. directly from board files).
It's a mix of cmd and subsystem cause you need to turn the LED on. Use the activity function to increase the conunter and toggle the LED at actual activity (example a mtd write command, a single block received from TFTP)
If the cyclic thing works as I think MAYBE we can just add this to the cmd part without having to disturb all the subsystem. And that is a much cleaner approach.
For the net protocols, why not hook this to the net_[sg]et_*_handler for example so it's protocol-agnostic? No clue how difficult this would be, or if you'd rather have something like per-protocol activity?
Attaching to the net handler would provide a way too generic activity thing and would result in a dump NIC LED. We want to show activity when we actually want to signal stuff not when the device is executin ASM instructions ahahah
So yes I feel adding this way in way too generic subsystem might deviates from the idea of this. Even attaching to the write and erase function was a bit problematic and too generic.
Finally, maybe we also want to have a Kconfig symbol per "type" of activity to control what should be "monitored", and I would also suggest if we go this route to have a Kconfig symbol for the frequency per "type" of activity as well, so that one can know which activity is happening right now.
Yes that might be good but I have some fear we might explode with Kconfig. I think it's better to first have a solid idea on how this works and then we can think of the configuration part.
Nothing against this patch series or its current implementation, just throwing ideas :)
Nha thanks for the cyclic API looks very interesting and when I was implementing this I was searching for a similar thing. If it works correctly implementation will be even simpler than it is now.
Cheers, Quentin
Hi Christian,
On 6/6/24 1:52 PM, Christian Marangi wrote:
On Thu, Jun 06, 2024 at 12:55:37PM +0200, Quentin Schulz wrote:
Hi Christian,
On 6/6/24 11:52 AM, Christian Marangi wrote:
On Thu, Jun 06, 2024 at 11:12:11AM +0200, Quentin Schulz wrote:
Hi Christian,
On 6/5/24 9:21 PM, Christian Marangi wrote:
This series expand the STATUS LED framework with a new color and a big new feature. 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.
I haven't used it yet but we do have a cyclic framework now for things happening in the background. I think this is a good use-case for this framework? Something would set the blinking frequency (could be from CLI directly, or as part of board files, or architecture, etc...) and the LED would just blink then. This would allow to highlight stages in the boot process, though that is not like an activity LED so if you're stuck in a stage, you wouldn't know if something is still happening or if you're really stuck (e.g. no packet on TFTP or TFTP very slow). The benefit is that it would be way less intrusive than patching all commands that could make use of that LED. Right now, this only adds support to MTD, SPI and TFTP, but what about MMC, NVMe, USB, other net stuff (e.g. wget), etc...
Can you hint me on where is this framework? Checking the tftp code i couldn't find extra call to it. Maybe it's attached to the schedule() function?
Thanks looks very interesting and looks handy to make use of the watchdog for it. I will try now to rework the implementation for the sw blink to make use of cyclic thing.
Also notice that it's really not a one setting since almost all device have GPIO LEDs and doesn't have a way to support HW Blink so the "activity" function needs to be called multiple time to increase the counter and toggle the LED.
Cyclic callback would be called twice per expected blink period, where you would toggle the GPIO (essentially making it 50% duty cycle, but could be more fine-grained if you want a different duty cycle).
Well status LED already have CONFIG_STATUS_LED_FREQ where you can set a value. I will just use this.
This actually only appears in the rST doc, nothing actually makes use of this right now, so it's not something we **need** to use.
What I meant is, if you only provide a frequency, a specific, hardcoded, pattern is expected. E.g. for 1KHz, you enable the LED for 0.5ms and disable it for 0.5ms (or 1ms and 1ms, depending on how you see LED_STATUS_FREQ working). Could be 0.2ms and 0.8ms but it would always be this. How do you differentiate between "something is happening on NAND" and "TFTP is being used" if you don't have the ability to change the duty cycle? Or are you expecting people to have multiple LED of different colors for that?
Also this have the extra feature that you can check if something gets stuck in the process. The lifecycle is:
- Turn on the ACTIVITY LED at the start of the thing
- Blink once in a while (for very little task this might not happen)
- Turn off the ACTIVITY LED at the end of the thing
Soo if something goes wrong the LED would never turn OFF but would stay solid ON.
Yes, that's something that wouldn't be covered by cyclic framework here. It all depends what you want to do, if it's an activity LED, then we need to hook ourselves deep into frameworks where stuff is actually happening. If it's just to specify which stage of the boot we reached, then cyclic would be good enough probably (register for stage 1, unregister stage1+register for stage2 for different frequency, etc...).
The cyclic framework can reduce the implementation to just START and STOP. We would lose the ability to know if there is an actual progress or not tho... So maybe that is bad but honestly a TFTP transfer can be tracked by the other machine and MTD write/erase won't magically stop and get stalled... (and even with that they will timeout and the status LED stop will be called anyway)
So a dumb blinking with the watchdog is O.K. This is really a simple thing to show that something is happening (use case of recovering the device without actually using serial)
Then it's not so much an activity LED anymore, rather a "i'm still alive and doing X thing right now, but maybe I'm stuck who knows", e.g. a little bit like a glorified heartbeat (I'm not saying it's bad, it's just a different use case :) ).
More than happy to rework this to a less intrusive implementation.
Maybe this can be generalized to some generic API like task_start(), task_processing() and task_end()? Might make more sense than having to add specific LED function to each function?
This also likely would introduce a hit in performance if we need to toggle the GPIO in the same thread that we do TFTP/storage medium reading/writing? I assume we could still adapt cyclic to make it spawn a one time event instead of looping (e.g. by unregistering itself at the end of its own callback?).
But the penality is that bad? Unless you have the crazy idea of an absurt low value for the freq, it would be triggered once every 200 iteration. If you are transfering MB of data you are probably on x86 or high end device where a GPIO bit set doesn't really affect anything.
The kernel on Aarch64 is usually a few MB, sometimes a few tens of MB, then you may have an initramfs which is also in the tens of MB.
People are trying to get U-Boot to boot into Linux ASAP, so maybe they'll be bothered by this. If they are, they can always disable the support for this LED status feature or improve it, so I wouldn't be too worried about it for now since it won't be on by default anyway?
My current idea is that start() will register a cyclic and LED will blink with watchdog. Stop will deregister it.
Sounds good :)
(AFAIK linux kernel have something similar used for all the trace framework so having something in uboot to trace these kind of operation might be interesting)
Indeed, that's what's being done with ledtrig_.* functions, they are however scheduled on a workqueue and called from the subsystem directly.
I'm a bit confused also as to why we control the LED blinking from the cmd/ ? E.g. for cmd/mtd.c I would assume that the changes made to the mtd subsystem should be enough to handle those? Similarly, since UBI is for use over NAND/MTD, shouldn't that already be handled by the MTD subsystem, and if not, why not in the UBI subsystem instead of the CMD_UBI? One of the issues is that you may not necessarily go through the cmd/ to do stuff with storage medium or network (e.g. directly from board files).
It's a mix of cmd and subsystem cause you need to turn the LED on. Use the activity function to increase the conunter and toggle the LED at actual activity (example a mtd write command, a single block received from TFTP)
If the cyclic thing works as I think MAYBE we can just add this to the cmd part without having to disturb all the subsystem. And that is a much cleaner approach.
Why isn't the subsystem turning the LED on when writing to the counter, why does it have to go through the CMD?
If I'm not mistaken, I have my disk-activity LED in Linux working just fine without having to rely on a userspace tool to activate it for me? Maybe I missed something there though :)
For the net protocols, why not hook this to the net_[sg]et_*_handler for example so it's protocol-agnostic? No clue how difficult this would be, or if you'd rather have something like per-protocol activity?
Attaching to the net handler would provide a way too generic activity thing and would result in a dump NIC LED. We want to show activity when we actually want to signal stuff not when the device is executin ASM instructions ahahah
How does one define "when we actually want to signal stuff"? The further away you're from the subsystem, the more consumers we're going to have to patch to add this logic.
So yes I feel adding this way in way too generic subsystem might deviates from the idea of this. Even attaching to the write and erase function was a bit problematic and too generic.
Finally, maybe we also want to have a Kconfig symbol per "type" of activity to control what should be "monitored", and I would also suggest if we go this route to have a Kconfig symbol for the frequency per "type" of activity as well, so that one can know which activity is happening right now.
Yes that might be good but I have some fear we might explode with Kconfig. I think it's better to first have a solid idea on how this works and then we can think of the configuration part.
Oh, we've seen much worse with the VPL_/TPL_/SPL_// symbols :) Can come later though if someone has a usecase for it.
Cheers, Quentin
On Thu, Jun 06, 2024 at 03:32:14PM +0200, Quentin Schulz wrote:
Hi Christian,
On 6/6/24 1:52 PM, Christian Marangi wrote:
On Thu, Jun 06, 2024 at 12:55:37PM +0200, Quentin Schulz wrote:
Hi Christian,
On 6/6/24 11:52 AM, Christian Marangi wrote:
On Thu, Jun 06, 2024 at 11:12:11AM +0200, Quentin Schulz wrote:
Hi Christian,
On 6/5/24 9:21 PM, Christian Marangi wrote:
This series expand the STATUS LED framework with a new color and a big new feature. 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.
I haven't used it yet but we do have a cyclic framework now for things happening in the background. I think this is a good use-case for this framework? Something would set the blinking frequency (could be from CLI directly, or as part of board files, or architecture, etc...) and the LED would just blink then. This would allow to highlight stages in the boot process, though that is not like an activity LED so if you're stuck in a stage, you wouldn't know if something is still happening or if you're really stuck (e.g. no packet on TFTP or TFTP very slow). The benefit is that it would be way less intrusive than patching all commands that could make use of that LED. Right now, this only adds support to MTD, SPI and TFTP, but what about MMC, NVMe, USB, other net stuff (e.g. wget), etc...
Can you hint me on where is this framework? Checking the tftp code i couldn't find extra call to it. Maybe it's attached to the schedule() function?
Thanks looks very interesting and looks handy to make use of the watchdog for it. I will try now to rework the implementation for the sw blink to make use of cyclic thing.
Also notice that it's really not a one setting since almost all device have GPIO LEDs and doesn't have a way to support HW Blink so the "activity" function needs to be called multiple time to increase the counter and toggle the LED.
Cyclic callback would be called twice per expected blink period, where you would toggle the GPIO (essentially making it 50% duty cycle, but could be more fine-grained if you want a different duty cycle).
Well status LED already have CONFIG_STATUS_LED_FREQ where you can set a value. I will just use this.
This actually only appears in the rST doc, nothing actually makes use of this right now, so it's not something we **need** to use.
What I meant is, if you only provide a frequency, a specific, hardcoded, pattern is expected. E.g. for 1KHz, you enable the LED for 0.5ms and disable it for 0.5ms (or 1ms and 1ms, depending on how you see LED_STATUS_FREQ working). Could be 0.2ms and 0.8ms but it would always be this. How do you differentiate between "something is happening on NAND" and "TFTP is being used" if you don't have the ability to change the duty cycle? Or are you expecting people to have multiple LED of different colors for that?
I just sent v2 to account for the Cyclic thing.
Honestly I wanted to keep the thing very simple with the config we already had. But yes it's pretty easy to add additional config to configure different blink times.
Also this have the extra feature that you can check if something gets stuck in the process. The lifecycle is:
- Turn on the ACTIVITY LED at the start of the thing
- Blink once in a while (for very little task this might not happen)
- Turn off the ACTIVITY LED at the end of the thing
Soo if something goes wrong the LED would never turn OFF but would stay solid ON.
Yes, that's something that wouldn't be covered by cyclic framework here. It all depends what you want to do, if it's an activity LED, then we need to hook ourselves deep into frameworks where stuff is actually happening. If it's just to specify which stage of the boot we reached, then cyclic would be good enough probably (register for stage 1, unregister stage1+register for stage2 for different frequency, etc...).
The cyclic framework can reduce the implementation to just START and STOP. We would lose the ability to know if there is an actual progress or not tho... So maybe that is bad but honestly a TFTP transfer can be tracked by the other machine and MTD write/erase won't magically stop and get stalled... (and even with that they will timeout and the status LED stop will be called anyway)
So a dumb blinking with the watchdog is O.K. This is really a simple thing to show that something is happening (use case of recovering the device without actually using serial)
Then it's not so much an activity LED anymore, rather a "i'm still alive and doing X thing right now, but maybe I'm stuck who knows", e.g. a little bit like a glorified heartbeat (I'm not saying it's bad, it's just a different use case :) ).
Well if we want a simple API that use Cyclic that it the path with an heartbeat. (but again IMHO we still need to account for context. for TFTP you can check progress from the device for any stall... For MTD write stall can't happen as any function have a timeout.)
But yes it's a different use case. v1 implementation actually have an activity that react to every "activity" (but for this you need to attach to the driver and you can't have simple start/stop API)
More than happy to rework this to a less intrusive implementation.
Maybe this can be generalized to some generic API like task_start(), task_processing() and task_end()? Might make more sense than having to add specific LED function to each function?
This also likely would introduce a hit in performance if we need to toggle the GPIO in the same thread that we do TFTP/storage medium reading/writing? I assume we could still adapt cyclic to make it spawn a one time event instead of looping (e.g. by unregistering itself at the end of its own callback?).
But the penality is that bad? Unless you have the crazy idea of an absurt low value for the freq, it would be triggered once every 200 iteration. If you are transfering MB of data you are probably on x86 or high end device where a GPIO bit set doesn't really affect anything.
The kernel on Aarch64 is usually a few MB, sometimes a few tens of MB, then you may have an initramfs which is also in the tens of MB.
People are trying to get U-Boot to boot into Linux ASAP, so maybe they'll be bothered by this. If they are, they can always disable the support for this LED status feature or improve it, so I wouldn't be too worried about it for now since it won't be on by default anyway?
Well it's for this exact reason I'm not touching read OPs. Yes perf it's a topic to account but nothing to worry about... the penality is really minimal and now that we use cyclic API is even non existant...
My current idea is that start() will register a cyclic and LED will blink with watchdog. Stop will deregister it.
Sounds good :)
Implemented in v2... It's pretty clean, I like it.
(AFAIK linux kernel have something similar used for all the trace framework so having something in uboot to trace these kind of operation might be interesting)
Indeed, that's what's being done with ledtrig_.* functions, they are however scheduled on a workqueue and called from the subsystem directly.
I'm a bit confused also as to why we control the LED blinking from the cmd/ ? E.g. for cmd/mtd.c I would assume that the changes made to the mtd subsystem should be enough to handle those? Similarly, since UBI is for use over NAND/MTD, shouldn't that already be handled by the MTD subsystem, and if not, why not in the UBI subsystem instead of the CMD_UBI? One of the issues is that you may not necessarily go through the cmd/ to do stuff with storage medium or network (e.g. directly from board files).
It's a mix of cmd and subsystem cause you need to turn the LED on. Use the activity function to increase the conunter and toggle the LED at actual activity (example a mtd write command, a single block received from TFTP)
If the cyclic thing works as I think MAYBE we can just add this to the cmd part without having to disturb all the subsystem. And that is a much cleaner approach.
Why isn't the subsystem turning the LED on when writing to the counter, why does it have to go through the CMD?
Userspace might call direct function of the subsystem and there is some problem to track single operations. (Example for MTD the _write OP can be called multiple time for each block or can be called one time for a big block... it's what I notice when I was implementing this. My original idea was to add everything in the single driver but it was hard to track when the single operation started and stopped)
If I'm not mistaken, I have my disk-activity LED in Linux working just fine without having to rely on a userspace tool to activate it for me? Maybe I missed something there though :)
Well the userspace tool probably call the generic subsystem that call the specific subsystem and so on. In uboot the cmd tools call directly the subsystem so IMHO it's not that bad to have the Status LED be started and stopped in cmd. (totally OK to change this if not liked tho)
For the net protocols, why not hook this to the net_[sg]et_*_handler for example so it's protocol-agnostic? No clue how difficult this would be, or if you'd rather have something like per-protocol activity?
Attaching to the net handler would provide a way too generic activity thing and would result in a dump NIC LED. We want to show activity when we actually want to signal stuff not when the device is executin ASM instructions ahahah
How does one define "when we actually want to signal stuff"? The further away you're from the subsystem, the more consumers we're going to have to patch to add this logic.
Logic would be everything that is slow enough and is not instant. I can already see USB activity to be not part of this as the trasnfer should be instant.
So yes I feel adding this way in way too generic subsystem might deviates from the idea of this. Even attaching to the write and erase function was a bit problematic and too generic.
Finally, maybe we also want to have a Kconfig symbol per "type" of activity to control what should be "monitored", and I would also suggest if we go this route to have a Kconfig symbol for the frequency per "type" of activity as well, so that one can know which activity is happening right now.
Yes that might be good but I have some fear we might explode with Kconfig. I think it's better to first have a solid idea on how this works and then we can think of the configuration part.
Oh, we've seen much worse with the VPL_/TPL_/SPL_// symbols :) Can come later though if someone has a usecase for it.
As said early these are very small thing, I want to first understand what is the right place to add the _start() and _stop().
See you in v2 ahahaha
participants (5)
-
Christian Marangi -
neil.armstrong＠linaro.org -
Peter Robinson -
Quentin Schulz -
Tom Rini