Change in coreboot[master]: mb/google/volteer: Add support for pmc core driver

[Venkata Krishna Nimmagadda (Code Review)]

Hello Wonkyu Kim, Caveh Jalali, Nick Vaccaro,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: mb/google/volteer: Add support for pmc core driver
......................................................................

mb/google/volteer: Add support for pmc core driver

This patch adds INT33A1 device in dsdt to support PMC core driver.

BUG=b:146236297
BRANCH=none
TEST="Build and flash volteer and verify it boots to kernel.
Checked for valid files under /sys/kernel/debug/pmc_core."

Change-Id: Ib4edc7b636725177d508b62d15633534e9f44236
Signed-off-by: Venkata Krishna Nimmagadda 
Reviewed-on: https://chrome-internal-review.googlesource.com/c/chromeos/third_party/coreb...
Reviewed-by: Venkata Krishna Nimmagadda 
Reviewed-by: Wonkyu Kim 
Reviewed-by: Caveh Jalali 
Reviewed-by: Nick Vaccaro 
Tested-by: Venkata Krishna Nimmagadda 
Commit-Queue: Alex Levin 
---
A src/soc/intel/common/acpi/pmc.asl
M src/soc/intel/tigerlake/acpi/southbridge.asl
2 files changed, 40 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39370/1

diff --git a/src/soc/intel/common/acpi/pmc.asl b/src/soc/intel/common/acpi/pmc.asl
new file mode 100644
index 0000000..d576481
--- /dev/null
+++ b/src/soc/intel/common/acpi/pmc.asl
@@ -0,0 +1,37 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright (C) 2020 Intel Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+Device (PEPD)
+{
+	Name (_HID, "INT33A1" /* Intel Power Engine */)  // _HID: Hardware ID
+	Name (_CID, EisaId ("PNP0D80") /* System Power Management Controller */)  // _CID: Compatible ID
+	Name (_UID, One)  // _UID: Unique ID
+	Name (PPD0, Package (0x03)
+	{
+		"\_SB.PCI0.SAT0",
+		Zero,
+		Package (0x02)
+		{
+			Zero,
+			Package (0x03)
+			{
+				0xFF,
+				Zero,
+				0x81
+			}
+		}
+	})
+}
+
diff --git a/src/soc/intel/tigerlake/acpi/southbridge.asl b/src/soc/intel/tigerlake/acpi/southbridge.asl
index 8593d07..1a198b0 100644
--- a/src/soc/intel/tigerlake/acpi/southbridge.asl
+++ b/src/soc/intel/tigerlake/acpi/southbridge.asl
@@ -51,3 +51,6 @@

/* PCI _OSC */
 #include <soc/intel/common/acpi/pci_osc.asl>
+
+/* PMC Core*/
+#include <soc/intel/common/acpi/pmc.asl>

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Venkata Krishna Nimmagadda (Code Review)]

Hello build bot (Jenkins), Wonkyu Kim, Caveh Jalali, Nick Vaccaro, Patrick Rudolph,

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

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

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

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

src/soc/intel: Add support for pmc core driver

This patch adds INT33A1 device in dsdt to support PMC core driver.

BUG=b:146236297
BRANCH=none
TEST="Build and flash volteer and verify it boots to kernel.
Checked for valid files under /sys/kernel/debug/pmc_core."

Change-Id: Ib4edc7b636725177d508b62d15633534e9f44236
Signed-off-by: Venkata Krishna Nimmagadda 
Reviewed-on: https://chrome-internal-review.googlesource.com/c/chromeos/third_party/coreb...
Reviewed-by: Venkata Krishna Nimmagadda 
Reviewed-by: Wonkyu Kim 
Reviewed-by: Caveh Jalali 
Reviewed-by: Nick Vaccaro 
Tested-by: Venkata Krishna Nimmagadda 
Commit-Queue: Alex Levin 
---
A src/soc/intel/common/acpi/pmc.asl
M src/soc/intel/tigerlake/acpi/southbridge.asl
2 files changed, 39 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39370/2

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Venkata Krishna Nimmagadda (Code Review)]

Hello build bot (Jenkins), Wonkyu Kim, Caveh Jalali, Nick Vaccaro, Patrick Rudolph,

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

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

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

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

src/soc/intel: Add support for pmc core driver

This patch adds INT33A1 device in dsdt to support PMC core driver.

BUG=b:146236297
BRANCH=none
TEST="Build and flash volteer and verify it boots to kernel.
Checked for valid files under /sys/kernel/debug/pmc_core."

Change-Id: Ib4edc7b636725177d508b62d15633534e9f44236
Signed-off-by: Venkata Krishna Nimmagadda 
Reviewed-on: https://chrome-internal-review.googlesource.com/c/chromeos/third_party/coreb...
Reviewed-by: Venkata Krishna Nimmagadda 
Reviewed-by: Wonkyu Kim 
Reviewed-by: Caveh Jalali 
Reviewed-by: Nick Vaccaro 
Tested-by: Venkata Krishna Nimmagadda 
Commit-Queue: Alex Levin 
---
A src/soc/intel/common/acpi/pmc.asl
M src/soc/intel/tigerlake/acpi/southbridge.asl
2 files changed, 39 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39370/3

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Furquan Shaikh (Code Review)]

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

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39370/3/src/soc/intel/common/acpi/p... 
File src/soc/intel/common/acpi/pmc.asl:

https://review.coreboot.org/c/coreboot/+/39370/3/src/soc/intel/common/acpi/p... 
PS3, Line 21: PPD0
What is this used for?

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Venkata Krishna Nimmagadda (Code Review)]

Venkata Krishna Nimmagadda has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39370/3/src/soc/intel/common/acpi/p... 
File src/soc/intel/common/acpi/pmc.asl:

https://review.coreboot.org/c/coreboot/+/39370/3/src/soc/intel/common/acpi/p... 
PS3, Line 21: PPD0
> What is this used for?

This is used for debugging s0ix through /sys/kernel interface.

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Furquan Shaikh (Code Review)]

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

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39370/3/src/soc/intel/common/acpi/p... 
File src/soc/intel/common/acpi/pmc.asl:

https://review.coreboot.org/c/coreboot/+/39370/3/src/soc/intel/common/acpi/p... 
PS3, Line 21: PPD0
> This is used for debugging s0ix through /sys/kernel interface.

Can you please point me to the kernel driver using this? What is the format of this Package being exposed? And what device is "\_SB.PCI0.SAT0"? I don't see it for any Intel SoCs.

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Caveh Jalali (Code Review)]

Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

Patch Set 3:

please set topic to TGL_UPSTREAM.

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Venkata Krishna Nimmagadda (Code Review)]

Hello build bot (Jenkins), Caveh Jalali, Wonkyu Kim, Tim Wawrzynczak, Nick Vaccaro, Raj Astekar, Patrick Rudolph, Karthik Ramasubramanian,

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

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

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

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

src/soc/intel: Add support for pmc core driver

This patch adds INT33A1 device in dsdt to support PMC core driver.

BUG=b:146236297
BRANCH=none
TEST="Build and flash volteer and verify it boots to kernel.
Checked for valid files under /sys/kernel/debug/pmc_core."

Change-Id: Ib4edc7b636725177d508b62d15633534e9f44236
Signed-off-by: Venkata Krishna Nimmagadda 
Reviewed-on: https://chrome-internal-review.googlesource.com/c/chromeos/third_party/coreb...
Reviewed-by: Venkata Krishna Nimmagadda 
Reviewed-by: Wonkyu Kim 
Reviewed-by: Caveh Jalali 
Reviewed-by: Nick Vaccaro 
Tested-by: Venkata Krishna Nimmagadda 
Commit-Queue: Alex Levin 
---
A src/soc/intel/common/acpi/pmc.asl
M src/soc/intel/tigerlake/acpi/southbridge.asl
2 files changed, 39 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39370/4

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

Patch Set 4: Code-Review+1

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39370/4/src/soc/intel/common/acpi/p... 
File src/soc/intel/common/acpi/pmc.asl:

https://review.coreboot.org/c/coreboot/+/39370/4/src/soc/intel/common/acpi/p... 
PS4, Line 23: "\_SB.PC00.SAT0"
What is this path supposed to be pointing to? I don't see any device generated anywhere that has this ACPI path.

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Venkata Krishna Nimmagadda (Code Review)]

Hello build bot (Jenkins), Wonkyu Kim, Caveh Jalali, Wonkyu Kim, Tim Wawrzynczak, Nick Vaccaro, Raj Astekar, Patrick Rudolph, Karthik Ramasubramanian,

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

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

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

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

src/soc/intel: Add support for pmc core driver

This patch adds INT33A1 device in dsdt to support PMC core driver.

BUG=b:146236297
BRANCH=none
TEST="Build and flash volteer and verify it boots to kernel.
Checked for valid files under /sys/kernel/debug/pmc_core."

Change-Id: Ib4edc7b636725177d508b62d15633534e9f44236
Signed-off-by: Venkata Krishna Nimmagadda 
Reviewed-on: https://chrome-internal-review.googlesource.com/c/chromeos/third_party/coreb...
Reviewed-by: Venkata Krishna Nimmagadda 
Reviewed-by: Wonkyu Kim 
Reviewed-by: Caveh Jalali 
Reviewed-by: Nick Vaccaro 
Tested-by: Venkata Krishna Nimmagadda 
Commit-Queue: Alex Levin 
---
A src/soc/intel/common/acpi/pmc.asl
M src/soc/intel/tigerlake/acpi/southbridge.asl
2 files changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39370/5

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Venkata Krishna Nimmagadda (Code Review)]

Venkata Krishna Nimmagadda has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

Patch Set 5: Code-Review+1

(2 comments)

https://review.coreboot.org/c/coreboot/+/39370/3/src/soc/intel/common/acpi/p... 
File src/soc/intel/common/acpi/pmc.asl:

https://review.coreboot.org/c/coreboot/+/39370/3/src/soc/intel/common/acpi/p... 
PS3, Line 21: PPD0
> Can you please point me to the kernel driver using this? What is the format of this Package being ex […]

I could not find anything using this. I have updated the patch now. Thanks

https://review.coreboot.org/c/coreboot/+/39370/4/src/soc/intel/common/acpi/p... 
File src/soc/intel/common/acpi/pmc.asl:

https://review.coreboot.org/c/coreboot/+/39370/4/src/soc/intel/common/acpi/p... 
PS4, Line 23: "\_SB.PC00.SAT0"
> What is this path supposed to be pointing to? I don't see any device generated anywhere that has thi […]

Thanks for pointing it out.I could not find anything using this or this reference existing. I have updated the patch.

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Furquan Shaikh (Code Review)]

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

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

Patch Set 5: Code-Review+2

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

Patch Set 5:

I'm curious how this worked before? For example, for our CML boards, this ACPI device was not exposed, but I definitely remember looking at, e.g. /sys/kernel/debug/pmc_core/slp_s0_residency_usec. Did the driver just get updated for the newer kernel?

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Venkata Krishna Nimmagadda (Code Review)]

Venkata Krishna Nimmagadda has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

Patch Set 5:
> Patch Set 5:
>
> I'm curious how this worked before? For example, for our CML boards, this ACPI device was not exposed, but I definitely remember looking at, e.g. /sys/kernel/debug/pmc_core/slp_s0_residency_usec. Did the driver just get updated for the newer kernel?

It seems that there is a hard-coded list of platform ids in the pmc core driver and it contains cml ( kernel v5.4 has cml. v4.19 does not have it). If that driver can't find this "INT33A1" in asl, it would fallback to that list based on the x86_match_cpu. Please refer to this discussion https://www.mail-archive.com/[e-mail]/msg1966991.html on why it was decided not to maintain that list and instead have the INT33A1 platform device defined in asl.

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Furquan Shaikh (Code Review)]

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

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39370/5/src/soc/intel/common/acpi/p... 
File src/soc/intel/common/acpi/pmc.asl:

PS5: 
Any reason why this cannot be moved to soc/intel/common/block/acpi/acpi/. And the SoCs that need it can include the file directly from there.

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

Patch Set 5: Code-Review+2

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39370/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39370/5//COMMIT_MSG@7 
PS5, Line 7: pmc
nit: Uppercase: 'PMC'

Change in coreboot[master]: src/soc/intel: Add support for pmc core driver

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: src/soc/intel: Add support for pmc core driver
......................................................................

Patch Set 5:
> Patch Set 5:
> > Patch Set 5:
> >
> > I'm curious how this worked before? For example, for our CML boards, this ACPI device was not exposed, but I definitely remember looking at, e.g. /sys/kernel/debug/pmc_core/slp_s0_residency_usec. Did the driver just get updated for the newer kernel?
>
> It seems that there is a hard-coded list of platform ids in the pmc core driver and it contains cml ( kernel v5.4 has cml. v4.19 does not have it). If that driver can't find this "INT33A1" in asl, it would fallback to that list based on the x86_match_cpu. Please refer to this discussion https://www.mail-archive.com/[e-mail]/msg1966991.html on why it was decided not to maintain that list and instead have the INT33A1 platform device defined in asl.

I see, thanks for the pointer!

Change in coreboot[master]: src/soc/intel: Add support for PMC core driver

[Venkata Krishna Nimmagadda (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Caveh Jalali, Wonkyu Kim, Tim Wawrzynczak, Nick Vaccaro, Raj Astekar, Patrick Rudolph, Karthik Ramasubramanian,

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

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

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

Change subject: src/soc/intel: Add support for PMC core driver
......................................................................

src/soc/intel: Add support for PMC core driver

This patch adds INT33A1 device in dsdt to support PMC core driver.

BUG=b:146236297
BRANCH=none
TEST="Build and flash volteer and verify it boots to kernel.
Checked for valid files under /sys/kernel/debug/pmc_core."

Change-Id: Ib4edc7b636725177d508b62d15633534e9f44236
Signed-off-by: Venkata Krishna Nimmagadda 
Reviewed-on: https://chrome-internal-review.googlesource.com/c/chromeos/third_party/coreb...
Reviewed-by: Venkata Krishna Nimmagadda 
Reviewed-by: Wonkyu Kim 
Reviewed-by: Caveh Jalali 
Reviewed-by: Nick Vaccaro 
Tested-by: Venkata Krishna Nimmagadda 
Commit-Queue: Alex Levin 
---
A src/soc/intel/common/block/acpi/acpi/pmc.asl
M src/soc/intel/tigerlake/acpi/southbridge.asl
2 files changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39370/6

Change in coreboot[master]: src/soc/intel: Add support for PMC core driver

[Venkata Krishna Nimmagadda (Code Review)]

Venkata Krishna Nimmagadda has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: src/soc/intel: Add support for PMC core driver
......................................................................

Patch Set 6: Code-Review+1

(2 comments)

https://review.coreboot.org/c/coreboot/+/39370/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39370/5//COMMIT_MSG@7 
PS5, Line 7: pmc
> nit: Uppercase: 'PMC'

Ack

https://review.coreboot.org/c/coreboot/+/39370/5/src/soc/intel/common/acpi/p... 
File src/soc/intel/common/acpi/pmc.asl:

PS5:
> Any reason why this cannot be moved to soc/intel/common/block/acpi/acpi/. […]

Ack

Change in coreboot[master]: src/soc/intel: Add support for PMC core driver

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: src/soc/intel: Add support for PMC core driver
......................................................................

Patch Set 6: Code-Review+2
> Patch Set 6: Code-Review+1
>
> (2 comments)

Change in coreboot[master]: src/soc/intel: Add support for PMC core driver

[Paul Menzel (Code Review)]

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

Change subject: src/soc/intel: Add support for PMC core driver
......................................................................

Patch Set 6:

(2 comments)

This should be two commits.

https://review.coreboot.org/c/coreboot/+/39370/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39370/6//COMMIT_MSG@7 
PS6, Line 7: src/soc/intel: Add support for PMC core driver
> soc/intel/common: Add ACPI support for PMC OS core driver

What is a “core” driver?

https://review.coreboot.org/c/coreboot/+/39370/6//COMMIT_MSG@10 
PS6, Line 10: 
The Tiger Lake change is not mentioned. Please split it out.

Change in coreboot[master]: soc/intel/common: Add ACPI support for PMC core OS driver

[Venkata Krishna Nimmagadda (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Caveh Jalali, Wonkyu Kim, Tim Wawrzynczak, Nick Vaccaro, Raj Astekar, Patrick Rudolph, Karthik Ramasubramanian,

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

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

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

Change subject: soc/intel/common: Add ACPI support for PMC core OS driver
......................................................................

soc/intel/common: Add ACPI support for PMC core OS driver

PMC core OS driver (intel_pmc_core.c in linux kernel) provides debug
hooks to developers and end users to quickly figure out why their
platform is not entering a deeper idle state such as S0ix.

This patch adds INT33A1 ACPI device to support PMC core OS
driver. Any SoC that supports this feature would include this asl file
to enable the support.

BUG=b:146236297
BRANCH=none
TEST="Build and flash volteer and verify it boots to kernel"

Change-Id: Ib4edc7b636725177d508b62d15633534e9f44236
Signed-off-by: Venkata Krishna Nimmagadda 
Reviewed-on: https://chrome-internal-review.googlesource.com/c/chromeos/third_party/coreb...
Reviewed-by: Venkata Krishna Nimmagadda 
Reviewed-by: Wonkyu Kim 
Reviewed-by: Caveh Jalali 
Reviewed-by: Nick Vaccaro 
Tested-by: Venkata Krishna Nimmagadda 
Commit-Queue: Alex Levin 
---
A src/soc/intel/common/block/acpi/acpi/pmc.asl
1 file changed, 21 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39370/7

Change in coreboot[master]: soc/intel/common: Add ACPI support for PMC core OS driver

[Venkata Krishna Nimmagadda (Code Review)]

Venkata Krishna Nimmagadda has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: soc/intel/common: Add ACPI support for PMC core OS driver
......................................................................

Patch Set 7: Code-Review+1

Change in coreboot[master]: soc/intel/common: Add ACPI support for PMC core OS driver

[Venkata Krishna Nimmagadda (Code Review)]

Venkata Krishna Nimmagadda has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: soc/intel/common: Add ACPI support for PMC core OS driver
......................................................................

Patch Set 7:

(2 comments)

https://review.coreboot.org/c/coreboot/+/39370/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39370/6//COMMIT_MSG@7 
PS6, Line 7: src/soc/intel: Add support for PMC core driver
> > soc/intel/common: Add ACPI support for PMC OS core driver […]

The name of the linux driver, that needs this support, is pmc_core. So I am using that name to be able to easily find this patch later

https://review.coreboot.org/c/coreboot/+/39370/6//COMMIT_MSG@10 
PS6, Line 10:
> The Tiger Lake change is not mentioned. Please split it out.

Ack

Change in coreboot[master]: soc/intel/common: Add ACPI support for PMC core OS driver

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: soc/intel/common: Add ACPI support for PMC core OS driver
......................................................................

Patch Set 7: Code-Review+2

Change in coreboot[master]: soc/intel/common: Add ACPI support for PMC core OS driver

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/common: Add ACPI support for PMC core OS driver
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39370/7/src/soc/intel/common/block/... 
File src/soc/intel/common/block/acpi/acpi/pmc.asl:

https://review.coreboot.org/c/coreboot/+/39370/7/src/soc/intel/common/block/... 
PS7, Line 18: // _HID: Hardware ID
Do we need these comments? I think _HID, _CID and _UID are pretty standard terms and used widely across all ACPI devices in coreboot.

Change in coreboot[master]: soc/intel/common: Add ACPI support for PMC core OS driver

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/common: Add ACPI support for PMC core OS driver
......................................................................

Patch Set 7: Code-Review+2

Change in coreboot[master]: soc/intel/common: Add ACPI support for PMC core OS driver

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/common: Add ACPI support for PMC core OS driver
......................................................................

Patch Set 7: Code-Review+2

Change in coreboot[master]: soc/intel/common: Add ACPI support for PMC core OS driver

[Srinidhi N Kaushik (Code Review)]

Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: soc/intel/common: Add ACPI support for PMC core OS driver
......................................................................

Patch Set 7: Code-Review+2

Change in coreboot[master]: soc/intel/common: Add ACPI support for PMC core OS driver

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: soc/intel/common: Add ACPI support for PMC core OS driver
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39370/7/src/soc/intel/common/block/... 
File src/soc/intel/common/block/acpi/acpi/pmc.asl:

https://review.coreboot.org/c/coreboot/+/39370/7/src/soc/intel/common/block/... 
PS7, Line 18: // _HID: Hardware ID
> Do we need these comments? I think _HID, _CID and _UID are pretty standard terms and used widely acr […]

Ack

Change in coreboot[master]: soc/intel/common: Add ACPI support for PMC core OS driver

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/common: Add ACPI support for PMC core OS driver
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39370/7/src/soc/intel/common/block/... 
File src/soc/intel/common/block/acpi/acpi/pmc.asl:

https://review.coreboot.org/c/coreboot/+/39370/7/src/soc/intel/common/block/... 
PS7, Line 18: // _HID: Hardware ID
> Ack

I don't understand the response ACK in this context. Do you mean those are not required? If yes, can those be removed then?

Change in coreboot[master]: soc/intel/common: Add ACPI support for PMC core OS driver

[Venkata Krishna Nimmagadda (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Wonkyu Kim, Subrata Banik, Patrick Rudolph, Caveh Jalali, Tim Wawrzynczak, Nick Vaccaro, Srinidhi N Kaushik, Raj Astekar, Karthik Ramasubramanian,

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

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

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

Change subject: soc/intel/common: Add ACPI support for PMC core OS driver
......................................................................

soc/intel/common: Add ACPI support for PMC core OS driver

PMC core OS driver (intel_pmc_core.c in linux kernel) provides debug
hooks to developers and end users to quickly figure out why their
platform is not entering a deeper idle state such as S0ix.

This patch adds INT33A1 ACPI device to support PMC core OS
driver. Any SoC that supports this feature would include this asl file
to enable the support.

BUG=b:146236297
BRANCH=none
TEST="Build and flash volteer and verify it boots to kernel"

Change-Id: Ib4edc7b636725177d508b62d15633534e9f44236
Signed-off-by: Venkata Krishna Nimmagadda 
Reviewed-on: https://chrome-internal-review.googlesource.com/c/chromeos/third_party/coreb...
Reviewed-by: Venkata Krishna Nimmagadda 
Reviewed-by: Wonkyu Kim 
Reviewed-by: Caveh Jalali 
Reviewed-by: Nick Vaccaro 
Tested-by: Venkata Krishna Nimmagadda 
Commit-Queue: Alex Levin 
---
A src/soc/intel/common/block/acpi/acpi/pmc.asl
1 file changed, 21 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39370/8

Change in coreboot[master]: soc/intel/common: Add ACPI support for PMC core OS driver

[Venkata Krishna Nimmagadda (Code Review)]

Venkata Krishna Nimmagadda has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: soc/intel/common: Add ACPI support for PMC core OS driver
......................................................................

Patch Set 8: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/39370/7/src/soc/intel/common/block/... 
File src/soc/intel/common/block/acpi/acpi/pmc.asl:

https://review.coreboot.org/c/coreboot/+/39370/7/src/soc/intel/common/block/... 
PS7, Line 18: // _HID: Hardware ID
> I don't understand the response ACK in this context. […]

removed those comments. Thanks

Change in coreboot[master]: soc/intel/common: Add ACPI support for PMC core OS driver

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/common: Add ACPI support for PMC core OS driver
......................................................................

Patch Set 8: Code-Review+2

Change in coreboot[master]: soc/intel/common: Add ACPI support for PMC core OS driver

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39370 )

Change subject: soc/intel/common: Add ACPI support for PMC core OS driver
......................................................................

soc/intel/common: Add ACPI support for PMC core OS driver

PMC core OS driver (intel_pmc_core.c in linux kernel) provides debug
hooks to developers and end users to quickly figure out why their
platform is not entering a deeper idle state such as S0ix.

This patch adds INT33A1 ACPI device to support PMC core OS
driver. Any SoC that supports this feature would include this asl file
to enable the support.

BUG=b:146236297
BRANCH=none
TEST="Build and flash volteer and verify it boots to kernel"

Change-Id: Ib4edc7b636725177d508b62d15633534e9f44236
Signed-off-by: Venkata Krishna Nimmagadda 
Reviewed-on: https://chrome-internal-review.googlesource.com/c/chromeos/third_party/coreb...
Reviewed-by: Venkata Krishna Nimmagadda 
Reviewed-by: Wonkyu Kim 
Reviewed-by: Caveh Jalali 
Reviewed-by: Nick Vaccaro 
Tested-by: Venkata Krishna Nimmagadda 
Commit-Queue: Alex Levin 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39370
Reviewed-by: Venkata Krishna Nimmagadda 
Reviewed-by: Furquan Shaikh 
Tested-by: build bot (Jenkins) 
---
A src/soc/intel/common/block/acpi/acpi/pmc.asl
1 file changed, 21 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Venkata Krishna Nimmagadda: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/common/block/acpi/acpi/pmc.asl b/src/soc/intel/common/block/acpi/acpi/pmc.asl
new file mode 100644
index 0000000..e534e8b
--- /dev/null
+++ b/src/soc/intel/common/block/acpi/acpi/pmc.asl
@@ -0,0 +1,21 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright (C) 2020 Intel Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+Device (PEPD)
+{
+	Name (_HID, "INT33A1" /* Intel Power Engine */)
+	Name (_CID, EisaId ("PNP0D80") /* System Power Management Controller */)
+	Name (_UID, One)
+}