Re: [PATCH] spi: pxa2xx: restore lpss state after resume

[Curtis Malainey <cujomalainey@xxxxxxxxxx>]

On Sun, Aug 18, 2019 at 11:46 PM Jarkko Nikula
<jarkko.nikula@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > Cc: Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx>
> > ---
> >   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,