diff mbox series

[v3,1/3] dt-bindings: qspi: cdns,qspi-nor: Add clocks for StarFive JH7110 SoC

Message ID 20230619083517.415597-2-william.qiu@starfivetech.com
State New
Headers show
Series Add initialization of clock for StarFive JH7110 SoC | expand

Commit Message

William Qiu June 19, 2023, 8:35 a.m. UTC 
The QSPI controller needs three clock items to work properly on StarFive
JH7110 SoC, so there is need to change the maxItems's value to 3. Other
platforms do not have this constraint.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../bindings/spi/cdns,qspi-nor.yaml           | 20 ++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Comments

Krzysztof Kozlowski June 19, 2023, 12:17 p.m. UTC | #1 
On 19/06/2023 10:35, William Qiu wrote:
> The QSPI controller needs three clock items to work properly on StarFive
> JH7110 SoC, so there is need to change the maxItems's value to 3. Other
> platforms do not have this constraint.
> 
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  .../bindings/spi/cdns,qspi-nor.yaml           | 20 ++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> index b310069762dd..1b83cbb9a086 100644
> --- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> +++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> @@ -26,6 +26,15 @@ allOf:
>              const: starfive,jh7110-qspi
>      then:
>        properties:
> +        clocks:
> +          maxItems: 3
> +
> +        clock-names:
> +          items:
> +            - const: ref
> +            - const: ahb
> +            - const: apb

You are duplicating top-level property. Define the items only in one
place. If this list is applicable to everything, then in top-level property.

> +
>          resets:
>            minItems: 2
>            maxItems: 3
> @@ -38,6 +47,9 @@ allOf:
>  
>      else:
>        properties:
> +        clocks:
> +          maxItems: 1

clock-names is missing. They must be in sync with clocks. What is the
first clock?

> +
>          resets:
>            maxItems: 2
>  
> @@ -70,7 +82,13 @@ properties:
>      maxItems: 1
>  
>    clocks:
> -    maxItems: 1
> +    maxItems: 3


You did not test it before sending. minItems is missing.

> +
> +  clock-names:
> +    items:
> +      - const: ref
> +      - const: ahb
> +      - const: apb


>  
>    cdns,fifo-depth:
>      description:

Best regards,
Krzysztof
William Qiu June 21, 2023, 6:16 a.m. UTC | #2 
On 2023/6/19 17:16, Rob Herring wrote:
> 
> On Mon, 19 Jun 2023 16:35:15 +0800, William Qiu wrote:
>> The QSPI controller needs three clock items to work properly on StarFive
>> JH7110 SoC, so there is need to change the maxItems's value to 3. Other
>> platforms do not have this constraint.
>> 
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>> ---
>>  .../bindings/spi/cdns,qspi-nor.yaml           | 20 ++++++++++++++++++-
>>  1 file changed, 19 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/spi/cdns,qspi-nor.example.dtb: spi@ff705000: clocks: [[4294967295]] is too short
> 	from schema $id: http://devicetree.org/schemas/spi/cdns,qspi-nor.yaml#
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230619083517.415597-2-william.qiu@starfivetech.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.
> 
It seems that my changes are not compatible with other dts files, I'll try to
fix it.

Thanks for reminding.

Best regards
William
William Qiu June 21, 2023, 6:45 a.m. UTC | #3 
On 2023/6/19 20:17, Krzysztof Kozlowski wrote:
> On 19/06/2023 10:35, William Qiu wrote:
>> The QSPI controller needs three clock items to work properly on StarFive
>> JH7110 SoC, so there is need to change the maxItems's value to 3. Other
>> platforms do not have this constraint.
>> 
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>> ---
>>  .../bindings/spi/cdns,qspi-nor.yaml           | 20 ++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
>> index b310069762dd..1b83cbb9a086 100644
>> --- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
>> +++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
>> @@ -26,6 +26,15 @@ allOf:
>>              const: starfive,jh7110-qspi
>>      then:
>>        properties:
>> +        clocks:
>> +          maxItems: 3
>> +
>> +        clock-names:
>> +          items:
>> +            - const: ref
>> +            - const: ahb
>> +            - const: apb
> 
> You are duplicating top-level property. Define the items only in one
> place. If this list is applicable to everything, then in top-level property.
> 
Only in JH7110 SoC need there clocks, other platforms do not have this constraint.
So I need to duplicating top-level property.
>> +
>>          resets:
>>            minItems: 2
>>            maxItems: 3
>> @@ -38,6 +47,9 @@ allOf:
>>  
>>      else:
>>        properties:
>> +        clocks:
>> +          maxItems: 1
> 
> clock-names is missing. They must be in sync with clocks. What is the
> first clock?
> 
But there are no clock-names before, should I add it?
>> +
>>          resets:
>>            maxItems: 2
>>  
>> @@ -70,7 +82,13 @@ properties:
>>      maxItems: 1
>>  
>>    clocks:
>> -    maxItems: 1
>> +    maxItems: 3
> 
> 
> You did not test it before sending. minItems is missing.
> 
I will add it.
As for other platforms, should I use enum to constraint the clocks?
>> +
>> +  clock-names:
>> +    items:
>> +      - const: ref
>> +      - const: ahb
>> +      - const: apb
> 
> 
>>  
>>    cdns,fifo-depth:
>>      description:
> 
> Best regards,
> Krzysztof
> 
Thanks for taking time to review this patches series.

Best regards,
William
Krzysztof Kozlowski June 21, 2023, 8:10 a.m. UTC | #4 
On 21/06/2023 08:45, William Qiu wrote:
> 
> 
> On 2023/6/19 20:17, Krzysztof Kozlowski wrote:
>> On 19/06/2023 10:35, William Qiu wrote:
>>> The QSPI controller needs three clock items to work properly on StarFive
>>> JH7110 SoC, so there is need to change the maxItems's value to 3. Other
>>> platforms do not have this constraint.
>>>
>>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>>> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
>>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>>> ---
>>>  .../bindings/spi/cdns,qspi-nor.yaml           | 20 ++++++++++++++++++-
>>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
>>> index b310069762dd..1b83cbb9a086 100644
>>> --- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
>>> +++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
>>> @@ -26,6 +26,15 @@ allOf:
>>>              const: starfive,jh7110-qspi
>>>      then:
>>>        properties:
>>> +        clocks:
>>> +          maxItems: 3
>>> +
>>> +        clock-names:
>>> +          items:
>>> +            - const: ref
>>> +            - const: ahb
>>> +            - const: apb
>>
>> You are duplicating top-level property. Define the items only in one
>> place. If this list is applicable to everything, then in top-level property.
>>
> Only in JH7110 SoC need there clocks, other platforms do not have this constraint.
> So I need to duplicating top-level property.

You don't need, why? Why writing something twice is an answer to "JH7110
needs 3 clocks"? It's not related.

What is the clock for all other variants?

>>> +
>>>          resets:
>>>            minItems: 2
>>>            maxItems: 3
>>> @@ -38,6 +47,9 @@ allOf:
>>>  
>>>      else:
>>>        properties:
>>> +        clocks:
>>> +          maxItems: 1
>>
>> clock-names is missing. They must be in sync with clocks. What is the
>> first clock?
>>
> But there are no clock-names before, should I add it?

Then let's just disallow it. Either you define it or you not allow it.

>>> +
>>>          resets:
>>>            maxItems: 2
>>>  
>>> @@ -70,7 +82,13 @@ properties:
>>>      maxItems: 1
>>>  
>>>    clocks:
>>> -    maxItems: 1
>>> +    maxItems: 3
>>
>>
>> You did not test it before sending. minItems is missing.
>>
> I will add it.
> As for other platforms, should I use enum to constraint the clocks?

What is the clock on other platforms?

Best regards,
Krzysztof
William Qiu June 27, 2023, 7:53 a.m. UTC | #5 
On 2023/6/21 16:10, Krzysztof Kozlowski wrote:
> On 21/06/2023 08:45, William Qiu wrote:
>> 
>> 
>> On 2023/6/19 20:17, Krzysztof Kozlowski wrote:
>>> On 19/06/2023 10:35, William Qiu wrote:
>>>> The QSPI controller needs three clock items to work properly on StarFive
>>>> JH7110 SoC, so there is need to change the maxItems's value to 3. Other
>>>> platforms do not have this constraint.
>>>>
>>>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>>>> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
>>>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>>>> ---
>>>>  .../bindings/spi/cdns,qspi-nor.yaml           | 20 ++++++++++++++++++-
>>>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
>>>> index b310069762dd..1b83cbb9a086 100644
>>>> --- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
>>>> +++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
>>>> @@ -26,6 +26,15 @@ allOf:
>>>>              const: starfive,jh7110-qspi
>>>>      then:
>>>>        properties:
>>>> +        clocks:
>>>> +          maxItems: 3
>>>> +
>>>> +        clock-names:
>>>> +          items:
>>>> +            - const: ref
>>>> +            - const: ahb
>>>> +            - const: apb
>>>
>>> You are duplicating top-level property. Define the items only in one
>>> place. If this list is applicable to everything, then in top-level property.
>>>
>> Only in JH7110 SoC need there clocks, other platforms do not have this constraint.
>> So I need to duplicating top-level property.
> 
> You don't need, why? Why writing something twice is an answer to "JH7110
> needs 3 clocks"? It's not related.
> 
> What is the clock for all other variants?
> 
I'll try to not duplicating top-level property.
>>>> +
>>>>          resets:
>>>>            minItems: 2
>>>>            maxItems: 3
>>>> @@ -38,6 +47,9 @@ allOf:
>>>>  
>>>>      else:
>>>>        properties:
>>>> +        clocks:
>>>> +          maxItems: 1
>>>
>>> clock-names is missing. They must be in sync with clocks. What is the
>>> first clock?
>>>
>> But there are no clock-names before, should I add it?
> 
> Then let's just disallow it. Either you define it or you not allow it.
> 
Fine, I'll keep it disallow.
>>>> +
>>>>          resets:
>>>>            maxItems: 2
>>>>  
>>>> @@ -70,7 +82,13 @@ properties:
>>>>      maxItems: 1
>>>>  
>>>>    clocks:
>>>> -    maxItems: 1
>>>> +    maxItems: 3
>>>
>>>
>>> You did not test it before sending. minItems is missing.
>>>
>> I will add it.
>> As for other platforms, should I use enum to constraint the clocks?
> 
> What is the clock on other platforms?
> 
Other platforms have only one clock.
> Best regards,
> Krzysztof
> 
Thanks for taking time to review this patch series and give usefull
suggestions.

Best Regards,
William
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
index b310069762dd..1b83cbb9a086 100644
--- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
+++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
@@ -26,6 +26,15 @@  allOf:
             const: starfive,jh7110-qspi
     then:
       properties:
+        clocks:
+          maxItems: 3
+
+        clock-names:
+          items:
+            - const: ref
+            - const: ahb
+            - const: apb
+
         resets:
           minItems: 2
           maxItems: 3
@@ -38,6 +47,9 @@  allOf:
 
     else:
       properties:
+        clocks:
+          maxItems: 1
+
         resets:
           maxItems: 2
 
@@ -70,7 +82,13 @@  properties:
     maxItems: 1
 
   clocks:
-    maxItems: 1
+    maxItems: 3
+
+  clock-names:
+    items:
+      - const: ref
+      - const: ahb
+      - const: apb
 
   cdns,fifo-depth:
     description: