diff mbox series

[v3,1/2] dt-bindings: remoteproc: ti: Add new compatible for AM62 SoC family

Message ID 20221223115638.20192-2-devarsht@ti.com
State New
Headers
Series Add single core R5F IPC for AM62 SoC family |

Commit Message

Devarsh Thakkar Dec. 23, 2022, 11:56 a.m. UTC 
  AM62 family of devices don't have a R5F cluster, instead
they have single core DM R5F.
Add new compatible string ti,am62-r5fss to support this scenario.

When this new compatible is used don't allow cluster-mode
property usage in device-tree as this implies that there
is no R5F cluster available and only single R5F core
is present.

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V2: Avoid acronyms, use "Device Manager" instead of "DM"
V3:
- Use separate if block for each compatible for ti,cluster-mode property
- Rearrange compatibles as per alphabatical order
---
 .../bindings/remoteproc/ti,k3-r5f-rproc.yaml  | 68 +++++++++++++------
 1 file changed, 47 insertions(+), 21 deletions(-)

Comments

Krzysztof Kozlowski Dec. 23, 2022, 12:05 p.m. UTC | #1 
On 23/12/2022 12:56, Devarsh Thakkar wrote:
> AM62 family of devices don't have a R5F cluster, instead
> they have single core DM R5F.
> Add new compatible string ti,am62-r5fss to support this scenario.
> 
> When this new compatible is used don't allow cluster-mode
> property usage in device-tree as this implies that there
> is no R5F cluster available and only single R5F core
> is present.

It's v3 but addresses are still not correct.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

Best regards,
Krzysztof
Krzysztof Kozlowski Dec. 26, 2022, 12:18 p.m. UTC | #2 
On 23/12/2022 12:56, Devarsh Thakkar wrote:
> AM62 family of devices don't have a R5F cluster, instead
> they have single core DM R5F.
> Add new compatible string ti,am62-r5fss to support this scenario.
> 
> When this new compatible is used don't allow cluster-mode
> property usage in device-tree as this implies that there
> is no R5F cluster available and only single R5F core
> is present.
> 
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
> V2: Avoid acronyms, use "Device Manager" instead of "DM"
> V3:
> - Use separate if block for each compatible for ti,cluster-mode property
> - Rearrange compatibles as per alphabatical order
> ---
>  .../bindings/remoteproc/ti,k3-r5f-rproc.yaml  | 68 +++++++++++++------
>  1 file changed, 47 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
> index fb9605f0655b..e8a861179bd9 100644
> --- a/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
> @@ -21,6 +21,9 @@ description: |
>    called "Single-CPU" mode, where only Core0 is used, but with ability to use
>    Core1's TCMs as well.
>  
> +  AM62 SoC family support a single R5F core only which runs Device Manager
> +  firmware and can also be used as a remote processor with IPC communication.
> +
>    Each Dual-Core R5F sub-system is represented as a single DTS node
>    representing the cluster, with a pair of child DT nodes representing
>    the individual R5F cores. Each node has a number of required or optional
> @@ -28,16 +31,20 @@ description: |
>    the device management of the remote processor and to communicate with the
>    remote processor.
>  
> +  Since AM62 SoC family only support a single core, there is no cluster-mode
> +  property setting required for it.
> +
>  properties:
>    $nodename:
>      pattern: "^r5fss(@.*)?"
>  
>    compatible:
>      enum:
> +      - ti,am62-r5fss
> +      - ti,am64-r5fss
>        - ti,am654-r5fss
> -      - ti,j721e-r5fss
>        - ti,j7200-r5fss
> -      - ti,am64-r5fss
> +      - ti,j721e-r5fss
>        - ti,j721s2-r5fss
>  
>    power-domains:
> @@ -80,7 +87,9 @@ patternProperties:
>        node representing a TI instantiation of the Arm Cortex R5F core. There
>        are some specific integration differences for the IP like the usage of
>        a Region Address Translator (RAT) for translating the larger SoC bus
> -      addresses into a 32-bit address space for the processor.
> +      addresses into a 32-bit address space for the processor. For AM62x,
> +      the R5F Sub-System device node should only define one R5F child node
> +      as it has only one core available.
>  
>        Each R5F core has an associated 64 KB of Tightly-Coupled Memory (TCM)
>        internal memories split between two banks - TCMA and TCMB (further
> @@ -100,11 +109,12 @@ patternProperties:
>      properties:
>        compatible:
>          enum:
> -          - ti,am654-r5f
> -          - ti,j721e-r5f
> -          - ti,j7200-r5f
> -          - ti,am64-r5f
> -          - ti,j721s2-r5f
> +          - ti,am62-r5fss
> +          - ti,am64-r5fss
> +          - ti,am654-r5fss
> +          - ti,j7200-r5fss
> +          - ti,j721e-r5fss
> +          - ti,j721s2-r5fss
>  
>        reg:
>          items:
> @@ -208,19 +218,35 @@ patternProperties:
>  
>      unevaluatedProperties: false
>  
> -if:
> -  properties:
> -    compatible:
> -      enum:
> -        - ti,am64-r5fss
> -then:
> -  properties:
> -    ti,cluster-mode:
> -      enum: [0, 2]
> -else:
> -  properties:
> -    ti,cluster-mode:
> -      enum: [0, 1]
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - ti,am64-r5fss
> +    then:
> +      properties:
> +        ti,cluster-mode:
> +          enum: [0, 2]
> +
> +  - if:
> +      properties:
> +        compatible:
> +          enum: ["ti,am654-r5fss", "ti,j7200-r5fss", "ti,j721e-r5fss",  "ti,j721s2-r5fss"]

That's not how enums are spelled for such cases. Git grep for examples -
this should be a enum with each item in new entry, no quotes.

Best regards,
Krzysztof
Devarsh Thakkar Dec. 27, 2022, 9:38 a.m. UTC | #3 
On 26/12/22 17:48, Krzysztof Kozlowski wrote:
> On 23/12/2022 12:56, Devarsh Thakkar wrote:
>> AM62 family of devices don't have a R5F cluster, instead
>> they have single core DM R5F.
>> Add new compatible string ti,am62-r5fss to support this scenario.
>>
>> When this new compatible is used don't allow cluster-mode
>> property usage in device-tree as this implies that there
>> is no R5F cluster available and only single R5F core
>> is present.
>>
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> ---
>> V2: Avoid acronyms, use "Device Manager" instead of "DM"
>> V3:
>> - Use separate if block for each compatible for ti,cluster-mode property
>> - Rearrange compatibles as per alphabatical order
>> ---
>>  .../bindings/remoteproc/ti,k3-r5f-rproc.yaml  | 68 +++++++++++++------
>>  1 file changed, 47 insertions(+), 21 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
>> index fb9605f0655b..e8a861179bd9 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
>> +++ b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
>> @@ -21,6 +21,9 @@ description: |
>>    called "Single-CPU" mode, where only Core0 is used, but with ability to use
>>    Core1's TCMs as well.
>>  
>> +  AM62 SoC family support a single R5F core only which runs Device Manager
>> +  firmware and can also be used as a remote processor with IPC communication.
>> +
>>    Each Dual-Core R5F sub-system is represented as a single DTS node
>>    representing the cluster, with a pair of child DT nodes representing
>>    the individual R5F cores. Each node has a number of required or optional
>> @@ -28,16 +31,20 @@ description: |
>>    the device management of the remote processor and to communicate with the
>>    remote processor.
>>  
>> +  Since AM62 SoC family only support a single core, there is no cluster-mode
>> +  property setting required for it.
>> +
>>  properties:
>>    $nodename:
>>      pattern: "^r5fss(@.*)?"
>>  
>>    compatible:
>>      enum:
>> +      - ti,am62-r5fss
>> +      - ti,am64-r5fss
>>        - ti,am654-r5fss
>> -      - ti,j721e-r5fss
>>        - ti,j7200-r5fss
>> -      - ti,am64-r5fss
>> +      - ti,j721e-r5fss
>>        - ti,j721s2-r5fss
>>  
>>    power-domains:
>> @@ -80,7 +87,9 @@ patternProperties:
>>        node representing a TI instantiation of the Arm Cortex R5F core. There
>>        are some specific integration differences for the IP like the usage of
>>        a Region Address Translator (RAT) for translating the larger SoC bus
>> -      addresses into a 32-bit address space for the processor.
>> +      addresses into a 32-bit address space for the processor. For AM62x,
>> +      the R5F Sub-System device node should only define one R5F child node
>> +      as it has only one core available.
>>  
>>        Each R5F core has an associated 64 KB of Tightly-Coupled Memory (TCM)
>>        internal memories split between two banks - TCMA and TCMB (further
>> @@ -100,11 +109,12 @@ patternProperties:
>>      properties:
>>        compatible:
>>          enum:
>> -          - ti,am654-r5f
>> -          - ti,j721e-r5f
>> -          - ti,j7200-r5f
>> -          - ti,am64-r5f
>> -          - ti,j721s2-r5f
>> +          - ti,am62-r5fss
>> +          - ti,am64-r5fss
>> +          - ti,am654-r5fss
>> +          - ti,j7200-r5fss
>> +          - ti,j721e-r5fss
>> +          - ti,j721s2-r5fss
>>  
There is a problem here, the compatibles still need to be "-r5f" I will
correct it in V4.
>>        reg:
>>          items:
>> @@ -208,19 +218,35 @@ patternProperties:
>>  
>>      unevaluatedProperties: false
>>  
>> -if:
>> -  properties:
>> -    compatible:
>> -      enum:
>> -        - ti,am64-r5fss
>> -then:
>> -  properties:
>> -    ti,cluster-mode:
>> -      enum: [0, 2]
>> -else:
>> -  properties:
>> -    ti,cluster-mode:
>> -      enum: [0, 1]
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          enum:
>> +            - ti,am64-r5fss
>> +    then:
>> +      properties:
>> +        ti,cluster-mode:
>> +          enum: [0, 2]
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          enum: ["ti,am654-r5fss", "ti,j7200-r5fss", "ti,j721e-r5fss",  "ti,j721s2-r5fss"]
> 
> That's not how enums are spelled for such cases. Git grep for examples -
> this should be a enum with each item in new entry, no quotes.
Yeah, that was my initial thought but then I looked at section 4.9.3 of
https://json-schema.org/understanding-json-schema/UnderstandingJSONSchema.pdf
which had below example and based on that did this change thus avoiding
separate entries for each enum.

Example:
{
"enum": ["red", "amber", "green", null, 42]
}

To confirm whether the change works fine, I deliberately modified cluster-mode
values for each of the SoC's beyond acceptable ranges as seen in
https://gist.github.com/devarsht/1956063c8e39f1bdbad3574ea96b95a3
and then ran "make dtbs_check" and it was able to catch the inappropriate
values of cluster-mode as seen in below logs :

https://gist.github.com/devarsht/bc8cfb82c55b75d85649585d3fd536a0#file-gistfile1-txt-L392
https://gist.github.com/devarsht/bc8cfb82c55b75d85649585d3fd536a0#file-gistfile1-txt-L500
https://gist.github.com/devarsht/bc8cfb82c55b75d85649585d3fd536a0#file-gistfile1-txt-L712
https://gist.github.com/devarsht/bc8cfb82c55b75d85649585d3fd536a0#file-gistfile1-txt-L741
https://gist.github.com/devarsht/bc8cfb82c55b75d85649585d3fd536a0#file-gistfile1-txt-L750
https://gist.github.com/devarsht/bc8cfb82c55b75d85649585d3fd536a0#file-gistfile1-txt-L766
https://gist.github.com/devarsht/bc8cfb82c55b75d85649585d3fd536a0#file-gistfile1-txt-L773

Kindly let me know if you see some issues with this approach.

Best Regards,
Devarsh

> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Dec. 27, 2022, 9:50 a.m. UTC | #4 
On 27/12/2022 10:38, Devarsh Thakkar wrote:
> 
> 
> On 26/12/22 17:48, Krzysztof Kozlowski wrote:
>> On 23/12/2022 12:56, Devarsh Thakkar wrote:
>>> AM62 family of devices don't have a R5F cluster, instead
>>> they have single core DM R5F.
>>> Add new compatible string ti,am62-r5fss to support this scenario.
>>>
>>> When this new compatible is used don't allow cluster-mode
>>> property usage in device-tree as this implies that there
>>> is no R5F cluster available and only single R5F core
>>> is present.
>>>
>>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>>> ---
>>> V2: Avoid acronyms, use "Device Manager" instead of "DM"
>>> V3:
>>> - Use separate if block for each compatible for ti,cluster-mode property
>>> - Rearrange compatibles as per alphabatical order
>>> ---
>>>  .../bindings/remoteproc/ti,k3-r5f-rproc.yaml  | 68 +++++++++++++------
>>>  1 file changed, 47 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
>>> index fb9605f0655b..e8a861179bd9 100644
>>> --- a/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
>>> +++ b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
>>> @@ -21,6 +21,9 @@ description: |
>>>    called "Single-CPU" mode, where only Core0 is used, but with ability to use
>>>    Core1's TCMs as well.
>>>  
>>> +  AM62 SoC family support a single R5F core only which runs Device Manager
>>> +  firmware and can also be used as a remote processor with IPC communication.
>>> +
>>>    Each Dual-Core R5F sub-system is represented as a single DTS node
>>>    representing the cluster, with a pair of child DT nodes representing
>>>    the individual R5F cores. Each node has a number of required or optional
>>> @@ -28,16 +31,20 @@ description: |
>>>    the device management of the remote processor and to communicate with the
>>>    remote processor.
>>>  
>>> +  Since AM62 SoC family only support a single core, there is no cluster-mode
>>> +  property setting required for it.
>>> +
>>>  properties:
>>>    $nodename:
>>>      pattern: "^r5fss(@.*)?"
>>>  
>>>    compatible:
>>>      enum:
>>> +      - ti,am62-r5fss
>>> +      - ti,am64-r5fss
>>>        - ti,am654-r5fss
>>> -      - ti,j721e-r5fss
>>>        - ti,j7200-r5fss
>>> -      - ti,am64-r5fss
>>> +      - ti,j721e-r5fss
>>>        - ti,j721s2-r5fss
>>>  
>>>    power-domains:
>>> @@ -80,7 +87,9 @@ patternProperties:
>>>        node representing a TI instantiation of the Arm Cortex R5F core. There
>>>        are some specific integration differences for the IP like the usage of
>>>        a Region Address Translator (RAT) for translating the larger SoC bus
>>> -      addresses into a 32-bit address space for the processor.
>>> +      addresses into a 32-bit address space for the processor. For AM62x,
>>> +      the R5F Sub-System device node should only define one R5F child node
>>> +      as it has only one core available.
>>>  
>>>        Each R5F core has an associated 64 KB of Tightly-Coupled Memory (TCM)
>>>        internal memories split between two banks - TCMA and TCMB (further
>>> @@ -100,11 +109,12 @@ patternProperties:
>>>      properties:
>>>        compatible:
>>>          enum:
>>> -          - ti,am654-r5f
>>> -          - ti,j721e-r5f
>>> -          - ti,j7200-r5f
>>> -          - ti,am64-r5f
>>> -          - ti,j721s2-r5f
>>> +          - ti,am62-r5fss
>>> +          - ti,am64-r5fss
>>> +          - ti,am654-r5fss
>>> +          - ti,j7200-r5fss
>>> +          - ti,j721e-r5fss
>>> +          - ti,j721s2-r5fss
>>>  
> There is a problem here, the compatibles still need to be "-r5f" I will
> correct it in V4.
>>>        reg:
>>>          items:
>>> @@ -208,19 +218,35 @@ patternProperties:
>>>  
>>>      unevaluatedProperties: false
>>>  
>>> -if:
>>> -  properties:
>>> -    compatible:
>>> -      enum:
>>> -        - ti,am64-r5fss
>>> -then:
>>> -  properties:
>>> -    ti,cluster-mode:
>>> -      enum: [0, 2]
>>> -else:
>>> -  properties:
>>> -    ti,cluster-mode:
>>> -      enum: [0, 1]
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          enum:
>>> +            - ti,am64-r5fss
>>> +    then:
>>> +      properties:
>>> +        ti,cluster-mode:
>>> +          enum: [0, 2]
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          enum: ["ti,am654-r5fss", "ti,j7200-r5fss", "ti,j721e-r5fss",  "ti,j721s2-r5fss"]
>>
>> That's not how enums are spelled for such cases. Git grep for examples -
>> this should be a enum with each item in new entry, no quotes.
> Yeah, that was my initial thought but then I looked at section 4.9.3 of
> https://json-schema.org/understanding-json-schema/UnderstandingJSONSchema.pdf
> which had below example and based on that did this change thus avoiding
> separate entries for each enum.
> 
> Example:
> {
> "enum": ["red", "amber", "green", null, 42]
> }
> 
> To confirm whether the change works fine, I deliberately modified cluster-mode
> values for each of the SoC's beyond acceptable ranges as seen in
> https://gist.github.com/devarsht/1956063c8e39f1bdbad3574ea96b95a3
> and then ran "make dtbs_check" and it was able to catch the inappropriate
> values of cluster-mode as seen in below logs :
> 
> https://gist.github.com/devarsht/bc8cfb82c55b75d85649585d3fd536a0#file-gistfile1-txt-L392
> https://gist.github.com/devarsht/bc8cfb82c55b75d85649585d3fd536a0#file-gistfile1-txt-L500
> https://gist.github.com/devarsht/bc8cfb82c55b75d85649585d3fd536a0#file-gistfile1-txt-L712
> https://gist.github.com/devarsht/bc8cfb82c55b75d85649585d3fd536a0#file-gistfile1-txt-L741
> https://gist.github.com/devarsht/bc8cfb82c55b75d85649585d3fd536a0#file-gistfile1-txt-L750
> https://gist.github.com/devarsht/bc8cfb82c55b75d85649585d3fd536a0#file-gistfile1-txt-L766
> https://gist.github.com/devarsht/bc8cfb82c55b75d85649585d3fd536a0#file-gistfile1-txt-L773
> 
> Kindly let me know if you see some issues with this approach.

Your syntax is correct just not matching the coding style. Use coding
style matching existing bindings. It's the same in C. Just because you
can write:

void max() {
    ...
}

does not mean we accept such code.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
index fb9605f0655b..e8a861179bd9 100644
--- a/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
@@ -21,6 +21,9 @@  description: |
   called "Single-CPU" mode, where only Core0 is used, but with ability to use
   Core1's TCMs as well.
 
+  AM62 SoC family support a single R5F core only which runs Device Manager
+  firmware and can also be used as a remote processor with IPC communication.
+
   Each Dual-Core R5F sub-system is represented as a single DTS node
   representing the cluster, with a pair of child DT nodes representing
   the individual R5F cores. Each node has a number of required or optional
@@ -28,16 +31,20 @@  description: |
   the device management of the remote processor and to communicate with the
   remote processor.
 
+  Since AM62 SoC family only support a single core, there is no cluster-mode
+  property setting required for it.
+
 properties:
   $nodename:
     pattern: "^r5fss(@.*)?"
 
   compatible:
     enum:
+      - ti,am62-r5fss
+      - ti,am64-r5fss
       - ti,am654-r5fss
-      - ti,j721e-r5fss
       - ti,j7200-r5fss
-      - ti,am64-r5fss
+      - ti,j721e-r5fss
       - ti,j721s2-r5fss
 
   power-domains:
@@ -80,7 +87,9 @@  patternProperties:
       node representing a TI instantiation of the Arm Cortex R5F core. There
       are some specific integration differences for the IP like the usage of
       a Region Address Translator (RAT) for translating the larger SoC bus
-      addresses into a 32-bit address space for the processor.
+      addresses into a 32-bit address space for the processor. For AM62x,
+      the R5F Sub-System device node should only define one R5F child node
+      as it has only one core available.
 
       Each R5F core has an associated 64 KB of Tightly-Coupled Memory (TCM)
       internal memories split between two banks - TCMA and TCMB (further
@@ -100,11 +109,12 @@  patternProperties:
     properties:
       compatible:
         enum:
-          - ti,am654-r5f
-          - ti,j721e-r5f
-          - ti,j7200-r5f
-          - ti,am64-r5f
-          - ti,j721s2-r5f
+          - ti,am62-r5fss
+          - ti,am64-r5fss
+          - ti,am654-r5fss
+          - ti,j7200-r5fss
+          - ti,j721e-r5fss
+          - ti,j721s2-r5fss
 
       reg:
         items:
@@ -208,19 +218,35 @@  patternProperties:
 
     unevaluatedProperties: false
 
-if:
-  properties:
-    compatible:
-      enum:
-        - ti,am64-r5fss
-then:
-  properties:
-    ti,cluster-mode:
-      enum: [0, 2]
-else:
-  properties:
-    ti,cluster-mode:
-      enum: [0, 1]
+allOf:
+  - if:
+      properties:
+        compatible:
+          enum:
+            - ti,am64-r5fss
+    then:
+      properties:
+        ti,cluster-mode:
+          enum: [0, 2]
+
+  - if:
+      properties:
+        compatible:
+          enum: ["ti,am654-r5fss", "ti,j7200-r5fss", "ti,j721e-r5fss",  "ti,j721s2-r5fss"]
+    then:
+      properties:
+        ti,cluster-mode:
+          enum: [0, 1]
+
+  - if:
+      properties:
+        compatible:
+          enum:
+            - ti,am62-r5fss
+    then:
+      properties:
+        ti,cluster-mode: false
+
 
 required:
   - compatible