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

[Siddharth Vadapalli]

On Tue, Dec 03, 2024 at 09:23:11PM +0200, Roger Quadros wrote:

Hello Roger,

> Hi Siddharth,
> 
> 
> On 03/12/2024 11:37, Siddharth Vadapalli wrote:
> > There are only two callers of "dwc3_generic_probe()", namely:
> > 1. dwc3_generic_peripheral_probe()
> > 2. dwc3_generic_host_probe()
> > Currently, the "mode" is set based on the device-tree node of the
> > platform device. Also, the DWC3 core doesn't support updating the "mode"
> > dynamically at runtime if it is set to "OTG", i.e. "OTG" is treated as a
> > separate mode in itself, rather than being treated as a mode which should
> > eventually lead to "host"/"peripheral".
> 
> Actually this is not entirely true. Sorry for not catching this earlier.
> I did a deep dive today to debug why XHCI is being registered for both ports on am62.
> 
> I see that configs/am62x_a53_usbdfu.config is setting CONFIG_USB_XHCI_DWC3.
> This is wrong. It should not be used for AM62x platform.

The issue however is at R5 SPL stage and not A53 SPL/U-Boot.
configs/am62x_r5_usbdfu.config doesn't have CONFIG_USB_XHCI_DWC3
and I run into the issue where the "alt_info" for tispl and u-boot doesn't
show up when tiboot3.bin built for USB DFU boot is sent using dfu-util.

Please test USB DFU boot on AM625-SK with dr_mode set to "otg" in the
device-tree. That should help you recreate the issue that I am observing
with AM62A7-SK.

> 
> Also.
> 
> Please see dwc3_glue_bind_common().
> 
>         if (CONFIG_IS_ENABLED(DM_USB_GADGET) &&
>             (dr_mode == USB_DR_MODE_PERIPHERAL || dr_mode == USB_DR_MODE_OTG)) {
>                 printf("%s: dr_mode: OTG or Peripheral\n", __func__);
>                 driver = "dwc3-generic-peripheral";
>         } else if (CONFIG_IS_ENABLED(USB_HOST) && dr_mode == USB_DR_MODE_HOST) {
>                 printf("%s: dr_mode: HOST\n", __func__);
>                 driver = "dwc3-generic-host";
>         } else {
>                 printf("%s: unsupported dr_mode %d\n", __func__, dr_mode);
>                 return -ENODEV;
>         }
> 
>         ret = device_bind_driver_to_node(parent, driver, name,
>                                          node, &dev);
>         if (ret) {
> 
> 
> So if dr_mode is "otg" it will be bound to dwc3-generic-peripheral driver only.
> I verified that even if I change dr_mode from "peripheral" to "otg" DFU mode
> does work from u-boot shell.
> 
> Which makes my suggestions for checking dr_mode during and your patch seem unnecessary.
> 
> What is not clear though is, why is DFU mode not working for you. At which stage
> DFU is failing? R5 SPL/A53 SPL?

R5 SPL.

> 
> Can you please try disabling CONFIG_USB_XHCI_DWC3 in all configs as it will
> hijack the controller exclusively for host mode.

It is already disabled as mentioned above.

> 
> > 
> > Given that the callers of "dwc3_generic_probe()" clarify the expected
> > "mode" of the USB Controller, use that "mode" instead of the one
> > specified in the device-tree. This shall allow the USB Controller to
> > function both as a "Host" and as a "Peripheral" when the "mode" is "otg"
> > in the device-tree, based on the caller of "dwc3_generic_probe()".
> > 
> > Ideally, the USB ID pin should be used to determine whether or not the
> > requested role can be enabled. However, that can be implemented in the
> > future as an incremental feature over the current implementation.
> > 
> > Signed-off-by: Siddharth Vadapalli <s-vadapalli at ti.com>
> > ---
> > 
> > v1:
> > https://patchwork.ozlabs.org/project/uboot/list/?series=434253&state=%2A&archive=both
> > Changes since v1:
> > - Based on the feedback received on the v1 series, the device-tree
> >   specified role is also taken into account in dwc3_generic_probe(), in
> >   addition to the caller of dwc3_generic_probe().
> > 
> >  drivers/usb/dwc3/dwc3-generic.c | 21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
> > index 2ab41cbae45..fe98b50c42c 100644
> > --- a/drivers/usb/dwc3/dwc3-generic.c
> > +++ 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().


> 
> >  {
> >  	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
to:
...set the role on the basis of the caller (intended role).
Device-tree will no longer be used i.e. plat->dr_mode shouldn't be used.

Regards,
Siddharth.