Move CMSDK Counters/Timers to unified interface #10025

fvincenzo · 2018-09-17T13:48:38Z

The CMSDK Timers (Single/Dual) can be used as a timer or as a counter. The unified interface proposed in #8340 unifies counter.h and rtc.h to provide a common interface.

This patchset makes the CMSDK Timers drivers compliant with the new proposed interface.

Changes:
v5:

Addressed review comments.

v4:

Implemented the new set_wrap/set_alarm API
Fixed a bug
Rebase on the latest topic-counters branch
General code cleanup

v3:

Removed status check in set_wrap_alarm
Rebase on latest master

v2:

Updated device tree for mps2_an385. The timers and counters are now working correctly even of this platform.

nashif · 2018-09-17T13:53:48Z

good to see you back @fvincenzo :)

fvincenzo · 2018-09-17T14:25:41Z

good to see you back @fvincenzo :)
Thanks 😄

drivers/counter/timer_dtmr_cmsdk_apb.c

drivers/counter/timer_tmr_cmsdk_apb.c

codecov-io · 2018-10-01T09:41:53Z

Codecov Report

Merging #10025 into topic-counters will increase coverage by 3.18%.
The diff coverage is n/a.

@@                Coverage Diff                 @@
##           topic-counters   #10025      +/-   ##
==================================================
+ Coverage           50.35%   53.53%   +3.18%     
==================================================
  Files                 320      239      -81     
  Lines               53658    27052   -26606     
  Branches            13377     6521    -6856     
==================================================
- Hits                27017    14482   -12535     
+ Misses              21574     9900   -11674     
+ Partials             5067     2670    -2397

Impacted Files	Coverage Δ
include/bluetooth/buf.h	`0% <0%> (-100%)`	⬇️
include/drivers/bluetooth/hci_driver.h	`0% <0%> (-100%)`	⬇️
include/bluetooth/hci.h	`0% <0%> (-77.78%)`	⬇️
include/misc/byteorder.h	`56.81% <0%> (-40.91%)`	⬇️
subsys/bluetooth/host/hci_core.c	`2.67% <0%> (-38.78%)`	⬇️
subsys/bluetooth/host/uuid.c	`0% <0%> (-34.15%)`	⬇️
include/bluetooth/bluetooth.h	`0% <0%> (-31.58%)`	⬇️
subsys/bluetooth/common/log.c	`0% <0%> (-21.43%)`	⬇️
subsys/net/ip/ipv6_mld.c	`48.85% <0%> (-11.56%)`	⬇️
lib/os/fdtable.c	`31.46% <0%> (-11.24%)`	⬇️
... and 125 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0aebb61...b448f19. Read the comment docs.

nordic-krch · 2018-11-08T06:31:16Z

please fix conflicts

nordic-krch · 2018-11-14T11:19:16Z

please rebase on topic-counters

galak · 2018-11-14T16:43:06Z

please rebase on topic-counters

rebased.

fvincenzo · 2018-11-16T11:35:51Z

please rebase on topic-counters

rebased.

@galak Thanks 👍

fvincenzo · 2018-11-16T15:39:04Z

please rebase on topic-counters

@nordic-krch I did the changes your required in the last iterations of these patches, could you please acknowledge you change requests and close them? Unless you have more comments 😄

drivers/counter/timer_dtmr_cmsdk_apb.c

drivers/counter/timer_tmr_cmsdk_apb.c

drivers/counter/counter_dtmr_cmsdk_apb.c

drivers/counter/timer_dtmr_cmsdk_apb.c

MaureenHelm · 2019-01-15T19:54:44Z

@fvincenzo It looks like some non-counter related stuff slipped into this PR since you submitted it from fvincenzo:master. Will you please move the counter patches to their own branch and resubmit the PR?

nordic-krch · 2019-01-24T08:27:39Z

drivers/counter/counter_dtmr_cmsdk_apb.c

please align to latest changes set_wrap -> set_top_value, get_wrap->get_top_value

The fact that CI is still passing suggests that we're not building this driver in CI. @fvincenzo will you please add counter to the supported list in the appropriate <board>.yaml file? Like this:

zephyr/boards/arm/frdm_kw41z/frdm_kw41z.yaml

Lines 9 to 12 in 817c74f

supported:

- ieee802154

- adc

- counter

nordic-krch · 2019-01-24T08:33:16Z

drivers/counter/counter_dtmr_cmsdk_apb.c

if counter_config_info is correctly set you don't need to provide this function as it will be never called if counter_config_info indicates that there is not channels. See counter_set_channel_alarm in counter.h

nordic-krch · 2019-01-24T08:33:48Z

drivers/counter/timer_tmr_cmsdk_apb.c

name has changed.

nordic-krch · 2019-01-24T08:35:47Z

@fvincenzo i added couple of minor comments. Please rebase on latest topic-counters and update the code. I think that build is green because this driver is never compiled. You could try to add a board to test so that it's verified.

fvincenzo · 2019-01-24T10:33:11Z

@fvincenzo i added couple of minor comments. Please rebase on latest topic-counters and update the code. I think that build is green because this driver is never compiled. You could try to add a board to test so that it's verified.

@nordic-krch thanks for adding the comments. It is quite strange though, the code has been tested on beetle locally for all the drivers I submitted in this pull request and works correctly. I do not like to push untested code for review. Said that I think that the situation is getting confusing (at least on my side 😄): my tree is currently rebased on origin/topic-counters as per your request and it is how I tested my code. Am I doing anything wrong?

nordic-krch · 2019-01-24T11:26:22Z

@fvincenzo that is indeed strange. counter api struct has no .set_wrap field anymore (it's renamed to set_top_value):

zephyr/include/counter.h

Line 111 in 26b5136

counter_api_set_top_value set_top_value;

And in your code you are setting it in the driver. So would expect that compilation will fail 😄

fvincenzo · 2019-01-24T11:43:08Z

@fvincenzo that is indeed strange. counter api struct has no .set_wrap field anymore (it's renamed to set_top_value):
zephyr/include/counter.h

Line 111 in 26b5136

counter_api_set_top_value set_top_value;
And in your code you are setting it in the driver. So would expect that compilation will fail

@nordic-krch Well then we have to work out why this is happening 😄. So are we sure that the topic branch is origin/topic-counters?

This is what the datastructure looks like (top commit f81e4d78702ec32ae811d2c86e5c91ba0f525cad) :

struct counter_driver_api {
	counter_api_start start;
	counter_api_stop stop;
	counter_api_read read;
	counter_api_set_alarm set_alarm;
	counter_api_disable_alarm disable_alarm;
	counter_api_set_wrap set_wrap;
	counter_api_get_pending_int get_pending_int;
	counter_api_get_wrap get_wrap;
	counter_api_get_max_relative_alarm get_max_relative_alarm;
	counter_api_get_user_data get_user_data;
};

nordic-krch · 2019-01-24T11:47:43Z

@fvincenzo you are missing 6 commits in origin/topic-counters: https://github.com/zephyrproject-rtos/zephyr/commits/topic-counters

fvincenzo · 2019-01-24T12:48:35Z

@fvincenzo i added couple of minor comments. Please rebase on latest topic-counters and update the code. I think that build is green because this driver is never compiled. You could try to add a board to test so that it's verified.

@nordic-krch This commits were put on the tree after I updated my pull request (this explains)... Anyway, since the interface changed several times from last September (and I have been rebasing few times to accomodate it), how about you freeze it and than we start the porting of the drivers, otherwise it will always be a race behind you... 😄

nordic-krch · 2019-01-24T12:53:36Z

@fvincenzo sorry for that, unfortunately for driver maintainers there are still active discussions so api will change again but we want to get to master with current api and do the second round once there is a conclusion.

fvincenzo · 2019-01-25T10:08:07Z

@fvincenzo sorry for that, unfortunately for driver maintainers there are still active discussions so api will change again but we want to get to master with current api and do the second round once there is a conclusion.

@nordic-krch That is fine by me. Changing an API is a complex process... and it is better to get it right. From the process stand point, I believe that the drivers porting should start only when the API reaches a versioned (i.e. 1.0) state. So that a driver can be declared compliant with a certain version. When are you going to release the first version?

nordic-krch · 2019-01-29T09:54:07Z

could you rebase your PR so we can merge it? I see that it contains additional 21 commits that shouldn't be there.

galak · 2019-01-29T17:11:17Z

could you rebase your PR so we can merge it? I see that it contains additional 21 commits that shouldn't be there.

rebased

nordic-krch · 2019-01-29T17:40:43Z

how about you freeze it and than we start the porting of the drivers, otherwise it will always be a race behind you... 😄

@fvincenzo It's freezed for now until we go to master. please apply comments so we can merge it.

galak · 2019-01-31T18:40:44Z

@MaureenHelm, @nordic-krch can you guys re-review.

drivers/counter/timer_dtmr_cmsdk_apb.c

drivers/counter/counter_tmr_cmsdk_apb.c

drivers/counter/timer_tmr_cmsdk_apb.c

Add a label property so we can use that in drivers rather than getting from Kconfig. Signed-off-by: Kumar Gala <[email protected]>

The CMSDK Timer can be used as a timer or as a counter. The unified interface proposed in zephyrproject-rtos#8340 unifies counter.h and rtc.h to provide a common interface. This patch modifies the timer implementation of the single timer to make it compliant with the new proposed interface. Signed-off-by: Vincenzo Frascino <[email protected]> Signed-off-by: Kumar Gala <[email protected]>

The CMSDK Dual Timer can be used as a timer or as a counter. The unified interface proposed in zephyrproject-rtos#8340 unifies counter.h and rtc.h to provide a common interface. This patch modifies the timer implementation of the dual timer to make it compliant with the new proposed interface. Signed-off-by: Vincenzo Frascino <[email protected]> Signed-off-by: Kumar Gala <[email protected]>

Enable testing of counter (COUNTER_{D}TMR_CMSDK) on mps2_an385 and enable timers on v2m_beetle (TIMER_{D}TMR_CMSDK). Signed-off-by: Kumar Gala <[email protected]>

Convert cmsdk driver to use new defines so we can remove the dts_fixup.h code for it. Signed-off-by: Kumar Gala <[email protected]>

galak · 2019-02-05T16:57:56Z

@nordic-krch @MaureenHelm can you guys re-review.

nordic-krch

approve with 2 minor comments (seems like description is not updated after taking it from template).

nordic-krch · 2019-02-05T17:24:42Z

dts/bindings/timer/arm,cmsdk-dtimer.yaml

+    label:
+      type: string
+      category: required
+      description: Human readable string describing the device (used by Zephyr for API name)


should that be here?

nordic-krch · 2019-02-05T17:24:51Z

dts/bindings/timer/arm,cmsdk-timer.yaml

+    label:
+      type: string
+      category: required
+      description: Human readable string describing the device (used by Zephyr for API name)


should that be here?

fvincenzo requested review from MaureenHelm, anangl and galak as code owners September 17, 2018 13:48

carlescufi requested a review from nordic-krch September 18, 2018 16:11

nordic-krch mentioned this pull request Sep 18, 2018

Unify counter.h and rtc.h interfaces #8331

Closed

7 tasks

nordic-krch requested changes Sep 24, 2018

View reviewed changes

drivers/counter/timer_dtmr_cmsdk_apb.c Outdated Show resolved Hide resolved

drivers/counter/timer_tmr_cmsdk_apb.c Outdated Show resolved Hide resolved

nordic-krch mentioned this pull request Sep 24, 2018

[DNM] [RFC] drivers: counter: API proposal unifying counter and rtc drivers. #8340

Merged

fvincenzo force-pushed the master branch from 6ba6072 to 30bc696 Compare October 1, 2018 09:08

fvincenzo force-pushed the master branch from 30bc696 to fb9187d Compare October 8, 2018 14:56

galak changed the base branch from master to topic-counters October 30, 2018 16:10

galak force-pushed the master branch from fb9187d to 88933d3 Compare November 14, 2018 16:42

fvincenzo force-pushed the master branch 2 times, most recently from e8d4de9 to bcedfc4 Compare November 16, 2018 15:25

fvincenzo force-pushed the master branch from bcedfc4 to ceb2695 Compare November 16, 2018 17:19

nordic-krch requested changes Nov 27, 2018

View reviewed changes

MaureenHelm added area: Counter area: API Changes to public APIs labels Nov 28, 2018

MaureenHelm requested changes Nov 28, 2018

View reviewed changes

drivers/counter/counter_dtmr_cmsdk_apb.c Outdated Show resolved Hide resolved

drivers/counter/timer_dtmr_cmsdk_apb.c Outdated Show resolved Hide resolved

nordic-krch force-pushed the topic-counters branch from 2e57cff to 215f872 Compare December 4, 2018 10:27

galak mentioned this pull request Dec 13, 2018

ARM beetle/mps2_an385 dt fixes #12076

Merged

nashif force-pushed the topic-counters branch from 215f872 to 6b96890 Compare January 15, 2019 13:45

fvincenzo force-pushed the master branch from ceb2695 to 850927d Compare January 23, 2019 12:15

nordic-krch requested changes Jan 24, 2019

View reviewed changes

nashif force-pushed the topic-counters branch from d71a3fd to db34d5f Compare January 24, 2019 13:55

galak force-pushed the master branch from 850927d to e184e06 Compare January 29, 2019 17:11

galak added this to the v1.14.0 milestone Jan 29, 2019

galak force-pushed the topic-counters branch from 817c74f to 4d8f735 Compare January 31, 2019 18:05

galak force-pushed the master branch from e184e06 to b9356bd Compare January 31, 2019 18:16

nordic-krch requested changes Feb 1, 2019

View reviewed changes

drivers/counter/timer_dtmr_cmsdk_apb.c Outdated Show resolved Hide resolved

drivers/counter/counter_tmr_cmsdk_apb.c Outdated Show resolved Hide resolved

drivers/counter/timer_tmr_cmsdk_apb.c Outdated Show resolved Hide resolved

galak added 5 commits February 5, 2019 09:29

dts: arm_cmsdk_(d)timer: Add label property to (d)timer

94b2b97

Add a label property so we can use that in drivers rather than getting from Kconfig. Signed-off-by: Kumar Gala <[email protected]>

tests: counter_basic_api: Enable tests for ARM CMSDK drivers

91ed76e

Enable testing of counter (COUNTER_{D}TMR_CMSDK) on mps2_an385 and enable timers on v2m_beetle (TIMER_{D}TMR_CMSDK). Signed-off-by: Kumar Gala <[email protected]>

drivers: counter: cmsdk: Convert to new DT_<COMPAT>_<INSTANCE> defines

b448f19

Convert cmsdk driver to use new defines so we can remove the dts_fixup.h code for it. Signed-off-by: Kumar Gala <[email protected]>

galak force-pushed the master branch from 8c83cef to b448f19 Compare February 5, 2019 16:56

nordic-krch approved these changes Feb 5, 2019

View reviewed changes

MaureenHelm approved these changes Feb 5, 2019

View reviewed changes

MaureenHelm merged commit b0d2c86 into zephyrproject-rtos:topic-counters Feb 5, 2019

Move CMSDK Counters/Timers to unified interface #10025

Move CMSDK Counters/Timers to unified interface #10025

Uh oh!

Conversation

fvincenzo commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nashif commented Sep 17, 2018

Uh oh!

fvincenzo commented Sep 17, 2018

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nordic-krch commented Nov 8, 2018

Uh oh!

nordic-krch commented Nov 14, 2018

Uh oh!

galak commented Nov 14, 2018

Uh oh!

fvincenzo commented Nov 16, 2018

Uh oh!

fvincenzo commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MaureenHelm commented Jan 15, 2019

Uh oh!

nordic-krch Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

MaureenHelm Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

nordic-krch Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

nordic-krch Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

nordic-krch commented Jan 24, 2019

Uh oh!

fvincenzo commented Jan 24, 2019

Uh oh!

nordic-krch commented Jan 24, 2019

Uh oh!

fvincenzo commented Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nordic-krch commented Jan 24, 2019

Uh oh!

fvincenzo commented Jan 24, 2019

Uh oh!

nordic-krch commented Jan 24, 2019

Uh oh!

fvincenzo commented Jan 25, 2019

Uh oh!

nordic-krch commented Jan 29, 2019

Uh oh!

galak commented Jan 29, 2019

Uh oh!

nordic-krch commented Jan 29, 2019

Uh oh!

galak commented Jan 31, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

galak commented Feb 5, 2019

Uh oh!

nordic-krch left a comment

fvincenzo commented Sep 17, 2018 •

edited

Loading

codecov-io commented Oct 1, 2018 •

edited

Loading

fvincenzo commented Nov 16, 2018 •

edited

Loading

fvincenzo commented Jan 24, 2019 •

edited

Loading