Martin Roth 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);
Wouldn't you end up still doing byte access with: […]
Yes, these should probably be updated to match pm_io_write16(). Marshall just added the PMIO routines, and updated them when he did that. These are the existing ones, and we should update them to match.
I'm ok with Marshall updating them in a follow-on commit if that's ok with you.
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);
Since this is the API for all helpers provided by this common block, it is helpful to know which set […]
I'm not going to object to having things better documented.
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.
How do you control some common code accessing a block that is actually not defined on a platform?
How about this: Let's leave this in iomap.h and just define the ones that are used in an SOC. That will give us an error at build time if we're trying to use a block that isn't defined.
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... PS4, Line 65: ACPIMMIO_AOAC_BASE
Comment in this file seems to indicate that not all blocks are actually present on all platforms. […]
We should only move things when it makes sense and makes the code cleaner. A list of fixed base addresses makes sense to keep in the chip specific headers.