> From: Felipe Balbi [mailto:[email protected]]
> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
