Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3389 bytes --]


Hi,

John Youn <John.Youn@synopsys.com> writes:
>>>>> As you mentioned this is handled in the soft_disconnect sysfs. Why
>>>>> shouldn't usb_gadget_disconnect() do the same thing, if not the gadget
>>>>
>>>> because there might be cases where we don't need/want the gadget to know
>>>> about this disconnect.
>>>>
>>>
>>> But what if we do?
>> 
>> well, if the gadget is the one faking a disconnect, then it ought to
>
> It's not a really "faking" since the dwc3 controller supports soft
> disconnect :)

It's "fake" in the sense that pins are still mated in the connector,
that's what I mean by fake ;)

>> cancel requests and do all the other necessary steps, right ? :-)
>> 
>
> It does take those steps whenever it's notified of disconnect via its
> disconnect callback. But since it doesn't get the callback when the
> disconnect happens, it has to call it explicitly. What I'm saying is

right, and that's the requirement. You're implementing it correctly.

> that the API or UDC is the one that should call it since it knows that
> the usb_gadget_disconnect() API was called and/or the UDC
> disconnected.

but udc-core doesn't know why it was called :) see
usb_function_activate()/deactivate(), we don't want to cancel requests
in that case, just delay the moment when host notices a port change.

>>>>> itself? Exposing the sysfs as an API function would work too. Though
>>>>
>>>> it already _is_ exported. I just don't know why people are re-inventing
>>>> the same solution :-)
>>>>
>>>>> both functions are "soft" disconnects and both are called
>>>>> "disconnect".
>>>>>
>>>>> In our gadget_driver we do the workaround where we notify ourself that
>>>>> we called the usb_gadget_disconnect() and that we should now be
>>>>
>>>> you could just rely on the sysfs interface, right ? :-)
>>>
>>> Not from the gadget driver, at least I don't think so. The gadget
>>> driver itself is the one that wants to initiate the soft disconnect.
>> 
>> I need to understand this requirement of yours a little better. Can you
>> describe exactly what your gadget is doing ? Also, any chance of showing
>> the code for that gadget ? I don't mind carrying an extra function
>> driver if it helps you validate your IP :-)
>> 
>
> This gadget driver does a programmable disconnect during testing. I
> don't think it will be released anytime soon. Which is why I never
> bothered to submit a fix. Also note that this isn't a function but a
> gadget driver (same place in the stack as libcomposite framework). I'm
> not sure if we have those anymore in the kernel.

We still have those, yes. But it seems like your stuff should be
integrated into composite.c itself under a #ifdef USB_GADGET_TESTING or
something like that.

If it helps IP providers using a real OS (linux, of course heh) for
silicon validation, I'd be very happy to integrate these
changes. There's a lot which we can still do to make Linux more
interesting for IP providers and SoC integrators (from validation point
of view), if this minimal change helps, we should do it :)

In fact, if you want to list out some extra requirements for dwc3's
trace functionality, I'd be happy to implement those. Are you missing
any visibility into some internal controller state ? Any sort of tooling
which you'd like to have ?

Let me know ;)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]