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 5:
(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
Done
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
Done
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/inclu... PS5, Line 93: __packed
Does this need to be packed?
Probably not. But I kept it consistent with other projects in Coreboot.
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?
Done...removed.
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/inclu... PS5, Line 40: *
nit: spaces around the *
Done
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. […]
I have a subsequent patch that needs this file to compiled in multiple areas (ramstage and SMM). Therefore, I need to keep it is.
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
Same comment as above.
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/pmc.c... PS5, Line 99: reg32 = ALIGN_UP(reg32, 12);
Up?
Actually, alignment is not even necessary. I borrowed this code from another project. The HW already takes care of this. Removed alignment and simplified this method.
https://review.coreboot.org/c/coreboot/+/42289/5/src/soc/intel/xeon_sp/pmc.c... PS5, Line 121:
This shouldn't be empty
It's required by PMC common code, else it won't compile. In Xeon we have no functionality for this. Not sure, what else to do besides leave it empty. Please advise.
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
Done
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
Done
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
Removed. In xeon we don't use GPE's for anything right now. It can be added later if anybody needs it. Removing for now.