diff mbox

[RFC/RFT,05/18] ARM: PCI: dove: Convert PCI scan API to pci_scan_root_bus_bridge()

Message ID 20170426111809.19922-6-lorenzo.pieralisi@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Lorenzo Pieralisi April 26, 2017, 11:17 a.m. UTC
The introduction of pci_scan_root_bus_bridge() provides a PCI core
API to scan a PCI root bus backed by an already initialized
struct pci_host_bridge object, which simplifies the bus scan
interface and makes the PCI scan root bus interface easier to
generalize as members are added to the struct pci_host_bridge().

Convert ARM dove platform code to pci_scan_root_bus_bridge() to improve
the PCI root bus scanning interface.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Andrew Lunn <andrew@lunn.ch>
---
 arch/arm/mach-dove/pcie.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

Comments

Arnd Bergmann April 28, 2017, 12:38 p.m. UTC | #1
On Wed, Apr 26, 2017 at 1:17 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> The introduction of pci_scan_root_bus_bridge() provides a PCI core
> API to scan a PCI root bus backed by an already initialized
> struct pci_host_bridge object, which simplifies the bus scan
> interface and makes the PCI scan root bus interface easier to
> generalize as members are added to the struct pci_host_bridge().
>
> Convert ARM dove platform code to pci_scan_root_bus_bridge() to improve
> the PCI root bus scanning interface.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

Is this patch required for one of the later steps in the series?

As non-DT dove uses the traditional pci_common_init() helper rather
than registering its own driver, I wonder if there is anything to gain here.

        Arnd
Arnd Bergmann April 28, 2017, 12:52 p.m. UTC | #2
On Fri, Apr 28, 2017 at 2:38 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Apr 26, 2017 at 1:17 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
>> The introduction of pci_scan_root_bus_bridge() provides a PCI core
>> API to scan a PCI root bus backed by an already initialized
>> struct pci_host_bridge object, which simplifies the bus scan
>> interface and makes the PCI scan root bus interface easier to
>> generalize as members are added to the struct pci_host_bridge().
>>
>> Convert ARM dove platform code to pci_scan_root_bus_bridge() to improve
>> the PCI root bus scanning interface.
>>
>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>
> Is this patch required for one of the later steps in the series?
>
> As non-DT dove uses the traditional pci_common_init() helper rather
> than registering its own driver, I wonder if there is anything to gain here.

Thinking about it some more, if we make the change to allocate from
pcibios_init_hw(), we can also initialize most of the fields there and
do the cleanup in common code when the scan callback fails, which
in turn makes the changes in arch/arm/mach*/pci.c drivers very simple.

        Arnd
Lorenzo Pieralisi May 3, 2017, 10:31 a.m. UTC | #3
On Fri, Apr 28, 2017 at 02:38:48PM +0200, Arnd Bergmann wrote:
> On Wed, Apr 26, 2017 at 1:17 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > The introduction of pci_scan_root_bus_bridge() provides a PCI core
> > API to scan a PCI root bus backed by an already initialized
> > struct pci_host_bridge object, which simplifies the bus scan
> > interface and makes the PCI scan root bus interface easier to
> > generalize as members are added to the struct pci_host_bridge().
> >
> > Convert ARM dove platform code to pci_scan_root_bus_bridge() to improve
> > the PCI root bus scanning interface.
> >
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> Is this patch required for one of the later steps in the series?
> 
> As non-DT dove uses the traditional pci_common_init() helper rather
> than registering its own driver, I wonder if there is anything to gain here.

Well, the point is, the non-DT platforms I patched implement a custom
.scan method in struct hw_pci. If we move the bridge allocation to
pcibios_init_hw() we would end up initializing some struct
pci_host_bridge fields in pcibios_init_hw() and some in the custom .scan
method (ie custom pci_ops) which I found not very elegant but it could be
done I reckon, I need to give it a go to see how the code looks like.

Lorenzo
Arnd Bergmann May 3, 2017, 12:02 p.m. UTC | #4
On Wed, May 3, 2017 at 12:31 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Fri, Apr 28, 2017 at 02:38:48PM +0200, Arnd Bergmann wrote:
>> On Wed, Apr 26, 2017 at 1:17 PM, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>> > The introduction of pci_scan_root_bus_bridge() provides a PCI core
>> > API to scan a PCI root bus backed by an already initialized
>> > struct pci_host_bridge object, which simplifies the bus scan
>> > interface and makes the PCI scan root bus interface easier to
>> > generalize as members are added to the struct pci_host_bridge().
>> >
>> > Convert ARM dove platform code to pci_scan_root_bus_bridge() to improve
>> > the PCI root bus scanning interface.
>> >
>> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>
>> Is this patch required for one of the later steps in the series?
>>
>> As non-DT dove uses the traditional pci_common_init() helper rather
>> than registering its own driver, I wonder if there is anything to gain here.
>
> Well, the point is, the non-DT platforms I patched implement a custom
> .scan method in struct hw_pci. If we move the bridge allocation to
> pcibios_init_hw() we would end up initializing some struct
> pci_host_bridge fields in pcibios_init_hw() and some in the custom .scan
> method (ie custom pci_ops) which I found not very elegant but it could be
> done I reckon, I need to give it a go to see how the code looks like.

I don't see anything wrong with initializing the members in different
places. The first set would give you a default that works for basic
drivers, and the second set is a way to override the defaults for drivers
that do something special. Conceptually we do this all the time in
other places.

     Arnd
diff mbox

Patch

diff --git a/arch/arm/mach-dove/pcie.c b/arch/arm/mach-dove/pcie.c
index 91fe971..999e465 100644
--- a/arch/arm/mach-dove/pcie.c
+++ b/arch/arm/mach-dove/pcie.c
@@ -155,13 +155,31 @@  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MARVELL, PCI_ANY_ID, rc_pci_fixup);
 static struct pci_bus __init *
 dove_pcie_scan_bus(int nr, struct pci_sys_data *sys)
 {
+	struct pci_host_bridge *bridge;
+	int ret;
+
 	if (nr >= num_pcie_ports) {
 		BUG();
 		return NULL;
 	}
 
-	return pci_scan_root_bus(NULL, sys->busnr, &pcie_ops, sys,
-				 &sys->resources);
+	bridge = pci_alloc_host_bridge(0);
+	if (!bridge)
+		return NULL;
+
+	list_splice_init(&sys->resources, &bridge->windows);
+	bridge->dev.parent = NULL;
+	bridge->sysdata = sys;
+	bridge->busnr = sys->busnr;
+	bridge->ops = &pcie_ops;
+
+	ret = pci_scan_root_bus_bridge(bridge);
+	if (ret < 0) {
+		pci_free_host_bridge(bridge);
+		return NULL;
+	}
+
+	return bridge->bus;
 }
 
 static int __init dove_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)