> From: Felipe Balbi [mailto:balbi@xxxxxx] > Sent: Thursday, February 21, 2013 1:16 AM > > On Tue, Feb 19, 2013 at 06:50:05PM -0800, Paul Zimmerman wrote: > > > +#ifdef DWC2_TRACK_MISSED_SOFS > > +#warning Compiling code to track missed SOFs > > +#define FRAME_NUM_ARRAY_SIZE 1000 > > + > > +/* This function is for debug only */ > > +static void dwc2_track_missed_sofs(struct dwc2_hsotg *hsotg) > > +{ > > + static u16 frame_num_array[FRAME_NUM_ARRAY_SIZE]; > > + static u16 last_frame_num_array[FRAME_NUM_ARRAY_SIZE]; > > you could turn this into a list_head and everytime you find a missed > sof, you allocate a e.g. struct dwc2_missec_isoc_data and add that to a > list of missed isocs. The only "problem" is that you would have to > allocate with GFP_ATOMIC. Actually, this function is called on every SOF, not just when one is missed, so that wouldn't make much difference. I guess I should rename this to dwc2_track_sofs(). I didn't notice the static allocations before, I will change that to use kmalloc() at init time. > > + static int frame_num_idx; > > + static u16 last_frame_num = HFNUM_MAX_FRNUM; > > + static int dumped_frame_num_array; > > + u16 curr_frame_number = hsotg->frame_number; > > + > > + if (frame_num_idx < FRAME_NUM_ARRAY_SIZE) { > > + if ((last_frame_num + 1 & HFNUM_MAX_FRNUM) != > > + curr_frame_number) { > > + frame_num_array[frame_num_idx] = curr_frame_number; > > + last_frame_num_array[frame_num_idx++] = last_frame_num; > > + } > > + } else if (!dumped_frame_num_array) { > > + int i; > > + > > + dev_info(hsotg->dev, "Frame Last Frame\n"); > > + dev_info(hsotg->dev, "----- ----------\n"); > > + for (i = 0; i < FRAME_NUM_ARRAY_SIZE; i++) { > > + dev_info(hsotg->dev, "0x%04x 0x%04x\n", > > + frame_num_array[i], last_frame_num_array[i]); > > + } > > + dumped_frame_num_array = 1; > > + } > > + last_frame_num = curr_frame_number; > > +} > > +#endif > > do you really need to dump this information at the time it happens ? I > ask this because this looks like a very nice feature to have in > production. You could just pack it under a debugfs interface and provide > a Kconfig choice to enable (or not) debugfs support. Then driver can > constantly track this information and let user see by "cat > /sys/kernel/debug/dwc2/missed_sofs" or something similar. Yes, that's a good suggestion. I will look into it. > > +static void dwc2_sof_intr(struct dwc2_hsotg *hsotg) > > +{ > > + struct list_head *qh_entry; > > + struct dwc2_qh *qh; > > + u32 hfnum; > > + enum dwc2_transaction_type tr_type; > > + > > +#ifdef DEBUG_SOF > > + dev_vdbg(hsotg->dev, "--Start of Frame Interrupt--\n"); > > +#endif > > + > > + hfnum = readl(hsotg->regs + HFNUM); > > + hsotg->frame_number = hfnum >> HFNUM_FRNUM_SHIFT & > > + HFNUM_FRNUM_MASK >> HFNUM_FRNUM_SHIFT; > > + > > +#ifdef DWC2_TRACK_MISSED_SOFS > > + dwc2_track_missed_sofs(hsotg); > > +#endif > > another suggestion, would be to move the ifdefs inside the function > body, so that it's a no-op when DWC2_TRACK_MISSED_SOFS isn't set, which > means you don't need to ifdef the call here. Yep, will do. > > + if (chan->hcint & HCINTMSK_XFERCOMPL) { > > + /* > > + * Todo: This is here because of a possible hardware bug. Spec > > + * says that on SPLIT-ISOC OUT transfers in DMA mode that a HALT > > + * interrupt w/ACK bit set should occur, but I only see the > > + * XFERCOMP bit, even with it masked out. This is a workaround > > + * for that behavior. Should fix this when hardware is fixed. > > + */ > > + if (chan->ep_type == USB_ENDPOINT_XFER_ISOC && !chan->ep_is_in) > > + dwc2_hc_ack_intr(hsotg, chan, chnum, qtd); > > + dwc2_hc_xfercomp_intr(hsotg, chan, chnum, qtd); > > + } else if (chan->hcint & HCINTMSK_STALL) { > > + dwc2_hc_stall_intr(hsotg, chan, chnum, qtd); > > + } else if ((chan->hcint & HCINTMSK_XACTERR) && > > + hsotg->core_params->dma_desc_enable <= 0) { > > + if (out_nak_enh) { > > + if (chan->hcint & > > + (HCINTMSK_NYET | HCINTMSK_NAK | HCINTMSK_ACK)) { > > + dev_vdbg(hsotg->dev, > > + "XactErr with NYET/NAK/ACK\n"); > > + qtd->error_count = 0; > > + } else { > > + dev_vdbg(hsotg->dev, > > + "XactErr without NYET/NAK/ACK\n"); > > + } > > + } > > + > > + /* > > + * Must handle xacterr before nak or ack. Could get a xacterr > > + * at the same time as either of these on a BULK/CONTROL OUT > > + * that started with a PING. The xacterr takes precedence. > > + */ > > + dwc2_hc_xacterr_intr(hsotg, chan, chnum, qtd); > > + } else if ((chan->hcint & HCINTMSK_XCS_XACT) && > > + hsotg->core_params->dma_desc_enable > 0) { > > are all these really mutually exclusive ? I mean, can you get XACTERR > and AHBERR simultaneously ? Yes, you can. I will fix this so they are not mutually exclusive. -- Paul -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
