Hi Leo,

On 13 April 2016 at 09:42, Leo Yan <leo.yan@linaro.org> wrote:

On Wed, Apr 13, 2016 at 08:07:23AM +0100, Peter Griffin wrote:
> Hi Leo / all,
>
> Sorry I'm coming to this a bit late as I was away for last 2 weeks.
>
> On 9 April 2016 at 15:25, Leo Yan <leo.yan@linaro.org> wrote:
>
> > On Fri, Apr 08, 2016 at 02:04:57PM +0800, Shawn Guo wrote:
> > > On Fri, Apr 8, 2016 at 12:14 AM, Leo Yan <leo.yan@linaro.org> wrote:
> > > > On Thu, Apr 07, 2016 at 09:03:44AM -0700, John Stultz wrote:
> > >
> > > <snip>
> > >
> > > > > So is it just 4.1 that is having problems? And if so, how critical is
> > > > > it to have 4.1? I'm currently working to move the hikey AOSP side to
> > > > > 4.4, so I'd like to understand the need better so I can prioritize
> > > > > this issue.
> > > >
> > > > Now 4.1 has problem. There have no any output from uart0/3 after
> > > > uboot jump to kernel.
> > >
> > > I think there are two problems with hi6220-hikey.dts on 4.1 branch
> > > when booting with U-Boot. Firstly, the 0x7400000 bytes memreserve at
> > > 0x0 is buggy, since we use reserved-memory node to reserve parts of
> > > that memory region. We are fine with UEFI, because UEFI doesn't use
> > > /memreserve/ in DTB. Secondly, the last 16MiB memory is marked as
> > > secured for TrustZone by ATF. If kernel tries to use that section of
> > > memory, we will have problem. U-Boot works around this by coding the
> > > physical memory size as 1024 - 16 = 1008 MiB. So to get the kernel
> > > work with U-Boot, we only need to remove the /memreserve/ line below
> > > from hi6220-hikey.dts.
> > >
> > > /memreserve/ 0x00000000 0x07400000;
> > >
> > > But a better change should be something like below, IMO.
> >
> > Thanks for hints, Shawn. For 4.1's boot failure, it does related with
> > /memreserve/. Below two patches are to fix booting failure: one
> > patch is for kernel, I also think this patch can fix kernel 3.18's
> > issue. Another patch is to fix uboot so that kernel can see correct
> > info of memory regions:
> >
> > Kernel's patch:
> >
> > ---8<---
> >
> > From 24a5e0152eea8d273b8679f61c155f30a077da06 Mon Sep 17 00:00:00 2001
> > From: Leo Yan <leo.yan@linaro.org>
> > Date: Sat, 9 Apr 2016 21:06:05 +0800
> > Subject: [PATCH] arm64: dts: polish reservation memory regions
> >
> > Now in dts it combines /memreserve/ and reserved-memory two methods to
> > reserve memory regions; so finally it introduce panic which caused by
> > allocate zero page in early_init().
> >
> > This will not introduce any issue when use UEFI, due EFI stub will
> > remove /memreserve/ from DTB; but it will work with u-boot. So polish
> > dts to use memory node to carve memory regions out.
> >
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> > arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 49
> > +++++++++++---------------
> > 1 file changed, 20 insertions(+), 29 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > index 36d300f..5de8acb1 100644
> > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > @@ -7,8 +7,6 @@
> >
> > /dts-v1/;
> >
> > -/memreserve/ 0x00000000 0x07400000;
> > -
> > #include "hikey-pinctrl.dtsi"
> > #include "hikey-gpio.dtsi"
> > #include "hi6220.dtsi"
> > @@ -36,36 +34,29 @@
> > linux,initrd-end = <0x0 0x0b600000>;
> > };
> >
> > - memory {
> > + /*
> > + * Reserve below regions from memory node:
> > + *
> > + * 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
> > + */
> > + memory@0 {
> > device_type = "memory";
> > - reg = <0x0 0x00000000 0x0 0x40000000>;
> > + reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
> > + <0x00000000 0x05f00000 0x00000000 0x00001000>,
> > + <0x00000000 0x05f02000 0x00000000 0x00efd000>,
> > + <0x00000000 0x06e00000 0x00000000 0x0060f000>,
> > + <0x00000000 0x07410000 0x00000000 0x1aaf0000>,
> > + <0x00000000 0x22000000 0x00000000 0x1c000000>;
> > };
> >
> > - reserved-memory {
> > - #address-cells = <2>;
> > - #size-cells = <2>;
> > - ranges;
> > -
> > - mcu-buf@05e00000 {
> > - no-map;
> > - reg = <0x0 0x05e00000 0x0 0x00100000>, /* MCU
> > firmware buffer */
> > - <0x0 0x0740f000 0x0 0x00001000>; /* MCU
> > firmware section */
> > - };
> > -
> > - reboot-reason@05f01000 {
> > - no-map;
> > - reg = <0x0 0x05f01000 0x0 0x00001000>;
> > - };
> > -
> > - mbox-buf@06dff000 {
> > - no-map;
> > - reg = <0x0 0x06dff000 0x0 0x00001000>; /* Mailbox
> > message buf */
> > - };
> > -
> > - pstore: pstore@0x21f00000 {
> > - no-map;
> > - reg = <0x0 0x21f00000 0x0 0x00100000>; /*
> > pstore/ramoops buffer */
> > - };
> > + pstore: pstore@0x21f00000 {
> > + no-map;
> > + reg = <0x0 0x21f00000 0x0 0x00100000>; /* pstore/ramoops
> > buffer */
> > };
> >
> > ramoops {
> > --
> > 1.9.1
> >
> >
> > Uboot's patch:
> >
> > ---8<---
> >
> > From ed5091a3f07d0b5c5bb0eece24aa24aa03523563 Mon Sep 17 00:00:00 2001
> > From: Leo Yan <leo.yan@linaro.org>
> > Date: Sat, 9 Apr 2016 22:03:08 +0800
> > Subject: [PATCH] arm: bootm-fdt.c: skip overwrite memory node for ARM64
> >
> > Device tree is used to describe exactly hardware info. So we need make
> > sure all the hardware info including memory node should be correct in
> > the first place rather than overwrite by u-boot to automatically detect
> > memory size; otherwise if there have memory reservation info in original
> > dtb file, the function arch_fixup_fdt will overwrite the memory node and
> > kernel cannot know related info anymore.
> >
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> >
>
> Thanks for the patches, however I doubt this U-Boot patch will get accepted
> by the upstream folks. I think what we will probably need to do is migrate
> HiKey U-Boot over to using DT for its configuration.

I also don't think this is an accpetable patch :)

> I didn't do this in the original port, as there was very little HiKey DT in
> the
> upstream kernel. Now that situation has changed over the last few kernel
> releases

I should have checked the upstream kernel before writing that....actually
nothing has changed upstream in terms of HiKey DT. In v4.6-rc3 we
still have no DT for pinmux, gpio, pmic, usb or sd/emmc. So it is not
possible to convert HiKey U-Boot over to being fully configured via DT.

The 'out of the box' upstream kernel only enables cpu's, interrupts,
timer, and some UARTs :( It looks like various drivers have been merged,
but strangely none of the DT which enables them for the HiKey board.

I have done some patches which start the conversion to a DT U-Boot, but

it can only configure the UART until some more DT lands in the upstream
kernel (this works, but the other U-Boot drivers still have to rely on
platform data for their configuration).

> we can import the DT into U-Boot and we will inherit all the memory nodes
> etc which should hopefully fix the issue (if I've understood correctly).

> Unless
> U-Boot is still buggy and overwrites the memory node of course ;)

Now U-boot forces to overwrite memory node. Or maybe we can add code
to detect if DTB has memory node, then directly return from the
function arch_fixup_fdt so can skip overwriting flow?

Looking at this in more detail, I don't think we can change the implementation of

arch_fixup_fdt() at all as it is used by many platforms and we will almost certainly
break things.

U-Boot generates the memory DT node based on its view of the memory

banks, and this is how it works on all architectures I've looked at (MIPS, x86, arm).

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) = {

DECLARE_GLOBAL_DATA_PTR;

+#if !CONFIG_IS_ENABLED(OF_CONTROL)
+
static const struct pl01x_serial_platdata serial_platdata = {
#if CONFIG_CONS_INDEX == 1
        .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;
}

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

there maybe reasons for reserving op-tee memory via the memory node, but I'm
not so sure about some of the other reservations. 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).

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?

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.

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.

regards,

Peter.