| Message ID | 1592818308-23001-5-git-send-email-mkshah@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/5] pinctrl: qcom: Remove irq_disable callback from msmgpio irqchip | expand |
Hi, On Mon, Jun 22, 2020 at 2:33 AM Maulik Shah <mkshah@codeaurora.org> wrote: > > Remove irq_disable callback to allow lazy disable for pdc interrupts. > > Add irq_set_wake callback that unmask interrupt in HW when drivers > mark interrupt for wakeup. Interrupt will be cleared in HW during > lazy disable if its not marked for wakeup. > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > --- > drivers/irqchip/qcom-pdc.c | 34 ++++++++++++++++------------------ > 1 file changed, 16 insertions(+), 18 deletions(-) > > diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c > index 6ae9e1f..8beb6f7 100644 > --- a/drivers/irqchip/qcom-pdc.c > +++ b/drivers/irqchip/qcom-pdc.c > @@ -36,6 +36,7 @@ struct pdc_pin_region { > u32 cnt; > }; > > +static DECLARE_BITMAP(pdc_wake_irqs, PDC_MAX_IRQS); > static DEFINE_RAW_SPINLOCK(pdc_lock); > static void __iomem *pdc_base; > static struct pdc_pin_region *pdc_region; > @@ -87,22 +88,17 @@ static void pdc_enable_intr(struct irq_data *d, bool on) > raw_spin_unlock(&pdc_lock); > } > > -static void qcom_pdc_gic_disable(struct irq_data *d) > +static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on) > { > - if (d->hwirq == GPIO_NO_WAKE_IRQ) > - return; > - > - pdc_enable_intr(d, false); > - irq_chip_disable_parent(d); > -} > - > -static void qcom_pdc_gic_enable(struct irq_data *d) > -{ > - if (d->hwirq == GPIO_NO_WAKE_IRQ) > - return; > + if (on) { > + pdc_enable_intr(d, true); > + irq_chip_enable_parent(d); > + set_bit(d->hwirq, pdc_wake_irqs); > + } else { > + clear_bit(d->hwirq, pdc_wake_irqs); > + } > > - pdc_enable_intr(d, true); > - irq_chip_enable_parent(d); > + return irq_chip_set_wake_parent(d, on); > } > > static void qcom_pdc_gic_mask(struct irq_data *d) > @@ -111,6 +107,9 @@ static void qcom_pdc_gic_mask(struct irq_data *d) > return; > > irq_chip_mask_parent(d); > + > + if (!test_bit(d->hwirq, pdc_wake_irqs)) > + pdc_enable_intr(d, false); I _think_ this will break masking, right? In other words, consider the following (having nothing to do with suspend/resume): 1. Driver requests an interrupt. 2. Driver masks interrupt (calls disable_irq()) 3. Interrupt fires while it is masked. 4. Driver unmasks interrupt (calls enable_irq(). After step #4 the interrupt should fire since it was only masked, not disabled (yes, it's super confusing that the driver calls disable_irq() but it expecting it to be masked--as I understand it that's just how it is). I haven't tested, but I suspect that's broken for you now (assuming you're working on a pin that wasn't a wakeup pin) because you won't track edges when you're "disabled". I suspect that the right thing to do here is to: a) Make qcom_pdc_gic_set_wake() just keep "pdc_wake_irqs" up to date and then call parent. b) Implement irq_suspend and irq_resume. In irq_suspend() you disable all interrupts that aren't in "pdc_wake_irqs". In irq_resume() you just re-enable all of them (masking will be handled by the parent). Would that work? ...oh, drat! The .irq_suspend() callback is only there if you're using "irq/generic-chip.c". OK, well unless we want to move over to using generic-chip we can just register for syscore ourselves. OK, I tested and <https://crrev.com/c/2296160> works. > } > > static void qcom_pdc_gic_unmask(struct irq_data *d) > @@ -118,6 +117,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d) > if (d->hwirq == GPIO_NO_WAKE_IRQ) > return; > > + pdc_enable_intr(d, true); > irq_chip_unmask_parent(d); > } > > @@ -197,15 +197,13 @@ static struct irq_chip qcom_pdc_gic_chip = { > .irq_eoi = irq_chip_eoi_parent, > .irq_mask = qcom_pdc_gic_mask, > .irq_unmask = qcom_pdc_gic_unmask, > - .irq_disable = qcom_pdc_gic_disable, > - .irq_enable = qcom_pdc_gic_enable, > .irq_get_irqchip_state = qcom_pdc_gic_get_irqchip_state, > .irq_set_irqchip_state = qcom_pdc_gic_set_irqchip_state, > .irq_retrigger = irq_chip_retrigger_hierarchy, > .irq_set_type = qcom_pdc_gic_set_type, > + .irq_set_wake = qcom_pdc_gic_set_wake, > .flags = IRQCHIP_MASK_ON_SUSPEND | > - IRQCHIP_SET_TYPE_MASKED | > - IRQCHIP_SKIP_SET_WAKE, > + IRQCHIP_SET_TYPE_MASKED, > .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent, > .irq_set_affinity = irq_chip_set_affinity_parent, > }; > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation >
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c index 6ae9e1f..8beb6f7 100644 --- a/drivers/irqchip/qcom-pdc.c +++ b/drivers/irqchip/qcom-pdc.c @@ -36,6 +36,7 @@ struct pdc_pin_region { u32 cnt; }; +static DECLARE_BITMAP(pdc_wake_irqs, PDC_MAX_IRQS); static DEFINE_RAW_SPINLOCK(pdc_lock); static void __iomem *pdc_base; static struct pdc_pin_region *pdc_region; @@ -87,22 +88,17 @@ static void pdc_enable_intr(struct irq_data *d, bool on) raw_spin_unlock(&pdc_lock); } -static void qcom_pdc_gic_disable(struct irq_data *d) +static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on) { - if (d->hwirq == GPIO_NO_WAKE_IRQ) - return; - - pdc_enable_intr(d, false); - irq_chip_disable_parent(d); -} - -static void qcom_pdc_gic_enable(struct irq_data *d) -{ - if (d->hwirq == GPIO_NO_WAKE_IRQ) - return; + if (on) { + pdc_enable_intr(d, true); + irq_chip_enable_parent(d); + set_bit(d->hwirq, pdc_wake_irqs); + } else { + clear_bit(d->hwirq, pdc_wake_irqs); + } - pdc_enable_intr(d, true); - irq_chip_enable_parent(d); + return irq_chip_set_wake_parent(d, on); } static void qcom_pdc_gic_mask(struct irq_data *d) @@ -111,6 +107,9 @@ static void qcom_pdc_gic_mask(struct irq_data *d) return; irq_chip_mask_parent(d); + + if (!test_bit(d->hwirq, pdc_wake_irqs)) + pdc_enable_intr(d, false); } static void qcom_pdc_gic_unmask(struct irq_data *d) @@ -118,6 +117,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d) if (d->hwirq == GPIO_NO_WAKE_IRQ) return; + pdc_enable_intr(d, true); irq_chip_unmask_parent(d); } @@ -197,15 +197,13 @@ static struct irq_chip qcom_pdc_gic_chip = { .irq_eoi = irq_chip_eoi_parent, .irq_mask = qcom_pdc_gic_mask, .irq_unmask = qcom_pdc_gic_unmask, - .irq_disable = qcom_pdc_gic_disable, - .irq_enable = qcom_pdc_gic_enable, .irq_get_irqchip_state = qcom_pdc_gic_get_irqchip_state, .irq_set_irqchip_state = qcom_pdc_gic_set_irqchip_state, .irq_retrigger = irq_chip_retrigger_hierarchy, .irq_set_type = qcom_pdc_gic_set_type, + .irq_set_wake = qcom_pdc_gic_set_wake, .flags = IRQCHIP_MASK_ON_SUSPEND | - IRQCHIP_SET_TYPE_MASKED | - IRQCHIP_SKIP_SET_WAKE, + IRQCHIP_SET_TYPE_MASKED, .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent, .irq_set_affinity = irq_chip_set_affinity_parent, };
Remove irq_disable callback to allow lazy disable for pdc interrupts. Add irq_set_wake callback that unmask interrupt in HW when drivers mark interrupt for wakeup. Interrupt will be cleared in HW during lazy disable if its not marked for wakeup. Signed-off-by: Maulik Shah <mkshah@codeaurora.org> --- drivers/irqchip/qcom-pdc.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)