[PATCH v1 0/4] Introduce the sysinfo command

[PATCH v1 0/4] 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 id.

I'm not sure about the last commit, if it needs to be here upstream or
if it chould be something done by packagers.

I'm also not sure if a test should be added and where to start to test
this kind of command.

Detlev Casanova (4):
  sysinfo: Add IDs for board id and revision
  cmd: Add a sysinfo command
  sysinfo: rcar3: Implement BOARD_ID and BOARD_REVISION
  configs: rcar3: Add shell function to select the linux devicetree

 cmd/Kconfig                        |   6 ++
 cmd/Makefile                       |   1 +
 cmd/sysinfo.c                      | 124 +++++++++++++++++++++++++++++
 configs/rcar3_ulcb_defconfig       |   1 +
 drivers/sysinfo/rcar3.c            |  46 ++++++++---
 include/configs/rcar-gen3-common.h |   9 ++-
 include/sysinfo.h                  |   4 +
 7 files changed, 180 insertions(+), 11 deletions(-)
 create mode 100644 cmd/sysinfo.c

-- 
2.39.3

[PATCH v1 1/4] 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_REVISION: The board revision as a string

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 include/sysinfo.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/sysinfo.h b/include/sysinfo.h
index b140d742e93..e2a7b10b6f6 100644
--- a/include/sysinfo.h
+++ b/include/sysinfo.h
@@ -47,6 +47,10 @@ enum sysinfo_id {
 	/* For show_board_info() */
 	SYSINFO_ID_BOARD_MODEL,
 
+	/* For sysinfo command */
+	SYSINFO_ID_BOARD_ID,
+	SYSINFO_ID_BOARD_REVISION,
+
 	/* First value available for downstream/board used */
 	SYSINFO_ID_USER = 0x1000,
 };
-- 
2.39.3

[PATCH v1 2/4] 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 | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+)
 create mode 100644 cmd/sysinfo.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 365371fb511..ba4844853a1 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..31cb27ae46e
--- /dev/null
+++ b/cmd/sysinfo.c
@@ -0,0 +1,124 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2023
+ * Detlev Casanova <detlev.casanova@collabora.com>
+ */
+
+#include <command.h>
+#include <env.h>
+#include <sysinfo.h>
+#include <vsprintf.h>
+
+static int get_sysinfo(struct udevice **dev)
+{
+	int ret = sysinfo_get(dev);
+
+	if (ret) {
+		debug("Cannot get sysinfo: %d\n", ret);
+		return ret;
+	}
+
+	ret = sysinfo_detect(*dev);
+	if (ret) {
+		debug("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 ret;
+
+	ret = sysinfo_get_str(dev,
+			      SYSINFO_ID_BOARD_MODEL,
+			      64,
+			      model);
+
+	if (ret) {
+		debug("Cannot get sysinfo str: %d\n", ret);
+		return ret;
+	}
+
+	if (argc == 2)
+		env_set(argv[1], model);
+	else
+		printf("%s\n", model);
+
+	return 0;
+}
+
+static int do_sysinfo_id(struct cmd_tbl *cmdtp, int flag, int argc,
+			 char *const argv[])
+{
+	struct udevice *dev;
+	u32 board_id;
+	char board_id_str[5] = { '\0' };
+	int ret = get_sysinfo(&dev);
+
+	if (ret)
+		return ret;
+
+	ret = sysinfo_get_int(dev,
+			      SYSINFO_ID_BOARD_ID,
+			      &board_id);
+
+	if (ret) {
+		debug("Cannot get sysinfo int: %d\n", ret);
+		return ret;
+	}
+
+	sprintf(board_id_str, "0x%02x", board_id);
+	if (argc == 2)
+		env_set(argv[1], board_id_str);
+	else
+		printf("%s\n", board_id_str);
+
+	return 0;
+}
+
+static int do_sysinfo_revision(struct cmd_tbl *cmdtp, int flag, int argc,
+			       char *const argv[])
+{
+	struct udevice *dev;
+	char rev[4];
+	int ret = get_sysinfo(&dev);
+
+	if (ret)
+		return ret;
+
+	ret = sysinfo_get_str(dev,
+			      SYSINFO_ID_BOARD_REVISION,
+			      4,
+			      rev);
+
+	if (ret) {
+		debug("Cannot get sysinfo str: %d\n", ret);
+		return ret;
+	}
+
+	if (argc == 2)
+		env_set(argv[1], rev);
+	else
+		printf("%s\n", rev);
+
+	return 0;
+}
+
+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.39.3

[PATCH v1 3/4] sysinfo: rcar3: Implement BOARD_ID and BOARD_REVISION

[Detlev Casanova]

Expose that information to the command shell to let scripts select the
correct devicetree name.

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

diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
index 7b127986da7..89ad46c5422 100644
--- a/drivers/sysinfo/rcar3.c
+++ b/drivers/sysinfo/rcar3.c
@@ -32,6 +32,8 @@
  */
 struct sysinfo_rcar_priv {
 	char	boardmodel[64];
+	u8	id;
+	char	revision[4];
 	u8	val;
 };
 
@@ -48,17 +50,37 @@ static int sysinfo_rcar_get_str(struct udevice *dev, int id, size_t size, char *
 
 	switch (id) {
 	case SYSINFO_ID_BOARD_MODEL:
-		strncpy(val, priv->boardmodel, size);
-		val[size - 1] = '\0';
+		strlcpy(val, priv->boardmodel, size);
+		break;
+	case SYSINFO_ID_BOARD_REVISION:
+		strlcpy(val, priv->revision, size);
+		break;
+	default:
+		return -EINVAL;
+	};
+
+	val[size - 1] = '\0';
+	return 0;
+}
+
+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;
 	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)
@@ -83,7 +105,7 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Salvator-X%s board rev %c.%c",
 			 salvator_xs ? "S" : "", rev_major, rev_minor);
-		return;
+		break;
 	case BOARD_STARTER_KIT:
 		if (!(board_rev & ~1)) { /* Only rev 0 and 1 is valid */
 			rev_major = (board_rev & BIT(0)) ? '3' : '1';
@@ -92,7 +114,7 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Starter Kit board rev %c.%c",
 			 rev_major, rev_minor);
-		return;
+		break;
 	case BOARD_STARTER_KIT_PRE:
 		if (!(board_rev & ~3)) { /* Only rev 0..3 is valid */
 			rev_major = (board_rev & BIT(1)) ? '2' : '1';
@@ -101,7 +123,7 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Starter Kit Premier board rev %c.%c",
 			 rev_major, rev_minor);
-		return;
+		break;
 	case BOARD_EAGLE:
 		if (!board_rev) { /* Only rev 0 is valid */
 			rev_major = '1';
@@ -110,7 +132,7 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Eagle board rev %c.%c",
 			 rev_major, rev_minor);
-		return;
+		break;
 	case BOARD_EBISU_4D:
 		ebisu_4d = true;
 		fallthrough;
@@ -122,7 +144,7 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Ebisu%s board rev %c.%c",
 			 ebisu_4d ? "-4D" : "", rev_major, rev_minor);
-		return;
+		break;
 	case BOARD_DRAAK:
 		if (!board_rev) { /* Only rev 0 is valid */
 			rev_major = '1';
@@ -131,7 +153,7 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Draak board rev %c.%c",
 			 rev_major, rev_minor);
-		return;
+		break;
 	case BOARD_KRIEK:
 		if (!board_rev) { /* Only rev 0 is valid */
 			rev_major = '1';
@@ -140,7 +162,7 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Kriek board rev %c.%c",
 			 rev_major, rev_minor);
-		return;
+		break;
 	case BOARD_CONDOR_I:
 		condor_i = true;
 		fallthrough;
@@ -152,13 +174,17 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			"Renesas Condor%s board rev %c.%c",
 			condor_i ? "-I" : "", rev_major, rev_minor);
-		return;
+		break;
 	default:
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas -Unknown- board rev ?.?");
 		priv->val = 0xff;
 		return;
 	}
+
+	snprintf(priv->revision, sizeof(priv->revision), "%c.%c", rev_major,
+		 rev_minor);
+	priv->id = board_id;
 }
 
 static int sysinfo_rcar_probe(struct udevice *dev)
-- 
2.39.3

[PATCH v1 4/4] configs: rcar3: Add shell function to select the linux devicetree

[Detlev Casanova]

This function uses the sysinfo command to determine which linux device
tree is selected for the running board.

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 configs/rcar3_ulcb_defconfig       | 1 +
 include/configs/rcar-gen3-common.h | 9 ++++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/configs/rcar3_ulcb_defconfig b/configs/rcar3_ulcb_defconfig
index b8fdb5e3826..bb5f8f742ea 100644
--- a/configs/rcar3_ulcb_defconfig
+++ b/configs/rcar3_ulcb_defconfig
@@ -47,6 +47,7 @@ CONFIG_CMD_EXT4=y
 CONFIG_CMD_EXT4_WRITE=y
 CONFIG_CMD_FAT=y
 CONFIG_CMD_FS_GENERIC=y
+CONFIG_CMD_SYSINFO=y
 CONFIG_OF_CONTROL=y
 CONFIG_OF_LIST="r8a77950-ulcb-u-boot r8a77960-ulcb-u-boot r8a77965-ulcb-u-boot"
 CONFIG_MULTI_DTB_FIT_LZO=y
diff --git a/include/configs/rcar-gen3-common.h b/include/configs/rcar-gen3-common.h
index 213caa75238..b278eb85a81 100644
--- a/include/configs/rcar-gen3-common.h
+++ b/include/configs/rcar-gen3-common.h
@@ -33,6 +33,13 @@
 /* ENV setting */
 
 #define CFG_EXTRA_ENV_SETTINGS	\
-	"bootm_size=0x10000000\0"
+	"bootm_size=0x10000000\0"					\
+	"set_board_fdt=sysinfo revision board_rev; "			\
+	"sysinfo id board_id; "						\
+	"if test ${board_rev} = 2.1 && test ${board_id} = 0x0b; then "	\
+		"setenv fdtfile renesas/r8a779m1-ulcb.dtb; "		\
+	"elif test ${board_rev} = 2.0 && test ${board_id} = 0x0b; then "\
+		"setenv fdtfile renesas/r8a77951-ulcb.dtb; "		\
+	"fi\0"								\
 
 #endif	/* __RCAR_GEN3_COMMON_H */
-- 
2.39.3

Re: [PATCH v1 2/4] cmd: Add a sysinfo command

[Marek Vasut]

On 6/16/23 17:21, Detlev Casanova wrote:

[...]

> +static int do_sysinfo_id(struct cmd_tbl *cmdtp, int flag, int argc,
> +			 char *const argv[])
> +{
> +	struct udevice *dev;
> +	u32 board_id;
> +	char board_id_str[5] = { '\0' };
> +	int ret = get_sysinfo(&dev);
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = sysinfo_get_int(dev,
> +			      SYSINFO_ID_BOARD_ID,
> +			      &board_id);
> +
> +	if (ret) {
> +		debug("Cannot get sysinfo int: %d\n", ret);
> +		return ret;
> +	}
> +
> +	sprintf(board_id_str, "0x%02x", board_id);
> +	if (argc == 2)
> +		env_set(argv[1], board_id_str);

env_set_hex()

> +	else
> +		printf("%s\n", board_id_str);

printf(...%02x...

> +	return 0;
> +}
> +
> +static int do_sysinfo_revision(struct cmd_tbl *cmdtp, int flag, int argc,
> +			       char *const argv[])
> +{
> +	struct udevice *dev;
> +	char rev[4];
> +	int ret = get_sysinfo(&dev);
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = sysinfo_get_str(dev,
> +			      SYSINFO_ID_BOARD_REVISION,
> +			      4,
> +			      rev);
> +
> +	if (ret) {
> +		debug("Cannot get sysinfo str: %d\n", ret);
> +		return ret;

Commands always return CMD_RET_* , not errno .

Re: [PATCH v1 3/4] sysinfo: rcar3: Implement BOARD_ID and BOARD_REVISION

[Marek Vasut]

On 6/16/23 17:21, Detlev Casanova wrote:
> Expose that information to the command shell to let scripts select the
> correct devicetree name.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>   drivers/sysinfo/rcar3.c | 46 ++++++++++++++++++++++++++++++++---------
>   1 file changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
> index 7b127986da7..89ad46c5422 100644
> --- a/drivers/sysinfo/rcar3.c
> +++ b/drivers/sysinfo/rcar3.c
> @@ -32,6 +32,8 @@
>    */
>   struct sysinfo_rcar_priv {
>   	char	boardmodel[64];
> +	u8	id;
> +	char	revision[4];
>   	u8	val;
>   };
>   
> @@ -48,17 +50,37 @@ static int sysinfo_rcar_get_str(struct udevice *dev, int id, size_t size, char *
>   
>   	switch (id) {
>   	case SYSINFO_ID_BOARD_MODEL:
> -		strncpy(val, priv->boardmodel, size);
> -		val[size - 1] = '\0';
> +		strlcpy(val, priv->boardmodel, size);
> +		break;
> +	case SYSINFO_ID_BOARD_REVISION:
> +		strlcpy(val, priv->revision, size);
> +		break;
> +	default:
> +		return -EINVAL;
> +	};
> +
> +	val[size - 1] = '\0';
> +	return 0;
> +}
> +
> +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;

Why not return SYSINFO_ID_BOARD_REVISION as integer here ?

[...]

Re: [PATCH v1 4/4] configs: rcar3: Add shell function to select the linux devicetree

[Marek Vasut]

On 6/16/23 17:21, Detlev Casanova wrote:
> This function uses the sysinfo command to determine which linux device
> tree is selected for the running board.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>   configs/rcar3_ulcb_defconfig       | 1 +
>   include/configs/rcar-gen3-common.h | 9 ++++++++-
>   2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/configs/rcar3_ulcb_defconfig b/configs/rcar3_ulcb_defconfig
> index b8fdb5e3826..bb5f8f742ea 100644
> --- a/configs/rcar3_ulcb_defconfig
> +++ b/configs/rcar3_ulcb_defconfig
> @@ -47,6 +47,7 @@ CONFIG_CMD_EXT4=y
>   CONFIG_CMD_EXT4_WRITE=y
>   CONFIG_CMD_FAT=y
>   CONFIG_CMD_FS_GENERIC=y
> +CONFIG_CMD_SYSINFO=y
>   CONFIG_OF_CONTROL=y
>   CONFIG_OF_LIST="r8a77950-ulcb-u-boot r8a77960-ulcb-u-boot r8a77965-ulcb-u-boot"
>   CONFIG_MULTI_DTB_FIT_LZO=y
> diff --git a/include/configs/rcar-gen3-common.h b/include/configs/rcar-gen3-common.h
> index 213caa75238..b278eb85a81 100644
> --- a/include/configs/rcar-gen3-common.h
> +++ b/include/configs/rcar-gen3-common.h
> @@ -33,6 +33,13 @@
>   /* ENV setting */
>   
>   #define CFG_EXTRA_ENV_SETTINGS	\
> -	"bootm_size=0x10000000\0"
> +	"bootm_size=0x10000000\0"					\
> +	"set_board_fdt=sysinfo revision board_rev; "			\
> +	"sysinfo id board_id; "						\
> +	"if test ${board_rev} = 2.1 && test ${board_id} = 0x0b; then "	\
> +		"setenv fdtfile renesas/r8a779m1-ulcb.dtb; "		\
> +	"elif test ${board_rev} = 2.0 && test ${board_id} = 0x0b; then "\
> +		"setenv fdtfile renesas/r8a77951-ulcb.dtb; "		\
> +	"fi\0"								\

You must also consider M3W and M3N ULCB, i.e. 77960/77965 ones.

You must also consider all the other boards which use this header file, 
salvator-xs comes to mind. That one also comes with M3W/M3N SoC. Also, 
all the other Gen3 boards use this header, E3 Ebise, V3* Condor/Eagle ...

Re: [PATCH v1 3/4] sysinfo: rcar3: Implement BOARD_ID and BOARD_REVISION

[Detlev Casanova]

On Friday, June 16, 2023 8:43:33 P.M. EDT Marek Vasut wrote:
> On 6/16/23 17:21, Detlev Casanova wrote:
> > Expose that information to the command shell to let scripts select the
> > correct devicetree name.
> > 
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >   drivers/sysinfo/rcar3.c | 46 ++++++++++++++++++++++++++++++++---------
> >   1 file changed, 36 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
> > index 7b127986da7..89ad46c5422 100644
> > --- a/drivers/sysinfo/rcar3.c
> > +++ b/drivers/sysinfo/rcar3.c
> > @@ -32,6 +32,8 @@
> > 
> >    */
> >   
> >   struct sysinfo_rcar_priv {
> >   
> >   	char	boardmodel[64];
> > 
> > +	u8	id;
> > +	char	revision[4];
> > 
> >   	u8	val;
> >   
> >   };
> > 
> > @@ -48,17 +50,37 @@ static int sysinfo_rcar_get_str(struct udevice *dev,
> > int id, size_t size, char *> 
> >   	switch (id) {
> > 
> >   	case SYSINFO_ID_BOARD_MODEL:
> > -		strncpy(val, priv->boardmodel, size);
> > -		val[size - 1] = '\0';
> > +		strlcpy(val, priv->boardmodel, size);
> > +		break;
> > +	case SYSINFO_ID_BOARD_REVISION:
> > +		strlcpy(val, priv->revision, size);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	};
> > +
> > +	val[size - 1] = '\0';
> > +	return 0;
> > +}
> > +
> > +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;
> 
> Why not return SYSINFO_ID_BOARD_REVISION as integer here ?

Because the revision (on r-car3 boards at least) is in the format X.Y. It 
could be returned as "(X << 8) | Y" or split in major/minor. But different 
boards will use different revisions and I think that having a str is easier to 
deal with in a shell script.

> [...]

Re: [PATCH v1 4/4] configs: rcar3: Add shell function to select the linux devicetree

[Detlev Casanova]

On Friday, June 16, 2023 8:45:31 P.M. EDT Marek Vasut wrote:
> On 6/16/23 17:21, Detlev Casanova wrote:
> > This function uses the sysinfo command to determine which linux device
> > tree is selected for the running board.
> > 
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >   configs/rcar3_ulcb_defconfig       | 1 +
> >   include/configs/rcar-gen3-common.h | 9 ++++++++-
> >   2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/configs/rcar3_ulcb_defconfig b/configs/rcar3_ulcb_defconfig
> > index b8fdb5e3826..bb5f8f742ea 100644
> > --- a/configs/rcar3_ulcb_defconfig
> > +++ b/configs/rcar3_ulcb_defconfig
> > @@ -47,6 +47,7 @@ CONFIG_CMD_EXT4=y
> > 
> >   CONFIG_CMD_EXT4_WRITE=y
> >   CONFIG_CMD_FAT=y
> >   CONFIG_CMD_FS_GENERIC=y
> > 
> > +CONFIG_CMD_SYSINFO=y
> > 
> >   CONFIG_OF_CONTROL=y
> >   CONFIG_OF_LIST="r8a77950-ulcb-u-boot r8a77960-ulcb-u-boot
> >   r8a77965-ulcb-u-boot" CONFIG_MULTI_DTB_FIT_LZO=y
> > 
> > diff --git a/include/configs/rcar-gen3-common.h
> > b/include/configs/rcar-gen3-common.h index 213caa75238..b278eb85a81
> > 100644
> > --- a/include/configs/rcar-gen3-common.h
> > +++ b/include/configs/rcar-gen3-common.h
> > @@ -33,6 +33,13 @@
> > 
> >   /* ENV setting */
> >   
> >   #define CFG_EXTRA_ENV_SETTINGS	\
> > 
> > -	"bootm_size=0x10000000\0"
> > +	"bootm_size=0x10000000\0"				
	\
> > +	"set_board_fdt=sysinfo revision board_rev; "			
\
> > +	"sysinfo id board_id; "					
	\
> > +	"if test ${board_rev} = 2.1 && test ${board_id} = 0x0b; then "	
\
> > +		"setenv fdtfile renesas/r8a779m1-ulcb.dtb; "		
\
> > +	"elif test ${board_rev} = 2.0 && test ${board_id} = 0x0b; then "\
> > +		"setenv fdtfile renesas/r8a77951-ulcb.dtb; "		
\
> > +	"fi\0"						
		\
> 
> You must also consider M3W and M3N ULCB, i.e. 77960/77965 ones.
> 
> You must also consider all the other boards which use this header file,
> salvator-xs comes to mind. That one also comes with M3W/M3N SoC. Also,
> all the other Gen3 boards use this header, E3 Ebise, V3* Condor/Eagle ...

Yeah, that's why I'm not sure this function should be here. I don't have the 
boards to check the revision values.

Re: [PATCH v1 3/4] sysinfo: rcar3: Implement BOARD_ID and BOARD_REVISION

[Marek Vasut]

On 6/19/23 16:42, Detlev Casanova wrote:
> On Friday, June 16, 2023 8:43:33 P.M. EDT Marek Vasut wrote:
>> On 6/16/23 17:21, Detlev Casanova wrote:
>>> Expose that information to the command shell to let scripts select the
>>> correct devicetree name.
>>>
>>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>>> ---
>>>
>>>    drivers/sysinfo/rcar3.c | 46 ++++++++++++++++++++++++++++++++---------
>>>    1 file changed, 36 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
>>> index 7b127986da7..89ad46c5422 100644
>>> --- a/drivers/sysinfo/rcar3.c
>>> +++ b/drivers/sysinfo/rcar3.c
>>> @@ -32,6 +32,8 @@
>>>
>>>     */
>>>    
>>>    struct sysinfo_rcar_priv {
>>>    
>>>    	char	boardmodel[64];
>>>
>>> +	u8	id;
>>> +	char	revision[4];
>>>
>>>    	u8	val;
>>>    
>>>    };
>>>
>>> @@ -48,17 +50,37 @@ static int sysinfo_rcar_get_str(struct udevice *dev,
>>> int id, size_t size, char *>
>>>    	switch (id) {
>>>
>>>    	case SYSINFO_ID_BOARD_MODEL:
>>> -		strncpy(val, priv->boardmodel, size);
>>> -		val[size - 1] = '\0';
>>> +		strlcpy(val, priv->boardmodel, size);
>>> +		break;
>>> +	case SYSINFO_ID_BOARD_REVISION:
>>> +		strlcpy(val, priv->revision, size);
>>> +		break;
>>> +	default:
>>> +		return -EINVAL;
>>> +	};
>>> +
>>> +	val[size - 1] = '\0';
>>> +	return 0;
>>> +}
>>> +
>>> +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;
>>
>> Why not return SYSINFO_ID_BOARD_REVISION as integer here ?
> 
> Because the revision (on r-car3 boards at least) is in the format X.Y. It
> could be returned as "(X << 8) | Y" or split in major/minor. But different
> boards will use different revisions and I think that having a str is easier to
> deal with in a shell script.

With rcar they are numbers, so lets go with major/minor integers please.

Re: [PATCH v1 3/4] sysinfo: rcar3: Implement BOARD_ID and BOARD_REVISION

[Detlev Casanova]

On Monday, June 19, 2023 12:11:18 P.M. EDT Marek Vasut wrote:
> On 6/19/23 16:42, Detlev Casanova wrote:
> > On Friday, June 16, 2023 8:43:33 P.M. EDT Marek Vasut wrote:
> >> On 6/16/23 17:21, Detlev Casanova wrote:
> >>> Expose that information to the command shell to let scripts select the
> >>> correct devicetree name.
> >>> 
> >>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> >>> ---
> >>> 
> >>>    drivers/sysinfo/rcar3.c | 46
> >>>    ++++++++++++++++++++++++++++++++---------
> >>>    1 file changed, 36 insertions(+), 10 deletions(-)
> >>> 
> >>> diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
> >>> index 7b127986da7..89ad46c5422 100644
> >>> --- a/drivers/sysinfo/rcar3.c
> >>> +++ b/drivers/sysinfo/rcar3.c
> >>> @@ -32,6 +32,8 @@
> >>> 
> >>>     */
> >>>    
> >>>    struct sysinfo_rcar_priv {
> >>>    
> >>>    	char	boardmodel[64];
> >>> 
> >>> +	u8	id;
> >>> +	char	revision[4];
> >>> 
> >>>    	u8	val;
> >>>    
> >>>    };
> >>> 
> >>> @@ -48,17 +50,37 @@ static int sysinfo_rcar_get_str(struct udevice *dev,
> >>> int id, size_t size, char *>
> >>> 
> >>>    	switch (id) {
> >>> 
> >>>    	case SYSINFO_ID_BOARD_MODEL:
> >>> -		strncpy(val, priv->boardmodel, size);
> >>> -		val[size - 1] = '\0';
> >>> +		strlcpy(val, priv->boardmodel, size);
> >>> +		break;
> >>> +	case SYSINFO_ID_BOARD_REVISION:
> >>> +		strlcpy(val, priv->revision, size);
> >>> +		break;
> >>> +	default:
> >>> +		return -EINVAL;
> >>> +	};
> >>> +
> >>> +	val[size - 1] = '\0';
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +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;
> >> 
> >> Why not return SYSINFO_ID_BOARD_REVISION as integer here ?
> > 
> > Because the revision (on r-car3 boards at least) is in the format X.Y. It
> > could be returned as "(X << 8) | Y" or split in major/minor. But different
> > boards will use different revisions and I think that having a str is
> > easier to deal with in a shell script.
> 
> With rcar they are numbers, so lets go with major/minor integers please.

Ok for this part, but shouldn't the sysinfo command use a common interface for 
all boards ? Or should it also have rev_major/rev_minor arguments ?

Re: [PATCH v1 3/4] sysinfo: rcar3: Implement BOARD_ID and BOARD_REVISION

[Marek Vasut]

On 6/19/23 20:27, Detlev Casanova wrote:
> On Monday, June 19, 2023 12:11:18 P.M. EDT Marek Vasut wrote:
>> On 6/19/23 16:42, Detlev Casanova wrote:
>>> On Friday, June 16, 2023 8:43:33 P.M. EDT Marek Vasut wrote:
>>>> On 6/16/23 17:21, Detlev Casanova wrote:
>>>>> Expose that information to the command shell to let scripts select the
>>>>> correct devicetree name.
>>>>>
>>>>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>>>>> ---
>>>>>
>>>>>     drivers/sysinfo/rcar3.c | 46
>>>>>     ++++++++++++++++++++++++++++++++---------
>>>>>     1 file changed, 36 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
>>>>> index 7b127986da7..89ad46c5422 100644
>>>>> --- a/drivers/sysinfo/rcar3.c
>>>>> +++ b/drivers/sysinfo/rcar3.c
>>>>> @@ -32,6 +32,8 @@
>>>>>
>>>>>      */
>>>>>     
>>>>>     struct sysinfo_rcar_priv {
>>>>>     
>>>>>     	char	boardmodel[64];
>>>>>
>>>>> +	u8	id;
>>>>> +	char	revision[4];
>>>>>
>>>>>     	u8	val;
>>>>>     
>>>>>     };
>>>>>
>>>>> @@ -48,17 +50,37 @@ static int sysinfo_rcar_get_str(struct udevice *dev,
>>>>> int id, size_t size, char *>
>>>>>
>>>>>     	switch (id) {
>>>>>
>>>>>     	case SYSINFO_ID_BOARD_MODEL:
>>>>> -		strncpy(val, priv->boardmodel, size);
>>>>> -		val[size - 1] = '\0';
>>>>> +		strlcpy(val, priv->boardmodel, size);
>>>>> +		break;
>>>>> +	case SYSINFO_ID_BOARD_REVISION:
>>>>> +		strlcpy(val, priv->revision, size);
>>>>> +		break;
>>>>> +	default:
>>>>> +		return -EINVAL;
>>>>> +	};
>>>>> +
>>>>> +	val[size - 1] = '\0';
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +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;
>>>>
>>>> Why not return SYSINFO_ID_BOARD_REVISION as integer here ?
>>>
>>> Because the revision (on r-car3 boards at least) is in the format X.Y. It
>>> could be returned as "(X << 8) | Y" or split in major/minor. But different
>>> boards will use different revisions and I think that having a str is
>>> easier to deal with in a shell script.
>>
>> With rcar they are numbers, so lets go with major/minor integers please.
> 
> Ok for this part, but shouldn't the sysinfo command use a common interface for
> all boards ? Or should it also have rev_major/rev_minor arguments ?

I would expect other boards to either report rev_major/rev_minor if 
implemented, or errno if those boards don't implement this property.

Re: [PATCH v1 3/4] sysinfo: rcar3: Implement BOARD_ID and BOARD_REVISION

[Detlev Casanova]

On Monday, June 19, 2023 5:54:44 P.M. EDT Marek Vasut wrote:
> On 6/19/23 20:27, Detlev Casanova wrote:
> > On Monday, June 19, 2023 12:11:18 P.M. EDT Marek Vasut wrote:
> >> On 6/19/23 16:42, Detlev Casanova wrote:
> >>> On Friday, June 16, 2023 8:43:33 P.M. EDT Marek Vasut wrote:
> >>>> On 6/16/23 17:21, Detlev Casanova wrote:
> >>>>> Expose that information to the command shell to let scripts select the
> >>>>> correct devicetree name.
> >>>>> 
> >>>>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> >>>>> ---
> >>>>> 
> >>>>>     drivers/sysinfo/rcar3.c | 46
> >>>>>     ++++++++++++++++++++++++++++++++---------
> >>>>>     1 file changed, 36 insertions(+), 10 deletions(-)
> >>>>> 
> >>>>> diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
> >>>>> index 7b127986da7..89ad46c5422 100644
> >>>>> --- a/drivers/sysinfo/rcar3.c
> >>>>> +++ b/drivers/sysinfo/rcar3.c
> >>>>> @@ -32,6 +32,8 @@
> >>>>> 
> >>>>>      */
> >>>>>     
> >>>>>     struct sysinfo_rcar_priv {
> >>>>>     
> >>>>>     	char	boardmodel[64];
> >>>>> 
> >>>>> +	u8	id;
> >>>>> +	char	revision[4];
> >>>>> 
> >>>>>     	u8	val;
> >>>>>     
> >>>>>     };
> >>>>> 
> >>>>> @@ -48,17 +50,37 @@ static int sysinfo_rcar_get_str(struct udevice
> >>>>> *dev,
> >>>>> int id, size_t size, char *>
> >>>>> 
> >>>>>     	switch (id) {
> >>>>> 
> >>>>>     	case SYSINFO_ID_BOARD_MODEL:
> >>>>> -		strncpy(val, priv->boardmodel, size);
> >>>>> -		val[size - 1] = '\0';
> >>>>> +		strlcpy(val, priv->boardmodel, size);
> >>>>> +		break;
> >>>>> +	case SYSINFO_ID_BOARD_REVISION:
> >>>>> +		strlcpy(val, priv->revision, size);
> >>>>> +		break;
> >>>>> +	default:
> >>>>> +		return -EINVAL;
> >>>>> +	};
> >>>>> +
> >>>>> +	val[size - 1] = '\0';
> >>>>> +	return 0;
> >>>>> +}
> >>>>> +
> >>>>> +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;
> >>>> 
> >>>> Why not return SYSINFO_ID_BOARD_REVISION as integer here ?
> >>> 
> >>> Because the revision (on r-car3 boards at least) is in the format X.Y.
> >>> It
> >>> could be returned as "(X << 8) | Y" or split in major/minor. But
> >>> different
> >>> boards will use different revisions and I think that having a str is
> >>> easier to deal with in a shell script.
> >> 
> >> With rcar they are numbers, so lets go with major/minor integers please.
> > 
> > Ok for this part, but shouldn't the sysinfo command use a common interface
> > for all boards ? Or should it also have rev_major/rev_minor arguments ?
> I would expect other boards to either report rev_major/rev_minor if
> implemented, or errno if those boards don't implement this property.

Another thing on rcar is that the revision is stored as 2 char values. Would 
you oppose a change form using a char (e.g. rev_major = '1') to using u8 
values (e.g. rev_major = 1) instead ?

Re: [PATCH v1 3/4] sysinfo: rcar3: Implement BOARD_ID and BOARD_REVISION

[Marek Vasut]

On 6/20/23 19:49, Detlev Casanova wrote:
> On Monday, June 19, 2023 5:54:44 P.M. EDT Marek Vasut wrote:
>> On 6/19/23 20:27, Detlev Casanova wrote:
>>> On Monday, June 19, 2023 12:11:18 P.M. EDT Marek Vasut wrote:
>>>> On 6/19/23 16:42, Detlev Casanova wrote:
>>>>> On Friday, June 16, 2023 8:43:33 P.M. EDT Marek Vasut wrote:
>>>>>> On 6/16/23 17:21, Detlev Casanova wrote:
>>>>>>> Expose that information to the command shell to let scripts select the
>>>>>>> correct devicetree name.
>>>>>>>
>>>>>>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>>>>>>> ---
>>>>>>>
>>>>>>>      drivers/sysinfo/rcar3.c | 46
>>>>>>>      ++++++++++++++++++++++++++++++++---------
>>>>>>>      1 file changed, 36 insertions(+), 10 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
>>>>>>> index 7b127986da7..89ad46c5422 100644
>>>>>>> --- a/drivers/sysinfo/rcar3.c
>>>>>>> +++ b/drivers/sysinfo/rcar3.c
>>>>>>> @@ -32,6 +32,8 @@
>>>>>>>
>>>>>>>       */
>>>>>>>      
>>>>>>>      struct sysinfo_rcar_priv {
>>>>>>>      
>>>>>>>      	char	boardmodel[64];
>>>>>>>
>>>>>>> +	u8	id;
>>>>>>> +	char	revision[4];
>>>>>>>
>>>>>>>      	u8	val;
>>>>>>>      
>>>>>>>      };
>>>>>>>
>>>>>>> @@ -48,17 +50,37 @@ static int sysinfo_rcar_get_str(struct udevice
>>>>>>> *dev,
>>>>>>> int id, size_t size, char *>
>>>>>>>
>>>>>>>      	switch (id) {
>>>>>>>
>>>>>>>      	case SYSINFO_ID_BOARD_MODEL:
>>>>>>> -		strncpy(val, priv->boardmodel, size);
>>>>>>> -		val[size - 1] = '\0';
>>>>>>> +		strlcpy(val, priv->boardmodel, size);
>>>>>>> +		break;
>>>>>>> +	case SYSINFO_ID_BOARD_REVISION:
>>>>>>> +		strlcpy(val, priv->revision, size);
>>>>>>> +		break;
>>>>>>> +	default:
>>>>>>> +		return -EINVAL;
>>>>>>> +	};
>>>>>>> +
>>>>>>> +	val[size - 1] = '\0';
>>>>>>> +	return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +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;
>>>>>>
>>>>>> Why not return SYSINFO_ID_BOARD_REVISION as integer here ?
>>>>>
>>>>> Because the revision (on r-car3 boards at least) is in the format X.Y.
>>>>> It
>>>>> could be returned as "(X << 8) | Y" or split in major/minor. But
>>>>> different
>>>>> boards will use different revisions and I think that having a str is
>>>>> easier to deal with in a shell script.
>>>>
>>>> With rcar they are numbers, so lets go with major/minor integers please.
>>>
>>> Ok for this part, but shouldn't the sysinfo command use a common interface
>>> for all boards ? Or should it also have rev_major/rev_minor arguments ?
>> I would expect other boards to either report rev_major/rev_minor if
>> implemented, or errno if those boards don't implement this property.
> 
> Another thing on rcar is that the revision is stored as 2 char values. Would
> you oppose a change form using a char (e.g. rev_major = '1') to using u8
> values (e.g. rev_major = 1) instead ?

Shouldn't those rev fields just be integer(s) to cover the generic case?

Re: [PATCH v1 3/4] sysinfo: rcar3: Implement BOARD_ID and BOARD_REVISION

[Detlev Casanova]

On Tuesday, June 20, 2023 4:03:18 P.M. EDT Marek Vasut wrote:
> On 6/20/23 19:49, Detlev Casanova wrote:
> > On Monday, June 19, 2023 5:54:44 P.M. EDT Marek Vasut wrote:
> >> On 6/19/23 20:27, Detlev Casanova wrote:
> >>> On Monday, June 19, 2023 12:11:18 P.M. EDT Marek Vasut wrote:
> >>>> On 6/19/23 16:42, Detlev Casanova wrote:
> >>>>> On Friday, June 16, 2023 8:43:33 P.M. EDT Marek Vasut wrote:
> >>>>>> On 6/16/23 17:21, Detlev Casanova wrote:
> >>>>>>> Expose that information to the command shell to let scripts select
> >>>>>>> the
> >>>>>>> correct devicetree name.
> >>>>>>> 
> >>>>>>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> >>>>>>> ---
> >>>>>>> 
> >>>>>>>      drivers/sysinfo/rcar3.c | 46
> >>>>>>>      ++++++++++++++++++++++++++++++++---------
> >>>>>>>      1 file changed, 36 insertions(+), 10 deletions(-)
> >>>>>>> 
> >>>>>>> diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
> >>>>>>> index 7b127986da7..89ad46c5422 100644
> >>>>>>> --- a/drivers/sysinfo/rcar3.c
> >>>>>>> +++ b/drivers/sysinfo/rcar3.c
> >>>>>>> @@ -32,6 +32,8 @@
> >>>>>>> 
> >>>>>>>       */
> >>>>>>>      
> >>>>>>>      struct sysinfo_rcar_priv {
> >>>>>>>      
> >>>>>>>      	char	boardmodel[64];
> >>>>>>> 
> >>>>>>> +	u8	id;
> >>>>>>> +	char	revision[4];
> >>>>>>> 
> >>>>>>>      	u8	val;
> >>>>>>>      
> >>>>>>>      };
> >>>>>>> 
> >>>>>>> @@ -48,17 +50,37 @@ static int sysinfo_rcar_get_str(struct udevice
> >>>>>>> *dev,
> >>>>>>> int id, size_t size, char *>
> >>>>>>> 
> >>>>>>>      	switch (id) {
> >>>>>>> 
> >>>>>>>      	case SYSINFO_ID_BOARD_MODEL:
> >>>>>>> -		strncpy(val, priv->boardmodel, size);
> >>>>>>> -		val[size - 1] = '\0';
> >>>>>>> +		strlcpy(val, priv->boardmodel, size);
> >>>>>>> +		break;
> >>>>>>> +	case SYSINFO_ID_BOARD_REVISION:
> >>>>>>> +		strlcpy(val, priv->revision, size);
> >>>>>>> +		break;
> >>>>>>> +	default:
> >>>>>>> +		return -EINVAL;
> >>>>>>> +	};
> >>>>>>> +
> >>>>>>> +	val[size - 1] = '\0';
> >>>>>>> +	return 0;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +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;
> >>>>>> 
> >>>>>> Why not return SYSINFO_ID_BOARD_REVISION as integer here ?
> >>>>> 
> >>>>> Because the revision (on r-car3 boards at least) is in the format X.Y.
> >>>>> It
> >>>>> could be returned as "(X << 8) | Y" or split in major/minor. But
> >>>>> different
> >>>>> boards will use different revisions and I think that having a str is
> >>>>> easier to deal with in a shell script.
> >>>> 
> >>>> With rcar they are numbers, so lets go with major/minor integers
> >>>> please.
> >>> 
> >>> Ok for this part, but shouldn't the sysinfo command use a common
> >>> interface
> >>> for all boards ? Or should it also have rev_major/rev_minor arguments ?
> >> 
> >> I would expect other boards to either report rev_major/rev_minor if
> >> implemented, or errno if those boards don't implement this property.
> > 
> > Another thing on rcar is that the revision is stored as 2 char values.
> > Would you oppose a change form using a char (e.g. rev_major = '1') to
> > using u8 values (e.g. rev_major = 1) instead ?
> 
> Shouldn't those rev fields just be integer(s) to cover the generic case?

On rcar, they are chars. I don't really see a reason for this except to show 
the '?.?' on unknown board ids. But that can be managed in other ways.

Re: [PATCH v1 3/4] sysinfo: rcar3: Implement BOARD_ID and BOARD_REVISION

[Marek Vasut]

On 6/20/23 22:40, Detlev Casanova wrote:
> On Tuesday, June 20, 2023 4:03:18 P.M. EDT Marek Vasut wrote:
>> On 6/20/23 19:49, Detlev Casanova wrote:
>>> On Monday, June 19, 2023 5:54:44 P.M. EDT Marek Vasut wrote:
>>>> On 6/19/23 20:27, Detlev Casanova wrote:
>>>>> On Monday, June 19, 2023 12:11:18 P.M. EDT Marek Vasut wrote:
>>>>>> On 6/19/23 16:42, Detlev Casanova wrote:
>>>>>>> On Friday, June 16, 2023 8:43:33 P.M. EDT Marek Vasut wrote:
>>>>>>>> On 6/16/23 17:21, Detlev Casanova wrote:
>>>>>>>>> Expose that information to the command shell to let scripts select
>>>>>>>>> the
>>>>>>>>> correct devicetree name.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>       drivers/sysinfo/rcar3.c | 46
>>>>>>>>>       ++++++++++++++++++++++++++++++++---------
>>>>>>>>>       1 file changed, 36 insertions(+), 10 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
>>>>>>>>> index 7b127986da7..89ad46c5422 100644
>>>>>>>>> --- a/drivers/sysinfo/rcar3.c
>>>>>>>>> +++ b/drivers/sysinfo/rcar3.c
>>>>>>>>> @@ -32,6 +32,8 @@
>>>>>>>>>
>>>>>>>>>        */
>>>>>>>>>       
>>>>>>>>>       struct sysinfo_rcar_priv {
>>>>>>>>>       
>>>>>>>>>       	char	boardmodel[64];
>>>>>>>>>
>>>>>>>>> +	u8	id;
>>>>>>>>> +	char	revision[4];
>>>>>>>>>
>>>>>>>>>       	u8	val;
>>>>>>>>>       
>>>>>>>>>       };
>>>>>>>>>
>>>>>>>>> @@ -48,17 +50,37 @@ static int sysinfo_rcar_get_str(struct udevice
>>>>>>>>> *dev,
>>>>>>>>> int id, size_t size, char *>
>>>>>>>>>
>>>>>>>>>       	switch (id) {
>>>>>>>>>
>>>>>>>>>       	case SYSINFO_ID_BOARD_MODEL:
>>>>>>>>> -		strncpy(val, priv->boardmodel, size);
>>>>>>>>> -		val[size - 1] = '\0';
>>>>>>>>> +		strlcpy(val, priv->boardmodel, size);
>>>>>>>>> +		break;
>>>>>>>>> +	case SYSINFO_ID_BOARD_REVISION:
>>>>>>>>> +		strlcpy(val, priv->revision, size);
>>>>>>>>> +		break;
>>>>>>>>> +	default:
>>>>>>>>> +		return -EINVAL;
>>>>>>>>> +	};
>>>>>>>>> +
>>>>>>>>> +	val[size - 1] = '\0';
>>>>>>>>> +	return 0;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +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;
>>>>>>>>
>>>>>>>> Why not return SYSINFO_ID_BOARD_REVISION as integer here ?
>>>>>>>
>>>>>>> Because the revision (on r-car3 boards at least) is in the format X.Y.
>>>>>>> It
>>>>>>> could be returned as "(X << 8) | Y" or split in major/minor. But
>>>>>>> different
>>>>>>> boards will use different revisions and I think that having a str is
>>>>>>> easier to deal with in a shell script.
>>>>>>
>>>>>> With rcar they are numbers, so lets go with major/minor integers
>>>>>> please.
>>>>>
>>>>> Ok for this part, but shouldn't the sysinfo command use a common
>>>>> interface
>>>>> for all boards ? Or should it also have rev_major/rev_minor arguments ?
>>>>
>>>> I would expect other boards to either report rev_major/rev_minor if
>>>> implemented, or errno if those boards don't implement this property.
>>>
>>> Another thing on rcar is that the revision is stored as 2 char values.
>>> Would you oppose a change form using a char (e.g. rev_major = '1') to
>>> using u8 values (e.g. rev_major = 1) instead ?
>>
>> Shouldn't those rev fields just be integer(s) to cover the generic case?
> 
> On rcar, they are chars. I don't really see a reason for this except to show
> the '?.?' on unknown board ids. But that can be managed in other ways.

Yes I know, I am more concerned about other boards which might not have 
such short revision number, so why not just make the revision fields 
integer which covers very much anything ? Also, if you have those fields 
set to integer, then -EINVAL could be printed as '?' .