[RFC PATCH v1 0/3] PCI: hisi: Add PCIe host support for Hisilicon Soc Hip05

[RFC PATCH v1 0/3] PCI: hisi: Add PCIe host support for Hisilicon Soc Hip05

[Zhou Wang]

This patchset adds PCIe host support for Hisilicon Soc Hip05. The PCIe hosts
use PCIe IP core from Synopsys, So this driver is base on designware PCIe driver.

Hip05 is an ARMv8 architecture Soc. It should be able to use ARM64 PCIe API in
designeware PCIe driver. Following Arnd's suggestion[1], just try to unify
ARM32 and ARM64 PCIe in designware driver.

This patchset is based on v4.1-rc1.

Change from RFC:
1. delete dw_pcie_setup, dw_pcie_scan_bus, dw_pcie_map_irq and struct hw_pci,
   merge related operations into dw_pcie_host_init.

[1] http://www.spinics.net/lists/devicetree/msg76463.html
Zhou Wang (3):
  PCI: designware: Add ARM64 support
  PCI: hisi: Add PCIe host support for Hisilicon Soc Hip05
  Documentation: DT: Add hisilicon PCIe host binding

 .../devicetree/bindings/pci/hisilicon-pcie.txt     |  46 ++++
 drivers/pci/host/Kconfig                           |   5 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pcie-designware.c                 | 128 ++++-------
 drivers/pci/host/pcie-hisi.c                       | 252 +++++++++++++++++++++
 5 files changed, 354 insertions(+), 78 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
 create mode 100644 drivers/pci/host/pcie-hisi.c

-- 
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[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:
>	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 <wangzhou1@hisilicon.com>
---
 drivers/pci/host/pcie-designware.c | 128 +++++++++++++++----------------------
 1 file changed, 50 insertions(+), 78 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 2e9f84f..7bad9e5 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -22,6 +22,7 @@
 #include <linux/pci_regs.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
+#include <asm/hardirq.h>
 
 #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);
-
-	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;
-	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)
-		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,
-};
-
 void dw_pcie_setup_rc(struct pcie_port *pp)
 {
 	u32 val;
-- 
1.9.1

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Zhou Wang]

Hi Jingoo and Hi Pratyush,

Do you have any comments about this patch? It will be very appreciated if I
could get some help for you.

Best Regards,
Zhou

On 2015/5/20 14:21, Zhou Wang wrote:
> 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:
>> 	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 <wangzhou1@hisilicon.com>
> ---
>  drivers/pci/host/pcie-designware.c | 128 +++++++++++++++----------------------
>  1 file changed, 50 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 2e9f84f..7bad9e5 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -22,6 +22,7 @@
>  #include <linux/pci_regs.h>
>  #include <linux/platform_device.h>
>  #include <linux/types.h>
> +#include <asm/hardirq.h>
>  
>  #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);
> -
> -	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;
> -	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)
> -		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,
> -};
> -
>  void dw_pcie_setup_rc(struct pcie_port *pp)
>  {
>  	u32 val;
> 

[RFC PATCH v1 2/3] PCI: hisi: Add PCIe host support for Hisilicon Soc Hip05

[Zhou Wang]

This patch adds PCIe host support for Hisilicon Soc Hip05.

Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
---
 drivers/pci/host/Kconfig     |   5 +
 drivers/pci/host/Makefile    |   1 +
 drivers/pci/host/pcie-hisi.c | 252 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 258 insertions(+)
 create mode 100644 drivers/pci/host/pcie-hisi.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 1dfb567..486d822 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -125,4 +125,9 @@ config PCIE_IPROC_PLATFORM
 	  Say Y here if you want to use the Broadcom iProc PCIe controller
 	  through the generic platform bus interface
 
+config PCI_HISI
+	depends on OF && ARM64
+	bool "Hisilicon Soc HIP05 PCIe controller"
+	select PCIE_DW
+
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index f733b4e..562142e 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
 obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
 obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
 obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
+obj-$(CONFIG_PCI_HISI) += pcie-hisi.o
diff --git a/drivers/pci/host/pcie-hisi.c b/drivers/pci/host/pcie-hisi.c
new file mode 100644
index 0000000..3f8cb9a
--- /dev/null
+++ b/drivers/pci/host/pcie-hisi.c
@@ -0,0 +1,252 @@
+/*
+ * PCIe host controller driver for Hisilicon Hip05 SoCs
+ *
+ * Copyright (C) 2014 Hisilicon Co., Ltd. http://www.hisilicon.com
+ *
+ * Author: Zhou Wang <wangzhou1@hisilicon.com>
+ *         Dacai Zhu <zhudacai@hisilicon.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_pci.h>
+#include <linux/platform_device.h>
+
+#include "pcie-designware.h"
+
+#define PCIE_SUBCTRL_MODE_REG                           (0x2800)
+#define PCIE_SUBCTRL_SYS_STATE4_REG                     (0x6818)
+#define PCIE_SLV_DBI_MODE                               (0x0)
+#define PCIE_SLV_SYSCTRL_MODE                           (0x1)
+#define PCIE_SLV_CONTENT_MODE                           (0x2)
+#define PCIE_LTSSM_LINKUP_STATE                         (0x11)
+#define PCIE_LTSSM_STATE_MASK                           (0x3F)
+#define PCIE_MSI_CONTEXT_VALUE                          (0x1011000)
+#define PCIE_MSI_TRANS_ENABLE                           (0x1ff0)
+#define PCIE_MSI_LOW_ADDRESS                            (0x1b4)
+#define PCIE_MSI_HIGH_ADDRESS                           (0x1c4)
+
+#define to_hisi_pcie(x)	container_of(x, struct hisi_pcie, pp)
+
+struct hisi_pcie {
+	void __iomem *subctrl_base;
+	void __iomem *reg_base;
+	struct msi_controller *msi;
+	u32 port_id;
+	struct pcie_port pp;
+};
+
+static inline void hisi_pcie_subctrl_writel(struct hisi_pcie *pcie,
+					    u32 val, u32 reg)
+{
+	writel(val, pcie->subctrl_base + reg);
+}
+
+static inline u32 hisi_pcie_subctrl_readl(struct hisi_pcie *pcie, u32 reg)
+{
+	return readl(pcie->subctrl_base + reg);
+}
+
+static inline void hisi_pcie_apb_writel(struct hisi_pcie *pcie,
+					u32 val, u32 reg)
+{
+	writel(val, pcie->reg_base + reg);
+}
+
+static inline u32 hisi_pcie_apb_readl(struct hisi_pcie *pcie, u32 reg)
+{
+	return readl(pcie->reg_base + reg);
+}
+
+/*
+ * Change mode to indicate the same reg_base to base of PCIe host configure
+ * registers, base of RC configure space or base of vmid/asid context table
+ */
+static void hisi_pcie_change_apb_mode(struct hisi_pcie *pcie, u32 mode)
+{
+	u32 val;
+	u32 bit_mask;
+	u32 bit_shift;
+	u32 port_id = pcie->port_id;
+	u32 reg = PCIE_SUBCTRL_MODE_REG + 0x100 * port_id;
+
+	if ((port_id == 1) || (port_id == 2)) {
+		bit_mask = 0xc;
+		bit_shift = 0x2;
+	} else {
+		bit_mask = 0x6;
+		bit_shift = 0x1;
+	}
+
+	val = hisi_pcie_subctrl_readl(pcie, reg);
+	val = (val & (~bit_mask)) | (mode << bit_shift);
+	hisi_pcie_subctrl_writel(pcie, val, reg);
+}
+
+/* Configure vmid/asid table in PCIe host */
+static void hisi_pcie_config_context(struct hisi_pcie *pcie)
+{
+	int i;
+
+	hisi_pcie_change_apb_mode(pcie, PCIE_SLV_CONTENT_MODE);
+
+	for (i = 0; i < 0x400; i++)
+		hisi_pcie_apb_writel(pcie, 0x0, i * 4);
+
+	for (i = 0x400; i < 0x800; i++)
+		hisi_pcie_apb_writel(pcie, 0x0, i * 4);
+
+	hisi_pcie_change_apb_mode(pcie, PCIE_SLV_SYSCTRL_MODE);
+
+	hisi_pcie_apb_writel(pcie, 0xb7010040, PCIE_MSI_LOW_ADDRESS);
+	hisi_pcie_apb_writel(pcie, 0x0, PCIE_MSI_HIGH_ADDRESS);
+	hisi_pcie_apb_writel(pcie, PCIE_MSI_CONTEXT_VALUE, 0x10);
+	hisi_pcie_apb_writel(pcie, PCIE_MSI_TRANS_ENABLE, 0x1c8);
+
+	hisi_pcie_change_apb_mode(pcie, PCIE_SLV_DBI_MODE);
+}
+
+static int hisi_pcie_link_up(struct pcie_port *pp)
+{
+	u32 val;
+
+	struct hisi_pcie *hisi_pcie = to_hisi_pcie(pp);
+
+	val = hisi_pcie_subctrl_readl(hisi_pcie, PCIE_SUBCTRL_SYS_STATE4_REG +
+				      0x100 * hisi_pcie->port_id);
+
+	return ((val & PCIE_LTSSM_STATE_MASK) == PCIE_LTSSM_LINKUP_STATE);
+}
+
+static
+int hisi_pcie_msi_host_init(struct pcie_port *pp, struct msi_controller *chip)
+{
+	struct device_node *msi_node;
+	struct msi_controller *msi;
+	struct device_node *np = pp->dev->of_node;
+	struct hisi_pcie *hisi_pcie = to_hisi_pcie(pp);
+
+	msi_node = of_parse_phandle(np, "msi-parent", 0);
+	if (!msi_node) {
+		pr_err("failed to find msi-parent\n");
+		return -ENODEV;
+	}
+
+	msi = of_pci_find_msi_chip_by_node(msi_node);
+	hisi_pcie->msi = msi;
+
+	pp->irq_domain = msi->domain;
+
+	return 0;
+}
+
+static struct pcie_host_ops hisi_pcie_host_ops = {
+	.link_up = hisi_pcie_link_up,
+	.msi_host_init = hisi_pcie_msi_host_init,
+};
+
+static int __init hisi_add_pcie_port(struct pcie_port *pp,
+				     struct platform_device *pdev)
+{
+	int ret;
+	u32 port_id;
+	struct resource busn;
+
+	struct hisi_pcie *hisi_pcie = to_hisi_pcie(pp);
+
+	if (of_property_read_u32(pdev->dev.of_node, "port-id", &port_id)) {
+		dev_err(&pdev->dev, "failed to read port-id\n");
+		return -EINVAL;
+	}
+	if (port_id > 3) {
+		dev_err(&pdev->dev, "Invalid port-id\n");
+		return -EINVAL;
+	}
+
+	hisi_pcie->port_id = port_id;
+
+	if (of_pci_parse_bus_range(pdev->dev.of_node, &busn)) {
+		dev_err(&pdev->dev, "failed to parse bus-ranges\n");
+		return -EINVAL;
+	}
+
+	pp->root_bus_nr = busn.start;
+	pp->ops = &hisi_pcie_host_ops;
+
+	hisi_pcie_config_context(hisi_pcie);
+
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to initialize host\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __init hisi_pcie_probe(struct platform_device *pdev)
+{
+	struct hisi_pcie *hisi_pcie;
+	struct pcie_port *pp;
+	struct resource *reg;
+	struct resource *subctrl;
+	int ret;
+
+	hisi_pcie = devm_kzalloc(&pdev->dev, sizeof(*hisi_pcie), GFP_KERNEL);
+	if (!hisi_pcie)
+		return -ENOMEM;
+
+	pp = &hisi_pcie->pp;
+	pp->dev = &pdev->dev;
+
+	subctrl = platform_get_resource_byname(pdev, IORESOURCE_MEM, "subctrl");
+	hisi_pcie->subctrl_base = devm_ioremap_nocache(&pdev->dev,
+					subctrl->start, resource_size(subctrl));
+	if (IS_ERR(hisi_pcie->subctrl_base)) {
+		dev_err(pp->dev, "cannot get subctrl base\n");
+		return PTR_ERR(hisi_pcie->subctrl_base);
+	}
+
+	reg = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rc_dbi");
+	hisi_pcie->reg_base = devm_ioremap_resource(&pdev->dev, reg);
+	if (IS_ERR(hisi_pcie->reg_base)) {
+		dev_err(pp->dev, "cannot get reg base\n");
+		return PTR_ERR(hisi_pcie->reg_base);
+	}
+
+	hisi_pcie->pp.dbi_base = hisi_pcie->reg_base;
+
+	ret = hisi_add_pcie_port(pp, pdev);
+	if (ret < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, hisi_pcie);
+
+	return ret;
+}
+
+static const struct of_device_id hisi_pcie_of_match[] = {
+	{.compatible = "hisilicon,hip05-pcie",},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, hisi_pcie_of_match);
+
+static struct platform_driver hisi_pcie_driver = {
+	.probe  = hisi_pcie_probe,
+	.driver = {
+		   .name = "hisi-pcie",
+		   .owner = THIS_MODULE,
+		   .of_match_table = hisi_pcie_of_match,
+	},
+};
+
+module_platform_driver(hisi_pcie_driver);
+
+MODULE_AUTHOR("Zhou Wang <wangzhou1@huawei.com>");
+MODULE_AUTHOR("Dacai Zhu <zhudacai@huawei.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[RFC PATCH v1 3/3] Documentation: DT: Add Hisilicon PCIe host binding

[Zhou Wang]

This patch adds related DTS binding document for Hisilicon PCIe host driver.

Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
---
 .../devicetree/bindings/pci/hisilicon-pcie.txt     | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/hisilicon-pcie.txt

diff --git a/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt b/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
new file mode 100644
index 0000000..c24578d
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
@@ -0,0 +1,46 @@
+Hisilicon PCIe host bridge DT description
+
+Hisilicon PCIe host controller is based on Designware PCI core.
+It shares common functions with PCIe Designware core driver and inherits
+common properties defined in
+Documentation/devicetree/bindings/pci/designware-pci.txt.
+
+Additional properties are described here:
+
+Required properties:
+- compatible: Should contain "hisilicon,hip05-pcie".
+- reg: Should contain rc_dbi, subctrl, config registers location and length.
+- reg-names: Must include the following entries:
+  "rc_dbi": controller configuration registers;
+  "subctrl": whole PCIe hosts configuration registers;
+  "config": PCIe configuration space registers.
+- msi-parent: Should be its_pcie which is an its receiving MSI interrupts.
+- port-id: Should be 0, 1, 2 or 3.
+
+Optional properties:
+- status: Either "ok" or "disabled".
+- dma-coherent: Present if dma operations are coherent.
+
+Example:
+	pcie@0xb0080000 {
+		compatible = "hisilicon,hip05-pcie", "snps,dw-pcie";
+		reg = <0 0xb0080000 0 0x10000>, <0 0xb0000000 0 0x10000>,
+		      <0x220 0x00000000 0 0x1000>;
+		reg-names = "rc_dbi", "subctrl", "config";
+		bus-range = <0  15>;
+		msi-parent = <&its_pcie>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		dma-coherent;
+		ranges = <0x82000000 0 0xb4100000 0x220 0x00100000 0 0x00f00000>;
+		num-lanes = <8>;
+		port-id = <1>;
+		#interrupts-cells = <1>;
+		interrupts-map-mask = <0xf800 0 0 7>;
+		interrupts-map = <0x0 0 0 1 &mbigen_pcie 1 10
+				  0x0 0 0 2 &mbigen_pcie 2 11
+				  0x0 0 0 3 &mbigen_pcie 3 12
+				  0x0 0 0 4 &mbigen_pcie 4 13>;
+		status = "ok";
+	};
-- 
1.9.1

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Jingoo Han]

On Wed, 20 May 2015 14:21:39 +0800, Zhou Wang wrote:
>
> 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:
> >	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 <wangzhou1@hisilicon.com>
> ---
>  drivers/pci/host/pcie-designware.c | 128 +++++++++++++++----------------------
>  1 file changed, 50 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 2e9f84f..7bad9e5 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -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>

>  
>  #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);
> -
> -	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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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. :-)

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

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

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Zhou Wang]

On 2015/5/25 13:10, Jingoo Han wrote:
> On Wed, 20 May 2015 14:21:39 +0800, Zhou Wang wrote:
>>
>> 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:
>>> 	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 <wangzhou1-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
>> ---
>>  drivers/pci/host/pcie-designware.c | 128 +++++++++++++++----------------------
>>  1 file changed, 50 insertions(+), 78 deletions(-)
>>
>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
>> index 2e9f84f..7bad9e5 100644
>> --- a/drivers/pci/host/pcie-designware.c
>> +++ b/drivers/pci/host/pcie-designware.c
>> @@ -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 :)

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

>>  
>>  #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);
>> -
>> -	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.
> 

'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?

> 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
> @@ -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
> @@ -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
> @@ -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
> @@ -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
> @@ -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. :-)
> 
>> -	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)
>> -		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
> 

Many thanks for your reviewing!
Zhou Wang

>> -
>>  void dw_pcie_setup_rc(struct pcie_port *pp)
>>  {
>>  	u32 val;
>> -- 
>> 1.9.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 


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

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Jingoo Han]

On Monday, May 25, 2015 6:52 PM, Zhou Wang wrote:
> On 2015/5/25 13:10, Jingoo Han wrote:
> > On Wed, 20 May 2015 14:21:39 +0800, Zhou Wang wrote:
> >>
> >> 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:
> >>> 	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 <wangzhou1@hisilicon.com>
> >> ---
> >>  drivers/pci/host/pcie-designware.c | 128 +++++++++++++++----------------------
> >>  1 file changed, 50 insertions(+), 78 deletions(-)
> >>
> >> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> >> index 2e9f84f..7bad9e5 100644
> >> --- a/drivers/pci/host/pcie-designware.c
> >> +++ b/drivers/pci/host/pcie-designware.c
> >> @@ -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 :)
> 
> > 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.
> 
> >>
> >>  #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);
> >> -
> >> -	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.
> >
> 
> '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, ...);

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

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

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

> 
> > 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
> > @@ -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
> > @@ -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
> > @@ -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
> > @@ -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
> > @@ -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. :-)
> >
> >> -	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)
> >> -		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
> >
> 
> Many thanks for your reviewing!
> Zhou Wang
> 
> >> -
> >>  void dw_pcie_setup_rc(struct pcie_port *pp)
> >>  {
> >>  	u32 val;
> >> --
> >> 1.9.1

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Fabrice Gasnier]

Hi Zhou,

On 05/25/2015 03:48 PM, Jingoo Han wrote:
>> >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 {
+#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

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Zhou Wang]

On 2015/5/25 23:51, Fabrice Gasnier wrote:
> Hi Zhou,
> 
> On 05/25/2015 03:48 PM, Jingoo Han wrote:
>>> >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 {
> +#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
> 

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

> .
> 

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Arnd Bergmann]

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

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Zhou Wang]

On 2015/5/26 16:02, Arnd Bergmann wrote:
> 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

Thanks for reminding.

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

> 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()?

Best Regards,
Zhou

> 
> 	Arnd
> 
> .
> 

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Arnd Bergmann]

On Wednesday 27 May 2015 21:56:27 Zhou Wang wrote:
> On 2015/5/26 16:02, Arnd Bergmann wrote:
> > 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
> 
> Thanks for reminding.
> 
> > 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 ?

Correct. The pointer in struct pci_sys_data was added for this driver, and
no other driver has needed it so far.

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

	Arnd

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Arnd Bergmann]

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

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index fcbbbb1b9e95..7afb6d253162 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -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;
@@ -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 {

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Fabrice Gasnier]

On 05/27/2015 05:43 PM, Arnd Bergmann wrote:
> 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

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.

>
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index fcbbbb1b9e95..7afb6d253162 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -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;
> @@ -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
> +#endif
>   
>   			list_add(&sys->node, head);
>   		} else {
>

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Arnd Bergmann]

On Wednesday 27 May 2015 18:19:57 Fabrice Gasnier wrote:
> On 05/27/2015 05:43 PM, Arnd Bergmann wrote:
> > 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
> 
> 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

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index fcbbbb1b9e95..20839d26a490 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -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;
 }
 
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 1ab863551920..74216c9c2822 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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);
 }
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 232f9254c11a..e1f5e13c4636 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -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);
+
+	if (phb->align_resource)
+		alignf = phb->align_resource;
+	else
+		alignf = pcibios_align_resource;
+
+	while ((ret = __pci_assign_resource(bus, dev, resno, size,
+					    min_align, alignf))) {
 		if (!bus->parent || !bus->self->transparent)
 			break;
 		bus = bus->parent;

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Zhou Wang]

On 2015/5/28 3:51, Arnd Bergmann wrote:
> On Wednesday 27 May 2015 18:19:57 Fabrice Gasnier wrote:
>> On 05/27/2015 05:43 PM, Arnd Bergmann wrote:
>>> 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
>>
>> 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
> 

Hi Arnd,

Many thanks for your kindly suggestions.

> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index fcbbbb1b9e95..20839d26a490 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -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;
>  }
>  
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index 1ab863551920..74216c9c2822 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -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);
>  }
> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index 232f9254c11a..e1f5e13c4636 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
> @@ -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);
> +
> +	if (phb->align_resource)
> +		alignf = phb->align_resource;
> +	else
> +		alignf = pcibios_align_resource;
> +
> +	while ((ret = __pci_assign_resource(bus, dev, resno, size,
> +					    min_align, alignf))) {
>  		if (!bus->parent || !bus->self->transparent)
>  			break;
>  		bus = bus->parent;
> 

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 ?

Best Regards,
Zhou

> 
> .
> 

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Arnd Bergmann]

On Thursday 28 May 2015 19:48:37 Zhou Wang wrote:
> 
> 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 ?
> 

Yes, that would be nice. I've run into a few build errors with the first version,
see below for a version that actually builds. You can base on top of that.

We also need to do something about pci_find_host_bridge() here, possibly just
storing a pointer to the host bridge in pci_bus to avoid the function call.

A few of the other members in pci_bus (ops, msi, sysdata, domain_nr, and
maybe more) could eventually be moved into pci_host_bridge as well, but
that requires changing all code accessing those fields of course.

	Arnd

commit 8140432e10b7cd59ec6246c2b7268abfccdeaaae
Author: Arnd Bergmann <arnd@arndb.de>
Date:   Thu May 28 12:21:27 2015 +0200

    [HACK] try to get rid of pci_sys_data dependency in bios32
    
    This is an experimental patch to remove the last two dependencies
    we have.
    
    Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index fcbbbb1b9e95..20839d26a490 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -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;
@@ -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
+			sys->bus->msi = hw->msi_ctrl;
+#endif
 
 			list_add(&sys->node, head);
 		} else {
@@ -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;
 }
 
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 1ab863551920..155d05fc11b9 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -22,6 +22,8 @@
 #include <linux/of_pci.h>
 #include <linux/of_platform.h>
 
+#include "../pci.h" /* HACK to see pci_find_host_bridge */
+
 /*
  * PCIe unit register offsets.
  */
@@ -751,27 +753,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 pci_dev *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 +791,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 +826,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);
 }
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 232f9254c11a..73abca7ccc7a 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -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,23 @@ 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;
+	resource_size_t (*alignf)(void *,
+				  const struct resource *,
+				  resource_size_t,
+				  resource_size_t);
 	int ret;
 
 	bus = dev->bus;
-	while ((ret = __pci_assign_resource(bus, dev, resno, size, min_align))) {
+	phb = pci_find_host_bridge(bus);
+
+	if (phb->align_resource)
+		alignf = phb->align_resource;
+	else
+		alignf = pcibios_align_resource;
+
+	while ((ret = __pci_assign_resource(bus, dev, resno, size,
+					    min_align, alignf))) {
 		if (!bus->parent || !bus->self->transparent)
 			break;
 		bus = bus->parent;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index bc50bb05f0ab..3aad084f12ad 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -404,6 +404,9 @@ struct pci_host_bridge {
 	struct device dev;
 	struct pci_bus *bus;		/* root bus */
 	struct list_head windows;	/* resource_entry */
+	resource_size_t (*align_resource)(void *data,
+			 const struct resource *res,
+			 resource_size_t size, resource_size_t align);
 	void (*release_fn)(struct pci_host_bridge *);
 	void *release_data;
 	unsigned int ignore_reset_delay:1;	/* for entire hierarchy */

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Zhou Wang]

On 2015/5/28 0:19, Fabrice Gasnier wrote:
> On 05/27/2015 05:43 PM, Arnd Bergmann wrote:
>> 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
> 
> 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.
>

Hi Fabrice,

Thanks for testing. But if you applied this patch, the code will not go into
pcibios_init_hw(), so it did not touch what Arnd modified.

Best Regards,
Zhou

>>
>> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
>> index fcbbbb1b9e95..7afb6d253162 100644
>> --- a/arch/arm/kernel/bios32.c
>> +++ b/arch/arm/kernel/bios32.c
>> @@ -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;
>> @@ -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
>> +#endif
>>                 list_add(&sys->node, head);
>>           } else {
>>
> 
> 
> .
> 

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Zhou Wang]

On 2015/5/27 23:43, Arnd Bergmann wrote:
> 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
> 
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index fcbbbb1b9e95..7afb6d253162 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -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;
> @@ -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.

Best Regards,
Zhou

> 
> .
> 

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Arnd Bergmann]

On Thursday 28 May 2015 19:34:52 Zhou Wang wrote:
> > @@ -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;
> > @@ -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.

Ah, too bad. I guess you are right, the bus->msi pointer here would
really need to be set between pci_create_root_bus() and pci_scan_child_bus(),
but we currently don't run any ARM specific code between the two.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Fabrice Gasnier]

Hi zhou,

On 05/26/2015 04:49 AM, Zhou Wang wrote:
>>   struct pcie_port {
>> >+#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
>> >
> 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.

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
>
> Thanks again and Best Regards,
> Zhou
>

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Zhou Wang]

On 2015/5/26 16:03, Fabrice Gasnier wrote:
> Hi zhou,
> 
> On 05/26/2015 04:49 AM, Zhou Wang wrote:
>>>   struct pcie_port {
>>> >+#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
>>> >
>> 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.
> 
> This is not completely true: there is still code in bios32 that requires pci_sys_data.

Right.

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

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

> 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
>>
>> Thanks again and Best Regards,
>> Zhou
>>
> 
> 
> .
> 

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Fabrice Gasnier]

On 05/27/2015 03:24 PM, Zhou Wang wrote:
>> there is still code in bios32 that requires pci_sys_data.
> Right.
>
>> >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.
> 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.
Hi Zhou,

Sorry, my mistake, you're right.
I double checked, issue I'm seeing is in pcibios_align_resource...
Please refer to Arnd's answer.

Best Regards,
Fabrice
>
> Best Regards,
> Zhou
>

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Zhou Wang]

On 2015/5/25 21:48, Jingoo Han wrote:
> On Monday, May 25, 2015 6:52 PM, Zhou Wang wrote:
>> On 2015/5/25 13:10, Jingoo Han wrote:
>>> On Wed, 20 May 2015 14:21:39 +0800, Zhou Wang wrote:
>>>>
>>>> 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:
>>>>> 	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 <wangzhou1@hisilicon.com>
>>>> ---
>>>>  drivers/pci/host/pcie-designware.c | 128 +++++++++++++++----------------------
>>>>  1 file changed, 50 insertions(+), 78 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
>>>> index 2e9f84f..7bad9e5 100644
>>>> --- a/drivers/pci/host/pcie-designware.c
>>>> +++ b/drivers/pci/host/pcie-designware.c
>>>> @@ -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 :)
>>
>>> 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.
>>
>>>>
>>>>  #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);
>>>> -
>>>> -	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.
>>>
>>
>> '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, ...);
> 
> Oh, you're right. Sorry for my mistake.
> 
>> 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.
> 
>>
>> 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
>

Right, the more comments, the better the driver will be.
Thanks for your reviewing.

Best regards,
Zhou

>>
>>> 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
>>> @@ -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
>>> @@ -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
>>> @@ -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
>>> @@ -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
>>> @@ -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. :-)
>>>
>>>> -	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)
>>>> -		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
>>>
>>
>> Many thanks for your reviewing!
>> Zhou Wang
>>
>>>> -
>>>>  void dw_pcie_setup_rc(struct pcie_port *pp)
>>>>  {
>>>>  	u32 val;
>>>> --
>>>> 1.9.1
> 
> 
> .
> 

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Arnd Bergmann]

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
> @@ -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
> @@ -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
> @@ -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
> @@ -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
> @@ -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. 

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

Re: [RFC PATCH v1 1/3] PCI: designware: Add ARM64 support

[Zhou Wang]

On 2015/5/26 16:09, Arnd Bergmann wrote:
> 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
>> @@ -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
>> @@ -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
>> @@ -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
>> @@ -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
>> @@ -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. 
> 
> 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

Hi Arnd,

Thanks for your explanation about this.

Best Regards,
Zhou

> 
> .
>