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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a9812897-2c79-4a1c-9431-c6f184233a57@topic.nl>
Date: Tue, 18 Mar 2025 07:21:28 +0100
From: Mike Looijmans <mike.looijmans@...ic.nl>
To: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
CC: "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Michal Simek <michal.simek@....com>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: dwc3: xilinx: Prevent spike in reset signal

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@...ic.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@...ic.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@...ic.nl
W: www.topic.nl