Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36702 )
Change subject: intel/skylake: Use new PCIe RP devicetree update ......................................................................
intel/skylake: Use new PCIe RP devicetree update
The old code stumbled when the whole first group of root ports was disabled and also made the (sometimes wrong) assumption that FSP would only hide function 0 if we explicitly told it to disable it.
Change-Id: Ia6938ca6929c6d9d0293c4f0f0421e38bf53fb55 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/soc/intel/skylake/chip.c 1 file changed, 18 insertions(+), 122 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/36702/1
diff --git a/src/soc/intel/skylake/chip.c b/src/soc/intel/skylake/chip.c index 7987f46..2ca67d8 100644 --- a/src/soc/intel/skylake/chip.c +++ b/src/soc/intel/skylake/chip.c @@ -15,10 +15,8 @@
#include <bootmode.h> #include <bootstate.h> -#include <device/pci.h> #include <fsp/api.h> #include <arch/acpi.h> -#include <device/pci_ops.h> #include <console/console.h> #include <device/device.h> #include <device/pci_ids.h> @@ -27,6 +25,7 @@ #include <intelblocks/itss.h> #include <intelblocks/lpc_lib.h> #include <intelblocks/mp_init.h> +#include <intelblocks/pcie_rp.h> #include <intelblocks/xdci.h> #include <intelblocks/p2sb.h> #include <intelpch/lockdown.h> @@ -40,133 +39,27 @@ #include <soc/pci_devs.h> #include <soc/ramstage.h> #include <soc/systemagent.h> +#include <stdint.h> #include <string.h>
#include "chip.h"
-struct pcie_entry { - unsigned int devfn; - unsigned int func_count; +static const struct pcie_rp_group pch_lp_rp_groups[] = { + { .slot = PCH_DEV_SLOT_PCIE, .count = 8 }, + { .slot = PCH_DEV_SLOT_PCIE_1, .count = 4 }, + { 0 } };
-/* - * According to table 2-2 in doc#546717: - * PCI bus[function] ID - * D28:[F0 - F7] 0xA110 - 0xA117 - * D29:[F0 - F7] 0xA118 - 0xA11F - * D27:[F0 - F3] 0xA167 - 0xA16A - */ -static const struct pcie_entry pcie_table_skl_pch_h[] = { - {PCH_DEVFN_PCIE1, 8}, - {PCH_DEVFN_PCIE9, 8}, - {PCH_DEVFN_PCIE17, 4}, +static const struct pcie_rp_group pch_h_rp_groups[] = { + { .slot = PCH_DEV_SLOT_PCIE, .count = 8 }, + { .slot = PCH_DEV_SLOT_PCIE_1, .count = 8 }, + /* Sunrise Point PCH-H actually only has 4 ports in the + third group. But that would require a runtime check + and probing 4 non-existent ports shouldn't hurt. */ + { .slot = PCH_DEV_SLOT_PCIE_2, .count = 8 }, + { 0 } };
-/* - * According to table 2-2 in doc#564464: - * PCI bus[function] ID - * D28:[F0 - F7] 0xA290 - 0xA297 - * D29:[F0 - F7] 0xA298 - 0xA29F - * D27:[F0 - F7] 0xA2E7 - 0xA2EE - */ -static const struct pcie_entry pcie_table_kbl_pch_h[] = { - {PCH_DEVFN_PCIE1, 8}, - {PCH_DEVFN_PCIE9, 8}, - {PCH_DEVFN_PCIE17, 8}, -}; - -/* - * According to table 2-2 in doc#567995/545659: - * PCI bus[function] ID - * D28:[F0 - F7] 0x9D10 - 0x9D17 - * D29:[F0 - F3] 0x9D18 - 0x9D1B - */ -static const struct pcie_entry pcie_table_skl_pch_lp[] = { - {PCH_DEVFN_PCIE1, 8}, - {PCH_DEVFN_PCIE9, 4}, -}; - -/* - * If the PCIe root port at function 0 is disabled, - * the PCIe root ports might be coalesced after FSP silicon init. - * The below function will swap the devfn of the first enabled device - * in devicetree and function 0 resides a pci device - * so that it won't confuse coreboot. - */ -static void pcie_update_device_tree(const struct pcie_entry *pcie_rp_group, - size_t pci_groups) -{ - struct device *func0; - unsigned int devfn, devfn0; - int i, group; - unsigned int inc = PCI_DEVFN(0, 1); - - for (group = 0; group < pci_groups; group++) { - devfn0 = pcie_rp_group[group].devfn; - func0 = pcidev_path_on_root(devfn0); - if (func0 == NULL) - continue; - - /* No more functions if function 0 is disabled. */ - if (pci_read_config32(func0, PCI_VENDOR_ID) == 0xffffffff) - continue; - - devfn = devfn0 + inc; - - /* - * Increase function by 1. - * Then find first enabled device to replace func0 - * as that port was move to func0. - */ - for (i = 1; i < pcie_rp_group[group].func_count; - i++, devfn += inc) { - struct device *dev = pcidev_path_on_root(devfn); - if (dev == NULL || !dev->enabled) - continue; - - /* - * Found the first enabled device in - * a given dev number. - */ - printk(BIOS_INFO, "PCI func %d was swapped" - " to func 0.\n", i); - func0->path.pci.devfn = dev->path.pci.devfn; - dev->path.pci.devfn = devfn0; - break; - } - } -} - -static void pcie_override_devicetree_after_silicon_init(void) -{ - uint16_t id, id_mask; - - id = pci_read_config16(PCH_DEV_PCIE1, PCI_DEVICE_ID); - /* - * We may read an ID other than func 0 after FSP-S. - * Strip out 4 least significant bits. - */ - id_mask = id & ~0xf; - printk(BIOS_INFO, "Override DT after FSP-S, PCH is "); - if (id_mask == (PCI_DEVICE_ID_INTEL_SPT_LP_PCIE_RP1 & ~0xf)) { - printk(BIOS_INFO, "KBL/SKL PCH-LP SKU\n"); - pcie_update_device_tree(&pcie_table_skl_pch_lp[0], - ARRAY_SIZE(pcie_table_skl_pch_lp)); - } else if (id_mask == (PCI_DEVICE_ID_INTEL_KBP_H_PCIE_RP1 & ~0xf)) { - printk(BIOS_INFO, "KBL PCH-H SKU\n"); - pcie_update_device_tree(&pcie_table_kbl_pch_h[0], - ARRAY_SIZE(pcie_table_kbl_pch_h)); - } else if (id_mask == (PCI_DEVICE_ID_INTEL_SPT_H_PCIE_RP1 & ~0xf)) { - printk(BIOS_INFO, "SKL PCH-H SKU\n"); - pcie_update_device_tree(&pcie_table_skl_pch_h[0], - ARRAY_SIZE(pcie_table_skl_pch_h)); - } else { - printk(BIOS_ERR, "[BUG] PCIE Root Port id 0x%x" - " is not found\n", id); - return; - } -} - void soc_init_pre_device(void *chip_info) { /* Snapshot the current GPIO IRQ polarities. FSP is setting a @@ -187,7 +80,10 @@ itss_restore_irq_polarities(GPIO_IRQ_START, GPIO_IRQ_END);
/* swap enabled PCI ports in device tree if needed */ - pcie_override_devicetree_after_silicon_init(); + if (CONFIG(SKYLAKE_SOC_PCH_H)) + pcie_rp_update_devicetree(pch_h_rp_groups); + else + pcie_rp_update_devicetree(pch_lp_rp_groups); }
void soc_fsp_load(void)
Hello Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36702
to look at the new patch set (#2).
Change subject: intel/skylake: Use new PCIe RP devicetree update ......................................................................
intel/skylake: Use new PCIe RP devicetree update
The old code stumbled when the whole first group of root ports was disabled and also made the (sometimes wrong) assumption that FSP would only hide function 0 if we explicitly told it to disable it.
Change-Id: Ia6938ca6929c6d9d0293c4f0f0421e38bf53fb55 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/soc/intel/skylake/chip.c 1 file changed, 17 insertions(+), 122 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/36702/2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36702 )
Change subject: intel/skylake: Use new PCIe RP devicetree update ......................................................................
Patch Set 2: Code-Review+1
Tested on X11SSM-F with multiple configurations; looks good
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36702 )
Change subject: intel/skylake: Use new PCIe RP devicetree update ......................................................................
Patch Set 3: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36702 )
Change subject: intel/skylake: Use new PCIe RP devicetree update ......................................................................
Patch Set 3: Code-Review+1
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36702 )
Change subject: intel/skylake: Use new PCIe RP devicetree update ......................................................................
intel/skylake: Use new PCIe RP devicetree update
The old code stumbled when the whole first group of root ports was disabled and also made the (sometimes wrong) assumption that FSP would only hide function 0 if we explicitly told it to disable it.
Change-Id: Ia6938ca6929c6d9d0293c4f0f0421e38bf53fb55 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/36702 Reviewed-by: Michael Niewöhner Reviewed-by: Patrick Rudolph patrick.rudolph@9elements.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/skylake/chip.c 1 file changed, 17 insertions(+), 122 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, but someone else must approve Michael Niewöhner: Looks good to me, approved
diff --git a/src/soc/intel/skylake/chip.c b/src/soc/intel/skylake/chip.c index 7987f46..1e0803c 100644 --- a/src/soc/intel/skylake/chip.c +++ b/src/soc/intel/skylake/chip.c @@ -15,10 +15,8 @@
#include <bootmode.h> #include <bootstate.h> -#include <device/pci.h> #include <fsp/api.h> #include <arch/acpi.h> -#include <device/pci_ops.h> #include <console/console.h> #include <device/device.h> #include <device/pci_ids.h> @@ -27,6 +25,7 @@ #include <intelblocks/itss.h> #include <intelblocks/lpc_lib.h> #include <intelblocks/mp_init.h> +#include <intelblocks/pcie_rp.h> #include <intelblocks/xdci.h> #include <intelblocks/p2sb.h> #include <intelpch/lockdown.h> @@ -44,129 +43,22 @@
#include "chip.h"
-struct pcie_entry { - unsigned int devfn; - unsigned int func_count; +static const struct pcie_rp_group pch_lp_rp_groups[] = { + { .slot = PCH_DEV_SLOT_PCIE, .count = 8 }, + { .slot = PCH_DEV_SLOT_PCIE_1, .count = 4 }, + { 0 } };
-/* - * According to table 2-2 in doc#546717: - * PCI bus[function] ID - * D28:[F0 - F7] 0xA110 - 0xA117 - * D29:[F0 - F7] 0xA118 - 0xA11F - * D27:[F0 - F3] 0xA167 - 0xA16A - */ -static const struct pcie_entry pcie_table_skl_pch_h[] = { - {PCH_DEVFN_PCIE1, 8}, - {PCH_DEVFN_PCIE9, 8}, - {PCH_DEVFN_PCIE17, 4}, +static const struct pcie_rp_group pch_h_rp_groups[] = { + { .slot = PCH_DEV_SLOT_PCIE, .count = 8 }, + { .slot = PCH_DEV_SLOT_PCIE_1, .count = 8 }, + /* Sunrise Point PCH-H actually only has 4 ports in the + third group. But that would require a runtime check + and probing 4 non-existent ports shouldn't hurt. */ + { .slot = PCH_DEV_SLOT_PCIE_2, .count = 8 }, + { 0 } };
-/* - * According to table 2-2 in doc#564464: - * PCI bus[function] ID - * D28:[F0 - F7] 0xA290 - 0xA297 - * D29:[F0 - F7] 0xA298 - 0xA29F - * D27:[F0 - F7] 0xA2E7 - 0xA2EE - */ -static const struct pcie_entry pcie_table_kbl_pch_h[] = { - {PCH_DEVFN_PCIE1, 8}, - {PCH_DEVFN_PCIE9, 8}, - {PCH_DEVFN_PCIE17, 8}, -}; - -/* - * According to table 2-2 in doc#567995/545659: - * PCI bus[function] ID - * D28:[F0 - F7] 0x9D10 - 0x9D17 - * D29:[F0 - F3] 0x9D18 - 0x9D1B - */ -static const struct pcie_entry pcie_table_skl_pch_lp[] = { - {PCH_DEVFN_PCIE1, 8}, - {PCH_DEVFN_PCIE9, 4}, -}; - -/* - * If the PCIe root port at function 0 is disabled, - * the PCIe root ports might be coalesced after FSP silicon init. - * The below function will swap the devfn of the first enabled device - * in devicetree and function 0 resides a pci device - * so that it won't confuse coreboot. - */ -static void pcie_update_device_tree(const struct pcie_entry *pcie_rp_group, - size_t pci_groups) -{ - struct device *func0; - unsigned int devfn, devfn0; - int i, group; - unsigned int inc = PCI_DEVFN(0, 1); - - for (group = 0; group < pci_groups; group++) { - devfn0 = pcie_rp_group[group].devfn; - func0 = pcidev_path_on_root(devfn0); - if (func0 == NULL) - continue; - - /* No more functions if function 0 is disabled. */ - if (pci_read_config32(func0, PCI_VENDOR_ID) == 0xffffffff) - continue; - - devfn = devfn0 + inc; - - /* - * Increase function by 1. - * Then find first enabled device to replace func0 - * as that port was move to func0. - */ - for (i = 1; i < pcie_rp_group[group].func_count; - i++, devfn += inc) { - struct device *dev = pcidev_path_on_root(devfn); - if (dev == NULL || !dev->enabled) - continue; - - /* - * Found the first enabled device in - * a given dev number. - */ - printk(BIOS_INFO, "PCI func %d was swapped" - " to func 0.\n", i); - func0->path.pci.devfn = dev->path.pci.devfn; - dev->path.pci.devfn = devfn0; - break; - } - } -} - -static void pcie_override_devicetree_after_silicon_init(void) -{ - uint16_t id, id_mask; - - id = pci_read_config16(PCH_DEV_PCIE1, PCI_DEVICE_ID); - /* - * We may read an ID other than func 0 after FSP-S. - * Strip out 4 least significant bits. - */ - id_mask = id & ~0xf; - printk(BIOS_INFO, "Override DT after FSP-S, PCH is "); - if (id_mask == (PCI_DEVICE_ID_INTEL_SPT_LP_PCIE_RP1 & ~0xf)) { - printk(BIOS_INFO, "KBL/SKL PCH-LP SKU\n"); - pcie_update_device_tree(&pcie_table_skl_pch_lp[0], - ARRAY_SIZE(pcie_table_skl_pch_lp)); - } else if (id_mask == (PCI_DEVICE_ID_INTEL_KBP_H_PCIE_RP1 & ~0xf)) { - printk(BIOS_INFO, "KBL PCH-H SKU\n"); - pcie_update_device_tree(&pcie_table_kbl_pch_h[0], - ARRAY_SIZE(pcie_table_kbl_pch_h)); - } else if (id_mask == (PCI_DEVICE_ID_INTEL_SPT_H_PCIE_RP1 & ~0xf)) { - printk(BIOS_INFO, "SKL PCH-H SKU\n"); - pcie_update_device_tree(&pcie_table_skl_pch_h[0], - ARRAY_SIZE(pcie_table_skl_pch_h)); - } else { - printk(BIOS_ERR, "[BUG] PCIE Root Port id 0x%x" - " is not found\n", id); - return; - } -} - void soc_init_pre_device(void *chip_info) { /* Snapshot the current GPIO IRQ polarities. FSP is setting a @@ -187,7 +79,10 @@ itss_restore_irq_polarities(GPIO_IRQ_START, GPIO_IRQ_END);
/* swap enabled PCI ports in device tree if needed */ - pcie_override_devicetree_after_silicon_init(); + if (CONFIG(SKYLAKE_SOC_PCH_H)) + pcie_rp_update_devicetree(pch_h_rp_groups); + else + pcie_rp_update_devicetree(pch_lp_rp_groups); }
void soc_fsp_load(void)
-
Michael Niewöhner (Code Review) -
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)