| Message ID | 20221209-dt-binding-ufs-v1-0-8d502f0e18d5@fairphone.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dt-bindings: ufs: qcom: Add reg-names property for ICE | expand |
On 09/12/2022 15:29, Luca Weiss wrote: > The code in ufs-qcom-ice.c needs the ICE reg to be named "ice". Add this > in the bindings so the existing dts can validate successfully. > > Also sm8450 is using ICE since commit 276ee34a40c1 ("arm64: dts: qcom: > sm8450: add Inline Crypto Engine registers and clock") so move the > compatible to the correct if. > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > --- > (no cover subject) > > The only remaining validation issues I see is the following on sc8280xp-crd.dtb > and sa8540p-ride.dtb: > > Unevaluated properties are not allowed ('required-opps', 'dma-coherent' were unexpected) > > Maybe someone who knows something about this can handle this? > > And the patch adding qcom,sm6115-ufshc hasn't been applied yet. > --- > Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml > index f2d6298d926c..58a2fb2c83c3 100644 > --- a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml > +++ b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml > @@ -102,7 +102,6 @@ allOf: > - qcom,sc8280xp-ufshc > - qcom,sm8250-ufshc > - qcom,sm8350-ufshc > - - qcom,sm8450-ufshc > then: > properties: > clocks: > @@ -130,6 +129,7 @@ allOf: > - qcom,sdm845-ufshc > - qcom,sm6350-ufshc > - qcom,sm8150-ufshc > + - qcom,sm8450-ufshc > then: > properties: > clocks: > @@ -149,6 +149,12 @@ allOf: > reg: > minItems: 2 > maxItems: 2 > + reg-names: There are no reg-names in top-level, so it's surprising to see its customized here. It seems no one ever documented that usage... > + items: > + - const: std > + - const: ice > + required: > + - reg-names > > - if: > properties: > > --- > base-commit: f925116b24c0c42dc6d5ab5111c55fd7f74e8dc7 > change-id: 20221209-dt-binding-ufs-2d7f64797ff2 > > Best regards, Best regards, Krzysztof
On Fri Dec 9, 2022 at 4:05 PM CET, Krzysztof Kozlowski wrote: > On 09/12/2022 15:29, Luca Weiss wrote: > > The code in ufs-qcom-ice.c needs the ICE reg to be named "ice". Add this > > in the bindings so the existing dts can validate successfully. > > > > Also sm8450 is using ICE since commit 276ee34a40c1 ("arm64: dts: qcom: > > sm8450: add Inline Crypto Engine registers and clock") so move the > > compatible to the correct if. > > > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > > --- > > (no cover subject) > > > > The only remaining validation issues I see is the following on sc8280xp-crd.dtb > > and sa8540p-ride.dtb: > > > > Unevaluated properties are not allowed ('required-opps', 'dma-coherent' were unexpected) > > > > Maybe someone who knows something about this can handle this? > > > > And the patch adding qcom,sm6115-ufshc hasn't been applied yet. > > --- > > Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml > > index f2d6298d926c..58a2fb2c83c3 100644 > > --- a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml > > +++ b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml > > @@ -102,7 +102,6 @@ allOf: > > - qcom,sc8280xp-ufshc > > - qcom,sm8250-ufshc > > - qcom,sm8350-ufshc > > - - qcom,sm8450-ufshc > > then: > > properties: > > clocks: > > @@ -130,6 +129,7 @@ allOf: > > - qcom,sdm845-ufshc > > - qcom,sm6350-ufshc > > - qcom,sm8150-ufshc > > + - qcom,sm8450-ufshc > > then: > > properties: > > clocks: > > @@ -149,6 +149,12 @@ allOf: > > reg: > > minItems: 2 > > maxItems: 2 > > + reg-names: > > There are no reg-names in top-level, so it's surprising to see its > customized here. It seems no one ever documented that usage... From what I can tell, from driver side all devices not using ICE don't need reg-names, only the "ice" reg is referenced by name in the driver. I didn't add it top-level because with only one reg I think we're not supposed to use reg-names, right? Regards Luca > > > + items: > > + - const: std > > + - const: ice > > + required: > > + - reg-names > > > > - if: > > properties: > > > > --- > > base-commit: f925116b24c0c42dc6d5ab5111c55fd7f74e8dc7 > > change-id: 20221209-dt-binding-ufs-2d7f64797ff2 > > > > Best regards, > > Best regards, > Krzysztof
On 09/12/2022 16:11, Luca Weiss wrote: > On Fri Dec 9, 2022 at 4:05 PM CET, Krzysztof Kozlowski wrote: >> On 09/12/2022 15:29, Luca Weiss wrote: >>> The code in ufs-qcom-ice.c needs the ICE reg to be named "ice". Add this >>> in the bindings so the existing dts can validate successfully. >>> >>> Also sm8450 is using ICE since commit 276ee34a40c1 ("arm64: dts: qcom: >>> sm8450: add Inline Crypto Engine registers and clock") so move the >>> compatible to the correct if. >>> >>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> >>> --- >>> (no cover subject) >>> >>> The only remaining validation issues I see is the following on sc8280xp-crd.dtb >>> and sa8540p-ride.dtb: >>> >>> Unevaluated properties are not allowed ('required-opps', 'dma-coherent' were unexpected) >>> >>> Maybe someone who knows something about this can handle this? >>> >>> And the patch adding qcom,sm6115-ufshc hasn't been applied yet. >>> --- >>> Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml >>> index f2d6298d926c..58a2fb2c83c3 100644 >>> --- a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml >>> +++ b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml >>> @@ -102,7 +102,6 @@ allOf: >>> - qcom,sc8280xp-ufshc >>> - qcom,sm8250-ufshc >>> - qcom,sm8350-ufshc >>> - - qcom,sm8450-ufshc >>> then: >>> properties: >>> clocks: >>> @@ -130,6 +129,7 @@ allOf: >>> - qcom,sdm845-ufshc >>> - qcom,sm6350-ufshc >>> - qcom,sm8150-ufshc >>> + - qcom,sm8450-ufshc >>> then: >>> properties: >>> clocks: >>> @@ -149,6 +149,12 @@ allOf: >>> reg: >>> minItems: 2 >>> maxItems: 2 >>> + reg-names: >> >> There are no reg-names in top-level, so it's surprising to see its >> customized here. It seems no one ever documented that usage... > > From what I can tell, from driver side all devices not using ICE don't > need reg-names, only the "ice" reg is referenced by name in the driver. > > I didn't add it top-level because with only one reg I think we're not > supposed to use reg-names, right? And you still won't need to use. Yet property should be rather described in top-level which also will unify the items here (so no different 2-item reg-names in variants). Just add it to top-level with minItems: 1 and per variant customize: 1. maxItems: 1 2. minItems: 2 + required The "required" is a bit questionable... this was never added by Eric to the bindings. Driver support and DTS were added completely skipping bindings... Best regards, Krzysztof
On Fri, Dec 09, 2022 at 03:29:47PM +0100, Luca Weiss wrote: > The code in ufs-qcom-ice.c needs the ICE reg to be named "ice". Add this > in the bindings so the existing dts can validate successfully. > > Also sm8450 is using ICE since commit 276ee34a40c1 ("arm64: dts: qcom: > sm8450: add Inline Crypto Engine registers and clock") so move the > compatible to the correct if. > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > --- > (no cover subject) > > The only remaining validation issues I see is the following on sc8280xp-crd.dtb > and sa8540p-ride.dtb: > > Unevaluated properties are not allowed ('required-opps', 'dma-coherent' were unexpected) > > Maybe someone who knows something about this can handle this? That's being addressed here: https://lore.kernel.org/lkml/20221205100837.29212-2-johan+linaro@kernel.org/ Johan
On Fri, 09 Dec 2022 15:29:47 +0100, Luca Weiss wrote: > The code in ufs-qcom-ice.c needs the ICE reg to be named "ice". Add this > in the bindings so the existing dts can validate successfully. > > Also sm8450 is using ICE since commit 276ee34a40c1 ("arm64: dts: qcom: > sm8450: add Inline Crypto Engine registers and clock") so move the > compatible to the correct if. > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > --- > (no cover subject) > > The only remaining validation issues I see is the following on sc8280xp-crd.dtb > and sa8540p-ride.dtb: > > Unevaluated properties are not allowed ('required-opps', 'dma-coherent' were unexpected) > > Maybe someone who knows something about this can handle this? > > And the patch adding qcom,sm6115-ufshc hasn't been applied yet. > --- > Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/ufs/qcom,ufs.example.dtb: ufs@1d84000: clocks: [[4294967295, 151], [4294967295, 10], [4294967295, 150], [4294967295, 166], [4294967295, 0], [4294967295, 164], [4294967295, 160], [4294967295, 162]] is too short From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/ufs/qcom,ufs.example.dtb: ufs@1d84000: clock-names: ['core_clk', 'bus_aggr_clk', 'iface_clk', 'core_clk_unipro', 'ref_clk', 'tx_lane0_sync_clk', 'rx_lane0_sync_clk', 'rx_lane1_sync_clk'] is too short From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/ufs/qcom,ufs.example.dtb: ufs@1d84000: reg: [[0, 30949376, 0, 12288]] is too short From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/ufs/qcom,ufs.example.dtb: ufs@1d84000: 'reg-names' is a required property From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221209-dt-binding-ufs-v1-0-8d502f0e18d5@fairphone.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Fri, Dec 09, 2022 at 04:19:20PM +0100, Krzysztof Kozlowski wrote: > On 09/12/2022 16:11, Luca Weiss wrote: > > On Fri Dec 9, 2022 at 4:05 PM CET, Krzysztof Kozlowski wrote: > >> On 09/12/2022 15:29, Luca Weiss wrote: > >>> The code in ufs-qcom-ice.c needs the ICE reg to be named "ice". Add this > >>> in the bindings so the existing dts can validate successfully. > >>> > >>> Also sm8450 is using ICE since commit 276ee34a40c1 ("arm64: dts: qcom: > >>> sm8450: add Inline Crypto Engine registers and clock") so move the > >>> compatible to the correct if. > >>> > >>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > >>> --- > >>> (no cover subject) > >>> > >>> The only remaining validation issues I see is the following on sc8280xp-crd.dtb > >>> and sa8540p-ride.dtb: > >>> > >>> Unevaluated properties are not allowed ('required-opps', 'dma-coherent' were unexpected) > >>> > >>> Maybe someone who knows something about this can handle this? > >>> > >>> And the patch adding qcom,sm6115-ufshc hasn't been applied yet. > >>> --- > >>> Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 8 +++++++- > >>> 1 file changed, 7 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml > >>> index f2d6298d926c..58a2fb2c83c3 100644 > >>> --- a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml > >>> +++ b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml > >>> @@ -102,7 +102,6 @@ allOf: > >>> - qcom,sc8280xp-ufshc > >>> - qcom,sm8250-ufshc > >>> - qcom,sm8350-ufshc > >>> - - qcom,sm8450-ufshc > >>> then: > >>> properties: > >>> clocks: > >>> @@ -130,6 +129,7 @@ allOf: > >>> - qcom,sdm845-ufshc > >>> - qcom,sm6350-ufshc > >>> - qcom,sm8150-ufshc > >>> + - qcom,sm8450-ufshc > >>> then: > >>> properties: > >>> clocks: > >>> @@ -149,6 +149,12 @@ allOf: > >>> reg: > >>> minItems: 2 > >>> maxItems: 2 > >>> + reg-names: > >> > >> There are no reg-names in top-level, so it's surprising to see its > >> customized here. It seems no one ever documented that usage... > > > > From what I can tell, from driver side all devices not using ICE don't > > need reg-names, only the "ice" reg is referenced by name in the driver. > > > > I didn't add it top-level because with only one reg I think we're not > > supposed to use reg-names, right? > > And you still won't need to use. Yet property should be rather described > in top-level which also will unify the items here (so no different > 2-item reg-names in variants). > > Just add it to top-level with minItems: 1 and per variant customize: > 1. maxItems: 1 > 2. minItems: 2 + required > > The "required" is a bit questionable... this was never added by Eric to > the bindings. Driver support and DTS were added completely skipping > bindings... > Sorry about that. At the time (https://lore.kernel.org/linux-scsi/20200722051143.GU388985@builder.lan/T/#t) I didn't know there was a Documentation file that should have been updated. The UFS core assumes that the reg at index 0 is the UFS standard registers. It is not referenced by name. ufs-qcom already had an optional reg at index 1. I needed to add another optional reg. So I made the regs at index 1 and later be optional named regs: dev_ref_clk_ctrl_mem and ice. That seemed better than hardcoding the indices. Is it causing a problem that the UFS standard reg at index 0 is being mixed with named regs in the same list? - Eric
On 09/12/2022 20:35, Eric Biggers wrote: > On Fri, Dec 09, 2022 at 04:19:20PM +0100, Krzysztof Kozlowski wrote: >> On 09/12/2022 16:11, Luca Weiss wrote: >>> On Fri Dec 9, 2022 at 4:05 PM CET, Krzysztof Kozlowski wrote: >>>> On 09/12/2022 15:29, Luca Weiss wrote: >>>>> The code in ufs-qcom-ice.c needs the ICE reg to be named "ice". Add this >>>>> in the bindings so the existing dts can validate successfully. >>>>> >>>>> Also sm8450 is using ICE since commit 276ee34a40c1 ("arm64: dts: qcom: >>>>> sm8450: add Inline Crypto Engine registers and clock") so move the >>>>> compatible to the correct if. >>>>> >>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> >>>>> --- >>>>> (no cover subject) >>>>> >>>>> The only remaining validation issues I see is the following on sc8280xp-crd.dtb >>>>> and sa8540p-ride.dtb: >>>>> >>>>> Unevaluated properties are not allowed ('required-opps', 'dma-coherent' were unexpected) >>>>> >>>>> Maybe someone who knows something about this can handle this? >>>>> >>>>> And the patch adding qcom,sm6115-ufshc hasn't been applied yet. >>>>> --- >>>>> Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 8 +++++++- >>>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml >>>>> index f2d6298d926c..58a2fb2c83c3 100644 >>>>> --- a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml >>>>> +++ b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml >>>>> @@ -102,7 +102,6 @@ allOf: >>>>> - qcom,sc8280xp-ufshc >>>>> - qcom,sm8250-ufshc >>>>> - qcom,sm8350-ufshc >>>>> - - qcom,sm8450-ufshc >>>>> then: >>>>> properties: >>>>> clocks: >>>>> @@ -130,6 +129,7 @@ allOf: >>>>> - qcom,sdm845-ufshc >>>>> - qcom,sm6350-ufshc >>>>> - qcom,sm8150-ufshc >>>>> + - qcom,sm8450-ufshc >>>>> then: >>>>> properties: >>>>> clocks: >>>>> @@ -149,6 +149,12 @@ allOf: >>>>> reg: >>>>> minItems: 2 >>>>> maxItems: 2 >>>>> + reg-names: >>>> >>>> There are no reg-names in top-level, so it's surprising to see its >>>> customized here. It seems no one ever documented that usage... >>> >>> From what I can tell, from driver side all devices not using ICE don't >>> need reg-names, only the "ice" reg is referenced by name in the driver. >>> >>> I didn't add it top-level because with only one reg I think we're not >>> supposed to use reg-names, right? >> >> And you still won't need to use. Yet property should be rather described >> in top-level which also will unify the items here (so no different >> 2-item reg-names in variants). >> >> Just add it to top-level with minItems: 1 and per variant customize: >> 1. maxItems: 1 >> 2. minItems: 2 + required >> >> The "required" is a bit questionable... this was never added by Eric to >> the bindings. Driver support and DTS were added completely skipping >> bindings... >> > > Sorry about that. At the time > (https://lore.kernel.org/linux-scsi/20200722051143.GU388985@builder.lan/T/#t) > I didn't know there was a Documentation file that should have been updated. Any change regarding bindings (so adding/changing compatibles, properties, nodes) a driver is using must be followed by... change in the bindings. > > The UFS core assumes that the reg at index 0 is the UFS standard registers. > It is not referenced by name. > > ufs-qcom already had an optional reg at index 1. I needed to add another > optional reg. So I made the regs at index 1 and later be optional named regs: > dev_ref_clk_ctrl_mem and ice. That seemed better than hardcoding the indices. > > Is it causing a problem that the UFS standard reg at index 0 is being mixed with > named regs in the same list? The indexes should be ordered (hard-coded), not flexible. If there is already second IO address at index 1, then the patch does not look correct. When fixing, please fix it completely. Best regards, Krzysztof
On 09/12/2022 15:29, Luca Weiss wrote: > The code in ufs-qcom-ice.c needs the ICE reg to be named "ice". Add this > in the bindings so the existing dts can validate successfully. > > Also sm8450 is using ICE since commit 276ee34a40c1 ("arm64: dts: qcom: > sm8450: add Inline Crypto Engine registers and clock") so move the > compatible to the correct if. > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > --- > (no cover subject) > > The only remaining validation issues I see is the following on sc8280xp-crd.dtb > and sa8540p-ride.dtb: > Any plans on fixing the patch (after testing it) and resending? Best regards, Krzysztof
Hi Krzysztof, On Wed Dec 28, 2022 at 12:50 PM CET, Krzysztof Kozlowski wrote: > On 09/12/2022 15:29, Luca Weiss wrote: > > The code in ufs-qcom-ice.c needs the ICE reg to be named "ice". Add this > > in the bindings so the existing dts can validate successfully. > > > > Also sm8450 is using ICE since commit 276ee34a40c1 ("arm64: dts: qcom: > > sm8450: add Inline Crypto Engine registers and clock") so move the > > compatible to the correct if. > > > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > > --- > > (no cover subject) > > > > The only remaining validation issues I see is the following on sc8280xp-crd.dtb > > and sa8540p-ride.dtb: > > > > Any plans on fixing the patch (after testing it) and resending? I wasn't quite sure how to fix the comments, but re-reading them this comment from you is how you expect it to be in v2? > Just add it to top-level with minItems: 1 and per variant customize: > 1. maxItems: 1 > 2. minItems: 2 + required If so I can spin v2 maybe today still. Regards Luca > > Best regards, > Krzysztof
On 28/12/2022 12:53, Luca Weiss wrote: > Hi Krzysztof, > > On Wed Dec 28, 2022 at 12:50 PM CET, Krzysztof Kozlowski wrote: >> On 09/12/2022 15:29, Luca Weiss wrote: >>> The code in ufs-qcom-ice.c needs the ICE reg to be named "ice". Add this >>> in the bindings so the existing dts can validate successfully. >>> >>> Also sm8450 is using ICE since commit 276ee34a40c1 ("arm64: dts: qcom: >>> sm8450: add Inline Crypto Engine registers and clock") so move the >>> compatible to the correct if. >>> >>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> >>> --- >>> (no cover subject) >>> >>> The only remaining validation issues I see is the following on sc8280xp-crd.dtb >>> and sa8540p-ride.dtb: >>> >> >> Any plans on fixing the patch (after testing it) and resending? > > I wasn't quite sure how to fix the comments, but re-reading them this > comment from you is how you expect it to be in v2? The patch fails testing, so I meant this. > >> Just add it to top-level with minItems: 1 and per variant customize: >> 1. maxItems: 1 >> 2. minItems: 2 + required > Yes. Best regards, Krzysztof
On Wed Dec 28, 2022 at 12:58 PM CET, Krzysztof Kozlowski wrote: > On 28/12/2022 12:53, Luca Weiss wrote: > > Hi Krzysztof, > > > > On Wed Dec 28, 2022 at 12:50 PM CET, Krzysztof Kozlowski wrote: > >> On 09/12/2022 15:29, Luca Weiss wrote: > >>> The code in ufs-qcom-ice.c needs the ICE reg to be named "ice". Add this > >>> in the bindings so the existing dts can validate successfully. > >>> > >>> Also sm8450 is using ICE since commit 276ee34a40c1 ("arm64: dts: qcom: > >>> sm8450: add Inline Crypto Engine registers and clock") so move the > >>> compatible to the correct if. > >>> > >>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > >>> --- > >>> (no cover subject) > >>> > >>> The only remaining validation issues I see is the following on sc8280xp-crd.dtb > >>> and sa8540p-ride.dtb: > >>> > >> > >> Any plans on fixing the patch (after testing it) and resending? > > > > I wasn't quite sure how to fix the comments, but re-reading them this > > comment from you is how you expect it to be in v2? > > The patch fails testing, so I meant this. > > > > >> Just add it to top-level with minItems: 1 and per variant customize: > >> 1. maxItems: 1 > >> 2. minItems: 2 + required > > I tried a bit now but couldn't get it to work when using 'items' so that we have the "std" and "ice" names in there. Documentation/devicetree/bindings/ufs/qcom,ufs.yaml: allOf:2:then:properties:reg-names: 'oneOf' conditional failed, one must be fixed: [{'const': 'std'}, {'const': 'ice'}] is too long [{'const': 'std'}, {'const': 'ice'}] is too short False schema does not allow 2 1 was expected hint: "minItems" is only needed if less than the "items" list length from schema $id: http://devicetree.org/meta-schemas/items.yaml# Since I have 'minItems: 1' in top-level I seemingly cannot use 'items' in the 'if' neither alone nor with 'minItems' and/or 'maxItems', getting different errors when doing that. Can I just put 'reg-names: true' top-level and then specify either items for the ones that use ICE or for the others use the 'maxItems: 1'? Or am I supposed to ignore 'items' completely but driver expects 'ice' name so I'd rather include it. Regards Luca > > Yes. > > Best regards, > Krzysztof
On 28/12/2022 16:24, Luca Weiss wrote: > On Wed Dec 28, 2022 at 12:58 PM CET, Krzysztof Kozlowski wrote: >> On 28/12/2022 12:53, Luca Weiss wrote: >>> Hi Krzysztof, >>> >>> On Wed Dec 28, 2022 at 12:50 PM CET, Krzysztof Kozlowski wrote: >>>> On 09/12/2022 15:29, Luca Weiss wrote: >>>>> The code in ufs-qcom-ice.c needs the ICE reg to be named "ice". Add this >>>>> in the bindings so the existing dts can validate successfully. >>>>> >>>>> Also sm8450 is using ICE since commit 276ee34a40c1 ("arm64: dts: qcom: >>>>> sm8450: add Inline Crypto Engine registers and clock") so move the >>>>> compatible to the correct if. >>>>> >>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> >>>>> --- >>>>> (no cover subject) >>>>> >>>>> The only remaining validation issues I see is the following on sc8280xp-crd.dtb >>>>> and sa8540p-ride.dtb: >>>>> >>>> >>>> Any plans on fixing the patch (after testing it) and resending? >>> >>> I wasn't quite sure how to fix the comments, but re-reading them this >>> comment from you is how you expect it to be in v2? >> >> The patch fails testing, so I meant this. >> >>> >>>> Just add it to top-level with minItems: 1 and per variant customize: >>>> 1. maxItems: 1 >>>> 2. minItems: 2 + required >>> > > I tried a bit now but couldn't get it to work when using 'items' so that > we have the "std" and "ice" names in there. > > Documentation/devicetree/bindings/ufs/qcom,ufs.yaml: allOf:2:then:properties:reg-names: 'oneOf' conditional failed, one must be fixed: > [{'const': 'std'}, {'const': 'ice'}] is too long > [{'const': 'std'}, {'const': 'ice'}] is too short > False schema does not allow 2 > 1 was expected > hint: "minItems" is only needed if less than the "items" list length > from schema $id: http://devicetree.org/meta-schemas/items.yaml# > > Since I have 'minItems: 1' in top-level I seemingly cannot use 'items' > in the 'if' neither alone nor with 'minItems' and/or 'maxItems', getting > different errors when doing that. top-level cannot have only minItems:1. > > Can I just put 'reg-names: true' top-level and then specify either items > for the ones that use ICE or for the others use the 'maxItems: 1'? > > Or am I supposed to ignore 'items' completely but driver expects 'ice' > name so I'd rather include it. Use the syntax like: https://elixir.bootlin.com/linux/v5.19-rc6/source/Documentation/devicetree/bindings/clock/samsung,exynos7-clock.yaml#L57 Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml index f2d6298d926c..58a2fb2c83c3 100644 --- a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml +++ b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml @@ -102,7 +102,6 @@ allOf: - qcom,sc8280xp-ufshc - qcom,sm8250-ufshc - qcom,sm8350-ufshc - - qcom,sm8450-ufshc then: properties: clocks: @@ -130,6 +129,7 @@ allOf: - qcom,sdm845-ufshc - qcom,sm6350-ufshc - qcom,sm8150-ufshc + - qcom,sm8450-ufshc then: properties: clocks: @@ -149,6 +149,12 @@ allOf: reg: minItems: 2 maxItems: 2 + reg-names: + items: + - const: std + - const: ice + required: + - reg-names - if: properties:
The code in ufs-qcom-ice.c needs the ICE reg to be named "ice". Add this in the bindings so the existing dts can validate successfully. Also sm8450 is using ICE since commit 276ee34a40c1 ("arm64: dts: qcom: sm8450: add Inline Crypto Engine registers and clock") so move the compatible to the correct if. Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> --- (no cover subject) The only remaining validation issues I see is the following on sc8280xp-crd.dtb and sa8540p-ride.dtb: Unevaluated properties are not allowed ('required-opps', 'dma-coherent' were unexpected) Maybe someone who knows something about this can handle this? And the patch adding qcom,sm6115-ufshc hasn't been applied yet. --- Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) --- base-commit: f925116b24c0c42dc6d5ab5111c55fd7f74e8dc7 change-id: 20221209-dt-binding-ufs-2d7f64797ff2 Best regards,