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/+/35073 )
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).
Change-Id: Id9d66fedd342eb5359d90d9778c515e7aa2de1d7 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- A src/drivers/generic/power_resource/Kconfig 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 4 files changed, 128 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/35073/1
diff --git a/src/drivers/generic/power_resource/Kconfig b/src/drivers/generic/power_resource/Kconfig new file mode 100644 index 0000000..447edb9 --- /dev/null +++ b/src/drivers/generic/power_resource/Kconfig @@ -0,0 +1,5 @@ +# TODO: once everything is moved to the new power resource structure, +# those drivers that use it should auto-select this config option +config DRIVERS_GENERIC_POWER_RESOURCE + bool + depends on HAVE_ACPI_TABLES diff --git a/src/drivers/generic/power_resource/Makefile.inc b/src/drivers/generic/power_resource/Makefile.inc new file mode 100644 index 0000000..2120ef1 --- /dev/null +++ b/src/drivers/generic/power_resource/Makefile.inc @@ -0,0 +1 @@ +ramstage-$(CONFIG_DRIVERS_GENERIC_POWER_RESOURCE) += power_resource.c diff --git a/src/drivers/generic/power_resource/chip.h b/src/drivers/generic/power_resource/chip.h new file mode 100644 index 0000000..e698296 --- /dev/null +++ b/src/drivers/generic/power_resource/chip.h @@ -0,0 +1,47 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google LLC. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef __GENERIC_POWER_RESOURCE_CHIP_H__ +#define __GENERIC_POWER_RESOURCE_CHIP_H__ + +#include <device/device.h> +#include <arch/acpi_device.h> + +extern struct chip_operations drivers_generic_power_resource_ops; + +struct drivers_generic_power_resource_config { + /* GPIO used to take device out of reset or to put it into reset. */ + struct acpi_gpio reset_gpio; + /* Delay to be inserted after device is taken out of reset. */ + unsigned int reset_delay_ms; + /* Delay to be inserted after device is put into reset. */ + unsigned int reset_off_delay_ms; + /* GPIO used to enable device. */ + struct acpi_gpio enable_gpio; + /* Delay to be inserted after device is enabled. */ + unsigned int enable_delay_ms; + /* Delay to be inserted after device is disabled. */ + unsigned int enable_off_delay_ms; + /* GPIO used to stop operation of device. */ + struct acpi_gpio stop_gpio; + /* Delay to be inserted after disabling stop. */ + unsigned int stop_delay_ms; + /* Delay to be inserted after enabling stop. */ + unsigned int stop_off_delay_ms; + /* ACPI device name */ + char acpi_name[5]; +}; + +#endif /* __GENERIC_POWER_RESOURCE_CHIP_H__ */ diff --git a/src/drivers/generic/power_resource/power_resource.c b/src/drivers/generic/power_resource/power_resource.c new file mode 100644 index 0000000..d08c1a2 --- /dev/null +++ b/src/drivers/generic/power_resource/power_resource.c @@ -0,0 +1,75 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google LLC. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <device/device.h> +#include <device/path.h> +#include <console/console.h> +#include <string.h> +#include "chip.h" + +static void power_resource_fill_ssdt(struct device *dev) +{ + bool ret; + struct drivers_generic_power_resource_config *config = dev->chip_info; + const struct acpi_power_res_params power_res_params = { + &config->reset_gpio, + config->reset_delay_ms, + config->reset_off_delay_ms, + &config->enable_gpio, + config->enable_delay_ms, + config->enable_off_delay_ms, + &config->stop_gpio, + config->stop_delay_ms, + config->stop_off_delay_ms + }; + + ret = acpi_declare_scoped_power_resource(acpi_device_scope(dev), + acpi_device_name(dev), + &power_res_params); + if (!ret) + printk(BIOS_ERR, "Failed to add power resource %s\n", + acpi_device_name(dev)); + +} + +static const char *power_resource_acpi_name(const struct device *dev) +{ + static unsigned int count; + struct drivers_generic_power_resource_config *config = dev->chip_info; + + if (config->acpi_name[0] == '\0') + snprintf(config->acpi_name, sizeof(config->acpi_name), "PR%02X", count++); + + return config->acpi_name; +} + +static struct device_operations power_resource_ops = { + .read_resources = DEVICE_NOOP, + .set_resources = DEVICE_NOOP, + .enable_resources = DEVICE_NOOP, + .scan_bus = scan_generic_bus, + .acpi_name = power_resource_acpi_name, + .acpi_fill_ssdt_generator = power_resource_fill_ssdt +}; + +static void power_resource_enable(struct device *dev) +{ + dev->ops = &power_resource_ops; +} + +struct chip_operations drivers_generic_power_resource_ops = { + CHIP_NAME("Power Resource") + .enable_dev = power_resource_enable +};
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/+/35073
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. It will also enable the sharing of power resources between multiple devices in the future, if they share power-related signals.
This would change the (effective) ASL from something like this:
Scope (_SB.PCI0) { Device (I2C0) { ... PowerResource (PRIC, 0, 0) { Method (_ON) ... Method (_OFF) ... } Name (_PR0, Package (1) { PRIC } ) Name (_PR3, Package (1) { PRIC } ) } }
to something more like this:
Scope (_SB.PCI0) { PowerResource (PR00, 0, 0) { Method (_ON)... Method (_OFF)... }
Device (I2C0) { ... Name (_PR0, Package (1) { PR00 } ) Name (_PR3, Package (1) { PR00 } ) } }
Change-Id: Id9d66fedd342eb5359d90d9778c515e7aa2de1d7 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- A src/drivers/generic/power_resource/Kconfig 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 4 files changed, 128 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/35073/4
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35073 )
Change subject: drivers/generic: Add new driver for power resource pseudo-device ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35073/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35073/4//COMMIT_MSG@36 PS4, Line 36: PowerResource (PR00, 0, 0) The semantics of this seem odd. This isn't a resource for the PCI0 device. It's a resource of the i2c peripheral.
This makes the most sense to me semantically:
_SB.PCI0 -> I2C0 controller -> i2c wrapper device with parent -> { i2c_periph1, i2c_periph2 }
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35073 )
Change subject: drivers/generic: Add new driver for power resource pseudo-device ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35073/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35073/4//COMMIT_MSG@36 PS4, Line 36: PowerResource (PR00, 0, 0)
The semantics of this seem odd. This isn't a resource for the PCI0 device. […]
Oops, that's a typo. It's actually created under the I2C0 scope in this case: Scope (_SB.PCI0.I2C0) { PowerResource (PR00, 0, 0) { ... } }
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35073 )
Change subject: drivers/generic: Add new driver for power resource pseudo-device ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35073/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35073/4//COMMIT_MSG@36 PS4, Line 36: PowerResource (PR00, 0, 0)
Oops, that's a typo. It's actually created under the I2C0 scope in this case: […]
Even so, it seems weird to attach a power resource to the i2c controller when it should be associated with the peripheral (or wrapper) device.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35073 )
Change subject: drivers/generic: Add new driver for power resource pseudo-device ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35073/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35073/4//COMMIT_MSG@36 PS4, Line 36: PowerResource (PR00, 0, 0)
Even so, it seems weird to attach a power resource to the i2c controller when it should be associate […]
Ah, I see what you mean. That's doable.
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/+/35073
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. It will also enable the sharing of power resources between multiple devices in the future, if they share power-related signals.
This would change the (effective) ASL from something like this:
Scope (_SB.PCI0) { Device (I2C0) { ... PowerResource (PRIC, 0, 0) { Method (_ON) ... Method (_OFF) ... } Name (_PR0, Package (1) { PRIC } ) Name (_PR3, Package (1) { PRIC } ) } }
to something more like this:
Scope (_SB.PCI0.I2C0) { PowerResource (PR00, 0, 0) { Method (_ON)... Method (_OFF)... } }
Scope (_SB.PCI0) { Device (I2C0) { ... Name (_PR0, Package (1) { PR00 } ) Name (_PR3, Package (1) { PR00 } ) } }
Change-Id: Id9d66fedd342eb5359d90d9778c515e7aa2de1d7 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- A src/drivers/generic/power_resource/Kconfig 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 4 files changed, 128 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/35073/5
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/+/35073
to look at the new patch set (#6).
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. It will also enable the sharing of power resources between multiple devices in the future, if they share power-related signals.
This would change the (effective) ASL from something like this:
Scope (_SB.PCI0.I2C0) { Device (D001) { ... PowerResource (PRIC, 0, 0) { Method (_ON) ... Method (_OFF) ... } Name (_PR0, Package (1) { PRIC } ) Name (_PR3, Package (1) { PRIC } ) } }
to something more like this:
Scope (_SB.PCI0.I2C0) { Device (D001) { ... Name (_PR0, Package (1) { _SB.PCI0.I2C0.D001.PR00 } ) Name (_PR3, Package (1) { _SB.PCI0.I2C0.D001.PR00 } ) } }
Scope (_SB.PCI0.I2C0.D001) { PowerResource (PR00, 0, 0) { Method (_ON)... Method (_OFF)... } }
Change-Id: Id9d66fedd342eb5359d90d9778c515e7aa2de1d7 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- A src/drivers/generic/power_resource/Kconfig 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 4 files changed, 132 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/35073/6
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35073 )
Change subject: drivers/generic: Add new driver for power resource pseudo-device ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35073/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35073/4//COMMIT_MSG@36 PS4, Line 36: PowerResource (PR00, 0, 0)
Ah, I see what you mean. That's doable.
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35073 )
Change subject: drivers/generic: Add new driver for power resource pseudo-device ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35073/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35073/4//COMMIT_MSG@36 PS4, Line 36: PowerResource (PR00, 0, 0)
Done
Was this done in both ACPI and devicetree? Looking at the CLs following this, I think it is organized very differently in coreboot and in ACPI:
ACPI --> Power resource associated with I2C peripheral. Lives under the peripheral device in ACPI hierarchy.
Devicetree in coreboot --> Power resource is parent of the I2C peripheral.
I think we should be consistent in both representations.
https://review.coreboot.org/c/coreboot/+/35073/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35073/6//COMMIT_MSG@52 PS6, Line 52: Can you also add example of what devicetree looked like before and what it would look like with this change?
https://review.coreboot.org/c/coreboot/+/35073/6/src/drivers/generic/power_r... File src/drivers/generic/power_resource/Kconfig:
https://review.coreboot.org/c/coreboot/+/35073/6/src/drivers/generic/power_r... PS6, Line 1: TODO TODO(twawrzynczak)
https://review.coreboot.org/c/coreboot/+/35073/6/src/drivers/generic/power_r... File src/drivers/generic/power_resource/power_resource.c:
https://review.coreboot.org/c/coreboot/+/35073/6/src/drivers/generic/power_r... PS6, Line 66: /* .acpi_fill_ssdt_generator = power_resource_fill_ssdt,*/ Get rid of commented code?
Tim Wawrzynczak has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35073 )
Change subject: drivers/generic: Add new driver for power resource pseudo-device ......................................................................
Abandoned
needs more thought
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Tim Wawrzynczak (Code Review)