MIME-Version: 1.0
In-Reply-To: <20170724060219.12244-1-furquan@google.com>
References: <20170724060219.12244-1-furquan@google.com>
From: Furquan Shaikh <furquan@google.com>
Date: Mon, 21 Aug 2017 20:37:57 -0700
Message-ID: <CAEGmHFEsTwEu-a6S=snLH4BhTV_a0W9V=r=F1RfcAH9_xDEfEg@mail.gmail.com>
Subject: Re: [PATCH] mfd: intel-lpss: Put I2C and SPI controllers into reset
 state on suspend
To: "Lee Jones )" <lee.jones@linaro.org>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Rajat Jain <rajatja@google.com>, Nicolas Boichat <drinkcat@google.com>,
        azhar.shaikh@intel.com,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        mika.westerberg@linux.intel.com, andriy.shevchenko@linux.intel.com,
        Furquan Shaikh <furquan@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2947
Lines: 79

Hello Lee,

Gentle ping. Do you see any issues with the following change?

Thanks,
Furquan

On Sun, Jul 23, 2017 at 11:02 PM, Furquan Shaikh <furquan@google.com> wrote:
>
> Commit 274e43edcda6f ("mfd: intel-lpss: Do not put device in reset
> state on suspend") changed the behavior on suspend by not putting LPSS
> controllers into reset. This was done because S3/S0ix fail if UART
> device is put into reset and no_console_suspend flag is enabled.
>
> Because of the above change, I2C controller gets into a bad state if
> it observes that the I2C lines are pulled low when power to I2C device
> is cut off during suspend (generally, I2C lines are pulled to power
> rail of the I2C device in order to ensure that there is no leakage
> because of the pulls when device is turned off). This results in the
> controller timing out for all future I2C operations after resume. It
> is primarily because of the following sequence of operations:
>
> During suspend:
> 1. I2C controller is disabled, but it is not put into reset.
> 2. Power to I2C device is cut off.
> 3. #2 results in the I2C lines being pulled low.
>
> ==> At this point the I2C controller gets into a bad state
>
> On resume:
> 1. Power to I2C device is enabled.
> 2. #2 results in the I2C lines being pulled high.
> 3. I2C controller is enabled.
>
> However, even after enabling the I2C controller, all future I2C xfers
> fail since the controller is in a bad state and does not attempt to
> make any transactions and hence times out.
>
> In order to ensure that the controller does not get into a bad state,
> this change puts it into reset if the controller type is not
> UART. With this change, the order of operations is:
>
> During suspend:
> 1. I2C controller is disabled and put into reset.
> 2. Power to I2C device is cut off.
> 3. #2 results in the I2C lines being pulled low.
>
> On resume:
> 1. Power to I2C device is enabled.
> 2. #2 results in the I2C lines being pulled high.
> 3. I2C controller is enabled and taken out of reset.
>
> Signed-off-by: Furquan Shaikh <furquan@google.com>
> ---
>  drivers/mfd/intel-lpss.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> index 70c646b0097d..0e0ab9bb1530 100644
> --- a/drivers/mfd/intel-lpss.c
> +++ b/drivers/mfd/intel-lpss.c
> @@ -502,6 +502,14 @@ int intel_lpss_suspend(struct device *dev)
>         for (i = 0; i < LPSS_PRIV_REG_COUNT; i++)
>                 lpss->priv_ctx[i] = readl(lpss->priv + i * 4);
>
> +       /*
> +        * If the device type is not UART, then put the controller into
> +        * reset. UART cannot be put into reset since S3/S0ix fail when
> +        * no_console_suspend flag is enabled.
> +        */
> +       if (lpss->type != LPSS_DEV_UART)
> +               writel(0, lpss->priv + LPSS_PRIV_RESETS);
> +
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(intel_lpss_suspend);
> --
> 2.14.0.rc0.284.gd933b75aa4-goog
>