WIP: Bug 1668 v2 - Provide method to prevent init of SYS_CLK, when left on during 'lightsleep' #1672

2024-03-17T19:50:33-05:00

mungewell commented

2024-03-17 19:50:33 -05:00

(Migrated from github.com)

During 'lightsleep' user may want to keep some clocks running (for example PIOs), but at present microPython calls 'clock-init()' causing the SYS_CLK to be reinitialized.

This patch allows microPython to provide the previously used 'sleep_en0 & sleep_en1' signifying which clocks stayed on. As most are dependant of SYS_CLK it can be inferred that this does NOT need to (/should NOT) be re-initialized.

During 'lightsleep' user may want to keep some clocks running (for example PIOs), but at present microPython calls 'clock-init()' causing the SYS_CLK to be reinitialized. This patch allows microPython to provide the previously used 'sleep_en0 & sleep_en1' signifying which clocks stayed on. As most are dependant of SYS_CLK it can be inferred that this does NOT need to (/should NOT) be re-initialized.

lurch (Migrated from github.com) reviewed

2024-03-18 04:35:10 -05:00

src/rp2_common/hardware_clocks/clocks.c

Is the & at the end of this line supposed to be && ?

Is the `&` at the end of this line supposed to be `&&` ?

lurch (Migrated from github.com) reviewed

2024-03-18 04:36:02 -05:00

src/rp2_common/hardware_clocks/include/hardware/clocks.h

					
				@ -168,0 +171,4 @@

				 *  During a 'lightsleep' some clocks may have been deliberately left running.

				 *  By specifying 'sleep_en0' and 'sleep_en1', it can be automatically determined

				 *  that SYS_CLK (and/or it's dependants) is running and thus it is not re-initialised.

				 */

Some Doxygen function-documentation here would be great 🙂

Some Doxygen function-documentation here would be great :slightly_smiling_face:

mungewell commented

2024-03-19 14:02:22 -05:00

(Migrated from github.com)

Yes, should be a logical "&&".

I'll update PR and add some documentation. Thanks.

Yes, should be a logical "&&". I'll update PR and add some documentation. Thanks.

lurch (Migrated from github.com) reviewed

2024-03-20 03:59:05 -05:00

src/rp2_common/hardware_clocks/include/hardware/clocks.h

					
				@ -165,6 +165,15 @@ extern "C" {

				 */

Looks like lightsleep() is only a MicroPython function, not a Pico C-SDK function, so perhaps 'lightsleep()' here should be changed to "light sleep" ?

Looks like `lightsleep()` is only a MicroPython function, not a Pico C-SDK function, so perhaps `'lightsleep()'` here should be changed to `"light sleep"` ?

lurch (Migrated from github.com) reviewed

2024-03-20 04:01:47 -05:00

src/rp2_common/hardware_clocks/clocks.c

Should the void be left in the parentheses, as it was originally? (IIRC some compilers get grumpy without this)

Should the `void` be left in the parentheses, as it was originally? (IIRC some compilers get grumpy without this)

lurch (Migrated from github.com) reviewed

2024-03-20 04:06:28 -05:00

src/rp2_common/hardware_clocks/clocks.c

Hmmm, rather than doing this same logic-test 3 separate times, perhaps it makes sense to do:

bool init_sys_clock = ((sleep_en0 & CLOCKS_SLEEP_EN0_CLK_SYS_DEPENDANTS_BITS) == 0) &&
                      ((sleep_en1 & CLOCKS_SLEEP_EN1_CLK_SYS_DEPENDANTS_BITS) == 0)

just once? 🤔

Hmmm, rather than doing this same logic-test 3 separate times, _perhaps_ it makes sense to do: ``` bool init_sys_clock = ((sleep_en0 & CLOCKS_SLEEP_EN0_CLK_SYS_DEPENDANTS_BITS) == 0) && ((sleep_en1 & CLOCKS_SLEEP_EN1_CLK_SYS_DEPENDANTS_BITS) == 0) ``` just once? :thinking:

lurch (Migrated from github.com) reviewed

2024-03-21 08:45:46 -05:00

src/rp2_common/hardware_clocks/clocks.c

Can you replace tabs with spaces please, and fix up the indentation? 🙂

Can you replace tabs with spaces please, and fix up the indentation? :slightly_smiling_face:

mungewell commented

2024-03-23 13:07:23 -05:00

(Migrated from github.com)

I removed the tabs.

Do you want the whole series resubmitted (_v3?) as a single patch/pull request?

I removed the tabs. Do you want the whole series resubmitted (_v3?) as a single patch/pull request?

lurch commented

2024-03-23 13:12:34 -05:00

(Migrated from github.com)

That's not my decision to make, so leave it as-is for now unless asked otherwise 🙂

That's not my decision to make, so leave it as-is for now unless asked otherwise :slightly_smiling_face:

kilograham commented

2024-05-19 16:31:44 -05:00

(Migrated from github.com)

fixes #1668

lurch (Migrated from github.com) reviewed

2024-05-20 05:00:01 -05:00

src/rp2_common/hardware_clocks/clocks.c

This "tag" is used to auto-include this code snippet into page 191 of https://datasheets.raspberrypi.com/rp2040/rp2040-datasheet.pdf

So in order not to clutter that example, it might make sense to move your new if check to outside of the tag? i.e.

    if (init_sys_clock) {
        /// \tag::configure_clk_sys[]
        // CLK SYS = PLL SYS (usually) 125MHz / 1 = 125MHz
        clock_configure(clk_sys,
                        CLOCKS_CLK_SYS_CTRL_SRC_VALUE_CLKSRC_CLK_SYS_AUX,
                        CLOCKS_CLK_SYS_CTRL_AUXSRC_VALUE_CLKSRC_PLL_SYS,
                        SYS_CLK_KHZ * KHZ,
                        SYS_CLK_KHZ * KHZ);
        /// \end::configure_clk_sys[]
    }

This "tag" is used to auto-include this code snippet into page 191 of https://datasheets.raspberrypi.com/rp2040/rp2040-datasheet.pdf ![Screenshot from 2024-05-20 10-55-44](https://github.com/raspberrypi/pico-sdk/assets/476186/b1585d31-0a42-4431-864c-7e4263d406b9) So in order not to clutter that example, it might make sense to move your new `if` check to **outside** of the tag? i.e. ```c if (init_sys_clock) { /// \tag::configure_clk_sys[] // CLK SYS = PLL SYS (usually) 125MHz / 1 = 125MHz clock_configure(clk_sys, CLOCKS_CLK_SYS_CTRL_SRC_VALUE_CLKSRC_CLK_SYS_AUX, CLOCKS_CLK_SYS_CTRL_AUXSRC_VALUE_CLKSRC_PLL_SYS, SYS_CLK_KHZ * KHZ, SYS_CLK_KHZ * KHZ); /// \end::configure_clk_sys[] } ```

projectgus commented

2024-06-25 18:17:39 -05:00

(Migrated from github.com)

If this patch was extended to also check and selectively init PLL_USB then it would also meet the use case in #1746. Looks like a good approach to reuse the sleep bits this way.

kilograham commented

2026-04-28 09:59:17 -05:00

(Migrated from github.com)

cc @will-v-pi to see if handled by new lower power code

will-v-pi commented

2026-04-29 01:43:59 -05:00

(Migrated from github.com)

cc @will-v-pi to see if handled by new lower power code

The new low power code doesn't have this issue as it doesn't disable any clocks while sleeping, so doesn't need to reinitialise the clocks when it wakes up.

Currently, Micropython disables the unused clocks during sleep, and therefore needs to reinitialise them afterwards, causing these issues. If they switch to the new low power code, this issue will go away, but there may be a slight increase in power consumption during sleep as the unused clocks are just gated during sleep, rather than being disabled.

> cc @will-v-pi to see if handled by new lower power code The new low power code doesn't have this issue as it doesn't disable any clocks while sleeping, so doesn't need to reinitialise the clocks when it wakes up. Currently, Micropython disables the unused clocks during sleep, and therefore needs to reinitialise them afterwards, causing these issues. If they switch to the new low power code, this issue will go away, but there may be a slight increase in power consumption during sleep as the unused clocks are just gated during sleep, rather than being disabled.

This pull request has changes conflicting with the target branch.

src/rp2_common/hardware_clocks/clocks.c
src/rp2_common/hardware_clocks/include/hardware/clocks.h

View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Merge commit title

Merge pull request 'WIP: Bug 1668 v2 - Provide method to prevent init of SYS_CLK, when left on during 'lightsleep'' (#1672) from mungewell/bug_1668_v2 into develop

Merge commit body

Reviewed-on: https://git.richey.dev/gh-raspberrypi/pico-sdk/pulls/1672

Merge pull request 'WIP: Bug 1668 v2 - Provide method to prevent init of SYS_CLK, when left on during 'lightsleep'' (#1672) from mungewell/bug_1668_v2 into develop

Reviewed-on: https://git.richey.dev/gh-raspberrypi/pico-sdk/pulls/1672

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'mungewell/bug_1668_v2:mungewell/bug_1668_v2'

git switch 'mungewell/bug_1668_v2'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'develop'

git merge --no-ff 'mungewell/bug_1668_v2'

git switch 'mungewell/bug_1668_v2'

git rebase 'develop'

git switch 'develop'

git merge --ff-only 'mungewell/bug_1668_v2'

git switch 'mungewell/bug_1668_v2'

git rebase 'develop'

git switch 'develop'

git merge --no-ff 'mungewell/bug_1668_v2'

git switch 'develop'

git merge --squash 'mungewell/bug_1668_v2'