Re: [PATCH v4 3/3] usb: dwc3: Log dwc3 address in traces

[Prashanth K <prashanth.k@oss.qualcomm.com>]

On 1/16/2026 11:21 PM, Thinh Nguyen wrote:
> On Fri, Jan 16, 2026, Prashanth K wrote:
>>
>>
>> On 1/15/2026 9:52 PM, Thinh Nguyen wrote:
>>> On Thu, Jan 15, 2026, Greg Kroah-Hartman wrote:
>>>> On Wed, Jan 14, 2026 at 11:54:03PM +0000, Thinh Nguyen wrote:
>>>>> On Wed, Jan 14, 2026, Thinh Nguyen wrote:
>>>>>> On Wed, Jan 14, 2026, Greg Kroah-Hartman wrote:
>>>>>>> On Wed, Jan 14, 2026 at 03:37:48PM +0530, Prashanth K wrote:
>>>>>>>> + * @address: Cached lower 32-bit base address to be used for logging.
>>>>>>>
>>>>>>> Why are 32bits enough / ok?  Why not use the full 64 that you really
>>>>>>> have?  What happens if you have 2 devices with just the upper 32 bits
>>>>>>> different?
>>>>>>>
>>>>>>> This is a resource value, so why not use the proper type for it?
>>>>>>>
>>>>>>
>>>>>> This is only intented to be used for logging, so I suggested to use u32.
>>>>>> I want to avoid treating this struct member as a phys_addr_t where it
>>>>>> may be misused.
>>>>>>
>>>>>> As for the reason to capture only the lower 32-bit, it's just base on
>>>>>> what I've seen so far. That I have not seen designs where the 2 or more
>>>>>> instances are placed that far apart and share the same lower 32-bit.
>>>>>> It's a bit nicer to shorten the address print at the start of a
>>>>>> tracepoint. But if it's insufficient, there's no problem with using
>>>>>> 64-bit.
>>>>>>
>>>>>
>>>>> Or we can just remove this and print the address from
>>>>> dwc->xhci_resources[0].start.
>>>>
>>>> I thought I asked for that a few revisions ago :)
>>>
>>> Ah, I missed that.
>>>
>>>>
>>>> I'd prefer that, instead of saving off a value that you can look up if
>>>> you need it.
>>>>
>>>
>>> Yes, this is better.
>>>
>>> Hi Prashanth, can we just use dwc->xhci_resources[0].start instead?
>>>
>>
>> While its true that we can avoid adding new variable into dwc3 struct,
>> using 'xhci_resources[0].start' in DWC3 core traces can be confusing for
>> someones reading code, since all of the traces are related to dwc3 core
>> and gadget.
>>
> 
> We can name the fast assign field in tracing to base_address. For those
> who do not have access to the databook to know that that's where base
> address is, if needed, we can also add a comment there.
> 
> Would something like this work for you?
> 

Yea I'll make the change and send next version.

> diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
> index b6ba984bafcd..8e5d474fd54a 100644
> --- a/drivers/usb/dwc3/trace.h
> +++ b/drivers/usb/dwc3/trace.h
> @@ -37,19 +37,22 @@ DEFINE_EVENT(dwc3_log_set_prtcap, dwc3_set_prtcap,
>  );
>  
>  DECLARE_EVENT_CLASS(dwc3_log_io,
> -	TP_PROTO(void *base, u32 offset, u32 value),
> -	TP_ARGS(base, offset, value),
> +	TP_PROTO(struct dwc3 *dwc, void *base, u31 offset, u32 value),
> +	TP_ARGS(dwc, base, offset, value),
>  	TP_STRUCT__entry(
> +		__field(phy_addr_t, base_address)

Shouldn't we be using 'resource_size_t' instead ? Anyways its just
typedef of 'phys_addr_t'.

>  		__field(void *, base)
>  		__field(u32, offset)
>  		__field(u32, value)
>  	),
>  	TP_fast_assign(
> +		__entry->base_address = dwc->xhci_resources[0].start;
>  		__entry->base = base;
>  		__entry->offset = offset;
>  		__entry->value = value;
>  	),
> -	TP_printk("addr %p offset %04x value %08x",
> +	TP_printk("%pa: addr %p offset %04x value %08x",
> +		dwc->base_address,
>  		__entry->base + __entry->offset,
>  		__entry->offset,
>  		__entry->value)
> 
> Thanks,
> Thinh