gh-raspberrypi/pico-sdk

Watch

Fork 0

[BUG] Clock div for peri_clock seems not to be working #1037

New issue

Open

opened 2022-09-29 05:08:01 -05:00 by nicolube · 7 comments

nicolube commented

2022-09-29 05:08:01 -05:00

(Migrated from github.com)

Hallo there,

I'll expect the clock div for the clock_peri not to be working correctly.

Test case 1:
clock_sys = src <- PLL_USB, src_freq <- 48MHz, target_freq <- 8MHz: works
clock_peri = src <- clock_sys, src_freq <- 8MHz, target_freq <- 8MHz: works

Test case 2:
clock_sys = src <- PLL_USB, src_freq <- 48MHz, target_freq <- 48MHz: works
clock_peri = src <- clock_sys, src_freq <- 48MHz, target_freq <- 8MHz: does not work

I tested test case 2 with 24Mhz, 8MHz and 1 MHz and non of the seems to set the target freq correctly.
Also tried to change the clock src for the clock_peri to PLL_USB also does not work.

As i understand it from the datasheet the clock div for the clock_peri should work as the clock div for the clock_sys
but this seems not to be the case, the peri_clock is unchanged after the the reconfig.

Test code:

/**
 * Copyright (c) 2020 Raspberry Pi (Trading) Ltd.
 *
 * SPDX-License-Identifier: BSD-3-Clause
 */

#include <stdio.h>
#include "pico/stdlib.h"
#include "hardware/pll.h"
#include "hardware/clocks.h"
#include "hardware/structs/pll.h"
#include "hardware/structs/clocks.h"
#include "hardware/pwm.h"

void measure_freqs(void)
{
    uint f_pll_sys = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_PLL_SYS_CLKSRC_PRIMARY);
    uint f_pll_usb = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_PLL_USB_CLKSRC_PRIMARY);
    uint f_rosc = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_ROSC_CLKSRC);
    uint f_clk_sys = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_CLK_SYS);
    uint f_clk_peri = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_CLK_PERI);
    uint f_clk_usb = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_CLK_USB);
    uint f_clk_adc = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_CLK_ADC);
    uint f_clk_rtc = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_CLK_RTC);

    printf("pll_sys  = %dkHz\n", f_pll_sys);
    printf("pll_usb  = %dkHz\n", f_pll_usb);
    printf("rosc     = %dkHz\n", f_rosc);
    printf("clk_sys  = %dkHz\n", f_clk_sys);
    printf("clk_peri = %dkHz\n", f_clk_peri);
    printf("clk_usb  = %dkHz\n", f_clk_usb);
    printf("clk_adc  = %dkHz\n", f_clk_adc);
    printf("clk_rtc  = %dkHz\n", f_clk_rtc);

    // Can't measure clk_ref / xosc as it is the ref
}

int main()
{
    stdio_init_all();

    printf("Hello, world!\n");

    measure_freqs();

    // Change clk_sys to be 48MHz. The simplest way is to take this from PLL_USB
    // which has a source frequency of 48MHz
    clock_configure(clk_sys,
                    CLOCKS_CLK_SYS_CTRL_SRC_VALUE_CLKSRC_CLK_SYS_AUX,
                    CLOCKS_CLK_SYS_CTRL_AUXSRC_VALUE_CLKSRC_PLL_USB,
                    48 * MHZ,
                    48 * MHZ);

    // Turn off PLL sys for good measure
    pll_deinit(pll_sys);

    // CLK peri is clocked from clk_sys so need to change clk_peri's freq
    clock_configure(clk_peri,
                    0,
                    CLOCKS_CLK_PERI_CTRL_AUXSRC_VALUE_CLK_SYS,
                    48 * MHZ,
                    8 * MHZ);

    // Re init uart now that clk_peri has changed
    stdio_init_all();

    measure_freqs();
    printf("Hello, 48MHz");

    return 0;
}

Output with default baud of 115200

Hello, world!
pll_sys  = 125000kHz
pll_usb  = 48000kHz
rosc     = 5605kHz
clk_sys  = 125000kHz
clk_peri = 125000kHz
clk_usb  = 48000kHz
clk_ad▒▒▒ge#▒a'e#%s▒e#%▒c'▒3

Output when baudrate is set to a 1/6 of the default speed after the second stdio_init_all();

    // Re init uart now that clk_peri has changed
    stdio_init_all();
    // Fixes baudrate form unchanged clock. 48MHz / 8MHz = 6
    uart_set_baudrate(uart0, 115200/6);

First measure_freqs() cuts of because uart is not done printing until the clocks is re-configuring

Hello, world!
pll_sys  = 125000kHz
pll_usb  = 48000kHz
rosc     = 5612kHz
clk_sys  = 125000kHz
clk_peri = 125000kHz
clk_usb  = 48000kHz
clk_ad▒pll_sys  = 0kHz
pll_usb  = 48000kHz
rosc     = 5621kHz
clk_sys  = 48000kHz
clk_peri = 48000kHz
clk_usb  = 48000kHz
clk_adc  = 48000kHz
clk_rtc  = 47kHz
Hello, 48MHz

Hallo there, I'll expect the clock div for the `clock_peri` not to be working correctly. Test case 1: clock_sys = src <- PLL_USB, src_freq <- 48MHz, target_freq <- 8MHz: works clock_peri = src <- clock_sys, src_freq <- 8MHz, target_freq <- 8MHz: works Test case 2: clock_sys = src <- PLL_USB, src_freq <- 48MHz, target_freq <- 48MHz: works clock_peri = src <- clock_sys, src_freq <- 48MHz, target_freq <- 8MHz: **does not work** I tested test case 2 with 24Mhz, 8MHz and 1 MHz and non of the seems to set the target freq correctly. Also tried to change the clock src for the `clock_peri` to PLL_USB also does not work. As i understand it from the datasheet the clock div for the `clock_peri` should work as the clock div for the `clock_sys` but this seems not to be the case, the `peri_clock` is unchanged after the the reconfig. Test code: ```cpp /** * Copyright (c) 2020 Raspberry Pi (Trading) Ltd. * * SPDX-License-Identifier: BSD-3-Clause */ #include <stdio.h> #include "pico/stdlib.h" #include "hardware/pll.h" #include "hardware/clocks.h" #include "hardware/structs/pll.h" #include "hardware/structs/clocks.h" #include "hardware/pwm.h" void measure_freqs(void) { uint f_pll_sys = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_PLL_SYS_CLKSRC_PRIMARY); uint f_pll_usb = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_PLL_USB_CLKSRC_PRIMARY); uint f_rosc = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_ROSC_CLKSRC); uint f_clk_sys = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_CLK_SYS); uint f_clk_peri = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_CLK_PERI); uint f_clk_usb = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_CLK_USB); uint f_clk_adc = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_CLK_ADC); uint f_clk_rtc = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_CLK_RTC); printf("pll_sys = %dkHz\n", f_pll_sys); printf("pll_usb = %dkHz\n", f_pll_usb); printf("rosc = %dkHz\n", f_rosc); printf("clk_sys = %dkHz\n", f_clk_sys); printf("clk_peri = %dkHz\n", f_clk_peri); printf("clk_usb = %dkHz\n", f_clk_usb); printf("clk_adc = %dkHz\n", f_clk_adc); printf("clk_rtc = %dkHz\n", f_clk_rtc); // Can't measure clk_ref / xosc as it is the ref } int main() { stdio_init_all(); printf("Hello, world!\n"); measure_freqs(); // Change clk_sys to be 48MHz. The simplest way is to take this from PLL_USB // which has a source frequency of 48MHz clock_configure(clk_sys, CLOCKS_CLK_SYS_CTRL_SRC_VALUE_CLKSRC_CLK_SYS_AUX, CLOCKS_CLK_SYS_CTRL_AUXSRC_VALUE_CLKSRC_PLL_USB, 48 * MHZ, 48 * MHZ); // Turn off PLL sys for good measure pll_deinit(pll_sys); // CLK peri is clocked from clk_sys so need to change clk_peri's freq clock_configure(clk_peri, 0, CLOCKS_CLK_PERI_CTRL_AUXSRC_VALUE_CLK_SYS, 48 * MHZ, 8 * MHZ); // Re init uart now that clk_peri has changed stdio_init_all(); measure_freqs(); printf("Hello, 48MHz"); return 0; } ``` Output with default baud of 115200 ``` Hello, world! pll_sys = 125000kHz pll_usb = 48000kHz rosc = 5605kHz clk_sys = 125000kHz clk_peri = 125000kHz clk_usb = 48000kHz clk_ad▒▒▒ge#▒a'e#%s▒e#%▒c'▒3 ``` Output when baudrate is set to a 1/6 of the default speed after the second `stdio_init_all()`; ```cpp // Re init uart now that clk_peri has changed stdio_init_all(); // Fixes baudrate form unchanged clock. 48MHz / 8MHz = 6 uart_set_baudrate(uart0, 115200/6); ``` First `measure_freqs()` cuts of because uart is not done printing until the clocks is re-configuring ``` Hello, world! pll_sys = 125000kHz pll_usb = 48000kHz rosc = 5612kHz clk_sys = 125000kHz clk_peri = 125000kHz clk_usb = 48000kHz clk_ad▒pll_sys = 0kHz pll_usb = 48000kHz rosc = 5621kHz clk_sys = 48000kHz clk_peri = 48000kHz clk_usb = 48000kHz clk_adc = 48000kHz clk_rtc = 47kHz Hello, 48MHz ```

Wren6991 commented

2022-10-10 07:27:36 -05:00

(Migrated from github.com)

clk_peri does not have a divider. You'll notice there is no listing for CLK_PERI_DIV in the datasheet, since there is no such register.

`clk_peri` does not have a divider. You'll notice there is no listing for CLK_PERI_DIV in the datasheet, since there is no such register.

Wren6991 commented

2022-10-10 07:29:58 -05:00

(Migrated from github.com)

The reason your first testcase works is that you are attaching clk_peri to clk_sys, and clk_sys is divided.

In the second example, you are attaching clk_peri to clk_sys and then attempting to divide it further, which is impossible since there is no such clock divider.

The reason your first testcase works is that you are attaching `clk_peri` to `clk_sys`, and `clk_sys` is divided. In the second example, you are attaching `clk_peri` to `clk_sys` and then attempting to divide it further, which is impossible since there is no such clock divider.

LukasGossmann commented

2022-12-31 12:58:44 -06:00

(Migrated from github.com)

I just stumbled over this too while trying to get the UART down to just 40 baud which isn't possible with the default 125mhz clk_peri.
However i noticed that this bug goes a bit deeper than just the divider not working.
The clock_configure function incorrectly reports success even though it did nothing and when clock_get_hz is used it reports that the frequency of the clock has changed while the actual measured frequency returned by frequency_count_khz shows that it didn't change.
Since uart_init uses the incorrect frequency reported by clock_get_hz to calculate the clock divider i got a ton of funky baud rates and couldn't figure out why for quite a while.
Oh and also the picture in the documentation that shows the layout of the clock structure has a divider in the path of clk_peri which does not exist. (Image on page 181)

Here is a code snippet to test what i described:

#include <stdio.h>
#include "pico/stdlib.h"
#include "hardware/clocks.h"

int main()
{
    stdio_usb_init();

    gpio_init(24);
    gpio_set_dir(24, false);

    while (gpio_get(24) && !stdio_usb_connected())
        sleep_ms(500);

    puts("Hello, world!");

    printf("PICO_SDK_VERSION_STRING: %s\n", PICO_SDK_VERSION_STRING);
    printf("rp2040_chip_version: %d\n", rp2040_chip_version());
    printf("rp2040_rom_version: %d\n", rp2040_rom_version());

    uint reported_clk_peri = clock_get_hz(clk_peri);
    printf("Reported frequency before any clock_configure: %dHz\n", reported_clk_peri);

    uint measured_clk_peri = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_CLK_PERI);
    printf("Measured frequency before any clock_configure: %dkHz\n", measured_clk_peri);

    // Try do divide clk_sys by 5, returns true but did nothing
    bool clockConfigSuccess = clock_configure(clk_peri,
                                              0,
                                              CLOCKS_CLK_PERI_CTRL_AUXSRC_VALUE_CLK_SYS,
                                              125 * MHZ,
                                              25 * MHZ);

    // Returns true
    printf("clock_configure success: %d\n", clockConfigSuccess);

    //  Reports that frequency has changed (incorrect)
    reported_clk_peri = clock_get_hz(clk_peri);
    printf("Reported frequency after first clock_configure: %dHz\n", reported_clk_peri);

    // Reports that frequency hasnt changed (correct)
    measured_clk_peri = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_CLK_PERI);
    printf("Measured frequency after first clock_configure: %dkHz\n", measured_clk_peri);

    // Switching clock source to XOSC
    clockConfigSuccess = clock_configure(clk_peri,
                                         0,
                                         CLOCKS_CLK_PERI_CTRL_AUXSRC_VALUE_XOSC_CLKSRC,
                                         12 * MHZ,
                                         12 * MHZ);

    // Returns true
    printf("clock_configure success: %d\n", clockConfigSuccess);

    // Reports that frequency has changed (correct)
    reported_clk_peri = clock_get_hz(clk_peri);
    printf("Reported frequency after second clock_configure: %dHz\n", reported_clk_peri);

    // Reports that frequency has changed (correct)
    measured_clk_peri = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_CLK_PERI);
    printf("Measured frequency after second clock_configure: %dkHz\n", measured_clk_peri);

    while (1)
        ;

    return 0;
}

And the output:

Hello, world!
PICO_SDK_VERSION_STRING: 1.4.0
rp2040_chip_version: 2
rp2040_rom_version: 3
Reported frequency before any clock_configure: 125000000Hz
Measured frequency before any clock_configure: 125000kHz
clock_configure success: 1
Reported frequency after first clock_configure: 25000000Hz
Measured frequency after first clock_configure: 125000kHz
clock_configure success: 1
Reported frequency after second clock_configure: 12000000Hz
Measured frequency after second clock_configure: 12001kHz

I just stumbled over this too while trying to get the UART down to just 40 baud which isn't possible with the default 125mhz `clk_peri`. However i noticed that this bug goes a bit deeper than just the divider not working. The `clock_configure` function incorrectly reports success even though it did nothing and when `clock_get_hz` is used it reports that the frequency of the clock has changed while the actual measured frequency returned by `frequency_count_khz` shows that it didn't change. Since `uart_init` uses the incorrect frequency reported by `clock_get_hz` to calculate the clock divider i got a ton of funky baud rates and couldn't figure out why for quite a while. Oh and also the picture in the documentation that shows the layout of the clock structure has a divider in the path of `clk_peri` which does not exist. (Image on page 181) Here is a code snippet to test what i described: ```C #include <stdio.h> #include "pico/stdlib.h" #include "hardware/clocks.h" int main() { stdio_usb_init(); gpio_init(24); gpio_set_dir(24, false); while (gpio_get(24) && !stdio_usb_connected()) sleep_ms(500); puts("Hello, world!"); printf("PICO_SDK_VERSION_STRING: %s\n", PICO_SDK_VERSION_STRING); printf("rp2040_chip_version: %d\n", rp2040_chip_version()); printf("rp2040_rom_version: %d\n", rp2040_rom_version()); uint reported_clk_peri = clock_get_hz(clk_peri); printf("Reported frequency before any clock_configure: %dHz\n", reported_clk_peri); uint measured_clk_peri = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_CLK_PERI); printf("Measured frequency before any clock_configure: %dkHz\n", measured_clk_peri); // Try do divide clk_sys by 5, returns true but did nothing bool clockConfigSuccess = clock_configure(clk_peri, 0, CLOCKS_CLK_PERI_CTRL_AUXSRC_VALUE_CLK_SYS, 125 * MHZ, 25 * MHZ); // Returns true printf("clock_configure success: %d\n", clockConfigSuccess); // Reports that frequency has changed (incorrect) reported_clk_peri = clock_get_hz(clk_peri); printf("Reported frequency after first clock_configure: %dHz\n", reported_clk_peri); // Reports that frequency hasnt changed (correct) measured_clk_peri = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_CLK_PERI); printf("Measured frequency after first clock_configure: %dkHz\n", measured_clk_peri); // Switching clock source to XOSC clockConfigSuccess = clock_configure(clk_peri, 0, CLOCKS_CLK_PERI_CTRL_AUXSRC_VALUE_XOSC_CLKSRC, 12 * MHZ, 12 * MHZ); // Returns true printf("clock_configure success: %d\n", clockConfigSuccess); // Reports that frequency has changed (correct) reported_clk_peri = clock_get_hz(clk_peri); printf("Reported frequency after second clock_configure: %dHz\n", reported_clk_peri); // Reports that frequency has changed (correct) measured_clk_peri = frequency_count_khz(CLOCKS_FC0_SRC_VALUE_CLK_PERI); printf("Measured frequency after second clock_configure: %dkHz\n", measured_clk_peri); while (1) ; return 0; } ``` And the output: ``` Hello, world! PICO_SDK_VERSION_STRING: 1.4.0 rp2040_chip_version: 2 rp2040_rom_version: 3 Reported frequency before any clock_configure: 125000000Hz Measured frequency before any clock_configure: 125000kHz clock_configure success: 1 Reported frequency after first clock_configure: 25000000Hz Measured frequency after first clock_configure: 125000kHz clock_configure success: 1 Reported frequency after second clock_configure: 12000000Hz Measured frequency after second clock_configure: 12001kHz ```

kilograham commented

2023-05-26 10:38:12 -05:00

(Migrated from github.com)

closing, this as its not clear if there is actually an issue that needs to be addressed; feel free to reopen

LukasGossmann commented

2023-05-27 01:08:28 -05:00

(Migrated from github.com)

Since nothing has changed about this issue in SDK version 1.5.0 this has to be reopened.

Output of the same code snipped i posted above:

Hello, world!
PICO_SDK_VERSION_STRING: 1.5.0
rp2040_chip_version: 2
rp2040_rom_version: 3
Reported frequency before any clock_configure: 125000000Hz
Measured frequency before any clock_configure: 125000kHz
clock_configure success: 1
Reported frequency after first clock_configure: 25000000Hz
Measured frequency after first clock_configure: 125000kHz
clock_configure success: 1
Reported frequency after second clock_configure: 12000000Hz
Measured frequency after second clock_configure: 12001kHz

Here is what needs to be changed:
1: On page 181 the Figure 28 needs to have its box with the division sign removed in the path of clk_peri. (build-date: 2023-03-02, build-version: ae3b121-clean)
2: clock_configure should not report success when the divider for clk_peri is changed as there is nothing to change since that register does not even exist
3: clock_configure should not modify the value of configured_freq (which is used by clock_get_hz and lots of other stuff like baud rate calculation for example) since the frequency didn't change
4: (probably unrealistic) The struct clock_hw_tshould not contain properties that don't exist for the given clock or changing the silicon so that all clock paths are the same and the divider etc. can be used no matter what clock is being configured.

@kilograham Sorry to be so blunt but i explained the issue very clearly as well as the effects of it so i don't understand why this was 1: ignored and 2: just closed like that without anyone looking at it and bothering to verify what was reported or at least asking for clarification. Since nothing has changed about this issue in SDK version 1.5.0 this has to be reopened. Output of the same code snipped i posted above: ``` Hello, world! PICO_SDK_VERSION_STRING: 1.5.0 rp2040_chip_version: 2 rp2040_rom_version: 3 Reported frequency before any clock_configure: 125000000Hz Measured frequency before any clock_configure: 125000kHz clock_configure success: 1 Reported frequency after first clock_configure: 25000000Hz Measured frequency after first clock_configure: 125000kHz clock_configure success: 1 Reported frequency after second clock_configure: 12000000Hz Measured frequency after second clock_configure: 12001kHz ``` Here is what needs to be changed: 1: On page 181 the Figure 28 needs to have its box with the division sign removed in the path of `clk_per`i. (build-date: 2023-03-02, build-version: ae3b121-clean) 2: `clock_configure` should not report success when the divider for `clk_peri` is changed as there is nothing to change since that register does not even exist 3: `clock_configure` should not modify the value of `configured_freq` (which is used by `clock_get_hz` and lots of other stuff like baud rate calculation for example) since the frequency didn't change 4: (probably unrealistic) The struct `clock_hw_t`should not contain properties that don't exist for the given clock or changing the silicon so that all clock paths are the same and the divider etc. can be used no matter what clock is being configured.

lurch commented

2023-05-28 14:21:11 -05:00

(Migrated from github.com)

1: On page 181 the Figure 28 needs to have its box with the division sign removed in the path of clk_peri. (build-date: 2023-03-02, build-version: ae3b121-clean)

Can you create a separate issue for this at https://github.com/raspberrypi/pico-feedback please? (as that is where we track issues with the Pico databooks)

> 1: On page 181 the Figure 28 needs to have its box with the division sign removed in the path of clk_peri. (build-date: 2023-03-02, build-version: ae3b121-clean) Can you create a separate issue for this at https://github.com/raspberrypi/pico-feedback please? (as that is where we track issues with the Pico databooks)

LukasGossmann commented

2023-06-06 12:53:34 -05:00

(Migrated from github.com)

Took me a while to get around to looking at this again. There is no need for yet another issue since someone else already noticed that something isn't right in the diagram: here
They were thinking that the diagram is correct and were wondering why the documentation is missing elsewhere.
I'll add the information from this issue and a link here in theirs as this should be fixed together.

Took me a while to get around to looking at this again. There is no need for yet another issue since someone else already noticed that something isn't right in the diagram: [here](https://github.com/raspberrypi/pico-feedback/issues/222) They were thinking that the diagram is correct and were wondering why the documentation is missing elsewhere. I'll add the information from this issue and a link here in theirs as this should be fixed together.

👍 1