[PATCH v2] usb: dwc3: xilinx: Prevent spike in reset signal

[PATCH v2] usb: dwc3: xilinx: Prevent spike in reset signal

[Mike Looijmans]

The "reset" GPIO controls the RESET signal to an external, usually
ULPI PHY, chip. The original code path acquires the signal in LOW
state, and then immediately asserts it HIGH again, if the reset
signal defaulted to asserted, there'd be a short "spike" before the
reset.

Here is what happens depending on the pre-existing state of the reset
signal:
Reset (previously asserted):   ~~~|_|~~~~|_______
Reset (previously deasserted): _____|~~~~|_______
                                  ^ ^    ^
                                  A B    C

At point A, the low going transition is because the reset line is
requested using GPIOD_OUT_LOW. If the line is successfully requested,
the first thing we do is set it high _without_ any delay. This is
point B. So, a glitch occurs between A and B.

Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
transitions. Instead we get:

Reset (previously asserted)  : ~~~~~~~~~~|______
Reset (previously deasserted): ____|~~~~~|______
                                   ^     ^
                                   A     C

Where A and C are the points described above in the code. Point B
has been eliminated.

The issue was found during code inspection.

Also remove the cryptic "toggle ulpi .." comment.

Fixes: ca05b38252d7 ("usb: dwc3: xilinx: Add gpio-reset support")
Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---

Changes in v2:
Add "Fixes" tag
Remove "toggle ulpi" comment
Extend comment to explain what is happening in detail

 drivers/usb/dwc3/dwc3-xilinx.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
index a33a42ba0249..4ca7f6240d07 100644
--- a/drivers/usb/dwc3/dwc3-xilinx.c
+++ b/drivers/usb/dwc3/dwc3-xilinx.c
@@ -207,15 +207,13 @@ 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");
 	}
 
 	if (reset_gpio) {
-		/* Toggle ulpi to reset the phy. */
-		gpiod_set_value_cansleep(reset_gpio, 1);
 		usleep_range(5000, 10000);
 		gpiod_set_value_cansleep(reset_gpio, 0);
 		usleep_range(5000, 10000);
-- 
2.43.0


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail

Re: [PATCH v2] usb: dwc3: xilinx: Prevent spike in reset signal

[Michal Simek]

+Radhey

On 3/18/25 07:44, Mike Looijmans wrote:
> The "reset" GPIO controls the RESET signal to an external, usually
> ULPI PHY, chip. The original code path acquires the signal in LOW
> state, and then immediately asserts it HIGH again, if the reset
> signal defaulted to asserted, there'd be a short "spike" before the
> reset.
> 
> Here is what happens depending on the pre-existing state of the reset
> signal:
> Reset (previously asserted):   ~~~|_|~~~~|_______
> Reset (previously deasserted): _____|~~~~|_______
>                                    ^ ^    ^
>                                    A B    C
> 
> At point A, the low going transition is because the reset line is
> requested using GPIOD_OUT_LOW. If the line is successfully requested,
> the first thing we do is set it high _without_ any delay. This is
> point B. So, a glitch occurs between A and B.
> 
> Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
> transitions. Instead we get:
> 
> Reset (previously asserted)  : ~~~~~~~~~~|______
> Reset (previously deasserted): ____|~~~~~|______
>                                     ^     ^
>                                     A     C
> 
> Where A and C are the points described above in the code. Point B
> has been eliminated.
> 
> The issue was found during code inspection.
> 
> Also remove the cryptic "toggle ulpi .." comment.
> 
> Fixes: ca05b38252d7 ("usb: dwc3: xilinx: Add gpio-reset support")
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> 
> Changes in v2:
> Add "Fixes" tag
> Remove "toggle ulpi" comment
> Extend comment to explain what is happening in detail
> 
>   drivers/usb/dwc3/dwc3-xilinx.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
> index a33a42ba0249..4ca7f6240d07 100644
> --- a/drivers/usb/dwc3/dwc3-xilinx.c
> +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> @@ -207,15 +207,13 @@ 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");
>   	}
>   
>   	if (reset_gpio) {
> -		/* Toggle ulpi to reset the phy. */
> -		gpiod_set_value_cansleep(reset_gpio, 1);
>   		usleep_range(5000, 10000);
>   		gpiod_set_value_cansleep(reset_gpio, 0);
>   		usleep_range(5000, 10000);

Re: [PATCH v2] usb: dwc3: xilinx: Prevent spike in reset signal

[Thinh Nguyen]

On Tue, Mar 18, 2025, Mike Looijmans wrote:
> The "reset" GPIO controls the RESET signal to an external, usually
> ULPI PHY, chip. The original code path acquires the signal in LOW
> state, and then immediately asserts it HIGH again, if the reset
> signal defaulted to asserted, there'd be a short "spike" before the
> reset.
> 
> Here is what happens depending on the pre-existing state of the reset
> signal:
> Reset (previously asserted):   ~~~|_|~~~~|_______
> Reset (previously deasserted): _____|~~~~|_______
>                                   ^ ^    ^
>                                   A B    C
> 
> At point A, the low going transition is because the reset line is
> requested using GPIOD_OUT_LOW. If the line is successfully requested,
> the first thing we do is set it high _without_ any delay. This is
> point B. So, a glitch occurs between A and B.
> 
> Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
> transitions. Instead we get:
> 
> Reset (previously asserted)  : ~~~~~~~~~~|______
> Reset (previously deasserted): ____|~~~~~|______
>                                    ^     ^
>                                    A     C
> 
> Where A and C are the points described above in the code. Point B
> has been eliminated.
> 
> The issue was found during code inspection.
> 
> Also remove the cryptic "toggle ulpi .." comment.
> 
> Fixes: ca05b38252d7 ("usb: dwc3: xilinx: Add gpio-reset support")

May want to add stable tag also?

> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> 
> Changes in v2:
> Add "Fixes" tag
> Remove "toggle ulpi" comment
> Extend comment to explain what is happening in detail
> 
>  drivers/usb/dwc3/dwc3-xilinx.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 

In any case,

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh

RE: [PATCH v2] usb: dwc3: xilinx: Prevent spike in reset signal

[Pandey, Radhey Shyam]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Simek, Michal <michal.simek@amd.com>
> Sent: Tuesday, March 18, 2025 1:07 PM
> To: Mike Looijmans <mike.looijmans@topic.nl>; linux-usb@vger.kernel.org; Pandey,
> Radhey Shyam <radhey.shyam.pandey@amd.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Thinh Nguyen
> <Thinh.Nguyen@synopsys.com>; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v2] usb: dwc3: xilinx: Prevent spike in reset signal
>
> +Radhey
>
> On 3/18/25 07:44, Mike Looijmans wrote:
> > The "reset" GPIO controls the RESET signal to an external, usually
> > ULPI PHY, chip. The original code path acquires the signal in LOW
> > state, and then immediately asserts it HIGH again, if the reset signal
> > defaulted to asserted, there'd be a short "spike" before the reset.
> >
> > Here is what happens depending on the pre-existing state of the reset
> > signal:
> > Reset (previously asserted):   ~~~|_|~~~~|_______
> > Reset (previously deasserted): _____|~~~~|_______
> >                                    ^ ^    ^
> >                                    A B    C
> >
> > At point A, the low going transition is because the reset line is
> > requested using GPIOD_OUT_LOW. If the line is successfully requested,
> > the first thing we do is set it high _without_ any delay. This is
> > point B. So, a glitch occurs between A and B.
> >
> > Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
> > transitions. Instead we get:
> >
> > Reset (previously asserted)  : ~~~~~~~~~~|______ Reset (previously
> > deasserted): ____|~~~~~|______
> >                                     ^     ^
> >                                     A     C
> >
> > Where A and C are the points described above in the code. Point B has
> > been eliminated.
> >
> > The issue was found during code inspection.
> >
> > Also remove the cryptic "toggle ulpi .." comment.
> >
> > Fixes: ca05b38252d7 ("usb: dwc3: xilinx: Add gpio-reset support")
> > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Thanks.
> > ---
> >
> > Changes in v2:
> > Add "Fixes" tag
> > Remove "toggle ulpi" comment
> > Extend comment to explain what is happening in detail
> >
> >   drivers/usb/dwc3/dwc3-xilinx.c | 4 +---
> >   1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/dwc3-xilinx.c
> > b/drivers/usb/dwc3/dwc3-xilinx.c index a33a42ba0249..4ca7f6240d07
> > 100644
> > --- a/drivers/usb/dwc3/dwc3-xilinx.c
> > +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> > @@ -207,15 +207,13 @@ 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");
> >     }
> >
> >     if (reset_gpio) {
> > -           /* Toggle ulpi to reset the phy. */
> > -           gpiod_set_value_cansleep(reset_gpio, 1);
> >             usleep_range(5000, 10000);
> >             gpiod_set_value_cansleep(reset_gpio, 0);
> >             usleep_range(5000, 10000);