| Message ID | 20231119144700.631322-2-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
| Series | video: Improve syncing performance with cyclic | expand |
On Sun, Nov 19, 2023 at 07:46:39AM -0700, Simon Glass wrote: > The cyclic subsystem is currently enabled in all build phases or none. > So far it doesn't have any purpose within SPL builds, so adjust the > rules to prevent it being built in that case. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > common/Makefile | 2 +- > include/cyclic.h | 6 ++++-- > 2 files changed, 5 insertions(+), 3 deletions(-) > > 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 This is fine, but I suspect we're missing SPL_CYCLIC as a symbol and SPL_WDT should be select'ing that, same as "WDT". > 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> Is this really needed? What's blowing up since we should have -include .../linux/kconfig.h cover this.
Hi Tom, On Sun, 19 Nov 2023 at 08:59, Tom Rini <trini@konsulko.com> wrote: > > On Sun, Nov 19, 2023 at 07:46:39AM -0700, Simon Glass wrote: > > > The cyclic subsystem is currently enabled in all build phases or none. > > So far it doesn't have any purpose within SPL builds, so adjust the > > rules to prevent it being built in that case. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > common/Makefile | 2 +- > > include/cyclic.h | 6 ++++-- > > 2 files changed, 5 insertions(+), 3 deletions(-) > > > > 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 > > This is fine, but I suspect we're missing SPL_CYCLIC as a symbol and > SPL_WDT should be select'ing that, same as "WDT". Ooops I had assumed that it wasn't used in SPL but of course watchdog is completely migrated to it now...I will add a symbol. BTW the watchdog Kconfig seems a little confusiing. There is only one board using WATCHDOG without WDT and only 3 using HW_WATCHDOG Setfan, I wonder if some more clean-up could be done? > > > 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> > > Is this really needed? What's blowing up since we should have -include > .../linux/kconfig.h cover this. It is the CONFIG_IS_ENABLED() when building. We have kconfig.h in a few other places (used by tools) too. In file included from include/watchdog.h:13, from tools/../lib/sha1.c:25, from tools/generated/lib/sha1.c:1: include/cyclic.h:47:22: error: missing binary operator before token "(" 47 | #if CONFIG_IS_ENABLED(CYCLIC) | ^ Regards, Simon
On Sun, Nov 19, 2023 at 11:23:41AM -0700, Simon Glass wrote: > Hi Tom, > > On Sun, 19 Nov 2023 at 08:59, Tom Rini <trini@konsulko.com> wrote: > > > > On Sun, Nov 19, 2023 at 07:46:39AM -0700, Simon Glass wrote: [snip] > > > 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> > > > > Is this really needed? What's blowing up since we should have -include > > .../linux/kconfig.h cover this. > > It is the CONFIG_IS_ENABLED() when building. We have kconfig.h in a > few other places (used by tools) too. > > In file included from include/watchdog.h:13, > from tools/../lib/sha1.c:25, > from tools/generated/lib/sha1.c:1: > include/cyclic.h:47:22: error: missing binary operator before token "(" > 47 | #if CONFIG_IS_ENABLED(CYCLIC) > | ^ OK, for this we need to cleanup lib/sha*.c include directives. I'm tackling that now.
Hi Simon, On 11/19/23 19:23, Simon Glass wrote: > Hi Tom, > > On Sun, 19 Nov 2023 at 08:59, Tom Rini <trini@konsulko.com> wrote: >> >> On Sun, Nov 19, 2023 at 07:46:39AM -0700, Simon Glass wrote: >> >>> The cyclic subsystem is currently enabled in all build phases or none. >>> So far it doesn't have any purpose within SPL builds, so adjust the >>> rules to prevent it being built in that case. >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> --- >>> >>> common/Makefile | 2 +- >>> include/cyclic.h | 6 ++++-- >>> 2 files changed, 5 insertions(+), 3 deletions(-) >>> >>> 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 >> >> This is fine, but I suspect we're missing SPL_CYCLIC as a symbol and >> SPL_WDT should be select'ing that, same as "WDT". > > Ooops I had assumed that it wasn't used in SPL but of course watchdog > is completely migrated to it now...I will add a symbol. > > BTW the watchdog Kconfig seems a little confusiing. There is only one > board using WATCHDOG without WDT and only 3 using HW_WATCHDOG > > Setfan, I wonder if some more clean-up could be done? I fully agree that the watchdog Kconfig options are confusing. Perhaps I can find some time in the Holidays to try to clean this up and finally get rid of this HW_WATCHDOG stuff completely. Thanks, Stefan >> >>> 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> >> >> Is this really needed? What's blowing up since we should have -include >> .../linux/kconfig.h cover this. > > It is the CONFIG_IS_ENABLED() when building. We have kconfig.h in a > few other places (used by tools) too. > > In file included from include/watchdog.h:13, > from tools/../lib/sha1.c:25, > from tools/generated/lib/sha1.c:1: > include/cyclic.h:47:22: error: missing binary operator before token "(" > 47 | #if CONFIG_IS_ENABLED(CYCLIC) > | ^ > > Regards, > Simon Viele Grüße, Stefan Roese
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/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 in all build phases or none. So far it doesn't have any purpose within SPL builds, so adjust the rules to prevent it being built in that case. Signed-off-by: Simon Glass <sjg@chromium.org> --- common/Makefile | 2 +- include/cyclic.h | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-)