Message-ID: <42c34b1a-3ea6-350a-86fe-89f93f32e893@amd.com>
Date:   Mon, 16 Jan 2023 10:17:42 -0800
From:   Tanmay Shah <tanmays@....com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Tanmay Shah <tanmay.shah@....com>, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org
Cc:     devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: sram: Tightly Coupled Memory (TCM) bindings

Hi Kryzsztop Thanks for reviews. Please find my comments below.

On 1/15/23 6:45 AM, Krzysztof Kozlowski wrote:
> On 13/01/2023 19:04, Tanmay Shah wrote:
>> Hi Krzysztof Thanks for your reviews.
>>
>> Please find my comments below.
>>
>> On 1/12/23 11:52 PM, Krzysztof Kozlowski wrote:
>>> On 13/01/2023 08:30, Tanmay Shah wrote:
>>>> This patch introduces bindings for TCM memory address space on AMD-xilinx
>>>> platforms. As of now TCM addresses are hardcoded in xilinx remoteproc
>>>> driver. This bindings will help in defining TCM in device-tree and
>>>> make it's access platform agnostic and data-driven from the driver.
>>>>
>>>> Signed-off-by: Tanmay Shah <tanmay.shah@....com>
>>>> ---
>>>>    .../devicetree/bindings/sram/xlnx,tcm.yaml    | 137 ++++++++++++++++++
>>>>    1 file changed, 137 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml b/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>>>> new file mode 100644
>>>> index 000000000000..02d17026fb1f
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>>>> @@ -0,0 +1,137 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/sram/xlnx,tcm.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Tightly Coupled Memory (TCM)
>>>> +
>>>> +maintainers:
>>>> +  - Tanmay Shah <tanmay.shah@....com>
>>>> +
>>>> +description: |
>>>> +  Tightly Coupled Memory(TCM) is available on AMD-Xilinx paltforms for ARM
>>>> +  cortex remote processors to use. It is low-latency memory that provide
>>>> +  predictable instruction execution and predictable data load/store timing.
>>>> +  TCM can be configured in lockstep mode or split mode. In split mode
>>>> +  configuration each RPU core has its own set of ATCM and BTCM memories and in
>>>> +  lockstep mode redundant processor's TCM become available to lockstep
>>>> +  processor. So In lockstep mode ATCM and BTCM size is increased.
>>>> +
>>>> +properties:
>>>> +  $nodename:
>>>> +    pattern: "sram-[0-9a-f]+$"
>>> Drop node name requirement.
>>> Why do you need sram node at all?
>>
>> I will remove sram- node. However, it device-tree I was planning to put
>>
>> all TCM nodes under single node for example:
>>
>> tcm {
>>
>>       tcm-lockstep {
>>
>>       };
>>
>>       tcm-core@0 {
> Mix of nodes with and without unit address is pointing to some design
> issues.


The design currently tries to accommodate physical relocation of the 
memory. May be there is another way to represent this.

Here is address space of TCM memory on zynqmp platform: 
https://docs.xilinx.com/r/en-US/ug1085-zynq-ultrascale-trm/Tightly-Coupled-Memory-Address-Map

As per above address map, there are 4 TCM banks, each 64KB ( 0x10000 ) 
size at following addresses:

*In split mode*:

ATCM0: 0xFFE00000---|---> Assigned to RPU core0
BTCM0: 0xFFE20000---|

ATCM1: 0xFFE90000---|---> Assigned to RPU 1
BTCM1: 0xFFEB0000---|

However, In lockstep mode, ATCM1 and BTCM1 relocates to different 
addresses (i.e. 0xFFE10000 and 0xFFE30000 respectively)

and becomes available for RPU core 0:


*In lockstep mode Both used by RPU core 0*:

ATCM0: 0xFFE00000-----|
                                          |----> ATCM: 0xFFE00000 size: 
128KB
ATCM1: 0xFFE10000-----|

BTCM0: 0xFFE20000-----|
                                          |----> BTCM: 0xFFE20000 size: 
128KB
BTCM1: 0xFFE30000-----|


I am not sure how to represent this physical relocation of addresses in 
device-tree.

Ideally such sram nodes can be represented as following:

[1] Representation of TCM in split mode:

[ a|b ]tcm[ 0|1 ] {

    compatible = "xlnx,zynqmp-tcm";

     reg <>;

     ranges <>;

     power-domain: (only 1 power domain for current bank)

}

However, to represent TCM in lockstep mode as well, I might have to add 
platform specific optional reg and ranges property which optionally 
represent address space of lockstep mode for atcm and btcm.

For example, ATCM0 and BTCM0 will be represented as above [1] However, 
ATCM1 and BTCM1 will have following extra properties:

[a|b]tcm1 {

    compatible = "xlnx,zynqmp-tcm";

     reg <>;

     lockstep-reg <>; /* represent address space of this bank in 
lockstep mode */

     ranges <>;

     lockstep-ranges <>; /* represent address space ranges of this bank 
in lockstep mode */

     power-domain: (only 1 power domain for current bank)

};


Does above approach looks good? If some other standard way is already 
available to represent this could you please suggest?


Thanks,

Tanmay


>
>>       };
>>
>> };
>>
>> The top-most tcm node I assumed sram node. So I kept sram@...x
>>
>>>> +
>>>> +patternProperties:
>>>> +  "^tcm-[a-z]+@[0-9a-f]+$":
>>>> +    type: object
>>>> +    description: |
>>>> +      During the split mode, each RPU core has its own set of ATCM and BTCM memory
>>>> +
>>>> +      During the lock-step operation, the TCMs that are associated with the
>>>> +      redundant processor become available to the lock-step processor.
>>>> +      For example if each individual processor has 64KB ATCM, then in lockstep mode
>>>> +      The size of ATCM become 128KB. Same for BTCM. tcm-lockstep node represents
>>>> +      TCM address space in lockstep mode. tcm-core@x node specfies each core's
>>>> +      TCM address space in split mode.
>>>> +
>>>> +    properties:
>>>> +      compatible:
>>>> +        oneOf:
>>> This is not oneOf.
>>>
>>>> +          - items:
>>> and you do not have more than one item.
>>>
>>>> +              - enum:
>>>> +                  - xlnx,tcm-lockstep
>>>> +                  - xlnx,tcm-split
>>> compatible describes hardware, not configuration. What you encode here
>>> does not fit compatible.
>>
>> I see. So, only xlnx,tcm is enough.
> No, it must be specific to SoC.


Ok. Then xlnx,zynqmp-tcm. I will change file name accordingly as well.


>
>>
>>>> +
>>>> +      "#address-cells":
>>> Use consistent quotes, either " or '
>>
>> Ack.
>>
>>
>>>> +        const: 1
>>>> +
>>>> +      "#size-cells":
>>>> +        const: 1
>>>> +
>>>> +      reg:
>>>> +        items:
>>>> +          - description: |
>>>> +              ATCM Memory address space. An ATCM typically holds interrupt or
>>>> +              exception code that must be accessed at high speed, without any
>>>> +              potential delay resulting from a cache miss.
>>>> +              RPU on AMD-Xilinx platform can also fetch data from ATCM
>>>> +          - description: |
>>>> +              BTCM Memory address space. A BTCM typically holds a block of data
>>>> +              for intensive processing, such as audio or video processing. RPU on
>>>> +              AMD-Xilinx Platforms can also fetch Code (Instructions) from BTCM
>>>> +
>>>> +      reg-names:
>>>> +        items:
>>>> +          - const: atcm
>>>> +          - const: btcm
>>>> +
>>>> +      ranges: true
>>>> +
>>>> +      power-domains:
>>>> +        maxItems: 8
>>>> +        items:
>>>> +          - description: list of ATCM Power domains
>>>> +          - description: list of BTCM Power domains
>>>> +        additionalItems: true
>>> And what are the rest?
>> As both items are list, we should be able to include more than one
>> power-domain I believe.
>>
>>
>> So first item I am trying to create list of ATCM power domains.
>>
>> In split mode, first item is ATCM power-domain and second item is BTCM
>> power domain.
>>
>> However, In lockstep mode, second core's TCM physically relocates and
>> two ATCM combines and
> Why power domains of a device depend on the mode? This does not look
> like binding describing hardware.
>
>> makes single region of ATCM. However, their power-domains remains same.
>>
>> So, In lockstep mode, first two banks are ATCM and so, first two items
>> are ATCM power-domains.
>>
>> I am not sure best way to represent this. But, first itmes is list.
>>
>> So, I am assuming list of all ATCM power-domains possible.
> List all items. Order is fixed, you cannot say BTCM is second item and
> then put here something else.
>
> Best regards,
> Krzysztof
>

Powered by blists - more mailing lists