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

[Guenter Roeck <linux@roeck-us.net>]

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