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

[Stefan Roese]

Hi Simon,

On 15.08.22 19:37, Simon Glass wrote:
> 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.

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