On 16/01/2023 19:17, Tanmay Shah wrote: > Hi Kryzsztop Thanks for reviews. Please find my comments below. > > On 1/15/23 6:45 AM, Krzysztof Kozlowski wrote: >> 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 design currently tries to accommodate physical relocation of the > memory. May be there is another way to represent this. > > Here is address space of TCM memory on zynqmp platform: > https://docs.xilinx.com/r/en-US/ug1085-zynq-ultrascale-trm/Tightly-Coupled-Memory-Address-Map > > As per above address map, there are 4 TCM banks, each 64KB ( 0x10000 ) > size at following addresses: > > *In split mode*: > > ATCM0: 0xFFE00000---|---> Assigned to RPU core0 > BTCM0: 0xFFE20000---| > > ATCM1: 0xFFE90000---|---> Assigned to RPU 1 > BTCM1: 0xFFEB0000---| > > However, In lockstep mode, ATCM1 and BTCM1 relocates to different > addresses (i.e. 0xFFE10000 and 0xFFE30000 respectively) > > and becomes available for RPU core 0: > > > *In lockstep mode Both used by RPU core 0*: > > ATCM0: 0xFFE00000-----| > |----> ATCM: 0xFFE00000 size: > 128KB > ATCM1: 0xFFE10000-----| > > BTCM0: 0xFFE20000-----| > |----> BTCM: 0xFFE20000 size: > 128KB > BTCM1: 0xFFE30000-----| > > > I am not sure how to represent this physical relocation of addresses in > device-tree. What do you mean by "relocates"? Move? You set one address in DT and other address appears to be used? Then just set the other address? > > Ideally such sram nodes can be represented as following: > > [1] Representation of TCM in split mode: > > [ a|b ]tcm[ 0|1 ] { You do not have unit address here. > > compatible = "xlnx,zynqmp-tcm"; > > reg <>; > > ranges <>; > > power-domain: (only 1 power domain for current bank) > > } > > However, to represent TCM in lockstep mode as well, I might have to add > platform specific optional reg and ranges property which optionally > represent address space of lockstep mode for atcm and btcm. I don't understand why. You have IO address space, so why do you remove unit address and make reg optional? > > For example, ATCM0 and BTCM0 will be represented as above [1] However, > ATCM1 and BTCM1 will have following extra properties: > > [a|b]tcm1 { > > compatible = "xlnx,zynqmp-tcm"; > > reg <>; > > lockstep-reg <>; /* represent address space of this bank in > lockstep mode */ Device is either in lockstep or it is not, right? Why do you put here some dualism? Best regards, Krzysztof
