rp2/clocks_extra: Tidy up and re-sync.#19419
Open
Gadgetoid wants to merge 2 commits into
Open
Conversation
Required by the include-based linker script support that follows. Signed-off-by: Phil Howard <github@gadgetoid.com>
Bring the port's runtime_init_clocks() in line with the pico-sdk 2.3.0 version, since it was originally derived from this function. PLL_COMMON_REFDIV is deprecated in the SDK in favour of the separate PLL_SYS_REFDIV and PLL_USB_REFDIV macros; use those so a board that only sets the new per-PLL values is honoured, and so the port keeps building if the deprecated macro is eventually removed. Configure the divide-by-1 clocks (ref, sys, usb, adc, peri, hstx) with clock_configure_undivided(), and clk_rtc with clock_configure_int_divider(), as the SDK now does. Both avoid the 64-bit division that clock_configure() pulls in to compute a fractional divider. Drop the --wrap=runtime_init_clocks linker flag: the SDK declares runtime_init_clocks() as __weak, so a plain strong definition in the port overrides it, which makes the wrap redundant. Also switch the HSTX guard to HAS_HSTX and take RTC_CLOCK_FREQ_HZ from hardware/rtc.h, matching upstream. No functional change to the clock configuration on existing boards. Signed-off-by: Phil Howard <github@gadgetoid.com>
Contributor
Author
|
Note this change came off the back of investigating whether eliminating div64 by changing the clock functions would benefit anything. Sadly it's also used by:
So, no dice 😆 sadly because libgcc's __udivmoddi4 is 832b and hazard3 allegedly drags in ~3.3kb of 64bit mayhem. Yikes. Edit, I have a strong suspicion that |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19419 +/- ##
=======================================
Coverage 98.51% 98.51%
=======================================
Files 177 177
Lines 22927 22927
=======================================
Hits 22586 22586
Misses 341 341 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Code size report: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: based on SDK 2.3.0 bump commit, since this is a Pico SDK 2.3.0 bound tidyup.
Summary
Bring the port's runtime_init_clocks() in line with the pico-sdk 2.3.0 version, since it was originally derived from this function.
Testing
Verified the drop of 64bit for clocks_extra, which is useless because it's pulled in elsewhere, but is correct and matches upstream Pico SDK.
Verified the __weak SDK
runtime_init_clocksis overriden by our version without the extra monkeying about. @projectgus original rationale for using linker wrap was to "avoid a code size increase" - see: 5dcffb5 - I don't think this still holds. Or ever did?? (in either case it's a small trampoline into the custom clocks init)Trade-offs and Alternatives
Since this code was originally copied from the Pico SDK function, and says as much, I think it's prudent it be periodically updated to track it.
Nonetheless this is part change for change's sake and part a meaningful guard against future breakage. We flip to a more permissive HTSX check and drop the use of a deprecated macro (
PLL_COMMON_REFDIV).The swap from a __wrap to using the weak ref in particular is heavy-handed, but I'm in favour of not doing things that aren't necessary (keeps the huge CMakeLists.txt that little bit less huge), though it could be argued that the "explicit"ness of wrap is better than the implicit nature of overriding __weak symbols.
Regrettably we also lose a little history:
// todo amy, what is this N1,2,4 meant to mean?😆 (clue it's N = 1, 2 or 4)Generative AI
I used generative AI tools when creating this PR, but a human has checked the
code and is responsible for the code and the description above.