[v1] usb: dwc3: xilinx: Prevent spike in reset signal

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

Posted by Mike Looijmans 9 months, 1 week ago

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.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---

 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. */
-		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] usb: dwc3: xilinx: Prevent spike in reset signal

Posted by Thinh Nguyen 9 months, 1 week ago

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?

> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> 
>  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?

> -		gpiod_set_value_cansleep(reset_gpio, 1);
>  		usleep_range(5000, 10000);

Do we still need this usleep_range here?

BR,
Thinh

>  		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: https://urldefense.com/v3/__http://www.topic.nl__;!!A4F2R9G_pg!dt2SxHPWske2gY2PNQ9GrJEr5lXXkUCMb5CMBd2ymPEYpU1TEemM__y9_6QwiMTyCIX9wXKKAOzhX5Ex5x5FWa1QV5I98u0$ 
> 
> Please consider the environment before printing this e-mail

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

Posted by Mike Looijmans 9 months ago

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?

The reset sequence was just plain wrong, the issue is almost the same as 
the 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@topic.nl>
>> ---
>>
>>   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.


>> -		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.



>
> BR,
> Thinh
>
>>   		gpiod_set_value_cansleep(reset_gpio, 0);
>>   		usleep_range(5000, 10000);
>> -- 
>> 2.43.0
>>
>>
>> Met vriendelijke groet / kind regards,
>>
>> Mike Looijmans
>>
>>

-- 
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

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

Posted by Thinh Nguyen 9 months ago

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.

> 
> 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?

> 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@topic.nl>
> > > ---
> > > 
> > >   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?

> 
> > > -		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?

BR,
Thinh

> 
> 
> > 
> > BR,
> > Thinh
> > 
> > >   		gpiod_set_value_cansleep(reset_gpio, 0);
> > >   		usleep_range(5000, 10000);
> > > -- 
> > > 2.43.0
> > > 
> > > 
> > > Met vriendelijke groet / kind regards,
> > > 
> > > Mike Looijmans
> > > 
> > > 
> 
> -- 
> 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: https://urldefense.com/v3/__http://www.topic.nl__;!!A4F2R9G_pg!e45B0wD5dvB4NV8gz5JjIWaRTQrX9M2uE0ouoBVX4TQC8sKqtYRL8rJY3y2bb061gzSoGL0FOPJv-3-adkP1b3ldzRZnxdY$
> 
> 
>

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

Posted by Mike Looijmans 9 months ago

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@topic.nl>
>>>> ---
>>>>
>>>>    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.


> BR,
> Thinh
>
>>
>>> BR,
>>> Thinh
>>>
>>>>    		gpiod_set_value_cansleep(reset_gpio, 0);
>>>>    		usleep_range(5000, 10000);
>>>> -- 
>>>> 2.43.0
>>>>
>>>>
>>>> Met vriendelijke groet / kind regards,
>>>>
>>>> Mike Looijmans
>>>>
>>>>
>> -- 
>> 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: https://urldefense.com/v3/__http://www.topic.nl__;!!A4F2R9G_pg!e45B0wD5dvB4NV8gz5JjIWaRTQrX9M2uE0ouoBVX4TQC8sKqtYRL8rJY3y2bb061gzSoGL0FOPJv-3-adkP1b3ldzRZnxdY$
>>
>>

-- 
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

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

Posted by Thinh Nguyen 9 months ago

On Tue, Mar 18, 2025, 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@topic.nl>
> > > > > ---
> > > > > 
> > > > >    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.
> 

Ah, that makes sense. Thanks for the explaination.

BR,
Thinh

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

Posted by Michal Simek 9 months ago

+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@topic.nl>
>>>>> ---
>>>>>
>>>>>    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