[PATCH] usb: dwc2: Skip clock gating on Samsung SoCs

[PATCH] usb: dwc2: Skip clock gating on Samsung SoCs

[Marek Szyprowski]

Commit 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr
function.") changed the way the driver handles power down modes in a such
way that it uses clock gating when no other power down mode is available.

This however doesn't work well on the DWC2 implementation used on the
Samsung SoCs. When a clock gating is enabled, system hangs. It looks that
the proper clock gating requires some additional glue code in the shared
USB2 PHY and/or Samsung glue code for the DWC2. To restore driver
operation on the Samsung SoCs simply skip enabling clock gating mode
until one finds what is really needed to make it working reliably.

Fixes: 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/usb/dwc2/core.h      | 4 ++++
 drivers/usb/dwc2/core_intr.c | 3 ++-
 drivers/usb/dwc2/hcd.c       | 6 ++++--
 drivers/usb/dwc2/params.c    | 1 +
 4 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index ab6b815e0089..483de2bbfaab 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -383,6 +383,9 @@ enum dwc2_ep0_state {
  *			0 - No (default)
  *			1 - Partial power down
  *			2 - Hibernation
+ * @no_clock_gating:	Specifies whether to avoid clock gating feature.
+ *			0 - No (use clock gating)
+ *			1 - Yes (avoid it)
  * @lpm:		Enable LPM support.
  *			0 - No
  *			1 - Yes
@@ -480,6 +483,7 @@ struct dwc2_core_params {
 #define DWC2_POWER_DOWN_PARAM_NONE		0
 #define DWC2_POWER_DOWN_PARAM_PARTIAL		1
 #define DWC2_POWER_DOWN_PARAM_HIBERNATION	2
+	bool no_clock_gating;
 
 	bool lpm;
 	bool lpm_clock_gating;
diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index a5ab03808da6..a5c52b237e72 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -556,7 +556,8 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
 				 * If neither hibernation nor partial power down are supported,
 				 * clock gating is used to save power.
 				 */
-				dwc2_gadget_enter_clock_gating(hsotg);
+				if (!hsotg->params.no_clock_gating)
+					dwc2_gadget_enter_clock_gating(hsotg);
 			}
 
 			/*
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 035d4911a3c3..2a7828971d05 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -3338,7 +3338,8 @@ int dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
 		 * If not hibernation nor partial power down are supported,
 		 * clock gating is used to save power.
 		 */
-		dwc2_host_enter_clock_gating(hsotg);
+		if (!hsotg->params.no_clock_gating)
+			dwc2_host_enter_clock_gating(hsotg);
 		break;
 	}
 
@@ -4402,7 +4403,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
 		 * If not hibernation nor partial power down are supported,
 		 * clock gating is used to save power.
 		 */
-		dwc2_host_enter_clock_gating(hsotg);
+		if (!hsotg->params.no_clock_gating)
+			dwc2_host_enter_clock_gating(hsotg);
 
 		/* After entering suspend, hardware is not accessible */
 		clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 67c5eb140232..59e119345994 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -76,6 +76,7 @@ static void dwc2_set_s3c6400_params(struct dwc2_hsotg *hsotg)
 	struct dwc2_core_params *p = &hsotg->params;
 
 	p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
+	p->no_clock_gating = true;
 	p->phy_utmi_width = 8;
 }
 
-- 
2.17.1

Re: [PATCH] usb: dwc2: Skip clock gating on Samsung SoCs

[Krzysztof Kozlowski]

On 16/07/2021 07:01, Marek Szyprowski wrote:
> Commit 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr
> function.") changed the way the driver handles power down modes in a such
> way that it uses clock gating when no other power down mode is available.
> 
> This however doesn't work well on the DWC2 implementation used on the
> Samsung SoCs. When a clock gating is enabled, system hangs. It looks that
> the proper clock gating requires some additional glue code in the shared
> USB2 PHY and/or Samsung glue code for the DWC2. To restore driver
> operation on the Samsung SoCs simply skip enabling clock gating mode
> until one finds what is really needed to make it working reliably.
> 
> Fixes: 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/usb/dwc2/core.h      | 4 ++++
>  drivers/usb/dwc2/core_intr.c | 3 ++-
>  drivers/usb/dwc2/hcd.c       | 6 ++++--
>  drivers/usb/dwc2/params.c    | 1 +
>  4 files changed, 11 insertions(+), 3 deletions(-)
> 


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

Re: [PATCH] usb: dwc2: Skip clock gating on Samsung SoCs

[Minas Harutyunyan]

Hi Krzysztof,

On 7/16/2021 12:10 PM, Krzysztof Kozlowski wrote:
> On 16/07/2021 07:01, Marek Szyprowski wrote:
>> Commit 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr
>> function.") changed the way the driver handles power down modes in a such
>> way that it uses clock gating when no other power down mode is available.
>>
>> This however doesn't work well on the DWC2 implementation used on the
>> Samsung SoCs. When a clock gating is enabled, system hangs. It looks that
>> the proper clock gating requires some additional glue code in the shared
>> USB2 PHY and/or Samsung glue code for the DWC2. To restore driver
>> operation on the Samsung SoCs simply skip enabling clock gating mode
>> until one finds what is really needed to make it working reliably.
>>
>> Fixes: 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.")
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/usb/dwc2/core.h      | 4 ++++
>>   drivers/usb/dwc2/core_intr.c | 3 ++-
>>   drivers/usb/dwc2/hcd.c       | 6 ++++--
>>   drivers/usb/dwc2/params.c    | 1 +
>>   4 files changed, 11 insertions(+), 3 deletions(-)
>>
> 
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> 
What mean your "Acked-by" tag? Do you want to mention that this commit 
"Tested-by" or "Reviewed-by" by you?

Thanks,
Minas

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH] usb: dwc2: Skip clock gating on Samsung SoCs

[Krzysztof Kozlowski]

On 16/07/2021 11:07, Minas Harutyunyan wrote:
> Hi Krzysztof,
> 
> On 7/16/2021 12:10 PM, Krzysztof Kozlowski wrote:
>> On 16/07/2021 07:01, Marek Szyprowski wrote:
>>> Commit 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr
>>> function.") changed the way the driver handles power down modes in a such
>>> way that it uses clock gating when no other power down mode is available.
>>>
>>> This however doesn't work well on the DWC2 implementation used on the
>>> Samsung SoCs. When a clock gating is enabled, system hangs. It looks that
>>> the proper clock gating requires some additional glue code in the shared
>>> USB2 PHY and/or Samsung glue code for the DWC2. To restore driver
>>> operation on the Samsung SoCs simply skip enabling clock gating mode
>>> until one finds what is really needed to make it working reliably.
>>>
>>> Fixes: 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.")
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>>   drivers/usb/dwc2/core.h      | 4 ++++
>>>   drivers/usb/dwc2/core_intr.c | 3 ++-
>>>   drivers/usb/dwc2/hcd.c       | 6 ++++--
>>>   drivers/usb/dwc2/params.c    | 1 +
>>>   4 files changed, 11 insertions(+), 3 deletions(-)
>>>
>>
>>
>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>>
> What mean your "Acked-by" tag? Do you want to mention that this commit 
> "Tested-by" or "Reviewed-by" by you?

My "Acked-by" means exactly what Linux process defines:
https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst#L426

Acked-by is neither Tested-by nor Reviewed-by. For the definition
of these other tags, please also see link above.

Best regards,
Krzysztof

Re: [PATCH] usb: dwc2: Skip clock gating on Samsung SoCs

[Minas Harutyunyan]

Hi Marek,

On 7/16/2021 9:01 AM, Marek Szyprowski wrote:
> Commit 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr
> function.") changed the way the driver handles power down modes in a such
> way that it uses clock gating when no other power down mode is available.
> 
> This however doesn't work well on the DWC2 implementation used on the
> Samsung SoCs. When a clock gating is enabled, system hangs. It looks that
> the proper clock gating requires some additional glue code in the shared
> USB2 PHY and/or Samsung glue code for the DWC2. To restore driver
> operation on the Samsung SoCs simply skip enabling clock gating mode
> until one finds what is really needed to make it working reliably.
> 
> Fixes: 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>   drivers/usb/dwc2/core.h      | 4 ++++
>   drivers/usb/dwc2/core_intr.c | 3 ++-
>   drivers/usb/dwc2/hcd.c       | 6 ++++--
>   drivers/usb/dwc2/params.c    | 1 +
>   4 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index ab6b815e0089..483de2bbfaab 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -383,6 +383,9 @@ enum dwc2_ep0_state {
>    *			0 - No (default)
>    *			1 - Partial power down
>    *			2 - Hibernation
> + * @no_clock_gating:	Specifies whether to avoid clock gating feature.
> + *			0 - No (use clock gating)
> + *			1 - Yes (avoid it)
>    * @lpm:		Enable LPM support.
>    *			0 - No
>    *			1 - Yes
> @@ -480,6 +483,7 @@ struct dwc2_core_params {
>   #define DWC2_POWER_DOWN_PARAM_NONE		0
>   #define DWC2_POWER_DOWN_PARAM_PARTIAL		1
>   #define DWC2_POWER_DOWN_PARAM_HIBERNATION	2
> +	bool no_clock_gating;
>   
>   	bool lpm;
>   	bool lpm_clock_gating;
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index a5ab03808da6..a5c52b237e72 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -556,7 +556,8 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
>   				 * If neither hibernation nor partial power down are supported,
>   				 * clock gating is used to save power.
>   				 */
> -				dwc2_gadget_enter_clock_gating(hsotg);
> +				if (!hsotg->params.no_clock_gating)
> +					dwc2_gadget_enter_clock_gating(hsotg);
>   			}
>   
>   			/*
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 035d4911a3c3..2a7828971d05 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -3338,7 +3338,8 @@ int dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
>   		 * If not hibernation nor partial power down are supported,
>   		 * clock gating is used to save power.
>   		 */
> -		dwc2_host_enter_clock_gating(hsotg);
> +		if (!hsotg->params.no_clock_gating)
> +			dwc2_host_enter_clock_gating(hsotg);
>   		break;
>   	}
>   
> @@ -4402,7 +4403,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>   		 * If not hibernation nor partial power down are supported,
>   		 * clock gating is used to save power.
>   		 */
> -		dwc2_host_enter_clock_gating(hsotg);
> +		if (!hsotg->params.no_clock_gating)
> +			dwc2_host_enter_clock_gating(hsotg);
>   
>   		/* After entering suspend, hardware is not accessible */
>   		clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index 67c5eb140232..59e119345994 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -76,6 +76,7 @@ static void dwc2_set_s3c6400_params(struct dwc2_hsotg *hsotg)
>   	struct dwc2_core_params *p = &hsotg->params;
>   
>   	p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
> +	p->no_clock_gating = true;
>   	p->phy_utmi_width = 8;
>   }
>   
> 
In which mode host/device you see the issue?
What is your core version? Please provide GHWCFG1-4 registers values.
Is the issue seen because of programming PCGCTL.PCGCTL_GATEHCLK bit 
only? To check it, could you please comment this bit setting/resetting 
in clock gating functions:
dwc2_host_exit_clock_gating()
dwc2_host_enter_clock_gating()
dwc2_gadget_exit_clock_gating()
dwc2_gadget_enter_clock_gating()

Thanks,
Minas

Re: [PATCH] usb: dwc2: Skip clock gating on Samsung SoCs

[Marek Szyprowski]

Hi Minas,

On 16.07.2021 16:54, Minas Harutyunyan wrote:
> On 7/16/2021 9:01 AM, Marek Szyprowski wrote:
>> Commit 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr
>> function.") changed the way the driver handles power down modes in a such
>> way that it uses clock gating when no other power down mode is available.
>>
>> This however doesn't work well on the DWC2 implementation used on the
>> Samsung SoCs. When a clock gating is enabled, system hangs. It looks that
>> the proper clock gating requires some additional glue code in the shared
>> USB2 PHY and/or Samsung glue code for the DWC2. To restore driver
>> operation on the Samsung SoCs simply skip enabling clock gating mode
>> until one finds what is really needed to make it working reliably.
>>
>> Fixes: 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.")
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>    drivers/usb/dwc2/core.h      | 4 ++++
>>    drivers/usb/dwc2/core_intr.c | 3 ++-
>>    drivers/usb/dwc2/hcd.c       | 6 ++++--
>>    drivers/usb/dwc2/params.c    | 1 +
>>    4 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>> index ab6b815e0089..483de2bbfaab 100644
>> --- a/drivers/usb/dwc2/core.h
>> +++ b/drivers/usb/dwc2/core.h
>> @@ -383,6 +383,9 @@ enum dwc2_ep0_state {
>>     *			0 - No (default)
>>     *			1 - Partial power down
>>     *			2 - Hibernation
>> + * @no_clock_gating:	Specifies whether to avoid clock gating feature.
>> + *			0 - No (use clock gating)
>> + *			1 - Yes (avoid it)
>>     * @lpm:		Enable LPM support.
>>     *			0 - No
>>     *			1 - Yes
>> @@ -480,6 +483,7 @@ struct dwc2_core_params {
>>    #define DWC2_POWER_DOWN_PARAM_NONE		0
>>    #define DWC2_POWER_DOWN_PARAM_PARTIAL		1
>>    #define DWC2_POWER_DOWN_PARAM_HIBERNATION	2
>> +	bool no_clock_gating;
>>    
>>    	bool lpm;
>>    	bool lpm_clock_gating;
>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>> index a5ab03808da6..a5c52b237e72 100644
>> --- a/drivers/usb/dwc2/core_intr.c
>> +++ b/drivers/usb/dwc2/core_intr.c
>> @@ -556,7 +556,8 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
>>    				 * If neither hibernation nor partial power down are supported,
>>    				 * clock gating is used to save power.
>>    				 */
>> -				dwc2_gadget_enter_clock_gating(hsotg);
>> +				if (!hsotg->params.no_clock_gating)
>> +					dwc2_gadget_enter_clock_gating(hsotg);
>>    			}
>>    
>>    			/*
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 035d4911a3c3..2a7828971d05 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -3338,7 +3338,8 @@ int dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
>>    		 * If not hibernation nor partial power down are supported,
>>    		 * clock gating is used to save power.
>>    		 */
>> -		dwc2_host_enter_clock_gating(hsotg);
>> +		if (!hsotg->params.no_clock_gating)
>> +			dwc2_host_enter_clock_gating(hsotg);
>>    		break;
>>    	}
>>    
>> @@ -4402,7 +4403,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>>    		 * If not hibernation nor partial power down are supported,
>>    		 * clock gating is used to save power.
>>    		 */
>> -		dwc2_host_enter_clock_gating(hsotg);
>> +		if (!hsotg->params.no_clock_gating)
>> +			dwc2_host_enter_clock_gating(hsotg);
>>    
>>    		/* After entering suspend, hardware is not accessible */
>>    		clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
>> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
>> index 67c5eb140232..59e119345994 100644
>> --- a/drivers/usb/dwc2/params.c
>> +++ b/drivers/usb/dwc2/params.c
>> @@ -76,6 +76,7 @@ static void dwc2_set_s3c6400_params(struct dwc2_hsotg *hsotg)
>>    	struct dwc2_core_params *p = &hsotg->params;
>>    
>>    	p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
>> +	p->no_clock_gating = true;
>>    	p->phy_utmi_width = 8;
>>    }
>>    
>>
> In which mode host/device you see the issue?
I do my test in my device mode.
> What is your core version? Please provide GHWCFG1-4 registers values.

This is a result of dwc2_dump_global_registers() added just after 
dwc2_lowlevel_hw_enable() in dwc2_driver_probe():

dwc2 12480000.hsotg: Core Global Registers

dwc2 12480000.hsotg: GOTGCTL      @0xE0260000 : 0x000D0000
dwc2 12480000.hsotg: GOTGINT      @0xE0260004 : 0x00000000
dwc2 12480000.hsotg: GAHBCFG      @0xE0260008 : 0x00000027
dwc2 12480000.hsotg: GUSBCFG      @0xE026000C : 0x0000540F
dwc2 12480000.hsotg: GRSTCTL      @0xE0260010 : 0x80000040
dwc2 12480000.hsotg: GINTSTS      @0xE0260014 : 0x54008428
dwc2 12480000.hsotg: GINTMSK      @0xE0260018 : 0x800C3800
dwc2 12480000.hsotg: GRXSTSR      @0xE026001C : 0x616EC77D
dwc2 12480000.hsotg: GRXFSIZ      @0xE0260024 : 0x00000400
dwc2 12480000.hsotg: GNPTXFSIZ    @0xE0260028 : 0x04000400
dwc2 12480000.hsotg: GNPTXSTS     @0xE026002C : 0x00080400
dwc2 12480000.hsotg: GI2CCTL      @0xE0260030 : 0x00000000
dwc2 12480000.hsotg: GPVNDCTL     @0xE0260034 : 0x00000000
dwc2 12480000.hsotg: GGPIO        @0xE0260038 : 0x00000000
dwc2 12480000.hsotg: GUID         @0xE026003C : 0x00000000
dwc2 12480000.hsotg: GSNPSID      @0xE0260040 : 0x4F54281A
dwc2 12480000.hsotg: GHWCFG1      @0xE0260044 : 0x00000000
dwc2 12480000.hsotg: GHWCFG2      @0xE0260048 : 0x228FFC50
dwc2 12480000.hsotg: GHWCFG3      @0xE026004C : 0x1E8084E8
dwc2 12480000.hsotg: GHWCFG4      @0xE0260050 : 0xFFF08030
dwc2 12480000.hsotg: GLPMCFG      @0xE0260054 : 0x00000000
dwc2 12480000.hsotg: GPWRDN       @0xE0260058 : 0x00000000
dwc2 12480000.hsotg: GDFIFOCFG    @0xE026005C : 0x00000000
dwc2 12480000.hsotg: HPTXFSIZ     @0xE0260100 : 0x00000000
dwc2 12480000.hsotg: PCGCTL       @0xE0260E00 : 0x00000000
dwc2 12480000.hsotg: dwc2_check_params: Invalid parameter besl=1
dwc2 12480000.hsotg: dwc2_check_params: Invalid parameter 
g_np_tx_fifo_size=1024
dwc2 12480000.hsotg: EPs: 16, dedicated fifos, 7808 entries in SPRAM
> Is the issue seen because of programming PCGCTL.PCGCTL_GATEHCLK bit
> only? To check it, could you please comment this bit setting/resetting
> in clock gating functions:
> dwc2_host_exit_clock_gating()
> dwc2_host_enter_clock_gating()
> dwc2_gadget_exit_clock_gating()
> dwc2_gadget_enter_clock_gating()

After removing programming PCGCTL.PCGCTL_GATEHCLK bit in the above 
functions, everything works fine.

Best regards

-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] usb: dwc2: Skip clock gating on Samsung SoCs

[Minas Harutyunyan]

Hi Marek,

On 8/3/2021 1:40 PM, Marek Szyprowski wrote:
> Hi Minas,
> 
> On 16.07.2021 16:54, Minas Harutyunyan wrote:
>> On 7/16/2021 9:01 AM, Marek Szyprowski wrote:
>>> Commit 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr
>>> function.") changed the way the driver handles power down modes in a such
>>> way that it uses clock gating when no other power down mode is available.
>>>
>>> This however doesn't work well on the DWC2 implementation used on the
>>> Samsung SoCs. When a clock gating is enabled, system hangs. It looks that
>>> the proper clock gating requires some additional glue code in the shared
>>> USB2 PHY and/or Samsung glue code for the DWC2. To restore driver
>>> operation on the Samsung SoCs simply skip enabling clock gating mode
>>> until one finds what is really needed to make it working reliably.
>>>
>>> Fixes: 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.")
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>>     drivers/usb/dwc2/core.h      | 4 ++++
>>>     drivers/usb/dwc2/core_intr.c | 3 ++-
>>>     drivers/usb/dwc2/hcd.c       | 6 ++++--
>>>     drivers/usb/dwc2/params.c    | 1 +
>>>     4 files changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>> index ab6b815e0089..483de2bbfaab 100644
>>> --- a/drivers/usb/dwc2/core.h
>>> +++ b/drivers/usb/dwc2/core.h
>>> @@ -383,6 +383,9 @@ enum dwc2_ep0_state {
>>>      *			0 - No (default)
>>>      *			1 - Partial power down
>>>      *			2 - Hibernation
>>> + * @no_clock_gating:	Specifies whether to avoid clock gating feature.
>>> + *			0 - No (use clock gating)
>>> + *			1 - Yes (avoid it)
>>>      * @lpm:		Enable LPM support.
>>>      *			0 - No
>>>      *			1 - Yes
>>> @@ -480,6 +483,7 @@ struct dwc2_core_params {
>>>     #define DWC2_POWER_DOWN_PARAM_NONE		0
>>>     #define DWC2_POWER_DOWN_PARAM_PARTIAL		1
>>>     #define DWC2_POWER_DOWN_PARAM_HIBERNATION	2
>>> +	bool no_clock_gating;
>>>     
>>>     	bool lpm;
>>>     	bool lpm_clock_gating;
>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>> index a5ab03808da6..a5c52b237e72 100644
>>> --- a/drivers/usb/dwc2/core_intr.c
>>> +++ b/drivers/usb/dwc2/core_intr.c
>>> @@ -556,7 +556,8 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
>>>     				 * If neither hibernation nor partial power down are supported,
>>>     				 * clock gating is used to save power.
>>>     				 */
>>> -				dwc2_gadget_enter_clock_gating(hsotg);
>>> +				if (!hsotg->params.no_clock_gating)
>>> +					dwc2_gadget_enter_clock_gating(hsotg);
>>>     			}
>>>     
>>>     			/*
>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>>> index 035d4911a3c3..2a7828971d05 100644
>>> --- a/drivers/usb/dwc2/hcd.c
>>> +++ b/drivers/usb/dwc2/hcd.c
>>> @@ -3338,7 +3338,8 @@ int dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
>>>     		 * If not hibernation nor partial power down are supported,
>>>     		 * clock gating is used to save power.
>>>     		 */
>>> -		dwc2_host_enter_clock_gating(hsotg);
>>> +		if (!hsotg->params.no_clock_gating)
>>> +			dwc2_host_enter_clock_gating(hsotg);
>>>     		break;
>>>     	}
>>>     
>>> @@ -4402,7 +4403,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>>>     		 * If not hibernation nor partial power down are supported,
>>>     		 * clock gating is used to save power.
>>>     		 */
>>> -		dwc2_host_enter_clock_gating(hsotg);
>>> +		if (!hsotg->params.no_clock_gating)
>>> +			dwc2_host_enter_clock_gating(hsotg);
>>>     
>>>     		/* After entering suspend, hardware is not accessible */
>>>     		clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
>>> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
>>> index 67c5eb140232..59e119345994 100644
>>> --- a/drivers/usb/dwc2/params.c
>>> +++ b/drivers/usb/dwc2/params.c
>>> @@ -76,6 +76,7 @@ static void dwc2_set_s3c6400_params(struct dwc2_hsotg *hsotg)
>>>     	struct dwc2_core_params *p = &hsotg->params;
>>>     
>>>     	p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
>>> +	p->no_clock_gating = true;
>>>     	p->phy_utmi_width = 8;
>>>     }
>>>     
>>>
>> In which mode host/device you see the issue?
> I do my test in my device mode.
>> What is your core version? Please provide GHWCFG1-4 registers values.
> 
> This is a result of dwc2_dump_global_registers() added just after
> dwc2_lowlevel_hw_enable() in dwc2_driver_probe():
> 
> dwc2 12480000.hsotg: Core Global Registers
> 
> dwc2 12480000.hsotg: GOTGCTL      @0xE0260000 : 0x000D0000
> dwc2 12480000.hsotg: GOTGINT      @0xE0260004 : 0x00000000
> dwc2 12480000.hsotg: GAHBCFG      @0xE0260008 : 0x00000027
> dwc2 12480000.hsotg: GUSBCFG      @0xE026000C : 0x0000540F
> dwc2 12480000.hsotg: GRSTCTL      @0xE0260010 : 0x80000040
> dwc2 12480000.hsotg: GINTSTS      @0xE0260014 : 0x54008428
> dwc2 12480000.hsotg: GINTMSK      @0xE0260018 : 0x800C3800
> dwc2 12480000.hsotg: GRXSTSR      @0xE026001C : 0x616EC77D
> dwc2 12480000.hsotg: GRXFSIZ      @0xE0260024 : 0x00000400
> dwc2 12480000.hsotg: GNPTXFSIZ    @0xE0260028 : 0x04000400
> dwc2 12480000.hsotg: GNPTXSTS     @0xE026002C : 0x00080400
> dwc2 12480000.hsotg: GI2CCTL      @0xE0260030 : 0x00000000
> dwc2 12480000.hsotg: GPVNDCTL     @0xE0260034 : 0x00000000
> dwc2 12480000.hsotg: GGPIO        @0xE0260038 : 0x00000000
> dwc2 12480000.hsotg: GUID         @0xE026003C : 0x00000000
> dwc2 12480000.hsotg: GSNPSID      @0xE0260040 : 0x4F54281A
> dwc2 12480000.hsotg: GHWCFG1      @0xE0260044 : 0x00000000
> dwc2 12480000.hsotg: GHWCFG2      @0xE0260048 : 0x228FFC50
> dwc2 12480000.hsotg: GHWCFG3      @0xE026004C : 0x1E8084E8
> dwc2 12480000.hsotg: GHWCFG4      @0xE0260050 : 0xFFF08030
> dwc2 12480000.hsotg: GLPMCFG      @0xE0260054 : 0x00000000
> dwc2 12480000.hsotg: GPWRDN       @0xE0260058 : 0x00000000
> dwc2 12480000.hsotg: GDFIFOCFG    @0xE026005C : 0x00000000
> dwc2 12480000.hsotg: HPTXFSIZ     @0xE0260100 : 0x00000000
> dwc2 12480000.hsotg: PCGCTL       @0xE0260E00 : 0x00000000
> dwc2 12480000.hsotg: dwc2_check_params: Invalid parameter besl=1
> dwc2 12480000.hsotg: dwc2_check_params: Invalid parameter
> g_np_tx_fifo_size=1024
> dwc2 12480000.hsotg: EPs: 16, dedicated fifos, 7808 entries in SPRAM
>> Is the issue seen because of programming PCGCTL.PCGCTL_GATEHCLK bit
>> only? To check it, could you please comment this bit setting/resetting
>> in clock gating functions:
>> dwc2_host_exit_clock_gating()
>> dwc2_host_enter_clock_gating()
>> dwc2_gadget_exit_clock_gating()
>> dwc2_gadget_enter_clock_gating()
> 
> After removing programming PCGCTL.PCGCTL_GATEHCLK bit in the above
> functions, everything works fine.
> 
> Best regards
> 
Thank you for testing and confirm my expectations.

There are 3 options:
1. Do not update your patch and accept it
2. Update your patch to exclude programming of PCGCTL.PCGCTL_GATEHCLK 
bit based on hsotg->params.no_clock_gating in all 
..._exit/enter_clock_gating functions
3. More radical solution, to have really ...POWER_DOWN_NONE case:
- rename DWC2_POWER_DOWN_PARAM_NONE to DWC2_POWER_DOWN_CLOCK_GATING in 
whole driver;
- define new power state "#define DWC2_POWER_DOWN_PARAM_NONE	-1"
- and for all platforms who doesn't want to have any power optimization 
keep:
     p->power_down = DWC2_POWER_DOWN_PARAM_NONE;


I would prefer option 3. What you think about this solution? Can you 
implement it (I guess it required 5 min) and test on your platform.

Thanks,
Minas

Re: [PATCH] usb: dwc2: Skip clock gating on Samsung SoCs

[Marek Szyprowski]

Hi

On 03.08.2021 12:08, Minas Harutyunyan wrote:
> On 8/3/2021 1:40 PM, Marek Szyprowski wrote:
>> On 16.07.2021 16:54, Minas Harutyunyan wrote:
>>> On 7/16/2021 9:01 AM, Marek Szyprowski wrote:
>>>> Commit 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr
>>>> function.") changed the way the driver handles power down modes in a such
>>>> way that it uses clock gating when no other power down mode is available.
>>>>
>>>> This however doesn't work well on the DWC2 implementation used on the
>>>> Samsung SoCs. When a clock gating is enabled, system hangs. It looks that
>>>> the proper clock gating requires some additional glue code in the shared
>>>> USB2 PHY and/or Samsung glue code for the DWC2. To restore driver
>>>> operation on the Samsung SoCs simply skip enabling clock gating mode
>>>> until one finds what is really needed to make it working reliably.
>>>>
>>>> Fixes: 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.")
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> ---
>>>>      drivers/usb/dwc2/core.h      | 4 ++++
>>>>      drivers/usb/dwc2/core_intr.c | 3 ++-
>>>>      drivers/usb/dwc2/hcd.c       | 6 ++++--
>>>>      drivers/usb/dwc2/params.c    | 1 +
>>>>      4 files changed, 11 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>>> index ab6b815e0089..483de2bbfaab 100644
>>>> --- a/drivers/usb/dwc2/core.h
>>>> +++ b/drivers/usb/dwc2/core.h
>>>> @@ -383,6 +383,9 @@ enum dwc2_ep0_state {
>>>>       *			0 - No (default)
>>>>       *			1 - Partial power down
>>>>       *			2 - Hibernation
>>>> + * @no_clock_gating:	Specifies whether to avoid clock gating feature.
>>>> + *			0 - No (use clock gating)
>>>> + *			1 - Yes (avoid it)
>>>>       * @lpm:		Enable LPM support.
>>>>       *			0 - No
>>>>       *			1 - Yes
>>>> @@ -480,6 +483,7 @@ struct dwc2_core_params {
>>>>      #define DWC2_POWER_DOWN_PARAM_NONE		0
>>>>      #define DWC2_POWER_DOWN_PARAM_PARTIAL		1
>>>>      #define DWC2_POWER_DOWN_PARAM_HIBERNATION	2
>>>> +	bool no_clock_gating;
>>>>      
>>>>      	bool lpm;
>>>>      	bool lpm_clock_gating;
>>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>>> index a5ab03808da6..a5c52b237e72 100644
>>>> --- a/drivers/usb/dwc2/core_intr.c
>>>> +++ b/drivers/usb/dwc2/core_intr.c
>>>> @@ -556,7 +556,8 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
>>>>      				 * If neither hibernation nor partial power down are supported,
>>>>      				 * clock gating is used to save power.
>>>>      				 */
>>>> -				dwc2_gadget_enter_clock_gating(hsotg);
>>>> +				if (!hsotg->params.no_clock_gating)
>>>> +					dwc2_gadget_enter_clock_gating(hsotg);
>>>>      			}
>>>>      
>>>>      			/*
>>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>>>> index 035d4911a3c3..2a7828971d05 100644
>>>> --- a/drivers/usb/dwc2/hcd.c
>>>> +++ b/drivers/usb/dwc2/hcd.c
>>>> @@ -3338,7 +3338,8 @@ int dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
>>>>      		 * If not hibernation nor partial power down are supported,
>>>>      		 * clock gating is used to save power.
>>>>      		 */
>>>> -		dwc2_host_enter_clock_gating(hsotg);
>>>> +		if (!hsotg->params.no_clock_gating)
>>>> +			dwc2_host_enter_clock_gating(hsotg);
>>>>      		break;
>>>>      	}
>>>>      
>>>> @@ -4402,7 +4403,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>>>>      		 * If not hibernation nor partial power down are supported,
>>>>      		 * clock gating is used to save power.
>>>>      		 */
>>>> -		dwc2_host_enter_clock_gating(hsotg);
>>>> +		if (!hsotg->params.no_clock_gating)
>>>> +			dwc2_host_enter_clock_gating(hsotg);
>>>>      
>>>>      		/* After entering suspend, hardware is not accessible */
>>>>      		clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
>>>> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
>>>> index 67c5eb140232..59e119345994 100644
>>>> --- a/drivers/usb/dwc2/params.c
>>>> +++ b/drivers/usb/dwc2/params.c
>>>> @@ -76,6 +76,7 @@ static void dwc2_set_s3c6400_params(struct dwc2_hsotg *hsotg)
>>>>      	struct dwc2_core_params *p = &hsotg->params;
>>>>      
>>>>      	p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
>>>> +	p->no_clock_gating = true;
>>>>      	p->phy_utmi_width = 8;
>>>>      }
>>>>      
>>>>
>>> In which mode host/device you see the issue?
>> I do my test in my device mode.
>>> What is your core version? Please provide GHWCFG1-4 registers values.
>> This is a result of dwc2_dump_global_registers() added just after
>> dwc2_lowlevel_hw_enable() in dwc2_driver_probe():
>>
>> dwc2 12480000.hsotg: Core Global Registers
>>
>> dwc2 12480000.hsotg: GOTGCTL      @0xE0260000 : 0x000D0000
>> dwc2 12480000.hsotg: GOTGINT      @0xE0260004 : 0x00000000
>> dwc2 12480000.hsotg: GAHBCFG      @0xE0260008 : 0x00000027
>> dwc2 12480000.hsotg: GUSBCFG      @0xE026000C : 0x0000540F
>> dwc2 12480000.hsotg: GRSTCTL      @0xE0260010 : 0x80000040
>> dwc2 12480000.hsotg: GINTSTS      @0xE0260014 : 0x54008428
>> dwc2 12480000.hsotg: GINTMSK      @0xE0260018 : 0x800C3800
>> dwc2 12480000.hsotg: GRXSTSR      @0xE026001C : 0x616EC77D
>> dwc2 12480000.hsotg: GRXFSIZ      @0xE0260024 : 0x00000400
>> dwc2 12480000.hsotg: GNPTXFSIZ    @0xE0260028 : 0x04000400
>> dwc2 12480000.hsotg: GNPTXSTS     @0xE026002C : 0x00080400
>> dwc2 12480000.hsotg: GI2CCTL      @0xE0260030 : 0x00000000
>> dwc2 12480000.hsotg: GPVNDCTL     @0xE0260034 : 0x00000000
>> dwc2 12480000.hsotg: GGPIO        @0xE0260038 : 0x00000000
>> dwc2 12480000.hsotg: GUID         @0xE026003C : 0x00000000
>> dwc2 12480000.hsotg: GSNPSID      @0xE0260040 : 0x4F54281A
>> dwc2 12480000.hsotg: GHWCFG1      @0xE0260044 : 0x00000000
>> dwc2 12480000.hsotg: GHWCFG2      @0xE0260048 : 0x228FFC50
>> dwc2 12480000.hsotg: GHWCFG3      @0xE026004C : 0x1E8084E8
>> dwc2 12480000.hsotg: GHWCFG4      @0xE0260050 : 0xFFF08030
>> dwc2 12480000.hsotg: GLPMCFG      @0xE0260054 : 0x00000000
>> dwc2 12480000.hsotg: GPWRDN       @0xE0260058 : 0x00000000
>> dwc2 12480000.hsotg: GDFIFOCFG    @0xE026005C : 0x00000000
>> dwc2 12480000.hsotg: HPTXFSIZ     @0xE0260100 : 0x00000000
>> dwc2 12480000.hsotg: PCGCTL       @0xE0260E00 : 0x00000000
>> dwc2 12480000.hsotg: dwc2_check_params: Invalid parameter besl=1
>> dwc2 12480000.hsotg: dwc2_check_params: Invalid parameter
>> g_np_tx_fifo_size=1024
>> dwc2 12480000.hsotg: EPs: 16, dedicated fifos, 7808 entries in SPRAM
>>> Is the issue seen because of programming PCGCTL.PCGCTL_GATEHCLK bit
>>> only? To check it, could you please comment this bit setting/resetting
>>> in clock gating functions:
>>> dwc2_host_exit_clock_gating()
>>> dwc2_host_enter_clock_gating()
>>> dwc2_gadget_exit_clock_gating()
>>> dwc2_gadget_enter_clock_gating()
>> After removing programming PCGCTL.PCGCTL_GATEHCLK bit in the above
>> functions, everything works fine.
>>
>> Best regards
>>
> Thank you for testing and confirm my expectations.
>
> There are 3 options:
> 1. Do not update your patch and accept it
> 2. Update your patch to exclude programming of PCGCTL.PCGCTL_GATEHCLK
> bit based on hsotg->params.no_clock_gating in all
> ..._exit/enter_clock_gating functions
> 3. More radical solution, to have really ...POWER_DOWN_NONE case:
> - rename DWC2_POWER_DOWN_PARAM_NONE to DWC2_POWER_DOWN_CLOCK_GATING in
> whole driver;
> - define new power state "#define DWC2_POWER_DOWN_PARAM_NONE	-1"
> - and for all platforms who doesn't want to have any power optimization
> keep:
>       p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
>
>
> I would prefer option 3. What you think about this solution? Can you
> implement it (I guess it required 5 min) and test on your platform.

Okay, I will do the 3rd option. However, the $subject patch has been 
already merged to v5.14-rc3 some time ago, so I will do that on top of 
v5.14-rc3.

Best regards

-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] usb: dwc2: Skip clock gating on Samsung SoCs

[Minas Harutyunyan]

Hi Marek,

On 8/4/2021 3:29 PM, Marek Szyprowski wrote:
> Hi
> 
> On 03.08.2021 12:08, Minas Harutyunyan wrote:
>> On 8/3/2021 1:40 PM, Marek Szyprowski wrote:
>>> On 16.07.2021 16:54, Minas Harutyunyan wrote:
>>>> On 7/16/2021 9:01 AM, Marek Szyprowski wrote:
>>>>> Commit 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr
>>>>> function.") changed the way the driver handles power down modes in a such
>>>>> way that it uses clock gating when no other power down mode is available.
>>>>>
>>>>> This however doesn't work well on the DWC2 implementation used on the
>>>>> Samsung SoCs. When a clock gating is enabled, system hangs. It looks that
>>>>> the proper clock gating requires some additional glue code in the shared
>>>>> USB2 PHY and/or Samsung glue code for the DWC2. To restore driver
>>>>> operation on the Samsung SoCs simply skip enabling clock gating mode
>>>>> until one finds what is really needed to make it working reliably.
>>>>>
>>>>> Fixes: 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.")
>>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>>> ---
>>>>>       drivers/usb/dwc2/core.h      | 4 ++++
>>>>>       drivers/usb/dwc2/core_intr.c | 3 ++-
>>>>>       drivers/usb/dwc2/hcd.c       | 6 ++++--
>>>>>       drivers/usb/dwc2/params.c    | 1 +
>>>>>       4 files changed, 11 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>>>> index ab6b815e0089..483de2bbfaab 100644
>>>>> --- a/drivers/usb/dwc2/core.h
>>>>> +++ b/drivers/usb/dwc2/core.h
>>>>> @@ -383,6 +383,9 @@ enum dwc2_ep0_state {
>>>>>        *			0 - No (default)
>>>>>        *			1 - Partial power down
>>>>>        *			2 - Hibernation
>>>>> + * @no_clock_gating:	Specifies whether to avoid clock gating feature.
>>>>> + *			0 - No (use clock gating)
>>>>> + *			1 - Yes (avoid it)
>>>>>        * @lpm:		Enable LPM support.
>>>>>        *			0 - No
>>>>>        *			1 - Yes
>>>>> @@ -480,6 +483,7 @@ struct dwc2_core_params {
>>>>>       #define DWC2_POWER_DOWN_PARAM_NONE		0
>>>>>       #define DWC2_POWER_DOWN_PARAM_PARTIAL		1
>>>>>       #define DWC2_POWER_DOWN_PARAM_HIBERNATION	2
>>>>> +	bool no_clock_gating;
>>>>>       
>>>>>       	bool lpm;
>>>>>       	bool lpm_clock_gating;
>>>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>>>> index a5ab03808da6..a5c52b237e72 100644
>>>>> --- a/drivers/usb/dwc2/core_intr.c
>>>>> +++ b/drivers/usb/dwc2/core_intr.c
>>>>> @@ -556,7 +556,8 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
>>>>>       				 * If neither hibernation nor partial power down are supported,
>>>>>       				 * clock gating is used to save power.
>>>>>       				 */
>>>>> -				dwc2_gadget_enter_clock_gating(hsotg);
>>>>> +				if (!hsotg->params.no_clock_gating)
>>>>> +					dwc2_gadget_enter_clock_gating(hsotg);
>>>>>       			}
>>>>>       
>>>>>       			/*
>>>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>>>>> index 035d4911a3c3..2a7828971d05 100644
>>>>> --- a/drivers/usb/dwc2/hcd.c
>>>>> +++ b/drivers/usb/dwc2/hcd.c
>>>>> @@ -3338,7 +3338,8 @@ int dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
>>>>>       		 * If not hibernation nor partial power down are supported,
>>>>>       		 * clock gating is used to save power.
>>>>>       		 */
>>>>> -		dwc2_host_enter_clock_gating(hsotg);
>>>>> +		if (!hsotg->params.no_clock_gating)
>>>>> +			dwc2_host_enter_clock_gating(hsotg);
>>>>>       		break;
>>>>>       	}
>>>>>       
>>>>> @@ -4402,7 +4403,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>>>>>       		 * If not hibernation nor partial power down are supported,
>>>>>       		 * clock gating is used to save power.
>>>>>       		 */
>>>>> -		dwc2_host_enter_clock_gating(hsotg);
>>>>> +		if (!hsotg->params.no_clock_gating)
>>>>> +			dwc2_host_enter_clock_gating(hsotg);
>>>>>       
>>>>>       		/* After entering suspend, hardware is not accessible */
>>>>>       		clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
>>>>> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
>>>>> index 67c5eb140232..59e119345994 100644
>>>>> --- a/drivers/usb/dwc2/params.c
>>>>> +++ b/drivers/usb/dwc2/params.c
>>>>> @@ -76,6 +76,7 @@ static void dwc2_set_s3c6400_params(struct dwc2_hsotg *hsotg)
>>>>>       	struct dwc2_core_params *p = &hsotg->params;
>>>>>       
>>>>>       	p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
>>>>> +	p->no_clock_gating = true;
>>>>>       	p->phy_utmi_width = 8;
>>>>>       }
>>>>>       
>>>>>
>>>> In which mode host/device you see the issue?
>>> I do my test in my device mode.
>>>> What is your core version? Please provide GHWCFG1-4 registers values.
>>> This is a result of dwc2_dump_global_registers() added just after
>>> dwc2_lowlevel_hw_enable() in dwc2_driver_probe():
>>>
>>> dwc2 12480000.hsotg: Core Global Registers
>>>
>>> dwc2 12480000.hsotg: GOTGCTL      @0xE0260000 : 0x000D0000
>>> dwc2 12480000.hsotg: GOTGINT      @0xE0260004 : 0x00000000
>>> dwc2 12480000.hsotg: GAHBCFG      @0xE0260008 : 0x00000027
>>> dwc2 12480000.hsotg: GUSBCFG      @0xE026000C : 0x0000540F
>>> dwc2 12480000.hsotg: GRSTCTL      @0xE0260010 : 0x80000040
>>> dwc2 12480000.hsotg: GINTSTS      @0xE0260014 : 0x54008428
>>> dwc2 12480000.hsotg: GINTMSK      @0xE0260018 : 0x800C3800
>>> dwc2 12480000.hsotg: GRXSTSR      @0xE026001C : 0x616EC77D
>>> dwc2 12480000.hsotg: GRXFSIZ      @0xE0260024 : 0x00000400
>>> dwc2 12480000.hsotg: GNPTXFSIZ    @0xE0260028 : 0x04000400
>>> dwc2 12480000.hsotg: GNPTXSTS     @0xE026002C : 0x00080400
>>> dwc2 12480000.hsotg: GI2CCTL      @0xE0260030 : 0x00000000
>>> dwc2 12480000.hsotg: GPVNDCTL     @0xE0260034 : 0x00000000
>>> dwc2 12480000.hsotg: GGPIO        @0xE0260038 : 0x00000000
>>> dwc2 12480000.hsotg: GUID         @0xE026003C : 0x00000000
>>> dwc2 12480000.hsotg: GSNPSID      @0xE0260040 : 0x4F54281A
>>> dwc2 12480000.hsotg: GHWCFG1      @0xE0260044 : 0x00000000
>>> dwc2 12480000.hsotg: GHWCFG2      @0xE0260048 : 0x228FFC50
>>> dwc2 12480000.hsotg: GHWCFG3      @0xE026004C : 0x1E8084E8
>>> dwc2 12480000.hsotg: GHWCFG4      @0xE0260050 : 0xFFF08030
>>> dwc2 12480000.hsotg: GLPMCFG      @0xE0260054 : 0x00000000
>>> dwc2 12480000.hsotg: GPWRDN       @0xE0260058 : 0x00000000
>>> dwc2 12480000.hsotg: GDFIFOCFG    @0xE026005C : 0x00000000
>>> dwc2 12480000.hsotg: HPTXFSIZ     @0xE0260100 : 0x00000000
>>> dwc2 12480000.hsotg: PCGCTL       @0xE0260E00 : 0x00000000
>>> dwc2 12480000.hsotg: dwc2_check_params: Invalid parameter besl=1
>>> dwc2 12480000.hsotg: dwc2_check_params: Invalid parameter
>>> g_np_tx_fifo_size=1024
>>> dwc2 12480000.hsotg: EPs: 16, dedicated fifos, 7808 entries in SPRAM
>>>> Is the issue seen because of programming PCGCTL.PCGCTL_GATEHCLK bit
>>>> only? To check it, could you please comment this bit setting/resetting
>>>> in clock gating functions:
>>>> dwc2_host_exit_clock_gating()
>>>> dwc2_host_enter_clock_gating()
>>>> dwc2_gadget_exit_clock_gating()
>>>> dwc2_gadget_enter_clock_gating()
>>> After removing programming PCGCTL.PCGCTL_GATEHCLK bit in the above
>>> functions, everything works fine.
>>>
>>> Best regards
>>>
>> Thank you for testing and confirm my expectations.
>>
>> There are 3 options:
>> 1. Do not update your patch and accept it
>> 2. Update your patch to exclude programming of PCGCTL.PCGCTL_GATEHCLK
>> bit based on hsotg->params.no_clock_gating in all
>> ..._exit/enter_clock_gating functions
>> 3. More radical solution, to have really ...POWER_DOWN_NONE case:
>> - rename DWC2_POWER_DOWN_PARAM_NONE to DWC2_POWER_DOWN_CLOCK_GATING in
>> whole driver;
>> - define new power state "#define DWC2_POWER_DOWN_PARAM_NONE	-1"
>> - and for all platforms who doesn't want to have any power optimization
>> keep:
>>        p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
>>
>>
>> I would prefer option 3. What you think about this solution? Can you
>> implement it (I guess it required 5 min) and test on your platform.
> 
> Okay, I will do the 3rd option. However, the $subject patch has been
> already merged to v5.14-rc3 some time ago, so I will do that on top of
> v5.14-rc3.
> 
> Best regards
> 

Thank you for patch. Give me 1-2 days to review and test on our platform.

Thanks,
Minas