Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42874 )
Change subject: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL ......................................................................
mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL
H1 is not a wake source and hence there is no need to configure SCI GEVENT for it. This change drops PAD_SCI() configuration for GPIO_9 i.e. H1_PCH_INT_ODL.
BUG=b:159944426
Change-Id: Iec2285b76f9c5fa1b4b1be15128fea316fa04555 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/mainboard/google/kahlee/variants/baseboard/gpio.c 1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/42874/1
diff --git a/src/mainboard/google/kahlee/variants/baseboard/gpio.c b/src/mainboard/google/kahlee/variants/baseboard/gpio.c index abdcb0d..6d8dd52 100644 --- a/src/mainboard/google/kahlee/variants/baseboard/gpio.c +++ b/src/mainboard/google/kahlee/variants/baseboard/gpio.c @@ -20,7 +20,6 @@
/* GPIO_9 - H1_PCH_INT_ODL, SCI */ PAD_INT(GPIO_9, PULL_UP, EDGE_LOW, STATUS), - PAD_SCI(GPIO_9, PULL_UP, EDGE_LOW),
/* GPIO_15 - EC_IN_RW_OD */ PAD_GPI(GPIO_15, PULL_UP),
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42874 )
Change subject: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42874/2/src/mainboard/google/kahlee... File src/mainboard/google/kahlee/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/42874/2/src/mainboard/google/kahlee... PS2, Line 21: /* GPIO_9 - H1_PCH_INT_ODL, SCI */ remove SCI in comment?
https://review.coreboot.org/c/coreboot/+/42874/2/src/mainboard/google/kahlee... PS2, Line 30: /* GPIO_24 - EC_PCH_WAKE_L */ add SCI in comment?
Hello build bot (Jenkins), Raul Rangel, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42874
to look at the new patch set (#3).
Change subject: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL ......................................................................
mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL
H1 is not a wake source and hence there is no need to configure SCI GEVENT for it. This change drops PAD_SCI() configuration for GPIO_9 i.e. H1_PCH_INT_ODL.
BUG=b:159944426
Change-Id: Iec2285b76f9c5fa1b4b1be15128fea316fa04555 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/mainboard/google/kahlee/variants/baseboard/gpio.c 1 file changed, 2 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/42874/3
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42874 )
Change subject: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42874/2/src/mainboard/google/kahlee... File src/mainboard/google/kahlee/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/42874/2/src/mainboard/google/kahlee... PS2, Line 21: /* GPIO_9 - H1_PCH_INT_ODL, SCI */
remove SCI in comment?
Done
https://review.coreboot.org/c/coreboot/+/42874/2/src/mainboard/google/kahlee... PS2, Line 30: /* GPIO_24 - EC_PCH_WAKE_L */
add SCI in comment?
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42874 )
Change subject: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL ......................................................................
Patch Set 3: Code-Review+1
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42874 )
Change subject: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL ......................................................................
Patch Set 3: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42874 )
Change subject: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL ......................................................................
Patch Set 3: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42874 )
Change subject: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42874/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42874/4//COMMIT_MSG@7 PS4, Line 7: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL A bit off-topic..
What do each of H1_ PCH_ INT_ and ODL mean in this context?
The GPIO #defines are without _ODL in soc/amd. Some boards use CR50_IRQ here?
google/cheza and google/trogdor uses name GPIO_H1_AP_INT here?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42874 )
Change subject: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42874/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42874/4//COMMIT_MSG@7 PS4, Line 7: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL
A bit off-topic.. […]
H1_PCH_INT_ODL is basically the net name for this signal. H1 is the chip name for Google Security Chip (https://2018.osfc.io/uploads/talk/paper/7/gsc_copy.pdf). PCH is platform controller hub. INT means that the signal is an interrupt line. _ODL is used for open drain signals.
These apply to how the signal is routed and used on the mainboard design and hence the abbreviations might not appear in soc/amd.
EEs working on different platforms (Intel, AMD, QC, Rockchip, etc.) seem to follow very different schemes for net names and hence the differences in coreboot comments/macros.
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42874 )
Change subject: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL ......................................................................
mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL
H1 is not a wake source and hence there is no need to configure SCI GEVENT for it. This change drops PAD_SCI() configuration for GPIO_9 i.e. H1_PCH_INT_ODL.
BUG=b:159944426
Change-Id: Iec2285b76f9c5fa1b4b1be15128fea316fa04555 Signed-off-by: Furquan Shaikh furquan@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42874 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-by: Raul Rangel rrangel@chromium.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/mainboard/google/kahlee/variants/baseboard/gpio.c 1 file changed, 2 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, but someone else must approve Aaron Durbin: Looks good to me, approved Raul Rangel: Looks good to me, approved
diff --git a/src/mainboard/google/kahlee/variants/baseboard/gpio.c b/src/mainboard/google/kahlee/variants/baseboard/gpio.c index abdcb0d..59d7631 100644 --- a/src/mainboard/google/kahlee/variants/baseboard/gpio.c +++ b/src/mainboard/google/kahlee/variants/baseboard/gpio.c @@ -18,9 +18,8 @@ /* GPIO_6 - APU_RST_L / EC_SMI_ODL, SMI */ PAD_SMI(GPIO_6, PULL_UP, LEVEL_LOW),
- /* GPIO_9 - H1_PCH_INT_ODL, SCI */ + /* GPIO_9 - H1_PCH_INT_ODL */ PAD_INT(GPIO_9, PULL_UP, EDGE_LOW, STATUS), - PAD_SCI(GPIO_9, PULL_UP, EDGE_LOW),
/* GPIO_15 - EC_IN_RW_OD */ PAD_GPI(GPIO_15, PULL_UP), @@ -28,7 +27,7 @@ /* GPIO_22 - EC_SCI_ODL, SCI */ PAD_SCI(GPIO_22, PULL_UP, EDGE_LOW),
- /* GPIO_24 - EC_PCH_WAKE_L */ + /* GPIO_24 - EC_PCH_WAKE_L, SCI */ PAD_SCI(GPIO_24, PULL_UP, EDGE_LOW),
/* GPIO_26 - APU_PCIE_RST_L */
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42874 )
Change subject: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42874/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42874/4//COMMIT_MSG@7 PS4, Line 7: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL
H1_PCH_INT_ODL is basically the net name for this signal. […]
Thanks.. if they just were consistent. Evidently ther are some confusing aliases, would you sync those with schema.
kahlee
gpio.c:/* GPIO_9 - H1_PCH_INT_ODL */ gpio.h:#define H1_PCH_INT GPIO_9
zork
gpio_baseboard_common.c: /* H1_FCH_INT_ODL */ gpio.h: #define H1_PCH_INT GPIO_3
I associate PCH with Intel, google/kahlee is AMD. And open-drain sounds irrelevant for input pins so _ODL was just dropped at some places from the names it seems.
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42874 )
Change subject: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL ......................................................................
Patch Set 5:
Automatic boot test returned (PASS/FAIL/TOTAL): 0/4/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : FAIL : https://lava.9esec.io/r/7713 "QEMU x86 q35/ich9" using payload SeaBIOS : FAIL : https://lava.9esec.io/r/7712 "QEMU x86 i440fx/piix4" using payload SeaBIOS : FAIL : https://lava.9esec.io/r/7711 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : FAIL : https://lava.9esec.io/r/7710
Please note: This test is under development and might not be accurate at all!
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42874 )
Change subject: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42874/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42874/4//COMMIT_MSG@7 PS4, Line 7: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL Yeah, I will update the aliases. I actually have a change to drop the alias from zork: https://review.coreboot.org/c/coreboot/+/42941. I can do the same for kahlee.
I associate PCH with Intel, google/kahlee is AMD.
Yeah, I suspect that the initial net-naming got copied over from another project and hence the _PCH_ name remained. It is fixed in zork.
And open-drain sounds irrelevant for input pins so _ODL was just dropped at some places from the names it seems.
Actually, the signal is open-drain coming into the FCH/SoC because of the way the signal is routed in hardware.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42874 )
Change subject: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42874/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42874/4//COMMIT_MSG@7 PS4, Line 7: mb/google/kahlee: Do not enable SCI for H1_PCH_INT_ODL
Yeah, I will update the aliases. […]
Done: https://review.coreboot.org/c/coreboot/+/43015
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Raul Rangel (Code Review)