Change in coreboot[master]: mb/supermicro/x11-lga1151: 5/5 Rewrite pad config using intelp2m

[Maxim Polyakov (Code Review)]

Maxim Polyakov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42919 )

Change subject: mb/supermicro/x11-lga1151: 5/5 Rewrite pad config using intelp2m
......................................................................

mb/supermicro/x11-lga1151: 5/5 Rewrite pad config using intelp2m

Take into account the remapping of reset sources [1] for the Sunrice
Point PCH, to fix Reset Config fields (bit 31:30) in the corresponding
macros. The intelp2m utility does this remapping automatically, but
these changes in a separate patch to pay attention to them.

[1] src/soc/intel/skylake/gpio.c#n9

Change-Id: I388d39ca8314de60610588223d024e1bb1681264
Signed-off-by: Maxim Polyakov 
---
M src/mainboard/supermicro/x11-lga1151-series/variants/x11ssh-tf/include/variant/gpio.h
M src/mainboard/supermicro/x11-lga1151-series/variants/x11ssm-f/include/variant/gpio.h
2 files changed, 4 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/42919/1

diff --git a/src/mainboard/supermicro/x11-lga1151-series/variants/x11ssh-tf/include/variant/gpio.h b/src/mainboard/supermicro/x11-lga1151-series/variants/x11ssh-tf/include/variant/gpio.h
index d005d8b..114095a 100644
--- a/src/mainboard/supermicro/x11-lga1151-series/variants/x11ssh-tf/include/variant/gpio.h
+++ b/src/mainboard/supermicro/x11-lga1151-series/variants/x11ssh-tf/include/variant/gpio.h
@@ -284,7 +284,7 @@
    PAD_CFG_GPO(GPP_D21, 0, DEEP),

/* GPP_D22 - GPIO */
-	PAD_CFG_GPI_TRIG_OWN(GPP_D22, NONE, PWROK, OFF, ACPI),
+	PAD_CFG_GPI_TRIG_OWN(GPP_D22, NONE, RSMRST, OFF, ACPI),

/* GPP_D23 - GPIO */
    PAD_NC(GPP_D23, NONE),
@@ -398,7 +398,7 @@
    PAD_NC(GPP_F22, NONE),

/* GPP_F23 - GPIO */
-	PAD_CFG_GPO(GPP_F23, 0, PWROK),
+	PAD_CFG_GPO(GPP_F23, 0, RSMRST),

/* GPP_G0 - GPIO */
    PAD_CFG_GPI_TRIG_OWN(GPP_G0, NONE, DEEP, OFF, ACPI),
diff --git a/src/mainboard/supermicro/x11-lga1151-series/variants/x11ssm-f/include/variant/gpio.h b/src/mainboard/supermicro/x11-lga1151-series/variants/x11ssm-f/include/variant/gpio.h
index 4f16424..df13a21 100644
--- a/src/mainboard/supermicro/x11-lga1151-series/variants/x11ssm-f/include/variant/gpio.h
+++ b/src/mainboard/supermicro/x11-lga1151-series/variants/x11ssm-f/include/variant/gpio.h
@@ -298,7 +298,7 @@
    PAD_CFG_GPO_GPIO_DRIVER(GPP_D21, 0, DEEP, NONE),

/* GPP_D22 - GPIO */
-	PAD_CFG_GPI_TRIG_OWN(GPP_D22, NONE, PWROK, OFF, DRIVER),
+	PAD_CFG_GPI_TRIG_OWN(GPP_D22, NONE, RSMRST, OFF, DRIVER),

/* GPP_D23 - GPIO */
    PAD_NC(GPP_D23, NONE),
@@ -416,7 +416,7 @@
    PAD_NC(GPP_F22, NONE),

/* GPP_F23 - GPIO */
-	PAD_CFG_GPO_GPIO_DRIVER(GPP_F23, 0, PWROK, NONE),
+	PAD_CFG_GPO_GPIO_DRIVER(GPP_F23, 0, RSMRST, NONE),

/* GPIO Group GPP_G */

[Paul Menzel (Code Review)]

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

Change subject: mb/supermicro/x11-lga1151: 5/5 Rewrite pad config using intelp2m
......................................................................

Patch Set 3:

(3 comments)

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

https://review.coreboot.org/c/coreboot/+/42919/3//COMMIT_MSG@7 
PS3, Line 7: mb/supermicro/x11-lga1151: 5/5 Rewrite pad config using intelp2m
Use a more descriptive commit message for the actual change?

https://review.coreboot.org/c/coreboot/+/42919/3//COMMIT_MSG@9 
PS3, Line 9: Sunrice
Sunri*s*e?

https://review.coreboot.org/c/coreboot/+/42919/3//COMMIT_MSG@11 
PS3, Line 11: but
            : these changes in a separate patch to pay attention to them.
> but put these changes
> but make these changes

Change in coreboot[master]: supermicro/x11-lga1151/gpio: 5/5 Remap reset sources

[Maxim Polyakov (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: supermicro/x11-lga1151/gpio: 5/5 Remap reset sources
......................................................................

supermicro/x11-lga1151/gpio: 5/5 Remap reset sources

Take into account the remapping of reset sources [1] for the Sunrise
Point PCH, to fix Reset Config fields (bit 31:30) in the corresponding
macros. The intelp2m utility does this remapping automatically, but
make these changes in a separate patch to pay attention to them.

[1] src/soc/intel/skylake/gpio.c#n9

This is part of the patch set
"mb/supermicro/x11-lga1151: Rewrite pad config using intelp2m":

CB:42916 - 1/5 Decode raw register values
CB:42917 - 2/5 Exclude fields for PAD_CFG
CB:42918 - 3/5 Fixes some field macro
CB:35679 - 3/4 Convert field macros to PAD_CFG
CB:42919 - 5/5 Remap reset sources

Change-Id: I388d39ca8314de60610588223d024e1bb1681264
Signed-off-by: Maxim Polyakov 
---
M src/mainboard/supermicro/x11-lga1151-series/variants/x11ssh-tf/include/variant/gpio.h
M src/mainboard/supermicro/x11-lga1151-series/variants/x11ssm-f/include/variant/gpio.h
2 files changed, 4 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/42919/4

Change in coreboot[master]: supermicro/x11-lga1151/gpio: 5/5 Remap reset sources

[Maxim Polyakov (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: supermicro/x11-lga1151/gpio: 5/5 Remap reset sources
......................................................................

supermicro/x11-lga1151/gpio: 5/5 Remap reset sources

Take into account the remapping of reset sources [1] for the Sunrise
Point PCH, to fix Reset Config fields (bit 31:30) in the corresponding
macros. The intelp2m utility does this remapping automatically, but
make these changes in a separate patch to pay attention to them.

[1] src/soc/intel/skylake/gpio.c#n9

This is part of the patch set
"mb/supermicro/x11-lga1151: Rewrite pad config using intelp2m":

CB:42916 - 1/5 Decode raw register values
CB:42917 - 2/5 Exclude fields for PAD_CFG
CB:42918 - 3/5 Fixes some field macro
CB:35679 - 3/4 Convert field macros to PAD_CFG
CB:42919 - 5/5 Remap reset sources

Change-Id: I388d39ca8314de60610588223d024e1bb1681264
Signed-off-by: Maxim Polyakov 
---
M src/mainboard/supermicro/x11-lga1151-series/variants/x11ssh-tf/include/variant/gpio.h
M src/mainboard/supermicro/x11-lga1151-series/variants/x11ssm-f/include/variant/gpio.h
2 files changed, 4 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/42919/5

Change in coreboot[master]: supermicro/x11-lga1151/gpio: 5/5 Remap reset sources

[Maxim Polyakov (Code Review)]

Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42919 )

Change subject: supermicro/x11-lga1151/gpio: 5/5 Remap reset sources
......................................................................

Patch Set 5:

(3 comments)

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

https://review.coreboot.org/c/coreboot/+/42919/3//COMMIT_MSG@7 
PS3, Line 7: mb/supermicro/x11-lga1151: 5/5 Rewrite pad config using intelp2m
> Use a more descriptive commit message for the actual change?

May be "5/5 Remap reset sources"?

https://review.coreboot.org/c/coreboot/+/42919/3//COMMIT_MSG@9 
PS3, Line 9: Sunrice
> Sunri*s*e?

Done

https://review.coreboot.org/c/coreboot/+/42919/3//COMMIT_MSG@11 
PS3, Line 11: but
            : these changes in a separate patch to pay attention to them.
> > but put these changes […]

Done

Change in coreboot[master]: supermicro/x11-lga1151/gpio: 5/5 Remap reset sources

[Maxim Polyakov (Code Review)]

Maxim Polyakov has removed Paul Menzel from this change.  ( https://review.coreboot.org/c/coreboot/+/42919 )

Change subject: supermicro/x11-lga1151/gpio: 5/5 Remap reset sources
......................................................................

Removed reviewer Paul Menzel.

Change in coreboot[master]: [RFC] supermicro/x11-lga1151/gpio: 5/5 Remap reset sources

[Maxim Polyakov (Code Review)]

Hello build bot (Jenkins), Matt DeVillier, Benjamin Doron, Paul Menzel, Christian Walter, Angel Pons, Michael Niewöhner, Patrick Rudolph,

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

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

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

Change subject: [RFC] supermicro/x11-lga1151/gpio: 5/5 Remap reset sources
......................................................................

[RFC] supermicro/x11-lga1151/gpio: 5/5 Remap reset sources

Take into account the remapping of reset sources [1] for the Sunrise
Point PCH, to fix Reset Config fields (bit 31:30) in the corresponding
macros. The intelp2m utility does this remapping automatically, but
make these changes in a separate patch to pay attention to them.

[1] src/soc/intel/skylake/gpio.c#n9

This is part of the patch set
"mb/supermicro/x11-lga1151: Rewrite pad config using intelp2m":

CB:42916 - 1/5 Decode raw register values
CB:42917 - 2/5 Exclude fields for PAD_CFG
CB:42918 - 3/5 Fixes some field macro
CB:35679 - 3/4 Convert field macros to PAD_CFG
CB:42919 - 5/5 Remap reset sources

Change-Id: I388d39ca8314de60610588223d024e1bb1681264
Signed-off-by: Maxim Polyakov 
---
M src/mainboard/supermicro/x11-lga1151-series/variants/x11ssh-tf/include/variant/gpio.h
M src/mainboard/supermicro/x11-lga1151-series/variants/x11ssm-f/include/variant/gpio.h
2 files changed, 4 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/42919/6

Change in coreboot[master]: [RFC] supermicro/x11-lga1151/gpio: 5/5 Remap reset sources

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42919 )

Change subject: [RFC] supermicro/x11-lga1151/gpio: 5/5 Remap reset sources
......................................................................

Patch Set 6:

so what exactly is the net effect of this patchset? In the process of converting from the raw dataword values to macros, you've obviously changed quite a bit in the way coreboot sets these GPIOs, in terms of values/features set.

Is there a reason to make these changes, other than the macros providing a clearer indication of how the GPIO is set/used? If not, do the changes affect how the GPIOs function? If yes, how?

Change in coreboot[master]: [RFC] supermicro/x11-lga1151/gpio: 5/5 Remap reset sources

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42919 )

Change subject: [RFC] supermicro/x11-lga1151/gpio: 5/5 Remap reset sources
......................................................................

Patch Set 6:
> Patch Set 6:
>
> so what exactly is the net effect of this patchset? In the process of converting from the raw dataword values to macros, you've obviously changed quite a bit in the way coreboot sets these GPIOs, in terms of values/features set.
>
> Is there a reason to make these changes, other than the macros providing a clearer indication of how the GPIO is set/used? If not, do the changes affect how the GPIOs function? If yes, how?

No, it should and shall not change the way they function but just convert the ugly raw values to human-readable macros.

Change in coreboot[master]: [RFC] supermicro/x11-lga1151/gpio: 5/5 Remap reset sources

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42919 )

Change subject: [RFC] supermicro/x11-lga1151/gpio: 5/5 Remap reset sources
......................................................................

Patch Set 6:
> Patch Set 6:
>
> No, it should and shall not change the way they function but just convert the ugly raw values to human-readable macros.

so the output from `inteltool -g` is identical before/after the patchset?

Change in coreboot[master]: [RFC] supermicro/x11-lga1151/gpio: 5/5 Remap reset sources

[Maxim Polyakov (Code Review)]

Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42919 )

Change subject: [RFC] supermicro/x11-lga1151/gpio: 5/5 Remap reset sources
......................................................................

Patch Set 6:
> Patch Set 6:
> > Patch Set 6:
> >
> > No, it should and shall not change the way they function but just convert the ugly raw values to human-readable macros.
>
> so the output from `inteltool -g` is identical before/after the patchset?

No, the dump of the DW0 and DW1 registers is different before and after the patches. To convert raw values to macros, we should ignore some bit fields. For example, RX/TX Buf Disable DW0[9:8] doesn't affect the pad in native function mode (intel doc #549921) and the PAD_CFG_NF macros don't take them into account (the value is set to 0). I change the values of the DW registers in separate patches:

CB:42917 - 2/5 Exclude fields for PAD_CFG
CB:42918 - 3/5 Fixes some field macro

Thus, it is easier to find a mistake or do revert to a patch in order to repeat the testing.
This is @Angel’s idea (This helped me fix some bugs in the utility. Good idea, thanks).

Similarly done for up board (Apollo Lake):

CB:42608 - 1/3 Decode raw register values
CB:42915 - 2/3 Exclude fields that are not in PAD_CFG*
CB:39765 - 3/3 Converts bit field macros to PAD_CFG

___

The last 5/5 patch isn't correct, as the reset source remapping is already applied in the configuration with raw values DW0 and DW1.

Change in coreboot[master]: [RFC] supermicro/x11-lga1151/gpio: 5/5 Remap reset sources

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42919 )

Change subject: [RFC] supermicro/x11-lga1151/gpio: 5/5 Remap reset sources
......................................................................

Patch Set 6:
> Patch Set 6:
> > Patch Set 6:
> > > Patch Set 6:
> > >
> > > No, it should and shall not change the way they function but just convert the ugly raw values to human-readable macros.
> >
> > so the output from `inteltool -g` is identical before/after the patchset?
>
> No, the dump of the DW0 and DW1 registers is different before and after the patches. To convert raw values to macros, we should ignore some bit fields. For example, RX/TX Buf Disable DW0[9:8] doesn't affect the pad in native function mode (intel doc #549921) and the PAD_CFG_NF macros don't take them into account (the value is set to 0). I change the values of the DW registers in separate patches:
>
> CB:42917 - 2/5 Exclude fields for PAD_CFG
> CB:42918 - 3/5 Fixes some field macro
>
> Thus, it is easier to find a mistake or do revert to a patch in order to repeat the testing.
> This is @Angel’s idea (This helped me fix some bugs in the utility. Good idea, thanks).
>
> Similarly done for up board (Apollo Lake):
>
> CB:42608 - 1/3 Decode raw register values
> CB:42915 - 2/3 Exclude fields that are not in PAD_CFG*
> CB:39765 - 3/3 Converts bit field macros to PAD_CFG
>
> The last 5/5 patch isn't correct, as the reset source remapping is already applied in the configuration with raw values DW0 and DW1.

Well, yes, the raw values differ but that does not change "the way they [the GPIOs] function", which is exactly the reaso *why* those bits can be ignored :-)

Change in coreboot[master]: [RFC] supermicro/x11-lga1151/gpio: 5/5 Remap reset sources

[Maxim Polyakov (Code Review)]

Maxim Polyakov has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42919 )

Change subject: [RFC] supermicro/x11-lga1151/gpio: 5/5 Remap reset sources
......................................................................

Abandoned

This patch isn't correct, as the reset source remapping is already applied in the configuration with raw values DW0 and DW1 (commit 71b1ed8f7)