Change in coreboot[master]: mainboard/google/{hatch,fizz}: Fix gpio common code TODO

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38740 )

Change subject: mainboard/google/{hatch,fizz}: Fix gpio common code TODO
......................................................................

mainboard/google/{hatch,fizz}: Fix gpio common code TODO

This is common gpio code that can come up in other cases as
as well. See to fixing the TODO and not let it rot.

BUG=b:147992492
BRANCH=none
TEST=none

Change-Id: I6c67f04ae458a0e47f4bb022d744dd8b8a0789ee
Signed-off-by: Edward O'Callaghan 
---
M src/include/gpio.h
M src/lib/gpio.c
M src/mainboard/google/fizz/mainboard.c
M src/mainboard/google/hatch/variants/puff/mainboard.c
4 files changed, 34 insertions(+), 46 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/38740/1

diff --git a/src/include/gpio.h b/src/include/gpio.h
index 0a37ee7..3736f95 100644
--- a/src/include/gpio.h
+++ b/src/include/gpio.h
@@ -19,6 +19,8 @@
 #include <soc/gpio.h>
 #include <types.h>

+long wait_for_gpio(gpio_t gpio, long timeout);
+
 /* <soc/gpio.h> must typedef a gpio_t that fits in 32 bits. */
 _Static_assert(sizeof(gpio_t) <= sizeof(u32), "gpio_t doesn't fit in lb_gpio");

diff --git a/src/lib/gpio.c b/src/lib/gpio.c
index 8ea3b5e..578d583 100644
--- a/src/lib/gpio.c
+++ b/src/lib/gpio.c
@@ -16,6 +16,7 @@
 #include <base3.h>
 #include <console/console.h>
 #include <delay.h>
+#include <timer.h>
 #include <gpio.h>

static void _check_num(const char *name, int num)
@@ -179,6 +180,19 @@
    return result;
 }

+long wait_for_gpio(gpio_t gpio, long timeout)
+{
+	struct stopwatch sw;
+	stopwatch_init_msecs_expire(&sw, timeout);
+	while (!gpio_get(gpio)) {
+		if (stopwatch_expired(&sw)) {
+			return -1;
+		}
+		mdelay(200);
+	}
+	return stopwatch_duration_msecs(&sw);
+}
+
 /* Default handler for ACPI path is to return NULL */
 __weak const char *gpio_acpi_path(gpio_t gpio)
 {
diff --git a/src/mainboard/google/fizz/mainboard.c b/src/mainboard/google/fizz/mainboard.c
index 9397786..527d5b2 100644
--- a/src/mainboard/google/fizz/mainboard.c
+++ b/src/mainboard/google/fizz/mainboard.c
@@ -238,27 +238,6 @@
 #define GPIO_HDMI_HPD		GPP_E13
 #define GPIO_DP_HPD		GPP_E14

-/* TODO: This can be moved to common directory */
-static void wait_for_hpd(gpio_t gpio, long timeout)
-{
-	struct stopwatch sw;
-
-	printk(BIOS_INFO, "Waiting for HPD\n");
-	gpio_input(gpio);
-
-	stopwatch_init_msecs_expire(&sw, timeout);
-	while (!gpio_get(gpio)) {
-		if (stopwatch_expired(&sw)) {
-			printk(BIOS_WARNING,
-			       "HPD not ready after %ldms. Abort.\n", timeout);
-			return;
-		}
-		mdelay(200);
-	}
-	printk(BIOS_INFO, "HPD ready after %lu ms\n",
-	       stopwatch_duration_msecs(&sw));
-}
-
 static void mainboard_chip_init(void *chip_info)
 {
    const struct pad_config *pads;
@@ -270,8 +249,15 @@
    gpio_input(GPIO_HDMI_HPD);
    if (display_init_required() && !gpio_get(GPIO_HDMI_HPD)) {
    	/* This has to be done before FSP-S runs. */
-		if (google_chromeec_wait_for_displayport(display_timeout_ms))
-			wait_for_hpd(GPIO_DP_HPD, display_timeout_ms);
+		if (google_chromeec_wait_for_displayport(display_timeout_ms)) {
+			printk(BIOS_INFO, "Waiting for HPD\n");
+			long t = wait_for_gpio(GPIO_DP_HPD, display_timeout_ms);
+			if (t >= 0)
+				printk(BIOS_INFO, "HPD ready after %lu ms\n", t);
+			else
+				printk(BIOS_WARNING,
+				       "HPD not ready after %ldms. Abort.\n", timeout);
+		}
    }

pads = variant_gpio_table(&num);
diff --git a/src/mainboard/google/hatch/variants/puff/mainboard.c b/src/mainboard/google/hatch/variants/puff/mainboard.c
index 7354ce9..a245f6e 100644
--- a/src/mainboard/google/hatch/variants/puff/mainboard.c
+++ b/src/mainboard/google/hatch/variants/puff/mainboard.c
@@ -15,34 +15,13 @@

#include <baseboard/variants.h>
 #include <chip.h>
-#include <delay.h>
 #include <device/device.h>
 #include <ec/google/chromeec/ec.h>
 #include <gpio.h>
-#include <timer.h>

#define GPIO_HDMI_HPD		GPP_E13
 #define GPIO_DP_HPD		GPP_E14

-/* TODO: This can be moved to common directory */
-static void wait_for_hpd(gpio_t gpio, long timeout)
-{
-	struct stopwatch sw;
-
-	printk(BIOS_INFO, "Waiting for HPD\n");
-	stopwatch_init_msecs_expire(&sw, timeout);
-	while (!gpio_get(gpio)) {
-		if (stopwatch_expired(&sw)) {
-			printk(BIOS_WARNING,
-			       "HPD not ready after %ldms. Abort.\n", timeout);
-			return;
-		}
-		mdelay(200);
-	}
-	printk(BIOS_INFO, "HPD ready after %lu ms\n",
-	       stopwatch_duration_msecs(&sw));
-}
-
 void variant_ramstage_init(void)
 {
    static const long display_timeout_ms = 3000;
@@ -55,8 +34,15 @@
    	&& !gpio_get(GPIO_HDMI_HPD)
    	&& !gpio_get(GPIO_DP_HPD)) {
    	/* This has to be done before FSP-S runs. */
-		if (google_chromeec_wait_for_displayport(display_timeout_ms))
-			wait_for_hpd(GPIO_DP_HPD, display_timeout_ms);
+		if (google_chromeec_wait_for_displayport(display_timeout_ms)) {
+			printk(BIOS_INFO, "Waiting for HPD\n");
+			long t = wait_for_gpio(GPIO_DP_HPD, display_timeout_ms);
+			if (t >= 0)
+				printk(BIOS_INFO, "HPD ready after %lu ms\n", t);
+			else
+				printk(BIOS_WARNING,
+				       "HPD not ready after %ldms. Abort.\n", timeout);
+		}
    }
 }

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38740 )

Change subject: mainboard/google/{hatch,fizz}: Fix gpio common code TODO
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38740/1/src/lib/gpio.c 
File src/lib/gpio.c:

https://review.coreboot.org/c/coreboot/+/38740/1/src/lib/gpio.c@188 
PS1, Line 188: 		if (stopwatch_expired(&sw)) {
braces {} are not necessary for single statement blocks

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38740 )

Change subject: mainboard/google/{hatch,fizz}: Fix gpio common code TODO
......................................................................

Patch Set 1:

(1 comment)

This change is ready for review.

https://review.coreboot.org/c/coreboot/+/38740/1/src/lib/gpio.c 
File src/lib/gpio.c:

https://review.coreboot.org/c/coreboot/+/38740/1/src/lib/gpio.c@193 
PS1, Line 193: 	return stopwatch_duration_msecs(&sw);
Equivalent:

long timeout = wait_us(timeout, gpio_get(gpio));
 if (timeout)
   return timeout;
 return -1;

(Considering that just using wait_us() directly in the code is barely any longer, do we really need this function?)

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38740 )

Change subject: mainboard/google/{hatch,fizz}: Fix gpio common code TODO
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38740/1/src/lib/gpio.c 
File src/lib/gpio.c:

https://review.coreboot.org/c/coreboot/+/38740/1/src/lib/gpio.c@193 
PS1, Line 193: 	return stopwatch_duration_msecs(&sw);
> Equivalent: […]

edit: wait_ms() in this case, not wait_us(). (If we do decide to keep this function, you should call it wait_for_gpio_ms() or something to avoid confusion about the time unit.)

[Daisuke Nojiri (Code Review)]

Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38740 )

Change subject: mainboard/google/{hatch,fizz}: Fix gpio common code TODO
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38740/1/src/include/gpio.h 
File src/include/gpio.h:

https://review.coreboot.org/c/coreboot/+/38740/1/src/include/gpio.h@22 
PS1, Line 22: wait_for_gpio
Can you add function description? Especially return value and 200 ms poll period are important.