| Message ID | 1447204345-143793-1-git-send-email-gabriele.paoloni@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 11 November 2015 09:12:25 Gabriele Paoloni wrote: > From: gabpao01 <gabriele.paoloni@huawei.com> Regardless of this patch, you might want to update your global git config to use the proper name for your commits. > commit b3a72384fe29 ("ARM/PCI: Replace > pci_sys_data->align_resource with global function pointer") has > introduced a global function pointer that makes the ARM specific > code not portable and broken in case any platform have a two > different HW IPs for the PCIe host bridge controller. > This patch moves align_resource function pointer into > pci_host_bridge structure so that the code is now suitable to be > reworked as we want to get rid of hw_pci structure (the host bridge > drivers can just set the align function pointer in the > pci_host_bridge structure) and there is no more broken code for a > SoC with two HW IPs. > > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> Thanks for doing this, that addresses my main concerns, and is probably small enough to still get it into 4.4 if Bjorn agrees with the general idea of putting more function pointers into struct pci_host_bridge. As a follow-on to this, we can now try to get rid of pcibios_align_resource by moving the existing per-architecture implementations in there as well, with a reasonable default for the common case. Arnd
> -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: Wednesday, November 11, 2015 4:48 PM > To: linux-arm-kernel@lists.infradead.org > Cc: Gabriele Paoloni; linux-pci@vger.kernel.org; > lorenzo.pieralisi@arm.com; linux@arm.linux.org.uk; xuwei (O); Wangzhou > (B); bhelgaas@google.com; Liguozhu (Kenneth) > Subject: Re: [PATCH] ARM/PCI: move align_resource function pointer into > pci_host_bridge structure > > On Wednesday 11 November 2015 09:12:25 Gabriele Paoloni wrote: > > From: gabpao01 <gabriele.paoloni@huawei.com> > > Regardless of this patch, you might want to update your global git > config > to use the proper name for your commits. Sorry, I am travelling and moved to another PC...I didn't notice the wrong name, my bad. > > > commit b3a72384fe29 ("ARM/PCI: Replace > > pci_sys_data->align_resource with global function pointer") has > > introduced a global function pointer that makes the ARM specific > > code not portable and broken in case any platform have a two > > different HW IPs for the PCIe host bridge controller. > > This patch moves align_resource function pointer into > > pci_host_bridge structure so that the code is now suitable to be > > reworked as we want to get rid of hw_pci structure (the host bridge > > drivers can just set the align function pointer in the > > pci_host_bridge structure) and there is no more broken code for a > > SoC with two HW IPs. > > > > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> > > Thanks for doing this, that addresses my main concerns, and is probably > small enough to still get it into 4.4 if Bjorn agrees with the general > idea of putting more function pointers into struct pci_host_bridge. > > As a follow-on to this, we can now try to get rid of > pcibios_align_resource > by moving the existing per-architecture implementations in there as > well, > with a reasonable default for the common case. > > Arnd
On Wed, Nov 11, 2015 at 09:12:25AM +0800, Gabriele Paoloni wrote: > From: gabpao01 <gabriele.paoloni@huawei.com> > > commit b3a72384fe29 ("ARM/PCI: Replace > pci_sys_data->align_resource with global function pointer") has > introduced a global function pointer that makes the ARM specific > code not portable and broken in case any platform have a two > different HW IPs for the PCIe host bridge controller. > This patch moves align_resource function pointer into > pci_host_bridge structure so that the code is now suitable to be > reworked as we want to get rid of hw_pci structure (the host bridge > drivers can just set the align function pointer in the > pci_host_bridge structure) and there is no more broken code for a > SoC with two HW IPs. > > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> This looks good to me. I merged b3a72384fe29 so maybe it would make sense for me to merge this follow-up as well. I put it on my for-linus branch for v4.4. If you'd rather merge it via an ARM tree, here's my ack: Acked-by: Bjorn Helgaas <bhelgaas@google.com> If you want me to merge it, an ARM ack would be nice. If you want to merge it, let me know and I'll drop it from my tree. > + /* Resource alignement requirements */ s/alignement/alignment/ > + resource_size_t (*align_resource)(struct pci_dev *dev, > ...
On Wednesday 25 November 2015 13:32:01 Bjorn Helgaas wrote: > On Wed, Nov 11, 2015 at 09:12:25AM +0800, Gabriele Paoloni wrote: > > From: gabpao01 <gabriele.paoloni@huawei.com> > > > > commit b3a72384fe29 ("ARM/PCI: Replace > > pci_sys_data->align_resource with global function pointer") has > > introduced a global function pointer that makes the ARM specific > > code not portable and broken in case any platform have a two > > different HW IPs for the PCIe host bridge controller. > > This patch moves align_resource function pointer into > > pci_host_bridge structure so that the code is now suitable to be > > reworked as we want to get rid of hw_pci structure (the host bridge > > drivers can just set the align function pointer in the > > pci_host_bridge structure) and there is no more broken code for a > > SoC with two HW IPs. > > > > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> > > This looks good to me. I merged b3a72384fe29 so maybe it would make > sense for me to merge this follow-up as well. I put it on my > for-linus branch for v4.4. > > If you'd rather merge it via an ARM tree, here's my ack: > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > If you want me to merge it, an ARM ack would be nice. If you want to > merge it, let me know and I'll drop it from my tree. > > > + /* Resource alignement requirements */ > > s/alignement/alignment/ > > > + resource_size_t (*align_resource)(struct pci_dev *dev, > > ... > I was the one who asked for this change, so you can definitely have my Reviewed-by: Arnd Bergmann <arnd@arndb.de> If Russell wants to take it through his tree, Gabriele can submit it to http://www.arm.linux.org.uk/developer/patches/, otherwise I'd suggest we merge it through your tree, as that was also the source of the patch we are trying to fix up. Arnd
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c index 6551d28..066f7f9 100644 --- a/arch/arm/kernel/bios32.c +++ b/arch/arm/kernel/bios32.c @@ -17,11 +17,6 @@ #include <asm/mach/pci.h> static int debug_pci; -static resource_size_t (*align_resource)(struct pci_dev *dev, - const struct resource *res, - resource_size_t start, - resource_size_t size, - resource_size_t align) = NULL; /* * We can't use pci_get_device() here since we are @@ -461,7 +456,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, sys->busnr = busnr; sys->swizzle = hw->swizzle; sys->map_irq = hw->map_irq; - align_resource = hw->align_resource; INIT_LIST_HEAD(&sys->resources); if (hw->private_data) @@ -470,6 +464,8 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, ret = hw->setup(nr, sys); if (ret > 0) { + struct pci_host_bridge *host_bridge; + ret = pcibios_init_resources(nr, sys); if (ret) { kfree(sys); @@ -491,6 +487,9 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, busnr = sys->bus->busn_res.end + 1; list_add(&sys->node, head); + + host_bridge = pci_find_host_bridge(sys->bus); + host_bridge->align_resource = hw->align_resource; } else { kfree(sys); if (ret < 0) @@ -578,14 +577,18 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, { struct pci_dev *dev = data; resource_size_t start = res->start; + struct pci_host_bridge *host_bridge; if (res->flags & IORESOURCE_IO && start & 0x300) start = (start + 0x3ff) & ~0x3ff; start = (start + align - 1) & ~(align - 1); - if (align_resource) - return align_resource(dev, res, start, size, align); + host_bridge = pci_find_host_bridge(dev->bus); + + if (host_bridge->align_resource) + return host_bridge->align_resource(dev, res, + start, size, align); return start; } diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index fd2f03f..d390fc1 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -337,6 +337,4 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe) } #endif -struct pci_host_bridge *pci_find_host_bridge(struct pci_bus *bus); - #endif /* DRIVERS_PCI_H */ diff --git a/include/linux/pci.h b/include/linux/pci.h index e828e7b..980b7bc 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -412,9 +412,18 @@ struct pci_host_bridge { void (*release_fn)(struct pci_host_bridge *); void *release_data; unsigned int ignore_reset_delay:1; /* for entire hierarchy */ + /* Resource alignement requirements */ + resource_size_t (*align_resource)(struct pci_dev *dev, + const struct resource *res, + resource_size_t start, + resource_size_t size, + resource_size_t align); }; #define to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev) + +struct pci_host_bridge *pci_find_host_bridge(struct pci_bus *bus); + void pci_set_host_bridge_release(struct pci_host_bridge *bridge, void (*release_fn)(struct pci_host_bridge *), void *release_data);