[PATCH v2 1/2] usb: dwc3-generic: set "mode" based on caller of dwc3_generic_probe()

Siddharth Vadapalli s-vadapalli at ti.com
Wed Dec 4 08:47:12 CET 2024


On Wed, Dec 04, 2024 at 09:32:23AM +0200, Roger Quadros wrote:
> 
> 
> On 04/12/2024 07:16, Siddharth Vadapalli wrote:
> > On Tue, Dec 03, 2024 at 09:23:11PM +0200, Roger Quadros wrote:

[...]

> >>> +++ b/drivers/usb/dwc3/dwc3-generic.c
> >>> @@ -51,7 +51,8 @@ struct dwc3_generic_host_priv {
> >>>  };
> >>>  
> >>>  static int dwc3_generic_probe(struct udevice *dev,
> >>> -			      struct dwc3_generic_priv *priv)
> >>> +			      struct dwc3_generic_priv *priv,
> >>> +			      enum usb_dr_mode mode)
> >>
> >> This is not necessary as dwc3_generic_host_probe will only be
> >> registered if dr_mode == "host" and dwc3_generic_peripheral_probe
> >> will only be registered if dr_mode == "host" or "peripheral"
> > 
> > But I don't see it happening that way. dwc3_generic_host_probe() or
> > dwc3_generic_peripheral_probe() seem to be invoked based on the command
> > executed - USB Host command will trigger generic_host_probe() while USB
> > Device/Gadget command will trigger generic_peripheral_probe().
> > 
> 
> But, when dr_mode is "otg" generic host driver is *not* bound to the
> DWC3 device. So dwc3_generic_host_probe() will never be called for it.

In USB DFU boot, "dwc3_generic_peripheral_boot()" is invoked. Please
test it out on AM625-SK. I was able to observe this on AM62A7-SK.

> 
> > 
> >>
> >>>  {
> >>>  	int rc;
> >>>  	struct dwc3_generic_plat *plat = dev_get_plat(dev);
> >>> @@ -62,7 +63,19 @@ static int dwc3_generic_probe(struct udevice *dev,
> >>>  
> >>>  	dwc3->dev = dev;
> >>>  	dwc3->maximum_speed = plat->maximum_speed;
> >>> -	dwc3->dr_mode = plat->dr_mode;
> >>> +
> >>> +	/*
> >>> +	 * If the controller supports OTG as indicated by plat->dr_mode,
> >>> +	 * then either Host or Peripheral mode is acceptable.
> >>> +	 * Otherwise, error out since the platform cannot support the mode
> >>> +	 * being requested by the caller of dwc3_generic_probe().
> >>> +	 */
> >>> +	if (plat->dr_mode != mode && plat->dr_mode != USB_DR_MODE_OTG) {
> >>> +		pr_err("Requested usb mode is not supported by platform\n");
> >>
> >> This is actually not necessary. Sorry for suggesting this approach before.
> > 
> > While the check may not be necessary, dr_mode *should* be set based on
> > the caller and not based on plat->dr_mode. The reason I say so is that
> > plat->dr_mode could be "otg" in the device-tree. But "otg" shouldn't be
> > used for further configuration. I had indicated the reason for this in
> > the cover-letter, which I am pasting below for your reference:
> > 
> > ---------------------------------------------------------------------------
> > Currently, dwc3_generic_probe() sets the mode based on the device-tree
> > rather than setting it on the basis of the caller. While this might be
> > correct when the mode is "host" or "peripheral" in the device-tree, it is
> > wrong when the mode is "otg" in the device-tree. It is wrong because of two
> > reasons:
> > 1. There is no OTG state machine in U-Boot. Hence the role will never
> > switch to the correct one eventually among host or peripheral.
> > 2. dr_mode = "otg" results in the "PRTCAPDIR" field of the "GCTL"
> > register of the USB Controller being set to 11b. According to the
> > datasheet of the Designware USB Dual Role Controller, "PRTCAPDIR"
> > should never be set to any value other than 01b (Host) and 10b (Device).
> > Quoting the datasheet:
> > "Programming this field with random data causes the controller
> > to keep toggling between the host mode and the device mode."
> > 
> > Therefore, in order to avoid programming 11b in "PRTCAPDIR", and, given
> > that the caller specifies the intended role, rather than simply using
> > the "dr_mode" property to set the role, set the role on the basis of the
> > caller (intended role) and the device-tree (platform support).
> > ----------------------------------------------------------------------------
> > 
> > Point #2 above is the main reason why dr_mode cannot be set to "otg" in
> > dwc3_generic_probe(). The only difference now is that the check can be
> > dropped as you have indicated, so the last paragraph above will change
> 
> Finally we are at the root of the problem. But the solution is not to
> change the dr_mode but instead handle it correctly in dwc3_core_init_mode()
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index a35b8c2f646..7f42b6e62c6 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -752,13 +752,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>  		}
>  		break;
>  	case USB_DR_MODE_OTG:
> -		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
> -		ret = dwc3_host_init(dwc);
> -		if (ret) {
> -			dev_err(dwc->dev, "failed to initialize host\n");
> -			return ret;
> -		}
> -
> +		/* We don't support dual-role so restrict to Device mode */
> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);

But is this acceptable? Why not default to Host? I went through a series
for defaulting OTG to Device for the Cadence Controller which was
objected at:
https://lore.kernel.org/r/62c5fe13-7f0e-e736-273e-31a8bbddbf13@kernel.org/
with the suggestion to use the cable to determine the role.

Regards,
Siddharth.


More information about the U-Boot mailing list