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

Implement RP2350 initialization clocks, uart, and (partial) USB support #4655

Merged
merged 7 commits into from
Dec 17, 2024

Conversation

cibomahto
Copy link
Contributor

This patch enables the clocks, uart, and partial USB support for the 2350.

I left the default console as 'uart' because if it is set to USB, using 'fmt' causes a panic. USB transfers work fine if only a single byte is transferred at a time.

Another issue is that the clock speed for RP2350 is set to 125MHz, because there were a number of places where this was hardcoded when setting up peripherals. This should at least be increased to 150MHz, perhaps as a new constant in machine_rp2_2xxxx.

@cibomahto cibomahto mentioned this pull request Dec 12, 2024
@deadprogram
Copy link
Member

Once you make the fix I suggest @cibomahto you should rebase this branch against the rp2350-add branch, since it is now rebased against the latest dev. If you do that, this PR should pass all CI tests.

@cibomahto cibomahto force-pushed the rp2350-add-cibomahto2 branch from adecf53 to 2333d38 Compare December 13, 2024 20:19
@deadprogram
Copy link
Member

@cibomahto I had to rebuild the cached LLVM because of reasons unrelated to this PR.

All tests are now passing! 🥳

@deadprogram
Copy link
Member

@cibomahto looks like you need to run go fmt on src/machine/machine_rp2350_rom.go

@cibomahto
Copy link
Contributor Author

@deadprogram Everything passes again

@deadprogram
Copy link
Member

I just confirmed that the bootloader changes work as expected @cibomahto great work!

@soypat IMO this PR can now be merged into #4459 and that PR can be set to ready for review

Copy link
Contributor

@soypat soypat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can merge this but I can see a lot of things I'm going to want to unexport before next release

@deadprogram
Copy link
Member

but I can see a lot of things I'm going to want to unexport before next release

@soypat do you mean functions that you think should not be exported from the package?

@soypat
Copy link
Contributor

soypat commented Dec 17, 2024

@deadprogram yes. A cursory glance showed a lot of USB and Clock exported functions which did not need to be exported or could better be hid from package scope behind methods on a type

@deadprogram
Copy link
Member

A cursory glance showed a lot of USB and Clock exported functions which did not need to be exported or could better be hid from package scope behind methods on a type

That is probably the case with the current rp2040 implementation as well. In any case we can address those things separately.

@deadprogram
Copy link
Member

Now rebasing this branch into the rp2350-add branch for further development. Thank you very much @cibomahto and @soypat

@deadprogram deadprogram merged commit ba1919f into tinygo-org:rp2350-add Dec 17, 2024
17 checks passed
@cibomahto
Copy link
Contributor Author

@deadprogram yes. A cursory glance showed a lot of USB and Clock exported functions which did not need to be exported or could better be hid from package scope behind methods on a type

Yes, apologies that I'm not fluent enough in Go to know the right way to do this yet :-)

@cibomahto
Copy link
Contributor Author

cibomahto commented Dec 17, 2024

Another thing for whatever list these go on, the USB driver both chips are almost exactly the same*, but the CMSIS register names are slightly different between the two. It would probably be best to merge them back together if possible.

  • Except for an extra init in the rp2350, and the rp2350 not needing the errata from the rp2040

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