Attention is currently required from: Jason Glenesk, Raul Rangel, Angel Pons, Felix Held. Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50510 )
Change subject: soc/amd/cezanne: Fill FADT and MADT ......................................................................
Patch Set 5:
(1 comment)
File src/soc/amd/cezanne/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/50510/comment/4ffbd66a_e99d1969 PS1, Line 32: 0x500
0x500 is correct if I'm reading it correct. […]
Oh I'm so frustrated with this PPR right now!
I was reading the source as the acpimmio_base + offset, i.e. 0xfed80000 + 0x800. You wrote it as pm_regs + 0x500 or 0xfed80000 + 0x300 + 0x500. They changed the individual register definitions, but left the table of blocks with the same offsets. Perhaps that's why the ACPI MMIO Space Allocation table has those two on the same row now.
Given a preference, I would go with the "old" way of doing things (i.e. make the offsets from acpimmio_acpi instead of acpimmio_pmio + 0x500). For example in common/block/acpi/acpi.c. And there's already definitions for offsets in amdblocks/acpi.h.
Otherwise, I can settle for matching the PPR precisely however then we'll be using two different means for defining the same registers.
Regardless, I think we really need a comment somewhere that this is defined differently from previous generations although functionally identical.
Also regardless, if you keep it similar to how it is now, would you mind also renaming the #define on line 32? Nearly anyone who's worked with AMD knows the entire region at 0xfed80000 as "acpimmio" even though the name has never made much sense. So I think a name of ACPI_MMIO_BASE is a bit confusing. And since these are now part of the PM regs and no longer I/O ranges, the defs should probably go into southbridge.h.