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@xxxxxxxxxxx>
Sent: Monday, January 9, 2023 11:42 AM
To: Jun Li (OSS) <jun.li@xxxxxxxxxxx>; Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx>; Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>
Cc: linux-usb@xxxxxxxxxxxxxxx; Jack Pham <quic_jackp@xxxxxxxxxxx>; Wesley
Cheng <quic_wcheng@xxxxxxxxxxx>
Subject: Re: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
On 1/9/2023 11:33 AM, Jun Li (OSS) wrote:
-----Original Message-----
From: Linyu Yuan <quic_linyyuan@xxxxxxxxxxx>
Sent: Friday, January 6, 2023 5:22 PM
To: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Thinh Nguyen
<Thinh.Nguyen@xxxxxxxxxxxx>
Cc: linux-usb@xxxxxxxxxxxxxxx; Jack Pham <quic_jackp@xxxxxxxxxxx>;
Wesley Cheng <quic_wcheng@xxxxxxxxxxx>; Linyu Yuan
<quic_linyyuan@xxxxxxxxxxx>
Subject: [PATCH 2/3] usb: dwc3: add dwc3_readl_notrace() function
There are multiple places which will retry up to 10000 times to read
a register,
It's "up to", I think at normal case, it's a small number, if a large
Iteration number is observed, probably there is something wrong should
be checked?
do you mean the original loop count can be reduced ?
No. I mean the max loop number is not a problem at good HW.
What's the actual loop number on you HW?
i didn't check it. how about you ? no matter what is good HW or bad HW,
current code define a big number.
actually i think we should not discuss is it a good number or not.
what is purpose to use status register record ? do it give you any
information to understand the IP behavior ?
While some polling numbers seem large, we should not remove the tracing
events during polling. There are useful info in the status register when
there's a timeout. Also, the number of polls needed for certain state
change is useful data point for debug.
What we may want to update is not depend on the register read delay for
the polling duration. Different setup will have different delay.
If we want to disable it for debugging purpose, make sure to have the
default option as enabled.
if so, do you accept define a new function and new trace event like,
static inline u32 dwc3_readl_timeout(void __iomem *base, u32 offset)
DEFINE_EVENT(dwc3_log_io, dwc3_readl_timeout,
TP_PROTO(void __iomem *base, u32 offset, u32 value),
TP_ARGS(base, offset, value)
);
let user enable it accordingly.
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@xxxxxxxxxx/T/*t__;Iw!!A4F2R9G_pg!eZBcW78zLMkOtfPIhxNvIGjItv_W2IVvpLyOV-3eCrWRk7T1PVERSmz037HLx1nLOHgOsRTK1Cz8DHFeBXZ21WwY5OkJlw$
Thanks,
Thinh
