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

From: pratikmanvar09
Date: Tue Mar 05 2024 - 04:24:53 EST

Next message: Christophe Leroy: "Re: [PATCH v2 3/3] arch/powerpc: Remove <linux/fb.h> from backlight code"
Previous message: Rafael J. Wysocki: "Re: [PATCH v3] driver core: Cancel scheduled pm_runtime_idle() on device removal"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Stefan,

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

>Hi Pratik,
>
>Am 04.02.24 um 07:36 schrieb pratikmanvar09@xxxxxxxxx:
>> Hi Stefan,
>>
>> Thanks for your review.
>> Please see my reply below inline.
>>
>>>> From: Clark Wang <xiaoning.wang@xxxxxxx>
>>>>
>>>> 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 <jun.li@xxxxxxx>
>>>> Signed-off-by: Clark Wang <xiaoning.wang@xxxxxxx>
>>>> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
>>>> Tested-by: Pratik Manvar <pratik.manvar@xxxxxxx>
>>>> ---
>>>> V1 -> V2: fix sparse warnings reported-by: kernel test robot <lkp@xxxxxxxxx>
>>>> Closes: https://lore.kernel.org/oe-kbuild-all/202312300907.RGtYsKxb-lkp@xxxxxxxxx/
>>>>
>>>> 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/20211220105555.zwq22vip7onafrck@xxxxxxxxxxxxxx/
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

Next message: Christophe Leroy: "Re: [PATCH v2 3/3] arch/powerpc: Remove <linux/fb.h> from backlight code"
Previous message: Rafael J. Wysocki: "Re: [PATCH v3] driver core: Cancel scheduled pm_runtime_idle() on device removal"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]