diff mbox series

pwm: imx27: workaround of the pwm output bug when decrease the duty cycle

Message ID 20211208085407.780844-1-xiaoning.wang@nxp.com
State Superseded
Headers show
Series pwm: imx27: workaround of the pwm output bug when decrease the duty cycle | expand

Commit Message

Clark Wang Dec. 8, 2021, 8:54 a.m. UTC 
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(-)

Comments

kernel test robot Dec. 9, 2021, 5:49 p.m. UTC | #1 
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
diff mbox series

Patch

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;