Subject: Re: [PATCH 2/4 V6] MSI support on 83xx/85xx/86xx board

From: Anton Vorontsov <[email-masked]>
Date: Fri, 23 May 2008 16:59:29 +0400

On Fri, May 23, 2008 at 04:32:46PM +0800, Jason Jin wrote:
> This MSI driver can be used on 83xx/85xx/86xx board.
> In this driver, virtual interrupt host and chip were
> setup. There are 256 MSI interrupts in this host, Every 32
> MSI interrupts cascaded to one IPIC/MPIC interrupt.
> The chip was treated as edge sensitive and some necessary
> functions were setup for this chip.
> 
> Before using the MSI interrupt, PCI/PCIE device need to
> ask for a MSI interrupt in the 256 MSI interrupts. A 256bit
> bitmap show which MSI interrupt was used, reserve bit in
> the bitmap can be used to force the device use some designate
> MSI interrupt in the 256 MSI interrupts. Sometimes this is useful
> for testing the all the MSI interrupts. The msi-available-ranges
> property in the dts file was used for this purpose.
> 
> Signed-off-by: Jason Jin <[email-masked]>
> ---

Hello Jason,

Few comments below.

> No changes in this send, just sent with the patchset.
> 
>  arch/powerpc/sysdev/Makefile  |    3 +-
>  arch/powerpc/sysdev/fsl_msi.c |  436 +++++++++++++++++++++++++++++++++++++++++
>  arch/powerpc/sysdev/fsl_msi.h |   42 ++++
>  arch/powerpc/sysdev/fsl_pci.c |   14 ++
>  4 files changed, 494 insertions(+), 1 deletions(-)
>  create mode 100644 arch/powerpc/sysdev/fsl_msi.c
>  create mode 100644 arch/powerpc/sysdev/fsl_msi.h
> 
> diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
> index 2cc5052..dd6dff3 100644
> --- a/arch/powerpc/sysdev/Makefile
> +++ b/arch/powerpc/sysdev/Makefile
> @@ -4,6 +4,7 @@ endif
>  
>  mpic-msi-obj-$(CONFIG_PCI_MSI)	+= mpic_msi.o mpic_u3msi.o mpic_pasemi_msi.o
>  obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-obj-y)
> +fsl-msi-obj-$(CONFIG_PCI_MSI)	+= fsl_msi.o
>  
>  obj-$(CONFIG_PPC_MPC106)	+= grackle.o
>  obj-$(CONFIG_PPC_DCR_NATIVE)	+= dcr-low.o
> @@ -11,7 +12,7 @@ obj-$(CONFIG_PPC_PMI)		+= pmi.o
>  obj-$(CONFIG_U3_DART)		+= dart_iommu.o
>  obj-$(CONFIG_MMIO_NVRAM)	+= mmio_nvram.o
>  obj-$(CONFIG_FSL_SOC)		+= fsl_soc.o
> -obj-$(CONFIG_FSL_PCI)		+= fsl_pci.o
> +obj-$(CONFIG_FSL_PCI)		+= fsl_pci.o $(fsl-msi-obj-y)
>  obj-$(CONFIG_FSL_LBC)		+= fsl_lbc.o
>  obj-$(CONFIG_RAPIDIO)		+= fsl_rio.o
>  obj-$(CONFIG_TSI108_BRIDGE)	+= tsi108_pci.o tsi108_dev.o
> diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c
> new file mode 100644
> index 0000000..9d0685b
> --- /dev/null
> +++ b/arch/powerpc/sysdev/fsl_msi.c
> @@ -0,0 +1,436 @@
> +/*
> + * Copyright (C) 2007-2008 Freescale Semiconductor, Inc. All rights reserved.
> + *
> + * Author: Tony Li <[email-masked]>
> + *	   Jason Jin <[email-masked]>
> + *
> + * The hwirq alloc and free code reuse from sysdev/mpic_msi.c
> + *
> + * 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 of the
> + * License.
> + *
> + */
> +#include <linux/irq.h>
> +#include <linux/bootmem.h>
> +#include <linux/bitmap.h>
> +#include <linux/msi.h>
> +#include <linux/pci.h>
> +#include <linux/of_platform.h>
> +#include <sysdev/fsl_soc.h>
> +#include <asm/prom.h>
> +#include <asm/hw_irq.h>
> +#include <asm/ppc-pci.h>
> +#include "fsl_msi.h"
> +
> +struct fsl_msi_feature {
> +	u32 fsl_pic_ip;
> +	u32 msiir_offset;
> +};
> +
> +static struct fsl_msi *fsl_msi;
> +
> +static inline u32 fsl_msi_read(u32 __iomem *base, unsigned int reg)
> +{
> +	return in_be32(base + (reg >> 2));
> +}
> +
> +static inline void fsl_msi_write(u32 __iomem *base,
> +				unsigned int reg, u32 value)

This function isn't used.

> +{
> +	out_be32(base + (reg >> 2), value);
> +}
> +
> +/*
> + * We do not need this actually. The MSIR register has been read once
> + * in the cascade interrupt. So, this MSI interrupt has been acked
> +*/
> +static void fsl_msi_end_irq(unsigned int virq)
> +{
> +}
> +
> +static struct irq_chip fsl_msi_chip = {
> +	.mask		= mask_msi_irq,
> +	.unmask		= unmask_msi_irq,
> +	.ack		= fsl_msi_end_irq,
> +	.typename	= " FSL-MSI  ",
> +};
> +
> +static int fsl_msi_host_map(struct irq_host *h, unsigned int virq,
> +				irq_hw_number_t hw)
> +{
> +	struct irq_chip *chip = &fsl_msi_chip;
> +
> +	get_irq_desc(virq)->status |= IRQ_TYPE_EDGE_FALLING;
> +
> +	set_irq_chip_and_handler(virq, chip,  handle_edge_irq);

Two spaces.

> +
> +	return 0;
> +}
> +
> +static struct irq_host_ops fsl_msi_host_ops = {
> +	.map = fsl_msi_host_map,
> +};
> +
> +irq_hw_number_t fsl_msi_alloc_hwirqs(struct fsl_msi *msi, int num)
> +{

This isn't extern'ed function, this probably should be static.

> +	unsigned long flags;
> +	int offset, order = get_count_order(num);

int order = get_count_order(num);
int offset;

> +	spin_lock_irqsave(&msi->bitmap_lock, flags);
> +
> +	offset = bitmap_find_free_region(msi->fsl_msi_bitmap,
> +					NR_MSI_IRQS, order);
> +
> +	spin_unlock_irqrestore(&msi->bitmap_lock, flags);
> +
> +	pr_debug("%s: allocated 0x%x (2^%d) at offset 0x%x\n",
> +		__func__, num, order, offset);
> +
> +	return offset;
> +}
> +
> +void fsl_msi_free_hwirqs(struct fsl_msi *msi, int offset, int num)

static?

> +{
> +	unsigned long flags;
> +	int order = get_count_order(num);
> +
> +	pr_debug("%s: freeing 0x%x (2^%d) at offset 0x%x\n",
> +		__func__, num, order, offset);
> +
> +	spin_lock_irqsave(&msi->bitmap_lock, flags);
> +	bitmap_release_region(msi->fsl_msi_bitmap, offset, order);
> +	spin_unlock_irqrestore(&msi->bitmap_lock, flags);
> +}
> +
> +static int fsl_msi_free_dt_hwirqs(struct fsl_msi *msi)
> +{
> +	int i, len;
> +	const u32 *p;
> +
> +	bitmap_allocate_region(msi->fsl_msi_bitmap, 0,
> +		       get_count_order(NR_MSI_IRQS));
> +
> +	p = of_get_property(msi->of_node, "msi-available-ranges", &len);
> +
> +	if (!p) {
> +		/* No msi-available-ranges property,
> +		 * All the 256 MSI interrupts can be used
> +		 */
> +		fsl_msi_free_hwirqs(msi, 0, 0x100);
> +		return 0;
> +	}
> +
> +	if ((len % (2 * sizeof(u32))) != 0) {
> +		printk(KERN_WARNING "fsl_msi: Malformed msi-available-ranges "
> +		       "property on %s\n", msi->of_node->full_name);
> +		return -EINVAL;
> +	}
> +
> +	/* Format is: (<u32 start> <u32 count>)+ */
> +	len /= 2 * sizeof(u32);
> +	for (i = 0; i < len; i++, p += 2)
> +		fsl_msi_free_hwirqs(msi, *p, *(p + 1));
> +
> +	return 0;
> +}
> +
> +static int fsl_msi_init_allocator(struct fsl_msi *msi_data)
> +{
> +	int rc, size;
> +
> +	size = BITS_TO_LONGS(NR_MSI_IRQS) * sizeof(u32);

int rc;
int size = BITS_TO_LONGS(NR_MSI_IRQS) * sizeof(u32);

> +	msi_data->fsl_msi_bitmap = kzalloc(size, GFP_KERNEL);
> +
> +	if (msi_data->fsl_msi_bitmap == NULL) {
> +		pr_debug("%s: ENOMEM allocating allocator bitmap!\n",
> +				__func__);
> +		return -ENOMEM;
> +	}
> +
> +	rc = fsl_msi_free_dt_hwirqs(msi_data);
> +	if (rc)
> +		goto out_free;
> +
> +	return 0;
> +out_free:
> +	kfree(msi_data->fsl_msi_bitmap);
> +
> +	msi_data->fsl_msi_bitmap = NULL;
> +	return rc;
> +
> +}
> +
> +static int fsl_msi_check_device(struct pci_dev *pdev, int nvec, int type)
> +{
> +	if (type == PCI_CAP_ID_MSIX)
> +		pr_debug("fslmsi: MSI-X untested, trying anyway.\n");
> +
> +	return 0;
> +}
> +
> +static void fsl_teardown_msi_irqs(struct pci_dev *pdev)
> +{
> +	struct msi_desc *entry;
> +	struct fsl_msi *msi_data = fsl_msi;
> +
> +	list_for_each_entry(entry, &pdev->msi_list, list) {
> +		if (entry->irq == NO_IRQ)
> +			continue;
> +		set_irq_msi(entry->irq, NULL);
> +		fsl_msi_free_hwirqs(msi_data, virq_to_hw(entry->irq), 1);
> +		irq_dispose_mapping(entry->irq);
> +	}
> +
> +	return;
> +}
> +
> +static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq,
> +				  struct msi_msg *msg)
> +{
> +	struct fsl_msi *msi_data = fsl_msi;
> +
> +	msg->address_lo = msi_data->msi_addr_lo;
> +	msg->address_hi = msi_data->msi_addr_hi;
> +	msg->data = hwirq;
> +
> +	pr_debug("%s: allocated srs: %d, ibs: %d\n",
> +		__func__, hwirq / IRQS_PER_MSI_REG, hwirq % IRQS_PER_MSI_REG);
> +}
> +
> +static int fsl_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
> +{
> +	irq_hw_number_t hwirq;
> +	int rc;
> +	unsigned int virq;
> +	struct msi_desc *entry;
> +	struct msi_msg msg;
> +	struct fsl_msi *msi_data = fsl_msi;
> +
> +	list_for_each_entry(entry, &pdev->msi_list, list) {
> +		hwirq = fsl_msi_alloc_hwirqs(msi_data, 1);
> +		if (hwirq < 0) {
> +			rc = hwirq;
> +			pr_debug("%s: fail allocating msi interrupt\n",
> +					__func__);
> +			goto out_free;
> +		}
> +
> +		virq = irq_create_mapping(msi_data->irqhost, hwirq);
> +
> +		if (virq == NO_IRQ) {
> +			pr_debug("%s: fail mapping hwirq 0x%lx\n",
> +					__func__, hwirq);
> +			fsl_msi_free_hwirqs(msi_data, hwirq, 1);
> +			rc = -ENOSPC;
> +			goto out_free;
> +		}
> +		set_irq_msi(virq, entry);
> +
> +		fsl_compose_msi_msg(pdev, hwirq, &msg);
> +		write_msi_msg(virq, &msg);
> +	}
> +	return 0;
> +
> +out_free:
> +	return rc;
> +}
> +
> +void fsl_msi_cascade(unsigned int irq, struct irq_desc *desc)
> +{

static?

> +	unsigned int cascade_irq;
> +	struct fsl_msi *msi_data = fsl_msi;
> +	int msir_index = -1;
> +	u32 msir_value = 0;
> +	u32 intr_index;
> +	u32 have_shift = 0;
> +
> +	spin_lock(&desc->lock);
> +	if ((msi_data->feature &  FSL_PIC_IP_MASK) == FSL_PIC_IP_IPIC) {
> +		if (desc->chip->mask_ack)
> +			desc->chip->mask_ack(irq);
> +		else {
> +			desc->chip->mask(irq);
> +			desc->chip->ack(irq);
> +		}
> +	}
> +
> +	if (unlikely(desc->status & IRQ_INPROGRESS))
> +		goto unlock;
> +
> +	msir_index = (int)(desc->handler_data);

Parentheses aren't necessary here.

> +	if (msir_index >= NR_MSI_REG)
> +		cascade_irq = NO_IRQ;
> +
> +	desc->status |= IRQ_INPROGRESS;
> +	switch (fsl_msi->feature & FSL_PIC_IP_MASK) {
> +	case FSL_PIC_IP_MPIC:
> +		msir_value = fsl_msi_read(msi_data->msi_regs,
> +			msir_index * 0x10);
> +		break;
> +	case FSL_PIC_IP_IPIC:
> +		msir_value = fsl_msi_read(msi_data->msi_regs, msir_index * 0x4);
> +		break;
> +	}
> +
> +	while (msir_value) {
> +		intr_index = ffs(msir_value) - 1;
> +
> +		cascade_irq = irq_linear_revmap(msi_data->irqhost,
> +				(msir_index * IRQS_PER_MSI_REG +
> +					intr_index + have_shift));

Some parentheses aren't necessary here.

> +		if (cascade_irq != NO_IRQ)
> +			generic_handle_irq(cascade_irq);
> +		have_shift += (intr_index + 1);

Parentheses..

> +		msir_value = (msir_value >> (intr_index + 1));

Outermost parentheses.

> +	}
> +	desc->status &= ~IRQ_INPROGRESS;
> +
> +	switch (msi_data->feature & FSL_PIC_IP_MASK) {
> +	case FSL_PIC_IP_MPIC:
> +		desc->chip->eoi(irq);
> +		break;
> +	case FSL_PIC_IP_IPIC:
> +		if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
> +			desc->chip->unmask(irq);
> +		break;
> +	}
> +unlock:
> +	spin_unlock(&desc->lock);
> +}
> +
> +static int __devinit fsl_of_msi_probe(struct of_device *dev,
> +				const struct of_device_id *match)
> +{
> +	struct fsl_msi *msi;
> +	struct resource res;
> +	int err, i, count;
> +	int rc;
> +	int virt_msir;
> +	const u32 *p;
> +	struct fsl_msi_feature *tmp_data;

(1) unfortunate name

> +	printk(KERN_DEBUG "Setting up Freescale MSI support\n");
> +
> +	msi = kzalloc(sizeof(struct fsl_msi), GFP_KERNEL);
> +	if (!msi) {
> +		dev_err(&dev->dev, "No memory for MSI structure\n");
> +		err = -ENOMEM;
> +		goto error_out;
> +	}
> +
> +	msi->of_node = of_node_get(dev->node);
> +
> +	msi->irqhost = irq_alloc_host(of_node_get(dev->node),
> +				IRQ_HOST_MAP_LINEAR,
> +				NR_MSI_IRQS, &fsl_msi_host_ops, 0);
> +	if (msi->irqhost == NULL) {
> +		dev_err(&dev->dev, "No memory for MSI irqhost\n");
> +		of_node_put(dev->node);
> +		err = -ENOMEM;
> +		goto error_out;
> +	}
> +
> +	/* Get the MSI reg base */
> +	err = of_address_to_resource(dev->node, 0, &res);
> +	if (err) {
> +		dev_err(&dev->dev, "%s resource error!\n",
> +				dev->node->full_name);
> +		goto error_out;
> +	}
> +
> +	msi->msi_regs = ioremap(res.start, res.end - res.start + 1);
> +	if (!msi->msi_regs) {
> +		dev_err(&dev->dev, "ioremap problem failed\n");
> +		goto error_out;
> +	}
> +
> +	tmp_data = (struct fsl_msi_feature *)match->data;

Cast isn't necessary, plus this could be placed in (1).

> +
> +	msi->feature = tmp_data->fsl_pic_ip;
> +
> +	msi->irqhost->host_data = msi;
> +
> +	msi->msi_addr_hi = 0x0;
> +	msi->msi_addr_lo = res.start + tmp_data->msiir_offset;
> +
> +	rc = fsl_msi_init_allocator(msi);
> +	if (rc) {
> +		dev_err(&dev->dev, "Error allocating MSI bitmap\n");
> +		goto error_out;
> +	}
> +
> +	p = of_get_property(dev->node, "interrupts", &count);
> +	if (!p) {
> +		dev_err(&dev->dev, "no interrupts property found on %s\n",
> +				dev->node->full_name);
> +		err = -ENODEV;
> +		goto error_out;
> +	}
> +	if (count % 8 != 0) {
> +		dev_err(&dev->dev, "Malformed interrupts property on %s\n",
> +				dev->node->full_name);
> +		err = -EINVAL;
> +		goto error_out;
> +	}
> +
> +	count /= sizeof(u32);
> +	for (i = 0; i < count / 2; i++) {
> +		if (i > NR_MSI_REG)
> +			break;
> +		virt_msir = irq_of_parse_and_map(dev->node, i);
> +		if (virt_msir != NO_IRQ) {
> +			set_irq_data(virt_msir, (void *)i);
> +			set_irq_chained_handler(virt_msir, fsl_msi_cascade);
> +		}
> +	}
> +
> +	fsl_msi = msi;
> +
> +	WARN_ON(ppc_md.setup_msi_irqs);
> +	ppc_md.setup_msi_irqs = fsl_setup_msi_irqs;
> +	ppc_md.teardown_msi_irqs = fsl_teardown_msi_irqs;
> +	ppc_md.msi_check_device = fsl_msi_check_device;
> +	return 0;
> +error_out:
> +	kfree(msi);
> +	return err;
> +}
> +
> +static const struct fsl_msi_feature mpic_msi_feature = {
> +	.fsl_pic_ip = FSL_PIC_IP_MPIC,
> +	.msiir_offset = 0x140,
> +};
> +
> +static const struct fsl_msi_feature ipic_msi_feature = {
> +	.fsl_pic_ip = FSL_PIC_IP_IPIC,
> +	.msiir_offset = 0x38,
> +};
> +
> +static const struct of_device_id fsl_of_msi_ids[] = {
> +	{
> +		.compatible = "fsl,mpic-msi",
> +		.data = (void *)&mpic_msi_feature,
> +	},
> +	{
> +		.compatible = "fsl,ipic-msi",
> +		.data = (void *)&ipic_msi_feature,
> +	},
> +	{}
> +};
> +
> +static struct of_platform_driver fsl_of_msi_driver = {
> +	.name = "fsl-msi",
> +	.match_table = fsl_of_msi_ids,
> +	.probe = fsl_of_msi_probe,
> +};
> +
> +static __init int fsl_of_msi_init(void)
> +{
> +	return of_register_platform_driver(&fsl_of_msi_driver);
> +}
> +
> +subsys_initcall(fsl_of_msi_init);
> diff --git a/arch/powerpc/sysdev/fsl_msi.h b/arch/powerpc/sysdev/fsl_msi.h
> new file mode 100644
> index 0000000..a653468
> --- /dev/null
> +++ b/arch/powerpc/sysdev/fsl_msi.h
> @@ -0,0 +1,42 @@
> +/*
> + * Copyright (C) 2007-2008 Freescale Semiconductor, Inc. All rights reserved.
> + *
> + * Author: Tony Li <[email-masked]>
> + *	   Jason Jin <[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; version 2 of the
> + * License.
> + *
> + */
> +#ifndef _POWERPC_SYSDEV_FSL_MSI_H
> +#define _POWERPC_SYSDEV_FSL_MSI_H
> +
> +#define NR_MSI_REG		8
> +#define IRQS_PER_MSI_REG	32
> +#define NR_MSI_IRQS	(NR_MSI_REG * IRQS_PER_MSI_REG)
> +
> +#define FSL_PIC_IP_MASK	0x0000000F
> +#define FSL_PIC_IP_MPIC	0x00000001
> +#define FSL_PIC_IP_IPIC	0x00000002
> +
> +struct fsl_msi {
> +	/* Device node of the MSI interrupt*/
> +	struct device_node *of_node;
> +
> +	struct irq_host *irqhost;
> +
> +	unsigned long cascade_irq;
> +
> +	u32 msi_addr_lo;
> +	u32 msi_addr_hi;
> +	void __iomem *msi_regs;
> +	u32 feature;
> +
> +	unsigned long *fsl_msi_bitmap;
> +	spinlock_t bitmap_lock;
> +};
> +
> +#endif /* _POWERPC_SYSDEV_FSL_MSI_H */
> +
> diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
> index bf13c21..52a5f7f 100644
> --- a/arch/powerpc/sysdev/fsl_pci.c
> +++ b/arch/powerpc/sysdev/fsl_pci.c
> @@ -106,6 +106,16 @@ void __init setup_pci_cmd(struct pci_controller *hose)
>  	}
>  }
>  
> +#ifdef CONFIG_PCI_MSI
> +void __init setup_pci_pcsrbar(struct pci_controller *hose)

static?

> +{
> +	phys_addr_t immr_base;
> +
> +	immr_base = get_immrbase();
> +	early_write_config_dword(hose, 0, 0, PCI_BASE_ADDRESS_0, immr_base);
> +}

#else
static inline void setup_pci_pcsrbar(struct pci_controller *hose) {}

^^ with this you can avoid  #ifdef below

> +#endif
> +
>  static int fsl_pcie_bus_fixup;
>  
>  static void __init quirk_fsl_pcie_header(struct pci_dev *dev)
> @@ -211,6 +221,10 @@ int __init fsl_add_bridge(struct device_node *dev, int is_primary)
>  	/* Setup PEX window registers */
>  	setup_pci_atmu(hose, &rsrc);
>  
> +	/* Setup PEXCSRBAR */
> +#ifdef CONFIG_PCI_MSI
> +	setup_pci_pcsrbar(hose);
> +#endif
>  	return 0;
>  }
>  
> -- 
> 1.5.4

-- 
Anton Vorontsov
email: [email-masked]
irc://irc.freenode.net/bd2