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

Linyu Yuan <quic_linyyuan@xxxxxxxxxxx> · Thu, 12 Jan 2023 09:56:27 +0800

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 ?



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






