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/[email protected]
v2: https://lore.kernel.org/all/[email protected]
v1: https://lore.kernel.org/all/[email protected]
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
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 <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
(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
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 <[email protected]>
>
> ---
> 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
On Sat, May 7, 2022 at 5:32 AM Nícolas F. R. A. Prado
<[email protected]> 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"?
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
> <[email protected]> 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
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 <[email protected]>
> >
> > ---
> > 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
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
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
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