diff mbox series

dt-bindings: sram: Tightly Coupled Memory (TCM) bindings

Message ID 20230113073045.4008853-1-tanmay.shah@amd.com
State New
Headers show
Series dt-bindings: sram: Tightly Coupled Memory (TCM) bindings | expand

Commit Message

Tanmay Shah Jan. 13, 2023, 7:30 a.m. UTC 
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


base-commit: 6b31ffe9c8b9947d6d3552d6e10752fd96d0f80f

Comments

Krzysztof Kozlowski Jan. 15, 2023, 2:38 p.m. UTC | #1 
On 13/01/2023 19:08, Tanmay Shah wrote:
> 
> 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.
>>>
>> Subject: drop second/last, redundant "bindings". The "dt-bindings"
>> prefix is already stating that these are bindings.
> 
> Ack.
> 
> 
>>
>> Where is driver or DTS? Are you now adding a dead binding without users?
> 
> 
> TCM is used by drivers/remoteproc/xlnx_r5_remoteproc.c driver. Howerver, 
> we have hardcode addresses in TCM as bindings are not available yet.

I don't see usage of these compatibles there. You also did not supply
DTS here. Please provide users of bindings within the same patchset.


Best regards,
Krzysztof
Krzysztof Kozlowski Jan. 15, 2023, 2:45 p.m. UTC | #2 
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@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 {

Mix of nodes with and without unit address is pointing to some design
issues.

> 
>      };
> 
> };
> 
> 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.

No, it must be specific to SoC.

> 
> 
>>
>>> +
>>> +      "#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
Tanmay Shah Jan. 16, 2023, 5:43 p.m. UTC | #3 
On 1/15/23 6:38 AM, Krzysztof Kozlowski wrote:
> On 13/01/2023 19:08, Tanmay Shah wrote:
>> 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.
>>>>
>>> Subject: drop second/last, redundant "bindings". The "dt-bindings"
>>> prefix is already stating that these are bindings.
>> Ack.
>>
>>
>>> Where is driver or DTS? Are you now adding a dead binding without users?
>>
>> TCM is used by drivers/remoteproc/xlnx_r5_remoteproc.c driver. Howerver,
>> we have hardcode addresses in TCM as bindings are not available yet.
> I don't see usage of these compatibles there. You also did not supply
> DTS here. Please provide users of bindings within the same patchset.


ACK. I will supply dts as well.

However, Is it ok if I convert this patch to RFC patch, and once 
bindings are fixed I will send actual patch with driver support.

If bindings design is not correct then I might have to change 
corresponding driver design lot.


>
>
> Best regards,
> Krzysztof
>
Tanmay Shah Jan. 18, 2023, 7:23 p.m. UTC | #4 
On 1/17/23 12:16 AM, Krzysztof Kozlowski wrote:
> On 16/01/2023 18:43, Tanmay Shah wrote:
>> On 1/15/23 6:38 AM, Krzysztof Kozlowski wrote:
>>> On 13/01/2023 19:08, Tanmay Shah wrote:
>>>> 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.
>>>>>>
>>>>> Subject: drop second/last, redundant "bindings". The "dt-bindings"
>>>>> prefix is already stating that these are bindings.
>>>> Ack.
>>>>
>>>>
>>>>> Where is driver or DTS? Are you now adding a dead binding without users?
>>>> TCM is used by drivers/remoteproc/xlnx_r5_remoteproc.c driver. Howerver,
>>>> we have hardcode addresses in TCM as bindings are not available yet.
>>> I don't see usage of these compatibles there. You also did not supply
>>> DTS here. Please provide users of bindings within the same patchset.
>>
>> ACK. I will supply dts as well.
>>
>> However, Is it ok if I convert this patch to RFC patch, and once
>> bindings are fixed I will send actual patch with driver support.
>>
>> If bindings design is not correct then I might have to change
>> corresponding driver design lot.
> First, why this driver is particularly special? Why should have other
> treatment then all other cases?


It's not different than others and shouldn't be treated differently. I 
just didn't know correct bindings representation.

Now I have some idea how this should be represented, so I will send 
bindings patch, dts patch and driver patch all in same series.


>
> Second, so think about bindings and do not submit something for "driver"
> but something describing hardware.


ACK. It will take me some time to post next patch, as I will add support 
of this tcm device in xlnx remoteproc driver as well.

Thanks for all your suggestions, they were helpful.


>
> Best regards,
> Krzysztof
>
diff mbox series

Patch

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]+$"
+
+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:
+          - items:
+              - enum:
+                  - xlnx,tcm-lockstep
+                  - xlnx,tcm-split
+
+      "#address-cells":
+        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
+
+    required:
+      - compatible
+      - '#address-cells'
+      - '#size-cells'
+      - reg
+      - ranges
+      - power-domains
+    unevaluatedProperties: false
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/power/xlnx-zynqmp-power.h>
+
+    amba {
+        sram@ffe00000 {
+            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>,
+                                <&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>;
+            };
+        };
+    };
+...