Marc Jones 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:
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 am trying to minimize ifdefs that do the same thing, especially on silicon. This should have less confusion as more silicon is added. There is a generic chip.h that includes the correct silicon file. The chip config files look like they are 95% the same, so common code can use the common structure names and the uncommon things should break at build time if there is a code mistake. It is harder to read and review definitions that are duplicated in each silicon version with the same file names and very similar code. After unwinding the SKx/CPX, there is very little difference, just a lot of things that have been moved around and renamed.
Going forward, common defines can be de-duplicated and moved here. I think that a common config struct that includes a silicon specific structure would work.
That said, this change is larger than the one specific need to check a config option in ACPI generation. The assumption is that there would be further de-duplication in the future.
The change we are fixing duplicating SILICON for each version in a function: #if SILICON const struct soc_intel_xeon_sp_SILICON_config *const config = config_of(device); #endif
... if (config->vtd_support)
A simpler fix is to ignore the silicon typedef as the other code does. I am going to remove the dependency on this patch group for now.