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-31a8bbddb...@kernel.org/ with the suggestion to use the cable to determine the role. Regards, Siddharth.