Skip to content

[RFC] drivers: mpsl: clock_control: Provide nrfx_clock shim #2861

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

Closed
wants to merge 6 commits into from

Conversation

nordic-krch
Copy link
Contributor

It seems that latest changes in clock_control and port to clock_control_mpsl created some issues (zephyrproject-rtos/zephyr#28068) and there is another updates in clock control that will come in next upmerge (clock_control using nrfx_clock). It seems like it is simpler to add shim which implements nrfx_clock api using mpsl_clock API and use clock_control directly from zephyr. Here is a first try to do that. As you can see shim is relatively small (nrfx_clock_mpsl.c) some tricks had to be applied to handle interrupt handler which is common for power and clock.

Please let me know what do you think about it.

There is a pr to zephyr nrfconnect/sdk-zephyr#351 which takes zephyr latest changes in clock control driver and adds small kconfig commit (nrfconnect/sdk-zephyr@e9377cf). If we go this approach then i will try to add this commit to upstream.

Zephyr clock control has more changes in pipeline that will need to be addressed here in the future:

zephyr_library_sources(clock_control_mpsl.c)
zephyr_library_sources(nrfx_clock_mpsl.c)

target_include_directories(zephyr_interface BEFORE INTERFACE
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems unnecessary to add this folder as an include folder

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's the trick i mentioned. I do not set CONFIG_NRFX_CLOCK to not fetch nrfx_clock.c but there is a header nrfx_power_clock.h which handles interrupt handler depending on nrfx_power and nrfx_clock being enabled. I had to replace nrfx version with custom one placed in that folder. It's @anangl area so maybe he will have some better proposal.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is something to use with extreme care.
Used once, then people will start copying, and suddenly every module expects to have their includes first.

Example of an area where this is can be accepted use is in (unit) test-code, where a certain path should come first to avoid the need for specific test headers/code in production code.

Let's keep for now, and stabilize upmerge, and then i'll look at cleaning up.

@rugeGerritsen
Copy link
Contributor

Thanks! It looks like this improves the maintainability a lot

@nordic-krch nordic-krch force-pushed the clock_control branch 3 times, most recently from 81d3475 to 1814e2a Compare September 8, 2020 11:13
#ifndef MPSL_CLOCK_CONTROL_NRFX_POWER_CLOCK_H__
#define MPSL_CLOCK_CONTROL_NRFX_POWER_CLOCK_H__

BUILD_ASSERT(NRFX_CLOCK_ENABLED == 0,"Expected disabled nrfx_clock");
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe even simpler:

#if (NRFX_CLOCK_ENABLED != 0)
#error "Expected disabled nrfx_clock."
#endif

Copy link
Contributor

@anangl anangl left a comment

Choose a reason for hiding this comment

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

I like the approach proposed in this PR. It can save us quite some maintenance trouble.

Comment on lines 10 to 11
#include <drivers/clock_control/nrf_clock_control.h>
#include <nrfx_power.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

No longer needed.

@nordic-krch nordic-krch force-pushed the clock_control branch 4 times, most recently from b35c613 to dfa3ee8 Compare September 14, 2020 06:43
mbolivar-nordic pushed a commit to mbolivar-nordic/sdk-zephyr that referenced this pull request Sep 16, 2020
This is a temporary fix in order to support MPSL in NCS.
This fix must be reverted when
nrfconnect/sdk-nrf#2861 is merged.

Signed-off-by: Torsten Rasmussen <[email protected]>
(cherry picked from commit a649ecaf652cfdf9d2868d4700c5fd2cd344eca4)
@doki-nordic
Copy link
Contributor

Hi, I removed myself from reviewers list, but github somehow also removed other people: @junqingzou, @ryanjh, @rlubos, @lmaciejonczyk, @jhn-nordic, @KAGA164, @simensrostad, @lats1980, @mariuszpos, @maciekfabia @MarekPorwisz. Github does not allow me to add them back probably because there are too many reviewers.

@nordic-krch can you check this.

@hakonfam hakonfam removed their request for review September 25, 2020 08:22
tejlmand added a commit to tejlmand/nrfxlib that referenced this pull request Sep 29, 2020
- 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]>
tejlmand added a commit to tejlmand/nrfxlib that referenced this pull request Oct 1, 2020
- 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]>
tejlmand added a commit to tejlmand/nrfxlib that referenced this pull request Oct 2, 2020
- 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]>
sdk-zephyr: pull/354/head
sdk-mcuboot: pull/120/head
sdk-mcumgr: pull/10/head
sdk-nrfxlib: pull/266/head

Summary of changes needed for this upmerge:

- modules: device: Const-ify all device driver instance pointers.

  See also ttps://github.com/zephyrproject-rtos/zephyr/pull/24873

- jenkins: remove --board-root zephyr/boards

  zephyr/boards are always part of sanity-check and with duplicate board
  checking with: zephyrproject-rtos/zephyr#27549
  this now results in CI failures.

- samples: whitelist -> allow

  sanitycheck: inclusive language

  change whitelist -> allow for all sample.yaml and testcase.yaml files

- scripts: ncs_commands: add hal_quicklogic to _BLOCKED_PROJECTS

  This is a vendor HAL for an SoC family not supported in NCS.

- doc: upmerge changes to docs

  Adjusted `CONFIG_BT_GATT_DIS` -> `CONFIG_BT_DIS` in docs.

- samples: remove GATT infix in conf files for applications and samples

- usb: hid: All hid_ops callbacks get device pointer.

  Updated usb_state.c to new hid_ops callbacks which includes the device
  pointer.

- cmake: bintools elfconvert

  Updated according to new toolchain abstraction.

- partition_manager: cmake: update regex for runner hex file

  The format of the runner yaml has changed. Update the regex used to
  replace the hex_file variable to match it.

Signed-off-by: Håkon Øye Amundsen <[email protected]>
Signed-off-by: Torsten Rasmussen <[email protected]>
nordic-krch and others added 5 commits October 5, 2020 11:44
Replace clock_control_mpsl with nrfx_clock implementation which is
using mpsl_clock API. This way standard clock_control driver is
used and only thin, straightforward nrfx_clock API is replaced with
implementation which is utilizing mpsl_clock API.

Signed-off-by: Krzysztof Chruscinski <[email protected]>

mpsl: removing clock control in mpsl

Signed-off-by: Torsten Rasmussen <[email protected]>
The API for setting IRQ state has changed.
Update to align with the new functionailty.

Ref: NCSDK-6463
Signed-off-by: Håkon Øye Amundsen <[email protected]>
Updated nrf_desktop applications to use upstream
SYSTEM_CLOCK_WAIT_FOR_STABILITY instead of
CLOCK_CONTROL_NRF_K32SRC_BLOCKING.

Signed-off-by: Torsten Rasmussen <[email protected]>
HAS_DTS_I2C is now selected by I2C and
always used as I2C && HAS_DTS_I2C.

It could then be purely removed.

Signed-off-by: Torsten Rasmussen <[email protected]>
@nordic-krch
Copy link
Contributor Author

closing since it is included in upmerge PR.

@nordic-krch nordic-krch closed this Oct 5, 2020
tejlmand added a commit to tejlmand/nrfxlib that referenced this pull request Oct 6, 2020
- 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]>
mbolivar-nordic pushed a commit to nrfconnect/sdk-nrfxlib that referenced this pull request Oct 6, 2020
- 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DNM doc-required PR must not be merged without tech writer approval. manifest RFC Request For Comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants