| Message ID | 20231202153400.537050-2-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | video: Improve syncing performance with cyclic | expand |
On 12/2/23 16:33, Simon Glass wrote: > The cyclic subsystem is currently enabled either in all build phases > or none. For tools this should not be enabled, but since lib/shc256.c > and other files include watchdog.h in the host build, we must make > sure that it is not enabled there. > > Add an SPL symbol so that there is more control of this. > > Add an include into cyclic.h so that tools can include this file. > > Also add the kconfig.h header so that CONFIG_IS_ENABLED() works. We > could avoid this for now by moving the location of the watchdog.h > inclusion to outside the USE_HOSTCC area. But at some point the #ifdefs > from these files will likely be removed, so there is no benefit in > going that way. > > Signed-off-by: Simon Glass <sjg@chromium.org> Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan > --- > > Changes in v2: > - Add an SPL_CYCLIC symbol > - Add a lot more explanation about the header files > > common/Kconfig | 8 ++++++++ > common/Makefile | 2 +- > drivers/watchdog/Kconfig | 1 + > include/asm-generic/global_data.h | 2 +- > include/cyclic.h | 6 ++++-- > 5 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/common/Kconfig b/common/Kconfig > index 0283701f1d05..5906a4af7c33 100644 > --- a/common/Kconfig > +++ b/common/Kconfig > @@ -626,6 +626,14 @@ config CYCLIC > > if CYCLIC > > +config SPL_CYCLIC > + bool "General-purpose cyclic execution mechanism (SPL)" > + help > + This enables a general-purpose cyclic execution infrastructure in SPL, > + to allow "small" (run-time wise) functions to be executed at > + a specified frequency. Things like LED blinking or watchdog > + triggering are examples for such tasks. > + > config CYCLIC_MAX_CPU_TIME_US > int "Sets the max allowed time for a cyclic function in us" > default 1000 > diff --git a/common/Makefile b/common/Makefile > index 1495436d5d45..27443863bf9b 100644 > --- a/common/Makefile > +++ b/common/Makefile > @@ -77,7 +77,7 @@ obj-$(CONFIG_CROS_EC) += cros_ec.o > obj-y += dlmalloc.o > obj-$(CONFIG_$(SPL_TPL_)SYS_MALLOC_F) += malloc_simple.o > > -obj-$(CONFIG_CYCLIC) += cyclic.o > +obj-$(CONFIG_$(SPL_TPL_)CYCLIC) += cyclic.o > obj-$(CONFIG_$(SPL_TPL_)EVENT) += event.o > > obj-$(CONFIG_$(SPL_TPL_)HASH) += hash.o > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 07fc4940e918..378435c55cc7 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -395,6 +395,7 @@ config WDT_ARM_SMC > config SPL_WDT > bool "Enable driver model for watchdog timer drivers in SPL" > depends on SPL_DM > + select SPL_CYCLIC if CYCLIC > help > Enable driver model for watchdog timer in SPL. > This is similar to CONFIG_WDT in U-Boot. > diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h > index e8c6412e3f8d..77f11a4383c9 100644 > --- a/include/asm-generic/global_data.h > +++ b/include/asm-generic/global_data.h > @@ -485,7 +485,7 @@ struct global_data { > */ > struct event_state event_state; > #endif > -#ifdef CONFIG_CYCLIC > +#if CONFIG_IS_ENABLED(CYCLIC) > /** > * @cyclic_list: list of registered cyclic functions > */ > diff --git a/include/cyclic.h b/include/cyclic.h > index 44ad3cb6b803..d3b368dd90df 100644 > --- a/include/cyclic.h > +++ b/include/cyclic.h > @@ -11,6 +11,7 @@ > #ifndef __cyclic_h > #define __cyclic_h > > +#include <linux/kconfig.h> > #include <linux/list.h> > #include <asm/types.h> > > @@ -44,7 +45,8 @@ struct cyclic_info { > /** Function type for cyclic functions */ > typedef void (*cyclic_func_t)(void *ctx); > > -#if defined(CONFIG_CYCLIC) > +#if CONFIG_IS_ENABLED(CYCLIC) > + > /** > * cyclic_register - Register a new cyclic function > * > @@ -122,6 +124,6 @@ static inline int cyclic_unregister_all(void) > { > return 0; > } > -#endif > +#endif /* CYCLIC */ > > #endif Viele Grüße, Stefan Roese
Hi Simon, On 02/12/23 21:03, Simon Glass wrote: > The cyclic subsystem is currently enabled either in all build phases > or none. For tools this should not be enabled, but since lib/shc256.c > and other files include watchdog.h in the host build, we must make > sure that it is not enabled there. > > Add an SPL symbol so that there is more control of this. > > Add an include into cyclic.h so that tools can include this file. > > Also add the kconfig.h header so that CONFIG_IS_ENABLED() works. We > could avoid this for now by moving the location of the watchdog.h > inclusion to outside the USE_HOSTCC area. But at some point the #ifdefs > from these files will likely be removed, so there is no benefit in > going that way. > > Signed-off-by: Simon Glass <sjg@chromium.org> Reviewed-by: Devarsh Thakkar <devarsht@ti.com> Regards Devarsh > --- > > Changes in v2: > - Add an SPL_CYCLIC symbol > - Add a lot more explanation about the header files > > common/Kconfig | 8 ++++++++ > common/Makefile | 2 +- > drivers/watchdog/Kconfig | 1 + > include/asm-generic/global_data.h | 2 +- > include/cyclic.h | 6 ++++-- > 5 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/common/Kconfig b/common/Kconfig > index 0283701f1d05..5906a4af7c33 100644 > --- a/common/Kconfig > +++ b/common/Kconfig > @@ -626,6 +626,14 @@ config CYCLIC > > if CYCLIC > > +config SPL_CYCLIC > + bool "General-purpose cyclic execution mechanism (SPL)" > + help > + This enables a general-purpose cyclic execution infrastructure in SPL, > + to allow "small" (run-time wise) functions to be executed at > + a specified frequency. Things like LED blinking or watchdog > + triggering are examples for such tasks. > + > config CYCLIC_MAX_CPU_TIME_US > int "Sets the max allowed time for a cyclic function in us" > default 1000 > diff --git a/common/Makefile b/common/Makefile > index 1495436d5d45..27443863bf9b 100644 > --- a/common/Makefile > +++ b/common/Makefile > @@ -77,7 +77,7 @@ obj-$(CONFIG_CROS_EC) += cros_ec.o > obj-y += dlmalloc.o > obj-$(CONFIG_$(SPL_TPL_)SYS_MALLOC_F) += malloc_simple.o > > -obj-$(CONFIG_CYCLIC) += cyclic.o > +obj-$(CONFIG_$(SPL_TPL_)CYCLIC) += cyclic.o > obj-$(CONFIG_$(SPL_TPL_)EVENT) += event.o > > obj-$(CONFIG_$(SPL_TPL_)HASH) += hash.o > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 07fc4940e918..378435c55cc7 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -395,6 +395,7 @@ config WDT_ARM_SMC > config SPL_WDT > bool "Enable driver model for watchdog timer drivers in SPL" > depends on SPL_DM > + select SPL_CYCLIC if CYCLIC > help > Enable driver model for watchdog timer in SPL. > This is similar to CONFIG_WDT in U-Boot. > diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h > index e8c6412e3f8d..77f11a4383c9 100644 > --- a/include/asm-generic/global_data.h > +++ b/include/asm-generic/global_data.h > @@ -485,7 +485,7 @@ struct global_data { > */ > struct event_state event_state; > #endif > -#ifdef CONFIG_CYCLIC > +#if CONFIG_IS_ENABLED(CYCLIC) > /** > * @cyclic_list: list of registered cyclic functions > */ > diff --git a/include/cyclic.h b/include/cyclic.h > index 44ad3cb6b803..d3b368dd90df 100644 > --- a/include/cyclic.h > +++ b/include/cyclic.h > @@ -11,6 +11,7 @@ > #ifndef __cyclic_h > #define __cyclic_h > > +#include <linux/kconfig.h> > #include <linux/list.h> > #include <asm/types.h> > > @@ -44,7 +45,8 @@ struct cyclic_info { > /** Function type for cyclic functions */ > typedef void (*cyclic_func_t)(void *ctx); > > -#if defined(CONFIG_CYCLIC) > +#if CONFIG_IS_ENABLED(CYCLIC) > + > /** > * cyclic_register - Register a new cyclic function > * > @@ -122,6 +124,6 @@ static inline int cyclic_unregister_all(void) > { > return 0; > } > -#endif > +#endif /* CYCLIC */ > > #endif
On Sat, Dec 02, 2023 at 08:33:48AM -0700, Simon Glass wrote: > The cyclic subsystem is currently enabled either in all build phases > or none. For tools this should not be enabled, but since lib/shc256.c > and other files include watchdog.h in the host build, we must make > sure that it is not enabled there. This part is what I see as the Wrong Direction. There's some code we really need to share with our user space tools, in order to not copy/paste the same code. In turn, this code must be as user-space friendly as possible. Maybe even we re-factor things a little more, if needed, so that we _just_ have the library functions in common files, and u-boot or user space only files have the make use of logic. I don't feel bad about tools/ needing: void sha256_csum_wd(const unsigned char *input, unsigned int ilen, unsigned char *output, unsigned int chunk_sz) { sha256_context ctx; sha256_starts(&ctx); sha256_update(&ctx, input, ilen); sha256_finish(&ctx, output); } (and so on for other algos) as a duplicate bit of code. Much less so than I do about adding <linux/kconfig.h> to a direct include list (since we should never be doing that) so that later on we can if (IS_ENABLED(..)) the existing code.
Hi Tom, On Sat, 9 Dec 2023 at 08:48, Tom Rini <trini@konsulko.com> wrote: > > On Sat, Dec 02, 2023 at 08:33:48AM -0700, Simon Glass wrote: > > > The cyclic subsystem is currently enabled either in all build phases > > or none. For tools this should not be enabled, but since lib/shc256.c > > and other files include watchdog.h in the host build, we must make > > sure that it is not enabled there. > > This part is what I see as the Wrong Direction. There's some code we > really need to share with our user space tools, in order to not > copy/paste the same code. In turn, this code must be as user-space > friendly as possible. Maybe even we re-factor things a little more, if > needed, so that we _just_ have the library functions in common files, > and u-boot or user space only files have the make use of logic. I don't > feel bad about tools/ needing: > void sha256_csum_wd(const unsigned char *input, unsigned int ilen, > unsigned char *output, unsigned int chunk_sz) > { > sha256_context ctx; > sha256_starts(&ctx); > sha256_update(&ctx, input, ilen); > sha256_finish(&ctx, output); > } > > (and so on for other algos) as a duplicate bit of code. Much less so > than I do about adding <linux/kconfig.h> to a direct include list (since > we should never be doing that) so that later on we can if > (IS_ENABLED(..)) the existing code. Bear in mind that we have the CONFIG_TOOLS_... options entirely to deal with the need for tools to enable features in common code. This SHA thing is a very small part of the code, compared to common code in boot/ for example. So is this really a win? Regards, Simon
On Wed, Dec 13, 2023 at 12:50:02PM -0700, Simon Glass wrote: > Hi Tom, > > On Sat, 9 Dec 2023 at 08:48, Tom Rini <trini@konsulko.com> wrote: > > > > On Sat, Dec 02, 2023 at 08:33:48AM -0700, Simon Glass wrote: > > > > > The cyclic subsystem is currently enabled either in all build phases > > > or none. For tools this should not be enabled, but since lib/shc256.c > > > and other files include watchdog.h in the host build, we must make > > > sure that it is not enabled there. > > > > This part is what I see as the Wrong Direction. There's some code we > > really need to share with our user space tools, in order to not > > copy/paste the same code. In turn, this code must be as user-space > > friendly as possible. Maybe even we re-factor things a little more, if > > needed, so that we _just_ have the library functions in common files, > > and u-boot or user space only files have the make use of logic. I don't > > feel bad about tools/ needing: > > void sha256_csum_wd(const unsigned char *input, unsigned int ilen, > > unsigned char *output, unsigned int chunk_sz) > > { > > sha256_context ctx; > > sha256_starts(&ctx); > > sha256_update(&ctx, input, ilen); > > sha256_finish(&ctx, output); > > } > > > > (and so on for other algos) as a duplicate bit of code. Much less so > > than I do about adding <linux/kconfig.h> to a direct include list (since > > we should never be doing that) so that later on we can if > > (IS_ENABLED(..)) the existing code. > > Bear in mind that we have the CONFIG_TOOLS_... options entirely to > deal with the need for tools to enable features in common code. This > SHA thing is a very small part of the code, compared to common code in > boot/ for example. > > So is this really a win? I don't follow you here, sorry. We share lib/sha*.c with host tools for generic mkimage functionality. I'm fine with continuing to use USE_HOSTCC as the guard for U-Boot / userspace here. And I don't think switching these files from: #if defined(CONFIG_HW_WATCHDOG) || defined(CONFIG_WATCHDOG) to: if (IS_ENABLED(CONFIG_HW_WATCHDOG) || IS_ENABLED(CONFIG_WATCHDOG)) { improves readability of the code.
Hi Tom, On Wed, 13 Dec 2023 at 13:42, Tom Rini <trini@konsulko.com> wrote: > > On Wed, Dec 13, 2023 at 12:50:02PM -0700, Simon Glass wrote: > > Hi Tom, > > > > On Sat, 9 Dec 2023 at 08:48, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Sat, Dec 02, 2023 at 08:33:48AM -0700, Simon Glass wrote: > > > > > > > The cyclic subsystem is currently enabled either in all build phases > > > > or none. For tools this should not be enabled, but since lib/shc256.c > > > > and other files include watchdog.h in the host build, we must make > > > > sure that it is not enabled there. > > > > > > This part is what I see as the Wrong Direction. There's some code we > > > really need to share with our user space tools, in order to not > > > copy/paste the same code. In turn, this code must be as user-space > > > friendly as possible. Maybe even we re-factor things a little more, if > > > needed, so that we _just_ have the library functions in common files, > > > and u-boot or user space only files have the make use of logic. I don't > > > feel bad about tools/ needing: > > > void sha256_csum_wd(const unsigned char *input, unsigned int ilen, > > > unsigned char *output, unsigned int chunk_sz) > > > { > > > sha256_context ctx; > > > sha256_starts(&ctx); > > > sha256_update(&ctx, input, ilen); > > > sha256_finish(&ctx, output); > > > } > > > > > > (and so on for other algos) as a duplicate bit of code. Much less so > > > than I do about adding <linux/kconfig.h> to a direct include list (since > > > we should never be doing that) so that later on we can if > > > (IS_ENABLED(..)) the existing code. > > > > Bear in mind that we have the CONFIG_TOOLS_... options entirely to > > deal with the need for tools to enable features in common code. This > > SHA thing is a very small part of the code, compared to common code in > > boot/ for example. > > > > So is this really a win? > > I don't follow you here, sorry. I mean that we share a lot of code already, code which contains CONFIG options. So does it matter avoiding adding one more? > We share lib/sha*.c with host tools for > generic mkimage functionality. I'm fine with continuing to use > USE_HOSTCC as the guard for U-Boot / userspace here. And I don't think > switching these files from: > #if defined(CONFIG_HW_WATCHDOG) || defined(CONFIG_WATCHDOG) > to: > if (IS_ENABLED(CONFIG_HW_WATCHDOG) || IS_ENABLED(CONFIG_WATCHDOG)) { > improves readability of the code. Perhaps I am being too hard on the #ifdefs. BTW we have a tools_build() function now. Regards, Simon
On Wed, Dec 13, 2023 at 01:51:00PM -0700, Simon Glass wrote: > Hi Tom, > > On Wed, 13 Dec 2023 at 13:42, Tom Rini <trini@konsulko.com> wrote: > > > > On Wed, Dec 13, 2023 at 12:50:02PM -0700, Simon Glass wrote: > > > Hi Tom, > > > > > > On Sat, 9 Dec 2023 at 08:48, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > On Sat, Dec 02, 2023 at 08:33:48AM -0700, Simon Glass wrote: > > > > > > > > > The cyclic subsystem is currently enabled either in all build phases > > > > > or none. For tools this should not be enabled, but since lib/shc256.c > > > > > and other files include watchdog.h in the host build, we must make > > > > > sure that it is not enabled there. > > > > > > > > This part is what I see as the Wrong Direction. There's some code we > > > > really need to share with our user space tools, in order to not > > > > copy/paste the same code. In turn, this code must be as user-space > > > > friendly as possible. Maybe even we re-factor things a little more, if > > > > needed, so that we _just_ have the library functions in common files, > > > > and u-boot or user space only files have the make use of logic. I don't > > > > feel bad about tools/ needing: > > > > void sha256_csum_wd(const unsigned char *input, unsigned int ilen, > > > > unsigned char *output, unsigned int chunk_sz) > > > > { > > > > sha256_context ctx; > > > > sha256_starts(&ctx); > > > > sha256_update(&ctx, input, ilen); > > > > sha256_finish(&ctx, output); > > > > } > > > > > > > > (and so on for other algos) as a duplicate bit of code. Much less so > > > > than I do about adding <linux/kconfig.h> to a direct include list (since > > > > we should never be doing that) so that later on we can if > > > > (IS_ENABLED(..)) the existing code. > > > > > > Bear in mind that we have the CONFIG_TOOLS_... options entirely to > > > deal with the need for tools to enable features in common code. This > > > SHA thing is a very small part of the code, compared to common code in > > > boot/ for example. > > > > > > So is this really a win? > > > > I don't follow you here, sorry. > > I mean that we share a lot of code already, code which contains CONFIG > options. So does it matter avoiding adding one more? It's adding <linux/kconfig.h> to files, we must not do that. And we don't need to if we don't have <watchdog.h> (and that in turn, cyclic.h) included outside of USE_HOSTCC :)
Hi Tom, On Wed, 13 Dec 2023 at 13:59, Tom Rini <trini@konsulko.com> wrote: > > On Wed, Dec 13, 2023 at 01:51:00PM -0700, Simon Glass wrote: > > Hi Tom, > > > > On Wed, 13 Dec 2023 at 13:42, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Wed, Dec 13, 2023 at 12:50:02PM -0700, Simon Glass wrote: > > > > Hi Tom, > > > > > > > > On Sat, 9 Dec 2023 at 08:48, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > > > On Sat, Dec 02, 2023 at 08:33:48AM -0700, Simon Glass wrote: > > > > > > > > > > > The cyclic subsystem is currently enabled either in all build phases > > > > > > or none. For tools this should not be enabled, but since lib/shc256.c > > > > > > and other files include watchdog.h in the host build, we must make > > > > > > sure that it is not enabled there. > > > > > > > > > > This part is what I see as the Wrong Direction. There's some code we > > > > > really need to share with our user space tools, in order to not > > > > > copy/paste the same code. In turn, this code must be as user-space > > > > > friendly as possible. Maybe even we re-factor things a little more, if > > > > > needed, so that we _just_ have the library functions in common files, > > > > > and u-boot or user space only files have the make use of logic. I don't > > > > > feel bad about tools/ needing: > > > > > void sha256_csum_wd(const unsigned char *input, unsigned int ilen, > > > > > unsigned char *output, unsigned int chunk_sz) > > > > > { > > > > > sha256_context ctx; > > > > > sha256_starts(&ctx); > > > > > sha256_update(&ctx, input, ilen); > > > > > sha256_finish(&ctx, output); > > > > > } > > > > > > > > > > (and so on for other algos) as a duplicate bit of code. Much less so > > > > > than I do about adding <linux/kconfig.h> to a direct include list (since > > > > > we should never be doing that) so that later on we can if > > > > > (IS_ENABLED(..)) the existing code. > > > > > > > > Bear in mind that we have the CONFIG_TOOLS_... options entirely to > > > > deal with the need for tools to enable features in common code. This > > > > SHA thing is a very small part of the code, compared to common code in > > > > boot/ for example. > > > > > > > > So is this really a win? > > > > > > I don't follow you here, sorry. > > > > I mean that we share a lot of code already, code which contains CONFIG > > options. So does it matter avoiding adding one more? > > It's adding <linux/kconfig.h> to files, we must not do that. And we > don't need to if we don't have <watchdog.h> (and that in turn, > cyclic.h) included outside of USE_HOSTCC :) $ git grep kconfig.h .azure-pipelines.yml: :^include/linux/kconfig.h :^tools/ && exit 1 || exit 0 .gitlab-ci.yml: :^include/linux/kconfig.h :^tools/ && exit 1 || exit 0 Makefile: -include $(srctree)/include/linux/kconfig.h Makefile: -include linux/kconfig.h -include include/config.h \ arch/arm/include/asm/arch-fsl-layerscape/config.h:#include <linux/kconfig.h> arch/arm/mach-rockchip/tpl.c:#include <linux/kconfig.h> arch/arm/mach-sunxi/dram_sun50i_h6.c:#include <linux/kconfig.h> arch/arm/mach-sunxi/dram_sun50i_h616.c:#include <linux/kconfig.h> arch/arm/mach-sunxi/dram_sunxi_dw.c:#include <linux/kconfig.h> boot/image-fit.c:#include <linux/kconfig.h> boot/image.c:#include <linux/kconfig.h> These are the sorts of ones which are a real pain to remove. common/hash.c:#include <linux/kconfig.h> drivers/timer/dw-apb-timer.c:#include <linux/kconfig.h> env/embedded.c:#include <linux/kconfig.h> include/bootstage.h:#include <linux/kconfig.h> include/configs/at91-sama5_common.h:#include <linux/kconfig.h> include/configs/tqma6.h:#include <linux/kconfig.h> include/env_internal.h:#include <linux/kconfig.h> include/hash.h:#include <linux/kconfig.h> include/image.h:#include <linux/kconfig.h> include/u-boot/ecdsa.h:#include <linux/kconfig.h> lib/rsa/rsa-verify.c:#include <linux/kconfig.h> scripts/Makefile.autoconf: -include $(srctree)/include/linux/kconfig.h scripts/Makefile.autoconf: echo \#include \<linux/kconfig.h\>; \ test/dm/scmi.c:#include <linux/kconfig.h> test/lib/kconfig.c: * Test of linux/kconfig.h macros test/lib/kconfig_spl.c: * Test of linux/kconfig.h macros for SPL tools/binman/test/blob_syms.c:#include <linux/kconfig.h> tools/binman/test/u_boot_binman_syms.c:#include <linux/kconfig.h> tools/binman/test/u_boot_binman_syms_size.c:#include <linux/kconfig.h> tools/env/fw_env_private.h:#include <linux/kconfig.h> tools/envcrc.c:#include <linux/kconfig.h> tools/mkeficapsule.c:#include <linux/kconfig.h> tools/printinitialenv.c:#include <linux/kconfig.h> I don't really mind what we do with sha, and it seems I am a bit too rabid on the anti-#ifdef thing compared to others. Regards, Simon
diff --git a/common/Kconfig b/common/Kconfig index 0283701f1d05..5906a4af7c33 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -626,6 +626,14 @@ config CYCLIC if CYCLIC +config SPL_CYCLIC + bool "General-purpose cyclic execution mechanism (SPL)" + help + This enables a general-purpose cyclic execution infrastructure in SPL, + to allow "small" (run-time wise) functions to be executed at + a specified frequency. Things like LED blinking or watchdog + triggering are examples for such tasks. + config CYCLIC_MAX_CPU_TIME_US int "Sets the max allowed time for a cyclic function in us" default 1000 diff --git a/common/Makefile b/common/Makefile index 1495436d5d45..27443863bf9b 100644 --- a/common/Makefile +++ b/common/Makefile @@ -77,7 +77,7 @@ obj-$(CONFIG_CROS_EC) += cros_ec.o obj-y += dlmalloc.o obj-$(CONFIG_$(SPL_TPL_)SYS_MALLOC_F) += malloc_simple.o -obj-$(CONFIG_CYCLIC) += cyclic.o +obj-$(CONFIG_$(SPL_TPL_)CYCLIC) += cyclic.o obj-$(CONFIG_$(SPL_TPL_)EVENT) += event.o obj-$(CONFIG_$(SPL_TPL_)HASH) += hash.o diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 07fc4940e918..378435c55cc7 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -395,6 +395,7 @@ config WDT_ARM_SMC config SPL_WDT bool "Enable driver model for watchdog timer drivers in SPL" depends on SPL_DM + select SPL_CYCLIC if CYCLIC help Enable driver model for watchdog timer in SPL. This is similar to CONFIG_WDT in U-Boot. diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index e8c6412e3f8d..77f11a4383c9 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -485,7 +485,7 @@ struct global_data { */ struct event_state event_state; #endif -#ifdef CONFIG_CYCLIC +#if CONFIG_IS_ENABLED(CYCLIC) /** * @cyclic_list: list of registered cyclic functions */ diff --git a/include/cyclic.h b/include/cyclic.h index 44ad3cb6b803..d3b368dd90df 100644 --- a/include/cyclic.h +++ b/include/cyclic.h @@ -11,6 +11,7 @@ #ifndef __cyclic_h #define __cyclic_h +#include <linux/kconfig.h> #include <linux/list.h> #include <asm/types.h> @@ -44,7 +45,8 @@ struct cyclic_info { /** Function type for cyclic functions */ typedef void (*cyclic_func_t)(void *ctx); -#if defined(CONFIG_CYCLIC) +#if CONFIG_IS_ENABLED(CYCLIC) + /** * cyclic_register - Register a new cyclic function * @@ -122,6 +124,6 @@ static inline int cyclic_unregister_all(void) { return 0; } -#endif +#endif /* CYCLIC */ #endif
The cyclic subsystem is currently enabled either in all build phases or none. For tools this should not be enabled, but since lib/shc256.c and other files include watchdog.h in the host build, we must make sure that it is not enabled there. Add an SPL symbol so that there is more control of this. Add an include into cyclic.h so that tools can include this file. Also add the kconfig.h header so that CONFIG_IS_ENABLED() works. We could avoid this for now by moving the location of the watchdog.h inclusion to outside the USE_HOSTCC area. But at some point the #ifdefs from these files will likely be removed, so there is no benefit in going that way. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v2: - Add an SPL_CYCLIC symbol - Add a lot more explanation about the header files common/Kconfig | 8 ++++++++ common/Makefile | 2 +- drivers/watchdog/Kconfig | 1 + include/asm-generic/global_data.h | 2 +- include/cyclic.h | 6 ++++-- 5 files changed, 15 insertions(+), 4 deletions(-)