[U-Boot] [PATCH 1/4] lib: fdt: Split fdtdec_setup_mem_size_base()

[U-Boot] [PATCH 1/4] lib: fdt: Split fdtdec_setup_mem_size_base()

[Marek Vasut]

Split fdtdec_setup_mem_size_base() into fdtdec_setup_mem_size_base_fdt(),
which allows the caller to pass custom blob into the function and the
original fdtdec_setup_mem_size_base(), which uses the gd->fdt_blob. This
is useful when configuring the DRAM properties from a FDT blob fragment
passed in by the firmware.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
 include/fdtdec.h | 20 ++++++++++++++++++++
 lib/fdtdec.c     | 11 ++++++++---
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index b7e35cd87c..3e681099a8 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -917,6 +917,26 @@ struct display_timing {
 int fdtdec_decode_display_timing(const void *blob, int node, int index,
 				 struct display_timing *config);
 
+/**
+ * fdtdec_setup_mem_size_base_fdt() - decode and setup gd->ram_size and
+ * gd->ram_start
+ *
+ * Decode the /memory 'reg' property to determine the size and start of the
+ * first memory bank, populate the global data with the size and start of the
+ * first bank of memory.
+ *
+ * This function should be called from a boards dram_init(). This helper
+ * function allows for boards to query the device tree for DRAM size and start
+ * address instead of hard coding the value in the case where the memory size
+ * and start address cannot be detected automatically.
+ *
+ * @param blob		FDT blob
+ *
+ * @return 0 if OK, -EINVAL if the /memory node or reg property is missing or
+ * invalid
+ */
+int fdtdec_setup_mem_size_base_fdt(const void *blob);
+
 /**
  * fdtdec_setup_mem_size_base() - decode and setup gd->ram_size and
  * gd->ram_start
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 09a7e133a5..3f29e9d647 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -1088,18 +1088,18 @@ int fdtdec_decode_display_timing(const void *blob, int parent, int index,
 	return ret;
 }
 
-int fdtdec_setup_mem_size_base(void)
+int fdtdec_setup_mem_size_base_fdt(const void *blob)
 {
 	int ret, mem;
 	struct fdt_resource res;
 
-	mem = fdt_path_offset(gd->fdt_blob, "/memory");
+	mem = fdt_path_offset(blob, "/memory");
 	if (mem < 0) {
 		debug("%s: Missing /memory node\n", __func__);
 		return -EINVAL;
 	}
 
-	ret = fdt_get_resource(gd->fdt_blob, mem, "reg", 0, &res);
+	ret = fdt_get_resource(blob, mem, "reg", 0, &res);
 	if (ret != 0) {
 		debug("%s: Unable to decode first memory bank\n", __func__);
 		return -EINVAL;
@@ -1113,6 +1113,11 @@ int fdtdec_setup_mem_size_base(void)
 	return 0;
 }
 
+int fdtdec_setup_mem_size_base(void)
+{
+	return fdtdec_setup_mem_size_base_fdt(gd->fdt_blob);
+}
+
 #if defined(CONFIG_NR_DRAM_BANKS)
 
 static int get_next_memory_node(const void *blob, int mem)
-- 
2.19.2

[U-Boot] [PATCH 2/4] lib: fdt: Split fdtdec_setup_memory_banksize()

[Marek Vasut]

Split fdtdec_setup_memory_banksize() into fdtdec_setup_memory_banksize_fdt(),
which allows the caller to pass custom blob into the function and the
original fdtdec_setup_memory_banksize(), which uses the gd->fdt_blob. This
is useful when configuring the DRAM properties from a FDT blob fragment
passed in by the firmware.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
 include/fdtdec.h | 19 +++++++++++++++++++
 lib/fdtdec.c     | 18 ++++++++++++------
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index 3e681099a8..ad00f79f20 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -955,6 +955,25 @@ int fdtdec_setup_mem_size_base_fdt(const void *blob);
  */
 int fdtdec_setup_mem_size_base(void);
 
+/**
+ * fdtdec_setup_memory_banksize_fdt() - decode and populate gd->bd->bi_dram
+ *
+ * Decode the /memory 'reg' property to determine the address and size of the
+ * memory banks. Use this data to populate the global data board info with the
+ * phys address and size of memory banks.
+ *
+ * This function should be called from a boards dram_init_banksize(). This
+ * helper function allows for boards to query the device tree for memory bank
+ * information instead of hard coding the information in cases where it cannot
+ * be detected automatically.
+ *
+ * @param blob		FDT blob
+ *
+ * @return 0 if OK, -EINVAL if the /memory node or reg property is missing or
+ * invalid
+ */
+int fdtdec_setup_memory_banksize_fdt(const void *blob);
+
 /**
  * fdtdec_setup_memory_banksize() - decode and populate gd->bd->bi_dram
  *
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 3f29e9d647..79915b59bd 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -1123,33 +1123,33 @@ int fdtdec_setup_mem_size_base(void)
 static int get_next_memory_node(const void *blob, int mem)
 {
 	do {
-		mem = fdt_node_offset_by_prop_value(gd->fdt_blob, mem,
+		mem = fdt_node_offset_by_prop_value(blob, mem,
 						    "device_type", "memory", 7);
 	} while (!fdtdec_get_is_enabled(blob, mem));
 
 	return mem;
 }
 
-int fdtdec_setup_memory_banksize(void)
+int fdtdec_setup_memory_banksize_fdt(const void *blob)
 {
 	int bank, ret, mem, reg = 0;
 	struct fdt_resource res;
 
-	mem = get_next_memory_node(gd->fdt_blob, -1);
+	mem = get_next_memory_node(blob, -1);
 	if (mem < 0) {
 		debug("%s: Missing /memory node\n", __func__);
 		return -EINVAL;
 	}
 
 	for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
-		ret = fdt_get_resource(gd->fdt_blob, mem, "reg", reg++, &res);
+		ret = fdt_get_resource(blob, mem, "reg", reg++, &res);
 		if (ret == -FDT_ERR_NOTFOUND) {
 			reg = 0;
-			mem = get_next_memory_node(gd->fdt_blob, mem);
+			mem = get_next_memory_node(blob, mem);
 			if (mem == -FDT_ERR_NOTFOUND)
 				break;
 
-			ret = fdt_get_resource(gd->fdt_blob, mem, "reg", reg++, &res);
+			ret = fdt_get_resource(blob, mem, "reg", reg++, &res);
 			if (ret == -FDT_ERR_NOTFOUND)
 				break;
 		}
@@ -1169,6 +1169,12 @@ int fdtdec_setup_memory_banksize(void)
 
 	return 0;
 }
+
+int fdtdec_setup_memory_banksize(void)
+{
+	return fdtdec_setup_memory_banksize_fdt(gd->fdt_blob);
+
+}
 #endif
 
 #if CONFIG_IS_ENABLED(MULTI_DTB_FIT)
-- 
2.19.2

[U-Boot] [PATCH 3/4] ARM: renesas: Save boot parameters passed in by ATF

[Marek Vasut]

The ATF can pass additional information via the first four registers,
x0...x3. The R-Car Gen3 with mainline ATF, register x1 contains pointer
to a device tree with platform information. Save these registers for
future use.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
---
 arch/arm/mach-rmobile/lowlevel_init_gen3.S | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/mach-rmobile/lowlevel_init_gen3.S b/arch/arm/mach-rmobile/lowlevel_init_gen3.S
index f42b53fd88..213ec143e2 100644
--- a/arch/arm/mach-rmobile/lowlevel_init_gen3.S
+++ b/arch/arm/mach-rmobile/lowlevel_init_gen3.S
@@ -16,6 +16,21 @@
 #include <linux/linkage.h>
 #include <asm/macro.h>
 
+.align 8
+.globl	rcar_atf_boot_args
+rcar_atf_boot_args:
+	.dword 0
+	.dword 0
+	.dword 0
+	.dword 0
+
+ENTRY(save_boot_params)
+	adr	x8, rcar_atf_boot_args
+	stp	x0, x1, [x8], #16
+	stp	x2, x3, [x8], #16
+	b	save_boot_params_ret
+ENDPROC(save_boot_params)
+
 ENTRY(lowlevel_init)
 	mov	x29, lr			/* Save LR */
 
-- 
2.19.2

[U-Boot] [PATCH 4/4] ARM: renesas: Configure DRAM size from ATF DT fragment

[Marek Vasut]

The ATF can pass additional information via the first four registers,
x0...x3. The R-Car Gen3 with mainline ATF, register x1 contains pointer
to a device tree with platform information. Parse this device tree and
extract DRAM size information from it. This is useful on systems where
the DRAM size can vary between configurations.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
---
 board/renesas/ebisu/ebisu.c           | 28 +++++++++++++++++++++++----
 board/renesas/salvator-x/salvator-x.c | 28 +++++++++++++++++++++++----
 board/renesas/ulcb/ulcb.c             | 28 +++++++++++++++++++++++----
 3 files changed, 72 insertions(+), 12 deletions(-)

diff --git a/board/renesas/ebisu/ebisu.c b/board/renesas/ebisu/ebisu.c
index 5d8b79eee3..60429e4529 100644
--- a/board/renesas/ebisu/ebisu.c
+++ b/board/renesas/ebisu/ebisu.c
@@ -43,17 +43,37 @@ int board_init(void)
 	return 0;
 }
 
+/*
+ * If the firmware passed a device tree use it for U-Boot DRAM setup.
+ */
+extern u64 rcar_atf_boot_args[];
+
 int dram_init(void)
 {
-	if (fdtdec_setup_mem_size_base() != 0)
-		return -EINVAL;
+	const void *atf_fdt_blob = (const void *)(rcar_atf_boot_args[1]);
+	const void *blob;
 
-	return 0;
+	/* Check if ATF passed us DTB. If not, fall back to builtin DTB. */
+	if (fdt_magic(atf_fdt_blob) == FDT_MAGIC)
+		blob = atf_fdt_blob;
+	else
+		blob = gd->fdt_blob;
+
+	return fdtdec_setup_mem_size_base_fdt(blob);
 }
 
 int dram_init_banksize(void)
 {
-	fdtdec_setup_memory_banksize();
+	const void *atf_fdt_blob = (const void *)(rcar_atf_boot_args[1]);
+	const void *blob;
+
+	/* Check if ATF passed us DTB. If not, fall back to builtin DTB. */
+	if (fdt_magic(atf_fdt_blob) == FDT_MAGIC)
+		blob = atf_fdt_blob;
+	else
+		blob = gd->fdt_blob;
+
+	fdtdec_setup_memory_banksize_fdt(blob);
 
 	return 0;
 }
diff --git a/board/renesas/salvator-x/salvator-x.c b/board/renesas/salvator-x/salvator-x.c
index 8f0247e046..1db08fce6a 100644
--- a/board/renesas/salvator-x/salvator-x.c
+++ b/board/renesas/salvator-x/salvator-x.c
@@ -69,17 +69,37 @@ int board_init(void)
 	return 0;
 }
 
+/*
+ * If the firmware passed a device tree use it for U-Boot DRAM setup.
+ */
+extern u64 rcar_atf_boot_args[];
+
 int dram_init(void)
 {
-	if (fdtdec_setup_mem_size_base() != 0)
-		return -EINVAL;
+	const void *atf_fdt_blob = (const void *)(rcar_atf_boot_args[1]);
+	const void *blob;
 
-	return 0;
+	/* Check if ATF passed us DTB. If not, fall back to builtin DTB. */
+	if (fdt_magic(atf_fdt_blob) == FDT_MAGIC)
+		blob = atf_fdt_blob;
+	else
+		blob = gd->fdt_blob;
+
+	return fdtdec_setup_mem_size_base_fdt(blob);
 }
 
 int dram_init_banksize(void)
 {
-	fdtdec_setup_memory_banksize();
+	const void *atf_fdt_blob = (const void *)(rcar_atf_boot_args[1]);
+	const void *blob;
+
+	/* Check if ATF passed us DTB. If not, fall back to builtin DTB. */
+	if (fdt_magic(atf_fdt_blob) == FDT_MAGIC)
+		blob = atf_fdt_blob;
+	else
+		blob = gd->fdt_blob;
+
+	fdtdec_setup_memory_banksize_fdt(blob);
 
 	return 0;
 }
diff --git a/board/renesas/ulcb/ulcb.c b/board/renesas/ulcb/ulcb.c
index 81d6f8f6f2..3ed7bfaa88 100644
--- a/board/renesas/ulcb/ulcb.c
+++ b/board/renesas/ulcb/ulcb.c
@@ -68,17 +68,37 @@ int board_init(void)
 	return 0;
 }
 
+/*
+ * If the firmware passed a device tree use it for U-Boot DRAM setup.
+ */
+extern u64 rcar_atf_boot_args[];
+
 int dram_init(void)
 {
-	if (fdtdec_setup_mem_size_base() != 0)
-		return -EINVAL;
+	const void *atf_fdt_blob = (const void *)(rcar_atf_boot_args[1]);
+	const void *blob;
 
-	return 0;
+	/* Check if ATF passed us DTB. If not, fall back to builtin DTB. */
+	if (fdt_magic(atf_fdt_blob) == FDT_MAGIC)
+		blob = atf_fdt_blob;
+	else
+		blob = gd->fdt_blob;
+
+	return fdtdec_setup_mem_size_base_fdt(blob);
 }
 
 int dram_init_banksize(void)
 {
-	fdtdec_setup_memory_banksize();
+	const void *atf_fdt_blob = (const void *)(rcar_atf_boot_args[1]);
+	const void *blob;
+
+	/* Check if ATF passed us DTB. If not, fall back to builtin DTB. */
+	if (fdt_magic(atf_fdt_blob) == FDT_MAGIC)
+		blob = atf_fdt_blob;
+	else
+		blob = gd->fdt_blob;
+
+	fdtdec_setup_memory_banksize_fdt(blob);
 
 	return 0;
 }
-- 
2.19.2

[U-Boot] [PATCH 1/4] lib: fdt: Split fdtdec_setup_mem_size_base()

[Simon Glass]

On Mon, 4 Mar 2019 at 20:26, Marek Vasut <marek.vasut@gmail.com> wrote:
>
> Split fdtdec_setup_mem_size_base() into fdtdec_setup_mem_size_base_fdt(),
> which allows the caller to pass custom blob into the function and the
> original fdtdec_setup_mem_size_base(), which uses the gd->fdt_blob. This
> is useful when configuring the DRAM properties from a FDT blob fragment
> passed in by the firmware.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  include/fdtdec.h | 20 ++++++++++++++++++++
>  lib/fdtdec.c     | 11 ++++++++---
>  2 files changed, 28 insertions(+), 3 deletions(-)
>

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

[U-Boot] [PATCH 2/4] lib: fdt: Split fdtdec_setup_memory_banksize()

[Simon Glass]

On Mon, 4 Mar 2019 at 20:26, Marek Vasut <marek.vasut@gmail.com> wrote:
>
> Split fdtdec_setup_memory_banksize() into fdtdec_setup_memory_banksize_fdt(),
> which allows the caller to pass custom blob into the function and the
> original fdtdec_setup_memory_banksize(), which uses the gd->fdt_blob. This
> is useful when configuring the DRAM properties from a FDT blob fragment
> passed in by the firmware.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  include/fdtdec.h | 19 +++++++++++++++++++
>  lib/fdtdec.c     | 18 ++++++++++++------
>  2 files changed, 31 insertions(+), 6 deletions(-)

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

[U-Boot] [PATCH 3/4] ARM: renesas: Save boot parameters passed in by ATF

[Eugeniu Rosca]

Hi Marek,

On Tue, Mar 5, 2019 at 4:41 AM Marek Vasut <marek.vasut@gmail.com> wrote:
[..]
> +.align 8
> +.globl rcar_atf_boot_args
> +rcar_atf_boot_args:
> +       .dword 0
> +       .dword 0
> +       .dword 0
> +       .dword 0
> +
> +ENTRY(save_boot_params)
> +       adr     x8, rcar_atf_boot_args
> +       stp     x0, x1, [x8], #16
> +       stp     x2, x3, [x8], #16
> +       b       save_boot_params_ret
> +ENDPROC(save_boot_params)

What about relocating the function to C like in [1] and passing the 4
arguments to it from ASM like in [2]?
This would allow to:
 - reduce asm operations to minimum (mov and bl) and make code transparent.
 - avoid custom globals and store the ATF information in some C-defined struct.

[1] https://github.com/ARM-software/arm-trusted-firmware/blob/c48d02bade88b07fa7f43aa44e5217f68e5d047f/bl1/bl1_main.c#L263
[2] https://github.com/ARM-software/arm-trusted-firmware/blob/c48d02bade88b07fa7f43aa44e5217f68e5d047f/bl1/aarch32/bl1_exceptions.S#L133

Best regards,
Eugeniu.

[U-Boot] [PATCH 4/4] ARM: renesas: Configure DRAM size from ATF DT fragment

[Eugeniu Rosca]

Hi Marek cc: Michael

On Tue, Mar 5, 2019 at 4:37 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> The ATF can pass additional information via the first four registers,
> x0...x3. The R-Car Gen3 with mainline ATF, register x1 contains pointer
> to a device tree with platform information. Parse this device tree and
> extract DRAM size information from it. This is useful on systems where
> the DRAM size can vary between configurations.

1. Do the ATF changes supporting this feature already exist in mainline ATF?

2. I see more DRAM-related compile-time knobs in vanilla ATF:
➜  arm-trusted-firmware (c48d02bade88) $ grep "DRAM.*:"
plat/renesas/rcar/platform.mk
RCAR_DRAM_SPLIT := 0
RCAR_DRAM_LPDDR4_MEMCONF :=1
RCAR_DRAM_DDR3L_MEMCONF :=1
RCAR_DRAM_DDR3L_MEMDUAL :=1
RCAR_DRAM_CHANNEL :=15

My understanding is that these are local to ATF and U-Boot can stay
agnostic about them? IOW, DRAM start and size are totally enough for
U-Boot? TIA!

Best regards,
Eugeniu.

[U-Boot] [PATCH 3/4] ARM: renesas: Save boot parameters passed in by ATF

[Tom Rini]

On Tue, Mar 12, 2019 at 07:59:40PM +0100, Eugeniu Rosca wrote:
> Hi Marek,
> 
> On Tue, Mar 5, 2019 at 4:41 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> [..]
> > +.align 8
> > +.globl rcar_atf_boot_args
> > +rcar_atf_boot_args:
> > +       .dword 0
> > +       .dword 0
> > +       .dword 0
> > +       .dword 0
> > +
> > +ENTRY(save_boot_params)
> > +       adr     x8, rcar_atf_boot_args
> > +       stp     x0, x1, [x8], #16
> > +       stp     x2, x3, [x8], #16
> > +       b       save_boot_params_ret
> > +ENDPROC(save_boot_params)
> 
> What about relocating the function to C like in [1] and passing the 4
> arguments to it from ASM like in [2]?
> This would allow to:
>  - reduce asm operations to minimum (mov and bl) and make code transparent.
>  - avoid custom globals and store the ATF information in some C-defined struct.
> 
> [1] https://github.com/ARM-software/arm-trusted-firmware/blob/c48d02bade88b07fa7f43aa44e5217f68e5d047f/bl1/bl1_main.c#L263
> [2] https://github.com/ARM-software/arm-trusted-firmware/blob/c48d02bade88b07fa7f43aa44e5217f68e5d047f/bl1/aarch32/bl1_exceptions.S#L133

This is super early in the boot process and I'm really not a fan in
general of writing and calling C before we have things setup for C calls
to actually work normally, even more so for small things that can be
commented to be obvious as to what's going on.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190312/830ee494/attachment.sig>

[U-Boot] [PATCH 3/4] ARM: renesas: Save boot parameters passed in by ATF

[Eugeniu Rosca]

Hi Tom,

On Tue, Mar 12, 2019 at 03:42:24PM -0400, Tom Rini wrote:
> On Tue, Mar 12, 2019 at 07:59:40PM +0100, Eugeniu Rosca wrote:
> > Hi Marek,
> > 
> > On Tue, Mar 5, 2019 at 4:41 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> > [..]
> > > +.align 8
> > > +.globl rcar_atf_boot_args
> > > +rcar_atf_boot_args:
> > > +       .dword 0
> > > +       .dword 0
> > > +       .dword 0
> > > +       .dword 0
> > > +
> > > +ENTRY(save_boot_params)
> > > +       adr     x8, rcar_atf_boot_args
> > > +       stp     x0, x1, [x8], #16
> > > +       stp     x2, x3, [x8], #16
> > > +       b       save_boot_params_ret
> > > +ENDPROC(save_boot_params)
> > 
> > What about relocating the function to C like in [1] and passing the 4
> > arguments to it from ASM like in [2]?
> > This would allow to:
> >  - reduce asm operations to minimum (mov and bl) and make code transparent.
> >  - avoid custom globals and store the ATF information in some C-defined struct.
> > 
> > [1] https://github.com/ARM-software/arm-trusted-firmware/blob/c48d02bade88b07fa7f43aa44e5217f68e5d047f/bl1/bl1_main.c#L263
> > [2] https://github.com/ARM-software/arm-trusted-firmware/blob/c48d02bade88b07fa7f43aa44e5217f68e5d047f/bl1/aarch32/bl1_exceptions.S#L133
> 
> This is super early in the boot process and I'm really not a fan in
> general of writing and calling C before we have things setup for C calls
> to actually work normally, even more so for small things that can be
> commented to be obvious as to what's going on.

I don't have a strong preference about that. It is just my experience
that Renesas once rewrote the elaborate Bosch-contributed BL2 routine
of enabling the cntfrq_el0 system counter from ASM [1] to C [2]. This
is at the BL1-BL2 boundary, while we are still in EL3, so much earlier
in the boot process. This implementation currently runs on the targets
of our customers.

I am fine with any working solution. Thanks for commenting!

[1] https://github.com/renesas-rcar/arm-trusted-firmware/commit/e23524689abb637b14a2961a305f6cfb43909319
[2] https://github.com/renesas-rcar/arm-trusted-firmware/commit/867d84191319#diff-471251fb86bd634ba14891d950c1440eR177

> -- 
> Tom

Best regards,
Eugeniu.

[U-Boot] [PATCH 3/4] ARM: renesas: Save boot parameters passed in by ATF

[Marek Vasut]

On 3/12/19 9:29 PM, Eugeniu Rosca wrote:
> Hi Tom,

Hi,

> On Tue, Mar 12, 2019 at 03:42:24PM -0400, Tom Rini wrote:
>> On Tue, Mar 12, 2019 at 07:59:40PM +0100, Eugeniu Rosca wrote:
>>> Hi Marek,
>>>
>>> On Tue, Mar 5, 2019 at 4:41 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>> [..]
>>>> +.align 8
>>>> +.globl rcar_atf_boot_args
>>>> +rcar_atf_boot_args:
>>>> +       .dword 0
>>>> +       .dword 0
>>>> +       .dword 0
>>>> +       .dword 0
>>>> +
>>>> +ENTRY(save_boot_params)
>>>> +       adr     x8, rcar_atf_boot_args
>>>> +       stp     x0, x1, [x8], #16
>>>> +       stp     x2, x3, [x8], #16
>>>> +       b       save_boot_params_ret
>>>> +ENDPROC(save_boot_params)
>>>
>>> What about relocating the function to C like in [1] and passing the 4
>>> arguments to it from ASM like in [2]?
>>> This would allow to:
>>>  - reduce asm operations to minimum (mov and bl) and make code transparent.
>>>  - avoid custom globals and store the ATF information in some C-defined struct.
>>>
>>> [1] https://github.com/ARM-software/arm-trusted-firmware/blob/c48d02bade88b07fa7f43aa44e5217f68e5d047f/bl1/bl1_main.c#L263
>>> [2] https://github.com/ARM-software/arm-trusted-firmware/blob/c48d02bade88b07fa7f43aa44e5217f68e5d047f/bl1/aarch32/bl1_exceptions.S#L133
>>
>> This is super early in the boot process and I'm really not a fan in
>> general of writing and calling C before we have things setup for C calls
>> to actually work normally, even more so for small things that can be
>> commented to be obvious as to what's going on.
> 
> I don't have a strong preference about that. It is just my experience
> that Renesas once rewrote the elaborate Bosch-contributed BL2 routine
> of enabling the cntfrq_el0 system counter from ASM [1] to C [2].

At that point, BL2 already has a C runtime set up.

This code is instead running without the C runtime and we want to make
sure the correct registers get stored at the correct location, without
the compiler interfering with it in any way. Keep in mind this code runs
right at the beginning of U-Boot execution to make sure registers used
to pass parameters stay intact (which I believe is what Tom meant by
"early in the boot process").

> This
> is at the BL1-BL2 boundary, while we are still in EL3, so much earlier
> in the boot process. This implementation currently runs on the targets
> of our customers.
> 
> I am fine with any working solution. Thanks for commenting!
> 
> [1] https://github.com/renesas-rcar/arm-trusted-firmware/commit/e23524689abb637b14a2961a305f6cfb43909319
> [2] https://github.com/renesas-rcar/arm-trusted-firmware/commit/867d84191319#diff-471251fb86bd634ba14891d950c1440eR177
> 
>> -- 
>> Tom
> 
> Best regards,
> Eugeniu.
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 4/4] ARM: renesas: Configure DRAM size from ATF DT fragment

[Marek Vasut]

On 3/12/19 8:30 PM, Eugeniu Rosca wrote:
> Hi Marek cc: Michael

Hi,

> On Tue, Mar 5, 2019 at 4:37 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>
>> The ATF can pass additional information via the first four registers,
>> x0...x3. The R-Car Gen3 with mainline ATF, register x1 contains pointer
>> to a device tree with platform information. Parse this device tree and
>> extract DRAM size information from it. This is useful on systems where
>> the DRAM size can vary between configurations.
> 
> 1. Do the ATF changes supporting this feature already exist in mainline ATF?

Yes, for quite some time, see
commit 1d85c4bd5b6291b99feb2409525c96f0c1744328
    plat: rcar: Pass DTB with DRAM layout from BL2 to next stages

> 2. I see more DRAM-related compile-time knobs in vanilla ATF:
> ➜  arm-trusted-firmware (c48d02bade88) $ grep "DRAM.*:"
> plat/renesas/rcar/platform.mk
> RCAR_DRAM_SPLIT := 0
> RCAR_DRAM_LPDDR4_MEMCONF :=1
> RCAR_DRAM_DDR3L_MEMCONF :=1
> RCAR_DRAM_DDR3L_MEMDUAL :=1
> RCAR_DRAM_CHANNEL :=15
> 
> My understanding is that these are local to ATF and U-Boot can stay
> agnostic about them?

Yes, although the long term goal is to get rid of those and replace them
with DT configuration too. So ATF BL2 would then parse supplied DT to
figure out the DRAM layout instead of using those knobs.

> IOW, DRAM start and size are totally enough for U-Boot?

No, U-Boot needs to know start/size of each bank. That's what's in the
DT that is passed in, plus a machine compatible string, so technically,
you might be able to build one U-Boot for all boards already, but then
you hit a size limit where the BL33 must be below 1 MiB. I'm looking
into that one too.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 4/4] ARM: renesas: Configure DRAM size from ATF DT fragment

[Eugeniu Rosca]

On Tue, Mar 12, 2019 at 10:11:21PM +0100, Marek Vasut wrote:
> On 3/12/19 8:30 PM, Eugeniu Rosca wrote:
> > Hi Marek cc: Michael
> 
> Hi,
> 
> > On Tue, Mar 5, 2019 at 4:37 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>
> >> The ATF can pass additional information via the first four registers,
> >> x0...x3. The R-Car Gen3 with mainline ATF, register x1 contains pointer
> >> to a device tree with platform information. Parse this device tree and
> >> extract DRAM size information from it. This is useful on systems where
> >> the DRAM size can vary between configurations.
> > 
> > 1. Do the ATF changes supporting this feature already exist in mainline ATF?
> 
> Yes, for quite some time, see
> commit 1d85c4bd5b6291b99feb2409525c96f0c1744328
>     plat: rcar: Pass DTB with DRAM layout from BL2 to next stages

That's helpful, but I think such information should go by default into
commit description.

[..]

Besides the above, I wonder why this patch duplicates code across three
board files? Could this code be relocated to some common area like [1]?
FWIW building the latter could be re-enabled by reverting the Makefile
changes from v2018.01-rc1 commit [2].

To differentiate between the boards which expect/require the ATF args
and those which don't, I guess that run-time (IS_ENABLED) checking of
a newly created Kconfig symbol, e.g. SUPPORTS_ATF_ARGS or similar,
selected on per-board basis, could do the job?

Note that we already face the need to have an area for Android features
common between all Gen3 boards, so I think reviving [1] is inevitable.

[1] board/renesas/rcar-common/common.c
[2] http://git.denx.de/?p=u-boot.git;a=commitdiff;h=e23eb942ad10
    ("ARM: rmobile: Stop using rcar-common/common.c on Gen3")

Best regards,
Eugeniu.

[U-Boot] [PATCH 4/4] ARM: renesas: Configure DRAM size from ATF DT fragment

[Marek Vasut]

On 3/13/19 4:25 PM, Eugeniu Rosca wrote:
> On Tue, Mar 12, 2019 at 10:11:21PM +0100, Marek Vasut wrote:
>> On 3/12/19 8:30 PM, Eugeniu Rosca wrote:
>>> Hi Marek cc: Michael
>>
>> Hi,
>>
>>> On Tue, Mar 5, 2019 at 4:37 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>
>>>> The ATF can pass additional information via the first four registers,
>>>> x0...x3. The R-Car Gen3 with mainline ATF, register x1 contains pointer
>>>> to a device tree with platform information. Parse this device tree and
>>>> extract DRAM size information from it. This is useful on systems where
>>>> the DRAM size can vary between configurations.
>>>
>>> 1. Do the ATF changes supporting this feature already exist in mainline ATF?
>>
>> Yes, for quite some time, see
>> commit 1d85c4bd5b6291b99feb2409525c96f0c1744328
>>     plat: rcar: Pass DTB with DRAM layout from BL2 to next stages
> 
> That's helpful, but I think such information should go by default into
> commit description.
> 
> [..]
> 
> Besides the above, I wonder why this patch duplicates code across three
> board files? Could this code be relocated to some common area like [1]?

That's the plan.

> FWIW building the latter could be re-enabled by reverting the Makefile
> changes from v2018.01-rc1 commit [2].

I prefer a more progressive approach, that is applying patches, rather
than reverting.

> To differentiate between the boards which expect/require the ATF args
> and those which don't, I guess that run-time (IS_ENABLED) checking of
> a newly created Kconfig symbol, e.g. SUPPORTS_ATF_ARGS or similar,
> selected on per-board basis, could do the job?

No, the code should auto-detect whether the ATF supports the DT passing
or not.

> Note that we already face the need to have an area for Android features
> common between all Gen3 boards, so I think reviving [1] is inevitable.
> 
> [1] board/renesas/rcar-common/common.c
> [2] http://git.denx.de/?p=u-boot.git;a=commitdiff;h=e23eb942ad10
>     ("ARM: rmobile: Stop using rcar-common/common.c on Gen3")
> 
> Best regards,
> Eugeniu.
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 4/4] ARM: renesas: Configure DRAM size from ATF DT fragment

[Eugeniu Rosca]

Hi Marek,

On Tue, Apr 02, 2019 at 05:45:29AM +0200, Marek Vasut wrote:
> On 3/13/19 4:25 PM, Eugeniu Rosca wrote:
> > On Tue, Mar 12, 2019 at 10:11:21PM +0100, Marek Vasut wrote:
> >> On 3/12/19 8:30 PM, Eugeniu Rosca wrote:
> >>> Hi Marek cc: Michael
> >>
> >> Hi,
> >>
> >>> On Tue, Mar 5, 2019 at 4:37 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>
> >>>> The ATF can pass additional information via the first four registers,
> >>>> x0...x3. The R-Car Gen3 with mainline ATF, register x1 contains pointer
> >>>> to a device tree with platform information. Parse this device tree and
> >>>> extract DRAM size information from it. This is useful on systems where
> >>>> the DRAM size can vary between configurations.
> >>>
> >>> 1. Do the ATF changes supporting this feature already exist in mainline ATF?
> >>
> >> Yes, for quite some time, see
> >> commit 1d85c4bd5b6291b99feb2409525c96f0c1744328
> >>     plat: rcar: Pass DTB with DRAM layout from BL2 to next stages
> > 
> > That's helpful, but I think such information should go by default into
> > commit description.
> > 
> > [..]
> > 
> > Besides the above, I wonder why this patch duplicates code across three
> > board files? Could this code be relocated to some common area like [1]?
> 
> That's the plan.

I look forward to seeing v2 then, since this would allow us not
duplicating this code over and over again in the board-specific files
of the customer R-Car3 targets.

> 
> > FWIW building the latter could be re-enabled by reverting the Makefile
> > changes from v2018.01-rc1 commit [2].
> 
> I prefer a more progressive approach, that is applying patches, rather
> than reverting.

"reverting" is used here figuratively, suggesting to undo the Makefile
changes added by commit [2], in order to create/restore the R-Car3
common/board-independent space. It doesn't ask creating a revert commit.

> > To differentiate between the boards which expect/require the ATF args
> > and those which don't, I guess that run-time (IS_ENABLED) checking of
> > a newly created Kconfig symbol, e.g. SUPPORTS_ATF_ARGS or similar,
> > selected on per-board basis, could do the job?
> 
> No, the code should auto-detect whether the ATF supports the DT passing
> or not.

Great. Less build-time options the better.

> > Note that we already face the need to have an area for Android features
> > common between all Gen3 boards, so I think reviving [1] is inevitable.
> > 
> > [1] board/renesas/rcar-common/common.c
> > [2] http://git.denx.de/?p=u-boot.git;a=commitdiff;h=e23eb942ad10
> >     ("ARM: rmobile: Stop using rcar-common/common.c on Gen3")
> > 
> > Best regards,
> > Eugeniu.
> > 
> -- 
> Best regards,
> Marek Vasut

I would appreciate being CC-ed in v2, if possible. Thanks!

[U-Boot] [PATCH 4/4] ARM: renesas: Configure DRAM size from ATF DT fragment

[Marek Vasut]

On 4/4/19 3:45 PM, Eugeniu Rosca wrote:
> Hi Marek,

Hi,

> On Tue, Apr 02, 2019 at 05:45:29AM +0200, Marek Vasut wrote:
>> On 3/13/19 4:25 PM, Eugeniu Rosca wrote:
>>> On Tue, Mar 12, 2019 at 10:11:21PM +0100, Marek Vasut wrote:
>>>> On 3/12/19 8:30 PM, Eugeniu Rosca wrote:
>>>>> Hi Marek cc: Michael
>>>>
>>>> Hi,
>>>>
>>>>> On Tue, Mar 5, 2019 at 4:37 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>
>>>>>> The ATF can pass additional information via the first four registers,
>>>>>> x0...x3. The R-Car Gen3 with mainline ATF, register x1 contains pointer
>>>>>> to a device tree with platform information. Parse this device tree and
>>>>>> extract DRAM size information from it. This is useful on systems where
>>>>>> the DRAM size can vary between configurations.
>>>>>
>>>>> 1. Do the ATF changes supporting this feature already exist in mainline ATF?
>>>>
>>>> Yes, for quite some time, see
>>>> commit 1d85c4bd5b6291b99feb2409525c96f0c1744328
>>>>     plat: rcar: Pass DTB with DRAM layout from BL2 to next stages
>>>
>>> That's helpful, but I think such information should go by default into
>>> commit description.
>>>
>>> [..]
>>>
>>> Besides the above, I wonder why this patch duplicates code across three
>>> board files? Could this code be relocated to some common area like [1]?
>>
>> That's the plan.
> 
> I look forward to seeing v2 then, since this would allow us not
> duplicating this code over and over again in the board-specific files
> of the customer R-Car3 targets.
Should be fixed by
http://patchwork.ozlabs.org/patch/1101744/

[...]

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 4/4] ARM: renesas: Configure DRAM size from ATF DT fragment

[Eugeniu Rosca]

Hi Marek,

On Mon, May 20, 2019 at 12:50:07AM +0200, Marek Vasut wrote:
> On 4/4/19 3:45 PM, Eugeniu Rosca wrote:
[..]
> >>> [..]
> >>>
> >>> Besides the above, I wonder why this patch duplicates code across three
> >>> board files? Could this code be relocated to some common area like [1]?
> >>
> >> That's the plan.
> > 
> > I look forward to seeing v2 then, since this would allow us not
> > duplicating this code over and over again in the board-specific files
> > of the customer R-Car3 targets.
> Should be fixed by
> http://patchwork.ozlabs.org/patch/1101744/

Thank you very much. We'll check it out later and report the results
(don't feel blocked however). Thanks again!

-- 
Best Regards,
Eugeniu.