[PATCH v4 0/2] Mediatek SCP dt-binding tweaks

[PATCH v4 0/2] Mediatek SCP dt-binding tweaks

[Nícolas F. R. A. Prado]

Two simple patches for the Mediatek SCP dt-binding. The first fixes the
reg/reg-names property while the second adds a new optional
memory-region property.

v3: https://lore.kernel.org/all/20220503211114.2656099-1-nfraprado@collabora.com
v2: https://lore.kernel.org/all/20220502192420.2548512-1-nfraprado@collabora.com
v1: https://lore.kernel.org/all/20220429211111.2214119-1-nfraprado@collabora.com

Changes in v4:
- Reworked presence of l1tcm reg to be if:then: based and present only
  on mt8192/mt8195

Changes in v3:
- Made the cfg reg required again. After looking again into the mtk-scp
  driver, only l1tcm is optional.

Changes in v2:
- Dropped type and description from memory-region since it's a
  well-known property
- Set memory-region maxItems to 1

Nícolas F. R. A. Prado (2):
  dt-bindings: remoteproc: mediatek: Make l1tcm reg exclusive to mt819x
  dt-bindings: remoteproc: mediatek: Add optional memory-region to
    mtk,scp

 .../bindings/remoteproc/mtk,scp.yaml          | 72 ++++++++++++++-----
 1 file changed, 53 insertions(+), 19 deletions(-)

-- 
2.36.0

[PATCH v4 1/2] dt-bindings: remoteproc: mediatek: Make l1tcm reg exclusive to mt819x

[Nícolas F. R. A. Prado]

Commit ca23ecfdbd44 ("remoteproc/mediatek: support L1TCM") added support
for the l1tcm memory region on the MT8192 SCP, adding a new da_to_va
callback that handles l1tcm while keeping the old one for
back-compatibility with MT8183. However, since the mt8192 compatible was
missing from the dt-binding, the accompanying dt-binding commit
503c64cc42f1 ("dt-bindings: remoteproc: mediatek: add L1TCM memory region")
mistakenly added this reg as if it were for mt8183. And later
it became common to all platforms as their compatibles were added.

Fix the dt-binding so that the l1tcm reg can, and must, be present only
on the supported platforms: mt8192 and mt8195.

Fixes: 503c64cc42f1 ("dt-bindings: remoteproc: mediatek: add L1TCM memory region")
Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

---
The if:then: branches became rather long since it seems that it's not
possible to override the properties in them, only add new ones. That is,
I couldn't leave the items definition for all three regs in the global
reg-names and just decrease minItems and maxItems to 2 for
mt8183/mt8186.

Also I had to add a description to the global reg-names, since it
couldn't be neither missing nor empty.

Let me know if there are better ways to achieve this.

Changes in v4:
- Reworked presence of l1tcm reg to be if:then: based and present only
  on mt8192/mt8195
- Rewrote commit message
- Added Fixes tag

Changes in v3:
- Made the cfg reg required again. After looking again into the mtk-scp
  driver, only l1tcm is optional.
- Added mention that a dtbs_check warning gets fixed by patch in commit
  message.

 .../bindings/remoteproc/mtk,scp.yaml          | 69 ++++++++++++++-----
 1 file changed, 50 insertions(+), 19 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
index 823a236242de..e1793a85e610 100644
--- a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
@@ -23,15 +23,12 @@ properties:
 
   reg:
     description:
-      Should contain the address ranges for memory regions SRAM, CFG, and
-      L1TCM.
-    maxItems: 3
+      Should contain the address ranges for memory regions SRAM, CFG, and,
+      on some platforms, L1TCM.
 
   reg-names:
-    items:
-      - const: sram
-      - const: cfg
-      - const: l1tcm
+    description:
+      Register names depend on the platform.
 
   clocks:
     description:
@@ -50,16 +47,50 @@ required:
   - reg
   - reg-names
 
-if:
-  properties:
-    compatible:
-      enum:
-        - mediatek,mt8183-scp
-        - mediatek,mt8192-scp
-then:
-  required:
-    - clocks
-    - clock-names
+allOf:
+  - if:
+      properties:
+        compatible:
+          enum:
+            - mediatek,mt8183-scp
+            - mediatek,mt8192-scp
+    then:
+      required:
+        - clocks
+        - clock-names
+
+  - if:
+      properties:
+        compatible:
+          enum:
+            - mediatek,mt8183-scp
+            - mediatek,mt8186-scp
+    then:
+      properties:
+        reg:
+          minItems: 2
+          maxItems: 2
+        reg-names:
+          items:
+            - const: sram
+            - const: cfg
+
+  - if:
+      properties:
+        compatible:
+          enum:
+            - mediatek,mt8192-scp
+            - mediatek,mt8195-scp
+    then:
+      properties:
+        reg:
+          minItems: 3
+          maxItems: 3
+        reg-names:
+          items:
+            - const: sram
+            - const: cfg
+            - const: l1tcm
 
 additionalProperties:
   type: object
@@ -79,10 +110,10 @@ additionalProperties:
 
 examples:
   - |
-    #include <dt-bindings/clock/mt8183-clk.h>
+    #include <dt-bindings/clock/mt8192-clk.h>
 
     scp@10500000 {
-        compatible = "mediatek,mt8183-scp";
+        compatible = "mediatek,mt8192-scp";
         reg = <0x10500000 0x80000>,
               <0x10700000 0x8000>,
               <0x10720000 0xe0000>;
-- 
2.36.0

[PATCH v4 2/2] dt-bindings: remoteproc: mediatek: Add optional memory-region to mtk,scp

[Nícolas F. R. A. Prado]

The SCP co-processor can optionally be passed a reserved memory region
to use. Add this property in the dt-binding.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Acked-by: Rob Herring <robh@kernel.org>

---

(no changes since v2)

Changes in v2:
- Dropped type and description since it's a well-known property
- Set maxItems to 1

 Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
index e1793a85e610..b0503146d7fe 100644
--- a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
@@ -42,6 +42,9 @@ properties:
   interrupts:
     maxItems: 1
 
+  memory-region:
+    maxItems: 1
+
 required:
   - compatible
   - reg
-- 
2.36.0

Re: [PATCH v4 1/2] dt-bindings: remoteproc: mediatek: Make l1tcm reg exclusive to mt819x

[Krzysztof Kozlowski]

On 06/05/2022 23:32, Nícolas F. R. A. Prado wrote:
> Commit ca23ecfdbd44 ("remoteproc/mediatek: support L1TCM") added support
> for the l1tcm memory region on the MT8192 SCP, adding a new da_to_va
> callback that handles l1tcm while keeping the old one for
> back-compatibility with MT8183. However, since the mt8192 compatible was
> missing from the dt-binding, the accompanying dt-binding commit
> 503c64cc42f1 ("dt-bindings: remoteproc: mediatek: add L1TCM memory region")
> mistakenly added this reg as if it were for mt8183. And later
> it became common to all platforms as their compatibles were added.
> 
> Fix the dt-binding so that the l1tcm reg can, and must, be present only
> on the supported platforms: mt8192 and mt8195.
> 
> Fixes: 503c64cc42f1 ("dt-bindings: remoteproc: mediatek: add L1TCM memory region")
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> 
> ---
> The if:then: branches became rather long since it seems that it's not
> possible to override the properties in them, only add new ones. That is,
> I couldn't leave the items definition for all three regs in the global
> reg-names and just decrease minItems and maxItems to 2 for
> mt8183/mt8186.
> 
> Also I had to add a description to the global reg-names, since it
> couldn't be neither missing nor empty.

It is possible:
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/example-schema.yaml#L91

Keep constraints and list of names in properties. Then in allOf:if:then
raise minItems or lower maxItems, depending on the variant.


Best regards,
Krzysztof

Re: [PATCH v4 1/2] dt-bindings: remoteproc: mediatek: Make l1tcm reg exclusive to mt819x

[Tzung-Bi Shih]

On Sat, May 7, 2022 at 5:32 AM Nícolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - mediatek,mt8192-scp
> +            - mediatek,mt8195-scp
> +    then:
> +      properties:
> +        reg:
> +          minItems: 3
> +          maxItems: 3
> +        reg-names:
> +          items:
> +            - const: sram
> +            - const: cfg
> +            - const: l1tcm

"l1tcm" should be optional.  Does it make more sense by using "minItems: 2"?

Re: [PATCH v4 1/2] dt-bindings: remoteproc: mediatek: Make l1tcm reg exclusive to mt819x

[Nícolas F. R. A. Prado]

On Mon, May 09, 2022 at 10:27:18AM +0800, Tzung-Bi Shih wrote:
> On Sat, May 7, 2022 at 5:32 AM Nícolas F. R. A. Prado
> <nfraprado@collabora.com> wrote:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          enum:
> > +            - mediatek,mt8192-scp
> > +            - mediatek,mt8195-scp
> > +    then:
> > +      properties:
> > +        reg:
> > +          minItems: 3
> > +          maxItems: 3
> > +        reg-names:
> > +          items:
> > +            - const: sram
> > +            - const: cfg
> > +            - const: l1tcm
> 
> "l1tcm" should be optional.  Does it make more sense by using "minItems: 2"?

Hi Tzung-Bi,

thank you for the information. I did notice from the driver code that l1tcm was
treated as optional for mt8192, but since I wasn't sure if that was intended, I
kept it as required for the mt8192/mt8195 binding, since making it optional
later wouldn't break the ABI, but the opposite would.

But yes, since it is indeed optional for those platforms, I will lower minItems
to 2 for them in the next version.

Thanks,
Nícolas

Re: [PATCH v4 1/2] dt-bindings: remoteproc: mediatek: Make l1tcm reg exclusive to mt819x

[Nícolas F. R. A. Prado]

On Sat, May 07, 2022 at 06:33:31PM +0200, Krzysztof Kozlowski wrote:
> On 06/05/2022 23:32, Nícolas F. R. A. Prado wrote:
> > Commit ca23ecfdbd44 ("remoteproc/mediatek: support L1TCM") added support
> > for the l1tcm memory region on the MT8192 SCP, adding a new da_to_va
> > callback that handles l1tcm while keeping the old one for
> > back-compatibility with MT8183. However, since the mt8192 compatible was
> > missing from the dt-binding, the accompanying dt-binding commit
> > 503c64cc42f1 ("dt-bindings: remoteproc: mediatek: add L1TCM memory region")
> > mistakenly added this reg as if it were for mt8183. And later
> > it became common to all platforms as their compatibles were added.
> > 
> > Fix the dt-binding so that the l1tcm reg can, and must, be present only
> > on the supported platforms: mt8192 and mt8195.
> > 
> > Fixes: 503c64cc42f1 ("dt-bindings: remoteproc: mediatek: add L1TCM memory region")
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > 
> > ---
> > The if:then: branches became rather long since it seems that it's not
> > possible to override the properties in them, only add new ones. That is,
> > I couldn't leave the items definition for all three regs in the global
> > reg-names and just decrease minItems and maxItems to 2 for
> > mt8183/mt8186.
> > 
> > Also I had to add a description to the global reg-names, since it
> > couldn't be neither missing nor empty.
> 
> It is possible:
> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/example-schema.yaml#L91
> 
> Keep constraints and list of names in properties. Then in allOf:if:then
> raise minItems or lower maxItems, depending on the variant.

Hi Krzysztof,

that example only shows setting minItems to override the default value, but the
issue here is that it's not possible to override minItems/maxItems (after
they're already set, even if implicitly) with a different value in the if.

That is:

	properties:
	  reg-names:
	    items:
	      - const: sram
	      - const: cfg
	      - const: l1tcm

	if:
	  properties:
	    compatible:
	      enum:
		- mediatek,mt8183-scp
		- mediatek,mt8186-scp
	then:
	  properties:
	    reg-names:
	      minItems: 2
	      maxItems: 2

Generates the error on dtbs_check:

/home/nfraprado/ext/git/linux/arch/arm64/boot/dts/mediatek/mt8183-kukui-kakadu.dtb: scp@10500000: reg-names: ['sram', 'cfg'] is too short

I believe the tooling is implicitly adding

	      minItems: 3
	      maxItems: 3

to the common reg-names, and since it's not possible to override them, the
override to 2 doesn't work so they are kept at 3, causing the error.

Moving the minItems/maxItems to the common reg-names as a test gives:

/home/nfraprado/ext/git/linux/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml: properties:reg-names: {'minItems': 2, 'maxItems': 2, 'items': [{'const': 'sram'}, {'const': 'cfg'}, {'const': 'l1tcm'}]} should not be valid under {'required': ['maxItems']}
	hint: "maxItems" is not needed with an "items" list

That error, plus looking in the items meta-schema, suggests me that maxItems
isn't supposed to be set lower then the length of items. So even if the
minItems/maxItems override is fixed, there's still this issue. It seems like
defining the reg-names list separetely in each if branch is indeed the right way
to go.

Thanks,
Nícolas

Re: [PATCH v4 1/2] dt-bindings: remoteproc: mediatek: Make l1tcm reg exclusive to mt819x

[Krzysztof Kozlowski]

On 10/05/2022 18:50, Nícolas F. R. A. Prado wrote:
>>> Also I had to add a description to the global reg-names, since it
>>> couldn't be neither missing nor empty.
>>
>> It is possible:
>> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/example-schema.yaml#L91
>>
>> Keep constraints and list of names in properties. Then in allOf:if:then
>> raise minItems or lower maxItems, depending on the variant.
> 
> Hi Krzysztof,
> 
> that example only shows setting minItems to override the default value, but the
> issue here is that it's not possible to override minItems/maxItems (after
> they're already set, even if implicitly) with a different value in the if.

No, this example shows exactly what you need in first step - make one
item on the list optional.

There are several other examples for the entire picture or different
aproach:
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/example-schema.yaml#L91

https://elixir.bootlin.com/linux/v5.18-rc2/source/Documentation/devicetree/bindings/clock/samsung,exynos7885-clock.yaml#L53

> 
> That is:
> 
> 	properties:
> 	  reg-names:
> 	    items:
> 	      - const: sram
> 	      - const: cfg
> 	      - const: l1tcm

You did not use the example I gave you. Where is the minItems?

> 
> 	if:
> 	  properties:
> 	    compatible:
> 	      enum:
> 		- mediatek,mt8183-scp
> 		- mediatek,mt8186-scp
> 	then:
> 	  properties:
> 	    reg-names:
> 	      minItems: 2
> 	      maxItems: 2
> 
> Generates the error on dtbs_check:
> 
> /home/nfraprado/ext/git/linux/arch/arm64/boot/dts/mediatek/mt8183-kukui-kakadu.dtb: scp@10500000: reg-names: ['sram', 'cfg'] is too short

Missing minItems in first properties.

> 
> I believe the tooling is implicitly adding
> 
> 	      minItems: 3
> 	      maxItems: 3
> 
> to the common reg-names, and since it's not possible to override them, the
> override to 2 doesn't work so they are kept at 3, causing the error.
> 
> Moving the minItems/maxItems to the common reg-names as a test gives:

You cannot just. You need it in both places.

> 
> /home/nfraprado/ext/git/linux/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml: properties:reg-names: {'minItems': 2, 'maxItems': 2, 'items': [{'const': 'sram'}, {'const': 'cfg'}, {'const': 'l1tcm'}]} should not be valid under {'required': ['maxItems']}
> 	hint: "maxItems" is not needed with an "items" list
> 
> That error, plus looking in the items meta-schema, suggests me that maxItems
> isn't supposed to be set lower then the length of items. So even if the
> minItems/maxItems override is fixed, there's still this issue. It seems like
> defining the reg-names list separetely in each if branch is indeed the right way
> to go.



Best regards,
Krzysztof

Re: [PATCH v4 1/2] dt-bindings: remoteproc: mediatek: Make l1tcm reg exclusive to mt819x

[Krzysztof Kozlowski]

On 11/05/2022 11:12, Krzysztof Kozlowski wrote:
> On 10/05/2022 18:50, Nícolas F. R. A. Prado wrote:
>>>> Also I had to add a description to the global reg-names, since it
>>>> couldn't be neither missing nor empty.
>>>
>>> It is possible:
>>> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/example-schema.yaml#L91
>>>
>>> Keep constraints and list of names in properties. Then in allOf:if:then
>>> raise minItems or lower maxItems, depending on the variant.
>>
>> Hi Krzysztof,
>>
>> that example only shows setting minItems to override the default value, but the
>> issue here is that it's not possible to override minItems/maxItems (after
>> they're already set, even if implicitly) with a different value in the if.
> 
> No, this example shows exactly what you need in first step - make one
> item on the list optional.
> 
> There are several other examples for the entire picture or different
> aproach:
> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/example-schema.yaml#L91

Wait, this is the same link... because it exactly matches your case.

Best regards,
Krzysztof

Re: [PATCH v4 1/2] dt-bindings: remoteproc: mediatek: Make l1tcm reg exclusive to mt819x

[Nícolas F. R. A. Prado]

On Wed, May 11, 2022 at 11:12:45AM +0200, Krzysztof Kozlowski wrote:
> On 10/05/2022 18:50, Nícolas F. R. A. Prado wrote:
> >>> Also I had to add a description to the global reg-names, since it
> >>> couldn't be neither missing nor empty.
> >>
> >> It is possible:
> >> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/example-schema.yaml#L91
> >>
> >> Keep constraints and list of names in properties. Then in allOf:if:then
> >> raise minItems or lower maxItems, depending on the variant.
> > 
> > Hi Krzysztof,
> > 
> > that example only shows setting minItems to override the default value, but the
> > issue here is that it's not possible to override minItems/maxItems (after
> > they're already set, even if implicitly) with a different value in the if.
> 
> No, this example shows exactly what you need in first step - make one
> item on the list optional.
> 
> There are several other examples for the entire picture or different
> aproach:
> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/example-schema.yaml#L91
> 
> https://elixir.bootlin.com/linux/v5.18-rc2/source/Documentation/devicetree/bindings/clock/samsung,exynos7885-clock.yaml#L53
> 
> > 
> > That is:
> > 
> > 	properties:
> > 	  reg-names:
> > 	    items:
> > 	      - const: sram
> > 	      - const: cfg
> > 	      - const: l1tcm
> 
> You did not use the example I gave you. Where is the minItems?
> 
> > 
> > 	if:
> > 	  properties:
> > 	    compatible:
> > 	      enum:
> > 		- mediatek,mt8183-scp
> > 		- mediatek,mt8186-scp
> > 	then:
> > 	  properties:
> > 	    reg-names:
> > 	      minItems: 2
> > 	      maxItems: 2
> > 
> > Generates the error on dtbs_check:
> > 
> > /home/nfraprado/ext/git/linux/arch/arm64/boot/dts/mediatek/mt8183-kukui-kakadu.dtb: scp@10500000: reg-names: ['sram', 'cfg'] is too short
> 
> Missing minItems in first properties.
> 
> > 
> > I believe the tooling is implicitly adding
> > 
> > 	      minItems: 3
> > 	      maxItems: 3
> > 
> > to the common reg-names, and since it's not possible to override them, the
> > override to 2 doesn't work so they are kept at 3, causing the error.
> > 
> > Moving the minItems/maxItems to the common reg-names as a test gives:
> 
> You cannot just. You need it in both places.

OK, now I get it. I sent v5 addressing this.

Thanks,
Nícolas

> 
> > 
> > /home/nfraprado/ext/git/linux/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml: properties:reg-names: {'minItems': 2, 'maxItems': 2, 'items': [{'const': 'sram'}, {'const': 'cfg'}, {'const': 'l1tcm'}]} should not be valid under {'required': ['maxItems']}
> > 	hint: "maxItems" is not needed with an "items" list
> > 
> > That error, plus looking in the items meta-schema, suggests me that maxItems
> > isn't supposed to be set lower then the length of items. So even if the
> > minItems/maxItems override is fixed, there's still this issue. It seems like
> > defining the reg-names list separetely in each if branch is indeed the right way
> > to go.
> 
> 
> 
> Best regards,
> Krzysztof