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);
I'll plan on a follow-on. This was mostly a direct move of existing code.
SGTM.
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.
... […]
SGTM. I think we should at least leave the definitions in iomap.h and just define the ones that are used in that particular SoC. That way, if some part of common code ends up calling a function which uses a particular macro, it would generate a build time error.
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... PS4, Line 44: AMD_SB_ACPI_MMIO_ADDR
In the older implementations it was configurable. […]
SGTM.
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... PS4, Line 45: 0xfed80000
That was my preference and I started out that way. […]
Aah interesting. Hadn't realized that this creates a problem for ASL.