[U-Boot] [PATCH] rpi: Adjust fdt_addr_r to a sane address

[U-Boot] [PATCH] rpi: Adjust fdt_addr_r to a sane address

[Alexander Graf]

Back in the old days, 0x100 was used as the address to pass the device tree
from firmware into the kernel. This has since changed to a more dynamic
location, so using 0x100 actually breaks more things than it helps with.

Let's move the device tree default location for distro boot to a more sane
place that gives us enough head room in low memory.

Reported-by: Tuomas Tynkkynen <tuomas@tuxera.com>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 include/configs/rpi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/configs/rpi.h b/include/configs/rpi.h
index 325e52a019..fcf7e0976b 100644
--- a/include/configs/rpi.h
+++ b/include/configs/rpi.h
@@ -124,7 +124,7 @@
 #define ENV_MEM_LAYOUT_SETTINGS \
 	"fdt_high=ffffffff\0" \
 	"initrd_high=ffffffff\0" \
-	"fdt_addr_r=0x00000100\0" \
+	"fdt_addr_r=0x01f00000\0" \
 	"pxefile_addr_r=0x00100000\0" \
 	"kernel_addr_r=0x01000000\0" \
 	"scriptaddr=0x02000000\0" \
-- 
2.12.3

[U-Boot] [PATCH] rpi: Adjust fdt_addr_r to a sane address

[Tuomas Tynkkynen]

Hi Alexander,

On Fri, 13 Apr 2018 17:49:00 +0200
Alexander Graf <agraf@suse.de> wrote:

[...]
> 
> diff --git a/include/configs/rpi.h b/include/configs/rpi.h
> index 325e52a019..fcf7e0976b 100644
> --- a/include/configs/rpi.h
> +++ b/include/configs/rpi.h
> @@ -124,7 +124,7 @@
>  #define ENV_MEM_LAYOUT_SETTINGS \
>  	"fdt_high=ffffffff\0" \
>  	"initrd_high=ffffffff\0" \
> -	"fdt_addr_r=0x00000100\0" \
> +	"fdt_addr_r=0x01f00000\0" \
>  	"pxefile_addr_r=0x00100000\0" \
>  	"kernel_addr_r=0x01000000\0" \
>  	"scriptaddr=0x02000000\0" \

Note that above the #define is a larger comment block that needs to be
updated as well. Also the other addresses also need updatingfor bigger
kernels on AArch64: https://patchwork.ozlabs.org/patch/777725/

Though now I double-checked that the smallest possible GPU-CPU memory
split is actually 64MB for the CPU, not 128M. So maybe something like:

         "kernel_addr_r=0x00080000\0" \
         "fdt_addr_r=0x02400000\0" \
         "scriptaddr=0x02500000\0" \
         "pxefile_addr_r=0x02600000\0" \
         "ramdisk_addr_r=0x02700000\0"

which would allow a kernel up to 36M, 1M for dtb, script and pxe files
each, and at least 25M for the initrd. Also I think giving up with the
constraint of locating the zImage high enough so that the kernel
decompressor doesn't need to relocate itself can be dropped. If the
boot speed of their Raspi matters that much, probably they wouldn't use
U-Boot in the first place.

What is the address that the RPi firmware loads its device tree to? I
hope that we don't have to worry about the positioning of that too...

- Tuomas

[U-Boot] [PATCH] rpi: Adjust fdt_addr_r to a sane address

[Tuomas Tynkkynen]

Hi Alexander,

What do you think of these patches? I haven't done testing with the
big kernels / DTBs yet, just that my previously-working kernel still
boots.

Tuomas Tynkkynen (2):
  rpi: Fix fdt_high & initrd_high for 64-bit builds
  rpi: Change load addresses to make more room for the kernel & DTB

 include/configs/rpi.h | 73 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 27 deletions(-)

-- 
2.16.3

[U-Boot] [PATCH 1/2] rpi: Fix fdt_high & initrd_high for 64-bit builds

[Tuomas Tynkkynen]

The magic value that disables relocation is dependent on the CPU word
size, so the current 'ffffffff' is doing the wrong thing on aarch64.

Signed-off-by: Tuomas Tynkkynen <tuomas@tuxera.com>
---
 include/configs/rpi.h | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/configs/rpi.h b/include/configs/rpi.h
index 325e52a019..f1189a27f3 100644
--- a/include/configs/rpi.h
+++ b/include/configs/rpi.h
@@ -91,6 +91,14 @@
 	"stdout=serial,vidconsole\0" \
 	"stderr=serial,vidconsole\0"
 
+#ifdef CONFIG_ARM64
+#define FDT_HIGH "ffffffffffffffff"
+#define INITRD_HIGH "ffffffffffffffff"
+#else
+#define FDT_HIGH "ffffffff"
+#define INITRD_HIGH "ffffffff"
+#endif
+
 /*
  * Memory layout for where various images get loaded by boot scripts:
  *
@@ -122,8 +130,8 @@
  *   for any boot script to be up to 1M, which is hopefully plenty.
  */
 #define ENV_MEM_LAYOUT_SETTINGS \
-	"fdt_high=ffffffff\0" \
-	"initrd_high=ffffffff\0" \
+	"fdt_high=" FDT_HIGH "\0" \
+	"initrd_high=" INITRD_HIGH "\0" \
 	"fdt_addr_r=0x00000100\0" \
 	"pxefile_addr_r=0x00100000\0" \
 	"kernel_addr_r=0x01000000\0" \
-- 
2.16.3

[U-Boot] [PATCH 2/2] rpi: Change load addresses to make more room for the kernel & DTB

[Tuomas Tynkkynen]

As of Linux 4.16, a multiplatform AArch64 kernel with our distro config
takes 26M. The current space reservation leaves only 17M for the kernel
and if it goes over it, the initrd gets overwritten when loading the
kernel from the filesystem.

A similar problem happens on ARMv7 with the DTBs taken from the
downstream Raspberry Pi foundation kernel. I guess they compile them
with DT overlay support enabled which grows them just enough.

Fix both of these problems by rewriting the memory map, which now allows
kernels to be up to 36M and DTBs up to 1M. Also the comment block was
kind of obsolete ever since the introduction of AArch64 support and the
firmware-loaded DTB doesn't get placed at 0x100 anymore either, so that
is fixed as well.

Signed-off-by: Tuomas Tynkkynen <tuomas@tuxera.com>
---
 include/configs/rpi.h | 61 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 25 deletions(-)

diff --git a/include/configs/rpi.h b/include/configs/rpi.h
index f1189a27f3..425c0c2e6d 100644
--- a/include/configs/rpi.h
+++ b/include/configs/rpi.h
@@ -104,39 +104,50 @@
  *
  * I suspect address 0 is used as the SMP pen on the RPi2, so avoid this.
  *
- * fdt_addr_r simply shouldn't overlap anything else. However, the RPi's
- *   binary firmware loads a DT to address 0x100, so we choose this address to
- *   match it. This allows custom boot scripts to pass this DT on to Linux
- *   simply by not over-writing the data at this address. When using U-Boot,
- *   U-Boot (and scripts it executes) typicaly ignore the DT loaded by the FW
- *   and loads its own DT from disk (triggered by boot.scr or extlinux.conf).
+ * Older versions of the boot firmware place the firmware-loaded DTB at 0x100,
+ * newer versions place it in high memory. So prevent U-Boot from doing its own
+ * DTB + initrd relocation so that we won't accidentally relocate the initrd
+ * over the firmware-loaded DTB and generally try to lay out things starting
+ * from the bottom of RAM.
  *
- * pxefile_addr_r can be pretty much anywhere that doesn't conflict with
- *   something else. Put it low in memory to avoid conflicts.
+ * kernel_addr_r has different constraints on ARM and Aarch64.  For 32-bit ARM,
+ * it must be within the first 128M of RAM in order for the kernel's
+ * CONFIG_AUTO_ZRELADDR option to work. The kernel itself will be decompressed
+ * to 0x8000 but the decompressor clobbers 0x4000-0x8000 as well. The
+ * decompressor also likes to relocate itself to right past the end of the
+ * decompressed kernel, so in total the sum of the compressed and and
+ * decompressed kernel needs to be reserved.
  *
- * kernel_addr_r must be within the first 128M of RAM in order for the
- *   kernel's CONFIG_AUTO_ZRELADDR option to work. Since the kernel will
- *   decompress itself to 0x8000 after the start of RAM, kernel_addr_r
- *   should not overlap that area, or the kernel will have to copy itself
- *   somewhere else before decompression. Similarly, the address of any other
- *   data passed to the kernel shouldn't overlap the start of RAM. Pushing
- *   this up to 16M allows for a sizable kernel to be decompressed below the
- *   compressed load address.
+ *   For Aarch64, the kernel image is uncompressed and must be loaded at
+ *   text_offset bytes (specified in the header of the Image) into a 2MB
+ *   boundary. The 'booti' command relocates the image if necessary. Linux uses
+ *   a default text_offset of 0x80000.  In summary, loading at 0x80000
+ *   satisfies all these constraints and reserving memory up to 0x02400000
+ *   permits fairly large (roughly 36M) kernels.
  *
- * scriptaddr can be pretty much anywhere that doesn't conflict with something
- *   else. Choosing 32M allows for the compressed kernel to be up to 16M.
+ * scriptaddr and pxefile_addr_r can be pretty much anywhere that doesn't
+ * conflict with something else. Reserving 1M for each of them at
+ * 0x02400000-0x02500000 and 0x02500000-0x02600000 should be plenty.
  *
- * ramdisk_addr_r simply shouldn't overlap anything else. Choosing 33M allows
- *   for any boot script to be up to 1M, which is hopefully plenty.
+ * On ARM, both the DTB and any possible initrd must be loaded such that they
+ * fit inside the lowmem mapping in Linux. In practice, this usually means not
+ * more than ~700M away from the start of the kernel image but this number can
+ * be larger OR smaller depending on e.g. the 'vmalloc=xxxM' command line
+ * parameter given to the kernel. So reserving memory from low to high
+ * satisfies this constraint again. Reserving 1M at 0x02600000-0x02700000 for
+ * the DTB leaves rest of the free RAM to the initrd starting at 0x02700000.
+ * Even with the smallest possible CPU-GPU memory split of the CPU getting
+ * only 64M, the remaining 25M starting at 0x02700000 should allow quite
+ * large initrds before they start colliding with U-Boot.
  */
 #define ENV_MEM_LAYOUT_SETTINGS \
 	"fdt_high=" FDT_HIGH "\0" \
 	"initrd_high=" INITRD_HIGH "\0" \
-	"fdt_addr_r=0x00000100\0" \
-	"pxefile_addr_r=0x00100000\0" \
-	"kernel_addr_r=0x01000000\0" \
-	"scriptaddr=0x02000000\0" \
-	"ramdisk_addr_r=0x02100000\0" \
+	"kernel_addr_r=0x00080000\0" \
+	"scriptaddr=0x02400000\0" \
+	"pxefile_addr_r=0x02500000\0" \
+	"fdt_addr_r=0x02600000\0" \
+	"ramdisk_addr_r=0x02700000\0"
 
 #define BOOT_TARGET_DEVICES(func) \
 	func(MMC, mmc, 0) \
-- 
2.16.3

[U-Boot] [PATCH] rpi: Adjust fdt_addr_r to a sane address

[Tuomas Tynkkynen]

On Fri, 20 Apr 2018 13:03:47 +0300
Tuomas Tynkkynen <tuomas@tuxera.com> wrote:

> Hi Alexander,
> 
> What do you think of these patches? I haven't done testing with the
> big kernels / DTBs yet, just that my previously-working kernel still
> boots.
> 

I've now verified that these two patches work as expected. I tested:

- RPi 1 running mainline kernel
- RPi 1 running downstream kernel
- RPi 3 running mainline kernel in 64-bit mode
- RPi 3 running mainline kernel in 32-bit mode
- RPi 3 running downstream kernel in 32-bit mode
- RPi 3B+ running downstream kernel in 32-bit mode

This was with the extlinux.conf distro boot. I don't know to what
extent these variables affect EFI boot.

> Tuomas Tynkkynen (2):
>   rpi: Fix fdt_high & initrd_high for 64-bit builds
>   rpi: Change load addresses to make more room for the kernel & DTB
> 

[U-Boot] [PATCH 1/2] rpi: Fix fdt_high & initrd_high for 64-bit builds

[Alexander Graf]

On 20.04.18 12:03, Tuomas Tynkkynen wrote:
> The magic value that disables relocation is dependent on the CPU word
> size, so the current 'ffffffff' is doing the wrong thing on aarch64.
> 
> Signed-off-by: Tuomas Tynkkynen <tuomas@tuxera.com>

The BCM283x series of SOCs is limited to 32bit address space, so I don't
quite see why the current (int)-1 is wrong?


Alex

> ---
>  include/configs/rpi.h | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/include/configs/rpi.h b/include/configs/rpi.h
> index 325e52a019..f1189a27f3 100644
> --- a/include/configs/rpi.h
> +++ b/include/configs/rpi.h
> @@ -91,6 +91,14 @@
>  	"stdout=serial,vidconsole\0" \
>  	"stderr=serial,vidconsole\0"
>  
> +#ifdef CONFIG_ARM64
> +#define FDT_HIGH "ffffffffffffffff"
> +#define INITRD_HIGH "ffffffffffffffff"
> +#else
> +#define FDT_HIGH "ffffffff"
> +#define INITRD_HIGH "ffffffff"
> +#endif
> +
>  /*
>   * Memory layout for where various images get loaded by boot scripts:
>   *
> @@ -122,8 +130,8 @@
>   *   for any boot script to be up to 1M, which is hopefully plenty.
>   */
>  #define ENV_MEM_LAYOUT_SETTINGS \
> -	"fdt_high=ffffffff\0" \
> -	"initrd_high=ffffffff\0" \
> +	"fdt_high=" FDT_HIGH "\0" \
> +	"initrd_high=" INITRD_HIGH "\0" \
>  	"fdt_addr_r=0x00000100\0" \
>  	"pxefile_addr_r=0x00100000\0" \
>  	"kernel_addr_r=0x01000000\0" \
> 

[U-Boot] [PATCH] rpi: Adjust fdt_addr_r to a sane address

[Alexander Graf]

On 14.04.18 20:04, Tuomas Tynkkynen wrote:
> Hi Alexander,
> 
> On Fri, 13 Apr 2018 17:49:00 +0200
> Alexander Graf <agraf@suse.de> wrote:
> 
> [...]
>>
>> diff --git a/include/configs/rpi.h b/include/configs/rpi.h
>> index 325e52a019..fcf7e0976b 100644
>> --- a/include/configs/rpi.h
>> +++ b/include/configs/rpi.h
>> @@ -124,7 +124,7 @@
>>  #define ENV_MEM_LAYOUT_SETTINGS \
>>  	"fdt_high=ffffffff\0" \
>>  	"initrd_high=ffffffff\0" \
>> -	"fdt_addr_r=0x00000100\0" \
>> +	"fdt_addr_r=0x01f00000\0" \
>>  	"pxefile_addr_r=0x00100000\0" \
>>  	"kernel_addr_r=0x01000000\0" \
>>  	"scriptaddr=0x02000000\0" \
> 
> Note that above the #define is a larger comment block that needs to be
> updated as well. Also the other addresses also need updatingfor bigger
> kernels on AArch64: https://patchwork.ozlabs.org/patch/777725/
> 
> Though now I double-checked that the smallest possible GPU-CPU memory
> split is actually 64MB for the CPU, not 128M. So maybe something like:
> 
>          "kernel_addr_r=0x00080000\0" \
>          "fdt_addr_r=0x02400000\0" \
>          "scriptaddr=0x02500000\0" \
>          "pxefile_addr_r=0x02600000\0" \
>          "ramdisk_addr_r=0x02700000\0"
> 
> which would allow a kernel up to 36M, 1M for dtb, script and pxe files
> each, and at least 25M for the initrd. Also I think giving up with the
> constraint of locating the zImage high enough so that the kernel
> decompressor doesn't need to relocate itself can be dropped. If the
> boot speed of their Raspi matters that much, probably they wouldn't use
> U-Boot in the first place.
> 
> What is the address that the RPi firmware loads its device tree to? I
> hope that we don't have to worry about the positioning of that too...

U-Boot> bdinfo
arch_number = 0x00000000
boot_params = 0x00000100
DRAM bank   = 0x00000000
-> start    = 0x00000000
-> size     = 0x3B400000
baudrate    = 115200 bps
TLB addr    = 0x3B3F0000
relocaddr   = 0x3B348000
reloc off   = 0x3B2C8000
irq_sp      = 0x3AF3E120
sp start    = 0x3AF3E120
Early malloc usage: 138 / 2000
fdt_blob = 000000003af3e130
U-Boot> print fdt_addr
fdt_addr=2effb300

So on boot the DT passed into U-Boot is either at ~750MB or close to the
top. I can't quite find any code that explains the difference in the two
variables. Either way, I guess firmware tries to put it reasonably high?

So as long as we keep the load addresses low enough, we should be safe.

Also fyi, I would like to switch to CONFIG_OF_BOARD by default, once I
added code that marks u-boot.bin as an "upstream kernel" for the RPi
firmware, because then newer RPi firmwares will pass us a fully upstream
compatible device tree [1] which we can then pass on to Linux as default.


Alex

[1] https://github.com/raspberrypi/firmware/issues/943

[U-Boot] [PATCH] rpi: Adjust fdt_addr_r to a sane address

[Tuomas Tynkkynen]

Hi Alexander,

On Thu, 24 May 2018 10:12:54 +0200
Alexander Graf <agraf@suse.de> wrote:

> On 14.04.18 20:04, Tuomas Tynkkynen wrote:
> > Hi Alexander,
> > 
> > On Fri, 13 Apr 2018 17:49:00 +0200
> > Alexander Graf <agraf@suse.de> wrote:
> > 
> > [...]  
> >>
> >> diff --git a/include/configs/rpi.h b/include/configs/rpi.h
> >> index 325e52a019..fcf7e0976b 100644
> >> --- a/include/configs/rpi.h
> >> +++ b/include/configs/rpi.h
> >> @@ -124,7 +124,7 @@
> >>  #define ENV_MEM_LAYOUT_SETTINGS \
> >>  	"fdt_high=ffffffff\0" \
> >>  	"initrd_high=ffffffff\0" \
> >> -	"fdt_addr_r=0x00000100\0" \
> >> +	"fdt_addr_r=0x01f00000\0" \
> >>  	"pxefile_addr_r=0x00100000\0" \
> >>  	"kernel_addr_r=0x01000000\0" \
> >>  	"scriptaddr=0x02000000\0" \  
> > 
> > Note that above the #define is a larger comment block that needs to be
> > updated as well. Also the other addresses also need updatingfor bigger
> > kernels on AArch64: https://patchwork.ozlabs.org/patch/777725/
> > 
> > Though now I double-checked that the smallest possible GPU-CPU memory
> > split is actually 64MB for the CPU, not 128M. So maybe something like:
> > 
> >          "kernel_addr_r=0x00080000\0" \
> >          "fdt_addr_r=0x02400000\0" \
> >          "scriptaddr=0x02500000\0" \
> >          "pxefile_addr_r=0x02600000\0" \
> >          "ramdisk_addr_r=0x02700000\0"
> > 
> > which would allow a kernel up to 36M, 1M for dtb, script and pxe files
> > each, and at least 25M for the initrd. Also I think giving up with the
> > constraint of locating the zImage high enough so that the kernel
> > decompressor doesn't need to relocate itself can be dropped. If the
> > boot speed of their Raspi matters that much, probably they wouldn't use
> > U-Boot in the first place.
> > 
> > What is the address that the RPi firmware loads its device tree to? I
> > hope that we don't have to worry about the positioning of that too...  
> 
> U-Boot> bdinfo
> arch_number = 0x00000000
> boot_params = 0x00000100
> DRAM bank   = 0x00000000
> -> start    = 0x00000000
> -> size     = 0x3B400000  
> baudrate    = 115200 bps
> TLB addr    = 0x3B3F0000
> relocaddr   = 0x3B348000
> reloc off   = 0x3B2C8000
> irq_sp      = 0x3AF3E120
> sp start    = 0x3AF3E120
> Early malloc usage: 138 / 2000
> fdt_blob = 000000003af3e130
> U-Boot> print fdt_addr
> fdt_addr=2effb300
> 
> So on boot the DT passed into U-Boot is either at ~750MB or close to the
> top. I can't quite find any code that explains the difference in the two
> variables. Either way, I guess firmware tries to put it reasonably high?

I believe fdt_addr is what is passed to the kernel and fdt_blob is what
U-Boot's device model is using.

> So as long as we keep the load addresses low enough, we should be safe.

Good.

> Also fyi, I would like to switch to CONFIG_OF_BOARD by default, once I
> added code that marks u-boot.bin as an "upstream kernel" for the RPi
> firmware, because then newer RPi firmwares will pass us a fully upstream
> compatible device tree [1] which we can then pass on to Linux as default.

Makes sense, but I guess needs some documentation. Also for existing things
like whether enable_uart=1 is needed or not.

> 
> Alex
> 
> [1] https://github.com/raspberrypi/firmware/issues/943

Thanks,
Tuomas

[U-Boot] [PATCH 1/2] rpi: Fix fdt_high & initrd_high for 64-bit builds

[Tuomas Tynkkynen]

Hi Alex,

On Thu, 24 May 2018 09:51:57 +0200
Alexander Graf <agraf@suse.de> wrote:

> On 20.04.18 12:03, Tuomas Tynkkynen wrote:
> > The magic value that disables relocation is dependent on the CPU word
> > size, so the current 'ffffffff' is doing the wrong thing on aarch64.
> > 
> > Signed-off-by: Tuomas Tynkkynen <tuomas@tuxera.com>  
> 
> The BCM283x series of SOCs is limited to 32bit address space, so I don't
> quite see why the current (int)-1 is wrong?
> 
> 

The comparison for the magic "don't relocate value" is done by parsing
the variable as ulong and then comparing to ~0. So on 64-bit, ffffffff
gets interpreted as literal 0xffffffff limit for the relocation (which
I think in practice is the same as not specifying initrd_high at all
since the end of DRAM is lower than that) instead.

[U-Boot] [PATCH 1/2] rpi: Fix fdt_high & initrd_high for 64-bit builds

[Alexander Graf]

On 24.05.18 16:57, Tuomas Tynkkynen wrote:
> Hi Alex,
> 
> On Thu, 24 May 2018 09:51:57 +0200
> Alexander Graf <agraf@suse.de> wrote:
> 
>> On 20.04.18 12:03, Tuomas Tynkkynen wrote:
>>> The magic value that disables relocation is dependent on the CPU word
>>> size, so the current 'ffffffff' is doing the wrong thing on aarch64.
>>>
>>> Signed-off-by: Tuomas Tynkkynen <tuomas@tuxera.com>  
>>
>> The BCM283x series of SOCs is limited to 32bit address space, so I don't
>> quite see why the current (int)-1 is wrong?
>>
>>
> 
> The comparison for the magic "don't relocate value" is done by parsing
> the variable as ulong and then comparing to ~0. So on 64-bit, ffffffff
> gets interpreted as literal 0xffffffff limit for the relocation (which
> I think in practice is the same as not specifying initrd_high at all
> since the end of DRAM is lower than that) instead.

Ouch, that logic is terrible. But it means your patch is correct. I'll
apply it.


Thanks,

Alex