[PATCH v2 0/2] Add clock and reset in cdns3 platform

[PATCH v2 0/2] Add clock and reset in cdns3 platform

[Minda Chen]

The patchset is add generic clock and reset init codes in
Cadence USBSS controller.

This patchset is base on v6.4-rc1.

patch 1 is add clock and reset dts in cdns3 dt-binding doc.
patch 2 is cdns3 platform codes changes.

previous commits
v1: https://patchwork.kernel.org/project/linux-usb/cover/20230502081805.112149-1-minda.chen@starfivetech.com/

changes:
v2:
    patch 1
      - add maxItem in clocks and resets.
      - add clocks and reset in example.
    patch 2
      - suspend/resume just disable/enable clocks.

Minda Chen (2):
  dt-bindings: cdns,usb3: Add clock and reset
  usb: cdns3: cdns3-plat: Add clk and reset init

 .../devicetree/bindings/usb/cdns,usb3.yaml    | 14 +++++
 drivers/usb/cdns3/cdns3-plat.c                | 58 +++++++++++++++++++
 drivers/usb/cdns3/core.h                      |  3 +
 3 files changed, 75 insertions(+)


base-commit: ac9a78681b921877518763ba0e89202254349d1b
-- 
2.17.1

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

[Minda Chen]

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.
+
+  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>;
         };
     };
-- 
2.17.1

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

[Krzysztof Kozlowski]

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

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

[Roger Quadros]

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.

-- 
cheers,
-roger

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

[Krzysztof Kozlowski]

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

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

[Minda Chen]

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

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

[Krzysztof Kozlowski]

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

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

[Roger Quadros]

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.

-- 
cheers,
-roger

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

[Krzysztof Kozlowski]

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

[PATCH v2 2/2] usb: cdns3: cdns3-plat: Add clk and reset init

[Minda Chen]

Add gereric clk and reset init codes to Cadence USBSS
controller. The codes has been tested by starfive vf2
board.

Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
---
 drivers/usb/cdns3/cdns3-plat.c | 58 ++++++++++++++++++++++++++++++++++
 drivers/usb/cdns3/core.h       |  3 ++
 2 files changed, 61 insertions(+)

diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
index 2bc5d094548b..30da808e0a2f 100644
--- a/drivers/usb/cdns3/cdns3-plat.c
+++ b/drivers/usb/cdns3/cdns3-plat.c
@@ -12,11 +12,13 @@
  *         Roger Quadros <rogerq@ti.com>
  */
 
+#include <linux/clk.h>
 #include <linux/module.h>
 #include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 
 #include "core.h"
 #include "gadget-export.h"
@@ -43,6 +45,40 @@ static void set_phy_power_off(struct cdns *cdns)
 	phy_power_off(cdns->usb2_phy);
 }
 
+static int cdns3_clk_rst_init(struct cdns *cdns, bool clk_only)
+{
+	int ret = 0;
+
+	if (cdns->num_clks) {
+		ret = clk_bulk_prepare_enable(cdns->num_clks, cdns->clks);
+		if (ret)
+			return ret;
+	}
+
+	if (clk_only)
+		return ret;
+
+	ret = reset_control_deassert(cdns->resets);
+	if (ret)
+		goto err_clk_init;
+
+	return ret;
+
+err_clk_init:
+	if (cdns->num_clks)
+		clk_bulk_disable_unprepare(cdns->num_clks, cdns->clks);
+	return ret;
+}
+
+static void cdns3_clk_rst_deinit(struct cdns *cdns, bool clk_only)
+{
+	if (!clk_only)
+		reset_control_assert(cdns->resets);
+
+	if (cdns->num_clks)
+		clk_bulk_disable_unprepare(cdns->num_clks, cdns->clks);
+}
+
 /**
  * cdns3_plat_probe - probe for cdns3 core device
  * @pdev: Pointer to cdns3 core platform device
@@ -116,6 +152,16 @@ static int cdns3_plat_probe(struct platform_device *pdev)
 		cdns->wakeup_irq = 0x0;
 	}
 
+	ret = devm_clk_bulk_get_all(dev, &cdns->clks);
+	if (ret < 0)
+		return ret;
+
+	cdns->num_clks = ret;
+
+	cdns->resets = devm_reset_control_array_get_optional_exclusive(dev);
+	if (IS_ERR(cdns->resets))
+		return PTR_ERR(cdns->resets);
+
 	cdns->usb2_phy = devm_phy_optional_get(dev, "cdns3,usb2-phy");
 	if (IS_ERR(cdns->usb2_phy))
 		return PTR_ERR(cdns->usb2_phy);
@@ -128,6 +174,10 @@ static int cdns3_plat_probe(struct platform_device *pdev)
 	if (IS_ERR(cdns->usb3_phy))
 		return PTR_ERR(cdns->usb3_phy);
 
+	ret = cdns3_clk_rst_init(cdns, false);
+	if (ret)
+		return ret;
+
 	ret = phy_init(cdns->usb3_phy);
 	if (ret)
 		goto err_phy3_init;
@@ -165,6 +215,7 @@ static int cdns3_plat_probe(struct platform_device *pdev)
 	phy_exit(cdns->usb3_phy);
 err_phy3_init:
 	phy_exit(cdns->usb2_phy);
+	cdns3_clk_rst_deinit(cdns, false);
 
 	return ret;
 }
@@ -187,6 +238,8 @@ static int cdns3_plat_remove(struct platform_device *pdev)
 	set_phy_power_off(cdns);
 	phy_exit(cdns->usb2_phy);
 	phy_exit(cdns->usb3_phy);
+	cdns3_clk_rst_deinit(cdns, false);
+
 	return 0;
 }
 
@@ -220,6 +273,8 @@ static int cdns3_controller_suspend(struct device *dev, pm_message_t msg)
 
 	cdns3_set_platform_suspend(cdns->dev, true, wakeup);
 	set_phy_power_off(cdns);
+	if (!PMSG_IS_AUTO(msg))
+		cdns3_clk_rst_deinit(cdns, true);
 	spin_lock_irqsave(&cdns->lock, flags);
 	cdns->in_lpm = true;
 	spin_unlock_irqrestore(&cdns->lock, flags);
@@ -237,6 +292,9 @@ static int cdns3_controller_resume(struct device *dev, pm_message_t msg)
 	if (!cdns->in_lpm)
 		return 0;
 
+	if (!PMSG_IS_AUTO(msg))
+		cdns3_clk_rst_init(cdns, true);
+
 	if (cdns_power_is_lost(cdns)) {
 		phy_exit(cdns->usb2_phy);
 		ret = phy_init(cdns->usb2_phy);
diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
index 2d332a788871..b894768ee485 100644
--- a/drivers/usb/cdns3/core.h
+++ b/drivers/usb/cdns3/core.h
@@ -111,6 +111,9 @@ struct cdns {
 	struct mutex			mutex;
 	enum usb_dr_mode		dr_mode;
 	struct usb_role_switch		*role_sw;
+	struct reset_control *resets;
+	struct clk_bulk_data *clks;
+	int num_clks;
 	bool				in_lpm;
 	bool				wakeup_pending;
 	struct cdns3_platform_data	*pdata;
-- 
2.17.1