usb: dwc3: debugfs: Dump internal states

usb: dwc3: debugfs: Dump internal states

[Thinh Nguyen]

To dump internal LSP and endpoint state debug registers, we must write
to GDBGLSPMUX register. This patch correctly dump internal BMU, LSP,
and endpoint states from the debug registers.

If the controller is in device mode, all LSP and endpoint state
registers will be dumped. In host mode, the user must specify a LSP
register by writing to the debugfs attribute "internal_states" with the
LSP number selection.

Fixes: 80b776340c78 ("usb: dwc3: Dump LSP and BMU debug info")
Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 drivers/usb/dwc3/core.h    |  11 +++
 drivers/usb/dwc3/debugfs.c | 169 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 176 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 5bfb62533e0f..662e49ae0510 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -174,6 +174,12 @@
 #define DWC3_GSBUSCFG0_INCRBRSTENA	(1 << 0) /* undefined length enable */
 #define DWC3_GSBUSCFG0_INCRBRST_MASK	0xff
 
+/* Global Debug LSP MUX Select */
+#define DWC3_GDBGLSPMUX_ENDBC		BIT(15)	/* Host only */
+#define DWC3_GDBGLSPMUX_HOSTSELECT(n)	((n) & 0x3fff)
+#define DWC3_GDBGLSPMUX_DEVSELECT(n)	(((n) & 0xf) << 4)
+#define DWC3_GDBGLSPMUX_EPSELECT(n)	((n) & 0xf)
+
 /* Global Debug Queue/FIFO Space Available Register */
 #define DWC3_GDBGFIFOSPACE_NUM(n)	((n) & 0x1f)
 #define DWC3_GDBGFIFOSPACE_TYPE(n)	(((n) << 5) & 0x1e0)
@@ -253,6 +259,9 @@
 #define DWC3_GSTS_DEVICE_IP	BIT(6)
 #define DWC3_GSTS_CSR_TIMEOUT	BIT(5)
 #define DWC3_GSTS_BUS_ERR_ADDR_VLD	BIT(4)
+#define DWC3_GSTS_CURMOD(n)	((n) & 0x3)
+#define DWC3_GSTS_CURMOD_DEVICE	0
+#define DWC3_GSTS_CURMOD_HOST	1
 
 /* Global USB2 PHY Configuration Register */
 #define DWC3_GUSB2PHYCFG_PHYSOFTRST	BIT(31)
@@ -945,6 +954,7 @@ struct dwc3_scratchpad_array {
  * @hwparams: copy of hwparams registers
  * @root: debugfs root folder pointer
  * @regset: debugfs pointer to regdump file
+ * @dbg_lsp_select: current debug lsp mux register selection
  * @test_mode: true when we're entering a USB test mode
  * @test_mode_nr: test feature selector
  * @lpm_nyet_threshold: LPM NYET response threshold
@@ -1120,6 +1130,7 @@ struct dwc3 {
 	struct dwc3_hwparams	hwparams;
 	struct dentry		*root;
 	struct debugfs_regset32	*regset;
+	int			dbg_lsp_select;
 
 	u8			test_mode;
 	u8			test_mode_nr;
diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index df8e73ec3342..cf3c95838a96 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -25,6 +25,8 @@
 #include "io.h"
 #include "debug.h"
 
+#define DWC3_LSP_MUX_UNSELECTED 0xfffff
+
 #define dump_register(nm)				\
 {							\
 	.name	= __stringify(nm),			\
@@ -82,10 +84,6 @@ static const struct debugfs_reg32 dwc3_regs[] = {
 	dump_register(GDBGFIFOSPACE),
 	dump_register(GDBGLTSSM),
 	dump_register(GDBGBMU),
-	dump_register(GDBGLSPMUX),
-	dump_register(GDBGLSP),
-	dump_register(GDBGEPINFO0),
-	dump_register(GDBGEPINFO1),
 	dump_register(GPRTBIMAP_HS0),
 	dump_register(GPRTBIMAP_HS1),
 	dump_register(GPRTBIMAP_FS0),
@@ -279,6 +277,164 @@ static const struct debugfs_reg32 dwc3_regs[] = {
 	dump_register(OSTS),
 };
 
+static u32 dwc3_host_lsp_register(struct dwc3 *dwc, bool is_dbc, int select)
+{
+	unsigned long		flags;
+	u32			reg;
+
+	reg = DWC3_GDBGLSPMUX_HOSTSELECT(select);
+
+	if (is_dbc)
+		reg |= DWC3_GDBGLSPMUX_ENDBC;
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc3_writel(dwc->regs, DWC3_GDBGLSPMUX, reg);
+	reg = dwc3_readl(dwc->regs, DWC3_GDBGLSP);
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	return reg;
+}
+
+static u32 dwc3_gadget_lsp_register(struct dwc3 *dwc, int select)
+{
+	unsigned long		flags;
+	u32			reg;
+
+	reg = DWC3_GDBGLSPMUX_DEVSELECT(select);
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc3_writel(dwc->regs, DWC3_GDBGLSPMUX, reg);
+	reg = dwc3_readl(dwc->regs, DWC3_GDBGLSP);
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	return reg;
+}
+
+static u64 dwc3_ep_info_register(struct dwc3 *dwc, int select)
+{
+	unsigned long		flags;
+	u64			ep_info;
+	u32			lower_32_bits;
+	u32			upper_32_bits;
+	u32			reg;
+
+	reg = DWC3_GDBGLSPMUX_EPSELECT(select);
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc3_writel(dwc->regs, DWC3_GDBGLSPMUX, reg);
+	lower_32_bits = dwc3_readl(dwc->regs, DWC3_GDBGEPINFO0);
+	upper_32_bits = dwc3_readl(dwc->regs, DWC3_GDBGEPINFO1);
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	ep_info = ((u64)upper_32_bits << 32) | lower_32_bits;
+
+	return ep_info;
+}
+
+static void dwc3_dump_host_internal_states(struct seq_file *s)
+{
+	struct dwc3		*dwc = s->private;
+	int			sel;
+	u32			reg;
+
+	sel = dwc->dbg_lsp_select;
+
+	if (sel == DWC3_LSP_MUX_UNSELECTED) {
+		seq_printf(s, "No LSP MUX selection\n");
+		return;
+	}
+
+	reg = dwc3_host_lsp_register(dwc, false, sel);
+	seq_printf(s, "GDBGLSP[%d] = 0x%08x\n", sel, reg);
+
+	reg = dwc3_host_lsp_register(dwc, true, sel);
+	seq_printf(s, "GDBGLSP_DBC[%d] = 0x%08x\n", sel, reg);
+}
+
+static void dwc3_dump_gadget_internal_states(struct seq_file *s)
+{
+	struct dwc3		*dwc = s->private;
+	int			num_selects = 16;
+	int			i;
+	u32			reg;
+	u64			ep_info;
+
+	for (i = 0; i < num_selects; i++) {
+		reg = dwc3_gadget_lsp_register(dwc, i);
+		seq_printf(s, "GDBGLSP[%d] = 0x%08x\n", i, reg);
+	}
+
+	for (i = 0; i < dwc->num_eps; i++) {
+		ep_info = dwc3_ep_info_register(dwc, i);
+		seq_printf(s, "GDBGEPINFO[%d] = 0x%016llx\n", i, ep_info);
+	}
+}
+
+static int dwc3_internal_states_show(struct seq_file *s, void *unused)
+{
+	struct dwc3		*dwc = s->private;
+	unsigned int		current_mode;
+	unsigned long		flags;
+	u32			reg;
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	reg = dwc3_readl(dwc->regs, DWC3_GSTS);
+	current_mode = DWC3_GSTS_CURMOD(reg);
+
+	reg = dwc3_readl(dwc->regs, DWC3_GDBGBMU);
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	seq_printf(s, "GDBGBMU = 0x%08x\n", reg);
+
+	switch (current_mode) {
+	case DWC3_GSTS_CURMOD_HOST:
+		dwc3_dump_host_internal_states(s);
+		break;
+	case DWC3_GSTS_CURMOD_DEVICE:
+		dwc3_dump_gadget_internal_states(s);
+		break;
+	default:
+		seq_printf(s, "Mode is unknown, no LSP register printed\n");
+		break;
+	}
+
+	return 0;
+}
+
+static int dwc3_internal_states_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, dwc3_internal_states_show, inode->i_private);
+}
+
+static ssize_t dwc3_internal_states_write(struct file *file,
+		const char __user *ubuf, size_t count, loff_t *ppos)
+{
+	struct seq_file		*s = file->private_data;
+	struct dwc3		*dwc = s->private;
+	char			buf[32] = { 0 };
+	int			sel;
+	int			ret;
+
+	if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
+		return -EFAULT;
+
+	ret = kstrtoint(buf, 0, &sel);
+	if (ret)
+		return ret;
+
+	dwc->dbg_lsp_select = sel;
+
+	return count;
+}
+
+static const struct file_operations dwc3_internal_states_fops = {
+	.open			= dwc3_internal_states_open,
+	.write			= dwc3_internal_states_write,
+	.read			= seq_read,
+	.llseek			= seq_lseek,
+	.release		= single_release,
+};
+
 static int dwc3_mode_show(struct seq_file *s, void *unused)
 {
 	struct dwc3		*dwc = s->private;
@@ -742,6 +898,8 @@ void dwc3_debugfs_init(struct dwc3 *dwc)
 	if (!dwc->regset)
 		return;
 
+	dwc->dbg_lsp_select = DWC3_LSP_MUX_UNSELECTED;
+
 	dwc->regset->regs = dwc3_regs;
 	dwc->regset->nregs = ARRAY_SIZE(dwc3_regs);
 	dwc->regset->base = dwc->regs - DWC3_GLOBALS_REGS_START;
@@ -751,6 +909,9 @@ void dwc3_debugfs_init(struct dwc3 *dwc)
 
 	debugfs_create_regset32("regdump", S_IRUGO, root, dwc->regset);
 
+	debugfs_create_file("internal_states", S_IRUGO | S_IWUSR, root, dwc,
+			    &dwc3_internal_states_fops);
+
 	if (IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)) {
 		debugfs_create_file("mode", S_IRUGO | S_IWUSR, root, dwc,
 				    &dwc3_mode_fops);

usb: dwc3: debugfs: Dump internal states

[Greg Kroah-Hartman]

On Wed, Nov 07, 2018 at 12:45:50PM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> >> Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
> >> >>>>> +static ssize_t dwc3_internal_states_write(struct file *file,
> >> >>>>> +		const char __user *ubuf, size_t count, loff_t *ppos)
> >> >>>> why is this necessary? Seems like it would be nicer to create a
> >> >>>> directory structure if the current operating mode is host so that we
> >> >>>> don't need to write anything.
> >> >>> Can you clarify more about the directory structure? Actually, I was
> >> >>> wondering what's the best way to do this also. The reason I want to
> >> >>> write to it is because the LSP selection for host is quite large (2^14).
> >> >> right, turn each of those into a directory of its own. If you don't want
> >> >> to or can't disclose proper names for those directories, just call them
> >> >> lsp0, lsp1, lsp2, and so on. Then a read of the file under lsp1
> >> >> directory would write and read the correct registers.
> >> >>
> >> >> Everything remains read-only.
> >> >
> >> > This means that there will be 16384 + 256 files create for host. It also
> >> > means that we need to kmalloc at least (16384 + 256) * (size of each
> >> > selection) so that each file knows what to print. On top of that, when
> >> > we change mode of operation (e.g. device->host), then we need to
> >> > create/destroy all these files. Is this the way to do it?
> >> 
> >> Hmm... indeed that's a lot. But since this is used only for debugging I
> >> wonder if we should care. Greg, Alan, what do you guys think? Do we
> >> create 16k files under debugfs or single file that needs to be written
> >> to before being read?
> >
> > 16k files under debugfs is crazy, don't do that :)
> 
> one file with write followed by read it is then :-p
> 
> > What type of interface would ever want such a thing?  I have not been
> > paying attention, but what is the end goal here?  What do you want to
> > expose in debugfs that is actually going to be useful?
> 
> internal debug information from dwc3's list processor (the HW
> itself). It's only useful for synopsys, really, as the content of such
> registers lacks documentation about how to decode it.

Ok, one file with crazy semantics is fine, remember the only "rule" for
debugfs is "there are no rules" :)

And of course, the normal operation of the driver should never require
debugfs to be present or even working.

thanks,

greg k-h

usb: dwc3: debugfs: Dump internal states

[Felipe Balbi]

Hi,

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
>> >>>>> +static ssize_t dwc3_internal_states_write(struct file *file,
>> >>>>> +		const char __user *ubuf, size_t count, loff_t *ppos)
>> >>>> why is this necessary? Seems like it would be nicer to create a
>> >>>> directory structure if the current operating mode is host so that we
>> >>>> don't need to write anything.
>> >>> Can you clarify more about the directory structure? Actually, I was
>> >>> wondering what's the best way to do this also. The reason I want to
>> >>> write to it is because the LSP selection for host is quite large (2^14).
>> >> right, turn each of those into a directory of its own. If you don't want
>> >> to or can't disclose proper names for those directories, just call them
>> >> lsp0, lsp1, lsp2, and so on. Then a read of the file under lsp1
>> >> directory would write and read the correct registers.
>> >>
>> >> Everything remains read-only.
>> >
>> > This means that there will be 16384 + 256 files create for host. It also
>> > means that we need to kmalloc at least (16384 + 256) * (size of each
>> > selection) so that each file knows what to print. On top of that, when
>> > we change mode of operation (e.g. device->host), then we need to
>> > create/destroy all these files. Is this the way to do it?
>> 
>> Hmm... indeed that's a lot. But since this is used only for debugging I
>> wonder if we should care. Greg, Alan, what do you guys think? Do we
>> create 16k files under debugfs or single file that needs to be written
>> to before being read?
>
> 16k files under debugfs is crazy, don't do that :)

one file with write followed by read it is then :-p

> What type of interface would ever want such a thing?  I have not been
> paying attention, but what is the end goal here?  What do you want to
> expose in debugfs that is actually going to be useful?

internal debug information from dwc3's list processor (the HW
itself). It's only useful for synopsys, really, as the content of such
registers lacks documentation about how to decode it.

usb: dwc3: debugfs: Dump internal states

[Greg Kroah-Hartman]

On Wed, Nov 07, 2018 at 08:39:41AM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
> >>>>> +static ssize_t dwc3_internal_states_write(struct file *file,
> >>>>> +		const char __user *ubuf, size_t count, loff_t *ppos)
> >>>> why is this necessary? Seems like it would be nicer to create a
> >>>> directory structure if the current operating mode is host so that we
> >>>> don't need to write anything.
> >>> Can you clarify more about the directory structure? Actually, I was
> >>> wondering what's the best way to do this also. The reason I want to
> >>> write to it is because the LSP selection for host is quite large (2^14).
> >> right, turn each of those into a directory of its own. If you don't want
> >> to or can't disclose proper names for those directories, just call them
> >> lsp0, lsp1, lsp2, and so on. Then a read of the file under lsp1
> >> directory would write and read the correct registers.
> >>
> >> Everything remains read-only.
> >
> > This means that there will be 16384 + 256 files create for host. It also
> > means that we need to kmalloc at least (16384 + 256) * (size of each
> > selection) so that each file knows what to print. On top of that, when
> > we change mode of operation (e.g. device->host), then we need to
> > create/destroy all these files. Is this the way to do it?
> 
> Hmm... indeed that's a lot. But since this is used only for debugging I
> wonder if we should care. Greg, Alan, what do you guys think? Do we
> create 16k files under debugfs or single file that needs to be written
> to before being read?

16k files under debugfs is crazy, don't do that :)

What type of interface would ever want such a thing?  I have not been
paying attention, but what is the end goal here?  What do you want to
expose in debugfs that is actually going to be useful?

thanks,

greg k-h

usb: dwc3: debugfs: Dump internal states

[Thinh Nguyen]

Hi Felipe,

On 11/5/2018 11:35 PM, Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
>>>> +static int dwc3_internal_states_show(struct seq_file *s, void *unused)
>>>> +{
>>>> +	struct dwc3		*dwc = s->private;
>>>> +	unsigned int		current_mode;
>>>> +	unsigned long		flags;
>>>> +	u32			reg;
>>>> +
>>>> +	spin_lock_irqsave(&dwc->lock, flags);
>>>> +	reg = dwc3_readl(dwc->regs, DWC3_GSTS);
>>>> +	current_mode = DWC3_GSTS_CURMOD(reg);
>>>> +
>>>> +	reg = dwc3_readl(dwc->regs, DWC3_GDBGBMU);
>>>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>>>> +
>>>> +	seq_printf(s, "GDBGBMU = 0x%08x\n", reg);
>>> shouldn't the print be done with locks held?
>> I think the lock for the prints should be held at a higher level.
>> Otherwise, I don't think it will make a difference using dwc->lock.
> what happens if this gets preempted when you release the lock and
> a different thread writes to internal_states and reads the result before
> $this thread?

I see.

>
>>>> +static ssize_t dwc3_internal_states_write(struct file *file,
>>>> +		const char __user *ubuf, size_t count, loff_t *ppos)
>>> why is this necessary? Seems like it would be nicer to create a
>>> directory structure if the current operating mode is host so that we
>>> don't need to write anything.
>> Can you clarify more about the directory structure? Actually, I was
>> wondering what's the best way to do this also. The reason I want to
>> write to it is because the LSP selection for host is quite large (2^14).
> right, turn each of those into a directory of its own. If you don't want
> to or can't disclose proper names for those directories, just call them
> lsp0, lsp1, lsp2, and so on. Then a read of the file under lsp1
> directory would write and read the correct registers.
>
> Everything remains read-only.

This means that there will be 16384 + 256 files create for host. It also
means that we need to kmalloc at least (16384 + 256) * (size of each
selection) so that each file knows what to print. On top of that, when
we change mode of operation (e.g. device->host), then we need to
create/destroy all these files. Is this the way to do it?

Thanks,

Thinh

usb: dwc3: debugfs: Dump internal states

[Felipe Balbi]

Hi,

Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
>>> +static int dwc3_internal_states_show(struct seq_file *s, void *unused)
>>> +{
>>> +	struct dwc3		*dwc = s->private;
>>> +	unsigned int		current_mode;
>>> +	unsigned long		flags;
>>> +	u32			reg;
>>> +
>>> +	spin_lock_irqsave(&dwc->lock, flags);
>>> +	reg = dwc3_readl(dwc->regs, DWC3_GSTS);
>>> +	current_mode = DWC3_GSTS_CURMOD(reg);
>>> +
>>> +	reg = dwc3_readl(dwc->regs, DWC3_GDBGBMU);
>>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>>> +
>>> +	seq_printf(s, "GDBGBMU = 0x%08x\n", reg);
>> shouldn't the print be done with locks held?
>
> I think the lock for the prints should be held at a higher level.
> Otherwise, I don't think it will make a difference using dwc->lock.

what happens if this gets preempted when you release the lock and
a different thread writes to internal_states and reads the result before
$this thread?

>>> +static ssize_t dwc3_internal_states_write(struct file *file,
>>> +		const char __user *ubuf, size_t count, loff_t *ppos)
>> why is this necessary? Seems like it would be nicer to create a
>> directory structure if the current operating mode is host so that we
>> don't need to write anything.
>
> Can you clarify more about the directory structure? Actually, I was
> wondering what's the best way to do this also. The reason I want to
> write to it is because the LSP selection for host is quite large (2^14).

right, turn each of those into a directory of its own. If you don't want
to or can't disclose proper names for those directories, just call them
lsp0, lsp1, lsp2, and so on. Then a read of the file under lsp1
directory would write and read the correct registers.

Everything remains read-only.

> It doesn't make sense to dump everything if the controller is in host
> mode. So I force the user to write a specific LSP selection to dump at a
> time.

you can just as well force the use to read a file under a specific
directory. And if that same user really wants to read everything, it's
easy to do so with a simple loop to cat every file under every
directory.

Now, if directories would have a single file under, then you may decide
to, instead, create a single directory named e.g. lsp and put several
files under it. It's a matter of how much information you need to dump.

cheers

usb: dwc3: debugfs: Dump internal states

[Thinh Nguyen]

Hi Felipe,

On 11/5/2018 4:16 AM, Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
>> +static void dwc3_dump_gadget_internal_states(struct seq_file *s)
>> +{
>> +	struct dwc3		*dwc = s->private;
>> +	int			num_selects = 16;
>> +	int			i;
>> +	u32			reg;
>> +	u64			ep_info;
>> +
>> +	for (i = 0; i < num_selects; i++) {
>> +		reg = dwc3_gadget_lsp_register(dwc, i);
>> +		seq_printf(s, "GDBGLSP[%d] = 0x%08x\n", i, reg);
>> +	}
>> +
>> +	for (i = 0; i < dwc->num_eps; i++) {
>> +		ep_info = dwc3_ep_info_register(dwc, i);
>> +		seq_printf(s, "GDBGEPINFO[%d] = 0x%016llx\n", i, ep_info);
>> +	}
>> +}
> we have per-endpoint directories already. Why don't you dump endpoint
> debug info there?

Yes, I can dump it there.


> Also, while at that, could you write a patch that
> properly decodes the queue sizes? It looks to me as the queue sizes are
> in same units as GTXFIFOSIZ registers

Sure. It can be done.

>> +static int dwc3_internal_states_show(struct seq_file *s, void *unused)
>> +{
>> +	struct dwc3		*dwc = s->private;
>> +	unsigned int		current_mode;
>> +	unsigned long		flags;
>> +	u32			reg;
>> +
>> +	spin_lock_irqsave(&dwc->lock, flags);
>> +	reg = dwc3_readl(dwc->regs, DWC3_GSTS);
>> +	current_mode = DWC3_GSTS_CURMOD(reg);
>> +
>> +	reg = dwc3_readl(dwc->regs, DWC3_GDBGBMU);
>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>> +
>> +	seq_printf(s, "GDBGBMU = 0x%08x\n", reg);
> shouldn't the print be done with locks held?

I think the lock for the prints should be held at a higher level.
Otherwise, I don't think it will make a difference using dwc->lock.

>> +static ssize_t dwc3_internal_states_write(struct file *file,
>> +		const char __user *ubuf, size_t count, loff_t *ppos)
> why is this necessary? Seems like it would be nicer to create a
> directory structure if the current operating mode is host so that we
> don't need to write anything.

Can you clarify more about the directory structure? Actually, I was
wondering what's the best way to do this also. The reason I want to
write to it is because the LSP selection for host is quite large (2^14).
It doesn't make sense to dump everything if the controller is in host
mode. So I force the user to write a specific LSP selection to dump at a
time.

Thanks for the review!
Thinh

usb: dwc3: debugfs: Dump internal states

[Felipe Balbi]

Hi,

Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
> +static void dwc3_dump_gadget_internal_states(struct seq_file *s)
> +{
> +	struct dwc3		*dwc = s->private;
> +	int			num_selects = 16;
> +	int			i;
> +	u32			reg;
> +	u64			ep_info;
> +
> +	for (i = 0; i < num_selects; i++) {
> +		reg = dwc3_gadget_lsp_register(dwc, i);
> +		seq_printf(s, "GDBGLSP[%d] = 0x%08x\n", i, reg);
> +	}
> +
> +	for (i = 0; i < dwc->num_eps; i++) {
> +		ep_info = dwc3_ep_info_register(dwc, i);
> +		seq_printf(s, "GDBGEPINFO[%d] = 0x%016llx\n", i, ep_info);
> +	}
> +}

we have per-endpoint directories already. Why don't you dump endpoint
debug info there? Also, while at that, could you write a patch that
properly decodes the queue sizes? It looks to me as the queue sizes are
in same units as GTXFIFOSIZ registers

> +static int dwc3_internal_states_show(struct seq_file *s, void *unused)
> +{
> +	struct dwc3		*dwc = s->private;
> +	unsigned int		current_mode;
> +	unsigned long		flags;
> +	u32			reg;
> +
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	reg = dwc3_readl(dwc->regs, DWC3_GSTS);
> +	current_mode = DWC3_GSTS_CURMOD(reg);
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GDBGBMU);
> +	spin_unlock_irqrestore(&dwc->lock, flags);
> +
> +	seq_printf(s, "GDBGBMU = 0x%08x\n", reg);

shouldn't the print be done with locks held?

> +static ssize_t dwc3_internal_states_write(struct file *file,
> +		const char __user *ubuf, size_t count, loff_t *ppos)

why is this necessary? Seems like it would be nicer to create a
directory structure if the current operating mode is host so that we
don't need to write anything.