Change in coreboot[master]: soc/intel/common/block/systemagent: Add choice option for PCIEX_LENGTH
Hello Usha P,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/40379
to review the following change.
Change subject: soc/intel/common/block/systemagent: Add choice option for PCIEX_LENGTH ......................................................................
soc/intel/common/block/systemagent: Add choice option for PCIEX_LENGTH
This patch adds choice option for PCIEX_LENGTH related Kconfig to avoid multiple selection from SoC Kconfig.
Change-Id: Icb61e9a0263c058726cc07442af1985a96bf37c2 Signed-off-by: Usha P usha.p@intel.com --- M src/soc/intel/common/block/systemagent/Kconfig 1 file changed, 12 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/40379/1
diff --git a/src/soc/intel/common/block/systemagent/Kconfig b/src/soc/intel/common/block/systemagent/Kconfig index 6dd1f3b..ea75d5b 100644 --- a/src/soc/intel/common/block/systemagent/Kconfig +++ b/src/soc/intel/common/block/systemagent/Kconfig @@ -18,14 +18,23 @@ help This option allows you to select length of PCIEX region.
+choice + prompt "Length of PCI Express Base Address Region" + default PCIEX_LENGTH_256MB + help + This is to provide new kconfig option that can be used to + select PCI Express Base Address Length. + config PCIEX_LENGTH_256MB - bool + bool "256 MiB"
config PCIEX_LENGTH_128MB - bool + bool "128 MiB"
config PCIEX_LENGTH_64MB - bool + bool "64 MiB" + +endchoice
config SA_ENABLE_IMR bool
Pratikkumar V Prajapati has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40379 )
Change subject: soc/intel/common/block/systemagent: Add choice option for PCIEX_LENGTH ......................................................................
Patch Set 1: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40379 )
Change subject: soc/intel/common/block/systemagent: Add choice option for PCIEX_LENGTH ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40379/1/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/40379/1/src/soc/intel/common/block/... PS1, Line 25: This is to provide new kconfig option Not needed?
https://review.coreboot.org/c/coreboot/+/40379/1/src/soc/intel/common/block/... PS1, Line 26: select PCI Express Base Address Length. It’d be great, if you described, what length should be used for what case.
Usha P has uploaded a new patch set (#2) to the change originally created by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/40379 )
Change subject: soc/intel/common/block/systemagent: Add choice option for PCIEX_LENGTH ......................................................................
soc/intel/common/block/systemagent: Add choice option for PCIEX_LENGTH
This patch adds choice option for PCIEX_LENGTH related Kconfig to avoid multiple selection from SoC Kconfig.
Change-Id: Icb61e9a0263c058726cc07442af1985a96bf37c2 Signed-off-by: Usha P usha.p@intel.com --- M src/soc/intel/common/block/systemagent/Kconfig 1 file changed, 14 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/40379/2
Usha P has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40379 )
Change subject: soc/intel/common/block/systemagent: Add choice option for PCIEX_LENGTH ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40379/1/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/40379/1/src/soc/intel/common/block/... PS1, Line 25: This is to provide new kconfig option
Not needed?
Done
https://review.coreboot.org/c/coreboot/+/40379/1/src/soc/intel/common/block/... PS1, Line 26: select PCI Express Base Address Length.
It’d be great, if you described, what length should be used for what case.
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40379 )
Change subject: soc/intel/common/block/systemagent: Add choice option for PCIEX_LENGTH ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40379/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/40379/2/src/soc/intel/common/block/... PS2, Line 12: config SA_PCIEX_LENGTH : hex : default 0x10000000 if (PCIEX_LENGTH_256MB) : default 0x8000000 if (PCIEX_LENGTH_128MB) : default 0x4000000 if (PCIEX_LENGTH_64MB) : default 0x10000000 : help : This option allows you to select length of PCIEX region. Should this not be a Kconfig option any longer then?
Hello build bot (Jenkins), Furquan Shaikh, Usha P, Pratikkumar V Prajapati, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40379
to look at the new patch set (#3).
Change subject: soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS ......................................................................
soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS
This patch performs below operations 1. Override CONFIG_PCI_SEGMENT_GROUPS if TCSS_PCIE_SEGMENT is selected 2. Remove unused PCIEXBAR_LENGTH 64/128MB 3. Replace all usage of CONFIG_SA_PCIEX_LENGTH with CONFIG_PCI_SEGMENT_GROUPS and PCIEXBAR_LENGTH_MIB
Change-Id: Icb61e9a0263c058726cc07442af1985a96bf37c2 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/apollolake/systemagent.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/block/acpi/acpi.c M src/soc/intel/common/block/include/intelblocks/systemagent.h 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/systemagent.c M src/soc/intel/jasperlake/systemagent.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/systemagent.c M src/soc/intel/tigerlake/systemagent.c 12 files changed, 29 insertions(+), 41 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/40379/3
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40379 )
Change subject: soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40379/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/40379/2/src/soc/intel/common/block/... PS2, Line 12: config SA_PCIEX_LENGTH : hex : default 0x10000000 if (PCIEX_LENGTH_256MB) : default 0x8000000 if (PCIEX_LENGTH_128MB) : default 0x4000000 if (PCIEX_LENGTH_64MB) : default 0x10000000 : help : This option allows you to select length of PCIEX region.
Should this not be a Kconfig option any longer then?
yes, i think choice might not be the right strategy here because it block freedom from SOC to select the right PCIEX length. like i might wish to select PCIEX_LENGTH_512MB from SoC code then choice won't allow that?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40379 )
Change subject: soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40379/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/40379/2/src/soc/intel/common/block/... PS2, Line 12: config SA_PCIEX_LENGTH : hex : default 0x10000000 if (PCIEX_LENGTH_256MB) : default 0x8000000 if (PCIEX_LENGTH_128MB) : default 0x4000000 if (PCIEX_LENGTH_64MB) : default 0x10000000 : help : This option allows you to select length of PCIEX region.
yes, i think choice might not be the right strategy here because it block freedom from SOC to select […]
Ack
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40379 )
Change subject: soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40379/3/src/soc/intel/apollolake/sy... File src/soc/intel/apollolake/systemagent.c:
https://review.coreboot.org/c/coreboot/+/40379/3/src/soc/intel/apollolake/sy... PS3, Line 35: "PCIEXBAR" }, Nit, while we are touching this, we could make it a single line (96 chars are allowed now).
https://review.coreboot.org/c/coreboot/+/40379/3/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/40379/3/src/soc/intel/common/block/... PS3, Line 13: int Nit, no need to specify the type for a default override, IIRC.
https://review.coreboot.org/c/coreboot/+/40379/3/src/soc/intel/common/block/... PS3, Line 12: config PCI_SEGMENT_GROUPS : int : default 2 if TCSS_PCIE_SEGMENT Please add this later in the patch series, around CB:41011 for instance.
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Usha P, Andrey Petrov, Pratikkumar V Prajapati, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40379
to look at the new patch set (#4).
Change subject: soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS ......................................................................
soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS
This patch performs below operations 1. Remove unused PCIEXBAR_LENGTH 64/128MB 2. Replace all usage of CONFIG_SA_PCIEX_LENGTH with CONFIG_PCI_SEGMENT_GROUPS and PCIEXBAR_LENGTH_MIB
Change-Id: Icb61e9a0263c058726cc07442af1985a96bf37c2 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/apollolake/systemagent.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/block/acpi/acpi.c M src/soc/intel/common/block/include/intelblocks/systemagent.h 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/systemagent.c M src/soc/intel/jasperlake/systemagent.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/systemagent.c M src/soc/intel/tigerlake/systemagent.c 12 files changed, 26 insertions(+), 48 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/40379/4
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40379 )
Change subject: soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40379/3/src/soc/intel/apollolake/sy... File src/soc/intel/apollolake/systemagent.c:
https://review.coreboot.org/c/coreboot/+/40379/3/src/soc/intel/apollolake/sy... PS3, Line 35: "PCIEXBAR" },
Nit, while we are touching this, we could make it a single line (96 chars are allowed now).
Ack
https://review.coreboot.org/c/coreboot/+/40379/3/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/40379/3/src/soc/intel/common/block/... PS3, Line 13: int
Nit, no need to specify the type for a default override, IIRC.
Ack
https://review.coreboot.org/c/coreboot/+/40379/3/src/soc/intel/common/block/... PS3, Line 12: config PCI_SEGMENT_GROUPS : int : default 2 if TCSS_PCIE_SEGMENT
Please add this later in the patch series, around CB:41011 for instance.
Ack
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40379 )
Change subject: soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40379/4/src/soc/intel/apollolake/sy... File src/soc/intel/apollolake/systemagent.c:
https://review.coreboot.org/c/coreboot/+/40379/4/src/soc/intel/apollolake/sy... PS4, Line 34: PCIEXBAR_LENGTH_MIB This value is supposed to be in bytes, right?
https://review.coreboot.org/c/coreboot/+/40379/4/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/40379/4/src/soc/intel/common/block/... PS4, Line 44: CONFIG_MMCONF_BASE_ADDRESS Is this supposed to be the same for all segments?
https://review.coreboot.org/c/coreboot/+/40379/4/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/40379/4/src/soc/intel/common/block/... PS4, Line 30: switch (CONFIG_PCI_SEGMENT_GROUPS) { : case 1: : pciexbar_length = PCIEXBAR_LENGTH_256MB; : break; I am a little confused here. Does the PCI segment group number determine the length? Can you please point me to some reference to understand this better?
https://review.coreboot.org/c/coreboot/+/40379/4/src/soc/intel/skylake/acpi.... File src/soc/intel/skylake/acpi.c:
https://review.coreboot.org/c/coreboot/+/40379/4/src/soc/intel/skylake/acpi.... PS4, Line 207: CONFIG_PCI_SEGMENT_GROUPS Does skylake support multiple segments?
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Usha P, Andrey Petrov, Pratikkumar V Prajapati, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40379
to look at the new patch set (#5).
Change subject: soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS ......................................................................
soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS
This patch performs below operations 1. Remove unused PCIEXBAR_LENGTH 64/128MB 2. Replace all usage of CONFIG_SA_PCIEX_LENGTH with CONFIG_PCI_SEGMENT_GROUPS and PCIEXBAR_LENGTH
Change-Id: Icb61e9a0263c058726cc07442af1985a96bf37c2 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/apollolake/systemagent.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/block/acpi/acpi.c M src/soc/intel/common/block/include/intelblocks/systemagent.h 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/systemagent.c M src/soc/intel/jasperlake/systemagent.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/systemagent.c M src/soc/intel/tigerlake/systemagent.c 12 files changed, 26 insertions(+), 48 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/40379/5
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40379 )
Change subject: soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40379/4/src/soc/intel/apollolake/sy... File src/soc/intel/apollolake/systemagent.c:
https://review.coreboot.org/c/coreboot/+/40379/4/src/soc/intel/apollolake/sy... PS4, Line 34: PCIEXBAR_LENGTH_MIB
This value is supposed to be in bytes, right?
very good catch.
https://review.coreboot.org/c/coreboot/+/40379/4/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/40379/4/src/soc/intel/common/block/... PS4, Line 44: CONFIG_MMCONF_BASE_ADDRESS
Is this supposed to be the same for all segments?
Yes please refer to PCIEXBAR_0_0_0_PCI
The address used to access the PCI Express configuration space for a specific device can be determined as follows: PCI Express Base Address +Segment Number*256MB+ Bus Number * 1MB + Device Number * 32KB + Function Number * 4KB
https://review.coreboot.org/c/coreboot/+/40379/4/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/40379/4/src/soc/intel/common/block/... PS4, Line 30: switch (CONFIG_PCI_SEGMENT_GROUPS) { : case 1: : pciexbar_length = PCIEXBAR_LENGTH_256MB; : break;
I am a little confused here. Does the PCI segment group number determine the length?
Its not likely that, but if i take an example of IA SoC where mostly 256 bus are referring to 1 segment.
if Bus is > 256, example: 257 then shouldn’t it like 257/256 = 1 refers to Segment 1 and Bus number is 257- (segment_no * 256) = 1
but there could be lesser buses also in a segment like 64/128.
In this process we are removing those unused 64/128 bus programming which was anyway not selected by common code
Can you please point me to some reference to understand this better?
You can refer to PCIEXBAR_0_0_0_PCI register as well.
https://review.coreboot.org/c/coreboot/+/40379/4/src/soc/intel/skylake/acpi.... File src/soc/intel/skylake/acpi.c:
https://review.coreboot.org/c/coreboot/+/40379/4/src/soc/intel/skylake/acpi.... PS4, Line 207: CONFIG_PCI_SEGMENT_GROUPS
Does skylake support multiple segments?
no.
in symmetry with common code (acpi.c) i have made this but eventually CONFIG_PCI_SEGMENT_GROUPS = 1 for all unless specified
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40379 )
Change subject: soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS ......................................................................
Patch Set 5: Code-Review+1
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40379 )
Change subject: soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40379/5/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/40379/5/src/soc/intel/common/block/... PS5, Line 30: switch (CONFIG_PCI_SEGMENT_GROUPS) { Do we need to switch on this at all? the mcfg region is only supporting segment 0 anyway. Just set the length to 256MB and not worry about it?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40379 )
Change subject: soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40379/5/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/40379/5/src/soc/intel/common/block/... PS5, Line 30: switch (CONFIG_PCI_SEGMENT_GROUPS) {
Do we need to switch on this at all? the mcfg region is only supporting segment 0 anyway. […]
Yes, you are right that for Segment 0 we are mostly setting 256 hence removed 64/128 unused configuration. But if we have 2 PCI segments 1. for all PCI device under host bridge 2. Dedicated for iTBT
In those case, don't we need to publish 2 dedicated MCFG table ? Looks like ACPI spec says so.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40379 )
Change subject: soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40379/5/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/40379/5/src/soc/intel/common/block/... PS5, Line 30: switch (CONFIG_PCI_SEGMENT_GROUPS) {
Yes, you are right that for Segment 0 we are mostly setting 256 hence removed 64/128 unused configur […]
Yes, we do. I was confused in that I thought this was just temporary and early -- not setting this for lifetime of being up.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40379 )
Change subject: soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS ......................................................................
Patch Set 5: Code-Review+1
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/40379?usp=email )
Change subject: soc/intel: Update SA common code based on CONFIG_PCI_SEGMENT_GROUPS ......................................................................
Abandoned
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Jonathan Zhang (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)
-
Pratikkumar V Prajapati (Code Review)
-
Stefan Reinauer (Code Review)
-
Subrata Banik (Code Review)
-
Usha P (Code Review)
-
Wonkyu Kim (Code Review)