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

Commit Message

Ben Levinsky April 24, 2020, 5:36 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: Ben Levinsky <[e-mail]>
Signed-off-by: Jason Wu <[e-mail]>
Signed-off-by: Wendy Liang <[e-mail]>
Signed-off-by: Michal Simek <[e-mail]>
---
Changes since v2:
- 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.
Changes since 3:
- 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.
---

 .../remoteproc/xilinx,zynqmp-r5-remoteproc.yaml    | 127 +++++++++++++++++++++
 1 file changed, 127 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml

Comments

Rob Herring May 11, 2020, 10:17 p.m. UTC | #1

On Fri, Apr 24, 2020 at 10:36:09AM -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.

This needs to be sorted out as part of the system DT effort that Xilinx 
is working on. I can't see this binding co-existing with it.

> 
> Signed-off-by: Ben Levinsky <[e-mail]>
> Signed-off-by: Jason Wu <[e-mail]>
> Signed-off-by: Wendy Liang <[e-mail]>
> Signed-off-by: Michal Simek <[e-mail]>
> ---
> Changes since v2:
> - 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.
> Changes since 3:
> - 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.
> ---
> 
>  .../remoteproc/xilinx,zynqmp-r5-remoteproc.yaml    | 127 +++++++++++++++++++++
>  1 file changed, 127 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 0000000..41520b6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
> @@ -0,0 +1,127 @@
> +# 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.
> +
> +maintainers:
> +  - Ed Mooring <[e-mail]>
> +  - Ben Levinsky <[e-mail]>
> +
> +properties:
> +  compatible:
> +    const: "xlnx,zynqmp-r5-remoteproc-1.0"
> +
> +  core_conf:
> +    description:
> +      R5 core configuration (valid string - split or lock-step)
> +    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:
> +    maxItems: 4
> +    minItems: 4
> +  pnode-id:
> +    maxItems: 1

What is this?

> +  mboxes:
> +    maxItems: 2
> +  mbox-names:
> +    maxItems: 2
> +
> +  r5@0:
> +    type: object
> +    required:
> +        - '#address-cells'
> +        - '#size-cells'
> +        - pnode-id
> +examples:
> +  - |
> +     reserved-memory {
> +          #address-cells = <1>;
> +          #size-cells = <1>;
> +          ranges;
> +          rpu0vdev0vring0: rpu0vdev0vring0@3ed40000 {
> +               no-map;
> +               reg = <0x3ed40000 0x4000>;
> +          };
> +          rpu0vdev0vring1: rpu0vdev0vring1@3ed44000 {
> +               no-map;
> +               reg = <0x3ed44000 0x4000>;
> +          };
> +          rpu0vdev0buffer: rpu0vdev0buffer@3ed48000 {
> +               no-map;
> +               reg = <0x3ed48000 0x100000>;
> +          };
> +          rproc_0_reserved: rproc@3ed000000 {
> +               no-map;
> +               reg = <0x3ed00000 0x40000>;
> +          };
> +     };
> +     rpu: rpu@ff9a0000 {
> +          compatible = "xlnx,zynqmp-r5-remoteproc-1.0";
> +          #address-cells = <1>;
> +          #size-cells = <1>;
> +          ranges;
> +          core_conf = "split";

If split, then where is the 2nd core?

> +          reg = <0xFF9A0000 0x10000>;
> +          r5_0: r5@0 {

Unit-addresses are based on 'reg' values.

> +               ranges;
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               reg = <0xFF9A0100 0x1000>;
> +               memory-region = <&rproc_0_reserved>, <&rpu0vdev0buffer>, <&rpu0vdev0vring0>, <&rpu0vdev0vring1>;
> +               pnode-id = <0x7>;
> +               mboxes = <&ipi_mailbox_rpu0 0>, <&ipi_mailbox_rpu0 1>;
> +               mbox-names = "tx", "rx";
> +               tcm_0_a: tcm_0@0 {
> +                    #address-cells = <1>;
> +                    #size-cells = <1>;
> +                    reg = <0xFFE00000 0x10000>;
> +                    pnode-id = <0xf>;

These nodes probably need some sort of compatible. And don't the TCMs 
have different addresses for R5 vs. the A cores?

> +               };
> +               tcm_0_b: tcm_0@1 {
> +                    #address-cells = <2>;
> +                    #size-cells = <2>;
> +                    reg = <0xFFE20000 0x10000>;
> +                    pnode-id = <0x10>;
> +               };
> +          };
> +     };
> +
> +
> +     zynqmp_ipi1 {
> +          compatible = "xlnx,zynqmp-ipi-mailbox";
> +          interrupt-parent = <&gic>;
> +          interrupts = <0 29 4>;
> +          xlnx,ipi-id = <7>;
> +          #address-cells = <1>;
> +          #size-cells = <1>;
> +          ranges;
> +
> +          /* APU<->RPU0 IPI mailbox controller */
> +          ipi_mailbox_rpu0: mailbox@ff90000 {
> +               reg = <0xff990600 0x20>,
> +                     <0xff990620 0x20>,
> +                     <0xff9900c0 0x20>,
> +                     <0xff9900e0 0x20>;
> +               reg-names = "local_request_region",
> +                        "local_response_region",
> +                        "remote_request_region",
> +                        "remote_response_region";
> +               #mbox-cells = <1>;
> +               xlnx,ipi-id = <1>;
> +          };
> +     };
> +
> +...
> -- 
> 2.7.4
>

Ben Levinsky May 26, 2020, 5:40 p.m. UTC | #2

Hi Rob,

The Xilinx R5 Remoteproc driver has been around for a long time -- admittedly we should have upstreamed it long ago. The driver in the current form is using an "classic" remoteproc device tree node as described here.

I am working with Stefano to come up with an appropriate System Device Tree representation but it is not going to be ready right away. Our preference would be to upstream the remoteproc node and driver in their current forms while system device tree is maturing.

Will also update as per your below comments in a v5 too.

Best Regards,
Ben Levinsky

-----Original Message-----
From: Rob Herring <[e-mail]> 
Sent: Monday, May 11, 2020 3:18 PM
To: Ben Levinsky <[e-mail]>
Cc: [e-mail]; [e-mail]; Michal Simek <[e-mail]>; Jolly Shah <[e-mail]>; Rajan Vaja <[e-mail]>; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; Jason Wu <[e-mail]>; Jiaying Liang <[e-mail]>
Subject: Re: [PATCH v4 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings

On Fri, Apr 24, 2020 at 10:36:09AM -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.

This needs to be sorted out as part of the system DT effort that Xilinx is working on. I can't see this binding co-existing with it.

> 
> Signed-off-by: Ben Levinsky <[e-mail]>
> Signed-off-by: Jason Wu <[e-mail]>
> Signed-off-by: Wendy Liang <[e-mail]>
> Signed-off-by: Michal Simek <[e-mail]>
> ---
> Changes since v2:
> - 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.
> Changes since 3:
> - 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.
> ---
> 
>  .../remoteproc/xilinx,zynqmp-r5-remoteproc.yaml    | 127 +++++++++++++++++++++
>  1 file changed, 127 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remotepr
> oc.yaml
> 
> diff --git 
> a/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remote
> proc.yaml 
> b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remote
> proc.yaml
> new file mode 100644
> index 0000000..41520b6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-re
> +++ moteproc.yaml
> @@ -0,0 +1,127 @@
> +# 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.
> +
> +maintainers:
> +  - Ed Mooring <[e-mail]>
> +  - Ben Levinsky <[e-mail]>
> +
> +properties:
> +  compatible:
> +    const: "xlnx,zynqmp-r5-remoteproc-1.0"
> +
> +  core_conf:
> +    description:
> +      R5 core configuration (valid string - split or lock-step)
> +    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:
> +    maxItems: 4
> +    minItems: 4
> +  pnode-id:
> +    maxItems: 1

What is this? 
[Ben Levinsky] I will description for this. This is used by the Xilinx power management code later on when configuring the RPU.

> +  mboxes:
> +    maxItems: 2
> +  mbox-names:
> +    maxItems: 2
> +
> +  r5@0:
> +    type: object
> +    required:
> +        - '#address-cells'
> +        - '#size-cells'
> +        - pnode-id
> +examples:
> +  - |
> +     reserved-memory {
> +          #address-cells = <1>;
> +          #size-cells = <1>;
> +          ranges;
> +          rpu0vdev0vring0: rpu0vdev0vring0@3ed40000 {
> +               no-map;
> +               reg = <0x3ed40000 0x4000>;
> +          };
> +          rpu0vdev0vring1: rpu0vdev0vring1@3ed44000 {
> +               no-map;
> +               reg = <0x3ed44000 0x4000>;
> +          };
> +          rpu0vdev0buffer: rpu0vdev0buffer@3ed48000 {
> +               no-map;
> +               reg = <0x3ed48000 0x100000>;
> +          };
> +          rproc_0_reserved: rproc@3ed000000 {
> +               no-map;
> +               reg = <0x3ed00000 0x40000>;
> +          };
> +     };
> +     rpu: rpu@ff9a0000 {
> +          compatible = "xlnx,zynqmp-r5-remoteproc-1.0";
> +          #address-cells = <1>;
> +          #size-cells = <1>;
> +          ranges;
> +          core_conf = "split";

If split, then where is the 2nd core?
[Ben Levinsky] Will fix, I will add second core in v5.

> +          reg = <0xFF9A0000 0x10000>;
> +          r5_0: r5@0 {

Unit-addresses are based on 'reg' values.
[Ben Levinsky] Will fix this, thanks

> +               ranges;
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               reg = <0xFF9A0100 0x1000>;
> +               memory-region = <&rproc_0_reserved>, <&rpu0vdev0buffer>, <&rpu0vdev0vring0>, <&rpu0vdev0vring1>;
> +               pnode-id = <0x7>;
> +               mboxes = <&ipi_mailbox_rpu0 0>, <&ipi_mailbox_rpu0 1>;
> +               mbox-names = "tx", "rx";
> +               tcm_0_a: tcm_0@0 {
> +                    #address-cells = <1>;
> +                    #size-cells = <1>;
> +                    reg = <0xFFE00000 0x10000>;
> +                    pnode-id = <0xf>;

These nodes probably need some sort of compatible. And don't the TCMs have different addresses for R5 vs. the A cores?
[Ben Levinsky] I can add a compatible. The addressesing here is absolute (i.e. 0xffex-xxxx ) as it is used from point of view of A core here.

> +               };
> +               tcm_0_b: tcm_0@1 {
> +                    #address-cells = <2>;
> +                    #size-cells = <2>;
> +                    reg = <0xFFE20000 0x10000>;
> +                    pnode-id = <0x10>;
> +               };
> +          };
> +     };
> +
> +
> +     zynqmp_ipi1 {
> +          compatible = "xlnx,zynqmp-ipi-mailbox";
> +          interrupt-parent = <&gic>;
> +          interrupts = <0 29 4>;
> +          xlnx,ipi-id = <7>;
> +          #address-cells = <1>;
> +          #size-cells = <1>;
> +          ranges;
> +
> +          /* APU<->RPU0 IPI mailbox controller */
> +          ipi_mailbox_rpu0: mailbox@ff90000 {
> +               reg = <0xff990600 0x20>,
> +                     <0xff990620 0x20>,
> +                     <0xff9900c0 0x20>,
> +                     <0xff9900e0 0x20>;
> +               reg-names = "local_request_region",
> +                        "local_response_region",
> +                        "remote_request_region",
> +                        "remote_response_region";
> +               #mbox-cells = <1>;
> +               xlnx,ipi-id = <1>;
> +          };
> +     };
> +
> +...
> --
> 2.7.4
>

Rob Herring June 10, 2020, 8:38 p.m. UTC | #3

On Tue, May 26, 2020 at 11:40 AM Ben Levinsky <[e-mail]> wrote:
>
> Hi Rob,
>
> The Xilinx R5 Remoteproc driver has been around for a long time -- admittedly we should have upstreamed it long ago. The driver in the current form is using an "classic" remoteproc device tree node as described here.

I would rather not have 2 possible bindings to maintain. If there's
been no rush to upstream this til now, then it can wait longer.

>
> I am working with Stefano to come up with an appropriate System Device Tree representation but it is not going to be ready right away. Our preference would be to upstream the remoteproc node and driver in their current forms while system device tree is maturing.

There's obviously going to still need to be some sort of description
of the interface between cores, but this has parts that obviously
conflict with what's getting defined for system DT. The TCMs are the
most obvious. If you can remove (or hardcode in the driver) what
conflicts, then perhaps this can be upstreamed now.

Rob

Stefano Stabellini June 30, 2020, 12:37 a.m. UTC | #4

On Wed, 10 Jun 2020, Rob Herring wrote:
> On Tue, May 26, 2020 at 11:40 AM Ben Levinsky <[e-mail]> wrote:
> >
> > Hi Rob,
> >
> > The Xilinx R5 Remoteproc driver has been around for a long time -- admittedly we should have upstreamed it long ago. The driver in the current form is using an "classic" remoteproc device tree node as described here.
> 
> I would rather not have 2 possible bindings to maintain. If there's
> been no rush to upstream this til now, then it can wait longer.
> 
> >
> > I am working with Stefano to come up with an appropriate System Device Tree representation but it is not going to be ready right away. Our preference would be to upstream the remoteproc node and driver in their current forms while system device tree is maturing.
> 
> There's obviously going to still need to be some sort of description
> of the interface between cores, but this has parts that obviously
> conflict with what's getting defined for system DT. The TCMs are the
> most obvious. If you can remove (or hardcode in the driver) what
> conflicts, then perhaps this can be upstreamed now.


Hi Rob,

Sorry it took a while to answer back but we wanted to do some research
to make sure the reply is correct.


The System Device Tree version of the OpenAMP remoteproc bindings aims
at being simpler and vendor-neutral. As anything else System Device
Tree, Lopper will read it and generate a "traditional" device tree with
the existing remoteproc bindings. In that sense, it might not affect
Linux directly.

However, given the fragmentation of the remoteproc bindings across
multiple vendors (they are all different), I think it is a good idea for
Linux, for System Device Tree, and in general to come up with simpler
remoteproc bindings, more aligned between the vendors. If nothing else,
it is going to make Lopper's development easier.


So I think it is a good idea to take this opportunity to simplify the
Xilinx remoteproc bindings as you suggested. The idea of to removing the
TCM nodes is a good one. In addition I asked Ben to have a look at
whether the mboxes and mbox-names properties can be removed too.

Ben will reply with a simplified bindings proposal.

Bjorn Andersson June 30, 2020, 2:20 a.m. UTC | #5

On Mon 29 Jun 17:37 PDT 2020, Stefano Stabellini wrote:

> On Wed, 10 Jun 2020, Rob Herring wrote:
> > On Tue, May 26, 2020 at 11:40 AM Ben Levinsky <[e-mail]> wrote:
> > >
> > > Hi Rob,
> > >
> > > The Xilinx R5 Remoteproc driver has been around for a long time -- admittedly we should have upstreamed it long ago. The driver in the current form is using an "classic" remoteproc device tree node as described here.
> > 
> > I would rather not have 2 possible bindings to maintain. If there's
> > been no rush to upstream this til now, then it can wait longer.
> > 
> > >
> > > I am working with Stefano to come up with an appropriate System Device Tree representation but it is not going to be ready right away. Our preference would be to upstream the remoteproc node and driver in their current forms while system device tree is maturing.
> > 
> > There's obviously going to still need to be some sort of description
> > of the interface between cores, but this has parts that obviously
> > conflict with what's getting defined for system DT. The TCMs are the
> > most obvious. If you can remove (or hardcode in the driver) what
> > conflicts, then perhaps this can be upstreamed now.
> 
> 
> Hi Rob,
> 
> Sorry it took a while to answer back but we wanted to do some research
> to make sure the reply is correct.
> 
> 
> The System Device Tree version of the OpenAMP remoteproc bindings aims
> at being simpler and vendor-neutral. As anything else System Device
> Tree, Lopper will read it and generate a "traditional" device tree with
> the existing remoteproc bindings. In that sense, it might not affect
> Linux directly.
> 

Can you give some examples of how you will be able to describe the
hardware involved in powering/clocking resources surrounding your
remoteproc and the necessary resources in a "simpler and vendor neutral"
way that then can be further lopped(?) into something that Linux can use
to control any remoteproc?

> However, given the fragmentation of the remoteproc bindings across
> multiple vendors (they are all different), I think it is a good idea for
> Linux, for System Device Tree, and in general to come up with simpler
> remoteproc bindings, more aligned between the vendors. If nothing else,
> it is going to make Lopper's development easier.
> 

In my view the big reason for the fragmentation between bindings is
because they all describe different hardware. There has been common
properties of remoteprocs discussed, but apart from the firmware-name
property I don't think we have agreed on any.

> 
> So I think it is a good idea to take this opportunity to simplify the
> Xilinx remoteproc bindings as you suggested. The idea of to removing the
> TCM nodes is a good one. In addition I asked Ben to have a look at
> whether the mboxes and mbox-names properties can be removed too.
> 

If your remoteproc uses a mailbox for signaling, then this should be
described in devicetree. This will allow you to reuse components in
other designs where either part is replaced or reused.

Regards,
Bjorn

> Ben will reply with a simplified bindings proposal.

Ben Levinsky June 30, 2020, 3:38 p.m. UTC | #6

Hi Bjorn,



-----Original Message-----
From: Bjorn Andersson <[e-mail]> 
Sent: Monday, June 29, 2020 7:20 PM
To: Stefano Stabellini <[e-mail]>
Cc: Rob Herring <[e-mail]>; Ben Levinsky <[e-mail]>; [e-mail]; Michal Simek <[e-mail]>; Jolly Shah <[e-mail]>; Rajan Vaja <[e-mail]>; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; Stefano Stabellini <[e-mail]>
Subject: Re: [PATCH v4 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings

On Mon 29 Jun 17:37 PDT 2020, Stefano Stabellini wrote:

> On Wed, 10 Jun 2020, Rob Herring wrote:
> > On Tue, May 26, 2020 at 11:40 AM Ben Levinsky <[e-mail]> wrote:
> > >
> > > Hi Rob,
> > >
> > > The Xilinx R5 Remoteproc driver has been around for a long time -- admittedly we should have upstreamed it long ago. The driver in the current form is using an "classic" remoteproc device tree node as described here.
> > 
> > I would rather not have 2 possible bindings to maintain. If there's 
> > been no rush to upstream this til now, then it can wait longer.
> > 
> > >
> > > I am working with Stefano to come up with an appropriate System Device Tree representation but it is not going to be ready right away. Our preference would be to upstream the remoteproc node and driver in their current forms while system device tree is maturing.
> > 
> > There's obviously going to still need to be some sort of description 
> > of the interface between cores, but this has parts that obviously 
> > conflict with what's getting defined for system DT. The TCMs are the 
> > most obvious. If you can remove (or hardcode in the driver) what 
> > conflicts, then perhaps this can be upstreamed now.
> 
> 
> Hi Rob,
> 
> Sorry it took a while to answer back but we wanted to do some research 
> to make sure the reply is correct.
> 
> 
> The System Device Tree version of the OpenAMP remoteproc bindings aims 
> at being simpler and vendor-neutral. As anything else System Device 
> Tree, Lopper will read it and generate a "traditional" device tree 
> with the existing remoteproc bindings. In that sense, it might not 
> affect Linux directly.
> 

Can you give some examples of how you will be able to describe the hardware involved in powering/clocking resources surrounding your remoteproc and the necessary resources in a "simpler and vendor neutral"
way that then can be further lopped(?) into something that Linux can use to control any remoteproc?

> However, given the fragmentation of the remoteproc bindings across 
> multiple vendors (they are all different), I think it is a good idea 
> for Linux, for System Device Tree, and in general to come up with 
> simpler remoteproc bindings, more aligned between the vendors. If 
> nothing else, it is going to make Lopper's development easier.
> 

In my view the big reason for the fragmentation between bindings is because they all describe different hardware. There has been common properties of remoteprocs discussed, but apart from the firmware-name property I don't think we have agreed on any.

> 
> So I think it is a good idea to take this opportunity to simplify the 
> Xilinx remoteproc bindings as you suggested. The idea of to removing 
> the TCM nodes is a good one. In addition I asked Ben to have a look at 
> whether the mboxes and mbox-names properties can be removed too.
> 

If your remoteproc uses a mailbox for signaling, then this should be described in devicetree. This will allow you to reuse components in other designs where either part is replaced or reused.

[Ben Levinsky] The Xilinx R5 remoteproc binding can optionally use the mailbox. That is if loading a simple binary that is not making use of IPC then the mailbox is optional and not needed. Not sure if you still would prefer to have the example showcasing use of mailbox then.


Regards,
Bjorn

> Ben will reply with a simplified bindings proposal.

Ben Levinsky June 30, 2020, 5:11 p.m. UTC | #7

Hi Rob,

Below is proposal simplified device tree binding as per the below discussion.

Best Regards,
Ben

reserved-memory {
     #address-cells = <1>;
     #size-cells = <1>;
     ranges;
     elf_load: rproc@300000000 {
          no-map;
          reg = <0x30000000 0x40000>;
     };
};
Rpu {
     compatible = "xlnx,zynqmp-r5-remoteproc-1.0";
     #address-cells = <1>;
     #size-cells = <1>;
     ranges;
     lockstep-mode = <0>;
     r5_0 {
          ranges;
          #address-cells = <1>;
          #size-cells = <1>;
          memory-region = <&elf_load>;
          pnode-id = <0x10>;
     };
};

-----Original Message-----
From: Ben Levinsky 
Sent: Tuesday, June 30, 2020 8:39 AM
To: Bjorn Andersson <[e-mail]>; Stefano Stabellini <[e-mail]>
Cc: Rob Herring <[e-mail]>; [e-mail]; Michal Simek <[e-mail]>; Jolly Shah <[e-mail]>; Rajan Vaja <[e-mail]>; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; Stefano Stabellini <[e-mail]>
Subject: RE: [PATCH v4 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings

Hi Bjorn,



-----Original Message-----
From: Bjorn Andersson <[e-mail]>
Sent: Monday, June 29, 2020 7:20 PM
To: Stefano Stabellini <[e-mail]>
Cc: Rob Herring <[e-mail]>; Ben Levinsky <[e-mail]>; [e-mail]; Michal Simek <[e-mail]>; Jolly Shah <[e-mail]>; Rajan Vaja <[e-mail]>; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; Stefano Stabellini <[e-mail]>
Subject: Re: [PATCH v4 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings

On Mon 29 Jun 17:37 PDT 2020, Stefano Stabellini wrote:

> On Wed, 10 Jun 2020, Rob Herring wrote:
> > On Tue, May 26, 2020 at 11:40 AM Ben Levinsky <[e-mail]> wrote:
> > >
> > > Hi Rob,
> > >
> > > The Xilinx R5 Remoteproc driver has been around for a long time -- admittedly we should have upstreamed it long ago. The driver in the current form is using an "classic" remoteproc device tree node as described here.
> > 
> > I would rather not have 2 possible bindings to maintain. If there's 
> > been no rush to upstream this til now, then it can wait longer.
> > 
> > >
> > > I am working with Stefano to come up with an appropriate System Device Tree representation but it is not going to be ready right away. Our preference would be to upstream the remoteproc node and driver in their current forms while system device tree is maturing.
> > 
> > There's obviously going to still need to be some sort of description 
> > of the interface between cores, but this has parts that obviously 
> > conflict with what's getting defined for system DT. The TCMs are the 
> > most obvious. If you can remove (or hardcode in the driver) what 
> > conflicts, then perhaps this can be upstreamed now.
> 
> 
> Hi Rob,
> 
> Sorry it took a while to answer back but we wanted to do some research 
> to make sure the reply is correct.
> 
> 
> The System Device Tree version of the OpenAMP remoteproc bindings aims 
> at being simpler and vendor-neutral. As anything else System Device 
> Tree, Lopper will read it and generate a "traditional" device tree 
> with the existing remoteproc bindings. In that sense, it might not 
> affect Linux directly.
> 

Can you give some examples of how you will be able to describe the hardware involved in powering/clocking resources surrounding your remoteproc and the necessary resources in a "simpler and vendor neutral"
way that then can be further lopped(?) into something that Linux can use to control any remoteproc?

> However, given the fragmentation of the remoteproc bindings across 
> multiple vendors (they are all different), I think it is a good idea 
> for Linux, for System Device Tree, and in general to come up with 
> simpler remoteproc bindings, more aligned between the vendors. If 
> nothing else, it is going to make Lopper's development easier.
> 

In my view the big reason for the fragmentation between bindings is because they all describe different hardware. There has been common properties of remoteprocs discussed, but apart from the firmware-name property I don't think we have agreed on any.

> 
> So I think it is a good idea to take this opportunity to simplify the 
> Xilinx remoteproc bindings as you suggested. The idea of to removing 
> the TCM nodes is a good one. In addition I asked Ben to have a look at 
> whether the mboxes and mbox-names properties can be removed too.
> 

If your remoteproc uses a mailbox for signaling, then this should be described in devicetree. This will allow you to reuse components in other designs where either part is replaced or reused.

[Ben Levinsky] The Xilinx R5 remoteproc binding can optionally use the mailbox. That is if loading a simple binary that is not making use of IPC then the mailbox is optional and not needed. Not sure if you still would prefer to have the example showcasing use of mailbox then.


Regards,
Bjorn

> Ben will reply with a simplified bindings proposal.

Stefano Stabellini July 10, 2020, 5:42 p.m. UTC | #8

Sorry for the late reply, a couple of conferences kept me busy.


On Mon, 29 Jun 2020, Bjorn Andersson wrote:
> > However, given the fragmentation of the remoteproc bindings across
> > multiple vendors (they are all different), I think it is a good idea for
> > Linux, for System Device Tree, and in general to come up with simpler
> > remoteproc bindings, more aligned between the vendors. If nothing else,
> > it is going to make Lopper's development easier.
> > 
> 
> In my view the big reason for the fragmentation between bindings is
> because they all describe different hardware. There has been common
> properties of remoteprocs discussed, but apart from the firmware-name
> property I don't think we have agreed on any.

Yeah, it is as you wrote.

I meant to say that there might be room for improvement if the vendors
come together and agree on a few more common properties. However, I
don't have any concrete suggestions on this yet.  Also, as mentioned, we
can work with today's bindings just fine from a system device tree
perspective.


> Can you give some examples of how you will be able to describe the
> hardware involved in powering/clocking resources surrounding your
> remoteproc and the necessary resources in a "simpler and vendor neutral"
> way that then can be further lopped(?) into something that Linux can use
> to control any remoteproc?

The description at the system device tree level looks a bit different,
which might make the problem a bit easier, or at least different.

Let me give you some context. Lopper
(https://github.com/devicetree-org/lopper) is a tool that takes a system
device tree as input and generates one or more traditional device trees
as output (i.e. today's device tree for Linux.)

System device tree comes with the description of multiple "execution
domains" (https://connect.linaro.org/resources/ltd20/ltd20-205/) and
the ability to assign resources to each of them. That part is
vendor-neutral.  We also have the ability to define a vendor-specific
flag when assigning resources.

All together it enables us to describe an openamp/remoteproc system with
only very few vendor-specific info. I am working on a full example of an
input system device tree with openamp information and the resulting
traditional Linux devicetree. I'll make sure to reach out when I have it
ready.



> > So I think it is a good idea to take this opportunity to simplify the
> > Xilinx remoteproc bindings as you suggested. The idea of to removing the
> > TCM nodes is a good one. In addition I asked Ben to have a look at
> > whether the mboxes and mbox-names properties can be removed too.
> > 
> 
> If your remoteproc uses a mailbox for signaling, then this should be
> described in devicetree. This will allow you to reuse components in
> other designs where either part is replaced or reused.

OK

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 0000000..41520b6
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
@@ -0,0 +1,127 @@ 
+# 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.
+
+maintainers:
+  - Ed Mooring <[e-mail]>
+  - Ben Levinsky <[e-mail]>
+
+properties:
+  compatible:
+    const: "xlnx,zynqmp-r5-remoteproc-1.0"
+
+  core_conf:
+    description:
+      R5 core configuration (valid string - split or lock-step)
+    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:
+    maxItems: 4
+    minItems: 4
+  pnode-id:
+    maxItems: 1
+  mboxes:
+    maxItems: 2
+  mbox-names:
+    maxItems: 2
+
+  r5@0:
+    type: object
+    required:
+        - '#address-cells'
+        - '#size-cells'
+        - pnode-id
+examples:
+  - |
+     reserved-memory {
+          #address-cells = <1>;
+          #size-cells = <1>;
+          ranges;
+          rpu0vdev0vring0: rpu0vdev0vring0@3ed40000 {
+               no-map;
+               reg = <0x3ed40000 0x4000>;
+          };
+          rpu0vdev0vring1: rpu0vdev0vring1@3ed44000 {
+               no-map;
+               reg = <0x3ed44000 0x4000>;
+          };
+          rpu0vdev0buffer: rpu0vdev0buffer@3ed48000 {
+               no-map;
+               reg = <0x3ed48000 0x100000>;
+          };
+          rproc_0_reserved: rproc@3ed000000 {
+               no-map;
+               reg = <0x3ed00000 0x40000>;
+          };
+     };
+     rpu: rpu@ff9a0000 {
+          compatible = "xlnx,zynqmp-r5-remoteproc-1.0";
+          #address-cells = <1>;
+          #size-cells = <1>;
+          ranges;
+          core_conf = "split";
+          reg = <0xFF9A0000 0x10000>;
+          r5_0: r5@0 {
+               ranges;
+               #address-cells = <1>;
+               #size-cells = <1>;
+               reg = <0xFF9A0100 0x1000>;
+               memory-region = <&rproc_0_reserved>, <&rpu0vdev0buffer>, <&rpu0vdev0vring0>, <&rpu0vdev0vring1>;
+               pnode-id = <0x7>;
+               mboxes = <&ipi_mailbox_rpu0 0>, <&ipi_mailbox_rpu0 1>;
+               mbox-names = "tx", "rx";
+               tcm_0_a: tcm_0@0 {
+                    #address-cells = <1>;
+                    #size-cells = <1>;
+                    reg = <0xFFE00000 0x10000>;
+                    pnode-id = <0xf>;
+               };
+               tcm_0_b: tcm_0@1 {
+                    #address-cells = <2>;
+                    #size-cells = <2>;
+                    reg = <0xFFE20000 0x10000>;
+                    pnode-id = <0x10>;
+               };
+          };
+     };
+
+
+     zynqmp_ipi1 {
+          compatible = "xlnx,zynqmp-ipi-mailbox";
+          interrupt-parent = <&gic>;
+          interrupts = <0 29 4>;
+          xlnx,ipi-id = <7>;
+          #address-cells = <1>;
+          #size-cells = <1>;
+          ranges;
+
+          /* APU<->RPU0 IPI mailbox controller */
+          ipi_mailbox_rpu0: mailbox@ff90000 {
+               reg = <0xff990600 0x20>,
+                     <0xff990620 0x20>,
+                     <0xff9900c0 0x20>,
+                     <0xff9900e0 0x20>;
+               reg-names = "local_request_region",
+                        "local_response_region",
+                        "remote_request_region",
+                        "remote_response_region";
+               #mbox-cells = <1>;
+               xlnx,ipi-id = <1>;
+          };
+     };
+
+...