[PATCH] ARM: pci: pass a parent to pci_common_init()

[PATCH] ARM: pci: pass a parent to pci_common_init()

[Arnd Bergmann]

On Friday 12 April 2013, Linus Walleij wrote:
> When working with device tree support for PCI on ARM you run
> into a problem when mapping IRQs from the device tree irqmaps:
> doing this the code in drivers/of/of_pci_irq.c will try to
> find the OF node on the root bridge and this fails, because
> bus->dev.of_node is NULL, and that in turn boils down to
> the fact that pci_set_bus_of_node() has called
> pcibios_get_phb_of_node() from drivers/pci/of.c to obtain
> the OF node of the bridge or its parent and none is set
> and thus NULL is returned.
> 
> Fix this by adding an additional parent argument when
> registering PCI bridges on the ARM architecture using the
> pci_common_init() call, and pass along this parent to
> pci_scan_root_bus() called from pcibios_init_hw() in
> bios32.c and voila: the IRQ mappings start working:
> the OF node can be retrieved from the parent.
> 
> Currently all users are set to use NULL as argument to this
> call, but subsequent patches to the Integrator/AP make use
> of this facility to pass a parent.

Could you try making it a less invasive patch by renaming
pci_common_init to something else and providing a wrapper
like this?

static inline void pci_common_init(struct hw_pci *hw)
{
	pci_common_init_dev(NULL, hw);
}

That would avoid most of the churn and get the same result.

	Arnd

[PATCH] ARM: pci: pass a parent to pci_common_init()

[Andrew Murray]

On Fri, Apr 12, 2013 at 12:26:55PM +0100, Arnd Bergmann wrote:
> On Friday 12 April 2013, Linus Walleij wrote:
> > When working with device tree support for PCI on ARM you run
> > into a problem when mapping IRQs from the device tree irqmaps:
> > doing this the code in drivers/of/of_pci_irq.c will try to
> > find the OF node on the root bridge and this fails, because
> > bus->dev.of_node is NULL, and that in turn boils down to
> > the fact that pci_set_bus_of_node() has called
> > pcibios_get_phb_of_node() from drivers/pci/of.c to obtain
> > the OF node of the bridge or its parent and none is set
> > and thus NULL is returned.
> > 
> > Fix this by adding an additional parent argument when
> > registering PCI bridges on the ARM architecture using the
> > pci_common_init() call, and pass along this parent to
> > pci_scan_root_bus() called from pcibios_init_hw() in
> > bios32.c and voila: the IRQ mappings start working:
> > the OF node can be retrieved from the parent.
> > 
> > Currently all users are set to use NULL as argument to this
> > call, but subsequent patches to the Integrator/AP make use
> > of this facility to pass a parent.
> 
> Could you try making it a less invasive patch by renaming
> pci_common_init to something else and providing a wrapper
> like this?
> 
> static inline void pci_common_init(struct hw_pci *hw)
> {
> 	pci_common_init_dev(NULL, hw);
> }
> 
> That would avoid most of the churn and get the same result.
> 
> 	Arnd
>

I would also be happy with this. Though alternatively could you
not just add a struct device to hw_pci? Then change pcibios_init_hw
to use hw->device instead of NULL?

Andrew Murray

[PATCH] ARM: pci: pass a parent to pci_common_init()

[Arnd Bergmann]

On Friday 12 April 2013, Andrew Murray wrote:
> I would also be happy with this. Though alternatively could you
> not just add a struct device to hw_pci? Then change pcibios_init_hw
> to use hw->device instead of NULL?
> 

I think struct hw_pci is meant to be statically defined, so that would
not work if you need the same hw_pci for two host bridge devices.

	Arnd

[PATCH] ARM: pci: pass a parent to pci_common_init()

[Andrew Murray]

On Fri, Apr 12, 2013 at 02:01:48PM +0100, Arnd Bergmann wrote:
> On Friday 12 April 2013, Andrew Murray wrote:
> > I would also be happy with this. Though alternatively could you
> > not just add a struct device to hw_pci? Then change pcibios_init_hw
> > to use hw->device instead of NULL?
> > 
> 
> I think struct hw_pci is meant to be statically defined, so that would
> not work if you need the same hw_pci for two host bridge devices.
> 
> 	Arnd
> 

Yes, though the new PCIe host/bridge drivers with DT support all seem to call
pci_common_init at probe with a non-statically defined hw_pci with the
nr_controllers always set to 1 (meaning that pci_common_init can be called
multiple times). Also hw_pci now has a field for private_data anyway which
breaks this assumption...

Is there an argument for removing hw_pci or at least .nr_controllers and call
pci_common_init multiple times (as is potential current use case for DT PCI
drivers)?

Andrew Murray

[PATCH] ARM: pci: pass a parent to pci_common_init()

[Russell King - ARM Linux]

On Fri, Apr 12, 2013 at 03:01:48PM +0200, Arnd Bergmann wrote:
> On Friday 12 April 2013, Andrew Murray wrote:
> > I would also be happy with this. Though alternatively could you
> > not just add a struct device to hw_pci? Then change pcibios_init_hw
> > to use hw->device instead of NULL?
> > 
> 
> I think struct hw_pci is meant to be statically defined, so that would
> not work if you need the same hw_pci for two host bridge devices.

The hw_pci structure is only referenced by the setup functions, and
never referred to again.  Anything of any relevance that is needed
later is copied out of that structure into others (like pci_sys_data).

[PATCH] ARM: pci: pass a parent to pci_common_init()

[Russell King - ARM Linux]

On Fri, Apr 12, 2013 at 02:48:11PM +0100, Andrew Murray wrote:
> Yes, though the new PCIe host/bridge drivers with DT support all seem to call
> pci_common_init at probe with a non-statically defined hw_pci with the
> nr_controllers always set to 1 (meaning that pci_common_init can be called
> multiple times). Also hw_pci now has a field for private_data anyway which
> breaks this assumption...
> 
> Is there an argument for removing hw_pci or at least .nr_controllers and call
> pci_common_init multiple times (as is potential current use case for DT PCI
> drivers)?

Well, I notice that there's bunches of code which have been introduced
post my cleanup of the PCI stuff which insist on doing their stuff using
the pre-cleanup ways...  I guess I need to go through and fix peoples
stuff up again in this area when I finish reading the mail backlog...

The idea behind bios32.c is that all buses get scanned and _then_ setup,
rather than each get scanned and setup in sequence.  This allows for
a variety of setups where the buses may share a space - it allows the
scan to be done, then the postinit hook to be called which can then do
any post-processing prior to resources being setup.