drivers: clock_control: nrf: Use onoff service #24334

nordic-krch · 2020-04-14T11:16:19Z

Ported nRF clock control driver to use onoff service.
Clock_control API no longer tracks number of users but only enables or disables clock. It should not be used directly for clocks which have multiple user.

Clock control driver implements onoff service which can be used to track clock users. Clock control driver implements error detection mechanism which detects cases when direct API and onoff API is used for the same clock subsystem.

Nordic specific API (nrf_clock_control.h) provides function for getting onoff manager associated with given clock subsystem: z_nrf_clock_control_get_onoff().

Since low frequency clock is typically used in enable once mode, nordic specific API provides a function for enabling LF clock without a need to maintain onoff client (z_nrf_clock_control_lf_on()). This is currently used by components which uses LF clock: system timer, bluetooth or counter driver.

PR contains port to use onoff service whenever HF clock is used.

PR is based on #23898 thus DNM.

Fixes #20712

zephyrbot · 2020-04-14T11:25:49Z

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

nordic-krch · 2020-07-15T11:34:25Z

except for the significant degradation in performance.

@cvinayak thanks for checking this. I will take a look what is the onoff overhead and what can be done about it.

nordic-krch · 2020-07-16T12:35:43Z

@cvinayak can you take one more look? I've added void z_nrf_clock_bt_ctrl_hf_request(void) and void z_nrf_clock_bt_ctrl_hf_release(void) to be used by the controller.

@anangl can you take another look at the driver implementation as i had to add some hacks&flags to support out of band hf clock requesting. I've also added a test suite where i tried to mix ble and onoff API and stress test it.

Overall, that didn't make clock control driver prettier so I hope that this backdoor was opened for only one guest.

anangl · 2020-07-17T06:09:31Z

tests/drivers/clock_control/clock_control_api/src/test_clock_control.c

 	LOG_INF("setup done");
 }

 static void tear_down_instance(const char *dev_name,
 				clock_control_subsys_t subsys)
 {
-	struct device *dev = device_get_binding(dev_name);
+#if DT_NODE_HAS_STATUS(DT_INST(0, nordic_nrf_clock), okay)


Suggested change

#if DT_NODE_HAS_STATUS(DT_INST(0, nordic_nrf_clock), okay)

#if DT_HAS_COMPAT_STATUS_OKAY(nordic_nrf_clock)

anangl · 2020-07-17T06:32:49Z

tests/drivers/clock_control/clock_control_api/src/test_clock_control.c

 	err = clock_control_off(dev, subsys);
-	zassert_equal(0, err, "%s: Unexpected err (%d)", dev_name, err);
+	zassert_equal(-EBUSY, err, "%s: Unexpected err (%d)", dev_name, err);


This error code is not specified by the clock_control API, so this check should be performed only for nRF clocks. Same applies to double start/stop test cases.

anangl · 2020-07-17T08:26:44Z

drivers/clock_control/nrf_power_clock.c

+/* Used only by HF clock */
+#define HF_BT_ACTIVE		BIT(8)
+#define HF_GENERIC_ACTIVE	BIT(9)
+#define HF_USER_MASK		(HF_BT_ACTIVE | HF_GENERIC_ACTIVE)


Since this is only for HFCLK, why not use a dedicated variable for it? Something like:

static atomic_t hfclk_users;

You would then avoid the need of this weird masking of flags with HF_USER_MASK in set_off_state():
https://github.com/zephyrproject-rtos/zephyr/blob/a7fbf5e80f3f702024c261dd63899f368f39028d/drivers/clock_control/nrf_power_clock.c#L158
and in set_starting_state():
https://github.com/zephyrproject-rtos/zephyr/blob/a7fbf5e80f3f702024c261dd63899f368f39028d/drivers/clock_control/nrf_power_clock.c#L173-L175
Also accesses to such variable should be faster than to flags for which you need to get the pointer via get_hf_flags().

anangl · 2020-07-17T08:50:46Z

include/drivers/clock_control/nrf_clock_control.h

+ * ensure that every z_nrf_clock_bt_ctrl_hf_request matches
+ * z_nrf_clock_bt_ctrl_hf_release call.
+ */
+void z_nrf_clock_bt_ctrl_hf_request(void);


You should rather use *_bt_ctlr_*, for consistency with other related stuff.

anangl · 2020-07-17T09:05:00Z

drivers/clock_control/nrf_power_clock.c

+
+	*flags |= HF_GENERIC_ACTIVE;
+	if (*flags & HF_BT_ACTIVE) {
+		nrf_clock_is_running(NRF_CLOCK, NRF_CLOCK_DOMAIN_HFCLK, &type);


The value returned by nrf_clock_is_running() is ignored here, so (void) could help make it clear (and perhaps also avoid warnings from some linters).

anangl · 2020-07-20T08:40:13Z

drivers/clock_control/nrf_power_clock.c

+/* Used only by HF clock */
+#define HF_USER_BT		BIT(0)
+#define HF_USER_GENERIC		BIT(1)
+#define HF_USER_MASK		(HF_USER_BT | HF_USER_GENERIC)


No longer needed.

nordic-krch · 2020-07-20T08:56:56Z

@cvinayak can you check bluetooth part?

cvinayak · 2020-07-20T11:50:33Z

PR:

Master 98bcc51:

The performance is now comparable.

cvinayak

Approved for BLE controller changes by analyzing performance changes.

boards/arm/qemu_cortex_m0/nrf_timer_timer.c

Updated nrf clock control driver to use onoff service for managing multiple users. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

Added shell command to clock driver to get clock status. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

Added suite to test clock control with onoff. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

Nordic platforms exposes API for controlling HF clock along onoff API. This API is dedicated to be used by the bluetooth controller and must not be used elsewhere. Test suite is validating stability of HF clock control using onoff API and specialized API. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

Adapted driver to use onoff service for clock control. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

Adapted driver to clock_control changes (usage of onoff manager). Since timer is permanenty requesting the clock, it is using API dedicated for that: z_nrf_clock_control_lf_on(). Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

Adapted nrfx_rtc counter to request clock through onoff API. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

Adapt link layer to use onoff service when controlling clock. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

Change to use onoff service when controlling HFXO. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

Removed HFXO request. It is a qemu target thus HF clock source should not matter. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

Adapted to use onoff service for clock control Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

nrf_hw_models require extension to support nrf_clock_is_running call. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

addressed

nordic-krch added platform: nRF Nordic nRFx DNM This PR should not be merged (Do Not Merge) area: Clock Control labels Apr 14, 2020

nordic-krch requested review from andrewboie, andyross, carlescufi, cvinayak, finikorg, galak, ioannisg, jfischer-no, MaureenHelm, mbolivar-nordic, nashif and thoh-ot as code owners April 14, 2020 11:16

zephyrbot added area: Sensors Sensors area: Counter area: Timer Timer area: Boards area: Bluetooth area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test area: Documentation labels Apr 14, 2020

nordic-krch force-pushed the clock_control_calib_onoff branch 2 times, most recently from fce8975 to c234836 Compare April 15, 2020 09:45

pabigot self-requested a review April 15, 2020 12:09

carlescufi removed request for nashif, andyross and finikorg April 15, 2020 12:10

nordic-krch force-pushed the clock_control_calib_onoff branch 2 times, most recently from 3b1552e to bcc244b Compare July 16, 2020 12:31

nordic-krch force-pushed the clock_control_calib_onoff branch from bcc244b to 8e3ce05 Compare July 16, 2020 12:40

anangl reviewed Jul 17, 2020

View reviewed changes

nordic-krch force-pushed the clock_control_calib_onoff branch from 8e3ce05 to f263f18 Compare July 20, 2020 07:50

anangl approved these changes Jul 20, 2020

View reviewed changes

cvinayak approved these changes Jul 20, 2020

View reviewed changes

ioannisg previously requested changes Jul 20, 2020

View reviewed changes

boards/arm/qemu_cortex_m0/nrf_timer_timer.c Show resolved Hide resolved

nordic-krch added 12 commits July 20, 2020 14:24

drivers: clock_control: nrf: Add onoff service support

3c33af5

Updated nrf clock control driver to use onoff service for managing multiple users. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

drivers: clock_control: nrf: Add clock status shell command

d888310

Added shell command to clock driver to get clock status. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

tests: drivers: clock_control: Add suite for onoff with clock

f99de97

Added suite to test clock control with onoff. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

drivers: sensor: nrf5: Adapt temp driver to use onoff for clock

887c644

Adapted driver to use onoff service for clock control. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

drivers: counter: nrfx_rtc: Adapt to clock control changes

39fee24

Adapted nrfx_rtc counter to request clock through onoff API. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

bluetooth: controller: Adapt to onoff clock control

cae03f5

Adapt link layer to use onoff service when controlling clock. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

drivers: usb: device: nrf: Adapt to control clock using onoff

0238069

Change to use onoff service when controlling HFXO. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

boards: arm: qemu_cortex_m0: Removed HFXO request from the timer

4ba487c

Removed HFXO request. It is a qemu target thus HF clock source should not matter. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

drivers: led_strip: ws2812_gpio: Adapt to clock using onoff

91a543b

Adapted to use onoff service for clock control Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

manifest: Refer to updated nrf_hw_models

3d5aaea

nrf_hw_models require extension to support nrf_clock_is_running call. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

nordic-krch force-pushed the clock_control_calib_onoff branch from f263f18 to 3d5aaea Compare July 20, 2020 12:24

carlescufi merged commit 83ee9a8 into zephyrproject-rtos:master Jul 20, 2020

nordic-krch mentioned this pull request Aug 7, 2020

RFC: API change: clock_control: Change parameters of clock_control_async_on #27423

Closed

anangl mentioned this pull request Nov 22, 2022

drivers: clock_control_nrf: Fix releasing/stopping of HFCLK #52454

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: clock_control: nrf: Use onoff service #24334

drivers: clock_control: nrf: Use onoff service #24334

nordic-krch commented Apr 14, 2020 •

edited by carlescufi

Loading

zephyrbot commented Apr 14, 2020 •

edited

Loading

nordic-krch commented Jul 15, 2020

nordic-krch commented Jul 16, 2020

anangl Jul 17, 2020

anangl Jul 17, 2020

anangl Jul 17, 2020

anangl Jul 17, 2020

anangl Jul 17, 2020

anangl Jul 20, 2020

nordic-krch Jul 20, 2020

nordic-krch commented Jul 20, 2020

cvinayak commented Jul 20, 2020

cvinayak left a comment

	#if DT_NODE_HAS_STATUS(DT_INST(0, nordic_nrf_clock), okay)
	#if DT_HAS_COMPAT_STATUS_OKAY(nordic_nrf_clock)

drivers: clock_control: nrf: Use onoff service #24334

drivers: clock_control: nrf: Use onoff service #24334

Conversation

nordic-krch commented Apr 14, 2020 • edited by carlescufi Loading

zephyrbot commented Apr 14, 2020 • edited Loading

nordic-krch commented Jul 15, 2020

nordic-krch commented Jul 16, 2020

anangl Jul 17, 2020

Choose a reason for hiding this comment

anangl Jul 17, 2020

Choose a reason for hiding this comment

anangl Jul 17, 2020

Choose a reason for hiding this comment

anangl Jul 17, 2020

Choose a reason for hiding this comment

anangl Jul 17, 2020

Choose a reason for hiding this comment

anangl Jul 20, 2020

Choose a reason for hiding this comment

nordic-krch Jul 20, 2020

Choose a reason for hiding this comment

nordic-krch commented Jul 20, 2020

cvinayak commented Jul 20, 2020

cvinayak left a comment

Choose a reason for hiding this comment

nordic-krch commented Apr 14, 2020 •

edited by carlescufi

Loading

zephyrbot commented Apr 14, 2020 •

edited

Loading