| Message ID | 20220805142610.375427-4-sr@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Add support for cyclic function execution infrastruture | expand |
Hi Stefan, On Fri, 5 Aug 2022 at 08:26, Stefan Roese <sr@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@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@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
Hi Simon, On 05.08.22 18:48, Simon Glass wrote: > Hi Stefan, > > On Fri, 5 Aug 2022 at 08:26, Stefan Roese <sr@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@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@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
Hi Stefan, On Mon, 15 Aug 2022 at 10:16, Stefan Roese <sr@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@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@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@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
Hi Simon, On 15.08.22 19:37, Simon Glass wrote: > Hi Stefan, > > On Mon, 15 Aug 2022 at 10:16, Stefan Roese <sr@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@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@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@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
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(); \ + } #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. */
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@denx.de> --- v3: - No change v2: - No change fs/cramfs/uncompress.c | 2 +- include/watchdog.h | 23 ++++++++++++++++++++--- 2 files changed, 21 insertions(+), 4 deletions(-)