[PATCH 0/2] ARM: pci: Prepare for Tegra PCIe controller driver

[PATCH 0/2] ARM: pci: Prepare for Tegra PCIe controller driver

[Thierry Reding]

Hi Russell,

These are two patches I've been carrying in a larger series that
converts the Tegra PCIe controller driver to a proper platform driver.
Since the complete series didn't get much feedback, I've begun to post
smaller subsets in an effort to get them merged more easily.

The first patch in this series converts the __init annotations for
pci_common_init() (and pcibios_init_hw() because it is called from the
former) to __devinit to make sure that they stay around after the init
stage. This is required because the Tegra driver depends on regulators
that become available only very late during boot and uses deferred
probing to handle this situation. It turned out that this postpones the
PCI bus initialization until after init, thus this patch.

The second patch is used to pass per-controller or per-host-bridge data
to the driver, such that it can be associated with the corresponding
bus. This is also required by the Tegra driver in order to pass a
driver-private structure to the PCI bus (or more precisely the
pci_sys_data structure associated with a bus). It is subsequently used
to obtain the root port private data given the corresponding PCI bus.

Thierry

Thierry Reding (2):
  ARM: pci: Keep pci_common_init() around after init
  ARM: pci: Allow passing per-controller private data

 arch/arm/include/asm/mach/pci.h | 1 +
 arch/arm/kernel/bios32.c        | 7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

-- 
1.7.12

[PATCH 1/2] ARM: pci: Keep pci_common_init() around after init

[Thierry Reding]

When using deferred driver probing, PCI host controller drivers may
actually require this function after the init stage.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 arch/arm/kernel/bios32.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 2b2f25e..7288093 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -423,7 +423,7 @@ static int pcibios_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 	return irq;
 }
 
-static void __init pcibios_init_hw(struct hw_pci *hw, struct list_head *head)
+static void __devinit pcibios_init_hw(struct hw_pci *hw, struct list_head *head)
 {
 	struct pci_sys_data *sys = NULL;
 	int ret;
@@ -472,7 +472,7 @@ static void __init pcibios_init_hw(struct hw_pci *hw, struct list_head *head)
 	}
 }
 
-void __init pci_common_init(struct hw_pci *hw)
+void __devinit pci_common_init(struct hw_pci *hw)
 {
 	struct pci_sys_data *sys;
 	LIST_HEAD(head);
-- 
1.7.12

Re: [PATCH 1/2] ARM: pci: Keep pci_common_init() around after init

[Bjorn Helgaas]

On Fri, Sep 14, 2012 at 3:11 PM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> When using deferred driver probing, PCI host controller drivers may
> actually require this function after the init stage.
>
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> ---
>  arch/arm/kernel/bios32.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index 2b2f25e..7288093 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -423,7 +423,7 @@ static int pcibios_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
>         return irq;
>  }
>
> -static void __init pcibios_init_hw(struct hw_pci *hw, struct list_head *head)
> +static void __devinit pcibios_init_hw(struct hw_pci *hw, struct list_head *head)

Shouldn't we just remove __init completely, since Greg KH is in the
process of removing __devinit?

>  {
>         struct pci_sys_data *sys = NULL;
>         int ret;
> @@ -472,7 +472,7 @@ static void __init pcibios_init_hw(struct hw_pci *hw, struct list_head *head)
>         }
>  }
>
> -void __init pci_common_init(struct hw_pci *hw)
> +void __devinit pci_common_init(struct hw_pci *hw)
>  {
>         struct pci_sys_data *sys;
>         LIST_HEAD(head);
> --
> 1.7.12
>

Re: [PATCH 1/2] ARM: pci: Keep pci_common_init() around after init

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]

On Tue, Sep 18, 2012 at 11:13:23AM -0600, Bjorn Helgaas wrote:
> On Fri, Sep 14, 2012 at 3:11 PM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> > When using deferred driver probing, PCI host controller drivers may
> > actually require this function after the init stage.
> >
> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> > ---
> >  arch/arm/kernel/bios32.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> > index 2b2f25e..7288093 100644
> > --- a/arch/arm/kernel/bios32.c
> > +++ b/arch/arm/kernel/bios32.c
> > @@ -423,7 +423,7 @@ static int pcibios_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
> >         return irq;
> >  }
> >
> > -static void __init pcibios_init_hw(struct hw_pci *hw, struct list_head *head)
> > +static void __devinit pcibios_init_hw(struct hw_pci *hw, struct list_head *head)
> 
> Shouldn't we just remove __init completely, since Greg KH is in the
> process of removing __devinit?

Yes, we probably should. In fact I have a patch on top of this marked
work-in-progress that I forgot to squash into this. That patch includes
the removal of __devinit for pcibios_update_irq() that should be covered
by a different patch I sent a few days ago as well as pcibios_swizzle()
and pcibios_init_resources(). I'll squash it into this patch and will
resend.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/2] ARM: pci: Allow passing per-controller private data

[Thierry Reding]

In order to allow drivers to specify private data for each controller,
this commit adds a private_data field to the struct hw_pci. This field
is an array of nr_controllers pointers that will be used to initialize
the private_data field of the corresponding controller's pci_sys_data
structure.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 arch/arm/include/asm/mach/pci.h | 1 +
 arch/arm/kernel/bios32.c        | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
index 26c511f..736cb8d 100644
--- a/arch/arm/include/asm/mach/pci.h
+++ b/arch/arm/include/asm/mach/pci.h
@@ -21,6 +21,7 @@ struct hw_pci {
 #endif
 	struct pci_ops	*ops;
 	int		nr_controllers;
+	void		**private_data;
 	int		(*setup)(int nr, struct pci_sys_data *);
 	struct pci_bus *(*scan)(int nr, struct pci_sys_data *);
 	void		(*preinit)(void);
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 7288093..07a38d8 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -442,6 +442,9 @@ static void __devinit pcibios_init_hw(struct hw_pci *hw, struct list_head *head)
 		sys->map_irq = hw->map_irq;
 		INIT_LIST_HEAD(&sys->resources);
 
+		if (hw->private_data)
+			sys->private_data = hw->private_data[nr];
+
 		ret = hw->setup(nr, sys);
 
 		if (ret > 0) {
-- 
1.7.12

Re: [PATCH 2/2] ARM: pci: Allow passing per-controller private data

[Bjorn Helgaas]

On Fri, Sep 14, 2012 at 3:11 PM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> In order to allow drivers to specify private data for each controller,
> this commit adds a private_data field to the struct hw_pci. This field
> is an array of nr_controllers pointers that will be used to initialize
> the private_data field of the corresponding controller's pci_sys_data
> structure.

I guess you aren't changing the design here because struct hw_pci
already includes "nr_controllers," but having nr_controllers and a
private_data[] array sounds like something that might make it hard to
hot-add a host bridge after boot.

> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> ---
>  arch/arm/include/asm/mach/pci.h | 1 +
>  arch/arm/kernel/bios32.c        | 3 +++
>  2 files changed, 4 insertions(+)
>
> diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
> index 26c511f..736cb8d 100644
> --- a/arch/arm/include/asm/mach/pci.h
> +++ b/arch/arm/include/asm/mach/pci.h
> @@ -21,6 +21,7 @@ struct hw_pci {
>  #endif
>         struct pci_ops  *ops;
>         int             nr_controllers;
> +       void            **private_data;
>         int             (*setup)(int nr, struct pci_sys_data *);
>         struct pci_bus *(*scan)(int nr, struct pci_sys_data *);
>         void            (*preinit)(void);
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index 7288093..07a38d8 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -442,6 +442,9 @@ static void __devinit pcibios_init_hw(struct hw_pci *hw, struct list_head *head)
>                 sys->map_irq = hw->map_irq;
>                 INIT_LIST_HEAD(&sys->resources);
>
> +               if (hw->private_data)
> +                       sys->private_data = hw->private_data[nr];
> +
>                 ret = hw->setup(nr, sys);
>
>                 if (ret > 0) {
> --
> 1.7.12
>

Re: [PATCH 2/2] ARM: pci: Allow passing per-controller private data

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2217 bytes --]

On Tue, Sep 18, 2012 at 11:21:21AM -0600, Bjorn Helgaas wrote:
> On Fri, Sep 14, 2012 at 3:11 PM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> > In order to allow drivers to specify private data for each controller,
> > this commit adds a private_data field to the struct hw_pci. This field
> > is an array of nr_controllers pointers that will be used to initialize
> > the private_data field of the corresponding controller's pci_sys_data
> > structure.
> 
> I guess you aren't changing the design here because struct hw_pci
> already includes "nr_controllers," but having nr_controllers and a
> private_data[] array sounds like something that might make it hard to
> hot-add a host bridge after boot.

What I do in the Tegra PCIe driver is to pass each of the root ports to
pci_common_init() individually because they can be enabled or disabled
by device tree data. I suppose to some degree you can consider that hot-
adding. Not that Tegra is likely to ever need to support that. I don't
know how likely it is for any ARM platform to ever need support for hot-
adding a host bridge.

Eventually I think it would be advantageous for this to be generalized
further such that PCI initialization can be shared across architectures.
That's probably not an easy task so I was going to start by making
incremental changes that enable the Tegra code to work and, if time
allows, help further with subsequent improvements.

It also seems that parts of the PCI core aren't ready yet for hot-adding
host bridges. One thing I came across while working on the Tegra code is
that MSI setup and teardown needs to be done by the arch_setup_msi_irq()
and arch_teardown_msi_irq() respectively, which are expected to be
builtin. That was also the last issue that keeps the Tegra PCIe driver
from being built as a module. I think that will also make it impossible
to hot-add host bridges. On x86 this seems to be handled by platform
code, but on Tegra for example MSI setup and teardown is tightly coupled
to the PCIe controller. That was one of the things I thought I could
take a look at eventually, but getting Tegra support cleaned up is
higher priority right now.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] ARM: pci: Allow passing per-controller private data

[Bjorn Helgaas]

On Tue, Sep 18, 2012 at 12:34 PM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> On Tue, Sep 18, 2012 at 11:21:21AM -0600, Bjorn Helgaas wrote:
>> On Fri, Sep 14, 2012 at 3:11 PM, Thierry Reding
>> <thierry.reding@avionic-design.de> wrote:
>> > In order to allow drivers to specify private data for each controller,
>> > this commit adds a private_data field to the struct hw_pci. This field
>> > is an array of nr_controllers pointers that will be used to initialize
>> > the private_data field of the corresponding controller's pci_sys_data
>> > structure.
>>
>> I guess you aren't changing the design here because struct hw_pci
>> already includes "nr_controllers," but having nr_controllers and a
>> private_data[] array sounds like something that might make it hard to
>> hot-add a host bridge after boot.
>
> What I do in the Tegra PCIe driver is to pass each of the root ports to
> pci_common_init() individually because they can be enabled or disabled
> by device tree data. I suppose to some degree you can consider that hot-
> adding. Not that Tegra is likely to ever need to support that. I don't
> know how likely it is for any ARM platform to ever need support for hot-
> adding a host bridge.
>
> Eventually I think it would be advantageous for this to be generalized
> further such that PCI initialization can be shared across architectures.
> That's probably not an easy task so I was going to start by making
> incremental changes that enable the Tegra code to work and, if time
> allows, help further with subsequent improvements.
>
> It also seems that parts of the PCI core aren't ready yet for hot-adding
> host bridges. One thing I came across while working on the Tegra code is
> that MSI setup and teardown needs to be done by the arch_setup_msi_irq()
> and arch_teardown_msi_irq() respectively, which are expected to be
> builtin. That was also the last issue that keeps the Tegra PCIe driver
> from being built as a module. I think that will also make it impossible
> to hot-add host bridges. On x86 this seems to be handled by platform
> code, but on Tegra for example MSI setup and teardown is tightly coupled
> to the PCIe controller. That was one of the things I thought I could
> take a look at eventually, but getting Tegra support cleaned up is
> higher priority right now.

The PCI core doesn't support hot-add of host bridges yet, but there's
a lot of work in that area right now, which is why it's on my mind :)
I'm not suggesting you change anything here; just keep it in the back
of your mind for the future.  Thanks for the arch_setup_msi_irq() tip;
I'll look into that.

Bjorn

Re: [PATCH 2/2] ARM: pci: Allow passing per-controller private data

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3404 bytes --]

On Tue, Sep 18, 2012 at 12:38:53PM -0600, Bjorn Helgaas wrote:
> On Tue, Sep 18, 2012 at 12:34 PM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> > On Tue, Sep 18, 2012 at 11:21:21AM -0600, Bjorn Helgaas wrote:
> >> On Fri, Sep 14, 2012 at 3:11 PM, Thierry Reding
> >> <thierry.reding@avionic-design.de> wrote:
> >> > In order to allow drivers to specify private data for each controller,
> >> > this commit adds a private_data field to the struct hw_pci. This field
> >> > is an array of nr_controllers pointers that will be used to initialize
> >> > the private_data field of the corresponding controller's pci_sys_data
> >> > structure.
> >>
> >> I guess you aren't changing the design here because struct hw_pci
> >> already includes "nr_controllers," but having nr_controllers and a
> >> private_data[] array sounds like something that might make it hard to
> >> hot-add a host bridge after boot.
> >
> > What I do in the Tegra PCIe driver is to pass each of the root ports to
> > pci_common_init() individually because they can be enabled or disabled
> > by device tree data. I suppose to some degree you can consider that hot-
> > adding. Not that Tegra is likely to ever need to support that. I don't
> > know how likely it is for any ARM platform to ever need support for hot-
> > adding a host bridge.
> >
> > Eventually I think it would be advantageous for this to be generalized
> > further such that PCI initialization can be shared across architectures.
> > That's probably not an easy task so I was going to start by making
> > incremental changes that enable the Tegra code to work and, if time
> > allows, help further with subsequent improvements.
> >
> > It also seems that parts of the PCI core aren't ready yet for hot-adding
> > host bridges. One thing I came across while working on the Tegra code is
> > that MSI setup and teardown needs to be done by the arch_setup_msi_irq()
> > and arch_teardown_msi_irq() respectively, which are expected to be
> > builtin. That was also the last issue that keeps the Tegra PCIe driver
> > from being built as a module. I think that will also make it impossible
> > to hot-add host bridges. On x86 this seems to be handled by platform
> > code, but on Tegra for example MSI setup and teardown is tightly coupled
> > to the PCIe controller. That was one of the things I thought I could
> > take a look at eventually, but getting Tegra support cleaned up is
> > higher priority right now.
> 
> The PCI core doesn't support hot-add of host bridges yet, but there's
> a lot of work in that area right now, which is why it's on my mind :)
> I'm not suggesting you change anything here; just keep it in the back
> of your mind for the future.  Thanks for the arch_setup_msi_irq() tip;
> I'll look into that.

I think it wouldn't even be that hard to implement. When I last looked
at this it seemed like a simple registration mechanism in the core
should work. That is the core would keep a list of registered host
bridges, each of which would implement setup_msi() and teardown_msi()
operations. Then, whenever an MSI is requested, the core would look up
the corresponding host bridge and call the associated setup_msi()
callback. Perhaps these could go into struct pci_ops. It already has
pointers for configuration space access, which is also controller-
specific.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]