Maxim Polyakov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44465 )
Change subject: mb/supermicro/x11-lga1151: undo set DRIVER for GPO ......................................................................
mb/supermicro/x11-lga1151: undo set DRIVER for GPO
GPIO Driver mode is used for configuration interrupt routing for external devices through GPI. But there is no point in configuring this for GPO. This patch replaces the PAD_CFG_GPO_GPIO_DRIVER macro with others that do not set the corresponding bit in the Host Software Pad Ownership register.
Change-Id: I5aed5769600bb9c442ad13a2efbcf9d5fd8da537 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/mainboard/supermicro/x11-lga1151-series/variants/x11ssm-f/include/variant/gpio.h 1 file changed, 27 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/44465/1
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 f5e71ad..98e2d62 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 @@ -34,8 +34,8 @@ PAD_NC(GPP_A23, NONE),
/* GPIO Group GPP_B */ - PAD_CFG_GPO_GPIO_DRIVER(GPP_B0, 1, DEEP, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_B1, 1, DEEP, NONE), + PAD_CFG_GPO(GPP_B0, 1, DEEP), + PAD_CFG_GPO(GPP_B1, 1, DEEP), PAD_NC(GPP_B2, NONE), PAD_NC(GPP_B3, NONE), PAD_NC(GPP_B4, NONE), @@ -45,7 +45,7 @@ PAD_NC(GPP_B8, NONE), PAD_NC(GPP_B9, NONE), PAD_NC(GPP_B10, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_B11, 0, DEEP, NONE), + PAD_CFG_GPO(GPP_B11, 0, DEEP), PAD_CFG_NF(GPP_B12, NONE, DEEP, NF1), PAD_CFG_NF(GPP_B13, NONE, DEEP, NF1), PAD_CFG_NF(GPP_B14, NONE, PLTRST, NF1), @@ -54,7 +54,7 @@ PAD_NC(GPP_B17, NONE), PAD_NC(GPP_B18, NONE), PAD_NC(GPP_B19, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_B20, 1, PLTRST, NONE), + PAD_CFG_GPO(GPP_B20, 1, PLTRST), PAD_NC(GPP_B21, NONE), PAD_NC(GPP_B22, NONE), PAD_CFG_NF(GPP_B23, NONE, DEEP, NF2), @@ -65,7 +65,7 @@ PAD_NC(GPP_C2, NONE), // PAD_CFG_NF(GPP_C3, NONE, DEEP, NF1), // PAD_CFG_NF(GPP_C4, NONE, DEEP, NF1), - PAD_CFG_GPO_GPIO_DRIVER(GPP_C5, 1, DEEP, NONE), + PAD_CFG_GPO(GPP_C5, 1, DEEP), // PAD_CFG_NF(GPP_C6, NONE, DEEP, NF1), // PAD_CFG_NF(GPP_C7, NONE, DEEP, NF1), PAD_CFG_GPI_INT(GPP_C8, NONE, PLTRST, OFF), @@ -87,10 +87,10 @@
/* GPIO Group GPP_D */ PAD_NC(GPP_D0, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_D1, 1, DEEP, NONE), + PAD_CFG_GPO(GPP_D1, 1, DEEP), PAD_CFG_GPI_NMI(GPP_D2, 20K_PU, DEEP, EDGE_SINGLE, NONE), PAD_NC(GPP_D3, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_D4, 0, PLTRST, NONE), + PAD_CFG_GPO(GPP_D4, 0, PLTRST), PAD_NC(GPP_D5, NONE), PAD_NC(GPP_D6, NONE), PAD_NC(GPP_D7, NONE), @@ -104,10 +104,10 @@ PAD_NC(GPP_D15, NONE), PAD_NC(GPP_D16, NONE), PAD_NC(GPP_D17, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_D18, 1, PLTRST, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_D19, 1, PLTRST, NONE), + PAD_CFG_GPO(GPP_D18, 1, PLTRST), + PAD_CFG_GPO(GPP_D19, 1, PLTRST), PAD_NC(GPP_D20, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_D21, 0, DEEP, NONE), + PAD_CFG_GPO(GPP_D21, 0, DEEP), PAD_CFG_GPI_INT(GPP_D22, NONE, RSMRST, OFF), PAD_NC(GPP_D23, NONE),
@@ -133,9 +133,9 @@ PAD_NC(GPP_F3, NONE), PAD_NC(GPP_F4, NONE), PAD_CFG_GPI_APIC(GPP_F5, NONE, PLTRST), - PAD_CFG_GPO_GPIO_DRIVER(GPP_F6, 1, PLTRST, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_F7, 1, PLTRST, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_F8, 1, PLTRST, NONE), + PAD_CFG_GPO(GPP_F6, 1, PLTRST), + PAD_CFG_GPO(GPP_F7, 1, PLTRST), + PAD_CFG_GPO(GPP_F8, 1, PLTRST), PAD_CFG_GPI_INT(GPP_F9, NONE, PLTRST, OFF), PAD_CFG_NF(GPP_F10, NONE, DEEP, NF1), PAD_CFG_NF(GPP_F11, NONE, DEEP, NF1), @@ -150,7 +150,7 @@ PAD_NC(GPP_F20, NONE), PAD_NC(GPP_F21, NONE), PAD_NC(GPP_F22, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_F23, 0, RSMRST, NONE), + PAD_CFG_GPO(GPP_F23, 0, RSMRST),
/* GPIO Group GPP_G */ PAD_CFG_GPI_INT(GPP_G0, NONE, DEEP, OFF), @@ -179,16 +179,16 @@ PAD_NC(GPP_G23, NONE),
/* GPIO Group GPP_H */ - PAD_CFG_GPO_GPIO_DRIVER(GPP_H0, 1, DEEP, NONE), + PAD_CFG_GPO(GPP_H0, 1, DEEP), PAD_CFG_GPI_INT(GPP_H1, NONE, PLTRST, OFF), - PAD_CFG_GPO_GPIO_DRIVER(GPP_H2, 1, DEEP, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_H3, 1, DEEP, NONE), + PAD_CFG_GPO(GPP_H2, 1, DEEP), + PAD_CFG_GPO(GPP_H3, 1, DEEP), PAD_CFG_GPI_INT(GPP_H4, NONE, PLTRST, OFF), - PAD_CFG_GPO_GPIO_DRIVER(GPP_H5, 1, PLTRST, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_H6, 1, PLTRST, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_H7, 1, PLTRST, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_H8, 1, PLTRST, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_H9, 1, PLTRST, NONE), + PAD_CFG_GPO(GPP_H5, 1, PLTRST), + PAD_CFG_GPO(GPP_H6, 1, PLTRST), + PAD_CFG_GPO(GPP_H7, 1, PLTRST), + PAD_CFG_GPO(GPP_H8, 1, PLTRST), + PAD_CFG_GPO(GPP_H9, 1, PLTRST), PAD_CFG_NF(GPP_H10, NONE, DEEP, NF1), PAD_CFG_NF(GPP_H11, NONE, DEEP, NF1), PAD_NC(GPP_H12, NONE), @@ -198,11 +198,11 @@ PAD_CFG_NF(GPP_H16, NONE, DEEP, NF1), PAD_CFG_NF(GPP_H17, NONE, DEEP, NF1), PAD_NC(GPP_H18, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_H19, 1, PLTRST, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_H20, 1, PLTRST, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_H21, 1, PLTRST, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_H22, 1, PLTRST, NONE), - PAD_CFG_GPO_GPIO_DRIVER(GPP_H23, 1, PLTRST, NONE), + PAD_CFG_GPO(GPP_H19, 1, PLTRST), + PAD_CFG_GPO(GPP_H20, 1, PLTRST), + PAD_CFG_GPO(GPP_H21, 1, PLTRST), + PAD_CFG_GPO(GPP_H22, 1, PLTRST), + PAD_CFG_GPO(GPP_H23, 1, PLTRST),
/* GPIO Group GPP_I */ PAD_CFG_NF(GPP_I0, NONE, DEEP, NF1),
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44465 )
Change subject: mb/supermicro/x11-lga1151: undo set DRIVER for GPO ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44465/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44465/1//COMMIT_MSG@9 PS1, Line 9: GPIO Driver mode is used for configuration interrupt routing for : external devices through GPI. This is true and also the only detail the register description of `host software pad ownership` mentions. But we can't imply that there are no other uses.
https://review.coreboot.org/c/coreboot/+/44465/1//COMMIT_MSG@10 PS1, Line 10: external devices through GPI. But there is no point in configuring : this for GPO. Why? can you give any reference?
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44465 )
Change subject: mb/supermicro/x11-lga1151: undo set DRIVER for GPO ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44465/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44465/1//COMMIT_MSG@10 PS1, Line 10: external devices through GPI. But there is no point in configuring : this for GPO.
Why? can you give any reference?
Please see page 73, document #549921 (Skylake/Kaby Lake H/LP Platform Controller Hub, BIOS Specification). Here you can see the algorithm for setting GPIO to GPIO driver mode:
Step 1: Configure the GPIO pin into GPI...
I looked at the docs again and unfortunately I didn't find any information about this for the GPO, info on how to do the same for a GPO.
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44465 )
Change subject: mb/supermicro/x11-lga1151: undo set DRIVER for GPO ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44465/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44465/1//COMMIT_MSG@10 PS1, Line 10: external devices through GPI. But there is no point in configuring : this for GPO.
Please see page 73, document #549921 (Skylake/Kaby Lake H/LP […]
@Nico, what do you think of this?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44465 )
Change subject: mb/supermicro/x11-lga1151: undo set DRIVER for GPO ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44465/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44465/1//COMMIT_MSG@10 PS1, Line 10: external devices through GPI. But there is no point in configuring : this for GPO.
@Nico, what do you think of this?
If I had such a board, I would start from scratch.
I think the history of the file is a mess. That's what we get when we allow to merge board ports that use internal macros that they shouldn't. No idea how to assess if the driver ownership is wanted for any pad (the original pad had bit 4 of the second word always set. does that make any sense?).
Using tools to translate that without reasoning about every single pad to other macros makes it worse. Now it looks like the driver ownership is intentional in specific cases. But it probably never was?
Marking this as resolved as my original concern is gone. Looking at the code it seems beyond reason. We shouldn't ask if it's reasonable to tag driver ownership. We should ask what was intended, first.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44465 )
Change subject: mb/supermicro/x11-lga1151: undo set DRIVER for GPO ......................................................................
Patch Set 1: Code-Review-1
see https://review.coreboot.org/c/coreboot/+/44471 - comment at Oct 9, 6:31 PM
Maxim Polyakov has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/44465 )
Change subject: mb/supermicro/x11-lga1151: undo set DRIVER for GPO ......................................................................
Abandoned
-
Maxim Polyakov (Code Review) -
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)