When multiple DWC3 controllers are being used, trace events from
different instances get mixed up making debugging difficult as
there's no way to distinguish which instance generated the trace.
Use the register base address of dwc3 controller and append it to
trace events, so that the source instance is clearly identifiable.
Example trace output,
before -> dwc3_event: event (00000101): Reset [U0]
after -> dwc3_event: 0a600000: event (00000101): Reset [U0]
Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
---
drivers/usb/dwc3/core.c | 5 ++-
drivers/usb/dwc3/core.h | 2 +
drivers/usb/dwc3/ep0.c | 2 +-
drivers/usb/dwc3/gadget.c | 2 +-
drivers/usb/dwc3/io.h | 4 +-
drivers/usb/dwc3/trace.h | 88 ++++++++++++++++++++++++---------------
6 files changed, 65 insertions(+), 38 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 670a9d4bfff2..125616489e04 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -158,7 +158,7 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode, bool ignore_susphy)
dwc3_writel(dwc, DWC3_GCTL, reg);
dwc->current_dr_role = mode;
- trace_dwc3_set_prtcap(mode);
+ trace_dwc3_set_prtcap(dwc, mode);
}
EXPORT_SYMBOL_GPL(dwc3_set_prtcap);
@@ -2193,6 +2193,9 @@ int dwc3_core_probe(const struct dwc3_probe_data *data)
dwc_res = *res;
dwc_res.start += DWC3_GLOBALS_REGS_START;
+ /* Store the base register address for using it in traces later */
+ dwc->address = (u32)res->start;
+
if (dev->of_node) {
struct device_node *parent = of_get_parent(dev->of_node);
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 23188b910528..bc5ad3206d58 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1043,6 +1043,7 @@ struct dwc3_glue_ops {
* @gadget_max_speed: maximum gadget speed requested
* @gadget_ssp_rate: Gadget driver's maximum supported SuperSpeed Plus signaling
* rate and lane count.
+ * @address: Cached lower 32-bit base address to be used for logging.
* @ip: controller's ID
* @revision: controller's version of an IP
* @version_type: VERSIONTYPE register contents, a sub release of a revision
@@ -1258,6 +1259,7 @@ struct dwc3 {
enum usb_ssp_rate max_ssp_rate;
enum usb_ssp_rate gadget_ssp_rate;
+ u32 address;
u32 ip;
#define DWC3_IP 0x5533
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index a8ff8db610d3..bfe616194dfa 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -833,7 +833,7 @@ static void dwc3_ep0_inspect_setup(struct dwc3 *dwc,
if (!dwc->gadget_driver || !dwc->softconnect || !dwc->connected)
goto out;
- trace_dwc3_ctrl_req(ctrl);
+ trace_dwc3_ctrl_req(dwc, ctrl);
len = le16_to_cpu(ctrl->wLength);
if (!len) {
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9355c952c140..384963151ece 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -278,7 +278,7 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
status = -ETIMEDOUT;
}
- trace_dwc3_gadget_generic_cmd(cmd, param, status);
+ trace_dwc3_gadget_generic_cmd(dwc, cmd, param, status);
return ret;
}
diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
index 7dd0c1e0cf74..cad9a2ae1547 100644
--- a/drivers/usb/dwc3/io.h
+++ b/drivers/usb/dwc3/io.h
@@ -33,7 +33,7 @@ static inline u32 dwc3_readl(struct dwc3 *dwc, u32 offset)
* documentation, so we revert it back to the proper addresses, the
* same way they are described on SNPS documentation
*/
- trace_dwc3_readl(base - DWC3_GLOBALS_REGS_START, offset, value);
+ trace_dwc3_readl(dwc, base - DWC3_GLOBALS_REGS_START, offset, value);
return value;
}
@@ -54,7 +54,7 @@ static inline void dwc3_writel(struct dwc3 *dwc, u32 offset, u32 value)
* documentation, so we revert it back to the proper addresses, the
* same way they are described on SNPS documentation
*/
- trace_dwc3_writel(base - DWC3_GLOBALS_REGS_START, offset, value);
+ trace_dwc3_writel(dwc, base - DWC3_GLOBALS_REGS_START, offset, value);
}
#endif /* __DRIVERS_USB_DWC3_IO_H */
diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
index b6ba984bafcd..6c2c4990917a 100644
--- a/drivers/usb/dwc3/trace.h
+++ b/drivers/usb/dwc3/trace.h
@@ -20,63 +20,70 @@
#include "debug.h"
DECLARE_EVENT_CLASS(dwc3_log_set_prtcap,
- TP_PROTO(u32 mode),
- TP_ARGS(mode),
+ TP_PROTO(struct dwc3 *dwc, u32 mode),
+ TP_ARGS(dwc, mode),
TP_STRUCT__entry(
+ __field(u32, address)
__field(u32, mode)
),
TP_fast_assign(
+ __entry->address = dwc->address;
__entry->mode = mode;
),
- TP_printk("mode %s", dwc3_mode_string(__entry->mode))
+ TP_printk("%08x: mode %s", __entry->address, dwc3_mode_string(__entry->mode))
);
DEFINE_EVENT(dwc3_log_set_prtcap, dwc3_set_prtcap,
- TP_PROTO(u32 mode),
- TP_ARGS(mode)
+ TP_PROTO(struct dwc3 *dwc, u32 mode),
+ TP_ARGS(dwc, mode)
);
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, u32 offset, u32 value),
+ TP_ARGS(dwc, base, offset, value),
TP_STRUCT__entry(
+ __field(u32, address)
__field(void *, base)
__field(u32, offset)
__field(u32, value)
),
TP_fast_assign(
+ __entry->address = dwc->address;
__entry->base = base;
__entry->offset = offset;
__entry->value = value;
),
- TP_printk("addr %p offset %04x value %08x",
+ TP_printk("%08x: addr %p offset %04x value %08x",
+ __entry->address,
__entry->base + __entry->offset,
__entry->offset,
__entry->value)
);
DEFINE_EVENT(dwc3_log_io, dwc3_readl,
- TP_PROTO(void __iomem *base, u32 offset, u32 value),
- TP_ARGS(base, offset, value)
+ TP_PROTO(struct dwc3 *dwc, void __iomem *base, u32 offset, u32 value),
+ TP_ARGS(dwc, base, offset, value)
);
DEFINE_EVENT(dwc3_log_io, dwc3_writel,
- TP_PROTO(void __iomem *base, u32 offset, u32 value),
- TP_ARGS(base, offset, value)
+ TP_PROTO(struct dwc3 *dwc, void __iomem *base, u32 offset, u32 value),
+ TP_ARGS(dwc, base, offset, value)
);
DECLARE_EVENT_CLASS(dwc3_log_event,
TP_PROTO(u32 event, struct dwc3 *dwc),
TP_ARGS(event, dwc),
TP_STRUCT__entry(
+ __field(u32, address)
__field(u32, event)
__field(u32, ep0state)
),
TP_fast_assign(
+ __entry->address = dwc->address;
__entry->event = event;
__entry->ep0state = dwc->ep0state;
),
- TP_printk("event (%08x): %s", __entry->event,
+ TP_printk("%08x: event (%08x): %s", __entry->address, __entry->event,
dwc3_decode_event(__get_buf(DWC3_MSG_MAX), DWC3_MSG_MAX,
__entry->event, __entry->ep0state))
);
@@ -87,9 +94,10 @@ DEFINE_EVENT(dwc3_log_event, dwc3_event,
);
DECLARE_EVENT_CLASS(dwc3_log_ctrl,
- TP_PROTO(struct usb_ctrlrequest *ctrl),
- TP_ARGS(ctrl),
+ TP_PROTO(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl),
+ TP_ARGS(dwc, ctrl),
TP_STRUCT__entry(
+ __field(u32, address)
__field(__u8, bRequestType)
__field(__u8, bRequest)
__field(__u16, wValue)
@@ -97,13 +105,15 @@ DECLARE_EVENT_CLASS(dwc3_log_ctrl,
__field(__u16, wLength)
),
TP_fast_assign(
+ __entry->address = dwc->address;
__entry->bRequestType = ctrl->bRequestType;
__entry->bRequest = ctrl->bRequest;
__entry->wValue = le16_to_cpu(ctrl->wValue);
__entry->wIndex = le16_to_cpu(ctrl->wIndex);
__entry->wLength = le16_to_cpu(ctrl->wLength);
),
- TP_printk("%s", usb_decode_ctrl(__get_buf(DWC3_MSG_MAX), DWC3_MSG_MAX,
+ TP_printk("%08x: %s", __entry->address, usb_decode_ctrl(__get_buf(DWC3_MSG_MAX),
+ DWC3_MSG_MAX,
__entry->bRequestType,
__entry->bRequest, __entry->wValue,
__entry->wIndex, __entry->wLength)
@@ -111,14 +121,15 @@ DECLARE_EVENT_CLASS(dwc3_log_ctrl,
);
DEFINE_EVENT(dwc3_log_ctrl, dwc3_ctrl_req,
- TP_PROTO(struct usb_ctrlrequest *ctrl),
- TP_ARGS(ctrl)
+ TP_PROTO(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl),
+ TP_ARGS(dwc, ctrl)
);
DECLARE_EVENT_CLASS(dwc3_log_request,
TP_PROTO(struct dwc3_request *req),
TP_ARGS(req),
TP_STRUCT__entry(
+ __field(u32, address)
__string(name, req->dep->name)
__field(struct dwc3_request *, req)
__field(unsigned int, actual)
@@ -129,7 +140,7 @@ DECLARE_EVENT_CLASS(dwc3_log_request,
__field(int, no_interrupt)
),
TP_fast_assign(
- __assign_str(name);
+ __entry->address = req->dep->dwc->address;
__entry->req = req;
__entry->actual = req->request.actual;
__entry->length = req->request.length;
@@ -138,8 +149,10 @@ DECLARE_EVENT_CLASS(dwc3_log_request,
__entry->short_not_ok = req->request.short_not_ok;
__entry->no_interrupt = req->request.no_interrupt;
),
- TP_printk("%s: req %p length %u/%u %s%s%s ==> %d",
- __get_str(name), __entry->req, __entry->actual, __entry->length,
+ TP_printk("%08x: %s: req %p length %u/%u %s%s%s ==> %d",
+ __entry->address,
+ __get_str(name), __entry->req,
+ __entry->actual, __entry->length,
__entry->zero ? "Z" : "z",
__entry->short_not_ok ? "S" : "s",
__entry->no_interrupt ? "i" : "I",
@@ -173,28 +186,30 @@ DEFINE_EVENT(dwc3_log_request, dwc3_gadget_giveback,
);
DECLARE_EVENT_CLASS(dwc3_log_generic_cmd,
- TP_PROTO(unsigned int cmd, u32 param, int status),
- TP_ARGS(cmd, param, status),
+ TP_PROTO(struct dwc3 *dwc, unsigned int cmd, u32 param, int status),
+ TP_ARGS(dwc, cmd, param, status),
TP_STRUCT__entry(
+ __field(u32, address)
__field(unsigned int, cmd)
__field(u32, param)
__field(int, status)
),
TP_fast_assign(
+ __entry->address = dwc->address;
__entry->cmd = cmd;
__entry->param = param;
__entry->status = status;
),
- TP_printk("cmd '%s' [%x] param %08x --> status: %s",
- dwc3_gadget_generic_cmd_string(__entry->cmd),
+ TP_printk("%08x: cmd '%s' [%x] param %08x --> status: %s",
+ __entry->address, dwc3_gadget_generic_cmd_string(__entry->cmd),
__entry->cmd, __entry->param,
dwc3_gadget_generic_cmd_status_string(__entry->status)
)
);
DEFINE_EVENT(dwc3_log_generic_cmd, dwc3_gadget_generic_cmd,
- TP_PROTO(unsigned int cmd, u32 param, int status),
- TP_ARGS(cmd, param, status)
+ TP_PROTO(struct dwc3 *dwc, unsigned int cmd, u32 param, int status),
+ TP_ARGS(dwc, cmd, param, status)
);
DECLARE_EVENT_CLASS(dwc3_log_gadget_ep_cmd,
@@ -202,6 +217,7 @@ DECLARE_EVENT_CLASS(dwc3_log_gadget_ep_cmd,
struct dwc3_gadget_ep_cmd_params *params, int cmd_status),
TP_ARGS(dep, cmd, params, cmd_status),
TP_STRUCT__entry(
+ __field(u32, address)
__string(name, dep->name)
__field(unsigned int, cmd)
__field(u32, param0)
@@ -210,6 +226,7 @@ DECLARE_EVENT_CLASS(dwc3_log_gadget_ep_cmd,
__field(int, cmd_status)
),
TP_fast_assign(
+ __entry->address = dep->dwc->address;
__assign_str(name);
__entry->cmd = cmd;
__entry->param0 = params->param0;
@@ -217,8 +234,9 @@ DECLARE_EVENT_CLASS(dwc3_log_gadget_ep_cmd,
__entry->param2 = params->param2;
__entry->cmd_status = cmd_status;
),
- TP_printk("%s: cmd '%s' [%x] params %08x %08x %08x --> status: %s",
- __get_str(name), dwc3_gadget_ep_cmd_string(__entry->cmd),
+ TP_printk("%08x: %s: cmd '%s' [%x] params %08x %08x %08x --> status: %s",
+ __entry->address, __get_str(name),
+ dwc3_gadget_ep_cmd_string(__entry->cmd),
__entry->cmd, __entry->param0,
__entry->param1, __entry->param2,
dwc3_ep_cmd_status_string(__entry->cmd_status)
@@ -235,6 +253,7 @@ DECLARE_EVENT_CLASS(dwc3_log_trb,
TP_PROTO(struct dwc3_ep *dep, struct dwc3_trb *trb),
TP_ARGS(dep, trb),
TP_STRUCT__entry(
+ __field(u32, address)
__string(name, dep->name)
__field(struct dwc3_trb *, trb)
__field(u32, bpl)
@@ -246,6 +265,7 @@ DECLARE_EVENT_CLASS(dwc3_log_trb,
__field(u32, dequeue)
),
TP_fast_assign(
+ __entry->address = dep->dwc->address;
__assign_str(name);
__entry->trb = trb;
__entry->bpl = trb->bpl;
@@ -256,8 +276,8 @@ DECLARE_EVENT_CLASS(dwc3_log_trb,
__entry->enqueue = dep->trb_enqueue;
__entry->dequeue = dep->trb_dequeue;
),
- TP_printk("%s: trb %p (E%d:D%d) buf %08x%08x size %s%d ctrl %08x sofn %08x (%c%c%c%c:%c%c:%s)",
- __get_str(name), __entry->trb, __entry->enqueue,
+ TP_printk("%08x: %s: trb %p (E%d:D%d) buf %08x%08x size %s%d ctrl %08x sofn %08x (%c%c%c%c:%c%c:%s)",
+ __entry->address, __get_str(name), __entry->trb, __entry->enqueue,
__entry->dequeue, __entry->bph, __entry->bpl,
({char *s;
int pcm = ((__entry->size >> 24) & 3) + 1;
@@ -307,6 +327,7 @@ DECLARE_EVENT_CLASS(dwc3_log_ep,
TP_PROTO(struct dwc3_ep *dep),
TP_ARGS(dep),
TP_STRUCT__entry(
+ __field(u32, address)
__string(name, dep->name)
__field(unsigned int, maxpacket)
__field(unsigned int, maxpacket_limit)
@@ -318,6 +339,7 @@ DECLARE_EVENT_CLASS(dwc3_log_ep,
__field(u8, trb_dequeue)
),
TP_fast_assign(
+ __entry->address = dep->dwc->address;
__assign_str(name);
__entry->maxpacket = dep->endpoint.maxpacket;
__entry->maxpacket_limit = dep->endpoint.maxpacket_limit;
@@ -328,8 +350,8 @@ DECLARE_EVENT_CLASS(dwc3_log_ep,
__entry->trb_enqueue = dep->trb_enqueue;
__entry->trb_dequeue = dep->trb_dequeue;
),
- TP_printk("%s: mps %d/%d streams %d burst %d ring %d/%d flags %c:%c%c%c%c:%c",
- __get_str(name), __entry->maxpacket,
+ TP_printk("%08x: %s: mps %d/%d streams %d burst %d ring %d/%d flags %c:%c%c%c%c:%c",
+ __entry->address, __get_str(name), __entry->maxpacket,
__entry->maxpacket_limit, __entry->max_streams,
__entry->maxburst, __entry->trb_enqueue,
__entry->trb_dequeue,
--
2.34.1On Wed, Jan 14, 2026 at 03:37:48PM +0530, Prashanth K wrote:
> When multiple DWC3 controllers are being used, trace events from
> different instances get mixed up making debugging difficult as
> there's no way to distinguish which instance generated the trace.
>
> Use the register base address of dwc3 controller and append it to
> trace events, so that the source instance is clearly identifiable.
>
> Example trace output,
> before -> dwc3_event: event (00000101): Reset [U0]
> after -> dwc3_event: 0a600000: event (00000101): Reset [U0]
>
> Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
> ---
> drivers/usb/dwc3/core.c | 5 ++-
> drivers/usb/dwc3/core.h | 2 +
> drivers/usb/dwc3/ep0.c | 2 +-
> drivers/usb/dwc3/gadget.c | 2 +-
> drivers/usb/dwc3/io.h | 4 +-
> drivers/usb/dwc3/trace.h | 88 ++++++++++++++++++++++++---------------
> 6 files changed, 65 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 670a9d4bfff2..125616489e04 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -158,7 +158,7 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode, bool ignore_susphy)
> dwc3_writel(dwc, DWC3_GCTL, reg);
>
> dwc->current_dr_role = mode;
> - trace_dwc3_set_prtcap(mode);
> + trace_dwc3_set_prtcap(dwc, mode);
> }
> EXPORT_SYMBOL_GPL(dwc3_set_prtcap);
>
> @@ -2193,6 +2193,9 @@ int dwc3_core_probe(const struct dwc3_probe_data *data)
> dwc_res = *res;
> dwc_res.start += DWC3_GLOBALS_REGS_START;
>
> + /* Store the base register address for using it in traces later */
> + dwc->address = (u32)res->start;
And shouldn't this be dwc_res.start? Why ignore
DWC3_GLOBALS_REGS_START?
And because of that, shouldn't this go below:
> +
> if (dev->of_node) {
> struct device_node *parent = of_get_parent(dev->of_node);
That if statement, which will change the real resource start range
value?
thanks,
greg k-h
On Wed, Jan 14, 2026, Greg Kroah-Hartman wrote:
> On Wed, Jan 14, 2026 at 03:37:48PM +0530, Prashanth K wrote:
> > When multiple DWC3 controllers are being used, trace events from
> > different instances get mixed up making debugging difficult as
> > there's no way to distinguish which instance generated the trace.
> >
> > Use the register base address of dwc3 controller and append it to
> > trace events, so that the source instance is clearly identifiable.
> >
> > Example trace output,
> > before -> dwc3_event: event (00000101): Reset [U0]
> > after -> dwc3_event: 0a600000: event (00000101): Reset [U0]
> >
> > Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
> > ---
> > drivers/usb/dwc3/core.c | 5 ++-
> > drivers/usb/dwc3/core.h | 2 +
> > drivers/usb/dwc3/ep0.c | 2 +-
> > drivers/usb/dwc3/gadget.c | 2 +-
> > drivers/usb/dwc3/io.h | 4 +-
> > drivers/usb/dwc3/trace.h | 88 ++++++++++++++++++++++++---------------
> > 6 files changed, 65 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 670a9d4bfff2..125616489e04 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -158,7 +158,7 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode, bool ignore_susphy)
> > dwc3_writel(dwc, DWC3_GCTL, reg);
> >
> > dwc->current_dr_role = mode;
> > - trace_dwc3_set_prtcap(mode);
> > + trace_dwc3_set_prtcap(dwc, mode);
> > }
> > EXPORT_SYMBOL_GPL(dwc3_set_prtcap);
> >
> > @@ -2193,6 +2193,9 @@ int dwc3_core_probe(const struct dwc3_probe_data *data)
> > dwc_res = *res;
> > dwc_res.start += DWC3_GLOBALS_REGS_START;
> >
> > + /* Store the base register address for using it in traces later */
> > + dwc->address = (u32)res->start;
>
> And shouldn't this be dwc_res.start? Why ignore
> DWC3_GLOBALS_REGS_START?
No. That's where the global registers start. The base address of dwc3
doesn't start at the global registers offset.
>
> And because of that, shouldn't this go below:
No. See above.
>
> > +
> > if (dev->of_node) {
> > struct device_node *parent = of_get_parent(dev->of_node);
>
> That if statement, which will change the real resource start range
> value?
>
BR,
Thinh
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? thanks, greg k-h
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. BR, Thinh
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. BR, Thinh
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 :) I'd prefer that, instead of saving off a value that you can look up if you need it. thanks, greg k-h
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? Thanks, Thinh
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. Regards, Prashanth K
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?
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)
__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
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
On Mon, Jan 19, 2026, Prashanth K wrote: > On 1/16/2026 11:21 PM, Thinh Nguyen wrote: > > 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'. > It seems more fitting to use phys_addr_t here for printing the address. I'd use resource_size_t when calculating for the resource size. Let me know if there's an issue. BR, Thinh
© 2016 - 2026 Red Hat, Inc.