Hi Felipe,
On 11/27/2018 11:16 PM, Felipe Balbi wrote:
> Hi,
>
> Tejas Joglekar <tejas.joglekar@xxxxxxxxxxxx> writes:
>> From: Thinh Nguyen <thinhn@xxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxx>
>> Signed-off-by: Tejas Joglekar <joglekar@xxxxxxxxxxxx>
>> ---
>> 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
