Message-ID: <20250912215444.egh5vdgvwbvqs3my@synopsys.com>
Date: Fri, 12 Sep 2025 21:54:49 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Prashanth K <prashanth.k@....qualcomm.com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: dwc3: Log dwc3 instance name in traces

On Thu, Sep 11, 2025, Prashanth K wrote:
> 
> 
> On 9/11/2025 7:06 AM, Thinh Nguyen wrote:
> > On Tue, Sep 09, 2025, Prashanth K wrote:
> >>
> >>
> >> On 9/5/2025 5:14 AM, Thinh Nguyen wrote:
> >>> On Thu, Sep 04, 2025, Prashanth K wrote:
> >>>>
> >>>>
> >>>> On 9/4/2025 5:30 AM, Thinh Nguyen wrote:
> >>>>> On Wed, Sep 03, 2025, Prashanth K wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 9/3/2025 5:14 AM, Thinh Nguyen wrote:
> >>>>>>> On Mon, Sep 01, 2025, Prashanth K wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 8/29/2025 4:18 AM, Thinh Nguyen wrote:
> >>>>>>>>> Hi,
> >>>>>>>>>
> >>>>>>>>> On Mon, Aug 25, 2025, 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.
> >>>>>>>>>>
> >>>>>>>>>> Append the device name to trace events to clearly identify the
> >>>>>>>>>> source instance.
> >>>>>>>>>
> >>>>>>>>> Can we print the base address instead of the device name? This will be
> >>>>>>>>> consistent across different device names, and it will be easier to
> >>>>>>>>> create filter.
> >>>>>>>>>
> >>>>>>>> Did you mean to print the iomem (base address) directly?
> >>>>>>>> I think using device name is more readable, in most cases device name
> >>>>>>>> would contain the base address also. Let me know if you are pointing to
> >>>>>>>> something else.>>
> >>>>>>>
> >>>>>>> Yes, I mean the device base address. PCI devices won't have the base
> >>>>>>> address as part of the device name.
> >>>>>>>
> >>>>>> But the base address (void __iomem *base) wouldn't be helpful.
> >>>>>> Using the base address, i guess we would be able to differentiate the
> >>>>>> traces when there are multiple instances, but it wouldn't help us
> >>>>>> identify which controller instance generated which trace.
> >>>>>>
> >>>>>> And for PCI devices, i agree that it doesn't have  address in device
> >>>>>> name, but i think we should be able to identify the correct instance
> >>>>>> based on the bus/device numbers, right ?
> >>>>>>
> >>>>>
> >>>>> We may not have the PCI domain numbers if it's a child device as in the
> >>>>> case of dwc3-pci or dwc3-haps.
> >>>>>
> >>>>> The base address _does_ tell you exactly which device the tracepoints
> >>>>> correspond to. The device name is inconsistent between different device
> >>>>> types and only relevant if we have access to the system to know which
> >>>>> name belongs to which instance.
> >>>>
> >>>> Yes, I agree that device name would be inconsistent for different for
> >>>> PCI (and HAPS) devices. But IMO using base address (virtual) would just
> >>>> make it more harder to read and identify the instance.
> >>>>
> >>>> Perhaps we can cache the register addr and use it, what do you think?
> >>>> Here we can at least differentiate the instances based on HW addr.
> >>>>
> >>>> snprintf(dwc->inst, sizeof(dwc->inst), "0x%08llx", (unsigned long
> >>>> long)res->start);
> >>>> dev_info(dwc->dev, "addr:%s\n", dwc->inst);
> >>>>
> >>>> Output --> [    4.521746] dwc3 a600000.usb: addr:0x0a600000
> >>>
> >>> I think there's some misunderstanding here. I refer the base address as
> >>> the hardware address.
> >>>
> >>> I prefer something like this:
> >>>
> >>>     dwc3_event: 0a600000: event (00000101): Reset [U0]
> >>>
> >>> instead of the device name like this:
> >>>
> >>>    dwc3_event: a600000.usb: event (00000101): Reset [U0]
> >>>
> >>> BR,
> >>> Thinh
> >>
> >> Initially I was also talking about HW address, but since we were
> >> discussing this under dwc3_readl/writel functions context, i also got
> >> confused whether you are pointing out the HW address or virtual address.
> >>
> >> Anyways, i guess the above method using snprintf on res->start is one
> >> way to get base address, is there any way to do this?
> >>
> > 
> > You're right. I forgot that we can't do virt_to_phys() for ioremapped
> > resource...
> > 
> > In that case, can we pass the dwc3 structure in the dwc3_readl/writel? I
> > know there are many places that this change may touch, but I feel that
> > it's easier to read than creating the new macro dwc3_readl/writel.
> > 
> > Thanks,
> > Thinh
> 
> 1) Passing dwc3 structure to dwc3_readl/writel will need changes in
> around 250 places, we can do that using 'find and replace'.

Yikes..

> 
> 2) OR we can use container_of(base, struct dwc3, regs)) to get dwc pointer,
> this will not work in few places where we use dep->regs (~6 places). we
> can just create a separate function dwc3_dep_readl/writel for dep->regs,

We can just update the endpoint macros to something like this:
#define DWC3_DEPCMD(n)		(DWC3_DEP_BASE(n) + 0x0c)

so we can do this:
	dwc3_readl(dwc->regs, DWC3_DEPCMD(dep->number));

We will get the proper endpoint offset, and we can also remove the dep->regs.

> and get dwc3 from dep. This will have lesser number of changes, and less
> impact on git history.
> 
> I'm more inclined towards approach2, but fine with both approaches, let
> me know which one suits here.
> 
> We can use snprintf on res->start to get the HW addr and store that
> string into dwc3. Is that fine?
> 

Option 2 sounds good.

Thanks!
Thinh

Powered by blists - more mailing lists