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

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

[Paul Kocialkowski]

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@paulk.fr>
---
 drivers/usb/musb/musb_virthub.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
index 5165d2b07ade..8fd5c5f86e82 100644
--- a/drivers/usb/musb/musb_virthub.c
+++ b/drivers/usb/musb/musb_virthub.c
@@ -249,7 +249,8 @@ static int musb_has_gadget(struct musb *musb)
 #ifdef CONFIG_USB_MUSB_HOST
 	return 1;
 #else
-	return musb->port_mode == MUSB_PORT_MODE_HOST;
+	return musb->port_mode == MUSB_PORT_MODE_HOST ||
+	       musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE;
 #endif
 }
 
-- 
2.16.2

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]

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@paulk.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?

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 2723 bytes --]

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@paulk.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.

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
feature, only that it fails to enable musb without this patch.

First, I'd like to make sure that this understanding of the flow is
correct as I may have missed something here. Does it make sense?

Then, it seems that the offending commit is: be9d39881fc4f
("usb: musb: host: rely on port_mode to call musb_start()")

That itself fixed: ae44df2e21b5
("usb: musb: call musb_start() only once in OTG mode")

Still, this commit was authored in June 2015, so almost 3 years ago.
In the meantime, the sunxi driver has received feature improvements, so
it seems hard to believe that it was broken all this time...

Cheers,

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3357 bytes --]

Hi,

On Thu, Mar 29, 2018 at 01:57:24PM +0200, Paul Kocialkowski wrote:
> 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@paulk.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.
> 
> 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
> feature, only that it fails to enable musb without this patch.
> 
> First, I'd like to make sure that this understanding of the flow is
> correct as I may have missed something here. Does it make sense?
> 
> Then, it seems that the offending commit is: be9d39881fc4f
> ("usb: musb: host: rely on port_mode to call musb_start()")
> 
> That itself fixed: ae44df2e21b5
> ("usb: musb: call musb_start() only once in OTG mode")
> 
> Still, this commit was authored in June 2015, so almost 3 years ago.
> In the meantime, the sunxi driver has received feature improvements, so
> it seems hard to believe that it was broken all this time...

I'm not that knowlegdeable about the musb driver, so I can't really
comment on whether what you're saying actually makes sense, but from
what you seem to say, the issue is just happening upon VBUS / ID
notification.

Have you tested without the role switching? For example, trying to
boot while acting as a gadget?

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 4280 bytes --]

Hi,

Le mardi 03 avril 2018 à 11:29 +0200, Maxime Ripard a écrit :
> Hi,
> 
> On Thu, Mar 29, 2018 at 01:57:24PM +0200, Paul Kocialkowski wrote:
> > 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@paulk.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.
> > 
> > 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
> > feature, only that it fails to enable musb without this patch.
> > 
> > First, I'd like to make sure that this understanding of the flow is
> > correct as I may have missed something here. Does it make sense?
> > 
> > Then, it seems that the offending commit is: be9d39881fc4f
> > ("usb: musb: host: rely on port_mode to call musb_start()")
> > 
> > That itself fixed: ae44df2e21b5
> > ("usb: musb: call musb_start() only once in OTG mode")
> > 
> > Still, this commit was authored in June 2015, so almost 3 years ago.
> > In the meantime, the sunxi driver has received feature improvements,
> > so
> > it seems hard to believe that it was broken all this time...
> 
> I'm not that knowlegdeable about the musb driver, so I can't really
> comment on whether what you're saying actually makes sense, but from
> what you seem to say, the issue is just happening upon VBUS / ID
> notification.
> 
> Have you tested without the role switching? For example, trying to
> boot while acting as a gadget?

In the end, the issue here was that I did not have a gadget (e.g.
g_ether) driver loaded, and this prevented host mode form working
(according to my initial explanation).

With a gadget loaded, things work normally. Still, I don't think this is
the right behavior here: one should definitely be able to use USB OTG in
host mode without any gadget loaded.

My initial patch fixes this, but I am not confident that it doesn't
break other platforms, for which this assumption was added in the first
place.

Cheers!

-- 
Paul Kocialkowski,

developer of free digital technology and hardware support.

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Bin Liu]

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@paulk.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.

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().

> 
> 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.

> feature, only that it fails to enable musb without this patch.
> 
> First, I'd like to make sure that this understanding of the flow is
> correct as I may have missed something here. Does it make sense?

I am guessing you didn't load a gadget driver when testing?

> Then, it seems that the offending commit is: be9d39881fc4f
> ("usb: musb: host: rely on port_mode to call musb_start()")
> 
> That itself fixed: ae44df2e21b5
> ("usb: musb: call musb_start() only once in OTG mode")
> 
> Still, this commit was authored in June 2015, so almost 3 years ago.
> In the meantime, the sunxi driver has received feature improvements, so
> it seems hard to believe that it was broken all this time...
> 
> Cheers,

Regards,
-Bin.

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

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 5257 bytes --]

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@paulk.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?

> > feature, only that it fails to enable musb without this patch.
> > 
> > First, I'd like to make sure that this understanding of the flow is
> > correct as I may have missed something here. Does it make sense?
> 
> I am guessing you didn't load a gadget driver when testing?

Correct.

> > Then, it seems that the offending commit is: be9d39881fc4f
> > ("usb: musb: host: rely on port_mode to call musb_start()")
> > 
> > That itself fixed: ae44df2e21b5
> > ("usb: musb: call musb_start() only once in OTG mode")
> > 
> > Still, this commit was authored in June 2015, so almost 3 years ago.
> > In the meantime, the sunxi driver has received feature improvements,
> > so
> > it seems hard to believe that it was broken all this time...
> > 
> > Cheers,
> 
> Regards,
> -Bin.

Cheers,

-- 
Paul Kocialkowski,

developer of free digital technology and hardware support.

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Bin Liu]

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@paulk.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.

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

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 2118 bytes --]

Hi,

Le samedi 21 avril 2018 à 09:34 -0500, Bin Liu a écrit :
> 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.

My use case here is to support common GNU/Linux-based distributions, not
use-case-specific varieties of GNU/Linux-based rootfs. So my point here
would be that most distros will (and probably should) ship g_ether as a
module but without any particular reason to autoload it, or any other
gadget module in particular, since the system is general-purpose.

Then, imagine a user wants to plug a USB device through OTG (say,
because it's the only USB port available at all on the tablet they're
using), it simply won't work. It won't be obvious to that user that this
is because no gadget is loaded, since what they want to do does not
involve using gadget mode at any point.

Do you think this is a valid use case? It surely is a common one and
perfectly depicts my situation.

Note that in addition to Allwinner devices, I also have omap3/4/5
devices for testing things. I don't think I have other MUSB-enabled
devices in my collection though, but I would be willing to test fixes to
this issue on the ones I have.

Cheers,

-- 
Paul Kocialkowski,

developer of free digital technology and hardware support.

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Bin Liu]

On Mon, Apr 30, 2018 at 11:08:42PM +0200, Paul Kocialkowski wrote:
> Hi,
> 
> Le samedi 21 avril 2018 à 09:34 -0500, Bin Liu a écrit :
> > 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.
> 
> My use case here is to support common GNU/Linux-based distributions, not
> use-case-specific varieties of GNU/Linux-based rootfs. So my point here
> would be that most distros will (and probably should) ship g_ether as a
> module but without any particular reason to autoload it, or any other
> gadget module in particular, since the system is general-purpose.

This is the case I called it "in a engineering lab", not a real product.

> Then, imagine a user wants to plug a USB device through OTG (say,
> because it's the only USB port available at all on the tablet they're
> using), it simply won't work. It won't be obvious to that user that this
> is because no gadget is loaded, since what they want to do does not
> involve using gadget mode at any point.

If a tablet has a dual-role usb port, it is designed to use a gadget
driver, which has to be loaded at some point. In the case you described
above, when the gadget driver will be loaded? and how?

If a gadget driver will never be used, a host-only port should be on
the board, not a dual-role port.

> Do you think this is a valid use case? It surely is a common one and
> perfectly depicts my situation.

As I explained above, I don't think so.

> Note that in addition to Allwinner devices, I also have omap3/4/5
> devices for testing things. I don't think I have other MUSB-enabled

Much more than what I have ;)

> devices in my collection though, but I would be willing to test fixes to
> this issue on the ones I have.

Appreciated it, but someone has to make the patches first. The one you
posted might be a good start, but it is not complete. The first problem
I see is that musb_start() will be called twice, one in the place you
patched, the other is when the gadget driver is bound to the UDC.

Regards,
-Bin.

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

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 5112 bytes --]

Hi,

Le mardi 01 mai 2018 à 07:25 -0500, Bin Liu a écrit :
> On Mon, Apr 30, 2018 at 11:08:42PM +0200, Paul Kocialkowski wrote:
> > Hi,
> > 
> > Le samedi 21 avril 2018 à 09:34 -0500, Bin Liu a écrit :
> > > 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.
> > 
> > My use case here is to support common GNU/Linux-based distributions,
> > not
> > use-case-specific varieties of GNU/Linux-based rootfs. So my point
> > here
> > would be that most distros will (and probably should) ship g_ether
> > as a
> > module but without any particular reason to autoload it, or any
> > other
> > gadget module in particular, since the system is general-purpose.
> 
> This is the case I called it "in a engineering lab", not a real
> product.

To me, this sounds more like "daily use with upstream like on any
laptop/desktop" rather than an engineering lab, but that's not the main
point here.

> > Then, imagine a user wants to plug a USB device through OTG (say,
> > because it's the only USB port available at all on the tablet
> > they're
> > using), it simply won't work. It won't be obvious to that user that
> > this
> > is because no gadget is loaded, since what they want to do does not
> > involve using gadget mode at any point.
> 
> If a tablet has a dual-role usb port, it is designed to use a gadget
> driver,

I don't understand the logic behind this assertion. If it has a dual-
role USB port, then its hardware allows both use cases. It's obvious
that the use case is up to the user of the device since it can be
switched by software and is not fixed at design time.

>  which has to be loaded at some point. In the case you described
> above, when the gadget driver will be loaded? and how?

Again, loading a gadget driver is not part of the use case. In what I
described, the user only wants to use the dual-role port for its host
capability and does not care about gadget at all. When the device is
plugged into a host, it will simply charge and not propose any USB
device features.

> If a gadget driver will never be used, a host-only port should be on
> the board, not a dual-role port.

Here as well, I think the use case is separate from the hardware design.
 I crafted this patch because I was in the use case I described, with a
tablet that only features a micro B USB OTG port. The form factor simply
does not allow having a full USB A female host-only port.

> > Do you think this is a valid use case? It surely is a common one and
> > perfectly depicts my situation.
> 
> As I explained above, I don't think so.

I am really surprised that using regular upstream GNU/Linux
distributions out of the box is not a valid use case for the MUSB
driver. The situation I'm describing is exactly the same as buying a
laptop with a preinstalled OS and replacing it with a regular distro. In
my case, that's what I did with the tablet (that had an old Android
version that did expose gadget features via USB) and I installed
upstream Linux and a distro on it.

> > Note that in addition to Allwinner devices, I also have omap3/4/5
> > devices for testing things. I don't think I have other MUSB-enabled
> 
> Much more than what I have ;)
> 
> > devices in my collection though, but I would be willing to test
> > fixes to
> > this issue on the ones I have.
> 
> Appreciated it, but someone has to make the patches first. The one you
> posted might be a good start, but it is not complete. The first 
> problem

Oh, I am definitely up for making the changes as well, I mentioned
testing to show what level of test coverage I could bring to the table,
since this will probably require making sure that it doesn't break
specific platforms, glue layers, etc.

> I see is that musb_start() will be called twice, one in the place you
> patched, the other is when the gadget driver is bound to the UDC.

Okay, I will look into this and make sure there is only a single call to
musb_start in all scenarios. Are there other things that should be
modified as well?

Cheers,

-- 
Paul Kocialkowski,

developer of free digital technology and hardware support.

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Bin Liu]

On Tue, May 01, 2018 at 03:26:57PM +0200, Paul Kocialkowski wrote:
> Hi,
> 
> Le mardi 01 mai 2018 à 07:25 -0500, Bin Liu a écrit :
> > On Mon, Apr 30, 2018 at 11:08:42PM +0200, Paul Kocialkowski wrote:
> > > Hi,
> > > 
> > > Le samedi 21 avril 2018 à 09:34 -0500, Bin Liu a écrit :
> > > > 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.
> > > 
> > > My use case here is to support common GNU/Linux-based distributions,
> > > not
> > > use-case-specific varieties of GNU/Linux-based rootfs. So my point
> > > here
> > > would be that most distros will (and probably should) ship g_ether
> > > as a
> > > module but without any particular reason to autoload it, or any
> > > other
> > > gadget module in particular, since the system is general-purpose.
> > 
> > This is the case I called it "in a engineering lab", not a real
> > product.
> 
> To me, this sounds more like "daily use with upstream like on any
> laptop/desktop" rather than an engineering lab, but that's not the main
> point here.
> 
> > > Then, imagine a user wants to plug a USB device through OTG (say,
> > > because it's the only USB port available at all on the tablet
> > > they're
> > > using), it simply won't work. It won't be obvious to that user that
> > > this
> > > is because no gadget is loaded, since what they want to do does not
> > > involve using gadget mode at any point.
> > 
> > If a tablet has a dual-role usb port, it is designed to use a gadget
> > driver,
> 
> I don't understand the logic behind this assertion. If it has a dual-
> role USB port, then its hardware allows both use cases. It's obvious
> that the use case is up to the user of the device since it can be
> switched by software and is not fixed at design time.

My view is the whole (embedded) system, not just Linux itself. If the
hardware designs a dual-role port, a gadget driver has to be used.
Otherwise, define the port as host-only, either in the hardware design,
or at least in device tree.

> >  which has to be loaded at some point. In the case you described
> > above, when the gadget driver will be loaded? and how?
> 
> Again, loading a gadget driver is not part of the use case. In what I
> described, the user only wants to use the dual-role port for its host
> capability and does not care about gadget at all. When the device is
> plugged into a host, it will simply charge and not propose any USB
> device features.

It sounds to me a hacking to an existing product, not designing a new
product. If so, please hack it completely, define the port dr_mode to
host in the board device tree, then the port should work for you.

> > If a gadget driver will never be used, a host-only port should be on
> > the board, not a dual-role port.
> 
> Here as well, I think the use case is separate from the hardware design.
>  I crafted this patch because I was in the use case I described, with a
> tablet that only features a micro B USB OTG port. The form factor simply

I guess you meant micro-AB port, microB doesn't have an ID pin, cannot
make MUSB to work in host mode.

> does not allow having a full USB A female host-only port.
> 
> > > Do you think this is a valid use case? It surely is a common one and
> > > perfectly depicts my situation.
> > 
> > As I explained above, I don't think so.
> 
> I am really surprised that using regular upstream GNU/Linux
> distributions out of the box is not a valid use case for the MUSB
> driver. The situation I'm describing is exactly the same as buying a
> laptop with a preinstalled OS and replacing it with a regular distro. In
> my case, that's what I did with the tablet (that had an old Android
> version that did expose gadget features via USB) and I installed
> upstream Linux and a distro on it.

Embedded system is different than PC, I don't expect to just drop in a
distro without any modification to work, especially in this case you
change the function of the product originally designed - dual-role port
to host-only port. You would have to at least modify the board device
tree for your new purpose. 

> 
> > > Note that in addition to Allwinner devices, I also have omap3/4/5
> > > devices for testing things. I don't think I have other MUSB-enabled
> > 
> > Much more than what I have ;)
> > 
> > > devices in my collection though, but I would be willing to test
> > > fixes to
> > > this issue on the ones I have.
> > 
> > Appreciated it, but someone has to make the patches first. The one you
> > posted might be a good start, but it is not complete. The first 
> > problem
> 
> Oh, I am definitely up for making the changes as well, I mentioned
> testing to show what level of test coverage I could bring to the table,
> since this will probably require making sure that it doesn't break
> specific platforms, glue layers, etc.

yes, no regression is required.

> > I see is that musb_start() will be called twice, one in the place you
> > patched, the other is when the gadget driver is bound to the UDC.
> 
> Okay, I will look into this and make sure there is only a single call to
> musb_start in all scenarios. Are there other things that should be
> modified as well?

As I said earlier I don't plan to change it because I don't think it is
an issue, so never really looked into this. But following is my initial
thought:

Currently musb_start() is called separately for host and gadget, because
the musb drivers don't want to couple these two configurations together.
(Though the host-only or gadget-only MUSB configuration might not work
today, I have tested them for a long time, but at least the Kconfig
options are there...)

So the final solution that I would think should not couple both
configurations even more. And I will not accept a solution which only
simply adds if-else to distinguish the configurations, the drivers
already have enough if-else to clean up.

If musb_start() could be moved in musb core where is independent to host
or device, that might be a possible solution.

Again, my opinion on removing this requirement is wasting of time, as
the hardware design should be done properly at first for the usage, or
if hacking on an existing product, device tree modification should be
done first, or just simply let the Linux init module to load a gadget
driver.

Regards,
-Bin.

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

[Bin Liu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 5258 bytes --]

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@paulk.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.

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6594 bytes --]

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@paulk.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.

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.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Greg Kroah-Hartman]

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@paulk.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.
> 
> 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 :)

thanks,

greg k-h

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

[Bin Liu]

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@paulk.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.
> > 
> > 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.

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. 


Regards,
-Bin.

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 9544 bytes --]

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@paulk.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.
> > >
> > > 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.

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

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?

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.

> 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?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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@paulk.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.
> > > >
> > > > 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.

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

[Paul Kocialkowski]

Le vendredi 22 mars 2019 à 08:28 -0500, Bin Liu a écrit :
> 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.

Why should we think of an embedded product where the end user doesn't
have access to the console? Unless I'm mistaken, the Linux kernel
doesn't target commercial products where users are powerless in
particular, and leaves out all other use cases (which may or may not be
commercial).

I don't think this assumption makes any sense in Linux as a project (or
that it's sane in any context of software development for that matter,
but that's beside the point).

> > 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.

I disagree: dr_mode describes the hardware capabilities, not what the
software does with it.

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

[Bin Liu]

On Fri, Mar 22, 2019 at 02:34:41PM +0100, Paul Kocialkowski wrote:
> Le vendredi 22 mars 2019 à 08:28 -0500, Bin Liu a écrit :
> > 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.
> 
> Why should we think of an embedded product where the end user doesn't
> have access to the console? Unless I'm mistaken, the Linux kernel
> doesn't target commercial products where users are powerless in
> particular, and leaves out all other use cases (which may or may not be
> commercial).

Okay, this will lead to an endless argument as well, so I will only
explain why I mentioned embedded - AFAIK musb is only used in embedded
processors. then we can stop arguing on this point...

> I don't think this assumption makes any sense in Linux as a project (or
> that it's sane in any context of software development for that matter,
> but that's beside the point).
> 
> > > 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.
> 
> I disagree: dr_mode describes the hardware capabilities, not what the
> software does with it.

I have different understanding, dr_mode describles the use case, not the
hardware capabilities. And software operates the hardware accordingly
based on dr_mode.

Regards,
-Bin.

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 11134 bytes --]

On Fri, Mar 22, 2019 at 08:28:46AM -0500, Bin Liu wrote:
> 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@paulk.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.
> > > > >
> > > > > 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,

That's all I'm thinking about.

> if dr_mode is 'otg' which indicates the peripheral mode will be used
> at some point

No, it indicates that it *might* be used at some point, based on a
number of external factors, including:
  - Whether or not the user has plugged something in the connected USB
    connector
  - If they did so, how the ID pin has been wired (and therefore, is
    a device or a host on the other end)
  - And how the system designer decided to configure their kernel and
    userspace.

> 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.

An application could load it. And really, we start seeing SoCs in more
and more pc-like devices, including with mUSB, so I don't think we
should be making assumptions here.

How do you think Fedora, Ubuntu or Debian would behave here?

> > 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.

I'm pretty sure that would be the definition of "standard", or of a
norm at least.

> > 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.

No. The hardware is perfectly capable of using OTG. The software has
been configured not to.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Bin Liu]

Hi all,

On Fri, Mar 22, 2019 at 02:46:30PM +0100, Maxime Ripard wrote:
> >
> > Again, think about an embedded product,
> 
> That's all I'm thinking about.
> 
> > if dr_mode is 'otg' which indicates the peripheral mode will be used
> > at some point
> 
> No, it indicates that it *might* be used at some point, based on a
> number of external factors, including:
>   - Whether or not the user has plugged something in the connected USB
>     connector
>   - If they did so, how the ID pin has been wired (and therefore, is
>     a device or a host on the other end)
>   - And how the system designer decided to configure their kernel and
>     userspace.
> 
> > 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.
> 
> An application could load it. And really, we start seeing SoCs in more
> and more pc-like devices, including with mUSB, so I don't think we
> should be making assumptions here.
> 
> How do you think Fedora, Ubuntu or Debian would behave here?
> 
> > > 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.
> 
> I'm pretty sure that would be the definition of "standard", or of a
> norm at least.
> 
> > > 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.
> 
> No. The hardware is perfectly capable of using OTG. The software has
> been configured not to.

I don't think the argument will lead to anywhere. Let's stop arguing
here, so that you can spend time to fix the driver if you want to.

Regards,
-Bin.

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

[Paul Kocialkowski]

Hi,

Le vendredi 22 mars 2019 à 07:46 -0500, Bin Liu a écrit :
> 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@paulk.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.
> > > 
> > > 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.

I still have a fundamental disagreement here, and I think it would be
interesting to discuss it with Greg in the loop. I think that whether
or not we load a gadget driver when the port is declared as OTG is
policy, which is should be left to config or userspace and should not
be a prerequisite for the driver to work.

To me, OTG implies that host and peripheral are supported equally, and
one should not prevail over the other.

I'm wondering if there are general kernel rules that should apply here,
independently from the technical argument related to using gadget
modules?

> 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. 

It is definitely very much appreciated that you are open to accepting a
fix for this issue even though you consider it a non-issue :)

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

[Bin Liu]

On Fri, Mar 22, 2019 at 02:10:01PM +0100, Paul Kocialkowski wrote:
> Hi,
> 
> Le vendredi 22 mars 2019 à 07:46 -0500, Bin Liu a écrit :
> > 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@paulk.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.
> > > > 
> > > > 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.
> 
> I still have a fundamental disagreement here, and I think it would be
> interesting to discuss it with Greg in the loop. I think that whether
> or not we load a gadget driver when the port is declared as OTG is
> policy, which is should be left to config or userspace and should not
> be a prerequisite for the driver to work.
> 
> To me, OTG implies that host and peripheral are supported equally, and
> one should not prevail over the other.
> 
> I'm wondering if there are general kernel rules that should apply here,
> independently from the technical argument related to using gadget
> modules?
> 
> > 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. 
> 
> It is definitely very much appreciated that you are open to accepting a
> fix for this issue even though you consider it a non-issue :)

I believe in previous discussion I have expressed I am open to a
solution ;)

but I don't consider "it is a non-issue", it is minor in my opinion,
then I don't have time to solve it myself.

Regards,
-Bin.

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

[Paul Kocialkowski]

Le vendredi 22 mars 2019 à 08:36 -0500, Bin Liu a écrit :
> On Fri, Mar 22, 2019 at 02:10:01PM +0100, Paul Kocialkowski wrote:
> > Hi,
> > 
> > Le vendredi 22 mars 2019 à 07:46 -0500, Bin Liu a écrit :
> > > 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@paulk.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.
> > > > > 
> > > > > 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.
> > 
> > I still have a fundamental disagreement here, and I think it would be
> > interesting to discuss it with Greg in the loop. I think that whether
> > or not we load a gadget driver when the port is declared as OTG is
> > policy, which is should be left to config or userspace and should not
> > be a prerequisite for the driver to work.
> > 
> > To me, OTG implies that host and peripheral are supported equally, and
> > one should not prevail over the other.
> > 
> > I'm wondering if there are general kernel rules that should apply here,
> > independently from the technical argument related to using gadget
> > modules?
> > 
> > > 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. 
> > 
> > It is definitely very much appreciated that you are open to accepting a
> > fix for this issue even though you consider it a non-issue :)
> 
> I believe in previous discussion I have expressed I am open to a
> solution ;)

Indeed, you were always open to seeing the issue solved :)

> but I don't consider "it is a non-issue", it is minor in my opinion,
> then I don't have time to solve it myself.

Right, sorry for over-stating this!

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

[Bin Liu]

On Fri, Mar 22, 2019 at 02:37:24PM +0100, Paul Kocialkowski wrote:
> > > 
> > > > 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. 
> > > 
> > > It is definitely very much appreciated that you are open to accepting a
> > > fix for this issue even though you consider it a non-issue :)
> > 
> > I believe in previous discussion I have expressed I am open to a
> > solution ;)
> 
> Indeed, you were always open to seeing the issue solved :)
> 
> > but I don't consider "it is a non-issue", it is minor in my opinion,
> > then I don't have time to solve it myself.
> 
> Right, sorry for over-stating this!

No worries at all.

-Bin.