* Re: [PATCH] DTS: DMA: Fix DMA3 interrupts
@ 2013-12-10 18:33 ` Scott Wood
0 siblings, 0 replies; 13+ messages in thread
From: Scott Wood @ 2013-12-10 18:33 UTC (permalink / raw)
To: Hongbo Zhang; +Cc: linuxppc-dev, linux-kernel
On Tue, 2013-12-10 at 18:33 +0800, Hongbo Zhang wrote:
> Scott,
> This issue is due to the non-continuous MPIC register, I think there is
> two ways to fix it.
>
> The first one is as what we are discussing, in fact the Bman/Qman DT
> author had introduced this way, and I had to follow it, it is a trick,
> adding 208 is a bit ugly I think, and even difficult to explain it to
> customers etc, but this way changes less codes.
>
> The second one is editing MPIC related codes without adding 208 to high
> interrupts. The point of translate interrupt number to MPIC register
> address is a so called 'isu' mechanism, we can do like the following
> example codes, then the tricky adding 208 isn't needed any more.
>
> Which one do you prefer?
> In fact I myself prefer the second, if the idea is acceptable, I will
> send a patch instead of this one. (and also alone with the internal
> patch decreasing 208 for the Bman/Qman)
>
> void __init corenet_ds_pic_init(void)
> {
> ......
>
> mpic = mpic_alloc(NULL, 0, flags, 0, 512, "OpenPIC");
> BUG_ON(mpic == NULL);
>
> // Add this start
> for (i = 0; i < 17; i++) {
> if (i < 11)
> addr_off = 0x10000 + 0x20 * 16 * i;
> else
> addr_off = 0x13000 + 0x20 * 16 * (i - 11); /* scape the
> address not for interrupts */
> mpic_assign_isu(mpic, i, mpic->paddr + addr_off);
> }
> // Add this end
>
> mpic_init(mpic);
> }
NACK
We already have a binding that states that the interrupt number is based
on the register offset, rather than whatever arbitrary numbers hardware
documenters decide to use next week.
While I'm not terribly happy with the usability of this, especially now
that it's not a simple "add 16", redefining the existing binding is not
OK (and in any case the code above seems obfuscatory). If we decide to
do something other than continue with register offset divided by 32,
then we need to define a new interrupt type (similar to current defined
types of error interrupt, timer, and IPI) for the new numberspace -- and
it should be handled when decoding that type of interrupt specifier,
rather than with the isu mechanism.
-Scott
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] DTS: DMA: Fix DMA3 interrupts
2013-12-10 18:33 ` Scott Wood
@ 2013-12-12 9:46 ` Hongbo Zhang
-1 siblings, 0 replies; 13+ messages in thread
From: Hongbo Zhang @ 2013-12-12 9:46 UTC (permalink / raw)
To: Scott Wood; +Cc: benh, linuxppc-dev, linux-kernel
On 12/11/2013 02:33 AM, Scott Wood wrote:
> On Tue, 2013-12-10 at 18:33 +0800, Hongbo Zhang wrote:
>> Scott,
>> This issue is due to the non-continuous MPIC register, I think there is
>> two ways to fix it.
>>
>> The first one is as what we are discussing, in fact the Bman/Qman DT
>> author had introduced this way, and I had to follow it, it is a trick,
>> adding 208 is a bit ugly I think, and even difficult to explain it to
>> customers etc, but this way changes less codes.
>>
>> The second one is editing MPIC related codes without adding 208 to high
>> interrupts. The point of translate interrupt number to MPIC register
>> address is a so called 'isu' mechanism, we can do like the following
>> example codes, then the tricky adding 208 isn't needed any more.
>>
>> Which one do you prefer?
>> In fact I myself prefer the second, if the idea is acceptable, I will
>> send a patch instead of this one. (and also alone with the internal
>> patch decreasing 208 for the Bman/Qman)
>>
>> void __init corenet_ds_pic_init(void)
>> {
>> ......
>>
>> mpic = mpic_alloc(NULL, 0, flags, 0, 512, "OpenPIC");
>> BUG_ON(mpic == NULL);
>>
>> // Add this start
>> for (i = 0; i < 17; i++) {
>> if (i < 11)
>> addr_off = 0x10000 + 0x20 * 16 * i;
>> else
>> addr_off = 0x13000 + 0x20 * 16 * (i - 11); /* scape the
>> address not for interrupts */
>> mpic_assign_isu(mpic, i, mpic->paddr + addr_off);
>> }
>> // Add this end
>>
>> mpic_init(mpic);
>> }
> NACK
>
> We already have a binding that states that the interrupt number is based
> on the register offset, rather than whatever arbitrary numbers hardware
> documenters decide to use next week.
>
> While I'm not terribly happy with the usability of this, especially now
> that it's not a simple "add 16", redefining the existing binding is not
> OK (and in any case the code above seems obfuscatory). If we decide to
> do something other than continue with register offset divided by 32,
> then we need to define a new interrupt type (similar to current defined
> types of error interrupt, timer, and IPI) for the new numberspace -- and
> it should be handled when decoding that type of interrupt specifier,
> rather than with the isu mechanism.
>
> -Scott
>
>
Scott,
Thanks for your comments. Since the second way isn't so good, let's
choose the original one.
But we meet a small accident now.
My patch is based on the http://patchwork.ozlabs.org/patch/291553/,
which had been superseded, so this thread can be closed now.
And Shenzhou has already sent a complete dma3 dtsi patch including
correct interrupt numbers, http://patchwork.ozlabs.org/patch/300026/, so
let's focus on this patch, and I will forward your first comments of my
patch there.
Thanks.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] DTS: DMA: Fix DMA3 interrupts
@ 2013-12-12 9:46 ` Hongbo Zhang
0 siblings, 0 replies; 13+ messages in thread
From: Hongbo Zhang @ 2013-12-12 9:46 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, linux-kernel
On 12/11/2013 02:33 AM, Scott Wood wrote:
> On Tue, 2013-12-10 at 18:33 +0800, Hongbo Zhang wrote:
>> Scott,
>> This issue is due to the non-continuous MPIC register, I think there is
>> two ways to fix it.
>>
>> The first one is as what we are discussing, in fact the Bman/Qman DT
>> author had introduced this way, and I had to follow it, it is a trick,
>> adding 208 is a bit ugly I think, and even difficult to explain it to
>> customers etc, but this way changes less codes.
>>
>> The second one is editing MPIC related codes without adding 208 to high
>> interrupts. The point of translate interrupt number to MPIC register
>> address is a so called 'isu' mechanism, we can do like the following
>> example codes, then the tricky adding 208 isn't needed any more.
>>
>> Which one do you prefer?
>> In fact I myself prefer the second, if the idea is acceptable, I will
>> send a patch instead of this one. (and also alone with the internal
>> patch decreasing 208 for the Bman/Qman)
>>
>> void __init corenet_ds_pic_init(void)
>> {
>> ......
>>
>> mpic = mpic_alloc(NULL, 0, flags, 0, 512, "OpenPIC");
>> BUG_ON(mpic == NULL);
>>
>> // Add this start
>> for (i = 0; i < 17; i++) {
>> if (i < 11)
>> addr_off = 0x10000 + 0x20 * 16 * i;
>> else
>> addr_off = 0x13000 + 0x20 * 16 * (i - 11); /* scape the
>> address not for interrupts */
>> mpic_assign_isu(mpic, i, mpic->paddr + addr_off);
>> }
>> // Add this end
>>
>> mpic_init(mpic);
>> }
> NACK
>
> We already have a binding that states that the interrupt number is based
> on the register offset, rather than whatever arbitrary numbers hardware
> documenters decide to use next week.
>
> While I'm not terribly happy with the usability of this, especially now
> that it's not a simple "add 16", redefining the existing binding is not
> OK (and in any case the code above seems obfuscatory). If we decide to
> do something other than continue with register offset divided by 32,
> then we need to define a new interrupt type (similar to current defined
> types of error interrupt, timer, and IPI) for the new numberspace -- and
> it should be handled when decoding that type of interrupt specifier,
> rather than with the isu mechanism.
>
> -Scott
>
>
Scott,
Thanks for your comments. Since the second way isn't so good, let's
choose the original one.
But we meet a small accident now.
My patch is based on the http://patchwork.ozlabs.org/patch/291553/,
which had been superseded, so this thread can be closed now.
And Shenzhou has already sent a complete dma3 dtsi patch including
correct interrupt numbers, http://patchwork.ozlabs.org/patch/300026/, so
let's focus on this patch, and I will forward your first comments of my
patch there.
Thanks.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] DTS: DMA: Fix DMA3 interrupts
2013-12-10 18:33 ` Scott Wood
@ 2013-12-17 8:48 ` Li Yang
-1 siblings, 0 replies; 13+ messages in thread
From: Li Yang @ 2013-12-17 8:48 UTC (permalink / raw)
To: Scott Wood; +Cc: Hongbo Zhang, Benjamin Herrenschmidt, linuxppc-dev, lkml
On Wed, Dec 11, 2013 at 2:33 AM, Scott Wood <scottwood@freescale.com> wrote:
> On Tue, 2013-12-10 at 18:33 +0800, Hongbo Zhang wrote:
>> Scott,
>> This issue is due to the non-continuous MPIC register, I think there is
>> two ways to fix it.
>>
>> The first one is as what we are discussing, in fact the Bman/Qman DT
>> author had introduced this way, and I had to follow it, it is a trick,
>> adding 208 is a bit ugly I think, and even difficult to explain it to
>> customers etc, but this way changes less codes.
>>
>> The second one is editing MPIC related codes without adding 208 to high
>> interrupts. The point of translate interrupt number to MPIC register
>> address is a so called 'isu' mechanism, we can do like the following
>> example codes, then the tricky adding 208 isn't needed any more.
>>
>> Which one do you prefer?
>> In fact I myself prefer the second, if the idea is acceptable, I will
>> send a patch instead of this one. (and also alone with the internal
>> patch decreasing 208 for the Bman/Qman)
>>
>> void __init corenet_ds_pic_init(void)
>> {
>> ......
>>
>> mpic = mpic_alloc(NULL, 0, flags, 0, 512, "OpenPIC");
>> BUG_ON(mpic == NULL);
>>
>> // Add this start
>> for (i = 0; i < 17; i++) {
>> if (i < 11)
>> addr_off = 0x10000 + 0x20 * 16 * i;
>> else
>> addr_off = 0x13000 + 0x20 * 16 * (i - 11); /* scape the
>> address not for interrupts */
>> mpic_assign_isu(mpic, i, mpic->paddr + addr_off);
>> }
>> // Add this end
>>
>> mpic_init(mpic);
>> }
>
> NACK
>
> We already have a binding that states that the interrupt number is based
> on the register offset, rather than whatever arbitrary numbers hardware
> documenters decide to use next week.
>
> While I'm not terribly happy with the usability of this, especially now
> that it's not a simple "add 16", redefining the existing binding is not
> OK (and in any case the code above seems obfuscatory). If we decide to
> do something other than continue with register offset divided by 32,
> then we need to define a new interrupt type (similar to current defined
> types of error interrupt, timer, and IPI) for the new numberspace -- and
> it should be handled when decoding that type of interrupt specifier,
> rather than with the isu mechanism.
I had to say that the current binding is terribly confusing. I know a
lot of people who were confused while looking into the device tree and
I had to help them out now and then. I even got confused for some
time at the very beginning. :( If we want to keep the current
binding, a big warning message is well deserved at the very beginning
of the binding document to clarify that the interrupt number used in
device tree has nothing to do with the interrupt number mentioned in
the silicon reference manuals. I think we should even mention the
"add 16" magic rule also to make it more intuitive.
Adding a new interrupt type for external interrupts is good to make
the interrupt number consistent with the Reference Manuals. But doing
that requires changing all the device trees that used the previous
binding, and we need a mechanism of judging which binding a device
tree is complying to.
Hongbo's proposal is not a complete fix for the readability issue.
But it makes the "add 16" magic rule continue to work, instead of
making it even worse by introducing the "+208" interrupts. The DMA3
patch is the first time that we add the "+208" interrupts in the
device trees of mainline kernel. It is a good time now to prevent us
making the readability even worse while not breaking things out there.
ISU is a standard mechanism in MPIC to deal with sparse configuration
register space, and IMO a good fit for this situation.
Regards,
Leo
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] DTS: DMA: Fix DMA3 interrupts
@ 2013-12-17 8:48 ` Li Yang
0 siblings, 0 replies; 13+ messages in thread
From: Li Yang @ 2013-12-17 8:48 UTC (permalink / raw)
To: Scott Wood; +Cc: Hongbo Zhang, linuxppc-dev, lkml
On Wed, Dec 11, 2013 at 2:33 AM, Scott Wood <scottwood@freescale.com> wrote:
> On Tue, 2013-12-10 at 18:33 +0800, Hongbo Zhang wrote:
>> Scott,
>> This issue is due to the non-continuous MPIC register, I think there is
>> two ways to fix it.
>>
>> The first one is as what we are discussing, in fact the Bman/Qman DT
>> author had introduced this way, and I had to follow it, it is a trick,
>> adding 208 is a bit ugly I think, and even difficult to explain it to
>> customers etc, but this way changes less codes.
>>
>> The second one is editing MPIC related codes without adding 208 to high
>> interrupts. The point of translate interrupt number to MPIC register
>> address is a so called 'isu' mechanism, we can do like the following
>> example codes, then the tricky adding 208 isn't needed any more.
>>
>> Which one do you prefer?
>> In fact I myself prefer the second, if the idea is acceptable, I will
>> send a patch instead of this one. (and also alone with the internal
>> patch decreasing 208 for the Bman/Qman)
>>
>> void __init corenet_ds_pic_init(void)
>> {
>> ......
>>
>> mpic = mpic_alloc(NULL, 0, flags, 0, 512, "OpenPIC");
>> BUG_ON(mpic == NULL);
>>
>> // Add this start
>> for (i = 0; i < 17; i++) {
>> if (i < 11)
>> addr_off = 0x10000 + 0x20 * 16 * i;
>> else
>> addr_off = 0x13000 + 0x20 * 16 * (i - 11); /* scape the
>> address not for interrupts */
>> mpic_assign_isu(mpic, i, mpic->paddr + addr_off);
>> }
>> // Add this end
>>
>> mpic_init(mpic);
>> }
>
> NACK
>
> We already have a binding that states that the interrupt number is based
> on the register offset, rather than whatever arbitrary numbers hardware
> documenters decide to use next week.
>
> While I'm not terribly happy with the usability of this, especially now
> that it's not a simple "add 16", redefining the existing binding is not
> OK (and in any case the code above seems obfuscatory). If we decide to
> do something other than continue with register offset divided by 32,
> then we need to define a new interrupt type (similar to current defined
> types of error interrupt, timer, and IPI) for the new numberspace -- and
> it should be handled when decoding that type of interrupt specifier,
> rather than with the isu mechanism.
I had to say that the current binding is terribly confusing. I know a
lot of people who were confused while looking into the device tree and
I had to help them out now and then. I even got confused for some
time at the very beginning. :( If we want to keep the current
binding, a big warning message is well deserved at the very beginning
of the binding document to clarify that the interrupt number used in
device tree has nothing to do with the interrupt number mentioned in
the silicon reference manuals. I think we should even mention the
"add 16" magic rule also to make it more intuitive.
Adding a new interrupt type for external interrupts is good to make
the interrupt number consistent with the Reference Manuals. But doing
that requires changing all the device trees that used the previous
binding, and we need a mechanism of judging which binding a device
tree is complying to.
Hongbo's proposal is not a complete fix for the readability issue.
But it makes the "add 16" magic rule continue to work, instead of
making it even worse by introducing the "+208" interrupts. The DMA3
patch is the first time that we add the "+208" interrupts in the
device trees of mainline kernel. It is a good time now to prevent us
making the readability even worse while not breaking things out there.
ISU is a standard mechanism in MPIC to deal with sparse configuration
register space, and IMO a good fit for this situation.
Regards,
Leo
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] DTS: DMA: Fix DMA3 interrupts
2013-12-17 8:48 ` Li Yang
(?)
@ 2013-12-17 19:45 ` Scott Wood
-1 siblings, 0 replies; 13+ messages in thread
From: Scott Wood @ 2013-12-17 19:45 UTC (permalink / raw)
To: Li Yang; +Cc: Hongbo Zhang, linuxppc-dev, lkml
On Tue, 2013-12-17 at 16:48 +0800, Li Yang wrote:
> On Wed, Dec 11, 2013 at 2:33 AM, Scott Wood <scottwood@freescale.com> wrote:
> > On Tue, 2013-12-10 at 18:33 +0800, Hongbo Zhang wrote:
> >> Scott,
> >> This issue is due to the non-continuous MPIC register, I think there is
> >> two ways to fix it.
> >>
> >> The first one is as what we are discussing, in fact the Bman/Qman DT
> >> author had introduced this way, and I had to follow it, it is a trick,
> >> adding 208 is a bit ugly I think, and even difficult to explain it to
> >> customers etc, but this way changes less codes.
> >>
> >> The second one is editing MPIC related codes without adding 208 to high
> >> interrupts. The point of translate interrupt number to MPIC register
> >> address is a so called 'isu' mechanism, we can do like the following
> >> example codes, then the tricky adding 208 isn't needed any more.
> >>
> >> Which one do you prefer?
> >> In fact I myself prefer the second, if the idea is acceptable, I will
> >> send a patch instead of this one. (and also alone with the internal
> >> patch decreasing 208 for the Bman/Qman)
> >>
> >> void __init corenet_ds_pic_init(void)
> >> {
> >> ......
> >>
> >> mpic = mpic_alloc(NULL, 0, flags, 0, 512, "OpenPIC");
> >> BUG_ON(mpic == NULL);
> >>
> >> // Add this start
> >> for (i = 0; i < 17; i++) {
> >> if (i < 11)
> >> addr_off = 0x10000 + 0x20 * 16 * i;
> >> else
> >> addr_off = 0x13000 + 0x20 * 16 * (i - 11); /* scape the
> >> address not for interrupts */
> >> mpic_assign_isu(mpic, i, mpic->paddr + addr_off);
> >> }
> >> // Add this end
> >>
> >> mpic_init(mpic);
> >> }
> >
> > NACK
> >
> > We already have a binding that states that the interrupt number is based
> > on the register offset, rather than whatever arbitrary numbers hardware
> > documenters decide to use next week.
> >
> > While I'm not terribly happy with the usability of this, especially now
> > that it's not a simple "add 16", redefining the existing binding is not
> > OK (and in any case the code above seems obfuscatory). If we decide to
> > do something other than continue with register offset divided by 32,
> > then we need to define a new interrupt type (similar to current defined
> > types of error interrupt, timer, and IPI) for the new numberspace -- and
> > it should be handled when decoding that type of interrupt specifier,
> > rather than with the isu mechanism.
>
> I had to say that the current binding is terribly confusing. I know a
> lot of people who were confused while looking into the device tree and
> I had to help them out now and then. I even got confused for some
> time at the very beginning. :( If we want to keep the current
> binding, a big warning message is well deserved at the very beginning
> of the binding document to clarify that the interrupt number used in
> device tree has nothing to do with the interrupt number mentioned in
> the silicon reference manuals. I think we should even mention the
> "add 16" magic rule also to make it more intuitive.
I suggested some explanatory text in another thread.
> Adding a new interrupt type for external interrupts is good to make
> the interrupt number consistent with the Reference Manuals. But doing
> that requires changing all the device trees that used the previous
> binding,
We may want to change them all for clarity, but there's no compatibility
issue. Old device trees must continue to be supported.
> and we need a mechanism of judging which binding a device
> tree is complying to.
If it uses the new interrupt type number, then it's the new binding.
Otherwise, it's the old. The binding would only be added to, not
changed.
I'm not convinced it's worth adding at this point, though -- let's add
some helpful text to the binding and see how much that helps.
-Scott
^ permalink raw reply [flat|nested] 13+ messages in thread