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

[Tanmay Shah <tanmay.shah@xilinx.com>]

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
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel