Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44394 )
Change subject: mb/emulation/qemu-armv7: Add MMU support ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44394/2/src/mainboard/emulation/qem... File src/mainboard/emulation/qemu-armv7/bootblock.c:
https://review.coreboot.org/c/coreboot/+/44394/2/src/mainboard/emulation/qem... PS2, Line 17: MMU_CONF_SYMBOL(flash, DCACHE_OFF) btw, if you want you could implement a DCACHE_READONLY access type for flash regions like this (which caches reads but aborts on a write). The page table format would support it.
https://review.coreboot.org/c/coreboot/+/44394/2/src/mainboard/emulation/qem... PS2, Line 25: MMU_CONF_SYMBOL(usb, DCACHE_OFF) Why not just follow what other Arm SoCs do (mmu_config_range(0, 4096, DCACHE_OFF))? Trying to separate out every single MMIO device seems very messy (and this is just an emulator, normally you would have more devices) and there's honestly no real benefit. It can be nice to block off the first page if that's possible (so you'd write (1, 4096, DCACHE_OFF)) to catch null pointer accesses, but that doesn't work here because you have your flash there... and for everything else, I think the chance that you can catch a rare pointer arithmetic bug that just happens to hit one of your unused parts isn't high enough to warrant the complexity.
Also, the mmu_config_range_kb() thing should better only be used where you *really* need it, because it is further restricted by the size of TTB_SUBTABLES.