Re: Enabling MUSB support

[Felipe Balbi <me@felipebalbi.com>]

On Fri, Aug 29, 2008 at 01:05:31PM -0700, David Brownell wrote:
> On Friday 29 August 2008, Felipe Balbi wrote:
> > On Fri, Aug 29, 2008 at 11:12:24AM -0700, David Brownell wrote:
> > > On Wednesday 27 August 2008, David Brownell wrote:
> > > > I tried it on Beagle and found that OTG mode wanted to oops
> > > > while binding the gadget driver ... static config.  That's
> > > > with current GIT.  Peripheral-only had the same issue ISTR.
> > > 
> > > This was with the new "CDC Composite" driver (ECM and ACM),
> > > so maybe Anand's observation is a useful clue.  That code
> > > surely does things a bit differently than older stuff.  I
> > > can try another gadget driver later.
> > > 
> > > However that code comes up fine on all the other peripheral
> > > controller drivers I tried (at least three), suggesting the
> > > issue is with the MUSB code...
> > 
> > I took a look at it today and couldn't come up with anything, but it
> > looks like the problem appears when composite.c:config_desc() is called.
> > It looks like windows doesn't accept any of the config descriptor sent by
> > g_ether when windows sends a get_config_descriptor request.
> 
> You're talking about some other problem.  I'm talking about
> the one where it *OOPSES* while binding to the gadget driver.

Hmm, that I've never seend. Using musb with omap3 and n810.

> > > > Host mode seemed to come up partially.  There seem to be
> > > > issues with control-OUT transfers, which caused problems
> > > > trying to use the Ethernet adapter I was hooking up.
> > 
> > I've got a dlink adapter at work, I'll see if I can reproduce an try to
> > patch.
> 
> This one's a bit old ... "DSB-650TX Ethernet", 10-BaseT,
> full speed, pegasus driver.

Bummer, I don't have full speed ones...

> > Beagle should be sourcing up to 200mA. See if this patch helps:
> 
> Just 200 mA?  That could explain a lot.  Though looking
> at the Beagle TRM, it says "100mA" (as with TUSB) ...
> some version of this patch seems necessary.  (I have some
> others to usb-musb.c, will send them all.)

Yeah, the TRM says 100mA but I could use 200mA storage devices with
heavy transfers without any problems.

Cool, please send them all. That file is not on mainline yet, so please
send to linux-omap, Tony should apply them here before merging
usb-musb.c upstream.

> > diff --git a/arch/arm/mach-omap2/usb-musb.c b/arch/arm/mach-omap2/usb-musb.c
> > index 3f90a93..a3f37ee 100644
> > --- a/arch/arm/mach-omap2/usb-musb.c
> > +++ b/arch/arm/mach-omap2/usb-musb.c
> > @@ -129,6 +129,7 @@ static struct musb_hdrc_platform_data musb_plat = {
> >                         : "usbhs_ick",
> >         .set_clock      = musb_set_clock,
> >         .config         = &musb_config,
> > +       .power          = 100 /* up to 200mA */
> >  };
> >  
> >  static u64 musb_dmamask = ~(u32)0;
> > 
> > 
> > > Next test:  can using an external (powered) hub avoid this
> > > VBUS_ERR on the Beagle's OTG port.
> > 
> > Should help.
> 
> Did help.  But I confirmed some root hub problems ... after I
> removed the Ethernet adapter, it refused to enumerate the hub.
> Didn't even try... I had to reboot.  This particular hub is
> a bit odd, which may explain why I had to reboot with the
> hub connected ... it wouldn't enumerate otherwise.

Do you have the BLACKLIST_HUB set? (just to be sure).

> > I think I recall seeing this with my sniffer, but as it seemed not be
> > generating much problems, I let it be since I had other stuff to check.
> > Looks like I was wrong... :-s
> 
> The ep3in/status stuff is no problem.  The problem is the VBUS_ERR irq.

That irq is buggy, we had to put a retry since musb was rasing that
interrupt durring enumeration for a few usb devices (bad capacitors?).

> Which is explained by wrongly initializing the root hub power capabilities.
> If the root hub can't support 500 mA per port, it's got to say so!

Yeah... I agree here. That power field was missing.

> > > ------------[ cut here ]------------
> > > WARNING: at drivers/usb/musb/musb_host.c:125 musb_h_tx_flush_fifo+0xbc/0xdc()
> > > Could not flush host TX0 fifo: csr: 000a
> > > [<c002a45c>] (dump_stack+0x0/0x14) from [<c004cf18>] (warn_slowpath+0x60/0x7c)
> > > [<c004ceb8>] (warn_slowpath+0x0/0x7c) from [<c01ee64c>] (musb_h_tx_flush_fifo+0xbc/0xdc)
> > >  r3:00000000 r2:c032a8f8
> > >  r6:c8800102 r5:ffffffff r4:0000000a
> > > [<c01ee590>] (musb_h_tx_flush_fifo+0x0/0xdc) from [<c01ef440>] (musb_cleanup_urb+0xbc/0x110)
> > >  r8:00000000 r7:c7976980 r6:c8800100 r5:00000000 r4:c785621c
> > > [<c01ef384>] (musb_cleanup_urb+0x0/0x110) from [<c01efb68>] (musb_urb_dequeue+0x13c/0x16c)
> > > [<c01efa2c>] (musb_urb_dequeue+0x0/0x16c) from [<c01d3fe4>] (unlink1+0x6c/0xe4)
> > > ...
> > > ---[ end trace 3e2a9bb5b77f00a0 ]---
> > 
> > musb overcurrent protection seems to be broken. Something else for next
> > week.
> 
> Well, *recovery* seems broken, yes.  I'm not sure this got properly
> reported as an overcurrent event to the root hub code, either.

at least for tusb we had quite a big trouble making it work nicely on
n810 times. Take a look at tusb6010.c:tusb_otg_ints()

 741                         case OTG_STATE_A_WAIT_VFALL:
 742                                 /* REVISIT this irq triggers during short
 743                                  * spikes caused by enumeration ...
 744                                  */
 745                                 if (musb->vbuserr_retry) {
 746                                         musb->vbuserr_retry--;
 747                                         tusb_source_power(musb, 1);
 748                                 } else {
 749                                         musb->vbuserr_retry
 750                                                 = VBUSERR_RETRY_COUNT;
 751                                         tusb_source_power(musb, 0);

and musb_core.c:musb_stage0_irq()

 536                 case OTG_STATE_A_WAIT_VRISE:
 537                         if (musb->vbuserr_retry) {
 538                                 musb->vbuserr_retry--;
 539                                 ignore = 1;
 540                                 devctl |= MUSB_DEVCTL_SESSION;
 541                                 musb_writeb(mbase, MUSB_DEVCTL, devctl);
 542                         } else {
 543                                 musb->port1_status |=
 544                                           (1 << USB_PORT_FEAT_OVER_CURRENT)
 545                                         | (1 << USB_PORT_FEAT_C_OVER_CURRENT);
 546                         }
 547                         break;

Maybe we need a bit of change there, and instead of if, use a while
loop, true until vbuserr_retry goes to 0, something like:

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index b398776..bcd0832 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -534,15 +534,15 @@ static irqreturn_t musb_stage0_irq(struct musb *musb, u8 int_usb,
                         */
                case OTG_STATE_A_WAIT_BCON:
                case OTG_STATE_A_WAIT_VRISE:
-                       if (musb->vbuserr_retry) {
-                               musb->vbuserr_retry--;
+                       while(--vbuserr_retry) {
                                ignore = 1;
                                devctl |= MUSB_DEVCTL_SESSION;
                                musb_writeb(mbase, MUSB_DEVCTL, devctl);
-                       } else {
-                               musb->port1_status |=
-                                         (1 << USB_PORT_FEAT_OVER_CURRENT)
-                                       | (1 << USB_PORT_FEAT_C_OVER_CURRENT);
+
+                               if (vbuserr_retry == 0)
+                                       musb->port1_status |=
+                                               (1 << USB_PORT_FEAT_OVER_CURRENT)
+                                               | (1 << USB_PORT_FEAT_C_OVER_CURRENT);
                        }
                        break;
                default:

Didn't test though. If it works nicely, the same logic will have to go
to tusb6010.c

-- 
balbi
--
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