Change in coreboot[master]: soc/intel/{cannonlake,icelake}: Do not define PCH_DEV_PMC in ramstage
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34120 )
Change subject: soc/intel/{cannonlake,icelake}: Do not define PCH_DEV_PMC in ramstage ......................................................................
soc/intel/{cannonlake,icelake}: Do not define PCH_DEV_PMC in ramstage
This change intentionally removes the definition of PCH_DEV_PMC from ramstage to avoid silent errors. This device gets hidden from PCI bus in FSP-S and hence dropped from the root bus by the resource allocator. In order to avoid incorrect references to the device, avoid defining it in ramstage where it known to return NULL.
BUG=b:136861224
Change-Id: I4f69470ec80c7127a2b604ed2b1f794f5a63e126 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/cannonlake/include/soc/pci_devs.h M src/soc/intel/icelake/include/soc/pci_devs.h 2 files changed, 24 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/34120/1
diff --git a/src/soc/intel/cannonlake/include/soc/pci_devs.h b/src/soc/intel/cannonlake/include/soc/pci_devs.h index 46bc1bf..1bc028f 100644 --- a/src/soc/intel/cannonlake/include/soc/pci_devs.h +++ b/src/soc/intel/cannonlake/include/soc/pci_devs.h @@ -187,7 +187,19 @@ #define PCH_DEVFN_TRACEHUB _PCH_DEVFN(LPC, 7) #define PCH_DEV_LPC _PCH_DEV(LPC, 0) #define PCH_DEV_P2SB _PCH_DEV(LPC, 1) + +#if !ENV_RAMSTAGE +/* + * PCH_DEV_PMC is intentionally not defined in RAMSTAGE since PMC device gets + * hidden from PCI bus after call to FSP-S. This leads to resource allocator + * dropping it from the root bus as unused device. All references to PCH_DEV_PMC + * would then return NULL and can go unnoticed if not handled properly. Since, + * this device does not have any special chip config associated with it, it is + * okay to not provide the definition for it in ramstage. + */ #define PCH_DEV_PMC _PCH_DEV(LPC, 2) +#endif + #define PCH_DEV_HDA _PCH_DEV(LPC, 3) #define PCH_DEV_SMBUS _PCH_DEV(LPC, 4) #define PCH_DEV_SPI _PCH_DEV(LPC, 5) diff --git a/src/soc/intel/icelake/include/soc/pci_devs.h b/src/soc/intel/icelake/include/soc/pci_devs.h index b242113..1348f23b 100644 --- a/src/soc/intel/icelake/include/soc/pci_devs.h +++ b/src/soc/intel/icelake/include/soc/pci_devs.h @@ -184,7 +184,19 @@ #define PCH_DEV_ESPI _PCH_DEV(ESPI, 0) #define PCH_DEV_LPC PCH_DEV_ESPI #define PCH_DEV_P2SB _PCH_DEV(ESPI, 1) + +#if !ENV_RAMSTAGE +/* + * PCH_DEV_PMC is intentionally not defined in RAMSTAGE since PMC device gets + * hidden from PCI bus after call to FSP-S. This leads to resource allocator + * dropping it from the root bus as unused device. All references to PCH_DEV_PMC + * would then return NULL and can go unnoticed if not handled properly. Since, + * this device does not have any special chip config associated with it, it is + * okay to not provide the definition for it in ramstage. + */ #define PCH_DEV_PMC _PCH_DEV(ESPI, 2) +#endif + #define PCH_DEV_HDA _PCH_DEV(ESPI, 3) #define PCH_DEV_SMBUS _PCH_DEV(ESPI, 4) #define PCH_DEV_SPI _PCH_DEV(ESPI, 5)
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34120 )
Change subject: soc/intel/{cannonlake,icelake}: Do not define PCH_DEV_PMC in ramstage ......................................................................
Patch Set 1: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34120 )
Change subject: soc/intel/{cannonlake,icelake}: Do not define PCH_DEV_PMC in ramstage ......................................................................
Patch Set 1: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34120 )
Change subject: soc/intel/{cannonlake,icelake}: Do not define PCH_DEV_PMC in ramstage ......................................................................
soc/intel/{cannonlake,icelake}: Do not define PCH_DEV_PMC in ramstage
This change intentionally removes the definition of PCH_DEV_PMC from ramstage to avoid silent errors. This device gets hidden from PCI bus in FSP-S and hence dropped from the root bus by the resource allocator. In order to avoid incorrect references to the device, avoid defining it in ramstage where it known to return NULL.
BUG=b:136861224
Change-Id: I4f69470ec80c7127a2b604ed2b1f794f5a63e126 Signed-off-by: Furquan Shaikh furquan@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34120 Reviewed-by: Subrata Banik subrata.banik@intel.com Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/cannonlake/include/soc/pci_devs.h M src/soc/intel/icelake/include/soc/pci_devs.h 2 files changed, 24 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved Subrata Banik: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/include/soc/pci_devs.h b/src/soc/intel/cannonlake/include/soc/pci_devs.h index 46bc1bf..1bc028f 100644 --- a/src/soc/intel/cannonlake/include/soc/pci_devs.h +++ b/src/soc/intel/cannonlake/include/soc/pci_devs.h @@ -187,7 +187,19 @@ #define PCH_DEVFN_TRACEHUB _PCH_DEVFN(LPC, 7) #define PCH_DEV_LPC _PCH_DEV(LPC, 0) #define PCH_DEV_P2SB _PCH_DEV(LPC, 1) + +#if !ENV_RAMSTAGE +/* + * PCH_DEV_PMC is intentionally not defined in RAMSTAGE since PMC device gets + * hidden from PCI bus after call to FSP-S. This leads to resource allocator + * dropping it from the root bus as unused device. All references to PCH_DEV_PMC + * would then return NULL and can go unnoticed if not handled properly. Since, + * this device does not have any special chip config associated with it, it is + * okay to not provide the definition for it in ramstage. + */ #define PCH_DEV_PMC _PCH_DEV(LPC, 2) +#endif + #define PCH_DEV_HDA _PCH_DEV(LPC, 3) #define PCH_DEV_SMBUS _PCH_DEV(LPC, 4) #define PCH_DEV_SPI _PCH_DEV(LPC, 5) diff --git a/src/soc/intel/icelake/include/soc/pci_devs.h b/src/soc/intel/icelake/include/soc/pci_devs.h index b242113..1348f23b 100644 --- a/src/soc/intel/icelake/include/soc/pci_devs.h +++ b/src/soc/intel/icelake/include/soc/pci_devs.h @@ -184,7 +184,19 @@ #define PCH_DEV_ESPI _PCH_DEV(ESPI, 0) #define PCH_DEV_LPC PCH_DEV_ESPI #define PCH_DEV_P2SB _PCH_DEV(ESPI, 1) + +#if !ENV_RAMSTAGE +/* + * PCH_DEV_PMC is intentionally not defined in RAMSTAGE since PMC device gets + * hidden from PCI bus after call to FSP-S. This leads to resource allocator + * dropping it from the root bus as unused device. All references to PCH_DEV_PMC + * would then return NULL and can go unnoticed if not handled properly. Since, + * this device does not have any special chip config associated with it, it is + * okay to not provide the definition for it in ramstage. + */ #define PCH_DEV_PMC _PCH_DEV(ESPI, 2) +#endif + #define PCH_DEV_HDA _PCH_DEV(ESPI, 3) #define PCH_DEV_SMBUS _PCH_DEV(ESPI, 4) #define PCH_DEV_SPI _PCH_DEV(ESPI, 5)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34120 )
Change subject: soc/intel/{cannonlake,icelake}: Do not define PCH_DEV_PMC in ramstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34120/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34120/2//COMMIT_MSG@10 PS2, Line 10: This device gets hidden from PCI bus : in FSP-S Small question: Is this because of POSTBOOT_SAI?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34120 )
Change subject: soc/intel/{cannonlake,icelake}: Do not define PCH_DEV_PMC in ramstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34120/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34120/2//COMMIT_MSG@10 PS2, Line 10: This device gets hidden from PCI bus : in FSP-S
Small question: Is this because of POSTBOOT_SAI?
No, in SPT PCH, PWRMBASE and ABASE are at offset 0x44 and 0x48 hence PCI enumeration don't change the predefined base addresses.
But in CNP PCH onwards, PWRMBASE and ABASE are located in offset 0x10 and 0x20 which is part of standard PCI BARs. Hence allowing PCI device available during PCI enumeration might allow it to override those pre-defined values. hence its been recommended that PMC device gets hidden from PCI tree and hold its pre-programmed value.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34120 )
Change subject: soc/intel/{cannonlake,icelake}: Do not define PCH_DEV_PMC in ramstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34120/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34120/2//COMMIT_MSG@10 PS2, Line 10: This device gets hidden from PCI bus : in FSP-S
No, in SPT PCH, PWRMBASE and ABASE are at offset 0x44 and 0x48 hence PCI enumeration don't change th […]
"its been recommended" -- by whom?
From coreboot perspective it would have been much cleaner to flag those BARs with ASSIGNED | FIXED in the respective .read_resources. That would have kept them in their pre-programmed values. But.. sounds like the blob takes this "offensive" option.
Is this hiding of PMC forced by FSP (and can you affect future builds to no do that!) ?
PCH_PWRM_BASE_ADDRESS 0xfe000000
I believe that thi is´ below IOAPICs. There is nothing preventing resource allocator from assigning other resources over this, is there?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34120 )
Change subject: soc/intel/{cannonlake,icelake}: Do not define PCH_DEV_PMC in ramstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34120/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34120/2//COMMIT_MSG@10 PS2, Line 10: This device gets hidden from PCI bus : in FSP-S
"its been recommended" -- by whom?
From coreboot perspective it would have been much cleaner to flag those BARs with ASSIGNED | FIXED in the respective .read_resources. That would have kept them in their pre-programmed values. But.. sounds like the blob takes this "offensive" option.
Is this hiding of PMC forced by FSP (and can you affect future builds to no do that!) ?
PCH_PWRM_BASE_ADDRESS 0xfe000000
I believe that thi is´ below IOAPICs. There is nothing preventing resource allocator from assigning other resources over this, is there?
It gets marked as reserved here: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/cannonlake/... to ensure coreboot resource allocator does not assign it to a different device.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34120 )
Change subject: soc/intel/{cannonlake,icelake}: Do not define PCH_DEV_PMC in ramstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34120/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34120/2//COMMIT_MSG@10 PS2, Line 10: This device gets hidden from PCI bus : in FSP-S
"its been recommended" -- by whom? […]
Uh... I found CB:22556
I am surprised Aaron approved that the resource is now being accounted for on a different device. Like our resource allocator was not buggy and complex enough with quirks like these :(
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34120 )
Change subject: soc/intel/{cannonlake,icelake}: Do not define PCH_DEV_PMC in ramstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34120/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34120/2//COMMIT_MSG@10 PS2, Line 10: This device gets hidden from PCI bus : in FSP-S
Uh... I found CB:22556 […]
What's the CL you were meaning? 22556 points to an em100 change. If devices are disappearing one needs to hang the fixed resources off of something. There's no 'platform device' that just floats currently, but we could add something like that. In either case, the resources need to be accounted for so the allocator will work.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34120 )
Change subject: soc/intel/{cannonlake,icelake}: Do not define PCH_DEV_PMC in ramstage ......................................................................
Patch Set 2:
CB:22566 sorry the typo
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34120 )
Change subject: soc/intel/{cannonlake,icelake}: Do not define PCH_DEV_PMC in ramstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34120/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34120/2//COMMIT_MSG@10 PS2, Line 10: This device gets hidden from PCI bus : in FSP-S
What's the CL you were meaning? 22556 points to an em100 change. […]
I just thought it could have been added as a fixed domain resource, instead of faking the BAR on a different PCI device. I believe we do that already elsewhere with domain .read_resources.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34120 )
Change subject: soc/intel/{cannonlake,icelake}: Do not define PCH_DEV_PMC in ramstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34120/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34120/2//COMMIT_MSG@10 PS2, Line 10: This device gets hidden from PCI bus : in FSP-S
I just thought it could have been added as a fixed domain resource, instead of faking the BAR on a d […]
That could be done as well, but one could also argue it's not really a part of the pci domain either. Either way would obviously work. Real question is what is the most appropriate. The presence of pci device is just a window into a more complex hierarchy of resources. That's why I was suggesting a platform device for fixed resources. Anyway, we domain device could be utilized as well, and if the system agent pci device became hidden we'd be in the same situation -- indicies need to be unique for resources and those need to be hung off of non-disabled devices because the resources are global.
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)
-
Subrata Banik (Code Review)