So to oversimplify, this controller builds on a non-PCI implementation
of EHCI, by reusing much of the core silicon design ... including most
usefully DMA queues which actually (on first glance) make sense. This
might be the first peripheral controller I've seen with a sensible DMA
interface, facilitating transfer queues as deep as host controllers do
(since it's basically using a host controller DMA engine)!
Please submit the gadget_chips.h part of this patch right away; that
would be handy in terms of at least reserving relevant symbols.
The rest still needs work. For example, I like to avoid merging code
that clearly can't pass milestones like the USBCV tests.
- Dave
> +#if defined(CONFIG_FSL_USB_OTG) || defined(CONFIG_FSL_USB_OTG_MODULE)
> +static const char driver_name[] = "fsl-usb2-dr-udc";
> +#else
> +static const char driver_name[] = "fsl-usb2-dr";
> +#endif
The driver name should not change here...
> + /* 5 configurable endpoints */
> + "ep1out",
> + "ep1in",
> + "ep2out",
> + "ep2in",
> + "ep3out",
> + "ep3in",
> + "ep4out",
> + "ep4in",
> + "ep5out",
> + "ep5in"
That's 10 endpoints not 5 ... the way to expose an endpoint which can
be used in either direction (but only one at a time) is to name it "ep1",
"ep2" etc and then use the direction specified in the endpoint descriptor
when the endpoint is enabled. If "ep1out" and "ep1in" can be used at the
same time, then describe that as 10 endpoints. (Endpoint ~= FIFO).
> +/*-----------------------------------------------------------------
> + * done() - retire a request; caller blocked irqs
> + * @status : when req->req.status is -EINPROGRESSS, it is input para
> + * else it will be a output parameter
Bad API policy ... luckily it's not the one you use!! I think what
you mean to say is that it's used as the request status unless some
fault has already been marked in req->req.status
> +/*-----------------------------------------------------------------
> + * nuke(): delete all requests related to this ep
> + * called by ep_disable() within spinlock held
> + * add status paramter?
> + *--------------------------------------------------------------*/
> +static void nuke(struct mpc_ep *ep, int status)
Sure looks to me like it _has_ a status parameter already.
While I generally approve of function docs, the trick is
as always keeping them accurate. Better to not have them
(given self-descriptive names) than to have inaccurate docs.
> + /* Wait reset completed */
> + while (le32_to_cpu(usb_slave_regs->usbcmd) & USB_CMD_CTRL_RESET) {
> + }
Two comments. One, style: usually I like to do such loops as
"while (condition) cpu_relax();" since that's more explicitly a
NOP, though it does nice things for bus access on some hardware.
Second, endless loops in drivers can be bad; for paranoia, add
a limit so the system can't lock up forever in case of hardware
breakage.
> + /* Config snooping, keep the cache consistent
> + * Snoop between 0x0 and 1GB */
> + tmp = 0;
> + tmp &= SNOOP_ADDRESS_MASK;
> + usb_sys_regs->snoop1 = tmp | SNOOP_SIZE_1GB;
Hmm, this is a bit odd. Is it actually necessary? In general it's
better to trust the software not to have the sort of bugs this would
be covering up. It's something I'd expect not all implementations
would support ... e.g. good luck with this on a version integrated to
an ARM-based SOC. And how do you know that 1GB is the right amount
of RAM address space to snoop? Vs e.g. 2GB, or 128 KB....
Also, that style of register access is not the one approved over
all architectures. Normally a functional style should be used,
like "writel(tmp|SNOOP_SIZE_1_GB, ®s->snoop1)", since that's
both easier to interpose on (e.g. when someone else debugs) and
also harder to abuse ... e.g. I don't know PPC at that level, but
many RISCs don't have read-modify-write that works on I/O registers,
and it's all too easy to code "regs |= mask;" and make the compiler
issue such instructions. (Likewise there's a performance argument:
memory access is usually faster than i/o registers, if for no other
reason that it's cachable.)
Especially if FreeScale is open to using this core on its other
chips, e.g. ARM, you should change your i/o register access idioms.
> +static int mpc_ep_enable(struct usb_ep *_ep,
> + const struct usb_endpoint_descriptor *desc)
> +{
> + struct mpc_udc *udc = NULL;
> + struct mpc_ep *ep = NULL;
> + unsigned short max = 0;
> + unsigned char mult = 0, zlt = 0;
> + int retval = 0;
> + unsigned long flags = 0;
> + char *val = NULL; /* for debug */
> +
> + ep = container_of(_ep, struct mpc_ep, ep);
> +
> + /* catch various bogus parameters */
> + if (!_ep || !desc || ep->desc || _ep->name == ep_name[0] ||
> + (desc->bDescriptorType != USB_DT_ENDPOINT))
> + /* FIXME: add judge for ep->bEndpointAddress */
Yes, that sounds like something that should be fixed. :)
> + return -EINVAL;
> +
> + udc = ep->udc;
> +
> + if (!udc->driver || (udc->gadget.speed == USB_SPEED_UNKNOWN))
> + return -ESHUTDOWN;
> +
> + max = le16_to_cpu(desc->wMaxPacketSize);
> + retval = -EINVAL;
> +
> + /* check the max package size validate for this endpoint */
> + /* Refer to USB2.0 spec table 9-13,
> + */
> + switch (desc->bmAttributes & 0x03) {
> + case USB_ENDPOINT_XFER_BULK:
> + if (strstr(ep->ep.name, "-iso")
> + || strstr(ep->ep.name, "-int"))
> + goto en_done;
... but you don't hand out names like that, so why bother testing?
> + mult = 0;
> + zlt = 1;
> + switch (udc->gadget.speed) {
> + case USB_SPEED_HIGH:
> + if ((max == 128) || (max == 256) || (max == 512))
> + break;
The correct test there is that max must be 512; everything else is an error.
> + default:
> + switch (max) {
> + case 4:
> + case 8:
> + case 16:
> + case 32:
> + case 64:
> + break;
> + default:
> + case USB_SPEED_LOW:
> + goto en_done;
Hmm, USB_SPEED_LOW is NOT a packet size ... or even possible as a
speed for this controller.
> + }
> + }
> + break;
> + case USB_ENDPOINT_XFER_INT:
> + if (strstr(ep->ep.name, "-iso")) /* bulk is ok */
> + goto en_done;
As above, you don't publish such names so that couldn't ever be ...
> + mult = 0;
> + zlt = 1;
> + switch (udc->gadget.speed) {
> + case USB_SPEED_HIGH:
> + if (max <= 1024)
> + break;
Evidently you don't support high bandwidth modes; if so,
that's worth a coment.
> + case USB_SPEED_FULL:
> + if (max <= 64)
> + break;
> + default:
> + if (max <= 8)
> + break;
This would be the SPEED_LOW case, right? If so, don't hide it.
> + goto en_done;
> + }
> + break;
> + case USB_ENDPOINT_XFER_ISOC:
> + if (strstr(ep->ep.name, "-bulk") || strstr(ep->ep.name, "-int"))
> + goto en_done;
Same comment about not publishing such names.
> + mult = (unsigned char) (1 +((le16_to_cpu(desc->wMaxPacketSize)
> + >> 11) & 0x03));
> + zlt = 0;
> + switch (udc->gadget.speed) {
> + case USB_SPEED_HIGH:
> + if (max <= 1024)
> + break;
Same comment about high bandwidth ... though here it looks like you
were trying. The problem is that "mult" is still in the high bits
of "max", so if that's nonzero then the above test fails.
> + case USB_SPEED_FULL:
> + if (max <= 1023)
> + break;
> + default:
> + goto en_done;
> + }
> + break;
> + case USB_ENDPOINT_XFER_CONTROL:
> + if (strstr(ep->ep.name, "-iso")
> + || strstr(ep->ep.name, "-int"))
> + goto en_done;
Same comment about not publishing. Plus it's surprising to me that you
even have code to enable a control endpoint ... yes, especially since you
rejected ep0 earlier. This whole case can/should vanish.
> + mult = 0;
> + zlt = 1;
> + switch (udc->gadget.speed) {
> + case USB_SPEED_HIGH:
> + case USB_SPEED_FULL:
> + switch (max) {
> + case 1:
> + case 2:
> + case 4:
> + case 8:
> + case 16:
> + case 32:
> + case 64:
> + break;
> + default:
> + goto en_done;
> + }
> + case USB_SPEED_LOW:
> + switch (max) {
> + case 1:
> + case 2:
> + case 4:
> + case 8:
> + break;
> + default:
> + goto en_done;
> + }
> + default:
> + goto en_done;
> + }
> + break;
> +
> + default:
> + goto en_done;
> + }
> +
> + /* here initialize variable of ep */
> + spin_lock_irqsave(&udc->lock, flags);
> + ep->ep.maxpacket = max;
> + ep->desc = desc;
> + ep->stopped = 0;
> +
> + /* hardware special operation */
> +
> + /* Init EPx Queue Head (Ep Capabilites field in QH
> + * according to max, zlt, mult) */
> + struct_ep_qh_setup((void *) udc, (unsigned char) ep_index(ep),
> + (unsigned char) ((desc->
> + bEndpointAddress & USB_DIR_IN) ?
> + USB_SEND : USB_RECV),
Here you take the endpoint direction from the descriptor, but earlier
you hard-coded it into the endpoint name. I suspect the earlier code
is wrong ... but if it isn't, then here you should be comparing the
hard-wired direction against the one in the endpoint name, and insist
that they match.
> + (unsigned char) (desc->bmAttributes &
> + USB_ENDPOINT_XFERTYPE_MASK),
> + max, zlt, mult);
> +
> + /* Init endpoint x at here */
> + /* 83xx RM chapter 16.3.2.24, here init the endpoint ctrl reg */
> + dr_ep_setup((unsigned char) ep_index(ep),
> + (unsigned char) ((desc->bEndpointAddress & USB_DIR_IN)
> + ? USB_SEND : USB_RECV),
> + (unsigned char) (desc->bmAttributes &
> + USB_ENDPOINT_XFERTYPE_MASK));
> +
> + /* Now HW will be NAKing transfers to that EP,
> + * until a buffer is queued to it. */
Good to highlight that; not all hardware will NAK an OUT transfer at
that point... though obviously everything NAKs an IN transfer here.
> + /* should have stop the lock */
> + spin_unlock_irqrestore(&udc->lock, flags);
> + retval = 0;
> + switch (desc->bmAttributes & 0x03) {
> + case USB_ENDPOINT_XFER_BULK:
> + val = "bulk";
> + break;
> + case USB_ENDPOINT_XFER_ISOC:
> + val = "iso";
> + break;
> + case USB_ENDPOINT_XFER_INT:
> + val = "intr";
> + break;
> + default:
> + val = "ctrl";
> + break;
> + }
> +
> + VDBG("enabled %s (ep%d%s-%s) maxpacket %d",ep->ep.name,
> + ep->desc->bEndpointAddress & 0x0f,
> + (desc->bEndpointAddress & USB_DIR_IN) ?
> + "in" : "out", val, max);
> +en_done:
> + return retval;
> +}
> +
> +/*---------------------------------------------------------------------
> + * @ep : the ep being unconfigured. May not be ep0
> + * Any pending and uncomplete req will complete with status (-ESHUTDOWN)
> +*---------------------------------------------------------------------*/
> +static int mpc_ep_disable(struct usb_ep *_ep)
> +{
> + struct mpc_udc *udc = NULL;
> + struct mpc_ep *ep = NULL;
> + unsigned long flags = 0;
> +
> + ep = container_of(_ep, struct mpc_ep, ep);
> + if (!_ep || !ep->desc) {
> + VDBG("%s not enabled", _ep ? ep->ep.name : NULL);
> + return -EINVAL;
> + }
> +
> + udc = (struct mpc_udc *) ep->udc;
> +
> + spin_lock_irqsave(&udc->lock, flags);
> +
> + /* Nuke all pending requests (does flush) */
> + nuke(ep, -ESHUTDOWN);
Actually the code I read does NOT flush the fifo. And nothing here
is actually disabling the endpoint ... so an IN endpoint would be able
to (wrongly) transmit data that had been queued before, and an OUT
endpoint would be able to receive packets. They should both be ignoring
transfers (not even NAKing them) after the endpoint is disabled.
> +
> + ep->desc = 0;
> + ep->stopped = 1;
> + spin_unlock_irqrestore(&udc->lock, flags);
> +
> + VDBG("disabled %s OK", _ep->name);
> + return 0;
> +}
> +static void *mpc_alloc_buffer(struct usb_ep *_ep, unsigned bytes,
> + dma_addr_t * dma, gfp_t gfp_flags)
> +{
> + void *retval = NULL;
> +
> + if (!bytes)
> + return 0;
> +
> + retval = kmalloc(bytes, gfp_flags);
> + if (retval)
> + *dma = virt_to_phys(retval);
That's why you enabled the cache snooping, I guess ... as an alternative
to calling dma_alloc_coherent() here.
> + return retval;
> +}
> +static int mpc_queue_td(struct mpc_ep *ep, struct mpc_req *req)
> +{
> + int i = ep_index(ep) * 2 + ep_is_in(ep);
> + u32 temp, bitmask, tmp_stat;
> + struct ep_queue_head *dQH = &ep->udc->ep_qh[i];
> +
> + //VDBG("QH addr Register 0x%8x", usb_slave_regs->endpointlistaddr);
> + //VDBG("ep_qh[%d] addr is 0x%8x", i, (u32)&(ep->udc->ep_qh[i]));
> +
> + bitmask = (ep_is_in(ep)) ? (1 << (ep_index(ep) + 16)) :
> + (1 << (ep_index(ep)));
> +
> + /* check if the pipe is empty */
> + if (!(list_empty(&ep->queue))) {
> + /* Add td to the end */
> + struct mpc_req *lastreq;
> + lastreq = list_entry(ep->queue.prev, struct mpc_req, queue);
> + lastreq->tail->next_td_ptr =
> + cpu_to_le32(virt_to_phys(req->head) & DTD_ADDR_MASK);
If the endpoint is stopped, you don't want to prime the fifo or
anything like that ... instead, the logic which restarts the
endpoint queue (e.g. after handling unlinks issued during the
request completion callback) should handle that.
> + /* Read prime bit, if 1 goto done */
> + if (usb_slave_regs->endpointprime & cpu_to_le32(bitmask))
> + goto out;
> +
> + do {
> + /* Set ATDTW bit in USBCMD */
> + usb_slave_regs->usbcmd |=
> + cpu_to_le32(USB_CMD_ATDTW);
> +
> + /* Read correct status bit */
> + tmp_stat = le32_to_cpu(usb_slave_regs->
> + endptstatus) & bitmask;
> +
> + } while (!(usb_slave_regs->
> + usbcmd & cpu_to_le32(USB_CMD_ATDTW)));
> +
> + /* Write ATDTW bit to 0 */
> + usb_slave_regs->usbcmd &= cpu_to_le32(~USB_CMD_ATDTW);
> +
> + if (tmp_stat)
> + goto out;
> + }
> +
> + /* Write dQH next pointer and terminate bit to 0 */
> + temp = virt_to_phys((void *)req->head) &
> + EP_QUEUE_HEAD_NEXT_POINTER_MASK;
> + dQH->next_dtd_ptr = cpu_to_le32(temp);
> +
> + /* Clear active and halt bit */
> + temp = cpu_to_le32(~(EP_QUEUE_HEAD_STATUS_ACTIVE |
> + EP_QUEUE_HEAD_STATUS_HALT));
Ditto -- that stuff shouldn't be done when queueing to an endpoint
that's been stopped during a callback (e.g. for fault cleanup).
> + dQH->size_ioc_int_sts &= temp;
> + flush_dcache_range((unsigned long)dQH,(unsigned long)dQH +
> + sizeof(struct ep_queue_head));
> +
> + /* Prime endpoint by writing 1 to ENDPTPRIME */
> + temp = (ep_is_in(ep)) ? (1 << (ep_index(ep) + 16)) :
> + (1 << (ep_index(ep)));
> +
> + usb_slave_regs->endpointprime = cpu_to_le32(temp);
> +out:
> + return 0;
> +}
> + /* check alignment */
> + if ((u32) dtd & ~DTD_ADDR_MASK)
> + panic("Can not allocate aligned memory for dtd");
That's what BUG_ON(dtd & ~DTD_ADDR_MASK) would be for ... please use
standard Linux idioms.
> +
> + /* Fill in the transfer size; set interrupt on every dtd;
> + set active bit */
> + swap_temp = ((length << DTD_LENGTH_BIT_POS) | DTD_IOC
> + | DTD_STATUS_ACTIVE);
> +
Well, there's the invisible whitespace-at-end-of-line glitch there (and
some other places). Please fix that.
And also there's my curiousity: why an IRQ after every DTD? Not that
peripherals are currently likely to issue more than 20KB per usb_request,
but surely (a) a 40 KB request only requires a single IRQ, not two, and
(b) the usb_request.no_interrupt hint should be usable here. Those are
basically just performance issues, not correctness ones; but on the host
side with EHCI, avoiding IOC was a significant win in terms of IRQ rate
since a high speed DMA queue can cause a LOT of needless irqs.
> + dtd->size_ioc_sts = cpu_to_le32(swap_temp);
> +
> + /* Clear reserved field */
> + swap_temp = cpu_to_le32(dtd->size_ioc_sts);
> + swap_temp &= ~DTD_RESERVED_FIELDS;
> + dtd->size_ioc_sts = cpu_to_le32(swap_temp);
> +
> + /* Init all of buffer page pointers */
> + swap_temp = (u32) (req->req.dma + req->req.actual);
> + dtd->buff_ptr0 = cpu_to_le32(swap_temp);
> + dtd->buff_ptr1 = cpu_to_le32(swap_temp + 0x1000);
> + dtd->buff_ptr2 = cpu_to_le32(swap_temp + 0x2000);
> + dtd->buff_ptr3 = cpu_to_le32(swap_temp + 0x3000);
> + dtd->buff_ptr4 = cpu_to_le32(swap_temp + 0x4000);
> +
> + req->req.actual += length;
> + *address = dtd;
Since you updated the DMA queue here, at least a wmb() should
be put here ... likely even a full mb(), e.g. using
set_mb(*address, dtd);
> + VDBG("length = %d address= 0x%x", length, (int) dtd);
> +
> + return length;
> +}
> +static int
> +mpc_req_to_dtd (struct mpc_req *req)
> +{
> + unsigned max;
> + unsigned count;
> + int is_last;
> + int is_first =1;
> + struct ep_td_struct *last_addr = NULL, *addr;
> +
> + max = EP_MAX_LENGTH_TRANSFER;
> + do {
> + count = mpc_build_dtd(req, max, &addr);
> +
> + if (is_first) {
> + is_first = 0;
> + req->head = addr;
> + }
> + else {
> + last_addr->next_td_ptr
> + = cpu_to_le32(virt_to_phys(addr));
> + flush_dcache_range((unsigned long)last_addr,
> + (unsigned long)last_addr +
> + sizeof(struct ep_td_struct));
Hmm ... I'm curious why you're using explicit cache flush primitives
rather than just using a dma_pool for the TD structures. Using the
dma_pool is obviously correct. That's far from true with explicit
cache flushing; it's tricky to get the caches properly flushed, since
the controller might be writing the TDs (and QHs) while you read...
> + last_addr = addr;
> + }
> + /* last packet is usually short (or a zlp) */
> + if (unlikely (count != max))
> + is_last = 1;
> + else if (likely(req->req.length != req->req.actual)
> + || req->req.zero)
> + is_last = 0;
> + else
> + is_last = 1;
> +
> + req->dtd_count ++;
> + }while(!is_last);
> +
> + addr->next_td_ptr = cpu_to_le32(DTD_NEXT_TERMINATE);
> + flush_dcache_range((unsigned long)addr, (unsigned long)addr +
> + sizeof(struct ep_td_struct));
> +
> + req->tail = addr;
> +
> + return 0;
> +}
> +
> +/* queues (submits) an I/O request to an endpoint */
> +static int
> +mpc_ep_queue(struct usb_ep *_ep, struct usb_request *_req, gfp_t gfp_flags)
> +{
> + struct mpc_ep *ep = container_of(_ep, struct mpc_ep, ep);
> + struct mpc_req *req = container_of(_req, struct mpc_req, req);
> + struct mpc_udc *udc;
> + unsigned long flags;
> + int is_iso = 0;
> +
> + /* catch various bogus parameters */
> + if (!_req || !req->req.complete || !req->req.buf
> + || !list_empty(&req->queue) ) {
> + VDBG("%s, bad params\n", __FUNCTION__);
> + return -EINVAL;
> + }
> + if (!_ep || (!ep->desc && ep_index(ep))) {
> + VDBG("%s, bad ep\n", __FUNCTION__);
> + return -EINVAL;
> + }
> + if (ep->desc->bmAttributes == USB_ENDPOINT_XFER_ISOC) {
> + if (req->req.length > ep->ep.maxpacket)
> + return -EMSGSIZE;
> + is_iso = 1;
> + }
> +
> + udc = ep->udc;
> + if (!udc->driver || udc->gadget.speed == USB_SPEED_UNKNOWN)
> + return -ESHUTDOWN;
> +
> + req->ep = ep;
> +
> + /* map virtual address to hardware */
> + if (req->req.dma == DMA_ADDR_INVALID) {
> + req->req.dma = dma_map_single(ep->udc->gadget.dev.parent,
> + req->req.buf,
> + req->req.length,
> + ep_is_in(ep)
> + ? DMA_TO_DEVICE :
> + DMA_FROM_DEVICE);
> + req->mapped = 1;
> + } else {
> + dma_sync_single_for_device(ep->udc->gadget.dev.parent,
> + req->req.dma, req->req.length,
> + ep_is_in(ep)
> + ? DMA_TO_DEVICE :
> + DMA_FROM_DEVICE);
> + req->mapped = 0;
> + }
> +
> + req->req.status = -EINPROGRESS;
> + req->req.actual = 0;
> + req->dtd_count = 0;
> +
> + spin_lock_irqsave(&udc->lock, flags);
> +
> + /* push the dtds to device queue */
> + if (!mpc_req_to_dtd(req))
> + mpc_queue_td(ep, req);
There's an "else" fault path missing there ...
> + /* EP0 */
> + if ((ep_index(ep) == 0))
> + udc->ep0_state = DATA_STATE_XMIT;
> +
> + /* irq handler advances the queue */
> + if (req != NULL)
> + list_add_tail(&req->queue, &ep->queue);
> + spin_unlock_irqrestore(&udc->lock, flags);
> +
> + return 0;
> +}
> +
> +/* dequeues (cancels, unlinks) an I/O request from an endpoint */
> +static int mpc_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> +{
> + struct mpc_ep *ep = container_of(_ep, struct mpc_ep, ep);
> + struct mpc_req *req;
> + unsigned long flags;
> +
> + if (!_ep || !_req)
> + return -EINVAL;
> +
> + spin_lock_irqsave(&ep->udc->lock, flags);
> +
> + /* make sure it's actually queued on this endpoint */
> + list_for_each_entry(req, &ep->queue, queue) {
> + if (&req->req == _req)
> + break;
> + }
> + if (&req->req != _req) {
> + spin_unlock_irqrestore(&ep->udc->lock, flags);
> + return -EINVAL;
> + }
> +
> + done(ep, req, -ECONNRESET);
That looks pretty clearly wrong. The issue is that it races ... you
need to mark the request as pending-dequeue, then cleanly stop the
endpoint DMA queue, and then carefully pluck that request out of the
queue and patch the DMA queue before restarting the endpoint's queue.
In fact I didn't see any code to patch up the DMA queue on this code
path ...
The basic scenarios here are:
* Controller already completed the transfer, but it's just not been
handed back yet. The DMA queue will have advanced already, so
this is a pretty safe case in terms of hardware interference.
* Controller is still working on this transfer. So clearly you've
got to stop that work before returning. Of course, maybe it'll
finish that work before you can stop the DMA engine ... in which
case this will (later) resemble the case above.
* Controller hasn't yet gotten to this transfer. Well, you can't
just patch the DMA queue, since the controller _could_ finish a
previous transfer and start on this one before you finish...
I hope you see what I mean. Handling unlinks with a DMA queue is
unfortunately messy. It's somewhat simpler if you just have a single
DMA transfer queued at a time, since that last case never happens,
but you can't get away from needing to stop the DMA queue and then
clean up from the first two cases ...
> + spin_unlock_irqrestore(&ep->udc->lock, flags);
> + return 0;
> +
> +}
> +static int mpc_ep_set_halt(struct usb_ep *_ep, int value)
> +{
> + return (_mpc_ep_set_halt(_ep, value));
> +}
You should not have two separate functions for this, when one is nothing
more than a call to the other ... ;)
> +/*-----------------------------------------------------------------------
> + * Tries to wake up the host connected to this gadget
> + *
> + * Return : 0-success
> + * Negative-this feature not enabled by host or not supported by device hw
> + * FIXME: RM 16.6.2.2.1 DR support this wake-up feature?
Extremely unlikely that the device not support wakeup.
However, it must be enabled by the host ... this is testable
with gadget zero in "autoresume" mode.
> + -----------------------------------------------------------------------*/
> +static int mpc_wakeup(struct usb_gadget *gadget)
> +{
> + return -ENOTSUPP;
> +}
> +
> +/* sets the device selfpowered feature
> + * this affects the device status reported by the hw driver
> + * to reflect that it now has a local power supply
> + * usually device hw has register for this feature
> + */
> +static int
> +mpc_set_selfpowered(struct usb_gadget *gadget, int is_selfpowered)
> +{
> + return -ENOTSUPP;
> +}
If you don't implement those methods (yet), then just don't provide
them in the function table ...
> +static void Ep0Stall(struct mpc_udc *udc)
Style: AvoidCaMeLcAsEdSyMbOlSiNlInUxCode.
> +static int udc_reset_ep_queue(struct mpc_udc *udc, u8 pipe)
The one place I noticed this getting used, the ECONNRESET
status was wrong .. it should have been ESHUTDOWN to indicate
the connection went away. The routine is sufficiently obvious
that it should probably just be an inlined call to nuke().
> +static void ch9GetStatus(struct mpc_udc *udc, u16 value, u16 index,
CamelCaseBad.
This is _device_ status yes? Not endpoint status?
> + u16 length)
> +{
> + u16 usb_status = 0; /* fix me to give correct status */
> +
> + struct mpc_req *req;
> + struct mpc_ep *ep;
> + int status = 0;
> +
> + ep = &udc->eps[0];
> +
> + req = container_of(mpc_alloc_request(&ep->ep, GFP_KERNEL),
> + struct mpc_req, req);
> + req->req.length = 2;
> + req->req.buf = &usb_status;
This is a bad approach, since that stack-based buffer will vanish when this
function returns and since the queue() is async ...
For device status you're best just having a u16 bitmask in the per-device
structure, updated according to the selfpowered (see above, you don't
implement this!) and remote-wakeup-enable (host-controlled) bits ... and
for later OTG support, for the three HNP-related bits.
> + req->req.status = -EINPROGRESS;
> + req->req.actual = 0;
> +
> + /* data phase */
> + if ((mpc_req_to_dtd(req) == 0))
> + status = mpc_queue_td(ep, req);
> + if (status) {
> + printk("Can't respond to getstatus request \n");
> + Ep0Stall(udc);
> + } else
> + udc->ep0_state = DATA_STATE_XMIT;
> +}
> +
> +/*
> + * ch9 Set config
> + */
> +static void ch9SetConfig(struct mpc_udc *udc, u16 value, u16 index,
CamelCaseIsStillBad.
> + u16 length)
> +{
> +
> + udc->ep0_dir = USB_DIR_IN;
> + if (udc->driver->setup(&udc->gadget, &udc->local_setup_buff) >= 0)
> + {
> + /* gadget layer deal with the status phase */
> + udc->usb_state = USB_STATE_CONFIGURED;
> + udc->ep0_state = WAIT_FOR_OUT_STATUS;
Likely if I did more than skim those chip specs I'd understand why this
code even cared about usb_state being "configured" ... but as a rule,
that's not something a controller driver should care about unless it's
forced to, kicking and screaming, by hardware. SET_ADDRESS is a bit
different because the address isn't supposed to take effect until after
the status stage is successful ... but caring about SET_CONFIGURATION
is a layering violation that good hardware doesn't require...
> + }
> +}
> +
> +static void setup_received_irq(struct mpc_udc *udc,
> + struct usb_ctrlrequest *setup)
> +{
> + u16 wValue = le16_to_cpu(setup->wValue);
> + u16 wIndex = le16_to_cpu(setup->wIndex);
> + u16 wLength = le16_to_cpu(setup->wLength);
> +
> + udc_reset_ep_queue(udc, 0);
> +
> + /* We asume setup only occurs on EP0 */
> + if (setup->bRequestType & USB_DIR_IN)
> + udc->ep0_dir = USB_DIR_IN;
> + else
> + udc->ep0_dir = USB_DIR_OUT;
> +
> + switch (setup->bRequest) {
> + case USB_BULK_RESET_REQUEST:
This is not a standard request. Why is it special cased?
The rule is that every request _except_ the handful that need to be dealt
with in the controller driver just gets handed to the gadget driver. So
there's no point in special casing things the gadget driver should handle.
> + if ((setup->bRequestType & USB_TYPE_MASK) != USB_TYPE_CLASS)
> + break;
> + udc->ep0_dir = USB_DIR_IN;
> + if (udc->driver->setup(&udc->gadget,
> + &udc->local_setup_buff) >= 0)
> + udc->ep0_state = WAIT_FOR_SETUP;
> + break;
> +
> + case USB_REQ_GET_STATUS:
> + if ((setup-> bRequestType & (USB_DIR_IN | USB_TYPE_STANDARD))
> + != (USB_DIR_IN | USB_TYPE_STANDARD))
> + break;
> + ch9GetStatus(udc, wValue, wIndex, wLength);
> + break;
OK, I see what's up with this. There are actually three standard GET_STATUS
requests this driver should handle: for device, interface, and endpoint.
You don't handle device status correctly, or endpoint status (which should
reflect halt status).
Evidently your testing has not included USBCV (from www.usb.org) ...
> + case USB_REQ_SET_ADDRESS:
> + if (setup->bRequestType != (USB_DIR_OUT | USB_TYPE_STANDARD |
> + USB_RECIP_DEVICE))
> + break;
> + ch9SetAddress(udc, wValue, wIndex, wLength);
> + break;
Yes, this is special enough that I expect peripheral controller drivers to
need to handle it ...
> + case USB_REQ_SET_CONFIGURATION:
> + if (setup->bRequestType != (USB_DIR_OUT | USB_TYPE_STANDARD |
> + USB_RECIP_DEVICE))
> + break;
> + /* gadget layer take over the status phase */
> + ch9SetConfig(udc, wValue, wIndex, wLength);
> + break;
> + case USB_REQ_SET_INTERFACE:
> + if (setup->bRequestType != (USB_DIR_OUT | USB_TYPE_STANDARD |
> + USB_RECIP_INTERFACE))
> + break;
> + udc->ep0_dir = USB_DIR_IN;
> + if (udc->driver->setup(&udc->gadget,
> + &udc->local_setup_buff) >= 0)
> + /* gadget layer take over the status phase */
> + break;
SET_CONFIGURATION and SET_INTERFACE should always be handled by the gadget
driver. Unless this controller is interfering?
> + /* Requests with no data phase */
> + case USB_REQ_CLEAR_FEATURE:
> + case USB_REQ_SET_FEATURE:
> + { /* status transaction */
> + int rc = -EOPNOTSUPP;
> +
> + if (setup->bRequestType == USB_RECIP_ENDPOINT) {
> + int dir = (wIndex & 0x0080) ?
> + EP_DIR_IN: EP_DIR_OUT;
> + int num = (wIndex & 0x000f);
> + struct mpc_ep *ep;
> +
> + if (wValue != 0 || wLength != 0
> + || (num *2 + dir) > USB_MAX_PIPES)
> + break;
> + ep = &udc->eps[num*2+dir];
> +
> + if (setup->bRequest == USB_REQ_SET_FEATURE) {
> + rc = _mpc_ep_set_halt(&ep->ep, 1);
> + }
> + else {
> + rc = _mpc_ep_set_halt(&ep->ep, 0);
> + }
If you don't need special attention here to both behave and pass the
USBCV tests, that'll be a pleasant surprise. :)
> + } else if (setup->bRequestType == USB_RECIP_DEVICE) {
THere's remote wakeup enable here too ...
> + if (!udc->gadget.is_otg)
> + break;
> + else if (setup->bRequest == USB_DEVICE_B_HNP_ENABLE)
> + udc->gadget.b_hnp_enable = 1;
> + else if (setup->bRequest == USB_DEVICE_A_HNP_SUPPORT)
> + udc->gadget.a_hnp_support = 1;
> + else if (setup->bRequest ==
> + USB_DEVICE_A_ALT_HNP_SUPPORT)
> + udc->gadget.a_alt_hnp_support = 1;
> + rc = 0;
> + }
> + if (rc == 0) {
> + /* send status only if _mpc_ep_set_halt success */
> + if (ep0_prime_status(udc, EP_DIR_IN))
> + Ep0Stall(udc);
> + }
> + break;
> + }
> + default:
> + if (udc->driver->setup(&udc->gadget, &udc->local_setup_buff)
> + != 0) {
> + Ep0Stall(udc);
> + }
> + else if (setup->bRequestType & USB_DIR_IN)
> + udc->ep0_state = DATA_STATE_XMIT;
> + else
> + udc->ep0_state = DATA_STATE_RECV;
This XMIT vs RECV also depends on whether there's a data stage...
> + break;
> + }
> +}
> +/*process-ep_req(): free the completed Tds for this req */
> +/* FIXME: ERROR handling for multi-dtd requests */
If you didn't support multi-DTD requests you wouldn't have to
cope with their complex fault modes. :)
That's what I eventually ended up doing with net2280, but in
that case I can at least point to DMA queue design that wasn't
particularly nice; this hardware didn't look (at first glance)
to have that issue.
> +static int process_ep_req(struct mpc_udc *udc, int pipe,
> + struct mpc_req* curr_req)
> +{
uck, "pipe" ... bad notion, avoid incorporating it. It's just
a placeholder for a QH/EP, yes? Then pass the QH everywhere.
You're going to need a routine that can scan a QH in two modes:
(a) live, where you pull off only DTDs that have been completed
by the hardware, and (b) stopped, where DMA has been deactivated
so that you can do things like handle dequeue() requests and then
patch up the QH. The live case will have to handle faults, which
if this is sufficiently EHCI-like will look like a "stopped" case.
> +static void reset_irq(struct mpc_udc *udc)
> +{
> + u32 temp;
> +
> + /* Clear the device address */
> + temp = le32_to_cpu(usb_slave_regs->deviceaddr);
> + temp &= ~USB_DEVICE_ADDRESS_MASK;
> + usb_slave_regs->deviceaddr = cpu_to_le32(temp);
> + udc->device_address = 0;
> +
> + /* Clear usb state */
> + udc->usb_state = USB_STATE_DEFAULT;
Also clear all the device status bits: disable remote wakeup
and HNP, etc.
> + ...
> +}
> + /* Sleep Enable (Suspend) */
> + if (irq_src & USB_STS_SUSPEND) {
> + suspend_irq(udc);
> + status = IRQ_HANDLED;
> + } else if (udc->resume_state) {
> + resume_irq(udc);
> + }
That looks odd ... surely there's a more affirmative indication
that the bus is getting resume signaling?
> +int usb_gadget_register_driver(struct usb_gadget_driver *driver)
> +{
> + int retval = -ENODEV;
> + unsigned long flags = 0;
> +
> + /* standard operations */
> + if (!udc_controller)
> + return -ENODEV;
> +
> + if (!driver || (driver->speed != USB_SPEED_FULL
> + && driver->speed != USB_SPEED_HIGH)
> + || !driver->bind || !driver->unbind ||
New rule: unbind() not required. (No point in it, for statically
linked drivers...)
> + !driver->disconnect || !driver->setup)
> + return -EINVAL;
> +
> + if (udc_controller->driver)
> + return -EBUSY;
> + /* lock is needed but whether should use this lock or another */
> + spin_lock_irqsave(&udc_controller->lock, flags);
> +
> + driver->driver.bus = 0;
> + /* hook up the driver */
> + udc_controller->driver = driver;
> + udc_controller->gadget.dev.driver = &driver->driver;
> + spin_unlock_irqrestore(&udc_controller->lock, flags);
> +
> + retval = driver->bind(&udc_controller->gadget);
> + if (retval) {
> + VDBG("bind to %s --> %d", driver->driver.name,
> + retval);
> + udc_controller->gadget.dev.driver = 0;
> + udc_controller->driver = 0;
> + goto out;
> + }
> +
> +#if defined(CONFIG_FSL_USB_OTG) || defined(CONFIG_FSL_USB_OTG_MODULE)
How far have you gotten with OTG functionality in this controller?
Does the cable based role sensing behave reliably, so that devel boards
can switch naturally between host and peripheral roles?
> +int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
> +{
> + struct mpc_ep *loop_ep;
> + unsigned long flags;
> +
> + if (!udc_controller)
> + return -ENODEV;
> +
> + if (!driver || driver != udc_controller->driver)
> + return -EINVAL;
And new rule, if driver->unbind is missing, fail cleanly.
> +#if defined(CONFIG_FSL_USB_OTG) || defined(CONFIG_FSL_USB_OTG_MODULE)
> + /* Memory and interrupt resources will be passed from OTG */
> + udc_controller->transceiver = otg_get_transceiver();
> + if (!udc_controller->transceiver) {
> + printk("Can't find OTG driver!\n");
> + return -ENODEV;
> + }
> +
> + dr_resources = otg_get_resources(&dr_num_resources);
I guess I missed a patch adding these otg calls ... presumably
specific to this controller?
> +static int mpc_udc_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> + dr_controller_stop(udc_controller);
And does this let the USB host be a wakeup event source for this
system? It should do so...
> +/* USB STS Register Bit Masks */
> +#define USB_STS_INT (0x00000001)
> +#define USB_STS_ERR (0x00000002)
> +#define USB_STS_PORT_CHANGE (0x00000004)
> +#define USB_STS_FRM_LST_ROLL (0x00000008)
> +#define USB_STS_SYS_ERR (0x00000010)
> +#define USB_STS_IAA (0x00000020)
> +#define USB_STS_RESET (0x00000040)
> +#define USB_STS_SOF (0x00000080)
> +#define USB_STS_SUSPEND (0x00000100)
> +#define USB_STS_HC_HALTED (0x00001000)
> +#define USB_STS_RCL (0x00002000)
> +#define USB_STS_PERIODIC_SCHEDULE (0x00004000)
> +#define USB_STS_ASYNC_SCHEDULE (0x00008000)
Um, please don't parenthesize such constants ...
> +/* Endpoint Queue Head data struct
> + * Rem: all the variables of qh are LittleEndian Mode
> + * and NEXT_POINTER_MASK should operate on a LittleEndian, Phy Addr
> + */
> +struct ep_queue_head {
I expect to see a one-to-one relationship between these and mpc_ep,
but there's no direct coupling between them. Why is that?
> + u32 max_pkt_length; /* Mult(31-30) , Zlt(29) , Max Pkt len
> + and IOS(15) */
> + u32 curr_dtd_ptr; /* Current dTD Pointer(31-5) */
> + u32 next_dtd_ptr; /* Next dTD Pointer(31-5), T(0) */
> + u32 size_ioc_int_sts; /* Total bytes (30-16), IOC (15),
> + MultO(11-10), STS (7-0) */
> + u32 buff_ptr0; /* Buffer pointer Page 0 (31-12) */
> + u32 buff_ptr1; /* Buffer pointer Page 1 (31-12) */
> + u32 buff_ptr2; /* Buffer pointer Page 2 (31-12) */
> + u32 buff_ptr3; /* Buffer pointer Page 3 (31-12) */
> + u32 buff_ptr4; /* Buffer pointer Page 4 (31-12) */
> + u32 res1;
> + u8 setup_buffer[8]; /* Setup data 8 bytes */
> + u32 res2[4];
> +};
> +
> +/* Endpoint Queue Head Bit Masks */
> +#define EP_QUEUE_HEAD_MULT_POS (30)
> +#define EP_QUEUE_HEAD_ZLT_SEL (0x20000000)
> +#define EP_QUEUE_HEAD_MAX_PKT_LEN_POS (16)
> +#define EP_QUEUE_HEAD_MAX_PKT_LEN(ep_info) (((ep_info)>>16)&0x07ff)
> +#define EP_QUEUE_HEAD_IOS (0x00008000)
> +#define EP_QUEUE_HEAD_NEXT_TERMINATE (0x00000001)
> +#define EP_QUEUE_HEAD_IOC (0x00008000)
> +#define EP_QUEUE_HEAD_MULTO (0x00000C00)
> +#define EP_QUEUE_HEAD_STATUS_HALT (0x00000040)
> +#define EP_QUEUE_HEAD_STATUS_ACTIVE (0x00000080)
> +#define EP_QUEUE_CURRENT_OFFSET_MASK (0x00000FFF)
> +#define EP_QUEUE_HEAD_NEXT_POINTER_MASK (0xFFFFFFE0)
> +#define EP_QUEUE_FRINDEX_MASK (0x000007FF)
> +#define EP_MAX_LENGTH_TRANSFER (0x4000)
> +
> +/* Endpoint Transfer Descriptor data struct */
> +/* Rem: all the variables of td are LittleEndian Mode */
> +struct ep_td_struct {
I expect to see one mpc_req correspond to one or more of these,
which does look to be there. Of course, the coupling is looser
than I'd like ... you allocate requests without TDs, so that it's
very possible to be able to queue a request and then get a memory
allocation failure. In general, the model is that queuing a
request should NOT imply memory allocation ... you should try to
keep allocations out of such fast paths.
> + u32 next_td_ptr; /* Next TD pointer(31-5), T(0) set
> + indicate invalid */
> + u32 size_ioc_sts; /* Total bytes (30-16), IOC (15),
> + MultO(11-10), STS (7-0) */
> + u32 buff_ptr0; /* Buffer pointer Page 0 */
> + u32 buff_ptr1; /* Buffer pointer Page 1 */
> + u32 buff_ptr2; /* Buffer pointer Page 2 */
> + u32 buff_ptr3; /* Buffer pointer Page 3 */
> + u32 buff_ptr4; /* Buffer pointer Page 4 */
> +};
> +
> +/* Endpoint Transfer Descriptor bit Masks */
> +#define DTD_NEXT_TERMINATE (0x00000001)
> +#define DTD_IOC (0x00008000)
> +#define DTD_STATUS_ACTIVE (0x00000080)
> +#define DTD_STATUS_HALTED (0x00000040)
> +#define DTD_STATUS_DATA_BUFF_ERR (0x00000020)
> +#define DTD_STATUS_TRANSACTION_ERR (0x00000008)
> +#define DTD_RESERVED_FIELDS (0x80007300)
> +#define DTD_ADDR_MASK (0xFFFFFFE0)
> +#define DTD_PACKET_SIZE (0x7FFF0000)
> +#define DTD_LENGTH_BIT_POS (16)
> +#define DTD_ERROR_MASK (DTD_STATUS_HALTED | \
> + DTD_STATUS_DATA_BUFF_ERR | \
> + DTD_STATUS_TRANSACTION_ERR)
> +
> +static inline void *KMALLOC_ALIGN(size_t size, int flags, unsigned int align,
> + void **base)
> +{
> + *base = kzalloc(size + align, flags);
> + if (*base == NULL)
> + return NULL;
> + return (void *) ALIGN((unsigned int) (*base), align);
> +}
Get rid of this macro ... you can't kfree() the resulting memory!!
> +/* Bulk only class request */
> +#define USB_BULK_RESET_REQUEST 0xff
... class driver constants don't belong here.
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
