diff mbox

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

Message ID 1431465781-10753-3-git-send-email-hauke@hauke-m.de (mailing list archive)
State New, archived
Headers show

Commit Message

Hauke Mehrtens May 12, 2015, 9:23 p.m. UTC
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

Comments

Ray Jui May 12, 2015, 10:18 p.m. UTC | #1
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
Rafał Miłecki May 13, 2015, 6:27 a.m. UTC | #2
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.
Ray Jui May 13, 2015, 3:56 p.m. UTC | #3
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
Rafał Miłecki May 13, 2015, 4:19 p.m. UTC | #4
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)
Ray Jui May 13, 2015, 4:30 p.m. UTC | #5
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
Hauke Mehrtens May 13, 2015, 6:43 p.m. UTC | #6
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
Bjorn Helgaas May 19, 2015, 11:14 p.m. UTC | #7
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
Ray Jui May 19, 2015, 11:17 p.m. UTC | #8
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
Rafał Miłecki May 20, 2015, 5:27 a.m. UTC | #9
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>
diff mbox

Patch

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