From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Subject: Re: [PATCH v1 7/7] pseries/setup: Add Initialization of VF Bars To: "Bryant G. Ly" , benh@kernel.crashing.org, paulus@samba.org, mpe@ellerman.id.au Cc: seroyer@linux.vnet.ibm.com, jjalvare@linux.vnet.ibm.com, alex.williamson@redhat.com, helgaas@kernel.org, ruscur@russell.cc, linux-pci@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, bodong@mellanox.com, eli@mellanox.com, saeedm@mellanox.com References: <20171213153242.98015-1-bryantly@linux.vnet.ibm.com> <20171213153242.98015-8-bryantly@linux.vnet.ibm.com> From: Alexey Kardashevskiy Message-ID: <4ea33f2e-d98c-85ef-eedf-2e8ca49aa839@ozlabs.ru> Date: Mon, 18 Dec 2017 18:21:04 +1100 MIME-Version: 1.0 In-Reply-To: <20171213153242.98015-8-bryantly@linux.vnet.ibm.com> Content-Type: text/plain; charset=utf-8 List-ID: On 14/12/17 02:32, Bryant G. Ly wrote: > When enabling SR-IOV in pseries platform, > the VF bar properties for a PF are reported on > the device node in the device tree. > > This patch adds the IOV Bar resources to Linux > structures from the device tree for later use > when configuring SR-IOV by PF driver. > > Signed-off-by: Bryant G. Ly > Signed-off-by: Juan J. Alvarez > --- > arch/powerpc/include/asm/pci.h | 2 + > arch/powerpc/kernel/pci_of_scan.c | 2 +- > arch/powerpc/platforms/pseries/setup.c | 183 +++++++++++++++++++++++++++++++++ > 3 files changed, 186 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h > index 8dc32eacc97c..d82802ff5088 100644 > --- a/arch/powerpc/include/asm/pci.h > +++ b/arch/powerpc/include/asm/pci.h > @@ -121,6 +121,8 @@ extern int remove_phb_dynamic(struct pci_controller *phb); > extern struct pci_dev *of_create_pci_dev(struct device_node *node, > struct pci_bus *bus, int devfn); > > +extern unsigned int pci_parse_of_flags(u32 addr0, int bridge); > + > extern void of_scan_pci_bridge(struct pci_dev *dev); > > extern void of_scan_bus(struct device_node *node, struct pci_bus *bus); > diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c > index 0d790f8432d2..20ceec4a5f5e 100644 > --- a/arch/powerpc/kernel/pci_of_scan.c > +++ b/arch/powerpc/kernel/pci_of_scan.c > @@ -38,7 +38,7 @@ static u32 get_int_prop(struct device_node *np, const char *name, u32 def) > * @addr0: value of 1st cell of a device tree PCI address. > * @bridge: Set this flag if the address is from a bridge 'ranges' property > */ > -static unsigned int pci_parse_of_flags(u32 addr0, int bridge) > +unsigned int pci_parse_of_flags(u32 addr0, int bridge) > { > unsigned int flags = 0; > > diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c > index 5f1beb8367ac..ce28882cbde8 100644 > --- a/arch/powerpc/platforms/pseries/setup.c > +++ b/arch/powerpc/platforms/pseries/setup.c > @@ -459,6 +459,181 @@ static void __init find_and_init_phbs(void) > of_pci_check_probe_only(); > } > > +#ifdef CONFIG_PCI_IOV > +enum rtas_iov_fw_value_map { > + NUM_RES_PROPERTY = 0, ///< Number of Resources > + LOW_INT = 1, ///< Lowest 32 bits of Address > + START_OF_ENTRIES = 2, ///< Always start of entry > + APERTURE_PROPERTY = 2, ///< Start of entry+ to Aperture Size > + WDW_SIZE_PROPERTY = 4, ///< Start of entry+ to Window Size > + NEXT_ENTRY = 7 ///< Go to next entry on array > +}; > + > +enum get_iov_fw_value_index { > + BAR_ADDRS = 1, ///< Get Bar Address > + APERTURE_SIZE = 2, ///< Get Aperture Size > + WDW_SIZE = 3 ///< Get Window Size > +}; > + > +resource_size_t pseries_get_iov_fw_values(struct pci_dev *dev, int resno, s/pseries_get_iov_fw_values/pseries_get_iov_fw_value/ as it returns a single value. > + enum get_iov_fw_value_index value) > +{ > + struct vf_bar_wdw { > + __be64 addr; > + __be64 aperture_size; > + __be64 wdw_size; > + }; > + > + struct vf_bar_wdw window_avail[PCI_SRIOV_NUM_BARS]; > + const int *indexes; > + struct device_node *dn = pci_device_to_OF_node(dev); > + int i, r, num_res; > + resource_size_t return_value; resource_size_t return_value = 0; and remove initialization below. Also, way too long, @ret is a more common name. > + > + indexes = of_get_property(dn, "ibm,open-sriov-vf-bar-info", NULL); > + if (!indexes) > + return 0; > + > + memset(window_avail, > + 0, sizeof(struct vf_bar_wdw) * PCI_SRIOV_NUM_BARS); This is more common way of doing the same initialization: struct vf_bar_wdw { __be64 addr; __be64 aperture_size; __be64 wdw_size; } window_avail[PCI_SRIOV_NUM_BARS] = { 0 }; > + return_value = 0; > + /* > + * First element in the array is the number of Bars > + * returned. Search through the list to find the matching > + * bar > + */ > + num_res = of_read_number(&indexes[NUM_RES_PROPERTY], 1); if (resno >= num_res) return 0; /* or an errror */ i = START_OF_ENTRIES + NEXT_ENTRY * resno; switch (value) { case BAR_ADDRS: ret = f_read_number(&indexes[i], 2); break; case APERTURE_SIZE: ret = of_read_number(&indexes[i + APERTURE_PROPERTY], 2); break; case WDW_SIZE: ret = of_read_number(&indexes[i + WDW_SIZE_PROPERTY], 2); break; } return ret; } and remove the reminder of the function, and window_avail, and vf_bar_wdw? > + for (i = START_OF_ENTRIES, r = 0; r < num_res && r < PCI_SRIOV_NUM_BARS; > + i += NEXT_ENTRY, r++) { > + window_avail[r].addr = of_read_number(&indexes[i], 2); > + window_avail[r].aperture_size = > + of_read_number(&indexes[i + APERTURE_PROPERTY], 2); > + window_avail[r].wdw_size = > + of_read_number(&indexes[i + WDW_SIZE_PROPERTY], 2); > + } > + > + switch (value) { > + case BAR_ADDRS: > + return_value = window_avail[resno].addr; > + break; > + case APERTURE_SIZE: > + return_value = window_avail[resno].aperture_size; > + break; > + case WDW_SIZE: > + return_value = window_avail[resno].wdw_size; > + break; > + default: > + break; > + } > + return return_value; > +} > + > +void of_pci_parse_vf_bar_size(struct pci_dev *dev, const int *indexes) It does not seem to make a lot of sense as a separate function imho... > +{ > + struct resource *res; > + resource_size_t base, size; > + int i, r, num_res; > + > + num_res = of_read_number(&indexes[NUM_RES_PROPERTY], 1); num_res = min_t(int, num_res, PCI_SRIOV_NUM_BARS) ? Matter of personal taste though. > + for (i = START_OF_ENTRIES, r = 0; r < num_res && r < PCI_SRIOV_NUM_BARS; > + i += NEXT_ENTRY, r++) { > + res = &dev->resource[r + PCI_IOV_RESOURCES]; > + base = of_read_number(&indexes[i], 2); > + size = of_read_number(&indexes[i + APERTURE_PROPERTY], 2); > + res->flags = pci_parse_of_flags(of_read_number > + (&indexes[i + LOW_INT], 1), 0); > + res->flags |= (IORESOURCE_MEM_64 | IORESOURCE_PCI_FIXED); > + res->name = pci_name(dev); > + res->start = base; > + res->end = base + size - 1; The function name suggests it only parses sizes but just above it assigns all resource parameters - size, address, flags. > + } > +} > + > +void of_pci_parse_iov_addrs(struct pci_dev *dev, const int *indexes) > +{ > + struct resource *res, *root, *conflict; > + resource_size_t base, size; > + int i, r, num_res; > + > + /* > + * First element in the array is the number of Bars > + * returned. Search through the list to find the matching > + * bars assign them from firmware into resources structure. > + */ > + num_res = of_read_number(&indexes[NUM_RES_PROPERTY], 1); > + for (i = START_OF_ENTRIES, r = 0; r < num_res && r < PCI_SRIOV_NUM_BARS; > + i += NEXT_ENTRY, r++) { > + res = &dev->resource[r + PCI_IOV_RESOURCES]; > + base = of_read_number(&indexes[i], 2); > + size = of_read_number(&indexes[i + WDW_SIZE_PROPERTY], 2); > + res->name = pci_name(dev); > + res->start = base; > + res->end = base + size - 1; > + root = pci_find_parent_resource(dev, res); > + > + if (!root) > + root = &iomem_resource; @dev here is a VF, right? I am not familiar with powervn much but from what I see - the devices are sitting on a root bus of their own PHB and they all either have a root returned from pci_find_parent_resource() or none of them has a root and will fall back to &iomem_resource, or both cases are possible? > + dev_dbg(&dev->dev, > + "Pseries IOV BAR %d: trying firmware assignment %pR\n", s/Pseries/pSeries/ ? > + r + PCI_IOV_RESOURCES, res); > + conflict = request_resource_conflict(root, res); > + if (conflict) { > + dev_info(&dev->dev, > + "BAR %d: %pR conflicts with %s %pR\n", > + r + PCI_IOV_RESOURCES, res, > + conflict->name, conflict); > + res->flags |= IORESOURCE_UNSET; > + } > + } > +} > + > +static void pseries_pci_fixup_resources(struct pci_dev *pdev) > +{ > + const int *indexes; > + struct device_node *dn = pci_device_to_OF_node(pdev); > + > + /*Firmware must support open sriov otherwise dont configure*/ > + indexes = of_get_property(dn, "ibm,open-sriov-vf-bar-info", NULL); > + if (!indexes) > + return; > + /* Assign the addresses from device tree*/ > + of_pci_parse_vf_bar_size(pdev, indexes); > +} > + > +static void pseries_pci_fixup_iov_resources(struct pci_dev *pdev) > +{ > + const int *indexes; > + struct device_node *dn = pci_device_to_OF_node(pdev); > + > + if (!pdev->is_physfn || pdev->is_added) > + return; > + /*Firmware must support open sriov otherwise dont configure*/ > + indexes = of_get_property(dn, "ibm,open-sriov-vf-bar-info", NULL); > + if (!indexes) > + return; > + /* Assign the addresses from device tree*/ > + of_pci_parse_iov_addrs(pdev, indexes); > +} > + > +static resource_size_t pseries_pci_iov_resource_alignment(struct pci_dev *pdev, > + int resno) > +{ > + const __be32 *reg; > + struct device_node *dn = pci_device_to_OF_node(pdev); > + > + /*Firmware must support open sriov otherwise report regular alignment*/ > + reg = of_get_property(dn, "ibm,is-open-sriov-pf", NULL); > + if (!reg) > + return pci_iov_resource_size(pdev, resno); > + > + if (!pdev->is_physfn) > + return 0; > + return pseries_get_iov_fw_values(pdev, > + resno - PCI_IOV_RESOURCES, > + APERTURE_SIZE); > +} > +#endif > + > static void __init pSeries_setup_arch(void) > { > set_arch_panic_timeout(10, ARCH_PANIC_TIMEOUT); > @@ -490,6 +665,14 @@ static void __init pSeries_setup_arch(void) > vpa_init(boot_cpuid); > ppc_md.power_save = pseries_lpar_idle; > ppc_md.enable_pmcs = pseries_lpar_enable_pmcs; > +#ifdef CONFIG_PCI_IOV > + ppc_md.pcibios_fixup_resources = > + pseries_pci_fixup_resources; > + ppc_md.pcibios_fixup_sriov = > + pseries_pci_fixup_iov_resources; > + ppc_md.pcibios_iov_resource_alignment = > + pseries_pci_iov_resource_alignment; > +#endif I wish one day this horrible ppc_md monster dies and all these helpers will go to PHB ops, hose ops, bus ops, whatever ops... > } else { > /* No special idle routine */ > ppc_md.enable_pmcs = power4_enable_pmcs; > -- Alexey