[PATCH v2 1/5] cyclic: Add a symbol for SPL

[PATCH v2 1/5] cyclic: Add a symbol for SPL

[Simon Glass]

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(-)

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
-- 
2.43.0.rc1.413.gea7ed67945-goog

Re: [PATCH v2 1/5] cyclic: Add a symbol for SPL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1285 bytes --]

On Mon, Nov 20, 2023 at 07:09:25PM -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.
> 
> 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>
[snip]
> --- 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>

As I said, we just need to fix lib/sha*.c so NAK. The only thing that
stopped me from posting that patch last night was that I went and
removed <common.h> from everything in lib/ as a follow-up and that's
taken a few more iterations.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/5] cyclic: Add a symbol for SPL

[Simon Glass]

Hi Tom,

On Tue, 21 Nov 2023 at 06:06, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Nov 20, 2023 at 07:09:25PM -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.
> >
> > 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>
> [snip]
> > --- 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>
>
> As I said, we just need to fix lib/sha*.c so NAK. The only thing that
> stopped me from posting that patch last night was that I went and
> removed <common.h> from everything in lib/ as a follow-up and that's
> taken a few more iterations.
>

Yes I had the same thought the first time and mentioned it
specifically in this version.

If you are a doing a clean-up, perhaps you could drop the #idefs in
the sha files?

Regards,
Simon

Re: [PATCH v2 1/5] cyclic: Add a symbol for SPL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1842 bytes --]

On Tue, Nov 21, 2023 at 09:20:39AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 21 Nov 2023 at 06:06, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Nov 20, 2023 at 07:09:25PM -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.
> > >
> > > 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>
> > [snip]
> > > --- 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>
> >
> > As I said, we just need to fix lib/sha*.c so NAK. The only thing that
> > stopped me from posting that patch last night was that I went and
> > removed <common.h> from everything in lib/ as a follow-up and that's
> > taken a few more iterations.
> >
> 
> Yes I had the same thought the first time and mentioned it
> specifically in this version.
> 
> If you are a doing a clean-up, perhaps you could drop the #idefs in
> the sha files?

Nope, they specifically need <cyclic.h> outside of tools builds for
schedule().

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH v2 0/5] video: Improve syncing performance with cyclic

[Simon Glass]

Now that U-Boot has a background-processing feature, it is possible to
reduce the amount of 'foreground' syncing of the display. At present
this happens quite often.

Foreground syncing blocks all other processing, sometimes for 10ms or
more. When pasting commands into U-Boot over the UART, this typically
result in characters being dropped. For example, on rpi_4 it isn't
possible to paste in more than 35 characters before things fail. This
makes updating the environment or entering long commands very painful
over the console, since text must be pasted in chunks, or the
vidconsole device must be dropped from stdout.

This series introduces background syncing, enabled by default for
boards which use video. The sync rates for foreground and background
are configurable.

With this series it is possible to paste in any amount of text to the
command line. Some sandbox-specific workarounds can now be removed and
sandbox video (./u-boot -Dl) is significantly more responsive.

This obviously increases code size, since it enables a subsystem not
normally used by default. However it only applies to boards which have
VIDEO enabled, which are presumably less worried about memory space
since the video code is fairly large.

Also it is possible to disable CMD_CYCLIC and reduce the growth to:

   aarch64: (for 1/1 boards) all +1081.0 rodata +65.0 text +1016.0
       arm: (for 1/1 boards) all +945.0 rodata +65.0 text +880.0

Without that, the increase doubles.

It is of course possible to disable CYCLIC and still use VIDEO but this
reverts to the current behaviour

Changes in v2:
- Add an SPL_CYCLIC symbol
- Add a lot more explanation about the header files
- Expand help for CONFIG_VIDEO
- Fix 'groth' and 'work-around' typos in cover letter

Simon Glass (5):
  cyclic: Add a symbol for SPL
  video: Move last_sync to private data
  video: Use cyclic to handle video sync
  sandbox: Increase cyclic CPU-time limit
  sandbox: Drop video-sync in serial driver

 common/Kconfig                    |  9 ++++++
 common/Makefile                   |  2 +-
 drivers/serial/sandbox.c          |  2 --
 drivers/video/Kconfig             | 35 +++++++++++++++++++++
 drivers/video/video-uclass.c      | 52 +++++++++++++++++++++++++------
 drivers/watchdog/Kconfig          |  1 +
 include/asm-generic/global_data.h |  2 +-
 include/cyclic.h                  |  6 ++--
 include/video.h                   |  2 ++
 9 files changed, 96 insertions(+), 15 deletions(-)

-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 1/5] cyclic: Add a symbol for SPL

[Simon Glass]

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(-)

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
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 2/5] video: Move last_sync to private data

[Simon Glass]

Rather than using a static variable, use the video device's private
data to remember when the last video sync was completed. This allows
each display to have its own sync and avoids using static data in SPL.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 drivers/video/video-uclass.c | 10 +++-------
 include/video.h              |  2 ++
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
index f743ed74c818..600e12b802d1 100644
--- a/drivers/video/video-uclass.c
+++ b/drivers/video/video-uclass.c
@@ -354,6 +354,7 @@ void video_set_default_colors(struct udevice *dev, bool invert)
 /* Flush video activity to the caches */
 int video_sync(struct udevice *vid, bool force)
 {
+	struct video_priv *priv = dev_get_uclass_priv(vid);
 	struct video_ops *ops = video_get_ops(vid);
 	int ret;
 
@@ -369,20 +370,15 @@ int video_sync(struct udevice *vid, bool force)
 	 * out whether it exists? For now, ARM is safe.
 	 */
 #if defined(CONFIG_ARM) && !CONFIG_IS_ENABLED(SYS_DCACHE_OFF)
-	struct video_priv *priv = dev_get_uclass_priv(vid);
-
 	if (priv->flush_dcache) {
 		flush_dcache_range((ulong)priv->fb,
 				   ALIGN((ulong)priv->fb + priv->fb_size,
 					 CONFIG_SYS_CACHELINE_SIZE));
 	}
 #elif defined(CONFIG_VIDEO_SANDBOX_SDL)
-	struct video_priv *priv = dev_get_uclass_priv(vid);
-	static ulong last_sync;
-
-	if (force || get_timer(last_sync) > 100) {
+	if (force || get_timer(priv->last_sync) > 100) {
 		sandbox_sdl_sync(priv->fb);
-		last_sync = get_timer(0);
+		priv->last_sync = get_timer(0);
 	}
 #endif
 	return 0;
diff --git a/include/video.h b/include/video.h
index 4d8df9baaada..4013a949983f 100644
--- a/include/video.h
+++ b/include/video.h
@@ -97,6 +97,7 @@ enum video_format {
  *		the LCD is updated
  * @fg_col_idx:	Foreground color code (bit 3 = bold, bit 0-2 = color)
  * @bg_col_idx:	Background color code (bit 3 = bold, bit 0-2 = color)
+ * @last_sync:	Monotonic time of last video sync
  */
 struct video_priv {
 	/* Things set up by the driver: */
@@ -121,6 +122,7 @@ struct video_priv {
 	bool flush_dcache;
 	u8 fg_col_idx;
 	u8 bg_col_idx;
+	ulong last_sync;
 };
 
 /**
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 3/5] video: Use cyclic to handle video sync

[Simon Glass]

At present U-Boot flushes the cache after every character written to
ths display. This makes the command-line slower, to the point that
pasting in long strings can fail.

Add a cyclic function to sync the display every 10ms. Enable this by
default.

Allow much longer times for sandbox, since the SDL display is quite
slow.

Avoid size growth if the feature is disabled by making the new init and
destroy functions dependent on CYCLIC being enabled.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Expand help for CONFIG_VIDEO

 drivers/video/Kconfig        | 35 +++++++++++++++++++++++++++
 drivers/video/video-uclass.c | 46 ++++++++++++++++++++++++++++++++----
 2 files changed, 77 insertions(+), 4 deletions(-)

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 6f319ba0d544..ccbfcbcfd824 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -7,6 +7,7 @@ menu "Graphics support"
 config VIDEO
 	bool "Enable driver model support for LCD/video"
 	depends on DM
+	imply CYCLIC
 	help
 	  This enables driver model for LCD and video devices. These support
 	  a bitmap display of various sizes and depths which can be drawn on
@@ -14,6 +15,11 @@ config VIDEO
 	  option compiles in the video uclass and routes all LCD/video access
 	  through this.
 
+	  If CYCLIC is enabled (which it is by default), the cyclic subsystem
+	  is used to flush pending output to the display periodically, rather
+	  than this happening with every chunk of output. This allows for more
+	  efficient operation and faster display output.
+
 if VIDEO
 
 config VIDEO_FONT_4X6
@@ -231,6 +237,35 @@ config NO_FB_CLEAR
 	  loads takes over the screen.  This, for example, can be used to
 	  keep splash image on screen until grub graphical boot menu starts.
 
+config VIDEO_SYNC_MS
+	int "Video-sync period in milliseconds for foreground processing"
+	default 300 if SANDBOX
+	default 100
+	help
+	  This sets the requested, maximum time before a video sync will take
+	  place, in milliseconds. Note that the time between video syncs
+	  may be longer than this, since syncs only happen when the video system
+	  is used, e.g. by outputting a character to the console.
+
+	  It may also be shorter, since the video uclass will automatically
+	  force a sync in certain situations.
+
+	  Many video-output systems require a sync operation before any output
+	  is visible. This may flush the CPU cache or perhaps copy the
+	  display contents to a hardware framebuffer. Without this, change to
+	  the video may never be displayed.
+
+config VIDEO_SYNC_CYCLIC_MS
+	int "Video-sync period in milliseconds for cyclic processing"
+	depends on CYCLIC
+	default 100 if SANDBOX
+	default 10
+	help
+	  This sets the frequency of cyclic video syncs. The cyclic system is
+	  used to ensure that when U-Boot is idle, it syncs the video. This
+	  improves the responsiveness of the command line to new characters
+	  being entered.
+
 config PANEL
 	bool "Enable panel uclass support"
 	default y
diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
index 600e12b802d1..fbed4ce861b4 100644
--- a/drivers/video/video-uclass.c
+++ b/drivers/video/video-uclass.c
@@ -9,6 +9,7 @@
 #include <bloblist.h>
 #include <console.h>
 #include <cpu_func.h>
+#include <cyclic.h>
 #include <dm.h>
 #include <log.h>
 #include <malloc.h>
@@ -53,6 +54,8 @@
  */
 DECLARE_GLOBAL_DATA_PTR;
 
+struct cyclic_info;
+
 /**
  * struct video_uc_priv - Information for the video uclass
  *
@@ -61,9 +64,11 @@ DECLARE_GLOBAL_DATA_PTR;
  *	available address to use for a device's framebuffer. It starts at
  *	gd->video_top and works downwards, running out of space when it hits
  *	gd->video_bottom.
+ * @cyc: handle for cyclic-execution function, or NULL if none
  */
 struct video_uc_priv {
 	ulong video_ptr;
+	struct cyclic_info *cyc;
 };
 
 /** struct vid_rgb - Describes a video colour */
@@ -364,6 +369,10 @@ int video_sync(struct udevice *vid, bool force)
 			return ret;
 	}
 
+	if (CONFIG_IS_ENABLED(CYCLIC) && !force &&
+	    get_timer(priv->last_sync) < CONFIG_VIDEO_SYNC_MS)
+		return 0;
+
 	/*
 	 * flush_dcache_range() is declared in common.h but it seems that some
 	 * architectures do not actually implement it. Is there a way to find
@@ -376,11 +385,10 @@ int video_sync(struct udevice *vid, bool force)
 					 CONFIG_SYS_CACHELINE_SIZE));
 	}
 #elif defined(CONFIG_VIDEO_SANDBOX_SDL)
-	if (force || get_timer(priv->last_sync) > 100) {
-		sandbox_sdl_sync(priv->fb);
-		priv->last_sync = get_timer(0);
-	}
+	sandbox_sdl_sync(priv->fb);
 #endif
+	priv->last_sync = get_timer(0);
+
 	return 0;
 }
 
@@ -525,10 +533,16 @@ int video_default_font_height(struct udevice *dev)
 	return vc_priv->y_charsize;
 }
 
+static void video_idle(void *ctx)
+{
+	video_sync_all();
+}
+
 /* Set up the display ready for use */
 static int video_post_probe(struct udevice *dev)
 {
 	struct video_uc_plat *plat = dev_get_uclass_plat(dev);
+	struct video_uc_priv *uc_priv = uclass_get_priv(dev->uclass);
 	struct video_priv *priv = dev_get_uclass_priv(dev);
 	char name[30], drv[15], *str;
 	const char *drv_name = drv;
@@ -599,6 +613,17 @@ static int video_post_probe(struct udevice *dev)
 		}
 	}
 
+	/* register cyclic as soon as the first video device is probed */
+	if (CONFIG_IS_ENABLED(CYCLIC) && (gd->flags && GD_FLG_RELOC) &&
+	    !uc_priv->cyc) {
+		uint ms = CONFIG_IF_ENABLED_INT(CYCLIC, VIDEO_SYNC_CYCLIC_MS);
+
+		uc_priv->cyc = cyclic_register(video_idle, ms * 1000,
+					       "video_init", NULL);
+		if (!uc_priv->cyc)
+			return log_msg_ret("cyc", -ENOMEM);
+	}
+
 	return 0;
 };
 
@@ -638,6 +663,18 @@ static int video_post_bind(struct udevice *dev)
 	return 0;
 }
 
+__maybe_unused static int video_destroy(struct uclass *uc)
+{
+	struct video_uc_priv *uc_priv = uclass_get_priv(uc);
+
+	if (uc_priv->cyc) {
+		cyclic_unregister(uc_priv->cyc);
+		uc_priv->cyc = NULL;
+	}
+
+	return 0;
+}
+
 UCLASS_DRIVER(video) = {
 	.id		= UCLASS_VIDEO,
 	.name		= "video",
@@ -647,4 +684,5 @@ UCLASS_DRIVER(video) = {
 	.priv_auto	= sizeof(struct video_uc_priv),
 	.per_device_auto	= sizeof(struct video_priv),
 	.per_device_plat_auto	= sizeof(struct video_uc_plat),
+	CONFIG_IS_ENABLED(CYCLIC, (.destroy = video_destroy, ))
 };
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 4/5] sandbox: Increase cyclic CPU-time limit

[Simon Glass]

Now that sandbox is using cyclic for video, it trips the 1us time
limit. Updating the sandbox display often takes 20ms or more.

Increase the limit to 100ms to avoid a warning.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 common/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/common/Kconfig b/common/Kconfig
index 5906a4af7c33..f3f6f495adfb 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -636,6 +636,7 @@ config SPL_CYCLIC
 
 config CYCLIC_MAX_CPU_TIME_US
 	int "Sets the max allowed time for a cyclic function in us"
+	default 100000 if SANDBOX  # sandbox video is quite slow
 	default 1000
 	help
 	  The max allowed time for a cyclic function in us. If a functions
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 5/5] sandbox: Drop video-sync in serial driver

[Simon Glass]

With sandbox, when U-Boot is waiting for input it syncs the video
display, since presumably the user has finished typing.

Now that cyclic is used for video syncing, we can drop this. Cyclic
will automatically call the video_idle() function when idle.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Fix 'groth' and 'work-around' typos in cover letter

 drivers/serial/sandbox.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/serial/sandbox.c b/drivers/serial/sandbox.c
index f6ac3d228526..be72fd2c65f7 100644
--- a/drivers/serial/sandbox.c
+++ b/drivers/serial/sandbox.c
@@ -139,8 +139,6 @@ static int sandbox_serial_pending(struct udevice *dev, bool input)
 		return 0;
 
 	os_usleep(100);
-	if (IS_ENABLED(CONFIG_VIDEO) && !IS_ENABLED(CONFIG_SPL_BUILD))
-		video_sync_all();
 	avail = membuff_putraw(&priv->buf, 100, false, &data);
 	if (!avail)
 		return 1;	/* buffer full */
-- 
2.43.0.rc2.451.g8631bc7472-goog

Re: [PATCH v2 1/5] cyclic: Add a symbol for SPL

[Stefan Roese]

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

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

Re: [PATCH v2 1/5] cyclic: Add a symbol for SPL

[Devarsh Thakkar]

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

Re: [PATCH v2 1/5] cyclic: Add a symbol for SPL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/5] cyclic: Add a symbol for SPL

[Simon Glass]

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

Re: [PATCH v2 1/5] cyclic: Add a symbol for SPL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2243 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/5] cyclic: Add a symbol for SPL

[Simon Glass]

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

Re: [PATCH v2 1/5] cyclic: Add a symbol for SPL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2492 bytes --]

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 :)

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/5] cyclic: Add a symbol for SPL

[Simon Glass]

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