Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 3/7] dt-bindings: remoteproc: Add bindings for R5F
 subsystem on TI K3 SoCs
From:   Suman Anna <s-anna@ti.com>
To:     Rob Herring <robh+dt@kernel.org>
CC:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        Lokesh Vutla <lokeshvutla@ti.com>,
        "open list:REMOTE PROCESSOR (REMOTEPROC) SUBSYSTEM" 
        <linux-remoteproc@vger.kernel.org>, <devicetree@vger.kernel.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20200324201819.23095-1-s-anna@ti.com>
 <20200324201819.23095-4-s-anna@ti.com>
 <CAL_JsqKpC=W-y3OdeqSROhiKLbQKf3sGyCXzQH__Wr-O=QS4Tg@mail.gmail.com>
 <30cb4bee-fe79-8683-8225-fad174e0d630@ti.com>
Message-ID: <5eafcf26-edfe-a8a3-f0eb-90def3eafa4c@ti.com>
Date:   Wed, 8 Apr 2020 19:15:40 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <30cb4bee-fe79-8683-8225-fad174e0d630@ti.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 4/8/20 7:02 PM, Suman Anna wrote:
> Hi Rob,
> 
> On 3/26/20 11:53 AM, Rob Herring wrote:
>> On Tue, Mar 24, 2020 at 2:18 PM Suman Anna <s-anna@ti.com> wrote:
>>>
>>> The Texas Instruments K3 family of SoCs have one or more dual-core
>>> Arm Cortex R5F processor subsystems/clusters (R5FSS). The clusters
>>> can be split between multiple voltage domains as well. Add the device
>>> tree bindings document for these R5F subsystem devices. These R5F
>>> processors do not have an MMU, and so require fixed memory carveout
>>> regions matching the firmware image addresses. The nodes require more
>>> than one memory region, with the first memory region used for DMA
>>> allocations at runtime. The remaining memory regions are reserved
>>> and are used for the loading and running of the R5F remote processors.
>>> The R5F processors can also optionally use any internal on-chip SRAM
>>> memories either for executing code or using it as fast-access data.
>>
>> I'm inclined to say the system DT stuff should be sorted out before
>> accepting this. Is the system DT stuff going to be useful for your R5
>> cores? Do you really want to be stuck with this binding?
> 
> Hmm, I am not dependent on System DT and prefer to be not gated by that.
> This is still all from the Linux host perspective, and we don't have any
> plans to use DT on the firmware-side.
> 
>>
>>> The added example illustrates the DT nodes for the single R5FSS device
>>> present on K3 AM65x family of SoCs.
>>>
>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>> ---
>>> Hi Rob,
>>>
>>> The dt_bindings_check seems to throw couple of warnings around the
>>> usage of ranges because the tooling is adding the #address-cells
>>> and #size-cells of 1 by default, whereas our actual code uses 2.
>>
>> Then change the default by specifying what you want. Or change the
>> example to be 1 cell. It is *just* an example.
> 
> OK, was using the actual dts nodes as how they would be added in our dts
> files. The only way to get rid of the warnings is to use 1 cell. I can
> do that for the R5F bindings, but cannot really do that for the DSPs
> since the addresses need 2 cells.
> 
>>
>>> No issues are found with dtbs_check.
>>
>> I doubt that if your dts matches the example.
> 
> The top-level cells value is 2 in our dts files (See either of
> arm64/dts/ti/k3-am65.dtsi or k3-j721e.dtsi).
> 
>>
>>>
>>> regards
>>> Suman
>>>
>>>  .../bindings/remoteproc/ti,k3-r5f-rproc.yaml  | 338 ++++++++++++++++++
>>>  1 file changed, 338 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
>>> new file mode 100644
>>> index 000000000000..bbfc1e6ae884
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
>>> @@ -0,0 +1,338 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/remoteproc/ti,k3-r5f-rproc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: TI K3 R5F processor subsystems
>>> +
>>> +maintainers:
>>> +  - Suman Anna <s-anna@ti.com>
>>> +
>>> +description: |
>>> +  The TI K3 family of SoCs usually have one or more dual-core Arm Cortex R5F
>>> +  processor subsystems/clusters (R5FSS). The dual core cluster can be used
>>> +  either in a LockStep mode providing safety/fault tolerance features or in a
>>> +  Split mode providing two individual compute cores for doubling the compute
>>> +  capacity. These are used together with other processors present on the SoC
>>> +  to achieve various system level goals.
>>> +
>>> +  Each Dual-Core R5F sub-system is represented as a single DTS node
>>> +  representing the cluster, with a pair of child DT nodes representing
>>> +  the individual R5F cores. Each node has a number of required or optional
>>> +  properties that enable the OS running on the host processor to perform
>>> +  the device management of the remote processor and to communicate with the
>>> +  remote processor.
>>> +
>>> +# Required properties:
>>> +# --------------------
>>> +# The following are the mandatory properties:
>>> +
>>> +properties:
>>> +  $nodename:
>>> +    pattern: "^r5fss(@.*)?"
>>> +
>>> +  compatible:
>>> +    enum:
>>> +      - ti,am654-r5fss
>>> +      - ti,j721e-r5fss
>>> +
>>> +  power-domains:
>>> +    description: |
>>> +      Should contain a phandle to a PM domain provider node and an args
>>> +      specifier containing the R5FSS device id value. This property is
>>> +      as per the binding,
>>> +      Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>
>> What implementation of power domains is used is outside the scope of
>> this binding. I'd just drop the whole description as it is pretty
>> generic.
> 
> OK.
> 
>>
>>> +    maxItems: 1
>>> +
>>> +  "#address-cells":
>>> +    const: 1
>>> +
>>> +  "#size-cells":
>>> +    const: 1
>>> +
>>> +  ranges:
>>> +    description: |
>>> +      Standard ranges definition providing address translations for
>>> +      local R5F TCM address spaces to bus addresses.
>>> +
>>> +# Optional properties:
>>> +# --------------------
>>> +
>>> +  lockstep-mode:
>>
>> Needs a vendor prefix.
> 
> Yep, will fix this one and all the others below.
> 
>>
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [0, 1]
>>> +    description: |
>>> +      Configuration Mode for the Dual R5F cores within the R5F
>>> +      cluster. Should be either a value of 1 (LockStep mode) or
>>> +      0 (Split mode), default is LockStep mode if omitted.
>>> +
>>> +# R5F Processor Child Nodes:
>>> +# ==========================
>>> +
>>> +patternProperties:
>>> +  "^r5f@[a-f0-9]+$":
>>> +    type: object
>>> +    description: |
>>> +        The R5F Sub-System device node should define two R5F child nodes, each
>>> +        node representing a TI instantiation of the Arm Cortex R5F core. There
>>> +        are some specific integration differences for the IP like the usage of
>>> +        a Region Address Translator (RAT) for translating the larger SoC bus
>>> +        addresses into a 32-bit address space for the processor.
>>> +
>>> +# Required properties:
>>> +# --------------------
>>> +# The following are the mandatory properties:
>>> +
>>> +    properties:
>>> +      compatible:
>>> +        enum:
>>> +          - ti,am654-r5f
>>> +          - ti,j721e-r5f
>>> +
>>> +      reg:
>>> +        description: |
>>> +          Should contain an entry for each value in 'reg-names'.
>>> +          Each entry should have the memory region's start address
>>> +          and the size of the region, the representation matching
>>> +          the parent node's '#address-cells' and '#size-cells' values.
>>
>> That's every 'reg' property.
>>
>>> +        maxItems: 2
>>
>> You need to define what each one is:
>>
>> items:
>>   - description: ...
>>   - description: ...
> 
> OK, will fix.
> 
>>
>>> +
>>> +      reg-names:
>>> +        description: |
>>> +          Should contain strings with the names of the specific internal
>>> +          internal memory regions, and should be defined in this order
>>> +        maxItems: 2
>>> +        items:
>>> +          - const: atcm
>>> +          - const: btcm
>>> +
>>> +      ti,sci:
>>> +        $ref: /schemas/types.yaml#/definitions/phandle
>>> +        description:
>>> +          Should be a phandle to the TI-SCI System Controller node
>>> +
>>> +      ti,sci-dev-id:
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +        description: |
>>> +          Should contain the TI-SCI device id corresponding to the R5F core.
>>> +          Please refer to the corresponding System Controller documentation
>>> +          for valid values for the R5F cores.
>>> +
>>> +      ti,sci-proc-ids:
>>> +        description: Should contain a single tuple of <proc_id host_id>.
>>> +        allOf:
>>> +          - $ref: /schemas/types.yaml#/definitions/uint32-matrix
>>
>> Sounds more like an array.
> 
> OK, I can modify this. Went with this originally to reflect the tuple,
> but guess both translate similarly for my usage.
> 
>>
>>> +          - maxItems: 1
>>> +            items:
>>> +              items:
>>> +                - description: TI-SCI processor id for the R5F core device
>>> +                - description: TI-SCI host id to which processor control
>>> +                               ownership should be transferred to
>>> +
>>> +      resets:
>>> +        description: |
>>> +          Should contain the phandle to the reset controller node
>>> +          managing the resets for this device, and a reset
>>> +          specifier. Please refer to the following reset bindings
>>> +          for the reset argument specifier,
>>> +          Documentation/devicetree/bindings/reset/ti,sci-reset.txt
>>> +            for AM65x and J721E SoCs
>>
>> Drop. How many resets (maxItems or items list)?
> 
> Yeah, this is 1, will update. Do you want me to drop just the specifier
> link or the entire description?
> 
>>
>>> +
>>> +      firmware-name:
>>> +        description: |
>>> +          Should contain the name of the default firmware image
>>> +          file located on the firmware search path
>>> +
>>> +# The following properties are mandatory for R5F Core0 in both LockStep and Split
>>> +# modes, and are mandatory for R5F Core1 _only_ in Split mode. They are unused for
>>> +# R5F Core1 in LockStep mode:

Rob,

If I were to actually encode this in YAML, I need to distinguish Core0
from Core1. I have currently followed the DT generic node name
convention when defining the two nodes and interpret in the driver based
on addresses. What is the preferred mechanism for this - define node
names as r5f0 and r5f1 or introduce a ti,core-id or ti,cpu-id property?

regards
Suman

>>> +
>>> +      mboxes:
>>> +        description: |
>>> +          OMAP Mailbox specifier denoting the sub-mailbox, to be used for
>>> +          communication with the remote processor. This property should match
>>> +          with the sub-mailbox node used in the firmware image. The specifier
>>> +          format is as per the bindings,
>>> +          Documentation/devicetree/bindings/mailbox/omap-mailbox.txt
>>
>> How many?
> 
> OK, will fix.
> 
>>
>>> +
>>> +      memory-region:
>>> +        minItems: 2
>>> +        description: |
>>> +          phandle to the reserved memory nodes to be associated with the remoteproc
>>> +          device. There should be atleast two reserved memory nodes defined - the
>>
>> What's the max number? As is, it will be 2.
> 
> Aah, I misinterpreted that not having would be open-ended. OK, I will
> have to give an arbitrary number here (maybe 4 or 8). Can we update this
> later on if a usecase really needs more?
> 
>>
>>> +          first one would be used for dynamic DMA allocations like vrings and vring
>>> +          buffers, and the remaining ones used for the firmware image sections. The
>>> +          reserved memory nodes should be carveout nodes, and should be defined as
>>> +          per the bindings in
>>> +          Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>> +
>>> +# Optional properties:
>>> +# --------------------
>>> +# The following properties are optional properties for each of the R5F cores:
>>> +
>>> +      atcm-enable:
>>
>> Vendor prefix needed.
>>
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +        enum: [0, 1]
>>> +        description: |
>>> +          R5F core configuration mode dictating if ATCM should be enabled. R5F
>>> +          view of ATCM dictated by loczrama property. Should be either a value
>>> +          of 1 (enabled) or 0 (disabled), default is disabled if omitted.
>>> +
>>> +      btcm-enable:
>>
>> ditto
>>
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +        enum: [0, 1]
>>> +        description: |
>>> +          R5F core configuration mode dictating if BTCM should be enabled. R5F
>>> +          view of BTCM dictated by loczrama property. Should be either a value
>>> +          of 1 (enabled) or 0 (disabled), default is enabled if omitted.
>>> +
>>> +      loczrama:
>>
>> ditto
>>
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +        enum: [0, 1]
>>> +        description: |
>>> +          R5F core configuration mode dictating which TCM should appear at
>>> +          address 0 (from core's view). Should be either a value of 1 (ATCM
>>> +          at 0x0) or 0 (BTCM at 0x0), default value is 1 if omitted.
>>
>> I can't decipher how you came up with 'loczrama' based on the description.
> 
> That's actually the signal name from the Arm R5 specs.
> 
>>
>>> +
>>> +      sram:
>>> +        $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +        minItems: 1
>>> +        description: |
>>> +          pHandles to one or more reserved on-chip SRAM region. 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
>>> +
>>> +    required:
>>> +     - compatible
>>> +     - reg
>>> +     - reg-names
>>> +     - ti,sci
>>> +     - ti,sci-dev-id
>>> +     - ti,sci-proc-ids
>>> +     - resets
>>> +     - firmware-name
>>> +
>>> +    additionalProperties: false
>>> +
>>> +required:
>>> + - compatible
>>> + - power-domains
>>> + - "#address-cells"
>>> + - "#size-cells"
>>> + - ranges
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +
>>> +    //Example: AM654 SoC
>>> +    /* R5F DDR Carveout reserved memory nodes */
>>> +    reserved-memory {
>>> +        #address-cells = <2>;
>>> +        #size-cells = <2>;
>>> +        ranges;
>>> +
>>> +        mcu_r5fss0_core1_dma_memory_region: r5f-dma-memory@9b000000 {
>>> +            compatible = "shared-dma-pool";
>>> +            reg = <0x00 0x9b000000 0x00 0x100000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        mcu_r5fss0_core1_memory_region: r5f-memory@9b100000 {
>>> +            compatible = "shared-dma-pool";
>>> +            reg = <0x00 0x9b100000 0x00 0xf00000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        mcu_r5fss0_core0_dma_memory_region: r5f-dma-memory@9c000000 {
>>> +            compatible = "shared-dma-pool";
>>> +            reg = <0x00 0x9c000000 0x00 0x100000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        mcu_r5fss0_core0_memory_region: r5f-memory@9c100000 {
>>> +            compatible = "shared-dma-pool";
>>> +            reg = <0x00 0x9c100000 0x00 0x700000>;
>>> +            no-map;
>>> +        };
>>> +    };
>>> +
>>> +    cbass_main: interconnect@100000 {
>>
>> bus@...
> 
> Yeah, will update the example to use bus. The DTS nodes in the kernel
> are already using the interconnect name.
> 
>>
>> Doesn't look like the right address either.
> 
> Yeah, I skipped the actual first entry from the ranges, and only
> mentioned the ones that I am using in the nodes. Will fix this.
> 
>>
>>> +        compatible = "simple-bus";
>>> +        #address-cells = <2>;
>>> +        #size-cells = <2>;
>>> +        ranges = <0x00 0x41000000 0x00 0x41000000 0x00 0x00020000>,
>>> +                 <0x00 0x41400000 0x00 0x41400000 0x00 0x00020000>,
>>> +                 <0x00 0x41c00000 0x00 0x41c00000 0x00 0x00080000>;
>>> +
>>> +        cbass_mcu: interconnect@28380000 {
>>
>> Doesn't look like the right address.
> 
> Same as above.
> 
>>
>>> +            compatible = "simple-bus";
>>> +            #address-cells = <2>;
>>> +            #size-cells = <2>;
>>> +            ranges = <0x00 0x41000000 0x00 0x41000000 0x00 0x00020000>, /* MCU R5F Core0 */
>>> +                     <0x00 0x41400000 0x00 0x41400000 0x00 0x00020000>, /* MCU R5F Core1 */
>>> +                     <0x00 0x41c00000 0x00 0x41c00000 0x00 0x00080000>; /* MCU SRAM */
>>> +
>>> +            /* MCU domain SRAM node */
>>> +            mcu_ram: mcu-ram@41c00000 {
>>
>> I would omit this node from the example. Nothing special here really.
> 
> Showcasing the optional sram property usage from the mcu_r5f0 node.
> 
> regards
> Suman
> 
>>
>>> +                compatible = "mmio-sram";
>>> +                reg = <0x00 0x41c00000 0x00 0x80000>;
>>> +                ranges = <0x0 0x00 0x41c00000 0x80000>;
>>> +                #address-cells = <1>;
>>> +                #size-cells = <1>;
>>> +
>>> +                mcu_r5fss0_core0_sram: r5f-sram@0 {
>>> +                    reg = <0x0 0x40000>;
>>> +                };
>>> +            };
>>> +
>>> +            /* AM65x MCU R5FSS node */
>>> +            mcu_r5fss0: r5fss@41000000 {
>>> +                compatible = "ti,am654-r5fss";
>>> +                power-domains = <&k3_pds 129>;
>>> +                lockstep-mode = <1>;
>>> +                #address-cells = <1>;
>>> +                #size-cells = <1>;
>>> +                ranges = <0x41000000 0x00 0x41000000 0x20000>,
>>> +                         <0x41400000 0x00 0x41400000 0x20000>;
>>> +
>>> +                mcu_r5f0: r5f@41000000 {
>>> +                    compatible = "ti,am654-r5f";
>>> +                    reg = <0x41000000 0x00008000>,
>>> +                          <0x41010000 0x00008000>;
>>> +                    reg-names = "atcm", "btcm";
>>> +                    ti,sci = <&dmsc>;
>>> +                    ti,sci-dev-id = <159>;
>>> +                    ti,sci-proc-ids = <0x01 0xFF>;
>>> +                    resets = <&k3_reset 159 1>;
>>> +                    firmware-name = "am65x-mcu-r5f0_0-fw";
>>> +                    atcm-enable = <1>;
>>> +                    btcm-enable = <1>;
>>> +                    loczrama = <1>;
>>> +                    mboxes = <&mailbox0 &mbox_mcu_r5fss0_core0>;
>>> +                    memory-region = <&mcu_r5fss0_core0_dma_memory_region>,
>>> +                                    <&mcu_r5fss0_core0_memory_region>;
>>> +                    sram = <&mcu_r5fss0_core0_sram>;
>>> +                };
>>> +
>>> +                mcu_r5f1: r5f@41400000 {
>>> +                    compatible = "ti,am654-r5f";
>>> +                    reg = <0x41400000 0x00008000>,
>>> +                          <0x41410000 0x00008000>;
>>> +                    reg-names = "atcm", "btcm";
>>> +                    ti,sci = <&dmsc>;
>>> +                    ti,sci-dev-id = <245>;
>>> +                    ti,sci-proc-ids = <0x02 0xFF>;
>>> +                    resets = <&k3_reset 245 1>;
>>> +                    firmware-name = "am65x-mcu-r5f0_1-fw";
>>> +                    atcm-enable = <1>;
>>> +                    btcm-enable = <1>;
>>> +                    loczrama = <1>;
>>> +                    mboxes = <&mailbox1 &mbox_mcu_r5fss0_core1>;
>>> +               };
>>> +           };
>>> +        };
>>> +    };
>>> --
>>> 2.23.0
>>>
>