| Message ID | 20170512134042.GK7154@uda0271908 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* 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: -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: