Open nicolube opened 1 year ago
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.
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.
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
closing, this as its not clear if there is actually an issue that needs to be addressed; feel free to reopen
@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.
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)
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.
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 theclock_sys
but this seems not to be the case, theperi_clock
is unchanged after the the reconfig.Test code:
Output with default baud of 115200
Output when baudrate is set to a 1/6 of the default speed after the second
stdio_init_all()
;First
measure_freqs()
cuts of because uart is not done printing until the clocks is re-configuring