On Wed, Dec 04, 2024 at 04:36:53PM +0200, Roger Quadros wrote: > > > On 04/12/2024 15:37, Siddharth Vadapalli wrote: > > On Wed, Dec 04, 2024 at 02:34:56PM +0200, Roger Quadros wrote: > >> > >> > >> On 04/12/2024 12:05, Siddharth Vadapalli wrote: > > > > [...] > > > >>> 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". > > > > We can also be sure that generic_periperhal_probe() expects dr_mode to > > be "peripheral" only and not "otg". So why not fix it at the root, which > > generic_peripheral_probe() expects nothing from dr_mode. > It just expects the USB controller to work in peripheral mode.
Yes. So configuring the USB controller for peripheral mode of operation should be fine, in which case, irrespective of what "dr_mode" says, using "peripheral" should enable the desired functionality. > > dr_mode is just a reflection of platform/device-tree dr_mode. Let's keep it > that way and not abuse dr_mode to "peripheral" when it is actually "otg". I am not asking to change what the device-tree has. I am asking the need to use "otg" as specified in the device-tree when in U-Boot, we cannot switch between Host and Peripheral dynamically using "otg". At the end of the day, there is no "generic_otg_probe()" which could enable such functionality. So I fail to understand why configuring the controller for "peripheral" mode as requested by "generic_peripheral_probe()" is equivalent to abusing the device-tree. > > > is what this patch does? Independent of the issue, is the following a > > valid use-case? > > > > generic_peripheral_probe() with plat->dr_mode = "otg" and hence > > generic_probe() using "otg" rather than "peripheral" > > It is a valid use case. The Platform says that the port is "otg" > i.e. (plat->dr_mode ="otg") > and the user says he wants to use it as peripheral > i.e. (by invoking generic_peripheral_probe()) > Don't you see this as valid. Does peripheral mode work when user sets "otg" in the device-tree? Clearly it doesn't and hence this patch. Linux supports "otg" mode in the sense that we can switch between "host" and "peripheral". U-Boot on the other hand cannot even enable "peripheral" functionality when dr_mode is "otg" in the device-tree, let alone switching roles. > > > as is the case now. Is this resulting in something functional for other > > users of the USB Designware Controller? If the user requests "peripheral" > > mode of operation, will "otg" meet the user's requirement? If yes, I have > > no further objections. Irrespective of this, if you believe that the fix > > should be in the core as you indicated below, please feel free to post > > the patch. > > Let's look at this from the other way around. > Why do you not agree that the fix should be in core.c and abusing dr_mode > is the correct way to go? Isn't "otg" meant to convey that the role isn't fixed and can be switched? So how is hard-coding it to "peripheral", not considered as abusing the core? I don't intend to argue any further. I probably shouldn't have looked at other discussions on the mailing list regarding hard-coding "otg" as "peripheral" being incorrect. I should have simply posted the PRTCAPDIR fix in the core, instead of trying to come up with an acceptable fix. It is only because I felt that it didn't seem acceptable that I spent time finding out this inconsistency in "dwc3_generic_probe" where configuring the controller for "otg" mode as indicated by plat->dr_mode when the caller is "generic_peripheral_probe()" is somehow supposed to work in U-Boot where there is no OTG state machine. If the fix in the core is the right way to go, please post the patch. I only wanted to make USB DFU boot functional on AM62A and didn't want to end up in an argument while doing so. Regards, Siddharth.
