nRF: Add system clock stability policy #27398

nordic-krch · 2020-08-06T07:59:27Z

Pull request adds clock stability policy configuration to the system clock (see #22758) and adds support to Nordic platform. Additionally, spin waiting for stable or available clock has been reworked for Nordic to allow going to idle while waiting for clock readiness (it can take up to 250 ms). Added test dedicated for Nordic platforms which validates correctness of behavior for each configuration against various clock sources.

Fixes #22758.
Fixes #26280.

andyross

No complaints about the code. But is this really a good candidate for a global API? Right now it's just the one platform that needs to care about this. Maybe we should start this out as an internal thing with the nRF timer driver and promote the kconfig API to a global one only later?

I guess my criteria for this is: if we add a kconfig value to the core kernel, and it's never inspected by or modified by any of the code or tests dealing with the core kernel, it shouldn't be a core kernel feature. ... ?

nordic-krch · 2020-08-07T05:56:18Z

Seems to me that there is also sam0_rtc driver which seems to currently wait for the clock:

zephyr/drivers/timer/sam0_rtc_timer.c

Line 191 in e227f5b

/* Synchronize GCLK. */

Not sure who is maintaining it.

pabigot

One question about whether EVENT_LFCLKSTART could be handled by the ISR and cleared before the spinwait that's looking for it. E.g. if something else like a bootloader had done some clock configuration so it could be triggered faster than normal.

I like the two-phase startup for LFXO to catch the distinct stages.

pabigot · 2020-08-07T13:23:49Z

drivers/clock_control/nrf_power_clock.c

+		/* Synth source start is almost instant and LFCLKSTARTED may
+		 * happen before calling idle. That would lead to deadlock.
+		 */
+		if (!IS_ENABLED(CONFIG_CLOCK_CONTROL_NRF_K32SRC_SYNTH)) {


I see the goal here, but z_clock_driver_init() will invoke z_nrf_clock_control_lf_on() which on its first entry will use the onoff manager to trigger TASK_LFCLKSTART. All that is before interrupts are locked on entry to this function.

There is a typical 1 ms startup time for this on nRF52840, but it does seem like the ISR may have already processed LFCLKSTARTED before the lock is taken, so deadlock is still possible.

There is a typical 1 ms startup time for this on nRF52840, but it does seem like the ISR may have already processed LFCLKSTARTED before the lock is taken, so deadlock is still possible.

Interrupts are locked at this point of startup. I call irq_lock just to get key which is required by k_cpu_atomic_idle and irq_unlock for consistency.

i will see if i can do something to support call of z_nrf_clock_control_lf_on without that assumption.

Interrupts are locked at this point of startup.

Could you expand on this?

z_clock_driver_init() at PRE_KERNEL_2/CONFIG_SYSTEM_CLOCK_INIT_PRIORITY invokes
z_nrf_clock_control_lf_on() invokes
onoff_request() invokes
lfclk_start() triggers NRF_CLOCK_TASK_LFCLKSTART then
z_nrf_clock_control_lf_on() invokes lfclk_spinwait()

Nothing on that path appears to disables interrupts, so theoretically the EVENT_LFCLKSTARTED could have been cleared by clock_event_check_and_clean() in nrf_power_clock_isr() before the subsequent lfclk_spinwait() gets control and does disable interrupt, expecting to be able to see that event itself.

What am I missing?

Nothing on that path appears to disables interrupts

driver initialization is called from z_cstart which has interrupts disabled.

I have added support to spinwaiting from thread context and added test for that.

carlescufi · 2020-08-12T12:16:55Z

DNM until #27422 is merged

pabigot

Switching from comment to block because we've lost momentum and I don't want this concern to be missed.

#27398 (comment)

nordic-krch · 2020-09-01T07:16:17Z

Upmerged since #27422 got merged. Removed DNM.

Question answered.

pabigot · 2020-09-02T14:56:26Z

Should this be tagged for 2.4.0? @carlescufi

pabigot

I'd like to see the logical operators used for boolean expressions, since we're going that way for MISRA. That's the only request changes. Otherwise this looks fine by inspection.

I get failures on drivers.clock.nrf_lf_clock_start_xtal_* variants on nrf52840dk_nrf52840 running sanitycheck with --device-testing. For *=available the failure mode is that it appears that configuration doesn't output anything so the test times out, and it always fails under sanity check but passes manually. This may be a race condition specific to the test infrastructure.

I got one failure with *=no_wait:

tirzah[765]$ cat sanity-out/nrf52840dk_nrf52840/tests/drivers/clock_control/nrf_lf_clock_start/drivers.clock.nrf_lf_clock_start_xtal_no_wait/handler.log
CLOCK_CONTROL_NRF_K32SRC=XTAL
SYSTEM_CLOCK_NO_WAIT=y
SYSTEM_CLOCK_WAIT_FOR_AVAILABILITY=n
SYSTEM_CLOCK_WAIT_FOR_STABILITY=n
Running test suite test_nrf_lf_clock_start
===================================================================
START - test_clock_check
PASS - test_clock_check
===================================================================
O	MhFE: SPSEL in thread mode does not indicate PSP
ASSERTION FAIL [esf != ((void *)0)] @ WEST_TOPDIR/zephyr/arch/arm/core/aarch32/cortex_m/fault.c:955
ESF could not be retrieved successfully. Shall never occur.
E: ***** HARD FAULT *****
E:   Fault escalation (see below)
E: ***** BUS FAULT *****
E:   Imprecise data bus error
E: r0/a1:  0x00000004  r1/a2:  0x000003bb  r2/a3:  0x00000001
E: r3/a4:  0x00000000 r12/ip:  0x00000000 r14/lr:  0x00001d71
E:  xpsr:  0x61000005
E: Faulting instruction address (r15/pc): 0x00005880
E: >>> ZEPHYR FATAL ERROR 0: CPU exception on CPU 0
E: Fault during interrupt handling
E: Current thread: 0x20000240 (main)
E: Halting system

pabigot · 2020-09-02T15:00:53Z

drivers/clock_control/clock_control_nrf.c

@@ -427,17 +426,64 @@ static void lfclk_spinwait(nrf_clock_lfclk_t t)
 {
 	nrf_clock_domain_t d = NRF_CLOCK_DOMAIN_LFCLK;
 	nrf_clock_lfclk_t type;
+	static const bool lfxo_target =
+		    IS_ENABLED(CONFIG_CLOCK_CONTROL_NRF_K32SRC_XTAL) |


These should be || since the operands and result are essentially boolean.

Same in other locations (isr_mode next definition)

pabigot · 2020-09-02T15:06:22Z

tests/drivers/clock_control/nrf_lf_clock_start/src/main.c

+		zassert_false(on, "Clock should be off");
+	} else if (IS_ENABLED(CONFIG_SYSTEM_CLOCK_WAIT_FOR_AVAILABILITY)) {
+		bool is_running =
+			rtc_cnt | (on && (type == NRF_CLOCK_LFCLK_RC));


so for essential type this should be (rtc_cnt != 0) || (on ...

pabigot · 2020-09-02T15:39:54Z

tests/drivers/clock_control/nrf_lf_clock_start/src/main.c

+	on = nrf_clock_is_running(NRF_CLOCK, NRF_CLOCK_DOMAIN_LFCLK, &type);
+	k_busy_wait(100);
+	rtc_cnt = k_cycle_get_32();
+


Would you be open to this patch at this point in the code:

--- a/tests/drivers/clock_control/nrf_lf_clock_start/src/main.c +++ b/tests/drivers/clock_control/nrf_lf_clock_start/src/main.c @@ -98,6 +98,18 @@ void test_main(void) k_busy_wait(100); rtc_cnt = k_cycle_get_32(); + TC_PRINT("CLOCK_CONTROL_NRF_K32SRC=%s\n", + IS_ENABLED(CONFIG_CLOCK_CONTROL_NRF_K32SRC_RC) ? "RC" + : IS_ENABLED(CONFIG_CLOCK_CONTROL_NRF_K32SRC_SYNTH) ? "SYNTH" + : IS_ENABLED(CONFIG_CLOCK_CONTROL_NRF_K32SRC_XTAL) ? "XTAL" + : "???"); + TC_PRINT("SYSTEM_CLOCK_NO_WAIT=%c\n", + IS_ENABLED(CONFIG_SYSTEM_CLOCK_NO_WAIT) ? 'y' : 'n'); + TC_PRINT("SYSTEM_CLOCK_WAIT_FOR_AVAILABILITY=%c\n", + IS_ENABLED(CONFIG_SYSTEM_CLOCK_WAIT_FOR_AVAILABILITY) ? 'y' : 'n'); + TC_PRINT("SYSTEM_CLOCK_WAIT_FOR_STABILITY=%c\n", + IS_ENABLED(CONFIG_SYSTEM_CLOCK_WAIT_FOR_STABILITY) ? 'y' : 'n'); + ztest_test_suite(test_nrf_lf_clock_start, ztest_unit_test(test_clock_check), ztest_unit_test(test_wait_in_thread)

This produces the following in the handler.log which is helpful when trying to correlate the build tags with the configuration:

CLOCK_CONTROL_NRF_K32SRC=SYNTH SYSTEM_CLOCK_NO_WAIT=n SYSTEM_CLOCK_WAIT_FOR_AVAILABILITY=n SYSTEM_CLOCK_WAIT_FOR_STABILITY=y Running test suite test_nrf_lf_clock_start

sure, added.

carlescufi · 2020-09-04T14:20:28Z

@andyross could you please take a look so we can merge this in time for 2.4.0?

PerMac · 2020-09-09T08:50:39Z

Running on this PR solves the problem with test_kernel_systicks on nrf9160dk_nrf9160. However, it introduces an error for nrf5340dk_nrf5340_cpuapp: after flashing the board hangs, and the test from tests/portability/cmsis_rtos_v1 ends with a timeout.

nordic-krch · 2020-09-09T08:58:29Z

@PerMac thanks for running it on that PR, i will look into nrf53 issue then.

nordic-krch · 2020-09-09T13:09:52Z

nrf53 is fixed by #28121

carlescufi · 2020-09-09T13:41:21Z

No complaints about the code. But is this really a good candidate for a global API? Right now it's just the one platform that needs to care about this. Maybe we should start this out as an internal thing with the nRF timer driver and promote the kconfig API to a global one only later?

We have restricted it to the scope of the nRF timer for now, and after the 2.4 release we will consider expanding this to the rest of platforms.

carlescufi · 2020-09-09T13:41:53Z

@anangl please take a look

anangl · 2020-09-10T06:02:01Z

drivers/clock_control/clock_control_nrf.c

@@ -463,13 +510,20 @@ void z_nrf_clock_control_lf_on(enum nrf_lfclk_start_mode start_mode)
 		__ASSERT_NO_MSG(err >= 0);
 	}

+	/* In case of simulated board leave immediately. */
+	if (IS_ENABLED(CONFIG_BOARD_NRF52_BSIM)) {


Make this check more generic and consistent with what is used in the Bluetooth controller:

Suggested change

if (IS_ENABLED(CONFIG_BOARD_NRF52_BSIM)) {

if (IS_ENABLED(CONFIG_SOC_SERIES_BSIM_NRFXX)) {

anangl · 2020-09-10T08:25:18Z

drivers/clock_control/clock_control_nrf.c

-	while (!(nrfx_clock_is_running(d, (void *)&type) && (type == t))) {
-		/* empty */
+	while (!(nrfx_clock_is_running(d, (void *)&type)
+		 && ((type == t) || (type == CLOCK_CONTROL_NRF_K32SRC)))) {


This won't work for CONFIG_CLOCK_CONTROL_NRF_K32SRC_EXT_LOW_SWING and CONFIG_CLOCK_CONTROL_NRF_K32SRC_EXT_FULL_SWING, as type will be NRF_CLOCK_LFCLK_Xtal then.

I'd propose to correct it and simplify a bit the logic used here as follows.
First, to change the function signature to:

static void lfclk_spinwait(enum nrf_lfclk_start_mode start_mode)

then instead of lfxo_target use something like:

nrf_clock_lfclk_t target_type = /* For sources XTAL, EXT_LOW_SWING, and EXT_FULL_SWING, * NRF_CLOCK_LFCLK_Xtal is returned as the type of running clock. */ (IS_ENABLED(CONFIG_CLOCK_CONTROL_NRF_K32SRC_XTAL) || IS_ENABLED(CONFIG_CLOCK_CONTROL_NRF_K32SRC_EXT_LOW_SWING) || IS_ENABLED(CONFIG_CLOCK_CONTROL_NRF_K32SRC_EXT_FULL_SWING)) ? NRF_CLOCK_LFCLK_Xtal : CLOCK_CONTROL_NRF_K32SRC;

and then use here:

while (!(nrfx_clock_is_running(d, (void *)&type) && ((type == target_type) || (mode == NRF_LFCLK_START_MODE_SPINWAIT_AVAILABLE))) {

and below:

if (target_type == NRF_CLOCK_LFCLK_Xtal && (nrf_clock_lf_src_get(NRF_CLOCK) == NRF_CLOCK_LFCLK_RC) && nrf_clock_event_check(NRF_CLOCK, ...

In the switch in z_nrf_clock_control_lf_on() we'll have then:

case NRF_LFCLK_START_MODE_SPINWAIT_STABLE: case NRF_LFCLK_START_MODE_SPINWAIT_AVAILABLE: lfclk_spinwait(mode);

Nice suggestion. Done.

anangl · 2020-09-10T08:40:53Z

include/drivers/clock_control/nrf_clock_control.h

 enum nrf_lfclk_start_mode {
 	NRF_LFCLK_START_MODE_NOWAIT,
-	NRF_LFCLK_START_MODE_SPINWAIT_RUNNING,
+	NRF_LFCLK_START_MODE_SPINWAIT_AVAILABLE,
 	NRF_LFCLK_START_MODE_SPINWAIT_STABLE,
 };


Shouldn't we use other prefixes here? I know, CLOCK_CONTROL_NRF_ is lengthy, but the current one is not consistent with the other stuff defined in this file.
(It's probably not something to be corrected in this PR, just spotted it here).

renamed to CLOCK_CONTROL_NRF_LF_START_[STABLE, AVAILABLE,NOWAIT]

Added configuration for approach to starting system clock source. There are 3 options: no wait, wait untill available, wait until stable. Added support for those modes in clock control driver which handles low frequency source clock. Signed-off-by: Krzysztof Chruscinski <[email protected]>

anangl

LGTM. One unnecessary comment could be removed.

anangl · 2020-09-10T15:25:09Z

drivers/clock_control/clock_control_nrf.c

+	case CLOCK_CONTROL_NRF_LF_START_AVAILABLE:
+		/* fall through. */
+	case CLOCK_CONTROL_NRF_LF_START_STABLE:


The comment here is not needed. There is no code between those two cases, so there will be no warning here. It's not a "true fall through". ;-)

Enabled going to idle when waiting for low frequency clock. Added 2 stages of starting LF clock when XTAL is used. First stage is starting RC and then when it is ready XTAL is started. It is done to get event/interrupt when RC is ready which means that LF clock is available (but not stable). Signed-off-by: Krzysztof Chruscinski <[email protected]>

Added test which verifies various modes of waiting for system clock (no wait, available, stable) for different clock configurations (xtal, rc, synth). Signed-off-by: Krzysztof Chruscinski <[email protected]>

nordic-krch · 2020-09-10T17:43:25Z

@carlescufi since this is now nordic only maybe i could be merged.

- nrf_security: update mbedtls heap to const struct device Zephyr upmerge introduces device bindings const struct as part of: zephyrproject-rtos/zephyr#24873 This const'ify mbedtls_heap.c init function. - mpls: Added `select MULTITHREADING_LOCK` to Kconfig With the changes related to: nrfconnect/sdk-nrf#2861 zephyrproject-rtos/zephyr#27398 The mpsl clock control is no longer needed, but mpsl still requires MULTITHREADING_LOCK, and is therefore moved to its right location. Signed-off-by: Torsten Rasmussen <[email protected]>

- nrf_security: update mbedtls heap to const struct device Zephyr upmerge introduces device bindings const struct as part of: zephyrproject-rtos/zephyr#24873 This const'ify mbedtls_heap.c init function. - mpls: Added `select MULTITHREADING_LOCK` to Kconfig With the changes related to: nrfconnect/sdk-nrf#2861 zephyrproject-rtos/zephyr#27398 The mpsl clock control is no longer needed, but mpsl still requires MULTITHREADING_LOCK, and is therefore moved to its right location. - softdevice_controller: Mark 2M and Coded PHY as supported These helper configs were introduced in upstream zephyr. Signed-off-by: Torsten Rasmussen <[email protected]> Signed-off-by: Rubin Gerritsen <[email protected]>

nordic-krch added area: Timer Timer platform: nRF Nordic nRFx area: Clock Control labels Aug 6, 2020

nordic-krch requested review from andyross, pabigot and anangl August 6, 2020 07:59

nordic-krch requested review from carlescufi, galak, MaureenHelm and nashif as code owners August 6, 2020 07:59

github-actions bot added area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test labels Aug 6, 2020

andyross reviewed Aug 6, 2020

View reviewed changes

nordic-krch force-pushed the clock_stability branch from 786dba1 to 300675d Compare August 7, 2020 10:48

pabigot reviewed Aug 7, 2020

View reviewed changes

nordic-krch force-pushed the clock_stability branch 2 times, most recently from 8f59953 to 9a817bd Compare August 10, 2020 06:55

carlescufi added the DNM This PR should not be merged (Do Not Merge) label Aug 12, 2020

pabigot self-requested a review August 24, 2020 14:13

pabigot previously requested changes Aug 24, 2020

View reviewed changes

nordic-krch force-pushed the clock_stability branch 2 times, most recently from 694b393 to 38458e6 Compare September 1, 2020 07:15

nordic-krch removed the DNM This PR should not be merged (Do Not Merge) label Sep 1, 2020

pabigot suggested changes Sep 2, 2020

View reviewed changes

nordic-krch force-pushed the clock_stability branch from 38458e6 to 7937b16 Compare September 3, 2020 06:02

This was referenced Sep 8, 2020

[RFC] drivers: mpsl: clock_control: Provide nrfx_clock shim nrfconnect/sdk-nrf#2861

Closed

test_kernel_systicks from tests/portability/cmsis_rtos_v1 fails on nrf platforms #26280

Closed

nordic-krch force-pushed the clock_stability branch from a44c12c to 7ce2fb7 Compare September 9, 2020 08:38

carlescufi added the bug The issue is a bug, or the PR is fixing a bug label Sep 9, 2020

nordic-krch mentioned this pull request Sep 9, 2020

drivers: clock_control: nrf: Fix nrf53 LFXO pin configuration #28121

Merged

carlescufi changed the title ~~Add system clock stability policy~~ nRF: Add system clock stability policy Sep 9, 2020

anangl requested changes Sep 10, 2020

View reviewed changes

anangl reviewed Sep 10, 2020

View reviewed changes

nordic-krch force-pushed the clock_stability branch 2 times, most recently from 4ed3453 to 1df5f57 Compare September 10, 2020 14:52

anangl approved these changes Sep 10, 2020

View reviewed changes

nordic-krch added 2 commits September 10, 2020 18:00

tests: drivers: clock_control: Add LF clock start test

356694d

Added test which verifies various modes of waiting for system clock (no wait, available, stable) for different clock configurations (xtal, rc, synth). Signed-off-by: Krzysztof Chruscinski <[email protected]>

nordic-krch force-pushed the clock_stability branch from 1df5f57 to 356694d Compare September 10, 2020 16:00

carlescufi merged commit 96b6da9 into zephyrproject-rtos:master Sep 10, 2020

	if (IS_ENABLED(CONFIG_BOARD_NRF52_BSIM)) {
	if (IS_ENABLED(CONFIG_SOC_SERIES_BSIM_NRFXX)) {

nRF: Add system clock stability policy #27398

nRF: Add system clock stability policy #27398

Uh oh!

Conversation

nordic-krch commented Aug 6, 2020 • edited by carlescufi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andyross left a comment

Choose a reason for hiding this comment

Uh oh!

nordic-krch commented Aug 7, 2020

Uh oh!

pabigot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nordic-krch Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi commented Aug 12, 2020

Uh oh!

pabigot left a comment

Choose a reason for hiding this comment

Uh oh!

nordic-krch commented Sep 1, 2020

Uh oh!

pabigot commented Sep 2, 2020

Uh oh!

pabigot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi commented Sep 4, 2020

Uh oh!

PerMac commented Sep 9, 2020

Uh oh!

nordic-krch commented Sep 9, 2020

Uh oh!

nordic-krch commented Sep 9, 2020

Uh oh!

carlescufi commented Sep 9, 2020

Uh oh!

carlescufi commented Sep 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anangl Sep 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anangl left a comment

Choose a reason for hiding this comment

Uh oh!

nordic-krch commented Aug 6, 2020 •

edited by carlescufi

Loading

nordic-krch Aug 7, 2020 •

edited

Loading

anangl Sep 10, 2020 •

edited

Loading

anangl Sep 10, 2020 •

edited

Loading