Julius Werner 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:
(8 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)
boot rom is the code run by the SoC itself, before coreboot's bootblock is executed. […]
Does it make sense to store this DTS in coreboot? It doesn't really have anything to do with coreboot. How about providing instructions (or a script) on how to generate it and add it to a FIT image instead? That way, it can't go stale if QEMU updates.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 50: main
main() is defined at src/lib/bootblock.c. I'm now using bootblock_custom. […]
Oohhhh... now I get how you did this. You added ../../../lib/bootblock.c manually in the mainboard Makefile.
Yeah, that's an ugly hack, let's not do that. We should just fix BOOTBLOCK_CUSTOM so that that function is always available. Can you please change the other BOOTBLOCK_CUSTOM boards (I think there's only 3) in a separate CL to rename their bootblock main() functions into something else (e.g. tegra124_main() and the like), and then remove the BOOTBLOCK_CUSTOM compile guard for bootblock.c in src/lib/Makefile?
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.
https://review.coreboot.org/c/coreboot/+/33387/16/src/mainboard/emulation/qe... PS16, Line 13: ic ialluis Do you actually need this?
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. (I still think you could just call memcpy() instead?)
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:
1. get the address of main 2. subtract _flash 3. add _bootblock 4. jump
? So something like
/* calculate relocation offset between bootblock in flash and in DRAM */ ldr x0, =_flash ldr x1, =_bootblock sub x1, x1, x0
/* jump to main() in DRAM */ ldr x0, =main add x0, x0, x1 bl x1
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
I created this file because I wanted to use these addresses in mmio.c and mainboard.c. […]
Commonly we define MMIO blocks (e.g. your GPIO, RTC, etc. here) in addressmap.h and actual memory (e.g. SRAM, DRAM, in your case I'd also say FLASH) in memlayout.
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. Like I said, one of the main goals of this file is to have an easy reference to look up what is at what address (e.g. if you're trying to understand what addresses from log lines or crash dumps are). That just gets harder and harder the more indirection you have in here. (Compare how other memlayout files like rockchip/rk3399 or mediatek/mt8183 look like.)