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@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.
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@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
