Change in coreboot[master]: UPSTREAM: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init ...

[Kane Chen (Code Review)]

Kane Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44563 )

Change subject: UPSTREAM: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops
......................................................................

UPSTREAM: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops

pmc_set_acpi_mode will set EC SMI mask to 1 in the end.
However google_chromeec_events_init will clear EC SMI mask.
If google_chromeec_events_init is ran after pmc_set_acpi_mode, the EC SMI mask
will be 0 in depthcharge and causes lidclose function not working.
So, pmc_set_acpi_mode() should run after google_chromeec_events_init.

This code is mainly from CL:42677

BUG=b:16338215
TEST=Close lid in depthcharge and the dut can be shutdown on waddledoo.

Signed-off-by: Kane Chen 
Change-Id: I0f06e8b5da00eb05a34a6ce1de6d713005211c08
---
M src/soc/intel/jasperlake/pmc.c
1 file changed, 15 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/44563/1

diff --git a/src/soc/intel/jasperlake/pmc.c b/src/soc/intel/jasperlake/pmc.c
index 4443055..711b63c 100644
--- a/src/soc/intel/jasperlake/pmc.c
+++ b/src/soc/intel/jasperlake/pmc.c
@@ -98,8 +98,23 @@
    res->flags = IORESOURCE_IO | IORESOURCE_ASSIGNED | IORESOURCE_FIXED;
 }

+static void soc_acpi_mode_init(struct device *dev)
+{
+	/*
+	 * pmc_set_acpi_mode() should be delayed until BS_DEV_INIT in order
+	 * to ensure the ordering does not break the assumptions that other
+	 * drivers make about ACPI mode (e.g. Chrome EC). Since it disables
+	 * ACPI mode, other drivers may take different actions based on this
+	 * (e.g. Chrome EC will flush any pending hostevent bits). Because
+	 * TGL has its PMC device available for device_operations, it can be
+	 * done from the "ops->init" callback.
+	 */
+	pmc_set_acpi_mode();
+}
+
 struct device_operations pmc_ops = {
    .read_resources		= soc_pmc_read_resources,
    .set_resources		= noop_set_resources,
+	.init		  = soc_acpi_mode_init,
    .enable			= pmc_init,
 };

Change in coreboot[master]: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops

[Kane Chen (Code Review)]

Hello Patrick Rudolph,

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

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

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

Change subject: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops
......................................................................

soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops

pmc_set_acpi_mode will set EC SMI mask to 1 in the end.
However google_chromeec_events_init will clear EC SMI mask.
If google_chromeec_events_init is ran after pmc_set_acpi_mode, the EC SMI mask
will be 0 in depthcharge and causes lidclose function not working.
So, pmc_set_acpi_mode() should run after google_chromeec_events_init.

This code is mainly from CL:42677

BUG=b:16338215
TEST=Close lid in depthcharge and the dut can be shutdown on waddledoo.

Signed-off-by: Kane Chen 
Change-Id: I0f06e8b5da00eb05a34a6ce1de6d713005211c08
---
M src/soc/intel/jasperlake/pmc.c
1 file changed, 15 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/44563/2

Change in coreboot[master]: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops

[Kane Chen (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops
......................................................................

soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops

pmc_set_acpi_mode will set EC SMI mask to 1 in the end.
However google_chromeec_events_init will clear EC SMI mask.
If google_chromeec_events_init is ran after pmc_set_acpi_mode, the EC SMI mask
will be 0 in depthcharge and causes lidclose function not working.
So, pmc_set_acpi_mode() should run after google_chromeec_events_init.

This code is mainly from CL:42677

BUG=b:16338215
TEST=Close lid in depthcharge and the dut can be shutdown on waddledoo.

Signed-off-by: Kane Chen 
Change-Id: I0f06e8b5da00eb05a34a6ce1de6d713005211c08
---
M src/soc/intel/jasperlake/pmc.c
1 file changed, 15 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/44563/3

Change in coreboot[master]: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops

[Dtrain Hsu (Code Review)]

Dtrain Hsu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44563 )

Change subject: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops
......................................................................

Patch Set 3: Code-Review+1

Change in coreboot[master]: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44563 )

Change subject: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44563/3/src/soc/intel/jasperlake/pm... 
File src/soc/intel/jasperlake/pmc.c:

https://review.coreboot.org/c/coreboot/+/44563/3/src/soc/intel/jasperlake/pm... 
PS3, Line 81: pmc_set_acpi_mode
can be removed now.

Change in coreboot[master]: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44563 )

Change subject: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44563/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/44563/3//COMMIT_MSG@15 
PS3, Line 15: CL:42677
Nit: CB:42677. That links to the correct coreboot CL.

Change in coreboot[master]: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops

[Kane Chen (Code Review)]

Hello build bot (Jenkins), Dtrain Hsu, Maulik V Vaghela, Aamir Bohra, Patrick Rudolph, Karthik Ramasubramanian,

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

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

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

Change subject: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops
......................................................................

soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops

pmc_set_acpi_mode will set EC SMI mask to 1 in the end.
However google_chromeec_events_init will clear EC SMI mask.
If google_chromeec_events_init is ran after pmc_set_acpi_mode, the EC SMI mask
will be 0 in depthcharge and causes lidclose function not working.
So, pmc_set_acpi_mode() should run after google_chromeec_events_init.

This code is mainly from CB:42677

BUG=b:16338215
TEST=Close lid in depthcharge and the dut can be shutdown on waddledoo.

Signed-off-by: Kane Chen 
Change-Id: I0f06e8b5da00eb05a34a6ce1de6d713005211c08
---
M src/soc/intel/jasperlake/pmc.c
1 file changed, 15 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/44563/4

Change in coreboot[master]: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops

[Kane Chen (Code Review)]

Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44563 )

Change subject: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/44563/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/44563/3//COMMIT_MSG@15 
PS3, Line 15: CL:42677
> Nit: CB:42677. That links to the correct coreboot CL.

Done

https://review.coreboot.org/c/coreboot/+/44563/3/src/soc/intel/jasperlake/pm... 
File src/soc/intel/jasperlake/pmc.c:

https://review.coreboot.org/c/coreboot/+/44563/3/src/soc/intel/jasperlake/pm... 
PS3, Line 81: pmc_set_acpi_mode
> can be removed now.

Done

Change in coreboot[master]: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44563 )

Change subject: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops
......................................................................

Patch Set 4: Code-Review+2

Change in coreboot[master]: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44563 )

Change subject: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops
......................................................................

Patch Set 4: Code-Review+2

Change in coreboot[master]: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops

[Maulik V Vaghela (Code Review)]

Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44563 )

Change subject: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops
......................................................................

Patch Set 4: Code-Review+2

Change in coreboot[master]: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops

[Kane Chen (Code Review)]

Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44563 )

Change subject: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops
......................................................................

Patch Set 4:

could we merge this?
thanks

Change in coreboot[master]: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops

[Subrata Banik (Code Review)]

Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44563 )

Change subject: soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops
......................................................................

soc/intel/jasperlake: Run pmc_set_acpi_mode() during .init in pmc_ops

pmc_set_acpi_mode will set EC SMI mask to 1 in the end.
However google_chromeec_events_init will clear EC SMI mask.
If google_chromeec_events_init is ran after pmc_set_acpi_mode, the EC SMI mask
will be 0 in depthcharge and causes lidclose function not working.
So, pmc_set_acpi_mode() should run after google_chromeec_events_init.

This code is mainly from CB:42677

BUG=b:16338215
TEST=Close lid in depthcharge and the dut can be shutdown on waddledoo.

Signed-off-by: Kane Chen 
Change-Id: I0f06e8b5da00eb05a34a6ce1de6d713005211c08
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44563
Tested-by: build bot (Jenkins) 
Reviewed-by: Aamir Bohra 
Reviewed-by: Karthik Ramasubramanian 
Reviewed-by: Maulik V Vaghela 
---
M src/soc/intel/jasperlake/pmc.c
1 file changed, 15 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aamir Bohra: Looks good to me, approved
  Maulik V Vaghela: Looks good to me, approved
  Karthik Ramasubramanian: Looks good to me, approved

diff --git a/src/soc/intel/jasperlake/pmc.c b/src/soc/intel/jasperlake/pmc.c
index 4443055..ed6a31d 100644
--- a/src/soc/intel/jasperlake/pmc.c
+++ b/src/soc/intel/jasperlake/pmc.c
@@ -78,8 +78,6 @@
    pmc_set_power_failure_state(true);
    pmc_gpe_init();

-	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);
@@ -98,8 +96,23 @@
    res->flags = IORESOURCE_IO | IORESOURCE_ASSIGNED | IORESOURCE_FIXED;
 }

+static void soc_acpi_mode_init(struct device *dev)
+{
+	/*
+	 * pmc_set_acpi_mode() should be delayed until BS_DEV_INIT in order
+	 * to ensure the ordering does not break the assumptions that other
+	 * drivers make about ACPI mode (e.g. Chrome EC). Since it disables
+	 * ACPI mode, other drivers may take different actions based on this
+	 * (e.g. Chrome EC will flush any pending hostevent bits). Because
+	 * JSL has its PMC device available for device_operations, it can be
+	 * done from the "ops->init" callback.
+	 */
+	pmc_set_acpi_mode();
+}
+
 struct device_operations pmc_ops = {
    .read_resources		= soc_pmc_read_resources,
    .set_resources		= noop_set_resources,
+	.init			= soc_acpi_mode_init,
    .enable			= pmc_init,
 };