coreboot-gerrit May 2021

coreboot-gerrit@coreboot.org

1 participants
6048 discussions

Change in coreboot[master]: tpm: Remove USER_TPMx options, make TPM1/TPM2 menuconfig visible
by Julius Werner (Code Review) 27 May '21

27 May '21

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/54641 ) Change subject: tpm: Remove USER_TPMx options, make TPM1/TPM2 menuconfig visible ...................................................................... tpm: Remove USER_TPMx options, make TPM1/TPM2 menuconfig visible We would like to have an easy way to completely disable TPM support on a board. For boards that don't pre-select a TPM protocol via the MAINBOARD_HAS_TPMx options, this is already possible with the USER_NO_TPM option. In order to make this available for all boards, this patch just removes the whole USER_TPMx option group and directly makes the TPM1 and TPM2 options visible to menuconfig. The MAINBOARD_HAS_TPMx options can still be used to select defaults and to prevent selection of a protocol that the TPM is known to not support, but the NO_TPM option always remains available. Also fix some mainboards that selected TPM2 directly, which they're not supposed to do (that's what MAINBOARD_HAS_TPM2 is for), and add a missing dependency to TPM_CR50 so it is set correctly for a NO_TPM scenario. Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: Ib0a73da3c42fa4e8deffecb53f29ee38cbb51a93 Reviewed-on: https://review.coreboot.org/c/coreboot/+/54641 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Aaron Durbin <adurbin(a)chromium.org> Reviewed-by: Christian Walter <christian.walter(a)9elements.com> --- M configs/config.asrock_b85m_pro4.tpm2_txt_placeholder_acms M configs/config.libretrend_lt1000 M src/mainboard/google/drallion/Kconfig M src/mainboard/protectli/vault_kbl/Kconfig M src/security/tpm/Kconfig M src/security/tpm/tss/vendor/cr50/Kconfig 6 files changed, 33 insertions(+), 43 deletions(-) Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Christian Walter: Looks good to me, approved diff --git a/configs/config.asrock_b85m_pro4.tpm2_txt_placeholder_acms b/configs/config.asrock_b85m_pro4.tpm2_txt_placeholder_acms index 856701f..4edeb0c 100644 --- a/configs/config.asrock_b85m_pro4.tpm2_txt_placeholder_acms +++ b/configs/config.asrock_b85m_pro4.tpm2_txt_placeholder_acms @@ -3,7 +3,7 @@ # Used ACMs were extracted from a Supermicro X10SLH firmware update. CONFIG_VENDOR_ASROCK=y CONFIG_BOARD_ASROCK_B85M_PRO4=y -CONFIG_USER_TPM2=y +CONFIG_TPM2=y CONFIG_INTEL_TXT=y CONFIG_INTEL_TXT_BIOSACM_FILE="3rdparty/blobs/cpu/intel/stm/stm.bin" CONFIG_INTEL_TXT_SINITACM_FILE="3rdparty/blobs/cpu/intel/stm/stm.bin" diff --git a/configs/config.libretrend_lt1000 b/configs/config.libretrend_lt1000 index f12ae3f..33159f1 100644 --- a/configs/config.libretrend_lt1000 +++ b/configs/config.libretrend_lt1000 @@ -1,5 +1,5 @@ CONFIG_VENDOR_LIBRETREND=y CONFIG_POWER_STATE_OFF_AFTER_FAILURE=y CONFIG_GENERIC_LINEAR_FRAMEBUFFER=y -CONFIG_USER_TPM2=y +CONFIG_TPM2=y CONFIG_SEABIOS_ADD_SERCON_PORT_FILE=y diff --git a/src/mainboard/google/drallion/Kconfig b/src/mainboard/google/drallion/Kconfig index 31cac26..9499a60 100644 --- a/src/mainboard/google/drallion/Kconfig +++ b/src/mainboard/google/drallion/Kconfig @@ -21,7 +21,6 @@ select SOC_INTEL_COMMON_BLOCK_HDA_VERB select SOC_INTEL_COMMON_BLOCK_SMM_ESPI_DISABLE select SYSTEM_TYPE_LAPTOP - select TPM2 select MAINBOARD_USES_IFD_EC_REGION select HAVE_SPD_IN_CBFS diff --git a/src/mainboard/protectli/vault_kbl/Kconfig b/src/mainboard/protectli/vault_kbl/Kconfig index 7cf80e0..7aa78aa 100644 --- a/src/mainboard/protectli/vault_kbl/Kconfig +++ b/src/mainboard/protectli/vault_kbl/Kconfig @@ -13,7 +13,7 @@ select SUPERIO_ITE_IT8772F select MAINBOARD_HAS_CRB_TPM select HAVE_INTEL_PTT - select TPM2 + select MAINBOARD_HAS_TPM2 config IRQ_SLOT_COUNT int diff --git a/src/security/tpm/Kconfig b/src/security/tpm/Kconfig index 96ab2e6..e228a3d 100644 --- a/src/security/tpm/Kconfig +++ b/src/security/tpm/Kconfig @@ -4,22 +4,42 @@ menu "Trusted Platform Module" +choice + prompt "Trusted Platform Module" + default TPM2 if MAINBOARD_HAS_TPM2 + default TPM1 if MAINBOARD_HAS_TPM1 + default NO_TPM + +config NO_TPM + bool "No TPM" + help + No TPM support. Select this option if your system doesn't have a TPM, + or if you don't want coreboot to communicate with your TPM in any way. + (If your board doesn't offer a TPM interface, this will be the only + possible option.) + config TPM1 - bool - default y if MAINBOARD_HAS_TPM1 || USER_TPM1 + bool "TPM 1.2" depends on MAINBOARD_HAS_LPC_TPM || \ MAINBOARD_HAS_I2C_TPM_GENERIC || \ MAINBOARD_HAS_I2C_TPM_ATMEL + depends on !MAINBOARD_HAS_TPM2 + help + Select this option if your TPM uses the older TPM 1.2 protocol. config TPM2 - bool - default y if MAINBOARD_HAS_TPM2 || USER_TPM2 + bool "TPM 2.0" depends on MAINBOARD_HAS_I2C_TPM_GENERIC || \ MAINBOARD_HAS_LPC_TPM || \ MAINBOARD_HAS_I2C_TPM_ATMEL || \ MAINBOARD_HAS_I2C_TPM_CR50 || \ MAINBOARD_HAS_SPI_TPM || \ MAINBOARD_HAS_CRB_TPM + depends on !MAINBOARD_HAS_TPM1 + help + Select this option if your TPM uses the newer TPM 2.0 protocol. + +endchoice config TPM bool @@ -28,45 +48,15 @@ config MAINBOARD_HAS_TPM1 bool + help + This option can be selected by a mainboard to represent that its TPM + always uses the 1.2 protocol, and that it should be on by default. config MAINBOARD_HAS_TPM2 bool - -if !MAINBOARD_HAS_TPM1 && !MAINBOARD_HAS_TPM2 - -choice - prompt "Trusted Platform Module" - default USER_NO_TPM - -config USER_NO_TPM - bool "disabled" - -config USER_TPM1 - bool "1.2" - depends on MAINBOARD_HAS_LPC_TPM || \ - MAINBOARD_HAS_I2C_TPM_GENERIC || \ - MAINBOARD_HAS_I2C_TPM_ATMEL help - Enable this option to enable TPM 1.0 - 1.2 support in coreboot. - - If unsure, say N. - -config USER_TPM2 - bool "2.0" - depends on MAINBOARD_HAS_I2C_TPM_GENERIC || \ - MAINBOARD_HAS_LPC_TPM || \ - MAINBOARD_HAS_I2C_TPM_ATMEL || \ - MAINBOARD_HAS_I2C_TPM_CR50 || \ - MAINBOARD_HAS_SPI_TPM || \ - MAINBOARD_HAS_CRB_TPM - help - Enable this option to enable TPM 2.0 support in coreboot. - - If unsure, say N. - -endchoice - -endif + This option can be selected by a mainboard to represent that its TPM + always uses the 2.0 protocol, and that it should be on by default. config TPM_DEACTIVATE bool "Deactivate TPM" diff --git a/src/security/tpm/tss/vendor/cr50/Kconfig b/src/security/tpm/tss/vendor/cr50/Kconfig index 52c7385..c4ecdef 100644 --- a/src/security/tpm/tss/vendor/cr50/Kconfig +++ b/src/security/tpm/tss/vendor/cr50/Kconfig @@ -2,6 +2,7 @@ config TPM_CR50 bool + depends on TPM2 default y if MAINBOARD_HAS_I2C_TPM_CR50 || MAINBOARD_HAS_SPI_TPM_CR50 if TPM_CR50 -- To view, visit https://review.coreboot.org/c/coreboot/+/54641 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib0a73da3c42fa4e8deffecb53f29ee38cbb51a93 Gerrit-Change-Number: 54641 Gerrit-PatchSet: 4 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: tpm: Remove USER_TPMx options, make TPM1/TPM2 menuconfig visible
by Julius Werner (Code Review) 27 May '21

27 May '21

Attention is currently required from: Philipp Deppenwiese, Michał Żygowski, Paul Menzel, Piotr Król. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54641 ) Change subject: tpm: Remove USER_TPMx options, make TPM1/TPM2 menuconfig visible ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/54641/comment/79fe9bc5_95781069 PS3, Line 19: Also fix some mainboards that selected TPM2 directly, which they're not : supposed to do (that's what MAINBOARD_HAS_TPM2 is for), and add a : missing dependency to TPM_CR50 so it is set correctly for a NO_TPM : scenario. > It’d be great if you split this into a separate commit. Let's please not make things too complicated, I don't want to reshuffle both my and Kyösti's patch just for that. -- To view, visit https://review.coreboot.org/c/coreboot/+/54641 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib0a73da3c42fa4e8deffecb53f29ee38cbb51a93 Gerrit-Change-Number: 54641 Gerrit-PatchSet: 3 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Comment-Date: Thu, 27 May 2021 22:01:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: Apply more uses for Kconfig TPM
by Julius Werner (Code Review) 27 May '21

27 May '21

Attention is currently required from: Christian Walter, Kyösti Mälkki. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55016 ) Change subject: Apply more uses for Kconfig TPM ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/55016 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I54b296563940cd46fe9da9fe789b746f2fc1987d Gerrit-Change-Number: 55016 Gerrit-PatchSet: 1 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Comment-Date: Thu, 27 May 2021 21:59:02 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/alderlake: Inject Pre-CPU reset TS into timestamp_table
by Tim Wawrzynczak (Code Review) 27 May '21

27 May '21

Attention is currently required from: Bora Guvendik, Martin Roth, Paul Menzel, Subrata Banik, Patrick Rudolph. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51446 ) Change subject: soc/intel/alderlake: Inject Pre-CPU reset TS into timestamp_table ...................................................................... Patch Set 5: (5 comments) File src/soc/intel/alderlake/telemetry.c: https://review.coreboot.org/c/coreboot/+/51446/comment/d187b300_dd89c679 PS5, Line 20: bool I'm not sure `bool` has a well-defined size for say, 32-bit and 64-bit code, and this is an ABI as well, I think you should use all fixed-size integers in an ABI. https://review.coreboot.org/c/coreboot/+/51446/comment/e436a1d2_3c8cd2e5 PS5, Line 37: u16 time_stamp_6_ms; same https://review.coreboot.org/c/coreboot/+/51446/comment/c5f4e517_ef2b3e78 PS5, Line 31: u32 time_stamp_0_ms; : u16 time_stamp_1_ms; : u16 time_stamp_2_ms; : u16 time_stamp_3_ms; : u16 time_stamp_4_ms; : u16 time_stamp_5_ms; : u16 time_stamp_6_ms; : } mbp_perf_data; doesn't this need to be packed as well? https://review.coreboot.org/c/coreboot/+/51446/comment/ef8e35f4_1d961ef7 PS5, Line 42: bool more bool https://review.coreboot.org/c/coreboot/+/51446/comment/1de9c3aa_537cbdda PS5, Line 97: for (; hob && hob->type != HOB_TYPE_END_OF_HOB_LIST; hob = fsp_next_hob(hob)) { : if (hob->type != HOB_TYPE_GUID_EXTENSION) : continue; : : res = fsp_hob_header_to_resource(hob); : : if (fsp_guid_compare(res->owner_guid, fsp_me_bios_payload_hob_guid)) { : get_pre_reset_ts_from_hob(hob); : return; : } : } I think this just could be: ``` size_t hob_size; const me_bios_payload_hob *mbp_hob = fsp_find_extension_hob_by_guid(fsp_me_bios_payload_hob_guid, &hob_size); if (mbp_hob) get_pre_reset_ts_from_hob(mbp_hob); ``` -- To view, visit https://review.coreboot.org/c/coreboot/+/51446 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4b7728d3da0d38dc0f1b465743486025dcf354b3 Gerrit-Change-Number: 51446 Gerrit-PatchSet: 5 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Thu, 27 May 2021 21:57:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: drivers/pcie/rtd3/device: Add PCIe RTD3 driver
by Raul Rangel (Code Review) 27 May '21

27 May '21

Attention is currently required from: Jason Glenesk, Furquan Shaikh, Marshall Dawson, Tim Wawrzynczak, Karthik Ramasubramanian, Felix Held. Hello build bot (Jenkins), Jason Glenesk, Marshall Dawson, Tim Wawrzynczak, Karthik Ramasubramanian, Felix Held, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/54966 to look at the new patch set (#3). Change subject: drivers/pcie/rtd3/device: Add PCIe RTD3 driver ...................................................................... drivers/pcie/rtd3/device: Add PCIe RTD3 driver This driver was inspired from soc/intel/common/block/pci/rtd3. I decided to copy and modify it because the Intel driver has a lot of Intel specific code. This driver has been stripped down to only provide a power resource and set the StorageD3Enable property. This driver is SoC agnostic and does not handle suspending the actual PCIe root port. That should be implemented by an SoC specific driver. This is required for Guybrush to suspend/resume properly because the NVMe power is tied to the S0 power rails, so the kernel needs to place the device into D3. BUG=b:184617186 TEST=Guybrush is able to suspend/resume properly. Also see power resource get enabled / disabled. [ 56.075559] power-0416 __acpi_power_off : Power resource [RTD3] turned off [ 56.075562] device_pm-0279 device_set_power : Device [PXSX] transitioned to D3cold [ 56.075567] pci_pm_suspend_noirq: nvme 0000:02:00.0: PCI PM: Suspend power state: D3cold [ 56.075569] nvme 0000:02:00.0: pci_pm_suspend_noirq+0x0/0x413 returned 0 after 15978 usecs [ 123.464874] nvme 0000:02:00.0: calling pci_pm_resume_noirq+0x0/0x11d @ 7, parent: 0000:00:02.4 [ 123.464891] acpi_device_set_power: ACPI: \_SB_.PCI0.GP14.PXSX: Power state change: D3cold -> D0 [ 123.464982] power-0360 __acpi_power_on : Power resource [RTD3] turned on [ 123.464984] device_pm-0279 device_set_power : Device [PXSX] transitioned to D0 [ 123.465039] nvme 0000:02:00.0: pci_pm_resume_noirq+0x0/0x11d returned 0 after 158 usecs Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: I2adfc925183ff7a19ab97e89212bc87c40d552d0 --- A src/drivers/pcie/rtd3/device/Kconfig A src/drivers/pcie/rtd3/device/Makefile.inc A src/drivers/pcie/rtd3/device/chip.c A src/drivers/pcie/rtd3/device/chip.h 4 files changed, 200 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/54966/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/54966 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2adfc925183ff7a19ab97e89212bc87c40d552d0 Gerrit-Change-Number: 54966 Gerrit-PatchSet: 3 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Duncan Laurie <dlaurie(a)gmail.com> Gerrit-CC: Furquan Shaikh <furquan(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: acpi/device: Add ability to generate proper _STA for PowerResource
by Raul Rangel (Code Review) 27 May '21

27 May '21

Attention is currently required from: Lance Zhao, Tim Wawrzynczak. Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/55027 ) Change subject: acpi/device: Add ability to generate proper _STA for PowerResource ...................................................................... acpi/device: Add ability to generate proper _STA for PowerResource acpi_device_add_power_res currently generates a `_STA` method hardcoded to ON. This change enables the ability to generate a `_STA` method that queries the status of the GPIOs to determine if the power resource is ON or OFF. The acpigen_write_power_res_STA method was copied from soc/intel/common/block/pcie/rtd3.c:pcie_rtd3_acpi_method_status. I also added support for checking the stop_gpio. BUG=b:184617186 TEST=Dump SSDT table for guybrush Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: I91410556db002c620fd9aaa99981457808da93a5 --- M src/acpi/device.c M src/include/acpi/acpi_device.h 2 files changed, 40 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/55027/1 diff --git a/src/acpi/device.c b/src/acpi/device.c index 5d4d9be..dfbab3e 100644 --- a/src/acpi/device.c +++ b/src/acpi/device.c @@ -598,6 +598,35 @@ acpi_device_fill_len(desc_length); } +static void acpigen_write_power_res_STA(const struct acpi_power_res_params *params) +{ + const struct acpi_gpio *gpio; + + acpigen_write_method("_STA", 0); + + /* Use enable GPIO for status if provided, otherwise reset GPIO, and finally stop */ + if (params->enable_gpio->pin_count) + gpio = params->enable_gpio; + else if (params->reset_gpio->pin_count) + gpio = params->reset_gpio; + else + gpio = params->stop_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_write_else(); + acpigen_write_return_op(ONE_OP); + acpigen_pop_len(); /* Else */ + + acpigen_pop_len(); /* Method */ +} + /* PowerResource() with Enable and/or Reset control */ void acpi_device_add_power_res(const struct acpi_power_res_params *params) { @@ -613,8 +642,12 @@ acpigen_write_power_res("PRIC", 0, 0, power_res_dev_states, ARRAY_SIZE(power_res_dev_states)); - /* Method (_STA, 0, NotSerialized) { Return (0x1) } */ - acpigen_write_STA(0x1); + if (params->enable_status) { + acpigen_write_power_res_STA(params); + } else { + /* Method (_STA, 0, NotSerialized) { Return (0x1) } */ + acpigen_write_STA(0x1); + } /* Method (_ON, 0, Serialized) */ acpigen_write_method_serialized("_ON", 0); diff --git a/src/include/acpi/acpi_device.h b/src/include/acpi/acpi_device.h index 301f9b0..da936aa 100644 --- a/src/include/acpi/acpi_device.h +++ b/src/include/acpi/acpi_device.h @@ -456,6 +456,11 @@ * (_OFF method delay) */ unsigned int stop_off_delay_ms; + + /* Write a _STA method that checks the state of the GPIOs. Otherwise + * the _STA method will always return _ON. + */ + bool enable_status; }; /* -- To view, visit https://review.coreboot.org/c/coreboot/+/55027 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I91410556db002c620fd9aaa99981457808da93a5 Gerrit-Change-Number: 55027 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Lance Zhao Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: mb/google/guybrush: Enable RTD3 support for NVMe
by Raul Rangel (Code Review) 27 May '21

27 May '21

Attention is currently required from: Karthikeyan Ramasubramanian. Hello build bot (Jenkins), Karthikeyan Ramasubramanian, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/54967 to look at the new patch set (#2). Change subject: mb/google/guybrush: Enable RTD3 support for NVMe ...................................................................... mb/google/guybrush: Enable RTD3 support for NVMe This will tell the kernel to ignore PCI ASPM when suspending the device and instead place the device into D3. We don't actually have a pin to control power to the NVMe so we leave it in D3Hot. I'm not sure if `PCI_RST#` is working correctly on S0i3 suspend/resume. If it's not acting as expected we can add the reset GPIO and have the OS do it. BUG=b:184617186 TEST=Run suspend_stress_test on guybrush for 10 cycles Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: I29539ac120a9f1b7c1bfeaca745cfc82acfa461a --- M src/mainboard/google/guybrush/Kconfig M src/mainboard/google/guybrush/variants/baseboard/devicetree.cb 2 files changed, 8 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/54967/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/54967 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I29539ac120a9f1b7c1bfeaca745cfc82acfa461a Gerrit-Change-Number: 54967 Gerrit-PatchSet: 2 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Karthikeyan Ramasubramanian <kramasub(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Karthikeyan Ramasubramanian <kramasub(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: drivers/pcie/rtd3/device: Add PCIe RTD3 driver
by Raul Rangel (Code Review) 27 May '21

27 May '21

Attention is currently required from: Jason Glenesk, Furquan Shaikh, Marshall Dawson, Tim Wawrzynczak, Karthik Ramasubramanian, Felix Held. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54966 ) Change subject: drivers/pcie/rtd3/device: Add PCIe RTD3 driver ...................................................................... Patch Set 2: (1 comment) File src/soc/amd/common/block/rtd3/rtd3.c: https://review.coreboot.org/c/coreboot/+/54966/comment/0ac52e5e_99230944 PS1, Line 210: pci_dev_read_resources > That would require 2 of these fake devices for Intel then, wouldn't it? It would require 2 devices, but they wouldn't be fake, we would need to attach the driver to the actual device. -- To view, visit https://review.coreboot.org/c/coreboot/+/54966 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2adfc925183ff7a19ab97e89212bc87c40d552d0 Gerrit-Change-Number: 54966 Gerrit-PatchSet: 2 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Duncan Laurie <dlaurie(a)gmail.com> Gerrit-CC: Furquan Shaikh <furquan(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Thu, 27 May 2021 21:53:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org> Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: drivers/pcie/rtd3/device: Add PCIe RTD3 driver
by Raul Rangel (Code Review) 27 May '21

27 May '21

Attention is currently required from: Jason Glenesk, Furquan Shaikh, Marshall Dawson, Tim Wawrzynczak, Karthik Ramasubramanian, Felix Held. Hello build bot (Jenkins), Jason Glenesk, Marshall Dawson, Tim Wawrzynczak, Karthik Ramasubramanian, Felix Held, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/54966 to look at the new patch set (#2). Change subject: drivers/pcie/rtd3/device: Add PCIe RTD3 driver ...................................................................... drivers/pcie/rtd3/device: Add PCIe RTD3 driver This driver was inspired from soc/intel/common/block/pci/rtd3. I decided to copy and modify it because the Intel driver has a lot of Intel specific code. This driver has been stripped down to only provide a power resource and set the StorageD3Enable property. This driver is SoC agnostic and does not handle suspending the actual PCIe root port. That should be implemented by an SoC specific driver. This is required for Guybrush to suspend/resume properly because the NVMe power is tied to the S0 power rails, so the kernel needs to place the device into D3. BUG=b:184617186 TEST=Guybrush is able to suspend/resume properly. Also see power resource get enabled / disabled. [ 56.075559] power-0416 __acpi_power_off : Power resource [RTD3] turned off [ 56.075562] device_pm-0279 device_set_power : Device [PXSX] transitioned to D3cold [ 56.075567] pci_pm_suspend_noirq: nvme 0000:02:00.0: PCI PM: Suspend power state: D3cold [ 56.075569] nvme 0000:02:00.0: pci_pm_suspend_noirq+0x0/0x413 returned 0 after 15978 usecs [ 123.464874] nvme 0000:02:00.0: calling pci_pm_resume_noirq+0x0/0x11d @ 7, parent: 0000:00:02.4 [ 123.464891] acpi_device_set_power: ACPI: \_SB_.PCI0.GP14.PXSX: Power state change: D3cold -> D0 [ 123.464982] power-0360 __acpi_power_on : Power resource [RTD3] turned on [ 123.464984] device_pm-0279 device_set_power : Device [PXSX] transitioned to D0 [ 123.465039] nvme 0000:02:00.0: pci_pm_resume_noirq+0x0/0x11d returned 0 after 158 usecs Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: I2adfc925183ff7a19ab97e89212bc87c40d552d0 --- A src/drivers/pcie/rtd3/device/Kconfig A src/drivers/pcie/rtd3/device/Makefile.inc A src/drivers/pcie/rtd3/device/chip.c A src/drivers/pcie/rtd3/device/chip.h 4 files changed, 198 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/54966/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/54966 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2adfc925183ff7a19ab97e89212bc87c40d552d0 Gerrit-Change-Number: 54966 Gerrit-PatchSet: 2 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Duncan Laurie <dlaurie(a)gmail.com> Gerrit-CC: Furquan Shaikh <furquan(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/amd/common/block/rtd3: Add AMD specific RTD3 driver
by Tim Wawrzynczak (Code Review) 27 May '21

27 May '21

Attention is currently required from: Jason Glenesk, Raul Rangel, Furquan Shaikh, Marshall Dawson, Karthik Ramasubramanian, Felix Held. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54966 ) Change subject: soc/amd/common/block/rtd3: Add AMD specific RTD3 driver ...................................................................... Patch Set 1: (4 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/54966/comment/3ddca407_c92c487c PS1, Line 9: I decided : to copy and modify it because the Intel driver has a lot of Intel : specific code. > From a quick look, I see that the following things were omitted: […] I warned you Raul 😉 File src/soc/amd/common/block/rtd3/rtd3.c: https://review.coreboot.org/c/coreboot/+/54966/comment/9a756fa6_314945c1 PS1, Line 23: static void pcie_rtd3_acpi_method_on(const struct soc_amd_common_block_rtd3_config *config) : { : acpigen_write_method_serialized("_ON", 0); : : /* Assert enable GPIO to turn on device power. */ : if (config->enable_gpio.pin_count) { : acpigen_enable_tx_gpio(&config->enable_gpio); : if (config->enable_delay_ms) : acpigen_write_sleep(config->enable_delay_ms); : } : : /* De-assert reset GPIO to bring device out of reset. */ : if (config->reset_gpio.pin_count) { : acpigen_disable_tx_gpio(&config->reset_gpio); : if (config->reset_delay_ms) : acpigen_write_sleep(config->reset_delay_ms); : } : : acpigen_write_method_end(); : } : : static void pcie_rtd3_acpi_method_off(const struct soc_amd_common_block_rtd3_config *config) : { : acpigen_write_method_serialized("_OFF", 0); : : /* Assert reset GPIO to place device into reset. */ : if (config->reset_gpio.pin_count) { : acpigen_enable_tx_gpio(&config->reset_gpio); : if (config->reset_off_delay_ms) : acpigen_write_sleep(config->reset_off_delay_ms); : } : : /* De-assert enable GPIO to turn off device power. */ : if (config->enable_gpio.pin_count) { : acpigen_disable_tx_gpio(&config->enable_gpio); : if (config->enable_off_delay_ms) : acpigen_write_sleep(config->enable_off_delay_ms); : } : : acpigen_write_method_end(); : } It's also on my todo-list to clean up the power resources to use acpigen and a separate device/chip... https://review.coreboot.org/c/coreboot/+/54966/comment/a3ce46ba_3a1edcf2 PS1, Line 206: PXSX > So I shot a message to the LKML yesterday asking if we could remove this hardcoded constant: https:/ […] Thanks Raul! https://review.coreboot.org/c/coreboot/+/54966/comment/43ddee71_2c1e0cd9 PS1, Line 210: pci_dev_read_resources > Ah, are you envisioning a case where we have a bridge connected to the root port? […] That would require 2 of these fake devices for Intel then, wouldn't it? -- To view, visit https://review.coreboot.org/c/coreboot/+/54966 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2adfc925183ff7a19ab97e89212bc87c40d552d0 Gerrit-Change-Number: 54966 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Duncan Laurie <dlaurie(a)gmail.com> Gerrit-CC: Furquan Shaikh <furquan(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Thu, 27 May 2021 21:33:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org> Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit May 2021