Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Roger Quadros]

On 29/03/17 13:32, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@xxxxxx> writes:
>>> Roger Quadros <rogerq@xxxxxx> writes:
>>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
>>>> when we're operating in dual-role.
>>>
>>> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no
>>> USB3 when OTGv2 was written.
>>>
>>> DRA7 just shouldn't use OTG core altogether. In fact, this is the very
>>> thing I've been saying for a long time. Make the simplest implementation
>>> possible. The dead simple, does-one-thing-only sort of implementation.
>>>
>>> All we need for Dual-Role (without OTG extras) is some input for ID and
>>> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.
>>>
>>
>> The catch is that on AM437x there is no way to get ID and VBUS events other
>> than the OTG controller so we have to rely on the OTG controller for that. :(
> 
> okay, so AM437x can get OTG interrupts properly. That's fine. We can
> still do everything we need using code that's already existing in dwc3
> if we refactor it a bit and hook it up to the OTG IRQ handler.
> 
> Here's what we do:
> 
> * First we re-factor all necessary code around so the API for OTG/DRD
>   is resumed to calling:
> 
> 	dwc3_add_udc(dwc);
>         dwc3_del_udc(dwc);
>         dwc3_add_hcd(dwc);
>         dwc3_del_hcd(dwc);
> 
> the semantics of these should be easy to understand and you can
> implement each in their respective host.c/gadget.c files.
> 
> * Second step is to modify our dwc3_init_mode() (or whatever that
>   function was called, sorry, didn't check) to make sure we have
>   something like:
> 
> 	case OTG:
>         	dwc3_add_udc(dwc);
>                 break;
> 
> We should *not* add HCD in this case yet.
> 
> * After that we add otg.c (or drd.c, no preference) and make that call
>   dwc3_add_udc(dwc) and, also, provide
>   dwc3_add_otg(dwc)/dwc3_del_otg(dwc) calls. Then patch the switch
>   statement above to:
> 
> 	case OTG:
>         	dwc3_add_otg(dwc);
>                 break;
> 
> Note that at this point, this is simply a direct replacement of
> dwc3_add_udc() to dwc3_add_otg(). This should maintain current behavior
> (which is starting with peripheral mode by default), but it should also
> add support for OTG interrupts to change the mode (from an interrupt
> thead)
> 
> 	otg_isr()
>         {
> 
> 		/* don't forget to remove preivous mode if necessary */
>         	if (perimode)
>                 	dwc3_add_udc(dwc);
>                 else
>                 	dwc3_add_hcd(dwc);
> 	}
> 
> * The next patch would be to choose default conditionally based on
>   PERIMODE or whatever.
> 
> Of course, this is an oversimplified view of reality. You still need to
> poke around at PRTCAPDIR, etc. But all this can, actually, be prototyped
> using our "mode" debugfs file. Just make that call
> dwc3_add/del_udc/hcd() apart from fiddling with PRTCAPDIR in GCTL.
> 
> Your first implementation could be just that. Refactoring what needs to
> be refactored, then patching "mode" debugfs to work properly in that
> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because
> then you know what needs to be taken into consideration.
> 
> Just to be clear, I'm not saying we should *ONLY* get the debugfs
> interface for v4.12, I'm saying you should start with that and get that
> stable and working properly (make an infinite loop constantly changing
> modes and keep it running over the weekend) before you add support for
> OTG interrupts, which could come in the same series ;-)
> 

Agree with you. Moreover I could get rid of OTG controller related code
and have just debugfs and extcon implementation. We can add the OTG controller
bits later.

I agree with you on everything you said except using add/del_gadget_udc. :)
I've explained why we can't use del_gadget_udc in the other thread
but I'll explain it here again.

1) If we start in host role, usb_add_gadget_udc() won't be called. That means
no UDC and user can't load a gadget driver. Typical applications need to have
a gadget driver ready *before* the peripheral mode starts so that it can
enumerate immediately.

2) If we use usb_del_gadget_udc() when switching to host mode and
usb_add_gadget_udc() when switching back to peripheral mode, the previously
loaded gadget driver will not be assigned to this UDC. User has to unload
and reload the gadget driver.

3) All this becomes even more complex for configfs based gadget driver.

So using stop/start gadget is a much simpler solution really as UDC software
side of things remain unchanged and the gadget driver can persist between
role switches.

cheers,
-roger