RE: [PATCH] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable - Wang Sawsd-A24013

From: "Wang Sawsd-A24013" <cqwang@motorola.com>
To: Kevin Hilman <khilman@deeprootsystems.com>
Cc: linux-omap@vger.kernel.org, nm@ti.com, Mike Chan <mikechan@google.com>
Subject: RE: [PATCH] OMAP2/3 Avoid GPIO pending irq status been set after irq_disable
Date: Fri, 5 Jun 2009 01:43:06 +0800	[thread overview]
Message-ID: <B00E06E2766C2744B022DE9BAF3C59D5372B8F@zmy16exm69.ds.mot.com> (raw)
In-Reply-To: <87r5y0gcc4.fsf@deeprootsystems.com>

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
> Sent: 2009年6月5日 1:04
> 
> Dumb question: Why use level?  Why not use falling edge for this?
> 
A good question, :-) We did use edge interrupt before, see the reason below.

> > The issue is, after the touch driver calls irq_disable, since it is
> > empty unless
> > Set the IRQ_DISABLED flag, so next time only the generic handler
> > function
> > handle_level_irq is called, this handler just ack to OMAP 
> GPIO IRQ that
> > is
> > To clear the IRQ status and mask the GPIO on OMAP side, but since NO
> > Touch driver IRQ action is called, so the touch Controller 
> can not gets
> > acknowledged, then the interrupt line will be always driven 
> low by the
> > external controller, 
> 
> If the GPIO is set to be edge triggered, the fact that it is still low
> won't matter, the genirq layer will have noticed a pending interrupt.
> 
If we use edge interrupt here, the potential issue is still existing, and also
We are liky to lose the interrupt.
After irq_disable and before HW suspend, if the interrupt line is driven low,
Then OMAP GPIO can catch this edge transition, so the IRQ is set,
Then the handle_edge_irq will clear the IRQ staus and mask the IRQ.
Since the controller is not ACKed, then the interrupt line is always driven low,
Then from then on, no edge can happen, and no more Touch interrupt can happen
Even when irq_enable is called, though we have the prepare for idle hooks,
But that only work when the edge transition happens after that prepare call,
Since it saves the GPIO data IN at that time, if the input level already changes
Before that time, then the workaround does not work. 

We ever made another patch to not only compare the data in, but also check
It is rising or falling edge, and check the CURRENT input level to decide whether to
Use LEVEL detect to manually trigger the interrupt, it works fine with our HW.
But later on, touch cotroller driver is updated to use level detect, then we
Met this issue. I think the patch we made to workaround the edge int lost is also needed
In current pm branch, currently we may still face the issue I mentioned above.

But think more generically, the kernel should be robust enough to handle such cases,
We can not let driver to do the workaround since it is specially OMAP platform issue,
Even we have the patch to workaround the edge GPIO IRQ lost issue, but the
IRQ staus maybe set again if the interrupt line is driven a second edge.
The generic handler will only be called once after irq_disable is called, and
If the interrupt happens more than once after that, then we still face the issue that
The IRQ status is set and pending there and prevent LPM.

I tried to summarize the root cause that we need this change, I think it can be simply
Two points:
1: on OMAP, GPIO IRQ status may be set as long as LEVEL/EDGE 
detect register is configured, regardless of IRQEN or WKEN
2: Pending GPIO IRQ status will impact LPM transition

Hope this can explain more on this patch.

Thanks,
Chunqiu

> > if we check the IRQ status for that GPIO pin, we
> > will find
> > The IRQ is always set. This will prevent PER enterring RET/OFF state
> > since GPIO
> > Will not acknowledge the IDLE request. The main purpose of 
> the second
> > patch
> > Is to clear the level/edge detect registers for OMAP GPIO 
> when their IRQ
> > is
> > Disabled, this can ensure the GPIO IRQ status will not be set and
> > prevent LPM.
> 
> Thanks for the very clear explanation.  I'm pretty sure I understand
> what is going on now.
> 
> I need to think some more about what you describe below, but 
> am curious
> what you think about using edge triggered GPIOs instead of level.
> 
See the above explanation

> Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html