Patchwork [2/5] powerpc/fsl_msi: add MSIIR1 support for MPIC v4.3

login
register
mail settings
Submitter Minghuan Lian
Date June 14, 2013, 7:15 a.m.
Message ID <1371194159-17332-2-git-send-email-Minghuan.Lian@freescale.com>
Download mbox | patch
Permalink /patch/251243/
State Superseded
Headers show

Comments

Minghuan Lian - June 14, 2013, 7:15 a.m.
MPIC controller v4.3 provides MSIIR1 to index 16 MSI registers.
MSIIR can only index 8 MSI registers. MSIIR1 uses different bits
definition than MSIIR. This patch adds ibs_shift and srs_shift to
indicate the bits definition of the MSIIR and MSIIR1, so the same
code can handle the MSIIR and MSIIR1 simultaneously.

Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
---
 arch/powerpc/sysdev/fsl_msi.c | 62 ++++++++++++++++++++++++++++++++++---------
 arch/powerpc/sysdev/fsl_msi.h |  4 ++-
 2 files changed, 53 insertions(+), 13 deletions(-)
Scott Wood - June 14, 2013, 10:09 p.m.
On 06/14/2013 02:15:56 AM, Minghuan Lian wrote:
> @@ -421,10 +440,29 @@ static int fsl_of_msi_probe(struct  
> platform_device *dev)
>  		}
>  		msi->msiir_offset =
>  			features->msiir_offset + (res.start & 0xfffff);
> +
> +		/*
> +		 * First read the MSIIR/MSIIR1 offset from dts
> +		 * If failure use the hardcode MSIIR offset

"On failure"

> +		 */
> +		if (of_address_to_resource(dev->dev.of_node, 1, &msiir))
> +			msi->msiir_offset = features->msiir_offset +
> +					    (res.start &  
> MSIIR_OFFSET_MASK);
> +		else
> +			msi->msiir_offset = msiir.start &  
> MSIIR_OFFSET_MASK;
>  	}
> 
>  	msi->feature = features->fsl_pic_ip;
> 
> +	if (of_device_is_compatible(dev->dev.of_node,  
> "fsl,mpic-msi-v4.3")) {
> +		msi->srs_shift = MSIIR1_SRS_SHIFT;
> +		msi->ibs_shift = MSIIR1_IBS_SHIFT;
> +
> +	} else {
> +		msi->srs_shift = MSIIR_SRS_SHIFT;
> +		msi->ibs_shift = MSIIR_IBS_SHIFT;
> +	}

Remove the blank line just before the "} else {".

> diff --git a/arch/powerpc/sysdev/fsl_msi.h  
> b/arch/powerpc/sysdev/fsl_msi.h
> index 8225f86..43a9d99 100644
> --- a/arch/powerpc/sysdev/fsl_msi.h
> +++ b/arch/powerpc/sysdev/fsl_msi.h
> @@ -16,7 +16,7 @@
>  #include <linux/of.h>
>  #include <asm/msi_bitmap.h>
> 
> -#define NR_MSI_REG		8
> +#define NR_MSI_REG		16
>  #define IRQS_PER_MSI_REG	32
>  #define NR_MSI_IRQS	(NR_MSI_REG * IRQS_PER_MSI_REG)

I don't see where you update all_avail in fsl_of_msi_probe.

We should also be bounds-checking the contents of msi-available-ranges.
Currently it looks like we just silently overrun the bitmap if we get  
bad
input from the device tree.

-Scott
Lian Minghaun-b31939 - June 17, 2013, 3 a.m.
Hi Scott,

please see my comments inline.

On 06/15/2013 06:09 AM, Scott Wood wrote:
> On 06/14/2013 02:15:56 AM, Minghuan Lian wrote:
>> @@ -421,10 +440,29 @@ static int fsl_of_msi_probe(struct 
>> platform_device *dev)
>>          }
>>          msi->msiir_offset =
>>              features->msiir_offset + (res.start & 0xfffff);
>> +
>> +        /*
>> +         * First read the MSIIR/MSIIR1 offset from dts
>> +         * If failure use the hardcode MSIIR offset
>
> "On failure"
>
[Minghuan] OK, thanks.
>> +         */
>> +        if (of_address_to_resource(dev->dev.of_node, 1, &msiir))
>> +            msi->msiir_offset = features->msiir_offset +
>> +                        (res.start & MSIIR_OFFSET_MASK);
>> +        else
>> +            msi->msiir_offset = msiir.start & MSIIR_OFFSET_MASK;
>>      }
>>
>>      msi->feature = features->fsl_pic_ip;
>>
>> +    if (of_device_is_compatible(dev->dev.of_node, 
>> "fsl,mpic-msi-v4.3")) {
>> +        msi->srs_shift = MSIIR1_SRS_SHIFT;
>> +        msi->ibs_shift = MSIIR1_IBS_SHIFT;
>> +
>> +    } else {
>> +        msi->srs_shift = MSIIR_SRS_SHIFT;
>> +        msi->ibs_shift = MSIIR_IBS_SHIFT;
>> +    }
>
> Remove the blank line just before the "} else {".
>
[Minghuan] OK, Thanks.
>> diff --git a/arch/powerpc/sysdev/fsl_msi.h 
>> b/arch/powerpc/sysdev/fsl_msi.h
>> index 8225f86..43a9d99 100644
>> --- a/arch/powerpc/sysdev/fsl_msi.h
>> +++ b/arch/powerpc/sysdev/fsl_msi.h
>> @@ -16,7 +16,7 @@
>>  #include <linux/of.h>
>>  #include <asm/msi_bitmap.h>
>>
>> -#define NR_MSI_REG        8
>> +#define NR_MSI_REG        16
>>  #define IRQS_PER_MSI_REG    32
>>  #define NR_MSI_IRQS    (NR_MSI_REG * IRQS_PER_MSI_REG)
>
> I don't see where you update all_avail in fsl_of_msi_probe.
>
> We should also be bounds-checking the contents of msi-available-ranges.
> Currently it looks like we just silently overrun the bitmap if we get bad
> input from the device tree.
>
[Minghuan] all_avail definition: static const u32 all_avail[] = { 0, 
NR_MSI_IRQS };
When changing NR_MSI_REG to 16, NR_MSI_IRQS has been changed to 16*32, 
and all_avail also is updated.

Before calling fsl_msi_setup_hwirq(), the code has checked 
'msi-available-ranges',  only the interrupts lied in 
'msi-available-ranges' will be initialized by call fsl_msi_setup_hwirq() 
, and the corresponding bitmap will be freed. I moved 
msi_bitmap_free_hwirqs() to fsl_msi_setup_hwirq(), because the code 
would generate different bitmap when using MSIIR or MSIIR1.

> -Scott
Scott Wood - June 18, 2013, 12:15 a.m.
On 06/16/2013 10:00:01 PM, Lian Minghuan-b31939 wrote:
> Hi Scott,
> 
> please see my comments inline.
> 
> On 06/15/2013 06:09 AM, Scott Wood wrote:
>> On 06/14/2013 02:15:56 AM, Minghuan Lian wrote:
>>> diff --git a/arch/powerpc/sysdev/fsl_msi.h  
>>> b/arch/powerpc/sysdev/fsl_msi.h
>>> index 8225f86..43a9d99 100644
>>> --- a/arch/powerpc/sysdev/fsl_msi.h
>>> +++ b/arch/powerpc/sysdev/fsl_msi.h
>>> @@ -16,7 +16,7 @@
>>>  #include <linux/of.h>
>>>  #include <asm/msi_bitmap.h>
>>> 
>>> -#define NR_MSI_REG        8
>>> +#define NR_MSI_REG        16
>>>  #define IRQS_PER_MSI_REG    32
>>>  #define NR_MSI_IRQS    (NR_MSI_REG * IRQS_PER_MSI_REG)
>> 
>> I don't see where you update all_avail in fsl_of_msi_probe.
>> 
>> We should also be bounds-checking the contents of  
>> msi-available-ranges.
>> Currently it looks like we just silently overrun the bitmap if we  
>> get bad
>> input from the device tree.
>> 
> [Minghuan] all_avail definition: static const u32 all_avail[] = { 0,  
> NR_MSI_IRQS };
> When changing NR_MSI_REG to 16, NR_MSI_IRQS has been changed to  
> 16*32, and all_avail also is updated.

That's my point.  It shouldn't change for older hardware.

> Before calling fsl_msi_setup_hwirq(), the code has checked  
> 'msi-available-ranges',  only the interrupts lied in  
> 'msi-available-ranges' will be initialized by call  
> fsl_msi_setup_hwirq() , and the corresponding bitmap will be freed. I  
> moved msi_bitmap_free_hwirqs() to fsl_msi_setup_hwirq(), because the  
> code would generate different bitmap when using MSIIR or MSIIR1.

And what happens if msi-available-ranges is bad, and refers to  
non-existent MSIs past the end of the bitmap?

-Scott
Lian Minghaun-b31939 - June 18, 2013, 2:34 a.m.
Hi Soctt,

please see my comments inline.

On 06/18/2013 08:15 AM, Scott Wood wrote:
> On 06/16/2013 10:00:01 PM, Lian Minghuan-b31939 wrote:
>> Hi Scott,
>>
>> please see my comments inline.
>>
>> On 06/15/2013 06:09 AM, Scott Wood wrote:
>>> On 06/14/2013 02:15:56 AM, Minghuan Lian wrote:
>>>> diff --git a/arch/powerpc/sysdev/fsl_msi.h 
>>>> b/arch/powerpc/sysdev/fsl_msi.h
>>>> index 8225f86..43a9d99 100644
>>>> --- a/arch/powerpc/sysdev/fsl_msi.h
>>>> +++ b/arch/powerpc/sysdev/fsl_msi.h
>>>> @@ -16,7 +16,7 @@
>>>>  #include <linux/of.h>
>>>>  #include <asm/msi_bitmap.h>
>>>>
>>>> -#define NR_MSI_REG        8
>>>> +#define NR_MSI_REG        16
>>>>  #define IRQS_PER_MSI_REG    32
>>>>  #define NR_MSI_IRQS    (NR_MSI_REG * IRQS_PER_MSI_REG)
>>>
>>> I don't see where you update all_avail in fsl_of_msi_probe.
>>>
>>> We should also be bounds-checking the contents of msi-available-ranges.
>>> Currently it looks like we just silently overrun the bitmap if we 
>>> get bad
>>> input from the device tree.
>>>
>> [Minghuan] all_avail definition: static const u32 all_avail[] = { 0, 
>> NR_MSI_IRQS };
>> When changing NR_MSI_REG to 16, NR_MSI_IRQS has been changed to 
>> 16*32, and all_avail also is updated.
>
> That's my point.  It shouldn't change for older hardware.
[Minghaun] the older hardware has 8 registers, mipcv4.3 has 16 
registers. If we do not use 16*32 bitmap to indicate 8*32 irqs.(this way 
just only wastes some memory and has no other harm)
we have two choice I think.
1. Use a variable assigned value 8 or 16 based on compatible, then 
dynamically create bitmap
2. Add a new file for mpic v4.3.
What do you think?
>
>> Before calling fsl_msi_setup_hwirq(), the code has checked 
>> 'msi-available-ranges',  only the interrupts lied in 
>> 'msi-available-ranges' will be initialized by call 
>> fsl_msi_setup_hwirq() , and the corresponding bitmap will be freed. I 
>> moved msi_bitmap_free_hwirqs() to fsl_msi_setup_hwirq(), because the 
>> code would generate different bitmap when using MSIIR or MSIIR1.
>
> And what happens if msi-available-ranges is bad, and refers to 
> non-existent MSIs past the end of the bitmap?
[Minghuan] If msi-available-ranges is bad,  the below code will get error.
virt_msir = irq_of_parse_and_map(dev->dev.of_node, irq_index);
And the related error will be printed out and fsl_msi_setup_hwirq() will 
return error directly. There is no chance to set non-existent MSIs past 
the end of the bitmap.

>
> -Scott

Patch

diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c
index ab02db3..34510b7 100644
--- a/arch/powerpc/sysdev/fsl_msi.c
+++ b/arch/powerpc/sysdev/fsl_msi.c
@@ -28,6 +28,18 @@ 
 #include "fsl_msi.h"
 #include "fsl_pci.h"
 
+#define MSIIR_OFFSET_MASK	0xfffff
+#define MSIIR_IBS_SHIFT		0
+#define MSIIR_SRS_SHIFT		5
+#define MSIIR1_IBS_SHIFT	4
+#define MSIIR1_SRS_SHIFT	0
+#define MSI_SRS_MASK		0xf
+#define MSI_IBS_MASK		0x1f
+
+#define msi_hwirq(msi, msir_index, intr_index) \
+		((msir_index) << (msi)->srs_shift | \
+		 ((intr_index) << (msi)->ibs_shift))
+
 static LIST_HEAD(msi_head);
 
 struct fsl_msi_feature {
@@ -80,18 +92,19 @@  static const struct irq_domain_ops fsl_msi_host_ops = {
 
 static int fsl_msi_init_allocator(struct fsl_msi *msi_data)
 {
-	int rc;
+	int rc, hwirq;
 
 	rc = msi_bitmap_alloc(&msi_data->bitmap, NR_MSI_IRQS,
 			      msi_data->irqhost->of_node);
 	if (rc)
 		return rc;
 
-	rc = msi_bitmap_reserve_dt_hwirqs(&msi_data->bitmap);
-	if (rc < 0) {
-		msi_bitmap_free(&msi_data->bitmap);
-		return rc;
-	}
+	/*
+	 * Reserve all the hwirqs
+	 * The available hwirqs will be released in fsl_msi_setup_hwirq()
+	 */
+	for (hwirq = 0; hwirq < NR_MSI_IRQS; hwirq++)
+		msi_bitmap_reserve_hwirq(&msi_data->bitmap, hwirq);
 
 	return 0;
 }
@@ -144,8 +157,9 @@  static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq,
 
 	msg->data = hwirq;
 
-	pr_debug("%s: allocated srs: %d, ibs: %d\n",
-		__func__, hwirq / IRQS_PER_MSI_REG, hwirq % IRQS_PER_MSI_REG);
+	pr_debug("%s: allocated srs: %d, ibs: %d\n", __func__,
+		 (hwirq >> msi_data->srs_shift) & MSI_SRS_MASK,
+		 (hwirq >> msi_data->ibs_shift) & MSI_IBS_MASK);
 }
 
 static int fsl_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
@@ -285,8 +299,8 @@  static void fsl_msi_cascade(unsigned int irq, struct irq_desc *desc)
 		intr_index = ffs(msir_value) - 1;
 
 		cascade_irq = irq_linear_revmap(msi_data->irqhost,
-				msir_index * IRQS_PER_MSI_REG +
-					intr_index + have_shift);
+				msi_hwirq(msi_data, msir_index,
+					  intr_index + have_shift));
 		if (cascade_irq != NO_IRQ)
 			generic_handle_irq(cascade_irq);
 		have_shift += intr_index + 1;
@@ -339,7 +353,7 @@  static int 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;
+	int virt_msir, i;
 
 	virt_msir = irq_of_parse_and_map(dev->dev.of_node, irq_index);
 	if (virt_msir == NO_IRQ) {
@@ -360,6 +374,11 @@  static int fsl_msi_setup_hwirq(struct fsl_msi *msi, struct platform_device *dev,
 	irq_set_handler_data(virt_msir, cascade_data);
 	irq_set_chained_handler(virt_msir, fsl_msi_cascade);
 
+	/* Release the hwirqs corresponding to this MSI register */
+	for (i = 0; i < IRQS_PER_MSI_REG; i++)
+		msi_bitmap_free_hwirqs(&msi->bitmap,
+				       msi_hwirq(msi, offset, i), 1);
+
 	return 0;
 }
 
@@ -368,7 +387,7 @@  static int fsl_of_msi_probe(struct platform_device *dev)
 {
 	const struct of_device_id *match;
 	struct fsl_msi *msi;
-	struct resource res;
+	struct resource res, msiir;
 	int err, i, j, irq_index, count;
 	int rc;
 	const u32 *p;
@@ -421,10 +440,29 @@  static int fsl_of_msi_probe(struct platform_device *dev)
 		}
 		msi->msiir_offset =
 			features->msiir_offset + (res.start & 0xfffff);
+
+		/*
+		 * First read the MSIIR/MSIIR1 offset from dts
+		 * If failure use the hardcode MSIIR offset
+		 */
+		if (of_address_to_resource(dev->dev.of_node, 1, &msiir))
+			msi->msiir_offset = features->msiir_offset +
+					    (res.start & MSIIR_OFFSET_MASK);
+		else
+			msi->msiir_offset = msiir.start & MSIIR_OFFSET_MASK;
 	}
 
 	msi->feature = features->fsl_pic_ip;
 
+	if (of_device_is_compatible(dev->dev.of_node, "fsl,mpic-msi-v4.3")) {
+		msi->srs_shift = MSIIR1_SRS_SHIFT;
+		msi->ibs_shift = MSIIR1_IBS_SHIFT;
+
+	} else {
+		msi->srs_shift = MSIIR_SRS_SHIFT;
+		msi->ibs_shift = MSIIR_IBS_SHIFT;
+	}
+
 	/*
 	 * Remember the phandle, so that we can match with any PCI nodes
 	 * that have an "fsl,msi" property.
diff --git a/arch/powerpc/sysdev/fsl_msi.h b/arch/powerpc/sysdev/fsl_msi.h
index 8225f86..43a9d99 100644
--- a/arch/powerpc/sysdev/fsl_msi.h
+++ b/arch/powerpc/sysdev/fsl_msi.h
@@ -16,7 +16,7 @@ 
 #include <linux/of.h>
 #include <asm/msi_bitmap.h>
 
-#define NR_MSI_REG		8
+#define NR_MSI_REG		16
 #define IRQS_PER_MSI_REG	32
 #define NR_MSI_IRQS	(NR_MSI_REG * IRQS_PER_MSI_REG)
 
@@ -31,6 +31,8 @@  struct fsl_msi {
 	unsigned long cascade_irq;
 
 	u32 msiir_offset; /* Offset of MSIIR, relative to start of CCSR */
+	u32 ibs_shift; /* Shift of interrupt bit select */
+	u32 srs_shift; /* Shift of the shared interrupt register select */
 	void __iomem *msi_regs;
 	u32 feature;
 	int msi_virqs[NR_MSI_REG];