* [PATCH] pwm: imx27: workaround of the pwm output bug
@ 2023-12-29 6:30 pratikmanvar09
2023-12-30 2:01 ` kernel test robot
2024-01-03 10:34 ` [PATCH] " Francesco Dolcini
0 siblings, 2 replies; 14+ messages in thread
From: pratikmanvar09 @ 2023-12-29 6:30 UTC (permalink / raw)
To: thierry.reding, u.kleine-koenig, shawnguo, s.hauer, kernel,
festevam, linux-imx, linux-pwm, linux-arm-kernel, linux-kernel
Cc: Clark Wang, Jun Li, Pratik Manvar
From: Clark Wang <xiaoning.wang@nxp.com>
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 <jun.li@nxp.com>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
Tested-by: Pratik Manvar <pratik.manvar@ifm.com>
---
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 29a3089c534c..c21b2a9f02ae 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 = cpu_to_le32(imx->duty_cycle);
+ sar_current = 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);
/*
@@ -325,6 +380,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;
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] pwm: imx27: workaround of the pwm output bug
2023-12-29 6:30 [PATCH] pwm: imx27: workaround of the pwm output bug pratikmanvar09
@ 2023-12-30 2:01 ` kernel test robot
2024-01-03 6:34 ` [PATCH v2] " pratikmanvar09
2024-01-03 10:34 ` [PATCH] " Francesco Dolcini
1 sibling, 1 reply; 14+ messages in thread
From: kernel test robot @ 2023-12-30 2:01 UTC (permalink / raw)
To: pratikmanvar09, thierry.reding, u.kleine-koenig, shawnguo,
s.hauer, kernel, festevam, linux-imx, linux-pwm,
linux-arm-kernel, linux-kernel
Cc: oe-kbuild-all, Clark Wang, Jun Li, Pratik Manvar
Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on thierry-reding-pwm/for-next]
[also build test WARNING on linus/master v6.7-rc7 next-20231222]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/pratikmanvar09-gmail-com/pwm-imx27-workaround-of-the-pwm-output-bug/20231229-143435
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
patch link: https://lore.kernel.org/r/20231229063013.1786-1-pratikmanvar09%40gmail.com
patch subject: [PATCH] pwm: imx27: workaround of the pwm output bug
config: arm-randconfig-r133-20231230 (https://download.01.org/0day-ci/archive/20231230/202312300907.RGtYsKxb-lkp@intel.com/config)
compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project 8a4266a626914765c0c69839e8a51be383013c1a)
reproduce: (https://download.01.org/0day-ci/archive/20231230/202312300907.RGtYsKxb-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312300907.RGtYsKxb-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> drivers/pwm/pwm-imx27.c:303:26: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [usertype] sar_last @@ got restricted __le32 [usertype] @@
drivers/pwm/pwm-imx27.c:303:26: sparse: expected unsigned int [usertype] sar_last
drivers/pwm/pwm-imx27.c:303:26: sparse: got restricted __le32 [usertype]
>> drivers/pwm/pwm-imx27.c:304:29: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [usertype] sar_current @@ got restricted __le32 [usertype] @@
drivers/pwm/pwm-imx27.c:304:29: sparse: expected unsigned int [usertype] sar_current
drivers/pwm/pwm-imx27.c:304:29: sparse: got restricted __le32 [usertype]
vim +303 drivers/pwm/pwm-imx27.c
219
220 static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
221 const struct pwm_state *state)
222 {
223 unsigned long period_cycles, duty_cycles, prescale, counter_check, flags;
224 struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
225 void __iomem *reg_sar = imx->mmio_base + MX3_PWMSAR;
226 __force u32 sar_last, sar_current;
227 struct pwm_state cstate;
228 unsigned long long c;
229 unsigned long long clkrate;
230 int ret;
231 u32 cr, timeout = 1000;
232
233 pwm_get_state(pwm, &cstate);
234
235 clkrate = clk_get_rate(imx->clk_per);
236 c = clkrate * state->period;
237
238 do_div(c, NSEC_PER_SEC);
239 period_cycles = c;
240
241 prescale = period_cycles / 0x10000 + 1;
242
243 period_cycles /= prescale;
244 c = clkrate * state->duty_cycle;
245 do_div(c, NSEC_PER_SEC);
246 duty_cycles = c;
247 duty_cycles /= prescale;
248
249 /*
250 * according to imx pwm RM, the real period value should be PERIOD
251 * value in PWMPR plus 2.
252 */
253 if (period_cycles > 2)
254 period_cycles -= 2;
255 else
256 period_cycles = 0;
257
258 /*
259 * Wait for a free FIFO slot if the PWM is already enabled, and flush
260 * the FIFO if the PWM was disabled and is about to be enabled.
261 */
262 if (cstate.enabled) {
263 pwm_imx27_wait_fifo_slot(chip, pwm);
264 } else {
265 ret = pwm_imx27_clk_prepare_enable(imx);
266 if (ret)
267 return ret;
268
269 pwm_imx27_sw_reset(chip);
270 }
271
272 /*
273 * This is a limited workaround. When the SAR FIFO is empty, the new
274 * write value will be directly applied to SAR even the current period
275 * is not over.
276 * If the new SAR value is less than the old one, and the counter is
277 * greater than the new SAR value, the current period will not filp
278 * the level. This will result in a pulse with a duty cycle of 100%.
279 * So, writing the current value of the SAR to SAR here before updating
280 * the new SAR value can avoid this issue.
281 *
282 * Add a spin lock and turn off the interrupt to ensure that the
283 * real-time performance can be guaranteed as much as possible when
284 * operating the following operations.
285 *
286 * 1. Add a threshold of 1.5us. If the time T between the read current
287 * count value CNR and the end of the cycle is less than 1.5us, wait
288 * for T to be longer than 1.5us before updating the SAR register.
289 * This is to avoid the situation that when the first SAR is written,
290 * the current cycle just ends and the SAR FIFO that just be written
291 * is emptied again.
292 *
293 * 2. Use __raw_writel() to minimize the interval between two writes to
294 * the SAR register to increase the fastest pwm frequency supported.
295 *
296 * When the PWM period is longer than 2us(or <500KHz), this workaround
297 * can solve this problem.
298 */
299 if (duty_cycles < imx->duty_cycle) {
300 c = clkrate * 1500;
301 do_div(c, NSEC_PER_SEC);
302 counter_check = c;
> 303 sar_last = cpu_to_le32(imx->duty_cycle);
> 304 sar_current = cpu_to_le32(duty_cycles);
305
306 spin_lock_irqsave(&imx->lock, flags);
307 if (state->period >= 2000) {
308 while ((period_cycles -
309 readl_relaxed(imx->mmio_base + MX3_PWMCNR))
310 < counter_check) {
311 if (!--timeout)
312 break;
313 };
314 }
315 if (!(MX3_PWMSR_FIFOAV &
316 readl_relaxed(imx->mmio_base + MX3_PWMSR)))
317 __raw_writel(sar_last, reg_sar);
318 __raw_writel(sar_current, reg_sar);
319 spin_unlock_irqrestore(&imx->lock, flags);
320 } else
321 writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
322
323 writel(period_cycles, imx->mmio_base + MX3_PWMPR);
324
325 /*
326 * Store the duty cycle for future reference in cases where the
327 * MX3_PWMSAR register can't be read (i.e. when the PWM is disabled).
328 */
329 imx->duty_cycle = duty_cycles;
330
331 cr = MX3_PWMCR_PRESCALER_SET(prescale) |
332 MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
333 FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
334 MX3_PWMCR_DBGEN;
335
336 if (state->polarity == PWM_POLARITY_INVERSED)
337 cr |= FIELD_PREP(MX3_PWMCR_POUTC,
338 MX3_PWMCR_POUTC_INVERTED);
339
340 if (state->enabled)
341 cr |= MX3_PWMCR_EN;
342
343 writel(cr, imx->mmio_base + MX3_PWMCR);
344
345 if (!state->enabled)
346 pwm_imx27_clk_disable_unprepare(imx);
347
348 return 0;
349 }
350
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] pwm: imx27: workaround of the pwm output bug
2023-12-30 2:01 ` kernel test robot
@ 2024-01-03 6:34 ` pratikmanvar09
2024-01-03 11:00 ` Stefan Wahren
2024-02-04 6:36 ` pratikmanvar09
0 siblings, 2 replies; 14+ messages in thread
From: pratikmanvar09 @ 2024-01-03 6:34 UTC (permalink / raw)
To: lkp
Cc: festevam, jun.li, kernel, linux-arm-kernel, linux-imx,
linux-kernel, linux-pwm, oe-kbuild-all, pratik.manvar,
pratikmanvar09, s.hauer, shawnguo, thierry.reding,
u.kleine-koenig, xiaoning.wang
From: Clark Wang <xiaoning.wang@nxp.com>
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 <jun.li@nxp.com>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
Tested-by: Pratik Manvar <pratik.manvar@ifm.com>
---
V1 -> V2: fix sparse warnings reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202312300907.RGtYsKxb-lkp@intel.com/
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));
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;
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] pwm: imx27: workaround of the pwm output bug
2023-12-29 6:30 [PATCH] pwm: imx27: workaround of the pwm output bug pratikmanvar09
2023-12-30 2:01 ` kernel test robot
@ 2024-01-03 10:34 ` Francesco Dolcini
2024-01-03 11:02 ` [PATCH v3] " pratikmanvar09
2024-01-03 12:18 ` [PATCH] " Uwe Kleine-König
1 sibling, 2 replies; 14+ messages in thread
From: Francesco Dolcini @ 2024-01-03 10:34 UTC (permalink / raw)
To: pratikmanvar09
Cc: thierry.reding, u.kleine-koenig, shawnguo, s.hauer, kernel,
festevam, linux-imx, linux-pwm, linux-arm-kernel, linux-kernel,
Clark Wang, Jun Li, Pratik Manvar
On Fri, Dec 29, 2023 at 12:00:07PM +0530, pratikmanvar09@gmail.com wrote:
> From: Clark Wang <xiaoning.wang@nxp.com>
>
> 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 <jun.li@nxp.com>
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
> Tested-by: Pratik Manvar <pratik.manvar@ifm.com>
You need to add your signed-off-by at the end when sending a patch, no
matter if you are the author or not.
Francesco
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] pwm: imx27: workaround of the pwm output bug
2024-01-03 6:34 ` [PATCH v2] " pratikmanvar09
@ 2024-01-03 11:00 ` Stefan Wahren
2024-02-04 6:36 ` pratikmanvar09
1 sibling, 0 replies; 14+ messages in thread
From: Stefan Wahren @ 2024-01-03 11:00 UTC (permalink / raw)
To: pratikmanvar09
Cc: festevam, jun.li, kernel, linux-arm-kernel, linux-imx,
linux-kernel, linux-pwm, oe-kbuild-all, pratik.manvar, s.hauer,
shawnguo, thierry.reding, u.kleine-koenig, xiaoning.wang, lkp
Hi Pratik,
Am 03.01.24 um 07:34 schrieb pratikmanvar09@gmail.com:
> From: Clark Wang <xiaoning.wang@nxp.com>
>
> 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 <jun.li@nxp.com>
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
> Tested-by: Pratik Manvar <pratik.manvar@ifm.com>
> ---
> V1 -> V2: fix sparse warnings reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202312300907.RGtYsKxb-lkp@intel.com/
>
> 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;
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3] pwm: imx27: workaround of the pwm output bug
2024-01-03 10:34 ` [PATCH] " Francesco Dolcini
@ 2024-01-03 11:02 ` pratikmanvar09
2024-01-03 12:20 ` Francesco Dolcini
2024-03-05 9:27 ` pratikmanvar09
2024-01-03 12:18 ` [PATCH] " Uwe Kleine-König
1 sibling, 2 replies; 14+ messages in thread
From: pratikmanvar09 @ 2024-01-03 11:02 UTC (permalink / raw)
To: francesco
Cc: festevam, jun.li, kernel, linux-arm-kernel, linux-imx,
linux-kernel, linux-pwm, oe-kbuild-all, pratik.manvar,
pratikmanvar09, s.hauer, shawnguo, thierry.reding,
u.kleine-koenig, xiaoning.wang
From: Clark Wang <xiaoning.wang@nxp.com>
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 <jun.li@nxp.com>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
Tested-by: Pratik Manvar <pratik.manvar@ifm.com>
Signed-off-by: Pratik Manvar <pratik.manvar@ifm.com>
---
V1 -> V2: fix sparse warnings reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202312300907.RGtYsKxb-lkp@intel.com/
V2 -> V3: Add signed-off-by as suggested by `Francesco Dolcini`
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));
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;
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] pwm: imx27: workaround of the pwm output bug
2024-01-03 10:34 ` [PATCH] " Francesco Dolcini
2024-01-03 11:02 ` [PATCH v3] " pratikmanvar09
@ 2024-01-03 12:18 ` Uwe Kleine-König
1 sibling, 0 replies; 14+ messages in thread
From: Uwe Kleine-König @ 2024-01-03 12:18 UTC (permalink / raw)
To: Francesco Dolcini
Cc: pratikmanvar09, linux-pwm, shawnguo, s.hauer, Pratik Manvar,
linux-kernel, thierry.reding, Clark Wang, linux-imx, kernel,
festevam, linux-arm-kernel, Jun Li
[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]
On Wed, Jan 03, 2024 at 11:34:21AM +0100, Francesco Dolcini wrote:
> On Fri, Dec 29, 2023 at 12:00:07PM +0530, pratikmanvar09@gmail.com wrote:
> > From: Clark Wang <xiaoning.wang@nxp.com>
> >
> > 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 <jun.li@nxp.com>
> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> > Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
> > Tested-by: Pratik Manvar <pratik.manvar@ifm.com>
>
> You need to add your signed-off-by at the end when sending a patch, no
> matter if you are the author or not.
FTR: This also applies to the v2 patch. I discarded both from the pwm
patchwork.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] pwm: imx27: workaround of the pwm output bug
2024-01-03 11:02 ` [PATCH v3] " pratikmanvar09
@ 2024-01-03 12:20 ` Francesco Dolcini
2024-01-24 11:48 ` Stefan Wahren
2024-03-05 9:27 ` pratikmanvar09
1 sibling, 1 reply; 14+ messages in thread
From: Francesco Dolcini @ 2024-01-03 12:20 UTC (permalink / raw)
To: pratikmanvar09
Cc: francesco, festevam, jun.li, kernel, linux-arm-kernel, linux-imx,
linux-kernel, linux-pwm, oe-kbuild-all, pratik.manvar, s.hauer,
shawnguo, thierry.reding, u.kleine-koenig, xiaoning.wang
Hello Pratik,
On Wed, Jan 03, 2024 at 04:32:00PM +0530, pratikmanvar09@gmail.com wrote:
> From: Clark Wang <xiaoning.wang@nxp.com>
>
> 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 <jun.li@nxp.com>
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
> Tested-by: Pratik Manvar <pratik.manvar@ifm.com>
> Signed-off-by: Pratik Manvar <pratik.manvar@ifm.com>
A very similar patch was already send in 2021 [1], did it had review
comments not addressed? Please have a look.
In general please refrain from sending a new patch version every other
day, while every Linux kernel subsystem has different rules and a
difference pace of development, in this specific case sending a v3 just
adding your signed-off-by without allowing a little bit of time to wait
for more feedback is just not sane.
[1] https://lore.kernel.org/all/?q=dfn%3Adrivers%2Fpwm%2Fpwm-imx27.c+AND+b%3A%22Clark+Wang%22
Francesco
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] pwm: imx27: workaround of the pwm output bug
2024-01-03 12:20 ` Francesco Dolcini
@ 2024-01-24 11:48 ` Stefan Wahren
2024-01-25 21:28 ` Uwe Kleine-König
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Wahren @ 2024-01-24 11:48 UTC (permalink / raw)
To: Francesco Dolcini, pratikmanvar09, u.kleine-koenig
Cc: festevam, jun.li, kernel, linux-arm-kernel, linux-imx,
linux-kernel, linux-pwm, oe-kbuild-all, pratik.manvar, s.hauer,
shawnguo, thierry.reding, xiaoning.wang
Hi,
Am 03.01.24 um 13:20 schrieb Francesco Dolcini:
> Hello Pratik,
>
> On Wed, Jan 03, 2024 at 04:32:00PM +0530, pratikmanvar09@gmail.com wrote:
>> From: Clark Wang <xiaoning.wang@nxp.com>
>>
>> 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 <jun.li@nxp.com>
>> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
>> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
>> Tested-by: Pratik Manvar <pratik.manvar@ifm.com>
>> Signed-off-by: Pratik Manvar <pratik.manvar@ifm.com>
> A very similar patch was already send in 2021 [1], did it had review
> comments not addressed? Please have a look.
>
> In general please refrain from sending a new patch version every other
> day, while every Linux kernel subsystem has different rules and a
> difference pace of development, in this specific case sending a v3 just
> adding your signed-off-by without allowing a little bit of time to wait
> for more feedback is just not sane.
>
> [1] https://lore.kernel.org/all/?q=dfn%3Adrivers%2Fpwm%2Fpwm-imx27.c+AND+b%3A%22Clark+Wang%22
thank you, this is very helpful. Unfortunately i don't have the
knowledge and resources to continue this work.
@Uwe It seems that you were able to reproduce this issue. Is it possible
to trigger this via sysfs and some kind of script?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] pwm: imx27: workaround of the pwm output bug
2024-01-24 11:48 ` Stefan Wahren
@ 2024-01-25 21:28 ` Uwe Kleine-König
0 siblings, 0 replies; 14+ messages in thread
From: Uwe Kleine-König @ 2024-01-25 21:28 UTC (permalink / raw)
To: Stefan Wahren
Cc: Francesco Dolcini, pratikmanvar09, linux-pwm, shawnguo, s.hauer,
pratik.manvar, linux-kernel, thierry.reding, xiaoning.wang,
linux-imx, kernel, oe-kbuild-all, festevam, linux-arm-kernel,
jun.li
[-- Attachment #1: Type: text/plain, Size: 2892 bytes --]
Hello Stefan,
On Wed, Jan 24, 2024 at 12:48:21PM +0100, Stefan Wahren wrote:
> Am 03.01.24 um 13:20 schrieb Francesco Dolcini:
> > Hello Pratik,
> >
> > On Wed, Jan 03, 2024 at 04:32:00PM +0530, pratikmanvar09@gmail.com wrote:
> > > From: Clark Wang <xiaoning.wang@nxp.com>
> > >
> > > 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 <jun.li@nxp.com>
> > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> > > Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
> > > Tested-by: Pratik Manvar <pratik.manvar@ifm.com>
> > > Signed-off-by: Pratik Manvar <pratik.manvar@ifm.com>
> > A very similar patch was already send in 2021 [1], did it had review
> > comments not addressed? Please have a look.
> >
> > In general please refrain from sending a new patch version every other
> > day, while every Linux kernel subsystem has different rules and a
> > difference pace of development, in this specific case sending a v3 just
> > adding your signed-off-by without allowing a little bit of time to wait
> > for more feedback is just not sane.
> >
> > [1] https://lore.kernel.org/all/?q=dfn%3Adrivers%2Fpwm%2Fpwm-imx27.c+AND+b%3A%22Clark+Wang%22
> thank you, this is very helpful. Unfortunately i don't have the
> knowledge and resources to continue this work.
>
> @Uwe It seems that you were able to reproduce this issue. Is it possible
> to trigger this via sysfs and some kind of script?
When I read through the linked thread I was surprised to read that I
could reproduce it. My best guess is that I did that using a long period
and an LED connected to the PWM's output to make the effect easily visible.
Something like: Configure for
.duty_cycle = 1s, .period = 5s
and change that to
.duty_cycle = 0s, .period = 5s
(e.g. using sysfs) when the output got high.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] pwm: imx27: workaround of the pwm output bug
2024-01-03 6:34 ` [PATCH v2] " pratikmanvar09
2024-01-03 11:00 ` Stefan Wahren
@ 2024-02-04 6:36 ` pratikmanvar09
2024-02-04 10:02 ` Stefan Wahren
2024-03-05 9:24 ` pratikmanvar09
1 sibling, 2 replies; 14+ messages in thread
From: pratikmanvar09 @ 2024-02-04 6:36 UTC (permalink / raw)
To: wahrenst
Cc: festevam, jun.li, kernel, linux-arm-kernel, linux-imx,
linux-kernel, linux-pwm, lkp, oe-kbuild-all, pratik.manvar,
pratikmanvar09, s.hauer, shawnguo, thierry.reding,
u.kleine-koenig, xiaoning.wang
Hi Stefan,
Thanks for your review.
Please see my reply below inline.
>> From: Clark Wang <xiaoning.wang@nxp.com>
>>
>> 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 <jun.li@nxp.com>
>> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
>> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
>> Tested-by: Pratik Manvar <pratik.manvar@ifm.com>
>> ---
>> V1 -> V2: fix sparse warnings reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202312300907.RGtYsKxb-lkp@intel.com/
>>
>> 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;
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] pwm: imx27: workaround of the pwm output bug
2024-02-04 6:36 ` pratikmanvar09
@ 2024-02-04 10:02 ` Stefan Wahren
2024-03-05 9:24 ` pratikmanvar09
1 sibling, 0 replies; 14+ messages in thread
From: Stefan Wahren @ 2024-02-04 10:02 UTC (permalink / raw)
To: pratikmanvar09
Cc: festevam, jun.li, kernel, linux-arm-kernel, linux-imx,
linux-kernel, linux-pwm, lkp, oe-kbuild-all, pratik.manvar,
s.hauer, shawnguo, thierry.reding, u.kleine-koenig,
xiaoning.wang
Hi Pratik,
Am 04.02.24 um 07:36 schrieb pratikmanvar09@gmail.com:
> Hi Stefan,
>
> Thanks for your review.
> Please see my reply below inline.
>
>>> From: Clark Wang <xiaoning.wang@nxp.com>
>>>
>>> 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 <jun.li@nxp.com>
>>> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
>>> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
>>> Tested-by: Pratik Manvar <pratik.manvar@ifm.com>
>>> ---
>>> V1 -> V2: fix sparse warnings reported-by: kernel test robot <lkp@intel.com>
>>> Closes: https://lore.kernel.org/oe-kbuild-all/202312300907.RGtYsKxb-lkp@intel.com/
>>>
>>> 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@pengutronix.de/
>
>> Best regards
>>> imx->chip.ops = &pwm_imx27_ops;
>>> imx->chip.dev = &pdev->dev;
>>> imx->chip.npwm = 1;
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] pwm: imx27: workaround of the pwm output bug
2024-02-04 6:36 ` pratikmanvar09
2024-02-04 10:02 ` Stefan Wahren
@ 2024-03-05 9:24 ` pratikmanvar09
1 sibling, 0 replies; 14+ messages in thread
From: pratikmanvar09 @ 2024-03-05 9:24 UTC (permalink / raw)
To: wahrenst
Cc: festevam, jun.li, kernel, linux-arm-kernel, linux-imx,
linux-kernel, linux-pwm, lkp, oe-kbuild-all, pratik.manvar,
pratikmanvar09, s.hauer, shawnguo, thierry.reding,
u.kleine-koenig, xiaoning.wang
Hi Stefan,
Sorry for the abysmal delay.
Thanks for your review and suggestions.
>Hi Pratik,
>
>Am 04.02.24 um 07:36 schrieb pratikmanvar09@gmail.com:
>> Hi Stefan,
>>
>> Thanks for your review.
>> Please see my reply below inline.
>>
>>>> From: Clark Wang <xiaoning.wang@nxp.com>
>>>>
>>>> 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@nxp.com>
>>>> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
>>>> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
>>>> Tested-by: Pratik Manvar <pratik.manvar@ifm.com>
>>>> ---
>>>> V1 -> V2: fix sparse warnings reported-by: kernel test robot <lkp@intel.com>
>>>> Closes: https://lore.kernel.org/oe-kbuild-all/202312300907.RGtYsKxb-lkp@intel.com/
>>>>
>>>> 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@pengutronix.de/
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] pwm: imx27: workaround of the pwm output bug
2024-01-03 11:02 ` [PATCH v3] " pratikmanvar09
2024-01-03 12:20 ` Francesco Dolcini
@ 2024-03-05 9:27 ` pratikmanvar09
1 sibling, 0 replies; 14+ messages in thread
From: pratikmanvar09 @ 2024-03-05 9:27 UTC (permalink / raw)
To: francesco
Cc: festevam, jun.li, kernel, linux-arm-kernel, linux-imx,
linux-kernel, linux-pwm, oe-kbuild-all, pratik.manvar,
pratikmanvar09, s.hauer, shawnguo, thierry.reding,
u.kleine-koenig, xiaoning.wang
Hello Francesco,
Sorry for the abysmal delay.
Thanks for your review and suggestions.
>Hello Pratik,
>
>On Wed, Jan 03, 2024 at 04:32:00PM +0530, pratikmanvar09@gmail.com wrote:
>> From: Clark Wang <xiaoning.wang@nxp.com>
>>
>> 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 <jun.li@nxp.com>
>> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
>> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
>> Tested-by: Pratik Manvar <pratik.manvar@ifm.com>
>> Signed-off-by: Pratik Manvar <pratik.manvar@ifm.com>
>
>A very similar patch was already send in 2021 [1], did it had review
>comments not addressed? Please have a look.
>
>In general please refrain from sending a new patch version every other
>day, while every Linux kernel subsystem has different rules and a
>difference pace of development, in this specific case sending a v3 just
>adding your signed-off-by without allowing a little bit of time to wait
>for more feedback is just not sane.
Ok, I will keep this in mind. Thanks!
>
>[1] https://lore.kernel.org/all/?q=dfn%3Adrivers%2Fpwm%2Fpwm-imx27.c+AND+b%3A%22Clark+Wang%22
Ok, I did not check this. I will look into this. Thanks!
Thanks & Regards,
Pratik Manvar
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-03-05 9:27 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-29 6:30 [PATCH] pwm: imx27: workaround of the pwm output bug pratikmanvar09
2023-12-30 2:01 ` kernel test robot
2024-01-03 6:34 ` [PATCH v2] " pratikmanvar09
2024-01-03 11:00 ` Stefan Wahren
2024-02-04 6:36 ` pratikmanvar09
2024-02-04 10:02 ` Stefan Wahren
2024-03-05 9:24 ` pratikmanvar09
2024-01-03 10:34 ` [PATCH] " Francesco Dolcini
2024-01-03 11:02 ` [PATCH v3] " pratikmanvar09
2024-01-03 12:20 ` Francesco Dolcini
2024-01-24 11:48 ` Stefan Wahren
2024-01-25 21:28 ` Uwe Kleine-König
2024-03-05 9:27 ` pratikmanvar09
2024-01-03 12:18 ` [PATCH] " Uwe Kleine-König
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).