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 18:
(12 comments)
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... PS18, Line 27: bootblock-y += ../../../lib/bootblock.c If you have both of your CLs on the same Git branch (first that other CL, then this one) you can upload them both at once and Gerrit will recognize the implicit dependency.
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)
Creating the util for adding DTS to a FIT image sounds good. In this case, users should be: […]
Right, sounds good.
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 30: x19
Sorry, do you mean to use memcpy() after the jump to C code? Or is there memcpy() in instruction set […]
I mean you can call memcpy() from here, like this:
bl memcpy dmb sy
Your code already sets up all the arguments right (destination in x0, source in x1, size in x2), so you should be all set. memcpy is implemented in assembly (see src/arch/arm64/memcpy.S) and doesn't use the stack, so it should be safe to call in this environment. (It doesn't do the cache line alignment, though, so if you find it to be significantly slower than your code here it's fine to keep it like this as well. But does the cache line even matter for QEMU? I would be very surprised if it emulated effects like that...)
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/bootblock_custom.S:
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... PS18, Line 43: br x0 nit: technically more correct would be 'blr'
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... PS18, Line 45: after_relocate: You don't need this anymore
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... PS18, Line 8: chip drivers/generic/generic # I2C0 controller I still think this shouldn't be here. It doesn't do anything and doesn't refer to anything real. I'd just leave this file empty (or does that not work?).
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/mainboard.c:
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... PS18, Line 18: #define VIRT_DRAM_BASE 0x40000000 You shouldn't need this, it's already in memlayout
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... PS18, Line 23: >> 10 Prefer
x / KiB
to
x >> 10
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... PS18, Line 23: VIRT_DRAM_BASE This should be (uintptr_t)_dram
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... PS18, Line 37: * 0x6111_0000..: TTB You shouldn't need extra comments to explain the layout if you just write this file itself to already make the layout obvious (see below).
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... PS18, Line 43: DRAM_START(VIRT_DRAM_BASE) No, sorry, this is still not what I meant. You should just write
DRAM_START(0x40000000)
here. No need to #define anything. See how src/soc/mediatek/mt8183/include/soc/memlayout.ld looks.
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/mmio.c:
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... PS18, Line 14: // TODO: Return VIRT_SECURE_UART_BASE when QEMU works on a secure mode. nit: please use /* C89 comments */, not // C99 comments
(I also don't really understand what the comment is saying. I thought this is running in secure mode (i.e. EL3) already?)