Hi Stefan, On Mon, 15 Aug 2022 at 10:16, Stefan Roese <[email protected]> wrote: > > Hi Simon, > > On 05.08.22 18:48, Simon Glass wrote: > > Hi Stefan, > > > > On Fri, 5 Aug 2022 at 08:26, Stefan Roese <[email protected]> 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 <[email protected]> > >> --- > >> 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 <[email protected]> > > > >> > >> 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(); \ > > > > 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. Regards, Simon
