Changes in v3:
- Drop gpiolib change (v2 patch 1) since its already in linux-next
- Add Acked-by Linus Walleij for v2 patch 2 and v2 patch 3.
- Address Stephen's comment to on v2 patch 3
- Address Stephen's comment to change variable to static on v2 patch 4.
- Add a new change to use return value from .irq_set_wake callback
- Add a new change to reset PDC irq enable bank during init time
Changes in v2:
- Fix compiler error on gpiolib patch
This series adds support to lazy disable pdc interrupt.
Some drivers using gpio interrupts want to configure gpio for wakeup using
enable_irq_wake() but during suspend entry disables irq and expects system
to resume when interrupt occurs. In the driver resume call interrupt is
re-enabled and removes wakeup capability using disable_irq_wake() one such
example is cros ec driver.
With [1] in documentation saying "An irq can be disabled with disable_irq()
and still wake the system as long as the irq has wake enabled".
The PDC IRQs are currently "unlazy disabled" (disable here means that it
will be masked in PDC & GIC HW GICD_ISENABLER, the moment driver invokes
disable_irq()) such IRQs can not wakeup from low power modes like suspend
to RAM since the driver chosen to disable this.
During suspend entry, no one re-enable/unmask in HW, even if its marked for
wakeup.
One solutions thought to address this problem was...During suspend entry at
last point, irq chip driver re-enable/unmask IRQs in HW that are marked for
wakeup. This was attemped in [2].
This series adds alternate solution to [2] by "lazy disable" IRQs in HW.
The genirq takes care of lazy disable in case if irqchip did not implement
irq_disable callback. Below is high level steps on how this works out..
a. During driver's disable_irq() call, IRQ will be marked disabled in SW
b. IRQ will still be enabled(read unmasked in HW)
c. The device then enters low power mode like suspend to RAM
d. The HW detects unmasked IRQs and wakesup the CPU
e. During resume after local_irq_enable() CPU goes to handle the wake IRQ
f. Generic handler comes to know that IRQ is disabled in SW
g. Generic handler marks IRQ as pending and now invokes mask callback
h. IRQ gets disabled/masked in HW now
i. When driver invokes enable_irq() the SW pending IRQ leads IRQ's handler
j. enable_irq() will again enable/unmask in HW
[1] https://www.spinics.net/lists/kernel/msg3398294.html
[2] https://patchwork.kernel.org/patch/11466021/
Maulik Shah (5):
pinctrl: qcom: Remove irq_disable callback from msmgpio irqchip
pinctrl: qcom: Add msmgpio irqchip flags
pinctrl: qcom: Use return value from irq_set_wake call
irqchip: qcom-pdc: Introduce irq_set_wake call
irqchip: qcom-pdc: Reset all pdc interrupts during init
drivers/irqchip/qcom-pdc.c | 47 +++++++++++++++++++++++---------------
drivers/pinctrl/qcom/pinctrl-msm.c | 23 ++++---------------
2 files changed, 34 insertions(+), 36 deletions(-)
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
Remove irq_disable callback to allow lazy disable for pdc interrupts.
Add irq_set_wake callback that unmask interrupt in HW when drivers
mark interrupt for wakeup. Interrupt will be cleared in HW during
lazy disable if its not marked for wakeup.
Signed-off-by: Maulik Shah <[email protected]>
---
drivers/irqchip/qcom-pdc.c | 34 ++++++++++++++++------------------
1 file changed, 16 insertions(+), 18 deletions(-)
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
msmgpio irqchip is not using return value of irq_set_wake call.
Start using it.
Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
Signed-off-by: Maulik Shah <[email protected]>
---
drivers/pinctrl/qcom/pinctrl-msm.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
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
Add irqchip specific flags for msmgpio irqchip to mask non wakeirqs
during suspend and mask before setting irq type.
Masking before changing type should make sure any spurious interrupt
is not detected during this operation.
Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
Acked-by: Linus Walleij <[email protected]>
Signed-off-by: Maulik Shah <[email protected]>
---
drivers/pinctrl/qcom/pinctrl-msm.c | 2 ++
1 file changed, 2 insertions(+)
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
Clear previous kernel's configuration during init by resetting
all interrupts in enable bank to zero.
Signed-off-by: Maulik Shah <[email protected]>
---
drivers/irqchip/qcom-pdc.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
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
Hi,
On Mon, Jun 22, 2020 at 2:32 AM Maulik Shah <[email protected]> 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 <[email protected]>
> ---
> drivers/pinctrl/qcom/pinctrl-msm.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
Seems right to me.
Reviewed-by: Douglas Anderson <[email protected]>
Hi,
On Mon, Jun 22, 2020 at 2:33 AM Maulik Shah <[email protected]> wrote:
>
> Clear previous kernel's configuration during init by resetting
> all interrupts in enable bank to zero.
>
> Signed-off-by: Maulik Shah <[email protected]>
> ---
> drivers/irqchip/qcom-pdc.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> 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.
Hi,
On Mon, Jun 22, 2020 at 2:33 AM Maulik Shah <[email protected]> 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 <[email protected]>
> ---
> 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
>
Hi,
On Mon, Jun 22, 2020 at 2:32 AM Maulik Shah <[email protected]> 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 <[email protected]>
> Signed-off-by: Maulik Shah <[email protected]>
> ---
> 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
Hi,
On 7/14/2020 3:47 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Jun 22, 2020 at 2:32 AM Maulik Shah <[email protected]> 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 <[email protected]>
>> Signed-off-by: Maulik Shah <[email protected]>
>> ---
>> 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
Hi,
On 7/14/2020 3:46 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Jun 22, 2020 at 2:33 AM Maulik Shah <[email protected]> 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 <[email protected]>
>> ---
>> 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
Hi,
On 7/14/2020 3:47 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Jun 22, 2020 at 2:33 AM Maulik Shah <[email protected]> wrote:
>> Clear previous kernel's configuration during init by resetting
>> all interrupts in enable bank to zero.
>>
>> Signed-off-by: Maulik Shah <[email protected]>
>> ---
>> drivers/irqchip/qcom-pdc.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> 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
On Mon, Jun 22, 2020 at 11:32 AM Maulik Shah <[email protected]> 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 <[email protected]>
Is this something that's causing regressions so I should apply it for
fixes, or is it fine to keep this with the rest of the series for v5.9?
Yours,
Linus Walleij
Hi,
On Thu, Jul 16, 2020 at 6:19 AM Linus Walleij <[email protected]> wrote:
>
> On Mon, Jun 22, 2020 at 11:32 AM Maulik Shah <[email protected]> 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 <[email protected]>
>
> Is this something that's causing regressions so I should apply it for
> fixes, or is it fine to keep this with the rest of the series for v5.9?
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