Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45969 )
Change subject: soc/intel/xeon_sp: Add common chip.h ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
To me, this way of organizing the code is more confusing. The chip.h and soc_intel_xeon_sp_ops are reloaded for different Xeon-SP processors. This will cause confusions for code review and debugging.
Another option is to keep different names (eg. soc_intel_xeon_sp_skx_ops and soc_intel_xeon_sp_cpx_ops), and use build time switch (ifdef) to pick appropriate struct for specific Xeon-SP processor.
I think the plan is to end up using the same struct for both flavors of Xeon-SP.
Realistically the flavors of Xeon-SP will not have same struct, due to FSP differences. Different processors of Xeon-SP are developed by different Intel product teams at different schedule, so the interfaces of FSPs will be different. Same struct is possible if we are okay with using a superset of configuration items in the struct.
Even though soc/intel uses the devicetree as a mirror for FSP configuration, this is not how the devicetree is meant to be used. While some FSP UPDs can be set through the devicetree, others are best set using Kconfig symbols, and others can be set depending on whether a PCI device is enabled or not. Differences in FSP UPDs can be handled in FSP-specific files, while keeping the same devicetree struct.