[WIP] drivers: mfd: add support for stm32 timers #20293

maxvankessel · 2019-11-03T13:48:53Z

So, I was a bit upset due to extensibility of the timer driver for the stm32 within Zephyr.
I opened up an enhancement issue #20262. And started digging around how Linux does this.

So I got inspired by by the MFD driver for stm32-timers, see stm32-timers.

I'm using the stm32f4 series as a motorcontroller (with hall sensors and field oriented control). And the timer can do a pretty good job here. But I want to use the timer as a base for other modules, like hall sensors and 3 phase generation. So I got it to work, and changed it only for the F4 series (all is based on the original drive, but I moved from the bulky HAL driver to to LL driver).

Also I moved from the DTS fixups to the instance based implementation. But it still needs more cleanup.

So I would like to hear from you, if it's useful I will cleanup things further so it can be merged.

TODO's

Fix DTS files for complete stm32 series
Cleanup dts_fixups
Test on more platforms

erwango · 2019-11-04T12:53:18Z

@maxvankessel, thanks for this contribution. Looks pretty nice with interesting bonuses like transition to LL API.
Since we're getting closer to V2.1 end of merge window. I'm tagging V2.2 as a merge target.
Also it is likely that closer review won't happen before V2.1 release.

erwango · 2019-11-22T10:28:20Z

@avisconti , FYI, IIRC, you need a timer driver for some application

avisconti · 2019-11-22T10:55:48Z

@avisconti , FYI, IIRC, you need a timer driver for some application

You remember correctly. It was the #17140 PR, which is frozen for the moment as there was some
points to be clarified (timer driver was one of the points).
I may come back again to this PR in the next future.
Thanks!!

erwango

I'm not a timer driver user, so I can't comment much on functional side of the driver.
I'l try to get more reviewers on that side.

Though, I've made a first review in term of integration in zephyr driver model. And this is already in good shape on that side. Few comments though:

To match other divers naming convention, please rename timer driver to timer_ll_stm32.c (or mfd_timer_...).
Also rename functions with same name space timer_stm32_enable, ...
Split your PR in few commits. I think that at least the mfd introduction might be reviewed in wider audience, even tought it is stm32 specific (keep one PR for now though)

drivers/mfd/stm32_timers.c

erwango · 2019-11-22T13:25:44Z

drivers/mfd/stm32_timers.c

Are there stm32 series on which this is not working ?

The comment corresponds to TIM_CR1_DIR_Pos. Maybe you could give some comments on it.

My opinion: TIM_CR1_DIR_Pos is not very portable over several series. It might be different on other devices. (I've seen this on the ADC)

Same goes for: TIM_CR1_CMS_Pos, TIM_SMCR_SMS_Pos, TIM_SMCR_TS_Pos, TIM_CR2_MMS_Pos

drivers/mfd/stm32_timers.c

drivers/mfd/stm32_timers.h

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

drivers/pwm/pwm_stm32.c

erwango · 2019-12-12T16:32:54Z

@maxvankessel, I'm starting having a deeper look.

To see real interest of the mfd, it would be nice to have an (even light) implementation of another driver based on this mfd_timer. Current in-tree potential user would be counter(timer).
Would you have time to work on a mfd based stm32 counter_timer ?

Also, to ease the review, it would be nice to split the PR in commits (spliting based on subystem would be a good start for now). This would be anyway a pre-requisite if we want to get this merged.

maxvankessel · 2019-12-14T22:04:47Z

@erwango So I've split the commit, removed pwm contents from the mfd commit.

What do you mean with "counter"?

Current in-tree potential user would be counter(timer).

Quadrature decoder? Or a capture interface? What is it you want to count?

Something went wrong on a rebase, so thats why it closed and reopend

zephyrbot · 2019-12-14T23:03:08Z

Some checks failed. Please fix and resubmit.

checkpatch issues

-:111: ERROR:CODE_INDENT: code indent should use tabs where possible
#111: FILE: drivers/mfd/mfd_timer_stm32.c:50:
+                                clock_control_subsys_t *sub_system)$

-:111: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#111: FILE: drivers/mfd/mfd_timer_stm32.c:50:
+                                clock_control_subsys_t *sub_system)$

- total: 1 errors, 1 warnings, 482 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

NOTE: Whitespace errors detected.
      You may wish to use scripts/cleanpatch or scripts/cleanfile

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Codeowners issues

New files added that are not covered in CODEOWNERS:

drivers/mfd/CMakeLists.txt
drivers/mfd/Kconfig

Please add one or more entries in the CODEOWNERS file to cover those files

Nits issues

Missing newline at end of 'drivers/mfd/Kconfig'. Check your text editor settings.

Please remove blank lines at end of 'drivers/mfd/mfd_timer_stm32.c'

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

erwango · 2019-12-16T09:54:18Z

@erwango So I've split the commit, removed pwm contents from the mfd commit.

Thanks for having taken the point into account, but I didn't meant to remove the PWM impact purely for the PR. Splitting the PR into several commits is generally a good way to ease the review process (then maybe we split the PR into several PRs to speed up the merge of large PRs, but we're not there yet).

What do you mean with "counter"?
Quadrature decoder? Or a capture interface? What is it you want to count?

include/drivers/counter.h is zephyr public API for counter and timer drivers.
You can find it's documentation here: https://docs.zephyrproject.org/latest/reference/peripherals/counter.html

Since interest of mfd is to serve multiple drivers, it would be valuable to demonstrate it within this change. This is why it would be valuable in your PR to have both pwm and counter demonstrated as timer-mfd users.

maxvankessel · 2020-02-26T10:22:03Z

@henrikbrixandersen I hear what you are saying, it's more like a base for all timer based drivers.
The ST timer is a base for multiple drivers, but I'm not sure if it could do more than one function at a time. It could trigger the DMA.

I have made a quadrature decoder, and I realised it was a near copy of the PWM driver.
Then I implemented a capture interface, again a near copy of the PWM driver.
So I needed a base driver, because there were more features I needed, e.g. HALL driver and PWM outputs for motor control. A timer could also be used to sync USB timings by receiving a SOF event. Same goes for the SAI driver.

Currently the PWM driver holds the complete timer instance, and a driver couldn't be written without kernel modification.

I was not planning to modify the PWM api, as the way Linux implements several features for the timer driver (capture, encoder, hall). But I was stuck using the current implementation.

Suggestions are welcome.

henrikbrixandersen · 2020-02-26T10:35:28Z

Suggestions are welcome.

@maxvankessel I am currently implementing quadrature decoder, frequency counter and pulse width measurement support atop of the NXP FlexTimer. What I have done so far is to use different device tree bindings per function, but this too feel wrong (the devicetree should describe the hardware, not the software configuration of the hardware). I too have a bit of duplicated code between the various drivers utilizing the FlexTimer, but it's not too much, I think.

A common approach to this problem (one device instance, different drivers on top) within Zephyr would be nice. IMO, this should be taken into consideration in the current devicetree rework effort. /cc @galak.

Added multi functional device driver. Added support for new timer base (mfd stm32 timers), which aims to be more versatile. Signed-off-by: Max van Kessel <[email protected]>

instance 5 Seemed to have slip past Signed-off-by: Max van Kessel <[email protected]>

Deprecation of the parent-bus and child-bus Signed-off-by: Max van Kessel <[email protected]>

Corrected clock control include Signed-off-by: Max van Kessel <[email protected]>

gmarull

@maxvankessel Overall looks like a nice addition, as it would allow to implement more specific drivers on top of STM32 timer generic functionality. I have a custom driver that would benefit from a these changes (I'm using a master & slave configuration). Let me know if I can help on testing or code changes.

gmarull · 2020-03-20T18:40:06Z

drivers/mfd/mfd_timer_stm32.c

+	u32_t tim_clk, apb_psc;
+
+	if (pclken->bus == STM32_CLOCK_BUS_APB1) {
+		apb_psc = CONFIG_CLOCK_STM32_APB1_PRESCALER;


In order to support H7 series PSC needs to use D2PRE1 / D2PRE2 (see #23590)

gmarull · 2020-03-20T18:41:27Z

drivers/mfd/mfd_timer_stm32.c

+	/* enable clock */
+	if (clock_control_on(data->clock,
+				(clock_control_subsys_t *)&cfg->pclken) == 0) {
+		err = 0;


I'd return -EIO here (avoids if below)

gmarull · 2020-03-20T18:54:18Z

drivers/mfd/mfd_timer_stm32.c

+	const struct mfd_timer_stm32_config *cfg = DEV_CFG(dev);
+
+	/* Timer is enabled by master timer if slave mode is set */
+	if ((cfg->slave_mode << TIM_SMCR_SMS_Pos) != LL_TIM_SLAVEMODE_TRIGGER) {


I'd just save LL_TIM_SLAVEMODE_TRIGGER to cfg->slave_mode, see next comment

gmarull · 2020-03-20T18:55:53Z

drivers/mfd/mfd_timer_stm32.c

+			LL_TIM_SetTriggerInput(tim, mode);
+		}
+
+		mode = cfg->master_trig << TIM_CR2_MMS_Pos;


Instead of using cfg->var << SHIFT strategy I'd implement some helper functions to convert from dt binding values to actual definitions in the LL headers. I think it would be more portable.

Example of the mentioned idea: https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/serial/uart_stm32.c#L139

gmarull · 2020-03-20T18:58:11Z

drivers/mfd/mfd_timer_stm32.c

+		mode = cfg->slave_mode << TIM_SMCR_SMS_Pos;
+		LL_TIM_SetSlaveMode(tim, mode);
+
+		if (cfg->slave_mode != 0) {


if shift (<<) approach is changed as suggested, cfg->slave_mode != LL_TIM_SLAVEMODE_DISABLED would be more readable.

gmarull · 2020-03-20T19:00:00Z

drivers/mfd/mfd_timer_stm32.c

+		LL_TIM_SetTriggerOutput(tim, mode);
+
+		/* TODO: create binding for me ?*/
+		LL_TIM_DisableARRPreload(tim);


That should be configurable as TODO suggests

maxvankessel requested review from erwango and rsalveti as code owners November 3, 2019 13:48

erwango added area: API Changes to public APIs area: Timer Timer labels Nov 4, 2019

erwango added this to the v2.2.0 milestone Nov 4, 2019

maxvankessel force-pushed the pr/mfd-stm32-timers branch from fa9f030 to 679311c Compare November 13, 2019 15:08

erwango added the platform: STM32 ST Micro STM32 label Nov 13, 2019

erwango requested changes Nov 22, 2019

View reviewed changes

maxvankessel force-pushed the pr/mfd-stm32-timers branch from 679311c to 0b11a01 Compare December 8, 2019 15:23

jmbrederveld reviewed Dec 12, 2019

View reviewed changes

drivers/pwm/pwm_stm32.c Outdated Show resolved Hide resolved

erwango requested review from carlescufi and galak December 12, 2019 16:29

erwango added the RFC Request For Comments: want input from the community label Dec 12, 2019

carlescufi requested review from andyross and nordic-krch December 13, 2019 13:35

maxvankessel force-pushed the pr/mfd-stm32-timers branch from 0b11a01 to e042060 Compare December 14, 2019 21:36

maxvankessel closed this Dec 14, 2019

maxvankessel force-pushed the pr/mfd-stm32-timers branch from e042060 to e1827c0 Compare December 14, 2019 22:18

maxvankessel reopened this Dec 14, 2019

zephyrbot added the area: Devicetree label Dec 14, 2019

maxvankessel force-pushed the pr/mfd-stm32-timers branch from 42084a1 to 88aa574 Compare December 14, 2019 23:02

maxvankessel mentioned this pull request Dec 14, 2019

drivers: pwm: allow stm32 pwm to use mfd timer #21401

Closed

maxvankessel force-pushed the pr/mfd-stm32-timers branch from 88aa574 to 944a1c0 Compare December 14, 2019 23:20

drivers: mfd: add support for stm32 mfd timers

63737d4

Added multi functional device driver. Added support for new timer base (mfd stm32 timers), which aims to be more versatile. Signed-off-by: Max van Kessel <[email protected]>

maxvankessel force-pushed the pr/mfd-stm32-timers branch from 944a1c0 to 63737d4 Compare March 16, 2020 19:44

maxvankessel added 3 commits March 16, 2020 22:30

drivers: mfd: add timer instance 5

45dd127

instance 5 Seemed to have slip past Signed-off-by: Max van Kessel <[email protected]>

dts: stm32 timer: comply to new dts tags

9e473a1

Deprecation of the parent-bus and child-bus Signed-off-by: Max van Kessel <[email protected]>

drivers: mfd: change clock control include

8857934

Corrected clock control include Signed-off-by: Max van Kessel <[email protected]>

gmarull reviewed Mar 20, 2020

View reviewed changes

erwango removed the dev-review To be discussed in dev-review meeting label Apr 9, 2020

github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR area: Device Tree area: Devicetree Binding PR modifies or adds a Device Tree binding labels Jun 29, 2020

galak removed the area: Device Tree label Jul 1, 2020

github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jul 1, 2020

nashif added the Stale label Sep 4, 2020

github-actions bot closed this Sep 19, 2020

erwango mentioned this pull request Oct 14, 2021

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

Merged

[WIP] drivers: mfd: add support for stm32 timers #20293

[WIP] drivers: mfd: add support for stm32 timers #20293

Uh oh!

Conversation

maxvankessel commented Nov 3, 2019

Uh oh!

erwango commented Nov 4, 2019

Uh oh!

erwango commented Nov 22, 2019

Uh oh!

avisconti commented Nov 22, 2019

Uh oh!

erwango left a comment • 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!

Uh oh!

erwango Nov 22, 2019

Choose a reason for hiding this comment

Uh oh!

maxvankessel Dec 8, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erwango commented Dec 12, 2019

Uh oh!

maxvankessel commented Dec 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

checkpatch issues

Codeowners issues

Nits issues

Uh oh!

erwango commented Dec 16, 2019

Uh oh!

maxvankessel commented Feb 26, 2020

Uh oh!

henrikbrixandersen commented Feb 26, 2020

Uh oh!

gmarull left a comment

Choose a reason for hiding this comment

Uh oh!

gmarull Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

gmarull Mar 20, 2020 • 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!

gmarull Mar 20, 2020 • 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!

gmarull Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

gmarull Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

gmarull Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

gmarull Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

erwango left a comment •

edited

Loading

maxvankessel commented Dec 14, 2019 •

edited

Loading

zephyrbot commented Dec 14, 2019 •

edited

Loading

gmarull Mar 20, 2020 •

edited

Loading

gmarull Mar 20, 2020 •

edited

Loading