* [PATCH] pwm: imx-tpm: keep channel state instead of counting
@ 2026-01-30 14:37 Viorel Suman (OSS)
2026-01-30 17:41 ` Frank Li
2026-01-30 23:00 ` Uwe Kleine-König
0 siblings, 2 replies; 5+ messages in thread
From: Viorel Suman (OSS) @ 2026-01-30 14:37 UTC (permalink / raw)
To: Uwe Kleine-König, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, linux-pwm, imx,
linux-arm-kernel, linux-kernel
Cc: Viorel Suman
On a soft reset TPM PWM IP may preserve its internal state from
previous runtime, therefore on a subsequent OS boot and driver
probe "enable_count" value and TPM PWM IP internal channels
"enabled" states may get unaligned. In consequence on a suspend/resume
cycle the call "if (--tpm->enable_count == 0)" may lead to
"enable_count" overflow the system being blocked from entering
suspend due to:
if (tpm->enable_count > 0)
return -EBUSY;
Fix this problem by replacing counting logic with per-channel state handling.
Signed-off-by: Viorel Suman (OSS) <viorel.suman@oss.nxp.com>
---
drivers/pwm/pwm-imx-tpm.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
index 5b399de16d60..0f8643f4a70b 100644
--- a/drivers/pwm/pwm-imx-tpm.c
+++ b/drivers/pwm/pwm-imx-tpm.c
@@ -62,7 +62,7 @@ struct imx_tpm_pwm_chip {
void __iomem *base;
struct mutex lock;
u32 user_count;
- u32 enable_count;
+ u32 enabled_channels;
u32 real_period;
};
@@ -166,6 +166,10 @@ static int pwm_imx_tpm_get_state(struct pwm_chip *chip,
/* get channel status */
state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true : false;
+ if (state->enabled)
+ tpm->enabled_channels |= BIT(pwm->hwpwm);
+ else
+ tpm->enabled_channels &= ~BIT(pwm->hwpwm);
return 0;
}
@@ -282,15 +286,19 @@ static int pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
}
writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
- /* control the counter status */
+ /* control the channel state */
if (state->enabled != c.enabled) {
val = readl(tpm->base + PWM_IMX_TPM_SC);
if (state->enabled) {
- if (++tpm->enable_count == 1)
+ if (tpm->enabled_channels == 0) {
val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
+ }
+ tpm->enabled_channels |= BIT(pwm->hwpwm);
} else {
- if (--tpm->enable_count == 0)
+ tpm->enabled_channels &= ~BIT(pwm->hwpwm);
+ if (tpm->enabled_channels == 0) {
val &= ~PWM_IMX_TPM_SC_CMOD;
+ }
}
writel(val, tpm->base + PWM_IMX_TPM_SC);
}
@@ -394,7 +402,7 @@ static int pwm_imx_tpm_suspend(struct device *dev)
struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
int ret;
- if (tpm->enable_count > 0)
+ if (tpm->enabled_channels > 0)
return -EBUSY;
/*
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] pwm: imx-tpm: keep channel state instead of counting
2026-01-30 14:37 [PATCH] pwm: imx-tpm: keep channel state instead of counting Viorel Suman (OSS)
@ 2026-01-30 17:41 ` Frank Li
2026-01-30 22:54 ` Uwe Kleine-König
2026-01-30 23:00 ` Uwe Kleine-König
1 sibling, 1 reply; 5+ messages in thread
From: Frank Li @ 2026-01-30 17:41 UTC (permalink / raw)
To: Viorel Suman (OSS)
Cc: Uwe Kleine-König, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, linux-pwm, imx, linux-arm-kernel, linux-kernel
On Fri, Jan 30, 2026 at 04:37:20PM +0200, Viorel Suman (OSS) wrote:
> On a soft reset TPM PWM IP may preserve its internal state from
> previous runtime, therefore on a subsequent OS boot and driver
> probe "enable_count" value and TPM PWM IP internal channels
> "enabled" states may get unaligned. In consequence on a suspend/resume
> cycle the call "if (--tpm->enable_count == 0)" may lead to
> "enable_count" overflow the system being blocked from entering
> suspend due to:
>
> if (tpm->enable_count > 0)
> return -EBUSY;
>
> Fix this problem by replacing counting logic with per-channel state handling.
>
> Signed-off-by: Viorel Suman (OSS) <viorel.suman@oss.nxp.com>
> ---
> drivers/pwm/pwm-imx-tpm.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> index 5b399de16d60..0f8643f4a70b 100644
> --- a/drivers/pwm/pwm-imx-tpm.c
> +++ b/drivers/pwm/pwm-imx-tpm.c
> @@ -62,7 +62,7 @@ struct imx_tpm_pwm_chip {
> void __iomem *base;
> struct mutex lock;
> u32 user_count;
> - u32 enable_count;
> + u32 enabled_channels;
> u32 real_period;
> };
>
> @@ -166,6 +166,10 @@ static int pwm_imx_tpm_get_state(struct pwm_chip *chip,
>
> /* get channel status */
> state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true : false;
> + if (state->enabled)
> + tpm->enabled_channels |= BIT(pwm->hwpwm);
> + else
> + tpm->enabled_channels &= ~BIT(pwm->hwpwm);
Do you have lock for RMW? or you should atomic_or() and atomic_and()
Frank
>
> return 0;
> }
> @@ -282,15 +286,19 @@ static int pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
> }
> writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
>
> - /* control the counter status */
> + /* control the channel state */
> if (state->enabled != c.enabled) {
> val = readl(tpm->base + PWM_IMX_TPM_SC);
> if (state->enabled) {
> - if (++tpm->enable_count == 1)
> + if (tpm->enabled_channels == 0) {
> val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> + }
> + tpm->enabled_channels |= BIT(pwm->hwpwm);
> } else {
> - if (--tpm->enable_count == 0)
> + tpm->enabled_channels &= ~BIT(pwm->hwpwm);
> + if (tpm->enabled_channels == 0) {
> val &= ~PWM_IMX_TPM_SC_CMOD;
> + }
> }
> writel(val, tpm->base + PWM_IMX_TPM_SC);
> }
> @@ -394,7 +402,7 @@ static int pwm_imx_tpm_suspend(struct device *dev)
> struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
> int ret;
>
> - if (tpm->enable_count > 0)
> + if (tpm->enabled_channels > 0)
> return -EBUSY;
>
> /*
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pwm: imx-tpm: keep channel state instead of counting
2026-01-30 17:41 ` Frank Li
@ 2026-01-30 22:54 ` Uwe Kleine-König
0 siblings, 0 replies; 5+ messages in thread
From: Uwe Kleine-König @ 2026-01-30 22:54 UTC (permalink / raw)
To: Frank Li
Cc: Viorel Suman (OSS), Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, linux-pwm, imx, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 642 bytes --]
On Fri, Jan 30, 2026 at 12:41:22PM -0500, Frank Li wrote:
> On Fri, Jan 30, 2026 at 04:37:20PM +0200, Viorel Suman (OSS) wrote:
> > @@ -166,6 +166,10 @@ static int pwm_imx_tpm_get_state(struct pwm_chip *chip,
> >
> > /* get channel status */
> > state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true : false;
> > + if (state->enabled)
> > + tpm->enabled_channels |= BIT(pwm->hwpwm);
> > + else
> > + tpm->enabled_channels &= ~BIT(pwm->hwpwm);
>
> Do you have lock for RMW? or you should atomic_or() and atomic_and()
.get_state() is called under the chip lock, so the locking should be
fine.
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pwm: imx-tpm: keep channel state instead of counting
2026-01-30 14:37 [PATCH] pwm: imx-tpm: keep channel state instead of counting Viorel Suman (OSS)
2026-01-30 17:41 ` Frank Li
@ 2026-01-30 23:00 ` Uwe Kleine-König
2026-02-02 10:46 ` Viorel Suman (OSS)
1 sibling, 1 reply; 5+ messages in thread
From: Uwe Kleine-König @ 2026-01-30 23:00 UTC (permalink / raw)
To: Viorel Suman (OSS)
Cc: Frank Li, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
linux-pwm, imx, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2053 bytes --]
Hello,
On Fri, Jan 30, 2026 at 04:37:20PM +0200, Viorel Suman (OSS) wrote:
> diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> index 5b399de16d60..0f8643f4a70b 100644
> --- a/drivers/pwm/pwm-imx-tpm.c
> +++ b/drivers/pwm/pwm-imx-tpm.c
> @@ -62,7 +62,7 @@ struct imx_tpm_pwm_chip {
> void __iomem *base;
> struct mutex lock;
> u32 user_count;
> - u32 enable_count;
> + u32 enabled_channels;
> u32 real_period;
> };
>
> @@ -166,6 +166,10 @@ static int pwm_imx_tpm_get_state(struct pwm_chip *chip,
>
> /* get channel status */
> state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true : false;
> + if (state->enabled)
> + tpm->enabled_channels |= BIT(pwm->hwpwm);
> + else
> + tpm->enabled_channels &= ~BIT(pwm->hwpwm);
I'm not sure about this being the right approach, feels like the driver
sells the pwm core down the river resulting in something the might work
now but isn't robust.
If I understand it right, keeping the enable count balanced depends on
.get_state() being called.
The usual and robust approach is that .probe() checks the device state
and initializes enable counts and the like accordingly.
> return 0;
> }
> @@ -282,15 +286,19 @@ static int pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
> }
> writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
>
> - /* control the counter status */
> + /* control the channel state */
> if (state->enabled != c.enabled) {
> val = readl(tpm->base + PWM_IMX_TPM_SC);
> if (state->enabled) {
> - if (++tpm->enable_count == 1)
> + if (tpm->enabled_channels == 0) {
> val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> + }
No { } for one line blocks please.
> + tpm->enabled_channels |= BIT(pwm->hwpwm);
> } else {
> - if (--tpm->enable_count == 0)
> + tpm->enabled_channels &= ~BIT(pwm->hwpwm);
> + if (tpm->enabled_channels == 0) {
> val &= ~PWM_IMX_TPM_SC_CMOD;
> + }
> }
> writel(val, tpm->base + PWM_IMX_TPM_SC);
> }
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pwm: imx-tpm: keep channel state instead of counting
2026-01-30 23:00 ` Uwe Kleine-König
@ 2026-02-02 10:46 ` Viorel Suman (OSS)
0 siblings, 0 replies; 5+ messages in thread
From: Viorel Suman (OSS) @ 2026-02-02 10:46 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Frank Li, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
linux-pwm, imx, linux-arm-kernel, linux-kernel
Hello,
On 26-01-31 00:00:02, Uwe Kleine-König wrote:
> Hello,
>
> On Fri, Jan 30, 2026 at 04:37:20PM +0200, Viorel Suman (OSS) wrote:
> > diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> > index 5b399de16d60..0f8643f4a70b 100644
> > --- a/drivers/pwm/pwm-imx-tpm.c
> > +++ b/drivers/pwm/pwm-imx-tpm.c
> > @@ -62,7 +62,7 @@ struct imx_tpm_pwm_chip {
> > void __iomem *base;
> > struct mutex lock;
> > u32 user_count;
> > - u32 enable_count;
> > + u32 enabled_channels;
> > u32 real_period;
> > };
> >
> > @@ -166,6 +166,10 @@ static int pwm_imx_tpm_get_state(struct pwm_chip *chip,
> >
> > /* get channel status */
> > state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true : false;
> > + if (state->enabled)
> > + tpm->enabled_channels |= BIT(pwm->hwpwm);
> > + else
> > + tpm->enabled_channels &= ~BIT(pwm->hwpwm);
>
> I'm not sure about this being the right approach, feels like the driver
> sells the pwm core down the river resulting in something the might work
> now but isn't robust.
>
> If I understand it right, keeping the enable count balanced depends on
> .get_state() being called.
>
> The usual and robust approach is that .probe() checks the device state
> and initializes enable counts and the like accordingly.
Thank you for review, I've moved in V2 the device state
check into the probe function as suggested.
>
> > return 0;
> > }
> > @@ -282,15 +286,19 @@ static int pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
> > }
> > writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> >
> > - /* control the counter status */
> > + /* control the channel state */
> > if (state->enabled != c.enabled) {
> > val = readl(tpm->base + PWM_IMX_TPM_SC);
> > if (state->enabled) {
> > - if (++tpm->enable_count == 1)
> > + if (tpm->enabled_channels == 0) {
> > val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> > + }
>
> No { } for one line blocks please.
Fixed in V2.
>
> > + tpm->enabled_channels |= BIT(pwm->hwpwm);
> > } else {
> > - if (--tpm->enable_count == 0)
> > + tpm->enabled_channels &= ~BIT(pwm->hwpwm);
> > + if (tpm->enabled_channels == 0) {
> > val &= ~PWM_IMX_TPM_SC_CMOD;
> > + }
> > }
> > writel(val, tpm->base + PWM_IMX_TPM_SC);
> > }
>
> Best regards
> Uwe
Best regards,
Vorel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-02-02 10:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-01-30 14:37 [PATCH] pwm: imx-tpm: keep channel state instead of counting Viorel Suman (OSS)
2026-01-30 17:41 ` Frank Li
2026-01-30 22:54 ` Uwe Kleine-König
2026-01-30 23:00 ` Uwe Kleine-König
2026-02-02 10:46 ` Viorel Suman (OSS)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox