* [PATCH] ARM: tegra: dalmore: fix irq trigger type
@ 2014-02-11 20:11 Stefan Agner
2014-02-11 20:47 ` Thierry Reding
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Agner @ 2014-02-11 20:11 UTC (permalink / raw)
To: linux-arm-kernel
Trigger type needs to be IRQ_TYPE_LEVEL_HIGH since the interrupt
signal gets inverted by the PMC (configured by the invert-interrupt
property).
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
I could not test that patch since I don't have such hardware.
However, I stumbled on that error while tracing a similar error
on Colibri T30.
arch/arm/boot/dts/tegra114-dalmore.dts | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
index 73aecfb..f5025fc 100644
--- a/arch/arm/boot/dts/tegra114-dalmore.dts
+++ b/arch/arm/boot/dts/tegra114-dalmore.dts
@@ -888,8 +888,9 @@
palmas: tps65913 at 58 {
compatible = "ti,palmas";
reg = <0x58>;
- interrupts = <0 86 IRQ_TYPE_LEVEL_LOW>;
+ /* active-low configured by PMC invert-interrupt */
+ interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
#interrupt-cells = <2>;
interrupt-controller;
--
1.8.5.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] ARM: tegra: dalmore: fix irq trigger type
2014-02-11 20:11 [PATCH] ARM: tegra: dalmore: fix irq trigger type Stefan Agner
@ 2014-02-11 20:47 ` Thierry Reding
2014-02-11 21:21 ` Stefan Agner
0 siblings, 1 reply; 6+ messages in thread
From: Thierry Reding @ 2014-02-11 20:47 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Feb 11, 2014 at 09:11:32PM +0100, Stefan Agner wrote:
> Trigger type needs to be IRQ_TYPE_LEVEL_HIGH since the interrupt
> signal gets inverted by the PMC (configured by the invert-interrupt
> property).
Isn't the reason the other way around? The PMIC generates a low-level
interrupt, but the GIC can only be configured to accept high-level (or
rising edge) and therefore the nvidia,invert-interrupt property needs to
be set in the PMC node?
One nitpick below.
> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
[...]
> @@ -888,8 +888,9 @@
> palmas: tps65913 at 58 {
> compatible = "ti,palmas";
> reg = <0x58>;
> - interrupts = <0 86 IRQ_TYPE_LEVEL_LOW>;
>
> + /* active-low configured by PMC invert-interrupt */
> + interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
I'd prefer to keep the properties grouped as before. interrupts is a
"client" property, whereas #interrupt-cells and interrupt-controller
are "provider" properties.
And I think the comment would be more appropriate in the pmc node, for
the same reason that I think the commit description isn't entirely
accurate.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140211/92fcdf86/attachment-0001.sig>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM: tegra: dalmore: fix irq trigger type
2014-02-11 20:47 ` Thierry Reding
@ 2014-02-11 21:21 ` Stefan Agner
2014-02-12 19:39 ` Stephen Warren
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Agner @ 2014-02-11 21:21 UTC (permalink / raw)
To: linux-arm-kernel
Am 2014-02-11 21:47, schrieb Thierry Reding:
> On Tue, Feb 11, 2014 at 09:11:32PM +0100, Stefan Agner wrote:
>> Trigger type needs to be IRQ_TYPE_LEVEL_HIGH since the interrupt
>> signal gets inverted by the PMC (configured by the invert-interrupt
>> property).
>
> Isn't the reason the other way around? The PMIC generates a low-level
> interrupt, but the GIC can only be configured to accept high-level (or
> rising edge) and therefore the nvidia,invert-interrupt property needs to
> be set in the PMC node?
Hm yes agreed. I should also write the whole story here, maybe this:
The GIC only support high-active interrupts. When using a PMIC with
low-active interrupt, the PMC has to be configured by using the
nvidia,invert-interrupt property in its node.
This fix sets the GIC back to high-active and reverts commit
eca8f98e404934027f84f72882c5e92ffbd9e5f5.
> One nitpick below.
>
>> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
> [...]
>> @@ -888,8 +888,9 @@
>> palmas: tps65913 at 58 {
>> compatible = "ti,palmas";
>> reg = <0x58>;
>> - interrupts = <0 86 IRQ_TYPE_LEVEL_LOW>;
>>
>> + /* active-low configured by PMC invert-interrupt */
>> + interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
>
> I'd prefer to keep the properties grouped as before. interrupts is a
> "client" property, whereas #interrupt-cells and interrupt-controller
> are "provider" properties.
>
> And I think the comment would be more appropriate in the pmc node, for
> the same reason that I think the commit description isn't entirely
> accurate.
Well, its kind a question where you are coming from. I read the data
sheet/schemata, and saw that its low-active. Then I went to the PMIC
node and checked how that interrupt is configured, and what you see is
HIGH_ACTIVE. You then think you've found the bug and fix it by setting
it to IRQ_TYPE_LEVEL_LOW. What you don't know is that PMC is in between
and alters the polarity...
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM: tegra: dalmore: fix irq trigger type
2014-02-11 21:21 ` Stefan Agner
@ 2014-02-12 19:39 ` Stephen Warren
2014-02-13 1:12 ` Joseph Lo
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Warren @ 2014-02-12 19:39 UTC (permalink / raw)
To: linux-arm-kernel
On 02/11/2014 02:21 PM, Stefan Agner wrote:
> Am 2014-02-11 21:47, schrieb Thierry Reding:
>> On Tue, Feb 11, 2014 at 09:11:32PM +0100, Stefan Agner wrote:
>>> Trigger type needs to be IRQ_TYPE_LEVEL_HIGH since the interrupt
>>> signal gets inverted by the PMC (configured by the invert-interrupt
>>> property).
>>
>> Isn't the reason the other way around? The PMIC generates a low-level
>> interrupt, but the GIC can only be configured to accept high-level (or
>> rising edge) and therefore the nvidia,invert-interrupt property needs to
>> be set in the PMC node?
> Hm yes agreed. I should also write the whole story here, maybe this:
>
> The GIC only support high-active interrupts. When using a PMIC with
> low-active interrupt, the PMC has to be configured by using the
> nvidia,invert-interrupt property in its node.
>
> This fix sets the GIC back to high-active and reverts commit
> eca8f98e404934027f84f72882c5e92ffbd9e5f5.
(Trimming CC lists)
Stefan,
It'd be best to include the commit subject rather than just the commit
hash, i.e.:
... and reverts commit eca8f98e4049 "ARM: tegra: dalmore: fix the irq
trigger type of Palmas MFD device".
It may also be helpful for the commit description to quote the kernel
boot message which this patch solves:
> [ 0.215178] genirq: Setting trigger mode 8 for irq 118 failed (gic_set_type+0x0/0xf4)
For me, applying this patch actually *causes* an interrupt storm, rather
than preventing one. Yet without it, no interrupts occur at all. I
wonder if the driver has a bug where it's not correctly clearing all
interrupt status (e.g. something pre-existing before boot), so once the
polarity is set up correctly, the interrupt is stuck?
Joseph,
As the author of the patch that's being reverted, can you please comment
here?
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM: tegra: dalmore: fix irq trigger type
2014-02-12 19:39 ` Stephen Warren
@ 2014-02-13 1:12 ` Joseph Lo
2014-02-13 2:19 ` Stephen Warren
0 siblings, 1 reply; 6+ messages in thread
From: Joseph Lo @ 2014-02-13 1:12 UTC (permalink / raw)
To: linux-arm-kernel
On 02/13/2014 03:39 AM, Stephen Warren wrote:
> On 02/11/2014 02:21 PM, Stefan Agner wrote:
>> Am 2014-02-11 21:47, schrieb Thierry Reding:
>>> On Tue, Feb 11, 2014 at 09:11:32PM +0100, Stefan Agner wrote:
>>>> Trigger type needs to be IRQ_TYPE_LEVEL_HIGH since the interrupt
>>>> signal gets inverted by the PMC (configured by the invert-interrupt
>>>> property).
>>>
>>> Isn't the reason the other way around? The PMIC generates a low-level
>>> interrupt, but the GIC can only be configured to accept high-level (or
>>> rising edge) and therefore the nvidia,invert-interrupt property needs to
>>> be set in the PMC node?
>> Hm yes agreed. I should also write the whole story here, maybe this:
>>
>> The GIC only support high-active interrupts. When using a PMIC with
>> low-active interrupt, the PMC has to be configured by using the
>> nvidia,invert-interrupt property in its node.
>>
>> This fix sets the GIC back to high-active and reverts commit
>> eca8f98e404934027f84f72882c5e92ffbd9e5f5.
>
> (Trimming CC lists)
>
> Stefan,
>
> It'd be best to include the commit subject rather than just the commit
> hash, i.e.:
>
> ... and reverts commit eca8f98e4049 "ARM: tegra: dalmore: fix the irq
> trigger type of Palmas MFD device".
>
> It may also be helpful for the commit description to quote the kernel
> boot message which this patch solves:
>
>> [ 0.215178] genirq: Setting trigger mode 8 for irq 118 failed (gic_set_type+0x0/0xf4)
>
> For me, applying this patch actually *causes* an interrupt storm, rather
> than preventing one. Yet without it, no interrupts occur at all. I
> wonder if the driver has a bug where it's not correctly clearing all
> interrupt status (e.g. something pre-existing before boot), so once the
> polarity is set up correctly, the interrupt is stuck?
>
> Joseph,
>
> As the author of the patch that's being reverted, can you please comment
> here?
>
I had explained why I fixed this here.
https://patchwork.kernel.org/patch/2832646/
By 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.
So this patch will make the PMIC not working on Dalmore and break the
suspend function.
-Joseph
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM: tegra: dalmore: fix irq trigger type
2014-02-13 1:12 ` Joseph Lo
@ 2014-02-13 2:19 ` Stephen Warren
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Warren @ 2014-02-13 2:19 UTC (permalink / raw)
To: linux-arm-kernel
On 02/12/2014 06:12 PM, Joseph Lo wrote:
> On 02/13/2014 03:39 AM, Stephen Warren wrote:
>> On 02/11/2014 02:21 PM, Stefan Agner wrote:
>>> Am 2014-02-11 21:47, schrieb Thierry Reding:
>>>> On Tue, Feb 11, 2014 at 09:11:32PM +0100, Stefan Agner wrote:
>>>>> Trigger type needs to be IRQ_TYPE_LEVEL_HIGH since the interrupt
>>>>> signal gets inverted by the PMC (configured by the invert-interrupt
>>>>> property).
...
>> For me, applying this patch actually *causes* an interrupt storm, rather
>> than preventing one. Yet without it, no interrupts occur at all. I
>> wonder if the driver has a bug where it's not correctly clearing all
>> interrupt status (e.g. something pre-existing before boot), so once the
>> polarity is set up correctly, the interrupt is stuck?
>>
>> Joseph,
>>
>> As the author of the patch that's being reverted, can you please comment
>> here?
>>
>
> I had explained why I fixed this here.
> https://patchwork.kernel.org/patch/2832646/
Looking back at that conversation, I'm not convinced by your explanation.
Some questions:
What does the PMIC output for an IRQ signal? It sounds like SW can
select either active-high level, or active-low level. Which of those is
the HW default? Does the Linux kernel driver attempt to program the HW
to select one or the other? If so, how does it determine which option to
choose, and where's the code that does that?
For now, let's not worry about system suspend issues, what the GIC
supports, etc. I only want to know about the PMIC for now.
Once we know the answer to that question, we can correctly configure the DT.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-02-13 2:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-11 20:11 [PATCH] ARM: tegra: dalmore: fix irq trigger type Stefan Agner
2014-02-11 20:47 ` Thierry Reding
2014-02-11 21:21 ` Stefan Agner
2014-02-12 19:39 ` Stephen Warren
2014-02-13 1:12 ` Joseph Lo
2014-02-13 2:19 ` Stephen Warren
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).