drivers: counter: Counter API implementation for STM32F4 Series (TIMER) #39414

kentjhall · 2021-10-13T21:34:18Z

This driver implements the counter API for STM32F4 series general-purpose timers.

erwango

This driver is a long standing missing part in STM32 drivers and it is good having someone taking a stab at it. Thanks for this work.

I guess one of the reason it was missing is the fact cohabitation with pwm driver implementations, which share the same hardware component, has to be sorted out, and first at device tree level. This is missing in this proposal. More comments around this in the review.

Second point which is giving me concern is the integration in Zephyr code base. You chose to use HAL API to its whole extent (including msp_init, HAL_cortex api, ..). This isn't usual and, as I miss description of the implementation, I don't know if this is made on purpose or just a copy/paste of an existing, old fashioned, implementation.
For instance, STM32 PWM driver provides a well "zephyr integrated" driver, LL API based, functional with the same hardware block. So, I would be curious to know why LL API was not selected for this driver.
One reason might be the choice of using ZERO_LATENCY_IRQS, but once again, w/o context, hard for me to judge.

Minor last point would be to rename the driver to stm32 instead of stm32f4. Even if only limited to F4 for now (which is ok), it should be possible to extend, with minor modifications to other series in future.

drivers/counter/Kconfig.stm32f4

drivers/counter/counter_stm32f4_timer.c

erwango · 2021-10-14T08:58:33Z

dts/arm/st/f4/stm32f4.dtsi

Likely, this property is the same as st,prescaler which is used for st,stm32-pwm.
This highlight an issue about device tree description of these timer nodes.
First, I'd suggest to create a new child node binding, similar to st,stm32-pwm, which would be used as reference for counter devices (st,stm32-counter for instance).
Then, some guards will be required in order to avoid resources conflicts between pwm and counter instances (that share the same hardware resources).
Last, it would be nice to be able to have some sort of code commonalisation between pwm and timer, as they will share common functionalities. For reference, this was the intent of #20293. Though I don't consider this last point as blocking for current PR.

Thanks for all the feedback! Almost everything makes sense to me; I guess I should have been following the PWM driver more, I was really just going off of the nrfx counter driver (and swapping out with the STM32’s HAL API) since my research group is using an nrfx board for our existing Zephyr use case. I’m working on swapping my HAL uses for LL. But I’m not sure I understand this point:

Then, some guards will be required in order to avoid resources conflicts between pwm and counter instances (that share the same hardware resources).

I’m not super familiar with DTS stuff, is there a way to implement such guards within the device tree? Or if not, what do you have in mind?

ABOSTM · 2021-10-20T07:34:16Z

@kentjhall It seems part of my comment was missing in my previous review:
Here it is.

@kentjhall,
Thanks for this Pull Request. This is a huge work.
But, even if I guess it works for your case (your F4 boards, with your configuration),
I think it lack some genericity, clock computation is tight to your configuration,
also all STM32 series have General Purpose Timer, so we need to have a generic implementation compatible with other series, and as said @erwango, we need to take care of cohabitation with PWM which shares the same resources, and try to get synergies to avoid code duplication.
That said, I am currently already working on such driver implementation of counter, based on STM32 General Purpose timers (aka TIMx) taking into account the constraints mentioned above. But it is a long journey, tackling genericity is complex, so I still need some consequent work before publishing.

So yes Device Tree allow to dedicate some hardware ressources to some drivers ... but don't worry I will take care of this

ABOSTM

It seems I completely miss my previous review,
Here is my initial comments

dts/bindings/timer/st,stm32-timers.yaml

tests/drivers/counter/counter_basic_api/boards/stm32f4_disco.overlay

drivers/counter/Kconfig

drivers/counter/counter_stm32f4_timer.c

kentjhall · 2021-10-20T19:07:09Z

Okay, I ported everything to the LL API, and also updated the naming and things. Note that despite the naming change, this driver is still only supporting STM32F4 (it should be easy to make it a bit more generic like how PWM driver does, but I don’t feel comfortable attempting it since I don’t have any other boards to test on). I understand that some things will likely still need to be fixed though, like figuring out the clock frequency correctly at compile-time and preventing resource conflicts with the PWM driver. If there are any similar examples I should look to across other drivers (particularly for the latter), let me know and I’ll be happy to implement these myself.

drivers/counter/Kconfig.stm32_timer

ABOSTM

@kentjhall,
Ok, I am fine if you continue on this PR, as you are on the right track and moving fast. Great Thanks.
So I will stop my own development.
Nevertheless I have an ongoing PR (#39628) to remove prescaler from PWM Device Tree binding and move it to timers binding. It has impact on counter timer as prescaler from counter would become useless, and prescaler from timer should be used instead..

tests/drivers/counter/counter_basic_api/boards/stm32f4_disco.overlay

drivers/counter/counter_ll_stm32_timer.c

ABOSTM · 2021-10-25T09:14:31Z

drivers/counter/counter_ll_stm32_timer.c

@nordic-krch , do you know why using arbitrary hardcoded half timer period instead of reusing the same guard period than for absolute (which is configurable) ?

If i recall correctly the assumption was that absolute guard may not be set and half period should be safe to use. We just need something that exceeds potential longest interruption. The only case when half period would fail if half period is less than potential longest preemption but handling counter in such scenario would not make much sense.

drivers/counter/counter_ll_stm32_timer.c

tests/drivers/counter/counter_basic_api/boards/stm32f4_disco.overlay

drivers/counter/counter_ll_stm32_timer.c

kentjhall · 2021-11-11T18:41:46Z

Can ISR_DIRECT_DECLARE() return value 1 to inform scheduler? Or can this be configurable? Maybe could be good idea to propagate this information from callback from application.

https://github.com/zephyrproject-rtos/zephyr/blob/6a838bfaba59f75675b5641305277f6ec9bdff76/drivers/counter/counter_ll_stm32_timer.c#L587

@cz8pru Yes I was wondering about this—does anyone know if I am using IRQ_DIRECT_CONNECT/ISR_DIRECT_DECLARE correctly? At the time I wrote this, I was somewhat blindly following the nrfx counter driver, which has this:

ISR_DIRECT_DECLARE(counter_timer##idx##_isr_wrapper)	       \
		{							       \
			irq_handler(DEVICE_DT_GET(TIMER(idx)));		       \
			/* No rescheduling, it shall not access zephyr primitives. */ \
			return 0;					       \
		}				       \

But I don't really understand what that comment means or if it applies here. Should I be returning 1, and/or invoking ISR_DIRECT_PM?

cz8pru · 2021-11-12T07:31:00Z

Can ISR_DIRECT_DECLARE() return value 1 to inform scheduler? Or can this be configurable? Maybe could be good idea to propagate this information from callback from application.
https://github.com/zephyrproject-rtos/zephyr/blob/6a838bfaba59f75675b5641305277f6ec9bdff76/drivers/counter/counter_ll_stm32_timer.c#L587

@cz8pru Yes I was wondering about this—does anyone know if I am using IRQ_DIRECT_CONNECT/ISR_DIRECT_DECLARE correctly? At the time I wrote this, I was somewhat blindly following the nrfx counter driver, which has this:
ISR_DIRECT_DECLARE(counter_timer##idx##_isr_wrapper)	       \
		{							       \
			irq_handler(DEVICE_DT_GET(TIMER(idx)));		       \
			/* No rescheduling, it shall not access zephyr primitives. */ \
			return 0;					       \
		}				       \
But I don't really understand what that comment means or if it applies here. Should I be returning 1, and/or invoking ISR_DIRECT_PM?

I am also not good guy to help you with this. I have here aplication which needs to periodically readout external device via SPI. So, I need periodically (precious, fast) start SPI transaction (for example every 500us). SPI transaction takes some time (OK, I can do it by using DMA but forget) and it cannot be placed into ISR. I can periodically run in in thread and run this thread which will depends on event. Event will be samaphore which is given in ISR and taken in such thread.
In this case, system has to inform sheduler that ISR is finished and scheduler has to reschedule task and run my thread. To do this, we have to return 1 from ISR_DIRECT_DECLARE.

I can imagine, that there are such situations to not inform scheduler... it will make it easier but with slower response. Is here some expert on drivers who can help us with interrupts? I prefer soltuin, where you can decided from app.
1/ propagate return value from application callback
2/ kconfig option
3/ ...

erwango · 2021-11-15T09:15:10Z

At the time I wrote this, I was somewhat blindly following the nrfx counter driver, which has this:

My initial response would be: unless you know what you are doing, please use IRQ_CONNECT/irq_enable. These are the ones used in most drivers.
Use of direct interrupts will provide gain in performance, but they come with constraints. You should read and understand the following before using them:

kentjhall · 2021-11-15T17:57:09Z

At the time I wrote this, I was somewhat blindly following the nrfx counter driver, which has this:

My initial response would be: unless you know what you are doing, please use IRQ_CONNECT/irq_enable. These are the ones used in most drivers.

Ok, I've gone this route and switched to using IRQ_CONNECT.

ABOSTM · 2021-11-17T08:54:56Z

@kentjhall #39628 has been merged, you can switch to timers node prescaler

kentjhall · 2021-11-17T14:06:52Z

@kentjhall #39628 has been merged, you can switch to timers node prescaler

@ABOSTM Done. Definitely agree this is better, thanks!

ABOSTM

LGTM, except the remaining open point about "freq" which should be computed at runtime but is part of Zephyr API. (see previous comments)
@nordic-krch , as code maintainer, could you help to fix this point ?

drivers/counter/counter_ll_stm32_timer.c

erwango

Last remarks, then, aside from the pending points reported by other reviewers, LGTM.

drivers/counter/counter_ll_stm32_timer.c

tests/drivers/counter/counter_basic_api/boards/stm32f4_disco.conf

- Shim for counter API using LL_TIM driver. - Supports all general-purpose (TIMx) timers. Signed-off-by: Kent Hall <[email protected]>

Updates counter driver test suite to support STM32F4 timers. Signed-off-by: Kent Hall <[email protected]>

erwango

Approved on my side. Thanks for all the efforts and patience @kentjhall, great work!
You can add yourself in CODEOWNER file in front of this driver (could be done in a later PR).

@gmarull, @nordic-krch, @ABOSTM, @mbolivar-nordic, would you mind having a new look ?

ABOSTM

LGTM

nordic-krch

looks good

mbolivar-nordic

DT bits LGTM

Dropping change request, I don't have time to review this one now.

kentjhall requested review from erwango and nordic-krch as code owners October 13, 2021 21:34

github-actions bot added area: Counter area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding area: Tests Issues related to a particular existing or missing test platform: STM32 ST Micro STM32 labels Oct 13, 2021

kentjhall force-pushed the no-pio branch 2 times, most recently from 48f1b17 to 333801b Compare October 14, 2021 02:07

erwango requested changes Oct 14, 2021

View reviewed changes

zephyrbot added the area: Build System label Oct 14, 2021

zephyrbot requested review from ABOSTM, FRASTM, galak, mbolivar-nordic, nashif and tejlmand October 14, 2021 11:45

zephyrbot assigned erwango Oct 14, 2021

ABOSTM suggested changes Oct 20, 2021

View reviewed changes

kentjhall force-pushed the no-pio branch from 333801b to 3f4682b Compare October 20, 2021 18:55

kentjhall force-pushed the no-pio branch from 3f4682b to 6968ec3 Compare October 20, 2021 19:09

cz8pru reviewed Oct 22, 2021

View reviewed changes

drivers/counter/Kconfig.stm32_timer Outdated Show resolved Hide resolved

ABOSTM suggested changes Oct 22, 2021

View reviewed changes

kentjhall force-pushed the no-pio branch from 6968ec3 to b3f5418 Compare October 25, 2021 00:52

nordic-krch reviewed Oct 25, 2021

View reviewed changes

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

ABOSTM suggested changes Oct 25, 2021

View reviewed changes

kentjhall force-pushed the no-pio branch from b3f5418 to 071ec14 Compare October 25, 2021 14:45

cz8pru reviewed Oct 26, 2021

View reviewed changes

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

kentjhall force-pushed the no-pio branch from b0c4a7c to 28b5cf4 Compare November 11, 2021 18:38

kentjhall force-pushed the no-pio branch from 28b5cf4 to a7350a9 Compare November 11, 2021 21:10

erwango mentioned this pull request Nov 15, 2021

https://github.com/zephyrproject-rtos/zephyr/blob/main/include/drivers/counter.h #40350

Closed

kentjhall force-pushed the no-pio branch from a7350a9 to 4cf1297 Compare November 15, 2021 17:56

kentjhall force-pushed the no-pio branch 2 times, most recently from c0040c2 to 89111b5 Compare November 16, 2021 00:40

kentjhall force-pushed the no-pio branch from 89111b5 to 41acc51 Compare November 17, 2021 14:06

erwango self-requested a review November 30, 2021 13:45

ABOSTM reviewed Dec 2, 2021

View reviewed changes

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

erwango requested changes Dec 2, 2021

View reviewed changes

kentjhall force-pushed the no-pio branch from 41acc51 to 5a4e8bf Compare December 2, 2021 18:07

kentjhall mentioned this pull request Dec 8, 2021

drivers: counter: Added get_freq to API #40973

Merged

Kent Hall added 2 commits December 9, 2021 20:04

drivers: counter: Counter API implementation for STM32F4 Series (TIMER).

1248442

- Shim for counter API using LL_TIM driver. - Supports all general-purpose (TIMx) timers. Signed-off-by: Kent Hall <[email protected]>

tests: drivers: counter: Update tests for STM32F4 timer counter driver

cf04bdd

Updates counter driver test suite to support STM32F4 timers. Signed-off-by: Kent Hall <[email protected]>

kentjhall force-pushed the no-pio branch from 5a4e8bf to cf04bdd Compare December 10, 2021 01:08

erwango approved these changes Dec 10, 2021

View reviewed changes

ABOSTM approved these changes Dec 10, 2021

View reviewed changes

nordic-krch approved these changes Dec 10, 2021

View reviewed changes

mbolivar-nordic approved these changes Dec 10, 2021

View reviewed changes

nashif merged commit ce95e03 into zephyrproject-rtos:main Dec 11, 2021

kabeljan mentioned this pull request Jul 8, 2022

Counter support stm32l4 series #47574

Merged

besmarsh mentioned this pull request Mar 17, 2023

Counter support for STM32H7 series and stm32h735g_disco board #55932

Merged

drivers: counter: Counter API implementation for STM32F4 Series (TIMER) #39414

drivers: counter: Counter API implementation for STM32F4 Series (TIMER) #39414

Uh oh!

Conversation

kentjhall commented Oct 13, 2021

Uh oh!

erwango left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erwango Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

kentjhall Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ABOSTM commented Oct 20, 2021

Uh oh!

ABOSTM left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kentjhall commented Oct 20, 2021

Uh oh!

Uh oh!

ABOSTM left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ABOSTM Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

nordic-krch Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kentjhall commented Nov 11, 2021

Uh oh!

cz8pru commented Nov 12, 2021

Uh oh!

erwango commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kentjhall commented Nov 15, 2021

Uh oh!

ABOSTM commented Nov 17, 2021

Uh oh!

kentjhall commented Nov 17, 2021

Uh oh!

ABOSTM left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erwango left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erwango left a comment

Choose a reason for hiding this comment

Uh oh!

kentjhall Oct 19, 2021 •

edited

Loading

erwango commented Nov 15, 2021 •

edited

Loading