diff mbox series

[1/7] arm: juno: Fix Juno address variables

Message ID 20200325144702.16288-2-andre.przywara@arm.com
State Superseded
Headers show
Series Arm Juno board OF_CONTROL upgrade | expand

Commit Message

Andre Przywara March 25, 2020, 2:46 p.m. UTC
The U-Boot documentation explains that variables ending with "_r" hold
addresses in DRAM, while those without that ending point to flash/ROM.
The default variables for the Juno board pointing to the kernel and DTB
load addresses were not complying with this scheme: they lack the
extension, but point to DRAM. This is particularly confusing since the
Juno board features parallel NOR flash, so there *is* a memory mapped
NOR address holding a DTB, for instance.

Fix the variables to use the proper names. On the way adjust the FDT
load address to be situated *before* the kernel, since users happened
to overwrite the DTB by the kernel clearing its .BSS section during
initialisation.

That fixes loading debug kernels, which happened to overwrite the DTB on
certain setups.

Signed-off-by: Andre Przywara <andre.przywara at arm.com>
Reviewed-by: Liviu Dudau <liviu.dudau at arm.com>
---
 include/configs/vexpress_aemv8a.h | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Comments

Tom Rini March 26, 2020, 2:38 a.m. UTC | #1
On Wed, Mar 25, 2020 at 02:46:56PM +0000, Andre Przywara wrote:

> The U-Boot documentation explains that variables ending with "_r" hold
> addresses in DRAM, while those without that ending point to flash/ROM.
> The default variables for the Juno board pointing to the kernel and DTB
> load addresses were not complying with this scheme: they lack the
> extension, but point to DRAM. This is particularly confusing since the
> Juno board features parallel NOR flash, so there *is* a memory mapped
> NOR address holding a DTB, for instance.
> 
> Fix the variables to use the proper names. On the way adjust the FDT
> load address to be situated *before* the kernel, since users happened
> to overwrite the DTB by the kernel clearing its .BSS section during
> initialisation.
> 
> That fixes loading debug kernels, which happened to overwrite the DTB on
> certain setups.
> 
> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> Reviewed-by: Liviu Dudau <liviu.dudau at arm.com>
[snip]
> -				"fdt_addr=0x83000000\0" \
> +				"fdt_addr_r=0x80000000\0" \
>  				"fdt_high=0xffffffffffffffff\0" \
>  				"initrd_high=0xffffffffffffffff\0" \

On a related note, using fdt_high=0xff... to disable relocation is a bad
idea and can lead to U-Boot knowing we have it at an invalid (unaligned)
location but doing nothing and causing problems down the chain.  Please
use bootm_size or similar (documented in the README, still) to limit
where the fdt can be.  Thanks!
Andre Przywara March 26, 2020, 4:14 p.m. UTC | #2
On 26/03/2020 02:38, Tom Rini wrote:

Hi,

> On Wed, Mar 25, 2020 at 02:46:56PM +0000, Andre Przywara wrote:
> 
>> The U-Boot documentation explains that variables ending with "_r" hold
>> addresses in DRAM, while those without that ending point to flash/ROM.
>> The default variables for the Juno board pointing to the kernel and DTB
>> load addresses were not complying with this scheme: they lack the
>> extension, but point to DRAM. This is particularly confusing since the
>> Juno board features parallel NOR flash, so there *is* a memory mapped
>> NOR address holding a DTB, for instance.
>>
>> Fix the variables to use the proper names. On the way adjust the FDT
>> load address to be situated *before* the kernel, since users happened
>> to overwrite the DTB by the kernel clearing its .BSS section during
>> initialisation.
>>
>> That fixes loading debug kernels, which happened to overwrite the DTB on
>> certain setups.
>>
>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>> Reviewed-by: Liviu Dudau <liviu.dudau at arm.com>
> [snip]
>> -				"fdt_addr=0x83000000\0" \
>> +				"fdt_addr_r=0x80000000\0" \
>>  				"fdt_high=0xffffffffffffffff\0" \
>>  				"initrd_high=0xffffffffffffffff\0" \
> 
> On a related note, using fdt_high=0xff... to disable relocation is a bad
> idea and can lead to U-Boot knowing we have it at an invalid (unaligned)
> location but doing nothing and causing problems down the chain.  Please
> use bootm_size or similar (documented in the README, still) to limit
> where the fdt can be.  Thanks!

Thanks, looks like I will drop this then. arm64 kernels before 4.2 had a
limit of placing the DTB with 512MB of the kernel image, but this has
been lifted since then. I might address this later shall people complain.

On a related note: I just see we use initrd_* variables here, where
there are more users of ramdisk_addr*. Shall I fix this here as well?
Seems like only ramdisk_addr* is mentioned in README.

Cheers,
Andre
Tom Rini March 26, 2020, 4:18 p.m. UTC | #3
On Thu, Mar 26, 2020 at 04:14:03PM +0000, Andr? Przywara wrote:
> On 26/03/2020 02:38, Tom Rini wrote:
> 
> Hi,
> 
> > On Wed, Mar 25, 2020 at 02:46:56PM +0000, Andre Przywara wrote:
> > 
> >> The U-Boot documentation explains that variables ending with "_r" hold
> >> addresses in DRAM, while those without that ending point to flash/ROM.
> >> The default variables for the Juno board pointing to the kernel and DTB
> >> load addresses were not complying with this scheme: they lack the
> >> extension, but point to DRAM. This is particularly confusing since the
> >> Juno board features parallel NOR flash, so there *is* a memory mapped
> >> NOR address holding a DTB, for instance.
> >>
> >> Fix the variables to use the proper names. On the way adjust the FDT
> >> load address to be situated *before* the kernel, since users happened
> >> to overwrite the DTB by the kernel clearing its .BSS section during
> >> initialisation.
> >>
> >> That fixes loading debug kernels, which happened to overwrite the DTB on
> >> certain setups.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> >> Reviewed-by: Liviu Dudau <liviu.dudau at arm.com>
> > [snip]
> >> -				"fdt_addr=0x83000000\0" \
> >> +				"fdt_addr_r=0x80000000\0" \
> >>  				"fdt_high=0xffffffffffffffff\0" \
> >>  				"initrd_high=0xffffffffffffffff\0" \
> > 
> > On a related note, using fdt_high=0xff... to disable relocation is a bad
> > idea and can lead to U-Boot knowing we have it at an invalid (unaligned)
> > location but doing nothing and causing problems down the chain.  Please
> > use bootm_size or similar (documented in the README, still) to limit
> > where the fdt can be.  Thanks!
> 
> Thanks, looks like I will drop this then. arm64 kernels before 4.2 had a
> limit of placing the DTB with 512MB of the kernel image, but this has
> been lifted since then. I might address this later shall people complain.

Thanks.
 
> On a related note: I just see we use initrd_* variables here, where
> there are more users of ramdisk_addr*. Shall I fix this here as well?
> Seems like only ramdisk_addr* is mentioned in README.

initrd_high is the other "do not relocate" variable.  For anything else,
yes, matching other platforms so that distro boot can be used at some
point (I assume it's not on Juno today) would be good.
Linus Walleij March 26, 2020, 10:29 p.m. UTC | #4
On Wed, Mar 25, 2020 at 3:47 PM Andre Przywara <andre.przywara at arm.com> wrote:

> The U-Boot documentation explains that variables ending with "_r" hold
> addresses in DRAM, while those without that ending point to flash/ROM.
> The default variables for the Juno board pointing to the kernel and DTB
> load addresses were not complying with this scheme: they lack the
> extension, but point to DRAM. This is particularly confusing since the
> Juno board features parallel NOR flash, so there *is* a memory mapped
> NOR address holding a DTB, for instance.
>
> Fix the variables to use the proper names. On the way adjust the FDT
> load address to be situated *before* the kernel, since users happened
> to overwrite the DTB by the kernel clearing its .BSS section during
> initialisation.
>
> That fixes loading debug kernels, which happened to overwrite the DTB on
> certain setups.
>
> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> Reviewed-by: Liviu Dudau <liviu.dudau at arm.com>

Makes perfect sense.
Reviewed-by: Linus Walleij <linus.walleij at linaro.org>

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/include/configs/vexpress_aemv8a.h b/include/configs/vexpress_aemv8a.h
index 9a9cec414c..edb08b0e68 100644
--- a/include/configs/vexpress_aemv8a.h
+++ b/include/configs/vexpress_aemv8a.h
@@ -138,35 +138,35 @@ 
 #define CONFIG_EXTRA_ENV_SETTINGS	\
 				"kernel_name=norkern\0"	\
 				"kernel_alt_name=Image\0"	\
-				"kernel_addr=0x80080000\0" \
+				"kernel_addr_r=0x80080000\0" \
 				"initrd_name=ramdisk.img\0"	\
-				"initrd_addr=0x84000000\0"	\
+				"initrd_addr_r=0x88000000\0"	\
 				"fdtfile=board.dtb\0" \
 				"fdt_alt_name=juno\0" \
-				"fdt_addr=0x83000000\0" \
+				"fdt_addr_r=0x80000000\0" \
 				"fdt_high=0xffffffffffffffff\0" \
 				"initrd_high=0xffffffffffffffff\0" \
 
 /* Copy the kernel and FDT to DRAM memory and boot */
-#define CONFIG_BOOTCOMMAND	"afs load ${kernel_name} ${kernel_addr} ; " \
+#define CONFIG_BOOTCOMMAND	"afs load ${kernel_name} ${kernel_addr_r} ;"\
 				"if test $? -eq 1; then "\
 				"  echo Loading ${kernel_alt_name} instead of "\
 				"${kernel_name}; "\
-				"  afs load ${kernel_alt_name} ${kernel_addr};"\
+				"  afs load ${kernel_alt_name} ${kernel_addr_r};"\
 				"fi ; "\
-				"afs load  ${fdtfile} ${fdt_addr} ; " \
+				"afs load ${fdtfile} ${fdt_addr_r} ;"\
 				"if test $? -eq 1; then "\
 				"  echo Loading ${fdt_alt_name} instead of "\
 				"${fdtfile}; "\
-				"  afs load ${fdt_alt_name} ${fdt_addr}; "\
+				"  afs load ${fdt_alt_name} ${fdt_addr_r}; "\
 				"fi ; "\
-				"fdt addr ${fdt_addr}; fdt resize; " \
-				"if afs load  ${initrd_name} ${initrd_addr} ; "\
+				"fdt addr ${fdt_addr_r}; fdt resize; " \
+				"if afs load  ${initrd_name} ${initrd_addr_r} ; "\
 				"then "\
-				"  setenv initrd_param ${initrd_addr}; "\
+				"  setenv initrd_param ${initrd_addr_r}; "\
 				"  else setenv initrd_param -; "\
 				"fi ; " \
-				"booti ${kernel_addr} ${initrd_param} ${fdt_addr}"
+				"booti ${kernel_addr_r} ${initrd_param} ${fdt_addr_r}"
 
 
 #elif CONFIG_TARGET_VEXPRESS64_BASE_FVP