Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39601 )
Change subject: soc/intel/xeon_sp: Refactor code to allow for additional CPUs types ......................................................................
Patch Set 6:
(3 comments)
Overall this looks good, thanks for the effort. Before we finish the bring-up of next generation xeon-sp processor based platform, we do not know what are the coreboot differences between skylake-sp and next generation xeon-sp. Most assumptions made in this patch on the differences will be corrected over the course. One approach is to start upstreaming once we know the differences at large. With this approach, we need to be prepared to adjust further along the way.
https://review.coreboot.org/c/coreboot/+/39601/6/src/soc/intel/xeon_sp/bootb... File src/soc/intel/xeon_sp/bootblock.c:
https://review.coreboot.org/c/coreboot/+/39601/6/src/soc/intel/xeon_sp/bootb... PS6, Line 38: , Removing of this is a clean-up, that should go to a separate patch. So this patch can focus on supporting additional CPU type.
https://review.coreboot.org/c/coreboot/+/39601/6/src/soc/intel/xeon_sp/inclu... File src/soc/intel/xeon_sp/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/39601/6/src/soc/intel/xeon_sp/inclu... PS6, Line 48: Addition of this code should go to a dedicated patch, so this patch can stick to its commit message.
https://review.coreboot.org/c/coreboot/+/39601/6/src/soc/intel/xeon_sp/skx/i... File src/soc/intel/xeon_sp/skx/include/soc/pmc.h:
https://review.coreboot.org/c/coreboot/+/39601/6/src/soc/intel/xeon_sp/skx/i... PS6, Line 2: * This file is part of the coreboot project. This file should be a rename from src/soc/intel/xeon_sp/include/soc/pmc.h, instead of a new file.