PCI: iproc: add bcma PCIe 2.0 controller

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 <[email protected]>

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

drivers/pci/host/pcie-iproc-platform.c +2 -0

index afad6c2..c8aa06f 100644
@@ -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");

drivers/pci/host/pcie-iproc.c +1 -1

index 329e1b5..cef31f6 100644
@@ -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;

drivers/pci/host/pcie-iproc.h +0 -0

index e28075e..a333d4b 100644

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 <[email protected]>

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

drivers/pci/host/Kconfig +11 -0

index 1dfb567..f23f96d 100644
@@ -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

drivers/pci/host/Makefile +1 -0

index f733b4e..2f7c36f 100644
@@ -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

drivers/pci/host/pcie-iproc-bcma.c new file +102 -0

new file mode 100644
index 0000000..c318f19
@@ -0,0 +1,112 @@
+/* 
+ * Copyright (C) 2015 Broadcom Corporation
+ * Copyright (C) 2015 Hauke Mehrtens <[email protected]>
+ *
+ * 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);

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 <[email protected]>

drivers/pci/host/pcie-iproc-bcma.c new file +17 -0

+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];

Quoted 12 lines · 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.

Quoted 6 lines

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 <[email protected]>

Acked-by Rafa? Mi?ecki <[email protected]>

Quoted 17 lines

drivers/pci/host/pcie-iproc-bcma.c new file +17 -0

+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];

drivers/pci/host/pcie-iproc-bcma.c line 69

Quoted 10 lines

drivers/pci/host/pcie-iproc-bcma.c new file +10 -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.

Quoted 55 lines · Rafa? Mi?ecki

Quoted 12 lines · 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.

Quoted 6 lines

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 <[email protected]>

Acked-by Rafa? Mi?ecki <[email protected]>

Quoted 17 lines

drivers/pci/host/pcie-iproc-bcma.c new file +17 -0

+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];

drivers/pci/host/pcie-iproc-bcma.c line 69

Quoted 10 lines

drivers/pci/host/pcie-iproc-bcma.c new file +10 -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.

Quoted 2 lines

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

Sorry Rafal, but could you please be more specific on this?

Quoted 56 lines · Ray Jui

Quoted 55 lines · Rafa? Mi?ecki

Quoted 12 lines · 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.

Quoted 6 lines

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 <[email protected]>

Acked-by Rafa? Mi?ecki <[email protected]>

Quoted 17 lines

drivers/pci/host/pcie-iproc-bcma.c new file +17 -0

+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];

drivers/pci/host/pcie-iproc-bcma.c line 69

Quoted 10 lines

drivers/pci/host/pcie-iproc-bcma.c new file +10 -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.

Quoted 3 lines

Quoted 2 lines

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)

Hi Rafal,

Quoted 73 lines · Rafa? Mi?ecki

Quoted 56 lines · Ray Jui

Quoted 55 lines · Rafa? Mi?ecki

Quoted 12 lines · 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.

Quoted 6 lines

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 <[email protected]>

Acked-by Rafa? Mi?ecki <[email protected]>

Quoted 17 lines

drivers/pci/host/pcie-iproc-bcma.c new file +17 -0

+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];

drivers/pci/host/pcie-iproc-bcma.c line 69

Quoted 10 lines

drivers/pci/host/pcie-iproc-bcma.c new file +10 -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.

Quoted 3 lines

Quoted 2 lines

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

Quoted 5 lines

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

Quoted 94 lines · Ray Jui

Hi Rafal,

Quoted 73 lines · Rafa? Mi?ecki

Quoted 56 lines · Ray Jui

Quoted 55 lines · Rafa? Mi?ecki

Quoted 12 lines · 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.

Quoted 6 lines

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 <[email protected]>

Acked-by Rafa? Mi?ecki <[email protected]>

Quoted 17 lines

drivers/pci/host/pcie-iproc-bcma.c new file +17 -0

+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];

drivers/pci/host/pcie-iproc-bcma.c line 69

Quoted 10 lines

drivers/pci/host/pcie-iproc-bcma.c new file +10 -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.

Quoted 3 lines

Quoted 2 lines

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

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 101 lines · Hauke Mehrtens

Quoted 91 lines · Ray Jui

Hi Rafal,

Quoted 73 lines · Rafa? Mi?ecki

Quoted 56 lines · Ray Jui

Quoted 55 lines · Rafa? Mi?ecki

Quoted 12 lines · 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.

Quoted 6 lines

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 <[email protected]>

Acked-by Rafa? Mi?ecki <[email protected]>

Quoted 17 lines

(quoted diff) +17 -0

+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];

Review

Quoted 10 lines

(quoted diff) +10 -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.

Quoted 3 lines

Quoted 2 lines

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

Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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

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 <[email protected]>

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?

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