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) = {
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 getsgenerated
* 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: -
- 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.h... [2] http://archive.arm.linux.org.uk/lurker/message/20151105.161315.cc3c99f5.en.h...
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.
- 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%3Ba=blob%3Bf=arch/arm/mach-tegra/board2.c%3...
and in particular the comment here
http://git.denx.de/?p=u-boot.git%3Ba=blob%3Bf=arch/arm/mach-tegra/board2.c%3...
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 world).
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: - https://www.mail-archive.com/u-boot@lists.denx.de/msg210319.html
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 :)
Regards,
Peter.