| Message ID | 20220811123925.23981-3-pali@kernel.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | console: Implement flush() function | expand |
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
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
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
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
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
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
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
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.
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
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?
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
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.
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 */
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(+)