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

  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,
  

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