Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor STM32 PWM driver using LL API #24569

Merged
merged 2 commits into from
Jun 19, 2020
Merged

Refactor STM32 PWM driver using LL API #24569

merged 2 commits into from
Jun 19, 2020

Conversation

gmarull
Copy link
Member

@gmarull gmarull commented Apr 21, 2020

The PWM driver has been refactored using the HAL LL API. Not only that, but the set pin_set function is now faster, as channel output compare is just initialized if needed.

Support for the polarity flag has also been added to the STM32 PWM driver. STM32 boards using PWM have been updated accordingly.

Fixes #23629

@gmarull gmarull added area: PWM Pulse Width Modulation platform: STM32 ST Micro STM32 labels Apr 21, 2020
@gmarull gmarull requested review from erwango and FRASTM April 21, 2020 20:16
@gmarull gmarull requested a review from arnopo as a code owner April 21, 2020 20:16
@zephyrbot
Copy link
Collaborator

zephyrbot commented Apr 21, 2020

All checks are passing now.

checkpatch (informational only, not a failure)

-:564: WARNING:TRAILING_SEMICOLON: macros should not use a trailing semicolon
#564: FILE: drivers/pwm/pwm_stm32.c:314:
+#define PWM_DEVICE_INIT(index)                                                 \
+	static struct pwm_stm32_data pwm_stm32_data_##index;                   \
+									       \
+	static const struct pwm_stm32_config pwm_stm32_config_##index = {      \
+		.timer = (TIM_TypeDef *)DT_REG_ADDR(                           \
+			DT_INST(index, st_stm32_timers)),                      \
+		.prescaler = DT_INST_PROP(index, st_prescaler),                \
+		.pclken = DT_INST_CLK(index, timer)                            \
+	};                                                                     \
+									       \
+	DEVICE_AND_API_INIT(pwm_stm32_##index, DT_INST_LABEL(index),           \
+			    &pwm_stm32_init, &pwm_stm32_data_##index,          \
+			    &pwm_stm32_config_##index, POST_KERNEL,            \
+			    CONFIG_KERNEL_INIT_PRIORITY_DEVICE,                \
+			    &pwm_stm32_driver_api);

- total: 0 errors, 1 warnings, 1657 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.

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 SPDX_LICENSE_TAG SPLIT_STRING VOLATILE

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

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

drivers/pwm/Kconfig.stm32 Outdated Show resolved Hide resolved
Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is ambitious has it encompass 2 major changes: Boards Kconfig to dts transition and conversion to LL, plus and additional rather unrelated change (polarity support).
Please split these 3 changes to individual PRs.

Regarding the Kconfig to dts transition, I think the node labeling sound like the best option:

		timers1: timers@40012c00 {
			compatible = "st,stm32-timers";
			{...}                  
			pmw1: pwm@1 {
				compatible = "st,stm32-pwm";
				status = "disabled";
				st,prescaler = <10000>;
				label = "PWM_1";
				#pwm-cells = <2>;
			};

Besides, it would help getting rid of the paths in following expressions:

		green_pwm_led: green_pwm_led {
			pwms = <&{/soc/timers@40000000/pwm} 1 4>;

@gmarull gmarull requested a review from erwango May 5, 2020 20:32
@gmarull
Copy link
Member Author

gmarull commented May 5, 2020

@erwango I have rebased this PR now that PWM uses DT. I have splitted it in two commits, the first one is the refactor and the second the addition of the polarity flag support. I can split it into 2 PRs, not sure if needed, though.


pwm-cells:
- channel
# period in terms of nanoseconds
- period
- flags
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flags is either PWM_POLARITY_NORMAL or PWM_POLARITY_INVERTED , correct ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they're defined #include <dt-bindings/pwm/pwm.h>. I guess that flags could potentially be used for other purposes in the future.

@erwango erwango added this to the v2.4.0 milestone May 25, 2020
@gmarull
Copy link
Member Author

gmarull commented Jun 15, 2020

@erwango ping

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Txs for this welcome change.
Some comments, but otherwise it's fine to me.

boards/arm/nucleo_f401re/nucleo_f401re.dts Outdated Show resolved Hide resolved
drivers/pwm/pwm_stm32.c Outdated Show resolved Hide resolved
drivers/pwm/pwm_stm32.c Show resolved Hide resolved
The PWM drivers has been refactored using the HAL LL API. Not only that,
but the set pin_set function is now faster, as channel output compare is
just initialized if needed.

NOTE: Has been tested using H743zi board for now.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Add support for the polarity flag in the STM32 PWM driver.

STM32 boards using PWM have been updated accordingly.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
@gmarull gmarull requested a review from erwango June 18, 2020 11:14
@carlescufi carlescufi merged commit 76f0d72 into zephyrproject-rtos:master Jun 19, 2020
@gmarull gmarull deleted the feature/refactor-stm32-pwm branch June 19, 2020 13:26
@lochej
Copy link
Collaborator

lochej commented Jul 6, 2020

I'm sorry if this is the wrong section but that is my first time contributing to Zephyr.
The LL PWM implementation has a bug in the get_polarity() function always returns LL_TIM_OCPOLARITY_LOW which in turn actually inverts your PWM on STM32s :

static uint32_t get_polarity(pwm_flags_t flags)
{
	if (flags & PWM_POLARITY_NORMAL) { <-- PWM_POLARITY_NORMAL expands to 0<<0 in include/dt-bindings/pwm/pwm.h
		return LL_TIM_OCPOLARITY_HIGH;
	}

	return LL_TIM_OCPOLARITY_LOW;
}

The PWM_POLARITY_MASK set to 0<<0 forces the if statement to always be false.

My recommendation would either be to change the defines in dt-bindings/pwm/pwm.h to:

/** PWM pin normal polarity (active-high pulse). */
#define PWM_POLARITY_NORMAL	(1 << 0)
// -> High polarity should be 1

/** PWM pin inverted polarity (active-low pulse). */
#define PWM_POLARITY_INVERTED	(0 << 0)
//-> Low polarity should be 0

Doing this fix has proven correct behavior of PWM polarity on my Nucleo64 board.

@gmarull
Copy link
Member Author

gmarull commented Jul 6, 2020

@lochej thanks for reporting, fix here: #26680. You can open an issue if you want so it can be referenced on the fix PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support inverted PWM on STM32
7 participants