Message-ID: <fc541e78-5304-42be-a844-70935d66f151@kernel.org>
Date: Mon, 16 Sep 2024 22:14:15 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Valentina Fernandez <valentina.fernandezalanis@...rochip.com>,
 paul.walmsley@...ive.com, palmer@...belt.com, aou@...s.berkeley.edu,
 peterlin@...estech.com, dminus@...estech.com, conor.dooley@...rochip.com,
 conor+dt@...nel.org, ycliang@...estech.com, jassisinghbrar@...il.com,
 robh@...nel.org, krzk+dt@...nel.org, andersson@...nel.org,
 mathieu.poirier@...aro.org
Cc: linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH v1 4/5] dt-bindings: remoteproc: add binding for Microchip
 IPC remoteproc

On 12/09/2024 19:00, Valentina Fernandez wrote:
> Microchip family of RISC-V SoCs typically has or more clusters. These
> clusters can be configured to run in Asymmetric Multi Processing (AMP)
> mode

A nit, subject: drop second/last, redundant "binding for". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> 
> Add a dt-binding for the Microchip IPC Remoteproc platform driver.
> 

Binding is for hardware, not driver. Please rephrase it to describe
hardware.


> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@...rochip.com>
> ---
>  .../remoteproc/microchip,ipc-remoteproc.yaml  | 84 +++++++++++++++++++
>  1 file changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
> new file mode 100644
> index 000000000000..1765c68d22cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
> @@ -0,0 +1,84 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-remoteproc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip IPC Remote Processor
> +
> +description:
> +  Microchip family of RISC-V SoCs typically have one or more
> +  clusters. These clusters can be configured to run in an Asymmetric
> +  Multi Processing (AMP) mode where clusters are split in independent
> +  software contexts.
> +
> +  This document defines the binding for the remoteproc component that
> +  loads and boots firmwares on remote clusters.

Don't say that binding is a binding for. Say what this hardware piece is.

> +
> +  This SBI interface is compatible with the Mi-V Inter-hart
> +  Communication (IHC) IP.
> +
> +maintainers:
> +  - Valentina Fernandez <valentina.fernandezalanis@...rochip.com>
> +
> +properties:
> +  compatible:
> +    const: microchip,ipc-remoteproc

That's quite generic. Basically this says it will handle IPC of all
possible Microchip SoCs, not only RISC-V but also ARM and whatever you
come up with.



> +
> +  mboxes:
> +    description:
> +      This property is required only if the rpmsg/virtio functionality is used.
> +      Microchip IPC mailbox specifier. To be used for communication with a
> +      remote cluster. The specifier format is as per the bindings,
> +      Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> +    maxItems: 1
> +
> +  microchip,auto-boot:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      If defined, when remoteproc is probed, it loads the default firmware and
> +      starts the remote processor.

You described the desired Linux feature or behavior, not the actual
hardware. The bindings are about the latter, so instead you need to
rephrase the property and its description to match actual hardware
capabilities/features/configuration etc.

> +
> +  microchip,skip-ready-wait:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      If defined, the master processor will not expect a ready signal from the
> +      remote processor indicating it has booted successfully. This allows the
> +      master processor to proceed with its operations without waiting for
> +      confirmation from the remote processor.
Same problem.


> +
> +  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 remote cluster.

missing constraints

> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +

Drop blank line

> +    reserved-memory {
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +
> +        contextb: contextb_reserved@...00000 {
> +          reg = <0x81000000 0x400000>;
> +          no-map;
> +        };
> +    };

Drop entire reserved-node. Obvious.

> +
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      rproc-contextb {

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

usually remoteproc

> +          compatible = "microchip,ipc-remoteproc";
> +          memory-region = <&contextb>;
> +          mboxes= <&ihc 8>;

Make the binding complete. Fix the white-space issues.

> +      };
> +    };
> +
> +...

Best regards,
Krzysztof

Powered by blists - more mailing lists