Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Add SOC_INTEL_COMMON_BLOCK_SA_VERSION_2 till 5 ......................................................................
Patch Set 4: Code-Review+1
(8 comments)
I would try to reduce #ifdef usage. An approach would be to define Kconfig symbols for the values that differ across the revisions of the SA FUB.
Also, for features present "from revision X onwards", the chain of ORed revisions will end up being too long. In that case I would use Kconfig symbols to indicate presence of a certain feature.
Note: I am not good at naming things, names are only suggestions.
https://review.coreboot.org/c/coreboot/+/38456/4/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/38456/4/src/soc/intel/common/block/... PS4, Line 39: 14 This could be a Kconfig symbol, for example: SA_MCHBAR_GRANULARITY
https://review.coreboot.org/c/coreboot/+/38456/4/src/soc/intel/common/block/... PS4, Line 40: 24 This could also be a Kconfig symbol: SA_MCHBAR_BIT_WIDTH
https://review.coreboot.org/c/coreboot/+/38456/4/src/soc/intel/common/block/... PS4, Line 54: PXSZ, 2, /* PCI Express Size */ : , 25, Same thing here
https://review.coreboot.org/c/coreboot/+/38456/4/src/soc/intel/common/block/... PS4, Line 66: DIBR, 27, /* DMIBAR [38:12] */ This has been changed for all platforms. I guess this belongs to CB:38387 ?
https://review.coreboot.org/c/coreboot/+/38456/4/src/soc/intel/common/block/... PS4, Line 240: 15 Oh, this number is SA_MCHBAR_GRANULARITY plus one. Not sure if it's better to add one here, or subtract one on the MHBR definition
https://review.coreboot.org/c/coreboot/+/38456/4/src/soc/intel/common/block/... PS4, Line 302: 0x08000 That seems to be the result of (1 << SA_MCHBAR_GRANULARITY). Not sure if ASL allows precalculating that though
https://review.coreboot.org/c/coreboot/+/38456/4/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/38456/4/src/soc/intel/common/block/... PS4, Line 55: SOC_INTEL_COMMON_BLOCK_SA_VERSION_4 || SOC_INTEL_COMMON_BLOCK_SA_VERSION_5 This can be replaced by a new boolean Kconfig symbol: SA_SUPPORTS_LARGE_PCIEX
It would default to n, and get selected by SA versions 4 and 5 (and later versions in the future)
https://review.coreboot.org/c/coreboot/+/38456/4/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38456/4/src/soc/intel/common/block/... PS4, Line 42: switch (CONFIG_SA_PCIEX_LENGTH) { Seeing this, maybe it's a good idea to define PCIEXBAR_LENGTH index in Kconfig, and calculate the offset in the code instead.