pico-sdk icon indicating copy to clipboard operation
pico-sdk copied to clipboard

Changing system clock sets peripheral clock to 48MHz

Open msearle5 opened this issue 3 years ago • 5 comments

At boot, the system and peripheral clocks both appear to be 125MHz. If the system clock is later set (to any speed, even 125MHz), the peripheral clock is set to 48MHz. I haven't measured the peripheral clock directly, but I have used a modified version of the FatFS "example" app: https://github.com/msearle5/no-OS-FatFS-SD-SPI-RPi-Pico - which has a "cpu" command to set the system clock and a "spi" command to set the SPI (divided from peripheral) clock. At boot, SPI clocks of 62.5, 31.25, 20.833 etc. MHz can be set implying 125MHz peripheral clock, while after setting the system clock only clocks of 24MHz, 12MHz etc. can be set implying 48MHz peripheral clock. Setting the system clock with set_sys_clock_khz() calls set_sys_clock_pll() in pico-sdk/src/rp2_common/pico_stdlib/stdlib.c. line 39..64 and line 64 looks as if it's setting clk_peri to 48MHz from the USB clock in all cases, while I would expect it to be set to the same as the system clock. (Maybe the fault is my expectation, and this is intentional. But if nothing else it was surprising to me, so I would recommend documenting it - the relevant section of the manual here https://raspberrypi.github.io/pico-sdk-doxygen/group__pico__stdlib.html doesn't mention the peripheral clock.)

msearle5 avatar May 30 '22 21:05 msearle5

Not sure where pico-sdk/src/rp2_common/pico_stdlib/stdlib.c's set_sys_clock_pll() is used. The default sdk clock setup happens in pico-sdk/src/rp2_common/pico_runtime/runtime.c: runtime_init() -> clocks_init(). Which gives you a clock setup like https://www.bruelltuete.com/picoclocks.txt (last time I checked).

bruelltuete avatar Jun 10 '22 16:06 bruelltuete

Yes, the default clock setup behaves as I would expect with system and peripheral clocks both 125MHz, as in your diagarm (picoclocks.txt). The problem shows up only when I set the system clock with set_sys_clock_khz(125000). (stdlib.c:126) This calls set_sys_clock_pll (stdlib.c:129: "set_sys_clock_pll(vco, postdiv1, postdiv2);"). That function is at stdlib.c:39, and afterwards it appears that the clocks are set up like this:

rosc @ 1-12 MHz

xosc @ 12 MHz
    |
    \-- clk_ref @ 12 MHz
            |
            \-- watchdog tick 1:12, @ 1 Mhz
            |       |
            |       \-- timer/alarm: get_absolute_time() in micro seconds
            |
            \-- pll_sys @ 125 MHz
            |       |
            |       \-- clk_sys @ 125 MHz           
            |
            \-- pll_usb @ 48 MHz
                  |               
                  \-- clk_peri @ 48 MHz, for UART but not I2C
                  |       |
                  |       \-- DMA pacing timers
                  |
                  \-- clk_usb
                  |
                  \-- clk_adc
                  |
                  \-- clk_rtc 1:1024 @ 46,875 Hz
                            |
                            \-- RTC 1:46875 @ 1Hz

That is the peripheral clock is running at 48MHz, not 125MHz. It has been seen before (https://forums.raspberrypi.com/viewtopic.php?p=1995296&hilit=48MHz#p1995296 and other posts on that forum) but I have not seen any actual fix for the issue there - by which I mean a method of running clk_peri and clk_sys at the same speed, when that speed is changed at run time and is not 125MHz. (Using PIO to get a higher speed SPI port may work, for example. But that looks like a workaround that should not be necessary, considering that as far as I can tell this is not required by any actual limitations of the clock hardware - it's the SDK doing something unexpected.)

msearle5 avatar Jun 10 '22 18:06 msearle5

Ah yes, sorry missed that part on your original question... looks like set_sys_clock_khz does too much stuff. git-blame looks like this function hasn't been touched in ages and is prob a left-over from the early days.

Have you figured out how to set the system clock only? If you haven't changed any other clock setup, a single call like: clock_configure(clk_sys, CLOCKS_CLK_SYS_CTRL_SRC_VALUE_CLKSRC_CLK_SYS_AUX, CLOCKS_CLK_SYS_CTRL_AUXSRC_VALUE_CLKSRC_PLL_SYS, /* default pll freq: */ 125 * MHZ , /* new sys freq: */ 42 * MHZ); might do the job. (but will also change clk_peri because it's derived from clk_sys by default)

bruelltuete avatar Jun 11 '22 16:06 bruelltuete

I don't have a strong opinion on the behaviour. I think the intention here is that if the user is changing the system clock we should switch to a known 48MHz that won't change. Then your uart will keep working etc for all subsequent clk_sys frequency changes. I agree we should document this in the doxygen at the very least. Thoughts @kilograham @Wren6991

liamfraser avatar Jun 13 '22 10:06 liamfraser

Slightly related question: is there ever any point in having a clk_peri run faster than clk_sys?

bruelltuete avatar Jun 14 '22 15:06 bruelltuete

I changed SDK like in the post here https://forums.raspberrypi.com/viewtopic.php?t=345948 and clock doesn't change clk_peri to 48Mhz anymore and SPI is a derivative of clk_sys again I guess. It works faster when you set it to do so.

matsobdev avatar Feb 09 '23 08:02 matsobdev

@andygpz11 somewhat related to the other clock issue

kilograham avatar Feb 23 '23 22:02 kilograham

@kilograham Indeed. I will look at this after https://github.com/raspberrypi/pico-sdk/pull/1272 has landed.

andygpz11 avatar Feb 24 '23 10:02 andygpz11

See also https://github.com/raspberrypi/pico-examples/pull/40

lurch avatar Mar 05 '23 23:03 lurch

merged into develop

kilograham avatar May 26 '23 14:05 kilograham