Attention is currently required from: Raul Rangel, Martin Roth, Rob Barnes, Karthik Ramasubramanian, Felix Held. Robert Zieba has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61259 )
Change subject: soc/amd/cezanne: Turn off gpp clock request for disabled devices ......................................................................
Patch Set 25:
(3 comments)
File src/soc/amd/cezanne/fch.c:
https://review.coreboot.org/c/coreboot/+/61259/comment/666d1931_760ded85 PS23, Line 167: BIOS_WARNING
inside the fsp there is a special case for 0xff and that's likely where this is from, but yes, that […]
From looking at the fsp code 0xf in clk_req doesn't map to any internal enum value. So I've updated the code to properly warn in case somebody tries to force that value in there for whatever reason.
https://review.coreboot.org/c/coreboot/+/61259/comment/397ba2af_b6591be6 PS23, Line 177: if (clk_req == CLK_DISABLE) {
no, this isn't about the clock output to be disabled. […]
In addition to what Felix said about this being a valid configuration, I've looked at the fsp code and there doesn't seem to be anything that needs to be done in this case so I've just removed the warning.
https://review.coreboot.org/c/coreboot/+/61259/comment/407350fe_e733052b PS23, Line 192: 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; : } : : /* Check for mismatches between devices and SoC gpp_clk_config. */ : if (!pci_device->enabled && gpp_clk_config[gpp_req_index] != GPP_CLK_OFF) { : gpp_clk_config[gpp_req_index] = GPP_CLK_OFF; : : const struct fw_config *probe; : if (fw_config_probe_dev(pci_device, &probe)) { : /* : * fw_config probes don't touch the SoC gpp_clk_config. : * So there's only a mismatch if the device was disabled : * by something other than a probe. : */ : printk(BIOS_WARNING, : "GPP clk req mismatch: %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 { : 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 (pci_device->enabled : && gpp_clk_config[gpp_req_index] == GPP_CLK_OFF) { : printk(BIOS_WARNING, : "GPP clk req mismatch: %d.%d enabled, but GPP clk req is off, DXIO descriptor %d\n", : dxio_desc->device_number, dxio_desc->function_number, i); : }
Can you simplify this by just using is_devfn_enabled? is_devfn_enabled takes care of probing fw_conf […]
The CLK_DISABLE block was removed. In the remaining code is_devfn_enabled would not work because it needs to check how a PCIe device was disabled, not just if it's disabled in general.