Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38222 )
Change subject: soc/amd/stoneyridge: Change some local SMBus function signatures ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38222/4/src/soc/amd/stoneyridge/smb... File src/soc/amd/stoneyridge/smbus.c:
https://review.coreboot.org/c/coreboot/+/38222/4/src/soc/amd/stoneyridge/smb... PS4, Line 28: return asf_read8(reg); Inspecting the assembly it looked like the compiler is not able to do a good optimisation of this even with these xxx_read8() functions inlined.
The expansion here is:
if (base == A) return read8(base + reg) else if (base == B) return read8(base + reg) else return -1;
Furthermore, there could be an implicit assert in the source to make sure only the base address of an SMBus host is ever passed:
ASSERT((base == A) || (base == B))
An MMIO operation that at its best optimises to a single x86 instruction takes some 20-25 instructions here. Yes, you can argue that there is no performance impact but the code structure looks weird (to me).