Re: [PATCH] dt-bindings: sram: Tightly Coupled Memory (TCM) bindings
From: Tanmay Shah
Date: Fri Jan 13 2023 - 13:12:04 EST
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@xxxxxxx>
---
.../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@xxxxxxx>
+
+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