Change in coreboot[master]: drivers/generic: Add new driver for power resource pseudo device
Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35036 )
Change subject: drivers/generic: Add new driver for power resource pseudo device ......................................................................
drivers/generic: Add new driver for power resource pseudo device
There is some duplication of fields dealing with ACPI power resources (see drivers/i2c/generic.c, also used by drivers/i2c/hid, as well as drivers/spi/acpi). This CL creates a new driver for this pseudo-device.
This patch allows a new style of dealing with power resources in the device tree:
device pci 00.0 chip drivers/generic/power_resource register "enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_A1)" device generic 0 on chip drivers/i2c/generic register ... end end end end
and then the power resource driver does most of the work. This should make it easier to support other drivers requiring ACPI power resources in the future (see the diff in the two changed here).
I tested this patch on a device using ACPI power resources, and it was successfully able to invoke the _ON and _OFF routines for the device, both with the new-style and old-style way of creating power resources.
My hope would be to update device trees in later patches to accomodate this new style, and then deprecate the old style.
Change-Id: I358c48f4d4e4c4ef591ecf09ea9e9488c2652c96 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/arch/x86/acpi_device.c M src/arch/x86/acpigen.c M src/arch/x86/include/arch/acpi_device.h M src/arch/x86/include/arch/acpigen.h M src/device/root_device.c M src/drivers/i2c/generic/generic.c M src/drivers/spi/acpi/acpi.c 7 files changed, 122 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/35036/1
diff --git a/src/arch/x86/acpi_device.c b/src/arch/x86/acpi_device.c index 47bcc52..40e9677 100644 --- a/src/arch/x86/acpi_device.c +++ b/src/arch/x86/acpi_device.c @@ -494,20 +494,12 @@ acpi_device_fill_len(desc_length); }
-/* PowerResource() with Enable and/or Reset control */ -void acpi_device_add_power_res(const struct acpi_power_res_params *params) +/* Adds _STA, _ON, and _OFF methods to a power resource, based on config */ +static void acpi_add_power_res_methods(const struct acpi_power_res_params *config) { - static const char *power_res_dev_states[] = { "_PR0", "_PR3" }; - unsigned int reset_gpio = params->reset_gpio->pins[0]; - unsigned int enable_gpio = params->enable_gpio->pins[0]; - unsigned int stop_gpio = params->stop_gpio->pins[0]; - - if (!reset_gpio && !enable_gpio && !stop_gpio) - return; - - /* PowerResource (PRIC, 0, 0) */ - acpigen_write_power_res("PRIC", 0, 0, power_res_dev_states, - ARRAY_SIZE(power_res_dev_states)); + const unsigned int reset_gpio = config->reset_gpio->pins[0]; + const unsigned int enable_gpio = config->enable_gpio->pins[0]; + const unsigned int stop_gpio = config->stop_gpio->pins[0];
/* Method (_STA, 0, NotSerialized) { Return (0x1) } */ acpigen_write_STA(0x1); @@ -515,44 +507,75 @@ /* Method (_ON, 0, Serialized) */ acpigen_write_method_serialized("_ON", 0); if (reset_gpio) - acpigen_enable_tx_gpio(params->reset_gpio); + acpigen_enable_tx_gpio(config->reset_gpio); if (enable_gpio) { - acpigen_enable_tx_gpio(params->enable_gpio); - if (params->enable_delay_ms) - acpigen_write_sleep(params->enable_delay_ms); + acpigen_enable_tx_gpio(config->enable_gpio); + if (config->enable_delay_ms) + acpigen_write_sleep(config->enable_delay_ms); } if (reset_gpio) { - acpigen_disable_tx_gpio(params->reset_gpio); - if (params->reset_delay_ms) - acpigen_write_sleep(params->reset_delay_ms); + acpigen_disable_tx_gpio(config->reset_gpio); + if (config->reset_delay_ms) + acpigen_write_sleep(config->reset_delay_ms); } if (stop_gpio) { - acpigen_disable_tx_gpio(params->stop_gpio); - if (params->stop_delay_ms) - acpigen_write_sleep(params->stop_delay_ms); + acpigen_disable_tx_gpio(config->stop_gpio); + if (config->stop_delay_ms) + acpigen_write_sleep(config->stop_delay_ms); } - acpigen_pop_len(); /* _ON method */ + + acpigen_pop_len(); /* _ON method */
/* Method (_OFF, 0, Serialized) */ acpigen_write_method_serialized("_OFF", 0); if (stop_gpio) { - acpigen_enable_tx_gpio(params->stop_gpio); - if (params->stop_off_delay_ms) - acpigen_write_sleep(params->stop_off_delay_ms); + acpigen_enable_tx_gpio(config->stop_gpio); + if (config->stop_off_delay_ms) + acpigen_write_sleep(config->stop_off_delay_ms); } if (reset_gpio) { - acpigen_enable_tx_gpio(params->reset_gpio); - if (params->reset_off_delay_ms) - acpigen_write_sleep(params->reset_off_delay_ms); + acpigen_enable_tx_gpio(config->reset_gpio); + if (config->reset_off_delay_ms) + acpigen_write_sleep(config->reset_off_delay_ms); } if (enable_gpio) { - acpigen_disable_tx_gpio(params->enable_gpio); - if (params->enable_off_delay_ms) - acpigen_write_sleep(params->enable_off_delay_ms); + acpigen_disable_tx_gpio(config->enable_gpio); + if (config->enable_off_delay_ms) + acpigen_write_sleep(config->enable_off_delay_ms); } - acpigen_pop_len(); /* _OFF method */
- acpigen_pop_len(); /* PowerResource PRIC */ + acpigen_pop_len(); /* _OFF method */ +} + +void acpi_add_power_res(const char *power_resource_name, + const struct acpi_power_res_params *config) +{ + const unsigned int reset_gpio = config->reset_gpio->pins[0]; + const unsigned int enable_gpio = config->enable_gpio->pins[0]; + const unsigned int stop_gpio = config->stop_gpio->pins[0]; + + if (!reset_gpio && !enable_gpio && !stop_gpio) + return; + + /* PowerResource (name, 0, 0) */ + acpigen_write_power_res(power_resource_name, 0, 0); + acpi_add_power_res_methods(config); + acpigen_pop_len(); /* PowerResource */ +} + +void acpi_add_power_states(const char *power_resource_name) +{ + static const char *dev_states[] = { "_PR0", "_PR3" }; + acpigen_write_power_states(power_resource_name, dev_states, 2); +} + +void acpi_add_scoped_power_res(const char *scope, + const char *name, + const struct acpi_power_res_params *config) +{ + acpigen_write_scope(scope); + acpi_add_power_res(name, config); + acpigen_pop_len(); /* Scope */ }
static void acpi_dp_write_array(const struct acpi_dp *array); diff --git a/src/arch/x86/acpigen.c b/src/arch/x86/acpigen.c index f9af102..553c4ff 100644 --- a/src/arch/x86/acpigen.c +++ b/src/arch/x86/acpigen.c @@ -1107,22 +1107,26 @@ acpigen_pop_len(); }
+void acpigen_write_power_states(const char *power_resource_name, + const char *power_states[], + size_t power_states_count) +{ + size_t i; + for (i = 0; i < power_states_count; i++) { + acpigen_write_name(power_states[i]); + acpigen_write_package(1); + acpigen_emit_namestring(power_resource_name); + acpigen_pop_len(); /* Package */ + } +} + /* * Name (_PRx, Package(One) { name }) * ... * PowerResource (name, level, order) */ -void acpigen_write_power_res(const char *name, uint8_t level, uint16_t order, - const char *dev_states[], size_t dev_states_count) +void acpigen_write_power_res(const char *name, uint8_t level, uint16_t order) { - size_t i; - for (i = 0; i < dev_states_count; i++) { - acpigen_write_name(dev_states[i]); - acpigen_write_package(1); - acpigen_emit_simple_namestring(name); - acpigen_pop_len(); /* Package */ - } - acpigen_emit_ext_op(POWER_RES_OP);
acpigen_write_len_f(); diff --git a/src/arch/x86/include/arch/acpi_device.h b/src/arch/x86/include/arch/acpi_device.h index d74af9d..6c7993f 100644 --- a/src/arch/x86/include/arch/acpi_device.h +++ b/src/arch/x86/include/arch/acpi_device.h @@ -403,7 +403,19 @@ * Enable - Enable / disable power to device. * Stop - Stop / start operation of device. */ -void acpi_device_add_power_res(const struct acpi_power_res_params *params); +void acpi_add_power_res(const char *power_resource_name, + const struct acpi_power_res_params *config); + +/* Adds a PowerResource (as above), but in its own scope */ +void acpi_add_scoped_power_res(const char *scope, + const char *name, + const struct acpi_power_res_params *config); + +/* + * Write out _PR0 and _PR3 methods for a device, using power_resource_name + * as the PowerResource to reference +*/ +void acpi_add_power_states(const char *power_resource_name);
/* * Writing Device Properties objects via _DSD diff --git a/src/arch/x86/include/arch/acpigen.h b/src/arch/x86/include/arch/acpigen.h index db003fe..242b1e1 100644 --- a/src/arch/x86/include/arch/acpigen.h +++ b/src/arch/x86/include/arch/acpigen.h @@ -359,8 +359,10 @@ void acpigen_write_mainboard_resources(const char *scope, const char *name); void acpigen_write_irq(u16 mask); void acpigen_write_uuid(const char *uuid); -void acpigen_write_power_res(const char *name, uint8_t level, uint16_t order, - const char *dev_states[], size_t dev_states_count); +void acpigen_write_power_res(const char *name, uint8_t level, uint16_t order); +void acpigen_write_power_states(const char *power_resource_name, + const char *power_states[], + size_t power_states_count); void acpigen_write_sleep(uint64_t sleep_ms); void acpigen_write_store(void); void acpigen_write_store_ops(uint8_t src, uint8_t dst); diff --git a/src/device/root_device.c b/src/device/root_device.c index f8e2907..ec24ff5 100644 --- a/src/device/root_device.c +++ b/src/device/root_device.c @@ -112,6 +112,13 @@
printk(BIOS_DEBUG, "%s %s\n", dev_path(child), child->enabled ? "enabled" : "disabled"); + + /* + * To handle cases of pseudo-devices containing a child + * device when it's not explicitly a bus, e.g., power + * resources, we recurse into them. + */ + scan_generic_bus(child); } }
diff --git a/src/drivers/i2c/generic/generic.c b/src/drivers/i2c/generic/generic.c index 9b4f00d..308f200 100644 --- a/src/drivers/i2c/generic/generic.c +++ b/src/drivers/i2c/generic/generic.c @@ -23,6 +23,7 @@ #include <stdint.h> #include <string.h> #include "chip.h" +#include "drivers/generic/power_resource/chip.h"
#if CONFIG(HAVE_ACPI_TABLES)
@@ -59,7 +60,13 @@ void (*callback)(struct device *dev), struct drivers_i2c_generic_config *config) { - const char *scope = acpi_device_scope(dev); + const int parent_is_power_resource = + (dev->bus && dev->bus->children == dev && + dev->bus->dev->chip_ops == &drivers_generic_power_resource_ops); + const char *pr_scope = parent_is_power_resource ? + acpi_device_scope(dev->bus->dev) : + NULL; + const char *scope = (pr_scope == NULL) ? acpi_device_scope(dev) : pr_scope; struct acpi_i2c i2c = { .address = dev->path.i2c.device, .mode_10bit = dev->path.i2c.mode_10bit, @@ -157,7 +164,11 @@ config->stop_delay_ms, config->stop_off_delay_ms }; - acpi_device_add_power_res(&power_res_params); + acpi_add_power_res("PRIC", &power_res_params); + acpi_add_power_states("PRIC"); + } else if (parent_is_power_resource) { + /* Parent device is a power resource, so set up _PR0/_PR3 */ + acpi_add_power_states(acpi_device_path(dev->bus->dev)); }
/* Callback if any. */ diff --git a/src/drivers/spi/acpi/acpi.c b/src/drivers/spi/acpi/acpi.c index 7c8a887..f7cb2e1 100644 --- a/src/drivers/spi/acpi/acpi.c +++ b/src/drivers/spi/acpi/acpi.c @@ -23,6 +23,7 @@ #include <stdint.h> #include <string.h> #include "chip.h" +#include "drivers/generic/power_resource/chip.h"
static int spi_acpi_get_bus(const struct device *dev) { @@ -73,13 +74,19 @@
static void spi_acpi_fill_ssdt_generator(struct device *dev) { + const int parent_is_power_resource = + (dev->bus && dev->bus->children == dev && + dev->bus->dev->chip_ops == &drivers_generic_power_resource_ops); + const char *pr_scope = parent_is_power_resource ? + acpi_device_scope(dev->bus->dev) : + NULL; struct drivers_spi_acpi_config *config = dev->chip_info; const char *scope = acpi_device_scope(dev); const char *path = acpi_device_path(dev); struct acpi_spi spi = { .device_select = dev->path.spi.cs, .speed = config->speed ? : 1 * MHz, - .resource = scope, + .resource = (pr_scope != NULL) ? pr_scope : scope, .device_select_polarity = SPI_POLARITY_LOW, .wire_mode = SPI_4_WIRE_MODE, .data_bit_length = 8, @@ -178,7 +185,11 @@ config->stop_delay_ms, config->stop_off_delay_ms }; - acpi_device_add_power_res(&power_res_params); + acpi_add_power_res("PRIC", &power_res_params); + acpi_add_power_states("PRIC"); + } else if (parent_is_power_resource) { + /* Parent device is a power resource, so set up _PR0/_PR3 */ + acpi_add_power_states(acpi_device_path(dev->bus->dev)); }
acpigen_pop_len(); /* Device */
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35036 )
Change subject: drivers/generic: Add new driver for power resource pseudo device ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35036/1/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/c/coreboot/+/35036/1/src/arch/x86/acpi_device.c@... PS1, Line 568: static const char *dev_states[] = { "_PR0", "_PR3" }; static const char * array should probably be static const char * const
Hello Patrick Rudolph, Julius Werner, Subrata Banik, Paul Menzel, Nico Huber, Furquan Shaikh, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35036
to look at the new patch set (#2).
Change subject: drivers/generic: Add new driver for power resource pseudo device ......................................................................
drivers/generic: Add new driver for power resource pseudo device
There is some duplication of fields dealing with ACPI power resources (see drivers/i2c/generic.c, also used by drivers/i2c/hid, as well as drivers/spi/acpi). This CL creates a new driver for this pseudo-device.
This patch allows a new style of dealing with power resources in the device tree:
device pci 00.0 chip drivers/generic/power_resource register "enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_A1)" device generic 0 on chip drivers/i2c/generic register ... end end end end
and then the power resource driver does most of the work. This should make it easier to support other drivers requiring ACPI power resources in the future (see the diff in the two changed here).
I tested this patch on a device using ACPI power resources, and it was successfully able to invoke the _ON and _OFF routines for the device, both with the new-style and old-style way of creating power resources.
My hope would be to update device trees in later patches to accomodate this new style, and then deprecate the old style.
Change-Id: I358c48f4d4e4c4ef591ecf09ea9e9488c2652c96 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/arch/x86/acpi_device.c M src/arch/x86/acpigen.c M src/arch/x86/include/arch/acpi_device.h M src/arch/x86/include/arch/acpigen.h M src/device/root_device.c M src/drivers/i2c/generic/generic.c M src/drivers/spi/acpi/acpi.c 7 files changed, 122 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/35036/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35036 )
Change subject: drivers/generic: Add new driver for power resource pseudo device ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35036/1/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/c/coreboot/+/35036/1/src/arch/x86/acpi_device.c@... PS1, Line 568: static const char *dev_states[] = { "_PR0", "_PR3" };
static const char * array should probably be static const char * const
Ack
Hello Patrick Rudolph, Julius Werner, Subrata Banik, Paul Menzel, build bot (Jenkins), Nico Huber, Furquan Shaikh, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35036
to look at the new patch set (#3).
Change subject: drivers/generic: Add new driver for power resource pseudo device ......................................................................
drivers/generic: Add new driver for power resource pseudo device
There is some duplication of fields dealing with ACPI power resources (see drivers/i2c/generic.c, also used by drivers/i2c/hid, as well as drivers/spi/acpi). This CL creates a new driver for this pseudo-device.
This patch allows a new style of dealing with power resources in the device tree:
device pci 00.0 chip drivers/generic/power_resource register "enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_A1)" device generic 0 on chip drivers/i2c/generic register ... end end end end
and then the power resource driver does most of the work. This should make it easier to support other drivers requiring ACPI power resources in the future (see the diff in the two changed here).
I tested this patch on a device using ACPI power resources, and it was successfully able to invoke the _ON and _OFF routines for the device, both with the new-style and old-style way of creating power resources.
My hope would be to update device trees in later patches to accomodate this new style, and then deprecate the old style.
Change-Id: I358c48f4d4e4c4ef591ecf09ea9e9488c2652c96 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/arch/x86/acpi_device.c M src/arch/x86/acpigen.c M src/arch/x86/include/arch/acpi_device.h M src/arch/x86/include/arch/acpigen.h M src/device/root_device.c A src/drivers/generic/power_resource/Makefile.inc A src/drivers/generic/power_resource/chip.h A src/drivers/generic/power_resource/power_resource.c M src/drivers/i2c/generic/generic.c M src/drivers/spi/acpi/acpi.c 10 files changed, 243 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/35036/3
Hello Patrick Rudolph, Julius Werner, Subrata Banik, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi, Furquan Shaikh, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35036
to look at the new patch set (#4).
Change subject: drivers/generic: Add new driver for power resource pseudo device ......................................................................
drivers/generic: Add new driver for power resource pseudo device
There is some duplication of fields dealing with ACPI power resources (see drivers/i2c/generic.c, also used by drivers/i2c/hid, as well as drivers/spi/acpi). This CL creates a new driver for this pseudo-device.
This patch allows a new style of dealing with power resources in the device tree:
device pci 00.0 chip drivers/generic/power_resource register "enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_A1)" device generic 0 on chip drivers/i2c/generic register ... end end end end
and then the power resource driver does most of the work. This should make it easier to support other drivers requiring ACPI power resources in the future (see the diff in the two changed here).
I tested this patch on a device using ACPI power resources, and it was successfully able to invoke the _ON and _OFF routines for the device, both with the new-style and old-style way of creating power resources.
My hope would be to update device trees in later patches to accomodate this new style, and then deprecate the old style.
Change-Id: I358c48f4d4e4c4ef591ecf09ea9e9488c2652c96 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/arch/x86/acpi_device.c M src/arch/x86/acpigen.c M src/arch/x86/include/arch/acpi_device.h M src/arch/x86/include/arch/acpigen.h M src/device/root_device.c A src/drivers/generic/power_resource/Makefile.inc A src/drivers/generic/power_resource/chip.h A src/drivers/generic/power_resource/power_resource.c M src/drivers/i2c/generic/generic.c M src/drivers/spi/acpi/acpi.c 10 files changed, 242 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/35036/4
Hello Patrick Rudolph, Julius Werner, Subrata Banik, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi, Furquan Shaikh, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35036
to look at the new patch set (#5).
Change subject: drivers/generic: Add new driver for power resource pseudo device ......................................................................
drivers/generic: Add new driver for power resource pseudo device
There is some duplication of fields dealing with ACPI power resources (see drivers/i2c/generic.c, also used by drivers/i2c/hid, as well as drivers/spi/acpi). This CL creates a new driver for this pseudo-device.
This patch allows a new style of dealing with power resources in the device tree:
device pci 00.0 chip drivers/generic/power_resource register "enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_A1)" device generic 0 on chip drivers/i2c/generic register ... end end end end
and then the power resource driver does most of the work. This should make it easier to support other drivers requiring ACPI power resources in the future (see the diff in the two changed here).
I tested this patch on a device using ACPI power resources, and it was successfully able to invoke the _ON and _OFF routines for the device, both with the new-style and old-style way of creating power resources.
My hope would be to update device trees in later patches to accomodate this new style, and then deprecate the old style.
Change-Id: I358c48f4d4e4c4ef591ecf09ea9e9488c2652c96 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/arch/x86/acpi_device.c M src/arch/x86/acpigen.c M src/arch/x86/include/arch/acpi_device.h M src/arch/x86/include/arch/acpigen.h M src/device/root_device.c A src/drivers/generic/power_resource/Makefile.inc A src/drivers/generic/power_resource/chip.h A src/drivers/generic/power_resource/power_resource.c M src/drivers/i2c/generic/generic.c M src/drivers/spi/acpi/acpi.c 10 files changed, 242 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/35036/5
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35036 )
Change subject: drivers/generic: Add new driver for power resource pseudo device ......................................................................
Patch Set 7:
I am still looking through the changes. But at minimum, I think we should split this CL into 3-4 separate changes: 1. Refactoring of acpi_device_add_power_res(). Addition of acpi_add_power_states(), acpi_add_scoped_power_res(), acpigen_write_power_states() and update the callers. 2. Change for scan_generic_bus() 3. Addition of new driver for power resource 4. Update current users of acpi_device_add_power_res() to use the new driver.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35036 )
Change subject: drivers/generic: Add new driver for power resource pseudo device ......................................................................
Patch Set 7:
Patch Set 7:
I am still looking through the changes. But at minimum, I think we should split this CL into 3-4 separate changes:
- Refactoring of acpi_device_add_power_res(). Addition of acpi_add_power_states(), acpi_add_scoped_power_res(), acpigen_write_power_states() and update the callers.
- Change for scan_generic_bus()
- Addition of new driver for power resource
- Update current users of acpi_device_add_power_res() to use the new driver.
That sounds like a fine idea. I was debating how to split it
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35036 )
Change subject: drivers/generic: Add new driver for power resource pseudo device ......................................................................
Patch Set 7:
(19 comments)
https://review.coreboot.org/c/coreboot/+/35036/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35036/7//COMMIT_MSG@17 PS7, Line 17: chip drivers/generic/power_resource : register "enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_A1)" : device generic 0 on : chip drivers/i2c/generic : register ... : end : end : end How do you envision handling cases like:
1. Shared power resource: Would both the devices be children of the same power resource? What if they live on completely different pci buses? i.e. devices on pci 14.1 and pci 14.2 sharing the same power resource. How would that be handled?
2. Additionally, in case of shared power resources, would the callers require two separate power resources if only enable line is shared but not the reset line? What would be the hierarchy of devices in that case?
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/acpi_device.c@... PS7, Line 550: void Just wondering if this should return a value indicating if the power resource was added? With this change to split addition of power resource and power states, does it help the caller to know if power resource addition was successful?
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/acpi_device.c@... PS7, Line 569: 2 ARRAY_SIZE(dev_states)
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/acpi_device.c@... PS7, Line 573: const char *name, This might fit on the above line within the character count limit?
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpi_device.h:
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... PS7, Line 411: const char *name, This will fit on the previous line.
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... PS7, Line 418: acpi_add_power_states Technically, this is adding power resource for a device as per the definition of _PR0 and _PR3. What do you think about renaming:
acpi_add_power_res --> acpi_declare_power_res acpi_add_scoped_power_res --> acpi_declare_scoped_power_res acpi_add_power_states --> acpi_add_device_power_res
Power state is a very different thing as per ACPI Spec -- _PS0, _PS1, ... _PSC, etc.
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpigen.h:
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... PS7, Line 362: acpigen_write_power_res ... and then these would have to be:
acpigen_write_declare_power_res acpigen_write_add_power_res
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... PS7, Line 363: power_resource_name This can just be name?
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... PS7, Line 364: power_states device_res
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... File src/drivers/generic/power_resource/Kconfig:
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 1: DRIVERS_GENERIC_POWER_RESOURCE I think this should ultimately be auto-selected by i2c and spi drivers once you move all boards to this new structure.
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 3: depends on HAVE_ACPI_TABLES
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... File src/drivers/generic/power_resource/chip.h:
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 22: extern struct chip_operations drivers_generic_power_resource_ops; I don't think this is required.
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 47: void power_resource_fill_ssdt(struct device *dev); Why is this exposed outside this driver?
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... File src/drivers/generic/power_resource/power_resource.c:
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 18: #include <console/console.h> Is this required?
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 20: string Is string.h intentionally placed after chip.h?
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 22: void static
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/i2c/generic/gen... File src/drivers/i2c/generic/generic.c:
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/i2c/generic/gen... PS7, Line 63: #if CONFIG(DRIVERS_GENERIC_POWER_RESOURCE) Any reason not to use if (CONFIG(DRIVERS_GENERIC_POWER_RESOURCE)) instead of #if?
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/i2c/generic/gen... PS7, Line 65: dev->bus->children == dev This is going to be tricky. The assumption here is that the first child of the power resource is always going to be this device. If the device has siblings, the siblings can not use the same power resource and developers have to be careful about always putting this device as the first child of the power resource. I feel that could be error prone.
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/i2c/generic/gen... PS7, Line 73: scope Is this being put in because the way acpi_device_scope() works, it will add power resource device within the scope which is not what we want for the device?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35036 )
Change subject: drivers/generic: Add new driver for power resource pseudo device ......................................................................
Patch Set 7:
(19 comments)
https://review.coreboot.org/c/coreboot/+/35036/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35036/7//COMMIT_MSG@17 PS7, Line 17: chip drivers/generic/power_resource : register "enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_A1)" : device generic 0 on : chip drivers/i2c/generic : register ... : end : end : end
How do you envision handling cases like: […]
1) Involves more work, which I have planned for future patches. Right now, they would have to be separate power resources. 2) What comes to mind for me would be to create separate "virtual" power resources, one for each GPIO (enable, reset, stop), link them all in _PR0/_PR3, and use the 'Level' parameter to dictate the order they execute in.
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/acpi_device.c@... PS7, Line 550: void
Just wondering if this should return a value indicating if the power resource was added? With this c […]
Good idea, can return a bool.
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/acpi_device.c@... PS7, Line 569: 2
ARRAY_SIZE(dev_states)
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/acpi_device.c@... PS7, Line 573: const char *name,
This might fit on the above line within the character count limit?
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpi_device.h:
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... PS7, Line 411: const char *name,
This will fit on the previous line.
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... PS7, Line 418: acpi_add_power_states
Technically, this is adding power resource for a device as per the definition of _PR0 and _PR3. […]
That's a very fair point, I like your suggestion.
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... File src/arch/x86/include/arch/acpigen.h:
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... PS7, Line 362: acpigen_write_power_res
... and then these would have to be: […]
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... PS7, Line 363: power_resource_name
This can just be name?
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/arch/x86/include/arch/a... PS7, Line 364: power_states
device_res
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... File src/drivers/generic/power_resource/Kconfig:
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 1: DRIVERS_GENERIC_POWER_RESOURCE
I think this should ultimately be auto-selected by i2c and spi drivers once you move all boards to t […]
Ack
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 3:
depends on HAVE_ACPI_TABLES
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... File src/drivers/generic/power_resource/chip.h:
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 22: extern struct chip_operations drivers_generic_power_resource_ops;
I don't think this is required.
Either that or extern it in i2c/generic.c and spi/acpi.c to know if the parent device is a power resource or not
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 47: void power_resource_fill_ssdt(struct device *dev);
Why is this exposed outside this driver?
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... File src/drivers/generic/power_resource/power_resource.c:
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 18: #include <console/console.h>
Is this required?
It is now with a print for failing to add the power resource.
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 20: string
Is string.h intentionally placed after chip. […]
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/generic/power_r... PS7, Line 22: void
static
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/i2c/generic/gen... File src/drivers/i2c/generic/generic.c:
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/i2c/generic/gen... PS7, Line 63: #if CONFIG(DRIVERS_GENERIC_POWER_RESOURCE)
Any reason not to use if (CONFIG(DRIVERS_GENERIC_POWER_RESOURCE)) instead of #if?
Done
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/i2c/generic/gen... PS7, Line 65: dev->bus->children == dev
This is going to be tricky. […]
That's a good point, it seems now like this check is not really doing anything; really just want to check that the dev->bus->dev->chip_ops is pointing to the correct object; it can be anywhere in the list of child devices.
https://review.coreboot.org/c/coreboot/+/35036/7/src/drivers/i2c/generic/gen... PS7, Line 73: scope
Is this being put in because the way acpi_device_scope() works, it will add power resource device wi […]
Yes. I can add a comment; otherwise acpigen_add_i2c() will use the incorrect resource and the kernel will not be able to locate the I2C controller.
Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Subrata Banik, Duncan Laurie, Paul Menzel, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35036
to look at the new patch set (#8).
Change subject: arch/x86/acpi: Refactor power resource-related functionality, to enable disparate power resource to be cleaned up. Most notably, a power resource can be declared independently of a parent device. ......................................................................
arch/x86/acpi: Refactor power resource-related functionality, to enable disparate power resource to be cleaned up. Most notably, a power resource can be declared independently of a parent device.
Change-Id: I358c48f4d4e4c4ef591ecf09ea9e9488c2652c96 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/arch/x86/acpi_device.c M src/arch/x86/acpigen.c M src/arch/x86/include/arch/acpi_device.h M src/arch/x86/include/arch/acpigen.h M src/drivers/i2c/generic/generic.c M src/drivers/spi/acpi/acpi.c 6 files changed, 110 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/35036/8
Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Subrata Banik, Duncan Laurie, Paul Menzel, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35036
to look at the new patch set (#9).
Change subject: arch/x86/acpi: Refactor power resource-related functionality ......................................................................
arch/x86/acpi: Refactor power resource-related functionality
This enables disparate power resource to be cleaned up. Most notably, a power resource can be declared independently of a parent device.
Change-Id: I358c48f4d4e4c4ef591ecf09ea9e9488c2652c96 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/arch/x86/acpi_device.c M src/arch/x86/acpigen.c M src/arch/x86/include/arch/acpi_device.h M src/arch/x86/include/arch/acpigen.h M src/drivers/i2c/generic/generic.c M src/drivers/spi/acpi/acpi.c 6 files changed, 110 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/35036/9
Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Subrata Banik, Duncan Laurie, Paul Menzel, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35036
to look at the new patch set (#11).
Change subject: arch/x86/acpi: Refactor power resource-related functionality ......................................................................
arch/x86/acpi: Refactor power resource-related functionality
In some boards that use power resources, two devices might share an enable or reset GPIO. This can have a negative effect on boot time if the kernel must power sequence both devices (say one has a much longer reset_enable_delay time than the other), or it could even violate the power sequencing of one or both devices. This patch helps move toward a paradigm of power resources that are separated from the devices that they power sequence. Eventually the goal would be to move to shared power resources in the case that a power signal is shared.
Change-Id: I358c48f4d4e4c4ef591ecf09ea9e9488c2652c96 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/arch/x86/acpi_device.c M src/arch/x86/acpigen.c M src/arch/x86/include/arch/acpi_device.h M src/arch/x86/include/arch/acpigen.h M src/drivers/i2c/generic/generic.c M src/drivers/spi/acpi/acpi.c 6 files changed, 110 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/35036/11
Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Subrata Banik, Duncan Laurie, Paul Menzel, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35036
to look at the new patch set (#12).
Change subject: arch/x86/acpi: Refactor power resource-related functionality ......................................................................
arch/x86/acpi: Refactor power resource-related functionality
In some boards that use power resources, two devices might share an enable or reset GPIO. This can have a negative effect on boot time if the kernel must power sequence both devices (say one has a much longer reset_enable_delay time than the other), or it could even violate the power sequencing of one or both devices. This patch helps move toward a paradigm of power resources that are separated from the devices that they power sequence. Eventually the goal would be to move to shared power resources in the case that a power signal is shared.
Change-Id: I358c48f4d4e4c4ef591ecf09ea9e9488c2652c96 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/arch/x86/acpi_device.c M src/arch/x86/acpigen.c M src/arch/x86/include/arch/acpi_device.h M src/arch/x86/include/arch/acpigen.h M src/drivers/i2c/generic/generic.c M src/drivers/spi/acpi/acpi.c 6 files changed, 110 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/35036/12
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35036 )
Change subject: arch/x86/acpi: Refactor power resource-related functionality ......................................................................
Patch Set 13: Code-Review+1
(8 comments)
Mostly nits. Overall LGTM.
https://review.coreboot.org/c/coreboot/+/35036/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35036/13//COMMIT_MSG@10 PS13, Line 10: reset GPIO Wouldn't it be wrong to have a design where two devices share only the reset GPIO? The reason that enable GPIO sharing is okay is because that is the first thing in power sequencing. So, power resources for one device can be enabled independently even with virtual resources in place. However, if reset GPIO is shared, you would always violate the power sequencing since one device will be enabled before the other.
https://review.coreboot.org/c/coreboot/+/35036/13//COMMIT_MSG@11 PS13, Line 11: say one has a much longer : reset_enable_delay time than the other Maybe I am nitpicking, but the boot time impact would be the same irrespective of whether the power resources are separate or shared. Kernel doesn't have the construct of saying we have already spent T1 amount of time from enable->reset for device 1, so we just need to spend T2 - T1 before deasserting reset for device 2.
https://review.coreboot.org/c/coreboot/+/35036/13//COMMIT_MSG@13 PS13, Line 13: helps How? The commit message talks about the motivation and the goal for doing this change, but not what the change really is.
https://review.coreboot.org/c/coreboot/+/35036/13//COMMIT_MSG@17 PS13, Line 17: It would be good to have a BUG to track the work you are doing.
https://review.coreboot.org/c/coreboot/+/35036/13/src/arch/x86/acpigen.c File src/arch/x86/acpigen.c:
https://review.coreboot.org/c/coreboot/+/35036/13/src/arch/x86/acpigen.c@110... PS13, Line 1109: Can you please add a comment indicating what this is actually writing in ASL?
https://review.coreboot.org/c/coreboot/+/35036/13/src/arch/x86/include/arch/... File src/arch/x86/include/arch/acpi_device.h:
https://review.coreboot.org/c/coreboot/+/35036/13/src/arch/x86/include/arch/... PS13, Line 399: Add Declare
https://review.coreboot.org/c/coreboot/+/35036/13/src/arch/x86/include/arch/... PS13, Line 407: add nit: declare
https://review.coreboot.org/c/coreboot/+/35036/13/src/arch/x86/include/arch/... PS13, Line 412: Adds nit: Declares
Tim Wawrzynczak has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35036 )
Change subject: arch/x86/acpi: Refactor power resource-related functionality ......................................................................
Abandoned
needs more thought
-
build bot (Jenkins) (Code Review) -
Furquan Shaikh (Code Review)
-
Tim Wawrzynczak (Code Review)