[crasbe]

Contribution description

The CLOCK_CORECLOCK was set to 12MHz instead of 125MHz. It is now derived from the actual PLL divider and multipliers.

Also, the range checks for the dividers were incorrect and did not actually catch out of bounds values.

Previously, the clk_ref which feeds Systick, TIMER0/1, the watchdog, ... was fed from ROSC, which runs at around 11.66MHz instead of 12MHz. Therefore the timer values were off by a small percentage.

The correct source is now used and the (as of now unused) other peripherals that require clk_ref are initialized properly.

Testing procedure

TBD.

Issues/PRs references

Prerequisite for #22910.
Partly implements things that would also be implemented by #22375.

Declaration of AI-Tools / LLMs usage:

AI-Tools / LLMs that were used are:

• Claude Code Opus 5.5 found the issues in cpu/rpxxxx: add usbdev support #22910 and provided the fixes, I rewrote the comments.

[crasbe]

Claude escalated a bit and created a whole test application that lets you output different clocks on a GPIO.
This is too specific to include it into RIOT as a test, but maybe it'll be useful for someone at some point.

/*
 * Clock test application for the RP2350 (scope + host timing)
 *
 * GPIO21 (pin 27): CLK_GPOUT0, source and divider selectable via `gpout`
 * GPIO22 (pin 29): square wave toggled every 500 µs by `toggle`
 */

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#include "busy_wait.h"
#include "periph/gpio.h"
#include "periph_cpu.h"
#include "regs/clocks.h"
#include "regs/io_bank0.h"
#include "regs/pads_bank0.h"
#include "regs/resets.h"
#include "shell.h"

#define GPOUT_PIN   21u
#define TOGGLE_PIN  22u
#define HALF_PERIOD_US 500u

/* FUNCSEL of GPIO21 for CLOCKS_GPOUT_0 on the RP2350 */
#define FUNCSEL_GPCK 9u

static const struct {
    const char *name;
    uint8_t fc_src;
    uint8_t gpout_src;
} _clocks[] = {
    { "ref",  CLOCKS_FC0_SRC_VALUE_CLK_REF,      CLOCKS_CLK_GPOUT0_CTRL_AUXSRC_VALUE_CLK_REF },
    { "xosc", CLOCKS_FC0_SRC_VALUE_XOSC_CLKSRC,  CLOCKS_CLK_GPOUT0_CTRL_AUXSRC_VALUE_XOSC_CLKSRC },
    { "rosc", CLOCKS_FC0_SRC_VALUE_ROSC_CLKSRC,  CLOCKS_CLK_GPOUT0_CTRL_AUXSRC_VALUE_ROSC_CLKSRC },
    { "sys",  CLOCKS_FC0_SRC_VALUE_CLK_SYS,      CLOCKS_CLK_GPOUT0_CTRL_AUXSRC_VALUE_CLK_SYS },
    { "peri", CLOCKS_FC0_SRC_VALUE_CLK_PERI,     CLOCKS_CLK_GPOUT0_CTRL_AUXSRC_VALUE_CLK_PERI },
    { "usb",  CLOCKS_FC0_SRC_VALUE_CLK_USB,      CLOCKS_CLK_GPOUT0_CTRL_AUXSRC_VALUE_CLK_USB },
};

/* frequency counter, the reference is CLK_REF which is assumed to be 12 MHz */
static uint32_t _fc_khz(uint8_t src)
{
    while (CLOCKS->FC0_STATUS & CLOCKS_FC0_STATUS_RUNNING_BITS) {}
    CLOCKS->FC0_REF_KHZ = 12000;
    CLOCKS->FC0_INTERVAL = 10;
    CLOCKS->FC0_MIN_KHZ = 0;
    CLOCKS->FC0_MAX_KHZ = 0xffffffff;
    CLOCKS->FC0_SRC = src;
    while (!(CLOCKS->FC0_STATUS & CLOCKS_FC0_STATUS_DONE_BITS)) {}
    return CLOCKS->FC0_RESULT >> CLOCKS_FC0_RESULT_KHZ_LSB;
}

static int _cmd_info(int argc, char **argv)
{
    (void)argc;
    (void)argv;
    printf("CLOCK_CORECLOCK: %lu Hz\n", (unsigned long)CLOCK_CORECLOCK);
    printf("CLK_REF_CTRL: 0x%08lx, CLK_REF_SELECTED: 0x%08lx (XOSC = 0x4, ROSC = 0x1)\n",
           (unsigned long)CLOCKS->CLK_REF_CTRL, (unsigned long)CLOCKS->CLK_REF_SELECTED);
    printf("TICKS TIMER0: CTRL 0x%08lx, CYCLES %lu\n",
           (unsigned long)TICKS->TIMER0_CTRL, (unsigned long)TICKS->TIMER0_CYCLES);
    return 0;
}

static int _cmd_fc(int argc, char **argv)
{
    (void)argc;
    (void)argv;
    puts("frequency counter (relative to CLK_REF, assumed to be 12 MHz):");
    for (unsigned i = 0; i < ARRAY_SIZE(_clocks); i++) {
        printf("  %-5s %8lu kHz\n", _clocks[i].name,
               (unsigned long)_fc_khz(_clocks[i].fc_src));
    }
    return 0;
}

static int _cmd_gpout(int argc, char **argv)
{
    if (argc < 3) {
        printf("usage: %s <ref|xosc|rosc|sys|peri|usb> <div>\n", argv[0]);
        return 1;
    }
    for (unsigned i = 0; i < ARRAY_SIZE(_clocks); i++) {
        if (strcmp(argv[1], _clocks[i].name) == 0) {
            uint32_t div = strtoul(argv[2], NULL, 0);
            atomic_clear(&CLOCKS->CLK_GPOUT0_CTRL, CLOCKS_CLK_GPOUT0_CTRL_ENABLE_BITS);
            CLOCKS->CLK_GPOUT0_DIV = div << CLOCKS_CLK_GPOUT0_DIV_INT_LSB;
            CLOCKS->CLK_GPOUT0_CTRL = (_clocks[i].gpout_src << CLOCKS_CLK_GPOUT0_CTRL_AUXSRC_LSB)
                                    | CLOCKS_CLK_GPOUT0_CTRL_ENABLE_BITS;
            printf("GPIO%u: %s / %lu\n", GPOUT_PIN, argv[1], (unsigned long)div);
            return 0;
        }
    }
    puts("unknown clock");
    return 1;
}

/* toggle TOGGLE_PIN every 500 µs for the given number of seconds,
 * timed either by TIMER0 or by busy_wait_us() */
static int _cmd_toggle(int argc, char **argv)
{
    if (argc < 3) {
        printf("usage: %s <timer|busy> <seconds>\n", argv[0]);
        return 1;
    }
    bool use_timer = strcmp(argv[1], "timer") == 0;
    uint32_t half_periods = strtoul(argv[2], NULL, 0) * (1000000u / HALF_PERIOD_US);

    puts("START");
    uint32_t start = TIMER0->TIMERAWL;
    uint32_t next = start;
    unsigned mismatch = 0;

    /* enable the input buffer to read back the pin level */
    *calculate_gpio_pad_register_addr(TOGGLE_PIN) |= PADS_BANK0_GPIO21_IE_BITS;
    for (uint32_t i = 0; i < half_periods; i++) {
        if (use_timer) {
            next += HALF_PERIOD_US;
            while ((int32_t)(TIMER0->TIMERAWL - next) < 0) {}
        }
        else {
            busy_wait_us(HALF_PERIOD_US);
        }
        gpio_toggle(TOGGLE_PIN);
        /* read back the pad level to check that the pin really toggles */
        busy_wait(10);
        if (!!(SIO->GPIO_IN & (1u << TOGGLE_PIN)) != !(i & 1)) {
            mismatch++;
        }
    }
    uint32_t elapsed = TIMER0->TIMERAWL - start;
    puts("STOP");
    printf("TIMER0 elapsed: %lu µs, pin read back wrong %u/%lu times\n",
           (unsigned long)elapsed, mismatch, (unsigned long)half_periods);
    return 0;
}

/* dump the IO and pad registers of a pin and sample its input */
static void _pin_dump(gpio_t pin)
{
    volatile uint32_t *pad = calculate_gpio_pad_register_addr(pin);
    volatile uint32_t *ctrl = calculate_gpio_io_ctrl_register_addr(pin);
    volatile uint32_t *status = ctrl - 1;

    /* enable the input buffer to read back the pin level */
    *pad |= PADS_BANK0_GPIO21_IE_BITS;

    unsigned high = 0;
    for (unsigned i = 0; i < 10000; i++) {
        high += (SIO->GPIO_IN >> pin) & 1;
    }
    printf("GPIO%u: CTRL 0x%08lx STATUS 0x%08lx (OUTTOPAD %u OETOPAD %u) "
           "PAD 0x%08lx, high %u/10000\n", (unsigned)pin,
           (unsigned long)*ctrl, (unsigned long)*status,
           !!(*status & IO_BANK0_GPIO21_STATUS_OUTTOPAD_BITS),
           !!(*status & IO_BANK0_GPIO21_STATUS_OETOPAD_BITS),
           (unsigned long)*pad, high);
}

static int _cmd_pins(int argc, char **argv)
{
    (void)argc;
    (void)argv;
    printf("SIO GPIO_OE 0x%08lx GPIO_OUT 0x%08lx\n",
           (unsigned long)SIO->GPIO_OE, (unsigned long)SIO->GPIO_OUT);
    _pin_dump(GPOUT_PIN);
    _pin_dump(TOGGLE_PIN);
    return 0;
}

static const shell_command_t _commands[] = {
    { "pins", "dump pin registers and sample the pin levels", _cmd_pins },
    { "info", "print clock configuration", _cmd_info },
    { "fc", "measure clocks with the frequency counter", _cmd_fc },
    { "gpout", "output a clock on GPIO21", _cmd_gpout },
    { "toggle", "toggle GPIO22 every 500 µs", _cmd_toggle },
    { NULL, NULL, NULL }
};

int main(void)
{
    /* make sure TIMER0 is out of reset */
    reset_component(RESETS_RESET_TIMER0_BITS, RESETS_RESET_TIMER0_BITS);

    gpio_init(TOGGLE_PIN, GPIO_OUT);
    gpio_init(GPOUT_PIN, GPIO_OUT);
    *calculate_gpio_io_ctrl_register_addr(GPOUT_PIN) = FUNCSEL_GPCK;

    /* output CLK_SYS / 125 = 1 MHz on GPIO21 by default */
    char *gpout_argv[] = { "gpout", "sys", "125" };
    _cmd_gpout(3, gpout_argv);

    char line_buf[SHELL_DEFAULT_BUFSIZE];
    shell_run(_commands, line_buf, SHELL_DEFAULT_BUFSIZE);
    return 0;
}

APPLICATION = clocktest
BOARD ?= rpi-pico-2-riscv
RIOTBASE ?= $(CURDIR)/..

USEMODULE += stdio_cdc_acm
USEMODULE += shell
FEATURES_REQUIRED += periph_gpio

USB_VID ?= $(USB_VID_TESTING)
USB_PID ?= $(USB_PID_TESTING)

DEVELHELP ?= 1
QUIET ?= 1

include $(RIOTBASE)/Makefile.include

[crasbe]

Testing on this PR with the clocktest application

Setting gpout ref 12, which outputs clk_ref divided by 12:

Setting gpout rosc 12, which outputs the ROSC divided by 12:

Setting gpout sys 125, which outputs the sys_clk divided by 125:

Setting toggle timer 20 will output the correct 1kHz clock and run for 20s:

Setting toggle busy 20 will output the correct 1kHz clock and run for 20s:

On RISC-V: Setting toggle busy 20 will output the correct 1kHz clock and run for 20s:

[crasbe]

Testing on master with the clocktest application

Setting gpout sys 125 results in the 1MHz output, because the system clock was running at the right speed already, but it was not announced to the rest of the code correctly:

Setting gpout ref 12 shows the incorrect frequency, since the clk_ref is actually running with 11.66MHz instead of 12MHz:

Running toggle timer 20 will output a 485Hz clock instead of 1kHz because TIMER0_CYCLES was not set to 12 but left at 24 from the boot rom.

Running toggle busy 200 (a lot longer than you'd expect) shows 10.31kHz because busy_wait_us() calculates its delay from CLOCK_CORECLOCK, which is set to 12MHz instead of 125MHz, so we have a factor of ~10ish. The test runs for 20ish seconds instead of 200 as well.

Running toggle busy 20 with the fixes, but not with CFLAGS+=-DCPU_CYCLES_PER_LOOP=2 will cause the wrong frequency to be output, because the jumps are faster than on ARM (two cycles instead of three):

[AnnsAnns]

When I review a PR I usually don't expect to mostly find errors in my old code and not the PR itself 🥀

[AnnsAnns]

The human that wrote this code must have vibecoded before AI existed, this was clearly copied from pico 1 code without fact checking /s

I will have to create a followup because it's outside of this PRs scope though (Doesn't break anything tho)

[AnnsAnns]

ty for clocking in hehe

[riot-ci]

Murdock results

✔️ PASSED

166674a cpu/rp2350_riscv: set CPU_CYCLES_PER_LOOP to 2

Success	Failures	Total	Runtime
14012	0	14013	10m:12s

Artifacts

• Documentation preview

[crasbe]

Thank you for the review :)