| Message ID | 20180413154900.84208-1-agraf@suse.de |
|---|---|
| State | Accepted |
| Commit | d295c3ec3543e697b6f9f077f52877e081db4c6f |
| Headers | show |
| Series | rpi: Adjust fdt_addr_r to a sane address | expand |
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
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(-)
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 >
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" \ >
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
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
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.
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
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" \
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(-)