Hi Micheal, Thank you for your comments. I'll send the new version according to feedback.
Jason > -----Original Message----- > From: Michael Ellerman [mailto:[EMAIL PROTECTED] > Sent: Monday, May 05, 2008 7:24 PM > To: Jin Zhengxiong > Cc: [EMAIL PROTECTED]; linuxppc-dev@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 <[EMAIL PROTECTED]> > > > 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 <[EMAIL PROTECTED]> > > + * Jason Jin <[EMAIL PROTECTED]> > > + * > > + * 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 > _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
