Message-ID: <20180421143426.GA10632@LTA0271908.dhcp.ti.com>
Date:   Thu, 21 Mar 2019 11:02:10 +0100
From:   Bin Liu <b-liu@...com>
To:     Paul Kocialkowski <contact@...lk.fr>
CC:     Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
        Maxime Ripard <maxime.ripard@...tlin.com>,
        <linux-kernel@...r.kernel.org>, <linux-usb@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Chen-Yu Tsai <wens@...e.org>
Subject: Re: [PATCH] usb: musb: Support gadget mode when the port is set to
 dual role

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@...lk.fr>
> > > > 
> > > > 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.

Regards,
-Bin.

Powered by blists - more mailing lists