drivers: clock_control: nrf driver reimplementation #17293

nordic-krch · 2019-07-03T08:28:38Z

Clock_control has been re-implemented to support new clock control API and calibration algorithm which optionally uses temperature sensor and skips calibration on no temperature change.

Added new API implementation
Added calibration algorithm (calibration support extracted to separate file)
Added test suite
Updated usage of clock _control API in nordic drivers, removed hacky usage of subsys parameter.

Fixes #12114

zephyrbot · 2019-07-03T08:30:44Z

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

drivers/clock_control/nrf_power_clock.c

anangl

Just some initial comments. I'll need some more time to dive into nrf_power_clock.c.

anangl · 2019-07-03T11:38:05Z

drivers/clock_control/nrf_clock_calibration.c

missing CONFIG_ before TEMP_NRF5

anangl · 2019-07-03T11:40:26Z

drivers/clock_control/nrf_clock_calibration.c

static missing

you should use K_WORK_DEFINE for this initialization

anangl · 2019-07-03T12:04:05Z

drivers/clock_control/nrf_clock_calibration.c

Do we really need to involve floats here? The sensor's resolution is 0.25 °C, so multiplying by 4 should be sufficient.

changed to work in 0,25 degree units. Also modified kconfig option to use same units which allows to chose fractional temperature diff that will trigger calibration.

anangl · 2019-07-03T12:09:17Z

drivers/clock_control/nrf_clock_calibration.c

k_work_submit(&temp_measure_work);

anangl · 2019-07-03T12:14:41Z

drivers/timer/nrf_rtc_timer.c

NULL, just for consistency.

anangl · 2019-07-03T12:24:58Z

drivers/sensor/nrf5/temp_nrf5.c

0 -> NULL, for consistency, same below

anangl · 2019-07-03T12:26:13Z

drivers/sensor/nrf5/temp_nrf5.c

static missing

anangl · 2019-07-03T12:34:38Z

drivers/clock_control/nrf_power_clock.c

NRF_POWER_HAS_POFCON is defined in <nrf_power.h>, which may not have been included yet.

now nrf_power.h is always included.

anangl · 2019-07-03T12:40:13Z

drivers/clock_control/nrf_power_clock.c

I guess it would be better to place this initial | after NRF_CLOCK_INT_LF_STARTED_MASK above and use (0) as the "else" part.

anangl · 2019-07-03T13:13:00Z

drivers/clock_control/nrf_power_clock.c

Why is it defined as atomic_t when atomic API is not used for this field?

changed to s32_t

aescolar · 2019-07-03T15:19:55Z

@nordic-krch In https://github.com/aescolar/zephyr/tree/clock_control_calibration_fix you have this branch with a couple of updates for simulation and rebased on top of #17301 . With it the nrf52_bsim works nicely

nordic-krch · 2019-10-03T06:04:20Z

It's been out for 3 months. There are 3 approvals. @carlescufi @ioannisg can we merge this then unless someone plans to review it soon?

aescolar · 2019-10-03T09:40:25Z

For the record, I have no issue with this PR. The only file changed I own is perfectly fine. But as this is a Nordic only change, I do not believe I should approve it. I presume BT and USB maintainers would have a similar feeling as the BT and USB code changes are minor and circumstantial.

ioannisg · 2019-10-03T09:57:57Z

It's been out for 3 months. There are 3 approvals. @carlescufi @ioannisg can we merge this then unless someone plans to review it soon?

I would appreciate if @pabigot could re-review once more (and approve if possible). I guess we can merge this afterwards ;)

pabigot

My comments appear to have been addressed. I haven't tested this or done an in-depth functional review so don't feel comfortable "approving" it, but I don't see any reason it should be kept out.

martintv · 2019-10-03T17:52:14Z

I haven't looked much at the code, this switch to zephyr is all new to me, so I am still confused. So so this is more a question than a suggestion. After this PR we will still use the Calibration timer in the NRF_CLOCK peripheral right? how easy would it be to switch to using some other source for the periodic timer to drive the calibration state machine in zephyr world? we always wanted to do that in SoftDevice, but never did. The calibration timer in in NRF_CLOCK has some big awkward weaknesses, since some register can not be written withing the same 32khz clock cycle as some things happens. Another reason to stop using the calibration timer, is that we want to remove it from the chip, to save silicon, if sw could do without it.

nordic-krch · 2019-10-04T10:05:35Z

@martintv i don't see any obstacles. In the next step we can replace calibration timer with k_timer.

nordic-krch · 2019-10-04T10:06:39Z

@ioannisg see @pabigot comment. What then blocks as from merging it?

ioannisg · 2019-10-04T10:21:59Z

@ioannisg see @pabigot comment. What then blocks as from merging it?

Your description is still blocking this PR @nordic-krch.
#17301 is not reviewed/merged. Please, advice

nordic-krch · 2019-10-04T10:25:37Z

@ioannisg i've updated description as this is no longer true. I don't use functionality which #17301 attempted to add.

ioannisg · 2019-10-04T10:28:04Z

@ioannisg i've updated description as this is no longer true. I don't use functionality which #17301 attempted to add.

OK, great, now this PR is not blocked-by-definition ;)
Could you rebase and run CI (PR hasn't been rebased for ~10 days, AFAICS)

The incomming new NRF clock driver requires extra nrf HAL and drivers functions, which are only supported in the HW models after version 1.8 Signed-off-by: Alberto Escolar Piedras <alpi@oticon.com>

nordic-krch · 2019-10-04T10:29:38Z

@ioannisg, done.

carlescufi

I tested the following:

central_hr on nRF51 DK
peripheral (complete with bonding and reconnection) on nRF51 DK
central_hr on BBC micro:bit
peripheral_hr on BBC micro:bit

joerchan · 2019-10-04T11:34:10Z

There is by default a lot of info lines being printed. Can we change the settings so that that doesn't happen?

[00:10:10.496,673] <inf> clock_control: CLOCK_16M: callback [00:10:10.496,704] <inf> clock_control: CLOCK_16M: node 0x0000000

Tested connections using 2 nrf52840. and 500 PPM, works fine.

carlescufi · 2019-10-04T11:36:58Z

There is by default a lot of info lines being printed. Can we change the settings so that that doesn't happen?
[00:10:10.496,673] <inf> clock_control: CLOCK_16M: callback [00:10:10.496,704] <inf> clock_control: CLOCK_16M: node 0x0000000 
Tested connections using 2 nrf52840. and 500 PPM, works fine.

I saw that as well, I assumed this was my config but now that @joerchan also brings it up I think this is worth cleaning up. @nordic-krch could you please disable those by default?

Reimplementation of clock control driver for nrf platform. It includes latest API changes: asynchronous starting and getting clock status. Additionally, it implements calibration algorithm which optionally skips calibration based on no temperature change. Internal temperature sensor is used for that. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

Added test suite for clock control driver. It covers latest API update for starting clock asynchronously and getting status. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

Added test for LF clock calibration when RC source is used. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

Align to use clock_control_get_status for blocking clock start. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

Align drivers to use new clock control API. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

Test for bbc_microbit was failing due to RAM usage. Reduced main stack size and turned off temperature algorithm to fit. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

nordic-krch · 2019-10-04T12:40:28Z

@carlescufi @joerchan removed those log calls.

@carlescufi I've reduced irq locking time. Additionally, i did measurement to check longest locking time in clock driver vs other lockings (used peripheral_hr sample for that). Clock locking is up to 1.4us while there are many many more longer lockings (up to 2.5ms which seems very odd). In general locking in clock is one of the shortest.

carlescufi · 2019-10-04T12:48:13Z

@carlescufi @joerchan removed those log calls.

@carlescufi I've reduced irq locking time. Additionally, i did measurement to check longest locking time in clock driver vs other lockings (used peripheral_hr sample for that). Clock locking is up to 1.4us while there are many many more longer lockings (up to 2.5ms which seems very odd). In general locking in clock is one of the shortest.

Thank you @nordic-krch. I will merge this as soon as CI passes.

nordic-krch requested review from anangl and cvinayak July 3, 2019 08:28

nordic-krch requested review from MaureenHelm, carlescufi, finikorg, galak, jfischer-no, nashif and thoh-ot as code owners July 3, 2019 08:28

zephyrbot added area: Sensors Sensors area: Counter area: Timer Timer area: Bluetooth area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test EXT Has change or related to ext/ (obsolete) labels Jul 3, 2019

aescolar self-requested a review July 3, 2019 08:33

nordic-krch force-pushed the clock_control_calibration branch 2 times, most recently from 9026473 to e320b09 Compare July 3, 2019 08:56

aescolar previously requested changes Jul 3, 2019

View reviewed changes

drivers/clock_control/nrf_power_clock.c Outdated Show resolved Hide resolved

aescolar requested a review from ioannisg July 3, 2019 09:19

nordic-krch force-pushed the clock_control_calibration branch 3 times, most recently from 604cbae to 8de2f92 Compare July 3, 2019 11:22

nordic-krch mentioned this pull request Jul 3, 2019

arch: arm: Add z_arch_irq_current_prio function #17301

Closed

anangl requested changes Jul 3, 2019

View reviewed changes

nordic-krch force-pushed the clock_control_calibration branch from 8de2f92 to 4dc534d Compare July 4, 2019 06:11

zephyrbot added the area: Boards label Jul 4, 2019

joerchan approved these changes Sep 26, 2019

View reviewed changes

aescolar removed the request for review from thoh-ot October 3, 2019 09:36

ioannisg requested a review from pabigot October 3, 2019 09:56

pabigot reviewed Oct 3, 2019

View reviewed changes

nrf52_bsim: Require v1.8 of the HW models

0ccfc39

The incomming new NRF clock driver requires extra nrf HAL and drivers functions, which are only supported in the HW models after version 1.8 Signed-off-by: Alberto Escolar Piedras <alpi@oticon.com>

nordic-krch force-pushed the clock_control_calibration branch from 1735cec to 364818b Compare October 4, 2019 10:29

carlescufi approved these changes Oct 4, 2019

View reviewed changes

nordic-krch added 6 commits October 4, 2019 14:33

tests: drivers: clock_control: Add test suite

b471f74

Added test suite for clock control driver. It covers latest API update for starting clock asynchronously and getting status. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

tests: drivers: clock_control: Add test for nRF clock calibration

f7670b3

Added test for LF clock calibration when RC source is used. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

bluetooth: controller: Align nrf link layer to new clock control API

6205037

Align to use clock_control_get_status for blocking clock start. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

drivers: Align nrf counter, timer and usb driver to new clock_control

84abe4a

Align drivers to use new clock control API. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

tests: bluetooth: mesh: Fix RAM shortage failure for bbc_microbit

7b6754d

Test for bbc_microbit was failing due to RAM usage. Reduced main stack size and turned off temperature algorithm to fit. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

nordic-krch force-pushed the clock_control_calibration branch from 364818b to 7b6754d Compare October 4, 2019 12:33

carlescufi merged commit 4b59690 into zephyrproject-rtos:master Oct 4, 2019

pabigot mentioned this pull request Oct 10, 2019

samples/bluetooth/hci_uart: change bbc_microbit RAM demands to so test passes #19742

Merged

drivers: clock_control: nrf driver reimplementation #17293

drivers: clock_control: nrf driver reimplementation #17293

Uh oh!

Conversation

nordic-krch commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

zephyrbot commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anangl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aescolar commented Jul 3, 2019

nordic-krch commented Oct 3, 2019

aescolar commented Oct 3, 2019

ioannisg commented Oct 3, 2019

pabigot left a comment

Choose a reason for hiding this comment

martintv commented Oct 3, 2019

nordic-krch commented Oct 4, 2019

nordic-krch commented Oct 4, 2019

ioannisg commented Oct 4, 2019

nordic-krch commented Oct 4, 2019

ioannisg commented Oct 4, 2019

nordic-krch commented Oct 4, 2019

carlescufi left a comment

Choose a reason for hiding this comment

joerchan commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

carlescufi commented Oct 4, 2019

nordic-krch commented Oct 4, 2019

carlescufi commented Oct 4, 2019

Labels

10 participants

nordic-krch commented Jul 3, 2019 •

edited

Loading

zephyrbot commented Jul 3, 2019 •

edited

Loading

joerchan commented Oct 4, 2019 •

edited

Loading