Hi, On Thu, Jan 12, 2023, Linyu Yuan wrote: > > On 1/11/2023 3:21 PM, Jun Li (OSS) wrote: > > > > > -----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? > > > i didn't check it. how about you ? no matter what is good HW or bad HW, > current code define a big number. > > > actually i think we should not discuss is it a good number or not. > > what is purpose to use status register record ? do it give you any > information to understand the IP behavior ? > While some polling numbers seem large, we should not remove the tracing events during polling. There are useful info in the status register when there's a timeout. Also, the number of polls needed for certain state change is useful data point for debug. What we may want to update is not depend on the register read delay for the polling duration. Different setup will have different delay. If we want to disable it for debugging purpose, make sure to have the default option as enabled. As for the inconsistent/large polling count, we can review and revisit the change Li Jun did a while ago to use readl_poll_timeout_atomic instead: https://lore.kernel.org/linux-usb/87blmymwlz.fsf@xxxxxxxxxx/T/#t Thanks, Thinh > > > > > > > > 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
