On Fri, Nov 29, 2024 at 02:28:36PM +0200, Roger Quadros wrote:
On 28/11/2024 19:20, Siddharth Vadapalli wrote:
[...]
USB DFU boot utilizes this driver when the "dr_mode" is set to "otg" in the device-tree for AM62A. This driver is invoked both for the host and device commands corresponding to USB. I came up with this patch while trying to enable USB DFU boot on AM62A with "dr_mode" as "otg" in the device-tree. Additionally, USB DFU boot works on AM62A with the change made by this patch and it doesn't work without this change. I have traced the call sequence to see that the driver is probed and have come up with the change performed in this patch on the basis of this observation.
Although the glue driver portion of the dwc3-generic driver is not used by AM62 it still uses the generic_probe() portion via U_BOOT_DRIVER(dwc3_generic_host) and U_BOOT_DRIVER(dwc3_generic_peripheral)
here is the dm tree output on am62-sk after executing "usb start" command.
simple_bus 5 [ + ] dwc3-am62 | |-- dwc3-usb@f900000 usb_gadget 0 [ ] dwc3-generic-periphe | | |-- usb@31000000 usb 0 [ + ] xhci-dwc3 | | `-- usb@31000000 usb_hub 0 [ + ] usb_hub | | `-- usb_hub simple_bus 6 [ + ] dwc3-am62 | |-- dwc3-usb@f910000 usb 1 [ ] dwc3-generic-host | | |-- usb@31100000 usb 1 [ + ] xhci-dwc3 | | `-- usb@31100000 usb_hub 1 [ + ] usb_hub | | `-- usb_hub
what is strange is that even though usb@31000000 is configured as "peripheral" (in k3-am625-sk-u-boot.dtsi), xhci-dwc3 and usb_hub drivers are still initialized for it.
CONFIG_OF_UPSTREAM is enabled in am62x_evm_a53_defconfig. So dr_mode might be "otg" as specified in: dts/upstream/src/arm64/ti/k3-am62-main.dtsi I'm not sure about this however. Additionally, this series isn't required for AM62. DFU Boot works on AM62 without this series as well. This series is required for AM62A.
[...]
I didn't test the case where both are used. I validated that peripheral mode works across various stages of boot with "otg" mode via USB DFU boot. This isn't the case without this patch.
If we forget the reason that this patch is introduced and take a moment to analyze the existing code prior to this patch, we see that:
- Both dwc3_generic_peripheral_probe() and dwc3_generic_host_probe()
invoke dwc3_generic_probe(). 2. dwc3_generic_probe() sets the role based on the role specified in the/ device-tree and not what the caller wants the role to be. This seems to be strange to me considering that dwc3_generic_probe() simply proceeds to configure without comparing the role supported by the platform with the role requested by the caller.
So even if we entirely forget the "otg" fix being attempted by this patch, the same patch is fixing something else i.e. ensuring that dwc3_generic_probe() does what the caller wants it to do by using the role that the caller expects. Isn't it incorrect that the existing code might end up with: a) Caller being dwc3_generic_host_probe() with dr_mode being set to "peripheral" within dwc3_generic_probe() based on the device-tree and proceeding to configure the controller
Maye this is the reason why host controller is initialized for peripheral only port in the dm tree output I listed above. This needs to be fixed.
Sure, I will post a patch to do the following: If dr_mode specified in device-tree is "host" then return -EINVAL within dwc3_generic_probe() if the caller is "dwc3_generic_peripheral_probe()". Similarly, if dr_mode specified in the device-tree is "peripheral" then return -EINVAL within dwc3_generic_probe() if the caller is "dwc3_generic_host_probe()". When dr_mode is specified as "otg" in the device-tree, then configure the mode based on the caller of dwc3_generic_probe().
Please let me know if the above implementation is acceptable.
b) Caller being dwc3_generic_peripheral_probe() with dr_mode being set to "host" within dwc3_generic_probe() based on the device-tree and proceeding to configure the controller.
This too needs to be fixed.
Shouldn't this inconsistency be fixed? This patch fixes that by setting dr_mode to what the caller expects. However, based on your suggestion, I agree that there should have been a check within dwc3_generic_probe() to ensure that the platform supports what the caller is requesting. I will update this patch by adding the check.
Apart from the platform supported mode check when not "otg", if platform supports "otg" we need to check and prevent role setting if hardware is not in the correct state based on connected USB cable. i.e. if host mode cable is used then prevent it from running as a peripheral. if peripheral mode cable is used then prevent it from running as a host. (based on ID status?)
Yes, I agree that using the ID status to determine the role is accurate. At the same time, I think that in addition to this, if the user runs a command corresponding to "peripheral" mode of operation with the ID status indicating "host", then it will be better to take this inconsistency into account and error out rather than simply proceeding to configure on the basis of the ID status. If you have any feedback, kindly let me know.
Regards, Siddharth.