[PATHC,v1] PPC4xx: Adding PCI(E) MSI support

From: Tirumala Marri <tmarri@apm.com>

This patch adds MSI support for 440SPe, 460Ex, 460Sx and 405Ex.

Signed-off-by: Tirumala R Marri <tmarri@apm.com>
---
v1:
  * Get rid of bitmap functions.
  * Remove irq mapping as each MSI is tied to UIC.
  * Cleaning up of prints.
---
 arch/powerpc/boot/dts/canyonlands.dts |   18 +++
 arch/powerpc/boot/dts/katmai.dts      |   18 +++
 arch/powerpc/boot/dts/kilauea.dts     |   28 ++++
 arch/powerpc/boot/dts/redwood.dts     |   20 +++
 arch/powerpc/platforms/40x/Kconfig    |    2 +
 arch/powerpc/platforms/44x/Kconfig    |    6 +
 arch/powerpc/sysdev/Kconfig           |    6 +
 arch/powerpc/sysdev/Makefile          |    1 +
 arch/powerpc/sysdev/ppc4xx_msi.c      |  251 +++++++++++++++++++++++++++++++++
 9 files changed, 350 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/sysdev/ppc4xx_msi.c

On Thu, 2010-10-28 at 15:55 -0700, tmarri@apm.com wrote:
> From: Tirumala Marri <tmarri@apm.com>
> 
> This patch adds MSI support for 440SPe, 460Ex, 460Sx and 405Ex.

Hi Marri,

I don't know anything about your hardware, but I'll try and review the
other parts of your patch.

..

> diff --git a/arch/powerpc/platforms/40x/Kconfig b/arch/powerpc/platforms/40x/Kconfig
> index b721764..92aeee6 100644
> --- a/arch/powerpc/platforms/40x/Kconfig
> +++ b/arch/powerpc/platforms/40x/Kconfig
> @@ -57,6 +57,8 @@ config KILAUEA
>  	select 405EX
>  	select PPC40x_SIMPLE
>  	select PPC4xx_PCI_EXPRESS
> +	select PCI_MSI
> +	select 4xx_MSI
>  	help
>  	  This option enables support for the AMCC PPC405EX evaluation board.
>  
> diff --git a/arch/powerpc/platforms/44x/Kconfig b/arch/powerpc/platforms/44x/Kconfig
> index 0f979c5..3836353 100644
> --- a/arch/powerpc/platforms/44x/Kconfig
> +++ b/arch/powerpc/platforms/44x/Kconfig
> @@ -74,6 +74,8 @@ config KATMAI
>  	select 440SPe
>  	select PCI
>  	select PPC4xx_PCI_EXPRESS
> +	select PCI_MSI
> +	select 4xx_MSI
>  	help
>  	  This option enables support for the AMCC PPC440SPe evaluation board.
>  
> @@ -119,6 +121,8 @@ config CANYONLANDS
>  	select 460EX
>  	select PCI
>  	select PPC4xx_PCI_EXPRESS
> +	select PCI_MSI
> +	select 4xx_MSI
>  	select IBM_NEW_EMAC_RGMII
>  	select IBM_NEW_EMAC_ZMII
>  	help
> @@ -145,6 +149,8 @@ config REDWOOD
>  	select 460SX
>  	select PCI
>  	select PPC4xx_PCI_EXPRESS
> +	select PCI_MSI
> +	select 4xx_MSI
>  	help
>  	  This option enables support for the AMCC PPC460SX Redwood board.
>  
> diff --git a/arch/powerpc/sysdev/Kconfig b/arch/powerpc/sysdev/Kconfig
> index 3965828..ec86000 100644
> --- a/arch/powerpc/sysdev/Kconfig
> +++ b/arch/powerpc/sysdev/Kconfig
> @@ -7,6 +7,12 @@ config PPC4xx_PCI_EXPRESS
>  	depends on PCI && 4xx
>  	default n
>  
> +config 4xx_MSI
> +	bool
> +	depends on PCI_MSI
> +	depends on PCI && 4xx

This can just be: depends on PCI_MSI && 4xx

Because PCI_MSI depends on PCI.

You only ever enable this via select, which ignores dependencies, but
it's probably still good to have the depends as documentation.

> diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
> index 0bef9da..30f4da4 100644
> --- a/arch/powerpc/sysdev/Makefile
> +++ b/arch/powerpc/sysdev/Makefile
> @@ -40,6 +40,7 @@ obj-$(CONFIG_XILINX_PCI)	+= xilinx_pci.o
>  obj-$(CONFIG_OF_RTC)		+= of_rtc.o
>  ifeq ($(CONFIG_PCI),y)
>  obj-$(CONFIG_4xx)		+= ppc4xx_pci.o
> +obj-$(CONFIG_4xx_MSI)		+= ppc4xx_msi.o

This needn't be inside the if PCI block, that's the whole point of
CONFIG_4xx_MSI. And you should send another patch to add a
CONFIG_4xx_PCI symbol and use that to build ppc4xx_pci.o in the same
way.

> diff --git a/arch/powerpc/sysdev/ppc4xx_msi.c b/arch/powerpc/sysdev/ppc4xx_msi.c
> new file mode 100644
> index 0000000..9d5e0c9
> --- /dev/null
> +++ b/arch/powerpc/sysdev/ppc4xx_msi.c
> @@ -0,0 +1,251 @@
> +/*
> + * Adding PCI-E MSI support for PPC4XX SoCs.
> + *
> + * Copyright (c) 2010, Applied Micro Circuits Corporation
> + * Authors: 	Tirumala R Marri <tmarri@apm.com>
> + * 		Feng Kan <fkan@apm.com>
> + *
> + * 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; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */

Blank line usually here.

> +#include <linux/irq.h>
> +#include <linux/bootmem.h>
> +#include <linux/pci.h>
> +#include <linux/msi.h>
> +#include <linux/of_platform.h>
> +#include <linux/interrupt.h>
> +#include <asm/prom.h>
> +#include <asm/hw_irq.h>
> +#include <asm/ppc-pci.h>
> +#include <boot/dcr.h>

Using headers in boot is "interesting".

> +#include <asm/msi_bitmap.h>

You don't use this, but maybe you should.

> +#define PEIH_TERMADH    0x00
> +#define PEIH_TERMADL    0x08
> +#define PEIH_MSIED      0x10
> +#define PEIH_MSIMK      0x18
> +#define PEIH_MSIASS     0x20
> +#define PEIH_FLUSH0     0x30
> +#define PEIH_FLUSH1     0x38
> +#define PEIH_CNTRST     0x48
> +
> +#define  UPPER_4BITS_OF36BIT 32
> +#define  LOWER_32BITS_OF36BIT 0xFFFFFFFF

These are badly named IMHO, it's not really clear that the first is a
shift and the second is a mask. I'd just get rid of them, everyone will
know what the code is doing.

> +struct ppc4xx_msi {
> +	u32 msi_addr_lo;
> +	u32 msi_addr_hi;
> +	void __iomem *msi_regs;
> +	u32 feature;
             ^
Unused AFAICS.

> +};
> +
> +static struct ppc4xx_msi *ppc4xx_msi;
> +struct ppc4xx_msi_feature {
> +	u32 ppc4xx_pic_ip;
> +	u32 msiir_offset;
> +};
> +
> +static int ppc4xx_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> +{
> +	struct msi_desc *entry;
> +	unsigned int virq = 0;

Don't need = 0.

> +	struct msi_msg msg;
> +	struct ppc4xx_msi *msi_data = ppc4xx_msi;

Just use the global.

> +	static int int_no = 0;	/* To remember last used interrupt */

This is a worry. There is nothing AFAIK to stop two drivers (eg. network
& scsi) calling into here at the same time, which could lead to
corrupting int_no. If you just want a global counter you need a lock.

But, AFAICS this is broken anyway. You never free the interrupt numbers,
so you're going to run out. Some of your device tree entries only have 3
(!!), so ifup/ifdown x 3 will exhaust the supply, won't it?

I realise I never replied to your mail the other week about the bitmap
code, so perhaps it's my fault :)

> +	struct device_node *msi_dev = NULL;
> +	const u32 *count;
> +
> +	msi_dev = of_find_node_by_name(NULL, "ppc4xx-msi");
> +	if (msi_dev)
> +		return -ENODEV;
> +
>+
> +	count = of_get_property(msi_dev, "interrupt-count", NULL);
> +	if (!count) {
> +		dev_err(&dev->dev, "no interrupts property found \n");
> +		return -ENODEV;
> +	}

You should grab this at probe time and store it in ppc4xx_msi to save
yourself parsing it over and over.

> +	if (int_no > *count)
> +		return -EINVAL;
> +
> +	list_for_each_entry(entry, &dev->msi_list, list) {
> +		virq = irq_of_parse_and_map(msi_dev, int_no);
> +		printk("virq = 0x%x\n", virq);

Either turn into a pr_debug() or remove.

> +		if (virq == NO_IRQ) {
> +			dev_err(&dev->dev, "%s: fail mapping irq\n", __func__);
> +			return -EINVAL;
> +		} else
> +			set_irq_data(virq, (void *)int_no);

Needn't be in the else block given the code flow. And why do you do
this?

> +		dev_dbg(&dev->dev, "%s: virq = %d \n", __func__, virq);
> +
> +		/* Setup msi address space */
> +		msg.address_hi = msi_data->msi_addr_hi;
> +		msg.address_lo = msi_data->msi_addr_lo;
> +
> +		set_irq_msi(virq, entry);
> +		msg.data = int_no;
> +		int_no++;
> +		write_msi_msg(virq, &msg);
> +	}
> +
> +	return 0;
> +}
> +
> +void ppc4xx_teardown_msi_irqs(struct pci_dev *dev)
> +{
> +	struct msi_desc *entry;
> +
> +	dev_dbg(&dev->dev, "PCIE-MSI: tearing down msi irqs\n");
> +
> +	list_for_each_entry(entry, &dev->msi_list, list) {
> +		if (entry->irq == NO_IRQ)
> +			continue;
> +		set_irq_msi(entry->irq, NULL);
> +		irq_dispose_mapping(entry->irq);

No free of int_no?!

> +	}
> +
> +	return;
> +}
> +
> +static int ppc4xx_msi_check_device(struct pci_dev *pdev, int nvec, int type)
> +{
> +	dev_dbg(&pdev->dev, "PCIE-MSI:%s called. vec %x type %d\n",
> +		__func__, nvec, type);

No constraints at all? What about MSI-X ?

> +	return 0;
> +}
> +
> +static int ppc4xx_setup_pcieh_hw(struct platform_device *dev,
> +				 struct resource res, struct ppc4xx_msi *msi)
> +{
> +	const u32 *msi_data;
> +	const u32 *msi_mask;
> +	const u32 *sdr_addr;
> +	int rc = 0;
> +	dma_addr_t msi_phys;
> +	void *msi_virt;
> +
> +	sdr_addr = of_get_property(dev->dev.of_node, "sdr-base", NULL);
> +	if (!sdr_addr)
> +		return -1;
> +
> +	SDR0_WRITE(sdr_addr, (u64)res.start >> UPPER_4BITS_OF36BIT);	/*HIGH addr */
> +	SDR0_WRITE(sdr_addr + 1, res.start & LOWER_32BITS_OF36BIT);	/* Low addr */
> +
> +	msi->msi_regs = ioremap((u64) res.start, res.end - res.start + 1);

You should be able to just use of_iomap() here.

> +	if (!msi->msi_regs) {
> +		dev_err(&dev->dev, "ioremap problem failed\n");
> +		return ENOMEM;
> +	}
> +	dev_dbg(&dev->dev, "PCIE-MSI: msi register mapped 0x%x 0x%x\n",
> +		(u32) (msi->msi_regs + PEIH_TERMADH), (u32) (msi->msi_regs));
> +
> +	msi_virt = dma_alloc_coherent(&dev->dev, 64, &msi_phys, GFP_KERNEL);
> +	msi->msi_addr_hi = 0x0;
> +	msi->msi_addr_lo = (u32) msi_phys;
> +	dev_dbg(&dev->dev, "PCIE-MSI: msi address 0x%x \n", msi->msi_addr_lo);

So your MSI is a write to just some arbitrary address?

> +	/* Progam the Interrupt handler Termination addr registers */
> +	out_be32(msi->msi_regs + PEIH_TERMADH, msi->msi_addr_hi);
> +	out_be32(msi->msi_regs + PEIH_TERMADL, msi->msi_addr_lo);

And the hardware detects it by catching writes to that address? But the
write still lands?

> +	msi_data = of_get_property(dev->dev.of_node, "msi-data", NULL);
> +	if (!msi_data) {
> +		rc = -1;
> +		goto error_out;
> +	}

Never used?

> +	msi_mask = of_get_property(dev->dev.of_node, "msi-mask", NULL);
> +	if (!msi_mask) {
> +		rc = -1;
> +		goto error_out;
> +	}
> +
> +	/* Program MSI Expected data and Mask bits */
> +	out_be32(msi->msi_regs + PEIH_MSIED, 0);
> +	out_be32(msi->msi_regs + PEIH_MSIMK, *msi_mask);
> +
> +	return 0;
> +      error_out:

Goto label should start in column 0.

> +	iounmap(msi->msi_regs);

You don't cleanup/undo any of the rest though? Ideally you'd structure
the routine so that you don't touch the hardware until you know you
can't fail.

> +	return rc;
> +}
> +static int __devinit ppc4xx_msi_probe(struct platform_device *dev,
> +				      const struct of_device_id *match)
> +{
> +	struct ppc4xx_msi *msi;
> +	struct resource res;
> +	int err = 0;
> +
> +	dev_dbg(&dev->dev, "PCIE-MSI: Setting up MSI support...\n");
> +
> +	msi = kzalloc(sizeof(struct ppc4xx_msi), GFP_KERNEL);
> +	if (!msi) {
> +		dev_err(&dev->dev, "No memory for MSI structure\n");
> +		err = -ENOMEM;
> +		goto error_out;
> +	}
> +	/* Get MSI ranges */
> +	err = of_address_to_resource(dev->dev.of_node, 0, &res);
> +	if (err) {
> +		dev_err(&dev->dev, "%s resource error!\n",
> +			dev->dev.of_node->full_name);
> +		goto error_out;
> +	}

Just use of_iomap().

> +	if (ppc4xx_setup_pcieh_hw(dev, res, msi))
> +		goto error_out;
> +
> +	ppc4xx_msi = msi;

You know there's only ever one in a system?

> +	WARN_ON(ppc_md.setup_msi_irqs);
> +	ppc_md.setup_msi_irqs = ppc4xx_setup_msi_irqs;
> +	ppc_md.teardown_msi_irqs = ppc4xx_teardown_msi_irqs;
> +	ppc_md.msi_check_device = ppc4xx_msi_check_device;

Don't bother setting check if yours does nothing.

> +	return 0;
> +
> +      error_out:
> +	kfree(msi);
> +	return err;
> +}

Blank line.

> +static const struct ppc4xx_msi_feature ppc4xx_msi_feature = {
> +	.ppc4xx_pic_ip = 0,
> +	.msiir_offset = 0x140,
> +};

This looks to be unused?

> +static const struct of_device_id ppc4xx_msi_ids[] = {
> +	{
> +	 .compatible = "amcc,ppc4xx-msi",
> +	 .data = (void *)&ppc4xx_msi_feature,

I know it's "used" here, but I don't see where you use that.

> +	 },
> +	{}
> +};
> +
> +static struct of_platform_driver ppc4xx_msi_driver = {
> +	.driver = {
> +		   .name = "ppc4xx-msi",
> +		   .owner = THIS_MODULE,
> +		   .of_match_table = ppc4xx_msi_ids,
> +		   },
> +	.probe = ppc4xx_msi_probe,
> +};
> +
> +static __init int ppc4xx_msi_init(void)
> +{
> +	return of_register_platform_driver(&ppc4xx_msi_driver);
> +}
> +
> +subsys_initcall(ppc4xx_msi_init);

On Wed, Nov 03, 2010 at 11:11:33PM +1100, Michael Ellerman wrote:
>> +	struct device_node *msi_dev = NULL;
>> +	const u32 *count;
>> +
>> +	msi_dev = of_find_node_by_name(NULL, "ppc4xx-msi");
>> +	if (msi_dev)
>> +		return -ENODEV;
>> +

You also leak a reference to the node here, as there is never a call to
of_node_put.

josh

Appreciate your review.
 > +     static int int_no = 0;  /* To remember last used interrupt */

>
> This is a worry. There is nothing AFAIK to stop two drivers (eg. network
> & scsi) calling into here at the same time, which could lead to
> corrupting int_no. If you just want a global counter you need a lock.
>
> But, AFAICS this is broken anyway. You never free the interrupt numbers,
> so you're going to run out. Some of your device tree entries only have 3
> (!!), so ifup/ifdown x 3 will exhaust the supply, won't it?
>
> I realise I never replied to your mail the other week about the bitmap
> code, so perhaps it's my fault :)
>
> [Marri] Good point.I will come up with a global array to see if the
interrupt nos are free
and use them.

 > +     return;

> > +}
> > +
> > +static int ppc4xx_msi_check_device(struct pci_dev *pdev, int nvec, int
> type)
> > +{
> > +     dev_dbg(&pdev->dev, "PCIE-MSI:%s called. vec %x type %d\n",
> > +             __func__, nvec, type);
>
> No constraints at all? What about MSI-X ?
> [marri] That will be another patch as it is only specific to some SoCs
>


> > +     msi_virt = dma_alloc_coherent(&dev->dev, 64, &msi_phys,
> GFP_KERNEL);
> > +     msi->msi_addr_hi = 0x0;
> > +     msi->msi_addr_lo = (u32) msi_phys;
> > +     dev_dbg(&dev->dev, "PCIE-MSI: msi address 0x%x \n",
> msi->msi_addr_lo);
>
> So your MSI is a write to just some arbitrary address?
>
> [Marri] We choose a arbitrary address and pass that to msi APIs which would
write
to endpoint device config space. If endpoint wants to cause an MSI it just
generates
memory transaction to this address, as soon as it is on system bus PCI-E
handler
will snoop this address and cause interrupt to CPU.




> > +     /* Progam the Interrupt handler Termination addr registers */
> > +     out_be32(msi->msi_regs + PEIH_TERMADH, msi->msi_addr_hi);
> > +     out_be32(msi->msi_regs + PEIH_TERMADL, msi->msi_addr_lo);
>
> And the hardware detects it by catching writes to that address? But the
> write still lands?
>
> [marri] write still happens to memory but we do't need need to wait for it
to complete.
So the termination would avoid un-necessary delay in acking the write or
race condition
to that particular address.


>
>