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

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

[Moreno Bartalucci]

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)

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

[Moreno Bartalucci]

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-Hj/TeGiWV4YL5bzFcGmneg@public.gmane.org>
---
 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)

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bin Liu]

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

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

[Bin Liu]

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-Hj/TeGiWV4YL5bzFcGmneg@public.gmane.org>
> ---
>  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)
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Lars Melin]

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

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

[Moreno Bartalucci]

> 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

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

[Moreno Bartalucci]

> Il giorno 25 mar 2017, alle ore 08:21, Lars Melin <larsm17-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bin Liu]

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.

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

[Bin Liu]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tony Lindgren]

* 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[] = {

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

[Moreno Bartalucci]

> 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

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

[Tony Lindgren]

* 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

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

[Tony Lindgren]

* Moreno Bartalucci <moreno.bartalucci-Hj/TeGiWV4YL5bzFcGmneg@public.gmane.org> [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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bin Liu]

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.

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

[Tony Lindgren]

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

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

[Tony Lindgren]

* Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [170327 10:17]:
> On Mon, Mar 27, 2017 at 09:59:47AM -0700, Tony Lindgren wrote:
> > * Moreno Bartalucci <moreno.bartalucci-Hj/TeGiWV4YL5bzFcGmneg@public.gmane.org> [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-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Moreno Bartalucci]

> 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

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

[Moreno Bartalucci]

> Il giorno 27 mar 2017, alle ore 19:15, Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tony Lindgren]

* 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

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

[Tony Lindgren]

* Moreno Bartalucci <moreno.bartalucci-Hj/TeGiWV4YL5bzFcGmneg@public.gmane.org> [170327 23:12]:
> > Il giorno 27 mar 2017, alle ore 19:15, Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bin Liu]

On Mon, Mar 27, 2017 at 10:55:37AM -0700, Tony Lindgren wrote:
> * 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.

The otg state machine implementation in the musb drivers are kind of strange.
OTG_STATE_A_WAIT_BCON suppose to be a steady state when no usb device is
attached, but the musb drivers use it as a transient state to handle error
cases, such as overcurrent ot HNP timeout, which is done in the 'case
OTG_STATE_A_WAIT_BCON' branch in otg_timer() (or dsps_check_status() for dsps
glue).

Then later when 2f3fd2c5bde1 adds

-       /* Poll for ID change in OTG port mode */
-       if (musb->xceiv->otg->state == OTG_STATE_B_IDLE &&
-                       musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE)
+       /* Poll for ID change and connect */
+       switch (musb->xceiv->otg->state) {
+       case OTG_STATE_B_IDLE:
+       case OTG_STATE_A_WAIT_BCON:
                mod_timer(&glue->timer, jiffies +
                                msecs_to_jiffies(wrp->poll_timeout));
+               break;

which causes dsps_check_status (or otg_timer()) got called for a normal
condition with OTG_STATE_A_WAIT_BCON, then turns off VBUS...

Will try to see how to solve this...

Regards,
-Bin.

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

[Tony Lindgren]

* Bin Liu <b-liu@ti.com> [170511 11:53]:
> On Mon, Mar 27, 2017 at 10:55:37AM -0700, Tony Lindgren wrote:
> > * 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.
> 
> The otg state machine implementation in the musb drivers are kind of strange.
> OTG_STATE_A_WAIT_BCON suppose to be a steady state when no usb device is
> attached, but the musb drivers use it as a transient state to handle error
> cases, such as overcurrent ot HNP timeout, which is done in the 'case
> OTG_STATE_A_WAIT_BCON' branch in otg_timer() (or dsps_check_status() for dsps
> glue).
> 
> Then later when 2f3fd2c5bde1 adds
> 
> -       /* Poll for ID change in OTG port mode */
> -       if (musb->xceiv->otg->state == OTG_STATE_B_IDLE &&
> -                       musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE)
> +       /* Poll for ID change and connect */
> +       switch (musb->xceiv->otg->state) {
> +       case OTG_STATE_B_IDLE:
> +       case OTG_STATE_A_WAIT_BCON:
>                 mod_timer(&glue->timer, jiffies +
>                                 msecs_to_jiffies(wrp->poll_timeout));
> +               break;
> 
> which causes dsps_check_status (or otg_timer()) got called for a normal
> condition with OTG_STATE_A_WAIT_BCON, then turns off VBUS...
> 
> Will try to see how to solve this...

Maybe we just need to add back the earlier check for non-OTG devices
"musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE" into case
OTG_STATE_A_WAIT_BCON?

Not sure if am335x configured with host port ever idle that way,
but maybe VBUS can be kept on with musb idle.

Regards,

Tony

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

[Bin Liu]

On Thu, May 11, 2017 at 11:55:28AM -0700, Tony Lindgren wrote:
> * Bin Liu <b-liu@ti.com> [170511 11:53]:
> > On Mon, Mar 27, 2017 at 10:55:37AM -0700, Tony Lindgren wrote:
> > > * 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.
> > 
> > The otg state machine implementation in the musb drivers are kind of strange.
> > OTG_STATE_A_WAIT_BCON suppose to be a steady state when no usb device is
> > attached, but the musb drivers use it as a transient state to handle error
> > cases, such as overcurrent ot HNP timeout, which is done in the 'case
> > OTG_STATE_A_WAIT_BCON' branch in otg_timer() (or dsps_check_status() for dsps
> > glue).
> > 
> > Then later when 2f3fd2c5bde1 adds
> > 
> > -       /* Poll for ID change in OTG port mode */
> > -       if (musb->xceiv->otg->state == OTG_STATE_B_IDLE &&
> > -                       musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE)
> > +       /* Poll for ID change and connect */
> > +       switch (musb->xceiv->otg->state) {
> > +       case OTG_STATE_B_IDLE:
> > +       case OTG_STATE_A_WAIT_BCON:
> >                 mod_timer(&glue->timer, jiffies +
> >                                 msecs_to_jiffies(wrp->poll_timeout));
> > +               break;
> > 
> > which causes dsps_check_status (or otg_timer()) got called for a normal
> > condition with OTG_STATE_A_WAIT_BCON, then turns off VBUS...
> > 
> > Will try to see how to solve this...
> 
> Maybe we just need to add back the earlier check for non-OTG devices
> "musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE" into case
> OTG_STATE_A_WAIT_BCON?

Not sure if it will work. The check is to kick out device-only mode.
DUAL_ROLE means both OTG and host-only.

> 
> Not sure if am335x configured with host port ever idle that way,
> but maybe VBUS can be kept on with musb idle.

Just tried to remove 

+       case OTG_STATE_A_WAIT_BCON:

for the polling, but the port stops detecting device attach, musb
power/devctl registers are good, trying to see why... maybe related to
PM state?

Regards,
-Bin.

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

[Bin Liu]

On Thu, May 11, 2017 at 02:01:00PM -0500, Bin Liu wrote:
> On Thu, May 11, 2017 at 11:55:28AM -0700, Tony Lindgren wrote:
> > * Bin Liu <b-liu@ti.com> [170511 11:53]:
> > > On Mon, Mar 27, 2017 at 10:55:37AM -0700, Tony Lindgren wrote:
> > > > * 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.
> > > 
> > > The otg state machine implementation in the musb drivers are kind of strange.
> > > OTG_STATE_A_WAIT_BCON suppose to be a steady state when no usb device is
> > > attached, but the musb drivers use it as a transient state to handle error
> > > cases, such as overcurrent ot HNP timeout, which is done in the 'case
> > > OTG_STATE_A_WAIT_BCON' branch in otg_timer() (or dsps_check_status() for dsps
> > > glue).
> > > 
> > > Then later when 2f3fd2c5bde1 adds
> > > 
> > > -       /* Poll for ID change in OTG port mode */
> > > -       if (musb->xceiv->otg->state == OTG_STATE_B_IDLE &&
> > > -                       musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE)
> > > +       /* Poll for ID change and connect */
> > > +       switch (musb->xceiv->otg->state) {
> > > +       case OTG_STATE_B_IDLE:
> > > +       case OTG_STATE_A_WAIT_BCON:
> > >                 mod_timer(&glue->timer, jiffies +
> > >                                 msecs_to_jiffies(wrp->poll_timeout));
> > > +               break;
> > > 
> > > which causes dsps_check_status (or otg_timer()) got called for a normal
> > > condition with OTG_STATE_A_WAIT_BCON, then turns off VBUS...
> > > 
> > > Will try to see how to solve this...
> > 
> > Maybe we just need to add back the earlier check for non-OTG devices
> > "musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE" into case
> > OTG_STATE_A_WAIT_BCON?
> 
> Not sure if it will work. The check is to kick out device-only mode.
> DUAL_ROLE means both OTG and host-only.
> 
> > 
> > Not sure if am335x configured with host port ever idle that way,
> > but maybe VBUS can be kept on with musb idle.
> 
> Just tried to remove 
> 
> +       case OTG_STATE_A_WAIT_BCON:
> 
> for the polling, but the port stops detecting device attach, musb
> power/devctl registers are good, trying to see why... maybe related to
> PM state?

usb_otg_hs is idle, so not able to detect attach. So need to get the
timer rolling in here to make it detect working.
Maybe need to add a flag in dsps_check_status() to not turn off vbus in
this case? Really don't like the idea...

Regards,
-Bin.

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

[Bin Liu]

On Thu, May 11, 2017 at 02:10:05PM -0500, Bin Liu wrote:
[...]
> > > > The otg state machine implementation in the musb drivers are kind of strange.
> > > > OTG_STATE_A_WAIT_BCON suppose to be a steady state when no usb device is
> > > > attached, but the musb drivers use it as a transient state to handle error
> > > > cases, such as overcurrent ot HNP timeout, which is done in the 'case
> > > > OTG_STATE_A_WAIT_BCON' branch in otg_timer() (or dsps_check_status() for dsps
> > > > glue).
> > > > 
> > > > Then later when 2f3fd2c5bde1 adds
> > > > 
> > > > -       /* Poll for ID change in OTG port mode */
> > > > -       if (musb->xceiv->otg->state == OTG_STATE_B_IDLE &&
> > > > -                       musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE)
> > > > +       /* Poll for ID change and connect */
> > > > +       switch (musb->xceiv->otg->state) {
> > > > +       case OTG_STATE_B_IDLE:
> > > > +       case OTG_STATE_A_WAIT_BCON:
> > > >                 mod_timer(&glue->timer, jiffies +
> > > >                                 msecs_to_jiffies(wrp->poll_timeout));
> > > > +               break;
> > > > 
> > > > which causes dsps_check_status (or otg_timer()) got called for a normal
> > > > condition with OTG_STATE_A_WAIT_BCON, then turns off VBUS...
> > > > 
> > > > Will try to see how to solve this...
> > > 
> > > Maybe we just need to add back the earlier check for non-OTG devices
> > > "musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE" into case
> > > OTG_STATE_A_WAIT_BCON?
> > 
> > Not sure if it will work. The check is to kick out device-only mode.
> > DUAL_ROLE means both OTG and host-only.
> > 
> > > 
> > > Not sure if am335x configured with host port ever idle that way,
> > > but maybe VBUS can be kept on with musb idle.
> > 
> > Just tried to remove 
> > 
> > +       case OTG_STATE_A_WAIT_BCON:
> > 
> > for the polling, but the port stops detecting device attach, musb
> > power/devctl registers are good, trying to see why... maybe related to
> > PM state?
> 
> usb_otg_hs is idle, so not able to detect attach. So need to get the
> timer rolling in here to make it detect working.
> Maybe need to add a flag in dsps_check_status() to not turn off vbus in
> this case? Really don't like the idea...

Tony,

I am not sure I gave out the picture clearly.

Before added the runtime PM support, in host mode (devctl=0x19) the
otg_timer is stopped, the controller is ready to detect attach.

Later when added the runtime PM support, usb_otg_hs hwmod becomes idle
when a device is detached, so the otg_timer is needed to keep hwmod
enable/idle periodically to detect attach, which triggers the issue
because otg_timer() turns off vbus unconditionally when otg state is
OTG_STATE_A_WAIT_BCON.

Regards,
-Bin.

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

[Tony Lindgren]

* Bin Liu <b-liu@ti.com> [170511 12:23]:
> On Thu, May 11, 2017 at 02:10:05PM -0500, Bin Liu wrote:
> [...]
> > > > > The otg state machine implementation in the musb drivers are kind of strange.
> > > > > OTG_STATE_A_WAIT_BCON suppose to be a steady state when no usb device is
> > > > > attached, but the musb drivers use it as a transient state to handle error
> > > > > cases, such as overcurrent ot HNP timeout, which is done in the 'case
> > > > > OTG_STATE_A_WAIT_BCON' branch in otg_timer() (or dsps_check_status() for dsps
> > > > > glue).
> > > > > 
> > > > > Then later when 2f3fd2c5bde1 adds
> > > > > 
> > > > > -       /* Poll for ID change in OTG port mode */
> > > > > -       if (musb->xceiv->otg->state == OTG_STATE_B_IDLE &&
> > > > > -                       musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE)
> > > > > +       /* Poll for ID change and connect */
> > > > > +       switch (musb->xceiv->otg->state) {
> > > > > +       case OTG_STATE_B_IDLE:
> > > > > +       case OTG_STATE_A_WAIT_BCON:
> > > > >                 mod_timer(&glue->timer, jiffies +
> > > > >                                 msecs_to_jiffies(wrp->poll_timeout));
> > > > > +               break;
> > > > > 
> > > > > which causes dsps_check_status (or otg_timer()) got called for a normal
> > > > > condition with OTG_STATE_A_WAIT_BCON, then turns off VBUS...
> > > > > 
> > > > > Will try to see how to solve this...
> > > > 
> > > > Maybe we just need to add back the earlier check for non-OTG devices
> > > > "musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE" into case
> > > > OTG_STATE_A_WAIT_BCON?
> > > 
> > > Not sure if it will work. The check is to kick out device-only mode.
> > > DUAL_ROLE means both OTG and host-only.
> > > 
> > > > 
> > > > Not sure if am335x configured with host port ever idle that way,
> > > > but maybe VBUS can be kept on with musb idle.
> > > 
> > > Just tried to remove 
> > > 
> > > +       case OTG_STATE_A_WAIT_BCON:
> > > 
> > > for the polling, but the port stops detecting device attach, musb
> > > power/devctl registers are good, trying to see why... maybe related to
> > > PM state?
> > 
> > usb_otg_hs is idle, so not able to detect attach. So need to get the
> > timer rolling in here to make it detect working.
> > Maybe need to add a flag in dsps_check_status() to not turn off vbus in
> > this case? Really don't like the idea...

Yeah that's what I recall too. The timer is needed until the m3 can
implement an irqchip and we can set that up as the irq for the host
only port.

> I am not sure I gave out the picture clearly.
> 
> Before added the runtime PM support, in host mode (devctl=0x19) the
> otg_timer is stopped, the controller is ready to detect attach.
> 
> Later when added the runtime PM support, usb_otg_hs hwmod becomes idle
> when a device is detached, so the otg_timer is needed to keep hwmod
> enable/idle periodically to detect attach, which triggers the issue
> because otg_timer() turns off vbus unconditionally when otg state is
> OTG_STATE_A_WAIT_BCON.

I wonder if just keeping VBUS on longer in OTG_STATE_A_WAIT_BCON
solves this issue? It seems the issue is with modems that get
reconfigured after the initial enumeration?

We could poll for new devices every 2 seconds, if anything is seen
on the bus, keep VBUS on at least 20 seconds, then if nothing is
found, poll every 2 seconds again.

Regards,

Tony

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

[Bin Liu]

On Thu, May 11, 2017 at 12:38:11PM -0700, Tony Lindgren wrote:
> * Bin Liu <b-liu@ti.com> [170511 12:23]:
> > On Thu, May 11, 2017 at 02:10:05PM -0500, Bin Liu wrote:
> > [...]
> > > > > > The otg state machine implementation in the musb drivers are kind of strange.
> > > > > > OTG_STATE_A_WAIT_BCON suppose to be a steady state when no usb device is
> > > > > > attached, but the musb drivers use it as a transient state to handle error
> > > > > > cases, such as overcurrent ot HNP timeout, which is done in the 'case
> > > > > > OTG_STATE_A_WAIT_BCON' branch in otg_timer() (or dsps_check_status() for dsps
> > > > > > glue).
> > > > > > 
> > > > > > Then later when 2f3fd2c5bde1 adds
> > > > > > 
> > > > > > -       /* Poll for ID change in OTG port mode */
> > > > > > -       if (musb->xceiv->otg->state == OTG_STATE_B_IDLE &&
> > > > > > -                       musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE)
> > > > > > +       /* Poll for ID change and connect */
> > > > > > +       switch (musb->xceiv->otg->state) {
> > > > > > +       case OTG_STATE_B_IDLE:
> > > > > > +       case OTG_STATE_A_WAIT_BCON:
> > > > > >                 mod_timer(&glue->timer, jiffies +
> > > > > >                                 msecs_to_jiffies(wrp->poll_timeout));
> > > > > > +               break;
> > > > > > 
> > > > > > which causes dsps_check_status (or otg_timer()) got called for a normal
> > > > > > condition with OTG_STATE_A_WAIT_BCON, then turns off VBUS...
> > > > > > 
> > > > > > Will try to see how to solve this...
> > > > > 
> > > > > Maybe we just need to add back the earlier check for non-OTG devices
> > > > > "musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE" into case
> > > > > OTG_STATE_A_WAIT_BCON?
> > > > 
> > > > Not sure if it will work. The check is to kick out device-only mode.
> > > > DUAL_ROLE means both OTG and host-only.
> > > > 
> > > > > 
> > > > > Not sure if am335x configured with host port ever idle that way,
> > > > > but maybe VBUS can be kept on with musb idle.
> > > > 
> > > > Just tried to remove 
> > > > 
> > > > +       case OTG_STATE_A_WAIT_BCON:
> > > > 
> > > > for the polling, but the port stops detecting device attach, musb
> > > > power/devctl registers are good, trying to see why... maybe related to
> > > > PM state?
> > > 
> > > usb_otg_hs is idle, so not able to detect attach. So need to get the
> > > timer rolling in here to make it detect working.
> > > Maybe need to add a flag in dsps_check_status() to not turn off vbus in
> > > this case? Really don't like the idea...
> 
> Yeah that's what I recall too. The timer is needed until the m3 can
> implement an irqchip and we can set that up as the irq for the host
> only port.
> 
> > I am not sure I gave out the picture clearly.
> > 
> > Before added the runtime PM support, in host mode (devctl=0x19) the
> > otg_timer is stopped, the controller is ready to detect attach.
> > 
> > Later when added the runtime PM support, usb_otg_hs hwmod becomes idle
> > when a device is detached, so the otg_timer is needed to keep hwmod
> > enable/idle periodically to detect attach, which triggers the issue
> > because otg_timer() turns off vbus unconditionally when otg state is
> > OTG_STATE_A_WAIT_BCON.
> 
> I wonder if just keeping VBUS on longer in OTG_STATE_A_WAIT_BCON
> solves this issue?

We don't cut VBUS intentionally for host mode (when devctl=0x19). The
VBUS got cut in this case only because when a device is detached, the
otg state changes from A_HOST -> A_WAIT_BCON, then otg_timer() cuts VBUS
and sets the state to A_IDLE, then next otg_timer() turns on VBUS (I
haven't check how the otg state becomes A_WAIT_VRISE from here).

Not sure how to *easily* keep VBUS here, without adding condition check
in otg_timer() for TG_STATE_A_WAIT_BCON.

> solves this issue? It seems the issue is with modems that get
> reconfigured after the initial enumeration?

Idealy we shouldn't cut VBUS at all in this case for host-only. I am
looking for a small patch to solve this, if possilbe ;)

> 
> We could poll for new devices every 2 seconds, if anything is seen
> on the bus, keep VBUS on at least 20 seconds, then if nothing is
> found, poll every 2 seconds again.

I am not sure this is relevant, VBUS is constantly on once the otg stage
becomes A_HOST -> A_WAIT_BCON -> A_IDLE -> A_WAIT_VRISE, within a
second.

Regards,
-Bin.

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

[Tony Lindgren]

* Bin Liu <b-liu@ti.com> [170511 13:05]:
> On Thu, May 11, 2017 at 12:38:11PM -0700, Tony Lindgren wrote:
> > 
> > I wonder if just keeping VBUS on longer in OTG_STATE_A_WAIT_BCON
> > solves this issue?
> 
> We don't cut VBUS intentionally for host mode (when devctl=0x19). The
> VBUS got cut in this case only because when a device is detached, the
> otg state changes from A_HOST -> A_WAIT_BCON, then otg_timer() cuts VBUS
> and sets the state to A_IDLE, then next otg_timer() turns on VBUS (I
> haven't check how the otg state becomes A_WAIT_VRISE from here).
> 
> Not sure how to *easily* keep VBUS here, without adding condition check
> in otg_timer() for TG_STATE_A_WAIT_BCON.
> 
> > solves this issue? It seems the issue is with modems that get
> > reconfigured after the initial enumeration?
> 
> Idealy we shouldn't cut VBUS at all in this case for host-only. I am
> looking for a small patch to solve this, if possilbe ;)

Maybe try something like below, compile tested only. I don't
think I have any USB modem here to test with.

> > We could poll for new devices every 2 seconds, if anything is seen
> > on the bus, keep VBUS on at least 20 seconds, then if nothing is
> > found, poll every 2 seconds again.
> 
> I am not sure this is relevant, VBUS is constantly on once the otg stage
> becomes A_HOST -> A_WAIT_BCON -> A_IDLE -> A_WAIT_VRISE, within a
> second.

Yup, but it sounds like once the modem changes mode, it disappears
from the USB bus for a long enough time where we go to A_WAIT_BCON
again. Or else I'm misunderstanding what's going on.

It's also possible that we have dsps_check_status() getting called
again on disconnect before the new 20 second period is over, I did
not check for that yet.

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
@@ -270,6 +270,10 @@ static int dsps_check_status(struct musb *musb, void *unused)
 		musb_writel(musb->ctrl_base, wrp->coreintr_set,
 			    MUSB_INTR_VBUSERROR << wrp->usb_shift);
 		break;
+	case OTG_STATE_A_HOST:
+		if (glue->vbus_irq)
+			dsps_mod_timer(glue, 20000);	/* 20s */
+		break;
 	default:
 		break;
 	}
-- 
2.13.0

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

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170511 13:26]:
> 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
> @@ -270,6 +270,10 @@ static int dsps_check_status(struct musb *musb, void *unused)
>  		musb_writel(musb->ctrl_base, wrp->coreintr_set,
>  			    MUSB_INTR_VBUSERROR << wrp->usb_shift);
>  		break;
> +	case OTG_STATE_A_HOST:
> +		if (glue->vbus_irq)
> +			dsps_mod_timer(glue, 20000);	/* 20s */
> +		break;
>  	default:
>  		break;
>  	}
> -- 

Heh that's the wrong way around.. I think we can leave out the check
for vbus_irq.

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
@@ -270,6 +270,10 @@ static int dsps_check_status(struct musb *musb, void *unused)
 		musb_writel(musb->ctrl_base, wrp->coreintr_set,
 			    MUSB_INTR_VBUSERROR << wrp->usb_shift);
 		break;
+	case OTG_STATE_A_HOST:
+		/* Keep VBUS on after disconnect for 20 seconds */
+		dsps_mod_timer(glue, 20000);
+		break;
 	default:
 		break;
 	}
-- 
2.13.0

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

[Bin Liu]

On Thu, May 11, 2017 at 01:23:06PM -0700, Tony Lindgren wrote:
> * Bin Liu <b-liu@ti.com> [170511 13:05]:
> > On Thu, May 11, 2017 at 12:38:11PM -0700, Tony Lindgren wrote:
> > > 
> > > I wonder if just keeping VBUS on longer in OTG_STATE_A_WAIT_BCON
> > > solves this issue?
> > 
> > We don't cut VBUS intentionally for host mode (when devctl=0x19). The
> > VBUS got cut in this case only because when a device is detached, the
> > otg state changes from A_HOST -> A_WAIT_BCON, then otg_timer() cuts VBUS
> > and sets the state to A_IDLE, then next otg_timer() turns on VBUS (I
> > haven't check how the otg state becomes A_WAIT_VRISE from here).
> > 
> > Not sure how to *easily* keep VBUS here, without adding condition check
> > in otg_timer() for TG_STATE_A_WAIT_BCON.
> > 
> > > solves this issue? It seems the issue is with modems that get
> > > reconfigured after the initial enumeration?
> > 
> > Idealy we shouldn't cut VBUS at all in this case for host-only. I am
> > looking for a small patch to solve this, if possilbe ;)
> 
> Maybe try something like below, compile tested only. I don't
> think I have any USB modem here to test with.

The patch below doesn't help. In device detach by the time
dsps_check_status() is called, the otg state is already A_WAIT_BCON, set
by musb_root_disconnect() in musb_stage0_irq() when handling DISCONNECT
interrupt.

> 
> > > We could poll for new devices every 2 seconds, if anything is seen
> > > on the bus, keep VBUS on at least 20 seconds, then if nothing is
> > > found, poll every 2 seconds again.
> > 
> > I am not sure this is relevant, VBUS is constantly on once the otg stage
> > becomes A_HOST -> A_WAIT_BCON -> A_IDLE -> A_WAIT_VRISE, within a
> > second.
> 
> Yup, but it sounds like once the modem changes mode, it disappears
> from the USB bus for a long enough time where we go to A_WAIT_BCON
> again. Or else I'm misunderstanding what's going on.

I don't think it is about how long the modem disappears. When detach happens,
DISCONNECT interrupt happens, then dsps_check_status() is scheduled with
state A_WAIT_BCON, then VBUS got cut.

BTY, I didn't debug with a modem, just with a device detach. Tring to
see how to not cut vbus at all in dsps_check_status().

> 
> It's also possible that we have dsps_check_status() getting called
> again on disconnect before the new 20 second period is over, I did
> not check for that yet.

After a few trials, I start to think about a little cleanup in the otg
state machine in the musb drivers, I think we need to somehow
distinguish between normal and error conditions for A_WAIT_BCON state.

Regards,
-Bin.

> 
> 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
> @@ -270,6 +270,10 @@ static int dsps_check_status(struct musb *musb, void *unused)
>  		musb_writel(musb->ctrl_base, wrp->coreintr_set,
>  			    MUSB_INTR_VBUSERROR << wrp->usb_shift);
>  		break;
> +	case OTG_STATE_A_HOST:
> +		if (glue->vbus_irq)
> +			dsps_mod_timer(glue, 20000);	/* 20s */
> +		break;
>  	default:
>  		break;
>  	}
> -- 
> 2.13.0

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

[Tony Lindgren]

* Bin Liu <b-liu@ti.com> [170511 13:47]:
> On Thu, May 11, 2017 at 01:23:06PM -0700, Tony Lindgren wrote:
> > 
> > Maybe try something like below, compile tested only. I don't
> > think I have any USB modem here to test with.
> 
> The patch below doesn't help. In device detach by the time
> dsps_check_status() is called, the otg state is already A_WAIT_BCON, set
> by musb_root_disconnect() in musb_stage0_irq() when handling DISCONNECT
> interrupt.

Oh OK.

> I don't think it is about how long the modem disappears. When detach happens,
> DISCONNECT interrupt happens, then dsps_check_status() is scheduled with
> state A_WAIT_BCON, then VBUS got cut.
> 
> BTY, I didn't debug with a modem, just with a device detach. Tring to
> see how to not cut vbus at all in dsps_check_status().

OK

> > It's also possible that we have dsps_check_status() getting called
> > again on disconnect before the new 20 second period is over, I did
> > not check for that yet.
> 
> After a few trials, I start to think about a little cleanup in the otg
> state machine in the musb drivers, I think we need to somehow
> distinguish between normal and error conditions for A_WAIT_BCON state.

Well maybe the minimal fix for now is just pretty much back to
square one of this thread. This should keep VBUS always on.
Then we can figure out some logic to cut VBUS later on.

And yeah, the state machine is really hard to follow so some kind
of clean up would be nice.

Regards,

Tony

8< -------------------
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -245,7 +245,6 @@ 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);
 		skip_session = 1;
 		/* fall */
 

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

[Bin Liu]

On Thu, May 11, 2017 at 02:06:28PM -0700, Tony Lindgren wrote:
> 
> Well maybe the minimal fix for now is just pretty much back to
> square one of this thread. This should keep VBUS always on.
> Then we can figure out some logic to cut VBUS later on.
> 
> And yeah, the state machine is really hard to follow so some kind
> of clean up would be nice.

Okay, figured out why clearing session in OTG_STATE_A_WAIT_BCON, it is
not for error condition handling (which is done in musb-core), but for
going back to b_idle state from a_host for dual-role mode. otg_timer()
(now is dsps_check_status()) was only called for otg port originally, so
it wasn't an issue, until started calling it for host mode as well when
runtime PM was added.
> 
> Regards,
> 
> Tony
> 
> 8< -------------------
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -245,7 +245,6 @@ 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);
>  		skip_session = 1;
>  		/* fall */
>  

So the above patch breaks otg port when switching from host to device
mode. The following change should solve it. But Tony do you see any way
to improve it with glue->vbus_irq?

Regards,
-Bin.

8< --------------------
diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 9c7ee26ef388..465281244596 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -245,9 +245,14 @@ static int dsps_check_status(struct musb *musb, void *unused)
                dsps_mod_timer_optional(glue);
                break;
        case OTG_STATE_A_WAIT_BCON:
+               /* keep VBUS on for host-only mode */
+               if (musb->port_mode == MUSB_PORT_MODE_HOST) {
+                       dsps_mod_timer_optional(glue);
+                       break;
+               }
                musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
                skip_session = 1;
-               /* fall */
+               /* fall through */
 
        case OTG_STATE_A_IDLE:
        case OTG_STATE_B_IDLE:

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

[Tony Lindgren]

* Bin Liu <b-liu@ti.com> [170512 06:43]:
> On Thu, May 11, 2017 at 02:06:28PM -0700, Tony Lindgren wrote:
> > 
> > Well maybe the minimal fix for now is just pretty much back to
> > square one of this thread. This should keep VBUS always on.
> > Then we can figure out some logic to cut VBUS later on.
> > 
> > And yeah, the state machine is really hard to follow so some kind
> > of clean up would be nice.
> 
> Okay, figured out why clearing session in OTG_STATE_A_WAIT_BCON, it is
> not for error condition handling (which is done in musb-core), but for
> going back to b_idle state from a_host for dual-role mode. otg_timer()
> (now is dsps_check_status()) was only called for otg port originally, so
> it wasn't an issue, until started calling it for host mode as well when
> runtime PM was added.

OK makes sense.

> > 8< -------------------
> > --- a/drivers/usb/musb/musb_dsps.c
> > +++ b/drivers/usb/musb/musb_dsps.c
> > @@ -245,7 +245,6 @@ 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);
> >  		skip_session = 1;
> >  		/* fall */
> >  
> 
> So the above patch breaks otg port when switching from host to device
> mode. The following change should solve it. But Tony do you see any way
> to improve it with glue->vbus_irq?

OK. No better ideas except I think we should probably have a separate
timer for keeping VBUS on after state changes eventually.

I guess the real test here would be to connect a USB modem that
changes state to the am335x-evm OTG port and make sure it works
with commit similar to d680414d0f42 ("ARM: dts: Configure BeagleBone
peripheral USB VBUS irq"). And also without configuring the vusb_irq.

For the patch below, seems like the way to go for the fix assuming
it fixes the $subject issue:

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

> 8< --------------------
> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> index 9c7ee26ef388..465281244596 100644
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -245,9 +245,14 @@ static int dsps_check_status(struct musb *musb, void *unused)
>                 dsps_mod_timer_optional(glue);
>                 break;
>         case OTG_STATE_A_WAIT_BCON:
> +               /* keep VBUS on for host-only mode */
> +               if (musb->port_mode == MUSB_PORT_MODE_HOST) {
> +                       dsps_mod_timer_optional(glue);
> +                       break;
> +               }
>                 musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
>                 skip_session = 1;
> -               /* fall */
> +               /* fall through */
>  
>         case OTG_STATE_A_IDLE:
>         case OTG_STATE_B_IDLE:

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

[Bin Liu]

On Fri, May 12, 2017 at 07:58:49AM -0700, Tony Lindgren wrote:
> * Bin Liu <b-liu@ti.com> [170512 06:43]:
> > On Thu, May 11, 2017 at 02:06:28PM -0700, Tony Lindgren wrote:
> > > 
> > > Well maybe the minimal fix for now is just pretty much back to
> > > square one of this thread. This should keep VBUS always on.
> > > Then we can figure out some logic to cut VBUS later on.
> > > 
> > > And yeah, the state machine is really hard to follow so some kind
> > > of clean up would be nice.
> > 
> > Okay, figured out why clearing session in OTG_STATE_A_WAIT_BCON, it is
> > not for error condition handling (which is done in musb-core), but for
> > going back to b_idle state from a_host for dual-role mode. otg_timer()
> > (now is dsps_check_status()) was only called for otg port originally, so
> > it wasn't an issue, until started calling it for host mode as well when
> > runtime PM was added.
> 
> OK makes sense.
> 
> > > 8< -------------------
> > > --- a/drivers/usb/musb/musb_dsps.c
> > > +++ b/drivers/usb/musb/musb_dsps.c
> > > @@ -245,7 +245,6 @@ 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);
> > >  		skip_session = 1;
> > >  		/* fall */
> > >  
> > 
> > So the above patch breaks otg port when switching from host to device
> > mode. The following change should solve it. But Tony do you see any way
> > to improve it with glue->vbus_irq?
> 
> OK. No better ideas except I think we should probably have a separate
> timer for keeping VBUS on after state changes eventually.

Currently with the patch below, VBUS is constantly on for host-only
mode, and this is what we want. Why we need a separate timer? No one
cuts VBUs now for host-only mode.

> 
> I guess the real test here would be to connect a USB modem that
> changes state to the am335x-evm OTG port and make sure it works
> with commit similar to d680414d0f42 ("ARM: dts: Configure BeagleBone
> peripheral USB VBUS irq"). And also without configuring the vusb_irq.

I will test w/ and w/o vbus_irq on BeagleBone.

Moreno, would you mind to test the patch below with your modem?

Thanks,
-Bin.

> 
> For the patch below, seems like the way to go for the fix assuming
> it fixes the $subject issue:
> 
> Acked-by: Tony Lindgren <tony@atomide.com>
> 
> > 8< --------------------
> > diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> > index 9c7ee26ef388..465281244596 100644
> > --- a/drivers/usb/musb/musb_dsps.c
> > +++ b/drivers/usb/musb/musb_dsps.c
> > @@ -245,9 +245,14 @@ static int dsps_check_status(struct musb *musb, void *unused)
> >                 dsps_mod_timer_optional(glue);
> >                 break;
> >         case OTG_STATE_A_WAIT_BCON:
> > +               /* keep VBUS on for host-only mode */
> > +               if (musb->port_mode == MUSB_PORT_MODE_HOST) {
> > +                       dsps_mod_timer_optional(glue);
> > +                       break;
> > +               }
> >                 musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
> >                 skip_session = 1;
> > -               /* fall */
> > +               /* fall through */
> >  
> >         case OTG_STATE_A_IDLE:
> >         case OTG_STATE_B_IDLE:

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

[Moreno Bartalucci]

> Il giorno 12 mag 2017, alle ore 17:21, Bin Liu <b-liu@ti.com> ha scritto:
> 
> On Fri, May 12, 2017 at 07:58:49AM -0700, Tony Lindgren wrote:
>> * Bin Liu <b-liu@ti.com> [170512 06:43]:
>>> On Thu, May 11, 2017 at 02:06:28PM -0700, Tony Lindgren wrote:
>>>> 
>>>> Well maybe the minimal fix for now is just pretty much back to
>>>> square one of this thread. This should keep VBUS always on.
>>>> Then we can figure out some logic to cut VBUS later on.
>>>> 
>>>> And yeah, the state machine is really hard to follow so some kind
>>>> of clean up would be nice.
>>> 
>>> Okay, figured out why clearing session in OTG_STATE_A_WAIT_BCON, it is
>>> not for error condition handling (which is done in musb-core), but for
>>> going back to b_idle state from a_host for dual-role mode. otg_timer()
>>> (now is dsps_check_status()) was only called for otg port originally, so
>>> it wasn't an issue, until started calling it for host mode as well when
>>> runtime PM was added.
>> 
>> OK makes sense.
>> 
>>>> 8< -------------------
>>>> --- a/drivers/usb/musb/musb_dsps.c
>>>> +++ b/drivers/usb/musb/musb_dsps.c
>>>> @@ -245,7 +245,6 @@ 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);
>>>> 		skip_session = 1;
>>>> 		/* fall */
>>>> 
>>> 
>>> So the above patch breaks otg port when switching from host to device
>>> mode. The following change should solve it. But Tony do you see any way
>>> to improve it with glue->vbus_irq?
>> 
>> OK. No better ideas except I think we should probably have a separate
>> timer for keeping VBUS on after state changes eventually.
> 
> Currently with the patch below, VBUS is constantly on for host-only
> mode, and this is what we want. Why we need a separate timer? No one
> cuts VBUs now for host-only mode.
> 
>> 
>> I guess the real test here would be to connect a USB modem that
>> changes state to the am335x-evm OTG port and make sure it works
>> with commit similar to d680414d0f42 ("ARM: dts: Configure BeagleBone
>> peripheral USB VBUS irq"). And also without configuring the vusb_irq.
> 
> I will test w/ and w/o vbus_irq on BeagleBone.
> 
> Moreno, would you mind to test the patch below with your modem?
> 
> Thanks,
> -Bin.
> 
>> 
>> For the patch below, seems like the way to go for the fix assuming
>> it fixes the $subject issue:
>> 
>> Acked-by: Tony Lindgren <tony@atomide.com>
>> 
>>> 8< --------------------
>>> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
>>> index 9c7ee26ef388..465281244596 100644
>>> --- a/drivers/usb/musb/musb_dsps.c
>>> +++ b/drivers/usb/musb/musb_dsps.c
>>> @@ -245,9 +245,14 @@ static int dsps_check_status(struct musb *musb, void *unused)
>>>                dsps_mod_timer_optional(glue);
>>>                break;
>>>        case OTG_STATE_A_WAIT_BCON:
>>> +               /* keep VBUS on for host-only mode */
>>> +               if (musb->port_mode == MUSB_PORT_MODE_HOST) {
>>> +                       dsps_mod_timer_optional(glue);
>>> +                       break;
>>> +               }
>>>                musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
>>>                skip_session = 1;
>>> -               /* fall */
>>> +               /* fall through */
>>> 
>>>        case OTG_STATE_A_IDLE:
>>>        case OTG_STATE_B_IDLE:

Hello Bin,

today is too late, here.

I’ll try to test it on Monday.

Best regards,

Moreno

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

[Tony Lindgren]

* Bin Liu <b-liu@ti.com> [170512 08:24]:
> On Fri, May 12, 2017 at 07:58:49AM -0700, Tony Lindgren wrote:
> > OK. No better ideas except I think we should probably have a separate
> > timer for keeping VBUS on after state changes eventually.
> 
> Currently with the patch below, VBUS is constantly on for host-only
> mode, and this is what we want. Why we need a separate timer? No one
> cuts VBUs now for host-only mode.

Oh I was just thinking what we might want to do in the future if
we want to cut off VBUS when no devices are connected. If we have
a USB modem for example it might first enumerate as some boot device,
then nothing for 20 seconds while it's booting, and then we have a
different device enumerating after the modem has booted. During this
period we want to keep VBUS on and will go through multiple
OTG_STATE_A_WAIT_BCON states. So we can't really control VBUS using
the OTG_STATE_WHATEVER alone.

Regards,

Tony

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

[Bin Liu]

On Fri, May 12, 2017 at 10:21:35AM -0700, Tony Lindgren wrote:
> * Bin Liu <b-liu@ti.com> [170512 08:24]:
> > On Fri, May 12, 2017 at 07:58:49AM -0700, Tony Lindgren wrote:
> > > OK. No better ideas except I think we should probably have a separate
> > > timer for keeping VBUS on after state changes eventually.
> > 
> > Currently with the patch below, VBUS is constantly on for host-only
> > mode, and this is what we want. Why we need a separate timer? No one
> > cuts VBUs now for host-only mode.
> 
> Oh I was just thinking what we might want to do in the future if
> we want to cut off VBUS when no devices are connected. If we have

Okay, I see. But I don't think we will ever want to turn off VBUS when
no devices attached for host-only mode. Any other controllers do this?

Turning off VBUS doesn't save us much, because it comes from an external
power rail, and no one consumes it when no devices are attached.

I believe keeping the controller idle as what we have now is sufficient.

Regards,
-Bin.

> a USB modem for example it might first enumerate as some boot device,
> then nothing for 20 seconds while it's booting, and then we have a
> different device enumerating after the modem has booted. During this
> period we want to keep VBUS on and will go through multiple
> OTG_STATE_A_WAIT_BCON states. So we can't really control VBUS using
> the OTG_STATE_WHATEVER alone.
> 
> Regards,
> 
> Tony

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

[Tony Lindgren]

* Bin Liu <b-liu@ti.com> [170512 10:43]:
> On Fri, May 12, 2017 at 10:21:35AM -0700, Tony Lindgren wrote:
> > * Bin Liu <b-liu@ti.com> [170512 08:24]:
> > > On Fri, May 12, 2017 at 07:58:49AM -0700, Tony Lindgren wrote:
> > > > OK. No better ideas except I think we should probably have a separate
> > > > timer for keeping VBUS on after state changes eventually.
> > > 
> > > Currently with the patch below, VBUS is constantly on for host-only
> > > mode, and this is what we want. Why we need a separate timer? No one
> > > cuts VBUs now for host-only mode.
> > 
> > Oh I was just thinking what we might want to do in the future if
> > we want to cut off VBUS when no devices are connected. If we have
> 
> Okay, I see. But I don't think we will ever want to turn off VBUS when
> no devices attached for host-only mode. Any other controllers do this?
> 
> Turning off VBUS doesn't save us much, because it comes from an external
> power rail, and no one consumes it when no devices are attached.
> 
> I believe keeping the controller idle as what we have now is sufficient.

OK fine with me.

Regards,

Tony

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

[Moreno Bartalucci]

> Il giorno 12 mag 2017, alle ore 17:21, Bin Liu <b-liu@ti.com> ha scritto:
> 
> […]
> 
> Moreno, would you mind to test the patch below with your modem?
> 
> […]
>> 
>>> 8< --------------------
>>> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
>>> index 9c7ee26ef388..465281244596 100644
>>> --- a/drivers/usb/musb/musb_dsps.c
>>> +++ b/drivers/usb/musb/musb_dsps.c
>>> @@ -245,9 +245,14 @@ static int dsps_check_status(struct musb *musb, void *unused)
>>>                dsps_mod_timer_optional(glue);
>>>                break;
>>>        case OTG_STATE_A_WAIT_BCON:
>>> +               /* keep VBUS on for host-only mode */
>>> +               if (musb->port_mode == MUSB_PORT_MODE_HOST) {
>>> +                       dsps_mod_timer_optional(glue);
>>> +                       break;
>>> +               }
>>>                musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
>>>                skip_session = 1;
>>> -               /* fall */
>>> +               /* fall through */
>>> 
>>>        case OTG_STATE_A_IDLE:
>>>        case OTG_STATE_B_IDLE:

Hello Bin,

I tested the above patch with my device and it seems to work correctly with the current mainline kernel.

I tested it with my “production” kernel as well (4.9.20) but, for it to work, I had to change it slightly.

This is what I tested for kernel 4.9.20:

--- a/drivers/usb/musb/musb_dsps.c	2017-05-15 08:40:23.000000000 +0200
+++ b/drivers/usb/musb/musb_dsps.c	2017-05-15 08:49:17.000000000 +0200
@@ -213,6 +213,12 @@ static int dsps_check_status(struct musb
 				msecs_to_jiffies(wrp->poll_timeout));
 		break;
 	case OTG_STATE_A_WAIT_BCON:
+		/* keep VBUS on for host-only mode */
+		if (musb->port_mode == MUSB_PORT_MODE_HOST) {
+			mod_timer(&glue->timer, jiffies +
+					msecs_to_jiffies(wrp->poll_timeout));
+			break;
+		}
 		musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
 		skip_session = 1;
 		/* fall */

In this form, it appears to work properly for 4.9.20 too.

Best regards,

Moreno

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

[Bin Liu]

On Mon, May 15, 2017 at 09:07:10AM +0200, Moreno Bartalucci wrote:
> 
> > Il giorno 12 mag 2017, alle ore 17:21, Bin Liu <b-liu@ti.com> ha scritto:
> > 
> > […]
> > 
> > Moreno, would you mind to test the patch below with your modem?
> > 
> > […]
> >> 
> >>> 8< --------------------
> >>> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> >>> index 9c7ee26ef388..465281244596 100644
> >>> --- a/drivers/usb/musb/musb_dsps.c
> >>> +++ b/drivers/usb/musb/musb_dsps.c
> >>> @@ -245,9 +245,14 @@ static int dsps_check_status(struct musb *musb, void *unused)
> >>>                dsps_mod_timer_optional(glue);
> >>>                break;
> >>>        case OTG_STATE_A_WAIT_BCON:
> >>> +               /* keep VBUS on for host-only mode */
> >>> +               if (musb->port_mode == MUSB_PORT_MODE_HOST) {
> >>> +                       dsps_mod_timer_optional(glue);
> >>> +                       break;
> >>> +               }
> >>>                musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
> >>>                skip_session = 1;
> >>> -               /* fall */
> >>> +               /* fall through */
> >>> 
> >>>        case OTG_STATE_A_IDLE:
> >>>        case OTG_STATE_B_IDLE:
> 
> Hello Bin,
> 
> I tested the above patch with my device and it seems to work correctly with the current mainline kernel.
> 
> I tested it with my “production” kernel as well (4.9.20) but, for it to work, I had to change it slightly.

Thanks for testing.

> 
> This is what I tested for kernel 4.9.20:
> 
> --- a/drivers/usb/musb/musb_dsps.c	2017-05-15 08:40:23.000000000 +0200
> +++ b/drivers/usb/musb/musb_dsps.c	2017-05-15 08:49:17.000000000 +0200
> @@ -213,6 +213,12 @@ static int dsps_check_status(struct musb
>  				msecs_to_jiffies(wrp->poll_timeout));
>  		break;
>  	case OTG_STATE_A_WAIT_BCON:
> +		/* keep VBUS on for host-only mode */
> +		if (musb->port_mode == MUSB_PORT_MODE_HOST) {
> +			mod_timer(&glue->timer, jiffies +
> +					msecs_to_jiffies(wrp->poll_timeout));
> +			break;
> +		}
>  		musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
>  		skip_session = 1;
>  		/* fall */
> 
> In this form, it appears to work properly for 4.9.20 too.

Yeah, the mod_timer() call is wrapped into dsps_mod_timer_optional() by
369469a92393d ("usb: musb: Add support for optional VBUS irq to dsps
glue layer"), introduced in v4.11-rc1.

Regards,
-Bin.