Change in coreboot[master]: mb/google/zork: Don't generate power resource for Raydium TS
Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
mb/google/zork: Don't generate power resource for Raydium TS
The Raydium ACPI entry currently provides a reset GPIO and an _ON/_OFF method to the kernel. These are contradictory. The ownership of the GPIO should be mutually exclusive between either the OS or the FW. The linux kernel driver has support for the reset GPIO, so omit the power resource.
I also changed the default GPIO value to high to leave the device in reset. The kernel will de-assert this when it's ready to talk with the device. It also asserts it before entering sleep, so the value is consistent.
BUG=b:160854397 TEST=Boot trembyle and make sure TS works. Suspend/Resume trembyle 300+ times.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I23131be5d7109eed660a8bd6e2c156c015aa3c4e --- M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c M src/mainboard/google/zork/variants/berknip/overridetree.cb M src/mainboard/google/zork/variants/dalboz/overridetree.cb M src/mainboard/google/zork/variants/ezkinil/overridetree.cb M src/mainboard/google/zork/variants/trembyle/overridetree.cb 6 files changed, 2 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/43467/1
diff --git a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c index 23b5458..c7e9823 100644 --- a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c +++ b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c @@ -166,7 +166,7 @@ /* DEV_BEEP_BCLK */ PAD_GPI(GPIO_139, PULL_NONE), /* USI_RESET */ - PAD_GPO(GPIO_140, LOW), + PAD_GPO(GPIO_140, HIGH), /* USB_HUB_RST_L */ PAD_GPO(GPIO_141, HIGH), /* BT_DISABLE */ diff --git a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c index 640b765..4d2e609 100644 --- a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c +++ b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c @@ -158,7 +158,7 @@ /* DEV_BEEP_BCLK */ PAD_GPI(GPIO_139, PULL_NONE), /* USI_RESET */ - PAD_GPO(GPIO_140, LOW), + PAD_GPO(GPIO_140, HIGH), /* UART1_RXD - FPMCU */ PAD_NF(GPIO_141, UART1_RXD, PULL_NONE), /* UART1_TXD - FPMCU */ diff --git a/src/mainboard/google/zork/variants/berknip/overridetree.cb b/src/mainboard/google/zork/variants/berknip/overridetree.cb index 229469e..8491e9d 100644 --- a/src/mainboard/google/zork/variants/berknip/overridetree.cb +++ b/src/mainboard/google/zork/variants/berknip/overridetree.cb @@ -83,8 +83,6 @@ register "probed" = "1" register "irq_gpio" = "ACPI_GPIO_IRQ_EDGE_LOW(GPIO_12)" register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPIO_140)" - register "reset_delay_ms" = "20" - register "has_power_resource" = "1" device i2c 10 on end end chip drivers/i2c/hid diff --git a/src/mainboard/google/zork/variants/dalboz/overridetree.cb b/src/mainboard/google/zork/variants/dalboz/overridetree.cb index 81fc4fd..cffc2c3 100644 --- a/src/mainboard/google/zork/variants/dalboz/overridetree.cb +++ b/src/mainboard/google/zork/variants/dalboz/overridetree.cb @@ -52,8 +52,6 @@ register "probed" = "1" register "irq_gpio" = "ACPI_GPIO_IRQ_EDGE_LOW(GPIO_12)" register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPIO_140)" - register "reset_delay_ms" = "20" - register "has_power_resource" = "1" device i2c 39 on end end chip drivers/i2c/generic diff --git a/src/mainboard/google/zork/variants/ezkinil/overridetree.cb b/src/mainboard/google/zork/variants/ezkinil/overridetree.cb index f7366ac..64a391e 100644 --- a/src/mainboard/google/zork/variants/ezkinil/overridetree.cb +++ b/src/mainboard/google/zork/variants/ezkinil/overridetree.cb @@ -75,8 +75,6 @@ register "probed" = "1" register "irq_gpio" = "ACPI_GPIO_IRQ_EDGE_LOW(GPIO_12)" register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPIO_140)" - register "reset_delay_ms" = "20" - register "has_power_resource" = "1" device i2c 39 on end end chip drivers/i2c/hid diff --git a/src/mainboard/google/zork/variants/trembyle/overridetree.cb b/src/mainboard/google/zork/variants/trembyle/overridetree.cb index 22aced2..e28adc5 100644 --- a/src/mainboard/google/zork/variants/trembyle/overridetree.cb +++ b/src/mainboard/google/zork/variants/trembyle/overridetree.cb @@ -143,8 +143,6 @@ register "probed" = "1" register "irq_gpio" = "ACPI_GPIO_IRQ_EDGE_LOW(GPIO_12)" register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPIO_140)" - register "reset_delay_ms" = "20" - register "has_power_resource" = "1" device i2c 39 on end end chip drivers/i2c/generic
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43467/2/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/berknip/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43467/2/src/mainboard/google/zork/v... PS2, Line 81: ELAN0001 Not Raydium TS.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43467/2/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/berknip/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43467/2/src/mainboard/google/zork/v... PS2, Line 81: ELAN0001
Not Raydium TS.
good catch
Raul Rangel has uploaded a new patch set (#3). ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
mb/google/zork: Don't generate power resource for Raydium TS
The Raydium ACPI entry currently provides a reset GPIO and an _ON/_OFF method to the kernel. These are contradictory. The ownership of the GPIO should be mutually exclusive between either the OS or the FW. The linux kernel driver has support for the reset GPIO, so omit the power resource.
BUG=b:160854397 TEST=Boot trembyle and make sure TS works. Suspend/Resume trembyle 300+ times.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I23131be5d7109eed660a8bd6e2c156c015aa3c4e --- M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_dalboz.c M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c M src/mainboard/google/zork/variants/berknip/overridetree.cb M src/mainboard/google/zork/variants/dalboz/overridetree.cb M src/mainboard/google/zork/variants/ezkinil/overridetree.cb M src/mainboard/google/zork/variants/trembyle/overridetree.cb 6 files changed, 2 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/43467/3
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG@16 PS4, Line 16: 300 I'm not quite at 300 yet, but I was going to let it run to 300.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43467/4/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/dalboz/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43467/4/src/mainboard/google/zork/v... PS4, Line 55: register "reset_delay_ms" = "20" Don't we need to keep the reset_delay_ms to enforce timing?
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43467/4/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/dalboz/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43467/4/src/mainboard/google/zork/v... PS4, Line 55: register "reset_delay_ms" = "20"
Don't we need to keep the reset_delay_ms to enforce timing?
The kernel driver has the correct code to enforce timing: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/th...
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43467/4/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/dalboz/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43467/4/src/mainboard/google/zork/v... PS4, Line 55: register "reset_delay_ms" = "20"
The kernel driver has the correct code to enforce timing: https://source.chromium. […]
That code is using the exposed resources, though, right? I was worried about _ON/_OFF timing as there's not a guaranteed delay from what I can tell.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43467/4/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/dalboz/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43467/4/src/mainboard/google/zork/v... PS4, Line 55: register "reset_delay_ms" = "20"
That code is using the exposed resources, though, right? I was worried about _ON/_OFF timing as ther […]
Or did you mean that we are exposing reset_gpio and that code doesn't meet the condition you linked. Therefore RM_POWERON_DELAY_USEC and RM_RESET_DELAY_MSEC are employed?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43467/4/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/dalboz/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43467/4/src/mainboard/google/zork/v... PS4, Line 55: register "reset_delay_ms" = "20"
Or did you mean that we are exposing reset_gpio and that code doesn't meet the condition you linked. […]
Oh. has_power_resource also controls _ON/_OFF generation.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43467/4/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/dalboz/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43467/4/src/mainboard/google/zork/v... PS4, Line 55: register "reset_delay_ms" = "20"
Oh. has_power_resource also controls _ON/_OFF generation.
That's right. This change is basically dropping _ON/_OFF routines for the device. So, reset_delay_ms is not required.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG@13 PS4, Line 13: resource. Should fit on the line above.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
Patch Set 4: Code-Review+2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG@7 PS4, Line 7: Don't generate power resource for Raydium TS I'm going to change this CL to not provide the GPIO to the kernel instead. That way we can account for the really long rise time on the reset line.
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG@16 PS4, Line 16: 300
I'm not quite at 300 yet, but I was going to let it run to 300.
I got 400+
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG@7 PS4, Line 7: Don't generate power resource for Raydium TS
I'm going to change this CL to not provide the GPIO to the kernel instead. […]
But that's only for one design that wasn't correct, no? You want to generate the _ON/_OFF methods, and not expose the reset gpio?
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG@7 PS4, Line 7: Don't generate power resource for Raydium TS
But that's only for one design that wasn't correct, no? You want to generate the _ON/_OFF methods, a […]
The Trembyle builds all have the rise time problem. As for the partner devices I can get the EEs to clarify which devices and which builds have the problem.
Yeah, we need to generate the _ON/_OFF if we want to support a custom reset time. The kernel driver has a hard coded 50ms.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG@7 PS4, Line 7: Don't generate power resource for Raydium TS
The Trembyle builds all have the rise time problem. […]
OK. Please include that information in your updated patch. We're going to need to fix all this once we fix hardware while supporting these older broken designs.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG@7 PS4, Line 7: Don't generate power resource for Raydium TS
OK. Please include that information in your updated patch. […]
After some testing, I think I would rather merge this for now. Once we get a power GPIO I think we can revisit this.
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG@13 PS4, Line 13: resource.
Should fit on the line above.
doesn't fit.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't generate power resource for Raydium TS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG@7 PS4, Line 7: Don't generate power resource for Raydium TS
After some testing, I think I would rather merge this for now. […]
Raul, do you mind updating commit description w/ your findings about your other testing? That way we'll have some more info to go back over and provide some context. Thanks.
Hello build bot (Jenkins), Paul Menzel, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43467
to look at the new patch set (#5).
Change subject: mb/google/zork: Don't expose reset GPIO for touchscreen to OS ......................................................................
mb/google/zork: Don't expose reset GPIO for touchscreen to OS
The Raydium ACPI entry currently provides a reset GPIO and an _ON/_OFF method to the kernel. These are contradictory. The ownership of the GPIO should be mutually exclusive between either the OS or the FW. Since we have two methods exposed this causes the OS to reset the TS twice. Once using the _ON method, and once using the GPIO. Additionally the _ON method is waiting for 20ms after reset while the OS driver uses a 50ms delay. The Raydium TS datasheet specifies 20ms for FW ready time, so the OS driver is adding additional padding.
The reference design has a 32ms rise time on the reset line. So without this patch, the OS tries to reset the TS using the _ON method and it waits for 20ms. This is not enough time for the reset line to reach high, let alone account for the FW ready time. The OS driver then tries to reset the device by toggling the GPIO. It waits 50ms which is still 2ms less than required.
This CL removes the GPIO from being exported in the _CRS so the OS driver won't try and reset the device. It also increases the reset delay by 32ms to account for the rise time.
This isn't a complete fix. I think that the slow rise time is causing some kind of metastability in the TS reset hardware. Using a script to bind and unbind the TS driver, the TS device becomes unresponsive after ~200 iterations. The only way to reset the device is to power cycle.
The TS power is also not currently controlled by the power resource. This means that we have no guarantee over when the reset line is toggled. This will lead to issues while spending and resuming.
BUG=b:160854397 TEST=Boot trembyle and make sure TS works. Suspend/Resume trembyle 300+ times.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I23131be5d7109eed660a8bd6e2c156c015aa3c4e --- M src/mainboard/google/zork/variants/berknip/overridetree.cb M src/mainboard/google/zork/variants/dalboz/overridetree.cb M src/mainboard/google/zork/variants/ezkinil/overridetree.cb M src/mainboard/google/zork/variants/trembyle/overridetree.cb 4 files changed, 16 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/43467/5
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't expose reset GPIO for touchscreen to OS ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43467/4//COMMIT_MSG@7 PS4, Line 7: Don't generate power resource for Raydium TS
Raul, do you mind updating commit description w/ your findings about your other testing? That way we […]
Done
Hello build bot (Jenkins), Furquan Shaikh, Paul Menzel, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43467
to look at the new patch set (#6).
Change subject: mb/google/zork: Don't expose reset GPIO for touchscreen to OS ......................................................................
mb/google/zork: Don't expose reset GPIO for touchscreen to OS
The Raydium ACPI entry currently provides a reset GPIO and an _ON/_OFF method to the kernel. These are contradictory. The ownership of the GPIO should be mutually exclusive between either the OS or the FW. Since we have two methods exposed this causes the OS to reset the TS twice. Once using the _ON method, and once using the GPIO. Additionally the _ON method is waiting for 20ms after reset while the OS driver uses a 50ms delay. The Raydium TS datasheet specifies 20ms for FW ready time, so the OS driver is adding additional padding.
The reference design has a 32ms rise time on the reset line. So without this patch, the OS tries to reset the TS using the _ON method and it waits for 20ms. This is not enough time for the reset line to reach high, let alone account for the FW ready time. The OS driver then tries to reset the device by toggling the GPIO. It waits 50ms which is still 2ms less than required.
This CL removes the GPIO from being exported in the _CRS so the OS driver won't try and reset the device. It also increases the reset delay by 32ms to account for the rise time.
This isn't a complete fix. I think that the slow rise time is causing some kind of metastability in the TS reset hardware. Using a script to bind and unbind the TS driver, the TS device becomes unresponsive after ~200 iterations. The only way to reset the device is to power cycle.
The TS power is also not currently controlled by the power resource. This means that we have no guarantee over when the reset line is toggled. This will lead to issues while spending and resuming.
BUG=b:160854397 TEST=Boot trembyle and make sure TS works. Suspend/Resume trembyle 300+ times.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I23131be5d7109eed660a8bd6e2c156c015aa3c4e --- M src/mainboard/google/zork/variants/berknip/overridetree.cb M src/mainboard/google/zork/variants/dalboz/overridetree.cb M src/mainboard/google/zork/variants/ezkinil/overridetree.cb M src/mainboard/google/zork/variants/trembyle/overridetree.cb 4 files changed, 16 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/43467/6
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't expose reset GPIO for touchscreen to OS ......................................................................
Patch Set 6: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't expose reset GPIO for touchscreen to OS ......................................................................
Patch Set 6: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't expose reset GPIO for touchscreen to OS ......................................................................
Patch Set 6: Code-Review+2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't expose reset GPIO for touchscreen to OS ......................................................................
Patch Set 6:
Can this be merged?
Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't expose reset GPIO for touchscreen to OS ......................................................................
mb/google/zork: Don't expose reset GPIO for touchscreen to OS
The Raydium ACPI entry currently provides a reset GPIO and an _ON/_OFF method to the kernel. These are contradictory. The ownership of the GPIO should be mutually exclusive between either the OS or the FW. Since we have two methods exposed this causes the OS to reset the TS twice. Once using the _ON method, and once using the GPIO. Additionally the _ON method is waiting for 20ms after reset while the OS driver uses a 50ms delay. The Raydium TS datasheet specifies 20ms for FW ready time, so the OS driver is adding additional padding.
The reference design has a 32ms rise time on the reset line. So without this patch, the OS tries to reset the TS using the _ON method and it waits for 20ms. This is not enough time for the reset line to reach high, let alone account for the FW ready time. The OS driver then tries to reset the device by toggling the GPIO. It waits 50ms which is still 2ms less than required.
This CL removes the GPIO from being exported in the _CRS so the OS driver won't try and reset the device. It also increases the reset delay by 32ms to account for the rise time.
This isn't a complete fix. I think that the slow rise time is causing some kind of metastability in the TS reset hardware. Using a script to bind and unbind the TS driver, the TS device becomes unresponsive after ~200 iterations. The only way to reset the device is to power cycle.
The TS power is also not currently controlled by the power resource. This means that we have no guarantee over when the reset line is toggled. This will lead to issues while spending and resuming.
BUG=b:160854397 TEST=Boot trembyle and make sure TS works. Suspend/Resume trembyle 300+ times.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I23131be5d7109eed660a8bd6e2c156c015aa3c4e Reviewed-on: https://review.coreboot.org/c/coreboot/+/43467 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Martin Roth martinroth@google.com Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/mainboard/google/zork/variants/berknip/overridetree.cb M src/mainboard/google/zork/variants/dalboz/overridetree.cb M src/mainboard/google/zork/variants/ezkinil/overridetree.cb M src/mainboard/google/zork/variants/trembyle/overridetree.cb 4 files changed, 16 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Aaron Durbin: Looks good to me, approved Furquan Shaikh: Looks good to me, approved
diff --git a/src/mainboard/google/zork/variants/berknip/overridetree.cb b/src/mainboard/google/zork/variants/berknip/overridetree.cb index ead39cb8..31bf643 100644 --- a/src/mainboard/google/zork/variants/berknip/overridetree.cb +++ b/src/mainboard/google/zork/variants/berknip/overridetree.cb @@ -66,8 +66,11 @@ register "probed" = "1" register "irq_gpio" = "ACPI_GPIO_IRQ_EDGE_LOW(GPIO_12)" register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPIO_140)" - register "reset_delay_ms" = "20" + # 32ms: Rise time of the reset line + # 20ms: Firmware ready time + register "reset_delay_ms" = "32 + 20" register "has_power_resource" = "1" + register "disable_gpio_export_in_crs" = "1" device i2c 39 on end end chip drivers/i2c/generic diff --git a/src/mainboard/google/zork/variants/dalboz/overridetree.cb b/src/mainboard/google/zork/variants/dalboz/overridetree.cb index bcfff1f..9fe15c5 100644 --- a/src/mainboard/google/zork/variants/dalboz/overridetree.cb +++ b/src/mainboard/google/zork/variants/dalboz/overridetree.cb @@ -49,8 +49,11 @@ register "probed" = "1" register "irq_gpio" = "ACPI_GPIO_IRQ_EDGE_LOW(GPIO_12)" register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPIO_140)" - register "reset_delay_ms" = "20" + # 32ms: Rise time of the reset line + # 20ms: Firmware ready time + register "reset_delay_ms" = "32 + 20" register "has_power_resource" = "1" + register "disable_gpio_export_in_crs" = "1" device i2c 39 on end end chip drivers/i2c/generic diff --git a/src/mainboard/google/zork/variants/ezkinil/overridetree.cb b/src/mainboard/google/zork/variants/ezkinil/overridetree.cb index a4cdac6..5579015 100644 --- a/src/mainboard/google/zork/variants/ezkinil/overridetree.cb +++ b/src/mainboard/google/zork/variants/ezkinil/overridetree.cb @@ -68,8 +68,11 @@ register "probed" = "1" register "irq_gpio" = "ACPI_GPIO_IRQ_EDGE_LOW(GPIO_12)" register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPIO_140)" - register "reset_delay_ms" = "20" + # 32ms: Rise time of the reset line + # 20ms: Firmware ready time + register "reset_delay_ms" = "32 + 20" register "has_power_resource" = "1" + register "disable_gpio_export_in_crs" = "1" device i2c 39 on end end chip drivers/i2c/hid diff --git a/src/mainboard/google/zork/variants/trembyle/overridetree.cb b/src/mainboard/google/zork/variants/trembyle/overridetree.cb index 4cd53ee..bead293 100644 --- a/src/mainboard/google/zork/variants/trembyle/overridetree.cb +++ b/src/mainboard/google/zork/variants/trembyle/overridetree.cb @@ -66,8 +66,11 @@ register "probed" = "1" register "irq_gpio" = "ACPI_GPIO_IRQ_EDGE_LOW(GPIO_12)" register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPIO_140)" - register "reset_delay_ms" = "20" + # 32ms: Rise time of the reset line + # 20ms: Firmware ready time + register "reset_delay_ms" = "32 + 20" register "has_power_resource" = "1" + register "disable_gpio_export_in_crs" = "1" device i2c 39 on end end chip drivers/i2c/generic
Martin Roth has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/43467 )
Change subject: mb/google/zork: Don't expose reset GPIO for touchscreen to OS ......................................................................
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Martin Roth (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)