Re: [PATCH 2/3] dt-bindings: remoteproc: add Versal-NET platform

[Tanmay Shah]

Hello Krzysztof,

Thanks for reviews. Please find my comments below.

On 3/17/24 1:53 PM, Krzysztof Kozlowski wrote:
> On 15/03/2024 22:15, Tanmay Shah wrote:
>> AMD-Xilinx Versal-NET platform is successor of Versal platform. It
>> contains multiple clusters of cortex-R52 real-time processing units.
>> Each cluster contains two cores of cortex-R52 processors. Each cluster
>> can be configured in lockstep mode or split mode.
>> 
>> Each R52 core is assigned 128KB of TCM memory. ATCM memory is 64KB, BTCM
>> and CTCM memoreis are 32KB each. Each TCM memory has its own dedicated
>> power-domain that needs to be requested before using it.
>> 
>> Signed-off-by: Tanmay Shah <tanmay.shah@xxxxxxx>
>> ---
>>  .../remoteproc/xlnx,zynqmp-r5fss.yaml         | 220 +++++++++++++++---
>>  1 file changed, 184 insertions(+), 36 deletions(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>> index 711da0272250..55654ee02eef 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>> +++ b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>> @@ -18,7 +18,9 @@ description: |
>>  
>>  properties:
>>    compatible:
>> -    const: xlnx,zynqmp-r5fss
>> +    enum:
>> +      - xlnx,zynqmp-r5fss
>> +      - xlnx,versal-net-r52fss
>>  
>>    "#address-cells":
>>      const: 2
>> @@ -64,7 +66,9 @@ patternProperties:
>>  
>>      properties:
>>        compatible:
>> -        const: xlnx,zynqmp-r5f
>> +        enum:
>> +          - xlnx,zynqmp-r5f
>> +          - xlnx,versal-net-r52f
>>  
>>        reg:
>>          minItems: 1
>> @@ -135,9 +139,11 @@ required:
>>  allOf:
>>    - if:
>>        properties:
>> -        xlnx,cluster-mode:
>> -          enum:
>> -            - 1
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - xlnx,versal-net-r52fss
> 
> Why do you touch these lines?
> 
>> +
>>      then:
>>        patternProperties:
>>          "^r5f@[0-9a-f]+$":
>> @@ -149,16 +155,14 @@ allOf:
>>                items:
>>                  - description: ATCM internal memory
>>                  - description: BTCM internal memory
>> -                - description: extra ATCM memory in lockstep mode
>> -                - description: extra BTCM memory in lockstep mode
>> +                - description: CTCM internal memory
>>  
>>              reg-names:
>>                minItems: 1
>>                items:
>> -                - const: atcm0
>> -                - const: btcm0
>> -                - const: atcm1
>> -                - const: btcm1
>> +                - const: atcm
>> +                - const: btcm
>> +                - const: ctcm
>>  
>>              power-domains:
>>                minItems: 2
>> @@ -166,33 +170,70 @@ allOf:
>>                  - description: RPU core power domain
>>                  - description: ATCM power domain
>>                  - description: BTCM power domain
>> -                - description: second ATCM power domain
>> -                - description: second BTCM power domain
>> +                - description: CTCM power domain
>>  
>>      else:
>> -      patternProperties:
>> -        "^r5f@[0-9a-f]+$":
>> -          type: object
>> -
>> -          properties:
>> -            reg:
>> -              minItems: 1
>> -              items:
>> -                - description: ATCM internal memory
>> -                - description: BTCM internal memory
>> -
>> -            reg-names:
>> -              minItems: 1
>> -              items:
>> -                - const: atcm0
>> -                - const: btcm0
>> -
>> -            power-domains:
>> -              minItems: 2
>> -              items:
>> -                - description: RPU core power domain
>> -                - description: ATCM power domain
>> -                - description: BTCM power domain
>> +      allOf:
>> +        - if:
>> +            properties:
>> +              xlnx,cluster-mode:
>> +                enum:
>> +                  - 1
> 
> Whatever you did here, is not really readable. You have now multiple
> if:then:if:then embedded.

For ZynqMP platform, TCM can be configured differently in lockstep mode
and split mode.

For Versal-NET no such configuration is available, but new CTCM memory
is added.

So, I am trying to achieve following representation of TCM for both:

if: versal-net compatible
then:
  ATCM - 64KB
  BTCM - 32KB
  CTCM - 32KB

else: (ZynqMP compatible)
  if:
    xlnx,cluster-mode (lockstep mode)
  then:
    ATCM0 - 64KB
    BTCM0 - 64KB
    ATCM1 - 64KB
    BTCM1 - 64KB
  else: (split mode)
    ATCM0 - 64KB
    BTCM0 - 64KB


If bindings are getting complicated, does it make sense to introduce
new file for Versal-NET bindings? Let me know how you would like me
to proceed.

Thanks,
Tanmay

> 
>> +          then:
>> +            patternProperties:
>> +              "^r5f@[0-9a-f]+$":
>> +                type: object
>> +
>> +                properties:
>> +                  reg:
>> +                    minItems: 1
>> +                    items:
>> +                      - description: ATCM internal memory
>> +                      - description: BTCM internal memory
>> +                      - description: extra ATCM memory in lockstep mode
>> +                      - description: extra BTCM memory in lockstep mode
>> +
>> +                  reg-names:
>> +                    minItems: 1
>> +                    items:
>> +                      - const: atcm0
>> +                      - const: btcm0
>> +                      - const: atcm1
>> +                      - const: btcm1
>> +
>> +                  power-domains:
>> +                    minItems: 2
>> +                    items:
>> +                      - description: RPU core power domain
>> +                      - description: ATCM power domain
>> +                      - description: BTCM power domain
>> +                      - description: second ATCM power domain
>> +                      - description: second BTCM power domain
>> +
>> +          else:
>> +            patternProperties:
>> +              "^r5f@[0-9a-f]+$":
>> +                type: object
>> +
>> +                properties:
>> +                  reg:
>> +                    minItems: 1
>> +                    items:
>> +                      - description: ATCM internal memory
>> +                      - description: BTCM internal memory
>> +
>> +                  reg-names:
>> +                    minItems: 1
>> +                    items:
>> +                      - const: atcm0
>> +                      - const: btcm0
>> +
>> +                  power-domains:
>> +                    minItems: 2
>> +                    items:
>> +                      - description: RPU core power domain
>> +                      - description: ATCM power domain
>> +                      - description: BTCM power domain
>>  
>>  additionalProperties: false
>>  
>> @@ -386,4 +427,111 @@ examples:
>>              };
>>          };
>>      };
>> +
>> +  - |
>> +    // Versal-NET split configuration
>> +    soc {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
> 
> Don't add examples per each platform.

Noted. If we go with different file for Versal-NET, then example should
be included?

> 
> Best regards,
> Krzysztof
>