From: "Du, Changbin" <changbin.du@intel.com>
To: Felipe Balbi <balbi@kernel.org>
CC: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue
 available space
Thread-Topic: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue
 available space
Thread-Index: AQHRlK13d4fDYt/zWE+iRtI9QMtm1J+ImJAAgAC0uAA=
Date: Thu, 14 Apr 2016 11:15:51 +0000
Message-ID: <0C18FE92A7765D4EB9EE5D38D86A563A05D09504@shsmsx102.ccr.corp.intel.com>
References: <1460459419-9487-1-git-send-email-changbin.du@intel.com>
 <1460459419-9487-3-git-send-email-changbin.du@intel.com>
 <87shyok47z.fsf@intel.com>
In-Reply-To: <87shyok47z.fsf@intel.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 12260
Lines: 472

Hi, Balbi.
Feel free to change it, I may not have enough time on this currently.
"per-endpoint directory" is great idea, then we do not need find out
wanted info from one big file, but just go to specific dir. 

Btw, I'd mention that not all out ep has a rx fifo. So in my original patch,
not all FIFO/Queue info are valid. We need pick out the real info we need.
And I didn't find any method to read the FIFO map.

At last, comparing with the FIFO/Queue info, I think software transfer
Requests list, TRBs info, EVENTs history are much more useful for debugging
the driver. If you can also add these info to each EP folder, that is awesome!
:)

Best Regards,
Du, Changbin

> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@kernel.org]
> Sent: Thursday, April 14, 2016 4:03 PM
> To: Du, Changbin <changbin.du@intel.com>
> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; Du, Changbin <changbin.du@intel.com>
> Subject: Re: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump
> FIFO/Queue available space
> 
> 
> Hi,
> 
> changbin.du@intel.com writes:
> > From: "Du, Changbin" <changbin.du@intel.com>
> >
> > For DWC3 USB controller, the Global Debug Queue/FIFO Space Available
> > Register(GDBGFIFOSPACE) can be used to dump FIFO/Queue available
> space.
> > This can be used to check some special issues, like whether data is
> > successfully copied from memory to fifo when a trb is blocked.
> >
> > Signed-off-by: Du, Changbin <changbin.du@intel.com>
> > ---
> > v4:
> >   Do not fail silently, but print error.
> >
> > ---
> >  drivers/usb/dwc3/core.h    |  5 +++++
> >  drivers/usb/dwc3/debugfs.c | 44
> ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 49 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > index 6254b2f..899cf76 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -348,6 +348,11 @@
> >  #define DWC3_DSTS_LOWSPEED		(2 << 0)
> >  #define DWC3_DSTS_FULLSPEED1		(3 << 0)
> >
> > +/* Global Debug Queue/FIFO Space Available Register */
> > +#define DWC3_GDBGFIFOSPACE_NUM(x)	(((x) << 0) & 0x1F)
> > +#define DWC3_GDBGFIFOSPACE_TYPE(x)	(((x) << 5) & 0xE0)
> > +#define DWC3_GDBGFIFOSPACE_GET_SPACE(x)	(((x) >> 16) & 0xFFFF)
> 
> we always use lower case hex numbers. Also, databook refers to top 16
> bits as "Space Avaiable" so I'd prefer that you called this macro:
> 
> 	DWC3_GDBGFIFOSPACE_SPACE_AVAILABLE(x)
> 
> as that will aid with grepping
> 
> > diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> > index 615d4dc..83e5bc1 100644
> > --- a/drivers/usb/dwc3/debugfs.c
> > +++ b/drivers/usb/dwc3/debugfs.c
> > @@ -426,6 +426,45 @@ static const struct file_operations
> dwc3_mode_fops = {
> >  	.release		= single_release,
> >  };
> >
> > +static int dwc3_fifo_show(struct seq_file *s, void *unused)
> 
> you call this file 'fifo' however you print more than just the
> fifos. You printk the request queues, info queue, descriptor fetch
> queue, event queue and protocol status queue.
> 
> It seems, to me, you're trying to do way too much in a single file.
> 
> > +{
> > +	struct dwc3		*dwc = s->private;
> > +	unsigned long		flags;
> > +	unsigned int		type, index;
> > +	const char		*name;
> > +	u32			reg;
> > +
> > +	static const char * const fifo_names[] = {
> > +		"TxFIFO", "RxFIFO", "TxReqQ", "RxReqQ", "RxInfoQ",
> > +		"DescFetchQ", "EventQ", "ProtocolStatusQ"};
> > +	spin_lock_irqsave(&dwc->lock, flags);
> > +	for (type = 0; type < 8; type++) {
> 
> type < ARRAY_SIZE(fifo_names) ??
> 
> > +		name = fifo_names[type];
> > +		for (index = 0; index < 32; index++) {
> 
> not *all* dwc3 implementations enable all 32 endpoints, that's why we
> have dwc->num_endpoints
> 
> > +			dwc3_writel(dwc->regs, DWC3_GDBGFIFOSPACE,
> > +				DWC3_GDBGFIFOSPACE_NUM(index) |
> > +				DWC3_GDBGFIFOSPACE_TYPE(type));
> > +			reg = dwc3_readl(dwc->regs,
> DWC3_GDBGFIFOSPACE);
> 
> this writel() followed by a readl() could be a nice little helper
> function in core.c. Remember that we need the FIFO Space and link state
> to make sure we're allowed to start a transfer ;-)
> 
> I'd suggesting adding the following to core.c:
> 
> int dwc3_core_fifo_space(struct dwc3_ep *dep, unsigned int type)
> {
> 	u32 reg;
> 
>         dwc3_writel(dwc->regs, DWC3_GDBGFIFOSPACE,
>         	DWC3_GDBGFIFOSPACE_NUM(dep->number) |
>                 DWC3_GDBGFIFOSPACE_TYPE(type));
> 
> 	reg = dwc3_readl(dwc->regs, DWC3_GDBGFIFOSPACE);
> 
> 	return DWC3_GDBGFIFOSPACE_SPACE_AVAILABLE(reg);
> }
> 
> then, add the prototype to core.h. (both of these in a separate patch).
> 
> Then $subject just uses that helper to print out the data you want to
> print. I still think we might be better off with one file per type (iow
> TX fifo file, RX fifo file, TX request queue file, and so on).
> 
> In fact, we could actually have a per-endpoint directory where all of
> these details are printed out in a much more structured form. If you
> want, I can work on that, no problem, but you're also welcome to
> implement it yourself and I'd be around to help as needed.
> 
> In summary, we should have our debugfs looking like so:
> 
> ./ep0
> ./ep0/descriptor_fetch_queue
> ./ep0/event_queue
> ./ep0/info_queue
> ./ep0/protocol_status_queue
> ./ep0/request_queue
> ./ep0/rx_fifo
> ./ep0/rx_request_queue
> ./ep0/tx_fifo
> ./ep0/tx_request_queue
> ./ep1
> ./ep10
> ./ep10/descriptor_fetch_queue
> ./ep10/event_queue
> ./ep10/info_queue
> ./ep10/protocol_status_queue
> ./ep10/request_queue
> ./ep10/rx_fifo
> ./ep10/rx_request_queue
> ./ep10/tx_fifo
> ./ep10/tx_request_queue
> ./ep11
> ./ep11/descriptor_fetch_queue
> ./ep11/event_queue
> ./ep11/info_queue
> ./ep11/protocol_status_queue
> ./ep11/request_queue
> ./ep11/rx_fifo
> ./ep11/rx_request_queue
> ./ep11/tx_fifo
> ./ep11/tx_request_queue
> ./ep12
> ./ep12/descriptor_fetch_queue
> ./ep12/event_queue
> ./ep12/info_queue
> ./ep12/protocol_status_queue
> ./ep12/request_queue
> ./ep12/rx_fifo
> ./ep12/rx_request_queue
> ./ep12/tx_fifo
> ./ep12/tx_request_queue
> ./ep13
> ./ep13/descriptor_fetch_queue
> ./ep13/event_queue
> ./ep13/info_queue
> ./ep13/protocol_status_queue
> ./ep13/request_queue
> ./ep13/rx_fifo
> ./ep13/rx_request_queue
> ./ep13/tx_fifo
> ./ep13/tx_request_queue
> ./ep14
> ./ep14/descriptor_fetch_queue
> ./ep14/event_queue
> ./ep14/info_queue
> ./ep14/protocol_status_queue
> ./ep14/request_queue
> ./ep14/rx_fifo
> ./ep14/rx_request_queue
> ./ep14/tx_fifo
> ./ep14/tx_request_queue
> ./ep15
> ./ep15/descriptor_fetch_queue
> ./ep15/event_queue
> ./ep15/info_queue
> ./ep15/protocol_status_queue
> ./ep15/request_queue
> ./ep15/rx_fifo
> ./ep15/rx_request_queue
> ./ep15/tx_fifo
> ./ep15/tx_request_queue
> ./ep16
> ./ep16/descriptor_fetch_queue
> ./ep16/event_queue
> ./ep16/info_queue
> ./ep16/protocol_status_queue
> ./ep16/request_queue
> ./ep16/rx_fifo
> ./ep16/rx_request_queue
> ./ep16/tx_fifo
> ./ep16/tx_request_queue
> ./ep17
> ./ep17/descriptor_fetch_queue
> ./ep17/event_queue
> ./ep17/info_queue
> ./ep17/protocol_status_queue
> ./ep17/request_queue
> ./ep17/rx_fifo
> ./ep17/rx_request_queue
> ./ep17/tx_fifo
> ./ep17/tx_request_queue
> ./ep18
> ./ep18/descriptor_fetch_queue
> ./ep18/event_queue
> ./ep18/info_queue
> ./ep18/protocol_status_queue
> ./ep18/request_queue
> ./ep18/rx_fifo
> ./ep18/rx_request_queue
> ./ep18/tx_fifo
> ./ep18/tx_request_queue
> ./ep19
> ./ep19/descriptor_fetch_queue
> ./ep19/event_queue
> ./ep19/info_queue
> ./ep19/protocol_status_queue
> ./ep19/request_queue
> ./ep19/rx_fifo
> ./ep19/rx_request_queue
> ./ep19/tx_fifo
> ./ep19/tx_request_queue
> ./ep1/descriptor_fetch_queue
> ./ep1/event_queue
> ./ep1/info_queue
> ./ep1/protocol_status_queue
> ./ep1/request_queue
> ./ep1/rx_fifo
> ./ep1/rx_request_queue
> ./ep1/tx_fifo
> ./ep1/tx_request_queue
> ./ep2
> ./ep20
> ./ep20/descriptor_fetch_queue
> ./ep20/event_queue
> ./ep20/info_queue
> ./ep20/protocol_status_queue
> ./ep20/request_queue
> ./ep20/rx_fifo
> ./ep20/rx_request_queue
> ./ep20/tx_fifo
> ./ep20/tx_request_queue
> ./ep21
> ./ep21/descriptor_fetch_queue
> ./ep21/event_queue
> ./ep21/info_queue
> ./ep21/protocol_status_queue
> ./ep21/request_queue
> ./ep21/rx_fifo
> ./ep21/rx_request_queue
> ./ep21/tx_fifo
> ./ep21/tx_request_queue
> ./ep22
> ./ep22/descriptor_fetch_queue
> ./ep22/event_queue
> ./ep22/info_queue
> ./ep22/protocol_status_queue
> ./ep22/request_queue
> ./ep22/rx_fifo
> ./ep22/rx_request_queue
> ./ep22/tx_fifo
> ./ep22/tx_request_queue
> ./ep23
> ./ep23/descriptor_fetch_queue
> ./ep23/event_queue
> ./ep23/info_queue
> ./ep23/protocol_status_queue
> ./ep23/request_queue
> ./ep23/rx_fifo
> ./ep23/rx_request_queue
> ./ep23/tx_fifo
> ./ep23/tx_request_queue
> ./ep24
> ./ep24/descriptor_fetch_queue
> ./ep24/event_queue
> ./ep24/info_queue
> ./ep24/protocol_status_queue
> ./ep24/request_queue
> ./ep24/rx_fifo
> ./ep24/rx_request_queue
> ./ep24/tx_fifo
> ./ep24/tx_request_queue
> ./ep25
> ./ep25/descriptor_fetch_queue
> ./ep25/event_queue
> ./ep25/info_queue
> ./ep25/protocol_status_queue
> ./ep25/request_queue
> ./ep25/rx_fifo
> ./ep25/rx_request_queue
> ./ep25/tx_fifo
> ./ep25/tx_request_queue
> ./ep26
> ./ep26/descriptor_fetch_queue
> ./ep26/event_queue
> ./ep26/info_queue
> ./ep26/protocol_status_queue
> ./ep26/request_queue
> ./ep26/rx_fifo
> ./ep26/rx_request_queue
> ./ep26/tx_fifo
> ./ep26/tx_request_queue
> ./ep27
> ./ep27/descriptor_fetch_queue
> ./ep27/event_queue
> ./ep27/info_queue
> ./ep27/protocol_status_queue
> ./ep27/request_queue
> ./ep27/rx_fifo
> ./ep27/rx_request_queue
> ./ep27/tx_fifo
> ./ep27/tx_request_queue
> ./ep28
> ./ep28/descriptor_fetch_queue
> ./ep28/event_queue
> ./ep28/info_queue
> ./ep28/protocol_status_queue
> ./ep28/request_queue
> ./ep28/rx_fifo
> ./ep28/rx_request_queue
> ./ep28/tx_fifo
> ./ep28/tx_request_queue
> ./ep29
> ./ep29/descriptor_fetch_queue
> ./ep29/event_queue
> ./ep29/info_queue
> ./ep29/protocol_status_queue
> ./ep29/request_queue
> ./ep29/rx_fifo
> ./ep29/rx_request_queue
> ./ep29/tx_fifo
> ./ep29/tx_request_queue
> ./ep2/descriptor_fetch_queue
> ./ep2/event_queue
> ./ep2/info_queue
> ./ep2/protocol_status_queue
> ./ep2/request_queue
> ./ep2/rx_fifo
> ./ep2/rx_request_queue
> ./ep2/tx_fifo
> ./ep2/tx_request_queue
> ./ep3
> ./ep30
> ./ep30/descriptor_fetch_queue
> ./ep30/event_queue
> ./ep30/info_queue
> ./ep30/protocol_status_queue
> ./ep30/request_queue
> ./ep30/rx_fifo
> ./ep30/rx_request_queue
> ./ep30/tx_fifo
> ./ep30/tx_request_queue
> ./ep31
> ./ep31/descriptor_fetch_queue
> ./ep31/event_queue
> ./ep31/info_queue
> ./ep31/protocol_status_queue
> ./ep31/request_queue
> ./ep31/rx_fifo
> ./ep31/rx_request_queue
> ./ep31/tx_fifo
> ./ep31/tx_request_queue
> ./ep3/descriptor_fetch_queue
> ./ep3/event_queue
> ./ep3/info_queue
> ./ep3/protocol_status_queue
> ./ep3/request_queue
> ./ep3/rx_fifo
> ./ep3/rx_request_queue
> ./ep3/tx_fifo
> ./ep3/tx_request_queue
> ./ep4
> ./ep4/descriptor_fetch_queue
> ./ep4/event_queue
> ./ep4/info_queue
> ./ep4/protocol_status_queue
> ./ep4/request_queue
> ./ep4/rx_fifo
> ./ep4/rx_request_queue
> ./ep4/tx_fifo
> ./ep4/tx_request_queue
> ./ep5
> ./ep5/descriptor_fetch_queue
> ./ep5/event_queue
> ./ep5/info_queue
> ./ep5/protocol_status_queue
> ./ep5/request_queue
> ./ep5/rx_fifo
> ./ep5/rx_request_queue
> ./ep5/tx_fifo
> ./ep5/tx_request_queue
> ./ep6
> ./ep6/descriptor_fetch_queue
> ./ep6/event_queue
> ./ep6/info_queue
> ./ep6/protocol_status_queue
> ./ep6/request_queue
> ./ep6/rx_fifo
> ./ep6/rx_request_queue
> ./ep6/tx_fifo
> ./ep6/tx_request_queue
> ./ep7
> ./ep7/descriptor_fetch_queue
> ./ep7/event_queue
> ./ep7/info_queue
> ./ep7/protocol_status_queue
> ./ep7/request_queue
> ./ep7/rx_fifo
> ./ep7/rx_request_queue
> ./ep7/tx_fifo
> ./ep7/tx_request_queue
> ./ep8
> ./ep8/descriptor_fetch_queue
> ./ep8/event_queue
> ./ep8/info_queue
> ./ep8/protocol_status_queue
> ./ep8/request_queue
> ./ep8/rx_fifo
> ./ep8/rx_request_queue
> ./ep8/tx_fifo
> ./ep8/tx_request_queue
> ./ep9
> ./ep9/descriptor_fetch_queue
> ./ep9/event_queue
> ./ep9/info_queue
> ./ep9/protocol_status_queue
> ./ep9/request_queue
> ./ep9/rx_fifo
> ./ep9/rx_request_queue
> ./ep9/tx_fifo
> ./ep9/tx_request_queue
> ./link_state
> ./mode
> ./regdump
> ./testmode
> 
> --
> balbi