Change in ...coreboot[master]: src/soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

[Jeremy Soller (Code Review)]

Jeremy Soller has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33941

Change subject: src/soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

src/soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

In order for GPEs to work properly, a number of the values used for GPIO_CFG
and MISCCFG were not correct. This adjusts them in order to have GPE_EN set
correctly, and thus enable GPEs, on the following GPIO blocks of PCH-H:

GPP_E
GPP_F
GPP_H
GPP_I
GPP_J
GPP_K
GPD

Signed-off-by: Jeremy Soller

Change-Id: I4ecc9552468037598ef5d4e10122d660dcbfe71d
---
M src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h
M src/soc/intel/cannonlake/include/soc/pmc.h
2 files changed, 14 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/33941/1

diff --git a/src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h b/src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h
index 5176ac7..16dd520 100644
--- a/src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h
+++ b/src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h
@@ -26,14 +26,14 @@
 #define GPP_B			1
 #define GPP_C			2
 #define GPP_D			3
+#define GPP_E			6
+#define GPP_F			7
 #define GPP_G			4
-#define GPP_K			5
-#define GPP_H			6
-#define GPP_E			7
-#define GPP_F			8
-#define GPP_I			9
-#define GPP_J			0xA
-#define GPD			0xC
+#define GPP_H			8
+#define GPP_I			0xA
+#define GPP_J			0xB
+#define GPP_K			9
+#define GPD			5
 #define GPIO_NUM_GROUPS		12
 #define GPIO_MAX_NUM_PER_GROUP	24

diff --git a/src/soc/intel/cannonlake/include/soc/pmc.h b/src/soc/intel/cannonlake/include/soc/pmc.h
index 67854d4..f990f6b 100644
--- a/src/soc/intel/cannonlake/include/soc/pmc.h
+++ b/src/soc/intel/cannonlake/include/soc/pmc.h
@@ -121,14 +121,14 @@
 #define PMC_GPP_B			0x1
 #define PMC_GPP_C			0x2
 #define PMC_GPP_D			0x3
-#define PMC_GPP_E			0x7
-#define PMC_GPP_F			0x8
+#define PMC_GPP_E			0xA
+#define PMC_GPP_F			0xB
 #define PMC_GPP_G			0x4
-#define PMC_GPP_H			0x6
-#define PMC_GPP_I			0x9
-#define PMC_GPP_J			0xA
-#define PMC_GPP_K			0x5
-#define PMC_GPD				0xC
+#define PMC_GPP_H			0x9
+#define PMC_GPP_I			0xC
+#define PMC_GPP_J			0xD
+#define PMC_GPP_K			0x8
+#define PMC_GPD				0x7
 #else
 #define  PMC_GPP_A			0x0
 #define  PMC_GPP_B			0x1

[Paul Menzel (Code Review)]

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

Change subject: src/soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

Patch Set 1:

(2 comments)

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

https://review.coreboot.org/#/c/33941/1//COMMIT_MSG@20 
PS1, Line 20: 
Please mention that EC starts working now.

https://review.coreboot.org/#/c/33941/1//COMMIT_MSG@21 
PS1, Line 21: Signed-off-by: Jeremy Soller 
Please move this directly below the Change-Id line.

[Jeremy Soller (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: src/soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

src/soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

In order for GPEs to work properly, a number of the values used for GPIO_CFG
and MISCCFG were not correct. This adjusts them in order to have GPE_EN set
correctly, and thus enable GPEs, on the following GPIO blocks of PCH-H:

GPP_E
GPP_F
GPP_H
GPP_I
GPP_J
GPP_K
GPD

This was tested on a System76 Gazelle (gaze14). The EC uses GPP_K3 for its
GPE and GPP_K6 is used for the lid switch GPE. Both function correctly
after applying this change.

Change-Id: I4ecc9552468037598ef5d4e10122d660dcbfe71d
Signed-off-by: Jeremy Soller 
---
M src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h
M src/soc/intel/cannonlake/include/soc/pmc.h
2 files changed, 14 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/33941/2

[Jeremy Soller (Code Review)]

Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33941 )

Change subject: src/soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

Patch Set 2:
> Patch Set 1:
>
> (2 comments)

Thanks, I fixed these issues

[Paul Menzel (Code Review)]

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

Change subject: src/soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

Patch Set 2: Code-Review+1

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33941 )

Change subject: src/soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

Patch Set 2: Code-Review+1

(5 comments)

https://review.coreboot.org/#/c/33941/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/33941/2//COMMIT_MSG@9 
PS2, Line 9: In order for GPEs to work properly
this part doesn't really fit the following sentence?

https://review.coreboot.org/#/c/33941/2//COMMIT_MSG@13 
PS2, Line 13: GPP_E
            : GPP_F
            : GPP_H
            : GPP_I
            : GPP_J
            : GPP_K
            : GPD
This is visible from the diff, no need to list them here.

What I do miss however is where you got the numbers from.
And please mention that the datasheet is not correct for
the PMC_ numbers.

https://review.coreboot.org/#/c/33941/2//COMMIT_MSG@23 
PS2, Line 23: after applying this change.
Please adhere to the 72 char line limit in commit messages.

https://review.coreboot.org/#/c/33941/2/src/soc/intel/cannonlake/include/soc... 
File src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h:

https://review.coreboot.org/#/c/33941/2/src/soc/intel/cannonlake/include/soc... 
PS2, Line 36: #define GPD			5
Please either use hex or decimal for all numbers.
Please keep them sorted by value (makes it easier
to review with the datasheet).

https://review.coreboot.org/#/c/33941/2/src/soc/intel/cannonlake/include/soc... 
File src/soc/intel/cannonlake/include/soc/pmc.h:

https://review.coreboot.org/#/c/33941/2/src/soc/intel/cannonlake/include/soc... 
PS2, Line 131: #define PMC_GPD				0x7
If these really differ from the datasheet, that must be
commented (otherwise, somebody could "correct" these
values by accident). Also, if you haven't already, please
report the documentation error to Intel.

Change in ...coreboot[master]: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

[Jeremy Soller (Code Review)]

Hello Paul Menzel, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

Soem of the values used for GPIO_CFG and MISCCFG were not correct,
causing GPEs to not work correctly. This adjusts them according to the
values found in the original ACPI tables for the System76 Gazelle.

This was tested on a System76 Gazelle (gaze14). The EC uses GPP_K3 for
its GPE and GPP_K6 is used for the lid switch GPE. Both function
correctly after applying this change.

Change-Id: I4ecc9552468037598ef5d4e10122d660dcbfe71d
Signed-off-by: Jeremy Soller 
---
M src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h
M src/soc/intel/cannonlake/include/soc/pmc.h
2 files changed, 14 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/33941/3

Change in ...coreboot[master]: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

[Jeremy Soller (Code Review)]

Hello Paul Menzel, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

Soem of the values used for GPIO_CFG and MISCCFG were not correct,
causing GPEs to not work correctly. This adjusts them according to the
values found in the original ACPI tables for the System76 Gazelle.

Unfortunately, the Intel documentation[1] mentioned below is
also incorrect. I have mentioned this to Intel already. The source
for the Intel CoffeeLake FSP also confirms these new numbers.

This was tested on a System76 Gazelle (gaze14). The EC uses GPP_K3 for
its GPE and GPP_K6 is used for the lid switch GPE. Both function
correctly after applying this change.

[1] Intel Document #572235:
    Intel ® 300 Series Chipset Families
    Platform Controller Hub
    External Design Specification (EDS) - Volume 2 of 2

Change-Id: I4ecc9552468037598ef5d4e10122d660dcbfe71d
Signed-off-by: Jeremy Soller 
---
M src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h
M src/soc/intel/cannonlake/include/soc/pmc.h
2 files changed, 14 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/33941/4

Change in ...coreboot[master]: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

[Jeremy Soller (Code Review)]

Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33941 )

Change subject: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

Patch Set 4:

(3 comments)

I have updated my commit message, hopefully that provides more information

https://review.coreboot.org/#/c/33941/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/33941/2//COMMIT_MSG@9 
PS2, Line 9: In order for GPEs to work properly
> this part doesn't really fit the following sentence?

Done

https://review.coreboot.org/#/c/33941/2//COMMIT_MSG@13 
PS2, Line 13: GPP_E
            : GPP_F
            : GPP_H
            : GPP_I
            : GPP_J
            : GPP_K
            : GPD
> This is visible from the diff, no need to list them here. […]

Done

https://review.coreboot.org/#/c/33941/2//COMMIT_MSG@23 
PS2, Line 23: after applying this change.
> Please adhere to the 72 char line limit in commit messages.

Done

Change in ...coreboot[master]: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

[Jeremy Soller (Code Review)]

Hello Paul Menzel, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

Soem of the values used for GPIO_CFG and MISCCFG were not correct,
causing GPEs to not work correctly. This adjusts them according to the
values found in the original ACPI tables for the System76 Gazelle.

Unfortunately, the Intel documentation[1] mentioned below is
also incorrect. I have mentioned this to Intel already. The source
for the Intel CoffeeLake FSP also confirms these new numbers.

This was tested on a System76 Gazelle (gaze14). The EC uses GPP_K3 for
its GPE and GPP_K6 is used for the lid switch GPE. Both function
correctly after applying this change.

[1] Intel Document #572235:
    Intel ® 300 Series Chipset Families
    Platform Controller Hub
    External Design Specification (EDS) - Volume 2 of 2

Change-Id: I4ecc9552468037598ef5d4e10122d660dcbfe71d
Signed-off-by: Jeremy Soller 
---
M src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h
M src/soc/intel/cannonlake/include/soc/pmc.h
2 files changed, 19 insertions(+), 19 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/33941/5

Change in ...coreboot[master]: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

[Jeremy Soller (Code Review)]

Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33941 )

Change subject: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

Patch Set 5:

(1 comment)

I have ordered the defines by value, and made them all hexadecimal

https://review.coreboot.org/#/c/33941/2/src/soc/intel/cannonlake/include/soc... 
File src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h:

https://review.coreboot.org/#/c/33941/2/src/soc/intel/cannonlake/include/soc... 
PS2, Line 36: #define GPD			5
> Please either use hex or decimal for all numbers. […]

Done

Change in ...coreboot[master]: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33941 )

Change subject: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

Patch Set 5: Code-Review+2

(2 comments)

Thanks

https://review.coreboot.org/#/c/33941/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/33941/5//COMMIT_MSG@9 
PS5, Line 9: Soem
Some

https://review.coreboot.org/#/c/33941/5/src/soc/intel/cannonlake/include/soc... 
File src/soc/intel/cannonlake/include/soc/pmc.h:

https://review.coreboot.org/#/c/33941/5/src/soc/intel/cannonlake/include/soc... 
PS5, Line 119: #if CONFIG(SOC_INTEL_CANNONLAKE_PCH_H)
I would really appreciate a comment here above the numbers
that the datasheet is wrong.

Change in ...coreboot[master]: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

[Jeremy Soller (Code Review)]

Hello Paul Menzel, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

Some of the values used for GPIO_CFG and MISCCFG were not correct,
causing GPEs to not work correctly. This adjusts them according to the
values found in the original ACPI tables for the System76 Gazelle.

Unfortunately, the Intel documentation[1] mentioned below is
also incorrect. I have mentioned this to Intel already. The source
for the Intel CoffeeLake FSP also confirms these new numbers.

This was tested on a System76 Gazelle (gaze14). The EC uses GPP_K3 for
its GPE and GPP_K6 is used for the lid switch GPE. Both function
correctly after applying this change.

[1] Intel Document #572235:
    Intel ® 300 Series Chipset Families
    Platform Controller Hub
    External Design Specification (EDS) - Volume 2 of 2

Change-Id: I4ecc9552468037598ef5d4e10122d660dcbfe71d
Signed-off-by: Jeremy Soller 
---
M src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h
M src/soc/intel/cannonlake/include/soc/pmc.h
2 files changed, 19 insertions(+), 19 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/33941/6

Change in ...coreboot[master]: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

[Jeremy Soller (Code Review)]

Hello Paul Menzel, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

Some of the values used for GPIO_CFG and MISCCFG were not correct,
causing GPEs to not work correctly. This adjusts them according to the
values found in the original ACPI tables for the System76 Gazelle.

Unfortunately, the Intel documentation[1] mentioned below is
also incorrect. I have mentioned this to Intel already. The source
for the Intel CoffeeLake FSP also confirms these new numbers.

This was tested on a System76 Gazelle (gaze14). The EC uses GPP_K3 for
its GPE and GPP_K6 is used for the lid switch GPE. Both function
correctly after applying this change.

[1] Intel Document #572235:
    Intel ® 300 Series Chipset Families
    Platform Controller Hub
    External Design Specification (EDS) - Volume 2 of 2

Change-Id: I4ecc9552468037598ef5d4e10122d660dcbfe71d
Signed-off-by: Jeremy Soller 
---
M src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h
M src/soc/intel/cannonlake/include/soc/pmc.h
2 files changed, 28 insertions(+), 19 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/33941/7

Change in ...coreboot[master]: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

[Jeremy Soller (Code Review)]

Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33941 )

Change subject: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

Patch Set 7:

(2 comments)

I have added a comment in both places identifying the incorrect documentation.

https://review.coreboot.org/#/c/33941/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/33941/5//COMMIT_MSG@9 
PS5, Line 9: Soem
> Some

Done

https://review.coreboot.org/#/c/33941/5/src/soc/intel/cannonlake/include/soc... 
File src/soc/intel/cannonlake/include/soc/pmc.h:

https://review.coreboot.org/#/c/33941/5/src/soc/intel/cannonlake/include/soc... 
PS5, Line 119: #if CONFIG(SOC_INTEL_CANNONLAKE_PCH_H)
> I would really appreciate a comment here above the numbers […]

Done

Change in ...coreboot[master]: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33941 )

Change subject: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

Patch Set 7: Code-Review+2

Change in ...coreboot[master]: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33941 )

Change subject: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

Patch Set 7: -Code-Review

(1 comment)

https://review.coreboot.org/#/c/33941/7/src/soc/intel/cannonlake/include/soc... 
File src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h:

https://review.coreboot.org/#/c/33941/7/src/soc/intel/cannonlake/include/soc... 
PS7, Line 26:  * please do not modify them.
Sorry, missed that you also added a comment here. These values
are correct in the datasheet, afaics.

Change in ...coreboot[master]: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

[Jeremy Soller (Code Review)]

Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33941 )

Change subject: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/33941/7/src/soc/intel/cannonlake/include/soc... 
File src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h:

https://review.coreboot.org/#/c/33941/7/src/soc/intel/cannonlake/include/soc... 
PS7, Line 26:  * please do not modify them.
> Sorry, missed that you also added a comment here. These values […]

Ah, thanks. You are right. Both values were wrong in coreboot, but only the PMC values were wrong in the datasheet. I will remove this comment

Change in ...coreboot[master]: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

[Jeremy Soller (Code Review)]

Hello Subrata Banik, Paul Menzel, Lijian Zhao, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

Some of the values used for GPIO_CFG and MISCCFG were not correct,
causing GPEs to not work correctly. This adjusts them according to the
values found in the original ACPI tables for the System76 Gazelle.

Unfortunately, the Intel documentation[1] mentioned below is
also incorrect. I have mentioned this to Intel already. The source
for the Intel CoffeeLake FSP also confirms these new numbers.

This was tested on a System76 Gazelle (gaze14). The EC uses GPP_K3 for
its GPE and GPP_K6 is used for the lid switch GPE. Both function
correctly after applying this change.

[1] Intel Document #572235:
    Intel ® 300 Series Chipset Families
    Platform Controller Hub
    External Design Specification (EDS) - Volume 2 of 2

Change-Id: I4ecc9552468037598ef5d4e10122d660dcbfe71d
Signed-off-by: Jeremy Soller 
---
M src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h
M src/soc/intel/cannonlake/include/soc/pmc.h
2 files changed, 24 insertions(+), 19 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/33941/8

Change in ...coreboot[master]: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33941 )

Change subject: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

Patch Set 8: Code-Review+2

Change in ...coreboot[master]: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33941 )

Change subject: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

Patch Set 8: Code-Review+1

not directly related to this patch, but did you also check if this is correct in inteltool?

Change in coreboot[master]: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

[Felix Held (Code Review)]

Felix Held has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33941 )

Change subject: soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H
......................................................................

soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H

Some of the values used for GPIO_CFG and MISCCFG were not correct,
causing GPEs to not work correctly. This adjusts them according to the
values found in the original ACPI tables for the System76 Gazelle.

Unfortunately, the Intel documentation[1] mentioned below is
also incorrect. I have mentioned this to Intel already. The source
for the Intel CoffeeLake FSP also confirms these new numbers.

This was tested on a System76 Gazelle (gaze14). The EC uses GPP_K3 for
its GPE and GPP_K6 is used for the lid switch GPE. Both function
correctly after applying this change.

[1] Intel Document #572235:
    Intel ® 300 Series Chipset Families
    Platform Controller Hub
    External Design Specification (EDS) - Volume 2 of 2

Change-Id: I4ecc9552468037598ef5d4e10122d660dcbfe71d
Signed-off-by: Jeremy Soller 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33941
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
Reviewed-by: Felix Held 
---
M src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h
M src/soc/intel/cannonlake/include/soc/pmc.h
2 files changed, 24 insertions(+), 19 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved
  Felix Held: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h b/src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h
index 5176ac7..2395314 100644
--- a/src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h
+++ b/src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h
@@ -22,18 +22,18 @@
  * communities.
  */

-#define GPP_A			0
-#define GPP_B			1
-#define GPP_C			2
-#define GPP_D			3
-#define GPP_G			4
-#define GPP_K			5
-#define GPP_H			6
-#define GPP_E			7
-#define GPP_F			8
-#define GPP_I			9
-#define GPP_J			0xA
-#define GPD			0xC
+#define GPP_A			0x0
+#define GPP_B			0x1
+#define GPP_C			0x2
+#define GPP_D			0x3
+#define GPP_G			0x4
+#define GPD			0x5
+#define GPP_E			0x6
+#define GPP_F			0x7
+#define GPP_H			0x8
+#define GPP_K			0x9
+#define GPP_I			0xA
+#define GPP_J			0xB
 #define GPIO_NUM_GROUPS		12
 #define GPIO_MAX_NUM_PER_GROUP	24

diff --git a/src/soc/intel/cannonlake/include/soc/pmc.h b/src/soc/intel/cannonlake/include/soc/pmc.h
index 67854d4..e0d2614 100644
--- a/src/soc/intel/cannonlake/include/soc/pmc.h
+++ b/src/soc/intel/cannonlake/include/soc/pmc.h
@@ -117,18 +117,23 @@
 #define  GPE0_DW_SHIFT(x)		(4*(x))

#if CONFIG(SOC_INTEL_CANNONLAKE_PCH_H)
+/*
+ * The values for GPIO_CFG in Intel Document #572235 are incorrect.
+ * These values now match what is used by the Intel CoffeeLake FSP,
+ * please do not modify them.
+ */
 #define PMC_GPP_A			0x0
 #define PMC_GPP_B			0x1
 #define PMC_GPP_C			0x2
 #define PMC_GPP_D			0x3
-#define PMC_GPP_E			0x7
-#define PMC_GPP_F			0x8
+#define PMC_GPP_E			0xA
+#define PMC_GPP_F			0xB
 #define PMC_GPP_G			0x4
-#define PMC_GPP_H			0x6
-#define PMC_GPP_I			0x9
-#define PMC_GPP_J			0xA
-#define PMC_GPP_K			0x5
-#define PMC_GPD				0xC
+#define PMC_GPP_H			0x9
+#define PMC_GPP_I			0xC
+#define PMC_GPP_J			0xD
+#define PMC_GPP_K			0x8
+#define PMC_GPD				0x7
 #else
 #define  PMC_GPP_A			0x0
 #define  PMC_GPP_B			0x1