[PATCH v2 0/3] rcar: Select the correct device tree

[PATCH v2 0/3] rcar: Select the correct device tree

[Detlev Casanova]

The R-Car Starter Kit Premier(H3e WS3.0) uses the r8a779m1-ulcb.dtb.
As u-boot can detect the board id and revision, let's use this to set
the correct device tree for that board when using pxe.

Changes since v1:
 * Also expose the board revision in sysinfo
 * Set the dtb depending on board revision
 * Move RCar board IDs definitions to sys_proto.h
 * fix checkpatch.pl warnings

Detlev Casanova (3):
  renesas: rcar3: Expose the board id in sysinfo
  renesas: rcar3: Expose the board revision in sysinfo
  renesas: rcar3: Load the correct device tree

 .../arm/mach-rmobile/include/mach/sys_proto.h | 21 +++++
 board/renesas/ulcb/ulcb.c                     | 59 ++++++++++++
 configs/rcar3_ulcb_defconfig                  |  1 +
 drivers/sysinfo/rcar3.c                       | 90 +++++++++++--------
 4 files changed, 133 insertions(+), 38 deletions(-)

-- 
2.39.3

[PATCH v2 1/3] renesas: rcar3: Expose the board id in sysinfo

[Detlev Casanova]

This is a preparation commit for selecting the correct device tree name
to be loaded depending on the board id.

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 .../arm/mach-rmobile/include/mach/sys_proto.h | 15 ++++++++++
 drivers/sysinfo/rcar3.c                       | 30 +++++++++++--------
 2 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-rmobile/include/mach/sys_proto.h b/arch/arm/mach-rmobile/include/mach/sys_proto.h
index ec8036a354c..e020b24f7c2 100644
--- a/arch/arm/mach-rmobile/include/mach/sys_proto.h
+++ b/arch/arm/mach-rmobile/include/mach/sys_proto.h
@@ -7,4 +7,19 @@
 #ifndef _SYS_PROTO_H_
 #define _SYS_PROTO_H_
 
+/*
+ * Renesas R-Car Board IDs
+ */
+#define BOARD_SALVATOR_X	0x0
+#define BOARD_KRIEK		0x1
+#define BOARD_STARTER_KIT	0x2
+#define BOARD_EAGLE		0x3
+#define BOARD_SALVATOR_XS	0x4
+#define BOARD_CONDOR		0x6
+#define BOARD_DRAAK		0x7
+#define BOARD_EBISU		0x8
+#define BOARD_STARTER_KIT_PRE	0xB
+#define BOARD_EBISU_4D		0xD
+#define BOARD_CONDOR_I		0x10
+
 #endif
diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
index 7b127986da7..b8b837341a2 100644
--- a/drivers/sysinfo/rcar3.c
+++ b/drivers/sysinfo/rcar3.c
@@ -8,23 +8,12 @@
 #include <i2c_eeprom.h>
 #include <log.h>
 #include <sysinfo.h>
+#include <asm/arch/sys_proto.h>
 
 #define BOARD_CODE_MASK		0xF8
 #define BOARD_REV_MASK		0x07
 #define BOARD_CODE_SHIFT	0x03
 
-#define BOARD_SALVATOR_X	0x0
-#define BOARD_KRIEK		0x1
-#define BOARD_STARTER_KIT	0x2
-#define BOARD_EAGLE		0x3
-#define BOARD_SALVATOR_XS	0x4
-#define BOARD_CONDOR		0x6
-#define BOARD_DRAAK		0x7
-#define BOARD_EBISU		0x8
-#define BOARD_STARTER_KIT_PRE	0xB
-#define BOARD_EBISU_4D		0xD
-#define BOARD_CONDOR_I		0x10
-
 /**
  * struct sysinfo_rcar_priv - sysinfo private data
  * @boardname: board model and revision
@@ -32,6 +21,7 @@
  */
 struct sysinfo_rcar_priv {
 	char	boardmodel[64];
+	u8	board_id;
 	u8	val;
 };
 
@@ -42,6 +32,19 @@ static int sysinfo_rcar_detect(struct udevice *dev)
 	return priv->val == 0xff;
 }
 
+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_MODEL:
+		*val = priv->board_id;
+		return 0;
+	default:
+		return -EINVAL;
+	};
+}
+
 static int sysinfo_rcar_get_str(struct udevice *dev, int id, size_t size, char *val)
 {
 	struct sysinfo_rcar_priv *priv = dev_get_priv(dev);
@@ -59,6 +62,7 @@ static int sysinfo_rcar_get_str(struct udevice *dev, int id, size_t size, char *
 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)
@@ -68,6 +72,8 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 	bool salvator_xs = false;
 	bool ebisu_4d = false;
 	bool condor_i = false;
+
+	priv->board_id = board_id;
 	char rev_major = '?';
 	char rev_minor = '?';
 
-- 
2.39.3

Re: [PATCH v2 1/3] renesas: rcar3: Expose the board id in sysinfo

[Marek Vasut]

On 6/12/23 21:51, Detlev Casanova wrote:
> This is a preparation commit for selecting the correct device tree name
> to be loaded depending on the board id.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>   .../arm/mach-rmobile/include/mach/sys_proto.h | 15 ++++++++++
>   drivers/sysinfo/rcar3.c                       | 30 +++++++++++--------
>   2 files changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/mach-rmobile/include/mach/sys_proto.h b/arch/arm/mach-rmobile/include/mach/sys_proto.h
> index ec8036a354c..e020b24f7c2 100644
> --- a/arch/arm/mach-rmobile/include/mach/sys_proto.h
> +++ b/arch/arm/mach-rmobile/include/mach/sys_proto.h
> @@ -7,4 +7,19 @@
>   #ifndef _SYS_PROTO_H_
>   #define _SYS_PROTO_H_
>   
> +/*
> + * Renesas R-Car Board IDs
> + */
> +#define BOARD_SALVATOR_X	0x0
> +#define BOARD_KRIEK		0x1
> +#define BOARD_STARTER_KIT	0x2
> +#define BOARD_EAGLE		0x3
> +#define BOARD_SALVATOR_XS	0x4
> +#define BOARD_CONDOR		0x6
> +#define BOARD_DRAAK		0x7
> +#define BOARD_EBISU		0x8
> +#define BOARD_STARTER_KIT_PRE	0xB
> +#define BOARD_EBISU_4D		0xD
> +#define BOARD_CONDOR_I		0x10
> +
>   #endif
> diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
> index 7b127986da7..b8b837341a2 100644
> --- a/drivers/sysinfo/rcar3.c
> +++ b/drivers/sysinfo/rcar3.c
> @@ -8,23 +8,12 @@
>   #include <i2c_eeprom.h>
>   #include <log.h>
>   #include <sysinfo.h>
> +#include <asm/arch/sys_proto.h>
>   
>   #define BOARD_CODE_MASK		0xF8
>   #define BOARD_REV_MASK		0x07
>   #define BOARD_CODE_SHIFT	0x03
>   
> -#define BOARD_SALVATOR_X	0x0
> -#define BOARD_KRIEK		0x1
> -#define BOARD_STARTER_KIT	0x2
> -#define BOARD_EAGLE		0x3
> -#define BOARD_SALVATOR_XS	0x4
> -#define BOARD_CONDOR		0x6
> -#define BOARD_DRAAK		0x7
> -#define BOARD_EBISU		0x8
> -#define BOARD_STARTER_KIT_PRE	0xB
> -#define BOARD_EBISU_4D		0xD
> -#define BOARD_CONDOR_I		0x10
> -

This part ^ should not be in this patch .
All this patch should do is expose the board ID .
Please split that into separate patch.

>   /**
>    * struct sysinfo_rcar_priv - sysinfo private data
>    * @boardname: board model and revision
> @@ -32,6 +21,7 @@
>    */
>   struct sysinfo_rcar_priv {
>   	char	boardmodel[64];
> +	u8	board_id;
>   	u8	val;
>   };
>   
> @@ -42,6 +32,19 @@ static int sysinfo_rcar_detect(struct udevice *dev)
>   	return priv->val == 0xff;
>   }
>   
> +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_MODEL:

The BOARD_MODEL is a string, please add another 'BOARD_ID' into 
include/sysinfo.h to discern the properties.

> +		*val = priv->board_id;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	};
> +}

[...]

[PATCH v2 2/3] renesas: rcar3: Expose the board revision in sysinfo

[Detlev Casanova]

The board revision is needed to determine which linux device tree to
load.

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 .../arm/mach-rmobile/include/mach/sys_proto.h |  6 ++
 drivers/sysinfo/rcar3.c                       | 60 +++++++++++--------
 2 files changed, 40 insertions(+), 26 deletions(-)

diff --git a/arch/arm/mach-rmobile/include/mach/sys_proto.h b/arch/arm/mach-rmobile/include/mach/sys_proto.h
index e020b24f7c2..1f204f06c17 100644
--- a/arch/arm/mach-rmobile/include/mach/sys_proto.h
+++ b/arch/arm/mach-rmobile/include/mach/sys_proto.h
@@ -22,4 +22,10 @@
 #define BOARD_EBISU_4D		0xD
 #define BOARD_CONDOR_I		0x10
 
+/*
+ * Renesas sysinfo board revision
+ */
+#define RCAR_SYSINFO_REV_MAJOR	SYSINFO_ID_USER
+#define RCAR_SYSINFO_REV_MINOR	(SYSINFO_ID_USER + 1)
+
 #endif
diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
index b8b837341a2..3223875a99c 100644
--- a/drivers/sysinfo/rcar3.c
+++ b/drivers/sysinfo/rcar3.c
@@ -22,6 +22,8 @@
 struct sysinfo_rcar_priv {
 	char	boardmodel[64];
 	u8	board_id;
+	u8	rev_major;
+	u8	rev_minor;
 	u8	val;
 };
 
@@ -40,6 +42,12 @@ static int sysinfo_rcar_get_int(struct udevice *dev, int id, int *val)
 	case SYSINFO_ID_BOARD_MODEL:
 		*val = priv->board_id;
 		return 0;
+	case RCAR_SYSINFO_REV_MAJOR:
+		*val = priv->rev_major;
+		return 0;
+	case RCAR_SYSINFO_REV_MINOR:
+		*val = priv->rev_minor;
+		return 0;
 	default:
 		return -EINVAL;
 	};
@@ -74,8 +82,8 @@ static void sysinfo_rcar_parse(struct sysinfo_rcar_priv *priv)
 	bool condor_i = false;
 
 	priv->board_id = board_id;
-	char rev_major = '?';
-	char rev_minor = '?';
+	priv->rev_major = '?';
+	priv->rev_minor = '?';
 
 	switch (board_id) {
 	case BOARD_SALVATOR_XS:
@@ -83,81 +91,81 @@ 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));
+			priv->rev_major = '1';
+			priv->rev_minor = '0' + (board_rev & BIT(0));
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Salvator-X%s board rev %c.%c",
-			 salvator_xs ? "S" : "", rev_major, rev_minor);
+			 salvator_xs ? "S" : "", priv->rev_major, priv->rev_minor);
 		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';
+			priv->rev_major = (board_rev & BIT(0)) ? '3' : '1';
+			priv->rev_minor = '0';
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Starter Kit board rev %c.%c",
-			 rev_major, rev_minor);
+			 priv->rev_major, priv->rev_minor);
 		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';
+			priv->rev_major = (board_rev & BIT(1)) ? '2' : '1';
+			priv->rev_minor = (board_rev == 3) ? '1' : '0';
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Starter Kit Premier board rev %c.%c",
-			 rev_major, rev_minor);
+			 priv->rev_major, priv->rev_minor);
 		return;
 	case BOARD_EAGLE:
 		if (!board_rev) { /* Only rev 0 is valid */
-			rev_major = '1';
-			rev_minor = '0';
+			priv->rev_major = '1';
+			priv->rev_minor = '0';
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Eagle board rev %c.%c",
-			 rev_major, rev_minor);
+			 priv->rev_major, priv->rev_minor);
 		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';
+			priv->rev_major = '1';
+			priv->rev_minor = '0';
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Ebisu%s board rev %c.%c",
-			 ebisu_4d ? "-4D" : "", rev_major, rev_minor);
+			 ebisu_4d ? "-4D" : "", priv->rev_major, priv->rev_minor);
 		return;
 	case BOARD_DRAAK:
 		if (!board_rev) { /* Only rev 0 is valid */
-			rev_major = '1';
-			rev_minor = '0';
+			priv->rev_major = '1';
+			priv->rev_minor = '0';
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Draak board rev %c.%c",
-			 rev_major, rev_minor);
+			 priv->rev_major, priv->rev_minor);
 		return;
 	case BOARD_KRIEK:
 		if (!board_rev) { /* Only rev 0 is valid */
-			rev_major = '1';
-			rev_minor = '0';
+			priv->rev_major = '1';
+			priv->rev_minor = '0';
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			 "Renesas Kriek board rev %c.%c",
-			 rev_major, rev_minor);
+			 priv->rev_major, priv->rev_minor);
 		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';
+			priv->rev_major = '1';
+			priv->rev_minor = '0';
 		}
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
 			"Renesas Condor%s board rev %c.%c",
-			condor_i ? "-I" : "", rev_major, rev_minor);
+			condor_i ? "-I" : "", priv->rev_major, priv->rev_minor);
 		return;
 	default:
 		snprintf(priv->boardmodel, sizeof(priv->boardmodel),
-- 
2.39.3

Re: [PATCH v2 2/3] renesas: rcar3: Expose the board revision in sysinfo

[Marek Vasut]

On 6/12/23 21:51, Detlev Casanova wrote:
> The board revision is needed to determine which linux device tree to
> load.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>   .../arm/mach-rmobile/include/mach/sys_proto.h |  6 ++
>   drivers/sysinfo/rcar3.c                       | 60 +++++++++++--------
>   2 files changed, 40 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm/mach-rmobile/include/mach/sys_proto.h b/arch/arm/mach-rmobile/include/mach/sys_proto.h
> index e020b24f7c2..1f204f06c17 100644
> --- a/arch/arm/mach-rmobile/include/mach/sys_proto.h
> +++ b/arch/arm/mach-rmobile/include/mach/sys_proto.h
> @@ -22,4 +22,10 @@
>   #define BOARD_EBISU_4D		0xD
>   #define BOARD_CONDOR_I		0x10
>   
> +/*
> + * Renesas sysinfo board revision
> + */
> +#define RCAR_SYSINFO_REV_MAJOR	SYSINFO_ID_USER
> +#define RCAR_SYSINFO_REV_MINOR	(SYSINFO_ID_USER + 1)

Please just introduce regular include/sysinfo.h board revision 
properties for this, not custom stuff.

>   #endif
> diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
> index b8b837341a2..3223875a99c 100644
> --- a/drivers/sysinfo/rcar3.c
> +++ b/drivers/sysinfo/rcar3.c
> @@ -22,6 +22,8 @@
>   struct sysinfo_rcar_priv {
>   	char	boardmodel[64];
>   	u8	board_id;
> +	u8	rev_major;
> +	u8	rev_minor;
>   	u8	val;
>   };

[...]

[PATCH v2 3/3] renesas: rcar3: Load the correct device tree

[Detlev Casanova]

The Renesas R-Car Gen3 boards use different device trees than
the default one.

This commit uses the sysinfo's board id and revision
to determine which linux device tree to load:
 * H3 (Starter Kit Premier v2.0): renesas/r8a77951-ulcb.dtb
 * H3e (Starter Kit Premier v2.1): renesas/r8a779m1-ulcb.dtb

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 board/renesas/ulcb/ulcb.c    | 59 ++++++++++++++++++++++++++++++++++++
 configs/rcar3_ulcb_defconfig |  1 +
 2 files changed, 60 insertions(+)

diff --git a/board/renesas/ulcb/ulcb.c b/board/renesas/ulcb/ulcb.c
index 1477750f921..cc78e0952b6 100644
--- a/board/renesas/ulcb/ulcb.c
+++ b/board/renesas/ulcb/ulcb.c
@@ -27,6 +27,7 @@
 #include <asm/arch/sh_sdhi.h>
 #include <i2c.h>
 #include <mmc.h>
+#include <sysinfo.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -65,6 +66,64 @@ int board_init(void)
 	return 0;
 }
 
+int misc_init_r(void)
+{
+	struct udevice *dev;
+	int board_id;
+	int rev_major, rev_minor;
+	int ret = sysinfo_get(&dev);
+
+	if (ret) {
+		debug("Cannot get sysinfo: %d\n", ret);
+		return 0;
+	}
+
+	ret = sysinfo_detect(dev);
+	if (ret) {
+		debug("Cannot detect sysinfo: %d\n", ret);
+		return 0;
+	}
+
+	ret = sysinfo_get_int(dev,
+			      SYSINFO_ID_BOARD_MODEL,
+			      &board_id);
+
+	if (ret) {
+		debug("Cannot get sysinfo int: %d\n", ret);
+		return 0;
+	}
+
+	ret = sysinfo_get_int(dev,
+			      RCAR_SYSINFO_REV_MAJOR,
+			      &rev_major);
+
+	if (ret) {
+		debug("Cannot get sysinfo int: %d\n", ret);
+		return 0;
+	}
+
+	ret = sysinfo_get_int(dev,
+			      RCAR_SYSINFO_REV_MINOR,
+			      &rev_minor);
+
+	if (ret) {
+		debug("Cannot get sysinfo int: %d\n", ret);
+		return 0;
+	}
+
+	if (board_id == BOARD_STARTER_KIT_PRE && rev_major == '2') {
+		/*
+		 * H3 and H3e boards
+		 */
+		if (rev_minor == '0')
+			env_set("fdtfile", "renesas/r8a77951-ulcb.dtb");
+		else if (rev_minor == '1')
+			env_set("fdtfile", "renesas/r8a779m1-ulcb.dtb");
+	}
+
+	return 0;
+}
+
 #ifdef CONFIG_MULTI_DTB_FIT
 int board_fit_config_name_match(const char *name)
 {
diff --git a/configs/rcar3_ulcb_defconfig b/configs/rcar3_ulcb_defconfig
index b8fdb5e3826..752d33d77ec 100644
--- a/configs/rcar3_ulcb_defconfig
+++ b/configs/rcar3_ulcb_defconfig
@@ -111,3 +111,4 @@ CONFIG_USB_XHCI_HCD=y
 CONFIG_USB_EHCI_HCD=y
 CONFIG_USB_EHCI_GENERIC=y
 CONFIG_USB_STORAGE=y
+CONFIG_MISC_INIT_R=y
-- 
2.39.3

Re: [PATCH v2 3/3] renesas: rcar3: Load the correct device tree

[Marek Vasut]

On 6/12/23 21:51, Detlev Casanova wrote:
> The Renesas R-Car Gen3 boards use different device trees than
> the default one.
> 
> This commit uses the sysinfo's board id and revision
> to determine which linux device tree to load:
>   * H3 (Starter Kit Premier v2.0): renesas/r8a77951-ulcb.dtb
>   * H3e (Starter Kit Premier v2.1): renesas/r8a779m1-ulcb.dtb

This is not about loading DTs (as the subject would suggest), this is 
about setting the correct default DT name in environment.

> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>   board/renesas/ulcb/ulcb.c    | 59 ++++++++++++++++++++++++++++++++++++
>   configs/rcar3_ulcb_defconfig |  1 +
>   2 files changed, 60 insertions(+)
> 
> diff --git a/board/renesas/ulcb/ulcb.c b/board/renesas/ulcb/ulcb.c
> index 1477750f921..cc78e0952b6 100644
> --- a/board/renesas/ulcb/ulcb.c
> +++ b/board/renesas/ulcb/ulcb.c
> @@ -27,6 +27,7 @@
>   #include <asm/arch/sh_sdhi.h>
>   #include <i2c.h>
>   #include <mmc.h>
> +#include <sysinfo.h>
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> @@ -65,6 +66,64 @@ int board_init(void)
>   	return 0;
>   }
>   
> +int misc_init_r(void)
> +{
> +	struct udevice *dev;
> +	int board_id;
> +	int rev_major, rev_minor;
> +	int ret = sysinfo_get(&dev);
> +
> +	if (ret) {
> +		debug("Cannot get sysinfo: %d\n", ret);
> +		return 0;

Why do we ignore errors here ?

> +	}
> +
> +	ret = sysinfo_detect(dev);
> +	if (ret) {
> +		debug("Cannot detect sysinfo: %d\n", ret);
> +		return 0;
> +	}

Looking at all this, I really have to wonder, wouldn't it be nicer to 
introduce a 'sysinfo' command which provides interface to obtain the 
different properties (like board name, id, revision ...) from U-Boot 
command line, and then script the DT selection in U-Boot shell ?

Re: [PATCH v2 3/3] renesas: rcar3: Load the correct device tree

[Detlev Casanova]

On Wednesday, June 14, 2023 9:53:14 A.M. EDT Marek Vasut wrote:
> On 6/12/23 21:51, Detlev Casanova wrote:
> > The Renesas R-Car Gen3 boards use different device trees than
> > the default one.
> > 
> > This commit uses the sysinfo's board id and revision
> > 
> > to determine which linux device tree to load:
> >   * H3 (Starter Kit Premier v2.0): renesas/r8a77951-ulcb.dtb
> >   * H3e (Starter Kit Premier v2.1): renesas/r8a779m1-ulcb.dtb
> 
> This is not about loading DTs (as the subject would suggest), this is
> about setting the correct default DT name in environment.
> 
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >   board/renesas/ulcb/ulcb.c    | 59 ++++++++++++++++++++++++++++++++++++
> >   configs/rcar3_ulcb_defconfig |  1 +
> >   2 files changed, 60 insertions(+)
> > 
> > diff --git a/board/renesas/ulcb/ulcb.c b/board/renesas/ulcb/ulcb.c
> > index 1477750f921..cc78e0952b6 100644
> > --- a/board/renesas/ulcb/ulcb.c
> > +++ b/board/renesas/ulcb/ulcb.c
> > @@ -27,6 +27,7 @@
> > 
> >   #include <asm/arch/sh_sdhi.h>
> >   #include <i2c.h>
> >   #include <mmc.h>
> > 
> > +#include <sysinfo.h>
> > 
> >   DECLARE_GLOBAL_DATA_PTR;
> > 
> > @@ -65,6 +66,64 @@ int board_init(void)
> > 
> >   	return 0;
> >   
> >   }
> > 
> > +int misc_init_r(void)
> > +{
> > +	struct udevice *dev;
> > +	int board_id;
> > +	int rev_major, rev_minor;
> > +	int ret = sysinfo_get(&dev);
> > +
> > +	if (ret) {
> > +		debug("Cannot get sysinfo: %d\n", ret);
> > +		return 0;
> 
> Why do we ignore errors here ?
> 
> > +	}
> > +
> > +	ret = sysinfo_detect(dev);
> > +	if (ret) {
> > +		debug("Cannot detect sysinfo: %d\n", ret);
> > +		return 0;
> > +	}
> 
> Looking at all this, I really have to wonder, wouldn't it be nicer to
> introduce a 'sysinfo' command which provides interface to obtain the
> different properties (like board name, id, revision ...) from U-Boot
> command line, and then script the DT selection in U-Boot shell ?

Yes, that could be a good option. This is more based on how raspberry pis are 
selecting the correct devicetree in `board/raspberrypi/rpi/rpi.c`.
It is either about having simple shell scripts that are similar between 
devices and the implementation is "hidden" in C for each platform (maybe 
easier to use but less flexible). Or more complex shell scripts with simpler C 
implementation (more flexible but having to modify a boot script can become 
complicated for users)

Has this direction choice been discussed in the past already ?

Re: [PATCH v2 3/3] renesas: rcar3: Load the correct device tree

[Marek Vasut]

On 6/14/23 17:10, Detlev Casanova wrote:
> On Wednesday, June 14, 2023 9:53:14 A.M. EDT Marek Vasut wrote:
>> On 6/12/23 21:51, Detlev Casanova wrote:
>>> The Renesas R-Car Gen3 boards use different device trees than
>>> the default one.
>>>
>>> This commit uses the sysinfo's board id and revision
>>>
>>> to determine which linux device tree to load:
>>>    * H3 (Starter Kit Premier v2.0): renesas/r8a77951-ulcb.dtb
>>>    * H3e (Starter Kit Premier v2.1): renesas/r8a779m1-ulcb.dtb
>>
>> This is not about loading DTs (as the subject would suggest), this is
>> about setting the correct default DT name in environment.
>>
>>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>>> ---
>>>
>>>    board/renesas/ulcb/ulcb.c    | 59 ++++++++++++++++++++++++++++++++++++
>>>    configs/rcar3_ulcb_defconfig |  1 +
>>>    2 files changed, 60 insertions(+)
>>>
>>> diff --git a/board/renesas/ulcb/ulcb.c b/board/renesas/ulcb/ulcb.c
>>> index 1477750f921..cc78e0952b6 100644
>>> --- a/board/renesas/ulcb/ulcb.c
>>> +++ b/board/renesas/ulcb/ulcb.c
>>> @@ -27,6 +27,7 @@
>>>
>>>    #include <asm/arch/sh_sdhi.h>
>>>    #include <i2c.h>
>>>    #include <mmc.h>
>>>
>>> +#include <sysinfo.h>
>>>
>>>    DECLARE_GLOBAL_DATA_PTR;
>>>
>>> @@ -65,6 +66,64 @@ int board_init(void)
>>>
>>>    	return 0;
>>>    
>>>    }
>>>
>>> +int misc_init_r(void)
>>> +{
>>> +	struct udevice *dev;
>>> +	int board_id;
>>> +	int rev_major, rev_minor;
>>> +	int ret = sysinfo_get(&dev);
>>> +
>>> +	if (ret) {
>>> +		debug("Cannot get sysinfo: %d\n", ret);
>>> +		return 0;
>>
>> Why do we ignore errors here ?
>>
>>> +	}
>>> +
>>> +	ret = sysinfo_detect(dev);
>>> +	if (ret) {
>>> +		debug("Cannot detect sysinfo: %d\n", ret);
>>> +		return 0;
>>> +	}
>>
>> Looking at all this, I really have to wonder, wouldn't it be nicer to
>> introduce a 'sysinfo' command which provides interface to obtain the
>> different properties (like board name, id, revision ...) from U-Boot
>> command line, and then script the DT selection in U-Boot shell ?
> 
> Yes, that could be a good option. This is more based on how raspberry pis are
> selecting the correct devicetree in `board/raspberrypi/rpi/rpi.c`.
> It is either about having simple shell scripts that are similar between
> devices and the implementation is "hidden" in C for each platform (maybe
> easier to use but less flexible). Or more complex shell scripts with simpler C
> implementation (more flexible but having to modify a boot script can become
> complicated for users)
> 
> Has this direction choice been discussed in the past already ?

The less hard-coded board code (which cannot be updated by the user 
easily), the better. Scripts can be updated in deployment far easier 
than the bootloader itself. Hence the push for scripts over custom C code.

Re: [PATCH v2 3/3] renesas: rcar3: Load the correct device tree

[Detlev Casanova]

On Wednesday, June 14, 2023 11:32:31 A.M. EDT Marek Vasut wrote:
> On 6/14/23 17:10, Detlev Casanova wrote:
> > On Wednesday, June 14, 2023 9:53:14 A.M. EDT Marek Vasut wrote:
> >> On 6/12/23 21:51, Detlev Casanova wrote:
> >>> The Renesas R-Car Gen3 boards use different device trees than
> >>> the default one.
> >>> 
> >>> This commit uses the sysinfo's board id and revision
> >>> 
> >>> to determine which linux device tree to load:
> >>>    * H3 (Starter Kit Premier v2.0): renesas/r8a77951-ulcb.dtb
> >>>    * H3e (Starter Kit Premier v2.1): renesas/r8a779m1-ulcb.dtb
> >> 
> >> This is not about loading DTs (as the subject would suggest), this is
> >> about setting the correct default DT name in environment.
> >> 
> >>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> >>> ---
> >>> 
> >>>    board/renesas/ulcb/ulcb.c    | 59
> >>>    ++++++++++++++++++++++++++++++++++++
> >>>    configs/rcar3_ulcb_defconfig |  1 +
> >>>    2 files changed, 60 insertions(+)
> >>> 
> >>> diff --git a/board/renesas/ulcb/ulcb.c b/board/renesas/ulcb/ulcb.c
> >>> index 1477750f921..cc78e0952b6 100644
> >>> --- a/board/renesas/ulcb/ulcb.c
> >>> +++ b/board/renesas/ulcb/ulcb.c
> >>> @@ -27,6 +27,7 @@
> >>> 
> >>>    #include <asm/arch/sh_sdhi.h>
> >>>    #include <i2c.h>
> >>>    #include <mmc.h>
> >>> 
> >>> +#include <sysinfo.h>
> >>> 
> >>>    DECLARE_GLOBAL_DATA_PTR;
> >>> 
> >>> @@ -65,6 +66,64 @@ int board_init(void)
> >>> 
> >>>    	return 0;
> >>>    
> >>>    }
> >>> 
> >>> +int misc_init_r(void)
> >>> +{
> >>> +	struct udevice *dev;
> >>> +	int board_id;
> >>> +	int rev_major, rev_minor;
> >>> +	int ret = sysinfo_get(&dev);
> >>> +
> >>> +	if (ret) {
> >>> +		debug("Cannot get sysinfo: %d\n", ret);
> >>> +		return 0;
> >> 
> >> Why do we ignore errors here ?
> >> 
> >>> +	}
> >>> +
> >>> +	ret = sysinfo_detect(dev);
> >>> +	if (ret) {
> >>> +		debug("Cannot detect sysinfo: %d\n", ret);
> >>> +		return 0;
> >>> +	}
> >> 
> >> Looking at all this, I really have to wonder, wouldn't it be nicer to
> >> introduce a 'sysinfo' command which provides interface to obtain the
> >> different properties (like board name, id, revision ...) from U-Boot
> >> command line, and then script the DT selection in U-Boot shell ?
> > 
> > Yes, that could be a good option. This is more based on how raspberry pis
> > are selecting the correct devicetree in `board/raspberrypi/rpi/rpi.c`. It
> > is either about having simple shell scripts that are similar between
> > devices and the implementation is "hidden" in C for each platform (maybe
> > easier to use but less flexible). Or more complex shell scripts with
> > simpler C implementation (more flexible but having to modify a boot
> > script can become complicated for users)
> > 
> > Has this direction choice been discussed in the past already ?
> 
> The less hard-coded board code (which cannot be updated by the user
> easily), the better. Scripts can be updated in deployment far easier
> than the bootloader itself. Hence the push for scripts over custom C code.

That makes sense. I'll create a new command for this then and use it to select 
the dtb in the ulcb boards script.

Re: [PATCH v2 3/3] renesas: rcar3: Load the correct device tree

[Marek Vasut]

On 6/14/23 17:40, Detlev Casanova wrote:
> On Wednesday, June 14, 2023 11:32:31 A.M. EDT Marek Vasut wrote:
>> On 6/14/23 17:10, Detlev Casanova wrote:
>>> On Wednesday, June 14, 2023 9:53:14 A.M. EDT Marek Vasut wrote:
>>>> On 6/12/23 21:51, Detlev Casanova wrote:
>>>>> The Renesas R-Car Gen3 boards use different device trees than
>>>>> the default one.
>>>>>
>>>>> This commit uses the sysinfo's board id and revision
>>>>>
>>>>> to determine which linux device tree to load:
>>>>>     * H3 (Starter Kit Premier v2.0): renesas/r8a77951-ulcb.dtb
>>>>>     * H3e (Starter Kit Premier v2.1): renesas/r8a779m1-ulcb.dtb
>>>>
>>>> This is not about loading DTs (as the subject would suggest), this is
>>>> about setting the correct default DT name in environment.
>>>>
>>>>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>>>>> ---
>>>>>
>>>>>     board/renesas/ulcb/ulcb.c    | 59
>>>>>     ++++++++++++++++++++++++++++++++++++
>>>>>     configs/rcar3_ulcb_defconfig |  1 +
>>>>>     2 files changed, 60 insertions(+)
>>>>>
>>>>> diff --git a/board/renesas/ulcb/ulcb.c b/board/renesas/ulcb/ulcb.c
>>>>> index 1477750f921..cc78e0952b6 100644
>>>>> --- a/board/renesas/ulcb/ulcb.c
>>>>> +++ b/board/renesas/ulcb/ulcb.c
>>>>> @@ -27,6 +27,7 @@
>>>>>
>>>>>     #include <asm/arch/sh_sdhi.h>
>>>>>     #include <i2c.h>
>>>>>     #include <mmc.h>
>>>>>
>>>>> +#include <sysinfo.h>
>>>>>
>>>>>     DECLARE_GLOBAL_DATA_PTR;
>>>>>
>>>>> @@ -65,6 +66,64 @@ int board_init(void)
>>>>>
>>>>>     	return 0;
>>>>>     
>>>>>     }
>>>>>
>>>>> +int misc_init_r(void)
>>>>> +{
>>>>> +	struct udevice *dev;
>>>>> +	int board_id;
>>>>> +	int rev_major, rev_minor;
>>>>> +	int ret = sysinfo_get(&dev);
>>>>> +
>>>>> +	if (ret) {
>>>>> +		debug("Cannot get sysinfo: %d\n", ret);
>>>>> +		return 0;
>>>>
>>>> Why do we ignore errors here ?
>>>>
>>>>> +	}
>>>>> +
>>>>> +	ret = sysinfo_detect(dev);
>>>>> +	if (ret) {
>>>>> +		debug("Cannot detect sysinfo: %d\n", ret);
>>>>> +		return 0;
>>>>> +	}
>>>>
>>>> Looking at all this, I really have to wonder, wouldn't it be nicer to
>>>> introduce a 'sysinfo' command which provides interface to obtain the
>>>> different properties (like board name, id, revision ...) from U-Boot
>>>> command line, and then script the DT selection in U-Boot shell ?
>>>
>>> Yes, that could be a good option. This is more based on how raspberry pis
>>> are selecting the correct devicetree in `board/raspberrypi/rpi/rpi.c`. It
>>> is either about having simple shell scripts that are similar between
>>> devices and the implementation is "hidden" in C for each platform (maybe
>>> easier to use but less flexible). Or more complex shell scripts with
>>> simpler C implementation (more flexible but having to modify a boot
>>> script can become complicated for users)
>>>
>>> Has this direction choice been discussed in the past already ?
>>
>> The less hard-coded board code (which cannot be updated by the user
>> easily), the better. Scripts can be updated in deployment far easier
>> than the bootloader itself. Hence the push for scripts over custom C code.
> 
> That makes sense. I'll create a new command for this then and use it to select
> the dtb in the ulcb boards script.

Thanks !