[RFC PATCH 0/3] watchdog: arm_smc_wdt: allow a list of SMC function ids

[RFC PATCH 0/3] watchdog: arm_smc_wdt: allow a list of SMC function ids

[Mateusz Nowicki]

Hi all,

this follows up on Marek's report about the stm32mp2 watchdog [1].

A quick recap: the arm,smc-wdt driver talks to the firmware through one
SMC function id that it reads from the device tree, and there is no way
to ask the firmware which id it uses. On the STM32MP2 SoCs that id
changed over time. ST's OP-TEE from ecosystem v5 expects 0xb200005a,
their 4.0.0 fork accepts both, and the 4.8.0 fork as well as upstream
OP-TEE only accept 0xbc000000.

My earlier patch, commit ae4d08bbe3f4 ("arm64: dts: st: fix arm,smc-id
of the SMC watchdog on stm32mp2"), switched the dtsi to 0xbc000000. That
made the watchdog work with current OP-TEE, but it broke boards that run
the older firmware. So today 6.18.y uses one id and 7.4 the other. When
the kernel and the firmware disagree, nobody pets the watchdog that the
bootloader started, and the board resets after 32 seconds. Marek hit
this even with a recovery kernel, which is about the worst place for it.

The idea from that thread: let arm,smc-id be a list. The driver tries
the ids one by one and keeps the first one the firmware accepts. Older
kernels only read the first cell, so they behave exactly as they do
now. For stm32mp2 the list is <0xbc000000 0xb200005a>. Calling an id
that belongs to some other firmware service would be bad, so the
binding says the list may only hold ids known to be this watchdog.

Why RFC: arm,smc-id already exists in firmware/arm,scmi.yaml as a plain
uint32, and dt_binding_check complains when another binding turns it
into an array:

  arm-smc-wdt.yaml: arm,smc-id: multiple incompatible types: uint32-array
  arm,scmi.yaml: arm,smc-id: multiple incompatible types: uint32

DT folks, is there a way to keep the list in arm,smc-id that you would
be fine with? If not, I can move the extra ids into a new property,
something like:

  arm,smc-id = <0xbc000000>;
  arm,smc-alt-ids = <0xb200005a>;

The driver side would stay the same, it would just read the fallback
ids from the other property.

One more question: should patches 2 and 3 go to stable as well? Then
6.18.y would also work with both firmware generations. The 6.18.y
version of patch 3 would turn its 0xb200005a into the same list.

[1] https://lore.kernel.org/all/1b14e1e4-962f-4391-91fd-b09143641643@nabladev.com/

Mateusz Nowicki (3):
  dt-bindings: watchdog: arm-smc-wdt: allow a list of SMC ids
  watchdog: arm_smc_wdt: try every id of arm,smc-id
  arm64: dts: st: list both watchdog SMC ids on stm32mp23 and stm32mp25

 .../bindings/watchdog/arm-smc-wdt.yaml         |  9 +++++++--
 arch/arm64/boot/dts/st/stm32mp231.dtsi         |  2 +-
 arch/arm64/boot/dts/st/stm32mp251.dtsi         |  2 +-
 drivers/watchdog/arm_smc_wdt.c                 | 18 ++++++++++++------
 4 files changed, 21 insertions(+), 10 deletions(-)


base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.56.0

[RFC PATCH 1/3] dt-bindings: watchdog: arm-smc-wdt: allow a list of SMC ids

[Mateusz Nowicki]

The SMC function id of the watchdog service is not discoverable and
differs between firmware versions for the same SoC. Let arm,smc-id hold
a list of ids that the driver tries in order, so a SoC dtsi can name
every id used by firmware for that SoC. A single id stays valid.

Signed-off-by: Mateusz Nowicki <mateusz.nowicki@posteo.net>
---
 .../devicetree/bindings/watchdog/arm-smc-wdt.yaml        | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml b/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml
index 8e9d0b7e8244..9844b23ef51c 100644
--- a/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml
@@ -18,9 +18,14 @@ properties:
       - arm,smc-wdt
 
   arm,smc-id:
-    $ref: /schemas/types.yaml#/definitions/uint32
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 4
     description: |
-      The ATF smc function id used by the firmware.
+      The ATF smc function id used by the firmware, or a list of ids for
+      firmware versions that use different ones. The ids are tried in order
+      until the firmware accepts the init call, so only ids that are known
+      to be this watchdog service on the platform belong in the list.
       Defaults to 0x82003D06 if unset.
 
 required:
-- 
2.56.0

[RFC PATCH 2/3] watchdog: arm_smc_wdt: try every id of arm,smc-id

[Mateusz Nowicki]

The SMC function id of the watchdog service is not discoverable, and
firmware for the same SoC does not always use the same one: on the
STM32MP2 SoCs it is 0xbc000000 (the OP-TEE default for the platform) or
0xb200005a. With a single id in the SoC dtsi, every board whose firmware
uses the other id loses its watchdog.

Read arm,smc-id as a list and use the first id for which SMCWD_INIT
succeeds. Go to the next id on any error: OP-TEE answers an unknown
function id with 0xffffffff in a0, which smcwd_call() reports as -EIO,
not -ENODEV. A missing property still means 0x82003D06.

Signed-off-by: Mateusz Nowicki <mateusz.nowicki@posteo.net>
---
 drivers/watchdog/arm_smc_wdt.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/arm_smc_wdt.c b/drivers/watchdog/arm_smc_wdt.c
index c73ed1dcdfcb..993c8f92c871 100644
--- a/drivers/watchdog/arm_smc_wdt.c
+++ b/drivers/watchdog/arm_smc_wdt.c
@@ -113,8 +113,9 @@ static const struct watchdog_ops smcwd_timeleft_ops = {
 
 static int smcwd_probe(struct platform_device *pdev)
 {
+	struct device_node *np = pdev->dev.of_node;
 	struct watchdog_device *wdd;
-	int err;
+	int err, i, n;
 	struct arm_smccc_res res;
 	u32 smc_func_id;
 
@@ -123,12 +124,17 @@ static int smcwd_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, wdd);
 
-	if (of_property_read_u32(pdev->dev.of_node, "arm,smc-id",
-				 &smc_func_id))
-		smc_func_id = 0x82003D06;
-	watchdog_set_drvdata(wdd, (void *)(uintptr_t)smc_func_id);
+	/* The id is not discoverable: take the first one of the list the firmware accepts */
+	n = of_property_count_u32_elems(np, "arm,smc-id");
+	for (i = 0; i < max(n, 1); i++) {
+		if (of_property_read_u32_index(np, "arm,smc-id", i, &smc_func_id))
+			smc_func_id = 0x82003D06;
+		watchdog_set_drvdata(wdd, (void *)(uintptr_t)smc_func_id);
 
-	err = smcwd_call(wdd, SMCWD_INIT, 0, &res);
+		err = smcwd_call(wdd, SMCWD_INIT, 0, &res);
+		if (!err)
+			break;
+	}
 	if (err < 0)
 		return err;
 
-- 
2.56.0

[RFC PATCH 3/3] arm64: dts: st: list both watchdog SMC ids on stm32mp23 and stm32mp25

[Mateusz Nowicki]

Commit ae4d08bbe3f4 ("arm64: dts: st: fix arm,smc-id of the SMC watchdog
on stm32mp2") changed the watchdog SMC id from 0xb200005a to
0xbc000000, the OP-TEE default for the platform. That fixed the watchdog
with such firmware but broke it on boards whose firmware still uses
0xb200005a. List both ids, the driver takes the first one the firmware
accepts. Kernels that read a single id keep using 0xbc000000.

Fixes: ae4d08bbe3f4 ("arm64: dts: st: fix arm,smc-id of the SMC watchdog on stm32mp2")
Reported-by: Marek Vasut <marex@nabladev.com>
Closes: https://lore.kernel.org/all/1b14e1e4-962f-4391-91fd-b09143641643@nabladev.com/
Signed-off-by: Mateusz Nowicki <mateusz.nowicki@posteo.net>
---
 arch/arm64/boot/dts/st/stm32mp231.dtsi | 2 +-
 arch/arm64/boot/dts/st/stm32mp251.dtsi | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/st/stm32mp231.dtsi b/arch/arm64/boot/dts/st/stm32mp231.dtsi
index fefbf6f4f153..7526e86f5a1d 100644
--- a/arch/arm64/boot/dts/st/stm32mp231.dtsi
+++ b/arch/arm64/boot/dts/st/stm32mp231.dtsi
@@ -35,7 +35,7 @@ arm-pmu {
 
 	arm_wdt: watchdog {
 		compatible = "arm,smc-wdt";
-		arm,smc-id = <0xbc000000>;
+		arm,smc-id = <0xbc000000 0xb200005a>;
 		status = "disabled";
 	};
 
diff --git a/arch/arm64/boot/dts/st/stm32mp251.dtsi b/arch/arm64/boot/dts/st/stm32mp251.dtsi
index dac3d2d23ff0..143d83216530 100644
--- a/arch/arm64/boot/dts/st/stm32mp251.dtsi
+++ b/arch/arm64/boot/dts/st/stm32mp251.dtsi
@@ -36,7 +36,7 @@ arm-pmu {
 
 	arm_wdt: watchdog {
 		compatible = "arm,smc-wdt";
-		arm,smc-id = <0xbc000000>;
+		arm,smc-id = <0xbc000000 0xb200005a>;
 		status = "disabled";
 	};
 
-- 
2.56.0

Re: [RFC PATCH 0/3] watchdog: arm_smc_wdt: allow a list of SMC function ids

[Guenter Roeck]

On 11/4/26 08:16, Mateusz Nowicki wrote:
> Hi all,
> 
> this follows up on Marek's report about the stm32mp2 watchdog [1].
> 
> A quick recap: the arm,smc-wdt driver talks to the firmware through one
> SMC function id that it reads from the device tree, and there is no way
> to ask the firmware which id it uses. On the STM32MP2 SoCs that id
> changed over time. ST's OP-TEE from ecosystem v5 expects 0xb200005a,
> their 4.0.0 fork accepts both, and the 4.8.0 fork as well as upstream
> OP-TEE only accept 0xbc000000.
> 
> My earlier patch, commit ae4d08bbe3f4 ("arm64: dts: st: fix arm,smc-id
> of the SMC watchdog on stm32mp2"), switched the dtsi to 0xbc000000. That
> made the watchdog work with current OP-TEE, but it broke boards that run
> the older firmware. So today 6.18.y uses one id and 7.4 the other. When
> the kernel and the firmware disagree, nobody pets the watchdog that the
> bootloader started, and the board resets after 32 seconds. Marek hit
> this even with a recovery kernel, which is about the worst place for it.
> 

So the DT on the board is updated to use the new ID but the firmware itself
isn't ? That seems a bit odd; I would have assumed that the DT on a board
is kept in sync with its firmware. Is that not the case here ?

> The idea from that thread: let arm,smc-id be a list. The driver tries
> the ids one by one and keeps the first one the firmware accepts. Older
> kernels only read the first cell, so they behave exactly as they do
> now. For stm32mp2 the list is <0xbc000000 0xb200005a>. Calling an id
> that belongs to some other firmware service would be bad, so the
> binding says the list may only hold ids known to be this watchdog.
> 

Would it possibly make sense to check keep a list of known IDs in the
driver ? It already checks 0x82003D06, so it might as well check a sequence.

Thanks,
Guenter

> Why RFC: arm,smc-id already exists in firmware/arm,scmi.yaml as a plain
> uint32, and dt_binding_check complains when another binding turns it
> into an array:
> 
>    arm-smc-wdt.yaml: arm,smc-id: multiple incompatible types: uint32-array
>    arm,scmi.yaml: arm,smc-id: multiple incompatible types: uint32
> 
> DT folks, is there a way to keep the list in arm,smc-id that you would
> be fine with? If not, I can move the extra ids into a new property,
> something like:
> 
>    arm,smc-id = <0xbc000000>;
>    arm,smc-alt-ids = <0xb200005a>;
> 
> The driver side would stay the same, it would just read the fallback
> ids from the other property.
> 
> One more question: should patches 2 and 3 go to stable as well? Then
> 6.18.y would also work with both firmware generations. The 6.18.y
> version of patch 3 would turn its 0xb200005a into the same list.
> 
> [1] https://lore.kernel.org/all/1b14e1e4-962f-4391-91fd-b09143641643@nabladev.com/
> 
> Mateusz Nowicki (3):
>    dt-bindings: watchdog: arm-smc-wdt: allow a list of SMC ids
>    watchdog: arm_smc_wdt: try every id of arm,smc-id
>    arm64: dts: st: list both watchdog SMC ids on stm32mp23 and stm32mp25
> 
>   .../bindings/watchdog/arm-smc-wdt.yaml         |  9 +++++++--
>   arch/arm64/boot/dts/st/stm32mp231.dtsi         |  2 +-
>   arch/arm64/boot/dts/st/stm32mp251.dtsi         |  2 +-
>   drivers/watchdog/arm_smc_wdt.c                 | 18 ++++++++++++------
>   4 files changed, 21 insertions(+), 10 deletions(-)
> 
> 
> base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4

Re: [RFC PATCH 0/3] watchdog: arm_smc_wdt: allow a list of SMC function ids

[mateusz.nowicki]

On 11/4/26 Guenter Roeck wrote:
> On 11/4/26 08:16, Mateusz Nowicki wrote:
>> My earlier patch, commit ae4d08bbe3f4 ("arm64: dts: st: fix arm,smc-id
>> of the SMC watchdog on stm32mp2"), switched the dtsi to 0xbc000000. 
>> That
>> made the watchdog work with current OP-TEE, but it broke boards that 
>> run
>> the older firmware. So today 6.18.y uses one id and 7.4 the other. 
>> When
>> the kernel and the firmware disagree, nobody pets the watchdog that 
>> the
>> bootloader started, and the board resets after 32 seconds. Marek hit
>> this even with a recovery kernel, which is about the worst place for 
>> it.
>> 
> 
> So the DT on the board is updated to use the new ID but the firmware 
> itself
> isn't ? That seems a bit odd; I would have assumed that the DT on a 
> board
> is kept in sync with its firmware. Is that not the case here ?

Not necessarily. On these boards the DTB is built from the kernel tree
and installed with the kernel, U-Boot just loads it from the boot
partition. TF-A and OP-TEE sit in the FIP and are updated separately, so
updating the kernel alone changes the ID. That's how Marek got hit: he
updated OP-TEE, but his recovery kernel still had the old DTB.

>> The idea from that thread: let arm,smc-id be a list. The driver tries
>> the ids one by one and keeps the first one the firmware accepts. Older
>> kernels only read the first cell, so they behave exactly as they do
>> now. For stm32mp2 the list is <0xbc000000 0xb200005a>. Calling an id
>> that belongs to some other firmware service would be bad, so the
>> binding says the list may only hold ids known to be this watchdog.
>> 
> 
> Would it possibly make sense to check keep a list of known IDs in the
> driver ? It already checks 0x82003D06, so it might as well check a 
> sequence.

Fine with me, and it avoids the binding issue. I'd just rather not try
the ST IDs on every platform, so how about making the list per
compatible:

   compatible = "st,stm32mp25-smc-wdt", "arm,smc-wdt";

with { 0xbc000000, 0xb200005a } as match data for it. arm,smc-id stays a
single u32 and, if present, is tried first. Older kernels just match
"arm,smc-wdt" and behave as today.

If that works for you and the DT folks, I'll send v2.

Thanks,
Mateusz

Re: [RFC PATCH 0/3] watchdog: arm_smc_wdt: allow a list of SMC function ids

[Guenter Roeck]

On 11/4/26 10:10, mateusz.nowicki@posteo.net wrote:
> On 11/4/26 Guenter Roeck wrote:
>> On 11/4/26 08:16, Mateusz Nowicki wrote:
>>> My earlier patch, commit ae4d08bbe3f4 ("arm64: dts: st: fix arm,smc-id
>>> of the SMC watchdog on stm32mp2"), switched the dtsi to 0xbc000000. That
>>> made the watchdog work with current OP-TEE, but it broke boards that run
>>> the older firmware. So today 6.18.y uses one id and 7.4 the other. When
>>> the kernel and the firmware disagree, nobody pets the watchdog that the
>>> bootloader started, and the board resets after 32 seconds. Marek hit
>>> this even with a recovery kernel, which is about the worst place for it.
>>>
>>
>> So the DT on the board is updated to use the new ID but the firmware itself
>> isn't ? That seems a bit odd; I would have assumed that the DT on a board
>> is kept in sync with its firmware. Is that not the case here ?
> 
> Not necessarily. On these boards the DTB is built from the kernel tree
> and installed with the kernel, U-Boot just loads it from the boot
> partition. TF-A and OP-TEE sit in the FIP and are updated separately, so
> updating the kernel alone changes the ID. That's how Marek got hit: he
> updated OP-TEE, but his recovery kernel still had the old DTB.
> 

If the DT does not really reflect the HW, is installed as part of the kernel
image, and the ID can change across FW versions, it seems to me that it does
not even make sense to store the ID in devicetree.

>>> The idea from that thread: let arm,smc-id be a list. The driver tries
>>> the ids one by one and keeps the first one the firmware accepts. Older
>>> kernels only read the first cell, so they behave exactly as they do
>>> now. For stm32mp2 the list is <0xbc000000 0xb200005a>. Calling an id
>>> that belongs to some other firmware service would be bad, so the
>>> binding says the list may only hold ids known to be this watchdog.
>>>
>>
>> Would it possibly make sense to check keep a list of known IDs in the
>> driver ? It already checks 0x82003D06, so it might as well check a sequence.
> 
> Fine with me, and it avoids the binding issue. I'd just rather not try
> the ST IDs on every platform, so how about making the list per
> compatible:
> 
>    compatible = "st,stm32mp25-smc-wdt", "arm,smc-wdt";
> 
> with { 0xbc000000, 0xb200005a } as match data for it. arm,smc-id stays a
> single u32 and, if present, is tried first. Older kernels just match
> "arm,smc-wdt" and behave as today.
> 
> If that works for you and the DT folks, I'll send v2.
> 
Works for me.

Thanks,
Guenter

Re: [RFC PATCH 0/3] watchdog: arm_smc_wdt: allow a list of SMC function ids

[Julius Werner]

> DT folks, is there a way to keep the list in arm,smc-id that you would
> be fine with?

Perhaps this is a bit of a hack, but you're in an unintended situation
that wasn't supposed to happen to begin with so I feel it might be
appropriate if it avoids adding a whole new mechanism to the driver:
What happens if you simply define two watchdog nodes in your DTS, one
for each SMC ID? Both of them would independently try to probe, one of
them would die from getting an SMC_UNK, and the other would continue
working to pet your watchdog?

This driver started as encoding the SMC ID in the DTS and that kinda
seems to be the standard for drivers interacting with non-standard
SMCs, so I would prefer to stick to that rather than hardcoding IDs in
the C source (if we do that for each board it may end up being a long
list eventually — I think one of the reasons the device tree was
originally adopted was to avoid things like that). I would be fine
with the array solution but I don't know about the validation checker
and whether people care about reusing the same prop name with
different types. (FWIW, in the device tree a 32-bit integer is
basically a 1-element array anyway, so we might just say it's always
an array and for SCMI only one element is allowed.)