Attention is currently required from: Jamie Ryu, Wonkyu Kim, Ethan Tsao, Ravishankar Sarawadi, Nick Vaccaro, Tim Wawrzynczak. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63170 )
Change subject: intel/common/block: move gpmr api to gpmr driver ......................................................................
Patch Set 9:
(1 comment)
Patchset:
PS9: At high-level, what you have done here is renaming the CB:47988 (soc/intel/common/dmi: Add DMI driver support) from DMI to GPMR.
I don't understand the purpose of this renaming. The DMI sideband access is used for all GPMR read and write.
Now as you have decided to rename DMI driver to GPMR then, take a look into the dependency over `SOC_INTEL_COMMON_BLOCK_DMI`
grep -rsn "SOC_INTEL_COMMON_BLOCK_DMI" src/1. src//soc/intel/common/pch/Kconfig:29: select SOC_INTEL_COMMON_BLOCK_DMI2. src//soc/intel/common/block/include/intelblocks/dmi.h:3:#ifndef SOC_INTEL_COMMON_BLOCK_DMI_H3. src//soc/intel/common/block/include/intelblocks/dmi.h:4:#define SOC_INTEL_COMMON_BLOCK_DMI_H4. src//soc/intel/common/block/include/intelblocks/dmi.h:20:#endif /* SOC_INTEL_COMMON_BLOCK_DMI_H */5. src//soc/intel/common/block/lpc/Kconfig:17: depends on SOC_INTEL_COMMON_BLOCK_DMI6. src//soc/intel/common/block/dmi/Kconfig:1:config SOC_INTEL_COMMON_BLOCK_DMI7. src//soc/intel/common/block/dmi/Makefile.inc:1:ifeq ($(CONFIG_SOC_INTEL_COMMON_BLOCK_DMI), y)
This changes already dropped #7, which compiled the `dmi.c` file for other IA common code for consumption. When you have introduced `SOC_INTEL_COMMON_BLOCK_GPMR` then why still have dependency over a Kconfig (https://review.coreboot.org/c/coreboot/+/63170/9/src/soc/intel/common/block/... have zero value with this code change. What I understood, is the dependency that you like to keep here (https://review.coreboot.org/c/coreboot/+/63170/9/src/soc/intel/common/block/... is SOC_INTEL_COMMON_BLOCK_PCR because you are accessing DMI over sideband hence PCR library is needed. Ideally #1 and #6 also should get dropped.
#5 dependency would not revised to SOC_INTEL_COMMON_BLOCK_GPMR instead SOC_INTEL_COMMON_BLOCK_DMI A better representation of CB:63170 IMO would be as below.
1. Add GPMR common driver in IA common code. 2. Migrate all DMI API usage to GPMR -- that includes changes in #5 above, https://review.coreboot.org/c/coreboot/+/63170/9/src/soc/intel/common/block/... etc. 3. Drop DMI driver from IA common code -- that includes #1 as above