| Message ID | 4fcfcfdbc86c37c6d47ec32cdbd987f3b406e9b5.1392147256.git.stefan@agner.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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
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@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...
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?
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
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.
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@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;
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(-)