Hello Arthur Heymans, Michael Niewöhner,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to review the following change.
Change subject: [WIP] intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
[WIP] intel/skylake: Implement PCIe RP devicetree update based on DID
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/soc/intel/skylake/Makefile.inc M src/soc/intel/skylake/chip_fsp20.c A src/soc/intel/skylake/include/soc/pcie_rp.h A src/soc/intel/skylake/pcie_rp.c 4 files changed, 159 insertions(+), 127 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/1
diff --git a/src/soc/intel/skylake/Makefile.inc b/src/soc/intel/skylake/Makefile.inc index ef741f8..691bdbc 100644 --- a/src/soc/intel/skylake/Makefile.inc +++ b/src/soc/intel/skylake/Makefile.inc @@ -57,6 +57,7 @@ ramstage-y += me.c ramstage-y += memmap.c ramstage-y += p2sb.c +ramstage-y += pcie_rp.c ramstage-y += pmc.c ramstage-y += pmutil.c ramstage-$(CONFIG_PLATFORM_USES_FSP2_0) += reset.c diff --git a/src/soc/intel/skylake/chip_fsp20.c b/src/soc/intel/skylake/chip_fsp20.c index 55fedd3..83e3be5 100644 --- a/src/soc/intel/skylake/chip_fsp20.c +++ b/src/soc/intel/skylake/chip_fsp20.c @@ -15,13 +15,10 @@
#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> #include <fsp/util.h> #include <intelblocks/cfg.h> #include <intelblocks/itss.h> @@ -37,135 +34,13 @@ #include <soc/irq.h> #include <soc/itss.h> #include <soc/pci_devs.h> +#include <soc/pcie_rp.h> #include <soc/ramstage.h> #include <soc/systemagent.h> #include <string.h>
#include "chip.h"
-struct pcie_entry { - unsigned int devfn; - unsigned int func_count; -}; - -/* - * 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}, -}; - -/* - * 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 @@ -179,7 +54,7 @@ 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(); + pcie_rp_update_devicetree(); }
void soc_fsp_load(void) diff --git a/src/soc/intel/skylake/include/soc/pcie_rp.h b/src/soc/intel/skylake/include/soc/pcie_rp.h new file mode 100644 index 0000000..2bc7095 --- /dev/null +++ b/src/soc/intel/skylake/include/soc/pcie_rp.h @@ -0,0 +1,19 @@ +/* + * This file is part of the coreboot project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef SOC_INTEL_SKYLAKE_PCIE_RP_H +#define SOC_INTEL_SKYLAKE_PCIE_RP_H + +void pcie_rp_update_devicetree(void); + +#endif /* SOC_INTEL_SKYLAKE_PCIE_RP_H */ diff --git a/src/soc/intel/skylake/pcie_rp.c b/src/soc/intel/skylake/pcie_rp.c new file mode 100644 index 0000000..5882dea --- /dev/null +++ b/src/soc/intel/skylake/pcie_rp.c @@ -0,0 +1,137 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Nico Huber nico.h@gmx.de + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <stdint.h> + +#include <console/console.h> +#include <device/device.h> +#include <device/pci_def.h> +#include <device/pci_ids.h> +#include <device/pci_ops.h> +#include <device/pci_type.h> +#include <soc/pcie_rp.h> + +/* negative if disabled, new function number otherwise */ +static int pcie_rp_fn_mapping[24]; + +/* The original DEVFN in the `devicetree.cb` is supposed to map directly to RPx. */ +static int pcie_rp_idx_from_devfn(const struct device *const dev) +{ + int rp_idx = PCI_FUNC(dev->path.pci.devfn); + switch (PCI_SLOT(dev->path.pci.devfn)) { + case 0x1c: + break; + case 0x1d: + rp_idx += 8; + break; + case 0x1b: + rp_idx += 16; + break; + default: + printk(BIOS_WARNING, + "Unexpected DEVFN for PCIe Root Port: %s.\n", + dev_path(dev)); + rp_idx = -1; + } + + return rp_idx; +} + +/* Check which RP (identified by its DID) has the given DEVFN assigned now. */ +static int pcie_rp_idx_from_did(const pci_devfn_t dev) +{ + unsigned int rp_idx; + + const uint16_t did = pci_s_read_config16(dev, PCI_DEVICE_ID); + + switch (did & ~0xf) { + case PCI_DEVICE_ID_INTEL_SPT_LP_PCIE_RP1: /* Fall through. */ + case PCI_DEVICE_ID_INTEL_SPT_H_PCIE_RP1: /* Fall through. */ + case PCI_DEVICE_ID_INTEL_KBP_H_PCIE_RP1: + rp_idx = did & 0xf; + break; + /* DIDs in the third groups on PCH-H are not aligned: */ + case PCI_DEVICE_ID_INTEL_SPT_H_PCIE_RP17: + rp_idx = 16 + did - PCI_DEVICE_ID_INTEL_SPT_H_PCIE_RP17; + break; + case PCI_DEVICE_ID_INTEL_KBP_H_PCIE_RP17: + rp_idx = 16 + did - PCI_DEVICE_ID_INTEL_KBP_H_PCIE_RP17; + break; + default: + printk(BIOS_WARNING, + "Unexpected DID for PCIe Root Port %02x.%x: %04x.\n", + PCI_SLOT(PCI_DEV2DEVFN(dev)), PCI_FUNC(PCI_DEV2DEVFN(dev)), did); + /* Fall through. */ + case 0xffff: + rp_idx = -1; + } + + return rp_idx; +} + +static void pcie_rp_register_group(const unsigned int devnum) +{ + unsigned int fn; + + /* Reconstruct current mapping */ + for (fn = 0; fn < 7; ++fn) { + const int rp_idx = pcie_rp_idx_from_did(PCI_DEV(0, devnum, fn)); + if (rp_idx < 0) { + if (fn == 0) + break; + else + continue; + } + pcie_rp_fn_mapping[rp_idx] = fn; + } +} + +static void pcie_rp_update_dev(struct device *const dev) +{ + const int rp_idx = pcie_rp_idx_from_devfn(dev); + if (rp_idx < 0) + return; + + const int fn = pcie_rp_fn_mapping[rp_idx]; + if (fn < 0) + dev->enabled = 0; + else + dev->path.pci.devfn = PCI_DEVFN(PCI_SLOT(dev->path.pci.devfn), fn); +} + +void pcie_rp_update_devicetree(void) +{ + const struct bus *const root = pci_root_bus(); + if (!root) + return; + + pcie_rp_register_group(0x1c); + pcie_rp_register_group(0x1d); + if (CONFIG(SKYLAKE_SOC_PCH_H)) + pcie_rp_register_group(0x1b); + + struct device *dev; + for (dev = root->children; dev; dev = dev->sibling) { + if (dev->path.type != DEVICE_PATH_PCI) + continue; + + if (PCI_SLOT(dev->path.pci.devfn) != 0x1c && + PCI_SLOT(dev->path.pci.devfn) != 0x1d && + (!CONFIG(SKYLAKE_SOC_PCH_H) || PCI_SLOT(dev->path.pci.devfn) != 0x1b)) + continue; + + pcie_rp_update_dev(dev); + } +}
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: [WIP] intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/1/src/soc/intel/skylake/pcie_... File src/soc/intel/skylake/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/1/src/soc/intel/skylake/pcie_... PS1, Line 94: else else is not generally useful after a break or return
Hello Patrick Rudolph, Arthur Heymans, Michael Niewöhner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#2).
Change subject: [WIP] intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
[WIP] intel/skylake: Implement PCIe RP devicetree update based on DID
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.
This, new implementation acts solely on device IDs read with pci_s_read_config16(). In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, and update the device paths.
In theory, this should work no matter who (coreboot vs. FSP) reordered the root ports and no matter what rules were used.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c M src/soc/intel/skylake/chip_fsp20.c 4 files changed, 179 insertions(+), 118 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: [WIP] intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/2/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/2/src/soc/intel/common/block/... PS2, Line 33: void pcie_rp_update_devicetree(const struct pcie_rp_mapper *); function definition argument 'const struct pcie_rp_mapper *' should also have an identifier name
Hello Patrick Rudolph, Arthur Heymans, Michael Niewöhner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#3).
Change subject: [WIP] intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
[WIP] intel/skylake: Implement PCIe RP devicetree update based on DID
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.
This, new implementation acts solely on device IDs read with pci_s_read_config16(). In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, and update the device paths.
In theory, this should work no matter who (coreboot vs. FSP) reordered the root ports and no matter what rules were used.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c M src/soc/intel/skylake/chip_fsp20.c 4 files changed, 179 insertions(+), 118 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: [WIP] intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/3/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/3/src/soc/intel/common/block/... PS3, Line 33: void pcie_rp_update_devicetree(const struct pcie_rp_mapper *); function definition argument 'const struct pcie_rp_mapper *' should also have an identifier name
Hello Patrick Rudolph, Arthur Heymans, Michael Niewöhner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#4).
Change subject: [WIP] intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
[WIP] intel/skylake: Implement PCIe RP devicetree update based on DID
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.
This, new implementation acts solely on device IDs read with pci_s_read_config16(). In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, and update the device paths.
In theory, this should work no matter who (coreboot vs. FSP) reordered the root ports and no matter what rules were used.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c M src/soc/intel/skylake/chip_fsp20.c 4 files changed, 179 insertions(+), 118 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: [WIP] intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/4/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/4/src/soc/intel/common/block/... PS4, Line 33: void pcie_rp_update_devicetree(const struct pcie_rp_mapper *); function definition argument 'const struct pcie_rp_mapper *' should also have an identifier name
Hello Patrick Rudolph, Arthur Heymans, Michael Niewöhner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#5).
Change subject: [WIP] intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
[WIP] intel/skylake: Implement PCIe RP devicetree update based on DID
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.
This, new implementation acts solely on device IDs read with pci_s_read_config16(). In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, and update the device paths.
In theory, this should work no matter who (coreboot vs. FSP) reordered the root ports and no matter what rules were used.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c M src/soc/intel/skylake/chip_fsp20.c 4 files changed, 179 insertions(+), 118 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: [WIP] intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/5/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/5/src/soc/intel/common/block/... PS5, Line 33: void pcie_rp_update_devicetree(const struct pcie_rp_mapper *); function definition argument 'const struct pcie_rp_mapper *' should also have an identifier name
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: [WIP] intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35985/5/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35985/5/src/soc/intel/skylake/chip_... PS5, Line 67: default: : printk(BIOS_WARNING, : "%s: Unexpected DID for PCIe Root Port at PCI %x:%02x.%x: %04x.\n", : __func__, PCI_DEV2SEGBUS(dev), PCI_SLOT(PCI_DEV2DEVFN(dev)), : PCI_FUNC(PCI_DEV2DEVFN(dev)), did); this will warn for disabled ports. I don't think we want that. move down and break in case 0xfff0
https://review.coreboot.org/c/coreboot/+/35985/5/src/soc/intel/skylake/chip_... PS5, Line 73: 0xffff 0xfff0
Hello Patrick Rudolph, Arthur Heymans, Michael Niewöhner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#6).
Change subject: [WIP] intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
[WIP] intel/skylake: Implement PCIe RP devicetree update based on DID
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.
This, new implementation acts solely on device IDs read with pci_s_read_config16(). In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, and update the device paths.
In theory, this should work no matter who (coreboot vs. FSP) reordered the root ports and no matter what rules were used.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c M src/soc/intel/skylake/chip_fsp20.c 4 files changed, 179 insertions(+), 118 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: [WIP] intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/6/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/6/src/soc/intel/common/block/... PS6, Line 33: void pcie_rp_update_devicetree(const struct pcie_rp_mapper *); function definition argument 'const struct pcie_rp_mapper *' should also have an identifier name
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: [WIP] intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35985/5/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35985/5/src/soc/intel/skylake/chip_... PS5, Line 67: default: : printk(BIOS_WARNING, : "%s: Unexpected DID for PCIe Root Port at PCI %x:%02x.%x: %04x.\n", : __func__, PCI_DEV2SEGBUS(dev), PCI_SLOT(PCI_DEV2DEVFN(dev)), : PCI_FUNC(PCI_DEV2DEVFN(dev)), did);
this will warn for disabled ports. I don't think we want that. […]
Done
https://review.coreboot.org/c/coreboot/+/35985/5/src/soc/intel/skylake/chip_... PS5, Line 73: 0xffff
0xfff0
Done
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: [WIP] intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 6: Code-Review+1
tested successfully on x11ssm-f
Hello Patrick Rudolph, Arthur Heymans, Michael Niewöhner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#7).
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
intel/skylake: Implement PCIe RP devicetree update based on DID
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.
This, new implementation acts solely on device IDs read with pci_s_read_config16(). In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, and update the device paths.
In theory, this should work no matter who (coreboot vs. FSP) reordered the root ports and no matter what rules were used.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c M src/soc/intel/skylake/chip_fsp20.c 4 files changed, 179 insertions(+), 118 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/7
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Arthur Heymans, Michael Niewöhner, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#8).
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
intel/skylake: Implement PCIe RP devicetree update based on DID
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.
This, new implementation acts solely on device IDs read with pci_s_read_config16(). In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, and update the device paths.
In theory, this should work no matter who (coreboot vs. FSP) reordered the root ports and no matter what rules were used.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c M src/soc/intel/skylake/chip_fsp20.c 4 files changed, 181 insertions(+), 119 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/8
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/8/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/8/src/soc/intel/common/block/... PS8, Line 33: void pcie_rp_update_devicetree(const struct pcie_rp_mapper *); function definition argument 'const struct pcie_rp_mapper *' should also have an identifier name
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Arthur Heymans, Michael Niewöhner, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#9).
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
intel/skylake: Implement PCIe RP devicetree update based on DID
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.
This, new implementation acts solely on device IDs read with pci_s_read_config16(). In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, and update the device paths.
In theory, this should work no matter who (coreboot vs. FSP) reordered the root ports and no matter what rules were used.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c M src/soc/intel/skylake/chip_fsp20.c 4 files changed, 182 insertions(+), 119 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/9
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/9/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/9/src/soc/intel/common/block/... PS9, Line 33: void pcie_rp_update_devicetree(const struct pcie_rp_mapper *); function definition argument 'const struct pcie_rp_mapper *' should also have an identifier name
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Arthur Heymans, Michael Niewöhner, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#10).
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
intel/skylake: Implement PCIe RP devicetree update based on DID
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.
This, new implementation acts solely on device IDs read with pci_s_read_config16(). In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, and update the device paths.
In theory, this should work no matter who (coreboot vs. FSP) reordered the root ports and no matter what rules were used.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c M src/soc/intel/skylake/chip_fsp20.c 4 files changed, 182 insertions(+), 119 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/10
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 33: void pcie_rp_update_devicetree(const struct pcie_rp_mapper *); function definition argument 'const struct pcie_rp_mapper *' should also have an identifier name
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 10: Code-Review+1
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 10:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 21: unsigned int offset; It's not clear what these fields represent.
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 25: /* Zero-terminated list */ Zero in what field? Looks like it's 'slot'. I think we should be more explicit in the comments.
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 34: Could you please add a description?
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 32: < Shouldn't this be <= ? There are 8 functions possible, right?
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 79: 24 why 24?
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 88: group->offset + 8 Could you please explain this check?
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Arthur Heymans, Michael Niewöhner, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#11).
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
intel/skylake: Implement PCIe RP devicetree update based on DID
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.
This, new implementation acts solely on device IDs read with pci_s_read_config16(). In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, and update the device paths.
In theory, this should work no matter who (coreboot vs. FSP) reordered the root ports and no matter what rules were used.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c M src/soc/intel/skylake/chip_fsp20.c 4 files changed, 232 insertions(+), 119 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/11
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 32: <
Shouldn't this be <= ? There are 8 functions possible, right?
Ack
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 79: 24
why 24?
there's a maximum of 24 ports depending on SKU
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/common/block... PS11, Line 56: void pcie_rp_update_devicetree(const struct pcie_rp_mapper *); function definition argument 'const struct pcie_rp_mapper *' should also have an identifier name
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 11:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 21: unsigned int offset;
It's not clear what these fields represent.
Done
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 25: /* Zero-terminated list */
Zero in what field? Looks like it's 'slot'. I think we should be more explicit in the comments.
Done
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 34:
Could you please add a description?
Done
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 32: <
Ack
Thanks!
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 79: 24
there's a maximum of 24 ports depending on SKU
Done
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 88: group->offset + 8
Could you please explain this check?
I hope the new version explains itself. Let me know if not.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 11: Code-Review+1
latest patchset tested on x11ssm-f
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 79: 24
Done
From Document Number: 335192-003: "PCH-H supports up to 16 PCIe* Ports and 24 PCIe* Lanes" - only 16 ports can be used
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 79: 24
From Document Number: 335192-003: […]
All that matters is that this PCH has 24 individual PCI device functions for root ports. If they can be enabled at once? we don't have to care.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... PS11, Line 77: mapper mapping? or map?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 11:
(6 comments)
Sorry for the delay. Trying to catch up from being out of town. I think the algorithm is a lot clearer now. However, I did provide a suggestion that is completely data driven w/o the need for a callback.
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/common/block... PS11, Line 39: to their original function Presumably this is returning the original function number? Can we make that clearer?
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/common/block... PS11, Line 54: * enumeration. How do we handle the mapping to the ACPI device names?
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... PS11, Line 51: case PCI_DEVICE_ID_INTEL_SPT_LP_PCIE_RP1: /* Fall through. */ I feel like we're being too slick here.
#define PCI_DEVICE_ID_INTEL_SPT_LP_PCIE_RP9>---->-------0x9d18
Masking off the lower nibble matches PCI_DEVICE_ID_INTEL_SPT_LP_PCIE_RP1. Can we just assume the compiler can optimize correctly if we blew out the switch statement to be more exhaustive?
Or if we passed in the slot number wouldn't that be sufficient?
Another idea:
Why encode these in code itself. We could just as easily add a array of 8 dids per pcie_rp_group. The index of the did match == original function number. And there's no callback required. We only need to encode the did array per group and generic code can handle everything else.
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... PS11, Line 72: { PCH_DEV_SLOT_PCIE, .offset = 0, .count = 8 }, For consistency can we please add '.slot = ' ?
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... PS11, Line 74: 0x00, 0, 0, I think you can just put '0' on its own.
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... PS11, Line 92: const struct pcie_rp_mapper pch_h_rp_mapper = { Any reason the pcie_rp_mapper ad pcie_rp_group objects are global?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... PS11, Line 51: case PCI_DEVICE_ID_INTEL_SPT_LP_PCIE_RP1: /* Fall through. */
I feel like we're being too slick here. […]
Oh... was nearly through implementing the array idea, then realized that it brings us back to one of the original problems: we'd have to know in advance which PCH we have. But it should still be doable with a more explicit mapping (i.e. no implicit index == function number). I'll test how it looks like.
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Arthur Heymans, Michael Niewöhner, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#12).
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
intel/skylake: Implement PCIe RP devicetree update based on DID
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.
This, new implementation acts solely on device IDs read with pci_s_read_config16(). In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, and update the device paths.
In theory, this should work no matter who (coreboot vs. FSP) reordered the root ports and no matter what rules were used.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c M src/soc/intel/skylake/chip_fsp20.c 4 files changed, 283 insertions(+), 115 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/12
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Arthur Heymans, Michael Niewöhner, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#13).
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
intel/skylake: Implement PCIe RP devicetree update based on DID
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.
This, new implementation acts solely on device IDs read with pci_s_read_config16(). In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, and update the device paths.
In theory, this should work no matter who (coreboot vs. FSP) reordered the root ports and no matter what rules were used.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c M src/soc/intel/skylake/chip.c 4 files changed, 283 insertions(+), 115 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/13
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 13:
(3 comments)
Current version implements long tables instead of C code that derives function numbers.
Originally, I thought the only negative impact would be on the diff stat. But after writing all the lines... I think it's more error-prone this way. Long, dull tables to write and to review. However, I'm undecided. If somebody promises to carefully review the tables, that should be fine :)
Let me know what you think. I've left some comments unresolved in case we want to go back to the C code.
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... PS11, Line 72: { PCH_DEV_SLOT_PCIE, .offset = 0, .count = 8 },
For consistency can we please add '. […]
Done
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... PS11, Line 74: 0x00, 0, 0,
I think you can just put '0' on its own.
Done
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... PS11, Line 92: const struct pcie_rp_mapper pch_h_rp_mapper = {
Any reason the pcie_rp_mapper ad pcie_rp_group objects are global?
None
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 13:
I've pushed the rebase separately, so the interesting diff is 11 -> 12.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... PS11, Line 51: case PCI_DEVICE_ID_INTEL_SPT_LP_PCIE_RP1: /* Fall through. */
Oh... was nearly through implementing the array idea, then realized that […]
This feels like artificially blowing up the code... OTOH the new approach is less error-prone and more generic
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... PS11, Line 77: mapper
mapping? or map?
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/common/block... PS11, Line 54: * enumeration.
How do we handle the mapping to the ACPI device names?
Looks like we map the final BDF to RP01..16 (don't ask me why 16, I don't know any chip supported by this code that has exactly 16 ports). However looking through the ASL code, I found a gimmick: it reads the port number from LCAP. AIUI, that should give us the original function number (off by one). I'll investigate further.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 14:
Need to update the commit message, the implementation with LCAP works \o/
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 27: It saves us a walk through the capabilites list per device.*/ 'capabilites' may be misspelled - perhaps 'capabilities'?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 14:
(3 comments)
Patch Set 14:
Need to update the commit message, the implementation with LCAP works \o/
Good work! I really like this new implementation
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 27: It saves us a walk through the capabilites list per device.*/
'capabilites' may be misspelled - perhaps 'capabilities'?
^
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 49: offset = 8, .count = 4 I didn't look into the new implementation, yet, but if we can't drop this? non-existing ports should return 0xffff anyways, so the count may be unneeded. I'm not sure about the offset, though. Will take a deeper look tomorrow
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 83: if (CONFIG(SKYLAKE_SOC_PCH_H)) : pcie_rp_update_devicetree(pch_h_rp_groups); : else : pcie_rp_update_devicetree(pch_lp_rp_groups); : same as above; maybe we can throw all possible groups in one list... non-existent ports would be skipped anyways (needs proof :) )
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 49: offset = 8, .count = 4
I didn't look into the new implementation, yet, but if we can't drop this? non-existing ports should […]
Well, I guess the count could be dropped if we assume to always have 8 ports per group... I don't know if that is spec or Intel specific. The same applies to the offset; simply count up by 8 in pcie_rp_update_devicetree() would make this unneeded.
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 83: if (CONFIG(SKYLAKE_SOC_PCH_H)) : pcie_rp_update_devicetree(pch_h_rp_groups); : else : pcie_rp_update_devicetree(pch_lp_rp_groups); :
same as above; maybe we can throw all possible groups in one list... […]
Not sure how that could be realized... I don't think it's really worth it :)
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
Patch Set 14:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 28: #define INTEL_RP_CLIST_XCAP 0x40 Individual PCIe capabilities aren't guaranteed to be at the same offset. Do we want to maintain this assumption?
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 44: /* Read 1-based absolute port number: */ Could you expand this comment indicating the original port number is in this field of the capabilities? We're inherently relying on that to hold true for this scheme to work. I think it could use an expanded comment to that effect.
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 47: port_num 1-based comment above means it starts at 1 and counts up? Wouldn't this check be invalid because offset is 0 based? e.g. the 2nd group would be offfset 8. But the first root port as read out of the capability field would be 9 ?
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 97: "Remapping PCIe Root Port #%u from %s to new function number %u.\n", Is it helpful to have the old function number in this message?
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Arthur Heymans, Michael Niewöhner, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#15).
Change subject: intel/skylake: Implement PCIe RP devicetree update based on DID ......................................................................
intel/skylake: Implement PCIe RP devicetree update based on DID
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.
This, new implementation acts solely on device IDs read with pci_s_read_config16(). In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, and update the device paths.
In theory, this should work no matter who (coreboot vs. FSP) reordered the root ports and no matter what rules were used.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/include/device/pci_def.h A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c M src/soc/intel/skylake/chip.c 5 files changed, 219 insertions(+), 122 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/15
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Arthur Heymans, Michael Niewöhner, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#16).
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
intel/skylake: Implement PCIe RP devicetree update based on LCAP
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. We don't have to make assumptions though, and can read the root-port number from the PCIe link capapilities (LCAP) instead. This is also what we do in ASL code for years already.
This new implementation acts solely on information read from the PCI config space. In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, and update the device paths.
In theory, this should work no matter who (coreboot vs. FSP) reordered the root ports and no matter what rules were used.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/include/device/pci_def.h A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c M src/soc/intel/skylake/chip.c 5 files changed, 219 insertions(+), 122 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/16
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 16:
(11 comments)
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/common/block... PS11, Line 39: to their original function
Presumably this is returning the original function number? Can we make that clearer?
n/a
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/common/block... PS11, Line 54: * enumeration.
Looks like we map the final BDF to RP01..16 (don't ask me why 16, I don't know […]
Done
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/10/src/soc/intel/common/block... PS10, Line 88: group->offset + 8
I hope the new version explains itself. Let me know if not.
n/a
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 27: It saves us a walk through the capabilites list per device.*/
^
Done
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 28: #define INTEL_RP_CLIST_XCAP 0x40
Individual PCIe capabilities aren't guaranteed to be at the same offset. […]
No guarantee, I know. But this is vendor specific code anyway. I think the assumption is fair and saves us a handful of PCI config reads per device.
Though, now that I have looked and found pci_s_find_capability() ;) At least I wouldn't have to write the code...
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 44: /* Read 1-based absolute port number: */
Could you expand this comment indicating the original port number is in this field of the capabiliti […]
Done
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 47: port_num
1-based comment above means it starts at 1 and counts up? Wouldn't this check be invalid because off […]
Seems correct to me,
if (port_num <= 8) bail out; if (port_num > 16) bail out;
works for 9 :)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 97: "Remapping PCIe Root Port #%u from %s to new function number %u.\n",
Is it helpful to have the old function number in this message?
Why not?
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 49: offset = 8, .count = 4
Well, I guess the count could be dropped if we assume to always have 8 ports per group... […]
Dropped `offset`
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 83: if (CONFIG(SKYLAKE_SOC_PCH_H)) : pcie_rp_update_devicetree(pch_h_rp_groups); : else : pcie_rp_update_devicetree(pch_lp_rp_groups); :
Not sure how that could be realized... […]
Hmmm, it's really just 5 additional PCI-config reads. But we'd always have to assume that the additional PCI-device numbers are not used for something other than RPs...
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35985/11/src/soc/intel/skylake/chip... PS11, Line 51: case PCI_DEVICE_ID_INTEL_SPT_LP_PCIE_RP1: /* Fall through. */
This feels like artificially blowing up the code... […]
n/a
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Arthur Heymans, Michael Niewöhner, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#17).
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
intel/skylake: Implement PCIe RP devicetree update based on LCAP
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. We don't have to make assumptions though, and can read the root-port number from the PCIe link capapilities (LCAP) instead. This is also what we do in ASL code for years already.
This new implementation acts solely on information read from the PCI config space. In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, and update the device paths.
In theory, this should work no matter who (coreboot vs. FSP) reordered the root ports and no matter what rules were used.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/include/device/pci_def.h A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c M src/soc/intel/skylake/chip.c 5 files changed, 219 insertions(+), 122 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/17
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 28: #define INTEL_RP_CLIST_XCAP 0x40
No guarantee, I know. But this is vendor specific code anyway. I think […]
PS#17 is the alternative
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 17:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 49: offset = 8, .count = 4
Dropped `offset`
curious, do you know if 8 ports is spec or intel? what do you think about assuming it's always 8?
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 83: if (CONFIG(SKYLAKE_SOC_PCH_H)) : pcie_rp_update_devicetree(pch_h_rp_groups); : else : pcie_rp_update_devicetree(pch_lp_rp_groups); :
Hmmm, it's really just 5 additional PCI-config reads. But we'd always […]
yeah, I've been thinking about that assumption, too. Since the list is at SoC level I would guess we won't ever have a problem here. At least for current SoCs the port IDs do not overlap / have another meaning irt H vs. LP.
For PCH-H you already have that set the count from 4 to 8. So when this is done for PCH-LP, too, we only have one difference: the additional group. So it would be easy to have exactly one list (actually that would be the PCH-H one).
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 83: if (CONFIG(SKYLAKE_SOC_PCH_H)) : pcie_rp_update_devicetree(pch_h_rp_groups); : else : pcie_rp_update_devicetree(pch_lp_rp_groups); :
yeah, I've been thinking about that assumption, too. […]
Hah! I found something, what about checking for "Port Type (DT)" in XCAP? That could be done for each first port per group for example
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 17:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 28: #define INTEL_RP_CLIST_XCAP 0x40
PS#17 is the alternative
Understood it's vendor code. I was just meaning it the capability offsets could change even with intel's implementations.
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 47: port_num
Seems correct to me, […]
That's fair. I think I was reading it as a positive as opposed to an error.
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 97: "Remapping PCIe Root Port #%u from %s to new function number %u.\n",
Why not?
I was meaning should we add it to the message.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 49: offset = 8, .count = 4
curious, do you know if 8 ports is spec or intel? what do you think about assuming it's always 8?
There's only pci functions under a slot.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 17:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 28: #define INTEL_RP_CLIST_XCAP 0x40
Understood it's vendor code. […]
I don't think the extra config reads hurt. So let's keep the pci_s_find_capability(). Now that it's written, I kind of like it.
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 47: port_num
That's fair. I think I was reading it as a positive as opposed to an error.
Ack
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/common/block... PS14, Line 97: "Remapping PCIe Root Port #%u from %s to new function number %u.\n",
I was meaning should we add it to the message.
Gotcha. The original function number is part of the `dev_path(dev)`, that is still unpatched.
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 49: offset = 8, .count = 4
There's only pci functions under a slot.
Right, 8 is the maximum per PCI device. It's not always 8, though. APL, for instance, has 4 + 2 groups.
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 83: if (CONFIG(SKYLAKE_SOC_PCH_H)) : pcie_rp_update_devicetree(pch_h_rp_groups); : else : pcie_rp_update_devicetree(pch_lp_rp_groups); : Well, we know that LP doesn't have 1b.*, so it's safe. One version is more explicit, the other saves us a few lines. I have no strong preference.
Hah! I found something, what about checking for "Port Type (DT)" in XCAP? That could be done for each first port per group for example
We already do that for every port. If it wasn't set to root port, it would we invalid to read LCAP :)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/17/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/17/src/soc/intel/common/block... PS17, Line 102: dev->enabled = 0; I thought about unlinking these nodes from the bus. And now I'm convinced that this is even necessary (if we don't make assumptions about the reordering) because we don't know a new fn that we could assign.
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Arthur Heymans, Michael Niewöhner, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#18).
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
intel/skylake: Implement PCIe RP devicetree update based on LCAP
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. We don't have to make assumptions though, and can read the root-port number from the PCIe link capapilities (LCAP) instead. This is also what we do in ASL code for years already.
This new implementation acts solely on information read from the PCI config space. In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, and update the device paths.
In theory, this should work no matter who (coreboot vs. FSP) reordered the root ports and no matter what rules were used.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/include/device/pci_def.h A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c M src/soc/intel/skylake/chip.c 5 files changed, 233 insertions(+), 122 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/18
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 18:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 160: *ptr = dev->sibling; Is the idea to prev_dev->sibling = dev->sibling; and set dev = NULL;?
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 161: dev->sibling dev = NULL; ?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 83: if (CONFIG(SKYLAKE_SOC_PCH_H)) : pcie_rp_update_devicetree(pch_h_rp_groups); : else : pcie_rp_update_devicetree(pch_lp_rp_groups); :
We already do that for every port. If it wasn't set to root port, it would we invalid to read LCAP :)
\o/ ofc. I read the datasheet but didn't look at the code again before commenting :D thanks
Well, we know that LP doesn't have 1b.*, so it's safe. One version is more explicit, the other saves us a few lines. I have no strong preference.
Since we already have the checks I mentioned, I would prefer having just one list, but having two isn't bad, too. I don't know. @Arthur what do you say?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 49: offset = 8, .count = 4
Right, 8 is the maximum per PCI device. It's not always 8, though. […]
Yeah, right, but we do check if it's enabled/available. So having 8 for all would not hurt, as the PCH-H example shows. If max. 8 functions per slot was PCI spec, we could hardcode it to 8 instead of having `.count` here
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 83: if (CONFIG(SKYLAKE_SOC_PCH_H)) : pcie_rp_update_devicetree(pch_h_rp_groups); : else : pcie_rp_update_devicetree(pch_lp_rp_groups); :
We already do that for every port. If it wasn't set to root port, it would we invalid to read LCAP :)
\o/ ofc. I read the datasheet but didn't look at the code again before commenting :D thanks
Well, we know that LP doesn't have 1b.*, so it's safe. One version is more explicit, the other saves us a few lines. I have no strong preference.
Since we already have the checks I mentioned, I would prefer having just one list, but having two isn't bad, too. I don't know. @Arthur what do you say?
I don't think it matters much here, but if you ever want to replace FSP and do the port mapping yourself you probably need to know how much root ports you have. That's a big IF, so I think either ways are fine.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 49: offset = 8, .count = 4
Yeah, right, but we do check if it's enabled/available. […]
Ah, "8 is the maximum per PCI device." -> so it's PCI spec? Then why not simply drop `.count`? As you already comment below, "probing 4 non-existent ports shouldn't hurt" :-)
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 83: if (CONFIG(SKYLAKE_SOC_PCH_H)) : pcie_rp_update_devicetree(pch_h_rp_groups); : else : pcie_rp_update_devicetree(pch_lp_rp_groups); :
We already do that for every port. If it wasn't set to root port, it would […]
Very big IF, unfortunately :/ Well, Nico you decide ;)
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 49: offset = 8, .count = 4
Ah, "8 is the maximum per PCI device." -> so it's PCI spec? Then why not simply drop `. […]
As discussed on IRC we need to find out the offset and doing this at runtime is not possible, so we need either count or offset.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/14/src/soc/intel/skylake/chip... PS14, Line 83: if (CONFIG(SKYLAKE_SOC_PCH_H)) : pcie_rp_update_devicetree(pch_h_rp_groups); : else : pcie_rp_update_devicetree(pch_lp_rp_groups); :
Very big IF, unfortunately :/ Well, Nico you decide ;)
left as-is as discussed on IRC
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 141: for (dev = *ptr; dev; dev = *ptr) { : if (dev->path.type != DEVICE_PATH_PCI) { : ptr = &dev->sibling; : continue; : } : : offset = 0; : for (group = groups; group->count; ++group) { : if (group->slot == PCI_SLOT(dev->path.pci.devfn)) : break; : offset += group->count; : } : if (!group->count) { : ptr = &dev->sibling; : continue; : } : : if (pcie_rp_update_dev(dev, offset, mapping) < 0) { : /* Unlink disabled device. */ : *ptr = dev->sibling; : dev->sibling = NULL; : } : } Is it something like the following you had in mind? Since this runs before enumeration it might be possible to simply skip the unlinking as this is done elsewhere? struct device *dev; struct device *prev_dev = NULL; for (dev = *ptr; dev; prev_dev = dev, dev = dev->sibling) { if (dev->path.type != DEVICE_PATH_PCI) { continue; }
offset = 0; for (group = groups; group->count; ++group) { if (group->slot == PCI_SLOT(dev->path.pci.devfn)) break; offset += group->count; } if (!group->count) { continue; }
if (pcie_rp_update_dev(dev, offset, mapping) < 0) { /* Unlink disabled device. */ prev_dev->sibling = dev->sibling; } }
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 18:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 160: *ptr = dev->sibling;
Is the idea to prev_dev->sibling = dev->sibling; and set dev = NULL;?
The former, yes, the latter, no.
But also including the first-iteration case, when we want to alter `root->children`.
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 161: dev->sibling
dev = NULL; ?
No. The next step is either leaving the function and destroying the stack frame including `dev`, or `dev = *ptr` (third part of the `for`) which would override it.
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 141: for (dev = *ptr; dev; dev = *ptr) { : if (dev->path.type != DEVICE_PATH_PCI) { : ptr = &dev->sibling; : continue; : } : : offset = 0; : for (group = groups; group->count; ++group) { : if (group->slot == PCI_SLOT(dev->path.pci.devfn)) : break; : offset += group->count; : } : if (!group->count) { : ptr = &dev->sibling; : continue; : } : : if (pcie_rp_update_dev(dev, offset, mapping) < 0) { : /* Unlink disabled device. */ : *ptr = dev->sibling; : dev->sibling = NULL; : } : } That reflects the idea but is flawed:
* It misses to update `root->children`. * The `prev_dev = dev,` would also set `prev_dev` to the unlinked device. So a second unlink in a row would work with the wrong `prev_dev`.
Could be fixed as follows:
... if (pcie_rp_update_dev(dev, offset, mapping) < 0) { /* Unlink disabled device. */ if (prev_dev) prev_dev->sibling = dev->sibling; else root->children = dev->sibling; dev = prev_dev; }
I'm not sure if rewinding `dev` makes things easier to read?
Since this runs before enumeration it might be possible to simply skip the unlinking as this is done elsewhere?
The problem I'm trying to solve is that inside pcie_rp_update_dev() we have no new function number for the disabled device. So if we don't unlink it, we have potentially more than one device with the same `devfn` on the bus. This would break the mixing of `devicetree.cb` nodes with new nodes during enumeration.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 141: for (dev = *ptr; dev; dev = *ptr) { : if (dev->path.type != DEVICE_PATH_PCI) { : ptr = &dev->sibling; : continue; : } : : offset = 0; : for (group = groups; group->count; ++group) { : if (group->slot == PCI_SLOT(dev->path.pci.devfn)) : break; : offset += group->count; : } : if (!group->count) { : ptr = &dev->sibling; : continue; : } : : if (pcie_rp_update_dev(dev, offset, mapping) < 0) { : /* Unlink disabled device. */ : *ptr = dev->sibling; : dev->sibling = NULL; : } : }
That reflects the idea but is flawed: […]
Would renaming `ptr` to `link` help?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 162: } Missed to update `ptr` on the else path...
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 141: for (dev = *ptr; dev; dev = *ptr) { : if (dev->path.type != DEVICE_PATH_PCI) { : ptr = &dev->sibling; : continue; : } : : offset = 0; : for (group = groups; group->count; ++group) { : if (group->slot == PCI_SLOT(dev->path.pci.devfn)) : break; : offset += group->count; : } : if (!group->count) { : ptr = &dev->sibling; : continue; : } : : if (pcie_rp_update_dev(dev, offset, mapping) < 0) { : /* Unlink disabled device. */ : *ptr = dev->sibling; : dev->sibling = NULL; : } : }
Could be fixed as follows:
... if (pcie_rp_update_dev(dev, offset, mapping) < 0) { /* Unlink disabled device. */ if (prev_dev) prev_dev->sibling = dev->sibling; else root->children = dev->sibling; dev = prev_dev; }
Hmmm, doesn't work. Rewinding may break the assumption that `dev != NULL` when we do `dev = dev->sibling` before the next iteration.
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Arthur Heymans, Michael Niewöhner, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#19).
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
soc/intel: Implement PCIe RP devicetree update based on LCAP
Most of the current implementations for FSP-based platforms make (sometimes wrong) assumptions how FSP reorders root ports and what is specified in the devicetree. We don't have to make assumptions though, and can read the root-port number from the PCIe link capapilities (LCAP) instead. This is also what we do in ASL code for years already.
This new implementation acts solely on information read from the PCI config space. In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, update device paths and unlink vanished devices.
To be most compatible, we work with the following constraints: o Use only standard PCI config registers. o Most notable, don't try to read the registers that configure the function numbers. FSP has undocumented ways to block access to non-standard registers. o Don't make assumptions what function is assigned to hidden devices.
The following assumptions were made, though: o The absolute root-port numbering as documented in datasheets matches what is read from LCAP. o This numbering doesn't contain any gaps. o Original root-port function numbers below a PCI device start at function zero and also don't contain any gaps.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/include/device/pci_def.h A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c 4 files changed, 221 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/19
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 19:
(4 comments)
Kyösti, now that turned to unlink device nodes, could you have a look at that?
https://review.coreboot.org/c/coreboot/+/35985/17/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/17/src/soc/intel/common/block... PS17, Line 102: dev->enabled = 0;
I thought about unlinking these nodes from the bus. And now I'm […]
Done
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 160: *ptr = dev->sibling;
The former, yes, the latter, no. […]
Done
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 161: dev->sibling
No. The next step is either leaving the function and destroying the stack frame including […]
Done
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 162: }
Missed to update `ptr` on the else path...
Done
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 19:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... PS19, Line 37: root-port nit: root port
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... PS19, Line 132: root-port nit: root port
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... PS19, Line 37: root-port
nit: root port
Not a native speaker, but to me it's a question of associativity: `(root-port) functions` vs `root (port functions)`
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... PS19, Line 37: root-port
Not a native speaker, but to me it's a question of associativity: […]
Well, non-native, too here, but AFAIK there is no `-` for combining words in English. That's just a typical German thing. I'd be ok with it if you wrote `root-port` everywhere, but this one is the only occurence :)
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 19: Code-Review+1
Tested on X11SSM-F with multiple configurations; looks good
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... PS19, Line 37: root-port
Well, non-native, too here, but AFAIK there is no `-` for combining words in English. […]
That's just not true. You can search for compound words and hyphenation. General rules I could ever find about it end at 2x nouns. Never found anything about 3 nouns. Some say there are no rules. Today I've read the hyphen can be used to avoid ambiguity, which is why I used it here. One could also argue that `root-port` is describing `functions` and apply the rules for compound adjectives...
In German, btw., the space wouldn't be allowed ;)
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... PS19, Line 37: root-port
That's just not true. You can search for compound words and hyphenation. […]
Ok, looks like I was wrong; thanks! :) but then be consistent at least :P
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 19:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/skylake/chip... PS19, Line 102: unsigned int inc = PCI_DEVFN(0, 1); I really think this is arithmetic on devfn is not good. We shouldn't be using the underlying encoding. We can fix it later, but these assumptions (along with all the pci.devfn directly accesses) leads to harder to change code.
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/skylake/chip... PS19, Line 102: unsigned int inc Why isn't inc marked const since it's never manipulated again?
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/skylake/chip... PS19, Line 133: func0->path.pci.devfn = dev->path.pci.devfn; Why is the swap necessary?
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/skylake/chip... PS19, Line 140: static void pcie_override_devicetree_after_silicon_init(void) Is this file just showing an alternative approach to the common one?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/skylake/chip... PS19, Line 102: unsigned int inc
Why isn't inc marked const since it's never manipulated again?
oh.. apparently this change is in an earlier dependency patch. I'll go try and track that one down.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 19:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/skylake/chip... PS19, Line 102: unsigned int inc
oh.. apparently this change is in an earlier dependency patch. I'll go try and track that one down.
I'm so confused. When I was doing diff between patchset 14 and 19 this file came up as being changed. I have no idea why, but my comments are here. I'll resolve those.
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/skylake/chip... PS19, Line 102: unsigned int inc = PCI_DEVFN(0, 1);
I really think this is arithmetic on devfn is not good. […]
Ack
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/skylake/chip... PS19, Line 133: func0->path.pci.devfn = dev->path.pci.devfn;
Why is the swap necessary?
Ack
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/skylake/chip... PS19, Line 140: static void pcie_override_devicetree_after_silicon_init(void)
Is this file just showing an alternative approach to the common one?
Ack
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/skylake/chip... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/skylake/chip... PS19, Line 102: unsigned int inc
I'm so confused. […]
Sorry, partially my fault. As the review became longer, I thought it would be a good idea to split the Skylake change out and focus on the new implementation. Didn't think about how it looks like in a diff between the patch sets -.-
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 19:
(3 comments)
Tested this again on SKL with an artificial corner-case devicetree. Inconsistencies in `on` settings were correctly discovered, devicetree nodes correctly patched (tested by specifying individual SSIDs and where coreboot wanted to assign them later; actual assignments failed though, because the registers were already locked). Also, no complaints about spurious nodes during PCI enumeration.
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... PS19, Line 37: root-port
Ok, looks like I was wrong; thanks! :) but then be consistent at least :P
AFAIK, I've consistently used it hyphenated when describing another noun and not hyphenated when used as a noun. Please let me know if you see any inconsistencies.
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 141: for (dev = *ptr; dev; dev = *ptr) { : if (dev->path.type != DEVICE_PATH_PCI) { : ptr = &dev->sibling; : continue; : } : : offset = 0; : for (group = groups; group->count; ++group) { : if (group->slot == PCI_SLOT(dev->path.pci.devfn)) : break; : offset += group->count; : } : if (!group->count) { : ptr = &dev->sibling; : continue; : } : : if (pcie_rp_update_dev(dev, offset, mapping) < 0) { : /* Unlink disabled device. */ : *ptr = dev->sibling; : dev->sibling = NULL; : } : }
Could be fixed as follows: […]
Rewrote the loop to make the (un)linking better visible. Let me know if it still worries you.
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... PS19, Line 132: root-port
nit: root port
Nope, as discussed elsewhere.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 19:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 141: for (dev = *ptr; dev; dev = *ptr) { : if (dev->path.type != DEVICE_PATH_PCI) { : ptr = &dev->sibling; : continue; : } : : offset = 0; : for (group = groups; group->count; ++group) { : if (group->slot == PCI_SLOT(dev->path.pci.devfn)) : break; : offset += group->count; : } : if (!group->count) { : ptr = &dev->sibling; : continue; : } : : if (pcie_rp_update_dev(dev, offset, mapping) < 0) { : /* Unlink disabled device. */ : *ptr = dev->sibling; : dev->sibling = NULL; : } : }
Rewrote the loop to make the (un)linking better visible. Let me know if it still worries you.
looks good to me.
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... PS19, Line 158: link maybe prev is a better name (like in device/pci_device.c)?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... PS19, Line 158: link
maybe prev is a better name (like in device/pci_device. […]
Hmmm, I'm not a fan of `prev`/`next` names if there is no associated `current`. If `prev` alone makes sense depends on the line that one reads. e.g. `prev = &dev->sibling`: at the time that will be executed, it's not the previous, it's the next...
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... PS19, Line 158: link
Hmmm, I'm not a fan of `prev`/`next` names if there is no associated […]
Ack
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 19: Code-Review+1
(1 comment)
Let me know what you think about the mapping[rp_idx] check.
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... PS19, Line 84: mapping[rp_idx] = fn; I think we should add a check here if mapping[rp_idx] is not == -1 prior to assignment. That way we know our assumptions aren't holding. Admittedly that would only be within a single group because of the port_num check in pcie_rp_original_idx(). But I think it's helpful in calling out weirdness we may see.
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Arthur Heymans, Michael Niewöhner, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35985
to look at the new patch set (#20).
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
soc/intel: Implement PCIe RP devicetree update based on LCAP
Most of the current implementations for FSP-based platforms make (sometimes wrong) assumptions how FSP reorders root ports and what is specified in the devicetree. We don't have to make assumptions though, and can read the root-port number from the PCIe link capapilities (LCAP) instead. This is also what we do in ASL code for years already.
This new implementation acts solely on information read from the PCI config space. In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, update device paths and unlink vanished devices.
To be most compatible, we work with the following constraints: o Use only standard PCI config registers. o Most notable, don't try to read the registers that configure the function numbers. FSP has undocumented ways to block access to non-standard registers. o Don't make assumptions what function is assigned to hidden devices.
The following assumptions were made, though: o The absolute root-port numbering as documented in datasheets matches what is read from LCAP. o This numbering doesn't contain any gaps. o Original root-port function numbers below a PCI device start at function zero and also don't contain any gaps.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/include/device/pci_def.h A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c 4 files changed, 229 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/35985/20
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/19/src/soc/intel/common/block... PS19, Line 84: mapping[rp_idx] = fn;
I think we should add a check here if mapping[rp_idx] is not == -1 prior to assignment. […]
I don't expect such hardware weirdness. But you are right, we can make one assumptions less. Done.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 20: Code-Review+2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 20: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 20: Code-Review+1
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: soc/intel: Implement PCIe RP devicetree update based on LCAP ......................................................................
soc/intel: Implement PCIe RP devicetree update based on LCAP
Most of the current implementations for FSP-based platforms make (sometimes wrong) assumptions how FSP reorders root ports and what is specified in the devicetree. We don't have to make assumptions though, and can read the root-port number from the PCIe link capapilities (LCAP) instead. This is also what we do in ASL code for years already.
This new implementation acts solely on information read from the PCI config space. In a first round, we scan all possible DEVFNs and store which root port has that DEVFN now. Then, we walk through the devicetree that still only knows devices that were originally mentioned in `devicetree.cb`, update device paths and unlink vanished devices.
To be most compatible, we work with the following constraints: o Use only standard PCI config registers. o Most notable, don't try to read the registers that configure the function numbers. FSP has undocumented ways to block access to non-standard registers. o Don't make assumptions what function is assigned to hidden devices.
The following assumptions were made, though: o The absolute root-port numbering as documented in datasheets matches what is read from LCAP. o This numbering doesn't contain any gaps. o Original root-port function numbers below a PCI device start at function zero and also don't contain any gaps.
Change-Id: Ib17d2b6fd34608603db3936d638bdf5acb46d717 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/35985 Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Michael Niewöhner Reviewed-by: Patrick Rudolph patrick.rudolph@9elements.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/include/device/pci_def.h A src/soc/intel/common/block/include/intelblocks/pcie_rp.h M src/soc/intel/common/block/pcie/Makefile.inc A src/soc/intel/common/block/pcie/pcie_rp.c 4 files changed, 229 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Patrick Rudolph: Looks good to me, but someone else must approve Michael Niewöhner: Looks good to me, approved
diff --git a/src/include/device/pci_def.h b/src/include/device/pci_def.h index c8b86d5..d906445 100644 --- a/src/include/device/pci_def.h +++ b/src/include/device/pci_def.h @@ -426,6 +426,7 @@ #define PCI_EXP_LNKCAP_L0SEL 0x7000 /* L0s Exit Latency */ #define PCI_EXP_LNKCAP_L1EL 0x38000 /* L1 Exit Latency */ #define PCI_EXP_CLK_PM 0x40000 /* Clock Power Management */ +#define PCI_EXP_LNKCAP_PORT 0xff000000 /* Port Number */ #define PCI_EXP_LNKCTL 16 /* Link Control */ #define PCI_EXP_LNKCTL_RL 0x20 /* Retrain Link */ #define PCI_EXP_LNKCTL_CCC 0x40 /* Common Clock COnfiguration */ diff --git a/src/soc/intel/common/block/include/intelblocks/pcie_rp.h b/src/soc/intel/common/block/include/intelblocks/pcie_rp.h new file mode 100644 index 0000000..52fde28 --- /dev/null +++ b/src/soc/intel/common/block/include/intelblocks/pcie_rp.h @@ -0,0 +1,50 @@ +/* + * This file is part of the coreboot project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef SOC_INTEL_COMMON_BLOCK_PCIE_RP_H +#define SOC_INTEL_COMMON_BLOCK_PCIE_RP_H + +#include <stdint.h> + +/* + * The PCIe Root Ports usually come in groups of up to 8 PCI-device + * functions. + * + * `slot` is the PCI device/slot number of such a group. + * `count` is the number of functions within the group. It is assumed that + * the first group includes the RPs 1 to the first group's `count` and that + * adjacent groups follow without gaps in the numbering. + */ +struct pcie_rp_group { + unsigned int slot; + unsigned int count; +}; + +/* + * Update PCI paths of the root ports in the devicetree. + * + * Depending on the board layout and physical presence of downstream + * devices, individual root-port functions can be hidden and reordered. + * If we have device nodes for root ports in the static `devicetree.cb`, + * we need to update their PCI paths, so the nodes still control the + * correct root port. Device nodes for disabled root ports will be + * unlinked from the bus, to not interfere with PCI enumeration. + * + * Call this once, after root ports have been reordered, but before PCI + * enumeration. + * + * `groups` points to a list of groups terminated by an entry with `count == 0`. + */ +void pcie_rp_update_devicetree(const struct pcie_rp_group *groups); + +#endif /* SOC_INTEL_COMMON_BLOCK_PCIE_RP_H */ diff --git a/src/soc/intel/common/block/pcie/Makefile.inc b/src/soc/intel/common/block/pcie/Makefile.inc index ac311a7..0cded9d 100644 --- a/src/soc/intel/common/block/pcie/Makefile.inc +++ b/src/soc/intel/common/block/pcie/Makefile.inc @@ -1 +1,2 @@ ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_PCIE) += pcie.c +ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_PCIE) += pcie_rp.c diff --git a/src/soc/intel/common/block/pcie/pcie_rp.c b/src/soc/intel/common/block/pcie/pcie_rp.c new file mode 100644 index 0000000..96f92f0 --- /dev/null +++ b/src/soc/intel/common/block/pcie/pcie_rp.c @@ -0,0 +1,177 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Nico Huber nico.h@gmx.de + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <stdint.h> + +#include <commonlib/helpers.h> +#include <console/console.h> +#include <device/device.h> +#include <device/pci_def.h> +#include <device/pci_ops.h> +#include <device/pci_type.h> +#include <intelblocks/pcie_rp.h> + +static int pcie_rp_original_idx( + const struct pcie_rp_group *const group, + const unsigned int offset, + const pci_devfn_t dev) +{ + const uint16_t clist = pci_s_find_capability(dev, PCI_CAP_ID_PCIE); + if (clist == 0) { + printk(BIOS_WARNING, + "%s: Can't find PCIe capapilities for PCI: 00:%02x.%x, ignoring.\n", + __func__, group->slot, PCI_FUNC(PCI_DEV2DEVFN(dev))); + return -1; + } + + const uint16_t xcap = pci_s_read_config16(dev, clist + PCI_EXP_FLAGS); + if ((xcap & PCI_EXP_FLAGS_TYPE) >> 4 != PCI_EXP_TYPE_ROOT_PORT) { + printk(BIOS_WARNING, "%s: Non root-port found at PCI: 00:%02x.%x, ignoring.\n", + __func__, group->slot, PCI_FUNC(PCI_DEV2DEVFN(dev))); + return -1; + } + + const uint32_t lcap = pci_s_read_config32(dev, clist + PCI_EXP_LNKCAP); + /* Read 1-based absolute port number. This reflects the numbering + scheme that Intel uses in their documentation and what we use + as index (0-based, though) in our mapping. */ + const unsigned int port_num = (lcap & PCI_EXP_LNKCAP_PORT) >> 24; + + /* `port_num` is 1-based, `offset` is 0-based. */ + if (port_num <= offset || port_num > offset + group->count) { + printk(BIOS_WARNING, "%s: Unexpected root-port number '%u'" + " at PCI: 00:%02x.%x, ignoring.\n", + __func__, port_num, group->slot, PCI_FUNC(PCI_DEV2DEVFN(dev))); + return -1; + } + + return port_num - 1; +} + +/* Scan actual PCI config space to reconstruct current mapping */ +static void pcie_rp_scan_groups(int mapping[], const struct pcie_rp_group *const groups) +{ + unsigned int offset = 0; + const struct pcie_rp_group *group; + for (group = groups; group->count; ++group) { + unsigned int fn; + for (fn = 0; fn < group->count; ++fn) { + const pci_devfn_t dev = PCI_DEV(0, group->slot, fn); + const uint16_t did = pci_s_read_config16(dev, PCI_DEVICE_ID); + if (did == 0xffff) { + if (fn == 0) + break; + continue; + } + + const int rp_idx = pcie_rp_original_idx(group, offset, dev); + if (rp_idx < 0) + continue; + if (mapping[rp_idx] != -1) { + printk(BIOS_WARNING, "%s: Root Port #%u reported by PCI: " + "00:%02x.%x already reported by PCI: 00:%02x.%x!\n", + __func__, rp_idx + 1, group->slot, fn, + group->slot, mapping[rp_idx]); + continue; + } + + printk(BIOS_INFO, "Found PCIe Root Port #%u at PCI: 00:%02x.%x.\n", + rp_idx + 1, group->slot, fn); + mapping[rp_idx] = fn; + } + offset += group->count; + } +} + +/* Returns `true` if the device should be unlinked. */ +static bool pcie_rp_update_dev( + struct device *const dev, + const struct pcie_rp_group *const groups, + const int mapping[]) +{ + if (dev->path.type != DEVICE_PATH_PCI) + return false; + + /* Find matching group and offset. */ + unsigned int offset = 0; + const struct pcie_rp_group *group; + for (group = groups; group->count; ++group) { + if (PCI_SLOT(dev->path.pci.devfn) == group->slot && + PCI_FUNC(dev->path.pci.devfn) < group->count) + break; + offset += group->count; + } + if (!group->count) + return false; + + /* Now update based on what we know. */ + const int rp_idx = offset + PCI_FUNC(dev->path.pci.devfn); + const int new_fn = mapping[rp_idx]; + if (new_fn < 0) { + if (dev->enabled) { + printk(BIOS_NOTICE, "%s: Couldn't find PCIe Root Port #%u " + "(originally %s) which was enabled in devicetree, removing.\n", + __func__, rp_idx + 1, dev_path(dev)); + } + return true; + } else if (PCI_FUNC(dev->path.pci.devfn) != new_fn) { + printk(BIOS_INFO, + "Remapping PCIe Root Port #%u from %s to new function number %u.\n", + rp_idx + 1, dev_path(dev), new_fn); + dev->path.pci.devfn = PCI_DEVFN(PCI_SLOT(dev->path.pci.devfn), new_fn); + } + return false; +} + +void pcie_rp_update_devicetree(const struct pcie_rp_group *const groups) +{ + /* Maps absolute root-port numbers to function numbers. + Negative if disabled, new function number otherwise. */ + int mapping[CONFIG_MAX_ROOT_PORTS]; + unsigned int offset, i; + + struct bus *const root = pci_root_bus(); + if (!root) + return; + + offset = 0; + const struct pcie_rp_group *group; + for (group = groups; group->count; ++group) + offset += group->count; + + if (offset > ARRAY_SIZE(mapping)) { + printk(BIOS_ERR, "%s: Error: Group exceeds CONFIG_MAX_ROOT_PORTS.\n", __func__); + return; + } + + /* Assume everything we don't encounter later is disabled */ + for (i = 0; i < ARRAY_SIZE(mapping); ++i) + mapping[i] = -1; + + pcie_rp_scan_groups(mapping, groups); + + struct device *dev; + struct device **link = &root->children; + for (dev = *link; dev; dev = *link) { + if (pcie_rp_update_dev(dev, groups, mapping)) { + /* Unlink vanished device. */ + *link = dev->sibling; + dev->sibling = NULL; + continue; + } + + link = &dev->sibling; + } +}