Re: [PATCH v10 2/7] usb: mux: add generic code for dual role port mux

[Roger Quadros]

On 07/06/16 18:05, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@xxxxxx> writes:
>>> I might be able to find some time to implement a proof of concept which
>>> would allow your platforms to get dual-role with code we already have,
>>> but I need DWC3's OTG support which, I'm assuming, you already have :-)
>>>
>>> If you wanna try something offline, just ping me ;-) I'll be happy to
>>> help.
>>
>> What you are proposing is a dwc3 only solution. With the otg/dual-role
>> series we are trying to be generic as much as possible.
> 
> Well, if there is a need for that, sure. Take MUSB for instance. It
> makes use of nothing of the sorts, because it doesn't have to.
> 
>> Whether controller drivers want to use it or not is upto the driver
>> maintainers but we should at least ensure that user space ABI if any,
>> is consistent across different implementations.
> 
> Role decisions should not be exposed to userspace unless as debug
> feature (using e.g. DebugFS). That should be done either by the HW or
> within the kernel.
> 
> If we're discussing userspace ABI here, there's something very wrong
> with OTG/DRD layer design.

Not really. There can be a need for user space application to control the
port role. Consider Apple carplay for instance or even full OTG support
which has user selectable role.

> 
>>>> How are you switching the port mux between host and peripheral? Only
>>>> by sysfs or do you have a GPIO for ID pin as well?
>>>
>>> depends. Some SoCs have GPIO-controller muxes while some just have mux's
>>> select signals (one for ID, one for VBUS) mapped on xHCI's address
>>> space.
>>>
>>>> What happens to the gadget controller when the port is muxed to the
>>>> host controller?  Is it stopped or it continues to run?
>>>
>>> it continues running, but that's pretty irrelevant for Intel's dual-role
>>
>> Isn't that unnecessary waste of power? Or you have firmware assisted
>> low power mode?
> 
> that's an implementation detail which brings nothing to this discussion,
> right? :-)
> 
> We can, certainly, put the other side to D3.
> 
>>> setup. We have an actual physical (inside the die, though) mux which
>>> muxes USB signals to XHCI (not DWC3's XHCI) or to a peripheral-only
>>> DWC3.
>>>
>>
>> Probably irrelevant for Intel's dual-role but many platforms that
>> share the port can't have device controller running when port is in
>> host mode and vice versa.
> 
> but that doesn't mean we need an entire new layer added to the kernel
> ;-)
> 
> DWC3 already gives us all the information necessary to make a decision
> on which role we should assume. Just consider your options. Here's how
> things would look like without any OTG/DRD layer:
> 
> -> DWC3 OTG IRQ
>  -> readl(OSTS);
>   -> if (OSTS & BIT(4))
>    -> dwc3_host_exit(); __dwc3_gadget_start();
>   -> else
>    -> __dwc3_gadget_stop(); dwc3_host_init();
> 
> Can you draw something similar for your proposed OTG/DRD layer?

What about B_IDLE state? We don't want either peripheral or host to run when no cable
is inserted.

Have you tested if it works? I'd be happy to test if you can prepare a patch
to get dual-role working on dwc3 without the OTG/DRD layer :).

> 
> I remember there were at least two schedule_work(). IIRC it looked
> something like below:
> 
> -> DWC3 OTG IRQ
>  -> readl(OSTS);
>   -> if (OSTS & BIT(4))
>    -> otg_set_mode(PERIPHERAL);
>     -> schedule_work();
>      -> otg_ops->stop_host();
>       -> usb_del_hcd();
>      -> otg_ops->start_peripheral();
>       -> usb_gadget_add_udc();
>   -> else
>    -> otg_set_mode(HOST);
>     -> schedule_work();
>      -> otg_ops->stop_peripheral();
>       -> usb_gadget_del_udc();
>      -> otg_ops->start_host();
>       -> usb_add_hcd();
> 
> I'm probably missing some steps there.    

As a user you just need to do this

        reg = read(OSTS);
        dwc->otg->fsm.id = !!(reg & STS_ID);
        dwc->otg->fsm.b_sess_vld = !!(reg &STS_BSESVLD);
        usb_otg_sync_inputs(dwc->otg);

And the layer does the rest.

But as I said earlier. I have absolutely no issues if dwc3 doesn't use that layer
as long as dual-role works on our platforms.

> 
>> So there has to be a central point of control where the respective
>> controllers are started/stopped.
> 
> some implementations might need this, yes. DWC3 and MUSB don't seem to
> be this type of system.

OK.
> 
>> That is the other point we are trying to address with the common
>> otg/dual-role code.
>>
>> Even in the TI dwc3 implementation we use dwc3's XHCI so I guess we need
>> to stop the host controller for device mode, right?
> 
> yes, see above. We already have that code.

Just code is not enough. We need to know if it works :).

> 
>> If so then who will deal with start/stop of the controllers then?
> 
> dwc3 itself.
> 
OK.

cheers,
-roger

Attachment: signature.asc
Description: OpenPGP digital signature