On 04/12/2024 12:05, Siddharth Vadapalli wrote:
On Wed, Dec 04, 2024 at 11:57:14AM +0200, Roger Quadros wrote:
On 04/12/2024 09:47, Siddharth Vadapalli wrote:
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.
We do want dwc3_generic_peripheral_boot() to be invoked with dr_mode is "otg". As we still do not support dual-role switching in u-boot.
And you need that for DFU as well. Did I miss something?
You were mentioning that "dwc3_generic_host_probe()" will not be called, but that is unrelated to the issue. I was pointing out that "dwc3_generic_peripheral_probe()" is invoked, with plat->dr_mode set to "otg" which currently results in PRTCAPDIR set to 11b which results in the issue. So the patch aims to address this by setting mode based on caller rather than using what is mentioned in the device-tree. This should be in sync when "dr_mode" is either "host"/"peripheral", but in the case of "otg", we run into issues which is fixed by this patch.
[...]
- 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
I will let others chime in if it is acceptable or not.
Why not default to host?
Because we don't bind the generic_host driver at all when dr_mode = "otg". So XHCI driver will not be probed and host mode will not function.
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.
dual-role is a new feature for this driver in u-boot which can be added in the future if needed. The current problem is to get DFU/device mode to work when dr_mode is "otg".
As the dwc3-generic driver only registers the device controller when dr_mode is "otg" I don't see any other neater way to fix it than to limit the controller to device mode if dr_mode is "otg".
Why not do what this patch does? What is the issue with the current patch (apart from the checks, which I could retain as well if others wish so)? The caller clearly mentions the expected role, so why not use that?
Because the patch is more complicated than it needs to be, now that we know: 1) generic_host_probe() will only be called when dr_mode is "host", 2) generic_peripheral_probe() will only be called when dr_mode is "peripheral" or "otg".
The problem case is generic_peripheral_probe() being called with when dr_mode is "otg".
Another alternative could be to override dr_mode to "peripheral" if it was "otg" in dwc3_generic_probe before calling dwc3_init() but I don't prefer that as we are loosing platform information here and it will have to be reverted when dual-role support is added in core.c.
So better to deal with this in core.c and add a note why we are doing it.
The only only other driver calling dwc3_init() is dwc3-layerscape.c. whose commit log states "OTG mode is not supported yet. The dr_mode in the devicetree will either have to be set to peripheral or host."
Or you can go all the way and get the dual-role mode working the right way where you check user request along with cable status and allow/prevent a certain role.
The current patch seems to be an intermediate solution (actually, it might be what should have been done when the code was first introduced itself. Why use plat->dr_mode at all, except maybe to check if the platform supports what is requested by the caller?)
Regards, Siddharth.