| Message ID | 20220711082940.39539-3-krzysztof.kozlowski@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dt-bindings: mmc: / ARM: qcom: correct reg-names and clock entries | expand |
Hi On Mon, Jul 11, 2022 at 1:29 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > The entries in arrays must have fixed order, so the bindings and Linux > driver expecting various combinations of 'reg' addresses was never > actually conforming to guidelines. > > The 'core' reg entry is valid only for SDCC v4 and lower, so disallow it > in SDCC v5. SDCC v4 supports CQE and ICE, so allow them, even though > the qcom,sdhci-msm-v4 compatible is used also for earlier SoCs with SDCC > v2 or v3, so it is not entirely accurate. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > Changes since v1: > 1. Rework the patch based on Doug's feedback. > --- > .../devicetree/bindings/mmc/sdhci-msm.yaml | 61 ++++++++++++------- > 1 file changed, 38 insertions(+), 23 deletions(-) In the ${SUBJECT} I'm not sure what a "perp variant" is. Is that a typo or just a phrase I'm not aware of? > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml > index fc6e5221985a..2f0fdd65e908 100644 > --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml > +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml > @@ -49,33 +49,11 @@ properties: > > reg: > minItems: 1 > - items: > - - description: Host controller register map > - - description: SD Core register map > - - description: CQE register map > - - description: Inline Crypto Engine register map > + maxItems: 4 > > reg-names: > minItems: 1 > maxItems: 4 > - oneOf: > - - items: > - - const: hc > - - items: > - - const: hc > - - const: core > - - items: > - - const: hc > - - const: cqhci > - - items: > - - const: hc > - - const: cqhci > - - const: ice > - - items: > - - const: hc > - - const: core > - - const: cqhci > - - const: ice > > clocks: > minItems: 3 > @@ -177,6 +155,43 @@ required: > allOf: > - $ref: mmc-controller.yaml# > > + - if: > + properties: > + compatible: > + contains: > + enum: > + - qcom,sdhci-msm-v4 > + then: > + properties: > + reg: > + minItems: 2 > + items: > + - description: Host controller register map > + - description: SD Core register map > + - description: CQE register map > + - description: Inline Crypto Engine register map > + reg-names: > + minItems: 2 > + items: > + - const: hc > + - const: core > + - const: cqhci > + - const: ice > + else: > + properties: > + reg: > + minItems: 1 > + items: > + - description: Host controller register map > + - description: CQE register map > + - description: Inline Crypto Engine register map > + reg-names: > + minItems: 1 > + items: > + - const: hc > + - const: cqhci > + - const: ice Do you need to set "maxItems" here? If you don't then will it inherit the maxItems of 4 from above? -Doug
On 11/07/2022 16:52, Doug Anderson wrote: > Hi > > On Mon, Jul 11, 2022 at 1:29 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> The entries in arrays must have fixed order, so the bindings and Linux >> driver expecting various combinations of 'reg' addresses was never >> actually conforming to guidelines. >> >> The 'core' reg entry is valid only for SDCC v4 and lower, so disallow it >> in SDCC v5. SDCC v4 supports CQE and ICE, so allow them, even though >> the qcom,sdhci-msm-v4 compatible is used also for earlier SoCs with SDCC >> v2 or v3, so it is not entirely accurate. >> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> >> --- >> >> Changes since v1: >> 1. Rework the patch based on Doug's feedback. >> --- >> .../devicetree/bindings/mmc/sdhci-msm.yaml | 61 ++++++++++++------- >> 1 file changed, 38 insertions(+), 23 deletions(-) > > In the ${SUBJECT} I'm not sure what a "perp variant" is. Is that a > typo or just a phrase I'm not aware of? Should be: "per variants" > > >> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml >> index fc6e5221985a..2f0fdd65e908 100644 >> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml >> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml >> @@ -49,33 +49,11 @@ properties: >> >> reg: >> minItems: 1 >> - items: >> - - description: Host controller register map >> - - description: SD Core register map >> - - description: CQE register map >> - - description: Inline Crypto Engine register map >> + maxItems: 4 >> >> reg-names: >> minItems: 1 >> maxItems: 4 >> - oneOf: >> - - items: >> - - const: hc >> - - items: >> - - const: hc >> - - const: core >> - - items: >> - - const: hc >> - - const: cqhci >> - - items: >> - - const: hc >> - - const: cqhci >> - - const: ice >> - - items: >> - - const: hc >> - - const: core >> - - const: cqhci >> - - const: ice >> >> clocks: >> minItems: 3 >> @@ -177,6 +155,43 @@ required: >> allOf: >> - $ref: mmc-controller.yaml# >> >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - qcom,sdhci-msm-v4 >> + then: >> + properties: >> + reg: >> + minItems: 2 >> + items: >> + - description: Host controller register map >> + - description: SD Core register map >> + - description: CQE register map >> + - description: Inline Crypto Engine register map >> + reg-names: >> + minItems: 2 >> + items: >> + - const: hc >> + - const: core >> + - const: cqhci >> + - const: ice >> + else: >> + properties: >> + reg: >> + minItems: 1 >> + items: >> + - description: Host controller register map >> + - description: CQE register map >> + - description: Inline Crypto Engine register map >> + reg-names: >> + minItems: 1 >> + items: >> + - const: hc >> + - const: cqhci >> + - const: ice > > Do you need to set "maxItems" here? If you don't then will it inherit > the maxItems of 4 from above? No, items determine the size instead. Best regards, Krzysztof
On 11/07/2022 17:11, Doug Anderson wrote: > Hi, > > On Mon, Jul 11, 2022 at 7:53 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 11/07/2022 16:52, Doug Anderson wrote: >>> Hi >>> >>> On Mon, Jul 11, 2022 at 1:29 AM Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>> The entries in arrays must have fixed order, so the bindings and Linux >>>> driver expecting various combinations of 'reg' addresses was never >>>> actually conforming to guidelines. >>>> >>>> The 'core' reg entry is valid only for SDCC v4 and lower, so disallow it >>>> in SDCC v5. SDCC v4 supports CQE and ICE, so allow them, even though >>>> the qcom,sdhci-msm-v4 compatible is used also for earlier SoCs with SDCC >>>> v2 or v3, so it is not entirely accurate. >>>> >>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>> >>>> --- >>>> >>>> Changes since v1: >>>> 1. Rework the patch based on Doug's feedback. >>>> --- >>>> .../devicetree/bindings/mmc/sdhci-msm.yaml | 61 ++++++++++++------- >>>> 1 file changed, 38 insertions(+), 23 deletions(-) >>> >>> In the ${SUBJECT} I'm not sure what a "perp variant" is. Is that a >>> typo or just a phrase I'm not aware of? >> >> Should be: >> "per variants" >> >>> >>> >>>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml >>>> index fc6e5221985a..2f0fdd65e908 100644 >>>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml >>>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml >>>> @@ -49,33 +49,11 @@ properties: >>>> >>>> reg: >>>> minItems: 1 >>>> - items: >>>> - - description: Host controller register map >>>> - - description: SD Core register map >>>> - - description: CQE register map >>>> - - description: Inline Crypto Engine register map >>>> + maxItems: 4 >>>> >>>> reg-names: >>>> minItems: 1 >>>> maxItems: 4 >>>> - oneOf: >>>> - - items: >>>> - - const: hc >>>> - - items: >>>> - - const: hc >>>> - - const: core >>>> - - items: >>>> - - const: hc >>>> - - const: cqhci >>>> - - items: >>>> - - const: hc >>>> - - const: cqhci >>>> - - const: ice >>>> - - items: >>>> - - const: hc >>>> - - const: core >>>> - - const: cqhci >>>> - - const: ice >>>> >>>> clocks: >>>> minItems: 3 >>>> @@ -177,6 +155,43 @@ required: >>>> allOf: >>>> - $ref: mmc-controller.yaml# >>>> >>>> + - if: >>>> + properties: >>>> + compatible: >>>> + contains: >>>> + enum: >>>> + - qcom,sdhci-msm-v4 >>>> + then: >>>> + properties: >>>> + reg: >>>> + minItems: 2 >>>> + items: >>>> + - description: Host controller register map >>>> + - description: SD Core register map >>>> + - description: CQE register map >>>> + - description: Inline Crypto Engine register map >>>> + reg-names: >>>> + minItems: 2 >>>> + items: >>>> + - const: hc >>>> + - const: core >>>> + - const: cqhci >>>> + - const: ice >>>> + else: >>>> + properties: >>>> + reg: >>>> + minItems: 1 >>>> + items: >>>> + - description: Host controller register map >>>> + - description: CQE register map >>>> + - description: Inline Crypto Engine register map >>>> + reg-names: >>>> + minItems: 1 >>>> + items: >>>> + - const: hc >>>> + - const: cqhci >>>> + - const: ice >>> >>> Do you need to set "maxItems" here? If you don't then will it inherit >>> the maxItems of 4 from above? >> >> No, items determine the size instead. > > Can you just remove the "maxItems" from above then? Does it buy us anything? There is no maxItems directly here... > > In any case, with the ${SUBJECT} fixed: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> Best regards, Krzysztof
Hi, On Tue, Jul 12, 2022 at 12:02 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 11/07/2022 17:11, Doug Anderson wrote: > > Hi, > > > > On Mon, Jul 11, 2022 at 7:53 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 11/07/2022 16:52, Doug Anderson wrote: > >>> Hi > >>> > >>> On Mon, Jul 11, 2022 at 1:29 AM Krzysztof Kozlowski > >>> <krzysztof.kozlowski@linaro.org> wrote: > >>>> > >>>> The entries in arrays must have fixed order, so the bindings and Linux > >>>> driver expecting various combinations of 'reg' addresses was never > >>>> actually conforming to guidelines. > >>>> > >>>> The 'core' reg entry is valid only for SDCC v4 and lower, so disallow it > >>>> in SDCC v5. SDCC v4 supports CQE and ICE, so allow them, even though > >>>> the qcom,sdhci-msm-v4 compatible is used also for earlier SoCs with SDCC > >>>> v2 or v3, so it is not entirely accurate. > >>>> > >>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >>>> > >>>> --- > >>>> > >>>> Changes since v1: > >>>> 1. Rework the patch based on Doug's feedback. > >>>> --- > >>>> .../devicetree/bindings/mmc/sdhci-msm.yaml | 61 ++++++++++++------- > >>>> 1 file changed, 38 insertions(+), 23 deletions(-) > >>> > >>> In the ${SUBJECT} I'm not sure what a "perp variant" is. Is that a > >>> typo or just a phrase I'm not aware of? > >> > >> Should be: > >> "per variants" > >> > >>> > >>> > >>>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml > >>>> index fc6e5221985a..2f0fdd65e908 100644 > >>>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml > >>>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml > >>>> @@ -49,33 +49,11 @@ properties: > >>>> > >>>> reg: > >>>> minItems: 1 > >>>> - items: > >>>> - - description: Host controller register map > >>>> - - description: SD Core register map > >>>> - - description: CQE register map > >>>> - - description: Inline Crypto Engine register map > >>>> + maxItems: 4 > >>>> > >>>> reg-names: > >>>> minItems: 1 > >>>> maxItems: 4 > >>>> - oneOf: > >>>> - - items: > >>>> - - const: hc > >>>> - - items: > >>>> - - const: hc > >>>> - - const: core > >>>> - - items: > >>>> - - const: hc > >>>> - - const: cqhci > >>>> - - items: > >>>> - - const: hc > >>>> - - const: cqhci > >>>> - - const: ice > >>>> - - items: > >>>> - - const: hc > >>>> - - const: core > >>>> - - const: cqhci > >>>> - - const: ice > >>>> > >>>> clocks: > >>>> minItems: 3 > >>>> @@ -177,6 +155,43 @@ required: > >>>> allOf: > >>>> - $ref: mmc-controller.yaml# > >>>> > >>>> + - if: > >>>> + properties: > >>>> + compatible: > >>>> + contains: > >>>> + enum: > >>>> + - qcom,sdhci-msm-v4 > >>>> + then: > >>>> + properties: > >>>> + reg: > >>>> + minItems: 2 > >>>> + items: > >>>> + - description: Host controller register map > >>>> + - description: SD Core register map > >>>> + - description: CQE register map > >>>> + - description: Inline Crypto Engine register map > >>>> + reg-names: > >>>> + minItems: 2 > >>>> + items: > >>>> + - const: hc > >>>> + - const: core > >>>> + - const: cqhci > >>>> + - const: ice > >>>> + else: > >>>> + properties: > >>>> + reg: > >>>> + minItems: 1 > >>>> + items: > >>>> + - description: Host controller register map > >>>> + - description: CQE register map > >>>> + - description: Inline Crypto Engine register map > >>>> + reg-names: > >>>> + minItems: 1 > >>>> + items: > >>>> + - const: hc > >>>> + - const: cqhci > >>>> + - const: ice > >>> > >>> Do you need to set "maxItems" here? If you don't then will it inherit > >>> the maxItems of 4 from above? > >> > >> No, items determine the size instead. > > > > Can you just remove the "maxItems" from above then? Does it buy us anything? > > There is no maxItems directly here... Sorry, I mean above in the schema. After your patch the schema is effectively: reg: minItems: 1 maxItems: 4 reg-names: minItems: 1 maxItems: 4 ... allOf: - if: blah-blah-blah then: properties: reg: minItems: 2 items: - description: ... - description: ... - description: ... - description: ... reg-names: blah-blah-blah else: blah-blah-blah I'm asking about the maxItems _above_, AKA in the section: reg: minItems: 1 maxItems: 4 reg-names: minItems: 1 maxItems: 4 Can we remove the "maxItems: 4" from the above and have it just be: reg: minItems: 1 reg-names: minItems: 1 -Doug
On 12/07/2022 16:29, Doug Anderson wrote: > Hi, > > On Tue, Jul 12, 2022 at 12:02 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 11/07/2022 17:11, Doug Anderson wrote: >>> Hi, >>> >>> On Mon, Jul 11, 2022 at 7:53 AM Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>> On 11/07/2022 16:52, Doug Anderson wrote: >>>>> Hi >>>>> >>>>> On Mon, Jul 11, 2022 at 1:29 AM Krzysztof Kozlowski >>>>> <krzysztof.kozlowski@linaro.org> wrote: >>>>>> >>>>>> The entries in arrays must have fixed order, so the bindings and Linux >>>>>> driver expecting various combinations of 'reg' addresses was never >>>>>> actually conforming to guidelines. >>>>>> >>>>>> The 'core' reg entry is valid only for SDCC v4 and lower, so disallow it >>>>>> in SDCC v5. SDCC v4 supports CQE and ICE, so allow them, even though >>>>>> the qcom,sdhci-msm-v4 compatible is used also for earlier SoCs with SDCC >>>>>> v2 or v3, so it is not entirely accurate. >>>>>> >>>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>>>> >>>>>> --- >>>>>> >>>>>> Changes since v1: >>>>>> 1. Rework the patch based on Doug's feedback. >>>>>> --- >>>>>> .../devicetree/bindings/mmc/sdhci-msm.yaml | 61 ++++++++++++------- >>>>>> 1 file changed, 38 insertions(+), 23 deletions(-) >>>>> >>>>> In the ${SUBJECT} I'm not sure what a "perp variant" is. Is that a >>>>> typo or just a phrase I'm not aware of? >>>> >>>> Should be: >>>> "per variants" >>>> >>>>> >>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml >>>>>> index fc6e5221985a..2f0fdd65e908 100644 >>>>>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml >>>>>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml >>>>>> @@ -49,33 +49,11 @@ properties: >>>>>> >>>>>> reg: >>>>>> minItems: 1 >>>>>> - items: >>>>>> - - description: Host controller register map >>>>>> - - description: SD Core register map >>>>>> - - description: CQE register map >>>>>> - - description: Inline Crypto Engine register map >>>>>> + maxItems: 4 >>>>>> >>>>>> reg-names: >>>>>> minItems: 1 >>>>>> maxItems: 4 >>>>>> - oneOf: >>>>>> - - items: >>>>>> - - const: hc >>>>>> - - items: >>>>>> - - const: hc >>>>>> - - const: core >>>>>> - - items: >>>>>> - - const: hc >>>>>> - - const: cqhci >>>>>> - - items: >>>>>> - - const: hc >>>>>> - - const: cqhci >>>>>> - - const: ice >>>>>> - - items: >>>>>> - - const: hc >>>>>> - - const: core >>>>>> - - const: cqhci >>>>>> - - const: ice >>>>>> >>>>>> clocks: >>>>>> minItems: 3 >>>>>> @@ -177,6 +155,43 @@ required: >>>>>> allOf: >>>>>> - $ref: mmc-controller.yaml# >>>>>> >>>>>> + - if: >>>>>> + properties: >>>>>> + compatible: >>>>>> + contains: >>>>>> + enum: >>>>>> + - qcom,sdhci-msm-v4 >>>>>> + then: >>>>>> + properties: >>>>>> + reg: >>>>>> + minItems: 2 >>>>>> + items: >>>>>> + - description: Host controller register map >>>>>> + - description: SD Core register map >>>>>> + - description: CQE register map >>>>>> + - description: Inline Crypto Engine register map >>>>>> + reg-names: >>>>>> + minItems: 2 >>>>>> + items: >>>>>> + - const: hc >>>>>> + - const: core >>>>>> + - const: cqhci >>>>>> + - const: ice >>>>>> + else: >>>>>> + properties: >>>>>> + reg: >>>>>> + minItems: 1 >>>>>> + items: >>>>>> + - description: Host controller register map >>>>>> + - description: CQE register map >>>>>> + - description: Inline Crypto Engine register map >>>>>> + reg-names: >>>>>> + minItems: 1 >>>>>> + items: >>>>>> + - const: hc >>>>>> + - const: cqhci >>>>>> + - const: ice >>>>> >>>>> Do you need to set "maxItems" here? If you don't then will it inherit >>>>> the maxItems of 4 from above? >>>> >>>> No, items determine the size instead. >>> >>> Can you just remove the "maxItems" from above then? Does it buy us anything? >> >> There is no maxItems directly here... > > Sorry, I mean above in the schema. After your patch the schema is effectively: > > reg: > minItems: 1 > maxItems: 4 > reg-names: > minItems: 1 > maxItems: 4 > > ... > > allOf: > - if: > blah-blah-blah > then: > properties: > reg: > minItems: 2 > items: > - description: ... > - description: ... > - description: ... > - description: ... > reg-names: > blah-blah-blah > else: > blah-blah-blah > > I'm asking about the maxItems _above_, AKA in the section: > > reg: > minItems: 1 > maxItems: 4 > reg-names: > minItems: 1 > maxItems: 4 > > Can we remove the "maxItems: 4" from the above and have it just be: > > reg: > minItems: 1 > reg-names: > minItems: 1 > Yes, we can, but preferred is to have it because it makes the broad constraints easily visible. You don't need to check each if:else branch to find upper bounds or check if maxItems are defined at all. This also matches pattern used in bindings without allOf:if:then - each time you are expected to see array types constraint in the list of properties. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml index fc6e5221985a..2f0fdd65e908 100644 --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml @@ -49,33 +49,11 @@ properties: reg: minItems: 1 - items: - - description: Host controller register map - - description: SD Core register map - - description: CQE register map - - description: Inline Crypto Engine register map + maxItems: 4 reg-names: minItems: 1 maxItems: 4 - oneOf: - - items: - - const: hc - - items: - - const: hc - - const: core - - items: - - const: hc - - const: cqhci - - items: - - const: hc - - const: cqhci - - const: ice - - items: - - const: hc - - const: core - - const: cqhci - - const: ice clocks: minItems: 3 @@ -177,6 +155,43 @@ required: allOf: - $ref: mmc-controller.yaml# + - if: + properties: + compatible: + contains: + enum: + - qcom,sdhci-msm-v4 + then: + properties: + reg: + minItems: 2 + items: + - description: Host controller register map + - description: SD Core register map + - description: CQE register map + - description: Inline Crypto Engine register map + reg-names: + minItems: 2 + items: + - const: hc + - const: core + - const: cqhci + - const: ice + else: + properties: + reg: + minItems: 1 + items: + - description: Host controller register map + - description: CQE register map + - description: Inline Crypto Engine register map + reg-names: + minItems: 1 + items: + - const: hc + - const: cqhci + - const: ice + unevaluatedProperties: false examples:
The entries in arrays must have fixed order, so the bindings and Linux driver expecting various combinations of 'reg' addresses was never actually conforming to guidelines. The 'core' reg entry is valid only for SDCC v4 and lower, so disallow it in SDCC v5. SDCC v4 supports CQE and ICE, so allow them, even though the qcom,sdhci-msm-v4 compatible is used also for earlier SoCs with SDCC v2 or v3, so it is not entirely accurate. Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- Changes since v1: 1. Rework the patch based on Doug's feedback. --- .../devicetree/bindings/mmc/sdhci-msm.yaml | 61 ++++++++++++------- 1 file changed, 38 insertions(+), 23 deletions(-)