Patchwork [PATCHv5,08/11] ARM: pci: add ->add_bus() and ->remove_bus() hooks to hw_pci

login
register
mail settings
Submitter Thomas Petazzoni
Date July 15, 2013, 11:52 a.m.
Message ID <1373889167-27878-9-git-send-email-thomas.petazzoni@free-electrons.com>
Download mbox | patch
Permalink /patch/259026/
State Not Applicable
Headers show

Comments

Thomas Petazzoni - July 15, 2013, 11:52 a.m.
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(+)
Thomas Petazzoni - July 16, 2013, 8:29 a.m.
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)
Thierry Reding - July 25, 2013, 4:53 p.m.
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
Thierry Reding - July 25, 2013, 4:57 p.m.
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
>
Thomas Petazzoni - July 26, 2013, 8:13 a.m.
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
Jason - July 26, 2013, 11:49 a.m.
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
Russell King - ARM Linux - July 26, 2013, 10:46 p.m.
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
Thierry Reding - July 29, 2013, 12:31 p.m.
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
Thomas Petazzoni - Aug. 7, 2013, 9:13 a.m.
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
Thierry Reding - Aug. 7, 2013, 1:37 p.m.
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
Thomas Petazzoni - Aug. 7, 2013, 3:06 p.m.
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

Patch

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)