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

[Tom Rini]

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.

> 
> 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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200820/1c6df960/attachment.sig>