Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40308 )
Change subject: drivers/ocp/dmi: Add OCP_DMI driver for populating SMBIOS from IPMI FRU data ......................................................................
Patch Set 45:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40308/44//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40308/44//COMMIT_MSG@12 PS44, Line 12: Need to configure the correct values for FRU_DEVICE_ID and BMC_KCS_BASE.
"Mainboard needs to configure"
Done
https://review.coreboot.org/c/coreboot/+/40308/23/src/drivers/ocp/dmi/Kconfi... File src/drivers/ocp/dmi/Kconfig:
https://review.coreboot.org/c/coreboot/+/40308/23/src/drivers/ocp/dmi/Kconfi... PS23, Line 14: config BMC_KCS_BASE
For ramstage maybe I can add a global variable for it and sets it during ipmi_kcs_init() from dev->p […]
I add a CONFIG_BMC_KCS to https://review.coreboot.org/c/coreboot/+/40234/31/src/drivers/ipmi/Kconfig#2... and all code should use CONFIG_BMC_KCS whenever it needs a hard-coded address to avoid duplicate definitions.
https://review.coreboot.org/c/coreboot/+/40308/44/src/drivers/ocp/dmi/ocp_dm... File src/drivers/ocp/dmi/ocp_dmi.h:
https://review.coreboot.org/c/coreboot/+/40308/44/src/drivers/ocp/dmi/ocp_dm... PS44, Line 6: #include <cpu/x86/msr.h>
Do we need to include this header file?
This is needed because of extern msr_t xeon_sp_ppin[] below. I added ARCH_X86 dependency to https://review.coreboot.org/c/coreboot/+/40308/45/src/drivers/ocp/dmi/Kconfi... Because from the FB OCP specification it defines the PPIN requirement which should be ARCH_X86 dependent.
https://review.coreboot.org/c/coreboot/+/40308/44/src/drivers/ocp/dmi/ocp_dm... PS44, Line 15: extern msr_t xeon_sp_ppin[];
All the PPIN related code are x86 specific. […]
The PPIN in OCP spec is x86 dependent, so I still need put it here just to read and store them to a global array for lafter access from mainboard (SET PPIN to BMC). Unless it's better to move the read PPIN functions (maybe including the msr_t global array) to xeon_sp and call it here.