Message-ID: <82eae461-3cce-4e36-905c-34c147fabcb3@microchip.com>
Date: Tue, 15 Oct 2024 12:09:08 +0000
From: <Valentina.FernandezAlanis@...rochip.com>
To: <krzk@...nel.org>, <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 16/09/2024 21:14, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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.
IPC is the actual name of the hardware block described in this binding. 
I'll update the description of the binding in v2 to mention this.

Additionally, I'll rename the compatible to microchip,ipc-sbi-remoteproc 
to further clarify that this binding is intended for devices using the 
Microchip IPC hardware block and for devices with an SBI interface (RISC-V).

Thanks,
Valentina
> 
> 
> 
>> +
>> +  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