* [PATCH 2/5] powerpc/fsl_msi: add MSIIR1 support for MPIC v4.3
2013-06-14 7:15 [PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node Minghuan Lian
@ 2013-06-14 7:15 ` Minghuan Lian
2013-06-14 22:09 ` Scott Wood
2013-06-14 7:15 ` [PATCH 3/5] powerpc/dts: update MSI bindings doc " Minghuan Lian
` (3 subsequent siblings)
4 siblings, 1 reply; 28+ messages in thread
From: Minghuan Lian @ 2013-06-14 7:15 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Scott Wood, Minghuan Lian, Zang Roy-R61911
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(-)
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];
--
1.8.1.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 2/5] powerpc/fsl_msi: add MSIIR1 support for MPIC v4.3
2013-06-14 7:15 ` [PATCH 2/5] powerpc/fsl_msi: add MSIIR1 support for MPIC v4.3 Minghuan Lian
@ 2013-06-14 22:09 ` Scott Wood
2013-06-17 3:00 ` Lian Minghuan-b31939
0 siblings, 1 reply; 28+ messages in thread
From: Scott Wood @ 2013-06-14 22:09 UTC (permalink / raw)
To: Minghuan Lian; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
On 06/14/2013 02:15:56 AM, Minghuan Lian wrote:
> @@ -421,10 +440,29 @@ static int fsl_of_msi_probe(struct =20
> platform_device *dev)
> }
> msi->msiir_offset =3D
> 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 =3D features->msiir_offset +
> + (res.start & =20
> MSIIR_OFFSET_MASK);
> + else
> + msi->msiir_offset =3D msiir.start & =20
> MSIIR_OFFSET_MASK;
> }
>=20
> msi->feature =3D features->fsl_pic_ip;
>=20
> + if (of_device_is_compatible(dev->dev.of_node, =20
> "fsl,mpic-msi-v4.3")) {
> + msi->srs_shift =3D MSIIR1_SRS_SHIFT;
> + msi->ibs_shift =3D MSIIR1_IBS_SHIFT;
> +
> + } else {
> + msi->srs_shift =3D MSIIR_SRS_SHIFT;
> + msi->ibs_shift =3D MSIIR_IBS_SHIFT;
> + }
Remove the blank line just before the "} else {".
> diff --git a/arch/powerpc/sysdev/fsl_msi.h =20
> 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>
>=20
> -#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 =20
bad
input from the device tree.
-Scott=
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 2/5] powerpc/fsl_msi: add MSIIR1 support for MPIC v4.3
2013-06-14 22:09 ` Scott Wood
@ 2013-06-17 3:00 ` Lian Minghuan-b31939
2013-06-18 0:15 ` Scott Wood
0 siblings, 1 reply; 28+ messages in thread
From: Lian Minghuan-b31939 @ 2013-06-17 3:00 UTC (permalink / raw)
To: Scott Wood; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
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
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 2/5] powerpc/fsl_msi: add MSIIR1 support for MPIC v4.3
2013-06-17 3:00 ` Lian Minghuan-b31939
@ 2013-06-18 0:15 ` Scott Wood
2013-06-18 2:34 ` Lian Minghuan-b31939
0 siblings, 1 reply; 28+ messages in thread
From: Scott Wood @ 2013-06-18 0:15 UTC (permalink / raw)
To: Lian Minghuan-b31939; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
On 06/16/2013 10:00:01 PM, Lian Minghuan-b31939 wrote:
> Hi Scott,
>=20
> please see my comments inline.
>=20
> 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 =20
>>> 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>
>>>=20
>>> -#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)
>>=20
>> I don't see where you update all_avail in fsl_of_msi_probe.
>>=20
>> We should also be bounds-checking the contents of =20
>> msi-available-ranges.
>> Currently it looks like we just silently overrun the bitmap if we =20
>> get bad
>> input from the device tree.
>>=20
> [Minghuan] all_avail definition: static const u32 all_avail[] =3D { 0, =20
> NR_MSI_IRQS };
> When changing NR_MSI_REG to 16, NR_MSI_IRQS has been changed to =20
> 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 =20
> 'msi-available-ranges', only the interrupts lied in =20
> 'msi-available-ranges' will be initialized by call =20
> fsl_msi_setup_hwirq() , and the corresponding bitmap will be freed. I =20
> moved msi_bitmap_free_hwirqs() to fsl_msi_setup_hwirq(), because the =20
> code would generate different bitmap when using MSIIR or MSIIR1.
And what happens if msi-available-ranges is bad, and refers to =20
non-existent MSIs past the end of the bitmap?
-Scott=
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 2/5] powerpc/fsl_msi: add MSIIR1 support for MPIC v4.3
2013-06-18 0:15 ` Scott Wood
@ 2013-06-18 2:34 ` Lian Minghuan-b31939
2013-06-18 18:08 ` Scott Wood
0 siblings, 1 reply; 28+ messages in thread
From: Lian Minghuan-b31939 @ 2013-06-18 2:34 UTC (permalink / raw)
To: Scott Wood; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
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
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 2/5] powerpc/fsl_msi: add MSIIR1 support for MPIC v4.3
2013-06-18 2:34 ` Lian Minghuan-b31939
@ 2013-06-18 18:08 ` Scott Wood
0 siblings, 0 replies; 28+ messages in thread
From: Scott Wood @ 2013-06-18 18:08 UTC (permalink / raw)
To: Lian Minghuan-b31939; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
On 06/17/2013 09:34:49 PM, Lian Minghuan-b31939 wrote:
> Hi Soctt,
>=20
> please see my comments inline.
>=20
> On 06/18/2013 08:15 AM, Scott Wood wrote:
>> On 06/16/2013 10:00:01 PM, Lian Minghuan-b31939 wrote:
>>> Hi Scott,
>>>=20
>>> please see my comments inline.
>>>=20
>>> 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 =20
>>>>> 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>
>>>>>=20
>>>>> -#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)
>>>>=20
>>>> I don't see where you update all_avail in fsl_of_msi_probe.
>>>>=20
>>>> We should also be bounds-checking the contents of =20
>>>> msi-available-ranges.
>>>> Currently it looks like we just silently overrun the bitmap if we =20
>>>> get bad
>>>> input from the device tree.
>>>>=20
>>> [Minghuan] all_avail definition: static const u32 all_avail[] =3D { =20
>>> 0, NR_MSI_IRQS };
>>> When changing NR_MSI_REG to 16, NR_MSI_IRQS has been changed to =20
>>> 16*32, and all_avail also is updated.
>>=20
>> That's my point. It shouldn't change for older hardware.
> [Minghaun] the older hardware has 8 registers, mipcv4.3 has 16 =20
> registers. If we do not use 16*32 bitmap to indicate 8*32 irqs.(this =20
> way just only wastes some memory and has no other harm)
Using the larger bitmap unconditionally is fine. What is not fine is, =20
on older hardware, acting as if all 16 irqs are present.
In other words, I'm talking about the contents of the bitmap, not its =20
size.
> we have two choice I think.
> 1. Use a variable assigned value 8 or 16 based on compatible, then =20
> dynamically create bitmap
If we have the mpic4.3 compatible, then we don't even support =20
msi-available-ranges, so we'd skip this code and free everything in the =20
bitmap.
> 2. Add a new file for mpic v4.3.
No. :-)
-Scott=
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 3/5] powerpc/dts: update MSI bindings doc for MPIC v4.3
2013-06-14 7:15 [PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node Minghuan Lian
2013-06-14 7:15 ` [PATCH 2/5] powerpc/fsl_msi: add MSIIR1 support for MPIC v4.3 Minghuan Lian
@ 2013-06-14 7:15 ` Minghuan Lian
2013-06-14 22:06 ` Scott Wood
2013-06-14 7:15 ` [PATCH 4/5] powerpc/dts: remove msi-available-ranges property Minghuan Lian
` (2 subsequent siblings)
4 siblings, 1 reply; 28+ messages in thread
From: Minghuan Lian @ 2013-06-14 7:15 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Scott Wood, Minghuan Lian, Zang Roy-R61911
Add compatible "fsl,mpic-msi-v4.3" for MPIC v4.3. MPIC v4.3 contains
MSIIR and MSIIR1. MSIIR supports 8 MSI registers and MSIIR1 supports
16 MSI registers, but uses different IBS and SRS shift. When using
MSIR1, the interrupt number is not consecutive. It is hard to use
'msi-available-ranges' to describe the ranges of the available
interrupt and the ranges are related to the application, rather than
the description of the hardware. this patch also removes
'msi-available-ranges' property.
Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
---
.../devicetree/bindings/powerpc/fsl/msi-pic.txt | 49 ++++++++++------------
1 file changed, 22 insertions(+), 27 deletions(-)
diff --git a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
index 5693877..e851e93 100644
--- a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
+++ b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
@@ -1,26 +1,23 @@
* Freescale MSI interrupt controller
Required properties:
-- compatible : compatible list, contains 2 entries,
+- compatible : compatible list, may contains one or two entries,
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.
+ etc.) and the second is "fsl,mpic-msi" or "fsl,ipic-msi" or
+ "fsl,mpic-msi-v4.3" depending on the parent type and version. If mpic
+ version is 4.3, the number of MSI registers is increased to 16, MSIIR1 is
+ provided to access these 16 registers, compatible "fsl,mpic-msi-v4.3"
+ should be used.
- reg : It may contain one or two regions. The first region should contain
the address and the length of the shared message interrupt register set.
- The second region should contain the address of aliased MSIIR register for
- platforms that have such an alias.
-
-- 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).
+ The second region should contain the address of aliased MSIIR or MSIIR1
+ register for platforms that have such an alias, if using MSIIR1, the second
+ region must be added because different MSI group has different MSIRR1 offset.
- 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. If msi-available-ranges is present, only
- the interrupts that correspond to available ranges shall be present.
+ be set as edge sensitive.
- interrupt-parent: the phandle for the interrupt controller
that services interrupts for this device. for 83xx cpu, the interrupts
@@ -39,20 +36,18 @@ Optional properties:
Example:
msi@41600 {
- compatible = "fsl,mpc8610-msi", "fsl,mpic-msi";
- reg = <0x41600 0x80>;
- msi-available-ranges = <0 0x100>;
- interrupts = <
- 0xe0 0
- 0xe1 0
- 0xe2 0
- 0xe3 0
- 0xe4 0
- 0xe5 0
- 0xe6 0
- 0xe7 0>;
- interrupt-parent = <&mpic>;
- };
+ compatible = "fsl,mpic-msi";
+ reg = <0x41600 0x200 0x44140 4>;
+ interrupts = <
+ 0xe0 0 0 0
+ 0xe1 0 0 0
+ 0xe2 0 0 0
+ 0xe3 0 0 0
+ 0xe4 0 0 0
+ 0xe5 0 0 0
+ 0xe6 0 0 0
+ 0xe7 0 0 0>;
+};
The Freescale hypervisor and msi-address-64
-------------------------------------------
--
1.8.1.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 3/5] powerpc/dts: update MSI bindings doc for MPIC v4.3
2013-06-14 7:15 ` [PATCH 3/5] powerpc/dts: update MSI bindings doc " Minghuan Lian
@ 2013-06-14 22:06 ` Scott Wood
2013-06-17 5:07 ` Lian Minghuan-b31939
0 siblings, 1 reply; 28+ messages in thread
From: Scott Wood @ 2013-06-14 22:06 UTC (permalink / raw)
To: Minghuan Lian; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
On 06/14/2013 02:15:57 AM, Minghuan Lian wrote:
> Add compatible "fsl,mpic-msi-v4.3" for MPIC v4.3. MPIC v4.3 contains
> MSIIR and MSIIR1. MSIIR supports 8 MSI registers and MSIIR1 supports
> 16 MSI registers, but uses different IBS and SRS shift. When using
> MSIR1, the interrupt number is not consecutive. It is hard to use
> 'msi-available-ranges' to describe the ranges of the available
> interrupt and the ranges are related to the application, rather than
> the description of the hardware. this patch also removes
> 'msi-available-ranges' property.
>=20
> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
> ---
> .../devicetree/bindings/powerpc/fsl/msi-pic.txt | 49 =20
> ++++++++++------------
> 1 file changed, 22 insertions(+), 27 deletions(-)
>=20
> diff --git =20
> a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt =20
> b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
> index 5693877..e851e93 100644
> --- a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
> +++ b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
> @@ -1,26 +1,23 @@
> * Freescale MSI interrupt controller
>=20
> Required properties:
> -- compatible : compatible list, contains 2 entries,
> +- compatible : compatible list, may contains one or two entries,
> first is "fsl,CHIP-msi", where CHIP is the processor(mpc8610, =20
> mpc8572,
> - etc.) and the second is "fsl,mpic-msi" or "fsl,ipic-msi" depending =20
> on
> - the parent type.
> + etc.) and the second is "fsl,mpic-msi" or "fsl,ipic-msi" or
> + "fsl,mpic-msi-v4.3" depending on the parent type and version. If =20
> mpic
> + version is 4.3, the number of MSI registers is increased to 16, =20
> MSIIR1 is
> + provided to access these 16 registers, compatible =20
> "fsl,mpic-msi-v4.3"
> + should be used.
Why "one or two"? What does it look like in the case where there's =20
just one?
> - reg : It may contain one or two regions. The first region should =20
> contain
> the address and the length of the shared message interrupt =20
> register set.
> - The second region should contain the address of aliased MSIIR =20
> register for
> - platforms that have such an alias.
> -
> -- msi-available-ranges: use <start count> style section to define =20
> which
> - msi interrupt can be used in the 256 msi interrupts. This property =20
> 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 =20
> interrupt).
> + The second region should contain the address of aliased MSIIR or =20
> MSIIR1
> + register for platforms that have such an alias, if using MSIIR1, =20
> the second
> + region must be added because different MSI group has different =20
> MSIRR1 offset.
Why are you removing msi-available-ranges? It's not valid for MPIC =20
v4.3, but it's still valid for older MPICs. It should move to the =20
optional section, though.
> - interrupts : each one of the interrupts here is one entry per 32 =20
> MSIs,
> and routed to the host interrupt controller. the interrupts should
> - be set as edge sensitive. If msi-available-ranges is present, only
> - the interrupts that correspond to available ranges shall be =20
> present.
> + be set as edge sensitive.
>=20
> - interrupt-parent: the phandle for the interrupt controller
> that services interrupts for this device. for 83xx cpu, the =20
> interrupts
> @@ -39,20 +36,18 @@ Optional properties:
>=20
> Example:
> msi@41600 {
> - compatible =3D "fsl,mpc8610-msi", "fsl,mpic-msi";
> - reg =3D <0x41600 0x80>;
> - msi-available-ranges =3D <0 0x100>;
> - interrupts =3D <
> - 0xe0 0
> - 0xe1 0
> - 0xe2 0
> - 0xe3 0
> - 0xe4 0
> - 0xe5 0
> - 0xe6 0
> - 0xe7 0>;
> - interrupt-parent =3D <&mpic>;
> - };
> + compatible =3D "fsl,mpic-msi";
> + reg =3D <0x41600 0x200 0x44140 4>;
Why 0x200?
-Scott=
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 3/5] powerpc/dts: update MSI bindings doc for MPIC v4.3
2013-06-14 22:06 ` Scott Wood
@ 2013-06-17 5:07 ` Lian Minghuan-b31939
2013-06-18 0:28 ` Scott Wood
0 siblings, 1 reply; 28+ messages in thread
From: Lian Minghuan-b31939 @ 2013-06-17 5:07 UTC (permalink / raw)
To: Scott Wood; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
Hi Soctt,
please see my comments.
On 06/15/2013 06:06 AM, Scott Wood wrote:
> On 06/14/2013 02:15:57 AM, Minghuan Lian wrote:
>> Add compatible "fsl,mpic-msi-v4.3" for MPIC v4.3. MPIC v4.3 contains
>> MSIIR and MSIIR1. MSIIR supports 8 MSI registers and MSIIR1 supports
>> 16 MSI registers, but uses different IBS and SRS shift. When using
>> MSIR1, the interrupt number is not consecutive. It is hard to use
>> 'msi-available-ranges' to describe the ranges of the available
>> interrupt and the ranges are related to the application, rather than
>> the description of the hardware. this patch also removes
>> 'msi-available-ranges' property.
>>
>> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
>> ---
>> .../devicetree/bindings/powerpc/fsl/msi-pic.txt | 49
>> ++++++++++------------
>> 1 file changed, 22 insertions(+), 27 deletions(-)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
>> b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
>> index 5693877..e851e93 100644
>> --- a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
>> +++ b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
>> @@ -1,26 +1,23 @@
>> * Freescale MSI interrupt controller
>>
>> Required properties:
>> -- compatible : compatible list, contains 2 entries,
>> +- compatible : compatible list, may contains one or two entries,
>> 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.
>> + etc.) and the second is "fsl,mpic-msi" or "fsl,ipic-msi" or
>> + "fsl,mpic-msi-v4.3" depending on the parent type and version. If mpic
>> + version is 4.3, the number of MSI registers is increased to 16,
>> MSIIR1 is
>> + provided to access these 16 registers, compatible "fsl,mpic-msi-v4.3"
>> + should be used.
>
> Why "one or two"? What does it look like in the case where there's
> just one?
>
[Minghuan] The original doc said 'contains 2 entries', but I notcie
pq3-mpic.dtsi and qoriq-mpic.dtsi have only one entry "fsl,mpic-msi", do
not have "fsl,CHIP-msi".
for example:
mpc8610_hpcd.dts: compatible = "fsl,mpc8610-msi", "fsl,mpic-msi";
fsl/qoriq-mpic.dtsi: compatible = "fsl,mpic-msi"
Maybe I should say " For some platforms, "fsl,CHIP-msi' is optional."
>> - reg : It may contain one or two regions. The first region should
>> contain
>> the address and the length of the shared message interrupt
>> register set.
>> - The second region should contain the address of aliased MSIIR
>> register for
>> - platforms that have such an alias.
>> -
>> -- 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).
>> + The second region should contain the address of aliased MSIIR or
>> MSIIR1
>> + register for platforms that have such an alias, if using MSIIR1,
>> the second
>> + region must be added because different MSI group has different
>> MSIRR1 offset.
>
> Why are you removing msi-available-ranges? It's not valid for MPIC
> v4.3, but it's still valid for older MPICs. It should move to the
> optional section, though.
[Minghuan] Because I would like to add kernel parameter 'msiregs'
instead of "msi-available-ranges", for all the MPICs, we will have a
uniform way to configure
For older MPICs, we can use "msi-available-ranges" and "msiregs", but
for MPCI4.3 we can only use "msiregs", this may easily lead to confusion.
>> - 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. If msi-available-ranges is present, only
>> - the interrupts that correspond to available ranges shall be present.
>> + be set as edge sensitive.
>>
>> - interrupt-parent: the phandle for the interrupt controller
>> that services interrupts for this device. for 83xx cpu, the
>> interrupts
>> @@ -39,20 +36,18 @@ Optional properties:
>>
>> Example:
>> msi@41600 {
>> - compatible = "fsl,mpc8610-msi", "fsl,mpic-msi";
>> - reg = <0x41600 0x80>;
>> - msi-available-ranges = <0 0x100>;
>> - interrupts = <
>> - 0xe0 0
>> - 0xe1 0
>> - 0xe2 0
>> - 0xe3 0
>> - 0xe4 0
>> - 0xe5 0
>> - 0xe6 0
>> - 0xe7 0>;
>> - interrupt-parent = <&mpic>;
>> - };
>> + compatible = "fsl,mpic-msi";
>> + reg = <0x41600 0x200 0x44140 4>;
>
> Why 0x200?
>
[Minghuan] The offsets of the MSIA registers are from 0x41600 to
0x417ff, and the size is 0x200.
offset 0x41600-0x4170 are MSIIRA1-7.
0x41720 is MSISRA,
0x41750 is MSIIR.
The others are reserved.
-Scott
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 3/5] powerpc/dts: update MSI bindings doc for MPIC v4.3
2013-06-17 5:07 ` Lian Minghuan-b31939
@ 2013-06-18 0:28 ` Scott Wood
2013-06-18 0:42 ` Scott Wood
0 siblings, 1 reply; 28+ messages in thread
From: Scott Wood @ 2013-06-18 0:28 UTC (permalink / raw)
To: Lian Minghuan-b31939; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
On 06/17/2013 12:07:41 AM, Lian Minghuan-b31939 wrote:
> Hi Soctt,
>=20
> please see my comments.
>=20
> On 06/15/2013 06:06 AM, Scott Wood wrote:
>> On 06/14/2013 02:15:57 AM, Minghuan Lian wrote:
>>> Add compatible "fsl,mpic-msi-v4.3" for MPIC v4.3. MPIC v4.3 contains
>>> MSIIR and MSIIR1. MSIIR supports 8 MSI registers and MSIIR1 supports
>>> 16 MSI registers, but uses different IBS and SRS shift. When using
>>> MSIR1, the interrupt number is not consecutive. It is hard to use
>>> 'msi-available-ranges' to describe the ranges of the available
>>> interrupt and the ranges are related to the application, rather than
>>> the description of the hardware. this patch also removes
>>> 'msi-available-ranges' property.
>>>=20
>>> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
>>> ---
>>> .../devicetree/bindings/powerpc/fsl/msi-pic.txt | 49 =20
>>> ++++++++++------------
>>> 1 file changed, 22 insertions(+), 27 deletions(-)
>>>=20
>>> diff --git =20
>>> a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt =20
>>> b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
>>> index 5693877..e851e93 100644
>>> --- a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
>>> +++ b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
>>> @@ -1,26 +1,23 @@
>>> * Freescale MSI interrupt controller
>>>=20
>>> Required properties:
>>> -- compatible : compatible list, contains 2 entries,
>>> +- compatible : compatible list, may contains one or two entries,
>>> first is "fsl,CHIP-msi", where CHIP is the processor(mpc8610, =20
>>> mpc8572,
>>> - etc.) and the second is "fsl,mpic-msi" or "fsl,ipic-msi" =20
>>> depending on
>>> - the parent type.
>>> + etc.) and the second is "fsl,mpic-msi" or "fsl,ipic-msi" or
>>> + "fsl,mpic-msi-v4.3" depending on the parent type and version. If =20
>>> mpic
>>> + version is 4.3, the number of MSI registers is increased to 16, =20
>>> MSIIR1 is
>>> + provided to access these 16 registers, compatible =20
>>> "fsl,mpic-msi-v4.3"
>>> + should be used.
>>=20
>> Why "one or two"? What does it look like in the case where there's =20
>> just one?
>>=20
> [Minghuan] The original doc said 'contains 2 entries', but I notcie =20
> pq3-mpic.dtsi and qoriq-mpic.dtsi have only one entry "fsl,mpic-msi", =20
> do not have "fsl,CHIP-msi".
> for example:
> mpc8610_hpcd.dts: compatible =3D "fsl,mpc8610-msi", "fsl,mpic-msi";
> fsl/qoriq-mpic.dtsi: compatible =3D "fsl,mpic-msi"
>=20
> Maybe I should say " For some platforms, "fsl,CHIP-msi' is optional."
Well, this is more a matter of some device trees not complying with the =20
binding, rather than an update for MPIC v4.3.
In any case, if the plan is to update the binding to match what we've =20
been doing in the actual trees, at least word it so that it's clear =20
which one of the two is optional.
>> Why are you removing msi-available-ranges? It's not valid for MPIC =20
>> v4.3, but it's still valid for older MPICs. It should move to the =20
>> optional section, though.
> [Minghuan] Because I would like to add kernel parameter 'msiregs' =20
> instead of "msi-available-ranges", for all the MPICs, we will have a =20
> uniform way to configure
I've responded elsewhere to this, but I'd also like to add that we =20
don't break compatibility with older device tree bindings just for "a =20
uniform way".
>>> Example:
>>> msi@41600 {
>>> - compatible =3D "fsl,mpc8610-msi", "fsl,mpic-msi";
>>> - reg =3D <0x41600 0x80>;
>>> - msi-available-ranges =3D <0 0x100>;
>>> - interrupts =3D <
>>> - 0xe0 0
>>> - 0xe1 0
>>> - 0xe2 0
>>> - 0xe3 0
>>> - 0xe4 0
>>> - 0xe5 0
>>> - 0xe6 0
>>> - 0xe7 0>;
>>> - interrupt-parent =3D <&mpic>;
>>> - };
>>> + compatible =3D "fsl,mpic-msi";
>>> + reg =3D <0x41600 0x200 0x44140 4>;
>>=20
>> Why 0x200?
>>=20
> [Minghuan] The offsets of the MSIA registers are from 0x41600 to =20
> 0x417ff, and the size is 0x200.
> offset 0x41600-0x4170 are MSIIRA1-7.
> 0x41720 is MSISRA,
> 0x41750 is MSIIR.
> The others are reserved.
There is no MSIIRA on fsl,mpic-msi.
If you want to show an fsl,mpic-msi-v4.3 example, update the compatible =20
and add the extra 8 interrupts. We should probably show an example of =20
each.
BTW, why are you changing/breaking the whitespace in the example?
-Scott=
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 3/5] powerpc/dts: update MSI bindings doc for MPIC v4.3
2013-06-18 0:28 ` Scott Wood
@ 2013-06-18 0:42 ` Scott Wood
2013-06-18 2:49 ` Lian Minghuan-b31939
0 siblings, 1 reply; 28+ messages in thread
From: Scott Wood @ 2013-06-18 0:42 UTC (permalink / raw)
To: Scott Wood
Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911,
Lian Minghuan-b31939
On 06/17/2013 07:28:07 PM, Scott Wood wrote:
> On 06/17/2013 12:07:41 AM, Lian Minghuan-b31939 wrote:
>>>> + compatible =3D "fsl,mpic-msi";
>>>> + reg =3D <0x41600 0x200 0x44140 4>;
>>>=20
>>> Why 0x200?
>>>=20
>> [Minghuan] The offsets of the MSIA registers are from 0x41600 to =20
>> 0x417ff, and the size is 0x200.
>> offset 0x41600-0x4170 are MSIIRA1-7.
>> 0x41720 is MSISRA,
>> 0x41750 is MSIIR.
>> The others are reserved.
>=20
> There is no MSIIRA on fsl,mpic-msi.
Sigh, I was thinking of MSIIR1A -- which of course is distinct from =20
both MSIIRA1 and MSIIRA. :-P
So it's just a bug that pq3-mpic.dtsi has a length of 0x80?
-Scott=
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/5] powerpc/dts: update MSI bindings doc for MPIC v4.3
2013-06-18 0:42 ` Scott Wood
@ 2013-06-18 2:49 ` Lian Minghuan-b31939
2013-06-18 16:21 ` Scott Wood
0 siblings, 1 reply; 28+ messages in thread
From: Lian Minghuan-b31939 @ 2013-06-18 2:49 UTC (permalink / raw)
To: Scott Wood; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
On 06/18/2013 08:42 AM, Scott Wood wrote:
> On 06/17/2013 07:28:07 PM, Scott Wood wrote:
>> On 06/17/2013 12:07:41 AM, Lian Minghuan-b31939 wrote:
>>>>> + compatible = "fsl,mpic-msi";
>>>>> + reg = <0x41600 0x200 0x44140 4>;
>>>>
>>>> Why 0x200?
>>>>
>>> [Minghuan] The offsets of the MSIA registers are from 0x41600 to
>>> 0x417ff, and the size is 0x200.
>>> offset 0x41600-0x4170 are MSIIRA1-7.
>>> 0x41720 is MSISRA,
>>> 0x41750 is MSIIR.
>>> The others are reserved.
>>
>> There is no MSIIRA on fsl,mpic-msi.
>
> Sigh, I was thinking of MSIIR1A -- which of course is distinct from
> both MSIIRA1 and MSIIRA. :-P
>
> So it's just a bug that pq3-mpic.dtsi has a length of 0x80?
[Minghuan] I am sorry, there is a typo.
offset 0x41600-0x4170 should be MSIRA0-7.
The MSI bank size is 0x200.
The MSIR 0-7 size is 0x80.
So the first region of 'reg' should indicate bank size or MSIR size?
I think it should be a bank size. So MSI driver can access MSISR and
MSIIR, and provide some new features in feature.
>
> -Scott
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/5] powerpc/dts: update MSI bindings doc for MPIC v4.3
2013-06-18 2:49 ` Lian Minghuan-b31939
@ 2013-06-18 16:21 ` Scott Wood
0 siblings, 0 replies; 28+ messages in thread
From: Scott Wood @ 2013-06-18 16:21 UTC (permalink / raw)
To: Lian Minghuan-b31939; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
On 06/17/2013 09:49:19 PM, Lian Minghuan-b31939 wrote:
> On 06/18/2013 08:42 AM, Scott Wood wrote:
>> On 06/17/2013 07:28:07 PM, Scott Wood wrote:
>>> On 06/17/2013 12:07:41 AM, Lian Minghuan-b31939 wrote:
>>>>>> + compatible =3D "fsl,mpic-msi";
>>>>>> + reg =3D <0x41600 0x200 0x44140 4>;
>>>>>=20
>>>>> Why 0x200?
>>>>>=20
>>>> [Minghuan] The offsets of the MSIA registers are from 0x41600 to =20
>>>> 0x417ff, and the size is 0x200.
>>>> offset 0x41600-0x4170 are MSIIRA1-7.
>>>> 0x41720 is MSISRA,
>>>> 0x41750 is MSIIR.
>>>> The others are reserved.
>>>=20
>>> There is no MSIIRA on fsl,mpic-msi.
>>=20
>> Sigh, I was thinking of MSIIR1A -- which of course is distinct from =20
>> both MSIIRA1 and MSIIRA. :-P
>>=20
>> So it's just a bug that pq3-mpic.dtsi has a length of 0x80?
> [Minghuan] I am sorry, there is a typo.
> offset 0x41600-0x4170 should be MSIRA0-7.
> The MSI bank size is 0x200.
> The MSIR 0-7 size is 0x80.
> So the first region of 'reg' should indicate bank size or MSIR size?
> I think it should be a bank size. So MSI driver can access MSISR and =20
> MSIIR, and provide some new features in feature.
It should be the bank size. There's already inconsistency between =20
pq3-mpic.dtsi and qoriq-mpic.dtsi (the latter has 0x200). I think =20
pq3-mpic.dtsi is just wrong and should be fixed.
-Scott=
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 4/5] powerpc/dts: remove msi-available-ranges property
2013-06-14 7:15 [PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node Minghuan Lian
2013-06-14 7:15 ` [PATCH 2/5] powerpc/fsl_msi: add MSIIR1 support for MPIC v4.3 Minghuan Lian
2013-06-14 7:15 ` [PATCH 3/5] powerpc/dts: update MSI bindings doc " Minghuan Lian
@ 2013-06-14 7:15 ` Minghuan Lian
2013-06-14 22:10 ` Scott Wood
2013-06-14 7:15 ` [PATCH 5/5] powerpc/fsl_msi: add 'msiregs' kernel parameter Minghuan Lian
2013-06-14 20:39 ` [PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node Scott Wood
4 siblings, 1 reply; 28+ messages in thread
From: Minghuan Lian @ 2013-06-14 7:15 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Scott Wood, Minghuan Lian, Zang Roy-R61911
For MPIC v4.3 MSIIR supports 8 MSI registers and MSIIR1 supports
16 MSI registers, but uses different IBS and SRS shift. For the
first register, when using MSIIR we will get the irqs 0x0 0x1 0x2
...0x1f, but when using MSIIR1, the irqs are 0x0 0x10 0x20 ... 0x1f0
It is hard to describe the available irqs using property
'msi-available-ranges'. The patch removes this property.
Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
---
arch/powerpc/boot/dts/fsl/pq3-mpic.dtsi | 1 -
arch/powerpc/boot/dts/fsl/qoriq-mpic.dtsi | 3 ---
arch/powerpc/boot/dts/fsl/qoriq-mpic4.3.dtsi | 4 ----
3 files changed, 8 deletions(-)
diff --git a/arch/powerpc/boot/dts/fsl/pq3-mpic.dtsi b/arch/powerpc/boot/dts/fsl/pq3-mpic.dtsi
index 71c30eb..1ac4f23 100644
--- a/arch/powerpc/boot/dts/fsl/pq3-mpic.dtsi
+++ b/arch/powerpc/boot/dts/fsl/pq3-mpic.dtsi
@@ -66,7 +66,6 @@ message@41400 {
msi@41600 {
compatible = "fsl,mpic-msi";
reg = <0x41600 0x80>;
- msi-available-ranges = <0 0x100>;
interrupts = <
0xe0 0 0 0
0xe1 0 0 0
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-mpic.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-mpic.dtsi
index 08f4227..cf7355c 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-mpic.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-mpic.dtsi
@@ -54,7 +54,6 @@ timer@41100 {
msi0: msi@41600 {
compatible = "fsl,mpic-msi";
reg = <0x41600 0x200 0x44140 4>;
- msi-available-ranges = <0 0x100>;
interrupts = <
0xe0 0 0 0
0xe1 0 0 0
@@ -69,7 +68,6 @@ msi0: msi@41600 {
msi1: msi@41800 {
compatible = "fsl,mpic-msi";
reg = <0x41800 0x200 0x45140 4>;
- msi-available-ranges = <0 0x100>;
interrupts = <
0xe8 0 0 0
0xe9 0 0 0
@@ -84,7 +82,6 @@ msi1: msi@41800 {
msi2: msi@41a00 {
compatible = "fsl,mpic-msi";
reg = <0x41a00 0x200 0x46140 4>;
- msi-available-ranges = <0 0x100>;
interrupts = <
0xf0 0 0 0
0xf1 0 0 0
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-mpic4.3.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-mpic4.3.dtsi
index e2665b8..8a997ea 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-mpic4.3.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-mpic4.3.dtsi
@@ -54,7 +54,6 @@ timer@41100 {
msi0: msi@41600 {
compatible = "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
reg = <0x41600 0x200 0x44148 4>;
- msi-available-ranges = <0 0x200>;
interrupts = <
0xe0 0 0 0
0xe1 0 0 0
@@ -77,7 +76,6 @@ msi0: msi@41600 {
msi1: msi@41800 {
compatible = "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
reg = <0x41800 0x200 0x45148 4>;
- msi-available-ranges = <0 0x200>;
interrupts = <
0xe8 0 0 0
0xe9 0 0 0
@@ -100,7 +98,6 @@ msi1: msi@41800 {
msi2: msi@41a00 {
compatible = "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
reg = <0x41a00 0x200 0x46148 4>;
- msi-available-ranges = <0 0x200>;
interrupts = <
0xf0 0 0 0
0xf1 0 0 0
@@ -123,7 +120,6 @@ msi2: msi@41a00 {
msi3: msi@41c00 {
compatible = "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
reg = <0x41c00 0x200 0x47148 4>;
- msi-available-ranges = <0 0x200>;
interrupts = <
0xf8 0 0 0
0xf9 0 0 0
--
1.8.1.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 4/5] powerpc/dts: remove msi-available-ranges property
2013-06-14 7:15 ` [PATCH 4/5] powerpc/dts: remove msi-available-ranges property Minghuan Lian
@ 2013-06-14 22:10 ` Scott Wood
2013-06-17 5:15 ` Lian Minghuan-b31939
0 siblings, 1 reply; 28+ messages in thread
From: Scott Wood @ 2013-06-14 22:10 UTC (permalink / raw)
To: Minghuan Lian; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
On 06/14/2013 02:15:58 AM, Minghuan Lian wrote:
> For MPIC v4.3 MSIIR supports 8 MSI registers and MSIIR1 supports
> 16 MSI registers, but uses different IBS and SRS shift. For the
> first register, when using MSIIR we will get the irqs 0x0 0x1 0x2
> ...0x1f, but when using MSIIR1, the irqs are 0x0 0x10 0x20 ... 0x1f0
> It is hard to describe the available irqs using property
> 'msi-available-ranges'. The patch removes this property.
Only remove it from mpic 4.3. And since you introduced =20
qoriq-mpic4.3.dtsi earlier in the patchset, why didn't you just avoid =20
adding it then?
-Scott=
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/5] powerpc/dts: remove msi-available-ranges property
2013-06-14 22:10 ` Scott Wood
@ 2013-06-17 5:15 ` Lian Minghuan-b31939
2013-06-18 0:13 ` Scott Wood
0 siblings, 1 reply; 28+ messages in thread
From: Lian Minghuan-b31939 @ 2013-06-17 5:15 UTC (permalink / raw)
To: Scott Wood; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
On 06/15/2013 06:10 AM, Scott Wood wrote:
> On 06/14/2013 02:15:58 AM, Minghuan Lian wrote:
>> For MPIC v4.3 MSIIR supports 8 MSI registers and MSIIR1 supports
>> 16 MSI registers, but uses different IBS and SRS shift. For the
>> first register, when using MSIIR we will get the irqs 0x0 0x1 0x2
>> ...0x1f, but when using MSIIR1, the irqs are 0x0 0x10 0x20 ... 0x1f0
>> It is hard to describe the available irqs using property
>> 'msi-available-ranges'. The patch removes this property.
>
> Only remove it from mpic 4.3. And since you introduced
> qoriq-mpic4.3.dtsi earlier in the patchset, why didn't you just avoid
> adding it then?
>
[Minghuan] If adding it in qoriq-mpic4.3, and the 3-5 patches are not
accepted, mpic4.3 can also work.
> -Scott
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/5] powerpc/dts: remove msi-available-ranges property
2013-06-17 5:15 ` Lian Minghuan-b31939
@ 2013-06-18 0:13 ` Scott Wood
0 siblings, 0 replies; 28+ messages in thread
From: Scott Wood @ 2013-06-18 0:13 UTC (permalink / raw)
To: Lian Minghuan-b31939; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
On 06/17/2013 12:15:36 AM, Lian Minghuan-b31939 wrote:
> On 06/15/2013 06:10 AM, Scott Wood wrote:
>> On 06/14/2013 02:15:58 AM, Minghuan Lian wrote:
>>> For MPIC v4.3 MSIIR supports 8 MSI registers and MSIIR1 supports
>>> 16 MSI registers, but uses different IBS and SRS shift. For the
>>> first register, when using MSIIR we will get the irqs 0x0 0x1 0x2
>>> ...0x1f, but when using MSIIR1, the irqs are 0x0 0x10 0x20 ... 0x1f0
>>> It is hard to describe the available irqs using property
>>> 'msi-available-ranges'. The patch removes this property.
>>=20
>> Only remove it from mpic 4.3. And since you introduced =20
>> qoriq-mpic4.3.dtsi earlier in the patchset, why didn't you just =20
>> avoid adding it then?
>>=20
> [Minghuan] If adding it in qoriq-mpic4.3, and the 3-5 patches are not =20
> accepted, mpic4.3 can also work.
mpic 4.3 cannot work with msi-available-ranges, at all. The hardware =20
just doesn't work that way.
-Scott=
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 5/5] powerpc/fsl_msi: add 'msiregs' kernel parameter
2013-06-14 7:15 [PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node Minghuan Lian
` (2 preceding siblings ...)
2013-06-14 7:15 ` [PATCH 4/5] powerpc/dts: remove msi-available-ranges property Minghuan Lian
@ 2013-06-14 7:15 ` Minghuan Lian
2013-06-14 22:13 ` Scott Wood
2013-06-14 20:39 ` [PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node Scott Wood
4 siblings, 1 reply; 28+ messages in thread
From: Minghuan Lian @ 2013-06-14 7:15 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Scott Wood, Minghuan Lian, Zang Roy-R61911
1. Only MSIIR1 can index 16 MSI registers, but when using MSIIR1
the IRQs of a register are not continuous. for example, the first
register irq values are 0x0, 0x10, 0x20, 0x30 ... 0x1f0. So it
is hard to use 'msi-available-ranges' property to indicate the
available ranges and 'msi-available-ranges' property has been
removed from dts node, so this patch removes the related code.
2. Add 'msiregs' kernel parameter instead of 'msi-available-ranges'
functionality. 'msiregs' is used to indicate the available MSI
registers ranges and uses a colon ':' to separate the multiple
banks. The range representation format is 'start-end', 'start'
and 'end' are integers describe the start and end register index,
the available registers lies between start and end and not include
end. For example, the available register x satisfying
start <= x < end.
Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
---
arch/powerpc/sysdev/fsl_msi.c | 118 ++++++++++++++++++++++++++++--------------
arch/powerpc/sysdev/fsl_msi.h | 1 +
2 files changed, 80 insertions(+), 39 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c
index 34510b7..db382ef9b 100644
--- a/arch/powerpc/sysdev/fsl_msi.c
+++ b/arch/powerpc/sysdev/fsl_msi.c
@@ -52,6 +52,60 @@ struct fsl_msi_cascade_data {
int index;
};
+struct msi_reg_range {
+ u32 start;
+ u32 end;
+};
+
+static struct msi_reg_range msiregs[NR_MSI_BANK] = {
+ {.start = 0, .end = NR_MSI_REG },
+ {.start = 0, .end = NR_MSI_REG },
+ {.start = 0, .end = NR_MSI_REG },
+ {.start = 0, .end = NR_MSI_REG },
+};
+
+/*
+ * Handle 'msiregs' parameter.
+ * msiregs is used to indicate the available MSI registers range and
+ * uses colon ':' to separate the multiple banks ranges.
+ * For each bank, the registers range format is 'start-end'
+ * start and end are integers, used to the indicate the start and end
+ * register index. The range is a set of real numbers that lies between
+ * start and end but not include end. For example, the set of all numbers
+ * x satisfying start <= x < end.
+ * if no range specified, driver will use the default range including all
+ * the registers.
+ * if you do no want to use this bank, you can set range as '0-0'
+ * For example msiregs=0-16:0-0::0-2
+ */
+static int msi_regs_setup(char *s)
+{
+ int bank = 0;
+ char *p;
+ struct msi_reg_range *range;
+
+ while ((p = strsep(&s, ":")) != NULL) {
+ int start = 0, end = NR_MSI_REG;
+
+ if (bank >= NR_MSI_BANK)
+ break;
+ range = &msiregs[bank];
+
+ if ((*p != '\0') && (sscanf(p, "%d-%d", &start, &end) < 1))
+ pr_err("msiregs correct format is: start-end\n");
+
+ /* Ok, gets the specified value */
+ range->start = start;
+ range->end = end;
+ pr_info("MSI bank%d available regs range is %d-%d\n",
+ bank, range->start, range->end);
+ bank++;
+ }
+ return 1;
+}
+
+__setup("msiregs=", msi_regs_setup);
+
static inline u32 fsl_msi_read(u32 __iomem *base, unsigned int reg)
{
return in_be32(base + (reg >> 2));
@@ -350,7 +404,7 @@ static int fsl_of_msi_remove(struct platform_device *ofdev)
static struct lock_class_key fsl_msi_irq_class;
static int fsl_msi_setup_hwirq(struct fsl_msi *msi, struct platform_device *dev,
- int offset, int irq_index)
+ int irq_index)
{
struct fsl_msi_cascade_data *cascade_data = NULL;
int virt_msir, i;
@@ -369,7 +423,7 @@ static int fsl_msi_setup_hwirq(struct fsl_msi *msi, struct platform_device *dev,
}
irq_set_lockdep_class(virt_msir, &fsl_msi_irq_class);
msi->msi_virqs[irq_index] = virt_msir;
- cascade_data->index = offset;
+ cascade_data->index = irq_index;
cascade_data->msi_data = msi;
irq_set_handler_data(virt_msir, cascade_data);
irq_set_chained_handler(virt_msir, fsl_msi_cascade);
@@ -377,7 +431,7 @@ static int fsl_msi_setup_hwirq(struct fsl_msi *msi, struct platform_device *dev,
/* 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);
+ msi_hwirq(msi, irq_index, i), 1);
return 0;
}
@@ -387,21 +441,29 @@ static int fsl_of_msi_probe(struct platform_device *dev)
{
const struct of_device_id *match;
struct fsl_msi *msi;
+ static int bank;
+ struct msi_reg_range *range;
struct resource res, msiir;
- int err, i, j, irq_index, count;
+ int err, irq_index, count;
int rc;
- const u32 *p;
const struct fsl_msi_feature *features;
- int len;
- u32 offset;
- static const u32 all_avail[] = { 0, NR_MSI_IRQS };
match = of_match_device(fsl_of_msi_ids, &dev->dev);
if (!match)
return -EINVAL;
features = match->data;
- printk(KERN_DEBUG "Setting up Freescale MSI support\n");
+ if (bank >= NR_MSI_BANK)
+ return -EINVAL;
+ range = &msiregs[bank];
+ pr_debug("Setting up Freescale MSI bank%d support\n", bank);
+
+ count = of_irq_count(dev->dev.of_node);
+ if (!count)
+ return -ENODEV;
+
+ if (count > NR_MSI_REG)
+ count = NR_MSI_REG;
msi = kzalloc(sizeof(struct fsl_msi), GFP_KERNEL);
if (!msi) {
@@ -475,39 +537,17 @@ static int fsl_of_msi_probe(struct platform_device *dev)
goto error_out;
}
- 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;
- }
-
- if (!p) {
- p = all_avail;
- len = sizeof(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;
+ for (irq_index = 0; irq_index < count; irq_index++) {
+ /* Check whether the register is contained in range */
+ if (irq_index < range->start ||
+ irq_index >= range->end)
+ continue;
+ err = fsl_msi_setup_hwirq(msi, dev, irq_index);
+ if (err)
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 + j, irq_index);
- if (err)
- goto error_out;
- }
}
+ bank++;
list_add_tail(&msi->list, &msi_head);
/* The multiple setting ppc_md.setup_msi_irqs will not harm things */
diff --git a/arch/powerpc/sysdev/fsl_msi.h b/arch/powerpc/sysdev/fsl_msi.h
index 43a9d99..6048415 100644
--- a/arch/powerpc/sysdev/fsl_msi.h
+++ b/arch/powerpc/sysdev/fsl_msi.h
@@ -16,6 +16,7 @@
#include <linux/of.h>
#include <asm/msi_bitmap.h>
+#define NR_MSI_BANK 4
#define NR_MSI_REG 16
#define IRQS_PER_MSI_REG 32
#define NR_MSI_IRQS (NR_MSI_REG * IRQS_PER_MSI_REG)
--
1.8.1.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 5/5] powerpc/fsl_msi: add 'msiregs' kernel parameter
2013-06-14 7:15 ` [PATCH 5/5] powerpc/fsl_msi: add 'msiregs' kernel parameter Minghuan Lian
@ 2013-06-14 22:13 ` Scott Wood
2013-06-17 5:36 ` Lian Minghuan-b31939
0 siblings, 1 reply; 28+ messages in thread
From: Scott Wood @ 2013-06-14 22:13 UTC (permalink / raw)
To: Minghuan Lian; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
On 06/14/2013 02:15:59 AM, Minghuan Lian wrote:
> 1. Only MSIIR1 can index 16 MSI registers, but when using MSIIR1
> the IRQs of a register are not continuous. for example, the first
> register irq values are 0x0, 0x10, 0x20, 0x30 ... 0x1f0. So it
> is hard to use 'msi-available-ranges' property to indicate the
> available ranges and 'msi-available-ranges' property has been
> removed from dts node, so this patch removes the related code.
>=20
> 2. Add 'msiregs' kernel parameter instead of 'msi-available-ranges'
> functionality.
The reason we used a device tree property was because this is for =20
virtualization and AMP scenarios where this instance of Linux does not =20
own all of the MSI registers.
I don't see any reasonable way to partition an MPIC v4.3 MSI group -- =20
but there are more groups, so it's not that bad. What's the use case =20
for this patch?
-Scott=
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/5] powerpc/fsl_msi: add 'msiregs' kernel parameter
2013-06-14 22:13 ` Scott Wood
@ 2013-06-17 5:36 ` Lian Minghuan-b31939
2013-06-18 0:18 ` Scott Wood
0 siblings, 1 reply; 28+ messages in thread
From: Lian Minghuan-b31939 @ 2013-06-17 5:36 UTC (permalink / raw)
To: Scott Wood; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
Hi Scott,
please see my comments inline.
On 06/15/2013 06:13 AM, Scott Wood wrote:
> On 06/14/2013 02:15:59 AM, Minghuan Lian wrote:
>> 1. Only MSIIR1 can index 16 MSI registers, but when using MSIIR1
>> the IRQs of a register are not continuous. for example, the first
>> register irq values are 0x0, 0x10, 0x20, 0x30 ... 0x1f0. So it
>> is hard to use 'msi-available-ranges' property to indicate the
>> available ranges and 'msi-available-ranges' property has been
>> removed from dts node, so this patch removes the related code.
>>
>> 2. Add 'msiregs' kernel parameter instead of 'msi-available-ranges'
>> functionality.
>
> The reason we used a device tree property was because this is for
> virtualization and AMP scenarios where this instance of Linux does not
> own all of the MSI registers.
>
> I don't see any reasonable way to partition an MPIC v4.3 MSI group --
> but there are more groups, so it's not that bad. What's the use case
> for this patch?
>
[Minghuan] I do not known any case about this patch. I add 'msiregs'
just for achieving "msi-available-ranges" functionality. I do not want
to remove partition functionality when updating to mpic4.3, although I
do not see virtualization and AMP cases on T4(KVM does not need this
functionality).
If you hope to keep 'msi-available-ranges' property for older mpic, can
I add 'msi-available-regs' property for mpic4.3.
Using 'msi-available-regs' to implement partition is similar to
'msi-available-ranges'. When considering the consistency,
'msi-available-regs' is better than msiregs.
> -Scott
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/5] powerpc/fsl_msi: add 'msiregs' kernel parameter
2013-06-17 5:36 ` Lian Minghuan-b31939
@ 2013-06-18 0:18 ` Scott Wood
2013-06-18 3:10 ` Lian Minghuan-b31939
0 siblings, 1 reply; 28+ messages in thread
From: Scott Wood @ 2013-06-18 0:18 UTC (permalink / raw)
To: Lian Minghuan-b31939; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
On 06/17/2013 12:36:50 AM, Lian Minghuan-b31939 wrote:
> Hi Scott,
>=20
> please see my comments inline.
>=20
> On 06/15/2013 06:13 AM, Scott Wood wrote:
>> On 06/14/2013 02:15:59 AM, Minghuan Lian wrote:
>>> 1. Only MSIIR1 can index 16 MSI registers, but when using MSIIR1
>>> the IRQs of a register are not continuous. for example, the first
>>> register irq values are 0x0, 0x10, 0x20, 0x30 ... 0x1f0. So it
>>> is hard to use 'msi-available-ranges' property to indicate the
>>> available ranges and 'msi-available-ranges' property has been
>>> removed from dts node, so this patch removes the related code.
>>>=20
>>> 2. Add 'msiregs' kernel parameter instead of 'msi-available-ranges'
>>> functionality.
>>=20
>> The reason we used a device tree property was because this is for =20
>> virtualization and AMP scenarios where this instance of Linux does =20
>> not own all of the MSI registers.
>>=20
>> I don't see any reasonable way to partition an MPIC v4.3 MSI group =20
>> -- but there are more groups, so it's not that bad. What's the use =20
>> case for this patch?
>>=20
> [Minghuan] I do not known any case about this patch. I add 'msiregs' =20
> just for achieving "msi-available-ranges" functionality. I do not =20
> want to remove partition functionality when updating to mpic4.3, =20
> although I do not see virtualization and AMP cases on T4(KVM does not =20
> need this functionality).
Such functionality does not work on mpic v4.3. There are conflicting =20
requirements of contiguous MSIs (because PCI devices can use them that =20
way) and the inability to partition a single register (because they all =20
go to the same MPIC interrupt).
Keep msi-available-ranges as is for older hardware, and just ignore it =20
(with a warning printed) if it's present on MPIC v4.3.
-Scott=
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/5] powerpc/fsl_msi: add 'msiregs' kernel parameter
2013-06-18 0:18 ` Scott Wood
@ 2013-06-18 3:10 ` Lian Minghuan-b31939
2013-06-18 16:22 ` Scott Wood
0 siblings, 1 reply; 28+ messages in thread
From: Lian Minghuan-b31939 @ 2013-06-18 3:10 UTC (permalink / raw)
To: Scott Wood; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
Hi Scott,
please see my comments inline.
On 06/18/2013 08:18 AM, Scott Wood wrote:
> On 06/17/2013 12:36:50 AM, Lian Minghuan-b31939 wrote:
>> Hi Scott,
>>
>> please see my comments inline.
>>
>> On 06/15/2013 06:13 AM, Scott Wood wrote:
>>> On 06/14/2013 02:15:59 AM, Minghuan Lian wrote:
>>>> 1. Only MSIIR1 can index 16 MSI registers, but when using MSIIR1
>>>> the IRQs of a register are not continuous. for example, the first
>>>> register irq values are 0x0, 0x10, 0x20, 0x30 ... 0x1f0. So it
>>>> is hard to use 'msi-available-ranges' property to indicate the
>>>> available ranges and 'msi-available-ranges' property has been
>>>> removed from dts node, so this patch removes the related code.
>>>>
>>>> 2. Add 'msiregs' kernel parameter instead of 'msi-available-ranges'
>>>> functionality.
>>>
>>> The reason we used a device tree property was because this is for
>>> virtualization and AMP scenarios where this instance of Linux does
>>> not own all of the MSI registers.
>>>
>>> I don't see any reasonable way to partition an MPIC v4.3 MSI group
>>> -- but there are more groups, so it's not that bad. What's the use
>>> case for this patch?
>>>
>> [Minghuan] I do not known any case about this patch. I add 'msiregs'
>> just for achieving "msi-available-ranges" functionality. I do not
>> want to remove partition functionality when updating to mpic4.3,
>> although I do not see virtualization and AMP cases on T4(KVM does not
>> need this functionality).
>
> Such functionality does not work on mpic v4.3. There are conflicting
> requirements of contiguous MSIs (because PCI devices can use them that
> way) and the inability to partition a single register (because they
> all go to the same MPIC interrupt).
>
> Keep msi-available-ranges as is for older hardware, and just ignore it
> (with a warning printed) if it's present on MPIC v4.3.
>
[Minghuan] Thanks for your guidance.
But 'msireg' should be remained or removed? if remaining 'msiregs', it
should be for all mpic or only for mpic v4.3?
> -Scott
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/5] powerpc/fsl_msi: add 'msiregs' kernel parameter
2013-06-18 3:10 ` Lian Minghuan-b31939
@ 2013-06-18 16:22 ` Scott Wood
0 siblings, 0 replies; 28+ messages in thread
From: Scott Wood @ 2013-06-18 16:22 UTC (permalink / raw)
To: Lian Minghuan-b31939; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
On 06/17/2013 10:10:17 PM, Lian Minghuan-b31939 wrote:
> Hi Scott,
>=20
> please see my comments inline.
>=20
> On 06/18/2013 08:18 AM, Scott Wood wrote:
>> On 06/17/2013 12:36:50 AM, Lian Minghuan-b31939 wrote:
>>> Hi Scott,
>>>=20
>>> please see my comments inline.
>>>=20
>>> On 06/15/2013 06:13 AM, Scott Wood wrote:
>>>> On 06/14/2013 02:15:59 AM, Minghuan Lian wrote:
>>>>> 1. Only MSIIR1 can index 16 MSI registers, but when using MSIIR1
>>>>> the IRQs of a register are not continuous. for example, the first
>>>>> register irq values are 0x0, 0x10, 0x20, 0x30 ... 0x1f0. So it
>>>>> is hard to use 'msi-available-ranges' property to indicate the
>>>>> available ranges and 'msi-available-ranges' property has been
>>>>> removed from dts node, so this patch removes the related code.
>>>>>=20
>>>>> 2. Add 'msiregs' kernel parameter instead of =20
>>>>> 'msi-available-ranges'
>>>>> functionality.
>>>>=20
>>>> The reason we used a device tree property was because this is for =20
>>>> virtualization and AMP scenarios where this instance of Linux does =20
>>>> not own all of the MSI registers.
>>>>=20
>>>> I don't see any reasonable way to partition an MPIC v4.3 MSI group =20
>>>> -- but there are more groups, so it's not that bad. What's the use =20
>>>> case for this patch?
>>>>=20
>>> [Minghuan] I do not known any case about this patch. I add =20
>>> 'msiregs' just for achieving "msi-available-ranges" functionality. =20
>>> I do not want to remove partition functionality when updating to =20
>>> mpic4.3, although I do not see virtualization and AMP cases on =20
>>> T4(KVM does not need this functionality).
>>=20
>> Such functionality does not work on mpic v4.3. There are =20
>> conflicting requirements of contiguous MSIs (because PCI devices can =20
>> use them that way) and the inability to partition a single register =20
>> (because they all go to the same MPIC interrupt).
>>=20
>> Keep msi-available-ranges as is for older hardware, and just ignore =20
>> it (with a warning printed) if it's present on MPIC v4.3.
>>=20
> [Minghuan] Thanks for your guidance.
> But 'msireg' should be remained or removed?
Removed.
-Scott=
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node
2013-06-14 7:15 [PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node Minghuan Lian
` (3 preceding siblings ...)
2013-06-14 7:15 ` [PATCH 5/5] powerpc/fsl_msi: add 'msiregs' kernel parameter Minghuan Lian
@ 2013-06-14 20:39 ` Scott Wood
2013-06-14 21:53 ` Scott Wood
4 siblings, 1 reply; 28+ messages in thread
From: Scott Wood @ 2013-06-14 20:39 UTC (permalink / raw)
To: Minghuan Lian; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
On 06/14/2013 02:15:55 AM, Minghuan Lian wrote:
> +msi0: msi@41600 {
> + compatible =3D "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
More specific compatibles come first -- and I don't think this is 100% =20
backwards compatible with "fsl,mpic-msi" anyway.
-Scott=
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node
2013-06-14 20:39 ` [PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node Scott Wood
@ 2013-06-14 21:53 ` Scott Wood
2013-06-17 2:23 ` Lian Minghuan-b31939
0 siblings, 1 reply; 28+ messages in thread
From: Scott Wood @ 2013-06-14 21:53 UTC (permalink / raw)
To: Scott Wood; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
On 06/14/2013 03:39:26 PM, Scott Wood wrote:
> On 06/14/2013 02:15:55 AM, Minghuan Lian wrote:
>> +msi0: msi@41600 {
>> + compatible =3D "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
>=20
> More specific compatibles come first -- and I don't think this is =20
> 100% backwards compatible with "fsl,mpic-msi" anyway.
Also please update the binding.
-Scott=
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node
2013-06-14 21:53 ` Scott Wood
@ 2013-06-17 2:23 ` Lian Minghuan-b31939
2013-06-18 0:18 ` Scott Wood
0 siblings, 1 reply; 28+ messages in thread
From: Lian Minghuan-b31939 @ 2013-06-17 2:23 UTC (permalink / raw)
To: Scott Wood; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
Hi Scott,
please see my comments.
On 06/15/2013 05:53 AM, Scott Wood wrote:
> On 06/14/2013 03:39:26 PM, Scott Wood wrote:
>> On 06/14/2013 02:15:55 AM, Minghuan Lian wrote:
>>> +msi0: msi@41600 {
>>> + compatible = "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
>>
>> More specific compatibles come first -- and I don't think this is
>> 100% backwards compatible with "fsl,mpic-msi" anyway.
>
> Also please update the binding.
[Minghuan] Yes, maybe I should remove "fsl,mpic-msi". What do you think?
>
> -Scott
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node
2013-06-17 2:23 ` Lian Minghuan-b31939
@ 2013-06-18 0:18 ` Scott Wood
0 siblings, 0 replies; 28+ messages in thread
From: Scott Wood @ 2013-06-18 0:18 UTC (permalink / raw)
To: Lian Minghuan-b31939; +Cc: Minghuan Lian, linuxppc-dev, Zang Roy-R61911
On 06/16/2013 09:23:37 PM, Lian Minghuan-b31939 wrote:
> Hi Scott,
>=20
> please see my comments.
>=20
> On 06/15/2013 05:53 AM, Scott Wood wrote:
>> On 06/14/2013 03:39:26 PM, Scott Wood wrote:
>>> On 06/14/2013 02:15:55 AM, Minghuan Lian wrote:
>>>> +msi0: msi@41600 {
>>>> + compatible =3D "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
>>>=20
>>> More specific compatibles come first -- and I don't think this is =20
>>> 100% backwards compatible with "fsl,mpic-msi" anyway.
>>=20
>> Also please update the binding.
> [Minghuan] Yes, maybe I should remove "fsl,mpic-msi". What do you =20
> think?
Yes.
-Scott=
^ permalink raw reply [flat|nested] 28+ messages in thread