Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix usb missed ep0 out packet and add recommended sync delays #405

Merged
merged 2 commits into from
Aug 1, 2022

Conversation

ithinuel
Copy link
Member

The ep0-out buffer must not be marked as available unless required.
Otherwise, the controller will acknowledge the data-out packet but won't reflect that in its status registers.

This patch forces the controller to nack the data-out phase until we have processed the setup packet.

As per 4.1.2.5.1, the access to the DPSRAM should "be considered asynchronous and not atomic".
It is recommended to write to buffer control register in two steps.

  • A first one to configure all bits but Available.
  • Wait clk_sys/clk_usb (typically 125MHz/48MHz).
  • Then set the available bit (if required).

The pico-sdk uses a blanket 12tick asm block so proposing a 12tick delay here too.

As per 4.1.2.5.1, the access to the DPSRAM should "be considered asynchronous
and not atomic".
It is recommended to write to buffer control register in two steps.
A first one to configure all bits but Available.
Wait clk_sys/clk_usb (typically 125MHz/48MHz).
Then set the available bit (if required).
The ep0-out buffer must not be marked as available unless required.
Otherwise, the controller will acknowledge the data-out packet but
won't reflect that in its status registers.

This patch forces the controller to nack the data-out phase until we have
processed the setup packet.
@jannic
Copy link
Member

jannic commented Jul 31, 2022

The pico-sdk uses a blanket 12tick asm block so proposing a 12tick delay here too.

This is a rather huge safety margin:

clk_sys/clk_usb should be at most 3 for supported frequencies (clk_usb is fixed at 48MHz and max for clk_sys is 133MHz).
A 12 tick delay would only be necessary for overclocking above 500MHz, far out of spec (but still in a range shown to be feasible on the rp2040).

And then, cortex_m::asm::delay currently waits for a larger number of clock cycles, to be safe: It doesn't know if it's running on some more advanced CPU which could compute the delay loop faster, so it takes the most conservative guess. If I count correctly, the delay(12) will actually wait for 20 clock cycles.
So the delay is wasting at least 17 clock cycles (when clk_sys is running at default speed).

But I guess this is code which would not run in a timing critical path? And even at a slower clk_sys of 48MHz (where no additional waits would be needed), it's only wasting about 0.4µs.

Conclusion: LGTM.

@jannic jannic merged commit 6dafcc1 into rp-rs:main Aug 1, 2022
@ithinuel ithinuel deleted the fix-usb-ep0-out branch August 2, 2022 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants