Re: [PATCH 2/2] spi: pxa2xx: restore LPSS private and IDMA registers on S3 resume

[Mark Brown <broonie@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 2854 bytes --]

On Mon, Jul 13, 2026 at 12:24:20AM +0800, Shih-Yuan Lee wrote:
> Intel LPSS SPI controllers lose all private register state across S3
> suspend because the LPSS power domain is fully removed.  On resume the
> driver only re-enables the SSP clock but leaves the LPSS private
> registers (BAR0 0x200-0x2ff) and the IDMA registers (0x800-0x814) in
> their power-on-reset state, which causes two separate problems:

> 1. LPSS_PRIV_RESETS (0x204) stays zero, keeping the functional block
>    and IDMA in reset.  Writing 7 to de-assert both resets before any
>    register access is mandatory; accessing MMIO while in reset causes a
>    PCIe Completion Timeout and a watchdog-triggered system reset.
> 
> 2. The IDMA block shares the SPI interrupt line.  With its registers
>    zeroed the IDMA asserts a spurious interrupt that masks the real SPI
>    interrupt, causing every subsequent SPI transfer to time out (-110).
> 
> 3. The LPSS software chip-select control register (0x224) must *not* be
>    blindly restored from its suspend-time snapshot: if CS was asserted
>    at the moment of suspend, restoring that state corrupts the first
>    post-resume SPI transaction.  Instead, call lpss_ssp_setup() which
>    unconditionally writes SW_MODE | CS_HIGH (idle/deasserted), matching
>    the state established at probe time.

That's three problems, not two.

> +/*
> + * LPSS private registers to save across S3 suspend.
> + * NOTE: 0x224 (CS control) is intentionally excluded - it is re-initialised
> + * by lpss_ssp_setup() on resume to ensure CS starts deasserted (idle-high).
> + */
> +static const unsigned int lpss_saved_regs[] = {
> +	0x200,
> +	0x204,
> +	0x220,
> +	0x238,
> +};

Unnamed registers?

>  static int pxa2xx_spi_suspend(struct device *dev)
>  {
>  	struct driver_data *drv_data = dev_get_drvdata(dev);
>  	struct ssp_device *ssp = drv_data->ssp;
>  	int status;
>  
> +	if (is_lpss_ssp(drv_data) && !pm_runtime_suspended(dev)) {
> +		struct pxa2xx_spi_controller *pdata = drv_data->controller_info;
> +		int i;
> +
> +		for (i = 0; i < ARRAY_SIZE(lpss_saved_regs); i++)
> +			pdata->lpss_priv_ctx[i] = readl(ssp->mmio_base + lpss_saved_regs[i]);
> +
> +		for (i = 0; i < 6; i++)
> +			pdata->lpss_idma_ctx[i] = readl(ssp->mmio_base + 0x800 + i * 4);
> +	}
> +
>  	status = spi_controller_suspend(drv_data->controller);

This is saving the registers before we quiesce the controller, the
values might change underneath us.

> +		if (is_lpss_ssp(drv_data)) {
> +			struct pxa2xx_spi_controller *pdata = drv_data->controller_info;
> +			int i;
> +
> +			/* First de-assert resets by writing 7 to 0x204 (LPSS_PRIV_RESETS) */
> +			writel(7, ssp->mmio_base + 0x204);

Do all the is_lpss_spi() devices have the same base offset?

These magic numbers are not good.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]