Messages in this thread | | | Date | Fri, 13 Jan 2023 10:04:04 -0800 | Subject | Re: [PATCH] dt-bindings: sram: Tightly Coupled Memory (TCM) bindings | From | Tanmay Shah <> |
| |
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 >
| |