Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1601452 > unrolled thread

[PATCH] usb-musb: keep VBUS on when device is disconnected

Back to article view | Back to linux.kernel

Contents

  [PATCH] usb-musb: keep VBUS on when device is disconnected Moreno Bartalucci <moreno.bartalucci@tecnorama.it> - 2017-03-15 15:50 +0100
    Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Bin Liu <b-liu@ti.com> - 2017-03-24 20:00 +0100
      Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Lars Melin <larsm17@gmail.com> - 2017-03-25 08:30 +0100
        Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Moreno Bartalucci <moreno.bartalucci@tecnorama.it> - 2017-03-27 15:00 +0200
          Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Bin Liu <b-liu@ti.com> - 2017-03-27 15:20 +0200
            Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Tony Lindgren <tony@atomide.com> - 2017-03-27 16:50 +0200
              Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Moreno Bartalucci <moreno.bartalucci@tecnorama.it> - 2017-03-27 18:30 +0200
                Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Tony Lindgren <tony@atomide.com> - 2017-03-27 19:20 +0200
                  Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Bin Liu <b-liu@ti.com> - 2017-03-27 19:20 +0200
                    Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Tony Lindgren <tony@atomide.com> - 2017-03-27 20:00 +0200
                    Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Moreno Bartalucci <moreno.bartalucci@tecnorama.it> - 2017-03-28 08:20 +0200
                      Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Tony Lindgren <tony@atomide.com> - 2017-03-28 17:10 +0200

#1601452 — [PATCH] usb-musb: keep VBUS on when device is disconnected

With usb-musb port in host mode, when the device
is disconnected, either logically (because of a mode switch) or
physically (by pulling the cable), the USB port should keep
suppling VBUS, with no interruption, to prevent power loss on
USB powered devices.

Signed-off-by: Moreno Bartalucci <moreno.bartalucci@tecnorama.it>
---
 drivers/usb/musb/musb_dsps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 7c047c4..5d9986b 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -245,7 +245,7 @@ static int dsps_check_status(struct musb *musb, void *unused)
 		dsps_mod_timer_optional(glue);
 		break;
 	case OTG_STATE_A_WAIT_BCON:
-		musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
+		musb_writeb(musb->mregs, MUSB_DEVCTL, MUSB_DEVCTL_SESSION);
 		skip_session = 1;
 		/* fall */
 
-- 
2.10.1 (Apple Git-78)

[toc] | [next] | [standalone]

#1608876

On Wed, Mar 15, 2017 at 09:08:01AM -0500, Moreno Bartalucci wrote:
> With usb-musb port in host mode, when the device
> is disconnected, either logically (because of a mode switch) or
> physically (by pulling the cable), the USB port should keep
> suppling VBUS, with no interruption, to prevent power loss on
> USB powered devices.

The usb device has been disconnected, why it still cares about VBUS
power?

Can you please give more details of the issue you try to solve? This
logic has been there since the driver was added 5 years ago, so I really
have to understand the issue before accept the change.

Regards,
-Bin.

> 
> Signed-off-by: Moreno Bartalucci <moreno.bartalucci@tecnorama.it>
> ---
>  drivers/usb/musb/musb_dsps.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> index 7c047c4..5d9986b 100644
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -245,7 +245,7 @@ static int dsps_check_status(struct musb *musb, void *unused)
>  		dsps_mod_timer_optional(glue);
>  		break;
>  	case OTG_STATE_A_WAIT_BCON:
> -		musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
> +		musb_writeb(musb->mregs, MUSB_DEVCTL, MUSB_DEVCTL_SESSION);
>  		skip_session = 1;
>  		/* fall */
>  
> -- 
> 2.10.1 (Apple Git-78)
> 

[toc] | [prev] | [next] | [standalone]

#1609113

On 2017-03-25 01:58, Bin Liu wrote:
> On Wed, Mar 15, 2017 at 09:08:01AM -0500, Moreno Bartalucci wrote:
>> With usb-musb port in host mode, when the device
>> is disconnected, either logically (because of a mode switch) or
>> physically (by pulling the cable), the USB port should keep
>> suppling VBUS, with no interruption, to prevent power loss on
>> USB powered devices.
>
> The usb device has been disconnected, why it still cares about VBUS
> power?

Morphing devices (3G dongles, wifi dongles, some printers) boots up in 
install mode, usually only as a virtual cd-rom containing Windows 
drivers and software.
They get switched into functional mode by usb_modeswitch sending them
a ctrl msg which makes the device disappear from the USB bus for a very 
short time after which it re-appears with a different interface 
composition and mostly also a different USB Id.

Cutting the VBUS supply while these devices are in progress of switching 
will inhibit switching, the device will reboot when VBUS is again 
asserted and will come up in initial mode as if no switch ctrl msg had 
ever been sent to it.

The problem has been seen both on host only as well as dual-role port 
configs. Dual-role may be a bit more complicated to solve because of
the role switching VBUS detection circuit but I can not see any reason
why a host only configured port should cut the VBUS supply, it could be 
always on right?


/Lars

[toc] | [prev] | [next] | [standalone]

#1609803

> Il giorno 25 mar 2017, alle ore 08:21, Lars Melin <larsm17@gmail.com> ha scritto:
> 
>> 
>> The usb device has been disconnected, why it still cares about VBUS
>> power?
> 
> Morphing devices (3G dongles, wifi dongles, some printers) boots up in install mode, usually only as a virtual cd-rom containing Windows drivers and software.
> [...]
> why a host only configured port should cut the VBUS supply, it could be always on right?
> 

Yes, that’s exactly the problem I tried to solve with this patch.
I have to add that the problem was not there with kernels up to 4.8.17, I started to see it with 4.9 and up.

By git-bisecting kernel sources, it appears this behaviour has been introduced by this commit:

2f3fd2c5bde1f94513c3dc311ae64494085ec371

I also agree that, in my opinion, a host only port should never remove the VBUS supply, as it happens on all the PCs (linux+windows+mac) that I tested until now.

I saw this problem on a beaglebone black. Of course I’m available to do all the tests that you might suggest me to help you better understand the issue.

Moreno Bartalucci

[toc] | [prev] | [next] | [standalone]

#1609825

On Mon, Mar 27, 2017 at 02:53:27PM +0200, Moreno Bartalucci wrote:
> > Il giorno 25 mar 2017, alle ore 08:21, Lars Melin <larsm17@gmail.com> ha scritto:
> > 
> >> 
> >> The usb device has been disconnected, why it still cares about VBUS
> >> power?
> > 
> > Morphing devices (3G dongles, wifi dongles, some printers) boots up in install mode, usually only as a virtual cd-rom containing Windows drivers and software.
> > [...]
> > why a host only configured port should cut the VBUS supply, it could be always on right?
> > 
> 
> Yes, that’s exactly the problem I tried to solve with this patch.

Yeah, the problem is clear to me now.

> I have to add that the problem was not there with kernels up to
> 4.8.17, I started to see it with 4.9 and up.
> 
> By git-bisecting kernel sources, it appears this behaviour has been
> introduced by this commit:
> 
> 2f3fd2c5bde1f94513c3dc311ae64494085ec371

It seems this patch changes how OTG_STATE_A_WAIT_VRISE and
OTG_STATE_A_WAIT_BCON are used.
> 
> I also agree that, in my opinion, a host only port should never remove
> the VBUS supply, as it happens on all the PCs (linux+windows+mac) that
> I tested until now.

True. It is just that the musb driver handles both dual-role and
host-only mode.

> 
> I saw this problem on a beaglebone black. Of course I’m available to
> do all the tests that you might suggest me to help you better
> understand the issue.

Thanks for the offer. Please let me look at the problem first, I have a
modem to test.
It is not clear why the original driver clears VBUS in this place, so I
have to ensure your patch is the correct change.

Regards,
-Bin.

[toc] | [prev] | [next] | [standalone]

#1609901

* Bin Liu <b-liu@ti.com> [170327 06:22]:
> On Mon, Mar 27, 2017 at 02:53:27PM +0200, Moreno Bartalucci wrote:
> > > Il giorno 25 mar 2017, alle ore 08:21, Lars Melin <larsm17@gmail.com> ha scritto:
> > > 
> > >> 
> > >> The usb device has been disconnected, why it still cares about VBUS
> > >> power?
> > > 
> > > Morphing devices (3G dongles, wifi dongles, some printers) boots up in install mode, usually only as a virtual cd-rom containing Windows drivers and software.
> > > [...]
> > > why a host only configured port should cut the VBUS supply, it could be always on right?
> > > 
> > 
> > Yes, that’s exactly the problem I tried to solve with this patch.
> 
> Yeah, the problem is clear to me now.

Ah so it's with the state changing devices.

> > I have to add that the problem was not there with kernels up to
> > 4.8.17, I started to see it with 4.9 and up.
> > 
> > By git-bisecting kernel sources, it appears this behaviour has been
> > introduced by this commit:
> > 
> > 2f3fd2c5bde1f94513c3dc311ae64494085ec371
> 
> It seems this patch changes how OTG_STATE_A_WAIT_VRISE and
> OTG_STATE_A_WAIT_BCON are used.
> > 
> > I also agree that, in my opinion, a host only port should never remove
> > the VBUS supply, as it happens on all the PCs (linux+windows+mac) that
> > I tested until now.
> 
> True. It is just that the musb driver handles both dual-role and
> host-only mode.

Maybe the other way to fix it would be to keep VBUS on longer when
checking for connected devices. Maybe we now recheck based on the
poll_timeout too early and then nothing is found.

> > I saw this problem on a beaglebone black. Of course I’m available to
> > do all the tests that you might suggest me to help you better
> > understand the issue.
> 
> Thanks for the offer. Please let me look at the problem first, I have a
> modem to test.
> It is not clear why the original driver clears VBUS in this place, so I
> have to ensure your patch is the correct change.

I wonder if the following test patch allows the mode changing
devices to been properly found? Of course it's just for testing,
and scanning for devices takes now 5 seconds.. But it might
provide clues why musb thinks no devices are connected and we
cut VBUS.

Regards,

Tony

8< ----------------------
diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -991,7 +991,7 @@ static const struct dsps_musb_wrapper am33xx_driver_data = {
 	.rxep_shift		= 16,
 	.rxep_mask		= 0xfffe,
 	.rxep_bitmap		= (0xfffe << 16),
-	.poll_timeout		= 2000, /* ms */
+	.poll_timeout		= 5000, /* ms */
 };
 
 static const struct of_device_id musb_dsps_of_match[] = {

[toc] | [prev] | [next] | [standalone]

#1609973

> Il giorno 27 mar 2017, alle ore 16:30, Tony Lindgren <tony@atomide.com> ha scritto:
> 
> […]
> I wonder if the following test patch allows the mode changing
> devices to been properly found? Of course it's just for testing,
> and scanning for devices takes now 5 seconds.. But it might
> provide clues why musb thinks no devices are connected and we
> cut VBUS.
> 
> […]

Hi Tony,

thanks for your patch.

I tested it with both current mainline kernel and my “production” kernel (4.9.13): they have the same behaviour.

During boot, vbus is first asserted, then removed for a slightly longer time than before (most likely 5 seconds now), then reasserted.

When my device is mode-switched, it is working properly with your patch.

If I understood your patch, however, if the device (anyone, not just my one) takes longer to switch, VBUS is deasserted anyway.

Although this patch is working for me, personally I would prefer a solution which would not deassert VBUS. At least on a host only port. Honestly I don’t know how a dual role port should work.

Regards,

Moreno

[toc] | [prev] | [next] | [standalone]

#1610006

* Moreno Bartalucci <moreno.bartalucci@tecnorama.it> [170327 09:23]:
> If I understood your patch, however, if the device (anyone, not just my one) takes longer to switch, VBUS is deasserted anyway.

Yeah some of them can take at least 10 seconds even to enumerate.
So probably we need to have to have some longer timeout set for
OTG_STATE_A_WAIT_BCON, like 20 or 30 seconds.

> Although this patch is working for me, personally I would prefer a solution which would not deassert VBUS. At least on a host only port. Honestly I don’t know how a dual role port should work.

It's been really long time since I read the OTG spec. There
may be some diagram showing the required timeouts in the spec
if there is one for VBUS.

Maybe we need some property to specify vbus-always-on-in-host-mode?

Regards,

Tony

[toc] | [prev] | [next] | [standalone]

#1610007

On Mon, Mar 27, 2017 at 09:59:47AM -0700, Tony Lindgren wrote:
> * Moreno Bartalucci <moreno.bartalucci@tecnorama.it> [170327 09:23]:
> > If I understood your patch, however, if the device (anyone, not just my one) takes longer to switch, VBUS is deasserted anyway.
> 
> Yeah some of them can take at least 10 seconds even to enumerate.
> So probably we need to have to have some longer timeout set for
> OTG_STATE_A_WAIT_BCON, like 20 or 30 seconds.
> 
> > Although this patch is working for me, personally I would prefer a solution which would not deassert VBUS. At least on a host only port. Honestly I don’t know how a dual role port should work.
> 
> It's been really long time since I read the OTG spec. There
> may be some diagram showing the required timeouts in the spec
> if there is one for VBUS.
> 
> Maybe we need some property to specify vbus-always-on-in-host-mode?

The MUSB otg state machine has been changed in many place since the last
time I looked at it, and I am not sure how exactly it works now.

If the $subject patch can correctly keep the VBUS on for host-only mode,
we can somehow use dr_modei value to distinguish the mode. We don't have
to create a new vbus-always-on-in-host-mode flag. VBUS has to be always
on in host-only mode anyway, until some error condition happens.

Regards,
-Bin.

[toc] | [prev] | [next] | [standalone]

#1610022

* Bin Liu <b-liu@ti.com> [170327 10:17]:
> On Mon, Mar 27, 2017 at 09:59:47AM -0700, Tony Lindgren wrote:
> > * Moreno Bartalucci <moreno.bartalucci@tecnorama.it> [170327 09:23]:
> > > If I understood your patch, however, if the device (anyone, not just my one) takes longer to switch, VBUS is deasserted anyway.
> > 
> > Yeah some of them can take at least 10 seconds even to enumerate.
> > So probably we need to have to have some longer timeout set for
> > OTG_STATE_A_WAIT_BCON, like 20 or 30 seconds.
> > 
> > > Although this patch is working for me, personally I would prefer a solution which would not deassert VBUS. At least on a host only port. Honestly I don’t know how a dual role port should work.
> > 
> > It's been really long time since I read the OTG spec. There
> > may be some diagram showing the required timeouts in the spec
> > if there is one for VBUS.
> > 
> > Maybe we need some property to specify vbus-always-on-in-host-mode?
> 
> The MUSB otg state machine has been changed in many place since the last
> time I looked at it, and I am not sure how exactly it works now.

Yup.. I looked up the timers in the OTG spec and they are described
in chapter "8.5.5.2" as a_wait_bcon_tmo or a_wait_bcon_tmr. But
I could not find any values for them.

Anyways, clearly we want things working with real devices :)

> If the $subject patch can correctly keep the VBUS on for host-only mode,
> we can somehow use dr_modei value to distinguish the mode. We don't have
> to create a new vbus-always-on-in-host-mode flag. VBUS has to be always
> on in host-only mode anyway, until some error condition happens.

Yeh and it seems PM still works with the $subject patch also for
host mode. So maybe that's enough to fix the issue.

Also I don't have any idea why for ages we have been writing
0 to devctl there.. Maybe we've had a bug there that only now
shows up when we idle things.

Anyways, for the $subject patch related to MUSB runtime PM:

Tested-by: Tony Lindgren <tony@atomide.com>

[toc] | [prev] | [next] | [standalone]

#1610352

> Il giorno 27 mar 2017, alle ore 19:15, Bin Liu <b-liu@ti.com> ha scritto:
> 
> […]
> 
> The MUSB otg state machine has been changed in many place since the last
> time I looked at it, and I am not sure how exactly it works now.
> 
> If the $subject patch can correctly keep the VBUS on for host-only mode,
> we can somehow use dr_modei value to distinguish the mode. We don't have
> to create a new vbus-always-on-in-host-mode flag. VBUS has to be always
> on in host-only mode anyway, until some error condition happens.
> 

During my research, I used this patch to try to print the status of the usb port:

--- a/drivers/usb/musb/musb_dsps.c	2017-03-13 09:34:31.000000000 +0100
+++ b/drivers/usb/musb/musb_dsps.c	2017-03-13 09:36:02.000000000 +0100
@@ -245,6 +245,8 @@ static void otg_timer(unsigned long _mus
 	dev_dbg(musb->controller, "Poll devctl %02x (%s)\n", devctl,
 				usb_otg_state_string(musb->xceiv->otg->state));

+	dev_emerg(musb->controller, "musb->xceiv->otg->state=%s, musb->port_mode=%d\n", usb_otg_state_string(musb->xceiv->otg->state),(int)musb->port_mode);
+
 	spin_lock_irqsave(&musb->lock, flags);
 	switch (musb->xceiv->otg->state) {
 	case OTG_STATE_A_WAIT_BCON:

Unless I did something wrong, maybe it’s worth to notice that before the patch that apparently introduced this bug (2f3fd2c5bde1f94513c3dc311ae64494085ec371) I got nothing printed anywhere.
With that patch applied, instead, I got the line printed in dmesg.
I might be wrong but my assumption is that without that patch otg_timer was never called.
If this is true, it would explain why writing 0 on DEVCTL didn’t bother anything: it never happened.

Regards,

Moreno

[toc] | [prev] | [next] | [standalone]

#1611069

* Moreno Bartalucci <moreno.bartalucci@tecnorama.it> [170327 23:12]:
> > Il giorno 27 mar 2017, alle ore 19:15, Bin Liu <b-liu@ti.com> ha scritto:
> > 
> > […]
> > 
> > The MUSB otg state machine has been changed in many place since the last
> > time I looked at it, and I am not sure how exactly it works now.
> > 
> > If the $subject patch can correctly keep the VBUS on for host-only mode,
> > we can somehow use dr_modei value to distinguish the mode. We don't have
> > to create a new vbus-always-on-in-host-mode flag. VBUS has to be always
> > on in host-only mode anyway, until some error condition happens.
> > 
> 
> During my research, I used this patch to try to print the status of the usb port:
> 
> --- a/drivers/usb/musb/musb_dsps.c	2017-03-13 09:34:31.000000000 +0100
> +++ b/drivers/usb/musb/musb_dsps.c	2017-03-13 09:36:02.000000000 +0100
> @@ -245,6 +245,8 @@ static void otg_timer(unsigned long _mus
>  	dev_dbg(musb->controller, "Poll devctl %02x (%s)\n", devctl,
>  				usb_otg_state_string(musb->xceiv->otg->state));
> 
> +	dev_emerg(musb->controller, "musb->xceiv->otg->state=%s, musb->port_mode=%d\n", usb_otg_state_string(musb->xceiv->otg->state),(int)musb->port_mode);
> +
>  	spin_lock_irqsave(&musb->lock, flags);
>  	switch (musb->xceiv->otg->state) {
>  	case OTG_STATE_A_WAIT_BCON:
> 
> Unless I did something wrong, maybe it’s worth to notice that before the patch that apparently introduced this bug (2f3fd2c5bde1f94513c3dc311ae64494085ec371) I got nothing printed anywhere.
> With that patch applied, instead, I got the line printed in dmesg.
> I might be wrong but my assumption is that without that patch otg_timer was never called.
> If this is true, it would explain why writing 0 on DEVCTL didn’t bother anything: it never happened.

OK yeah that makes sense.

Thanks,

Tony

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web