Change in ...coreboot[master]: google/kukui: Get write protection status from WP GPIO

[Hung-Te Lin (Code Review)]

Hung-Te Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32336

Change subject: google/kukui: Get write protection status from WP GPIO
......................................................................

google/kukui: Get write protection status from WP GPIO

Write protection (get_write_protect_state) was hard-coded to 0 and
should be fixed to read from correct GPIO (PERIPHERAL_EN0 from
schematics).

BUG=b:130681408
TEST=make -j; boots on Kukui Rev2.
BRANCH=None

Change-Id: I75b98b1d587abe5e8cdf3df28ea661bc1ffa19f9
Signed-off-by: Hung-Te Lin 
---
M src/mainboard/google/kukui/chromeos.c
M src/mainboard/google/kukui/gpio.h
2 files changed, 5 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/32336/1

diff --git a/src/mainboard/google/kukui/chromeos.c b/src/mainboard/google/kukui/chromeos.c
index 09c47ee..44e75e2 100644
--- a/src/mainboard/google/kukui/chromeos.c
+++ b/src/mainboard/google/kukui/chromeos.c
@@ -22,6 +22,7 @@

void setup_chromeos_gpios(void)
 {
+	gpio_input(GPIO_WP);
    gpio_input_pullup(EC_IN_RW);
    gpio_input_pullup(EC_IRQ);
    gpio_input_pullup(CR50_IRQ);
@@ -31,6 +32,8 @@
 void fill_lb_gpios(struct lb_gpios *gpios)
 {
    struct lb_gpio chromeos_gpios[] = {
+		{GPIO_WP.id, ACTIVE_LOW,
+		 !get_write_protect_state(), "write protect"},
    	{-1, ACTIVE_HIGH, get_recovery_mode_switch(), "recovery"},
    	{EC_IN_RW.id, ACTIVE_HIGH, -1, "EC in RW"},
    	{EC_IRQ.id, ACTIVE_LOW, -1, "EC interrupt"},
@@ -41,7 +44,7 @@

int get_write_protect_state(void)
 {
-	return 0;
+	return !gpio_get(GPIO_WP);
 }

int tis_plat_irq_status(void)
diff --git a/src/mainboard/google/kukui/gpio.h b/src/mainboard/google/kukui/gpio.h
index 024b0d7..92d238e 100644
--- a/src/mainboard/google/kukui/gpio.h
+++ b/src/mainboard/google/kukui/gpio.h
@@ -18,6 +18,7 @@

#include <soc/gpio.h>

+#define GPIO_WP		GPIO(PERIPHERAL_EN0)
 #define EC_IRQ		GPIO(PERIPHERAL_EN1)
 #define EC_IN_RW	GPIO(PERIPHERAL_EN14)
 #define CR50_IRQ	GPIO(PERIPHERAL_EN3)

[Tony Lin (Code Review)]

Tony Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32336 )

Change subject: google/kukui: Get write protection status from WP GPIO
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32336/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32336/1//COMMIT_MSG@14 
PS1, Line 14: TEST=make -j; boots on Kukui Rev2.
Do you want to check *wp on/off* in cr50 console and see if AP gets the correct wp state?

[You-Cheng Syu (Code Review)]

You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32336 )

Change subject: google/kukui: Get write protection status from WP GPIO
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/#/c/32336/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32336/1//COMMIT_MSG@14 
PS1, Line 14: TEST=make -j; boots on Kukui Rev2.
> Do you want to check *wp on/off* in cr50 console and see if AP gets the correct wp state?

Verified that "wp on/off" can change the result of "crossystem wpsw_cur"

[You-Cheng Syu (Code Review)]

You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32336 )

Change subject: google/kukui: Get write protection status from WP GPIO
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32336/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32336/1//COMMIT_MSG@14 
PS1, Line 14: TEST=make -j; boots on Kukui Rev2.
> Verified that "wp on/off" can change the result of "crossystem wpsw_cur"

wpsw_boot is also correct

[Hung-Te Lin (Code Review)]

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

Change subject: google/kukui: Get write protection status from WP GPIO
......................................................................

Patch Set 1:

Thanks for verification.

+jwerner for final review.

[Daisuke Nojiri (Code Review)]

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

Change subject: google/kukui: Get write protection status from WP GPIO
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32336/1/src/mainboard/google/kukui/chromeos.... 
File src/mainboard/google/kukui/chromeos.c:

https://review.coreboot.org/#/c/32336/1/src/mainboard/google/kukui/chromeos.... 
PS1, Line 36: !
You're negating here and also at line 47. Can you verify this is intended?

[Hung-Te Lin (Code Review)]

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

Change subject: google/kukui: Get write protection status from WP GPIO
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32336/1/src/mainboard/google/kukui/chromeos.... 
File src/mainboard/google/kukui/chromeos.c:

https://review.coreboot.org/#/c/32336/1/src/mainboard/google/kukui/chromeos.... 
PS1, Line 36: !
> You're negating here and also at line 47. […]

The GPIO_WP is active low.

get_write_protect_state always returns 1 = enabled, so we have to add ! for it.

lb_gpio provides a way for payloads to read GPIO dynamically using GPIO_WP, so we have to specify ACTIVE_LOW here. As a result, the initial value must be reversed again from get_write_protect_state results, because the value will be evaluated as active_low then.

I think this is very common across all boards - see cheza, butterfly, veyron, storm, even gru for example. So there should be no need to document explicitly?

[Daisuke Nojiri (Code Review)]

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

Change subject: google/kukui: Get write protection status from WP GPIO
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32336/1/src/mainboard/google/kukui/chromeos.... 
File src/mainboard/google/kukui/chromeos.c:

https://review.coreboot.org/#/c/32336/1/src/mainboard/google/kukui/chromeos.... 
PS1, Line 36: !
> The GPIO_WP is active low. […]

Can we do 'gpio_get(GPIO_WP)' like daisy or gru?

[Hung-Te Lin (Code Review)]

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

Change subject: google/kukui: Get write protection status from WP GPIO
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32336/1/src/mainboard/google/kukui/chromeos.... 
File src/mainboard/google/kukui/chromeos.c:

https://review.coreboot.org/#/c/32336/1/src/mainboard/google/kukui/chromeos.... 
PS1, Line 36: !
> Can we do 'gpio_get(GPIO_WP)' like daisy or gru?

gru:
 fill_lb_gpios: get_write_protect_state() ^ !wp_polarity,
 get_write_protect_state: gpio_get(GPIO_WP) ^ !wp_polarity;

daisy:
 fill_lb_gpios: !get_write_protect_state()
 get_write_protect_state:  !gpio_get_value(GPIO_D16)

They are both doing exactly same thing (well, gru is slightly different because it has to deal with multiple config, but at least it's following same scheme).

[Daisuke Nojiri (Code Review)]

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

Change subject: google/kukui: Get write protection status from WP GPIO
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

https://review.coreboot.org/#/c/32336/1/src/mainboard/google/kukui/chromeos.... 
File src/mainboard/google/kukui/chromeos.c:

https://review.coreboot.org/#/c/32336/1/src/mainboard/google/kukui/chromeos.... 
PS1, Line 36: !
> gru: […]

Looks like it's changed recently by https://review.coreboot.org/c/coreboot/+/32233 for whatever reason. Thanks for clarification.

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32336 )

Change subject: google/kukui: Get write protection status from WP GPIO
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

https://review.coreboot.org/#/c/32336/1/src/mainboard/google/kukui/chromeos.... 
File src/mainboard/google/kukui/chromeos.c:

https://review.coreboot.org/#/c/32336/1/src/mainboard/google/kukui/chromeos.... 
PS1, Line 36: !
> Looks like it's changed recently by https://review.coreboot. […]

Daisuke, the current format is indeed preferred.  You're correct to point out the recent CL which corrected many discrepancies of WP GPIO and also unified them all to use get_write_protect_state() for the initial value.  We are still in discussion about re-designing this table, or changing policy of which GPIOs should be saved there.  This is because some of the GPIOs never need to be read again (always use the initial value) and some of them don't even need an initial value (they are re-sampled in depthcharge).  If you have any good ideas here, please get in touch.

[Julius Werner (Code Review)]

Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32336 )

Change subject: google/kukui: Get write protection status from WP GPIO
......................................................................

google/kukui: Get write protection status from WP GPIO

Write protection (get_write_protect_state) was hard-coded to 0 and
should be fixed to read from correct GPIO (PERIPHERAL_EN0 from
schematics).

BUG=b:130681408
TEST=make -j; boots on Kukui Rev2.
BRANCH=None

Change-Id: I75b98b1d587abe5e8cdf3df28ea661bc1ffa19f9
Signed-off-by: Hung-Te Lin 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32336
Tested-by: build bot (Jenkins) 
Reviewed-by: You-Cheng Syu 
Reviewed-by: Daisuke Nojiri 
Reviewed-by: Joel Kitching 
---
M src/mainboard/google/kukui/chromeos.c
M src/mainboard/google/kukui/gpio.h
2 files changed, 5 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Daisuke Nojiri: Looks good to me, approved
  Joel Kitching: Looks good to me, approved
  You-Cheng Syu: Looks good to me, but someone else must approve

diff --git a/src/mainboard/google/kukui/chromeos.c b/src/mainboard/google/kukui/chromeos.c
index 09c47ee..44e75e2 100644
--- a/src/mainboard/google/kukui/chromeos.c
+++ b/src/mainboard/google/kukui/chromeos.c
@@ -22,6 +22,7 @@

void setup_chromeos_gpios(void)
 {
+	gpio_input(GPIO_WP);
    gpio_input_pullup(EC_IN_RW);
    gpio_input_pullup(EC_IRQ);
    gpio_input_pullup(CR50_IRQ);
@@ -31,6 +32,8 @@
 void fill_lb_gpios(struct lb_gpios *gpios)
 {
    struct lb_gpio chromeos_gpios[] = {
+		{GPIO_WP.id, ACTIVE_LOW,
+		 !get_write_protect_state(), "write protect"},
    	{-1, ACTIVE_HIGH, get_recovery_mode_switch(), "recovery"},
    	{EC_IN_RW.id, ACTIVE_HIGH, -1, "EC in RW"},
    	{EC_IRQ.id, ACTIVE_LOW, -1, "EC interrupt"},
@@ -41,7 +44,7 @@

int get_write_protect_state(void)
 {
-	return 0;
+	return !gpio_get(GPIO_WP);
 }

int tis_plat_irq_status(void)
diff --git a/src/mainboard/google/kukui/gpio.h b/src/mainboard/google/kukui/gpio.h
index 024b0d7..92d238e 100644
--- a/src/mainboard/google/kukui/gpio.h
+++ b/src/mainboard/google/kukui/gpio.h
@@ -18,6 +18,7 @@

#include <soc/gpio.h>

+#define GPIO_WP		GPIO(PERIPHERAL_EN0)
 #define EC_IRQ		GPIO(PERIPHERAL_EN1)
 #define EC_IN_RW	GPIO(PERIPHERAL_EN14)
 #define CR50_IRQ	GPIO(PERIPHERAL_EN3)