Skip to content

USB: Check the peripheral frequency for a stable connection #884

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rnd-ash
Copy link
Contributor

@rnd-ash rnd-ash commented Jun 8, 2025

Summary

This PR fixes an issue that is mainly only found on the V2 clocking API.

With the V2 clocking API giving the user the freedom to configure clocks as they wish, the USB peripheral can be clocked to any frequency. This causes massive stability issues.

Therefore, the USBBus constructor now forces the user to provide a 48Mhz constructor in the UsbBus::new function, since this will be used by almost all users. This function returns an error if the provided USB clock Freq is not 48Mhz.

In case the user wishes to provide a faster running USB clock (GCLK_USB can run up to 60Mhz) - For the crazy reason of connecting 2 SAMx chips together over a faster USB bus, I have provided unsafe fn UsbBus::new_unchecked function to bypass the clock freq check

@jbeaurivage
Copy link
Contributor

LGTM, there is just a minor doc issue causing CI to fail.

@ianrrees
Copy link
Contributor

Do we have prior art for a constructor putting requirements on the supplied clock's speed? It seems like returning a Result<> where we didn't have one before, is a breaking change that's not actually adding any utility because realistically firmware isn't going to handle the Err() case with anything other than a panic. In the current thumbv6 context, I think someone would have to go to some effort to trigger that Err() even if they wanted to...

Let's put the requirement from the datasheet in the docstring, which is largely about the accuracy of the clock (and I suppose we have no way to check in general). There's the "CLK_USB_AHB clock should be at minimum 8 MHz" requirement too - wondering if that's feasible to check while we're in the area?

I'm also hesitant about documenting overclocking; if someone's looking to do that then they've got the ability to, but IDK if it's a good idea to tacitly encourage it (esp when we don't really do USB host, so it's not easy to test or provide examples).

@rnd-ash
Copy link
Contributor Author

rnd-ash commented Jun 12, 2025

@ianrrees the returning error when supplying the wrong clock speed is something we introduced in the ADCV2 API for the same reason, clock V2 allows you to set a clock speed far higher for a peripheral than it supports

@ianrrees
Copy link
Contributor

What about if we leave the small targets alone, except adding a note to make this change when updating to clockv2?

@rnd-ash
Copy link
Contributor Author

rnd-ash commented Jun 25, 2025

For some reason the doc pipeline is failing now and I'm not sure why, its complaining that a public enum is private

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.

3 participants