Furquan Shaikh 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:
(4 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);
BIOSRAM can only be accessed as byte, so word and dword access are actually constructs using byte ac […]
Wouldn't you end up still doing byte access with: (biosram_read8(offset + sizeof(uint8_t)) << 8 | biosram_read8(offset);
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);
That can be found in iomap.h, and I don't believe it's needed when you are using these functions. […]
Since this is the API for all helpers provided by this common block, it is helpful to know which set of functions to use for what purpose.
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.
All have to be defined (even if not used by a particular platform) to avoid build errors, and the li […]
How do you control some common code accessing a block that is actually not defined on a platform?
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... PS4, Line 65: ACPIMMIO_AOAC_BASE
That's what all these changes are about. These addresses were originally at iomap.h (chip specific). […]
Comment in this file seems to indicate that not all blocks are actually present on all platforms. In that case shouldn't there be a way for the common code to know which block is actually present?