* [PATCH v9 1/6] dt-bindings: spmi: apple,spmi: Add t603x and t8122
2026-09-01 10:28 [PATCH v9 0/6] spmi: apple: Additional commands and interrupt support Sasha Finkelstein
@ 2026-09-01 10:29 ` Sasha Finkelstein
2026-09-01 10:39 ` sashiko-bot
2026-09-01 17:41 ` Conor Dooley
2026-09-01 10:29 ` [PATCH v9 2/6] spmi: apple: Validate FIFO state Sasha Finkelstein
` (4 subsequent siblings)
5 siblings, 2 replies; 13+ messages in thread
From: Sasha Finkelstein @ 2026-09-01 10:29 UTC (permalink / raw)
To: Sven Peter, Janne Grunau, Neal Gompa, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: asahi, linux-arm-kernel, linux-kernel, devicetree,
Sasha Finkelstein
Add t603x and t8122 compatibles, interrupt support, and support for
SPMI controllers that are not always-on.
Reviewed-by: Janne Grunau <j@jannau.net>
Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
---
Documentation/devicetree/bindings/spmi/apple,spmi.yaml | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/Documentation/devicetree/bindings/spmi/apple,spmi.yaml b/Documentation/devicetree/bindings/spmi/apple,spmi.yaml
index 3e5b14bc8c31..4e73083ecec3 100644
--- a/Documentation/devicetree/bindings/spmi/apple,spmi.yaml
+++ b/Documentation/devicetree/bindings/spmi/apple,spmi.yaml
@@ -20,8 +20,11 @@ properties:
- items:
- enum:
- apple,t6020-spmi
+ - apple,t6030-spmi
+ - apple,t6031-spmi
- apple,t8012-spmi
- apple,t8015-spmi
+ - apple,t8122-spmi
- const: apple,t8103-spmi
- items:
- enum:
@@ -34,10 +37,36 @@ properties:
reg:
maxItems: 1
+ interrupts:
+ maxItems: 1
+ description: Optional, operates in polled mode if not present
+
+ interrupt-controller: true
+
+ "#interrupt-cells":
+ const: 2
+
+ power-domains:
+ maxItems: 1
+
required:
- compatible
- reg
+if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - apple,t8122-spmi
+ - apple,t6030-spmi
+ - apple,t6031-spmi
+then:
+ required:
+ - interrupt-controller
+ - "#interrupt-cells"
+ - interrupts
+
unevaluatedProperties: false
examples:
--
2.55.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v9 1/6] dt-bindings: spmi: apple,spmi: Add t603x and t8122
2026-09-01 10:29 ` [PATCH v9 1/6] dt-bindings: spmi: apple,spmi: Add t603x and t8122 Sasha Finkelstein
@ 2026-09-01 10:39 ` sashiko-bot
2026-09-01 17:41 ` Conor Dooley
1 sibling, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-09-01 10:39 UTC (permalink / raw)
To: Sasha Finkelstein; +Cc: robh, conor+dt, devicetree
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing required `power-domains` property for not-always-on SPMI controllers.
- [Low] Missing example coverage for new interrupt controller capabilities.
--
commit 57c88d4c6dd7d628065665fa42a855aa5ac54ce4
Author: Sasha Finkelstein <k@chaosmail.tech>
dt-bindings: spmi: apple,spmi: Add t603x and t8122
This commit adds the t603x and t8122 compatibles for apple,spmi. It introduces
interrupt support and support for SPMI controllers that are not always-on.
> diff --git a/Documentation/devicetree/bindings/spmi/apple,spmi.yaml b/Documentation/devicetree/bindings/spmi/apple,spmi.yaml
> index 3e5b14bc8c317..4e73083ecec33 100644
> --- a/Documentation/devicetree/bindings/spmi/apple,spmi.yaml
> +++ b/Documentation/devicetree/bindings/spmi/apple,spmi.yaml
[ ... ]
> @@ -34,10 +37,36 @@ properties:
> reg:
> maxItems: 1
>
> + interrupts:
> + maxItems: 1
> + description: Optional, operates in polled mode if not present
> +
> + interrupt-controller: true
> +
> + "#interrupt-cells":
> + const: 2
> +
> + power-domains:
> + maxItems: 1
> +
> required:
> - compatible
> - reg
>
> +if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - apple,t8122-spmi
> + - apple,t6030-spmi
> + - apple,t6031-spmi
> +then:
> + required:
> + - interrupt-controller
> + - "#interrupt-cells"
> + - interrupts
[Severity: Medium]
Looking at this if-then block in Documentation/devicetree/bindings/spmi/apple,spmi.yaml,
does it also need to mandate power-domains?
The commit message mentions adding support for SPMI controllers that are
not always-on (corresponding to the newly introduced t6030, t6031, and t8122).
For not-always-on hardware to be accessible without triggering a synchronous
external abort (kernel crash) on register read/write, the driver core must
power it on prior to the driver's probe.
This mechanism relies on the power-domains property being present in the
device tree node. Although power-domains is added to the properties list,
omitting it from this required list allows incomplete device trees to silently
pass schema validation.
> +
> unevaluatedProperties: false
>
> examples:
[Severity: Low]
Looking at the examples section in Documentation/devicetree/bindings/spmi/apple,spmi.yaml,
does it provide coverage for the new interrupt controller capabilities?
The patch adds interrupt controller capabilities to the new hardware variants
(t6030, t6031, t8122) and makes interrupt-controller and #interrupt-cells
conditionally required. However, the existing examples section only uses the
apple,t6000-spmi compatible, which does not trigger the new if-then block
condition:
spmi@920a1300 {
compatible = "apple,t6000-spmi", "apple,spmi";
Because of this, the newly added conditionally required properties and their
constraints are completely bypassed and remain untested by schema validation
tools.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-t603x-spmi-v9-0-09e702e3b153@chaosmail.tech?part=1
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v9 1/6] dt-bindings: spmi: apple,spmi: Add t603x and t8122
2026-09-01 10:29 ` [PATCH v9 1/6] dt-bindings: spmi: apple,spmi: Add t603x and t8122 Sasha Finkelstein
2026-09-01 10:39 ` sashiko-bot
@ 2026-09-01 17:41 ` Conor Dooley
2026-09-01 18:24 ` Sasha Finkelstein
1 sibling, 1 reply; 13+ messages in thread
From: Conor Dooley @ 2026-09-01 17:41 UTC (permalink / raw)
To: Sasha Finkelstein
Cc: Sven Peter, Janne Grunau, Neal Gompa, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, asahi, linux-arm-kernel,
linux-kernel, devicetree
[-- Attachment #1: Type: text/plain, Size: 1909 bytes --]
On Tue, Sep 01, 2026 at 12:29:00PM +0200, Sasha Finkelstein wrote:
> Add t603x and t8122 compatibles, interrupt support, and support for
> SPMI controllers that are not always-on.
>
> Reviewed-by: Janne Grunau <j@jannau.net>
> Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
> ---
> Documentation/devicetree/bindings/spmi/apple,spmi.yaml | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/spmi/apple,spmi.yaml b/Documentation/devicetree/bindings/spmi/apple,spmi.yaml
> index 3e5b14bc8c31..4e73083ecec3 100644
> --- a/Documentation/devicetree/bindings/spmi/apple,spmi.yaml
> +++ b/Documentation/devicetree/bindings/spmi/apple,spmi.yaml
> @@ -20,8 +20,11 @@ properties:
> - items:
> - enum:
> - apple,t6020-spmi
> + - apple,t6030-spmi
> + - apple,t6031-spmi
> - apple,t8012-spmi
> - apple,t8015-spmi
> + - apple,t8122-spmi
> - const: apple,t8103-spmi
> - items:
> - enum:
> @@ -34,10 +37,36 @@ properties:
> reg:
> maxItems: 1
>
> + interrupts:
> + maxItems: 1
> + description: Optional, operates in polled mode if not present
> +
> + interrupt-controller: true
> +
> + "#interrupt-cells":
> + const: 2
> +
> + power-domains:
> + maxItems: 1
> +
> required:
> - compatible
> - reg
>
> +if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - apple,t8122-spmi
> + - apple,t6030-spmi
> + - apple,t6031-spmi
> +then:
> + required:
> + - interrupt-controller
> + - "#interrupt-cells"
> + - interrupts
else: false?
Or do the existing devices also support these?
> +
> unevaluatedProperties: false
>
> examples:
>
> --
> 2.55.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v9 1/6] dt-bindings: spmi: apple,spmi: Add t603x and t8122
2026-09-01 17:41 ` Conor Dooley
@ 2026-09-01 18:24 ` Sasha Finkelstein
2026-09-02 17:02 ` Conor Dooley
0 siblings, 1 reply; 13+ messages in thread
From: Sasha Finkelstein @ 2026-09-01 18:24 UTC (permalink / raw)
To: Conor Dooley
Cc: Sven Peter, Janne Grunau, Neal Gompa, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, asahi, linux-arm-kernel,
linux-kernel, devicetree
> On Sep 1, 2026, at 19:41, Conor Dooley <conor@kernel.org> wrote:
>> +if:
>> + properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - apple,t8122-spmi
>> + - apple,t6030-spmi
>> + - apple,t6031-spmi
>> +then:
>> + required:
>> + - interrupt-controller
>> + - "#interrupt-cells"
>> + - interrupts
>
> else: false?
> Or do the existing devices also support these?
Yes, they do, we are just not planning to use that.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v9 1/6] dt-bindings: spmi: apple,spmi: Add t603x and t8122
2026-09-01 18:24 ` Sasha Finkelstein
@ 2026-09-02 17:02 ` Conor Dooley
0 siblings, 0 replies; 13+ messages in thread
From: Conor Dooley @ 2026-09-02 17:02 UTC (permalink / raw)
To: Sasha Finkelstein
Cc: Sven Peter, Janne Grunau, Neal Gompa, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, asahi, linux-arm-kernel,
linux-kernel, devicetree
[-- Attachment #1: Type: text/plain, Size: 689 bytes --]
On Tue, Sep 01, 2026 at 08:24:19PM +0200, Sasha Finkelstein wrote:
>
>
> > On Sep 1, 2026, at 19:41, Conor Dooley <conor@kernel.org> wrote:
> >> +if:
> >> + properties:
> >> + compatible:
> >> + contains:
> >> + enum:
> >> + - apple,t8122-spmi
> >> + - apple,t6030-spmi
> >> + - apple,t6031-spmi
> >> +then:
> >> + required:
> >> + - interrupt-controller
> >> + - "#interrupt-cells"
> >> + - interrupts
> >
> > else: false?
> > Or do the existing devices also support these?
>
> Yes, they do, we are just not planning to use that.
Acked-by: Conor Dooley <conor.dooley@microchip.com>
pw-bot: not-applicable
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v9 2/6] spmi: apple: Validate FIFO state
2026-09-01 10:28 [PATCH v9 0/6] spmi: apple: Additional commands and interrupt support Sasha Finkelstein
2026-09-01 10:29 ` [PATCH v9 1/6] dt-bindings: spmi: apple,spmi: Add t603x and t8122 Sasha Finkelstein
@ 2026-09-01 10:29 ` Sasha Finkelstein
2026-09-01 10:29 ` [PATCH v9 3/6] spmi: apple: check transaction status Sasha Finkelstein
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Sasha Finkelstein @ 2026-09-01 10:29 UTC (permalink / raw)
To: Sven Peter, Janne Grunau, Neal Gompa, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: asahi, linux-arm-kernel, linux-kernel, devicetree,
Sasha Finkelstein, Alba Mendez
From: Alba Mendez <me@alba.sh>
Check for data before reading the body of a reply, and check for
end of data afterwards.
Signed-off-by: Alba Mendez <me@alba.sh>
Reviewed-by: Janne Grunau <j@jannau.net>
Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
---
drivers/spmi/spmi-apple-controller.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
index 376cf682c43e..15721cb41d5c 100644
--- a/drivers/spmi/spmi-apple-controller.c
+++ b/drivers/spmi/spmi-apple-controller.c
@@ -21,7 +21,9 @@
#define SPMI_STATUS_REG 0
#define SPMI_CMD_REG 0x4
#define SPMI_RSP_REG 0x8
+#define SPMI_ACT_REG 0xa4
+#define SPMI_ACT_FIFO_FLUSH BIT(0)
#define SPMI_RX_FIFO_EMPTY BIT(24)
#define REG_POLL_INTERVAL_US 10000
@@ -29,6 +31,7 @@
struct apple_spmi {
void __iomem *regs;
+ bool prev_fail;
};
#define poll_reg(spmi, reg, val, cond) \
@@ -49,6 +52,7 @@ static int apple_spmi_wait_rx_not_empty(struct spmi_controller *ctrl)
ret = poll_reg(spmi, SPMI_STATUS_REG, status, !(status & SPMI_RX_FIFO_EMPTY));
if (ret) {
+ spmi->prev_fail = true;
dev_err(&ctrl->dev,
"failed to wait for RX FIFO not empty\n");
return ret;
@@ -67,6 +71,11 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
u8 i;
int ret;
+ if (spmi->prev_fail) {
+ writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_ACT_REG);
+ spmi->prev_fail = false;
+ }
+
writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
ret = apple_spmi_wait_rx_not_empty(ctrl);
@@ -78,6 +87,12 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
/* Read SPMI data reply */
while (len_read < len) {
+ if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY) {
+ spmi->prev_fail = true;
+ dev_err_ratelimited(&ctrl->dev,
+ "FIFO lacks reply data, controller stuck?\n");
+ return -EIO;
+ }
rsp = readl(spmi->regs + SPMI_RSP_REG);
i = 0;
while ((len_read < len) && (i < 4)) {
@@ -86,6 +101,11 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
}
}
+ if (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY)) {
+ dev_warn(&ctrl->dev, "FIFO has extra data\n");
+ spmi->prev_fail = true;
+ }
+
return 0;
}
@@ -97,6 +117,11 @@ static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
size_t i = 0, j;
int ret;
+ if (spmi->prev_fail) {
+ writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_ACT_REG);
+ spmi->prev_fail = false;
+ }
+
writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
while (i < len) {
@@ -115,6 +140,11 @@ static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
/* Discard */
readl(spmi->regs + SPMI_RSP_REG);
+ if (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY)) {
+ dev_warn(&ctrl->dev, "FIFO has extra data\n");
+ spmi->prev_fail = true;
+ }
+
return 0;
}
--
2.55.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v9 3/6] spmi: apple: check transaction status
2026-09-01 10:28 [PATCH v9 0/6] spmi: apple: Additional commands and interrupt support Sasha Finkelstein
2026-09-01 10:29 ` [PATCH v9 1/6] dt-bindings: spmi: apple,spmi: Add t603x and t8122 Sasha Finkelstein
2026-09-01 10:29 ` [PATCH v9 2/6] spmi: apple: Validate FIFO state Sasha Finkelstein
@ 2026-09-01 10:29 ` Sasha Finkelstein
2026-09-01 10:29 ` [PATCH v9 4/6] spmi: apple: Implement remaining commands Sasha Finkelstein
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Sasha Finkelstein @ 2026-09-01 10:29 UTC (permalink / raw)
To: Sven Peter, Janne Grunau, Neal Gompa, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: asahi, linux-arm-kernel, linux-kernel, devicetree,
Sasha Finkelstein, Alba Mendez
From: Alba Mendez <me@alba.sh>
Check for parity errors and missing command ACKs.
Signed-off-by: Alba Mendez <me@alba.sh>
Reviewed-by: Janne Grunau <j@jannau.net>
Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
---
drivers/spmi/spmi-apple-controller.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
index 15721cb41d5c..b1c127cf5f44 100644
--- a/drivers/spmi/spmi-apple-controller.c
+++ b/drivers/spmi/spmi-apple-controller.c
@@ -11,6 +11,8 @@
* spmi-pmic-arb.c Copyright (c) 2021, The Linux Foundation.
*/
+#include <linux/bitfield.h>
+#include <linux/bits.h>
#include <linux/io.h>
#include <linux/iopoll.h>
#include <linux/module.h>
@@ -23,6 +25,12 @@
#define SPMI_RSP_REG 0x8
#define SPMI_ACT_REG 0xa4
+/* SPMI_RSP_REG reply word */
+#define SPMI_REPLY_FRAME_PARITY_STATUS GENMASK(31, 16)
+#define SPMI_REPLY_ACK BIT(15)
+#define SPMI_REPLY_SLAVE_ID GENMASK(14, 8)
+#define SPMI_REPLY_CMD GENMASK(7, 0)
+
#define SPMI_ACT_FIFO_FLUSH BIT(0)
#define SPMI_RX_FIFO_EMPTY BIT(24)
@@ -66,7 +74,7 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
{
struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, saddr, len);
- u32 rsp;
+ u32 reply, rsp;
size_t len_read = 0;
u8 i;
int ret;
@@ -82,8 +90,7 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
if (ret)
return ret;
- /* Discard SPMI reply status */
- readl(spmi->regs + SPMI_RSP_REG);
+ reply = readl(spmi->regs + SPMI_RSP_REG);
/* Read SPMI data reply */
while (len_read < len) {
@@ -106,6 +113,10 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
spmi->prev_fail = true;
}
+ if (~FIELD_GET(SPMI_REPLY_FRAME_PARITY_STATUS, reply) & ((1 << len) - 1)) {
+ dev_err(&ctrl->dev, "some frames failed parity check\n");
+ return -EIO;
+ }
return 0;
}
@@ -114,6 +125,7 @@ static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
{
struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, saddr, len);
+ u32 reply;
size_t i = 0, j;
int ret;
@@ -137,14 +149,17 @@ static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
if (ret)
return ret;
- /* Discard */
- readl(spmi->regs + SPMI_RSP_REG);
+ reply = readl(spmi->regs + SPMI_RSP_REG);
if (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY)) {
dev_warn(&ctrl->dev, "FIFO has extra data\n");
spmi->prev_fail = true;
}
+ if (!FIELD_GET(SPMI_REPLY_ACK, reply)) {
+ dev_err(&ctrl->dev, "command not acknowledged\n");
+ return -EIO;
+ }
return 0;
}
--
2.55.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v9 4/6] spmi: apple: Implement remaining commands
2026-09-01 10:28 [PATCH v9 0/6] spmi: apple: Additional commands and interrupt support Sasha Finkelstein
` (2 preceding siblings ...)
2026-09-01 10:29 ` [PATCH v9 3/6] spmi: apple: check transaction status Sasha Finkelstein
@ 2026-09-01 10:29 ` Sasha Finkelstein
2026-09-01 10:40 ` sashiko-bot
2026-09-01 10:29 ` [PATCH v9 5/6] spmi: apple: lock around FIFOs Sasha Finkelstein
2026-09-01 10:29 ` [PATCH v9 6/6] spmi: apple: Add interrupt functionality Sasha Finkelstein
5 siblings, 1 reply; 13+ messages in thread
From: Sasha Finkelstein @ 2026-09-01 10:29 UTC (permalink / raw)
To: Sven Peter, Janne Grunau, Neal Gompa, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: asahi, linux-arm-kernel, linux-kernel, devicetree,
Sasha Finkelstein, Alba Mendez
From: Alba Mendez <me@alba.sh>
Add support for zero write and power management commands.
Signed-off-by: Alba Mendez <me@alba.sh>
Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
---
drivers/spmi/spmi-apple-controller.c | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------
1 file changed, 69 insertions(+), 47 deletions(-)
diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
index b1c127cf5f44..9843dc871d6c 100644
--- a/drivers/spmi/spmi-apple-controller.c
+++ b/drivers/spmi/spmi-apple-controller.c
@@ -46,9 +46,9 @@ struct apple_spmi {
readl_poll_timeout((spmi)->regs + (reg), (val), (cond), \
REG_POLL_INTERVAL_US, REG_POLL_TIMEOUT_US)
-static inline u32 apple_spmi_pack_cmd(u8 opc, u8 sid, u16 saddr, size_t len)
+static inline u32 apple_spmi_pack_cmd(u8 opc, u8 sid, u16 param)
{
- return opc | sid << 8 | saddr << 16 | (len - 1) | (1 << 15);
+ return opc | sid << 8 | (u32)param << 16 | (1 << 15);
}
/* Wait for Rx FIFO to have something */
@@ -69,14 +69,13 @@ static int apple_spmi_wait_rx_not_empty(struct spmi_controller *ctrl)
return 0;
}
-static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
- u16 saddr, u8 *buf, size_t len)
+static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid, u16 param,
+ const u8 *buf_wr, size_t len_wr, u8 *buf_rd, size_t len_rd)
{
struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
- u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, saddr, len);
+ u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, param);
u32 reply, rsp;
- size_t len_read = 0;
- u8 i;
+ size_t i = 0, j;
int ret;
if (spmi->prev_fail) {
@@ -86,6 +85,14 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
+ while (i < len_wr) {
+ j = min_t(size_t, sizeof(spmi_cmd), len_wr - i);
+ spmi_cmd = 0;
+ memcpy(&spmi_cmd, buf_wr + i, j);
+ writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
+ i += j;
+ }
+
ret = apple_spmi_wait_rx_not_empty(ctrl);
if (ret)
return ret;
@@ -93,7 +100,8 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
reply = readl(spmi->regs + SPMI_RSP_REG);
/* Read SPMI data reply */
- while (len_read < len) {
+ i = 0;
+ while (i < len_rd) {
if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY) {
spmi->prev_fail = true;
dev_err_ratelimited(&ctrl->dev,
@@ -101,11 +109,9 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
return -EIO;
}
rsp = readl(spmi->regs + SPMI_RSP_REG);
- i = 0;
- while ((len_read < len) && (i < 4)) {
- buf[len_read++] = ((0xff << (8 * i)) & rsp) >> (8 * i);
- i += 1;
- }
+ j = min_t(size_t, sizeof(spmi_cmd), len_rd - i);
+ memcpy(buf_rd + i, &rsp, j);
+ i += j;
}
if (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY)) {
@@ -113,54 +119,69 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
spmi->prev_fail = true;
}
- if (~FIELD_GET(SPMI_REPLY_FRAME_PARITY_STATUS, reply) & ((1 << len) - 1)) {
+ if (!len_rd && !FIELD_GET(SPMI_REPLY_ACK, reply)) {
+ dev_err(&ctrl->dev, "command not acknowledged\n");
+ return -EIO;
+ }
+ if (~FIELD_GET(SPMI_REPLY_FRAME_PARITY_STATUS, reply) & ((1 << len_rd) - 1)) {
dev_err(&ctrl->dev, "some frames failed parity check\n");
return -EIO;
}
return 0;
}
-static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
- u16 saddr, const u8 *buf, size_t len)
+/* Send a raw command with 1..16 input data frames */
+static int spmi_raw_cmd_input(struct spmi_controller *ctrl, u8 opc, u8 sid,
+ u16 param, u8 *buf, size_t len)
{
- struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
- u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, saddr, len);
- u32 reply;
- size_t i = 0, j;
- int ret;
-
- if (spmi->prev_fail) {
- writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_ACT_REG);
- spmi->prev_fail = false;
- }
-
- writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
+ return spmi_raw_cmd(ctrl, opc, sid, param, NULL, 0, buf, len);
+}
- while (i < len) {
- j = 0;
- spmi_cmd = 0;
- while ((j < 4) & (i < len))
- spmi_cmd |= buf[i++] << (j++ * 8);
+/* Send a raw command with (optional) body and an input ACK */
+static int spmi_raw_cmd_ack(struct spmi_controller *ctrl, u8 opc, u8 sid,
+ u16 param, const u8 *buf, size_t len)
+{
+ return spmi_raw_cmd(ctrl, opc, sid, param, buf, len, NULL, 0);
+}
- writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
+static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
+ u16 saddr, u8 *buf, size_t len)
+{
+ switch (opc) {
+ case SPMI_CMD_EXT_READ:
+ case SPMI_CMD_EXT_READL:
+ return spmi_raw_cmd_input(ctrl, opc | (len - 1), sid, saddr, buf, len);
+ case SPMI_CMD_READ:
+ return spmi_raw_cmd_input(ctrl, opc | saddr, sid, saddr, buf, len);
}
+ return -EINVAL;
+}
- ret = apple_spmi_wait_rx_not_empty(ctrl);
- if (ret)
- return ret;
-
- reply = readl(spmi->regs + SPMI_RSP_REG);
-
- if (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY)) {
- dev_warn(&ctrl->dev, "FIFO has extra data\n");
- spmi->prev_fail = true;
+static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
+ u16 saddr, const u8 *buf, size_t len)
+{
+ switch (opc) {
+ case SPMI_CMD_WRITE:
+ return spmi_raw_cmd_ack(ctrl, opc | saddr, sid, buf[0] << 8 | saddr, NULL, 0);
+ case SPMI_CMD_ZERO_WRITE:
+ return spmi_raw_cmd_ack(ctrl, opc | buf[0], sid, buf[0] << 8 | saddr, NULL, 0);
+ case SPMI_CMD_EXT_WRITE:
+ case SPMI_CMD_EXT_WRITEL:
+ return spmi_raw_cmd_ack(ctrl, opc | (len - 1), sid, saddr, buf, len);
}
+ return -EINVAL;
+}
- if (!FIELD_GET(SPMI_REPLY_ACK, reply)) {
- dev_err(&ctrl->dev, "command not acknowledged\n");
- return -EIO;
+static int spmi_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid)
+{
+ switch (opc) {
+ case SPMI_CMD_RESET:
+ case SPMI_CMD_SLEEP:
+ case SPMI_CMD_SHUTDOWN:
+ case SPMI_CMD_WAKEUP:
+ return spmi_raw_cmd_ack(ctrl, opc, sid, 0, NULL, 0);
}
- return 0;
+ return -EINVAL;
}
static int apple_spmi_probe(struct platform_device *pdev)
@@ -183,6 +204,7 @@ static int apple_spmi_probe(struct platform_device *pdev)
ctrl->read_cmd = spmi_read_cmd;
ctrl->write_cmd = spmi_write_cmd;
+ ctrl->cmd = spmi_cmd;
ret = devm_spmi_controller_add(&pdev->dev, ctrl);
if (ret)
--
2.55.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v9 4/6] spmi: apple: Implement remaining commands
2026-09-01 10:29 ` [PATCH v9 4/6] spmi: apple: Implement remaining commands Sasha Finkelstein
@ 2026-09-01 10:40 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-09-01 10:40 UTC (permalink / raw)
To: Sasha Finkelstein; +Cc: robh, devicetree, conor+dt
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Endianness bug in `spmi_raw_cmd` due to improper mixing of `memcpy()` and `writel()`/`readl()` for FIFO stream accesses.
--
commit cb6fc258a7a80697550d0e3d62f761cda4b89087
Author: Alba Mendez <me@alba.sh>
spmi: apple: Implement remaining commands
Add support for zero write and power management commands.
> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index b1c127cf5f441..9843dc871d6c7 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c
[ ... ]
> @@ -86,6 +85,14 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>
> writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
>
> + while (i < len_wr) {
> + j = min_t(size_t, sizeof(spmi_cmd), len_wr - i);
> + spmi_cmd = 0;
> + memcpy(&spmi_cmd, buf_wr + i, j);
> + writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
[Severity: Medium]
Will using memcpy() to write into a u32 followed by writel() cause an
endianness regression on big-endian systems?
On big-endian systems, memcpy() places the first byte at the most
significant byte of the u32. writel() then applies a byte-swap, placing
that first byte at the highest bus address (byte 3) instead of byte 0,
which would corrupt the data stream going into the FIFO.
The pre-patch code avoided this by using bitwise shifts to pack the u32
in a CPU-endian-independent manner. Mixing memcpy() and writel() violates
subsystem rules for FIFO I/O accessors.
> + i += j;
> + }
> +
> ret = apple_spmi_wait_rx_not_empty(ctrl);
> if (ret)
> return ret;
[ ... ]
> @@ -101,11 +109,9 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
> return -EIO;
> }
> rsp = readl(spmi->regs + SPMI_RSP_REG);
> - i = 0;
> - while ((len_read < len) && (i < 4)) {
> - buf[len_read++] = ((0xff << (8 * i)) & rsp) >> (8 * i);
> - i += 1;
> - }
> + j = min_t(size_t, sizeof(spmi_cmd), len_rd - i);
> + memcpy(buf_rd + i, &rsp, j);
[Severity: Medium]
Can this readl() followed by memcpy() cause the inverse data corruption on
the read path?
Like the write path, readl() performs a byte swap on big-endian systems,
meaning the memcpy() will extract the stream bytes in reverse order,
corrupting the responses received from the FIFO.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-t603x-spmi-v9-0-09e702e3b153@chaosmail.tech?part=4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v9 5/6] spmi: apple: lock around FIFOs
2026-09-01 10:28 [PATCH v9 0/6] spmi: apple: Additional commands and interrupt support Sasha Finkelstein
` (3 preceding siblings ...)
2026-09-01 10:29 ` [PATCH v9 4/6] spmi: apple: Implement remaining commands Sasha Finkelstein
@ 2026-09-01 10:29 ` Sasha Finkelstein
2026-09-01 10:29 ` [PATCH v9 6/6] spmi: apple: Add interrupt functionality Sasha Finkelstein
5 siblings, 0 replies; 13+ messages in thread
From: Sasha Finkelstein @ 2026-09-01 10:29 UTC (permalink / raw)
To: Sven Peter, Janne Grunau, Neal Gompa, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: asahi, linux-arm-kernel, linux-kernel, devicetree,
Sasha Finkelstein, Alba Mendez
From: Alba Mendez <me@alba.sh>
The driver was missing locking around register interactions.
Signed-off-by: Alba Mendez <me@alba.sh>
Reviewed-by: Janne Grunau <j@jannau.net>
Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
---
drivers/spmi/spmi-apple-controller.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
index 9843dc871d6c..fabccd25aa0d 100644
--- a/drivers/spmi/spmi-apple-controller.c
+++ b/drivers/spmi/spmi-apple-controller.c
@@ -16,6 +16,7 @@
#include <linux/io.h>
#include <linux/iopoll.h>
#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/platform_device.h>
#include <linux/spmi.h>
@@ -39,6 +40,7 @@
struct apple_spmi {
void __iomem *regs;
+ struct mutex fifo_lock;
bool prev_fail;
};
@@ -78,6 +80,8 @@ static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid, u16 param,
size_t i = 0, j;
int ret;
+ guard(mutex)(&spmi->fifo_lock);
+
if (spmi->prev_fail) {
writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_ACT_REG);
spmi->prev_fail = false;
@@ -195,6 +199,7 @@ static int apple_spmi_probe(struct platform_device *pdev)
return -ENOMEM;
spmi = spmi_controller_get_drvdata(ctrl);
+ mutex_init(&spmi->fifo_lock);
spmi->regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(spmi->regs))
--
2.55.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v9 6/6] spmi: apple: Add interrupt functionality
2026-09-01 10:28 [PATCH v9 0/6] spmi: apple: Additional commands and interrupt support Sasha Finkelstein
` (4 preceding siblings ...)
2026-09-01 10:29 ` [PATCH v9 5/6] spmi: apple: lock around FIFOs Sasha Finkelstein
@ 2026-09-01 10:29 ` Sasha Finkelstein
2026-09-01 10:40 ` sashiko-bot
5 siblings, 1 reply; 13+ messages in thread
From: Sasha Finkelstein @ 2026-09-01 10:29 UTC (permalink / raw)
To: Sven Peter, Janne Grunau, Neal Gompa, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: asahi, linux-arm-kernel, linux-kernel, devicetree,
Sasha Finkelstein, Alba Mendez
From: Alba Mendez <me@alba.sh>
Add support for interrupts sent by slave devices and use IRQ for
RX FIFO if possible, as that IRQ fires as soon as the reply is
available, which is usually takes a few us instead of the 10ms sleep
interval for polling.
Signed-off-by: Alba Mendez <me@alba.sh>
Reviewed-by: Janne Grunau <j@jannau.net>
Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
---
drivers/spmi/Kconfig | 3 +-
drivers/spmi/spmi-apple-controller.c | 257 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 258 insertions(+), 2 deletions(-)
diff --git a/drivers/spmi/Kconfig b/drivers/spmi/Kconfig
index a80cf4047b86..7243863a09b4 100644
--- a/drivers/spmi/Kconfig
+++ b/drivers/spmi/Kconfig
@@ -13,7 +13,8 @@ if SPMI
config SPMI_APPLE
tristate "Apple SoC SPMI Controller platform driver"
- depends on ARCH_APPLE || COMPILE_TEST
+ select IRQ_DOMAIN_HIERARCHY
+ depends on ARCH_APPLE || (COMPILE_TEST && 64BIT)
help
If you say yes to this option, support will be included for the
SPMI controller present on many Apple SoCs, including the
diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
index fabccd25aa0d..2390174452ab 100644
--- a/drivers/spmi/spmi-apple-controller.c
+++ b/drivers/spmi/spmi-apple-controller.c
@@ -13,11 +13,17 @@
#include <linux/bitfield.h>
#include <linux/bits.h>
+#include <linux/completion.h>
+#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/iopoll.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/platform_device.h>
+#include <linux/spinlock.h>
#include <linux/spmi.h>
/* SPMI Controller Registers */
@@ -26,6 +32,13 @@
#define SPMI_RSP_REG 0x8
#define SPMI_ACT_REG 0xa4
+#define SPMI_IRQ_MASK_BASE 0x20
+#define SPMI_IRQ_ACK_BASE 0x60
+#define SPMI_NUM_PERIPHERAL_IRQS 256
+#define SPMI_NUM_IRQS (SPMI_NUM_PERIPHERAL_IRQS + 32)
+
+#define SPMI_IRQ_NOTIFY 256
+
/* SPMI_RSP_REG reply word */
#define SPMI_REPLY_FRAME_PARITY_STATUS GENMASK(31, 16)
#define SPMI_REPLY_ACK BIT(15)
@@ -41,6 +54,12 @@
struct apple_spmi {
void __iomem *regs;
struct mutex fifo_lock;
+ struct completion fifo_rx;
+ struct irq_domain *irqd;
+ raw_spinlock_t irq_mask_lock;
+ DECLARE_BITMAP(irq_mask_cache, SPMI_NUM_PERIPHERAL_IRQS);
+ int irq;
+ bool notify_irq;
bool prev_fail;
};
@@ -48,6 +67,56 @@ struct apple_spmi {
readl_poll_timeout((spmi)->regs + (reg), (val), (cond), \
REG_POLL_INTERVAL_US, REG_POLL_TIMEOUT_US)
+static void apple_spmi_irq_ack_raw(struct apple_spmi *spmi, u32 irq)
+{
+ u32 __iomem *reg = spmi->regs + SPMI_IRQ_ACK_BASE + (irq / 32) * 4;
+
+ writel(BIT(irq % 32), reg);
+}
+
+static void apple_spmi_irq_mask_raw(struct apple_spmi *spmi, u32 irq)
+{
+ u32 __iomem *reg = spmi->regs + SPMI_IRQ_MASK_BASE + (irq / 32) * 4;
+
+ writel(readl(reg) & ~BIT(irq % 32), reg);
+}
+
+static void apple_spmi_irq_unmask_raw(struct apple_spmi *spmi, u32 irq)
+{
+ u32 __iomem *reg = spmi->regs + SPMI_IRQ_MASK_BASE + (irq / 32) * 4;
+
+ writel(readl(reg) | BIT(irq % 32), reg);
+}
+
+static void apple_spmi_irq_ack(struct irq_data *d)
+{
+ struct apple_spmi *spmi = irq_data_get_irq_chip_data(d);
+
+ apple_spmi_irq_ack_raw(spmi, d->hwirq);
+}
+
+static void apple_spmi_irq_mask(struct irq_data *d)
+{
+ struct apple_spmi *spmi = irq_data_get_irq_chip_data(d);
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&spmi->irq_mask_lock, flags);
+ apple_spmi_irq_mask_raw(spmi, d->hwirq);
+ clear_bit(d->hwirq, spmi->irq_mask_cache);
+ raw_spin_unlock_irqrestore(&spmi->irq_mask_lock, flags);
+}
+
+static void apple_spmi_irq_unmask(struct irq_data *d)
+{
+ struct apple_spmi *spmi = irq_data_get_irq_chip_data(d);
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&spmi->irq_mask_lock, flags);
+ set_bit(d->hwirq, spmi->irq_mask_cache);
+ apple_spmi_irq_unmask_raw(spmi, d->hwirq);
+ raw_spin_unlock_irqrestore(&spmi->irq_mask_lock, flags);
+}
+
static inline u32 apple_spmi_pack_cmd(u8 opc, u8 sid, u16 param)
{
return opc | sid << 8 | (u32)param << 16 | (1 << 15);
@@ -60,7 +129,19 @@ static int apple_spmi_wait_rx_not_empty(struct spmi_controller *ctrl)
int ret;
u32 status;
- ret = poll_reg(spmi, SPMI_STATUS_REG, status, !(status & SPMI_RX_FIFO_EMPTY));
+ if (spmi->notify_irq) {
+ ret = wait_for_completion_timeout(&spmi->fifo_rx,
+ usecs_to_jiffies(REG_POLL_TIMEOUT_US));
+ if (!ret)
+ ret = -ETIMEDOUT;
+ else if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY)
+ ret = -EIO;
+ else
+ ret = 0;
+ } else {
+ ret = poll_reg(spmi, SPMI_STATUS_REG, status, !(status & SPMI_RX_FIFO_EMPTY));
+ }
+
if (ret) {
spmi->prev_fail = true;
dev_err(&ctrl->dev,
@@ -84,8 +165,10 @@ static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid, u16 param,
if (spmi->prev_fail) {
writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_ACT_REG);
+ apple_spmi_irq_ack_raw(spmi, SPMI_IRQ_NOTIFY);
spmi->prev_fail = false;
}
+ reinit_completion(&spmi->fifo_rx);
writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
@@ -188,6 +271,167 @@ static int spmi_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid)
return -EINVAL;
}
+static int apple_spmi_irq_set_type(struct irq_data *d, unsigned int type)
+{
+ /* all interrupts have MSI semantics */
+ return type == IRQ_TYPE_EDGE_RISING ? 0 : -EINVAL;
+}
+
+static struct irq_chip apple_spmi_irq_chip = {
+ .name = "apple_spmi",
+ .irq_mask = apple_spmi_irq_mask,
+ .irq_unmask = apple_spmi_irq_unmask,
+ .irq_ack = apple_spmi_irq_ack,
+ .irq_set_type = apple_spmi_irq_set_type,
+ .flags = IRQCHIP_ONESHOT_SAFE,
+};
+
+static int apple_spmi_irq_domain_map(struct irq_domain *irqd,
+ unsigned int irq, irq_hw_number_t hw)
+{
+ irq_domain_set_info(irqd, irq, hw, &apple_spmi_irq_chip, irqd->host_data,
+ handle_edge_irq, NULL, NULL);
+ return 0;
+}
+
+static int apple_spmi_irq_domain_translate(struct irq_domain *irqd,
+ struct irq_fwspec *fwspec,
+ unsigned long *hwirq,
+ unsigned int *type)
+{
+ u32 *args = fwspec->param;
+
+ if (fwspec->param_count != 2)
+ return -EINVAL;
+
+ if (args[0] >= SPMI_NUM_PERIPHERAL_IRQS)
+ return -EINVAL;
+ *hwirq = args[0];
+ *type = args[1] & IRQ_TYPE_SENSE_MASK;
+ return 0;
+}
+
+static int apple_spmi_irq_domain_alloc(struct irq_domain *irqd, unsigned int virq,
+ unsigned int nr_irqs, void *arg)
+{
+ unsigned int type = IRQ_TYPE_NONE;
+ struct irq_fwspec *fwspec = arg;
+ irq_hw_number_t hwirq;
+ int i, ret;
+
+ ret = apple_spmi_irq_domain_translate(irqd, fwspec, &hwirq, &type);
+ if (ret)
+ return ret;
+
+ if (hwirq + nr_irqs > SPMI_NUM_PERIPHERAL_IRQS)
+ return -EINVAL;
+
+ for (i = 0; i < nr_irqs; i++) {
+ ret = apple_spmi_irq_domain_map(irqd, virq + i, hwirq + i);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static void apple_spmi_irq_domain_free(struct irq_domain *irqd, unsigned int virq,
+ unsigned int nr_irqs)
+{
+ int i;
+
+ for (i = 0; i < nr_irqs; i++) {
+ struct irq_data *d = irq_domain_get_irq_data(irqd, virq + i);
+
+ irq_set_handler(virq + i, NULL);
+ irq_domain_reset_irq_data(d);
+ }
+}
+
+static const struct irq_domain_ops apple_spmi_irq_domain_ops = {
+ .translate = apple_spmi_irq_domain_translate,
+ .alloc = apple_spmi_irq_domain_alloc,
+ .free = apple_spmi_irq_domain_free,
+};
+
+static void apple_spmi_irq_handler(struct irq_desc *desc)
+{
+ struct apple_spmi *spmi = irq_desc_get_handler_data(desc);
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ bool handled = false;
+ unsigned long val, offset, bit;
+
+ chained_irq_enter(chip, desc);
+ val = readl(spmi->regs + SPMI_IRQ_ACK_BASE + (SPMI_IRQ_NOTIFY / 32) * 4);
+ if (val & BIT(SPMI_IRQ_NOTIFY % 32)) {
+ apple_spmi_irq_ack_raw(spmi, SPMI_IRQ_NOTIFY);
+ complete(&spmi->fifo_rx);
+ handled = true;
+ }
+
+ for (offset = 0; offset < SPMI_NUM_PERIPHERAL_IRQS / 8; offset += sizeof(val)) {
+ val = readq(spmi->regs + SPMI_IRQ_ACK_BASE + offset);
+ /**
+ * because of other masters in the bus, we're going to get a multitude of
+ * interrupts we're not interested in. irq_resolve_mapping isn't very
+ * optimized for the nonexistent path, so instead we mask with (a locally
+ * cached version of) the IRQ mask
+ */
+ val &= spmi->irq_mask_cache[offset / sizeof(val)];
+ for_each_set_bit(bit, &val, 64) {
+ generic_handle_domain_irq(spmi->irqd, offset * 8 + bit);
+ handled = true;
+ }
+ }
+ if (!handled)
+ handle_bad_irq(desc);
+ chained_irq_exit(chip, desc);
+}
+
+static void apple_spmi_teardown_irq(void *data)
+{
+ struct apple_spmi *spmi = data;
+
+ for (size_t offset = 0; offset < SPMI_NUM_IRQS / 8; offset += 4)
+ writel(0, spmi->regs + SPMI_IRQ_MASK_BASE + offset);
+
+ synchronize_irq(spmi->irq);
+ irq_set_chained_handler_and_data(spmi->irq, NULL, NULL);
+}
+
+static int apple_spmi_init_irq(struct platform_device *pdev,
+ struct apple_spmi *spmi, int irq)
+{
+ int ret;
+ struct irq_domain_info info = {
+ .fwnode = pdev->dev.fwnode,
+ .hwirq_max = ~0U,
+ .ops = &apple_spmi_irq_domain_ops,
+ .host_data = spmi,
+ };
+
+ raw_spin_lock_init(&spmi->irq_mask_lock);
+
+ for (size_t offset = 0; offset < SPMI_NUM_IRQS / 8; offset += 4) {
+ writel(0, spmi->regs + SPMI_IRQ_MASK_BASE + offset);
+ writel(U32_MAX, spmi->regs + SPMI_IRQ_ACK_BASE + offset);
+ }
+
+ spmi->irqd = devm_irq_domain_instantiate(&pdev->dev, &info);
+ if (IS_ERR(spmi->irqd))
+ return PTR_ERR(spmi->irqd);
+
+ ret = devm_add_action(&pdev->dev, apple_spmi_teardown_irq, spmi);
+ if (ret)
+ return ret;
+
+ irq_set_chained_handler_and_data(spmi->irq, apple_spmi_irq_handler, spmi);
+ apple_spmi_irq_unmask_raw(spmi, SPMI_IRQ_NOTIFY);
+ spmi->notify_irq = true;
+
+ return 0;
+}
+
static int apple_spmi_probe(struct platform_device *pdev)
{
struct apple_spmi *spmi;
@@ -200,6 +444,8 @@ static int apple_spmi_probe(struct platform_device *pdev)
spmi = spmi_controller_get_drvdata(ctrl);
mutex_init(&spmi->fifo_lock);
+ init_completion(&spmi->fifo_rx);
+ platform_set_drvdata(pdev, spmi);
spmi->regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(spmi->regs))
@@ -211,6 +457,15 @@ static int apple_spmi_probe(struct platform_device *pdev)
ctrl->write_cmd = spmi_write_cmd;
ctrl->cmd = spmi_cmd;
+ spmi->irq = platform_get_irq_optional(pdev, 0);
+ if (spmi->irq < 0 && spmi->irq != -ENXIO)
+ return spmi->irq;
+ if (spmi->irq >= 0) {
+ ret = apple_spmi_init_irq(pdev, spmi, spmi->irq);
+ if (ret)
+ return ret;
+ }
+
ret = devm_spmi_controller_add(&pdev->dev, ctrl);
if (ret)
return dev_err_probe(&pdev->dev, ret,
--
2.55.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v9 6/6] spmi: apple: Add interrupt functionality
2026-09-01 10:29 ` [PATCH v9 6/6] spmi: apple: Add interrupt functionality Sasha Finkelstein
@ 2026-09-01 10:40 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-09-01 10:40 UTC (permalink / raw)
To: Sasha Finkelstein; +Cc: robh, devicetree, conor+dt
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Race between command timeout recovery and delayed interrupt handler causes spurious command failures (-EIO).
--
commit b173ce6913a5026fc32908944fd6149c07832b2c
Author: Alba Mendez <me@alba.sh>
spmi: apple: Add interrupt functionality
Add support for interrupts sent by slave devices and use IRQ for
RX FIFO if possible, reducing latency compared to polling.
> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index fabccd25aa0da..2390174452ab4 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c
[ ... ]
> @@ -84,8 +165,10 @@ static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid, u16 param,
>
> if (spmi->prev_fail) {
> writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_ACT_REG);
> + apple_spmi_irq_ack_raw(spmi, SPMI_IRQ_NOTIFY);
> spmi->prev_fail = false;
> }
> + reinit_completion(&spmi->fifo_rx);
>
> writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
[Severity: High]
Could this sequence lead to a race condition with a delayed interrupt?
If a previous command times out, setting spmi->prev_fail = true, but the
hardware responds shortly after, the NOTIFY interrupt might be raised.
If spmi_raw_cmd() is then called for the next command, it runs concurrently
with apple_spmi_irq_handler() on another CPU. The handler reads the NOTIFY
bit and commits to calling complete():
apple_spmi_irq_handler() {
...
if (val & BIT(SPMI_IRQ_NOTIFY % 32)) {
apple_spmi_irq_ack_raw(spmi, SPMI_IRQ_NOTIFY);
complete(&spmi->fifo_rx);
handled = true;
}
}
Concurrently, the thread clears the hardware IRQ state and calls
reinit_completion(&spmi->fifo_rx) here without synchronizing against the
IRQ handler (as the handler does not take fifo_lock).
If complete() executes after or concurrently with reinit_completion(), it
leaves fifo_rx signaled. The new command's wait_for_completion_timeout()
will return immediately, see an empty FIFO, and fail with -EIO.
Additionally, reinit_completion() performs a non-atomic update which races
with the spinlock operations inside complete().
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-t603x-spmi-v9-0-09e702e3b153@chaosmail.tech?part=6
^ permalink raw reply [flat|nested] 13+ messages in thread