Marc Jones has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45969 )
Change subject: soc/intel/xeon_sp: Add common chip.h ......................................................................
soc/intel/xeon_sp: Add common chip.h
Add a common chip.h which includes the configured Xeon specific header file. Rename the Xeon specific header files. Common files should call chip.h while Xeon silicon specific files call their named header file. This should allow addition of new Xeon silicon.
This change prepares for additional common code changes.
Change-Id: I1c5e3dec3ee24328b1e987fdae856e9e77fe5499 Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/mainboard/ocp/deltalake/romstage.c A src/soc/intel/xeon_sp/chip.h M src/soc/intel/xeon_sp/cpx/acpi.c M src/soc/intel/xeon_sp/cpx/cpu.c M src/soc/intel/xeon_sp/cpx/romstage.c M src/soc/intel/xeon_sp/cpx/soc_acpi.c M src/soc/intel/xeon_sp/lpc.c M src/soc/intel/xeon_sp/skx/acpi.c M src/soc/intel/xeon_sp/skx/cpu.c M src/soc/intel/xeon_sp/skx/romstage.c M src/soc/intel/xeon_sp/skx/soc_acpi.c 11 files changed, 29 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/45969/1
diff --git a/src/mainboard/ocp/deltalake/romstage.c b/src/mainboard/ocp/deltalake/romstage.c index 71a26c8..a711601 100644 --- a/src/mainboard/ocp/deltalake/romstage.c +++ b/src/mainboard/ocp/deltalake/romstage.c @@ -8,7 +8,7 @@ #include <soc/romstage.h> #include <string.h>
-#include "chip.h" +#include <soc/intel/xeon_sp/chip.h> #include "ipmi.h" #include "vpd.h"
diff --git a/src/soc/intel/xeon_sp/chip.h b/src/soc/intel/xeon_sp/chip.h new file mode 100644 index 0000000..77d15643 --- /dev/null +++ b/src/soc/intel/xeon_sp/chip.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#ifndef _SOC_COMMON_CHIP_H_ +#define _SOC_COMMON_CHIP_H_ + +/* Common chip.h includes the approprate soc chip.h. */ + +#if (CONFIG(SOC_INTEL_SKYLAKE_SP)) + #include "skx/skx.h" +#endif + +#if (CONFIG(SOC_INTEL_COOPERLAKE_SP)) + #include "cpx/cpx.h" +#endif + +#endif diff --git a/src/soc/intel/xeon_sp/cpx/acpi.c b/src/soc/intel/xeon_sp/cpx/acpi.c index 93cd8ed..ea61c58 100644 --- a/src/soc/intel/xeon_sp/cpx/acpi.c +++ b/src/soc/intel/xeon_sp/cpx/acpi.c @@ -23,7 +23,7 @@ #include <soc/pm.h> #include <soc/soc_util.h>
-#include "chip.h" +#include "cpx.h"
static int acpi_sci_irq(void) { diff --git a/src/soc/intel/xeon_sp/cpx/cpu.c b/src/soc/intel/xeon_sp/cpx/cpu.c index eb8c0eb..25ad74b 100644 --- a/src/soc/intel/xeon_sp/cpx/cpu.c +++ b/src/soc/intel/xeon_sp/cpx/cpu.c @@ -15,7 +15,8 @@ #include <soc/cpu.h> #include <soc/msr.h> #include <soc/soc_util.h> -#include "chip.h" + +#include "cpx.h"
static const void *microcode_patch;
diff --git a/src/soc/intel/xeon_sp/cpx/romstage.c b/src/soc/intel/xeon_sp/cpx/romstage.c index a198c99..ee58313 100644 --- a/src/soc/intel/xeon_sp/cpx/romstage.c +++ b/src/soc/intel/xeon_sp/cpx/romstage.c @@ -4,7 +4,8 @@ #include <fsp/api.h> #include <soc/romstage.h> #include <soc/pci_devs.h> -#include "chip.h" + +#include "cpx.h"
void __weak mainboard_memory_init_params(FSPM_UPD *mupd) { diff --git a/src/soc/intel/xeon_sp/cpx/soc_acpi.c b/src/soc/intel/xeon_sp/cpx/soc_acpi.c index 4850855..038d7ae 100644 --- a/src/soc/intel/xeon_sp/cpx/soc_acpi.c +++ b/src/soc/intel/xeon_sp/cpx/soc_acpi.c @@ -17,7 +17,7 @@ #include <soc/pm.h> #include <soc/soc_util.h>
-#include "chip.h" +#include "cpx.h"
/* Check if the common/acpi weak function can be used */ unsigned long acpi_fill_mcfg(unsigned long current) diff --git a/src/soc/intel/xeon_sp/lpc.c b/src/soc/intel/xeon_sp/lpc.c index 3169545..5edfa4b 100644 --- a/src/soc/intel/xeon_sp/lpc.c +++ b/src/soc/intel/xeon_sp/lpc.c @@ -7,7 +7,7 @@ #include <soc/iomap.h> #include <soc/pcr_ids.h>
-#include <chip.h> +#include "chip.h"
static const struct lpc_mmio_range xeon_lpc_fixed_mmio_ranges[] = { { 0, 0 } diff --git a/src/soc/intel/xeon_sp/skx/acpi.c b/src/soc/intel/xeon_sp/skx/acpi.c index cbafbdb..8b2f9a1 100644 --- a/src/soc/intel/xeon_sp/skx/acpi.c +++ b/src/soc/intel/xeon_sp/skx/acpi.c @@ -15,7 +15,7 @@ #include <soc/pm.h> #include <string.h>
-#include "chip.h" +#include "skx.h"
acpi_cstate_t *soc_get_cstate_map(size_t *entries) { diff --git a/src/soc/intel/xeon_sp/skx/cpu.c b/src/soc/intel/xeon_sp/skx/cpu.c index ea9f531..3e7aeb3 100644 --- a/src/soc/intel/xeon_sp/skx/cpu.c +++ b/src/soc/intel/xeon_sp/skx/cpu.c @@ -10,7 +10,8 @@ #include <soc/cpu.h> #include <soc/soc_util.h> #include <assert.h> -#include "chip.h" + +#include "skx.h"
static const config_t *chip_config = NULL;
diff --git a/src/soc/intel/xeon_sp/skx/romstage.c b/src/soc/intel/xeon_sp/skx/romstage.c index a1c370d..93a6cd0 100644 --- a/src/soc/intel/xeon_sp/skx/romstage.c +++ b/src/soc/intel/xeon_sp/skx/romstage.c @@ -6,7 +6,7 @@ #include <soc/romstage.h> #include <soc/soc_util.h>
-#include "chip.h" +#include "skx.h"
void platform_fsp_memory_init_params_cb(FSPM_UPD *mupd, uint32_t version) { diff --git a/src/soc/intel/xeon_sp/skx/soc_acpi.c b/src/soc/intel/xeon_sp/skx/soc_acpi.c index c116506..fedb83c 100644 --- a/src/soc/intel/xeon_sp/skx/soc_acpi.c +++ b/src/soc/intel/xeon_sp/skx/soc_acpi.c @@ -18,7 +18,7 @@ #include <soc/pm.h> #include <soc/soc_util.h>
-#include "chip.h" +#include "skx.h"
/* Check if the common/acpi weak function can be used */ unsigned long acpi_fill_mcfg(unsigned long current)
Hello build bot (Jenkins), Jonathan Zhang, Stefan Reinauer, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45969
to look at the new patch set (#2).
Change subject: soc/intel/xeon_sp: Add common chip.h ......................................................................
soc/intel/xeon_sp: Add common chip.h
Add a common chip.h which includes the configured Xeon specific header file. Rename the Xeon specific header files. Common files should call chip.h while Xeon silicon specific files call their named header file. This should allow addition of new Xeon silicon.
This change prepares for additional common code changes.
Change-Id: I1c5e3dec3ee24328b1e987fdae856e9e77fe5499 Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/mainboard/ocp/deltalake/romstage.c A src/soc/intel/xeon_sp/chip.h M src/soc/intel/xeon_sp/cpx/acpi.c M src/soc/intel/xeon_sp/cpx/cpu.c R src/soc/intel/xeon_sp/cpx/cpx.h M src/soc/intel/xeon_sp/cpx/romstage.c M src/soc/intel/xeon_sp/cpx/soc_acpi.c M src/soc/intel/xeon_sp/lpc.c M src/soc/intel/xeon_sp/skx/acpi.c M src/soc/intel/xeon_sp/skx/cpu.c M src/soc/intel/xeon_sp/skx/romstage.c R src/soc/intel/xeon_sp/skx/skx.h M src/soc/intel/xeon_sp/skx/soc_acpi.c 13 files changed, 29 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/45969/2
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: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/45969/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45969/2//COMMIT_MSG@11 PS2, Line 11: call include? use?
https://review.coreboot.org/c/coreboot/+/45969/2//COMMIT_MSG@11 PS2, Line 11: call same here
https://review.coreboot.org/c/coreboot/+/45969/2/src/soc/intel/xeon_sp/chip.... File src/soc/intel/xeon_sp/chip.h:
https://review.coreboot.org/c/coreboot/+/45969/2/src/soc/intel/xeon_sp/chip.... PS2, Line 10: endif : : #if nit: elif
Jonathan Zhang 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:
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.
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:
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.
Jonathan Zhang 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:
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.
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.
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.
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:
WIP - look at combining chip config ops.
Hello build bot (Jenkins), Jonathan Zhang, Stefan Reinauer, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45969
to look at the new patch set (#3).
Change subject: [WIP]soc/intel/xeon_sp: Add common chip.h ......................................................................
[WIP]soc/intel/xeon_sp: Add common chip.h
Add a common chip.h which includes the configured Xeon specific header file. Rename the Xeon specific header files. Common files should call chip.h while Xeon silicon specific files call their named header file. This should allow addition of new Xeon silicon.
This change prepares for additional common code changes.
Change-Id: I1c5e3dec3ee24328b1e987fdae856e9e77fe5499 Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/mainboard/ocp/deltalake/romstage.c A src/soc/intel/xeon_sp/chip.h M src/soc/intel/xeon_sp/cpx/acpi.c M src/soc/intel/xeon_sp/cpx/cpu.c R src/soc/intel/xeon_sp/cpx/cpx.h M src/soc/intel/xeon_sp/cpx/romstage.c M src/soc/intel/xeon_sp/cpx/soc_acpi.c M src/soc/intel/xeon_sp/lpc.c M src/soc/intel/xeon_sp/skx/acpi.c M src/soc/intel/xeon_sp/skx/cpu.c M src/soc/intel/xeon_sp/skx/romstage.c R src/soc/intel/xeon_sp/skx/skx.h M src/soc/intel/xeon_sp/skx/soc_acpi.c 13 files changed, 29 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/45969/3
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45969 )
Change subject: [WIP]soc/intel/xeon_sp: Add common chip.h ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45969/3/src/soc/intel/xeon_sp/chip.... File src/soc/intel/xeon_sp/chip.h:
https://review.coreboot.org/c/coreboot/+/45969/3/src/soc/intel/xeon_sp/chip.... PS3, Line 6: chip.h ...
https://review.coreboot.org/c/coreboot/+/45969/3/src/soc/intel/xeon_sp/cpx/c... File src/soc/intel/xeon_sp/cpx/cpx.h:
https://review.coreboot.org/c/coreboot/+/45969/3/src/soc/intel/xeon_sp/cpx/c... PS3, Line 3: #ifndef _SOC_CHIP_H_ Is there a reason why this can't still be called chip.h? (If so, maybe rename the guard?)
Marc Jones has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/45969 )
Change subject: [WIP]soc/intel/xeon_sp: Add common chip.h ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Jonathan Zhang (Code Review)
-
Marc Jones (Code Review)
-
Stefan Reinauer (Code Review)