[PATCH v3 3/8] cyclic: Integrate cyclic infrastructure into WATCHDOG_RESET

[Simon Glass]

Hi Stefan,

On Mon, 15 Aug 2022 at 10:16, Stefan Roese <sr at 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 at 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 at 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 at 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;
> >>   #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