[U-Boot] [PATCH v2 1/3] display_options: Refactor to allow obtaining the banner
Move the display options code into a separate function so that the U-Boot banner can be obtained from other code. Adjust the 'version' command to use it.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Update the code to be more testable - Avoid a warning on spring, etc.
cmd/version.c | 4 +++- include/display_options.h | 19 +++++++++++++++++++ lib/display_options.c | 36 +++++++++++++++++++++++++++++++----- 3 files changed, 53 insertions(+), 6 deletions(-)
diff --git a/cmd/version.c b/cmd/version.c index 1be0667f09..15aab5dc18 100644 --- a/cmd/version.c +++ b/cmd/version.c @@ -17,7 +17,9 @@ const char __weak version_string[] = U_BOOT_VERSION_STRING;
static int do_version(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { - printf("\n%s\n", version_string); + char buf[DISPLAY_OPTIONS_BANNER_LENGTH]; + + printf(display_options_get_banner(false, buf, sizeof(buf))); #ifdef CC_VERSION_STRING puts(CC_VERSION_STRING "\n"); #endif diff --git a/include/display_options.h b/include/display_options.h index ac44c459b3..ad707a344c 100644 --- a/include/display_options.h +++ b/include/display_options.h @@ -56,4 +56,23 @@ int print_buffer(ulong addr, const void *data, uint width, uint count, */ int display_options(void);
+/* Suggested length of the buffer to pass to display_options_get_banner() */ +#define DISPLAY_OPTIONS_BANNER_LENGTH 200 + +/** + * display_options_get_banner() - Get the U-Boot banner as a string + * + * This returns the U-Boot banner string + * + * @newlines: true to include two newlines at the start + * @buf: place to put string + * @size: Size of buf (string is truncated to fit) + * @return buf + */ +char *display_options_get_banner(bool newlines, char *buf, int size); + +/** This function is used for testing only */ +char *display_options_get_banner_priv(bool newlines, const char *build_tag, + char *buf, int size); + #endif diff --git a/lib/display_options.c b/lib/display_options.c index 29343fc00e..4ea27ca99d 100644 --- a/lib/display_options.c +++ b/lib/display_options.c @@ -13,13 +13,39 @@ #include <linux/ctype.h> #include <asm/io.h>
-int display_options (void) +char *display_options_get_banner_priv(bool newlines, const char *build_tag, + char *buf, int size) { -#if defined(BUILD_TAG) - printf ("\n\n%s, Build: %s\n\n", version_string, BUILD_TAG); -#else - printf ("\n\n%s\n\n", version_string); + int len; + + len = snprintf(buf, size, "%s%s", newlines ? "\n\n" : "", + version_string); + if (build_tag && len < size) + len += snprintf(buf + len, size - len, ", Build: %s", + build_tag); + if (len > size - 3) + len = size - 3; + strcpy(buf + len, "\n\n"); + + return buf; +} + +#ifndef BUILD_TAG +#define BUILD_TAG NULL #endif + +char *display_options_get_banner(bool newlines, char *buf, int size) +{ + return display_options_get_banner_priv(newlines, BUILD_TAG, buf, size); +} + +int display_options(void) +{ + char buf[DISPLAY_OPTIONS_BANNER_LENGTH]; + + display_options_get_banner(true, buf, sizeof(buf)); + printf("%s", buf); + return 0; }
At present the U-Boot banner is only displayed on the serial console. If this is not visible to the user, the banner does not show. Some devices have a video display which can usefully display this information.
Add a banner which is printed after relocation only on non-serial devices if CONFIG_DISPLAY_BOARDINFO_LATE is defined.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Reword function comment for console_announce_r() slighty
common/board_r.c | 1 + common/console.c | 15 +++++++++++---- include/console.h | 12 ++++++++++++ 3 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/common/board_r.c b/common/board_r.c index 15977e4bca..ff11eba5d3 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -846,6 +846,7 @@ static init_fnc_t init_sequence_r[] = { #endif console_init_r, /* fully init console as a device */ #ifdef CONFIG_DISPLAY_BOARDINFO_LATE + console_announce_r, show_board_info, #endif #ifdef CONFIG_ARCH_MISC_INIT diff --git a/common/console.c b/common/console.c index 1232808df5..3fcd7ce66b 100644 --- a/common/console.c +++ b/common/console.c @@ -202,7 +202,6 @@ static void console_putc(int file, const char c) } }
-#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER) static void console_puts_noserial(int file, const char *s) { int i; @@ -214,7 +213,6 @@ static void console_puts_noserial(int file, const char *s) dev->puts(dev, s); } } -#endif
static void console_puts(int file, const char *s) { @@ -248,13 +246,11 @@ static inline void console_putc(int file, const char c) stdio_devices[file]->putc(stdio_devices[file], c); }
-#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER) static inline void console_puts_noserial(int file, const char *s) { if (strcmp(stdio_devices[file]->name, "serial") != 0) stdio_devices[file]->puts(stdio_devices[file], s); } -#endif
static inline void console_puts(int file, const char *s) { @@ -699,6 +695,17 @@ static void console_update_silent(void) #endif }
+int console_announce_r(void) +{ + char buf[DISPLAY_OPTIONS_BANNER_LENGTH]; + + display_options_get_banner(false, buf, sizeof(buf)); + + console_puts_noserial(stdout, buf); + + return 0; +} + /* Called before relocation - use serial functions */ int console_init_f(void) { diff --git a/include/console.h b/include/console.h index 3d37f6a53b..cea29ed6dc 100644 --- a/include/console.h +++ b/include/console.h @@ -42,6 +42,18 @@ void console_record_reset(void); */ void console_record_reset_enable(void);
+/** + * console_announce_r() - print a U-Boot console on non-serial consoles + * + * When U-Boot starts up with a display it generally does not announce itself + * on the display. The banner is instead emitted on the UART before relocation. + * This function prints a banner on devices which (we assume) did not receive + * it before relocation. + * + * @return 0 (meaning no errors) + */ +int console_announce_r(void); + /* * CONSOLE multiplexing. */
Hi Simon,
On Sun, Jun 11, 2017 at 1:59 AM, Simon Glass sjg@chromium.org wrote:
At present the U-Boot banner is only displayed on the serial console. If this is not visible to the user, the banner does not show. Some devices have a video display which can usefully display this information.
Add a banner which is printed after relocation only on non-serial devices if CONFIG_DISPLAY_BOARDINFO_LATE is defined.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Reword function comment for console_announce_r() slighty
common/board_r.c | 1 + common/console.c | 15 +++++++++++---- include/console.h | 12 ++++++++++++ 3 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/common/board_r.c b/common/board_r.c index 15977e4bca..ff11eba5d3 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -846,6 +846,7 @@ static init_fnc_t init_sequence_r[] = { #endif console_init_r, /* fully init console as a device */ #ifdef CONFIG_DISPLAY_BOARDINFO_LATE
console_announce_r, show_board_info,#endif #ifdef CONFIG_ARCH_MISC_INIT diff --git a/common/console.c b/common/console.c index 1232808df5..3fcd7ce66b 100644 --- a/common/console.c +++ b/common/console.c @@ -202,7 +202,6 @@ static void console_putc(int file, const char c) } }
-#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER)
Why removing this? Without PRE_CONSOLE_BUFFER, I doubt it works.
static void console_puts_noserial(int file, const char *s) { int i; @@ -214,7 +213,6 @@ static void console_puts_noserial(int file, const char *s) dev->puts(dev, s); } } -#endif
static void console_puts(int file, const char *s) { @@ -248,13 +246,11 @@ static inline void console_putc(int file, const char c) stdio_devices[file]->putc(stdio_devices[file], c); }
-#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER) static inline void console_puts_noserial(int file, const char *s) { if (strcmp(stdio_devices[file]->name, "serial") != 0) stdio_devices[file]->puts(stdio_devices[file], s); } -#endif
static inline void console_puts(int file, const char *s) { @@ -699,6 +695,17 @@ static void console_update_silent(void) #endif }
+int console_announce_r(void) +{
char buf[DISPLAY_OPTIONS_BANNER_LENGTH];display_options_get_banner(false, buf, sizeof(buf));console_puts_noserial(stdout, buf);return 0;+}
/* Called before relocation - use serial functions */ int console_init_f(void) { diff --git a/include/console.h b/include/console.h index 3d37f6a53b..cea29ed6dc 100644 --- a/include/console.h +++ b/include/console.h @@ -42,6 +42,18 @@ void console_record_reset(void); */ void console_record_reset_enable(void);
+/**
- console_announce_r() - print a U-Boot console on non-serial consoles
- When U-Boot starts up with a display it generally does not announce itself
- on the display. The banner is instead emitted on the UART before relocation.
- This function prints a banner on devices which (we assume) did not receive
- it before relocation.
- @return 0 (meaning no errors)
- */
+int console_announce_r(void);
/*
- CONSOLE multiplexing.
*/
And I see another (same) patch @ https://patchwork.ozlabs.org/patch/769426/ which got applied, but this one was sent later than the applied one. I am confused..
Regards, Bin
Hi Bin,
On 12 June 2017 at 09:13, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Sun, Jun 11, 2017 at 1:59 AM, Simon Glass sjg@chromium.org wrote:
At present the U-Boot banner is only displayed on the serial console. If this is not visible to the user, the banner does not show. Some devices have a video display which can usefully display this information.
Add a banner which is printed after relocation only on non-serial devices if CONFIG_DISPLAY_BOARDINFO_LATE is defined.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Reword function comment for console_announce_r() slighty
common/board_r.c | 1 + common/console.c | 15 +++++++++++---- include/console.h | 12 ++++++++++++ 3 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/common/board_r.c b/common/board_r.c index 15977e4bca..ff11eba5d3 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -846,6 +846,7 @@ static init_fnc_t init_sequence_r[] = { #endif console_init_r, /* fully init console as a device */ #ifdef CONFIG_DISPLAY_BOARDINFO_LATE
console_announce_r, show_board_info,#endif #ifdef CONFIG_ARCH_MISC_INIT diff --git a/common/console.c b/common/console.c index 1232808df5..3fcd7ce66b 100644 --- a/common/console.c +++ b/common/console.c @@ -202,7 +202,6 @@ static void console_putc(int file, const char c) } }
-#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER)
Why removing this? Without PRE_CONSOLE_BUFFER, I doubt it works.
I want to be able to call the function below. It seems to work OK and it doesn't rely on that option.
static void console_puts_noserial(int file, const char *s) { int i; @@ -214,7 +213,6 @@ static void console_puts_noserial(int file, const char *s) dev->puts(dev, s); } } -#endif
static void console_puts(int file, const char *s) { @@ -248,13 +246,11 @@ static inline void console_putc(int file, const char c) stdio_devices[file]->putc(stdio_devices[file], c); }
-#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER) static inline void console_puts_noserial(int file, const char *s) { if (strcmp(stdio_devices[file]->name, "serial") != 0) stdio_devices[file]->puts(stdio_devices[file], s); } -#endif
static inline void console_puts(int file, const char *s) { @@ -699,6 +695,17 @@ static void console_update_silent(void) #endif }
+int console_announce_r(void) +{
char buf[DISPLAY_OPTIONS_BANNER_LENGTH];display_options_get_banner(false, buf, sizeof(buf));console_puts_noserial(stdout, buf);return 0;+}
/* Called before relocation - use serial functions */ int console_init_f(void) { diff --git a/include/console.h b/include/console.h index 3d37f6a53b..cea29ed6dc 100644 --- a/include/console.h +++ b/include/console.h @@ -42,6 +42,18 @@ void console_record_reset(void); */ void console_record_reset_enable(void);
+/**
- console_announce_r() - print a U-Boot console on non-serial consoles
- When U-Boot starts up with a display it generally does not announce itself
- on the display. The banner is instead emitted on the UART before relocation.
- This function prints a banner on devices which (we assume) did not receive
- it before relocation.
- @return 0 (meaning no errors)
- */
+int console_announce_r(void);
/*
- CONSOLE multiplexing.
*/
And I see another (same) patch @ https://patchwork.ozlabs.org/patch/769426/ which got applied, but this one was sent later than the applied one. I am confused..
Another patch in this series caused a breakage using a long BUILD_TAG due to a bug in how the snprintf() calls were done. So I dropped two patches from that series and sent this series instead.
It wasn't noticed since I don't use BUILD_TAG and no tests were in place. It just happened that Stephen Warren has automated testing that caught it before it was pulled to mainline.
Regards, Simon
Hi Simon,
On Mon, Jun 12, 2017 at 11:17 PM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 12 June 2017 at 09:13, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Sun, Jun 11, 2017 at 1:59 AM, Simon Glass sjg@chromium.org wrote:
At present the U-Boot banner is only displayed on the serial console. If this is not visible to the user, the banner does not show. Some devices have a video display which can usefully display this information.
Add a banner which is printed after relocation only on non-serial devices if CONFIG_DISPLAY_BOARDINFO_LATE is defined.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Reword function comment for console_announce_r() slighty
common/board_r.c | 1 + common/console.c | 15 +++++++++++---- include/console.h | 12 ++++++++++++ 3 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/common/board_r.c b/common/board_r.c index 15977e4bca..ff11eba5d3 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -846,6 +846,7 @@ static init_fnc_t init_sequence_r[] = { #endif console_init_r, /* fully init console as a device */ #ifdef CONFIG_DISPLAY_BOARDINFO_LATE
console_announce_r, show_board_info,#endif #ifdef CONFIG_ARCH_MISC_INIT diff --git a/common/console.c b/common/console.c index 1232808df5..3fcd7ce66b 100644 --- a/common/console.c +++ b/common/console.c @@ -202,7 +202,6 @@ static void console_putc(int file, const char c) } }
-#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER)
Why removing this? Without PRE_CONSOLE_BUFFER, I doubt it works.
I want to be able to call the function below. It seems to work OK and it doesn't rely on that option.
See comments below.
static void console_puts_noserial(int file, const char *s) { int i; @@ -214,7 +213,6 @@ static void console_puts_noserial(int file, const char *s) dev->puts(dev, s); } } -#endif
static void console_puts(int file, const char *s) { @@ -248,13 +246,11 @@ static inline void console_putc(int file, const char c) stdio_devices[file]->putc(stdio_devices[file], c); }
-#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER) static inline void console_puts_noserial(int file, const char *s) { if (strcmp(stdio_devices[file]->name, "serial") != 0) stdio_devices[file]->puts(stdio_devices[file], s); } -#endif
static inline void console_puts(int file, const char *s) { @@ -699,6 +695,17 @@ static void console_update_silent(void) #endif }
+int console_announce_r(void) +{
char buf[DISPLAY_OPTIONS_BANNER_LENGTH];display_options_get_banner(false, buf, sizeof(buf));console_puts_noserial(stdout, buf);
Then I think we should do something like this:
int console_announce_r(void) { #ifndef CONFIG_PRE_CONSOLE_BUFFER char buf[DISPLAY_OPTIONS_BANNER_LENGTH];
display_options_get_banner(false, buf, sizeof(buf)); console_puts_noserial(stdout, buf); #endif }
If we don't do this, there will be duplicated U-Boot version string shown on the screen if CONFIG_PRE_CONSOLE_BUFFER is defined.
return 0;+}
/* Called before relocation - use serial functions */ int console_init_f(void) { diff --git a/include/console.h b/include/console.h index 3d37f6a53b..cea29ed6dc 100644 --- a/include/console.h +++ b/include/console.h @@ -42,6 +42,18 @@ void console_record_reset(void); */ void console_record_reset_enable(void);
+/**
- console_announce_r() - print a U-Boot console on non-serial consoles
- When U-Boot starts up with a display it generally does not announce itself
- on the display. The banner is instead emitted on the UART before relocation.
- This function prints a banner on devices which (we assume) did not receive
- it before relocation.
- @return 0 (meaning no errors)
- */
+int console_announce_r(void);
/*
- CONSOLE multiplexing.
*/
And I see another (same) patch @ https://patchwork.ozlabs.org/patch/769426/ which got applied, but this one was sent later than the applied one. I am confused..
Another patch in this series caused a breakage using a long BUILD_TAG due to a bug in how the snprintf() calls were done. So I dropped two patches from that series and sent this series instead.
It wasn't noticed since I don't use BUILD_TAG and no tests were in place. It just happened that Stephen Warren has automated testing that caught it before it was pulled to mainline.
OK, thanks for the clarification.
Regards, Bin
Hi Bin,
On 12 June 2017 at 19:10, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Mon, Jun 12, 2017 at 11:17 PM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 12 June 2017 at 09:13, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Sun, Jun 11, 2017 at 1:59 AM, Simon Glass sjg@chromium.org wrote:
At present the U-Boot banner is only displayed on the serial console. If this is not visible to the user, the banner does not show. Some devices have a video display which can usefully display this information.
Add a banner which is printed after relocation only on non-serial devices if CONFIG_DISPLAY_BOARDINFO_LATE is defined.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Reword function comment for console_announce_r() slighty
common/board_r.c | 1 + common/console.c | 15 +++++++++++---- include/console.h | 12 ++++++++++++ 3 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/common/board_r.c b/common/board_r.c index 15977e4bca..ff11eba5d3 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -846,6 +846,7 @@ static init_fnc_t init_sequence_r[] = { #endif console_init_r, /* fully init console as a device */ #ifdef CONFIG_DISPLAY_BOARDINFO_LATE
console_announce_r, show_board_info,#endif #ifdef CONFIG_ARCH_MISC_INIT diff --git a/common/console.c b/common/console.c index 1232808df5..3fcd7ce66b 100644 --- a/common/console.c +++ b/common/console.c @@ -202,7 +202,6 @@ static void console_putc(int file, const char c) } }
-#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER)
Why removing this? Without PRE_CONSOLE_BUFFER, I doubt it works.
I want to be able to call the function below. It seems to work OK and it doesn't rely on that option.
See comments below.
static void console_puts_noserial(int file, const char *s) { int i; @@ -214,7 +213,6 @@ static void console_puts_noserial(int file, const char *s) dev->puts(dev, s); } } -#endif
static void console_puts(int file, const char *s) { @@ -248,13 +246,11 @@ static inline void console_putc(int file, const char c) stdio_devices[file]->putc(stdio_devices[file], c); }
-#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER) static inline void console_puts_noserial(int file, const char *s) { if (strcmp(stdio_devices[file]->name, "serial") != 0) stdio_devices[file]->puts(stdio_devices[file], s); } -#endif
static inline void console_puts(int file, const char *s) { @@ -699,6 +695,17 @@ static void console_update_silent(void) #endif }
+int console_announce_r(void) +{
char buf[DISPLAY_OPTIONS_BANNER_LENGTH];display_options_get_banner(false, buf, sizeof(buf));console_puts_noserial(stdout, buf);Then I think we should do something like this:
int console_announce_r(void) { #ifndef CONFIG_PRE_CONSOLE_BUFFER char buf[DISPLAY_OPTIONS_BANNER_LENGTH];
display_options_get_banner(false, buf, sizeof(buf)); console_puts_noserial(stdout, buf);#endif }
If we don't do this, there will be duplicated U-Boot version string shown on the screen if CONFIG_PRE_CONSOLE_BUFFER is defined.
I don't think so. This is a different thing. The pre-console buffer only exists until console_init_f(), i.e. very early in pre-relocation U-Boot. When console_init_f() is called the buffer is printed to the (serial) console.
My patch is all about what happens after relocation. By this time the pre-console buffer has been sent to the serial port. I want to display the version number on the screen, and this should not affect the pre-console buffer in any way.
return 0;+}
/* Called before relocation - use serial functions */ int console_init_f(void) { diff --git a/include/console.h b/include/console.h index 3d37f6a53b..cea29ed6dc 100644 --- a/include/console.h +++ b/include/console.h @@ -42,6 +42,18 @@ void console_record_reset(void); */ void console_record_reset_enable(void);
+/**
- console_announce_r() - print a U-Boot console on non-serial consoles
- When U-Boot starts up with a display it generally does not announce itself
- on the display. The banner is instead emitted on the UART before relocation.
- This function prints a banner on devices which (we assume) did not receive
- it before relocation.
- @return 0 (meaning no errors)
- */
+int console_announce_r(void);
/*
- CONSOLE multiplexing.
*/
And I see another (same) patch @ https://patchwork.ozlabs.org/patch/769426/ which got applied, but this one was sent later than the applied one. I am confused..
Another patch in this series caused a breakage using a long BUILD_TAG due to a bug in how the snprintf() calls were done. So I dropped two patches from that series and sent this series instead.
It wasn't noticed since I don't use BUILD_TAG and no tests were in place. It just happened that Stephen Warren has automated testing that caught it before it was pulled to mainline.
OK, thanks for the clarification.
Regards, Bin
Regards, Simon
Hi Simon,
On Fri, Jun 16, 2017 at 12:09 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 12 June 2017 at 19:10, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Mon, Jun 12, 2017 at 11:17 PM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 12 June 2017 at 09:13, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Sun, Jun 11, 2017 at 1:59 AM, Simon Glass sjg@chromium.org wrote:
At present the U-Boot banner is only displayed on the serial console. If this is not visible to the user, the banner does not show. Some devices have a video display which can usefully display this information.
Add a banner which is printed after relocation only on non-serial devices if CONFIG_DISPLAY_BOARDINFO_LATE is defined.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Reword function comment for console_announce_r() slighty
common/board_r.c | 1 + common/console.c | 15 +++++++++++---- include/console.h | 12 ++++++++++++ 3 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/common/board_r.c b/common/board_r.c index 15977e4bca..ff11eba5d3 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -846,6 +846,7 @@ static init_fnc_t init_sequence_r[] = { #endif console_init_r, /* fully init console as a device */ #ifdef CONFIG_DISPLAY_BOARDINFO_LATE
console_announce_r, show_board_info,#endif #ifdef CONFIG_ARCH_MISC_INIT diff --git a/common/console.c b/common/console.c index 1232808df5..3fcd7ce66b 100644 --- a/common/console.c +++ b/common/console.c @@ -202,7 +202,6 @@ static void console_putc(int file, const char c) } }
-#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER)
Why removing this? Without PRE_CONSOLE_BUFFER, I doubt it works.
I want to be able to call the function below. It seems to work OK and it doesn't rely on that option.
See comments below.
static void console_puts_noserial(int file, const char *s) { int i; @@ -214,7 +213,6 @@ static void console_puts_noserial(int file, const char *s) dev->puts(dev, s); } } -#endif
static void console_puts(int file, const char *s) { @@ -248,13 +246,11 @@ static inline void console_putc(int file, const char c) stdio_devices[file]->putc(stdio_devices[file], c); }
-#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER) static inline void console_puts_noserial(int file, const char *s) { if (strcmp(stdio_devices[file]->name, "serial") != 0) stdio_devices[file]->puts(stdio_devices[file], s); } -#endif
static inline void console_puts(int file, const char *s) { @@ -699,6 +695,17 @@ static void console_update_silent(void) #endif }
+int console_announce_r(void) +{
char buf[DISPLAY_OPTIONS_BANNER_LENGTH];display_options_get_banner(false, buf, sizeof(buf));console_puts_noserial(stdout, buf);Then I think we should do something like this:
int console_announce_r(void) { #ifndef CONFIG_PRE_CONSOLE_BUFFER char buf[DISPLAY_OPTIONS_BANNER_LENGTH];
display_options_get_banner(false, buf, sizeof(buf)); console_puts_noserial(stdout, buf);#endif }
If we don't do this, there will be duplicated U-Boot version string shown on the screen if CONFIG_PRE_CONSOLE_BUFFER is defined.
I don't think so. This is a different thing. The pre-console buffer only exists until console_init_f(), i.e. very early in pre-relocation U-Boot. When console_init_f() is called the buffer is printed to the (serial) console.
But in fact, the version string will be printed twice on the screen (not on the serial) if CONFIG_PRE_CONSOLE_BUFFER is defined. One existed in the pre-console buffer, as you mentioned. The other one is what you added here.
My patch is all about what happens after relocation. By this time the pre-console buffer has been sent to the serial port. I want to display the version number on the screen, and this should not affect the pre-console buffer in any way.
[snip]
Regards, Bin
Hi Bin,
On 15 June 2017 at 19:12, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Fri, Jun 16, 2017 at 12:09 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 12 June 2017 at 19:10, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Mon, Jun 12, 2017 at 11:17 PM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 12 June 2017 at 09:13, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Sun, Jun 11, 2017 at 1:59 AM, Simon Glass sjg@chromium.org wrote:
At present the U-Boot banner is only displayed on the serial console. If this is not visible to the user, the banner does not show. Some devices have a video display which can usefully display this information.
Add a banner which is printed after relocation only on non-serial devices if CONFIG_DISPLAY_BOARDINFO_LATE is defined.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Reword function comment for console_announce_r() slighty
common/board_r.c | 1 + common/console.c | 15 +++++++++++---- include/console.h | 12 ++++++++++++ 3 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/common/board_r.c b/common/board_r.c index 15977e4bca..ff11eba5d3 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -846,6 +846,7 @@ static init_fnc_t init_sequence_r[] = { #endif console_init_r, /* fully init console as a device */ #ifdef CONFIG_DISPLAY_BOARDINFO_LATE
console_announce_r, show_board_info,#endif #ifdef CONFIG_ARCH_MISC_INIT diff --git a/common/console.c b/common/console.c index 1232808df5..3fcd7ce66b 100644 --- a/common/console.c +++ b/common/console.c @@ -202,7 +202,6 @@ static void console_putc(int file, const char c) } }
-#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER)
Why removing this? Without PRE_CONSOLE_BUFFER, I doubt it works.
I want to be able to call the function below. It seems to work OK and it doesn't rely on that option.
See comments below.
static void console_puts_noserial(int file, const char *s) { int i; @@ -214,7 +213,6 @@ static void console_puts_noserial(int file, const char *s) dev->puts(dev, s); } } -#endif
static void console_puts(int file, const char *s) { @@ -248,13 +246,11 @@ static inline void console_putc(int file, const char c) stdio_devices[file]->putc(stdio_devices[file], c); }
-#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER) static inline void console_puts_noserial(int file, const char *s) { if (strcmp(stdio_devices[file]->name, "serial") != 0) stdio_devices[file]->puts(stdio_devices[file], s); } -#endif
static inline void console_puts(int file, const char *s) { @@ -699,6 +695,17 @@ static void console_update_silent(void) #endif }
+int console_announce_r(void) +{
char buf[DISPLAY_OPTIONS_BANNER_LENGTH];display_options_get_banner(false, buf, sizeof(buf));console_puts_noserial(stdout, buf);Then I think we should do something like this:
int console_announce_r(void) { #ifndef CONFIG_PRE_CONSOLE_BUFFER char buf[DISPLAY_OPTIONS_BANNER_LENGTH];
display_options_get_banner(false, buf, sizeof(buf)); console_puts_noserial(stdout, buf);#endif }
If we don't do this, there will be duplicated U-Boot version string shown on the screen if CONFIG_PRE_CONSOLE_BUFFER is defined.
I don't think so. This is a different thing. The pre-console buffer only exists until console_init_f(), i.e. very early in pre-relocation U-Boot. When console_init_f() is called the buffer is printed to the (serial) console.
But in fact, the version string will be printed twice on the screen (not on the serial) if CONFIG_PRE_CONSOLE_BUFFER is defined. One existed in the pre-console buffer, as you mentioned. The other one is what you added here.
OK I see what you mean. I didn't know (or forgot) that the pre-console buffer emitted its output later also.
I think I'll enable it for sandbox and try to rationalise the code a little. Thanks for spotting this!
My patch is all about what happens after relocation. By this time the pre-console buffer has been sent to the serial port. I want to display the version number on the screen, and this should not affect the pre-console buffer in any way.
[snip]
Regards, Bin
Regards, Simon
Add a simple test to make sure that these functions obey the buffer size passed into them.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Fix buffer overflow problem when there is not enough space for the build tag - Add test to check for buffer overflow problems
test/Makefile | 1 + test/print_ut.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+) create mode 100644 test/print_ut.c
diff --git a/test/Makefile b/test/Makefile index 0f5de57399..6305afb211 100644 --- a/test/Makefile +++ b/test/Makefile @@ -8,4 +8,5 @@ obj-$(CONFIG_UNIT_TEST) += cmd_ut.o obj-$(CONFIG_UNIT_TEST) += ut.o obj-$(CONFIG_SANDBOX) += command_ut.o obj-$(CONFIG_SANDBOX) += compression.o +obj-$(CONFIG_SANDBOX) += print_ut.o obj-$(CONFIG_UT_TIME) += time_ut.o diff --git a/test/print_ut.c b/test/print_ut.c new file mode 100644 index 0000000000..baad289972 --- /dev/null +++ b/test/print_ut.c @@ -0,0 +1,83 @@ +/* + * Copyright (c) 2012, The Chromium Authors + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#define DEBUG + +#include <common.h> +#include <display_options.h> +#include <version.h> + +#define FAKE_BUILD_TAG "jenkins-u-boot-denx_uboot_dm-master-build-aarch64" \ + "and a lot more text to come" + +static int do_ut_print(cmd_tbl_t *cmdtp, int flag, int argc, + char *const argv[]) +{ + char big_str[400]; + int big_str_len; + char str[10], *s; + int len; + + printf("%s: Testing print\n", __func__); + + snprintf(str, sizeof(str), "testing"); + assert(!strcmp("testing", str)); + + snprintf(str, sizeof(str), "testing but too long"); + assert(!strcmp("testing b", str)); + + snprintf(str, 1, "testing none"); + assert(!strcmp("", str)); + + *str = 'x'; + snprintf(str, 0, "testing none"); + assert(*str == 'x'); + + /* Test the banner function */ + s = display_options_get_banner(true, str, sizeof(str)); + assert(s == str); + assert(!strcmp("\n\nU-Boo\n\n", s)); + + s = display_options_get_banner(true, str, 1); + assert(s == str); + assert(!strcmp("", s)); + + s = display_options_get_banner(true, str, 2); + assert(s == str); + assert(!strcmp("\n", s)); + + s = display_options_get_banner(false, str, sizeof(str)); + assert(s == str); + assert(!strcmp("U-Boot \n\n", s)); + + /* Give it enough space for some of the version */ + big_str_len = strlen(version_string) - 5; + s = display_options_get_banner_priv(false, FAKE_BUILD_TAG, big_str, + big_str_len); + assert(s == big_str); + assert(!strncmp(version_string, s, big_str_len - 3)); + assert(!strcmp("\n\n", s + big_str_len - 3)); + + /* Give it enough space for the version and some of the build tag */ + big_str_len = strlen(version_string) + 9 + 20; + s = display_options_get_banner_priv(false, FAKE_BUILD_TAG, big_str, + big_str_len); + assert(s == big_str); + len = strlen(version_string); + assert(!strncmp(version_string, s, len)); + assert(!strncmp(", Build: ", s + len, 9)); + assert(!strncmp(FAKE_BUILD_TAG, s + 9 + len, 12)); + assert(!strcmp("\n\n", s + big_str_len - 3)); + + printf("%s: Everything went swimmingly\n", __func__); + return 0; +} + +U_BOOT_CMD( + ut_print, 1, 1, do_ut_print, + "Very basic test of printf(), etc.", + "" +);
On Sun, Jun 11, 2017 at 1:59 AM, Simon Glass sjg@chromium.org wrote:
Add a simple test to make sure that these functions obey the buffer size passed into them.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Fix buffer overflow problem when there is not enough space for the build tag
- Add test to check for buffer overflow problems
test/Makefile | 1 + test/print_ut.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+) create mode 100644 test/print_ut.c
Reviewed-by: Bin Meng bmeng.cn@gmail.com
On Sun, Jun 11, 2017 at 1:59 AM, Simon Glass sjg@chromium.org wrote:
Move the display options code into a separate function so that the U-Boot banner can be obtained from other code. Adjust the 'version' command to use it.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Update the code to be more testable
- Avoid a warning on spring, etc.
cmd/version.c | 4 +++- include/display_options.h | 19 +++++++++++++++++++ lib/display_options.c | 36 +++++++++++++++++++++++++++++++----- 3 files changed, 53 insertions(+), 6 deletions(-)
Reviewed-by: Bin Meng bmeng.cn@gmail.com
One nits below.
diff --git a/cmd/version.c b/cmd/version.c index 1be0667f09..15aab5dc18 100644 --- a/cmd/version.c +++ b/cmd/version.c @@ -17,7 +17,9 @@ const char __weak version_string[] = U_BOOT_VERSION_STRING;
static int do_version(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) {
printf("\n%s\n", version_string);
char buf[DISPLAY_OPTIONS_BANNER_LENGTH];printf(display_options_get_banner(false, buf, sizeof(buf)));#ifdef CC_VERSION_STRING puts(CC_VERSION_STRING "\n"); #endif diff --git a/include/display_options.h b/include/display_options.h index ac44c459b3..ad707a344c 100644 --- a/include/display_options.h +++ b/include/display_options.h @@ -56,4 +56,23 @@ int print_buffer(ulong addr, const void *data, uint width, uint count, */ int display_options(void);
+/* Suggested length of the buffer to pass to display_options_get_banner() */ +#define DISPLAY_OPTIONS_BANNER_LENGTH 200
+/**
- display_options_get_banner() - Get the U-Boot banner as a string
- This returns the U-Boot banner string
- @newlines: true to include two newlines at the start
- @buf: place to put string
- @size: Size of buf (string is truncated to fit)
- @return buf
- */
+char *display_options_get_banner(bool newlines, char *buf, int size);
+/** This function is used for testing only */
nits: should be /*
[snip]
Regards, Bin
On 06/10/2017 11:59 AM, Simon Glass wrote:
Move the display options code into a separate function so that the U-Boot banner can be obtained from other code. Adjust the 'version' command to use it.
This series passed my automated testing, so: Tested-by: Stephen Warren swarren@nvidia.com
On Tue, Jun 13, 2017 at 3:28 AM, Stephen Warren swarren@wwwdotorg.org wrote:
On 06/10/2017 11:59 AM, Simon Glass wrote:
Move the display options code into a separate function so that the U-Boot banner can be obtained from other code. Adjust the 'version' command to use it.
This series passed my automated testing, so: Tested-by: Stephen Warren swarren@nvidia.com
Tested on Intel MinnowMax Tested-by: Bin Meng bmeng.cn@gmail.com
participants (3)
-
Bin Meng -
Simon Glass -
Stephen Warren