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

Groups > linux.kernel > #1639964 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Bin Liu <b-liu@ti.com> - 2017-05-11 21:00 +0200
    Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Tony Lindgren <tony@atomide.com> - 2017-05-11 21:00 +0200
      Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Bin Liu <b-liu@ti.com> - 2017-05-11 21:10 +0200
        Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Bin Liu <b-liu@ti.com> - 2017-05-11 21:20 +0200
          Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Bin Liu <b-liu@ti.com> - 2017-05-11 21:30 +0200
            Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Tony Lindgren <tony@atomide.com> - 2017-05-11 21:40 +0200
              Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Bin Liu <b-liu@ti.com> - 2017-05-11 22:10 +0200
                Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Tony Lindgren <tony@atomide.com> - 2017-05-11 22:30 +0200
                Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Tony Lindgren <tony@atomide.com> - 2017-05-11 22:30 +0200
                  Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Bin Liu <b-liu@ti.com> - 2017-05-11 22:50 +0200
                    Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Tony Lindgren <tony@atomide.com> - 2017-05-11 23:10 +0200
                      Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Bin Liu <b-liu@ti.com> - 2017-05-12 15:50 +0200
                        Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Tony Lindgren <tony@atomide.com> - 2017-05-12 17:00 +0200
                          Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Bin Liu <b-liu@ti.com> - 2017-05-12 17:30 +0200
                            Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Moreno Bartalucci <moreno.bartalucci@tecnorama.it> - 2017-05-12 18:10 +0200
                            Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Tony Lindgren <tony@atomide.com> - 2017-05-12 19:30 +0200
                              Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Bin Liu <b-liu@ti.com> - 2017-05-12 19:50 +0200
                                Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Tony Lindgren <tony@atomide.com> - 2017-05-12 19:50 +0200
                            Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Moreno Bartalucci <moreno.bartalucci@tecnorama.it> - 2017-05-15 09:10 +0200
                              Re: [PATCH] usb-musb: keep VBUS on when device is disconnected Bin Liu <b-liu@ti.com> - 2017-05-15 14:30 +0200

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

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.

[toc] | [next] | [standalone]

#1639966

* 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

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

#1639969

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.

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

#1639973

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.

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

#1639979

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.

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

#1639982

* 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

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

#1639994

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.

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

#1640005

* 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

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

#1640007

* 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

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

#1640012

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

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

#1640037

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

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

#1640451

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:

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

#1640521

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

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

#1640542

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:

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

#1640562

> 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

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

#1640651

* 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

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

#1640665

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

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

#1640666

* 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

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

#1641319

> 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

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

#1641618

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web