overwrite grstctl on edpt_disable

[Iktek]

Solves #1453

[cr1901]

From #1453

For my opinion it's completely ok to clear the whole grstctl because all bits (except the TXFNUM) are self-cleared.

This looks reasonable/fine to me in light of that (unless it's somehow possible to get to this code before all the bits self-clear).

If you look at my comments on PRs/Issues over the years, you'll find that I had a number of bugs where I do |= instead of doing =. This appears to be no exception. Also, epnum - 1 in the linked line is also wrong, but that was fixed some time ago :P.

[hathach]

Sorry for late response, and thank for your PR. This somehow falls off my radar. The code is spot-on, though I think we can actually set the fifo number, and flush it at the same time. Let me know if you have time to test that out, otherwise, I will merge this as it is and test the combined code later on.

[hathach]

I think we can actually merge them into 1 line as

dwc2->grstctl = (epnum << GRSTCTL_TXFNUM_Pos) | GRSTCTL_TXFFLSH;

from linux dwc2

https://github.com/torvalds/linux/blob/babf0bb978e3c9fce6c4eba6b744c8754fd43d8e/drivers/usb/dwc2/core.c#L823-L825

[Iktek]

Hi, @hathach, unfortunately I currently don't have the test-equipment on desk any more currently, so no further possibility to test. I'll do further tests with this software in the 2nd half of the year and also might test that change then, but I'd propose to first merge the current solution. Regards Pascal Am 26.05.22 um 16:34 schrieb Ha Thach:

…

***@***.**** approved this pull request. Sorry for late response, and thank for your PR. This somehow falls off my radar. The code is spot-on, though I think we can actually set the fifo number, and flush it at the same time. Let me know if you have time to test that out, otherwise, I will merge this as it is and test the combined code later on. -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- In src/portable/synopsys/dwc2/dcd_dwc2.c <#1454 (comment)>: > @@ -796,7 +796,7 @@ static void dcd_edpt_disable (uint8_t rhport, uint8_t ep_addr, bool stall) } // Flush the FIFO, and wait until we have confirmed it cleared. - dwc2->grstctl |= (epnum << GRSTCTL_TXFNUM_Pos); + dwc2->grstctl = (epnum << GRSTCTL_TXFNUM_Pos); I think we can actually merge them into 1 line as dwc2->grstctl = (epnum << GRSTCTL_TXFNUM_Pos) | GRSTCTL_TXFFLSH; — Reply to this email directly, view it on GitHub <#1454 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF6RRCUMZ5HQTWMUT5R4IM3VL6DWVANCNFSM5U6EYHUQ>. You are receiving this because you authored the thread.Message ID: ***@***.***> [ { ***@***.***": "http://schema.org", ***@***.***": "EmailMessage", "potentialAction": { ***@***.***": "ViewAction", "target": "#1454 (review)","url": "#1454 (review)", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { ***@***.***": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

-- Mit freundlichen Grüßen Pascal Speck

______________________________________________ IKTEK - Informations und Kommunikationstechnik Oranienstrasse 49 35716 Dietzhölztal Tel: 02774/9236918 Fax: 02774/9236919 Mail: ***@***.***

[hathach]

Hi, @hathach, unfortunately I currently don't have the test-equipment on desk any more currently, so no further possibility to test. I'll do further tests with this software in the 2nd half of the year and also might test that change then, but I'd propose to first merge the current solution. Regards Pascal Am 26.05.22 um 16:34 schrieb Ha Thach:

No problems, I think it is safe to do so when observing code from linux driver. I will make the change to pr and merge asap.