[PATCH v2 0/7] Add support for cyclic function execution infrastruture

[PATCH v2 0/7] Add support for cyclic function execution infrastruture

[Stefan Roese]

This patchset adds the basic infrastructure to periodically execute
code, e.g. all 100ms. Examples for such functions might be LED blinking
etc. The functions that are hooked into this cyclic list should be
small timewise as otherwise the execution of the other code that relies
on a high frequent polling (e.g. UART rx char ready check) might be
delayed too much. This patch also adds the Kconfig option
CONFIG_CYCLIC_MAX_CPU_TIME_US, which configures the max allowed time
for such a cyclic function. If it's execution time exceeds this time,
this cyclic function will get removed from the cyclic list.

How is this cyclic functionality executed?
This patchset integrates the main function responsible for calling all
registered cyclic functions cyclic_run() into the common WATCHDOG_RESET
macro. This guarantees that cyclic_run() is executed very often, which
is necessary for the cyclic functions to get scheduled and executed at
their configured periods.

This cyclic infrastructure will be used by a board specific function on
the NIC23 MIPS Octeon board, which needs to check periodically, if a
PCIe FLR has occurred.

Ideas how to continue:
One idea is to rename WATCHDOG_RESET to something like SCHEDULE and
move the watchdog_reset call into this cyclic infrastructure as well.
Or to perhaps move the shell UART RX ready polling to a cyclic
function.

It's also possible to extend the "cyclic" command, to support the
creation of periodically executed shell commands (for testing etc).

Here the Azure build, without any issues:
https://dev.azure.com/sr0718/u-boot/_build/results?buildId=219&view=results

Thanks,
Stefan

Aaron Williams (1):
  mips: octeon_nic23: Add PCIe FLR fixup via cyclic infrastructure

Stefan Roese (6):
  time: Import time_after64() and friends from Linux
  cyclic: Add basic support for cyclic function execution infrastruture
  cyclic: Integrate cyclic infrastructure into WATCHDOG_RESET
  cyclic: Integrate cyclic functionality at bootup in board_r/f
  cyclic: Add 'cyclic list' command
  sandbox: Add cyclic demo function

 MAINTAINERS                        |   7 +
 board/Marvell/octeon_nic23/board.c | 197 +++++++++++++++++++++++++++++
 board/sandbox/sandbox.c            |  15 +++
 cmd/Kconfig                        |   7 +
 cmd/Makefile                       |   1 +
 cmd/cyclic.c                       |  40 ++++++
 common/Kconfig                     |  20 +++
 common/Makefile                    |   1 +
 common/board_f.c                   |   2 +
 common/board_r.c                   |   2 +
 common/cyclic.c                    | 112 ++++++++++++++++
 configs/octeon_nic23_defconfig     |   3 +
 configs/sandbox_defconfig          |   3 +
 fs/cramfs/uncompress.c             |   2 +-
 include/cyclic.h                   |  97 ++++++++++++++
 include/time.h                     |  19 +++
 include/watchdog.h                 |  23 +++-
 17 files changed, 547 insertions(+), 4 deletions(-)
 create mode 100644 cmd/cyclic.c
 create mode 100644 common/cyclic.c
 create mode 100644 include/cyclic.h

-- 
2.37.1

[PATCH v2 1/7] time: Import time_after64() and friends from Linux

[Stefan Roese]

When using us times it makes sense to use 64bit variables for storage.
The currently implemented time_after() and friends functions only handle
32bit variables. This patch now includes the 64bit variants as well
from Linux. This will be used by the upcoming generic cyclic function
infrastructure.

These macros were copied from include/linux/jiffies.h of Linux 5.18.

Signed-off-by: Stefan Roese <sr@denx.de>
---
v2:
- No change

 include/time.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/include/time.h b/include/time.h
index 9deb2cf61cc4..3b2ba0912470 100644
--- a/include/time.h
+++ b/include/time.h
@@ -83,6 +83,25 @@ uint64_t usec_to_tick(unsigned long usec);
 	(time_after_eq(a,b) && \
 	 time_before(a,c))
 
+/* Same as above, but does so with platform independent 64bit types.
+ * These must be used when utilizing jiffies_64 (i.e. return value of
+ * get_jiffies_64() */
+#define time_after64(a,b)	\
+	(typecheck(__u64, a) &&	\
+	 typecheck(__u64, b) && \
+	 ((__s64)((b) - (a)) < 0))
+#define time_before64(a,b)	time_after64(b,a)
+
+#define time_after_eq64(a,b)	\
+	(typecheck(__u64, a) && \
+	 typecheck(__u64, b) && \
+	 ((__s64)((a) - (b)) >= 0))
+#define time_before_eq64(a,b)	time_after_eq64(b,a)
+
+#define time_in_range64(a, b, c) \
+	(time_after_eq64(a, b) && \
+	 time_before_eq64(a, c))
+
 /**
  * usec2ticks() - Convert microseconds to internal ticks
  *
-- 
2.37.1

[PATCH v2 2/7] cyclic: Add basic support for cyclic function execution infrastruture

[Stefan Roese]

Add the basic infrastructure to periodically execute code, e.g. all
100ms. Examples for such functions might be LED blinking etc. The
functions that are hooked into this cyclic list should be small timewise
as otherwise the execution of the other code that relies on a high
frequent polling (e.g. UART rx char ready check) might be delayed too
much. This patch also adds the Kconfig option
CONFIG_CYCLIC_MAX_CPU_TIME_US, which configures the max allowed time
for such a cyclic function. If it's execution time exceeds this time,
this cyclic function will get removed from the cyclic list.

How is this cyclic functionality executed?
The following patch integrates the main function responsible for
calling all registered cyclic functions cyclic_run() into the
common WATCHDOG_RESET macro. This guarantees that cyclic_run() is
executed very often, which is necessary for the cyclic functions to
get scheduled and executed at their configured periods.

This cyclic infrastructure will be used by a board specific function on
the NIC23 MIPS Octeon board, which needs to check periodically, if a
PCIe FLR has occurred.

Signed-off-by: Stefan Roese <sr@denx.de>
---
v2:
- Also add cyclic_register() and cyclic_unregister() as empty functions
  when CONFIG_CYCLIC is not defined
- Misc minor changes

 MAINTAINERS      |   6 +++
 common/Kconfig   |  20 +++++++++
 common/Makefile  |   1 +
 common/cyclic.c  | 112 +++++++++++++++++++++++++++++++++++++++++++++++
 include/cyclic.h |  97 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 236 insertions(+)
 create mode 100644 common/cyclic.c
 create mode 100644 include/cyclic.h

diff --git a/MAINTAINERS b/MAINTAINERS
index f371d864f29b..4490b9d3737a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -788,6 +788,12 @@ T:	git https://source.denx.de/u-boot/custodians/u-boot-coldfire.git
 F:	arch/m68k/
 F:	doc/arch/m68k.rst
 
+CYCLIC
+M:	Stefan Roese <sr@denx.de>
+S:	Maintained
+F:	common/cyclic.c
+F:	include/cyclic.h
+
 DFU
 M:	Lukasz Majewski <lukma@denx.de>
 S:	Maintained
diff --git a/common/Kconfig b/common/Kconfig
index e7914ca750a3..a6a94ab8dfbf 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -545,6 +545,26 @@ config DISPLAY_BOARDINFO_LATE
 
 menu "Start-up hooks"
 
+config CYCLIC
+	bool "General-purpose cyclic execution mechanism"
+	help
+	  This enables a general-purpose cyclic execution infrastructure,
+	  to allow "small" (run-time wise) functions to be executed at
+	  a specified frequency. Things like LED blinking or watchdog
+	  triggering are examples for such tasks.
+
+if CYCLIC
+
+config CYCLIC_MAX_CPU_TIME_US
+	int "Sets the max allowed time for a cyclic function in us"
+	default 1000
+	help
+	  The max allowed time for a cyclic function in us. If a functions
+	  takes longer than this duration this function will get unregistered
+	  automatically.
+
+endif # CYCLIC
+
 config EVENT
 	bool "General-purpose event-handling mechanism"
 	default y if SANDBOX
diff --git a/common/Makefile b/common/Makefile
index 2ed8672c3ac1..1d56c9f2895a 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -84,6 +84,7 @@ obj-y += malloc_simple.o
 endif
 endif
 
+obj-$(CONFIG_CYCLIC) += cyclic.o
 obj-$(CONFIG_$(SPL_TPL_)EVENT) += event.o
 
 obj-$(CONFIG_$(SPL_TPL_)HASH) += hash.o
diff --git a/common/cyclic.c b/common/cyclic.c
new file mode 100644
index 000000000000..6e7ebbe7b218
--- /dev/null
+++ b/common/cyclic.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * A general-purpose cyclic execution infrastructure, to allow "small"
+ * (run-time wise) functions to be executed at a specified frequency.
+ * Things like LED blinking or watchdog triggering are examples for such
+ * tasks.
+ *
+ * Copyright (C) 2022 Stefan Roese <sr@denx.de>
+ */
+
+#include <cyclic.h>
+#include <log.h>
+#include <linker_lists.h>
+#include <malloc.h>
+#include <time.h>
+#include <linux/errno.h>
+#include <linux/list.h>
+
+struct list_head cyclic_list;
+static bool cyclic_ready;
+static bool cyclic_running;
+
+struct cyclic_struct *cyclic_register(cyclic_func_t func, uint64_t delay_us,
+				      const char *name, void *ctx)
+{
+	struct cyclic_struct *cyclic;
+
+	if (!cyclic_ready) {
+		pr_debug("Cyclic IF not ready yet\n");
+		return NULL;
+	}
+
+	cyclic = calloc(1, sizeof(struct cyclic_struct));
+	if (!cyclic) {
+		pr_debug("Memory allocation error\n");
+		return NULL;
+	}
+
+	/* Store values in struct */
+	cyclic->func = func;
+	cyclic->ctx = ctx;
+	cyclic->name = strdup(name);
+	cyclic->delay_us = delay_us;
+	cyclic->start_time_us = timer_get_us();
+	list_add_tail(&cyclic->list, &cyclic_list);
+
+	return cyclic;
+}
+
+int cyclic_unregister(struct cyclic_struct *cyclic)
+{
+	list_del(&cyclic->list);
+	free(cyclic);
+
+	return 0;
+}
+
+void cyclic_run(void)
+{
+	struct cyclic_struct *cyclic, *tmp;
+	uint64_t now, cpu_time;
+
+	/* Prevent recursion */
+	if (cyclic_running)
+		return;
+
+	cyclic_running = true;
+	list_for_each_entry_safe(cyclic, tmp, &cyclic_list, list) {
+		/*
+		 * Check if this cyclic function needs to get called, e.g.
+		 * do not call the cyclic func too often
+		 */
+		now = timer_get_us();
+		if (time_after_eq64(now, cyclic->next_call)) {
+			/* Call cyclic function and account it's cpu-time */
+			cyclic->next_call = now + cyclic->delay_us;
+			cyclic->func(cyclic->ctx);
+			cyclic->run_cnt++;
+			cpu_time = timer_get_us() - now;
+			cyclic->cpu_time_us += cpu_time;
+
+			/* Check if cpu-time exceeds max allowed time */
+			if (cpu_time > CONFIG_CYCLIC_MAX_CPU_TIME_US) {
+				pr_err("cyclic function %s took too long: %lldus vs %dus max, disabling\n",
+				       cyclic->name, cpu_time,
+				       CONFIG_CYCLIC_MAX_CPU_TIME_US);
+
+				/* Unregister this cyclic function */
+				cyclic_unregister(cyclic);
+			}
+		}
+	}
+	cyclic_running = false;
+}
+
+int cyclic_uninit(void)
+{
+	struct cyclic_struct *cyclic, *tmp;
+
+	list_for_each_entry_safe(cyclic, tmp, &cyclic_list, list)
+		cyclic_unregister(cyclic);
+
+	return 0;
+}
+
+int cyclic_init(void)
+{
+	INIT_LIST_HEAD(&cyclic_list);
+	cyclic_ready = true;
+
+	return 0;
+}
diff --git a/include/cyclic.h b/include/cyclic.h
new file mode 100644
index 000000000000..6b48113e056a
--- /dev/null
+++ b/include/cyclic.h
@@ -0,0 +1,97 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * A general-purpose cyclic execution infrastructure, to allow "small"
+ * (run-time wise) functions to be executed at a specified frequency.
+ * Things like LED blinking or watchdog triggering are examples for such
+ * tasks.
+ *
+ * Copyright (C) 2022 Stefan Roese <sr@denx.de>
+ */
+
+#ifndef __cyclic_h
+#define __cyclic_h
+
+#include <linux/list.h>
+#include <asm/types.h>
+
+struct cyclic_struct {
+	void (*func)(void *ctx);
+	void *ctx;
+	char *name;
+	uint64_t delay_us;
+	uint64_t start_time_us;
+	uint64_t cpu_time_us;
+	uint64_t run_cnt;
+	uint64_t next_call;
+	struct list_head list;
+};
+
+/** Function type for cyclic functions */
+typedef void (*cyclic_func_t)(void *ctx);
+
+#if defined(CONFIG_CYCLIC)
+/**
+ * cyclic_register - Register a new cyclic function
+ *
+ * @func: Function to call periodically
+ * @delay_us: Delay is us after which this function shall get executed
+ * @name: Cyclic function name/id
+ * @ctx: Context to pass to the function
+ * @return: pointer to cyclic_struct if OK, NULL on error
+ */
+struct cyclic_struct *cyclic_register(cyclic_func_t func, uint64_t delay_us,
+				      const char *name, void *ctx);
+
+/**
+ * cyclic_unregister - Unregister a cyclic function
+ *
+ * @cyclic: Pointer to cyclic_struct of the function that shall be removed
+ * @return: 0 if OK, -ve on error
+ */
+int cyclic_unregister(struct cyclic_struct *cyclic);
+
+/**
+ * cyclic_init() - Set up cyclic functions
+ *
+ * Init a list of cyclic functions, so that these can be added as needed
+ */
+int cyclic_init(void);
+
+/**
+ * cyclic_uninit() - Clean up cyclic functions
+ *
+ * This removes all cyclic functions
+ */
+int cyclic_uninit(void);
+
+void cyclic_run(void);
+#else
+static inline struct cyclic_struct *cyclic_register(cyclic_func_t func,
+						    uint64_t delay_us,
+						    const char *name,
+						    void *ctx)
+{
+	return NULL;
+}
+
+static inline int cyclic_unregister(struct cyclic_struct *cyclic)
+{
+	return 0;
+}
+
+static inline void cyclic_run(void)
+{
+}
+
+static inline int cyclic_init(void)
+{
+	return 0;
+}
+
+static inline int cyclic_uninit(void)
+{
+	return 0;
+}
+#endif
+
+#endif
-- 
2.37.1

[PATCH v2 3/7] cyclic: Integrate cyclic infrastructure into WATCHDOG_RESET

[Stefan Roese]

This patch integrates the main function responsible for calling all
registered cyclic functions cyclic_run() into the common WATCHDOG_RESET
macro. This guarantees that cyclic_run() is executed very often, which
is necessary for the cyclic functions to get scheduled and executed at
their configured periods.

If CONFIG_WATCHDOG is not enabled, only cyclic_run() without calling
watchdog_reset(). This guarantees that the cyclic functionality does not
rely on CONFIG_WATCHDOG being enabled.

Signed-off-by: Stefan Roese <sr@denx.de>
---
v2:
- No change

 fs/cramfs/uncompress.c |  2 +-
 include/watchdog.h     | 23 ++++++++++++++++++++---
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/fs/cramfs/uncompress.c b/fs/cramfs/uncompress.c
index f431cc46c1f7..38e10e2e4422 100644
--- a/fs/cramfs/uncompress.c
+++ b/fs/cramfs/uncompress.c
@@ -62,7 +62,7 @@ int cramfs_uncompress_init (void)
 	stream.avail_in = 0;
 
 #if defined(CONFIG_HW_WATCHDOG) || defined(CONFIG_WATCHDOG)
-	stream.outcb = (cb_func) WATCHDOG_RESET;
+	stream.outcb = (cb_func)watchdog_reset_func;
 #else
 	stream.outcb = Z_NULL;
 #endif /* CONFIG_HW_WATCHDOG */
diff --git a/include/watchdog.h b/include/watchdog.h
index 813cc8f2a5d3..0a9777edcbad 100644
--- a/include/watchdog.h
+++ b/include/watchdog.h
@@ -11,6 +11,8 @@
 #define _WATCHDOG_H_
 
 #if !defined(__ASSEMBLY__)
+#include <cyclic.h>
+
 /*
  * Reset the watchdog timer, always returns 0
  *
@@ -60,11 +62,16 @@ int init_func_watchdog_reset(void);
 			/* Don't require the watchdog to be enabled in SPL */
 			#if defined(CONFIG_SPL_BUILD) &&		\
 				!defined(CONFIG_SPL_WATCHDOG)
-				#define WATCHDOG_RESET() {}
+				#define WATCHDOG_RESET() { \
+					cyclic_run(); \
+				}
 			#else
 				extern void watchdog_reset(void);
 
-				#define WATCHDOG_RESET watchdog_reset
+				#define WATCHDOG_RESET() { \
+					watchdog_reset(); \
+					cyclic_run(); \
+				}
 			#endif
 		#endif
 	#else
@@ -74,11 +81,21 @@ int init_func_watchdog_reset(void);
 		#if defined(__ASSEMBLY__)
 			#define WATCHDOG_RESET /*XXX DO_NOT_DEL_THIS_COMMENT*/
 		#else
-			#define WATCHDOG_RESET() {}
+			#define WATCHDOG_RESET() { \
+				cyclic_run(); \
+			}
 		#endif /* __ASSEMBLY__ */
 	#endif /* CONFIG_WATCHDOG && !__ASSEMBLY__ */
 #endif /* CONFIG_HW_WATCHDOG */
 
+#if !defined(__ASSEMBLY__)
+/* Currently only needed for fs/cramfs/uncompress.c */
+static inline void watchdog_reset_func(void)
+{
+	WATCHDOG_RESET();
+}
+#endif
+
 /*
  * Prototypes from $(CPU)/cpu.c.
  */
-- 
2.37.1

[PATCH v2 4/7] cyclic: Integrate cyclic functionality at bootup in board_r/f

[Stefan Roese]

This patch adds a call to cyclic_init() to board_f/r.c, enabling the
common cyclic infrastructure. After this it's possible to add cyclic
functions via cyclic_register().

Signed-off-by: Stefan Roese <sr@denx.de>
---
v2:
- No change

 common/board_f.c | 2 ++
 common/board_r.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/common/board_f.c b/common/board_f.c
index 5c86faeb217b..694fb89b0a3e 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -16,6 +16,7 @@
 #include <console.h>
 #include <cpu.h>
 #include <cpu_func.h>
+#include <cyclic.h>
 #include <dm.h>
 #include <env.h>
 #include <env_internal.h>
@@ -827,6 +828,7 @@ static const init_fnc_t init_sequence_f[] = {
 	initf_malloc,
 	log_init,
 	initf_bootstage,	/* uses its own timer, so does not need DM */
+	cyclic_init,
 	event_init,
 #ifdef CONFIG_BLOBLIST
 	bloblist_init,
diff --git a/common/board_r.c b/common/board_r.c
index ed29069d2de6..5c260d093678 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -13,6 +13,7 @@
 #include <api.h>
 #include <bootstage.h>
 #include <cpu_func.h>
+#include <cyclic.h>
 #include <exports.h>
 #include <flash.h>
 #include <hang.h>
@@ -588,6 +589,7 @@ static int run_main_loop(void)
 static init_fnc_t init_sequence_r[] = {
 	initr_trace,
 	initr_reloc,
+	cyclic_init,
 	event_init,
 	/* TODO: could x86/PPC have this also perhaps? */
 #if defined(CONFIG_ARM) || defined(CONFIG_RISCV)
-- 
2.37.1

[PATCH v2 5/7] cyclic: Add 'cyclic list' command

[Stefan Roese]

This patch adds the cyclic command, which currently only supports the
'list' subcommand, to list all currently registered cyclic functions.
Here an example:

=> cyclic list
function: cyclic_demo, cpu-time: 7010 us, frequency: 99.80 times/s
function: cyclic_demo2, cpu-time: 1 us, frequency: 1.13 times/s

As you can see, the cpu-time is accounted, so that cyclic functions
that take too long might be discovered. Additionally the frequency is
logged.

Signed-off-by: Stefan Roese <sr@denx.de>
---
v2:
- Add depends on CYCLIC in Kconfig

 MAINTAINERS  |  1 +
 cmd/Kconfig  |  7 +++++++
 cmd/Makefile |  1 +
 cmd/cyclic.c | 40 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 49 insertions(+)
 create mode 100644 cmd/cyclic.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4490b9d3737a..ce611c861308 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -791,6 +791,7 @@ F:	doc/arch/m68k.rst
 CYCLIC
 M:	Stefan Roese <sr@denx.de>
 S:	Maintained
+F:	cmd/cyclic.c
 F:	common/cyclic.c
 F:	include/cyclic.h
 
diff --git a/cmd/Kconfig b/cmd/Kconfig
index a8260aa170d0..641c053003f0 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -2494,6 +2494,13 @@ config CMD_CBSYSINFO
 	  memory by coreboot before jumping to U-Boot. It can be useful for
 	  debugging the beaaviour of coreboot or U-Boot.
 
+config CMD_CYCLIC
+	bool "cyclic - Show information about cyclic functions"
+	depends on CYCLIC
+	help
+	  This enables the 'cyclic' command which provides information about
+	  cyclic excution functions.
+
 config CMD_DIAG
 	bool "diag - Board diagnostics"
 	help
diff --git a/cmd/Makefile b/cmd/Makefile
index 5e43a1e022e8..c5a4fc5a5cfc 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_CMD_DIAG) += diag.o
 endif
 obj-$(CONFIG_CMD_ADTIMG) += adtimg.o
 obj-$(CONFIG_CMD_ABOOTIMG) += abootimg.o
+obj-$(CONFIG_CMD_CYCLIC) += cyclic.o
 obj-$(CONFIG_CMD_EVENT) += event.o
 obj-$(CONFIG_CMD_EXTENSION) += extension_board.o
 obj-$(CONFIG_CMD_ECHO) += echo.o
diff --git a/cmd/cyclic.c b/cmd/cyclic.c
new file mode 100644
index 000000000000..1714db18e480
--- /dev/null
+++ b/cmd/cyclic.c
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * A general-purpose cyclic execution infrastructure, to allow "small"
+ * (run-time wise) functions to be executed at a specified frequency.
+ * Things like LED blinking or watchdog triggering are examples for such
+ * tasks.
+ *
+ * Copyright (C) 2022 Stefan Roese <sr@denx.de>
+ */
+
+#include <common.h>
+#include <command.h>
+#include <cyclic.h>
+
+extern struct list_head cyclic_list;
+
+static int do_cyclic_list(struct cmd_tbl *cmdtp, int flag, int argc,
+			  char *const argv[])
+{
+	struct cyclic_struct *cyclic, *tmp;
+	uint64_t cnt, freq;
+
+	list_for_each_entry_safe(cyclic, tmp, &cyclic_list, list) {
+		cnt = cyclic->run_cnt * 1000000ULL * 100ULL;
+		freq = cnt / (timer_get_us() - cyclic->start_time_us);
+		printf("function: %s, cpu-time: %lld us, frequency: %lld.%02lld times/s\n",
+		       cyclic->name, cyclic->cpu_time_us,
+		       freq / 100, freq % 100);
+	}
+
+	return 0;
+}
+
+#ifdef CONFIG_SYS_LONGHELP
+static char cyclic_help_text[] =
+	"cyclic list   - list cyclic functions";
+#endif
+
+U_BOOT_CMD_WITH_SUBCMDS(cyclic, "Cyclic", cyclic_help_text,
+	U_BOOT_SUBCMD_MKENT(list, 1, 1, do_cyclic_list));
-- 
2.37.1

[PATCH v2 6/7] sandbox: Add cyclic demo function

[Stefan Roese]

This patch enables the cyclic infrastructure on sandbox and also adds
one simple example/demo functions using this cyclic functionality.

Signed-off-by: Stefan Roese <sr@denx.de>
---
v2:
- Extend CONFIG_CYCLIC_MAX_CPU_TIME_US to 10000ms as running this
  in CI might take a bit longer

 board/sandbox/sandbox.c   | 15 +++++++++++++++
 configs/sandbox_defconfig |  3 +++
 2 files changed, 18 insertions(+)

diff --git a/board/sandbox/sandbox.c b/board/sandbox/sandbox.c
index ca9a2ca5b17c..8dda4b5f4fc9 100644
--- a/board/sandbox/sandbox.c
+++ b/board/sandbox/sandbox.c
@@ -7,6 +7,7 @@
 #include <addr_map.h>
 #include <cpu_func.h>
 #include <cros_ec.h>
+#include <cyclic.h>
 #include <dm.h>
 #include <efi.h>
 #include <efi_loader.h>
@@ -17,6 +18,7 @@
 #include <asm/global_data.h>
 #include <asm/test.h>
 #include <asm/u-boot-sandbox.h>
+#include <linux/delay.h>
 #include <linux/kernel.h>
 #include <malloc.h>
 
@@ -106,8 +108,21 @@ int dram_init(void)
 	return 0;
 }
 
+static void cyclic_demo(void *ctx)
+{
+	/* Just a small dummy delay here */
+	udelay(10);
+}
+
 int board_init(void)
 {
+	struct cyclic_struct *cyclic;
+
+	/* Register demo cyclic function */
+	cyclic = cyclic_register(cyclic_demo, 10 * 1000, "cyclic_demo", NULL);
+	if (!cyclic)
+		printf("Registering of cyclic_demo failed\n");
+
 	return 0;
 }
 
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index eba7bcbb483b..8b6c003760f2 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -34,6 +34,8 @@ CONFIG_LOG=y
 CONFIG_LOG_MAX_LEVEL=9
 CONFIG_LOG_DEFAULT_LEVEL=6
 CONFIG_DISPLAY_BOARDINFO_LATE=y
+CONFIG_CYCLIC=y
+CONFIG_CYCLIC_MAX_CPU_TIME_US=10000
 CONFIG_STACKPROTECTOR=y
 CONFIG_ANDROID_AB=y
 CONFIG_CMD_CPU=y
@@ -114,6 +116,7 @@ CONFIG_CMD_EROFS=y
 CONFIG_CMD_EXT4_WRITE=y
 CONFIG_CMD_SQUASHFS=y
 CONFIG_CMD_MTDPARTS=y
+CONFIG_CMD_CYCLIC=y
 CONFIG_CMD_STACKPROTECTOR_TEST=y
 CONFIG_MAC_PARTITION=y
 CONFIG_AMIGA_PARTITION=y
-- 
2.37.1

[PATCH v2 7/7] mips: octeon_nic23: Add PCIe FLR fixup via cyclic infrastructure

[Stefan Roese]

From: Aaron Williams <awilliams@marvell.com>

This patch adds a fixup function related to a PCIe FLR (Function Level
Reset) problem on the NIC23 PCIe board. This function is imported from
the Marvell Octeon 2013 U-Boot version as a (nearly) verbatim copy. It
uses the newly introduced cyclic infrastructure, so that this function
gets called every 100us, which is needed to detect this FLR issue.

Signed-off-by: Aaron Williams <awilliams@marvell.com>
Signed-off-by: Stefan Roese <sr@denx.de>
---
v2:
- No change

 board/Marvell/octeon_nic23/board.c | 197 +++++++++++++++++++++++++++++
 configs/octeon_nic23_defconfig     |   3 +
 2 files changed, 200 insertions(+)

diff --git a/board/Marvell/octeon_nic23/board.c b/board/Marvell/octeon_nic23/board.c
index 3e2c54444397..446fa5a659a8 100644
--- a/board/Marvell/octeon_nic23/board.c
+++ b/board/Marvell/octeon_nic23/board.c
@@ -3,8 +3,10 @@
  * Copyright (C) 2021-2022 Stefan Roese <sr@denx.de>
  */
 
+#include <cyclic.h>
 #include <dm.h>
 #include <ram.h>
+#include <time.h>
 #include <asm/gpio.h>
 
 #include <mach/octeon_ddr.h>
@@ -15,11 +17,90 @@
 #include <mach/cvmx-helper-cfg.h>
 #include <mach/cvmx-helper-util.h>
 #include <mach/cvmx-bgxx-defs.h>
+#include <mach/cvmx-dtx-defs.h>
 
 #include "board_ddr.h"
 
+/**
+ * cvmx_spem#_cfg_rd
+ *
+ * This register allows read access to the configuration in the PCIe core.
+ *
+ */
+union cvmx_spemx_cfg_rd {
+	u64 u64;
+	struct cvmx_spemx_cfg_rd_s {
+		u64 data                         : 32;
+		u64 addr                         : 32;
+	} s;
+	struct cvmx_spemx_cfg_rd_s            cn73xx;
+};
+
+/**
+ * cvmx_spem#_cfg_wr
+ *
+ * This register allows write access to the configuration in the PCIe core.
+ *
+ */
+union cvmx_spemx_cfg_wr {
+	u64 u64;
+	struct cvmx_spemx_cfg_wr_s {
+		u64 data                         : 32;
+		u64 addr                         : 32;
+	} s;
+	struct cvmx_spemx_cfg_wr_s            cn73xx;
+};
+
+/**
+ * cvmx_spem#_flr_pf_stopreq
+ *
+ * PF function level reset stop outbound requests register.
+ * Hardware automatically sets the STOPREQ bit for the PF when it enters a
+ * function level reset (FLR).  Software is responsible for clearing the STOPREQ
+ * bit but must not do so prior to hardware taking down the FLR, which could be
+ * as long as 100ms.  It may be appropriate for software to wait longer before clearing
+ * STOPREQ, software may need to drain deep DPI queues for example.
+ * Whenever SPEM receives a PF or child VF request mastered by CNXXXX over S2M (i.e. P or NP),
+ * when STOPREQ is set for the function, SPEM will discard the outgoing request
+ * before sending it to the PCIe core.  If a NP, SPEM will schedule an immediate
+ * SWI_RSP_ERROR completion for the request - no timeout is required.
+ * In both cases, SPEM()_DBG_PF()_INFO[P()_BMD_E] will be set and a error
+ * interrupt is generated.
+ *
+ * STOPREQ mimics the behavior of PCIEEP()_CFG001[ME] for outbound requests that will
+ * master the PCIe bus (P and NP).
+ *
+ * STOPREQ will have no effect on completions returned by CNXXXX over the S2M,
+ * nor on M2S traffic.
+ *
+ * When a PF()_STOPREQ is set, none of the associated
+ * PEM()_FLR_PF()_VF_STOPREQ[VF_STOPREQ] will be set.
+ *
+ * STOPREQ is reset when the MAC is reset, and is not reset after a chip soft reset.
+ */
+union cvmx_spemx_flr_pf_stopreq {
+	u64 u64;
+	struct cvmx_spemx_flr_pf_stopreq_s {
+		u64 reserved_3_63                : 61;
+		u64 pf2_stopreq                  : 1;
+		u64 pf1_stopreq                  : 1;
+		u64 pf0_stopreq                  : 1;
+	} s;
+	struct cvmx_spemx_flr_pf_stopreq_s    cn73xx;
+};
+
+#define CVMX_SPEMX_CFG_WR(offset)		0x00011800C0000028ull
+#define CVMX_SPEMX_CFG_RD(offset)		0x00011800C0000030ull
+#define CVMX_SPEMX_FLR_PF_STOPREQ(offset)	0x00011800C0000218ull
+
+#define DTX_SELECT_LTSSM		0x0
+#define DTX_SELECT_LTSSM_ENA		0x3ff
+#define LTSSM_L0			0x11
+
 #define NIC23_DEF_DRAM_FREQ		800
 
+static u32 pci_cfgspace_reg0[2] = { 0, 0 };
+
 static u8 octeon_nic23_cfg0_spd_values[512] = {
 	OCTEON_NIC23_CFG0_SPD_VALUES
 };
@@ -145,8 +226,118 @@ void board_configure_qlms(void)
 	      cvmx_qlm_measure_clock(4), cvmx_qlm_measure_clock(5));
 }
 
+/**
+ * If there is a PF FLR then the PCI EEPROM is not re-read.  In this case
+ * we need to re-program the vendor and device ID immediately after hardware
+ * completes FLR.
+ *
+ * PCI spec requires FLR to be completed within 100ms.  The user who triggered
+ * FLR expects hardware to finish FLR within 100ms, otherwise the user will
+ * end up reading DEVICE_ID incorrectly from the reset value.
+ * CN23XX exits FLR at any point between 66 and 99ms, so U-Boot has to wait
+ * 99ms to let hardware finish its part, then finish reprogramming the
+ * correct device ID before the end of 100ms.
+ *
+ * Note: this solution only works properly when there is no other activity
+ * within U-Boot for 100ms from the time FLR is triggered.
+ *
+ * This function gets called every 100usec.  If FLR happens during any
+ * other activity like bootloader/image update then it is possible that
+ * this function does not get called for more than the FLR period which will
+ * cause the PF device ID restore to happen after whoever initiated the FLR to
+ * read the incorrect device ID 0x9700 (reset value) instead of 0x9702
+ * (restored value).
+ */
+static void octeon_board_restore_pf(void *ctx)
+{
+	union cvmx_spemx_flr_pf_stopreq stopreq;
+	static bool start_initialized[2] = {false, false};
+	bool pf0_flag, pf1_flag;
+	u64 ltssm_bits;
+	const u64 pf_flr_wait_usecs = 99700;
+	u64 elapsed_usecs;
+	union cvmx_spemx_cfg_wr cfg_wr;
+	union cvmx_spemx_cfg_rd cfg_rd;
+	static u64 start_us[2];
+	int pf_num;
+
+	csr_wr(CVMX_DTX_SPEM_SELX(0), DTX_SELECT_LTSSM);
+	csr_rd(CVMX_DTX_SPEM_SELX(0));
+	csr_wr(CVMX_DTX_SPEM_ENAX(0), DTX_SELECT_LTSSM_ENA);
+	csr_rd(CVMX_DTX_SPEM_ENAX(0));
+	ltssm_bits = csr_rd(CVMX_DTX_SPEM_DATX(0));
+	if (((ltssm_bits >> 3) & 0x3f) != LTSSM_L0)
+		return;
+
+	stopreq.u64 = csr_rd(CVMX_SPEMX_FLR_PF_STOPREQ(0));
+	pf0_flag = stopreq.s.pf0_stopreq;
+	pf1_flag = stopreq.s.pf1_stopreq;
+	/* See if PF interrupt happened */
+	if (!(pf0_flag || pf1_flag))
+		return;
+
+	if (pf0_flag && !start_initialized[0]) {
+		start_initialized[0] = true;
+		start_us[0] = get_timer_us(0);
+	}
+
+	/* Store programmed PCIe DevID SPEM0 PF0 */
+	if (pf0_flag && !pci_cfgspace_reg0[0]) {
+		cfg_rd.s.addr = (0 << 24) | 0x0;
+		csr_wr(CVMX_SPEMX_CFG_RD(0), cfg_rd.u64);
+		cfg_rd.u64 = csr_rd(CVMX_SPEMX_CFG_RD(0));
+		pci_cfgspace_reg0[0] = cfg_rd.s.data;
+	}
+
+	if (pf1_flag && !start_initialized[1]) {
+		start_initialized[1] = true;
+		start_us[1] = get_timer_us(0);
+	}
+
+	/* Store programmed PCIe DevID SPEM0 PF1 */
+	if (pf1_flag && !pci_cfgspace_reg0[1]) {
+		cfg_rd.s.addr = (1 << 24) | 0x0;
+		csr_wr(CVMX_SPEMX_CFG_RD(0), cfg_rd.u64);
+		cfg_rd.u64 = csr_rd(CVMX_SPEMX_CFG_RD(0));
+		pci_cfgspace_reg0[1] = cfg_rd.s.data;
+	}
+
+	/* For PF, rewrite pci config space reg 0 */
+	for (pf_num = 0; pf_num < 2; pf_num++) {
+		if (!start_initialized[pf_num])
+			continue;
+
+		elapsed_usecs = get_timer_us(0) - start_us[pf_num];
+
+		if (elapsed_usecs > pf_flr_wait_usecs) {
+			/* Here, our measured FLR duration has passed;
+			 * check if device ID has been reset,
+			 * which indicates FLR completion (per MA team).
+			 */
+			cfg_rd.s.addr = (pf_num << 24) | 0x0;
+			csr_wr(CVMX_SPEMX_CFG_RD(0), cfg_rd.u64);
+			cfg_rd.u64 = csr_rd(CVMX_SPEMX_CFG_RD(0));
+			/* if DevID has NOT been reset, FLR is not yet
+			 * complete
+			 */
+			if (cfg_rd.s.data != pci_cfgspace_reg0[pf_num]) {
+				stopreq.s.pf0_stopreq = (pf_num == 0) ? 1 : 0;
+				stopreq.s.pf1_stopreq = (pf_num == 1) ? 1 : 0;
+				csr_wr(CVMX_SPEMX_FLR_PF_STOPREQ(0), stopreq.u64);
+
+				cfg_wr.u64 = 0;
+				cfg_wr.s.addr = (pf_num << 24) | 0;
+				cfg_wr.s.data = pci_cfgspace_reg0[pf_num];
+				csr_wr(CVMX_SPEMX_CFG_WR(0), cfg_wr.u64);
+				start_initialized[pf_num] = false;
+			}
+		}
+	}
+}
+
 int board_late_init(void)
 {
+	struct cyclic_struct *cyclic;
 	struct gpio_desc gpio = {};
 	ofnode node;
 
@@ -164,6 +355,12 @@ int board_late_init(void)
 
 	board_configure_qlms();
 
+	/* Register cyclic function for PCIe FLR fixup */
+	cyclic = cyclic_register(octeon_board_restore_pf, 100,
+				 "pcie_flr_fix", NULL);
+	if (!cyclic)
+		printf("Registering of cyclic function failed\n");
+
 	return 0;
 }
 
diff --git a/configs/octeon_nic23_defconfig b/configs/octeon_nic23_defconfig
index 95e98c1161db..098831e0b1ed 100644
--- a/configs/octeon_nic23_defconfig
+++ b/configs/octeon_nic23_defconfig
@@ -20,6 +20,8 @@ CONFIG_AHCI=y
 CONFIG_OF_BOARD_FIXUP=y
 CONFIG_SYS_CONSOLE_ENV_OVERWRITE=y
 # CONFIG_SYS_DEVICE_NULLDEV is not set
+CONFIG_CYCLIC=y
+CONFIG_CYCLIC_MAX_CPU_TIME_US=5000
 CONFIG_ARCH_MISC_INIT=y
 CONFIG_BOARD_EARLY_INIT_F=y
 CONFIG_BOARD_LATE_INIT=y
@@ -41,6 +43,7 @@ CONFIG_CMD_TIME=y
 CONFIG_CMD_EXT4=y
 CONFIG_CMD_FAT=y
 CONFIG_CMD_FS_GENERIC=y
+CONFIG_CMD_CYCLIC=y
 CONFIG_EFI_PARTITION=y
 CONFIG_ENV_IS_IN_SPI_FLASH=y
 CONFIG_TFTP_TSIZE=y
-- 
2.37.1

Re: [PATCH v2 0/7] Add support for cyclic function execution infrastruture

[Simon Glass]

Hi Stefan,

On Thu, 28 Jul 2022 at 01:09, Stefan Roese <sr@denx.de> wrote:
>
> This patchset adds the basic infrastructure to periodically execute
> code, e.g. all 100ms. Examples for such functions might be LED blinking
> etc. The functions that are hooked into this cyclic list should be
> small timewise as otherwise the execution of the other code that relies
> on a high frequent polling (e.g. UART rx char ready check) might be
> delayed too much. This patch also adds the Kconfig option
> CONFIG_CYCLIC_MAX_CPU_TIME_US, which configures the max allowed time
> for such a cyclic function. If it's execution time exceeds this time,
> this cyclic function will get removed from the cyclic list.
>
> How is this cyclic functionality executed?
> This patchset integrates the main function responsible for calling all
> registered cyclic functions cyclic_run() into the common WATCHDOG_RESET
> macro. This guarantees that cyclic_run() is executed very often, which
> is necessary for the cyclic functions to get scheduled and executed at
> their configured periods.
>
> This cyclic infrastructure will be used by a board specific function on
> the NIC23 MIPS Octeon board, which needs to check periodically, if a
> PCIe FLR has occurred.
>
> Ideas how to continue:
> One idea is to rename WATCHDOG_RESET to something like SCHEDULE and
> move the watchdog_reset call into this cyclic infrastructure as well.
> Or to perhaps move the shell UART RX ready polling to a cyclic
> function.
>
> It's also possible to extend the "cyclic" command, to support the
> creation of periodically executed shell commands (for testing etc).
>
> Here the Azure build, without any issues:
> https://dev.azure.com/sr0718/u-boot/_build/results?buildId=219&view=results
>
> Thanks,
> Stefan
>
> Aaron Williams (1):
>   mips: octeon_nic23: Add PCIe FLR fixup via cyclic infrastructure
>
> Stefan Roese (6):
>   time: Import time_after64() and friends from Linux
>   cyclic: Add basic support for cyclic function execution infrastruture
>   cyclic: Integrate cyclic infrastructure into WATCHDOG_RESET
>   cyclic: Integrate cyclic functionality at bootup in board_r/f
>   cyclic: Add 'cyclic list' command
>   sandbox: Add cyclic demo function
>
>  MAINTAINERS                        |   7 +
>  board/Marvell/octeon_nic23/board.c | 197 +++++++++++++++++++++++++++++
>  board/sandbox/sandbox.c            |  15 +++
>  cmd/Kconfig                        |   7 +
>  cmd/Makefile                       |   1 +
>  cmd/cyclic.c                       |  40 ++++++
>  common/Kconfig                     |  20 +++
>  common/Makefile                    |   1 +
>  common/board_f.c                   |   2 +
>  common/board_r.c                   |   2 +
>  common/cyclic.c                    | 112 ++++++++++++++++
>  configs/octeon_nic23_defconfig     |   3 +
>  configs/sandbox_defconfig          |   3 +
>  fs/cramfs/uncompress.c             |   2 +-
>  include/cyclic.h                   |  97 ++++++++++++++
>  include/time.h                     |  19 +++
>  include/watchdog.h                 |  23 +++-
>  17 files changed, 547 insertions(+), 4 deletions(-)
>  create mode 100644 cmd/cyclic.c
>  create mode 100644 common/cyclic.c
>  create mode 100644 include/cyclic.h
>
> --
> 2.37.1
>

This looks interesting. I like the idea of integrating the watchdog
stuff too, since you are making use of it.

Would it make sense to make use of the new event system, since it has
static and dynamic handlers?

Regards,
Simon

Re: [PATCH v2 0/7] Add support for cyclic function execution infrastruture

[Stefan Roese]

Hi Simon,

On 31.07.22 03:27, Simon Glass wrote:
> Hi Stefan,
> 
> On Thu, 28 Jul 2022 at 01:09, Stefan Roese <sr@denx.de> wrote:
>>
>> This patchset adds the basic infrastructure to periodically execute
>> code, e.g. all 100ms. Examples for such functions might be LED blinking
>> etc. The functions that are hooked into this cyclic list should be
>> small timewise as otherwise the execution of the other code that relies
>> on a high frequent polling (e.g. UART rx char ready check) might be
>> delayed too much. This patch also adds the Kconfig option
>> CONFIG_CYCLIC_MAX_CPU_TIME_US, which configures the max allowed time
>> for such a cyclic function. If it's execution time exceeds this time,
>> this cyclic function will get removed from the cyclic list.
>>
>> How is this cyclic functionality executed?
>> This patchset integrates the main function responsible for calling all
>> registered cyclic functions cyclic_run() into the common WATCHDOG_RESET
>> macro. This guarantees that cyclic_run() is executed very often, which
>> is necessary for the cyclic functions to get scheduled and executed at
>> their configured periods.
>>
>> This cyclic infrastructure will be used by a board specific function on
>> the NIC23 MIPS Octeon board, which needs to check periodically, if a
>> PCIe FLR has occurred.
>>
>> Ideas how to continue:
>> One idea is to rename WATCHDOG_RESET to something like SCHEDULE and
>> move the watchdog_reset call into this cyclic infrastructure as well.
>> Or to perhaps move the shell UART RX ready polling to a cyclic
>> function.
>>
>> It's also possible to extend the "cyclic" command, to support the
>> creation of periodically executed shell commands (for testing etc).
>>
>> Here the Azure build, without any issues:
>> https://dev.azure.com/sr0718/u-boot/_build/results?buildId=219&view=results
>>
>> Thanks,
>> Stefan
>>
>> Aaron Williams (1):
>>    mips: octeon_nic23: Add PCIe FLR fixup via cyclic infrastructure
>>
>> Stefan Roese (6):
>>    time: Import time_after64() and friends from Linux
>>    cyclic: Add basic support for cyclic function execution infrastruture
>>    cyclic: Integrate cyclic infrastructure into WATCHDOG_RESET
>>    cyclic: Integrate cyclic functionality at bootup in board_r/f
>>    cyclic: Add 'cyclic list' command
>>    sandbox: Add cyclic demo function
>>
>>   MAINTAINERS                        |   7 +
>>   board/Marvell/octeon_nic23/board.c | 197 +++++++++++++++++++++++++++++
>>   board/sandbox/sandbox.c            |  15 +++
>>   cmd/Kconfig                        |   7 +
>>   cmd/Makefile                       |   1 +
>>   cmd/cyclic.c                       |  40 ++++++
>>   common/Kconfig                     |  20 +++
>>   common/Makefile                    |   1 +
>>   common/board_f.c                   |   2 +
>>   common/board_r.c                   |   2 +
>>   common/cyclic.c                    | 112 ++++++++++++++++
>>   configs/octeon_nic23_defconfig     |   3 +
>>   configs/sandbox_defconfig          |   3 +
>>   fs/cramfs/uncompress.c             |   2 +-
>>   include/cyclic.h                   |  97 ++++++++++++++
>>   include/time.h                     |  19 +++
>>   include/watchdog.h                 |  23 +++-
>>   17 files changed, 547 insertions(+), 4 deletions(-)
>>   create mode 100644 cmd/cyclic.c
>>   create mode 100644 common/cyclic.c
>>   create mode 100644 include/cyclic.h
>>
>> --
>> 2.37.1
>>
> 
> This looks interesting. I like the idea of integrating the watchdog
> stuff too, since you are making use of it.
> 
> Would it make sense to make use of the new event system, since it has
> static and dynamic handlers?

IIRC, I tried to make use of this infrastructure but it did not work
out. Or do you see some way to integrate this cyclic IF into the
event system? FWICT, the only way to get this done is to make use of
the (ugly) WATCHDOG_RESET calls.

Thanks,
Stefan

Re: [PATCH v2 0/7] Add support for cyclic function execution infrastruture

[Heinrich Schuchardt]

On 7/28/22 09:09, Stefan Roese wrote:
> This patchset adds the basic infrastructure to periodically execute
> code, e.g. all 100ms. Examples for such functions might be LED blinking
> etc. The functions that are hooked into this cyclic list should be
> small timewise as otherwise the execution of the other code that relies
> on a high frequent polling (e.g. UART rx char ready check) might be
> delayed too much. This patch also adds the Kconfig option
> CONFIG_CYCLIC_MAX_CPU_TIME_US, which configures the max allowed time
> for such a cyclic function. If it's execution time exceeds this time,
> this cyclic function will get removed from the cyclic list.
>
> How is this cyclic functionality executed?
> This patchset integrates the main function responsible for calling all
> registered cyclic functions cyclic_run() into the common WATCHDOG_RESET
> macro. This guarantees that cyclic_run() is executed very often, which
> is necessary for the cyclic functions to get scheduled and executed at
> their configured periods.
>
> This cyclic infrastructure will be used by a board specific function on
> the NIC23 MIPS Octeon board, which needs to check periodically, if a
> PCIe FLR has occurred.
>
> Ideas how to continue:
> One idea is to rename WATCHDOG_RESET to something like SCHEDULE and
> move the watchdog_reset call into this cyclic infrastructure as well.
> Or to perhaps move the shell UART RX ready polling to a cyclic
> function.

Hello Stefan

I am missing rework defining where WATCHDOG_RESET has to be called:

WATCHDOG_RESET is called in net_loop() but not in eth_rx() and eth_tx().
This is clearly the wrong place as not all network traffic uses net_loop().

Same is true for your reference to UART rx. WATCHDOG_RESET is called in
individual UART drivers. But this does not help if tstc() is calling
usb_kbd_testc().

So before adding this patchset please provide the concept defining where
WATCHDOG_RESET should be invoked.

A framework like the one proposed requires documentation. This needs to
be an integral part of the next version of the series.

With the infrastructure you are building efi_timer_check() is a
candidate for refactoring. Currently efi_timer_check() is called
whenever the EFI sub-system is waiting for anything (keyboard input,
polling events, network I/O).

Best regards

Heinrich

>
> It's also possible to extend the "cyclic" command, to support the
> creation of periodically executed shell commands (for testing etc).
>
> Here the Azure build, without any issues:
> https://dev.azure.com/sr0718/u-boot/_build/results?buildId=219&view=results
>
> Thanks,
> Stefan
>
> Aaron Williams (1):
>    mips: octeon_nic23: Add PCIe FLR fixup via cyclic infrastructure
>
> Stefan Roese (6):
>    time: Import time_after64() and friends from Linux
>    cyclic: Add basic support for cyclic function execution infrastruture
>    cyclic: Integrate cyclic infrastructure into WATCHDOG_RESET
>    cyclic: Integrate cyclic functionality at bootup in board_r/f
>    cyclic: Add 'cyclic list' command
>    sandbox: Add cyclic demo function
>
>   MAINTAINERS                        |   7 +
>   board/Marvell/octeon_nic23/board.c | 197 +++++++++++++++++++++++++++++
>   board/sandbox/sandbox.c            |  15 +++
>   cmd/Kconfig                        |   7 +
>   cmd/Makefile                       |   1 +
>   cmd/cyclic.c                       |  40 ++++++
>   common/Kconfig                     |  20 +++
>   common/Makefile                    |   1 +
>   common/board_f.c                   |   2 +
>   common/board_r.c                   |   2 +
>   common/cyclic.c                    | 112 ++++++++++++++++
>   configs/octeon_nic23_defconfig     |   3 +
>   configs/sandbox_defconfig          |   3 +
>   fs/cramfs/uncompress.c             |   2 +-
>   include/cyclic.h                   |  97 ++++++++++++++
>   include/time.h                     |  19 +++
>   include/watchdog.h                 |  23 +++-
>   17 files changed, 547 insertions(+), 4 deletions(-)
>   create mode 100644 cmd/cyclic.c
>   create mode 100644 common/cyclic.c
>   create mode 100644 include/cyclic.h
>

Re: [PATCH v2 0/7] Add support for cyclic function execution infrastruture

[Stefan Roese]

Hi Heinrich,

On 01.08.22 09:54, Heinrich Schuchardt wrote:
> On 7/28/22 09:09, Stefan Roese wrote:
>> This patchset adds the basic infrastructure to periodically execute
>> code, e.g. all 100ms. Examples for such functions might be LED blinking
>> etc. The functions that are hooked into this cyclic list should be
>> small timewise as otherwise the execution of the other code that relies
>> on a high frequent polling (e.g. UART rx char ready check) might be
>> delayed too much. This patch also adds the Kconfig option
>> CONFIG_CYCLIC_MAX_CPU_TIME_US, which configures the max allowed time
>> for such a cyclic function. If it's execution time exceeds this time,
>> this cyclic function will get removed from the cyclic list.
>>
>> How is this cyclic functionality executed?
>> This patchset integrates the main function responsible for calling all
>> registered cyclic functions cyclic_run() into the common WATCHDOG_RESET
>> macro. This guarantees that cyclic_run() is executed very often, which
>> is necessary for the cyclic functions to get scheduled and executed at
>> their configured periods.
>>
>> This cyclic infrastructure will be used by a board specific function on
>> the NIC23 MIPS Octeon board, which needs to check periodically, if a
>> PCIe FLR has occurred.
>>
>> Ideas how to continue:
>> One idea is to rename WATCHDOG_RESET to something like SCHEDULE and
>> move the watchdog_reset call into this cyclic infrastructure as well.
>> Or to perhaps move the shell UART RX ready polling to a cyclic
>> function.
> 
> Hello Stefan
> 
> I am missing rework defining where WATCHDOG_RESET has to be called:
> 
> WATCHDOG_RESET is called in net_loop() but not in eth_rx() and eth_tx().
> This is clearly the wrong place as not all network traffic uses net_loop().
> 
> Same is true for your reference to UART rx. WATCHDOG_RESET is called in
> individual UART drivers. But this does not help if tstc() is calling
> usb_kbd_testc().
> 
> So before adding this patchset please provide the concept defining where
> WATCHDOG_RESET should be invoked.

I agree that you are addressing valid points here. But this patchset
is not intended to change the WATCHDOG_RESET functionality. These
WDT calls and their integration (sprinkling) in the U-Boot source
are very ancient. I intentionally did not change anything here to
not break / change anything in this area for now.

The points listed from you above are all valid and most likely good
improvements or even fixes to the watchdog and once accepted also the
cyclic infrastructure. Still I think it makes much sense to address
them, once this patchset is in mainline. After this, many improvements
might be done, like:

- Integrate WDT IF as a "user" into the cyclic IF
- Rename WATCHDOG_RESET to something better matching its functionality
- Improve locations, from where these cyclic IF is called (see your
   suggestions above)
- ...

> A framework like the one proposed requires documentation. This needs to
> be an integral part of the next version of the series.

Ok, I admit that I'm a bit lazy here. I'll add some documentation in
the next version.

> With the infrastructure you are building efi_timer_check() is a
> candidate for refactoring. Currently efi_timer_check() is called
> whenever the EFI sub-system is waiting for anything (keyboard input,
> polling events, network I/O).

Interesting. I have to admit that my internal knowledge of the EFI
subsystem in U-Boot is a bit "limited". If you see improvements to this
cyclic IF that should be made, so that this cyclic IF is even more
generic and can better be used by e.g. efi_timer_check(), please let
me know.

Thanks,
Stefan

Re: [PATCH v2 0/7] Add support for cyclic function execution infrastruture

[Simon Glass]

Hi Stefan,

On Mon, 1 Aug 2022 at 01:17, Stefan Roese <sr@denx.de> wrote:
>
> Hi Simon,
>
> On 31.07.22 03:27, Simon Glass wrote:
> > Hi Stefan,
> >
> > On Thu, 28 Jul 2022 at 01:09, Stefan Roese <sr@denx.de> wrote:
> >>
> >> This patchset adds the basic infrastructure to periodically execute
> >> code, e.g. all 100ms. Examples for such functions might be LED blinking
> >> etc. The functions that are hooked into this cyclic list should be
> >> small timewise as otherwise the execution of the other code that relies
> >> on a high frequent polling (e.g. UART rx char ready check) might be
> >> delayed too much. This patch also adds the Kconfig option
> >> CONFIG_CYCLIC_MAX_CPU_TIME_US, which configures the max allowed time
> >> for such a cyclic function. If it's execution time exceeds this time,
> >> this cyclic function will get removed from the cyclic list.
> >>
> >> How is this cyclic functionality executed?
> >> This patchset integrates the main function responsible for calling all
> >> registered cyclic functions cyclic_run() into the common WATCHDOG_RESET
> >> macro. This guarantees that cyclic_run() is executed very often, which
> >> is necessary for the cyclic functions to get scheduled and executed at
> >> their configured periods.
> >>
> >> This cyclic infrastructure will be used by a board specific function on
> >> the NIC23 MIPS Octeon board, which needs to check periodically, if a
> >> PCIe FLR has occurred.
> >>
> >> Ideas how to continue:
> >> One idea is to rename WATCHDOG_RESET to something like SCHEDULE and
> >> move the watchdog_reset call into this cyclic infrastructure as well.
> >> Or to perhaps move the shell UART RX ready polling to a cyclic
> >> function.
> >>
> >> It's also possible to extend the "cyclic" command, to support the
> >> creation of periodically executed shell commands (for testing etc).
> >>
> >> Here the Azure build, without any issues:
> >> https://dev.azure.com/sr0718/u-boot/_build/results?buildId=219&view=results
> >>
> >> Thanks,
> >> Stefan
> >>
> >> Aaron Williams (1):
> >>    mips: octeon_nic23: Add PCIe FLR fixup via cyclic infrastructure
> >>
> >> Stefan Roese (6):
> >>    time: Import time_after64() and friends from Linux
> >>    cyclic: Add basic support for cyclic function execution infrastruture
> >>    cyclic: Integrate cyclic infrastructure into WATCHDOG_RESET
> >>    cyclic: Integrate cyclic functionality at bootup in board_r/f
> >>    cyclic: Add 'cyclic list' command
> >>    sandbox: Add cyclic demo function
> >>
> >>   MAINTAINERS                        |   7 +
> >>   board/Marvell/octeon_nic23/board.c | 197 +++++++++++++++++++++++++++++
> >>   board/sandbox/sandbox.c            |  15 +++
> >>   cmd/Kconfig                        |   7 +
> >>   cmd/Makefile                       |   1 +
> >>   cmd/cyclic.c                       |  40 ++++++
> >>   common/Kconfig                     |  20 +++
> >>   common/Makefile                    |   1 +
> >>   common/board_f.c                   |   2 +
> >>   common/board_r.c                   |   2 +
> >>   common/cyclic.c                    | 112 ++++++++++++++++
> >>   configs/octeon_nic23_defconfig     |   3 +
> >>   configs/sandbox_defconfig          |   3 +
> >>   fs/cramfs/uncompress.c             |   2 +-
> >>   include/cyclic.h                   |  97 ++++++++++++++
> >>   include/time.h                     |  19 +++
> >>   include/watchdog.h                 |  23 +++-
> >>   17 files changed, 547 insertions(+), 4 deletions(-)
> >>   create mode 100644 cmd/cyclic.c
> >>   create mode 100644 common/cyclic.c
> >>   create mode 100644 include/cyclic.h
> >>
> >> --
> >> 2.37.1
> >>
> >
> > This looks interesting. I like the idea of integrating the watchdog
> > stuff too, since you are making use of it.
> >
> > Would it make sense to make use of the new event system, since it has
> > static and dynamic handlers?
>
> IIRC, I tried to make use of this infrastructure but it did not work
> out. Or do you see some way to integrate this cyclic IF into the
> event system? FWICT, the only way to get this done is to make use of
> the (ugly) WATCHDOG_RESET calls.

Yes that makes sense. I don't see another way.

But within that, one option would be to send an event every time
WATCHDOG_RESET is used, and have things register as an event spy, thus
making use of that existing system. The event feature is not enabled
by default, but it could be enabled when this functionality is needed.

Regards,
Simon

Re: [PATCH v2 0/7] Add support for cyclic function execution infrastruture

[Stefan Roese]

Hi Simon,

On 01.08.22 14:22, Simon Glass wrote:
> Hi Stefan,
> 
> On Mon, 1 Aug 2022 at 01:17, Stefan Roese <sr@denx.de> wrote:
>>
>> Hi Simon,
>>
>> On 31.07.22 03:27, Simon Glass wrote:
>>> Hi Stefan,
>>>
>>> On Thu, 28 Jul 2022 at 01:09, Stefan Roese <sr@denx.de> wrote:
>>>>
>>>> This patchset adds the basic infrastructure to periodically execute
>>>> code, e.g. all 100ms. Examples for such functions might be LED blinking
>>>> etc. The functions that are hooked into this cyclic list should be
>>>> small timewise as otherwise the execution of the other code that relies
>>>> on a high frequent polling (e.g. UART rx char ready check) might be
>>>> delayed too much. This patch also adds the Kconfig option
>>>> CONFIG_CYCLIC_MAX_CPU_TIME_US, which configures the max allowed time
>>>> for such a cyclic function. If it's execution time exceeds this time,
>>>> this cyclic function will get removed from the cyclic list.
>>>>
>>>> How is this cyclic functionality executed?
>>>> This patchset integrates the main function responsible for calling all
>>>> registered cyclic functions cyclic_run() into the common WATCHDOG_RESET
>>>> macro. This guarantees that cyclic_run() is executed very often, which
>>>> is necessary for the cyclic functions to get scheduled and executed at
>>>> their configured periods.
>>>>
>>>> This cyclic infrastructure will be used by a board specific function on
>>>> the NIC23 MIPS Octeon board, which needs to check periodically, if a
>>>> PCIe FLR has occurred.
>>>>
>>>> Ideas how to continue:
>>>> One idea is to rename WATCHDOG_RESET to something like SCHEDULE and
>>>> move the watchdog_reset call into this cyclic infrastructure as well.
>>>> Or to perhaps move the shell UART RX ready polling to a cyclic
>>>> function.
>>>>
>>>> It's also possible to extend the "cyclic" command, to support the
>>>> creation of periodically executed shell commands (for testing etc).
>>>>
>>>> Here the Azure build, without any issues:
>>>> https://dev.azure.com/sr0718/u-boot/_build/results?buildId=219&view=results
>>>>
>>>> Thanks,
>>>> Stefan
>>>>
>>>> Aaron Williams (1):
>>>>     mips: octeon_nic23: Add PCIe FLR fixup via cyclic infrastructure
>>>>
>>>> Stefan Roese (6):
>>>>     time: Import time_after64() and friends from Linux
>>>>     cyclic: Add basic support for cyclic function execution infrastruture
>>>>     cyclic: Integrate cyclic infrastructure into WATCHDOG_RESET
>>>>     cyclic: Integrate cyclic functionality at bootup in board_r/f
>>>>     cyclic: Add 'cyclic list' command
>>>>     sandbox: Add cyclic demo function
>>>>
>>>>    MAINTAINERS                        |   7 +
>>>>    board/Marvell/octeon_nic23/board.c | 197 +++++++++++++++++++++++++++++
>>>>    board/sandbox/sandbox.c            |  15 +++
>>>>    cmd/Kconfig                        |   7 +
>>>>    cmd/Makefile                       |   1 +
>>>>    cmd/cyclic.c                       |  40 ++++++
>>>>    common/Kconfig                     |  20 +++
>>>>    common/Makefile                    |   1 +
>>>>    common/board_f.c                   |   2 +
>>>>    common/board_r.c                   |   2 +
>>>>    common/cyclic.c                    | 112 ++++++++++++++++
>>>>    configs/octeon_nic23_defconfig     |   3 +
>>>>    configs/sandbox_defconfig          |   3 +
>>>>    fs/cramfs/uncompress.c             |   2 +-
>>>>    include/cyclic.h                   |  97 ++++++++++++++
>>>>    include/time.h                     |  19 +++
>>>>    include/watchdog.h                 |  23 +++-
>>>>    17 files changed, 547 insertions(+), 4 deletions(-)
>>>>    create mode 100644 cmd/cyclic.c
>>>>    create mode 100644 common/cyclic.c
>>>>    create mode 100644 include/cyclic.h
>>>>
>>>> --
>>>> 2.37.1
>>>>
>>>
>>> This looks interesting. I like the idea of integrating the watchdog
>>> stuff too, since you are making use of it.
>>>
>>> Would it make sense to make use of the new event system, since it has
>>> static and dynamic handlers?
>>
>> IIRC, I tried to make use of this infrastructure but it did not work
>> out. Or do you see some way to integrate this cyclic IF into the
>> event system? FWICT, the only way to get this done is to make use of
>> the (ugly) WATCHDOG_RESET calls.
> 
> Yes that makes sense. I don't see another way.
> 
> But within that, one option would be to send an event every time
> WATCHDOG_RESET is used, and have things register as an event spy, thus
> making use of that existing system. The event feature is not enabled
> by default, but it could be enabled when this functionality is needed.

I still need to double-check, sorry: You are thinking about this call-
trace:

WATCHDOG_RESET -> event IF -> cylic IF -> cylic user

?

If yes, what would be the advantage(s) against implementing this
separately?

Thanks,
Stefan

Re: [PATCH v2 0/7] Add support for cyclic function execution infrastruture

[Simon Glass]

Hi Stefan,

On Mon, 1 Aug 2022 at 06:40, Stefan Roese <sr@denx.de> wrote:
>
> Hi Simon,
>
> On 01.08.22 14:22, Simon Glass wrote:
> > Hi Stefan,
> >
> > On Mon, 1 Aug 2022 at 01:17, Stefan Roese <sr@denx.de> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 31.07.22 03:27, Simon Glass wrote:
> >>> Hi Stefan,
> >>>
> >>> On Thu, 28 Jul 2022 at 01:09, Stefan Roese <sr@denx.de> wrote:
> >>>>
> >>>> This patchset adds the basic infrastructure to periodically execute
> >>>> code, e.g. all 100ms. Examples for such functions might be LED blinking
> >>>> etc. The functions that are hooked into this cyclic list should be
> >>>> small timewise as otherwise the execution of the other code that relies
> >>>> on a high frequent polling (e.g. UART rx char ready check) might be
> >>>> delayed too much. This patch also adds the Kconfig option
> >>>> CONFIG_CYCLIC_MAX_CPU_TIME_US, which configures the max allowed time
> >>>> for such a cyclic function. If it's execution time exceeds this time,
> >>>> this cyclic function will get removed from the cyclic list.
> >>>>
> >>>> How is this cyclic functionality executed?
> >>>> This patchset integrates the main function responsible for calling all
> >>>> registered cyclic functions cyclic_run() into the common WATCHDOG_RESET
> >>>> macro. This guarantees that cyclic_run() is executed very often, which
> >>>> is necessary for the cyclic functions to get scheduled and executed at
> >>>> their configured periods.
> >>>>
> >>>> This cyclic infrastructure will be used by a board specific function on
> >>>> the NIC23 MIPS Octeon board, which needs to check periodically, if a
> >>>> PCIe FLR has occurred.
> >>>>
> >>>> Ideas how to continue:
> >>>> One idea is to rename WATCHDOG_RESET to something like SCHEDULE and
> >>>> move the watchdog_reset call into this cyclic infrastructure as well.
> >>>> Or to perhaps move the shell UART RX ready polling to a cyclic
> >>>> function.
> >>>>
> >>>> It's also possible to extend the "cyclic" command, to support the
> >>>> creation of periodically executed shell commands (for testing etc).
> >>>>
> >>>> Here the Azure build, without any issues:
> >>>> https://dev.azure.com/sr0718/u-boot/_build/results?buildId=219&view=results
> >>>>
> >>>> Thanks,
> >>>> Stefan
> >>>>
> >>>> Aaron Williams (1):
> >>>>     mips: octeon_nic23: Add PCIe FLR fixup via cyclic infrastructure
> >>>>
> >>>> Stefan Roese (6):
> >>>>     time: Import time_after64() and friends from Linux
> >>>>     cyclic: Add basic support for cyclic function execution infrastruture
> >>>>     cyclic: Integrate cyclic infrastructure into WATCHDOG_RESET
> >>>>     cyclic: Integrate cyclic functionality at bootup in board_r/f
> >>>>     cyclic: Add 'cyclic list' command
> >>>>     sandbox: Add cyclic demo function
> >>>>
> >>>>    MAINTAINERS                        |   7 +
> >>>>    board/Marvell/octeon_nic23/board.c | 197 +++++++++++++++++++++++++++++
> >>>>    board/sandbox/sandbox.c            |  15 +++
> >>>>    cmd/Kconfig                        |   7 +
> >>>>    cmd/Makefile                       |   1 +
> >>>>    cmd/cyclic.c                       |  40 ++++++
> >>>>    common/Kconfig                     |  20 +++
> >>>>    common/Makefile                    |   1 +
> >>>>    common/board_f.c                   |   2 +
> >>>>    common/board_r.c                   |   2 +
> >>>>    common/cyclic.c                    | 112 ++++++++++++++++
> >>>>    configs/octeon_nic23_defconfig     |   3 +
> >>>>    configs/sandbox_defconfig          |   3 +
> >>>>    fs/cramfs/uncompress.c             |   2 +-
> >>>>    include/cyclic.h                   |  97 ++++++++++++++
> >>>>    include/time.h                     |  19 +++
> >>>>    include/watchdog.h                 |  23 +++-
> >>>>    17 files changed, 547 insertions(+), 4 deletions(-)
> >>>>    create mode 100644 cmd/cyclic.c
> >>>>    create mode 100644 common/cyclic.c
> >>>>    create mode 100644 include/cyclic.h
> >>>>
> >>>> --
> >>>> 2.37.1
> >>>>
> >>>
> >>> This looks interesting. I like the idea of integrating the watchdog
> >>> stuff too, since you are making use of it.
> >>>
> >>> Would it make sense to make use of the new event system, since it has
> >>> static and dynamic handlers?
> >>
> >> IIRC, I tried to make use of this infrastructure but it did not work
> >> out. Or do you see some way to integrate this cyclic IF into the
> >> event system? FWICT, the only way to get this done is to make use of
> >> the (ugly) WATCHDOG_RESET calls.
> >
> > Yes that makes sense. I don't see another way.
> >
> > But within that, one option would be to send an event every time
> > WATCHDOG_RESET is used, and have things register as an event spy, thus
> > making use of that existing system. The event feature is not enabled
> > by default, but it could be enabled when this functionality is needed.
>
> I still need to double-check, sorry: You are thinking about this call-
> trace:
>
> WATCHDOG_RESET -> event IF -> cylic IF -> cylic user
>

More this, i.e. I am wondering if the cyclic functionality can be done
using events.

WATCHDOG_RESET -> sends watchdog event -> event spy receives event

> ?
>
> If yes, what would be the advantage(s) against implementing this
> separately?

It would need handling of max CPU time and perhaps some other tweaks,
but it would result in less code (?) and one less thing for people to
learn. The cyclic command could still be provided if that is useful,
by showing only cyclic certain event handlers.

Regards,
Simon

Re: [PATCH v2 0/7] Add support for cyclic function execution infrastruture

[Stefan Roese]

Hi Simon,

On 01.08.22 15:00, Simon Glass wrote:
> Hi Stefan,
> 
> On Mon, 1 Aug 2022 at 06:40, Stefan Roese <sr@denx.de> wrote:
>>
>> Hi Simon,
>>
>> On 01.08.22 14:22, Simon Glass wrote:
>>> Hi Stefan,
>>>
>>> On Mon, 1 Aug 2022 at 01:17, Stefan Roese <sr@denx.de> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> On 31.07.22 03:27, Simon Glass wrote:
>>>>> Hi Stefan,
>>>>>
>>>>> On Thu, 28 Jul 2022 at 01:09, Stefan Roese <sr@denx.de> wrote:
>>>>>>
>>>>>> This patchset adds the basic infrastructure to periodically execute
>>>>>> code, e.g. all 100ms. Examples for such functions might be LED blinking
>>>>>> etc. The functions that are hooked into this cyclic list should be
>>>>>> small timewise as otherwise the execution of the other code that relies
>>>>>> on a high frequent polling (e.g. UART rx char ready check) might be
>>>>>> delayed too much. This patch also adds the Kconfig option
>>>>>> CONFIG_CYCLIC_MAX_CPU_TIME_US, which configures the max allowed time
>>>>>> for such a cyclic function. If it's execution time exceeds this time,
>>>>>> this cyclic function will get removed from the cyclic list.
>>>>>>
>>>>>> How is this cyclic functionality executed?
>>>>>> This patchset integrates the main function responsible for calling all
>>>>>> registered cyclic functions cyclic_run() into the common WATCHDOG_RESET
>>>>>> macro. This guarantees that cyclic_run() is executed very often, which
>>>>>> is necessary for the cyclic functions to get scheduled and executed at
>>>>>> their configured periods.
>>>>>>
>>>>>> This cyclic infrastructure will be used by a board specific function on
>>>>>> the NIC23 MIPS Octeon board, which needs to check periodically, if a
>>>>>> PCIe FLR has occurred.
>>>>>>
>>>>>> Ideas how to continue:
>>>>>> One idea is to rename WATCHDOG_RESET to something like SCHEDULE and
>>>>>> move the watchdog_reset call into this cyclic infrastructure as well.
>>>>>> Or to perhaps move the shell UART RX ready polling to a cyclic
>>>>>> function.
>>>>>>
>>>>>> It's also possible to extend the "cyclic" command, to support the
>>>>>> creation of periodically executed shell commands (for testing etc).
>>>>>>
>>>>>> Here the Azure build, without any issues:
>>>>>> https://dev.azure.com/sr0718/u-boot/_build/results?buildId=219&view=results
>>>>>>
>>>>>> Thanks,
>>>>>> Stefan
>>>>>>
>>>>>> Aaron Williams (1):
>>>>>>      mips: octeon_nic23: Add PCIe FLR fixup via cyclic infrastructure
>>>>>>
>>>>>> Stefan Roese (6):
>>>>>>      time: Import time_after64() and friends from Linux
>>>>>>      cyclic: Add basic support for cyclic function execution infrastruture
>>>>>>      cyclic: Integrate cyclic infrastructure into WATCHDOG_RESET
>>>>>>      cyclic: Integrate cyclic functionality at bootup in board_r/f
>>>>>>      cyclic: Add 'cyclic list' command
>>>>>>      sandbox: Add cyclic demo function
>>>>>>
>>>>>>     MAINTAINERS                        |   7 +
>>>>>>     board/Marvell/octeon_nic23/board.c | 197 +++++++++++++++++++++++++++++
>>>>>>     board/sandbox/sandbox.c            |  15 +++
>>>>>>     cmd/Kconfig                        |   7 +
>>>>>>     cmd/Makefile                       |   1 +
>>>>>>     cmd/cyclic.c                       |  40 ++++++
>>>>>>     common/Kconfig                     |  20 +++
>>>>>>     common/Makefile                    |   1 +
>>>>>>     common/board_f.c                   |   2 +
>>>>>>     common/board_r.c                   |   2 +
>>>>>>     common/cyclic.c                    | 112 ++++++++++++++++
>>>>>>     configs/octeon_nic23_defconfig     |   3 +
>>>>>>     configs/sandbox_defconfig          |   3 +
>>>>>>     fs/cramfs/uncompress.c             |   2 +-
>>>>>>     include/cyclic.h                   |  97 ++++++++++++++
>>>>>>     include/time.h                     |  19 +++
>>>>>>     include/watchdog.h                 |  23 +++-
>>>>>>     17 files changed, 547 insertions(+), 4 deletions(-)
>>>>>>     create mode 100644 cmd/cyclic.c
>>>>>>     create mode 100644 common/cyclic.c
>>>>>>     create mode 100644 include/cyclic.h
>>>>>>
>>>>>> --
>>>>>> 2.37.1
>>>>>>
>>>>>
>>>>> This looks interesting. I like the idea of integrating the watchdog
>>>>> stuff too, since you are making use of it.
>>>>>
>>>>> Would it make sense to make use of the new event system, since it has
>>>>> static and dynamic handlers?
>>>>
>>>> IIRC, I tried to make use of this infrastructure but it did not work
>>>> out. Or do you see some way to integrate this cyclic IF into the
>>>> event system? FWICT, the only way to get this done is to make use of
>>>> the (ugly) WATCHDOG_RESET calls.
>>>
>>> Yes that makes sense. I don't see another way.
>>>
>>> But within that, one option would be to send an event every time
>>> WATCHDOG_RESET is used, and have things register as an event spy, thus
>>> making use of that existing system. The event feature is not enabled
>>> by default, but it could be enabled when this functionality is needed.
>>
>> I still need to double-check, sorry: You are thinking about this call-
>> trace:
>>
>> WATCHDOG_RESET -> event IF -> cylic IF -> cylic user
>>
> 
> More this, i.e. I am wondering if the cyclic functionality can be done
> using events.
> 
> WATCHDOG_RESET -> sends watchdog event -> event spy receives event

This would mean in the end, that all U-Boot targets would need to
enable the event subsystem as well. This might lead to problems with
image sizes on some of those especially old/ancient targets. Hmmm...

>> ?
>>
>> If yes, what would be the advantage(s) against implementing this
>> separately?
> 
> It would need handling of max CPU time and perhaps some other tweaks,
> but it would result in less code (?) and one less thing for people to
> learn. The cyclic command could still be provided if that is useful,
> by showing only cyclic certain event handlers.

Frankly, I'm not 100% convinced that this would really result in less
code. Or even worse, it could (?) result in more complex code, as both
interfaces are in the same area but still handle different tasks,
e.g. synchronous vs. asynchronous.

So my preference would be to continue on my current path. I'll integrate
some documentation in my next patchset version and will resubmit
hopefully sometime later this week.

Thanks,
Stefan

Re: [PATCH v2 0/7] Add support for cyclic function execution infrastruture

[Simon Glass]

Hi Stefan,

On Mon, 1 Aug 2022 at 08:09, Stefan Roese <sr@denx.de> wrote:
>
> Hi Simon,
>
> On 01.08.22 15:00, Simon Glass wrote:
> > Hi Stefan,
> >
> > On Mon, 1 Aug 2022 at 06:40, Stefan Roese <sr@denx.de> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 01.08.22 14:22, Simon Glass wrote:
> >>> Hi Stefan,
> >>>
> >>> On Mon, 1 Aug 2022 at 01:17, Stefan Roese <sr@denx.de> wrote:
> >>>>
> >>>> Hi Simon,
> >>>>
> >>>> On 31.07.22 03:27, Simon Glass wrote:
> >>>>> Hi Stefan,
> >>>>>
> >>>>> On Thu, 28 Jul 2022 at 01:09, Stefan Roese <sr@denx.de> wrote:
> >>>>>>
> >>>>>> This patchset adds the basic infrastructure to periodically execute
> >>>>>> code, e.g. all 100ms. Examples for such functions might be LED blinking
> >>>>>> etc. The functions that are hooked into this cyclic list should be
> >>>>>> small timewise as otherwise the execution of the other code that relies
> >>>>>> on a high frequent polling (e.g. UART rx char ready check) might be
> >>>>>> delayed too much. This patch also adds the Kconfig option
> >>>>>> CONFIG_CYCLIC_MAX_CPU_TIME_US, which configures the max allowed time
> >>>>>> for such a cyclic function. If it's execution time exceeds this time,
> >>>>>> this cyclic function will get removed from the cyclic list.
> >>>>>>
> >>>>>> How is this cyclic functionality executed?
> >>>>>> This patchset integrates the main function responsible for calling all
> >>>>>> registered cyclic functions cyclic_run() into the common WATCHDOG_RESET
> >>>>>> macro. This guarantees that cyclic_run() is executed very often, which
> >>>>>> is necessary for the cyclic functions to get scheduled and executed at
> >>>>>> their configured periods.
> >>>>>>
> >>>>>> This cyclic infrastructure will be used by a board specific function on
> >>>>>> the NIC23 MIPS Octeon board, which needs to check periodically, if a
> >>>>>> PCIe FLR has occurred.
> >>>>>>
> >>>>>> Ideas how to continue:
> >>>>>> One idea is to rename WATCHDOG_RESET to something like SCHEDULE and
> >>>>>> move the watchdog_reset call into this cyclic infrastructure as well.
> >>>>>> Or to perhaps move the shell UART RX ready polling to a cyclic
> >>>>>> function.
> >>>>>>
> >>>>>> It's also possible to extend the "cyclic" command, to support the
> >>>>>> creation of periodically executed shell commands (for testing etc).
> >>>>>>
> >>>>>> Here the Azure build, without any issues:
> >>>>>> https://dev.azure.com/sr0718/u-boot/_build/results?buildId=219&view=results
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Stefan
> >>>>>>
> >>>>>> Aaron Williams (1):
> >>>>>>      mips: octeon_nic23: Add PCIe FLR fixup via cyclic infrastructure
> >>>>>>
> >>>>>> Stefan Roese (6):
> >>>>>>      time: Import time_after64() and friends from Linux
> >>>>>>      cyclic: Add basic support for cyclic function execution infrastruture
> >>>>>>      cyclic: Integrate cyclic infrastructure into WATCHDOG_RESET
> >>>>>>      cyclic: Integrate cyclic functionality at bootup in board_r/f
> >>>>>>      cyclic: Add 'cyclic list' command
> >>>>>>      sandbox: Add cyclic demo function
> >>>>>>
> >>>>>>     MAINTAINERS                        |   7 +
> >>>>>>     board/Marvell/octeon_nic23/board.c | 197 +++++++++++++++++++++++++++++
> >>>>>>     board/sandbox/sandbox.c            |  15 +++
> >>>>>>     cmd/Kconfig                        |   7 +
> >>>>>>     cmd/Makefile                       |   1 +
> >>>>>>     cmd/cyclic.c                       |  40 ++++++
> >>>>>>     common/Kconfig                     |  20 +++
> >>>>>>     common/Makefile                    |   1 +
> >>>>>>     common/board_f.c                   |   2 +
> >>>>>>     common/board_r.c                   |   2 +
> >>>>>>     common/cyclic.c                    | 112 ++++++++++++++++
> >>>>>>     configs/octeon_nic23_defconfig     |   3 +
> >>>>>>     configs/sandbox_defconfig          |   3 +
> >>>>>>     fs/cramfs/uncompress.c             |   2 +-
> >>>>>>     include/cyclic.h                   |  97 ++++++++++++++
> >>>>>>     include/time.h                     |  19 +++
> >>>>>>     include/watchdog.h                 |  23 +++-
> >>>>>>     17 files changed, 547 insertions(+), 4 deletions(-)
> >>>>>>     create mode 100644 cmd/cyclic.c
> >>>>>>     create mode 100644 common/cyclic.c
> >>>>>>     create mode 100644 include/cyclic.h
> >>>>>>
> >>>>>> --
> >>>>>> 2.37.1
> >>>>>>
> >>>>>
> >>>>> This looks interesting. I like the idea of integrating the watchdog
> >>>>> stuff too, since you are making use of it.
> >>>>>
> >>>>> Would it make sense to make use of the new event system, since it has
> >>>>> static and dynamic handlers?
> >>>>
> >>>> IIRC, I tried to make use of this infrastructure but it did not work
> >>>> out. Or do you see some way to integrate this cyclic IF into the
> >>>> event system? FWICT, the only way to get this done is to make use of
> >>>> the (ugly) WATCHDOG_RESET calls.
> >>>
> >>> Yes that makes sense. I don't see another way.
> >>>
> >>> But within that, one option would be to send an event every time
> >>> WATCHDOG_RESET is used, and have things register as an event spy, thus
> >>> making use of that existing system. The event feature is not enabled
> >>> by default, but it could be enabled when this functionality is needed.
> >>
> >> I still need to double-check, sorry: You are thinking about this call-
> >> trace:
> >>
> >> WATCHDOG_RESET -> event IF -> cylic IF -> cylic user
> >>
> >
> > More this, i.e. I am wondering if the cyclic functionality can be done
> > using events.
> >
> > WATCHDOG_RESET -> sends watchdog event -> event spy receives event
>
> This would mean in the end, that all U-Boot targets would need to
> enable the event subsystem as well. This might lead to problems with
> image sizes on some of those especially old/ancient targets. Hmmm...

It depends on whether cyclic is a small adder to events or something
larger. Actually events are not too big, but having 'dynamic' ones
(allocated at runtime) does add a bit more.

>
> >> ?
> >>
> >> If yes, what would be the advantage(s) against implementing this
> >> separately?
> >
> > It would need handling of max CPU time and perhaps some other tweaks,
> > but it would result in less code (?) and one less thing for people to
> > learn. The cyclic command could still be provided if that is useful,
> > by showing only cyclic certain event handlers.
>
> Frankly, I'm not 100% convinced that this would really result in less
> code. Or even worse, it could (?) result in more complex code, as both
> interfaces are in the same area but still handle different tasks,
> e.g. synchronous vs. asynchronous.
>
> So my preference would be to continue on my current path. I'll integrate
> some documentation in my next patchset version and will resubmit
> hopefully sometime later this week.

OK, sounds good. You are writing the code :-)

Regards,
Simon