Hi Simon,
On 15.08.22 19:37, Simon Glass wrote:
Hi Stefan,
On Mon, 15 Aug 2022 at 10:16, Stefan Roese sr@denx.de wrote:
Hi Simon,
On 05.08.22 18:48, Simon Glass wrote:
Hi Stefan,
On Fri, 5 Aug 2022 at 08:26, Stefan Roese sr@denx.de wrote:
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
v3:
- No change
v2:
No change
fs/cramfs/uncompress.c | 2 +- include/watchdog.h | 23 ++++++++++++++++++++--- 2 files changed, 21 insertions(+), 4 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
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;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(); \Doesn't this create two function calls from every reset site? I worry about code size.
Good point. Even though the world build did not trigger any new problems with oversized images.
I suggest creating a new function like check_watchdog() which you can define (in the C file) with IS_ENABLED() as either empty, calling watchdog_reset() and/or calling cyclic_run(). LTO should help here.
I tried a bit to get clean up this ugly #ifdef construct. And move this as you suggested into a C file. Just now I noticed, that we don't have a matching C file, which is compiled in all cases. wdt-uclass.c and cyclic.c are both only compiled when actually enabled in Kconfig.
So do you have some other / better idea on how to improve this?
BTW: Moving the watchdog integration into the cyclic infrastructure in some follow-up patches will make all this much cleaner AFAICT.
If you are going to do this soon, then I suggest not working about it too much.
But you could create wdt_common.c or similar. It should be OK to always compile it since the code will be dropped if nothing calls it.
Yes, this would be one potential intermediate "solution". I'll try to work on the WDT migration to cyclic IF soon and will keep the code in this header as-is for now.
Thanks, Stefan