diff mbox series

[v3] usb: dwc3: Issue core soft reset before enabling run/stop

Message ID 20220315014317.14265-1-quic_wcheng@quicinc.com (mailing list archive)
State Superseded
Headers show
Series [v3] usb: dwc3: Issue core soft reset before enabling run/stop | expand

Commit Message

Wesley Cheng March 15, 2022, 1:43 a.m. UTC
It is recommended by the Synopsis databook to issue a DCTL.CSftReset
when reconnecting from a device-initiated disconnect routine.  This
resolves issues with enumeration during fast composition switching
cases, which result in an unknown device on the host.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 Changes in v3:
   - Removed change from RFC series and placed into its own patch.

 Previous patches:
  https://lore.kernel.org/linux-usb/20220203080017.27339-1-quic_wcheng@quicinc.com/

 drivers/usb/dwc3/core.c   |  4 +---
 drivers/usb/dwc3/core.h   |  2 ++
 drivers/usb/dwc3/gadget.c | 11 +++++++++++
 3 files changed, 14 insertions(+), 3 deletions(-)

Comments

Thinh Nguyen March 16, 2022, 12:28 a.m. UTC | #1
Wesley Cheng wrote:
> It is recommended by the Synopsis databook to issue a DCTL.CSftReset
> when reconnecting from a device-initiated disconnect routine.  This
> resolves issues with enumeration during fast composition switching
> cases, which result in an unknown device on the host.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>  Changes in v3:
>    - Removed change from RFC series and placed into its own patch.
> 
>  Previous patches:
>   https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20220203080017.27339-1-quic_wcheng@quicinc.com/__;!!A4F2R9G_pg!OjNviLgYSUvXpUWpeTDiI6OVuwjW2kjQpACAYYo5MdI09GClnUHLGFjuoMrtquF8Qe9X$ 
> 
>  drivers/usb/dwc3/core.c   |  4 +---
>  drivers/usb/dwc3/core.h   |  2 ++
>  drivers/usb/dwc3/gadget.c | 11 +++++++++++
>  3 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 18adddfba3da..02d10e1cb774 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -115,8 +115,6 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
>  	dwc->current_dr_role = mode;
>  }
>  
> -static int dwc3_core_soft_reset(struct dwc3 *dwc);
> -
>  static void __dwc3_set_mode(struct work_struct *work)
>  {
>  	struct dwc3 *dwc = work_to_dwc(work);
> @@ -261,7 +259,7 @@ u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type)
>   * dwc3_core_soft_reset - Issues core soft reset and PHY reset
>   * @dwc: pointer to our context structure
>   */
> -static int dwc3_core_soft_reset(struct dwc3 *dwc)
> +int dwc3_core_soft_reset(struct dwc3 *dwc)
>  {
>  	u32		reg;
>  	int		retries = 1000;
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index eb9c1efced05..86e27afef6c5 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1530,6 +1530,8 @@ bool dwc3_has_imod(struct dwc3 *dwc);
>  int dwc3_event_buffers_setup(struct dwc3 *dwc);
>  void dwc3_event_buffers_cleanup(struct dwc3 *dwc);
>  
> +int dwc3_core_soft_reset(struct dwc3 *dwc);
> +
>  #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
>  int dwc3_host_init(struct dwc3 *dwc);
>  void dwc3_host_exit(struct dwc3 *dwc);
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index a0c883f19a41..448ff6cb9c22 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2544,6 +2544,17 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>  						dwc->ev_buf->length;
>  		}
>  	} else {
> +		/*
> +		 * In the Synopsis DesignWare Cores USB3 Databook Rev. 1.90a

It's "Synopsys". Version 1.90a is for DWC_usb31 controller.

> +		 * Section 4.1.9, it specifies that for a reconnect after a
> +		 * device-initiated disconnect requires a core soft reset
> +		 * (DCTL.CSftRst) before enabling the run/stop bit.

Just want to note that we're skipping some controller initialization on
soft-reset here. But it's probably fine because the global registers
don't get reset on soft reset. Just need to make sure that the registers
that do get reset get reinitialized on __dwc3_gadget_start().

> +		 */
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +		dwc3_core_soft_reset(dwc);
> +		spin_lock_irqsave(&dwc->lock, flags);
> +
> +		dwc3_event_buffers_setup(dwc);
>  		__dwc3_gadget_start(dwc);
>  	}
>  

It's a little awkward because dwc3 also issues soft-reset during driver
probe. Pullup() is called during driver bindings. So soft-reset is
called multiple times. I don't have a better solution at the moment, but
I don't see a problem with it either.

After fixing the typos,

Reviewed-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh
Wesley Cheng March 16, 2022, 1:10 a.m. UTC | #2
On 3/15/2022 5:28 PM, Thinh Nguyen wrote:
> Wesley Cheng wrote:
>> It is recommended by the Synopsis databook to issue a DCTL.CSftReset
>> when reconnecting from a device-initiated disconnect routine.  This
>> resolves issues with enumeration during fast composition switching
>> cases, which result in an unknown device on the host.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>  Changes in v3:
>>    - Removed change from RFC series and placed into its own patch.
>>
>>  Previous patches:
>>   https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20220203080017.27339-1-quic_wcheng@quicinc.com/__;!!A4F2R9G_pg!OjNviLgYSUvXpUWpeTDiI6OVuwjW2kjQpACAYYo5MdI09GClnUHLGFjuoMrtquF8Qe9X$ 
>>
>>  drivers/usb/dwc3/core.c   |  4 +---
>>  drivers/usb/dwc3/core.h   |  2 ++
>>  drivers/usb/dwc3/gadget.c | 11 +++++++++++
>>  3 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 18adddfba3da..02d10e1cb774 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -115,8 +115,6 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
>>  	dwc->current_dr_role = mode;
>>  }
>>  
>> -static int dwc3_core_soft_reset(struct dwc3 *dwc);
>> -
>>  static void __dwc3_set_mode(struct work_struct *work)
>>  {
>>  	struct dwc3 *dwc = work_to_dwc(work);
>> @@ -261,7 +259,7 @@ u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type)
>>   * dwc3_core_soft_reset - Issues core soft reset and PHY reset
>>   * @dwc: pointer to our context structure
>>   */
>> -static int dwc3_core_soft_reset(struct dwc3 *dwc)
>> +int dwc3_core_soft_reset(struct dwc3 *dwc)
>>  {
>>  	u32		reg;
>>  	int		retries = 1000;
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index eb9c1efced05..86e27afef6c5 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1530,6 +1530,8 @@ bool dwc3_has_imod(struct dwc3 *dwc);
>>  int dwc3_event_buffers_setup(struct dwc3 *dwc);
>>  void dwc3_event_buffers_cleanup(struct dwc3 *dwc);
>>  
>> +int dwc3_core_soft_reset(struct dwc3 *dwc);
>> +
>>  #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
>>  int dwc3_host_init(struct dwc3 *dwc);
>>  void dwc3_host_exit(struct dwc3 *dwc);
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index a0c883f19a41..448ff6cb9c22 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2544,6 +2544,17 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>  						dwc->ev_buf->length;
>>  		}
>>  	} else {
>> +		/*
>> +		 * In the Synopsis DesignWare Cores USB3 Databook Rev. 1.90a
Hi Thinh,

Thanks for the review.
> 
> It's "Synopsys". Version 1.90a is for DWC_usb31 controller.
> 
Ah, I need to get the incorrect spelling out of my head! :)

>> +		 * Section 4.1.9, it specifies that for a reconnect after a
>> +		 * device-initiated disconnect requires a core soft reset
>> +		 * (DCTL.CSftRst) before enabling the run/stop bit.
> 
> Just want to note that we're skipping some controller initialization on
> soft-reset here. But it's probably fine because the global registers
> don't get reset on soft reset. Just need to make sure that the registers
> that do get reset get reinitialized on __dwc3_gadget_start().
> 
>> +		 */
>> +		spin_unlock_irqrestore(&dwc->lock, flags);
>> +		dwc3_core_soft_reset(dwc);
>> +		spin_lock_irqsave(&dwc->lock, flags);
>> +
>> +		dwc3_event_buffers_setup(dwc);
>>  		__dwc3_gadget_start(dwc);
>>  	}
>>  
> 
> It's a little awkward because dwc3 also issues soft-reset during driver
> probe. Pullup() is called during driver bindings. So soft-reset is
> called multiple times. I don't have a better solution at the moment, but
> I don't see a problem with it either.
> Correct...this would mainly help w/ the soft connect/disconnect
situations.  Will fixup the typos and resend.

> After fixing the typos,
> 
> Reviewed-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> 
> Thanks,
> Thinh

Thanks
Wesley Cheng
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 18adddfba3da..02d10e1cb774 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -115,8 +115,6 @@  void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
 	dwc->current_dr_role = mode;
 }
 
-static int dwc3_core_soft_reset(struct dwc3 *dwc);
-
 static void __dwc3_set_mode(struct work_struct *work)
 {
 	struct dwc3 *dwc = work_to_dwc(work);
@@ -261,7 +259,7 @@  u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type)
  * dwc3_core_soft_reset - Issues core soft reset and PHY reset
  * @dwc: pointer to our context structure
  */
-static int dwc3_core_soft_reset(struct dwc3 *dwc)
+int dwc3_core_soft_reset(struct dwc3 *dwc)
 {
 	u32		reg;
 	int		retries = 1000;
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index eb9c1efced05..86e27afef6c5 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1530,6 +1530,8 @@  bool dwc3_has_imod(struct dwc3 *dwc);
 int dwc3_event_buffers_setup(struct dwc3 *dwc);
 void dwc3_event_buffers_cleanup(struct dwc3 *dwc);
 
+int dwc3_core_soft_reset(struct dwc3 *dwc);
+
 #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
 int dwc3_host_init(struct dwc3 *dwc);
 void dwc3_host_exit(struct dwc3 *dwc);
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index a0c883f19a41..448ff6cb9c22 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2544,6 +2544,17 @@  static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
 						dwc->ev_buf->length;
 		}
 	} else {
+		/*
+		 * In the Synopsis DesignWare Cores USB3 Databook Rev. 1.90a
+		 * Section 4.1.9, it specifies that for a reconnect after a
+		 * device-initiated disconnect requires a core soft reset
+		 * (DCTL.CSftRst) before enabling the run/stop bit.
+		 */
+		spin_unlock_irqrestore(&dwc->lock, flags);
+		dwc3_core_soft_reset(dwc);
+		spin_lock_irqsave(&dwc->lock, flags);
+
+		dwc3_event_buffers_setup(dwc);
 		__dwc3_gadget_start(dwc);
 	}