Change in ...coreboot[master]: soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init
......................................................................

soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init

PMC initialization on Cannon lake happens earlier in the boot sequence
than other SoCs because FSP-Silicon init hides PMC from PCI bus. As
ACPI disabling was done as part of PMC init, it was being called
earlier than what other SoCs do. This resulted in a different order of
events for some drivers e.g. ChromeOS EC. In case of ChromeOS EC, it
ended up clearing EC events (which happens as part of ACPI disabling
in SMM) before logging any events of interest that happen during
mainboard initialization.

This change moves the call to disable ACPI to pmc_soc_init just like
other SoCs to keep the order of events more aligned.

BUG=b:126016602
TEST=Verified that EC panic event gets logged to eventlog correctly.

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/31614/1

diff --git a/src/soc/intel/cannonlake/pmc.c b/src/soc/intel/cannonlake/pmc.c
index e111e94..24ddfee 100644
--- a/src/soc/intel/cannonlake/pmc.c
+++ b/src/soc/intel/cannonlake/pmc.c
@@ -144,8 +144,6 @@
    /* Initialize power management */
    pch_power_options(dev);

-	pmc_set_acpi_mode();
-
    config_deep_s3(config->deep_s3_enable_ac, config->deep_s3_enable_dc);
    config_deep_s5(config->deep_s5_enable_ac, config->deep_s5_enable_dc);
    config_deep_sx(config->deep_sx_config);
@@ -159,3 +157,21 @@
 * allocate resources.
 */
 BOOT_STATE_INIT_ENTRY(BS_DEV_INIT_CHIPS, BS_ON_EXIT, pmc_init, NULL);
+
+void pmc_soc_init(struct device *dev)
+{
+	/*
+	 * 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.
+	 *
+	 * 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).
+	 */
+	pmc_set_acpi_mode();
+}

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31614 )

Change subject: soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init
......................................................................

Patch Set 1: Code-Review+2

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31614 )

Change subject: soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/#/c/31614/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31614/1//COMMIT_MSG@9 
PS1, Line 9: lake
Lake

[Furquan Shaikh (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Paul Menzel, Duncan Laurie, Shelley Chen, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31614

to look at the new patch set (#2).

Change subject: soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init
......................................................................

soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init

PMC initialization on Cannon Lake happens earlier in the boot sequence
than other SoCs because FSP-Silicon init hides PMC from PCI bus. As
ACPI disabling was done as part of PMC init, it was being called
earlier than what other SoCs do. This resulted in a different order of
events for some drivers e.g. ChromeOS EC. In case of ChromeOS EC, it
ended up clearing EC events (which happens as part of ACPI disabling
in SMM) before logging any events of interest that happen during
mainboard initialization.

This change moves the call to disable ACPI to pmc_soc_init just like
other SoCs to keep the order of events more aligned.

BUG=b:126016602
TEST=Verified that EC panic event gets logged to eventlog correctly.

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/31614/2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31614 )

Change subject: soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31614/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31614/1//COMMIT_MSG@9 
PS1, Line 9: lake
> Lake

Done

[Furquan Shaikh (Code Review)]

Furquan Shaikh has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31614 )

Change subject: soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init
......................................................................

soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init

PMC initialization on Cannon Lake happens earlier in the boot sequence
than other SoCs because FSP-Silicon init hides PMC from PCI bus. As
ACPI disabling was done as part of PMC init, it was being called
earlier than what other SoCs do. This resulted in a different order of
events for some drivers e.g. ChromeOS EC. In case of ChromeOS EC, it
ended up clearing EC events (which happens as part of ACPI disabling
in SMM) before logging any events of interest that happen during
mainboard initialization.

This change moves the call to disable ACPI to pmc_soc_init just like
other SoCs to keep the order of events more aligned.

BUG=b:126016602
TEST=Verified that EC panic event gets logged to eventlog correctly.

Change-Id: Ib73883424a8dfd315893ca712ca86c7c08cee551
Signed-off-by: Furquan Shaikh 
Reviewed-on: https://review.coreboot.org/c/31614
Tested-by: build bot (Jenkins) 
Reviewed-by: Duncan Laurie 
Reviewed-by: Paul Menzel 
---
M src/soc/intel/cannonlake/pmc.c
1 file changed, 18 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Duncan Laurie: Looks good to me, approved
  Paul Menzel: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/cannonlake/pmc.c b/src/soc/intel/cannonlake/pmc.c
index e111e94..24ddfee 100644
--- a/src/soc/intel/cannonlake/pmc.c
+++ b/src/soc/intel/cannonlake/pmc.c
@@ -144,8 +144,6 @@
    /* Initialize power management */
    pch_power_options(dev);

-	pmc_set_acpi_mode();
-
    config_deep_s3(config->deep_s3_enable_ac, config->deep_s3_enable_dc);
    config_deep_s5(config->deep_s5_enable_ac, config->deep_s5_enable_dc);
    config_deep_sx(config->deep_sx_config);
@@ -159,3 +157,21 @@
 * allocate resources.
 */
 BOOT_STATE_INIT_ENTRY(BS_DEV_INIT_CHIPS, BS_ON_EXIT, pmc_init, NULL);
+
+void pmc_soc_init(struct device *dev)
+{
+	/*
+	 * 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.
+	 *
+	 * 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).
+	 */
+	pmc_set_acpi_mode();
+}

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31614/3/src/soc/intel/cannonlake/pmc.c 
File src/soc/intel/cannonlake/pmc.c:

https://review.coreboot.org/#/c/31614/3/src/soc/intel/cannonlake/pmc.c@176 
PS3, Line 176: pmc_set_acpi_mode();
This is function getting called before FSP-S?

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31614 )

Change subject: soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31614/3/src/soc/intel/cannonlake/pmc.c 
File src/soc/intel/cannonlake/pmc.c:

https://review.coreboot.org/#/c/31614/3/src/soc/intel/cannonlake/pmc.c@176 
PS3, Line 176: pmc_set_acpi_mode();
> This is function getting called before FSP-S?

No, after FSP-S is run.

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31614/3/src/soc/intel/cannonlake/pmc.c 
File src/soc/intel/cannonlake/pmc.c:

https://review.coreboot.org/#/c/31614/3/src/soc/intel/cannonlake/pmc.c@176 
PS3, Line 176: pmc_set_acpi_mode();
> No, after FSP-S is run.

then do you see PMC device in bus? else its not possible to call this function

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31614 )

Change subject: soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31614/3/src/soc/intel/cannonlake/pmc.c 
File src/soc/intel/cannonlake/pmc.c:

https://review.coreboot.org/#/c/31614/3/src/soc/intel/cannonlake/pmc.c@176 
PS3, Line 176: pmc_set_acpi_mode();
> then do you see PMC device in bus? else its not possible to call this function

That function actually doesn't talk directly to the PMC device: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/bloc...

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31614/3/src/soc/intel/cannonlake/pmc.c 
File src/soc/intel/cannonlake/pmc.c:

https://review.coreboot.org/#/c/31614/3/src/soc/intel/cannonlake/pmc.c@176 
PS3, Line 176: pmc_set_acpi_mode();
> That function actually doesn't talk directly to the PMC device: https://review.coreboot. […]

my point is that .init may not get called if PMC device is not seeing over bus. it may not able to call ops function

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31614 )

Change subject: soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31614/3/src/soc/intel/cannonlake/pmc.c 
File src/soc/intel/cannonlake/pmc.c:

https://review.coreboot.org/#/c/31614/3/src/soc/intel/cannonlake/pmc.c@176 
PS3, Line 176: pmc_set_acpi_mode();
> my point is that .init may not get called if PMC device is not seeing over bus. […]

That is a valid point. I did not add prints to check if this function is getting called. But I see your point about device not enumerating and hence no init call. Let me run some experiments tomorrow. This will probably have to be moved to BS_DEV_INIT. That is unfortunate.

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31614/3/src/soc/intel/cannonlake/pmc.c 
File src/soc/intel/cannonlake/pmc.c:

https://review.coreboot.org/#/c/31614/3/src/soc/intel/cannonlake/pmc.c@176 
PS3, Line 176: pmc_set_acpi_mode();
> That is a valid point. I did not add prints to check if this function is getting called. […]

yes, looks like i only move that PMC programming into BS state machine.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31614 )

Change subject: soc/intel/cannonlake: Disable ACPI mode as part of pmc_soc_init
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31614/3/src/soc/intel/cannonlake/pmc.c 
File src/soc/intel/cannonlake/pmc.c:

https://review.coreboot.org/#/c/31614/3/src/soc/intel/cannonlake/pmc.c@176 
PS3, Line 176: pmc_set_acpi_mode();
> yes, looks like i only move that PMC programming into BS state machine.

Tested and pushed patch here: https://review.coreboot.org/c/coreboot/+/31633