On Thu, Feb 26, 2015 at 02:38:07PM -0600, Bin Liu wrote:
> On Thu, Feb 26, 2015 at 2:31 PM, Felipe Balbi <balbi@xxxxxx> wrote:
> > On Thu, Feb 26, 2015 at 02:25:20PM -0600, Bin Liu wrote:
> >> On Thu, Feb 26, 2015 at 2:19 PM, Felipe Balbi <balbi@xxxxxx> wrote:
> >> > Hi again,
> >> >
> >> > On Thu, Feb 26, 2015 at 02:15:40PM -0600, Felipe Balbi wrote:
> >> >> > > >> >> > On Thu, Feb 26, 2015 at 12:25:16PM -0600, Felipe Balbi wrote:
> >> >> > > >> >> >> FSDEV is set for both HIGH and FULL speeds,
> >> >> > > >> >> >> the correct HIGHSPEED check is done through
> >> >> > > >> >> >> power register's HSMODE bit.
> >> >> > > >> >> >>
> >> >> > > >> >> >> Signed-off-by: Felipe Balbi <balbi@xxxxxx>
> >> >> > > >> >> >
> >> >> > > >> >> > I'm still unsure if we should really ignore babble on FS/LS. It seems to
> >> >> > > >> >> > me we should never ignore it, but I really don't have a way to prove
> >> >> > > >> >> > this statement. For the sake of reducing impact, we will just fix HS
> >> >> > > >> >> > check here.
> >> >> > > >> >> >
> >> >> > > >> >>
> >> >> > > >> >> I believe we should drop speed check in here and not ignore babble
> >> >> > > >> >> regardless. We have seen many cases that full-speed babble causes MUSB
> >> >> > > >> >> stop working.
> >> >> > > >> >
> >> >> > > >> > I'll make that as a separate patch then, just to make sure we can revert
> >> >> > > >> > it later if something goes wrong ;-)
> >> >> > > >>
> >> >> > > >> Agreed.
> >> >> > > >
> >> >> > > > I noticed something else. If we really don't need to reset musb in case
> >> >> > > > of babble, then we can drop that recover_work completely which
> >> >> > > > simplifies babble handling quite a bit.
> >> >> > > >
> >> >> > > > I'll fiddle with that, if you don't mind.
> >> >> > >
> >> >> > > That is fine with me. I am writing the comments for the dropping reset
> >> >> > > patch right now ;)
> >> >> > >
> >> >> > > We only need the following in musb_recover_work() for the recovery.
> >> >> > >
> >> >> > > 1852 /*
> >> >> > > 1853 * When a babble condition occurs, the musb controller
> >> >> > > 1854 * removes the session bit and the endpoint config is lost.
> >> >> > > 1855 */
> >> >> > > 1856 if (musb->dyn_fifo)
> >> >> > > 1857 status = ep_config_from_table(musb);
> >> >> > > 1858 else
> >> >> > > 1859 status = ep_config_from_hw(musb);
> >> >> > > 1860
> >> >> > > 1861 /* start the session again */
> >> >> > > 1862 if (status == 0)
> >> >> > > 1863 musb_start(musb);
> >> >> >
> >> >> > the statement that musb looses ep configuration seems bogus to me. I
> >> >> > dropped that too:
> >> >> >
> >> >> > commit 4a07d415bf5894c66f61827c30053a65d6dfce26
> >> >> > Author: Felipe Balbi <balbi@xxxxxx>
> >> >> > Date: Thu Feb 26 14:02:35 2015 -0600
> >> >> >
> >> >> > usb: musb: core: simplify musb_recover_work()
> >> >> >
> >> >> > we're not resetting musb at all, just restarting
> >> >> > the session. This means we don't need to touch PHYs
> >> >> > or VBUS or anything like that. Just make sure session
> >> >> > bit is reenabled after MUSB dropped it.
> >> >> >
> >> >> > Signed-off-by: Felipe Balbi <balbi@xxxxxx>
> >> >> >
> >> >> > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> >> >> > index d9c627d54db6..219636c1b020 100644
> >> >> > --- a/drivers/usb/musb/musb_core.c
> >> >> > +++ b/drivers/usb/musb/musb_core.c
> >> >> > @@ -1835,7 +1835,8 @@ static void musb_irq_work(struct work_struct *data)
> >> >> > static void musb_recover_work(struct work_struct *data)
> >> >> > {
> >> >> > struct musb *musb = container_of(data, struct musb, recover_work.work);
> >> >> > - int status, ret;
> >> >> > + int ret;
> >> >> > + u8 devctl;
> >> >> >
> >> >> > ret = musb_platform_reset(musb);
> >> >> > if (ret) {
> >> >> > @@ -1843,24 +1844,17 @@ static void musb_recover_work(struct work_struct *data)
> >> >> > return;
> >> >> > }
> >> >> >
> >> >> > - usb_phy_vbus_off(musb->xceiv);
> >> >> > - usleep_range(100, 200);
> >> >> > -
> >> >> > - usb_phy_vbus_on(musb->xceiv);
> >> >> > - usleep_range(100, 200);
> >> >> > + /* drop session bit */
> >> >> > + devctl = musb_readb(musb->mregs, MUSB_DEVCTL);
> >> >> > + devctl &= ~MUSB_DEVCTL_SESSION;
> >> >> > + musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
> >> >> >
> >> >> > - /*
> >> >> > - * When a babble condition occurs, the musb controller
> >> >> > - * removes the session bit and the endpoint config is lost.
> >> >> > - */
> >> >> > - if (musb->dyn_fifo)
> >> >> > - status = ep_config_from_table(musb);
> >> >> > - else
> >> >> > - status = ep_config_from_hw(musb);
> >> >> > + /* wait for session to really drop */
> >> >> > + udelay(100);
> >> >> >
> >> >> > - /* start the session again */
> >> >> > - if (status == 0)
> >> >> > - musb_start(musb);
> >> >> > + /* restart session */
> >> >> > + devctl |= MUSB_DEVCTL_SESSION;
> >> >> > + musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
> >> >> > }
> >> >> >
> >> >> > /* --------------------------------------------------------------------------
> >> >> >
> >> >> >
> >> >> > the only thing that we're still missing is a notification to usbcore
> >> >> > that we have a port being disabled, that's the only way to have testusb
> >> >> > know that it has been disconnected by the host.
> >> >>
> >> >> here's the final version:
> >> >>
> >> >> commit 3db11e7da5938bbd955410355194625f699a424c
> >> >> Author: Felipe Balbi <balbi@xxxxxx>
> >> >> Date: Thu Feb 26 14:02:35 2015 -0600
> >> >>
> >> >> usb: musb: core: simplify musb_recover_work()
> >> >>
> >> >> we're not resetting musb at all, just restarting
> >> >> the session. This means we don't need to touch PHYs
> >> >> or VBUS or anything like that. Just make sure session
> >> >> bit is reenabled after MUSB dropped it.
> >> >>
> >> >> while at that, make sure to tell usbcore that we're
> >> >> dropping the session and, thus, disconnecting the
> >> >> device.
> >> >>
> >> >> Signed-off-by: Felipe Balbi <balbi@xxxxxx>
> >> >>
> >> >> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> >> >> index d9c627d54db6..64cb242d92f7 100644
> >> >> --- a/drivers/usb/musb/musb_core.c
> >> >> +++ b/drivers/usb/musb/musb_core.c
> >> >> @@ -1835,7 +1835,8 @@ static void musb_irq_work(struct work_struct *data)
> >> >> static void musb_recover_work(struct work_struct *data)
> >> >> {
> >> >> struct musb *musb = container_of(data, struct musb, recover_work.work);
> >> >> - int status, ret;
> >> >> + int ret;
> >> >> + u8 devctl;
> >> >>
> >> >> ret = musb_platform_reset(musb);
> >> >> if (ret) {
> >> >> @@ -1843,24 +1844,17 @@ static void musb_recover_work(struct work_struct *data)
> >> >> return;
> >> >> }
> >> >>
> >> >> - usb_phy_vbus_off(musb->xceiv);
> >> >> - usleep_range(100, 200);
> >> >> -
> >> >> - usb_phy_vbus_on(musb->xceiv);
> >> >> - usleep_range(100, 200);
> >> >> + /* drop session bit */
> >> >> + devctl = musb_readb(musb->mregs, MUSB_DEVCTL);
> >> >> + devctl &= ~MUSB_DEVCTL_SESSION;
> >> >> + musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
> >> >>
> >> >> - /*
> >> >> - * When a babble condition occurs, the musb controller
> >> >> - * removes the session bit and the endpoint config is lost.
> >> >> - */
> >> >> - if (musb->dyn_fifo)
> >> >> - status = ep_config_from_table(musb);
> >> >> - else
> >> >> - status = ep_config_from_hw(musb);
> >> >> + /* tell usbcore about it */
> >> >> + musb_root_disconnect(musb);
> >> >>
> >> >> - /* start the session again */
> >> >> - if (status == 0)
> >> >> - musb_start(musb);
> >> >> + /* restart session */
> >> >> + devctl |= MUSB_DEVCTL_SESSION;
> >> >> + musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
> >> >> }
> >> >>
> >> >> /* --------------------------------------------------------------------------
> >> >>
> >> >> now I'll move all of this inside the irq handler and rename
> >> >> musb_platform_reset() to musb_platform_recover_from_babble()
> >> >
> >> > it looks like we really do need to redo fifo sizing. So here's a version
> >> > which I tested and can see that even after babble, device reenumerates.
> >> >
> >> > commit 1b9f864804aab6b250084b7adab9b520903f0a1d
> >> > Author: Felipe Balbi <balbi@xxxxxx>
> >> > Date: Thu Feb 26 14:02:35 2015 -0600
> >> >
> >> > usb: musb: core: simplify musb_recover_work()
> >> >
> >> > we're not resetting musb at all, just restarting
> >> > the session. This means we don't need to touch PHYs
> >> > or VBUS or anything like that. Just make sure session
> >> > bit is reenabled after MUSB dropped it.
> >> >
> >> > while at that, make sure to tell usbcore that we're
> >> > dropping the session and, thus, disconnecting the
> >> > device.
> >> >
> >> > Signed-off-by: Felipe Balbi <balbi@xxxxxx>
> >> >
> >> > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> >> > index d9c627d54db6..1ae4c3c28d68 100644
> >> > --- a/drivers/usb/musb/musb_core.c
> >> > +++ b/drivers/usb/musb/musb_core.c
> >> > @@ -1835,7 +1835,8 @@ static void musb_irq_work(struct work_struct *data)
> >> > static void musb_recover_work(struct work_struct *data)
> >> > {
> >> > struct musb *musb = container_of(data, struct musb, recover_work.work);
> >> > - int status, ret;
> >> > + int ret;
> >> > + u8 devctl;
> >> >
> >> > ret = musb_platform_reset(musb);
> >> > if (ret) {
> >> > @@ -1843,23 +1844,25 @@ static void musb_recover_work(struct work_struct *data)
> >> > return;
> >> > }
> >> >
> >> > - usb_phy_vbus_off(musb->xceiv);
> >> > - usleep_range(100, 200);
> >> > + /* drop session bit */
> >> > + devctl = musb_readb(musb->mregs, MUSB_DEVCTL);
> >> > + devctl &= ~MUSB_DEVCTL_SESSION;
> >> > + musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
> >> >
> >> > - usb_phy_vbus_on(musb->xceiv);
> >> > - usleep_range(100, 200);
> >> > + /* tell usbcore about it */
> >> > + musb_root_disconnect(musb);
> >> >
> >> > /*
> >> > * When a babble condition occurs, the musb controller
> >> > * removes the session bit and the endpoint config is lost.
> >> > */
> >> > if (musb->dyn_fifo)
> >> > - status = ep_config_from_table(musb);
> >> > + ret = ep_config_from_table(musb);
> >> > else
> >> > - status = ep_config_from_hw(musb);
> >> > + ret = ep_config_from_hw(musb);
> >> >
> >> > - /* start the session again */
> >> > - if (status == 0)
> >> > + /* restart session */
> >> > + if (ret == 0)
> >> > musb_start(musb);
> >> > }
> >> >
> >>
> >> Looks good to me. You also need the following to not reset MUSB.
> >>
> >> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> >> index b52eeca..4da8388 100644
> >> --- a/drivers/usb/musb/musb_dsps.c
> >> +++ b/drivers/usb/musb/musb_dsps.c
> >> @@ -592,20 +592,8 @@ static int dsps_musb_reset(struct musb *musb)
> >>
> >> if (glue->sw_babble_enabled)
> >> session_restart = dsps_sw_babble_control(musb);
> >> - /*
> >> - * In case of new silicon version babble condition can be recovered
> >> - * without resetting the MUSB. But for older silicon versions, MUSB
> >> - * reset is needed
> >> - */
> >> - if (session_restart || !glue->sw_babble_enabled) {
> >> - dev_info(musb->controller, "Restarting MUSB to recover
> >> from Babble\n");
> >> - dsps_writel(musb->ctrl_base, wrp->control, (1 << wrp->reset));
> >> - usleep_range(100, 200);
> >> - usb_phy_shutdown(musb->xceiv);
> >> - usleep_range(100, 200);
> >> - usb_phy_init(musb->xceiv);
> >> + else
> >> session_restart = 1;
> >> - }
> >>
> >> return session_restart ? 0 : -EPIPE;
> >> }
> >
> > got that covered too :-)
> >
> > https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=testing/next&id=12326d485aa676a09bc17026dce9094394db8bdc
> >
>
> Cool. If you change
>
> + if (!glue->sw_babble_enabled)
>
> to
>
> + else
>
> then your patch is same as mine ;)
oh, good point. Completely unnecessary to have that extra if there :-p
thanks
--
balbi
Attachment:
signature.asc
Description: Digital signature
