[v1] pwm: imx-tpm: keep channel state instead of counting

[PATCH] pwm: imx-tpm: keep channel state instead of counting

Posted by Viorel Suman (OSS) 1 week, 1 day ago

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

Re: [PATCH] pwm: imx-tpm: keep channel state instead of counting

Posted by Uwe Kleine-König 1 week, 1 day ago

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

Re: [PATCH] pwm: imx-tpm: keep channel state instead of counting

Posted by Viorel Suman (OSS) 5 days, 17 hours ago

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

Re: [PATCH] pwm: imx-tpm: keep channel state instead of counting

Posted by Frank Li 1 week, 1 day ago

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
>

Re: [PATCH] pwm: imx-tpm: keep channel state instead of counting

Posted by Uwe Kleine-König 1 week, 1 day ago

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