summaryrefslogtreecommitdiff
path: root/src/soc/amd/common/block
AgeCommit message (Collapse)Author
2021-09-09soc/amd/common/block/acpi/gpio: add warning for remote GPIO usageFelix Held
Right now the ACPI code doesn't support accessing the remote GPIO block yet, so don't generate invalid remote GPIO access functions and warn about those being unsupported. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: Id364a59c9650bf4e3633b494b01ab23c0bbc50b2 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56817 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Raul Rangel <rrangel@chromium.org> Reviewed-by: Paul Menzel <paulepanter@mailbox.org> Reviewed-by: Jason Glenesk <jason.glenesk@gmail.com>
2021-09-09soc/amd/common/block/gpio_banks: add remote GPIO supportFelix Held
Some AMD SoCs have a 5th GPIO bank, the remote GPIO bank, which isn't located right after the 4th GPIO bank, but instead at a different location inside the APCIMMIO region. A difference to the first 4 GPIO banks is that the corresponding GPIO MUX registers aren't in a separate bank, but at the end of the remote GPIO region. So this remote GPIO region only supports 48 GPIOs with a 32 bit configuration register each and has the 8 bit GPIO MUX registers beginning at offset 0xc0 in the remote GPIO region. For now using the remote GPIOs from verstage on PSP isn't supported. To support this, it would need to map acpimmio_remote_gpio and update the pointer like it already does for acpimmio_gpio0, acpimmio_iomux and a few others. BUG=b:194524995 Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: Ic8d7ff677a99381a5558782b80b0c4cae67602db Reviewed-on: https://review.coreboot.org/c/coreboot/+/56810 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Raul Rangel <rrangel@chromium.org>
2021-09-09soc/amd/common/block/acpimmio: add remote GPIO bank ACPIMMIO regionFelix Held
Currently coreboot for the AMD SOCs only supports accessing the up to 4 main GPIO banks of up to 64 GPIOs each. Some AMD SoCs including Cezanne have another GPIO bank in the ACPIMMIO region that can contain up to 48 GPIOs beginning with GPIO 256 which is called the remote GPIO bank. The first 48 DWORDs of that ACPIMMIO bank are the 32 bit wide GPIO registers and beginning at offset 0xc0 it has the corresponding 8 bit wide GPIO MUX registers. BUG=b:194524995 Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: Ice4e3358de17ac2601621814978cdb70e6f2c926 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56676 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Martin Roth <martinroth@google.com>
2021-09-09soc/amd/common/block/include/i2c: introduce I2C_RESET_SCL_PIN macroFelix Held
Add and use the I2C_RESET_SCL_PIN macro for populating the i2c_scl_pins array that is used for the sb_reset_i2c_peripherals call to bring the I2C buses into a defined state. TEST=Timeless build results in identical image for Mandolin. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Suggested-by: Raul E Rangel <rrangel@chromium.org> Change-Id: Ifedc09d0bf745545fa0510df7d5037f02b9012a6 Reviewed-on: https://review.coreboot.org/c/coreboot/+/57479 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Raul Rangel <rrangel@chromium.org>
2021-09-08cpu/x86/tsc: Deduplicate Makefile logicAngel Pons
The code under `cpu/x86/tsc` is only compiled in when its `Makefile.inc` is included from platform (CPU/SoC) code and the `UDELAY_TSC` Kconfig option is enabled. Include `cpu/x86/tsc/Makefile.inc` once from `cpu/x86/Makefile.inc` and drop the now-redundant inclusions from platform code. Also, deduplicate the `UDELAY_TSC` guards. Change-Id: I41e96026f37f19de954fd5985b92a08cb97876c1 Signed-off-by: Angel Pons <th3fanbus@gmail.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/57456 Reviewed-by: Arthur Heymans <arthur@aheymans.xyz> Reviewed-by: Felix Held <felix-coreboot@felixheld.de> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-09-08soc/amd/common/block/gpio_banks: add comment about acpimmio_* symbolsFelix Held
Suggested-by: Martin Roth <martinroth@chromium.org> Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I0016a6c7d6581cb261cab6178268c1a86b89c839 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56831 Reviewed-by: Raul Rangel <rrangel@chromium.org> Reviewed-by: Jason Glenesk <jason.glenesk@gmail.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-09-08soc/amd/common/block/gpio_banks: inline iomux_read8 and iomux_write8Felix Held
Since both functions are only called from one function each, inline them into those functions. Also get_gpio_mux just returned the return value of iomux_read8, so there were two functions with identical functionality which shouldn't be the case. Suggested-by: Martin Roth <martinroth@chromium.org> Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I5662d0226edb25a9954fa47b42e208729a79e5a9 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56830 Reviewed-by: Raul Rangel <rrangel@chromium.org> Reviewed-by: Jason Glenesk <jason.glenesk@gmail.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-09-08soc/amd/common/block/gpio_banks: factor out gpio_mux_ptrFelix Held
This aligns the GPIO MUX access more with the GPIO control register access and will facilitate adding support for the remote GPIO bank. Also change the GPIO number argument type to gpio_t. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I4054656c5cc23ea942e8dd370fbbffca304755d6 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56787 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Martin Roth <martinroth@google.com>
2021-09-08soc/amd/common/include/acpimmio: reduce visibility of GPIO MMIO accessFelix Held
Introduce amdblocks/acpimmio_legacy_gpio100.h so that the old pre-SoC chipsets can still access the raw GPIO100 and IOMUX ACPIMMIO registers while only allowing GPIO accesses through the GPIO API on the SoCs. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I18872dfa40d53ba8b0d7802eec52ede5e2ae617a Reviewed-on: https://review.coreboot.org/c/coreboot/+/56786 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Martin Roth <martinroth@google.com>
2021-09-08soc/amd/common/block/gpio_banks: move GPIO MUX access functionsFelix Held
Move those two functions near the top of the file to have all functions that do the hardware accesses in one place. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: If787e6e1d124a932beafd73e5ce7d0ce4869e800 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56782 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Raul Rangel <rrangel@chromium.org>
2021-09-08soc/amd/common/block/gpio_banks/gpio: use unsigned types where neededFelix Held
Use unsigned integers for variables that aren't supposed to become negative. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I5ee037221b9818b0474fe0376323e522c1b3b516 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56701 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com>
2021-09-08soc/amd/common/block/gpio_banks/gpio: use gpio_t for GPIO numbersFelix Held
Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I7cf9cbd2a287dcfe3a47a8a6b164c2b3d8ae95d6 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56700 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Martin Roth <martinroth@google.com>
2021-09-08soc/amd/common: move GPIO ACPIMMIO access functions to gpio_banks blockFelix Held
Since the raw GPIO MMIO register access is now only used inside the gpio_banks block, the gpio_read32 and gpio_write32 functions can be moved to that block to reduce the visibility and enforce the usage of the functions provided by the gpio_banks block. The iomux_read8 and iomux_write8 functions can't be easily moved to the gpio_banks block, since it's also used in the pre-SOC AMD chipsets that use the ACPIMMIO access functions directly. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: Ia0d6dea72c6bebbbe6ce545bedfc74f91e0042c4 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56781 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Martin Roth <martinroth@google.com> Reviewed-by: Jason Glenesk <jason.glenesk@amd.corp-partner.google.com>
2021-09-08soc/amd/common/block/gpio_banks: factor out get_gpio_muxFelix Held
Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I7d7a8c5a7188fd558a577352f8b246e61f3edd63 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56780 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Martin Roth <martinroth@google.com>
2021-09-08soc/amd/common/block/i2c: move raw GPIO access functions to gpio_banksFelix Held
The I2C code should use some GPIO API to access the GPIO registers instead of accessing the GPIO MMIO regions itself. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I84dff381ad86e0c7f879f0f079186aec9cafc604 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56779 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Martin Roth <martinroth@google.com>
2021-09-08soc/amd/common/block/i2c: use common GPIO API in drive_sclFelix Held
No need to do raw GPIO MMIO accesses when basically the same functionality can be achieved by using existing APIs. Using the existing GPIO API instead of raw GPIO MMIO register accesses allows containing all direct GPIO MMIO accesses inside the common AMD GPIO code which will be done in subsequent patches. Since the value parameter of gpio_set is int, change the type of the val parameter of drive_scl to int as well even though I'm not sure why a signed integer was used for this in the common GPIO API. Since program_gpios already configures the SCL GPIOs as outputs, gpio_set can be used in drive_scl which only sets the output value, but doesn't configure the direction. TEST=The waveform on the SCL pin of I2C3 on a barla/careena Chromebook looks similar to the same as before during the reset_i2c_peripherals call, but due to the additional overhead of the read-modify-write to the GPIO register instead of just a write, the pulse width gets about 50% longer. Since the udelay call in drive_scl still has an open TODO to make this configurable and the pulses being longer is in the safe side, this side-effect can be addressed in a follow-up patch. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: Ic323cebc1c83ecd6f0e1fbab419c69489d77face Reviewed-on: https://review.coreboot.org/c/coreboot/+/56777 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Raul Rangel <rrangel@chromium.org>
2021-09-03soc/amd/common/block/gpio_banks/Kconfig: add option for non-soc/ chipsFelix Held
southbridge/amd/pi/hudson uses the common GPIO bank access code from soc/amd, but doesn't provide all functionality that would be needed to use the full functionality. Add a Kconfig option that switches off some functionality in the common SoC GPIO access code, so that more of the functionality proviced by the common SoC GPIO code can be used in the AMD binaryPI chipset and board code. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: Ib10d5d5580aab30a359aa001bb6fc7e9fdb8fc41 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56783 Reviewed-by: Martin Roth <martinroth@google.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-09-01soc/amd/common: Change default spi speeds to 33MHzMartin Roth
In CB:56884 we discussed changing the default fast_read speed from 66MHz, which some platforms may not be capable of running, to 33MHz, which should be generally suitable for all platforms. This same change has been applied to the default for all SPI speeds. Signed-off-by: Martin Roth <martinroth@chromium.org> Change-Id: Ibf926df6829ffdcbae947aaa245356f219615ce8 Reviewed-on: https://review.coreboot.org/c/coreboot/+/57148 Reviewed-by: Felix Held <felix-coreboot@felixheld.de> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-08-30soc/amd/common: Show current SPI speeds and modesMartin Roth
This patch adds code to print the current SPI speeds for each of the 4 different speeds, Normal, Fast-read, Alt-mode, & TPM. It also displays the SPI mode and whether or not SPI100 mode is enabled. BUG=b:194919326 TEST: Display the speed, change speeds, show that new speeds are the expected values. Signed-off-by: Martin Roth <martinroth@chromium.org> Change-Id: I7825a9337474c147b803c85c9af7f9dc24670459 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56960 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-by: Karthik Ramasubramanian <kramasub@google.com>
2021-08-30soc/amd/common: Update SPI based on Kconfig & EFS instead of devtreeMartin Roth
Get the settings for fast-read and mode from EFS, and reprogram those. Program Normal reads, Alt-mode, and TPM speeds from Kconfig settings. BUG=b:195943311 TEST=Boot and see that SPI was set to the correct speed & mode Signed-off-by: Martin Roth <martinroth@chromium.org> Change-Id: I8a24f637b2a0061f60a8f736121d224d4c4ba69b Reviewed-on: https://review.coreboot.org/c/coreboot/+/56959 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Felix Held <felix-coreboot@felixheld.de>
2021-08-30soc/amd/common: move GPIO register state save struct to gpio_banks.hFelix Held
The common_i2c_save struct isn't specific to the I2C code and since it contains the state of the GPIO control & status register and the state of the GPIO MUX register, move it to include/amdblocks/gpio_banks.h and rename it to soc_amd_gpio_register_save. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: If7cd47e5a32427d856948e319de8dfad8c928e96 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56778 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Martin Roth <martinroth@google.com>
2021-08-27soc/amd/common/block/spi: Add SPI config to KconfigMartin Roth
Currently, The SPI speed/mode configuration is split between Kconfig and devicetree. We'd like to have everything in one place. Since we need the fast-read speed and the mode available in the Makefile to build the AMD EFS table, we currently need it in Kconfig. Move all of the settings to Kconfig and remove them from Devicetree in a later commit. BUG=b:195943311 TEST=boot majolica & guybrush, verify spi settings Signed-off-by: Martin Roth <martinroth@chromium.org> Change-Id: I8f29e49e886bd99b39172905e21bfd392c6c10e2 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56884 Reviewed-by: Karthik Ramasubramanian <kramasub@google.com> Reviewed-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-by: Raul Rangel <rrangel@chromium.org> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-08-05soc/amd/common/block/spi: Enable host burst to 4 DWORD when using DMAKarthikeyan Ramasubramanian
Disabling the 4 DWORD bursts causes SPI DMA operations to stall, so leave it enabled when SPI DMA is used. BUG=b:194919326 TEST=Build and boot to OS in Guybrush. Change-Id: I363acdcdb4178a10e4f7eb2bbcbd6d0ca7924f2d Signed-off-by: Karthikeyan Ramasubramanian <kramasub@google.com> Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56683 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Jason Glenesk <jason.glenesk@gmail.com> Reviewed-by: Paul Menzel <paulepanter@mailbox.org>
2021-08-05soc/amd/common/block/spi: introduce SOC_AMD_COMMON_BLOCK_SPI_4DW_BURSTFelix Held
Add a new Kconfig option to enable or disable the 4 DWORD burst support of the SPI controller and use this setting to determine if the corresponding feature bit in SPI100_HOST_PREF_CONFIG will be set or cleared. Since fch_spi_disable_4dw_burst can now enable or disable the feature, rename it to fch_spi_configure_4dw_burst. On Stoneyridge the SPI_RD4DW_EN_HOST bit needs to be cleared (see the Rd4dw_en_host bit definition in the SPIx2C SPI100 Host Prefetch Config register in the public BKDG #55072 Rev 3.09), so add a SoC dependency to the Kconfig option. Signed-off-by: Karthikeyan Ramasubramanian <kramasub@google.com> Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: Id754fa8d5f9554ed25cf9f3341bfdd1968693788 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56814 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Jason Glenesk <jason.glenesk@gmail.com> Reviewed-by: Paul Menzel <paulepanter@mailbox.org>
2021-08-05soc/amd/picasso: Move IVRS generation code to commonJason Glenesk
Move IVRS acpi table generation code to common, so that it can be shared by other programs. BUG=b:190515051 TEST=Build picasso coreboot image. Compare IVRS tables before/after change. Change-Id: Icd5fec3a9d66e8301e267312020e726d9bc1aa70 Signed-off-by: Jason Glenesk <jason.glenesk@amd.corp-partner.google.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56450 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Felix Held <felix-coreboot@felixheld.de>
2021-07-31soc/amd/common/block/acpi: Add IVRS kconfigJason Glenesk
Add new IVRS kconfig option to control IVRS generation. BUG=b:190515051 Change-Id: Iad0c6401dbccd2f3f75464a69e4c27f64d3507a5 Signed-off-by: Jason Glenesk <jason.glenesk@amd.corp-partner.google.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56694 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Felix Held <felix-coreboot@felixheld.de>
2021-07-31soc/amd/common/block/gpio_banks: use unsigned int for gevent parameterFelix Held
A valid GEVENT number is never negative. The local variable in set_single_gpio still needs to be a signed integer, since the return value of get_gpio_gevent being -1 indicates that the GPIO can't generate a GEVENT. The check for that makes the function return before calling program_smi of program_sci, so the parameter of those functions can be changed to unsigned. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I6ce23ceed1585589932824b8cab2a138328672a9 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56705 Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-31soc/amd/common/block/gpio_banks/gpio: add missing newlineFelix Held
Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I87595aea45bb3852a70c7322eae5a94abecb76a1 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56704 Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-31soc/amd/common/block/gpio_banks/gpio: add comment in check_gpiosFelix Held
Each bit in the GPIO wake status index registers is set to 1 when at least one of 4 corresponding GPIO pins has its wake status register set. Added the comment since the gpio_base + i * 4 in the next line looked as if it calculates some absolute register value which is not what the code does or should be doing. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I2fc8e9c5bd7c1b011f364b05d0cfdeb0df88ada6 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56703 Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-31soc/amd/common/block/gpio_banks/gpio: use size_t where neededFelix Held
Since the parameter the variable gets compared with is size_t type, use size_t as type for that variable too. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: If82a948bf71079d456616f4438f4b754e0d7262d Reviewed-on: https://review.coreboot.org/c/coreboot/+/56702 Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-30soc/amd/common/block/include/gpio_banks: use gpio_t for gpio numbersFelix Held
With the addition of the remote GPIO support, the GPIO number won't fit into 8 bit any more, so use the gpio_t type instead which is an uint32_t typedef. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I3de93fd3a2f2af3c1e3b335fef84019c56482051 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56693 Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-30soc/amd/common/block/gpio_banks/gpio: factor out set_gpio_muxFelix Held
Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I75f1e45ead4a5f04cba1eecb220ef027a8bfd09e Reviewed-on: https://review.coreboot.org/c/coreboot/+/56678 Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-30soc/amd/common/block/include/acpimmio_map: drop unused GPIO bank definesFelix Held
The offsets of all GPIOs in the up to four regular banks are all calculated relatively to ACPIMMIO_GPIO0_BANK, so we can just drop the unused defines for ACPIMMIO_GPIO1_BANK and ACPIMMIO_GPIO2_BANK. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I832ffdca479c1f07219a23b4a7f9be69322dfe03 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56675 Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-30soc/amd/common/block/include/acpimmio_map: add GPIO bank 3 to tableFelix Held
GPIO bank 3 isn't used in coreboot, but the existence is documented in both the Picasso PPR #55570 Rev 3.16 and Cezanne PPR #56569 Rev 3.01 and for those two SoCs all 4 banks are covered by the corresponding Memory32Fixed region in the DSDT. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: Id444a97a398d7e3abfd1f5c4a32e762ee6ff68f1 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56674 Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-26soc/amd/common/block/acpimmio: add Kconfig option for biosram accessorsFelix Held
The biosram accessor support in soc/amd/common/block/acpimmio/biosram.c is only used on Stoneyridge and the old amd/southbridge code and not on Picasso or Cezanne. It also only builds as a 32 bit binary and breaks when trying to build as a 64 bit binary, since the size of an uintptr_t is different on those two. There is no support for using the 32 bit binaryPI with a 64 bit coreboot while there is code to use a 32 bit FSP with 64 bit coreboot, so not building this for FSP-based SoC support moves us one step closer to be able to build coreboot as 64 bit binary for Picasso and Cezanne. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I2d87ec2fa1b217eaf55d865e4390308812502e56 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56570 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com>
2021-07-26soc/amd/common/block/pm: Add support for Modern Standby event loggingKarthikeyan Ramasubramanian
Log the GPE and PM1 wake events into the event log using the SMI handler platform callback. BUG=b:186792595, b:186800045 TEST=Build and boot to OS in Guybrush. Ensure that the wake sources are logged into the event logs. 5 | 2021-07-15 16:26:43 | S0ix Enter 6 | 2021-07-15 16:26:49 | S0ix Exit 7 | 2021-07-15 16:26:49 | Wake Source | GPE # | 22 <- Trackpad 8 | 2021-07-15 16:27:07 | S0ix Enter 9 | 2021-07-15 16:27:13 | S0ix Exit 10 | 2021-07-15 16:27:13 | Wake Source | RTC Alarm | 0 25 | 2021-07-15 16:38:13 | S0ix Enter 26 | 2021-07-15 16:38:17 | S0ix Exit 27 | 2021-07-15 16:38:17 | Wake Source | GPE # | 5 <- Fingerprint Change-Id: Icec6fc03f4871cc46b32886575a7054bc289f4bf Signed-off-by: Karthikeyan Ramasubramanian <kramasub@google.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56363 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Furquan Shaikh <furquan@google.com>
2021-07-26soc/amd/common/block/acpi: Extract event logging helpersKarthikeyan Ramasubramanian
Move the event logging helpers defined in acpi into a separate library. This will allow logging power management and GPE events for both S3 and Modern Standby. Introduce a single helper acpi_log_events function to log both PM and GPE events. BUG=None TEST=Build and boot to OS in Guybrush. Change-Id: I96df66edfc824eb3db108098a560d33d758f55ba Signed-off-by: Karthikeyan Ramasubramanian <kramasub@google.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56360 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Furquan Shaikh <furquan@google.com> Reviewed-by: Felix Held <felix-coreboot@felixheld.de>
2021-07-25soc/amd/common/block/cpu/mca/mcax: add comment about McaXEnable bitFelix Held
TEST=Checked on amd/mandolin with PCO APU and google/guybrush with CZN APU that the McaXEnable bit is set in the CONFIG registers of all used MCAX banks. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: Ia4515ba529e758f910d1d135cdce819f83ea0b5c Reviewed-on: https://review.coreboot.org/c/coreboot/+/56535 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Angel Pons <th3fanbus@gmail.com>
2021-07-24soc/amd/common/block/cpu/mca/mca_common: remove additional newlineFelix Held
Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I49a27eb084b59db455153dd662d564a95940a0ab Reviewed-on: https://review.coreboot.org/c/coreboot/+/56534 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Angel Pons <th3fanbus@gmail.com>
2021-07-21commonlib/timestamp,amd/common/block/cpu: Add uCode timestampsRaul E Rangel
This allows keeping track of how long it takes to load the microcode. BUG=b:179699789 TEST=Boot guybrush 112:started reading uCode 990,448 (10,615) 113:finished reading uCode 991,722 (1,274) Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I86b67cf9d17786a380e90130a8fe424734e64657 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56391 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Jason Glenesk <jason.glenesk@gmail.com> Reviewed-by: Paul Menzel <paulepanter@mailbox.org> Reviewed-by: Felix Held <felix-coreboot@felixheld.de>
2021-07-19soc/amd/{common,cezanne}: Implement HAVE_PAYLOAD_PRELOAD_CACHERaul E Rangel
This change allows preloading the payload. BUG=b:179699789 TEST=Boot guybrush and see payload read/decompress drop by 20 ms. We now spend 7ms decompression from RAM. By switching to LZ4 we drop that to 500us. Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I3ec78e628f24f2ba0c9fcf2a9e3bde64687eec44 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56053 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Martin Roth <martinroth@google.com>
2021-07-18soc/amd/common/apob: Add support for asynchronously reading APOB_NVRaul E Rangel
This CL adds a method that can start the processes of reading the APOB from SPI. It does require more RAM in ramstage since we no longer mmap the buffer in the happy path. This will allow us to reduce our boot time by ~10ms. The SoC code will need to be updated to call start_apob_cache_read at a point where it makes sense. BUG=b:179699789 TEST=With this and the patches above I can see a 10 ms reduction in boot time on guybrush. Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I930d58b76eb4558bc4f48ed928c4d6538fefb1e5 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56232 Reviewed-by: Karthik Ramasubramanian <kramasub@google.com> Reviewed-by: Paul Menzel <paulepanter@mailbox.org> Reviewed-by: Furquan Shaikh <furquan@google.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-18soc/amd/common/apob: Switch to using fmap_locate_area_as_rdevRaul E Rangel
Using fmap_locate_area is discouraged. BUG=b:179699789 TEST=Boot guybrush and verify APOB got updated, then reboot and verify APOB was valid. Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I7f58eace8adb4b7ddaf9047d9b8153405d3941a1 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56390 Reviewed-by: Karthik Ramasubramanian <kramasub@google.com> Reviewed-by: Furquan Shaikh <furquan@google.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-18soc/amd/common/block/lpc/spi_dma: Yield after completing transactionRaul E Rangel
There is no telling when the next udelay will be, so explicitly call `thread_yield()` after completing a transaction. This will allow any pending transactions to immediately start. BUG=b:179699789 TEST=Verify new transaction is enqueued right after another. Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I9c1272bde46c3e0c15305b76c2ea7a6dde5ed0b0 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56321 Reviewed-by: Karthik Ramasubramanian <kramasub@google.com> Reviewed-by: Furquan Shaikh <furquan@google.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-18soc/amd/common/block/lpc/spi_dma: Use mutex to protect DMA registersRaul E Rangel
Once we enable COOP_MULTITASKING, we need to guarantee that we don't have multiple threads trying to access the DMA hardware. BUG=b:179699789 TEST=Boot guybrush with APOB patches. Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: Ibb8e31c95d6722521425772f4210af45626c8e09 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56231 Reviewed-by: Karthik Ramasubramanian <kramasub@google.com> Reviewed-by: Furquan Shaikh <furquan@google.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-18soc/amd/common/block/lpc/spi_dma: Implement SPI DMA functionalityRaul E Rangel
This change will make it so the standard rdev readat call will use the SPI DMA controller if the alignment is correct, and the transfer size is larger than 64 bytes. There is a magic bit that needs to be set for the SPI DMA controller to function correctly. This is only available in RN/CZN+. BUG=b:179699789 TEST=Boot guybrush to OS. This reduces loading verstage by 40ms, verifying RW by 500us and loading romstage by 500 us. Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I0be555956581fd82bbe1482d8afa8828c61aaa01 Reviewed-on: https://review.coreboot.org/c/coreboot/+/56228 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Julius Werner <jwerner@chromium.org> Reviewed-by: Furquan Shaikh <furquan@google.com> Reviewed-by: Karthik Ramasubramanian <kramasub@google.com>
2021-07-17soc/amd/common/block/graphics: add GPU PCI ID for BarceloFelix Held
Also rename the existing PCI_DEVICE_ID_ATI_FAM19H_MODEL51H_GPU definition to PCI_DEVICE_ID_ATI_FAM19H_MODEL51H_GPU_CEZANNE to clarify that that is the one for Cezanne. BUG=b:193888172 Change-Id: I1c5446c1517f2e0cd708d3275b08d2bce4be0ea8 Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56396 Reviewed-by: Raul Rangel <rrangel@chromium.org> Reviewed-by: Matt Papageorge <matthewpapa07@gmail.com> Reviewed-by: Nikolai Vyssotski <nikolai.vyssotski@amd.corp-partner.google.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-16include/cpu/amd/msr: don't redefine the IA32_BIOS_SIGN_ID MSRFelix Held
Change-Id: Iff19ae495fb9c0795dae4b2844dc8e0220a57b2c Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56310 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com>
2021-07-15soc/amd/common/block/cpu/mca/mcax: print all MCAX registersFelix Held
Also move the registers in the order they are in the hardware. Change-Id: If018e746e58c14475caeda76feb8b5281d7732f1 Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56315 Reviewed-by: Martin Roth <martinroth@google.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-15soc/amd/common/block/cpu/mca: make building the BERT support conditionalFelix Held
Only when ACPI_BERT is selected the BERT functionality needs to be included in the build. Change-Id: I8a21562f4535fb0ea3c53f2ea8df50f66cc6a64c Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56314 Reviewed-by: Martin Roth <martinroth@google.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-15soc/amd/common/block/cpu/mca: commonize mca_check_all_banksFelix Held
Since we don't need to skip the MCA check on cold boot on MCAX capable systems, add a mca_skip_check implementation that always returns false. Change-Id: Id8fc4b6f02b6c02b03172fe11f0451a9893e514d Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56313 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Martin Roth <martinroth@google.com>
2021-07-15soc/amd/common/block/cpu/mca/mca: factor out mca_skip_checkFelix Held
This will allow moving mca_check_all_banks to mca_common.c. Change-Id: I58e100c1447907bab984a2fdff6c6e0181910c23 Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56312 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Martin Roth <martinroth@google.com>
2021-07-15soc/amd/stoneyridge/mca: implement and use mca_has_expected_bank_countFelix Held
This aligns the mca_check_all_banks implementation in the common mca.c with the one in the common mcax.c file. Do the MCA bank count check before the !is_warm_reset() check, so that a mismatch also gets printed on the cold boot path. Change-Id: Idbd3e9ce9c7483f84f87adab7adac47335cd59aa Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56311 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Martin Roth <martinroth@google.com>
2021-07-15soc/amd/common/block/cpu/mca: move function prototypes to local headerFelix Held
Since those functions are implemented and used only inside the common MCA(X) code, there's no need to have them in the header file that gets included in the SoC-specific code. Change-Id: Ia84e149d67ac7d80de595379c73a6cf08730719d Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56309 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Martin Roth <martinroth@google.com>
2021-07-15soc/amd/*/mca: factor out common MCA/MCAX check & print functionalityFelix Held
For Cezanne stubs are added for the functions that the SoC-specific code needs to provide. Since the mca_is_valid_bank stub on Cezanne always returns false, the checks get skipped for it at the moment. The actual functionality will be added in a later patch. Change-Id: Ic31e9b1ca7f8fac0721c95935c79150d7f774aa4 Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56290 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Martin Roth <martinroth@google.com>
2021-07-15soc/amd/common/blocks/cpu/mca: factor out common BERT helper functionsFelix Held
Change-Id: I03365c3820cbe7277f14adc5460e892fb8d9b7a5 Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56284 Reviewed-by: Martin Roth <martinroth@google.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-15soc/amd/*/mca: factor out BERT entry generation to soc/amd/commonFelix Held
Change-Id: I960a2f384f11e4aa5aa2eb0645b6046f9f2f8847 Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56283 Reviewed-by: Martin Roth <martinroth@google.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-14soc/amd: factor out check_mca to common codeFelix Held
Change-Id: I139d1fe41bad5213da8890c2867f275b6847e3e1 Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56281 Reviewed-by: Martin Roth <martinroth@google.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-14soc/amd: move check_mca prototype to soc/amd/common/blocks/includeFelix Held
Change-Id: Ia489dbfba59c334cf29f96a4000cef73b9b797d4 Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56279 Reviewed-by: Raul Rangel <rrangel@chromium.org> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-14soc/amd/common/block/lpc: Don't disable the HOG bitRaul E Rangel
According to the AMD FCH architects, we should be using the default value for the NO_HOG bit. This fixes a problem where the SPI DMA no longer functions after the LPC init runs. BUG=b:179699789, b:192373221 TEST=Boot guybrush and see SPI DMA working Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: If015869657f36d3533f4ab9ebd1f54b0d4eb283a Reviewed-on: https://review.coreboot.org/c/coreboot/+/56233 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com>
2021-07-14soc/amd/cezanne: Move APOB update into ramstageRaul E Rangel
There is no technical reason this needs to be done in romstage. Moving it into ramstage allow us (in future CLs) to use threads to pre-load the apob from SPI. BUG=b:179699789 TEST=Boot and Ezkinil and Guybrush and verify APOB update still work Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I960437ff4400645de5a3e7447fcdbc52de85943e Reviewed-on: https://review.coreboot.org/c/coreboot/+/56227 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Karthik Ramasubramanian <kramasub@google.com>
2021-07-14soc/amd/common/block/apob: Fix incorrect printf formatRaul E Rangel
The %p format specifier already prints out 0x, so remove the 0x from the string. I also updated the other format specifiers to use the %# syntax to print out the 0x. BUG=b:179699789 TEST=see correct format. Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I5b00d2c06687e549f69486eb5e18f7bed560b2ec Reviewed-on: https://review.coreboot.org/c/coreboot/+/56225 Reviewed-by: Karthik Ramasubramanian <kramasub@google.com> Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-07-12soc/amd/{cezanne,common}: Enable IOMMU PCIe DeviceRaul E Rangel
This change only enables the IOMMU device. We still require the IVRS table to take advantage of the IOMMU. This will happen when the picasso IVRS code is moved into common. BUG=b:190515051 TEST=lspci shows IOMMU device 00:00.2 IOMMU: Advanced Micro Devices, Inc. [AMD] Device 1631 Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I5c7cae3d25af5a45d48658ffa948a2856adc4346 Reviewed-on: https://review.coreboot.org/c/coreboot/+/55737 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Jason Glenesk <jason.glenesk@gmail.com> Reviewed-by: Felix Held <felix-coreboot@felixheld.de>
2021-07-02soc/amd/common/espi: Fix debug message log levelRaul E Rangel
BUG=none TEST=Boot with CONSOLE_LOGLEVEL_3 and no longer see the message printed. Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I0bdb92f547ceb8be624521211f4a3b94a91dae22 Reviewed-on: https://review.coreboot.org/c/coreboot/+/55972 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Martin Roth <martinroth@google.com>
2021-07-02soc/amd/common/block/cpu: Cache the uCode to avoid multiple SPI readsRaul E Rangel
We are currently reading the uCode for each CPU. This is unnecessary since the uCode never changes. BUG=b:177909625 TEST=Boot guybrush and see "microcode: being updated to patch id" for each CPU. I no longer see CBFS access for each CPU. This drops device initialization time by 32 ms. Also boot Ezkinil and verify microcode was also updated. Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I98b9d4ce8290a1f08063176809e903e671663208 Reviewed-on: https://review.coreboot.org/c/coreboot/+/55987 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Furquan Shaikh <furquan@google.com> Reviewed-by: Karthik Ramasubramanian <kramasub@google.com>
2021-07-02soc/amd/common/block/lpc: Add custom SPI DMA boot deviceRaul E Rangel
This is a copy of mmap_boot.c and mem_rdev_ro_ops. I split it up so it was easier to review. The next patches will add support for the SPI DMA controller. This will provide a minor speed up vs using mmap reads. It will also provide the facilities to perform asynchronous SPI loading. BUG=b:179699789 TEST=Boot guybrush to OS Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: Id26e2a69601d0f31e256d0010008904a447c8e21 Reviewed-on: https://review.coreboot.org/c/coreboot/+/55854 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Martin Roth <martinroth@google.com>
2021-06-28soc/amd/cezanne: Add call to mb to configure eSPI requirementsMartin Roth
When initializing espi early, there may be mainboard requirements to configure the bus properly. This allows the mainboard to do that. BUG=192100564 TEST=Build along with next patch, eSPI works on guybrush Change-Id: Icc02877a09b8f8ed20fd1b04f3cee0509f1a85c5 Signed-off-by: Martin Roth <martinroth@chromium.org> Reviewed-on: https://review.coreboot.org/c/coreboot/+/55863 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Nikolai Vyssotski <nikolai.vyssotski@amd.corp-partner.google.com> Reviewed-by: Jason Glenesk <jason.glenesk@gmail.com> Reviewed-by: Raul Rangel <rrangel@chromium.org>
2021-06-23soc/amd/common/acp: Populate _WOV ACPI methodKarthikeyan Ramasubramanian
In order to support Audio Co-processor (ACP) DMIC hardware runtime detection on the platform, ACPI _WOV method is populated on the concerned ACP device. This method returns the ACPI Integer value as 1 if ACP DMIC exists on the platform. BUG=b:182960979 TEST=Build and boot to OS in guybrush. Ensure that the _WOV ACPI method is populated under the scope of ACP device. Scope (\_SB.PCI0.GP41.ACPD) { Method (_WOV, 0, NotSerialized) { Return (One) } } Change-Id: Ide84f45f5ea2ae42d5efe71ac6d1595886157045 Signed-off-by: Karthikeyan Ramasubramanian <kramasub@google.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/55029 Reviewed-by: Furquan Shaikh <furquan@google.com> Reviewed-by: Raul Rangel <rrangel@chromium.org> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-06-23soc/amd/common/block/cpu/noncar/memmap: move BERT region back into CBMEMFelix Held
The original reason the BERT table was moved out of CBMEM was because the OS was not able to access the region. This happened because the CBMEM region was marked as type 16 in the e820 table. The OS isn't aware of this type, so it prevents any drivers from accessing it. Depthcharge now correctly labels the CBMEM region as reserved in the e820 table so we can move the BERT table into CBMEM. TEST=BERT ACPI table generation still works on AMD/Mandolin with SeaBIOS as payload and BERT region inside CBMEM is inside a BIOS-e820 reserved range. BERT generation also works on Zork with depthcharge. Link: https://crrev.com/c/2939677 Signed-off-by: Raul E Rangel <rrangel@chromium.org> Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: Ie640e91c19ae5f9b275cc333284b4be34211fbf6 Reviewed-on: https://review.coreboot.org/c/coreboot/+/55279 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com>
2021-06-22soc/amd/common: Add GPIO config for native func w/ output driveMartin Roth
Our existing native function gpio configuration macro (PAD_NF) only sets the pull. For PCIe reset, we now need to be able to set it to its native function (PCIE_RST_L), and drive it low, then high. BUG=b:182805349 TEST=Configure GPIO, see correct behavior. Signed-off-by: Martin Roth <martinroth@chromium.org> Change-Id: I636371517c99f94f76834abc4575795d51aa0368 Reviewed-on: https://review.coreboot.org/c/coreboot/+/55652 Reviewed-by: Raul Rangel <rrangel@chromium.org> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-06-16timestamp,amd/common/apob_cache: Add timestamps for APOBRaul E Rangel
Updating the APOB takes a considerable amount of time. I decided to be granular and split out the operations so we know when we read vs read + erase + write. BUG=b:179092979 TEST=Boot guybrush and dump timestamps 3:after RAM initialization 3,025,425 (44) 920:starting APOB read 3,025,430 (5) 921:starting APOB erase 3,025,478 (48) 922:starting APOB write 3,027,727 (2,249) 923:finished APOB 3,210,965 (183,238) 4:end of romstage 3,210,971 (6) Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I08e371873112e38f623f452af0eb946f5471c399 Reviewed-on: https://review.coreboot.org/c/coreboot/+/55401 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Angel Pons <th3fanbus@gmail.com>
2021-06-16soc/amd/common/block/acpi/bert: fix NULL checkFelix Held
In acpi_soc_get_bert_region after the bert_errors_region call is was checked if the region parameter is NULL after the call; since region is a parameter of acpi_soc_get_bert_region, it's non-NULL. What we should be checking here is if region points to a non-NULL pointer. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Reported-by: Coverity (CID:1457506) Change-Id: I0523504d65725ab2d2df4db28a5dedd90697b917 Reviewed-on: https://review.coreboot.org/c/coreboot/+/55541 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Raul Rangel <rrangel@chromium.org> Reviewed-by: Angel Pons <th3fanbus@gmail.com>
2021-06-14soc/amd/common/pi/agesawrapper: use IOAPIC ID definesFelix Held
Part of the soc/amd/stoneyridge code already uses the FCH_IOAPIC_ID and GNB_IOAPIC_ID defines. Use those defines in the remaining location to make sure that the IOAPIC IDs are always consistent between the hardware register, the MADT and the IVRS ACPI tables. TEST=Timeless build of amd/gardenia results in identical binary. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I410a6560de66889b153c8a66b8dc5474ac114ba7 Reviewed-on: https://review.coreboot.org/c/coreboot/+/55429 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Kyösti Mälkki <kyosti.malkki@gmail.com> Reviewed-by: Michał Żygowski <michal.zygowski@3mdeb.com>
2021-06-08soc/amd: factor out acpi_soc_get_bert_region to amd/commonFelix Held
This also adds BERT table gerenation support for Cezanne, but since the functionality to populate the BERT memory region isn't implemented yet, this won't result in a BERT table being generated on Cezanne, since bert_generate_ssdt will always return false there. TEST=BERT ACPI table generation still works on AMD/Mandolin Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I69b4a9a7432041e1f4902436fa4e6dee5332dbd2 Reviewed-on: https://review.coreboot.org/c/coreboot/+/55056 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com>
2021-05-30soc/amd/common/block/espi: Explicitly assert PLTRST#Raul E Rangel
PLTRST# is currently asserted and latched when eSPI_RST# gets asserted. If eSPI_RST# isn't used on a platform or it doesn't properly assert in all cases, then PLTRST# will never be asserted. This could result in the AP and EC being out of sync. BUG=b:188188172, b:188935533 TEST=Warm reset guybrush with partial #22 rework. Verify that peripheral channel is correctly reset. Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I20d12edf3efc6100096e24aa8d1aec76bbde264f Reviewed-on: https://review.coreboot.org/c/coreboot/+/54884 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Rob Barnes <robbarnes@google.com>
2021-05-27soc/amd/common/block: Fix missing include in acp.hRaul E Rangel
We were missing the stdint.h header, and the header was sorted incorrectly in chip.h BUG=non TEST=build guybrush Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I209d3c9c48e5b06b2a56759af51cf2858eb99f51 Reviewed-on: https://review.coreboot.org/c/coreboot/+/54922 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Felix Held <felix-coreboot@felixheld.de>
2021-05-26soc/amd/common/block/espi: Fix typo in espi_setup_periph_channelRaul E Rangel
ESPI_SLAVE_CHANNEL_READY is a read-only bit from the host perspective. It is set when the eSPI peripheral has configured the channel. We actually want to set the ESPI_SLAVE_CHANNEL_ENABLE flag. This never caused an issue before because the peripheral channel is enabled by default after PLTRST# is deasserted. This does fix the case where periph_ch_en == 0. It now properly clears the enable flag. BUG=b:188188172, b:188935533 TEST=Boot guybrush to OS, perform warm reset Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I24e0734d5652601ae9c967da528fec5e3f780991 Reviewed-on: https://review.coreboot.org/c/coreboot/+/54883 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Rob Barnes <robbarnes@google.com> Reviewed-by: Furquan Shaikh <furquan@google.com>
2021-05-25soc/amd/common/block/espi: Increase ESPI_CH_READY_TIMEOUT_US to 10msRaul E Rangel
The ChromeEC might take longer than 1ms for the peripheral channel to be enabled. The PLTRST# interrupt handler takes about ~539us. This doesn't account for the time it takes for the interrupt handler to be scheduled. Increasing the timeout to 10ms gives ample time. BUG=b:188188172, b:188935533 TEST=Boot guybrush and no longer see channel enable errors Suggested-by: Rob Barnes <robbarnes@google.com> Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: Ib6db577bf06175ceb17b446af706ad8c9f891481 Reviewed-on: https://review.coreboot.org/c/coreboot/+/54788 Reviewed-by: Jason Glenesk <jason.glenesk@amd.corp-partner.google.com> Reviewed-by: Furquan Shaikh <furquan@google.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-05-21soc/amd/cezanne,common,picasso: use BERT region reserved by FSP driverFelix Held
commit ce0e2a014009390c4527e064efb59260ef4d3a3b (drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region) adds a mechanism to reserve the BERT region inside the coreboot code, so we can get rid of the workaround to reserve it in the FSP and return the location in a HOB. mcfg->bert_size defaults to 0 which makes the FSP not generate the corresponding HOB, but that field is planned to be removed at least on Cezanne, so don't explicitly set it to 0. BUG=b:169934025 TEST=BERT table that gets generated in a follow-up patch for Picasso points to expected BERT region and Linux is able to access, decode and display it. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: Iaca89b47793bf9982181560f026459a18e7db134 Reviewed-on: https://review.coreboot.org/c/coreboot/+/52584 Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-05-20soc/amd/common: Show espi init in logMartin Roth
BUG=None TEST=See espi init messages in the log. Signed-off-by: Martin Roth <martinroth@chromium.org> Change-Id: I9f856402ed9a026427d3529e6d61450b0623fe48 Reviewed-on: https://review.coreboot.org/c/coreboot/+/54637 Reviewed-by: Raul Rangel <rrangel@chromium.org> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-05-19soc/amd/common/block/espi_util: Work around in-band reset race conditionRaul E Rangel
When performing an in-band reset the host controller and the peripheral can have mismatched IO configs. i.e., The eSPI peripheral can be in IO-4 mode while, the eSPI host will be in IO-1. This results in the peripheral getting invalid packets and thus not responding. This causes the NO_RESPONSE status bit to be set and cause eSPI init to fail. If the peripheral is alerting when we perform an in-band reset, there is a race condition in espi_send_command. 1) espi_send_command clears the interrupt status. 2) eSPI host controller hardware notices the alert and sends a GET_STATUS. 3) espi_send_command writes the in-band reset command. 4) eSPI hardware enqueues the in-band reset until GET_STATUS is complete. 5) GET_STATUS fails with NO_RESPONSE and sets the interrupt status. 6) eSPI hardware performs in-band reset. 7) espi_send_command checks the status and sees a NO_RESPONSE bit. As a workaround we allow the NO_RESPONSE status code when we perform an in-band reset. BUG=b:186135022 TEST=suspend_stress_test and S5->S0 tests on guybrush and zork. Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I71271377f20eaf29032214be98794e1645d9b70a Reviewed-on: https://review.coreboot.org/c/coreboot/+/54070 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-by: Rob Barnes <robbarnes@google.com>
2021-05-18cpu/x86: Only include smm code if CONFIG_HAVE_SMI_HANDLER=yArthur Heymans
This removes the need to include this code separately on each platform. Change-Id: I3d848b1adca4921d7ffa2203348073f0a11d090e Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> Reviewed-on: https://review.coreboot.org/c/coreboot/+/46380 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Raul Rangel <rrangel@chromium.org>
2021-05-13soc/amd: factor out acpigen_write_alib_dptc to common codeFelix Held
Also drop unneeded intermediate cast to void * before casting the address of the struct dptc_input type variables to uint8_t *. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: Ie1e2aa1ec728a4e16d3a587d7400cdfc8962f443 Reviewed-on: https://review.coreboot.org/c/coreboot/+/54077 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Raul Rangel <rrangel@chromium.org>
2021-05-12soc/amd/{common,picasso}: Use common PCIE_GPP_DRIVER driverRaul E Rangel
This will change the names of the GPP bridges, but this ok since there is no hand written ASL that references these names. BUG=b:184766519 TEST=Boot picasso and dump ACPI Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: Ic09200156e8a37bd1a29ca95a17c8f8ae2b92bd3 Reviewed-on: https://review.coreboot.org/c/coreboot/+/54028 Reviewed-by: Jason Glenesk <jason.glenesk@gmail.com> Reviewed-by: Felix Held <felix-coreboot@felixheld.de> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-05-12soc/amd/common/block/pci: Capitalize PCI ACPI namesRaul E Rangel
Lowercase characters are not valid ACPI identifiers. BUG=b:184766519 TEST=Boot picasso to OS and verify ACPI errors are no longer printed. Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I75aca67f4607e97ced8ac00ac68e51c359aff944 Reviewed-on: https://review.coreboot.org/c/coreboot/+/54027 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com> Reviewed-by: Jason Glenesk <jason.glenesk@gmail.com>
2021-05-12soc/amd{common,cezanne}: Move pcie_gpp.c to commonRaul E Rangel
Cezanne and Picasso can now use the same driver. BUG=b:184766519 TEST=Boot guybrush and dump ASL. Verified it didn't change. Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: Ie4ede82935d6c69b323c1fdceaa61e306aa2820a Reviewed-on: https://review.coreboot.org/c/coreboot/+/54026 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-by: Jason Glenesk <jason.glenesk@gmail.com>
2021-05-10soc/amd/picasso: move acpigen_dptc_call_alib to new common alibFelix Held
Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: Ib0f7da12429b6278d1e4bc5d6650c7ee0f3b5209 Reviewed-on: https://review.coreboot.org/c/coreboot/+/53940 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Martin Roth <martinroth@google.com>
2021-05-10Revert "soc/amd/common/espi: Don't set alert pin in espi_set_initial_config"Raul E Rangel
This reverts commit 6eced03b25954e370e20e62f2cbe41f9d5626eae. This prevents zork from booting. We get the following error: eSPI cmd0-cmd2: 00080009 00000000 00000000 data: 00000000. Error: unexpected eSPI status register bits set (Status = 0x10000010) Error: Slave GET_CONFIGURATION failed! This isn't a pure revert. It is more of a fix that keeps the old behavior. BUG=b:187122344 TEST=Boot zork an no longer see eSPI error Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: If75a35d3994b0fd23945a450032d3cc81abeb136 Reviewed-on: https://review.coreboot.org/c/coreboot/+/53932 Reviewed-by: Felix Held <felix-coreboot@felixheld.de> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-05-09soc/amd/common/block/pci: Implement acpigen_write_pci_{GNB,FCH}_PRTRaul E Rangel
This is loosely based off of picasso/pcie_gpp.c. This version uses the acpigen_write_PRT_X methods to write the actual records. There are also two functions, 1 for using the GNB, and one for using the FCH. The FCH one is useful when the GNB IO-APIC has not been initialized. BUG=b:184766519 TEST=Dump guybrush ACPI and verify it looks correct Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I926430074acb969ceb11fdb60ab56dcf91ac4c76 Reviewed-on: https://review.coreboot.org/c/coreboot/+/52917 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Felix Held <felix-coreboot@felixheld.de>
2021-05-09soc/amd/{picasso,common/blocks/pci}: Move populate_pirq_dataRaul E Rangel
The method now dynamically allocates the pirq structure and uses the get_pci_routing_table method. BUG=b:184766519 TEST=Build guybrush and verify picasso SSDT has not changed. Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I297fc3ca7227fb4794ac70bd046ce2f93da8b869 Reviewed-on: https://review.coreboot.org/c/coreboot/+/52913 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Felix Held <felix-coreboot@felixheld.de>
2021-05-09soc/amd/common/block/pci: Introduce struct pci_routing_infoRaul E Rangel
This struct is similar to `struct pci_routing` defined in picasso/pcie_gpp.c. It additionally contains the irq used for the bridge and is structured in a way that the FSP can provide via HOB. The next set of CLs will migrate the pci routing functions used by picasso into common and enable pci routing table generation for cezanne. BUG=b:184766519 TEST=Build guybrush Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I1a8d988d125f407f0aa7bc1722d432446aa9aff8 Reviewed-on: https://review.coreboot.org/c/coreboot/+/53922 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Felix Held <felix-coreboot@felixheld.de>
2021-05-08soc/amd/picasso,common: move ALIB DPTC parameter struct to common codeFelix Held
Also add an alib_ prefix to avoid possible name collisions. TEST=Timeless build for Mandolin results in identical binary. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: Ib0f220a4cde6da764bb8bc589b5f44ae16496bd7 Reviewed-on: https://review.coreboot.org/c/coreboot/+/53918 Reviewed-by: Raul Rangel <rrangel@chromium.org> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-05-08soc/amd/picasso,common: move ALIB DPTC IDs to common codeFelix Held
These parameter IDs are defined in the AGESA Interface specification #55483. This patch also adds a ALIB_DPTC_ prefix to the IDs and makes the names more consistent. TEST=Timeless build for Mandolin results in identical binary. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I75e0504f6274ad50c53faa8fcbde4d6821d85a04 Reviewed-on: https://review.coreboot.org/c/coreboot/+/53917 Reviewed-by: Raul Rangel <rrangel@chromium.org> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-05-08soc/amd: factor out ACPI ALIB function numbers to common codeFelix Held
The ACPI ALIB function numbers are defined in the AMD Generic Encapsulated Software Architecture (AGESA™) Interface Specification (document #55483). TEST=Timeless build stays the same for Mandolin (Picasso) and Gardenia (Stoneyridge). Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I290ef0db32c65ebb2bbbe4f65db4df772b884161 Reviewed-on: https://review.coreboot.org/c/coreboot/+/53915 Reviewed-by: Raul Rangel <rrangel@chromium.org> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-05-06soc/amd/common/espi,mb/: Allow configuring open drain ALERT#Raul E Rangel
Some designs might wish to use an open drain eSPI ALERT#. This change adds an enum that allows setting the eSPI alert mode. BUG=b:187122344, b:186135022 TEST=Boot guybrush using all 3 alert modes Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: Ia35fc59a699cf9444b53aad5c9bb71aa27ce9251 Reviewed-on: https://review.coreboot.org/c/coreboot/+/52954 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Karthik Ramasubramanian <kramasub@google.com> Reviewed-by: Furquan Shaikh <furquan@google.com>
2021-05-06soc/amd/common/espi: Don't set alert pin in espi_set_initial_configRaul E Rangel
The eSPI spec says that the Alert Mode defaults to in-band on reset. This change ensures the controller is in sync with the eSPI peripheral. The configured alert mode is configured in espi_set_general_configuration. BUG=b:187122344, b:186135022 TEST=Boot guybrush and make sure we don't get any eSPI errors. Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: Ib43e190d08d77ecfcd22ead2bf42e5de2202b555 Reviewed-on: https://review.coreboot.org/c/coreboot/+/52953 Reviewed-by: Karthik Ramasubramanian <kramasub@google.com> Reviewed-by: Furquan Shaikh <furquan@google.com> Reviewed-by: Rob Barnes <robbarnes@google.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-05-06soc/amd/common/espi: Print set eSPI peripheral configRaul E Rangel
This will print the config we are setting on the eSPI peripheral. e.g., Setting general configuration: slave: 0x98a00000 controller: 0xe2000000 eSPI Slave configuration: CRC checking enabled Dedicated Alert# used to signal alert event eSPI quad IO mode selected Only eSPI single IO mode supported Alert# pin is open-drain eSPI 33MHz selected eSPI up to 20MHz supported Maximum Wait state: 0 BUG=b:187122344, b:186135022 TEST=Boot guybrush Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I1a2382d8ab3d3f0d14a139c57470cb895112eca9 Reviewed-on: https://review.coreboot.org/c/coreboot/+/52952 Reviewed-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-by: Furquan Shaikh <furquan@google.com> Reviewed-by: Rob Barnes <robbarnes@google.com> Reviewed-by: Karthik Ramasubramanian <kramasub@google.com> Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
2021-05-06soc/amd/{picasso/common}: Move populate_pirq_data prototype to commonRaul E Rangel
This method signature will also be used by cezanne, so move it to common. BUG=b:184766519 TEST=Build picasso Signed-off-by: Raul E Rangel <rrangel@chromium.org> Change-Id: I421bdad51776278f83148174e6f72bdc38249e54 Reviewed-on: https://review.coreboot.org/c/coreboot/+/52912 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-by: Karthik Ramasubramanian <kramasub@google.com>
2021-05-05soc/amd/common/fsp/fsp-acpi: factor out SSDT from HOB functionalityFelix Held
This function will be reused in Cezanne, so move it from the Picasso directory to the common FSP integration code. TEST=On Mandolin Linux finds the AMD SSDT that contains ALIB. Signed-off-by: Felix Held <felix-coreboot@felixheld.de> Change-Id: I7b256de712fe60d1c021cb875aaadec1d331584b Reviewed-on: https://review.coreboot.org/c/coreboot/+/52896 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Raul Rangel <rrangel@chromium.org> Reviewed-by: Marshall Dawson <marshalldawson3rd@gmail.com>
2021-04-30soc/amd/common: Move external oscillator config away from commonKarthikeyan Ramasubramanian
The usage of external oscillator has got nothing to do with Audio Co-processor (ACP). Hence move it out of common config and put it into the SoC config where it is being used. BUG=None TEST=Build Dalboz and Vilboz mainboards. Change-Id: I8c5d98addfba750f9ddb87a846599541b4a8340a Signed-off-by: Karthikeyan Ramasubramanian <kramasub@google.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/52771 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Felix Held <felix-coreboot@felixheld.de> Reviewed-by: Raul Rangel <rrangel@chromium.org>