| Message ID | 0847a3b3698e70ae9a736cca8161706715e0a42e.1541209076.git.thinhn@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: debugfs: Dump internal states | expand |
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.
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
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
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
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
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.
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
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);
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(-)