* [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE
@ 2022-12-09 14:29 Luca Weiss
2022-12-09 15:05 ` Krzysztof Kozlowski
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Luca Weiss @ 2022-12-09 14:29 UTC (permalink / raw)
To: Luca Weiss, ~postmarketos/upstreaming, phone-devel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Alim Akhtar, Avri Altman,
Bart Van Assche, Rob Herring, Krzysztof Kozlowski
Cc: linux-arm-msm, linux-scsi, devicetree, linux-kernel,
~postmarketos/upstreaming, phone-devel
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:
+ items:
+ - const: std
+ - const: ice
+ required:
+ - reg-names
- if:
properties:
---
base-commit: f925116b24c0c42dc6d5ab5111c55fd7f74e8dc7
change-id: 20221209-dt-binding-ufs-2d7f64797ff2
Best regards,
--
Luca Weiss <luca.weiss@fairphone.com>
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE
2022-12-09 14:29 [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE Luca Weiss
@ 2022-12-09 15:05 ` Krzysztof Kozlowski
2022-12-09 15:11 ` Luca Weiss
2022-12-09 15:43 ` Johan Hovold
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Krzysztof Kozlowski @ 2022-12-09 15:05 UTC (permalink / raw)
To: Luca Weiss, ~postmarketos/upstreaming, phone-devel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Alim Akhtar, Avri Altman,
Bart Van Assche, Rob Herring, Krzysztof Kozlowski
Cc: linux-arm-msm, linux-scsi, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE
2022-12-09 15:05 ` Krzysztof Kozlowski
@ 2022-12-09 15:11 ` Luca Weiss
2022-12-09 15:19 ` Krzysztof Kozlowski
0 siblings, 1 reply; 13+ messages in thread
From: Luca Weiss @ 2022-12-09 15:11 UTC (permalink / raw)
To: Krzysztof Kozlowski, ~postmarketos/upstreaming, phone-devel,
Andy Gross, Bjorn Andersson, Konrad Dybcio, Alim Akhtar,
Avri Altman, Bart Van Assche, Rob Herring, Krzysztof Kozlowski
Cc: linux-arm-msm, linux-scsi, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE
2022-12-09 15:11 ` Luca Weiss
@ 2022-12-09 15:19 ` Krzysztof Kozlowski
2022-12-09 19:35 ` Eric Biggers
0 siblings, 1 reply; 13+ messages in thread
From: Krzysztof Kozlowski @ 2022-12-09 15:19 UTC (permalink / raw)
To: Luca Weiss, ~postmarketos/upstreaming, phone-devel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Alim Akhtar, Avri Altman,
Bart Van Assche, Rob Herring, Krzysztof Kozlowski, Eric Biggers
Cc: linux-arm-msm, linux-scsi, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE
2022-12-09 14:29 [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE Luca Weiss
2022-12-09 15:05 ` Krzysztof Kozlowski
@ 2022-12-09 15:43 ` Johan Hovold
2022-12-09 17:41 ` Rob Herring
2022-12-28 11:50 ` Krzysztof Kozlowski
3 siblings, 0 replies; 13+ messages in thread
From: Johan Hovold @ 2022-12-09 15:43 UTC (permalink / raw)
To: Luca Weiss
Cc: ~postmarketos/upstreaming, phone-devel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Alim Akhtar, Avri Altman,
Bart Van Assche, Rob Herring, Krzysztof Kozlowski, linux-arm-msm,
linux-scsi, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE
2022-12-09 14:29 [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE Luca Weiss
2022-12-09 15:05 ` Krzysztof Kozlowski
2022-12-09 15:43 ` Johan Hovold
@ 2022-12-09 17:41 ` Rob Herring
2022-12-28 11:50 ` Krzysztof Kozlowski
3 siblings, 0 replies; 13+ messages in thread
From: Rob Herring @ 2022-12-09 17:41 UTC (permalink / raw)
To: Luca Weiss
Cc: linux-scsi, Bart Van Assche, Alim Akhtar, Krzysztof Kozlowski,
linux-arm-msm, phone-devel, Bjorn Andersson, Rob Herring,
linux-kernel, Avri Altman, ~postmarketos/upstreaming,
Konrad Dybcio, Andy Gross, devicetree
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE
2022-12-09 15:19 ` Krzysztof Kozlowski
@ 2022-12-09 19:35 ` Eric Biggers
2022-12-10 11:01 ` Krzysztof Kozlowski
0 siblings, 1 reply; 13+ messages in thread
From: Eric Biggers @ 2022-12-09 19:35 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Luca Weiss, ~postmarketos/upstreaming, phone-devel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Alim Akhtar, Avri Altman,
Bart Van Assche, Rob Herring, Krzysztof Kozlowski, linux-arm-msm,
linux-scsi, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE
2022-12-09 19:35 ` Eric Biggers
@ 2022-12-10 11:01 ` Krzysztof Kozlowski
0 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Kozlowski @ 2022-12-10 11:01 UTC (permalink / raw)
To: Eric Biggers
Cc: Luca Weiss, ~postmarketos/upstreaming, phone-devel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Alim Akhtar, Avri Altman,
Bart Van Assche, Rob Herring, Krzysztof Kozlowski, linux-arm-msm,
linux-scsi, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE
2022-12-09 14:29 [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE Luca Weiss
` (2 preceding siblings ...)
2022-12-09 17:41 ` Rob Herring
@ 2022-12-28 11:50 ` Krzysztof Kozlowski
2022-12-28 11:53 ` Luca Weiss
3 siblings, 1 reply; 13+ messages in thread
From: Krzysztof Kozlowski @ 2022-12-28 11:50 UTC (permalink / raw)
To: Luca Weiss, ~postmarketos/upstreaming, phone-devel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Alim Akhtar, Avri Altman,
Bart Van Assche, Rob Herring, Krzysztof Kozlowski
Cc: linux-arm-msm, linux-scsi, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE
2022-12-28 11:50 ` Krzysztof Kozlowski
@ 2022-12-28 11:53 ` Luca Weiss
2022-12-28 11:58 ` Krzysztof Kozlowski
0 siblings, 1 reply; 13+ messages in thread
From: Luca Weiss @ 2022-12-28 11:53 UTC (permalink / raw)
To: Krzysztof Kozlowski, ~postmarketos/upstreaming, phone-devel,
Andy Gross, Bjorn Andersson, Konrad Dybcio, Alim Akhtar,
Avri Altman, Bart Van Assche, Rob Herring, Krzysztof Kozlowski
Cc: linux-arm-msm, linux-scsi, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE
2022-12-28 11:53 ` Luca Weiss
@ 2022-12-28 11:58 ` Krzysztof Kozlowski
2022-12-28 15:24 ` Luca Weiss
0 siblings, 1 reply; 13+ messages in thread
From: Krzysztof Kozlowski @ 2022-12-28 11:58 UTC (permalink / raw)
To: Luca Weiss, ~postmarketos/upstreaming, phone-devel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Alim Akhtar, Avri Altman,
Bart Van Assche, Rob Herring, Krzysztof Kozlowski
Cc: linux-arm-msm, linux-scsi, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE
2022-12-28 11:58 ` Krzysztof Kozlowski
@ 2022-12-28 15:24 ` Luca Weiss
2022-12-29 8:06 ` Krzysztof Kozlowski
0 siblings, 1 reply; 13+ messages in thread
From: Luca Weiss @ 2022-12-28 15:24 UTC (permalink / raw)
To: Krzysztof Kozlowski, ~postmarketos/upstreaming, phone-devel,
Andy Gross, Bjorn Andersson, Konrad Dybcio, Alim Akhtar,
Avri Altman, Bart Van Assche, Rob Herring, Krzysztof Kozlowski
Cc: linux-arm-msm, linux-scsi, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE
2022-12-28 15:24 ` Luca Weiss
@ 2022-12-29 8:06 ` Krzysztof Kozlowski
0 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Kozlowski @ 2022-12-29 8:06 UTC (permalink / raw)
To: Luca Weiss, ~postmarketos/upstreaming, phone-devel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Alim Akhtar, Avri Altman,
Bart Van Assche, Rob Herring, Krzysztof Kozlowski
Cc: linux-arm-msm, linux-scsi, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-12-29 8:06 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-09 14:29 [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE Luca Weiss
2022-12-09 15:05 ` Krzysztof Kozlowski
2022-12-09 15:11 ` Luca Weiss
2022-12-09 15:19 ` Krzysztof Kozlowski
2022-12-09 19:35 ` Eric Biggers
2022-12-10 11:01 ` Krzysztof Kozlowski
2022-12-09 15:43 ` Johan Hovold
2022-12-09 17:41 ` Rob Herring
2022-12-28 11:50 ` Krzysztof Kozlowski
2022-12-28 11:53 ` Luca Weiss
2022-12-28 11:58 ` Krzysztof Kozlowski
2022-12-28 15:24 ` Luca Weiss
2022-12-29 8:06 ` Krzysztof Kozlowski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.