Hi Leo,

On 21 April 2016 at 21:20, Leo Yan <leo.yan@linaro.org> wrote:
On Tue, Apr 19, 2016 at 11:13:42AM +0100, Peter Griffin wrote:


> So I think if the memory has to be reserved in this way (via the memory
> node),
> the correct (although not pretty) way to do it will be for U-Boot to
> represent the dram banks
> as they are currently being described in your DT patch. That way the memory
> DT
> node will be correctly re-generated based on U-Boots view of the dram banks
> when booting the kernel. Something like this: -
> diff --git a/board/hisilicon/hikey/hikey.c b/board/hisilicon/hikey/hikey.c
> index 3b484a9..8570972 100644
> --- a/board/hisilicon/hikey/hikey.c
> +++ b/board/hisilicon/hikey/hikey.c
> @@ -71,6 +71,8 @@ U_BOOT_DEVICES(hi6220_gpios) = {
> +
>  static const struct pl01x_serial_platdata serial_platdata = {
>         .base = HI6220_UART0_BASE,
> @@ -87,6 +89,7 @@ U_BOOT_DEVICE(hikey_seriala) = {
>         .name = "serial_pl01x",
>         .platdata = &serial_platdata,
>  };
> +#endif
>  static struct mm_region hikey_mem_map[] = {
>         {
> @@ -407,8 +410,35 @@ int dram_init(void)
>  void dram_init_banksize(void)
>  {
> +       /*
> +        * Reserve below regions from DT memory node (which gets generated
> +        * by U-Boot from the dram banks before booting the kernel.
> +        *
> +        *  0x05e0,0000 - 0x05ef,ffff: MCU firmware runtime using
> +        *  0x05f0,1000 - 0x05f0,1fff: Reboot reason
> +        *  0x06df,f000 - 0x06df,ffff: Mailbox message data
> +        *  0x0740,f000 - 0x0740,ffff: MCU firmware section
> +        *  0x21f0,0000 - 0x21ff,ffff: pstore/ramoops buffer
> +        *  0x3e00,0000 - 0x3fff,ffff: OP-TEE
> +       */
> +
>         gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
> -       gd->bd->bi_dram[0].size = PHYS_SDRAM_1_SIZE;
> +       gd->bd->bi_dram[0].size = 0x05e00000;
> +
> +       gd->bd->bi_dram[1].start = 0x05f00000;
> +       gd->bd->bi_dram[1].size = 0x00001000;
> +
> +       gd->bd->bi_dram[2].start = 0x05f02000;
> +       gd->bd->bi_dram[2].size = 0x00efd000;
> +
> +       gd->bd->bi_dram[3].start = 0x06e00000;
> +       gd->bd->bi_dram[3].size = 0x0060f000;
> +
> +       gd->bd->bi_dram[4].start = 0x07410000;
> +       gd->bd->bi_dram[4].size = 0x1aaf0000;
> +
> +       gd->bd->bi_dram[5].start = 0x22000000;
> +       gd->bd->bi_dram[5].size = 0x1c000000;
>  }

If so, then later if we need reserve more memory regions then we need
maintain in 3 places: kernel's dts, uboot and uefi :(

Yes it is definitely not ideal. Although if these locations are mainly dictated
by the mcu firmware over which we have no control. Hopefully they won't
change very often.

> I have a couple of questions though: -
> 1) Why can't we use memreserve or reserved-memory properties for some of
> these
> (especially for things like reboot-reason / mbox buf)? I can understand

By using memreserve, kernel will map the memory node as usual but just
don't allocte it from memory block. So if later other driver module
maps it, then that means mapping the reserved region twice. Mark
reminded this will introduce cache alias issue [1].

For reserved-memory, actually EPAPR has not defined this node.

[1] http://archive.arm.linux.org.uk/lurker/message/20160118.154259.eebaf8cd.en.html
[2] http://archive.arm.linux.org.uk/lurker/message/20151105.161315.cc3c99f5.en.html

Thanks for the links, that was an interesting read and actually has implications
for some of our ST co-processor / mailbox drivers that are being upstreamed,
so thanks for sharing it :)

> there maybe reasons for reserving op-tee memory via the memory node,  but
> I'm
> not so sure about some of the other reservations.

Due Hi6220 there have some hardcode to reserve some memory regions
which used by MCU firmware, which is usually is used by power
management related stuffs.

> FYI U-Boot already correctly
> handles memreserve regions found in the DT (e.g. it won't place a initrd,
> or fdt blob
> in a reserved region).

Thanks for sharing this.

> 2) If we do have to have some/all memory carveout regions represented in
> the memory node,
> do we have to have these reserved regions spread throughout the memory map?
> It would be much tidier to have one chunk for example 64Mb which is
> reserved at the end of RAM. I guess
> the mcuimage is statically linked by hisilicon at the address currently
> reserved for it, but
> others like mbox-buf, reboot reason etc presumably could be moved?

mbox-buf also is used by MCU :)

MCU will reserve memory regions for itself code and data sections, but
it also reserve mbox-buf which used to communicate message between CA53
and MCU.

Ah OK.

> This would be similar to how tegra U-Boot is operating (which is also
> taking the same
> approach as I have described here for overcoming this problem), and has
> some memory
> reserved for the secure monitor which is not described to the kernel DT
> memory node.
> See
> http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/mach-tegra/board2.c;h=ac274e17e8bd7deb7e3d9c7e392a7a6bbfeeab89;hb=HEAD#l282
> and in particular the comment here
> http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/mach-tegra/board2.c;h=ac274e17e8bd7deb7e3d9c7e392a7a6bbfeeab89;hb=HEAD#l337
> > > This should also help with detecting if we are a 1gb/2gb board etc.
> >
> > Could do this by setting cmdline's mem=XXX, so can remain intact for
> > DTB? Finally we can only maintain DTB in only one place (in kernel).
> >
> I can certainly see why that would be preferable, but the U-Boot project
> seems to import copies of the DT from the kernel into the U-Boot tree.
> Also there can be some variation, as my HiKey U-Boot  DT patches show
> for example the "arm,pl011" UART needs an extra <clock> DT property
> in U-Boot which isn't present in the kernel DT to work. So in short I'm not
> sure it is possible to only have it in one place.

So does U-boot also need use DT to initialize uart? If so, could we
change DT documentation in kernel so can describe hardware properly?

Upstream U-Boot currently doesn't, but when this set
https://www.mail-archive.com/u-boot@lists.denx.de/msg210314.html gets
merged it will. The plan would be to progressively move over so that
all / most U-Boot drivers use DT for configuration.

> This is one reason for wanting the DT to merge in the upstream kernel
> before importing and converting U-Boot to be configured from DT. That
> way we can ensure the DT bindings will match exactly for the hi6220
> SoC devices.

From my understanding, DTB is a description for hardware. So it should
be consistent for one specific platform.

Yes that is my understanding as well, I'm not sure why U-Boot has deviated
from the kernel bindings in this case. Hopefully it will be the only DT difference
and if we add a better clock driver into U-Boot we could probably eliminate
the need for the extra property and be totally aligned.  As most of the U-Boot
driver / glue code is stuff I wrote, I can make the HiKey bindings match
exactly to the kernel ones.

>From ARMv8 acutally we have good chance to unify booting sequence (ARMv7
is hard to unify booting sequence due hardware have not been unified,
some SoCs can directly run in secure world but some will switch to normal

So I checked Juno's code in U-boot; it uses your suggested method to
carve out reserved memory from memory bank but not really use dtb's
memory node.

By describing the carveout in the U-Boot memory banks, it ends up being
described in the memory node from the kernels DT point of view, because
as you discovered the memory node gets overwritten in arch_fixup_fdt()
based on the dram_bank layout.

I sent a patch here: -

Which adjusts the HiKey memory layout to be aligned with your patch (and
what will become the mainline kernel memory node in v4.7 DT). I was unsure
if it was the correct approach, but it has now been reviewed by Tom Rini the
U-Boot maintainer. So if he is happy...so am I.

Also thanks for your help in diagnosing and debugging the issue :)