[PATCH] mfd: intel-lpss: Put I2C and SPI controllers into reset state on suspend

[PATCH] mfd: intel-lpss: Put I2C and SPI controllers into reset state on suspend

[Furquan Shaikh]

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

Re: [PATCH] mfd: intel-lpss: Put I2C and SPI controllers into reset state on suspend

[Furquan Shaikh]

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
>

Re: [PATCH] mfd: intel-lpss: Put I2C and SPI controllers into reset state on suspend

[Lee Jones]

On Mon, 21 Aug 2017, Furquan Shaikh wrote:

> Hello Lee,
> 
> Gentle ping. Do you see any issues with the following change?

Couple of things:

1. Please do not top post.
2. Please do not send contentless pings.

If you think a patch had fallen through the cracks (which this one
had), please resend the patch with [RESEND] in the subject line.
[RESEND]s will normally be prioritised over [PATCH]es.

No need to do that this time, but please remember for next.

I'll deal with this patch now, since it looks pretty trivial.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] mfd: intel-lpss: Put I2C and SPI controllers into reset state on suspend

[Lee Jones]

On Sun, 23 Jul 2017, Furquan Shaikh 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(+)

Applied, thanks.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog