Received-SPF: pass (google.com: domain of linux-kernel+bounces-106873-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <14be0aa6-49b7-4342-9ca6-750c30c8e1e9@amd.com>
Date: Mon, 18 Mar 2024 19:51:19 -0500
User-Agent: Mozilla Thunderbird Beta
Subject: Re: [PATCH 2/3] dt-bindings: remoteproc: add Versal-NET platform
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
 andersson@kernel.org, mathieu.poirier@linaro.org, robh+dt@kernel.org,
 krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
 michal.simek@amd.com, ben.levinsky@amd.com
Cc: linux-remoteproc@vger.kernel.org, devicetree@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20240315211533.1996543-1-tanmay.shah@amd.com>
 <20240315211533.1996543-3-tanmay.shah@amd.com>
 <3ca1c419-d185-4318-92ed-3c4e40dcf5bb@linaro.org>
Content-Language: en-US
From: Tanmay Shah <tanmay.shah@amd.com>
In-Reply-To: <3ca1c419-d185-4318-92ed-3c4e40dcf5bb@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?MmtUSzlyc1ZjOEVEL0ViMVJjeURHeUUvcWdCT3NPTVJXOXZHRFBnbThhbVYv?=
 =?utf-8?B?S1VKTFRmNkZmcm1FeS9EcnBYMmpiSjJvbVF2U0Zrb1FmM2FoeEwwd09Eczd1?=
 =?utf-8?B?Snl1cVYzSW53d3hMYkFIV29WdGs0TUZuRUdTd29OMEhzR2NxdG8reHRpbWtP?=
 =?utf-8?B?OWRMWHBlWExtMzIvQ0RXeWNWOVUwSis3YjVWbnhMTUVydkdxQ3dFaDRHaWEx?=
 =?utf-8?B?akNLbUVzQ21LMVhLVlRBNGhOZFg2TVVSZVdsYkNaODVIN0ZqMk5TbHJ1aHBU?=
 =?utf-8?B?VUo0WDFtVEtMRzNQMnlCalRaVm5JQU04eEMvZWJCaVRBb1FUcDF3cTF1Q3pr?=
 =?utf-8?B?Y1ErTHU2MG1ReUtLb1FydEYzTldPWXd5b2hMK0QvMWlMZHdUbXJpRWFqTG9a?=
 =?utf-8?B?dnVrb3d6RGZ4VEF4OXdheDZ5UGV5NkI0UkgzQTk1Z29wMjVVRE9sZFpqTkVk?=
 =?utf-8?B?Y2dUMmM5NHFwSVdEYVZYUGlYUG8rWTQ2Q0VCYjl2c0hLSlpuRlk0ODAvVlln?=
 =?utf-8?B?MkQ1Y2RnYnRreko3ODFxaEQ1cXYxYW9mUllTclNwNlJnM2VEMHNqTmowQVVC?=
 =?utf-8?B?RWM2bHFvTzBIOWJkWVYrUmFQZmZkWm5STUlZbUhXWmtzbDE3eDJINmtLVVJj?=
 =?utf-8?B?MXlsTVNwdUlQWC9BTExEOHphSmFJcFZ2bDNLeWxGMy81WjFwZ1gzL0RWdENx?=
 =?utf-8?B?M252VVhxSU1yb013eUljb290VGdLbXlXT1ZIVzh5aU5OL2NQalkyWTN4d0tu?=
 =?utf-8?B?aTJoUVJxVHdYTnZVcVVwTmMrb3AvZk55blI3YURMVk9wSUhGdFJjS3NUVzN2?=
 =?utf-8?B?Y24rK2Q3WTVOd0p1RUJzWGx3Y2FqMm96TDJ2dk1GcnZpdnc3VXNFVkQrWkxt?=
 =?utf-8?B?bXZScEg2Mk9yQmQ3djFVbDRnZ09JTWJiaVMvWThsNmlYc3R4UUYyU0lmZFpl?=
 =?utf-8?B?ZUg5Q2xGVWptNStyUVhIaUhDK1NSQ0N5V01CR3l6REJFNkpVZS9obkN0OUlW?=
 =?utf-8?B?WUpZU2l1V1hrUlRacTZRUjE0ZW95QVVkQU1VaytzUjhYbXI1eGJuVGM4YjV1?=
 =?utf-8?B?S2kvWW9nQmVBT2xsR2U3c0UzUGRUc2ZpN3dTVkVPYngwM3RSbEtoSUhFcHVD?=
 =?utf-8?B?SzdWSlRjd3BjZXQ3MVhZeVBtMFRISUlDMk91MGMxUFJsd1BoVlFERTU0OUNz?=
 =?utf-8?B?UzJLcXFxMWQza2E4bEdjanYwVmhuenlFcXluNDlLRFc5V0F5Z3Q3Z1cyRXE5?=
 =?utf-8?B?V1ZJd2dIcVNEemVLM3ZLVk9obW9rNjBWZlJqeEJVb05oM3l0OGJib3QySm9a?=
 =?utf-8?B?ZXpmRkdxOHMzYjdTeFZlWjMzWjFhZnFVUW85QzlWbEZtQ3NKaXQwQWwvNlY4?=
 =?utf-8?B?UzRSb3lnQjRTNGx1QlVuVTE2dzFLUmhMMy9QTGl1aFlWL21sVFllSzlrK1FE?=
 =?utf-8?B?R0dLVHZEVzdqak9JSEMxL3YzQW13Vm92WVF1UXVkc3dUdjNKbTRDcUpDeDFN?=
 =?utf-8?B?RUJUQ0kwencwVzNVcmxYMlByWmdOM1FzWm5RSXIzVVJiaEloMEM5U0RhOVBW?=
 =?utf-8?B?SEUvTE5sNGxRVVBqdUlnZnU4Z2NrV3J0dmpEMlBYUy93SU42dkI0WTJYdW02?=
 =?utf-8?B?dlFtc2c0VkIwNGl3Y2JoNTh4VVdZTmdqUVpLeEppTytkN3V3a3FHNlQwTUdv?=
 =?utf-8?B?THhhbzJaQzRIOTZOeWhhL0tCOTFMNENXZkxLY3psRXo4RlNzVXAvckMzOE9U?=
 =?utf-8?B?RmtuRS9tbmFXbWJRRlk5Vzc5eWlNVTBjSUVxZmh5YlZiTjE0MmxtbVk5eGU4?=
 =?utf-8?B?NVBMSU02MXM0Y1RlZFZBOEFobEk0QTJGRzBxN2dteHZUUXl4cWR5amQyRHN1?=
 =?utf-8?B?N256NFA3STkrM1FnVEZYRjNBYzZObzlhdDVQQUkrdFh6eEpPUXowbzN3TmJz?=
 =?utf-8?B?bTl4SGg0eGNBZUR3eCsxSjJlUktOMlFacHdOZnNyNTl0aHJSWWRIMXZRWlUx?=
 =?utf-8?B?VU80UDFyMHNSVW42ZFE3SDBkdWRYazRTRENXK0ZmMzJpMmloL2VPTThQUmRz?=
 =?utf-8?B?dktJb2lMYmZxZjRYTDR4VHltV081TWkzRWVEV2NWZUVQUU5waHFoQlNid2No?=
 =?utf-8?Q?wp0fb2xLf/HKJHMH7V2wr1k2+?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2b690d11-b971-4b30-f63f-08dc47aeb299
X-MS-Exchange-CrossTenant-AuthSource: BL1PR12MB5874.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Mar 2024 00:51:22.7085
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: oCw1p4ID2ep6kuYlyJbWOHrCgOH13tzEQoasrIuMymPQAOt2XdlZrA/CSWq7CfXM
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR12MB6733

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@amd.com>
>> ---
>>  .../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
>