spi: pxa2xx: restore lpss state after resume
diff mbox series

Message ID 20190816223333.144924-1-cujomalainey@chromium.org
State New
Headers show
Series
  • spi: pxa2xx: restore lpss state after resume
Related show

Commit Message

Curtis Malainey Aug. 16, 2019, 10:33 p.m. UTC
On broadwell machines it has been observed that the registers do not
maintain their state through a suspend resume cycle. This is given that
after a suspend resume cycle the SW CS bit is no longer set. This can
break reads as CS will now be asserted between transmissions in messages
and therefore reset the slave device unintentionally.

Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/spi/spi-pxa2xx.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Jarkko Nikula Aug. 19, 2019, 6:46 a.m. UTC | #1
On 8/17/19 1:33 AM, Curtis Malainey wrote:
> On broadwell machines it has been observed that the registers do not
> maintain their state through a suspend resume cycle. This is given that
> after a suspend resume cycle the SW CS bit is no longer set. This can
> break reads as CS will now be asserted between transmissions in messages
> and therefore reset the slave device unintentionally.
> 
> Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
>   drivers/spi/spi-pxa2xx.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
> index fc7ab4b268802..3f313a9755640 100644
> --- a/drivers/spi/spi-pxa2xx.c
> +++ b/drivers/spi/spi-pxa2xx.c
> @@ -1913,6 +1913,9 @@ static int pxa2xx_spi_resume(struct device *dev)
>   			return status;
>   	}
>   
> +	if (is_lpss_ssp(drv_data))
> +		lpss_ssp_setup(drv_data);
> +
>   	/* Start the queue running */
>   	return spi_controller_resume(drv_data->controller);
>   }

So there is actually a regression caused by my b53548f9d9e4 ("spi: 
pxa2xx: Remove LPSS private register restoring during resume").

Which suggests to me there may be need to save/restore other private 
registers too.

Do you Andy or Heikki remember why this LPSS context save/restore wasn't 
implemented for Lynxpoint? I was testing my above commit on a Haswell 
based machine which didn't need it but apparently Broadwell needs.

Curtis: would a diff below fix the issue you are seeing? I added context 
save/restore for I2C and UART controllers too.

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index d696f165a50e..60bbc5090abe 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -219,12 +219,13 @@ static void bsw_pwm_setup(struct lpss_private_data 
*pdata)
  }

  static const struct lpss_device_desc lpt_dev_desc = {
-	.flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_CLK_DIVIDER | LPSS_LTR,
+	.flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_CLK_DIVIDER | LPSS_LTR
+			| LPSS_SAVE_CTX,
  	.prv_offset = 0x800,
  };

  static const struct lpss_device_desc lpt_i2c_dev_desc = {
-	.flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_LTR,
+	.flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_LTR | LPSS_SAVE_CTX,
  	.prv_offset = 0x800,
  };

@@ -236,7 +237,8 @@ static struct property_entry uart_properties[] = {
  };

  static const struct lpss_device_desc lpt_uart_dev_desc = {
-	.flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_CLK_DIVIDER | LPSS_LTR,
+	.flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_CLK_DIVIDER | LPSS_LTR
+			| LPSS_SAVE_CTX,
  	.clk_con_id = "baudclk",
  	.prv_offset = 0x800,
  	.setup = lpss_uart_setup,
Curtis Malainey Aug. 19, 2019, 7:43 p.m. UTC | #2
On Sun, Aug 18, 2019 at 11:46 PM Jarkko Nikula
<jarkko.nikula@linux.intel.com> wrote:
>
> On 8/17/19 1:33 AM, Curtis Malainey wrote:
> > On broadwell machines it has been observed that the registers do not
> > maintain their state through a suspend resume cycle. This is given that
> > after a suspend resume cycle the SW CS bit is no longer set. This can
> > break reads as CS will now be asserted between transmissions in messages
> > and therefore reset the slave device unintentionally.
> >
> > Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
> > Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > ---
> >   drivers/spi/spi-pxa2xx.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
> > index fc7ab4b268802..3f313a9755640 100644
> > --- a/drivers/spi/spi-pxa2xx.c
> > +++ b/drivers/spi/spi-pxa2xx.c
> > @@ -1913,6 +1913,9 @@ static int pxa2xx_spi_resume(struct device *dev)
> >                       return status;
> >       }
> >
> > +     if (is_lpss_ssp(drv_data))
> > +             lpss_ssp_setup(drv_data);
> > +
> >       /* Start the queue running */
> >       return spi_controller_resume(drv_data->controller);
> >   }
>
> So there is actually a regression caused by my b53548f9d9e4 ("spi:
> pxa2xx: Remove LPSS private register restoring during resume").
>
> Which suggests to me there may be need to save/restore other private
> registers too.
>
> Do you Andy or Heikki remember why this LPSS context save/restore wasn't
> implemented for Lynxpoint? I was testing my above commit on a Haswell
> based machine which didn't need it but apparently Broadwell needs.
>
> Curtis: would a diff below fix the issue you are seeing? I added context
> save/restore for I2C and UART controllers too.
>

I tried that and it worked with SPI for me. It preserved the CS SW bit. :)
Thanks for the quick response. I'll add a tested-by if you send a patch.

> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index d696f165a50e..60bbc5090abe 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -219,12 +219,13 @@ static void bsw_pwm_setup(struct lpss_private_data
> *pdata)
>   }
>
>   static const struct lpss_device_desc lpt_dev_desc = {
> -       .flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_CLK_DIVIDER | LPSS_LTR,
> +       .flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_CLK_DIVIDER | LPSS_LTR
> +                       | LPSS_SAVE_CTX,
>         .prv_offset = 0x800,
>   };
>
>   static const struct lpss_device_desc lpt_i2c_dev_desc = {
> -       .flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_LTR,
> +       .flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_LTR | LPSS_SAVE_CTX,
>         .prv_offset = 0x800,
>   };
>
> @@ -236,7 +237,8 @@ static struct property_entry uart_properties[] = {
>   };
>
>   static const struct lpss_device_desc lpt_uart_dev_desc = {
> -       .flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_CLK_DIVIDER | LPSS_LTR,
> +       .flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_CLK_DIVIDER | LPSS_LTR
> +                       | LPSS_SAVE_CTX,
>         .clk_con_id = "baudclk",
>         .prv_offset = 0x800,
>         .setup = lpss_uart_setup,

Patch
diff mbox series

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index fc7ab4b268802..3f313a9755640 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1913,6 +1913,9 @@  static int pxa2xx_spi_resume(struct device *dev)
 			return status;
 	}
 
+	if (is_lpss_ssp(drv_data))
+		lpss_ssp_setup(drv_data);
+
 	/* Start the queue running */
 	return spi_controller_resume(drv_data->controller);
 }