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

[Roger Quadros <rogerq@kernel.org>]

On 04/12/2024 17:00, Siddharth Vadapalli wrote:
> 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.

Sorry if you took this as an argument. I was only weighing in if there were
better alternatives.
If we have to go with your solution then your v1 series is better
than v2. I will give my Reviewed-by: there.

-- 
cheers,
-roger