* [PATCH 2/2] PCI: iproc: add bcma pcie driver
@ 2015-05-12 21:23 ` Hauke Mehrtens
0 siblings, 0 replies; 30+ messages in thread
From: Hauke Mehrtens @ 2015-05-12 21:23 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 2/2] PCI: iproc: add bcma pcie driver
2015-05-12 21:23 ` Hauke Mehrtens
@ 2015-05-12 22:18 ` Ray Jui
-1 siblings, 0 replies; 30+ messages in thread
From: Ray Jui @ 2015-05-12 22:18 UTC (permalink / raw)
To: Hauke Mehrtens, bhelgaas, arnd
Cc: linux-pci, sbranden, linux-arm-kernel, bcm-kernel-feedback-list, zajec5
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
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 2/2] PCI: iproc: add bcma pcie driver
@ 2015-05-12 22:18 ` Ray Jui
0 siblings, 0 replies; 30+ messages in thread
From: Ray Jui @ 2015-05-12 22:18 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] PCI: iproc: add bcma pcie driver
2015-05-12 21:23 ` Hauke Mehrtens
@ 2015-05-13 6:27 ` Rafał Miłecki
-1 siblings, 0 replies; 30+ messages in thread
From: Rafał Miłecki @ 2015-05-13 6:27 UTC (permalink / raw)
To: Hauke Mehrtens
Cc: Ray Jui, Bjorn Helgaas, Arnd Bergmann, Linux PCI, Scott Branden,
linux-arm-kernel, bcm-kernel-feedback-list
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.
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 2/2] PCI: iproc: add bcma pcie driver
@ 2015-05-13 6:27 ` Rafał Miłecki
0 siblings, 0 replies; 30+ messages in thread
From: Rafał Miłecki @ 2015-05-13 6:27 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 2/2] PCI: iproc: add bcma pcie driver
2015-05-13 6:27 ` Rafał Miłecki
@ 2015-05-13 15:56 ` Ray Jui
-1 siblings, 0 replies; 30+ messages in thread
From: Ray Jui @ 2015-05-13 15:56 UTC (permalink / raw)
To: Rafał Miłecki, Hauke Mehrtens
Cc: Bjorn Helgaas, Arnd Bergmann, Linux PCI, Scott Branden,
linux-arm-kernel, bcm-kernel-feedback-list
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
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 2/2] PCI: iproc: add bcma pcie driver
@ 2015-05-13 15:56 ` Ray Jui
0 siblings, 0 replies; 30+ messages in thread
From: Ray Jui @ 2015-05-13 15:56 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 2/2] PCI: iproc: add bcma pcie driver
2015-05-13 15:56 ` Ray Jui
@ 2015-05-13 16:19 ` Rafał Miłecki
-1 siblings, 0 replies; 30+ messages in thread
From: Rafał Miłecki @ 2015-05-13 16:19 UTC (permalink / raw)
To: Ray Jui
Cc: Hauke Mehrtens, Bjorn Helgaas, Arnd Bergmann, Linux PCI,
Scott Branden, linux-arm-kernel, bcm-kernel-feedback-list
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ł
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 2/2] PCI: iproc: add bcma pcie driver
@ 2015-05-13 16:19 ` Rafał Miłecki
0 siblings, 0 replies; 30+ messages in thread
From: Rafał Miłecki @ 2015-05-13 16:19 UTC (permalink / raw)
To: linux-arm-kernel
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?
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 2/2] PCI: iproc: add bcma pcie driver
2015-05-13 16:19 ` Rafał Miłecki
@ 2015-05-13 16:30 ` Ray Jui
-1 siblings, 0 replies; 30+ messages in thread
From: Ray Jui @ 2015-05-13 16:30 UTC (permalink / raw)
To: Rafał Miłecki
Cc: Hauke Mehrtens, Bjorn Helgaas, Arnd Bergmann, Linux PCI,
Scott Branden, linux-arm-kernel, bcm-kernel-feedback-list
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
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 2/2] PCI: iproc: add bcma pcie driver
@ 2015-05-13 16:30 ` Ray Jui
0 siblings, 0 replies; 30+ messages in thread
From: Ray Jui @ 2015-05-13 16:30 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 2/2] PCI: iproc: add bcma pcie driver
2015-05-13 16:30 ` Ray Jui
@ 2015-05-13 18:43 ` Hauke Mehrtens
-1 siblings, 0 replies; 30+ messages in thread
From: Hauke Mehrtens @ 2015-05-13 18:43 UTC (permalink / raw)
To: Ray Jui, Rafał Miłecki
Cc: Bjorn Helgaas, Arnd Bergmann, Linux PCI, Scott Branden,
linux-arm-kernel, bcm-kernel-feedback-list
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
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 2/2] PCI: iproc: add bcma pcie driver
@ 2015-05-13 18:43 ` Hauke Mehrtens
0 siblings, 0 replies; 30+ messages in thread
From: Hauke Mehrtens @ 2015-05-13 18:43 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 2/2] PCI: iproc: add bcma pcie driver
2015-05-13 18:43 ` Hauke Mehrtens
@ 2015-05-19 23:14 ` Bjorn Helgaas
-1 siblings, 0 replies; 30+ messages in thread
From: Bjorn Helgaas @ 2015-05-19 23:14 UTC (permalink / raw)
To: Hauke Mehrtens
Cc: Ray Jui, Rafał Miłecki, Arnd Bergmann, Linux PCI,
Scott Branden, linux-arm-kernel, bcm-kernel-feedback-list
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
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 2/2] PCI: iproc: add bcma pcie driver
@ 2015-05-19 23:14 ` Bjorn Helgaas
0 siblings, 0 replies; 30+ messages in thread
From: Bjorn Helgaas @ 2015-05-19 23:14 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 2/2] PCI: iproc: add bcma pcie driver
2015-05-19 23:14 ` Bjorn Helgaas
@ 2015-05-19 23:17 ` Ray Jui
-1 siblings, 0 replies; 30+ messages in thread
From: Ray Jui @ 2015-05-19 23:17 UTC (permalink / raw)
To: Bjorn Helgaas, Hauke Mehrtens
Cc: Rafał Miłecki, Arnd Bergmann, Linux PCI, Scott Branden,
linux-arm-kernel, bcm-kernel-feedback-list
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
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 2/2] PCI: iproc: add bcma pcie driver
@ 2015-05-19 23:17 ` Ray Jui
0 siblings, 0 replies; 30+ messages in thread
From: Ray Jui @ 2015-05-19 23:17 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] PCI: iproc: add bcma pcie driver
2015-05-12 21:23 ` Hauke Mehrtens
@ 2015-05-20 5:27 ` Rafał Miłecki
-1 siblings, 0 replies; 30+ messages in thread
From: Rafał Miłecki @ 2015-05-20 5:27 UTC (permalink / raw)
To: Hauke Mehrtens
Cc: Ray Jui, Bjorn Helgaas, Arnd Bergmann, Linux PCI, Scott Branden,
linux-arm-kernel, bcm-kernel-feedback-list
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>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 2/2] PCI: iproc: add bcma pcie driver
@ 2015-05-20 5:27 ` Rafał Miłecki
0 siblings, 0 replies; 30+ messages in thread
From: Rafał Miłecki @ 2015-05-20 5:27 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 30+ messages in thread