Patchwork pci: Add support for creating a generic host_bridge from device tree

login
register
mail settings
Submitter Arnd Bergmann
Date Feb. 13, 2014, 11:27 a.m.
Message ID <3204351.WykFFcX4zJ@wuerfel>
Download mbox | patch
Permalink /patch/319961/
State Not Applicable
Headers show

Comments

Arnd Bergmann - Feb. 13, 2014, 11:27 a.m.
On Thursday 13 February 2014 17:57:41 Jingoo Han wrote:
> I want to use 'drivers/pci/host/pcie-designware.c' for both arm32
> and arm64, without any code changes. However, it looks impossible.

It is impossible at the moment, and I agree we have to fix that.

> I made 'drivers/pci/host/pcie-designware.c' based on 32bit arm PCI
> support. Then, with Liviu's patch, do I have to make new code for arm64,
> even though the same HW PCIe IP is used?
> 
> - For arm32
>   drivers/pci/host/pcie-designware.c
> 
> - For arm64
>   drivers/pci/host/pcie-designware-arm64.c

As a start, I'd suggest using "#ifdef CONFIG_ARM" in the driver,
but sharing as much code as you can. We should try to make the #else
section of the #ifdef architecture independent and get have the arm64
implementation shared with any architecture that doesn't have or want
its own pcibios32.c implementation.

> > > I am reviewing and compiling your patch.
> > > Would you consider adding 'struct pci_sys_data' and 'struct hw_pci'?

I would rather get rid of struct hw_pci for architecture independent
drivers and add a different registration method on arm32 that is
compatible with what we come up with on arm64. The main purpose of
hw_pci is to allow multiple PCI controllers to be initialized at
once, but we don't actually need that for any of the "modern" platforms
where we already have a probe function that gets called once for
each controller.

As a start, we could add a pci_host_bridge_register() function like
the one below to arm32 and migrate the drivers/pci/host/ drivers
over to use it with little effort. Instead of filling out hw_pci,
these drivers would allocate (by embedding in their device struct)
and fill out pci_sys_data directly. After that, we can gradually
move more code out of the arm32 implementation into common code, if
it doesn't already exist there, up to the point where a host driver
no longer has to call any function in bios32.c.

	Arnd


--
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 - Feb. 13, 2014, 11:53 a.m.
On Thu, Feb 13, 2014 at 12:27:05PM +0100, Arnd Bergmann wrote:
> I would rather get rid of struct hw_pci for architecture independent
> drivers and add a different registration method on arm32 that is
> compatible with what we come up with on arm64. The main purpose of
> hw_pci is to allow multiple PCI controllers to be initialized at
> once, but we don't actually need that for any of the "modern" platforms
> where we already have a probe function that gets called once for
> each controller.

No.  The main purpose of hw_pci is as a container to support multiple
different platform specific PCI implementations in one kernel.  It's
exactly what you need for single zImage.
Arnd Bergmann - Feb. 13, 2014, 12:15 p.m.
On Thursday 13 February 2014 11:53:27 Russell King - ARM Linux wrote:
> On Thu, Feb 13, 2014 at 12:27:05PM +0100, Arnd Bergmann wrote:
> > I would rather get rid of struct hw_pci for architecture independent
> > drivers and add a different registration method on arm32 that is
> > compatible with what we come up with on arm64. The main purpose of
> > hw_pci is to allow multiple PCI controllers to be initialized at
> > once, but we don't actually need that for any of the "modern" platforms
> > where we already have a probe function that gets called once for
> > each controller.
> 
> No.  The main purpose of hw_pci is as a container to support multiple
> different platform specific PCI implementations in one kernel.  It's
> exactly what you need for single zImage.

Well, we definitely need something to manage the assignment of domains,
bus numbers and I/O space windows, but the main issue I see with existing
hw_pci container is that it assumes that you can pass give it all
host bridges for a given domain at once. The problem with this is
that the pci host bridge drivers don't interact with one another, so
a system that needs two different PCI host drivers can't use hw_pci
to register them both, unless we come up with some extra
infrastructure.

Also, the calling conventions for pci_common_init_dev() mean that
it's hard to propagate -EPROBE_DEFER errors back to the driver
probe function, so it seems easier to come up with something new
that deals with all issues at once and that is outside of architecture
specific code.

	Arnd
--
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

Patch

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 317da88..12c2178 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -514,6 +514,26 @@  static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 	}
 }
 
+static void pci_common_bus_probe(struct pci_bus *bus)
+{
+	if (!pci_has_flag(PCI_PROBE_ONLY)) {
+		/*
+		 * Size the bridge windows.
+		 */
+		pci_bus_size_bridges(bus);
+
+		/*
+		 * Assign resources.
+		 */
+		pci_bus_assign_resources(bus);
+	}
+
+	/*
+	 * Tell drivers about devices found.
+	 */
+	pci_bus_add_devices(bus);
+}
+
 void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
 {
 	struct pci_sys_data *sys;
@@ -528,27 +548,38 @@  void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
 
 	pci_fixup_irqs(pcibios_swizzle, pcibios_map_irq);
 
-	list_for_each_entry(sys, &head, node) {
-		struct pci_bus *bus = sys->bus;
+	list_for_each_entry(sys, &head, node)
+		pci_common_bus_probe(sys->bus);
+}
 
-		if (!pci_has_flag(PCI_PROBE_ONLY)) {
-			/*
-			 * Size the bridge windows.
-			 */
-			pci_bus_size_bridges(bus);
 
-			/*
-			 * Assign resources.
-			 */
-			pci_bus_assign_resources(bus);
-		}
 
-		/*
-		 * Tell drivers about devices found.
-		 */
-		pci_bus_add_devices(bus);
-	}
+
+int pci_host_bridge_register(struct device *parent, struct pci_sys_data *sys, struct pci_ops *ops, int (*setup)(int nr, struct pci_sys_data *))
+{
+	int ret;
+
+	pci_add_flags(PCI_REASSIGN_ALL_RSRC);
+	INIT_LIST_HEAD(&sys->resources);
+
+	ret = setup(0, sys);
+	if (ret)
+		return ret;
+
+	ret = pcibios_init_resources(0, sys);
+	if (ret)
+		return ret;
+
+	sys->bus = pci_scan_root_bus(parent, sys->busnr, ops, sys, &sys->resources);
+	if (!sys->bus)
+		return -ENODEV;
+
+	pci_fixup_irqs(pcibios_swizzle, pcibios_map_irq);
+
+	pci_common_bus_probe(sys->bus);
+	return ret;
 }
+EXPORT_SYMBOL_GPL(pci_host_bridge_register);
 
 #ifndef CONFIG_PCI_HOST_ITE8152
 void pcibios_set_master(struct pci_dev *dev)