Message-ID: <AM4PR04MB213087AEEAFE9D26E19CC145895E0@AM4PR04MB2130.eurprd04.prod.outlook.com>
Date:	Wed, 8 Jun 2016 03:04:41 +0000
From:	Jun Li <jun.li@....com>
To:	Felipe Balbi <felipe.balbi@...ux.intel.com>,
	Roger Quadros <rogerq@...com>,
	Lu Baolu <baolu.lu@...ux.intel.com>,
	Peter Chen <hzpeterchen@...il.com>
CC:	Mathias Nyman <mathias.nyman@...el.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Lee Jones <lee.jones@...aro.org>,
	"Heikki Krogerus" <heikki.krogerus@...ux.intel.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v10 2/7] usb: mux: add generic code for dual role port mux

Hi,
> -----Original Message-----
> From: Felipe Balbi [mailto:felipe.balbi@...ux.intel.com]
> Sent: Tuesday, June 07, 2016 11:05 PM
> To: Roger Quadros <rogerq@...com>; Lu Baolu <baolu.lu@...ux.intel.com>;
> Jun Li <jun.li@....com>; Peter Chen <hzpeterchen@...il.com>
> Cc: Mathias Nyman <mathias.nyman@...el.com>; Greg Kroah-Hartman
> <gregkh@...uxfoundation.org>; Lee Jones <lee.jones@...aro.org>; Heikki
> Krogerus <heikki.krogerus@...ux.intel.com>; Liam Girdwood
> <lgirdwood@...il.com>; Mark Brown <broonie@...nel.org>; linux-
> usb@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v10 2/7] usb: mux: add generic code for dual role port
> mux
> 
> 
> Hi,
> 
> Roger Quadros <rogerq@...com> 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.

In many cases the role decision is made by usersapce, this also should be
covered.
This patchset also expose it to userspace but I think it isn't for debug:
/sys/bus/platform/devices/.../portmux.N/state

Li Jun
> 
> If we're discussing userspace ABI here, there's something very wrong with
> OTG/DRD layer design.
> 
> >>> 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?
> 
> 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.
> 
> > 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.
> 
> > 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.
> 
> > If so then who will deal with start/stop of the controllers then?
> 
> dwc3 itself.
> 
> --
> balbi

Powered by blists - more mailing lists