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

[Simon Glass]

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. 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.

> +                               }
>                         #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
>

Regards,
Simon