RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Du, Changbin]

> Hi,
> 
> "Du, Changbin"  writes:
> >> > 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!
> >> > :)
> >>
> >> I'll think about adding these but for the lifetime of requests and trbs
> >> and events, etc, we have tracepoints for that. I usually do the
> >> following when debugging:
> >>
> >> # mount -t debugfs none /sys/kernel/debug
> >> # cd /sys/kernel/debug/tracing
> >> # echo 2048 > buffer_size_kb
> >> # echo 1 > events/dwc3/enable
> >>
> >> (do something to break it)
> >>
> >> # cp trace /mnt/sdcard # or something like that
> >>
> >> then read the file. You can make it as large or as small as you like
> >> (given some constraints, of course ;-) but I've had no issues allocating
> >> 128MiB in the past.
> >>
> >> --
> >> Balbi
> >
> > Thanks for the sharing, this is a good approach to capture dynamic
> > behaviors. But a dump of current state has below advantages:
> > 1. a quick view for the pending transfers. Then we can quickly
> >  checking the transfer status.
> > 2. no side-effect. This is important in some case. We usually
> > encounter some transfer issues but very hard to reproduce
> > it. But we cannot enable trace all the time since performance
> > concern. Then I thought it was so great if I can have a look for
> > the trb status. :)
> 
> yeah, okay. We can definitely add "current state" of almost anything,
> but if you need history, then debugfs is not the best interface and I'd
> point you to tracepoints ;-)
> 
> I'll think about how I can add TRB state, seems like we'd need to dump
> the entire endpoint ring, and that's 256 TRBs per endpoint :-p Then we
> also need to know endpoint's dequeue and enqueue pointer. Oh well, let
> me get this first setup of files out of the way, then we can add more
> later much more easily.
> 
> --
> Balbi

Okay, things need finish step by step. Thank you, Balbi. ( �b- �b)つロ

Best Regards,
Du, Changbin

RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Du, Changbin]

> Hi,
> 
> "Du, Changbin"  writes:
> >> > 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!
> >> > :)
> >>
> >> I'll think about adding these but for the lifetime of requests and trbs
> >> and events, etc, we have tracepoints for that. I usually do the
> >> following when debugging:
> >>
> >> # mount -t debugfs none /sys/kernel/debug
> >> # cd /sys/kernel/debug/tracing
> >> # echo 2048 > buffer_size_kb
> >> # echo 1 > events/dwc3/enable
> >>
> >> (do something to break it)
> >>
> >> # cp trace /mnt/sdcard # or something like that
> >>
> >> then read the file. You can make it as large or as small as you like
> >> (given some constraints, of course ;-) but I've had no issues allocating
> >> 128MiB in the past.
> >>
> >> --
> >> Balbi
> >
> > Thanks for the sharing, this is a good approach to capture dynamic
> > behaviors. But a dump of current state has below advantages:
> > 1. a quick view for the pending transfers. Then we can quickly
> >  checking the transfer status.
> > 2. no side-effect. This is important in some case. We usually
> > encounter some transfer issues but very hard to reproduce
> > it. But we cannot enable trace all the time since performance
> > concern. Then I thought it was so great if I can have a look for
> > the trb status. :)
> 
> yeah, okay. We can definitely add "current state" of almost anything,
> but if you need history, then debugfs is not the best interface and I'd
> point you to tracepoints ;-)
> 
> I'll think about how I can add TRB state, seems like we'd need to dump
> the entire endpoint ring, and that's 256 TRBs per endpoint :-p Then we
> also need to know endpoint's dequeue and enqueue pointer. Oh well, let
> me get this first setup of files out of the way, then we can add more
> later much more easily.
> 
> --
> Balbi

Okay, things need finish step by step. Thank you, Balbi. ( �b- �b)つロ

Best Regards,
Du, Changbin

RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Felipe Balbi]

Hi,

"Du, Changbin"  writes:
>> > 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!
>> > :)
>> 
>> I'll think about adding these but for the lifetime of requests and trbs
>> and events, etc, we have tracepoints for that. I usually do the
>> following when debugging:
>> 
>> # mount -t debugfs none /sys/kernel/debug
>> # cd /sys/kernel/debug/tracing
>> # echo 2048 > buffer_size_kb
>> # echo 1 > events/dwc3/enable
>> 
>> (do something to break it)
>> 
>> # cp trace /mnt/sdcard # or something like that
>> 
>> then read the file. You can make it as large or as small as you like
>> (given some constraints, of course ;-) but I've had no issues allocating
>> 128MiB in the past.
>> 
>> --
>> Balbi
>
> Thanks for the sharing, this is a good approach to capture dynamic
> behaviors. But a dump of current state has below advantages:
> 1. a quick view for the pending transfers. Then we can quickly 
>  checking the transfer status.
> 2. no side-effect. This is important in some case. We usually
> encounter some transfer issues but very hard to reproduce
> it. But we cannot enable trace all the time since performance
> concern. Then I thought it was so great if I can have a look for
> the trb status. :)

yeah, okay. We can definitely add "current state" of almost anything,
but if you need history, then debugfs is not the best interface and I'd
point you to tracepoints ;-)

I'll think about how I can add TRB state, seems like we'd need to dump
the entire endpoint ring, and that's 256 TRBs per endpoint :-p Then we
also need to know endpoint's dequeue and enqueue pointer. Oh well, let
me get this first setup of files out of the way, then we can add more
later much more easily.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Felipe Balbi]

Hi,

"Du, Changbin"  writes:
>> > 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!
>> > :)
>> 
>> I'll think about adding these but for the lifetime of requests and trbs
>> and events, etc, we have tracepoints for that. I usually do the
>> following when debugging:
>> 
>> # mount -t debugfs none /sys/kernel/debug
>> # cd /sys/kernel/debug/tracing
>> # echo 2048 > buffer_size_kb
>> # echo 1 > events/dwc3/enable
>> 
>> (do something to break it)
>> 
>> # cp trace /mnt/sdcard # or something like that
>> 
>> then read the file. You can make it as large or as small as you like
>> (given some constraints, of course ;-) but I've had no issues allocating
>> 128MiB in the past.
>> 
>> --
>> Balbi
>
> Thanks for the sharing, this is a good approach to capture dynamic
> behaviors. But a dump of current state has below advantages:
> 1. a quick view for the pending transfers. Then we can quickly 
>  checking the transfer status.
> 2. no side-effect. This is important in some case. We usually
> encounter some transfer issues but very hard to reproduce
> it. But we cannot enable trace all the time since performance
> concern. Then I thought it was so great if I can have a look for
> the trb status. :)

yeah, okay. We can definitely add "current state" of almost anything,
but if you need history, then debugfs is not the best interface and I'd
point you to tracepoints ;-)

I'll think about how I can add TRB state, seems like we'd need to dump
the entire endpoint ring, and that's 256 TRBs per endpoint :-p Then we
also need to know endpoint's dequeue and enqueue pointer. Oh well, let
me get this first setup of files out of the way, then we can add more
later much more easily.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Du, Changbin]

> > 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!
> > :)
> 
> I'll think about adding these but for the lifetime of requests and trbs
> and events, etc, we have tracepoints for that. I usually do the
> following when debugging:
> 
> # mount -t debugfs none /sys/kernel/debug
> # cd /sys/kernel/debug/tracing
> # echo 2048 > buffer_size_kb
> # echo 1 > events/dwc3/enable
> 
> (do something to break it)
> 
> # cp trace /mnt/sdcard # or something like that
> 
> then read the file. You can make it as large or as small as you like
> (given some constraints, of course ;-) but I've had no issues allocating
> 128MiB in the past.
> 
> --
> Balbi

Thanks for the sharing, this is a good approach to capture dynamic
behaviors. But a dump of current state has below advantages:
1. a quick view for the pending transfers. Then we can quickly 
 checking the transfer status.
2. no side-effect. This is important in some case. We usually
encounter some transfer issues but very hard to reproduce
it. But we cannot enable trace all the time since performance
concern. Then I thought it was so great if I can have a look for
the trb status. :)

Best Regards,
Du, Changbin

RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Du, Changbin]

> > 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!
> > :)
> 
> I'll think about adding these but for the lifetime of requests and trbs
> and events, etc, we have tracepoints for that. I usually do the
> following when debugging:
> 
> # mount -t debugfs none /sys/kernel/debug
> # cd /sys/kernel/debug/tracing
> # echo 2048 > buffer_size_kb
> # echo 1 > events/dwc3/enable
> 
> (do something to break it)
> 
> # cp trace /mnt/sdcard # or something like that
> 
> then read the file. You can make it as large or as small as you like
> (given some constraints, of course ;-) but I've had no issues allocating
> 128MiB in the past.
> 
> --
> Balbi

Thanks for the sharing, this is a good approach to capture dynamic
behaviors. But a dump of current state has below advantages:
1. a quick view for the pending transfers. Then we can quickly 
 checking the transfer status.
2. no side-effect. This is important in some case. We usually
encounter some transfer issues but very hard to reproduce
it. But we cannot enable trace all the time since performance
concern. Then I thought it was so great if I can have a look for
the trb status. :)

Best Regards,
Du, Changbin

RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Felipe Balbi]

Hi,

"Du, Changbin"  writes:
> 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. 

that was the idea, glad you liked it ;-)

> Btw, I'd mention that not all out ep has a rx fifo. So in my original patch,

yeah, rx fifos are dynamically allocated by the HW itself, AFAICT.

> 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!
> :)

I'll think about adding these but for the lifetime of requests and trbs
and events, etc, we have tracepoints for that. I usually do the
following when debugging:

# mount -t debugfs none /sys/kernel/debug
# cd /sys/kernel/debug/tracing
# echo 2048 > buffer_size_kb
# echo 1 > events/dwc3/enable

(do something to break it)

# cp trace /mnt/sdcard # or something like that

then read the file. You can make it as large or as small as you like
(given some constraints, of course ;-) but I've had no issues allocating
128MiB in the past.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Felipe Balbi]

Hi,

"Du, Changbin"  writes:
> 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. 

that was the idea, glad you liked it ;-)

> Btw, I'd mention that not all out ep has a rx fifo. So in my original patch,

yeah, rx fifos are dynamically allocated by the HW itself, AFAICT.

> 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!
> :)

I'll think about adding these but for the lifetime of requests and trbs
and events, etc, we have tracepoints for that. I usually do the
following when debugging:

# mount -t debugfs none /sys/kernel/debug
# cd /sys/kernel/debug/tracing
# echo 2048 > buffer_size_kb
# echo 1 > events/dwc3/enable

(do something to break it)

# cp trace /mnt/sdcard # or something like that

then read the file. You can make it as large or as small as you like
(given some constraints, of course ;-) but I've had no issues allocating
128MiB in the past.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Du, Changbin]

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:ba...@kernel.org]
> Sent: Thursday, April 14, 2016 4:03 PM
> To: Du, Changbin <changbin...@intel.com>
> Cc: gre...@linuxfoundation.org; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Du, Changbin <changbin...@intel.com>
> Subject: Re: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump
> FIFO/Queue available space
> 
> 
> Hi,
> 
> changbin...@intel.com writes:
> > From: "Du, Changbin" <changbin...@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...@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) & 0x)
> 
> 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 inttype, index;
> > +   const char  *name;
> > +   u32 reg;
> > +
> > +   static const char * const fifo_names[] = {
> > +   "TxFIFO", "RxFIFO", "TxReqQ", "RxReqQ", "RxInfoQ",
> > +   "DescFetchQ", "EventQ", "ProtocolStatusQ"};
> > +   spin_lock_irqsave(>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

RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Du, Changbin]

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:ba...@kernel.org]
> Sent: Thursday, April 14, 2016 4:03 PM
> To: Du, Changbin 
> Cc: gre...@linuxfoundation.org; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Du, Changbin 
> Subject: Re: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump
> FIFO/Queue available space
> 
> 
> Hi,
> 
> changbin...@intel.com writes:
> > From: "Du, Changbin" 
> >
> > 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 
> > ---
> > 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) & 0x)
> 
> 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 inttype, index;
> > +   const char  *name;
> > +   u32 reg;
> > +
> > +   static const char * const fifo_names[] = {
> > +   "TxFIFO", "RxFIFO", "TxReqQ", "RxReqQ", "RxInfoQ",
> > +   "DescFetchQ", "EventQ", "ProtocolStatusQ"};
> > +   spin_lock_irqsave(>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:
>

Re: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Felipe Balbi]

Hi,

changbin...@intel.com writes:
> From: "Du, Changbin" 
>
> 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 
> ---
> 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) & 0x)

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 inttype, index;
> + const char  *name;
> + u32 reg;
> +
> + static const char * const fifo_names[] = {
> + "TxFIFO", "RxFIFO", "TxReqQ", "RxReqQ", "RxInfoQ",
> + "DescFetchQ", "EventQ", "ProtocolStatusQ"};
> + spin_lock_irqsave(>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

Re: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Felipe Balbi]

Hi,

changbin...@intel.com writes:
> From: "Du, Changbin" 
>
> 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 
> ---
> 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) & 0x)

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 inttype, index;
> + const char  *name;
> + u32 reg;
> +
> + static const char * const fifo_names[] = {
> + "TxFIFO", "RxFIFO", "TxReqQ", "RxReqQ", "RxInfoQ",
> + "DescFetchQ", "EventQ", "ProtocolStatusQ"};
> + spin_lock_irqsave(>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

RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Du, Changbin]

Hello, Sergei,
> > From: "Du, Changbin" 
> >
> > For DWC3 USB controller, the Global Debug Queue/FIFO Space Available
> > Register(GDBGFIFOSPACE) can be used to dump FIFO/Queue available
> space.
> 
> Space needed before (.

Okay.

> 
> > 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 
> > ---
> > v4:
> >Do not fail silently, but print error.
> [...]
> > 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
> [...]
> > @@ -642,10 +681,15 @@ void dwc3_debugfs_init(struct dwc3 *dwc)
> > dwc->regset->nregs = ARRAY_SIZE(dwc3_regs);
> > dwc->regset->base = dwc->regs;
> >
> > +
> 
> Why?

Thanks for point out, I will remove this additional line.

> 
> > file = debugfs_create_regset32("regdump", S_IRUGO, root, dwc-
> >regset);
> > if (!file)
> > dev_err(dwc->dev, "Can't create debugfs regdump\n");
> >
> > +   file = debugfs_create_file("fifo", S_IRUGO, root, dwc,
> _fifo_fops);
> > +   if (!file)
> > +   dev_err(dwc->dev, "Can't create debugfs fifo\n");
> > +
> > if (IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)) {
> > file = debugfs_create_file("mode", S_IRUGO | S_IWUSR,
> root,
> > dwc, _mode_fops);
> 
> MBR, Sergei

RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Du, Changbin]

Hello, Sergei,
> > From: "Du, Changbin" 
> >
> > For DWC3 USB controller, the Global Debug Queue/FIFO Space Available
> > Register(GDBGFIFOSPACE) can be used to dump FIFO/Queue available
> space.
> 
> Space needed before (.

Okay.

> 
> > 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 
> > ---
> > v4:
> >Do not fail silently, but print error.
> [...]
> > 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
> [...]
> > @@ -642,10 +681,15 @@ void dwc3_debugfs_init(struct dwc3 *dwc)
> > dwc->regset->nregs = ARRAY_SIZE(dwc3_regs);
> > dwc->regset->base = dwc->regs;
> >
> > +
> 
> Why?

Thanks for point out, I will remove this additional line.

> 
> > file = debugfs_create_regset32("regdump", S_IRUGO, root, dwc-
> >regset);
> > if (!file)
> > dev_err(dwc->dev, "Can't create debugfs regdump\n");
> >
> > +   file = debugfs_create_file("fifo", S_IRUGO, root, dwc,
> _fifo_fops);
> > +   if (!file)
> > +   dev_err(dwc->dev, "Can't create debugfs fifo\n");
> > +
> > if (IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)) {
> > file = debugfs_create_file("mode", S_IRUGO | S_IWUSR,
> root,
> > dwc, _mode_fops);
> 
> MBR, Sergei

Re: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Sergei Shtylyov]

Hello.

On 4/12/2016 2:10 PM, changbin...@intel.com wrote:


From: "Du, Changbin" 

For DWC3 USB controller, the Global Debug Queue/FIFO Space Available
Register(GDBGFIFOSPACE) can be used to dump FIFO/Queue available space.


   Space needed before (.


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 
---
v4:
   Do not fail silently, but print error.

[...]

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

[...]

@@ -642,10 +681,15 @@ void dwc3_debugfs_init(struct dwc3 *dwc)
dwc->regset->nregs = ARRAY_SIZE(dwc3_regs);
dwc->regset->base = dwc->regs;

+


   Why?


file = debugfs_create_regset32("regdump", S_IRUGO, root, dwc->regset);
if (!file)
dev_err(dwc->dev, "Can't create debugfs regdump\n");

+   file = debugfs_create_file("fifo", S_IRUGO, root, dwc, _fifo_fops);
+   if (!file)
+   dev_err(dwc->dev, "Can't create debugfs fifo\n");
+
if (IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)) {
file = debugfs_create_file("mode", S_IRUGO | S_IWUSR, root,
dwc, _mode_fops);


MBR, Sergei

Re: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Sergei Shtylyov]

Hello.

On 4/12/2016 2:10 PM, changbin...@intel.com wrote:


From: "Du, Changbin" 

For DWC3 USB controller, the Global Debug Queue/FIFO Space Available
Register(GDBGFIFOSPACE) can be used to dump FIFO/Queue available space.


   Space needed before (.


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 
---
v4:
   Do not fail silently, but print error.

[...]

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

[...]

@@ -642,10 +681,15 @@ void dwc3_debugfs_init(struct dwc3 *dwc)
dwc->regset->nregs = ARRAY_SIZE(dwc3_regs);
dwc->regset->base = dwc->regs;

+


   Why?


file = debugfs_create_regset32("regdump", S_IRUGO, root, dwc->regset);
if (!file)
dev_err(dwc->dev, "Can't create debugfs regdump\n");

+   file = debugfs_create_file("fifo", S_IRUGO, root, dwc, _fifo_fops);
+   if (!file)
+   dev_err(dwc->dev, "Can't create debugfs fifo\n");
+
if (IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)) {
file = debugfs_create_file("mode", S_IRUGO | S_IWUSR, root,
dwc, _mode_fops);


MBR, Sergei