Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32649 )
Change subject: soc/amd/common: Create AcpiMmio functionality from stoneyridge ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/acpimmio/mm... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/acpimmio/mm... PS4, Line 147: for (i = sizeof(value) - 1 ; i >= 0 ; i--) : value = (value << 8) | biosram_read8(offset + i);
Any reason why this is not: […]
BIOSRAM can only be accessed as byte, so word and dword access are actually constructs using byte access.
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... File src/soc/amd/common/block/include/amdblocks/acpimmio.h:
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... PS4, Line 21: void enable_acpimmio_decode(void);
It might be helpful to add a comment for each block indicating what base the set of functions are re […]
That can be found in iomap.h, and I don't believe it's needed when you are using these functions. All you need is "what functionality these functions affect", and that's provided by their names.
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... PS4, Line 37: All products do not support all blocks below, however AMD has avoided : * redefining addresses and consumes new ranges as necessary.
I am wondering if it makes sense to define a structure with offsets to each of the blocks below and […]
All have to be defined (even if not used by a particular platform) to avoid build errors, and the linker is smart enough to only link into the final image what will be used. That's partially the reason why AMD has avoided redefining addresses. Once an address is defined for a particular functionality, it stays put, for backward compatibility.
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... PS4, Line 44: AMD_SB_ACPI_MMIO_ADDR
Is this guaranteed to be the same across all AMD platforms? If not, we should define that as a Kconf […]
From my understanding, they have not changed in the last 10 years, since the concept was developed.
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... PS4, Line 65: ACPIMMIO_AOAC_BASE
Looking at all these base addresses, is it bad to have them defined in the iomap. […]
That's what all these changes are about. These addresses were originally at iomap.h (chip specific). However theses changes are about moving all we can to the common folder, to be reused by future chips. Coreboot community is complaining of continuous dumps of identical code with each new chip, without reusing previous code (thus consuming more space and making it harder to maintain). With common code, if a fix is needed, the fix goes to all platforms simultaneously.