View Change

12 comments:

• File src/mainboard/emulation/qemu-aarch64/Makefile.inc:

  • Patch Set #18, 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.

• File src/mainboard/emulation/qemu-aarch64/bootblock_custom.S:

  • Patch Set #15, 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.

• File src/mainboard/emulation/qemu-aarch64/bootblock_custom.S:

  • Patch Set #16, 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...)

• File src/mainboard/emulation/qemu-aarch64/bootblock_custom.S:

  • Patch Set #18, Line 43: br x0

    nit: technically more correct would be 'blr'

  • Patch Set #18, Line 45: after_relocate:

    You don't need this anymore

• File src/mainboard/emulation/qemu-aarch64/devicetree.cb:

  • Patch Set #18, 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?).

• File src/mainboard/emulation/qemu-aarch64/mainboard.c:

  • Patch Set #18, Line 18: #define VIRT_DRAM_BASE 0x40000000

    You shouldn't need this, it's already in memlayout

  • Patch Set #18, Line 23: >> 10

    Prefer

     x / KiB

    to

     x >> 10

  • Patch Set #18, Line 23: VIRT_DRAM_BASE

    This should be (uintptr_t)_dram

• File src/mainboard/emulation/qemu-aarch64/memlayout.ld:

  • Patch Set #18, 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).

  • Patch Set #18, 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.

• File src/mainboard/emulation/qemu-aarch64/mmio.c:

  • Patch Set #18, 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?)

To view, visit change 33387. To unsubscribe, or for help writing mail filters, visit settings.