[PATCH] powerpc/fsl_msi: Handle msi-available-ranges better

[PATCH] powerpc/fsl_msi: Handle msi-available-ranges better

[Scott Wood]

Now handles multiple ranges, doesn't make assumptions about interrupt
specifier format, and doesn't claim interrupts that don't correspond to an
available range.

Also has some better error checking.

The device tree binding is updated to clarify some existing assumptions.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 Documentation/powerpc/dts-bindings/fsl/msi-pic.txt |    9 ++-
 arch/powerpc/sysdev/fsl_msi.c                      |   92 ++++++++++++--------
 2 files changed, 64 insertions(+), 37 deletions(-)

diff --git a/Documentation/powerpc/dts-bindings/fsl/msi-pic.txt b/Documentation/powerpc/dts-bindings/fsl/msi-pic.txt
index bcc30ba..c0d80fb 100644
--- a/Documentation/powerpc/dts-bindings/fsl/msi-pic.txt
+++ b/Documentation/powerpc/dts-bindings/fsl/msi-pic.txt
@@ -5,14 +5,21 @@ Required properties:
   first is "fsl,CHIP-msi", where CHIP is the processor(mpc8610, mpc8572,
   etc.) and the second is "fsl,mpic-msi" or "fsl,ipic-msi" depending on
   the parent type.
+
 - reg : should contain the address and the length of the shared message
   interrupt register set.
+
 - msi-available-ranges: use <start count> style section to define which
   msi interrupt can be used in the 256 msi interrupts. This property is
   optional, without this, all the 256 MSI interrupts can be used.
+  Each available range must begin and end on a multiple of 32 (i.e.
+  no splitting an individual MSI register or the associated PIC interrupt).
+
 - interrupts : each one of the interrupts here is one entry per 32 MSIs,
   and routed to the host interrupt controller. the interrupts should
-  be set as edge sensitive.
+  be set as edge sensitive.  If msi-available-ranges is present, only
+  the interrupts that correspond to available ranges shall be present.
+
 - interrupt-parent: the phandle for the interrupt controller
   that services interrupts for this device. for 83xx cpu, the interrupts
   are routed to IPIC, and for 85xx/86xx cpu the interrupts are routed
diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c
index 108d76f..46e27ac 100644
--- a/arch/powerpc/sysdev/fsl_msi.c
+++ b/arch/powerpc/sysdev/fsl_msi.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2007-2010 Freescale Semiconductor, Inc.
+ * Copyright (C) 2007-2011 Freescale Semiconductor, Inc.
  *
  * Author: Tony Li <tony.li@freescale.com>
  *	   Jason Jin <Jason.jin@freescale.com>
@@ -273,19 +273,47 @@ static int fsl_of_msi_remove(struct platform_device *ofdev)
 	return 0;
 }
 
+static int __devinit fsl_msi_setup_hwirq(struct fsl_msi *msi,
+					 struct platform_device *dev,
+					 int offset, int irq_index)
+{
+	struct fsl_msi_cascade_data *cascade_data = NULL;
+	int virt_msir;
+
+	virt_msir = irq_of_parse_and_map(dev->dev.of_node, irq_index);
+	if (virt_msir == NO_IRQ) {
+		dev_err(&dev->dev, "%s: Cannot translate IRQ index %d\n",
+			__func__, irq_index);
+		return 0;
+	}
+
+	cascade_data = kzalloc(sizeof(struct fsl_msi_cascade_data), GFP_KERNEL);
+	if (!cascade_data) {
+		dev_err(&dev->dev, "No memory for MSI cascade data\n");
+		return -ENOMEM;
+	}
+
+	msi->msi_virqs[irq_index] = virt_msir;
+	cascade_data->index = offset + irq_index;
+	cascade_data->msi_data = msi;
+	set_irq_data(virt_msir, cascade_data);
+	set_irq_chained_handler(virt_msir, fsl_msi_cascade);
+
+	return 0;
+}
+
 static int __devinit fsl_of_msi_probe(struct platform_device *dev,
 				const struct of_device_id *match)
 {
 	struct fsl_msi *msi;
 	struct resource res;
-	int err, i, count;
+	int err, i, j, irq_index, count;
 	int rc;
-	int virt_msir;
 	const u32 *p;
 	struct fsl_msi_feature *features = match->data;
-	struct fsl_msi_cascade_data *cascade_data = NULL;
 	int len;
 	u32 offset;
+	static const u32 all_avail[] = { 0, NR_MSI_IRQS };
 
 	printk(KERN_DEBUG "Setting up Freescale MSI support\n");
 
@@ -332,42 +360,34 @@ static int __devinit fsl_of_msi_probe(struct platform_device *dev,
 		goto error_out;
 	}
 
-	p = of_get_property(dev->dev.of_node, "interrupts", &count);
-	if (!p) {
-		dev_err(&dev->dev, "no interrupts property found on %s\n",
-				dev->dev.of_node->full_name);
-		err = -ENODEV;
-		goto error_out;
-	}
-	if (count % 8 != 0) {
-		dev_err(&dev->dev, "Malformed interrupts property on %s\n",
-				dev->dev.of_node->full_name);
+	p = of_get_property(dev->dev.of_node, "msi-available-ranges", &len);
+	if (p && len % (2 * sizeof(u32)) != 0) {
+		dev_err(&dev->dev, "%s: Malformed msi-available-ranges property\n",
+			__func__);
 		err = -EINVAL;
 		goto error_out;
 	}
-	offset = 0;
-	p = of_get_property(dev->dev.of_node, "msi-available-ranges", &len);
-	if (p)
-		offset = *p / IRQS_PER_MSI_REG;
-
-	count /= sizeof(u32);
-	for (i = 0; i < min(count / 2, NR_MSI_REG); i++) {
-		virt_msir = irq_of_parse_and_map(dev->dev.of_node, i);
-		if (virt_msir != NO_IRQ) {
-			cascade_data = kzalloc(
-					sizeof(struct fsl_msi_cascade_data),
-					GFP_KERNEL);
-			if (!cascade_data) {
-				dev_err(&dev->dev,
-					"No memory for MSI cascade data\n");
-				err = -ENOMEM;
+
+	if (!p)
+		p = all_avail;
+
+	for (irq_index = 0, i = 0; i < len / (2 * sizeof(u32)); i++) {
+		if (p[i * 2] % IRQS_PER_MSI_REG ||
+		    p[i * 2 + 1] % IRQS_PER_MSI_REG) {
+			printk(KERN_WARNING "%s: %s: msi available range of %u at %u is not IRQ-aligned\n",
+			       __func__, dev->dev.of_node->full_name,
+			       p[i * 2 + 1], p[i * 2]);
+			err = -EINVAL;
+			goto error_out;
+		}
+
+		offset = p[i * 2] / IRQS_PER_MSI_REG;
+		count = p[i * 2 + 1] / IRQS_PER_MSI_REG;
+
+		for (j = 0; j < count; j++, irq_index++) {
+			err = fsl_msi_setup_hwirq(msi, dev, offset, irq_index);
+			if (err)
 				goto error_out;
-			}
-			msi->msi_virqs[i] = virt_msir;
-			cascade_data->index = i + offset;
-			cascade_data->msi_data = msi;
-			set_irq_data(virt_msir, (void *)cascade_data);
-			set_irq_chained_handler(virt_msir, fsl_msi_cascade);
 		}
 	}
 
-- 
1.7.0.4

Re: [PATCH] powerpc/fsl_msi: Handle msi-available-ranges better

[Kumar Gala]

On Jan 17, 2011, at 2:25 PM, Scott Wood wrote:

> Now handles multiple ranges, doesn't make assumptions about interrupt
> specifier format, and doesn't claim interrupts that don't correspond =
to an
> available range.
>=20
> Also has some better error checking.
>=20
> The device tree binding is updated to clarify some existing =
assumptions.
>=20
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
> Documentation/powerpc/dts-bindings/fsl/msi-pic.txt |    9 ++-
> arch/powerpc/sysdev/fsl_msi.c                      |   92 =
++++++++++++--------
> 2 files changed, 64 insertions(+), 37 deletions(-)

applied

- k=

Re: [PATCH] powerpc/fsl_msi: Handle msi-available-ranges better

[Tabi Timur-B04825]

On Mon, Jan 17, 2011 at 2:25 PM, Scott Wood <scottwood@freescale.com> wrote=
:

> + =A0 =A0 =A0 for (irq_index =3D 0, i =3D 0; i < len / (2 * sizeof(u32));=
 i++) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (p[i * 2] % IRQS_PER_MSI_REG ||
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 p[i * 2 + 1] % IRQS_PER_MSI_REG) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_WARNING "%s: %s=
: msi available range of %u at %u is not IRQ-aligned\n",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0__func__, de=
v->dev.of_node->full_name,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0p[i * 2 + 1]=
, p[i * 2]);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 err =3D -EINVAL;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto error_out;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 offset =3D p[i * 2] / IRQS_PER_MSI_REG;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 count =3D p[i * 2 + 1] / IRQS_PER_MSI_REG;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 for (j =3D 0; j < count; j++, irq_index++) =
{
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 err =3D fsl_msi_setup_hwirq=
(msi, dev, offset, irq_index);

Scott,

I know it's been eight months since you posted this patch, but I'm not
sure this part is correct.   fsl_msi_setup_hwirq() does this:

cascade_data->index =3D offset + irq_index;

"cascade_data->index" is supposed to be the index of the MSIIR
register, so it's a number between 0 and 7.  irq_index is incremented
on every pass of loop 'j'.  My understanding is that the following two
definitions of msi-available-ranges are equivalent:

static const u32 all_avail[] =3D { 0, NR_MSI_IRQS };

static const u32 all_avail[] =3D { 0, 64, 64, 32, 96, 32, 128, 32, 160, 96}=
;

When I use the first definition, I get this on each call to
fsl_msi_setup_hwirq():

fsl_msi_setup_hwirq:299 offset=3D0 irq_index=3D0 cascade_data->index=3D0
fsl_msi_setup_hwirq:299 offset=3D0 irq_index=3D1 cascade_data->index=3D1
fsl_msi_setup_hwirq:299 offset=3D0 irq_index=3D2 cascade_data->index=3D2
fsl_msi_setup_hwirq:299 offset=3D0 irq_index=3D3 cascade_data->index=3D3
fsl_msi_setup_hwirq:299 offset=3D0 irq_index=3D4 cascade_data->index=3D4
fsl_msi_setup_hwirq:299 offset=3D0 irq_index=3D5 cascade_data->index=3D5
fsl_msi_setup_hwirq:299 offset=3D0 irq_index=3D6 cascade_data->index=3D6
fsl_msi_setup_hwirq:299 offset=3D0 irq_index=3D7 cascade_data->index=3D7

But when I use the second definition of all_avail[], I get this instead:

fsl_msi_setup_hwirq:299 offset=3D0 irq_index=3D0 cascade_data->index=3D0
fsl_msi_setup_hwirq:299 offset=3D0 irq_index=3D1 cascade_data->index=3D1
fsl_msi_setup_hwirq:299 offset=3D2 irq_index=3D2 cascade_data->index=3D4
fsl_msi_setup_hwirq:299 offset=3D3 irq_index=3D3 cascade_data->index=3D6
fsl_msi_setup_hwirq:299 offset=3D4 irq_index=3D4 cascade_data->index=3D8
fsl_msi_setup_hwirq:299 offset=3D5 irq_index=3D5 cascade_data->index=3D10
fsl_msi_setup_hwirq:299 offset=3D5 irq_index=3D6 cascade_data->index=3D11
fsl_msi_setup_hwirq:299 offset=3D5 irq_index=3D7 cascade_data->index=3D12

The problem is that both offset and irq_index are being incremented in
the loop, and cascade_data->index is set to the sum of the two.

Perhaps you meant this:

		err =3D fsl_msi_setup_hwirq(msi, dev, offset, j);

--=20
Timur Tabi
Linux kernel developer at Freescale=

Re: [PATCH] powerpc/fsl_msi: Handle msi-available-ranges better

[Scott Wood]

On 09/07/2011 11:20 AM, Tabi Timur-B04825 wrote:
> The problem is that both offset and irq_index are being incremented in
> the loop, and cascade_data->index is set to the sum of the two.
> 
> Perhaps you meant this:
> 
> 		err = fsl_msi_setup_hwirq(msi, dev, offset, j);

That's not right either, it would retrieve the wrong IRQ from the
interrupts property if you have holes -- try with something like
{ 0 64, 128, 64 }.  The desired behavior there is:

  offset = 0 irq_index = 0
  offset = 1 irq_index = 1
  offset = 4 irq_index = 2
  offset = 5 irq_index = 3

I think the right code (untested) might be:

	err = fsl_msi_setup_hwirq(msi, dev, offset + j, irq_index);

and

	cascade_data->index = offset;

-Scott