Message-ID: <20180820152629.GR5081@codeaurora.org>
Date:   Mon, 20 Aug 2018 09:26:29 -0600
From:   Lina Iyer <ilina@...eaurora.org>
To:     Marc Zyngier <marc.zyngier@....com>
Cc:     bjorn.andersson@...aro.org, sboyd@...nel.org, evgreen@...omium.org,
        linus.walleij@...aro.org, rplsssn@...eaurora.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        rnayak@...eaurora.org, devicetree@...r.kernel.org,
        andy.gross@...aro.org, dianders@...omium.org
Subject: Re: [PATCH RESEND v1 2/5] drivers: pinctrl: msm: enable PDC
 interrupt only during suspend

On Sat, Aug 18 2018 at 07:13 -0600, Marc Zyngier wrote:
>Hi Lina,
>
>On Fri, 17 Aug 2018 20:10:23 +0100,
>Lina Iyer <ilina@...eaurora.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@...eaurora.org>
>> ---
>>  drivers/pinctrl/qcom/pinctrl-msm.c | 60 +++++++++++++++++++++++++++++-
>>  drivers/pinctrl/qcom/pinctrl-msm.h |  3 ++
>>  2 files changed, 62 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index 03ef1d29d078..17e541f8f09d 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_IRQ 1024
>
>Where is this value coming from? Is it guaranteed to be an
>architectural maximum? Or something that is likely to vary in future
>implementations?
>
>>  #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_irqs:       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,14 @@ struct msm_pinctrl {
>>
>>  	DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
>>  	DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
>> +	DECLARE_BITMAP(pdc_irqs, MAX_PDC_IRQ);
>>
>>  	const struct msm_pinctrl_soc_data *soc;
>>  	void __iomem *regs;
>>  };
>>
>> +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 +792,11 @@ 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);
>> +		on ? set_bit(pdc_irqd->irq, pctrl->pdc_irqs) :
>> +		     clear_bit(pdc_irqd->irq, pctrl->pdc_irqs);
>
>I think we'll all survive the two extra lines if you write this as an
>'if' statement (unless you're competing for the next IOCCC, and then
>you need to up your game a bit).
>
>Also, are you indexing the bitmap using a Linux irq number? If so,
>that's an absolute NACK. Out of the box, a Linux IRQ can be up to
>NR_IRQS+8196 on arm64, and there are plans to push that to be a much
>larger space.
>
I didn't realize this. I have been using linux IRQ number on this
bitmask and I will need to fix this.

>> +	}
>>
>>  	irq_set_irq_wake(pctrl->irq, on);
>
>I'm a bit worried by the way you call into the irq subsystem with this
>spinlock held. Have you run that code with lockdep enabled?
>
I have not tried lockdep. Will try it.
This specific line is already part of the driver. I added a similar line
irq_set_irq_wake(pdc_irqd->irq) just above following the same pattern.

>>
>> @@ -920,6 +928,8 @@ 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);
>
>Could you explain what this is trying to do? I'm trying to understand
>this code, but this function isn't in 4.18...
>
Oh, I have been able to test only on 4.14 so far. The flag does seem to
exist at least, I didn't get a compiler error.

I read this in kernel/irq/chip.c -

If the interrupt chip does not implement the irq_disable callback,
a driver can disable the lazy approach for a particular irq line by
calling 'irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY)'. This can
be used for devices which cannot disable the interrupt at the
device level under certain circumstances and have to use
disable_irq[_nosync] instead.

And interpreted this as something that this would prevent 'relaxed'
disable. I am enabling and disabling the IRQ in suspend path, that I
thought this would help avoid issues caused by late disable. Am I
mistaken?

>>  	disable_irq(irq);
>>
>>  	return 0;
>> @@ -1070,6 +1080,54 @@ 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;
>> +	int i;
>> +
>> +	in_suspend = true;
>> +	for_each_set_bit(i, pctrl->pdc_irqs, MAX_PDC_IRQ) {
>> +		irqd = irq_get_handler_data(i);
>
>So this is what I though. You're using the Linux IRQ, and not the pin
>number (or whatever HW-dependent index that would otherwise make
>sense). Please fix it.
>
Noted.

>> +		/*
>> +		 * 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.
>> +		 */
>> +		if (irqd_is_wakeup_set(irqd)) {
>> +			irq_set_irq_wake(irqd->irq, false);
>> +			disable_irq(irqd->irq);
>> +			enable_irq(i);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int __maybe_unused msm_pinctrl_resume_late(struct device *dev)
>> +{
>> +	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
>> +	struct irq_data *irqd;
>> +	int i;
>> +
>> +	for_each_set_bit(i, pctrl->pdc_irqs, MAX_PDC_IRQ) {
>> +		irqd = irq_get_handler_data(i);
>> +		/*
>> +		 * The TLMM will be operational now, so disable
>> +		 * the PDC IRQ.
>> +		 */
>> +		if (irqd_is_wakeup_set(irq_get_irq_data(i))) {
>> +			disable_irq_nosync(i);
>> +			irq_set_irq_wake(irqd->irq, true);
>> +			enable_irq(irqd->irq);
>> +		}
>> +	}
>> +	in_suspend = false;
>> +
>> +	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
>
>I can't really review this code any further, as it seems that I'm
>missing some crucial dependencies. But there is a number of things
>that feel quite wrong in this code, and that need to be addressed
>anyway.
>
Thanks for reviewing Marc.

-- Lina

Powered by blists - more mailing lists