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:
(9 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_read8(offset + sizeof(uint8_t)) << 8 | biosram_read8(offset);
like other definitions?
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/acpimmio/mm... PS4, Line 167: biosram_write8(offset + i, value & 0xff); same here.
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/acpimmio/mm... PS4, Line 176: iosram_write8(offset + i, value & 0xff); and here
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 reading/writing from/to.
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 25: TODO Does it make sense to start with a Kconfig for this which is set to 0x4 by default and can be overriden if necessary by the SoC that defines it differently.
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 each platform can set only the offsets/blocks that it actually supports and rest can be set to -1. That will ensure common code does not end up accessing blocks which are not present.
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 Kconfig.
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... PS4, Line 45: 0xfed80000 Set these as (AMD_SB_ACPI_MMIO_ADDR + 0) (AMD_SB_ACPI_MMIO_ADDR + 0x200) to indicate that they are offsets from the ACPI_MMIO_ADDR?
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.h file in the specific soc?