No subject

No subject

[Uwe Kleine-König]

<jason77.wang@gmail.com>
Bcc: 
Subject: Re: i.MX & IRQF_ONESHOT
Reply-To: 
In-Reply-To: <4D2EB6EF.7030608@eukrea.com>

Hello,

[adding tglx who AFAIK invented threaded irqs and the people involved
in 2991a1ca6e9b to Cc]

On Thu, Jan 13, 2011 at 09:25:19AM +0100, Eric B?nard wrote:
> while testing 2.6.37 on our i.MX27 based board - code in
> arch/arm/mach-imx/eukrea_mbimx27-baseboard.c - I noticed the
> touchscreen controller (ADS7846) doesn't work anymore.
> 
> A few IRQ are generated when probing for the chipset and starting
> calibration (usually first point works), then nothing more (even if
> the IRQ signals is generated as seen on the scope, the irq count
> doesn't increase anymore and stays <= 4 and no data is reported to
> the input layer).
> 
> drivers/input/touchscreen/ads7846.c was switched to threaded IRQ in
> commit 2991a1ca6e9b13b639a82c0eec0cbc191bf1f42f where was added :
> irq_flags |= IRQF_ONESHOT;
AFAIK this is how threaded irq usually work.  The irq should get
reenabled by irq_thread -> irq_finalize_oneshot then.

> Commenting out this line in the ads7846 driver makes it work again.
> Am I missing something obvious or is there a reason for IRQF_ONESHOT
> creating trouble with gpio irq or SPI on i.MX ?
I don't know.  Is the irq masked?  pending?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

i.MX & IRQF_ONESHOT

[Nicolas Ferre]

Le 13/01/2011 10:13, Uwe Kleine-K?nig :
> <jason77.wang@gmail.com>
> Bcc: 
> Subject: Re: i.MX & IRQF_ONESHOT
> Reply-To: 
> In-Reply-To: <4D2EB6EF.7030608@eukrea.com>
> 
> Hello,
> 
> [adding tglx who AFAIK invented threaded irqs and the people involved
> in 2991a1ca6e9b to Cc]
> 
> On Thu, Jan 13, 2011 at 09:25:19AM +0100, Eric B?nard wrote:
>> while testing 2.6.37 on our i.MX27 based board - code in
>> arch/arm/mach-imx/eukrea_mbimx27-baseboard.c - I noticed the
>> touchscreen controller (ADS7846) doesn't work anymore.
>>
>> A few IRQ are generated when probing for the chipset and starting
>> calibration (usually first point works), then nothing more (even if
>> the IRQ signals is generated as seen on the scope, the irq count
>> doesn't increase anymore and stays <= 4 and no data is reported to
>> the input layer).
>>
>> drivers/input/touchscreen/ads7846.c was switched to threaded IRQ in
>> commit 2991a1ca6e9b13b639a82c0eec0cbc191bf1f42f where was added :
>> irq_flags |= IRQF_ONESHOT;
> AFAIK this is how threaded irq usually work.  The irq should get
> reenabled by irq_thread -> irq_finalize_oneshot then.
> 
>> Commenting out this line in the ads7846 driver makes it work again.
>> Am I missing something obvious or is there a reason for IRQF_ONESHOT
>> creating trouble with gpio irq or SPI on i.MX ?
> I don't know.  Is the irq masked?  pending?

Just to let you know that I have the same issue on my at91sam9g10ek:
atmel_spi + ads7846 (using ADS7843e actually).
... solved by same workaround.

Best regards,
-- 
Nicolas Ferre

i.MX & IRQF_ONESHOT

[Thomas Gleixner]

On Thu, 13 Jan 2011, Nicolas Ferre wrote:
> Le 13/01/2011 10:13, Uwe Kleine-K?nig :
> > On Thu, Jan 13, 2011 at 09:25:19AM +0100, Eric B?nard wrote:
> >> while testing 2.6.37 on our i.MX27 based board - code in
> >> arch/arm/mach-imx/eukrea_mbimx27-baseboard.c - I noticed the
> >> touchscreen controller (ADS7846) doesn't work anymore.
> >>
> >> A few IRQ are generated when probing for the chipset and starting
> >> calibration (usually first point works), then nothing more (even if
> >> the IRQ signals is generated as seen on the scope, the irq count
> >> doesn't increase anymore and stays <= 4 and no data is reported to
> >> the input layer).
> >>
> >> drivers/input/touchscreen/ads7846.c was switched to threaded IRQ in
> >> commit 2991a1ca6e9b13b639a82c0eec0cbc191bf1f42f where was added :
> >> irq_flags |= IRQF_ONESHOT;
> > AFAIK this is how threaded irq usually work.  The irq should get
> > reenabled by irq_thread -> irq_finalize_oneshot then.

Well, yes and no. That applies only when you have a level type
interrupt as you want to keep the irq line masked until the thread did
it's magic with the device. Keeping the line masked for edge type
interrupts would be fatal as you might loose interrupts. That's why we
have the different flow handlers and the ONESHOT check is only
implemented in handle_level_irq().

Though the driver sets the ONESHOT flag unconditionally, which should
be not a problem in theory as we don't handle oneshot stuff in other
flow handlers. The only problem could be irq_finalize_oneshot()
fiddling with the irq line, but that's guarded by a IRQ_DISABLED and
IRQ_MASKED check so this should not hurt.

Though it might hurt when the interrupt line has handle_level_irq()
and the interrupt pin is configured for edge. The driver uses
TRIGGER_RAISING/FALLING which is usually a sign of edge type. So I
assume that there is some wreckage lurking in there, which just got
covered up by the old code in some way.

> >> Commenting out this line in the ads7846 driver makes it work again.
> >> Am I missing something obvious or is there a reason for IRQF_ONESHOT
> >> creating trouble with gpio irq or SPI on i.MX ?
> > I don't know.  Is the irq masked?  pending?
> 
> Just to let you know that I have the same issue on my at91sam9g10ek:
> atmel_spi + ads7846 (using ADS7843e actually).
> ... solved by same workaround.

Eric, Nicolas: How are the interrupt handlers set for the relevant
interrupt lines and how are the interrupt pins configured?

Thanks,

	tglx

i.MX & IRQF_ONESHOT

[Eric Bénard]

On 13/01/2011 19:15, Thomas Gleixner wrote:
> On Thu, 13 Jan 2011, Nicolas Ferre wrote:
>> Le 13/01/2011 10:13, Uwe Kleine-K?nig :
>>> On Thu, Jan 13, 2011 at 09:25:19AM +0100, Eric B?nard wrote:
>>>> Commenting out this line in the ads7846 driver makes it work again.
>>>> Am I missing something obvious or is there a reason for IRQF_ONESHOT
>>>> creating trouble with gpio irq or SPI on i.MX ?
>>> I don't know.  Is the irq masked?  pending?
>>
>> Just to let you know that I have the same issue on my at91sam9g10ek:
>> atmel_spi + ads7846 (using ADS7843e actually).
>> ... solved by same workaround.
>
> Eric, Nicolas: How are the interrupt handlers set for the relevant
> interrupt lines and how are the interrupt pins configured?
>
on the i.MX27 :
- gpio configured as a plain input (in function ads7846_dev_init() in 
arch/arm/mach-imx/eukrea_mbimx27-baseboard.c)
- threaded interrupt handler registered with flags IRQF_TRIGGER_FALLING | 
IRQF_ONESHOT (in function ads7846_probe() in drivers/input/touchscreen/ads7846.c)
- the gpio's irq is handled by a chained irq handled registered in 
arch/arm/plat-mxc/gpio.c (the CPU has only one real interrupt for all the GPIO 
banks so this real IRQ handles up to 5 x 32 gpio's IRQ).

Eric

i.MX & IRQF_ONESHOT

[Uwe Kleine-König]

On Thu, Jan 13, 2011 at 10:12:23PM +0100, Eric B?nard wrote:
> On 13/01/2011 19:15, Thomas Gleixner wrote:
> >On Thu, 13 Jan 2011, Nicolas Ferre wrote:
> >>Le 13/01/2011 10:13, Uwe Kleine-K?nig :
> >>>On Thu, Jan 13, 2011 at 09:25:19AM +0100, Eric B?nard wrote:
> >>>>Commenting out this line in the ads7846 driver makes it work again.
> >>>>Am I missing something obvious or is there a reason for IRQF_ONESHOT
> >>>>creating trouble with gpio irq or SPI on i.MX ?
> >>>I don't know.  Is the irq masked?  pending?
> >>
> >>Just to let you know that I have the same issue on my at91sam9g10ek:
> >>atmel_spi + ads7846 (using ADS7843e actually).
> >>... solved by same workaround.
> >
> >Eric, Nicolas: How are the interrupt handlers set for the relevant
> >interrupt lines and how are the interrupt pins configured?
> >
> on the i.MX27 :
> - gpio configured as a plain input (in function ads7846_dev_init()
> in arch/arm/mach-imx/eukrea_mbimx27-baseboard.c)
> - threaded interrupt handler registered with flags
> IRQF_TRIGGER_FALLING | IRQF_ONESHOT (in function ads7846_probe() in
> drivers/input/touchscreen/ads7846.c)
I didn't recheck the hw manual, but adding IRQF_TRIGGER_FALLING calls
gpio_set_irq_type (defined in arch/arm/plat-mxc/gpio.c) which results
into something called GPIO_INT_FALL_EDGE being written into a register.

So I'd say it's configured to be edge sensitive.

> - the gpio's irq is handled by a chained irq handled registered in
> arch/arm/plat-mxc/gpio.c (the CPU has only one real interrupt for
> all the GPIO banks so this real IRQ handles up to 5 x 32 gpio's
> IRQ).

Best reagrds
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

i.MX & IRQF_ONESHOT

[Eric Bénard]

On 13/01/2011 22:24, Uwe Kleine-K?nig wrote:
> On Thu, Jan 13, 2011 at 10:12:23PM +0100, Eric B?nard wrote:
>> On 13/01/2011 19:15, Thomas Gleixner wrote:
>>> On Thu, 13 Jan 2011, Nicolas Ferre wrote:
>>>> Le 13/01/2011 10:13, Uwe Kleine-K?nig :
>>>>> On Thu, Jan 13, 2011 at 09:25:19AM +0100, Eric B?nard wrote:
>>>>>> Commenting out this line in the ads7846 driver makes it work again.
>>>>>> Am I missing something obvious or is there a reason for IRQF_ONESHOT
>>>>>> creating trouble with gpio irq or SPI on i.MX ?
>>>>> I don't know.  Is the irq masked?  pending?
>>>>
>>>> Just to let you know that I have the same issue on my at91sam9g10ek:
>>>> atmel_spi + ads7846 (using ADS7843e actually).
>>>> ... solved by same workaround.
>>>
>>> Eric, Nicolas: How are the interrupt handlers set for the relevant
>>> interrupt lines and how are the interrupt pins configured?
>>>
>> on the i.MX27 :
>> - gpio configured as a plain input (in function ads7846_dev_init()
>> in arch/arm/mach-imx/eukrea_mbimx27-baseboard.c)
>> - threaded interrupt handler registered with flags
>> IRQF_TRIGGER_FALLING | IRQF_ONESHOT (in function ads7846_probe() in
>> drivers/input/touchscreen/ads7846.c)
> I didn't recheck the hw manual, but adding IRQF_TRIGGER_FALLING calls
> gpio_set_irq_type (defined in arch/arm/plat-mxc/gpio.c) which results
> into something called GPIO_INT_FALL_EDGE being written into a register.
>
> So I'd say it's configured to be edge sensitive.
>
I confirm this point : the gpio is configured to trigger an irq on falling edge.

Eric

i.MX & IRQF_ONESHOT

[Thomas Gleixner]

On Fri, 14 Jan 2011, Eric B?nard wrote:

> On 13/01/2011 22:24, Uwe Kleine-K?nig wrote:
> > On Thu, Jan 13, 2011 at 10:12:23PM +0100, Eric B?nard wrote:
> > > On 13/01/2011 19:15, Thomas Gleixner wrote:
> > > > On Thu, 13 Jan 2011, Nicolas Ferre wrote:
> > > > > Le 13/01/2011 10:13, Uwe Kleine-K?nig :
> > > > > > On Thu, Jan 13, 2011 at 09:25:19AM +0100, Eric B?nard wrote:
> > > > > > > Commenting out this line in the ads7846 driver makes it work
> > > > > > > again.
> > > > > > > Am I missing something obvious or is there a reason for
> > > > > > > IRQF_ONESHOT
> > > > > > > creating trouble with gpio irq or SPI on i.MX ?
> > > > > > I don't know.  Is the irq masked?  pending?
> > > > > 
> > > > > Just to let you know that I have the same issue on my at91sam9g10ek:
> > > > > atmel_spi + ads7846 (using ADS7843e actually).
> > > > > ... solved by same workaround.
> > > > 
> > > > Eric, Nicolas: How are the interrupt handlers set for the relevant
> > > > interrupt lines and how are the interrupt pins configured?
> > > > 
> > > on the i.MX27 :
> > > - gpio configured as a plain input (in function ads7846_dev_init()
> > > in arch/arm/mach-imx/eukrea_mbimx27-baseboard.c)
> > > - threaded interrupt handler registered with flags
> > > IRQF_TRIGGER_FALLING | IRQF_ONESHOT (in function ads7846_probe() in
> > > drivers/input/touchscreen/ads7846.c)
> > I didn't recheck the hw manual, but adding IRQF_TRIGGER_FALLING calls
> > gpio_set_irq_type (defined in arch/arm/plat-mxc/gpio.c) which results
> > into something called GPIO_INT_FALL_EDGE being written into a register.
> > 
> > So I'd say it's configured to be edge sensitive.
> > 
> I confirm this point : the gpio is configured to trigger an irq on falling
> edge.

Right, but that does not tell me which flow handler is used for this
particular irq. Looking at arch/arm/plat-mxc/gpio.c it seems that the
gpio irqs are set up using handle_level_irq ! That explains the
ONESHOT wreckage nicely as you are missing an edge irq which comes in
_BEFORE_ the finalize code unmasks the line.

So there are several things wrong here:

1) Using handle_level_irq for an edge interrupt is wrong. That's not
   restricted to the TS driver problem at hand. That's wrong in
   general. Other set_type() functions adjust the handler as well when
   they switch the edge/level types.

2) Using ONESHOT for an edge interrupt is wrong as well. Removing the
   ONESHOT flag is just papering over the problem because it does not
   make the #1 problem go away, it just hides it.

The TS driver uses probably edge for historical reasons to avoid an
interrupt storm. But now with the oneshot mechanism we really should
use level type interrupts and keep the ONESHOT flag. 

Thanks,

	tglx

i.MX & IRQF_ONESHOT

[Nicolas Ferre]

Le 13/01/2011 19:15, Thomas Gleixner :
> On Thu, 13 Jan 2011, Nicolas Ferre wrote:
>> Le 13/01/2011 10:13, Uwe Kleine-K?nig :
>>> On Thu, Jan 13, 2011 at 09:25:19AM +0100, Eric B?nard wrote:
>>>> while testing 2.6.37 on our i.MX27 based board - code in
>>>> arch/arm/mach-imx/eukrea_mbimx27-baseboard.c - I noticed the
>>>> touchscreen controller (ADS7846) doesn't work anymore.
>>>>
>>>> A few IRQ are generated when probing for the chipset and starting
>>>> calibration (usually first point works), then nothing more (even if
>>>> the IRQ signals is generated as seen on the scope, the irq count
>>>> doesn't increase anymore and stays <= 4 and no data is reported to
>>>> the input layer).
>>>>
>>>> drivers/input/touchscreen/ads7846.c was switched to threaded IRQ in
>>>> commit 2991a1ca6e9b13b639a82c0eec0cbc191bf1f42f where was added :
>>>> irq_flags |= IRQF_ONESHOT;
>>> AFAIK this is how threaded irq usually work.  The irq should get
>>> reenabled by irq_thread -> irq_finalize_oneshot then.
> 
> Well, yes and no. That applies only when you have a level type
> interrupt as you want to keep the irq line masked until the thread did
> it's magic with the device. Keeping the line masked for edge type
> interrupts would be fatal as you might loose interrupts. That's why we
> have the different flow handlers and the ONESHOT check is only
> implemented in handle_level_irq().
> 
> Though the driver sets the ONESHOT flag unconditionally, which should
> be not a problem in theory as we don't handle oneshot stuff in other
> flow handlers. The only problem could be irq_finalize_oneshot()
> fiddling with the irq line, but that's guarded by a IRQ_DISABLED and
> IRQ_MASKED check so this should not hurt.
> 
> Though it might hurt when the interrupt line has handle_level_irq()
> and the interrupt pin is configured for edge. The driver uses
> TRIGGER_RAISING/FALLING which is usually a sign of edge type. So I
> assume that there is some wreckage lurking in there, which just got
> covered up by the old code in some way.
> 
>>>> Commenting out this line in the ads7846 driver makes it work again.
>>>> Am I missing something obvious or is there a reason for IRQF_ONESHOT
>>>> creating trouble with gpio irq or SPI on i.MX ?
>>> I don't know.  Is the irq masked?  pending?
>>
>> Just to let you know that I have the same issue on my at91sam9g10ek:
>> atmel_spi + ads7846 (using ADS7843e actually).
>> ... solved by same workaround.
> 
> Eric, Nicolas: How are the interrupt handlers set for the relevant
> interrupt lines and how are the interrupt pins configured?

Thomas,

For AT91:
- the platform does not provides irq_flags so default to
IRQF_TRIGGER_FALLING | IRQF_ONESHOT as stated in the driver.
- on at91sam9g10ek the touchscreen is wired to an full featured IRQ
entry (configured with pullup in arch/arm/mach-at91/board-sam9261ek.c).
- the irq description of plain interrupts is in arch/arm/mach-at91/irq.c
and at91_aic_set_type() has an IRQ_TYPE_EDGE_FALLING entry.

So it should be configured on falling edge...

But: is seems that looking at at91_aic_set_type() initialization
function, AIC is configured with:
set_irq_handler(i, handle_level_irq);

I do not know if it is the same as i.MX but it sound to be covered by
explanation that you gave...

Thanks, best regards,
-- 
Nicolas Ferre

i.MX & IRQF_ONESHOT

[Thomas Gleixner]

On Fri, 14 Jan 2011, Nicolas Ferre wrote:
> For AT91:
> - the platform does not provides irq_flags so default to
> IRQF_TRIGGER_FALLING | IRQF_ONESHOT as stated in the driver.
> - on at91sam9g10ek the touchscreen is wired to an full featured IRQ
> entry (configured with pullup in arch/arm/mach-at91/board-sam9261ek.c).
> - the irq description of plain interrupts is in arch/arm/mach-at91/irq.c
> and at91_aic_set_type() has an IRQ_TYPE_EDGE_FALLING entry.
> 
> So it should be configured on falling edge...
> 
> But: is seems that looking at at91_aic_set_type() initialization
> function, AIC is configured with:
> set_irq_handler(i, handle_level_irq);
> 
> I do not know if it is the same as i.MX but it sound to be covered by
> explanation that you gave...

Seems to be the same problem. 

Thanks,

	tglx

i.MX & IRQF_ONESHOT

[Uwe Kleine-König]

Hello,

On Fri, Jan 14, 2011 at 11:57:34AM +0100, Thomas Gleixner wrote:
> So there are several things wrong here:
> 
> 1) Using handle_level_irq for an edge interrupt is wrong. That's not
>    restricted to the TS driver problem at hand. That's wrong in
>    general. Other set_type() functions adjust the handler as well when
>    they switch the edge/level types.
That was introduced by 060d20d32ae7c6a20a8eac465795ed5bc9b37f7c.
As a quick test you can try to revert that.  If that helps I can look
deeper into it.  That won't help on at91 though.

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

i.MX & IRQF_ONESHOT

[Uwe Kleine-König]

On Fri, Jan 14, 2011 at 02:08:21PM +0100, Uwe Kleine-K?nig wrote:
> Hello,
> 
> On Fri, Jan 14, 2011 at 11:57:34AM +0100, Thomas Gleixner wrote:
> > So there are several things wrong here:
> > 
> > 1) Using handle_level_irq for an edge interrupt is wrong. That's not
> >    restricted to the TS driver problem at hand. That's wrong in
> >    general. Other set_type() functions adjust the handler as well when
> >    they switch the edge/level types.
> That was introduced by 060d20d32ae7c6a20a8eac465795ed5bc9b37f7c.
> As a quick test you can try to revert that.  If that helps I can look
> deeper into it.  That won't help on at91 though.
Eric, did you resolve the problem, did reverting 060d20d32ae7c6 help?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

i.MX & IRQF_ONESHOT

[Eric Bénard]

Hi Uwe,

On 02/02/2011 22:20, Uwe Kleine-K?nig wrote:
> On Fri, Jan 14, 2011 at 02:08:21PM +0100, Uwe Kleine-K?nig wrote:
>> Hello,
>>
>> On Fri, Jan 14, 2011 at 11:57:34AM +0100, Thomas Gleixner wrote:
>>> So there are several things wrong here:
>>>
>>> 1) Using handle_level_irq for an edge interrupt is wrong. That's not
>>>     restricted to the TS driver problem at hand. That's wrong in
>>>     general. Other set_type() functions adjust the handler as well when
>>>     they switch the edge/level types.
>> That was introduced by 060d20d32ae7c6a20a8eac465795ed5bc9b37f7c.
>> As a quick test you can try to revert that.  If that helps I can look
>> deeper into it.  That won't help on at91 though.
> Eric, did you resolve the problem, did reverting 060d20d32ae7c6 help?
>
Sorry, I didn't have the time to test yet.
I'll try to test this and provide feedback before the end of the week.

Best regards,
Eric

i.MX & IRQF_ONESHOT

[Eric Bénard]

Hi,

while testing 2.6.37 on our i.MX27 based board - code in 
arch/arm/mach-imx/eukrea_mbimx27-baseboard.c - I noticed the touchscreen 
controller (ADS7846) doesn't work anymore.

A few IRQ are generated when probing for the chipset and starting calibration 
(usually first point works), then nothing more (even if the IRQ signals is 
generated as seen on the scope, the irq count doesn't increase anymore and 
stays <= 4 and no data is reported to the input layer).

drivers/input/touchscreen/ads7846.c was switched to threaded IRQ in commit 
2991a1ca6e9b13b639a82c0eec0cbc191bf1f42f where was added :
irq_flags |= IRQF_ONESHOT;

Commenting out this line in the ads7846 driver makes it work again.
Am I missing something obvious or is there a reason for IRQF_ONESHOT creating 
trouble with gpio irq or SPI on i.MX ?

Thanks
Eric