Re: [PATCH v2 03/15] usb: musb: core: fix highspeed check

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux