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; -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 1 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-MessageType: newchange
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 2 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
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? -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 2 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 17 Jan 2020 10:16:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
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... -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 2 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 17 Jan 2020 10:25:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 3 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 4 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
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. -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 4 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 21 Jan 2020 11:55:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
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? -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 4 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 21 Jan 2020 12:05:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 4 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 21 Jan 2020 15:52:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Gerrit-MessageType: comment
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.
-- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 4 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Wed, 22 Jan 2020 00:03:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subrata.banik@intel.com> Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Gerrit-MessageType: comment
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
-- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 4 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Wed, 22 Jan 2020 03:10:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Comment-In-Reply-To: Subrata Banik <subrata.banik@intel.com> Gerrit-MessageType: comment
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?
-- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 4 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Wed, 22 Jan 2020 10:57:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 5 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
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
-- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 5 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Wed, 22 Jan 2020 12:36:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Comment-In-Reply-To: Subrata Banik <subrata.banik@intel.com> Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 6 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
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.
-- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 6 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Wed, 22 Jan 2020 16:33:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 7 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 8 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
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
-- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 8 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 23 Jan 2020 07:50:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Comment-In-Reply-To: Subrata Banik <subrata.banik@intel.com> Gerrit-MessageType: comment
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.
-- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 8 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 23 Jan 2020 13:25:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Subrata Banik <subrata.banik@intel.com> Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 9 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
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
-- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 9 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 23 Jan 2020 14:53:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 10 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 11 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 12 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 13 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 13 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 23 Jan 2020 23:33:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 14 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
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
-- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 14 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 24 Jan 2020 02:31:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 14 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 24 Jan 2020 08:27:40 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
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
-- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 14 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Sat, 25 Jan 2020 01:48:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Gerrit-MessageType: comment
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. -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 14 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Sun, 26 Jan 2020 22:36:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
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? -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 14 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Sun, 26 Jan 2020 23:33:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 14 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Mon, 27 Jan 2020 07:56:50 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
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
-- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 14 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Mon, 27 Jan 2020 10:08:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh <furquan@google.com> Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 15 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
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...
-- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 15 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Mon, 27 Jan 2020 10:58:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subrata.banik@intel.com> Comment-In-Reply-To: Furquan Shaikh <furquan@google.com> Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 16 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
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...
-- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 16 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 28 Jan 2020 02:46:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subrata.banik@intel.com> Comment-In-Reply-To: Furquan Shaikh <furquan@google.com> Gerrit-MessageType: comment
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 ?
-- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 16 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 28 Jan 2020 02:48:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subrata.banik@intel.com> Comment-In-Reply-To: Furquan Shaikh <furquan@google.com> Gerrit-MessageType: comment
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.
-- To view, visit https://review.coreboot.org/c/coreboot/+/38456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 16 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 28 Jan 2020 02:51:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subrata.banik@intel.com> Comment-In-Reply-To: Furquan Shaikh <furquan@google.com> Gerrit-MessageType: comment
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. -- To view, visit https://review.coreboot.org/c/coreboot/+/38456?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa00c4e6b872896ace975f1c6bd56c6efb172410 Gerrit-Change-Number: 38456 Gerrit-PatchSet: 16 Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Aamir Bohra <aamirbohra@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan.m.shaikh@gmail.com> Gerrit-Reviewer: Martin L Roth <gaumless@gmail.com> Gerrit-Reviewer: Maulik Vaghela <maulikvaghela@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> Gerrit-Reviewer: Wim Vervoorn <wvervoorn@eltan.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-MessageType: abandon
participants (7)
-
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)