diff mbox series

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

Message ID 1590253873-11556-4-git-send-email-mkshah@codeaurora.org (mailing list archive)
State Superseded
Headers show
Series irqchip: qcom: pdc: Introduce irq_set_wake call | expand

Commit Message

Maulik Shah May 23, 2020, 5:11 p.m. UTC
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(+)

Comments

Stephen Boyd May 27, 2020, 9:47 a.m. UTC | #1
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) {
Maulik Shah May 27, 2020, 12:30 p.m. UTC | #2
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) {
diff mbox series

Patch

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