Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors

Krzysztof Kozlowski
2024-08-23T06:24:54Z

On Thu, Aug 22, 2024 at 06:09:47PM +0100, Abdellatif El Khlifi wrote:
> Add devicetree binding schema for the External Systems remote processors
>
> The External Systems remote processors are provided on the Corstone-1000
> IoT Reference Design Platform via the SSE-710 subsystem.
>
> For more details about the External Systems, please see Corstone SSE-710
> subsystem features [1].
>

Do not attach (thread) your patchsets to some other threads (unrelated
or older versions). This buries them deep in the mailbox and might
interfere with applying entire sets.

> [1]: https://developer.arm.com/documentation/102360/0000/Overview-of-Corstone-1000/Corstone-SSE-710-subsystem-features
>
> Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> ---
> .../remoteproc/arm,sse710-extsys.yaml | 90 +++++++++++++++++++
> 1 file changed, 90 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
>
> diff --git a/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml b/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
> new file mode 100644
> index 000000000000..827ba8d962f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/arm,sse710-extsys.yaml
> @@ -0,0 +1,90 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/arm,sse710-extsys.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SSE-710 External System Remote Processor
> +
> +maintainers:
> + - Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> + - Hugues Kamba Mpiana <hugues.kambampiana@arm.com>
> +
> +description: |

dt-preserve-formatting

> + SSE-710 is an heterogeneous subsystem supporting up to two remote
> + processors aka the External Systems.
> +
> +properties:
> + compatible:
> + enum:
> + - arm,sse710-extsys
> +
> + firmware-name:
> + description:
> + The default name of the firmware to load to the remote processor.
> +
> + '#extsys-id':

'#' is not correct for sure, that's not a cell specifier.

But anyway, we do not accept in general instance IDs.

> + description:
> + The External System ID.

This tells me nothing. You basically copied property name.

> + enum: [0, 1]
> +
> + mbox-names:
> + items:
> + - const: txes0
> + - const: rxes0
> +
> + mboxes:
> + description:
> + The list of Message Handling Unit (MHU) channels used for bidirectional
> + communication. This property is only required if the virtio-based Rpmsg
> + messaging bus is used. For more details see the Arm MHUv2 Mailbox
> + Controller at devicetree/bindings/mailbox/arm,mhuv2.yaml
> +

Drop blank line

> + minItems: 2

This is redundant if equals to maxItemns, drop.

> + maxItems: 2
> +
> + memory-region:
> + description:
> + If present, a phandle for a reserved memory area that used for vdev
> + buffer, resource table, vring region and others used by the remote
> + processor.
> + minItems: 2
> + maxItems: 32
> +
> +required:
> + - compatible
> + - firmware-name
> + - '#extsys-id'
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + reserved-memory {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + extsys0_vring0: vdev0vring0@82001000 {
> + reg = <0 0x82001000 0 0x8000>;
> + no-map;
> + };
> +
> + extsys0_vring1: vdev0vring1@82009000 {
> + reg = <0 0x82009000 0 0x8000>;
> + no-map;
> + };
> + };

Drop, it is fairly common.

> +
> + syscon@1a010000 {
> + compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
> + reg = <0x1a010000 0x1000>;

So this is a part of other block? Then make one complete example in the
parent device bindings.

> +
> + extsys0 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
e.g. remoteproc

> + compatible = "arm,sse710-extsys";
> + #extsys-id = <0>;
> + firmware-name = "es_flashfw.elf";
> + mbox-names = "txes0", "rxes0";
> + mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>;

First go mboxes, then mbox-names. The same in the binding, BTW.

Best regards,
Krzysztof