[PATCH] ARM: imx6q_logic: Fix broken booting by moving fdt_addr_r address

[Adam Ford]

On Thu, Aug 20, 2020 at 8:11 AM Tom Rini <trini at 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 at 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 at 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