diff mbox series

[v2,1/2] dt-bindings: cdns,usb3: Add clock and reset

Message ID 20230510132816.108820-2-minda.chen@starfivetech.com
State New
Headers
Series Add clock and reset in cdns3 platform |

Commit Message

Minda Chen May 10, 2023, 1:28 p.m. UTC 
  To support generic clock and reset init in Cadence USBSS
controller, add clock and reset dts configuration.

Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
---
 .../devicetree/bindings/usb/cdns,usb3.yaml         | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Krzysztof Kozlowski May 11, 2023, 9:26 a.m. UTC | #1 
On 10/05/2023 15:28, Minda Chen wrote:
> To support generic clock and reset init in Cadence USBSS
> controller, add clock and reset dts configuration.
> 
> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> ---
>  .../devicetree/bindings/usb/cdns,usb3.yaml         | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
> index cae46c4982ad..623c6b34dee3 100644
> --- a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
> +++ b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
> @@ -42,6 +42,18 @@ properties:
>        - const: otg
>        - const: wakeup
>  
> +  clocks:
> +    minItems: 1
> +    maxItems: 8
> +    description:
> +      USB controller clocks.

You need to list the items. And why is it variable? Your clock choice in
the example is poor, I doubt it is real.

> +
> +  resets:
> +    minItems: 1
> +    maxItems: 8
> +    description:
> +      USB controller generic resets.

Here as well.

You had one clock last time, thus the review was - drop the names. Now
you changed it to 8 clocks... I don't understand.

Best regards,
Krzysztof
Roger Quadros May 11, 2023, 12:16 p.m. UTC | #2 
On 11/05/2023 12:26, Krzysztof Kozlowski wrote:
> On 10/05/2023 15:28, Minda Chen wrote:
>> To support generic clock and reset init in Cadence USBSS
>> controller, add clock and reset dts configuration.
>>
>> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
>> ---
>>  .../devicetree/bindings/usb/cdns,usb3.yaml         | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>> index cae46c4982ad..623c6b34dee3 100644
>> --- a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>> +++ b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>> @@ -42,6 +42,18 @@ properties:
>>        - const: otg
>>        - const: wakeup
>>  
>> +  clocks:
>> +    minItems: 1
>> +    maxItems: 8
>> +    description:
>> +      USB controller clocks.
> 
> You need to list the items. And why is it variable? Your clock choice in
> the example is poor, I doubt it is real.
> 
>> +
>> +  resets:
>> +    minItems: 1
>> +    maxItems: 8
>> +    description:
>> +      USB controller generic resets.
> 
> Here as well.
> 
> You had one clock last time, thus the review was - drop the names. Now
> you changed it to 8 clocks... I don't understand.
> 

Different platforms may have different number of clocks/resets or none.
So I don't think minItems/maxItems should be specified.
Krzysztof Kozlowski May 11, 2023, 2:49 p.m. UTC | #3 
On 11/05/2023 14:16, Roger Quadros wrote:
> 
> 
> On 11/05/2023 12:26, Krzysztof Kozlowski wrote:
>> On 10/05/2023 15:28, Minda Chen wrote:
>>> To support generic clock and reset init in Cadence USBSS
>>> controller, add clock and reset dts configuration.
>>>
>>> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
>>> ---
>>>  .../devicetree/bindings/usb/cdns,usb3.yaml         | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>>> index cae46c4982ad..623c6b34dee3 100644
>>> --- a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>>> @@ -42,6 +42,18 @@ properties:
>>>        - const: otg
>>>        - const: wakeup
>>>  
>>> +  clocks:
>>> +    minItems: 1
>>> +    maxItems: 8
>>> +    description:
>>> +      USB controller clocks.
>>
>> You need to list the items. And why is it variable? Your clock choice in
>> the example is poor, I doubt it is real.
>>
>>> +
>>> +  resets:
>>> +    minItems: 1
>>> +    maxItems: 8
>>> +    description:
>>> +      USB controller generic resets.
>>
>> Here as well.
>>
>> You had one clock last time, thus the review was - drop the names. Now
>> you changed it to 8 clocks... I don't understand.
>>
> 
> Different platforms may have different number of clocks/resets or none.
> So I don't think minItems/maxItems should be specified.

Yeah, but we want the clocks to be specific per platform. Not anything
anywhere.

Best regards,
Krzysztof
Minda Chen May 12, 2023, 10:22 a.m. UTC | #4 
On 2023/5/11 22:49, Krzysztof Kozlowski wrote:
> On 11/05/2023 14:16, Roger Quadros wrote:
>> 
>> 
>> On 11/05/2023 12:26, Krzysztof Kozlowski wrote:
>>> On 10/05/2023 15:28, Minda Chen wrote:
>>>> To support generic clock and reset init in Cadence USBSS
>>>> controller, add clock and reset dts configuration.
>>>>
>>>> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
>>>> ---
>>>>  .../devicetree/bindings/usb/cdns,usb3.yaml         | 14 ++++++++++++++
>>>>  1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>>>> index cae46c4982ad..623c6b34dee3 100644
>>>> --- a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>>>> +++ b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>>>> @@ -42,6 +42,18 @@ properties:
>>>>        - const: otg
>>>>        - const: wakeup
>>>>  
>>>> +  clocks:
>>>> +    minItems: 1
>>>> +    maxItems: 8
>>>> +    description:
>>>> +      USB controller clocks.
>>>
>>> You need to list the items. And why is it variable? Your clock choice in
>>> the example is poor, I doubt it is real.
>>>
>>>> +
>>>> +  resets:
>>>> +    minItems: 1
>>>> +    maxItems: 8
>>>> +    description:
>>>> +      USB controller generic resets.
>>>
>>> Here as well.
>>>
>>> You had one clock last time, thus the review was - drop the names. Now
>>> you changed it to 8 clocks... I don't understand.
>>>
>> 
>> Different platforms may have different number of clocks/resets or none.
>> So I don't think minItems/maxItems should be specified.
> 
> Yeah, but we want the clocks to be specific per platform. Not anything
> anywhere.
> 
> Best regards,
> Krzysztof
> 

I can change like these. Are these changes can be approved?
lpm , bus clock and "pwrup" reset can be specific cases. (The changes are from snps,dwc3.yaml.)

  clocks:
    description:
      In general the core supports two types of clocks. bus is a SoC Bus
      Clock(AHB/AXI/APB). lpm is a link power management clock. But particular
      cases may differ from that having less or more clock sources with
      another names.

  clock-names:
    contains:
      anyOf:
        - enum: [bus, lpm]
        - true

  resets:
    description:
      In general the core supports controller power-up reset. Also clock and
      other resets can be added. Particular cases may differ from that having
      less or more resets with another names.

  reset-names:
    contains:
      anyOf:
        - const: pwrup
        - true
Roger Quadros May 12, 2023, 11:08 a.m. UTC | #5 
On 11/05/2023 17:49, Krzysztof Kozlowski wrote:
> On 11/05/2023 14:16, Roger Quadros wrote:
>>
>>
>> On 11/05/2023 12:26, Krzysztof Kozlowski wrote:
>>> On 10/05/2023 15:28, Minda Chen wrote:
>>>> To support generic clock and reset init in Cadence USBSS
>>>> controller, add clock and reset dts configuration.
>>>>
>>>> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
>>>> ---
>>>>  .../devicetree/bindings/usb/cdns,usb3.yaml         | 14 ++++++++++++++
>>>>  1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>>>> index cae46c4982ad..623c6b34dee3 100644
>>>> --- a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>>>> +++ b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>>>> @@ -42,6 +42,18 @@ properties:
>>>>        - const: otg
>>>>        - const: wakeup
>>>>  
>>>> +  clocks:
>>>> +    minItems: 1
>>>> +    maxItems: 8
>>>> +    description:
>>>> +      USB controller clocks.
>>>
>>> You need to list the items. And why is it variable? Your clock choice in
>>> the example is poor, I doubt it is real.
>>>
>>>> +
>>>> +  resets:
>>>> +    minItems: 1
>>>> +    maxItems: 8
>>>> +    description:
>>>> +      USB controller generic resets.
>>>
>>> Here as well.
>>>
>>> You had one clock last time, thus the review was - drop the names. Now
>>> you changed it to 8 clocks... I don't understand.
>>>
>>
>> Different platforms may have different number of clocks/resets or none.
>> So I don't think minItems/maxItems should be specified.
> 
> Yeah, but we want the clocks to be specific per platform. Not anything
> anywhere.
> 

Agreed. So we don't specify min/maxItems at top level but use conditional
constraints per platform?
Which means we will need to add platform specific compatibles as well.
Krzysztof Kozlowski May 13, 2023, 6:42 p.m. UTC | #6 
On 12/05/2023 12:22, Minda Chen wrote:
> 
> 
> On 2023/5/11 22:49, Krzysztof Kozlowski wrote:
>> On 11/05/2023 14:16, Roger Quadros wrote:
>>>
>>>
>>> On 11/05/2023 12:26, Krzysztof Kozlowski wrote:
>>>> On 10/05/2023 15:28, Minda Chen wrote:
>>>>> To support generic clock and reset init in Cadence USBSS
>>>>> controller, add clock and reset dts configuration.
>>>>>
>>>>> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
>>>>> ---
>>>>>  .../devicetree/bindings/usb/cdns,usb3.yaml         | 14 ++++++++++++++
>>>>>  1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>>>>> index cae46c4982ad..623c6b34dee3 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>>>>> +++ b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>>>>> @@ -42,6 +42,18 @@ properties:
>>>>>        - const: otg
>>>>>        - const: wakeup
>>>>>  
>>>>> +  clocks:
>>>>> +    minItems: 1
>>>>> +    maxItems: 8
>>>>> +    description:
>>>>> +      USB controller clocks.
>>>>
>>>> You need to list the items. And why is it variable? Your clock choice in
>>>> the example is poor, I doubt it is real.
>>>>
>>>>> +
>>>>> +  resets:
>>>>> +    minItems: 1
>>>>> +    maxItems: 8
>>>>> +    description:
>>>>> +      USB controller generic resets.
>>>>
>>>> Here as well.
>>>>
>>>> You had one clock last time, thus the review was - drop the names. Now
>>>> you changed it to 8 clocks... I don't understand.
>>>>
>>>
>>> Different platforms may have different number of clocks/resets or none.
>>> So I don't think minItems/maxItems should be specified.
>>
>> Yeah, but we want the clocks to be specific per platform. Not anything
>> anywhere.
>>
>> Best regards,
>> Krzysztof
>>
> 
> I can change like these. Are these changes can be approved?
> lpm , bus clock and "pwrup" reset can be specific cases. (The changes are from snps,dwc3.yaml.)
> 
>   clocks:
>     description:
>       In general the core supports two types of clocks. bus is a SoC Bus
>       Clock(AHB/AXI/APB). lpm is a link power management clock. But particular
>       cases may differ from that having less or more clock sources with
>       another names.
> 
>   clock-names:
>     contains:
>       anyOf:
>         - enum: [bus, lpm]
>         - true
> 

No because this does not solve my concern. You allow here anything,
which is not desired. The device bindings should specify what clocks
(and resets) you have here. Order is also fixed (with exceptions).

Now, if this is generic IP block used by different SoC vendors and it
has different clocks in different implementations, it means one
compatible for all of them is not enough anymore.

Best regards,
Krzysztof
Krzysztof Kozlowski May 13, 2023, 6:43 p.m. UTC | #7 
On 12/05/2023 13:08, Roger Quadros wrote:
> 
> 
> On 11/05/2023 17:49, Krzysztof Kozlowski wrote:
>> On 11/05/2023 14:16, Roger Quadros wrote:
>>>
>>>
>>> On 11/05/2023 12:26, Krzysztof Kozlowski wrote:
>>>> On 10/05/2023 15:28, Minda Chen wrote:
>>>>> To support generic clock and reset init in Cadence USBSS
>>>>> controller, add clock and reset dts configuration.
>>>>>
>>>>> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
>>>>> ---
>>>>>  .../devicetree/bindings/usb/cdns,usb3.yaml         | 14 ++++++++++++++
>>>>>  1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>>>>> index cae46c4982ad..623c6b34dee3 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>>>>> +++ b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
>>>>> @@ -42,6 +42,18 @@ properties:
>>>>>        - const: otg
>>>>>        - const: wakeup
>>>>>  
>>>>> +  clocks:
>>>>> +    minItems: 1
>>>>> +    maxItems: 8
>>>>> +    description:
>>>>> +      USB controller clocks.
>>>>
>>>> You need to list the items. And why is it variable? Your clock choice in
>>>> the example is poor, I doubt it is real.
>>>>
>>>>> +
>>>>> +  resets:
>>>>> +    minItems: 1
>>>>> +    maxItems: 8
>>>>> +    description:
>>>>> +      USB controller generic resets.
>>>>
>>>> Here as well.
>>>>
>>>> You had one clock last time, thus the review was - drop the names. Now
>>>> you changed it to 8 clocks... I don't understand.
>>>>
>>>
>>> Different platforms may have different number of clocks/resets or none.
>>> So I don't think minItems/maxItems should be specified.
>>
>> Yeah, but we want the clocks to be specific per platform. Not anything
>> anywhere.
>>
> 
> Agreed. So we don't specify min/maxItems at top level but use conditional
> constraints per platform?
> Which means we will need to add platform specific compatibles as well.

Yes, exactly. This can be done here in this binding or through some
re-usable common part and then multiple bindings using it and customizing.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
index cae46c4982ad..623c6b34dee3 100644
--- a/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
+++ b/Documentation/devicetree/bindings/usb/cdns,usb3.yaml
@@ -42,6 +42,18 @@  properties:
       - const: otg
       - const: wakeup
 
+  clocks:
+    minItems: 1
+    maxItems: 8
+    description:
+      USB controller clocks.
+
+  resets:
+    minItems: 1
+    maxItems: 8
+    description:
+      USB controller generic resets.
+
   dr_mode:
     enum: [host, otg, peripheral]
 
@@ -98,5 +110,7 @@  examples:
             interrupt-names = "host", "peripheral", "otg";
             maximum-speed = "super-speed";
             dr_mode = "otg";
+            clocks = <&clk 1>, <&clk 2>, <&clk 3>;
+            resets = <&rst 1>, <&rst 2>, <&rst 3>;
         };
     };