[PATCH 1/3] MSI driver for Freescale 83xx/85xx/86xx cpu

[Jin Zhengxiong]

Hi, Michael,

Thank you very much for you input, please see my inline answer.

B.R
Jason 

> -----Original Message-----
> From: Michael Ellerman [mailto:michael at ellerman.id.au] 
> Sent: Monday, April 21, 2008 2:22 PM
> To: Jin Zhengxiong
> Cc: linuxppc-dev at ozlabs.org; Gala Kumar
> Subject: Re: [PATCH 1/3] MSI driver for Freescale 83xx/85xx/86xx cpu
> 
> > 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.
>  
> 
> Hi Jason, some comments inline ...
> 
>  
> > diff --git a/arch/powerpc/sysdev/Makefile 
> b/arch/powerpc/sysdev/Makefile
> > index 6d386d0..bfd3fe4 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)
> > +obj-$(CONFIG_PCI_MSI)		+= fsl_msi.o
> 
> Do we really always want to build this if we have MSI? Might it depend
> on something else as well? CONFIG_FSL_PCI maybe?
> 
I'll try to change the depend.

> > diff --git a/arch/powerpc/sysdev/fsl_msi.c 
> b/arch/powerpc/sysdev/fsl_msi.c
> > new file mode 100644
> > index 0000000..e8132cf
> > --- /dev/null
> > +++ b/arch/powerpc/sysdev/fsl_msi.c
> > @@ -0,0 +1,457 @@
> > +/*
> > + * Copyright (C) 2007-2008 Freescale Semiconductor, Inc. 
> All rights reserved.
> > + *
> > + * Author: Tony Li <tony.li at freescale.com>
> > + *	   Jason Jin <Jason.jin at freescale.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; version 2 of the
> > + * License.
> > + *
> > + */
> > +
> > +#include <linux/irq.h>
> > +#include <linux/bootmem.h>
> > +#include <linux/bitmap.h>
> > +#include <linux/msi.h>
> > +#include <asm/prom.h>
> > +#include <asm/hw_irq.h>
> > +#include <linux/pci.h>
> > +#include <asm/ppc-pci.h>
> > +#include <linux/of_platform.h>
> > +
> > +#include <sysdev/fsl_soc.h>
> > +#include "fsl_msi.h"
> 
> People consider it good style to have the linux includes before the
> asm includes.
> 
OK

> > +#ifdef DEBUG
> > +#define pr_debug(fmt...) printk(fmt)
> > +#else
> > +#define pr_debug(fmt...)
> > +#endif
> 
> Please don't do this, just use pr_debug(). In fact I don't 
> see where you
> do use it :)
> 
OK.

> > +/* A bit ugly, can we get this from the pci_dev somehow? */
> > +static struct fsl_msi *fsl_msi;
> 
> I recognise that comment :)  The answer is "no" we can't (easily) get
> this from the pci_dev.
> 
> > +static inline u32 fsl_msi_read(u32 __iomem *base,
> > +				unsigned int reg)
> 
> This would fit in 80 chars wouldn't it?
> 
OK

> > +{
> > +	return in_be32(base + (reg >> 2));
> > +}
> > +
> > +static inline void fsl_msi_write(u32 __iomem *base,
> > +				unsigned int reg, u32 value)
> > +{
> > +	out_be32(base + (reg >> 2), value);
> > +}
> > +
> > +#define	fsl_msi_irq_to_hw(virq)	 ((unsigned 
> int)irq_map[virq].hwirq)
> 
> I can't see where this is used, you probably don't need it.
>
I'll remove this.
 
> > +/*
> > + * 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)
> > +{
> > +}
> 
> I guess the generic code assumes you have an ack, bummer.
> 
Yes, the generic code did not check it.

> > +static struct irq_chip fsl_msi_chip = {
> > +	.mask		= mask_msi_irq,
> > +	.unmask		= unmask_msi_irq,
> > +	.ack		= fsl_msi_end_irq,
> > +	.typename	= " FSL-MSI  ",
> 
> I'd rather you didn't try and pad the name by hand, if we 
> want /proc/interrupts
> to look pretty we should do that in show_interrupts().
> 
Thanks, but I feel it's easier and cleaner to add the typename here to
make 
the /proc/interrupts clear. 

> > +static int fsl_msi_host_match(struct irq_host *h, struct 
> device_node *node)
> > +{
> > +	struct fsl_msi *msi = h->host_data;
> > +
> > +	/* Exact match, unless node is NULL */
> > +	return msi->of_node == NULL || msi->of_node == node;
> > +}
> 
> Do you really want the MSI to be the default irq host?
> 
Thanks. I'll change the host match function.

> > +static int fsl_msi_host_map(struct irq_host *h, unsigned int virq,
> > +				irq_hw_number_t hw)
> > +{
> > +	struct fsl_msi *msi = h->host_data;
> > +	struct irq_chip *chip = msi->hc_irq;
> > +
> > +	set_irq_chip_data(virq, msi);
> 
> You don't seem to use chip_data anywhere?
> 
I'll check this.

> > +	get_irq_desc(virq)->status |= IRQ_TYPE_EDGE_FALLING;
> > +
> > +	set_irq_chip_and_handler(virq, chip,  handle_edge_irq);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct irq_host_ops fsl_msi_host_ops = {
> > +	.match = fsl_msi_host_match,
> > +	.map = fsl_msi_host_map,
> > +};
> > +
> > +irq_hw_number_t fsl_msi_alloc_hwirqs(struct fsl_msi *msi, int num)
> > +{
> > +	unsigned long flags;
> > +	int offset, order = get_count_order(num);
> > +
> > +	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)
> > +{
> > +	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_reserve_dt_hwirqs(struct fsl_msi *msi)
> > +{
> > +	int i, len;
> > +	const u32 *p;
> > +
> > +	p = of_get_property(msi->of_node, "msi-available-ranges", &len);
> > +	if (!p) {
> > +		pr_debug("fsl_msi: no msi-available-ranges 
> property found \
> > +				on %s\n", msi->of_node->full_name);
> > +		return -ENODEV;
> > +	}
> > +
> > +	if (len & 0x8 != 0) {
> > +		printk(KERN_WARNING "fsl_msi: Malformed 
> msi-available-ranges "
> > +		       "property on %s\n", msi->of_node->full_name);
> > +		return -EINVAL;
> > +	}
> 
> Do you really want a bitwise and with 0x8?
> 
The range for the msi interrupt can be seperated to several part. 
This can used to check the if the ranges is correct. 

> > +
> > +	bitmap_allocate_region(msi->fsl_msi_bitmap, 0,
> > +			       get_count_order(msi->irq_count));
> > +
> > +	/* Format is: (<u32 start> <u32 count>)+ */
> > +	len /= sizeof(u32);
> > +	len /= 2;
> > +	for (i = 0; i < len; i++, p += 2)
> > +		fsl_msi_free_hwirqs(msi, *p, *(p + 1));
> > +
> > +	return 0;
> > +}
> 
> We could share a bunch of that code with mpic_msi.c, but 
> that's not your
> job - I'll look at doing a patch once this goes in.
> 
Thanks for the code you write for the MPIC msi.
 I'll use this until you share the code.

> > +static int fsl_msi_init_allocator(struct fsl_msi *msi)
> > +{
> > +	int rc, size;
> > +
> > +	size = BITS_TO_LONGS(NR_MSI_IRQS) * sizeof(long);
> > +
> > +	msi->fsl_msi_bitmap = kmalloc(size, GFP_KERNEL);
> > +
> > +	if (msi->fsl_msi_bitmap == NULL) {
> > +		pr_debug("%s: ENOMEM allocating allocator bitmap!\n",
> > +				__func__);
> > +		return -ENOMEM;
> > +	}
> > +	memset(msi->fsl_msi_bitmap, 0, size);
> 
> Use kzalloc() and you can lose the memset.
> 
OK.

> > +	rc = fsl_msi_reserve_dt_hwirqs(msi);
> 
> This routine is badly named (my fault), it really frees the available
> MSI ranges.
> 
> > +	if (rc)
> > +		goto out_free;
> > +
> > +	return 0;
> > +out_free:
> > +	if (mem_init_done)
> > +		kfree(msi->fsl_msi_bitmap);
> 
> You don't need to test mem_init_done, this is running at initcall time
> which is way later than mem_init.
> 
Yes, I'll remove this. This code once used at just after the mpic was
intialized,and this
was left here.

> > +	msi->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 = 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, 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)
> > +{
> > +	unsigned int srs;
> > +	unsigned int ibs;
> > +	struct fsl_msi *msi = fsl_msi;
> > +
> > +	srs = hwirq / INT_PER_MSIR;
> > +	ibs = hwirq % INT_PER_MSIR;
> > +
> > +	msg->address_lo = msi->msi_addr_lo;
> > +	msg->address_hi = msi->msi_addr_hi;
> > +	msg->data = (srs << 5) | (ibs & 0x1F);
> 
> Is the 5 and 0x1F independent of the INT_PER_MSIR value? Given the
> current values isn't this a no-op, or am I missing something?
> 
Do you mean there're another way to get the msg->data from the hwirq?  

> > +	pr_debug("%s: allocated srs: %d, ibs: %d\n",
> > +		__func__, srs, ibs);
> > +}
> > +
> > +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 = fsl_msi;
> 
> A couple of places you put this into a local called "msi" 
> which is not the
> greatest name in the world IMHO :)
> 
Thank you, I'll try to use another name, Do you have any suggestion?

> > +	list_for_each_entry(entry, &pdev->msi_list, list) {
> > +		hwirq = fsl_msi_alloc_hwirqs(msi, 1);
> > +		if (hwirq < 0) {
> > +			rc = hwirq;
> > +			pr_debug("%s: fail allocating msi interrupt\n",
> > +					__func__);
> > +			goto out_free;
> > +		}
> > +
> > +		virq = irq_create_mapping(msi->irqhost, hwirq);
> > +
> > +		if (virq == NO_IRQ) {
> > +			pr_debug("%s: fail mapping hwirq 0x%lx\n",
> > +					__func__, hwirq);
> > +			fsl_msi_free_hwirqs(msi, 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:
> > +	fsl_teardown_msi_irqs(pdev);
> 
> You don't need to call teardown, the generic code does that for you.
> 
OK.

> > +	return rc;
> > +}
> > +
> > +void fsl_msi_cascade(unsigned int irq, struct irq_desc *desc)
> > +{
> > +	unsigned int cascade_irq;
> > +	struct fsl_msi *msi = fsl_msi;
> > +	int msir_index = -1;
> > +	int i;
> > +	u32 msir_value = 0;
> > +	u32 intr_index;
> > +	u32 have_shift = 0;
> > +
> > +	spin_lock(&desc->lock);
> > +	if ((msi->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;
> > +
> > +	for (i = 0; i < NR_MSIR; i++)
> > +		if (irq == msi->msir[i]) {
> > +			msir_index = i;
> > +			break;
> > +		}
> 
> This is a bit ugly :)  Because you get the *msi from fsl_msi 
> (above), you could
> store the msir_index in the handler_data (with set_irq_data), 
> and save doing
> this loop.
> 
I'll find if the handler data *msi was used somewhere. if not I change
to
save the msir_index to the handler data. 

> > +	if (i >= NR_MSIR)
> > +		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->msi_regs, 
> msir_index * 0x10);
> > +		break;
> > +	case FSL_PIC_IP_IPIC:
> > +		msir_value = fsl_msi_read(msi->msi_regs, 
> msir_index * 0x4);
> > +		break;
> > +	}
> > +
> > +	while (msir_value) {
> > +		intr_index = ffs(msir_value) - 1;
> > +
> > +		cascade_irq = irq_linear_revmap(msi->irqhost,
> > +			(msir_index * INT_PER_MSIR + intr_index 
> + have_shift));
> > +
> > +		if (cascade_irq != NO_IRQ)
> > +			generic_handle_irq(cascade_irq);
> > +		have_shift += (intr_index + 1);
> > +		msir_value = (msir_value >> (intr_index + 1));
> > +	}
> 
> It took me a while to grok all the shifting and so on here, I 
> don't know if
> there's a cleaner way to do it.
> 
To improve the efficency here, I tried to use the bit operation which
can 
carry out by the cpu with one instruction. Maybe I also need to improve 
readability :).

> > +	desc->status &= ~IRQ_INPROGRESS;
> > +
> > +	switch (fsl_msi->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);
> 
> Missing indent.
> 
OK.

> > +		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_data *tmp_data;
> > +
> > +	printk(KERN_INFO "Setting up fsl msi support\n");
> 
> KERN_DEBUG would do here IMHO, users don't usually need to see it.
> 
> > +	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 = 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");
> > +		err = -ENOMEM;
> 
> You need an of_node_put(dev->node) in the error case here.
> 
Thanks, I'll add it.

> > +		goto error_out;
> > +	}
> > +
> > +	/*Get the MSI reg base */
> 
> Missing space after /*
> 
> > +	err = of_address_to_resource(dev->node, 0, &res);
> > +	if (err) {
> > +		dev_err(&dev->dev, "Can't get %s property 'reg'\n",
> > +				dev->node->full_name);
> 
> That's a little misleading, aren't there's lots of reasons
> of_address_to_resource() might fail?
> 
> > +		goto error_out;
> > +	}
> > +
> > +	msi->msi_regs = ioremap(res.start, res.end - res.start + 1);
> 
> ioremap() can fail.
> 
OK,  I'll add the error process code here.

> > +	tmp_data = (struct fsl_msi_data *)match->data;
> > +
> > +	msi->feature = tmp_data->fsl_pic_ip;
> > +
> > +	msi->irqhost->host_data = msi;
> > +	msi->hc_irq = &fsl_msi_chip;
> 
> Any reason this needs to be a variable, isn't it always &fsl_msi_chip?
> 
> > +	msi->msi_addr_hi = 0x0;
> > +	msi->msi_addr_lo = res.start + tmp_data->msiir_offset;
> > +
> > +	msi->irq_count = NR_MSI_IRQS;
> 
> Ditto.
> 
> > +	p = of_get_property(dev->node, "interrupts", &count);
> > +	if (!p) {
> > +		dev_err(&dev->dev, "no msi-interrupts property 
> found on %s\n",
> > +				dev->node->full_name);
> > +		err = -ENODEV;
> > +		goto error_out;
> > +	}
> > +	if (count % 8 != 0) {
> > +		dev_err(&dev->dev, "Malformed msi-interrupts 
> property on %s\n",
> > +				dev->node->full_name);
> 
> Messages don't match the code, "interrupts" vs "msi-interrupts".
> 
Thanks, I'll change the description.

> > +		err = -EINVAL;
> > +		goto error_out;
> > +	}
> > +
> > +	count /= sizeof(u32);
> > +	for (i = 0; i < count / 2; i++) {
> > +		if (i > NR_MSIR)
> > +			break;
> > +		virt_msir = irq_of_parse_and_map(dev->node, i);
> > +		if (virt_msir != NO_IRQ) {
> > +			set_irq_data(virt_msir, msi);
> > +			set_irq_chained_handler(virt_msir, 
> fsl_msi_cascade);
> > +			msi->msir[i] = virt_msir;
> > +		} else
> > +			msi->msir[i] = NO_IRQ;
> > +	}
> > +
> > +	rc = fsl_msi_init_allocator(msi);
> > +	if (rc) {
> > +		dev_err(&dev->dev, "Error allocating MSI bitmap\n");
> 
> If you hit this then the error case needs to get rid of all 
> the irq mappings
> you created just above (which it doesn't). It would be 
> simpler if you just move
> this call above the for loop, then you don't need to worry about it.
>
Thanks, good idea.
 
> > +		goto error_out;
> > +	}
> > +
> > +	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:
> > +	if (msi)
> > +		kfree(msi);
> 
> kfree(NULL) is fine.
> 
> > +	return err;
> > +}
> > +
> > +static const struct fsl_msi_data mpic_msi_feature = 
> {FSL_PIC_IP_MPIC, 0x140};
> > +static const struct fsl_msi_data ipic_msi_feature = 
> {FSL_PIC_IP_IPIC, 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-of-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..7eef9ec
> > --- /dev/null
> > +++ b/arch/powerpc/sysdev/fsl_msi.h
> > @@ -0,0 +1,40 @@
> > +#ifndef _ASM_POWERPC_MSI_H
> > +#define _ASM_POWERPC_MSI_H
> 
> Include guards don't match the filename, should be 
> _POWERPC_SYSDEV_FSL_MSI_H
> 
OK.

> > +#define NR_MSIR	8
> > +#define INT_PER_MSIR	32
> > +#define NR_MSI_IRQS	(NR_MSIR * INT_PER_MSIR)
> > +
> > +#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;
> > +	struct irq_chip *hc_irq;
> > +
> > +	unsigned long cascade_irq;
> > +	unsigned int msir[NR_MSIR];
> > +
> > +	u32 msi_addr_lo;
> > +	u32 msi_addr_hi;
> > +	void __iomem *msi_regs;
> > +	u32 irq_count;
> > +	u32 feature;
> > +
> > +	spinlock_t fsl_msi_lock;
> 
> Unused?
> 
> > +	unsigned long *fsl_msi_bitmap;
> 
> Do we need fsl_msi in the name?
> 
> > +	spinlock_t bitmap_lock;
> > +	const char *name;
> 
> Unused?
> 
> > +};
> > +
> > +struct fsl_msi_data {
> > +	u32 fsl_pic_ip;
> > +	u32 msiir_offset;
> > +};
> > +
> > +#endif /* _ASM_POWERPC_MSI_H */
> > +
> > diff --git a/arch/powerpc/sysdev/fsl_pci.c 
> b/arch/powerpc/sysdev/fsl_pci.c
> > index bf13c21..fede767 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)
> > +{
> > +	phys_addr_t immr_base;
> > +
> > +	immr_base = get_immrbase();
> > +	early_write_config_dword(hose, 0, 0, 
> PCI_BASE_ADDRESS_0, immr_base);
> > +}
> > +#endif
> 
> Up to you, but I prefer an empty static inline here which 
> saves an #ifdef
> at the call site.
> 
> >  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;
> >  }
> 
> 
> cheers
>