Skip to content

PWM with power management fix #25

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

Merged
Aksel Mellbye (asmellby) merged 2 commits into from
Jul 7, 2026
Merged

PWM with power management fix #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3b498e3
drivers: clock_control: siwx91x: fix PWM clock rate retrieval
Martinhoff-maker Jul 7, 2026
772f6db
drivers: pwm: siwx91x: add PM runtime management for PWM
Martinhoff-maker Jul 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions drivers/clock_control/clock_control_silabs_siwx91x.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,13 @@ static int siwx91x_clock_get_rate(const struct device *dev, clock_control_subsys
*rate = RSI_CLK_GetBaseClock(M4_UART1);
return 0;
case SIWX91X_CLK_PWM:
/* PWM peripheral operates at the system clock frequency */
*rate = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC;
/*
* PWM peripheral runs from the M4 core clock domain.
* Do not use CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC here because it may be
* 32.768 kHz when the sleeptimer system timer is selected, which breaks
* pwm_set() nsec-to-cycles conversion.
*/
*rate = SystemCoreClock;

@asmellby Aksel Mellbye (asmellby) Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Use devicetree for this: DT_PROP(DT_NODELABEL(cpu0), clock_frequency)

@Martinhoff-maker Martin Hoff (Martinhoff-maker) Jul 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

So we have a debate about that with Jérôme Pouiller (@jerome-pouiller), SystemCoreClock is actually the real Core clock value updated dynamically in Wiseconnect. But fair point, will use DT_PROP(DT_NODELABEL(cpu0), clock_frequency) for the moment to have consistency.

@asmellby Aksel Mellbye (asmellby) Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I thought this was in the PWM driver when I first reviewed, in which case it would be a violation of responsibility between drivers (PWM should use clock control only). Since this is in the clock driver, it's more OK since the clock driver is responsible for clocks, and should therefore be allowed to use that global as part of its HAL usage.

@Martinhoff-maker Martin Hoff (Martinhoff-maker) Jul 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok then let's still use SystemCoreClock for the moment (since this is with what i've tested).

return 0;
case SIWX91X_CLK_WATCHDOG:
*rate = LF_FSM_CLOCK_FREQUENCY;
Expand Down
25 changes: 20 additions & 5 deletions drivers/pwm/pwm_silabs_siwx91x.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <zephyr/drivers/pinctrl.h>
#include <zephyr/drivers/clock_control.h>
#include <zephyr/pm/device.h>
#include <zephyr/pm/device_runtime.h>
#include <zephyr/types.h>
#include "sl_si91x_pwm.h"

Expand Down Expand Up @@ -121,24 +122,28 @@ static int pwm_siwx91x_set_cycles(const struct device *dev, uint32_t channel,
return -ENOTSUP;
}

if (pulse_cycles > 0 && data->pwm_channel_cfg[channel].is_chan_active == false) {
pm_device_runtime_get(dev);
}

if (data->pwm_channel_cfg[channel].is_chan_active == false) {
/* Configure the channel with default parameters */
ret = siwx91x_default_channel_config(dev, channel);
if (ret) {
return -EINVAL;
goto out;
}
}

ret = sl_si91x_pwm_get_time_period(channel, (uint16_t *)&prev_period);
if (ret) {
return -EINVAL;
goto out;
}

if (period_cycles != prev_period) {
ret = sl_si91x_pwm_set_time_period(channel, period_cycles, 0);
if (ret) {
/* Programmed value must be out of range (>65535) */
return -EINVAL;
goto out;
}
}

Expand All @@ -149,7 +154,7 @@ static int pwm_siwx91x_set_cycles(const struct device *dev, uint32_t channel,
if (duty_cycle != data->pwm_channel_cfg[channel].duty_cycle) {
ret = sl_si91x_pwm_set_duty_cycle(pulse_cycles, channel);
if (ret) {
return -EINVAL;
goto out;
}
data->pwm_channel_cfg[channel].duty_cycle = duty_cycle;
}
Expand All @@ -158,12 +163,22 @@ static int pwm_siwx91x_set_cycles(const struct device *dev, uint32_t channel,
/* Start PWM after configuring the channel for first time */
ret = sl_si91x_pwm_start(channel);
if (ret) {
return -EINVAL;
goto out;
}
data->pwm_channel_cfg[channel].is_chan_active = true;
}

if (pulse_cycles == 0 && data->pwm_channel_cfg[channel].is_chan_active == true) {

@jhedberg Johan Hedberg (jhedberg) Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor: is_chan_active is an essentially boolean type (bool) so the explicit comparison with true is redundant.

@Martinhoff-maker Martin Hoff (Martinhoff-maker) Jul 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agree, I've to admit that I've reapplied the existing pattern in the function but that's a fair point. Will do that in another PR if that's ok for you.

@jhedberg Johan Hedberg (jhedberg) Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sure

pm_device_runtime_put(dev);
data->pwm_channel_cfg[channel].is_chan_active = false;
}

return 0;

out:
pm_device_runtime_put(dev);
data->pwm_channel_cfg[channel].is_chan_active = false;
return -EINVAL;
}

static int pwm_siwx91x_get_cycles_per_sec(const struct device *dev, uint32_t channel,
Expand Down