| Message ID | 20231229063013.1786-1-pratikmanvar09@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: imx27: workaround of the pwm output bug | expand |
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
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
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
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;