clock_configure() will stop sysclock entirely if ROSC is not running #1589

New issue

Open

opened 2023-12-20 05:42:17 -06:00 by canique · 4 comments

canique commented

2023-12-20 05:42:17 -06:00

(Migrated from github.com)

In https://github.com/raspberrypi/pico-sdk/blob/master/src/rp2_common/hardware_clocks/clocks.c the function clock_configure() contains this code snippet

// If switching a glitchless slice (ref or sys) to an aux source, switch
// away from aux *first* to avoid passing glitches when changing aux mux.
// Assume (!!!) glitchless source 0 is no faster than the aux source.
if (has_glitchless_mux(clk_index) && src == CLOCKS_CLK_SYS_CTRL_SRC_VALUE_CLKSRC_CLK_SYS_AUX) {
    hw_clear_bits(&clock->ctrl, CLOCKS_CLK_REF_CTRL_SRC_BITS);
    while (!(clock->selected & 1u))
        tight_loop_contents();
}

The bug is in:

hw_clear_bits(&clock->ctrl, CLOCKS_CLK_REF_CTRL_SRC_BITS);

If before running clock_configure(), ROSC has been disabled (by rosc_disable() or changing clocks_hw->wake_en0 bits respectively), then this line will switch the glitchless mux to ROSC. The result is a halted sysclock.

The line does not check whether ROSC is enabled at all or whether it would be better to use XOSC.

In https://github.com/raspberrypi/pico-sdk/blob/master/src/rp2_common/hardware_clocks/clocks.c the function clock_configure() contains this code snippet ```c // If switching a glitchless slice (ref or sys) to an aux source, switch // away from aux *first* to avoid passing glitches when changing aux mux. // Assume (!!!) glitchless source 0 is no faster than the aux source. if (has_glitchless_mux(clk_index) && src == CLOCKS_CLK_SYS_CTRL_SRC_VALUE_CLKSRC_CLK_SYS_AUX) { hw_clear_bits(&clock->ctrl, CLOCKS_CLK_REF_CTRL_SRC_BITS); while (!(clock->selected & 1u)) tight_loop_contents(); } ``` The bug is in: ```c hw_clear_bits(&clock->ctrl, CLOCKS_CLK_REF_CTRL_SRC_BITS); ``` If before running clock_configure(), ROSC has been disabled (by rosc_disable() or changing clocks_hw->wake_en0 bits respectively), then this line will switch the glitchless mux to ROSC. The result is a halted sysclock. The line does not check whether ROSC is enabled at all or whether it would be better to use XOSC.

peterharperuk commented

2023-12-20 05:45:24 -06:00

(Migrated from github.com)

Any comment @andygpz11 ?

andygpz11 commented

2023-12-20 08:16:58 -06:00

(Migrated from github.com)

@peterharperuk It does appear the SDK does not automatically handle the case where ROSC has been disabled after boot.

@canique For now, can you:

Re-enable ROSC
Make the clock selection(s) you want
Disable ROSC

I think we might look to improve this behaviour although, as a general rule, we try to keep the SDK very thin and provide all the APIs needed to allow full control of the RP2040.

Thanks.

@peterharperuk It does appear the SDK does not automatically handle the case where ROSC has been disabled after boot. @canique For now, can you: - Re-enable ROSC - Make the clock selection(s) you want - Disable ROSC ? I think we might look to improve this behaviour although, as a general rule, we try to keep the SDK very thin and provide all the APIs needed to allow full control of the RP2040. Thanks.

canique commented

2023-12-20 08:21:19 -06:00

(Migrated from github.com)

For a developer an error like this is extremely hard to find. Pretty impossible to debug, hard to spot (I had to write a little framework to find it), and undocumented.

The reference manual says ROSC can be disabled for power saving purposes. It does not say that changing the clock speed in the SDK breaks everything if ROSC has been disabled.

For a developer an error like this is extremely hard to find. Pretty impossible to debug, hard to spot (I had to write a little framework to find it), and undocumented. The reference manual says ROSC can be disabled for power saving purposes. It does not say that changing the clock speed in the SDK breaks everything if ROSC has been disabled.

andygpz11 commented

2023-12-20 08:34:37 -06:00

(Migrated from github.com)

I am sorry this was hard to discover. We will improve the documentation to make this clear and consider if the SDK should automatically handle this case.