[PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE

[PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE

[Luca Weiss]

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>

Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE

[Krzysztof Kozlowski]

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

Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE

[Luca Weiss]

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

Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE

[Krzysztof Kozlowski]

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

Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE

[Johan Hovold]

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

Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE

[Rob Herring]

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.

Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE

[Eric Biggers]

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

Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE

[Krzysztof Kozlowski]

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

Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE

[Krzysztof Kozlowski]

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

Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE

[Luca Weiss]

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

Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE

[Krzysztof Kozlowski]

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

Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE

[Luca Weiss]

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

Re: [PATCH] dt-bindings: ufs: qcom: Add reg-names property for ICE

[Krzysztof Kozlowski]

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