Attention is currently required from: Jamie Ryu, Subrata Banik, Ethan Tsao, Ravishankar Sarawadi, Tim Wawrzynczak, Paul Menzel, Angel Pons, Nick Vaccaro. Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63198 )
Change subject: soc/intel/common: implement ioc driver ......................................................................
Patch Set 5:
(16 comments)
File src/soc/intel/common/block/gpmr/Kconfig:
https://review.coreboot.org/c/coreboot/+/63198/comment/f11151ea_689230ee 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. […]
SOC_INTEL_COMMON_BLOCK_DMI will be updated SOC_INTEL_COMMON_PCR. And Add checking as you mentioned.
https://review.coreboot.org/c/coreboot/+/63198/comment/6d53c9a6_43a09fd6 PS5, Line 5: DMI
I guess you are talking about access mechanism rather interface here hence, PCR might be more applic […]
Ack
File src/soc/intel/common/block/include/intelblocks/gpmr.h:
https://review.coreboot.org/c/coreboot/+/63198/comment/09879b03_cd6847b6 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. […]
Ack
File src/soc/intel/common/block/include/intelblocks/ioc.h:
https://review.coreboot.org/c/coreboot/+/63198/comment/53696746_547a9f19 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 f […]
I'll remove this option for now. Just add this option in case new SOC has different offsets. And add change if we have new SOC which has different offsets.
File src/soc/intel/common/block/ioc/ioc.c:
https://review.coreboot.org/c/coreboot/+/63198/comment/f47e05ce_ccb84a9e PS5, Line 9: write32
use `write32p` so, you don't need (void *) casting
Ack
https://review.coreboot.org/c/coreboot/+/63198/comment/75eb8aee_d3d0f93f PS5, Line 15: read32
read32p
Ack
File src/soc/intel/common/block/lpc/lpc_lib.c:
https://review.coreboot.org/c/coreboot/+/63198/comment/e1f8052e_f5fdb819 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.
Ack
https://review.coreboot.org/c/coreboot/+/63198/comment/6e81f274_08711b23 PS5, Line 51: pcr_write16(PID_DMI, PCR_DMI_LPCIOD, io_ranges);
same as above
Ack
https://review.coreboot.org/c/coreboot/+/63198/comment/2e0ef84b_c2b62737 PS5, Line 125:
same as above
Ack
https://review.coreboot.org/c/coreboot/+/63198/comment/77675418_8fa04e1c PS5, Line 161: pcr_write32(PID_DMI, PCR_DMI_LPCGMR, lgmr);
same as above
Ack
https://review.coreboot.org/c/coreboot/+/63198/comment/88e24c5f_b3e305b7 PS5, Line 265: }
same as above
Ack
File src/soc/intel/common/block/smbus/tco.c:
https://review.coreboot.org/c/coreboot/+/63198/comment/0c49e3ea_29e22587 PS5, Line 126: "
Remove stray "
Ack
https://review.coreboot.org/c/coreboot/+/63198/comment/4b86d933_dc894415 PS5, Line 130: pcr_write32(PID_DMI, PCR_DMI_TCOBASE, tcobase | TCOEN);
use gpmr_write32
Ack
File src/soc/intel/common/pch/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/63198/comment/70bc97b7_88836f74 PS5, Line 31: if (CONFIG(SOC_INTEL_COMMON_BLOCK_IOC))
As per the coding style, this should have braces: https://doc.coreboot. […]
Ack
https://review.coreboot.org/c/coreboot/+/63198/comment/bf728e03_51f08318 PS5, Line 35: /* : * GCS reg of DMI : * : * When set, prevents GCS.BBS from being changed : * GCS.BBS: (Boot BIOS Strap) This field determines the destination : * of accesses to the BIOS memory range. : * Bits Description : * "0b": SPI : * "1b": LPC/eSPI : */ : pcr_or8(PID_DMI, PCR_DMI_GCS, PCR_DMI_GCS_BILD); : : /* : * Set Secure Register Lock (SRL) bit in DMI control register to lock : * DMI configuration. : */
Please indent the comments accordingly as well.
pcr_XXX(PID_DMI, function will be replaced with gpmr apis.
https://review.coreboot.org/c/coreboot/+/63198/comment/d9a2f7f7_7fa33f66 PS5, Line 53: }
you can create gmpr read_or8/32 function.
Sure we can make or32 or8 but PCR_DMI_DMICTL_SRLOCK is not exist in IOC.
-
Wonkyu Kim (Code Review)