* [PATCH 1/3] usb: dwc3: simplify operation in dwc3_readl() and dwc3_writel()
@ 2023-01-06 9:21 Linyu Yuan
2023-01-06 9:21 ` [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function Linyu Yuan
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Linyu Yuan @ 2023-01-06 9:21 UTC (permalink / raw)
To: Greg Kroah-Hartman, Thinh Nguyen
Cc: linux-usb, Jack Pham, Wesley Cheng, Linyu Yuan
when dwc3_readl() read register and dwc3_writel() write register,
it will run operation 'base + offset - DWC3_GLOBALS_REGS_START' to
calculate register address, seem the minus operation can avoid.
the original register definition is offset from XHCI base 0x0,
now change it to offset from DWC3_GLOBALS_REGS_START(0xc100).
Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
drivers/usb/dwc3/core.h | 150 ++++++++++++++++++++++-----------------------
drivers/usb/dwc3/debugfs.c | 2 +-
drivers/usb/dwc3/io.h | 12 ++--
3 files changed, 82 insertions(+), 82 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 8f9959b..3af244e 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -85,90 +85,90 @@
#define DWC3_OTG_REGS_END 0xccff
/* Global Registers */
-#define DWC3_GSBUSCFG0 0xc100
-#define DWC3_GSBUSCFG1 0xc104
-#define DWC3_GTXTHRCFG 0xc108
-#define DWC3_GRXTHRCFG 0xc10c
-#define DWC3_GCTL 0xc110
-#define DWC3_GEVTEN 0xc114
-#define DWC3_GSTS 0xc118
-#define DWC3_GUCTL1 0xc11c
-#define DWC3_GSNPSID 0xc120
-#define DWC3_GGPIO 0xc124
-#define DWC3_GUID 0xc128
-#define DWC3_GUCTL 0xc12c
-#define DWC3_GBUSERRADDR0 0xc130
-#define DWC3_GBUSERRADDR1 0xc134
-#define DWC3_GPRTBIMAP0 0xc138
-#define DWC3_GPRTBIMAP1 0xc13c
-#define DWC3_GHWPARAMS0 0xc140
-#define DWC3_GHWPARAMS1 0xc144
-#define DWC3_GHWPARAMS2 0xc148
-#define DWC3_GHWPARAMS3 0xc14c
-#define DWC3_GHWPARAMS4 0xc150
-#define DWC3_GHWPARAMS5 0xc154
-#define DWC3_GHWPARAMS6 0xc158
-#define DWC3_GHWPARAMS7 0xc15c
-#define DWC3_GDBGFIFOSPACE 0xc160
-#define DWC3_GDBGLTSSM 0xc164
-#define DWC3_GDBGBMU 0xc16c
-#define DWC3_GDBGLSPMUX 0xc170
-#define DWC3_GDBGLSP 0xc174
-#define DWC3_GDBGEPINFO0 0xc178
-#define DWC3_GDBGEPINFO1 0xc17c
-#define DWC3_GPRTBIMAP_HS0 0xc180
-#define DWC3_GPRTBIMAP_HS1 0xc184
-#define DWC3_GPRTBIMAP_FS0 0xc188
-#define DWC3_GPRTBIMAP_FS1 0xc18c
-#define DWC3_GUCTL2 0xc19c
-
-#define DWC3_VER_NUMBER 0xc1a0
-#define DWC3_VER_TYPE 0xc1a4
-
-#define DWC3_GUSB2PHYCFG(n) (0xc200 + ((n) * 0x04))
-#define DWC3_GUSB2I2CCTL(n) (0xc240 + ((n) * 0x04))
-
-#define DWC3_GUSB2PHYACC(n) (0xc280 + ((n) * 0x04))
-
-#define DWC3_GUSB3PIPECTL(n) (0xc2c0 + ((n) * 0x04))
-
-#define DWC3_GTXFIFOSIZ(n) (0xc300 + ((n) * 0x04))
-#define DWC3_GRXFIFOSIZ(n) (0xc380 + ((n) * 0x04))
-
-#define DWC3_GEVNTADRLO(n) (0xc400 + ((n) * 0x10))
-#define DWC3_GEVNTADRHI(n) (0xc404 + ((n) * 0x10))
-#define DWC3_GEVNTSIZ(n) (0xc408 + ((n) * 0x10))
-#define DWC3_GEVNTCOUNT(n) (0xc40c + ((n) * 0x10))
-
-#define DWC3_GHWPARAMS8 0xc600
-#define DWC3_GUCTL3 0xc60c
-#define DWC3_GFLADJ 0xc630
-#define DWC3_GHWPARAMS9 0xc6e0
+#define DWC3_GSBUSCFG0 0x0000
+#define DWC3_GSBUSCFG1 0x0004
+#define DWC3_GTXTHRCFG 0x0008
+#define DWC3_GRXTHRCFG 0x000c
+#define DWC3_GCTL 0x0010
+#define DWC3_GEVTEN 0x0014
+#define DWC3_GSTS 0x0018
+#define DWC3_GUCTL1 0x001c
+#define DWC3_GSNPSID 0x0020
+#define DWC3_GGPIO 0x0024
+#define DWC3_GUID 0x0028
+#define DWC3_GUCTL 0x002c
+#define DWC3_GBUSERRADDR0 0x0030
+#define DWC3_GBUSERRADDR1 0x0034
+#define DWC3_GPRTBIMAP0 0x0038
+#define DWC3_GPRTBIMAP1 0x003c
+#define DWC3_GHWPARAMS0 0x0040
+#define DWC3_GHWPARAMS1 0x0044
+#define DWC3_GHWPARAMS2 0x0048
+#define DWC3_GHWPARAMS3 0x004c
+#define DWC3_GHWPARAMS4 0x0050
+#define DWC3_GHWPARAMS5 0x0054
+#define DWC3_GHWPARAMS6 0x0058
+#define DWC3_GHWPARAMS7 0x005c
+#define DWC3_GDBGFIFOSPACE 0x0060
+#define DWC3_GDBGLTSSM 0x0064
+#define DWC3_GDBGBMU 0x006c
+#define DWC3_GDBGLSPMUX 0x0070
+#define DWC3_GDBGLSP 0x0074
+#define DWC3_GDBGEPINFO0 0x0078
+#define DWC3_GDBGEPINFO1 0x007c
+#define DWC3_GPRTBIMAP_HS0 0x0080
+#define DWC3_GPRTBIMAP_HS1 0x0084
+#define DWC3_GPRTBIMAP_FS0 0x0088
+#define DWC3_GPRTBIMAP_FS1 0x008c
+#define DWC3_GUCTL2 0x009c
+
+#define DWC3_VER_NUMBER 0x00a0
+#define DWC3_VER_TYPE 0x00a4
+
+#define DWC3_GUSB2PHYCFG(n) (0x0100 + ((n) * 0x04))
+#define DWC3_GUSB2I2CCTL(n) (0x0140 + ((n) * 0x04))
+
+#define DWC3_GUSB2PHYACC(n) (0x0180 + ((n) * 0x04))
+
+#define DWC3_GUSB3PIPECTL(n) (0x01c0 + ((n) * 0x04))
+
+#define DWC3_GTXFIFOSIZ(n) (0x0200 + ((n) * 0x04))
+#define DWC3_GRXFIFOSIZ(n) (0x0280 + ((n) * 0x04))
+
+#define DWC3_GEVNTADRLO(n) (0x0300 + ((n) * 0x10))
+#define DWC3_GEVNTADRHI(n) (0x0304 + ((n) * 0x10))
+#define DWC3_GEVNTSIZ(n) (0x0308 + ((n) * 0x10))
+#define DWC3_GEVNTCOUNT(n) (0x030c + ((n) * 0x10))
+
+#define DWC3_GHWPARAMS8 0x0500
+#define DWC3_GUCTL3 0x050c
+#define DWC3_GFLADJ 0x0530
+#define DWC3_GHWPARAMS9 0x05e0
/* Device Registers */
-#define DWC3_DCFG 0xc700
-#define DWC3_DCTL 0xc704
-#define DWC3_DEVTEN 0xc708
-#define DWC3_DSTS 0xc70c
-#define DWC3_DGCMDPAR 0xc710
-#define DWC3_DGCMD 0xc714
-#define DWC3_DALEPENA 0xc720
-#define DWC3_DCFG1 0xc740 /* DWC_usb32 only */
-
-#define DWC3_DEP_BASE(n) (0xc800 + ((n) * 0x10))
+#define DWC3_DCFG 0x0600
+#define DWC3_DCTL 0x0604
+#define DWC3_DEVTEN 0x0608
+#define DWC3_DSTS 0x060c
+#define DWC3_DGCMDPAR 0x0610
+#define DWC3_DGCMD 0x0614
+#define DWC3_DALEPENA 0x0620
+#define DWC3_DCFG1 0x0640 /* DWC_usb32 only */
+
+#define DWC3_DEP_BASE(n) (0x0700 + ((n) * 0x10))
#define DWC3_DEPCMDPAR2 0x00
#define DWC3_DEPCMDPAR1 0x04
#define DWC3_DEPCMDPAR0 0x08
#define DWC3_DEPCMD 0x0c
-#define DWC3_DEV_IMOD(n) (0xca00 + ((n) * 0x4))
+#define DWC3_DEV_IMOD(n) (0x0900 + ((n) * 0x4))
/* OTG Registers */
-#define DWC3_OCFG 0xcc00
-#define DWC3_OCTL 0xcc04
-#define DWC3_OEVT 0xcc08
-#define DWC3_OEVTEN 0xcc0C
-#define DWC3_OSTS 0xcc10
+#define DWC3_OCFG 0x0b00
+#define DWC3_OCTL 0x0b04
+#define DWC3_OEVT 0x0b08
+#define DWC3_OEVTEN 0x0b0C
+#define DWC3_OSTS 0x0b10
/* Bit fields */
diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index f2b7675..ea6c0e4 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -908,7 +908,7 @@ void dwc3_debugfs_init(struct dwc3 *dwc)
dwc->regset->regs = dwc3_regs;
dwc->regset->nregs = ARRAY_SIZE(dwc3_regs);
- dwc->regset->base = dwc->regs - DWC3_GLOBALS_REGS_START;
+ dwc->regset->base = dwc->regs;
root = debugfs_create_dir(dev_name(dwc->dev), usb_debug_root);
debugfs_create_regset32("regdump", 0444, root, dwc->regset);
diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
index 1e96ea3..d9283f4 100644
--- a/drivers/usb/dwc3/io.h
+++ b/drivers/usb/dwc3/io.h
@@ -23,16 +23,16 @@ static inline u32 dwc3_readl(void __iomem *base, u32 offset)
/*
* We requested the mem region starting from the Globals address
* space, see dwc3_probe in core.c.
- * However, the offsets are given starting from xHCI address space.
+ * The offsets are also given starting from Globals address space.
*/
- value = readl(base + offset - DWC3_GLOBALS_REGS_START);
+ value = readl(base + offset);
/*
* When tracing we want to make it easy to find the correct address on
* documentation, so we revert it back to the proper addresses, the
* same way they are described on SNPS documentation
*/
- trace_dwc3_readl(base - DWC3_GLOBALS_REGS_START, offset, value);
+ trace_dwc3_readl(base, offset, value);
return value;
}
@@ -42,16 +42,16 @@ static inline void dwc3_writel(void __iomem *base, u32 offset, u32 value)
/*
* We requested the mem region starting from the Globals address
* space, see dwc3_probe in core.c.
- * However, the offsets are given starting from xHCI address space.
+ * The offsets are also given starting from Globals address space.
*/
- writel(value, base + offset - DWC3_GLOBALS_REGS_START);
+ writel(value, base + offset);
/*
* When tracing we want to make it easy to find the correct address on
* documentation, so we revert it back to the proper addresses, the
* same way they are described on SNPS documentation
*/
- trace_dwc3_writel(base - DWC3_GLOBALS_REGS_START, offset, value);
+ trace_dwc3_writel(base, offset, value);
}
#endif /* __DRIVERS_USB_DWC3_IO_H */
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
2023-01-06 9:21 [PATCH 1/3] usb: dwc3: simplify operation in dwc3_readl() and dwc3_writel() Linyu Yuan
@ 2023-01-06 9:21 ` Linyu Yuan
2023-01-09 3:33 ` Jun Li (OSS)
2023-01-06 9:21 ` [PATCH 3/3] usb: dwc3: remove base parameter of event class dwc3_log_io Linyu Yuan
2023-01-09 3:02 ` [PATCH 1/3] usb: dwc3: simplify operation in dwc3_readl() and dwc3_writel() Jun Li (OSS)
2 siblings, 1 reply; 23+ messages in thread
From: Linyu Yuan @ 2023-01-06 9:21 UTC (permalink / raw)
To: Greg Kroah-Hartman, Thinh Nguyen
Cc: linux-usb, Jack Pham, Wesley Cheng, Linyu Yuan
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(-)
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/3] usb: dwc3: remove base parameter of event class dwc3_log_io
2023-01-06 9:21 [PATCH 1/3] usb: dwc3: simplify operation in dwc3_readl() and dwc3_writel() Linyu Yuan
2023-01-06 9:21 ` [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function Linyu Yuan
@ 2023-01-06 9:21 ` Linyu Yuan
2023-01-09 18:47 ` Thinh Nguyen
2023-01-09 3:02 ` [PATCH 1/3] usb: dwc3: simplify operation in dwc3_readl() and dwc3_writel() Jun Li (OSS)
2 siblings, 1 reply; 23+ messages in thread
From: Linyu Yuan @ 2023-01-06 9:21 UTC (permalink / raw)
To: Greg Kroah-Hartman, Thinh Nguyen
Cc: linux-usb, Jack Pham, Wesley Cheng, Linyu Yuan
when trace register read/write operation, it is not necessary to show
virtual address cacaulate from base and offset.
remove the base register value, it will save trace buffer.
it is enough only save and show the offset.
Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
drivers/usb/dwc3/io.h | 4 ++--
drivers/usb/dwc3/trace.h | 17 +++++++----------
2 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
index d24513e..fcb5726 100644
--- a/drivers/usb/dwc3/io.h
+++ b/drivers/usb/dwc3/io.h
@@ -32,7 +32,7 @@ static inline u32 dwc3_readl(void __iomem *base, u32 offset)
* documentation, so we revert it back to the proper addresses, the
* same way they are described on SNPS documentation
*/
- trace_dwc3_readl(base, offset, value);
+ trace_dwc3_readl(offset, value);
return value;
}
@@ -61,7 +61,7 @@ static inline void dwc3_writel(void __iomem *base, u32 offset, u32 value)
* documentation, so we revert it back to the proper addresses, the
* same way they are described on SNPS documentation
*/
- trace_dwc3_writel(base, offset, value);
+ trace_dwc3_writel(offset, value);
}
#endif /* __DRIVERS_USB_DWC3_IO_H */
diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
index 1975aec..536b9a1 100644
--- a/drivers/usb/dwc3/trace.h
+++ b/drivers/usb/dwc3/trace.h
@@ -20,32 +20,29 @@
#include "debug.h"
DECLARE_EVENT_CLASS(dwc3_log_io,
- TP_PROTO(void *base, u32 offset, u32 value),
- TP_ARGS(base, offset, value),
+ TP_PROTO(u32 offset, u32 value),
+ TP_ARGS(offset, value),
TP_STRUCT__entry(
- __field(void *, base)
__field(u32, offset)
__field(u32, value)
),
TP_fast_assign(
- __entry->base = base;
__entry->offset = offset;
__entry->value = value;
),
- TP_printk("addr %p offset %04x value %08x",
- __entry->base + __entry->offset,
+ TP_printk("offset %04x value %08x",
__entry->offset,
__entry->value)
);
DEFINE_EVENT(dwc3_log_io, dwc3_readl,
- TP_PROTO(void __iomem *base, u32 offset, u32 value),
- TP_ARGS(base, offset, value)
+ TP_PROTO(u32 offset, u32 value),
+ TP_ARGS(offset, value)
);
DEFINE_EVENT(dwc3_log_io, dwc3_writel,
- TP_PROTO(void __iomem *base, u32 offset, u32 value),
- TP_ARGS(base, offset, value)
+ TP_PROTO(u32 offset, u32 value),
+ TP_ARGS(offset, value)
);
DECLARE_EVENT_CLASS(dwc3_log_event,
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* RE: [PATCH 1/3] usb: dwc3: simplify operation in dwc3_readl() and dwc3_writel()
2023-01-06 9:21 [PATCH 1/3] usb: dwc3: simplify operation in dwc3_readl() and dwc3_writel() Linyu Yuan
2023-01-06 9:21 ` [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function Linyu Yuan
2023-01-06 9:21 ` [PATCH 3/3] usb: dwc3: remove base parameter of event class dwc3_log_io Linyu Yuan
@ 2023-01-09 3:02 ` Jun Li (OSS)
2023-01-09 3:09 ` Linyu Yuan
2 siblings, 1 reply; 23+ messages in thread
From: Jun Li (OSS) @ 2023-01-09 3:02 UTC (permalink / raw)
To: Linyu Yuan, Greg Kroah-Hartman, Thinh Nguyen
Cc: linux-usb, Jack Pham, Wesley Cheng
> -----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 1/3] usb: dwc3: simplify operation in dwc3_readl() and
> dwc3_writel()
>
> when dwc3_readl() read register and dwc3_writel() write register,
> it will run operation 'base + offset - DWC3_GLOBALS_REGS_START' to
> calculate register address, seem the minus operation can avoid.
>
> the original register definition is offset from XHCI base 0x0,
> now change it to offset from DWC3_GLOBALS_REGS_START(0xc100).
Is this really can bring benefit? With this change user has to takes
care an offset, looks to me the original definition is very straightforward,
use the offset defined in DWC3 Databook, user can directly check each register
definition by offset (not just by name).
Li Jun
>
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> drivers/usb/dwc3/core.h | 150
> ++++++++++++++++++++++-----------------------
> drivers/usb/dwc3/debugfs.c | 2 +-
> drivers/usb/dwc3/io.h | 12 ++--
> 3 files changed, 82 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 8f9959b..3af244e 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -85,90 +85,90 @@
> #define DWC3_OTG_REGS_END 0xccff
>
> /* Global Registers */
> -#define DWC3_GSBUSCFG0 0xc100
> -#define DWC3_GSBUSCFG1 0xc104
> -#define DWC3_GTXTHRCFG 0xc108
> -#define DWC3_GRXTHRCFG 0xc10c
> -#define DWC3_GCTL 0xc110
> -#define DWC3_GEVTEN 0xc114
> -#define DWC3_GSTS 0xc118
> -#define DWC3_GUCTL1 0xc11c
> -#define DWC3_GSNPSID 0xc120
> -#define DWC3_GGPIO 0xc124
> -#define DWC3_GUID 0xc128
> -#define DWC3_GUCTL 0xc12c
> -#define DWC3_GBUSERRADDR0 0xc130
> -#define DWC3_GBUSERRADDR1 0xc134
> -#define DWC3_GPRTBIMAP0 0xc138
> -#define DWC3_GPRTBIMAP1 0xc13c
> -#define DWC3_GHWPARAMS0 0xc140
> -#define DWC3_GHWPARAMS1 0xc144
> -#define DWC3_GHWPARAMS2 0xc148
> -#define DWC3_GHWPARAMS3 0xc14c
> -#define DWC3_GHWPARAMS4 0xc150
> -#define DWC3_GHWPARAMS5 0xc154
> -#define DWC3_GHWPARAMS6 0xc158
> -#define DWC3_GHWPARAMS7 0xc15c
> -#define DWC3_GDBGFIFOSPACE 0xc160
> -#define DWC3_GDBGLTSSM 0xc164
> -#define DWC3_GDBGBMU 0xc16c
> -#define DWC3_GDBGLSPMUX 0xc170
> -#define DWC3_GDBGLSP 0xc174
> -#define DWC3_GDBGEPINFO0 0xc178
> -#define DWC3_GDBGEPINFO1 0xc17c
> -#define DWC3_GPRTBIMAP_HS0 0xc180
> -#define DWC3_GPRTBIMAP_HS1 0xc184
> -#define DWC3_GPRTBIMAP_FS0 0xc188
> -#define DWC3_GPRTBIMAP_FS1 0xc18c
> -#define DWC3_GUCTL2 0xc19c
> -
> -#define DWC3_VER_NUMBER 0xc1a0
> -#define DWC3_VER_TYPE 0xc1a4
> -
> -#define DWC3_GUSB2PHYCFG(n) (0xc200 + ((n) * 0x04))
> -#define DWC3_GUSB2I2CCTL(n) (0xc240 + ((n) * 0x04))
> -
> -#define DWC3_GUSB2PHYACC(n) (0xc280 + ((n) * 0x04))
> -
> -#define DWC3_GUSB3PIPECTL(n) (0xc2c0 + ((n) * 0x04))
> -
> -#define DWC3_GTXFIFOSIZ(n) (0xc300 + ((n) * 0x04))
> -#define DWC3_GRXFIFOSIZ(n) (0xc380 + ((n) * 0x04))
> -
> -#define DWC3_GEVNTADRLO(n) (0xc400 + ((n) * 0x10))
> -#define DWC3_GEVNTADRHI(n) (0xc404 + ((n) * 0x10))
> -#define DWC3_GEVNTSIZ(n) (0xc408 + ((n) * 0x10))
> -#define DWC3_GEVNTCOUNT(n) (0xc40c + ((n) * 0x10))
> -
> -#define DWC3_GHWPARAMS8 0xc600
> -#define DWC3_GUCTL3 0xc60c
> -#define DWC3_GFLADJ 0xc630
> -#define DWC3_GHWPARAMS9 0xc6e0
> +#define DWC3_GSBUSCFG0 0x0000
> +#define DWC3_GSBUSCFG1 0x0004
> +#define DWC3_GTXTHRCFG 0x0008
> +#define DWC3_GRXTHRCFG 0x000c
> +#define DWC3_GCTL 0x0010
> +#define DWC3_GEVTEN 0x0014
> +#define DWC3_GSTS 0x0018
> +#define DWC3_GUCTL1 0x001c
> +#define DWC3_GSNPSID 0x0020
> +#define DWC3_GGPIO 0x0024
> +#define DWC3_GUID 0x0028
> +#define DWC3_GUCTL 0x002c
> +#define DWC3_GBUSERRADDR0 0x0030
> +#define DWC3_GBUSERRADDR1 0x0034
> +#define DWC3_GPRTBIMAP0 0x0038
> +#define DWC3_GPRTBIMAP1 0x003c
> +#define DWC3_GHWPARAMS0 0x0040
> +#define DWC3_GHWPARAMS1 0x0044
> +#define DWC3_GHWPARAMS2 0x0048
> +#define DWC3_GHWPARAMS3 0x004c
> +#define DWC3_GHWPARAMS4 0x0050
> +#define DWC3_GHWPARAMS5 0x0054
> +#define DWC3_GHWPARAMS6 0x0058
> +#define DWC3_GHWPARAMS7 0x005c
> +#define DWC3_GDBGFIFOSPACE 0x0060
> +#define DWC3_GDBGLTSSM 0x0064
> +#define DWC3_GDBGBMU 0x006c
> +#define DWC3_GDBGLSPMUX 0x0070
> +#define DWC3_GDBGLSP 0x0074
> +#define DWC3_GDBGEPINFO0 0x0078
> +#define DWC3_GDBGEPINFO1 0x007c
> +#define DWC3_GPRTBIMAP_HS0 0x0080
> +#define DWC3_GPRTBIMAP_HS1 0x0084
> +#define DWC3_GPRTBIMAP_FS0 0x0088
> +#define DWC3_GPRTBIMAP_FS1 0x008c
> +#define DWC3_GUCTL2 0x009c
> +
> +#define DWC3_VER_NUMBER 0x00a0
> +#define DWC3_VER_TYPE 0x00a4
> +
> +#define DWC3_GUSB2PHYCFG(n) (0x0100 + ((n) * 0x04))
> +#define DWC3_GUSB2I2CCTL(n) (0x0140 + ((n) * 0x04))
> +
> +#define DWC3_GUSB2PHYACC(n) (0x0180 + ((n) * 0x04))
> +
> +#define DWC3_GUSB3PIPECTL(n) (0x01c0 + ((n) * 0x04))
> +
> +#define DWC3_GTXFIFOSIZ(n) (0x0200 + ((n) * 0x04))
> +#define DWC3_GRXFIFOSIZ(n) (0x0280 + ((n) * 0x04))
> +
> +#define DWC3_GEVNTADRLO(n) (0x0300 + ((n) * 0x10))
> +#define DWC3_GEVNTADRHI(n) (0x0304 + ((n) * 0x10))
> +#define DWC3_GEVNTSIZ(n) (0x0308 + ((n) * 0x10))
> +#define DWC3_GEVNTCOUNT(n) (0x030c + ((n) * 0x10))
> +
> +#define DWC3_GHWPARAMS8 0x0500
> +#define DWC3_GUCTL3 0x050c
> +#define DWC3_GFLADJ 0x0530
> +#define DWC3_GHWPARAMS9 0x05e0
>
> /* Device Registers */
> -#define DWC3_DCFG 0xc700
> -#define DWC3_DCTL 0xc704
> -#define DWC3_DEVTEN 0xc708
> -#define DWC3_DSTS 0xc70c
> -#define DWC3_DGCMDPAR 0xc710
> -#define DWC3_DGCMD 0xc714
> -#define DWC3_DALEPENA 0xc720
> -#define DWC3_DCFG1 0xc740 /* DWC_usb32 only */
> -
> -#define DWC3_DEP_BASE(n) (0xc800 + ((n) * 0x10))
> +#define DWC3_DCFG 0x0600
> +#define DWC3_DCTL 0x0604
> +#define DWC3_DEVTEN 0x0608
> +#define DWC3_DSTS 0x060c
> +#define DWC3_DGCMDPAR 0x0610
> +#define DWC3_DGCMD 0x0614
> +#define DWC3_DALEPENA 0x0620
> +#define DWC3_DCFG1 0x0640 /* DWC_usb32 only */
> +
> +#define DWC3_DEP_BASE(n) (0x0700 + ((n) * 0x10))
> #define DWC3_DEPCMDPAR2 0x00
> #define DWC3_DEPCMDPAR1 0x04
> #define DWC3_DEPCMDPAR0 0x08
> #define DWC3_DEPCMD 0x0c
>
> -#define DWC3_DEV_IMOD(n) (0xca00 + ((n) * 0x4))
> +#define DWC3_DEV_IMOD(n) (0x0900 + ((n) * 0x4))
>
> /* OTG Registers */
> -#define DWC3_OCFG 0xcc00
> -#define DWC3_OCTL 0xcc04
> -#define DWC3_OEVT 0xcc08
> -#define DWC3_OEVTEN 0xcc0C
> -#define DWC3_OSTS 0xcc10
> +#define DWC3_OCFG 0x0b00
> +#define DWC3_OCTL 0x0b04
> +#define DWC3_OEVT 0x0b08
> +#define DWC3_OEVTEN 0x0b0C
> +#define DWC3_OSTS 0x0b10
>
> /* Bit fields */
>
> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> index f2b7675..ea6c0e4 100644
> --- a/drivers/usb/dwc3/debugfs.c
> +++ b/drivers/usb/dwc3/debugfs.c
> @@ -908,7 +908,7 @@ void dwc3_debugfs_init(struct dwc3 *dwc)
>
> dwc->regset->regs = dwc3_regs;
> dwc->regset->nregs = ARRAY_SIZE(dwc3_regs);
> - dwc->regset->base = dwc->regs - DWC3_GLOBALS_REGS_START;
> + dwc->regset->base = dwc->regs;
>
> root = debugfs_create_dir(dev_name(dwc->dev), usb_debug_root);
> debugfs_create_regset32("regdump", 0444, root, dwc->regset);
> diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
> index 1e96ea3..d9283f4 100644
> --- a/drivers/usb/dwc3/io.h
> +++ b/drivers/usb/dwc3/io.h
> @@ -23,16 +23,16 @@ static inline u32 dwc3_readl(void __iomem *base, u32
> offset)
> /*
> * We requested the mem region starting from the Globals address
> * space, see dwc3_probe in core.c.
> - * However, the offsets are given starting from xHCI address space.
> + * The offsets are also given starting from Globals address space.
> */
> - value = readl(base + offset - DWC3_GLOBALS_REGS_START);
> + value = readl(base + offset);
>
> /*
> * When tracing we want to make it easy to find the correct address on
> * documentation, so we revert it back to the proper addresses, the
> * same way they are described on SNPS documentation
> */
> - trace_dwc3_readl(base - DWC3_GLOBALS_REGS_START, offset, value);
> + trace_dwc3_readl(base, offset, value);
>
> return value;
> }
> @@ -42,16 +42,16 @@ static inline void dwc3_writel(void __iomem *base, u32
> offset, u32 value)
> /*
> * We requested the mem region starting from the Globals address
> * space, see dwc3_probe in core.c.
> - * However, the offsets are given starting from xHCI address space.
> + * The offsets are also given starting from Globals address space.
> */
> - writel(value, base + offset - DWC3_GLOBALS_REGS_START);
> + writel(value, base + offset);
>
> /*
> * When tracing we want to make it easy to find the correct address on
> * documentation, so we revert it back to the proper addresses, the
> * same way they are described on SNPS documentation
> */
> - trace_dwc3_writel(base - DWC3_GLOBALS_REGS_START, offset, value);
> + trace_dwc3_writel(base, offset, value);
> }
>
> #endif /* __DRIVERS_USB_DWC3_IO_H */
> --
> 2.7.4
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/3] usb: dwc3: simplify operation in dwc3_readl() and dwc3_writel()
2023-01-09 3:02 ` [PATCH 1/3] usb: dwc3: simplify operation in dwc3_readl() and dwc3_writel() Jun Li (OSS)
@ 2023-01-09 3:09 ` Linyu Yuan
2023-01-09 4:54 ` Linyu Yuan
0 siblings, 1 reply; 23+ messages in thread
From: Linyu Yuan @ 2023-01-09 3:09 UTC (permalink / raw)
To: Jun Li (OSS), Greg Kroah-Hartman, Thinh Nguyen
Cc: linux-usb, Jack Pham, Wesley Cheng
On 1/9/2023 11:02 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 1/3] usb: dwc3: simplify operation in dwc3_readl() and
>> dwc3_writel()
>>
>> when dwc3_readl() read register and dwc3_writel() write register,
>> it will run operation 'base + offset - DWC3_GLOBALS_REGS_START' to
>> calculate register address, seem the minus operation can avoid.
>>
>> the original register definition is offset from XHCI base 0x0,
>> now change it to offset from DWC3_GLOBALS_REGS_START(0xc100).
> Is this really can bring benefit? With this change user has to takes
I didn't check all compiler generated code and if more instruction
needed for original code.
> care an offset, looks to me the original definition is very straightforward,
> use the offset defined in DWC3 Databook, user can directly check each register
> definition by offset (not just by name).
this is good point, let me check one compile code and make a decision to
remove this change or not.
>
> Li Jun
>
>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>> ---
>> drivers/usb/dwc3/core.h | 150
>> ++++++++++++++++++++++-----------------------
>> drivers/usb/dwc3/debugfs.c | 2 +-
>> drivers/usb/dwc3/io.h | 12 ++--
>> 3 files changed, 82 insertions(+), 82 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 8f9959b..3af244e 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -85,90 +85,90 @@
>> #define DWC3_OTG_REGS_END 0xccff
>>
>> /* Global Registers */
>> -#define DWC3_GSBUSCFG0 0xc100
>> -#define DWC3_GSBUSCFG1 0xc104
>> -#define DWC3_GTXTHRCFG 0xc108
>> -#define DWC3_GRXTHRCFG 0xc10c
>> -#define DWC3_GCTL 0xc110
>> -#define DWC3_GEVTEN 0xc114
>> -#define DWC3_GSTS 0xc118
>> -#define DWC3_GUCTL1 0xc11c
>> -#define DWC3_GSNPSID 0xc120
>> -#define DWC3_GGPIO 0xc124
>> -#define DWC3_GUID 0xc128
>> -#define DWC3_GUCTL 0xc12c
>> -#define DWC3_GBUSERRADDR0 0xc130
>> -#define DWC3_GBUSERRADDR1 0xc134
>> -#define DWC3_GPRTBIMAP0 0xc138
>> -#define DWC3_GPRTBIMAP1 0xc13c
>> -#define DWC3_GHWPARAMS0 0xc140
>> -#define DWC3_GHWPARAMS1 0xc144
>> -#define DWC3_GHWPARAMS2 0xc148
>> -#define DWC3_GHWPARAMS3 0xc14c
>> -#define DWC3_GHWPARAMS4 0xc150
>> -#define DWC3_GHWPARAMS5 0xc154
>> -#define DWC3_GHWPARAMS6 0xc158
>> -#define DWC3_GHWPARAMS7 0xc15c
>> -#define DWC3_GDBGFIFOSPACE 0xc160
>> -#define DWC3_GDBGLTSSM 0xc164
>> -#define DWC3_GDBGBMU 0xc16c
>> -#define DWC3_GDBGLSPMUX 0xc170
>> -#define DWC3_GDBGLSP 0xc174
>> -#define DWC3_GDBGEPINFO0 0xc178
>> -#define DWC3_GDBGEPINFO1 0xc17c
>> -#define DWC3_GPRTBIMAP_HS0 0xc180
>> -#define DWC3_GPRTBIMAP_HS1 0xc184
>> -#define DWC3_GPRTBIMAP_FS0 0xc188
>> -#define DWC3_GPRTBIMAP_FS1 0xc18c
>> -#define DWC3_GUCTL2 0xc19c
>>
>>
>> --
>> 2.7.4
^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
2023-01-06 9:21 ` [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function Linyu Yuan
@ 2023-01-09 3:33 ` Jun Li (OSS)
2023-01-09 3:41 ` Linyu Yuan
0 siblings, 1 reply; 23+ messages in thread
From: Jun Li (OSS) @ 2023-01-09 3:33 UTC (permalink / raw)
To: Linyu Yuan, Greg Kroah-Hartman, Thinh Nguyen
Cc: linux-usb, Jack Pham, Wesley Cheng
> -----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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
2023-01-09 3:33 ` Jun Li (OSS)
@ 2023-01-09 3:41 ` Linyu Yuan
2023-01-11 7:21 ` Jun Li (OSS)
0 siblings, 1 reply; 23+ messages in thread
From: Linyu Yuan @ 2023-01-09 3:41 UTC (permalink / raw)
To: Jun Li (OSS), Greg Kroah-Hartman, Thinh Nguyen
Cc: linux-usb, Jack Pham, Wesley Cheng
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 ?
>
>> 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@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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/3] usb: dwc3: simplify operation in dwc3_readl() and dwc3_writel()
2023-01-09 3:09 ` Linyu Yuan
@ 2023-01-09 4:54 ` Linyu Yuan
0 siblings, 0 replies; 23+ messages in thread
From: Linyu Yuan @ 2023-01-09 4:54 UTC (permalink / raw)
To: Jun Li (OSS), Greg Kroah-Hartman, Thinh Nguyen
Cc: linux-usb, Jack Pham, Wesley Cheng
On 1/9/2023 11:09 AM, Linyu Yuan wrote:
> On 1/9/2023 11:02 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 1/3] usb: dwc3: simplify operation in dwc3_readl() and
>>> dwc3_writel()
>>>
>>> when dwc3_readl() read register and dwc3_writel() write register,
>>> it will run operation 'base + offset - DWC3_GLOBALS_REGS_START' to
>>> calculate register address, seem the minus operation can avoid.
>>>
>>> the original register definition is offset from XHCI base 0x0,
>>> now change it to offset from DWC3_GLOBALS_REGS_START(0xc100).
>> Is this really can bring benefit? With this change user has to takes
> I didn't check all compiler generated code and if more instruction
> needed for original code.
>> care an offset, looks to me the original definition is very
>> straightforward,
>> use the offset defined in DWC3 Databook, user can directly check each
>> register
>> definition by offset (not just by name).
> this is good point, let me check one compile code and make a decision
> to remove this change or not.
will remove this one as no difference in generated asm code.
>>
>> Li Jun
>>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>>> ---
>>> drivers/usb/dwc3/core.h | 150
>>> ++++++++++++++++++++++-----------------------
>>> drivers/usb/dwc3/debugfs.c | 2 +-
>>> drivers/usb/dwc3/io.h | 12 ++--
>>> 3 files changed, 82 insertions(+), 82 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 8f9959b..3af244e 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -85,90 +85,90 @@
>>> #define DWC3_OTG_REGS_END 0xccff
>>>
>>> /* Global Registers */
>>> -#define DWC3_GSBUSCFG0 0xc100
>>> -#define DWC3_GSBUSCFG1 0xc104
>>> -#define DWC3_GTXTHRCFG 0xc108
>>> -#define DWC3_GRXTHRCFG 0xc10c
>>> -#define DWC3_GCTL 0xc110
>>> -#define DWC3_GEVTEN 0xc114
>>> -#define DWC3_GSTS 0xc118
>>> -#define DWC3_GUCTL1 0xc11c
>>> -#define DWC3_GSNPSID 0xc120
>>> -#define DWC3_GGPIO 0xc124
>>> -#define DWC3_GUID 0xc128
>>> -#define DWC3_GUCTL 0xc12c
>>> -#define DWC3_GBUSERRADDR0 0xc130
>>> -#define DWC3_GBUSERRADDR1 0xc134
>>> -#define DWC3_GPRTBIMAP0 0xc138
>>> -#define DWC3_GPRTBIMAP1 0xc13c
>>> -#define DWC3_GHWPARAMS0 0xc140
>>> -#define DWC3_GHWPARAMS1 0xc144
>>> -#define DWC3_GHWPARAMS2 0xc148
>>> -#define DWC3_GHWPARAMS3 0xc14c
>>> -#define DWC3_GHWPARAMS4 0xc150
>>> -#define DWC3_GHWPARAMS5 0xc154
>>> -#define DWC3_GHWPARAMS6 0xc158
>>> -#define DWC3_GHWPARAMS7 0xc15c
>>> -#define DWC3_GDBGFIFOSPACE 0xc160
>>> -#define DWC3_GDBGLTSSM 0xc164
>>> -#define DWC3_GDBGBMU 0xc16c
>>> -#define DWC3_GDBGLSPMUX 0xc170
>>> -#define DWC3_GDBGLSP 0xc174
>>> -#define DWC3_GDBGEPINFO0 0xc178
>>> -#define DWC3_GDBGEPINFO1 0xc17c
>>> -#define DWC3_GPRTBIMAP_HS0 0xc180
>>> -#define DWC3_GPRTBIMAP_HS1 0xc184
>>> -#define DWC3_GPRTBIMAP_FS0 0xc188
>>> -#define DWC3_GPRTBIMAP_FS1 0xc18c
>>> -#define DWC3_GUCTL2 0xc19c
>>>
>>>
>>> --
>>> 2.7.4
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] usb: dwc3: remove base parameter of event class dwc3_log_io
2023-01-06 9:21 ` [PATCH 3/3] usb: dwc3: remove base parameter of event class dwc3_log_io Linyu Yuan
@ 2023-01-09 18:47 ` Thinh Nguyen
2023-01-10 2:26 ` Linyu Yuan
0 siblings, 1 reply; 23+ messages in thread
From: Thinh Nguyen @ 2023-01-09 18:47 UTC (permalink / raw)
To: Linyu Yuan
Cc: Greg Kroah-Hartman, Thinh Nguyen, linux-usb, Jack Pham, Wesley Cheng
On Fri, Jan 06, 2023, Linyu Yuan wrote:
> when trace register read/write operation, it is not necessary to show
> virtual address cacaulate from base and offset.
>
> remove the base register value, it will save trace buffer.
> it is enough only save and show the offset.
>
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> drivers/usb/dwc3/io.h | 4 ++--
> drivers/usb/dwc3/trace.h | 17 +++++++----------
> 2 files changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
> index d24513e..fcb5726 100644
> --- a/drivers/usb/dwc3/io.h
> +++ b/drivers/usb/dwc3/io.h
> @@ -32,7 +32,7 @@ static inline u32 dwc3_readl(void __iomem *base, u32 offset)
> * documentation, so we revert it back to the proper addresses, the
> * same way they are described on SNPS documentation
> */
> - trace_dwc3_readl(base, offset, value);
> + trace_dwc3_readl(offset, value);
>
> return value;
> }
> @@ -61,7 +61,7 @@ static inline void dwc3_writel(void __iomem *base, u32 offset, u32 value)
> * documentation, so we revert it back to the proper addresses, the
> * same way they are described on SNPS documentation
> */
> - trace_dwc3_writel(base, offset, value);
> + trace_dwc3_writel(offset, value);
> }
>
> #endif /* __DRIVERS_USB_DWC3_IO_H */
> diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
> index 1975aec..536b9a1 100644
> --- a/drivers/usb/dwc3/trace.h
> +++ b/drivers/usb/dwc3/trace.h
> @@ -20,32 +20,29 @@
> #include "debug.h"
>
> DECLARE_EVENT_CLASS(dwc3_log_io,
> - TP_PROTO(void *base, u32 offset, u32 value),
> - TP_ARGS(base, offset, value),
> + TP_PROTO(u32 offset, u32 value),
> + TP_ARGS(offset, value),
> TP_STRUCT__entry(
> - __field(void *, base)
> __field(u32, offset)
> __field(u32, value)
> ),
> TP_fast_assign(
> - __entry->base = base;
> __entry->offset = offset;
> __entry->value = value;
> ),
> - TP_printk("addr %p offset %04x value %08x",
> - __entry->base + __entry->offset,
Please don't remove this. Sometime we need to check the base address for
different register offset. Currently some offsets need this to be able
to differientiate between different registers (e.g. different endpoint
registers DEPCMP and parameters)
Thanks,
Thinh
> + TP_printk("offset %04x value %08x",
> __entry->offset,
> __entry->value)
> );
>
> DEFINE_EVENT(dwc3_log_io, dwc3_readl,
> - TP_PROTO(void __iomem *base, u32 offset, u32 value),
> - TP_ARGS(base, offset, value)
> + TP_PROTO(u32 offset, u32 value),
> + TP_ARGS(offset, value)
> );
>
> DEFINE_EVENT(dwc3_log_io, dwc3_writel,
> - TP_PROTO(void __iomem *base, u32 offset, u32 value),
> - TP_ARGS(base, offset, value)
> + TP_PROTO(u32 offset, u32 value),
> + TP_ARGS(offset, value)
> );
>
> DECLARE_EVENT_CLASS(dwc3_log_event,
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] usb: dwc3: remove base parameter of event class dwc3_log_io
2023-01-09 18:47 ` Thinh Nguyen
@ 2023-01-10 2:26 ` Linyu Yuan
2023-01-10 2:57 ` Thinh Nguyen
0 siblings, 1 reply; 23+ messages in thread
From: Linyu Yuan @ 2023-01-10 2:26 UTC (permalink / raw)
To: Thinh Nguyen; +Cc: Greg Kroah-Hartman, linux-usb, Jack Pham, Wesley Cheng
On 1/10/2023 2:47 AM, Thinh Nguyen wrote:
> On Fri, Jan 06, 2023, Linyu Yuan wrote:
>> when trace register read/write operation, it is not necessary to show
>> virtual address cacaulate from base and offset.
>>
>> remove the base register value, it will save trace buffer.
>> it is enough only save and show the offset.
>>
>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>> ---
>> drivers/usb/dwc3/io.h | 4 ++--
>> drivers/usb/dwc3/trace.h | 17 +++++++----------
>> 2 files changed, 9 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
>> index d24513e..fcb5726 100644
>> --- a/drivers/usb/dwc3/io.h
>> +++ b/drivers/usb/dwc3/io.h
>> @@ -32,7 +32,7 @@ static inline u32 dwc3_readl(void __iomem *base, u32 offset)
>> * documentation, so we revert it back to the proper addresses, the
>> * same way they are described on SNPS documentation
>> */
>> - trace_dwc3_readl(base, offset, value);
>> + trace_dwc3_readl(offset, value);
>>
>> return value;
>> }
>> @@ -61,7 +61,7 @@ static inline void dwc3_writel(void __iomem *base, u32 offset, u32 value)
>> * documentation, so we revert it back to the proper addresses, the
>> * same way they are described on SNPS documentation
>> */
>> - trace_dwc3_writel(base, offset, value);
>> + trace_dwc3_writel(offset, value);
>> }
>>
>> #endif /* __DRIVERS_USB_DWC3_IO_H */
>> diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
>> index 1975aec..536b9a1 100644
>> --- a/drivers/usb/dwc3/trace.h
>> +++ b/drivers/usb/dwc3/trace.h
>> @@ -20,32 +20,29 @@
>> #include "debug.h"
>>
>> DECLARE_EVENT_CLASS(dwc3_log_io,
>> - TP_PROTO(void *base, u32 offset, u32 value),
>> - TP_ARGS(base, offset, value),
>> + TP_PROTO(u32 offset, u32 value),
>> + TP_ARGS(offset, value),
>> TP_STRUCT__entry(
>> - __field(void *, base)
>> __field(u32, offset)
>> __field(u32, value)
>> ),
>> TP_fast_assign(
>> - __entry->base = base;
>> __entry->offset = offset;
>> __entry->value = value;
>> ),
>> - TP_printk("addr %p offset %04x value %08x",
>> - __entry->base + __entry->offset,
> Please don't remove this. Sometime we need to check the base address for
> different register offset. Currently some offsets need this to be able
> to differientiate between different registers (e.g. different endpoint
> registers DEPCMP and parameters)
thanks, will drop this patch.
>
> Thanks,
> Thinh
>
>> + TP_printk("offset %04x value %08x",
>> __entry->offset,
>> __entry->value)
>> );
>>
>> DEFINE_EVENT(dwc3_log_io, dwc3_readl,
>> - TP_PROTO(void __iomem *base, u32 offset, u32 value),
>> - TP_ARGS(base, offset, value)
>> + TP_PROTO(u32 offset, u32 value),
>> + TP_ARGS(offset, value)
>> );
>>
>> DEFINE_EVENT(dwc3_log_io, dwc3_writel,
>> - TP_PROTO(void __iomem *base, u32 offset, u32 value),
>> - TP_ARGS(base, offset, value)
>> + TP_PROTO(u32 offset, u32 value),
>> + TP_ARGS(offset, value)
>> );
>>
>> DECLARE_EVENT_CLASS(dwc3_log_event,
>> --
>> 2.7.4
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] usb: dwc3: remove base parameter of event class dwc3_log_io
2023-01-10 2:26 ` Linyu Yuan
@ 2023-01-10 2:57 ` Thinh Nguyen
2023-01-10 3:09 ` Linyu Yuan
0 siblings, 1 reply; 23+ messages in thread
From: Thinh Nguyen @ 2023-01-10 2:57 UTC (permalink / raw)
To: Linyu Yuan
Cc: Thinh Nguyen, Greg Kroah-Hartman, linux-usb, Jack Pham, Wesley Cheng
On Tue, Jan 10, 2023, Linyu Yuan wrote:
>
> On 1/10/2023 2:47 AM, Thinh Nguyen wrote:
> > On Fri, Jan 06, 2023, Linyu Yuan wrote:
> > > when trace register read/write operation, it is not necessary to show
> > > virtual address cacaulate from base and offset.
> > >
> > > remove the base register value, it will save trace buffer.
> > > it is enough only save and show the offset.
> > >
> > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > > ---
> > > drivers/usb/dwc3/io.h | 4 ++--
> > > drivers/usb/dwc3/trace.h | 17 +++++++----------
> > > 2 files changed, 9 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
> > > index d24513e..fcb5726 100644
> > > --- a/drivers/usb/dwc3/io.h
> > > +++ b/drivers/usb/dwc3/io.h
> > > @@ -32,7 +32,7 @@ static inline u32 dwc3_readl(void __iomem *base, u32 offset)
> > > * documentation, so we revert it back to the proper addresses, the
> > > * same way they are described on SNPS documentation
> > > */
> > > - trace_dwc3_readl(base, offset, value);
> > > + trace_dwc3_readl(offset, value);
> > > return value;
> > > }
> > > @@ -61,7 +61,7 @@ static inline void dwc3_writel(void __iomem *base, u32 offset, u32 value)
> > > * documentation, so we revert it back to the proper addresses, the
> > > * same way they are described on SNPS documentation
> > > */
> > > - trace_dwc3_writel(base, offset, value);
> > > + trace_dwc3_writel(offset, value);
> > > }
> > > #endif /* __DRIVERS_USB_DWC3_IO_H */
> > > diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
> > > index 1975aec..536b9a1 100644
> > > --- a/drivers/usb/dwc3/trace.h
> > > +++ b/drivers/usb/dwc3/trace.h
> > > @@ -20,32 +20,29 @@
> > > #include "debug.h"
> > > DECLARE_EVENT_CLASS(dwc3_log_io,
> > > - TP_PROTO(void *base, u32 offset, u32 value),
> > > - TP_ARGS(base, offset, value),
> > > + TP_PROTO(u32 offset, u32 value),
> > > + TP_ARGS(offset, value),
> > > TP_STRUCT__entry(
> > > - __field(void *, base)
> > > __field(u32, offset)
> > > __field(u32, value)
> > > ),
> > > TP_fast_assign(
> > > - __entry->base = base;
> > > __entry->offset = offset;
> > > __entry->value = value;
> > > ),
> > > - TP_printk("addr %p offset %04x value %08x",
> > > - __entry->base + __entry->offset,
> > Please don't remove this. Sometime we need to check the base address for
> > different register offset. Currently some offsets need this to be able
> > to differientiate between different registers (e.g. different endpoint
> > registers DEPCMP and parameters)
DEPCMP -> DEPCMD*
>
> thanks, will drop this patch.
>
If we fix it so that all the offset prints represent the correct offset
of all the registers, then we can drop this base address print. That
would be great.
Thanks,
Thinh
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] usb: dwc3: remove base parameter of event class dwc3_log_io
2023-01-10 2:57 ` Thinh Nguyen
@ 2023-01-10 3:09 ` Linyu Yuan
0 siblings, 0 replies; 23+ messages in thread
From: Linyu Yuan @ 2023-01-10 3:09 UTC (permalink / raw)
To: Thinh Nguyen; +Cc: Greg Kroah-Hartman, linux-usb, Jack Pham, Wesley Cheng
On 1/10/2023 10:57 AM, Thinh Nguyen wrote:
> On Tue, Jan 10, 2023, Linyu Yuan wrote:
>> On 1/10/2023 2:47 AM, Thinh Nguyen wrote:
>>> On Fri, Jan 06, 2023, Linyu Yuan wrote:
>>>> when trace register read/write operation, it is not necessary to show
>>>> virtual address cacaulate from base and offset.
>>>>
>>>> remove the base register value, it will save trace buffer.
>>>> it is enough only save and show the offset.
>>>>
>>>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>>>> ---
>>>> drivers/usb/dwc3/io.h | 4 ++--
>>>> drivers/usb/dwc3/trace.h | 17 +++++++----------
>>>> 2 files changed, 9 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
>>>> index d24513e..fcb5726 100644
>>>> --- a/drivers/usb/dwc3/io.h
>>>> +++ b/drivers/usb/dwc3/io.h
>>>> @@ -32,7 +32,7 @@ static inline u32 dwc3_readl(void __iomem *base, u32 offset)
>>>> * documentation, so we revert it back to the proper addresses, the
>>>> * same way they are described on SNPS documentation
>>>> */
>>>> - trace_dwc3_readl(base, offset, value);
>>>> + trace_dwc3_readl(offset, value);
>>>> return value;
>>>> }
>>>> @@ -61,7 +61,7 @@ static inline void dwc3_writel(void __iomem *base, u32 offset, u32 value)
>>>> * documentation, so we revert it back to the proper addresses, the
>>>> * same way they are described on SNPS documentation
>>>> */
>>>> - trace_dwc3_writel(base, offset, value);
>>>> + trace_dwc3_writel(offset, value);
>>>> }
>>>> #endif /* __DRIVERS_USB_DWC3_IO_H */
>>>> diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
>>>> index 1975aec..536b9a1 100644
>>>> --- a/drivers/usb/dwc3/trace.h
>>>> +++ b/drivers/usb/dwc3/trace.h
>>>> @@ -20,32 +20,29 @@
>>>> #include "debug.h"
>>>> DECLARE_EVENT_CLASS(dwc3_log_io,
>>>> - TP_PROTO(void *base, u32 offset, u32 value),
>>>> - TP_ARGS(base, offset, value),
>>>> + TP_PROTO(u32 offset, u32 value),
>>>> + TP_ARGS(offset, value),
>>>> TP_STRUCT__entry(
>>>> - __field(void *, base)
>>>> __field(u32, offset)
>>>> __field(u32, value)
>>>> ),
>>>> TP_fast_assign(
>>>> - __entry->base = base;
>>>> __entry->offset = offset;
>>>> __entry->value = value;
>>>> ),
>>>> - TP_printk("addr %p offset %04x value %08x",
>>>> - __entry->base + __entry->offset,
>>> Please don't remove this. Sometime we need to check the base address for
>>> different register offset. Currently some offsets need this to be able
>>> to differientiate between different registers (e.g. different endpoint
>>> registers DEPCMP and parameters)
> DEPCMP -> DEPCMD*
I can't understand your comment, I think every DEPCMD* register has it
own offset.
>
>> thanks, will drop this patch.
>>
> If we fix it so that all the offset prints represent the correct offset
> of all the registers, then we can drop this base address print. That
> would be great.
I think only if multiple gadgets exist in real world, then base address
is needed.
>
> Thanks,
> Thinh
^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
2023-01-09 3:41 ` Linyu Yuan
@ 2023-01-11 7:21 ` Jun Li (OSS)
2023-01-12 1:56 ` Linyu Yuan
0 siblings, 1 reply; 23+ messages in thread
From: Jun Li (OSS) @ 2023-01-11 7:21 UTC (permalink / raw)
To: Linyu Yuan, Jun Li (OSS), Greg Kroah-Hartman, Thinh Nguyen
Cc: linux-usb, Jack Pham, Wesley Cheng
> -----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?
> >
> >> 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
2023-01-11 7:21 ` Jun Li (OSS)
@ 2023-01-12 1:56 ` Linyu Yuan
2023-01-13 1:08 ` Thinh Nguyen
0 siblings, 1 reply; 23+ messages in thread
From: Linyu Yuan @ 2023-01-12 1:56 UTC (permalink / raw)
To: Jun Li (OSS), Greg Kroah-Hartman, Thinh Nguyen
Cc: linux-usb, Jack Pham, Wesley Cheng
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 ?
>
>>>> 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
2023-01-12 1:56 ` Linyu Yuan
@ 2023-01-13 1:08 ` Thinh Nguyen
2023-01-13 2:37 ` Linyu Yuan
0 siblings, 1 reply; 23+ messages in thread
From: Thinh Nguyen @ 2023-01-13 1:08 UTC (permalink / raw)
To: Linyu Yuan
Cc: Jun Li (OSS),
Greg Kroah-Hartman, Thinh Nguyen, linux-usb, Jack Pham,
Wesley Cheng
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
2023-01-13 1:08 ` Thinh Nguyen
@ 2023-01-13 2:37 ` Linyu Yuan
2023-01-13 3:18 ` Thinh Nguyen
0 siblings, 1 reply; 23+ messages in thread
From: Linyu Yuan @ 2023-01-13 2:37 UTC (permalink / raw)
To: Thinh Nguyen
Cc: Jun Li (OSS), Greg Kroah-Hartman, linux-usb, Jack Pham, Wesley Cheng
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
2023-01-13 2:37 ` Linyu Yuan
@ 2023-01-13 3:18 ` Thinh Nguyen
2023-01-13 3:25 ` Linyu Yuan
0 siblings, 1 reply; 23+ messages in thread
From: Thinh Nguyen @ 2023-01-13 3:18 UTC (permalink / raw)
To: Linyu Yuan
Cc: Thinh Nguyen, Jun Li (OSS),
Greg Kroah-Hartman, linux-usb, Jack Pham, Wesley Cheng
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
> >
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
2023-01-13 3:18 ` Thinh Nguyen
@ 2023-01-13 3:25 ` Linyu Yuan
2023-01-13 5:24 ` Thinh Nguyen
0 siblings, 1 reply; 23+ messages in thread
From: Linyu Yuan @ 2023-01-13 3:25 UTC (permalink / raw)
To: Thinh Nguyen
Cc: Jun Li (OSS), Greg Kroah-Hartman, linux-usb, Jack Pham, Wesley Cheng
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
2023-01-13 3:25 ` Linyu Yuan
@ 2023-01-13 5:24 ` Thinh Nguyen
2023-01-13 5:55 ` Linyu Yuan
0 siblings, 1 reply; 23+ messages in thread
From: Thinh Nguyen @ 2023-01-13 5:24 UTC (permalink / raw)
To: Linyu Yuan
Cc: Thinh Nguyen, Jun Li (OSS),
Greg Kroah-Hartman, linux-usb, Jack Pham, Wesley Cheng
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
2023-01-13 5:24 ` Thinh Nguyen
@ 2023-01-13 5:55 ` Linyu Yuan
2023-01-13 23:16 ` Thinh Nguyen
0 siblings, 1 reply; 23+ messages in thread
From: Linyu Yuan @ 2023-01-13 5:55 UTC (permalink / raw)
To: Thinh Nguyen
Cc: Jun Li (OSS), Greg Kroah-Hartman, linux-usb, Jack Pham, Wesley Cheng
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
2023-01-13 5:55 ` Linyu Yuan
@ 2023-01-13 23:16 ` Thinh Nguyen
2023-01-18 2:04 ` Linyu Yuan
0 siblings, 1 reply; 23+ messages in thread
From: Thinh Nguyen @ 2023-01-13 23:16 UTC (permalink / raw)
To: Linyu Yuan
Cc: Thinh Nguyen, Jun Li (OSS),
Greg Kroah-Hartman, linux-usb, Jack Pham, Wesley Cheng
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
2023-01-13 23:16 ` Thinh Nguyen
@ 2023-01-18 2:04 ` Linyu Yuan
2023-01-20 23:15 ` Thinh Nguyen
0 siblings, 1 reply; 23+ messages in thread
From: Linyu Yuan @ 2023-01-18 2:04 UTC (permalink / raw)
To: Thinh Nguyen
Cc: Jun Li (OSS), Greg Kroah-Hartman, linux-usb, Jack Pham, Wesley Cheng
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
2023-01-18 2:04 ` Linyu Yuan
@ 2023-01-20 23:15 ` Thinh Nguyen
0 siblings, 0 replies; 23+ messages in thread
From: Thinh Nguyen @ 2023-01-20 23:15 UTC (permalink / raw)
To: Linyu Yuan
Cc: Thinh Nguyen, Jun Li (OSS),
Greg Kroah-Hartman, linux-usb, Jack Pham, Wesley Cheng
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
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2023-01-20 23:15 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-06 9:21 [PATCH 1/3] usb: dwc3: simplify operation in dwc3_readl() and dwc3_writel() Linyu Yuan
2023-01-06 9:21 ` [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function Linyu Yuan
2023-01-09 3:33 ` Jun Li (OSS)
2023-01-09 3:41 ` Linyu Yuan
2023-01-11 7:21 ` Jun Li (OSS)
2023-01-12 1:56 ` Linyu Yuan
2023-01-13 1:08 ` Thinh Nguyen
2023-01-13 2:37 ` Linyu Yuan
2023-01-13 3:18 ` Thinh Nguyen
2023-01-13 3:25 ` Linyu Yuan
2023-01-13 5:24 ` Thinh Nguyen
2023-01-13 5:55 ` Linyu Yuan
2023-01-13 23:16 ` Thinh Nguyen
2023-01-18 2:04 ` Linyu Yuan
2023-01-20 23:15 ` Thinh Nguyen
2023-01-06 9:21 ` [PATCH 3/3] usb: dwc3: remove base parameter of event class dwc3_log_io Linyu Yuan
2023-01-09 18:47 ` Thinh Nguyen
2023-01-10 2:26 ` Linyu Yuan
2023-01-10 2:57 ` Thinh Nguyen
2023-01-10 3:09 ` Linyu Yuan
2023-01-09 3:02 ` [PATCH 1/3] usb: dwc3: simplify operation in dwc3_readl() and dwc3_writel() Jun Li (OSS)
2023-01-09 3:09 ` Linyu Yuan
2023-01-09 4:54 ` Linyu Yuan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.