| Message ID | 1672996895-22762-2-git-send-email-quic_linyyuan@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] usb: dwc3: simplify operation in dwc3_readl() and dwc3_writel() | expand |
> -----Original Message----- > From: Linyu Yuan <quic_linyyuan@quicinc.com> > Sent: Friday, January 6, 2023 5:22 PM > To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Thinh Nguyen > <Thinh.Nguyen@synopsys.com> > Cc: linux-usb@vger.kernel.org; Jack Pham <quic_jackp@quicinc.com>; Wesley > Cheng <quic_wcheng@quicinc.com>; Linyu Yuan <quic_linyyuan@quicinc.com> > 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? > 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. Li Jun > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > --- > 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
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@quicinc.com> > > > Sent: Monday, January 9, 2023 11:42 AM > > > To: Jun Li (OSS) <jun.li@oss.nxp.com>; Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org>; Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > > Cc: linux-usb@vger.kernel.org; Jack Pham <quic_jackp@quicinc.com>; Wesley > > > Cheng <quic_wcheng@quicinc.com> > > > 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@quicinc.com> > > > > > Sent: Friday, January 6, 2023 5:22 PM > > > > > To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Thinh Nguyen > > > > > <Thinh.Nguyen@synopsys.com> > > > > > Cc: linux-usb@vger.kernel.org; Jack Pham <quic_jackp@quicinc.com>; > > > > > Wesley Cheng <quic_wcheng@quicinc.com>; Linyu Yuan > > > > > <quic_linyyuan@quicinc.com> > > > > > 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@kernel.org/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@quicinc.com> > > > > > --- > > > > > 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
On 1/13/2023 9:08 AM, Thinh Nguyen wrote: > 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@quicinc.com> >>>> Sent: Monday, January 9, 2023 11:42 AM >>>> To: Jun Li (OSS) <jun.li@oss.nxp.com>; Greg Kroah-Hartman >>>> <gregkh@linuxfoundation.org>; Thinh Nguyen <Thinh.Nguyen@synopsys.com> >>>> Cc: linux-usb@vger.kernel.org; Jack Pham <quic_jackp@quicinc.com>; Wesley >>>> Cheng <quic_wcheng@quicinc.com> >>>> 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@quicinc.com> >>>>>> Sent: Friday, January 6, 2023 5:22 PM >>>>>> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Thinh Nguyen >>>>>> <Thinh.Nguyen@synopsys.com> >>>>>> Cc: linux-usb@vger.kernel.org; Jack Pham <quic_jackp@quicinc.com>; >>>>>> Wesley Cheng <quic_wcheng@quicinc.com>; Linyu Yuan >>>>>> <quic_linyyuan@quicinc.com> >>>>>> 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. if so, do you accept define a new function and new trace event like, static inline u32 dwc3_readl_timeout(void __iomem *base, u32 offset) DEFINE_EVENT(dwc3_log_io, dwc3_readl_timeout, TP_PROTO(void __iomem *base, u32 offset, u32 value), TP_ARGS(base, offset, value) ); let user enable it accordingly. > > 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@kernel.org/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@quicinc.com> >>>>>> --- >>>>>> 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
On Fri, Jan 13, 2023, Linyu Yuan wrote: > > On 1/13/2023 9:08 AM, Thinh Nguyen wrote: > > 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@quicinc.com> > > > > > Sent: Monday, January 9, 2023 11:42 AM > > > > > To: Jun Li (OSS) <jun.li@oss.nxp.com>; Greg Kroah-Hartman > > > > > <gregkh@linuxfoundation.org>; Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > > > > Cc: linux-usb@vger.kernel.org; Jack Pham <quic_jackp@quicinc.com>; Wesley > > > > > Cheng <quic_wcheng@quicinc.com> > > > > > 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@quicinc.com> > > > > > > > Sent: Friday, January 6, 2023 5:22 PM > > > > > > > To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Thinh Nguyen > > > > > > > <Thinh.Nguyen@synopsys.com> > > > > > > > Cc: linux-usb@vger.kernel.org; Jack Pham <quic_jackp@quicinc.com>; > > > > > > > Wesley Cheng <quic_wcheng@quicinc.com>; Linyu Yuan > > > > > > > <quic_linyyuan@quicinc.com> > > > > > > > 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. > > > if so, do you accept define a new function and new trace event like, > > static inline u32 dwc3_readl_timeout(void __iomem *base, u32 offset) > > DEFINE_EVENT(dwc3_log_io, dwc3_readl_timeout, > TP_PROTO(void __iomem *base, u32 offset, u32 value), > TP_ARGS(base, offset, value) > ); > > let user enable it accordingly. > We can just redefine the event with an additional parameter for event filtering option. Make sure not to change dwc3_readl() declaration so that the patch doesn't touch every part of the driver. BR, Thinh > > > > > 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://urldefense.com/v3/__https://lore.kernel.org/linux-usb/87blmymwlz.fsf@kernel.org/T/*t__;Iw!!A4F2R9G_pg!eZBcW78zLMkOtfPIhxNvIGjItv_W2IVvpLyOV-3eCrWRk7T1PVERSmz037HLx1nLOHgOsRTK1Cz8DHFeBXZ21WwY5OkJlw$ > > > > Thanks, > > Thinh > >
On 1/13/2023 11:18 AM, Thinh Nguyen wrote: > On Fri, Jan 13, 2023, Linyu Yuan wrote: >> On 1/13/2023 9:08 AM, Thinh Nguyen wrote: >>> 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@quicinc.com> >>>>>> Sent: Monday, January 9, 2023 11:42 AM >>>>>> To: Jun Li (OSS) <jun.li@oss.nxp.com>; Greg Kroah-Hartman >>>>>> <gregkh@linuxfoundation.org>; Thinh Nguyen <Thinh.Nguyen@synopsys.com> >>>>>> Cc: linux-usb@vger.kernel.org; Jack Pham <quic_jackp@quicinc.com>; Wesley >>>>>> Cheng <quic_wcheng@quicinc.com> >>>>>> 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@quicinc.com> >>>>>>>> Sent: Friday, January 6, 2023 5:22 PM >>>>>>>> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Thinh Nguyen >>>>>>>> <Thinh.Nguyen@synopsys.com> >>>>>>>> Cc: linux-usb@vger.kernel.org; Jack Pham <quic_jackp@quicinc.com>; >>>>>>>> Wesley Cheng <quic_wcheng@quicinc.com>; Linyu Yuan >>>>>>>> <quic_linyyuan@quicinc.com> >>>>>>>> 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. >> >> if so, do you accept define a new function and new trace event like, >> >> static inline u32 dwc3_readl_timeout(void __iomem *base, u32 offset) >> >> DEFINE_EVENT(dwc3_log_io, dwc3_readl_timeout, >> TP_PROTO(void __iomem *base, u32 offset, u32 value), >> TP_ARGS(base, offset, value) >> ); >> >> let user enable it accordingly. >> > We can just redefine the event with an additional parameter for event > filtering option. actually filtering option only work at event output time, it will show data match filter, ignore data which not match. there is still no filter which run before event save buffer, event still save into buffer, if read happen too many times, it will flush useful event like write register operation. > > Make sure not to change dwc3_readl() declaration so that the patch > doesn't touch every part of the driver. > > BR, > Thinh > >>> 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://urldefense.com/v3/__https://lore.kernel.org/linux-usb/87blmymwlz.fsf@kernel.org/T/*t__;Iw!!A4F2R9G_pg!eZBcW78zLMkOtfPIhxNvIGjItv_W2IVvpLyOV-3eCrWRk7T1PVERSmz037HLx1nLOHgOsRTK1Cz8DHFeBXZ21WwY5OkJlw$ >>> >>> Thanks, >>> Thinh
On Fri, Jan 13, 2023, Linyu Yuan wrote: > > On 1/13/2023 11:18 AM, Thinh Nguyen wrote: > > On Fri, Jan 13, 2023, Linyu Yuan wrote: > > > On 1/13/2023 9:08 AM, Thinh Nguyen wrote: > > > > 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@quicinc.com> > > > > > > > Sent: Monday, January 9, 2023 11:42 AM > > > > > > > To: Jun Li (OSS) <jun.li@oss.nxp.com>; Greg Kroah-Hartman > > > > > > > <gregkh@linuxfoundation.org>; Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > > > > > > Cc: linux-usb@vger.kernel.org; Jack Pham <quic_jackp@quicinc.com>; Wesley > > > > > > > Cheng <quic_wcheng@quicinc.com> > > > > > > > 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@quicinc.com> > > > > > > > > > Sent: Friday, January 6, 2023 5:22 PM > > > > > > > > > To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Thinh Nguyen > > > > > > > > > <Thinh.Nguyen@synopsys.com> > > > > > > > > > Cc: linux-usb@vger.kernel.org; Jack Pham <quic_jackp@quicinc.com>; > > > > > > > > > Wesley Cheng <quic_wcheng@quicinc.com>; Linyu Yuan > > > > > > > > > <quic_linyyuan@quicinc.com> > > > > > > > > > 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. > > > > > > if so, do you accept define a new function and new trace event like, > > > > > > static inline u32 dwc3_readl_timeout(void __iomem *base, u32 offset) > > > > > > DEFINE_EVENT(dwc3_log_io, dwc3_readl_timeout, > > > TP_PROTO(void __iomem *base, u32 offset, u32 value), > > > TP_ARGS(base, offset, value) > > > ); > > > > > > let user enable it accordingly. > > > > > We can just redefine the event with an additional parameter for event > > filtering option. > > > actually filtering option only work at event output time, it will show data > match filter, ignore data which not match. > > there is still no filter which run before event save buffer, event still > save into buffer, > > if read happen too many times, it will flush useful event like write > register operation. > Ok. What do you think of also reviving Jun's change to using readl_poll_timeout_atomic? It makes sense to create a new trace event in addition to that: https://lore.kernel.org/linux-usb/87blmymwlz.fsf@kernel.org/T/#t Thanks, Thinh > > > > > Make sure not to change dwc3_readl() declaration so that the patch > > doesn't touch every part of the driver. > > > > BR, > > Thinh > > > > > > 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://urldefense.com/v3/__https://lore.kernel.org/linux-usb/87blmymwlz.fsf@kernel.org/T/*t__;Iw!!A4F2R9G_pg!eZBcW78zLMkOtfPIhxNvIGjItv_W2IVvpLyOV-3eCrWRk7T1PVERSmz037HLx1nLOHgOsRTK1Cz8DHFeBXZ21WwY5OkJlw$ > > > > > > > > Thanks, > > > > Thinh
On 1/13/2023 1:24 PM, Thinh Nguyen wrote: > >>>>> 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. >>>> if so, do you accept define a new function and new trace event like, >>>> >>>> static inline u32 dwc3_readl_timeout(void __iomem *base, u32 offset) >>>> >>>> DEFINE_EVENT(dwc3_log_io, dwc3_readl_timeout, >>>> TP_PROTO(void __iomem *base, u32 offset, u32 value), >>>> TP_ARGS(base, offset, value) >>>> ); >>>> >>>> let user enable it accordingly. >>>> >>> We can just redefine the event with an additional parameter for event >>> filtering option. >> >> actually filtering option only work at event output time, it will show data >> match filter, ignore data which not match. >> >> there is still no filter which run before event save buffer, event still >> save into buffer, >> >> if read happen too many times, it will flush useful event like write >> register operation. >> > Ok. What do you think of also reviving Jun's change to using > readl_poll_timeout_atomic? It makes sense to create a new trace event > in addition to that: > https://lore.kernel.org/linux-usb/87blmymwlz.fsf@kernel.org/T/#t 1.if you review all places which read in this way, not all of them can convert to readl_poll_timeout_atomic() Jun introduce. 2. also his change block for more than two years, will it be approved ? > > Thanks, > Thinh > > Thinh
On Fri, Jan 13, 2023, Linyu Yuan wrote: > > On 1/13/2023 1:24 PM, Thinh Nguyen wrote: > > > > > > > > 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. > > > > > if so, do you accept define a new function and new trace event like, > > > > > > > > > > static inline u32 dwc3_readl_timeout(void __iomem *base, u32 offset) > > > > > > > > > > DEFINE_EVENT(dwc3_log_io, dwc3_readl_timeout, > > > > > TP_PROTO(void __iomem *base, u32 offset, u32 value), > > > > > TP_ARGS(base, offset, value) > > > > > ); > > > > > > > > > > let user enable it accordingly. > > > > > > > > > We can just redefine the event with an additional parameter for event > > > > filtering option. > > > > > > actually filtering option only work at event output time, it will show data > > > match filter, ignore data which not match. > > > > > > there is still no filter which run before event save buffer, event still > > > save into buffer, > > > > > > if read happen too many times, it will flush useful event like write > > > register operation. > > > > > Ok. What do you think of also reviving Jun's change to using > > readl_poll_timeout_atomic? It makes sense to create a new trace event > > in addition to that: > > https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/87blmymwlz.fsf@kernel.org/T/*t__;Iw!!A4F2R9G_pg!bgeN4Mp9hv2b33BpJ7QRAXAdm-vgafgurxXP-YSdQsdWMZkNFXnWV4qXNT4iCTz_0mLMHylw6Z84J5IVPYZnroj5eiNQnw$ > > > 1.if you review all places which read in this way, not all of them can > convert to > > readl_poll_timeout_atomic() Jun introduce. > which ones? > > 2. also his change block for more than two years, will it be approved ? > Previously this was done to resolve a separate issue. It was a combination of a fix and an enhancement that touched on different areas of the driver. It's better to keep them separated. IMHO it's a good change. It's better to keep the polling duration clear and determinable. BR, Thinh
On 1/14/2023 7:16 AM, Thinh Nguyen wrote: > On Fri, Jan 13, 2023, Linyu Yuan wrote: >> On 1/13/2023 1:24 PM, Thinh Nguyen wrote: >>>>>>> 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. >>>>>> if so, do you accept define a new function and new trace event like, >>>>>> >>>>>> static inline u32 dwc3_readl_timeout(void __iomem *base, u32 offset) >>>>>> >>>>>> DEFINE_EVENT(dwc3_log_io, dwc3_readl_timeout, >>>>>> TP_PROTO(void __iomem *base, u32 offset, u32 value), >>>>>> TP_ARGS(base, offset, value) >>>>>> ); >>>>>> >>>>>> let user enable it accordingly. >>>>>> >>>>> We can just redefine the event with an additional parameter for event >>>>> filtering option. >>>> actually filtering option only work at event output time, it will show data >>>> match filter, ignore data which not match. >>>> >>>> there is still no filter which run before event save buffer, event still >>>> save into buffer, >>>> >>>> if read happen too many times, it will flush useful event like write >>>> register operation. >>>> >>> Ok. What do you think of also reviving Jun's change to using >>> readl_poll_timeout_atomic? It makes sense to create a new trace event >>> in addition to that: >>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/87blmymwlz.fsf@kernel.org/T/*t__;Iw!!A4F2R9G_pg!bgeN4Mp9hv2b33BpJ7QRAXAdm-vgafgurxXP-YSdQsdWMZkNFXnWV4qXNT4iCTz_0mLMHylw6Z84J5IVPYZnroj5eiNQnw$ >> >> 1.if you review all places which read in this way, not all of them can >> convert to >> >> readl_poll_timeout_atomic() Jun introduce. >> > which ones? I try it before, but if strictly follow original code logic, it is hard to convert all of them, e.g. dwc3_ulpi_busyloop(). but if you prefer read_poll_timeout_atomic() implementation, @jun can you review my change and covert all places into it ? > >> 2. also his change block for more than two years, will it be approved ? >> > Previously this was done to resolve a separate issue. It was a > combination of a fix and an enhancement that touched on different areas > of the driver. It's better to keep them separated. > > IMHO it's a good change. It's better to keep the polling duration clear > and determinable. @Jun can you review this comment and update a new patch ? > > BR, > Thinh
On Wed, Jan 18, 2023, Linyu Yuan wrote: > > On 1/14/2023 7:16 AM, Thinh Nguyen wrote: > > On Fri, Jan 13, 2023, Linyu Yuan wrote: > > > On 1/13/2023 1:24 PM, Thinh Nguyen wrote: > > > > > > > > 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. > > > > > > > if so, do you accept define a new function and new trace event like, > > > > > > > > > > > > > > static inline u32 dwc3_readl_timeout(void __iomem *base, u32 offset) > > > > > > > > > > > > > > DEFINE_EVENT(dwc3_log_io, dwc3_readl_timeout, > > > > > > > TP_PROTO(void __iomem *base, u32 offset, u32 value), > > > > > > > TP_ARGS(base, offset, value) > > > > > > > ); > > > > > > > > > > > > > > let user enable it accordingly. > > > > > > > > > > > > > We can just redefine the event with an additional parameter for event > > > > > > filtering option. > > > > > actually filtering option only work at event output time, it will show data > > > > > match filter, ignore data which not match. > > > > > > > > > > there is still no filter which run before event save buffer, event still > > > > > save into buffer, > > > > > > > > > > if read happen too many times, it will flush useful event like write > > > > > register operation. > > > > > > > > > Ok. What do you think of also reviving Jun's change to using > > > > readl_poll_timeout_atomic? It makes sense to create a new trace event > > > > in addition to that: > > > > https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/87blmymwlz.fsf@kernel.org/T/*t__;Iw!!A4F2R9G_pg!bgeN4Mp9hv2b33BpJ7QRAXAdm-vgafgurxXP-YSdQsdWMZkNFXnWV4qXNT4iCTz_0mLMHylw6Z84J5IVPYZnroj5eiNQnw$ > > > > > > 1.if you review all places which read in this way, not all of them can > > > convert to > > > > > > readl_poll_timeout_atomic() Jun introduce. > > > > > which ones? > > > I try it before, but if strictly follow original code logic, it is hard to > convert all of them, e.g. dwc3_ulpi_busyloop(). > What's the problem with that one? Also, we're not exactly following the original code logic. The current logic doesn't ensure consistent/determinate polling duration between different setup, and keeping it consistent is a good thing. Just need to make sure to use the appropriate atomic/non-atomic version of the readl_poll_timeout where it can sleep or not. Thanks, Thinh
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();
There are multiple places which will retry up to 10000 times to read a register, 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. Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> --- 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(-)