| Message ID | 1365758001-15562-2-git-send-email-andreas.fenkart@streamunlimited.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 }
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 -- 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
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, }; /*---------------------------------------------------------------------*/
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(+)