Attention is currently required from: Jamie Ryu, Wonkyu Kim, Ethan Tsao, Ravishankar Sarawadi, Tim Wawrzynczak, Paul Menzel, Nick Vaccaro. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63198 )
Change subject: soc/intel/common: implement ioc driver ......................................................................
Patch Set 5:
(17 comments)
File src/soc/intel/common/block/gpmr/Kconfig:
https://review.coreboot.org/c/coreboot/+/63198/comment/c654ad16_687627e8 PS5, Line 3: SOC_INTEL_COMMON_BLOCK_DMI || SOC_INTEL_COMMON_BLOCK_IOC how do you plan to restrict a user from selecting both Kconfig.
Prior to MTL, the GPMR is configurable using PCR read/write and from MTL, it's using IOC mmio read/write.
Check this, https://review.coreboot.org/c/coreboot/+/59804/6/src/soc/intel/alderlake/inc...
you can also apply the same logic in gpmr.c file as well.
https://review.coreboot.org/c/coreboot/+/63198/comment/9295571b_85fb1375 PS5, Line 5: DMI I guess you are talking about access mechanism rather interface here hence, PCR might be more applicable
File src/soc/intel/common/block/gpmr/gpmr.c:
https://review.coreboot.org/c/coreboot/+/63198/comment/969c24c5_003f4026 PS5, Line 11: if (CONFIG(SOC_INTEL_COMMON_BLOCK_IOC)) : return ioc_reg_read32(offset); : else : return pcr_read32(PID_DMI, offset); nit: you can implement as https://review.coreboot.org/c/coreboot/+/62723 to abstract the common code with SoC specific function.
File src/soc/intel/common/block/include/intelblocks/gpmr.h:
https://review.coreboot.org/c/coreboot/+/63198/comment/70eb4b1e_a2348882 PS5, Line 12: #define MAX_GPMR_REGS IOC_MAX_GPMR_REGS : #define GPMR_OFFSET IOC_GPMR_OFFSET : #define GPMR_LIMIT_MASK IOC_GPMR_LIMIT_MASK : #define GPMR_BASE_SHIFT IOC_GPMR_BASE_SHIFT : #define GPMR_BASE_MASK IOC_GPMR_BASE_MASK : #define GPMR_DID_OFFSET IOC_GPMR_DID_OFFSET : #define GPMR_EN IOC_GPMR_EN suggestion: keep two .h file as ioc_gpmr.h and pcr_gpmr.h and pick the one based on CONFIG(SOC_INTEL_COMMON_BLOCK_IOC)
File src/soc/intel/common/block/include/intelblocks/ioc.h:
https://review.coreboot.org/c/coreboot/+/63198/comment/95530b59_df96db89 PS5, Line 10: <soc/ioc_reg.h> I don't prefer override like UEFI does, it creates confusion while debugging, if you like to refer few macros from SoC, please have it defined there, in that case, any SoC that selects IOC driver in future and missed to implement the correct macros as needed, would run int compilation error
https://review.coreboot.org/c/coreboot/+/63198/comment/15ff2d97_ddae2380 PS5, Line 15: : void ioc_reg_write32(uint32_t offset, uint32_t value); : uint32_t ioc_reg_read32(uint32_t offset); outside gpmr.c, is there any other consumer of these APIs ?
File src/soc/intel/common/block/ioc/Kconfig:
https://review.coreboot.org/c/coreboot/+/63198/comment/85595a37_5af8c5f8 PS5, Line 1: SOC_INTEL_COMMON_BLOCK_IOC you are depending on MCH_BASE_ADDRESS being programmed for IOC read/write, hence, required driver is needed.
File src/soc/intel/common/block/ioc/ioc.c:
https://review.coreboot.org/c/coreboot/+/63198/comment/53c74b4a_c62d6b68 PS5, Line 9: write32 use `write32p` so, you don't need (void *) casting
https://review.coreboot.org/c/coreboot/+/63198/comment/67c5563a_7c660b0c PS5, Line 9: MCH_BASE_ADDRESS worth checking MCHBAR bit 0 if memory space is enabled ?
https://review.coreboot.org/c/coreboot/+/63198/comment/55f70868_9b307327 PS5, Line 15: read32 read32p
File src/soc/intel/common/block/lpc/lpc_lib.c:
https://review.coreboot.org/c/coreboot/+/63198/comment/17300b22_8239e963 PS5, Line 28: if (CONFIG(SOC_INTEL_COMMON_BLOCK_IOC)) : ioc_reg_write32(R_IOC_CFG_LPCIOE, io_enables); : else if (CONFIG(SOC_INTEL_COMMON_BLOCK_LPC_MIRROR_TO_DMI)) : pcr_write16(PID_DMI, PCR_DMI_LPCIOE, io_enables); why not `gpmr_write32`, i guess it's possible to unify based on register name.
https://review.coreboot.org/c/coreboot/+/63198/comment/4c318f15_303f0d40 PS5, Line 51: pcr_write16(PID_DMI, PCR_DMI_LPCIOD, io_ranges); same as above
https://review.coreboot.org/c/coreboot/+/63198/comment/c6332945_f80d9f46 PS5, Line 125: same as above
https://review.coreboot.org/c/coreboot/+/63198/comment/810aa5f1_900fbe10 PS5, Line 161: pcr_write32(PID_DMI, PCR_DMI_LPCGMR, lgmr); same as above
https://review.coreboot.org/c/coreboot/+/63198/comment/0f0cedd0_e07bfd65 PS5, Line 265: } same as above
File src/soc/intel/common/block/smbus/tco.c:
https://review.coreboot.org/c/coreboot/+/63198/comment/e50b731b_e25f938d PS5, Line 130: pcr_write32(PID_DMI, PCR_DMI_TCOBASE, tcobase | TCOEN); use gpmr_write32
File src/soc/intel/common/pch/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/63198/comment/b2c942ff_05c76fae PS5, Line 53: } you can create gmpr read_or8/32 function.
-
Subrata Banik (Code Review)