ARM: imx6q_logic: Fix broken booting by moving fdt_addr_r address

The loading address is too close to the kernel address, so newer kernels
may overlap memory space, so loading the device tree may corrupt zImage.

This patch moves the fdt_addr_r to 0x18000000 which is also consistent
with some other i.MX6Q boards.

Signed-off-by: Adam Ford <aford173@gmail.com>

On Wed, Aug 19, 2020 at 02:11:08PM -0500, Adam Ford wrote:

> The loading address is too close to the kernel address, so newer kernels
> may overlap memory space, so loading the device tree may corrupt zImage.
> 
> This patch moves the fdt_addr_r to 0x18000000 which is also consistent
> with some other i.MX6Q boards.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> diff --git a/include/configs/imx6_logic.h b/include/configs/imx6_logic.h
> index 63662dd18d..58862e4c49 100644
> --- a/include/configs/imx6_logic.h
> +++ b/include/configs/imx6_logic.h
> @@ -34,7 +34,7 @@
>  	"script=boot.scr\0" \
>  	"image=zImage\0" \
>  	"bootm_size=0x10000000\0" \
> -	"fdt_addr_r=0x13000000\0" \
> +	"fdt_addr_r=0x18000000\0" \
>  	"ramdisk_addr_r=0x14000000\0" \
>  	"kernel_addr_r=" __stringify(CONFIG_LOADADDR) "\0" \
>  	"ramdisk_file=rootfs.cpio.uboot\0" \

Having the fdt above ramdisk is a bad idea.  To quote myself from
include/configs/ti_armv7_common.h:
 * We setup defaults based on constraints from the Linux kernel, which
 * should also be safe elsewhere.  We have the default load at 32MB into
 * DDR (for the kernel), FDT above 128MB (the maximum location for the
 * end of the kernel), and the ramdisk 512KB above that (allowing for
 * hopefully never seen large trees).  We say all of this must be within
 * the first 256MB as that will normally be within the kernel lowmem and
 * thus visible via bootm_size and we only run on platforms with 256MB
 * or more of memory.

With the fdt being placed above the ramdisk, it will get broken by a
sufficiently large distro ramdisk and that ends up being a very annoying
problem to debug.

And as Linus Walleij's article the other week[1] noted, the zImage will
relocate itself if there's overlap but the layout above avoids that.

[1]: https://people.kernel.org/linusw/how-the-arm32-linux-kernel-decompresses

On Thu, Aug 20, 2020 at 7:17 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Aug 19, 2020 at 02:11:08PM -0500, Adam Ford wrote:
>
> > The loading address is too close to the kernel address, so newer kernels
> > may overlap memory space, so loading the device tree may corrupt zImage.
> >
> > This patch moves the fdt_addr_r to 0x18000000 which is also consistent
> > with some other i.MX6Q boards.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > diff --git a/include/configs/imx6_logic.h b/include/configs/imx6_logic.h
> > index 63662dd18d..58862e4c49 100644
> > --- a/include/configs/imx6_logic.h
> > +++ b/include/configs/imx6_logic.h
> > @@ -34,7 +34,7 @@
> >       "script=boot.scr\0" \
> >       "image=zImage\0" \
> >       "bootm_size=0x10000000\0" \
> > -     "fdt_addr_r=0x13000000\0" \
> > +     "fdt_addr_r=0x18000000\0" \
> >       "ramdisk_addr_r=0x14000000\0" \
> >       "kernel_addr_r=" __stringify(CONFIG_LOADADDR) "\0" \
> >       "ramdisk_file=rootfs.cpio.uboot\0" \
>
> Having the fdt above ramdisk is a bad idea.  To quote myself from
> include/configs/ti_armv7_common.h:

I didn't look at the TIstuff, because I'm running iMX6 from NXP.  I
wonder if we could move this to a more obvious place.

>  * We setup defaults based on constraints from the Linux kernel, which
>  * should also be safe elsewhere.  We have the default load at 32MB into
>  * DDR (for the kernel), FDT above 128MB (the maximum location for the
>  * end of the kernel), and the ramdisk 512KB above that (allowing for
>  * hopefully never seen large trees).  We say all of this must be within
>  * the first 256MB as that will normally be within the kernel lowmem and
>  * thus visible via bootm_size and we only run on platforms with 256MB
>  * or more of memory.

This makes a lot of sense to me.

I'll run some tests with
loadaddr = 0x12000000 (same address as before)
fdt_addr_r = 0x14000000 (32MB after loadaddr)
ramdisk_addr_r 0x14080000  (512MB after fdt_addr_r)

If everythiing looks good, and the board boots again, I'll submit a V2 patch.

adam
>
> With the fdt being placed above the ramdisk, it will get broken by a
> sufficiently large distro ramdisk and that ends up being a very annoying
> problem to debug.
>
> And as Linus Walleij's article the other week[1] noted, the zImage will
> relocate itself if there's overlap but the layout above avoids that.

That's an interesting read.  My problem is that reading the fdt blows
away part of the zImage before the relocation and decompression phase,
so I need to allocate more space to the kernel.

adam
>
> [1]: https://people.kernel.org/linusw/how-the-arm32-linux-kernel-decompresses
>
> --
> Tom

On Thu, Aug 20, 2020 at 07:42:13AM -0500, Adam Ford wrote:
> On Thu, Aug 20, 2020 at 7:17 AM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Aug 19, 2020 at 02:11:08PM -0500, Adam Ford wrote:
> >
> > > The loading address is too close to the kernel address, so newer kernels
> > > may overlap memory space, so loading the device tree may corrupt zImage.
> > >
> > > This patch moves the fdt_addr_r to 0x18000000 which is also consistent
> > > with some other i.MX6Q boards.
> > >
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > >
> > > diff --git a/include/configs/imx6_logic.h b/include/configs/imx6_logic.h
> > > index 63662dd18d..58862e4c49 100644
> > > --- a/include/configs/imx6_logic.h
> > > +++ b/include/configs/imx6_logic.h
> > > @@ -34,7 +34,7 @@
> > >       "script=boot.scr\0" \
> > >       "image=zImage\0" \
> > >       "bootm_size=0x10000000\0" \
> > > -     "fdt_addr_r=0x13000000\0" \
> > > +     "fdt_addr_r=0x18000000\0" \
> > >       "ramdisk_addr_r=0x14000000\0" \
> > >       "kernel_addr_r=" __stringify(CONFIG_LOADADDR) "\0" \
> > >       "ramdisk_file=rootfs.cpio.uboot\0" \
> >
> > Having the fdt above ramdisk is a bad idea.  To quote myself from
> > include/configs/ti_armv7_common.h:
> 
> I didn't look at the TIstuff, because I'm running iMX6 from NXP.  I
> wonder if we could move this to a more obvious place.

Yes, something under doc/ that talks about best practices for addresses
would be a good idea.

> >  * We setup defaults based on constraints from the Linux kernel, which
> >  * should also be safe elsewhere.  We have the default load at 32MB into
> >  * DDR (for the kernel), FDT above 128MB (the maximum location for the
> >  * end of the kernel), and the ramdisk 512KB above that (allowing for
> >  * hopefully never seen large trees).  We say all of this must be within
> >  * the first 256MB as that will normally be within the kernel lowmem and
> >  * thus visible via bootm_size and we only run on platforms with 256MB
> >  * or more of memory.
> 
> This makes a lot of sense to me.
> 
> I'll run some tests with
> loadaddr = 0x12000000 (same address as before)
> fdt_addr_r = 0x14000000 (32MB after loadaddr)
> ramdisk_addr_r 0x14080000  (512MB after fdt_addr_r)
> 
> If everythiing looks good, and the board boots again, I'll submit a V2 patch.

How much memory does the platform have?  32MB for kernel + BSS isn't as
much room as it used to be when you're talking about a vanilla
multi_v7_defconfig build.  That's part of why I went for the whole
maximum allowed kernel size being between the kernel and fdt.

> 
> adam
> >
> > With the fdt being placed above the ramdisk, it will get broken by a
> > sufficiently large distro ramdisk and that ends up being a very annoying
> > problem to debug.
> >
> > And as Linus Walleij's article the other week[1] noted, the zImage will
> > relocate itself if there's overlap but the layout above avoids that.
> 
> That's an interesting read.  My problem is that reading the fdt blows
> away part of the zImage before the relocation and decompression phase,
> so I need to allocate more space to the kernel.

Yup.  I've run in to that before too which is why I went for as big of
gaps as possible in the TI stuff and pointed folks at it a time or two.

On Thu, Aug 20, 2020 at 8:11 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Aug 20, 2020 at 07:42:13AM -0500, Adam Ford wrote:
> > On Thu, Aug 20, 2020 at 7:17 AM Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Aug 19, 2020 at 02:11:08PM -0500, Adam Ford wrote:
> > >
> > > > The loading address is too close to the kernel address, so newer kernels
> > > > may overlap memory space, so loading the device tree may corrupt zImage.
> > > >
> > > > This patch moves the fdt_addr_r to 0x18000000 which is also consistent
> > > > with some other i.MX6Q boards.
> > > >
> > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > >
> > > > diff --git a/include/configs/imx6_logic.h b/include/configs/imx6_logic.h
> > > > index 63662dd18d..58862e4c49 100644
> > > > --- a/include/configs/imx6_logic.h
> > > > +++ b/include/configs/imx6_logic.h
> > > > @@ -34,7 +34,7 @@
> > > >       "script=boot.scr\0" \
> > > >       "image=zImage\0" \
> > > >       "bootm_size=0x10000000\0" \
> > > > -     "fdt_addr_r=0x13000000\0" \
> > > > +     "fdt_addr_r=0x18000000\0" \
> > > >       "ramdisk_addr_r=0x14000000\0" \
> > > >       "kernel_addr_r=" __stringify(CONFIG_LOADADDR) "\0" \
> > > >       "ramdisk_file=rootfs.cpio.uboot\0" \
> > >
> > > Having the fdt above ramdisk is a bad idea.  To quote myself from
> > > include/configs/ti_armv7_common.h:
> >
> > I didn't look at the TIstuff, because I'm running iMX6 from NXP.  I
> > wonder if we could move this to a more obvious place.
>
> Yes, something under doc/ that talks about best practices for addresses
> would be a good idea.
>
> > >  * We setup defaults based on constraints from the Linux kernel, which
> > >  * should also be safe elsewhere.  We have the default load at 32MB into
> > >  * DDR (for the kernel), FDT above 128MB (the maximum location for the
> > >  * end of the kernel), and the ramdisk 512KB above that (allowing for
> > >  * hopefully never seen large trees).  We say all of this must be within
> > >  * the first 256MB as that will normally be within the kernel lowmem and
> > >  * thus visible via bootm_size and we only run on platforms with 256MB
> > >  * or more of memory.
> >
> > This makes a lot of sense to me.
> >
> > I'll run some tests with
> > loadaddr = 0x12000000 (same address as before)
> > fdt_addr_r = 0x14000000 (32MB after loadaddr)
> > ramdisk_addr_r 0x14080000  (512MB after fdt_addr_r)
> >
> > If everythiing looks good, and the board boots again, I'll submit a V2 patch.
>
> How much memory does the platform have?  32MB for kernel + BSS isn't as
> much room as it used to be when you're talking about a vanilla
> multi_v7_defconfig build.  That's part of why I went for the whole
> maximum allowed kernel size being between the kernel and fdt.

Our i.MX6Q board has 2GB of DDR.

I just tested the above.  I'm ready to push a V2 patch unless you have
any other suggestions.


>
> >
> > adam
> > >
> > > With the fdt being placed above the ramdisk, it will get broken by a
> > > sufficiently large distro ramdisk and that ends up being a very annoying
> > > problem to debug.
> > >
> > > And as Linus Walleij's article the other week[1] noted, the zImage will
> > > relocate itself if there's overlap but the layout above avoids that.
> >
> > That's an interesting read.  My problem is that reading the fdt blows
> > away part of the zImage before the relocation and decompression phase,
> > so I need to allocate more space to the kernel.
>
> Yup.  I've run in to that before too which is why I went for as big of
> gaps as possible in the TI stuff and pointed folks at it a time or two.

This may be
>
> --
> Tom