[PATCH 1/4 V3] MSI support on 83xx/85xx/86xx board

[Jin Zhengxiong]

Hi Micheal,

Thank you for your comments.
I'll send the new version according to feedback.

Jason

> -----Original Message-----
> From: Michael Ellerman [mailto:michael at ellerman.id.au] 
> Sent: Monday, May 05, 2008 7:24 PM
> To: Jin Zhengxiong
> Cc: galak at kernel.crashing.org; linuxppc-dev at ozlabs.org
> Subject: Re: [PATCH 1/4 V3] MSI support on 83xx/85xx/86xx board
> 
> On Mon, 2008-05-05 at 15:47 +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 <Jason.jin at freescale.com>
> 
> 
> Hi Jason,
> 
> Just a couple of comments below:
> 
> > diff --git a/arch/powerpc/sysdev/fsl_msi.c 
> > b/arch/powerpc/sysdev/fsl_msi.c new file mode 100644 index 
> > 0000000..c53f716
> > --- /dev/null
> > +++ b/arch/powerpc/sysdev/fsl_msi.c
> > @@ -0,0 +1,442 @@
> > +/*
> > + * 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 <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;
> > +};
> > +
> > +/* A bit ugly, can we get this from the pci_dev somehow? */
> 
> This comment is old (from my code) and should go.
> 
> > +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)
> > +{
> > +	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_match(struct irq_host *h, struct 
> device_node 
> > +*node) {
> > +	/* Exact match, unless node is NULL */
> > +	return h->of_node == NULL || h->of_node == node; }
> 
> Are you sure you want this as your match routine? It looks 
> wrong to me.
> You won't ever create a fsl_msi in your probe routine unless 
> you have a device node, so AFAICT the of_node == NULL is only 
> ever going to match some _other_ irqhost.
> 
> 
> > diff --git a/arch/powerpc/sysdev/fsl_msi.h 
> > b/arch/powerpc/sysdev/fsl_msi.h new file mode 100644 index 
> > 0000000..0449c96
> > --- /dev/null
> > +++ b/arch/powerpc/sysdev/fsl_msi.h
> > @@ -0,0 +1,31 @@
> > +#ifndef _POWERPC_SYSDEV_FSL_MSI_H
> > +#define _POWERPC_SYSDEV_FSL_MSI_H
> 
> 
> This should have a copyright header.
> 
> > +#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;
> > +	const char *name;
> 
> I don't see where this is used?
> 
> 
> cheers
> 
> --
> Michael Ellerman
> OzLabs, IBM Australia Development Lab
> 
> wwweb: http://michael.ellerman.id.au
> phone: +61 2 6212 1183 (tie line 70 21183)
> 
> We do not inherit the earth from our ancestors, we borrow it 
> from our children. - S.M.A.R.T Person
>