Re: [PATCH v16 6/7] spi: pxa2xx: restore LPSS private register state on S3 resume

[Shih-Yuan Lee (FourDollars)]

Hi Andy,

> Is this series AI-assisted?
Yes, I used an LLM assistant to help format and polish the commit messages.
However, all hardware register analysis (such as inspecting BAR0 +
0x204 LPSS Private Resets), DSDT ACPI overrides, schematic
verifications for logic board 820-00244-A, and physical S3
suspend/resume hardware testing on MacBook8,1 were performed
empirically on real hardware.

> > 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, leaving the LPSS private registers
> > in their power-on-reset state, which causes two problems:
> >
> > 1. LPSS_PRIV_RESETS (offset 0x04 within the LPSS private space) stays
> >    zero, keeping the functional block in reset. Any MMIO access while
> >    the block is held in reset causes a PCIe Completion Timeout and a
> >    watchdog-triggered system reset. LPSS_PRIV_RESETS_FUNC and
> >    LPSS_PRIV_RESETS_IDMA must be de-asserted before any other register
> >    access on resume.
> >
> > 2. The LPSS software chip-select control register 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.
> >
> > To resolve these issues safely:
> > - Wrap S3 suspend/resume with pm_runtime_resume_and_get() and
> >   pm_runtime_put_noidle() to guarantee active clocks during MMIO
> >   access and preserve PM reference counting.
> > - Restrict LPSS private register save/restore to LPT, BYT, and BSW
>
> ^^^^ (1)
>
> >   platforms via pxa2xx_spi_need_lpss_restore() (newer platforms are
> >   handled by intel-lpss.c).
> > - Save only the first 6 LPSS private registers (offsets 0x00..0x14) in
> >   drv_data during suspend, avoiding reserved offsets beyond 0x14.
> > - On resume, de-assert resets first, restore saved registers, call
> >   lpss_ssp_setup(), and clear drv_data->suspended to prevent unclocked
> >   IRQ access.
> > - Add error recovery paths for spi_controller_suspend/resume failures.
> > - On the resume error path, call pm_runtime_set_suspended() before
> >   pm_runtime_put_noidle() to align the PM runtime state with the
> >   already-disabled hardware clock, preventing pxa2xx_spi_runtime_suspend()
> >   from attempting unclocked MMIO via pxa2xx_spi_off().
>
> This is an ugly hack.
>
> Saving context is done in drivers/acpi/x86/lpss.c (see #1 why this file).
> If something wrong in the flow it has to be fixed there, not here.
I checked drivers/acpi/x86/lpss.c and its LPSS_SAVE_CTX mechanism.
However, on MacBook8,1, the LPSS SPI controller is instantiated as a
PCI device (00:15.4 / 8086:9ce6) bound to spi-pxa2xx-pci.c, rather
than an ACPI platform device handled by drivers/acpi/x86/lpss.c. As a
result, acpi_lpss_restore_ctx() is never invoked for this PCI device.

Physical hardware S3 testing (rtcwake -m mem) confirms that across S3
suspend, LPSS_PRIV_RESETS (BAR0 + 0x204) is reset to 0x00000000
(functional block held in reset) by the PCH, causing all post-resume
SPI transfers to fail  continuously with -110 timeouts (applespi
spi-APP000D:00: SPI transfer timed out).

I understand your point about not doing context saving in spi-pxa2xx.c.
Would moving the LPSS private register context save/restore into
spi-pxa2xx-pci.c (the PCI glue driver) be an acceptable approach for
PCI-enumerated devices?

Thanks,
Shih-Yuan