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

[Michael Ellerman <michael@ellerman.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4485 bytes --]

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@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@freescale.com>
> + *	   Jason Jin <Jason.jin@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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]