drivers: counter: added ctimer driver for lpcexpresso55s69 #37613

tobyfirth · 2021-08-11T12:10:05Z

Added shim driver for the CTIMERs for the lpcexpresso55s69 board.

Signed-off-by: Toby Firth [email protected]

tobyfirth · 2021-08-11T12:14:02Z

I have had to make a couple of changes to the 'test_multiple_alarms' test to enable it to pass for the ctimers driver. The test takes quite a long time to run as it takes about 45 seconds for each timer to loop around which is required for the test. There might be away to make this faster but I couldn't see a clock configuration that would make this the case.

The frequency of the timer for some of the possible clock configurations for a timer looks like it can only be discovered at runtime unless there is a way to get the frequency of some of the clocks that can be used with the timers at compile time with the device tree macros. So the config struct is not a const struct as the driver updates it when the init function is called.

The ctimers don't have any functionality to do the top alarm api but it could be implemented using one of the timers channels but this would reduce the number of avaible channels for the user by 1. Or maybe some sort of dynamically using one of the channels if the user calls that API. I haven't implemented this but would like to hear people's thoughts on enabling the API call in this way.

MaureenHelm

Thank you for the contribution!

Please update west.yml to reference your hal_nxp PR, this will help fix the buildkite CI failure. See https://docs.zephyrproject.org/latest/guides/modules.html#changes-to-existing-module for details.

drivers/counter/counter_mcux_ctimer.c

MaureenHelm · 2021-08-11T19:42:12Z

samples/drivers/counter/alarm/src/main.c

Please move all the whitespace changes to a separate commit

Should be removed now.

MaureenHelm · 2021-08-11T19:43:23Z

tests/drivers/counter/counter_basic_api/src/test_counter.c

Please decrease the indentation to match the other entries.

github-actions · 2021-08-12T18:28:56Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_nxp	zephyrproject-rtos/hal_nxp@`ef99b11`	zephyrproject-rtos/hal_nxp@`6287a07`	zephyrproject-rtos/[email protected]

Note: This message is automatically posted and updated by the Manifest GitHub Action.

tobyfirth · 2021-08-12T18:44:31Z

I think I have updated west.yml correctly but the build still seems to be breaking.

nordic-krch · 2021-08-13T05:05:56Z

drivers/counter/counter_mcux_ctimer.c

i wouldn't do that. It should be possible to setup alarm and then start the timer.

I have moved the code to the init function now.

nordic-krch · 2021-08-13T05:11:34Z

tests/drivers/counter/counter_basic_api/src/test_counter.c

this looks strange. If it's because ctimer does not support setting top value then i would just skip this test for that counter. You can do this by adding set_top_value_capable() to multiple_channel_alarm_capable check.

That feeds back into one of my earlier comments, the ctimers could support this behaviour by making use of one of the channels to provide the resetting behaviour that setting the top value provides. This could be done by reducing the number of user usable channels from 4 to 3, reserving a channel for detecting the set top value and resetting or if the user sets a top value using one of the channels and producing an error if the user wants to use that channel as well as setting a top value.

Or I could just leave the driver as is and exclude the test as you suggest.

mmahadevan108 · 2021-08-19T22:18:44Z

Could you please help rebase and resolve conflicts. That should hopefully resolve CI failures

tobyfirth · 2021-08-20T15:15:57Z

Conflicts are now resolved. And I have properly updated west.yaml correctly so the builds run.

mmahadevan108 · 2021-08-20T16:13:07Z

boards/arm/lpcxpresso55s69/lpcxpresso55s69_cpu0.dts

Should we add this change to lpcxpresso55s69_ns.dts

Good point. Should the config option be also added to Kconfig.defconfig.lpc55S69_cpu1? I am guessing that the ctimers are also addressable for cpu1?

Also since the compile support for the imxrt6xx group of chips was added it should probably be added into the code. I don't actually know which specific boards are based on the imxrt6xx which is why I haven't added the configuration details for the ctimer/counter to be enabled with them.

Don't worry about rt6xx in the main repo yet, we will follow up with another PR. I asked you to go ahead and extend your hal_nxp PR to include rt6xx now because that change is really simple and then we won't have to update west.yml again in the follow-up PR.

I am not sure if we should add to cpu1. For now we can add to _ns.dts file. Also could you please rebase to the latest mainline code to see if it addresses the CI build error.

Updated the _ns.dts and _ns.yaml files and rebased.

The build error seems to be related to the clock_control_mcux_syscon.c file. And something to do with the mcux_lpc_syscon_clock_control_get_subsys_rate and mcux_lpc_syscon_clock_control_init functions.

MaureenHelm

Looks good, almost there.

MaureenHelm · 2021-08-20T17:41:39Z

boards/arm/lpcxpresso55s69/lpcxpresso55s69_cpu0.dts

Don't worry about rt6xx in the main repo yet, we will follow up with another PR. I asked you to go ahead and extend your hal_nxp PR to include rt6xx now because that change is really simple and then we won't have to update west.yml again in the follow-up PR.

MaureenHelm · 2021-08-20T17:44:06Z

drivers/counter/CMakeLists.txt

Please move this line together with the other mcux drivers.

MaureenHelm · 2021-08-20T17:53:26Z

west.yml

The hal_nxp PR has now been merged, so you can update this to the new SHA.

mmahadevan108 · 2021-08-24T16:23:47Z

drivers/clock_control/clock_control_mcux_syscon.c

I think this #endif should move after line 99 so the closing brace for the switch command is available for all test cases.

Didn't spot that. Fixed it now.

Added shim driver for the CTIMERs for the lpcexpresso55s69 board. Fixes: zephyrproject-rtos#22705 Signed-off-by: Toby Firth <[email protected]>

mmahadevan108 · 2021-08-24T20:10:00Z

boards/arm/lpcxpresso55s69/doc/index.rst

 | USB       | on-chip    | USB device                          |
 +-----------+------------+-------------------------------------+
+| COUNTER   | on-chip    | counter                             |
+-----------+------------+-------------------------------------+


Just noticed this, should we change COUNTER to CTIMER

tobyfirth requested review from MaureenHelm, dleach02, mmahadevan108, nashif and nordic-krch as code owners August 11, 2021 12:10

github-actions bot added area: Boards area: Counter area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding area: Documentation area: Modules area: Samples Samples area: Tests Issues related to a particular existing or missing test platform: NXP NXP labels Aug 11, 2021

tobyfirth force-pushed the lpcxpresso55s69_counter branch from 0aa3ddb to d8dc98a Compare August 11, 2021 12:26

MaureenHelm requested changes Aug 11, 2021

View reviewed changes

tobyfirth force-pushed the lpcxpresso55s69_counter branch from d8dc98a to 3be22a9 Compare August 12, 2021 18:28

tobyfirth requested review from carlescufi and galak as code owners August 12, 2021 18:28

github-actions bot added area: API Changes to public APIs manifest west manifest-hal_nxp labels Aug 12, 2021

tobyfirth force-pushed the lpcxpresso55s69_counter branch from 3be22a9 to 46887ab Compare August 12, 2021 18:38

nordic-krch reviewed Aug 13, 2021

View reviewed changes

tobyfirth force-pushed the lpcxpresso55s69_counter branch from 46887ab to 6c32f88 Compare August 13, 2021 10:39

zephyrbot added the area: Clock Control label Aug 19, 2021

zephyrbot requested a review from mbolivar-nordic August 19, 2021 12:41

zephyrbot assigned MaureenHelm Aug 19, 2021

tobyfirth force-pushed the lpcxpresso55s69_counter branch 4 times, most recently from abb9c1b to 6513613 Compare August 20, 2021 13:50

github-actions bot added the DNM This PR should not be merged (Do Not Merge) label Aug 20, 2021

tobyfirth force-pushed the lpcxpresso55s69_counter branch from 6513613 to 24956df Compare August 20, 2021 13:59

mmahadevan108 reviewed Aug 20, 2021

View reviewed changes

tobyfirth force-pushed the lpcxpresso55s69_counter branch from 24956df to 8cd127f Compare August 20, 2021 16:35

MaureenHelm requested changes Aug 20, 2021

View reviewed changes

tobyfirth force-pushed the lpcxpresso55s69_counter branch from 8cd127f to 9189861 Compare August 23, 2021 10:51

github-actions bot removed the DNM This PR should not be merged (Do Not Merge) label Aug 23, 2021

tobyfirth force-pushed the lpcxpresso55s69_counter branch from 9189861 to 03635cc Compare August 23, 2021 14:48

mmahadevan108 reviewed Aug 24, 2021

View reviewed changes

drivers: counter: added ctimer driver for lpcexpresso55s69

84f7bea

Added shim driver for the CTIMERs for the lpcexpresso55s69 board. Fixes: zephyrproject-rtos#22705 Signed-off-by: Toby Firth <[email protected]>

tobyfirth force-pushed the lpcxpresso55s69_counter branch from 03635cc to 84f7bea Compare August 24, 2021 17:09

MaureenHelm approved these changes Aug 24, 2021

View reviewed changes

MaureenHelm added this to the v2.7.0 milestone Aug 24, 2021

mmahadevan108 approved these changes Aug 24, 2021

View reviewed changes

mmahadevan108 reviewed Aug 24, 2021

View reviewed changes

nashif merged commit 893bfc0 into zephyrproject-rtos:main Aug 24, 2021

drivers: counter: added ctimer driver for lpcexpresso55s69 #37613

drivers: counter: added ctimer driver for lpcexpresso55s69 #37613

Uh oh!

Conversation

tobyfirth commented Aug 11, 2021

Uh oh!

tobyfirth commented Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaureenHelm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobyfirth commented Aug 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmahadevan108 commented Aug 19, 2021

Uh oh!

tobyfirth commented Aug 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaureenHelm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tobyfirth commented Aug 11, 2021 •

edited

Loading

github-actions bot commented Aug 12, 2021 •

edited

Loading