Change in coreboot[master]: soc/intel/common/systemagent: Add SOC_INTEL_COMMON_BLOCK_SA_VERSION_2
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Add SOC_INTEL_COMMON_BLOCK_SA_VERSION_2 ......................................................................
soc/intel/common/systemagent: Add SOC_INTEL_COMMON_BLOCK_SA_VERSION_2
System Agent (SA) register 0x60:PCIEXBAR register LENGTH offset definition has been changed for newer SoC. This change provides a new Kconfig option that can be selected by SoCs using these new bit definitions of LENGTH. Common code takes care of setting the right value for pciex length depending upon the version selected by SOC.
TEST=DSDT dump shows PCIEXBAR.LENGTH offset (3:1) for TGL
Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c 4 files changed, 62 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/38456/1
diff --git a/src/soc/intel/common/block/acpi/acpi/northbridge.asl b/src/soc/intel/common/block/acpi/acpi/northbridge.asl index d271dda..ac9e843 100644 --- a/src/soc/intel/common/block/acpi/acpi/northbridge.asl +++ b/src/soc/intel/common/block/acpi/acpi/northbridge.asl @@ -40,8 +40,13 @@
Offset(0x60), /* PCIEXBAR (0:0:0:60) */ PXEN, 1, /* Enable */ - PXSZ, 2, /* PCI Express Size */ +#if CONFIG(SOC_INTEL_COMMON_BLOCK_SA_VERSION_2) + PXSZ, 3, /* PCI Express Size */ + , 22, +#else + PXSZ, 2, /* PCI Express Size */ , 23, +#endif PXBR, 6, /* PCI Express BAR [31:26] */
Offset(0x68), /* DMIBAR (0:0:0:68) */ @@ -241,7 +246,15 @@ /* Get PCIe Length */ Method (GPCL, 0, Serialized) { +#if CONFIG(SOC_INTEL_COMMON_BLOCK_SA_VERSION_2) + If (LLess (_SB.PCI0.MCHC.PXSZ, 3)) { + ShiftRight (0x10000000, _SB.PCI0.MCHC.PXSZ, Local0) + } Else { + Store(0x10000000, Local0) + } +#else ShiftRight (0x10000000, _SB.PCI0.MCHC.PXSZ, Local0) +#endif Return (Local0) }
diff --git a/src/soc/intel/common/block/systemagent/Kconfig b/src/soc/intel/common/block/systemagent/Kconfig index 1222573..017d85e 100644 --- a/src/soc/intel/common/block/systemagent/Kconfig +++ b/src/soc/intel/common/block/systemagent/Kconfig @@ -3,12 +3,24 @@ help Intel Processor common System Agent support
+config SOC_INTEL_COMMON_BLOCK_SA_VERSION_2 + bool + default n + select SOC_INTEL_COMMON_BLOCK_SA + help + Intel Processor Common System Agent support version 2 to handle + DRAM changes introduced in TGL. + config MMCONF_BASE_ADDRESS hex default 0xe0000000
config SA_PCIEX_LENGTH hex + default 0x100000000 if (PCIEX_LENGTH_4096MB) + default 0x80000000 if (PCIEX_LENGTH_2048MB) + default 0x40000000 if (PCIEX_LENGTH_1024MB) + default 0x20000000 if (PCIEX_LENGTH_512MB) default 0x10000000 if (PCIEX_LENGTH_256MB) default 0x8000000 if (PCIEX_LENGTH_128MB) default 0x4000000 if (PCIEX_LENGTH_64MB) @@ -16,6 +28,22 @@ help This option allows you to select length of PCIEX region.
+if SOC_INTEL_COMMON_BLOCK_SA_VERSION_2 + +config PCIEX_LENGTH_4096MB + bool + +config PCIEX_LENGTH_2048MB + bool + +config PCIEX_LENGTH_1024MB + bool + +config PCIEX_LENGTH_512MB + bool + +endif + config PCIEX_LENGTH_256MB bool
diff --git a/src/soc/intel/common/block/systemagent/systemagent_def.h b/src/soc/intel/common/block/systemagent/systemagent_def.h index b89a10d..5f54c72 100644 --- a/src/soc/intel/common/block/systemagent/systemagent_def.h +++ b/src/soc/intel/common/block/systemagent/systemagent_def.h @@ -31,6 +31,12 @@ #define DPR_PRS (1 << 1) #define DPR_SIZE_MASK 0xff0
+#if CONFIG(SOC_INTEL_COMMON_BLOCK_SA_VERSION_2) +#define PCIEXBAR_LENGTH_4096MB 6 +#define PCIEXBAR_LENGTH_2048MB 5 +#define PCIEXBAR_LENGTH_1024MB 4 +#define PCIEXBAR_LENGTH_512MB 3 +#endif #define PCIEXBAR_LENGTH_64MB 2 #define PCIEXBAR_LENGTH_128MB 1 #define PCIEXBAR_LENGTH_256MB 0 diff --git a/src/soc/intel/common/block/systemagent/systemagent_early.c b/src/soc/intel/common/block/systemagent/systemagent_early.c index d6f129d..f0a867a 100644 --- a/src/soc/intel/common/block/systemagent/systemagent_early.c +++ b/src/soc/intel/common/block/systemagent/systemagent_early.c @@ -40,6 +40,20 @@
/* Get PCI Express Region Length */ switch (CONFIG_SA_PCIEX_LENGTH) { +#if CONFIG(SOC_INTEL_COMMON_BLOCK_SA_VERSION_2) + case 4096 * MiB: + pciexbar_length = PCIEXBAR_LENGTH_4096MB; + break; + case 2048 * MiB: + pciexbar_length = PCIEXBAR_LENGTH_2048MB; + break; + case 1024 * MiB: + pciexbar_length = PCIEXBAR_LENGTH_1024MB; + break; + case 512 * MiB: + pciexbar_length = PCIEXBAR_LENGTH_512MB; + break; +#endif case 256 * MiB: pciexbar_length = PCIEXBAR_LENGTH_256MB; break;
Hello Patrick Rudolph, Aamir Bohra, Wonkyu Kim, Maulik V Vaghela, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38456
to look at the new patch set (#2).
Change subject: soc/intel/common/systemagent: Add SOC_INTEL_COMMON_BLOCK_SA_VERSION_2 ......................................................................
soc/intel/common/systemagent: Add SOC_INTEL_COMMON_BLOCK_SA_VERSION_2
System Agent (SA) register 0x60:PCIEXBAR register LENGTH offset definition has been changed for newer SoC. This change provides a new Kconfig option that can be selected by SoCs using these new bit definitions of LENGTH. Common code takes care of setting the right value for pciex length depending upon the version selected by SOC.
TEST=DSDT dump shows PCIEXBAR.LENGTH offset (3:1) for TGL
Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c 4 files changed, 62 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/38456/2
Paul Menzel 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 ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38456/2/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/38456/2/src/soc/intel/common/block/... PS2, Line 49: #endif Can’t this be detected at run-time, and the table be created at run-time?
https://review.coreboot.org/c/coreboot/+/38456/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38456/2/src/soc/intel/common/block/... PS2, Line 44: (unsigned long) Why is this needed?
Subrata Banik 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 ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38456/2/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/38456/2/src/soc/intel/common/block/... PS2, Line 49: #endif
Can’t this be detected at run-time, and the table be created at run-time?
this is EDS register changes hence their is not way to detect using runtime.
Also i was referring to GSPI version 2 made by Furquan for similar reason during CNL
https://review.coreboot.org/c/coreboot/+/38456/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38456/2/src/soc/intel/common/block/... PS2, Line 44: (unsigned long)
Why is this needed?
4096 * MiB = 0x100000000 (which is > 32bit) hence Case is treating this as 0 (type casted as int), ignoring > 32bit (bit 33)
Also compilation error https://qa.coreboot.org/job/coreboot-gerrit/114754/testReport/junit/board/ch...
Hello Patrick Rudolph, Aamir Bohra, Wonkyu Kim, Maulik V Vaghela, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38456
to look at the new patch set (#3).
Change subject: soc/intel/common/systemagent: Add SOC_INTEL_COMMON_BLOCK_SA_VERSION_2 till 5 ......................................................................
soc/intel/common/systemagent: Add SOC_INTEL_COMMON_BLOCK_SA_VERSION_2 till 5
System Agent (SA) register 0x48:MCHBAR and 0x60:PCIEXBAR register LENGTH offset definition has been changed for each SoC. This change provides a new Kconfig option that can be selected by SoCs using these new bit definitions of LENGTH and MCHBAR.
Common code takes care of setting the right value for mchbar and pciex length depending upon the version selected by SOC.
TEST=DSDT dump shows PCIEXBAR.LENGTH offset (3:1) for TGL
Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/asrock/h110m/dsdt.asl M src/mainboard/facebook/monolith/dsdt.asl M src/mainboard/google/eve/dsdt.asl M src/mainboard/google/fizz/dsdt.asl M src/mainboard/google/glados/dsdt.asl M src/mainboard/google/poppy/dsdt.asl M src/mainboard/intel/kblrvp/dsdt.asl M src/mainboard/intel/kunimitsu/dsdt.asl M src/mainboard/intel/saddlebrook/dsdt.asl M src/mainboard/purism/librem_skl/dsdt.asl M src/mainboard/razer/blade_stealth_kbl/dsdt.asl M src/mainboard/supermicro/x11-lga1151-series/dsdt.asl M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig D src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/tigerlake/Kconfig 21 files changed, 137 insertions(+), 354 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/38456/3
Hello Patrick Rudolph, Aamir Bohra, Frans Hendriks, Wonkyu Kim, Maulik V Vaghela, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38456
to look at the new patch set (#4).
Change subject: soc/intel/common/systemagent: Add SOC_INTEL_COMMON_BLOCK_SA_VERSION_2 till 5 ......................................................................
soc/intel/common/systemagent: Add SOC_INTEL_COMMON_BLOCK_SA_VERSION_2 till 5
System Agent (SA) register 0x48:MCHBAR and 0x60:PCIEXBAR register LENGTH offset definition has been changed for each SoC. This change provides a new Kconfig option that can be selected by SoCs using these new bit definitions of LENGTH and MCHBAR.
Common code takes care of setting the right value for mchbar and pciex length depending upon the version selected by SOC.
TEST=DSDT dump shows PCIEXBAR.LENGTH offset (3:1) for TGL
Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/asrock/h110m/dsdt.asl M src/mainboard/facebook/monolith/dsdt.asl M src/mainboard/google/eve/dsdt.asl M src/mainboard/google/fizz/dsdt.asl M src/mainboard/google/glados/dsdt.asl M src/mainboard/google/poppy/dsdt.asl M src/mainboard/intel/kblrvp/dsdt.asl M src/mainboard/intel/kunimitsu/dsdt.asl M src/mainboard/intel/saddlebrook/dsdt.asl M src/mainboard/purism/librem_skl/dsdt.asl M src/mainboard/razer/blade_stealth_kbl/dsdt.asl M src/mainboard/supermicro/x11-lga1151-series/dsdt.asl M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig D src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/tigerlake/Kconfig 21 files changed, 140 insertions(+), 354 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/38456/4
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.
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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38456/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38456/4//COMMIT_MSG@7 PS4, Line 7: 2 till 5 The obvious question is: What does "SA version 1" correspond to? I guess it would correspond to HSW/BDW?
Subrata Banik 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38456/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38456/4//COMMIT_MSG@7 PS4, Line 7: 2 till 5
The obvious question is: What does "SA version 1" correspond to? I guess it would correspond to HSW/ […]
I was trying to avoid version_1 and trying to convey that SOC_INTEL_COMMON_BLOCK_SA is the base code for all those SoC, starting from APL till latest TGL. Anything changes from APL till now, divided into version.
But i like your way, converting those bits into Kconfig and assign those Kconfig from respected SOC so we can fill in correct value without bothering about version.
Let me modify this code
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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38456/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38456/4//COMMIT_MSG@7 PS4, Line 7: 2 till 5
I was trying to avoid version_1 and trying to convey that SOC_INTEL_COMMON_BLOCK_SA is the base code […]
Ah, I thought the versions were tied to hardware block revisions.
Subrata Banik 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38456/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38456/4//COMMIT_MSG@7 PS4, Line 7: 2 till 5
Ah, I thought the versions were tied to hardware block revisions.
its true that version is tied with HW revision too and those soc has different SA HW block as mentioned but i like the way you ask t configure
Subrata Banik 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:
(1 comment)
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 66: DIBR, 27, /* DMIBAR [38:12] */
This has been changed for all platforms. […]
true but i thought we can clean up all 64bit related ASL changes in this file itself. Having only 1 register change in Cb:38387 might not make sense?
Hello Patrick Rudolph, Angel Pons, Aamir Bohra, Frans Hendriks, Wonkyu Kim, Maulik V Vaghela, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38456
to look at the new patch set (#5).
Change subject: soc/intel/common/systemagent: Add SOC_INTEL_COMMON_BLOCK_SA_VERSION_2 till 5 ......................................................................
soc/intel/common/systemagent: Add SOC_INTEL_COMMON_BLOCK_SA_VERSION_2 till 5
System Agent (SA) register 0x48:MCHBAR and 0x60:PCIEXBAR register LENGTH offset definition has been changed for each SoC. This change provides a new Kconfig option that can be selected by SoCs using these new bit definitions of LENGTH and MCHBAR.
Common code takes care of setting the right value for mchbar and pciex length depending upon the version selected by SOC.
TEST=DSDT dump shows PCIEXBAR.LENGTH offset (3:1) for TGL
Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/asrock/h110m/dsdt.asl M src/mainboard/facebook/monolith/dsdt.asl M src/mainboard/google/eve/dsdt.asl M src/mainboard/google/fizz/dsdt.asl M src/mainboard/google/glados/dsdt.asl M src/mainboard/google/poppy/dsdt.asl M src/mainboard/intel/kblrvp/dsdt.asl M src/mainboard/intel/kunimitsu/dsdt.asl M src/mainboard/intel/saddlebrook/dsdt.asl M src/mainboard/purism/librem_skl/dsdt.asl M src/mainboard/razer/blade_stealth_kbl/dsdt.asl M src/mainboard/supermicro/x11-lga1151-series/dsdt.asl M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig D src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/tigerlake/Kconfig 21 files changed, 139 insertions(+), 358 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/38456/5
Subrata Banik 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 5:
(6 comments)
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
Done
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
Done
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
Done
https://review.coreboot.org/c/coreboot/+/38456/4/src/soc/intel/common/block/... PS4, Line 66: DIBR, 27, /* DMIBAR [38:12] */
true but i thought we can clean up all 64bit related ASL changes in this file itself. […]
Done
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. […]
Done
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). […]
Done
Hello Patrick Rudolph, Angel Pons, Aamir Bohra, Frans Hendriks, Wonkyu Kim, Maulik V Vaghela, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38456
to look at the new patch set (#6).
Change subject: soc/intel/common/systemagent: Add SOC_INTEL_COMMON_BLOCK_SA_VERSION_2 till 5 ......................................................................
soc/intel/common/systemagent: Add SOC_INTEL_COMMON_BLOCK_SA_VERSION_2 till 5
System Agent (SA) register 0x48:MCHBAR and 0x60:PCIEXBAR register LENGTH offset definition has been changed for each SoC. This change provides a new Kconfig option that can be selected by SoCs using these new bit definitions of LENGTH and MCHBAR.
Common code takes care of setting the right value for mchbar and pciex length depending upon the version selected by SOC.
TEST=DSDT dump shows PCIEXBAR.LENGTH offset (3:1) for TGL
Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/asrock/h110m/dsdt.asl M src/mainboard/facebook/monolith/dsdt.asl M src/mainboard/google/eve/dsdt.asl M src/mainboard/google/fizz/dsdt.asl M src/mainboard/google/glados/dsdt.asl M src/mainboard/google/poppy/dsdt.asl M src/mainboard/intel/kblrvp/dsdt.asl M src/mainboard/intel/kunimitsu/dsdt.asl M src/mainboard/intel/saddlebrook/dsdt.asl M src/mainboard/purism/librem_skl/dsdt.asl M src/mainboard/razer/blade_stealth_kbl/dsdt.asl M src/mainboard/supermicro/x11-lga1151-series/dsdt.asl M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig D src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/tigerlake/Kconfig 21 files changed, 149 insertions(+), 358 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/38456/6
Subrata Banik 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 6:
(1 comment)
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 of […]
are you telling to have a if check before case ? we already have CONFIG_SA_PCIEXBAR_LENGTH_BIT_WIDTH and value >2 means more than 256MB length.
Hello Patrick Rudolph, Angel Pons, Aamir Bohra, Frans Hendriks, Wonkyu Kim, Maulik V Vaghela, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38456
to look at the new patch set (#7).
Change subject: soc/intel/common/systemagent: Make common northbridge.asl ......................................................................
soc/intel/common/systemagent: Make common northbridge.asl
This patch make common northbridge.asl based on System Agent (SA) register analysis between SKL till TGL.
Major delta are 0x48:MCHBAR and 0x60:PCIEXBAR register LENGTH offset definition has been changed for each SoC.
This change provides a new Kconfig option that can be selected by SoCs using these new bit definitions of PCIEXBAR.LENGTH and MCHBAR.
Common code takes care of setting the right value for mchbar and pciex length depending upon those Kconfig.
TEST=DSDT dump shows PCIEXBAR.LENGTH offset (3:1) for TGL
Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/asrock/h110m/dsdt.asl M src/mainboard/facebook/monolith/dsdt.asl M src/mainboard/google/eve/dsdt.asl M src/mainboard/google/fizz/dsdt.asl M src/mainboard/google/glados/dsdt.asl M src/mainboard/google/poppy/dsdt.asl M src/mainboard/intel/kblrvp/dsdt.asl M src/mainboard/intel/kunimitsu/dsdt.asl M src/mainboard/intel/saddlebrook/dsdt.asl M src/mainboard/purism/librem_skl/dsdt.asl M src/mainboard/razer/blade_stealth_kbl/dsdt.asl M src/mainboard/supermicro/x11-lga1151-series/dsdt.asl M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/Kconfig D src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/tigerlake/Kconfig 20 files changed, 145 insertions(+), 362 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/38456/7
Hello Patrick Rudolph, Angel Pons, Aamir Bohra, Frans Hendriks, Wonkyu Kim, Maulik V Vaghela, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38456
to look at the new patch set (#8).
Change subject: soc/intel/common/systemagent: Make common northbridge.asl ......................................................................
soc/intel/common/systemagent: Make common northbridge.asl
This patch make common northbridge.asl based on System Agent (SA) register analysis between SKL till TGL.
Major delta are 0x48:MCHBAR and 0x60:PCIEXBAR register LENGTH offset definition has been changed for each SoC.
This change provides a new Kconfig option that can be selected by SoCs using these new bit definitions of PCIEXBAR.LENGTH and MCHBAR.
Common code takes care of setting the right value for mchbar and pciex length depending upon those Kconfig.
TEST=DSDT dump shows PCIEXBAR.LENGTH offset (3:1) for TGL
Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/asrock/h110m/dsdt.asl M src/mainboard/facebook/monolith/dsdt.asl M src/mainboard/google/eve/dsdt.asl M src/mainboard/google/fizz/dsdt.asl M src/mainboard/google/glados/dsdt.asl M src/mainboard/google/poppy/dsdt.asl M src/mainboard/intel/kblrvp/dsdt.asl M src/mainboard/intel/kunimitsu/dsdt.asl M src/mainboard/intel/saddlebrook/dsdt.asl M src/mainboard/purism/librem_skl/dsdt.asl M src/mainboard/razer/blade_stealth_kbl/dsdt.asl M src/mainboard/supermicro/x11-lga1151-series/dsdt.asl M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/Kconfig D src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/tigerlake/Kconfig 20 files changed, 145 insertions(+), 362 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/38456/8
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Make common northbridge.asl ......................................................................
Patch Set 8:
(2 comments)
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 […]
Ack
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) {
are you telling to have a if check before case ? we already have CONFIG_SA_PCIEXBAR_LENGTH_BIT_WIDTH […]
Ack
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Make common northbridge.asl ......................................................................
Patch Set 8: Code-Review+1
(5 comments)
It looks much better now, thanks!
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... PS8, Line 284: _BAS Both _BAS and _LEN get updated now, so this comment should be updated.
Also, I really like this approach. Even though I know very little ASL, it's very easy to understand :D
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... PS8, Line 12: default 14 I understand that, as most platforms use the same value, one would want to make it the default setting. But what happens if, when adding a new platform, somebody forgets to specify the correct value? Everything builds, but the used value can be incorrect.
IMHO, it would be a good idea to put invalid defaults here, so that each SoC has to define the correct value. That way, the build process can be aborted if an invalid configuration is detected. For example, zero: it is not a valid value, and is a common "default" value.
To implement this check, I would add to `soc/intel/common/block/systemagent/Makefile.inc`:
ifeq ($(CONFIG_SA_MCHBAR_GRANULARITY),0) $(error SA_MCHBAR_GRANULARITY value is invalid. SoC must provide this) endif
(this also applies to the other fields below)
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... PS8, Line 14: reserve reserve*d*
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... PS8, Line 32: reserve reserve*d*
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) {
Ack
Sorry if my previous message was confusing. I meant to say that, since all the values here are known at compile-time, the switch() statement could be avoided. In any case, this belongs to a separate change.
Hello Patrick Rudolph, Wim Vervoorn, Angel Pons, Aamir Bohra, Frans Hendriks, Wonkyu Kim, Maulik V Vaghela, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38456
to look at the new patch set (#9).
Change subject: soc/intel/common/systemagent: Make common northbridge.asl ......................................................................
soc/intel/common/systemagent: Make common northbridge.asl
This patch make common northbridge.asl based on System Agent (SA) register analysis between SKL till TGL.
Major delta are 0x48:MCHBAR and 0x60:PCIEXBAR register LENGTH offset definition has been changed for each SoC.
This change provides a new Kconfig option that can be selected by SoCs using these new bit definitions of PCIEXBAR.LENGTH and MCHBAR.
Common code takes care of setting the right value for mchbar and pciex length depending upon those Kconfig.
TEST=DSDT dump shows PCIEXBAR.LENGTH offset (3:1) for TGL
Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/asrock/h110m/dsdt.asl M src/mainboard/facebook/monolith/dsdt.asl M src/mainboard/google/eve/dsdt.asl M src/mainboard/google/fizz/dsdt.asl M src/mainboard/google/glados/dsdt.asl M src/mainboard/google/poppy/dsdt.asl M src/mainboard/intel/kblrvp/dsdt.asl M src/mainboard/intel/kunimitsu/dsdt.asl M src/mainboard/intel/saddlebrook/dsdt.asl M src/mainboard/purism/librem_skl/dsdt.asl M src/mainboard/razer/blade_stealth_kbl/dsdt.asl M src/mainboard/supermicro/x11-lga1151-series/dsdt.asl M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/Makefile.inc M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig D src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/tigerlake/Kconfig 22 files changed, 194 insertions(+), 357 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/38456/9
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Make common northbridge.asl ......................................................................
Patch Set 9:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... PS8, Line 284: _BAS
Both _BAS and _LEN get updated now, so this comment should be updated. […]
Ack
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... PS8, Line 12: default 14
I understand that, as most platforms use the same value, one would want to make it the default setti […]
Ack
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... PS8, Line 14: reserve
reserve*d*
Ack
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... PS8, Line 32: reserve
reserve*d*
Ack
Hello Patrick Rudolph, Wim Vervoorn, Angel Pons, Aamir Bohra, Frans Hendriks, Wonkyu Kim, Maulik V Vaghela, build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38456
to look at the new patch set (#10).
Change subject: soc/intel/common/systemagent: Make common northbridge.asl ......................................................................
soc/intel/common/systemagent: Make common northbridge.asl
This patch make common northbridge.asl based on System Agent (SA) register analysis between SKL till TGL.
Major delta are 0x48:MCHBAR and 0x60:PCIEXBAR register LENGTH offset definition has been changed for each SoC.
This change provides a new Kconfig option that can be selected by SoCs using these new bit definitions of PCIEXBAR.LENGTH and MCHBAR.
Common code takes care of setting the right value for mchbar and pciex length depending upon those Kconfig.
TEST=DSDT dump shows PCIEXBAR.LENGTH offset (3:1) for TGL
Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/asrock/h110m/dsdt.asl M src/mainboard/facebook/monolith/dsdt.asl M src/mainboard/google/eve/dsdt.asl M src/mainboard/google/fizz/dsdt.asl M src/mainboard/google/glados/dsdt.asl M src/mainboard/google/poppy/dsdt.asl M src/mainboard/intel/kblrvp/dsdt.asl M src/mainboard/intel/kunimitsu/dsdt.asl M src/mainboard/intel/saddlebrook/dsdt.asl M src/mainboard/purism/librem_skl/dsdt.asl M src/mainboard/razer/blade_stealth_kbl/dsdt.asl M src/mainboard/supermicro/x11-lga1151-series/dsdt.asl M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/Makefile.inc M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig D src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/tigerlake/Kconfig 22 files changed, 191 insertions(+), 360 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/38456/10
Hello Patrick Rudolph, Wim Vervoorn, Angel Pons, Aamir Bohra, Frans Hendriks, Wonkyu Kim, Maulik V Vaghela, build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38456
to look at the new patch set (#11).
Change subject: soc/intel/common/systemagent: Make common northbridge.asl ......................................................................
soc/intel/common/systemagent: Make common northbridge.asl
This patch make common northbridge.asl based on System Agent (SA) register analysis between SKL till TGL.
Major delta are 0x48:MCHBAR and 0x60:PCIEXBAR register LENGTH offset definition has been changed for each SoC.
This change provides a new Kconfig option that can be selected by SoCs using these new bit definitions of PCIEXBAR.LENGTH and MCHBAR.
Common code takes care of setting the right value for mchbar and pciex length depending upon those Kconfig.
TEST=DSDT dump shows PCIEXBAR.LENGTH offset (3:1) for TGL
Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/asrock/h110m/dsdt.asl M src/mainboard/facebook/monolith/dsdt.asl M src/mainboard/google/eve/dsdt.asl M src/mainboard/google/fizz/dsdt.asl M src/mainboard/google/glados/dsdt.asl M src/mainboard/google/poppy/dsdt.asl M src/mainboard/intel/kblrvp/dsdt.asl M src/mainboard/intel/kunimitsu/dsdt.asl M src/mainboard/intel/saddlebrook/dsdt.asl M src/mainboard/purism/librem_skl/dsdt.asl M src/mainboard/razer/blade_stealth_kbl/dsdt.asl M src/mainboard/supermicro/x11-lga1151-series/dsdt.asl M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/Makefile.inc M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig D src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/tigerlake/Kconfig 22 files changed, 189 insertions(+), 360 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/38456/11
Hello Patrick Rudolph, Wim Vervoorn, Angel Pons, Aamir Bohra, Frans Hendriks, Wonkyu Kim, Maulik V Vaghela, build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38456
to look at the new patch set (#12).
Change subject: soc/intel/common/systemagent: Make common northbridge.asl ......................................................................
soc/intel/common/systemagent: Make common northbridge.asl
This patch make common northbridge.asl based on System Agent (SA) register analysis between SKL till TGL.
Major delta are 0x48:MCHBAR and 0x60:PCIEXBAR register LENGTH offset definition has been changed for each SoC.
This change provides a new Kconfig option that can be selected by SoCs using these new bit definitions of PCIEXBAR.LENGTH and MCHBAR.
Common code takes care of setting the right value for mchbar and pciex length depending upon those Kconfig.
TEST=DSDT dump shows PCIEXBAR.LENGTH offset (3:1) for TGL
Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/asrock/h110m/dsdt.asl M src/mainboard/facebook/monolith/dsdt.asl M src/mainboard/google/eve/dsdt.asl M src/mainboard/google/fizz/dsdt.asl M src/mainboard/google/glados/dsdt.asl M src/mainboard/google/poppy/dsdt.asl M src/mainboard/intel/kblrvp/dsdt.asl M src/mainboard/intel/kunimitsu/dsdt.asl M src/mainboard/intel/saddlebrook/dsdt.asl M src/mainboard/purism/librem_skl/dsdt.asl M src/mainboard/razer/blade_stealth_kbl/dsdt.asl M src/mainboard/supermicro/x11-lga1151-series/dsdt.asl M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/Makefile.inc M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig D src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/tigerlake/Kconfig 22 files changed, 189 insertions(+), 360 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/38456/12
Hello Patrick Rudolph, Wim Vervoorn, Angel Pons, Aamir Bohra, Frans Hendriks, Wonkyu Kim, Maulik V Vaghela, build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38456
to look at the new patch set (#13).
Change subject: soc/intel/common/systemagent: Make common northbridge.asl ......................................................................
soc/intel/common/systemagent: Make common northbridge.asl
This patch make common northbridge.asl based on System Agent (SA) register analysis between SKL till TGL.
Major delta are 0x48:MCHBAR and 0x60:PCIEXBAR register LENGTH offset definition has been changed for each SoC.
This change provides a new Kconfig option that can be selected by SoCs using these new bit definitions of PCIEXBAR.LENGTH and MCHBAR.
Common code takes care of setting the right value for mchbar and pciex length depending upon those Kconfig.
TEST=DSDT dump shows PCIEXBAR.LENGTH offset (3:1) for TGL
Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/asrock/h110m/dsdt.asl M src/mainboard/facebook/monolith/dsdt.asl M src/mainboard/google/eve/dsdt.asl M src/mainboard/google/fizz/dsdt.asl M src/mainboard/google/glados/dsdt.asl M src/mainboard/google/poppy/dsdt.asl M src/mainboard/intel/kblrvp/dsdt.asl M src/mainboard/intel/kunimitsu/dsdt.asl M src/mainboard/intel/saddlebrook/dsdt.asl M src/mainboard/purism/librem_skl/dsdt.asl M src/mainboard/razer/blade_stealth_kbl/dsdt.asl M src/mainboard/supermicro/x11-lga1151-series/dsdt.asl M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig D src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/tigerlake/Kconfig 21 files changed, 189 insertions(+), 360 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/38456/13
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Make common northbridge.asl ......................................................................
Patch Set 13: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/38456/13//COMMIT_MSG Commit Message:
PS13: Not sure if the commit message lines are a bit too long (should be 72 chars)
https://review.coreboot.org/c/coreboot/+/38456/13//COMMIT_MSG@7 PS13, Line 7: Make common northbridge.asl Make northbridge.asl common
https://review.coreboot.org/c/coreboot/+/38456/13//COMMIT_MSG@9 PS13, Line 9: make common unifies
Hello Patrick Rudolph, Wim Vervoorn, Angel Pons, Aamir Bohra, Frans Hendriks, Wonkyu Kim, Maulik V Vaghela, build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38456
to look at the new patch set (#14).
Change subject: soc/intel/common/systemagent: Make northbridge.asl common ......................................................................
soc/intel/common/systemagent: Make northbridge.asl common
This patch unifies northbridge.asl based on System Agent (SA) register analysis between SKL till TGL.
Major delta are 0x48:MCHBAR and 0x60:PCIEXBAR register LENGTH offset definition has been changed for each SoC.
This change provides a new Kconfig option that can be selected by SoCs using these new bit definitions of PCIEXBAR.LENGTH and MCHBAR.
Common code takes care of setting the right value for mchbar and pciex length depending upon those Kconfig.
TEST=DSDT dump shows PCIEXBAR.LENGTH offset (3:1) for TGL and ICL. DSDT dump shows PCIEXBAR.LENGTH offset (2:1) for SKL and CML
Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/asrock/h110m/dsdt.asl M src/mainboard/facebook/monolith/dsdt.asl M src/mainboard/google/eve/dsdt.asl M src/mainboard/google/fizz/dsdt.asl M src/mainboard/google/glados/dsdt.asl M src/mainboard/google/poppy/dsdt.asl M src/mainboard/intel/kblrvp/dsdt.asl M src/mainboard/intel/kunimitsu/dsdt.asl M src/mainboard/intel/saddlebrook/dsdt.asl M src/mainboard/purism/librem_skl/dsdt.asl M src/mainboard/razer/blade_stealth_kbl/dsdt.asl M src/mainboard/supermicro/x11-lga1151-series/dsdt.asl M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig D src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/tigerlake/Kconfig 21 files changed, 189 insertions(+), 360 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/38456/14
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Make northbridge.asl common ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38456/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38456/13//COMMIT_MSG@7 PS13, Line 7: Make common northbridge.asl
Make northbridge. […]
Ack
https://review.coreboot.org/c/coreboot/+/38456/13//COMMIT_MSG@9 PS13, Line 9: make common
unifies
Ack
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Make northbridge.asl common ......................................................................
Patch Set 14: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Make northbridge.asl common ......................................................................
Patch Set 14: Code-Review+2
(1 comment)
Thanks :)
https://review.coreboot.org/c/coreboot/+/38456/13//COMMIT_MSG Commit Message:
PS13:
Not sure if the commit message lines are a bit too long (should be 72 chars)
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Make northbridge.asl common ......................................................................
Patch Set 14:
(9 comments)
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... File src/soc/intel/common/block/acpi/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 200: #if !CONFIG(TPM_CR50) Commit message does not talk about this change.
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 241: ShiftLeft (1, Add (CONFIG_SA_MCHBAR_GRANULARITY, 1), Local0) Just use ASL 2.0 syntax?
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 248: ShiftLeft (_SB.PCI0.MCHC.MHBR, Add (CONFIG_SA_MCHBAR_GRANULARITY, 1), Local0) Use ASL 2.0 syntax?
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 269: If (LLess (_SB.PCI0.MCHC.PXSZ, 3)) { : ShiftRight (0x10000000, _SB.PCI0.MCHC.PXSZ, Local0) : } Else { : Store(0x10000000, Local0) : } Shouldn't this just return CONFIG_SA_PCIEX_LENGTH?
https://review.coreboot.org/c/coreboot/+/38456/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/38456/2/src/soc/intel/common/block/... PS2, Line 6: SOC_INTEL_COMMON_BLOCK_SA_VERSION_2 To be very honest, I think it would be best if we can avoid this. It becomes really tricky managing versions for these IPs especially when they are not really tied to visible hardware version.
https://review.coreboot.org/c/coreboot/+/38456/2/src/soc/intel/common/block/... PS2, Line 18: SA_PCIEX_LENGTH Can we change this to SA_PCIEX_LENGTH_MIB and then use the values in MiB for different lengths.Then you won't need the cast in systemagent_early.c
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 14: This field corresponds to the reserved bits (bit 1 onwards) of MCHBAR register. Sorry, but I am really confused by this explanation. Does this mean number/count of reserved bits in MCHBAR register?
Wouldn't it be easier to have SA_MCHBAR_BIT_OFFSET indicating the bit offset where the MCHBAR field is supposed to start within the register?
Looking at northbridge.asl, I see that you are using this field to indicate number of reserved bits before you can get to MCHBAR. With SA_MCHBAR_BIT_OFFSET, I think you can still achieve the same by using SA_MCHBAR_BIT_OFFSET - 1. It just makes it easier to compare the Kconfigs with the EDS register.
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 28: This field corresponds to the length of PCIEXBAR region. This is not completely correct. This Kconfig basically indicates the "*width of the* field that represents the length of PCIEXBAR region". Length of PCIEXBAR region is provided by SA_PCIEX_LENGTH.
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 35: This field corresponds to the reserved bits (after pciex length) of PCIEXBAR register. Same comment as above about using BIT_OFFSET instead of GRANULARITY.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Make northbridge.asl common ......................................................................
Patch Set 14:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 42: CONFIG_SA_PCIEX_LENGTH nit: You can just divide this by MiB and then you wouldn't need UL or ULL or typecase for the different cases.
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 43: CONFIG_SA_PCIEXBAR_LENGTH_BIT_WIDTH >= PCIEXBAR_LENGTH_512MB This is a weird check.
Currently, CONFIG_SA_PCIEXBAR_LENGTH_BIT_WIDTH can be 2 or 3 which indicates the number of bits that the field has within the register. That is being compared with PCIEXBAR_LENGTH_512MB which is the value of that field when length is 512MiB. I understand that PCIEXBAR_LENGTH_512MB happens to be 3, but what should be really checked here is that the width of the field is 3 so that it supports length values greater than 256MiB.
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 44: ULL This is unsigned long long and the typecast is for unsigned long?
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Make northbridge.asl common ......................................................................
Patch Set 14: Code-Review+2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Make northbridge.asl common ......................................................................
Patch Set 14:
(12 comments)
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... File src/soc/intel/common/block/acpi/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 200: #if !CONFIG(TPM_CR50)
Commit message does not talk about this change.
Ack
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 241: ShiftLeft (1, Add (CONFIG_SA_MCHBAR_GRANULARITY, 1), Local0)
Just use ASL 2. […]
Furquan, was thinking if we can make ASL2.0 syntax conversion for entire file at one shot after this common CL goes in. what is your thought ?
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 248: ShiftLeft (_SB.PCI0.MCHC.MHBR, Add (CONFIG_SA_MCHBAR_GRANULARITY, 1), Local0)
Use ASL 2. […]
same as above ?
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 269: If (LLess (_SB.PCI0.MCHC.PXSZ, 3)) { : ShiftRight (0x10000000, _SB.PCI0.MCHC.PXSZ, Local0) : } Else { : Store(0x10000000, Local0) : }
Shouldn't this just return CONFIG_SA_PCIEX_LENGTH?
Ack
https://review.coreboot.org/c/coreboot/+/38456/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/38456/2/src/soc/intel/common/block/... PS2, Line 6: SOC_INTEL_COMMON_BLOCK_SA_VERSION_2
To be very honest, I think it would be best if we can avoid this. […]
Ack
https://review.coreboot.org/c/coreboot/+/38456/2/src/soc/intel/common/block/... PS2, Line 18: SA_PCIEX_LENGTH
Can we change this to SA_PCIEX_LENGTH_MIB and then use the values in MiB for different lengths. […]
Ack
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 14: This field corresponds to the reserved bits (bit 1 onwards) of MCHBAR register.
Sorry, but I am really confused by this explanation. […]
Understood and tried the same implementation but i'm expected to see an error like this on CML-Hatch
coreboot toolchain vc3c9afb 2019-02-03 Compiler aborting due to parser-detected syntax error(s) dsdt.asl 140: , (15 - 1), Error 6126 - ^ syntax error, unexpected PARSEOP_OPEN_PAREN, expecting PARSEOP_INTEGER
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 28: This field corresponds to the length of PCIEXBAR region.
This is not completely correct. […]
Ack
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 35: This field corresponds to the reserved bits (after pciex length) of PCIEXBAR register.
Same comment as above about using BIT_OFFSET instead of GRANULARITY.
coreboot toolchain vc3c9afb 2019-02-03 Compiler aborting due to parser-detected syntax error(s) dsdt.asl 144: , (26 - 2 - 1), Error 6126 - ^ syntax error, unexpected PARSEOP_OPEN_PAREN, expecting PARSEOP_INTEGER
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 42: CONFIG_SA_PCIEX_LENGTH
nit: You can just divide this by MiB and then you wouldn't need UL or ULL or typecase for the differ […]
Ack
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 43: CONFIG_SA_PCIEXBAR_LENGTH_BIT_WIDTH >= PCIEXBAR_LENGTH_512MB
This is a weird check. […]
Ack
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 44: ULL
This is unsigned long long and the typecast is for unsigned long?
Ack
Hello Patrick Rudolph, Wim Vervoorn, Angel Pons, Aamir Bohra, Frans Hendriks, Wonkyu Kim, Maulik V Vaghela, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38456
to look at the new patch set (#15).
Change subject: soc/intel/common/systemagent: Make northbridge.asl common ......................................................................
soc/intel/common/systemagent: Make northbridge.asl common
This patch unifies northbridge.asl based on System Agent (SA) register analysis between SKL till TGL.
Major delta are 0x48:MCHBAR and 0x60:PCIEXBAR register LENGTH offset definition has been changed for each SoC.
This change provides a new Kconfig option that can be selected by SoCs using these new bit definitions of PCIEXBAR.LENGTH and MCHBAR.
Common code takes care of setting the right value for mchbar and pciex length depending upon those Kconfig.
Additionally only reserve ACPI TPM area for !CONFIG_TPM_CR50 device.
Rename CONFIG_PCIEX_LENGTH to CONFIG_PCIEX_LENGTH_MIB to avoid ugly typecasting.
TEST=DSDT dump shows PCIEXBAR.LENGTH offset (3:1) for TGL and ICL. DSDT dump shows PCIEXBAR.LENGTH offset (2:1) for SKL and CML
Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/asrock/h110m/dsdt.asl M src/mainboard/facebook/monolith/dsdt.asl M src/mainboard/google/eve/dsdt.asl M src/mainboard/google/fizz/dsdt.asl M src/mainboard/google/glados/dsdt.asl M src/mainboard/google/poppy/dsdt.asl M src/mainboard/intel/kblrvp/dsdt.asl M src/mainboard/intel/kunimitsu/dsdt.asl M src/mainboard/intel/saddlebrook/dsdt.asl M src/mainboard/purism/librem_skl/dsdt.asl M src/mainboard/razer/blade_stealth_kbl/dsdt.asl M src/mainboard/supermicro/x11-lga1151-series/dsdt.asl M src/soc/intel/apollolake/systemagent.c M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/block/acpi/acpi.c M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/Kconfig M src/soc/intel/icelake/systemagent.c M src/soc/intel/skylake/Kconfig M src/soc/intel/skylake/acpi.c D src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/skylake/systemagent.c M src/soc/intel/tigerlake/Kconfig M src/soc/intel/tigerlake/systemagent.c 28 files changed, 220 insertions(+), 384 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/38456/15
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Make northbridge.asl common ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 14: This field corresponds to the reserved bits (bit 1 onwards) of MCHBAR register.
Understood and tried the same implementation but i'm expected to see an error like this on CML-Hatch […]
Refer to error https://qa.coreboot.org/job/coreboot-gerrit/115392/testReport/junit/board/ch...
Hello Patrick Rudolph, Wim Vervoorn, Angel Pons, Aamir Bohra, Frans Hendriks, Wonkyu Kim, Maulik V Vaghela, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38456
to look at the new patch set (#16).
Change subject: soc/intel/common/systemagent: Make northbridge.asl common ......................................................................
soc/intel/common/systemagent: Make northbridge.asl common
This patch unifies northbridge.asl based on System Agent (SA) register analysis between SKL till TGL.
Major delta are 0x48:MCHBAR and 0x60:PCIEXBAR register LENGTH offset definition has been changed for each SoC.
This change provides a new Kconfig option that can be selected by SoCs using these new bit definitions of PCIEXBAR.LENGTH and MCHBAR.
Common code takes care of setting the right value for mchbar and pciex length depending upon those Kconfig.
Additionally only reserve ACPI TPM area for !CONFIG_TPM_CR50 device.
Rename CONFIG_PCIEX_LENGTH to CONFIG_PCIEX_LENGTH_MIB to avoid ugly typecasting.
TEST=DSDT dump shows PCIEXBAR.LENGTH offset (3:1) for TGL and ICL. DSDT dump shows PCIEXBAR.LENGTH offset (2:1) for SKL and CML
Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/asrock/h110m/dsdt.asl M src/mainboard/facebook/monolith/dsdt.asl M src/mainboard/google/eve/dsdt.asl M src/mainboard/google/fizz/dsdt.asl M src/mainboard/google/glados/dsdt.asl M src/mainboard/google/poppy/dsdt.asl M src/mainboard/intel/kblrvp/dsdt.asl M src/mainboard/intel/kunimitsu/dsdt.asl M src/mainboard/intel/saddlebrook/dsdt.asl M src/mainboard/purism/librem_skl/dsdt.asl M src/mainboard/razer/blade_stealth_kbl/dsdt.asl M src/mainboard/supermicro/x11-lga1151-series/dsdt.asl M src/soc/intel/apollolake/systemagent.c M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/block/acpi/acpi.c M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/Kconfig M src/soc/intel/icelake/systemagent.c M src/soc/intel/skylake/Kconfig M src/soc/intel/skylake/acpi.c D src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/skylake/systemagent.c M src/soc/intel/tigerlake/Kconfig M src/soc/intel/tigerlake/systemagent.c 28 files changed, 224 insertions(+), 384 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/38456/16
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Make northbridge.asl common ......................................................................
Patch Set 16:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... File src/soc/intel/common/block/acpi/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 241: ShiftLeft (1, Add (CONFIG_SA_MCHBAR_GRANULARITY, 1), Local0)
Furquan, was thinking if we can make ASL2. […]
Ack. Sounds okay to me.
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 14: This field corresponds to the reserved bits (bit 1 onwards) of MCHBAR register.
Refer to error […]
Ah yes. FieldUnit allows only constant values. Humm...
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Make northbridge.asl common ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 14: This field corresponds to the reserved bits (bit 1 onwards) of MCHBAR register.
Ah yes. FieldUnit allows only constant values. Humm...
yeah, so should i roll back to previous patchset Kconfig like "reserve bit width" rather starting offset ?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Make northbridge.asl common ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 14: This field corresponds to the reserved bits (bit 1 onwards) of MCHBAR register.
yeah, so should i roll back to previous patchset Kconfig like "reserve bit width" rather starting of […]
Subrata -- I know I had asked this before. Any reason why northbridge.asl cannot be moved to runtime generation? I think that will drastically simplify the whole generation. There will not be any need for bit width/reserved width (saves a number of Kconfigs that need to be added). Also, simplifies handling of things like TPM spaces needed only with certain CONFIGs defined. What are your thoughts? I think the overall change would look very straightforward.
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38456?usp=email )
Change subject: soc/intel/common/systemagent: Make northbridge.asl common ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Angel Pons (Code Review) -
Frans Hendriks (Code Review)
-
Furquan Shaikh (Code Review)
-
Martin L Roth (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)
-
Wim Vervoorn (Code Review)