diff mbox series

[v16,4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings

Message ID 20200922223930.4710-5-ben.levinsky@xilinx.com (mailing list archive)
State Superseded
Headers show
Series Provide basic driver to control Arm R5 co-processor found on Xilinx ZynqMP | expand

Commit Message

Ben Levinsky Sept. 22, 2020, 10:39 p.m. UTC
Add binding for ZynqMP R5 OpenAMP.

Represent the RPU domain resources in one device node. Each RPU
processor is a subnode of the top RPU domain node.

Signed-off-by: Jason Wu <j.wu@xilinx.com>
Signed-off-by: Wendy Liang <jliang@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
---
v3:
- update zynqmp_r5 yaml parsing to not raise warnings for extra
  information in children of R5 node. The warning "node has a unit
  name, but no reg or ranges property" will still be raised though 
  as this particular node is needed to describe the
  '#address-cells' and '#size-cells' information.
v4::
- remove warning '/example-0/rpu@ff9a0000/r5@0: 
  node has a unit name, but no reg or ranges property'
  by adding reg to r5 node.
v5:
- update device tree sample and yaml parsing to not raise any warnings
- description for memory-region in yaml parsing
- compatible string in yaml parsing for TCM
v6:
- remove coupling TCM nodes with remoteproc 
- remove mailbox as it is optional not needed
v7:
- change lockstep-mode to xlnx,cluster-mode
v9:
- show example IPC nodes and tcm bank nodes
v11:
- add property meta-memory-regions to illustrate link
  between r5 and TCM banks
- update so no warnings from 'make dt_binding_check'
v14:
- concerns were raised about the new property meta-memory-regions.
  There is no clear direction so for the moment I kept it in the series
- place IPC nodes in RAM in the reserved memory section
v15:
- change lockstep-mode prop as follows: if present, then RPU cluster is in
  lockstep mode. if not present, cluster is in split mode.
---
 .../xilinx,zynqmp-r5-remoteproc.yaml          | 120 ++++++++++++++++++
 1 file changed, 120 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml

Comments

Rob Herring Sept. 29, 2020, 6:36 p.m. UTC | #1
On Tue, Sep 22, 2020 at 03:39:29PM -0700, Ben Levinsky wrote:
> Add binding for ZynqMP R5 OpenAMP.
> 
> Represent the RPU domain resources in one device node. Each RPU
> processor is a subnode of the top RPU domain node.
> 
> Signed-off-by: Jason Wu <j.wu@xilinx.com>
> Signed-off-by: Wendy Liang <jliang@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
> ---
> v3:
> - update zynqmp_r5 yaml parsing to not raise warnings for extra
>   information in children of R5 node. The warning "node has a unit
>   name, but no reg or ranges property" will still be raised though 
>   as this particular node is needed to describe the
>   '#address-cells' and '#size-cells' information.
> v4::
> - remove warning '/example-0/rpu@ff9a0000/r5@0: 
>   node has a unit name, but no reg or ranges property'
>   by adding reg to r5 node.
> v5:
> - update device tree sample and yaml parsing to not raise any warnings
> - description for memory-region in yaml parsing
> - compatible string in yaml parsing for TCM
> v6:
> - remove coupling TCM nodes with remoteproc 
> - remove mailbox as it is optional not needed
> v7:
> - change lockstep-mode to xlnx,cluster-mode
> v9:
> - show example IPC nodes and tcm bank nodes
> v11:
> - add property meta-memory-regions to illustrate link
>   between r5 and TCM banks
> - update so no warnings from 'make dt_binding_check'
> v14:
> - concerns were raised about the new property meta-memory-regions.
>   There is no clear direction so for the moment I kept it in the series
> - place IPC nodes in RAM in the reserved memory section
> v15:
> - change lockstep-mode prop as follows: if present, then RPU cluster is in
>   lockstep mode. if not present, cluster is in split mode.
> ---
>  .../xilinx,zynqmp-r5-remoteproc.yaml          | 120 ++++++++++++++++++
>  1 file changed, 120 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
> new file mode 100644
> index 000000000000..ce02e425692e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
> @@ -0,0 +1,120 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Xilinx R5 remote processor controller bindings
> +
> +description:
> +  This document defines the binding for the remoteproc component that loads and
> +  boots firmwares on the Xilinx Zynqmp and Versal family chipset.
> +
> +  Note that the Linux has global addressing view of the R5-related memory (TCM)
> +  so the absolute address ranges are provided in TCM reg's.

blank line needed.

TCMs specifically I'm concerned about how they are represented in system 
DT and here...

> +maintainers:
> +  - Ed Mooring <ed.mooring@xilinx.com>
> +  - Ben Levinsky <ben.levinsky@xilinx.com>
> +
> +properties:
> +  compatible:
> +    const: "xlnx,zynqmp-r5-remoteproc-1.0"

Don't need quotes.

The use of version numbers was allowed for Xilinx programmable IP. I 
don't think this falls into that category.

> +
> +  lockstep-mode:
> +    description:
> +      If this property is present, then the configuration is lock-step.

boolean...

> +      Otherwise RPU is split.
> +    maxItems: 1

...or an array?

Either way, doesn't work for TI R5.

> +
> +  interrupts:
> +    description:
> +      Interrupt mapping for remoteproc IPI. It is required if the
> +      user uses the remoteproc driver with the RPMsg kernel driver.
> +    maxItems: 6
> +
> +  memory-region:
> +    description:
> +      collection of memory carveouts used for elf-loading and inter-processor
> +      communication.
> +    maxItems: 4
> +    minItems: 4

Need to define what each region is.

One blank line between properties.

> +  meta-memory-regions:
> +    description:
> +      collection of memories that are not present in the top level memory
> +      nodes' mapping. For example, R5s' TCM banks. These banks are needed
> +      for R5 firmware meta data such as the R5 firmware's heap and stack

Humm, needs a better explanation.

> +  pnode-id:
> +    maxItems: 1

What's this?

> +  mboxes:
> +    maxItems: 2

Need to define what each one is.

> +  mbox-names:
> +    maxItems: 2

Need to define the names.

> +
> +examples:
> +  - |
> +     reserved-memory {
> +          #address-cells = <1>;
> +          #size-cells = <1>;
> +          ranges;
> +          elf_load: rproc@3ed000000 {
> +               no-map;
> +               reg = <0x3ed00000 0x40000>;
> +          };
> +
> +          rpu0vdev0vring0: rpu0vdev0vring0@3ed40000 {
> +               no-map;
> +               reg = <0x3ed40000 0x4000>;
> +          };
> +          rpu0vdev0vring1: rpu0vdev0vring1@3ed44000 {
> +               no-map;
> +               reg = <0x3ed44000 0x4000>;
> +          };
> +          rpu0vdev0buffer: rpu0vdev0buffer@3ed48000 {
> +               no-map;
> +               reg = <0x3ed48000 0x100000>;
> +          };
> +
> +     };
> +
> +     /*
> +      * Below nodes are required if using TCM to load R5 firmware
> +      * if not, then either do not provide nodes are label as disabled in
> +      * status property
> +      */
> +     tcm0a: tcm_0a@ffe00000 {
> +         reg = <0xffe00000 0x10000>;
> +         pnode-id = <0xf>;
> +         no-map;
> +         status = "okay";
> +         phandle = <0x40>;
> +         compatible = "xlnx,tcm";

TCM is a Xilinx specific thing?

> +     };
> +     tcm0b: tcm_1a@ffe20000 {
> +         reg = <0xffe20000 0x10000>;
> +         pnode-id = <0x10>;
> +         no-map;
> +         status = "okay";
> +         compatible = "xlnx,tcm";
> +         phandle = <0x41>;
> +     };
> +
> +     rpu {
> +          compatible = "xlnx,zynqmp-r5-remoteproc-1.0";
> +          #address-cells = <1>;
> +          #size-cells = <1>;
> +          ranges;
> +          lockstep-mode;
> +          r5_0 {
> +               ranges;
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               memory-region = <&elf_load>,
> +                               <&rpu0vdev0vring0>,
> +                               <&rpu0vdev0vring1>,
> +                               <&rpu0vdev0buffer>;
> +               meta-memory-regions = <&tcm_0a>, <&tcm_0b>;
> +               pnode-id = <0x7>;
> +          };
> +     };
> +
> +...
> -- 
> 2.17.1
>
Ben Levinsky Sept. 30, 2020, 4:21 p.m. UTC | #2
Hi Rob,

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Tuesday, September 29, 2020 11:36 AM
> To: Ben Levinsky <BLEVINSK@xilinx.com>
> Cc: Stefano Stabellini <stefanos@xilinx.com>; Michal Simek
> <michals@xilinx.com>; michael.auchter@ni.com; devicetree@vger.kernel.org;
> mathieu.poirier@linaro.org; Ed T. Mooring <emooring@xilinx.com>; linux-
> remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; Jason Wu <j.wu@xilinx.com>; Jiaying Liang
> <jliang@xilinx.com>; Michal Simek <michals@xilinx.com>
> Subject: Re: [PATCH v16 4/5] dt-bindings: remoteproc: Add documentation for
> ZynqMP R5 rproc bindings
> 
> On Tue, Sep 22, 2020 at 03:39:29PM -0700, Ben Levinsky wrote:
> > Add binding for ZynqMP R5 OpenAMP.
> >
> > Represent the RPU domain resources in one device node. Each RPU
> > processor is a subnode of the top RPU domain node.
> >
> > Signed-off-by: Jason Wu <j.wu@xilinx.com>
> > Signed-off-by: Wendy Liang <jliang@xilinx.com>
> > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
> > ---
> > v3:
> > - update zynqmp_r5 yaml parsing to not raise warnings for extra
> >   information in children of R5 node. The warning "node has a unit
> >   name, but no reg or ranges property" will still be raised though
> >   as this particular node is needed to describe the
> >   '#address-cells' and '#size-cells' information.
> > v4::
> > - remove warning '/example-0/rpu@ff9a0000/r5@0:
> >   node has a unit name, but no reg or ranges property'
> >   by adding reg to r5 node.
> > v5:
> > - update device tree sample and yaml parsing to not raise any warnings
> > - description for memory-region in yaml parsing
> > - compatible string in yaml parsing for TCM
> > v6:
> > - remove coupling TCM nodes with remoteproc
> > - remove mailbox as it is optional not needed
> > v7:
> > - change lockstep-mode to xlnx,cluster-mode
> > v9:
> > - show example IPC nodes and tcm bank nodes
> > v11:
> > - add property meta-memory-regions to illustrate link
> >   between r5 and TCM banks
> > - update so no warnings from 'make dt_binding_check'
> > v14:
> > - concerns were raised about the new property meta-memory-regions.
> >   There is no clear direction so for the moment I kept it in the series
> > - place IPC nodes in RAM in the reserved memory section
> > v15:
> > - change lockstep-mode prop as follows: if present, then RPU cluster is in
> >   lockstep mode. if not present, cluster is in split mode.
> > ---
> >  .../xilinx,zynqmp-r5-remoteproc.yaml          | 120 ++++++++++++++++++
> >  1 file changed, 120 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-
> remoteproc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-
> r5-remoteproc.yaml
> b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-
> remoteproc.yaml
> > new file mode 100644
> > index 000000000000..ce02e425692e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-
> remoteproc.yaml
> > @@ -0,0 +1,120 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/remoteproc/xilinx,zynqmp-r5-
> remoteproc.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Xilinx R5 remote processor controller bindings
> > +
> > +description:
> > +  This document defines the binding for the remoteproc component that
> loads and
> > +  boots firmwares on the Xilinx Zynqmp and Versal family chipset.
> > +
> > +  Note that the Linux has global addressing view of the R5-related memory
> (TCM)
> > +  so the absolute address ranges are provided in TCM reg's.
> 
> blank line needed.
> 
will fix in next rev.
> TCMs specifically I'm concerned about how they are represented in system
> DT and here...
> 
So System DT can tie in with lopper plugin/assists so that TCMs are output to whatever the linux driver expects. 
> > +maintainers:
> > +  - Ed Mooring <ed.mooring@xilinx.com>
> > +  - Ben Levinsky <ben.levinsky@xilinx.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: "xlnx,zynqmp-r5-remoteproc-1.0"
> 
> Don't need quotes.
> 
will fix in next rev.
> The use of version numbers was allowed for Xilinx programmable IP. I
> don't think this falls into that category.
> 
will fix in next rev.
> > +
> > +  lockstep-mode:
> > +    description:
> > +      If this property is present, then the configuration is lock-step.
> 
> boolean...
> 
By this comment do you mean you want this to change or mention that it is implicitly Boolean?
> > +      Otherwise RPU is split.
> > +    maxItems: 1
> 
> ...or an array?
> 
> Either way, doesn't work for TI R5.
> 
So as the configuration for both the TI R5 and Xilinx R5 is independent what in common would you like to see here?
For example Xilinx driver can similarly have the "Xilinx,cluster-mode" or "cluster-mode" but for Xilinx platform manager our split configuration value in Xilinx platform manager is '0' while in TI drver it is '1'. So I can make the driver expect it then translate if needed if this what you prefer. 

Otherwise how would  you suggest the Xilinx r5 remoteproc driver describe split/lockstep mode?
> > +
> > +  interrupts:
> > +    description:
> > +      Interrupt mapping for remoteproc IPI. It is required if the
> > +      user uses the remoteproc driver with the RPMsg kernel driver.
> > +    maxItems: 6
> > +
> > +  memory-region:
> > +    description:
> > +      collection of memory carveouts used for elf-loading and inter-processor
> > +      communication.
> > +    maxItems: 4
> > +    minItems: 4
> 
> Need to define what each region is.
> 
> One blank line between properties.
> 
will fix in next rev. for memory-regions is the following ok?
      collection of phandles for memory carveouts for elf-loading and
       inter-processor communication.

       This collection can consist of reserved-mem carveouts in DDR.

> > +  meta-memory-regions:
> > +    description:
> > +      collection of memories that are not present in the top level memory
> > +      nodes' mapping. For example, R5s' TCM banks. These banks are needed
> > +      for R5 firmware meta data such as the R5 firmware's heap and stack
> 
> Humm, needs a better explanation.
How is the following:
Collection of phandles for reserved on-chip SRAM regions.


Otherwise I can get rid of this property and combine into memory-region if you wish.

> 
> > +  pnode-id:
> > +    maxItems: 1
> 
> What's this?
> 
Can add the following:
power node id that is used to uniquely identify the RPU for Xilinx
      Power Management. The value is then passed to Xilinx platform
      manager for power on/off and access.
> > +  mboxes:
> > +    maxItems: 2
> 
> Need to define what each one is.
> 
How is the following:
array of phandles that describe the rx and tx for xilinx zynqmp
      mailbox driver. order of rx and tx is described by the mbox-names
      property. This will be used for communication with remote
      processor.
> > +  mbox-names:
> > +    maxItems: 2
> 
> Need to define the names.
> 
How is the following:
array of strings that denote which item in the mboxes property array
      are the rx and tx for xilinx zynqmp mailbox driver
> > +
> > +examples:
> > +  - |
> > +     reserved-memory {
> > +          #address-cells = <1>;
> > +          #size-cells = <1>;
> > +          ranges;
> > +          elf_load: rproc@3ed000000 {
> > +               no-map;
> > +               reg = <0x3ed00000 0x40000>;
> > +          };
> > +
> > +          rpu0vdev0vring0: rpu0vdev0vring0@3ed40000 {
> > +               no-map;
> > +               reg = <0x3ed40000 0x4000>;
> > +          };
> > +          rpu0vdev0vring1: rpu0vdev0vring1@3ed44000 {
> > +               no-map;
> > +               reg = <0x3ed44000 0x4000>;
> > +          };
> > +          rpu0vdev0buffer: rpu0vdev0buffer@3ed48000 {
> > +               no-map;
> > +               reg = <0x3ed48000 0x100000>;
> > +          };
> > +
> > +     };
> > +
> > +     /*
> > +      * Below nodes are required if using TCM to load R5 firmware
> > +      * if not, then either do not provide nodes are label as disabled in
> > +      * status property
> > +      */
> > +     tcm0a: tcm_0a@ffe00000 {
> > +         reg = <0xffe00000 0x10000>;
> > +         pnode-id = <0xf>;
> > +         no-map;
> > +         status = "okay";
> > +         phandle = <0x40>;
> > +         compatible = "xlnx,tcm";
> 
> TCM is a Xilinx specific thing?
> 
No... good point. Can remove the compatible string in next rev.
> > +     };
> > +     tcm0b: tcm_1a@ffe20000 {
> > +         reg = <0xffe20000 0x10000>;
> > +         pnode-id = <0x10>;
> > +         no-map;
> > +         status = "okay";
> > +         compatible = "xlnx,tcm";
> > +         phandle = <0x41>;
> > +     };
> > +
> > +     rpu {
> > +          compatible = "xlnx,zynqmp-r5-remoteproc-1.0";
> > +          #address-cells = <1>;
> > +          #size-cells = <1>;
> > +          ranges;
> > +          lockstep-mode;
> > +          r5_0 {
> > +               ranges;
> > +               #address-cells = <1>;
> > +               #size-cells = <1>;
> > +               memory-region = <&elf_load>,
> > +                               <&rpu0vdev0vring0>,
> > +                               <&rpu0vdev0vring1>,
> > +                               <&rpu0vdev0buffer>;
> > +               meta-memory-regions = <&tcm_0a>, <&tcm_0b>;
> > +               pnode-id = <0x7>;
> > +          };
> > +     };
> > +
> > +...
> > --
> > 2.17.1
> >
Thank you,
Ben
Stefano Stabellini Sept. 30, 2020, 10:40 p.m. UTC | #3
On Tue, 29 Sep 2020, Rob Herring wrote:
> > index 000000000000..ce02e425692e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
> > @@ -0,0 +1,120 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Xilinx R5 remote processor controller bindings
> > +
> > +description:
> > +  This document defines the binding for the remoteproc component that loads and
> > +  boots firmwares on the Xilinx Zynqmp and Versal family chipset.
> > +
> > +  Note that the Linux has global addressing view of the R5-related memory (TCM)
> > +  so the absolute address ranges are provided in TCM reg's.
> 
> blank line needed.
> 
> TCMs specifically I'm concerned about how they are represented in system 
> DT and here...

So far I have been keeping the TCMs in system DT as regular nodes under
/amba. E.g.:

		tcm: tcm@ffe00000 {
			compatible = "mmio-sram";
			reg = <0x0 0xffe00000 0x0 0x10000>;
		};

(I am not sure if "mmio-sram" is the right compatible.)
Rob Herring Nov. 5, 2020, 7:42 p.m. UTC | #4
On Wed, Sep 30, 2020 at 11:21 AM Ben Levinsky <BLEVINSK@xilinx.com> wrote:
>
> Hi Rob,
>
> > -----Original Message-----
> > From: Rob Herring <robh@kernel.org>
> > Sent: Tuesday, September 29, 2020 11:36 AM
> > To: Ben Levinsky <BLEVINSK@xilinx.com>
> > Cc: Stefano Stabellini <stefanos@xilinx.com>; Michal Simek
> > <michals@xilinx.com>; michael.auchter@ni.com; devicetree@vger.kernel.org;
> > mathieu.poirier@linaro.org; Ed T. Mooring <emooring@xilinx.com>; linux-
> > remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; Jason Wu <j.wu@xilinx.com>; Jiaying Liang
> > <jliang@xilinx.com>; Michal Simek <michals@xilinx.com>
> > Subject: Re: [PATCH v16 4/5] dt-bindings: remoteproc: Add documentation for
> > ZynqMP R5 rproc bindings
> >
> > On Tue, Sep 22, 2020 at 03:39:29PM -0700, Ben Levinsky wrote:
> > > Add binding for ZynqMP R5 OpenAMP.
> > >
> > > Represent the RPU domain resources in one device node. Each RPU
> > > processor is a subnode of the top RPU domain node.
> > >
> > > Signed-off-by: Jason Wu <j.wu@xilinx.com>
> > > Signed-off-by: Wendy Liang <jliang@xilinx.com>
> > > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > > Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
> > > ---
> > > v3:
> > > - update zynqmp_r5 yaml parsing to not raise warnings for extra
> > >   information in children of R5 node. The warning "node has a unit
> > >   name, but no reg or ranges property" will still be raised though
> > >   as this particular node is needed to describe the
> > >   '#address-cells' and '#size-cells' information.
> > > v4::
> > > - remove warning '/example-0/rpu@ff9a0000/r5@0:
> > >   node has a unit name, but no reg or ranges property'
> > >   by adding reg to r5 node.
> > > v5:
> > > - update device tree sample and yaml parsing to not raise any warnings
> > > - description for memory-region in yaml parsing
> > > - compatible string in yaml parsing for TCM
> > > v6:
> > > - remove coupling TCM nodes with remoteproc
> > > - remove mailbox as it is optional not needed
> > > v7:
> > > - change lockstep-mode to xlnx,cluster-mode
> > > v9:
> > > - show example IPC nodes and tcm bank nodes
> > > v11:
> > > - add property meta-memory-regions to illustrate link
> > >   between r5 and TCM banks
> > > - update so no warnings from 'make dt_binding_check'
> > > v14:
> > > - concerns were raised about the new property meta-memory-regions.
> > >   There is no clear direction so for the moment I kept it in the series
> > > - place IPC nodes in RAM in the reserved memory section
> > > v15:
> > > - change lockstep-mode prop as follows: if present, then RPU cluster is in
> > >   lockstep mode. if not present, cluster is in split mode.
> > > ---
> > >  .../xilinx,zynqmp-r5-remoteproc.yaml          | 120 ++++++++++++++++++
> > >  1 file changed, 120 insertions(+)
> > >  create mode 100644
> > Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-
> > remoteproc.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-
> > r5-remoteproc.yaml
> > b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-
> > remoteproc.yaml
> > > new file mode 100644
> > > index 000000000000..ce02e425692e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-
> > remoteproc.yaml
> > > @@ -0,0 +1,120 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: "http://devicetree.org/schemas/remoteproc/xilinx,zynqmp-r5-
> > remoteproc.yaml#"
> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > +
> > > +title: Xilinx R5 remote processor controller bindings
> > > +
> > > +description:
> > > +  This document defines the binding for the remoteproc component that
> > loads and
> > > +  boots firmwares on the Xilinx Zynqmp and Versal family chipset.
> > > +
> > > +  Note that the Linux has global addressing view of the R5-related memory
> > (TCM)
> > > +  so the absolute address ranges are provided in TCM reg's.
> >
> > blank line needed.
> >
> will fix in next rev.
> > TCMs specifically I'm concerned about how they are represented in system
> > DT and here...
> >
> So System DT can tie in with lopper plugin/assists so that TCMs are output to whatever the linux driver expects.

Sorry, I don't buy lopper can handle it. That leaves too much hand waving IMO.

> > > +maintainers:
> > > +  - Ed Mooring <ed.mooring@xilinx.com>
> > > +  - Ben Levinsky <ben.levinsky@xilinx.com>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: "xlnx,zynqmp-r5-remoteproc-1.0"
> >
> > Don't need quotes.
> >
> will fix in next rev.
> > The use of version numbers was allowed for Xilinx programmable IP. I
> > don't think this falls into that category.
> >
> will fix in next rev.
> > > +
> > > +  lockstep-mode:
> > > +    description:
> > > +      If this property is present, then the configuration is lock-step.
> >
> > boolean...
> >
> By this comment do you mean you want this to change or mention that it is implicitly Boolean?

You defining this as a boolean and then using a schema that applies to
arrays (maxItems).

> > > +      Otherwise RPU is split.
> > > +    maxItems: 1
> >
> > ...or an array?
> >
> > Either way, doesn't work for TI R5.
> >
> So as the configuration for both the TI R5 and Xilinx R5 is independent what in common would you like to see here?
> For example Xilinx driver can similarly have the "Xilinx,cluster-mode" or "cluster-mode" but for Xilinx platform manager our split configuration value in Xilinx platform manager is '0' while in TI drver it is '1'. So I can make the driver expect it then translate if needed if this what you prefer.

What's Xilinx platform manager? The drivers are irrelevant for the binding.

>
> Otherwise how would  you suggest the Xilinx r5 remoteproc driver describe split/lockstep mode?

For what's clearly the same functionality, I want the binding the
same. TI can have more modes if that's what they need.

> > > +
> > > +  interrupts:
> > > +    description:
> > > +      Interrupt mapping for remoteproc IPI. It is required if the
> > > +      user uses the remoteproc driver with the RPMsg kernel driver.
> > > +    maxItems: 6
> > > +
> > > +  memory-region:
> > > +    description:
> > > +      collection of memory carveouts used for elf-loading and inter-processor
> > > +      communication.
> > > +    maxItems: 4
> > > +    minItems: 4
> >
> > Need to define what each region is.
> >
> > One blank line between properties.
> >
> will fix in next rev. for memory-regions is the following ok?
>       collection of phandles for memory carveouts for elf-loading and
>        inter-processor communication.
>
>        This collection can consist of reserved-mem carveouts in DDR.

No.

items:
  - description: This is the 1st entry...
  - description: This is the 2nd entry...
  ...

>
> > > +  meta-memory-regions:
> > > +    description:
> > > +      collection of memories that are not present in the top level memory
> > > +      nodes' mapping. For example, R5s' TCM banks. These banks are needed
> > > +      for R5 firmware meta data such as the R5 firmware's heap and stack
> >
> > Humm, needs a better explanation.
> How is the following:
> Collection of phandles for reserved on-chip SRAM regions.
>
>
> Otherwise I can get rid of this property and combine into memory-region if you wish.

Not sure really.

> > > +  pnode-id:
> > > +    maxItems: 1
> >
> > What's this?
> >
> Can add the following:
> power node id that is used to uniquely identify the RPU for Xilinx
>       Power Management. The value is then passed to Xilinx platform
>       manager for power on/off and access.

Sounds like you should be using the power-domain binding.

> > > +  mboxes:
> > > +    maxItems: 2
> >
> > Need to define what each one is.
> >
> How is the following:
> array of phandles that describe the rx and tx for xilinx zynqmp
>       mailbox driver. order of rx and tx is described by the mbox-names
>       property. This will be used for communication with remote
>       processor.
> > > +  mbox-names:
> > > +    maxItems: 2
> >
> > Need to define the names.
> >
> How is the following:
> array of strings that denote which item in the mboxes property array
>       are the rx and tx for xilinx zynqmp mailbox driver

Terrible. We have a schema to define constraints.

items:
  - const: rx
  - const: tx


> > > +
> > > +examples:
> > > +  - |
> > > +     reserved-memory {
> > > +          #address-cells = <1>;
> > > +          #size-cells = <1>;
> > > +          ranges;
> > > +          elf_load: rproc@3ed000000 {
> > > +               no-map;
> > > +               reg = <0x3ed00000 0x40000>;
> > > +          };
> > > +
> > > +          rpu0vdev0vring0: rpu0vdev0vring0@3ed40000 {
> > > +               no-map;
> > > +               reg = <0x3ed40000 0x4000>;
> > > +          };
> > > +          rpu0vdev0vring1: rpu0vdev0vring1@3ed44000 {
> > > +               no-map;
> > > +               reg = <0x3ed44000 0x4000>;
> > > +          };
> > > +          rpu0vdev0buffer: rpu0vdev0buffer@3ed48000 {
> > > +               no-map;
> > > +               reg = <0x3ed48000 0x100000>;
> > > +          };
> > > +
> > > +     };
> > > +
> > > +     /*
> > > +      * Below nodes are required if using TCM to load R5 firmware
> > > +      * if not, then either do not provide nodes are label as disabled in
> > > +      * status property
> > > +      */
> > > +     tcm0a: tcm_0a@ffe00000 {
> > > +         reg = <0xffe00000 0x10000>;
> > > +         pnode-id = <0xf>;
> > > +         no-map;
> > > +         status = "okay";
> > > +         phandle = <0x40>;
> > > +         compatible = "xlnx,tcm";
> >
> > TCM is a Xilinx specific thing?
> >
> No... good point. Can remove the compatible string in next rev.

Well, it should probably have some sort of compatible so we know what
the thing is...

If we have a DT for the R5 side or view of the system, we'd need to
define TCMs, right? For the A cores, it shouldn't look any different
other than perhaps the address. So, you first need a TCM binding.
Maybe that ends up being just mmio-sram binding, maybe not. Probably
not if there's power controls.

> > > +     };
> > > +     tcm0b: tcm_1a@ffe20000 {
> > > +         reg = <0xffe20000 0x10000>;
> > > +         pnode-id = <0x10>;
> > > +         no-map;
> > > +         status = "okay";
> > > +         compatible = "xlnx,tcm";
> > > +         phandle = <0x41>;
> > > +     };
> > > +
> > > +     rpu {
> > > +          compatible = "xlnx,zynqmp-r5-remoteproc-1.0";
> > > +          #address-cells = <1>;
> > > +          #size-cells = <1>;
> > > +          ranges;
> > > +          lockstep-mode;
> > > +          r5_0 {
> > > +               ranges;
> > > +               #address-cells = <1>;
> > > +               #size-cells = <1>;
> > > +               memory-region = <&elf_load>,
> > > +                               <&rpu0vdev0vring0>,
> > > +                               <&rpu0vdev0vring1>,
> > > +                               <&rpu0vdev0buffer>;
> > > +               meta-memory-regions = <&tcm_0a>, <&tcm_0b>;
> > > +               pnode-id = <0x7>;
> > > +          };
> > > +     };
> > > +
> > > +...
> > > --
> > > 2.17.1
> > >
> Thank you,
> Ben
>
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
new file mode 100644
index 000000000000..ce02e425692e
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
@@ -0,0 +1,120 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Xilinx R5 remote processor controller bindings
+
+description:
+  This document defines the binding for the remoteproc component that loads and
+  boots firmwares on the Xilinx Zynqmp and Versal family chipset.
+
+  Note that the Linux has global addressing view of the R5-related memory (TCM)
+  so the absolute address ranges are provided in TCM reg's.
+maintainers:
+  - Ed Mooring <ed.mooring@xilinx.com>
+  - Ben Levinsky <ben.levinsky@xilinx.com>
+
+properties:
+  compatible:
+    const: "xlnx,zynqmp-r5-remoteproc-1.0"
+
+  lockstep-mode:
+    description:
+      If this property is present, then the configuration is lock-step.
+      Otherwise RPU is split.
+    maxItems: 1
+
+  interrupts:
+    description:
+      Interrupt mapping for remoteproc IPI. It is required if the
+      user uses the remoteproc driver with the RPMsg kernel driver.
+    maxItems: 6
+
+  memory-region:
+    description:
+      collection of memory carveouts used for elf-loading and inter-processor
+      communication.
+    maxItems: 4
+    minItems: 4
+  meta-memory-regions:
+    description:
+      collection of memories that are not present in the top level memory
+      nodes' mapping. For example, R5s' TCM banks. These banks are needed
+      for R5 firmware meta data such as the R5 firmware's heap and stack
+  pnode-id:
+    maxItems: 1
+  mboxes:
+    maxItems: 2
+  mbox-names:
+    maxItems: 2
+
+examples:
+  - |
+     reserved-memory {
+          #address-cells = <1>;
+          #size-cells = <1>;
+          ranges;
+          elf_load: rproc@3ed000000 {
+               no-map;
+               reg = <0x3ed00000 0x40000>;
+          };
+
+          rpu0vdev0vring0: rpu0vdev0vring0@3ed40000 {
+               no-map;
+               reg = <0x3ed40000 0x4000>;
+          };
+          rpu0vdev0vring1: rpu0vdev0vring1@3ed44000 {
+               no-map;
+               reg = <0x3ed44000 0x4000>;
+          };
+          rpu0vdev0buffer: rpu0vdev0buffer@3ed48000 {
+               no-map;
+               reg = <0x3ed48000 0x100000>;
+          };
+
+     };
+
+     /*
+      * Below nodes are required if using TCM to load R5 firmware
+      * if not, then either do not provide nodes are label as disabled in
+      * status property
+      */
+     tcm0a: tcm_0a@ffe00000 {
+         reg = <0xffe00000 0x10000>;
+         pnode-id = <0xf>;
+         no-map;
+         status = "okay";
+         phandle = <0x40>;
+         compatible = "xlnx,tcm";
+     };
+     tcm0b: tcm_1a@ffe20000 {
+         reg = <0xffe20000 0x10000>;
+         pnode-id = <0x10>;
+         no-map;
+         status = "okay";
+         compatible = "xlnx,tcm";
+         phandle = <0x41>;
+     };
+
+     rpu {
+          compatible = "xlnx,zynqmp-r5-remoteproc-1.0";
+          #address-cells = <1>;
+          #size-cells = <1>;
+          ranges;
+          lockstep-mode;
+          r5_0 {
+               ranges;
+               #address-cells = <1>;
+               #size-cells = <1>;
+               memory-region = <&elf_load>,
+                               <&rpu0vdev0vring0>,
+                               <&rpu0vdev0vring1>,
+                               <&rpu0vdev0buffer>;
+               meta-memory-regions = <&tcm_0a>, <&tcm_0b>;
+               pnode-id = <0x7>;
+          };
+     };
+
+...