Wim Vervoorn has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37704 )
Change subject: soc/intel/skylake: Change SA_PCIEX_LENGTH to 256MB ......................................................................
soc/intel/skylake: Change SA_PCIEX_LENGTH to 256MB
Skylake soc code sets the length of the PCIe configuration space to 64 MB while the specification allows up to 256 MB. Linux reports "acpi PNP0A08:00: [Firmware Info]: MMCONFIG for domain 0000 [bos 00-3f] only partially covers this bridge".
Remove "select PCIEX_LENGTH_64MB" from Kconfig so the default 256MB will be used and the size can be reduced on mainboard level when required.
BUG=N/A TEST=tested on facebook monolith
Change-Id: I8a06b9fba5ad561d8595292a73136091ab532faa Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/soc/intel/skylake/Kconfig 1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/37704/1
diff --git a/src/soc/intel/skylake/Kconfig b/src/soc/intel/skylake/Kconfig index 31f809a..5fc2a2d 100644 --- a/src/soc/intel/skylake/Kconfig +++ b/src/soc/intel/skylake/Kconfig @@ -43,7 +43,6 @@ select NO_FIXED_XIP_ROM_SIZE select PARALLEL_MP select PARALLEL_MP_AP_WORK - select PCIEX_LENGTH_64MB select PLATFORM_USES_FSP2_0 select REG_SCRIPT select SA_ENABLE_DPR
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37704 )
Change subject: soc/intel/skylake: Change SA_PCIEX_LENGTH to 256MB ......................................................................
Patch Set 1: Code-Review+1
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37704 )
Change subject: soc/intel/skylake: Change SA_PCIEX_LENGTH to 256MB ......................................................................
Patch Set 1:
The number of busses is typically configurable in PCI configuration, which afaik is done by FSP. ACPI has to match the hardware configuration, so the commit message should say if that is the case and how it was tested/verified.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37704 )
Change subject: soc/intel/skylake: Change SA_PCIEX_LENGTH to 256MB ......................................................................
Patch Set 1:
Patch Set 1:
The number of busses is typically configurable in PCI configuration, which afaik is done by FSP. ACPI has to match the hardware configuration, so the commit message should say if that is the case and how it was tested/verified.
The space used here depends on the assigned bus numbers, no on the enabled root ports.
The coreboot code is in theory free to assign any bus number between 0 and 255 to a bus it configures. There is no dependency on FSP. I also looked and the hardware specification and could not find a way that the bus number assigned can be limited (by masking bits e.g.)
So what I have done is change the values and checked it the area was reserved and reported properly and doesn't overlap. Please note, it is valid to limit the area if a huge amount of MMIO is needed and you are sure no bus above 63 is used. But then you do this on purpose and know where the dmesg reporting comes from.
Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37704
to look at the new patch set (#2).
Change subject: soc/intel/skylake: Change SA_PCIEX_LENGTH to 256MB ......................................................................
soc/intel/skylake: Change SA_PCIEX_LENGTH to 256MB
Skylake soc code sets the length of the PCIe configuration space to 64 MB while the specification allows up to 256 MB. Linux reports "acpi PNP0A08:00: [Firmware Info]: MMCONFIG for domain 0000 [bos 00-3f] only partially covers this bridge".
Remove "select PCIEX_LENGTH_64MB" from Kconfig so the default 256MB will be used and the size can be reduced on the mainboard level when required.
BUG=N/A TEST=tested on facebook monolith
Tested is by booting Linux 4.15 and analyzing the coreboot and Linux dmesg to make sure the memory range is reported correctly and doesn't create an overlap.
Change-Id: I8a06b9fba5ad561d8595292a73136091ab532faa Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/soc/intel/skylake/Kconfig 1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/37704/2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37704 )
Change subject: soc/intel/skylake: Change SA_PCIEX_LENGTH to 256MB ......................................................................
Patch Set 2: Code-Review-1
Patch Set 1:
Patch Set 1:
The number of busses is typically configurable in PCI configuration, which afaik is done by FSP. ACPI has to match the hardware configuration, so the commit message should say if that is the case and how it was tested/verified.
The space used here depends on the assigned bus numbers, no on the enabled root ports.
The coreboot code is in theory free to assign any bus number between 0 and 255 to a bus it configures. There is no dependency on FSP. I also looked and the hardware specification and could not find a way that the bus number assigned can be limited (by masking bits e.g.)
The length of the PCIe configuration space (and therefor the max #busses) is actually configurable in 'PCIEXBAR' offset 0x60 of 00:00.0. It's just bad coreboot (or lack of coreboot code) to not report what is actually programmed in that register.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37704 )
Change subject: soc/intel/skylake: Change SA_PCIEX_LENGTH to 256MB ......................................................................
Patch Set 2:
Patch Set 2: Code-Review-1
Patch Set 1:
Patch Set 1:
The number of busses is typically configurable in PCI configuration, which afaik is done by FSP. ACPI has to match the hardware configuration, so the commit message should say if that is the case and how it was tested/verified.
The space used here depends on the assigned bus numbers, no on the enabled root ports.
The coreboot code is in theory free to assign any bus number between 0 and 255 to a bus it configures. There is no dependency on FSP. I also looked and the hardware specification and could not find a way that the bus number assigned can be limited (by masking bits e.g.)
The length of the PCIe configuration space (and therefore the max #busses) is actually configurable in 'PCIEXBAR' offset 0x60 of 00:00.0. It's just bad coreboot (or lack of coreboot code) to not report what is actually programmed in that register.
You are right but this register is set by coreboot depending on the SA_PCIEX_LENGTH setting and indeed within coreboot, there are no further dependencies on this register (regarding bus allocation e.g.). So if the memory mapping is correct I don't see why we shouldn't align the Skylake soc with all other Intel socs using the common system agent code. I will double check if this register is unmodified after post.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37704 )
Change subject: soc/intel/skylake: Change SA_PCIEX_LENGTH to 256MB ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review-1
Patch Set 1:
Patch Set 1:
The number of busses is typically configurable in PCI configuration, which afaik is done by FSP. ACPI has to match the hardware configuration, so the commit message should say if that is the case and how it was tested/verified.
The space used here depends on the assigned bus numbers, no on the enabled root ports.
The coreboot code is in theory free to assign any bus number between 0 and 255 to a bus it configures. There is no dependency on FSP. I also looked and the hardware specification and could not find a way that the bus number assigned can be limited (by masking bits e.g.)
The length of the PCIe configuration space (and therefore the max #busses) is actually configurable in 'PCIEXBAR' offset 0x60 of 00:00.0. It's just bad coreboot (or lack of coreboot code) to not report what is actually programmed in that register.
You are right but this register is set by coreboot depending on the SA_PCIEX_LENGTH setting and indeed within coreboot, there are no further dependencies on this register (regarding bus allocation e.g.). So if the memory mapping is correct I don't see why we shouldn't align the Skylake soc with all other Intel socs using the common system agent code. I will double check if this register is unmodified after post.
Just verified. This register contains 0xE0000001 which indicates 256 MB so it should be fine.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37704 )
Change subject: soc/intel/skylake: Change SA_PCIEX_LENGTH to 256MB ......................................................................
Patch Set 2: Code-Review+2
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review-1
Patch Set 1:
Patch Set 1:
The number of busses is typically configurable in PCI configuration, which afaik is done by FSP. ACPI has to match the hardware configuration, so the commit message should say if that is the case and how it was tested/verified.
The space used here depends on the assigned bus numbers, no on the enabled root ports.
The coreboot code is in theory free to assign any bus number between 0 and 255 to a bus it configures. There is no dependency on FSP. I also looked and the hardware specification and could not find a way that the bus number assigned can be limited (by masking bits e.g.)
The length of the PCIe configuration space (and therefore the max #busses) is actually configurable in 'PCIEXBAR' offset 0x60 of 00:00.0. It's just bad coreboot (or lack of coreboot code) to not report what is actually programmed in that register.
You are right but this register is set by coreboot depending on the SA_PCIEX_LENGTH setting and indeed within coreboot, there are no further dependencies on this register (regarding bus allocation e.g.). So if the memory mapping is correct I don't see why we shouldn't align the Skylake soc with all other Intel socs using the common system agent code. I will double check if this register is unmodified after post.
Just verified. This register contains 0xE0000001 which indicates 256 MB so it should be fine.
Ok thanks for testing. Looks like indeed bootblock_systemagent_early_init() properly sets this up and not FSP.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37704 )
Change subject: soc/intel/skylake: Change SA_PCIEX_LENGTH to 256MB ......................................................................
soc/intel/skylake: Change SA_PCIEX_LENGTH to 256MB
Skylake soc code sets the length of the PCIe configuration space to 64 MB while the specification allows up to 256 MB. Linux reports "acpi PNP0A08:00: [Firmware Info]: MMCONFIG for domain 0000 [bos 00-3f] only partially covers this bridge".
Remove "select PCIEX_LENGTH_64MB" from Kconfig so the default 256MB will be used and the size can be reduced on the mainboard level when required.
BUG=N/A TEST=tested on facebook monolith
Tested is by booting Linux 4.15 and analyzing the coreboot and Linux dmesg to make sure the memory range is reported correctly and doesn't create an overlap.
Change-Id: I8a06b9fba5ad561d8595292a73136091ab532faa Signed-off-by: Wim Vervoorn wvervoorn@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37704 Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Patrick Rudolph siro@das-labor.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/skylake/Kconfig 1 file changed, 0 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, but someone else must approve Arthur Heymans: Looks good to me, approved
diff --git a/src/soc/intel/skylake/Kconfig b/src/soc/intel/skylake/Kconfig index 31f809a..5fc2a2d 100644 --- a/src/soc/intel/skylake/Kconfig +++ b/src/soc/intel/skylake/Kconfig @@ -43,7 +43,6 @@ select NO_FIXED_XIP_ROM_SIZE select PARALLEL_MP select PARALLEL_MP_AP_WORK - select PCIEX_LENGTH_64MB select PLATFORM_USES_FSP2_0 select REG_SCRIPT select SA_ENABLE_DPR
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37704 )
Change subject: soc/intel/skylake: Change SA_PCIEX_LENGTH to 256MB ......................................................................
Patch Set 3:
Should such changes of defaults in chipset code be announced to the list, or be visibly documented in the release notes?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37704 )
Change subject: soc/intel/skylake: Change SA_PCIEX_LENGTH to 256MB ......................................................................
Patch Set 3:
`git grep "MMCONFIG for domain 0000"` in the board status repository shows this message for quite a few boards.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37704 )
Change subject: soc/intel/skylake: Change SA_PCIEX_LENGTH to 256MB ......................................................................
Patch Set 3:
Patch Set 3:
Should such changes of defaults in chipset code be announced to the list, or be visibly documented in the release notes?
I don't think announcing in the list is a good idea. This is easily missed.
Listing this type of information in the general readme for the version would result in a huge amount of information that isn't that useful as I think the readme should document the important high level stuff only and not every chipset or board detail.
Perhaps it is a good idea to have a READMEon the component (in this case SoC) level that documents the state and features of the specific code. This can be useful when maintained correctly.
-
Arthur Heymans (Code Review) -
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Wim Vervoorn (Code Review)