Re: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function

[Linyu Yuan <quic_linyyuan@xxxxxxxxxxx>]

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 ?
> > 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 ?
> 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