drivers: counter: Add support for NXP Multirate Timer Peripheral #64801

decsny · 2023-11-03T17:48:23Z

Add support for NXP Multirate Timer Peripheral

Enabled on LPC55XXX, RT 3 digit

Depends on #64787

mmahadevan108 · 2023-11-03T20:24:56Z

drivers/counter/counter_nxp_mrt.c

+	const struct device *clock_dev;
+	clock_control_subsys_t clock_subsys;
+	void (*irq_config_func)(const struct device *dev);
+	struct nxp_mrt_channel_data *data;


Can you please clarify why the nxp_mrt_channel_data is stored inside the config structure and not provided to dev structure by passing it to DEVICE_DT_INST_DEFINE

I'm not sure what you mean. It's not stored inside the config structure, it's stored in RAM, and it is provided to the dev structure (line 247 in the DEVICE_DT_DEFINE?)

I am referring to void *data; in struct device

mmahadevan108 · 2023-11-03T20:44:03Z

drivers/counter/counter_nxp_mrt.c

+
+
+	/* If not currently running, nothing else to do but wait for counter_start */
+	if (!active) {


I don't follow this. Why cannot we change the top value if counter is not active?

setting the value will start the timer, see line 99 and 107 comments

I improved this comment on line 107 in the latest push

Add binding for nxp,mrt and nxp,mrt-channel. MRT is NXP multirate timer, a simple timer with multiple independent channels. Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

Add code to handle MRT subsys clock to LPC syscon driver Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

decsny · 2023-11-06T17:41:17Z

updated with rebase and clarifying comment

danieldegrasse · 2023-11-07T15:40:20Z

drivers/counter/CMakeLists.txt

@@ -48,3 +48,4 @@ zephyr_library_sources_ifdef(CONFIG_COUNTER_TIMER_GD32		counter_gd32_timer.c)
 zephyr_library_sources_ifdef(CONFIG_COUNTER_SNPS_DW             counter_dw_timer.c)
 zephyr_library_sources_ifdef(CONFIG_COUNTER_SHELL               counter_timer_shell.c)
 zephyr_library_sources_ifdef(CONFIG_COUNTER_TIMER_RPI_PICO	counter_rpi_pico_timer.c)
+zephyr_library_sources_ifdef(CONFIG_COUNTER_NXP_MRT		counter_nxp_mrt.c)


Nit- can you align indentation here? It seems that a few lines, like the rpi_pico_timer are deviating from convention, but I don't see why we should add to the trend

it looks aligned to me, I think the issue is tabs vs spaces, I'll change it to spaces

danieldegrasse · 2023-11-07T15:44:51Z

drivers/counter/counter_nxp_mrt.c

+				POST_KERNEL,					\
+				CONFIG_COUNTER_INIT_PRIORITY, NULL);
+
+#define DT_DRV_COMPAT nxp_mrt


Nit- by convention, we usually define this near the top of the file (unless there is a good reason not to?)

the reason no longer applies to this revision so I will move it

danieldegrasse · 2023-11-07T15:53:00Z

drivers/counter/counter_nxp_mrt.c

+	}									\
+										\
+	static struct nxp_mrt_channel_data					\
+		nxp_mrt_##n##_channel_datas[DT_INST_PROP(n, num_channels)];	\


Could we use DT_FOREACH_CHILD with some macro to count the number of children MRT nodes here, instead of encoding it using a devicetree property?

That is overcomplicated for no reason. It is perfectly valid to encode the number of channels in DT anyways because it varies based on IP version

Not saying it is invalid to encode the number of channels in DT. I saw this as a change to reduce duplicated information, but it is just a suggestion

danieldegrasse · 2023-11-07T15:56:50Z

drivers/counter/counter_nxp_mrt.c

+	const struct nxp_mrt_config *config = dev->config;
+	struct nxp_mrt_channel_data *data = dev->data;
+	MRT_Type *base = config->base;
+	int channel_id = data - config->data;


Excellent trick to get the channel id:) . Could we define some kind of macro here for this, and add a documentation comment on that macro? I think that will make this a bit easier to maintain in the future.

danieldegrasse · 2023-11-07T16:06:21Z

dts/bindings/counter/nxp,mrt.yaml

+# Copyright 2023 NXP
+# SPDX-License-Identifier: Apache-2.0
+
+description: NXP Multirate Timer


Could we use child-binding for the MRT channels? I guess the issue with that might be that the Kconfig symbol depends on both DT_HAS_xxx_ENABLED symbols.

I tried, it is not convenient

Add driver for NXP Multirate Timer Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

Update counter test to test NXP MRT devices Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

Enable NXP MRT on RT5xx soc and MIMXRT595_EVK board Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

Add NXP MRT to RT6xx DT definition and add peripheral reset to soc.c Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

Support NXP MRT on LPC55XXX SOC series, enable on lpcxpresso55s69_cpu0, add test overlay to counter basic api test Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

decsny added area: Drivers area: Counter platform: NXP NXP labels Nov 3, 2023

decsny requested review from nordic-krch, nashif, carlescufi, galak, MaureenHelm, mmahadevan108 and dleach02 as code owners November 3, 2023 17:48

zephyrbot added area: Clock Control area: Devicetree Binding PR modifies or adds a Device Tree binding labels Nov 3, 2023

zephyrbot requested review from bperseghetti, danieldegrasse, dbaluta, DerekSnell, EmilioCBen, manuargue, PetervdPerk-NXP and yvanderv November 3, 2023 17:49

zephyrbot assigned nordic-krch Nov 3, 2023

decsny force-pushed the feature/nxp_mrt branch 2 times, most recently from fe02aea to cdaedb0 Compare November 3, 2023 18:06

mmahadevan108 reviewed Nov 3, 2023

View reviewed changes

decsny added 2 commits November 6, 2023 11:40

dts: bindings: Add binding for NXP Multirate Timer

eedb1a2

Add binding for nxp,mrt and nxp,mrt-channel. MRT is NXP multirate timer, a simple timer with multiple independent channels. Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

drivers: clock_control_mcux_syscon: Add MRT subsys

4e1415d

Add code to handle MRT subsys clock to LPC syscon driver Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

decsny force-pushed the feature/nxp_mrt branch from cdaedb0 to 9c3848c Compare November 6, 2023 17:40

danieldegrasse reviewed Nov 7, 2023

View reviewed changes

decsny force-pushed the feature/nxp_mrt branch from 9c3848c to 8cbc804 Compare November 8, 2023 21:23

decsny added 5 commits November 8, 2023 16:58

drivers: counter: Add NXP MRT driver

5b1ea6b

Add driver for NXP Multirate Timer Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

tests: counter: Update for NXP MRT

6436f06

Update counter test to test NXP MRT devices Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

soc: rt5xx: Enable NXP MRT

eedd2a7

Enable NXP MRT on RT5xx soc and MIMXRT595_EVK board Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

soc: rt6xx: Add NXP MRT

698adff

Add NXP MRT to RT6xx DT definition and add peripheral reset to soc.c Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

soc: lpc55xxx: Support, enable, test NXP MRT

1a45995

Support NXP MRT on LPC55XXX SOC series, enable on lpcxpresso55s69_cpu0, add test overlay to counter basic api test Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

decsny force-pushed the feature/nxp_mrt branch from 8cbc804 to 1a45995 Compare November 8, 2023 22:58

danieldegrasse approved these changes Nov 9, 2023

View reviewed changes

mmahadevan108 approved these changes Nov 9, 2023

View reviewed changes

carlescufi merged commit fef0018 into zephyrproject-rtos:main Nov 10, 2023
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: counter: Add support for NXP Multirate Timer Peripheral #64801

drivers: counter: Add support for NXP Multirate Timer Peripheral #64801

decsny commented Nov 3, 2023

mmahadevan108 Nov 3, 2023

decsny Nov 3, 2023 •

edited

mmahadevan108 Nov 9, 2023

mmahadevan108 Nov 3, 2023

decsny Nov 3, 2023 •

edited

decsny Nov 6, 2023 •

edited

decsny commented Nov 6, 2023

danieldegrasse Nov 7, 2023

decsny Nov 8, 2023

danieldegrasse Nov 7, 2023

decsny Nov 8, 2023

danieldegrasse Nov 7, 2023

decsny Nov 7, 2023

danieldegrasse Nov 7, 2023

danieldegrasse Nov 7, 2023

decsny Nov 8, 2023

danieldegrasse Nov 7, 2023

decsny Nov 8, 2023



		/* If not currently running, nothing else to do but wait for counter_start */
		if (!active) {

drivers: counter: Add support for NXP Multirate Timer Peripheral #64801

drivers: counter: Add support for NXP Multirate Timer Peripheral #64801

Conversation

decsny commented Nov 3, 2023

Choose a reason for hiding this comment

decsny Nov 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

decsny Nov 3, 2023 • edited

Choose a reason for hiding this comment

decsny Nov 6, 2023 • edited

Choose a reason for hiding this comment

decsny commented Nov 6, 2023

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

decsny Nov 3, 2023 •

edited

decsny Nov 3, 2023 •

edited

decsny Nov 6, 2023 •

edited