[PATCH v2 1/2] usb: dwc3-generic: set "mode" based on caller of dwc3_generic_probe()
Roger Quadros
rogerq at kernel.org
Wed Dec 4 08:32:23 CET 2024
On 04/12/2024 07:16, Siddharth Vadapalli wrote:
> 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().
>
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.
>
>>
>>> {
>>> 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);
ret = dwc3_gadget_init(dwc);
if (ret) {
dev_err(dwc->dev, "failed to initialize gadget\n");
> 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.
--
cheers,
-roger
More information about the U-Boot
mailing list