Skip to content

Allow support of crystals other than 12MHz (#1024) #1272

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Mar 21, 2023
Merged

Allow support of crystals other than 12MHz (#1024) #1272

merged 11 commits into from
Mar 21, 2023

Conversation

@andygpz11
Copy link
Contributor

@andygpz11 andygpz11 commented Feb 21, 2023

Improve the current hardcoded PLL and clock set-up

@andygpz11
Copy link
Contributor Author

andygpz11 commented Feb 21, 2023 •
edited
Loading

I have just pushed some changes for comment which I think address the XOSC and system frequency related hard-codings in the SDK.
Making set_sys_clock_pll() and check_sys_clock_khz() properly general purpose wasn’t possible without changing their APIs so, for now, I left them: This is because neither of them currently support ’refdiv’ setting other than 1. I have coned a new #define to keep things in step along with a tripwire to enforce this.
Having used vcocalc.py it is not clear to me that refdiv settings other than 1 can sensibly be used anyway.
A few ‘pico-examples’ will need updating to use the definitions I have added to platform_defs.h

I expect some review changes but I'd like to do some more testing of this before it is merged anyway.

kilograham
kilograham requested changes Feb 21, 2023
View reviewed changes
lurch
lurch reviewed Feb 22, 2023
View reviewed changes
lurch
lurch reviewed Feb 22, 2023
View reviewed changes
liamfraser
liamfraser reviewed Feb 22, 2023
View reviewed changes
andygpz11
andygpz11 commented Feb 22, 2023
View reviewed changes
Copy link
Contributor Author

@andygpz11 andygpz11 left a comment

Choose a reason for hiding this comment

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

Latest version. I think I have addressed all comments.
I also found that one of the asserts in pll_init() was non-functional due to an error of scaling and also that check_sys_clock_khz() should not use clock_get_hz() but the XOSC_MHZ define instead.
Apparently the PLL ref divs inputs are always connected to the output of the XOSC cell and therefore the (possible) multiplexing and division present in the clock switching is not applicable to them.

kilograham
kilograham requested changes Feb 22, 2023
View reviewed changes
kilograham
kilograham requested changes Feb 22, 2023
View reviewed changes
lurch
lurch reviewed Feb 23, 2023
View reviewed changes
lurch
lurch reviewed Feb 23, 2023
View reviewed changes
@liamfraser
Copy link
Contributor

liamfraser commented Feb 23, 2023

This looks good to me, apart from the btstack related commit

@andygpz11
Copy link
Contributor Author

andygpz11 commented Feb 23, 2023 •
edited
Loading

This looks good to me, apart from the btstack related commit

Thanks and agreed. I found I could not push to my branch because there was another push that I hadn't got locally.
So I did a 'git pull --rebase' and then a push. It was not enough for me to rebase my local branch from develop which is what I also tried.

kilograham
kilograham requested changes Feb 23, 2023
View reviewed changes
@kilograham
Copy link
Contributor

kilograham commented Feb 23, 2023

This looks good to me, apart from the btstack related commit

Thanks and agreed. I found I could not push to my branch because there was another push that I hadn't got locally. So I did a 'git pull --rebase' and then a push. It was not enough for me to rebase my local branch from develop which is what I also tried.

well you have picked up some other stuff it seems (bt license)

@andygpz11 andygpz11 force-pushed the support_xtals_other_than_12mhz branch from d83a4a0 to d4b975a Compare February 23, 2023 14:54
@andygpz11 andygpz11 mentioned this pull request Feb 24, 2023
Closed
lurch
lurch reviewed Feb 24, 2023
View reviewed changes
kilograham
kilograham requested changes Mar 1, 2023
View reviewed changes
Copy link
Contributor

@kilograham kilograham left a comment

Choose a reason for hiding this comment

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

please read thru all pre-existing review comments, and issue comments - they keep getting ignored

@andygpz11 andygpz11 self-assigned this Mar 2, 2023
kilograham
kilograham reviewed Mar 2, 2023
View reviewed changes
Copy link
Contributor

@kilograham kilograham left a comment

Choose a reason for hiding this comment

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

Note all the new user overridable defines should also have PICO_CONFIG comments

@kilograham kilograham assigned kilograham and unassigned andygpz11 Mar 3, 2023
@lurch
Copy link
Contributor

lurch commented Mar 5, 2023

The tag::pll_settings block has now been totally removed - could it (or something very similar) be restored please? As a reminder, it gets auto-included into Section 2.18.3. of https://datasheets.raspberrypi.com/rp2040/rp2040-datasheet.pdf

@kilograham kilograham requested a review from liamfraser March 19, 2023 23:47
liamfraser
liamfraser previously approved these changes Mar 20, 2023
View reviewed changes
Copy link
Contributor

@liamfraser liamfraser left a comment

Choose a reason for hiding this comment

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

I think this looks good

lurch
lurch reviewed Mar 21, 2023
View reviewed changes
src/rp2_common/hardware_clocks/include/hardware/clocks.h Outdated
//
// The two PLLs use the crystal oscillator output directly as their reference frequency input; the PLLs reference
// frequency cannot be reduced by the dividers present in the clocks block. The crystal frequency is defined by `XOSC_KHZ` or
// `XOSC_MKHZ`.
Copy link
Contributor

@lurch lurch Mar 21, 2023

Choose a reason for hiding this comment

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

MKHZ? 🤔

@kilograham kilograham self-requested a review March 21, 2023 17:08
@kilograham kilograham merged commit a42564b into raspberrypi:develop Mar 21, 2023
@kilograham
Copy link
Contributor

kilograham commented Mar 21, 2023

fixes #1024

@kilograham kilograham mentioned this pull request Mar 21, 2023
Closed
andygpz11 added a commit to andygpz11/pico-sdk that referenced this pull request May 31, 2023
@andygpz11 @kilograham
Allow support of crystals other than 12MHz (raspberrypi#1024) (raspbe…
4ed246f 
…rrypi#1272)

* Allow pre-processor overrides for Clock/PLL setup
* Use `_KHZ` rather than `_MHZ` for `XOSC_` `SYS_CLOCK_` etc definitions (`_MHZ` versions are provided for compatibility when `_KHZ` is a multiple of 1000)

Co-authored-by: graham sanderson <[email protected]>
@lurch lurch mentioned this pull request Jul 27, 2023
Closed
@lurch lurch mentioned this pull request Mar 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lurch lurch lurch left review comments

@kilograham kilograham kilograham approved these changes

@liamfraser liamfraser liamfraser left review comments

Assignees

@kilograham kilograham

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@andygpz11 @liamfraser @kilograham @lurch