Change in ...coreboot[master]: soc/intel/cannonlake: Disable ACPI mode on BS_DEV_INIT exit

[Furquan Shaikh (Code Review)]

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31633

Change subject: soc/intel/cannonlake: Disable ACPI mode on BS_DEV_INIT exit
......................................................................

soc/intel/cannonlake: Disable ACPI mode on BS_DEV_INIT exit

Change ac8c60e (soc/intel/cannonlake: Disable ACPI mode as part of
pmc_soc_init) moved disabling of ACPI mode to pmc_soc_init to keep it
more aligned with the behavior on other Intel SoCs. However, as the
PMC device is hidden, it never gets enumerated and so init function
does not get called for it. This change moves the call to disable ACPI
mode to exit of BS_DEV_INIT instead.

BUG=b:126016602
TEST=Verified that:
1. pmc_set_acpi_mode is actually getting called.
2. EC panic event gets logged to eventlog correctly.

Change-Id: Ie7025e322fa0abc21367a520184a4c7741eba1e6
Signed-off-by: Furquan Shaikh 
---
M src/soc/intel/cannonlake/pmc.c
1 file changed, 10 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/31633/1

diff --git a/src/soc/intel/cannonlake/pmc.c b/src/soc/intel/cannonlake/pmc.c
index 24ddfee..84bfba0 100644
--- a/src/soc/intel/cannonlake/pmc.c
+++ b/src/soc/intel/cannonlake/pmc.c
@@ -158,20 +158,26 @@
 */
 BOOT_STATE_INIT_ENTRY(BS_DEV_INIT_CHIPS, BS_ON_EXIT, pmc_init, NULL);

-void pmc_soc_init(struct device *dev)
+static void soc_acpi_mode_init(void *unused)
 {
    /*
     * PMC initialization happens earlier for this SoC because FSP-Silicon
     * init hides PMC from PCI bus. However, pmc_set_acpi_mode, which
     * disables ACPI mode doesn't need to happen that early and can be
-	 * delayed till typical pmc_soc_init callback. This ensures that ACPI
-	 * mode disabling happens the same way for all SoCs and hence the
-	 * ordering of events is the same.
+	 * delayed till typical BS_DEV_INIT. This ensures that ACPI mode
+	 * disabling happens the same way for all SoCs and hence the ordering of
+	 * events is the same.
     *
     * This is important to ensure that the ordering does not break the
     * assumptions of any other drivers (e.g. ChromeEC) which could be
     * taking different actions based on disabling of ACPI (e.g. flushing of
     * all EC hostevent bits).
+	 *
+	 * P.S.: This cannot be done as part of pmc_soc_init as PMC device is
+	 * hidden and hence the PMC driver never gets enumerated and so init is
+	 * not called for it.
     */
    pmc_set_acpi_mode();
 }
+
+BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT, soc_acpi_mode_init, NULL);

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31633 )

Change subject: soc/intel/cannonlake: Disable ACPI mode on BS_DEV_INIT exit
......................................................................

Patch Set 1: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31633 )

Change subject: soc/intel/cannonlake: Disable ACPI mode on BS_DEV_INIT exit
......................................................................

soc/intel/cannonlake: Disable ACPI mode on BS_DEV_INIT exit

Change ac8c60e (soc/intel/cannonlake: Disable ACPI mode as part of
pmc_soc_init) moved disabling of ACPI mode to pmc_soc_init to keep it
more aligned with the behavior on other Intel SoCs. However, as the
PMC device is hidden, it never gets enumerated and so init function
does not get called for it. This change moves the call to disable ACPI
mode to exit of BS_DEV_INIT instead.

BUG=b:126016602
TEST=Verified that:
1. pmc_set_acpi_mode is actually getting called.
2. EC panic event gets logged to eventlog correctly.

Change-Id: Ie7025e322fa0abc21367a520184a4c7741eba1e6
Signed-off-by: Furquan Shaikh 
Reviewed-on: https://review.coreboot.org/c/31633
Reviewed-by: Subrata Banik 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/cannonlake/pmc.c
1 file changed, 10 insertions(+), 4 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Subrata Banik: Looks good to me, approved

diff --git a/src/soc/intel/cannonlake/pmc.c b/src/soc/intel/cannonlake/pmc.c
index 24ddfee..84bfba0 100644
--- a/src/soc/intel/cannonlake/pmc.c
+++ b/src/soc/intel/cannonlake/pmc.c
@@ -158,20 +158,26 @@
 */
 BOOT_STATE_INIT_ENTRY(BS_DEV_INIT_CHIPS, BS_ON_EXIT, pmc_init, NULL);

-void pmc_soc_init(struct device *dev)
+static void soc_acpi_mode_init(void *unused)
 {
    /*
     * PMC initialization happens earlier for this SoC because FSP-Silicon
     * init hides PMC from PCI bus. However, pmc_set_acpi_mode, which
     * disables ACPI mode doesn't need to happen that early and can be
-	 * delayed till typical pmc_soc_init callback. This ensures that ACPI
-	 * mode disabling happens the same way for all SoCs and hence the
-	 * ordering of events is the same.
+	 * delayed till typical BS_DEV_INIT. This ensures that ACPI mode
+	 * disabling happens the same way for all SoCs and hence the ordering of
+	 * events is the same.
     *
     * This is important to ensure that the ordering does not break the
     * assumptions of any other drivers (e.g. ChromeEC) which could be
     * taking different actions based on disabling of ACPI (e.g. flushing of
     * all EC hostevent bits).
+	 *
+	 * P.S.: This cannot be done as part of pmc_soc_init as PMC device is
+	 * hidden and hence the PMC driver never gets enumerated and so init is
+	 * not called for it.
     */
    pmc_set_acpi_mode();
 }
+
+BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT, soc_acpi_mode_init, NULL);