[PATCH V2] ARM: dts: tegra114: dalmore: fix the irq trigger type of Palmas MFD device

[PATCH V2] ARM: dts: tegra114: dalmore: fix the irq trigger type of Palmas MFD device

[Joseph Lo]

The IRQ trigger type of Palmas MFD device (tps65913) is edge trigger. The
wrong configuration would cause an interrupt storm when booting the
system. Fixing it in DT with appropriate interrupt type.

Cc: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra114-dalmore.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
index 76529e4..a2a1b5f 100644
--- a/arch/arm/boot/dts/tegra114-dalmore.dts
+++ b/arch/arm/boot/dts/tegra114-dalmore.dts
@@ -840,7 +840,7 @@
 		palmas: tps65913 {
 			compatible = "ti,palmas";
 			reg = <0x58>;
-			interrupts = <0 86 0x4>;
+			interrupts = <0 86 0x0>;
 
 			#interrupt-cells = <2>;
 			interrupt-controller;
-- 
1.8.3.2

Re: [PATCH V2] ARM: dts: tegra114: dalmore: fix the irq trigger type of Palmas MFD device

[Laxman Dewangan]

On Wednesday 24 July 2013 04:24 PM, Joseph Lo wrote:
> The IRQ trigger type of Palmas MFD device (tps65913) is edge trigger. The
> wrong configuration would cause an interrupt storm when booting the
> system. Fixing it in DT with appropriate interrupt type.
>
> Cc: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>   
Tested by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Re: [PATCH V2] ARM: dts: tegra114: dalmore: fix the irq trigger type of Palmas MFD device

[Stephen Warren]

On 07/24/2013 04:54 AM, Joseph Lo wrote:
> The IRQ trigger type of Palmas MFD device (tps65913) is edge trigger. The
> wrong configuration would cause an interrupt storm when booting the
> system. Fixing it in DT with appropriate interrupt type.

> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts

>  		palmas: tps65913 {
>  			compatible = "ti,palmas";
>  			reg = <0x58>;
> -			interrupts = <0 86 0x4>;
> +			interrupts = <0 86 0x0>;

The legal values for that final cell are:

    - bits[3:0] trigger type and level flags
        1 = low-to-high edge triggered
        2 = high-to-low edge triggered
        4 = active high level-sensitive
        8 = active low level-sensitive

0 isn't one of those values. This patch can't be correct.

BTW, this cell should use the constants from
<dt-bindings/interrupt-controller/irq.h>.

Re: [PATCH V2] ARM: dts: tegra114: dalmore: fix the irq trigger type of Palmas MFD device

[Joseph Lo]

On Fri, 2013-07-26 at 23:16 +0800, Stephen Warren wrote:
> On 07/24/2013 04:54 AM, Joseph Lo wrote:
> > The IRQ trigger type of Palmas MFD device (tps65913) is edge trigger. The
> > wrong configuration would cause an interrupt storm when booting the
> > system. Fixing it in DT with appropriate interrupt type.
> 
> > diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
> 
> >  		palmas: tps65913 {
> >  			compatible = "ti,palmas";
> >  			reg = <0x58>;
> > -			interrupts = <0 86 0x4>;
> > +			interrupts = <0 86 0x0>;
> 
> The legal values for that final cell are:
> 
>     - bits[3:0] trigger type and level flags
>         1 = low-to-high edge triggered
>         2 = high-to-low edge triggered
>         4 = active high level-sensitive
>         8 = active low level-sensitive
> 
> 0 isn't one of those values. This patch can't be correct.
> 
> BTW, this cell should use the constants from
> <dt-bindings/interrupt-controller/irq.h>.

Oops. I made a wrong description in the commit message.

Update my test result again.

> 1 = low-to-high edge triggered
No IRQ storm, but the system always auto wake up by Palmas RTC.
> 2 = high-to-low edge triggered
No IRQ storm, but the GIC didn't support this trigger type. The flag
would be re-configured as 0x0.
> 4 = active high level-sensitive
There is a IRQ storm when system booting.
> 8 = active low level-sensitive
No IRQ strom, but the GIC didn't support this trigger type. The flag
would be re-configured as 0x0.

> nvidia, invert-interrupt
Removing this can fix IRQ storm too, but the system always auto wake up
by Palmas RTC.

So we can only three trigger type here.
IRQ_TYPE_NONE 0
IRQ_TYPE_EDGE_FALLING 2
IRQ_TYPE_LEVEL_LOW 8

But using IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_LEVEL_LOW, it would
configure to IRQ_TYPE_NONE. Because the PMIC is low level trigger to PMC
on Dalmore, I would prefer to use IRQ_TYPE_LEVEL_LOW just like the v1.
Any comments?

Thanks,
Joseph

Re: [PATCH V2] ARM: dts: tegra114: dalmore: fix the irq trigger type of Palmas MFD device

[Stephen Warren]

On 07/30/2013 03:46 AM, Joseph Lo wrote:
> On Fri, 2013-07-26 at 23:16 +0800, Stephen Warren wrote:
>> On 07/24/2013 04:54 AM, Joseph Lo wrote:
>>> The IRQ trigger type of Palmas MFD device (tps65913) is edge trigger. The
>>> wrong configuration would cause an interrupt storm when booting the
>>> system. Fixing it in DT with appropriate interrupt type.
>>
>>> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
>>
>>>  		palmas: tps65913 {
>>>  			compatible = "ti,palmas";
>>>  			reg = <0x58>;
>>> -			interrupts = <0 86 0x4>;
>>> +			interrupts = <0 86 0x0>;
>>
>> The legal values for that final cell are:
>>
>>     - bits[3:0] trigger type and level flags
>>         1 = low-to-high edge triggered
>>         2 = high-to-low edge triggered
>>         4 = active high level-sensitive
>>         8 = active low level-sensitive
>>
>> 0 isn't one of those values. This patch can't be correct.
>>
>> BTW, this cell should use the constants from
>> <dt-bindings/interrupt-controller/irq.h>.
> 
> Oops. I made a wrong description in the commit message.
> 
> Update my test result again.
> 
>> 1 = low-to-high edge triggered
> No IRQ storm, but the system always auto wake up by Palmas RTC.
>> 2 = high-to-low edge triggered
> No IRQ storm, but the GIC didn't support this trigger type. The flag
> would be re-configured as 0x0.
>> 4 = active high level-sensitive
> There is a IRQ storm when system booting.
>> 8 = active low level-sensitive
> No IRQ strom, but the GIC didn't support this trigger type. The flag
> would be re-configured as 0x0.
> 
>> nvidia, invert-interrupt
> Removing this can fix IRQ storm too, but the system always auto wake up
> by Palmas RTC.
> 
> So we can only three trigger type here.
> IRQ_TYPE_NONE 0

That's not a valid value; it just means that the GIC driver doesn't
explicitly set the type, and the HW probably defaults to active high?

> IRQ_TYPE_EDGE_FALLING 2
> IRQ_TYPE_LEVEL_LOW 8
> 
> But using IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_LEVEL_LOW, it would
> configure to IRQ_TYPE_NONE. Because the PMIC is low level trigger to PMC
> on Dalmore, I would prefer to use IRQ_TYPE_LEVEL_LOW just like the v1.
> Any comments?

Comments no, I just have confusion!

We should simply set this flag to the correct value. Does the PMIC
output an edge-sensitive or level-sensitive signal? Is the signal
active-high/rising or active-low/falling? Those are the only things that
should be considered when selecting the correct value for the IRQ flags.
This information can be determined by reading the data sheet for the
PMIC; while the test results you mention above are interesting, the HW
documentation should drive the value we select here, unless the
documentation has a known bug.

If there is an IRQ storm, then that sounds like a bug in the code. That
needs to be fixed too, not worked around by setting an incorrect IRQ
flags value.

To be clear, here is what I would expect depending on what kind of IRQ
signal the PMIC outputs:

active-low:
GIC: active-high, PMC: nvidia,invert-interrupt

active-high:
GIC: active-high, PMC: NO nvidia,invert-interrupt

falling-edge:
GIC: rising-edge-triggered, PMC: nvidia,invert-interrupt

rising-edge:
GIC: rising-edge-triggered, PMC: NO nvidia,invert-interrupt

Re: [PATCH V2] ARM: dts: tegra114: dalmore: fix the irq trigger type of Palmas MFD device

[Joseph Lo]

On Wed, 2013-07-31 at 00:24 +0800, Stephen Warren wrote:
> On 07/30/2013 03:46 AM, Joseph Lo wrote:
> > On Fri, 2013-07-26 at 23:16 +0800, Stephen Warren wrote:
> >> On 07/24/2013 04:54 AM, Joseph Lo wrote:
> >>> The IRQ trigger type of Palmas MFD device (tps65913) is edge trigger. The
> >>> wrong configuration would cause an interrupt storm when booting the
> >>> system. Fixing it in DT with appropriate interrupt type.
> >>
> >>> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
> >>
> >>>  		palmas: tps65913 {
> >>>  			compatible = "ti,palmas";
> >>>  			reg = <0x58>;
> >>> -			interrupts = <0 86 0x4>;
> >>> +			interrupts = <0 86 0x0>;
> >>
> >> The legal values for that final cell are:
> >>
> >>     - bits[3:0] trigger type and level flags
> >>         1 = low-to-high edge triggered
> >>         2 = high-to-low edge triggered
> >>         4 = active high level-sensitive
> >>         8 = active low level-sensitive
> >>
> >> 0 isn't one of those values. This patch can't be correct.
> >>
> >> BTW, this cell should use the constants from
> >> <dt-bindings/interrupt-controller/irq.h>.
> > 
> > Oops. I made a wrong description in the commit message.
> > 
> > Update my test result again.
> > 
> >> 1 = low-to-high edge triggered
> > No IRQ storm, but the system always auto wake up by Palmas RTC.
> >> 2 = high-to-low edge triggered
> > No IRQ storm, but the GIC didn't support this trigger type. The flag
> > would be re-configured as 0x0.
> >> 4 = active high level-sensitive
> > There is a IRQ storm when system booting.
> >> 8 = active low level-sensitive
> > No IRQ strom, but the GIC didn't support this trigger type. The flag
> > would be re-configured as 0x0.
> > 
> >> nvidia, invert-interrupt
> > Removing this can fix IRQ storm too, but the system always auto wake up
> > by Palmas RTC.
> > 
> > So we can only three trigger type here.
> > IRQ_TYPE_NONE 0
> 
> That's not a valid value; it just means that the GIC driver doesn't
> explicitly set the type, and the HW probably defaults to active high?
> 
oh,yes. May be.

> > IRQ_TYPE_EDGE_FALLING 2
> > IRQ_TYPE_LEVEL_LOW 8
> > 
> > But using IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_LEVEL_LOW, it would
> > configure to IRQ_TYPE_NONE. Because the PMIC is low level trigger to PMC
> > on Dalmore, I would prefer to use IRQ_TYPE_LEVEL_LOW just like the v1.
> > Any comments?
> 
> Comments no, I just have confusion!
> 
> We should simply set this flag to the correct value. Does the PMIC
> output an edge-sensitive or level-sensitive signal? Is the signal
> active-high/rising or active-low/falling? Those are the only things that
> should be considered when selecting the correct value for the IRQ flags.
> This information can be determined by reading the data sheet for the
> PMIC; while the test results you mention above are interesting, the HW
> documentation should drive the value we select here, unless the
> documentation has a known bug.
> 
OK, the PMIC(Palmas here) only support level-sensitive and default is
active low.

But it was being configured as active high in the Dalmore's DT. And yes,
it can work fine with the configuration you mentioned below. The suspend
mode of LP1/LP2 that could be woken up by IRQ is OK in this case.

But if checking the PMU_INT signal in the schematic of Dalmore, it only
supports active low. It was connected to !PWR_INT of Tegra114 that will
cause the system wake up automatically when syspended to LP0 if active
high.

That's why my change is setting the IRQ type of PMIC to active low and
keep the "nvidia,invert-interrupt". And it can make the LP0/1/2 work
proper.

> If there is an IRQ storm, then that sounds like a bug in the code. That
> needs to be fixed too, not worked around by setting an incorrect IRQ
> flags value.
> 
By the way, don't you see IRQ storm like below(You use "if" here)?

[   87.054085] irq 118: nobody cared (try booting with the "irqpoll"
option)
[   87.060862] CPU: 0 PID: 41 Comm: irq/118-palmas Not tainted
3.11.0-rc2-next-20130722-00011-gadb202a-dirty #11
[   87.070811] [<c0012161>] (unwind_backtrace+0x1/0x98) from
[<c000f07b>] (show_stack+0xb/0xc)
[   87.079134] [<c000f07b>] (show_stack+0xb/0xc) from [<c038af89>]
(dump_stack+0x59/0x8c)
[   87.087035] [<c038af89>] (dump_stack+0x59/0x8c) from [<c005eef9>]
(__report_bad_irq+0x15/0x80)
[   87.095676] [<c005eef9>] (__report_bad_irq+0x15/0x80) from
[<c005f22d>] (note_interrupt+0x139/0x17c)
[   87.104782] [<c005f22d>] (note_interrupt+0x139/0x17c) from
[<c005e4d7>] (irq_thread+0xc3/0xd4)
[   87.113424] [<c005e4d7>] (irq_thread+0xc3/0xd4) from [<c002f87b>]
(kthread+0x6b/0x74)
[   87.121237] [<c002f87b>] (kthread+0x6b/0x74) from [<c000ccfd>]
(ret_from_fork+0x11/0x34)
[   87.121325] vdd-sensor-2v85: disabling
[   87.133064] handlers:
[   87.135335] [<c005dd85>] irq_default_primary_handler threaded
[<c01dbd99>] regmap_irq_thread
[   87.143762] Disabling IRQ #118

           CPU0       CPU1       CPU2       CPU3       
...
 85:     804308          0          0          0       GIC  85  i2c
118:     100001          0          0          0       GIC 118  palmas
122:        152          0          0          0       GIC 122  serial
...

> To be clear, here is what I would expect depending on what kind of IRQ
> signal the PMIC outputs:
> 
> active-low:
> GIC: active-high, PMC: nvidia,invert-interrupt
> 
> active-high:
> GIC: active-high, PMC: NO nvidia,invert-interrupt
> 
> falling-edge:
> GIC: rising-edge-triggered, PMC: nvidia,invert-interrupt
> 
> rising-edge:
> GIC: rising-edge-triggered, PMC: NO nvidia,invert-interrupt

Re: [PATCH V2] ARM: dts: tegra114: dalmore: fix the irq trigger type of Palmas MFD device

[Stephen Warren]

On 07/31/2013 03:10 AM, Joseph Lo wrote:
> On Wed, 2013-07-31 at 00:24 +0800, Stephen Warren wrote:
>> On 07/30/2013 03:46 AM, Joseph Lo wrote:
>>> On Fri, 2013-07-26 at 23:16 +0800, Stephen Warren wrote:
>>>> On 07/24/2013 04:54 AM, Joseph Lo wrote:
>>>>> The IRQ trigger type of Palmas MFD device (tps65913) is edge trigger. The
>>>>> wrong configuration would cause an interrupt storm when booting the
>>>>> system. Fixing it in DT with appropriate interrupt type.
>>>>
>>>>> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
>>>>
>>>>>  		palmas: tps65913 {
>>>>>  			compatible = "ti,palmas";
>>>>>  			reg = <0x58>;
>>>>> -			interrupts = <0 86 0x4>;
>>>>> +			interrupts = <0 86 0x0>;
>>>>
>>>> The legal values for that final cell are:
>>>>
>>>>     - bits[3:0] trigger type and level flags
>>>>         1 = low-to-high edge triggered
>>>>         2 = high-to-low edge triggered
>>>>         4 = active high level-sensitive
>>>>         8 = active low level-sensitive
>>>>
>>>> 0 isn't one of those values. This patch can't be correct.
>>>>
>>>> BTW, this cell should use the constants from
>>>> <dt-bindings/interrupt-controller/irq.h>.
>>>
>>> Oops. I made a wrong description in the commit message.
>>>
>>> Update my test result again.
>>>
>>>> 1 = low-to-high edge triggered
>>> No IRQ storm, but the system always auto wake up by Palmas RTC.
>>>> 2 = high-to-low edge triggered
>>> No IRQ storm, but the GIC didn't support this trigger type. The flag
>>> would be re-configured as 0x0.
>>>> 4 = active high level-sensitive
>>> There is a IRQ storm when system booting.
>>>> 8 = active low level-sensitive
>>> No IRQ strom, but the GIC didn't support this trigger type. The flag
>>> would be re-configured as 0x0.
>>>
>>>> nvidia, invert-interrupt
>>> Removing this can fix IRQ storm too, but the system always auto wake up
>>> by Palmas RTC.
>>>
>>> So we can only three trigger type here.
>>> IRQ_TYPE_NONE 0
>>
>> That's not a valid value; it just means that the GIC driver doesn't
>> explicitly set the type, and the HW probably defaults to active high?
>>
> oh,yes. May be.
> 
>>> IRQ_TYPE_EDGE_FALLING 2
>>> IRQ_TYPE_LEVEL_LOW 8
>>>
>>> But using IRQ_TYPE_EDGE_FALLING or IRQ_TYPE_LEVEL_LOW, it would
>>> configure to IRQ_TYPE_NONE. Because the PMIC is low level trigger to PMC
>>> on Dalmore, I would prefer to use IRQ_TYPE_LEVEL_LOW just like the v1.
>>> Any comments?
>>
>> Comments no, I just have confusion!
>>
>> We should simply set this flag to the correct value. Does the PMIC
>> output an edge-sensitive or level-sensitive signal? Is the signal
>> active-high/rising or active-low/falling? Those are the only things that
>> should be considered when selecting the correct value for the IRQ flags.
>> This information can be determined by reading the data sheet for the
>> PMIC; while the test results you mention above are interesting, the HW
>> documentation should drive the value we select here, unless the
>> documentation has a known bug.
>>
> OK, the PMIC(Palmas here) only support level-sensitive and default is
> active low.
> 
> But it was being configured as active high in the Dalmore's DT. And yes,
> it can work fine with the configuration you mentioned below. The suspend
> mode of LP1/LP2 that could be woken up by IRQ is OK in this case.
> 
> But if checking the PMU_INT signal in the schematic of Dalmore, it only
> supports active low. It was connected to !PWR_INT of Tegra114 that will
> cause the system wake up automatically when syspended to LP0 if active
> high.
> 
> That's why my change is setting the IRQ type of PMIC to active low and
> keep the "nvidia,invert-interrupt". And it can make the LP0/1/2 work
> proper.

OK, the explanation all basically makes sense. We do clearly need to fix
the DT so that the Palmas IRQ signal polarity matches what we've
configured the Tegra PMC/GIC to expect.

However, your patch was:

		palmas:·tps65913·{
			compatible·=·"ti,palmas";
			reg·=·<0x58>;
-			interrupts·=·<0·86·0x4>;
+			interrupts·=·<0·86·0x0>;

0 isn't valid. The replacement shouldn't be 0, but instead should be
IRQ_TYPE_LEVEL_LOW.

Alternatively, it sounds fine to leave that interrupts property as-is,
and remove the pmc's nvidia,invert-interrupt. Either sound like they
should work fine, although perhaps one polarity might cause glitches
before the inversion/... is programmed in the PMIC and/or based on
whatever pull-ups/downs are on the board?

>> If there is an IRQ storm, then that sounds like a bug in the code. That
>> needs to be fixed too, not worked around by setting an incorrect IRQ
>> flags value.
>>
> By the way, don't you see IRQ storm like below(You use "if" here)?
> 
> [   87.054085] irq 118: nobody cared (try booting with the "irqpoll" option)
> [   87.060862] CPU: 0 PID: 41 Comm: irq/118-palmas Not tainted 3.11.0-rc2-next-20130722-00011-gadb202a-dirty #11
> [   87.070811] [<c0012161>] (unwind_backtrace+0x1/0x98) from [<c000f07b>] (show_stack+0xb/0xc)
> [   87.079134] [<c000f07b>] (show_stack+0xb/0xc) from [<c038af89>] (dump_stack+0x59/0x8c)
> [   87.087035] [<c038af89>] (dump_stack+0x59/0x8c) from [<c005eef9>] (__report_bad_irq+0x15/0x80)
> [   87.095676] [<c005eef9>] (__report_bad_irq+0x15/0x80) from [<c005f22d>] (note_interrupt+0x139/0x17c)
> [   87.104782] [<c005f22d>] (note_interrupt+0x139/0x17c) from [<c005e4d7>] (irq_thread+0xc3/0xd4)
> [   87.113424] [<c005e4d7>] (irq_thread+0xc3/0xd4) from [<c002f87b>] (kthread+0x6b/0x74)
> [   87.121237] [<c002f87b>] (kthread+0x6b/0x74) from [<c000ccfd>] (ret_from_fork+0x11/0x34)
> [   87.121325] vdd-sensor-2v85: disabling
> [   87.133064] handlers:
> [   87.135335] [<c005dd85>] irq_default_primary_handler threaded [<c01dbd99>] regmap_irq_thread
> [   87.143762] Disabling IRQ #118

I don't recall seeing that.

>            CPU0       CPU1       CPU2       CPU3       
> ...
>  85:     804308          0          0          0       GIC  85  i2c
> 118:     100001          0          0          0       GIC 118  palmas
> 122:        152          0          0          0       GIC 122  serial
> ...

I didn't check that.