[PATCH v2 0/4] irqchip: qcom: pdc: Introduce irq_set_wake call

[PATCH v2 0/4] irqchip: qcom: pdc: Introduce irq_set_wake call

[Maulik Shah]

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 (4):
  gpio: gpiolib: Allow GPIO IRQs to lazy disable
  pinctrl: qcom: Remove irq_disable callback from msmgpio irqchip
  pinctrl: qcom: Add msmgpio irqchip flags
  irqchip: qcom-pdc: Introduce irq_set_wake call

 drivers/gpio/gpiolib.c             | 55 +++++++++++++++++++++++++-------------
 drivers/irqchip/qcom-pdc.c         | 33 ++++++++++++-----------
 drivers/pinctrl/qcom/pinctrl-msm.c | 15 ++---------
 include/linux/gpio/driver.h        | 13 +++++++++
 4 files changed, 68 insertions(+), 48 deletions(-)

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

[PATCH v2 1/4] gpio: gpiolib: Allow GPIO IRQs to lazy disable

[Maulik Shah]

With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib
overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable
callback is implemented then genirq takes unlazy path to disable irq.

Underlying irqchip may not want to implement irq_disable callback to lazy
disable irq when client drivers invokes disable_irq(). By overriding
irq_disable callback, gpiolib ends up always unlazy disabling IRQ.

Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only
if irqchip implemented irq_disable. In cases where irq_disable is not
implemented irq_mask is overridden. Similarly override irq_enable callback
only if irqchip implemented irq_enable otherwise irq_unmask is overridden.

Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable)
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/gpio/gpiolib.c      | 55 +++++++++++++++++++++++++++++----------------
 include/linux/gpio/driver.h | 13 +++++++++++
 2 files changed, 49 insertions(+), 19 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index eaa0e20..3810cd0 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2465,32 +2465,37 @@ static void gpiochip_irq_relres(struct irq_data *d)
 	gpiochip_relres_irq(gc, d->hwirq);
 }
 
+static void gpiochip_irq_mask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	if (gc->irq.irq_mask)
+		gc->irq.irq_mask(d);
+	gpiochip_disable_irq(gc, d->hwirq);
+}
+
+static void gpiochip_irq_unmask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	gpiochip_enable_irq(gc, d->hwirq);
+	if (gc->irq.irq_unmask)
+		gc->irq.irq_unmask(d);
+}
+
 static void gpiochip_irq_enable(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 
 	gpiochip_enable_irq(gc, d->hwirq);
-	if (gc->irq.irq_enable)
-		gc->irq.irq_enable(d);
-	else
-		gc->irq.chip->irq_unmask(d);
+	gc->irq.irq_enable(d);
 }
 
 static void gpiochip_irq_disable(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 
-	/*
-	 * Since we override .irq_disable() we need to mimic the
-	 * behaviour of __irq_disable() in irq/chip.c.
-	 * First call .irq_disable() if it exists, else mimic the
-	 * behaviour of mask_irq() which calls .irq_mask() if
-	 * it exists.
-	 */
-	if (gc->irq.irq_disable)
-		gc->irq.irq_disable(d);
-	else if (gc->irq.chip->irq_mask)
-		gc->irq.chip->irq_mask(d);
+	gc->irq.irq_disable(d);
 	gpiochip_disable_irq(gc, d->hwirq);
 }
 
@@ -2515,10 +2520,22 @@ static void gpiochip_set_irq_hooks(struct gpio_chip *gc)
 			  "detected irqchip that is shared with multiple gpiochips: please fix the driver.\n");
 		return;
 	}
-	gc->irq.irq_enable = irqchip->irq_enable;
-	gc->irq.irq_disable = irqchip->irq_disable;
-	irqchip->irq_enable = gpiochip_irq_enable;
-	irqchip->irq_disable = gpiochip_irq_disable;
+
+	if (irqchip->irq_disable) {
+		gc->irq.irq_disable = irqchip->irq_disable;
+		irqchip->irq_disable = gpiochip_irq_disable;
+	} else {
+		gc->irq.irq_mask = irqchip->irq_mask;
+		irqchip->irq_mask = gpiochip_irq_mask;
+	}
+
+	if (irqchip->irq_enable) {
+		gc->irq.irq_enable = irqchip->irq_enable;
+		irqchip->irq_enable = gpiochip_irq_enable;
+	} else {
+		gc->irq.irq_unmask = irqchip->irq_unmask;
+		irqchip->irq_unmask = gpiochip_irq_unmask;
+	}
 }
 
 /**
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 8c41ae4..c8bcaf3 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -253,6 +253,19 @@ struct gpio_irq_chip {
 	 * Store old irq_chip irq_disable callback
 	 */
 	void		(*irq_disable)(struct irq_data *data);
+	/**
+	 * @irq_unmask:
+	 *
+	 * Store old irq_chip irq_unmask callback
+	 */
+	void		(*irq_unmask)(struct irq_data *data);
+
+	/**
+	 * @irq_mask:
+	 *
+	 * Store old irq_chip irq_mask callback
+	 */
+	void		(*irq_mask)(struct irq_data *data);
 };
 
 /**
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 2/4] 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.

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 v2 3/4] 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.

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 v2 4/4] 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 | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 6ae9e1f..f7c0662 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -36,6 +36,7 @@ struct pdc_pin_region {
 	u32 cnt;
 };
 
+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,20 @@ 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);
-}
+		return 0;
 
-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)
@@ -110,6 +109,9 @@ static void qcom_pdc_gic_mask(struct irq_data *d)
 	if (d->hwirq == GPIO_NO_WAKE_IRQ)
 		return;
 
+	if (!test_bit(d->hwirq, pdc_wake_irqs))
+		pdc_enable_intr(d, false);
+
 	irq_chip_mask_parent(d);
 }
 
@@ -118,6 +120,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 +200,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 v2 1/4] gpio: gpiolib: Allow GPIO IRQs to lazy disable

[Linus Walleij]

On Sat, May 23, 2020 at 7:11 PM Maulik Shah <mkshah@codeaurora.org> wrote:

> With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib
> overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable
> callback is implemented then genirq takes unlazy path to disable irq.
>
> Underlying irqchip may not want to implement irq_disable callback to lazy
> disable irq when client drivers invokes disable_irq(). By overriding
> irq_disable callback, gpiolib ends up always unlazy disabling IRQ.
>
> Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only
> if irqchip implemented irq_disable. In cases where irq_disable is not
> implemented irq_mask is overridden. Similarly override irq_enable callback
> only if irqchip implemented irq_enable otherwise irq_unmask is overridden.
>
> Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable)
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>

I definitely want Hans Verkuils test and review on this, since it
is a usecase that he is really dependent on.

Also the irqchip people preferredly.

But it does seem to mop up my mistakes and fix this up properly!

So with some testing I'll be happy to merge it, even this one
patch separately if Hans can verify that it works.

Yours,
Linus Walleij

Re: [PATCH v2 1/4] gpio: gpiolib: Allow GPIO IRQs to lazy disable

[Hans Verkuil]

On 25/05/2020 13:55, Linus Walleij wrote:
> On Sat, May 23, 2020 at 7:11 PM Maulik Shah <mkshah@codeaurora.org> wrote:
> 
>> With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib
>> overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable
>> callback is implemented then genirq takes unlazy path to disable irq.
>>
>> Underlying irqchip may not want to implement irq_disable callback to lazy
>> disable irq when client drivers invokes disable_irq(). By overriding
>> irq_disable callback, gpiolib ends up always unlazy disabling IRQ.
>>
>> Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only
>> if irqchip implemented irq_disable. In cases where irq_disable is not
>> implemented irq_mask is overridden. Similarly override irq_enable callback
>> only if irqchip implemented irq_enable otherwise irq_unmask is overridden.
>>
>> Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable)
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> 
> I definitely want Hans Verkuils test and review on this, since it
> is a usecase that he is really dependent on.

Maulik, since I am no longer subscribed to linux-gpio, can you mail the
series to me?

I have two use-cases, but I can only test one (I don't have access to the
SBC I need to test the other use-case for the next few months).

Once I have the whole series I'll try to test the first use-case and at
least look into the code if this series could affect the second use-case.

Regards,

	Hans

> 
> Also the irqchip people preferredly.
> 
> But it does seem to mop up my mistakes and fix this up properly!
> 
> So with some testing I'll be happy to merge it, even this one
> patch separately if Hans can verify that it works.
> 
> Yours,
> Linus Walleij
> 

Re: [PATCH v2 1/4] gpio: gpiolib: Allow GPIO IRQs to lazy disable

[Maulik Shah]

Hi,

On 5/25/2020 5:52 PM, Hans Verkuil wrote:
> On 25/05/2020 13:55, Linus Walleij wrote:
>> On Sat, May 23, 2020 at 7:11 PM Maulik Shah <mkshah@codeaurora.org> wrote:
>>
>>> With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib
>>> overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable
>>> callback is implemented then genirq takes unlazy path to disable irq.
>>>
>>> Underlying irqchip may not want to implement irq_disable callback to lazy
>>> disable irq when client drivers invokes disable_irq(). By overriding
>>> irq_disable callback, gpiolib ends up always unlazy disabling IRQ.
>>>
>>> Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only
>>> if irqchip implemented irq_disable. In cases where irq_disable is not
>>> implemented irq_mask is overridden. Similarly override irq_enable callback
>>> only if irqchip implemented irq_enable otherwise irq_unmask is overridden.
>>>
>>> Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable)
>>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>> I definitely want Hans Verkuils test and review on this, since it
>> is a usecase that he is really dependent on.
> Maulik, since I am no longer subscribed to linux-gpio, can you mail the
> series to me?
>
> I have two use-cases, but I can only test one (I don't have access to the
> SBC I need to test the other use-case for the next few months).
>
> Once I have the whole series I'll try to test the first use-case and at
> least look into the code if this series could affect the second use-case.
>
> Regards,
>
> 	Hans

Hi Hans,

Mailed you the entire series.

Thanks,
Maulik
>
>> Also the irqchip people preferredly.
>>
>> But it does seem to mop up my mistakes and fix this up properly!
>>
>> So with some testing I'll be happy to merge it, even this one
>> patch separately if Hans can verify that it works.
>>
>> Yours,
>> Linus Walleij
>>
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 1/4] gpio: gpiolib: Allow GPIO IRQs to lazy disable

[Stephen Boyd]

Quoting Maulik Shah (2020-05-23 10:11:10)
> With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib
> overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable
> callback is implemented then genirq takes unlazy path to disable irq.
> 
> Underlying irqchip may not want to implement irq_disable callback to lazy
> disable irq when client drivers invokes disable_irq(). By overriding
> irq_disable callback, gpiolib ends up always unlazy disabling IRQ.
> 
> Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only
> if irqchip implemented irq_disable. In cases where irq_disable is not
> implemented irq_mask is overridden. Similarly override irq_enable callback
> only if irqchip implemented irq_enable otherwise irq_unmask is overridden.
> 
> Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable)

This isn't a proper Fixes line. Should have quotes

Fixes: 461c1a7d4733 ("gpiolib: override irq_enable/disable")

> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
>  drivers/gpio/gpiolib.c      | 55 +++++++++++++++++++++++++++++----------------
>  include/linux/gpio/driver.h | 13 +++++++++++
>  2 files changed, 49 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index eaa0e20..3810cd0 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2465,32 +2465,37 @@ static void gpiochip_irq_relres(struct irq_data *d)
>         gpiochip_relres_irq(gc, d->hwirq);
>  }
>  
> +static void gpiochip_irq_mask(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +
> +       if (gc->irq.irq_mask)
> +               gc->irq.irq_mask(d);
> +       gpiochip_disable_irq(gc, d->hwirq);

How does this work in the lazy case when I want to drive the GPIO? Say I
have a GPIO that is also an interrupt. The code would look like

 struct gpio_desc *gpio = gpiod_get(...)
 unsigned int girq = gpiod_to_irq(gpio)

 request_irq(girq, ...);

 disable_irq(girq);
 gpiod_direction_output(gpio, 1);

In the lazy case genirq wouldn't call the mask function until the first
interrupt arrived on the GPIO line. If that never happened then wouldn't
we be blocked in gpiod_direction_output() when the test_bit() sees
FLAG_USED_AS_IRQ? Or do we need irqs to be released before driving
gpios?

> +}
> +
> +static void gpiochip_irq_unmask(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +
> +       gpiochip_enable_irq(gc, d->hwirq);
> +       if (gc->irq.irq_unmask)
> +               gc->irq.irq_unmask(d);
> +}
> +

Re: [PATCH v2 3/4] pinctrl: qcom: Add msmgpio irqchip flags

[Stephen Boyd]

Quoting Maulik Shah (2020-05-23 10:11:12)
> Add irqchip specific flags for msmgpio irqchip to mask non wakeirqs
> during suspend and mask before setting irq type.

Why do we need to mask before setting irq type? Does something go wrong?
Can you explain in the commit text?

> 
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>

Does this need a Fixes tag?

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

This is sort of sad. We have to set the IRQCHIP_MASK_ON_SUSPEND flag
here so that genirq can call the mask op during suspend for the parent
irqchip (pdc)? Is there some way to not need to do that and instead let
genirq do mask on suspend at the chip level instead of the irq level?

> +                               | IRQCHIP_SET_TYPE_MASKED;
>  
>         np = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0);
>         if (np) {

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

[Stephen Boyd]

Quoting Maulik Shah (2020-05-23 10:11:13)
> 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 | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index 6ae9e1f..f7c0662 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -36,6 +36,7 @@ struct pdc_pin_region {
>         u32 cnt;
>  };
>  
> +DECLARE_BITMAP(pdc_wake_irqs, PDC_MAX_IRQS);

static?

>  static DEFINE_RAW_SPINLOCK(pdc_lock);
>  static void __iomem *pdc_base;
>  static struct pdc_pin_region *pdc_region;
> @@ -87,22 +88,20 @@ 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);
> -}
> +               return 0;

Shouldn't this fail if we can't set for wake?

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

The diff is really hard to read too. Maybe set_wake can be added first
and then the enable/disable functions removed?

> @@ -110,6 +109,9 @@ static void qcom_pdc_gic_mask(struct irq_data *d)
>         if (d->hwirq == GPIO_NO_WAKE_IRQ)
>                 return;
>  
> +       if (!test_bit(d->hwirq, pdc_wake_irqs))
> +               pdc_enable_intr(d, false);
> +
>         irq_chip_mask_parent(d);
>  }
>  
> @@ -118,6 +120,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);
>  }
>  

I find these two hunks deeply confusing. I'm not sure what the
maintainers think though. I hope it would be simpler to always enable
the hwirqs in the pdc when an irq is requested and only disable it in
the pdc when the system goes to suspend and the pdc pin isn't for an irq
that's marked for wakeup. Does that break somehow?

My understanding of the hardware is that the GPIO controller has lines
directly connected to various SPI lines on the GIC and PDC has a way to
monitor those direct connections and wakeup the CPUs when they trigger
the detection logic in the PDC. The enable/disable bit in PDC gates that
logic for each wire between the GPIO controller and the GIC.

So isn't it simpler to leave the PDC monitoring pins that we care about
all the time and only stop monitoring when we enter and leave suspend?
And shouldn't the driver set something sane in qcom_pdc_init() to
disable all the pdc pins so that we don't rely on boot state to
configure pins for wakeup?

> @@ -197,15 +200,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,

Re: [PATCH v2 1/4] gpio: gpiolib: Allow GPIO IRQs to lazy disable

[Hans Verkuil]

On 25/05/2020 13:55, Linus Walleij wrote:
> On Sat, May 23, 2020 at 7:11 PM Maulik Shah <mkshah@codeaurora.org> wrote:
> 
>> With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib
>> overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable
>> callback is implemented then genirq takes unlazy path to disable irq.
>>
>> Underlying irqchip may not want to implement irq_disable callback to lazy
>> disable irq when client drivers invokes disable_irq(). By overriding
>> irq_disable callback, gpiolib ends up always unlazy disabling IRQ.
>>
>> Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only
>> if irqchip implemented irq_disable. In cases where irq_disable is not
>> implemented irq_mask is overridden. Similarly override irq_enable callback
>> only if irqchip implemented irq_enable otherwise irq_unmask is overridden.
>>
>> Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable)
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> 
> I definitely want Hans Verkuils test and review on this, since it
> is a usecase that he is really dependent on.

For this patch:

Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

However, I discovered that patch 256efaea1fdc ("gpiolib: fix up emulated
open drain outputs") broke the cec-gpio driver on the Raspberry Pi starting
with kernel v5.5.

The CEC pin is an open drain pin that is used in both input and output
directions and has an interrupt (which is of course disabled while in
output mode).

With this patch the interrupt can no longer be requested:

[    4.157806] gpio gpiochip0: (pinctrl-bcm2835): gpiochip_lock_as_irq: tried to flag a GPIO set as output for IRQ

[    4.168086] gpio gpiochip0: (pinctrl-bcm2835): unable to lock HW IRQ 7 for IRQ
[    4.175425] genirq: Failed to request resources for cec-gpio@7 (irq 79) on irqchip pinctrl-bcm2835
[    4.184597] cec-gpio: probe of cec-gpio@7 failed with error -5

You can easily test this with a RPi by enabling cec-gpio:

------------------------------------------------------
diff --git a/arch/arm/boot/dts/bcm2837-rpi-3-b.dts b/arch/arm/boot/dts/bcm2837-rpi-3-b.dts
index 054ecaa355c9..87d6ed711ce2 100644
--- a/arch/arm/boot/dts/bcm2837-rpi-3-b.dts
+++ b/arch/arm/boot/dts/bcm2837-rpi-3-b.dts
@@ -29,6 +29,13 @@ wifi_pwrseq: wifi-pwrseq {
 		compatible = "mmc-pwrseq-simple";
 		reset-gpios = <&expgpio 1 GPIO_ACTIVE_LOW>;
 	};
+
+	cec-gpio@7 {
+		compatible = "cec-gpio";
+		cec-gpios = <&gpio 7 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
+		hpd-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>;
+		v5-gpios = <&gpio 22 GPIO_ACTIVE_HIGH>;
+	};
 };

 &firmware {
------------------------------------------------------

Reverting that patch makes it work again.

I'm happy to test a fix for this.

Regards,

	Hans

> 
> Also the irqchip people preferredly.
> 
> But it does seem to mop up my mistakes and fix this up properly!
> 
> So with some testing I'll be happy to merge it, even this one
> patch separately if Hans can verify that it works.
> 
> Yours,
> Linus Walleij
> 

Re: [PATCH v2 1/4] gpio: gpiolib: Allow GPIO IRQs to lazy disable

[Maulik Shah]

Hi,

On 5/27/2020 3:14 PM, Stephen Boyd wrote:
> Quoting Maulik Shah (2020-05-23 10:11:10)
>> With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib
>> overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable
>> callback is implemented then genirq takes unlazy path to disable irq.
>>
>> Underlying irqchip may not want to implement irq_disable callback to lazy
>> disable irq when client drivers invokes disable_irq(). By overriding
>> irq_disable callback, gpiolib ends up always unlazy disabling IRQ.
>>
>> Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only
>> if irqchip implemented irq_disable. In cases where irq_disable is not
>> implemented irq_mask is overridden. Similarly override irq_enable callback
>> only if irqchip implemented irq_enable otherwise irq_unmask is overridden.
>>
>> Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable)
> This isn't a proper Fixes line. Should have quotes
>
> Fixes: 461c1a7d4733 ("gpiolib: override irq_enable/disable")
Thanks for pointing this, i will address in next revision.
>
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>> ---
>>   drivers/gpio/gpiolib.c      | 55 +++++++++++++++++++++++++++++----------------
>>   include/linux/gpio/driver.h | 13 +++++++++++
>>   2 files changed, 49 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index eaa0e20..3810cd0 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -2465,32 +2465,37 @@ static void gpiochip_irq_relres(struct irq_data *d)
>>          gpiochip_relres_irq(gc, d->hwirq);
>>   }
>>   
>> +static void gpiochip_irq_mask(struct irq_data *d)
>> +{
>> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> +
>> +       if (gc->irq.irq_mask)
>> +               gc->irq.irq_mask(d);
>> +       gpiochip_disable_irq(gc, d->hwirq);
> How does this work in the lazy case when I want to drive the GPIO? Say I
> have a GPIO that is also an interrupt. The code would look like
>
>   struct gpio_desc *gpio = gpiod_get(...)
>   unsigned int girq = gpiod_to_irq(gpio)
>
>   request_irq(girq, ...);
>
>   disable_irq(girq);
>   gpiod_direction_output(gpio, 1);
>
> In the lazy case genirq wouldn't call the mask function until the first
> interrupt arrived on the GPIO line. If that never happened then wouldn't
> we be blocked in gpiod_direction_output() when the test_bit() sees
> FLAG_USED_AS_IRQ? Or do we need irqs to be released before driving
> gpios?

The client driver can decide to unlazy disable IRQ with below API...

  irq_set_status_flags(girq, IRQ_DISABLE_UNLAZY);

This will immediatly invoke mask function (unlazy disable) from genirq, 
even though irq_disable is not implemented.

Thanks,
Maulik
>
>> +}
>> +
>> +static void gpiochip_irq_unmask(struct irq_data *d)
>> +{
>> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> +
>> +       gpiochip_enable_irq(gc, d->hwirq);
>> +       if (gc->irq.irq_unmask)
>> +               gc->irq.irq_unmask(d);
>> +}
>> +

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

Re: [PATCH v2 3/4] pinctrl: qcom: Add msmgpio irqchip flags

[Maulik Shah]

Hi,

On 5/27/2020 3:17 PM, Stephen Boyd wrote:
> Quoting Maulik Shah (2020-05-23 10:11:12)
>> Add irqchip specific flags for msmgpio irqchip to mask non wakeirqs
>> during suspend and mask before setting irq type.
> Why do we need to mask before setting irq type? Does something go wrong?
> Can you explain in the commit text?

i don't think anything goes wrong but there might be a case where some 
driver changing type at runtime,

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

>
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> Does this need a Fixes tag?
Thanks i will add.
>
>> ---
>>   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
> This is sort of sad. We have to set the IRQCHIP_MASK_ON_SUSPEND flag
> here so that genirq can call the mask op during suspend for the parent
> irqchip (pdc)?
During suspend, suspend_device_irq() will check this flag in msmgpio 
irqchip and then call it to mask if its not marked for wakeup.

in this case, setting this flag will call first invoke gpiolib's 
callback  (we override in first patch of this series), then it goes to 
msmgpio chip's mask callback,

this call will then get forwarded to its parent PDC and then to PDC's 
parent GIC.

This seems the way hierarchical irqchip works. i don't see any issue 
with this.
> Is there some way to not need to do that and instead let
> genirq do mask on suspend at the chip level instead of the irq level?
>
>> +                               | 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

Re: [PATCH v2 1/4] gpio: gpiolib: Allow GPIO IRQs to lazy disable

[Linus Walleij]

On Wed, May 27, 2020 at 12:38 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:

> However, I discovered that patch 256efaea1fdc ("gpiolib: fix up emulated
> open drain outputs") broke the cec-gpio driver on the Raspberry Pi starting
> with kernel v5.5.
>
> The CEC pin is an open drain pin that is used in both input and output
> directions and has an interrupt (which is of course disabled while in
> output mode).
>
> With this patch the interrupt can no longer be requested:
>
> [    4.157806] gpio gpiochip0: (pinctrl-bcm2835): gpiochip_lock_as_irq: tried to flag a GPIO set as output for IRQ
>
> [    4.168086] gpio gpiochip0: (pinctrl-bcm2835): unable to lock HW IRQ 7 for IRQ
> [    4.175425] genirq: Failed to request resources for cec-gpio@7 (irq 79) on irqchip pinctrl-bcm2835
> [    4.184597] cec-gpio: probe of cec-gpio@7 failed with error -5

There is nothing conceptually wrong with that patch so I think we
need to have the irqchip code check if it is input *OR* open drain.

I'll send a separate patch for this, it should be an easy fix.

Yours,
Linus Walleij

Re: [PATCH v2 1/4] gpio: gpiolib: Allow GPIO IRQs to lazy disable

[Russell King - ARM Linux admin]

On Wed, May 27, 2020 at 03:56:16PM +0200, Linus Walleij wrote:
> On Wed, May 27, 2020 at 12:38 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
> > However, I discovered that patch 256efaea1fdc ("gpiolib: fix up emulated
> > open drain outputs") broke the cec-gpio driver on the Raspberry Pi starting
> > with kernel v5.5.
> >
> > The CEC pin is an open drain pin that is used in both input and output
> > directions and has an interrupt (which is of course disabled while in
> > output mode).
> >
> > With this patch the interrupt can no longer be requested:
> >
> > [    4.157806] gpio gpiochip0: (pinctrl-bcm2835): gpiochip_lock_as_irq: tried to flag a GPIO set as output for IRQ
> >
> > [    4.168086] gpio gpiochip0: (pinctrl-bcm2835): unable to lock HW IRQ 7 for IRQ
> > [    4.175425] genirq: Failed to request resources for cec-gpio@7 (irq 79) on irqchip pinctrl-bcm2835
> > [    4.184597] cec-gpio: probe of cec-gpio@7 failed with error -5
> 
> There is nothing conceptually wrong with that patch so I think we
> need to have the irqchip code check if it is input *OR* open drain.

Right - because, before this patch:

- if you have hardware that is capable of open-drain outputs, gpiolib
  will report that the GPIO is in *output* mode.
- if you have hardware that is not capable of open-drain outputs, and
  gpiolib emulates the open-drain nature by switching the GPIO
  direction, then gpiolib will report that the GPIO is in input mode.

What my patch does is provide consistent behaviour across all cases
by making open-drain outputs consistently report output mode
irrespective of the underlying hardware.

Whether an open-drain GPIO should be viewed as an input or as an output
is an interesting question, but the important thing as far as this
subsystem goes is to have consistent behaviour, otherwise having a
subsystem is utterly pointless.  However, consider whether it is sane
to request a change of state via gpiod_set_value() of a gpio that
reports itself as input, and have that request honoured and cause a
change of state of the "input" - clearly that is not sane.

In any case:

        cec->cec_gpio = devm_gpiod_get(dev, "cec", GPIOD_OUT_HIGH_OPEN_DRAIN);
        if (IS_ERR(cec->cec_gpio))
                return PTR_ERR(cec->cec_gpio);
        cec->cec_irq = gpiod_to_irq(cec->cec_gpio);
...
        ret = devm_request_irq(dev, cec->cec_irq, cec_gpio_irq_handler,
                               IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
                               cec->adap->name, cec);

So, the GPIO is requested in an _output_ mode, so it would be weird if
it were subsequently to be reported as an input just because it ended
up being an emulated open-drain output.

Hence, the above code would have failed if cec-gpio were used with
hardware that had open-drain semantics without needing the gpiolib
emulation for the same reason that's now triggering; such hardware
would report that the pin is in output mode, and the interrupt
allocation would fail.

While it's regrettable that fixing the inconsistency has caused a
regression, I think that has found another place where the semantics
aren't entirely sane, and as Linus says, _that_ needs fixing.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH v2 1/4] gpio: gpiolib: Allow GPIO IRQs to lazy disable

[Stephen Boyd]

Quoting Maulik Shah (2020-05-27 04:26:14)
> On 5/27/2020 3:14 PM, Stephen Boyd wrote:
> > Quoting Maulik Shah (2020-05-23 10:11:10)
> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> >> index eaa0e20..3810cd0 100644
> >> --- a/drivers/gpio/gpiolib.c
> >> +++ b/drivers/gpio/gpiolib.c
> >> @@ -2465,32 +2465,37 @@ static void gpiochip_irq_relres(struct irq_data *d)
> >>          gpiochip_relres_irq(gc, d->hwirq);
> >>   }
> >>   
> >> +static void gpiochip_irq_mask(struct irq_data *d)
> >> +{
> >> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> >> +
> >> +       if (gc->irq.irq_mask)
> >> +               gc->irq.irq_mask(d);
> >> +       gpiochip_disable_irq(gc, d->hwirq);
> > How does this work in the lazy case when I want to drive the GPIO? Say I
> > have a GPIO that is also an interrupt. The code would look like
> >
> >   struct gpio_desc *gpio = gpiod_get(...)
> >   unsigned int girq = gpiod_to_irq(gpio)
> >
> >   request_irq(girq, ...);
> >
> >   disable_irq(girq);
> >   gpiod_direction_output(gpio, 1);
> >
> > In the lazy case genirq wouldn't call the mask function until the first
> > interrupt arrived on the GPIO line. If that never happened then wouldn't
> > we be blocked in gpiod_direction_output() when the test_bit() sees
> > FLAG_USED_AS_IRQ? Or do we need irqs to be released before driving
> > gpios?
> 
> The client driver can decide to unlazy disable IRQ with below API...
> 
>   irq_set_status_flags(girq, IRQ_DISABLE_UNLAZY);
> 
> This will immediatly invoke mask function (unlazy disable) from genirq, 
> even though irq_disable is not implemented.
> 

Sure a consumer can disable the lazy feature, but that shouldn't be
required to make this work. The flag was introduced in commit
e9849777d0e2 ("genirq: Add flag to force mask in
disable_irq[_nosync]()") specifically to help devices that can't disable
the interrupt in their own device avoid a double interrupt.

Re: [PATCH v2 1/4] gpio: gpiolib: Allow GPIO IRQs to lazy disable

[Maulik Shah]

Hi,

On 5/28/2020 6:38 AM, Stephen Boyd wrote:
> Quoting Maulik Shah (2020-05-27 04:26:14)
>> On 5/27/2020 3:14 PM, Stephen Boyd wrote:
>>> Quoting Maulik Shah (2020-05-23 10:11:10)
>>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>>> index eaa0e20..3810cd0 100644
>>>> --- a/drivers/gpio/gpiolib.c
>>>> +++ b/drivers/gpio/gpiolib.c
>>>> @@ -2465,32 +2465,37 @@ static void gpiochip_irq_relres(struct irq_data *d)
>>>>           gpiochip_relres_irq(gc, d->hwirq);
>>>>    }
>>>>    
>>>> +static void gpiochip_irq_mask(struct irq_data *d)
>>>> +{
>>>> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>>>> +
>>>> +       if (gc->irq.irq_mask)
>>>> +               gc->irq.irq_mask(d);
>>>> +       gpiochip_disable_irq(gc, d->hwirq);
>>> How does this work in the lazy case when I want to drive the GPIO? Say I
>>> have a GPIO that is also an interrupt. The code would look like
>>>
>>>    struct gpio_desc *gpio = gpiod_get(...)
>>>    unsigned int girq = gpiod_to_irq(gpio)
>>>
>>>    request_irq(girq, ...);
>>>
>>>    disable_irq(girq);
>>>    gpiod_direction_output(gpio, 1);
>>>
>>> In the lazy case genirq wouldn't call the mask function until the first
>>> interrupt arrived on the GPIO line. If that never happened then wouldn't
>>> we be blocked in gpiod_direction_output() when the test_bit() sees
>>> FLAG_USED_AS_IRQ? Or do we need irqs to be released before driving
>>> gpios?
>> The client driver can decide to unlazy disable IRQ with below API...
>>
>>    irq_set_status_flags(girq, IRQ_DISABLE_UNLAZY);
>>
>> This will immediatly invoke mask function (unlazy disable) from genirq,
>> even though irq_disable is not implemented.
>>
> Sure a consumer can disable the lazy feature, but that shouldn't be
> required to make this work. The flag was introduced in commit
> e9849777d0e2 ("genirq: Add flag to force mask in
> disable_irq[_nosync]()") specifically to help devices that can't disable
> the interrupt in their own device avoid a double interrupt.
i don't think this will be a problem.

Case 1) Client driver have locked gpio to be used as IRQ using 
gpiochip_lock_as_irq()

In this case, When client driver want to change the direction for a 
gpio, they will invoke gpiod_direction_output().
I see it checks for two flags (pasted below), if GPIO is used as IRQ and 
whether its enabled IRQ or not.

        /* GPIOs used for enabled IRQs shall not be set as output */
         if (test_bit(FLAG_USED_AS_IRQ, &desc->flags) &&
             test_bit(FLAG_IRQ_IS_ENABLED, &desc->flags)) {

The first one (FLAG_USED_AS_IRQ) is set only if client driver in past 
have locked gpio to use as IRQ with a call to gpiochip_lock_as_irq()
then it never gets unlocked until clients invoke gpiochip_unlock_as_irq().

So i presume the client driver which in past locked gpio to be used as 
IRQ, now wants to change direction then it will
a. first unlock to use as IRQ
b. then change the direction.

Once it unlocks in step (a), both these flags will be cleared and there 
won't be any error in changing direction in step (b).

Case 2) Client driver did not lock gpio to be used as IRQ.

In this case, it will be straight forward to change the direction, as 
FLAG_USED_AS_IRQ is never set.

Thanks,
Maulik

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

Re: [PATCH v2 1/4] gpio: gpiolib: Allow GPIO IRQs to lazy disable

[Linus Walleij]

On Sat, May 23, 2020 at 7:11 PM Maulik Shah <mkshah@codeaurora.org> wrote:

> With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib
> overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable
> callback is implemented then genirq takes unlazy path to disable irq.
>
> Underlying irqchip may not want to implement irq_disable callback to lazy
> disable irq when client drivers invokes disable_irq(). By overriding
> irq_disable callback, gpiolib ends up always unlazy disabling IRQ.
>
> Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only
> if irqchip implemented irq_disable. In cases where irq_disable is not
> implemented irq_mask is overridden. Similarly override irq_enable callback
> only if irqchip implemented irq_enable otherwise irq_unmask is overridden.
>
> Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable)
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>

I applied this patch 1/4 to the GPIO tree since it is nice on its own
and it is soon merge window.

Yours,
Linus Walleij

Re: [PATCH v2 1/4] gpio: gpiolib: Allow GPIO IRQs to lazy disable

[Stephen Boyd]

Quoting Maulik Shah (2020-05-28 06:11:23)
> Hi,
> 
> On 5/28/2020 6:38 AM, Stephen Boyd wrote:
> > Quoting Maulik Shah (2020-05-27 04:26:14)
> >> On 5/27/2020 3:14 PM, Stephen Boyd wrote:
> >>> Quoting Maulik Shah (2020-05-23 10:11:10)
> >>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> >>>> index eaa0e20..3810cd0 100644
> >>>> --- a/drivers/gpio/gpiolib.c
> >>>> +++ b/drivers/gpio/gpiolib.c
> >>>> @@ -2465,32 +2465,37 @@ static void gpiochip_irq_relres(struct irq_data *d)
> >>>>           gpiochip_relres_irq(gc, d->hwirq);
> >>>>    }
> >>>>    
> >>>> +static void gpiochip_irq_mask(struct irq_data *d)
> >>>> +{
> >>>> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> >>>> +
> >>>> +       if (gc->irq.irq_mask)
> >>>> +               gc->irq.irq_mask(d);
> >>>> +       gpiochip_disable_irq(gc, d->hwirq);
> >>> How does this work in the lazy case when I want to drive the GPIO? Say I
> >>> have a GPIO that is also an interrupt. The code would look like
> >>>
> >>>    struct gpio_desc *gpio = gpiod_get(...)
> >>>    unsigned int girq = gpiod_to_irq(gpio)
> >>>
> >>>    request_irq(girq, ...);
> >>>
> >>>    disable_irq(girq);
> >>>    gpiod_direction_output(gpio, 1);
> >>>
> >>> In the lazy case genirq wouldn't call the mask function until the first
> >>> interrupt arrived on the GPIO line. If that never happened then wouldn't
> >>> we be blocked in gpiod_direction_output() when the test_bit() sees
> >>> FLAG_USED_AS_IRQ? Or do we need irqs to be released before driving
> >>> gpios?
> >> The client driver can decide to unlazy disable IRQ with below API...
> >>
> >>    irq_set_status_flags(girq, IRQ_DISABLE_UNLAZY);
> >>
> >> This will immediatly invoke mask function (unlazy disable) from genirq,
> >> even though irq_disable is not implemented.
> >>
> > Sure a consumer can disable the lazy feature, but that shouldn't be
> > required to make this work. The flag was introduced in commit
> > e9849777d0e2 ("genirq: Add flag to force mask in
> > disable_irq[_nosync]()") specifically to help devices that can't disable
> > the interrupt in their own device avoid a double interrupt.
> i don't think this will be a problem.
> 
> Case 1) Client driver have locked gpio to be used as IRQ using 
> gpiochip_lock_as_irq()
> 
> In this case, When client driver want to change the direction for a 
> gpio, they will invoke gpiod_direction_output().
> I see it checks for two flags (pasted below), if GPIO is used as IRQ and 
> whether its enabled IRQ or not.
> 
>         /* GPIOs used for enabled IRQs shall not be set as output */
>          if (test_bit(FLAG_USED_AS_IRQ, &desc->flags) &&
>              test_bit(FLAG_IRQ_IS_ENABLED, &desc->flags)) {
> 
> The first one (FLAG_USED_AS_IRQ) is set only if client driver in past 
> have locked gpio to use as IRQ with a call to gpiochip_lock_as_irq()
> then it never gets unlocked until clients invoke gpiochip_unlock_as_irq().
> 
> So i presume the client driver which in past locked gpio to be used as 
> IRQ, now wants to change direction then it will
> a. first unlock to use as IRQ
> b. then change the direction.

How does a client driver unlock to use as an IRQ though? I don't
understand how that is done. gpiochip_lock_as_irq() isn't a gpio
consumer API, it's a gpiochip/gpio provider API.

> 
> Once it unlocks in step (a), both these flags will be cleared and there 
> won't be any error in changing direction in step (b).

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

[Maulik Shah]

Hi,

On 5/27/2020 3:45 PM, Stephen Boyd wrote:
> Quoting Maulik Shah (2020-05-23 10:11:13)
>> 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 | 33 +++++++++++++++++----------------
>>   1 file changed, 17 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
>> index 6ae9e1f..f7c0662 100644
>> --- a/drivers/irqchip/qcom-pdc.c
>> +++ b/drivers/irqchip/qcom-pdc.c
>> @@ -36,6 +36,7 @@ struct pdc_pin_region {
>>          u32 cnt;
>>   };
>>   
>> +DECLARE_BITMAP(pdc_wake_irqs, PDC_MAX_IRQS);
> static?
Thanks i will declare as static in v3.
>
>>   static DEFINE_RAW_SPINLOCK(pdc_lock);
>>   static void __iomem *pdc_base;
>>   static struct pdc_pin_region *pdc_region;
>> @@ -87,22 +88,20 @@ 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);
>> -}
>> +               return 0;
> Shouldn't this fail if we can't set for wake?

we return success/failure from parent chip with below call at end of 
set_wake.

return irq_chip_set_wake_parent(d, on);

>
>>   
>> -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)
> The diff is really hard to read too. Maybe set_wake can be added first
> and then the enable/disable functions removed?
i think should be ok in same patch, if you insist i can split this 
change in to two.
>
>> @@ -110,6 +109,9 @@ static void qcom_pdc_gic_mask(struct irq_data *d)
>>          if (d->hwirq == GPIO_NO_WAKE_IRQ)
>>                  return;
>>   
>> +       if (!test_bit(d->hwirq, pdc_wake_irqs))
>> +               pdc_enable_intr(d, false);
>> +
>>          irq_chip_mask_parent(d);
>>   }
>>   
>> @@ -118,6 +120,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);
>>   }
>>   
> I find these two hunks deeply confusing. I'm not sure what the
> maintainers think though. I hope it would be simpler to always enable
> the hwirqs in the pdc when an irq is requested and only disable it in
> the pdc when the system goes to suspend and the pdc pin isn't for an irq
> that's marked for wakeup. Does that break somehow?
PDC monitors interrupts during CPUidle as well, in cases where deepest 
low power mode happened from cpuidle where GIC is not active.
If we keep PDC IRQ always enabled/unmasked during idle and then 
disable/mask when entering to suspend, it will break cpuidle.

>
> My understanding of the hardware is that the GPIO controller has lines
> directly connected to various SPI lines on the GIC and PDC has a way to
> monitor those direct connections and wakeup the CPUs when they trigger
> the detection logic in the PDC. The enable/disable bit in PDC gates that
> logic for each wire between the GPIO controller and the GIC.
>
> So isn't it simpler to leave the PDC monitoring pins that we care about
> all the time and only stop monitoring when we enter and leave suspend?

it can affect idle path as explained above.

> And shouldn't the driver set something sane in qcom_pdc_init() to
> disable all the pdc pins so that we don't rely on boot state to
> configure pins for wakeup?

We don't rely on boot state, by default all interrupt will be disabled.

This is same to GIC driver having GICD_ISENABLER register, where all 
bits (one bit per interrupt) set to 0 (masked irqs) during boot up.

Similarly PDC also have all bits set to 0 in PDC's IRQ_ENABLE_BANK.

Thanks,
Maulik
>
>> @@ -197,15 +200,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 v2 1/4] gpio: gpiolib: Allow GPIO IRQs to lazy disable

[Maulik Shah]

Hi,

On 5/29/2020 3:52 AM, Stephen Boyd wrote:
> Quoting Maulik Shah (2020-05-28 06:11:23)
>> Hi,
>>
>> On 5/28/2020 6:38 AM, Stephen Boyd wrote:
>>> Quoting Maulik Shah (2020-05-27 04:26:14)
>>>> On 5/27/2020 3:14 PM, Stephen Boyd wrote:
>>>>> Quoting Maulik Shah (2020-05-23 10:11:10)
>>>>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>>>>> index eaa0e20..3810cd0 100644
>>>>>> --- a/drivers/gpio/gpiolib.c
>>>>>> +++ b/drivers/gpio/gpiolib.c
>>>>>> @@ -2465,32 +2465,37 @@ static void gpiochip_irq_relres(struct irq_data *d)
>>>>>>            gpiochip_relres_irq(gc, d->hwirq);
>>>>>>     }
>>>>>>     
>>>>>> +static void gpiochip_irq_mask(struct irq_data *d)
>>>>>> +{
>>>>>> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>>>>>> +
>>>>>> +       if (gc->irq.irq_mask)
>>>>>> +               gc->irq.irq_mask(d);
>>>>>> +       gpiochip_disable_irq(gc, d->hwirq);
>>>>> How does this work in the lazy case when I want to drive the GPIO? Say I
>>>>> have a GPIO that is also an interrupt. The code would look like
>>>>>
>>>>>     struct gpio_desc *gpio = gpiod_get(...)
>>>>>     unsigned int girq = gpiod_to_irq(gpio)
>>>>>
>>>>>     request_irq(girq, ...);
>>>>>
>>>>>     disable_irq(girq);
>>>>>     gpiod_direction_output(gpio, 1);
>>>>>
>>>>> In the lazy case genirq wouldn't call the mask function until the first
>>>>> interrupt arrived on the GPIO line. If that never happened then wouldn't
>>>>> we be blocked in gpiod_direction_output() when the test_bit() sees
>>>>> FLAG_USED_AS_IRQ? Or do we need irqs to be released before driving
>>>>> gpios?
>>>> The client driver can decide to unlazy disable IRQ with below API...
>>>>
>>>>     irq_set_status_flags(girq, IRQ_DISABLE_UNLAZY);
>>>>
>>>> This will immediatly invoke mask function (unlazy disable) from genirq,
>>>> even though irq_disable is not implemented.
>>>>
>>> Sure a consumer can disable the lazy feature, but that shouldn't be
>>> required to make this work. The flag was introduced in commit
>>> e9849777d0e2 ("genirq: Add flag to force mask in
>>> disable_irq[_nosync]()") specifically to help devices that can't disable
>>> the interrupt in their own device avoid a double interrupt.
>> i don't think this will be a problem.
>>
>> Case 1) Client driver have locked gpio to be used as IRQ using
>> gpiochip_lock_as_irq()
>>
>> In this case, When client driver want to change the direction for a
>> gpio, they will invoke gpiod_direction_output().
>> I see it checks for two flags (pasted below), if GPIO is used as IRQ and
>> whether its enabled IRQ or not.
>>
>>          /* GPIOs used for enabled IRQs shall not be set as output */
>>           if (test_bit(FLAG_USED_AS_IRQ, &desc->flags) &&
>>               test_bit(FLAG_IRQ_IS_ENABLED, &desc->flags)) {
>>
>> The first one (FLAG_USED_AS_IRQ) is set only if client driver in past
>> have locked gpio to use as IRQ with a call to gpiochip_lock_as_irq()
>> then it never gets unlocked until clients invoke gpiochip_unlock_as_irq().
>>
>> So i presume the client driver which in past locked gpio to be used as
>> IRQ, now wants to change direction then it will
>> a. first unlock to use as IRQ
>> b. then change the direction.
> How does a client driver unlock to use as an IRQ though? I don't
> understand how that is done. gpiochip_lock_as_irq() isn't a gpio
> consumer API, it's a gpiochip/gpio provider API.

>>In the lazy case genirq wouldn't call the mask function until the first
>>interrupt arrived on the GPIO line. If that never happened then wouldn't
>>we be blocked in gpiod_direction_output() when the test_bit() sees
>>FLAG_USED_AS_IRQ?

What i was trying to explain in above two cases is..
FLAG_USED_AS_IRQ is set only when client driver locks GPIO to be used as IRQ
with gpiochip_lock_as_irq() API call.

Coming to query of test_bit() seeing this flag as set won't be a problem.
Lets take an example...

1. Client driver locks gpio to be used as IRQ gpiochip_lock_as_irq()
    This makes gpiolib set two flags..
    a. FLAG_USED_AS_IRQ
    b. FLAG_IRQ_IS_ENABLED
    
    Note that this is the only API which sets the flag (a) FLAG_USED_AS_IRQ.
    
2. During gpiochip_disable_irq() it only clears the flag (b) but the flag (a) is still set.

3. Now client driver does disable_irq()...
    With this patch, in case the irq_chip did not implement irq_disable callback then irq_mask will be overridden.
    so during first disable_irq() call, the gpiolib won't come to immediatly know that interrupt is disabled (lazy disable)
    hence both these flags are still set.

4. After disabling irq, client driver want to change the direction, so it wants to now call gpiod_direction_output()
    But before calling this, client driver knows that in step (1), client driver locked GPIO to be used only as IRQ.
    So GPIO cannot be used as any other purpose other than IRQ till the point its locked.
	
    hence before calling  gpiod_direction_output(), it has to first invoke gpiochip_unlock_as_irq().
    Calling this will clear both flags and allow GPIO to change the direction.
	
    Now client can invoke gpiod_direction_output() and the test_bit() check inside this won't complain.

Case 1) in my previous mail was where in above example client driver did step (1) which locks the gpio as IRQ then
its expected to do unlock in step (4) before changing direction.
so no issue in case 1) regarding test_bit complain.

Case 2) is where driver didn't even do step-1, so in step-4, they can directly invoke gpiod_direction_output()
client didn't lock, so the flag FLAG_USED_AS_IRQ is never set, so test_bit for this flag won't complain.
    
So in both cases it looks to be working as expected to me.
Hope that its answers your query.

Thanks,
Maulik

>> Once it unlocks in step (a), both these flags will be cleared and there
>> won't be any error in changing direction in step (b).

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

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

[Stephen Boyd]

Quoting Maulik Shah (2020-05-29 02:20:32)
> Hi,
> 
> On 5/27/2020 3:45 PM, Stephen Boyd wrote:
> > Quoting Maulik Shah (2020-05-23 10:11:13)
> >> @@ -87,22 +88,20 @@ 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);
> >> -}
> >> +               return 0;
> > Shouldn't this fail if we can't set for wake?
> 
> we return success/failure from parent chip with below call at end of 
> set_wake.
> 
> return irq_chip_set_wake_parent(d, on);

It's not a question about the parent irqchip. I'm wondering why we would
return success for a gpio irq that can't be marked for wakeup when a
client driver tries to enable wake on it. My understanding is that all
gpios irqs call here and PDC can't monitor all of them so some are
GPIO_NO_WAKE_IRQ and thus trying to mark those for wakeup should fail.
Of course msm_gpio_irq_set_wake() should also fail if it can't mark the
gpio for wakeup, but that's another problem.

> >> @@ -118,6 +120,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);
> >>   }
> >>   
> > I find these two hunks deeply confusing. I'm not sure what the
> > maintainers think though. I hope it would be simpler to always enable
> > the hwirqs in the pdc when an irq is requested and only disable it in
> > the pdc when the system goes to suspend and the pdc pin isn't for an irq
> > that's marked for wakeup. Does that break somehow?
> PDC monitors interrupts during CPUidle as well, in cases where deepest 
> low power mode happened from cpuidle where GIC is not active.
> If we keep PDC IRQ always enabled/unmasked during idle and then 
> disable/mask when entering to suspend, it will break cpuidle.

How does it break cpuidle? The irqs that would be enabled/unmasked in
pdc would only be the irqs that the kernel has setup irq handlers for
(from request_irq() and friends).  We want those irqs to keep working
during cpuidle and wake the CPU from the deepest idle states.

> 
> >
> > My understanding of the hardware is that the GPIO controller has lines
> > directly connected to various SPI lines on the GIC and PDC has a way to
> > monitor those direct connections and wakeup the CPUs when they trigger
> > the detection logic in the PDC. The enable/disable bit in PDC gates that
> > logic for each wire between the GPIO controller and the GIC.
> >
> > So isn't it simpler to leave the PDC monitoring pins that we care about
> > all the time and only stop monitoring when we enter and leave suspend?
> 
> it can affect idle path as explained above.
> 
> > And shouldn't the driver set something sane in qcom_pdc_init() to
> > disable all the pdc pins so that we don't rely on boot state to
> > configure pins for wakeup?
> 
> We don't rely on boot state, by default all interrupt will be disabled.

Does 'default' mean the hardware register reset state? I'm worried that
we will kexec and then various pdc pins will be enabled because the
previous kernel had them enabled but then the new kernel doesn't care
about those pins and we'll never be able to suspend or go idle. I don't
know what happens in the GIC case but I think gic_dist_config() and
things set a sane state at kernel boot.

> 
> This is same to GIC driver having GICD_ISENABLER register, where all 
> bits (one bit per interrupt) set to 0 (masked irqs) during boot up.
> 
> Similarly PDC also have all bits set to 0 in PDC's IRQ_ENABLE_BANK.
> 

What code sets the IRQ_ENABLE_BANK to all zero when this driver probes?

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

[Maulik Shah]

Hi,

On 5/31/2020 12:56 AM, Stephen Boyd wrote:
> Quoting Maulik Shah (2020-05-29 02:20:32)
>> Hi,
>>
>> On 5/27/2020 3:45 PM, Stephen Boyd wrote:
>>> Quoting Maulik Shah (2020-05-23 10:11:13)
>>>> @@ -87,22 +88,20 @@ 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);
>>>> -}
>>>> +               return 0;
>>> Shouldn't this fail if we can't set for wake?
>> we return success/failure from parent chip with below call at end of
>> set_wake.
>>
>> return irq_chip_set_wake_parent(d, on);
> It's not a question about the parent irqchip. I'm wondering why we would
> return success for a gpio irq that can't be marked for wakeup when a
> client driver tries to enable wake on it. My understanding is that all
> gpios irqs call here and PDC can't monitor all of them so some are
> GPIO_NO_WAKE_IRQ and thus trying to mark those for wakeup should fail.
> Of course msm_gpio_irq_set_wake() should also fail if it can't mark the
> gpio for wakeup, but that's another problem.
i can change this to return error code.

PDC's caller msmgpio chip, currently don't check for return value for 
irq_set_wake callback.
i will udpate that as well in next revision.
>
>>>> @@ -118,6 +120,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);
>>>>    }
>>>>    
>>> I find these two hunks deeply confusing. I'm not sure what the
>>> maintainers think though. I hope it would be simpler to always enable
>>> the hwirqs in the pdc when an irq is requested and only disable it in
>>> the pdc when the system goes to suspend and the pdc pin isn't for an irq
>>> that's marked for wakeup. Does that break somehow?
>> PDC monitors interrupts during CPUidle as well, in cases where deepest
>> low power mode happened from cpuidle where GIC is not active.
>> If we keep PDC IRQ always enabled/unmasked during idle and then
>> disable/mask when entering to suspend, it will break cpuidle.
> How does it break cpuidle? The irqs that would be enabled/unmasked in
> pdc would only be the irqs that the kernel has setup irq handlers for
> (from request_irq() and friends).  We want those irqs to keep working
> during cpuidle and wake the CPU from the deepest idle states.

>>I hope it would be simpler to always enable
>>the hwirqs in the pdc when an irq is requested and only disable it in
>>the pdc when the system goes to suspend and the pdc pin isn't for an irq
>>that's marked for wakeup

>>How does it break cpuidle?

Consider a scenario..
1. All PDC irqs enabled/unmasked in HW when request_irq() happened/alloc happens
2. Client driver disable's irq. (lazy disable is there, so in HW its still unmasked) but disabled in SW.
3. Device enters deep CPUidle low power modes where only PDC monitors IRQ.
4. This IRQ can still wakeup from CPUidle since it was monitored by PDC.
5. From handler, it comes to know that IRQ is disabled in SW, so it really invokes irq_mask callback now to disable in HW.
6. This mask callback doesn't operate on PDC (since in PDC, IRQs gets masked only during suspend, all other times its enabled)
7. step 3 to 6 repeats, if this IRQ keeps on coming and waking up from deep cpuidle states.

>
>>> My understanding of the hardware is that the GPIO controller has lines
>>> directly connected to various SPI lines on the GIC and PDC has a way to
>>> monitor those direct connections and wakeup the CPUs when they trigger
>>> the detection logic in the PDC. The enable/disable bit in PDC gates that
>>> logic for each wire between the GPIO controller and the GIC.
>>>
>>> So isn't it simpler to leave the PDC monitoring pins that we care about
>>> all the time and only stop monitoring when we enter and leave suspend?
>> it can affect idle path as explained above.
>>
>>> And shouldn't the driver set something sane in qcom_pdc_init() to
>>> disable all the pdc pins so that we don't rely on boot state to
>>> configure pins for wakeup?
>> We don't rely on boot state, by default all interrupt will be disabled.
> Does 'default' mean the hardware register reset state?
correct.
> I'm worried that
> we will kexec and then various pdc pins will be enabled because the
> previous kernel had them enabled but then the new kernel doesn't care
> about those pins and we'll never be able to suspend or go idle. I don't
> know what happens in the GIC case but I think gic_dist_config() and
> things set a sane state at kernel boot.

Right however when switching kernel, i suppose client drivers will do a 
free_irq(), then this will

clear the PDC interrupt in HW by invoking mask_irq() from within free_irq().

>
>> This is same to GIC driver having GICD_ISENABLER register, where all
>> bits (one bit per interrupt) set to 0 (masked irqs) during boot up.
>>
>> Similarly PDC also have all bits set to 0 in PDC's IRQ_ENABLE_BANK.
>>
> What code sets the IRQ_ENABLE_BANK to all zero when this driver probes?

Enable bank will be zero as part of HW reset status when booting up 
first time.

Thanks,
Maulik

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

Re: [PATCH v2 1/4] gpio: gpiolib: Allow GPIO IRQs to lazy disable

[Linus Walleij]

On Fri, May 29, 2020 at 12:27 AM Stephen Boyd <swboyd@chromium.org> wrote:
> Quoting Linus Walleij (2020-05-28 14:33:36)
> > On Sat, May 23, 2020 at 7:11 PM Maulik Shah <mkshah@codeaurora.org> wrote:
> >
> > > With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib
> > > overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable
> > > callback is implemented then genirq takes unlazy path to disable irq.
> > >
> > > Underlying irqchip may not want to implement irq_disable callback to lazy
> > > disable irq when client drivers invokes disable_irq(). By overriding
> > > irq_disable callback, gpiolib ends up always unlazy disabling IRQ.
> > >
> > > Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only
> > > if irqchip implemented irq_disable. In cases where irq_disable is not
> > > implemented irq_mask is overridden. Similarly override irq_enable callback
> > > only if irqchip implemented irq_enable otherwise irq_unmask is overridden.
> > >
> > > Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable)
> > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> >
> > I applied this patch 1/4 to the GPIO tree since it is nice on its own
> > and it is soon merge window.
> >
>
> Can you please clarify how this doesn't break things as discussed in a
> fork of this thread[1]?
>
> [1] https://lore.kernel.org/r/948defc1-5ea0-adbb-185b-5f5a81f2e28a@codeaurora.org

I don't really understand I'm afraid.

Hans tested this on the one system that uses the method to disable the IRQ
and turn the line into an output, play around with it, then switch it back to
input (actually open drain) and then use the IRQ again.

Is something broken in practice or in theory?

Yours,
Linus Walleij

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

[Stephen Boyd]

Quoting Maulik Shah (2020-06-01 04:38:25)
> On 5/31/2020 12:56 AM, Stephen Boyd wrote:
> > Quoting Maulik Shah (2020-05-29 02:20:32)
> >> On 5/27/2020 3:45 PM, Stephen Boyd wrote:
> >>> Quoting Maulik Shah (2020-05-23 10:11:13)
> >>>> @@ -118,6 +120,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);
> >>>>    }
> >>>>    
> >>> I find these two hunks deeply confusing. I'm not sure what the
> >>> maintainers think though. I hope it would be simpler to always enable
> >>> the hwirqs in the pdc when an irq is requested and only disable it in
> >>> the pdc when the system goes to suspend and the pdc pin isn't for an irq
> >>> that's marked for wakeup. Does that break somehow?
> >> PDC monitors interrupts during CPUidle as well, in cases where deepest
> >> low power mode happened from cpuidle where GIC is not active.
> >> If we keep PDC IRQ always enabled/unmasked during idle and then
> >> disable/mask when entering to suspend, it will break cpuidle.
> > How does it break cpuidle? The irqs that would be enabled/unmasked in
> > pdc would only be the irqs that the kernel has setup irq handlers for
> > (from request_irq() and friends).  We want those irqs to keep working
> > during cpuidle and wake the CPU from the deepest idle states.
> 
> >>I hope it would be simpler to always enable
> >>the hwirqs in the pdc when an irq is requested and only disable it in
> >>the pdc when the system goes to suspend and the pdc pin isn't for an irq
> >>that's marked for wakeup
> 
> >>How does it break cpuidle?
> 
> Consider a scenario..
> 1. All PDC irqs enabled/unmasked in HW when request_irq() happened/alloc happens
> 2. Client driver disable's irq. (lazy disable is there, so in HW its still unmasked) but disabled in SW.
> 3. Device enters deep CPUidle low power modes where only PDC monitors IRQ.
> 4. This IRQ can still wakeup from CPUidle since it was monitored by PDC.
> 5. From handler, it comes to know that IRQ is disabled in SW, so it really invokes irq_mask callback now to disable in HW.
> 6. This mask callback doesn't operate on PDC (since in PDC, IRQs gets masked only during suspend, all other times its enabled)
> 7. step 3 to 6 repeats, if this IRQ keeps on coming and waking up from deep cpuidle states.

Ok so in summary, irq is left unmasked in pdc during deep cpu idle and
it keeps waking up the CPU because it isn't masked at the PDC after the
first time it interrupts? Is this a power problem? Because from a
correctness standpoint we don't really care. It woke up the CPU because
it happened, and the GIC can decide to ignore it or not by masking it at
the GIC. I thought that the PDC wouldn't wake up the CPU if we masked
the irq at the GIC level. Is that not true?

> 
> >
> >>> My understanding of the hardware is that the GPIO controller has lines
> >>> directly connected to various SPI lines on the GIC and PDC has a way to
> >>> monitor those direct connections and wakeup the CPUs when they trigger
> >>> the detection logic in the PDC. The enable/disable bit in PDC gates that
> >>> logic for each wire between the GPIO controller and the GIC.
> >>>
> >>> So isn't it simpler to leave the PDC monitoring pins that we care about
> >>> all the time and only stop monitoring when we enter and leave suspend?
> >> it can affect idle path as explained above.
> >>
> >>> And shouldn't the driver set something sane in qcom_pdc_init() to
> >>> disable all the pdc pins so that we don't rely on boot state to
> >>> configure pins for wakeup?
> >> We don't rely on boot state, by default all interrupt will be disabled.
> > Does 'default' mean the hardware register reset state?
> correct.
> > I'm worried that
> > we will kexec and then various pdc pins will be enabled because the
> > previous kernel had them enabled but then the new kernel doesn't care
> > about those pins and we'll never be able to suspend or go idle. I don't
> > know what happens in the GIC case but I think gic_dist_config() and
> > things set a sane state at kernel boot.
> 
> Right however when switching kernel, i suppose client drivers will do a 
> free_irq(), then this will
> 
> clear the PDC interrupt in HW by invoking mask_irq() from within free_irq().

We can't rely on drivers to do that.

> 
> >
> >> This is same to GIC driver having GICD_ISENABLER register, where all
> >> bits (one bit per interrupt) set to 0 (masked irqs) during boot up.
> >>
> >> Similarly PDC also have all bits set to 0 in PDC's IRQ_ENABLE_BANK.
> >>
> > What code sets the IRQ_ENABLE_BANK to all zero when this driver probes?
> 
> Enable bank will be zero as part of HW reset status when booting up 
> first time.
> 

It's not a concern about the hardware reset state of these registers at
boot. I'm worried that the bootloaders or previous OS will configure pdc
pins to wake us up. It's better to just force it to something sane, i.e.
everything disabled in the PDC, at driver probe time so that nothing can
be wrong.

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

[Maulik Shah]

Hi,

On 6/16/2020 5:27 PM, Stephen Boyd wrote:
> Quoting Maulik Shah (2020-06-01 04:38:25)
>> On 5/31/2020 12:56 AM, Stephen Boyd wrote:
>>> Quoting Maulik Shah (2020-05-29 02:20:32)
>>>> On 5/27/2020 3:45 PM, Stephen Boyd wrote:
>>>>> Quoting Maulik Shah (2020-05-23 10:11:13)
>>>>>> @@ -118,6 +120,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);
>>>>>>     }
>>>>>>     
>>>>> I find these two hunks deeply confusing. I'm not sure what the
>>>>> maintainers think though. I hope it would be simpler to always enable
>>>>> the hwirqs in the pdc when an irq is requested and only disable it in
>>>>> the pdc when the system goes to suspend and the pdc pin isn't for an irq
>>>>> that's marked for wakeup. Does that break somehow?
>>>> PDC monitors interrupts during CPUidle as well, in cases where deepest
>>>> low power mode happened from cpuidle where GIC is not active.
>>>> If we keep PDC IRQ always enabled/unmasked during idle and then
>>>> disable/mask when entering to suspend, it will break cpuidle.
>>> How does it break cpuidle? The irqs that would be enabled/unmasked in
>>> pdc would only be the irqs that the kernel has setup irq handlers for
>>> (from request_irq() and friends).  We want those irqs to keep working
>>> during cpuidle and wake the CPU from the deepest idle states.
>>>> I hope it would be simpler to always enable
>>>> the hwirqs in the pdc when an irq is requested and only disable it in
>>>> the pdc when the system goes to suspend and the pdc pin isn't for an irq
>>>> that's marked for wakeup
>>>> How does it break cpuidle?
>> Consider a scenario..
>> 1. All PDC irqs enabled/unmasked in HW when request_irq() happened/alloc happens
>> 2. Client driver disable's irq. (lazy disable is there, so in HW its still unmasked) but disabled in SW.
>> 3. Device enters deep CPUidle low power modes where only PDC monitors IRQ.
>> 4. This IRQ can still wakeup from CPUidle since it was monitored by PDC.
>> 5. From handler, it comes to know that IRQ is disabled in SW, so it really invokes irq_mask callback now to disable in HW.
>> 6. This mask callback doesn't operate on PDC (since in PDC, IRQs gets masked only during suspend, all other times its enabled)
>> 7. step 3 to 6 repeats, if this IRQ keeps on coming and waking up from deep cpuidle states.
> Ok so in summary, irq is left unmasked in pdc during deep cpu idle and
> it keeps waking up the CPU because it isn't masked at the PDC after the
> first time it interrupts? Is this a power problem?
yes it can be a power problem.
>   Because from a
> correctness standpoint we don't really care. It woke up the CPU because
> it happened, and the GIC can decide to ignore it or not by masking it at
> the GIC. I thought that the PDC wouldn't wake up the CPU if we masked
> the irq at the GIC level. Is that not true?

once PDC detects IRQ, it directly doesn't wake up CPU. it replays IRQ to 
GIC.

since at GIC its masked, GIC doesn't forward to cpu to immediatly wake 
it up.

however after PDC detecting IRQ, it exits low power mode and 
watchdog/timer can wakeup upon expiry.


>
>>>>> My understanding of the hardware is that the GPIO controller has lines
>>>>> directly connected to various SPI lines on the GIC and PDC has a way to
>>>>> monitor those direct connections and wakeup the CPUs when they trigger
>>>>> the detection logic in the PDC. The enable/disable bit in PDC gates that
>>>>> logic for each wire between the GPIO controller and the GIC.
>>>>>
>>>>> So isn't it simpler to leave the PDC monitoring pins that we care about
>>>>> all the time and only stop monitoring when we enter and leave suspend?
>>>> it can affect idle path as explained above.
>>>>
>>>>> And shouldn't the driver set something sane in qcom_pdc_init() to
>>>>> disable all the pdc pins so that we don't rely on boot state to
>>>>> configure pins for wakeup?
>>>> We don't rely on boot state, by default all interrupt will be disabled.
>>> Does 'default' mean the hardware register reset state?
>> correct.
>>> I'm worried that
>>> we will kexec and then various pdc pins will be enabled because the
>>> previous kernel had them enabled but then the new kernel doesn't care
>>> about those pins and we'll never be able to suspend or go idle. I don't
>>> know what happens in the GIC case but I think gic_dist_config() and
>>> things set a sane state at kernel boot.
>> Right however when switching kernel, i suppose client drivers will do a
>> free_irq(), then this will
>>
>> clear the PDC interrupt in HW by invoking mask_irq() from within free_irq().
> We can't rely on drivers to do that.
>
>>>> This is same to GIC driver having GICD_ISENABLER register, where all
>>>> bits (one bit per interrupt) set to 0 (masked irqs) during boot up.
>>>>
>>>> Similarly PDC also have all bits set to 0 in PDC's IRQ_ENABLE_BANK.
>>>>
>>> What code sets the IRQ_ENABLE_BANK to all zero when this driver probes?
>> Enable bank will be zero as part of HW reset status when booting up
>> first time.
>>
> It's not a concern about the hardware reset state of these registers at
> boot. I'm worried that the bootloaders or previous OS will configure pdc
> pins to wake us up. It's better to just force it to something sane, i.e.
> everything disabled in the PDC, at driver probe time so that nothing can
> be wrong.

okay, i will include a patch to disable all IRQs in PDC hw during init 
time in next revision.

Thanks,
Maulik

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

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

[Stephen Boyd]

Quoting Maulik Shah (2020-06-18 03:03:03)
> On 6/16/2020 5:27 PM, Stephen Boyd wrote:
> > Quoting Maulik Shah (2020-06-01 04:38:25)
> >> On 5/31/2020 12:56 AM, Stephen Boyd wrote:
> >>> Quoting Maulik Shah (2020-05-29 02:20:32)
> >>>> On 5/27/2020 3:45 PM, Stephen Boyd wrote:
> >>>>> Quoting Maulik Shah (2020-05-23 10:11:13)
> >>>>>> @@ -118,6 +120,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);
> >>>>>>     }
> >>>>>>     
> >>>>> I find these two hunks deeply confusing. I'm not sure what the
> >>>>> maintainers think though. I hope it would be simpler to always enable
> >>>>> the hwirqs in the pdc when an irq is requested and only disable it in
> >>>>> the pdc when the system goes to suspend and the pdc pin isn't for an irq
> >>>>> that's marked for wakeup. Does that break somehow?
> >>>> PDC monitors interrupts during CPUidle as well, in cases where deepest
> >>>> low power mode happened from cpuidle where GIC is not active.
> >>>> If we keep PDC IRQ always enabled/unmasked during idle and then
> >>>> disable/mask when entering to suspend, it will break cpuidle.
> >>> How does it break cpuidle? The irqs that would be enabled/unmasked in
> >>> pdc would only be the irqs that the kernel has setup irq handlers for
> >>> (from request_irq() and friends).  We want those irqs to keep working
> >>> during cpuidle and wake the CPU from the deepest idle states.
> >>>> I hope it would be simpler to always enable
> >>>> the hwirqs in the pdc when an irq is requested and only disable it in
> >>>> the pdc when the system goes to suspend and the pdc pin isn't for an irq
> >>>> that's marked for wakeup
> >>>> How does it break cpuidle?
> >> Consider a scenario..
> >> 1. All PDC irqs enabled/unmasked in HW when request_irq() happened/alloc happens
> >> 2. Client driver disable's irq. (lazy disable is there, so in HW its still unmasked) but disabled in SW.
> >> 3. Device enters deep CPUidle low power modes where only PDC monitors IRQ.
> >> 4. This IRQ can still wakeup from CPUidle since it was monitored by PDC.
> >> 5. From handler, it comes to know that IRQ is disabled in SW, so it really invokes irq_mask callback now to disable in HW.
> >> 6. This mask callback doesn't operate on PDC (since in PDC, IRQs gets masked only during suspend, all other times its enabled)
> >> 7. step 3 to 6 repeats, if this IRQ keeps on coming and waking up from deep cpuidle states.
> > Ok so in summary, irq is left unmasked in pdc during deep cpu idle and
> > it keeps waking up the CPU because it isn't masked at the PDC after the
> > first time it interrupts? Is this a power problem?
> yes it can be a power problem.
> >   Because from a
> > correctness standpoint we don't really care. It woke up the CPU because
> > it happened, and the GIC can decide to ignore it or not by masking it at
> > the GIC. I thought that the PDC wouldn't wake up the CPU if we masked
> > the irq at the GIC level. Is that not true?
> 
> once PDC detects IRQ, it directly doesn't wake up CPU. it replays IRQ to 
> GIC.
> 
> since at GIC its masked, GIC doesn't forward to cpu to immediatly wake 
> it up.
> 
> however after PDC detecting IRQ, it exits low power mode and 
> watchdog/timer can wakeup upon expiry.

Ok. So the only problem is some screaming irq that really wants to be
handled but the driver that requested it has disabled it at runtime. The
IRQ keeps kicking the CPUs out of deep idle and then eventually the
timer tick happens and we've run the CPUs in a shallower idle state for
this time? Presumably we'd like to have these irqs be lazily masked at
the PDC so that they can become pending when they first arrive but not
block deep idle states if they're interrupting often while being
handled.

On the other hand, we want irq wake state to be the only factor in irqs
being unmasked at the PDC on the entry to suspend. Purely
masking/unmasking at the PDC when the irq is masked in software doesn't
work because suspend/resume will break for disabled but wake enabled
irqs. But doing that makes idle work easily because we can assume during
idle that leaving it unmasked until it fires and then masking it in the
PDC until it is handled gives us good deep idle states in the face of
screaming irqs.

What are the actual requirements? Here is my attempt to boil this
discussion down into a few bullet points:

 1. During system suspend, wake enabled irqs should be enabled in PDC
 and all other irqs should be disabled in PDC.

 2. During idle, enabled irqs must be enabled in PDC, unless they're
 pending in which case they should be masked in the PDC so as to not
 wake up the CPU from deep idle states

 3. During non-idle, non-suspend, enabled irqs must be enabled in PDC.

Or is #3 actually false and PDC has no bearing on this?

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

[Maulik Shah]

Hi,

On 6/19/2020 2:56 PM, Stephen Boyd wrote:
> Quoting Maulik Shah (2020-06-18 03:03:03)
>> On 6/16/2020 5:27 PM, Stephen Boyd wrote:
>>> Quoting Maulik Shah (2020-06-01 04:38:25)
>>>> On 5/31/2020 12:56 AM, Stephen Boyd wrote:
>>>>> Quoting Maulik Shah (2020-05-29 02:20:32)
>>>>>> On 5/27/2020 3:45 PM, Stephen Boyd wrote:
>>>>>>> Quoting Maulik Shah (2020-05-23 10:11:13)
>>>>>>>> @@ -118,6 +120,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);
>>>>>>>>      }
>>>>>>>>      
>>>>>>> I find these two hunks deeply confusing. I'm not sure what the
>>>>>>> maintainers think though. I hope it would be simpler to always enable
>>>>>>> the hwirqs in the pdc when an irq is requested and only disable it in
>>>>>>> the pdc when the system goes to suspend and the pdc pin isn't for an irq
>>>>>>> that's marked for wakeup. Does that break somehow?
>>>>>> PDC monitors interrupts during CPUidle as well, in cases where deepest
>>>>>> low power mode happened from cpuidle where GIC is not active.
>>>>>> If we keep PDC IRQ always enabled/unmasked during idle and then
>>>>>> disable/mask when entering to suspend, it will break cpuidle.
>>>>> How does it break cpuidle? The irqs that would be enabled/unmasked in
>>>>> pdc would only be the irqs that the kernel has setup irq handlers for
>>>>> (from request_irq() and friends).  We want those irqs to keep working
>>>>> during cpuidle and wake the CPU from the deepest idle states.
>>>>>> I hope it would be simpler to always enable
>>>>>> the hwirqs in the pdc when an irq is requested and only disable it in
>>>>>> the pdc when the system goes to suspend and the pdc pin isn't for an irq
>>>>>> that's marked for wakeup
>>>>>> How does it break cpuidle?
>>>> Consider a scenario..
>>>> 1. All PDC irqs enabled/unmasked in HW when request_irq() happened/alloc happens
>>>> 2. Client driver disable's irq. (lazy disable is there, so in HW its still unmasked) but disabled in SW.
>>>> 3. Device enters deep CPUidle low power modes where only PDC monitors IRQ.
>>>> 4. This IRQ can still wakeup from CPUidle since it was monitored by PDC.
>>>> 5. From handler, it comes to know that IRQ is disabled in SW, so it really invokes irq_mask callback now to disable in HW.
>>>> 6. This mask callback doesn't operate on PDC (since in PDC, IRQs gets masked only during suspend, all other times its enabled)
>>>> 7. step 3 to 6 repeats, if this IRQ keeps on coming and waking up from deep cpuidle states.
>>> Ok so in summary, irq is left unmasked in pdc during deep cpu idle and
>>> it keeps waking up the CPU because it isn't masked at the PDC after the
>>> first time it interrupts? Is this a power problem?
>> yes it can be a power problem.
>>>    Because from a
>>> correctness standpoint we don't really care. It woke up the CPU because
>>> it happened, and the GIC can decide to ignore it or not by masking it at
>>> the GIC. I thought that the PDC wouldn't wake up the CPU if we masked
>>> the irq at the GIC level. Is that not true?
>> once PDC detects IRQ, it directly doesn't wake up CPU. it replays IRQ to
>> GIC.
>>
>> since at GIC its masked, GIC doesn't forward to cpu to immediatly wake
>> it up.
>>
>> however after PDC detecting IRQ, it exits low power mode and
>> watchdog/timer can wakeup upon expiry.
> Ok. So the only problem is some screaming irq that really wants to be
> handled but the driver that requested it has disabled it at runtime. The
> IRQ keeps kicking the CPUs out of deep idle and then eventually the
> timer tick happens and we've run the CPUs in a shallower idle state for
> this time?
No it may still enter deeper state next time.
> Presumably we'd like to have these irqs be lazily masked at
> the PDC so that they can become pending when they first arrive but not
> block deep idle states if they're interrupting often while being
> handled.

We do lazily disable IRQ.  but didnot understand why lazily disable when 
they are being handled?

The edge type irqs gets masked immediatly if one irq is being handled 
and another comes in.

but that's not a problem.

>
> On the other hand, we want irq wake state to be the only factor in irqs
> being unmasked at the PDC on the entry to suspend. Purely
> masking/unmasking at the PDC when the irq is masked in software doesn't
> work because suspend/resume will break for disabled but wake enabled
> irqs. But doing that makes idle work easily because we can assume during
> idle that leaving it unmasked until it fires and then masking it in the
> PDC until it is handled gives us good deep idle states in the face of
> screaming irqs.
>
> What are the actual requirements? Here is my attempt to boil this
> discussion down into a few bullet points:
>
>   1. During system suspend, wake enabled irqs should be enabled in PDC
>   and all other irqs should be disabled in PDC.
yes, IRQs should be enabled in both PDC and GIC before platform (PSCI 
suspend) happens if they are marked for wakeup (enable_irq_wake())
>
>   2. During idle, enabled irqs must be enabled in PDC, unless they're
>   pending in which case they should be masked in the PDC so as to not
>   wake up the CPU from deep idle states

i didn't get this point.

During idle, if the driver choosen to keep IRQ enabled, it should be 
enabled in both PDC and GIC

if the driver choosen to keep IRQ disabled, with this series...

a. do a lay disable when driver's call disable_irq(), meaning set the SW 
state as disabled but leave in PDC and GIC HW as unmasked/enabled.

b. if the IRQ comes inbetween and its of edge type, the generic 
handle_edge_irq will really mask in HW.

>
>   3. During non-idle, non-suspend, enabled irqs must be enabled in PDC.
>
> Or is #3 actually false and PDC has no bearing on this?

Correct, During this time (non-idle, non-suspend) PDC will be in 
something called "by pass mode" where it plays role of type conversion.

(a level low to level high / edge falling to edge rising) since GIC 
doesn't detect level low/falling edge IRQs.

Thanks,
Maulik

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