> -----Original Message-----
> From: Linyu Yuan <quic_linyyuan@xxxxxxxxxxx>
> Sent: Monday, January 9, 2023 11:42 AM
> To: Jun Li (OSS) <jun.li@xxxxxxxxxxx>; Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx>; Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>
> Cc: linux-usb@xxxxxxxxxxxxxxx; Jack Pham <quic_jackp@xxxxxxxxxxx>; Wesley
> Cheng <quic_wcheng@xxxxxxxxxxx>
> Subject: Re: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
>
>
> On 1/9/2023 11:33 AM, Jun Li (OSS) wrote:
> >
> >> -----Original Message-----
> >> From: Linyu Yuan <quic_linyyuan@xxxxxxxxxxx>
> >> Sent: Friday, January 6, 2023 5:22 PM
> >> To: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Thinh Nguyen
> >> <Thinh.Nguyen@xxxxxxxxxxxx>
> >> Cc: linux-usb@xxxxxxxxxxxxxxx; Jack Pham <quic_jackp@xxxxxxxxxxx>;
> >> Wesley Cheng <quic_wcheng@xxxxxxxxxxx>; Linyu Yuan
> >> <quic_linyyuan@xxxxxxxxxxx>
> >> Subject: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
> >>
> >> There are multiple places which will retry up to 10000 times to read
> >> a register,
> > It's "up to", I think at normal case, it's a small number, if a large
> > Iteration number is observed, probably there is something wrong should
> > be checked?
> do you mean the original loop count can be reduced ?
No. I mean the max loop number is not a problem at good HW.
What's the actual loop number on you HW?
> >
> >> when trace event enable, it is not good as all events may show same data.
> >>
> >> Add dwc3_readl_notrace() function which will not save trace event
> >> when read register.
> > Simply drop part of register read is not good, this cause the final io
> > trace of dwc3 is not complete, I think a full/complete foot print of
> > io register read/write should be kept.
> if you prefer save them, is there a better solution ?
If the actual loop number is not a problem, then I think current
trace is just fine.
Li Jun
> >
> > Li Jun
> >
> >> Signed-off-by: Linyu Yuan <quic_linyyuan@xxxxxxxxxxx>
> >> ---
> >> drivers/usb/dwc3/core.c | 2 +-
> >> drivers/usb/dwc3/gadget.c | 12 ++++++------
> >> drivers/usb/dwc3/io.h | 10 ++++++++++
> >> drivers/usb/dwc3/ulpi.c | 2 +-
> >> 4 files changed, 18 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
> >> 476b636..550bb23 100644
> >> --- a/drivers/usb/dwc3/core.c
> >> +++ b/drivers/usb/dwc3/core.c
> >> @@ -297,7 +297,7 @@ int dwc3_core_soft_reset(struct dwc3 *dwc)
> >> retries = 10;
> >>
> >> do {
> >> - reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> >> + reg = dwc3_readl_notrace(dwc->regs, DWC3_DCTL);
> >> if (!(reg & DWC3_DCTL_CSFTRST))
> >> goto done;
> >>
> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> >> index
> >> 7899765..f2126f24 100644
> >> --- a/drivers/usb/dwc3/gadget.c
> >> +++ b/drivers/usb/dwc3/gadget.c
> >> @@ -97,7 +97,7 @@ int dwc3_gadget_set_link_state(struct dwc3 *dwc,
> >> enum dwc3_link_state state)
> >> */
> >> if (!DWC3_VER_IS_PRIOR(DWC3, 194A)) {
> >> while (--retries) {
> >> - reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> >> + reg = dwc3_readl_notrace(dwc->regs, DWC3_DSTS);
> >> if (reg & DWC3_DSTS_DCNRD)
> >> udelay(5);
> >> else
> >> @@ -128,7 +128,7 @@ int dwc3_gadget_set_link_state(struct dwc3 *dwc,
> >> enum dwc3_link_state state)
> >> /* wait for a change in DSTS */
> >> retries = 10000;
> >> while (--retries) {
> >> - reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> >> + reg = dwc3_readl_notrace(dwc->regs, DWC3_DSTS);
> >>
> >> if (DWC3_DSTS_USBLNKST(reg) == state)
> >> return 0;
> >> @@ -239,7 +239,7 @@ int dwc3_send_gadget_generic_command(struct dwc3
> >> *dwc, unsigned int cmd,
> >> dwc3_writel(dwc->regs, DWC3_DGCMD, cmd | DWC3_DGCMD_CMDACT);
> >>
> >> do {
> >> - reg = dwc3_readl(dwc->regs, DWC3_DGCMD);
> >> + reg = dwc3_readl_notrace(dwc->regs, DWC3_DGCMD);
> >> if (!(reg & DWC3_DGCMD_CMDACT)) {
> >> status = DWC3_DGCMD_STATUS(reg);
> >> if (status)
> >> @@ -375,7 +375,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep,
> >> unsigned int cmd,
> >> }
> >>
> >> do {
> >> - reg = dwc3_readl(dep->regs, DWC3_DEPCMD);
> >> + reg = dwc3_readl_notrace(dep->regs, DWC3_DEPCMD);
> >> if (!(reg & DWC3_DEPCMD_CMDACT)) {
> >> cmd_status = DWC3_DEPCMD_STATUS(reg);
> >>
> >> @@ -2324,7 +2324,7 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
> >> retries = 20000;
> >>
> >> while (retries--) {
> >> - reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> >> + reg = dwc3_readl_notrace(dwc->regs, DWC3_DSTS);
> >>
> >> /* in HS, means ON */
> >> if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U0) @@ -2510,7
> >> +2510,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int
> >> +is_on, int
> >> suspend)
> >>
> >> do {
> >> usleep_range(1000, 2000);
> >> - reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> >> + reg = dwc3_readl_notrace(dwc->regs, DWC3_DSTS);
> >> reg &= DWC3_DSTS_DEVCTRLHLT;
> >> } while (--timeout && !(!is_on ^ !reg));
> >>
> >> diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h index
> >> d9283f4..d24513e 100644
> >> --- a/drivers/usb/dwc3/io.h
> >> +++ b/drivers/usb/dwc3/io.h
> >> @@ -37,6 +37,16 @@ static inline u32 dwc3_readl(void __iomem *base,
> >> u32
> >> offset)
> >> return value;
> >> }
> >>
> >> +static inline u32 dwc3_readl_notrace(void __iomem *base, u32 offset)
> {
> >> + /*
> >> + * We requested the mem region starting from the Globals address
> >> + * space, see dwc3_probe in core.c.
> >> + * The offsets are also given starting from Globals address space.
> >> + */
> >> + return readl(base + offset);
> >> +}
> >> +
> >> static inline void dwc3_writel(void __iomem *base, u32 offset, u32
> >> value) {
> >> /*
> >> diff --git a/drivers/usb/dwc3/ulpi.c b/drivers/usb/dwc3/ulpi.c index
> >> f23f4c9..7b220b0 100644
> >> --- a/drivers/usb/dwc3/ulpi.c
> >> +++ b/drivers/usb/dwc3/ulpi.c
> >> @@ -39,7 +39,7 @@ static int dwc3_ulpi_busyloop(struct dwc3 *dwc, u8
> >> addr, bool read)
> >>
> >> while (count--) {
> >> ndelay(ns);
> >> - reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYACC(0));
> >> + reg = dwc3_readl_notrace(dwc->regs, DWC3_GUSB2PHYACC(0));
> >> if (reg & DWC3_GUSB2PHYACC_DONE)
> >> return 0;
> >> cpu_relax();
> >> --
> >> 2.7.4
