ARM: boot: Obtain start of physical memory from DTB
diff mbox series

Message ID 20200121192741.20597-1-geert+renesas@glider.be
State New
Headers show
Series
  • ARM: boot: Obtain start of physical memory from DTB
Related show

Commit Message

Geert Uytterhoeven Jan. 21, 2020, 7:27 p.m. UTC
Currently, the start address of physical memory is obtained by masking
the program counter with a fixed mask of 0xf8000000.  This mask value
was chosen as a balance between the requirements of different platforms.
However, this does require that the start address of physical memory is
a multiple of 128 MiB, precluding booting Linux on platforms where this
requirement is not fulfilled.

Fix this limitation by obtaining the start address from the passed DTB
instead, if available.  Note that for now this is limited to DTBs passed
explicitly by the boot loader.  DTBs appended to a zImage or uImage are
not inspected.  Fall back to the traditional method when needed.

This allows to boot Linux on r7s9210/rza2mevb using the 64 MiB of SDRAM
on the RZA2MEVB sub board, which is located at 0x0C000000 (CS3 space),
i.e. not at a multiple of 128 MiB.

Suggested-by: Nicolas Pitre <nico@fluxnic.net>
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Against arm/for-next.

Tested with the following configurations:
  - zImage + DTB (r8a7791/koelsch): physical memory start address
    obtained from DT,
  - uImage + DTB (r8a73a4/ape6evm, r7s72100/rskrza1, r7s9210/rza2mevb):
    physical memory start address obtained from DT,
  - zImage with appended DTB (r8a7740/armadillo, sh73a0/kzm9g): physical
    memory start address obtained by masking, as before.

An appended DTB is currently processed after the start of physical
memory is obtained.  Hence obtaining that address from an appended DTB
requires moving/copying that copy.  Given the complexity w.r.t. the
"restart" label, and the lack of a need for me to support this, I didn't
implement that part.
---
 arch/arm/boot/compressed/Makefile            |  6 ++-
 arch/arm/boot/compressed/fdt_get_mem_start.c | 52 ++++++++++++++++++++
 arch/arm/boot/compressed/head.S              | 16 +++++-
 3 files changed, 72 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/boot/compressed/fdt_get_mem_start.c

Comments

Russell King - ARM Linux admin Jan. 21, 2020, 10:24 p.m. UTC | #1
On Tue, Jan 21, 2020 at 08:27:41PM +0100, Geert Uytterhoeven wrote:
> Currently, the start address of physical memory is obtained by masking
> the program counter with a fixed mask of 0xf8000000.  This mask value
> was chosen as a balance between the requirements of different platforms.
> However, this does require that the start address of physical memory is
> a multiple of 128 MiB, precluding booting Linux on platforms where this
> requirement is not fulfilled.
> 
> Fix this limitation by obtaining the start address from the passed DTB
> instead, if available.  Note that for now this is limited to DTBs passed
> explicitly by the boot loader.  DTBs appended to a zImage or uImage are
> not inspected.  Fall back to the traditional method when needed.
> 
> This allows to boot Linux on r7s9210/rza2mevb using the 64 MiB of SDRAM
> on the RZA2MEVB sub board, which is located at 0x0C000000 (CS3 space),
> i.e. not at a multiple of 128 MiB.
> 
> Suggested-by: Nicolas Pitre <nico@fluxnic.net>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Against arm/for-next.
> 
> Tested with the following configurations:
>   - zImage + DTB (r8a7791/koelsch): physical memory start address
>     obtained from DT,
>   - uImage + DTB (r8a73a4/ape6evm, r7s72100/rskrza1, r7s9210/rza2mevb):
>     physical memory start address obtained from DT,
>   - zImage with appended DTB (r8a7740/armadillo, sh73a0/kzm9g): physical
>     memory start address obtained by masking, as before.
> 
> An appended DTB is currently processed after the start of physical
> memory is obtained.  Hence obtaining that address from an appended DTB
> requires moving/copying that copy.  Given the complexity w.r.t. the
> "restart" label, and the lack of a need for me to support this, I didn't
> implement that part.
> ---
>  arch/arm/boot/compressed/Makefile            |  6 ++-
>  arch/arm/boot/compressed/fdt_get_mem_start.c | 52 ++++++++++++++++++++
>  arch/arm/boot/compressed/head.S              | 16 +++++-
>  3 files changed, 72 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm/boot/compressed/fdt_get_mem_start.c
> 
> diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
> index da599c3a11934332..bbfecd648a1a3b57 100644
> --- a/arch/arm/boot/compressed/Makefile
> +++ b/arch/arm/boot/compressed/Makefile
> @@ -86,12 +86,15 @@ libfdt_objs	:= $(addsuffix .o, $(basename $(libfdt)))
>  $(addprefix $(obj)/,$(libfdt) $(libfdt_hdrs)): $(obj)/%: $(srctree)/scripts/dtc/libfdt/%
>  	$(call cmd,shipped)
>  
> -$(addprefix $(obj)/,$(libfdt_objs) atags_to_fdt.o): \
> +$(addprefix $(obj)/,$(libfdt_objs) atags_to_fdt.o fdt_get_mem_start.o): \
>  	$(addprefix $(obj)/,$(libfdt_hdrs))
>  
>  ifeq ($(CONFIG_ARM_ATAG_DTB_COMPAT),y)
>  OBJS	+= $(libfdt_objs) atags_to_fdt.o
>  endif
> +ifeq ($(CONFIG_USE_OF),y)
> +OBJS	+= $(libfdt_objs) fdt_get_mem_start.o
> +endif
>  
>  targets       := vmlinux vmlinux.lds piggy_data piggy.o \
>  		 lib1funcs.o ashldi3.o bswapsdi2.o \
> @@ -116,6 +119,7 @@ CFLAGS_fdt.o := $(nossp-flags-y)
>  CFLAGS_fdt_ro.o := $(nossp-flags-y)
>  CFLAGS_fdt_rw.o := $(nossp-flags-y)
>  CFLAGS_fdt_wip.o := $(nossp-flags-y)
> +CFLAGS_fdt_get_mem_start.o := $(nossp-flags-y)
>  
>  ccflags-y := -fpic $(call cc-option,-mno-single-pic-base,) -fno-builtin -I$(obj)
>  asflags-y := -DZIMAGE
> diff --git a/arch/arm/boot/compressed/fdt_get_mem_start.c b/arch/arm/boot/compressed/fdt_get_mem_start.c
> new file mode 100644
> index 0000000000000000..2c5ac47f656317ee
> --- /dev/null
> +++ b/arch/arm/boot/compressed/fdt_get_mem_start.c
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <libfdt.h>
> +
> +static const void *getprop(const void *fdt, const char *node_path,
> +			   const char *property)
> +{
> +	int offset = fdt_path_offset(fdt, node_path);
> +
> +	if (offset == -FDT_ERR_NOTFOUND)
> +		return NULL;
> +
> +	return fdt_getprop(fdt, offset, property, NULL);
> +}
> +
> +static uint32_t get_addr_size(const void *fdt)
> +{
> +	const __be32 *addr_len = getprop(fdt, "/", "#address-cells");
> +
> +	if (!addr_len) {
> +		/* default */
> +		return 1;
> +	}
> +
> +	return fdt32_to_cpu(*addr_len);
> +}
> +
> +/*
> + * Get the start of physical memory
> + */
> +
> +unsigned long fdt_get_mem_start(const void *fdt)
> +{
> +	const __be32 *memory;
> +	uint32_t addr_size;
> +
> +	if (!fdt)
> +		return -1;
> +
> +	if (*(__be32 *)fdt != cpu_to_fdt32(FDT_MAGIC))
> +		return -1;
> +
> +	/* Find the first memory node */
> +	memory = getprop(fdt, "/memory", "reg");
> +	if (!memory)
> +		return -1;
> +
> +	/* There may be multiple cells on LPAE platforms */
> +	addr_size = get_addr_size(fdt);
> +
> +	return fdt32_to_cpu(memory[addr_size - 1]);
> +}
> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> index 927f5dc413d7dff2..cb4e6a84b156c204 100644
> --- a/arch/arm/boot/compressed/head.S
> +++ b/arch/arm/boot/compressed/head.S
> @@ -235,8 +235,20 @@ not_angel:
>  		.text
>  
>  #ifdef CONFIG_AUTO_ZRELADDR
> +#ifdef CONFIG_USE_OF
>  		/*
> -		 * Find the start of physical memory.  As we are executing
> +		 * Find the start of physical memory.
> +		 * Try the passed DTB first, if available.
> +		 */
> +		mov	r0, r8
> +		bl	fdt_get_mem_start

NAK.  You don't have a stack setup here.  The fact this works for you
is merely due to "sp" happening to be pointing somewhere sane, but
there is no such requirement.  Therefore, this is fragile.

> +		mov	r4, r0
> +		cmn	r0, #1
> +		bne	1f
> +#endif
> +
> +		/*
> +		 * Fall back to the traditional method.  As we are executing
>  		 * without the MMU on, we are in the physical address space.
>  		 * We just need to get rid of any offset by aligning the
>  		 * address.
> @@ -254,6 +266,8 @@ not_angel:
>  		 */
>  		mov	r4, pc
>  		and	r4, r4, #0xf8000000
> +
> +1:
>  		/* Determine final kernel image address. */
>  		add	r4, r4, #TEXT_OFFSET
>  #else
> -- 
> 2.17.1
> 
>
Nicolas Pitre Jan. 21, 2020, 10:44 p.m. UTC | #2
On Tue, 21 Jan 2020, Geert Uytterhoeven wrote:

> Currently, the start address of physical memory is obtained by masking
> the program counter with a fixed mask of 0xf8000000.  This mask value
> was chosen as a balance between the requirements of different platforms.
> However, this does require that the start address of physical memory is
> a multiple of 128 MiB, precluding booting Linux on platforms where this
> requirement is not fulfilled.
> 
> Fix this limitation by obtaining the start address from the passed DTB
> instead, if available.  Note that for now this is limited to DTBs passed
> explicitly by the boot loader.  DTBs appended to a zImage or uImage are
> not inspected.  Fall back to the traditional method when needed.
> 
> This allows to boot Linux on r7s9210/rza2mevb using the 64 MiB of SDRAM
> on the RZA2MEVB sub board, which is located at 0x0C000000 (CS3 space),
> i.e. not at a multiple of 128 MiB.
> 
> Suggested-by: Nicolas Pitre <nico@fluxnic.net>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Against arm/for-next.
> 
> Tested with the following configurations:
>   - zImage + DTB (r8a7791/koelsch): physical memory start address
>     obtained from DT,
>   - uImage + DTB (r8a73a4/ape6evm, r7s72100/rskrza1, r7s9210/rza2mevb):
>     physical memory start address obtained from DT,
>   - zImage with appended DTB (r8a7740/armadillo, sh73a0/kzm9g): physical
>     memory start address obtained by masking, as before.
> 
> An appended DTB is currently processed after the start of physical
> memory is obtained.  Hence obtaining that address from an appended DTB
> requires moving/copying that copy.  Given the complexity w.r.t. the
> "restart" label, and the lack of a need for me to support this, I didn't
> implement that part.

Well, not exactly. You don't have to move anything. But more on that 
later.

One important detail: you didn't set up the stack pointer. That means 
you're relying on whatever value left in sp by the bootloader. If you're 
lucky that might be fine, but it isn't a good idea to leave things to 
luck.

Before calling the C code, you should probably do:

	adr	r0, LC0
	ldr	r1, [r0]
	ldr	sp, [r0, #28]
	sub	r0, r0, r1
	add	sp, sp, r0

But if there is an appended dtb then you'll overwrite it. So you need 
to look for one and account for its size.
Something like this:

	adr	r0, LC0
	ldr	r1, [r0]		@ get absolute LC0
	ldr	sp, [r0, #28]		@ get stack location
	sub	r1, r0, r1		@ compute relocation offset
	add	sp, sp, r1		@ apply relocation

#ifdef CONFIG_ARM_APPENDED_DTB
	/*
	 * Look for an appended DTB. If found, use it and
	 * move stack away from it.
	 */
	ldr	r6, [r0, #12]		@ get &_end
	add	r6, r6, r1		@ relocate it
	ldmia	r6, {r0, r5}		@ get DTB signature and size
#ifndef __ARMEB__
	ldr	r1, =0xedfe0dd0         @ sig is 0xd00dfeed big endian
	/* convert DTB size to little endian */
	eor     r2, r5, r5, ror #16
	bic     r2, r2, #0x00ff0000
	mov     r5, r5, ror #8
	eor     r5, r5, r2, lsr #8
#else
	ldr	r1, =0xd00dfeed
#endif
	cmp	r0, r1			@ do we have a DTB there?
	moveq	r8, r0			@ use it if so
	addeq	sp, sp, r5		@ and move stack above it
#endif

	bl	fdt_get_mem_start
	...

This is a little involved but there is no way around that for a safe 
stack. Benefit is that you get appended DTB support with a single 
additional instruction.

Also one minor nit:

> +		bl	fdt_get_mem_start
> +		mov	r4, r0
> +		cmn	r0, #1

Please just use "cmp r0 #-1" here. The assembler will convert it for 
you.


Nicolas
Geert Uytterhoeven Jan. 27, 2020, 2:08 p.m. UTC | #3
Hi Nicolas,

On Tue, Jan 21, 2020 at 11:44 PM Nicolas Pitre <nico@fluxnic.net> wrote:
> On Tue, 21 Jan 2020, Geert Uytterhoeven wrote:
> > Currently, the start address of physical memory is obtained by masking
> > the program counter with a fixed mask of 0xf8000000.  This mask value
> > was chosen as a balance between the requirements of different platforms.
> > However, this does require that the start address of physical memory is
> > a multiple of 128 MiB, precluding booting Linux on platforms where this
> > requirement is not fulfilled.
> >
> > Fix this limitation by obtaining the start address from the passed DTB
> > instead, if available.  Note that for now this is limited to DTBs passed
> > explicitly by the boot loader.  DTBs appended to a zImage or uImage are
> > not inspected.  Fall back to the traditional method when needed.
> >
> > This allows to boot Linux on r7s9210/rza2mevb using the 64 MiB of SDRAM
> > on the RZA2MEVB sub board, which is located at 0x0C000000 (CS3 space),
> > i.e. not at a multiple of 128 MiB.
> >
> > Suggested-by: Nicolas Pitre <nico@fluxnic.net>
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > Against arm/for-next.
> >
> > Tested with the following configurations:
> >   - zImage + DTB (r8a7791/koelsch): physical memory start address
> >     obtained from DT,
> >   - uImage + DTB (r8a73a4/ape6evm, r7s72100/rskrza1, r7s9210/rza2mevb):
> >     physical memory start address obtained from DT,
> >   - zImage with appended DTB (r8a7740/armadillo, sh73a0/kzm9g): physical
> >     memory start address obtained by masking, as before.
> >
> > An appended DTB is currently processed after the start of physical
> > memory is obtained.  Hence obtaining that address from an appended DTB
> > requires moving/copying that copy.  Given the complexity w.r.t. the
> > "restart" label, and the lack of a need for me to support this, I didn't
> > implement that part.
>
> Well, not exactly. You don't have to move anything. But more on that
> later.
>
> One important detail: you didn't set up the stack pointer. That means
> you're relying on whatever value left in sp by the bootloader. If you're
> lucky that might be fine, but it isn't a good idea to leave things to
> luck.
>
> Before calling the C code, you should probably do:
>
>         adr     r0, LC0
>         ldr     r1, [r0]
>         ldr     sp, [r0, #28]
>         sub     r0, r0, r1
>         add     sp, sp, r0
>
> But if there is an appended dtb then you'll overwrite it. So you need
> to look for one and account for its size.

Thank you very much for the very constructive feedback!

> Something like this:
>
>         adr     r0, LC0
>         ldr     r1, [r0]                @ get absolute LC0
>         ldr     sp, [r0, #28]           @ get stack location
>         sub     r1, r0, r1              @ compute relocation offset
>         add     sp, sp, r1              @ apply relocation
>
> #ifdef CONFIG_ARM_APPENDED_DTB
>         /*
>          * Look for an appended DTB. If found, use it and
>          * move stack away from it.
>          */
>         ldr     r6, [r0, #12]           @ get &_end

[r0, #12] is actually &_edata, not &_end.

>         add     r6, r6, r1              @ relocate it
>         ldmia   r6, {r0, r5}            @ get DTB signature and size
> #ifndef __ARMEB__
>         ldr     r1, =0xedfe0dd0         @ sig is 0xd00dfeed big endian
>         /* convert DTB size to little endian */
>         eor     r2, r5, r5, ror #16
>         bic     r2, r2, #0x00ff0000
>         mov     r5, r5, ror #8
>         eor     r5, r5, r2, lsr #8
> #else
>         ldr     r1, =0xd00dfeed
> #endif
>         cmp     r0, r1                  @ do we have a DTB there?
>         moveq   r8, r0                  @ use it if so

moveq r8, r6

>         addeq   sp, sp, r5              @ and move stack above it

Care must be taken to keep sp 64-bit aligned.

> #endif
>
>         bl      fdt_get_mem_start
>         ...
>
> This is a little involved but there is no way around that for a safe
> stack. Benefit is that you get appended DTB support with a single
> additional instruction.

True.

> Also one minor nit:
>
> > +             bl      fdt_get_mem_start
> > +             mov     r4, r0
> > +             cmn     r0, #1
>
> Please just use "cmp r0 #-1" here. The assembler will convert it for
> you.

OK, thanks.

Sent v2...

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Patch
diff mbox series

diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index da599c3a11934332..bbfecd648a1a3b57 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -86,12 +86,15 @@  libfdt_objs	:= $(addsuffix .o, $(basename $(libfdt)))
 $(addprefix $(obj)/,$(libfdt) $(libfdt_hdrs)): $(obj)/%: $(srctree)/scripts/dtc/libfdt/%
 	$(call cmd,shipped)
 
-$(addprefix $(obj)/,$(libfdt_objs) atags_to_fdt.o): \
+$(addprefix $(obj)/,$(libfdt_objs) atags_to_fdt.o fdt_get_mem_start.o): \
 	$(addprefix $(obj)/,$(libfdt_hdrs))
 
 ifeq ($(CONFIG_ARM_ATAG_DTB_COMPAT),y)
 OBJS	+= $(libfdt_objs) atags_to_fdt.o
 endif
+ifeq ($(CONFIG_USE_OF),y)
+OBJS	+= $(libfdt_objs) fdt_get_mem_start.o
+endif
 
 targets       := vmlinux vmlinux.lds piggy_data piggy.o \
 		 lib1funcs.o ashldi3.o bswapsdi2.o \
@@ -116,6 +119,7 @@  CFLAGS_fdt.o := $(nossp-flags-y)
 CFLAGS_fdt_ro.o := $(nossp-flags-y)
 CFLAGS_fdt_rw.o := $(nossp-flags-y)
 CFLAGS_fdt_wip.o := $(nossp-flags-y)
+CFLAGS_fdt_get_mem_start.o := $(nossp-flags-y)
 
 ccflags-y := -fpic $(call cc-option,-mno-single-pic-base,) -fno-builtin -I$(obj)
 asflags-y := -DZIMAGE
diff --git a/arch/arm/boot/compressed/fdt_get_mem_start.c b/arch/arm/boot/compressed/fdt_get_mem_start.c
new file mode 100644
index 0000000000000000..2c5ac47f656317ee
--- /dev/null
+++ b/arch/arm/boot/compressed/fdt_get_mem_start.c
@@ -0,0 +1,52 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <libfdt.h>
+
+static const void *getprop(const void *fdt, const char *node_path,
+			   const char *property)
+{
+	int offset = fdt_path_offset(fdt, node_path);
+
+	if (offset == -FDT_ERR_NOTFOUND)
+		return NULL;
+
+	return fdt_getprop(fdt, offset, property, NULL);
+}
+
+static uint32_t get_addr_size(const void *fdt)
+{
+	const __be32 *addr_len = getprop(fdt, "/", "#address-cells");
+
+	if (!addr_len) {
+		/* default */
+		return 1;
+	}
+
+	return fdt32_to_cpu(*addr_len);
+}
+
+/*
+ * Get the start of physical memory
+ */
+
+unsigned long fdt_get_mem_start(const void *fdt)
+{
+	const __be32 *memory;
+	uint32_t addr_size;
+
+	if (!fdt)
+		return -1;
+
+	if (*(__be32 *)fdt != cpu_to_fdt32(FDT_MAGIC))
+		return -1;
+
+	/* Find the first memory node */
+	memory = getprop(fdt, "/memory", "reg");
+	if (!memory)
+		return -1;
+
+	/* There may be multiple cells on LPAE platforms */
+	addr_size = get_addr_size(fdt);
+
+	return fdt32_to_cpu(memory[addr_size - 1]);
+}
diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
index 927f5dc413d7dff2..cb4e6a84b156c204 100644
--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -235,8 +235,20 @@  not_angel:
 		.text
 
 #ifdef CONFIG_AUTO_ZRELADDR
+#ifdef CONFIG_USE_OF
 		/*
-		 * Find the start of physical memory.  As we are executing
+		 * Find the start of physical memory.
+		 * Try the passed DTB first, if available.
+		 */
+		mov	r0, r8
+		bl	fdt_get_mem_start
+		mov	r4, r0
+		cmn	r0, #1
+		bne	1f
+#endif
+
+		/*
+		 * Fall back to the traditional method.  As we are executing
 		 * without the MMU on, we are in the physical address space.
 		 * We just need to get rid of any offset by aligning the
 		 * address.
@@ -254,6 +266,8 @@  not_angel:
 		 */
 		mov	r4, pc
 		and	r4, r4, #0xf8000000
+
+1:
 		/* Determine final kernel image address. */
 		add	r4, r4, #TEXT_OFFSET
 #else