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

[Siddharth Vadapalli]

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.

[...]

> >>> 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
> 
> 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?

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