| Message ID | 1373889167-27878-9-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hello Russell, Would it be possible to have your opinion on the below patch? It has already been sent on June, 19th and July, 1st, and I'd really like to see it merged for 3.12, as part of this series enabling MSI support for the Marvell PCIe driver. Thanks! Thomas On Mon, 15 Jul 2013 13:52:44 +0200, Thomas Petazzoni wrote: > Some PCI drivers may need to adjust the pci_bus structure after it has > been allocated by the Linux PCI core. The PCI core allows > architectures to implement the pcibios_add_bus() and > pcibios_remove_bus() for this purpose. This commit therefore extends > the hw_pci and pci_sys_data structures of the ARM PCI core to allow > PCI drivers to register ->add_bus() and ->remove_bus() in hw_pci, > which will get called when a bus is added or removed from the system. > > This will be used for example by the Marvell PCIe driver to connect a > particular PCI bus with its corresponding MSI chip to handle Message > Signaled Interrupts. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > Reviewed-by: Thierry Reding <thierry.reding@gmail.com> > --- > arch/arm/include/asm/mach/pci.h | 4 ++++ > arch/arm/kernel/bios32.c | 16 ++++++++++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h > index a1c90d7..487155c 100644 > --- a/arch/arm/include/asm/mach/pci.h > +++ b/arch/arm/include/asm/mach/pci.h > @@ -36,6 +36,8 @@ struct hw_pci { > resource_size_t start, > resource_size_t size, > resource_size_t align); > + void (*add_bus)(struct pci_bus *bus); > + void (*remove_bus)(struct pci_bus *bus); > }; > > /* > @@ -63,6 +65,8 @@ struct pci_sys_data { > resource_size_t start, > resource_size_t size, > resource_size_t align); > + void (*add_bus)(struct pci_bus *bus); > + void (*remove_bus)(struct pci_bus *bus); > void *private_data; /* platform controller private data */ > }; > > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c > index 261fcc8..530e59d 100644 > --- a/arch/arm/kernel/bios32.c > +++ b/arch/arm/kernel/bios32.c > @@ -363,6 +363,20 @@ void pcibios_fixup_bus(struct pci_bus *bus) > } > EXPORT_SYMBOL(pcibios_fixup_bus); > > +void pcibios_add_bus(struct pci_bus *bus) > +{ > + struct pci_sys_data *sys = bus->sysdata; > + if (sys->add_bus) > + sys->add_bus(bus); > +} > + > +void pcibios_remove_bus(struct pci_bus *bus) > +{ > + struct pci_sys_data *sys = bus->sysdata; > + if (sys->remove_bus) > + sys->remove_bus(bus); > +} > + > /* > * Swizzle the device pin each time we cross a bridge. If a platform does > * not provide a swizzle function, we perform the standard PCI swizzling. > @@ -464,6 +478,8 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, > sys->swizzle = hw->swizzle; > sys->map_irq = hw->map_irq; > sys->align_resource = hw->align_resource; > + sys->add_bus = hw->add_bus; > + sys->remove_bus = hw->remove_bus; > INIT_LIST_HEAD(&sys->resources); > > if (hw->private_data)
On Mon, Jul 15, 2013 at 01:52:44PM +0200, Thomas Petazzoni wrote: [...] Hi Thomas, I just noticed two minor issues, see below. > diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h [...] > @@ -36,6 +36,8 @@ struct hw_pci { > resource_size_t start, > resource_size_t size, > resource_size_t align); > + void (*add_bus)(struct pci_bus *bus); > + void (*remove_bus)(struct pci_bus *bus); This hunk... > @@ -63,6 +65,8 @@ struct pci_sys_data { > resource_size_t start, > resource_size_t size, > resource_size_t align); > + void (*add_bus)(struct pci_bus *bus); > + void (*remove_bus)(struct pci_bus *bus); > void *private_data; /* platform controller private data */ > }; and this one use inconsistent indentation (spaces instead of tabs). > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c [...] > @@ -464,6 +478,8 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, > sys->swizzle = hw->swizzle; > sys->map_irq = hw->map_irq; > sys->align_resource = hw->align_resource; > + sys->add_bus = hw->add_bus; > + sys->remove_bus = hw->remove_bus; And this one aligns the '=' sign, whereas none of the other assignments do. Might be nice to fix those up before sending them to Russell's patch tracker. Thierry
On Mon, Jul 15, 2013 at 01:52:44PM +0200, Thomas Petazzoni wrote: > Some PCI drivers may need to adjust the pci_bus structure after it has > been allocated by the Linux PCI core. The PCI core allows > architectures to implement the pcibios_add_bus() and > pcibios_remove_bus() for this purpose. This commit therefore extends > the hw_pci and pci_sys_data structures of the ARM PCI core to allow > PCI drivers to register ->add_bus() and ->remove_bus() in hw_pci, > which will get called when a bus is added or removed from the system. > > This will be used for example by the Marvell PCIe driver to connect a > particular PCI bus with its corresponding MSI chip to handle Message > Signaled Interrupts. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > Reviewed-by: Thierry Reding <thierry.reding@gmail.com> > --- > arch/arm/include/asm/mach/pci.h | 4 ++++ > arch/arm/kernel/bios32.c | 16 ++++++++++++++++ > 2 files changed, 20 insertions(+) Hi Russell, Can we have your Acked-by on this patch so that it can be moved to a stable branch along with some of the other patches in this branch that both Marvell and Tegra PCIe drivers need? Thanks, Thierry > > diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h > index a1c90d7..487155c 100644 > --- a/arch/arm/include/asm/mach/pci.h > +++ b/arch/arm/include/asm/mach/pci.h > @@ -36,6 +36,8 @@ struct hw_pci { > resource_size_t start, > resource_size_t size, > resource_size_t align); > + void (*add_bus)(struct pci_bus *bus); > + void (*remove_bus)(struct pci_bus *bus); > }; > > /* > @@ -63,6 +65,8 @@ struct pci_sys_data { > resource_size_t start, > resource_size_t size, > resource_size_t align); > + void (*add_bus)(struct pci_bus *bus); > + void (*remove_bus)(struct pci_bus *bus); > void *private_data; /* platform controller private data */ > }; > > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c > index 261fcc8..530e59d 100644 > --- a/arch/arm/kernel/bios32.c > +++ b/arch/arm/kernel/bios32.c > @@ -363,6 +363,20 @@ void pcibios_fixup_bus(struct pci_bus *bus) > } > EXPORT_SYMBOL(pcibios_fixup_bus); > > +void pcibios_add_bus(struct pci_bus *bus) > +{ > + struct pci_sys_data *sys = bus->sysdata; > + if (sys->add_bus) > + sys->add_bus(bus); > +} > + > +void pcibios_remove_bus(struct pci_bus *bus) > +{ > + struct pci_sys_data *sys = bus->sysdata; > + if (sys->remove_bus) > + sys->remove_bus(bus); > +} > + > /* > * Swizzle the device pin each time we cross a bridge. If a platform does > * not provide a swizzle function, we perform the standard PCI swizzling. > @@ -464,6 +478,8 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, > sys->swizzle = hw->swizzle; > sys->map_irq = hw->map_irq; > sys->align_resource = hw->align_resource; > + sys->add_bus = hw->add_bus; > + sys->remove_bus = hw->remove_bus; > INIT_LIST_HEAD(&sys->resources); > > if (hw->private_data) > -- > 1.8.1.2 >
Dear Thierry Reding, On Thu, 25 Jul 2013 09:57:00 -0700, Thierry Reding wrote: > On Mon, Jul 15, 2013 at 01:52:44PM +0200, Thomas Petazzoni wrote: > > Some PCI drivers may need to adjust the pci_bus structure after it has > > been allocated by the Linux PCI core. The PCI core allows > > architectures to implement the pcibios_add_bus() and > > pcibios_remove_bus() for this purpose. This commit therefore extends > > the hw_pci and pci_sys_data structures of the ARM PCI core to allow > > PCI drivers to register ->add_bus() and ->remove_bus() in hw_pci, > > which will get called when a bus is added or removed from the system. > > > > This will be used for example by the Marvell PCIe driver to connect a > > particular PCI bus with its corresponding MSI chip to handle Message > > Signaled Interrupts. > > > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > Reviewed-by: Thierry Reding <thierry.reding@gmail.com> > > --- > > arch/arm/include/asm/mach/pci.h | 4 ++++ > > arch/arm/kernel/bios32.c | 16 ++++++++++++++++ > > 2 files changed, 20 insertions(+) > > Hi Russell, > > Can we have your Acked-by on this patch so that it can be moved to a > stable branch along with some of the other patches in this branch that > both Marvell and Tegra PCIe drivers need? I've pinged Russell a few times about this patch already, but haven't received feedback from him. Do you think I can just go ahead and submit the patch to Russell's patch system, after fixing the minor nits you mentioned? Thanks, Thomas
On Fri, Jul 26, 2013 at 10:13:05AM +0200, Thomas Petazzoni wrote: > Dear Thierry Reding, > > On Thu, 25 Jul 2013 09:57:00 -0700, Thierry Reding wrote: > > On Mon, Jul 15, 2013 at 01:52:44PM +0200, Thomas Petazzoni wrote: > > > Some PCI drivers may need to adjust the pci_bus structure after it has > > > been allocated by the Linux PCI core. The PCI core allows > > > architectures to implement the pcibios_add_bus() and > > > pcibios_remove_bus() for this purpose. This commit therefore extends > > > the hw_pci and pci_sys_data structures of the ARM PCI core to allow > > > PCI drivers to register ->add_bus() and ->remove_bus() in hw_pci, > > > which will get called when a bus is added or removed from the system. > > > > > > This will be used for example by the Marvell PCIe driver to connect a > > > particular PCI bus with its corresponding MSI chip to handle Message > > > Signaled Interrupts. > > > > > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > > Reviewed-by: Thierry Reding <thierry.reding@gmail.com> > > > --- > > > arch/arm/include/asm/mach/pci.h | 4 ++++ > > > arch/arm/kernel/bios32.c | 16 ++++++++++++++++ > > > 2 files changed, 20 insertions(+) > > > > Hi Russell, > > > > Can we have your Acked-by on this patch so that it can be moved to a > > stable branch along with some of the other patches in this branch that > > both Marvell and Tegra PCIe drivers need? > > I've pinged Russell a few times about this patch already, but haven't > received feedback from him. Do you think I can just go ahead and submit > the patch to Russell's patch system, after fixing the minor nits you > mentioned? When you do that, please ask Russell to place it in a publicly accessible, stable topic branch for us to base off of. thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 25, 2013 at 09:57:00AM -0700, Thierry Reding wrote: > On Mon, Jul 15, 2013 at 01:52:44PM +0200, Thomas Petazzoni wrote: > > Some PCI drivers may need to adjust the pci_bus structure after it has > > been allocated by the Linux PCI core. The PCI core allows > > architectures to implement the pcibios_add_bus() and > > pcibios_remove_bus() for this purpose. This commit therefore extends > > the hw_pci and pci_sys_data structures of the ARM PCI core to allow > > PCI drivers to register ->add_bus() and ->remove_bus() in hw_pci, > > which will get called when a bus is added or removed from the system. > > > > This will be used for example by the Marvell PCIe driver to connect a > > particular PCI bus with its corresponding MSI chip to handle Message > > Signaled Interrupts. > > > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > Reviewed-by: Thierry Reding <thierry.reding@gmail.com> > > --- > > arch/arm/include/asm/mach/pci.h | 4 ++++ > > arch/arm/kernel/bios32.c | 16 ++++++++++++++++ > > 2 files changed, 20 insertions(+) > > Hi Russell, > > Can we have your Acked-by on this patch so that it can be moved to a > stable branch along with some of the other patches in this branch that > both Marvell and Tegra PCIe drivers need? Acked-by: Russell King <rmk+kernel@arm.linux.org.uk> -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 26, 2013 at 11:46:07PM +0100, Russell King - ARM Linux wrote: > On Thu, Jul 25, 2013 at 09:57:00AM -0700, Thierry Reding wrote: > > On Mon, Jul 15, 2013 at 01:52:44PM +0200, Thomas Petazzoni wrote: > > > Some PCI drivers may need to adjust the pci_bus structure after it has > > > been allocated by the Linux PCI core. The PCI core allows > > > architectures to implement the pcibios_add_bus() and > > > pcibios_remove_bus() for this purpose. This commit therefore extends > > > the hw_pci and pci_sys_data structures of the ARM PCI core to allow > > > PCI drivers to register ->add_bus() and ->remove_bus() in hw_pci, > > > which will get called when a bus is added or removed from the system. > > > > > > This will be used for example by the Marvell PCIe driver to connect a > > > particular PCI bus with its corresponding MSI chip to handle Message > > > Signaled Interrupts. > > > > > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > > Reviewed-by: Thierry Reding <thierry.reding@gmail.com> > > > --- > > > arch/arm/include/asm/mach/pci.h | 4 ++++ > > > arch/arm/kernel/bios32.c | 16 ++++++++++++++++ > > > 2 files changed, 20 insertions(+) > > > > Hi Russell, > > > > Can we have your Acked-by on this patch so that it can be moved to a > > stable branch along with some of the other patches in this branch that > > both Marvell and Tegra PCIe drivers need? > > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk> Thanks, Russell. Thierry
Dear Thierry Reding, On Thu, 25 Jul 2013 09:53:03 -0700, Thierry Reding wrote: > I just noticed two minor issues, see below. > > > diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h > [...] > > @@ -36,6 +36,8 @@ struct hw_pci { > > resource_size_t start, > > resource_size_t size, > > resource_size_t align); > > + void (*add_bus)(struct pci_bus *bus); > > + void (*remove_bus)(struct pci_bus *bus); > > This hunk... > > > @@ -63,6 +65,8 @@ struct pci_sys_data { > > resource_size_t start, > > resource_size_t size, > > resource_size_t align); > > + void (*add_bus)(struct pci_bus *bus); > > + void (*remove_bus)(struct pci_bus *bus); > > void *private_data; /* platform controller private data */ > > }; > > and this one use inconsistent indentation (spaces instead of tabs). Hum, I must be missing something here. Looking at the patch you're quoting, and my code, I see one tab before 'void' in both cases, and spaces between 'void' and the (*function) in both cases. Am I missing something here? > > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c > [...] > > @@ -464,6 +478,8 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, > > sys->swizzle = hw->swizzle; > > sys->map_irq = hw->map_irq; > > sys->align_resource = hw->align_resource; > > + sys->add_bus = hw->add_bus; > > + sys->remove_bus = hw->remove_bus; > > And this one aligns the '=' sign, whereas none of the other assignments > do. Right, will be fixed as part of PATCHv7. Thanks, Thomas
On Wed, Aug 07, 2013 at 11:13:54AM +0200, Thomas Petazzoni wrote: > Dear Thierry Reding, > > On Thu, 25 Jul 2013 09:53:03 -0700, Thierry Reding wrote: > > > I just noticed two minor issues, see below. > > > > > diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h > > [...] > > > @@ -36,6 +36,8 @@ struct hw_pci { > > > resource_size_t start, > > > resource_size_t size, > > > resource_size_t align); > > > + void (*add_bus)(struct pci_bus *bus); > > > + void (*remove_bus)(struct pci_bus *bus); > > > > This hunk... > > > > > @@ -63,6 +65,8 @@ struct pci_sys_data { > > > resource_size_t start, > > > resource_size_t size, > > > resource_size_t align); > > > + void (*add_bus)(struct pci_bus *bus); > > > + void (*remove_bus)(struct pci_bus *bus); > > > void *private_data; /* platform controller private data */ > > > }; > > > > and this one use inconsistent indentation (spaces instead of tabs). > > Hum, I must be missing something here. Looking at the patch you're > quoting, and my code, I see one tab before 'void' in both cases, and > spaces between 'void' and the (*function) in both cases. > > Am I missing something here? Perhaps I should've said "alignment" instead of "indentation" to make it more explicit. All other members use tabs to separate the data type and the field name, while the lines that you add use spaces. Thierry
Dear Thierry Reding, On Wed, 7 Aug 2013 15:37:20 +0200, Thierry Reding wrote: > > Hum, I must be missing something here. Looking at the patch you're > > quoting, and my code, I see one tab before 'void' in both cases, and > > spaces between 'void' and the (*function) in both cases. > > > > Am I missing something here? > > Perhaps I should've said "alignment" instead of "indentation" to make it > more explicit. All other members use tabs to separate the data type and > the field name, while the lines that you add use spaces. Ah, right, I understand now. Unfortunately, I've already sent PATCHv7 with some other fixes. So either Jason Cooper can fix this when applying, or I'll fix it if a PATCHv8 is needed for some other reason. Thanks! Thomas
diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h index a1c90d7..487155c 100644 --- a/arch/arm/include/asm/mach/pci.h +++ b/arch/arm/include/asm/mach/pci.h @@ -36,6 +36,8 @@ struct hw_pci { resource_size_t start, resource_size_t size, resource_size_t align); + void (*add_bus)(struct pci_bus *bus); + void (*remove_bus)(struct pci_bus *bus); }; /* @@ -63,6 +65,8 @@ struct pci_sys_data { resource_size_t start, resource_size_t size, resource_size_t align); + void (*add_bus)(struct pci_bus *bus); + void (*remove_bus)(struct pci_bus *bus); void *private_data; /* platform controller private data */ }; diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c index 261fcc8..530e59d 100644 --- a/arch/arm/kernel/bios32.c +++ b/arch/arm/kernel/bios32.c @@ -363,6 +363,20 @@ void pcibios_fixup_bus(struct pci_bus *bus) } EXPORT_SYMBOL(pcibios_fixup_bus); +void pcibios_add_bus(struct pci_bus *bus) +{ + struct pci_sys_data *sys = bus->sysdata; + if (sys->add_bus) + sys->add_bus(bus); +} + +void pcibios_remove_bus(struct pci_bus *bus) +{ + struct pci_sys_data *sys = bus->sysdata; + if (sys->remove_bus) + sys->remove_bus(bus); +} + /* * Swizzle the device pin each time we cross a bridge. If a platform does * not provide a swizzle function, we perform the standard PCI swizzling. @@ -464,6 +478,8 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, sys->swizzle = hw->swizzle; sys->map_irq = hw->map_irq; sys->align_resource = hw->align_resource; + sys->add_bus = hw->add_bus; + sys->remove_bus = hw->remove_bus; INIT_LIST_HEAD(&sys->resources); if (hw->private_data)