[PATCH v3 0/5] irqchip: qcom: pdc: Introduce irq_set_wake call

[PATCH v3 0/5] irqchip: qcom: pdc: Introduce irq_set_wake call

[Maulik Shah]

Changes in v3:
- Drop gpiolib change (v2 patch 1) since its already in linux-next
- Add Acked-by Linus Walleij for v2 patch 2 and v2 patch 3.
- Address Stephen's comment to on v2 patch 3
- Address Stephen's comment to change variable to static on v2 patch 4.
- Add a new change to use return value from .irq_set_wake callback
- Add a new change to reset PDC irq enable bank during init time

Changes in v2:
- Fix compiler error on gpiolib patch

This series adds support to lazy disable pdc interrupt.

Some drivers using gpio interrupts want to configure gpio for wakeup using
enable_irq_wake() but during suspend entry disables irq and expects system
to resume when interrupt occurs. In the driver resume call interrupt is
re-enabled and removes wakeup capability using disable_irq_wake() one such
example is cros ec driver.

With [1] in documentation saying "An irq can be disabled with disable_irq()
and still wake the system as long as the irq has wake enabled".

The PDC IRQs are currently "unlazy disabled" (disable here means that it
will be masked in PDC & GIC HW GICD_ISENABLER, the moment driver invokes
disable_irq()) such IRQs can not wakeup from low power modes like suspend
to RAM since the driver chosen to disable this.

During suspend entry, no one re-enable/unmask in HW, even if its marked for
wakeup.

One solutions thought to address this problem was...During suspend entry at
last point, irq chip driver re-enable/unmask IRQs in HW that are marked for
wakeup. This was attemped in [2].

This series adds alternate solution to [2] by "lazy disable" IRQs in HW.
The genirq takes care of lazy disable in case if irqchip did not implement
irq_disable callback. Below is high level steps on how this works out..

a. During driver's disable_irq() call, IRQ will be marked disabled in SW
b. IRQ will still be enabled(read unmasked in HW)
c. The device then enters low power mode like suspend to RAM
d. The HW detects unmasked IRQs and wakesup the CPU
e. During resume after local_irq_enable() CPU goes to handle the wake IRQ
f. Generic handler comes to know that IRQ is disabled in SW
g. Generic handler marks IRQ as pending and now invokes mask callback
h. IRQ gets disabled/masked in HW now
i. When driver invokes enable_irq() the SW pending IRQ leads IRQ's handler
j. enable_irq() will again enable/unmask in HW

[1] https://www.spinics.net/lists/kernel/msg3398294.html
[2] https://patchwork.kernel.org/patch/11466021/

Maulik Shah (5):
  pinctrl: qcom: Remove irq_disable callback from msmgpio irqchip
  pinctrl: qcom: Add msmgpio irqchip flags
  pinctrl: qcom: Use return value from irq_set_wake call
  irqchip: qcom-pdc: Introduce irq_set_wake call
  irqchip: qcom-pdc: Reset all pdc interrupts during init

 drivers/irqchip/qcom-pdc.c         | 47 +++++++++++++++++++++++---------------
 drivers/pinctrl/qcom/pinctrl-msm.c | 23 ++++---------------
 2 files changed, 34 insertions(+), 36 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 1/5] pinctrl: qcom: Remove irq_disable callback from msmgpio irqchip

[Maulik Shah]

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 <linus.walleij@linaro.org>
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 83b7d64..2419023 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -815,18 +815,6 @@ static void msm_gpio_irq_enable(struct irq_data *d)
 	msm_gpio_irq_clear_unmask(d, true);
 }
 
-static void msm_gpio_irq_disable(struct irq_data *d)
-{
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
-
-	if (d->parent_data)
-		irq_chip_disable_parent(d);
-
-	if (!test_bit(d->hwirq, pctrl->skip_wake_irqs))
-		msm_gpio_irq_mask(d);
-}
-
 static void msm_gpio_irq_unmask(struct irq_data *d)
 {
 	msm_gpio_irq_clear_unmask(d, false);
@@ -1146,7 +1134,6 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 
 	pctrl->irq_chip.name = "msmgpio";
 	pctrl->irq_chip.irq_enable = msm_gpio_irq_enable;
-	pctrl->irq_chip.irq_disable = msm_gpio_irq_disable;
 	pctrl->irq_chip.irq_mask = msm_gpio_irq_mask;
 	pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask;
 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 2/5] pinctrl: qcom: Add msmgpio irqchip flags

[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 <linus.walleij@linaro.org>
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 2419023..b909ffe 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -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
+				| IRQCHIP_SET_TYPE_MASKED;
 
 	np = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0);
 	if (np) {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 3/5] pinctrl: qcom: Use return value from irq_set_wake call

[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 <mkshah@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index b909ffe..92fe7d6 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -978,12 +978,10 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
 	 * when TLMM is powered on. To allow that, enable the GPIO
 	 * summary line to be wakeup capable at GIC.
 	 */
-	if (d->parent_data)
-		irq_chip_set_wake_parent(d, on);
-
-	irq_set_irq_wake(pctrl->irq, on);
+	if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
+		return irq_chip_set_wake_parent(d, on);
 
-	return 0;
+	return irq_set_irq_wake(pctrl->irq, on);
 }
 
 static int msm_gpio_irq_reqres(struct irq_data *d)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 4/5] irqchip: qcom-pdc: Introduce irq_set_wake call

[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 <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);
 }
 
 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

[PATCH v3 5/5] irqchip: qcom-pdc: Reset all pdc interrupts during init

[Maulik Shah]

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

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/irqchip/qcom-pdc.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 8beb6f7..11a9d3a 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -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)
 			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++;
+
+	for (reg = 0; reg < max_regs; reg++)
+		pdc_reg_write(IRQ_ENABLE_BANK, reg, 0);
+
 	return 0;
 }
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 4/5] irqchip: qcom-pdc: Introduce irq_set_wake call

[Doug Anderson]

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
>

Re: [PATCH v3 5/5] irqchip: qcom-pdc: Reset all pdc interrupts during init

[Doug Anderson]

Hi,

On Mon, Jun 22, 2020 at 2:33 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> Clear previous kernel's configuration during init by resetting
> all interrupts in enable bank to zero.
>
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
>  drivers/irqchip/qcom-pdc.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index 8beb6f7..11a9d3a 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -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)
>                         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)


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

Re: [PATCH v3 3/5] pinctrl: qcom: Use return value from irq_set_wake call

[Doug Anderson]

Hi,

On Mon, Jun 22, 2020 at 2:32 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> 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 <mkshah@codeaurora.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)

Seems right to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v3 2/5] pinctrl: qcom: Add msmgpio irqchip flags

[Doug Anderson]

Hi,

On Mon, Jun 22, 2020 at 2:32 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> 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 <linus.walleij@linaro.org>
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 2419023..b909ffe 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -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?


> +                               | IRQCHIP_SET_TYPE_MASKED;

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

-Doug

Re: [PATCH v3 1/5] pinctrl: qcom: Remove irq_disable callback from msmgpio irqchip

[Doug Anderson]

Hi,

On Mon, Jun 22, 2020 at 2:32 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> 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 <linus.walleij@linaro.org>
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 13 -------------
>  1 file changed, 13 deletions(-)

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

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.


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.


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

Re: [PATCH v3 2/5] pinctrl: qcom: Add msmgpio irqchip flags

[Maulik Shah]

Hi,

On 7/14/2020 3:47 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Jun 22, 2020 at 2:32 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>> 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 <linus.walleij@linaro.org>
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>> ---
>>   drivers/pinctrl/qcom/pinctrl-msm.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index 2419023..b909ffe 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>> @@ -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?
it will still be needed, to let the non wakeup capable IRQ masked during 
suspend.
>
>
>> +                               | IRQCHIP_SET_TYPE_MASKED;
> IIUC adding "IRQCHIP_SET_TYPE_MASKED" is unrelated to the rest of this
> series, right?

Right, but since we are adding missing flags, i added it together.

Thanks,
Maulik

>
> -Doug

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 4/5] irqchip: qcom-pdc: Introduce irq_set_wake call

[Maulik Shah]

Hi,

On 7/14/2020 3:46 AM, Doug Anderson wrote:
> 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".
No its not broken, it works as expected. after step #4, interrupt will fire.
>
> 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.

I too thought of using syscore ops earlier, but syscore ops won't work 
if device chooses to enter "s2idle" suspend state since they are not 
invoked in s2idle entry path.

even if you register for "irq/generic-chip.c" , this driver too 
registers for syscore ops only, it won't work if you enter s2idle 
suspend state.

Current patch works fine with both s2idle and deep suspend states.

Thanks,
Maulik
>
>
>
>>   }
>>
>>   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
>>
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 5/5] irqchip: qcom-pdc: Reset all pdc interrupts during init

[Maulik Shah]

Hi,

On 7/14/2020 3:47 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Jun 22, 2020 at 2:33 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>> Clear previous kernel's configuration during init by resetting
>> all interrupts in enable bank to zero.
>>
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>> ---
>>   drivers/irqchip/qcom-pdc.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
>> index 8beb6f7..11a9d3a 100644
>> --- a/drivers/irqchip/qcom-pdc.c
>> +++ b/drivers/irqchip/qcom-pdc.c
>> @@ -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)
>>                          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)
>
>
>> +       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.

Right, Thanks for cacthing this. I will fix in next revision.

Thanks,
Maulik

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

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 1/5] pinctrl: qcom: Remove irq_disable callback from msmgpio irqchip

[Doug Anderson]

Hi,

On Tue, Jul 14, 2020 at 3:38 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> Hi,
>
> On 7/14/2020 3:47 AM, Doug Anderson wrote:
>
> Hi,
>
> On Mon, Jun 22, 2020 at 2:32 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> > > 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 <linus.walleij@linaro.org>
> > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> > > ---
> > >  drivers/pinctrl/qcom/pinctrl-msm.c | 13 -------------
> > >  1 file changed, 13 deletions(-)
> >
> > 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..

I hadn't followed all the previous discussions, but generally if two
people are independently confused by the same thing it's a sign that
it needs to be explained better.  In this case that means a better
commit message that explains exactly why this isn't a problem.


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

OK, I think I understand what you're trying to say.  What you're
saying is that the important thing is that when you're using the
kernel's "lazy" mode that the kernel will have knowledge of whether a
disabled IRQ is pending.  That's because the IRQ fired once (and the
kernel set IRQS_PENDING) before it got masked.  If we're using a
non-lazy case then the IRQ could be pending but the kernel wouldn't
know.

If that's what you're relying on for this patch to work then it
belongs in the commit message.

...but (see below) I don't think it's working like you think it does.


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

...but what if it's an edge interrupt?  Then:

1. It will fire.
2. It will get marked as IRQS_PENDING and Acked.
3. It will get masked.
4. Your code will unmask and wake up from future edges but the edge
that already came in won't cause a wakeup, right?

Hrm, though I guess that's just a problem in general.  Probably
suspend_device_irq() should check to see if an IRQ is pending?  In any
case, at this point in time it's not a bug that is affecting me since
(right now) cros_ec sets the wakeup _before_ disabling, but it
probably should still be fixed.


>     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 I tested this and it didn't seem to work.  I went into
cros_ec_suspend() and added a delay after the disable_irq() call.  I
pressed a key on my keyboard while the delay was happening.  When I
pressed the key I saw qcom_pdc_gic_mask() get called for the
interrupt.  ...and the suspend was _not_ aborted.  I watched and the
system continued all the way.

I watched the system go all the way down and shut down the CPUs.
Then, after 6 seconds (!) it woke back up.  I don't quite understand
it, but the 6 seconds here seems to be the time needed to wakeup if
PDC is enabled but the interrupt is masked at the gic level.

Digging a little deeper, I see that in irq_may_run() it was getting true for:

!irqd_has_set(&desc->irq_data, mask)

...and thus it was returning true for irq_may_run() without setting
irq_pm_check_wakeup().

Given that it's not working as you describe it to be working, I feel
like you might need to go back and re-evaluate your approach.  I'll
try to spend more time thinking about this tomorrow too, but I'm about
out of time for the day.


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

As per above, if nothing else you need to clarify things so people
aren't so confused.


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

I kinda ran out of time to dig more here, but it still worries me.
I'll try to dig more tomorrow.  In any case, can't you just clear out
any erroneous IRQs at init time and make it symmetric?

-Doug

Re: [PATCH v3 3/5] pinctrl: qcom: Use return value from irq_set_wake call

[Linus Walleij]

On Mon, Jun 22, 2020 at 11:32 AM Maulik Shah <mkshah@codeaurora.org> wrote:

> 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 <mkshah@codeaurora.org>

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?

Yours,
Linus Walleij

Re: [PATCH v3 3/5] pinctrl: qcom: Use return value from irq_set_wake call

[Doug Anderson]

Hi,

On Thu, Jul 16, 2020 at 6:19 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Mon, Jun 22, 2020 at 11:32 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> > 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 <mkshah@codeaurora.org>
>
> 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?

I would let Maulik comment more, but as far as I can tell the function
has been ignoring the return value of irq_set_irq_wake() for much
longer.  Presumably one could logically say:

Fixes: 6aced33f4974 ("pinctrl: msm: drop wake_irqs bitmap")

...though when you get past the commit that Maulik tagged you need a
backport rather than a straight cherry-pick.

That would make me believe that there is no real hurry to land the fix here.


-Doug