Re: [PATCH 3/7] usb: musb: core: move babble recovery inside babble check
On Thu, Feb 26, 2015 at 11:56 AM, Felipe Balbi wrote:
> Hi again,
>
> On Thu, Feb 26, 2015 at 11:54:43AM -0600, Felipe Balbi wrote:
>> > >> >> > >> > There was already a proper place where we were
>> > >> >> > >> > checking for babble interrupts, move babble
>> > >> >> > >> > recovery there.
>> > >> >> > >> >
>> > >> >> > >> > Signed-off-by: Felipe Balbi
>> > >> >> > >> > ---
>> > >> >> > >> > drivers/usb/musb/musb_core.c | 13 ++---
>> > >> >> > >> > 1 file changed, 6 insertions(+), 7 deletions(-)
>> > >> >> > >> >
>> > >> >> > >> > diff --git a/drivers/usb/musb/musb_core.c
>> > >> >> > >> > b/drivers/usb/musb/musb_core.c
>> > >> >> > >> > index 2767ce1bf016..0569b24719e6 100644
>> > >> >> > >> > --- a/drivers/usb/musb/musb_core.c
>> > >> >> > >> > +++ b/drivers/usb/musb/musb_core.c
>> > >> >> > >> > @@ -892,6 +892,12 @@ b_host:
>> > >> >> > >> > } else {
>> > >> >> > >> > ERR("Stopping host session --
>> > >> >> > >> > babble\n");
>> > >> >> > >> > musb_writeb(musb->mregs,
>> > >> >> > >> > MUSB_DEVCTL, 0);
>> > >> >> > >> > +
>> > >> >> > >> > + if (is_host_active(musb)) {
>> > >> >> > >> > +
>> > >> >> > >> > musb_generic_disable(musb);
>> > >> >> > >> > +
>> > >> >> > >> > schedule_delayed_work(&musb->recover_work,
>> > >> >> > >> > +
>> > >> >> > >> > msecs_to_jiffies(100));
>> > >> >> > >> > + }
>> > >> >> > >>
>> > >> >> > >> This change breaks babble recovery, because the following lines
>> > >> >> > >> above here
>> > >> >> > >>
>> > >> >> > >> 873 if (devctl & (MUSB_DEVCTL_FSDEV |
>> > >> >> > >> MUSB_DEVCTL_LSDEV)) {
>> > >> >> > >> 874 dev_dbg(musb->controller,
>> > >> >> > >> "BABBLE
>> > >> >> > >> devctl: %02x\n", devctl);
>> > >> >> > >>
>> > >> >> > >> have a bug - DEVCTL_FSDEV bit will be set for high-speed too,
>> > >> >> > >> so this
>> > >> >> > >> 'if' traps babble handling for all cases, never hit on 'else'.
>> > >> >> > >
>> > >> >> > > We might as well drop that check altogether. Let me see what
>> > >> >> > > happens
>> > >> >> > > here.
>> > >> >> >
>> > >> >> > It is good to clean it up, but I guess the babble storm you see is
>> > >> >> > caused by something else. I debugged the storm last year in an
>> > >> >> > older
>> > >> >> > kernel, it was due to the babble recovery routine does not
>> > >> >> > maintain a
>> > >> >> > bit in MUSB_BABBLE_CTL, though I forgot the details now. I am
>> > >> >> > looking
>> > >> >> > at this part in the upstream kernel right now.
>> > >>
>> > >> I am unable to recall why this bug causes the storm, but here is the
>> > >> bug fix - SW_SESSION_CTRL bit gets cleared after reset. Please let me
>> > >> know if I need to send an seperate patch email.
>> > >
>> > > please send it as a patch, but please rebase on top of my testing/next.
>> > > I've just pushed quite a few patches fixing a bunch of weird
>> > > inconsistencies with babble recovery.
>> >
>> > I can do that. But the hw reset is unnecessary for babble recover, it
>> > is also a problem due to AM335x Errata Adversary 1.0.34. If not reset,
>> > SW_SESSION_CTRL bit will not be cleared.
>> >
>> > Do you want me to send this patch or a new patch to not reset hw in
>> > recovery?
>>
>> I would rather not reset the IP if we don't have to :-s
>
> what I see though, is that we're only resetting if sw_babble_control()
> decided taht we need a session to be restarted, if we managed to recover
> from babble, then we're not resetting, right ? here's the relevant piece
> of code:
That is correct.
>
> 593 if (glue->sw_babble_enabled)
> 594 session_restart = dsps_sw_babble_control(musb);
> 595 /*
> 596 * In case of new silicon version babble condition can be
> recovered
> 597 * without resetting the MUSB. But for older silicon versions,
> MUSB
> 598 * reset is needed
> 599 */
> 600 if (session_restart || !glue->sw_babble_enabled) {
> 601 dev_info(musb->controller, "Restarting MUSB to recover
> from Babble\n");
> 602 dsps_writel(musb->ctrl_base, wrp->control, (1 <<
> wrp->reset));
> 603 usleep_range(100, 200);
> 604 usb_phy_shutdown(musb->xceiv);
> 605 usleep_range(100, 200);
> 606 usb_phy_init(musb->xceiv);
> 607 session_restart = 1;
> 608 }
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] usb: musb: core: move babble recovery inside babble check
Felipe,
On Thu, Feb 26, 2015 at 11:54 AM, Felipe Balbi wrote:
> On Thu, Feb 26, 2015 at 11:51:11AM -0600, Bin Liu wrote:
>> Felipe,
>>
>> On Thu, Feb 26, 2015 at 11:40 AM, Felipe Balbi wrote:
>> > Hi,
>> >
>> > On Thu, Feb 26, 2015 at 11:20:29AM -0600, Bin Liu wrote:
>> >> >> > >> > There was already a proper place where we were
>> >> >> > >> > checking for babble interrupts, move babble
>> >> >> > >> > recovery there.
>> >> >> > >> >
>> >> >> > >> > Signed-off-by: Felipe Balbi
>> >> >> > >> > ---
>> >> >> > >> > drivers/usb/musb/musb_core.c | 13 ++---
>> >> >> > >> > 1 file changed, 6 insertions(+), 7 deletions(-)
>> >> >> > >> >
>> >> >> > >> > diff --git a/drivers/usb/musb/musb_core.c
>> >> >> > >> > b/drivers/usb/musb/musb_core.c
>> >> >> > >> > index 2767ce1bf016..0569b24719e6 100644
>> >> >> > >> > --- a/drivers/usb/musb/musb_core.c
>> >> >> > >> > +++ b/drivers/usb/musb/musb_core.c
>> >> >> > >> > @@ -892,6 +892,12 @@ b_host:
>> >> >> > >> > } else {
>> >> >> > >> > ERR("Stopping host session --
>> >> >> > >> > babble\n");
>> >> >> > >> > musb_writeb(musb->mregs,
>> >> >> > >> > MUSB_DEVCTL, 0);
>> >> >> > >> > +
>> >> >> > >> > + if (is_host_active(musb)) {
>> >> >> > >> > +
>> >> >> > >> > musb_generic_disable(musb);
>> >> >> > >> > +
>> >> >> > >> > schedule_delayed_work(&musb->recover_work,
>> >> >> > >> > +
>> >> >> > >> > msecs_to_jiffies(100));
>> >> >> > >> > + }
>> >> >> > >>
>> >> >> > >> This change breaks babble recovery, because the following lines
>> >> >> > >> above here
>> >> >> > >>
>> >> >> > >> 873 if (devctl & (MUSB_DEVCTL_FSDEV |
>> >> >> > >> MUSB_DEVCTL_LSDEV)) {
>> >> >> > >> 874 dev_dbg(musb->controller,
>> >> >> > >> "BABBLE
>> >> >> > >> devctl: %02x\n", devctl);
>> >> >> > >>
>> >> >> > >> have a bug - DEVCTL_FSDEV bit will be set for high-speed too, so
>> >> >> > >> this
>> >> >> > >> 'if' traps babble handling for all cases, never hit on 'else'.
>> >> >> > >
>> >> >> > > We might as well drop that check altogether. Let me see what
>> >> >> > > happens
>> >> >> > > here.
>> >> >> >
>> >> >> > It is good to clean it up, but I guess the babble storm you see is
>> >> >> > caused by something else. I debugged the storm last year in an older
>> >> >> > kernel, it was due to the babble recovery routine does not maintain a
>> >> >> > bit in MUSB_BABBLE_CTL, though I forgot the details now. I am looking
>> >> >> > at this part in the upstream kernel right now.
>> >>
>> >> I am unable to recall why this bug causes the storm, but here is the
>> >> bug fix - SW_SESSION_CTRL bit gets cleared after reset. Please let me
>> >> know if I need to send an seperate patch email.
>> >
>> > please send it as a patch, but please rebase on top of my testing/next.
>> > I've just pushed quite a few patches fixing a bunch of weird
>> > inconsistencies with babble recovery.
>>
>> I can do that. But the hw reset is unnecessary for babble recover, it
>> is also a problem due to AM335x Errata Adversary 1.0.34. If not reset,
>> SW_SESSION_CTRL bit will not be cleared.
>>
>> Do you want me to send this patch or a new patch to not reset hw in recovery?
>
> I would rather not reset the IP if we don't have to :-s
Me too. I will make a patch for it.
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] usb: musb: core: move babble recovery inside babble check
Hi again,
On Thu, Feb 26, 2015 at 11:54:43AM -0600, Felipe Balbi wrote:
> > >> >> > >> > There was already a proper place where we were
> > >> >> > >> > checking for babble interrupts, move babble
> > >> >> > >> > recovery there.
> > >> >> > >> >
> > >> >> > >> > Signed-off-by: Felipe Balbi
> > >> >> > >> > ---
> > >> >> > >> > drivers/usb/musb/musb_core.c | 13 ++---
> > >> >> > >> > 1 file changed, 6 insertions(+), 7 deletions(-)
> > >> >> > >> >
> > >> >> > >> > diff --git a/drivers/usb/musb/musb_core.c
> > >> >> > >> > b/drivers/usb/musb/musb_core.c
> > >> >> > >> > index 2767ce1bf016..0569b24719e6 100644
> > >> >> > >> > --- a/drivers/usb/musb/musb_core.c
> > >> >> > >> > +++ b/drivers/usb/musb/musb_core.c
> > >> >> > >> > @@ -892,6 +892,12 @@ b_host:
> > >> >> > >> > } else {
> > >> >> > >> > ERR("Stopping host session --
> > >> >> > >> > babble\n");
> > >> >> > >> > musb_writeb(musb->mregs,
> > >> >> > >> > MUSB_DEVCTL, 0);
> > >> >> > >> > +
> > >> >> > >> > + if (is_host_active(musb)) {
> > >> >> > >> > +
> > >> >> > >> > musb_generic_disable(musb);
> > >> >> > >> > +
> > >> >> > >> > schedule_delayed_work(&musb->recover_work,
> > >> >> > >> > +
> > >> >> > >> > msecs_to_jiffies(100));
> > >> >> > >> > + }
> > >> >> > >>
> > >> >> > >> This change breaks babble recovery, because the following lines
> > >> >> > >> above here
> > >> >> > >>
> > >> >> > >> 873 if (devctl & (MUSB_DEVCTL_FSDEV |
> > >> >> > >> MUSB_DEVCTL_LSDEV)) {
> > >> >> > >> 874 dev_dbg(musb->controller,
> > >> >> > >> "BABBLE
> > >> >> > >> devctl: %02x\n", devctl);
> > >> >> > >>
> > >> >> > >> have a bug - DEVCTL_FSDEV bit will be set for high-speed too, so
> > >> >> > >> this
> > >> >> > >> 'if' traps babble handling for all cases, never hit on 'else'.
> > >> >> > >
> > >> >> > > We might as well drop that check altogether. Let me see what
> > >> >> > > happens
> > >> >> > > here.
> > >> >> >
> > >> >> > It is good to clean it up, but I guess the babble storm you see is
> > >> >> > caused by something else. I debugged the storm last year in an older
> > >> >> > kernel, it was due to the babble recovery routine does not maintain
> > >> >> > a
> > >> >> > bit in MUSB_BABBLE_CTL, though I forgot the details now. I am
> > >> >> > looking
> > >> >> > at this part in the upstream kernel right now.
> > >>
> > >> I am unable to recall why this bug causes the storm, but here is the
> > >> bug fix - SW_SESSION_CTRL bit gets cleared after reset. Please let me
> > >> know if I need to send an seperate patch email.
> > >
> > > please send it as a patch, but please rebase on top of my testing/next.
> > > I've just pushed quite a few patches fixing a bunch of weird
> > > inconsistencies with babble recovery.
> >
> > I can do that. But the hw reset is unnecessary for babble recover, it
> > is also a problem due to AM335x Errata Adversary 1.0.34. If not reset,
> > SW_SESSION_CTRL bit will not be cleared.
> >
> > Do you want me to send this patch or a new patch to not reset hw in
> > recovery?
>
> I would rather not reset the IP if we don't have to :-s
what I see though, is that we're only resetting if sw_babble_control()
decided taht we need a session to be restarted, if we managed to recover
from babble, then we're not resetting, right ? here's the relevant piece
of code:
593 if (glue->sw_babble_enabled)
594 session_restart = dsps_sw_babble_control(musb);
595 /*
596 * In case of new silicon version babble condition can be recovered
597 * without resetting the MUSB. But for older silicon versions, MUSB
598 * reset is needed
599 */
600 if (session_restart || !glue->sw_babble_enabled) {
601 dev_info(musb->controller, "Restarting MUSB to recover from
Babble\n");
602 dsps_writel(musb->ctrl_base, wrp->control, (1 <<
wrp->reset));
603 usleep_range(100, 200);
604 usb_phy_shutdown(musb->xceiv);
605 usleep_range(100, 200);
606 usb_phy_init(musb->xceiv);
607 session_restart = 1;
608 }
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 3/7] usb: musb: core: move babble recovery inside babble check
On Thu, Feb 26, 2015 at 11:51:11AM -0600, Bin Liu wrote:
> Felipe,
>
> On Thu, Feb 26, 2015 at 11:40 AM, Felipe Balbi wrote:
> > Hi,
> >
> > On Thu, Feb 26, 2015 at 11:20:29AM -0600, Bin Liu wrote:
> >> >> > >> > There was already a proper place where we were
> >> >> > >> > checking for babble interrupts, move babble
> >> >> > >> > recovery there.
> >> >> > >> >
> >> >> > >> > Signed-off-by: Felipe Balbi
> >> >> > >> > ---
> >> >> > >> > drivers/usb/musb/musb_core.c | 13 ++---
> >> >> > >> > 1 file changed, 6 insertions(+), 7 deletions(-)
> >> >> > >> >
> >> >> > >> > diff --git a/drivers/usb/musb/musb_core.c
> >> >> > >> > b/drivers/usb/musb/musb_core.c
> >> >> > >> > index 2767ce1bf016..0569b24719e6 100644
> >> >> > >> > --- a/drivers/usb/musb/musb_core.c
> >> >> > >> > +++ b/drivers/usb/musb/musb_core.c
> >> >> > >> > @@ -892,6 +892,12 @@ b_host:
> >> >> > >> > } else {
> >> >> > >> > ERR("Stopping host session --
> >> >> > >> > babble\n");
> >> >> > >> > musb_writeb(musb->mregs,
> >> >> > >> > MUSB_DEVCTL, 0);
> >> >> > >> > +
> >> >> > >> > + if (is_host_active(musb)) {
> >> >> > >> > +
> >> >> > >> > musb_generic_disable(musb);
> >> >> > >> > +
> >> >> > >> > schedule_delayed_work(&musb->recover_work,
> >> >> > >> > +
> >> >> > >> > msecs_to_jiffies(100));
> >> >> > >> > + }
> >> >> > >>
> >> >> > >> This change breaks babble recovery, because the following lines
> >> >> > >> above here
> >> >> > >>
> >> >> > >> 873 if (devctl & (MUSB_DEVCTL_FSDEV |
> >> >> > >> MUSB_DEVCTL_LSDEV)) {
> >> >> > >> 874 dev_dbg(musb->controller,
> >> >> > >> "BABBLE
> >> >> > >> devctl: %02x\n", devctl);
> >> >> > >>
> >> >> > >> have a bug - DEVCTL_FSDEV bit will be set for high-speed too, so
> >> >> > >> this
> >> >> > >> 'if' traps babble handling for all cases, never hit on 'else'.
> >> >> > >
> >> >> > > We might as well drop that check altogether. Let me see what happens
> >> >> > > here.
> >> >> >
> >> >> > It is good to clean it up, but I guess the babble storm you see is
> >> >> > caused by something else. I debugged the storm last year in an older
> >> >> > kernel, it was due to the babble recovery routine does not maintain a
> >> >> > bit in MUSB_BABBLE_CTL, though I forgot the details now. I am looking
> >> >> > at this part in the upstream kernel right now.
> >>
> >> I am unable to recall why this bug causes the storm, but here is the
> >> bug fix - SW_SESSION_CTRL bit gets cleared after reset. Please let me
> >> know if I need to send an seperate patch email.
> >
> > please send it as a patch, but please rebase on top of my testing/next.
> > I've just pushed quite a few patches fixing a bunch of weird
> > inconsistencies with babble recovery.
>
> I can do that. But the hw reset is unnecessary for babble recover, it
> is also a problem due to AM335x Errata Adversary 1.0.34. If not reset,
> SW_SESSION_CTRL bit will not be cleared.
>
> Do you want me to send this patch or a new patch to not reset hw in recovery?
I would rather not reset the IP if we don't have to :-s
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 3/7] usb: musb: core: move babble recovery inside babble check
Felipe,
On Thu, Feb 26, 2015 at 11:40 AM, Felipe Balbi wrote:
> Hi,
>
> On Thu, Feb 26, 2015 at 11:20:29AM -0600, Bin Liu wrote:
>> >> > >> > There was already a proper place where we were
>> >> > >> > checking for babble interrupts, move babble
>> >> > >> > recovery there.
>> >> > >> >
>> >> > >> > Signed-off-by: Felipe Balbi
>> >> > >> > ---
>> >> > >> > drivers/usb/musb/musb_core.c | 13 ++---
>> >> > >> > 1 file changed, 6 insertions(+), 7 deletions(-)
>> >> > >> >
>> >> > >> > diff --git a/drivers/usb/musb/musb_core.c
>> >> > >> > b/drivers/usb/musb/musb_core.c
>> >> > >> > index 2767ce1bf016..0569b24719e6 100644
>> >> > >> > --- a/drivers/usb/musb/musb_core.c
>> >> > >> > +++ b/drivers/usb/musb/musb_core.c
>> >> > >> > @@ -892,6 +892,12 @@ b_host:
>> >> > >> > } else {
>> >> > >> > ERR("Stopping host session --
>> >> > >> > babble\n");
>> >> > >> > musb_writeb(musb->mregs,
>> >> > >> > MUSB_DEVCTL, 0);
>> >> > >> > +
>> >> > >> > + if (is_host_active(musb)) {
>> >> > >> > + musb_generic_disable(musb);
>> >> > >> > +
>> >> > >> > schedule_delayed_work(&musb->recover_work,
>> >> > >> > +
>> >> > >> > msecs_to_jiffies(100));
>> >> > >> > + }
>> >> > >>
>> >> > >> This change breaks babble recovery, because the following lines
>> >> > >> above here
>> >> > >>
>> >> > >> 873 if (devctl & (MUSB_DEVCTL_FSDEV |
>> >> > >> MUSB_DEVCTL_LSDEV)) {
>> >> > >> 874 dev_dbg(musb->controller, "BABBLE
>> >> > >> devctl: %02x\n", devctl);
>> >> > >>
>> >> > >> have a bug - DEVCTL_FSDEV bit will be set for high-speed too, so this
>> >> > >> 'if' traps babble handling for all cases, never hit on 'else'.
>> >> > >
>> >> > > We might as well drop that check altogether. Let me see what happens
>> >> > > here.
>> >> >
>> >> > It is good to clean it up, but I guess the babble storm you see is
>> >> > caused by something else. I debugged the storm last year in an older
>> >> > kernel, it was due to the babble recovery routine does not maintain a
>> >> > bit in MUSB_BABBLE_CTL, though I forgot the details now. I am looking
>> >> > at this part in the upstream kernel right now.
>>
>> I am unable to recall why this bug causes the storm, but here is the
>> bug fix - SW_SESSION_CTRL bit gets cleared after reset. Please let me
>> know if I need to send an seperate patch email.
>
> please send it as a patch, but please rebase on top of my testing/next.
> I've just pushed quite a few patches fixing a bunch of weird
> inconsistencies with babble recovery.
I can do that. But the hw reset is unnecessary for babble recover, it
is also a problem due to AM335x Errata Adversary 1.0.34. If not reset,
SW_SESSION_CTRL bit will not be cleared.
Do you want me to send this patch or a new patch to not reset hw in recovery?
>
> Basically, even though we set SW_SESSION_CONTROL, we were still writing
> 0 to devctl which was dropping the session anyway!
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] usb: musb: core: move babble recovery inside babble check
Hi,
On Thu, Feb 26, 2015 at 11:20:29AM -0600, Bin Liu wrote:
> >> > >> > There was already a proper place where we were
> >> > >> > checking for babble interrupts, move babble
> >> > >> > recovery there.
> >> > >> >
> >> > >> > Signed-off-by: Felipe Balbi
> >> > >> > ---
> >> > >> > drivers/usb/musb/musb_core.c | 13 ++---
> >> > >> > 1 file changed, 6 insertions(+), 7 deletions(-)
> >> > >> >
> >> > >> > diff --git a/drivers/usb/musb/musb_core.c
> >> > >> > b/drivers/usb/musb/musb_core.c
> >> > >> > index 2767ce1bf016..0569b24719e6 100644
> >> > >> > --- a/drivers/usb/musb/musb_core.c
> >> > >> > +++ b/drivers/usb/musb/musb_core.c
> >> > >> > @@ -892,6 +892,12 @@ b_host:
> >> > >> > } else {
> >> > >> > ERR("Stopping host session --
> >> > >> > babble\n");
> >> > >> > musb_writeb(musb->mregs,
> >> > >> > MUSB_DEVCTL, 0);
> >> > >> > +
> >> > >> > + if (is_host_active(musb)) {
> >> > >> > + musb_generic_disable(musb);
> >> > >> > +
> >> > >> > schedule_delayed_work(&musb->recover_work,
> >> > >> > +
> >> > >> > msecs_to_jiffies(100));
> >> > >> > + }
> >> > >>
> >> > >> This change breaks babble recovery, because the following lines above
> >> > >> here
> >> > >>
> >> > >> 873 if (devctl & (MUSB_DEVCTL_FSDEV |
> >> > >> MUSB_DEVCTL_LSDEV)) {
> >> > >> 874 dev_dbg(musb->controller, "BABBLE
> >> > >> devctl: %02x\n", devctl);
> >> > >>
> >> > >> have a bug - DEVCTL_FSDEV bit will be set for high-speed too, so this
> >> > >> 'if' traps babble handling for all cases, never hit on 'else'.
> >> > >
> >> > > We might as well drop that check altogether. Let me see what happens
> >> > > here.
> >> >
> >> > It is good to clean it up, but I guess the babble storm you see is
> >> > caused by something else. I debugged the storm last year in an older
> >> > kernel, it was due to the babble recovery routine does not maintain a
> >> > bit in MUSB_BABBLE_CTL, though I forgot the details now. I am looking
> >> > at this part in the upstream kernel right now.
>
> I am unable to recall why this bug causes the storm, but here is the
> bug fix - SW_SESSION_CTRL bit gets cleared after reset. Please let me
> know if I need to send an seperate patch email.
please send it as a patch, but please rebase on top of my testing/next.
I've just pushed quite a few patches fixing a bunch of weird
inconsistencies with babble recovery.
Basically, even though we set SW_SESSION_CONTROL, we were still writing
0 to devctl which was dropping the session anyway!
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 3/7] usb: musb: core: move babble recovery inside babble check
Felipe,
On Thu, Feb 26, 2015 at 10:54 AM, Felipe Balbi wrote:
> On Thu, Feb 26, 2015 at 10:44:15AM -0600, Felipe Balbi wrote:
>> On Thu, Feb 26, 2015 at 10:37:50AM -0600, Bin Liu wrote:
>> > Felipe,
>> >
>> > On Thu, Feb 26, 2015 at 10:31 AM, Felipe Balbi wrote:
>> > > On Thu, Feb 26, 2015 at 10:21:38AM -0600, Bin Liu wrote:
>> > >> Felipe,
>> > >>
>> > >> On Thu, Feb 26, 2015 at 9:07 AM, Felipe Balbi wrote:
>> > >> > There was already a proper place where we were
>> > >> > checking for babble interrupts, move babble
>> > >> > recovery there.
>> > >> >
>> > >> > Signed-off-by: Felipe Balbi
>> > >> > ---
>> > >> > drivers/usb/musb/musb_core.c | 13 ++---
>> > >> > 1 file changed, 6 insertions(+), 7 deletions(-)
>> > >> >
>> > >> > diff --git a/drivers/usb/musb/musb_core.c
>> > >> > b/drivers/usb/musb/musb_core.c
>> > >> > index 2767ce1bf016..0569b24719e6 100644
>> > >> > --- a/drivers/usb/musb/musb_core.c
>> > >> > +++ b/drivers/usb/musb/musb_core.c
>> > >> > @@ -892,6 +892,12 @@ b_host:
>> > >> > } else {
>> > >> > ERR("Stopping host session --
>> > >> > babble\n");
>> > >> > musb_writeb(musb->mregs, MUSB_DEVCTL,
>> > >> > 0);
>> > >> > +
>> > >> > + if (is_host_active(musb)) {
>> > >> > + musb_generic_disable(musb);
>> > >> > +
>> > >> > schedule_delayed_work(&musb->recover_work,
>> > >> > +
>> > >> > msecs_to_jiffies(100));
>> > >> > + }
>> > >>
>> > >> This change breaks babble recovery, because the following lines above
>> > >> here
>> > >>
>> > >> 873 if (devctl & (MUSB_DEVCTL_FSDEV |
>> > >> MUSB_DEVCTL_LSDEV)) {
>> > >> 874 dev_dbg(musb->controller, "BABBLE
>> > >> devctl: %02x\n", devctl);
>> > >>
>> > >> have a bug - DEVCTL_FSDEV bit will be set for high-speed too, so this
>> > >> 'if' traps babble handling for all cases, never hit on 'else'.
>> > >
>> > > We might as well drop that check altogether. Let me see what happens
>> > > here.
>> >
>> > It is good to clean it up, but I guess the babble storm you see is
>> > caused by something else. I debugged the storm last year in an older
>> > kernel, it was due to the babble recovery routine does not maintain a
>> > bit in MUSB_BABBLE_CTL, though I forgot the details now. I am looking
>> > at this part in the upstream kernel right now.
I am unable to recall why this bug causes the storm, but here is the
bug fix - SW_SESSION_CTRL bit gets cleared after reset. Please let me
know if I need to send an seperate patch email.
diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 5965ed6..b4a92e2 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -607,6 +607,14 @@ static int dsps_musb_reset(struct musb *musb)
session_restart = 1;
}
+ if (glue->sw_babble_enabled) {
+ u32 val;
+
+ val = dsps_readb(musb->mregs, MUSB_BABBLE_CTL);
+ val |= MUSB_BABBLE_SW_SESSION_CTRL;
+ dsps_writeb(musb->mregs, MUSB_BABBLE_CTL, val);
+ }
+
return !session_restart;
}
>>
>> alright, I'll have a look, let's see.
>
> I'll split below into two patches, but here you go:
>
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index e23eb3e517de..c3c5a6462600 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -862,16 +862,23 @@ b_host:
> if (int_usb & MUSB_INTR_RESET) {
> handled = IRQ_HANDLED;
> if (devctl & MUSB_DEVCTL_HM) {
> + u8 power = musb_readl(musb->mregs, MUSB_POWER);
> +
> /*
> * Looks like non-HS BABBLE can be ignored, but
> -* HS BABBLE is an error condition. For HS the
> solution
> -* is to avoid babble in the first place and fix what
> -* caused BABBLE. When HS BABBLE happens we can only
> -* stop the session.
> +* HS BABBLE is an error condition.
> +*
> +* For HS the solution is to avoid babble in the first
> +* place and fix what caused BABBLE.
> +*
> +* When HS BABBLE happens what we can depends on which
> +* platform MUSB is running, because some platforms
> +* implemented proprietary means for 'recovering' from
> +* Babble conditions. One such platform is AM335x. In
> +* most cases, however, the only thing we can do is
> drop
> +* the session.
Re: [PATCH 3/7] usb: musb: core: move babble recovery inside babble check
On Thu, Feb 26, 2015 at 10:44:15AM -0600, Felipe Balbi wrote:
> On Thu, Feb 26, 2015 at 10:37:50AM -0600, Bin Liu wrote:
> > Felipe,
> >
> > On Thu, Feb 26, 2015 at 10:31 AM, Felipe Balbi wrote:
> > > On Thu, Feb 26, 2015 at 10:21:38AM -0600, Bin Liu wrote:
> > >> Felipe,
> > >>
> > >> On Thu, Feb 26, 2015 at 9:07 AM, Felipe Balbi wrote:
> > >> > There was already a proper place where we were
> > >> > checking for babble interrupts, move babble
> > >> > recovery there.
> > >> >
> > >> > Signed-off-by: Felipe Balbi
> > >> > ---
> > >> > drivers/usb/musb/musb_core.c | 13 ++---
> > >> > 1 file changed, 6 insertions(+), 7 deletions(-)
> > >> >
> > >> > diff --git a/drivers/usb/musb/musb_core.c
> > >> > b/drivers/usb/musb/musb_core.c
> > >> > index 2767ce1bf016..0569b24719e6 100644
> > >> > --- a/drivers/usb/musb/musb_core.c
> > >> > +++ b/drivers/usb/musb/musb_core.c
> > >> > @@ -892,6 +892,12 @@ b_host:
> > >> > } else {
> > >> > ERR("Stopping host session --
> > >> > babble\n");
> > >> > musb_writeb(musb->mregs, MUSB_DEVCTL,
> > >> > 0);
> > >> > +
> > >> > + if (is_host_active(musb)) {
> > >> > + musb_generic_disable(musb);
> > >> > +
> > >> > schedule_delayed_work(&musb->recover_work,
> > >> > +
> > >> > msecs_to_jiffies(100));
> > >> > + }
> > >>
> > >> This change breaks babble recovery, because the following lines above
> > >> here
> > >>
> > >> 873 if (devctl & (MUSB_DEVCTL_FSDEV |
> > >> MUSB_DEVCTL_LSDEV)) {
> > >> 874 dev_dbg(musb->controller, "BABBLE
> > >> devctl: %02x\n", devctl);
> > >>
> > >> have a bug - DEVCTL_FSDEV bit will be set for high-speed too, so this
> > >> 'if' traps babble handling for all cases, never hit on 'else'.
> > >
> > > We might as well drop that check altogether. Let me see what happens
> > > here.
> >
> > It is good to clean it up, but I guess the babble storm you see is
> > caused by something else. I debugged the storm last year in an older
> > kernel, it was due to the babble recovery routine does not maintain a
> > bit in MUSB_BABBLE_CTL, though I forgot the details now. I am looking
> > at this part in the upstream kernel right now.
>
> alright, I'll have a look, let's see.
I'll split below into two patches, but here you go:
diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index e23eb3e517de..c3c5a6462600 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -862,16 +862,23 @@ b_host:
if (int_usb & MUSB_INTR_RESET) {
handled = IRQ_HANDLED;
if (devctl & MUSB_DEVCTL_HM) {
+ u8 power = musb_readl(musb->mregs, MUSB_POWER);
+
/*
* Looks like non-HS BABBLE can be ignored, but
-* HS BABBLE is an error condition. For HS the solution
-* is to avoid babble in the first place and fix what
-* caused BABBLE. When HS BABBLE happens we can only
-* stop the session.
+* HS BABBLE is an error condition.
+*
+* For HS the solution is to avoid babble in the first
+* place and fix what caused BABBLE.
+*
+* When HS BABBLE happens what we can depends on which
+* platform MUSB is running, because some platforms
+* implemented proprietary means for 'recovering' from
+* Babble conditions. One such platform is AM335x. In
+* most cases, however, the only thing we can do is drop
+* the session.
*/
- if (devctl & (MUSB_DEVCTL_FSDEV | MUSB_DEVCTL_LSDEV)) {
- dev_dbg(musb->controller, "BABBLE devctl:
%02x\n", devctl);
- } else {
+ if (power & MUSB_POWER_HSMODE) {
ERR("Stopping host session -- babble\n");
musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 5965ed69e457..b79202c3dd65 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -607,7 +607,7 @@ static int dsps_musb_reset(struct musb *musb)
session_restart = 1;
}
- return !session_restart;
+ return session_restart ? 0 : -EPIPE;
}
static struct musb_platform_ops dsps_ops = {
When I look at this with g_zero, BABBLE_CTL always reads as 0x44
(SW_SESSION_CTRL | RCV_
Re: [PATCH 3/7] usb: musb: core: move babble recovery inside babble check
On Thu, Feb 26, 2015 at 10:37:50AM -0600, Bin Liu wrote:
> Felipe,
>
> On Thu, Feb 26, 2015 at 10:31 AM, Felipe Balbi wrote:
> > On Thu, Feb 26, 2015 at 10:21:38AM -0600, Bin Liu wrote:
> >> Felipe,
> >>
> >> On Thu, Feb 26, 2015 at 9:07 AM, Felipe Balbi wrote:
> >> > There was already a proper place where we were
> >> > checking for babble interrupts, move babble
> >> > recovery there.
> >> >
> >> > Signed-off-by: Felipe Balbi
> >> > ---
> >> > drivers/usb/musb/musb_core.c | 13 ++---
> >> > 1 file changed, 6 insertions(+), 7 deletions(-)
> >> >
> >> > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> >> > index 2767ce1bf016..0569b24719e6 100644
> >> > --- a/drivers/usb/musb/musb_core.c
> >> > +++ b/drivers/usb/musb/musb_core.c
> >> > @@ -892,6 +892,12 @@ b_host:
> >> > } else {
> >> > ERR("Stopping host session -- babble\n");
> >> > musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
> >> > +
> >> > + if (is_host_active(musb)) {
> >> > + musb_generic_disable(musb);
> >> > +
> >> > schedule_delayed_work(&musb->recover_work,
> >> > +
> >> > msecs_to_jiffies(100));
> >> > + }
> >>
> >> This change breaks babble recovery, because the following lines above here
> >>
> >> 873 if (devctl & (MUSB_DEVCTL_FSDEV |
> >> MUSB_DEVCTL_LSDEV)) {
> >> 874 dev_dbg(musb->controller, "BABBLE
> >> devctl: %02x\n", devctl);
> >>
> >> have a bug - DEVCTL_FSDEV bit will be set for high-speed too, so this
> >> 'if' traps babble handling for all cases, never hit on 'else'.
> >
> > We might as well drop that check altogether. Let me see what happens
> > here.
>
> It is good to clean it up, but I guess the babble storm you see is
> caused by something else. I debugged the storm last year in an older
> kernel, it was due to the babble recovery routine does not maintain a
> bit in MUSB_BABBLE_CTL, though I forgot the details now. I am looking
> at this part in the upstream kernel right now.
alright, I'll have a look, let's see.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 3/7] usb: musb: core: move babble recovery inside babble check
Felipe,
On Thu, Feb 26, 2015 at 10:31 AM, Felipe Balbi wrote:
> On Thu, Feb 26, 2015 at 10:21:38AM -0600, Bin Liu wrote:
>> Felipe,
>>
>> On Thu, Feb 26, 2015 at 9:07 AM, Felipe Balbi wrote:
>> > There was already a proper place where we were
>> > checking for babble interrupts, move babble
>> > recovery there.
>> >
>> > Signed-off-by: Felipe Balbi
>> > ---
>> > drivers/usb/musb/musb_core.c | 13 ++---
>> > 1 file changed, 6 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
>> > index 2767ce1bf016..0569b24719e6 100644
>> > --- a/drivers/usb/musb/musb_core.c
>> > +++ b/drivers/usb/musb/musb_core.c
>> > @@ -892,6 +892,12 @@ b_host:
>> > } else {
>> > ERR("Stopping host session -- babble\n");
>> > musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
>> > +
>> > + if (is_host_active(musb)) {
>> > + musb_generic_disable(musb);
>> > +
>> > schedule_delayed_work(&musb->recover_work,
>> > +
>> > msecs_to_jiffies(100));
>> > + }
>>
>> This change breaks babble recovery, because the following lines above here
>>
>> 873 if (devctl & (MUSB_DEVCTL_FSDEV |
>> MUSB_DEVCTL_LSDEV)) {
>> 874 dev_dbg(musb->controller, "BABBLE
>> devctl: %02x\n", devctl);
>>
>> have a bug - DEVCTL_FSDEV bit will be set for high-speed too, so this
>> 'if' traps babble handling for all cases, never hit on 'else'.
>
> We might as well drop that check altogether. Let me see what happens
> here.
It is good to clean it up, but I guess the babble storm you see is
caused by something else. I debugged the storm last year in an older
kernel, it was due to the babble recovery routine does not maintain a
bit in MUSB_BABBLE_CTL, though I forgot the details now. I am looking
at this part in the upstream kernel right now.
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] usb: musb: core: move babble recovery inside babble check
On Thu, Feb 26, 2015 at 10:21:38AM -0600, Bin Liu wrote:
> Felipe,
>
> On Thu, Feb 26, 2015 at 9:07 AM, Felipe Balbi wrote:
> > There was already a proper place where we were
> > checking for babble interrupts, move babble
> > recovery there.
> >
> > Signed-off-by: Felipe Balbi
> > ---
> > drivers/usb/musb/musb_core.c | 13 ++---
> > 1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> > index 2767ce1bf016..0569b24719e6 100644
> > --- a/drivers/usb/musb/musb_core.c
> > +++ b/drivers/usb/musb/musb_core.c
> > @@ -892,6 +892,12 @@ b_host:
> > } else {
> > ERR("Stopping host session -- babble\n");
> > musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
> > +
> > + if (is_host_active(musb)) {
> > + musb_generic_disable(musb);
> > +
> > schedule_delayed_work(&musb->recover_work,
> > +
> > msecs_to_jiffies(100));
> > + }
>
> This change breaks babble recovery, because the following lines above here
>
> 873 if (devctl & (MUSB_DEVCTL_FSDEV |
> MUSB_DEVCTL_LSDEV)) {
> 874 dev_dbg(musb->controller, "BABBLE
> devctl: %02x\n", devctl);
>
> have a bug - DEVCTL_FSDEV bit will be set for high-speed too, so this
> 'if' traps babble handling for all cases, never hit on 'else'.
We might as well drop that check altogether. Let me see what happens
here.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 3/7] usb: musb: core: move babble recovery inside babble check
Felipe,
On Thu, Feb 26, 2015 at 9:07 AM, Felipe Balbi wrote:
> There was already a proper place where we were
> checking for babble interrupts, move babble
> recovery there.
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/usb/musb/musb_core.c | 13 ++---
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index 2767ce1bf016..0569b24719e6 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -892,6 +892,12 @@ b_host:
> } else {
> ERR("Stopping host session -- babble\n");
> musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
> +
> + if (is_host_active(musb)) {
> + musb_generic_disable(musb);
> +
> schedule_delayed_work(&musb->recover_work,
> +
> msecs_to_jiffies(100));
> + }
This change breaks babble recovery, because the following lines above here
873 if (devctl & (MUSB_DEVCTL_FSDEV |
MUSB_DEVCTL_LSDEV)) {
874 dev_dbg(musb->controller, "BABBLE
devctl: %02x\n", devctl);
have a bug - DEVCTL_FSDEV bit will be set for high-speed too, so this
'if' traps babble handling for all cases, never hit on 'else'.
> }
> } else {
> dev_dbg(musb->controller, "BUS RESET as %s\n",
> @@ -931,13 +937,6 @@ b_host:
> }
> }
>
> - /* handle babble condition */
> - if (int_usb & MUSB_INTR_BABBLE && is_host_active(musb)) {
> - musb_generic_disable(musb);
> - schedule_delayed_work(&musb->recover_work,
> - msecs_to_jiffies(100));
> - }
> -
> #if 0
> /* REVISIT ... this would be for multiplexing periodic endpoints, or
> * supporting transfer phasing to prevent exceeding ISO bandwidth
> --
> 2.3.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] usb: musb: core: move babble recovery inside babble check
Felipe,
On Thu, Feb 26, 2015 at 9:07 AM, Felipe Balbi wrote:
> There was already a proper place where we were
> checking for babble interrupts, move babble
> recovery there.
I commented on the same before, discussed in [1].
[1]: http://marc.info/?l=linux-usb&m=140109400304196&w=2
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/usb/musb/musb_core.c | 13 ++---
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index 2767ce1bf016..0569b24719e6 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -892,6 +892,12 @@ b_host:
> } else {
> ERR("Stopping host session -- babble\n");
> musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
> +
> + if (is_host_active(musb)) {
> + musb_generic_disable(musb);
> +
> schedule_delayed_work(&musb->recover_work,
> +
> msecs_to_jiffies(100));
> + }
> }
> } else {
> dev_dbg(musb->controller, "BUS RESET as %s\n",
> @@ -931,13 +937,6 @@ b_host:
> }
> }
>
> - /* handle babble condition */
> - if (int_usb & MUSB_INTR_BABBLE && is_host_active(musb)) {
> - musb_generic_disable(musb);
> - schedule_delayed_work(&musb->recover_work,
> - msecs_to_jiffies(100));
> - }
> -
> #if 0
> /* REVISIT ... this would be for multiplexing periodic endpoints, or
> * supporting transfer phasing to prevent exceeding ISO bandwidth
> --
> 2.3.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
