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:
- 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.