[PATCH v4 1/7] dt-bindings: arm: fsl: add se-fw binding doc

Pankaj Gupta posted 7 patches 2 years, 7 months ago
[PATCH v4 1/7] dt-bindings: arm: fsl: add se-fw binding doc
Posted by Pankaj Gupta 2 years, 7 months ago
The NXP's i.MX EdgeLock Enclave, a HW IP creating an embedded
secure enclave within the SoC boundary to enable features like
- HSM
- SHE
- V2X

Communicates via message unit with linux kernel. This driver
is enables communication ensuring well defined message sequence
protocol between Application Core and enclave's firmware.

Driver configures multiple misc-device on the MU, for multiple
user-space applications can communicate on single MU.

It exists on some i.MX processors. e.g. i.MX8ULP, i.MX93 etc.

Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
---
 .../bindings/arm/freescale/fsl,se-fw.yaml     | 121 ++++++++++++++++++
 1 file changed, 121 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml

diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml b/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
new file mode 100644
index 000000000000..7567da0b4c21
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
@@ -0,0 +1,121 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/freescale/fsl,se-fw.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP i.MX EdgeLock Enclave Firmware (ELEFW)
+
+maintainers:
+  - Pankaj Gupta <pankaj.gupta@nxp.com>
+
+description: |
+
+  The NXP's i.MX EdgeLock Enclave, a HW IP creating an embedded
+  secure enclave within the SoC boundary to enable features like
+  - HSM
+  - SHE
+  - V2X
+
+  It uses message unit to communicate and coordinate to pass messages
+  (e.g., data,  status and control) through its interfaces.
+
+  This driver configures multiple misc-devices on the MU, to exchange
+  messages from User-space application and NXP's Edgelocke Enclave firmware.
+  The driver ensures that the messages must follow the following protocol
+  defined.
+
+                                     Non-Secure           +   Secure
+                                                          |
+                                                          |
+                   +---------+      +-------------+       |
+                   | ele_mu.c+<---->+imx-mailbox.c|       |
+                   |         |      |  mailbox.c  +<-->+------+    +------+
+                   +---+-----+      +-------------+    | MU X +<-->+ ELE |
+                       |                               +------+    +------+
+                       +----------------+                 |
+                       |                |                 |
+                       v                v                 |
+                   logical           logical              |
+                   receiver          waiter               |
+                      +                 +                 |
+                      |                 |                 |
+                      |                 |                 |
+                      |            +----+------+          |
+                      |            |           |          |
+                      |            |           |          |
+               device_ctx     device_ctx     device_ctx   |
+                                                          |
+                 User 0        User 1       User Y        |
+                 +------+      +------+     +------+      |
+                 |misc.c|      |misc.c|     |misc.c|      |
+  kernel space   +------+      +------+     +------+      |
+                                                          |
+  +------------------------------------------------------ |
+                     |             |           |          |
+  userspace     /dev/ele_muXch0    |           |          |
+                           /dev/ele_muXch1     |          |
+                                         /dev/ele_muXchY  |
+                                                          |
+
+  When a user sends a command to the ELE, it registers its device_ctx as
+  waiter of a response from ELE.
+
+  A user can be registered as receiver of command from the ELE.
+  Create char devices in /dev as channels of the form /dev/ele_muXchY with X
+  the id of the driver and Y for each users. It allows to send and receive
+  messages to the NXP EdgeLock Enclave IP on NXP SoC, where current possible
+  value, i.e., supported SoC(s) are imx8ulp, imx93.
+
+properties:
+  compatible:
+    enum:
+      - fsl,imx-ele
+      - fsl,imx93-ele
+
+  mboxes:
+    description:
+      A list of phandles of TX MU channels followed by a list of phandles of
+      RX MU channels. The number of expected tx and rx channels is 1 TX, and
+      1 RX channels. All MU channels must be within the same MU instance.
+      Cross instances are not allowed. The MU instance to be used is S4MUAP
+      for imx8ulp & imx93. Users need to ensure that used MU instance does not
+      conflict with other execution environments.
+    items:
+      - description: TX0 MU channel
+      - description: RX0 MU channel
+
+  mbox-names:
+    items:
+      - const: tx
+      - const: rx
+
+  fsl,mu-did:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Owner of message-unit, is identified via Domain ID or did.
+
+  fsl,mu-id:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Identifier to the message-unit among the multiple message-unit that exists on SoC.
+      It is used to create the channels, default to 2
+
+
+required:
+  - compatible
+  - mboxes
+  - mbox-names
+
+additionalProperties: false
+
+examples:
+  - |
+    ele_mu: ele_mu {
+      compatible = "fsl,imx93-ele";
+      mbox-names = "tx", "rx";
+      mboxes = <&s4muap 2 0
+                &s4muap 3 0>;
+      fsl,mu-did = <1>;
+      fsl,mu-id = <1>;
+    };
-- 
2.34.1
Re: [PATCH v4 1/7] dt-bindings: arm: fsl: add se-fw binding doc
Posted by Conor Dooley 2 years, 7 months ago
Hey,

On Wed, Jul 12, 2023 at 05:42:13PM +0530, Pankaj Gupta wrote:
> The NXP's i.MX EdgeLock Enclave, a HW IP creating an embedded
> secure enclave within the SoC boundary to enable features like
> - HSM
> - SHE
> - V2X
> 
> Communicates via message unit with linux kernel. This driver
> is enables communication ensuring well defined message sequence
> protocol between Application Core and enclave's firmware.
> 
> Driver configures multiple misc-device on the MU, for multiple
> user-space applications can communicate on single MU.
> 
> It exists on some i.MX processors. e.g. i.MX8ULP, i.MX93 etc.
> 
> Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
> ---
>  .../bindings/arm/freescale/fsl,se-fw.yaml     | 121 ++++++++++++++++++
>  1 file changed, 121 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml b/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
> new file mode 100644
> index 000000000000..7567da0b4c21
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
> @@ -0,0 +1,121 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/freescale/fsl,se-fw.yaml#

I think on v3 you were asked to use a filename that matches the
compatibles?

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP i.MX EdgeLock Enclave Firmware (ELEFW)
> +
> +maintainers:
> +  - Pankaj Gupta <pankaj.gupta@nxp.com>

> +  value, i.e., supported SoC(s) are imx8ulp, imx93.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - fsl,imx-ele

This looks like a generic compatible, not a specific one, but you use it
on the imx8ulp. I would have expected that you would have something like
"fsl,imx8ulp-ele" for that.

> +      - fsl,imx93-ele


> +
> +  mboxes:
> +    description:
> +      A list of phandles of TX MU channels followed by a list of phandles of
> +      RX MU channels. The number of expected tx and rx channels is 1 TX, and
> +      1 RX channels. All MU channels must be within the same MU instance.
> +      Cross instances are not allowed. The MU instance to be used is S4MUAP
> +      for imx8ulp & imx93. Users need to ensure that used MU instance does not
> +      conflict with other execution environments.
> +    items:
> +      - description: TX0 MU channel
> +      - description: RX0 MU channel
> +
> +  mbox-names:
> +    items:
> +      - const: tx
> +      - const: rx
> +
> +  fsl,mu-did:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Owner of message-unit, is identified via Domain ID or did.

On v3 you had constraints:
	enum: [0, 1, 2, 3, 4, 5, 6, 7]
Do constraints no longer apply? If they do, you can use minimum &
maximum to specify them.

> +  fsl,mu-id:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Identifier to the message-unit among the multiple message-unit that exists on SoC.
> +      It is used to create the channels, default to 2

Are there constraints here? If so, same applies.
You should use "default:" for defaults, rather than describing them in
freeform text.

Thanks,
Conor.
Re: [PATCH v4 1/7] dt-bindings: arm: fsl: add se-fw binding doc
Posted by Krzysztof Kozlowski 2 years, 7 months ago
On 12/07/2023 20:26, Conor Dooley wrote:
> Hey,
> 
> On Wed, Jul 12, 2023 at 05:42:13PM +0530, Pankaj Gupta wrote:
>> The NXP's i.MX EdgeLock Enclave, a HW IP creating an embedded
>> secure enclave within the SoC boundary to enable features like
>> - HSM
>> - SHE
>> - V2X
>>
>> Communicates via message unit with linux kernel. This driver
>> is enables communication ensuring well defined message sequence
>> protocol between Application Core and enclave's firmware.
>>
>> Driver configures multiple misc-device on the MU, for multiple
>> user-space applications can communicate on single MU.
>>
>> It exists on some i.MX processors. e.g. i.MX8ULP, i.MX93 etc.
>>
>> Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
>> ---
>>  .../bindings/arm/freescale/fsl,se-fw.yaml     | 121 ++++++++++++++++++
>>  1 file changed, 121 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml b/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
>> new file mode 100644
>> index 000000000000..7567da0b4c21
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
>> @@ -0,0 +1,121 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/arm/freescale/fsl,se-fw.yaml#
> 
> I think on v3 you were asked to use a filename that matches the
> compatibles?
> 
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: NXP i.MX EdgeLock Enclave Firmware (ELEFW)
>> +
>> +maintainers:
>> +  - Pankaj Gupta <pankaj.gupta@nxp.com>
> 
>> +  value, i.e., supported SoC(s) are imx8ulp, imx93.
> 
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - fsl,imx-ele
> 
> This looks like a generic compatible, not a specific one, but you use it
> on the imx8ulp. I would have expected that you would have something like
> "fsl,imx8ulp-ele" for that.

Yeah, this one looks generic, so not what we expect.

> 
>> +      - fsl,imx93-ele
> 
> 
>> +
>> +  mboxes:
>> +    description:
>> +      A list of phandles of TX MU channels followed by a list of phandles of
>> +      RX MU channels. The number of expected tx and rx channels is 1 TX, and
>> +      1 RX channels. 

Don't repeat constraints in free form text. This is obvious from the
items below.


Best regards,
Krzysztof
RE: [EXT] Re: [PATCH v4 1/7] dt-bindings: arm: fsl: add se-fw binding doc
Posted by Pankaj Gupta 2 years, 6 months ago

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Thursday, July 13, 2023 12:05 AM
> To: Conor Dooley <conor@kernel.org>; Pankaj Gupta
> <pankaj.gupta@nxp.com>
> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> clin@suse.com; conor+dt@kernel.org; pierre.gondois@arm.com; Jacky Bai
> <ping.bai@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; Wei Fang
> <wei.fang@nxp.com>; Peng Fan <peng.fan@nxp.com>; Bough Chen
> <haibo.chen@nxp.com>; festevam@gmail.com; dl-linux-imx <linux-
> imx@nxp.com>; davem@davemloft.net; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Gaurav Jain
> <gaurav.jain@nxp.com>; alexander.stein@ew.tq-group.com; Sahil Malhotra
> <sahil.malhotra@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>; Varun
> Sethi <V.Sethi@nxp.com>
> Subject: [EXT] Re: [PATCH v4 1/7] dt-bindings: arm: fsl: add se-fw binding doc
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> On 12/07/2023 20:26, Conor Dooley wrote:
> > Hey,
> >
> > On Wed, Jul 12, 2023 at 05:42:13PM +0530, Pankaj Gupta wrote:
> >> The NXP's i.MX EdgeLock Enclave, a HW IP creating an embedded secure
> >> enclave within the SoC boundary to enable features like
> >> - HSM
> >> - SHE
> >> - V2X
> >>
> >> Communicates via message unit with linux kernel. This driver is
> >> enables communication ensuring well defined message sequence protocol
> >> between Application Core and enclave's firmware.
> >>
> >> Driver configures multiple misc-device on the MU, for multiple
> >> user-space applications can communicate on single MU.
> >>
> >> It exists on some i.MX processors. e.g. i.MX8ULP, i.MX93 etc.
> >>
> >> Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
> >> ---
> >>  .../bindings/arm/freescale/fsl,se-fw.yaml     | 121 ++++++++++++++++++
> >>  1 file changed, 121 insertions(+)
> >>  create mode 100644
> >> Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
> >> b/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
> >> new file mode 100644
> >> index 000000000000..7567da0b4c21
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
> >> @@ -0,0 +1,121 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> >> +---
> >> +$id:
> >> +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdev
> >> +icetree.org%2Fschemas%2Farm%2Ffreescale%2Ffsl%2Cse-
> fw.yaml%23&data=0
> >>
> +5%7C01%7Cpankaj.gupta%40nxp.com%7Cde3f4d12573845f8b7db08db830
> 6b165%7
> >>
> +C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638247836986636
> 866%7CUnk
> >>
> +nown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik
> 1ha
> >>
> +WwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=PUeKu1fQmYpTsN72LA
> YGRS2daqnO
> >> +gzATfe0ckNfYdik%3D&reserved=0
> >
> > I think on v3 you were asked to use a filename that matches the
> > compatibles?
> >
> >> +$schema:
> >> +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdev
> >> +icetree.org%2Fmeta-
> schemas%2Fcore.yaml%23&data=05%7C01%7Cpankaj.gupt
> >>
> +a%40nxp.com%7Cde3f4d12573845f8b7db08db8306b165%7C686ea1d3bc2
> b4c6fa92
> >>
> +cd99c5c301635%7C0%7C0%7C638247836986636866%7CUnknown%7CTW
> FpbGZsb3d8e
> >>
> +yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> %7C
> >>
> +3000%7C%7C%7C&sdata=tufHqsLbzUCQjGG9KzQ5dy1Htlk2gc2Ik5gEAFWym
> %2FI%3D
> >> +&reserved=0
> >> +
> >> +title: NXP i.MX EdgeLock Enclave Firmware (ELEFW)
> >> +
> >> +maintainers:
> >> +  - Pankaj Gupta <pankaj.gupta@nxp.com>
> >
> >> +  value, i.e., supported SoC(s) are imx8ulp, imx93.
> >
> >> +
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - fsl,imx-ele
> >
> > This looks like a generic compatible, not a specific one, but you use
> > it on the imx8ulp. I would have expected that you would have something
> > like "fsl,imx8ulp-ele" for that.
> 
> Yeah, this one looks generic, so not what we expect.

This change left un-changed in V4. It is "fsl,se-fw", instead of "fsl,imx8ulp-ele".
I will change in V5.

> 
> >
> >> +      - fsl,imx93-ele
> >
> >
> >> +
> >> +  mboxes:
> >> +    description:
> >> +      A list of phandles of TX MU channels followed by a list of phandles of
> >> +      RX MU channels. The number of expected tx and rx channels is 1 TX,
> and
> >> +      1 RX channels.
> 
> Don't repeat constraints in free form text. This is obvious from the items
> below.

Removed the line till here.

> 
> 
> Best regards,
> Krzysztof
Re: [EXT] Re: [PATCH v4 1/7] dt-bindings: arm: fsl: add se-fw binding doc
Posted by Conor Dooley 2 years, 6 months ago
On Mon, Jul 24, 2023 at 06:37:22AM +0000, Pankaj Gupta wrote:
> > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > On 12/07/2023 20:26, Conor Dooley wrote:
> > > On Wed, Jul 12, 2023 at 05:42:13PM +0530, Pankaj Gupta wrote:

> > >> +  value, i.e., supported SoC(s) are imx8ulp, imx93.

> > >> +
> > >> +properties:
> > >> +  compatible:
> > >> +    enum:
> > >> +      - fsl,imx-ele
> > >
> > > This looks like a generic compatible, not a specific one, but you use
> > > it on the imx8ulp. I would have expected that you would have something
> > > like "fsl,imx8ulp-ele" for that.
> > 
> > Yeah, this one looks generic, so not what we expect.
> 
> This change left un-changed in V4. It is "fsl,se-fw", instead of "fsl,imx8ulp-ele".
> I will change in V5.

That's a generic compatible too, so no different to "fsl,imx-ele".
What is the reason for avoiding the SoC-specific "fsl,imx8ulp-ele"?

> > >> +      - fsl,imx93-ele

RE: [EXT] Re: [PATCH v4 1/7] dt-bindings: arm: fsl: add se-fw binding doc
Posted by Pankaj Gupta 2 years, 6 months ago

> -----Original Message-----
> From: Conor Dooley <conor.dooley@microchip.com>
> Sent: Monday, July 24, 2023 12:18 PM
> To: Pankaj Gupta <pankaj.gupta@nxp.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; Conor Dooley
> <conor@kernel.org>; shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; clin@suse.com; conor+dt@kernel.org;
> pierre.gondois@arm.com; Jacky Bai <ping.bai@nxp.com>; Clark Wang
> <xiaoning.wang@nxp.com>; Wei Fang <wei.fang@nxp.com>; Peng Fan
> <peng.fan@nxp.com>; Bough Chen <haibo.chen@nxp.com>;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> davem@davemloft.net; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Gaurav Jain
> <gaurav.jain@nxp.com>; alexander.stein@ew.tq-group.com; Sahil Malhotra
> <sahil.malhotra@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>; Varun
> Sethi <V.Sethi@nxp.com>
> Subject: Re: [EXT] Re: [PATCH v4 1/7] dt-bindings: arm: fsl: add se-fw binding
> doc
> 
> On Mon, Jul 24, 2023 at 06:37:22AM +0000, Pankaj Gupta wrote:
> > > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> On
> > > 12/07/2023 20:26, Conor Dooley wrote:
> > > > On Wed, Jul 12, 2023 at 05:42:13PM +0530, Pankaj Gupta wrote:
> 
> > > >> +  value, i.e., supported SoC(s) are imx8ulp, imx93.
> 
> > > >> +
> > > >> +properties:
> > > >> +  compatible:
> > > >> +    enum:
> > > >> +      - fsl,imx-ele
> > > >
> > > > This looks like a generic compatible, not a specific one, but you
> > > > use it on the imx8ulp. I would have expected that you would have
> > > > something like "fsl,imx8ulp-ele" for that.
> > >
> > > Yeah, this one looks generic, so not what we expect.
> >
> > This change left un-changed in V4. It is "fsl,se-fw", instead of "fsl,imx8ulp-
> ele".
> > I will change in V5.

> 
> That's a generic compatible too, so no different to "fsl,imx-ele".
> What is the reason for avoiding the SoC-specific "fsl,imx8ulp-ele"?

Sorry. I missed this point.
Not trying to avoid the SoC specific compatible. I will add the soc id to make the compatible = "fsl,se-8ulpfw", instead of "fsl,se-fw".

Thanks for pointing out here.

> 
> > > >> +      - fsl,imx93-ele
Re: [PATCH v4 1/7] dt-bindings: arm: fsl: add se-fw binding doc
Posted by Krzysztof Kozlowski 2 years, 7 months ago
On 12/07/2023 14:12, Pankaj Gupta wrote:
> The NXP's i.MX EdgeLock Enclave, a HW IP creating an embedded
> secure enclave within the SoC boundary to enable features like
> - HSM
> - SHE
> - V2X
> 
> Communicates via message unit with linux kernel. This driver
> is enables communication ensuring well defined message sequence
> protocol between Application Core and enclave's firmware.
> 
> Driver configures multiple misc-device on the MU, for multiple
> user-space applications can communicate on single MU.
> 
> It exists on some i.MX processors. e.g. i.MX8ULP, i.MX93 etc.
> 
> Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
> ---
>  .../bindings/arm/freescale/fsl,se-fw.yaml     | 121 ++++++++++++++++++
>  1 file changed, 121 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml b/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
> new file mode 100644
> index 000000000000..7567da0b4c21
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
> @@ -0,0 +1,121 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/freescale/fsl,se-fw.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP i.MX EdgeLock Enclave Firmware (ELEFW)
> +
> +maintainers:
> +  - Pankaj Gupta <pankaj.gupta@nxp.com>
> +
> +description: |
> +
> +  The NXP's i.MX EdgeLock Enclave, a HW IP creating an embedded
> +  secure enclave within the SoC boundary to enable features like
> +  - HSM
> +  - SHE
> +  - V2X
> +
> +  It uses message unit to communicate and coordinate to pass messages
> +  (e.g., data,  status and control) through its interfaces.
> +
> +  This driver configures multiple misc-devices on the MU, to exchange
> +  messages from User-space application and NXP's Edgelocke Enclave firmware.
> +  The driver ensures that the messages must follow the following protocol
> +  defined.
> +
> +                                     Non-Secure           +   Secure
> +                                                          |
> +                                                          |
> +                   +---------+      +-------------+       |
> +                   | ele_mu.c+<---->+imx-mailbox.c|       |
> +                   |         |      |  mailbox.c  +<-->+------+    +------+
> +                   +---+-----+      +-------------+    | MU X +<-->+ ELE |
> +                       |                               +------+    +------+
> +                       +----------------+                 |
> +                       |                |                 |
> +                       v                v                 |
> +                   logical           logical              |
> +                   receiver          waiter               |
> +                      +                 +                 |
> +                      |                 |                 |
> +                      |                 |                 |
> +                      |            +----+------+          |
> +                      |            |           |          |
> +                      |            |           |          |
> +               device_ctx     device_ctx     device_ctx   |
> +                                                          |
> +                 User 0        User 1       User Y        |
> +                 +------+      +------+     +------+      |
> +                 |misc.c|      |misc.c|     |misc.c|      |
> +  kernel space   +------+      +------+     +------+      |
> +                                                          |
> +  +------------------------------------------------------ |
> +                     |             |           |          |
> +  userspace     /dev/ele_muXch0    |           |          |
> +                           /dev/ele_muXch1     |          |
> +                                         /dev/ele_muXchY  |
> +                                                          |
> +
> +  When a user sends a command to the ELE, it registers its device_ctx as
> +  waiter of a response from ELE.
> +
> +  A user can be registered as receiver of command from the ELE.
> +  Create char devices in /dev as channels of the form /dev/ele_muXchY with X
> +  the id of the driver and Y for each users. It allows to send and receive
> +  messages to the NXP EdgeLock Enclave IP on NXP SoC, where current possible
> +  value, i.e., supported SoC(s) are imx8ulp, imx93.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - fsl,imx-ele
> +      - fsl,imx93-ele
> +
> +  mboxes:
> +    description:
> +      A list of phandles of TX MU channels followed by a list of phandles of
> +      RX MU channels. The number of expected tx and rx channels is 1 TX, and
> +      1 RX channels. All MU channels must be within the same MU instance.
> +      Cross instances are not allowed. The MU instance to be used is S4MUAP
> +      for imx8ulp & imx93. Users need to ensure that used MU instance does not
> +      conflict with other execution environments.
> +    items:
> +      - description: TX0 MU channel
> +      - description: RX0 MU channel
> +
> +  mbox-names:
> +    items:
> +      - const: tx
> +      - const: rx
> +
> +  fsl,mu-did:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Owner of message-unit, is identified via Domain ID or did.

What is Domain ID?

> +
> +  fsl,mu-id:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Identifier to the message-unit among the multiple message-unit that exists on SoC.
> +      It is used to create the channels, default to 2

Do you expect then multiple ele nodes in the DTS? What are these two
properties and why they are fixed per SoC, but still embedded in DTS?

> +
> +

Drop stray blank line.

> +required:
> +  - compatible
> +  - mboxes
> +  - mbox-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    ele_mu: ele_mu {

No underscores in node names, generic node names, e.g. firmware. Look at
existing code.

> +      compatible = "fsl,imx93-ele";
> +      mbox-names = "tx", "rx";
> +      mboxes = <&s4muap 2 0
> +                &s4muap 3 0>;

Two items, not one.

> +      fsl,mu-did = <1>;
> +      fsl,mu-id = <1>;
> +    };

Plus you clearly did not test the binding and DTS. You said you did some
internal review, so I assume this also includes some testing. How did
you test your DTS?

Best regards,
Krzysztof
RE: [EXT] Re: [PATCH v4 1/7] dt-bindings: arm: fsl: add se-fw binding doc
Posted by Pankaj Gupta 2 years, 6 months ago

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Thursday, July 13, 2023 12:09 AM
> To: Pankaj Gupta <pankaj.gupta@nxp.com>; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; clin@suse.com;
> conor+dt@kernel.org; pierre.gondois@arm.com; Jacky Bai
> <ping.bai@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; Wei Fang
> <wei.fang@nxp.com>; Peng Fan <peng.fan@nxp.com>; Bough Chen
> <haibo.chen@nxp.com>; festevam@gmail.com; dl-linux-imx <linux-
> imx@nxp.com>; davem@davemloft.net; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Gaurav Jain
> <gaurav.jain@nxp.com>; alexander.stein@ew.tq-group.com; Sahil Malhotra
> <sahil.malhotra@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>; Varun
> Sethi <V.Sethi@nxp.com>
> Subject: [EXT] Re: [PATCH v4 1/7] dt-bindings: arm: fsl: add se-fw binding doc
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> On 12/07/2023 14:12, Pankaj Gupta wrote:
> > The NXP's i.MX EdgeLock Enclave, a HW IP creating an embedded secure
> > enclave within the SoC boundary to enable features like
> > - HSM
> > - SHE
> > - V2X
> >
> > Communicates via message unit with linux kernel. This driver is
> > enables communication ensuring well defined message sequence protocol
> > between Application Core and enclave's firmware.
> >
> > Driver configures multiple misc-device on the MU, for multiple
> > user-space applications can communicate on single MU.
> >
> > It exists on some i.MX processors. e.g. i.MX8ULP, i.MX93 etc.
> >
> > Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
> > ---
> >  .../bindings/arm/freescale/fsl,se-fw.yaml     | 121 ++++++++++++++++++
> >  1 file changed, 121 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
> > b/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
> > new file mode 100644
> > index 000000000000..7567da0b4c21
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,se-fw.yaml
> > @@ -0,0 +1,121 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fschemas%2Farm%2Ffreescale%2Ffsl%2Cse-
> fw.yaml%23&data=05%
> >
> +7C01%7Cpankaj.gupta%40nxp.com%7Cfd14adabdde046302b1908db83073
> a2d%7C68
> >
> +6ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638247839285279031
> %7CUnknown
> >
> +%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> WwiLC
> >
> +JXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vUn1TrC%2Fk2f%2B5do%2FoK
> OSu7cWDl3s
> > +6BddlvIIO%2FxZGMA%3D&reserved=0
> > +$schema:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fmeta-
> schemas%2Fcore.yaml%23&data=05%7C01%7Cpankaj.gupta%
> >
> +40nxp.com%7Cfd14adabdde046302b1908db83073a2d%7C686ea1d3bc2b4
> c6fa92cd9
> >
> +9c5c301635%7C0%7C0%7C638247839285279031%7CUnknown%7CTWFpb
> GZsb3d8eyJWI
> >
> +joiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3
> 000%
> >
> +7C%7C%7C&sdata=9sbFCHNNCHHsjLbZ%2BHQls3ZrXKF%2BhbpizZhIxfvytlo%
> 3D&res
> > +erved=0
> > +
> > +title: NXP i.MX EdgeLock Enclave Firmware (ELEFW)
> > +
> > +maintainers:
> > +  - Pankaj Gupta <pankaj.gupta@nxp.com>
> > +
> > +description: |
> > +
> > +  The NXP's i.MX EdgeLock Enclave, a HW IP creating an embedded
> > + secure enclave within the SoC boundary to enable features like
> > +  - HSM
> > +  - SHE
> > +  - V2X
> > +
> > +  It uses message unit to communicate and coordinate to pass messages
> > + (e.g., data,  status and control) through its interfaces.
> > +
> > +  This driver configures multiple misc-devices on the MU, to exchange
> > + messages from User-space application and NXP's Edgelocke Enclave
> firmware.
> > +  The driver ensures that the messages must follow the following
> > + protocol  defined.
> > +
> > +                                     Non-Secure           +   Secure
> > +                                                          |
> > +                                                          |
> > +                   +---------+      +-------------+       |
> > +                   | ele_mu.c+<---->+imx-mailbox.c|       |
> > +                   |         |      |  mailbox.c  +<-->+------+    +------+
> > +                   +---+-----+      +-------------+    | MU X +<-->+ ELE |
> > +                       |                               +------+    +------+
> > +                       +----------------+                 |
> > +                       |                |                 |
> > +                       v                v                 |
> > +                   logical           logical              |
> > +                   receiver          waiter               |
> > +                      +                 +                 |
> > +                      |                 |                 |
> > +                      |                 |                 |
> > +                      |            +----+------+          |
> > +                      |            |           |          |
> > +                      |            |           |          |
> > +               device_ctx     device_ctx     device_ctx   |
> > +                                                          |
> > +                 User 0        User 1       User Y        |
> > +                 +------+      +------+     +------+      |
> > +                 |misc.c|      |misc.c|     |misc.c|      |
> > +  kernel space   +------+      +------+     +------+      |
> > +                                                          |
> > +  +------------------------------------------------------ |
> > +                     |             |           |          |
> > +  userspace     /dev/ele_muXch0    |           |          |
> > +                           /dev/ele_muXch1     |          |
> > +                                         /dev/ele_muXchY  |
> > +                                                          |
> > +
> > +  When a user sends a command to the ELE, it registers its device_ctx
> > + as  waiter of a response from ELE.
> > +
> > +  A user can be registered as receiver of command from the ELE.
> > +  Create char devices in /dev as channels of the form /dev/ele_muXchY
> > + with X  the id of the driver and Y for each users. It allows to send
> > + and receive  messages to the NXP EdgeLock Enclave IP on NXP SoC,
> > + where current possible  value, i.e., supported SoC(s) are imx8ulp, imx93.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - fsl,imx-ele
> > +      - fsl,imx93-ele
> > +
> > +  mboxes:
> > +    description:
> > +      A list of phandles of TX MU channels followed by a list of phandles of
> > +      RX MU channels. The number of expected tx and rx channels is 1 TX,
> and
> > +      1 RX channels. All MU channels must be within the same MU instance.
> > +      Cross instances are not allowed. The MU instance to be used is S4MUAP
> > +      for imx8ulp & imx93. Users need to ensure that used MU instance does
> not
> > +      conflict with other execution environments.
> > +    items:
> > +      - description: TX0 MU channel
> > +      - description: RX0 MU channel
> > +
> > +  mbox-names:
> > +    items:
> > +      - const: tx
> > +      - const: rx
> > +
> > +  fsl,mu-did:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Owner of message-unit, is identified via Domain ID or did.
> 
> What is Domain ID?

By design, Domain is a clean separated processing island with separate power,
      clocking and peripheral; but with a tightly integrated bus fabric for efficient
      communication. The Domain to which this message-unit is associated, is identified
      via Domain ID or did.

I will update this info in the YAML file too.

> 
> > +
> > +  fsl,mu-id:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Identifier to the message-unit among the multiple message-unit that
> exists on SoC.
> > +      It is used to create the channels, default to 2
> 
> Do you expect then multiple ele nodes in the DTS? What are these two
> properties and why they are fixed per SoC, but still embedded in DTS?

Removed this line.

> 
> > +
> > +
> 
> Drop stray blank line.
> 

Removed the blank line.

> > +required:
> > +  - compatible
> > +  - mboxes
> > +  - mbox-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    ele_mu: ele_mu {
> 
> No underscores in node names, generic node names, e.g. firmware. Look at
> existing code.

Changed from:
-  ele_mu to ele-mu.
- "ele_mu {" to "se-fw {"

Name of yaml file, is se-fw.yaml.

> 
> > +      compatible = "fsl,imx93-ele";
> > +      mbox-names = "tx", "rx";
> > +      mboxes = <&s4muap 2 0
> > +                &s4muap 3 0>;
> 
> Two items, not one.

Corrected it to "mboxes= = <&s4muap 2 0 &s4muap 3 0>;"

> 
> > +      fsl,mu-did = <1>;
> > +      fsl,mu-id = <1>;
> > +    };
> 
> Plus you clearly did not test the binding and DTS. You said you did some
> internal review, so I assume this also includes some testing. How did you test
> your DTS?
> 

Each version is tested before sent for review here.
I have tested the DTS file by compiling it and loading the DTB to the board.
Executed test on the board. 


> Best regards,
> Krzysztof
Re: [EXT] Re: [PATCH v4 1/7] dt-bindings: arm: fsl: add se-fw binding doc
Posted by Krzysztof Kozlowski 2 years, 6 months ago
On 24/07/2023 08:37, Pankaj Gupta wrote:
>>> +examples:
>>> +  - |
>>> +    ele_mu: ele_mu {
>>
>> No underscores in node names, generic node names, e.g. firmware. Look at
>> existing code.
> 
> Changed from:
> -  ele_mu to ele-mu.
> - "ele_mu {" to "se-fw {"

Still not generic. Why do you change it twice? You understand I talk
here about node name?


> 
> Name of yaml file, is se-fw.yaml.
> 
>>
>>> +      compatible = "fsl,imx93-ele";
>>> +      mbox-names = "tx", "rx";
>>> +      mboxes = <&s4muap 2 0
>>> +                &s4muap 3 0>;
>>
>> Two items, not one.
> 
> Corrected it to "mboxes= = <&s4muap 2 0 &s4muap 3 0>;"
> 
>>
>>> +      fsl,mu-did = <1>;
>>> +      fsl,mu-id = <1>;
>>> +    };
>>
>> Plus you clearly did not test the binding and DTS. You said you did some
>> internal review, so I assume this also includes some testing. How did you test
>> your DTS?
>>
> 
> Each version is tested before sent for review here.
> I have tested the DTS file by compiling it and loading the DTB to the board.
> Executed test on the board. 

That's not enough and your colleagues should tell you that... Read our
guides for bindings.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.


Best regards,
Krzysztof