Re: [PATCH v4 2/7] usb: dwc3: Switch to device mode on gadget start

[Roger Quadros <rogerq@kernel.org>]

On 12/01/2024 16:18, Sjoerd Simons wrote:
> On Fri, 2024-01-12 at 14:56 +0200, Roger Quadros wrote:
>>
>>
>> On 12/01/2024 13:06, Sjoerd Simons wrote:
>>> On Fri, 2024-01-12 at 12:39 +0200, Roger Quadros wrote:
>>>>
>>>>
>>>> On 12/01/2024 10:52, Sjoerd Simons wrote:
>>>>> When dr_mode is "otg" the dwc3 is initially configured in _OTG
>>>>> mode;
>>>>> However in this mode the gadget functionality doesn't work
>>>>> without
>>>>> further configuration. To resolve that on gadget start switch
>>>>> to
>>>>> _DEVICE
>>>>> mode globally and go back to _OTG on stop again.
>>>>>
>>>>> For this the dwc3_set_mode is renamed to dwc3_core_set_mode to
>>>>> avoid a
>>>>> conflict with the same function exposed by xhci-dwc3
>>>>
>>>> Aren't they both doing the same thing? I'd rather define them at
>>>> one
>>>> place
>>>> i.e. dwc3/core.c.
>>>
>>> They twiddle the same registers afaict indeed; but the way to get
>>> there
>>> is rather different. So having just one for both really needs a
>>> bigger
>>> amount of rework.
>>>
>>>> The driver model design for dwc3 looks really broken in u-boot.
>>>>
>>>> "snps,dwc3" is claimed by xhci-dwc3.c instead of being claimed by
>>>> dwc3/core.c.
>>>> This is probably because at the time USB host was the more needed
>>>> use
>>>> case at u-boot.
>>>>
>>>> Ideally dwc3/core.c should claim "snps,dwc3" device and
>>>> instantiate
>>>> the respective Host/Device/OTG device based on the provided otg
>>>> mode.
>>>>
>>>> For Host/Device mode it is straight forwa
>>>> dwc3/core.c does dwc3_set_mode() depending on the mode and:
>>>> for host mode -> register xhci-usb driver.
>>>> for device mode -> register UDC device driver.
>>>>
>>>> For dual-role mode, the solution is not very clear as I don't
>>>> think
>>>> there is a role switch framework present
>>>>
>>>> To begin with, we could probably restrict it to just device mode
>>>> as most platforms were forcing it to device mode in any case as
>>>> they
>>>> usually have a 2nd USB port that is host only.
>>>
>>> Right we don't have role switching; And if we had we'd have to add
>>> support of the Type-C controller on the SK boards which determines
>>> the
>>> roles for this.
>>>
>>> This one more minimal patch was modelled after the discussion last
>>> year
>>> around otg mode[0]. And similar to how the cdns3 driver handles it.
>>> Essentially letting host/gadget mode be determined by the usage
>>> rather
>>> then the cables plugged, which is not pretty but works.
>>>
>>> While I agree with you this could all be a lot nicer of u-boot did
>>> "proper" OTG/role-switching; I unfortunately don't have the
>>> bandwidth
>>> to introduce all of that and it seems a shame to block DFU support
>>> on
>>> it. For reference my previous series just set the peripheral role
>>> in
>>> the dts which reflects your suggestion of forcing device mode. I'd
>>> be
>>> wary to do so in the driver because i would hope the OTG handling
>>> in
>>> dwc3 is there for a reason :) 
>>>
>>>
>>> 0: https://lists.denx.de/pipermail/u-boot/2023-August/527709.html
>>>
>>>
>>
>> In your current series, how does it work?
>> What happens if user starts both host and device controllers?
> 
> Testing host functionality on the same port as used for DFU (the only
> one that's otg) doesn't seem to work at all, not sure why that is. host
> on usb1 (usb-a port, host only) works fine ofcourse.
> 
> practically however once the user has used a gadget it's likely they'd
> have to reset the host side (or at least do a rescan) though as likely
> they'll have also switched around the cable :).
> 
>>
>> e.g.
>>> usb start
>>> dfu 0
> 
> That seems to work fine, modulo usb0 not seeing attached devices, but
> that's not a regression introduced by this patch.

am62-sk USB0 was never tested in host mode at u-boot. Let me try out and see
if I can get it to work.

-- 
cheers,
-roger