* [PATCH] usb: musb: Support gadget mode when the port is set to dual role @ 2018-03-28 21:52 Paul Kocialkowski 2018-03-29 9:23 ` Maxime Ripard 0 siblings, 1 reply; 26+ messages in thread From: Paul Kocialkowski @ 2018-03-28 21:52 UTC (permalink / raw) To: linux-kernel, linux-usb Cc: Greg Kroah-Hartman, Bin Liu, Chen-Yu Tsai, Maxime Ripard, 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2018-03-28 21:52 [PATCH] usb: musb: Support gadget mode when the port is set to dual role Paul Kocialkowski @ 2018-03-29 9:23 ` Maxime Ripard 2018-03-29 11:57 ` Paul Kocialkowski 0 siblings, 1 reply; 26+ messages in thread From: Maxime Ripard @ 2018-03-29 9:23 UTC (permalink / raw) To: Paul Kocialkowski Cc: linux-kernel, linux-usb, Greg Kroah-Hartman, Bin Liu, Chen-Yu Tsai [-- 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 --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2018-03-29 9:23 ` Maxime Ripard @ 2018-03-29 11:57 ` Paul Kocialkowski 2018-04-03 9:29 ` Maxime Ripard 2018-04-20 14:25 ` Bin Liu 0 siblings, 2 replies; 26+ messages in thread From: Paul Kocialkowski @ 2018-03-29 11:57 UTC (permalink / raw) To: Maxime Ripard Cc: linux-kernel, linux-usb, Greg Kroah-Hartman, Bin Liu, Chen-Yu Tsai, 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 --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2018-03-29 11:57 ` Paul Kocialkowski @ 2018-04-03 9:29 ` Maxime Ripard 2018-04-21 10:51 ` Paul Kocialkowski 2018-04-20 14:25 ` Bin Liu 1 sibling, 1 reply; 26+ messages in thread From: Maxime Ripard @ 2018-04-03 9:29 UTC (permalink / raw) To: Paul Kocialkowski Cc: linux-kernel, linux-usb, Greg Kroah-Hartman, Bin Liu, Chen-Yu Tsai, Paul Kocialkowski [-- 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 --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2018-04-03 9:29 ` Maxime Ripard @ 2018-04-21 10:51 ` Paul Kocialkowski 0 siblings, 0 replies; 26+ messages in thread From: Paul Kocialkowski @ 2018-04-21 10:51 UTC (permalink / raw) To: Maxime Ripard, Paul Kocialkowski Cc: linux-kernel, linux-usb, Greg Kroah-Hartman, Bin Liu, Chen-Yu Tsai [-- 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 --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2018-03-29 11:57 ` Paul Kocialkowski 2018-04-03 9:29 ` Maxime Ripard @ 2018-04-20 14:25 ` Bin Liu 2018-04-21 10:59 ` Paul Kocialkowski 1 sibling, 1 reply; 26+ messages in thread From: Bin Liu @ 2018-04-20 14:25 UTC (permalink / raw) To: Paul Kocialkowski Cc: Maxime Ripard, linux-kernel, linux-usb, Greg Kroah-Hartman, Chen-Yu Tsai, Paul Kocialkowski 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2018-04-20 14:25 ` Bin Liu @ 2018-04-21 10:59 ` Paul Kocialkowski 2018-04-21 14:34 ` Bin Liu 0 siblings, 1 reply; 26+ messages in thread From: Paul Kocialkowski @ 2018-04-21 10:59 UTC (permalink / raw) To: Bin Liu, Paul Kocialkowski Cc: Maxime Ripard, linux-kernel, linux-usb, Greg Kroah-Hartman, Chen-Yu Tsai [-- 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 --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2018-04-21 10:59 ` Paul Kocialkowski @ 2018-04-21 14:34 ` Bin Liu 2018-04-30 21:08 ` Paul Kocialkowski ` (2 more replies) 0 siblings, 3 replies; 26+ messages in thread From: Bin Liu @ 2018-04-21 14:34 UTC (permalink / raw) To: Paul Kocialkowski Cc: Paul Kocialkowski, Maxime Ripard, linux-kernel, linux-usb, Greg Kroah-Hartman, Chen-Yu Tsai 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2018-04-21 14:34 ` Bin Liu @ 2018-04-30 21:08 ` Paul Kocialkowski 2018-05-01 12:25 ` Bin Liu 2019-03-21 10:02 ` Bin Liu 2019-03-21 13:01 ` Maxime Ripard 2 siblings, 1 reply; 26+ messages in thread From: Paul Kocialkowski @ 2018-04-30 21:08 UTC (permalink / raw) To: Bin Liu Cc: Paul Kocialkowski, Maxime Ripard, linux-kernel, linux-usb, Greg Kroah-Hartman, Chen-Yu Tsai [-- 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 --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2018-04-30 21:08 ` Paul Kocialkowski @ 2018-05-01 12:25 ` Bin Liu 2018-05-01 13:26 ` Paul Kocialkowski 0 siblings, 1 reply; 26+ messages in thread From: Bin Liu @ 2018-05-01 12:25 UTC (permalink / raw) To: Paul Kocialkowski Cc: Paul Kocialkowski, Maxime Ripard, linux-kernel, linux-usb, Greg Kroah-Hartman, Chen-Yu Tsai 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2018-05-01 12:25 ` Bin Liu @ 2018-05-01 13:26 ` Paul Kocialkowski 2018-05-01 16:22 ` Bin Liu 0 siblings, 1 reply; 26+ messages in thread From: Paul Kocialkowski @ 2018-05-01 13:26 UTC (permalink / raw) To: Bin Liu Cc: Paul Kocialkowski, Maxime Ripard, linux-kernel, linux-usb, Greg Kroah-Hartman, Chen-Yu Tsai [-- 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 --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2018-05-01 13:26 ` Paul Kocialkowski @ 2018-05-01 16:22 ` Bin Liu 0 siblings, 0 replies; 26+ messages in thread From: Bin Liu @ 2018-05-01 16:22 UTC (permalink / raw) To: Paul Kocialkowski Cc: Paul Kocialkowski, Maxime Ripard, linux-kernel, linux-usb, Greg Kroah-Hartman, Chen-Yu Tsai 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2018-04-21 14:34 ` Bin Liu 2018-04-30 21:08 ` Paul Kocialkowski @ 2019-03-21 10:02 ` Bin Liu 2019-03-21 13:01 ` Maxime Ripard 2 siblings, 0 replies; 26+ messages in thread From: Bin Liu @ 2019-03-21 10:02 UTC (permalink / raw) To: Paul Kocialkowski Cc: Paul Kocialkowski, Maxime Ripard, linux-kernel, linux-usb, Greg Kroah-Hartman, Chen-Yu Tsai [-- 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2018-04-21 14:34 ` Bin Liu 2018-04-30 21:08 ` Paul Kocialkowski 2019-03-21 10:02 ` Bin Liu @ 2019-03-21 13:01 ` Maxime Ripard 2019-03-21 16:41 ` Greg Kroah-Hartman 2 siblings, 1 reply; 26+ messages in thread From: Maxime Ripard @ 2019-03-21 13:01 UTC (permalink / raw) To: Bin Liu, Paul Kocialkowski, Paul Kocialkowski, linux-kernel, linux-usb, Greg Kroah-Hartman, Chen-Yu Tsai [-- 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 --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2019-03-21 13:01 ` Maxime Ripard @ 2019-03-21 16:41 ` Greg Kroah-Hartman 2019-03-22 12:46 ` Bin Liu 0 siblings, 1 reply; 26+ messages in thread From: Greg Kroah-Hartman @ 2019-03-21 16:41 UTC (permalink / raw) To: Maxime Ripard Cc: Bin Liu, Paul Kocialkowski, Paul Kocialkowski, linux-kernel, linux-usb, Chen-Yu Tsai 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2019-03-21 16:41 ` Greg Kroah-Hartman @ 2019-03-22 12:46 ` Bin Liu 2019-03-22 13:09 ` Maxime Ripard 2019-03-22 13:10 ` Paul Kocialkowski 0 siblings, 2 replies; 26+ messages in thread From: Bin Liu @ 2019-03-22 12:46 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Maxime Ripard, Paul Kocialkowski, Paul Kocialkowski, linux-kernel, linux-usb, Chen-Yu Tsai 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2019-03-22 12:46 ` Bin Liu @ 2019-03-22 13:09 ` Maxime Ripard 2019-03-22 13:28 ` Bin Liu 2019-03-22 13:10 ` Paul Kocialkowski 1 sibling, 1 reply; 26+ messages in thread From: Maxime Ripard @ 2019-03-22 13:09 UTC (permalink / raw) To: Bin Liu, Greg Kroah-Hartman, Paul Kocialkowski, Paul Kocialkowski, linux-kernel, linux-usb, Chen-Yu Tsai [-- 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 --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2019-03-22 13:09 ` Maxime Ripard @ 2019-03-22 13:28 ` Bin Liu 2019-03-22 13:34 ` Paul Kocialkowski 2019-03-22 13:46 ` Maxime Ripard 0 siblings, 2 replies; 26+ messages in thread From: Bin Liu @ 2019-03-22 13:28 UTC (permalink / raw) To: Maxime Ripard Cc: Greg Kroah-Hartman, Paul Kocialkowski, Paul Kocialkowski, linux-kernel, linux-usb, Chen-Yu Tsai 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2019-03-22 13:28 ` Bin Liu @ 2019-03-22 13:34 ` Paul Kocialkowski 2019-03-22 13:44 ` Bin Liu 2019-03-22 13:46 ` Maxime Ripard 1 sibling, 1 reply; 26+ messages in thread From: Paul Kocialkowski @ 2019-03-22 13:34 UTC (permalink / raw) To: Bin Liu, Maxime Ripard Cc: Greg Kroah-Hartman, linux-kernel, linux-usb, Chen-Yu Tsai 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2019-03-22 13:34 ` Paul Kocialkowski @ 2019-03-22 13:44 ` Bin Liu 0 siblings, 0 replies; 26+ messages in thread From: Bin Liu @ 2019-03-22 13:44 UTC (permalink / raw) To: Paul Kocialkowski Cc: Maxime Ripard, Greg Kroah-Hartman, linux-kernel, linux-usb, Chen-Yu Tsai 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2019-03-22 13:28 ` Bin Liu 2019-03-22 13:34 ` Paul Kocialkowski @ 2019-03-22 13:46 ` Maxime Ripard 2019-03-22 14:04 ` Bin Liu 1 sibling, 1 reply; 26+ messages in thread From: Maxime Ripard @ 2019-03-22 13:46 UTC (permalink / raw) To: Bin Liu, Greg Kroah-Hartman, Paul Kocialkowski, Paul Kocialkowski, linux-kernel, linux-usb, Chen-Yu Tsai [-- 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 --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2019-03-22 13:46 ` Maxime Ripard @ 2019-03-22 14:04 ` Bin Liu 0 siblings, 0 replies; 26+ messages in thread From: Bin Liu @ 2019-03-22 14:04 UTC (permalink / raw) To: Maxime Ripard Cc: Greg Kroah-Hartman, Paul Kocialkowski, Paul Kocialkowski, linux-kernel, linux-usb, Chen-Yu Tsai 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2019-03-22 12:46 ` Bin Liu 2019-03-22 13:09 ` Maxime Ripard @ 2019-03-22 13:10 ` Paul Kocialkowski 2019-03-22 13:36 ` Bin Liu 1 sibling, 1 reply; 26+ messages in thread From: Paul Kocialkowski @ 2019-03-22 13:10 UTC (permalink / raw) To: Bin Liu, Greg Kroah-Hartman Cc: Maxime Ripard, linux-kernel, linux-usb, Chen-Yu Tsai 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2019-03-22 13:10 ` Paul Kocialkowski @ 2019-03-22 13:36 ` Bin Liu 2019-03-22 13:37 ` Paul Kocialkowski 0 siblings, 1 reply; 26+ messages in thread From: Bin Liu @ 2019-03-22 13:36 UTC (permalink / raw) To: Paul Kocialkowski Cc: Greg Kroah-Hartman, Maxime Ripard, linux-kernel, linux-usb, Chen-Yu Tsai 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2019-03-22 13:36 ` Bin Liu @ 2019-03-22 13:37 ` Paul Kocialkowski 2019-03-22 13:46 ` Bin Liu 0 siblings, 1 reply; 26+ messages in thread From: Paul Kocialkowski @ 2019-03-22 13:37 UTC (permalink / raw) To: Bin Liu Cc: Greg Kroah-Hartman, Maxime Ripard, linux-kernel, linux-usb, Chen-Yu Tsai 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role 2019-03-22 13:37 ` Paul Kocialkowski @ 2019-03-22 13:46 ` Bin Liu 0 siblings, 0 replies; 26+ messages in thread From: Bin Liu @ 2019-03-22 13:46 UTC (permalink / raw) To: Paul Kocialkowski Cc: Greg Kroah-Hartman, Maxime Ripard, linux-kernel, linux-usb, Chen-Yu Tsai 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2019-03-22 14:04 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-28 21:52 [PATCH] usb: musb: Support gadget mode when the port is set to dual role Paul Kocialkowski 2018-03-29 9:23 ` Maxime Ripard 2018-03-29 11:57 ` Paul Kocialkowski 2018-04-03 9:29 ` Maxime Ripard 2018-04-21 10:51 ` Paul Kocialkowski 2018-04-20 14:25 ` Bin Liu 2018-04-21 10:59 ` Paul Kocialkowski 2018-04-21 14:34 ` Bin Liu 2018-04-30 21:08 ` Paul Kocialkowski 2018-05-01 12:25 ` Bin Liu 2018-05-01 13:26 ` Paul Kocialkowski 2018-05-01 16:22 ` Bin Liu 2019-03-21 10:02 ` Bin Liu 2019-03-21 13:01 ` Maxime Ripard 2019-03-21 16:41 ` Greg Kroah-Hartman 2019-03-22 12:46 ` Bin Liu 2019-03-22 13:09 ` Maxime Ripard 2019-03-22 13:28 ` Bin Liu 2019-03-22 13:34 ` Paul Kocialkowski 2019-03-22 13:44 ` Bin Liu 2019-03-22 13:46 ` Maxime Ripard 2019-03-22 14:04 ` Bin Liu 2019-03-22 13:10 ` Paul Kocialkowski 2019-03-22 13:36 ` Bin Liu 2019-03-22 13:37 ` Paul Kocialkowski 2019-03-22 13:46 ` Bin Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).