| Message ID | 1366620861-5489-2-git-send-email-andreas.fenkart@streamunlimited.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Andreas Fenkart <andreas.fenkart@streamunlimited.com> writes: > 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. > > Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com> The approach seems OK to me, but the changelog is still missing quite a bit of information. I know it's been hashed over in the various threads, but the changelog needs all that discussion summarized. The changelog should at least: - describe the problem/bug being fixed - describe the fix - answer "why" the approach was taken (not how.) This part is very important to reviewers and maintainers. - how it was tested, and on what platforms In addition, I'd like to see a description of how (or whether) this affects the genirq lazy disable functionality, and how it was tested. 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
On 04/22/2013 03:54 AM, Andreas Fenkart wrote: > 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. > > Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com> > --- > drivers/gpio/gpio-omap.c | 69 ++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 60 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 159f5c5..0b66c45 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -772,6 +772,12 @@ static void gpio_ack_irq(struct irq_data *d) > _clear_gpio_irqstatus(bank, gpio); > } > > +/** > + * gpio_mask_irq - mask IRQ signalling > + * @d : the gpio data we're acting upon > + * > + * Only signalling disabled. New IRQ still latched to IRQ status register. > + */ > static void gpio_mask_irq(struct irq_data *d) > { > struct gpio_bank *bank = irq_data_get_irq_chip_data(d); > @@ -780,33 +786,76 @@ static void gpio_mask_irq(struct irq_data *d) > > spin_lock_irqsave(&bank->lock, flags); > _set_gpio_irqenable(bank, gpio, 0); > - _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE); My only concern here is that _set_gpio_triggering() is also handling the wake-up enable register. So by clearing the triggering here it was also clearing the wake-up enable. Ideally, I would think that when masking the interrupt we would also want to disable the wake-up generation too (if enabled). I think it would only be a problem if someone called gpio_enable_irq() and the gpio_mask_irq(). Ideally the wake-up enable stuff should be removed from the triggering function. May be we could add explicit calls to _set_gpio_wakeup(). > spin_unlock_irqrestore(&bank->lock, flags); > } > > +/** > + * gpio_unmask_irq - unmask IRQ signalling > + * @d : the gpio data we're acting upon > + * > + * If an IRQ occured while IRQ was masked, you will get an IRQ straight away. > + */ > static void gpio_unmask_irq(struct irq_data *d) > { > struct gpio_bank *bank = irq_data_get_irq_chip_data(d); > unsigned int gpio = irq_to_gpio(bank, d->irq); > unsigned int irq_mask = GPIO_BIT(bank, gpio); > - u32 trigger = irqd_get_trigger_type(d); > unsigned long flags; > > spin_lock_irqsave(&bank->lock, flags); > - if (trigger) > - _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger); > > - /* For level-triggered GPIOs, the clearing must be done after > - * the HW source is cleared, thus after the handler has run */ > - if (bank->level_mask & irq_mask) { > - _set_gpio_irqenable(bank, gpio, 0); > + /* > + * For level-triggered GPIOs, clear the IRQ. If the HW > + * still needs service, IRQ will be latched again > + */ > + if (bank->level_mask & irq_mask) > _clear_gpio_irqstatus(bank, gpio); > - } > > _set_gpio_irqenable(bank, gpio, 1); > spin_unlock_irqrestore(&bank->lock, flags); > } Similarly here, can we ensure the wake-up enable is set? Cheers Jon -- 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
On 04/22/2013 03:54 AM, Andreas Fenkart wrote: > 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. > > Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com> > --- > drivers/gpio/gpio-omap.c | 69 ++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 60 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 159f5c5..0b66c45 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -772,6 +772,12 @@ static void gpio_ack_irq(struct irq_data *d) > _clear_gpio_irqstatus(bank, gpio); > } > > +/** > + * gpio_mask_irq - mask IRQ signalling > + * @d : the gpio data we're acting upon > + * > + * Only signalling disabled. New IRQ still latched to IRQ status register. > + */ > static void gpio_mask_irq(struct irq_data *d) > { > struct gpio_bank *bank = irq_data_get_irq_chip_data(d); > @@ -780,33 +786,76 @@ static void gpio_mask_irq(struct irq_data *d) > > spin_lock_irqsave(&bank->lock, flags); > _set_gpio_irqenable(bank, gpio, 0); > - _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE); > spin_unlock_irqrestore(&bank->lock, flags); > } > > +/** > + * gpio_unmask_irq - unmask IRQ signalling > + * @d : the gpio data we're acting upon > + * > + * If an IRQ occured while IRQ was masked, you will get an IRQ straight away. > + */ > static void gpio_unmask_irq(struct irq_data *d) > { > struct gpio_bank *bank = irq_data_get_irq_chip_data(d); > unsigned int gpio = irq_to_gpio(bank, d->irq); > unsigned int irq_mask = GPIO_BIT(bank, gpio); > - u32 trigger = irqd_get_trigger_type(d); > unsigned long flags; > > spin_lock_irqsave(&bank->lock, flags); > - if (trigger) > - _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger); > > - /* For level-triggered GPIOs, the clearing must be done after > - * the HW source is cleared, thus after the handler has run */ > - if (bank->level_mask & irq_mask) { > - _set_gpio_irqenable(bank, gpio, 0); > + /* > + * For level-triggered GPIOs, clear the IRQ. If the HW > + * still needs service, IRQ will be latched again > + */ > + if (bank->level_mask & irq_mask) > _clear_gpio_irqstatus(bank, gpio); > - } > > _set_gpio_irqenable(bank, gpio, 1); > spin_unlock_irqrestore(&bank->lock, flags); > } > > +/** > + * gpio_disable_irq - disable the interrupt > + * @d : the gpio data we're acting upon > + * > + * While disabled all IRQ events are ignored. No latching to IRQ status > + * register. > + */ > +static void gpio_disable_irq(struct irq_data *d) > +{ > + struct gpio_bank *bank = irq_data_get_irq_chip_data(d); > + unsigned int gpio = irq_to_gpio(bank, d->irq); > + unsigned long flags; > + > + spin_lock_irqsave(&bank->lock, flags); > + _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE); > + _set_gpio_irqenable(bank, gpio, 0); > + spin_unlock_irqrestore(&bank->lock, flags); > +} > + > +/** > + * gpio_enable_irq - enable the interrupt > + * @d : the gpio data we're acting upon > + * > + * Enables latching and signalling IRQ. > + */ > +static void gpio_enable_irq(struct irq_data *d) > +{ > + struct gpio_bank *bank = irq_data_get_irq_chip_data(d); > + unsigned int gpio = irq_to_gpio(bank, d->irq); > + unsigned int irq_mask = GPIO_BIT(bank, gpio); > + u32 trigger = irqd_get_trigger_type(d); > + unsigned long flags; > + > + spin_lock_irqsave(&bank->lock, flags); > + if (trigger) > + _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger); > + _clear_gpio_irqstatus(bank, gpio); > + _set_gpio_irqenable(bank, gpio, 1); > + spin_unlock_irqrestore(&bank->lock, flags); > +} This generates the following warning ... drivers/gpio/gpio-omap.c: In function 'gpio_enable_irq': drivers/gpio/gpio-omap.c:852:15: warning: unused variable 'irq_mask' [-Wunused-variable] So you can get rid of irq_mask in the above function. Jon -- 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
On 04/22/2013 03:54 AM, Andreas Fenkart wrote: > 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. > > Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com> > --- > drivers/gpio/gpio-omap.c | 69 ++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 60 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 159f5c5..0b66c45 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -772,6 +772,12 @@ static void gpio_ack_irq(struct irq_data *d) > _clear_gpio_irqstatus(bank, gpio); > } > > +/** > + * gpio_mask_irq - mask IRQ signalling > + * @d : the gpio data we're acting upon > + * > + * Only signalling disabled. New IRQ still latched to IRQ status register. > + */ > static void gpio_mask_irq(struct irq_data *d) > { > struct gpio_bank *bank = irq_data_get_irq_chip_data(d); > @@ -780,33 +786,76 @@ static void gpio_mask_irq(struct irq_data *d) > > spin_lock_irqsave(&bank->lock, flags); > _set_gpio_irqenable(bank, gpio, 0); > - _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE); > spin_unlock_irqrestore(&bank->lock, flags); > } > > +/** > + * gpio_unmask_irq - unmask IRQ signalling > + * @d : the gpio data we're acting upon > + * > + * If an IRQ occured while IRQ was masked, you will get an IRQ straight away. > + */ > static void gpio_unmask_irq(struct irq_data *d) > { > struct gpio_bank *bank = irq_data_get_irq_chip_data(d); > unsigned int gpio = irq_to_gpio(bank, d->irq); > unsigned int irq_mask = GPIO_BIT(bank, gpio); > - u32 trigger = irqd_get_trigger_type(d); > unsigned long flags; > > spin_lock_irqsave(&bank->lock, flags); > - if (trigger) > - _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger); > > - /* For level-triggered GPIOs, the clearing must be done after > - * the HW source is cleared, thus after the handler has run */ > - if (bank->level_mask & irq_mask) { > - _set_gpio_irqenable(bank, gpio, 0); > + /* > + * For level-triggered GPIOs, clear the IRQ. If the HW > + * still needs service, IRQ will be latched again > + */ > + if (bank->level_mask & irq_mask) > _clear_gpio_irqstatus(bank, gpio); > - } > > _set_gpio_irqenable(bank, gpio, 1); > spin_unlock_irqrestore(&bank->lock, flags); > } > > +/** > + * gpio_disable_irq - disable the interrupt > + * @d : the gpio data we're acting upon > + * > + * While disabled all IRQ events are ignored. No latching to IRQ status > + * register. > + */ > +static void gpio_disable_irq(struct irq_data *d) > +{ > + struct gpio_bank *bank = irq_data_get_irq_chip_data(d); > + unsigned int gpio = irq_to_gpio(bank, d->irq); > + unsigned long flags; > + > + spin_lock_irqsave(&bank->lock, flags); > + _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE); > + _set_gpio_irqenable(bank, gpio, 0); > + spin_unlock_irqrestore(&bank->lock, flags); > +} > + > +/** > + * gpio_enable_irq - enable the interrupt > + * @d : the gpio data we're acting upon > + * > + * Enables latching and signalling IRQ. > + */ > +static void gpio_enable_irq(struct irq_data *d) > +{ > + struct gpio_bank *bank = irq_data_get_irq_chip_data(d); > + unsigned int gpio = irq_to_gpio(bank, d->irq); > + unsigned int irq_mask = GPIO_BIT(bank, gpio); > + u32 trigger = irqd_get_trigger_type(d); > + unsigned long flags; > + > + spin_lock_irqsave(&bank->lock, flags); > + if (trigger) > + _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger); > + _clear_gpio_irqstatus(bank, gpio); > + _set_gpio_irqenable(bank, gpio, 1); > + spin_unlock_irqrestore(&bank->lock, flags); > +} > + > static struct irq_chip gpio_irq_chip = { > .name = "GPIO", > .irq_shutdown = gpio_irq_shutdown, > @@ -815,6 +864,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_disable_irq, > + .irq_enable = gpio_enable_irq, > }; I tested the above patch on some omap2/3/4 boards and this appears to break ethernet support (which is using a gpio as an interrupt). I am guessing this is related to the wake-up enable, so this needs some more work. Cheers Jon -- 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..0b66c45 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -772,6 +772,12 @@ static void gpio_ack_irq(struct irq_data *d) _clear_gpio_irqstatus(bank, gpio); } +/** + * gpio_mask_irq - mask IRQ signalling + * @d : the gpio data we're acting upon + * + * Only signalling disabled. New IRQ still latched to IRQ status register. + */ static void gpio_mask_irq(struct irq_data *d) { struct gpio_bank *bank = irq_data_get_irq_chip_data(d); @@ -780,33 +786,76 @@ static void gpio_mask_irq(struct irq_data *d) spin_lock_irqsave(&bank->lock, flags); _set_gpio_irqenable(bank, gpio, 0); - _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE); spin_unlock_irqrestore(&bank->lock, flags); } +/** + * gpio_unmask_irq - unmask IRQ signalling + * @d : the gpio data we're acting upon + * + * If an IRQ occured while IRQ was masked, you will get an IRQ straight away. + */ static void gpio_unmask_irq(struct irq_data *d) { struct gpio_bank *bank = irq_data_get_irq_chip_data(d); unsigned int gpio = irq_to_gpio(bank, d->irq); unsigned int irq_mask = GPIO_BIT(bank, gpio); - u32 trigger = irqd_get_trigger_type(d); unsigned long flags; spin_lock_irqsave(&bank->lock, flags); - if (trigger) - _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger); - /* For level-triggered GPIOs, the clearing must be done after - * the HW source is cleared, thus after the handler has run */ - if (bank->level_mask & irq_mask) { - _set_gpio_irqenable(bank, gpio, 0); + /* + * For level-triggered GPIOs, clear the IRQ. If the HW + * still needs service, IRQ will be latched again + */ + if (bank->level_mask & irq_mask) _clear_gpio_irqstatus(bank, gpio); - } _set_gpio_irqenable(bank, gpio, 1); spin_unlock_irqrestore(&bank->lock, flags); } +/** + * gpio_disable_irq - disable the interrupt + * @d : the gpio data we're acting upon + * + * While disabled all IRQ events are ignored. No latching to IRQ status + * register. + */ +static void gpio_disable_irq(struct irq_data *d) +{ + struct gpio_bank *bank = irq_data_get_irq_chip_data(d); + unsigned int gpio = irq_to_gpio(bank, d->irq); + unsigned long flags; + + spin_lock_irqsave(&bank->lock, flags); + _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE); + _set_gpio_irqenable(bank, gpio, 0); + spin_unlock_irqrestore(&bank->lock, flags); +} + +/** + * gpio_enable_irq - enable the interrupt + * @d : the gpio data we're acting upon + * + * Enables latching and signalling IRQ. + */ +static void gpio_enable_irq(struct irq_data *d) +{ + struct gpio_bank *bank = irq_data_get_irq_chip_data(d); + unsigned int gpio = irq_to_gpio(bank, d->irq); + unsigned int irq_mask = GPIO_BIT(bank, gpio); + u32 trigger = irqd_get_trigger_type(d); + unsigned long flags; + + spin_lock_irqsave(&bank->lock, flags); + if (trigger) + _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger); + _clear_gpio_irqstatus(bank, gpio); + _set_gpio_irqenable(bank, gpio, 1); + spin_unlock_irqrestore(&bank->lock, flags); +} + static struct irq_chip gpio_irq_chip = { .name = "GPIO", .irq_shutdown = gpio_irq_shutdown, @@ -815,6 +864,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_disable_irq, + .irq_enable = gpio_enable_irq, }; /*---------------------------------------------------------------------*/
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. Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com> --- drivers/gpio/gpio-omap.c | 69 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 60 insertions(+), 9 deletions(-)