Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role

[Bin Liu]

On Fri, Mar 22, 2019 at 02:09:53PM +0100, Maxime Ripard wrote:
> On Fri, Mar 22, 2019 at 07:46:22AM -0500, Bin Liu wrote:
> > On Thu, Mar 21, 2019 at 05:41:38PM +0100, Greg Kroah-Hartman wrote:
> > > On Thu, Mar 21, 2019 at 02:01:33PM +0100, Maxime Ripard wrote:
> > > > Hi,
> > > >
> > > > I'm reviving this thread a bit, because I encountered this bug today.
> > > >
> > > > On Thu, Mar 21, 2019 at 11:02:10AM +0100, Bin Liu wrote:
> > > > > On Sat, Apr 21, 2018 at 12:59:23PM +0200, Paul Kocialkowski wrote:
> > > > > > Hi,
> > > > > >
> > > > > > Le vendredi 20 avril 2018 à 09:25 -0500, Bin Liu a écrit :
> > > > > > > On Thu, Mar 29, 2018 at 01:57:24PM +0200, Paul Kocialkowski wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On Thu, 2018-03-29 at 11:23 +0200, Maxime Ripard wrote:
> > > > > > > > > On Wed, Mar 28, 2018 at 11:52:13PM +0200, Paul Kocialkowski wrote:
> > > > > > > > > > This allows dual-role ports to be reported as having gadget mode
> > > > > > > > > > by
> > > > > > > > > > the
> > > > > > > > > > musb_has_gadget helper. This is required to enable MUSB at all
> > > > > > > > > > with
> > > > > > > > > > MUSB
> > > > > > > > > > glue layers that set the port mode to MUSB_PORT_MODE_DUAL_ROLE
> > > > > > > > > > at
> > > > > > > > > > init.
> > > > > > > > > >
> > > > > > > > > > Most notably, this allows calling musb_start when needed in the
> > > > > > > > > > virtual
> > > > > > > > > > MUSB root HUB, regardless of whether the current mode should be
> > > > > > > > > > gadget
> > > > > > > > > > or host.
> > > > > > > > > >
> > > > > > > > > > This fixes USB OTG on Allwinner devices that I could test it
> > > > > > > > > > with,
> > > > > > > > > > mainly A20 devices.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Paul Kocialkowski <contact@xxxxxxxx>
> > > > > > > > >
> > > > > > > > > Surely there's more to it than that. The gadget mode of A20 boards
> > > > > > > > > have been working in the past, including when compiling with mUSB
> > > > > > > > > setup as dual role.
> > > > > > > > >
> > > > > > > > > Is this a regression since a particular commit? Or is there
> > > > > > > > > another,
> > > > > > > > > deeper issue overlooked in the commit log?
> > > > > > > >
> > > > > > > > The root of the issue here is that musb_start is not called at any
> > > > > > > > point
> > > > > > > > without this patch. My understanding of the flow is the following:
> > > > > > > > when
> > > > > > > > the PHY detects that there was a VBUS/ID change, it will notify its
> > > > > > > > listeners (mainly the musb sunxi glue layer). This will then
> > > > > > > > schedule
> > > > > > > > the driver's work (sunxi_musb_work), which does nothing since the
> > > > > > > > SUNXI_MUSB_FL_ENABLED bit was never set. This bit is only set after
> > > > > > > > calling sunxi_musb_enable, which is called from
> > > > > > > > musb_platform_enable,
> > > > > > > > that originates from musb_start.
> > > > > > > >
> > > > > > > > Currently I see two places where musb_start is called:
> > > > > > > > * musb_virthub
> > > > > > > > * musb_gadget
> > > > > > > >
> > > > > > > > In the latter case, it is in turn called from udc_start, which
> > > > > > > > should
> > > > > > > > probably (correct me if I'm wrong) happen later in the call chain
> > > > > > > > than
> > > > > > > > ID/VBUS change notification time.
> > > > > > >
> > > > > > > I don't think it is correct that udc_start() is triggered by ID/VBUS
> > > > > > > events, but I don't have an Allwinner platform to verify the callflow.
> > > > > >
> > > > > > Yes you're right, I didn't make myself very clear here. I didn't
> > > > > > investigate the udc_start call path much since it was apparently not the
> > > > > > culprit.
> > > > > >
> > > > > > > Have you tried to load with a gadget driver? When a gadget function is
> > > > > > > bound to UDC, udc_start() is triggered, which in turn calls
> > > > > > > musb_start().
> > > > > >
> > > > > > It does work under that scenario, although my used case here is using
> > > > > > musb with DUAL_ROLE but no gadget driver loaded. That it, I want the
> > > > > > musb_start call to originate from the virtual hub, not from the gadget
> > > > > > side.
> > > > > >
> > > > > > > > In the former case, musb_start is called in the root controller hub
> > > > > > > > control, when setting the USB_PORT_FEAT_POWER feature. This looks
> > > > > > > > perfectly legit and IMO this is where it should be initially calling
> > > > > > > > musb_start in the dual role case. The kernel is indeed setting the
> > > > > > >
> > > > > > > No actually. A dual-role port should be in b_idle state by default, so
> > > > > > > logically all actions should go to the gadget path until the port
> > > > > > > switches to host mode.
> > > > > >
> > > > > > It makes sense that the port should be in b_idle state by default, but
> > > > > > here it fails to switch to host mode when the ID pin detects that it
> > > > > > should. Or does b_idle state entail that a gadget must be loaded (per
> > > > > > the USB spec), and thus nothing should (ever) happen until that happens?
> > > > > >
> > > > > > I find it really odd to need a gadget device to trigger host mode.
> > > > > > This patch does fix the issue, but I am puzzled as to why it is needed
> > > > > > in the first place. The comment above it mentions that "In OTG mode we
> > > > > > have to wait until we loaded a gadget. We don't really need a gadget if
> > > > > > we operate as a host but we should not start a session as a device
> > > > > > without a gadget or else we explode.", which is apparently compatible
> > > > > > with my use case: a gadget is not really needed and I'm not trying to
> > > > > > start a session as a device without a gadget loaded.
> > > > > >
> > > > > > What do you think?
> > > > >
> > > > > Okay, this came down to an argument that whether we should require
> > > > > loading a gadget driver on a dual-role port to work in host mode,
> > > > > which is currently required on musb since a long long time ago.
> > > > >
> > > > > I understand the requirement is kinda unnecessary, but since it already
> > > > > exists on musb stack for a long time, I don't plan to change it. Because I
> > > > > cannot think of a use case in real products that doesn't automatically
> > > > > load a gadget function on the dual-role port.
> > > > >
> > > > > If you can explain a use case in real world (not a engineering lab) that
> > > > > the gadget driver will not be loaded at linux booting up, but later
> > > > > based on user's input, I will reconsider my decision. To remove this
> > > > > requirement from musb stack, the work is more than this patch.
> > > >
> > > > I have one for you: we're working on a device that boots pretty fast,
> > > > and therefore are pushing as much things as we can to modules. It
> > > > includes gadgets, the musb driver and glue, etc. That doesn't sound
> > > > way very different from what a generic distro would do as well.
> > > >
> > > > At boot, the various modules for the hardware are loaded
> > > > automatically: the musb glue, the musb core, our USB PHY, etc. We end
> > > > up in a situation where the musb driver is loaded and reported to work
> > > > properly. The USB cable to the OTG port (in peripheral) might or might
> > > > not be connected, it's kind of irrelevant.
> > > >
> > > > The gadgets, however, are not loaded automatically.
> > > >
> > > > Now comes a user that wants to use musb as a host, and connect a
> > > > proper USB adapter, that wires the ID pin properly. In our case, the
> > > > phy detects it, reports the mode change, and .... nothing.
> > > >
> > > > That doesn't really look like an engineering lab setup to me.
> > >
> > > I agree, that sounds like a valid setup.
> > >
> > > Also realize that Android is pushing to have all drivers as modules, so
> > > you will start to see a whole lot more devices out there be modular
> > > instead of statically built kernels.  So issues like this are good to
> > > resolve :)
> >
> > This issue here is not related to building all drivers as modules. Today
> > we already have all musb related drivers including gadget drivers in
> > modules.
> >
> > The issue discussed here is that when musb is configured in dual-role
> > mode (dr_mode = 'otg' in dts), a gadget driver has to be bound to the
> > udc to make musb working in host mode.
> >
> > I never disagree it is not ideal, but I consider it is minor - since the
> > port is configured to dual-role mode, it is intended to work in
> > peripheral mode, then why not automatically load the gadget driver when
> > linux boots up.

Again, think about an embedded product, if dr_mode is 'otg' which
indicates the peripheral mode will be used at some point, when and how
to load the gadget driver if it is not loaded automatically when Linux
boots up? the end user doesn't have access to the console.

> Because no other controller requires it and therefore it's not
> standard and violates the principle of least surprise?

I know no other controller does this, but this doesn't mean it is not
standard.

> And even without taking this into account, there's also the fact that
> while the *hardware* can do dual role, the software might decide
> otherwise. If I don't want to have support for any gadget (at all) in
> the end system, then why should I be forced to compile and load
> something I don't even want to use in the first place?

then dr_mode should be set to 'host' instead, you don't have to load a
gadget if peripheral mode will never be used.

> The same story goes with cold booting with a device plugged in, and
> therefore acting as a host from the very beginning. It will never ever
> be configured as a peripheral, even though you might have loaded a
> module.

I don't understand the problem in this case - will the device ever be
removed? will the port ever be used in peripheral mode? If so, the
gadget driver will be needed. If not, dr_mode should be 'host' then a
gadget driver is not needed.

> > To summaries my comments on this again, since it is minor in my opinion,
> > I won't spend time to solve it myself (in a near future), but I am more
> > than happy to review and take any patch which solve it.
> 
> Paul provided a patch that started this discussion. You mentionned
> that it's not enough and other parts should be missing. What are
> those?

The previous discussion has been a while ago, but I reviewd the thread
again, at least I mentioned musb_start() will be called twice with this
patch, first by this patch, then when a gadget driver is bound.

Regards,
-Bin.