2024-05-21 08:12:43

by Arnaud Pouliquen

[permalink] [raw]
Subject: [PATCH v5 2/7] dt-bindings: remoteproc: Add compatibility for TEE support

The "st,stm32mp1-m4-tee" compatible is utilized in a system configuration
where the Cortex-M4 firmware is loaded by the Trusted execution Environment
(TEE).
For instance, this compatible is used in both the Linux and OP-TEE
device-tree:
- In OP-TEE, a node is defined in the device tree with the
st,stm32mp1-m4-tee to support signed remoteproc firmware.
Based on DT properties, OP-TEE authenticates, loads, starts, and stops
the firmware.
- On Linux, when the compatibility is set, the Cortex-M resets should not
be declared in the device tree.

Signed-off-by: Arnaud Pouliquen <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
.../bindings/remoteproc/st,stm32-rproc.yaml | 51 ++++++++++++++++---
1 file changed, 43 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
index 370af61d8f28..36ea54016b76 100644
--- a/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
@@ -16,7 +16,12 @@ maintainers:

properties:
compatible:
- const: st,stm32mp1-m4
+ enum:
+ - st,stm32mp1-m4
+ - st,stm32mp1-m4-tee
+ description:
+ Use "st,stm32mp1-m4" for the Cortex-M4 coprocessor management by non-secure context
+ Use "st,stm32mp1-m4-tee" for the Cortex-M4 coprocessor management by secure context

reg:
description:
@@ -142,21 +147,41 @@ properties:
required:
- compatible
- reg
- - resets

allOf:
- if:
properties:
- reset-names:
- not:
- contains:
- const: hold_boot
+ compatible:
+ contains:
+ const: st,stm32mp1-m4
then:
+ if:
+ properties:
+ reset-names:
+ not:
+ contains:
+ const: hold_boot
+ then:
+ required:
+ - st,syscfg-holdboot
+ else:
+ properties:
+ st,syscfg-holdboot: false
+ required:
+ - reset-names
required:
- - st,syscfg-holdboot
- else:
+ - resets
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: st,stm32mp1-m4-tee
+ then:
properties:
st,syscfg-holdboot: false
+ reset-names: false
+ resets: false

additionalProperties: false

@@ -188,5 +213,15 @@ examples:
st,syscfg-rsc-tbl = <&tamp 0x144 0xFFFFFFFF>;
st,syscfg-m4-state = <&tamp 0x148 0xFFFFFFFF>;
};
+ - |
+ #include <dt-bindings/reset/stm32mp1-resets.h>
+ m4@10000000 {
+ compatible = "st,stm32mp1-m4-tee";
+ reg = <0x10000000 0x40000>,
+ <0x30000000 0x40000>,
+ <0x38000000 0x10000>;
+ st,syscfg-rsc-tbl = <&tamp 0x144 0xFFFFFFFF>;
+ st,syscfg-m4-state = <&tamp 0x148 0xFFFFFFFF>;
+ };

...
--
2.25.1



2024-05-21 09:25:18

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v5 2/7] dt-bindings: remoteproc: Add compatibility for TEE support

On 21/05/2024 10:09, Arnaud Pouliquen wrote:
> The "st,stm32mp1-m4-tee" compatible is utilized in a system configuration
> where the Cortex-M4 firmware is loaded by the Trusted execution Environment
> (TEE).
> For instance, this compatible is used in both the Linux and OP-TEE
> device-tree:
> - In OP-TEE, a node is defined in the device tree with the
> st,stm32mp1-m4-tee to support signed remoteproc firmware.
> Based on DT properties, OP-TEE authenticates, loads, starts, and stops
> the firmware.
> - On Linux, when the compatibility is set, the Cortex-M resets should not
> be declared in the device tree.
>

Not tested.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline), work on fork of kernel
(don't, instead use mainline) or you ignore some maintainers (really
don't). Just use b4 and everything should be fine, although remember
about `b4 prep --auto-to-cc` if you added new patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time, thus I will skip this patch entirely till you follow
the process allowing the patch to be tested.

Please kindly resend and include all necessary To/Cc entries.

Best regards,
Krzysztof


2024-05-21 12:18:08

by Arnaud Pouliquen

[permalink] [raw]
Subject: Re: [PATCH v5 2/7] dt-bindings: remoteproc: Add compatibility for TEE support


On 5/21/24 11:24, Krzysztof Kozlowski wrote:
> On 21/05/2024 10:09, Arnaud Pouliquen wrote:
>> The "st,stm32mp1-m4-tee" compatible is utilized in a system configuration
>> where the Cortex-M4 firmware is loaded by the Trusted execution Environment
>> (TEE).
>> For instance, this compatible is used in both the Linux and OP-TEE
>> device-tree:
>> - In OP-TEE, a node is defined in the device tree with the
>> st,stm32mp1-m4-tee to support signed remoteproc firmware.
>> Based on DT properties, OP-TEE authenticates, loads, starts, and stops
>> the firmware.
>> - On Linux, when the compatibility is set, the Cortex-M resets should not
>> be declared in the device tree.
>>
>
> Not tested.
>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
>
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline), work on fork of kernel
> (don't, instead use mainline) or you ignore some maintainers (really
> don't). Just use b4 and everything should be fine, although remember
> about `b4 prep --auto-to-cc` if you added new patches to the patchset.
>
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might be
> a waste of time, thus I will skip this patch entirely till you follow
> the process allowing the patch to be tested.
>
> Please kindly resend and include all necessary To/Cc entries.

I apologize for this oversight; I will resend the pull request and adding
the missing CC and To.

Thanks!
Arnaud

>
> Best regards,
> Krzysztof
>

2024-05-28 20:09:12

by Mathieu Poirier

[permalink] [raw]
Subject: Re: [PATCH v5 2/7] dt-bindings: remoteproc: Add compatibility for TEE support

On Tue, May 21, 2024 at 10:09:56AM +0200, Arnaud Pouliquen wrote:
> The "st,stm32mp1-m4-tee" compatible is utilized in a system configuration
> where the Cortex-M4 firmware is loaded by the Trusted execution Environment
> (TEE).
> For instance, this compatible is used in both the Linux and OP-TEE
> device-tree:
> - In OP-TEE, a node is defined in the device tree with the
> st,stm32mp1-m4-tee to support signed remoteproc firmware.
> Based on DT properties, OP-TEE authenticates, loads, starts, and stops
> the firmware.

I don't see how firmware can be started and stopped. Please rework.

> - On Linux, when the compatibility is set, the Cortex-M resets should not
> be declared in the device tree.

This is a description of "what" is happening and not "why".

More comments to come shortly.

Thanks,
Mathieu

>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
> ---
> .../bindings/remoteproc/st,stm32-rproc.yaml | 51 ++++++++++++++++---
> 1 file changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
> index 370af61d8f28..36ea54016b76 100644
> --- a/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
> @@ -16,7 +16,12 @@ maintainers:
>
> properties:
> compatible:
> - const: st,stm32mp1-m4
> + enum:
> + - st,stm32mp1-m4
> + - st,stm32mp1-m4-tee
> + description:
> + Use "st,stm32mp1-m4" for the Cortex-M4 coprocessor management by non-secure context
> + Use "st,stm32mp1-m4-tee" for the Cortex-M4 coprocessor management by secure context
>
> reg:
> description:
> @@ -142,21 +147,41 @@ properties:
> required:
> - compatible
> - reg
> - - resets
>
> allOf:
> - if:
> properties:
> - reset-names:
> - not:
> - contains:
> - const: hold_boot
> + compatible:
> + contains:
> + const: st,stm32mp1-m4
> then:
> + if:
> + properties:
> + reset-names:
> + not:
> + contains:
> + const: hold_boot
> + then:
> + required:
> + - st,syscfg-holdboot
> + else:
> + properties:
> + st,syscfg-holdboot: false
> + required:
> + - reset-names
> required:
> - - st,syscfg-holdboot
> - else:
> + - resets
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: st,stm32mp1-m4-tee
> + then:
> properties:
> st,syscfg-holdboot: false
> + reset-names: false
> + resets: false
>
> additionalProperties: false
>
> @@ -188,5 +213,15 @@ examples:
> st,syscfg-rsc-tbl = <&tamp 0x144 0xFFFFFFFF>;
> st,syscfg-m4-state = <&tamp 0x148 0xFFFFFFFF>;
> };
> + - |
> + #include <dt-bindings/reset/stm32mp1-resets.h>
> + m4@10000000 {
> + compatible = "st,stm32mp1-m4-tee";
> + reg = <0x10000000 0x40000>,
> + <0x30000000 0x40000>,
> + <0x38000000 0x10000>;
> + st,syscfg-rsc-tbl = <&tamp 0x144 0xFFFFFFFF>;
> + st,syscfg-m4-state = <&tamp 0x148 0xFFFFFFFF>;
> + };
>
> ...
> --
> 2.25.1
>