From e2bde83a518d10fc6eaa7edca507daf271fdcf0a Mon Sep 17 00:00:00 2001 From: Robert Zieba Date: Wed, 19 Jan 2022 14:15:24 -0700 Subject: [PATCH] soc/amd/cezanne: Turn off gpp clock request for disabled devices The current behavior does not actually check if a device is present before enabling the corresponding gpp_clkx_clock_request_mapping bits which may cause issues with L1SS. This change sets the corresponding gpp_clkx_clock_request_mapping to off if the corresponding device is disabled. BUG=b:202252869 TEST=Checked that value of GPP_CLK_CNTRL matched the expected value when devices are enabled/disabled, checked that physically removing a device that is marked as enabled also disables the corresponding clk req BRANCH=guybrush Signed-off-by: Robert Zieba Change-Id: I77389372c60bdec572622a3b49484d4789fd4e4c Reviewed-on: https://review.coreboot.org/c/coreboot/+/61259 Reviewed-by: Felix Held Tested-by: build bot (Jenkins) --- src/mainboard/amd/majolica/Makefile.inc | 1 + src/mainboard/google/guybrush/Makefile.inc | 1 + src/soc/amd/cezanne/chip.h | 12 +-- src/soc/amd/cezanne/fch.c | 100 ++++++++++++++++++++- 4 files changed, 106 insertions(+), 8 deletions(-) diff --git a/src/mainboard/amd/majolica/Makefile.inc b/src/mainboard/amd/majolica/Makefile.inc index 23e9dd1b5e..59bc3cf0dd 100644 --- a/src/mainboard/amd/majolica/Makefile.inc +++ b/src/mainboard/amd/majolica/Makefile.inc @@ -6,6 +6,7 @@ bootblock-y += early_gpio.c romstage-y += port_descriptors.c ramstage-y += chromeos.c +ramstage-y += port_descriptors.c APCB_SOURCES = $(MAINBOARD_BLOBS_DIR)/APCB_CZN_D4.bin APCB_SOURCES_RECOVERY = $(MAINBOARD_BLOBS_DIR)/APCB_CZN_D4_DefaultRecovery.bin diff --git a/src/mainboard/google/guybrush/Makefile.inc b/src/mainboard/google/guybrush/Makefile.inc index cdfb53bd63..1158dcfaca 100644 --- a/src/mainboard/google/guybrush/Makefile.inc +++ b/src/mainboard/google/guybrush/Makefile.inc @@ -8,6 +8,7 @@ romstage-y += romstage.c ramstage-y += mainboard.c ramstage-y += ec.c +ramstage-y += port_descriptors.c ramstage-$(CONFIG_CHROMEOS) += chromeos.c all-y += spi_speeds.c diff --git a/src/soc/amd/cezanne/chip.h b/src/soc/amd/cezanne/chip.h index 1e59153ef5..b01c6c7fa8 100644 --- a/src/soc/amd/cezanne/chip.h +++ b/src/soc/amd/cezanne/chip.h @@ -12,6 +12,12 @@ #include #include +enum gpp_clk_req { + GPP_CLK_ON, /* GPP clock always on; default */ + GPP_CLK_REQ, /* GPP clock controlled by corresponding #CLK_REQx pin */ + GPP_CLK_OFF, /* GPP clk off */ +}; + struct soc_amd_cezanne_config { struct soc_amd_common_config common_config; u8 i2c_scl_reset; @@ -91,11 +97,7 @@ struct soc_amd_cezanne_config { /* The array index is the general purpose PCIe clock output number. Values in here aren't the values written to the register to have the default to be always on. */ - enum { - GPP_CLK_ON, /* GPP clock always on; default */ - GPP_CLK_REQ, /* GPP clock controlled by corresponding #CLK_REQx pin */ - GPP_CLK_OFF, /* GPP clk off */ - } gpp_clk_config[GPP_CLK_OUTPUT_COUNT]; + enum gpp_clk_req gpp_clk_config[GPP_CLK_OUTPUT_COUNT]; /* performance policy for the PCIe links: power consumption vs. link speed */ enum { diff --git a/src/soc/amd/cezanne/fch.c b/src/soc/amd/cezanne/fch.c index 2c57f08d96..b791037e0d 100644 --- a/src/soc/amd/cezanne/fch.c +++ b/src/soc/amd/cezanne/fch.c @@ -4,14 +4,19 @@ #include #include #include +#include #include #include #include #include -#include +#include +#include +#include +#include #include #include #include +#include #include #include #include "chip.h" @@ -129,10 +134,97 @@ static void fch_init_resets(void) pm_write16(PWR_RESET_CFG, pm_read16(PWR_RESET_CFG) | TOGGLE_ALL_PWR_GOOD); } -/* configure the general purpose PCIe clock outputs according to the devicetree settings */ +/* Update gpp glk req config based on DXIO descriptors and enabled devices. */ +static void gpp_dxio_update_clk_req_config(enum gpp_clk_req *gpp_clk_config, + size_t gpp_clk_config_num) +{ + const fsp_dxio_descriptor *dxio_descs = NULL; + const fsp_ddi_descriptor *ddi_descs = NULL; + size_t dxio_num = 0; + size_t ddi_num = 0; + + mainboard_get_dxio_ddi_descriptors(&dxio_descs, &dxio_num, &ddi_descs, &ddi_num); + if (dxio_descs == NULL) { + printk(BIOS_WARNING, + "No DXIO descriptors found, GPP clk req may not reflect enabled devices\n"); + return; + } + + for (int i = 0; i < dxio_num; i++) { + const fsp_dxio_descriptor *dxio_desc = &dxio_descs[i]; + + /* Only consider PCIe and unused engine types. */ + if (dxio_desc->engine_type != PCIE_ENGINE + && dxio_desc->engine_type != UNUSED_ENGINE) + continue; + enum cpm_clk_req dxio_clk_req = dxio_desc->clk_req; + + /* CLK_DISABLE means there's no corresponding clk req line in use */ + if (dxio_clk_req == CLK_DISABLE) + continue; + + /* + * dxio_clk_req is only 4 bits so having CLK_ENABLE as a value for + * a descriptor should cause a compiler error. 0xF isn't a + * valid clk_req value according to AMD's internal code either. + * This is here to draw attention in case this code is ever used + * in a situation where this has changed. + */ + if (dxio_clk_req == (CLK_ENABLE & 0xF)) { + printk(BIOS_WARNING, + "CLK_ENABLE is an invalid clk_req value for PCIe device %d.%d, DXIO descriptor %d\n", + dxio_desc->device_number, dxio_desc->function_number, i); + continue; + } + + /* cpm_clk_req 0 is CLK_DISABLE */ + int gpp_req_index = dxio_clk_req - CLK_REQ0; + /* Ensure that our index is valid */ + if (gpp_req_index < 0 || gpp_req_index >= gpp_clk_config_num) { + printk(BIOS_ERR, "Failed to convert DXIO clk req value %d to GPP clk req index for PCIe device %d.%d, DXIO descriptor %d, clk req settings may be incorrect\n", + dxio_clk_req, dxio_desc->device_number, + dxio_desc->function_number, i); + continue; + } + + const struct device *pci_device = pcidev_path_on_root( + PCI_DEVFN(dxio_desc->device_number, dxio_desc->function_number)); + if (pci_device == NULL) { + gpp_clk_config[gpp_req_index] = GPP_CLK_OFF; + printk(BIOS_WARNING, + "Cannot find PCIe device %d.%d, disabling GPP clk req %d, DXIO descriptor %d\n", + dxio_desc->device_number, dxio_desc->function_number, i, + gpp_req_index); + continue; + } + + /* PCIe devices haven't been fully set up yet, so directly read the vendor id + * and device id to determine if a device is physically present. If a device + * is not present then the id should be 0xffffffff. 0x00000000, 0xffff0000, + * and 0x0000ffff are there to account for any odd failure cases. */ + u32 id = pci_read_config32(pci_device, PCI_VENDOR_ID); + bool enabled = pci_device->enabled && (id != 0xffffffff) && + (id != 0x00000000) && (id != 0x0000ffff) && (id != 0xffff0000); + + /* Inform of possible mismatches between devices and SoC gpp_clk_config. */ + if (!enabled && gpp_clk_config[gpp_req_index] != GPP_CLK_OFF) { + gpp_clk_config[gpp_req_index] = GPP_CLK_OFF; + printk(BIOS_INFO, + "PCIe device %d.%d disabled, disabling GPP clk req %d, DXIO descriptor %d\n", + dxio_desc->device_number, dxio_desc->function_number, + gpp_req_index, i); + } else if (enabled && gpp_clk_config[gpp_req_index] == GPP_CLK_OFF) { + printk(BIOS_INFO, + "PCIe device %d.%d enabled, GPP clk req is off, DXIO descriptor %d\n", + dxio_desc->device_number, dxio_desc->function_number, i); + } + } +} + +/* Configure the general purpose PCIe clock outputs according to the devicetree settings */ static void gpp_clk_setup(void) { - const struct soc_amd_cezanne_config *cfg = config_of_soc(); + struct soc_amd_cezanne_config *cfg = config_of_soc(); /* look-up table to be able to iterate over the PCIe clock output settings */ const uint8_t gpp_clk_shift_lut[GPP_CLK_OUTPUT_COUNT] = { @@ -147,8 +239,10 @@ static void gpp_clk_setup(void) uint32_t gpp_clk_ctl = misc_read32(GPP_CLK_CNTRL); + gpp_dxio_update_clk_req_config(&cfg->gpp_clk_config[0], GPP_CLK_OUTPUT_COUNT); for (int i = 0; i < GPP_CLK_OUTPUT_COUNT; i++) { gpp_clk_ctl &= ~GPP_CLK_REQ_MASK(gpp_clk_shift_lut[i]); + /* * The remapping of values is done so that the default of the enum used for the * devicetree settings is the clock being enabled, so that a missing devicetree