Re: [PATCH v4 2/7] usb: dwc3: Switch to device mode on gadget start

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Sjoerd,

Thank you for the patch.

On ven., janv. 12, 2024 at 09:52, Sjoerd Simons <sjoerd@collabora.com> wrote:

> When dr_mode is "otg" the dwc3 is initially configured in _OTG mode;
> However in this mode the gadget functionality doesn't work without
> further configuration. To resolve that on gadget start switch to _DEVICE
> mode globally and go back to _OTG on stop again.
>
> For this the dwc3_set_mode is renamed to dwc3_core_set_mode to avoid a
> conflict with the same function exposed by xhci-dwc3
>
> Signed-off-by: Sjoerd Simons <sjoerd@collabora.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

nitpick below.

>
> ---
>
> Changes in v4:
> - New patch
>
>  drivers/usb/dwc3/core.c   | 10 +++++-----
>  drivers/usb/dwc3/core.h   |  1 +
>  drivers/usb/dwc3/gadget.c |  6 ++++++
>  3 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 4b4fcd8a22e..d22d4c4bb6a 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -42,7 +42,7 @@
>  static LIST_HEAD(dwc3_list);
>  /* -------------------------------------------------------------------------- */
>  
> -static void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
> +void dwc3_core_set_mode(struct dwc3 *dwc, u32 mode)
>  {
>  	u32 reg;
>  
> @@ -736,7 +736,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>  
>  	switch (dwc->dr_mode) {
>  	case USB_DR_MODE_PERIPHERAL:
> -		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> +		dwc3_core_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>  		ret = dwc3_gadget_init(dwc);
>  		if (ret) {
>  			dev_err(dwc->dev, "failed to initialize gadget\n");
> @@ -744,7 +744,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>  		}
>  		break;
>  	case USB_DR_MODE_HOST:
> -		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
> +		dwc3_core_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
>  		ret = dwc3_host_init(dwc);
>  		if (ret) {
>  			dev_err(dwc->dev, "failed to initialize host\n");
> @@ -752,7 +752,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>  		}
>  		break;
>  	case USB_DR_MODE_OTG:
> -		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
> +		dwc3_core_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
>  		ret = dwc3_host_init(dwc);
>  		if (ret) {
>  			dev_err(dwc->dev, "failed to initialize host\n");
> @@ -810,7 +810,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
>  	 * switch back to peripheral mode
>  	 * This enables the phy to enter idle and then, if enabled, suspend.
>  	 */
> -	dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> +	dwc3_core_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>  	dwc3_gadget_run(dwc);
>  }
>  
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 4162a682298..1e7eda89a34 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1057,6 +1057,7 @@ int dwc3_gadget_resize_tx_fifos(struct dwc3 *dwc);
>  void dwc3_of_parse(struct dwc3 *dwc);
>  int dwc3_init(struct dwc3 *dwc);
>  void dwc3_remove(struct dwc3 *dwc);
> +void dwc3_core_set_mode(struct dwc3 *dwc, u32 mode);
>  
>  static inline int dwc3_host_init(struct dwc3 *dwc)
>  { return 0; }
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 406d36ceafe..69d9fe40e2f 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1468,6 +1468,9 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>  
>  	dwc->gadget_driver	= driver;
>  

Doesn't this bit deserves a comment ? When looking at the code it's not
obvious that we do this because PRTCAP_OTG is non-functional without any
additional configuration.

Maybe something in the lines of:

   /**
    * WORKAROUND: in OTG mode, gadget functionality is non-functional.
    * Switch to gadget mode only to enable gadget mode
    */

?

> +	if (dwc->dr_mode == DWC3_GCTL_PRTCAP_OTG)
> +		dwc3_core_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> +
>  	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
>  	reg &= ~(DWC3_DCFG_SPEED_MASK);
>  
> @@ -1559,6 +1562,9 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>  	__dwc3_gadget_ep_disable(dwc->eps[0]);
>  	__dwc3_gadget_ep_disable(dwc->eps[1]);
>  
> +	if (dwc->dr_mode == DWC3_GCTL_PRTCAP_OTG)
> +		dwc3_core_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
> +
>  	dwc->gadget_driver	= NULL;
>  
>  	spin_unlock_irqrestore(&dwc->lock, flags);
> -- 
> 2.43.0