Re: [PATCH] dt-bindings: sram: Tightly Coupled Memory (TCM) bindings

[Krzysztof Kozlowski]

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