Re: [PATCH] usb: dwc3: xilinx: Prevent spike in reset signal
From: Michal Simek
Date: Tue Mar 18 2025 - 03:39:08 EST
+Radhey
On 3/18/25 07:21, Mike Looijmans wrote:
On 18-03-2025 01:12, Thinh Nguyen wrote:
On Mon, Mar 17, 2025, Mike Looijmans wrote:
On 14-03-2025 22:14, Thinh Nguyen wrote:
On Fri, Mar 14, 2025, Mike Looijmans wrote:
Set the gpio to "high" on acquisition, instead of acquiring it in low
state and then immediately setting it high again. This prevents a
short "spike" on the reset signal.
How does this affect the current programming flow beside preventing a
spike to the reset signal?
I don't understand your question. What "programming flow" are you referring
to?
It's not obvious to me if this is an error in Xilinx documentation, the
driver issue, or whether this is found through experiment. Since I don't
have the info of this platform, it would help to know where the source
of error is so we can document this in the code or change-log.
It's a bug in the driver, found through code inspection.
The reset GPIO here is to control the reset signal to an external, usually ULPI
PHY, chip. This external chip is not part of the Xilinx hardware.
The reset sequence was just plain wrong, the issue is almost the same as the
Do we need a fix tag and add to stable then?
That would be appropriate I think.
one described in this commit:
e0183b974d30 "net: mdiobus: Prevent spike on MDIO bus reset signal"
Note that I see this high-low-high-low double reset toggle in many Xilinx
software drivers, they seem to teach that at the Xilinx academy or so.
Signed-off-by: Mike Looijmans <mike.looijmans@xxxxxxxx>
---
drivers/usb/dwc3/dwc3-xilinx.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
index a33a42ba0249..a159a511483b 100644
--- a/drivers/usb/dwc3/dwc3-xilinx.c
+++ b/drivers/usb/dwc3/dwc3-xilinx.c
@@ -207,7 +207,7 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx
*priv_data)
skip_usb3_phy:
/* ulpi reset via gpio-modepin or gpio-framework driver */
- reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+ reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(reset_gpio)) {
return dev_err_probe(dev, PTR_ERR(reset_gpio),
"Failed to request reset GPIO\n");
@@ -215,7 +215,6 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx
*priv_data)
if (reset_gpio) {
/* Toggle ulpi to reset the phy. */
Does the comment above still apply?
Now you mention it, the comment never made any sense anyway.
Then can we remove it?
Removing would be better, yes. I'll make a v2 patch.
- gpiod_set_value_cansleep(reset_gpio, 1);
usleep_range(5000, 10000);
Do we still need this usleep_range here?
Yes, this is the "reset active" time.
But why do we need 2 calls to usleep_range? From just looking at this
here, it appears that the first was intended for the removed
gpiod_set_value_cansleep(reset_gpio, 1). If this "reset active" time is
needed irrespective of the existent reset_gpio, then shouldn't it be set
outside of this if statement?
It helps to see the whole thing instead of just the patch.
If I omit error handling and comments then the original code reads:
reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
if (reset_gpio) {
gpiod_set_value_cansleep(reset_gpio, 1);
usleep_range(5000, 10000);
gpiod_set_value_cansleep(reset_gpio, 0);
usleep_range(5000, 10000);
}
So the gpio is acquired in a LOW state and then, without delay, is set to a high
state again. This causes the "spike" I'm mentioning here. The correct procedure
is to acquire it in the HIGH state immediately, so the sequence becomes:
reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
if (reset_gpio) {
usleep_range(5000, 10000);
gpiod_set_value_cansleep(reset_gpio, 0);
usleep_range(5000, 10000);
}
This patch does exactly that.
Please keep Radhey in loop. He will take a look at it from our side.
Thanks,
Michal