Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37328 )
Change subject: sb/amd/cimx: replace cimx_util with common ACPIMMIO AMD block ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/coreboot/+/37328/1/src/mainboard/amd/inagua/ma... File src/mainboard/amd/inagua/mainboard.c:
https://review.coreboot.org/c/coreboot/+/37328/1/src/mainboard/amd/inagua/ma... PS1, Line 66: pm_write8(0x29, 0x80); To use pm_writeX over pm_iowriteX sounds good, but you need to verify the ACPIMMIO region for PM exists (and is potentially separately enabled early enough).
https://review.coreboot.org/c/coreboot/+/37328/1/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/gpio_ftns.c:
https://review.coreboot.org/c/coreboot/+/37328/1/src/mainboard/pcengines/apu... PS1, Line 28: return (base_addr); () not needed
https://review.coreboot.org/c/coreboot/+/37328/1/src/mainboard/pcengines/apu... PS1, Line 37: *memptr = iomux_ftn; MMIO needs to use read/writeX(). Probably does not happen here, but for some code flows compiler can actually optimise things in a bad way. Like if you tried to flip bits in a certain sequence, the hardware register might only see the final state.
Separate commit to touch these please.
https://review.coreboot.org/c/coreboot/+/37328/1/src/mainboard/pcengines/apu... PS1, Line 48: u8 *memptr = (u8 *)(base_addr + GPIO_OFFSET + gpio); Like above.
https://review.coreboot.org/c/coreboot/+/37328/1/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/gpio_ftns.c:
https://review.coreboot.org/c/coreboot/+/37328/1/src/mainboard/pcengines/apu... PS1, Line 25: bdata = read32((const volatile void *)(ACPI_MMIO_BASE + GPIO_OFFSET OT: Looks like a candidate followup for some of the new ACPIMMIO functions. Note that apu1 and apu2 have differente GPIO_OFFSET defined..
https://review.coreboot.org/c/coreboot/+/37328/1/src/southbridge/amd/cimx/ci... File src/southbridge/amd/cimx/cimx_util.h:
https://review.coreboot.org/c/coreboot/+/37328/1/src/southbridge/amd/cimx/ci... PS1, Line 26: #define PM_DATA 0xcd7 If you have interest, grep source for remaining cases of 0xcd6/0xcd7 manipulations.