linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Anson Huang <anson.huang@nxp.com>
To: "linus.walleij@linaro.org" <linus.walleij@linaro.org>,
	"bgolaszewski@baylibre.com" <bgolaszewski@baylibre.com>,
	"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc: dl-linux-imx <linux-imx@nxp.com>
Subject: RE: [PATCH] gpio: mxc: skip GPIO_IMR restore in noirq resume
Date: Fri, 9 Nov 2018 05:01:12 +0000	[thread overview]
Message-ID: <DB3PR0402MB39160977196DBEA775A4AFCEF5C60@DB3PR0402MB3916.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <1541674114-843-1-git-send-email-Anson.Huang@nxp.com>

Please ignore this patch, as it can NOT completely fix the issue of the case when GPIO IRQ coming during the noirq suspend/resume phase, the correct solution should be to save/restore the GPIO registers when local irq is off, so move the GPIO noirq suspend/resume to syscore phase, I have send out another patch "[PATCH] gpio: mxc: move gpio noirq suspend/resume to syscore phase", please review this patch, thanks!

Best Regards!
Anson Huang

> -----Original Message-----
> From: Anson Huang
> Sent: 2018年11月8日 18:54
> To: linus.walleij@linaro.org; bgolaszewski@baylibre.com;
> linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: [PATCH] gpio: mxc: skip GPIO_IMR restore in noirq resume
> 
> During the time window of noirq suspend and noirq resume, if a GPIO pin is
> enabled as system wakeup source, the corresponding GPIO line's IRQ will be
> unmasked, and GPIO bank will NOT lost power, when GPIO irq arrives, generic
> irq handler will mask it until GPIO driver is ready to handle it, but in GPIO noirq
> resume callback, current implementation will restore the IMR register using
> the value saved in previous noirq suspend callback which is unmasked, so this
> GPIO line with IRQ pending will be unmasked again after GPIO IMR regitster is
> restored, ARM will be triggered to handle this IRQ again, because of the change
> of commit bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function
> calls"), the mask_irq function will check the status of irq_data to decide
> whether to mask the irq, in this scenario, since the GPIO IRQ is being masked
> before GPIO noirq resume, IRQD_IRQ_MASKED flag is set, so the second time
> GPIO IRQ triggered by restoring GPIO IMR register, the irq_mask callback will
> be skipped and cause ARM busy handling the GPIO IRQ come all the way and
> no response to user anymore.
> 
> To make the scenario easy to understand, I take GPIO1_0 for example when it
> is used as wake up source:
> 
> 1. GPIO1_0 is enabled as wakeup source, it will be unmasked; 2. In noirq
> suspend, GPIO driver saves GPIO1_0's mask state in
>    IMR register as "unmasked";
> 3. System go into suspend;
> 4. GPIO1_0 IRQ arrives, system wakes up; 5. Before noirq resume phase, ARM
> handles the GPIO1_0 IRQ, set
>    IRQD_IRQ_MASKED flag and call GPIO irq_mask callback to mask
>    it, as GPIO driver is NOT ready to handle it, now GPIO1_0
>    IRQ is masked;
> 6. In GPIO noirq resume, GPIO driver restores GPIO registers,
>    GPIO1_0 IRQ will be restored to unmask state again and system
>    IRQ triggered;
> 7. ARM handles GPIO1_0 IRQ again, this time the GPIO1_0 will NOT be
>    masked since its irq_data already has IRQD_IRQ_MASKED flag set; 8.
> GPIO1_0 IRQ keeps coming, ARM will be busy handling it but always
>    skip the irq_mask operation, system no response.
> 
> Although this issue is exposed by commit bf22ff45bed6 ("genirq: Avoid
> unnecessary low level irq function calls"), but actually we should skip the GPIO
> IMR register restore, as the IMR register is NOT atomic during the time window
> of GPIO noirq suspend and noirq resume, it could be changed by ARM if there
> is GPIO IRQ pending at this window.
> 
> For the scenario of GPIO bank lost power, that means no GPIO pin is enabled
> as wakeup source, all GPIO IRQs will be masked and it is same as the reset
> value when GPIO bank power ON, so no issue for this case.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/gpio/gpio-mxc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c index
> 5c69516..3d74ad7 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -531,7 +531,6 @@ static void mxc_gpio_save_regs(struct mxc_gpio_port
> *port)
> 
>  	port->gpio_saved_reg.icr1 = readl(port->base + GPIO_ICR1);
>  	port->gpio_saved_reg.icr2 = readl(port->base + GPIO_ICR2);
> -	port->gpio_saved_reg.imr = readl(port->base + GPIO_IMR);
>  	port->gpio_saved_reg.gdir = readl(port->base + GPIO_GDIR);
>  	port->gpio_saved_reg.edge_sel = readl(port->base + GPIO_EDGE_SEL);
>  	port->gpio_saved_reg.dr = readl(port->base + GPIO_DR); @@ -544,7
> +543,6 @@ static void mxc_gpio_restore_regs(struct mxc_gpio_port *port)
> 
>  	writel(port->gpio_saved_reg.icr1, port->base + GPIO_ICR1);
>  	writel(port->gpio_saved_reg.icr2, port->base + GPIO_ICR2);
> -	writel(port->gpio_saved_reg.imr, port->base + GPIO_IMR);
>  	writel(port->gpio_saved_reg.gdir, port->base + GPIO_GDIR);
>  	writel(port->gpio_saved_reg.edge_sel, port->base + GPIO_EDGE_SEL);
>  	writel(port->gpio_saved_reg.dr, port->base + GPIO_DR);
> --
> 2.7.4


      reply	other threads:[~2018-11-09  5:01 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-08 10:53 Anson Huang
2018-11-09  5:01 ` Anson Huang [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DB3PR0402MB39160977196DBEA775A4AFCEF5C60@DB3PR0402MB3916.eurprd04.prod.outlook.com \
    --to=anson.huang@nxp.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --subject='RE: [PATCH] gpio: mxc: skip GPIO_IMR restore in noirq resume' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).