Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42289 )
Change subject: soc/intel/xeon_sp: Enable PMC support ......................................................................
Patch Set 4:
(6 comments)
https://review.coreboot.org/c/coreboot/+/42289/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42289/3//COMMIT_MSG@19 PS3, Line 19: select CPU_INTEL_COMMON_SMM
These Kconfigs are fundamental to a slew of features. Let's add them to soc/intel/xeon_sp/Kconfig.
Agreed. We have another patchset that will address the Kconfig options for SMM. PMC common code is heavily dependent on board/SOC code.
https://review.coreboot.org/c/coreboot/+/42289/3/src/soc/intel/xeon_sp/inclu... File src/soc/intel/xeon_sp/include/soc/pm.h:
https://review.coreboot.org/c/coreboot/+/42289/3/src/soc/intel/xeon_sp/inclu... PS3, Line 13: #define PM1_STS 0x00
For the xeon_sp command header file changes, please make sure they are same for SKX-SP and CPX-SP.
Will do. The PMC enabling will be the same for CPX.
https://review.coreboot.org/c/coreboot/+/42289/3/src/soc/intel/xeon_sp/pmc.c File src/soc/intel/xeon_sp/pmc.c:
https://review.coreboot.org/c/coreboot/+/42289/3/src/soc/intel/xeon_sp/pmc.c... PS3, Line 65: rtc_init();
rtc_init() is already called in romstage. […]
Good catch. This is now removed in the new patchset.
https://review.coreboot.org/c/coreboot/+/42289/3/src/soc/intel/xeon_sp/pmuti... File src/soc/intel/xeon_sp/pmutil.c:
https://review.coreboot.org/c/coreboot/+/42289/3/src/soc/intel/xeon_sp/pmuti... PS3, Line 81: reg32 &= ~0xfff;
We can use ALIGN_UP() here.
Done
https://review.coreboot.org/c/coreboot/+/42289/3/src/soc/intel/xeon_sp/pmuti... PS3, Line 119: PCH_DEV_PMC
Why not use `PCH_DEV_PMC` in the two places `dev` is used?
Agreed. Done... This code was taken from another project which was working so I took it as is. Good catch.
https://review.coreboot.org/c/coreboot/+/42289/4/src/soc/intel/xeon_sp/skx/c... File src/soc/intel/xeon_sp/skx/chip.h:
https://review.coreboot.org/c/coreboot/+/42289/4/src/soc/intel/xeon_sp/skx/c... PS4, Line 45: uint8_t gpe0_dw2; /* GPE0_95_64 STS/EN */
Is there another patch updating a mainboard (such as tiogapass)'s devicetree. […]
Yes, that's the plan. As soon as this is merged. I will submit another patch to enable it in Kconfig.