| Message ID | 1431465781-10753-3-git-send-email-hauke@hauke-m.de (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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> -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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