-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
mpsl: Add integration layer for MPSL external clock driver #19498
Conversation
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 233c61a94e9a9e67bcdd45436bd8b27253737884 more detailssdk-nrf:
Github labels
List of changed files detected by CI (11)
Outputs:ToolchainVersion: 342151af73 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
9ae3f8c
to
a5d7313
Compare
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. |
a5d7313
to
f712fd7
Compare
include/mpsl/mpsl_clock_ctrl.h
Outdated
* @retval -NRF_EPERM Clock control is already initialized. | ||
* @retval -NRF_EINVAL Invalid parameters supplied. | ||
*/ | ||
int32_t mpsl_clock_ctrl_init(void); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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/mpsl/init/Kconfig
Outdated
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
f712fd7
to
06d2cc1
Compare
10bb5d8
to
379cb4d
Compare
#if !defined(CONFIG_MPSL_USE_EXTERNAL_CLOCK_CONTROL) | ||
__ASSERT_NO_MSG(ticks_difference >= 0); | ||
#endif /* CONFIG_MPSL_USE_EXTERNAL_CLOCK_CONTROL */ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
e0be461
to
9d9adf0
Compare
There was a problem hiding this 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
9d9adf0
to
3e8750d
Compare
d122c36
to
6532f9a
Compare
@@ -18,4 +18,8 @@ if (CONFIG_MPSL_USE_ZEPHYR_PM) | |||
add_subdirectory(pm) | |||
endif() | |||
|
|||
if (CONFIG_MPSL_USE_EXTERNAL_CLOCK_CONTROL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (CONFIG_MPSL_USE_EXTERNAL_CLOCK_CONTROL) | |
if(CONFIG_MPSL_USE_EXTERNAL_CLOCK_CONTROL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nordicjm please see:
sdk-nrf/subsys/mpsl/CMakeLists.txt
Line 17 in 7815f14
if (CONFIG_MPSL_USE_ZEPHYR_PM) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the file is wrong and needs fixing (outside the scope of this PR) but new code should be added correctly
5dc3a46
to
734971a
Compare
if (m_hfclk_refcnt < 1) { | ||
return; | ||
} |
There was a problem hiding this comment.
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()
?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
- 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.
/* The function is executed in POST_KERNEL hence sleep is allowed */ | ||
k_msleep(1); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 bool m_hfclk_is_running(void) | ||
{ | ||
/* As of now assume the HFCLK is runnig after the request was put */ |
There was a problem hiding this comment.
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
/* 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(); |
There was a problem hiding this comment.
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
return z_nrf_clock_calibration_is_in_progress(); | |
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 */ |
There was a problem hiding this comment.
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()
/* 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. | |
*/ |
There was a problem hiding this comment.
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.
*/
b149f68
to
59a8322
Compare
@rugeGerritsen @nordicjm @knutel-nordic your comments were addressed. It would be nice if you re-review the PR. |
* # -ERRTIMEDOUT - nRFS service timeout | ||
* # -EIO - nRFS service error | ||
* # -ENXIO - request rejected | ||
* All these mean failure for MPLS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* All these mean failure for MPLS. | |
* All these mean failure for MPSL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -1523,10 +1523,15 @@ static int hci_driver_init(const struct device *dev) | |||
return err; | |||
} | |||
|
|||
#if defined(CONFIG_MPSL_USE_EXTERNAL_CLOCK_CONTROL) | |||
BUILD_ASSERT(CONFIG_BT_LL_SOFTDEVICE_INIT_PRIORITY > CONFIG_MPSL_INIT_PRIORITY, | |||
"MPSL must be initialized before Soft Device Controller"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"MPSL must be initialized before Soft Device Controller"); | |
"MPSL must be initialized before SoftDevice Controller"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
if (atomic_get(&m_hfclk_refcnt) < (atomic_val_t)1) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a warning here?
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()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to both implementation of the function for nrf/nrf2 clock control
59a8322
to
465b4da
Compare
This PR fails on RS tests due to too low init priority of 802.15.4 in Zephyr. PR fixing: zephyrproject-rtos/zephyr#85527 |
Downstream PR: nrfconnect/sdk-zephyr#2481 |
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]>
465b4da
to
233c61a
Compare
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.