Change in coreboot[master]: soc/intel/common/block/pcie/rtd3: Make changes to support S3
Duncan Laurie has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48682 )
Change subject: soc/intel/common/block/pcie/rtd3: Make changes to support S3 ......................................................................
soc/intel/common/block/pcie/rtd3: Make changes to support S3
The RTD3 ACPI _ON method will unconditionally attempt to re-initialize the device, setting up GPIOs, clocks, and the L23 ready exit flow.
When using S3 instead of S0ix this work is not needed and instead results in the device disappearing and the resume failing in the OS.
BUG=b:174776411 TEST=test S3 and S0ix on volteer device with both NVMe and SD using the RTD3 driver to ensure suspend/resume works in both cases.
Change-Id: I6bd7d001890939850381858fe663366472aacefc --- M src/soc/intel/common/block/pcie/rtd3/rtd3.c 1 file changed, 36 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/48682/1
diff --git a/src/soc/intel/common/block/pcie/rtd3/rtd3.c b/src/soc/intel/common/block/pcie/rtd3/rtd3.c index be412e7..52a7d03 100644 --- a/src/soc/intel/common/block/pcie/rtd3/rtd3.c +++ b/src/soc/intel/common/block/pcie/rtd3/rtd3.c @@ -74,12 +74,46 @@ acpigen_write_store_int_to_namestr(1, ACPI_REG_PCI_L23_SAVE_STATE); }
+static void pcie_rtd3_check_gpio(const struct soc_intel_common_block_pcie_rtd3_config *config) +{ + const struct acpi_gpio *gpio; + + /* Use enable GPIO for status if provided, otherwise use reset GPIO. */ + if (config->enable_gpio.pin_count) + gpio = &config->enable_gpio; + else + gpio = &config->reset_gpio; + + /* Read current GPIO value into Local0. */ + acpigen_get_tx_gpio(gpio); + + /* Ensure check works for both active low and active high GPIOs. */ + acpigen_write_store_int_to_op(gpio->active_low, LOCAL1_OP); + + acpigen_write_if_lequal_op_op(LOCAL0_OP, LOCAL1_OP); + acpigen_write_store_int_to_op(0, LOCAL0_OP); + acpigen_pop_len(); /* If */ + acpigen_write_else(); + acpigen_write_store_int_to_op(1, LOCAL0_OP); + acpigen_pop_len(); /* Else */ +} + static void pcie_rtd3_acpi_method_on(unsigned int pcie_rp, const struct soc_intel_common_block_pcie_rtd3_config *config) { acpigen_write_method_serialized("_ON", 0);
+ /* + * Exit early if device status GPIOs indicate device is already in the on/ready state. + * This will happen if S3 resume is used instead of S0ix resume where coreboot has + * already re-initalized the device and the ACPI code does not need to run. + */ + pcie_rtd3_check_gpio(config); + acpigen_write_if_lequal_op_int(LOCAL0_OP, 1); + acpigen_write_return_op(ZERO_OP); + acpigen_pop_len(); /* If */ + /* Assert enable GPIO to turn on device power. */ if (config->enable_gpio.pin_count) { acpigen_enable_tx_gpio(&config->enable_gpio); @@ -140,28 +174,10 @@ pcie_rtd3_acpi_method_status(int pcie_rp, const struct soc_intel_common_block_pcie_rtd3_config *config) { - const struct acpi_gpio *gpio; - acpigen_write_method("_STA", 0);
- /* Use enable GPIO for status if provided, otherwise use reset GPIO. */ - if (config->enable_gpio.pin_count) - gpio = &config->enable_gpio; - else - gpio = &config->reset_gpio; - - /* Read current GPIO value into Local0. */ - acpigen_get_tx_gpio(gpio); - - /* Ensure check works for both active low and active high GPIOs. */ - acpigen_write_store_int_to_op(gpio->active_low, LOCAL1_OP); - - acpigen_write_if_lequal_op_op(LOCAL0_OP, LOCAL1_OP); - acpigen_write_return_op(ZERO_OP); - acpigen_pop_len(); /* If */ - acpigen_write_else(); - acpigen_write_return_op(ONE_OP); - acpigen_pop_len(); /* Else */ + pcie_rtd3_check_gpio(config); + acpigen_write_return_op(LOCAL0_OP);
acpigen_pop_len(); /* Method */ }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48682 )
Change subject: soc/intel/common/block/pcie/rtd3: Make changes to support S3 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48682/1/src/soc/intel/common/block/... File src/soc/intel/common/block/pcie/rtd3/rtd3.c:
https://review.coreboot.org/c/coreboot/+/48682/1/src/soc/intel/common/block/... PS1, Line 110: * already re-initalized the device and the ACPI code does not need to run. 'initalized' may be misspelled - perhaps 'initialized'?
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48682
to look at the new patch set (#2).
Change subject: soc/intel/common/block/pcie/rtd3: Make changes to support S3 ......................................................................
soc/intel/common/block/pcie/rtd3: Make changes to support S3
The RTD3 ACPI _ON method will unconditionally attempt to re-initialize the device, setting up GPIOs, clocks, and the L23 ready exit flow.
When using S3 instead of S0ix this work is not needed and instead results in the device disappearing and the resume failing in the OS.
BUG=b:174776411 TEST=test S3 and S0ix on volteer device with both NVMe and SD using the RTD3 driver to ensure suspend/resume works in both cases.
Signed-off-by: Duncan Laurie dlaurie@google.com Change-Id: I6bd7d001890939850381858fe663366472aacefc --- M src/soc/intel/common/block/pcie/rtd3/rtd3.c 1 file changed, 37 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/48682/2
Attention is currently required from: Duncan Laurie, Tim Wawrzynczak. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48682 )
Change subject: soc/intel/common/block/pcie/rtd3: Make changes to support S3 ......................................................................
Patch Set 2: Code-Review+2
Attention is currently required from: Duncan Laurie. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48682 )
Change subject: soc/intel/common/block/pcie/rtd3: Make changes to support S3 ......................................................................
Patch Set 2:
(1 comment)
File src/soc/intel/common/block/pcie/rtd3/rtd3.c:
https://review.coreboot.org/c/coreboot/+/48682/comment/6cc211e1_8da3e8ab PS2, Line 91: /* Ensure check works for both active low and active high GPIOs. */ : acpigen_write_store_int_to_op(gpio->active_low, LOCAL1_OP); : : acpigen_write_if_lequal_op_op(LOCAL0_OP, LOCAL1_OP); : acpigen_write_store_int_to_op(0, LOCAL0_OP); : acpigen_pop_len(); /* If */ : acpigen_write_else(); : acpigen_write_store_int_to_op(1, LOCAL0_OP); : acpigen_pop_len(); /* Else */ doesn't acpigen_get_tx_gpio already take care of the polarity inversion?
Attention is currently required from: Tim Wawrzynczak, Duncan Laurie. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48682 )
Change subject: soc/intel/common/block/pcie/rtd3: Make changes to support S3 ......................................................................
Patch Set 2: -Code-Review
(1 comment)
File src/soc/intel/common/block/pcie/rtd3/rtd3.c:
https://review.coreboot.org/c/coreboot/+/48682/comment/70a9f421_6b793175 PS2, Line 91: /* Ensure check works for both active low and active high GPIOs. */ : acpigen_write_store_int_to_op(gpio->active_low, LOCAL1_OP); : : acpigen_write_if_lequal_op_op(LOCAL0_OP, LOCAL1_OP); : acpigen_write_store_int_to_op(0, LOCAL0_OP); : acpigen_pop_len(); /* If */ : acpigen_write_else(); : acpigen_write_store_int_to_op(1, LOCAL0_OP); : acpigen_pop_len(); /* Else */
doesn't acpigen_get_tx_gpio already take care of the polarity inversion?
You are right. It does.
Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak. Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48682 )
Change subject: soc/intel/common/block/pcie/rtd3: Make changes to support S3 ......................................................................
Patch Set 2:
(1 comment)
File src/soc/intel/common/block/pcie/rtd3/rtd3.c:
https://review.coreboot.org/c/coreboot/+/48682/comment/48470000_c3b5c5e9 PS2, Line 91: /* Ensure check works for both active low and active high GPIOs. */ : acpigen_write_store_int_to_op(gpio->active_low, LOCAL1_OP); : : acpigen_write_if_lequal_op_op(LOCAL0_OP, LOCAL1_OP); : acpigen_write_store_int_to_op(0, LOCAL0_OP); : acpigen_pop_len(); /* If */ : acpigen_write_else(); : acpigen_write_store_int_to_op(1, LOCAL0_OP); : acpigen_pop_len(); /* Else */
You are right. It does.
hm I guess this isn't doing what I wanted anyway, I just copied the same code from before but it needs to account for the fact that power and reset behave differently (power asserted means device is on, reset asserted means device is held in reset) and while most device have GPIOs configured the same way (active high power, active low reset) that isn't guaranteed to be true.
Duncan Laurie has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/48682 )
Change subject: soc/intel/common/block/pcie/rtd3: Make changes to support S3 ......................................................................
Abandoned
This was not working on all systems and does not seem like it should be necessary.
-
build bot (Jenkins) (Code Review) -
Duncan Laurie (Code Review)
-
Furquan Shaikh (Code Review)
-
Tim Wawrzynczak (Code Review)