Re: [PATCH] dt-bindings: sram: Tightly Coupled Memory (TCM) bindings

[Tanmay Shah <tanmays@amd.com>]

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@amd.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@amd.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 {

     };

};

The top-most tcm node I assumed sram node. So I kept sram@xxxx

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


>
>> +
>> +      "#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

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.


>
>> +
>> +    required:
>> +      - compatible
>> +      - '#address-cells'
>> +      - '#size-cells'
>> +      - reg
>> +      - ranges
>> +      - power-domains
>> +    unevaluatedProperties: false
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/power/xlnx-zynqmp-power.h>
>> +
>> +    amba {
> Drop.


ACK


>> +        sram@ffe00000 {
> This does not match your bindings.


Ok. This was node-name. I will remove it from example.


>
>> +            tcm-lockstep@ffe00000 {
>> +                compatible = "xlnx,tcm-lockstep";
>> +
>> +                #address-cells = <1>;
>> +                #size-cells = <1>;
>> +
>> +                reg = <0xffe00000 0x20000>, <0xffe20000 0x20000>;
>> +                reg-names = "atcm", "btcm";
>> +                ranges = <0x0 0xffe00000 0x20000>, <0x20000 0xffe20000 0x20000>;
>> +                power-domains = <&zynqmp_firmware PD_R5_0_ATCM>,
>> +                                <&zynqmp_firmware PD_R5_1_ATCM>,
> This is BTCM domain according to your binding. Your binding here is
> probably wrong and does not match real DTS.


As explained above, the first Item is list of all ATCM power-domains.

So, I kept both ATCM power-domains for lockstep mode.

We don't have dts nodes for TCM yet. We are using hard-coded address in 
xlnx_r5_remoteproc.c driver.

As the bindings are new, I was hoping to introduce dts nodes once 
bindings are designed right.



>
>> +                                <&zynqmp_firmware PD_R5_0_BTCM>,
>> +                                <&zynqmp_firmware PD_R5_1_BTCM>;
>> +            };
>> +
>> +            tcm-core@0 {
>> +                compatible = "xlnx,tcm-split";
>> +
>> +                #address-cells = <1>;
>> +                #size-cells = <1>;
>> +
>> +                reg = <0xffe00000 0x10000>, <0xffe20000 0x10000>;
>> +                reg-names = "atcm", "btcm";
>> +                ranges = <0x0 0xffe00000 0x10000>, <0x20000 0xffe20000 0x10000>;
>> +                power-domains = <&zynqmp_firmware PD_R5_0_ATCM>,
>> +                                <&zynqmp_firmware PD_R5_0_BTCM>;
>> +            };
>> +
>> +            tcm-core@1 {
>> +                compatible = "xlnx,tcm-split";
>> +
>> +                #address-cells = <1>;
>> +                #size-cells = <1>;
>> +
>> +                reg = <0xffe90000 0x10000>, <0xffeb0000 0x10000>;
>> +                reg-names = "atcm", "btcm";
>> +                ranges = <0x0 0xffe90000 0x10000>, <0x20000 0xffeb0000 0x10000>;
>> +                power-domains = <&zynqmp_firmware PD_R5_1_ATCM>,
>> +                                <&zynqmp_firmware PD_R5_1_BTCM>;
>> +            };
>> +        };
>> +    };
>> +...
>>
>> base-commit: 6b31ffe9c8b9947d6d3552d6e10752fd96d0f80f
> Best regards,
> Krzysztof
>