Change in coreboot[master]: mb/google/kukui: fix damu touchscreen reset sequence

[Magf - (Code Review)]

Magf - has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43319 )

Change subject: mb/google/kukui: fix damu touchscreen reset sequence
......................................................................

mb/google/kukui: fix damu touchscreen reset sequence

Damu touchscreen is a typical hid-over-i2c device and its reset pin
has a sequence requirement (T5) > 500us. Kernel hid-over-i2c driver
has no interface to support a reset pin, so current implementation
will be using a default pull down pin and rely on kernel to pull
it to high to make it exit reset. But when warm reboot, because
kernel will not pull it low, if we want a reset, we can pull it low
and rely on kernel to release it to get a valid reset > 500us.

BUG=b:159688118
BRANCH=kukui
TEST=build and boot damu device, when warm reboot, we can get a
     valid reset sequence which is greater than 500us.

Change-Id: I069f5ef3e9477410d5349e5a702a4fbc14c201ed
Signed-off-by: Paul Ma 
---
M src/mainboard/google/kukui/chromeos.c
M src/mainboard/google/kukui/gpio.h
2 files changed, 4 insertions(+), 0 deletions(-)

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

diff --git a/src/mainboard/google/kukui/chromeos.c b/src/mainboard/google/kukui/chromeos.c
index c5810d1..755a386 100644
--- a/src/mainboard/google/kukui/chromeos.c
+++ b/src/mainboard/google/kukui/chromeos.c
@@ -15,6 +15,9 @@
    gpio_input_pullup(CR50_IRQ);
    gpio_output(GPIO_RESET, 0);
    gpio_output(GPIO_EN_SPK_AMP, 0);
+
+	if (CONFIG(BOARD_GOOGLE_DAMU))
+		gpio_output(GPIO_TOUCH_RST, 0);
 }

void fill_lb_gpios(struct lb_gpios *gpios)
diff --git a/src/mainboard/google/kukui/gpio.h b/src/mainboard/google/kukui/gpio.h
index c71fe3e..e0329ce 100644
--- a/src/mainboard/google/kukui/gpio.h
+++ b/src/mainboard/google/kukui/gpio.h
@@ -11,6 +11,7 @@
 #define CR50_IRQ	GPIO(PERIPHERAL_EN3)
 #define GPIO_RESET	GPIO(PERIPHERAL_EN8)
 #define GPIO_EN_SPK_AMP	GPIO(PERIPHERAL_EN12)
+#define GPIO_TOUCH_RST	GPIO(ANT_SEL1)

void setup_chromeos_gpios(void);

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43319 )

Change subject: mb/google/kukui: fix damu touchscreen reset sequence
......................................................................

Patch Set 1:

I think the "touchpad" is not a chromeos specific thing, so this is probably not the right place (any linux kernel would need it right?)

Please move the gpio_ouptut to mainboard.c, as

static void configure_touchscreen(void) {
     if (....)
        gpio_output()...
  }

static void mainboard_init() {
     ...
     configure_touchscreen();
     ...
  }

[Magf - (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Chen-Tsung Hsieh,

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

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

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

Change subject: mb/google/kukui: fix damu touchscreen reset sequence
......................................................................

mb/google/kukui: fix damu touchscreen reset sequence

Damu touchscreen is a typical hid-over-i2c device and its reset pin
has a sequence requirement (T5) > 500us. Kernel hid-over-i2c driver
has no interface to support a reset pin, so current implementation
will be using a default pull down pin and rely on kernel to pull
it to high to make it exit reset. But when warm reboot, because
kernel will not pull it low, if we want a reset, we can pull it low
and rely on kernel to release it to get a valid reset > 500us.

BUG=b:159688118
BRANCH=kukui
TEST=build and boot damu device, when warm reboot, we can get a
     valid reset sequence which is greater than 500us.

Change-Id: I069f5ef3e9477410d5349e5a702a4fbc14c201ed
Signed-off-by: Paul Ma 
---
M src/mainboard/google/kukui/gpio.h
M src/mainboard/google/kukui/mainboard.c
2 files changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/43319/2

[Magf - (Code Review)]

Magf - has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43319 )

Change subject: mb/google/kukui: fix damu touchscreen reset sequence
......................................................................

Patch Set 2:
> Patch Set 1:
>
> I think the "touchpad" is not a chromeos specific thing, so this is probably not the right place (any linux kernel would need it right?)
>
> Please move the gpio_ouptut to mainboard.c, as
>
> static void configure_touchscreen(void) {
>      if (....)
>         gpio_output()...
>   }
>
> static void mainboard_init() {
>      ...
>      configure_touchscreen();
>      ...
>   }

Yes, the patch ensure touchscreen get a valid reset when OS do warm reset. Has refactored the code according to your suggestions. thanks.

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43319 )

Change subject: mb/google/kukui: fix damu touchscreen reset sequence
......................................................................

Patch Set 2:

(2 comments)

Thanks. Only few nits left.

https://review.coreboot.org/c/coreboot/+/43319/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43319/2//COMMIT_MSG@13 
PS2, Line 13: But when warm reboot, because
            : kernel will not pull it low, if we want a reset, we can pull it low
            : and rely on kernel to release it to get a valid reset > 500us.
            : 
In warm boot, we have to pull it low in firmware stage so kernel can release it to get a valid reset > 500us.

https://review.coreboot.org/c/coreboot/+/43319/2/src/mainboard/google/kukui/... 
File src/mainboard/google/kukui/mainboard.c:

https://review.coreboot.org/c/coreboot/+/43319/2/src/mainboard/google/kukui/... 
PS2, Line 198: configure_touchscreen();
We want to configure emmc & USB as early as possible.
Please move touchscreen init to after configure_usb or configure_audio.

[Magf - (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Chen-Tsung Hsieh,

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

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

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

Change subject: mb/google/kukui: fix damu touchscreen reset sequence
......................................................................

mb/google/kukui: fix damu touchscreen reset sequence

Damu touchscreen is a typical hid-over-i2c device and its reset pin
has a sequence requirement (T5) > 500us. Kernel hid-over-i2c driver
has no interface to support a reset pin, so current implementation
will be using a default pull down pin and rely on kernel to pull
it to high to make it exit reset. In warm boot, we have to pull it
low in firmware stage so kernel can release it to get a valid
reset > 500us.

BUG=b:159688118
BRANCH=kukui
TEST=build and boot damu device, when warm reboot, we can get a
     valid reset sequence which is greater than 500us.

Change-Id: I069f5ef3e9477410d5349e5a702a4fbc14c201ed
Signed-off-by: Paul Ma 
---
M src/mainboard/google/kukui/gpio.h
M src/mainboard/google/kukui/mainboard.c
2 files changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/43319/3

[Magf - (Code Review)]

Magf - has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43319 )

Change subject: mb/google/kukui: fix damu touchscreen reset sequence
......................................................................

Patch Set 3:

(2 comments)

Have fixed, please help review again, thanks.

https://review.coreboot.org/c/coreboot/+/43319/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43319/2//COMMIT_MSG@13 
PS2, Line 13: But when warm reboot, because
            : kernel will not pull it low, if we want a reset, we can pull it low
            : and rely on kernel to release it to get a valid reset > 500us.
            :
> In warm boot, we have to pull it low in firmware stage so kernel can release it to get a valid reset […]

Done

https://review.coreboot.org/c/coreboot/+/43319/2/src/mainboard/google/kukui/... 
File src/mainboard/google/kukui/mainboard.c:

https://review.coreboot.org/c/coreboot/+/43319/2/src/mainboard/google/kukui/... 
PS2, Line 198: configure_touchscreen();
> We want to configure emmc & USB as early as possible. […]

Done

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43319 )

Change subject: mb/google/kukui: fix damu touchscreen reset sequence
......................................................................

Patch Set 3: Code-Review+2

[Magf - (Code Review)]

Magf - has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43319 )

Change subject: mb/google/kukui: fix damu touchscreen reset sequence
......................................................................

Patch Set 3: Code-Review-1

This change will lead to touchscreen not able to work sometimes,

[Magf - (Code Review)]

Magf - has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43319 )

Change subject: mb/google/kukui: fix damu touchscreen reset sequence
......................................................................

Patch Set 3:

I'll do more test to verify.

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43319 )

Change subject: mb/google/kukui: fix damu touchscreen reset sequence
......................................................................

Patch Set 3: -Code-Review

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43319 )

Change subject: mb/google/kukui: fix damu touchscreen reset sequence
......................................................................

Patch Set 3:

@magf any updates?

[Magf - (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Chen-Tsung Hsieh,

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

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

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

Change subject: mb/google/kukui: fix damu touchscreen reset sequence
......................................................................

mb/google/kukui: fix damu touchscreen reset sequence

Damu touchscreen is a typical hid-over-i2c device and its reset pin
has a sequence requirement (T5) > 500us. Kernel hid-over-i2c driver
has no interface to support a reset pin, so current implementation
will be using a default pull down pin and rely on kernel to pull
it to high to make it exit reset. In warm boot, in order to get a
valid reset, we have to pull it low then high in firmware stage
to construct a valid reset.

BUG=b:159688118
BRANCH=kukui
TEST=build and boot damu device, when warm reboot, we can get a
     valid reset sequence which is greater than 500us.

Change-Id: I069f5ef3e9477410d5349e5a702a4fbc14c201ed
Signed-off-by: Paul Ma 
---
M src/mainboard/google/kukui/gpio.h
M src/mainboard/google/kukui/mainboard.c
2 files changed, 12 insertions(+), 0 deletions(-)

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

[Magf - (Code Review)]

Magf - has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43319 )

Change subject: mb/google/kukui: fix damu touchscreen reset sequence
......................................................................

Patch Set 4:
> Patch Set 3:
>
> @magf any update

patchset 3 has no function issue sometimes. patchset 4 will construct a 500us reset in coreboot so that both cold boot and warm boot can have a reset pulse. I tested single device and works well, we will do more verification with more UNITs and feedback.

thanks.

[S] Change in coreboot[master]: mb/google/kukui: fix damu touchscreen reset sequence

[Stefan Reinauer (Code Review)]

Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/43319?usp=email )

Change subject: mb/google/kukui: fix damu touchscreen reset sequence
......................................................................

Abandoned