Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: src/arch/x86/acpi: Increase Max PCI bus count support ......................................................................
src/arch/x86/acpi: Increase Max PCI bus count support
This patch increase maximum bus end variable type to match latest SoC specification [Ice Lake EDS vol 1 chapter 3.18]
Change-Id: I90660c5cfd8af5bb40e36bb409e534541c786cae Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/acpi.c M src/arch/x86/include/arch/acpi.h 2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/40335/1
diff --git a/src/arch/x86/acpi.c b/src/arch/x86/acpi.c index 6eded1d..ccc24db 100644 --- a/src/arch/x86/acpi.c +++ b/src/arch/x86/acpi.c @@ -104,7 +104,7 @@ }
int acpi_create_mcfg_mmconfig(acpi_mcfg_mmconfig_t *mmconfig, u32 base, - u16 seg_nr, u8 start, u8 end) + u16 seg_nr, u8 start, u16 end) { memset(mmconfig, 0, sizeof(*mmconfig)); mmconfig->base_address = base; diff --git a/src/arch/x86/include/arch/acpi.h b/src/arch/x86/include/arch/acpi.h index 644f52f..82faa8f 100644 --- a/src/arch/x86/include/arch/acpi.h +++ b/src/arch/x86/include/arch/acpi.h @@ -871,7 +871,7 @@ int acpi_create_srat_mem(acpi_srat_mem_t *mem, u8 node, u32 basek, u32 sizek, u32 flags); int acpi_create_mcfg_mmconfig(acpi_mcfg_mmconfig_t *mmconfig, u32 base, - u16 seg_nr, u8 start, u8 end); + u16 seg_nr, u8 start, u16 end); unsigned long acpi_create_srat_lapics(unsigned long current); void acpi_create_srat(acpi_srat_t *srat, unsigned long (*acpi_fill_srat)(unsigned long current));
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: src/arch/x86/acpi: Increase Max PCI bus count support ......................................................................
Patch Set 1:
(3 comments)
I'm curious if there really are SoCs yet with multiple PCI segments? Is this commit series just for preparation or is there already a use case?
I don't think it's really needed and wonder if we can't normalize on 256MiB instead? All current SoCs seem to use that. Also, values >=2GiB naturally can't work with FSP, because it forces us into 32-bit mode.
https://review.coreboot.org/c/coreboot/+/40335/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40335/1//COMMIT_MSG@10 PS1, Line 10: SoC specification [Ice Lake EDS vol 1 chapter 3.18] ICL can address multiple PCI segments, but does it really have them?
https://review.coreboot.org/c/coreboot/+/40335/1//COMMIT_MSG@10 PS1, Line 10: 1 vol 2?
https://review.coreboot.org/c/coreboot/+/40335/1/src/arch/x86/acpi.c File src/arch/x86/acpi.c:
https://review.coreboot.org/c/coreboot/+/40335/1/src/arch/x86/acpi.c@114 PS1, Line 114: end_bus_number This is `u8` too and we can't change it. Having more than 256 buses means that there is more than one PCI segment, and this function should be called for each.
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Usha P, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40335
to look at the new patch set (#2).
Change subject: src/arch/x86/acpi: Increase Max PCI bus count support ......................................................................
src/arch/x86/acpi: Increase Max PCI bus count support
This patch increase maximum bus end variable type to match latest SoC specification [Ice Lake EDS vol 2 chapter 3.18]
Change-Id: I90660c5cfd8af5bb40e36bb409e534541c786cae Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/acpi.c M src/arch/x86/include/arch/acpi.h 2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/40335/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: src/arch/x86/acpi: Increase Max PCI bus count support ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40335/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40335/1//COMMIT_MSG@10 PS1, Line 10: SoC specification [Ice Lake EDS vol 1 chapter 3.18]
ICL can address multiple PCI segments, but does it really have them?
As per EDS, the formula out there
https://review.coreboot.org/c/coreboot/+/40335/1//COMMIT_MSG@10 PS1, Line 10: 1
vol 2?
Ack
https://review.coreboot.org/c/coreboot/+/40335/1/src/arch/x86/acpi.c File src/arch/x86/acpi.c:
https://review.coreboot.org/c/coreboot/+/40335/1/src/arch/x86/acpi.c@114 PS1, Line 114: end_bus_number
This is `u8` too and we can't change it. Having more than 256 buses means […]
got your point, we can have a wrapper here which converts (end / 256) into end_bus_number and (end % 256) into pci_segment_group_number ?
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Usha P, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40335
to look at the new patch set (#4).
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
soc/intel/common/block/acpi: Add provision for multiple PCI segments
This patch ensures semanticity in acpi_create_mcfg_mmconfig() programming if SoC supports more than 256 buses and SoC user selects CONFIG_SA_PCIEX_LENGTH > 256.
Refer to Ice Lake EDS vol 2 chapter 3.18.
Change-Id: I90660c5cfd8af5bb40e36bb409e534541c786cae Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/acpi/acpi.c 1 file changed, 14 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/40335/4
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40335/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40335/1//COMMIT_MSG@10 PS1, Line 10: 1
Ack
Ack
https://review.coreboot.org/c/coreboot/+/40335/1//COMMIT_MSG@10 PS1, Line 10: SoC specification [Ice Lake EDS vol 1 chapter 3.18]
As per EDS, the formula out there
Ack
https://review.coreboot.org/c/coreboot/+/40335/1/src/arch/x86/acpi.c File src/arch/x86/acpi.c:
https://review.coreboot.org/c/coreboot/+/40335/1/src/arch/x86/acpi.c@114 PS1, Line 114: end_bus_number
got your point, we can have a wrapper here which converts (end / 256) into end_bus_number and (end […]
Ack
Pratikkumar V Prajapati has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Patch Set 4: Code-Review+1
Pratikkumar V Prajapati has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Removed Code-Review+1 by Pratikkumar V Prajapati pratikkumar.v.prajapati@intel.com
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... PS4, Line 38: uint16_t seg_nr = 0; Assignment is not needed. See below.
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... PS4, Line 39: uint16_t pci_ex_len = (CONFIG_SA_PCIEX_LENGTH >> 20) - 1; Why is the conversion needed? pci_ex_len and PCIEX_LENGTH sounds the same.
Maybe also use `pciex_len` as name?
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... PS4, Line 44: seg_nr = pci_ex_len / 256; Move this out of the if statement, as x / 256 with x <= 255, is 0 for integers.
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... PS4, Line 48: bus_end_nr = pci_ex_len; That’s the same as above with `seg_nr = 0`. So, the else branch, and therefore the if statement is not needed.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... PS4, Line 38: uint16_t seg_nr = 0;
Assignment is not needed. See below.
agree we can avoid this as local variable
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... PS4, Line 39: uint16_t pci_ex_len = (CONFIG_SA_PCIEX_LENGTH >> 20) - 1;
Why is the conversion needed? pci_ex_len and PCIEX_LENGTH sounds the same.
Right now assume CONFIG_SA_PCIEX_LENGTH = 0x1000_0000 which need to covert those into bus hence RSH with 20. Now to address ur comment we can use pciex_bus rather pci_ex_len
Maybe also use `pciex_len` as name?
Yes we will use pciex_bus
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... PS4, Line 44: seg_nr = pci_ex_len / 256;
Move this out of the if statement, as x / 256 with x <= 255, is 0 for integers.
make sense
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... PS4, Line 48: bus_end_nr = pci_ex_len;
That’s the same as above with `seg_nr = 0`. […]
make sense
Usha P has uploaded a new patch set (#5) to the change originally created by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
soc/intel/common/block/acpi: Add provision for multiple PCI segments
This patch ensures semanticity in acpi_create_mcfg_mmconfig() programming if SoC supports more than 256 buses and SoC user selects CONFIG_SA_PCIEX_LENGTH > 256.
Refer to Ice Lake EDS vol 2 chapter 3.18.
Change-Id: I90660c5cfd8af5bb40e36bb409e534541c786cae Signed-off-by: Subrata Banik subrata.banik@intel.com Signed-off-by: Usha P usha.p@intel.com --- M src/soc/intel/common/block/acpi/acpi.c 1 file changed, 9 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/40335/5
Usha P has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... PS4, Line 38: uint16_t seg_nr = 0;
agree we can avoid this as local variable
Done
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... PS4, Line 39: uint16_t pci_ex_len = (CONFIG_SA_PCIEX_LENGTH >> 20) - 1;
Why is the conversion needed? pci_ex_len and PCIEX_LENGTH sounds the same. […]
Done
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... PS4, Line 44: seg_nr = pci_ex_len / 256;
make sense
Done
https://review.coreboot.org/c/coreboot/+/40335/4/src/soc/intel/common/block/... PS4, Line 48: bus_end_nr = pci_ex_len;
make sense
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40335/5/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/40335/5/src/soc/intel/common/block/... PS5, Line 43: seg_nr = pciex_bus / 256; Doesn't this need _SEG to be updated as well?
https://review.coreboot.org/c/coreboot/+/40335/5/src/soc/intel/common/block/... PS5, Line 46: acpi_create_mcfg_mmconfig((void *)current, : CONFIG_MMCONF_BASE_ADDRESS, seg_nr, 0, bus_end_nr); Don't you need an entry for each segment of 256 buses? I haven't looked at the ACPI table descriptions closely, but that was my undertanding.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40335/5/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/40335/5/src/soc/intel/common/block/... PS5, Line 43: seg_nr = pciex_bus / 256;
Doesn't this need _SEG to be updated as well?
Sorry, i didn't get your point here
https://review.coreboot.org/c/coreboot/+/40335/5/src/soc/intel/common/block/... PS5, Line 46: acpi_create_mcfg_mmconfig((void *)current, : CONFIG_MMCONF_BASE_ADDRESS, seg_nr, 0, bus_end_nr);
Don't you need an entry for each segment of 256 buses? I haven't looked at the ACPI table descriptio […]
As per ACPI MCFG table i believe we are good as i don't see any ACPI entry for multi segment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Patch Set 5:
Before we dive deeper into this, can please discuss the reason behind it?
What can/will it be used for?
Does ICL really have multiple PCI segments?
What devices are on the other segments?
Why do we suddenly need to give the OS access to these devices?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Patch Set 5:
Patch Set 5:
Before we dive deeper into this, can please discuss the reason behind it?
Nothing specific rather ensuring Intel Common Code has support for latest SoC release with EDS been updated with new features.
What can/will it be used for?
To incorporate any devices sitting at Segment 1.
Does ICL really have multiple PCI segments?
If a platform supports more than 256 buses, then it should support multi-segment. It aligns with the below description in the EDS. TGL as you know supports 2 segments, 1 for TBT and 1 for the rest of the devices which adds up to a total of 512 buses.
What devices are on the other segments?
Now you have answer based on above question
Why do we suddenly need to give the OS access to these devices?
Ideally configuration mechanism makes use of memory mapped address space range/s to access PCI configuration space. Put simply, the memory address determines the segment group, bus, device, function and register being accessed. On x86 and x64 platforms, the address of each memory area is determined by the ACPI 'MCFG' table. Thought of updating correct information to OS via MCFG as applicable for platform.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Patch Set 5:
Does ICL really have multiple PCI segments?
If a platform supports more than 256 buses, then it should support multi-segment. It aligns with the below description in the EDS.
I guess that's a no (for ICL).
TGL as you know supports 2 segments,
Nope, I didn't know that.
1 for TBT and 1 for the rest of the devices which adds up to a total of 512 buses.
Thanks for mentioning this. Even if it is not your reason for this change, please put this information into the commit message. It makes a huge difference: without it, your changes look like added complexity for naught; with it, there is a reason and reviewers can do their job.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Patch Set 5:
(2 comments)
I've left some inline comments. But before we contiue discussing the implementation, IMHO, it's better to discuss some design decisions around it.
First of all, I don't think we should make the number of segment groups follow the SA_PCIEX_LENGTH Kconfig. Instead, I would prefer another Kconfig, to be set by the mainboard or soc code, that sets the number of segment groups. If that is > 1, then SA_PCIEX_LENGTH should be increased automatically. Let's call this PCI_SEGMENT_GROUPS.
We can then add additional segment groups to the DSDT, according to PCI_SEGMENT_GROUPS. If we should do that with an SSDT generator, or in static .asl files, at the soc level or at the mainboard level, depends a lot on the actual use case. Which I know next to nothing about.
Only when patches to add additional segment groups to the DSDT, along with their _SEG numbers have landed, this change should land. cf. PCI Firmware Specification "MCFG Table Description"
https://review.coreboot.org/c/coreboot/+/40335/5/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/40335/5/src/soc/intel/common/block/... PS5, Line 43: seg_nr = pciex_bus / 256;
Sorry, i didn't get your point here
_SEG is an object in the DSDT below the PCI root. Currently, we have something like this in coreboot:
Scope (_SB) { Device (PCI0) { Name (_SEG, 0) ... } }
This means _SB.PCI0 forms segment group number 0.
What you need for a second segment group on TGL, is a second root with _SEG 1, for instance:
Scope (_SB) { Device (PCI0) { Name (_SEG, 0) ... } Device (PCI1) { // or TBT0? Name (_SEG, 1) ... } }
https://review.coreboot.org/c/coreboot/+/40335/5/src/soc/intel/common/block/... PS5, Line 46: acpi_create_mcfg_mmconfig((void *)current, : CONFIG_MMCONF_BASE_ADDRESS, seg_nr, 0, bus_end_nr);
As per ACPI MCFG table i believe we are good as i don't see any ACPI entry for multi segment
acpi_create_mcfg_mmconfig() is supposed to be called once for each segment group.
The `seg_nr` parameter of acpi_create_mcfg_mmconfig() means the id (or number) of the given segment group, _not_ the overall count (number) of segment groups.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Patch Set 5:
Patch Set 5:
(2 comments)
I've left some inline comments. But before we contiue discussing the implementation, IMHO, it's better to discuss some design decisions around it.
First of all, I don't think we should make the number of segment groups follow the SA_PCIEX_LENGTH Kconfig. Instead, I would prefer another Kconfig, to be set by the mainboard or soc code, that sets the number of segment groups. If that is > 1, then SA_PCIEX_LENGTH should be increased automatically. Let's call this PCI_SEGMENT_GROUPS.
We can then add additional segment groups to the DSDT, according to PCI_SEGMENT_GROUPS. If we should do that with an SSDT generator, or in static .asl files, at the soc level or at the mainboard level, depends a lot on the actual use case. Which I know next to nothing about.
Only when patches to add additional segment groups to the DSDT, along with their _SEG numbers have landed, this change should land. cf. PCI Firmware Specification "MCFG Table Description"
Most of design i agree with except that we need a dedicated Kconfig for knowing PCI segment PCI_SEGMENT_GROUPS because EDS never talks about how many segment present, its deduced based on PCI BUS number SA_PCIEX_LENGTH we have.
Can't we use SA_PCIEX_LENGTH to calculate segment number dynamically (rather doing opposite?) as we are doing now and then pass those information from .c file .asl (northbridge.asl) to create additional capability for new segment (for _SEG 1). The reason is knowing SA_PCIEX_LENGTH and PCI_SEGMENT_GROUPS are too much information we expect from soc/mb user to define SoC programming logic.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Patch Set 5:
I've left some inline comments. But before we contiue discussing the implementation, IMHO, it's better to discuss some design decisions around it.
First of all, I don't think we should make the number of segment groups follow the SA_PCIEX_LENGTH Kconfig. Instead, I would prefer another Kconfig, to be set by the mainboard or soc code, that sets the number of segment groups. If that is > 1, then SA_PCIEX_LENGTH should be increased automatically. Let's call this PCI_SEGMENT_GROUPS.
We can then add additional segment groups to the DSDT, according to PCI_SEGMENT_GROUPS. If we should do that with an SSDT generator, or in static .asl files, at the soc level or at the mainboard level, depends a lot on the actual use case. Which I know next to nothing about.
Only when patches to add additional segment groups to the DSDT, along with their _SEG numbers have landed, this change should land. cf. PCI Firmware Specification "MCFG Table Description"
Most of design i agree with except that we need a dedicated Kconfig for knowing PCI segment PCI_SEGMENT_GROUPS because EDS never talks about how many segment present, its deduced based on PCI BUS number SA_PCIEX_LENGTH we have.
The EDS only describes one register and doesn't tell the whole story. PCIEXBAR is just a tool to access the PCI devices' config space, it doesn't decide on which segment/bus the devices are.
In my experience, things turn out much easier if you reflect reality in Kconfig. We need a bigger SA_PCIEX_LENGTH if there is a second segment group. Not the other way around.
Can't we use SA_PCIEX_LENGTH to calculate segment number dynamically (rather doing opposite?) as we are doing now and then pass those information from .c file .asl (northbridge.asl) to create additional capability for new segment (for _SEG 1).
I just fear we'd end up repeating the /256 calculation everywhere. But if that happens depends a lot on the actual implementation. You can just try.
The reason is knowing SA_PCIEX_LENGTH and PCI_SEGMENT_GROUPS are too much information we expect from soc/mb user to define SoC programming logic.
That's why I suggested to increase SA_PCIEX_LENGTH automatically based on other Kconfigs.
The BIOS Spec mentions some dependencies, so it should be decided per mainboard, AFAICS. Overriding SA_PCIEX_LENGTH at the mainboard level wouldn't look good. So I guess we need an additional Kconfig anyway.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Patch Set 5:
Patch Set 5:
I've left some inline comments. But before we contiue discussing the implementation, IMHO, it's better to discuss some design decisions around it.
First of all, I don't think we should make the number of segment groups follow the SA_PCIEX_LENGTH Kconfig. Instead, I would prefer another Kconfig, to be set by the mainboard or soc code, that sets the number of segment groups. If that is > 1, then SA_PCIEX_LENGTH should be increased automatically. Let's call this PCI_SEGMENT_GROUPS.
We can then add additional segment groups to the DSDT, according to PCI_SEGMENT_GROUPS. If we should do that with an SSDT generator, or in static .asl files, at the soc level or at the mainboard level, depends a lot on the actual use case. Which I know next to nothing about.
Only when patches to add additional segment groups to the DSDT, along with their _SEG numbers have landed, this change should land. cf. PCI Firmware Specification "MCFG Table Description"
Most of design i agree with except that we need a dedicated Kconfig for knowing PCI segment PCI_SEGMENT_GROUPS because EDS never talks about how many segment present, its deduced based on PCI BUS number SA_PCIEX_LENGTH we have.
The EDS only describes one register and doesn't tell the whole story. PCIEXBAR is just a tool to access the PCI devices' config space, it doesn't decide on which segment/bus the devices are.
In my experience, things turn out much easier if you reflect reality in Kconfig. We need a bigger SA_PCIEX_LENGTH if there is a second segment group. Not the other way around.
Can't we use SA_PCIEX_LENGTH to calculate segment number dynamically (rather doing opposite?) as we are doing now and then pass those information from .c file .asl (northbridge.asl) to create additional capability for new segment (for _SEG 1).
I just fear we'd end up repeating the /256 calculation everywhere. But if that happens depends a lot on the actual implementation. You can just try.
The reason is knowing SA_PCIEX_LENGTH and PCI_SEGMENT_GROUPS are too much information we expect from soc/mb user to define SoC programming logic.
That's why I suggested to increase SA_PCIEX_LENGTH automatically based on other Kconfigs.
The BIOS Spec mentions some dependencies, so it should be decided per mainboard, AFAICS. Overriding SA_PCIEX_LENGTH at the mainboard level wouldn't look good. So I guess we need an additional Kconfig anyway.
Can you please review this design proposal made based on your feedback above
https://review.coreboot.org/c/coreboot/+/40722
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: soc/intel/common/block/acpi: Add provision for multiple PCI segments ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
I've left some inline comments. But before we contiue discussing the implementation, IMHO, it's better to discuss some design decisions around it.
First of all, I don't think we should make the number of segment groups follow the SA_PCIEX_LENGTH Kconfig. Instead, I would prefer another Kconfig, to be set by the mainboard or soc code, that sets the number of segment groups. If that is > 1, then SA_PCIEX_LENGTH should be increased automatically. Let's call this PCI_SEGMENT_GROUPS.
We can then add additional segment groups to the DSDT, according to PCI_SEGMENT_GROUPS. If we should do that with an SSDT generator, or in static .asl files, at the soc level or at the mainboard level, depends a lot on the actual use case. Which I know next to nothing about.
Only when patches to add additional segment groups to the DSDT, along with their _SEG numbers have landed, this change should land. cf. PCI Firmware Specification "MCFG Table Description"
Most of design i agree with except that we need a dedicated Kconfig for knowing PCI segment PCI_SEGMENT_GROUPS because EDS never talks about how many segment present, its deduced based on PCI BUS number SA_PCIEX_LENGTH we have.
The EDS only describes one register and doesn't tell the whole story. PCIEXBAR is just a tool to access the PCI devices' config space, it doesn't decide on which segment/bus the devices are.
In my experience, things turn out much easier if you reflect reality in Kconfig. We need a bigger SA_PCIEX_LENGTH if there is a second segment group. Not the other way around.
Can't we use SA_PCIEX_LENGTH to calculate segment number dynamically (rather doing opposite?) as we are doing now and then pass those information from .c file .asl (northbridge.asl) to create additional capability for new segment (for _SEG 1).
I just fear we'd end up repeating the /256 calculation everywhere. But if that happens depends a lot on the actual implementation. You can just try.
The reason is knowing SA_PCIEX_LENGTH and PCI_SEGMENT_GROUPS are too much information we expect from soc/mb user to define SoC programming logic.
That's why I suggested to increase SA_PCIEX_LENGTH automatically based on other Kconfigs.
The BIOS Spec mentions some dependencies, so it should be decided per mainboard, AFAICS. Overriding SA_PCIEX_LENGTH at the mainboard level wouldn't look good. So I guess we need an additional Kconfig anyway.
Can you please review this design proposal made based on your feedback above
@Nico: Anything here https://review.coreboot.org/c/coreboot/+/40722
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Usha P, Pratikkumar V Prajapati, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40335
to look at the new patch set (#6).
Change subject: device: Add provision for multiple PCI segments ......................................................................
device: Add provision for multiple PCI segments
Add CONFIG_PCI_SEGMENT_GROUPS with default value 1 refer as single PCI segment with 256 buses.
SoC user can override CONFIG_PCI_SEGMENT_GROUPS from respective SoC Kconfig if SoC has support for multiple PCI segment (> 256 buses).
Change-Id: I90660c5cfd8af5bb40e36bb409e534541c786cae Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/Kconfig 1 file changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/40335/6
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: device: Add provision for multiple PCI segments ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40335/5/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/40335/5/src/soc/intel/common/block/... PS5, Line 43: seg_nr = pciex_bus / 256;
_SEG is an object in the DSDT below the PCI root. Currently, we have […]
Ack
https://review.coreboot.org/c/coreboot/+/40335/5/src/soc/intel/common/block/... PS5, Line 46: acpi_create_mcfg_mmconfig((void *)current, : CONFIG_MMCONF_BASE_ADDRESS, seg_nr, 0, bus_end_nr);
acpi_create_mcfg_mmconfig() is supposed to be called once for each segment […]
Ack
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: device: Add provision for multiple PCI segments ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40335/6/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/40335/6/src/device/Kconfig@794 PS6, Line 794: help : This option allows you to select required PCI segment. : : Each PCI segment supports up to 256 buses (0-255). : PCIE length selection is depends on PCI_SEGMENT_GROUPS. : Formula for PCIE Length = PCI_SEGMENT_GROUPS * 256 MB : If PCI segment is 1 then length of PCIEX region is 256MB to support 256 buses : else PCIEX region would be multiple of 256MB based on PCI_SEGMENT_GROUPS. I don't think a help text can help here. 256 buses is the maximum per spec, but individual chips can have a lower limit, or even groups of different size. So please move everything about the 256 to `soc/intel/`.
The only thing we can say here for sure is that "this should be changed when there are multiple PCI segment groups". But that's just stating the obvious.
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Usha P, Pratikkumar V Prajapati, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40335
to look at the new patch set (#7).
Change subject: device: Add provision for multiple PCI segments ......................................................................
device: Add provision for multiple PCI segments
Add CONFIG_PCI_SEGMENT_GROUPS with default value 1.
SoC user can override CONFIG_PCI_SEGMENT_GROUPS from respective SoC Kconfig if SoC has support for multiple PCI segment.
Change-Id: I90660c5cfd8af5bb40e36bb409e534541c786cae Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/Kconfig 1 file changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/40335/7
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: device: Add provision for multiple PCI segments ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40335/6/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/40335/6/src/device/Kconfig@794 PS6, Line 794: help : This option allows you to select required PCI segment. : : Each PCI segment supports up to 256 buses (0-255). : PCIE length selection is depends on PCI_SEGMENT_GROUPS. : Formula for PCIE Length = PCI_SEGMENT_GROUPS * 256 MB : If PCI segment is 1 then length of PCIEX region is 256MB to support 256 buses : else PCIEX region would be multiple of 256MB based on PCI_SEGMENT_GROUPS.
I don't think a help text can help here. 256 buses is the maximum […]
Ack
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: device: Add provision for multiple PCI segments ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40335/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40335/7//COMMIT_MSG@11 PS7, Line 11: SoC user can override CONFIG_PCI_SEGMENT_GROUPS from respective : SoC Kconfig if SoC has support for multiple PCI segment. What is the impact of selecting this Kconfig. How does coreboot behavior change or is expected to change? Does the SoC need to handle this differently?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: device: Add provision for multiple PCI segments ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40335/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40335/7//COMMIT_MSG@11 PS7, Line 11: SoC user can override CONFIG_PCI_SEGMENT_GROUPS from respective : SoC Kconfig if SoC has support for multiple PCI segment.
What is the impact of selecting this Kconfig.
By default value is 1 hence overriding is only allowed from SOC side if SOc has support.
How does coreboot behavior change or is expected to change?
1. SoC to perform additional PCI config 0x60 programming to select the correct PCIEX length 2. Publish MCFG ACPI table for all segment
Does the SoC need to handle this differently?
Yes, will handle by Intel common code those additional programming
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Usha P, Pratikkumar V Prajapati, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40335
to look at the new patch set (#8).
Change subject: device: Add provision for multiple PCI segments ......................................................................
device: Add provision for multiple PCI segments
Add CONFIG_PCI_SEGMENT_GROUPS with default value 1.
SoC user can override CONFIG_PCI_SEGMENT_GROUPS from respective SoC Kconfig if SoC has support for multiple PCI segment.
Right now only IA SoC support been added for pci multiple segment hence IA common code block to address additional programming based on SoC overide CONFIG_PCI_SEGMENT_GROUPS with > 1 value.
1. SoC to program PCI config space PCIEBAR length 2. Publish MCFG ACPI table for all segments
Change-Id: I90660c5cfd8af5bb40e36bb409e534541c786cae Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/device/Kconfig 1 file changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/40335/8
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: device: Add provision for multiple PCI segments ......................................................................
Patch Set 8: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40335 )
Change subject: device: Add provision for multiple PCI segments ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40335/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40335/7//COMMIT_MSG@11 PS7, Line 11: SoC user can override CONFIG_PCI_SEGMENT_GROUPS from respective : SoC Kconfig if SoC has support for multiple PCI segment.
What is the impact of selecting this Kconfig. […]
It was my idea to have a Kconfig in common code. As Subrata described, we would (for now) only have chip specific code that is affected. Nevertheless, I would prefer to have it in src/device/ because the overall concept of PCI segment groups is not chip specific.
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/40335?usp=email )
Change subject: device: Add provision for multiple PCI segments ......................................................................
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.
-
Furquan Shaikh (Code Review) -
Martin L Roth (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)
-
Pratikkumar V Prajapati (Code Review)
-
Subrata Banik (Code Review)
-
Usha P (Code Review)
-
Wonkyu Kim (Code Review)