* [PATCH v17 1/6] spi: pxa2xx: rename local status variable to ret
2026-09-30 16:06 [PATCH v17 0/6] spi: pxa2xx: PM fixes, teardown overhaul, and LPSS restore for MacBook8,1 Shih-Yuan Lee
@ 2026-09-30 16:06 ` Shih-Yuan Lee
2026-09-30 16:06 ` [PATCH v17 2/6] spi: pxa2xx: introduce clock enable and disable helper functions Shih-Yuan Lee
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Shih-Yuan Lee @ 2026-09-30 16:06 UTC (permalink / raw)
To: Mark Brown
Cc: Andy Shevchenko, Mika Westerberg, Lukas Wunner, Daniel Mack,
Haojian Zhuang, Robert Jarzmik, linux-spi, linux-kernel,
linux-arm-kernel, Shih-Yuan Lee
Rename the return value variable name from 'status' to 'ret' in the
pxa2xx_spi_probe(), pxa2xx_spi_suspend(), and pxa2xx_spi_resume()
functions to conform to standard Linux kernel coding conventions.
Assisted-by: Antigravity:gemini-3.8-flash sparse
Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
drivers/spi/spi-pxa2xx.c | 44 ++++++++++++++++++++--------------------
1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 6291d7c2e06f..b11e30589074 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1274,7 +1274,7 @@ int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
struct spi_controller *controller;
struct driver_data *drv_data;
const struct lpss_config *config;
- int status;
+ int ret;
u32 tmp;
if (platform_info->is_target)
@@ -1330,15 +1330,15 @@ int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
| SSSR_ROR | SSSR_TUR;
}
- status = request_irq(ssp->irq, ssp_int, IRQF_SHARED, dev_name(dev),
+ ret = request_irq(ssp->irq, ssp_int, IRQF_SHARED, dev_name(dev),
drv_data);
- if (status < 0)
- return dev_err_probe(dev, status, "cannot get IRQ %d\n", ssp->irq);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "cannot get IRQ %d\n", ssp->irq);
/* Setup DMA if requested */
if (platform_info->enable_dma) {
- status = pxa2xx_spi_dma_setup(drv_data);
- if (status) {
+ ret = pxa2xx_spi_dma_setup(drv_data);
+ if (ret) {
dev_warn(dev, "no DMA channels available, using PIO\n");
platform_info->enable_dma = false;
} else {
@@ -1352,8 +1352,8 @@ int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
}
/* Enable SOC clock */
- status = clk_prepare_enable(ssp->clk);
- if (status)
+ ret = clk_prepare_enable(ssp->clk);
+ if (ret)
goto out_error_dma_irq_alloc;
controller->max_speed_hz = clk_get_rate(ssp->clk);
@@ -1433,20 +1433,20 @@ int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
drv_data->gpiod_ready = devm_gpiod_get_optional(dev,
"ready", GPIOD_OUT_LOW);
if (IS_ERR(drv_data->gpiod_ready)) {
- status = PTR_ERR(drv_data->gpiod_ready);
+ ret = PTR_ERR(drv_data->gpiod_ready);
goto out_error_clock_enabled;
}
}
/* Register with the SPI framework */
dev_set_drvdata(dev, drv_data);
- status = spi_register_controller(controller);
- if (status) {
- dev_err_probe(dev, status, "problem registering SPI controller\n");
+ ret = spi_register_controller(controller);
+ if (ret) {
+ dev_err_probe(dev, ret, "problem registering SPI controller\n");
goto out_error_clock_enabled;
}
- return status;
+ return ret;
out_error_clock_enabled:
clk_disable_unprepare(ssp->clk);
@@ -1455,7 +1455,7 @@ int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
pxa2xx_spi_dma_release(drv_data);
free_irq(ssp->irq, drv_data);
- return status;
+ return ret;
}
EXPORT_SYMBOL_NS_GPL(pxa2xx_spi_probe, "SPI_PXA2xx");
@@ -1483,11 +1483,11 @@ 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;
+ int ret;
- status = spi_controller_suspend(drv_data->controller);
- if (status)
- return status;
+ ret = spi_controller_suspend(drv_data->controller);
+ if (ret)
+ return ret;
pxa_ssp_disable(ssp);
@@ -1501,13 +1501,13 @@ static int pxa2xx_spi_resume(struct device *dev)
{
struct driver_data *drv_data = dev_get_drvdata(dev);
struct ssp_device *ssp = drv_data->ssp;
- int status;
+ int ret;
/* Enable the SSP clock */
if (!pm_runtime_suspended(dev)) {
- status = clk_prepare_enable(ssp->clk);
- if (status)
- return status;
+ ret = clk_prepare_enable(ssp->clk);
+ if (ret)
+ return ret;
}
/* Start the queue running */
--
2.39.5
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v17 2/6] spi: pxa2xx: introduce clock enable and disable helper functions
2026-09-30 16:06 [PATCH v17 0/6] spi: pxa2xx: PM fixes, teardown overhaul, and LPSS restore for MacBook8,1 Shih-Yuan Lee
2026-09-30 16:06 ` [PATCH v17 1/6] spi: pxa2xx: rename local status variable to ret Shih-Yuan Lee
@ 2026-09-30 16:06 ` Shih-Yuan Lee
2026-09-30 17:31 ` Mark Brown
2026-09-30 16:06 ` [PATCH v17 3/6] spi: pxa2xx: acquire active PM runtime reference in interrupt handler Shih-Yuan Lee
` (3 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Shih-Yuan Lee @ 2026-09-30 16:06 UTC (permalink / raw)
To: Mark Brown
Cc: Andy Shevchenko, Mika Westerberg, Lukas Wunner, Daniel Mack,
Haojian Zhuang, Robert Jarzmik, linux-spi, linux-kernel,
linux-arm-kernel, Shih-Yuan Lee
The driver enables and disables the SOC clock during probe, teardown,
and power management callbacks. Directly calling clk_disable_unprepare()
when the clock is already disabled—such as when removing a device that
is runtime-suspended—causes an unbalanced clock disable warning from the
Common Clock Framework.
Introduce pxa2xx_spi_clk_enable() and pxa2xx_spi_clk_disable() helper
functions that track the clock state with a 'clk_enabled' boolean flag
protected by a 'clk_lock' mutex in struct driver_data. These helpers
make clock toggling idempotent: repeated enable or disable invocations
are safe no-ops serialized by clk_lock.
Convert probe, remove, suspend, resume, and runtime PM callbacks to use
these helpers instead of direct clk_prepare_enable() and
clk_disable_unprepare() calls.
Pack 'clk_enabled' immediately after 'n_bytes' into the existing
padding hole in struct driver_data, avoiding additional alignment
padding.
Assisted-by: Antigravity:gemini-3.8-flash sparse
Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
drivers/spi/spi-pxa2xx.c | 41 ++++++++++++++++++++++++++++++++--------
drivers/spi/spi-pxa2xx.h | 2 ++
2 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index b11e30589074..a9d19d9a5343 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -713,6 +713,31 @@ static void handle_bad_msg(struct driver_data *drv_data)
dev_err(drv_data->ssp->dev, "bad message state in interrupt handler\n");
}
+static int pxa2xx_spi_clk_enable(struct driver_data *drv_data)
+{
+ int ret = 0;
+
+ mutex_lock(&drv_data->clk_lock);
+ if (!drv_data->clk_enabled) {
+ ret = clk_prepare_enable(drv_data->ssp->clk);
+ if (!ret)
+ drv_data->clk_enabled = true;
+ }
+ mutex_unlock(&drv_data->clk_lock);
+
+ return ret;
+}
+
+static void pxa2xx_spi_clk_disable(struct driver_data *drv_data)
+{
+ mutex_lock(&drv_data->clk_lock);
+ if (drv_data->clk_enabled) {
+ drv_data->clk_enabled = false;
+ clk_disable_unprepare(drv_data->ssp->clk);
+ }
+ mutex_unlock(&drv_data->clk_lock);
+}
+
static irqreturn_t ssp_int(int irq, void *dev_id)
{
struct driver_data *drv_data = dev_id;
@@ -1288,6 +1313,7 @@ int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
drv_data->controller = controller;
drv_data->controller_info = platform_info;
drv_data->ssp = ssp;
+ mutex_init(&drv_data->clk_lock);
/* The spi->mode bits understood by this driver: */
controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP;
@@ -1352,7 +1378,7 @@ int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
}
/* Enable SOC clock */
- ret = clk_prepare_enable(ssp->clk);
+ ret = pxa2xx_spi_clk_enable(drv_data);
if (ret)
goto out_error_dma_irq_alloc;
@@ -1449,7 +1475,7 @@ int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
return ret;
out_error_clock_enabled:
- clk_disable_unprepare(ssp->clk);
+ pxa2xx_spi_clk_disable(drv_data);
out_error_dma_irq_alloc:
pxa2xx_spi_dma_release(drv_data);
@@ -1468,7 +1494,7 @@ void pxa2xx_spi_remove(struct device *dev)
/* Disable the SSP at the peripheral and SOC level */
pxa_ssp_disable(ssp);
- clk_disable_unprepare(ssp->clk);
+ pxa2xx_spi_clk_disable(drv_data);
/* Release DMA */
if (drv_data->controller_info->enable_dma)
@@ -1492,7 +1518,7 @@ static int pxa2xx_spi_suspend(struct device *dev)
pxa_ssp_disable(ssp);
if (!pm_runtime_suspended(dev))
- clk_disable_unprepare(ssp->clk);
+ pxa2xx_spi_clk_disable(drv_data);
return 0;
}
@@ -1500,12 +1526,11 @@ static int pxa2xx_spi_suspend(struct device *dev)
static int pxa2xx_spi_resume(struct device *dev)
{
struct driver_data *drv_data = dev_get_drvdata(dev);
- struct ssp_device *ssp = drv_data->ssp;
int ret;
/* Enable the SSP clock */
if (!pm_runtime_suspended(dev)) {
- ret = clk_prepare_enable(ssp->clk);
+ ret = pxa2xx_spi_clk_enable(drv_data);
if (ret)
return ret;
}
@@ -1518,7 +1543,7 @@ 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);
+ pxa2xx_spi_clk_disable(drv_data);
return 0;
}
@@ -1526,7 +1551,7 @@ static int pxa2xx_spi_runtime_resume(struct device *dev)
{
struct driver_data *drv_data = dev_get_drvdata(dev);
- return clk_prepare_enable(drv_data->ssp->clk);
+ return pxa2xx_spi_clk_enable(drv_data);
}
EXPORT_NS_GPL_DEV_PM_OPS(pxa2xx_spi_pm_ops, SPI_PXA2xx) = {
diff --git a/drivers/spi/spi-pxa2xx.h b/drivers/spi/spi-pxa2xx.h
index 447be0369384..3352239f74a0 100644
--- a/drivers/spi/spi-pxa2xx.h
+++ b/drivers/spi/spi-pxa2xx.h
@@ -66,6 +66,8 @@ struct driver_data {
void *rx;
void *rx_end;
u8 n_bytes;
+ bool clk_enabled;
+ struct mutex clk_lock;
int (*write)(struct driver_data *drv_data);
int (*read)(struct driver_data *drv_data);
irqreturn_t (*transfer_handler)(struct driver_data *drv_data);
--
2.39.5
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v17 2/6] spi: pxa2xx: introduce clock enable and disable helper functions
2026-09-30 16:06 ` [PATCH v17 2/6] spi: pxa2xx: introduce clock enable and disable helper functions Shih-Yuan Lee
@ 2026-09-30 17:31 ` Mark Brown
0 siblings, 0 replies; 10+ messages in thread
From: Mark Brown @ 2026-09-30 17:31 UTC (permalink / raw)
To: Shih-Yuan Lee
Cc: Andy Shevchenko, Mika Westerberg, Lukas Wunner, Daniel Mack,
Haojian Zhuang, Robert Jarzmik, linux-spi, linux-kernel,
linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]
On Thu, Oct 01, 2026 at 12:06:25AM +0800, Shih-Yuan Lee wrote:
> The driver enables and disables the SOC clock during probe, teardown,
> and power management callbacks. Directly calling clk_disable_unprepare()
> when the clock is already disabled—such as when removing a device that
> is runtime-suspended—causes an unbalanced clock disable warning from the
> Common Clock Framework.
> Introduce pxa2xx_spi_clk_enable() and pxa2xx_spi_clk_disable() helper
> functions that track the clock state with a 'clk_enabled' boolean flag
> protected by a 'clk_lock' mutex in struct driver_data. These helpers
> make clock toggling idempotent: repeated enable or disable invocations
> are safe no-ops serialized by clk_lock.
What problem is this solving? Usually if something is dropping a
reference to a shared resource like a clock without knowing if it took
it then whatever else might have been using the resource is going to be
broken when the clock suddenly vanishes underneath it. If we are
coordinating properly we shouldn't need the flag, the refcount should be
good enough.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v17 3/6] spi: pxa2xx: acquire active PM runtime reference in interrupt handler
2026-09-30 16:06 [PATCH v17 0/6] spi: pxa2xx: PM fixes, teardown overhaul, and LPSS restore for MacBook8,1 Shih-Yuan Lee
2026-09-30 16:06 ` [PATCH v17 1/6] spi: pxa2xx: rename local status variable to ret Shih-Yuan Lee
2026-09-30 16:06 ` [PATCH v17 2/6] spi: pxa2xx: introduce clock enable and disable helper functions Shih-Yuan Lee
@ 2026-09-30 16:06 ` Shih-Yuan Lee
2026-09-30 17:40 ` Mark Brown
2026-09-30 16:06 ` [PATCH v17 4/6] spi: pxa2xx: overhaul teardown and suspend sequence to synchronize IRQ before clock gating Shih-Yuan Lee
` (2 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Shih-Yuan Lee @ 2026-09-30 16:06 UTC (permalink / raw)
To: Mark Brown
Cc: Andy Shevchenko, Mika Westerberg, Lukas Wunner, Daniel Mack,
Haojian Zhuang, Robert Jarzmik, linux-spi, linux-kernel,
linux-arm-kernel, Shih-Yuan Lee
On a shared interrupt line, ssp_int() can be invoked while the SPI
controller is suspended or transitioning power states. In ssp_int(),
evaluating device power state without acquiring a reference before
accessing MMIO registers is not atomic: another thread executing
pm_runtime_suspend() can drop the last reference and gate the clock
immediately after the check passes. Accessing unclocked registers then
leads to PCIe Completion Timeouts and system hangs.
In addition, request_irq() is called in pxa2xx_spi_probe() before the
clock is enabled and the controller hardware is initialized, allowing
shared interrupts to fire against partially configured hardware.
Ensure the interrupt handler runs only when the hardware is clocked and
active:
1. In ssp_int(), check READ_ONCE(drv_data->clk_enabled), immediately
returning IRQ_NONE if the clock is disabled. Update 'clk_enabled'
with WRITE_ONCE() in the clock helpers to pair with the lockless
reader.
2. Call pm_runtime_get_if_active(dev) in ssp_int() to atomically check
if the device is active and acquire a PM runtime reference. If
runtime PM is enabled and the device is suspended (return value 0),
return IRQ_NONE without accessing MMIO registers. When runtime PM is
disabled (return value -EINVAL), the controller is permanently
active, so proceed with servicing the interrupt.
3. On exit, if an active PM reference was acquired (> 0), release it
via pm_runtime_put_autosuspend() when an interrupt was handled
(IRQ_HANDLED) to preserve the autosuspend delay, or via
pm_runtime_put() on IRQ_NONE to avoid refreshing the autosuspend
timer on foreign interrupts.
4. In pxa2xx_spi_probe(), move request_irq() after the clock is enabled
and hardware registers are fully initialized, right before
spi_register_controller(), ensuring no interrupt can run against
unconfigured hardware, and update error unwinding accordingly.
Assisted-by: Antigravity:gemini-3.8-flash spin sparse
Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
drivers/spi/spi-pxa2xx.c | 68 +++++++++++++++++++++++++++-------------
1 file changed, 47 insertions(+), 21 deletions(-)
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index a9d19d9a5343..b091434977af 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -721,7 +721,7 @@ static int pxa2xx_spi_clk_enable(struct driver_data *drv_data)
if (!drv_data->clk_enabled) {
ret = clk_prepare_enable(drv_data->ssp->clk);
if (!ret)
- drv_data->clk_enabled = true;
+ WRITE_ONCE(drv_data->clk_enabled, true);
}
mutex_unlock(&drv_data->clk_lock);
@@ -732,7 +732,7 @@ static void pxa2xx_spi_clk_disable(struct driver_data *drv_data)
{
mutex_lock(&drv_data->clk_lock);
if (drv_data->clk_enabled) {
- drv_data->clk_enabled = false;
+ WRITE_ONCE(drv_data->clk_enabled, false);
clk_disable_unprepare(drv_data->ssp->clk);
}
mutex_unlock(&drv_data->clk_lock);
@@ -741,17 +741,25 @@ static void pxa2xx_spi_clk_disable(struct driver_data *drv_data)
static irqreturn_t ssp_int(int irq, void *dev_id)
{
struct driver_data *drv_data = dev_id;
+ struct device *dev = drv_data->ssp->dev;
u32 sccr1_reg;
u32 mask = drv_data->mask_sr;
u32 status;
+ irqreturn_t ret;
+ int active;
/*
- * 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. Ensure the clock is active, and acquire
+ * an active PM runtime reference while ssp_int() runs. If runtime
+ * PM is enabled and the device is suspended (active == 0), return
+ * IRQ_NONE immediately without touching MMIO registers.
*/
- if (pm_runtime_suspended(drv_data->ssp->dev))
+ if (!READ_ONCE(drv_data->clk_enabled))
+ return IRQ_NONE;
+
+ active = pm_runtime_get_if_active(dev);
+ if (active == 0)
return IRQ_NONE;
/*
@@ -762,7 +770,7 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
*/
status = pxa2xx_spi_read(drv_data, SSSR);
if (status == ~0)
- return IRQ_NONE;
+ goto out_put;
sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
@@ -775,18 +783,32 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
mask &= ~SSSR_TINT;
if (!(status & mask))
- return IRQ_NONE;
+ goto out_put;
pxa2xx_spi_write(drv_data, SSCR1, sccr1_reg & ~drv_data->int_cr1);
pxa2xx_spi_write(drv_data, SSCR1, sccr1_reg);
if (!drv_data->controller->cur_msg) {
handle_bad_msg(drv_data);
- /* Never fail */
- return IRQ_HANDLED;
+ ret = IRQ_HANDLED;
+ goto out;
}
- return drv_data->transfer_handler(drv_data);
+ ret = drv_data->transfer_handler(drv_data);
+ goto out;
+
+out_put:
+ ret = IRQ_NONE;
+
+out:
+ if (active > 0) {
+ if (ret == IRQ_HANDLED)
+ pm_runtime_put_autosuspend(dev);
+ else
+ pm_runtime_put(dev);
+ }
+
+ return ret;
}
/*
@@ -1356,11 +1378,6 @@ int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
| SSSR_ROR | SSSR_TUR;
}
- ret = request_irq(ssp->irq, ssp_int, IRQF_SHARED, dev_name(dev),
- drv_data);
- if (ret < 0)
- return dev_err_probe(dev, ret, "cannot get IRQ %d\n", ssp->irq);
-
/* Setup DMA if requested */
if (platform_info->enable_dma) {
ret = pxa2xx_spi_dma_setup(drv_data);
@@ -1380,7 +1397,7 @@ int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
/* Enable SOC clock */
ret = pxa2xx_spi_clk_enable(drv_data);
if (ret)
- goto out_error_dma_irq_alloc;
+ goto out_error_dma_alloc;
controller->max_speed_hz = clk_get_rate(ssp->clk);
/*
@@ -1464,22 +1481,31 @@ int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
}
}
+ ret = request_irq(ssp->irq, ssp_int, IRQF_SHARED, dev_name(dev),
+ drv_data);
+ if (ret < 0) {
+ ret = dev_err_probe(dev, ret, "cannot get IRQ %d\n", ssp->irq);
+ goto out_error_clock_enabled;
+ }
+
/* Register with the SPI framework */
dev_set_drvdata(dev, drv_data);
ret = spi_register_controller(controller);
if (ret) {
dev_err_probe(dev, ret, "problem registering SPI controller\n");
- goto out_error_clock_enabled;
+ goto out_error_irq_alloc;
}
return ret;
+out_error_irq_alloc:
+ free_irq(ssp->irq, drv_data);
+
out_error_clock_enabled:
pxa2xx_spi_clk_disable(drv_data);
-out_error_dma_irq_alloc:
+out_error_dma_alloc:
pxa2xx_spi_dma_release(drv_data);
- free_irq(ssp->irq, drv_data);
return ret;
}
--
2.39.5
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v17 3/6] spi: pxa2xx: acquire active PM runtime reference in interrupt handler
2026-09-30 16:06 ` [PATCH v17 3/6] spi: pxa2xx: acquire active PM runtime reference in interrupt handler Shih-Yuan Lee
@ 2026-09-30 17:40 ` Mark Brown
0 siblings, 0 replies; 10+ messages in thread
From: Mark Brown @ 2026-09-30 17:40 UTC (permalink / raw)
To: Shih-Yuan Lee
Cc: Andy Shevchenko, Mika Westerberg, Lukas Wunner, Daniel Mack,
Haojian Zhuang, Robert Jarzmik, linux-spi, linux-kernel,
linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]
On Thu, Oct 01, 2026 at 12:06:26AM +0800, Shih-Yuan Lee wrote:
> On a shared interrupt line, ssp_int() can be invoked while the SPI
> controller is suspended or transitioning power states. In ssp_int(),
> evaluating device power state without acquiring a reference before
> accessing MMIO registers is not atomic: another thread executing
> pm_runtime_suspend() can drop the last reference and gate the clock
> immediately after the check passes. Accessing unclocked registers then
> leads to PCIe Completion Timeouts and system hangs.
> @@ -1464,22 +1481,31 @@ int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
> }
> }
>
> + ret = request_irq(ssp->irq, ssp_int, IRQF_SHARED, dev_name(dev),
> + drv_data);
> + if (ret < 0) {
> + ret = dev_err_probe(dev, ret, "cannot get IRQ %d\n", ssp->irq);
> + goto out_error_clock_enabled;
> + }
> +
> /* Register with the SPI framework */
> dev_set_drvdata(dev, drv_data);
We need the driver data before we request the interrupt, the interrupt
handler does runtime PM and the runtime PM operations use driver data.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v17 4/6] spi: pxa2xx: overhaul teardown and suspend sequence to synchronize IRQ before clock gating
2026-09-30 16:06 [PATCH v17 0/6] spi: pxa2xx: PM fixes, teardown overhaul, and LPSS restore for MacBook8,1 Shih-Yuan Lee
` (2 preceding siblings ...)
2026-09-30 16:06 ` [PATCH v17 3/6] spi: pxa2xx: acquire active PM runtime reference in interrupt handler Shih-Yuan Lee
@ 2026-09-30 16:06 ` Shih-Yuan Lee
2026-09-30 16:06 ` [PATCH v17 5/6] spi: pxa2xx-pci: restore LPSS private register state across S3 resume Shih-Yuan Lee
2026-09-30 16:06 ` [PATCH v17 6/6] spi: pxa2xx-pci: disable DMA and runtime autosuspend for Apple MacBook8,1 Shih-Yuan Lee
5 siblings, 0 replies; 10+ messages in thread
From: Shih-Yuan Lee @ 2026-09-30 16:06 UTC (permalink / raw)
To: Mark Brown
Cc: Andy Shevchenko, Mika Westerberg, Lukas Wunner, Daniel Mack,
Haojian Zhuang, Robert Jarzmik, linux-spi, linux-kernel,
linux-arm-kernel, Shih-Yuan Lee
When removing the driver or suspending the device, the clock must not
be disabled while shared interrupts are still active. Gating the clock
before waiting for in-flight interrupt handlers to complete results
in race conditions where the handler performs unclocked MMIO accesses,
causing PCIe Completion Timeouts.
Overhaul the remove, suspend, and runtime_suspend paths to use a strict
synchronized teardown order:
1. In remove, call free_irq() (which internally synchronizes any in-flight
handlers) before disabling the clock via pxa2xx_spi_clk_disable().
2. In runtime_suspend, under clk_lock and only when the clock is enabled,
disable the SSP peripheral via pxa_ssp_disable(), clear 'clk_enabled'
via WRITE_ONCE() so that new interrupts immediately bail out with
IRQ_NONE, drain in-flight handlers via synchronize_irq() while the
clock is still running, and finally gate the clock with
clk_disable_unprepare().
3. In system suspend, suspend the controller queue and use
pm_runtime_force_suspend() to invoke runtime_suspend, ensuring
in-flight interrupts are drained before the clock is gated. If
pm_runtime_force_suspend() fails, resume the controller queue so
the controller remains operational since the system will stay awake.
4. In system resume, restore the device state using
pm_runtime_force_resume() before restarting the controller queue.
If pm_runtime_force_resume() fails, return the error immediately
without calling spi_controller_resume(), keeping the queue stopped
to prevent transferring messages against unclocked or unpowered
hardware. If the device was already runtime-suspended prior to
system sleep, pm_runtime_force_resume() leaves the clock gated until
the next transfer resumes it, optimizing idle power.
Throughout suspended states, 'clk_enabled' being false serves as the
primary invariant ensuring that any subsequent interrupt handler
invocation safely returns IRQ_NONE without accessing hardware registers.
Assisted-by: Antigravity:gemini-3.8-flash spin sparse
Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
drivers/spi/spi-pxa2xx.c | 39 +++++++++++++++++++++++----------------
1 file changed, 23 insertions(+), 16 deletions(-)
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index b091434977af..2a3fa9ca7213 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1520,31 +1520,33 @@ void pxa2xx_spi_remove(struct device *dev)
/* Disable the SSP at the peripheral and SOC level */
pxa_ssp_disable(ssp);
+
+ /* Release IRQ before gating the SOC clock */
+ free_irq(ssp->irq, drv_data);
+
+ /* Safe to disable the SSP clock now */
pxa2xx_spi_clk_disable(drv_data);
/* Release DMA */
if (drv_data->controller_info->enable_dma)
pxa2xx_spi_dma_release(drv_data);
-
- /* Release IRQ */
- free_irq(ssp->irq, drv_data);
}
EXPORT_SYMBOL_NS_GPL(pxa2xx_spi_remove, "SPI_PXA2xx");
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 ret;
ret = spi_controller_suspend(drv_data->controller);
if (ret)
return ret;
- pxa_ssp_disable(ssp);
-
- if (!pm_runtime_suspended(dev))
- pxa2xx_spi_clk_disable(drv_data);
+ ret = pm_runtime_force_suspend(dev);
+ if (ret) {
+ spi_controller_resume(drv_data->controller);
+ return ret;
+ }
return 0;
}
@@ -1554,14 +1556,10 @@ static int pxa2xx_spi_resume(struct device *dev)
struct driver_data *drv_data = dev_get_drvdata(dev);
int ret;
- /* Enable the SSP clock */
- if (!pm_runtime_suspended(dev)) {
- ret = pxa2xx_spi_clk_enable(drv_data);
- if (ret)
- return ret;
- }
+ ret = pm_runtime_force_resume(dev);
+ if (ret)
+ return ret;
- /* Start the queue running */
return spi_controller_resume(drv_data->controller);
}
@@ -1569,7 +1567,16 @@ static int pxa2xx_spi_runtime_suspend(struct device *dev)
{
struct driver_data *drv_data = dev_get_drvdata(dev);
- pxa2xx_spi_clk_disable(drv_data);
+ mutex_lock(&drv_data->clk_lock);
+ if (drv_data->clk_enabled) {
+ pxa_ssp_disable(drv_data->ssp);
+ WRITE_ONCE(drv_data->clk_enabled, false);
+ mutex_unlock(&drv_data->clk_lock);
+ synchronize_irq(drv_data->ssp->irq);
+ clk_disable_unprepare(drv_data->ssp->clk);
+ } else {
+ mutex_unlock(&drv_data->clk_lock);
+ }
return 0;
}
--
2.39.5
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v17 5/6] spi: pxa2xx-pci: restore LPSS private register state across S3 resume
2026-09-30 16:06 [PATCH v17 0/6] spi: pxa2xx: PM fixes, teardown overhaul, and LPSS restore for MacBook8,1 Shih-Yuan Lee
` (3 preceding siblings ...)
2026-09-30 16:06 ` [PATCH v17 4/6] spi: pxa2xx: overhaul teardown and suspend sequence to synchronize IRQ before clock gating Shih-Yuan Lee
@ 2026-09-30 16:06 ` Shih-Yuan Lee
2026-10-01 4:13 ` Mika Westerberg
2026-09-30 16:06 ` [PATCH v17 6/6] spi: pxa2xx-pci: disable DMA and runtime autosuspend for Apple MacBook8,1 Shih-Yuan Lee
5 siblings, 1 reply; 10+ messages in thread
From: Shih-Yuan Lee @ 2026-09-30 16:06 UTC (permalink / raw)
To: Mark Brown
Cc: Andy Shevchenko, Mika Westerberg, Lukas Wunner, Daniel Mack,
Haojian Zhuang, Robert Jarzmik, linux-spi, linux-kernel,
linux-arm-kernel, Shih-Yuan Lee
On platforms where Intel LPSS SPI is enumerated as a bare PCI device
(such as the Apple MacBook8,1 on Lynxpoint-LP), the LPSS power island
loses power during system sleep and powers up with the controller held
in reset. Because the device lacks ACPI companion objects or MFD
binding, neither drivers/acpi/x86/lpss.c nor drivers/mfd/intel-lpss.c
restore the private register context across S3 resume.
This causes memory-mapped I/O reads on resume to return ~0, resulting
in PCIe Completion Timeouts, missing interrupts, and dead
keyboard/touchpad.
Restore LPSS private registers and deassert functional, APB, and iDMA
resets in the PCI glue layer:
1. In suspend, quiesce the controller queue via spi_controller_suspend()
so no transfers remain in flight and chip select is deasserted.
Then save the LPSS private registers (LPSS_PRIV_REG_COUNT 9 covering
up to offset 0x20 including reg_cs_ctrl at 0x18). Ensure the clock is
active for reading MMIO registers using pm_runtime_resume_and_get(),
balanced by pm_runtime_put_noidle(), before calling
pm_runtime_force_suspend().
2. In resume, restore LPSS private registers and deassert functional,
APB, and iDMA resets via pxa2xx_spi_pci_lpss_restore_ctx() before
calling pm_runtime_force_resume(). Because the controller clock
remains gated (clk_enabled is false) during this restoration, any
interrupt arriving on a shared line bails out early in ssp_int()
without accessing registers on a controller held in reset. Once
resets are deasserted and private registers are restored, call
pm_runtime_force_resume() to enable the clock and mark the device
active, and restart the queue with spi_controller_resume().
3. In probe, call pci_d3cold_disable() for Lynxpoint-LP (is_lpt) to
ensure the PCI device cannot enter D3cold during runtime PM,
guaranteeing that LPSS private registers are retained during S0 idle
periods and only require restoration across S3 system sleep. Set
DPM_FLAG_NO_DIRECT_COMPLETE to prevent PCI subsystem direct-complete
bypassing system sleep callbacks.
4. Scope LPSS context restoration specifically to Lynxpoint-LP
(PCI_DEVICE_ID_INTEL_LPT*) devices to avoid perturbing other LPSS
PCI platforms (BYT, BSW) or non-LPSS platforms.
Export lpss_ssp_setup() and core runtime PM ops so the PCI glue layer
can invoke them around private register restoration.
Assisted-by: Antigravity:gemini-3.8-flash sparse
Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
drivers/spi/spi-pxa2xx-pci.c | 151 ++++++++++++++++++++++++++++++++++-
drivers/spi/spi-pxa2xx.c | 9 ++-
drivers/spi/spi-pxa2xx.h | 4 +
3 files changed, 157 insertions(+), 7 deletions(-)
diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c
index cae77ac18520..f8d71362e840 100644
--- a/drivers/spi/spi-pxa2xx-pci.c
+++ b/drivers/spi/spi-pxa2xx-pci.c
@@ -12,6 +12,7 @@
#include <linux/pci.h>
#include <linux/pm.h>
#include <linux/pm_runtime.h>
+#include <linux/spi/spi.h>
#include <linux/sprintf.h>
#include <linux/string.h>
#include <linux/types.h>
@@ -33,10 +34,22 @@
#define PCI_DEVICE_ID_INTEL_LPT1_0 0x9ce5
#define PCI_DEVICE_ID_INTEL_LPT1_1 0x9ce6
+#define LPSS_PRIV_RESETS 0x04
+#define LPSS_PRIV_RESETS_FUNC BIT(0)
+#define LPSS_PRIV_RESETS_APB BIT(1)
+#define LPSS_PRIV_RESETS_IDMA BIT(2)
+#define LPSS_PRIV_REG_COUNT 9
+
struct pxa_spi_info {
int (*setup)(struct pci_dev *pdev, struct pxa2xx_spi_controller *c);
};
+struct pxa2xx_spi_pci_config {
+ struct pxa2xx_spi_controller pdata;
+ u32 lpss_priv_ctx[LPSS_PRIV_REG_COUNT];
+ bool is_lpt;
+};
+
static struct dw_dma_slave byt_tx_param = { .dst_id = 0 };
static struct dw_dma_slave byt_rx_param = { .src_id = 1 };
@@ -95,6 +108,7 @@ static void lpss_dma_put_device(void *dma_dev)
static int lpss_spi_setup(struct pci_dev *dev, struct pxa2xx_spi_controller *c)
{
+ struct pxa2xx_spi_pci_config *cfg = container_of(c, struct pxa2xx_spi_pci_config, pdata);
struct ssp_device *ssp = &c->ssp;
struct dw_dma_slave *tx, *rx;
struct pci_dev *dma_dev;
@@ -131,6 +145,7 @@ static int lpss_spi_setup(struct pci_dev *dev, struct pxa2xx_spi_controller *c)
ssp->port_id = 0;
c->tx_param = &lpt0_tx_param;
c->rx_param = &lpt0_rx_param;
+ cfg->is_lpt = true;
break;
case PCI_DEVICE_ID_INTEL_LPT0_1:
case PCI_DEVICE_ID_INTEL_LPT1_1:
@@ -138,6 +153,7 @@ static int lpss_spi_setup(struct pci_dev *dev, struct pxa2xx_spi_controller *c)
ssp->port_id = 1;
c->tx_param = &lpt1_tx_param;
c->rx_param = &lpt1_rx_param;
+ cfg->is_lpt = true;
break;
default:
return -ENODEV;
@@ -261,22 +277,143 @@ static const struct pxa_spi_info qrk_info_config = {
.setup = qrk_spi_setup,
};
+static void pxa2xx_spi_pci_lpss_save_ctx(struct device *dev)
+{
+ struct driver_data *drv_data = dev_get_drvdata(dev);
+ struct pxa2xx_spi_pci_config *cfg;
+ unsigned int i;
+
+ if (!drv_data || !drv_data->controller_info)
+ return;
+
+ cfg = container_of(drv_data->controller_info,
+ struct pxa2xx_spi_pci_config, pdata);
+
+ if (cfg->is_lpt && drv_data->lpss_base) {
+ for (i = 0; i < LPSS_PRIV_REG_COUNT; i++)
+ cfg->lpss_priv_ctx[i] = readl(drv_data->lpss_base + i * 4);
+ }
+}
+
+static void pxa2xx_spi_pci_lpss_restore_ctx(struct device *dev)
+{
+ struct driver_data *drv_data = dev_get_drvdata(dev);
+ struct pxa2xx_spi_pci_config *cfg;
+ unsigned int i;
+ u32 resets;
+
+ if (!drv_data || !drv_data->controller_info)
+ return;
+
+ cfg = container_of(drv_data->controller_info,
+ struct pxa2xx_spi_pci_config, pdata);
+
+ if (cfg->is_lpt && drv_data->lpss_base) {
+ /*
+ * Deassert functional, APB, and iDMA resets, preserving any
+ * platform-specific bits in LPSS_PRIV_RESETS.
+ */
+ resets = cfg->lpss_priv_ctx[LPSS_PRIV_RESETS / 4] |
+ LPSS_PRIV_RESETS_FUNC | LPSS_PRIV_RESETS_APB |
+ LPSS_PRIV_RESETS_IDMA;
+ writel(resets, drv_data->lpss_base + LPSS_PRIV_RESETS);
+
+ for (i = 0; i < LPSS_PRIV_REG_COUNT; i++) {
+ if (i == LPSS_PRIV_RESETS / 4)
+ continue;
+ writel(cfg->lpss_priv_ctx[i], drv_data->lpss_base + i * 4);
+ }
+
+ /*
+ * Program chip select to deasserted state before resuming the
+ * controller queue, preventing races with the message pump kthread.
+ */
+ lpss_ssp_setup(drv_data);
+ }
+}
+
+static int pxa2xx_spi_pci_suspend(struct device *dev)
+{
+ struct driver_data *drv_data = dev_get_drvdata(dev);
+ struct pxa2xx_spi_pci_config *cfg;
+ int ret;
+
+ if (!drv_data || !drv_data->controller_info)
+ return 0;
+
+ cfg = container_of(drv_data->controller_info,
+ struct pxa2xx_spi_pci_config, pdata);
+
+ ret = spi_controller_suspend(drv_data->controller);
+ if (ret)
+ return ret;
+
+ if (cfg->is_lpt) {
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret < 0) {
+ spi_controller_resume(drv_data->controller);
+ return ret;
+ }
+
+ pxa2xx_spi_pci_lpss_save_ctx(dev);
+ pm_runtime_put_noidle(dev);
+ }
+
+ ret = pm_runtime_force_suspend(dev);
+ if (ret) {
+ spi_controller_resume(drv_data->controller);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int pxa2xx_spi_pci_resume(struct device *dev)
+{
+ struct driver_data *drv_data = dev_get_drvdata(dev);
+ struct pxa2xx_spi_pci_config *cfg;
+ int ret;
+
+ if (!drv_data || !drv_data->controller_info)
+ return 0;
+
+ cfg = container_of(drv_data->controller_info,
+ struct pxa2xx_spi_pci_config, pdata);
+
+ if (cfg->is_lpt)
+ pxa2xx_spi_pci_lpss_restore_ctx(dev);
+
+ ret = pm_runtime_force_resume(dev);
+ if (ret)
+ return ret;
+
+ return spi_controller_resume(drv_data->controller);
+}
+
+static const struct dev_pm_ops pxa2xx_spi_pci_pm_ops = {
+ SYSTEM_SLEEP_PM_OPS(pxa2xx_spi_pci_suspend, pxa2xx_spi_pci_resume)
+ RUNTIME_PM_OPS(pxa2xx_spi_runtime_suspend, pxa2xx_spi_runtime_resume, NULL)
+};
+
static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
const struct pci_device_id *ent)
{
const struct pxa_spi_info *info;
- int ret;
+ struct pxa2xx_spi_pci_config *cfg;
struct pxa2xx_spi_controller *pdata;
struct ssp_device *ssp;
+ int ret;
ret = pcim_enable_device(dev);
if (ret)
return ret;
- pdata = devm_kzalloc(&dev->dev, sizeof(*pdata), GFP_KERNEL);
- if (!pdata)
+ cfg = devm_kzalloc(&dev->dev, sizeof(*cfg), GFP_KERNEL);
+ if (!cfg)
return -ENOMEM;
+ pdata = &cfg->pdata;
+
ssp = &pdata->ssp;
ssp->dev = &dev->dev;
ssp->phys_base = pci_resource_start(dev, 0);
@@ -300,6 +437,12 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
if (ret)
return ret;
+ if (cfg->is_lpt) {
+ pci_d3cold_disable(dev);
+ dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_NO_DIRECT_COMPLETE);
+ pxa2xx_spi_pci_lpss_save_ctx(&dev->dev);
+ }
+
pm_runtime_set_autosuspend_delay(&dev->dev, 50);
pm_runtime_use_autosuspend(&dev->dev);
pm_runtime_put_autosuspend(&dev->dev);
@@ -336,7 +479,7 @@ static struct pci_driver pxa2xx_spi_pci_driver = {
.name = "pxa2xx_spi_pci",
.id_table = pxa2xx_spi_pci_devices,
.driver = {
- .pm = pm_ptr(&pxa2xx_spi_pm_ops),
+ .pm = pm_ptr(&pxa2xx_spi_pci_pm_ops),
},
.probe = pxa2xx_spi_pci_probe,
.remove = pxa2xx_spi_pci_remove,
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 2a3fa9ca7213..56ddfa67708e 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -343,7 +343,7 @@ static bool __lpss_ssp_update_priv(struct driver_data *drv_data, unsigned int of
* Perform LPSS SSP specific setup. This function must be called first if
* one is going to use LPSS SSP private registers.
*/
-static void lpss_ssp_setup(struct driver_data *drv_data)
+void lpss_ssp_setup(struct driver_data *drv_data)
{
const struct lpss_config *config;
u32 value;
@@ -365,6 +365,7 @@ static void lpss_ssp_setup(struct driver_data *drv_data)
}
}
}
+EXPORT_SYMBOL_NS_GPL(lpss_ssp_setup, "SPI_PXA2xx");
static void lpss_ssp_select_cs(struct spi_device *spi,
const struct lpss_config *config)
@@ -1563,7 +1564,7 @@ static int pxa2xx_spi_resume(struct device *dev)
return spi_controller_resume(drv_data->controller);
}
-static int pxa2xx_spi_runtime_suspend(struct device *dev)
+int pxa2xx_spi_runtime_suspend(struct device *dev)
{
struct driver_data *drv_data = dev_get_drvdata(dev);
@@ -1579,13 +1580,15 @@ static int pxa2xx_spi_runtime_suspend(struct device *dev)
}
return 0;
}
+EXPORT_SYMBOL_NS_GPL(pxa2xx_spi_runtime_suspend, "SPI_PXA2xx");
-static int pxa2xx_spi_runtime_resume(struct device *dev)
+int pxa2xx_spi_runtime_resume(struct device *dev)
{
struct driver_data *drv_data = dev_get_drvdata(dev);
return pxa2xx_spi_clk_enable(drv_data);
}
+EXPORT_SYMBOL_NS_GPL(pxa2xx_spi_runtime_resume, "SPI_PXA2xx");
EXPORT_NS_GPL_DEV_PM_OPS(pxa2xx_spi_pm_ops, SPI_PXA2xx) = {
SYSTEM_SLEEP_PM_OPS(pxa2xx_spi_suspend, pxa2xx_spi_resume)
diff --git a/drivers/spi/spi-pxa2xx.h b/drivers/spi/spi-pxa2xx.h
index 3352239f74a0..a31022256202 100644
--- a/drivers/spi/spi-pxa2xx.h
+++ b/drivers/spi/spi-pxa2xx.h
@@ -138,6 +138,10 @@ int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
struct pxa2xx_spi_controller *platform_info);
void pxa2xx_spi_remove(struct device *dev);
+void lpss_ssp_setup(struct driver_data *drv_data);
+int pxa2xx_spi_runtime_suspend(struct device *dev);
+int pxa2xx_spi_runtime_resume(struct device *dev);
+
extern const struct dev_pm_ops pxa2xx_spi_pm_ops;
#endif /* SPI_PXA2XX_H */
--
2.39.5
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v17 5/6] spi: pxa2xx-pci: restore LPSS private register state across S3 resume
2026-09-30 16:06 ` [PATCH v17 5/6] spi: pxa2xx-pci: restore LPSS private register state across S3 resume Shih-Yuan Lee
@ 2026-10-01 4:13 ` Mika Westerberg
0 siblings, 0 replies; 10+ messages in thread
From: Mika Westerberg @ 2026-10-01 4:13 UTC (permalink / raw)
To: Shih-Yuan Lee
Cc: Mark Brown, Andy Shevchenko, Lukas Wunner, Daniel Mack,
Haojian Zhuang, Robert Jarzmik, linux-spi, linux-kernel,
linux-arm-kernel
Hi,
On Thu, Oct 01, 2026 at 12:06:28AM +0800, Shih-Yuan Lee wrote:
> On platforms where Intel LPSS SPI is enumerated as a bare PCI device
> (such as the Apple MacBook8,1 on Lynxpoint-LP), the LPSS power island
> loses power during system sleep and powers up with the controller held
> in reset. Because the device lacks ACPI companion objects or MFD
> binding, neither drivers/acpi/x86/lpss.c nor drivers/mfd/intel-lpss.c
> restore the private register context across S3 resume.
Hmm, how is that possible? The LPSS is a single PCI device that only the
the LPSS MFD driver binds to. I don't think it is any different in Apple
platforms than any other regular PC platforms in that sense. What does
lspci -vv show on this system?
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v17 6/6] spi: pxa2xx-pci: disable DMA and runtime autosuspend for Apple MacBook8,1
2026-09-30 16:06 [PATCH v17 0/6] spi: pxa2xx: PM fixes, teardown overhaul, and LPSS restore for MacBook8,1 Shih-Yuan Lee
` (4 preceding siblings ...)
2026-09-30 16:06 ` [PATCH v17 5/6] spi: pxa2xx-pci: restore LPSS private register state across S3 resume Shih-Yuan Lee
@ 2026-09-30 16:06 ` Shih-Yuan Lee
5 siblings, 0 replies; 10+ messages in thread
From: Shih-Yuan Lee @ 2026-09-30 16:06 UTC (permalink / raw)
To: Mark Brown
Cc: Andy Shevchenko, Mika Westerberg, Lukas Wunner, Daniel Mack,
Haojian Zhuang, Robert Jarzmik, linux-spi, linux-kernel,
linux-arm-kernel, Shih-Yuan Lee
On MacBook8,1 (early 2015 12" MacBook), the LPSS SPI controller at
00:15.4 (PCI ID 8086:9ce6, PCI_DEVICE_ID_INTEL_LPT1_1) registers and
allocates DMA channels successfully. However, during actual SPI
transfers, the DMA handshake synchronization fails.
This leads to continuous transfer timeouts and locks up the driver
queue, making the connected keyboard and touchpad completely dead:
[ 1582.050494] spi_master spi1: failed to transfer one message from queue
[ 1582.258387] applespi spi-APP000D:00: SPI transfer timed out
[ 1582.258498] applespi spi-APP000D:00: Error reading from device: -110
Empirical hardware register inspection reveals that the companion LPSS
DMA controller at 00:15.0 has its LPSS Private Reset Register (BAR0 +
0x204) left strictly at 0x00000000 by the EFI firmware. Even after
deasserting this reset and overriding ACPI _PRT to assign shared IRQ 21,
the DMA interrupt counter remains strictly at 0, confirming that DMA
completion interrupts are not delivered on this platform. Furthermore,
the SPI controller driver at 00:15.4 cannot safely reach across the PCI
bus to manipulate private registers of the separate DMAC function at
00:15.0 without violating driver layering.
Reverse engineering of the official OS drivers on the same hardware
corroborates that DMA is deliberately bypassed:
1. On macOS (Big Sur), IOKit registry shows that while the LPSS DMAC
driver (AppleIntelLpssDmac) is loaded, the allocated channel count
(AppleIntelLpssDmacChannel) is 0 when the SPI device driver
(AppleHSSPIHIDDriver) is active.
2. On Windows 10 (Boot Camp), dynamic MMIO monitoring of BAR1 shows
that the LPSS DMA control registers (offset 0x800+) remain
inactive (all zeros) during active keyboard and touchpad
transactions, and transfers are handled solely via the FIFO data
registers at the beginning of BAR1.
3. Logic board schematics confirm that a dedicated out-of-band GPIO
interrupt line (TPAD_SPI_INT_L) is routed directly to the PCH for
input events.
Additionally, when operating in PIO mode servicing high-frequency
keyboard and touchpad input streams operating at 60-125 Hz, the default
50 ms autosuspend delay causes the controller to repeatedly transition
between D0 and D3hot between bursts of user input. These runtime resume
transitions introduce latency spikes that drop key presses and cause
erratic touchpad motion.
Implement the forced PIO mode and runtime autosuspend lockout DMI quirk
in spi-pxa2xx-pci.c (the LPSS host controller PCI glue driver) scoped
strictly to MacBook8,1:
1. Include <linux/dmi.h> in alphabetical order for DMI system matching.
2. Add 'quirks' field in struct pxa2xx_spi_pci_config and define
PXA2XX_SPI_QUIRK_NO_AUTOSUSPEND.
3. Add pxa2xx_spi_pci_dmi_table matching Apple MacBook8,1 and query it
once in lpss_spi_setup() scoped to PCI_DEVICE_ID_INTEL_LPT1_1
(0x9ce6). When matched, log the system ident with pci_info(), set
enable_dma = 0 as a workaround, cache PXA2XX_SPI_QUIRK_NO_AUTOSUSPEND
in cfg->quirks, and return immediately to skip unused DMA channel
setup.
4. In pxa2xx_spi_pci_probe(), configure autosuspend with a 50 ms delay
for all devices, but conditionally skip pm_runtime_allow() when
PXA2XX_SPI_QUIRK_NO_AUTOSUSPEND is set. This keeps the controller in
D0 by default (as initialized by PCI core pci_pm_init()) while bound,
eliminating D3hot resume latency spikes, while ensuring that if
userspace explicitly enables autosuspend via sysfs, the safe 50 ms
delay is preserved rather than defaulting to 0. Standard system
sleep (S3) remains fully operational via pm_runtime_force_suspend().
Assisted-by: Antigravity:gemini-3.8-flash sparse
Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
drivers/spi/spi-pxa2xx-pci.c | 61 +++++++++++++++++++++++++++++++++++-
1 file changed, 60 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c
index f8d71362e840..f12a8ef12b17 100644
--- a/drivers/spi/spi-pxa2xx-pci.c
+++ b/drivers/spi/spi-pxa2xx-pci.c
@@ -7,6 +7,7 @@
*/
#include <linux/clk-provider.h>
#include <linux/device.h>
+#include <linux/dmi.h>
#include <linux/err.h>
#include <linux/module.h>
#include <linux/pci.h>
@@ -40,6 +41,8 @@
#define LPSS_PRIV_RESETS_IDMA BIT(2)
#define LPSS_PRIV_REG_COUNT 9
+#define PXA2XX_SPI_QUIRK_NO_AUTOSUSPEND BIT(0)
+
struct pxa_spi_info {
int (*setup)(struct pci_dev *pdev, struct pxa2xx_spi_controller *c);
};
@@ -47,6 +50,7 @@ struct pxa_spi_info {
struct pxa2xx_spi_pci_config {
struct pxa2xx_spi_controller pdata;
u32 lpss_priv_ctx[LPSS_PRIV_REG_COUNT];
+ u8 quirks;
bool is_lpt;
};
@@ -106,8 +110,53 @@ static void lpss_dma_put_device(void *dma_dev)
pci_dev_put(dma_dev);
}
+/*
+ * Apple MacBook8,1 (early 2015 12" MacBook):
+ *
+ * The LPSS SPI controller at 00:15.4 connects to the internal SPI keyboard
+ * and touchpad (applespi). When DMA mode is enabled, DMA transfers consistently
+ * time out (-110) because DMA completion interrupts are never delivered.
+ *
+ * Empirical hardware inspection and OS analysis show that:
+ * 1. Hardware/firmware configuration: EFI leaves companion LPSS DMA (00:15.0)
+ * held in reset (LPSS_PRIV_RESETS == 0x0 / BAR0 + 0x204). Even after
+ * deasserting the reset and overriding ACPI _PRT to assign shared IRQ 21,
+ * the DMA interrupt counter remains strictly 0, confirming that DMA
+ * completion interrupts are not delivered on this platform.
+ * 2. Operating system parity: Both macOS (AppleIntelLpssDmac channel count
+ * is 0) and Windows 10 Boot Camp bypass DMA and handle keyboard/touchpad
+ * transactions strictly in PIO mode via FIFO registers.
+ * 3. Layering: The SPI controller driver (00:15.4) cannot safely reconfigure
+ * the private registers of the separate DMAC PCI function (00:15.0).
+ * 4. Runtime autosuspend: In PIO mode servicing 60-125 Hz input events, the
+ * default 50 ms autosuspend delay introduces D3hot <-> D0 transition
+ * latency spikes that drop key presses and cause erratic touchpad motion.
+ *
+ * Therefore, as a workaround, disable DMA channel allocation to force PIO mode
+ * and keep the device in D0 without enabling runtime autosuspend.
+ */
+static const struct dmi_system_id pxa2xx_spi_pci_dmi_table[] = {
+ {
+ .ident = "Apple MacBook8,1",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."),
+ DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "MacBook8,1"),
+ },
+ },
+ { }
+};
+
+static const struct dmi_system_id *pxa2xx_spi_pci_get_dmi_id(struct pci_dev *dev)
+{
+ if (dev->device == PCI_DEVICE_ID_INTEL_LPT1_1)
+ return dmi_first_match(pxa2xx_spi_pci_dmi_table);
+
+ return NULL;
+}
+
static int lpss_spi_setup(struct pci_dev *dev, struct pxa2xx_spi_controller *c)
{
+ const struct dmi_system_id *dmi_id;
struct pxa2xx_spi_pci_config *cfg = container_of(c, struct pxa2xx_spi_pci_config, pdata);
struct ssp_device *ssp = &c->ssp;
struct dw_dma_slave *tx, *rx;
@@ -165,6 +214,14 @@ static int lpss_spi_setup(struct pci_dev *dev, struct pxa2xx_spi_controller *c)
if (ret)
return ret;
+ dmi_id = pxa2xx_spi_pci_get_dmi_id(dev);
+ if (dmi_id) {
+ pci_info(dev, "%s detected: disabling DMA to force PIO mode\n", dmi_id->ident);
+ c->enable_dma = 0;
+ cfg->quirks |= PXA2XX_SPI_QUIRK_NO_AUTOSUSPEND;
+ return 0;
+ }
+
dma_dev = pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
ret = devm_add_action_or_reset(&dev->dev, lpss_dma_put_device, dma_dev);
if (ret)
@@ -446,7 +503,9 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
pm_runtime_set_autosuspend_delay(&dev->dev, 50);
pm_runtime_use_autosuspend(&dev->dev);
pm_runtime_put_autosuspend(&dev->dev);
- pm_runtime_allow(&dev->dev);
+
+ if (!(cfg->quirks & PXA2XX_SPI_QUIRK_NO_AUTOSUSPEND))
+ pm_runtime_allow(&dev->dev);
return 0;
}
--
2.39.5
^ permalink raw reply [flat|nested] 10+ messages in thread