On Thu, 3 Oct 2024 at 15:28, Rasmus Villemoes ravi@prevas.dk wrote:

WATCHDOG_RESET is no more. Replace the reference by schedule().

While here, rearrange the sentence a bit so that "cyclic_run()" becomes the object and "the main function responsible for calling all registered cyclic functions" a parenthetical rather than the other way around, which at least to me makes it more readable.

Signed-off-by: Rasmus Villemoes ravi@prevas.dk

doc/develop/cyclic.rst | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

diff --git a/doc/develop/cyclic.rst b/doc/develop/cyclic.rst index 893c269099a..6f1da6f0d9b 100644 --- a/doc/develop/cyclic.rst +++ b/doc/develop/cyclic.rst @@ -49,8 +49,8 @@ executed all 10ms. How is this cyclic functionality integrated / executed?

---

-The cyclic infrastructure 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. +The cyclic infrastructure integrates cyclic_run(), the main function +responsible for calling all registered cyclic functions, into the +common schedule() function. 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.

2.46.2

On 10/3/24 23:27, Rasmus Villemoes wrote:

I noticed an "unnecessary" include of <cyclic.h> in global_data.h, in the sense that nothing in cyclic.h is needed in order to define 'struct global_data'.

Well, it's not unnecessary, as it implicitly ensures that everybody gets a declaration of schedule(), and schedule() is (obviously) called all over the tree. Almost none of those places directly include <cyclic.h>, but for historical reasons, many do include <watchdog.h> (most schedule() instances are replacements of WATCHDOG_RESET()).

However, very few TUs actually need the declarations of the cyclic_register() and struct cyclic_info, and they also don't really need anything from the watchdog.h header.

So introduce a new header which just contains a declaration of schedule(), which can then be included from all the places that do call schedule(). I removed the direct reference to cyclic_run(), because we shouldn't have two public functions for doing roughly the same without being very explicit about when one should call one or the other.

Testing of later patches that explicitly include <schedule.h> when schedule() is used revealed a problem with host tool build on win32, which apparently picked up a host <schedule.h>. To avoid that problem, put the new header in include/u-boot/ and hence make the include statements say <u-boot/schedule.h>.

Signed-off-by: Rasmus Villemoes ravi@prevas.dk

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

common/cyclic.c | 1 + include/cyclic.h | 12 +----------- include/u-boot/schedule.h | 24 ++++++++++++++++++++++++ 3 files changed, 26 insertions(+), 11 deletions(-) create mode 100644 include/u-boot/schedule.h

diff --git a/common/cyclic.c b/common/cyclic.c index ec38fad6775..38d815bec35 100644 --- a/common/cyclic.c +++ b/common/cyclic.c @@ -15,6 +15,7 @@ #include <linux/errno.h> #include <linux/list.h> #include <asm/global_data.h> +#include <u-boot/schedule.h>

DECLARE_GLOBAL_DATA_PTR;

diff --git a/include/cyclic.h b/include/cyclic.h index cd95b691d48..e8de616dcd5 100644 --- a/include/cyclic.h +++ b/include/cyclic.h @@ -13,6 +13,7 @@

#include <linux/list.h> #include <asm/types.h> +#include <u-boot/schedule.h> // to be removed later

/**

• struct cyclic_info - Information about cyclic execution function

@@ -94,13 +95,6 @@ struct hlist_head *cyclic_get_list(void); */ void cyclic_run(void);

-/**

• • schedule() - Schedule all potentially waiting tasks
• • Basically a wrapper for cyclic_run(), pontentially enhanced by some
• • other parts, that need to get handled periodically.
• */

-void schedule(void); #else

static inline void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func, @@ -116,10 +110,6 @@ static inline void cyclic_run(void) { }

-static inline void schedule(void) -{ -}

• static inline int cyclic_unregister_all(void) { return 0;

diff --git a/include/u-boot/schedule.h b/include/u-boot/schedule.h new file mode 100644 index 00000000000..4fd34c41229 --- /dev/null +++ b/include/u-boot/schedule.h @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: GPL-2.0+ */

+#ifndef _U_BOOT_SCHEDULE_H +#define _U_BOOT_SCHEDULE_H

+#if CONFIG_IS_ENABLED(CYCLIC) +/**

• • schedule() - Schedule all potentially waiting tasks
• • Run all pending tasks registered via the cyclic framework, and
• • potentially perform other actions that need to be done
• • periodically.
• */

+void schedule(void);

+#else

+static inline void schedule(void) +{ +}

+#endif

+#endif

Viele Grüße, Stefan Roese

On Thu, 3 Oct 2024 at 15:28, Rasmus Villemoes ravi@prevas.dk wrote:

This makes use of the cyclic API but relies on implicitly getting the appropriate declarations through some nested include. Include the cyclic.h header directly.

Signed-off-by: Rasmus Villemoes ravi@prevas.dk

drivers/led/led_sw_blink.c | 1 + 1 file changed, 1 insertion(+)

Reviewed-by: Simon Glass sjg@chromium.org

diff --git a/drivers/led/led_sw_blink.c b/drivers/led/led_sw_blink.c index 9e36edbee47..6976b84544e 100644 --- a/drivers/led/led_sw_blink.c +++ b/drivers/led/led_sw_blink.c @@ -5,6 +5,7 @@

• Author: Mikhail Kshevetskiy mikhail.kshevetskiy@iopsys.eu

*/

+#include <cyclic.h> #include <dm.h> #include <led.h>

#include <time.h>

2.46.2

On 10/3/24 23:27, Rasmus Villemoes wrote:

Modifying a generic header like watchdog.h, removing not directly used #includes, can apparently break the build of m68k, because the asm/ptrace.h header relies on whoever includes it to already have included something that defines the type ulong.

Make the asm/ptrace.h header self-contained by including the proper header.

Signed-off-by: Rasmus Villemoes ravi@prevas.dk

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

arch/m68k/include/asm/ptrace.h | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/arch/m68k/include/asm/ptrace.h b/arch/m68k/include/asm/ptrace.h index d419824806c..5decf73a1d1 100644 --- a/arch/m68k/include/asm/ptrace.h +++ b/arch/m68k/include/asm/ptrace.h @@ -9,6 +9,8 @@ */ #ifndef __ASSEMBLY__

+#include <linux/types.h>

• struct pt_regs { ulong d0; ulong d1;

Viele Grüße, Stefan Roese

On Thu, 3 Oct 2024 at 15:29, Rasmus Villemoes ravi@prevas.dk wrote:

Prior to commit 29caf9305b6f ("cyclic: Use schedule() instead of WATCHDOG_RESET()") we had

/* Currently only needed for fs/cramfs/uncompress.c */ static inline void watchdog_reset_func(void) { WATCHDOG_RESET(); }

and .outcb was set to that watchdog_reset_func(). Said commit changed that .outcb to cyclic_run instead of schedule, which would otherwise match all the other WATCHDOG_RESET replacements done. As the HW_WATCHDOG case is not handled by cyclic_run, this seems to be an oversight.

Signed-off-by: Rasmus Villemoes ravi@prevas.dk

fs/cramfs/uncompress.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

diff --git a/fs/cramfs/uncompress.c b/fs/cramfs/uncompress.c index 2141edf22e4..97af8cb2b4f 100644 --- a/fs/cramfs/uncompress.c +++ b/fs/cramfs/uncompress.c @@ -21,9 +21,9 @@ */

#include <stdio.h> -#include <cyclic.h> #include <malloc.h> #include <watchdog.h> +#include <u-boot/schedule.h> #include <u-boot/zlib.h>

static z_stream stream; @@ -63,7 +63,7 @@ int cramfs_uncompress_init (void) stream.avail_in = 0;

#if defined(CONFIG_HW_WATCHDOG) || defined(CONFIG_WATCHDOG)

•   stream.outcb = (cb_func)cyclic_run;
  

•   stream.outcb = (cb_func)schedule;
  

#else stream.outcb = Z_NULL;

#endif /* CONFIG_HW_WATCHDOG */

2.46.2

On 10/3/24 23:27, Rasmus Villemoes wrote:

This is the last place outside of cyclic.c that references cyclic_run() directly. Replace by schedule(), so that cyclic_run() can be made private. This also better matches what I believe commit 29caf9305b6f ("cyclic: Use schedule() instead of WATCHDOG_RESET()") intended to do.

Signed-off-by: Rasmus Villemoes ravi@prevas.dk

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

test/dm/wdt.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/test/dm/wdt.c b/test/dm/wdt.c index 1df2da23c6c..4d751b7e8cf 100644 --- a/test/dm/wdt.c +++ b/test/dm/wdt.c @@ -3,7 +3,6 @@

• Copyright 2017 Google, Inc

*/

-#include <cyclic.h> #include <dm.h> #include <time.h> #include <wdt.h> @@ -14,6 +13,7 @@ #include <test/test.h> #include <test/ut.h> #include <linux/delay.h> +#include <u-boot/schedule.h> #include <watchdog.h>

/* Test that watchdog driver functions are called */ @@ -131,7 +131,7 @@ static int dm_test_wdt_watchdog_reset(struct unit_test_state *uts) /* Neither device should be "started", so watchdog_reset() should be a no-op. */ reset_count = state->wdt.reset_count; val = sandbox_gpio_get_value(gpio, offset);

• cyclic_run();

• schedule(); ut_asserteq(reset_count, state->wdt.reset_count); ut_asserteq(val, sandbox_gpio_get_value(gpio, offset));

@@ -141,19 +141,19 @@ static int dm_test_wdt_watchdog_reset(struct unit_test_state *uts)

/* Make sure both devices have just been pinged. */ timer_test_add_offset(100);

• cyclic_run();

• schedule(); reset_count = state->wdt.reset_count; val = sandbox_gpio_get_value(gpio, offset);

  /* The gpio watchdog should be pinged, the sandbox one not. */ timer_test_add_offset(30);

• cyclic_run();

• schedule(); ut_asserteq(reset_count, state->wdt.reset_count); ut_asserteq(!val, sandbox_gpio_get_value(gpio, offset));

  /* After another ~30ms, both devices should get pinged. */ timer_test_add_offset(30);

• cyclic_run();

• schedule(); ut_asserteq(reset_count + 1, state->wdt.reset_count); ut_asserteq(val, sandbox_gpio_get_value(gpio, offset));

Viele Grüße, Stefan Roese

On 10/3/24 23:27, Rasmus Villemoes wrote:

Nobody relies on getting the cyclic API declared by including the watchdog.h header, but for historical reasons, many TUs include watchdog.h to get a declaration of schedule(). Now that we have a dedicated header for just that, include that header instead of cyclic.h.

Eventually, all TUs that call schedule() should themselves include u-boot/schedule.h, but this is a step towards getting rid of unnecessary include statements in cyclic.h and global_data.h.

Signed-off-by: Rasmus Villemoes ravi@prevas.dk

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

include/watchdog.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/watchdog.h b/include/watchdog.h index d1956fafca1..0149b44d077 100644 --- a/include/watchdog.h +++ b/include/watchdog.h @@ -10,7 +10,7 @@ #ifndef _WATCHDOG_H_ #define _WATCHDOG_H_

-#include <cyclic.h> +#include <u-boot/schedule.h> // to be removed later

/*

• Reset the watchdog timer, always returns 0

Viele Grüße, Stefan Roese

On 10/3/24 23:27, Rasmus Villemoes wrote:

This TU currently relies on getting a declaration of schedule() through some nested include. Include the proper header directly.

Signed-off-by: Rasmus Villemoes ravi@prevas.dk

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

drivers/i2c/rz_riic.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/rz_riic.c b/drivers/i2c/rz_riic.c index 5f3f8d1b24b..f292c824362 100644 --- a/drivers/i2c/rz_riic.c +++ b/drivers/i2c/rz_riic.c @@ -14,6 +14,7 @@ #include <linux/bitops.h> #include <linux/delay.h> #include <reset.h> +#include <u-boot/schedule.h> #include <wait_bit.h>

#define RIIC_ICCR1 0x00

Viele Grüße, Stefan Roese

On Thu, 3 Oct 2024 at 15:30, Rasmus Villemoes ravi@prevas.dk wrote:

These TUs currently rely on getting a declaration of schedule() through some nested include. Include the proper header directly.

Signed-off-by: Rasmus Villemoes ravi@prevas.dk

drivers/ddr/altera/sdram_n5x.c | 1 + drivers/ddr/altera/sdram_soc64.c | 1 + 2 files changed, 2 insertions(+)

Reviewed-by: Simon Glass sjg@chromium.org

diff --git a/drivers/ddr/altera/sdram_n5x.c b/drivers/ddr/altera/sdram_n5x.c index db09986f64b..d1fc93b6bdd 100644 --- a/drivers/ddr/altera/sdram_n5x.c +++ b/drivers/ddr/altera/sdram_n5x.c @@ -22,6 +22,7 @@ #include <asm/io.h> #include <linux/err.h> #include <linux/sizes.h> +#include <u-boot/schedule.h>

DECLARE_GLOBAL_DATA_PTR;

diff --git a/drivers/ddr/altera/sdram_soc64.c b/drivers/ddr/altera/sdram_soc64.c index 9e57c2ecfa4..10a8e64af3d 100644 --- a/drivers/ddr/altera/sdram_soc64.c +++ b/drivers/ddr/altera/sdram_soc64.c @@ -24,6 +24,7 @@ #include <asm/io.h> #include <dm/device_compat.h> #include <linux/sizes.h> +#include <u-boot/schedule.h>

#define PGTABLE_OFF 0x4000

-- 2.46.2

On 10/3/24 23:28, Rasmus Villemoes wrote:

This TU currently relies on getting a declaration of schedule() through some nested include. Include the proper header directly.

Signed-off-by: Rasmus Villemoes ravi@prevas.dk

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

boot/cedit.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/boot/cedit.c b/boot/cedit.c index c29a2be14ce..ff0efda3a7b 100644 --- a/boot/cedit.c +++ b/boot/cedit.c @@ -20,6 +20,7 @@ #include <video.h> #include <linux/delay.h> #include "scene_internal.h" +#include <u-boot/schedule.h>

enum { CMOS_MAX_BITS = 2048,

Viele Grüße, Stefan Roese