Re: [PATCH v2] USB: Add MSM USB Device Controller driver

[Matthieu CASTET <matthieu.castet@parrot.com>]

[-- Attachment #1: Type: text/plain, Size: 2019 bytes --]

Hi,

Brian Swetland a écrit :
> On Tue, Nov 9, 2010 at 6:54 PM, David Brownell <david-b@pacbell.net> wrote:
>>
>> --- On Tue, 11/9/10, Pavan Kondeti <pkondeti@codeaurora.org> wrote:
>>
>>>>> Hi Matthieu,
>>>>>
>>>>>> This look like the arc/chipidea/mips ehci otg
>>> core.
>>>>> Yes. It is chipidea core for ARM.
>>>>>> Why can't you reuse the ci13xxx_udc.c driver
>> That basic approach is FAR PREFERABLE.  Fix
>> the bugs once, tune once, and so forth, reuse
>> the ULPI support, etc.  Work on more
>> platforms, since the silicon IP is reused.
>>
>> You'll end up with more folk who can help
>> maintain the driver too, since the pool of
>> potential helpers won't be limited to those
>> who have/use MSM hardware.
>>
>> Just be sure to cleanly factor the bus
>> (PCI vs MSM-s ARM platform flavor and
>> SoC glues (bus-related).  That factoring
>> will likely be the hardest part; but there
>> are examples of similar stuff in Linux today.
> 
> The main headache is that this particular IP has different bugs in
> different instantiations (I know, for example, it exists in Tegra with
> a different set of issues around fetching descriptor heads and cache
> alignment, on MSM7201A after extensive testing we discovered there was
> no reliable way of adding a descriptor to a list of transactions once
> that queue was active, etc...), so things that work in one SoC may
> break another, etc, etc, but that's part of the adventure I suppose.
> I certainly agree that one unified driver is the way to go if you can
> make it all work.
> 
The best way to handle this is to introduce flags in the driver. For 
example look at drivers/mmc/host/sdhci.c (quirk flags).

But for now, let's make work msm version. We can add workaround for 
other controller later.

Now you should check if it is better to start on ci13xxx_udc or make 
generic your driver.

I had worked a bit on ci13xxx_udc, and the code is sometimes messy, hard 
to understand.
Also to making work on our core, we need to the attached patch.


Matthieu



[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 1112 bytes --]

diff --git a/drivers/usb/gadget/ci13xxx_udc.c b/drivers/usb/gadget/ci13xxx_udc.c
index 0252bbc..5a65cda 100644
--- a/drivers/usb/gadget/ci13xxx_udc.c
+++ b/drivers/usb/gadget/ci13xxx_udc.c
@@ -2140,6 +2140,7 @@ static int ep_queue(struct usb_ep *ep, struct usb_request *req,
 	struct ci13xxx_req *mReq = container_of(req, struct ci13xxx_req, req);
 	int retval = 0;
 	unsigned long flags;
+	int empty;
 
 	trace("%p, %p, %X", ep, req, gfp_flags);
 
@@ -2170,15 +2171,18 @@ static int ep_queue(struct usb_ep *ep, struct usb_request *req,
 
 	dbg_queue(_usb_addr(mEp), req, retval);
 
+	empty = list_empty(&mEp->qh[mEp->dir].queue);
 	/* push request */
 	mReq->req.status = -EINPROGRESS;
 	mReq->req.actual = 0;
 	list_add_tail(&mReq->queue, &mEp->qh[mEp->dir].queue);
 
-	retval = _hardware_enqueue(mEp, mReq);
-	if (retval == -EALREADY || retval == -EBUSY) {
-		dbg_event(_usb_addr(mEp), "QUEUE", retval);
-		retval = 0;
+	if (empty) {
+		retval = _hardware_enqueue(mEp, mReq);
+		if (retval == -EALREADY || retval == -EBUSY) {
+			dbg_event(_usb_addr(mEp), "QUEUE", retval);
+			retval = 0;
+		}
 	}
 
  done: