[PATCH v3 0/5] Introduce the sysinfo command

[PATCH v3 0/5] 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 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   | 141 ++++++++++++++++++++++++++------------
 drivers/sysinfo/sandbox.c |  17 +++++
 include/sysinfo.h         |   5 ++
 test/cmd/Makefile         |   1 +
 test/cmd/test_sysinfo.c   |  51 ++++++++++++++
 10 files changed, 370 insertions(+), 42 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 v3 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

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 v3 1/6] sysinfo: Add IDs for board id and revision

[Marek Vasut]

On 7/14/23 18:43, 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
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>

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

[PATCH v3 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.

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 c1941849f98..0df6d7e8bb7 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -210,6 +210,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 6c37521b4e2..ba4d6de9a1b 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -165,6 +165,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 v3 2/6] cmd: Add a sysinfo command

[Marek Vasut]

On 7/14/23 18:43, Detlev Casanova 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.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>

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

[PATCH v3 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.

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 1ec44d5b33b..bdaac205d02 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -128,6 +128,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 a3cf983739e..d3ac5bf2d5e 100644
--- a/test/cmd/Makefile
+++ b/test/cmd/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_CMD_SEAMA) += seama.o
 ifdef CONFIG_SANDBOX
 obj-$(CONFIG_CMD_READ) += rw.o
 obj-$(CONFIG_CMD_SETEXPR) += setexpr.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 v3 3/6] sysinfo: Add a test

[Marek Vasut]

On 7/14/23 18:43, Detlev Casanova wrote:
> The test runs one of each subcommand and checks that the output matches
> the values in the sandbox sysinfo driver.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>

Nice

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

[PATCH v3 4/6] sysinfo: Add documentation

[Detlev Casanova]

Add documentation for the sysinfo command with examples.

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..3dad1f4f38d
--- /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 succededd. If an
+error occurs, the return value $? is set to 1 (false).
-- 
2.41.0

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

[Marek Vasut]

On 7/14/23 18:43, Detlev Casanova wrote:

[...]

> +Return value
> +------------
> +
> +The return value $? is set to 0 (true) if the command succededd

succeeded -- typo

. If an
> +error occurs, the return value $? is set to 1 (false).

With that fixed:

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

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

[Detlev Casanova]

On Friday, July 14, 2023 1:31:00 P.M. EDT Marek Vasut wrote:
> On 7/14/23 18:43, Detlev Casanova wrote:
> 
> [...]
> 
> > +Return value
> > +------------
> > +
> > +The return value $? is set to 0 (true) if the command succededd
> 
> succeeded -- typo
> 
> . If an
> 
> > +error occurs, the return value $? is set to 1 (false).
> 
> With that fixed:
> 
> Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

Do you need me to make a patch v4 to fix this ?

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

[Marek Vasut]

On 7/17/23 14:27, Detlev Casanova wrote:
> On Friday, July 14, 2023 1:31:00 P.M. EDT Marek Vasut wrote:
>> On 7/14/23 18:43, Detlev Casanova wrote:
>>
>> [...]
>>
>>> +Return value
>>> +------------
>>> +
>>> +The return value $? is set to 0 (true) if the command succededd
>>
>> succeeded -- typo
>>
>> . If an
>>
>>> +error occurs, the return value $? is set to 1 (false).
>>
>> With that fixed:
>>
>> Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> 
> Do you need me to make a patch v4 to fix this ?

Yes please, although I see a v4 already, thanks !

[PATCH v3 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 | 104 ++++++++++++++++++++++++----------------
 1 file changed, 62 insertions(+), 42 deletions(-)

diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
index 7b127986da7..450a4c26773 100644
--- a/drivers/sysinfo/rcar3.c
+++ b/drivers/sysinfo/rcar3.c
@@ -68,90 +68,110 @@ 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 model[64];
+	char rev[4] = "?.?";
+	u8 rev_major = 0;
+	u8 rev_minor = 0;
 
 	switch (board_id) {
 	case BOARD_SALVATOR_XS:
 		salvator_xs = true;
 		fallthrough;
 	case BOARD_SALVATOR_X:
+		snprintf(model, sizeof(model),
+			 "Renesas Salvator-X%s board", salvator_xs ? "S" : "");
 		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);
+
+		snprintf(priv->boardmodel, sizeof(priv->boardmodel), "%s rev %s",
+			 model, rev);
+
 		return;
 	case BOARD_STARTER_KIT:
+		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
+			 "Renesas Starter Kit board");
 		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);
+		snprintf(priv->boardmodel, sizeof(priv->boardmodel), "%s rev %s",
+			 model, rev);
 		return;
 	case BOARD_STARTER_KIT_PRE:
+		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
+			 "Renesas Starter Kit Premier board");
 		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);
+		snprintf(priv->boardmodel, sizeof(priv->boardmodel), "%s rev %s",
+			 model, rev);
 		return;
 	case BOARD_EAGLE:
+		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
+			 "Renesas Eagle board");
 		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);
+		snprintf(priv->boardmodel, sizeof(priv->boardmodel), "%s rev %s",
+			 model, rev);
 		return;
 	case BOARD_EBISU_4D:
 		ebisu_4d = true;
 		fallthrough;
 	case BOARD_EBISU:
+		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
+			 "Renesas Ebisu%s board", ebisu_4d ? "-4D" : "");
 		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);
+		snprintf(priv->boardmodel, sizeof(priv->boardmodel), "%s rev %s",
+			 model, rev);
 		return;
 	case BOARD_DRAAK:
+		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
+			 "Renesas Draak board");
 		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);
+		snprintf(priv->boardmodel, sizeof(priv->boardmodel), "%s rev %s",
+			 model, rev);
 		return;
 	case BOARD_KRIEK:
+		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
+			 "Renesas Kriek board");
 		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);
+		snprintf(priv->boardmodel, sizeof(priv->boardmodel), "%s rev %s",
+			 model, rev);
 		return;
 	case BOARD_CONDOR_I:
 		condor_i = true;
 		fallthrough;
 	case BOARD_CONDOR:
+		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
+			 "Renesas Condor%s board", condor_i ? "-I" : "");
 		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);
+		snprintf(priv->boardmodel, sizeof(priv->boardmodel), "%s rev %s",
+			 model, rev);
 		return;
 	default:
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
-- 
2.41.0

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

[Marek Vasut]

On 7/14/23 18:43, 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 | 104 ++++++++++++++++++++++++----------------
>   1 file changed, 62 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
> index 7b127986da7..450a4c26773 100644
> --- a/drivers/sysinfo/rcar3.c
> +++ b/drivers/sysinfo/rcar3.c
> @@ -68,90 +68,110 @@ 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 model[64];
> +	char rev[4] = "?.?";
> +	u8 rev_major = 0;
> +	u8 rev_minor = 0;
>   
>   	switch (board_id) {
>   	case BOARD_SALVATOR_XS:
>   		salvator_xs = true;
>   		fallthrough;
>   	case BOARD_SALVATOR_X:
> +		snprintf(model, sizeof(model),
> +			 "Renesas Salvator-X%s board", salvator_xs ? "S" : "");
>   		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);
> +
> +		snprintf(priv->boardmodel, sizeof(priv->boardmodel), "%s rev %s",
> +			 model, rev);

Is there really no way to do this with single snprintf() call instead of 
two snprintf() calls ?

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

[Detlev Casanova]

On Friday, July 14, 2023 1:33:01 P.M. EDT Marek Vasut wrote:
> On 7/14/23 18:43, 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 | 104 ++++++++++++++++++++++++----------------
> >   1 file changed, 62 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
> > index 7b127986da7..450a4c26773 100644
> > --- a/drivers/sysinfo/rcar3.c
> > +++ b/drivers/sysinfo/rcar3.c
> > @@ -68,90 +68,110 @@ 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 model[64];
> > +	char rev[4] = "?.?";
> > +	u8 rev_major = 0;
> > +	u8 rev_minor = 0;
> > 
> >   	switch (board_id) {
> >   	
> >   	case BOARD_SALVATOR_XS:
> >   		salvator_xs = true;
> >   		fallthrough;
> >   	
> >   	case BOARD_SALVATOR_X:
> > +		snprintf(model, sizeof(model),
> > +			 "Renesas Salvator-X%s board", salvator_xs ? 
"S" : "");
> > 
> >   		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);
> > +
> > +		snprintf(priv->boardmodel, sizeof(priv->boardmodel), 
"%s rev %s",
> > +			 model, rev);
> 
> Is there really no way to do this with single snprintf() call instead of
> two snprintf() calls ?

I find it more readable like this, as opposed to have an snprintf in the if and 
one in an else block for each switch case.

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

[Marek Vasut]

On 7/14/23 20:27, Detlev Casanova wrote:
> On Friday, July 14, 2023 1:33:01 P.M. EDT Marek Vasut wrote:
>> On 7/14/23 18:43, 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 | 104 ++++++++++++++++++++++++----------------
>>>    1 file changed, 62 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
>>> index 7b127986da7..450a4c26773 100644
>>> --- a/drivers/sysinfo/rcar3.c
>>> +++ b/drivers/sysinfo/rcar3.c
>>> @@ -68,90 +68,110 @@ 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 model[64];
>>> +	char rev[4] = "?.?";
>>> +	u8 rev_major = 0;
>>> +	u8 rev_minor = 0;
>>>
>>>    	switch (board_id) {
>>>    	
>>>    	case BOARD_SALVATOR_XS:
>>>    		salvator_xs = true;
>>>    		fallthrough;
>>>    	
>>>    	case BOARD_SALVATOR_X:
>>> +		snprintf(model, sizeof(model),
>>> +			 "Renesas Salvator-X%s board", salvator_xs ?
> "S" : "");
>>>
>>>    		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);
>>> +
>>> +		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
> "%s rev %s",
>>> +			 model, rev);
>>
>> Is there really no way to do this with single snprintf() call instead of
>> two snprintf() calls ?
> 
> I find it more readable like this, as opposed to have an snprintf in the if and
> one in an else block for each switch case.

I think I don't get it here -- why can't this be done with single 
snprintf() ? I mean, the revision is always set anyway.

[PATCH v3 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 | 103 +++++++++++++++++++++++++++-------------
 1 file changed, 70 insertions(+), 33 deletions(-)

diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
index 450a4c26773..1a99642bf75 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_REVISION_MAJOR:
+		if (!priv->has_rev)
+			return -EINVAL;
+		*val = priv->rev_major;
+		return 0;
+	case SYSINFO_ID_BOARD_REVISION_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)
@@ -70,8 +98,9 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 	bool condor_i = false;
 	char model[64];
 	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:
@@ -81,9 +110,10 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 		snprintf(model, sizeof(model),
 			 "Renesas Salvator-X%s board", salvator_xs ? "S" : "");
 		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), "%s rev %s",
@@ -91,34 +121,37 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 
 		return;
 	case BOARD_STARTER_KIT:
-		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
+		snprintf(model, sizeof(model),
 			 "Renesas Starter Kit board");
 		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), "%s rev %s",
 			 model, rev);
 		return;
 	case BOARD_STARTER_KIT_PRE:
-		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
+		snprintf(model, sizeof(model),
 			 "Renesas Starter Kit Premier board");
 		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), "%s rev %s",
 			 model, rev);
 		return;
 	case BOARD_EAGLE:
-		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
+		snprintf(model, sizeof(model),
 			 "Renesas Eagle board");
 		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), "%s rev %s",
 			 model, rev);
@@ -127,34 +160,37 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 		ebisu_4d = true;
 		fallthrough;
 	case BOARD_EBISU:
-		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
+		snprintf(model, sizeof(model),
 			 "Renesas Ebisu%s board", ebisu_4d ? "-4D" : "");
 		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), "%s rev %s",
 			 model, rev);
 		return;
 	case BOARD_DRAAK:
-		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
+		snprintf(model, sizeof(model),
 			 "Renesas Draak board");
 		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), "%s rev %s",
 			 model, rev);
 		return;
 	case BOARD_KRIEK:
-		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
+		snprintf(model, sizeof(model),
 			 "Renesas Kriek board");
 		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), "%s rev %s",
 			 model, rev);
@@ -163,12 +199,13 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 		condor_i = true;
 		fallthrough;
 	case BOARD_CONDOR:
-		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
+		snprintf(model, sizeof(model),
 			 "Renesas Condor%s board", condor_i ? "-I" : "");
 		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), "%s rev %s",
 			 model, rev);
-- 
2.41.0