[U-Boot] [PATCH v2 1/3] display_options: Refactor to allow obtaining the banner

[U-Boot] [PATCH v2 1/3] display_options: Refactor to allow obtaining the banner

[Simon Glass]

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;
 }
 
-- 
2.13.1.508.gb3defc5cc-goog

[U-Boot] [PATCH v2 2/3] Allow displaying the U-Boot banner on a video display

[Simon Glass]

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.
  */
-- 
2.13.1.508.gb3defc5cc-goog

[U-Boot] [PATCH v2 3/3] test: Add a test for snprintf() and the banner/version

[Simon Glass]

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.",
+	""
+);
-- 
2.13.1.508.gb3defc5cc-goog

[U-Boot] [PATCH v2 1/3] display_options: Refactor to allow obtaining the banner

[Bin Meng]

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

[U-Boot] [PATCH v2 2/3] Allow displaying the U-Boot banner on a video display

[Bin Meng]

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

[U-Boot] [PATCH v2 2/3] Allow displaying the U-Boot banner on a video display

[Simon Glass]

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

[U-Boot] [PATCH v2 1/3] display_options: Refactor to allow obtaining the banner

[Stephen Warren]

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>

[U-Boot] [PATCH v2 1/3] display_options: Refactor to allow obtaining the banner

[Bin Meng]

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>

[U-Boot] [PATCH v2 2/3] Allow displaying the U-Boot banner on a video display

[Bin Meng]

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

[U-Boot] [PATCH v2 3/3] test: Add a test for snprintf() and the banner/version

[Bin Meng]

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>

[U-Boot] [PATCH v2 2/3] Allow displaying the U-Boot banner on a video display

[Simon Glass]

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

[U-Boot] [PATCH v2 2/3] Allow displaying the U-Boot banner on a video display

[Bin Meng]

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

[U-Boot] [PATCH v2 2/3] Allow displaying the U-Boot banner on a video display

[Simon Glass]

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