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

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

 



Hi,

On Tue, Feb 19, 2013 at 06:50:05PM -0800, Paul Zimmerman wrote:
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/spinlock.h>
> +#include <linux/interrupt.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +
> +#include <linux/usb/hcd.h>
> +#include <linux/usb/ch11.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/ch9.h>

the usual set of unused headers ;-)

> +#include "core.h"
> +#include "hcd.h"
> +
> +#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.

> +	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.

> +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.

> +static void dwc2_hc_chhltd_intr_dma(struct dwc2_hsotg *hsotg,
> +				    struct dwc2_host_chan *chan, int chnum,
> +				    struct dwc2_qtd *qtd)
> +{
> +	u32 hcintmsk;
> +	int out_nak_enh = 0;
> +
> +	dev_vdbg(hsotg->dev,
> +		 "--Host Channel %d Interrupt: DMA Channel Halted--\n", chnum);
> +
> +	/*
> +	 * For core with OUT NAK enhancement, the flow for high-speed
> +	 * CONTROL/BULK OUT is handled a little differently
> +	 */
> +	if (hsotg->snpsid >= DWC2_CORE_REV_2_71a) {
> +		if (chan->speed == USB_SPEED_HIGH && !chan->ep_is_in &&
> +		    (chan->ep_type == USB_ENDPOINT_XFER_CONTROL ||
> +		     chan->ep_type == USB_ENDPOINT_XFER_BULK)) {
> +			out_nak_enh = 1;
> +		}
> +	}
> +
> +	if (chan->halt_status == DWC2_HC_XFER_URB_DEQUEUE ||
> +	    (chan->halt_status == DWC2_HC_XFER_AHB_ERR &&
> +	     hsotg->core_params->dma_desc_enable <= 0)) {
> +		if (hsotg->core_params->dma_desc_enable > 0)
> +			dwc2_hcd_complete_xfer_ddma(hsotg, chan, chnum,
> +						    chan->halt_status);
> +		else
> +			/*
> +			 * Just release the channel. A dequeue can happen on a
> +			 * transfer timeout. In the case of an AHB Error, the
> +			 * channel was forced to halt because there's no way to
> +			 * gracefully recover.
> +			 */
> +			dwc2_release_channel(hsotg, chan, qtd,
> +					     chan->halt_status);
> +		return;
> +	}
> +
> +	hcintmsk = readl(hsotg->regs + HCINTMSK(chnum));
> +
> +	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 ?

-- 
balbi

Attachment: signature.asc
Description: Digital signature


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

  Powered by Linux