[PATCH] gpio/omap: implement irq_enable/disable using mask/unmask.

[PATCH] gpio/omap: implement irq_enable/disable using mask/unmask.

[Andreas Fenkart]

Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
---
 drivers/gpio/gpio-omap.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index d335af1..c1951ec 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -815,6 +815,8 @@ static struct irq_chip gpio_irq_chip = {
 	.irq_unmask	= gpio_unmask_irq,
 	.irq_set_type	= gpio_irq_type,
 	.irq_set_wake	= gpio_wake_enable,
+	.irq_disable    = gpio_mask_irq,
+	.irq_enable     = gpio_unmask_irq,
 };
 
 /*---------------------------------------------------------------------*/
-- 
1.7.10.4

Re: [PATCH] gpio/omap: implement irq_enable/disable using mask/unmask.

[Santosh Shilimkar]

On Monday 17 December 2012 02:57 PM, Andreas Fenkart wrote:

Please add some changelog here too.

> Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> ---
Patch seems straight forward thought will be interesting where you found
the need of it.

>   drivers/gpio/gpio-omap.c |    2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index d335af1..c1951ec 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -815,6 +815,8 @@ static struct irq_chip gpio_irq_chip = {
>   	.irq_unmask	= gpio_unmask_irq,
>   	.irq_set_type	= gpio_irq_type,
>   	.irq_set_wake	= gpio_wake_enable,
> +	.irq_disable    = gpio_mask_irq,
> +	.irq_enable     = gpio_unmask_irq,
>   };
>
>   /*---------------------------------------------------------------------*/
>

Re: [PATCH] gpio/omap: implement irq_enable/disable using mask/unmask.

[Jon Hunter]

On 12/19/2012 11:59 PM, Santosh Shilimkar wrote:
> On Monday 17 December 2012 02:57 PM, Andreas Fenkart wrote:
> 
> Please add some changelog here too.
> 
>> Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
>> ---
> Patch seems straight forward thought will be interesting where you found
> the need of it.

The only item that I was thinking of if the behaviour of mask/unmask
should be different from enable/disable?

When a gpio interrupt is masked, the gpio event will still be latched in
the interrupt status register so when you unmask it later you may get an
interrupt straight away. However, if the interrupt is disabled then gpio
events occurring will not be latched/stored.

I am also interested in the need for this, and if we should have a true
enable/disable here.

Cheers
Jon

> 
>>   drivers/gpio/gpio-omap.c |    2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index d335af1..c1951ec 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -815,6 +815,8 @@ static struct irq_chip gpio_irq_chip = {
>>       .irq_unmask    = gpio_unmask_irq,
>>       .irq_set_type    = gpio_irq_type,
>>       .irq_set_wake    = gpio_wake_enable,
>> +    .irq_disable    = gpio_mask_irq,
>> +    .irq_enable     = gpio_unmask_irq,
>>   };
>>
>>  
>> /*---------------------------------------------------------------------*/
>>
> 
> -- 
> 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

Re: [PATCH] gpio/omap: implement irq_enable/disable using mask/unmask.

[Andreas Fenkart]

Hi,

On Thu, Dec 20, 2012 at 10:16:56AM -0600, Jon Hunter wrote:
> 
> On 12/19/2012 11:59 PM, Santosh Shilimkar wrote:
> > On Monday 17 December 2012 02:57 PM, Andreas Fenkart wrote:
> > 
> > Please add some changelog here too.

::

In pm suspend the omap_hsmmc driver can't detect SDIO IRQs. This
is worked around by reconfiguring the SDIO IRQ pin (dat1) as a
GPIO before entering suspend and back upon resume. This requires
low overhead functions for enable/disable of GPIO IRQs.

> > 
> >> Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> >> ---
> > Patch seems straight forward thought will be interesting where you found
> > the need of it.
> 
> The only item that I was thinking of if the behaviour of mask/unmask
> should be different from enable/disable?
> 
> When a gpio interrupt is masked, the gpio event will still be latched in
> the interrupt status register so when you unmask it later you may get an
> interrupt straight away. However, if the interrupt is disabled then gpio
> events occurring will not be latched/stored.

Thanks for clarification.

The HW seems to support this, see Fig 25-3 in the manual. 
https://www.google.com/search?q=spruh73c

This problem though, applies only to edge triggered irqs.

In the case of level triggered the mask/unmask implementation
clears irqs upon resume. This is safe, because if the level still
indicates irq, it will be latched again and if not, the HW needs
no service.

For edge triggered the current implementation of mask/unmask
seems to behave more like enable/disable:

- irq detection is disabled during the masking period, which is
  like disable according above description. But leaves a small
  window for latching another one that is not served immediately,
  which is more like masking

	static void gpio_mask_irq(struct irq_data *d)                                                      
	{
													   
		spin_lock_irqsave(&bank->lock, flags);
		_set_gpio_irqenable(bank, gpio, 0);
                <--- new irqs latched here --->
		_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
		spin_unlock_irqrestore(&bank->lock, flags);
	}

- does not clear latched irqs upon resume. So not all irq raised
  after unmask are new, on the other hand some irqs that
  occured during the masking period might be lost.

> I am also interested in the need for this, and if we should have a true
> enable/disable here.

Since SDIO irqs are level triggered, I'm still okay with my patch.
For edge triggered though, it probably needs some more thoughts.

Suggestions? Should I split masking/disabling functions and make
them behave properly according above semantics or are you fine with
the interim solution of mapping enable/disable to mask/unmask?


rgds,
Andi


> >>   drivers/gpio/gpio-omap.c |    2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> >> index d335af1..c1951ec 100644
> >> --- a/drivers/gpio/gpio-omap.c
> >> +++ b/drivers/gpio/gpio-omap.c
> >> @@ -815,6 +815,8 @@ static struct irq_chip gpio_irq_chip = {
> >>       .irq_unmask    = gpio_unmask_irq,
> >>       .irq_set_type    = gpio_irq_type,
> >>       .irq_set_wake    = gpio_wake_enable,
> >> +    .irq_disable    = gpio_mask_irq,
> >> +    .irq_enable     = gpio_unmask_irq,
> >>   };
> >>

Re: [PATCH] gpio/omap: implement irq_enable/disable using mask/unmask.

[Andreas Fenkart]

Hi Felipe,


On Fri, Apr 12, 2013 at 02:07:01PM +0300, Felipe Balbi wrote:
[snip]
> > > Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> > > ---
> > Patch is incomplete and still confusing ;-) if some one reads the
> > patch without the thread. I think you have already ask the question/
> > suggestion in past but its better to split masking/disabling functions
> > and make them behave properly. Mapping enable/disable to mask/unmask
> > to get around the issue seems more of a hack.
> 
> right, specially since IRQ susystem will already do that for
> irq_enable():
> 
> kernel/irq/chip.c::irq_enable()
> 
> 192 void irq_enable(struct irq_desc *desc)
> 193 {
> 194         irq_state_clr_disabled(desc);
> 195         if (desc->irq_data.chip->irq_enable)
> 196                 desc->irq_data.chip->irq_enable(&desc->irq_data);
> 197         else
> 198                 desc->irq_data.chip->irq_unmask(&desc->irq_data);
> 199         irq_state_clr_masked(desc);
> 200 }
> 
> In fact this patch shouldn't be necessary if only IRQ subsystem would do
> the same for irq_disable() (though it doesn't and I haven't fully read
> the code you to understand why, however there's definitely a reason):
> 
> 202 void irq_disable(struct irq_desc *desc)
> 203 {
> 204         irq_state_set_disabled(desc);
> 205         if (desc->irq_data.chip->irq_disable) {
> 206                 desc->irq_data.chip->irq_disable(&desc->irq_data);
> 207                 irq_state_set_masked(desc);
> 208         }
> 209 }

I started some other thread over here:
[PATCH] genirq: use irq_mask as fallback for irq_disable.
https://lkml.org/lkml/2013/4/19/229

/Andi

Re: [PATCH] gpio/omap: implement irq_enable/disable using mask/unmask.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2475 bytes --]

Hi,

On Fri, Apr 12, 2013 at 03:49:52PM +0530, Santosh Shilimkar wrote:
> On Friday 12 April 2013 02:43 PM, Andreas Fenkart wrote:
> > In PM suspend, some omaps can't detect sdio irqs via the sdio interface.
> > The way to implement this, is to declare the corresponding pin as part
> > of the sdio interface and as a gpio input via pinctrl-single states.
> > The MMC driver request states "default" "active" and "idle" during the
> > probe, then toggles between active and idle during the runtime. This
> > requires low overhead functions for enable/disable of gpio irqs.
> > 
> > For level triggered interrupt there is no difference between masking
> > and disabling an interrupt. For edge interrupt interrupts there is.
> > When masked, interrupts should still be latched to the interrupt status
> > register so when unmasked later there is an interrupt straight away.
> > However, if the interrupt is disabled then gpio events occurring will not
> > be latched/stored. Hence proposed patch is incomplete for edge type
> > interrupts.
> > 
> > Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> > ---
> Patch is incomplete and still confusing ;-) if some one reads the
> patch without the thread. I think you have already ask the question/
> suggestion in past but its better to split masking/disabling functions
> and make them behave properly. Mapping enable/disable to mask/unmask
> to get around the issue seems more of a hack.

right, specially since IRQ susystem will already do that for
irq_enable():

kernel/irq/chip.c::irq_enable()

192 void irq_enable(struct irq_desc *desc)
193 {
194         irq_state_clr_disabled(desc);
195         if (desc->irq_data.chip->irq_enable)
196                 desc->irq_data.chip->irq_enable(&desc->irq_data);
197         else
198                 desc->irq_data.chip->irq_unmask(&desc->irq_data);
199         irq_state_clr_masked(desc);
200 }

In fact this patch shouldn't be necessary if only IRQ subsystem would do
the same for irq_disable() (though it doesn't and I haven't fully read
the code you to understand why, however there's definitely a reason):

202 void irq_disable(struct irq_desc *desc)
203 {
204         irq_state_set_disabled(desc);
205         if (desc->irq_data.chip->irq_disable) {
206                 desc->irq_data.chip->irq_disable(&desc->irq_data);
207                 irq_state_set_masked(desc);
208         }
209 }

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] gpio/omap: implement irq_enable/disable using mask/unmask.

[Santosh Shilimkar]

On Friday 12 April 2013 02:43 PM, Andreas Fenkart wrote:
> In PM suspend, some omaps can't detect sdio irqs via the sdio interface.
> The way to implement this, is to declare the corresponding pin as part
> of the sdio interface and as a gpio input via pinctrl-single states.
> The MMC driver request states "default" "active" and "idle" during the
> probe, then toggles between active and idle during the runtime. This
> requires low overhead functions for enable/disable of gpio irqs.
> 
> For level triggered interrupt there is no difference between masking
> and disabling an interrupt. For edge interrupt interrupts there is.
> When masked, interrupts should still be latched to the interrupt status
> register so when unmasked later there is an interrupt straight away.
> However, if the interrupt is disabled then gpio events occurring will not
> be latched/stored. Hence proposed patch is incomplete for edge type
> interrupts.
> 
> Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> ---
Patch is incomplete and still confusing ;-) if some one reads the
patch without the thread. I think you have already ask the question/
suggestion in past but its better to split masking/disabling functions
and make them behave properly. Mapping enable/disable to mask/unmask
to get around the issue seems more of a hack.

Also I think, we can address the edge type IRQ issue along with this
patch to be complete.

Before you go ahead with the update, I would like to hear Jon's
opinion on the edge IRQ related fixing.

>  drivers/gpio/gpio-omap.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 159f5c5..69ef2d8 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -815,6 +815,8 @@ static struct irq_chip gpio_irq_chip = {
>  	.irq_unmask	= gpio_unmask_irq,
>  	.irq_set_type	= gpio_irq_type,
>  	.irq_set_wake	= gpio_wake_enable,
> +	.irq_disable    = gpio_mask_irq, /* FIXME for edge type IRQ */
> +	.irq_enable     = gpio_unmask_irq,
>  };
>  
>  /*---------------------------------------------------------------------*/
> 
Sorry to make you wait for the proposal discussion.

Regards,
Santosh

[PATCH] gpio/omap: implement irq_enable/disable using mask/unmask.

[Andreas Fenkart]

In PM suspend, some omaps can't detect sdio irqs via the sdio interface.
The way to implement this, is to declare the corresponding pin as part
of the sdio interface and as a gpio input via pinctrl-single states.
The MMC driver request states "default" "active" and "idle" during the
probe, then toggles between active and idle during the runtime. This
requires low overhead functions for enable/disable of gpio irqs.

For level triggered interrupt there is no difference between masking
and disabling an interrupt. For edge interrupt interrupts there is.
When masked, interrupts should still be latched to the interrupt status
register so when unmasked later there is an interrupt straight away.
However, if the interrupt is disabled then gpio events occurring will not
be latched/stored. Hence proposed patch is incomplete for edge type
interrupts.

Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
---
 drivers/gpio/gpio-omap.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 159f5c5..69ef2d8 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -815,6 +815,8 @@ static struct irq_chip gpio_irq_chip = {
 	.irq_unmask	= gpio_unmask_irq,
 	.irq_set_type	= gpio_irq_type,
 	.irq_set_wake	= gpio_wake_enable,
+	.irq_disable    = gpio_mask_irq, /* FIXME for edge type IRQ */
+	.irq_enable     = gpio_unmask_irq,
 };
 
 /*---------------------------------------------------------------------*/
-- 
1.7.10.4