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.

Sign up for GitHub

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

Jump to bottom

drivers: pwm: nrfx: add global hsfll request for fast PWM #82133

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

drivers: pwm: nrfx: add global hsfll request for fast PWM #82133

wants to merge 1 commit into from

Conversation

mstasiaknordic
Copy link
Contributor

@mstasiaknordic mstasiaknordic commented Nov 27, 2024 •
edited
Loading

Added clock control api for global hsfll used in fast PWM120 driver.

@mstasiaknordic mstasiaknordic force-pushed the pwm_global_hsfll branch 3 times, most recently from e94eb66 to 1e59406 Compare December 4, 2024 08:08
@mstasiaknordic mstasiaknordic marked this pull request as ready for review December 11, 2024 09:56
@zephyrbot zephyrbot added platform: nRF Nordic nRFx area: PWM Pulse Width Modulation labels Dec 11, 2024
@mstasiaknordic
Copy link
Contributor Author

@bjarki-andreasen You can have a look

bjarki-andreasen
bjarki-andreasen previously approved these changes Dec 11, 2024
View reviewed changes
Copy link
Collaborator

@bjarki-andreasen bjarki-andreasen left a comment

Choose a reason for hiding this comment

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

Looks good

@mstasiaknordic mstasiaknordic force-pushed the pwm_global_hsfll branch 3 times, most recently from 98008da to 89e31f4 Compare December 12, 2024 11:06
masz-nordic
masz-nordic reviewed Dec 16, 2024
View reviewed changes
drivers/pwm/Kconfig.nrfx Outdated
@@ -20,3 +20,8 @@ config PWM_NRFX
select PINCTRL
help
Enable support for nrfx Hardware PWM driver for nRF52 MCU series.

config PWM_NRFX_INIT_PRIORITY
int
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
int
int "NRFX PWM init priority"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want it to be user-assignable? In my opinion, CONFIG_PWM_INIT_PRIORITY can still be set and used if the PWM instance is not 120, in which case priority needs to be 99 to match clock device

Copy link
Contributor

Choose a reason for hiding this comment

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

You assume here that clock control has 99 priority but this may change.
I used approach where in the code I'm trying to pick CONFIG_CLOCK_CONTROL_NRF2_GLOBAL_HSFLL_INIT_PRIORITY+1 for fast instance (similar approach is in spim, too):

zephyr/drivers/serial/uart_nrfx_uarte.c

Line 2437 in f8f9ed7

(UTIL_INC(CONFIG_CLOCK_CONTROL_NRF2_GLOBAL_HSFLL_INIT_PRIORITY)), \

@bjarki-andreasen bjarki-andreasen removed the DNM This PR should not be merged (Do Not Merge) label Dec 17, 2024
@decsny decsny removed their request for review December 17, 2024 17:33
nordic-krch
nordic-krch reviewed Jan 2, 2025
View reviewed changes
Copy link
Contributor

@nordic-krch nordic-krch left a comment

Choose a reason for hiding this comment

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

You can also add
select CLOCK_CONTROL if HAS_HW_NRF_PWM120 to PWM_NRFX Kconfig.

drivers/pwm/Kconfig.nrfx Outdated
@@ -20,3 +20,8 @@ config PWM_NRFX
select PINCTRL
help
Enable support for nrfx Hardware PWM driver for nRF52 MCU series.

config PWM_NRFX_INIT_PRIORITY
int
Copy link
Contributor

Choose a reason for hiding this comment

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

You assume here that clock control has 99 priority but this may change.
I used approach where in the code I'm trying to pick CONFIG_CLOCK_CONTROL_NRF2_GLOBAL_HSFLL_INIT_PRIORITY+1 for fast instance (similar approach is in spim, too):

zephyr/drivers/serial/uart_nrfx_uarte.c

Line 2437 in f8f9ed7

(UTIL_INC(CONFIG_CLOCK_CONTROL_NRF2_GLOBAL_HSFLL_INIT_PRIORITY)), \

@mstasiaknordic
Copy link
Contributor Author

select CLOCK_CONTROL if HAS_HW_NRF_PWM120

This would work best, however, it results in dependency loop

@mstasiaknordic mstasiaknordic force-pushed the pwm_global_hsfll branch 2 times, most recently from f2732b0 to e8a4f1c Compare January 3, 2025 12:08
nika-nordic
nika-nordic previously approved these changes Jan 7, 2025
View reviewed changes
@mstasiaknordic mstasiaknordic mentioned this pull request Jan 7, 2025
Open
nika-nordic
nika-nordic previously approved these changes Jan 8, 2025
View reviewed changes
anangl
anangl reviewed Jan 8, 2025
View reviewed changes
drivers/pwm/pwm_nrfx.c Outdated
Comment on lines 16 to 18
#ifdef CONFIG_CLOCK_CONTROL
#include <zephyr/drivers/clock_control/nrf_clock_control.h>
#endif
Copy link
Member

Choose a reason for hiding this comment

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

This #ifdef here is superfluous.

drivers/pwm/pwm_nrfx.c Outdated
(IS_EQ(DT_PHA(PWM(idx), power_domains, id), NRF_GPD_FAST_ACTIVE1)), \
(0))), (0))

#if (NRFX_FOREACH_PRESENT(PWM, PWM_NRFX_IS_FAST, (||), (0))) && CONFIG_CLOCK_CONTROL
Copy link
Member

@anangl anangl Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'd rather use CONFIG_CLOCK_CONTROL_NRF2_GLOBAL_HSFLL, as this is what is actually used by this driver.

drivers/pwm/pwm_nrfx.c Outdated
Comment on lines 324 to 396
pwm_resume(dev);
ret = pwm_resume(dev);
} else if (IS_ENABLED(CONFIG_PM_DEVICE) && (action == PM_DEVICE_ACTION_SUSPEND)) {
pwm_suspend(dev);
ret = pwm_suspend(dev);
Copy link
Member

Choose a reason for hiding this comment

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

Why not just add return in these two lines (and remove that unneeded then return 0 at the of the function) instead of introducing the ret variable?

drivers/pwm/pwm_nrfx.c Outdated
Comment on lines 473 to 475
.accuracy = 0, \
.precision = \
NRF_CLOCK_CONTROL_PRECISION_DEFAULT, \
Copy link
Member

Choose a reason for hiding this comment

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

This is unneeded. Only the specified frequency is requested anyway.

drivers/pwm/pwm_nrfx.c
@@ -248,6 +284,20 @@ static int pwm_nrfx_set_cycles(const struct device *dev, uint32_t channel,
* until another playback is requested (new values will be
* loaded then) or the PWM peripheral is stopped.
*/
#if PWM_NRFX_USE_CLOCK_CONTROL
if (!data->clock_requested) {
int ret = nrf_clock_control_request_sync(config->clk_dev,
Copy link
Member

Choose a reason for hiding this comment

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

This is only needed for the fast instances. You could have clk_dev set to NULL for others and avoid this call.

@mstasiaknordic mstasiaknordic requested a review from anangl January 8, 2025 11:19
@mstasiaknordic
drivers: pwm: nrfx: add global hsfll request for fast PWM
6f113f8 
Added clock control api for global hsfll used in fast PWM120 driver.

Signed-off-by: Michał Stasiak <[email protected]>
@mstasiaknordic mstasiaknordic mentioned this pull request Jan 8, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjarki-andreasen bjarki-andreasen bjarki-andreasen left review comments

@nika-nordic nika-nordic nika-nordic left review comments

@henrikbrixandersen henrikbrixandersen Awaiting requested review from henrikbrixandersen

@jaz1-nordic jaz1-nordic Awaiting requested review from jaz1-nordic

@kl-cruz kl-cruz Awaiting requested review from kl-cruz

@magp-nordic magp-nordic Awaiting requested review from magp-nordic

@andyross andyross Awaiting requested review from andyross

@dcpleung dcpleung Awaiting requested review from dcpleung

@galak galak Awaiting requested review from galak

@nashif nashif Awaiting requested review from nashif

@peter-mitsis peter-mitsis Awaiting requested review from peter-mitsis

@nordic-krch nordic-krch Awaiting requested review from nordic-krch

@masz-nordic masz-nordic Awaiting requested review from masz-nordic

@anangl anangl Awaiting requested review from anangl

At least 2 approving reviews are required to merge this pull request.

Assignees

@anangl anangl

@masz-nordic masz-nordic

Labels
area: Base OS Base OS Library (lib/os) area: Clock Control area: PWM Pulse Width Modulation platform: nRF Nordic nRFx
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mstasiaknordic @bjarki-andreasen @nordic-krch @anangl @nika-nordic @masz-nordic @zephyrbot @decsny