| Message ID | 20220506213226.257859-2-nfraprado@collabora.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Mediatek SCP dt-binding tweaks | expand |
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
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"?
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
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
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 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
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
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>;
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(-)