Change in ...coreboot[master]: vendorcode/google/chromeos: Add option for using ACPI GPIO pin

[Duncan Laurie (Code Review)]

Duncan Laurie has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31080

Change subject: vendorcode/google/chromeos: Add option for using ACPI GPIO pin
......................................................................

vendorcode/google/chromeos: Add option for using ACPI GPIO pin

This new option will have the generated Chrome OS ACPI GPIO table
provide the ACPI GPIO pin number instead of the raw GPIO number.

This is necessary if the OS uses a different numbering for GPIOs
that are reported in ACPI than the actual underlying GPIO number.

For example, if the SOC OS driver declares more pins in an ACPI GPIO
bank than there are actual pins in the hardware it will have gaps in
the number space.

This is a reworked version of 6217e9beff16d805ca833e79a2931bcdb3d02a44
which uses a new option instead of just relying on GENERIC_GPIO_LIB.

BUG=b:120686247
TEST=pass firmware_WriteProtect test on Sarien

Change-Id: I3ad5099b7f2f871c7e516988f60a54eb2a75bef7
Signed-off-by: Duncan Laurie 
---
M src/vendorcode/google/chromeos/Kconfig
M src/vendorcode/google/chromeos/acpi.c
2 files changed, 19 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/31080/1

diff --git a/src/vendorcode/google/chromeos/Kconfig b/src/vendorcode/google/chromeos/Kconfig
index 26ee31e..2edb46f 100644
--- a/src/vendorcode/google/chromeos/Kconfig
+++ b/src/vendorcode/google/chromeos/Kconfig
@@ -89,5 +89,17 @@
      on normal boot as well as resume and coreboot is only involved
      in the resume piece w.r.t. the platform hierarchy.

+config CHROMEOS_ACPI_GPIO_GENERATE_PIN
+	bool
+	default n
+	depends on HAVE_ACPI_TABLES && GENERIC_GPIO_LIB
+	help
+	  This option will have the generated Chrome OS ACPI GPIO table
+	  provide the ACPI GPIO pin number instead of the raw GPIO number.
+	  This is necessary if the OS uses a different numbering for GPIOs
+	  that are reported in ACPI.  For example, if the SOC declares more
+	  pins in an ACPI GPIO bank than there are actual pins in the hardware
+	  it will have gaps in the number space.
+
 endif # CHROMEOS
 endmenu
diff --git a/src/vendorcode/google/chromeos/acpi.c b/src/vendorcode/google/chromeos/acpi.c
index 6605809..849f4c3 100644
--- a/src/vendorcode/google/chromeos/acpi.c
+++ b/src/vendorcode/google/chromeos/acpi.c
@@ -15,6 +15,9 @@

#include <arch/acpigen.h>
 #include "chromeos.h"
+#if IS_ENABLED(CONFIG_CHROMEOS_ACPI_GPIO_GENERATE_PIN)
+#include <gpio.h>
+#endif

void chromeos_acpi_gpio_generate(const struct cros_gpio *gpios, size_t num)
 {
@@ -28,7 +31,11 @@
    	acpigen_write_package(4);
    	acpigen_write_integer(gpios[i].type);
    	acpigen_write_integer(gpios[i].polarity);
+#if IS_ENABLED(CONFIG_CHROMEOS_ACPI_GPIO_GENERATE_PIN)
+		acpigen_write_integer(gpio_acpi_pin(gpios[i].gpio_num));
+#else
    	acpigen_write_integer(gpios[i].gpio_num);
+#endif
    	acpigen_write_string(gpios[i].device);
    	acpigen_pop_len();
    }

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31080 )

Change subject: vendorcode/google/chromeos: Add option for using ACPI GPIO pin
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/31080/1/src/vendorcode/google/chromeos/acpi.... 
File src/vendorcode/google/chromeos/acpi.c:

https://review.coreboot.org/#/c/31080/1/src/vendorcode/google/chromeos/acpi.... 
PS1, Line 18: #if IS_ENABLED(CONFIG_CHROMEOS_ACPI_GPIO_GENERATE_PIN)
Would it hurt to always include the header file?

https://review.coreboot.org/#/c/31080/1/src/vendorcode/google/chromeos/acpi.... 
PS1, Line 34: #if IS_ENABLED(CONFIG_CHROMEOS_ACPI_GPIO_GENERATE_PIN)
Any reason to use pre-processor code instead of C?

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31080 )

Change subject: vendorcode/google/chromeos: Add option for using ACPI GPIO pin
......................................................................

Patch Set 1:

I do understand the requirement to report the mapped
pin number. What I don't understand is, when would we
ever need to have the raw number? i.e. why can't we
make the mapped numbers the default?

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31080 )

Change subject: vendorcode/google/chromeos: Add option for using ACPI GPIO pin
......................................................................

Patch Set 1:

(2 comments)
> Patch Set 1:
>
> I do understand the requirement to report the mapped
> pin number. What I don't understand is, when would we
> ever need to have the raw number? i.e. why can't we
> make the mapped numbers the default?

That was my original plan, use this if CONFIG_GENERIC_GPIO_LIB was defined but that ended up breaking a bunch of random boards that don't have a full GPIO implementation.

https://review.coreboot.org/#/c/31080/1/src/vendorcode/google/chromeos/acpi.... 
File src/vendorcode/google/chromeos/acpi.c:

https://review.coreboot.org/#/c/31080/1/src/vendorcode/google/chromeos/acpi.... 
PS1, Line 18: #if IS_ENABLED(CONFIG_CHROMEOS_ACPI_GPIO_GENERATE_PIN)
> Would it hurt to always include the header file?

gpio.h is poor in this regard, if the SOC doesn't have an implementation it will fail to compile.

https://review.coreboot.org/#/c/31080/1/src/vendorcode/google/chromeos/acpi.... 
PS1, Line 34: #if IS_ENABLED(CONFIG_CHROMEOS_ACPI_GPIO_GENERATE_PIN)
> Any reason to use pre-processor code instead of C?

Same reason as above, if the header is now guarded the code has to be as well.

Change in ...coreboot[master]: vendorcode/google/chromeos: Use ACPI GPIO pin when possible

[Duncan Laurie (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: vendorcode/google/chromeos: Use ACPI GPIO pin when possible
......................................................................

vendorcode/google/chromeos: Use ACPI GPIO pin when possible

Have the generated Chrome OS ACPI GPIO table provide the ACPI GPIO
pin number instead of the raw GPIO number when possible.

This is necessary if the OS uses a different numbering for GPIOs
that are reported in ACPI than the actual underlying GPIO number.

For example, if the SOC OS driver declares more pins in an ACPI GPIO
bank than there are actual pins in the hardware it will have gaps in
the number space.

This is a reworked version of 6217e9beff16d805ca833e79a2931bcdb3d02a44
which does not try to convert CROS_GPIO_VIRTUAL.

BUG=b:120686247
TEST=pass firmware_WriteProtect test on Sarien

Signed-off-by: Duncan Laurie 
Change-Id: I3ad5099b7f2f871c7e516988f60a54eb2a75bef7
---
M src/vendorcode/google/chromeos/acpi.c
1 file changed, 11 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/31080/2

Change in ...coreboot[master]: vendorcode/google/chromeos: Use ACPI GPIO pin when possible

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31080 )

Change subject: vendorcode/google/chromeos: Use ACPI GPIO pin when possible
......................................................................

Patch Set 2:
> Patch Set 1:
>
> (2 comments)
> > Patch Set 1:
> >
> > I do understand the requirement to report the mapped
> > pin number. What I don't understand is, when would we
> > ever need to have the raw number? i.e. why can't we
> > make the mapped numbers the default?
>
> That was my original plan, use this if CONFIG_GENERIC_GPIO_LIB was defined but that ended up breaking a bunch of random boards that don't have a full GPIO implementation.

I realized I had not really considered the easy fix for my previous patch that was reverted.  This seems easier and removes the kconfig option.

Change in ...coreboot[master]: vendorcode/google/chromeos: Use ACPI GPIO pin when possible

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31080 )

Change subject: vendorcode/google/chromeos: Use ACPI GPIO pin when possible
......................................................................

Patch Set 2:

This is a reworked version of the previous patch that caused issues, I tested it on Sarien and Atlas.

Change in ...coreboot[master]: vendorcode/google/chromeos: Use ACPI GPIO pin when possible

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31080 )

Change subject: vendorcode/google/chromeos: Use ACPI GPIO pin when possible
......................................................................

Patch Set 2: Code-Review+2

Change in ...coreboot[master]: vendorcode/google/chromeos: Use ACPI GPIO pin when possible

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31080 )

Change subject: vendorcode/google/chromeos: Use ACPI GPIO pin when possible
......................................................................

Patch Set 2: Code-Review+1

Change in ...coreboot[master]: vendorcode/google/chromeos: Use ACPI GPIO pin when possible

[Duncan Laurie (Code Review)]

Duncan Laurie has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31080 )

Change subject: vendorcode/google/chromeos: Use ACPI GPIO pin when possible
......................................................................

vendorcode/google/chromeos: Use ACPI GPIO pin when possible

Have the generated Chrome OS ACPI GPIO table provide the ACPI GPIO
pin number instead of the raw GPIO number when possible.

This is necessary if the OS uses a different numbering for GPIOs
that are reported in ACPI than the actual underlying GPIO number.

For example, if the SOC OS driver declares more pins in an ACPI GPIO
bank than there are actual pins in the hardware it will have gaps in
the number space.

This is a reworked version of 6217e9beff16d805ca833e79a2931bcdb3d02a44
which does not try to convert CROS_GPIO_VIRTUAL.

BUG=b:120686247
TEST=pass firmware_WriteProtect test on Sarien

Signed-off-by: Duncan Laurie 
Change-Id: I3ad5099b7f2f871c7e516988f60a54eb2a75bef7
Reviewed-on: https://review.coreboot.org/c/31080
Tested-by: build bot (Jenkins) 
Reviewed-by: Subrata Banik 
Reviewed-by: Nico Huber 
---
M src/vendorcode/google/chromeos/acpi.c
1 file changed, 11 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, but someone else must approve
  Subrata Banik: Looks good to me, approved

diff --git a/src/vendorcode/google/chromeos/acpi.c b/src/vendorcode/google/chromeos/acpi.c
index 6605809..8fd47a6 100644
--- a/src/vendorcode/google/chromeos/acpi.c
+++ b/src/vendorcode/google/chromeos/acpi.c
@@ -14,11 +14,15 @@
  */

#include <arch/acpigen.h>
+#if IS_ENABLED(CONFIG_GENERIC_GPIO_LIB)
+#include <gpio.h>
+#endif
 #include "chromeos.h"

void chromeos_acpi_gpio_generate(const struct cros_gpio *gpios, size_t num)
 {
    size_t i;
+	int gpio_num;

acpigen_write_scope("\");
    acpigen_write_name("OIPG");
@@ -28,7 +32,13 @@
    	acpigen_write_package(4);
    	acpigen_write_integer(gpios[i].type);
    	acpigen_write_integer(gpios[i].polarity);
-		acpigen_write_integer(gpios[i].gpio_num);
+		gpio_num = gpios[i].gpio_num;
+#if IS_ENABLED(CONFIG_GENERIC_GPIO_LIB)
+		/* Get ACPI pin from GPIO library if available */
+		if (gpios[i].gpio_num != CROS_GPIO_VIRTUAL)
+			gpio_num = gpio_acpi_pin(gpio_num);
+#endif
+		acpigen_write_integer(gpio_num);
    	acpigen_write_string(gpios[i].device);
    	acpigen_pop_len();
    }