Change in coreboot[master]: acpi/device: Add GPIO binding property for an array of GPIOs
Karthik Ramasubramanian has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46056 )
Change subject: acpi/device: Add GPIO binding property for an array of GPIOs ......................................................................
acpi/device: Add GPIO binding property for an array of GPIOs
This change is required for use-cases like GPIO based I2C multiplexer where more than one GPIOs are used as select lines.
BUG=b:169444894 TEST=Build and boot waddledee to OS. Ensure that the GPIO bindings for an array of GPIOs are added to the ACPI table as follows: Device (MUX0) { ... Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings { GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\_SB.PCI0.GPIO", 0x00, ResourceConsumer, , ) { // Pin list 0x0125 } GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\_SB.PCI0.GPIO", 0x00, ResourceConsumer, , ) { // Pin list 0x0126 } }) Name (_DSD, Package (0x02) // _DSD: Device-Specific Data { ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */, Package (0x01) { Package (0x02) { "mux-gpios", Package (0x08) { _SB.PCI0.I2C3.MUX0, Zero, Zero, Zero, _SB.PCI0.I2C3.MUX0, One, Zero, Zero } } } }) }
Change-Id: I7c6cc36b1bfca2d48c84f169e6b43fd4be8ba330 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/acpi/device.c M src/include/acpi/acpi_device.h 2 files changed, 40 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/46056/1
diff --git a/src/acpi/device.c b/src/acpi/device.c index 450427d..06002c9 100644 --- a/src/acpi/device.c +++ b/src/acpi/device.c @@ -1019,33 +1019,45 @@ return dp_array; }
+struct acpi_dp *acpi_dp_add_gpio_array(struct acpi_dp *dp, const char *name, + const char *ref, int index, int pin, + uint32_t gpio_count, int active_low) +{ + struct acpi_dp *gpio; + uint32_t i; + + if (!dp || !gpio_count) + return NULL; + + gpio = acpi_dp_new_table(name); + if (!gpio) + return NULL; + + for (i = 0; i < gpio_count; i++) { + /* The device that has _CRS containing GpioIO()/GpioInt() */ + acpi_dp_add_reference(gpio, NULL, ref); + + /* Index of the GPIO resource in _CRS starting from zero */ + acpi_dp_add_integer(gpio, NULL, index + i); + + /* Pin in the GPIO resource, typically zero */ + acpi_dp_add_integer(gpio, NULL, pin); + + /* Set if pin is active low */ + acpi_dp_add_integer(gpio, NULL, active_low); + } + acpi_dp_add_array(dp, gpio); + + return gpio; + +} + + struct acpi_dp *acpi_dp_add_gpio(struct acpi_dp *dp, const char *name, const char *ref, int index, int pin, int active_low) { - if (!dp) - return NULL; - - struct acpi_dp *gpio = acpi_dp_new_table(name); - - if (!gpio) - return NULL; - - /* The device that has _CRS containing GpioIO()/GpioInt() */ - acpi_dp_add_reference(gpio, NULL, ref); - - /* Index of the GPIO resource in _CRS starting from zero */ - acpi_dp_add_integer(gpio, NULL, index); - - /* Pin in the GPIO resource, typically zero */ - acpi_dp_add_integer(gpio, NULL, pin); - - /* Set if pin is active low */ - acpi_dp_add_integer(gpio, NULL, active_low); - - acpi_dp_add_array(dp, gpio); - - return gpio; + return acpi_dp_add_gpio_array(dp, name, ref, index, pin, 1, active_low); }
/* diff --git a/src/include/acpi/acpi_device.h b/src/include/acpi/acpi_device.h index be13bd7..6fcdc2d 100644 --- a/src/include/acpi/acpi_device.h +++ b/src/include/acpi/acpi_device.h @@ -545,6 +545,11 @@ const char *ref, int index, int pin, int active_low);
+/* Add a GPIO binding device property for array of GPIOs */ +struct acpi_dp *acpi_dp_add_gpio_array(struct acpi_dp *dp, const char *name, + const char *ref, int index, int pin, + uint32_t gpio_count, int active_low); + /* Add a child table of Device Properties */ struct acpi_dp *acpi_dp_add_child(struct acpi_dp *dp, const char *name, struct acpi_dp *child);
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46056 )
Change subject: acpi/device: Add GPIO binding property for an array of GPIOs ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46056/1/src/include/acpi/acpi_devic... File src/include/acpi/acpi_device.h:
https://review.coreboot.org/c/coreboot/+/46056/1/src/include/acpi/acpi_devic... PS1, Line 550: index Might be good to call this "start_index" and add a comment that it must match the defined GPIOs in the _CRS without gaps.
https://review.coreboot.org/c/coreboot/+/46056/1/src/include/acpi/acpi_devic... PS1, Line 550: pin We do support adding a gpio into _CRS with multiple pins (https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src...) but I'm not sure how best to handle that here since it is probably not the common case. Perhaps also with a comment that this expects one gpio entry per pin? And in that case perhaps pin should not even be a provided argument to the array method since it only works with pin 0.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46056 )
Change subject: acpi/device: Add GPIO binding property for an array of GPIOs ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46056/1/src/include/acpi/acpi_devic... File src/include/acpi/acpi_device.h:
https://review.coreboot.org/c/coreboot/+/46056/1/src/include/acpi/acpi_devic... PS1, Line 550: pin
We do support adding a gpio into _CRS with multiple pins (https://review.coreboot. […]
I guess that would complicate the easy re-use of this array method in the single use method...
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46056 )
Change subject: acpi/device: Add GPIO binding property for an array of GPIOs ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46056/1/src/include/acpi/acpi_devic... File src/include/acpi/acpi_device.h:
https://review.coreboot.org/c/coreboot/+/46056/1/src/include/acpi/acpi_devic... PS1, Line 550: pin
I guess that would complicate the easy re-use of this array method in the single use method...
I was thinking if we should allow the caller to provide details about each GPIO separately since we can potentially have cases in the future where the polarity for all GPIOs is not the same or need to reference a different device. That should help with the pin # as well as allow the caller to have non-contiguous GPIO indices from _CRS.
Basically something like: struct acpi_gpio_property { const char *ref; int index; int pin; int active_low; }
And have this function defined as: struct acpi_dp *acpi_dp_add_gpio_array(struct acpi_dp *dp, const struct acpi_gpio_property *gpio_table, size_t gpio_count) { }
That should also allow the case of single use method to call `acpi_dp_add_gpio_array` by setting the same value as the caller.
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46056
to look at the new patch set (#2).
Change subject: acpi/device: Add GPIO binding property for an array of GPIOs ......................................................................
acpi/device: Add GPIO binding property for an array of GPIOs
This change is required for use-cases like GPIO based I2C multiplexer where more than one GPIOs are used as select lines.
BUG=b:169444894 TEST=Build and boot waddledee to OS. Ensure that the GPIO bindings for an array of GPIOs are added to the ACPI table as follows: Device (MUX0) { ... Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings { GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\_SB.PCI0.GPIO", 0x00, ResourceConsumer, , ) { // Pin list 0x0125 } GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\_SB.PCI0.GPIO", 0x00, ResourceConsumer, , ) { // Pin list 0x0126 } }) Name (_DSD, Package (0x02) // _DSD: Device-Specific Data { ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */, Package (0x01) { Package (0x02) { "mux-gpios", Package (0x08) { _SB.PCI0.I2C3.MUX0, Zero, Zero, Zero, _SB.PCI0.I2C3.MUX0, One, Zero, Zero } } } }) }
Change-Id: I7c6cc36b1bfca2d48c84f169e6b43fd4be8ba330 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/acpi/device.c M src/include/acpi/acpi_device.h 2 files changed, 57 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/46056/2
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46056 )
Change subject: acpi/device: Add GPIO binding property for an array of GPIOs ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46056/1/src/include/acpi/acpi_devic... File src/include/acpi/acpi_device.h:
https://review.coreboot.org/c/coreboot/+/46056/1/src/include/acpi/acpi_devic... PS1, Line 550: index
Might be good to call this "start_index" and add a comment that it must match the defined GPIOs in t […]
Now with the updated function signature, one can pass indices for individual GPIO resource.
https://review.coreboot.org/c/coreboot/+/46056/1/src/include/acpi/acpi_devic... PS1, Line 550: pin
I was thinking if we should allow the caller to provide details about each GPIO separately since we […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46056 )
Change subject: acpi/device: Add GPIO binding property for an array of GPIOs ......................................................................
Patch Set 2: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/46056/2/src/acpi/device.c File src/acpi/device.c:
https://review.coreboot.org/c/coreboot/+/46056/2/src/acpi/device.c@1035 PS2, Line 1035: nit: Probably we should add a comment here indicating what this actually generates?
Package () { name, // e.g. cs-gpios Package() { ref, index, pin, active_low, // GPIO-0 (params[0]) ref, index, pin, active_low, // GPIO-1 (params[1]) ... } }
https://review.coreboot.org/c/coreboot/+/46056/2/src/acpi/device.c@1038 PS2, Line 1038: acpi_dp_add_reference(gpio, NULL, params->ref); Do we want to support the case where params->ref is NULL and so we will have to just write a 0 for that index? Linux kernel supports the possibility of leaving holes in the array of GPIOs as well: https://www.kernel.org/doc/html/latest/firmware-guide/acpi/gpio-properties.h...
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46056
to look at the new patch set (#3).
Change subject: acpi/device: Add GPIO binding property for an array of GPIOs ......................................................................
acpi/device: Add GPIO binding property for an array of GPIOs
This change is required for use-cases like GPIO based I2C multiplexer where more than one GPIOs are used as select lines.
BUG=b:169444894 TEST=Build and boot waddledee to OS. Ensure that the GPIO bindings for an array of GPIOs are added to the ACPI table as follows: Device (MUX0) { ... Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings { GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\_SB.PCI0.GPIO", 0x00, ResourceConsumer, , ) { // Pin list 0x0125 } GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\_SB.PCI0.GPIO", 0x00, ResourceConsumer, , ) { // Pin list 0x0126 } }) Name (_DSD, Package (0x02) // _DSD: Device-Specific Data { ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */, Package (0x01) { Package (0x02) { "mux-gpios", Package (0x08) { _SB.PCI0.I2C3.MUX0, Zero, Zero, Zero, _SB.PCI0.I2C3.MUX0, One, Zero, Zero } } } }) }
Change-Id: I7c6cc36b1bfca2d48c84f169e6b43fd4be8ba330 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/acpi/device.c M src/include/acpi/acpi_device.h 2 files changed, 77 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/46056/3
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46056 )
Change subject: acpi/device: Add GPIO binding property for an array of GPIOs ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46056/2/src/acpi/device.c File src/acpi/device.c:
https://review.coreboot.org/c/coreboot/+/46056/2/src/acpi/device.c@1035 PS2, Line 1035:
nit: Probably we should add a comment here indicating what this actually generates? […]
Done
https://review.coreboot.org/c/coreboot/+/46056/2/src/acpi/device.c@1038 PS2, Line 1038: acpi_dp_add_reference(gpio, NULL, params->ref);
Do we want to support the case where params->ref is NULL and so we will have to just write a 0 for t […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46056 )
Change subject: acpi/device: Add GPIO binding property for an array of GPIOs ......................................................................
Patch Set 3: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46056 )
Change subject: acpi/device: Add GPIO binding property for an array of GPIOs ......................................................................
acpi/device: Add GPIO binding property for an array of GPIOs
This change is required for use-cases like GPIO based I2C multiplexer where more than one GPIOs are used as select lines.
BUG=b:169444894 TEST=Build and boot waddledee to OS. Ensure that the GPIO bindings for an array of GPIOs are added to the ACPI table as follows: Device (MUX0) { ... Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings { GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\_SB.PCI0.GPIO", 0x00, ResourceConsumer, , ) { // Pin list 0x0125 } GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\_SB.PCI0.GPIO", 0x00, ResourceConsumer, , ) { // Pin list 0x0126 } }) Name (_DSD, Package (0x02) // _DSD: Device-Specific Data { ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */, Package (0x01) { Package (0x02) { "mux-gpios", Package (0x08) { _SB.PCI0.I2C3.MUX0, Zero, Zero, Zero, _SB.PCI0.I2C3.MUX0, One, Zero, Zero } } } }) }
Change-Id: I7c6cc36b1bfca2d48c84f169e6b43fd4be8ba330 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46056 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/acpi/device.c M src/include/acpi/acpi_device.h 2 files changed, 77 insertions(+), 22 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/acpi/device.c b/src/acpi/device.c index 450427d..5de31b7 100644 --- a/src/acpi/device.c +++ b/src/acpi/device.c @@ -1019,33 +1019,72 @@ return dp_array; }
+struct acpi_dp *acpi_dp_add_gpio_array(struct acpi_dp *dp, const char *name, + const struct acpi_gpio_res_params *params, + size_t param_count) +{ + struct acpi_dp *gpio; + uint32_t i; + + if (!dp || !param_count) + return NULL; + + gpio = acpi_dp_new_table(name); + if (!gpio) + return NULL; + + /* + * Generate ACPI identifiers as follows: + * Package () { + * name, // e.g. cs-gpios + * Package() { + * ref, index, pin, active_low, // GPIO-0 (params[0]) + * ref, index, pin, active_low, // GPIO-1 (params[1]) + * ... + * } + * } + */ + for (i = 0; i < param_count; i++, params++) { + /* + * If refs is NULL, leave a hole in the gpio array. This can be used in + * conditions where some controllers use both GPIOs and native signals. + */ + if (!params->ref) { + acpi_dp_add_integer(gpio, NULL, 0); + continue; + } + + /* The device that has _CRS containing GpioIO()/GpioInt() */ + acpi_dp_add_reference(gpio, NULL, params->ref); + + /* Index of the GPIO resource in _CRS starting from zero */ + acpi_dp_add_integer(gpio, NULL, params->index); + + /* Pin in the GPIO resource, typically zero */ + acpi_dp_add_integer(gpio, NULL, params->pin); + + /* Set if pin is active low */ + acpi_dp_add_integer(gpio, NULL, params->active_low); + } + acpi_dp_add_array(dp, gpio); + + return gpio; + +} + + struct acpi_dp *acpi_dp_add_gpio(struct acpi_dp *dp, const char *name, const char *ref, int index, int pin, int active_low) { - if (!dp) - return NULL; + struct acpi_gpio_res_params param = { + .ref = ref, + .index = index, + .pin = pin, + .active_low = active_low, + };
- struct acpi_dp *gpio = acpi_dp_new_table(name); - - if (!gpio) - return NULL; - - /* The device that has _CRS containing GpioIO()/GpioInt() */ - acpi_dp_add_reference(gpio, NULL, ref); - - /* Index of the GPIO resource in _CRS starting from zero */ - acpi_dp_add_integer(gpio, NULL, index); - - /* Pin in the GPIO resource, typically zero */ - acpi_dp_add_integer(gpio, NULL, pin); - - /* Set if pin is active low */ - acpi_dp_add_integer(gpio, NULL, active_low); - - acpi_dp_add_array(dp, gpio); - - return gpio; + return acpi_dp_add_gpio_array(dp, name, ¶m, 1); }
/* diff --git a/src/include/acpi/acpi_device.h b/src/include/acpi/acpi_device.h index be13bd7..301f9b0 100644 --- a/src/include/acpi/acpi_device.h +++ b/src/include/acpi/acpi_device.h @@ -545,6 +545,22 @@ const char *ref, int index, int pin, int active_low);
+struct acpi_gpio_res_params { + /* Reference to the parent device. */ + const char *ref; + /* Index to the GpioIo resource within the _CRS. */ + int index; + /* Index to the pin within the GpioIo resource, usually 0. */ + int pin; + /* Flag to indicate if pin is active low. */ + int active_low; +}; + +/* Add a GPIO binding device property for array of GPIOs */ +struct acpi_dp *acpi_dp_add_gpio_array(struct acpi_dp *dp, const char *name, + const struct acpi_gpio_res_params *params, + size_t param_count); + /* Add a child table of Device Properties */ struct acpi_dp *acpi_dp_add_child(struct acpi_dp *dp, const char *name, struct acpi_dp *child);
-
Duncan Laurie (Code Review) -
Furquan Shaikh (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Patrick Georgi (Code Review)