Asami Doi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33387 )
Change subject: mainboard/emulation/qemu-aarch64: Add new board for ARMv8 ......................................................................
Patch Set 16:
(19 comments)
Thank you for your reviews. I add a documentation, deleted cpu/armltd/cortex-a53, and updated files. I have some things I don't understand yet.
1. I'm trying to add a payload LinuxBoot by selecting it via 'make menuconfig'. However, I got the following error: make LINUXBOOT_CROSS_COMPILE=aarch64-linux-gnu- ... cp: -r not specified; omitting directory '../../../' make[2]: *** [targets/linux.mk:112: linuxboot/target.dtb] Error 1 make[1]: *** [Makefile:55: kernel] Error 2 make: *** [payloads/external/Makefile.inc:266: linuxboot] Error 2
I think this error relates to the second question about FDT.
2. What is an FDT and how can I use it? I looked it up and found an FDT is a flattened device tree which is used especially for ARM system. We can configure peripherals via an FDT dynamically. How can I pass an FDT when building the image?
https://review.coreboot.org/c/coreboot/+/33387/15/src/cpu/armltd/cortex-a53/... File src/cpu/armltd/cortex-a53/Kconfig:
https://review.coreboot.org/c/coreboot/+/33387/15/src/cpu/armltd/cortex-a53/... PS15, Line 1: config CPU_ARMLTD_CORTEX_A53
Note that src/cpu is pretty much deprecated, at least for non-x86. […]
I deleted cpu/armld/cortex-a53 because they have nothing functions for now. They just existed for future functions.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/Kconfig:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 19: select ARCH_RAMSTAGE_ARMV8_64
If you select these here directly, what does the CPU_ARMLTD_CORTEX_A53 even do?
cpu/armld/cortex-a53 have nothing functions for now. So I deleted them.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/bootblock_custom.S:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 11: ENTRY(_start)
doesn't boot rom pass FDT as argument?
Sorry, I only have a few knowledge about FDT. How can I pass it as an argument?
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 26: b copy_code
nit: I think the branch should be unnecessary because . […]
Ack
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 32: ldp q0, q1, [x1], 32 /* Load 32 bytes */
Should be okay to use memcpy() here instead? You can put the stuff you want preserved into x19-x29 i […]
Yes, it's ok. I used normal instructions and general purpose registers instead.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 45: /* Jump to code in DRAM */
restore saved FDT pointer and pass it to bootblock
Sorry, where is the FDT pointer?
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 50: main
Sorry, where's this main()? I don't see it. […]
main() is defined at src/lib/bootblock.c. I'm now using bootblock_custom.S by "select BOOTBLOCK_CUSTOM" in src/mainboard/emulation/qemu-aarch64/Kconfig. Isn't it enough?
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/cbmem.c:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 40: int probe_ramsize(void)
Can this reuse what Patrick is doing in CB:33934?
It seems good. I will reuse it. The CL is now on the review so I added TODO comment, for now, to reuse it after the CL is merged.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/debug.S:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 11: * Just debugging. I will remove it later.
When is later? Looks like this is unused.
Sorry, this is already unused. I removed it.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 10: device i2c 6 on end # Fake component for testing
Note that Arm devices usually don't use devicetree. […]
Thank you for your comment. Is it enough to pass FDT for ARM to setup device configurations? I got the following error when I remove this file: make: *** No rule to make target 'src/mainboard/emulation/qemu-aarch64/devicetree.cb', needed by 'build/mainboard/emulation/qemu-aarch64/static.c'.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/include/mainboard/addressmap.h:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 28: #define VIRT_FLASH_START 0x00000000
nit: convention is to call these _BASE, not _START
Ack
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 36: #define VIRT_DRAM_START 0x40000000
For things that appear in memlayout, the convention is to have the constant directly in memlayout ra […]
I created this file because I wanted to use these addresses in mmio.c and mainboard.c. I avoided defining addresses in multiple files. But I can move these addresses to memlayout.ld. Which is better?
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/mainboard.c:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 22: if (!dev) {
I guess this is dead code and will never happen
OK. I removed the if-statement.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 28: ram_resource(dev, 0, VIRT_DRAM_START >> 10, discovered << 10);
ram_resouce() should be called in "discover resources" phase, not in "enable" phase.
Sorry, does "discover resources" mean BS_DEV_RESOURCES?
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 29: cbmem_recovery(0);
I guess it's copied from another emulated board, doing it wrong, too.
Sorry, I copied this file from mainboard/emulation/qemu-armv7/. I removed this line.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 17: DRAM_START(VIRT_DRAM_START)
Like Patrick said, it's best to leave some free space at the beginning of DRAM for FIT images becaus […]
OK, thank you for your advice. I made space more than 512MiB at the beginning of the DRAM for FIT images.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 19: 0x10000
Is this an arbitrary number, or was it derived?
This is an arbitrary number as long as a region doesn't overlap. In this case, STACK starts just after BOOTBLOCK.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 24: 0x01040000
How was this constant derived? Should we move it to a #define?
This is the number just after the end of RAMSTAGE. We can put TTB anywhere as long as a region doesn't overlap but I currently don't use TTB.
I moveed numbers to #define.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/timer.c:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 9: void udelay(unsigned int n);
I think this file can be droped by simply selecting CONFIG_ARM64_USE_ARCH_TIMER
src/arch/arm64/arch_timer.c will be enable when we select CONFIG_ARM64_USE_ARCH_TIMER. However, it doesn't have init_timer() then it should cause the error "undefined reference to `init_timer'".