| Message ID | 20190816223333.144924-1-cujomalainey@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: pxa2xx: restore lpss state after resume | expand |
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,
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,
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); }
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(+)