[v2] pwm: imx27: workaround of the pwm output bug

Commit Message

[e-mail] Jan. 3, 2024, 6:34 a.m. UTC

From: Clark Wang <[e-mail]>

This fixes the pwm output bug when decrease the duty cycle.
This is a limited workaround for the PWM IP issue TKT0577206.

Root cause:
When the SAR FIFO is empty, the new write value will be directly applied
to SAR even the current period is not over.
If the new SAR value is less than the old one, and the counter is
greater than the new SAR value, the current period will not filp the
level. This will result in a pulse with a duty cycle of 100%.

Workaround:
Add an old value SAR write before updating the new duty cycle to SAR.
This will keep the new value is always in a not empty fifo, and can be
wait to update after a period finished.

Limitation:
This workaround can only solve this issue when the PWM period is longer
than 2us(or <500KHz).

Reviewed-by: Jun Li <[e-mail]>
Signed-off-by: Clark Wang <[e-mail]>
Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
Tested-by: Pratik Manvar <[e-mail]>
---
 V1 -> V2: fix sparse warnings reported-by: kernel test robot <[e-mail]>
           Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

 drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 62 insertions(+), 5 deletions(-)

Comments

Stefan Wahren Jan. 3, 2024, 11 a.m. UTC | #1

Hi Pratik,

Am 03.01.24 um 07:34 schrieb [e-mail]:
> From: Clark Wang <[e-mail]>
>
> This fixes the pwm output bug when decrease the duty cycle.
> This is a limited workaround for the PWM IP issue TKT0577206.
this looks like a patch from the vendor tree.

Could you please provide a link to the origin or at least to the
document which describes TKT0577206?

As a i.MX6ULL user i couldn't find this issue in the chip errata. So are
you sure that every PWM IP handled by this driver is affected?
>
> Root cause:
> When the SAR FIFO is empty, the new write value will be directly applied
> to SAR even the current period is not over.
> If the new SAR value is less than the old one, and the counter is
> greater than the new SAR value, the current period will not filp the
s/filp/flip/ ?
> level. This will result in a pulse with a duty cycle of 100%.
>
> Workaround:
> Add an old value SAR write before updating the new duty cycle to SAR.
> This will keep the new value is always in a not empty fifo, and can be
> wait to update after a period finished.
>
> Limitation:
> This workaround can only solve this issue when the PWM period is longer
> than 2us(or <500KHz).
>
> Reviewed-by: Jun Li <[e-mail]>
> Signed-off-by: Clark Wang <[e-mail]>
> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
> Tested-by: Pratik Manvar <[e-mail]>
> ---
>   V1 -> V2: fix sparse warnings reported-by: kernel test robot <[e-mail]>
>             Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/
>
>   drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 62 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 7d9bc43f12b0..1e500a5bf564 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -21,11 +21,13 @@
>   #include <linux/platform_device.h>
>   #include <linux/pwm.h>
>   #include <linux/slab.h>
> +#include <linux/spinlock.h>
>
>   #define MX3_PWMCR			0x00    /* PWM Control Register */
>   #define MX3_PWMSR			0x04    /* PWM Status Register */
>   #define MX3_PWMSAR			0x0C    /* PWM Sample Register */
>   #define MX3_PWMPR			0x10    /* PWM Period Register */
> +#define MX3_PWMCNR			0x14    /* PWM Counter Register */
>
>   #define MX3_PWMCR_FWM			GENMASK(27, 26)
>   #define MX3_PWMCR_STOPEN		BIT(25)
> @@ -91,6 +93,7 @@ struct pwm_imx27_chip {
>   	 * value to return in that case.
>   	 */
>   	unsigned int duty_cycle;
> +	spinlock_t lock;
>   };
>
>   #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
> @@ -203,10 +206,10 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
>
>   	sr = readl(imx->mmio_base + MX3_PWMSR);
>   	fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
> -	if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
> +	if (fifoav >= MX3_PWMSR_FIFOAV_3WORDS) {
>   		period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm),
>   					 NSEC_PER_MSEC);
> -		msleep(period_ms);
> +		msleep(period_ms * (fifoav - 2));
This touches a different workaround ("pwm: imx: Avoid sample FIFO
overflow for i.MX PWM version2") without any explanation.
>
>   		sr = readl(imx->mmio_base + MX3_PWMSR);
>   		if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr))
> @@ -217,13 +220,15 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
>   static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   			   const struct pwm_state *state)
>   {
> -	unsigned long period_cycles, duty_cycles, prescale;
> +	unsigned long period_cycles, duty_cycles, prescale, counter_check, flags;
>   	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
> +	void __iomem *reg_sar = imx->mmio_base + MX3_PWMSAR;
> +	__force u32 sar_last, sar_current;
>   	struct pwm_state cstate;
>   	unsigned long long c;
>   	unsigned long long clkrate;
>   	int ret;
> -	u32 cr;
> +	u32 cr, timeout = 1000;
>
>   	pwm_get_state(pwm, &cstate);
>
> @@ -264,7 +269,57 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   		pwm_imx27_sw_reset(chip);
>   	}
>
> -	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> +	/*
> +	 * This is a limited workaround. When the SAR FIFO is empty, the new
> +	 * write value will be directly applied to SAR even the current period
> +	 * is not over.
> +	 * If the new SAR value is less than the old one, and the counter is
> +	 * greater than the new SAR value, the current period will not filp
The same typo as in the commit message.
> +	 * the level. This will result in a pulse with a duty cycle of 100%.
> +	 * So, writing the current value of the SAR to SAR here before updating
> +	 * the new SAR value can avoid this issue.
> +	 *
> +	 * Add a spin lock and turn off the interrupt to ensure that the
> +	 * real-time performance can be guaranteed as much as possible when
> +	 * operating the following operations.
> +	 *
> +	 * 1. Add a threshold of 1.5us. If the time T between the read current
> +	 * count value CNR and the end of the cycle is less than 1.5us, wait
> +	 * for T to be longer than 1.5us before updating the SAR register.
> +	 * This is to avoid the situation that when the first SAR is written,
> +	 * the current cycle just ends and the SAR FIFO that just be written
> +	 * is emptied again.
> +	 *
> +	 * 2. Use __raw_writel() to minimize the interval between two writes to
> +	 * the SAR register to increase the fastest pwm frequency supported.
> +	 *
> +	 * When the PWM period is longer than 2us(or <500KHz), this workaround
> +	 * can solve this problem.
> +	 */
> +	if (duty_cycles < imx->duty_cycle) {
> +		c = clkrate * 1500;
> +		do_div(c, NSEC_PER_SEC);
> +		counter_check = c;
> +		sar_last = (__force u32) cpu_to_le32(imx->duty_cycle);
> +		sar_current = (__force u32) cpu_to_le32(duty_cycles);
> +
> +		spin_lock_irqsave(&imx->lock, flags);
> +		if (state->period >= 2000) {
> +			while ((period_cycles -
> +				readl_relaxed(imx->mmio_base + MX3_PWMCNR))
> +				< counter_check) {
> +				if (!--timeout)
> +					break;
> +			};
> +		}
> +		if (!(MX3_PWMSR_FIFOAV &
> +		      readl_relaxed(imx->mmio_base + MX3_PWMSR)))
> +			__raw_writel(sar_last, reg_sar);
> +		__raw_writel(sar_current, reg_sar);
> +		spin_unlock_irqrestore(&imx->lock, flags);
> +	} else
> +		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> +
This is hard to believe that checkpatch.pl is fine with this patch.
Please use it before submission.
>   	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
>
>   	/*
> @@ -324,6 +379,8 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>   		return dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_per),
>   				     "failed to get peripheral clock\n");
>
> +	spin_lock_init(&imx->lock);
> +	imx->duty_cycle = 0;
This line looks unrelated and unnecessary.

Best regards
>   	imx->chip.ops = &pwm_imx27_ops;
>   	imx->chip.dev = &pdev->dev;
>   	imx->chip.npwm = 1;

[e-mail] Feb. 4, 2024, 6:36 a.m. UTC | #2

Hi Stefan,

Thanks for your review.
Please see my reply below inline.

>> From: Clark Wang <[e-mail]>
>>
>> This fixes the pwm output bug when decrease the duty cycle.
>> This is a limited workaround for the PWM IP issue TKT0577206.
>this looks like a patch from the vendor tree.

[Pratik]: Yes, this is the patch from NXP. Please see original link of the patch https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317

>Could you please provide a link to the origin or at least to the
>document which describes TKT0577206?
[Pratik]: Please refer i.MX8MN errata #ERR051198 in https://www.nxp.com/docs/en/errata/IMX8MN_0N14Y.pdf.

>As a i.MX6ULL user i couldn't find this issue in the chip errata. So are
>you sure that every PWM IP handled by this driver is affected?
[Pratik]: Yes, looks like this issue is on all platforms which uses this PWM IP.

>> Root cause:
>> When the SAR FIFO is empty, the new write value will be directly applied
>> to SAR even the current period is not over.
>> If the new SAR value is less than the old one, and the counter is
>> greater than the new SAR value, the current period will not filp the
>s/filp/flip/ ?
>> level. This will result in a pulse with a duty cycle of 100%.
>>
>> Workaround:
>> Add an old value SAR write before updating the new duty cycle to SAR.
>> This will keep the new value is always in a not empty fifo, and can be
>> wait to update after a period finished.
>>
>> Limitation:
>> This workaround can only solve this issue when the PWM period is longer
>> than 2us(or <500KHz).
>>
>> Reviewed-by: Jun Li <[e-mail]>
>> Signed-off-by: Clark Wang <[e-mail]>
>> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
>> Tested-by: Pratik Manvar <[e-mail]>
>> ---
>>   V1 -> V2: fix sparse warnings reported-by: kernel test robot <[e-mail]>
>>             Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/
>>
>>   drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 62 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
>> index 7d9bc43f12b0..1e500a5bf564 100644
>> --- a/drivers/pwm/pwm-imx27.c
>> +++ b/drivers/pwm/pwm-imx27.c
>> @@ -21,11 +21,13 @@
>>   #include <linux/platform_device.h>
>>   #include <linux/pwm.h>
>>   #include <linux/slab.h>
>> +#include <linux/spinlock.h>
>>
>>   #define MX3_PWMCR			0x00    /* PWM Control Register */
>>   #define MX3_PWMSR			0x04    /* PWM Status Register */
>>   #define MX3_PWMSAR			0x0C    /* PWM Sample Register */
>>   #define MX3_PWMPR			0x10    /* PWM Period Register */
>> +#define MX3_PWMCNR			0x14    /* PWM Counter Register */
>>
>>   #define MX3_PWMCR_FWM			GENMASK(27, 26)
>>   #define MX3_PWMCR_STOPEN		BIT(25)
>> @@ -91,6 +93,7 @@ struct pwm_imx27_chip {
>>   	 * value to return in that case.
>>   	 */
>>   	unsigned int duty_cycle;
>> +	spinlock_t lock;
>>   };
>>
>>   #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
>> @@ -203,10 +206,10 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
>>
>>   	sr = readl(imx->mmio_base + MX3_PWMSR);
>>   	fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
>> -	if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
>> +	if (fifoav >= MX3_PWMSR_FIFOAV_3WORDS) {
>>   		period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm),
>>   					 NSEC_PER_MSEC);
>> -		msleep(period_ms);
>> +		msleep(period_ms * (fifoav - 2));
>This touches a different workaround ("pwm: imx: Avoid sample FIFO
>overflow for i.MX PWM version2") without any explanation.
[Pratik]: Sure, I will look into this. Thanks!
>>
>>   		sr = readl(imx->mmio_base + MX3_PWMSR);
>>   		if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr))
>> @@ -217,13 +220,15 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
>>   static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>   			   const struct pwm_state *state)
>>   {
>> -	unsigned long period_cycles, duty_cycles, prescale;
>> +	unsigned long period_cycles, duty_cycles, prescale, counter_check, flags;
>>   	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
>> +	void __iomem *reg_sar = imx->mmio_base + MX3_PWMSAR;
>> +	__force u32 sar_last, sar_current;
>>   	struct pwm_state cstate;
>>   	unsigned long long c;
>>   	unsigned long long clkrate;
>>   	int ret;
>> -	u32 cr;
>> +	u32 cr, timeout = 1000;
>>
>>   	pwm_get_state(pwm, &cstate);
>>
>> @@ -264,7 +269,57 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>   		pwm_imx27_sw_reset(chip);
>>   	}
>>
>> -	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>> +	/*
>> +	 * This is a limited workaround. When the SAR FIFO is empty, the new
>> +	 * write value will be directly applied to SAR even the current period
>> +	 * is not over.
>> +	 * If the new SAR value is less than the old one, and the counter is
>> +	 * greater than the new SAR value, the current period will not filp
>The same typo as in the commit message.
>> +	 * the level. This will result in a pulse with a duty cycle of 100%.
>> +	 * So, writing the current value of the SAR to SAR here before updating
>> +	 * the new SAR value can avoid this issue.
>> +	 *
>> +	 * Add a spin lock and turn off the interrupt to ensure that the
>> +	 * real-time performance can be guaranteed as much as possible when
>> +	 * operating the following operations.
>> +	 *
>> +	 * 1. Add a threshold of 1.5us. If the time T between the read current
>> +	 * count value CNR and the end of the cycle is less than 1.5us, wait
>> +	 * for T to be longer than 1.5us before updating the SAR register.
>> +	 * This is to avoid the situation that when the first SAR is written,
>> +	 * the current cycle just ends and the SAR FIFO that just be written
>> +	 * is emptied again.
>> +	 *
>> +	 * 2. Use __raw_writel() to minimize the interval between two writes to
>> +	 * the SAR register to increase the fastest pwm frequency supported.
>> +	 *
>> +	 * When the PWM period is longer than 2us(or <500KHz), this workaround
>> +	 * can solve this problem.
>> +	 */
>> +	if (duty_cycles < imx->duty_cycle) {
>> +		c = clkrate * 1500;
>> +		do_div(c, NSEC_PER_SEC);
>> +		counter_check = c;
>> +		sar_last = (__force u32) cpu_to_le32(imx->duty_cycle);
>> +		sar_current = (__force u32) cpu_to_le32(duty_cycles);
>> +
>> +		spin_lock_irqsave(&imx->lock, flags);
>> +		if (state->period >= 2000) {
>> +			while ((period_cycles -
>> +				readl_relaxed(imx->mmio_base + MX3_PWMCNR))
>> +				< counter_check) {
>> +				if (!--timeout)
>> +					break;
>> +			};
>> +		}
>> +		if (!(MX3_PWMSR_FIFOAV &
>> +		      readl_relaxed(imx->mmio_base + MX3_PWMSR)))
>> +			__raw_writel(sar_last, reg_sar);
>> +		__raw_writel(sar_current, reg_sar);
>> +		spin_unlock_irqrestore(&imx->lock, flags);
>> +	} else
>> +		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>> +
>This is hard to believe that checkpatch.pl is fine with this patch.
>Please use it before submission.
[Pratik]: I used the checkpatch.pl in this patch and that runs without any warnings/errors!

>>   	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
>>
>>   	/*
>> @@ -324,6 +379,8 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>>   		return dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_per),
>>   				     "failed to get peripheral clock\n");
>>
>> +	spin_lock_init(&imx->lock);
>> +	imx->duty_cycle = 0;
>This line looks unrelated and unnecessary.
[Pratik]: Right. I will remove this line in next patch version.

>Best regards
>>   	imx->chip.ops = &pwm_imx27_ops;
>>   	imx->chip.dev = &pdev->dev;
>>   	imx->chip.npwm = 1;

Stefan Wahren Feb. 4, 2024, 10:02 a.m. UTC | #3

Hi Pratik,

Am 04.02.24 um 07:36 schrieb [e-mail]:
> Hi Stefan,
>
> Thanks for your review.
> Please see my reply below inline.
>
>>> From: Clark Wang <[e-mail]>
>>>
>>> This fixes the pwm output bug when decrease the duty cycle.
>>> This is a limited workaround for the PWM IP issue TKT0577206.
>> this looks like a patch from the vendor tree.
> [Pratik]: Yes, this is the patch from NXP. Please see original link of the patch https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
>
>> Could you please provide a link to the origin or at least to the
>> document which describes TKT0577206?
> [Pratik]: Please refer i.MX8MN errata #ERR051198 in https://www.nxp.com/docs/en/errata/IMX8MN_0N14Y.pdf.
Thanks, i think this ERR... reference is better than TKT... because it's
links to the errata documents and other Freescale/NXP drivers use them
too. So having this code in a comment would be great.
>
>> As a i.MX6ULL user i couldn't find this issue in the chip errata. So are
>> you sure that every PWM IP handled by this driver is affected?
> [Pratik]: Yes, looks like this issue is on all platforms which uses this PWM IP.
>
>>> Root cause:
>>> When the SAR FIFO is empty, the new write value will be directly applied
>>> to SAR even the current period is not over.
>>> If the new SAR value is less than the old one, and the counter is
>>> greater than the new SAR value, the current period will not filp the
>> s/filp/flip/ ?
>>> level. This will result in a pulse with a duty cycle of 100%.
>>>
>>> Workaround:
>>> Add an old value SAR write before updating the new duty cycle to SAR.
>>> This will keep the new value is always in a not empty fifo, and can be
>>> wait to update after a period finished.
>>>
>>> Limitation:
>>> This workaround can only solve this issue when the PWM period is longer
>>> than 2us(or <500KHz).
>>>
>>> Reviewed-by: Jun Li <[e-mail]>
>>> Signed-off-by: Clark Wang <[e-mail]>
>>> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
>>> Tested-by: Pratik Manvar <[e-mail]>
>>> ---
>>>    V1 -> V2: fix sparse warnings reported-by: kernel test robot <[e-mail]>
>>>              Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/
>>>
>>>    drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++---
>>>    1 file changed, 62 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
>>> index 7d9bc43f12b0..1e500a5bf564 100644
>>> --- a/drivers/pwm/pwm-imx27.c
>>> +++ b/drivers/pwm/pwm-imx27.c
>>> @@ -21,11 +21,13 @@
>>>    #include <linux/platform_device.h>
>>>    #include <linux/pwm.h>
>>>    #include <linux/slab.h>
>>> +#include <linux/spinlock.h>
>>>
>>>    #define MX3_PWMCR			0x00    /* PWM Control Register */
>>>    #define MX3_PWMSR			0x04    /* PWM Status Register */
>>>    #define MX3_PWMSAR			0x0C    /* PWM Sample Register */
>>>    #define MX3_PWMPR			0x10    /* PWM Period Register */
>>> +#define MX3_PWMCNR			0x14    /* PWM Counter Register */
>>>
>>>    #define MX3_PWMCR_FWM			GENMASK(27, 26)
>>>    #define MX3_PWMCR_STOPEN		BIT(25)
>>> @@ -91,6 +93,7 @@ struct pwm_imx27_chip {
>>>    	 * value to return in that case.
>>>    	 */
>>>    	unsigned int duty_cycle;
>>> +	spinlock_t lock;
>>>    };
>>>
>>>    #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
>>> @@ -203,10 +206,10 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
>>>
>>>    	sr = readl(imx->mmio_base + MX3_PWMSR);
>>>    	fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
>>> -	if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
>>> +	if (fifoav >= MX3_PWMSR_FIFOAV_3WORDS) {
>>>    		period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm),
>>>    					 NSEC_PER_MSEC);
>>> -		msleep(period_ms);
>>> +		msleep(period_ms * (fifoav - 2));
>> This touches a different workaround ("pwm: imx: Avoid sample FIFO
>> overflow for i.MX PWM version2") without any explanation.
> [Pratik]: Sure, I will look into this. Thanks!
>>>    		sr = readl(imx->mmio_base + MX3_PWMSR);
>>>    		if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr))
>>> @@ -217,13 +220,15 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
>>>    static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>>    			   const struct pwm_state *state)
>>>    {
>>> -	unsigned long period_cycles, duty_cycles, prescale;
>>> +	unsigned long period_cycles, duty_cycles, prescale, counter_check, flags;
>>>    	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
>>> +	void __iomem *reg_sar = imx->mmio_base + MX3_PWMSAR;
>>> +	__force u32 sar_last, sar_current;
>>>    	struct pwm_state cstate;
>>>    	unsigned long long c;
>>>    	unsigned long long clkrate;
>>>    	int ret;
>>> -	u32 cr;
>>> +	u32 cr, timeout = 1000;
>>>
>>>    	pwm_get_state(pwm, &cstate);
>>>
>>> @@ -264,7 +269,57 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>>    		pwm_imx27_sw_reset(chip);
>>>    	}
>>>
>>> -	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>>> +	/*
>>> +	 * This is a limited workaround. When the SAR FIFO is empty, the new
>>> +	 * write value will be directly applied to SAR even the current period
>>> +	 * is not over.
>>> +	 * If the new SAR value is less than the old one, and the counter is
>>> +	 * greater than the new SAR value, the current period will not filp
>> The same typo as in the commit message.
>>> +	 * the level. This will result in a pulse with a duty cycle of 100%.
>>> +	 * So, writing the current value of the SAR to SAR here before updating
>>> +	 * the new SAR value can avoid this issue.
>>> +	 *
>>> +	 * Add a spin lock and turn off the interrupt to ensure that the
>>> +	 * real-time performance can be guaranteed as much as possible when
>>> +	 * operating the following operations.
>>> +	 *
>>> +	 * 1. Add a threshold of 1.5us. If the time T between the read current
>>> +	 * count value CNR and the end of the cycle is less than 1.5us, wait
>>> +	 * for T to be longer than 1.5us before updating the SAR register.
>>> +	 * This is to avoid the situation that when the first SAR is written,
>>> +	 * the current cycle just ends and the SAR FIFO that just be written
>>> +	 * is emptied again.
>>> +	 *
>>> +	 * 2. Use __raw_writel() to minimize the interval between two writes to
>>> +	 * the SAR register to increase the fastest pwm frequency supported.
>>> +	 *
>>> +	 * When the PWM period is longer than 2us(or <500KHz), this workaround
>>> +	 * can solve this problem.
>>> +	 */
>>> +	if (duty_cycles < imx->duty_cycle) {
>>> +		c = clkrate * 1500;
>>> +		do_div(c, NSEC_PER_SEC);
>>> +		counter_check = c;
>>> +		sar_last = (__force u32) cpu_to_le32(imx->duty_cycle);
>>> +		sar_current = (__force u32) cpu_to_le32(duty_cycles);
>>> +
>>> +		spin_lock_irqsave(&imx->lock, flags);
>>> +		if (state->period >= 2000) {
>>> +			while ((period_cycles -
>>> +				readl_relaxed(imx->mmio_base + MX3_PWMCNR))
>>> +				< counter_check) {
>>> +				if (!--timeout)
>>> +					break;
>>> +			};
>>> +		}
>>> +		if (!(MX3_PWMSR_FIFOAV &
>>> +		      readl_relaxed(imx->mmio_base + MX3_PWMSR)))
>>> +			__raw_writel(sar_last, reg_sar);
>>> +		__raw_writel(sar_current, reg_sar);
>>> +		spin_unlock_irqrestore(&imx->lock, flags);
>>> +	} else
>>> +		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>>> +
>> This is hard to believe that checkpatch.pl is fine with this patch.
>> Please use it before submission.
> [Pratik]: I used the checkpatch.pl in this patch and that runs without any warnings/errors!
Okay, AFAIR the coding style suggests braces for the else case.
>
>>>    	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
>>>
>>>    	/*
>>> @@ -324,6 +379,8 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>>>    		return dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_per),
>>>    				     "failed to get peripheral clock\n");
>>>
>>> +	spin_lock_init(&imx->lock);
>>> +	imx->duty_cycle = 0;
>> This line looks unrelated and unnecessary.
> [Pratik]: Right. I will remove this line in next patch version.
Could you also please look at Uwe's comments [1]?

Thanks

[1] -
https://lore.kernel.org/all/[e-mail]/
>
>> Best regards
>>>    	imx->chip.ops = &pwm_imx27_ops;
>>>    	imx->chip.dev = &pdev->dev;
>>>    	imx->chip.npwm = 1;

[e-mail] March 5, 2024, 9:24 a.m. UTC | #4

Hi Stefan,

Sorry for the abysmal delay.
Thanks for your review and suggestions.

>Hi Pratik,
>
>Am 04.02.24 um 07:36 schrieb [e-mail]:
>> Hi Stefan,
>>
>> Thanks for your review.
>> Please see my reply below inline.
>>
>>>> From: Clark Wang <[e-mail]>
>>>>
>>>> This fixes the pwm output bug when decrease the duty cycle.
>>>> This is a limited workaround for the PWM IP issue TKT0577206.
>>> this looks like a patch from the vendor tree.
>> [Pratik]: Yes, this is the patch from NXP. Please see original link of the patch https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
>>
>>> Could you please provide a link to the origin or at least to the
>>> document which describes TKT0577206?
>> [Pratik]: Please refer i.MX8MN errata #ERR051198 in https://www.nxp.com/docs/en/errata/IMX8MN_0N14Y.pdf.
>Thanks, i think this ERR... reference is better than TKT... because it's
>links to the errata documents and other Freescale/NXP drivers use them
>too. So having this code in a comment would be great.
Sure, I will mention this #ERR051198 code in commit message.
>>
>>> As a i.MX6ULL user i couldn't find this issue in the chip errata. So are
>>> you sure that every PWM IP handled by this driver is affected?
>> [Pratik]: Yes, looks like this issue is on all platforms which uses this PWM IP.
>>
>>>> Root cause:
>>>> When the SAR FIFO is empty, the new write value will be directly applied
>>>> to SAR even the current period is not over.
>>>> If the new SAR value is less than the old one, and the counter is
>>>> greater than the new SAR value, the current period will not filp the
>>> s/filp/flip/ ?
>>>> level. This will result in a pulse with a duty cycle of 100%.
>>>>
>>>> Workaround:
>>>> Add an old value SAR write before updating the new duty cycle to SAR.
>>>> This will keep the new value is always in a not empty fifo, and can be
>>>> wait to update after a period finished.
>>>>
>>>> Limitation:
>>>> This workaround can only solve this issue when the PWM period is longer
>>>> than 2us(or <500KHz).
>>>>
>>>> Reviewed-by: Jun Li <[e-mail]>
>>>> Signed-off-by: Clark Wang <[e-mail]>
>>>> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
>>>> Tested-by: Pratik Manvar <[e-mail]>
>>>> ---
>>>>    V1 -> V2: fix sparse warnings reported-by: kernel test robot <[e-mail]>
>>>>              Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/
>>>>
>>>>    drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++---
>>>>    1 file changed, 62 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
>>>> index 7d9bc43f12b0..1e500a5bf564 100644
>>>> --- a/drivers/pwm/pwm-imx27.c
>>>> +++ b/drivers/pwm/pwm-imx27.c
>>>> @@ -21,11 +21,13 @@
>>>>    #include <linux/platform_device.h>
>>>>    #include <linux/pwm.h>
>>>>    #include <linux/slab.h>
>>>> +#include <linux/spinlock.h>
>>>>
>>>>    #define MX3_PWMCR			0x00    /* PWM Control Register */
>>>>    #define MX3_PWMSR			0x04    /* PWM Status Register */
>>>>    #define MX3_PWMSAR			0x0C    /* PWM Sample Register */
>>>>    #define MX3_PWMPR			0x10    /* PWM Period Register */
>>>> +#define MX3_PWMCNR			0x14    /* PWM Counter Register */
>>>>
>>>>    #define MX3_PWMCR_FWM			GENMASK(27, 26)
>>>>    #define MX3_PWMCR_STOPEN		BIT(25)
>>>> @@ -91,6 +93,7 @@ struct pwm_imx27_chip {
>>>>    	 * value to return in that case.
>>>>    	 */
>>>>    	unsigned int duty_cycle;
>>>> +	spinlock_t lock;
>>>>    };
>>>>
>>>>    #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
>>>> @@ -203,10 +206,10 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
>>>>
>>>>    	sr = readl(imx->mmio_base + MX3_PWMSR);
>>>>    	fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
>>>> -	if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
>>>> +	if (fifoav >= MX3_PWMSR_FIFOAV_3WORDS) {
>>>>    		period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm),
>>>>    					 NSEC_PER_MSEC);
>>>> -		msleep(period_ms);
>>>> +		msleep(period_ms * (fifoav - 2));
>>> This touches a different workaround ("pwm: imx: Avoid sample FIFO
>>> overflow for i.MX PWM version2") without any explanation.
>> [Pratik]: Sure, I will look into this. Thanks!
>>>>    		sr = readl(imx->mmio_base + MX3_PWMSR);
>>>>    		if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr))
>>>> @@ -217,13 +220,15 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
>>>>    static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>>>    			   const struct pwm_state *state)
>>>>    {
>>>> -	unsigned long period_cycles, duty_cycles, prescale;
>>>> +	unsigned long period_cycles, duty_cycles, prescale, counter_check, flags;
>>>>    	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
>>>> +	void __iomem *reg_sar = imx->mmio_base + MX3_PWMSAR;
>>>> +	__force u32 sar_last, sar_current;
>>>>    	struct pwm_state cstate;
>>>>    	unsigned long long c;
>>>>    	unsigned long long clkrate;
>>>>    	int ret;
>>>> -	u32 cr;
>>>> +	u32 cr, timeout = 1000;
>>>>
>>>>    	pwm_get_state(pwm, &cstate);
>>>>
>>>> @@ -264,7 +269,57 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>>>    		pwm_imx27_sw_reset(chip);
>>>>    	}
>>>>
>>>> -	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>>>> +	/*
>>>> +	 * This is a limited workaround. When the SAR FIFO is empty, the new
>>>> +	 * write value will be directly applied to SAR even the current period
>>>> +	 * is not over.
>>>> +	 * If the new SAR value is less than the old one, and the counter is
>>>> +	 * greater than the new SAR value, the current period will not filp
>>> The same typo as in the commit message.
>>>> +	 * the level. This will result in a pulse with a duty cycle of 100%.
>>>> +	 * So, writing the current value of the SAR to SAR here before updating
>>>> +	 * the new SAR value can avoid this issue.
>>>> +	 *
>>>> +	 * Add a spin lock and turn off the interrupt to ensure that the
>>>> +	 * real-time performance can be guaranteed as much as possible when
>>>> +	 * operating the following operations.
>>>> +	 *
>>>> +	 * 1. Add a threshold of 1.5us. If the time T between the read current
>>>> +	 * count value CNR and the end of the cycle is less than 1.5us, wait
>>>> +	 * for T to be longer than 1.5us before updating the SAR register.
>>>> +	 * This is to avoid the situation that when the first SAR is written,
>>>> +	 * the current cycle just ends and the SAR FIFO that just be written
>>>> +	 * is emptied again.
>>>> +	 *
>>>> +	 * 2. Use __raw_writel() to minimize the interval between two writes to
>>>> +	 * the SAR register to increase the fastest pwm frequency supported.
>>>> +	 *
>>>> +	 * When the PWM period is longer than 2us(or <500KHz), this workaround
>>>> +	 * can solve this problem.
>>>> +	 */
>>>> +	if (duty_cycles < imx->duty_cycle) {
>>>> +		c = clkrate * 1500;
>>>> +		do_div(c, NSEC_PER_SEC);
>>>> +		counter_check = c;
>>>> +		sar_last = (__force u32) cpu_to_le32(imx->duty_cycle);
>>>> +		sar_current = (__force u32) cpu_to_le32(duty_cycles);
>>>> +
>>>> +		spin_lock_irqsave(&imx->lock, flags);
>>>> +		if (state->period >= 2000) {
>>>> +			while ((period_cycles -
>>>> +				readl_relaxed(imx->mmio_base + MX3_PWMCNR))
>>>> +				< counter_check) {
>>>> +				if (!--timeout)
>>>> +					break;
>>>> +			};
>>>> +		}
>>>> +		if (!(MX3_PWMSR_FIFOAV &
>>>> +		      readl_relaxed(imx->mmio_base + MX3_PWMSR)))
>>>> +			__raw_writel(sar_last, reg_sar);
>>>> +		__raw_writel(sar_current, reg_sar);
>>>> +		spin_unlock_irqrestore(&imx->lock, flags);
>>>> +	} else
>>>> +		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>>>> +
>>> This is hard to believe that checkpatch.pl is fine with this patch.
>>> Please use it before submission.
>> [Pratik]: I used the checkpatch.pl in this patch and that runs without any warnings/errors!
>Okay, AFAIR the coding style suggests braces for the else case.
>>
>>>>    	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
>>>>
>>>>    	/*
>>>> @@ -324,6 +379,8 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>>>>    		return dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_per),
>>>>    				     "failed to get peripheral clock\n");
>>>>
>>>> +	spin_lock_init(&imx->lock);
>>>> +	imx->duty_cycle = 0;
>>> This line looks unrelated and unnecessary.
>> [Pratik]: Right. I will remove this line in next patch version.
>Could you also please look at Uwe's comments [1]?
>
>Thanks
>
>[1] -
>https://lore.kernel.org/all/[e-mail]/
Actually, I did not get much time to work on this. But, I will look into this now.
>>
>>> Best regards
>>>>    	imx->chip.ops = &pwm_imx27_ops;
>>>>    	imx->chip.dev = &pdev->dev;
>>>>    	imx->chip.npwm = 1;

Thanks & Regards,
Pratik Manvar

Patch

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 7d9bc43f12b0..1e500a5bf564 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -21,11 +21,13 @@ 
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 
 #define MX3_PWMCR			0x00    /* PWM Control Register */
 #define MX3_PWMSR			0x04    /* PWM Status Register */
 #define MX3_PWMSAR			0x0C    /* PWM Sample Register */
 #define MX3_PWMPR			0x10    /* PWM Period Register */
+#define MX3_PWMCNR			0x14    /* PWM Counter Register */
 
 #define MX3_PWMCR_FWM			GENMASK(27, 26)
 #define MX3_PWMCR_STOPEN		BIT(25)
@@ -91,6 +93,7 @@  struct pwm_imx27_chip {
 	 * value to return in that case.
 	 */
 	unsigned int duty_cycle;
+	spinlock_t lock;
 };
 
 #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
@@ -203,10 +206,10 @@  static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
 
 	sr = readl(imx->mmio_base + MX3_PWMSR);
 	fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
-	if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
+	if (fifoav >= MX3_PWMSR_FIFOAV_3WORDS) {
 		period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm),
 					 NSEC_PER_MSEC);
-		msleep(period_ms);
+		msleep(period_ms * (fifoav - 2));
 
 		sr = readl(imx->mmio_base + MX3_PWMSR);
 		if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr))
@@ -217,13 +220,15 @@  static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
 static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			   const struct pwm_state *state)
 {
-	unsigned long period_cycles, duty_cycles, prescale;
+	unsigned long period_cycles, duty_cycles, prescale, counter_check, flags;
 	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
+	void __iomem *reg_sar = imx->mmio_base + MX3_PWMSAR;
+	__force u32 sar_last, sar_current;
 	struct pwm_state cstate;
 	unsigned long long c;
 	unsigned long long clkrate;
 	int ret;
-	u32 cr;
+	u32 cr, timeout = 1000;
 
 	pwm_get_state(pwm, &cstate);
 
@@ -264,7 +269,57 @@  static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		pwm_imx27_sw_reset(chip);
 	}
 
-	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
+	/*
+	 * This is a limited workaround. When the SAR FIFO is empty, the new
+	 * write value will be directly applied to SAR even the current period
+	 * is not over.
+	 * If the new SAR value is less than the old one, and the counter is
+	 * greater than the new SAR value, the current period will not filp
+	 * the level. This will result in a pulse with a duty cycle of 100%.
+	 * So, writing the current value of the SAR to SAR here before updating
+	 * the new SAR value can avoid this issue.
+	 *
+	 * Add a spin lock and turn off the interrupt to ensure that the
+	 * real-time performance can be guaranteed as much as possible when
+	 * operating the following operations.
+	 *
+	 * 1. Add a threshold of 1.5us. If the time T between the read current
+	 * count value CNR and the end of the cycle is less than 1.5us, wait
+	 * for T to be longer than 1.5us before updating the SAR register.
+	 * This is to avoid the situation that when the first SAR is written,
+	 * the current cycle just ends and the SAR FIFO that just be written
+	 * is emptied again.
+	 *
+	 * 2. Use __raw_writel() to minimize the interval between two writes to
+	 * the SAR register to increase the fastest pwm frequency supported.
+	 *
+	 * When the PWM period is longer than 2us(or <500KHz), this workaround
+	 * can solve this problem.
+	 */
+	if (duty_cycles < imx->duty_cycle) {
+		c = clkrate * 1500;
+		do_div(c, NSEC_PER_SEC);
+		counter_check = c;
+		sar_last = (__force u32) cpu_to_le32(imx->duty_cycle);
+		sar_current = (__force u32) cpu_to_le32(duty_cycles);
+
+		spin_lock_irqsave(&imx->lock, flags);
+		if (state->period >= 2000) {
+			while ((period_cycles -
+				readl_relaxed(imx->mmio_base + MX3_PWMCNR))
+				< counter_check) {
+				if (!--timeout)
+					break;
+			};
+		}
+		if (!(MX3_PWMSR_FIFOAV &
+		      readl_relaxed(imx->mmio_base + MX3_PWMSR)))
+			__raw_writel(sar_last, reg_sar);
+		__raw_writel(sar_current, reg_sar);
+		spin_unlock_irqrestore(&imx->lock, flags);
+	} else
+		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
+
 	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
 
 	/*
@@ -324,6 +379,8 @@  static int pwm_imx27_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_per),
 				     "failed to get peripheral clock\n");
 
+	spin_lock_init(&imx->lock);
+	imx->duty_cycle = 0;
 	imx->chip.ops = &pwm_imx27_ops;
 	imx->chip.dev = &pdev->dev;
 	imx->chip.npwm = 1;