HI,
On Tue, Feb 19, 2013 at 06:50:06PM -0800, Paul Zimmerman wrote:
> +#include <linux/kernel.h>
> +#include <linux/module.h>
apparently not used.
> +#include <linux/moduleparam.h>
not used.
> +#include <linux/spinlock.h>
not used
> +#include <linux/interrupt.h>
not used either
> +#include <linux/dma-mapping.h>
> +#include <linux/debugfs.h>
not used
> +#include <linux/seq_file.h>
not used
> +#include <linux/delay.h>
not used.
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +
> +#include <linux/usb/hcd.h>
> +#include <linux/usb/ch11.h>
this doesn't exist anymore (moved to uapi/linux/usb/ch11.h) have you
build tested ?
In any case, I think it makes sense to have a small <linux/usb/ch11.h>
header simply including uapi/linux/usb/ch11.h. Greg ?
> +#include <linux/usb/gadget.h>
not used
> +static void dwc2_per_sched_enable(struct dwc2_hsotg *hsotg, u16 fr_list_en)
> +{
> + u32 hcfg = readl(hsotg->regs + HCFG);
> + u32 frlisten;
> +
> + if (hcfg & HCFG_PERSCHEDENA)
> + /* already enabled */
> + return;
> +
> + writel(hsotg->frame_list_dma, hsotg->regs + HFLBADDR);
> +
> + switch (fr_list_en) {
> + case 64:
> + frlisten = 3;
> + break;
> + case 32:
> + frlisten = 2;
> + break;
> + case 16:
> + frlisten = 1;
> + break;
> + case 8:
what are these cases ? Frame list size in bits ? Care to provide macros?
Or perhaps something like below would be faster and have the same
outcome:
frlisten = __ffs(fr_list_en >> 3);
Just make sure to add a comment stating what the code is doing ;-)
> +int dwc2_hcd_qh_init_ddma(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
> + gfp_t mem_flags)
> +{
> + int retval = 0;
> +
> + if (qh->do_split) {
> + dev_err(hsotg->dev,
> + "SPLIT Transfers are not supported in Descriptor DMA.\n");
> + return -1;
> + }
> +
> + retval = dwc2_desc_list_alloc(hsotg, qh, mem_flags);
> +
> + if (retval == 0 && (qh->ep_type == USB_ENDPOINT_XFER_ISOC ||
I think it's best to make the retval check explict:
if (retval)
goto out;
if (qh->ep_type == USB_ENDPOINT_XFER_ISOC ||
qh->ep_type == USB_ENDPOINT_XFER_INT) {
if (hsotg->frame_list)
goto out;
retval = dwc2_frame_list_alloc(hsotg, mem_flags);
if (retval)
goto out;
dwc2_per_sched_enable(hsotg, MAX_FRLIST_EN_NUM);
out:
> +void dwc2_hcd_qh_free_ddma(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
> +{
> + dwc2_desc_list_free(hsotg, qh);
> +
> + /*
> + * Channel still assigned due to some reasons.
> + * Seen on Isoc URB dequeue. Channel halted but no subsequent
> + * ChHalted interrupt to release the channel. Afterwards
> + * when it comes here from endpoint disable routine
> + * channel remains assigned.
> + */
> + if (qh->channel)
> + dwc2_release_channel_ddma(hsotg, qh);
would be cool to debug this properly at some point. Could be pointing to
a race condition somewhere.
> +static void dwc2_fill_host_dma_desc(struct dwc2_hsotg *hsotg,
> + struct dwc2_host_chan *chan,
> + struct dwc2_qtd *qtd, struct dwc2_qh *qh,
> + int n_desc)
> +{
> + struct dwc2_hcd_dma_desc *dma_desc = &qh->desc_list[n_desc];
> + int len = chan->xfer_len;
> +
> + if (len > MAX_DMA_DESC_SIZE)
> + len = MAX_DMA_DESC_SIZE - chan->max_packet + 1;
> +
> + if (chan->ep_is_in) {
> + int num_packets;
> +
> + if (len > 0 && chan->max_packet)
> + num_packets = (len + chan->max_packet - 1)
> + / chan->max_packet;
> + else
> + /* Need 1 packet for transfer length of 0 */
> + num_packets = 1;
> +
> + /* Always program an integral # of packets for IN transfers */
> + len = num_packets * chan->max_packet;
> + }
> +
> + dma_desc->status = len << HOST_DMA_NBYTES_SHIFT & HOST_DMA_NBYTES_MASK;
> + qh->n_bytes[n_desc] = len;
> +
> + if (qh->ep_type == USB_ENDPOINT_XFER_CONTROL &&
> + qtd->control_phase == DWC2_CONTROL_SETUP)
> + dma_desc->status |= HOST_DMA_SUP;
> +
> + dma_desc->buf = (unsigned long)chan->xfer_buff & 0xffffffff;
is this really enough ? I wonder if you shouldn't be mapping the buffer
to dma with dma_map_single() ?
> + list_for_each(qtd_item, &qh->qtd_list) {
> + qtd = list_entry(qtd_item, struct dwc2_qtd, qtd_list_entry);
these two can be combined into list_for_each_entry()
> + if (n_desc) {
> + /* SG request - more than 1 QTD */
> + chan->xfer_buff = (u8 *)qtd->urb->dma +
> + qtd->urb->actual_length;
hmm... this casting looks weird. Seems like you need some re-factoring
here. You shouldn't be casting dma addresses to u8 pointers and casting
it back to unsigned long later...
Just fix your data structures to use proper data types.
> +static void dwc2_complete_isoc_xfer_ddma(struct dwc2_hsotg *hsotg,
> + struct dwc2_host_chan *chan,
> + enum dwc2_halt_status halt_status)
> +{
> + struct dwc2_hcd_iso_packet_desc *frame_desc;
> + struct list_head *qtd_item, *qtd_tmp;
> + struct dwc2_qtd *qtd;
> + struct dwc2_qh *qh;
> + u16 idx;
> + int rc;
> +
> + qh = chan->qh;
> + idx = qh->td_first;
> +
> + if (chan->halt_status == DWC2_HC_XFER_URB_DEQUEUE) {
> + list_for_each_safe(qtd_item, qtd_tmp, &qh->qtd_list) {
> + qtd = list_entry(qtd_item, struct dwc2_qtd,
> + qtd_list_entry);
list_for_each_entry_safe()
> + qtd->in_process = 0;
> + }
> + return;
> + } else if (halt_status == DWC2_HC_XFER_AHB_ERR ||
else is unnecessary here.
> + halt_status == DWC2_HC_XFER_BABBLE_ERR) {
> + /*
> + * Channel is halted in these error cases, considered as serious
> + * issues.
> + * Complete all URBs marking all frames as failed, irrespective
> + * whether some of the descriptors (frames) succeeded or not.
> + * Pass error code to completion routine as well, to update
> + * urb->status, some of class drivers might use it to stop
> + * queing transfer requests.
> + */
> + int err = halt_status == DWC2_HC_XFER_AHB_ERR ?
> + -EIO : -EOVERFLOW;
> +
> + list_for_each_safe(qtd_item, qtd_tmp, &qh->qtd_list) {
> + qtd = list_entry(qtd_item, struct dwc2_qtd,
> + qtd_list_entry);
list_for_each_entry_safe().
> + for (idx = 0; idx < qtd->urb->packet_count; idx++) {
> + frame_desc = &qtd->urb->iso_descs[idx];
> + frame_desc->status = err;
> + }
> + dwc2_host_complete(hsotg, qtd->urb->priv, qtd->urb,
> + err);
> + dwc2_hcd_qtd_remove_and_free(hsotg, qtd, qh);
> + }
> + return;
> + }
> +
> + list_for_each_safe(qtd_item, qtd_tmp, &qh->qtd_list) {
> + qtd = list_entry(qtd_item, struct dwc2_qtd, qtd_list_entry);
list_for_each_entry_safe().
> +static int dwc2_process_non_isoc_desc(struct dwc2_hsotg *hsotg,
> + struct dwc2_host_chan *chan,
> + int chnum, struct dwc2_qtd *qtd,
> + int desc_num,
> + enum dwc2_halt_status halt_status,
> + int *xfer_done)
> +{
> + struct dwc2_qh *qh = chan->qh;
> + struct dwc2_hcd_urb *urb = qtd->urb;
> + struct dwc2_hcd_dma_desc *dma_desc;
> + u32 n_bytes;
> + int failed;
> +
> + dev_vdbg(hsotg->dev, "%s()\n", __func__);
> +
> + dma_desc = &qh->desc_list[desc_num];
> + n_bytes = qh->n_bytes[desc_num];
> + dev_vdbg(hsotg->dev,
> + "qtd=%p dwc2_urb=%p desc_num=%d desc=%p n_bytes=%d\n",
> + qtd, urb, desc_num, dma_desc, n_bytes);
> + failed = dwc2_update_non_isoc_urb_state_ddma(hsotg, chan, qtd, dma_desc,
> + halt_status, n_bytes,
> + xfer_done);
> + if (failed || (*xfer_done && urb->status != -EINPROGRESS)) {
> + dwc2_host_complete(hsotg, urb->priv, urb, urb->status);
> + dwc2_hcd_qtd_remove_and_free(hsotg, qtd, qh);
> + dev_vdbg(hsotg->dev, "failed=%1x xfer_done=%1x status=%08x\n",
> + failed, *xfer_done, urb->status);
> + return failed;
> +
> + } else if (qh->ep_type == USB_ENDPOINT_XFER_CONTROL) {
else is unnecessary here.
> + if (qtd->control_phase == DWC2_CONTROL_SETUP) {
> + if (urb->length > 0)
> + qtd->control_phase = DWC2_CONTROL_DATA;
> + else
> + qtd->control_phase = DWC2_CONTROL_STATUS;
> + dev_vdbg(hsotg->dev,
> + " Control setup transaction done\n");
> + } else if (qtd->control_phase == DWC2_CONTROL_DATA) {
this might look nicer with a switch.
> +static void dwc2_complete_non_isoc_xfer_ddma(struct dwc2_hsotg *hsotg,
> + struct dwc2_host_chan *chan,
> + int chnum,
> + enum dwc2_halt_status halt_status)
> +{
> + struct list_head *qtd_item, *qtd_tmp;
> + struct dwc2_qh *qh = chan->qh;
> + struct dwc2_qtd *qtd = NULL;
> + int xfer_done;
> + int desc_num = 0;
> +
> + if (chan->halt_status == DWC2_HC_XFER_URB_DEQUEUE) {
> + list_for_each_safe(qtd_item, qtd_tmp, &qh->qtd_list) {
> + qtd = list_entry(qtd_item, struct dwc2_qtd,
list_for_each_entry_safe()
> + qtd_list_entry);
> + qtd->in_process = 0;
> + }
> + return;
> + }
> +
> + list_for_each_safe(qtd_item, qtd_tmp, &qh->qtd_list) {
> + int i;
> +
> + qtd = list_entry(qtd_item, struct dwc2_qtd, qtd_list_entry);
list_for_each_entry_safe()
--
balbi
Attachment:
signature.asc
Description: Digital signature
