Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 22 Sep 2018 17:29:07 +0100
Message-ID: <868t3twl64.wl-marc.zyngier@arm.com>
From:   Marc Zyngier <marc.zyngier@arm.com>
To:     Lina Iyer <ilina@codeaurora.org>
Cc:     bjorn.andersson@linaro.org, sboyd@kernel.org, evgreen@chromium.org,
        linus.walleij@linaro.org, rplsssn@codeaurora.org,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        rnayak@codeaurora.org, devicetree@vger.kernel.org,
        andy.gross@linaro.org, dianders@chromium.org
Subject: Re: [PATCH v3 3/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend
In-Reply-To: <20180904211810.5506-4-ilina@codeaurora.org>
References: <20180904211810.5506-1-ilina@codeaurora.org>
        <20180904211810.5506-4-ilina@codeaurora.org>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/25.1
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
Organization: ARM Ltd
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Lina,

On Tue, 04 Sep 2018 22:18:08 +0100,
Lina Iyer <ilina@codeaurora.org> wrote:
> 
> During suspend the system may power down some of the system rails. As a
> result, the TLMM hw block may not be operational anymore and wakeup
> capable GPIOs will not be detected. The PDC however will be operational
> and the GPIOs that are routed to the PDC as IRQs can wake the system up.
> 
> To avoid being interrupted twice (for TLMM and once for PDC IRQ) when a
> GPIO trips, use TLMM for active and switch to PDC for suspend. When
> entering suspend, disable the TLMM wakeup interrupt and instead enable
> the PDC IRQ and revert upon resume.
> 
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
> Changes in v3:
> 	- Enable PDC-IRQ swap only for edge interrupts
> Changes in v2:
> 	- Fix PDC IRQ max port, 126 is the max supported in h/w
> 	- Use PDC hwirq in bitmap, linux numbers could be large
> 	- Setup DISABLE_UNLAZY for both TLMM and PDC IRQs
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 73 +++++++++++++++++++++++++++++-
>  drivers/pinctrl/qcom/pinctrl-msm.h |  3 ++
>  2 files changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 6527a0a9edd1..01a455f86fcd 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -37,6 +37,7 @@
>  #include "../pinctrl-utils.h"
>  
>  #define MAX_NR_GPIO 300
> +#define MAX_PDC_HWIRQ 126
>  #define PS_HOLD_OFFSET 0x820
>  
>  /**
> @@ -51,6 +52,7 @@
>   * @enabled_irqs:   Bitmap of currently enabled irqs.
>   * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
>   *                  detection.
> + * @pdc_hwirqs:     Bitmap of wakeup capable irqs.
>   * @soc;            Reference to soc_data of platform specific data.
>   * @regs:           Base address for the TLMM register map.
>   */
> @@ -68,11 +70,15 @@ struct msm_pinctrl {
>  
>  	DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
>  	DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
> +	DECLARE_BITMAP(pdc_hwirqs, MAX_PDC_HWIRQ);
>  
>  	const struct msm_pinctrl_soc_data *soc;
>  	void __iomem *regs;
> +	struct irq_domain *pdc_irq_domain;
>  };
>  
> +static bool in_suspend;
> +
>  static int msm_get_groups_count(struct pinctrl_dev *pctldev)
>  {
>  	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> @@ -787,8 +793,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
>  
>  	raw_spin_lock_irqsave(&pctrl->lock, flags);
>  
> -	if (pdc_irqd)
> +	if (pdc_irqd && !in_suspend) {
>  		irq_set_irq_wake(pdc_irqd->irq, on);
> +		if (on)
> +			set_bit(pdc_irqd->hwirq, pctrl->pdc_hwirqs);
> +		else
> +			clear_bit(pdc_irqd->hwirq, pctrl->pdc_hwirqs);
> +	}
>  
>  	irq_set_irq_wake(pctrl->irq, on);
>  
> @@ -919,7 +930,12 @@ static int msm_gpio_pdc_pin_request(struct irq_data *d)
>  	}
>  
>  	irq_set_handler_data(d->irq, irq_get_irq_data(irq));
> +	irq_set_handler_data(irq, d);
> +	irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY);
> +	irq_set_status_flags(d->irq, IRQ_DISABLE_UNLAZY);
>  	disable_irq(irq);
> +	if (!pctrl->pdc_irq_domain)
> +		pctrl->pdc_irq_domain = irq_get_irq_data(irq)->domain;
>  
>  	return 0;
>  }
> @@ -1071,6 +1087,61 @@ static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl)
>  		}
>  }
>  
> +int __maybe_unused msm_pinctrl_suspend_late(struct device *dev)
> +{
> +	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
> +	struct irq_data *irqd;
> +	unsigned int irq;
> +	int i;
> +
> +	in_suspend = true;
> +	for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) {
> +		irq = irq_find_mapping(pctrl->pdc_irq_domain, i);
> +		irqd = irq_get_handler_data(irq);
> +		/*
> +		 * We don't know if the TLMM will be functional
> +		 * or not, during the suspend. If its functional,
> +		 * we do not want duplicate interrupts from PDC.
> +		 * Hence disable the GPIO IRQ and enable PDC IRQ
> +		 * for edge interrupt only.
> +		 */
> +		if (irqd_is_wakeup_set(irqd) && !irqd_is_level_type(irqd)) {
> +			disable_irq_wake(irqd->irq);

There is something I don't quite get here. If the PDC is used to wake
up the platform, why is the TLMM interrupt configured as a wakeup
source the first place? Or is it just to keep things simple and not
have to track it in the TLMM driver itself?

> +			disable_irq(irqd->irq);
> +			enable_irq(irq);
> +		}
> +	}

Given that you're changing in_suspend and parsing the bitmap,
shouldn't take the pdc spinlock?

> +
> +	return 0;
> +}
> +
> +int __maybe_unused msm_pinctrl_resume_late(struct device *dev)
> +{
> +	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
> +	struct irq_data *irqd, *pdc_irqd;
> +	unsigned int irq;
> +	int i;
> +
> +	for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) {
> +		irq = irq_find_mapping(pctrl->pdc_irq_domain, i);
> +		irqd = irq_get_handler_data(irq);
> +		pdc_irqd = irq_get_irq_data(irq);
> +		/*
> +		 * The TLMM will be operational now, so disable
> +		 * the PDC IRQ for edge interrupts only.
> +		 */
> +		if (irqd_is_wakeup_set(pdc_irqd) &&
> +		    !irqd_is_level_type(pdc_irqd)) {
> +			disable_irq_nosync(irq);
> +			enable_irq_wake(irqd->irq);
> +			enable_irq(irqd->irq);
> +		}
> +	}
> +	in_suspend = false;

Same remark about the lock.

> +
> +	return 0;
> +}
> +
>  int msm_pinctrl_probe(struct platform_device *pdev,
>  		      const struct msm_pinctrl_soc_data *soc_data)
>  {
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> index 9b9feea540ff..21b56fb5dae9 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> @@ -123,4 +123,7 @@ int msm_pinctrl_probe(struct platform_device *pdev,
>  		      const struct msm_pinctrl_soc_data *soc_data);
>  int msm_pinctrl_remove(struct platform_device *pdev);
>  
> +int msm_pinctrl_suspend_late(struct device *dev);
> +int msm_pinctrl_resume_late(struct device *dev);
> +
>  #endif
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.