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

From: Michael Ellerman <[email-masked]>
Date: Wed, 03 Nov 2010 23:11:33 +1100

On Thu, 2010-10-28 at 15:55 -0700, [email-masked] wrote:
> From: Tirumala Marri <[email-masked]>
> 
> 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 <[email-masked]>
> + * 		Feng Kan <[email-masked]>
> + *
> + * 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);