usb: dwc3: debug: Print register name

[Felipe Balbi <balbi@kernel.org>]

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