* [PATCH v3 1/5] pinctrl: qcom: Remove irq_disable callback from msmgpio irqchip
2020-06-22 9:31 [PATCH v3 0/5] irqchip: qcom: pdc: Introduce irq_set_wake call Maulik Shah
@ 2020-06-22 9:31 ` Maulik Shah
2020-07-13 22:17 ` Doug Anderson
2020-06-22 9:31 ` [PATCH v3 2/5] pinctrl: qcom: Add msmgpio irqchip flags Maulik Shah
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Maulik Shah @ 2020-06-22 9:31 UTC (permalink / raw)
To: bjorn.andersson, maz, linus.walleij, swboyd, evgreen, mka
Cc: linux-kernel, linux-arm-msm, linux-gpio, agross, tglx, jason,
dianders, rnayak, ilina, lsrao, Maulik Shah
The gpio can be marked for wakeup and drivers can invoke disable_irq()
during suspend, in such cases unlazy approach will also disable at HW
and such gpios will not wakeup device from suspend to RAM.
Remove irq_disable callback to allow gpio interrupts to lazy disabled.
The gpio interrupts will get disabled during irq_mask callback.
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
drivers/pinctrl/qcom/pinctrl-msm.c | 13 -------------
1 file changed, 13 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 83b7d64..2419023 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -815,18 +815,6 @@ static void msm_gpio_irq_enable(struct irq_data *d)
msm_gpio_irq_clear_unmask(d, true);
}
-static void msm_gpio_irq_disable(struct irq_data *d)
-{
- struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
- struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
-
- if (d->parent_data)
- irq_chip_disable_parent(d);
-
- if (!test_bit(d->hwirq, pctrl->skip_wake_irqs))
- msm_gpio_irq_mask(d);
-}
-
static void msm_gpio_irq_unmask(struct irq_data *d)
{
msm_gpio_irq_clear_unmask(d, false);
@@ -1146,7 +1134,6 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
pctrl->irq_chip.name = "msmgpio";
pctrl->irq_chip.irq_enable = msm_gpio_irq_enable;
- pctrl->irq_chip.irq_disable = msm_gpio_irq_disable;
pctrl->irq_chip.irq_mask = msm_gpio_irq_mask;
pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask;
pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/5] pinctrl: qcom: Remove irq_disable callback from msmgpio irqchip
2020-06-22 9:31 ` [PATCH v3 1/5] pinctrl: qcom: Remove irq_disable callback from msmgpio irqchip Maulik Shah
@ 2020-07-13 22:17 ` Doug Anderson
[not found] ` <723acb53-364a-9045-8dbd-fa2a270798a6@codeaurora.org>
0 siblings, 1 reply; 17+ messages in thread
From: Doug Anderson @ 2020-07-13 22:17 UTC (permalink / raw)
To: Maulik Shah
Cc: Bjorn Andersson, Marc Zyngier, LinusW, Stephen Boyd, Evan Green,
Matthias Kaehlcke, LKML, linux-arm-msm, open list:GPIO SUBSYSTEM,
Andy Gross, Thomas Gleixner, Jason Cooper, Rajendra Nayak,
Lina Iyer, Srinivas Rao L
Hi,
On Mon, Jun 22, 2020 at 2:32 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> The gpio can be marked for wakeup and drivers can invoke disable_irq()
> during suspend, in such cases unlazy approach will also disable at HW
> and such gpios will not wakeup device from suspend to RAM.
>
> Remove irq_disable callback to allow gpio interrupts to lazy disabled.
> The gpio interrupts will get disabled during irq_mask callback.
>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
> drivers/pinctrl/qcom/pinctrl-msm.c | 13 -------------
> 1 file changed, 13 deletions(-)
While the code of this patch looks fairly correct to me (there's no
need to implement the irq_disable callback and we can just rely on the
masking), I'm slightly anxious about the description. It almost feels
like you're somehow relying on the laziness to "fix" your issue here.
...but the laziness is supposed to just be an optimization.
Specifically if an interrupt happens to fire at any point in time
after a driver has called disable_irq() then it acts just like a
non-lazy disable.
Said another way, I think this is a valid thing for a driver to do and
it should get woken up if the irq fires in suspend:
disable_irq();
msleep(1000);
enable_irq_wake()
Specifically if an IRQ comes in during that sleep then it will be just
like you had a non-lazy IRQ.
So while I'm for this patch, I'd suggest a simpler description
(assuming my understanding is correct):
There is no reason to implement irq_disable() and irq_mask(). Let's just
use irq_mask() and let the rest of the core handle it.
Also: it feels really weird to me that you're getting rid of the
irq_disable() but keeping irq_enable(). That seems like asking for
trouble, though I'd have to do more research to see if I could figure
out exactly what might go wrong. Could you remove your irq_enable()
too?
-Doug
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 2/5] pinctrl: qcom: Add msmgpio irqchip flags
2020-06-22 9:31 [PATCH v3 0/5] irqchip: qcom: pdc: Introduce irq_set_wake call Maulik Shah
2020-06-22 9:31 ` [PATCH v3 1/5] pinctrl: qcom: Remove irq_disable callback from msmgpio irqchip Maulik Shah
@ 2020-06-22 9:31 ` Maulik Shah
2020-07-13 22:17 ` Doug Anderson
2020-06-22 9:31 ` [PATCH v3 3/5] pinctrl: qcom: Use return value from irq_set_wake call Maulik Shah
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Maulik Shah @ 2020-06-22 9:31 UTC (permalink / raw)
To: bjorn.andersson, maz, linus.walleij, swboyd, evgreen, mka
Cc: linux-kernel, linux-arm-msm, linux-gpio, agross, tglx, jason,
dianders, rnayak, ilina, lsrao, Maulik Shah
Add irqchip specific flags for msmgpio irqchip to mask non wakeirqs
during suspend and mask before setting irq type.
Masking before changing type should make sure any spurious interrupt
is not detected during this operation.
Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
drivers/pinctrl/qcom/pinctrl-msm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 2419023..b909ffe 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1143,6 +1143,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
pctrl->irq_chip.irq_set_affinity = msm_gpio_irq_set_affinity;
pctrl->irq_chip.irq_set_vcpu_affinity = msm_gpio_irq_set_vcpu_affinity;
+ pctrl->irq_chip.flags = IRQCHIP_MASK_ON_SUSPEND
+ | IRQCHIP_SET_TYPE_MASKED;
np = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0);
if (np) {
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/5] pinctrl: qcom: Add msmgpio irqchip flags
2020-06-22 9:31 ` [PATCH v3 2/5] pinctrl: qcom: Add msmgpio irqchip flags Maulik Shah
@ 2020-07-13 22:17 ` Doug Anderson
2020-07-14 10:43 ` Maulik Shah
0 siblings, 1 reply; 17+ messages in thread
From: Doug Anderson @ 2020-07-13 22:17 UTC (permalink / raw)
To: Maulik Shah
Cc: Bjorn Andersson, Marc Zyngier, LinusW, Stephen Boyd, Evan Green,
Matthias Kaehlcke, LKML, linux-arm-msm, open list:GPIO SUBSYSTEM,
Andy Gross, Thomas Gleixner, Jason Cooper, Rajendra Nayak,
Lina Iyer, Srinivas Rao L
Hi,
On Mon, Jun 22, 2020 at 2:32 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> Add irqchip specific flags for msmgpio irqchip to mask non wakeirqs
> during suspend and mask before setting irq type.
>
> Masking before changing type should make sure any spurious interrupt
> is not detected during this operation.
>
> Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
> drivers/pinctrl/qcom/pinctrl-msm.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 2419023..b909ffe 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -1143,6 +1143,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
> pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
> pctrl->irq_chip.irq_set_affinity = msm_gpio_irq_set_affinity;
> pctrl->irq_chip.irq_set_vcpu_affinity = msm_gpio_irq_set_vcpu_affinity;
> + pctrl->irq_chip.flags = IRQCHIP_MASK_ON_SUSPEND
I haven't tested it, but with my suggestion in patch #4 to use
irq_suspend and irq_resume, I presume adding IRQCHIP_MASK_ON_SUSPEND
is no longer needed?
> + | IRQCHIP_SET_TYPE_MASKED;
IIUC adding "IRQCHIP_SET_TYPE_MASKED" is unrelated to the rest of this
series, right?
-Doug
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/5] pinctrl: qcom: Add msmgpio irqchip flags
2020-07-13 22:17 ` Doug Anderson
@ 2020-07-14 10:43 ` Maulik Shah
0 siblings, 0 replies; 17+ messages in thread
From: Maulik Shah @ 2020-07-14 10:43 UTC (permalink / raw)
To: Doug Anderson
Cc: Bjorn Andersson, Marc Zyngier, LinusW, Stephen Boyd, Evan Green,
Matthias Kaehlcke, LKML, linux-arm-msm, open list:GPIO SUBSYSTEM,
Andy Gross, Thomas Gleixner, Jason Cooper, Rajendra Nayak,
Lina Iyer, Srinivas Rao L
Hi,
On 7/14/2020 3:47 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Jun 22, 2020 at 2:32 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>> Add irqchip specific flags for msmgpio irqchip to mask non wakeirqs
>> during suspend and mask before setting irq type.
>>
>> Masking before changing type should make sure any spurious interrupt
>> is not detected during this operation.
>>
>> Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>> ---
>> drivers/pinctrl/qcom/pinctrl-msm.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index 2419023..b909ffe 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>> @@ -1143,6 +1143,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>> pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
>> pctrl->irq_chip.irq_set_affinity = msm_gpio_irq_set_affinity;
>> pctrl->irq_chip.irq_set_vcpu_affinity = msm_gpio_irq_set_vcpu_affinity;
>> + pctrl->irq_chip.flags = IRQCHIP_MASK_ON_SUSPEND
> I haven't tested it, but with my suggestion in patch #4 to use
> irq_suspend and irq_resume, I presume adding IRQCHIP_MASK_ON_SUSPEND
> is no longer needed?
it will still be needed, to let the non wakeup capable IRQ masked during
suspend.
>
>
>> + | IRQCHIP_SET_TYPE_MASKED;
> IIUC adding "IRQCHIP_SET_TYPE_MASKED" is unrelated to the rest of this
> series, right?
Right, but since we are adding missing flags, i added it together.
Thanks,
Maulik
>
> -Doug
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 3/5] pinctrl: qcom: Use return value from irq_set_wake call
2020-06-22 9:31 [PATCH v3 0/5] irqchip: qcom: pdc: Introduce irq_set_wake call Maulik Shah
2020-06-22 9:31 ` [PATCH v3 1/5] pinctrl: qcom: Remove irq_disable callback from msmgpio irqchip Maulik Shah
2020-06-22 9:31 ` [PATCH v3 2/5] pinctrl: qcom: Add msmgpio irqchip flags Maulik Shah
@ 2020-06-22 9:31 ` Maulik Shah
2020-07-13 22:17 ` Doug Anderson
2020-07-16 13:18 ` Linus Walleij
2020-06-22 9:31 ` [PATCH v3 4/5] irqchip: qcom-pdc: Introduce " Maulik Shah
2020-06-22 9:31 ` [PATCH v3 5/5] irqchip: qcom-pdc: Reset all pdc interrupts during init Maulik Shah
4 siblings, 2 replies; 17+ messages in thread
From: Maulik Shah @ 2020-06-22 9:31 UTC (permalink / raw)
To: bjorn.andersson, maz, linus.walleij, swboyd, evgreen, mka
Cc: linux-kernel, linux-arm-msm, linux-gpio, agross, tglx, jason,
dianders, rnayak, ilina, lsrao, Maulik Shah
msmgpio irqchip is not using return value of irq_set_wake call.
Start using it.
Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
drivers/pinctrl/qcom/pinctrl-msm.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index b909ffe..92fe7d6 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -978,12 +978,10 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
* when TLMM is powered on. To allow that, enable the GPIO
* summary line to be wakeup capable at GIC.
*/
- if (d->parent_data)
- irq_chip_set_wake_parent(d, on);
-
- irq_set_irq_wake(pctrl->irq, on);
+ if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
+ return irq_chip_set_wake_parent(d, on);
- return 0;
+ return irq_set_irq_wake(pctrl->irq, on);
}
static int msm_gpio_irq_reqres(struct irq_data *d)
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/5] pinctrl: qcom: Use return value from irq_set_wake call
2020-06-22 9:31 ` [PATCH v3 3/5] pinctrl: qcom: Use return value from irq_set_wake call Maulik Shah
@ 2020-07-13 22:17 ` Doug Anderson
2020-07-16 13:18 ` Linus Walleij
1 sibling, 0 replies; 17+ messages in thread
From: Doug Anderson @ 2020-07-13 22:17 UTC (permalink / raw)
To: Maulik Shah
Cc: Bjorn Andersson, Marc Zyngier, LinusW, Stephen Boyd, Evan Green,
Matthias Kaehlcke, LKML, linux-arm-msm, open list:GPIO SUBSYSTEM,
Andy Gross, Thomas Gleixner, Jason Cooper, Rajendra Nayak,
Lina Iyer, Srinivas Rao L
Hi,
On Mon, Jun 22, 2020 at 2:32 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> msmgpio irqchip is not using return value of irq_set_wake call.
> Start using it.
>
> Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
> drivers/pinctrl/qcom/pinctrl-msm.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
Seems right to me.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/5] pinctrl: qcom: Use return value from irq_set_wake call
2020-06-22 9:31 ` [PATCH v3 3/5] pinctrl: qcom: Use return value from irq_set_wake call Maulik Shah
2020-07-13 22:17 ` Doug Anderson
@ 2020-07-16 13:18 ` Linus Walleij
2020-07-16 21:51 ` Doug Anderson
1 sibling, 1 reply; 17+ messages in thread
From: Linus Walleij @ 2020-07-16 13:18 UTC (permalink / raw)
To: Maulik Shah
Cc: Bjorn Andersson, Marc Zyngier, Stephen Boyd, Evan Green,
Matthias Kaehlcke, linux-kernel, MSM, open list:GPIO SUBSYSTEM,
Andy Gross, Thomas Gleixner, Jason Cooper, Doug Anderson,
Rajendra Nayak, Lina Iyer,
open list:GPIO SUBSYSTEM <linux-gpio@vger.kernel.org>,
Andy Gross <agross@kernel.org>,
Thomas Gleixner <tglx@linutronix.de>,
Jason Cooper <jason@lakedaemon.net>,
Doug Anderson <dianders@chromium.org>,
Rajendra Nayak <rnayak@codeaurora.org>,
Lina Iyer <ilina@codeaurora.org>,
On Mon, Jun 22, 2020 at 11:32 AM Maulik Shah <mkshah@codeaurora.org> wrote:
> msmgpio irqchip is not using return value of irq_set_wake call.
> Start using it.
>
> Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
Is this something that's causing regressions so I should apply it for
fixes, or is it fine to keep this with the rest of the series for v5.9?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/5] pinctrl: qcom: Use return value from irq_set_wake call
2020-07-16 13:18 ` Linus Walleij
@ 2020-07-16 21:51 ` Doug Anderson
0 siblings, 0 replies; 17+ messages in thread
From: Doug Anderson @ 2020-07-16 21:51 UTC (permalink / raw)
To: Linus Walleij
Cc: Maulik Shah, Bjorn Andersson, Marc Zyngier, Stephen Boyd,
Evan Green, Matthias Kaehlcke, linux-kernel, MSM,
open list:GPIO SUBSYSTEM, Andy Gross, Thomas Gleixner,
Jason Cooper, Rajendra Nayak, Lina Iyer,
open list:GPIO SUBSYSTEM <linux-gpio@vger.kernel.org>,
Andy Gross <agross@kernel.org>,
Thomas Gleixner <tglx@linutronix.de>,
Jason Cooper <jason@lakedaemon.net>,
Doug Anderson <dianders@chromium.org>,
Rajendra Nayak <rnayak@codeaurora.org>,
Lina Iyer <ilina@codeaurora.org>,
Hi,
On Thu, Jul 16, 2020 at 6:19 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Mon, Jun 22, 2020 at 11:32 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> > msmgpio irqchip is not using return value of irq_set_wake call.
> > Start using it.
> >
> > Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
> > Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>
> Is this something that's causing regressions so I should apply it for
> fixes, or is it fine to keep this with the rest of the series for v5.9?
I would let Maulik comment more, but as far as I can tell the function
has been ignoring the return value of irq_set_irq_wake() for much
longer. Presumably one could logically say:
Fixes: 6aced33f4974 ("pinctrl: msm: drop wake_irqs bitmap")
...though when you get past the commit that Maulik tagged you need a
backport rather than a straight cherry-pick.
That would make me believe that there is no real hurry to land the fix here.
-Doug
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 4/5] irqchip: qcom-pdc: Introduce irq_set_wake call
2020-06-22 9:31 [PATCH v3 0/5] irqchip: qcom: pdc: Introduce irq_set_wake call Maulik Shah
` (2 preceding siblings ...)
2020-06-22 9:31 ` [PATCH v3 3/5] pinctrl: qcom: Use return value from irq_set_wake call Maulik Shah
@ 2020-06-22 9:31 ` Maulik Shah
2020-07-13 22:16 ` Doug Anderson
2020-06-22 9:31 ` [PATCH v3 5/5] irqchip: qcom-pdc: Reset all pdc interrupts during init Maulik Shah
4 siblings, 1 reply; 17+ messages in thread
From: Maulik Shah @ 2020-06-22 9:31 UTC (permalink / raw)
To: bjorn.andersson, maz, linus.walleij, swboyd, evgreen, mka
Cc: linux-kernel, linux-arm-msm, linux-gpio, agross, tglx, jason,
dianders, rnayak, ilina, lsrao, Maulik Shah
Remove irq_disable callback to allow lazy disable for pdc interrupts.
Add irq_set_wake callback that unmask interrupt in HW when drivers
mark interrupt for wakeup. Interrupt will be cleared in HW during
lazy disable if its not marked for wakeup.
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
drivers/irqchip/qcom-pdc.c | 34 ++++++++++++++++------------------
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 6ae9e1f..8beb6f7 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -36,6 +36,7 @@ struct pdc_pin_region {
u32 cnt;
};
+static DECLARE_BITMAP(pdc_wake_irqs, PDC_MAX_IRQS);
static DEFINE_RAW_SPINLOCK(pdc_lock);
static void __iomem *pdc_base;
static struct pdc_pin_region *pdc_region;
@@ -87,22 +88,17 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
raw_spin_unlock(&pdc_lock);
}
-static void qcom_pdc_gic_disable(struct irq_data *d)
+static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on)
{
- if (d->hwirq == GPIO_NO_WAKE_IRQ)
- return;
-
- pdc_enable_intr(d, false);
- irq_chip_disable_parent(d);
-}
-
-static void qcom_pdc_gic_enable(struct irq_data *d)
-{
- if (d->hwirq == GPIO_NO_WAKE_IRQ)
- return;
+ if (on) {
+ pdc_enable_intr(d, true);
+ irq_chip_enable_parent(d);
+ set_bit(d->hwirq, pdc_wake_irqs);
+ } else {
+ clear_bit(d->hwirq, pdc_wake_irqs);
+ }
- pdc_enable_intr(d, true);
- irq_chip_enable_parent(d);
+ return irq_chip_set_wake_parent(d, on);
}
static void qcom_pdc_gic_mask(struct irq_data *d)
@@ -111,6 +107,9 @@ static void qcom_pdc_gic_mask(struct irq_data *d)
return;
irq_chip_mask_parent(d);
+
+ if (!test_bit(d->hwirq, pdc_wake_irqs))
+ pdc_enable_intr(d, false);
}
static void qcom_pdc_gic_unmask(struct irq_data *d)
@@ -118,6 +117,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d)
if (d->hwirq == GPIO_NO_WAKE_IRQ)
return;
+ pdc_enable_intr(d, true);
irq_chip_unmask_parent(d);
}
@@ -197,15 +197,13 @@ static struct irq_chip qcom_pdc_gic_chip = {
.irq_eoi = irq_chip_eoi_parent,
.irq_mask = qcom_pdc_gic_mask,
.irq_unmask = qcom_pdc_gic_unmask,
- .irq_disable = qcom_pdc_gic_disable,
- .irq_enable = qcom_pdc_gic_enable,
.irq_get_irqchip_state = qcom_pdc_gic_get_irqchip_state,
.irq_set_irqchip_state = qcom_pdc_gic_set_irqchip_state,
.irq_retrigger = irq_chip_retrigger_hierarchy,
.irq_set_type = qcom_pdc_gic_set_type,
+ .irq_set_wake = qcom_pdc_gic_set_wake,
.flags = IRQCHIP_MASK_ON_SUSPEND |
- IRQCHIP_SET_TYPE_MASKED |
- IRQCHIP_SKIP_SET_WAKE,
+ IRQCHIP_SET_TYPE_MASKED,
.irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent,
.irq_set_affinity = irq_chip_set_affinity_parent,
};
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 4/5] irqchip: qcom-pdc: Introduce irq_set_wake call
2020-06-22 9:31 ` [PATCH v3 4/5] irqchip: qcom-pdc: Introduce " Maulik Shah
@ 2020-07-13 22:16 ` Doug Anderson
2020-07-14 10:54 ` Maulik Shah
0 siblings, 1 reply; 17+ messages in thread
From: Doug Anderson @ 2020-07-13 22:16 UTC (permalink / raw)
To: Maulik Shah
Cc: Bjorn Andersson, Marc Zyngier, LinusW, Stephen Boyd, Evan Green,
Matthias Kaehlcke, LKML, linux-arm-msm, open list:GPIO SUBSYSTEM,
Andy Gross, Thomas Gleixner, Jason Cooper, Rajendra Nayak,
Lina Iyer, Srinivas Rao L
Hi,
On Mon, Jun 22, 2020 at 2:33 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> Remove irq_disable callback to allow lazy disable for pdc interrupts.
>
> Add irq_set_wake callback that unmask interrupt in HW when drivers
> mark interrupt for wakeup. Interrupt will be cleared in HW during
> lazy disable if its not marked for wakeup.
>
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
> drivers/irqchip/qcom-pdc.c | 34 ++++++++++++++++------------------
> 1 file changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index 6ae9e1f..8beb6f7 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -36,6 +36,7 @@ struct pdc_pin_region {
> u32 cnt;
> };
>
> +static DECLARE_BITMAP(pdc_wake_irqs, PDC_MAX_IRQS);
> static DEFINE_RAW_SPINLOCK(pdc_lock);
> static void __iomem *pdc_base;
> static struct pdc_pin_region *pdc_region;
> @@ -87,22 +88,17 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
> raw_spin_unlock(&pdc_lock);
> }
>
> -static void qcom_pdc_gic_disable(struct irq_data *d)
> +static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on)
> {
> - if (d->hwirq == GPIO_NO_WAKE_IRQ)
> - return;
> -
> - pdc_enable_intr(d, false);
> - irq_chip_disable_parent(d);
> -}
> -
> -static void qcom_pdc_gic_enable(struct irq_data *d)
> -{
> - if (d->hwirq == GPIO_NO_WAKE_IRQ)
> - return;
> + if (on) {
> + pdc_enable_intr(d, true);
> + irq_chip_enable_parent(d);
> + set_bit(d->hwirq, pdc_wake_irqs);
> + } else {
> + clear_bit(d->hwirq, pdc_wake_irqs);
> + }
>
> - pdc_enable_intr(d, true);
> - irq_chip_enable_parent(d);
> + return irq_chip_set_wake_parent(d, on);
> }
>
> static void qcom_pdc_gic_mask(struct irq_data *d)
> @@ -111,6 +107,9 @@ static void qcom_pdc_gic_mask(struct irq_data *d)
> return;
>
> irq_chip_mask_parent(d);
> +
> + if (!test_bit(d->hwirq, pdc_wake_irqs))
> + pdc_enable_intr(d, false);
I _think_ this will break masking, right? In other words, consider
the following (having nothing to do with suspend/resume):
1. Driver requests an interrupt.
2. Driver masks interrupt (calls disable_irq())
3. Interrupt fires while it is masked.
4. Driver unmasks interrupt (calls enable_irq().
After step #4 the interrupt should fire since it was only masked, not
disabled (yes, it's super confusing that the driver calls
disable_irq() but it expecting it to be masked--as I understand it
that's just how it is). I haven't tested, but I suspect that's broken
for you now (assuming you're working on a pin that wasn't a wakeup
pin) because you won't track edges when you're "disabled".
I suspect that the right thing to do here is to:
a) Make qcom_pdc_gic_set_wake() just keep "pdc_wake_irqs" up to date
and then call parent.
b) Implement irq_suspend and irq_resume. In irq_suspend() you disable
all interrupts that aren't in "pdc_wake_irqs". In irq_resume() you
just re-enable all of them (masking will be handled by the parent).
Would that work?
...oh, drat! The .irq_suspend() callback is only there if you're
using "irq/generic-chip.c". OK, well unless we want to move over to
using generic-chip we can just register for syscore ourselves. OK, I
tested and <https://crrev.com/c/2296160> works.
> }
>
> static void qcom_pdc_gic_unmask(struct irq_data *d)
> @@ -118,6 +117,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d)
> if (d->hwirq == GPIO_NO_WAKE_IRQ)
> return;
>
> + pdc_enable_intr(d, true);
> irq_chip_unmask_parent(d);
> }
>
> @@ -197,15 +197,13 @@ static struct irq_chip qcom_pdc_gic_chip = {
> .irq_eoi = irq_chip_eoi_parent,
> .irq_mask = qcom_pdc_gic_mask,
> .irq_unmask = qcom_pdc_gic_unmask,
> - .irq_disable = qcom_pdc_gic_disable,
> - .irq_enable = qcom_pdc_gic_enable,
> .irq_get_irqchip_state = qcom_pdc_gic_get_irqchip_state,
> .irq_set_irqchip_state = qcom_pdc_gic_set_irqchip_state,
> .irq_retrigger = irq_chip_retrigger_hierarchy,
> .irq_set_type = qcom_pdc_gic_set_type,
> + .irq_set_wake = qcom_pdc_gic_set_wake,
> .flags = IRQCHIP_MASK_ON_SUSPEND |
> - IRQCHIP_SET_TYPE_MASKED |
> - IRQCHIP_SKIP_SET_WAKE,
> + IRQCHIP_SET_TYPE_MASKED,
> .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent,
> .irq_set_affinity = irq_chip_set_affinity_parent,
> };
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 4/5] irqchip: qcom-pdc: Introduce irq_set_wake call
2020-07-13 22:16 ` Doug Anderson
@ 2020-07-14 10:54 ` Maulik Shah
0 siblings, 0 replies; 17+ messages in thread
From: Maulik Shah @ 2020-07-14 10:54 UTC (permalink / raw)
To: Doug Anderson
Cc: Bjorn Andersson, Marc Zyngier, LinusW, Stephen Boyd, Evan Green,
Matthias Kaehlcke, LKML, linux-arm-msm, open list:GPIO SUBSYSTEM,
Andy Gross, Thomas Gleixner, Jason Cooper, Rajendra Nayak,
Lina Iyer, Srinivas Rao L
Hi,
On 7/14/2020 3:46 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Jun 22, 2020 at 2:33 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>> Remove irq_disable callback to allow lazy disable for pdc interrupts.
>>
>> Add irq_set_wake callback that unmask interrupt in HW when drivers
>> mark interrupt for wakeup. Interrupt will be cleared in HW during
>> lazy disable if its not marked for wakeup.
>>
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>> ---
>> drivers/irqchip/qcom-pdc.c | 34 ++++++++++++++++------------------
>> 1 file changed, 16 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
>> index 6ae9e1f..8beb6f7 100644
>> --- a/drivers/irqchip/qcom-pdc.c
>> +++ b/drivers/irqchip/qcom-pdc.c
>> @@ -36,6 +36,7 @@ struct pdc_pin_region {
>> u32 cnt;
>> };
>>
>> +static DECLARE_BITMAP(pdc_wake_irqs, PDC_MAX_IRQS);
>> static DEFINE_RAW_SPINLOCK(pdc_lock);
>> static void __iomem *pdc_base;
>> static struct pdc_pin_region *pdc_region;
>> @@ -87,22 +88,17 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
>> raw_spin_unlock(&pdc_lock);
>> }
>>
>> -static void qcom_pdc_gic_disable(struct irq_data *d)
>> +static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on)
>> {
>> - if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> - return;
>> -
>> - pdc_enable_intr(d, false);
>> - irq_chip_disable_parent(d);
>> -}
>> -
>> -static void qcom_pdc_gic_enable(struct irq_data *d)
>> -{
>> - if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> - return;
>> + if (on) {
>> + pdc_enable_intr(d, true);
>> + irq_chip_enable_parent(d);
>> + set_bit(d->hwirq, pdc_wake_irqs);
>> + } else {
>> + clear_bit(d->hwirq, pdc_wake_irqs);
>> + }
>>
>> - pdc_enable_intr(d, true);
>> - irq_chip_enable_parent(d);
>> + return irq_chip_set_wake_parent(d, on);
>> }
>>
>> static void qcom_pdc_gic_mask(struct irq_data *d)
>> @@ -111,6 +107,9 @@ static void qcom_pdc_gic_mask(struct irq_data *d)
>> return;
>>
>> irq_chip_mask_parent(d);
>> +
>> + if (!test_bit(d->hwirq, pdc_wake_irqs))
>> + pdc_enable_intr(d, false);
> I _think_ this will break masking, right? In other words, consider
> the following (having nothing to do with suspend/resume):
>
> 1. Driver requests an interrupt.
> 2. Driver masks interrupt (calls disable_irq())
> 3. Interrupt fires while it is masked.
> 4. Driver unmasks interrupt (calls enable_irq().
>
> After step #4 the interrupt should fire since it was only masked, not
> disabled (yes, it's super confusing that the driver calls
> disable_irq() but it expecting it to be masked--as I understand it
> that's just how it is). I haven't tested, but I suspect that's broken
> for you now (assuming you're working on a pin that wasn't a wakeup
> pin) because you won't track edges when you're "disabled".
No its not broken, it works as expected. after step #4, interrupt will fire.
>
> I suspect that the right thing to do here is to:
>
> a) Make qcom_pdc_gic_set_wake() just keep "pdc_wake_irqs" up to date
> and then call parent.
>
> b) Implement irq_suspend and irq_resume. In irq_suspend() you disable
> all interrupts that aren't in "pdc_wake_irqs". In irq_resume() you
> just re-enable all of them (masking will be handled by the parent).
>
> Would that work?
>
> ...oh, drat! The .irq_suspend() callback is only there if you're
> using "irq/generic-chip.c". OK, well unless we want to move over to
> using generic-chip we can just register for syscore ourselves. OK, I
> tested and <https://crrev.com/c/2296160> works.
I too thought of using syscore ops earlier, but syscore ops won't work
if device chooses to enter "s2idle" suspend state since they are not
invoked in s2idle entry path.
even if you register for "irq/generic-chip.c" , this driver too
registers for syscore ops only, it won't work if you enter s2idle
suspend state.
Current patch works fine with both s2idle and deep suspend states.
Thanks,
Maulik
>
>
>
>> }
>>
>> static void qcom_pdc_gic_unmask(struct irq_data *d)
>> @@ -118,6 +117,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d)
>> if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> return;
>>
>> + pdc_enable_intr(d, true);
>> irq_chip_unmask_parent(d);
>> }
>>
>> @@ -197,15 +197,13 @@ static struct irq_chip qcom_pdc_gic_chip = {
>> .irq_eoi = irq_chip_eoi_parent,
>> .irq_mask = qcom_pdc_gic_mask,
>> .irq_unmask = qcom_pdc_gic_unmask,
>> - .irq_disable = qcom_pdc_gic_disable,
>> - .irq_enable = qcom_pdc_gic_enable,
>> .irq_get_irqchip_state = qcom_pdc_gic_get_irqchip_state,
>> .irq_set_irqchip_state = qcom_pdc_gic_set_irqchip_state,
>> .irq_retrigger = irq_chip_retrigger_hierarchy,
>> .irq_set_type = qcom_pdc_gic_set_type,
>> + .irq_set_wake = qcom_pdc_gic_set_wake,
>> .flags = IRQCHIP_MASK_ON_SUSPEND |
>> - IRQCHIP_SET_TYPE_MASKED |
>> - IRQCHIP_SKIP_SET_WAKE,
>> + IRQCHIP_SET_TYPE_MASKED,
>> .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent,
>> .irq_set_affinity = irq_chip_set_affinity_parent,
>> };
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 5/5] irqchip: qcom-pdc: Reset all pdc interrupts during init
2020-06-22 9:31 [PATCH v3 0/5] irqchip: qcom: pdc: Introduce irq_set_wake call Maulik Shah
` (3 preceding siblings ...)
2020-06-22 9:31 ` [PATCH v3 4/5] irqchip: qcom-pdc: Introduce " Maulik Shah
@ 2020-06-22 9:31 ` Maulik Shah
2020-07-13 22:17 ` Doug Anderson
4 siblings, 1 reply; 17+ messages in thread
From: Maulik Shah @ 2020-06-22 9:31 UTC (permalink / raw)
To: bjorn.andersson, maz, linus.walleij, swboyd, evgreen, mka
Cc: linux-kernel, linux-arm-msm, linux-gpio, agross, tglx, jason,
dianders, rnayak, ilina, lsrao, Maulik Shah
Clear previous kernel's configuration during init by resetting
all interrupts in enable bank to zero.
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
drivers/irqchip/qcom-pdc.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 8beb6f7..11a9d3a 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -19,6 +19,7 @@
#include <linux/slab.h>
#include <linux/types.h>
+#define PDC_MAX_IRQS_PER_REG 32
#define PDC_MAX_IRQS 168
#define PDC_MAX_GPIO_IRQS 256
@@ -339,6 +340,7 @@ static const struct irq_domain_ops qcom_pdc_gpio_ops = {
static int pdc_setup_pin_mapping(struct device_node *np)
{
int ret, n;
+ u32 reg, max_regs, max_pins = 0;
n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
if (n <= 0 || n % 3)
@@ -367,8 +369,19 @@ static int pdc_setup_pin_mapping(struct device_node *np)
&pdc_region[n].cnt);
if (ret)
return ret;
+ max_pins += pdc_region[n].cnt;
}
+ if (max_pins > PDC_MAX_IRQS)
+ return -EINVAL;
+
+ max_regs = max_pins / PDC_MAX_IRQS_PER_REG;
+ if (max_pins % PDC_MAX_IRQS_PER_REG)
+ max_regs++;
+
+ for (reg = 0; reg < max_regs; reg++)
+ pdc_reg_write(IRQ_ENABLE_BANK, reg, 0);
+
return 0;
}
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 5/5] irqchip: qcom-pdc: Reset all pdc interrupts during init
2020-06-22 9:31 ` [PATCH v3 5/5] irqchip: qcom-pdc: Reset all pdc interrupts during init Maulik Shah
@ 2020-07-13 22:17 ` Doug Anderson
2020-07-14 11:01 ` Maulik Shah
0 siblings, 1 reply; 17+ messages in thread
From: Doug Anderson @ 2020-07-13 22:17 UTC (permalink / raw)
To: Maulik Shah
Cc: Bjorn Andersson, Marc Zyngier, LinusW, Stephen Boyd, Evan Green,
Matthias Kaehlcke, LKML, linux-arm-msm, open list:GPIO SUBSYSTEM,
Andy Gross, Thomas Gleixner, Jason Cooper, Rajendra Nayak,
Lina Iyer, Srinivas Rao L
Hi,
On Mon, Jun 22, 2020 at 2:33 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> Clear previous kernel's configuration during init by resetting
> all interrupts in enable bank to zero.
>
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
> drivers/irqchip/qcom-pdc.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index 8beb6f7..11a9d3a 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -19,6 +19,7 @@
> #include <linux/slab.h>
> #include <linux/types.h>
>
> +#define PDC_MAX_IRQS_PER_REG 32
> #define PDC_MAX_IRQS 168
> #define PDC_MAX_GPIO_IRQS 256
>
> @@ -339,6 +340,7 @@ static const struct irq_domain_ops qcom_pdc_gpio_ops = {
> static int pdc_setup_pin_mapping(struct device_node *np)
> {
> int ret, n;
> + u32 reg, max_regs, max_pins = 0;
>
> n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
> if (n <= 0 || n % 3)
> @@ -367,8 +369,19 @@ static int pdc_setup_pin_mapping(struct device_node *np)
> &pdc_region[n].cnt);
> if (ret)
> return ret;
> + max_pins += pdc_region[n].cnt;
> }
>
> + if (max_pins > PDC_MAX_IRQS)
> + return -EINVAL;
> +
> + max_regs = max_pins / PDC_MAX_IRQS_PER_REG;
> + if (max_pins % PDC_MAX_IRQS_PER_REG)
> + max_regs++;
nit: max_regs = DIV_ROUND_UP(max_pins, PDC_MAX_IRQS_PER_REG)
> + for (reg = 0; reg < max_regs; reg++)
> + pdc_reg_write(IRQ_ENABLE_BANK, reg, 0);
This doesn't feel correct to me, but maybe I'm misunderstanding the
hardware (I don't think I have access to a reference manual). Looking
at the example in the bindings, I see:
qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
In that example we have mappings for PDC ports:
0 - 93 (count = 94)
94 - 108 (count = 15)
115 - 121 (count = 7)
Notice the slight discontinuity there. I presume that discontinuity
is normal / allowed? If so, if there is enough of it then I think
your math could be wrong, though with the example you get lucky and it
works out OK. It's easy to see the problem with a slightly different
example: Imagine that you had this:
0 - 33 (count = 34)
94 - 108 (count = 15)
115 - 121 (count = 7)
...now max_pins = 56 and max_regs = 2. So you'll init reg 0 and 1.
...but (IIUC) you actually should be initting 0, 1, 2, and 3.
I have no idea what might be in those discontinuous ranges and if it's
always OK to clear, but (assuming it is) one fix is to put your
clearing loop _inside_ the other "for" loop in this function, AKA:
for (reg = pdc_region[n].pin_base / PDC_MAX_IRQS_PER_REG;
reg < DIV_ROUND_UP(pdc_region[n].pin_base + pdc_region[n].cnt),
PDC_MAX_IRQS_PER_REG)
reg++)
...or another option is to keep track of the max "pin_base + cnt" and
loop from 0 to there? I just don't know your hardware well enough to
tell which would be right.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 5/5] irqchip: qcom-pdc: Reset all pdc interrupts during init
2020-07-13 22:17 ` Doug Anderson
@ 2020-07-14 11:01 ` Maulik Shah
0 siblings, 0 replies; 17+ messages in thread
From: Maulik Shah @ 2020-07-14 11:01 UTC (permalink / raw)
To: Doug Anderson
Cc: Bjorn Andersson, Marc Zyngier, LinusW, Stephen Boyd, Evan Green,
Matthias Kaehlcke, LKML, linux-arm-msm, open list:GPIO SUBSYSTEM,
Andy Gross, Thomas Gleixner, Jason Cooper, Rajendra Nayak,
Lina Iyer, Srinivas Rao L
Hi,
On 7/14/2020 3:47 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Jun 22, 2020 at 2:33 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>> Clear previous kernel's configuration during init by resetting
>> all interrupts in enable bank to zero.
>>
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>> ---
>> drivers/irqchip/qcom-pdc.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
>> index 8beb6f7..11a9d3a 100644
>> --- a/drivers/irqchip/qcom-pdc.c
>> +++ b/drivers/irqchip/qcom-pdc.c
>> @@ -19,6 +19,7 @@
>> #include <linux/slab.h>
>> #include <linux/types.h>
>>
>> +#define PDC_MAX_IRQS_PER_REG 32
>> #define PDC_MAX_IRQS 168
>> #define PDC_MAX_GPIO_IRQS 256
>>
>> @@ -339,6 +340,7 @@ static const struct irq_domain_ops qcom_pdc_gpio_ops = {
>> static int pdc_setup_pin_mapping(struct device_node *np)
>> {
>> int ret, n;
>> + u32 reg, max_regs, max_pins = 0;
>>
>> n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
>> if (n <= 0 || n % 3)
>> @@ -367,8 +369,19 @@ static int pdc_setup_pin_mapping(struct device_node *np)
>> &pdc_region[n].cnt);
>> if (ret)
>> return ret;
>> + max_pins += pdc_region[n].cnt;
>> }
>>
>> + if (max_pins > PDC_MAX_IRQS)
>> + return -EINVAL;
>> +
>> + max_regs = max_pins / PDC_MAX_IRQS_PER_REG;
>> + if (max_pins % PDC_MAX_IRQS_PER_REG)
>> + max_regs++;
> nit: max_regs = DIV_ROUND_UP(max_pins, PDC_MAX_IRQS_PER_REG)
>
>
>> + for (reg = 0; reg < max_regs; reg++)
>> + pdc_reg_write(IRQ_ENABLE_BANK, reg, 0);
> This doesn't feel correct to me, but maybe I'm misunderstanding the
> hardware (I don't think I have access to a reference manual). Looking
> at the example in the bindings, I see:
>
> qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
>
> In that example we have mappings for PDC ports:
> 0 - 93 (count = 94)
> 94 - 108 (count = 15)
> 115 - 121 (count = 7)
>
> Notice the slight discontinuity there. I presume that discontinuity
> is normal / allowed? If so, if there is enough of it then I think
> your math could be wrong, though with the example you get lucky and it
> works out OK. It's easy to see the problem with a slightly different
> example: Imagine that you had this:
>
> 0 - 33 (count = 34)
> 94 - 108 (count = 15)
> 115 - 121 (count = 7)
>
> ...now max_pins = 56 and max_regs = 2. So you'll init reg 0 and 1.
> ...but (IIUC) you actually should be initting 0, 1, 2, and 3.
Right, Thanks for cacthing this. I will fix in next revision.
Thanks,
Maulik
> I have no idea what might be in those discontinuous ranges and if it's
> always OK to clear, but (assuming it is) one fix is to put your
> clearing loop _inside_ the other "for" loop in this function, AKA:
>
> for (reg = pdc_region[n].pin_base / PDC_MAX_IRQS_PER_REG;
> reg < DIV_ROUND_UP(pdc_region[n].pin_base + pdc_region[n].cnt),
> PDC_MAX_IRQS_PER_REG)
> reg++)
>
> ...or another option is to keep track of the max "pin_base + cnt" and
> loop from 0 to there? I just don't know your hardware well enough to
> tell which would be right.
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 17+ messages in thread