PCI: designware: Add ARM64 support

This patch tries to unify ARM32 and ARM64 PCIe in designware driver. Delete function dw_pcie_setup, dw_pcie_scan_bus, dw_pcie_map_irq and struct hw_pci, move related operations to dw_pcie_host_init.

I am not very clear about I/O resource management:

Quoted 6 lines

	if (global_io_offset < SZ_1M && pp->io_size > 0) {
		pci_ioremap_io(global_io_offset, pp->io_base);
		global_io_offset += SZ_64K;
		pci_add_resource_offset(&res, &pp->io,
					global_io_offset - pp->io_bus_addr);
	}

so just move steps in dw_pcie_setup to dw_pcie_host_init.

I have compiled the driver with multi_v7_defconfig. However, I don't have ARM32 PCIe related board to do test. It will be appreciated if someone could help to test it.

Signed-off-by Zhou Wang <[email protected]>

---
 drivers/pci/host/pcie-designware.c | 128 +++++++++++++++----------------------
 1 file changed, 50 insertions(+), 78 deletions(-)

drivers/pci/host/pcie-designware.c +0 -0

index 2e9f84f..7bad9e5 100644

drivers/pci/host/pcie-designware.c +49 -37

 #include "pcie-designware.h"
 

@@ -67,17 +68,10 @@
 #define PCIE_ATU_FUNC(x)		(((x) & 0x7) << 16)
 #define PCIE_ATU_UPPER_TARGET		0x91C
 
-static struct hw_pci dw_pci;
+static struct pci_ops dw_pcie_ops;
 
 static unsigned long global_io_offset;
 
-static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
-{
-	BUG_ON(!sys->private_data);
-
-	return sys->private_data;
-}
-
 int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
 {
 	*val = readl(addr);

@@ -238,7 +232,7 @@ static void dw_pcie_msi_set_irq(struct pcie_port *pp, int irq)
 static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos)
 {
 	int irq, pos0, i;
-	struct pcie_port *pp = sys_to_pcie(desc->dev->bus->sysdata);
+	struct pcie_port *pp = desc->dev->bus->sysdata;
 
 	pos0 = bitmap_find_free_region(pp->msi_irq_in_use, MAX_MSI_IRQS,
 				       order_base_2(no_irqs));

@@ -281,7 +275,7 @@ static int dw_msi_setup_irq(struct msi_controller *chip, struct pci_dev *pdev,
 {
 	int irq, pos;
 	struct msi_msg msg;
-	struct pcie_port *pp = sys_to_pcie(pdev->bus->sysdata);
+	struct pcie_port *pp = pdev->bus->sysdata;
 
 	if (desc->msi_attrib.is_msix)
 		return -EINVAL;

@@ -310,7 +304,7 @@ static void dw_msi_teardown_irq(struct msi_controller *chip, unsigned int irq)
 {
 	struct irq_data *data = irq_get_irq_data(irq);
 	struct msi_desc *msi = irq_data_get_msi(data);
-	struct pcie_port *pp = sys_to_pcie(msi->dev->bus->sysdata);
+	struct pcie_port *pp = msi->dev->bus->sysdata;
 
 	clear_irq_range(pp, irq, 1, data->hwirq);
 }

@@ -342,13 +336,15 @@ static const struct irq_domain_ops msi_domain_ops = {
 	.map = dw_pcie_msi_map,
 };
 
-int dw_pcie_host_init(struct pcie_port *pp)
+int __init dw_pcie_host_init(struct pcie_port *pp)
 {
 	struct device_node *np = pp->dev->of_node;
 	struct platform_device *pdev = to_platform_device(pp->dev);
 	struct of_pci_range range;
 	struct of_pci_range_parser parser;
+	struct pci_bus *bus;
 	struct resource *cfg_res;
+	LIST_HEAD(res);
 	u32 val, na, ns;
 	const __be32 *addrp;
 	int i, index, ret;

@@ -502,15 +498,49 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	val |= PORT_LOGIC_SPEED_CHANGE;
 	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
 
-#ifdef CONFIG_PCI_MSI
-	dw_pcie_msi_chip.dev = pp->dev;
-	dw_pci.msi_ctrl = &dw_pcie_msi_chip;
+#ifdef CONFIG_ARM
+	/*
+	 * FIXME: we should really be able to use
+	 * of_pci_get_host_bridge_resources on arm32 as well,
+	 * but the conversion needs some more testing
+	 */
+	if (global_io_offset < SZ_1M && pp->io_size > 0) {
+		pci_ioremap_io(global_io_offset, pp->io_base);
+		global_io_offset += SZ_64K;
+		pci_add_resource_offset(&res, &pp->io,
+					global_io_offset - pp->io_bus_addr);
+	}
+	pci_add_resource_offset(&res, &pp->mem,
+				pp->mem.start - pp->mem_bus_addr);
+	pci_add_resource(&res, &pp->busn);
+#else
+	ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &res, &pp->io_base);
+	if (ret)
+		return ret;
+#endif
+
+	bus = pci_create_root_bus(pp->dev, pp->root_bus_nr, &dw_pcie_ops,
+			      pp, &res);
+	if (!bus)
+		return -ENOMEM;
+
+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+	bus->msi = container_of(&pp->irq_domain, struct msi_controller, domain);
+#else
+	bus->msi = &dw_pcie_msi_chip;
 #endif
 
-	dw_pci.nr_controllers = 1;
-	dw_pci.private_data = (void **)&pp;
+	pci_scan_child_bus(bus);
+	if (pp->ops->scan_bus)
+		pp->ops->scan_bus(pp);
 
-	pci_common_init_dev(pp->dev, &dw_pci);
+#ifdef CONFIG_ARM
+	/* support old dtbs that incorrectly describe IRQs */
+	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
+#endif
+
+	pci_assign_unassigned_bus_resources(bus);
+	pci_bus_add_devices(bus);
 
 	return 0;
 }

@@ -653,7 +683,7 @@ static int dw_pcie_valid_config(struct pcie_port *pp,
 static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 			int size, u32 *val)
 {
-	struct pcie_port *pp = sys_to_pcie(bus->sysdata);
+	struct pcie_port *pp = bus->sysdata;
 	int ret;
 
 	if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) {

@@ -677,7 +707,7 @@ static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 static int dw_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 			int where, int size, u32 val)
 {
-	struct pcie_port *pp = sys_to_pcie(bus->sysdata);
+	struct pcie_port *pp = bus->sysdata;
 	int ret;
 
 	if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0)

@@ -701,64 +731,6 @@ static struct pci_ops dw_pcie_ops = {
 	.write = dw_pcie_wr_conf,
 };
 
-static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
-{
-	struct pcie_port *pp;
-
-	pp = sys_to_pcie(sys);
-
-	if (global_io_offset < SZ_1M && pp->io_size > 0) {
-		sys->io_offset = global_io_offset - pp->io_bus_addr;
-		pci_ioremap_io(global_io_offset, pp->io_base);
-		global_io_offset += SZ_64K;
-		pci_add_resource_offset(&sys->resources, &pp->io,
-					sys->io_offset);
-	}
-
-	sys->mem_offset = pp->mem.start - pp->mem_bus_addr;
-	pci_add_resource_offset(&sys->resources, &pp->mem, sys->mem_offset);
-	pci_add_resource(&sys->resources, &pp->busn);

drivers/pci/host/pcie-designware.c +0 -20

-	bus = pci_create_root_bus(pp->dev, sys->busnr,
-				  &dw_pcie_ops, sys, &sys->resources);
-	if (!bus)
-		return NULL;
-
-	pci_scan_child_bus(bus);
-
-	if (bus && pp->ops->scan_bus)
-		pp->ops->scan_bus(pp);
-
-	return bus;
-}
-
-static int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
-{
-	struct pcie_port *pp = sys_to_pcie(dev->bus->sysdata);
-	int irq;
-
-	irq = of_irq_parse_and_map_pci(dev, slot, pin);
-	if (!irq)

drivers/pci/host/pcie-designware.c +0 -1

-
 void dw_pcie_setup_rc(struct pcie_port *pp)
 {
 	u32 val;

Signature · 1 line

1.9.1

Quoted 23 lines · Zhou Wang

This patch tries to unify ARM32 and ARM64 PCIe in designware driver. Delete function dw_pcie_setup, dw_pcie_scan_bus, dw_pcie_map_irq and struct hw_pci, move related operations to dw_pcie_host_init.

I am not very clear about I/O resource management:

Quoted 6 lines

	if (global_io_offset < SZ_1M && pp->io_size > 0) {
		pci_ioremap_io(global_io_offset, pp->io_base);
		global_io_offset += SZ_64K;
		pci_add_resource_offset(&res, &pp->io,
					global_io_offset - pp->io_bus_addr);
	}

so just move steps in dw_pcie_setup to dw_pcie_host_init.

I have compiled the driver with multi_v7_defconfig. However, I don't have ARM32 PCIe related board to do test. It will be appreciated if someone could help to test it.

Signed-off-by Zhou Wang <[email protected]>

---
 drivers/pci/host/pcie-designware.c | 128 +++++++++++++++----------------------
 1 file changed, 50 insertions(+), 78 deletions(-)

drivers/pci/host/pcie-designware.c line 25

Quoted 9 lines

drivers/pci/host/pcie-designware.c +1 -0

index 2e9f84f..7bad9e5 100644
@@ -22,6 +22,7 @@
 #include <linux/pci_regs.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
+#include <asm/hardirq.h>

+cc Kishon Vijay Abraham I, Richard Zhu, Lucas Stach

Please use <linux/hardirq.h> and insert it alphabetically.

+#include <linux/hardirq.h>
#include <linux/irq.h>
#include <linux/irqdomain.h>
#include <linux/kernel.h>

Quoted 161 lines

drivers/pci/host/pcie-designware.c +49 -37

 #include "pcie-designware.h"
 

@@ -67,17 +68,10 @@
 #define PCIE_ATU_FUNC(x)		(((x) & 0x7) << 16)
 #define PCIE_ATU_UPPER_TARGET		0x91C
 
-static struct hw_pci dw_pci;
+static struct pci_ops dw_pcie_ops;
 
 static unsigned long global_io_offset;
 
-static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
-{
-	BUG_ON(!sys->private_data);
-
-	return sys->private_data;
-}
-
 int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
 {
 	*val = readl(addr);

@@ -238,7 +232,7 @@ static void dw_pcie_msi_set_irq(struct pcie_port *pp, int irq)
 static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos)
 {
 	int irq, pos0, i;
-	struct pcie_port *pp = sys_to_pcie(desc->dev->bus->sysdata);
+	struct pcie_port *pp = desc->dev->bus->sysdata;
 
 	pos0 = bitmap_find_free_region(pp->msi_irq_in_use, MAX_MSI_IRQS,
 				       order_base_2(no_irqs));

@@ -281,7 +275,7 @@ static int dw_msi_setup_irq(struct msi_controller *chip, struct pci_dev *pdev,
 {
 	int irq, pos;
 	struct msi_msg msg;
-	struct pcie_port *pp = sys_to_pcie(pdev->bus->sysdata);
+	struct pcie_port *pp = pdev->bus->sysdata;
 
 	if (desc->msi_attrib.is_msix)
 		return -EINVAL;

@@ -310,7 +304,7 @@ static void dw_msi_teardown_irq(struct msi_controller *chip, unsigned int irq)
 {
 	struct irq_data *data = irq_get_irq_data(irq);
 	struct msi_desc *msi = irq_data_get_msi(data);
-	struct pcie_port *pp = sys_to_pcie(msi->dev->bus->sysdata);
+	struct pcie_port *pp = msi->dev->bus->sysdata;
 
 	clear_irq_range(pp, irq, 1, data->hwirq);
 }

@@ -342,13 +336,15 @@ static const struct irq_domain_ops msi_domain_ops = {
 	.map = dw_pcie_msi_map,
 };
 
-int dw_pcie_host_init(struct pcie_port *pp)
+int __init dw_pcie_host_init(struct pcie_port *pp)
 {
 	struct device_node *np = pp->dev->of_node;
 	struct platform_device *pdev = to_platform_device(pp->dev);
 	struct of_pci_range range;
 	struct of_pci_range_parser parser;
+	struct pci_bus *bus;
 	struct resource *cfg_res;
+	LIST_HEAD(res);
 	u32 val, na, ns;
 	const __be32 *addrp;
 	int i, index, ret;

@@ -502,15 +498,49 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	val |= PORT_LOGIC_SPEED_CHANGE;
 	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
 
-#ifdef CONFIG_PCI_MSI
-	dw_pcie_msi_chip.dev = pp->dev;
-	dw_pci.msi_ctrl = &dw_pcie_msi_chip;
+#ifdef CONFIG_ARM
+	/*
+	 * FIXME: we should really be able to use
+	 * of_pci_get_host_bridge_resources on arm32 as well,
+	 * but the conversion needs some more testing
+	 */
+	if (global_io_offset < SZ_1M && pp->io_size > 0) {
+		pci_ioremap_io(global_io_offset, pp->io_base);
+		global_io_offset += SZ_64K;
+		pci_add_resource_offset(&res, &pp->io,
+					global_io_offset - pp->io_bus_addr);
+	}
+	pci_add_resource_offset(&res, &pp->mem,
+				pp->mem.start - pp->mem_bus_addr);
+	pci_add_resource(&res, &pp->busn);
+#else
+	ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &res, &pp->io_base);
+	if (ret)
+		return ret;
+#endif
+
+	bus = pci_create_root_bus(pp->dev, pp->root_bus_nr, &dw_pcie_ops,
+			      pp, &res);
+	if (!bus)
+		return -ENOMEM;
+
+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+	bus->msi = container_of(&pp->irq_domain, struct msi_controller, domain);
+#else
+	bus->msi = &dw_pcie_msi_chip;
 #endif
 
-	dw_pci.nr_controllers = 1;
-	dw_pci.private_data = (void **)&pp;
+	pci_scan_child_bus(bus);
+	if (pp->ops->scan_bus)
+		pp->ops->scan_bus(pp);
 
-	pci_common_init_dev(pp->dev, &dw_pci);
+#ifdef CONFIG_ARM
+	/* support old dtbs that incorrectly describe IRQs */
+	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
+#endif
+
+	pci_assign_unassigned_bus_resources(bus);
+	pci_bus_add_devices(bus);
 
 	return 0;
 }

@@ -653,7 +683,7 @@ static int dw_pcie_valid_config(struct pcie_port *pp,
 static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 			int size, u32 *val)
 {
-	struct pcie_port *pp = sys_to_pcie(bus->sysdata);
+	struct pcie_port *pp = bus->sysdata;
 	int ret;
 
 	if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) {

@@ -677,7 +707,7 @@ static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 static int dw_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 			int where, int size, u32 val)
 {
-	struct pcie_port *pp = sys_to_pcie(bus->sysdata);
+	struct pcie_port *pp = bus->sysdata;
 	int ret;
 
 	if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0)

@@ -701,64 +731,6 @@ static struct pci_ops dw_pcie_ops = {
 	.write = dw_pcie_wr_conf,
 };
 
-static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
-{
-	struct pcie_port *pp;
-
-	pp = sys_to_pcie(sys);
-
-	if (global_io_offset < SZ_1M && pp->io_size > 0) {
-		sys->io_offset = global_io_offset - pp->io_bus_addr;
-		pci_ioremap_io(global_io_offset, pp->io_base);
-		global_io_offset += SZ_64K;
-		pci_add_resource_offset(&sys->resources, &pp->io,
-					sys->io_offset);
-	}
-
-	sys->mem_offset = pp->mem.start - pp->mem_bus_addr;
-	pci_add_resource_offset(&sys->resources, &pp->mem, sys->mem_offset);
-	pci_add_resource(&sys->resources, &pp->busn);

drivers/pci/host/pcie-designware.c static struct pci_ops dw_pcie_ops = { · line 730

Quoted 10 lines

drivers/pci/host/pcie-designware.c +0 -10

-
-	return 1;
-}
-
-static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
-{
-	struct pci_bus *bus;
-	struct pcie_port *pp = sys_to_pcie(sys);
-
-	pp->root_bus_nr = sys->busnr;

'pp->root_bus_nr' is initialized as '-1' at some soc-specific drivers However, 'sys->busnr' is set as '0, 1, 2 ...' by pcibios_init_hw() in arch/arm/kernel/bios32.c. '0, 1, 2 ...' means the number of each host controller.

So, without setting 'pp->root_bus_nr' as '0, 1, 2, ...', it makes the problem.

Thus, we need to come up with the way to resolve this.

1. Setting 'pp->root_bus_nr' as '0, 1, 2 ..' by each soc-specific driver.

e.g)

./drivers/pci/host/pci-exynos.c

(quoted diff) +1 -1

@@ -534,7 +534,7 @@ static int __init exynos_add_pcie_port(struct pcie_port *pp,
                }
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &exynos_pcie_host_ops;
 

./drivers/pci/host/pci-imx6.c

(quoted diff) +1 -1

@@ -541,7 +541,7 @@ static int __init imx6_add_pcie_port(struct pcie_port *pp,
                }
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &imx6_pcie_host_ops;
 

./drivers/pci/host/pci-keystone.c

(quoted diff) +1 -1

@@ -312,7 +312,7 @@ static int __init ks_add_pcie_port(struct keystone_pcie *ks_pcie,
                        return ret;
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &keystone_pcie_host_ops;
 

./drivers/pci/host/pci-layerscape.c

(quoted diff) +1 -1

@@ -101,7 +101,7 @@ static int ls_add_pcie_port(struct ls_pcie *pcie)
        pp = &pcie->pp;
        pp->dev = pcie->dev;
        pp->dbi_base = pcie->dbi;
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &ls_pcie_host_ops;
 

./drivers/pci/host/pcie-spear13xx.c

(quoted diff) +1 -1

@@ -287,7 +287,7 @@ static int spear13xx_add_pcie_port(struct pcie_port *pp,
                return ret;
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &spear13xx_pcie_host_ops;
 
 

2. Setting 'pp->root_bus_nr' as '0, 1, 2 ..' by pcie-designware.c

I believe that someone will give better idea. :-)

Quoted 20 lines

drivers/pci/host/pcie-designware.c +0 -20

-	bus = pci_create_root_bus(pp->dev, sys->busnr,
-				  &dw_pcie_ops, sys, &sys->resources);
-	if (!bus)
-		return NULL;
-
-	pci_scan_child_bus(bus);
-
-	if (bus && pp->ops->scan_bus)
-		pp->ops->scan_bus(pp);
-
-	return bus;
-}
-
-static int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
-{
-	struct pcie_port *pp = sys_to_pcie(dev->bus->sysdata);
-	int irq;
-
-	irq = of_irq_parse_and_map_pci(dev, slot, pin);
-	if (!irq)

drivers/pci/host/pcie-designware.c static struct pci_ops dw_pcie_ops = { · line 760

Quoted 10 lines

drivers/pci/host/pcie-designware.c +0 -10

-		irq = pp->irq;
-
-	return irq;
-}
-
-static struct hw_pci dw_pci = {
-	.setup		= dw_pcie_setup,
-	.scan		= dw_pcie_scan_bus,
-	.map_irq	= dw_pcie_map_irq,
-};

Right, 'struct hw_pci' should not be used in order to unify ARM32 and ARM64. I have no objection to remove 'struct hw_pci' from 'pcie-designware.c'. Thank you for your patch.

Best regards,
Jingoo Han

Quoted 6 lines

drivers/pci/host/pcie-designware.c +0 -1

-
 void dw_pcie_setup_rc(struct pcie_port *pp)
 {
 	u32 val;

Signature · 1 line

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

.

Quoted 24 lines · Jingoo Han

Quoted 23 lines · Zhou Wang

This patch tries to unify ARM32 and ARM64 PCIe in designware driver. Delete function dw_pcie_setup, dw_pcie_scan_bus, dw_pcie_map_irq and struct hw_pci, move related operations to dw_pcie_host_init.

I am not very clear about I/O resource management:

Quoted 6 lines

	if (global_io_offset < SZ_1M && pp->io_size > 0) {
		pci_ioremap_io(global_io_offset, pp->io_base);
		global_io_offset += SZ_64K;
		pci_add_resource_offset(&res, &pp->io,
					global_io_offset - pp->io_bus_addr);
	}

so just move steps in dw_pcie_setup to dw_pcie_host_init.

I have compiled the driver with multi_v7_defconfig. However, I don't have ARM32 PCIe related board to do test. It will be appreciated if someone could help to test it.

Signed-off-by Zhou Wang <[email protected]>

---
 drivers/pci/host/pcie-designware.c | 128 +++++++++++++++----------------------
 1 file changed, 50 insertions(+), 78 deletions(-)

Quoted 12 lines

drivers/pci/host/pcie-designware.c line 25

Quoted 9 lines

drivers/pci/host/pcie-designware.c +1 -0

index 2e9f84f..7bad9e5 100644
@@ -22,6 +22,7 @@
 #include <linux/pci_regs.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
+#include <asm/hardirq.h>

+cc Kishon Vijay Abraham I, Richard Zhu, Lucas Stach

Thanks to involve more related guys to this discussion :)

Quoted 7 lines

Please use <linux/hardirq.h> and insert it alphabetically.

+#include <linux/hardirq.h>
#include <linux/irq.h>
#include <linux/irqdomain.h>
#include <linux/kernel.h>

OK, will do in next version patch.

Quoted 177 lines

Quoted 161 lines

drivers/pci/host/pcie-designware.c +49 -37

 #include "pcie-designware.h"
 

@@ -67,17 +68,10 @@
 #define PCIE_ATU_FUNC(x)		(((x) & 0x7) << 16)
 #define PCIE_ATU_UPPER_TARGET		0x91C
 
-static struct hw_pci dw_pci;
+static struct pci_ops dw_pcie_ops;
 
 static unsigned long global_io_offset;
 
-static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
-{
-	BUG_ON(!sys->private_data);
-
-	return sys->private_data;
-}
-
 int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
 {
 	*val = readl(addr);

@@ -238,7 +232,7 @@ static void dw_pcie_msi_set_irq(struct pcie_port *pp, int irq)
 static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos)
 {
 	int irq, pos0, i;
-	struct pcie_port *pp = sys_to_pcie(desc->dev->bus->sysdata);
+	struct pcie_port *pp = desc->dev->bus->sysdata;
 
 	pos0 = bitmap_find_free_region(pp->msi_irq_in_use, MAX_MSI_IRQS,
 				       order_base_2(no_irqs));

@@ -281,7 +275,7 @@ static int dw_msi_setup_irq(struct msi_controller *chip, struct pci_dev *pdev,
 {
 	int irq, pos;
 	struct msi_msg msg;
-	struct pcie_port *pp = sys_to_pcie(pdev->bus->sysdata);
+	struct pcie_port *pp = pdev->bus->sysdata;
 
 	if (desc->msi_attrib.is_msix)
 		return -EINVAL;

@@ -310,7 +304,7 @@ static void dw_msi_teardown_irq(struct msi_controller *chip, unsigned int irq)
 {
 	struct irq_data *data = irq_get_irq_data(irq);
 	struct msi_desc *msi = irq_data_get_msi(data);
-	struct pcie_port *pp = sys_to_pcie(msi->dev->bus->sysdata);
+	struct pcie_port *pp = msi->dev->bus->sysdata;
 
 	clear_irq_range(pp, irq, 1, data->hwirq);
 }

@@ -342,13 +336,15 @@ static const struct irq_domain_ops msi_domain_ops = {
 	.map = dw_pcie_msi_map,
 };
 
-int dw_pcie_host_init(struct pcie_port *pp)
+int __init dw_pcie_host_init(struct pcie_port *pp)
 {
 	struct device_node *np = pp->dev->of_node;
 	struct platform_device *pdev = to_platform_device(pp->dev);
 	struct of_pci_range range;
 	struct of_pci_range_parser parser;
+	struct pci_bus *bus;
 	struct resource *cfg_res;
+	LIST_HEAD(res);
 	u32 val, na, ns;
 	const __be32 *addrp;
 	int i, index, ret;

@@ -502,15 +498,49 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	val |= PORT_LOGIC_SPEED_CHANGE;
 	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
 
-#ifdef CONFIG_PCI_MSI
-	dw_pcie_msi_chip.dev = pp->dev;
-	dw_pci.msi_ctrl = &dw_pcie_msi_chip;
+#ifdef CONFIG_ARM
+	/*
+	 * FIXME: we should really be able to use
+	 * of_pci_get_host_bridge_resources on arm32 as well,
+	 * but the conversion needs some more testing
+	 */
+	if (global_io_offset < SZ_1M && pp->io_size > 0) {
+		pci_ioremap_io(global_io_offset, pp->io_base);
+		global_io_offset += SZ_64K;
+		pci_add_resource_offset(&res, &pp->io,
+					global_io_offset - pp->io_bus_addr);
+	}
+	pci_add_resource_offset(&res, &pp->mem,
+				pp->mem.start - pp->mem_bus_addr);
+	pci_add_resource(&res, &pp->busn);
+#else
+	ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &res, &pp->io_base);
+	if (ret)
+		return ret;
+#endif
+
+	bus = pci_create_root_bus(pp->dev, pp->root_bus_nr, &dw_pcie_ops,
+			      pp, &res);
+	if (!bus)
+		return -ENOMEM;
+
+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+	bus->msi = container_of(&pp->irq_domain, struct msi_controller, domain);
+#else
+	bus->msi = &dw_pcie_msi_chip;
 #endif
 
-	dw_pci.nr_controllers = 1;
-	dw_pci.private_data = (void **)&pp;
+	pci_scan_child_bus(bus);
+	if (pp->ops->scan_bus)
+		pp->ops->scan_bus(pp);
 
-	pci_common_init_dev(pp->dev, &dw_pci);
+#ifdef CONFIG_ARM
+	/* support old dtbs that incorrectly describe IRQs */
+	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
+#endif
+
+	pci_assign_unassigned_bus_resources(bus);
+	pci_bus_add_devices(bus);
 
 	return 0;
 }

@@ -653,7 +683,7 @@ static int dw_pcie_valid_config(struct pcie_port *pp,
 static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 			int size, u32 *val)
 {
-	struct pcie_port *pp = sys_to_pcie(bus->sysdata);
+	struct pcie_port *pp = bus->sysdata;
 	int ret;
 
 	if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) {

@@ -677,7 +707,7 @@ static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 static int dw_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 			int where, int size, u32 val)
 {
-	struct pcie_port *pp = sys_to_pcie(bus->sysdata);
+	struct pcie_port *pp = bus->sysdata;
 	int ret;
 
 	if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0)

@@ -701,64 +731,6 @@ static struct pci_ops dw_pcie_ops = {
 	.write = dw_pcie_wr_conf,
 };
 
-static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
-{
-	struct pcie_port *pp;
-
-	pp = sys_to_pcie(sys);
-
-	if (global_io_offset < SZ_1M && pp->io_size > 0) {
-		sys->io_offset = global_io_offset - pp->io_bus_addr;
-		pci_ioremap_io(global_io_offset, pp->io_base);
-		global_io_offset += SZ_64K;
-		pci_add_resource_offset(&sys->resources, &pp->io,
-					sys->io_offset);
-	}
-
-	sys->mem_offset = pp->mem.start - pp->mem_bus_addr;
-	pci_add_resource_offset(&sys->resources, &pp->mem, sys->mem_offset);
-	pci_add_resource(&sys->resources, &pp->busn);

drivers/pci/host/pcie-designware.c static struct pci_ops dw_pcie_ops = { · line 730

Quoted 10 lines

drivers/pci/host/pcie-designware.c +0 -10

-
-	return 1;
-}
-
-static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
-{
-	struct pci_bus *bus;
-	struct pcie_port *pp = sys_to_pcie(sys);
-
-	pp->root_bus_nr = sys->busnr;

'pp->root_bus_nr' is initialized as '-1' at some soc-specific drivers However, 'sys->busnr' is set as '0, 1, 2 ...' by pcibios_init_hw() in arch/arm/kernel/bios32.c. '0, 1, 2 ...' means the number of each host controller.

Quoted 3 lines

So, without setting 'pp->root_bus_nr' as '0, 1, 2, ...', it makes the problem.

'sys->busnr' is set as "0, busn_res.end + 1 ..." in pcibios_init_hw() I think:

pcibios_init_hw()

	for (nr = busnr = 0; nr < hw->nr_controllers; nr++)
		sys->busnr = busnr;
		...
		busnr = sys->bus->busn_res.end + 1;

and it indicates root bus number of one pcie controller. so in dw_pcie_scan_bus:

	pp->root_bus_nr = sys->busnr;
	bus = pci_create_root_bus(..., sys->busnr, ...);

we set root bus number as "0, busn_res.end + 1". But in pcie-designware, nr_controllers is set as 1, so it only comes into the loop in pcibios_init_hw() one time.

so how about we set 0 as default value of pp->root_bus_nr, then use pci_create_root_bus(pp->dev, pp->root_bus_nr, &dw_pcie_ops, pp, &res); to create root bus?

Quoted 92 lines

Thus, we need to come up with the way to resolve this.

1. Setting 'pp->root_bus_nr' as '0, 1, 2 ..' by each soc-specific driver.

e.g)

./drivers/pci/host/pci-exynos.c

(quoted diff) +1 -1

@@ -534,7 +534,7 @@ static int __init exynos_add_pcie_port(struct pcie_port *pp,
                }
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &exynos_pcie_host_ops;
 

./drivers/pci/host/pci-imx6.c

(quoted diff) +1 -1

@@ -541,7 +541,7 @@ static int __init imx6_add_pcie_port(struct pcie_port *pp,
                }
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &imx6_pcie_host_ops;
 

./drivers/pci/host/pci-keystone.c

(quoted diff) +1 -1

@@ -312,7 +312,7 @@ static int __init ks_add_pcie_port(struct keystone_pcie *ks_pcie,
                        return ret;
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &keystone_pcie_host_ops;
 

./drivers/pci/host/pci-layerscape.c

(quoted diff) +1 -1

@@ -101,7 +101,7 @@ static int ls_add_pcie_port(struct ls_pcie *pcie)
        pp = &pcie->pp;
        pp->dev = pcie->dev;
        pp->dbi_base = pcie->dbi;
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &ls_pcie_host_ops;
 

./drivers/pci/host/pcie-spear13xx.c

(quoted diff) +1 -1

@@ -287,7 +287,7 @@ static int spear13xx_add_pcie_port(struct pcie_port *pp,
                return ret;
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &spear13xx_pcie_host_ops;
 
 

2. Setting 'pp->root_bus_nr' as '0, 1, 2 ..' by pcie-designware.c

I believe that someone will give better idea. :-)

Quoted 20 lines

drivers/pci/host/pcie-designware.c +0 -20

-	bus = pci_create_root_bus(pp->dev, sys->busnr,
-				  &dw_pcie_ops, sys, &sys->resources);
-	if (!bus)
-		return NULL;
-
-	pci_scan_child_bus(bus);
-
-	if (bus && pp->ops->scan_bus)
-		pp->ops->scan_bus(pp);
-
-	return bus;
-}
-
-static int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
-{
-	struct pcie_port *pp = sys_to_pcie(dev->bus->sysdata);
-	int irq;
-
-	irq = of_irq_parse_and_map_pci(dev, slot, pin);
-	if (!irq)

drivers/pci/host/pcie-designware.c static struct pci_ops dw_pcie_ops = { · line 760

Quoted 10 lines

drivers/pci/host/pcie-designware.c +0 -10

-		irq = pp->irq;
-
-	return irq;
-}
-
-static struct hw_pci dw_pci = {
-	.setup		= dw_pcie_setup,
-	.scan		= dw_pcie_scan_bus,
-	.map_irq	= dw_pcie_map_irq,
-};

Right, 'struct hw_pci' should not be used in order to unify ARM32 and ARM64. I have no objection to remove 'struct hw_pci' from 'pcie-designware.c'. Thank you for your patch.

Quoted 3 lines

Best regards,
Jingoo Han

Many thanks for your reviewing!
Zhou Wang

Quoted 6 lines

Quoted 6 lines

drivers/pci/host/pcie-designware.c +0 -2

-
 void dw_pcie_setup_rc(struct pcie_port *pp)
 {
 	u32 val;
--

1.9.1

Quoted 3 lines

Quoted 3 lines

so how about we set 0 as default value of pp->root_bus_nr, then use pci_create_root_bus(pp->dev, pp->root_bus_nr, &dw_pcie_ops, pp, &res); to create root bus?

I agree with your opinion. However, I still want to wait for other people's opinions. They will give good comments.
Thank you.

Best regards,
Jingoo Han

Quoted 239 lines · Zhou Wang

Quoted 24 lines · Jingoo Han

Quoted 23 lines · Zhou Wang

This patch tries to unify ARM32 and ARM64 PCIe in designware driver. Delete function dw_pcie_setup, dw_pcie_scan_bus, dw_pcie_map_irq and struct hw_pci, move related operations to dw_pcie_host_init.

I am not very clear about I/O resource management:

Quoted 6 lines

	if (global_io_offset < SZ_1M && pp->io_size > 0) {
		pci_ioremap_io(global_io_offset, pp->io_base);
		global_io_offset += SZ_64K;
		pci_add_resource_offset(&res, &pp->io,
					global_io_offset - pp->io_bus_addr);
	}

so just move steps in dw_pcie_setup to dw_pcie_host_init.

I have compiled the driver with multi_v7_defconfig. However, I don't have ARM32 PCIe related board to do test. It will be appreciated if someone could help to test it.

Signed-off-by Zhou Wang <[email protected]>

---
 drivers/pci/host/pcie-designware.c | 128 +++++++++++++++----------------------
 1 file changed, 50 insertions(+), 78 deletions(-)

Quoted 12 lines

drivers/pci/host/pcie-designware.c line 25

Quoted 9 lines

drivers/pci/host/pcie-designware.c +1 -0

index 2e9f84f..7bad9e5 100644
@@ -22,6 +22,7 @@
 #include <linux/pci_regs.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
+#include <asm/hardirq.h>

+cc Kishon Vijay Abraham I, Richard Zhu, Lucas Stach

Thanks to involve more related guys to this discussion :)

Quoted 7 lines

Please use <linux/hardirq.h> and insert it alphabetically.

+#include <linux/hardirq.h>
#include <linux/irq.h>
#include <linux/irqdomain.h>
#include <linux/kernel.h>

OK, will do in next version patch.

Quoted 177 lines

Quoted 161 lines

drivers/pci/host/pcie-designware.c +49 -37

 #include "pcie-designware.h"
 

@@ -67,17 +68,10 @@
 #define PCIE_ATU_FUNC(x)		(((x) & 0x7) << 16)
 #define PCIE_ATU_UPPER_TARGET		0x91C
 
-static struct hw_pci dw_pci;
+static struct pci_ops dw_pcie_ops;
 
 static unsigned long global_io_offset;
 
-static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
-{
-	BUG_ON(!sys->private_data);
-
-	return sys->private_data;
-}
-
 int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
 {
 	*val = readl(addr);

@@ -238,7 +232,7 @@ static void dw_pcie_msi_set_irq(struct pcie_port *pp, int irq)
 static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos)
 {
 	int irq, pos0, i;
-	struct pcie_port *pp = sys_to_pcie(desc->dev->bus->sysdata);
+	struct pcie_port *pp = desc->dev->bus->sysdata;
 
 	pos0 = bitmap_find_free_region(pp->msi_irq_in_use, MAX_MSI_IRQS,
 				       order_base_2(no_irqs));

@@ -281,7 +275,7 @@ static int dw_msi_setup_irq(struct msi_controller *chip, struct pci_dev *pdev,
 {
 	int irq, pos;
 	struct msi_msg msg;
-	struct pcie_port *pp = sys_to_pcie(pdev->bus->sysdata);
+	struct pcie_port *pp = pdev->bus->sysdata;
 
 	if (desc->msi_attrib.is_msix)
 		return -EINVAL;

@@ -310,7 +304,7 @@ static void dw_msi_teardown_irq(struct msi_controller *chip, unsigned int irq)
 {
 	struct irq_data *data = irq_get_irq_data(irq);
 	struct msi_desc *msi = irq_data_get_msi(data);
-	struct pcie_port *pp = sys_to_pcie(msi->dev->bus->sysdata);
+	struct pcie_port *pp = msi->dev->bus->sysdata;
 
 	clear_irq_range(pp, irq, 1, data->hwirq);
 }

@@ -342,13 +336,15 @@ static const struct irq_domain_ops msi_domain_ops = {
 	.map = dw_pcie_msi_map,
 };
 
-int dw_pcie_host_init(struct pcie_port *pp)
+int __init dw_pcie_host_init(struct pcie_port *pp)
 {
 	struct device_node *np = pp->dev->of_node;
 	struct platform_device *pdev = to_platform_device(pp->dev);
 	struct of_pci_range range;
 	struct of_pci_range_parser parser;
+	struct pci_bus *bus;
 	struct resource *cfg_res;
+	LIST_HEAD(res);
 	u32 val, na, ns;
 	const __be32 *addrp;
 	int i, index, ret;

@@ -502,15 +498,49 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	val |= PORT_LOGIC_SPEED_CHANGE;
 	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
 
-#ifdef CONFIG_PCI_MSI
-	dw_pcie_msi_chip.dev = pp->dev;
-	dw_pci.msi_ctrl = &dw_pcie_msi_chip;
+#ifdef CONFIG_ARM
+	/*
+	 * FIXME: we should really be able to use
+	 * of_pci_get_host_bridge_resources on arm32 as well,
+	 * but the conversion needs some more testing
+	 */
+	if (global_io_offset < SZ_1M && pp->io_size > 0) {
+		pci_ioremap_io(global_io_offset, pp->io_base);
+		global_io_offset += SZ_64K;
+		pci_add_resource_offset(&res, &pp->io,
+					global_io_offset - pp->io_bus_addr);
+	}
+	pci_add_resource_offset(&res, &pp->mem,
+				pp->mem.start - pp->mem_bus_addr);
+	pci_add_resource(&res, &pp->busn);
+#else
+	ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &res, &pp->io_base);
+	if (ret)
+		return ret;
+#endif
+
+	bus = pci_create_root_bus(pp->dev, pp->root_bus_nr, &dw_pcie_ops,
+			      pp, &res);
+	if (!bus)
+		return -ENOMEM;
+
+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+	bus->msi = container_of(&pp->irq_domain, struct msi_controller, domain);
+#else
+	bus->msi = &dw_pcie_msi_chip;
 #endif
 
-	dw_pci.nr_controllers = 1;
-	dw_pci.private_data = (void **)&pp;
+	pci_scan_child_bus(bus);
+	if (pp->ops->scan_bus)
+		pp->ops->scan_bus(pp);
 
-	pci_common_init_dev(pp->dev, &dw_pci);
+#ifdef CONFIG_ARM
+	/* support old dtbs that incorrectly describe IRQs */
+	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
+#endif
+
+	pci_assign_unassigned_bus_resources(bus);
+	pci_bus_add_devices(bus);
 
 	return 0;
 }

@@ -653,7 +683,7 @@ static int dw_pcie_valid_config(struct pcie_port *pp,
 static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 			int size, u32 *val)
 {
-	struct pcie_port *pp = sys_to_pcie(bus->sysdata);
+	struct pcie_port *pp = bus->sysdata;
 	int ret;
 
 	if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) {

@@ -677,7 +707,7 @@ static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 static int dw_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 			int where, int size, u32 val)
 {
-	struct pcie_port *pp = sys_to_pcie(bus->sysdata);
+	struct pcie_port *pp = bus->sysdata;
 	int ret;
 
 	if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0)

@@ -701,64 +731,6 @@ static struct pci_ops dw_pcie_ops = {
 	.write = dw_pcie_wr_conf,
 };
 
-static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
-{
-	struct pcie_port *pp;
-
-	pp = sys_to_pcie(sys);
-
-	if (global_io_offset < SZ_1M && pp->io_size > 0) {
-		sys->io_offset = global_io_offset - pp->io_bus_addr;
-		pci_ioremap_io(global_io_offset, pp->io_base);
-		global_io_offset += SZ_64K;
-		pci_add_resource_offset(&sys->resources, &pp->io,
-					sys->io_offset);
-	}
-
-	sys->mem_offset = pp->mem.start - pp->mem_bus_addr;
-	pci_add_resource_offset(&sys->resources, &pp->mem, sys->mem_offset);
-	pci_add_resource(&sys->resources, &pp->busn);

drivers/pci/host/pcie-designware.c static struct pci_ops dw_pcie_ops = { · line 730

Quoted 10 lines

drivers/pci/host/pcie-designware.c +0 -10

-
-	return 1;
-}
-
-static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
-{
-	struct pci_bus *bus;
-	struct pcie_port *pp = sys_to_pcie(sys);
-
-	pp->root_bus_nr = sys->busnr;

'pp->root_bus_nr' is initialized as '-1' at some soc-specific drivers However, 'sys->busnr' is set as '0, 1, 2 ...' by pcibios_init_hw() in arch/arm/kernel/bios32.c. '0, 1, 2 ...' means the number of each host controller.

Quoted 3 lines

So, without setting 'pp->root_bus_nr' as '0, 1, 2, ...', it makes the problem.

'sys->busnr' is set as "0, busn_res.end + 1 ..." in pcibios_init_hw() I think:

pcibios_init_hw()

	for (nr = busnr = 0; nr < hw->nr_controllers; nr++)
		sys->busnr = busnr;
		...
		busnr = sys->bus->busn_res.end + 1;

Quoted 4 lines

and it indicates root bus number of one pcie controller. so in dw_pcie_scan_bus:

	pp->root_bus_nr = sys->busnr;
	bus = pci_create_root_bus(..., sys->busnr, ...);

Oh, you're right. Sorry for my mistake.

Quoted 3 lines

we set root bus number as "0, busn_res.end + 1". But in pcie-designware, nr_controllers is set as 1, so it only comes into the loop in pcibios_init_hw() one time.

Right, 'nr_controllers' is set as just '1', even though more than 2 controllers are used.

Quoted 4 lines

so how about we set 0 as default value of pp->root_bus_nr, then use pci_create_root_bus(pp->dev, pp->root_bus_nr, &dw_pcie_ops, pp, &res); to create root bus?

I agree with your opinion. However, I still want to wait for other people's opinions. They will give good comments.
Thank you.

Best regards,
Jingoo Han

Quoted 92 lines

Quoted 92 lines

Thus, we need to come up with the way to resolve this.

1. Setting 'pp->root_bus_nr' as '0, 1, 2 ..' by each soc-specific driver.

e.g)

./drivers/pci/host/pci-exynos.c

(quoted diff) +1 -1

@@ -534,7 +534,7 @@ static int __init exynos_add_pcie_port(struct pcie_port *pp,
                }
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &exynos_pcie_host_ops;
 

./drivers/pci/host/pci-imx6.c

(quoted diff) +1 -1

@@ -541,7 +541,7 @@ static int __init imx6_add_pcie_port(struct pcie_port *pp,
                }
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &imx6_pcie_host_ops;
 

./drivers/pci/host/pci-keystone.c

(quoted diff) +1 -1

@@ -312,7 +312,7 @@ static int __init ks_add_pcie_port(struct keystone_pcie *ks_pcie,
                        return ret;
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &keystone_pcie_host_ops;
 

./drivers/pci/host/pci-layerscape.c

(quoted diff) +1 -1

@@ -101,7 +101,7 @@ static int ls_add_pcie_port(struct ls_pcie *pcie)
        pp = &pcie->pp;
        pp->dev = pcie->dev;
        pp->dbi_base = pcie->dbi;
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &ls_pcie_host_ops;
 

./drivers/pci/host/pcie-spear13xx.c

(quoted diff) +1 -1

@@ -287,7 +287,7 @@ static int spear13xx_add_pcie_port(struct pcie_port *pp,
                return ret;
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &spear13xx_pcie_host_ops;
 
 

2. Setting 'pp->root_bus_nr' as '0, 1, 2 ..' by pcie-designware.c

I believe that someone will give better idea. :-)

Quoted 20 lines

drivers/pci/host/pcie-designware.c +0 -20

-	bus = pci_create_root_bus(pp->dev, sys->busnr,
-				  &dw_pcie_ops, sys, &sys->resources);
-	if (!bus)
-		return NULL;
-
-	pci_scan_child_bus(bus);
-
-	if (bus && pp->ops->scan_bus)
-		pp->ops->scan_bus(pp);
-
-	return bus;
-}
-
-static int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
-{
-	struct pcie_port *pp = sys_to_pcie(dev->bus->sysdata);
-	int irq;
-
-	irq = of_irq_parse_and_map_pci(dev, slot, pin);
-	if (!irq)

drivers/pci/host/pcie-designware.c static struct pci_ops dw_pcie_ops = { · line 760

Quoted 10 lines

drivers/pci/host/pcie-designware.c +0 -10

-		irq = pp->irq;
-
-	return irq;
-}
-
-static struct hw_pci dw_pci = {
-	.setup		= dw_pcie_setup,
-	.scan		= dw_pcie_scan_bus,
-	.map_irq	= dw_pcie_map_irq,
-};

Right, 'struct hw_pci' should not be used in order to unify ARM32 and ARM64. I have no objection to remove 'struct hw_pci' from 'pcie-designware.c'. Thank you for your patch.

Quoted 13 lines

Quoted 3 lines

Best regards,
Jingoo Han

Many thanks for your reviewing!
Zhou Wang

Quoted 6 lines

Quoted 6 lines

drivers/pci/host/pcie-designware.c +0 -2

-
 void dw_pcie_setup_rc(struct pcie_port *pp)
 {
 	u32 val;
--

1.9.1

.

Hi Zhou,

Quoted 10 lines · Jingoo Han

Quoted 3 lines

Quoted 3 lines

so how about we set 0 as default value of pp->root_bus_nr, then use pci_create_root_bus(pp->dev, pp->root_bus_nr, &dw_pcie_ops, pp, &res); to create root bus?

I agree with your opinion. However, I still want to wait for other people's opinions. They will give good comments.
Thank you.

Best regards,
Jingoo Han

I did a quick test with your patch in the context of another driver on arm32. Just to confirm I added as well pp->root_bus_nr = 0;

Then, I needed to add dummy pci_sys_data as suggested by Arnd in the first review :
http://www.spinics.net/lists/linux-pci/msg40445.html

struct pcie_port {

(quoted diff) +9 -0

+#ifdef CONFIG_ARM
+    /*
+     * this is a temporary hack to let the driver work on
+     * both arm32 and arm64. it can be removed after the
+     * arm32 cleanup is complete and bios32.c has stopped
+     * referencing host->pci_sys_data.
+     */
+    struct pci_sys_data    dummy;
+#endif
     struct device        *dev;

This looks like necessary as bios32 is dereferencing sysdata:
struct pci_sys_data *sysdata = dev->bus->sysdata;

Best Regards,
Fabrice

.

I am a little confused that pci_sys_data is still needed as we don't use the code in bios32. What was the problem you met? Could you share me more information? Then let's see how to solve the problem.

Quoted 13 lines

struct pcie_port {

Quoted 12 lines

(quoted diff) +9 -0

+#ifdef CONFIG_ARM
+    /*
+     * this is a temporary hack to let the driver work on
+     * both arm32 and arm64. it can be removed after the
+     * arm32 cleanup is complete and bios32.c has stopped
+     * referencing host->pci_sys_data.
+     */
+    struct pci_sys_data    dummy;
+#endif
     struct device        *dev;

Quoted 6 lines

Quoted 6 lines

This looks like necessary as bios32 is dereferencing sysdata:
struct pci_sys_data *sysdata = dev->bus->sysdata;

Best Regards,
Fabrice

Hi Fabrice,

Firstly, many thanks for your test:)

I am a little confused that pci_sys_data is still needed as we don't use the code in bios32. What was the problem you met? Could you share me more information? Then let's see how to solve the problem.

Thanks again and Best Regards,
Zhou

'pp->root_bus_nr' is initialized as '-1' at some soc-specific drivers However, 'sys->busnr' is set as '0, 1, 2 ...' by pcibios_init_hw() in arch/arm/kernel/bios32.c. '0, 1, 2 ...' means the number of each host controller.

So, without setting 'pp->root_bus_nr' as '0, 1, 2, ...', it makes the problem.

Thus, we need to come up with the way to resolve this.

1. Setting 'pp->root_bus_nr' as '0, 1, 2 ..' by each soc-specific driver.

e.g)

./drivers/pci/host/pci-exynos.c

(quoted diff) +1 -1

@@ -534,7 +534,7 @@ static int __init exynos_add_pcie_port(struct pcie_port *pp,
                }
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &exynos_pcie_host_ops;
 

./drivers/pci/host/pci-imx6.c

(quoted diff) +1 -1

@@ -541,7 +541,7 @@ static int __init imx6_add_pcie_port(struct pcie_port *pp,
                }
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &imx6_pcie_host_ops;
 

./drivers/pci/host/pci-keystone.c

(quoted diff) +1 -1

@@ -312,7 +312,7 @@ static int __init ks_add_pcie_port(struct keystone_pcie *ks_pcie,
                        return ret;
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &keystone_pcie_host_ops;
 

./drivers/pci/host/pci-layerscape.c

(quoted diff) +1 -1

@@ -101,7 +101,7 @@ static int ls_add_pcie_port(struct ls_pcie *pcie)
        pp = &pcie->pp;
        pp->dev = pcie->dev;
        pp->dbi_base = pcie->dbi;
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &ls_pcie_host_ops;
 

./drivers/pci/host/pcie-spear13xx.c

(quoted diff) +1 -1

@@ -287,7 +287,7 @@ static int spear13xx_add_pcie_port(struct pcie_port *pp,
                return ret;
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &spear13xx_pcie_host_ops;
 
 

2. Setting 'pp->root_bus_nr' as '0, 1, 2 ..' by pcie-designware.c

I believe that someone will give better idea.

Hi zhou,

Quoted 21 lines · Zhou Wang

Quoted 13 lines

struct pcie_port {

Quoted 12 lines

(quoted diff) +9 -0

+#ifdef CONFIG_ARM
+    /*
+     * this is a temporary hack to let the driver work on
+     * both arm32 and arm64. it can be removed after the
+     * arm32 cleanup is complete and bios32.c has stopped
+     * referencing host->pci_sys_data.
+     */
+    struct pci_sys_data    dummy;
+#endif
     struct device        *dev;

Quoted 6 lines

Quoted 6 lines

This looks like necessary as bios32 is dereferencing sysdata:
struct pci_sys_data *sysdata = dev->bus->sysdata;

Best Regards,
Fabrice

Hi Fabrice,

Quoted 5 lines

Firstly, many thanks for your test:)

I am a little confused that pci_sys_data is still needed as we don't use the code in bios32. What was the problem you met? Could you share me more information? Then let's see how to solve the problem.

This is not completely true: there is still code in bios32 that requires pci_sys_data.

No matters pci_common_init_dev() is being called or not :

You can see pcibios_msi_controller() has a weak definition (in drivers/pci/msi.c). But bios32 defines it for arm. Then it's being used.

So, basically, pci_sys_data is still needed on arm. This is how above workaround proposed by Arnd takes care of it. Without this, dereferencing the pointer as done in bios32, makes it hazardous and lead to crashes... e.g. in pcibios_msi_controller(), pci_sys_data will point to "dev->bus->sysdata", aka "struct device *dev" above.

Best Regards,
Fabrice

Quoted 4 lines

Thanks again and Best Regards,
Zhou

.

Quoted 62 lines · On Monday 25 May 2015 14:10:37 Jingoo Han wrote:

'pp->root_bus_nr' is initialized as '-1' at some soc-specific drivers However, 'sys->busnr' is set as '0, 1, 2 ...' by pcibios_init_hw() in arch/arm/kernel/bios32.c. '0, 1, 2 ...' means the number of each host controller.

So, without setting 'pp->root_bus_nr' as '0, 1, 2, ...', it makes the problem.

Thus, we need to come up with the way to resolve this.

1. Setting 'pp->root_bus_nr' as '0, 1, 2 ..' by each soc-specific driver.

e.g)

./drivers/pci/host/pci-exynos.c

(quoted diff) +1 -1

@@ -534,7 +534,7 @@ static int __init exynos_add_pcie_port(struct pcie_port *pp,
                }
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &exynos_pcie_host_ops;
 

./drivers/pci/host/pci-imx6.c

(quoted diff) +1 -1

@@ -541,7 +541,7 @@ static int __init imx6_add_pcie_port(struct pcie_port *pp,
                }
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &imx6_pcie_host_ops;
 

./drivers/pci/host/pci-keystone.c

(quoted diff) +1 -1

@@ -312,7 +312,7 @@ static int __init ks_add_pcie_port(struct keystone_pcie *ks_pcie,
                        return ret;
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &keystone_pcie_host_ops;
 

./drivers/pci/host/pci-layerscape.c

(quoted diff) +1 -1

@@ -101,7 +101,7 @@ static int ls_add_pcie_port(struct ls_pcie *pcie)
        pp = &pcie->pp;
        pp->dev = pcie->dev;
        pp->dbi_base = pcie->dbi;
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &ls_pcie_host_ops;
 

./drivers/pci/host/pcie-spear13xx.c

(quoted diff) +1 -1

@@ -287,7 +287,7 @@ static int spear13xx_add_pcie_port(struct pcie_port *pp,
                return ret;
        }
 
-       pp->root_bus_nr = -1;
+       pp->root_bus_nr++;
        pp->ops = &spear13xx_pcie_host_ops;
 
 

2. Setting 'pp->root_bus_nr' as '0, 1, 2 ..' by pcie-designware.c

Quoted 1 line

I believe that someone will give better idea.

Assigning the root bus number through hw_pci is a historic artifact from drivers that probe multiple host bridges at the same time. Some legacy ARM platforms still do that (specifically all the ones that set nr_controllers to >1: dove, mv78xx0, orion5x, and iop), but all modern platforms should probe each host bridge separately, either from a platform driver probe function, or they only have one and hardcode that fact in the ARM platform code.

For the drivers in drivers/pci/host, we give each host controller its own PCI domain, which means we can have overlapping bus numbers and do not need the heuristic to split the available 255 bus numbers across the present host bridges.

Arnd

.

Quoted 1 line

there is still code in bios32 that requires pci_sys_data.

Right.

Quoted 2 lines

Quoted 2 lines

No matters pci_common_init_dev() is being called or not :

Quoted 2 lines

Quoted 2 lines

You can see pcibios_msi_controller() has a weak definition (in drivers/pci/msi.c). But bios32 defines it for arm. Then it's being used.

But here in dw_pcie_host_init, I directly set bus->msi = &dw_pcie_msi_chip. So in drivers/pci/msi.c, we will get the msi controller by dev->bus->msi in pci_msi_controller. It will not run into pcibios_msi_controller().

Maybe there is still something I miss understood, if so, please let me know.

Best Regards,
Zhou

Quoted 5 lines · On Tuesday 26 May 2015 10:49:51 Zhou Wang wrote:

I am a little confused that pci_sys_data is still needed as we don't use the code in bios32. What was the problem you met? Could you share me more information? Then let's see how to solve the problem.

The PCI core code still calls pcibios_align_resource(), which will try to use the dev->sysdata pointer as 'struct pci_sys_data'. To solve this, we

need to change the pci-mvebu driver and the core code first to let the

driver override pcibios_align_resource() through an operation in struct pci_host_bridge.

The other remaining use of dev->sysdata is the ARM pcibios_msi_controller() function that overrides the generic implementation. To solve this, we need to change the five remaining drivers that set hw_pci->msi_ctrl to use the new generic method, and remove the ARM specific implementation.

Arnd

.

Quoted 7 lines · Arnd Bergmann

Quoted 5 lines · On Tuesday 26 May 2015 10:49:51 Zhou Wang wrote:

I am a little confused that pci_sys_data is still needed as we don't use the code in bios32. What was the problem you met? Could you share me more information? Then let's see how to solve the problem.

Quoted 2 lines

The PCI core code still calls pcibios_align_resource(), which will try to use the dev->sysdata pointer as 'struct pci_sys_data'. To solve this, we

Thanks for reminding.

Quoted 1 line

need to change the pci-mvebu driver and the core code first to let the

It seams that only pci-mvebu implemented align_resource callback in ARM ?

Quoted 7 lines

driver override pcibios_align_resource() through an operation in struct pci_host_bridge.

The other remaining use of dev->sysdata is the ARM pcibios_msi_controller() function that overrides the generic implementation. To solve this, we need to change the five remaining drivers that set hw_pci->msi_ctrl to use the new generic method, and remove the ARM specific implementation.

Yes, that is better if we can do like this. But for pcie-designware, can we just set bus->msi = &dw_pcie_msi_chip and get msi controller using dev->bus->msi in pci_msi_controller()?

Yes, that works. However there are two problems with the approach:

- we have to change all PCI host drivers on ARM to do this in order to remove the ARM-specific pcibios_msi_controller() function
- it's possible that there are dw_pcie implementations that do not include an MSI controller, so that pointer would be NULL, which leads to the core code to still call the ARM-specific pcibios_msi_controller() function unless we remove it.

Quoted 8 lines · On Wednesday 27 May 2015 17:31:46 Arnd Bergmann wrote:

Yes, that works. However there are two problems with the approach:

- we have to change all PCI host drivers on ARM to do this in order to remove the ARM-specific pcibios_msi_controller() function
- it's possible that there are dw_pcie implementations that do not include an MSI controller, so that pointer would be NULL, which leads to the core code to still call the ARM-specific pcibios_msi_controller() function unless we remove it.

I wonder if this simple patch would be sufficient to kill off pcibios_msi_controller().

Can one of you try it?

Arnd

arch/arm/kernel/bios32.c +0 -12

index fcbbbb1b9e95..7afb6d253162 100644
@@ -18,15 +18,6 @@
 
  static int debug_pci;
 
-#ifdef CONFIG_PCI_MSI
-struct msi_controller *pcibios_msi_controller(struct pci_dev *dev)
-{
-	struct pci_sys_data *sysdata = dev->bus->sysdata;
-
-	return sysdata->msi_ctrl;
-}
-#endif
-
  /*
   * We can't use pci_get_device() here since we are
   * called from interrupt context.

@@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
  		if (!sys)
  			panic("PCI: unable to allocate sys data!");
 
-#ifdef CONFIG_PCI_MSI
-		sys->msi_ctrl = hw->msi_ctrl;
-#endif
  		sys->busnr   = busnr;
  		sys->swizzle = hw->swizzle;
  		sys->map_irq = hw->map_irq;

arch/arm/kernel/bios32.c +1 -0

+#endif
 
  			list_add(&sys->node, head);
  		} else {

Quoted 12 lines · Arnd Bergmann

Quoted 8 lines · On Wednesday 27 May 2015 17:31:46 Arnd Bergmann wrote:

Yes, that works. However there are two problems with the approach:

- we have to change all PCI host drivers on ARM to do this in order to remove the ARM-specific pcibios_msi_controller() function
- it's possible that there are dw_pcie implementations that do not include an MSI controller, so that pointer would be NULL, which leads to the core code to still call the ARM-specific pcibios_msi_controller() function unless we remove it.

I wonder if this simple patch would be sufficient to kill off pcibios_msi_controller().

Quoted 3 lines

Can one of you try it?

Arnd

Hi Arnd,
I tested it quickly on my platform, with and without CONFIG_PCI_MSI. In both case it seems to run fine with dw pcie.

Quoted 8 lines · On Wednesday 27 May 2015 17:31:46 Arnd Bergmann wrote:

Yes, that works. However there are two problems with the approach:

- we have to change all PCI host drivers on ARM to do this in order to remove the ARM-specific pcibios_msi_controller() function
- it's possible that there are dw_pcie implementations that do not include an MSI controller, so that pointer would be NULL, which leads to the core code to still call the ARM-specific pcibios_msi_controller() function unless we remove it.

I wonder if this simple patch would be sufficient to kill off pcibios_msi_controller().

Can one of you try it?

Arnd

arch/arm/kernel/bios32.c +0 -12

index fcbbbb1b9e95..7afb6d253162 100644
@@ -18,15 +18,6 @@
 
 static int debug_pci;
 
-#ifdef CONFIG_PCI_MSI
-struct msi_controller *pcibios_msi_controller(struct pci_dev *dev)
-{
-	struct pci_sys_data *sysdata = dev->bus->sysdata;
-
-	return sysdata->msi_ctrl;
-}
-#endif
-
 /*
  * We can't use pci_get_device() here since we are
  * called from interrupt context.

@@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 		if (!sys)
 			panic("PCI: unable to allocate sys data!");
 
-#ifdef CONFIG_PCI_MSI
-		sys->msi_ctrl = hw->msi_ctrl;
-#endif
 		sys->busnr   = busnr;
 		sys->swizzle = hw->swizzle;
 		sys->map_irq = hw->map_irq;

.

Quoted 12 lines · Arnd Bergmann

Quoted 8 lines · On Wednesday 27 May 2015 17:31:46 Arnd Bergmann wrote:

Yes, that works. However there are two problems with the approach:

- we have to change all PCI host drivers on ARM to do this in order to remove the ARM-specific pcibios_msi_controller() function
- it's possible that there are dw_pcie implementations that do not include an MSI controller, so that pointer would be NULL, which leads to the core code to still call the ARM-specific pcibios_msi_controller() function unless we remove it.

I wonder if this simple patch would be sufficient to kill off pcibios_msi_controller().

Quoted 3 lines

Can one of you try it?

Arnd

Hi Arnd,
I tested it quickly on my platform, with and without CONFIG_PCI_MSI. In both case it seems to run fine with dw pcie.

Quoted 29 lines

arch/arm/kernel/bios32.c +0 -10

index fcbbbb1b9e95..7afb6d253162 100644
@@ -18,15 +18,6 @@
    static int debug_pci;
  -#ifdef CONFIG_PCI_MSI
-struct msi_controller *pcibios_msi_controller(struct pci_dev *dev)
-{
-    struct pci_sys_data *sysdata = dev->bus->sysdata;
-
-    return sysdata->msi_ctrl;
-}
-#endif
-
  /*
   * We can't use pci_get_device() here since we are
   * called from interrupt context.

@@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
          if (!sys)
              panic("PCI: unable to allocate sys data!");
  -#ifdef CONFIG_PCI_MSI
-        sys->msi_ctrl = hw->msi_ctrl;
-#endif
          sys->busnr   = busnr;
          sys->swizzle = hw->swizzle;
          sys->map_irq = hw->map_irq;

arch/arm/kernel/bios32.c static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, · line 484

Quoted 4 lines

arch/arm/kernel/bios32.c +2 -0

@@ -493,6 +481,9 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
                  panic("PCI: unable to scan bus!");
                busnr = sys->bus->busn_res.end + 1;
+#ifdef CONFIG_PCI_MSI
+            bus->msi = hw->msi_ctrl;

(with small adaptation here: s/bus->msi/sys->bus->msi)

BR,
Fabrice

arch/arm/kernel/bios32.c static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,

Quoted 4 lines

arch/arm/kernel/bios32.c +1 -0

+#endif
                list_add(&sys->node, head);
          } else {
 

.

Quoted 13 lines · On Wednesday 27 May 2015 18:19:57 Fabrice Gasnier wrote:

Quoted 12 lines · Arnd Bergmann

Quoted 8 lines · On Wednesday 27 May 2015 17:31:46 Arnd Bergmann wrote:

Yes, that works. However there are two problems with the approach:

- we have to change all PCI host drivers on ARM to do this in order to remove the ARM-specific pcibios_msi_controller() function
- it's possible that there are dw_pcie implementations that do not include an MSI controller, so that pointer would be NULL, which leads to the core code to still call the ARM-specific pcibios_msi_controller() function unless we remove it.

I wonder if this simple patch would be sufficient to kill off pcibios_msi_controller().

Quoted 8 lines

Quoted 3 lines

Can one of you try it?

Arnd

Hi Arnd,
I tested it quickly on my platform, with and without CONFIG_PCI_MSI. In both case it seems to run fine with dw pcie.

Awesome. Now we just need to find a way to move the align_resource callback. I wonder if the approach below would be acceptable.

Arnd

arch/arm/kernel/bios32.c +0 -3

index fcbbbb1b9e95..20839d26a490 100644
@@ -597,9 +588,6 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 
 	start = (start + align - 1) & ~(align - 1);
 
-	if (sys->align_resource)
-		return sys->align_resource(dev, res, start, size, align);
-
 	return start;
 }
 

drivers/pci/host/pci-mvebu.c +28 -17

index 1ab863551920..74216c9c2822 100644
@@ -751,27 +751,20 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
 	return 1;
 }
 
-static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_data *sys)
+static resource_size_t mvebu_pcie_align_resource(void *data,
+						 const struct resource *res,
+						 resource_size_t size,
+						 resource_size_t align)
 {
-	struct mvebu_pcie *pcie = sys_to_pcie(sys);
-	struct pci_bus *bus;
+	struct device *dev = data;
 
-	bus = pci_create_root_bus(&pcie->pdev->dev, sys->busnr,
-				  &mvebu_pcie_ops, sys, &sys->resources);
-	if (!bus)
-		return NULL;
+	resource_size_t start = res->start;
 
-	pci_scan_child_bus(bus);
+	if (res->flags & IORESOURCE_IO && start & 0x300)
+		start = (start + 0x3ff) & ~0x3ff;
 
-	return bus;
-}
+	start = (start + align - 1) & ~(align - 1);
 
-static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev,
-						 const struct resource *res,
-						 resource_size_t start,
-						 resource_size_t size,
-						 resource_size_t align)
-{
 	if (dev->bus->number != 0)
 		return start;
 

@@ -796,6 +789,25 @@ static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev,
 		return start;
 }
 
+static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_data *sys)
+{
+	struct mvebu_pcie *pcie = sys_to_pcie(sys);
+	struct pci_host_bridge *phb;
+	struct pci_bus *bus;
+
+	bus = pci_create_root_bus(&pcie->pdev->dev, sys->busnr,
+				  &mvebu_pcie_ops, sys, &sys->resources);
+	if (!bus)
+		return NULL;
+
+	phb = pci_find_host_bridge(bus);
+	phb->align_resource = mvebu_pcie_align_resource;
+
+	pci_scan_child_bus(bus);
+
+	return bus;
+}
+
 static void mvebu_pcie_enable(struct mvebu_pcie *pcie)
 {
 	struct hw_pci hw;

@@ -812,7 +824,6 @@ static void mvebu_pcie_enable(struct mvebu_pcie *pcie)
 	hw.scan           = mvebu_pcie_scan_bus;
 	hw.map_irq        = of_irq_parse_and_map_pci;
 	hw.ops            = &mvebu_pcie_ops;
-	hw.align_resource = mvebu_pcie_align_resource;
 
 	pci_common_init(&hw);
 }

drivers/pci/setup-res.c +11 -5

index 232f9254c11a..e1f5e13c4636 100644
@@ -200,7 +200,11 @@ static int pci_revert_fw_address(struct resource *res, struct pci_dev *dev,
 }
 
 static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
-		int resno, resource_size_t size, resource_size_t align)
+		int resno, resource_size_t size, resource_size_t align,
+		resource_size_t (*alignf)(void *,
+					  const struct resource *,
+					  resource_size_t,
+					  resource_size_t))
 {
 	struct resource *res = dev->resource + resno;
 	resource_size_t min;

@@ -217,7 +221,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 	 */
 	ret = pci_bus_alloc_resource(bus, res, size, align, min,
 				     IORESOURCE_PREFETCH | IORESOURCE_MEM_64,
-				     pcibios_align_resource, dev);
+				     alignf, dev);
 	if (ret == 0)
 		return 0;
 

@@ -229,7 +233,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 	     (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) {
 		ret = pci_bus_alloc_resource(bus, res, size, align, min,
 					     IORESOURCE_PREFETCH,
-					     pcibios_align_resource, dev);
+					     alignf, dev);
 		if (ret == 0)
 			return 0;
 	}

@@ -242,7 +246,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 	 */
 	if (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64))
 		ret = pci_bus_alloc_resource(bus, res, size, align, min, 0,
-					     pcibios_align_resource, dev);
+					     alignf, dev);
 
 	return ret;
 }

@@ -251,10 +255,19 @@ static int _pci_assign_resource(struct pci_dev *dev, int resno,
 				resource_size_t size, resource_size_t min_align)
 {
 	struct pci_bus *bus;
+	struct pci_host_bridge *phb;
 	int ret;
 
 	bus = dev->bus;
-	while ((ret = __pci_assign_resource(bus, dev, resno, size, min_align))) {
+	phb = pci_find_host_bridge(bus);
+

.

I think this code is fine, and we also should add a funtion point in struct pci_host_bridge to store mvebu_pcie_align_resource. How about I make a new version patchset with above codes for further reviewing ?

Quoted 12 lines

arch/arm/kernel/bios32.c +0 -3

@@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
              if (!sys)
                      panic("PCI: unable to allocate sys data!");
 
-#ifdef CONFIG_PCI_MSI
-             sys->msi_ctrl = hw->msi_ctrl;
-#endif
              sys->busnr   = busnr;
              sys->swizzle = hw->swizzle;
              sys->map_irq = hw->map_irq;

arch/arm/kernel/bios32.c static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, · line 489

Quoted 9 lines

arch/arm/kernel/bios32.c +3 -0

@@ -493,6 +481,9 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
                              panic("PCI: unable to scan bus!");
 
                      busnr = sys->bus->busn_res.end + 1;
+#ifdef CONFIG_PCI_MSI
+                     bus->msi = hw->msi_ctrl;
+#endif
 
                      list_add(&sys->node, head);
              } else {

Hi Arnd,

I think it does not work in above way(with adaptation as Fabrice mentioned).
As the msi controller is passed to secondary bus one by one during the process of enumeration. Here we just set msi controller for root bus.