[v1] pwm: imx-tpm: reset counter if CMOD is 0

[PATCH] pwm: imx-tpm: reset counter if CMOD is 0

Posted by Laurentiu Mihalcea 3 months, 1 week ago

From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

As per the i.MX93 TRM, section 67.3.2.1 "MOD register update", the value
of the TPM counter does NOT get updated when writing MOD.MOD unless
SC.CMOD != 0. Therefore, with the current code, assuming the following
sequence:

	1) pwm_disable()
	2) pwm_apply_might_sleep() /* period is changed here */
	3) pwm_enable()

and assuming only one channel is active, if CNT.COUNT is higher than the
MOD.MOD value written during the pwm_apply_might_sleep() call then, when
re-enabling the PWM during pwm_enable(), the counter will end up resetting
after UINT32_MAX - CNT.COUNT + MOD.MOD cycles instead of MOD.MOD cycles as
normally expected.

Fix this problem by forcing a reset of the TPM counter before MOD.MOD is
written.

Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
---
 drivers/pwm/pwm-imx-tpm.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
index 7ee7b65b9b90..30f271826aed 100644
--- a/drivers/pwm/pwm-imx-tpm.c
+++ b/drivers/pwm/pwm-imx-tpm.c
@@ -204,6 +204,19 @@ static int pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
 		val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, p->prescale);
 		writel(val, tpm->base + PWM_IMX_TPM_SC);
 
+		/*
+		 * VERY IMPORTANT: if CMOD is set to 0 then writing
+		 * MOD will NOT reset the value of the TPM counter.
+		 *
+		 * Therefore, if CNT.COUNT > MOD.MOD, the counter will reset
+		 * after UINT32_MAX - CNT.COUNT + MOD.MOD cycles, which is
+		 * incorrect.
+		 *
+		 * To avoid this, we need to force a reset of the
+		 * counter before writing the new MOD value.
+		 */
+		if (!cmod)
+			writel(0x0, tpm->base + PWM_IMX_TPM_CNT);
 		/*
 		 * set period count:
 		 * if the PWM is disabled (CMOD[1:0] = 2b00), then MOD register
-- 
2.34.1

Re: [PATCH] pwm: imx-tpm: reset counter if CMOD is 0

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

Hello,

On Tue, Jul 01, 2025 at 06:01:47PM -0400, Laurentiu Mihalcea wrote:
> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> 
> As per the i.MX93 TRM, section 67.3.2.1 "MOD register update", the value
> of the TPM counter does NOT get updated when writing MOD.MOD unless
> SC.CMOD != 0. Therefore, with the current code, assuming the following
> sequence:
> 
> 	1) pwm_disable()
> 	2) pwm_apply_might_sleep() /* period is changed here */
> 	3) pwm_enable()
> 
> and assuming only one channel is active, if CNT.COUNT is higher than the
> MOD.MOD value written during the pwm_apply_might_sleep() call then, when
> re-enabling the PWM during pwm_enable(), the counter will end up resetting
> after UINT32_MAX - CNT.COUNT + MOD.MOD cycles instead of MOD.MOD cycles as
> normally expected.
> 
> Fix this problem by forcing a reset of the TPM counter before MOD.MOD is
> written.
> 
> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> ---
>  drivers/pwm/pwm-imx-tpm.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> index 7ee7b65b9b90..30f271826aed 100644
> --- a/drivers/pwm/pwm-imx-tpm.c
> +++ b/drivers/pwm/pwm-imx-tpm.c
> @@ -204,6 +204,19 @@ static int pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
>  		val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, p->prescale);
>  		writel(val, tpm->base + PWM_IMX_TPM_SC);
>  
> +		/*
> +		 * VERY IMPORTANT: if CMOD is set to 0 then writing

The "VERY IMPORTANT" is correct today as this is missing and so
disturbing operation. However once this patch is applied, it's only
normal to have it. So I suggest to drop this.

> +		 * MOD will NOT reset the value of the TPM counter.
> +		 *
> +		 * Therefore, if CNT.COUNT > MOD.MOD, the counter will reset
> +		 * after UINT32_MAX - CNT.COUNT + MOD.MOD cycles, which is
> +		 * incorrect.
> +		 *
> +		 * To avoid this, we need to force a reset of the
> +		 * counter before writing the new MOD value.
> +		 */

Without the reference manual at hand or a deeper understanding of the
hardware this isn't understandable. What is MOD? What is CMOD?

> +		if (!cmod)
> +			writel(0x0, tpm->base + PWM_IMX_TPM_CNT);
>  		/*
>  		 * set period count:
>  		 * if the PWM is disabled (CMOD[1:0] = 2b00), then MOD register

Best regards
Uwe

Re: [PATCH] pwm: imx-tpm: reset counter if CMOD is 0

Posted by Laurentiu Mihalcea 3 months, 1 week ago


On 7/2/2025 8:51 AM, Uwe Kleine-König wrote:
> Hello,
>
> On Tue, Jul 01, 2025 at 06:01:47PM -0400, Laurentiu Mihalcea wrote:
>> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
>>
>> As per the i.MX93 TRM, section 67.3.2.1 "MOD register update", the value
>> of the TPM counter does NOT get updated when writing MOD.MOD unless
>> SC.CMOD != 0. Therefore, with the current code, assuming the following
>> sequence:
>>
>> 	1) pwm_disable()
>> 	2) pwm_apply_might_sleep() /* period is changed here */
>> 	3) pwm_enable()
>>
>> and assuming only one channel is active, if CNT.COUNT is higher than the
>> MOD.MOD value written during the pwm_apply_might_sleep() call then, when
>> re-enabling the PWM during pwm_enable(), the counter will end up resetting
>> after UINT32_MAX - CNT.COUNT + MOD.MOD cycles instead of MOD.MOD cycles as
>> normally expected.
>>
>> Fix this problem by forcing a reset of the TPM counter before MOD.MOD is
>> written.
>>
>> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
>> ---
>>  drivers/pwm/pwm-imx-tpm.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
>> index 7ee7b65b9b90..30f271826aed 100644
>> --- a/drivers/pwm/pwm-imx-tpm.c
>> +++ b/drivers/pwm/pwm-imx-tpm.c
>> @@ -204,6 +204,19 @@ static int pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
>>  		val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, p->prescale);
>>  		writel(val, tpm->base + PWM_IMX_TPM_SC);
>>  
>> +		/*
>> +		 * VERY IMPORTANT: if CMOD is set to 0 then writing
> The "VERY IMPORTANT" is correct today as this is missing and so
> disturbing operation. However once this patch is applied, it's only
> normal to have it. So I suggest to drop this.

ACK

>
>> +		 * MOD will NOT reset the value of the TPM counter.
>> +		 *
>> +		 * Therefore, if CNT.COUNT > MOD.MOD, the counter will reset
>> +		 * after UINT32_MAX - CNT.COUNT + MOD.MOD cycles, which is
>> +		 * incorrect.
>> +		 *
>> +		 * To avoid this, we need to force a reset of the
>> +		 * counter before writing the new MOD value.
>> +		 */
> Without the reference manual at hand or a deeper understanding of the
> hardware this isn't understandable. What is MOD? What is CMOD?

so, MOD is the reference value for the counter. The counter needs to count until this
value is reached, at which point the counter value gets reset to 0 and the output signal
is driven HIGH or LOW (depends on the configured polarity). This value is used to define
the period of the PWM.


CMOD, on the other hand, is a clocking-related configuration option. I'd say what we're most
interested in here is the fact that if CMOD is 0 then the counter will be disabled. Otherwise,
it will be enabled.

>
>> +		if (!cmod)
>> +			writel(0x0, tpm->base + PWM_IMX_TPM_CNT);
>>  		/*
>>  		 * set period count:
>>  		 * if the PWM is disabled (CMOD[1:0] = 2b00), then MOD register
> Best regards
> Uwe

Re: [PATCH] pwm: imx-tpm: reset counter if CMOD is 0

Posted by Uwe Kleine-König 3 months ago

Hello Laurentiu,

On Wed, Jul 02, 2025 at 11:31:28AM +0300, Laurentiu Mihalcea wrote:
> On 7/2/2025 8:51 AM, Uwe Kleine-König wrote:
> > On Tue, Jul 01, 2025 at 06:01:47PM -0400, Laurentiu Mihalcea wrote:
> >> +		 * MOD will NOT reset the value of the TPM counter.
> >> +		 *
> >> +		 * Therefore, if CNT.COUNT > MOD.MOD, the counter will reset
> >> +		 * after UINT32_MAX - CNT.COUNT + MOD.MOD cycles, which is
> >> +		 * incorrect.
> >> +		 *
> >> +		 * To avoid this, we need to force a reset of the
> >> +		 * counter before writing the new MOD value.
> >> +		 */
> > Without the reference manual at hand or a deeper understanding of the
> > hardware this isn't understandable. What is MOD? What is CMOD?
> 
> so, MOD is the reference value for the counter. The counter needs to
> count until this value is reached, at which point the counter value
> gets reset to 0 and the output signal is driven HIGH or LOW (depends
> on the configured polarity). This value is used to define the period
> of the PWM.
> 
> CMOD, on the other hand, is a clocking-related configuration option.
> I'd say what we're most interested in here is the fact that if CMOD is
> 0 then the counter will be disabled. Otherwise, it will be enabled.

JFTR: I marked your patch as "changes requested" now in patchwork and my
inbox and expect an updated patch, but without holding my breath :-)

Best regards
Uwe

Re: [PATCH] pwm: imx-tpm: reset counter if CMOD is 0

Posted by Laurentiu Mihalcea 3 months ago


On 7/9/2025 9:05 AM, Uwe Kleine-König wrote:
> Hello Laurentiu,
>
> On Wed, Jul 02, 2025 at 11:31:28AM +0300, Laurentiu Mihalcea wrote:
>> On 7/2/2025 8:51 AM, Uwe Kleine-König wrote:
>>> On Tue, Jul 01, 2025 at 06:01:47PM -0400, Laurentiu Mihalcea wrote:
>>>> +		 * MOD will NOT reset the value of the TPM counter.
>>>> +		 *
>>>> +		 * Therefore, if CNT.COUNT > MOD.MOD, the counter will reset
>>>> +		 * after UINT32_MAX - CNT.COUNT + MOD.MOD cycles, which is
>>>> +		 * incorrect.
>>>> +		 *
>>>> +		 * To avoid this, we need to force a reset of the
>>>> +		 * counter before writing the new MOD value.
>>>> +		 */
>>> Without the reference manual at hand or a deeper understanding of the
>>> hardware this isn't understandable. What is MOD? What is CMOD?
>> so, MOD is the reference value for the counter. The counter needs to
>> count until this value is reached, at which point the counter value
>> gets reset to 0 and the output signal is driven HIGH or LOW (depends
>> on the configured polarity). This value is used to define the period
>> of the PWM.
>>
>> CMOD, on the other hand, is a clocking-related configuration option.
>> I'd say what we're most interested in here is the fact that if CMOD is
>> 0 then the counter will be disabled. Otherwise, it will be enabled.
> JFTR: I marked your patch as "changes requested" now in patchwork and my
> inbox and expect an updated patch, but without holding my breath :-)

Sorry for the long delay! Got this on my TODO list. Busy week :(


Thanks,
Laurentiu

Re: [PATCH] pwm: imx-tpm: reset counter if CMOD is 0

Posted by Uwe Kleine-König 2 months, 4 weeks ago

Hello Laurentiu,

On Wed, Jul 09, 2025 at 11:29:16AM +0300, Laurentiu Mihalcea wrote:
> On 7/9/2025 9:05 AM, Uwe Kleine-König wrote:
> > JFTR: I marked your patch as "changes requested" now in patchwork and my
> > inbox and expect an updated patch, but without holding my breath :-)
> 
> Sorry for the long delay! Got this on my TODO list. Busy week :(

No need to hurry for me, I'm not busy waiting and we won't be able to
stop Linus releasting 6.16 anyhow :-)

I prefer well considered patches over patches in quick succession anyhow.

So it's ok to take your time
Uwe