RE: [PATCH v4 2/5] HCD files for the DWC2 driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux