Attention is currently required from: Jamie Ryu, Wonkyu Kim, Ethan Tsao, Ravishankar Sarawadi, Paul Menzel, Tim Wawrzynczak, Angel Pons, 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 12:
(6 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/63198/comment/684f2ee9_82d8f904 PS11, Line 11: https://github.com/otcshare/CCG-MTL-Generic-PSS/blob/master/
I get a 404 error, accessing that URL. Maybe mention, that it’s not public?
@will, don't refer which is not in public in commit msg, you can explain the design change in SoC, you don't need to refer to UEFI BIOS code.
IMO, starting with MTL, the GPMR access registers are migrated to IOC (I/O cache) instead DMI over PCR in previous generation, hence, added the required support in IA common code so that the SoC can select the required interface while programming the GPMR register.
File src/soc/intel/common/block/gpmr/gpmr.c:
https://review.coreboot.org/c/coreboot/+/63198/comment/6b895abf_bb1a450f PS12, Line 27: uint32_t data32; : : data32 = gpmr_read32(offset); : data32 |= ordata; : gpmr_write32(offset, data32); this also need change for IOC isn't it ?
File src/soc/intel/common/block/ioc/Kconfig:
https://review.coreboot.org/c/coreboot/+/63198/comment/4866dc92_9a0a4dac PS5, Line 1: SOC_INTEL_COMMON_BLOCK_IOC
Do we have KConfig for this?
may be SA Kconfig ?
File src/soc/intel/common/block/ioc/ioc.c:
https://review.coreboot.org/c/coreboot/+/63198/comment/aee05df6_19f4532b PS12, Line 15: read32 isn't I have suggested to use read32p ?
File src/soc/intel/common/pch/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/63198/comment/07d1729f_bcec6f37 PS12, Line 28: dmi_lockdown_cfg can you please try like this at it's source CL first? CB:63471
Initially, when you have just PCR interface then adjust it accordingly and add IOC interface later.
enum gpmr_register_access_interface { GPMR_OVER_PCR, GPMR_OVER_IOC, GPMR_OVER_BOTH, };
static void dmi_lockdown_cfg(void) { struct gpmr_info { enum gpmr_register_interface interface; uint16_t offset; uint32_t ordata; } info[] = { { .interface = GPMR_OVER_PCR, .offset = GPMR_GCS, .ordata = GPMR_GCS_BILD }. { .interface = GPMR_OVER_PCR, .offset = GPMR_DMICTL, .ordata = GPMR_DMICTL_SRLOCK }. };
for (size_t i = 0; I < ARRAY_SIZE(info); i++) { if (info[i].interface == GPMR_OVER_BOTH) gpmr_or32(info[i].offset, info[i].ordata); else if (info[i].interface == GPMR_OVER_PCR) pcr_or32(info[i].offset, info[i].ordata); else if (info[i].interface == GPMR_OVER_IOC) ioc_or32(info[i].offset, info[i].ordata); else // print error msg } }
https://review.coreboot.org/c/coreboot/+/63198/comment/69c3aab5_b6068664 PS12, Line 32: GPMR_DMICTL is that because you don't have `GPMR_DMICTL` for IOC ?