Hi,
On Mon, Jun 22, 2020 at 2:33 AM Maulik Shah <mkshah@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> ---
> 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
>
