mpsl: Add integration layer for MPSL external clock driver #19498

ppryga-nordic · 2024-12-13T09:27:35Z

The MPLS is enhanced with possibility to use it external clock driver.
That feature requires implementation of integration layer between nrf-clock-control for nRF52, nRF53 SoC series and nrf2-clock-control for nRF54H SoC series (the nRF54L SoC series is not available yet).

The integration with nrf-clock-control is experimental.
The integration with nrf2-clock-control is supported and enabled by default for nRF54H SoC series.

nrf2-clock-control required some changes to MSPL and SDC HCI driver initialization. The nrf2-clock-control depends on nRFS
so MPLS and SDC HCI driver that depend on MPLS need to be initialized in POST_KERNEL system initialization level and with priority lower than nRFS initialization priority.

Besides that there were a change in radio-notification-cb host extension that was dependent on RTC0 and RTC1 start order. With changed initialization level of MPSL the RTC1 (system timer) is started before the RTC0 used by MPSL.

NordicBuilder · 2024-12-13T09:29:05Z

The following west manifest projects have changed revision in this Pull Request:

Name	Old Revision	New Revision	Diff

✅ All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

NordicBuilder · 2024-12-13T09:34:00Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 37

Inputs:

Sources:

sdk-nrf: PR head: 233c61a94e9a9e67bcdd45436bd8b27253737884

more details

sdk-nrf:

PR head: 233c61a94e9a9e67bcdd45436bd8b27253737884
merge base: 83d7c5cc16e17ebfc07ff6e1708e525c5e3930d5
target head (main): a1a9f4429536ade819dff062b0a03cbe2c8b6ca8
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (11)

drivers
│  ├── mpsl
│  │  ├── clock_control
│  │  │  │ Kconfig
subsys
│  ├── bluetooth
│  │  ├── controller
│  │  │  ├── Kconfig
│  │  │  │ hci_driver.c
│  ├── mpsl
│  │  ├── CMakeLists.txt
│  │  ├── Kconfig
│  │  ├── clock_ctrl
│  │  │  ├── CMakeLists.txt
│  │  │  ├── Kconfig
│  │  │  ├── mpsl_clock_ctrl.c
│  │  │  │ mpsl_clock_ctrl.h
│  │  ├── init
│  │  │  ├── Kconfig
│  │  │  │ mpsl_init.c

Outputs:

Toolchain

Version: 342151af73
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:342151af73_bbe5b33786

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister - Skipped: Skipping Build & Test as it succeeded in a previous run: 36
✅ Integration tests
- ✅ desktop52_verification - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test_ble_nrf_config - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-ble_samples - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-chip
- ✅ test-fw-nrfconnect-rs - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-fem - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-thread - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-zigbee - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-sdk-find-my - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-sdk-sidewalk - Skipped: Job was skipped as it succeeded in a previous run

Disabled integration tests

- doc-internal
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-boot
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_lwm2m
- test-fw-nrfconnect-nrf-iot_mosh
- test-fw-nrfconnect-nrf-iot_positioning
- test-fw-nrfconnect-nrf-iot_samples
- test-fw-nrfconnect-nrf-iot_serial_lte_modem
- test-fw-nrfconnect-nrf-iot_thingy91
- test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- test-fw-nrfconnect-nrf_crypto
- test-fw-nrfconnect-proprietary_esb
- test-fw-nrfconnect-ps
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-tfm
- test-low-level
- test-sdk-audio
- test-sdk-dfu
- test-sdk-mcuboot
- test-sdk-pmic-samples
- test-sdk-wifi
- test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

NordicBuilder · 2024-12-16T10:27:52Z

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publish GitHub Action.

subsys/bluetooth/host_extensions/radio_notification_conn_cb.c

drivers/mpsl/clock_control/Kconfig

rugeGerritsen · 2024-12-16T12:32:25Z

include/mpsl/mpsl_clock_ctrl.h

The APIs mpsl_clock_ctrl_init() and mpsl_clock_ctrl_uninit() are not supposed to be called from application layer, only from MPSL-internal components. We should therefore move this header file from include/mpsl to subsys/mpsl

Then mpsl_pm.h should also be moved to mpsl/pm, it is also used internally by the mpsl subsystem.

File moved.

subsys/bluetooth/controller/Kconfig

subsys/mpsl/clock_ctrl/mpsl_clock_ctrl.c

subsys/mpsl/init/Kconfig

rugeGerritsen · 2024-12-16T13:18:16Z

subsys/mpsl/init/Kconfig

I would consider naming this MPSL_INIT_PRIORITY. Other drivers tend to use the convention <drvier>_INIT_PRIO_LEVEL or <driver>_INIT_PRIORITY

Changed to: MPSL_INIT_LEVEL

subsys/mpsl/init/mpsl_init.c

rugeGerritsen · 2024-12-17T07:25:14Z

subsys/bluetooth/host_extensions/radio_notification_conn_cb.c

This assertion is no longer needed

bjarki-andreasen

Looks good, definately like the introduction of MPSL_USE_EXTERNAL_CLOCK_CONTROL

nordicjm · 2025-02-03T10:48:40Z

subsys/mpsl/CMakeLists.txt

Suggested change

if (CONFIG_MPSL_USE_EXTERNAL_CLOCK_CONTROL)

if(CONFIG_MPSL_USE_EXTERNAL_CLOCK_CONTROL)

@nordicjm please see:

sdk-nrf/subsys/mpsl/CMakeLists.txt

Line 17 in 7815f14

if (CONFIG_MPSL_USE_ZEPHYR_PM)

. The code here is aligned to what was in the file already.

the file is wrong and needs fixing (outside the scope of this PR) but new code should be added correctly

subsys/mpsl/clock_ctrl/mpsl_clock_ctrl.h

subsys/mpsl/clock_ctrl/mpsl_clock_ctrl.c

rugeGerritsen · 2025-02-05T12:56:50Z

subsys/mpsl/clock_ctrl/mpsl_clock_ctrl.c

Is it intentional that we here allow a mismatch between the number of calls to m_hfclk_release() and m_hfclk_request()?

The only thing here is that there are allowed more releases than requests. That is not a problem from functional stand point of view I think. Do you see any risk?

I was wondering why we have added it here in the first place. It shouldn't be needed. It could indicate we have a bug somewhere

That is added for following purposes:

We do not release the clock if there is more than one request.

To avoid calling z_nrf_clock_bt_ctlr_hf_release more times than needed.

Gives some insight how many times did we request clock e.g. if there is more requests in runtime than releases you can check if immediately why HFXO is left requested.

subsys/mpsl/clock_ctrl/mpsl_clock_ctrl.c

rugeGerritsen · 2025-02-05T13:02:53Z

subsys/mpsl/clock_ctrl/mpsl_clock_ctrl.c

Why do we sleep when the operation didn't complete instead of retrying immediately?
Is there a reason for choosing 1 ms?

Discussed offline. Since the mpsl_init() happens in POST_KERNEL it is possible to use a callback notification from clock control. The decision was to switch from wait in a loop to use callback and semaphore.

subsys/mpsl/clock_ctrl/mpsl_clock_ctrl.c

rugeGerritsen · 2025-02-05T13:07:42Z

subsys/mpsl/clock_ctrl/mpsl_clock_ctrl.c

It would be nice to add a comment here describing that this is not always true.
Maybe something like

Suggested change

/* As of now assume the HFCLK is runnig after the request was put */

/* As of now assume the HFCLK is running after the request was put.

* This puts the responsibility to the user to check if the time since last

* request is larger than the HFXO rampup time.

*/

Another approach would be to disallow calling this API (__ASSERT_NO_MSG(false) ) to make it clear that the result of this function call cannot be trusted. The user would anyways have check the time since the last request.

rugeGerritsen · 2025-02-05T13:10:01Z

subsys/mpsl/clock_ctrl/mpsl_clock_ctrl.c

Calibration "is enabled" is something else than "is in progress", So I would assume the correct thing to do is to change this to

Suggested change

return z_nrf_clock_calibration_is_in_progress();

return true;

No, there is a difference in naming convention between nrf clock control and former mpsl clock control. What is enabled for mpsl clock control refers to: https://github.com/nrfconnect/sdk-zephyr/blob/693769a5c7354efce42af62b6c7f68f77c6678b7/drivers/clock_control/nrf_clock_calibration.c#L23-L24.
The nrf clock control uses this global to store state: https://github.com/nrfconnect/sdk-zephyr/blob/693769a5c7354efce42af62b6c7f68f77c6678b7/drivers/clock_control/nrf_clock_calibration.c#L34 and
the function used returns its value: https://github.com/nrfconnect/sdk-zephyr/blob/693769a5c7354efce42af62b6c7f68f77c6678b7/drivers/clock_control/nrf_clock_calibration.c#L298-L301

I'm a bit confused here. cal_process_in_progress in nrf_clock_calibration.c seems to be cleared in start_cycle. That is, is looks like the function only returns true when calibration is actually running.

I would expect m_lfclk_calibration_is_enabled to return true statically when calibration is enabled

Discussed offline, the correct approach is to return true; if CONFIG_CLOCK_CONTROL_NRF_DRIVER_CALIBRATION is enabled.

rugeGerritsen · 2025-02-05T13:11:54Z

subsys/mpsl/clock_ctrl/mpsl_clock_ctrl.c

Maybe we can clarify a bit here and for m_lfclk_calibration_is_enabled()

Suggested change

/* This function should not be called from MPSL if CONFIG_CLOCK_CONTROL_NRF2 is set */

/* This function is not supported when CONFIG_CLOCK_CONTROL_NRF2 is set.

* Currently MPSL will never use this API for this configuration.

*/

Changed to

/* This function is not supported when CONFIG_CLOCK_CONTROL_NRF2 is set. * As of now MPSL does not use this API in this configuration. */

ppryga-nordic · 2025-02-07T15:27:43Z

@rugeGerritsen @nordicjm @knutel-nordic your comments were addressed. It would be nice if you re-review the PR.

rugeGerritsen · 2025-02-10T08:44:16Z

subsys/mpsl/clock_ctrl/mpsl_clock_ctrl.c

Suggested change

* All these mean failure for MPLS.

* All these mean failure for MPSL.

rugeGerritsen · 2025-02-10T08:44:58Z

subsys/bluetooth/controller/hci_driver.c

Suggested change

"MPSL must be initialized before Soft Device Controller");

"MPSL must be initialized before SoftDevice Controller");

rugeGerritsen · 2025-02-10T08:46:40Z

subsys/mpsl/clock_ctrl/mpsl_clock_ctrl.c

Should we have a warning here?

Suggested change

if (atomic_get(&m_hfclk_refcnt) < (atomic_val_t)1) {

return;

}

if (atomic_get(&m_hfclk_refcnt) < (atomic_val_t)1) {

LOG_WRN("Mismatch between HFCLK request/release");

return;

}

Similar for the other implementation of m_hfclk_release()

Good idea, thank you!

Added to both implementation of the function for nrf/nrf2 clock control

ppryga-nordic · 2025-02-10T17:55:42Z

This PR fails on RS tests due to too low init priority of 802.15.4 in Zephyr. PR fixing: zephyrproject-rtos/zephyr#85527

ppryga-nordic · 2025-02-11T11:09:52Z

Downstream PR: nrfconnect/sdk-zephyr#2481

ppryga-nordic · 2025-02-12T08:55:40Z

Manifest fixing the 802.15.4 init priority is merged: #20203. The PR was rebased.

In the past MPSL used its own implementation of clock control. The approach changes due to lack of acces to clocks from Radio core in nRF54h20 SoC. Thaking this opportunity we provide experimental support for external clock control to nRF52 and nRF53 SoC Series. The new module is an integration layer between new MPSL API that allows for registration of external clock driver and nRF clock control driver. The implementation in this commit provides integration with MPSL for nrf clock control for nRF52 and nRF53 SoC series. Note: The support for nRF52 and nRF53 SoC series is experimental and is not enabled by default. Use of nrf clock control with MPSL allows to initialize the library in POST_KERNEL stage. Thanks to that we can use kernel synchronization primitives and non blocking waits. The change in MPSL init level and priority affects BT_LL_SOFTDEVIDE- _HCI_SYS_INIT_PRIORITY. The HCI driver for SDC depends on MPSL so it must be initialized after the MPSL. Signed-off-by: Piotr Pryga <[email protected]>

Add integration layer for MPSL external clock driver and nrf2 clock control for nRF5420. This is mandatory for the nRF54H20. Note: The nrf2 clock control requires the MPSL initialization to be done later. The nrf2 clock control depends on nRFS that is initialized at POST_KENREL init level. Its init priority is CONFIG_NRFS_BACKEND_IPC_SERVICE_INIT_PRIO that is lower than former MPSL init level. To make sure the mpsl lfclk request and response is handled corrently we must make the MPSL is initialized after it. Signed-off-by: Piotr Pryga <[email protected]>

The nrf clock control driver doesn't enable HFXO when LFSYNTH is selected as a source of LFCLK. That causes the accuracy of LFCLK to be not within the expected by BT Core Specification range up to 500 ppm. To fix the problem the mpsl clock control integration layer has to request the hfxo in case the lfclk is requested with LFSYNTH as a source. Use of z_nrf_clock_bt_ctlr_hf_release makes the call to be faster. That unfortunately requires reference counting do avoid release of HFXO later in runtime by MPSL. For that reason the m_hfclk_request and m_hfclk_release are used. Signed-off-by: Piotr Pryga <[email protected]>

…issue Add temporary workaround that allows the LFCLK to timeout on nRF54H20 (if CLOCK_CONTROL_NRF2 is enabled). The potential timeout is not an issue for now because the integration layer request the lowest accuracy of LFCLK and such or better LFCLK should be runing from boot of the radio core. Signed-off-by: Piotr Pryga <[email protected]>

ppryga-nordic · 2025-02-12T17:11:34Z

@nordicjm or @tejlmand could one of you approve?

ppryga-nordic requested review from a team as code owners December 13, 2024 09:27

github-actions bot added manifest changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Dec 13, 2024

NordicBuilder added manifest-zephyr manifest-dragoon DNM labels Dec 13, 2024

ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch 3 times, most recently from 9ae3f8c to a5d7313 Compare December 16, 2024 10:09

ppryga-nordic changed the title ~~[WIP] Mpsl nrf clock control integration~~ mpsl: Add integration layer for MPLS external clock driver Dec 16, 2024

ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch from a5d7313 to f712fd7 Compare December 16, 2024 10:50

ppryga-nordic added this to the 2.9.0-nRF54H20 milestone Dec 16, 2024

rugeGerritsen requested changes Dec 16, 2024

View reviewed changes

rugeGerritsen self-requested a review December 16, 2024 14:04

ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch from f712fd7 to 06d2cc1 Compare December 16, 2024 17:34

ppryga-nordic changed the title ~~mpsl: Add integration layer for MPLS external clock driver~~ mpsl: Add integration layer for MPSL external clock driver Dec 16, 2024

ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch 4 times, most recently from 10bb5d8 to 379cb4d Compare December 16, 2024 19:19

rugeGerritsen requested changes Dec 17, 2024

View reviewed changes

ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch 2 times, most recently from e0be461 to 9d9adf0 Compare December 17, 2024 11:32

bjarki-andreasen approved these changes Dec 17, 2024

View reviewed changes

ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch from 9d9adf0 to 3e8750d Compare December 19, 2024 13:04

NordicBuilder removed manifest-dragoon DNM labels Feb 3, 2025

ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch from d122c36 to 6532f9a Compare February 3, 2025 08:32

nordicjm reviewed Feb 3, 2025

View reviewed changes

ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch 7 times, most recently from 5dc3a46 to 734971a Compare February 5, 2025 12:21

rugeGerritsen requested changes Feb 5, 2025

View reviewed changes

ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch 2 times, most recently from b149f68 to 59a8322 Compare February 7, 2025 15:23

rugeGerritsen requested changes Feb 10, 2025

View reviewed changes

ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch from 59a8322 to 465b4da Compare February 10, 2025 10:08

rugeGerritsen approved these changes Feb 10, 2025

View reviewed changes

piotrkoziar mentioned this pull request Feb 11, 2025

modules: hal_norfic: rise 802.15.4 NET core init priority nrfconnect/sdk-zephyr#2481

Merged

ppryga-nordic added 4 commits February 12, 2025 10:12

ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch from 465b4da to 233c61a Compare February 12, 2025 09:12

nordicjm approved these changes Feb 13, 2025

View reviewed changes

nordicjm merged commit 7ff8355 into nrfconnect:main Feb 13, 2025
13 checks passed

	if (CONFIG_MPSL_USE_EXTERNAL_CLOCK_CONTROL)
	if(CONFIG_MPSL_USE_EXTERNAL_CLOCK_CONTROL)

-	/* As of now assume the HFCLK is runnig after the request was put */
+	/* As of now assume the HFCLK is running after the request was put.
+	 * This puts the responsibility to the user to check if the time since last
+	 * request is larger than the HFXO rampup time.
+	 */

	return z_nrf_clock_calibration_is_in_progress();
	return true;

-	/* This function should not be called from MPSL if CONFIG_CLOCK_CONTROL_NRF2 is set */
+	/* This function is not supported when CONFIG_CLOCK_CONTROL_NRF2 is set.
+	 * Currently MPSL will never use this API for this configuration.
+	 */

	* All these mean failure for MPLS.
	* All these mean failure for MPSL.

	"MPSL must be initialized before Soft Device Controller");
	"MPSL must be initialized before SoftDevice Controller");

-	if (atomic_get(&m_hfclk_refcnt) < (atomic_val_t)1) {
-		return;
-	}
+	if (atomic_get(&m_hfclk_refcnt) < (atomic_val_t)1) {
+		LOG_WRN("Mismatch between HFCLK request/release");
+		return;
+	}

mpsl: Add integration layer for MPSL external clock driver #19498

mpsl: Add integration layer for MPSL external clock driver #19498

Uh oh!

Conversation

ppryga-nordic commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NordicBuilder commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NordicBuilder commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

Uh oh!

NordicBuilder commented Dec 16, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ppryga-nordic Dec 16, 2024 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ppryga-nordic Feb 3, 2025 • 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjarki-andreasen 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!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ppryga-nordic Feb 5, 2025 • 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!

ppryga-nordic commented Dec 13, 2024 •

edited

Loading

NordicBuilder commented Dec 13, 2024 •

edited

Loading

NordicBuilder commented Dec 13, 2024 •

edited

Loading

ppryga-nordic Dec 16, 2024 •

edited

Loading

ppryga-nordic Feb 3, 2025 •

edited

Loading

ppryga-nordic Feb 5, 2025 •

edited

Loading

ppryga-nordic Feb 5, 2025 •

edited

Loading

ppryga-nordic commented Feb 12, 2025 •

edited

Loading