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@xxxxxxx>
> Sent: Tuesday, March 18, 2025 1:07 PM
> To: Mike Looijmans <mike.looijmans@xxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; Pandey,
> Radhey Shyam <radhey.shyam.pandey@xxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Thinh Nguyen
> <Thinh.Nguyen@xxxxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxxx>

Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xxxxxxx>
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);