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