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

2023-02-21T13:23:20-06:00

andygpz11 commented

2023-02-21 13:23:20 -06:00

(Migrated from github.com)

Improve the current hardcoded PLL and clock set-up

andygpz11 commented

2023-02-21 13:26:35 -06:00

(Migrated from github.com)

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.

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 (Migrated from github.com) requested changes

2023-02-21 17:13:39 -06:00

src/rp2040/hardware_regs/include/hardware/platform_defs.h

				`@ -37,9 +37,38 @@`

this does not belong here; better in hardware/clocks.h

this should only be a #error if the six/seven things aren't defined (we do not expect people to edit this header, we expect them to set them in their compile_defintions or via a board header)

also,

separate out the definition checks USB since it seems to be independent of SYS_CLK_MHZ, i.e. two separate checks for 3 defines, vs 1 for 6.

also presumably PLL_COMMON_REFDIV maybe should be just #ifndef-ed above all of this and defaulted to 1.

this should only be a #error if the six/seven things aren't defined (we do not expect people to edit this header, we expect them to set them in their compile_defintions or via a board header) also, separate out the definition checks USB since it seems to be independent of SYS_CLK_MHZ, i.e. two separate checks for 3 defines, vs 1 for 6. also presumably PLL_COMMON_REFDIV maybe should be just #ifndef-ed above all of this and defaulted to 1.

src/rp2_common/pico_stdlib/stdlib.c

					
				@ -64,23 +64,23 @@ void set_sys_clock_pll(uint32_t vco_freq, uint post_div1, uint post_div2) {

				        clock_configure(clk_peri,

um, if you've made a define for it, why not use it?

lurch (Migrated from github.com) reviewed

2023-02-22 03:57:45 -06:00

src/rp2040/hardware_regs/include/hardware/platform_defs.h

				`@ -37,9 +37,38 @@`

https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/hardware_pll/pll.c#L55 seems to suggest that each PLL has its own REFDIV, rather than there being a single "global" REFDIV? I.e. should there be separate defines for PLL_SYS_REFDIV and PLL_USB_REFDIV ? 🤷‍♂️

https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/hardware_pll/pll.c#L55 seems to suggest that each PLL has its own REFDIV, rather than there being a single "global" REFDIV? I.e. should there be separate defines for `PLL_SYS_REFDIV` and `PLL_USB_REFDIV` ? :man_shrugging:

lurch (Migrated from github.com) reviewed

2023-02-22 04:01:49 -06:00

src/rp2_common/hardware_clocks/clocks.c

Apologies for the naive question, but should this value also get its own #define? Or is this a value that can't be (sensibly) changed?

Apologies for the naive question, but should this value also get its own `#define`? Or is this a value that can't be (sensibly) changed?

liamfraser (Migrated from github.com) reviewed

2023-02-22 04:02:30 -06:00

src/rp2040/hardware_regs/include/hardware/platform_defs.h

				`@ -37,9 +37,38 @@`

Each pll does have it's own reference divider. In practice the reference just has to be in a certain range and can be divided if it is not.

liamfraser (Migrated from github.com) reviewed

2023-02-22 04:06:36 -06:00

src/rp2040/hardware_structs/include/hardware/structs/clocks.h

This is actually not true. The PLLs are directly connected to the xosc as their reference clock.

andygpz11 (Migrated from github.com) reviewed

2023-02-22 04:44:08 -06:00

src/rp2_common/hardware_clocks/clocks.c

@lurch I get your point but have no opinion on this.
@kilograham?

@lurch I get your point but have no opinion on this. @kilograham?

kilograham (Migrated from github.com) reviewed

2023-02-22 09:36:01 -06:00

src/rp2_common/hardware_clocks/clocks.c

i have no idea whu the RTC is set to 46875 so probably a define, with integer math if it has a sensible reason

andygpz11 (Migrated from github.com) reviewed

2023-02-22 09:48:19 -06:00

andygpz11 (Migrated from github.com) left a comment

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.

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 (Migrated from github.com) requested changes

2023-02-22 09:52:55 -06:00

src/rp2040/hardware_regs/include/hardware/platform_defs.h

				`@ -37,9 +37,38 @@`

is this 100% true; i mean could you set it a little off

Also i wonder if we should specify this and SYS_CLK in khz

is this 100% true; i mean could you set it a little off Also i wonder if we should specify this and SYS_CLK in khz

kilograham (Migrated from github.com) reviewed

2023-02-22 09:53:27 -06:00

src/rp2040/hardware_regs/include/hardware/platform_defs.h

				`@ -37,9 +37,38 @@`

the first part of this is not resolved

andygpz11 (Migrated from github.com) reviewed

2023-02-22 09:57:22 -06:00

src/rp2_common/hardware_clocks/clocks.c

I'd guess 46875 Hz is the (geometrically) nearest frequency to the common 32 or 32.768 kHz slow clock rates found on many microcontrollers that (on RP2040) can be obtained using a 2^N divisor from the 48 MHz USB clock & where N is an integer (1024).
Using a factional divider ratio, which the clock switching hardware does support, would present "jittery" RTC timing I think.

I'd guess 46875 Hz is the (geometrically) nearest frequency to the common 32 or 32.768 kHz slow clock rates found on many microcontrollers that (on RP2040) can be obtained using a 2^N divisor from the 48 MHz USB clock & where N is an integer (1024). Using a factional divider ratio, which the clock switching hardware does support, would present "jittery" RTC timing I think.

andygpz11 (Migrated from github.com) reviewed

2023-02-22 10:03:47 -06:00

src/rp2040/hardware_regs/include/hardware/platform_defs.h

				`@ -37,9 +37,38 @@`

Yes, you can set the USB clock 'off bit'. In fact I worked out you could use a 'PAL crystal', 4.43361875 MHz, and still meet the (appropriate) USB spec (obviously not for the bootrom of course.)
But I'm not sure how far we want to go with this...
Step one is removing hardcoded-ness that will trip unwary users, I think done.
Further steps would be to allow more or less any frequencies at build time - not done.
There's nothing stopping anyone doing that by changing the defines present by step one though.

Yes, you can set the USB clock 'off bit'. In fact I worked out you could use a 'PAL crystal', 4.43361875 MHz, and still meet the (appropriate) USB spec (obviously not for the bootrom of course.) But I'm not sure how far we want to go with this... Step one is removing hardcoded-ness that will trip unwary users, I think done. Further steps would be to allow more or less any frequencies at build time - not done. There's nothing stopping anyone doing that by changing the defines present by step one though.

kilograham (Migrated from github.com) requested changes

2023-02-22 14:09:18 -06:00

src/rp2040/hardware_regs/include/hardware/platform_defs.h

				`@ -37,9 +37,38 @@`

Also i wonder if we should specify this and SYS_CLK in khz

I guess i meant, "let's specify these in khz" since i don't think we want to force people to use multiples of Mhz

> Also i wonder if we should specify this and SYS_CLK in khz I guess i meant, "let's specify these in khz" since i don't think we want to force people to use multiples of Mhz

lurch (Migrated from github.com) reviewed

2023-02-22 17:08:03 -06:00

src/rp2040/hardware_regs/include/hardware/platform_defs.h

				`@ -37,9 +37,38 @@`

Also, given that these are ultimately derived from XOSC_MHZ, perhaps it makes sense to list them in this file atfer XOSC_MHZ rather than before?

Also, given that these are ultimately derived from XOSC_MHZ, perhaps it makes sense to list them in this file _atfer_ `XOSC_MHZ` rather than before?

lurch (Migrated from github.com) reviewed

2023-02-22 18:09:43 -06:00

src/rp2_common/hardware_clocks/include/hardware/clocks.h

					
				@ -89,6 +89,70 @@ extern "C" {

				#define KHZ 1000

#951 provides an example of why you might want to do this? 🤷‍♂️

#951 provides an example of why you _might_ want to do this? :man_shrugging:

lurch (Migrated from github.com) reviewed

2023-02-22 18:10:40 -06:00

src/rp2_common/hardware_clocks/include/hardware/clocks.h

					
				@ -89,6 +89,70 @@ extern "C" {

				#define KHZ 1000

Minor nitpick, but perhaps PLL should always be capitalised as thus?

liamfraser commented

2023-02-23 05:53:17 -06:00

(Migrated from github.com)

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

andygpz11 commented

2023-02-23 05:57:21 -06:00

(Migrated from github.com)

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.

> 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.

lurch (Migrated from github.com) reviewed

2023-02-23 06:16:23 -06:00

src/rp2040/hardware_regs/include/hardware/platform_defs.h

					
				@ -43,0 +45,4 @@

				#ifndef XOSC_KHZ

				#define XOSC_KHZ _u(12000)

				#endif

Given that this is a PICO_CONFIG value, renaming it from XOSC_MHZ to XOSC_KHZ feels like a backwards-incompatible change? 🤔
Perhaps adding

#ifdef XOSC_MHZ
#define XOSC_KHZ (XOSC_MHZ * KHZ)
#endif

is "enough" to fix that? 🤷‍♂️

Given that this is a PICO_CONFIG value, renaming it from `XOSC_MHZ` to `XOSC_KHZ` feels like a backwards-incompatible change? :thinking: Perhaps adding ``` #ifdef XOSC_MHZ #define XOSC_KHZ (XOSC_MHZ * KHZ) #endif ``` is "enough" to fix that? :man_shrugging:

kilograham (Migrated from github.com) requested changes

2023-02-23 07:45:05 -06:00

src/rp2_common/hardware_clocks/include/hardware/clocks.h

					
				@ -89,6 +89,70 @@ extern "C" {

				#define KHZ 1000

we should #ifdef setting these defaults with USB lock being 48Mhz

similarly with SYS_CLK_KHZ == 125000

src/rp2_common/hardware_clocks/include/hardware/clocks.h

					
				@ -89,6 +89,70 @@ extern "C" {

				#define KHZ 1000

				#define MHZ 1000000

				/// \tag::pll_settings[]

note the "tag" here hoists the comment into the documentation, so it may be worth checking whether this new content make sense in that context (cc @lurch)

kilograham (Migrated from github.com) reviewed

2023-02-23 07:46:15 -06:00

src/rp2040/hardware_regs/include/hardware/platform_defs.h

				`@ -37,9 +37,38 @@`

and now there is no #error

and now there is no `#error`

kilograham (Migrated from github.com) reviewed

2023-02-23 07:48:06 -06:00

src/rp2040/hardware_regs/include/hardware/platform_defs.h

				`@ -37,9 +37,38 @@`

i was referring to USB_CLK_MHZ and SYS_CLK_MHZ (github may have been confusing here as i clicked on USB_CLK_MHZ but it shouws a comment about XOSC_MHZ)

Can XOSC actually be specified in non MHZ increments? (I had assumed not)

i was referring to USB_CLK_MHZ and SYS_CLK_MHZ (github may have been confusing here as i clicked on USB_CLK_MHZ but it shouws a comment about XOSC_MHZ) Can XOSC actually be specified in non MHZ increments? (I had assumed not)

kilograham commented

2023-02-23 07:50:08 -06:00

(Migrated from github.com)

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)

> > 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)

lurch (Migrated from github.com) reviewed

2023-02-23 07:54:25 -06:00

src/rp2_common/hardware_clocks/include/hardware/clocks.h

					
				@ -89,6 +89,70 @@ extern "C" {

				#define KHZ 1000

				#define MHZ 1000000

				/// \tag::pll_settings[]

The documentation just grabs whatever appears between the start and end tags - see the top of page 232 of https://datasheets.raspberrypi.com/rp2040/rp2040-datasheet.pdf for where this tag-content is displayed currently (and its surrounding context).
So yeah the "settings below" part should probably be moved outside of the tags.

The documentation just grabs whatever appears between the start and end tags - see the top of page 232 of https://datasheets.raspberrypi.com/rp2040/rp2040-datasheet.pdf for where this tag-content is displayed currently (and its surrounding context). So yeah the "settings below" part should probably be moved outside of the tags.

lurch (Migrated from github.com) reviewed

2023-02-23 09:58:14 -06:00

src/rp2040/hardware_regs/include/hardware/platform_defs.h

					
				@ -43,0 +45,4 @@

				#ifndef XOSC_KHZ

				#define XOSC_KHZ _u(12000)

				#endif

bump

**bump**

lurch (Migrated from github.com) reviewed

2023-02-24 11:07:51 -06:00

src/rp2_common/hardware_xosc/xosc.c

				`@ -6,18 +6,20 @@`

See #644 for context about why this was set to 50MHz ?

kilograham (Migrated from github.com) requested changes

2023-03-01 15:20:12 -06:00

kilograham (Migrated from github.com) left a comment

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

src/rp2040/hardware_regs/include/hardware/platform_defs.h

					
				@ -43,0 +45,4 @@

				#ifndef XOSC_KHZ

				#define XOSC_KHZ _u(12000)

				#endif

indeed, bump

src/rp2040/hardware_regs/include/hardware/platform_defs.h

					
				@ -43,0 +56,4 @@

				#define USB_CLK_KHZ _u(48000)

				#endif

				// For backwards compatibility define XOSC_MHZ if the frequency is indeed an integer number of Mhz.

pretty sure i asked for this to be #ifndef-ed

src/rp2_common/hardware_clocks/include/hardware/clocks.h

					
				@ -89,6 +89,70 @@ extern "C" {

				#define KHZ 1000

bump

kilograham (Migrated from github.com) reviewed

2023-03-02 11:16:40 -06:00

kilograham (Migrated from github.com) left a comment

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

src/rp2_common/hardware_clocks/include/hardware/clocks.h

					
				@ -89,6 +89,70 @@ extern "C" {

				#define KHZ 1000

"these" defaults for VCO, DIV1, DIV2 only make sense when USB clock is 48Mhz, so #ifdef setting them based on USB clock being 48Mhz

"these" defaults for VCO, DIV1, DIV2 only make sense when USB clock is 125Mhz, so #ifdef setting them based on USB clock being 48Mhz

is this assertion strictly true due to rounding?

lurch commented

2023-03-05 17:57:43 -06:00

(Migrated from github.com)

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

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

liamfraser (Migrated from github.com) reviewed

2023-03-20 05:28:02 -05:00

liamfraser (Migrated from github.com) left a comment

No description provided.

I think this looks good

lurch (Migrated from github.com) reviewed

2023-03-20 19:55:16 -05:00

src/rp2_common/hardware_clocks/include/hardware/clocks.h

					
				@ -89,6 +89,70 @@ extern "C" {

				#define KHZ 1000

MKHZ? 🤔

MKHZ? :thinking:

kilograham (Migrated from github.com) approved these changes

2023-03-21 12:10:02 -05:00

kilograham commented

2023-03-21 12:49:55 -05:00

(Migrated from github.com)

fixes #1024

Sign in to join this conversation.

No reviewers

kilograham

No labels

compiler version issues

needs reporter detail

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-raspberrypi/pico-sdk!1272

				`@ -64,23 +64,23 @@ void set_sys_clock_pll(uint32_t vco_freq, uint post_div1, uint post_div2) {`
				`clock_configure(clk_peri,`

Rows
Columns