Change in coreboot[master]: gpio: Pull down HiZ pins after reading tristate GPIO strapping

[Julius Werner (Code Review)]

Hello Furquan Shaikh, Alexandru Stan, Douglas Anderson,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: gpio: Pull down HiZ pins after reading tristate GPIO strapping
......................................................................

gpio: Pull down HiZ pins after reading tristate GPIO strapping

People who know a lot more about electrons and stuff than I do tell me
that leaving a HiZ pin floating without a pull resistor may waste power.
So if we find a pin to be HiZ when reading tristate strapping GPIOs, we
should make sure the internal pull-down is enabled when we're done with
it. (For pins that are externally pulled high or low, we should continue
to leave the internal pull disabled instead.)

Signed-off-by: Julius Werner 
Change-Id: I1669823c8a7faab536e0441cb4c6cfeb9f696189
---
M src/lib/gpio.c
1 file changed, 6 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/44253/1

diff --git a/src/lib/gpio.c b/src/lib/gpio.c
index 3f3ae60..801a3b3 100644
--- a/src/lib/gpio.c
+++ b/src/lib/gpio.c
@@ -114,6 +114,12 @@
    	printk(BIOS_DEBUG, "%c ", tristate_char[temp]);
    	result = (result * 3) + temp;

+		/* Disable pull to avoid wasting power. For HiZ we leave the
+		   pull-down enabled, since letting them float freely back and
+		   forth may waste power in the SoC's GPIO input logic. */
+		if (temp != Z)
+			gpio_input(gpio[index]);
+
    	/*
    	 * For binary_first we keep track of the normal ternary result
    	 * and whether we found any pin that was a Z. We also determine
@@ -159,10 +165,6 @@
    printk(BIOS_DEBUG, "= %d (%s base3 number system)\n", result,
           binary_first ? "binary_first" : "standard");

-	/* Disable pull up / pull down to conserve power */
-	for (index = 0; index < num_gpio; ++index)
-		gpio_input(gpio[index]);
-
    return result;
 }

[Douglas Anderson (Code Review)]

Douglas Anderson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44253 )

Change subject: gpio: Pull down HiZ pins after reading tristate GPIO strapping
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

I don't have +2, but this looks fine to me.  I assume coreboot folks know what the rules are for comment style, but it seemed weird to me that it didn't match the rest of the function.

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

https://review.coreboot.org/c/coreboot/+/44253/1/src/lib/gpio.c@117 
PS1, Line 117: /* Disable pull to avoid wasting power. For HiZ we leave the
             : 		   pull-down enabled, since letting them float freely back and
             : 		   forth may waste power in the SoC's GPIO input logic. */
Different comment style matter?

[Julius Werner (Code Review)]

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

Change subject: gpio: Pull down HiZ pins after reading tristate GPIO strapping
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/44253/1/src/lib/gpio.c@117 
PS1, Line 117: /* Disable pull to avoid wasting power. For HiZ we leave the
             : 		   pull-down enabled, since letting them float freely back and
             : 		   forth may waste power in the SoC's GPIO input logic. */
> Different comment style matter?

This is correct "short form" coreboot style (the one below is the "long form").

[Alexandru Stan (Code Review)]

Alexandru Stan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44253 )

Change subject: gpio: Pull down HiZ pins after reading tristate GPIO strapping
......................................................................

Patch Set 1: Code-Review+1

Looks good from the functional side.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44253 )

Change subject: gpio: Pull down HiZ pins after reading tristate GPIO strapping
......................................................................

Patch Set 1: Code-Review+2

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44253 )

Change subject: gpio: Pull down HiZ pins after reading tristate GPIO strapping
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/44253/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/44253/1//COMMIT_MSG@10 
PS1, Line 10: that leaving a HiZ pin floating without a pull resistor may waste power.
It can cause the FETs to switch a lot because of noise.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44253 )

Change subject: gpio: Pull down HiZ pins after reading tristate GPIO strapping
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/44253/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/44253/1//COMMIT_MSG@10 
PS1, Line 10: that leaving a HiZ pin floating without a pull resistor may waste power.
> It can cause the FETs to switch a lot because of noise.

And FETs primarily waste power when logic levels change. That's why we have clock gating everywhere, for example.

[Julius Werner (Code Review)]

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44253 )

Change subject: gpio: Pull down HiZ pins after reading tristate GPIO strapping
......................................................................

gpio: Pull down HiZ pins after reading tristate GPIO strapping

People who know a lot more about electrons and stuff than I do tell me
that leaving a HiZ pin floating without a pull resistor may waste power.
So if we find a pin to be HiZ when reading tristate strapping GPIOs, we
should make sure the internal pull-down is enabled when we're done with
it. (For pins that are externally pulled high or low, we should continue
to leave the internal pull disabled instead.)

Signed-off-by: Julius Werner 
Change-Id: I1669823c8a7faab536e0441cb4c6cfeb9f696189
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44253
Reviewed-by: Douglas Anderson 
Reviewed-by: Alexandru Stan 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Aaron Durbin 
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/lib/gpio.c
1 file changed, 6 insertions(+), 4 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, approved
  Furquan Shaikh: Looks good to me, approved
  Douglas Anderson: Looks good to me, but someone else must approve
  Angel Pons: Looks good to me, approved
  Alexandru Stan: Looks good to me, but someone else must approve

diff --git a/src/lib/gpio.c b/src/lib/gpio.c
index 3f3ae60..801a3b3 100644
--- a/src/lib/gpio.c
+++ b/src/lib/gpio.c
@@ -114,6 +114,12 @@
    	printk(BIOS_DEBUG, "%c ", tristate_char[temp]);
    	result = (result * 3) + temp;

+		/* Disable pull to avoid wasting power. For HiZ we leave the
+		   pull-down enabled, since letting them float freely back and
+		   forth may waste power in the SoC's GPIO input logic. */
+		if (temp != Z)
+			gpio_input(gpio[index]);
+
    	/*
    	 * For binary_first we keep track of the normal ternary result
    	 * and whether we found any pin that was a Z. We also determine
@@ -159,10 +165,6 @@
    printk(BIOS_DEBUG, "= %d (%s base3 number system)\n", result,
           binary_first ? "binary_first" : "standard");

-	/* Disable pull up / pull down to conserve power */
-	for (index = 0; index < num_gpio; ++index)
-		gpio_input(gpio[index]);
-
    return result;
 }