[PATCH v2 0/6] console: Implement flush() function

[PATCH v2 0/6] console: Implement flush() function

[Pali Rohár]

On certain places it is required to flush output print buffers to ensure
that text strings were sent to console or serial devices. For example when
printing message that U-Boot is going to boot kernel or when U-Boot is
going to change baudrate of terminal device.

Some console devices, like UART, have putc/puts functions which just put
characters into HW transmit queue and do not wait until all data are
transmitted. Doing some sensitive operations (like changing baudrate or
starting kernel which resets UART HW) cause that U-Boot messages are lost.

Therefore introduce a new flush() function, implement it for all serial
devices via pending(false) callback and use this new flush() function on
sensitive places after which output device may go into reset state.

This change fixes printing of U-Boot messages:
"## Starting application at ..."
"## Switch baudrate to ..."

Changes in v2:
* split one big patch into smaller 6 patches
* add config option to allow disabling this new function

Pali Rohár (6):
  sandbox: Add function os_flush()
  console: Implement flush() function
  serial: Implement flush callback
  serial: Implement serial_flush() function for console flush() fallback
  serial: Call flush() before changing baudrate
  boot: Call flush() before booting

 arch/sandbox/cpu/os.c          |  5 +++
 boot/bootm_os.c                |  1 +
 cmd/boot.c                     |  1 +
 cmd/elf.c                      |  2 ++
 cmd/load.c                     |  5 +++
 common/Kconfig                 |  6 ++++
 common/console.c               | 66 ++++++++++++++++++++++++++++++++++
 common/stdio.c                 | 10 ++++++
 drivers/serial/serial-uclass.c | 33 +++++++++++++++++
 drivers/serial/serial.c        |  1 +
 include/_exports.h             |  3 ++
 include/os.h                   |  8 +++++
 include/serial.h               |  5 +++
 include/stdio.h                | 15 ++++++++
 include/stdio_dev.h            |  4 +++
 15 files changed, 165 insertions(+)

-- 
2.20.1

[PATCH v2 1/6] sandbox: Add function os_flush()

[Pali Rohár]

It flushes stdout.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 arch/sandbox/cpu/os.c | 5 +++++
 include/os.h          | 8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index f937991139c9..01845e388d35 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -669,6 +669,11 @@ void os_puts(const char *str)
 		os_putc(*str++);
 }
 
+void os_flush(void)
+{
+	fflush(stdout);
+}
+
 int os_write_ram_buf(const char *fname)
 {
 	struct sandbox_state *state = state_get_current();
diff --git a/include/os.h b/include/os.h
index 148178787bc2..5b353ae9d94b 100644
--- a/include/os.h
+++ b/include/os.h
@@ -295,6 +295,14 @@ void os_putc(int ch);
  */
 void os_puts(const char *str);
 
+/**
+ * os_flush() - flush controlling OS terminal
+ *
+ * This bypasses the U-Boot console support and flushes directly the OS
+ * stdout file descriptor.
+ */
+void os_flush(void);
+
 /**
  * os_write_ram_buf() - write the sandbox RAM buffer to a existing file
  *
-- 
2.20.1

[PATCH v2 2/6] console: Implement flush() function

[Pali Rohár]

On certain places it is required to flush output print buffers to ensure
that text strings were sent to console or serial devices. For example when
printing message that U-Boot is going to boot kernel or when U-Boot is
going to change baudrate of terminal device.

Therefore introduce a new flush() and fflush() functions into console code.
These functions will call .flush callback of associated stdio_dev device.

As this function may increase U-Boot side, allow to compile U-Boot without
this function. For this purpose there is a new config CONSOLE_FLUSH_SUPPORT
which is enabled by default and can be disabled. It is a good idea to have
this option enabled for all boards which have enough space for it.

When option is disabled when U-Boot defines just empty static inline
function fflush() to avoid ifdefs in other code.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 common/Kconfig      |  6 +++++
 common/console.c    | 63 +++++++++++++++++++++++++++++++++++++++++++++
 include/_exports.h  |  3 +++
 include/stdio.h     | 15 +++++++++++
 include/stdio_dev.h |  4 +++
 5 files changed, 91 insertions(+)

diff --git a/common/Kconfig b/common/Kconfig
index e7914ca750a3..109741cc6c44 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -186,6 +186,12 @@ config PRE_CON_BUF_ADDR
 	  We should consider removing this option and allocating the memory
 	  in board_init_f_init_reserve() instead.
 
+config CONSOLE_FLUSH_SUPPORT
+	bool "Enable console flush support"
+	default y
+	help
+	  This enables compilation of flush() function for console flush support.
+
 config CONSOLE_MUX
 	bool "Enable console multiplexing"
 	default y if DM_VIDEO || VIDEO || LCD
diff --git a/common/console.c b/common/console.c
index dc071f1ed665..42415e34d16f 100644
--- a/common/console.c
+++ b/common/console.c
@@ -198,6 +198,9 @@ static int console_setfile(int file, struct stdio_dev * dev)
 		case stdout:
 			gd->jt->putc  = putc;
 			gd->jt->puts  = puts;
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+			gd->jt->flush = flush;
+#endif
 			gd->jt->printf = printf;
 			break;
 		}
@@ -363,6 +366,19 @@ static void console_puts(int file, const char *s)
 	}
 }
 
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+static void console_flush(int file)
+{
+	int i;
+	struct stdio_dev *dev;
+
+	for_each_console_dev(i, file, dev) {
+		if (dev->flush != NULL)
+			dev->flush(dev);
+	}
+}
+#endif
+
 #if CONFIG_IS_ENABLED(SYS_CONSOLE_IS_IN_ENV)
 static inline void console_doenv(int file, struct stdio_dev *dev)
 {
@@ -412,6 +428,14 @@ static inline void console_puts(int file, const char *s)
 	stdio_devices[file]->puts(stdio_devices[file], s);
 }
 
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+static inline void console_flush(int file)
+{
+	if (stdio_devices[file]->flush)
+		stdio_devices[file]->flush(stdio_devices[file]);
+}
+#endif
+
 #if CONFIG_IS_ENABLED(SYS_CONSOLE_IS_IN_ENV)
 static inline void console_doenv(int file, struct stdio_dev *dev)
 {
@@ -525,6 +549,14 @@ void fputs(int file, const char *s)
 		console_puts(file, s);
 }
 
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+void fflush(int file)
+{
+	if (file < MAX_FILES)
+		console_flush(file);
+}
+#endif
+
 int fprintf(int file, const char *fmt, ...)
 {
 	va_list args;
@@ -731,6 +763,37 @@ void puts(const char *s)
 	}
 }
 
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+void flush(void)
+{
+	if (!gd)
+		return;
+
+	/* sandbox can send characters to stdout before it has a console */
+	if (IS_ENABLED(CONFIG_SANDBOX) && !(gd->flags & GD_FLG_SERIAL_READY)) {
+		os_flush();
+		return;
+	}
+
+	if (IS_ENABLED(CONFIG_DEBUG_UART) && !(gd->flags & GD_FLG_SERIAL_READY))
+		return;
+
+	if (IS_ENABLED(CONFIG_SILENT_CONSOLE) && (gd->flags & GD_FLG_SILENT))
+		return;
+
+	if (IS_ENABLED(CONFIG_DISABLE_CONSOLE) && (gd->flags & GD_FLG_DISABLE_CONSOLE))
+		return;
+
+	if (!gd->have_console)
+		return;
+
+	if (gd->flags & GD_FLG_DEVINIT) {
+		/* Send to the standard output */
+		fflush(stdout);
+	}
+}
+#endif
+
 #ifdef CONFIG_CONSOLE_RECORD
 int console_record_init(void)
 {
diff --git a/include/_exports.h b/include/_exports.h
index f6df8b610734..1af946fac327 100644
--- a/include/_exports.h
+++ b/include/_exports.h
@@ -12,6 +12,9 @@
 	EXPORT_FUNC(tstc, int, tstc, void)
 	EXPORT_FUNC(putc, void, putc, const char)
 	EXPORT_FUNC(puts, void, puts, const char *)
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+	EXPORT_FUNC(flush, void, flush, void)
+#endif
 	EXPORT_FUNC(printf, int, printf, const char*, ...)
 #if (defined(CONFIG_X86) && !defined(CONFIG_X86_64)) || defined(CONFIG_PPC)
 	EXPORT_FUNC(irq_install_handler, void, install_hdlr,
diff --git a/include/stdio.h b/include/stdio.h
index 1939a48f0fb6..3241e2d493fa 100644
--- a/include/stdio.h
+++ b/include/stdio.h
@@ -15,6 +15,11 @@ int tstc(void);
 		defined(CONFIG_SPL_SERIAL))
 void putc(const char c);
 void puts(const char *s);
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+void flush(void);
+#else
+static inline void flush(void) {}
+#endif
 int __printf(1, 2) printf(const char *fmt, ...);
 int vprintf(const char *fmt, va_list args);
 #else
@@ -26,6 +31,10 @@ static inline void puts(const char *s)
 {
 }
 
+static inline void flush(void)
+{
+}
+
 static inline int __printf(1, 2) printf(const char *fmt, ...)
 {
 	return 0;
@@ -48,11 +57,17 @@ static inline int vprintf(const char *fmt, va_list args)
 /* stderr */
 #define eputc(c)		fputc(stderr, c)
 #define eputs(s)		fputs(stderr, s)
+#define eflush()		fflush(stderr)
 #define eprintf(fmt, args...)	fprintf(stderr, fmt, ##args)
 
 int __printf(2, 3) fprintf(int file, const char *fmt, ...);
 void fputs(int file, const char *s);
 void fputc(int file, const char c);
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+void fflush(int file);
+#else
+static inline void fflush(int file) {}
+#endif
 int ftstc(int file);
 int fgetc(int file);
 
diff --git a/include/stdio_dev.h b/include/stdio_dev.h
index 270fa2729fb2..06278366ae88 100644
--- a/include/stdio_dev.h
+++ b/include/stdio_dev.h
@@ -37,6 +37,10 @@ struct stdio_dev {
 	void (*putc)(struct stdio_dev *dev, const char c);
 	/* To put a string (accelerator) */
 	void (*puts)(struct stdio_dev *dev, const char *s);
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+	/* To flush output queue */
+	void (*flush)(struct stdio_dev *dev);
+#endif
 
 /* INPUT functions */
 
-- 
2.20.1

[PATCH v2 3/6] serial: Implement flush callback

[Pali Rohár]

UART drivers have putc/puts functions which just put characters into HW
transmit queue and do not wait until all data are transmitted.

Implement flush callback via serial driver's pending(false) callback which
waits until HW transmit all characters from the queue.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/serial/serial-uclass.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
index 4b7819fa473b..71ea88e31c9f 100644
--- a/drivers/serial/serial-uclass.c
+++ b/drivers/serial/serial-uclass.c
@@ -238,6 +238,18 @@ static void _serial_puts(struct udevice *dev, const char *str)
 	} while (*str);
 }
 
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+static void _serial_flush(struct udevice *dev)
+{
+	struct dm_serial_ops *ops = serial_get_ops(dev);
+
+	if (!ops->pending)
+		return;
+	while (ops->pending(dev, false) > 0)
+		;
+}
+#endif
+
 static int __serial_getc(struct udevice *dev)
 {
 	struct dm_serial_ops *ops = serial_get_ops(dev);
@@ -412,6 +424,13 @@ static void serial_stub_puts(struct stdio_dev *sdev, const char *str)
 	_serial_puts(sdev->priv, str);
 }
 
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+static void serial_stub_flush(struct stdio_dev *sdev)
+{
+	_serial_flush(sdev->priv);
+}
+#endif
+
 static int serial_stub_getc(struct stdio_dev *sdev)
 {
 	return _serial_getc(sdev->priv);
@@ -571,6 +590,9 @@ static int serial_post_probe(struct udevice *dev)
 	sdev.priv = dev;
 	sdev.putc = serial_stub_putc;
 	sdev.puts = serial_stub_puts;
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+	sdev.flush = serial_stub_flush;
+#endif
 	sdev.getc = serial_stub_getc;
 	sdev.tstc = serial_stub_tstc;
 
-- 
2.20.1

[PATCH v2 4/6] serial: Implement serial_flush() function for console flush() fallback

[Pali Rohár]

Like in all other console functions, implement also serial_flush() function
as a fallback int console flush() function.

Flush support is available only when config option CONSOLE_FLUSH_SUPPORT is
enabled. So when it is disabled then provides just empty static inline
function serial_flush().

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 common/console.c               |  3 +++
 common/stdio.c                 | 10 ++++++++++
 drivers/serial/serial-uclass.c | 10 ++++++++++
 include/serial.h               |  5 +++++
 4 files changed, 28 insertions(+)

diff --git a/common/console.c b/common/console.c
index 42415e34d16f..72ed7cd3b3b4 100644
--- a/common/console.c
+++ b/common/console.c
@@ -790,6 +790,9 @@ void flush(void)
 	if (gd->flags & GD_FLG_DEVINIT) {
 		/* Send to the standard output */
 		fflush(stdout);
+	} else {
+		/* Send directly to the handler */
+		serial_flush();
 	}
 }
 #endif
diff --git a/common/stdio.c b/common/stdio.c
index 92161a0df87d..b49a02f16f46 100644
--- a/common/stdio.c
+++ b/common/stdio.c
@@ -87,6 +87,13 @@ static void stdio_serial_puts(struct stdio_dev *dev, const char *s)
 	serial_puts(s);
 }
 
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+static void stdio_serial_flush(struct stdio_dev *dev)
+{
+	serial_flush();
+}
+#endif
+
 static int stdio_serial_getc(struct stdio_dev *dev)
 {
 	return serial_getc();
@@ -112,6 +119,9 @@ static void drv_system_init (void)
 	dev.flags = DEV_FLAGS_OUTPUT | DEV_FLAGS_INPUT;
 	dev.putc = stdio_serial_putc;
 	dev.puts = stdio_serial_puts;
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+	dev.flush = stdio_serial_flush;
+#endif
 	dev.getc = stdio_serial_getc;
 	dev.tstc = stdio_serial_tstc;
 	stdio_register (&dev);
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
index 71ea88e31c9f..4e32270d4660 100644
--- a/drivers/serial/serial-uclass.c
+++ b/drivers/serial/serial-uclass.c
@@ -327,6 +327,16 @@ void serial_puts(const char *str)
 		_serial_puts(gd->cur_serial_dev, str);
 }
 
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+void serial_flush(void)
+{
+	if (!gd->cur_serial_dev)
+		return;
+
+	_serial_flush(gd->cur_serial_dev);
+}
+#endif
+
 int serial_getc(void)
 {
 	if (!gd->cur_serial_dev)
diff --git a/include/serial.h b/include/serial.h
index e6051e0a7faf..4e951f0c5f52 100644
--- a/include/serial.h
+++ b/include/serial.h
@@ -386,6 +386,11 @@ void serial_setbrg(void);
 void serial_putc(const char ch);
 void serial_putc_raw(const char ch);
 void serial_puts(const char *str);
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+void serial_flush(void);
+#else
+static inline void serial_flush(void) {}
+#endif
 int serial_getc(void);
 int serial_tstc(void);
 
-- 
2.20.1

[PATCH v2 5/6] serial: Call flush() before changing baudrate

[Pali Rohár]

Changing baudrate is sensitive operation. To ensure that U-Boot messages
printed before changing baudrate are not lost, call new U-Boot console
flush() function.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 cmd/load.c                     | 5 +++++
 drivers/serial/serial-uclass.c | 1 +
 drivers/serial/serial.c        | 1 +
 3 files changed, 7 insertions(+)

diff --git a/cmd/load.c b/cmd/load.c
index e44ae0d56b75..5c4f34781d45 100644
--- a/cmd/load.c
+++ b/cmd/load.c
@@ -83,6 +83,7 @@ static int do_load_serial(struct cmd_tbl *cmdtp, int flag, int argc,
 		printf("## Switch baudrate to %d bps and press ENTER ...\n",
 			load_baudrate);
 		udelay(50000);
+		flush();
 		gd->baudrate = load_baudrate;
 		serial_setbrg();
 		udelay(50000);
@@ -126,6 +127,7 @@ static int do_load_serial(struct cmd_tbl *cmdtp, int flag, int argc,
 		printf("## Switch baudrate to %d bps and press ESC ...\n",
 			current_baudrate);
 		udelay(50000);
+		flush();
 		gd->baudrate = current_baudrate;
 		serial_setbrg();
 		udelay(50000);
@@ -317,6 +319,7 @@ int do_save_serial(struct cmd_tbl *cmdtp, int flag, int argc,
 		printf("## Switch baudrate to %d bps and press ESC ...\n",
 			(int)current_baudrate);
 		udelay(50000);
+		flush();
 		gd->baudrate = current_baudrate;
 		serial_setbrg();
 		udelay(50000);
@@ -471,6 +474,7 @@ static int do_load_serial_bin(struct cmd_tbl *cmdtp, int flag, int argc,
 		printf("## Switch baudrate to %d bps and press ENTER ...\n",
 			load_baudrate);
 		udelay(50000);
+		flush();
 		gd->baudrate = load_baudrate;
 		serial_setbrg();
 		udelay(50000);
@@ -533,6 +537,7 @@ static int do_load_serial_bin(struct cmd_tbl *cmdtp, int flag, int argc,
 		printf("## Switch baudrate to %d bps and press ESC ...\n",
 			current_baudrate);
 		udelay(50000);
+		flush();
 		gd->baudrate = current_baudrate;
 		serial_setbrg();
 		udelay(50000);
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
index 4e32270d4660..0f6860d9100e 100644
--- a/drivers/serial/serial-uclass.c
+++ b/drivers/serial/serial-uclass.c
@@ -527,6 +527,7 @@ static int on_baudrate(const char *name, const char *value, enum env_op op,
 			printf("## Switch baudrate to %d bps and press ENTER ...\n",
 			       baudrate);
 			udelay(50000);
+			flush();
 		}
 
 		gd->baudrate = baudrate;
diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
index 6cdbb89841c1..4b525b644928 100644
--- a/drivers/serial/serial.c
+++ b/drivers/serial/serial.c
@@ -81,6 +81,7 @@ static int on_baudrate(const char *name, const char *value, enum env_op op,
 			printf("## Switch baudrate to %d"
 				" bps and press ENTER ...\n", baudrate);
 			udelay(50000);
+			flush();
 		}
 
 		gd->baudrate = baudrate;
-- 
2.20.1

[PATCH v2 6/6] boot: Call flush() before booting

[Pali Rohár]

In lot of cases kernel resets UART HW. To ensure that U-Boot messages
printed before booting kernel are not lost, call new U-Boot console flush()
function.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 boot/bootm_os.c | 1 +
 cmd/boot.c      | 1 +
 cmd/elf.c       | 2 ++
 3 files changed, 4 insertions(+)

diff --git a/boot/bootm_os.c b/boot/bootm_os.c
index f31820cd07ef..079224ce585b 100644
--- a/boot/bootm_os.c
+++ b/boot/bootm_os.c
@@ -303,6 +303,7 @@ static void do_bootvx_fdt(bootm_headers_t *images)
 #else
 	printf("## Starting vxWorks at 0x%08lx\n", (ulong)images->ep);
 #endif
+	flush();
 
 	boot_jump_vxworks(images);
 
diff --git a/cmd/boot.c b/cmd/boot.c
index be67a5980de3..14839c1cedcc 100644
--- a/cmd/boot.c
+++ b/cmd/boot.c
@@ -32,6 +32,7 @@ static int do_go(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	addr = hextoul(argv[1], NULL);
 
 	printf ("## Starting application at 0x%08lX ...\n", addr);
+	flush();
 
 	/*
 	 * pass address parameter as argv[0] (aka command name),
diff --git a/cmd/elf.c b/cmd/elf.c
index 2b33c50bd026..413d551dc05e 100644
--- a/cmd/elf.c
+++ b/cmd/elf.c
@@ -72,6 +72,7 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		return rcode;
 
 	printf("## Starting application at 0x%08lx ...\n", addr);
+	flush();
 
 	/*
 	 * pass address parameter as argv[0] (aka command name),
@@ -274,6 +275,7 @@ int do_bootvx(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		puts("## Not an ELF image, assuming binary\n");
 
 	printf("## Starting vxWorks at 0x%08lx ...\n", addr);
+	flush();
 
 	dcache_disable();
 #if defined(CONFIG_ARM64) && defined(CONFIG_ARMV8_PSCI)
-- 
2.20.1

Re: [PATCH v2 1/6] sandbox: Add function os_flush()

[Simon Glass]

On Thu, 11 Aug 2022 at 06:39, Pali Rohár <pali@kernel.org> wrote:
>
> It flushes stdout.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  arch/sandbox/cpu/os.c | 5 +++++
>  include/os.h          | 8 ++++++++
>  2 files changed, 13 insertions(+)

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

Re: [PATCH v2 2/6] console: Implement flush() function

[Simon Glass]

Hi Pali,

On Thu, 11 Aug 2022 at 06:39, Pali Rohár <pali@kernel.org> wrote:
>
> On certain places it is required to flush output print buffers to ensure
> that text strings were sent to console or serial devices. For example when
> printing message that U-Boot is going to boot kernel or when U-Boot is
> going to change baudrate of terminal device.
>
> Therefore introduce a new flush() and fflush() functions into console code.
> These functions will call .flush callback of associated stdio_dev device.
>
> As this function may increase U-Boot side, allow to compile U-Boot without
> this function. For this purpose there is a new config CONSOLE_FLUSH_SUPPORT
> which is enabled by default and can be disabled. It is a good idea to have
> this option enabled for all boards which have enough space for it.
>
> When option is disabled when U-Boot defines just empty static inline
> function fflush() to avoid ifdefs in other code.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  common/Kconfig      |  6 +++++
>  common/console.c    | 63 +++++++++++++++++++++++++++++++++++++++++++++
>  include/_exports.h  |  3 +++
>  include/stdio.h     | 15 +++++++++++
>  include/stdio_dev.h |  4 +++
>  5 files changed, 91 insertions(+)
>
> diff --git a/common/Kconfig b/common/Kconfig
> index e7914ca750a3..109741cc6c44 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -186,6 +186,12 @@ config PRE_CON_BUF_ADDR
>           We should consider removing this option and allocating the memory
>           in board_init_f_init_reserve() instead.
>
> +config CONSOLE_FLUSH_SUPPORT
> +       bool "Enable console flush support"
> +       default y
> +       help
> +         This enables compilation of flush() function for console flush support.

This needs at least two more lines, I think. Perhaps you can explain
what flush does and the fact that there is no timeout?

> +
>  config CONSOLE_MUX
>         bool "Enable console multiplexing"
>         default y if DM_VIDEO || VIDEO || LCD
> diff --git a/common/console.c b/common/console.c
> index dc071f1ed665..42415e34d16f 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -198,6 +198,9 @@ static int console_setfile(int file, struct stdio_dev * dev)
>                 case stdout:
>                         gd->jt->putc  = putc;
>                         gd->jt->puts  = puts;
> +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT

How about CONFIG_IS_ENABLED() so we don't enable this in SPL too?

> +                       gd->jt->flush = flush;
> +#endif
>                         gd->jt->printf = printf;
>                         break;
>                 }
> @@ -363,6 +366,19 @@ static void console_puts(int file, const char *s)
>         }
>  }
>
> +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> +static void console_flush(int file)
> +{
> +       int i;
> +       struct stdio_dev *dev;
> +
> +       for_each_console_dev(i, file, dev) {
> +               if (dev->flush != NULL)
> +                       dev->flush(dev);
> +       }
> +}
> +#endif
> +
>  #if CONFIG_IS_ENABLED(SYS_CONSOLE_IS_IN_ENV)
>  static inline void console_doenv(int file, struct stdio_dev *dev)
>  {
> @@ -412,6 +428,14 @@ static inline void console_puts(int file, const char *s)
>         stdio_devices[file]->puts(stdio_devices[file], s);
>  }
>
> +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT

I hope you can drop this #ifdef - see below.

> +static inline void console_flush(int file)
> +{
> +       if (stdio_devices[file]->flush)
> +               stdio_devices[file]->flush(stdio_devices[file]);
> +}
> +#endif
> +
>  #if CONFIG_IS_ENABLED(SYS_CONSOLE_IS_IN_ENV)
>  static inline void console_doenv(int file, struct stdio_dev *dev)
>  {
> @@ -525,6 +549,14 @@ void fputs(int file, const char *s)
>                 console_puts(file, s);
>  }
>
> +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> +void fflush(int file)
> +{
> +       if (file < MAX_FILES)
> +               console_flush(file);
> +}
> +#endif
> +
>  int fprintf(int file, const char *fmt, ...)
>  {
>         va_list args;
> @@ -731,6 +763,37 @@ void puts(const char *s)
>         }
>  }
>
> +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> +void flush(void)
> +{
> +       if (!gd)
> +               return;
> +
> +       /* sandbox can send characters to stdout before it has a console */
> +       if (IS_ENABLED(CONFIG_SANDBOX) && !(gd->flags & GD_FLG_SERIAL_READY)) {
> +               os_flush();
> +               return;
> +       }
> +
> +       if (IS_ENABLED(CONFIG_DEBUG_UART) && !(gd->flags & GD_FLG_SERIAL_READY))
> +               return;
> +
> +       if (IS_ENABLED(CONFIG_SILENT_CONSOLE) && (gd->flags & GD_FLG_SILENT))
> +               return;
> +
> +       if (IS_ENABLED(CONFIG_DISABLE_CONSOLE) && (gd->flags & GD_FLG_DISABLE_CONSOLE))
> +               return;
> +
> +       if (!gd->have_console)
> +               return;
> +
> +       if (gd->flags & GD_FLG_DEVINIT) {
> +               /* Send to the standard output */
> +               fflush(stdout);
> +       }
> +}
> +#endif
> +
>  #ifdef CONFIG_CONSOLE_RECORD
>  int console_record_init(void)
>  {
> diff --git a/include/_exports.h b/include/_exports.h
> index f6df8b610734..1af946fac327 100644
> --- a/include/_exports.h
> +++ b/include/_exports.h
> @@ -12,6 +12,9 @@
>         EXPORT_FUNC(tstc, int, tstc, void)
>         EXPORT_FUNC(putc, void, putc, const char)
>         EXPORT_FUNC(puts, void, puts, const char *)
> +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> +       EXPORT_FUNC(flush, void, flush, void)
> +#endif
>         EXPORT_FUNC(printf, int, printf, const char*, ...)
>  #if (defined(CONFIG_X86) && !defined(CONFIG_X86_64)) || defined(CONFIG_PPC)
>         EXPORT_FUNC(irq_install_handler, void, install_hdlr,
> diff --git a/include/stdio.h b/include/stdio.h
> index 1939a48f0fb6..3241e2d493fa 100644
> --- a/include/stdio.h
> +++ b/include/stdio.h
> @@ -15,6 +15,11 @@ int tstc(void);
>                 defined(CONFIG_SPL_SERIAL))
>  void putc(const char c);
>  void puts(const char *s);
> +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> +void flush(void);
> +#else
> +static inline void flush(void) {}
> +#endif
>  int __printf(1, 2) printf(const char *fmt, ...);
>  int vprintf(const char *fmt, va_list args);
>  #else
> @@ -26,6 +31,10 @@ static inline void puts(const char *s)
>  {
>  }
>
> +static inline void flush(void)
> +{
> +}
> +
>  static inline int __printf(1, 2) printf(const char *fmt, ...)
>  {
>         return 0;
> @@ -48,11 +57,17 @@ static inline int vprintf(const char *fmt, va_list args)
>  /* stderr */
>  #define eputc(c)               fputc(stderr, c)
>  #define eputs(s)               fputs(stderr, s)
> +#define eflush()               fflush(stderr)
>  #define eprintf(fmt, args...)  fprintf(stderr, fmt, ##args)
>
>  int __printf(2, 3) fprintf(int file, const char *fmt, ...);
>  void fputs(int file, const char *s);
>  void fputc(int file, const char c);
> +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> +void fflush(int file);
> +#else
> +static inline void fflush(int file) {}
> +#endif
>  int ftstc(int file);
>  int fgetc(int file);
>
> diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> index 270fa2729fb2..06278366ae88 100644
> --- a/include/stdio_dev.h
> +++ b/include/stdio_dev.h
> @@ -37,6 +37,10 @@ struct stdio_dev {
>         void (*putc)(struct stdio_dev *dev, const char c);
>         /* To put a string (accelerator) */
>         void (*puts)(struct stdio_dev *dev, const char *s);
> +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT

I'd argue it isn't worth the #ifdef and we might as well have this
member here always. Then you can drop some #ifdefs from your code.

> +       /* To flush output queue */
> +       void (*flush)(struct stdio_dev *dev);
> +#endif
>
>  /* INPUT functions */
>
> --
> 2.20.1
>

Regards,
SImon

Re: [PATCH v2 3/6] serial: Implement flush callback

[Simon Glass]

On Thu, 11 Aug 2022 at 06:39, Pali Rohár <pali@kernel.org> wrote:
>
> UART drivers have putc/puts functions which just put characters into HW
> transmit queue and do not wait until all data are transmitted.
>
> Implement flush callback via serial driver's pending(false) callback which
> waits until HW transmit all characters from the queue.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  drivers/serial/serial-uclass.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)

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

But let's always add this to the struct.



>
> diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> index 4b7819fa473b..71ea88e31c9f 100644
> --- a/drivers/serial/serial-uclass.c
> +++ b/drivers/serial/serial-uclass.c
> @@ -238,6 +238,18 @@ static void _serial_puts(struct udevice *dev, const char *str)
>         } while (*str);
>  }
>
> +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> +static void _serial_flush(struct udevice *dev)
> +{
> +       struct dm_serial_ops *ops = serial_get_ops(dev);
> +
> +       if (!ops->pending)
> +               return;
> +       while (ops->pending(dev, false) > 0)
> +               ;
> +}
> +#endif
> +
>  static int __serial_getc(struct udevice *dev)
>  {
>         struct dm_serial_ops *ops = serial_get_ops(dev);
> @@ -412,6 +424,13 @@ static void serial_stub_puts(struct stdio_dev *sdev, const char *str)
>         _serial_puts(sdev->priv, str);
>  }
>
> +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> +static void serial_stub_flush(struct stdio_dev *sdev)
> +{
> +       _serial_flush(sdev->priv);
> +}
> +#endif
> +
>  static int serial_stub_getc(struct stdio_dev *sdev)
>  {
>         return _serial_getc(sdev->priv);
> @@ -571,6 +590,9 @@ static int serial_post_probe(struct udevice *dev)
>         sdev.priv = dev;
>         sdev.putc = serial_stub_putc;
>         sdev.puts = serial_stub_puts;
> +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> +       sdev.flush = serial_stub_flush;
> +#endif
>         sdev.getc = serial_stub_getc;
>         sdev.tstc = serial_stub_tstc;
>
> --
> 2.20.1
>

Re: [PATCH v2 4/6] serial: Implement serial_flush() function for console flush() fallback

[Simon Glass]

Hi Pali,

On Thu, 11 Aug 2022 at 06:39, Pali Rohár <pali@kernel.org> wrote:
>
> Like in all other console functions, implement also serial_flush() function
> as a fallback int console flush() function.
>
> Flush support is available only when config option CONSOLE_FLUSH_SUPPORT is
> enabled. So when it is disabled then provides just empty static inline
> function serial_flush().
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  common/console.c               |  3 +++
>  common/stdio.c                 | 10 ++++++++++
>  drivers/serial/serial-uclass.c | 10 ++++++++++
>  include/serial.h               |  5 +++++
>  4 files changed, 28 insertions(+)

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

Re: [PATCH v2 5/6] serial: Call flush() before changing baudrate

[Simon Glass]

On Thu, 11 Aug 2022 at 06:39, Pali Rohár <pali@kernel.org> wrote:
>
> Changing baudrate is sensitive operation. To ensure that U-Boot messages

is a sensitive

> printed before changing baudrate are not lost, call new U-Boot console
> flush() function.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  cmd/load.c                     | 5 +++++
>  drivers/serial/serial-uclass.c | 1 +
>  drivers/serial/serial.c        | 1 +
>  3 files changed, 7 insertions(+)
>
> diff --git a/cmd/load.c b/cmd/load.c
> index e44ae0d56b75..5c4f34781d45 100644
> --- a/cmd/load.c
> +++ b/cmd/load.c
> @@ -83,6 +83,7 @@ static int do_load_serial(struct cmd_tbl *cmdtp, int flag, int argc,
>                 printf("## Switch baudrate to %d bps and press ENTER ...\n",
>                         load_baudrate);
>                 udelay(50000);
> +               flush();
>                 gd->baudrate = load_baudrate;
>                 serial_setbrg();
>                 udelay(50000);
> @@ -126,6 +127,7 @@ static int do_load_serial(struct cmd_tbl *cmdtp, int flag, int argc,
>                 printf("## Switch baudrate to %d bps and press ESC ...\n",
>                         current_baudrate);
>                 udelay(50000);
> +               flush();
>                 gd->baudrate = current_baudrate;
>                 serial_setbrg();
>                 udelay(50000);
> @@ -317,6 +319,7 @@ int do_save_serial(struct cmd_tbl *cmdtp, int flag, int argc,
>                 printf("## Switch baudrate to %d bps and press ESC ...\n",
>                         (int)current_baudrate);
>                 udelay(50000);
> +               flush();
>                 gd->baudrate = current_baudrate;
>                 serial_setbrg();
>                 udelay(50000);
> @@ -471,6 +474,7 @@ static int do_load_serial_bin(struct cmd_tbl *cmdtp, int flag, int argc,
>                 printf("## Switch baudrate to %d bps and press ENTER ...\n",
>                         load_baudrate);
>                 udelay(50000);
> +               flush();
>                 gd->baudrate = load_baudrate;
>                 serial_setbrg();
>                 udelay(50000);
> @@ -533,6 +537,7 @@ static int do_load_serial_bin(struct cmd_tbl *cmdtp, int flag, int argc,
>                 printf("## Switch baudrate to %d bps and press ESC ...\n",
>                         current_baudrate);
>                 udelay(50000);
> +               flush();
>                 gd->baudrate = current_baudrate;
>                 serial_setbrg();
>                 udelay(50000);
> diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> index 4e32270d4660..0f6860d9100e 100644
> --- a/drivers/serial/serial-uclass.c
> +++ b/drivers/serial/serial-uclass.c
> @@ -527,6 +527,7 @@ static int on_baudrate(const char *name, const char *value, enum env_op op,
>                         printf("## Switch baudrate to %d bps and press ENTER ...\n",
>                                baudrate);
>                         udelay(50000);
> +                       flush();
>                 }
>
>                 gd->baudrate = baudrate;
> diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
> index 6cdbb89841c1..4b525b644928 100644
> --- a/drivers/serial/serial.c
> +++ b/drivers/serial/serial.c
> @@ -81,6 +81,7 @@ static int on_baudrate(const char *name, const char *value, enum env_op op,
>                         printf("## Switch baudrate to %d"
>                                 " bps and press ENTER ...\n", baudrate);
>                         udelay(50000);
> +                       flush();

Please don't implement new features in legacy code.

>                 }
>
>                 gd->baudrate = baudrate;
> --
> 2.20.1
>

Regards,
SImon

Re: [PATCH v2 6/6] boot: Call flush() before booting

[Simon Glass]

On Thu, 11 Aug 2022 at 06:39, Pali Rohár <pali@kernel.org> wrote:
>
> In lot of cases kernel resets UART HW. To ensure that U-Boot messages

In a lot

> printed before booting kernel are not lost, call new U-Boot console flush()

booting the kernel

> function.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  boot/bootm_os.c | 1 +
>  cmd/boot.c      | 1 +
>  cmd/elf.c       | 2 ++
>  3 files changed, 4 insertions(+)

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

Re: [PATCH v2 2/6] console: Implement flush() function

[Pali Rohár]

On Thursday 11 August 2022 08:47:50 Simon Glass wrote:
> > diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> > index 270fa2729fb2..06278366ae88 100644
> > --- a/include/stdio_dev.h
> > +++ b/include/stdio_dev.h
> > @@ -37,6 +37,10 @@ struct stdio_dev {
> >         void (*putc)(struct stdio_dev *dev, const char c);
> >         /* To put a string (accelerator) */
> >         void (*puts)(struct stdio_dev *dev, const char *s);
> > +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> 
> I'd argue it isn't worth the #ifdef and we might as well have this
> member here always. Then you can drop some #ifdefs from your code.

But then it will increase binary code size.

> > +       /* To flush output queue */
> > +       void (*flush)(struct stdio_dev *dev);
> > +#endif
> >
> >  /* INPUT functions */
> >
> > --
> > 2.20.1
> >
> 
> Regards,
> SImon

Re: [PATCH v2 5/6] serial: Call flush() before changing baudrate

[Pali Rohár]

On Thursday 11 August 2022 08:47:53 Simon Glass wrote:
> On Thu, 11 Aug 2022 at 06:39, Pali Rohár <pali@kernel.org> wrote:
> >
> > Changing baudrate is sensitive operation. To ensure that U-Boot messages
> 
> is a sensitive
> 
> > printed before changing baudrate are not lost, call new U-Boot console
> > flush() function.
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >  cmd/load.c                     | 5 +++++
> >  drivers/serial/serial-uclass.c | 1 +
> >  drivers/serial/serial.c        | 1 +
> >  3 files changed, 7 insertions(+)
> >
> > diff --git a/cmd/load.c b/cmd/load.c
> > index e44ae0d56b75..5c4f34781d45 100644
> > --- a/cmd/load.c
> > +++ b/cmd/load.c
> > @@ -83,6 +83,7 @@ static int do_load_serial(struct cmd_tbl *cmdtp, int flag, int argc,
> >                 printf("## Switch baudrate to %d bps and press ENTER ...\n",
> >                         load_baudrate);
> >                 udelay(50000);
> > +               flush();
> >                 gd->baudrate = load_baudrate;
> >                 serial_setbrg();
> >                 udelay(50000);
> > @@ -126,6 +127,7 @@ static int do_load_serial(struct cmd_tbl *cmdtp, int flag, int argc,
> >                 printf("## Switch baudrate to %d bps and press ESC ...\n",
> >                         current_baudrate);
> >                 udelay(50000);
> > +               flush();
> >                 gd->baudrate = current_baudrate;
> >                 serial_setbrg();
> >                 udelay(50000);
> > @@ -317,6 +319,7 @@ int do_save_serial(struct cmd_tbl *cmdtp, int flag, int argc,
> >                 printf("## Switch baudrate to %d bps and press ESC ...\n",
> >                         (int)current_baudrate);
> >                 udelay(50000);
> > +               flush();
> >                 gd->baudrate = current_baudrate;
> >                 serial_setbrg();
> >                 udelay(50000);
> > @@ -471,6 +474,7 @@ static int do_load_serial_bin(struct cmd_tbl *cmdtp, int flag, int argc,
> >                 printf("## Switch baudrate to %d bps and press ENTER ...\n",
> >                         load_baudrate);
> >                 udelay(50000);
> > +               flush();
> >                 gd->baudrate = load_baudrate;
> >                 serial_setbrg();
> >                 udelay(50000);
> > @@ -533,6 +537,7 @@ static int do_load_serial_bin(struct cmd_tbl *cmdtp, int flag, int argc,
> >                 printf("## Switch baudrate to %d bps and press ESC ...\n",
> >                         current_baudrate);
> >                 udelay(50000);
> > +               flush();
> >                 gd->baudrate = current_baudrate;
> >                 serial_setbrg();
> >                 udelay(50000);
> > diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> > index 4e32270d4660..0f6860d9100e 100644
> > --- a/drivers/serial/serial-uclass.c
> > +++ b/drivers/serial/serial-uclass.c
> > @@ -527,6 +527,7 @@ static int on_baudrate(const char *name, const char *value, enum env_op op,
> >                         printf("## Switch baudrate to %d bps and press ENTER ...\n",
> >                                baudrate);
> >                         udelay(50000);
> > +                       flush();
> >                 }
> >
> >                 gd->baudrate = baudrate;
> > diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
> > index 6cdbb89841c1..4b525b644928 100644
> > --- a/drivers/serial/serial.c
> > +++ b/drivers/serial/serial.c
> > @@ -81,6 +81,7 @@ static int on_baudrate(const char *name, const char *value, enum env_op op,
> >                         printf("## Switch baudrate to %d"
> >                                 " bps and press ENTER ...\n", baudrate);
> >                         udelay(50000);
> > +                       flush();
> 
> Please don't implement new features in legacy code.

Sorry, I do not know what is the legacy code in this spaghetti monster.

> >                 }
> >
> >                 gd->baudrate = baudrate;
> > --
> > 2.20.1
> >
> 
> Regards,
> SImon

Re: [PATCH v2 5/6] serial: Call flush() before changing baudrate

[Simon Glass]

Hi Pali,

On Thu, 11 Aug 2022 at 08:51, Pali Rohár <pali@kernel.org> wrote:
>
> On Thursday 11 August 2022 08:47:53 Simon Glass wrote:
> > On Thu, 11 Aug 2022 at 06:39, Pali Rohár <pali@kernel.org> wrote:
> > >
> > > Changing baudrate is sensitive operation. To ensure that U-Boot messages
> >
> > is a sensitive
> >
> > > printed before changing baudrate are not lost, call new U-Boot console
> > > flush() function.
> > >
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > ---
> > >  cmd/load.c                     | 5 +++++
> > >  drivers/serial/serial-uclass.c | 1 +
> > >  drivers/serial/serial.c        | 1 +
> > >  3 files changed, 7 insertions(+)
> > >
> > > diff --git a/cmd/load.c b/cmd/load.c
> > > index e44ae0d56b75..5c4f34781d45 100644
> > > --- a/cmd/load.c
> > > +++ b/cmd/load.c
> > > @@ -83,6 +83,7 @@ static int do_load_serial(struct cmd_tbl *cmdtp, int flag, int argc,
> > >                 printf("## Switch baudrate to %d bps and press ENTER ...\n",
> > >                         load_baudrate);
> > >                 udelay(50000);
> > > +               flush();
> > >                 gd->baudrate = load_baudrate;
> > >                 serial_setbrg();
> > >                 udelay(50000);
> > > @@ -126,6 +127,7 @@ static int do_load_serial(struct cmd_tbl *cmdtp, int flag, int argc,
> > >                 printf("## Switch baudrate to %d bps and press ESC ...\n",
> > >                         current_baudrate);
> > >                 udelay(50000);
> > > +               flush();
> > >                 gd->baudrate = current_baudrate;
> > >                 serial_setbrg();
> > >                 udelay(50000);
> > > @@ -317,6 +319,7 @@ int do_save_serial(struct cmd_tbl *cmdtp, int flag, int argc,
> > >                 printf("## Switch baudrate to %d bps and press ESC ...\n",
> > >                         (int)current_baudrate);
> > >                 udelay(50000);
> > > +               flush();
> > >                 gd->baudrate = current_baudrate;
> > >                 serial_setbrg();
> > >                 udelay(50000);
> > > @@ -471,6 +474,7 @@ static int do_load_serial_bin(struct cmd_tbl *cmdtp, int flag, int argc,
> > >                 printf("## Switch baudrate to %d bps and press ENTER ...\n",
> > >                         load_baudrate);
> > >                 udelay(50000);
> > > +               flush();
> > >                 gd->baudrate = load_baudrate;
> > >                 serial_setbrg();
> > >                 udelay(50000);
> > > @@ -533,6 +537,7 @@ static int do_load_serial_bin(struct cmd_tbl *cmdtp, int flag, int argc,
> > >                 printf("## Switch baudrate to %d bps and press ESC ...\n",
> > >                         current_baudrate);
> > >                 udelay(50000);
> > > +               flush();
> > >                 gd->baudrate = current_baudrate;
> > >                 serial_setbrg();
> > >                 udelay(50000);
> > > diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> > > index 4e32270d4660..0f6860d9100e 100644
> > > --- a/drivers/serial/serial-uclass.c
> > > +++ b/drivers/serial/serial-uclass.c
> > > @@ -527,6 +527,7 @@ static int on_baudrate(const char *name, const char *value, enum env_op op,
> > >                         printf("## Switch baudrate to %d bps and press ENTER ...\n",
> > >                                baudrate);
> > >                         udelay(50000);
> > > +                       flush();
> > >                 }
> > >
> > >                 gd->baudrate = baudrate;
> > > diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
> > > index 6cdbb89841c1..4b525b644928 100644
> > > --- a/drivers/serial/serial.c
> > > +++ b/drivers/serial/serial.c
> > > @@ -81,6 +81,7 @@ static int on_baudrate(const char *name, const char *value, enum env_op op,
> > >                         printf("## Switch baudrate to %d"
> > >                                 " bps and press ENTER ...\n", baudrate);
> > >                         udelay(50000);
> > > +                       flush();
> >
> > Please don't implement new features in legacy code.
>
> Sorry, I do not know what is the legacy code in this spaghetti monster.

This file is legacy...you only need to change the uclass.

>
> > >                 }
> > >
> > >                 gd->baudrate = baudrate;
> > > --
> > > 2.20.1
> > >
> >
> > Regards,
> > SImon

Regards,
Simon

Re: [PATCH v2 2/6] console: Implement flush() function

[Simon Glass]

Hi Pali,

On Thu, 11 Aug 2022 at 08:50, Pali Rohár <pali@kernel.org> wrote:
>
> On Thursday 11 August 2022 08:47:50 Simon Glass wrote:
> > > diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> > > index 270fa2729fb2..06278366ae88 100644
> > > --- a/include/stdio_dev.h
> > > +++ b/include/stdio_dev.h
> > > @@ -37,6 +37,10 @@ struct stdio_dev {
> > >         void (*putc)(struct stdio_dev *dev, const char c);
> > >         /* To put a string (accelerator) */
> > >         void (*puts)(struct stdio_dev *dev, const char *s);
> > > +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> >
> > I'd argue it isn't worth the #ifdef and we might as well have this
> > member here always. Then you can drop some #ifdefs from your code.
>
> But then it will increase binary code size.

Using the member will increase code size. But I think the only place
you need an #ifdef for that is when you include it in the driver
struct. You can use __maybe_unused in the other place.

Having the member will only increase memory usage, not code size.

Regards,
Simon

Re: [PATCH v2 5/6] serial: Call flush() before changing baudrate

[Pali Rohár]

On Thursday 11 August 2022 18:08:45 Simon Glass wrote:
> Hi Pali,
> 
> On Thu, 11 Aug 2022 at 08:51, Pali Rohár <pali@kernel.org> wrote:
> >
> > On Thursday 11 August 2022 08:47:53 Simon Glass wrote:
> > > On Thu, 11 Aug 2022 at 06:39, Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > Changing baudrate is sensitive operation. To ensure that U-Boot messages
> > >
> > > is a sensitive
> > >
> > > > printed before changing baudrate are not lost, call new U-Boot console
> > > > flush() function.
> > > >
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > ---
> > > >  cmd/load.c                     | 5 +++++
> > > >  drivers/serial/serial-uclass.c | 1 +
> > > >  drivers/serial/serial.c        | 1 +
> > > >  3 files changed, 7 insertions(+)
> > > >
> > > > diff --git a/cmd/load.c b/cmd/load.c
> > > > index e44ae0d56b75..5c4f34781d45 100644
> > > > --- a/cmd/load.c
> > > > +++ b/cmd/load.c
> > > > @@ -83,6 +83,7 @@ static int do_load_serial(struct cmd_tbl *cmdtp, int flag, int argc,
> > > >                 printf("## Switch baudrate to %d bps and press ENTER ...\n",
> > > >                         load_baudrate);
> > > >                 udelay(50000);
> > > > +               flush();
> > > >                 gd->baudrate = load_baudrate;
> > > >                 serial_setbrg();
> > > >                 udelay(50000);
> > > > @@ -126,6 +127,7 @@ static int do_load_serial(struct cmd_tbl *cmdtp, int flag, int argc,
> > > >                 printf("## Switch baudrate to %d bps and press ESC ...\n",
> > > >                         current_baudrate);
> > > >                 udelay(50000);
> > > > +               flush();
> > > >                 gd->baudrate = current_baudrate;
> > > >                 serial_setbrg();
> > > >                 udelay(50000);
> > > > @@ -317,6 +319,7 @@ int do_save_serial(struct cmd_tbl *cmdtp, int flag, int argc,
> > > >                 printf("## Switch baudrate to %d bps and press ESC ...\n",
> > > >                         (int)current_baudrate);
> > > >                 udelay(50000);
> > > > +               flush();
> > > >                 gd->baudrate = current_baudrate;
> > > >                 serial_setbrg();
> > > >                 udelay(50000);
> > > > @@ -471,6 +474,7 @@ static int do_load_serial_bin(struct cmd_tbl *cmdtp, int flag, int argc,
> > > >                 printf("## Switch baudrate to %d bps and press ENTER ...\n",
> > > >                         load_baudrate);
> > > >                 udelay(50000);
> > > > +               flush();
> > > >                 gd->baudrate = load_baudrate;
> > > >                 serial_setbrg();
> > > >                 udelay(50000);
> > > > @@ -533,6 +537,7 @@ static int do_load_serial_bin(struct cmd_tbl *cmdtp, int flag, int argc,
> > > >                 printf("## Switch baudrate to %d bps and press ESC ...\n",
> > > >                         current_baudrate);
> > > >                 udelay(50000);
> > > > +               flush();
> > > >                 gd->baudrate = current_baudrate;
> > > >                 serial_setbrg();
> > > >                 udelay(50000);
> > > > diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> > > > index 4e32270d4660..0f6860d9100e 100644
> > > > --- a/drivers/serial/serial-uclass.c
> > > > +++ b/drivers/serial/serial-uclass.c
> > > > @@ -527,6 +527,7 @@ static int on_baudrate(const char *name, const char *value, enum env_op op,
> > > >                         printf("## Switch baudrate to %d bps and press ENTER ...\n",
> > > >                                baudrate);
> > > >                         udelay(50000);
> > > > +                       flush();
> > > >                 }
> > > >
> > > >                 gd->baudrate = baudrate;
> > > > diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
> > > > index 6cdbb89841c1..4b525b644928 100644
> > > > --- a/drivers/serial/serial.c
> > > > +++ b/drivers/serial/serial.c
> > > > @@ -81,6 +81,7 @@ static int on_baudrate(const char *name, const char *value, enum env_op op,
> > > >                         printf("## Switch baudrate to %d"
> > > >                                 " bps and press ENTER ...\n", baudrate);
> > > >                         udelay(50000);
> > > > +                       flush();
> > >
> > > Please don't implement new features in legacy code.
> >
> > Sorry, I do not know what is the legacy code in this spaghetti monster.
> 
> This file is legacy...you only need to change the uclass.

Ok

> >
> > > >                 }
> > > >
> > > >                 gd->baudrate = baudrate;
> > > > --
> > > > 2.20.1
> > > >
> > >
> > > Regards,
> > > SImon
> 
> Regards,
> Simon

Re: [PATCH v2 2/6] console: Implement flush() function

[Pali Rohár]

On Thursday 11 August 2022 18:08:46 Simon Glass wrote:
> Hi Pali,
> 
> On Thu, 11 Aug 2022 at 08:50, Pali Rohár <pali@kernel.org> wrote:
> >
> > On Thursday 11 August 2022 08:47:50 Simon Glass wrote:
> > > > diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> > > > index 270fa2729fb2..06278366ae88 100644
> > > > --- a/include/stdio_dev.h
> > > > +++ b/include/stdio_dev.h
> > > > @@ -37,6 +37,10 @@ struct stdio_dev {
> > > >         void (*putc)(struct stdio_dev *dev, const char c);
> > > >         /* To put a string (accelerator) */
> > > >         void (*puts)(struct stdio_dev *dev, const char *s);
> > > > +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> > >
> > > I'd argue it isn't worth the #ifdef and we might as well have this
> > > member here always. Then you can drop some #ifdefs from your code.
> >
> > But then it will increase binary code size.
> 
> Using the member will increase code size. But I think the only place
> you need an #ifdef for that is when you include it in the driver
> struct. You can use __maybe_unused in the other place.
> 
> Having the member will only increase memory usage, not code size.

But static memory structures are part of the u-boot.bin binary and also
their usage increase code size (required copy, etc...), so at the end it
increase code size.

> Regards,
> Simon

Re: [PATCH v2 2/6] console: Implement flush() function

[Simon Glass]

Hi Pali,

On Fri, 12 Aug 2022 at 03:01, Pali Rohár <pali@kernel.org> wrote:
>
> On Thursday 11 August 2022 18:08:46 Simon Glass wrote:
> > Hi Pali,
> >
> > On Thu, 11 Aug 2022 at 08:50, Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Thursday 11 August 2022 08:47:50 Simon Glass wrote:
> > > > > diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> > > > > index 270fa2729fb2..06278366ae88 100644
> > > > > --- a/include/stdio_dev.h
> > > > > +++ b/include/stdio_dev.h
> > > > > @@ -37,6 +37,10 @@ struct stdio_dev {
> > > > >         void (*putc)(struct stdio_dev *dev, const char c);
> > > > >         /* To put a string (accelerator) */
> > > > >         void (*puts)(struct stdio_dev *dev, const char *s);
> > > > > +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> > > >
> > > > I'd argue it isn't worth the #ifdef and we might as well have this
> > > > member here always. Then you can drop some #ifdefs from your code.
> > >
> > > But then it will increase binary code size.
> >
> > Using the member will increase code size. But I think the only place
> > you need an #ifdef for that is when you include it in the driver
> > struct. You can use __maybe_unused in the other place.
> >
> > Having the member will only increase memory usage, not code size.
>
> But static memory structures are part of the u-boot.bin binary and also
> their usage increase code size (required copy, etc...), so at the end it
> increase code size.

From what I understand stdio_dev is allocated at runtime in BSS:

struct stdio_dev *stdio_devices[] = { NULL, NULL, NULL };

(the NULL stuff should not be there, but does nothing, I believe)

So long as no code accesses your new member, then it should only
affect the BSS size.

If you are very worried about it, you could use the technique in
asm-generic/global_data.h to avoid #ifdefs in the C code:

#ifdef CONFIG_GENERATE_ACPI_TABLE
#define gd_acpi_start() gd->acpi_start
#define gd_set_acpi_start(addr) gd->acpi_start = addr
#else
#define gd_acpi_start() 0UL
#define gd_set_acpi_start(addr)
#endif

Regards,
Simon

Re: [PATCH v2 2/6] console: Implement flush() function

[Pali Rohár]

On Friday 12 August 2022 09:11:10 Simon Glass wrote:
> Hi Pali,
> 
> On Fri, 12 Aug 2022 at 03:01, Pali Rohár <pali@kernel.org> wrote:
> >
> > On Thursday 11 August 2022 18:08:46 Simon Glass wrote:
> > > Hi Pali,
> > >
> > > On Thu, 11 Aug 2022 at 08:50, Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Thursday 11 August 2022 08:47:50 Simon Glass wrote:
> > > > > > diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> > > > > > index 270fa2729fb2..06278366ae88 100644
> > > > > > --- a/include/stdio_dev.h
> > > > > > +++ b/include/stdio_dev.h
> > > > > > @@ -37,6 +37,10 @@ struct stdio_dev {
> > > > > >         void (*putc)(struct stdio_dev *dev, const char c);
> > > > > >         /* To put a string (accelerator) */
> > > > > >         void (*puts)(struct stdio_dev *dev, const char *s);
> > > > > > +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> > > > >
> > > > > I'd argue it isn't worth the #ifdef and we might as well have this
> > > > > member here always. Then you can drop some #ifdefs from your code.
> > > >
> > > > But then it will increase binary code size.
> > >
> > > Using the member will increase code size. But I think the only place
> > > you need an #ifdef for that is when you include it in the driver
> > > struct. You can use __maybe_unused in the other place.
> > >
> > > Having the member will only increase memory usage, not code size.
> >
> > But static memory structures are part of the u-boot.bin binary and also
> > their usage increase code size (required copy, etc...), so at the end it
> > increase code size.
> 
> From what I understand stdio_dev is allocated at runtime in BSS:
> 
> struct stdio_dev *stdio_devices[] = { NULL, NULL, NULL };
> 
> (the NULL stuff should not be there, but does nothing, I believe)
> 
> So long as no code accesses your new member, then it should only
> affect the BSS size.

Code of course has to access new member. How otherwise would be able to
call that new callbacks? E.g. all new code in patch 2/6 or 3/6.

> If you are very worried about it, you could use the technique in
> asm-generic/global_data.h to avoid #ifdefs in the C code:
> 
> #ifdef CONFIG_GENERATE_ACPI_TABLE
> #define gd_acpi_start() gd->acpi_start
> #define gd_set_acpi_start(addr) gd->acpi_start = addr
> #else
> #define gd_acpi_start() 0UL
> #define gd_set_acpi_start(addr)
> #endif
> 
> Regards,
> Simon

Re: [PATCH v2 2/6] console: Implement flush() function

[Simon Glass]

Hi Pali,

On Fri, 12 Aug 2022 at 09:17, Pali Rohár <pali@kernel.org> wrote:
>
> On Friday 12 August 2022 09:11:10 Simon Glass wrote:
> > Hi Pali,
> >
> > On Fri, 12 Aug 2022 at 03:01, Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Thursday 11 August 2022 18:08:46 Simon Glass wrote:
> > > > Hi Pali,
> > > >
> > > > On Thu, 11 Aug 2022 at 08:50, Pali Rohár <pali@kernel.org> wrote:
> > > > >
> > > > > On Thursday 11 August 2022 08:47:50 Simon Glass wrote:
> > > > > > > diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> > > > > > > index 270fa2729fb2..06278366ae88 100644
> > > > > > > --- a/include/stdio_dev.h
> > > > > > > +++ b/include/stdio_dev.h
> > > > > > > @@ -37,6 +37,10 @@ struct stdio_dev {
> > > > > > >         void (*putc)(struct stdio_dev *dev, const char c);
> > > > > > >         /* To put a string (accelerator) */
> > > > > > >         void (*puts)(struct stdio_dev *dev, const char *s);
> > > > > > > +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> > > > > >
> > > > > > I'd argue it isn't worth the #ifdef and we might as well have this
> > > > > > member here always. Then you can drop some #ifdefs from your code.
> > > > >
> > > > > But then it will increase binary code size.
> > > >
> > > > Using the member will increase code size. But I think the only place
> > > > you need an #ifdef for that is when you include it in the driver
> > > > struct. You can use __maybe_unused in the other place.
> > > >
> > > > Having the member will only increase memory usage, not code size.
> > >
> > > But static memory structures are part of the u-boot.bin binary and also
> > > their usage increase code size (required copy, etc...), so at the end it
> > > increase code size.
> >
> > From what I understand stdio_dev is allocated at runtime in BSS:
> >
> > struct stdio_dev *stdio_devices[] = { NULL, NULL, NULL };
> >
> > (the NULL stuff should not be there, but does nothing, I believe)
> >
> > So long as no code accesses your new member, then it should only
> > affect the BSS size.
>
> Code of course has to access new member. How otherwise would be able to
> call that new callbacks? E.g. all new code in patch 2/6 or 3/6.

Yes, see below. Also, do check whether it actually adds code or not.

>
> > If you are very worried about it, you could use the technique in
> > asm-generic/global_data.h to avoid #ifdefs in the C code:
> >
> > #ifdef CONFIG_GENERATE_ACPI_TABLE
> > #define gd_acpi_start() gd->acpi_start
> > #define gd_set_acpi_start(addr) gd->acpi_start = addr
> > #else
> > #define gd_acpi_start() 0UL
> > #define gd_set_acpi_start(addr)
> > #endif
> >
> > Regards,
> > Simon

Regards,
Simon

Re: [PATCH v2 2/6] console: Implement flush() function

[Pali Rohár]

On Friday 12 August 2022 20:21:00 Simon Glass wrote:
> Hi Pali,
> 
> On Fri, 12 Aug 2022 at 09:17, Pali Rohár <pali@kernel.org> wrote:
> >
> > On Friday 12 August 2022 09:11:10 Simon Glass wrote:
> > > Hi Pali,
> > >
> > > On Fri, 12 Aug 2022 at 03:01, Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Thursday 11 August 2022 18:08:46 Simon Glass wrote:
> > > > > Hi Pali,
> > > > >
> > > > > On Thu, 11 Aug 2022 at 08:50, Pali Rohár <pali@kernel.org> wrote:
> > > > > >
> > > > > > On Thursday 11 August 2022 08:47:50 Simon Glass wrote:
> > > > > > > > diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> > > > > > > > index 270fa2729fb2..06278366ae88 100644
> > > > > > > > --- a/include/stdio_dev.h
> > > > > > > > +++ b/include/stdio_dev.h
> > > > > > > > @@ -37,6 +37,10 @@ struct stdio_dev {
> > > > > > > >         void (*putc)(struct stdio_dev *dev, const char c);
> > > > > > > >         /* To put a string (accelerator) */
> > > > > > > >         void (*puts)(struct stdio_dev *dev, const char *s);
> > > > > > > > +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> > > > > > >
> > > > > > > I'd argue it isn't worth the #ifdef and we might as well have this
> > > > > > > member here always. Then you can drop some #ifdefs from your code.
> > > > > >
> > > > > > But then it will increase binary code size.
> > > > >
> > > > > Using the member will increase code size. But I think the only place
> > > > > you need an #ifdef for that is when you include it in the driver
> > > > > struct. You can use __maybe_unused in the other place.
> > > > >
> > > > > Having the member will only increase memory usage, not code size.
> > > >
> > > > But static memory structures are part of the u-boot.bin binary and also
> > > > their usage increase code size (required copy, etc...), so at the end it
> > > > increase code size.
> > >
> > > From what I understand stdio_dev is allocated at runtime in BSS:
> > >
> > > struct stdio_dev *stdio_devices[] = { NULL, NULL, NULL };
> > >
> > > (the NULL stuff should not be there, but does nothing, I believe)
> > >
> > > So long as no code accesses your new member, then it should only
> > > affect the BSS size.
> >
> > Code of course has to access new member. How otherwise would be able to
> > call that new callbacks? E.g. all new code in patch 2/6 or 3/6.
> 
> Yes, see below. Also, do check whether it actually adds code or not.
> 
> >
> > > If you are very worried about it, you could use the technique in
> > > asm-generic/global_data.h to avoid #ifdefs in the C code:
> > >
> > > #ifdef CONFIG_GENERATE_ACPI_TABLE
> > > #define gd_acpi_start() gd->acpi_start
> > > #define gd_set_acpi_start(addr) gd->acpi_start = addr
> > > #else
> > > #define gd_acpi_start() 0UL
> > > #define gd_set_acpi_start(addr)
> > > #endif
> > >
> > > Regards,
> > > Simon
> 
> Regards,
> Simon

But in this case, it is needed to introduce a new special macro and hide
assigning code into it. In my opinion such macro with side effect and
worse than writing it explicitly - open coded to show what the code is
doing.

Re: [PATCH v2 2/6] console: Implement flush() function

[Simon Glass]

Hi Pali,

On Thu, 25 Aug 2022 at 08:20, Pali Rohár <pali@kernel.org> wrote:
>
> On Friday 12 August 2022 20:21:00 Simon Glass wrote:
> > Hi Pali,
> >
> > On Fri, 12 Aug 2022 at 09:17, Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Friday 12 August 2022 09:11:10 Simon Glass wrote:
> > > > Hi Pali,
> > > >
> > > > On Fri, 12 Aug 2022 at 03:01, Pali Rohár <pali@kernel.org> wrote:
> > > > >
> > > > > On Thursday 11 August 2022 18:08:46 Simon Glass wrote:
> > > > > > Hi Pali,
> > > > > >
> > > > > > On Thu, 11 Aug 2022 at 08:50, Pali Rohár <pali@kernel.org> wrote:
> > > > > > >
> > > > > > > On Thursday 11 August 2022 08:47:50 Simon Glass wrote:
> > > > > > > > > diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> > > > > > > > > index 270fa2729fb2..06278366ae88 100644
> > > > > > > > > --- a/include/stdio_dev.h
> > > > > > > > > +++ b/include/stdio_dev.h
> > > > > > > > > @@ -37,6 +37,10 @@ struct stdio_dev {
> > > > > > > > >         void (*putc)(struct stdio_dev *dev, const char c);
> > > > > > > > >         /* To put a string (accelerator) */
> > > > > > > > >         void (*puts)(struct stdio_dev *dev, const char *s);
> > > > > > > > > +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> > > > > > > >
> > > > > > > > I'd argue it isn't worth the #ifdef and we might as well have this
> > > > > > > > member here always. Then you can drop some #ifdefs from your code.
> > > > > > >
> > > > > > > But then it will increase binary code size.
> > > > > >
> > > > > > Using the member will increase code size. But I think the only place
> > > > > > you need an #ifdef for that is when you include it in the driver
> > > > > > struct. You can use __maybe_unused in the other place.
> > > > > >
> > > > > > Having the member will only increase memory usage, not code size.
> > > > >
> > > > > But static memory structures are part of the u-boot.bin binary and also
> > > > > their usage increase code size (required copy, etc...), so at the end it
> > > > > increase code size.
> > > >
> > > > From what I understand stdio_dev is allocated at runtime in BSS:
> > > >
> > > > struct stdio_dev *stdio_devices[] = { NULL, NULL, NULL };
> > > >
> > > > (the NULL stuff should not be there, but does nothing, I believe)
> > > >
> > > > So long as no code accesses your new member, then it should only
> > > > affect the BSS size.
> > >
> > > Code of course has to access new member. How otherwise would be able to
> > > call that new callbacks? E.g. all new code in patch 2/6 or 3/6.
> >
> > Yes, see below. Also, do check whether it actually adds code or not.
> >
> > >
> > > > If you are very worried about it, you could use the technique in
> > > > asm-generic/global_data.h to avoid #ifdefs in the C code:
> > > >
> > > > #ifdef CONFIG_GENERATE_ACPI_TABLE
> > > > #define gd_acpi_start() gd->acpi_start
> > > > #define gd_set_acpi_start(addr) gd->acpi_start = addr
> > > > #else
> > > > #define gd_acpi_start() 0UL
> > > > #define gd_set_acpi_start(addr)
> > > > #endif
> > > >
> > > > Regards,
> > > > Simon
> >
> > Regards,
> > Simon
>
> But in this case, it is needed to introduce a new special macro and hide
> assigning code into it. In my opinion such macro with side effect and
> worse than writing it explicitly - open coded to show what the code is
> doing.

You can use an inline function if you prefer.

My main objection is to adding #ifdefs to C files. They are OK (for
now) in driver struct declarations, where some members are optional.
But I think it is worth going to some lengths to avoid them elsewhere.

+Tom Rini for thoughts on this

Regards,
SImon

Re: [PATCH v2 2/6] console: Implement flush() function

[Pali Rohár]

On Thursday 25 August 2022 09:08:18 Simon Glass wrote:
> Hi Pali,
> 
> On Thu, 25 Aug 2022 at 08:20, Pali Rohár <pali@kernel.org> wrote:
> >
> > On Friday 12 August 2022 20:21:00 Simon Glass wrote:
> > > Hi Pali,
> > >
> > > On Fri, 12 Aug 2022 at 09:17, Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Friday 12 August 2022 09:11:10 Simon Glass wrote:
> > > > > Hi Pali,
> > > > >
> > > > > On Fri, 12 Aug 2022 at 03:01, Pali Rohár <pali@kernel.org> wrote:
> > > > > >
> > > > > > On Thursday 11 August 2022 18:08:46 Simon Glass wrote:
> > > > > > > Hi Pali,
> > > > > > >
> > > > > > > On Thu, 11 Aug 2022 at 08:50, Pali Rohár <pali@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Thursday 11 August 2022 08:47:50 Simon Glass wrote:
> > > > > > > > > > diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> > > > > > > > > > index 270fa2729fb2..06278366ae88 100644
> > > > > > > > > > --- a/include/stdio_dev.h
> > > > > > > > > > +++ b/include/stdio_dev.h
> > > > > > > > > > @@ -37,6 +37,10 @@ struct stdio_dev {
> > > > > > > > > >         void (*putc)(struct stdio_dev *dev, const char c);
> > > > > > > > > >         /* To put a string (accelerator) */
> > > > > > > > > >         void (*puts)(struct stdio_dev *dev, const char *s);
> > > > > > > > > > +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> > > > > > > > >
> > > > > > > > > I'd argue it isn't worth the #ifdef and we might as well have this
> > > > > > > > > member here always. Then you can drop some #ifdefs from your code.
> > > > > > > >
> > > > > > > > But then it will increase binary code size.
> > > > > > >
> > > > > > > Using the member will increase code size. But I think the only place
> > > > > > > you need an #ifdef for that is when you include it in the driver
> > > > > > > struct. You can use __maybe_unused in the other place.
> > > > > > >
> > > > > > > Having the member will only increase memory usage, not code size.
> > > > > >
> > > > > > But static memory structures are part of the u-boot.bin binary and also
> > > > > > their usage increase code size (required copy, etc...), so at the end it
> > > > > > increase code size.
> > > > >
> > > > > From what I understand stdio_dev is allocated at runtime in BSS:
> > > > >
> > > > > struct stdio_dev *stdio_devices[] = { NULL, NULL, NULL };
> > > > >
> > > > > (the NULL stuff should not be there, but does nothing, I believe)
> > > > >
> > > > > So long as no code accesses your new member, then it should only
> > > > > affect the BSS size.
> > > >
> > > > Code of course has to access new member. How otherwise would be able to
> > > > call that new callbacks? E.g. all new code in patch 2/6 or 3/6.
> > >
> > > Yes, see below. Also, do check whether it actually adds code or not.
> > >
> > > >
> > > > > If you are very worried about it, you could use the technique in
> > > > > asm-generic/global_data.h to avoid #ifdefs in the C code:
> > > > >
> > > > > #ifdef CONFIG_GENERATE_ACPI_TABLE
> > > > > #define gd_acpi_start() gd->acpi_start
> > > > > #define gd_set_acpi_start(addr) gd->acpi_start = addr
> > > > > #else
> > > > > #define gd_acpi_start() 0UL
> > > > > #define gd_set_acpi_start(addr)
> > > > > #endif
> > > > >
> > > > > Regards,
> > > > > Simon
> > >
> > > Regards,
> > > Simon
> >
> > But in this case, it is needed to introduce a new special macro and hide
> > assigning code into it. In my opinion such macro with side effect and
> > worse than writing it explicitly - open coded to show what the code is
> > doing.
> 
> You can use an inline function if you prefer.

But it has still same issue, hiding conditional logic indirectly to
additional file. Some members would be assigned directly via = operator
and some via macro/inlinefunction is inconsistent.

> My main objection is to adding #ifdefs to C files. They are OK (for
> now) in driver struct declarations, where some members are optional.
> But I think it is worth going to some lengths to avoid them elsewhere.
> 
> +Tom Rini for thoughts on this
> 
> Regards,
> SImon

Any comments on this?

Re: [PATCH v2 2/6] console: Implement flush() function

[Simon Glass]

Hi Pali,

On Wed, 31 Aug 2022 at 03:13, Pali Rohár <pali@kernel.org> wrote:
>
> On Thursday 25 August 2022 09:08:18 Simon Glass wrote:
> > Hi Pali,
> >
> > On Thu, 25 Aug 2022 at 08:20, Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Friday 12 August 2022 20:21:00 Simon Glass wrote:
> > > > Hi Pali,
> > > >
> > > > On Fri, 12 Aug 2022 at 09:17, Pali Rohár <pali@kernel.org> wrote:
> > > > >
> > > > > On Friday 12 August 2022 09:11:10 Simon Glass wrote:
> > > > > > Hi Pali,
> > > > > >
> > > > > > On Fri, 12 Aug 2022 at 03:01, Pali Rohár <pali@kernel.org> wrote:
> > > > > > >
> > > > > > > On Thursday 11 August 2022 18:08:46 Simon Glass wrote:
> > > > > > > > Hi Pali,
> > > > > > > >
> > > > > > > > On Thu, 11 Aug 2022 at 08:50, Pali Rohár <pali@kernel.org> wrote:
> > > > > > > > >
> > > > > > > > > On Thursday 11 August 2022 08:47:50 Simon Glass wrote:
> > > > > > > > > > > diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> > > > > > > > > > > index 270fa2729fb2..06278366ae88 100644
> > > > > > > > > > > --- a/include/stdio_dev.h
> > > > > > > > > > > +++ b/include/stdio_dev.h
> > > > > > > > > > > @@ -37,6 +37,10 @@ struct stdio_dev {
> > > > > > > > > > >         void (*putc)(struct stdio_dev *dev, const char c);
> > > > > > > > > > >         /* To put a string (accelerator) */
> > > > > > > > > > >         void (*puts)(struct stdio_dev *dev, const char *s);
> > > > > > > > > > > +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> > > > > > > > > >
> > > > > > > > > > I'd argue it isn't worth the #ifdef and we might as well have this
> > > > > > > > > > member here always. Then you can drop some #ifdefs from your code.
> > > > > > > > >
> > > > > > > > > But then it will increase binary code size.
> > > > > > > >
> > > > > > > > Using the member will increase code size. But I think the only place
> > > > > > > > you need an #ifdef for that is when you include it in the driver
> > > > > > > > struct. You can use __maybe_unused in the other place.
> > > > > > > >
> > > > > > > > Having the member will only increase memory usage, not code size.
> > > > > > >
> > > > > > > But static memory structures are part of the u-boot.bin binary and also
> > > > > > > their usage increase code size (required copy, etc...), so at the end it
> > > > > > > increase code size.
> > > > > >
> > > > > > From what I understand stdio_dev is allocated at runtime in BSS:
> > > > > >
> > > > > > struct stdio_dev *stdio_devices[] = { NULL, NULL, NULL };
> > > > > >
> > > > > > (the NULL stuff should not be there, but does nothing, I believe)
> > > > > >
> > > > > > So long as no code accesses your new member, then it should only
> > > > > > affect the BSS size.
> > > > >
> > > > > Code of course has to access new member. How otherwise would be able to
> > > > > call that new callbacks? E.g. all new code in patch 2/6 or 3/6.
> > > >
> > > > Yes, see below. Also, do check whether it actually adds code or not.
> > > >
> > > > >
> > > > > > If you are very worried about it, you could use the technique in
> > > > > > asm-generic/global_data.h to avoid #ifdefs in the C code:
> > > > > >
> > > > > > #ifdef CONFIG_GENERATE_ACPI_TABLE
> > > > > > #define gd_acpi_start() gd->acpi_start
> > > > > > #define gd_set_acpi_start(addr) gd->acpi_start = addr
> > > > > > #else
> > > > > > #define gd_acpi_start() 0UL
> > > > > > #define gd_set_acpi_start(addr)
> > > > > > #endif
> > > > > >
> > > > > > Regards,
> > > > > > Simon
> > > >
> > > > Regards,
> > > > Simon
> > >
> > > But in this case, it is needed to introduce a new special macro and hide
> > > assigning code into it. In my opinion such macro with side effect and
> > > worse than writing it explicitly - open coded to show what the code is
> > > doing.
> >
> > You can use an inline function if you prefer.
>
> But it has still same issue, hiding conditional logic indirectly to
> additional file. Some members would be assigned directly via = operator
> and some via macro/inlinefunction is inconsistent.

I've said everything I can say about this. If Tom has some thoughts
I'll leave it to him.

>
> > My main objection is to adding #ifdefs to C files. They are OK (for
> > now) in driver struct declarations, where some members are optional.
> > But I think it is worth going to some lengths to avoid them elsewhere.
> >
> > +Tom Rini for thoughts on this
> >
> > Regards,
> > SImon
>
> Any comments on this?

Regards,
Simon

Re: [PATCH v2 2/6] console: Implement flush() function

[Tom Rini]

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

On Wed, Aug 31, 2022 at 07:46:58AM -0600, Simon Glass wrote:
> Hi Pali,
> 
> On Wed, 31 Aug 2022 at 03:13, Pali Rohár <pali@kernel.org> wrote:
> >
> > On Thursday 25 August 2022 09:08:18 Simon Glass wrote:
> > > Hi Pali,
> > >
> > > On Thu, 25 Aug 2022 at 08:20, Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Friday 12 August 2022 20:21:00 Simon Glass wrote:
> > > > > Hi Pali,
> > > > >
> > > > > On Fri, 12 Aug 2022 at 09:17, Pali Rohár <pali@kernel.org> wrote:
> > > > > >
> > > > > > On Friday 12 August 2022 09:11:10 Simon Glass wrote:
> > > > > > > Hi Pali,
> > > > > > >
> > > > > > > On Fri, 12 Aug 2022 at 03:01, Pali Rohár <pali@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Thursday 11 August 2022 18:08:46 Simon Glass wrote:
> > > > > > > > > Hi Pali,
> > > > > > > > >
> > > > > > > > > On Thu, 11 Aug 2022 at 08:50, Pali Rohár <pali@kernel.org> wrote:
> > > > > > > > > >
> > > > > > > > > > On Thursday 11 August 2022 08:47:50 Simon Glass wrote:
> > > > > > > > > > > > diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> > > > > > > > > > > > index 270fa2729fb2..06278366ae88 100644
> > > > > > > > > > > > --- a/include/stdio_dev.h
> > > > > > > > > > > > +++ b/include/stdio_dev.h
> > > > > > > > > > > > @@ -37,6 +37,10 @@ struct stdio_dev {
> > > > > > > > > > > >         void (*putc)(struct stdio_dev *dev, const char c);
> > > > > > > > > > > >         /* To put a string (accelerator) */
> > > > > > > > > > > >         void (*puts)(struct stdio_dev *dev, const char *s);
> > > > > > > > > > > > +#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
> > > > > > > > > > >
> > > > > > > > > > > I'd argue it isn't worth the #ifdef and we might as well have this
> > > > > > > > > > > member here always. Then you can drop some #ifdefs from your code.
> > > > > > > > > >
> > > > > > > > > > But then it will increase binary code size.
> > > > > > > > >
> > > > > > > > > Using the member will increase code size. But I think the only place
> > > > > > > > > you need an #ifdef for that is when you include it in the driver
> > > > > > > > > struct. You can use __maybe_unused in the other place.
> > > > > > > > >
> > > > > > > > > Having the member will only increase memory usage, not code size.
> > > > > > > >
> > > > > > > > But static memory structures are part of the u-boot.bin binary and also
> > > > > > > > their usage increase code size (required copy, etc...), so at the end it
> > > > > > > > increase code size.
> > > > > > >
> > > > > > > From what I understand stdio_dev is allocated at runtime in BSS:
> > > > > > >
> > > > > > > struct stdio_dev *stdio_devices[] = { NULL, NULL, NULL };
> > > > > > >
> > > > > > > (the NULL stuff should not be there, but does nothing, I believe)
> > > > > > >
> > > > > > > So long as no code accesses your new member, then it should only
> > > > > > > affect the BSS size.
> > > > > >
> > > > > > Code of course has to access new member. How otherwise would be able to
> > > > > > call that new callbacks? E.g. all new code in patch 2/6 or 3/6.
> > > > >
> > > > > Yes, see below. Also, do check whether it actually adds code or not.
> > > > >
> > > > > >
> > > > > > > If you are very worried about it, you could use the technique in
> > > > > > > asm-generic/global_data.h to avoid #ifdefs in the C code:
> > > > > > >
> > > > > > > #ifdef CONFIG_GENERATE_ACPI_TABLE
> > > > > > > #define gd_acpi_start() gd->acpi_start
> > > > > > > #define gd_set_acpi_start(addr) gd->acpi_start = addr
> > > > > > > #else
> > > > > > > #define gd_acpi_start() 0UL
> > > > > > > #define gd_set_acpi_start(addr)
> > > > > > > #endif
> > > > > > >
> > > > > > > Regards,
> > > > > > > Simon
> > > > >
> > > > > Regards,
> > > > > Simon
> > > >
> > > > But in this case, it is needed to introduce a new special macro and hide
> > > > assigning code into it. In my opinion such macro with side effect and
> > > > worse than writing it explicitly - open coded to show what the code is
> > > > doing.
> > >
> > > You can use an inline function if you prefer.
> >
> > But it has still same issue, hiding conditional logic indirectly to
> > additional file. Some members would be assigned directly via = operator
> > and some via macro/inlinefunction is inconsistent.
> 
> I've said everything I can say about this. If Tom has some thoughts
> I'll leave it to him.
> 
> >
> > > My main objection is to adding #ifdefs to C files. They are OK (for
> > > now) in driver struct declarations, where some members are optional.
> > > But I think it is worth going to some lengths to avoid them elsewhere.
> > >
> > > +Tom Rini for thoughts on this
> > >
> > > Regards,
> > > SImon
> >
> > Any comments on this?

Given the plethora of tools to demonstrate that approach X results in
size change Y, I'm not sure why we're pondering if something will or
won't result in increases? I would like to see this series result in the
smallest increase in binary size on platforms that don't enable the
feature. An #ifdef isn't more or less readable than using some macro
instead, and decorators like __maybe_unused have their own impact on
code readability.

-- 
Tom

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