STM32: Enhance PWM driver #5469

vaussard · 2017-12-22T08:28:59Z

This patchset performs the following tasks:

Ease device registration by using a macro to factorize common code
Add support for more PWM devices
Replace hardcoded prescalers by Kconfigs

Fixes #6625

erwango

LGTM for user selectable prescaler.

Code concatenation should be reworked to take F0 into account.

mbolivar

Approach in the last 3 patches looks sane -- seems like this needs a rebase though

vaussard · 2018-01-15T09:48:25Z

Hi @mbolivar and @erwango, thanks for the review!

@erwango : The current driver does not seem to have F0-specific bits. Do you want me to port this driver to F0? I do not have such a board around to test. I can add the necessary part if you tell me what to look for. Is the clock name different on F0 ?

@mbolivar : Rebase done.

codecov-io · 2018-01-15T10:03:40Z

Codecov Report

Merging #5469 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5469   +/-   ##
=======================================
  Coverage   55.03%   55.03%           
=======================================
  Files         483      483           
  Lines       53965    53965           
  Branches    10489    10489           
=======================================
  Hits        29697    29697           
  Misses      19992    19992           
  Partials     4276     4276

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 d8fd97a...a8f946b. Read the comment docs.

erwango · 2018-01-15T10:13:38Z

@vaussard : You don't need to develop PWM for F0, but considering this is an exception to the rule you used to factorize code. I'd like it to be taken into account from the start.
F0: #define LL_APB1_GRP2_PERIPH_TIM1 (same for 15/16/17)
F1/F3/...: #define LL_APB2_GRP1_PERIPH_TIM1 (same for 8/9/10/11/15/16/17)

vaussard · 2018-01-15T11:48:42Z

Ok @erwango, so I will add a new patch to support F0 and see if nucleo_f091rc and alike compile. I won't be able to test however. Would it be acceptable for you?

erwango · 2018-01-15T12:39:55Z

@vaussard : Yes, of course. You can mention in your commit message that minimum is done on F0 to compile but feature should not be considered as available on F0 until futher dev/test.

vaussard · 2018-01-19T08:27:43Z

Hi @erwango, I addressed your comments (and even more):

Added all the PWM up to 20 as I realized that some MCUs can have them
Added the STM32F0-specific clocks
Fixed the compilation of the PWM driver on STMF3 and nucleo_f334r8

dbkinder

+1 for docs

vaussard · 2018-01-29T08:33:36Z

I just rebased due to forced changes in the arm branch.
Sorry @dbkinder could you tell me where you would like to see more docs? Github does not allow me anymore to see the location of your comment due to the rebase.

ydamigos

Could you move commit 8caa2cf3ffbe7c0229b3baed1fc0fd076281fc5f before commit 1aedb4e54e669df869e8cc0c31485b6542c4deb8 and squash commits 1aedb4e54e669df869e8cc0c31485b6542c4deb8 with bc0db4de0ea839431dd713a266dc2cda7f2a2cd0 and aad0b8dc0ca2d8f73f6aedb89d285cd92517ceb7 with 8caa2cf3ffbe7c0229b3baed1fc0fd076281fc5f?

vaussard · 2018-01-29T08:57:49Z

Hi @ydamigos. These patches were done later and the only reason that I did not squashed 1aedb4e and bc0db4d was to avoid facing a huge amount of conflicts. But I will do that if you insist :-)
But I am not sure about moving 8caa2cf before commit 1aedb4e, because it is more related to the STM32F0 changes that are done in bde95a9.

ydamigos · 2018-01-29T09:10:44Z

@vaussard My comments are not blocking. IMO, the proposed changes make the PR easier to check.

But I am not sure about moving 8caa2cf before commit 1aedb4e, because it is more related to the STM32F0 changes that are done in bde95a9.

In that case, I would move both 8caa2cf and bde95a9 before 1aedb4e.

vaussard · 2018-01-29T10:13:40Z

@ydamigos Moving 8caa2cf and bde95a9 before 1aedb4e will force me to rewrite almost all the patches due to the conflicts. IMHO such ordering would be acceptable:

Convert to use a macro
Add all PWM up to PWM20
Use Kconfig for prescalers
Add clock group
Add STM32F0 bits

It boils down to enhancing-before-extending or extending-before-enhancing. I am OK with both but one is more work to rewrite.

ydamigos · 2018-01-29T10:19:27Z

Convert to use a macro
Add all PWM up to PWM20
Use Kconfig for prescalers
Add clock group
Add STM32F0 bits

@vaussard Sounds good.

dbkinder · 2018-01-29T17:41:30Z

Not sure now what docs were updated that I reviewed before the rebase... the API material will update automatically if the header files changed (http://docs.zephyrproject.org/api/io_interfaces.html#pwm-interface). Might be worth looking through the PWM samples docs (http://docs.zephyrproject.org/samples/basic/basic.html) to see if they're still OK or if a new sample would be useful.

jli157 · 2018-05-24T15:27:53Z

@vaussard Nice job! Hopefully this PR can be merged as soon as possible!

vaussard · 2018-05-25T13:59:42Z

Nice job! Hopefully this PR can be merged as soon as possible!

Hi @jli157, thank you. I hope also that this PR can be merged soon, presumably after the freeze window.

recheck

Hi @erwango, I rebased on master and CI succeeded. It looks like all lights are green for merging.

erwango · 2018-05-25T14:05:15Z

@galak : Can you check then?

The registration of each PWM differs only by a few details. These differences can be factorized in order to create a generic registration macro. This has several advantages: - Less code - Easier to add new PWM Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

All the PWM between 1 and 20 can be found on the STM32 depending on the serie and the specific model. Add all the missing PWMs up to PWM20 to plan for the future needs. Tested on STM32L496 with available PWMs. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

The prescalers are currently hardcoded and are not user-selectable. As a result, the frequency of the timer can be inadequate to the task. For instance, the frequency of the 16-bit timers (prescaler 10000) is usually too low to correctly generate of PWM of a few kilohertz. Hardcoded prescalers are replaced by Kconfigs so the user can choose at compile time. The default value of each Kconfig matches the hardcoded prescaler, so the change will be transparent. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

Not all PWM clocks belong to the GRP1, for example TIM1 on STM32F0. Add the group information to the macro to enable supporting these PWMs in the future. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

PWM1 / PWM15 / PWM16 and PWM17 on STM32F0 do not use the same clocks compared to the other series. Add the STM32F0-specific clocks where needed. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

CONFIG_CLOCK_STM32_APB2_PRESCALER does not exist for STM32F0 as it was removed from the RCC Kconfig by commit d067820 ("drivers: clock_control: provide support for stm32f0."). This will break the PWM driver if compiled for STM32F0. Conditionally disable usage of this symbol for STM32F0 as all PWMs are on APB1 for this family. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

The nucleo_f334r8 uses STM32F3_PINMUX_FUNC_PA8_PWM1_CH1 inside its pinmux but it is not defined anywhere. Add the definition into the pinmux file to fix the build of nucleo_f334r8 when enabling CONFIG_PWM. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

In some cases, node label could be generated with "/" character in name string, which prevents compilation Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org> Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

Add new device tree bindings for STM32 Timer and PWM IPs. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

Add available PWM nodes to the existing STM32F0 device trees. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

Add available PWM nodes to the existing STM32F1 device trees. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

Add available PWM nodes to the existing STM32F3 device trees. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

Add available PWM nodes to the existing STM32F4 device trees. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

Add available PWM nodes to the existing STM32L4 device trees. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

Enable the PWM nodes for the board already using them in their pinmux. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

Add fixup info for PWM nodes on STM32F0/F1/F3/F4/L4 and remove the conflicting Kconfig symbols to fully switch STM32 PWM to device tree. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

galak

It seems like we need to update various stm32 board.yaml files to say we support PWM.

Update various stm32 board yaml files to add pwm as a supported feature of the board. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

vaussard · 2018-05-28T07:16:28Z

It seems like we need to update various stm32 board.yaml files to say we support PWM.

Thank you @galak for the update.

jli157 · 2018-05-29T21:11:44Z

Is anything still blocking this PR from being merged? I'm looking forward to using some new things from this PR. Thank you very much!

vaussard · 2018-05-30T07:29:37Z

I guess that new features should not be merged before Zephyr 1.12 is released.

jli157 · 2018-06-11T18:28:53Z

Waiting for this one to be merged. Thanks!

vaussard requested a review from erwango as a code owner December 22, 2017 08:28

galak added the platform: STM32 ST Micro STM32 label Jan 9, 2018

erwango requested changes Jan 9, 2018

View reviewed changes

pfalcon requested review from mbolivar and ydamigos January 11, 2018 11:46

galak force-pushed the arm branch 2 times, most recently from ee5b12f to f95e5fd Compare January 11, 2018 22:07

mbolivar reviewed Jan 12, 2018

View reviewed changes

galak added the area: ARM ARM (32-bit) Architecture label Jan 13, 2018

vaussard force-pushed the stm32_enhance_pwm branch from 035de66 to f1f8942 Compare January 15, 2018 09:42

galak force-pushed the arm branch 2 times, most recently from 6b38c09 to ff730a4 Compare January 23, 2018 14:54

dbkinder previously approved these changes Jan 26, 2018

View reviewed changes

vaussard dismissed dbkinder’s stale review via 9393419 January 29, 2018 08:25

vaussard force-pushed the stm32_enhance_pwm branch from 6371b3f to 9393419 Compare January 29, 2018 08:25

ydamigos reviewed Jan 29, 2018

View reviewed changes

galak force-pushed the arm branch from ff730a4 to d5216a5 Compare January 29, 2018 19:42

vaussard force-pushed the stm32_enhance_pwm branch from 9393419 to c62478e Compare January 30, 2018 07:01

vaussard force-pushed the stm32_enhance_pwm branch from 8033989 to e638508 Compare May 25, 2018 12:34

vaussard and others added 16 commits May 25, 2018 13:08

pwm: stm32: Add clock group information

76f628a

Not all PWM clocks belong to the GRP1, for example TIM1 on STM32F0. Add the group information to the macro to enable supporting these PWMs in the future. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

pwm: stm32: Add STM32F0-specific clocks

397c7f8

PWM1 / PWM15 / PWM16 and PWM17 on STM32F0 do not use the same clocks compared to the other series. Add the STM32F0-specific clocks where needed. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

dts: stm32: Add Timer and PWM binding

e8e4ac2

Add new device tree bindings for STM32 Timer and PWM IPs. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

dts: stm32f0: Add PWM nodes

4a32901

Add available PWM nodes to the existing STM32F0 device trees. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

dts: stm32f1: Add PWM nodes

205729a

Add available PWM nodes to the existing STM32F1 device trees. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

dts: stm32f3: Add PWM nodes

386889f

Add available PWM nodes to the existing STM32F3 device trees. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

dts: stm32f4: Add PWM nodes

06dd8e0

Add available PWM nodes to the existing STM32F4 device trees. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

dts: stm32l4: Add PWM nodes

3ca3702

Add available PWM nodes to the existing STM32L4 device trees. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

dts: stm32: Enable PWM nodes on selected boards

fb6b762

Enable the PWM nodes for the board already using them in their pinmux. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

pwm: stm32: Add PWM fixup for STM32* and remove Kconfig options

75ce743

Add fixup info for PWM nodes on STM32F0/F1/F3/F4/L4 and remove the conflicting Kconfig symbols to fully switch STM32 PWM to device tree. Signed-off-by: Florian Vaussard <florian.vaussard@gmail.com>

galak requested changes May 25, 2018

View reviewed changes

boards: arm: stm32: Added pwm to supported list

a8f946b

Update various stm32 board yaml files to add pwm as a supported feature of the board. Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

galak force-pushed the stm32_enhance_pwm branch from e638508 to a8f946b Compare May 25, 2018 18:35

galak approved these changes May 25, 2018

View reviewed changes

galak merged commit 558eac2 into zephyrproject-rtos:master Jun 12, 2018

anangl mentioned this pull request Apr 12, 2019

drivers: pwm_nrf5_sw: Fix the driver and make it support nRF9x SoCs #15287

Closed

STM32: Enhance PWM driver #5469

STM32: Enhance PWM driver #5469

Uh oh!

Conversation

vaussard commented Dec 22, 2017 • edited by nashif Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erwango left a comment

Choose a reason for hiding this comment

Uh oh!

mbolivar left a comment

Choose a reason for hiding this comment

Uh oh!

vaussard commented Jan 15, 2018

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

erwango commented Jan 15, 2018

Uh oh!

vaussard commented Jan 15, 2018

Uh oh!

erwango commented Jan 15, 2018

Uh oh!

vaussard commented Jan 19, 2018

Uh oh!

dbkinder left a comment

Choose a reason for hiding this comment

Uh oh!

vaussard commented Jan 29, 2018

Uh oh!

ydamigos 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!

vaussard commented Jan 29, 2018

Uh oh!

ydamigos commented Jan 29, 2018

Uh oh!

vaussard commented Jan 29, 2018

Uh oh!

ydamigos commented Jan 29, 2018

Uh oh!

dbkinder commented Jan 29, 2018

Uh oh!

jli157 commented May 24, 2018

Uh oh!

vaussard commented May 25, 2018

Uh oh!

erwango commented May 25, 2018

Uh oh!

galak left a comment

Choose a reason for hiding this comment

Uh oh!

vaussard commented May 28, 2018

Uh oh!

jli157 commented May 29, 2018

Uh oh!

vaussard commented May 30, 2018

Uh oh!

jli157 commented Jun 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

vaussard commented Dec 22, 2017 •

edited by nashif

Loading

codecov-io commented Jan 15, 2018 •

edited

Loading

ydamigos left a comment •

edited

Loading