irqchip: qcom: pdc: Introduce irq_set_wake call

The gpio can be marked for wakeup and drivers can invoke disable_irq() during suspend, in such cases unlazy approach will also disable at HW and such gpios will not wakeup device from suspend to RAM.

Remove irq_disable callback to allow gpio interrupts to lazy disabled. The gpio interrupts will get disabled during irq_mask callback.

Acked-by Linus Walleij <[email protected]>

Signed-off-by Maulik Shah <[email protected]>

---
 drivers/pinctrl/qcom/pinctrl-msm.c | 13 -------------
 1 file changed, 13 deletions(-)

Hi,

On 7/14/2020 3:47 AM, Doug Anderson wrote:

Hi,

Quoted 13 lines · Maulik Shah

Quoted 12 lines

The gpio can be marked for wakeup and drivers can invoke disable_irq() during suspend, in such cases unlazy approach will also disable at HW and such gpios will not wakeup device from suspend to RAM.

Remove irq_disable callback to allow gpio interrupts to lazy disabled. The gpio interrupts will get disabled during irq_mask callback.

Acked-by Linus Walleij <[email protected]>

Signed-off-by Maulik Shah <[email protected]>

---
 drivers/pinctrl/qcom/pinctrl-msm.c | 13 -------------
 1 file changed, 13 deletions(-)

Quoted 4 lines

While the code of this patch looks fairly correct to me (there's no need to implement the irq_disable callback and we can just rely on the masking), I'm slightly anxious about the description. It almost feels like you're somehow relying on the laziness to "fix" your issue here.

i don't think thats the case here. As i have mentioned in previous discussions, reiterating here..

During suspend there is no way IRQ will be enabled/unmasked in HW even if its marked for wakeup.

All kernel does during suspend is if an IRQ is not marked for wakeup (did not invoke enable_irq_wake()) then during suspend those IRQs will get disabled/masked in HW to prevent them waking up.

Note that kernel don't do anything for the IRQs that are marked for wakeup. those IRQs are left in its original state. by original state, i mean if the IRQ was enabled/unmasked in HW, it will stay as is, if its disabled/masked it will stay same. suspend process won't change the state of those IRQs.

Lets take two cases of lazy and unlazy disable/mask.

case-1)

if the irq_chip implements .irq_disable callback, genirq by default takes unlazy path (immediatly disabled in SW + HW). if device enters suspend after client driver calls disable_irq(), there is no way to wakeup with such IRQs, even though driver choosen to mark it for wakeup. As i told above, kernel leaves such IRQ in its original state (disabled in SW + HW here) This is the problem case where we started with.

case -2)

If the irq_chip did not implements .irq_disable callback, genirq takes lazy disable path and only marks IRQ disabled in SW. It is still left enabled in HW. This is what current series is implemented for.

Quoted 8 lines

...but the laziness is supposed to just be an optimization. Specifically if an interrupt happens to fire at any point in time after a driver has called disable_irq() then it acts just like a non-lazy disable.

Said another way, I think this is a valid thing for a driver to do and it should get woken up if the irq fires in suspend:

Quoted 6 lines

disable_irq();
msleep(1000);
enable_irq_wake()

Specifically if an IRQ comes in during that sleep then it will be just like you had a non-lazy IRQ.

i don't think, Let me take your example...driver calls below during suspend

1. disable_irq();
2. msleep(1000);
3. enable_irq_wake();

a) if the IRQ comes in before (1) No issue in this case, its just like during active time if any other IRQ gets handled.

b) if IRQ comes anytime after (1) is over, but (3) is not done (i.e. during msleep())

- genirq will find that IRQ was disabled in SW,
- driver's IRQ handler will not get called since it was disabled in SW via (1).
- it will mark pending in SW and then really disables in HW now (lazy disable)
- next call is enable_irq_wake(), which will mark IRQ as wake up capable and also re-enable in HW from patch-4 of this series in PDC driver's .irq_set_wake call...
if (on) {
pdc_enable_intr(d, true);
irq_chip_enable_parent(d);
set_bit(d->hwirq, pdc_wake_irqs);
}
with this IRQ will be left enabled/unmasked in HW.
- device goes to suspend.
- since its enabled/unmasked in HW, it will be able to wake up with this IRQ since GIC will forward this IRQ to CPU to wake it up.

c) if IRQ comes in anytime after (3) is done,

- genirq will find that IRQ was disabled in SW
- driver's IRQ handler will not get called since it was disabled in SW via (1).
- it will mark pending in SW and then really disables in HW now (lazy disable)
- it will also notify suspend PM core about this event to abort the suspend since this IRQ was marked for wakeup.

So, in all cases we are fine.

So while I'm for this patch, I'd suggest a simpler description (assuming my understanding is correct):

There is no reason to implement irq_disable() and irq_mask(). Let's just use irq_mask() and let the rest of the core handle it.

i think current description is fine with above explanation.

Also: it feels really weird to me that you're getting rid of the irq_disable() but keeping irq_enable(). That seems like asking for trouble, though I'd have to do more research to see if I could figure out exactly what might go wrong. Could you remove your irq_enable() too?

-Doug

irq_enable() servers another purpose of clearing any errornous IRQ when enabling it first time, so its kept as it is.

i do not think it causes any troubles.

Add irqchip specific flags for msmgpio irqchip to mask non wakeirqs during suspend and mask before setting irq type.

Masking before changing type should make sure any spurious interrupt is not detected during this operation.

Fixes e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")

Acked-by Linus Walleij <[email protected]>

Signed-off-by Maulik Shah <[email protected]>

---
 drivers/pinctrl/qcom/pinctrl-msm.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/pinctrl/qcom/pinctrl-msm.c +0 -0

index 2419023..b909ffe 100644

Hi,

Quoted 13 lines · Maulik Shah

Add irqchip specific flags for msmgpio irqchip to mask non wakeirqs during suspend and mask before setting irq type.

Masking before changing type should make sure any spurious interrupt is not detected during this operation.

Fixes e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")

Acked-by Linus Walleij <[email protected]>

Signed-off-by Maulik Shah <[email protected]>

---
  drivers/pinctrl/qcom/pinctrl-msm.c | 2 ++
  1 file changed, 2 insertions(+)

drivers/pinctrl/qcom/pinctrl-msm.c static int msm_gpio_init(struct msm_pinctrl *pctrl) · line 1146

Quoted 9 lines

drivers/pinctrl/qcom/pinctrl-msm.c +1 -0

index 2419023..b909ffe 100644
@@ -1143,6 +1143,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
         pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
         pctrl->irq_chip.irq_set_affinity = msm_gpio_irq_set_affinity;
         pctrl->irq_chip.irq_set_vcpu_affinity = msm_gpio_irq_set_vcpu_affinity;
+       pctrl->irq_chip.flags = IRQCHIP_MASK_ON_SUSPEND

I haven't tested it, but with my suggestion in patch #4 to use irq_suspend and irq_resume, I presume adding IRQCHIP_MASK_ON_SUSPEND is no longer needed?

drivers/pinctrl/qcom/pinctrl-msm.c static int msm_gpio_init(struct msm_pinctrl *pctrl) · line 1147

Quoted 3 lines

drivers/pinctrl/qcom/pinctrl-msm.c +1 -0

+                               | IRQCHIP_SET_TYPE_MASKED;

IIUC adding "IRQCHIP_SET_TYPE_MASKED" is unrelated to the rest of this series, right?

-Doug

msmgpio irqchip is not using return value of irq_set_wake call. Start using it.

Fixes e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")

Signed-off-by Maulik Shah <[email protected]>

---
 drivers/pinctrl/qcom/pinctrl-msm.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

msmgpio irqchip is not using return value of irq_set_wake call. Start using it.

Fixes e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")

Signed-off-by Maulik Shah <[email protected]>

Quoted 5 lines · Maulik Shah

msmgpio irqchip is not using return value of irq_set_wake call. Start using it.

Fixes e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")

Signed-off-by Maulik Shah <[email protected]>

Is this something that's causing regressions so I should apply it for fixes, or is it fine to keep this with the rest of the series for v5.9?

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 <[email protected]>

---
 drivers/irqchip/qcom-pdc.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

drivers/irqchip/qcom-pdc.c +10 -14

index 6ae9e1f..8beb6f7 100644
@@ -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)

drivers/irqchip/qcom-pdc.c +3 -5

 }
 
 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

Hi,

Quoted 56 lines · Maulik Shah

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 <[email protected]>

---
  drivers/irqchip/qcom-pdc.c | 34 ++++++++++++++++------------------
  1 file changed, 16 insertions(+), 18 deletions(-)

drivers/irqchip/qcom-pdc.c +10 -14

index 6ae9e1f..8beb6f7 100644
@@ -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)

drivers/irqchip/qcom-pdc.c static void qcom_pdc_gic_mask(struct irq_data *d) · line 112

Quoted 6 lines

drivers/irqchip/qcom-pdc.c +3 -0

@@ -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.

Quoted 36 lines

drivers/irqchip/qcom-pdc.c +3 -5

  }
 
  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

Clear previous kernel's configuration during init by resetting all interrupts in enable bank to zero.

Signed-off-by Maulik Shah <[email protected]>

---
 drivers/irqchip/qcom-pdc.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

drivers/irqchip/qcom-pdc.c +2 -0

index 8beb6f7..11a9d3a 100644
@@ -19,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/types.h>
 
+#define PDC_MAX_IRQS_PER_REG   32
 #define PDC_MAX_IRQS           168
 #define PDC_MAX_GPIO_IRQS      256
 

@@ -339,6 +340,7 @@ static const struct irq_domain_ops qcom_pdc_gpio_ops = {
 static int pdc_setup_pin_mapping(struct device_node *np)
 {
        int ret, n;
+       u32 reg, max_regs, max_pins = 0;
 
        n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
        if (n <= 0 || n % 3)

@@ -367,8 +369,19 @@ static int pdc_setup_pin_mapping(struct device_node *np)
                                                 &pdc_region[n].cnt);
                if (ret)

Hi,

Quoted 31 lines · Maulik Shah

Clear previous kernel's configuration during init by resetting all interrupts in enable bank to zero.

Signed-off-by Maulik Shah <[email protected]>

---
  drivers/irqchip/qcom-pdc.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)

drivers/irqchip/qcom-pdc.c +2 -0

index 8beb6f7..11a9d3a 100644
@@ -19,6 +19,7 @@
  #include <linux/slab.h>
  #include <linux/types.h>
 
+#define PDC_MAX_IRQS_PER_REG   32
  #define PDC_MAX_IRQS           168
  #define PDC_MAX_GPIO_IRQS      256
 

@@ -339,6 +340,7 @@ static const struct irq_domain_ops qcom_pdc_gpio_ops = {
  static int pdc_setup_pin_mapping(struct device_node *np)
  {
         int ret, n;
+       u32 reg, max_regs, max_pins = 0;
 
         n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
         if (n <= 0 || n % 3)

@@ -367,8 +369,19 @@ static int pdc_setup_pin_mapping(struct device_node *np)
                                                  &pdc_region[n].cnt);
                 if (ret)

drivers/irqchip/qcom-pdc.c static int pdc_setup_pin_mapping(struct device_node *np) · line 380

Quoted 10 lines

drivers/irqchip/qcom-pdc.c +7 -0

                         return ret;
+               max_pins += pdc_region[n].cnt;
         }
 
+       if (max_pins > PDC_MAX_IRQS)
+               return -EINVAL;
+
+       max_regs = max_pins / PDC_MAX_IRQS_PER_REG;
+       if (max_pins % PDC_MAX_IRQS_PER_REG)
+               max_regs++;

nit: max_regs = DIV_ROUND_UP(max_pins, PDC_MAX_IRQS_PER_REG)

drivers/irqchip/qcom-pdc.c static int pdc_setup_pin_mapping(struct device_node *np) · line 383

Quoted 2 lines

drivers/irqchip/qcom-pdc.c +2 -0

+       for (reg = 0; reg < max_regs; reg++)
+               pdc_reg_write(IRQ_ENABLE_BANK, reg, 0);

This doesn't feel correct to me, but maybe I'm misunderstanding the hardware (I don't think I have access to a reference manual). Looking at the example in the bindings, I see:

qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;

In that example we have mappings for PDC ports:
0 - 93 (count = 94)
94 - 108 (count = 15)
115 - 121 (count = 7)

Notice the slight discontinuity there. I presume that discontinuity is normal / allowed? If so, if there is enough of it then I think your math could be wrong, though with the example you get lucky and it works out OK. It's easy to see the problem with a slightly different
example: Imagine that you had this:

0 - 33 (count = 34)
94 - 108 (count = 15)
115 - 121 (count = 7)

...now max_pins = 56 and max_regs = 2. So you'll init reg 0 and 1. ...but (IIUC) you actually should be initting 0, 1, 2, and 3.

I have no idea what might be in those discontinuous ranges and if it's always OK to clear, but (assuming it is) one fix is to put your clearing loop _inside_ the other "for" loop in this function, AKA:

for (reg = pdc_region[n].pin_base / PDC_MAX_IRQS_PER_REG;
      reg < DIV_ROUND_UP(pdc_region[n].pin_base + pdc_region[n].cnt),
                         PDC_MAX_IRQS_PER_REG)
      reg++)

...or another option is to keep track of the max "pin_base + cnt" and loop from 0 to there? I just don't know your hardware well enough to tell which would be right.