On 12/01/2024 10: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
Aren't they both doing the same thing? I'd rather define them at one place
i.e. dwc3/core.c.
The driver model design for dwc3 looks really broken in u-boot.
"snps,dwc3" is claimed by xhci-dwc3.c instead of being claimed by dwc3/core.c.
This is probably because at the time USB host was the more needed use
case at u-boot.
Ideally dwc3/core.c should claim "snps,dwc3" device and instantiate
the respective Host/Device/OTG device based on the provided otg mode.
For Host/Device mode it is straight forward.
dwc3/core.c does dwc3_set_mode() depending on the mode and:
for host mode -> register xhci-usb driver.
for device mode -> register UDC device driver.
For dual-role mode, the solution is not very clear as I don't think
there is a role switch framework present.
To begin with, we could probably restrict it to just device mode
as most platforms were forcing it to device mode in any case as they
usually have a 2nd USB port that is host only.
A better solution would be to introduce a role switch mechanism
like it exists in Linux so user can choose what mode he wants to
operate. That could wait till someone really wants such a feature.
>
> Signed-off-by: Sjoerd Simons <sjo...@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);
--
cheers,
-roger
