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 5: Code-Review+1
(12 comments)
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/inclu... File src/soc/intel/xeon_sp/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/inclu... PS5, Line 36: double blank line
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 11: double blank line
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/inclu... PS5, Line 93: __packed Does this need to be packed?
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/inclu... File src/soc/intel/xeon_sp/include/soc/pmc.h:
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/inclu... PS5, Line 33: Why this empty line?
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/inclu... PS5, Line 40: * nit: spaces around the *
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 21: #if defined(__SIMPLE_DEVICE__) : const pci_devfn_t dev = PCH_DEV_PMC; : #else This shouldn't be needed, this file is only compiled in ramstage.
In any case, using PCH_DEV_PMC directly avoids the need of preprocessor
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/pmc.c... PS5, Line 35: #if ENV_RAMSTAGE This file is only built in ramstage, the guard isn't necessary
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/pmc.c... PS5, Line 99: reg32 = ALIGN_UP(reg32, 12); Up?
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/pmc.c... PS5, Line 121: This shouldn't be empty
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/pmc.c... PS5, Line 143: int prev_sleep_state) fits in the previous line
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/pmc.c... PS5, Line 212: Jenkins complains because there's multiple newlines here
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 */ Is this used somewhere? I think it's what's missing in soc_get_gpi_gpe_configs