Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42289 )
Change subject: soc/intel/xeon_sp: Enable PMC support ......................................................................
Patch Set 6:
(4 comments)
Patch Set 6:
(3 comments)
It looks like pmc.c has some ROMSTAGE ifdefs. I don't think that they are needed. I'm not sure why they are in the Skylake pmc.c, but they are not in the others socs. Maybe it was a SMI/powerbutton need for chromebook, but this patch doesn't seem to need it.
I don't see anything about romstage. This will eventually be used in ramstage and SMM, and SMM uses SIMPLE_DEVICE. SMM for Xeon-SP isn't set up yet. It needs a new version of the SMM module loader, because there's lots of threads. CB:43684 added this new SMM module loader.
As to why only Skylake has the ifdefs, I guess it's because the PMC PCI device wasn't hidden yet. From Cannon/Coffee Lake onwards, it is hidden by FSP and cannot be discovered through PCI enumeration. Yes, this has only caused massive headaches for everyone, but hardware is cursed and undocumented...
https://review.coreboot.org/c/coreboot/+/42289/6/src/soc/intel/xeon_sp/pmc.c File src/soc/intel/xeon_sp/pmc.c:
https://review.coreboot.org/c/coreboot/+/42289/6/src/soc/intel/xeon_sp/pmc.c... PS6, Line 21: #if defined(__SIMPLE_DEVICE__)
Not needed if not built for ROMSTAGE
This file will be built in SMM at some point, that's why there's so many guards. We don't need to guard this, though: just drop the `dev` variable.
https://review.coreboot.org/c/coreboot/+/42289/6/src/soc/intel/xeon_sp/pmc.c... PS6, Line 27: dev PCH_DEV_PMC
https://review.coreboot.org/c/coreboot/+/42289/6/src/soc/intel/xeon_sp/pmc.c... PS6, Line 32: dev PCH_DEV_PMC
https://review.coreboot.org/c/coreboot/+/42289/6/src/soc/intel/xeon_sp/pmc.c... PS6, Line 35: ENV_RAMSTAGE
ROMSTAGE?
No, this is correct. These files are never compiled for romstage.