lkml.org 
[lkml]   [2023]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] dt-bindings: sram: Tightly Coupled Memory (TCM) bindings
From
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
>

\
 
 \ /
  Last update: 2023-03-26 23:41    [W:0.282 / U:0.388 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site