* [PATCH] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle
@ 2021-12-08 8:54 Clark Wang
2021-12-09 17:49 ` kernel test robot
0 siblings, 1 reply; 2+ messages in thread
From: Clark Wang @ 2021-12-08 8:54 UTC (permalink / raw)
To: thierry.reding, u.kleine-koenig, lee.jones
Cc: shawnguo, s.hauer, kernel, festevam, linux-imx, linux-pwm,
linux-arm-kernel, linux-kernel
This is a limited workaround for the PWM IP issue.
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>
---
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 ea91a2f81a9f..bd97382622dd 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)
@@ -201,10 +204,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))
@@ -215,13 +218,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);
@@ -262,7 +267,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);
/*
@@ -323,6 +378,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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle
2021-12-08 8:54 [PATCH] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle Clark Wang
@ 2021-12-09 17:49 ` kernel test robot
0 siblings, 0 replies; 2+ messages in thread
From: kernel test robot @ 2021-12-09 17:49 UTC (permalink / raw)
To: Clark Wang, thierry.reding, u.kleine-koenig, lee.jones
Cc: kbuild-all, shawnguo, s.hauer, kernel, festevam, linux-imx,
linux-pwm, linux-arm-kernel
Hi Clark,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on thierry-reding-pwm/for-next]
[also build test WARNING on v5.16-rc4 next-20211208]
[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]
url: https://github.com/0day-ci/linux/commits/Clark-Wang/pwm-imx27-workaround-of-the-pwm-output-bug-when-decrease-the-duty-cycle/20211208-165523
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: riscv-randconfig-s031-20211209 (https://download.01.org/0day-ci/archive/20211210/202112100135.Qng8J561-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 11.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-dirty
# https://github.com/0day-ci/linux/commit/e56186a9b8501a89d138d2072cc63d107fb303a0
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Clark-Wang/pwm-imx27-workaround-of-the-pwm-output-bug-when-decrease-the-duty-cycle/20211208-165523
git checkout e56186a9b8501a89d138d2072cc63d107fb303a0
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=riscv SHELL=/bin/bash drivers/pwm/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/pwm/pwm-imx27.c:301: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:301:26: sparse: expected unsigned int [usertype] sar_last
drivers/pwm/pwm-imx27.c:301:26: sparse: got restricted __le32 [usertype]
>> drivers/pwm/pwm-imx27.c:302: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:302:29: sparse: expected unsigned int [usertype] sar_current
drivers/pwm/pwm-imx27.c:302:29: sparse: got restricted __le32 [usertype]
vim +301 drivers/pwm/pwm-imx27.c
217
218 static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
219 const struct pwm_state *state)
220 {
221 unsigned long period_cycles, duty_cycles, prescale, counter_check, flags;
222 struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
223 void __iomem *reg_sar = imx->mmio_base + MX3_PWMSAR;
224 __force u32 sar_last, sar_current;
225 struct pwm_state cstate;
226 unsigned long long c;
227 unsigned long long clkrate;
228 int ret;
229 u32 cr, timeout = 1000;
230
231 pwm_get_state(pwm, &cstate);
232
233 clkrate = clk_get_rate(imx->clk_per);
234 c = clkrate * state->period;
235
236 do_div(c, NSEC_PER_SEC);
237 period_cycles = c;
238
239 prescale = period_cycles / 0x10000 + 1;
240
241 period_cycles /= prescale;
242 c = clkrate * state->duty_cycle;
243 do_div(c, NSEC_PER_SEC);
244 duty_cycles = c;
245 duty_cycles /= prescale;
246
247 /*
248 * according to imx pwm RM, the real period value should be PERIOD
249 * value in PWMPR plus 2.
250 */
251 if (period_cycles > 2)
252 period_cycles -= 2;
253 else
254 period_cycles = 0;
255
256 /*
257 * Wait for a free FIFO slot if the PWM is already enabled, and flush
258 * the FIFO if the PWM was disabled and is about to be enabled.
259 */
260 if (cstate.enabled) {
261 pwm_imx27_wait_fifo_slot(chip, pwm);
262 } else {
263 ret = pwm_imx27_clk_prepare_enable(imx);
264 if (ret)
265 return ret;
266
267 pwm_imx27_sw_reset(chip);
268 }
269
270 /*
271 * This is a limited workaround. When the SAR FIFO is empty, the new
272 * write value will be directly applied to SAR even the current period
273 * is not over.
274 * If the new SAR value is less than the old one, and the counter is
275 * greater than the new SAR value, the current period will not filp
276 * the level. This will result in a pulse with a duty cycle of 100%.
277 * So, writing the current value of the SAR to SAR here before updating
278 * the new SAR value can avoid this issue.
279 *
280 * Add a spin lock and turn off the interrupt to ensure that the
281 * real-time performance can be guaranteed as much as possible when
282 * operating the following operations.
283 *
284 * 1. Add a threshold of 1.5us. If the time T between the read current
285 * count value CNR and the end of the cycle is less than 1.5us, wait
286 * for T to be longer than 1.5us before updating the SAR register.
287 * This is to avoid the situation that when the first SAR is written,
288 * the current cycle just ends and the SAR FIFO that just be written
289 * is emptied again.
290 *
291 * 2. Use __raw_writel() to minimize the interval between two writes to
292 * the SAR register to increase the fastest pwm frequency supported.
293 *
294 * When the PWM period is longer than 2us(or <500KHz), this workaround
295 * can solve this problem.
296 */
297 if (duty_cycles < imx->duty_cycle) {
298 c = clkrate * 1500;
299 do_div(c, NSEC_PER_SEC);
300 counter_check = c;
> 301 sar_last = cpu_to_le32(imx->duty_cycle);
> 302 sar_current = cpu_to_le32(duty_cycles);
303
304 spin_lock_irqsave(&imx->lock, flags);
305 if (state->period >= 2000) {
306 while ((period_cycles -
307 readl_relaxed(imx->mmio_base + MX3_PWMCNR))
308 < counter_check) {
309 if (!--timeout)
310 break;
311 };
312 }
313 if (!(MX3_PWMSR_FIFOAV &
314 readl_relaxed(imx->mmio_base + MX3_PWMSR)))
315 __raw_writel(sar_last, reg_sar);
316 __raw_writel(sar_current, reg_sar);
317 spin_unlock_irqrestore(&imx->lock, flags);
318 } else
319 writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
320
321 writel(period_cycles, imx->mmio_base + MX3_PWMPR);
322
323 /*
324 * Store the duty cycle for future reference in cases where the
325 * MX3_PWMSAR register can't be read (i.e. when the PWM is disabled).
326 */
327 imx->duty_cycle = duty_cycles;
328
329 cr = MX3_PWMCR_PRESCALER_SET(prescale) |
330 MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
331 FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
332 MX3_PWMCR_DBGEN;
333
334 if (state->polarity == PWM_POLARITY_INVERSED)
335 cr |= FIELD_PREP(MX3_PWMCR_POUTC,
336 MX3_PWMCR_POUTC_INVERTED);
337
338 if (state->enabled)
339 cr |= MX3_PWMCR_EN;
340
341 writel(cr, imx->mmio_base + MX3_PWMCR);
342
343 if (!state->enabled)
344 pwm_imx27_clk_disable_unprepare(imx);
345
346 return 0;
347 }
348
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-12-09 17:58 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-08 8:54 [PATCH] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle Clark Wang
2021-12-09 17:49 ` kernel test robot
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).