Re: [PATCH v2 2/3] arm64: dts: ti: k3-{j784s4-j742s2/j721s2}-mcu-wakeup: Add HSM M4F node

[Nishanth Menon <nm@ti.com>]

On 16:17-20260106, Beleswar Padhi wrote:
> The TI K3 J721S2, J784S4 and J742S2 SoCs have a HSM (High Security
> Module) M4F core in the Wakeup Voltage Domain which could be used to run
> secure services like Authentication. Add Device Tree Node definitions
> for the HSM core in the respective SoC wakeup dtsi files.
> 
> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
> ---
> v2: Changelog:
> 1. None
> 
> Link to v1:
> https://lore.kernel.org/all/20251231165102.950644-3-b-padhi@ti.com/
> 
>  arch/arm64/boot/dts/ti/k3-j721s2-mcu-wakeup.dtsi  | 15 +++++++++++++++
>  .../ti/k3-j784s4-j742s2-mcu-wakeup-common.dtsi    | 15 +++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-j721s2-mcu-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-j721s2-mcu-wakeup.dtsi
> index fd01437726ab4..c3d78d4a838a1 100644
> --- a/arch/arm64/boot/dts/ti/k3-j721s2-mcu-wakeup.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j721s2-mcu-wakeup.dtsi
> @@ -766,4 +766,19 @@ mcu_watchdog1: watchdog@40610000 {
>  		/* reserved for MCU_R5F0_1 */
>  		status = "reserved";
>  	};
> +
> +	hsm_m4fss: m4fss@43c00000 {

You did fix this in the binding example.. but missed in dts.

The node name should use the generic type, not the instance name. It should
be "remoteproc@43c00000", not "m4fss@43c00000".

Additionally for the label, why not just use hsm: like we have for sms?

> +		compatible = "ti,hsm-m4fss";
> +		reg = <0x00 0x43c00000 0x00 0x20000>,
> +		      <0x00 0x43c20000 0x00 0x10000>,
> +		      <0x00 0x43c30000 0x00 0x10000>;

The total address range covered here is 0x43c00000-0x43c40000, which is
0x40000 bytes, matching the ranges entry. However, you're defining three
separate regions: 0x43c00000-0x43c20000 (0x20000), 0x43c20000-0x43c30000
(0x10000), and 0x43c30000-0x43c40000 (0x10000).

I assume you are doing this since the h/w integration could be
instantiated differently?


> +		reg-names = "sram0_0", "sram0_1", "sram1";
> +		resets = <&k3_reset 304 1>;
> +		firmware-name = "hsm.bin";

I am not a fan of putting firmware-name in SoC.dtsi - esp when it is
reserved, further, so far we have been using j722s-wkup-r5f0_0-fw and
so on.. which allows for firmware specific to SoC.. which kind of makes
sense here as well.

> +		ti,sci = <&sms>;
> +		ti,sci-dev-id = <304>;
> +		ti,sci-proc-ids = <0x80 0xff>;
> +		status = "disabled";

As usual, document why? Additionally, should this be reserved?

> +		bootph-pre-ram;

"standard property"

Documentation/devicetree/bindings/dts-coding-style.rst - note the order:
1. "compatible"
2. "reg"
3. "ranges"
4. Standard/common properties (defined by common bindings, e.g. without
   vendor-prefixes)
5. Vendor-specific properties
6. "status" (if applicable), preceded by a blank line if there is content
   before the property
7. Child nodes, where each node is preceded with a blank line
> +	};
>  };

Same for the rest of the patches and nodes

> diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-j742s2-mcu-wakeup-common.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-j742s2-mcu-wakeup-common.dtsi
> index cc22bfb5f5996..42565f41b7bac 100644
> --- a/arch/arm64/boot/dts/ti/k3-j784s4-j742s2-mcu-wakeup-common.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j784s4-j742s2-mcu-wakeup-common.dtsi
> @@ -762,4 +762,19 @@ mcu_watchdog1: watchdog@40610000 {
>  		/* reserved for MCU_R5F0_1 */
>  		status = "reserved";
>  	};
> +
> +	hsm_m4fss: m4fss@43c00000 {
> +		compatible = "ti,hsm-m4fss";
> +		reg = <0x00 0x43c00000 0x00 0x20000>,
> +		      <0x00 0x43c20000 0x00 0x10000>,
> +		      <0x00 0x43c30000 0x00 0x10000>;
> +		reg-names = "sram0_0", "sram0_1", "sram1";
> +		resets = <&k3_reset 371 1>;
> +		firmware-name = "hsm.bin";
> +		ti,sci = <&sms>;
> +		ti,sci-dev-id = <371>;
> +		ti,sci-proc-ids = <0x80 0xff>;
> +		status = "disabled";
> +		bootph-pre-ram;
> +	};
>  };
> -- 
> 2.34.1
> 

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D
https://ti.com/opensource