Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41816 )
Change subject: soc/amd/picasso: add psp_verstage ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... PS7, Line 5: CPPFLAGS_verstage += -I$(src)/soc/amd/picasso/psp_verstage/include
Exactly, and the same for io.h. […]
Why are you replacing arch/mmio.h at all? Looks like your versions aren't doing anything fundamentally different, they're just removing the dmb()s and the __builtin_assume_aligned(). The dmb()s are there so that programmers don't need to explicitly put memory barriers in their hardware access code (this roughly matches Linux behavior of readl()/writel() type functions). While that could be done differently (I've thought about this a couple of times but never enough to do anything) to save some instructions where this isn't necessary, that would be something that should be done at the whole architecture level, not one individual SoC (e.g. we'd probably introduce separate write8_relaxed() calls like Linux has them for people who explicitly know they don't need barriers). Right now the global assumption is that MMIO accessors are implicitly ordered against each other and SoCs shouldn't individually break that.
The __builtin_assume_aligned() was there because some specific compiler version would otherwise sometimes generate LDRB instead of LDR instructions in certain specific inlining edge cases. This just fixes that rare bug, but in most cases it has no effect.
(I don't think you should rely on include order magic if possible, so at least getting rid of arch/mmio.h would be good. arch/io.h doesn't seem as bad because at least there's no other version of that under arch/arm, although an SoC providing an arch/ header is still kinda weird.)