[PATCH v5 0/6] Introduce the sysinfo command

[PATCH v5 0/6] Introduce the sysinfo command

[Detlev Casanova]

The command can be used to show various information that can be used to
identify the running system.

Currently supported subcommands are:
* model: A string representing the model
* id: The id of the board
* revision: The revision of this board.

Changes since v4:
 - Use less snprintf calls in drivers/sysinfo/rcar3.c
 - Rebase on master
Changes since v3:
 - Fix documentation typo.
Changes since v2:
 - Fix code style.
 - Use printf() instead of debug().
 - Clarify sysinfo new ids types (int).
 - Add a test for sysinfo command.
 - Add documentation for sysinfo command.
Changes since v1:
 - Removed shell function to select linux device tree. This will be
   distributions job.
 - Break revision in rev_major and rev_minor in the sysinfo driver.

Detlev Casanova (6):
  sysinfo: Add IDs for board id and revision
  cmd: Add a sysinfo command
  sysinfo: Add a test
  sysinfo: Add documentation
  sysinfo: rcar3: Use int instead of char for revision
  sysinfo: rcar3: Implement BOARD_ID and BOARD_REV_*

 cmd/Kconfig               |   6 ++
 cmd/Makefile              |   1 +
 cmd/sysinfo.c             | 133 ++++++++++++++++++++++++++++++++++++++
 configs/sandbox_defconfig |   1 +
 doc/usage/cmd/sysinfo.rst |  56 ++++++++++++++++
 drivers/sysinfo/rcar3.c   | 110 +++++++++++++++++++++----------
 drivers/sysinfo/sandbox.c |  17 +++++
 include/sysinfo.h         |   5 ++
 test/cmd/Makefile         |   1 +
 test/cmd/test_sysinfo.c   |  51 +++++++++++++++
 10 files changed, 347 insertions(+), 34 deletions(-)
 create mode 100644 cmd/sysinfo.c
 create mode 100644 doc/usage/cmd/sysinfo.rst
 create mode 100644 test/cmd/test_sysinfo.c

-- 
2.41.0

[PATCH v5 1/6] sysinfo: Add IDs for board id and revision

[Detlev Casanova]

These IDs will be used by the sysinfo command. The new IDs are:
 * SYSINFO_ID_BOARD_ID: The board ID as an integer
 * SYSINFO_ID_BOARD_REV_MAJOR: The board major revision as int
 * SYSINFO_ID_BOARD_REV_MINOR: The board minor revision as int

Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 include/sysinfo.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/sysinfo.h b/include/sysinfo.h
index b140d742e93..13815600ae6 100644
--- a/include/sysinfo.h
+++ b/include/sysinfo.h
@@ -47,6 +47,11 @@ enum sysinfo_id {
 	/* For show_board_info() */
 	SYSINFO_ID_BOARD_MODEL,
 
+	/* For sysinfo command (all int) */
+	SYSINFO_ID_BOARD_ID,
+	SYSINFO_ID_BOARD_REV_MAJOR,
+	SYSINFO_ID_BOARD_REV_MINOR,
+
 	/* First value available for downstream/board used */
 	SYSINFO_ID_USER = 0x1000,
 };
-- 
2.41.0

Re: [PATCH v5 1/6] sysinfo: Add IDs for board id and revision

[Simon Glass]

On Mon, 2 Oct 2023 at 09:21, Detlev Casanova
<detlev.casanova@collabora.com> wrote:
>
> These IDs will be used by the sysinfo command. The new IDs are:
>  * SYSINFO_ID_BOARD_ID: The board ID as an integer
>  * SYSINFO_ID_BOARD_REV_MAJOR: The board major revision as int
>  * SYSINFO_ID_BOARD_REV_MINOR: The board minor revision as int
>
> Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  include/sysinfo.h | 5 +++++
>  1 file changed, 5 insertions(+)
>

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

Re: [PATCH v5 1/6] sysinfo: Add IDs for board id and revision

[Heinrich Schuchardt]

On 10/2/23 17:20, Detlev Casanova wrote:
> These IDs will be used by the sysinfo command. The new IDs are:
>   * SYSINFO_ID_BOARD_ID: The board ID as an integer
>   * SYSINFO_ID_BOARD_REV_MAJOR: The board major revision as int
>   * SYSINFO_ID_BOARD_REV_MINOR: The board minor revision as int

Integers will not work in the generic case. E.g. for the VisionFive 2
board we have seen board revisions:

1.2A, 1.2B, 1.3B

For the Wandboard:

B1, D1

>
> Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>   include/sysinfo.h | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/include/sysinfo.h b/include/sysinfo.h
> index b140d742e93..13815600ae6 100644
> --- a/include/sysinfo.h
> +++ b/include/sysinfo.h
> @@ -47,6 +47,11 @@ enum sysinfo_id {
>   	/* For show_board_info() */
>   	SYSINFO_ID_BOARD_MODEL,
>
> +	/* For sysinfo command (all int) */

Please, provide a Sphinx style documentation for every value. Cf.
https://www.kernel.org/doc/html/v5.3/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation

Best regards

Heinrich

> +	SYSINFO_ID_BOARD_ID,
> +	SYSINFO_ID_BOARD_REV_MAJOR,
> +	SYSINFO_ID_BOARD_REV_MINOR,
> +
>   	/* First value available for downstream/board used */
>   	SYSINFO_ID_USER = 0x1000,
>   };

Re: [PATCH v5 1/6] sysinfo: Add IDs for board id and revision

[Marek Vasut]

On 10/16/23 19:37, Heinrich Schuchardt wrote:
> On 10/2/23 17:20, Detlev Casanova wrote:
>> These IDs will be used by the sysinfo command. The new IDs are:
>>   * SYSINFO_ID_BOARD_ID: The board ID as an integer
>>   * SYSINFO_ID_BOARD_REV_MAJOR: The board major revision as int
>>   * SYSINFO_ID_BOARD_REV_MINOR: The board minor revision as int
> 
> Integers will not work in the generic case. E.g. for the VisionFive 2
> board we have seen board revisions:
> 
> 1.2A, 1.2B, 1.3B
> 
> For the Wandboard:
> 
> B1, D1

Dang, I literally had this in my upstreaming queue today, so, sigh.
Dropped for now :-(

-- 
Best regards,
Marek Vasut

Re: [PATCH v5 1/6] sysinfo: Add IDs for board id and revision

[Detlev Casanova]

On Monday, October 16, 2023 1:37:40 P.M. EDT Heinrich Schuchardt wrote:
> On 10/2/23 17:20, Detlev Casanova wrote:
> > These IDs will be used by the sysinfo command. The new IDs are:
> >   * SYSINFO_ID_BOARD_ID: The board ID as an integer
> >   * SYSINFO_ID_BOARD_REV_MAJOR: The board major revision as int
> >   * SYSINFO_ID_BOARD_REV_MINOR: The board minor revision as int
> 
> Integers will not work in the generic case. E.g. for the VisionFive 2
> board we have seen board revisions:
> 
> 1.2A, 1.2B, 1.3B
> 
> For the Wandboard:
> 
> B1, D1

From this, I gather that the revision should just be a string value, as there 
is no generic way to represent all variations of the revision "numbers".

> > Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >   include/sysinfo.h | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/include/sysinfo.h b/include/sysinfo.h
> > index b140d742e93..13815600ae6 100644
> > --- a/include/sysinfo.h
> > +++ b/include/sysinfo.h
> > @@ -47,6 +47,11 @@ enum sysinfo_id {
> > 
> >   	/* For show_board_info() */
> >   	SYSINFO_ID_BOARD_MODEL,
> > 
> > +	/* For sysinfo command (all int) */
> 
> Please, provide a Sphinx style documentation for every value. Cf.
> https://www.kernel.org/doc/html/v5.3/doc-guide/kernel-doc.html#structure-uni
> on-and-enumeration-documentation
> 
> Best regards
> 
> Heinrich
> 
> > +	SYSINFO_ID_BOARD_ID,
> > +	SYSINFO_ID_BOARD_REV_MAJOR,
> > +	SYSINFO_ID_BOARD_REV_MINOR,
> > +
> > 
> >   	/* First value available for downstream/board used */
> >   	SYSINFO_ID_USER = 0x1000,
> >   
> >   };

[PATCH v5 2/6] cmd: Add a sysinfo command

[Detlev Casanova]

The command is able to show different information for the running
system:
* Model name
* Board ID
* Revision

This command can be used by boot shell scripts to select configurations
depending on the specific running system.

Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 cmd/Kconfig   |   6 +++
 cmd/Makefile  |   1 +
 cmd/sysinfo.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 140 insertions(+)
 create mode 100644 cmd/sysinfo.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 43ca10f69cc..67d019aa795 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -218,6 +218,12 @@ config CMD_SBI
 	help
 	  Display information about the SBI implementation.
 
+config CMD_SYSINFO
+	bool "sysinfo"
+	depends on SYSINFO
+	help
+	  Display information about the system.
+
 endmenu
 
 menu "Boot commands"
diff --git a/cmd/Makefile b/cmd/Makefile
index 9bebf321c39..972f3a4720e 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -167,6 +167,7 @@ obj-$(CONFIG_CMD_SPI) += spi.o
 obj-$(CONFIG_CMD_STRINGS) += strings.o
 obj-$(CONFIG_CMD_SMC) += smccc.o
 obj-$(CONFIG_CMD_SYSBOOT) += sysboot.o
+obj-$(CONFIG_CMD_SYSINFO) += sysinfo.o
 obj-$(CONFIG_CMD_STACKPROTECTOR_TEST) += stackprot_test.o
 obj-$(CONFIG_CMD_TEMPERATURE) += temperature.o
 obj-$(CONFIG_CMD_TERMINAL) += terminal.o
diff --git a/cmd/sysinfo.c b/cmd/sysinfo.c
new file mode 100644
index 00000000000..46369ff9ac7
--- /dev/null
+++ b/cmd/sysinfo.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2023
+ * Detlev Casanova <detlev.casanova@collabora.com>
+ */
+
+#include <command.h>
+#include <env.h>
+#include <sysinfo.h>
+#include <exports.h>
+
+static int get_sysinfo(struct udevice **devp)
+{
+	int ret = sysinfo_get(devp);
+
+	if (ret) {
+		printf("Cannot get sysinfo: %d\n", ret);
+		return ret;
+	}
+
+	ret = sysinfo_detect(*devp);
+	if (ret) {
+		printf("Cannot detect sysinfo: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int do_sysinfo_model(struct cmd_tbl *cmdtp, int flag, int argc,
+			    char *const argv[])
+{
+	struct udevice *dev;
+	char model[64];
+	int ret = get_sysinfo(&dev);
+
+	if (ret)
+		return CMD_RET_FAILURE;
+
+	ret = sysinfo_get_str(dev,
+			      SYSINFO_ID_BOARD_MODEL,
+			      sizeof(model),
+			      model);
+	if (ret) {
+		printf("Cannot get sysinfo str: %d\n", ret);
+		return CMD_RET_FAILURE;
+	}
+
+	if (argc == 2)
+		ret = env_set(argv[1], model);
+	else
+		printf("%s\n", model);
+
+	if (ret)
+		return CMD_RET_FAILURE;
+	else
+		return CMD_RET_SUCCESS;
+}
+
+static int do_sysinfo_id(struct cmd_tbl *cmdtp, int flag, int argc,
+			 char *const argv[])
+{
+	struct udevice *dev;
+	u32 board_id;
+	int ret = get_sysinfo(&dev);
+
+	if (ret)
+		return CMD_RET_FAILURE;
+
+	ret = sysinfo_get_int(dev, SYSINFO_ID_BOARD_ID, &board_id);
+	if (ret) {
+		printf("Cannot get sysinfo int: %d\n", ret);
+		return CMD_RET_FAILURE;
+	}
+
+	if (argc == 2)
+		ret = env_set_hex(argv[1], board_id);
+	else
+		printf("0x%02x\n", board_id);
+
+	if (ret)
+		return CMD_RET_FAILURE;
+	else
+		return CMD_RET_SUCCESS;
+}
+
+static int do_sysinfo_revision(struct cmd_tbl *cmdtp, int flag, int argc,
+			       char *const argv[])
+{
+	struct udevice *dev;
+	int rev_major;
+	int rev_minor;
+	char rev[64];
+	int ret = get_sysinfo(&dev);
+
+	if (ret)
+		return CMD_RET_FAILURE;
+
+	ret = sysinfo_get_int(dev, SYSINFO_ID_BOARD_REV_MAJOR, &rev_major);
+	if (ret) {
+		printf("Cannot get sysinfo int: %d\n", ret);
+		return CMD_RET_FAILURE;
+	}
+
+	ret = sysinfo_get_int(dev, SYSINFO_ID_BOARD_REV_MINOR, &rev_minor);
+	if (ret) {
+		printf("Cannot get sysinfo int: %d\n", ret);
+		return CMD_RET_FAILURE;
+	}
+
+	snprintf(rev, sizeof(rev), "%d.%d", rev_major, rev_minor);
+
+	if (argc == 2)
+		ret = env_set(argv[1], rev);
+	else
+		printf("%s\n", rev);
+
+	if (ret)
+		return CMD_RET_FAILURE;
+	else
+		return CMD_RET_SUCCESS;
+}
+
+static char sysinfo_help_text[] =
+	"model <varname>     - Show or set the board model in varname\n"
+	"sysinfo id <varname>        - Show or set the board id in varname (in format 0xHH)\n"
+	"sysinfo revision <varname>  - Show or set the board revision in varname";
+
+U_BOOT_CMD_WITH_SUBCMDS(sysinfo, "System information", sysinfo_help_text,
+			U_BOOT_SUBCMD_MKENT(model, 2, 1, do_sysinfo_model),
+			U_BOOT_SUBCMD_MKENT(id, 2, 1, do_sysinfo_id),
+			U_BOOT_SUBCMD_MKENT(revision, 2, 1, do_sysinfo_revision),
+);
-- 
2.41.0

Re: [PATCH v5 2/6] cmd: Add a sysinfo command

[Simon Glass]

On Mon, 2 Oct 2023 at 09:21, Detlev Casanova
<detlev.casanova@collabora.com> wrote:
>
> The command is able to show different information for the running
> system:
> * Model name
> * Board ID
> * Revision
>
> This command can be used by boot shell scripts to select configurations
> depending on the specific running system.
>
> Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  cmd/Kconfig   |   6 +++
>  cmd/Makefile  |   1 +
>  cmd/sysinfo.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 140 insertions(+)
>  create mode 100644 cmd/sysinfo.c

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

Would prefer more help text in the Kconfig as this is pretty vague


>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 43ca10f69cc..67d019aa795 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -218,6 +218,12 @@ config CMD_SBI
>         help
>           Display information about the SBI implementation.
>
> +config CMD_SYSINFO
> +       bool "sysinfo"
> +       depends on SYSINFO
> +       help
> +         Display information about the system.
> +
>  endmenu
>
>  menu "Boot commands"
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 9bebf321c39..972f3a4720e 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -167,6 +167,7 @@ obj-$(CONFIG_CMD_SPI) += spi.o
>  obj-$(CONFIG_CMD_STRINGS) += strings.o
>  obj-$(CONFIG_CMD_SMC) += smccc.o
>  obj-$(CONFIG_CMD_SYSBOOT) += sysboot.o
> +obj-$(CONFIG_CMD_SYSINFO) += sysinfo.o
>  obj-$(CONFIG_CMD_STACKPROTECTOR_TEST) += stackprot_test.o
>  obj-$(CONFIG_CMD_TEMPERATURE) += temperature.o
>  obj-$(CONFIG_CMD_TERMINAL) += terminal.o
> diff --git a/cmd/sysinfo.c b/cmd/sysinfo.c
> new file mode 100644
> index 00000000000..46369ff9ac7
> --- /dev/null
> +++ b/cmd/sysinfo.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2023
> + * Detlev Casanova <detlev.casanova@collabora.com>
> + */
> +
> +#include <command.h>
> +#include <env.h>
> +#include <sysinfo.h>
> +#include <exports.h>
> +
> +static int get_sysinfo(struct udevice **devp)
> +{
> +       int ret = sysinfo_get(devp);
> +
> +       if (ret) {
> +               printf("Cannot get sysinfo: %d\n", ret);
> +               return ret;
> +       }
> +
> +       ret = sysinfo_detect(*devp);
> +       if (ret) {
> +               printf("Cannot detect sysinfo: %d\n", ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int do_sysinfo_model(struct cmd_tbl *cmdtp, int flag, int argc,
> +                           char *const argv[])
> +{
> +       struct udevice *dev;
> +       char model[64];
> +       int ret = get_sysinfo(&dev);
> +
> +       if (ret)
> +               return CMD_RET_FAILURE;
> +
> +       ret = sysinfo_get_str(dev,
> +                             SYSINFO_ID_BOARD_MODEL,
> +                             sizeof(model),
> +                             model);
> +       if (ret) {
> +               printf("Cannot get sysinfo str: %d\n", ret);
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       if (argc == 2)
> +               ret = env_set(argv[1], model);
> +       else
> +               printf("%s\n", model);
> +
> +       if (ret)
> +               return CMD_RET_FAILURE;
> +       else
> +               return CMD_RET_SUCCESS;
> +}
> +
> +static int do_sysinfo_id(struct cmd_tbl *cmdtp, int flag, int argc,
> +                        char *const argv[])
> +{
> +       struct udevice *dev;
> +       u32 board_id;
> +       int ret = get_sysinfo(&dev);
> +
> +       if (ret)
> +               return CMD_RET_FAILURE;
> +
> +       ret = sysinfo_get_int(dev, SYSINFO_ID_BOARD_ID, &board_id);
> +       if (ret) {
> +               printf("Cannot get sysinfo int: %d\n", ret);
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       if (argc == 2)
> +               ret = env_set_hex(argv[1], board_id);
> +       else
> +               printf("0x%02x\n", board_id);
> +
> +       if (ret)
> +               return CMD_RET_FAILURE;
> +       else
> +               return CMD_RET_SUCCESS;
> +}
> +
> +static int do_sysinfo_revision(struct cmd_tbl *cmdtp, int flag, int argc,
> +                              char *const argv[])
> +{
> +       struct udevice *dev;
> +       int rev_major;
> +       int rev_minor;
> +       char rev[64];
> +       int ret = get_sysinfo(&dev);
> +
> +       if (ret)
> +               return CMD_RET_FAILURE;
> +
> +       ret = sysinfo_get_int(dev, SYSINFO_ID_BOARD_REV_MAJOR, &rev_major);
> +       if (ret) {
> +               printf("Cannot get sysinfo int: %d\n", ret);
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       ret = sysinfo_get_int(dev, SYSINFO_ID_BOARD_REV_MINOR, &rev_minor);
> +       if (ret) {
> +               printf("Cannot get sysinfo int: %d\n", ret);
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       snprintf(rev, sizeof(rev), "%d.%d", rev_major, rev_minor);
> +
> +       if (argc == 2)
> +               ret = env_set(argv[1], rev);
> +       else
> +               printf("%s\n", rev);
> +
> +       if (ret)
> +               return CMD_RET_FAILURE;
> +       else
> +               return CMD_RET_SUCCESS;
> +}
> +
> +static char sysinfo_help_text[] =
> +       "model <varname>     - Show or set the board model in varname\n"
> +       "sysinfo id <varname>        - Show or set the board id in varname (in format 0xHH)\n"
> +       "sysinfo revision <varname>  - Show or set the board revision in varname";
> +
> +U_BOOT_CMD_WITH_SUBCMDS(sysinfo, "System information", sysinfo_help_text,
> +                       U_BOOT_SUBCMD_MKENT(model, 2, 1, do_sysinfo_model),
> +                       U_BOOT_SUBCMD_MKENT(id, 2, 1, do_sysinfo_id),
> +                       U_BOOT_SUBCMD_MKENT(revision, 2, 1, do_sysinfo_revision),
> +);
> --
> 2.41.0
>

Re: [PATCH v5 2/6] cmd: Add a sysinfo command

[Detlev Casanova]

On Monday, October 2, 2023 2:56:28 P.M. EDT Simon Glass wrote:
> On Mon, 2 Oct 2023 at 09:21, Detlev Casanova
> 
> <detlev.casanova@collabora.com> wrote:
> > The command is able to show different information for the running
> > system:
> > * Model name
> > * Board ID
> > * Revision
> > 
> > This command can be used by boot shell scripts to select configurations
> > depending on the specific running system.
> > 
> > Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >  cmd/Kconfig   |   6 +++
> >  cmd/Makefile  |   1 +
> >  cmd/sysinfo.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 140 insertions(+)
> >  create mode 100644 cmd/sysinfo.c
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Would prefer more help text in the Kconfig as this is pretty vague

Indeed, how would that sound ?

Display information to identify the system. It supports showing a board id, a 
revision number as well as a human readable name.

> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index 43ca10f69cc..67d019aa795 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -218,6 +218,12 @@ config CMD_SBI
> > 
> >         help
> >         
> >           Display information about the SBI implementation.
> > 
> > +config CMD_SYSINFO
> > +       bool "sysinfo"
> > +       depends on SYSINFO
> > +       help
> > +         Display information about the system.
> > +
> > 
> >  endmenu
> >  
> >  menu "Boot commands"
> > 
> > diff --git a/cmd/Makefile b/cmd/Makefile
> > index 9bebf321c39..972f3a4720e 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -167,6 +167,7 @@ obj-$(CONFIG_CMD_SPI) += spi.o
> > 
> >  obj-$(CONFIG_CMD_STRINGS) += strings.o
> >  obj-$(CONFIG_CMD_SMC) += smccc.o
> >  obj-$(CONFIG_CMD_SYSBOOT) += sysboot.o
> > 
> > +obj-$(CONFIG_CMD_SYSINFO) += sysinfo.o
> > 
> >  obj-$(CONFIG_CMD_STACKPROTECTOR_TEST) += stackprot_test.o
> >  obj-$(CONFIG_CMD_TEMPERATURE) += temperature.o
> >  obj-$(CONFIG_CMD_TERMINAL) += terminal.o
> > 
> > diff --git a/cmd/sysinfo.c b/cmd/sysinfo.c
> > new file mode 100644
> > index 00000000000..46369ff9ac7
> > --- /dev/null
> > +++ b/cmd/sysinfo.c
> > @@ -0,0 +1,133 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2023
> > + * Detlev Casanova <detlev.casanova@collabora.com>
> > + */
> > +
> > +#include <command.h>
> > +#include <env.h>
> > +#include <sysinfo.h>
> > +#include <exports.h>
> > +
> > +static int get_sysinfo(struct udevice **devp)
> > +{
> > +       int ret = sysinfo_get(devp);
> > +
> > +       if (ret) {
> > +               printf("Cannot get sysinfo: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       ret = sysinfo_detect(*devp);
> > +       if (ret) {
> > +               printf("Cannot detect sysinfo: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int do_sysinfo_model(struct cmd_tbl *cmdtp, int flag, int argc,
> > +                           char *const argv[])
> > +{
> > +       struct udevice *dev;
> > +       char model[64];
> > +       int ret = get_sysinfo(&dev);
> > +
> > +       if (ret)
> > +               return CMD_RET_FAILURE;
> > +
> > +       ret = sysinfo_get_str(dev,
> > +                             SYSINFO_ID_BOARD_MODEL,
> > +                             sizeof(model),
> > +                             model);
> > +       if (ret) {
> > +               printf("Cannot get sysinfo str: %d\n", ret);
> > +               return CMD_RET_FAILURE;
> > +       }
> > +
> > +       if (argc == 2)
> > +               ret = env_set(argv[1], model);
> > +       else
> > +               printf("%s\n", model);
> > +
> > +       if (ret)
> > +               return CMD_RET_FAILURE;
> > +       else
> > +               return CMD_RET_SUCCESS;
> > +}
> > +
> > +static int do_sysinfo_id(struct cmd_tbl *cmdtp, int flag, int argc,
> > +                        char *const argv[])
> > +{
> > +       struct udevice *dev;
> > +       u32 board_id;
> > +       int ret = get_sysinfo(&dev);
> > +
> > +       if (ret)
> > +               return CMD_RET_FAILURE;
> > +
> > +       ret = sysinfo_get_int(dev, SYSINFO_ID_BOARD_ID, &board_id);
> > +       if (ret) {
> > +               printf("Cannot get sysinfo int: %d\n", ret);
> > +               return CMD_RET_FAILURE;
> > +       }
> > +
> > +       if (argc == 2)
> > +               ret = env_set_hex(argv[1], board_id);
> > +       else
> > +               printf("0x%02x\n", board_id);
> > +
> > +       if (ret)
> > +               return CMD_RET_FAILURE;
> > +       else
> > +               return CMD_RET_SUCCESS;
> > +}
> > +
> > +static int do_sysinfo_revision(struct cmd_tbl *cmdtp, int flag, int argc,
> > +                              char *const argv[])
> > +{
> > +       struct udevice *dev;
> > +       int rev_major;
> > +       int rev_minor;
> > +       char rev[64];
> > +       int ret = get_sysinfo(&dev);
> > +
> > +       if (ret)
> > +               return CMD_RET_FAILURE;
> > +
> > +       ret = sysinfo_get_int(dev, SYSINFO_ID_BOARD_REV_MAJOR,
> > &rev_major);
> > +       if (ret) {
> > +               printf("Cannot get sysinfo int: %d\n", ret);
> > +               return CMD_RET_FAILURE;
> > +       }
> > +
> > +       ret = sysinfo_get_int(dev, SYSINFO_ID_BOARD_REV_MINOR,
> > &rev_minor);
> > +       if (ret) {
> > +               printf("Cannot get sysinfo int: %d\n", ret);
> > +               return CMD_RET_FAILURE;
> > +       }
> > +
> > +       snprintf(rev, sizeof(rev), "%d.%d", rev_major, rev_minor);
> > +
> > +       if (argc == 2)
> > +               ret = env_set(argv[1], rev);
> > +       else
> > +               printf("%s\n", rev);
> > +
> > +       if (ret)
> > +               return CMD_RET_FAILURE;
> > +       else
> > +               return CMD_RET_SUCCESS;
> > +}
> > +
> > +static char sysinfo_help_text[] =
> > +       "model <varname>     - Show or set the board model in varname\n"
> > +       "sysinfo id <varname>        - Show or set the board id in varname
> > (in format 0xHH)\n" +       "sysinfo revision <varname>  - Show or set
> > the board revision in varname"; +
> > +U_BOOT_CMD_WITH_SUBCMDS(sysinfo, "System information", sysinfo_help_text,
> > +                       U_BOOT_SUBCMD_MKENT(model, 2, 1,
> > do_sysinfo_model),
> > +                       U_BOOT_SUBCMD_MKENT(id, 2, 1, do_sysinfo_id),
> > +                       U_BOOT_SUBCMD_MKENT(revision, 2, 1,
> > do_sysinfo_revision), +);
> > --
> > 2.41.0

Re: [PATCH v5 2/6] cmd: Add a sysinfo command

[Simon Glass]

Hi Detlev,

On Mon, 16 Oct 2023 at 08:21, Detlev Casanova
<detlev.casanova@collabora.com> wrote:
>
> On Monday, October 2, 2023 2:56:28 P.M. EDT Simon Glass wrote:
> > On Mon, 2 Oct 2023 at 09:21, Detlev Casanova
> >
> > <detlev.casanova@collabora.com> wrote:
> > > The command is able to show different information for the running
> > > system:
> > > * Model name
> > > * Board ID
> > > * Revision
> > >
> > > This command can be used by boot shell scripts to select configurations
> > > depending on the specific running system.
> > >
> > > Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> > > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > > ---
> > >
> > >  cmd/Kconfig   |   6 +++
> > >  cmd/Makefile  |   1 +
> > >  cmd/sysinfo.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 140 insertions(+)
> > >  create mode 100644 cmd/sysinfo.c
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > Would prefer more help text in the Kconfig as this is pretty vague
>
> Indeed, how would that sound ?
>
> Display information to identify the system. It supports showing a board id, a
> revision number as well as a human readable name.

Also it uses the sysinfo driver for your board to get the information.
You could link to the sysinfo docs or header file.

Regards,
Simon

[PATCH v5 3/6] sysinfo: Add a test

[Detlev Casanova]

The test runs one of each subcommand and checks that the output matches
the values in the sandbox sysinfo driver.

Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 configs/sandbox_defconfig |  1 +
 drivers/sysinfo/sandbox.c | 17 +++++++++++++
 test/cmd/Makefile         |  1 +
 test/cmd/test_sysinfo.c   | 51 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 70 insertions(+)
 create mode 100644 test/cmd/test_sysinfo.c

diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 62bc182ca16..409e3d88012 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -129,6 +129,7 @@ CONFIG_CMD_EROFS=y
 CONFIG_CMD_EXT4_WRITE=y
 CONFIG_CMD_SQUASHFS=y
 CONFIG_CMD_MTDPARTS=y
+CONFIG_CMD_SYSINFO=y
 CONFIG_CMD_STACKPROTECTOR_TEST=y
 CONFIG_MAC_PARTITION=y
 CONFIG_AMIGA_PARTITION=y
diff --git a/drivers/sysinfo/sandbox.c b/drivers/sysinfo/sandbox.c
index d270a26aa43..cc7783907a9 100644
--- a/drivers/sysinfo/sandbox.c
+++ b/drivers/sysinfo/sandbox.c
@@ -7,9 +7,14 @@
 #include <common.h>
 #include <dm.h>
 #include <sysinfo.h>
+#include <version.h>
 
 #include "sandbox.h"
 
+#define SANDBOX_BOARD_ID	0x42
+#define SANDBOX_BOARD_REV_MAJOR	U_BOOT_VERSION_NUM
+#define SANDBOX_BOARD_REV_MINOR	U_BOOT_VERSION_NUM_PATCH
+
 struct sysinfo_sandbox_priv {
 	bool called_detect;
 	int test_i1;
@@ -48,6 +53,15 @@ int sysinfo_sandbox_get_int(struct udevice *dev, int id, int *val)
 	struct sysinfo_sandbox_priv *priv = dev_get_priv(dev);
 
 	switch (id) {
+	case SYSINFO_ID_BOARD_ID:
+		*val = SANDBOX_BOARD_ID;
+		return 0;
+	case SYSINFO_ID_BOARD_REV_MAJOR:
+		*val = SANDBOX_BOARD_REV_MAJOR;
+		return 0;
+	case SYSINFO_ID_BOARD_REV_MINOR:
+		*val = SANDBOX_BOARD_REV_MINOR;
+		return 0;
 	case INT_TEST1:
 		*val = priv->test_i1;
 		/* Increments with every call */
@@ -71,6 +85,9 @@ int sysinfo_sandbox_get_str(struct udevice *dev, int id, size_t size, char *val)
 	int index = (i1 * i2) % ARRAY_SIZE(vacation_spots);
 
 	switch (id) {
+	case SYSINFO_ID_BOARD_MODEL:
+		snprintf(val, size, "sandbox");
+		return 0;
 	case STR_VACATIONSPOT:
 		/* Picks a vacation spot depending on i1 and i2 */
 		snprintf(val, size, vacation_spots[index]);
diff --git a/test/cmd/Makefile b/test/cmd/Makefile
index 6e3d7e919ef..dc8f59d87f2 100644
--- a/test/cmd/Makefile
+++ b/test/cmd/Makefile
@@ -26,6 +26,7 @@ ifdef CONFIG_SANDBOX
 obj-$(CONFIG_CMD_READ) += rw.o
 obj-$(CONFIG_CMD_SETEXPR) += setexpr.o
 obj-$(CONFIG_ARM_FFA_TRANSPORT) += armffa.o
+obj-$(CONFIG_CMD_SYSINFO) += test_sysinfo.o
 endif
 obj-$(CONFIG_CMD_TEMPERATURE) += temperature.o
 obj-$(CONFIG_CMD_WGET) += wget.o
diff --git a/test/cmd/test_sysinfo.c b/test/cmd/test_sysinfo.c
new file mode 100644
index 00000000000..7ba6dd0df89
--- /dev/null
+++ b/test/cmd/test_sysinfo.c
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Tests for sysinfo command
+ *
+ * Copyright 2023, Detlev Casanova <detlev.casanova@collabora.com>
+ */
+
+#include <common.h>
+#include <command.h>
+#include <asm/global_data.h>
+#include <display_options.h>
+#include <test/lib.h>
+#include <test/test.h>
+#include <test/ut.h>
+#include <version.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+#define REV_(x, y) #x "." #y
+#define REV(x, y) REV_(x, y)
+
+struct test_data {
+	char *cmd;
+	char *expected;
+};
+
+static struct test_data sysinfo_data[] = {
+	{"sysinfo model", "sandbox"},
+	{"sysinfo id", "0x42"},
+	{"sysinfo revision", REV(U_BOOT_VERSION_NUM, U_BOOT_VERSION_NUM_PATCH)},
+};
+
+static int lib_test_sysinfo(struct unit_test_state *uts)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(sysinfo_data); ++i) {
+		ut_silence_console(uts);
+		console_record_reset_enable();
+		ut_assertok(run_command(sysinfo_data[i].cmd, 0));
+		ut_unsilence_console(uts);
+		console_record_readline(uts->actual_str,
+					sizeof(uts->actual_str));
+		ut_asserteq_str(sysinfo_data[i].expected, uts->actual_str);
+		ut_assertok(ut_check_console_end(uts));
+	}
+
+	return 0;
+}
+
+LIB_TEST(lib_test_sysinfo, 0);
-- 
2.41.0

Re: [PATCH v5 3/6] sysinfo: Add a test

[Simon Glass]

Hi Detlev,

On Mon, 2 Oct 2023 at 09:22, Detlev Casanova
<detlev.casanova@collabora.com> wrote:
>
> The test runs one of each subcommand and checks that the output matches
> the values in the sandbox sysinfo driver.
>
> Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  configs/sandbox_defconfig |  1 +
>  drivers/sysinfo/sandbox.c | 17 +++++++++++++
>  test/cmd/Makefile         |  1 +
>  test/cmd/test_sysinfo.c   | 51 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 70 insertions(+)
>  create mode 100644 test/cmd/test_sysinfo.c
>

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

nits below

> diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> index 62bc182ca16..409e3d88012 100644
> --- a/configs/sandbox_defconfig
> +++ b/configs/sandbox_defconfig
> @@ -129,6 +129,7 @@ CONFIG_CMD_EROFS=y
>  CONFIG_CMD_EXT4_WRITE=y
>  CONFIG_CMD_SQUASHFS=y
>  CONFIG_CMD_MTDPARTS=y
> +CONFIG_CMD_SYSINFO=y
>  CONFIG_CMD_STACKPROTECTOR_TEST=y
>  CONFIG_MAC_PARTITION=y
>  CONFIG_AMIGA_PARTITION=y
> diff --git a/drivers/sysinfo/sandbox.c b/drivers/sysinfo/sandbox.c
> index d270a26aa43..cc7783907a9 100644
> --- a/drivers/sysinfo/sandbox.c
> +++ b/drivers/sysinfo/sandbox.c
> @@ -7,9 +7,14 @@
>  #include <common.h>
>  #include <dm.h>
>  #include <sysinfo.h>
> +#include <version.h>
>
>  #include "sandbox.h"
>
> +#define SANDBOX_BOARD_ID       0x42
> +#define SANDBOX_BOARD_REV_MAJOR        U_BOOT_VERSION_NUM
> +#define SANDBOX_BOARD_REV_MINOR        U_BOOT_VERSION_NUM_PATCH
> +
>  struct sysinfo_sandbox_priv {
>         bool called_detect;
>         int test_i1;
> @@ -48,6 +53,15 @@ int sysinfo_sandbox_get_int(struct udevice *dev, int id, int *val)
>         struct sysinfo_sandbox_priv *priv = dev_get_priv(dev);
>
>         switch (id) {
> +       case SYSINFO_ID_BOARD_ID:
> +               *val = SANDBOX_BOARD_ID;
> +               return 0;
> +       case SYSINFO_ID_BOARD_REV_MAJOR:
> +               *val = SANDBOX_BOARD_REV_MAJOR;
> +               return 0;
> +       case SYSINFO_ID_BOARD_REV_MINOR:
> +               *val = SANDBOX_BOARD_REV_MINOR;
> +               return 0;
>         case INT_TEST1:
>                 *val = priv->test_i1;
>                 /* Increments with every call */
> @@ -71,6 +85,9 @@ int sysinfo_sandbox_get_str(struct udevice *dev, int id, size_t size, char *val)
>         int index = (i1 * i2) % ARRAY_SIZE(vacation_spots);
>
>         switch (id) {
> +       case SYSINFO_ID_BOARD_MODEL:
> +               snprintf(val, size, "sandbox");
> +               return 0;
>         case STR_VACATIONSPOT:
>                 /* Picks a vacation spot depending on i1 and i2 */
>                 snprintf(val, size, vacation_spots[index]);
> diff --git a/test/cmd/Makefile b/test/cmd/Makefile
> index 6e3d7e919ef..dc8f59d87f2 100644
> --- a/test/cmd/Makefile
> +++ b/test/cmd/Makefile
> @@ -26,6 +26,7 @@ ifdef CONFIG_SANDBOX
>  obj-$(CONFIG_CMD_READ) += rw.o
>  obj-$(CONFIG_CMD_SETEXPR) += setexpr.o
>  obj-$(CONFIG_ARM_FFA_TRANSPORT) += armffa.o
> +obj-$(CONFIG_CMD_SYSINFO) += test_sysinfo.o
>  endif
>  obj-$(CONFIG_CMD_TEMPERATURE) += temperature.o
>  obj-$(CONFIG_CMD_WGET) += wget.o
> diff --git a/test/cmd/test_sysinfo.c b/test/cmd/test_sysinfo.c
> new file mode 100644
> index 00000000000..7ba6dd0df89
> --- /dev/null
> +++ b/test/cmd/test_sysinfo.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Tests for sysinfo command
> + *
> + * Copyright 2023, Detlev Casanova <detlev.casanova@collabora.com>
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <asm/global_data.h>
> +#include <display_options.h>
> +#include <test/lib.h>
> +#include <test/test.h>
> +#include <test/ut.h>
> +#include <version.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#define REV_(x, y) #x "." #y
> +#define REV(x, y) REV_(x, y)
> +
> +struct test_data {
> +       char *cmd;
> +       char *expected;
> +};
> +
> +static struct test_data sysinfo_data[] = {
> +       {"sysinfo model", "sandbox"},
> +       {"sysinfo id", "0x42"},
> +       {"sysinfo revision", REV(U_BOOT_VERSION_NUM, U_BOOT_VERSION_NUM_PATCH)},

I would prefer that write out the lines in code. I don't think it
helps much to put these in a table.

> +};
> +
> +static int lib_test_sysinfo(struct unit_test_state *uts)
> +{
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(sysinfo_data); ++i) {
> +               ut_silence_console(uts);
> +               console_record_reset_enable();

drop those two lines

> +               ut_assertok(run_command(sysinfo_data[i].cmd, 0));
> +               ut_unsilence_console(uts);
> +               console_record_readline(uts->actual_str,
> +                                       sizeof(uts->actual_str));
> +               ut_asserteq_str(sysinfo_data[i].expected, uts->actual_str);

ut_assert_nextline(sysinfo_data[i].expected, uts->actual_str);

> +               ut_assertok(ut_check_console_end(uts));
> +       }
> +
> +       return 0;
> +}
> +
> +LIB_TEST(lib_test_sysinfo, 0);

Add UT_TESTF_CONSOLE_REC

> --
> 2.41.0
>

Regards,
Simon

[PATCH v5 4/6] sysinfo: Add documentation

[Detlev Casanova]

Add documentation for the sysinfo command with examples.

Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 doc/usage/cmd/sysinfo.rst | 56 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 doc/usage/cmd/sysinfo.rst

diff --git a/doc/usage/cmd/sysinfo.rst b/doc/usage/cmd/sysinfo.rst
new file mode 100644
index 00000000000..1660b2aa1a6
--- /dev/null
+++ b/doc/usage/cmd/sysinfo.rst
@@ -0,0 +1,56 @@
+.. SPDX-License-Identifier: GPL-2.0+:
+
+sysinfo command
+===============
+
+Synopis
+-------
+
+::
+
+    sysinfo id <varname>
+    sysinfo model <varname>
+    sysinfo revision <varname>
+
+Description
+-----------
+
+The `sysinfo` command is used to show information about the running system
+
+The `sysinfo id` command prints or sets an environment variable to the board id
+as an hex value.
+
+    varname
+        an optional environment variable to store the board id into.
+
+The `sysinfo model` command prints or sets an environment variable to the board
+model name as a string value.
+
+    varname
+        an optional environment variable to store the board model name into.
+
+The `sysinfo revision` command prints or sets an environment variable to the
+board revision in the <MAJOR>.<MINOR> format, where MINOR and MINOR are int
+values.
+
+    varname
+        an optional environment variable to store the board revision into.
+
+Examples
+--------
+
+::
+
+    => sysinfo id
+    0x0b
+    => sysinfo model
+    Renesas Starter Kit Premier board rev 2.1
+    => sysinfo revision varname
+    => env print varname
+    2.1
+
+Return value
+------------
+
+The return value $? is set to 0 (true) if the command succeeded. If an
+error occurs, the return value $? is set to 1 (false).
-- 
2.41.0

Re: [PATCH v5 4/6] sysinfo: Add documentation

[Simon Glass]

Hi Detlev,

On Mon, 2 Oct 2023 at 09:22, Detlev Casanova
<detlev.casanova@collabora.com> wrote:
>
> Add documentation for the sysinfo command with examples.
>
> Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  doc/usage/cmd/sysinfo.rst | 56 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 doc/usage/cmd/sysinfo.rst

Reviewed-by: Simon Glass <sjg@chromium.org>
>
> diff --git a/doc/usage/cmd/sysinfo.rst b/doc/usage/cmd/sysinfo.rst
> new file mode 100644
> index 00000000000..1660b2aa1a6
> --- /dev/null
> +++ b/doc/usage/cmd/sysinfo.rst
> @@ -0,0 +1,56 @@
> +.. SPDX-License-Identifier: GPL-2.0+:
> +
> +sysinfo command
> +===============
> +
> +Synopis
> +-------
> +
> +::
> +
> +    sysinfo id <varname>

[<varname>]

I think, since the params are optional?

> +    sysinfo model <varname>
> +    sysinfo revision <varname>
> +
> +Description
> +-----------
> +
> +The `sysinfo` command is used to show information about the running system
> +
> +The `sysinfo id` command prints or sets an environment variable to the board id
> +as an hex value.
> +
> +    varname
> +        an optional environment variable to store the board id into.
> +
> +The `sysinfo model` command prints or sets an environment variable to the board
> +model name as a string value.
> +
> +    varname
> +        an optional environment variable to store the board model name into.
> +
> +The `sysinfo revision` command prints or sets an environment variable to the
> +board revision in the <MAJOR>.<MINOR> format, where MINOR and MINOR are int
> +values.
> +
> +    varname
> +        an optional environment variable to store the board revision into.
> +
> +Examples
> +--------
> +
> +::
> +
> +    => sysinfo id
> +    0x0b
> +    => sysinfo model
> +    Renesas Starter Kit Premier board rev 2.1
> +    => sysinfo revision varname
> +    => env print varname
> +    2.1
> +
> +Return value
> +------------
> +
> +The return value $? is set to 0 (true) if the command succeeded. If an
> +error occurs, the return value $? is set to 1 (false).
> --
> 2.41.0
>

Regards.

Simon

Re: [PATCH v5 4/6] sysinfo: Add documentation

[Heinrich Schuchardt]

On 10/2/23 17:20, Detlev Casanova wrote:
> Add documentation for the sysinfo command with examples.
>
> Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>   doc/usage/cmd/sysinfo.rst | 56 +++++++++++++++++++++++++++++++++++++++

Thank you for providing a man-page.

Unfortunately this does not build:
usage/cmd/sysinfo.rst:document isn't included in any toctree

Please, add sysinfo to doc/usage/index.rst.

Please, use run 'make htmldocs' before resubmitting.

>   1 file changed, 56 insertions(+)
>   create mode 100644 doc/usage/cmd/sysinfo.rst
>
> diff --git a/doc/usage/cmd/sysinfo.rst b/doc/usage/cmd/sysinfo.rst
> new file mode 100644
> index 00000000000..1660b2aa1a6
> --- /dev/null
> +++ b/doc/usage/cmd/sysinfo.rst
> @@ -0,0 +1,56 @@
> +.. SPDX-License-Identifier: GPL-2.0+:
> +
> +sysinfo command
> +===============
> +
> +Synopis

%s/Synopis/Synopsis/

> +-------
> +
> +::
> +
> +    sysinfo id <varname>
> +    sysinfo model <varname>
> +    sysinfo revision <varname>
> +
> +Description
> +-----------
> +
> +The `sysinfo` command is used to show information about the running system
> +
> +The `sysinfo id` command prints or sets an environment variable to the board id
> +as an hex value.

%s/an hex/a hexadecimal/

> +
> +    varname
> +        an optional environment variable to store the board id into.
> +
> +The `sysinfo model` command prints or sets an environment variable to the board
> +model name as a string value.
> +
> +    varname
> +        an optional environment variable to store the board model name into.

If varname were optional, the synopsis would be

sysinfo model [varname]

> +
> +The `sysinfo revision` command prints or sets an environment variable to the
> +board revision in the <MAJOR>.<MINOR> format, where MINOR and MINOR are int
> +values.
> +
> +    varname
> +        an optional environment variable to store the board revision into.
> +
> +Examples
> +--------
> +
> +::
> +
> +    => sysinfo id
> +    0x0b
> +    => sysinfo model
> +    Renesas Starter Kit Premier board rev 2.1
> +    => sysinfo revision varname
> +    => env print varname
> +    2.1

This is too much hassle to print the information.

We should make the parameters optional:

'sysinfo' should print all information.
'sysinfo revision' should print the revision.


> +
> +Return value
> +------------
> +
> +The return value $? is set to 0 (true) if the command succeeded. If an
> +error occurs, the return value $? is set to 1 (false).

Please, use the same tense:

%s/succeeded/succeeds/

Best regards

Heinrich

[PATCH v5 5/6] sysinfo: rcar3: Use int instead of char for revision

[Detlev Casanova]

To be used with the sysinfo command, revision values must be considered
as integers, not chars as some boards will implement BOARD_REVISION_*
and might use numbers greater than 9.

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 drivers/sysinfo/rcar3.c | 73 ++++++++++++++++++++++-------------------
 1 file changed, 39 insertions(+), 34 deletions(-)

diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
index 7b127986da7..633e80bc19b 100644
--- a/drivers/sysinfo/rcar3.c
+++ b/drivers/sysinfo/rcar3.c
@@ -68,8 +68,9 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 	bool salvator_xs = false;
 	bool ebisu_4d = false;
 	bool condor_i = false;
-	char rev_major = '?';
-	char rev_minor = '?';
+	char rev[4] = "?.?";
+	u8 rev_major = 0;
+	u8 rev_minor = 0;
 
 	switch (board_id) {
 	case BOARD_SALVATOR_XS:
@@ -77,81 +78,85 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 		fallthrough;
 	case BOARD_SALVATOR_X:
 		if (!(board_rev & ~1)) { /* Only rev 0 and 1 is valid */
-			rev_major = '1';
-			rev_minor = '0' + (board_rev & BIT(0));
+			rev_major = 1;
+			rev_minor = board_rev & BIT(0);
+			snprintf(rev, sizeof(rev), "%u.%u", rev_major, rev_minor);
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
-			 "Renesas Salvator-X%s board rev %c.%c",
-			 salvator_xs ? "S" : "", rev_major, rev_minor);
+			 "Renesas Salvator-X%s board rev %s",
+			 salvator_xs ? "S" : "", rev);
+
 		return;
 	case BOARD_STARTER_KIT:
 		if (!(board_rev & ~1)) { /* Only rev 0 and 1 is valid */
-			rev_major = (board_rev & BIT(0)) ? '3' : '1';
-			rev_minor = '0';
+			rev_major = (board_rev & BIT(0)) ? 3 : 1;
+			rev_minor = 0;
+			snprintf(rev, sizeof(rev), "%u.%u", rev_major, rev_minor);
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
-			 "Renesas Starter Kit board rev %c.%c",
-			 rev_major, rev_minor);
+			 "Renesas Starter Kit board rev %s", rev);
 		return;
 	case BOARD_STARTER_KIT_PRE:
 		if (!(board_rev & ~3)) { /* Only rev 0..3 is valid */
-			rev_major = (board_rev & BIT(1)) ? '2' : '1';
-			rev_minor = (board_rev == 3) ? '1' : '0';
+			rev_major = (board_rev & BIT(1)) ? 2 : 1;
+			rev_minor = (board_rev == 3) ? 1 : 0;
+			snprintf(rev, sizeof(rev), "%u.%u", rev_major, rev_minor);
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
-			 "Renesas Starter Kit Premier board rev %c.%c",
-			 rev_major, rev_minor);
+			 "Renesas Starter Kit Premier board rev %s", rev);
 		return;
 	case BOARD_EAGLE:
 		if (!board_rev) { /* Only rev 0 is valid */
-			rev_major = '1';
-			rev_minor = '0';
+			rev_major = 1;
+			rev_minor = 0;
+			snprintf(rev, sizeof(rev), "%u.%u", rev_major, rev_minor);
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
-			 "Renesas Eagle board rev %c.%c",
-			 rev_major, rev_minor);
+			 "Renesas Eagle board rev %s", rev);
 		return;
 	case BOARD_EBISU_4D:
 		ebisu_4d = true;
 		fallthrough;
 	case BOARD_EBISU:
 		if (!board_rev) { /* Only rev 0 is valid */
-			rev_major = '1';
-			rev_minor = '0';
+			rev_major = 1;
+			rev_minor = 0;
+			snprintf(rev, sizeof(rev), "%u.%u", rev_major, rev_minor);
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
-			 "Renesas Ebisu%s board rev %c.%c",
-			 ebisu_4d ? "-4D" : "", rev_major, rev_minor);
+			 "Renesas Ebisu%s board rev %s",
+			 ebisu_4d ? "-4D" : "", rev);
 		return;
 	case BOARD_DRAAK:
 		if (!board_rev) { /* Only rev 0 is valid */
-			rev_major = '1';
-			rev_minor = '0';
+			rev_major = 1;
+			rev_minor = 0;
+			snprintf(rev, sizeof(rev), "%u.%u", rev_major, rev_minor);
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
-			 "Renesas Draak board rev %c.%c",
-			 rev_major, rev_minor);
+			 "Renesas Draak board rev %s", rev);
 		return;
 	case BOARD_KRIEK:
 		if (!board_rev) { /* Only rev 0 is valid */
-			rev_major = '1';
-			rev_minor = '0';
+			rev_major = 1;
+			rev_minor = 0;
+			snprintf(rev, sizeof(rev), "%u.%u", rev_major, rev_minor);
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
-			 "Renesas Kriek board rev %c.%c",
-			 rev_major, rev_minor);
+			 "Renesas Kriek board rev %s", rev);
 		return;
 	case BOARD_CONDOR_I:
 		condor_i = true;
 		fallthrough;
 	case BOARD_CONDOR:
 		if (!board_rev) { /* Only rev 0 is valid */
-			rev_major = '1';
-			rev_minor = '0';
+			rev_major = 1;
+			rev_minor = 0;
+			snprintf(rev, sizeof(rev), "%u.%u", rev_major, rev_minor);
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
-			"Renesas Condor%s board rev %c.%c",
-			condor_i ? "-I" : "", rev_major, rev_minor);
+			 "Renesas Condor%s board rev %s",
+			 condor_i ? "-I" : "", rev);
 		return;
 	default:
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
-- 
2.41.0

Re: [PATCH v5 5/6] sysinfo: rcar3: Use int instead of char for revision

[Marek Vasut]

On 10/2/23 17:20, Detlev Casanova wrote:
> To be used with the sysinfo command, revision values must be considered
> as integers, not chars as some boards will implement BOARD_REVISION_*
> and might use numbers greater than 9.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>   drivers/sysinfo/rcar3.c | 73 ++++++++++++++++++++++-------------------
>   1 file changed, 39 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
> index 7b127986da7..633e80bc19b 100644
> --- a/drivers/sysinfo/rcar3.c
> +++ b/drivers/sysinfo/rcar3.c
> @@ -68,8 +68,9 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
>   	bool salvator_xs = false;
>   	bool ebisu_4d = false;
>   	bool condor_i = false;
> -	char rev_major = '?';
> -	char rev_minor = '?';
> +	char rev[4] = "?.?";
> +	u8 rev_major = 0;
> +	u8 rev_minor = 0;
>   
>   	switch (board_id) {
>   	case BOARD_SALVATOR_XS:
> @@ -77,81 +78,85 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
>   		fallthrough;
>   	case BOARD_SALVATOR_X:
>   		if (!(board_rev & ~1)) { /* Only rev 0 and 1 is valid */
> -			rev_major = '1';
> -			rev_minor = '0' + (board_rev & BIT(0));
> +			rev_major = 1;
> +			rev_minor = board_rev & BIT(0);
> +			snprintf(rev, sizeof(rev), "%u.%u", rev_major, rev_minor);
>   		}
>   		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
> -			 "Renesas Salvator-X%s board rev %c.%c",
> -			 salvator_xs ? "S" : "", rev_major, rev_minor);
> +			 "Renesas Salvator-X%s board rev %s",
> +			 salvator_xs ? "S" : "", rev);
> +
>   		return;

Extra newline got added just before the return here. If you want to do 
V6 with that fixed, that would be nice. In either case:

Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

Thanks !

[PATCH v5 6/6] sysinfo: rcar3: Implement BOARD_ID and BOARD_REV_*

[Detlev Casanova]

Expose that information to the sysinfo command to let scripts make
decisions based on the board id and revision.

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 drivers/sysinfo/rcar3.c | 89 +++++++++++++++++++++++++++++------------
 1 file changed, 63 insertions(+), 26 deletions(-)

diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
index 633e80bc19b..a554323506a 100644
--- a/drivers/sysinfo/rcar3.c
+++ b/drivers/sysinfo/rcar3.c
@@ -32,6 +32,10 @@
  */
 struct sysinfo_rcar_priv {
 	char	boardmodel[64];
+	u8	id;
+	u8	rev_major;
+	u8	rev_minor;
+	bool	has_rev;
 	u8	val;
 };
 
@@ -56,9 +60,33 @@ static int sysinfo_rcar_get_str(struct udevice *dev, int id, size_t size, char *
 	};
 }
 
+static int sysinfo_rcar_get_int(struct udevice *dev, int id, int *val)
+{
+	struct sysinfo_rcar_priv *priv = dev_get_priv(dev);
+
+	switch (id) {
+	case SYSINFO_ID_BOARD_ID:
+		*val = priv->id;
+		return 0;
+	case SYSINFO_ID_BOARD_REV_MAJOR:
+		if (!priv->has_rev)
+			return -EINVAL;
+		*val = priv->rev_major;
+		return 0;
+	case SYSINFO_ID_BOARD_REV_MINOR:
+		if (!priv->has_rev)
+			return -EINVAL;
+		*val = priv->rev_minor;
+		return 0;
+	default:
+		return -EINVAL;
+	};
+}
+
 static const struct sysinfo_ops sysinfo_rcar_ops = {
 	.detect = sysinfo_rcar_detect,
 	.get_str = sysinfo_rcar_get_str,
+	.get_int = sysinfo_rcar_get_int,
 };
 
 static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
@@ -69,8 +97,9 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 	bool ebisu_4d = false;
 	bool condor_i = false;
 	char rev[4] = "?.?";
-	u8 rev_major = 0;
-	u8 rev_minor = 0;
+
+	priv->id = board_id;
+	priv->has_rev = false;
 
 	switch (board_id) {
 	case BOARD_SALVATOR_XS:
@@ -78,9 +107,10 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 		fallthrough;
 	case BOARD_SALVATOR_X:
 		if (!(board_rev & ~1)) { /* Only rev 0 and 1 is valid */
-			rev_major = 1;
-			rev_minor = board_rev & BIT(0);
-			snprintf(rev, sizeof(rev), "%u.%u", rev_major, rev_minor);
+			priv->rev_major = 1;
+			priv->rev_minor = board_rev & BIT(0);
+			priv->has_rev = true;
+			snprintf(rev, sizeof(rev), "%u.%u", priv->rev_major, priv->rev_minor);
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Salvator-X%s board rev %s",
@@ -89,27 +119,30 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 		return;
 	case BOARD_STARTER_KIT:
 		if (!(board_rev & ~1)) { /* Only rev 0 and 1 is valid */
-			rev_major = (board_rev & BIT(0)) ? 3 : 1;
-			rev_minor = 0;
-			snprintf(rev, sizeof(rev), "%u.%u", rev_major, rev_minor);
+			priv->rev_major = (board_rev & BIT(0)) ? 3 : 1;
+			priv->rev_minor = 0;
+			snprintf(rev, sizeof(rev), "%u.%u", priv->rev_major, priv->rev_minor);
+			priv->has_rev = true;
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Starter Kit board rev %s", rev);
 		return;
 	case BOARD_STARTER_KIT_PRE:
 		if (!(board_rev & ~3)) { /* Only rev 0..3 is valid */
-			rev_major = (board_rev & BIT(1)) ? 2 : 1;
-			rev_minor = (board_rev == 3) ? 1 : 0;
-			snprintf(rev, sizeof(rev), "%u.%u", rev_major, rev_minor);
+			priv->rev_major = (board_rev & BIT(1)) ? 2 : 1;
+			priv->rev_minor = (board_rev == 3) ? 1 : 0;
+			snprintf(rev, sizeof(rev), "%u.%u", priv->rev_major, priv->rev_minor);
+			priv->has_rev = true;
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Starter Kit Premier board rev %s", rev);
 		return;
 	case BOARD_EAGLE:
 		if (!board_rev) { /* Only rev 0 is valid */
-			rev_major = 1;
-			rev_minor = 0;
-			snprintf(rev, sizeof(rev), "%u.%u", rev_major, rev_minor);
+			priv->rev_major = 1;
+			priv->rev_minor = 0;
+			snprintf(rev, sizeof(rev), "%u.%u", priv->rev_major, priv->rev_minor);
+			priv->has_rev = true;
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Eagle board rev %s", rev);
@@ -119,9 +152,10 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 		fallthrough;
 	case BOARD_EBISU:
 		if (!board_rev) { /* Only rev 0 is valid */
-			rev_major = 1;
-			rev_minor = 0;
-			snprintf(rev, sizeof(rev), "%u.%u", rev_major, rev_minor);
+			priv->rev_major = 1;
+			priv->rev_minor = 0;
+			snprintf(rev, sizeof(rev), "%u.%u", priv->rev_major, priv->rev_minor);
+			priv->has_rev = true;
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Ebisu%s board rev %s",
@@ -129,18 +163,20 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 		return;
 	case BOARD_DRAAK:
 		if (!board_rev) { /* Only rev 0 is valid */
-			rev_major = 1;
-			rev_minor = 0;
-			snprintf(rev, sizeof(rev), "%u.%u", rev_major, rev_minor);
+			priv->rev_major = 1;
+			priv->rev_minor = 0;
+			snprintf(rev, sizeof(rev), "%u.%u", priv->rev_major, priv->rev_minor);
+			priv->has_rev = true;
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Draak board rev %s", rev);
 		return;
 	case BOARD_KRIEK:
 		if (!board_rev) { /* Only rev 0 is valid */
-			rev_major = 1;
-			rev_minor = 0;
-			snprintf(rev, sizeof(rev), "%u.%u", rev_major, rev_minor);
+			priv->rev_major = 1;
+			priv->rev_minor = 0;
+			snprintf(rev, sizeof(rev), "%u.%u", priv->rev_major, priv->rev_minor);
+			priv->has_rev = true;
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Kriek board rev %s", rev);
@@ -150,9 +186,10 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 		fallthrough;
 	case BOARD_CONDOR:
 		if (!board_rev) { /* Only rev 0 is valid */
-			rev_major = 1;
-			rev_minor = 0;
-			snprintf(rev, sizeof(rev), "%u.%u", rev_major, rev_minor);
+			priv->rev_major = 1;
+			priv->rev_minor = 0;
+			snprintf(rev, sizeof(rev), "%u.%u", priv->rev_major, priv->rev_minor);
+			priv->has_rev = true;
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Condor%s board rev %s",
-- 
2.41.0

Re: [PATCH v5 6/6] sysinfo: rcar3: Implement BOARD_ID and BOARD_REV_*

[Marek Vasut]

On 10/2/23 17:20, Detlev Casanova wrote:
> Expose that information to the sysinfo command to let scripts make
> decisions based on the board id and revision.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>   drivers/sysinfo/rcar3.c | 89 +++++++++++++++++++++++++++++------------
>   1 file changed, 63 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
> index 633e80bc19b..a554323506a 100644
> --- a/drivers/sysinfo/rcar3.c
> +++ b/drivers/sysinfo/rcar3.c
> @@ -32,6 +32,10 @@
>    */
>   struct sysinfo_rcar_priv {
>   	char	boardmodel[64];
> +	u8	id;
> +	u8	rev_major;
> +	u8	rev_minor;
> +	bool	has_rev;
>   	u8	val;
>   };
>   
> @@ -56,9 +60,33 @@ static int sysinfo_rcar_get_str(struct udevice *dev, int id, size_t size, char *
>   	};
>   }
>   
> +static int sysinfo_rcar_get_int(struct udevice *dev, int id, int *val)
> +{
> +	struct sysinfo_rcar_priv *priv = dev_get_priv(dev);
> +
> +	switch (id) {
> +	case SYSINFO_ID_BOARD_ID:
> +		*val = priv->id;
> +		return 0;
> +	case SYSINFO_ID_BOARD_REV_MAJOR:
> +		if (!priv->has_rev)
> +			return -EINVAL;
> +		*val = priv->rev_major;
> +		return 0;
> +	case SYSINFO_ID_BOARD_REV_MINOR:
> +		if (!priv->has_rev)
> +			return -EINVAL;
> +		*val = priv->rev_minor;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	};
> +}
> +
>   static const struct sysinfo_ops sysinfo_rcar_ops = {
>   	.detect = sysinfo_rcar_detect,
>   	.get_str = sysinfo_rcar_get_str,
> +	.get_int = sysinfo_rcar_get_int,
>   };
>   
>   static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
> @@ -69,8 +97,9 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
>   	bool ebisu_4d = false;
>   	bool condor_i = false;
>   	char rev[4] = "?.?";
> -	u8 rev_major = 0;
> -	u8 rev_minor = 0;
> +
> +	priv->id = board_id;
> +	priv->has_rev = false;

Would it make more sense to assign

priv->rev_major = 1;
priv->rev_minor = 0;

And get rid of priv->has_rev entirely ?

Basically say that the default case is rev. 1.0 board.

[...]

Re: [PATCH v5 6/6] sysinfo: rcar3: Implement BOARD_ID and BOARD_REV_*

[Detlev Casanova]

On Saturday, October 7, 2023 5:35:27 P.M. EDT Marek Vasut wrote:
> On 10/2/23 17:20, Detlev Casanova wrote:
> > Expose that information to the sysinfo command to let scripts make
> > decisions based on the board id and revision.
> > 
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >   drivers/sysinfo/rcar3.c | 89 +++++++++++++++++++++++++++++------------
> >   1 file changed, 63 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
> > index 633e80bc19b..a554323506a 100644
> > --- a/drivers/sysinfo/rcar3.c
> > +++ b/drivers/sysinfo/rcar3.c
> > @@ -32,6 +32,10 @@
> > 
> >    */
> >   
> >   struct sysinfo_rcar_priv {
> >   
> >   	char	boardmodel[64];
> > 
> > +	u8	id;
> > +	u8	rev_major;
> > +	u8	rev_minor;
> > +	bool	has_rev;
> > 
> >   	u8	val;
> >   
> >   };
> > 
> > @@ -56,9 +60,33 @@ static int sysinfo_rcar_get_str(struct udevice *dev,
> > int id, size_t size, char *> 
> >   	};
> >   
> >   }
> > 
> > +static int sysinfo_rcar_get_int(struct udevice *dev, int id, int *val)
> > +{
> > +	struct sysinfo_rcar_priv *priv = dev_get_priv(dev);
> > +
> > +	switch (id) {
> > +	case SYSINFO_ID_BOARD_ID:
> > +		*val = priv->id;
> > +		return 0;
> > +	case SYSINFO_ID_BOARD_REV_MAJOR:
> > +		if (!priv->has_rev)
> > +			return -EINVAL;
> > +		*val = priv->rev_major;
> > +		return 0;
> > +	case SYSINFO_ID_BOARD_REV_MINOR:
> > +		if (!priv->has_rev)
> > +			return -EINVAL;
> > +		*val = priv->rev_minor;
> > +		return 0;
> > +	default:
> > +		return -EINVAL;
> > +	};
> > +}
> > +
> > 
> >   static const struct sysinfo_ops sysinfo_rcar_ops = {
> >   
> >   	.detect = sysinfo_rcar_detect,
> >   	.get_str = sysinfo_rcar_get_str,
> > 
> > +	.get_int = sysinfo_rcar_get_int,
> > 
> >   };
> >   
> >   static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
> > 
> > @@ -69,8 +97,9 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv
> > *priv)> 
> >   	bool ebisu_4d = false;
> >   	bool condor_i = false;
> >   	char rev[4] = "?.?";
> > 
> > -	u8 rev_major = 0;
> > -	u8 rev_minor = 0;
> > +
> > +	priv->id = board_id;
> > +	priv->has_rev = false;
> 
> Would it make more sense to assign
> 
> priv->rev_major = 1;
> priv->rev_minor = 0;
> 
> And get rid of priv->has_rev entirely ?
> 
> Basically say that the default case is rev. 1.0 board.
> 
> [...]

I'm not really sure about this has it doesn't differentiate between rev 1.0 and 
unknown revision.

Re: [PATCH v5 6/6] sysinfo: rcar3: Implement BOARD_ID and BOARD_REV_*

[Marek Vasut]

On 10/16/23 19:03, Detlev Casanova wrote:
> On Saturday, October 7, 2023 5:35:27 P.M. EDT Marek Vasut wrote:
>> On 10/2/23 17:20, Detlev Casanova wrote:
>>> Expose that information to the sysinfo command to let scripts make
>>> decisions based on the board id and revision.
>>>
>>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>>> ---
>>>
>>>    drivers/sysinfo/rcar3.c | 89 +++++++++++++++++++++++++++++------------
>>>    1 file changed, 63 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
>>> index 633e80bc19b..a554323506a 100644
>>> --- a/drivers/sysinfo/rcar3.c
>>> +++ b/drivers/sysinfo/rcar3.c
>>> @@ -32,6 +32,10 @@
>>>
>>>     */
>>>    
>>>    struct sysinfo_rcar_priv {
>>>    
>>>    	char	boardmodel[64];
>>>
>>> +	u8	id;
>>> +	u8	rev_major;
>>> +	u8	rev_minor;
>>> +	bool	has_rev;
>>>
>>>    	u8	val;
>>>    
>>>    };
>>>
>>> @@ -56,9 +60,33 @@ static int sysinfo_rcar_get_str(struct udevice *dev,
>>> int id, size_t size, char *>
>>>    	};
>>>    
>>>    }
>>>
>>> +static int sysinfo_rcar_get_int(struct udevice *dev, int id, int *val)
>>> +{
>>> +	struct sysinfo_rcar_priv *priv = dev_get_priv(dev);
>>> +
>>> +	switch (id) {
>>> +	case SYSINFO_ID_BOARD_ID:
>>> +		*val = priv->id;
>>> +		return 0;
>>> +	case SYSINFO_ID_BOARD_REV_MAJOR:
>>> +		if (!priv->has_rev)
>>> +			return -EINVAL;
>>> +		*val = priv->rev_major;
>>> +		return 0;
>>> +	case SYSINFO_ID_BOARD_REV_MINOR:
>>> +		if (!priv->has_rev)
>>> +			return -EINVAL;
>>> +		*val = priv->rev_minor;
>>> +		return 0;
>>> +	default:
>>> +		return -EINVAL;
>>> +	};
>>> +}
>>> +
>>>
>>>    static const struct sysinfo_ops sysinfo_rcar_ops = {
>>>    
>>>    	.detect = sysinfo_rcar_detect,
>>>    	.get_str = sysinfo_rcar_get_str,
>>>
>>> +	.get_int = sysinfo_rcar_get_int,
>>>
>>>    };
>>>    
>>>    static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
>>>
>>> @@ -69,8 +97,9 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv
>>> *priv)>
>>>    	bool ebisu_4d = false;
>>>    	bool condor_i = false;
>>>    	char rev[4] = "?.?";
>>>
>>> -	u8 rev_major = 0;
>>> -	u8 rev_minor = 0;
>>> +
>>> +	priv->id = board_id;
>>> +	priv->has_rev = false;
>>
>> Would it make more sense to assign
>>
>> priv->rev_major = 1;
>> priv->rev_minor = 0;
>>
>> And get rid of priv->has_rev entirely ?
>>
>> Basically say that the default case is rev. 1.0 board.
>>
>> [...]
> 
> I'm not really sure about this has it doesn't differentiate between rev 1.0 and
> unknown revision.

Good point, please document this in the commit message.