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

[Siddharth Vadapalli]

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.