Re: [PATCH v2 1/7] doc: device tree binding addition for ele MU

[Krzysztof Kozlowski]

On 19/04/2023 19:55, Pankaj Gupta wrote:
> Documentation update with addition of new device tree
> for NXP ele-mu (Edgelock Enclave Message Unit), driver.


Your patchset is sent with wrong clock. Fix your system because now it
goes before all other patchsets...

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

Subject: drop second/last, redundant "bindings". The "dt-bindings"
prefix is already stating that these are bindings.


> 
> Signed-off-by: Pankaj Gupta <pankaj.gupta@xxxxxxx>
> ---
>  .../bindings/arm/freescale/fsl,ele_mu.yaml    | 139 ++++++++++++++++++
>  1 file changed, 139 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,ele_mu.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,ele_mu.yaml b/Documentation/devicetree/bindings/arm/freescale/fsl,ele_mu.yaml
> new file mode 100644
> index 000000000000..8c4cc32f62ab
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,ele_mu.yaml

Underscores are not allowed.

> @@ -0,0 +1,139 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/freescale/fsl,ele_mu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP i.MX EdgeLock Enclave MUAP driver

Drop driver.

> +
> +maintainers:
> +  - Pankaj Gupta <pankaj.gupta@xxxxxxx>
> +
> +description: |
> +

Drop stray blank line

> +  NXP i.MX EdgeLock Enclave Message Unit Driver.

Drop driver and rephrase it to describe hardware.

> +  The Messaging Unit module enables two processing elements within the SoC to
> +  communicate and coordinate by passing messages (e.g., data, status and control)
> +  through its interfaces.
> +
> +  The NXP i.MX EdgeLock Enclave Message Unit (ELE-MUAP) is specifically targeted
> +  for use between application core and Edgelocke Enclave. It allows to send
> +  messages to the EL Enclave using a shared mailbox.
> +
> +  The messages must follow the 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:
> +      List of <&phandle type channel> - 4 channels for TX, 4 channels for RX,
> +      1 channel for TXDB
> +    maxItems: 9
> +
> +  mbox-names:
> +    items:
> +      - const: tx
> +      - const: rx

This does not make sense. You have 9 items, not two. Unless you want
some customization.

> +
> +  fsl,ele_mu_did:

Underscores are not allowed.

> +    description:
> +      Owner of message-unit, is identified via Domain ID or did.
> +    allOf:

That's not how they this is written. Drop allOf. Please start from
example-schema.

Anyway, half of the properties here do not look like hardware
properties. You need to explain why these are board specific.

> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - enum: [0, 1, 2, 3, 4, 5, 6, 7]
> +
> +  fsl,ele_mu_id:
> +    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
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - enum: [0, 1, 2, 3]
> +
> +  fsl,ele_max_users:
> +    description:
> +      Number of misclleneous devices to be created, default to 4
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
> +
> +  fsl,cmd_tag:
> +    description:
> +      Tag in message header for commands on this MU, default to 0x17

Don't repeat constraints in free form text. Use default:

> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint8
> +      - enum: [0x17, 0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e]
> +
> +  fsl,rsp_tag:
> +    description:
> +      Tag in message header for responses on this MU, default to 0xe1
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint8
> +      - enum: [0xe1, 0xe2, 0xe3, 0xe4, 0xe5, 0xe6, 0xe7, 0xe8]
> +
> +required:
> +  - compatible
> +  - mboxes
> +  - mbox-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    ele_mu: ele_mu {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Also, underscores are not allowed iin node names.

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

Fix indentation and number of items. These should be separate phandles,
not one phandle.

> +
> +      fsl,ele_mu_id = <1>;
> +      fsl,ele_max_users = <4>;
> +      fsl,cmd_tag = /bits/ 8 <0x17>;
> +      fsl,rsp_tag = /bits/ 8 <0xe1>;
> +    };

Best regards,
Krzysztof