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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Sjoerd, Caleb

On ven., janv. 12, 2024 at 12:55, Caleb Connolly <caleb.connolly@linaro.org> wrote:

> Hi Sjoerd,
>
> On 12/01/2024 08:52, Sjoerd Simons 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
>
> I think exporting dwc3_core_set_mode() is probably sensible here. But
> I'm not so sure on calling it from dwc3_gadget_start(), that's making
> assumptions about board specific implementation details - some boards
> might require additional configuration to switch into gadget mode.

Indeed. As an example, take the Khadas VIM3 which has a glue driver to
do the mode switching.

>
> What about calling dwc3_core_set_mode() from within your board-specific
> board_usb_init() function?

With DM_USB_GADGET, board_usb_init/cleanup() are no longer used.

For VIM3, I solved this with:
https://lore.kernel.org/all/20221024-meson-dm-usb-v1-1-2ab077a503b9@baylibre.com/

Maybe that can help?

>
> Obviously as you said the ideal solution here is that we can correctly
> traverse the type-c connector model from within U-Boot, but that's a
> whole other kettle of fish :P
>
> On Qualcomm platforms I'm currently handling this by overriding the
> dr_mode property in a <board>-u-boot.dtsi file, if you aren't using the
> same DT for U-Boot and Linux then maybe this would be acceptable for you
> too?

I would prefer a <board>-u-boot.dtsi change as well.

>
> Kind regards,
>> 
>> Signed-off-by: Sjoerd Simons <sjoerd@collabora.com>
>> 
>> ---
>> 
>> 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;
>>  
>> +	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);
>
> -- 
> // Caleb (they/them)