[PATCH 0/2] PCI: iproc: add bcma PCIe 2.0 controller

[PATCH 0/2] PCI: iproc: add bcma PCIe 2.0 controller

[Hauke Mehrtens]

After Ray submitted the PCIe controller for the iProc devices using 
pure device tree this adds support for the devices using bcma to scan 
the bus. This should replaces the driver I initially send for inclusion.

@Ray The driver found in the Broadcom SDK does some more stuff:
* setting up some DMA memory areas
* chaining MPS and MRRS to 512
* PHY changes:
 * "improving" the PCIe jitter 
 * some special initializations for the 3rd PCIe port

For me it works without these additions, but I haven't tested for 
performance or stability or 3rd port. Can you check if this is needed 
at all for the devices found on the market?

Hauke Mehrtens (2):
  PCI: iproc: make of_irq_parse_and_map_pci() configurable
  PCI: iproc: add bcma pcie driver

 drivers/pci/host/Kconfig               |  11 ++++
 drivers/pci/host/Makefile              |   1 +
 drivers/pci/host/pcie-iproc-bcma.c     | 112 +++++++++++++++++++++++++++++++++
 drivers/pci/host/pcie-iproc-platform.c |   2 +
 drivers/pci/host/pcie-iproc.c          |   2 +-
 drivers/pci/host/pcie-iproc.h          |   1 +
 6 files changed, 128 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pci/host/pcie-iproc-bcma.c

-- 
2.1.4

[PATCH 1/2] PCI: iproc: make of_irq_parse_and_map_pci() configurable

[Hauke Mehrtens]

This patch makes the generic code not use the function
of_irq_parse_and_map_pci() always to get the irqs, but to take a
function the actual driver has provided.  This is needed to make this
function work with drivers not using device tree.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/pci/host/pcie-iproc-platform.c | 2 ++
 drivers/pci/host/pcie-iproc.c          | 2 +-
 drivers/pci/host/pcie-iproc.h          | 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
index afad6c2..c8aa06f 100644
--- a/drivers/pci/host/pcie-iproc-platform.c
+++ b/drivers/pci/host/pcie-iproc-platform.c
@@ -71,6 +71,8 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
 
 	pcie->resources = &res;
 
+	pcie->map_irq = of_irq_parse_and_map_pci;
+
 	ret = iproc_pcie_setup(pcie);
 	if (ret) {
 		dev_err(pcie->dev, "PCIe controller setup failed\n");
diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 329e1b5..cef31f6 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -229,7 +229,7 @@ int iproc_pcie_setup(struct iproc_pcie *pcie)
 
 	pci_scan_child_bus(bus);
 	pci_assign_unassigned_bus_resources(bus);
-	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
+	pci_fixup_irqs(pci_common_swizzle, pcie->map_irq);
 	pci_bus_add_devices(bus);
 
 	return 0;
diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
index e28075e..a333d4b 100644
--- a/drivers/pci/host/pcie-iproc.h
+++ b/drivers/pci/host/pcie-iproc.h
@@ -34,6 +34,7 @@ struct iproc_pcie {
 	struct pci_bus *root_bus;
 	struct phy *phy;
 	int irqs[IPROC_PCIE_MAX_NUM_IRQS];
+	int (*map_irq)(const struct pci_dev *, u8, u8);
 };
 
 int iproc_pcie_setup(struct iproc_pcie *pcie);
-- 
2.1.4

[PATCH 1/2] PCI: iproc: make of_irq_parse_and_map_pci() configurable

[Ray Jui]

Hi Hauke,

On 5/12/2015 2:23 PM, Hauke Mehrtens wrote:
> This patch makes the generic code not use the function
> of_irq_parse_and_map_pci() always to get the irqs, but to take a
> function the actual driver has provided.  This is needed to make this
> function work with drivers not using device tree.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/pci/host/pcie-iproc-platform.c | 2 ++
>  drivers/pci/host/pcie-iproc.c          | 2 +-
>  drivers/pci/host/pcie-iproc.h          | 1 +
>  3 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
> index afad6c2..c8aa06f 100644
> --- a/drivers/pci/host/pcie-iproc-platform.c
> +++ b/drivers/pci/host/pcie-iproc-platform.c
> @@ -71,6 +71,8 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
>  
>  	pcie->resources = &res;
>  
> +	pcie->map_irq = of_irq_parse_and_map_pci;
> +
>  	ret = iproc_pcie_setup(pcie);
>  	if (ret) {
>  		dev_err(pcie->dev, "PCIe controller setup failed\n");
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index 329e1b5..cef31f6 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -229,7 +229,7 @@ int iproc_pcie_setup(struct iproc_pcie *pcie)
>  
>  	pci_scan_child_bus(bus);
>  	pci_assign_unassigned_bus_resources(bus);
> -	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> +	pci_fixup_irqs(pci_common_swizzle, pcie->map_irq);
>  	pci_bus_add_devices(bus);
>  
>  	return 0;
> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
> index e28075e..a333d4b 100644
> --- a/drivers/pci/host/pcie-iproc.h
> +++ b/drivers/pci/host/pcie-iproc.h
> @@ -34,6 +34,7 @@ struct iproc_pcie {
>  	struct pci_bus *root_bus;
>  	struct phy *phy;
>  	int irqs[IPROC_PCIE_MAX_NUM_IRQS];
> +	int (*map_irq)(const struct pci_dev *, u8, u8);
>  };
>  
>  int iproc_pcie_setup(struct iproc_pcie *pcie);
> 

This change looks fine to me.

Reviewed-by: Ray Jui <rjui@broadcom.com.com>

Thanks,

Ray

[PATCH 2/2] PCI: iproc: add bcma pcie driver

[Hauke Mehrtens]

This driver adds support for the PCIe 2.0 controller found on the bcma
bus. This controller can be found on (mostly) all Broadcom BCM470X /
BCM5301X ARM SoCs.

The driver found in the Broadcom SDK does some more stuff, like setting
up some DMA memory areas, chaining MPS and MRRS to 512 and also some
PHY changes like "improving" the PCIe jitter and doing some special
initializations for the 3rd PCIe port.

This was tested on a bcm4708 board with 2 PCIe ports and wireless cards
connected to them.

PCI_DOMAINS is needed by this driver, because normally there is more
than one PCIe controller and without PCI_DOMAINS only the first
controller gets registered.
This controller gets 6 IRQs, the last one is trigged by all IRQ events.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/pci/host/Kconfig           |  11 ++++
 drivers/pci/host/Makefile          |   1 +
 drivers/pci/host/pcie-iproc-bcma.c | 112 +++++++++++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+)
 create mode 100644 drivers/pci/host/pcie-iproc-bcma.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 1dfb567..f23f96d 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -125,4 +125,15 @@ config PCIE_IPROC_PLATFORM
 	  Say Y here if you want to use the Broadcom iProc PCIe controller
 	  through the generic platform bus interface
 
+config PCIE_IPROC_BCMA
+	bool "Broadcom iProc PCIe bcma bus driver"
+	depends on ARCH_BCM_IPROC || (ARM && COMPILE_TEST)
+	select PCIE_IPROC
+	select BCMA
+	select PCI_DOMAINS
+	default ARCH_BCM_5301X
+	help
+	  Say Y here if you want to use the Broadcom iProc PCIe controller
+	  through the bcma bus interface
+
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index f733b4e..2f7c36f 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_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
diff --git a/drivers/pci/host/pcie-iproc-bcma.c b/drivers/pci/host/pcie-iproc-bcma.c
new file mode 100644
index 0000000..c318f19
--- /dev/null
+++ b/drivers/pci/host/pcie-iproc-bcma.c
@@ -0,0 +1,112 @@
+/* 
+ * Copyright (C) 2015 Broadcom Corporation
+ * Copyright (C) 2015 Hauke Mehrtens <hauke@hauke-m.de>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/phy/phy.h>
+#include <linux/bcma/bcma.h>
+#include <linux/ioport.h>
+
+#include "pcie-iproc.h"
+
+
+/* NS: CLASS field is R/O, and set to wrong 0x200 value */
+static void bcma_pcie2_fixup_class(struct pci_dev *dev)
+{
+	dev->class = PCI_CLASS_BRIDGE_PCI << 8;
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x8011, bcma_pcie2_fixup_class);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x8012, bcma_pcie2_fixup_class);
+
+static int iproc_pcie_bcma_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
+{
+	struct pci_sys_data *sys = dev->sysdata;
+	struct iproc_pcie *pcie = sys->private_data;
+	struct bcma_device *bdev = container_of(pcie->dev, struct bcma_device, dev);
+
+	return bcma_core_irq(bdev, 5);
+}
+
+static int iproc_pcie_bcma_probe(struct bcma_device *bdev)
+{
+	struct iproc_pcie *pcie;
+	LIST_HEAD(res);
+	struct resource res_mem;
+	int ret;
+
+	pcie = devm_kzalloc(&bdev->dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pcie->dev = &bdev->dev;
+	bcma_set_drvdata(bdev, pcie);
+
+	pcie->base = bdev->io_addr;
+
+	res_mem.start = bdev->addr_s[0];
+	res_mem.end = bdev->addr_s[0] + SZ_128M - 1;
+	res_mem.name = "PCIe MEM space";
+	res_mem.flags = IORESOURCE_MEM;
+	pci_add_resource(&res, &res_mem);
+
+	pcie->resources = &res;
+
+	pcie->map_irq = iproc_pcie_bcma_map_irq;
+
+	ret = iproc_pcie_setup(pcie);
+	if (ret) {
+		dev_err(pcie->dev, "PCIe controller setup failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void iproc_pcie_bcma_remove(struct bcma_device *bdev)
+{
+	struct iproc_pcie *pcie = bcma_get_drvdata(bdev);
+
+	iproc_pcie_remove(pcie);
+}
+
+static const struct bcma_device_id iproc_pcie_bcma_table[] = {
+	BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_NS_PCIEG2, BCMA_ANY_REV, BCMA_ANY_CLASS),
+	{},
+};
+MODULE_DEVICE_TABLE(bcma, iproc_pcie_bcma_table);
+
+static struct bcma_driver iproc_pcie_bcma_driver = {
+	.name		= KBUILD_MODNAME,
+	.id_table	= iproc_pcie_bcma_table,
+	.probe		= iproc_pcie_bcma_probe,
+	.remove		= iproc_pcie_bcma_remove,
+};
+
+static int __init iproc_pcie_bcma_init(void)
+{
+	return bcma_driver_register(&iproc_pcie_bcma_driver);
+}
+module_init(iproc_pcie_bcma_init);
+
+static void __exit iproc_pcie_bcma_exit(void)
+{
+	bcma_driver_unregister(&iproc_pcie_bcma_driver);
+}
+module_exit(iproc_pcie_bcma_exit);
+
+MODULE_AUTHOR("Hauke Mehrtens");
+MODULE_DESCRIPTION("Broadcom iPROC PCIe bcma driver");
+MODULE_LICENSE("GPLv2");
-- 
2.1.4

[PATCH 2/2] PCI: iproc: add bcma pcie driver

[Ray Jui]

Hi Hauke,

On 5/12/2015 2:23 PM, Hauke Mehrtens wrote:
> This driver adds support for the PCIe 2.0 controller found on the bcma
> bus. This controller can be found on (mostly) all Broadcom BCM470X /
> BCM5301X ARM SoCs.
> 
> The driver found in the Broadcom SDK does some more stuff, like setting
> up some DMA memory areas, chaining MPS and MRRS to 512 and also some
> PHY changes like "improving" the PCIe jitter and doing some special
> initializations for the 3rd PCIe port.
> 
> This was tested on a bcm4708 board with 2 PCIe ports and wireless cards
> connected to them.
> 
> PCI_DOMAINS is needed by this driver, because normally there is more
> than one PCIe controller and without PCI_DOMAINS only the first
> controller gets registered.
> This controller gets 6 IRQs, the last one is trigged by all IRQ events.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/pci/host/Kconfig           |  11 ++++
>  drivers/pci/host/Makefile          |   1 +
>  drivers/pci/host/pcie-iproc-bcma.c | 112 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 124 insertions(+)
>  create mode 100644 drivers/pci/host/pcie-iproc-bcma.c
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 1dfb567..f23f96d 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -125,4 +125,15 @@ config PCIE_IPROC_PLATFORM
>  	  Say Y here if you want to use the Broadcom iProc PCIe controller
>  	  through the generic platform bus interface
>  
> +config PCIE_IPROC_BCMA
> +	bool "Broadcom iProc PCIe bcma bus driver"
> +	depends on ARCH_BCM_IPROC || (ARM && COMPILE_TEST)
> +	select PCIE_IPROC
> +	select BCMA
> +	select PCI_DOMAINS
> +	default ARCH_BCM_5301X
> +	help
> +	  Say Y here if you want to use the Broadcom iProc PCIe controller
> +	  through the bcma bus interface
> +
>  endmenu
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index f733b4e..2f7c36f 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_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
> diff --git a/drivers/pci/host/pcie-iproc-bcma.c b/drivers/pci/host/pcie-iproc-bcma.c
> new file mode 100644
> index 0000000..c318f19
> --- /dev/null
> +++ b/drivers/pci/host/pcie-iproc-bcma.c
> @@ -0,0 +1,112 @@
> +/* 
> + * Copyright (C) 2015 Broadcom Corporation
> + * Copyright (C) 2015 Hauke Mehrtens <hauke@hauke-m.de>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/pci.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/phy/phy.h>
> +#include <linux/bcma/bcma.h>
> +#include <linux/ioport.h>
> +
> +#include "pcie-iproc.h"
> +
> +
> +/* NS: CLASS field is R/O, and set to wrong 0x200 value */
> +static void bcma_pcie2_fixup_class(struct pci_dev *dev)
> +{
> +	dev->class = PCI_CLASS_BRIDGE_PCI << 8;
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x8011, bcma_pcie2_fixup_class);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x8012, bcma_pcie2_fixup_class);
> +
> +static int iproc_pcie_bcma_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
> +{
> +	struct pci_sys_data *sys = dev->sysdata;
> +	struct iproc_pcie *pcie = sys->private_data;
> +	struct bcma_device *bdev = container_of(pcie->dev, struct bcma_device, dev);
> +
> +	return bcma_core_irq(bdev, 5);
> +}
> +
> +static int iproc_pcie_bcma_probe(struct bcma_device *bdev)
> +{
> +	struct iproc_pcie *pcie;
> +	LIST_HEAD(res);
> +	struct resource res_mem;
> +	int ret;
> +
> +	pcie = devm_kzalloc(&bdev->dev, sizeof(*pcie), GFP_KERNEL);
> +	if (!pcie)
> +		return -ENOMEM;
> +
> +	pcie->dev = &bdev->dev;
> +	bcma_set_drvdata(bdev, pcie);
> +
> +	pcie->base = bdev->io_addr;
> +
> +	res_mem.start = bdev->addr_s[0];
> +	res_mem.end = bdev->addr_s[0] + SZ_128M - 1;
> +	res_mem.name = "PCIe MEM space";
> +	res_mem.flags = IORESOURCE_MEM;
> +	pci_add_resource(&res, &res_mem);
> +
> +	pcie->resources = &res;
> +
> +	pcie->map_irq = iproc_pcie_bcma_map_irq;
> +
> +	ret = iproc_pcie_setup(pcie);
> +	if (ret) {
> +		dev_err(pcie->dev, "PCIe controller setup failed\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void iproc_pcie_bcma_remove(struct bcma_device *bdev)
> +{
> +	struct iproc_pcie *pcie = bcma_get_drvdata(bdev);
> +
> +	iproc_pcie_remove(pcie);
> +}
> +
> +static const struct bcma_device_id iproc_pcie_bcma_table[] = {
> +	BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_NS_PCIEG2, BCMA_ANY_REV, BCMA_ANY_CLASS),
> +	{},
> +};
> +MODULE_DEVICE_TABLE(bcma, iproc_pcie_bcma_table);
> +
> +static struct bcma_driver iproc_pcie_bcma_driver = {
> +	.name		= KBUILD_MODNAME,
> +	.id_table	= iproc_pcie_bcma_table,
> +	.probe		= iproc_pcie_bcma_probe,
> +	.remove		= iproc_pcie_bcma_remove,
> +};
> +
> +static int __init iproc_pcie_bcma_init(void)
> +{
> +	return bcma_driver_register(&iproc_pcie_bcma_driver);
> +}
> +module_init(iproc_pcie_bcma_init);
> +
> +static void __exit iproc_pcie_bcma_exit(void)
> +{
> +	bcma_driver_unregister(&iproc_pcie_bcma_driver);
> +}
> +module_exit(iproc_pcie_bcma_exit);
> +
> +MODULE_AUTHOR("Hauke Mehrtens");
> +MODULE_DESCRIPTION("Broadcom iPROC PCIe bcma driver");
> +MODULE_LICENSE("GPLv2");
> 

This change looks okay to me in general. But I'm not that familiar with
BCMA, so someone else should also review this code.

Thanks,

Ray

[PATCH 2/2] PCI: iproc: add bcma pcie driver

[Rafał Miłecki]

On 12 May 2015 at 23:23, Hauke Mehrtens <hauke@hauke-m.de> wrote:
> This driver adds support for the PCIe 2.0 controller found on the bcma
> bus. This controller can be found on (mostly) all Broadcom BCM470X /
> BCM5301X ARM SoCs.
>
> The driver found in the Broadcom SDK does some more stuff, like setting
> up some DMA memory areas, chaining MPS and MRRS to 512 and also some
> PHY changes like "improving" the PCIe jitter and doing some special
> initializations for the 3rd PCIe port.
>
> This was tested on a bcm4708 board with 2 PCIe ports and wireless cards
> connected to them.
>
> PCI_DOMAINS is needed by this driver, because normally there is more
> than one PCIe controller and without PCI_DOMAINS only the first
> controller gets registered.
> This controller gets 6 IRQs, the last one is trigged by all IRQ events.
>
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

Acked-by: Rafa? Mi?ecki <zajec5@gmail.com>


> +static int iproc_pcie_bcma_probe(struct bcma_device *bdev)
> +{
> +       struct iproc_pcie *pcie;
> +       LIST_HEAD(res);
> +       struct resource res_mem;
> +       int ret;
> +
> +       pcie = devm_kzalloc(&bdev->dev, sizeof(*pcie), GFP_KERNEL);
> +       if (!pcie)
> +               return -ENOMEM;
> +
> +       pcie->dev = &bdev->dev;
> +       bcma_set_drvdata(bdev, pcie);
> +
> +       pcie->base = bdev->io_addr;
> +
> +       res_mem.start = bdev->addr_s[0];
> +       res_mem.end = bdev->addr_s[0] + SZ_128M - 1;
> +       res_mem.name = "PCIe MEM space";
> +       res_mem.flags = IORESOURCE_MEM;
> +       pci_add_resource(&res, &res_mem);
> +
> +       pcie->resources = &res;
> +
> +       pcie->map_irq = iproc_pcie_bcma_map_irq;
> +
> +       ret = iproc_pcie_setup(pcie);

I think I don't like this part of iproc design. It lefts
pcie->resources pointing to some random memory after the setup/probe
are done. Guess it should be a separated parameter or sth.

The patch is still OK, I just refer to generic iproc possible issue.

[PATCH 2/2] PCI: iproc: add bcma pcie driver

[Ray Jui]

Hi Rafal,

On 5/12/2015 11:27 PM, Rafa? Mi?ecki wrote:
> On 12 May 2015 at 23:23, Hauke Mehrtens <hauke@hauke-m.de> wrote:
>> This driver adds support for the PCIe 2.0 controller found on the bcma
>> bus. This controller can be found on (mostly) all Broadcom BCM470X /
>> BCM5301X ARM SoCs.
>>
>> The driver found in the Broadcom SDK does some more stuff, like setting
>> up some DMA memory areas, chaining MPS and MRRS to 512 and also some
>> PHY changes like "improving" the PCIe jitter and doing some special
>> initializations for the 3rd PCIe port.
>>
>> This was tested on a bcm4708 board with 2 PCIe ports and wireless cards
>> connected to them.
>>
>> PCI_DOMAINS is needed by this driver, because normally there is more
>> than one PCIe controller and without PCI_DOMAINS only the first
>> controller gets registered.
>> This controller gets 6 IRQs, the last one is trigged by all IRQ events.
>>
>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> 
> Acked-by: Rafa? Mi?ecki <zajec5@gmail.com>
> 
> 
>> +static int iproc_pcie_bcma_probe(struct bcma_device *bdev)
>> +{
>> +       struct iproc_pcie *pcie;
>> +       LIST_HEAD(res);
>> +       struct resource res_mem;
>> +       int ret;
>> +
>> +       pcie = devm_kzalloc(&bdev->dev, sizeof(*pcie), GFP_KERNEL);
>> +       if (!pcie)
>> +               return -ENOMEM;
>> +
>> +       pcie->dev = &bdev->dev;
>> +       bcma_set_drvdata(bdev, pcie);
>> +
>> +       pcie->base = bdev->io_addr;
>> +
>> +       res_mem.start = bdev->addr_s[0];
>> +       res_mem.end = bdev->addr_s[0] + SZ_128M - 1;
>> +       res_mem.name = "PCIe MEM space";
>> +       res_mem.flags = IORESOURCE_MEM;
>> +       pci_add_resource(&res, &res_mem);
>> +
>> +       pcie->resources = &res;
>> +
>> +       pcie->map_irq = iproc_pcie_bcma_map_irq;
>> +
>> +       ret = iproc_pcie_setup(pcie);
> 
> I think I don't like this part of iproc design. It lefts
> pcie->resources pointing to some random memory after the setup/probe
> are done. Guess it should be a separated parameter or sth.
> 
> The patch is still OK, I just refer to generic iproc possible issue.
> 
Sorry Rafal, but could you please be more specific on this?

Thanks,

Ray

[PATCH 2/2] PCI: iproc: add bcma pcie driver

[Rafał Miłecki]

On 13 May 2015 at 17:56, Ray Jui <rjui@broadcom.com> wrote:
> On 5/12/2015 11:27 PM, Rafa? Mi?ecki wrote:
>> On 12 May 2015 at 23:23, Hauke Mehrtens <hauke@hauke-m.de> wrote:
>>> This driver adds support for the PCIe 2.0 controller found on the bcma
>>> bus. This controller can be found on (mostly) all Broadcom BCM470X /
>>> BCM5301X ARM SoCs.
>>>
>>> The driver found in the Broadcom SDK does some more stuff, like setting
>>> up some DMA memory areas, chaining MPS and MRRS to 512 and also some
>>> PHY changes like "improving" the PCIe jitter and doing some special
>>> initializations for the 3rd PCIe port.
>>>
>>> This was tested on a bcm4708 board with 2 PCIe ports and wireless cards
>>> connected to them.
>>>
>>> PCI_DOMAINS is needed by this driver, because normally there is more
>>> than one PCIe controller and without PCI_DOMAINS only the first
>>> controller gets registered.
>>> This controller gets 6 IRQs, the last one is trigged by all IRQ events.
>>>
>>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>>
>> Acked-by: Rafa? Mi?ecki <zajec5@gmail.com>
>>
>>
>>> +static int iproc_pcie_bcma_probe(struct bcma_device *bdev)
>>> +{
>>> +       struct iproc_pcie *pcie;
>>> +       LIST_HEAD(res);
>>> +       struct resource res_mem;
>>> +       int ret;
>>> +
>>> +       pcie = devm_kzalloc(&bdev->dev, sizeof(*pcie), GFP_KERNEL);
>>> +       if (!pcie)
>>> +               return -ENOMEM;
>>> +
>>> +       pcie->dev = &bdev->dev;
>>> +       bcma_set_drvdata(bdev, pcie);
>>> +
>>> +       pcie->base = bdev->io_addr;
>>> +
>>> +       res_mem.start = bdev->addr_s[0];
>>> +       res_mem.end = bdev->addr_s[0] + SZ_128M - 1;
>>> +       res_mem.name = "PCIe MEM space";
>>> +       res_mem.flags = IORESOURCE_MEM;
>>> +       pci_add_resource(&res, &res_mem);
>>> +
>>> +       pcie->resources = &res;
>>> +
>>> +       pcie->map_irq = iproc_pcie_bcma_map_irq;
>>> +
>>> +       ret = iproc_pcie_setup(pcie);
>>
>> I think I don't like this part of iproc design. It lefts
>> pcie->resources pointing to some random memory after the setup/probe
>> are done. Guess it should be a separated parameter or sth.
>>
>> The patch is still OK, I just refer to generic iproc possible issue.
>>
> Sorry Rafal, but could you please be more specific on this?

iproc_pcie_pltfm_probe (and iproc_pcie_bcma_probe) have local "res"
variable (each its own). They do:
pcie->resources = &res;
and then they call
iproc_pcie_setup(pcie);

After iproc_pcie_pltfm_probe / iproc_pcie_bcma_probe returns, the
pointer pcie->resources is not valid anymore, yet pcie struct is still
in use. Of course pcie->resources isn't used anymore, but still, it's
some in-struct pointer (to the random memory since local variable is
not accessible anymore).

I think you should drop
struct list_head *resources;
from the struct iproc_pcie and use
iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *resources)

-- 
Rafa?

[PATCH 2/2] PCI: iproc: add bcma pcie driver

[Ray Jui]

Hi Rafal,

On 5/13/2015 9:19 AM, Rafa? Mi?ecki wrote:
> On 13 May 2015 at 17:56, Ray Jui <rjui@broadcom.com> wrote:
>> On 5/12/2015 11:27 PM, Rafa? Mi?ecki wrote:
>>> On 12 May 2015 at 23:23, Hauke Mehrtens <hauke@hauke-m.de> wrote:
>>>> This driver adds support for the PCIe 2.0 controller found on the bcma
>>>> bus. This controller can be found on (mostly) all Broadcom BCM470X /
>>>> BCM5301X ARM SoCs.
>>>>
>>>> The driver found in the Broadcom SDK does some more stuff, like setting
>>>> up some DMA memory areas, chaining MPS and MRRS to 512 and also some
>>>> PHY changes like "improving" the PCIe jitter and doing some special
>>>> initializations for the 3rd PCIe port.
>>>>
>>>> This was tested on a bcm4708 board with 2 PCIe ports and wireless cards
>>>> connected to them.
>>>>
>>>> PCI_DOMAINS is needed by this driver, because normally there is more
>>>> than one PCIe controller and without PCI_DOMAINS only the first
>>>> controller gets registered.
>>>> This controller gets 6 IRQs, the last one is trigged by all IRQ events.
>>>>
>>>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>>>
>>> Acked-by: Rafa? Mi?ecki <zajec5@gmail.com>
>>>
>>>
>>>> +static int iproc_pcie_bcma_probe(struct bcma_device *bdev)
>>>> +{
>>>> +       struct iproc_pcie *pcie;
>>>> +       LIST_HEAD(res);
>>>> +       struct resource res_mem;
>>>> +       int ret;
>>>> +
>>>> +       pcie = devm_kzalloc(&bdev->dev, sizeof(*pcie), GFP_KERNEL);
>>>> +       if (!pcie)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       pcie->dev = &bdev->dev;
>>>> +       bcma_set_drvdata(bdev, pcie);
>>>> +
>>>> +       pcie->base = bdev->io_addr;
>>>> +
>>>> +       res_mem.start = bdev->addr_s[0];
>>>> +       res_mem.end = bdev->addr_s[0] + SZ_128M - 1;
>>>> +       res_mem.name = "PCIe MEM space";
>>>> +       res_mem.flags = IORESOURCE_MEM;
>>>> +       pci_add_resource(&res, &res_mem);
>>>> +
>>>> +       pcie->resources = &res;
>>>> +
>>>> +       pcie->map_irq = iproc_pcie_bcma_map_irq;
>>>> +
>>>> +       ret = iproc_pcie_setup(pcie);
>>>
>>> I think I don't like this part of iproc design. It lefts
>>> pcie->resources pointing to some random memory after the setup/probe
>>> are done. Guess it should be a separated parameter or sth.
>>>
>>> The patch is still OK, I just refer to generic iproc possible issue.
>>>
>> Sorry Rafal, but could you please be more specific on this?
> 
> iproc_pcie_pltfm_probe (and iproc_pcie_bcma_probe) have local "res"
> variable (each its own). They do:
> pcie->resources = &res;
> and then they call
> iproc_pcie_setup(pcie);
> 
> After iproc_pcie_pltfm_probe / iproc_pcie_bcma_probe returns, the
> pointer pcie->resources is not valid anymore, yet pcie struct is still
> in use. Of course pcie->resources isn't used anymore, but still, it's
> some in-struct pointer (to the random memory since local variable is
> not accessible anymore).
> 
> I think you should drop
> struct list_head *resources;
> from the struct iproc_pcie and use
> iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *resources)
> 

Okay thanks. That makes sense.

Or I should keep a copy of the resources under pcie->resources. In the
current pcie-iproc.c, the resource is not used anywhere else except when
creating the root bus under iproc_pcie_setup. But in the future, I might
need to add more code to explicitly program the outbound/inbound windows
so this driver can work with some other iProc SoCs where the desired
windows do not match power-on-reset values.

I plan to change this along with the window programming patch in the
future. I also think Hauke's current patch is fine.

Thanks,

Ray

[PATCH 2/2] PCI: iproc: add bcma pcie driver

[Hauke Mehrtens]

On 05/13/2015 06:30 PM, Ray Jui wrote:
> Hi Rafal,
> 
> On 5/13/2015 9:19 AM, Rafa? Mi?ecki wrote:
>> On 13 May 2015 at 17:56, Ray Jui <rjui@broadcom.com> wrote:
>>> On 5/12/2015 11:27 PM, Rafa? Mi?ecki wrote:
>>>> On 12 May 2015 at 23:23, Hauke Mehrtens <hauke@hauke-m.de> wrote:
>>>>> This driver adds support for the PCIe 2.0 controller found on the bcma
>>>>> bus. This controller can be found on (mostly) all Broadcom BCM470X /
>>>>> BCM5301X ARM SoCs.
>>>>>
>>>>> The driver found in the Broadcom SDK does some more stuff, like setting
>>>>> up some DMA memory areas, chaining MPS and MRRS to 512 and also some
>>>>> PHY changes like "improving" the PCIe jitter and doing some special
>>>>> initializations for the 3rd PCIe port.
>>>>>
>>>>> This was tested on a bcm4708 board with 2 PCIe ports and wireless cards
>>>>> connected to them.
>>>>>
>>>>> PCI_DOMAINS is needed by this driver, because normally there is more
>>>>> than one PCIe controller and without PCI_DOMAINS only the first
>>>>> controller gets registered.
>>>>> This controller gets 6 IRQs, the last one is trigged by all IRQ events.
>>>>>
>>>>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>>>>
>>>> Acked-by: Rafa? Mi?ecki <zajec5@gmail.com>
>>>>
>>>>
>>>>> +static int iproc_pcie_bcma_probe(struct bcma_device *bdev)
>>>>> +{
>>>>> +       struct iproc_pcie *pcie;
>>>>> +       LIST_HEAD(res);
>>>>> +       struct resource res_mem;
>>>>> +       int ret;
>>>>> +
>>>>> +       pcie = devm_kzalloc(&bdev->dev, sizeof(*pcie), GFP_KERNEL);
>>>>> +       if (!pcie)
>>>>> +               return -ENOMEM;
>>>>> +
>>>>> +       pcie->dev = &bdev->dev;
>>>>> +       bcma_set_drvdata(bdev, pcie);
>>>>> +
>>>>> +       pcie->base = bdev->io_addr;
>>>>> +
>>>>> +       res_mem.start = bdev->addr_s[0];
>>>>> +       res_mem.end = bdev->addr_s[0] + SZ_128M - 1;
>>>>> +       res_mem.name = "PCIe MEM space";
>>>>> +       res_mem.flags = IORESOURCE_MEM;
>>>>> +       pci_add_resource(&res, &res_mem);
>>>>> +
>>>>> +       pcie->resources = &res;
>>>>> +
>>>>> +       pcie->map_irq = iproc_pcie_bcma_map_irq;
>>>>> +
>>>>> +       ret = iproc_pcie_setup(pcie);
>>>>
>>>> I think I don't like this part of iproc design. It lefts
>>>> pcie->resources pointing to some random memory after the setup/probe
>>>> are done. Guess it should be a separated parameter or sth.
>>>>
>>>> The patch is still OK, I just refer to generic iproc possible issue.
>>>>
>>> Sorry Rafal, but could you please be more specific on this?
>>
>> iproc_pcie_pltfm_probe (and iproc_pcie_bcma_probe) have local "res"
>> variable (each its own). They do:
>> pcie->resources = &res;
>> and then they call
>> iproc_pcie_setup(pcie);
>>
>> After iproc_pcie_pltfm_probe / iproc_pcie_bcma_probe returns, the
>> pointer pcie->resources is not valid anymore, yet pcie struct is still
>> in use. Of course pcie->resources isn't used anymore, but still, it's
>> some in-struct pointer (to the random memory since local variable is
>> not accessible anymore).
>>
>> I think you should drop
>> struct list_head *resources;
>> from the struct iproc_pcie and use
>> iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *resources)
>>
> 
> Okay thanks. That makes sense.
> 
> Or I should keep a copy of the resources under pcie->resources. In the
> current pcie-iproc.c, the resource is not used anywhere else except when
> creating the root bus under iproc_pcie_setup. But in the future, I might
> need to add more code to explicitly program the outbound/inbound windows
> so this driver can work with some other iProc SoCs where the desired
> windows do not match power-on-reset values.
> 
> I plan to change this along with the window programming patch in the
> future. I also think Hauke's current patch is fine.
> 
> Thanks,
> 
> Ray
> 
I think parts of this resources are allocated and never freed.

pci_add_resource() allocates some bytes, but I do not see where they are
freed, this also applies to the platform driver.

Hauke

[PATCH 2/2] PCI: iproc: add bcma pcie driver

[Bjorn Helgaas]

On Wed, May 13, 2015 at 08:43:47PM +0200, Hauke Mehrtens wrote:
> On 05/13/2015 06:30 PM, Ray Jui wrote:
> > Hi Rafal,
> > 
> > On 5/13/2015 9:19 AM, Rafa? Mi?ecki wrote:
> >> On 13 May 2015 at 17:56, Ray Jui <rjui@broadcom.com> wrote:
> >>> On 5/12/2015 11:27 PM, Rafa? Mi?ecki wrote:
> >>>> On 12 May 2015 at 23:23, Hauke Mehrtens <hauke@hauke-m.de> wrote:
> >>>>> This driver adds support for the PCIe 2.0 controller found on the bcma
> >>>>> bus. This controller can be found on (mostly) all Broadcom BCM470X /
> >>>>> BCM5301X ARM SoCs.
> >>>>>
> >>>>> The driver found in the Broadcom SDK does some more stuff, like setting
> >>>>> up some DMA memory areas, chaining MPS and MRRS to 512 and also some
> >>>>> PHY changes like "improving" the PCIe jitter and doing some special
> >>>>> initializations for the 3rd PCIe port.
> >>>>>
> >>>>> This was tested on a bcm4708 board with 2 PCIe ports and wireless cards
> >>>>> connected to them.
> >>>>>
> >>>>> PCI_DOMAINS is needed by this driver, because normally there is more
> >>>>> than one PCIe controller and without PCI_DOMAINS only the first
> >>>>> controller gets registered.
> >>>>> This controller gets 6 IRQs, the last one is trigged by all IRQ events.
> >>>>>
> >>>>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> >>>>
> >>>> Acked-by: Rafa? Mi?ecki <zajec5@gmail.com>
> >>>>
> >>>>
> >>>>> +static int iproc_pcie_bcma_probe(struct bcma_device *bdev)
> >>>>> +{
> >>>>> +       struct iproc_pcie *pcie;
> >>>>> +       LIST_HEAD(res);
> >>>>> +       struct resource res_mem;
> >>>>> +       int ret;
> >>>>> +
> >>>>> +       pcie = devm_kzalloc(&bdev->dev, sizeof(*pcie), GFP_KERNEL);
> >>>>> +       if (!pcie)
> >>>>> +               return -ENOMEM;
> >>>>> +
> >>>>> +       pcie->dev = &bdev->dev;
> >>>>> +       bcma_set_drvdata(bdev, pcie);
> >>>>> +
> >>>>> +       pcie->base = bdev->io_addr;
> >>>>> +
> >>>>> +       res_mem.start = bdev->addr_s[0];
> >>>>> +       res_mem.end = bdev->addr_s[0] + SZ_128M - 1;
> >>>>> +       res_mem.name = "PCIe MEM space";
> >>>>> +       res_mem.flags = IORESOURCE_MEM;
> >>>>> +       pci_add_resource(&res, &res_mem);
> >>>>> +
> >>>>> +       pcie->resources = &res;
> >>>>> +
> >>>>> +       pcie->map_irq = iproc_pcie_bcma_map_irq;
> >>>>> +
> >>>>> +       ret = iproc_pcie_setup(pcie);
> >>>>
> >>>> I think I don't like this part of iproc design. It lefts
> >>>> pcie->resources pointing to some random memory after the setup/probe
> >>>> are done. Guess it should be a separated parameter or sth.
> >>>>
> >>>> The patch is still OK, I just refer to generic iproc possible issue.
> >>>>
> >>> Sorry Rafal, but could you please be more specific on this?
> >>
> >> iproc_pcie_pltfm_probe (and iproc_pcie_bcma_probe) have local "res"
> >> variable (each its own). They do:
> >> pcie->resources = &res;
> >> and then they call
> >> iproc_pcie_setup(pcie);
> >>
> >> After iproc_pcie_pltfm_probe / iproc_pcie_bcma_probe returns, the
> >> pointer pcie->resources is not valid anymore, yet pcie struct is still
> >> in use. Of course pcie->resources isn't used anymore, but still, it's
> >> some in-struct pointer (to the random memory since local variable is
> >> not accessible anymore).
> >>
> >> I think you should drop
> >> struct list_head *resources;
> >> from the struct iproc_pcie and use
> >> iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *resources)
> >>
> > 
> > Okay thanks. That makes sense.
> > 
> > Or I should keep a copy of the resources under pcie->resources. In the
> > current pcie-iproc.c, the resource is not used anywhere else except when
> > creating the root bus under iproc_pcie_setup. But in the future, I might
> > need to add more code to explicitly program the outbound/inbound windows
> > so this driver can work with some other iProc SoCs where the desired
> > windows do not match power-on-reset values.
> > 
> > I plan to change this along with the window programming patch in the
> > future. I also think Hauke's current patch is fine.
> > 
> > Thanks,
> > 
> > Ray
> > 
> I think parts of this resources are allocated and never freed.
> 
> pci_add_resource() allocates some bytes, but I do not see where they are
> freed, this also applies to the platform driver.

Where are we on this patch?  I see Ray's ack for [1/2], and a "I think the
current patch is fine"; is that an ack for [2/2] as well?

Bjorn

[PATCH 2/2] PCI: iproc: add bcma pcie driver

[Ray Jui]

On 5/19/2015 4:14 PM, Bjorn Helgaas wrote:
> On Wed, May 13, 2015 at 08:43:47PM +0200, Hauke Mehrtens wrote:
>> On 05/13/2015 06:30 PM, Ray Jui wrote:
>>> Hi Rafal,
>>>
>>> On 5/13/2015 9:19 AM, Rafa? Mi?ecki wrote:
>>>> On 13 May 2015 at 17:56, Ray Jui <rjui@broadcom.com> wrote:
>>>>> On 5/12/2015 11:27 PM, Rafa? Mi?ecki wrote:
>>>>>> On 12 May 2015 at 23:23, Hauke Mehrtens <hauke@hauke-m.de> wrote:
>>>>>>> This driver adds support for the PCIe 2.0 controller found on the bcma
>>>>>>> bus. This controller can be found on (mostly) all Broadcom BCM470X /
>>>>>>> BCM5301X ARM SoCs.
>>>>>>>
>>>>>>> The driver found in the Broadcom SDK does some more stuff, like setting
>>>>>>> up some DMA memory areas, chaining MPS and MRRS to 512 and also some
>>>>>>> PHY changes like "improving" the PCIe jitter and doing some special
>>>>>>> initializations for the 3rd PCIe port.
>>>>>>>
>>>>>>> This was tested on a bcm4708 board with 2 PCIe ports and wireless cards
>>>>>>> connected to them.
>>>>>>>
>>>>>>> PCI_DOMAINS is needed by this driver, because normally there is more
>>>>>>> than one PCIe controller and without PCI_DOMAINS only the first
>>>>>>> controller gets registered.
>>>>>>> This controller gets 6 IRQs, the last one is trigged by all IRQ events.
>>>>>>>
>>>>>>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>>>>>>
>>>>>> Acked-by: Rafa? Mi?ecki <zajec5@gmail.com>
>>>>>>
>>>>>>
>>>>>>> +static int iproc_pcie_bcma_probe(struct bcma_device *bdev)
>>>>>>> +{
>>>>>>> +       struct iproc_pcie *pcie;
>>>>>>> +       LIST_HEAD(res);
>>>>>>> +       struct resource res_mem;
>>>>>>> +       int ret;
>>>>>>> +
>>>>>>> +       pcie = devm_kzalloc(&bdev->dev, sizeof(*pcie), GFP_KERNEL);
>>>>>>> +       if (!pcie)
>>>>>>> +               return -ENOMEM;
>>>>>>> +
>>>>>>> +       pcie->dev = &bdev->dev;
>>>>>>> +       bcma_set_drvdata(bdev, pcie);
>>>>>>> +
>>>>>>> +       pcie->base = bdev->io_addr;
>>>>>>> +
>>>>>>> +       res_mem.start = bdev->addr_s[0];
>>>>>>> +       res_mem.end = bdev->addr_s[0] + SZ_128M - 1;
>>>>>>> +       res_mem.name = "PCIe MEM space";
>>>>>>> +       res_mem.flags = IORESOURCE_MEM;
>>>>>>> +       pci_add_resource(&res, &res_mem);
>>>>>>> +
>>>>>>> +       pcie->resources = &res;
>>>>>>> +
>>>>>>> +       pcie->map_irq = iproc_pcie_bcma_map_irq;
>>>>>>> +
>>>>>>> +       ret = iproc_pcie_setup(pcie);
>>>>>>
>>>>>> I think I don't like this part of iproc design. It lefts
>>>>>> pcie->resources pointing to some random memory after the setup/probe
>>>>>> are done. Guess it should be a separated parameter or sth.
>>>>>>
>>>>>> The patch is still OK, I just refer to generic iproc possible issue.
>>>>>>
>>>>> Sorry Rafal, but could you please be more specific on this?
>>>>
>>>> iproc_pcie_pltfm_probe (and iproc_pcie_bcma_probe) have local "res"
>>>> variable (each its own). They do:
>>>> pcie->resources = &res;
>>>> and then they call
>>>> iproc_pcie_setup(pcie);
>>>>
>>>> After iproc_pcie_pltfm_probe / iproc_pcie_bcma_probe returns, the
>>>> pointer pcie->resources is not valid anymore, yet pcie struct is still
>>>> in use. Of course pcie->resources isn't used anymore, but still, it's
>>>> some in-struct pointer (to the random memory since local variable is
>>>> not accessible anymore).
>>>>
>>>> I think you should drop
>>>> struct list_head *resources;
>>>> from the struct iproc_pcie and use
>>>> iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *resources)
>>>>
>>>
>>> Okay thanks. That makes sense.
>>>
>>> Or I should keep a copy of the resources under pcie->resources. In the
>>> current pcie-iproc.c, the resource is not used anywhere else except when
>>> creating the root bus under iproc_pcie_setup. But in the future, I might
>>> need to add more code to explicitly program the outbound/inbound windows
>>> so this driver can work with some other iProc SoCs where the desired
>>> windows do not match power-on-reset values.
>>>
>>> I plan to change this along with the window programming patch in the
>>> future. I also think Hauke's current patch is fine.
>>>
>>> Thanks,
>>>
>>> Ray
>>>
>> I think parts of this resources are allocated and never freed.
>>
>> pci_add_resource() allocates some bytes, but I do not see where they are
>> freed, this also applies to the platform driver.
> 
> Where are we on this patch?  I see Ray's ack for [1/2], and a "I think the
> current patch is fine"; is that an ack for [2/2] as well?
> 
> Bjorn
> 

Yes, IMO, both patches from Hauke to extend the Iproc PCIe to support
BCMA bus are fine.

Thanks,

Ray

[PATCH 2/2] PCI: iproc: add bcma pcie driver

[Rafał Miłecki]

On 12 May 2015 at 23:23, Hauke Mehrtens <hauke@hauke-m.de> wrote:
> This driver adds support for the PCIe 2.0 controller found on the bcma
> bus. This controller can be found on (mostly) all Broadcom BCM470X /
> BCM5301X ARM SoCs.
>
> The driver found in the Broadcom SDK does some more stuff, like setting
> up some DMA memory areas, chaining MPS and MRRS to 512 and also some
> PHY changes like "improving" the PCIe jitter and doing some special
> initializations for the 3rd PCIe port.
>
> This was tested on a bcm4708 board with 2 PCIe ports and wireless cards
> connected to them.
>
> PCI_DOMAINS is needed by this driver, because normally there is more
> than one PCIe controller and without PCI_DOMAINS only the first
> controller gets registered.
> This controller gets 6 IRQs, the last one is trigged by all IRQ events.
>
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

Acked-by: Rafa? Mi?ecki <zajec5@gmail.com>

[PATCH 0/2] PCI: iproc: add bcma PCIe 2.0 controller

[Ray Jui]

Hi Hauke,

On 5/12/2015 2:22 PM, Hauke Mehrtens wrote:
> After Ray submitted the PCIe controller for the iProc devices using 
> pure device tree this adds support for the devices using bcma to scan 
> the bus. This should replaces the driver I initially send for inclusion.
> 
> @Ray The driver found in the Broadcom SDK does some more stuff:
> * setting up some DMA memory areas
> * chaining MPS and MRRS to 512
> * PHY changes:
>  * "improving" the PCIe jitter 
>  * some special initializations for the 3rd PCIe port
> 
> For me it works without these additions, but I haven't tested for 
> performance or stability or 3rd port. Can you check if this is needed 
> at all for the devices found on the market?

Good work, Hauke!

I'll eventually get to these. It may take a while but I'll eventually
get to them.

Thanks,

Ray

[PATCH 0/2] PCI: iproc: add bcma PCIe 2.0 controller

[Bjorn Helgaas]

On Tue, May 12, 2015 at 11:22:59PM +0200, Hauke Mehrtens wrote:
> After Ray submitted the PCIe controller for the iProc devices using 
> pure device tree this adds support for the devices using bcma to scan 
> the bus. This should replaces the driver I initially send for inclusion.
> 
> @Ray The driver found in the Broadcom SDK does some more stuff:
> * setting up some DMA memory areas
> * chaining MPS and MRRS to 512
> * PHY changes:
>  * "improving" the PCIe jitter 
>  * some special initializations for the 3rd PCIe port
> 
> For me it works without these additions, but I haven't tested for 
> performance or stability or 3rd port. Can you check if this is needed 
> at all for the devices found on the market?
> 
> Hauke Mehrtens (2):
>   PCI: iproc: make of_irq_parse_and_map_pci() configurable
>   PCI: iproc: add bcma pcie driver
> 
>  drivers/pci/host/Kconfig               |  11 ++++
>  drivers/pci/host/Makefile              |   1 +
>  drivers/pci/host/pcie-iproc-bcma.c     | 112 +++++++++++++++++++++++++++++++++
>  drivers/pci/host/pcie-iproc-platform.c |   2 +
>  drivers/pci/host/pcie-iproc.c          |   2 +-
>  drivers/pci/host/pcie-iproc.h          |   1 +
>  6 files changed, 128 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pci/host/pcie-iproc-bcma.c

Applied to pci/host-iproc for v4.2 with acks from Ray and Rafa?, thanks!

I corrected some typos, the most important being MODULE_LICENSE, where I
changed "GPLv2" to "GPL v2".  See license_is_gpl_compatible() and
eed6542dd53f ("PCI: generic: Fix GPL v2 license string typo").

Bjorn