Change in coreboot[master]: soc/intel/cannonlake: Use SA_DEV_ROOT instead of PCH_DEV_PMC
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34116 )
Change subject: soc/intel/cannonlake: Use SA_DEV_ROOT instead of PCH_DEV_PMC ......................................................................
soc/intel/cannonlake: Use SA_DEV_ROOT instead of PCH_DEV_PMC
PMC device gets hidden from PCI bus after FSP-S call. Thus, it gets removed from the root bus as leftover unused device. With change 903b40a8a46 ("soc/intel: Replace uses of dev_find_slot()"), all uses of dev_find_slot() were replaced by pcidev_path_on_root() which relies on scanning of root bus to find the requested device. Since PMC device is removed from the root bus, pcidev_path_on_root() returns NULL for it thus resulting in configuration being skipped for the PMC ultimately resulting in S3 failures.
Since the PCH_DEV_PMC was just used to get to chip config, this change replaces the use of PCH_DEV_PMC with SA_DEV_ROOT.
BUG=b:136861224 TEST=Verified that S3 works fine on hatch.
Change-Id: Ie5ade00ac2aca697608f1bdea9764b71c26e2112 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/cannonlake/finalize.c M src/soc/intel/cannonlake/pmc.c 2 files changed, 7 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/34116/1
diff --git a/src/soc/intel/cannonlake/finalize.c b/src/soc/intel/cannonlake/finalize.c index 4dfd15b..d099d77 100644 --- a/src/soc/intel/cannonlake/finalize.c +++ b/src/soc/intel/cannonlake/finalize.c @@ -68,8 +68,13 @@ * * Disabling ACPI PM timer is necessary for XTAL OSC shutdown. * Disabling ACPI PM timer also switches off TCO + * + * SA_DEV_ROOT device is used here instead of PCH_DEV_PMC since it is + * just required to get to chip config. PCH_DEV_PMC is hidden by this + * point and hence removed from the root bus. pcidev_path_on_root thus + * returns NULL for PCH_DEV_PMC device. */ - dev = PCH_DEV_PMC; + dev = SA_DEV_ROOT; config = dev->chip_info; pmcbase = pmc_mmio_regs(); if (config->PmTimerDisabled) { diff --git a/src/soc/intel/cannonlake/pmc.c b/src/soc/intel/cannonlake/pmc.c index 6834aa2..0b23568 100644 --- a/src/soc/intel/cannonlake/pmc.c +++ b/src/soc/intel/cannonlake/pmc.c @@ -153,7 +153,7 @@
static void pmc_init(void *unused) { - struct device *dev = PCH_DEV_PMC; + struct device *dev = SA_DEV_ROOT; config_t *config = dev->chip_info;
rtc_init();
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34116 )
Change subject: soc/intel/cannonlake: Use SA_DEV_ROOT instead of PCH_DEV_PMC ......................................................................
Patch Set 1: Code-Review+2
In general, I really don't like function calls being disguised as simple macros.
I have some plans to get rid of __SIMPLE_DEVICE__ and start using struct device *dev in all stages.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34116 )
Change subject: soc/intel/cannonlake: Use SA_DEV_ROOT instead of PCH_DEV_PMC ......................................................................
Patch Set 1: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34116 )
Change subject: soc/intel/cannonlake: Use SA_DEV_ROOT instead of PCH_DEV_PMC ......................................................................
soc/intel/cannonlake: Use SA_DEV_ROOT instead of PCH_DEV_PMC
PMC device gets hidden from PCI bus after FSP-S call. Thus, it gets removed from the root bus as leftover unused device. With change 903b40a8a46 ("soc/intel: Replace uses of dev_find_slot()"), all uses of dev_find_slot() were replaced by pcidev_path_on_root() which relies on scanning of root bus to find the requested device. Since PMC device is removed from the root bus, pcidev_path_on_root() returns NULL for it thus resulting in configuration being skipped for the PMC ultimately resulting in S3 failures.
Since the PCH_DEV_PMC was just used to get to chip config, this change replaces the use of PCH_DEV_PMC with SA_DEV_ROOT.
BUG=b:136861224 TEST=Verified that S3 works fine on hatch.
Change-Id: Ie5ade00ac2aca697608f1bdea9764b71c26e2112 Signed-off-by: Furquan Shaikh furquan@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34116 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/finalize.c M src/soc/intel/cannonlake/pmc.c 2 files changed, 7 insertions(+), 2 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/finalize.c b/src/soc/intel/cannonlake/finalize.c index 4dfd15b..d099d77 100644 --- a/src/soc/intel/cannonlake/finalize.c +++ b/src/soc/intel/cannonlake/finalize.c @@ -68,8 +68,13 @@ * * Disabling ACPI PM timer is necessary for XTAL OSC shutdown. * Disabling ACPI PM timer also switches off TCO + * + * SA_DEV_ROOT device is used here instead of PCH_DEV_PMC since it is + * just required to get to chip config. PCH_DEV_PMC is hidden by this + * point and hence removed from the root bus. pcidev_path_on_root thus + * returns NULL for PCH_DEV_PMC device. */ - dev = PCH_DEV_PMC; + dev = SA_DEV_ROOT; config = dev->chip_info; pmcbase = pmc_mmio_regs(); if (config->PmTimerDisabled) { diff --git a/src/soc/intel/cannonlake/pmc.c b/src/soc/intel/cannonlake/pmc.c index 6834aa2..0b23568 100644 --- a/src/soc/intel/cannonlake/pmc.c +++ b/src/soc/intel/cannonlake/pmc.c @@ -153,7 +153,7 @@
static void pmc_init(void *unused) { - struct device *dev = PCH_DEV_PMC; + struct device *dev = SA_DEV_ROOT; config_t *config = dev->chip_info;
rtc_init();
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34116 )
Change subject: soc/intel/cannonlake: Use SA_DEV_ROOT instead of PCH_DEV_PMC ......................................................................
Patch Set 2: Code-Review+2
Patch Set 1: Code-Review+2
In general, I really don't like function calls being disguised as simple macros.
I have some plans to get rid of __SIMPLE_DEVICE__ and start using struct device *dev in all stages.
I agree.
-
Angel Pons (Code Review) -
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)
-
Subrata Banik (Code Review)