Nick Vaccaro has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35967 )
Change subject: mb/google/poppy/variant/nocturne: don't invert GPP_D17 ......................................................................
mb/google/poppy/variant/nocturne: don't invert GPP_D17
Removed inversion of GPP_D17 was causing the device to get stuck in a reboot loop where the kernel was crashing within the first couple seconds of kernel boot.
BUG=b:142515200 BRANCH=none TEST=Flash and boot nocturne, verify boot is stable and that device doesn't reboot after jumping into kernel.
Change-Id: Ia1408ef6ea92f6b31a9f3eee8720954af3a7c382 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/mainboard/google/poppy/variants/nocturne/gpio.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/35967/1
diff --git a/src/mainboard/google/poppy/variants/nocturne/gpio.c b/src/mainboard/google/poppy/variants/nocturne/gpio.c index a4ea3c3..c62317a 100644 --- a/src/mainboard/google/poppy/variants/nocturne/gpio.c +++ b/src/mainboard/google/poppy/variants/nocturne/gpio.c @@ -196,7 +196,7 @@ /* D16 : ISH_UART0_CTS# ==> RCAM_RST_L */ PAD_CFG_GPO(GPP_D16, 0, DEEP), /* D17 : DMIC_CLK1 ==> EC_PCH_ARCORE_INT_L */ - PAD_CFG_GPI_APIC_INVERT(GPP_D17, NONE, PLTRST), + PAD_CFG_GPI_APIC(GPP_D17, NONE, PLTRST), /* D18 : DMIC_DATA1 ==> TP131 */ PAD_CFG_NC(GPP_D18), /* D19 : DMIC_CLK0 ==> PCH_DMIC_CLK_OUT */
Gwendal Grignou has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35967 )
Change subject: mb/google/poppy/variant/nocturne: don't invert GPP_D17 ......................................................................
Patch Set 1:
Check sensor are still working with tast test: "tast -build=false -verbose run <ip> hardware.SensorRing"
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35967 )
Change subject: mb/google/poppy/variant/nocturne: don't invert GPP_D17 ......................................................................
Patch Set 1: Code-Review-2
I'm placing this CL into a -2 state as I'm not convinced yet that this is the right way to fix the problem and have some more investigation to do before I'm convinced. I will adjust the level back once I've completed those investigations if it's determined this is the right way to fix this problem.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35967 )
Change subject: mb/google/poppy/variant/nocturne: don't invert GPP_D17 ......................................................................
Patch Set 1:
Patch Set 1:
Check sensor are still working with tast test: "tast -build=false -verbose run <ip> hardware.SensorRing"
Gwendal, tast fails in the following way :
(cr) (firmware-nocturne-10984.B/(a41058a...)) nvaccaro@nvaccaro ~/trunk/src/scripts $ tast -verbose run $KOHAKU hardware.SensorRing 2019/10/15 15:01:21 Command line: tast -verbose run 100.90.30.10 hardware.SensorRing 2019/10/15 15:01:21 Using SSH key /home/nvaccaro/trunk/chromite/ssh_keys/testing_rsa 2019/10/15 15:01:21 Using SSH dir /home/nvaccaro/.ssh 2019/10/15 15:01:21 Writing results to /tmp/tast/results/20191015-150121 2019/10/15 15:01:21 Connecting to 100.90.30.10 2019/10/15 15:01:22 Getting architecture from target 2019/10/15 15:01:22 Building chromiumos/tast/local/bundles/cros from /home/nvaccaro/trunk/src/platform/tast-tests 2019/10/15 15:01:26 Built test bundle in 4.592s 2019/10/15 15:01:26 Pushing test bundle /tmp/tast/build/x86_64/local_bundles/cros to /usr/local/libexec/tast/bundles/local_pushed on target 2019/10/15 15:01:27 Pushed test bundle in 690ms (sent 3.1 MB) 2019/10/15 15:01:27 Getting data file list from target 2019/10/15 15:01:27 Failed building or pushing tests: failed to get data file list: Process exited with status 2: args not set for mode 2
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35967 )
Change subject: mb/google/poppy/variant/nocturne: don't invert GPP_D17 ......................................................................
Patch Set 1: -Code-Review
Change has been verified in that the volume buttons still work in the dev screen and it passes the tast test for sensors.
Yicheng Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35967 )
Change subject: mb/google/poppy/variant/nocturne: don't invert GPP_D17 ......................................................................
Patch Set 1: Code-Review+1
Yicheng Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35967 )
Change subject: mb/google/poppy/variant/nocturne: don't invert GPP_D17 ......................................................................
Patch Set 1:
Hi Furquan and Gwendal, can we land this?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35967 )
Change subject: mb/google/poppy/variant/nocturne: don't invert GPP_D17 ......................................................................
Patch Set 1: Code-Review+2
Nick has been trying out some experiments, but +2 in case this seems absolutely necessary. For debug we should check ITSS polarities before and after FSP runs to ensure that it is not getting messed up.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35967 )
Change subject: mb/google/poppy/variant/nocturne: don't invert GPP_D17 ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35967/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35967/1//COMMIT_MSG@9 PS1, Line 9: Removed “Removed” in some commit?
https://review.coreboot.org/c/coreboot/+/35967/1//COMMIT_MSG@10 PS1, Line 10: where s/where/because/?
https://review.coreboot.org/c/coreboot/+/35967/1//COMMIT_MSG@12 PS1, Line 12: What does the schematic say?
https://review.coreboot.org/c/coreboot/+/35967/1//COMMIT_MSG@16 PS1, Line 16: device doesn't reboot after jumping into kernel. Some more words fit on the line above.
Hello Gwendal Grignou, Gwendal Grignou, Yicheng Li, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35967
to look at the new patch set (#2).
Change subject: mb/google/poppy/variant/nocturne: don't invert GPP_D17 ......................................................................
mb/google/poppy/variant/nocturne: don't invert GPP_D17
This change removes an inversion of GPP_D17 that caused the device to get stuck in a reboot loop because the kernel was crashing within the first couple seconds of kernel boot.
BUG=b:142515200 BRANCH=none TEST=Flash and boot nocturne, verify boot is stable and that device doesn't reboot after jumping into kernel, and that it passes the 'tast -verbose run <ip> hardware.SensorRing' test.
Change-Id: Ia1408ef6ea92f6b31a9f3eee8720954af3a7c382 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- M src/mainboard/google/poppy/variants/nocturne/gpio.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/35967/2
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35967 )
Change subject: mb/google/poppy/variant/nocturne: don't invert GPP_D17 ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35967/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35967/1//COMMIT_MSG@9 PS1, Line 9: Removed
“Removed” in some commit?
Done
https://review.coreboot.org/c/coreboot/+/35967/1//COMMIT_MSG@10 PS1, Line 10: where
s/where/because/?
Done
https://review.coreboot.org/c/coreboot/+/35967/1//COMMIT_MSG@16 PS1, Line 16: device doesn't reboot after jumping into kernel.
Some more words fit on the line above.
Done
Yicheng Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35967 )
Change subject: mb/google/poppy/variant/nocturne: don't invert GPP_D17 ......................................................................
Patch Set 2:
Hi, this is blocking our fingerprint firmware uprev for Nocturne, so it would be very helpful to land this soon. Thanks!
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35967 )
Change subject: mb/google/poppy/variant/nocturne: don't invert GPP_D17 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35967/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35967/1//COMMIT_MSG@12 PS1, Line 12:
What does the schematic say?
The schematic just shows that this GPIO goes to a GPIO on the EC.
I agree that this commit message makes it sound like there may not be a good understanding of what actually caused the error. I trust that Nick has a better explanation and was just simplifying things for the sake of a simple commit message.
Martin Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35967 )
Change subject: mb/google/poppy/variant/nocturne: don't invert GPP_D17 ......................................................................
mb/google/poppy/variant/nocturne: don't invert GPP_D17
This change removes an inversion of GPP_D17 that caused the device to get stuck in a reboot loop because the kernel was crashing within the first couple seconds of kernel boot.
BUG=b:142515200 BRANCH=none TEST=Flash and boot nocturne, verify boot is stable and that device doesn't reboot after jumping into kernel, and that it passes the 'tast -verbose run <ip> hardware.SensorRing' test.
Change-Id: Ia1408ef6ea92f6b31a9f3eee8720954af3a7c382 Signed-off-by: Nick Vaccaro nvaccaro@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35967 Reviewed-by: Yicheng Li yichengli@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/poppy/variants/nocturne/gpio.c 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Yicheng Li: Looks good to me, but someone else must approve
diff --git a/src/mainboard/google/poppy/variants/nocturne/gpio.c b/src/mainboard/google/poppy/variants/nocturne/gpio.c index a4ea3c3..c62317a 100644 --- a/src/mainboard/google/poppy/variants/nocturne/gpio.c +++ b/src/mainboard/google/poppy/variants/nocturne/gpio.c @@ -196,7 +196,7 @@ /* D16 : ISH_UART0_CTS# ==> RCAM_RST_L */ PAD_CFG_GPO(GPP_D16, 0, DEEP), /* D17 : DMIC_CLK1 ==> EC_PCH_ARCORE_INT_L */ - PAD_CFG_GPI_APIC_INVERT(GPP_D17, NONE, PLTRST), + PAD_CFG_GPI_APIC(GPP_D17, NONE, PLTRST), /* D18 : DMIC_DATA1 ==> TP131 */ PAD_CFG_NC(GPP_D18), /* D19 : DMIC_CLK0 ==> PCH_DMIC_CLK_OUT */
-
Furquan Shaikh (Code Review) -
Gwendal Grignou (Code Review)
-
Martin Roth (Code Review)
-
Nick Vaccaro (Code Review)
-
Paul Menzel (Code Review)
-
Yicheng Li (Code Review)