Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ppryga-nordic
Copy link
Contributor

@ppryga-nordic ppryga-nordic commented Dec 13, 2024

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.

@ppryga-nordic ppryga-nordic requested review from a team as code owners December 13, 2024 09:27
@github-actions 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
Copy link
Contributor

NordicBuilder commented Dec 13, 2024

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
Copy link
Contributor

NordicBuilder commented Dec 13, 2024

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 34

Inputs:

Sources:

sdk-nrf: PR head: 59a8322ea6fe6a93299b80bb66854074e8738e16

more details

sdk-nrf:

PR head: 59a8322ea6fe6a93299b80bb66854074e8738e16
merge base: 9b0572b0e7fd01873cc06ef12a3a780cf57e52a6
target head (main): 6887b6ffaf73447742f7e776fbc38d0762be6fea
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
    • sdk-nrf test count: 1080
  • ❌ Integration tests
    • ✅ desktop52_verification
    • ✅ test_ble_nrf_config
    • ✅ test-fw-nrfconnect-ble_samples
    • ✅ test-fw-nrfconnect-chip
    • ❌ test-fw-nrfconnect-rs
    • ✅ test-fw-nrfconnect-fem
    • ✅ test-fw-nrfconnect-thread
    • ✅ test-fw-nrfconnect-zigbee
    • ✅ test-sdk-find-my
    • ✅ test-sdk-sidewalk
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-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

@ppryga-nordic 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 ppryga-nordic changed the title [WIP] Mpsl nrf clock control integration mpsl: Add integration layer for MPLS external clock driver Dec 16, 2024
@NordicBuilder
Copy link
Contributor

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.

@ppryga-nordic ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch from a5d7313 to f712fd7 Compare December 16, 2024 10:50
@ppryga-nordic ppryga-nordic added this to the 2.9.0-nRF54H20 milestone Dec 16, 2024
Comment on lines 18 to 28
/* In case of use of nrf clock control MPSL is stared on POST_KERNEL level, hence NRF_RTC0 is
* started after system clock which is NRF_RTC1. In case MPLS is configured as clock control driver
* then NRF_RTC0 is started on PRE_KERNEL_1 init level.
*/
#if defined(CONFIG_MPSL_USE_ZEPHYR_CLOCK_CONTROL)
#define REF_RTC NRF_RTC1
#define CMP_RTC NRF_RTC0
#else
#define REF_RTC NRF_RTC0
#define CMP_RTC NRF_RTC1
#endif /* CONFIG_MPSL_USE_ZEPHYR_CLOCK_CONTROL */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code change is unnecessary if set_sys_clock_start_to_bt_clk_start_us() is updated to set sys_clock_start_to_bt_clk_start_us as a signed value. I suggest to change that instead to avoid extra complexity in this module.

That change can be separated out to a separate PR and be merged independently

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted a PR: #20166

drivers/mpsl/clock_control/Kconfig Show resolved Hide resolved
* @retval -NRF_EPERM Clock control is already initialized.
* @retval -NRF_EINVAL Invalid parameters supplied.
*/
int32_t mpsl_clock_ctrl_init(void);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ppryga-nordic ppryga-nordic Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Comment on lines 619 to 620
This option configures LL_SOFTDEVICE system init priority level. The priority must be lower
than the CONFIG_MPLS_SYS_INIT_PRIO_LEVEL due to dependency on the MPSL library.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This option configures LL_SOFTDEVICE system init priority level. The priority must be lower
than the CONFIG_MPLS_SYS_INIT_PRIO_LEVEL due to dependency on the MPSL library.
This option configures the LL_SOFTDEVICE initialization priority level. The priority must be lower
than the CONFIG_MPSL_SYS_INIT_PRIO_LEVEL due to dependency on the MPSL library.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

subsys/mpsl/clock_ctrl/mpsl_clock_ctrl.c Show resolved Hide resolved
Comment on lines 108 to 116
config MPSL_SYS_INIT_LEVEL_POST_KERNEL
bool
default y if MPSL_USE_ZEPHYR_CLOCK_CONTROL
help
Change MPSL system init level to POST_KERNEL. Default init level is PRE_KERNEL_1.
The POST_KERNEL init level must be used in case of use of external clock driver.
That gives integration layer possibilty to use kernel object for clocks handling.
The option shall not be set in case of use of MPLS internal clock driver.
Then the MPSL system init level must be set to PRE_KERNEL_1.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need MPSL_SYS_INIT_LEVEL_POST_KERNEL` or should we always initialize mpsl post-kernel when MPSL_USE_ZEPHYR_CLOCK_CONTROL is set? Otherwise we need to add tests for both configurations

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, use of MPSL_USE_ZEPHYR_CLOCK_CONTROL should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed and used MPSL_USE_EXTERNAL_CLOCK_CONTROL instead

The option shall not be set in case of use of MPLS internal clock driver.
Then the MPSL system init level must be set to PRE_KERNEL_1.

config MPSL_SYS_INIT_PRIO_LEVEL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ppryga-nordic ppryga-nordic Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to: MPSL_INIT_LEVEL

@@ -390,7 +395,14 @@ static int32_t mpsl_lib_init_internal(void)
* 500ppm or better regardless of the value passed in clock_cfg.
*/
memset(&clock_cfg, 0, sizeof(clock_cfg));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would consider passing in NULL when CONFIG_MPSL_USE_ZEPHYR_CLOCK_CONTROL is used. That makes it more clear the configuration is not used and also saves some code size

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change done, the mpsl_init is called with NULL if CONFIG_MPSL_USE_EXTERNAL_CLOCK_CONTROL is set.

#endif /* CONFIG_MPLS_SYS_INIT_LEVEL_POST_KERNEL */

#if defined(CONFIG_MPSL_USE_ZEPHYR_CLOCK_CONTROL) && defined(CONFIG_SOC_SERIES_NRF54HX)
BUILD_ASSERT(CONFIG_MPLS_SYS_INIT_PRIO_LEVEL > CONFIG_NRFS_BACKEND_IPC_SERVICE_INIT_PRIO,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some typos here: MPLS -> MPSL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also fix the commit message and title of this PR :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@rugeGerritsen rugeGerritsen self-requested a review December 16, 2024 14:04
@ppryga-nordic ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch from f712fd7 to 06d2cc1 Compare December 16, 2024 17:34
@ppryga-nordic 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 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
Comment on lines 177 to 179
#if !defined(CONFIG_MPSL_USE_EXTERNAL_CLOCK_CONTROL)
__ASSERT_NO_MSG(ticks_difference >= 0);
#endif /* CONFIG_MPSL_USE_EXTERNAL_CLOCK_CONTROL */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion is no longer needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@ppryga-nordic 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
Copy link
Contributor

@bjarki-andreasen bjarki-andreasen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, definately like the introduction of MPSL_USE_EXTERNAL_CLOCK_CONTROL

@ppryga-nordic ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch from 9d9adf0 to 3e8750d Compare December 19, 2024 13:04
Comment on lines 15 to 17
/* In case of use of nrf clock control MPSL is stared on POST_KERNEL level, hence NRF_RTC0 is
* started after system clock which is NRF_RTC1. In case MPLS is configured as clock control driver
* then NRF_RTC0 is started on PRE_KERNEL_1 init level.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* In case of use of nrf clock control MPSL is stared on POST_KERNEL level, hence NRF_RTC0 is
* started after system clock which is NRF_RTC1. In case MPLS is configured as clock control driver
* then NRF_RTC0 is started on PRE_KERNEL_1 init level.
/* In case of use of nrf clock control MPSL is started on POST_KERNEL level, hence NRF_RTC0 is
* started after system clock which is NRF_RTC1. In case MPSL is configured as clock control driver
* then NRF_RTC0 is started on PRE_KERNEL_1 init level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting these

@ppryga-nordic ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch 2 times, most recently from 5d077d7 to 2bb2cd2 Compare January 7, 2025 07:09
@shanthanordic shanthanordic modified the milestones: 2.9.0-nRF54H20, 3.0.0 Jan 7, 2025
@ppryga-nordic ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch 2 times, most recently from 6280e4b to 8e499b7 Compare January 9, 2025 06:44
@ppryga-nordic ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch from 8e499b7 to 2d80bbe Compare February 3, 2025 06:29
@github-actions github-actions bot removed the manifest label Feb 3, 2025
@ppryga-nordic ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch from d122c36 to 6532f9a Compare February 3, 2025 08:32
@@ -18,4 +18,8 @@ if (CONFIG_MPSL_USE_ZEPHYR_PM)
add_subdirectory(pm)
endif()

if (CONFIG_MPSL_USE_EXTERNAL_CLOCK_CONTROL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (CONFIG_MPSL_USE_EXTERNAL_CLOCK_CONTROL)
if(CONFIG_MPSL_USE_EXTERNAL_CLOCK_CONTROL)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nordicjm please see:

if (CONFIG_MPSL_USE_ZEPHYR_PM)
. The code here is aligned to what was in the file already.

@ppryga-nordic 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
/** @brief Uninitialize MPSL integration with NRF clock control
*
* @retval 0 MPSL clock was uninitialized successfully.
* @retval -NRF_EPERM MPSL was not uninitialized before the call.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @retval -NRF_EPERM MPSL was not uninitialized before the call.
* @retval -NRF_EPERM MPSL was not initialized before the call.

@@ -0,0 +1,33 @@
/*
* Copyright (c) 2024 Nordic Semiconductor ASA
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicking: Code is added in 2025. There are some other occurrences in this PR as well

Suggested change
* Copyright (c) 2024 Nordic Semiconductor ASA
* Copyright (c) 2025 Nordic Semiconductor ASA

Comment on lines 303 to 309
#if defined(CONFIG_CLOCK_CONTROL_NRF_ACCURACY)
m_nrf_lfclk_ctrl_data.accuracy_ppm = CONFIG_CLOCK_CONTROL_NRF_ACCURACY;
#else
m_nrf_lfclk_ctrl_data.accuracy_ppm = MPSL_LFCLK_ACCURACY_PPM;
#endif /* CONFIG_CLOCK_CONTROL_NRF_ACCURACY */
m_nrf_lfclk_ctrl_data.skip_wait_lfclk_started = IS_ENABLED(CONFIG_SYSTEM_CLOCK_NO_WAIT);
m_nrf_hfclk_ctrl_data.startup_time_us = CONFIG_MPSL_HFCLK_LATENCY;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we setting some m_nrf_lfclk_ctrl_data and m_nrf_hfclk_ctrl_data at run-time and others at compile time?
I would expect it is more space efficient to set everything at compile time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change that. As of now all can be initialized during build.
The accuracy_ppm for 54h may be initialized in future in runtime.

Comment on lines 264 to 306
if (m_hfclk_refcnt < 1) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is added for following purposes:

  1. We do not release the clock if there is more than one request.
  2. To avoid calling z_nrf_clock_bt_ctlr_hf_release more times than needed.
  3. 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.


static bool m_hfclk_is_running(void)
{
if (m_hfclk_refcnt < 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for not using atomic_get() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I didn't find the ARM Cotrex-M specific implementation in sdk-zphyr. The one available does plain load: https://github.com/nrfconnect/sdk-zephyr/blob/693769a5c7354efce42af62b6c7f68f77c6678b7/kernel/atomic_c.c#L220-L223.

Anyway I'll change it to make it complete and safe in case someone adds custom implementation.

Comment on lines 78 to 79
/* The function is executed in POST_KERNEL hence sleep is allowed */
k_msleep(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

static void m_hfclk_release(void)
{
/* The z_nrf_clock_bt_ctlr_hf_request doesn't count references to HFCLK,
* it is caller responsibility to do not release the clock if there is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* it is caller responsibility to do not release the clock if there is
* it is caller responsibility to not release the clock if there is


static bool m_hfclk_is_running(void)
{
/* As of now assume the HFCLK is runnig after the request was put */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

static bool m_lfclk_calibration_is_enabled(void)
{
if (IS_ENABLED(CONFIG_CLOCK_CONTROL_NRF_DRIVER_CALIBRATION)) {
return z_nrf_clock_calibration_is_in_progress();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

@ppryga-nordic ppryga-nordic Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


static void m_lfclk_calibration_start(void)
{
/* This function should not be called from MPSL if CONFIG_CLOCK_CONTROL_NRF2 is set */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
*/

Copy link
Contributor Author

@ppryga-nordic ppryga-nordic Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch from 734971a to b149f68 Compare February 7, 2025 15:01
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 <piotr.pryga@nordicsemi.no>
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 <piotr.pryga@nordicsemi.no>
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 <piotr.pryga@nordicsemi.no>
…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 <piotr.pryga@nordicsemi.no>
@ppryga-nordic ppryga-nordic force-pushed the mpsl-nrf-clock-control-integration branch from b149f68 to 59a8322 Compare February 7, 2025 15:23
@ppryga-nordic
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants