Re: [PATCH v14 2/3] spi: pxa2xx: fix runtime PM and interrupt handling in PIO mode

Mark Brown <[email protected]> Sat, 18 Jul 2026 21:14:23 +0100
Newsgroups	org.kernel.vger.linux-spi,org.kernel.vger.linux-kernel
Message-ID	<[email protected]>
On Sun, Jul 19, 2026 at 12:07:01AM +0800, Shih-Yuan Lee wrote:

> When operating in PIO mode on Intel LPSS SPI controllers, runtime PM
> autosuspend clock-gates the hardware block, causing PCIe Completion
> Timeouts on subsequent MMIO accesses.
> 
> To fix this:
> - Scope PIO mode autosuspend lockout (pm_runtime_get_noresume() /
>   put_sync()) strictly to LPSS controllers via is_lpss_ssp(), preserving
>   autosuspend for non-LPSS SoCs.
> - Acquire pm_runtime_get_noresume() exactly once in pxa2xx_spi_probe()
>   after spi_register_controller() succeeds, and release it via
>   pm_runtime_put_sync() in pxa2xx_spi_remove(). Using pm_runtime_put_sync()
>   ensures the runtime PM state machine transitions to RPM_SUSPENDED upon
>   driver unbind, preventing active child count leaks on the parent LPSS
>   power domain.
> - Track clock state using drv_data->clk_enabled in helper functions
>   pxa2xx_spi_clk_enable/disable() to prevent clock count underflows
>   on error paths.
> - Protect ssp_int() using drv_data->clk_enabled and drv_data->suspended
>   flags to return IRQ_NONE immediately if the device is suspended or
>   unclocked, avoiding unclocked MMIO reads and level-triggered shared IRQ
>   storms. Active SPI transfers hold a PM reference via the SPI core,
>   preventing autosuspend during FIFO transfers.
> - Register IRQ in probe only after enabling the clock and clearing
>   suspended.
> - Overhaul teardown sequence in pxa2xx_spi_remove(), suspend, and
>   runtime_suspend: first disable SSP hardware IRQ generation via
>   pxa_ssp_disable(), set suspended flag, synchronize IRQ to drain in-flight
>   handlers, and then disable clocks.
> - Guard MMIO access in pxa2xx_spi_runtime_suspend() with
>   drv_data->clk_enabled check.

This is a very big change doing a lot of different things which could
probably use being split out, if you find yourself writing a list like
this it's often a sign that the bullet points should be split out (as
submitting-patches.rst hints at).

> +static int pxa2xx_spi_clk_enable(struct driver_data *drv_data)
> +{
> +	int status;
> +
> +	if (drv_data->clk_enabled)
> +		return 0;
> +
> +	status = clk_prepare_enable(drv_data->ssp->clk);
> +	if (status == 0)
> +		drv_data->clk_enabled = true;
> +
> +	return status;
> +}

This sort of refcount suppression is often a bit worrying, why are we
doing things this way - the changelog mentions error paths but which
error paths, and why is this the most sensible fix?  

Also we typically use 'ret' in the kernel.

>  	/*
> -	 * The IRQ might be shared with other peripherals so we must first
> -	 * check that are we RPM suspended or not. If we are we assume that
> -	 * the IRQ was not for us (we shouldn't be RPM suspended when the
> -	 * interrupt is enabled).
> +	 * The IRQ might be shared with other peripherals or trigger during
> +	 * power state transitions. First check if device is suspended or if
> +	 * clock is disabled; if so, return IRQ_NONE immediately to avoid
> +	 * unclocked MMIO reads.
>  	 */
> -	if (pm_runtime_suspended(drv_data->ssp->dev))
> +	if (drv_data->suspended || !drv_data->clk_enabled)
>  		return IRQ_NONE;

Why are we switching to this suspended flag rather than using
pm_runtime_suspended()?

>  static int pxa2xx_spi_runtime_suspend(struct device *dev)
>  {
>  	struct driver_data *drv_data = dev_get_drvdata(dev);
>  
> -	clk_disable_unprepare(drv_data->ssp->clk);
> +	if (!drv_data->clk_enabled)
> +		return 0;
> +
> +	pxa_ssp_disable(drv_data->ssp);

Note that there's an inconsistently quirk to suppress SSP disables on
MMP2, did you check if we need to do that here?  It comes from
29d7e05c5f75 ("spi: pxa2xx: Avoid touching SSCR0_SSE on MMP2").
signature.asc (application/pgp-signature, 488 B)