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

[Stefan Roese]

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.

Thanks,
Stefan

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

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de