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: Code-Review+1
(5 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
Agreed. We have another patchset that will address the Kconfig options for SMM. […]
CB:45041 will select CPU_INTEL_COMMON_SMM, but the other two symbols still need to be selected somewhere (soc/intel/xeon_sp/Kconfig)
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/inclu... File src/soc/intel/xeon_sp/include/soc/pm.h:
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/inclu... PS5, Line 93: __packed
Probably not. But I kept it consistent with other projects in Coreboot.
Looks like it was marked as packed at some point. With the current data layout, packing shouldn't make any differences. I don't really mind
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/pmc.c File src/soc/intel/xeon_sp/pmc.c:
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/pmc.c... PS5, Line 99: reg32 = ALIGN_UP(reg32, 12);
Actually, alignment is not even necessary. I borrowed this code from another project. […]
I see that bit 0 is hardwired to zero already (indicates this base address corresponds to a memory space).
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/pmc.c... PS5, Line 121:
It's required by PMC common code, else it won't compile. In Xeon we have no functionality for this. […]
I see that you dropped the GPE stuff and added a comment here. Looks good to me.
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/skx/c... File src/soc/intel/xeon_sp/skx/chip.h:
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/skx/c... PS5, Line 41: /* Gpio group routed to each dword of the GPE0 block. Values are : * of the form GPP_[A:G] or GPD. */ : uint8_t gpe0_dw0; /* GPE0_31_0 STS/EN */ : uint8_t gpe0_dw1; /* GPE0_63_32 STS/EN */ : uint8_t gpe0_dw2; /* GPE0_95_64 STS/EN */
Removed. In xeon we don't use GPE's for anything right now. […]
Ack