diff mbox series

[v8,1/6] dt-bindings: remoteproc: Add Xilinx RPU subsystem bindings

Message ID 20220602203834.3675160-2-tanmay.shah@xilinx.com (mailing list archive)
State Changes Requested
Headers show
Series Add Xilinx RPU subsystem support | expand

Commit Message

Tanmay Shah June 2, 2022, 8:38 p.m. UTC
Xilinx ZynqMP platform has dual-core ARM Cortex R5 Realtime Processing
Unit(RPU) subsystem. This patch adds dt-bindings for RPU subsystem
(cluster).

Signed-off-by: Tanmay Shah <tanmay.shah@xilinx.com>
---

Changes in v8:
  - Add 'items:' for sram property

Changes in v7:
  - Add minItems in sram property

Changes in v6:
  - Add maxItems to sram and memory-region property

Changes in v5:
- Add constraints of the possible values of xlnx,cluster-mode property
- fix description of power-domains property for r5 core
- Remove reg, address-cells and size-cells properties as it is not required
- Fix description of mboxes property
- Add description of each memory-region and remove old .txt binding link
  reference in the description

Changes in v4:
  - Add memory-region, mboxes and mbox-names properties in example

Changes in v3:
  - None

 .../bindings/remoteproc/xlnx,r5f-rproc.yaml   | 130 ++++++++++++++++++
 include/dt-bindings/power/xlnx-zynqmp-power.h |   6 +
 2 files changed, 138 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml

Comments

Peter Korsgaard June 3, 2022, 6:33 a.m. UTC | #1
>>>>> "Tanmay" == Tanmay Shah <tanmay.shah@xilinx.com> writes:

Hi,

 > Xilinx ZynqMP platform has dual-core ARM Cortex R5 Realtime Processing
 > Unit(RPU) subsystem. This patch adds dt-bindings for RPU subsystem
 > (cluster).

 > Signed-off-by: Tanmay Shah <tanmay.shah@xilinx.com>
 > ---

 > Changes in v8:
 >   - Add 'items:' for sram property

 > Changes in v7:
 >   - Add minItems in sram property

 > Changes in v6:
 >   - Add maxItems to sram and memory-region property

 > Changes in v5:
 > - Add constraints of the possible values of xlnx,cluster-mode property
 > - fix description of power-domains property for r5 core
 > - Remove reg, address-cells and size-cells properties as it is not required
 > - Fix description of mboxes property
 > - Add description of each memory-region and remove old .txt binding link
 >   reference in the description

 > Changes in v4:
 >   - Add memory-region, mboxes and mbox-names properties in example

 > Changes in v3:
 >   - None

 >  .../bindings/remoteproc/xlnx,r5f-rproc.yaml   | 130 ++++++++++++++++++
 >  include/dt-bindings/power/xlnx-zynqmp-power.h |   6 +
 >  2 files changed, 138 insertions(+)
 >  create mode 100644 Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml

 > diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml
 > new file mode 100644
 > index 000000000000..adfe05ff157a
 > --- /dev/null
 > +++ b/Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml
 > @@ -0,0 +1,132 @@
 > +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
 > +%YAML 1.2
 > +---
 > +$id: http://devicetree.org/schemas/remoteproc/xlnx,r5f-rproc.yaml#
 > +$schema: http://devicetree.org/meta-schemas/core.yaml#
 > +
 > +title: Xilinx R5F processor subsystem
 > +
 > +maintainers:
 > +  - Ben Levinsky <ben.levinsky@xilinx.com>
 > +  - Tanmay Shah <tanmay.shah@xilinx.com>
 > +
 > +description: |
 > +  The Xilinx platforms include a pair of Cortex-R5F processors (RPU) for
 > +  real-time processing based on the Cortex-R5F processor core from ARM.
 > +  The Cortex-R5F processor implements the Arm v7-R architecture and includes a
 > +  floating-point unit that implements the Arm VFPv3 instruction set.
 > +
 > +properties:
 > +  compatible:
 > +    const: xlnx,zynqmp-r5fss
 > +
 > +  xlnx,cluster-mode:
 > +    $ref: /schemas/types.yaml#/definitions/uint32
 > +    enum: [0, 1, 2]

A textual mode ("dual", "lock-step", "single") would be more readable.


 > +    description: |
 > +      The RPU MPCore can operate in split mode(Dual-processor performance), Safety

space missing before "(Dual-processor"


 > +      lock-step mode(Both RPU cores execute the same code in lock-step,
 > +      clock-for-clock) or Single CPU mode (RPU core 0 can be held in reset while

"can be" sounds a bit weak, perhaps "is"


 > +      core 1 runs normally). The processor does not support dynamic configuration.
 > +      Switching between modes is only permitted immediately after a processor reset.
 > +      If set to  1 then lockstep mode and if 0 then split mode.
 > +      If set to  2 then single CPU mode. When not defined, default will be lockstep mode.

This looks a bit confusing. If you decide to stick to the numerical
modes, then at least list them in numerical order, E.G.:

 0: split
 1: lockstep
 2: single


> +
 > +patternProperties:
 > +  "^r5f-[a-f0-9]+$":
 > +    type: object
 > +    description: |
 > +      The RPU is located in the Low Power Domain of the Processor Subsystem.
 > +      Each processor includes separate L1 instruction and data caches and
 > +      tightly coupled memories (TCM). System memory is cacheable, but the TCM
 > +      memory space is non-cacheable.
 > +
 > +      Each RPU contains one 64KB memory and two 32KB memories that
 > +      are accessed via the TCM A and B port interfaces, for a total of 128KB
 > +      per processor. In lock-step mode, the processor has access to 256KB of
 > +      TCM memory.
 > +
 > +    properties:
 > +      compatible:
 > +        const: xlnx,zynqmp-r5f
 > +
 > +      power-domains:
 > +        description: RPU core PM domain specifier
 > +        maxItems: 1

A bit more detail would be good, E.G. something like arm/cpus.yaml does:

      List of phandles and PM domain specifiers, as defined by bindings of the
      PM domain provider (see also ../power_domain.txt).

And the phandle-array ref.


> +
 > +      mboxes:
 > +        minItems: 1
 > +        items:
 > +          - description: mailbox channel to send data to RPU
 > +          - description: mailbox channel to receive data from RPU
 > +
 > +      mbox-names:
 > +        minItems: 1
 > +        items:
 > +          - const: tx
 > +          - const: rx

And here as well for mailbox/mailbox.txt


 > +
 > +      sram:
 > +        $ref: /schemas/types.yaml#/definitions/phandle-array
 > +        minItems: 1
 > +        maxItems: 8
 > +        items:
 > +          maxItems: 1
 > +        description: |
 > +          phandles to one or more reserved on-chip SRAM regions. Other than TCM,
 > +          the RPU can execute instructions and access data from, the OCM memory,
 > +          the main DDR memory, and other system memories.

Drop the comma after "from"


 > +
 > +          The regions should be defined as child nodes of the respective SRAM
 > +          node, and should be defined as per the generic bindings in,

Drop the comma after "in"


 > +          Documentation/devicetree/bindings/sram/sram.yaml
 > +
 > +      memory-region:
 > +        description: |
 > +          List of phandles to the reserved memory regions associated with the
 > +          remoteproc device. This is variable and describes the memories shared with
 > +          the remote processor (e.g. remoteproc firmware and carveouts, rpmsg
 > +          vrings, ...). This reserved memory region will be allocated on DDR memory.

s/on DDR/in DDR/

 > +        minItems: 1
 > +        maxItems: 8
 > +        items:
 > +          - description: region used for RPU firmware image section
 > +          - description: vdev buffer
 > +          - description: vring0
 > +          - description: vring1
 > +        additionalItems: true
 > +
 > +    required:
 > +      - compatible
 > +      - power-domains
 > +
 > +    unevaluatedProperties: false
 > +
 > +required:
 > +  - compatible
 > +
 > +additionalProperties: false
 > +
 > +examples:
 > +  - |
 > +    r5fss: r5fss {
 > +        compatible = "xlnx,zynqmp-r5fss";
 > +        xlnx,cluster-mode = <1>;
 > +
 > +        r5f-0 {
 > +            compatible = "xlnx,zynqmp-r5f";
 > +            power-domains = <&zynqmp_firmware 0x7>;
 > +            memory-region = <&rproc_0_fw_image>, <&rpu0vdev0buffer>, <&rpu0vdev0vring0>, <&rpu0vdev0vring1>;
 > +            mboxes = <&ipi_mailbox_rpu0 0>, <&ipi_mailbox_rpu0 1>;
 > +            mbox-names = "tx", "rx";
 > +        };
 > +
 > +        r5f-1 {
 > +            compatible = "xlnx,zynqmp-r5f";
 > +            power-domains = <&zynqmp_firmware 0x8>;
 > +            memory-region = <&rproc_1_fw_image>, <&rpu1vdev0buffer>, <&rpu1vdev0vring0>, <&rpu1vdev0vring1>;
 > +            mboxes = <&ipi_mailbox_rpu1 0>, <&ipi_mailbox_rpu1 1>;
 > +            mbox-names = "tx", "rx";
 > +        };
 > +    };
 > +...
 > diff --git a/include/dt-bindings/power/xlnx-zynqmp-power.h b/include/dt-bindings/power/xlnx-zynqmp-power.h
 > index 0d9a412fd5e0..618024cbb20d 100644
 > --- a/include/dt-bindings/power/xlnx-zynqmp-power.h
 > +++ b/include/dt-bindings/power/xlnx-zynqmp-power.h
 > @@ -6,6 +6,12 @@
 >  #ifndef _DT_BINDINGS_ZYNQMP_POWER_H
 >  #define _DT_BINDINGS_ZYNQMP_POWER_H
 
 > +#define		PD_RPU_0	7
 > +#define		PD_RPU_1	8
 > +#define		PD_R5_0_ATCM	15
 > +#define		PD_R5_0_BTCM	16
 > +#define		PD_R5_1_ATCM	17
 > +#define		PD_R5_1_BTCM	18
 >  #define		PD_USB_0	22
 >  #define		PD_USB_1	23
 >  #define		PD_TTC_0	24
 > -- 

 > 2.25.1
Tanmay Shah June 7, 2022, 5:32 p.m. UTC | #2
Hi Rob,

Ping for reviews.

Thanks,

Tanmay

On 6/2/22 1:38 PM, Tanmay Shah wrote:
> Xilinx ZynqMP platform has dual-core ARM Cortex R5 Realtime Processing
> Unit(RPU) subsystem. This patch adds dt-bindings for RPU subsystem
> (cluster).
>
> Signed-off-by: Tanmay Shah <tanmay.shah@xilinx.com>
> ---
>
> Changes in v8:
>    - Add 'items:' for sram property
>
> Changes in v7:
>    - Add minItems in sram property
>
> Changes in v6:
>    - Add maxItems to sram and memory-region property
>
> Changes in v5:
> - Add constraints of the possible values of xlnx,cluster-mode property
> - fix description of power-domains property for r5 core
> - Remove reg, address-cells and size-cells properties as it is not required
> - Fix description of mboxes property
> - Add description of each memory-region and remove old .txt binding link
>    reference in the description
>
> Changes in v4:
>    - Add memory-region, mboxes and mbox-names properties in example
>
> Changes in v3:
>    - None
>
>   .../bindings/remoteproc/xlnx,r5f-rproc.yaml   | 130 ++++++++++++++++++
>   include/dt-bindings/power/xlnx-zynqmp-power.h |   6 +
>   2 files changed, 138 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml
>
> diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml
> new file mode 100644
> index 000000000000..adfe05ff157a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml
> @@ -0,0 +1,132 @@
> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/xlnx,r5f-rproc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx R5F processor subsystem
> +
> +maintainers:
> +  - Ben Levinsky <ben.levinsky@xilinx.com>
> +  - Tanmay Shah <tanmay.shah@xilinx.com>
> +
> +description: |
> +  The Xilinx platforms include a pair of Cortex-R5F processors (RPU) for
> +  real-time processing based on the Cortex-R5F processor core from ARM.
> +  The Cortex-R5F processor implements the Arm v7-R architecture and includes a
> +  floating-point unit that implements the Arm VFPv3 instruction set.
> +
> +properties:
> +  compatible:
> +    const: xlnx,zynqmp-r5fss
> +
> +  xlnx,cluster-mode:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1, 2]
> +    description: |
> +      The RPU MPCore can operate in split mode(Dual-processor performance), Safety
> +      lock-step mode(Both RPU cores execute the same code in lock-step,
> +      clock-for-clock) or Single CPU mode (RPU core 0 can be held in reset while
> +      core 1 runs normally). The processor does not support dynamic configuration.
> +      Switching between modes is only permitted immediately after a processor reset.
> +      If set to  1 then lockstep mode and if 0 then split mode.
> +      If set to  2 then single CPU mode. When not defined, default will be lockstep mode.
> +
> +patternProperties:
> +  "^r5f-[a-f0-9]+$":
> +    type: object
> +    description: |
> +      The RPU is located in the Low Power Domain of the Processor Subsystem.
> +      Each processor includes separate L1 instruction and data caches and
> +      tightly coupled memories (TCM). System memory is cacheable, but the TCM
> +      memory space is non-cacheable.
> +
> +      Each RPU contains one 64KB memory and two 32KB memories that
> +      are accessed via the TCM A and B port interfaces, for a total of 128KB
> +      per processor. In lock-step mode, the processor has access to 256KB of
> +      TCM memory.
> +
> +    properties:
> +      compatible:
> +        const: xlnx,zynqmp-r5f
> +
> +      power-domains:
> +        description: RPU core PM domain specifier
> +        maxItems: 1
> +
> +      mboxes:
> +        minItems: 1
> +        items:
> +          - description: mailbox channel to send data to RPU
> +          - description: mailbox channel to receive data from RPU
> +
> +      mbox-names:
> +        minItems: 1
> +        items:
> +          - const: tx
> +          - const: rx
> +
> +      sram:
> +        $ref: /schemas/types.yaml#/definitions/phandle-array
> +        minItems: 1
> +        maxItems: 8
> +        items:
> +          maxItems: 1
> +        description: |
> +          phandles to one or more reserved on-chip SRAM regions. Other than TCM,
> +          the RPU can execute instructions and access data from, the OCM memory,
> +          the main DDR memory, and other system memories.
> +
> +          The regions should be defined as child nodes of the respective SRAM
> +          node, and should be defined as per the generic bindings in,
> +          Documentation/devicetree/bindings/sram/sram.yaml
> +
> +      memory-region:
> +        description: |
> +          List of phandles to the reserved memory regions associated with the
> +          remoteproc device. This is variable and describes the memories shared with
> +          the remote processor (e.g. remoteproc firmware and carveouts, rpmsg
> +          vrings, ...). This reserved memory region will be allocated on DDR memory.
> +        minItems: 1
> +        maxItems: 8
> +        items:
> +          - description: region used for RPU firmware image section
> +          - description: vdev buffer
> +          - description: vring0
> +          - description: vring1
> +        additionalItems: true
> +
> +    required:
> +      - compatible
> +      - power-domains
> +
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    r5fss: r5fss {
> +        compatible = "xlnx,zynqmp-r5fss";
> +        xlnx,cluster-mode = <1>;
> +
> +        r5f-0 {
> +            compatible = "xlnx,zynqmp-r5f";
> +            power-domains = <&zynqmp_firmware 0x7>;
> +            memory-region = <&rproc_0_fw_image>, <&rpu0vdev0buffer>, <&rpu0vdev0vring0>, <&rpu0vdev0vring1>;
> +            mboxes = <&ipi_mailbox_rpu0 0>, <&ipi_mailbox_rpu0 1>;
> +            mbox-names = "tx", "rx";
> +        };
> +
> +        r5f-1 {
> +            compatible = "xlnx,zynqmp-r5f";
> +            power-domains = <&zynqmp_firmware 0x8>;
> +            memory-region = <&rproc_1_fw_image>, <&rpu1vdev0buffer>, <&rpu1vdev0vring0>, <&rpu1vdev0vring1>;
> +            mboxes = <&ipi_mailbox_rpu1 0>, <&ipi_mailbox_rpu1 1>;
> +            mbox-names = "tx", "rx";
> +        };
> +    };
> +...
> diff --git a/include/dt-bindings/power/xlnx-zynqmp-power.h b/include/dt-bindings/power/xlnx-zynqmp-power.h
> index 0d9a412fd5e0..618024cbb20d 100644
> --- a/include/dt-bindings/power/xlnx-zynqmp-power.h
> +++ b/include/dt-bindings/power/xlnx-zynqmp-power.h
> @@ -6,6 +6,12 @@
>   #ifndef _DT_BINDINGS_ZYNQMP_POWER_H
>   #define _DT_BINDINGS_ZYNQMP_POWER_H
>   
> +#define		PD_RPU_0	7
> +#define		PD_RPU_1	8
> +#define		PD_R5_0_ATCM	15
> +#define		PD_R5_0_BTCM	16
> +#define		PD_R5_1_ATCM	17
> +#define		PD_R5_1_BTCM	18
>   #define		PD_USB_0	22
>   #define		PD_USB_1	23
>   #define		PD_TTC_0	24
Rob Herring (Arm) June 9, 2022, 5:33 p.m. UTC | #3
On Tue, Jun 07, 2022 at 10:32:48AM -0700, Tanmay Shah wrote:
> Hi Rob,
> 
> Ping for reviews.

No need to ping me. You can check status of your patch here:

https://patchwork.ozlabs.org/project/devicetree-bindings/list/

If it is listed, Krzysztof or I will get to it.

Rob
Rob Herring (Arm) June 9, 2022, 5:41 p.m. UTC | #4
On Fri, Jun 03, 2022 at 08:33:13AM +0200, Peter Korsgaard wrote:
> >>>>> "Tanmay" == Tanmay Shah <tanmay.shah@xilinx.com> writes:
> 
> Hi,
> 
>  > Xilinx ZynqMP platform has dual-core ARM Cortex R5 Realtime Processing
>  > Unit(RPU) subsystem. This patch adds dt-bindings for RPU subsystem
>  > (cluster).
> 
>  > Signed-off-by: Tanmay Shah <tanmay.shah@xilinx.com>
>  > ---
> 
>  > Changes in v8:
>  >   - Add 'items:' for sram property
> 
>  > Changes in v7:
>  >   - Add minItems in sram property
> 
>  > Changes in v6:
>  >   - Add maxItems to sram and memory-region property
> 
>  > Changes in v5:
>  > - Add constraints of the possible values of xlnx,cluster-mode property
>  > - fix description of power-domains property for r5 core
>  > - Remove reg, address-cells and size-cells properties as it is not required
>  > - Fix description of mboxes property
>  > - Add description of each memory-region and remove old .txt binding link
>  >   reference in the description
> 
>  > Changes in v4:
>  >   - Add memory-region, mboxes and mbox-names properties in example
> 
>  > Changes in v3:
>  >   - None
> 
>  >  .../bindings/remoteproc/xlnx,r5f-rproc.yaml   | 130 ++++++++++++++++++
>  >  include/dt-bindings/power/xlnx-zynqmp-power.h |   6 +
>  >  2 files changed, 138 insertions(+)
>  >  create mode 100644 Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml
> 
>  > diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml
>  > new file mode 100644
>  > index 000000000000..adfe05ff157a
>  > --- /dev/null
>  > +++ b/Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml
>  > @@ -0,0 +1,132 @@
>  > +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
>  > +%YAML 1.2
>  > +---
>  > +$id: http://devicetree.org/schemas/remoteproc/xlnx,r5f-rproc.yaml#
>  > +$schema: http://devicetree.org/meta-schemas/core.yaml#
>  > +
>  > +title: Xilinx R5F processor subsystem
>  > +
>  > +maintainers:
>  > +  - Ben Levinsky <ben.levinsky@xilinx.com>
>  > +  - Tanmay Shah <tanmay.shah@xilinx.com>
>  > +
>  > +description: |
>  > +  The Xilinx platforms include a pair of Cortex-R5F processors (RPU) for
>  > +  real-time processing based on the Cortex-R5F processor core from ARM.
>  > +  The Cortex-R5F processor implements the Arm v7-R architecture and includes a
>  > +  floating-point unit that implements the Arm VFPv3 instruction set.
>  > +
>  > +properties:
>  > +  compatible:
>  > +    const: xlnx,zynqmp-r5fss
>  > +
>  > +  xlnx,cluster-mode:
>  > +    $ref: /schemas/types.yaml#/definitions/uint32
>  > +    enum: [0, 1, 2]
> 
> A textual mode ("dual", "lock-step", "single") would be more readable.
> 
> 
>  > +    description: |
>  > +      The RPU MPCore can operate in split mode(Dual-processor performance), Safety
> 
> space missing before "(Dual-processor"
> 
> 
>  > +      lock-step mode(Both RPU cores execute the same code in lock-step,
>  > +      clock-for-clock) or Single CPU mode (RPU core 0 can be held in reset while
> 
> "can be" sounds a bit weak, perhaps "is"
> 
> 
>  > +      core 1 runs normally). The processor does not support dynamic configuration.
>  > +      Switching between modes is only permitted immediately after a processor reset.
>  > +      If set to  1 then lockstep mode and if 0 then split mode.
>  > +      If set to  2 then single CPU mode. When not defined, default will be lockstep mode.
> 
> This looks a bit confusing. If you decide to stick to the numerical
> modes, then at least list them in numerical order, E.G.:
> 
>  0: split
>  1: lockstep
>  2: single

The bigger issue has been multiple Cortex-R5 bindings all doing their 
own thing for this feature which is not vendor specific (except TI has 
their own extra mode or something). How TCMs are described is the 
other issue. I've just stopped caring because no one listens.

> 
> > +
>  > +patternProperties:
>  > +  "^r5f-[a-f0-9]+$":
>  > +    type: object
>  > +    description: |
>  > +      The RPU is located in the Low Power Domain of the Processor Subsystem.
>  > +      Each processor includes separate L1 instruction and data caches and
>  > +      tightly coupled memories (TCM). System memory is cacheable, but the TCM
>  > +      memory space is non-cacheable.
>  > +
>  > +      Each RPU contains one 64KB memory and two 32KB memories that
>  > +      are accessed via the TCM A and B port interfaces, for a total of 128KB
>  > +      per processor. In lock-step mode, the processor has access to 256KB of
>  > +      TCM memory.
>  > +
>  > +    properties:
>  > +      compatible:
>  > +        const: xlnx,zynqmp-r5f
>  > +
>  > +      power-domains:
>  > +        description: RPU core PM domain specifier
>  > +        maxItems: 1
> 
> A bit more detail would be good, E.G. something like arm/cpus.yaml does:
> 
>       List of phandles and PM domain specifiers, as defined by bindings of the
>       PM domain provider (see also ../power_domain.txt).
> 
> And the phandle-array ref.

Both suggestions are wrong. We don't need common properties re-described 
by every user. They already have a type definition too, so we don't need 
to repeat phandle-array ref either.

The existing description is not needed either as it doesn't provide any 
information specific to this binding. You only need to describe each 
entry when there is more than 1 entry because we need to know what each 
entry is and the order.

> > +
>  > +      mboxes:
>  > +        minItems: 1
>  > +        items:
>  > +          - description: mailbox channel to send data to RPU
>  > +          - description: mailbox channel to receive data from RPU
>  > +
>  > +      mbox-names:
>  > +        minItems: 1
>  > +        items:
>  > +          - const: tx
>  > +          - const: rx
> 
> And here as well for mailbox/mailbox.txt

Nope!

> 
> 
>  > +
>  > +      sram:
>  > +        $ref: /schemas/types.yaml#/definitions/phandle-array
>  > +        minItems: 1
>  > +        maxItems: 8
>  > +        items:
>  > +          maxItems: 1
>  > +        description: |
>  > +          phandles to one or more reserved on-chip SRAM regions. Other than TCM,
>  > +          the RPU can execute instructions and access data from, the OCM memory,
>  > +          the main DDR memory, and other system memories.
> 
> Drop the comma after "from"
> 
> 
>  > +
>  > +          The regions should be defined as child nodes of the respective SRAM
>  > +          node, and should be defined as per the generic bindings in,
> 
> Drop the comma after "in"
> 
> 
>  > +          Documentation/devicetree/bindings/sram/sram.yaml
>  > +
>  > +      memory-region:
>  > +        description: |
>  > +          List of phandles to the reserved memory regions associated with the
>  > +          remoteproc device. This is variable and describes the memories shared with
>  > +          the remote processor (e.g. remoteproc firmware and carveouts, rpmsg
>  > +          vrings, ...). This reserved memory region will be allocated on DDR memory.
> 
> s/on DDR/in DDR/
> 
>  > +        minItems: 1
>  > +        maxItems: 8
>  > +        items:
>  > +          - description: region used for RPU firmware image section
>  > +          - description: vdev buffer
>  > +          - description: vring0
>  > +          - description: vring1
>  > +        additionalItems: true
>  > +
>  > +    required:
>  > +      - compatible
>  > +      - power-domains
>  > +
>  > +    unevaluatedProperties: false
>  > +
>  > +required:
>  > +  - compatible
>  > +
>  > +additionalProperties: false
>  > +
>  > +examples:
>  > +  - |
>  > +    r5fss: r5fss {
>  > +        compatible = "xlnx,zynqmp-r5fss";
>  > +        xlnx,cluster-mode = <1>;
>  > +
>  > +        r5f-0 {
>  > +            compatible = "xlnx,zynqmp-r5f";
>  > +            power-domains = <&zynqmp_firmware 0x7>;
>  > +            memory-region = <&rproc_0_fw_image>, <&rpu0vdev0buffer>, <&rpu0vdev0vring0>, <&rpu0vdev0vring1>;
>  > +            mboxes = <&ipi_mailbox_rpu0 0>, <&ipi_mailbox_rpu0 1>;
>  > +            mbox-names = "tx", "rx";
>  > +        };
>  > +
>  > +        r5f-1 {
>  > +            compatible = "xlnx,zynqmp-r5f";
>  > +            power-domains = <&zynqmp_firmware 0x8>;
>  > +            memory-region = <&rproc_1_fw_image>, <&rpu1vdev0buffer>, <&rpu1vdev0vring0>, <&rpu1vdev0vring1>;
>  > +            mboxes = <&ipi_mailbox_rpu1 0>, <&ipi_mailbox_rpu1 1>;
>  > +            mbox-names = "tx", "rx";
>  > +        };
>  > +    };
>  > +...
>  > diff --git a/include/dt-bindings/power/xlnx-zynqmp-power.h b/include/dt-bindings/power/xlnx-zynqmp-power.h
>  > index 0d9a412fd5e0..618024cbb20d 100644
>  > --- a/include/dt-bindings/power/xlnx-zynqmp-power.h
>  > +++ b/include/dt-bindings/power/xlnx-zynqmp-power.h
>  > @@ -6,6 +6,12 @@
>  >  #ifndef _DT_BINDINGS_ZYNQMP_POWER_H
>  >  #define _DT_BINDINGS_ZYNQMP_POWER_H
>  
>  > +#define		PD_RPU_0	7
>  > +#define		PD_RPU_1	8
>  > +#define		PD_R5_0_ATCM	15
>  > +#define		PD_R5_0_BTCM	16
>  > +#define		PD_R5_1_ATCM	17
>  > +#define		PD_R5_1_BTCM	18
>  >  #define		PD_USB_0	22
>  >  #define		PD_USB_1	23
>  >  #define		PD_TTC_0	24
>  > -- 
> 
>  > 2.25.1
> 
> 
> -- 
> Bye, Peter Korsgaard
>
Tanmay Shah June 10, 2022, 4:17 p.m. UTC | #5
HI Rob,

Thanks for reviews. Please find my comments below.


On 6/9/22 10:41 AM, Rob Herring wrote:
> On Fri, Jun 03, 2022 at 08:33:13AM +0200, Peter Korsgaard wrote:
>>>>>>> "Tanmay" == Tanmay Shah <tanmay.shah@xilinx.com> writes:
>> Hi,
>>
>>   > Xilinx ZynqMP platform has dual-core ARM Cortex R5 Realtime Processing
>>   > Unit(RPU) subsystem. This patch adds dt-bindings for RPU subsystem
>>   > (cluster).
>>
>>   > Signed-off-by: Tanmay Shah <tanmay.shah@xilinx.com>
>>   > ---
>>
>>   > Changes in v8:
>>   >   - Add 'items:' for sram property
>>
>>   > Changes in v7:
>>   >   - Add minItems in sram property
>>
>>   > Changes in v6:
>>   >   - Add maxItems to sram and memory-region property
>>
>>   > Changes in v5:
>>   > - Add constraints of the possible values of xlnx,cluster-mode property
>>   > - fix description of power-domains property for r5 core
>>   > - Remove reg, address-cells and size-cells properties as it is not required
>>   > - Fix description of mboxes property
>>   > - Add description of each memory-region and remove old .txt binding link
>>   >   reference in the description
>>
>>   > Changes in v4:
>>   >   - Add memory-region, mboxes and mbox-names properties in example
>>
>>   > Changes in v3:
>>   >   - None
>>
>>   >  .../bindings/remoteproc/xlnx,r5f-rproc.yaml   | 130 ++++++++++++++++++
>>   >  include/dt-bindings/power/xlnx-zynqmp-power.h |   6 +
>>   >  2 files changed, 138 insertions(+)
>>   >  create mode 100644 Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml
>>
>>   > diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml
>>   > new file mode 100644
>>   > index 000000000000..adfe05ff157a
>>   > --- /dev/null
>>   > +++ b/Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml
>>   > @@ -0,0 +1,132 @@
>>   > +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
>>   > +%YAML 1.2
>>   > +---
>>   > +$id: http://devicetree.org/schemas/remoteproc/xlnx,r5f-rproc.yaml#
>>   > +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>   > +
>>   > +title: Xilinx R5F processor subsystem
>>   > +
>>   > +maintainers:
>>   > +  - Ben Levinsky <ben.levinsky@xilinx.com>
>>   > +  - Tanmay Shah <tanmay.shah@xilinx.com>
>>   > +
>>   > +description: |
>>   > +  The Xilinx platforms include a pair of Cortex-R5F processors (RPU) for
>>   > +  real-time processing based on the Cortex-R5F processor core from ARM.
>>   > +  The Cortex-R5F processor implements the Arm v7-R architecture and includes a
>>   > +  floating-point unit that implements the Arm VFPv3 instruction set.
>>   > +
>>   > +properties:
>>   > +  compatible:
>>   > +    const: xlnx,zynqmp-r5fss
>>   > +
>>   > +  xlnx,cluster-mode:
>>   > +    $ref: /schemas/types.yaml#/definitions/uint32
>>   > +    enum: [0, 1, 2]
>>
>> A textual mode ("dual", "lock-step", "single") would be more readable.
>>
>>
>>   > +    description: |
>>   > +      The RPU MPCore can operate in split mode(Dual-processor performance), Safety
>>
>> space missing before "(Dual-processor"
>>
>>
>>   > +      lock-step mode(Both RPU cores execute the same code in lock-step,
>>   > +      clock-for-clock) or Single CPU mode (RPU core 0 can be held in reset while
>>
>> "can be" sounds a bit weak, perhaps "is"
>>
>>
>>   > +      core 1 runs normally). The processor does not support dynamic configuration.
>>   > +      Switching between modes is only permitted immediately after a processor reset.
>>   > +      If set to  1 then lockstep mode and if 0 then split mode.
>>   > +      If set to  2 then single CPU mode. When not defined, default will be lockstep mode.
>>
>> This looks a bit confusing. If you decide to stick to the numerical
>> modes, then at least list them in numerical order, E.G.:
>>
>>   0: split
>>   1: lockstep
>>   2: single

I think the description is fine. for readability I can just add 
numerical order as above.


> The bigger issue has been multiple Cortex-R5 bindings all doing their
> own thing for this feature which is not vendor specific (except TI has
> their own extra mode or something). How TCMs are described is the
> other issue. I've just stopped caring because no one listens.
Values used for cluster-mode property for xlnx platform is same as ti's 
cluster mode property.

0: split, 1: lockstep and 2: single-cpu mode. It is matching with 
ti,k3-r5 bindings.

However, default value for some of the ti platforms are different.

xlnx platforms have single-cpu mode as well.

It will take some time to design system-dt spec for TCM.

So, for now I use hard-code values of TCM from driver.

TCM bindings for linux will be posted once system-dt spec is accepted.

Meanwhile we would like to upstream rest of the bindings and driver.


>
>>> +
>>   > +patternProperties:
>>   > +  "^r5f-[a-f0-9]+$":
>>   > +    type: object
>>   > +    description: |
>>   > +      The RPU is located in the Low Power Domain of the Processor Subsystem.
>>   > +      Each processor includes separate L1 instruction and data caches and
>>   > +      tightly coupled memories (TCM). System memory is cacheable, but the TCM
>>   > +      memory space is non-cacheable.
>>   > +
>>   > +      Each RPU contains one 64KB memory and two 32KB memories that
>>   > +      are accessed via the TCM A and B port interfaces, for a total of 128KB
>>   > +      per processor. In lock-step mode, the processor has access to 256KB of
>>   > +      TCM memory.
>>   > +
>>   > +    properties:
>>   > +      compatible:
>>   > +        const: xlnx,zynqmp-r5f
>>   > +
>>   > +      power-domains:
>>   > +        description: RPU core PM domain specifier
>>   > +        maxItems: 1
>>
>> A bit more detail would be good, E.G. something like arm/cpus.yaml does:
>>
>>        List of phandles and PM domain specifiers, as defined by bindings of the
>>        PM domain provider (see also ../power_domain.txt).
>>
>> And the phandle-array ref.
> Both suggestions are wrong. We don't need common properties re-described
> by every user. They already have a type definition too, so we don't need
> to repeat phandle-array ref either.
>
> The existing description is not needed either as it doesn't provide any
> information specific to this binding. You only need to describe each
> entry when there is more than 1 entry because we need to know what each
> entry is and the order.

I will remove current description as well.

Thanks.

>
>>> +
>>   > +      mboxes:
>>   > +        minItems: 1
>>   > +        items:
>>   > +          - description: mailbox channel to send data to RPU
>>   > +          - description: mailbox channel to receive data from RPU
>>   > +
>>   > +      mbox-names:
>>   > +        minItems: 1
>>   > +        items:
>>   > +          - const: tx
>>   > +          - const: rx
>>
>> And here as well for mailbox/mailbox.txt
> Nope!
>
>>
>>   > +
>>   > +      sram:
>>   > +        $ref: /schemas/types.yaml#/definitions/phandle-array
>>   > +        minItems: 1
>>   > +        maxItems: 8
>>   > +        items:
>>   > +          maxItems: 1
>>   > +        description: |
>>   > +          phandles to one or more reserved on-chip SRAM regions. Other than TCM,
>>   > +          the RPU can execute instructions and access data from, the OCM memory,
>>   > +          the main DDR memory, and other system memories.
>>
>> Drop the comma after "from"
>>
>>
>>   > +
>>   > +          The regions should be defined as child nodes of the respective SRAM
>>   > +          node, and should be defined as per the generic bindings in,
>>
>> Drop the comma after "in"
>>
>>
>>   > +          Documentation/devicetree/bindings/sram/sram.yaml
>>   > +
>>   > +      memory-region:
>>   > +        description: |
>>   > +          List of phandles to the reserved memory regions associated with the
>>   > +          remoteproc device. This is variable and describes the memories shared with
>>   > +          the remote processor (e.g. remoteproc firmware and carveouts, rpmsg
>>   > +          vrings, ...). This reserved memory region will be allocated on DDR memory.
>>
>> s/on DDR/in DDR/
>>
>>   > +        minItems: 1
>>   > +        maxItems: 8
>>   > +        items:
>>   > +          - description: region used for RPU firmware image section
>>   > +          - description: vdev buffer
>>   > +          - description: vring0
>>   > +          - description: vring1
>>   > +        additionalItems: true
>>   > +
>>   > +    required:
>>   > +      - compatible
>>   > +      - power-domains
>>   > +
>>   > +    unevaluatedProperties: false
>>   > +
>>   > +required:
>>   > +  - compatible
>>   > +
>>   > +additionalProperties: false
>>   > +
>>   > +examples:
>>   > +  - |
>>   > +    r5fss: r5fss {
>>   > +        compatible = "xlnx,zynqmp-r5fss";
>>   > +        xlnx,cluster-mode = <1>;
>>   > +
>>   > +        r5f-0 {
>>   > +            compatible = "xlnx,zynqmp-r5f";
>>   > +            power-domains = <&zynqmp_firmware 0x7>;
>>   > +            memory-region = <&rproc_0_fw_image>, <&rpu0vdev0buffer>, <&rpu0vdev0vring0>, <&rpu0vdev0vring1>;
>>   > +            mboxes = <&ipi_mailbox_rpu0 0>, <&ipi_mailbox_rpu0 1>;
>>   > +            mbox-names = "tx", "rx";
>>   > +        };
>>   > +
>>   > +        r5f-1 {
>>   > +            compatible = "xlnx,zynqmp-r5f";
>>   > +            power-domains = <&zynqmp_firmware 0x8>;
>>   > +            memory-region = <&rproc_1_fw_image>, <&rpu1vdev0buffer>, <&rpu1vdev0vring0>, <&rpu1vdev0vring1>;
>>   > +            mboxes = <&ipi_mailbox_rpu1 0>, <&ipi_mailbox_rpu1 1>;
>>   > +            mbox-names = "tx", "rx";
>>   > +        };
>>   > +    };
>>   > +...
>>   > diff --git a/include/dt-bindings/power/xlnx-zynqmp-power.h b/include/dt-bindings/power/xlnx-zynqmp-power.h
>>   > index 0d9a412fd5e0..618024cbb20d 100644
>>   > --- a/include/dt-bindings/power/xlnx-zynqmp-power.h
>>   > +++ b/include/dt-bindings/power/xlnx-zynqmp-power.h
>>   > @@ -6,6 +6,12 @@
>>   >  #ifndef _DT_BINDINGS_ZYNQMP_POWER_H
>>   >  #define _DT_BINDINGS_ZYNQMP_POWER_H
>>   
>>   > +#define		PD_RPU_0	7
>>   > +#define		PD_RPU_1	8
>>   > +#define		PD_R5_0_ATCM	15
>>   > +#define		PD_R5_0_BTCM	16
>>   > +#define		PD_R5_1_ATCM	17
>>   > +#define		PD_R5_1_BTCM	18
>>   >  #define		PD_USB_0	22
>>   >  #define		PD_USB_1	23
>>   >  #define		PD_TTC_0	24
>>   > --
>>
>>   > 2.25.1
>>
>>
>> -- 
>> Bye, Peter Korsgaard
>>
Tanmay Shah June 10, 2022, 4:18 p.m. UTC | #6
On 6/9/22 10:33 AM, Rob Herring wrote:
> On Tue, Jun 07, 2022 at 10:32:48AM -0700, Tanmay Shah wrote:
>> Hi Rob,
>>
>> Ping for reviews.
> No need to ping me. You can check status of your patch here:
>
> https://patchwork.ozlabs.org/project/devicetree-bindings/list/
>
> If it is listed, Krzysztof or I will get to it.

Thanks for the link. This is good to know.

> Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml
new file mode 100644
index 000000000000..adfe05ff157a
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/xlnx,r5f-rproc.yaml
@@ -0,0 +1,132 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/remoteproc/xlnx,r5f-rproc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx R5F processor subsystem
+
+maintainers:
+  - Ben Levinsky <ben.levinsky@xilinx.com>
+  - Tanmay Shah <tanmay.shah@xilinx.com>
+
+description: |
+  The Xilinx platforms include a pair of Cortex-R5F processors (RPU) for
+  real-time processing based on the Cortex-R5F processor core from ARM.
+  The Cortex-R5F processor implements the Arm v7-R architecture and includes a
+  floating-point unit that implements the Arm VFPv3 instruction set.
+
+properties:
+  compatible:
+    const: xlnx,zynqmp-r5fss
+
+  xlnx,cluster-mode:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2]
+    description: |
+      The RPU MPCore can operate in split mode(Dual-processor performance), Safety
+      lock-step mode(Both RPU cores execute the same code in lock-step,
+      clock-for-clock) or Single CPU mode (RPU core 0 can be held in reset while
+      core 1 runs normally). The processor does not support dynamic configuration.
+      Switching between modes is only permitted immediately after a processor reset.
+      If set to  1 then lockstep mode and if 0 then split mode.
+      If set to  2 then single CPU mode. When not defined, default will be lockstep mode.
+
+patternProperties:
+  "^r5f-[a-f0-9]+$":
+    type: object
+    description: |
+      The RPU is located in the Low Power Domain of the Processor Subsystem.
+      Each processor includes separate L1 instruction and data caches and
+      tightly coupled memories (TCM). System memory is cacheable, but the TCM
+      memory space is non-cacheable.
+
+      Each RPU contains one 64KB memory and two 32KB memories that
+      are accessed via the TCM A and B port interfaces, for a total of 128KB
+      per processor. In lock-step mode, the processor has access to 256KB of
+      TCM memory.
+
+    properties:
+      compatible:
+        const: xlnx,zynqmp-r5f
+
+      power-domains:
+        description: RPU core PM domain specifier
+        maxItems: 1
+
+      mboxes:
+        minItems: 1
+        items:
+          - description: mailbox channel to send data to RPU
+          - description: mailbox channel to receive data from RPU
+
+      mbox-names:
+        minItems: 1
+        items:
+          - const: tx
+          - const: rx
+
+      sram:
+        $ref: /schemas/types.yaml#/definitions/phandle-array
+        minItems: 1
+        maxItems: 8
+        items:
+          maxItems: 1
+        description: |
+          phandles to one or more reserved on-chip SRAM regions. Other than TCM,
+          the RPU can execute instructions and access data from, the OCM memory,
+          the main DDR memory, and other system memories.
+
+          The regions should be defined as child nodes of the respective SRAM
+          node, and should be defined as per the generic bindings in,
+          Documentation/devicetree/bindings/sram/sram.yaml
+
+      memory-region:
+        description: |
+          List of phandles to the reserved memory regions associated with the
+          remoteproc device. This is variable and describes the memories shared with
+          the remote processor (e.g. remoteproc firmware and carveouts, rpmsg
+          vrings, ...). This reserved memory region will be allocated on DDR memory.
+        minItems: 1
+        maxItems: 8
+        items:
+          - description: region used for RPU firmware image section
+          - description: vdev buffer
+          - description: vring0
+          - description: vring1
+        additionalItems: true
+
+    required:
+      - compatible
+      - power-domains
+
+    unevaluatedProperties: false
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    r5fss: r5fss {
+        compatible = "xlnx,zynqmp-r5fss";
+        xlnx,cluster-mode = <1>;
+
+        r5f-0 {
+            compatible = "xlnx,zynqmp-r5f";
+            power-domains = <&zynqmp_firmware 0x7>;
+            memory-region = <&rproc_0_fw_image>, <&rpu0vdev0buffer>, <&rpu0vdev0vring0>, <&rpu0vdev0vring1>;
+            mboxes = <&ipi_mailbox_rpu0 0>, <&ipi_mailbox_rpu0 1>;
+            mbox-names = "tx", "rx";
+        };
+
+        r5f-1 {
+            compatible = "xlnx,zynqmp-r5f";
+            power-domains = <&zynqmp_firmware 0x8>;
+            memory-region = <&rproc_1_fw_image>, <&rpu1vdev0buffer>, <&rpu1vdev0vring0>, <&rpu1vdev0vring1>;
+            mboxes = <&ipi_mailbox_rpu1 0>, <&ipi_mailbox_rpu1 1>;
+            mbox-names = "tx", "rx";
+        };
+    };
+...
diff --git a/include/dt-bindings/power/xlnx-zynqmp-power.h b/include/dt-bindings/power/xlnx-zynqmp-power.h
index 0d9a412fd5e0..618024cbb20d 100644
--- a/include/dt-bindings/power/xlnx-zynqmp-power.h
+++ b/include/dt-bindings/power/xlnx-zynqmp-power.h
@@ -6,6 +6,12 @@ 
 #ifndef _DT_BINDINGS_ZYNQMP_POWER_H
 #define _DT_BINDINGS_ZYNQMP_POWER_H
 
+#define		PD_RPU_0	7
+#define		PD_RPU_1	8
+#define		PD_R5_0_ATCM	15
+#define		PD_R5_0_BTCM	16
+#define		PD_R5_1_ATCM	17
+#define		PD_R5_1_BTCM	18
 #define		PD_USB_0	22
 #define		PD_USB_1	23
 #define		PD_TTC_0	24