usb: dwc3: debug: Print register name

usb: dwc3: debug: Print register name

[Thinh Nguyen]

Hi Felipe,

On 11/27/2018 11:16 PM, Felipe Balbi wrote:
> Hi,
>
> Tejas Joglekar <tejas.joglekar@synopsys.com> writes:
>> From: Thinh Nguyen <thinhn@synopsys.com>
>>
>> This commit adds a new debugging option CONFIG_USB_DWC3_DEBUG_REG_PRINT
>> to enable printing of register names to tracepoints for
>> register read/write.
>>
>> Sample trace:
>> ----------
>>     283.675504: dwc3_writel: DEPCMDPAR0 addr ffffc9000ba1c838 value 00000000
>>     283.675505: dwc3_writel: DEPCMDPAR1 addr ffffc9000ba1c834 value 00000000
>>     283.675505: dwc3_writel: DEPCMDPAR2 addr ffffc9000ba1c830 value 00000000
>>     283.675506: dwc3_writel: DEPCMD addr ffffc9000ba1c83c value 00030d08
>>     283.675506: dwc3_readl:  DEPCMD addr ffffc9000ba1c83c value 00030d08
>>     283.675509: dwc3_readl:  DEPCMD addr ffffc9000ba1c83c value 00030908
>>     283.675512: dwc3_gadget_ep_cmd: ep1in: cmd 'End Transfer' [199944] params 00000000 00000000 00000000 --> status: Successful
>>     283.675524: dwc3_readl:  GEVNTCOUNT addr ffffc9000ba1c40c value 00000004
>>     283.675526: dwc3_readl:  GEVNTSIZ addr ffffc9000ba1c408 value 00001000
>>     283.675528: dwc3_writel: GEVNTSIZ addr ffffc9000ba1c408 value 80001000
>>     283.675529: dwc3_writel: GEVNTCOUNT addr ffffc9000ba1c40c value 00000004
>>     283.675614: dwc3_readl:  DALEPENA addr ffffc9000ba1c720 value 00000003
>>     283.675615: dwc3_writel: DALEPENA addr ffffc9000ba1c720 value 00000003
>> ----------
>>
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>> Signed-off-by: Tejas Joglekar <joglekar@synopsys.com>
>> ---
>>  drivers/usb/dwc3/Kconfig   |   8 +++
>>  drivers/usb/dwc3/debug.h   |  10 ++++
>>  drivers/usb/dwc3/debugfs.c | 139 +++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/usb/dwc3/trace.h   |   7 ++-
>>  4 files changed, 162 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>> index 1a0404f..8039c22 100644
>> --- a/drivers/usb/dwc3/Kconfig
>> +++ b/drivers/usb/dwc3/Kconfig
>> @@ -48,6 +48,14 @@ config USB_DWC3_DUAL_ROLE
>>  
>>  endchoice
>>  
>> +config USB_DWC3_DEBUG_REG_PRINT
>> +	bool "Enable register name printing to the trace"
>> +	depends on (USB_DWC3 && DEBUG_FS)
>> +	default n
>> +	help
>> +	  Select this option if you want to enable trace logging with
>> +	  register name prints on register read and write.
> Why is this a compile-time option?

We added this as an optional debugging printout to avoid any performance
hit by the giant switch case (however minuscule), and we try to avoid
changing the default printout to keep it compatible with any previous
parser for DWC3 tracepoint.

>
>> diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
>> index c66d216..20b05a1 100644
>> --- a/drivers/usb/dwc3/debug.h
>> +++ b/drivers/usb/dwc3/debug.h
>> @@ -75,6 +75,16 @@ dwc3_gadget_generic_cmd_string(u8 cmd)
>>  	}
>>  }
>>  
>> +#ifdef CONFIG_USB_DWC3_DEBUG_REG_PRINT
>> +const char *dwc3_gadget_register_string(u16 offset);
>> +#else
>> +static inline const char *
>> +dwc3_gadget_register_string(u16 offset)
>> +{
>> +	return "";
>> +}
>> +#endif
> no ifdefs, please.

Ok.

>
>> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
>> index df8e73e..0d25c03 100644
>> --- a/drivers/usb/dwc3/debugfs.c
>> +++ b/drivers/usb/dwc3/debugfs.c
>> @@ -734,6 +734,145 @@ static void dwc3_debugfs_create_endpoint_dirs(struct dwc3 *dwc,
>>  	}
>>  }
>>  
>> +#ifdef CONFIG_USB_DWC3_DEBUG_REG_PRINT
>> +/**
>> + * dwc3_gadget_register_string - returns register name
>> + * @offset: register offset
>> + */
>> +const char *dwc3_gadget_register_string(u16 offset)
>> +{
>> +	if (offset >= DWC3_GTXFIFOSIZ(0) && offset < DWC3_GTXFIFOSIZ(32))
>> +		return "GTXFIFOSIZ ";
>> +	if (offset >= DWC3_GRXFIFOSIZ(0) && offset < DWC3_GRXFIFOSIZ(32))
>> +		return "GRXFIFOSIZ ";
>> +
>> +	switch (offset) {
>> +	case DWC3_GUSB2PHYCFG(0):
>> +		return "GUSB2PHYCFG(0) ";
> I really wonder if we really need this in the kernel. I mean, look at
> the amount of code just to convert an address into a string. Why don't
> we do this as a post-processing step? Should be fairly simple to write a
> parser for this, no?

Yeah.. It's a giant switch case. That's why we put it as an additional
debugging option. The trace for DWC3 is quite easy to read with the
exception of deciphering the register read/write. Creating and
maintaining a parser will not be needed if this is added.

>
> I mean, I don't mind having the kernel do this, but it just seems like a
> bit overkill.

Probably.  But it can help everyone with this in the kernel.

>  One can argue that the same can be said about control
> requests which I've added support for decoding, but the idea was that
> the decoding logic would be turned into a generic library which UDCs and
> HCDs can use. This, on the other hand, is dwc3 specific.
>
> Would you be open to a tool that does post-processing on dwc3
> tracepoints? I can help write it out, no issues, but we need to decide
> what needs to go in there.

Sure. I may not be as active while I have other projects on the side.
I'll contribute whatever I can if you want to go this route. :)

> I'm also open to being persuaded to accepting
> $subject, but I need someone to convince me that this is the best way
> forward.
>
> cheers
>

Thanks,
Thinh

usb: dwc3: debug: Print register name

[Felipe Balbi]

Hi,

Tejas Joglekar <tejas.joglekar@synopsys.com> writes:
> From: Thinh Nguyen <thinhn@synopsys.com>
>
> This commit adds a new debugging option CONFIG_USB_DWC3_DEBUG_REG_PRINT
> to enable printing of register names to tracepoints for
> register read/write.
>
> Sample trace:
> ----------
>     283.675504: dwc3_writel: DEPCMDPAR0 addr ffffc9000ba1c838 value 00000000
>     283.675505: dwc3_writel: DEPCMDPAR1 addr ffffc9000ba1c834 value 00000000
>     283.675505: dwc3_writel: DEPCMDPAR2 addr ffffc9000ba1c830 value 00000000
>     283.675506: dwc3_writel: DEPCMD addr ffffc9000ba1c83c value 00030d08
>     283.675506: dwc3_readl:  DEPCMD addr ffffc9000ba1c83c value 00030d08
>     283.675509: dwc3_readl:  DEPCMD addr ffffc9000ba1c83c value 00030908
>     283.675512: dwc3_gadget_ep_cmd: ep1in: cmd 'End Transfer' [199944] params 00000000 00000000 00000000 --> status: Successful
>     283.675524: dwc3_readl:  GEVNTCOUNT addr ffffc9000ba1c40c value 00000004
>     283.675526: dwc3_readl:  GEVNTSIZ addr ffffc9000ba1c408 value 00001000
>     283.675528: dwc3_writel: GEVNTSIZ addr ffffc9000ba1c408 value 80001000
>     283.675529: dwc3_writel: GEVNTCOUNT addr ffffc9000ba1c40c value 00000004
>     283.675614: dwc3_readl:  DALEPENA addr ffffc9000ba1c720 value 00000003
>     283.675615: dwc3_writel: DALEPENA addr ffffc9000ba1c720 value 00000003
> ----------
>
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> Signed-off-by: Tejas Joglekar <joglekar@synopsys.com>
> ---
>  drivers/usb/dwc3/Kconfig   |   8 +++
>  drivers/usb/dwc3/debug.h   |  10 ++++
>  drivers/usb/dwc3/debugfs.c | 139 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/dwc3/trace.h   |   7 ++-
>  4 files changed, 162 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index 1a0404f..8039c22 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -48,6 +48,14 @@ config USB_DWC3_DUAL_ROLE
>  
>  endchoice
>  
> +config USB_DWC3_DEBUG_REG_PRINT
> +	bool "Enable register name printing to the trace"
> +	depends on (USB_DWC3 && DEBUG_FS)
> +	default n
> +	help
> +	  Select this option if you want to enable trace logging with
> +	  register name prints on register read and write.

Why is this a compile-time option?

> diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
> index c66d216..20b05a1 100644
> --- a/drivers/usb/dwc3/debug.h
> +++ b/drivers/usb/dwc3/debug.h
> @@ -75,6 +75,16 @@ dwc3_gadget_generic_cmd_string(u8 cmd)
>  	}
>  }
>  
> +#ifdef CONFIG_USB_DWC3_DEBUG_REG_PRINT
> +const char *dwc3_gadget_register_string(u16 offset);
> +#else
> +static inline const char *
> +dwc3_gadget_register_string(u16 offset)
> +{
> +	return "";
> +}
> +#endif

no ifdefs, please.

> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> index df8e73e..0d25c03 100644
> --- a/drivers/usb/dwc3/debugfs.c
> +++ b/drivers/usb/dwc3/debugfs.c
> @@ -734,6 +734,145 @@ static void dwc3_debugfs_create_endpoint_dirs(struct dwc3 *dwc,
>  	}
>  }
>  
> +#ifdef CONFIG_USB_DWC3_DEBUG_REG_PRINT
> +/**
> + * dwc3_gadget_register_string - returns register name
> + * @offset: register offset
> + */
> +const char *dwc3_gadget_register_string(u16 offset)
> +{
> +	if (offset >= DWC3_GTXFIFOSIZ(0) && offset < DWC3_GTXFIFOSIZ(32))
> +		return "GTXFIFOSIZ ";
> +	if (offset >= DWC3_GRXFIFOSIZ(0) && offset < DWC3_GRXFIFOSIZ(32))
> +		return "GRXFIFOSIZ ";
> +
> +	switch (offset) {
> +	case DWC3_GUSB2PHYCFG(0):
> +		return "GUSB2PHYCFG(0) ";

I really wonder if we really need this in the kernel. I mean, look at
the amount of code just to convert an address into a string. Why don't
we do this as a post-processing step? Should be fairly simple to write a
parser for this, no?

I mean, I don't mind having the kernel do this, but it just seems like a
bit overkill. One can argue that the same can be said about control
requests which I've added support for decoding, but the idea was that
the decoding logic would be turned into a generic library which UDCs and
HCDs can use. This, on the other hand, is dwc3 specific.

Would you be open to a tool that does post-processing on dwc3
tracepoints? I can help write it out, no issues, but we need to decide
what needs to go in there. I'm also open to being persuaded to accepting
$subject, but I need someone to convince me that this is the best way
forward.

cheers

usb: dwc3: debug: Print register name

[Tejas Joglekar]

From: Thinh Nguyen <thinhn@synopsys.com>

This commit adds a new debugging option CONFIG_USB_DWC3_DEBUG_REG_PRINT
to enable printing of register names to tracepoints for
register read/write.

Sample trace:
----------
    283.675504: dwc3_writel: DEPCMDPAR0 addr ffffc9000ba1c838 value 00000000
    283.675505: dwc3_writel: DEPCMDPAR1 addr ffffc9000ba1c834 value 00000000
    283.675505: dwc3_writel: DEPCMDPAR2 addr ffffc9000ba1c830 value 00000000
    283.675506: dwc3_writel: DEPCMD addr ffffc9000ba1c83c value 00030d08
    283.675506: dwc3_readl:  DEPCMD addr ffffc9000ba1c83c value 00030d08
    283.675509: dwc3_readl:  DEPCMD addr ffffc9000ba1c83c value 00030908
    283.675512: dwc3_gadget_ep_cmd: ep1in: cmd 'End Transfer' [199944] params 00000000 00000000 00000000 --> status: Successful
    283.675524: dwc3_readl:  GEVNTCOUNT addr ffffc9000ba1c40c value 00000004
    283.675526: dwc3_readl:  GEVNTSIZ addr ffffc9000ba1c408 value 00001000
    283.675528: dwc3_writel: GEVNTSIZ addr ffffc9000ba1c408 value 80001000
    283.675529: dwc3_writel: GEVNTCOUNT addr ffffc9000ba1c40c value 00000004
    283.675614: dwc3_readl:  DALEPENA addr ffffc9000ba1c720 value 00000003
    283.675615: dwc3_writel: DALEPENA addr ffffc9000ba1c720 value 00000003
----------

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
Signed-off-by: Tejas Joglekar <joglekar@synopsys.com>
---
 drivers/usb/dwc3/Kconfig   |   8 +++
 drivers/usb/dwc3/debug.h   |  10 ++++
 drivers/usb/dwc3/debugfs.c | 139 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/dwc3/trace.h   |   7 ++-
 4 files changed, 162 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 1a0404f..8039c22 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -48,6 +48,14 @@ config USB_DWC3_DUAL_ROLE
 
 endchoice
 
+config USB_DWC3_DEBUG_REG_PRINT
+	bool "Enable register name printing to the trace"
+	depends on (USB_DWC3 && DEBUG_FS)
+	default n
+	help
+	  Select this option if you want to enable trace logging with
+	  register name prints on register read and write.
+
 comment "Platform Glue Driver Support"
 
 config USB_DWC3_OMAP
diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
index c66d216..20b05a1 100644
--- a/drivers/usb/dwc3/debug.h
+++ b/drivers/usb/dwc3/debug.h
@@ -75,6 +75,16 @@ dwc3_gadget_generic_cmd_string(u8 cmd)
 	}
 }
 
+#ifdef CONFIG_USB_DWC3_DEBUG_REG_PRINT
+const char *dwc3_gadget_register_string(u16 offset);
+#else
+static inline const char *
+dwc3_gadget_register_string(u16 offset)
+{
+	return "";
+}
+#endif
+
 /**
  * dwc3_gadget_link_string - returns link name
  * @link_state: link state code
diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index df8e73e..0d25c03 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -734,6 +734,145 @@ static void dwc3_debugfs_create_endpoint_dirs(struct dwc3 *dwc,
 	}
 }
 
+#ifdef CONFIG_USB_DWC3_DEBUG_REG_PRINT
+/**
+ * dwc3_gadget_register_string - returns register name
+ * @offset: register offset
+ */
+const char *dwc3_gadget_register_string(u16 offset)
+{
+	if (offset >= DWC3_GTXFIFOSIZ(0) && offset < DWC3_GTXFIFOSIZ(32))
+		return "GTXFIFOSIZ ";
+	if (offset >= DWC3_GRXFIFOSIZ(0) && offset < DWC3_GRXFIFOSIZ(32))
+		return "GRXFIFOSIZ ";
+
+	switch (offset) {
+	case DWC3_GUSB2PHYCFG(0):
+		return "GUSB2PHYCFG(0) ";
+	case DWC3_GUSB2PHYACC(0):
+		return "GUSB2PHYACC(0) ";
+	case DWC3_GUSB3PIPECTL(0):
+		return "DWC3_GUSB3PIPECTL(0) ";
+	case DWC3_GUSB2I2CCTL(0):
+		return "DWC3_GUSB2I2CCTL(0) ";
+	case DWC3_DEPCMDPAR2:
+		return "DEPCMDPAR2 ";
+	case DWC3_DEPCMDPAR1:
+		return "DEPCMDPAR1 ";
+	case DWC3_DEPCMDPAR0:
+		return "DEPCMDPAR0 ";
+	case DWC3_DEPCMD:
+		return "DEPCMD ";
+	case DWC3_GSBUSCFG0:
+		return "GSBUSCFG0 ";
+	case DWC3_GSBUSCFG1:
+		return "GSBUSCFG1 ";
+	case DWC3_GTXTHRCFG:
+		return "GTXTHRCFG ";
+	case DWC3_GRXTHRCFG:
+		return "GRXTHRCFG ";
+	case DWC3_GCTL:
+		return "GCTL ";
+	case DWC3_GEVTEN:
+		return "GEVTEN ";
+	case DWC3_GSTS:
+		return "GSTS ";
+	case DWC3_GUCTL1:
+		return "GUCTL1 ";
+	case DWC3_GSNPSID:
+		return "GSNPSID ";
+	case DWC3_GGPIO:
+		return "GGPIO ";
+	case DWC3_GUID:
+		return "GUID ";
+	case DWC3_GUCTL:
+		return "GUCTL ";
+	case DWC3_GBUSERRADDR0:
+		return "GBUSERRADDR0 ";
+	case DWC3_GBUSERRADDR1:
+		return "GBUSERRADDR1 ";
+	case DWC3_GPRTBIMAP0:
+		return "GPRTBIMAP0 ";
+	case DWC3_GPRTBIMAP1:
+		return "GPRTBIMAP1 ";
+	case DWC3_GHWPARAMS0:
+		return "GHWPARAMS0 ";
+	case DWC3_GHWPARAMS1:
+		return "GHWPARAMS1 ";
+	case DWC3_GHWPARAMS2:
+		return "GHWPARAMS2 ";
+	case DWC3_GHWPARAMS3:
+		return "GHWPARAMS3 ";
+	case DWC3_GHWPARAMS4:
+		return "GHWPARAMS4 ";
+	case DWC3_GHWPARAMS5:
+		return "GHWPARAMS5 ";
+	case DWC3_GHWPARAMS6:
+		return "GHWPARAMS6 ";
+	case DWC3_GHWPARAMS7:
+		return "GHWPARAMS7 ";
+	case DWC3_GHWPARAMS8:
+		return "GHWPARAMS8 ";
+	case DWC3_GDBGFIFOSPACE:
+		return "GDBGFIFOSPACE ";
+	case DWC3_GDBGLTSSM:
+		return "GDBGLTSSM ";
+	case DWC3_GPRTBIMAP_HS0:
+		return "GPRTBIMAP_HS0 ";
+	case DWC3_GPRTBIMAP_HS1:
+		return "GPRTBIMAP_HS1 ";
+	case DWC3_GPRTBIMAP_FS0:
+		return "GPRTBIMAP_FS0 ";
+	case DWC3_GPRTBIMAP_FS1:
+		return "GPRTBIMAP_FS1 ";
+	case DWC3_GUCTL2:
+		return "GUCTL2 ";
+	case DWC3_VER_NUMBER:
+		return "VER_NUMBER ";
+	case DWC3_VER_TYPE:
+		return "VER_TYPE ";
+	case DWC3_DEV_IMOD(0):
+		return "DEV_IMOD ";
+	case DWC3_GEVNTADRLO(0):
+		return "GEVNTADRLO ";
+	case DWC3_GEVNTADRHI(0):
+		return "GEVNTADRHI ";
+	case DWC3_GEVNTSIZ(0):
+		return "GEVNTSIZ ";
+	case DWC3_GEVNTCOUNT(0):
+		return "GEVNTCOUNT ";
+	case DWC3_GFLADJ:
+		return "GFLADJ ";
+	case DWC3_DCFG:
+		return "DCFG ";
+	case DWC3_DCTL:
+		return "DCTL ";
+	case DWC3_DEVTEN:
+		return "DEVTEN ";
+	case DWC3_DSTS:
+		return "DSTS ";
+	case DWC3_DGCMDPAR:
+		return "DGCMDPAR ";
+	case DWC3_DGCMD:
+		return "DGCMD ";
+	case DWC3_DALEPENA:
+		return "DALEPENA ";
+	case DWC3_OCFG:
+		return "OCFG ";
+	case DWC3_OCTL:
+		return "OCTL ";
+	case DWC3_OEVT:
+		return "OEVT ";
+	case DWC3_OEVTEN:
+		return "OEVTEN ";
+	case DWC3_OSTS:
+		return "OSTS ";
+	default:
+		return "UNKNOWN ";
+	}
+}
+#endif
+
 void dwc3_debugfs_init(struct dwc3 *dwc)
 {
 	struct dentry		*root;
diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
index f22714c..326f0e2 100644
--- a/drivers/usb/dwc3/trace.h
+++ b/drivers/usb/dwc3/trace.h
@@ -32,8 +32,11 @@ DECLARE_EVENT_CLASS(dwc3_log_io,
 		__entry->offset = offset;
 		__entry->value = value;
 	),
-	TP_printk("addr %p value %08x", __entry->base + __entry->offset,
-			__entry->value)
+	TP_printk("%saddr %p value %08x",
+		  dwc3_gadget_register_string(__entry->offset),
+		  __entry->base + __entry->offset,
+		  __entry->value
+	)
 );
 
 DEFINE_EVENT(dwc3_log_io, dwc3_readl,