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:
(10 comments)
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)
Does it make sense to store this DTS in coreboot? It doesn't really have anything to do with coreboo […]
Creating the util for adding DTS to a FIT image sounds good. In this case, users should be: 1. build a FIT payload 2. add a DTS to a FIT image by new util 3. make coreboot.rom with an empty payload 4. add the payload to coreboot.rom by util/cbfstool
Is the flow correct? I will write the step in Documentation/.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 50: main
Oohhhh... now I get how you did this. You added ../../../lib/bootblock. […]
I see. OK, I created a new CL to deal with the problem. I found the 3 SoCs to use BOOTBLCOK_CUSTOM. - nvidia/tegra124 - nvidia/tegra240 - cavium/cn81xx
Could you review this CL? https://review.coreboot.org/c/coreboot/+/34250
https://review.coreboot.org/c/coreboot/+/33387/16/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/bootblock_custom.S:
https://review.coreboot.org/c/coreboot/+/33387/16/src/mainboard/emulation/qe... PS16, Line 12: .org 0
Does this actually do anything? I think you can take it out.
No, it's just explicit for developers. I removed it.
https://review.coreboot.org/c/coreboot/+/33387/16/src/mainboard/emulation/qe... PS16, Line 13: ic ialluis
Do you actually need this?
No, it doesn't affect the output. Thank you for pointing it out.
https://review.coreboot.org/c/coreboot/+/33387/16/src/mainboard/emulation/qe... PS16, Line 30: x19
nit: this might as well be x3 since you're not calling other functions right now. […]
Sorry, do you mean to use memcpy() after the jump to C code? Or is there memcpy() in instruction sets?
https://review.coreboot.org/c/coreboot/+/33387/16/src/mainboard/emulation/qe... PS16, Line 43: ldr x1, =_bootblock
While this works it looks pretty confusing to me. Why not just: […]
Thank you for your simpler way. I changed your code a little bit but I used it.
/* jump to main() in DRAM */ adr x0, main add x0, x0, x1 br x0
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 36: #define VIRT_DRAM_START 0x40000000
Commonly we define MMIO blocks (e.g. your GPIO, RTC, etc. here) in addressmap. […]
I moved FLASH_BASE and DRAM_BASE to memlayout.ld and other definitions remain in this file.
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 28: ram_resource(dev, 0, VIRT_DRAM_START >> 10, discovered << 10);
have a look at "mainboard/emulation/qemu-i440fx/northbridge.c" […]
Thank you for the reference. I updated this file but I'm not sure it's perfect. Please tell me if I understand it wrongly.
https://review.coreboot.org/c/coreboot/+/33387/16/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/33387/16/src/mainboard/emulation/qe... PS16, Line 28: VIRT_DRAM_BASE + FIT_SPACE + GT_128KiB
I'd suggest to use literals for all of these and not add multiple things together. […]
Thank you for the references. I defined addresses in a simpler way.
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);
Have a look at "soc/qualcomm/sdm845/timer.c" […]
Thank you for the reference. Sorry, I made a mistake and this file can be dropped. I deleted this file instead of implementing init_timer().