| Message ID | 20241008194123.1943141-1-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | In Next, archived |
| Headers | show |
| Series | [v8,1/1] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle | expand |
On 10/8/24 9:41 PM, Frank Li wrote: [...] > + c = clkrate * 1500; > + do_div(c, NSEC_PER_SEC); > + > + local_irq_save(flags); > + val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR)); > + > + if (duty_cycles < imx->duty_cycle && (cr & MX3_PWMCR_EN)) { I think you can use state->enabled instead of (cr & MX3_PWMCR_EN).
On Wed, Oct 09, 2024 at 03:55:35AM +0200, Marek Vasut wrote: > On 10/8/24 9:41 PM, Frank Li wrote: > > [...] > > > + c = clkrate * 1500; > > + do_div(c, NSEC_PER_SEC); > > + > > + local_irq_save(flags); > > + val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR)); > > + > > + if (duty_cycles < imx->duty_cycle && (cr & MX3_PWMCR_EN)) { > > I think you can use state->enabled instead of (cr & MX3_PWMCR_EN). state->enabled is new state. Need check old state here. If old state is disable, needn't this workaround at all. Frank
On 10/9/24 5:37 PM, Frank Li wrote: > On Wed, Oct 09, 2024 at 03:55:35AM +0200, Marek Vasut wrote: >> On 10/8/24 9:41 PM, Frank Li wrote: >> >> [...] >> >>> + c = clkrate * 1500; >>> + do_div(c, NSEC_PER_SEC); >>> + >>> + local_irq_save(flags); >>> + val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR)); >>> + >>> + if (duty_cycles < imx->duty_cycle && (cr & MX3_PWMCR_EN)) { >> >> I think you can use state->enabled instead of (cr & MX3_PWMCR_EN). > > state->enabled is new state. Need check old state here. If old state is > disable, needn't this workaround at all. Can you add code comment like this please ? Thank you !
On Tue, Oct 08, 2024 at 03:41:23PM -0400, Frank Li wrote: > From: Clark Wang <xiaoning.wang@nxp.com> > > Implement workaround for ERR051198 > (https://www.nxp.com/docs/en/errata/IMX8MN_0N14Y.pdf) > > PWM output may not function correctly if the FIFO is empty when a new SAR > value is programmed > > Description: > When the PWM FIFO is empty, a new value programmed to the PWM Sample > register (PWM_PWMSAR) will be directly applied even if the current timer > period has not expired. If the new SAMPLE value programmed in the > PWM_PWMSAR register is less than the previous value, and the PWM counter > register (PWM_PWMCNR) that contains the current COUNT value is greater > than the new programmed SAMPLE value, the current period will not flip > the level. This may result in an output pulse with a duty cycle of 100%. > > Workaround: > Program the current SAMPLE value in the PWM_PWMSAR register before > updating the new duty cycle to the SAMPLE value in the PWM_PWMSAR > register. This will ensure that the new SAMPLE value is modified during > a non-empty FIFO, and can be successfully updated after the period > expires. > > Write the old SAR value before updating the new duty cycle to SAR. This > avoids writing the new value into an empty FIFO. > > This only resolves the issue when the PWM period is longer than 2us > (or <500kHz) because write register is not quick enough when PWM period is > very short. > > Reproduce steps: > cd /sys/class/pwm/pwmchip1/pwm0 > echo 2000000000 > period # It is easy to observe by using long period > echo 1000000000 > duty_cycle > echo 1 > enable > echo 8000 > duty_cycle # One full high pulse will be seen by scope > > Fixes: 166091b1894d ("[ARM] MXC: add pwm driver for i.MX SoCs") > Reviewed-by: Jun Li <jun.li@nxp.com> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- Uwe Kleine-König: Do you satisfy for what my merged comments's results and do you have other comments about this workaround? best regards Frank > Chagne from v7 to v8 > - combine Uwe's diagram and errata document. > - use old period > - use udelay(3 * period / 1000) > - Only apply workaround when PWM enabled. > > Change from v6 to v7 > - Add continue write for < 500hz case to try best to workaround this > problem. > > Change from v5 to v6 > - KHz to KHz > - sar to SAR > - move comments above if > > Change from v4 to v5 > - fix typo PMW & If > - using imx->mmio_base + MX3_PWMSAR > > Change from v3 to v4 > - none, wrong bump version number > Change from v2 to v3 > - simple workaround implement. > - add reproduce steps. > > Change from v1 to v2 > - address comments in https://lore.kernel.org/linux-pwm/20211221095053.uz4qbnhdqziftymw@pengutronix.de/ > About disable/enable pwm instead of disable/enable irq: > Some pmw periphal may sensitive to period. Disable/enable pwm will > increase period, althouhg it is okay for most case, such as LED backlight > or FAN speed. But some device such servo may require strict period. > > - address comments in https://lore.kernel.org/linux-pwm/d72d1ae5-0378-4bac-8b77-0bb69f55accd@gmx.net/ > Using official errata number > fix typo 'filp' > add {} for else > > I supposed fixed all previous issues, let me know if I missed one. > --- > drivers/pwm/pwm-imx27.c | 98 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 96 insertions(+), 2 deletions(-) > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > index 9e2bbf5b4a8ce..0375987194318 100644 > --- a/drivers/pwm/pwm-imx27.c > +++ b/drivers/pwm/pwm-imx27.c > @@ -26,6 +26,7 @@ > #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) > @@ -219,10 +220,12 @@ 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, period_us, tmp; > struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip); > unsigned long long c; > unsigned long long clkrate; > + unsigned long flags; > + int val; > int ret; > u32 cr; > > @@ -263,7 +266,98 @@ 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); > + val = readl(imx->mmio_base + MX3_PWMPR); > + val = val >= MX3_PWMPR_MAX ? MX3_PWMPR_MAX : val; > + cr = readl(imx->mmio_base + MX3_PWMCR); > + tmp = NSEC_PER_SEC * (u64)(val + 2) * MX3_PWMCR_PRESCALER_GET(cr); > + tmp = DIV_ROUND_UP_ULL(tmp, clkrate); > + period_us = DIV_ROUND_UP_ULL(tmp, 1000); > + > + /* > + * ERR051198: > + * PWM: PWM output may not function correctly if the FIFO is empty when > + * a new SAR value is programmed > + * > + * Description: > + * When the PWM FIFO is empty, a new value programmed to the PWM Sample > + * register (PWM_PWMSAR) will be directly applied even if the current > + * timer period has not expired. > + * > + * If the new SAMPLE value programmed in the PWM_PWMSAR register is > + * less than the previous value, and the PWM counter register > + * (PWM_PWMCNR) that contains the current COUNT value is greater than > + * the new programmed SAMPLE value, the current period will not flip > + * the level. This may result in an output pulse with a duty cycle of > + * 100%. > + * > + * Consider a change from > + * ________ > + * / \______/ > + * ^ * ^ > + * to > + * ____ > + * / \__________/ > + * ^ ^ > + * At the time marked by *, the new write value will be directly applied > + * to SAR even the current period is not over if FIFO is empty. > + * > + * ________ ____________________ > + * / \______/ \__________/ > + * ^ ^ * ^ ^ > + * |<-- old SAR -->| |<-- new SAR -->| > + * > + * That is the output is active for a whole period. > + * > + * Workaround: > + * Check new SAR less than old SAR and current counter is in errata > + * windows, write extra old SAR into FIFO and new SAR will effect at > + * next period. > + * > + * Sometime period is quite long, such as over 1 second. If add old SAR > + * into FIFO unconditional, new SAR have to wait for next period. It > + * may be too long. > + * > + * Turn off the interrupt to ensure that not IRQ and schedule happen > + * during above operations. If any irq and schedule happen, counter > + * in PWM will be out of data and take wrong action. > + * > + * Add a safety margin 1.5us because it needs some time to complete > + * IO write. > + * > + * Use writel_relaxed() 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. No software workaround is available if PWM > + * period is shorter than IO write. Just try best to fill old data > + * into FIFO. > + */ > + c = clkrate * 1500; > + do_div(c, NSEC_PER_SEC); > + > + local_irq_save(flags); > + val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR)); > + > + if (duty_cycles < imx->duty_cycle && (cr & MX3_PWMCR_EN)) { > + if (period_us < 2) { /* 2us = 500 kHz */ > + /* Best effort attempt to fix up >500 kHz case */ > + udelay(3 * period_us); > + writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); > + writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); > + } else if (val < MX3_PWMSR_FIFOAV_2WORDS) { > + val = readl_relaxed(imx->mmio_base + MX3_PWMCNR); > + /* > + * If counter is close to period, controller may roll over when > + * next IO write. > + */ > + if ((val + c >= duty_cycles && val < imx->duty_cycle) || > + val + c >= period_cycles) > + writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); > + } > + } > + writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR); > + local_irq_restore(flags); > + > writel(period_cycles, imx->mmio_base + MX3_PWMPR); > > /* > -- > 2.34.1 >
Hello Frank, On Wed, Oct 16, 2024 at 12:14:32PM -0400, Frank Li wrote: > Do you satisfy for what my merged comments's results and do you > have other comments about this workaround? I think it's good enough even without the comment that Marek requested and applied it to https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-next as-is. (Although the mathematical pedant inside me shivers when reading things like 2us = 500 kHz :-) .) Best regards Uwe
diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c index 9e2bbf5b4a8ce..0375987194318 100644 --- a/drivers/pwm/pwm-imx27.c +++ b/drivers/pwm/pwm-imx27.c @@ -26,6 +26,7 @@ #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) @@ -219,10 +220,12 @@ 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, period_us, tmp; struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip); unsigned long long c; unsigned long long clkrate; + unsigned long flags; + int val; int ret; u32 cr; @@ -263,7 +266,98 @@ 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); + val = readl(imx->mmio_base + MX3_PWMPR); + val = val >= MX3_PWMPR_MAX ? MX3_PWMPR_MAX : val; + cr = readl(imx->mmio_base + MX3_PWMCR); + tmp = NSEC_PER_SEC * (u64)(val + 2) * MX3_PWMCR_PRESCALER_GET(cr); + tmp = DIV_ROUND_UP_ULL(tmp, clkrate); + period_us = DIV_ROUND_UP_ULL(tmp, 1000); + + /* + * ERR051198: + * PWM: PWM output may not function correctly if the FIFO is empty when + * a new SAR value is programmed + * + * Description: + * When the PWM FIFO is empty, a new value programmed to the PWM Sample + * register (PWM_PWMSAR) will be directly applied even if the current + * timer period has not expired. + * + * If the new SAMPLE value programmed in the PWM_PWMSAR register is + * less than the previous value, and the PWM counter register + * (PWM_PWMCNR) that contains the current COUNT value is greater than + * the new programmed SAMPLE value, the current period will not flip + * the level. This may result in an output pulse with a duty cycle of + * 100%. + * + * Consider a change from + * ________ + * / \______/ + * ^ * ^ + * to + * ____ + * / \__________/ + * ^ ^ + * At the time marked by *, the new write value will be directly applied + * to SAR even the current period is not over if FIFO is empty. + * + * ________ ____________________ + * / \______/ \__________/ + * ^ ^ * ^ ^ + * |<-- old SAR -->| |<-- new SAR -->| + * + * That is the output is active for a whole period. + * + * Workaround: + * Check new SAR less than old SAR and current counter is in errata + * windows, write extra old SAR into FIFO and new SAR will effect at + * next period. + * + * Sometime period is quite long, such as over 1 second. If add old SAR + * into FIFO unconditional, new SAR have to wait for next period. It + * may be too long. + * + * Turn off the interrupt to ensure that not IRQ and schedule happen + * during above operations. If any irq and schedule happen, counter + * in PWM will be out of data and take wrong action. + * + * Add a safety margin 1.5us because it needs some time to complete + * IO write. + * + * Use writel_relaxed() 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. No software workaround is available if PWM + * period is shorter than IO write. Just try best to fill old data + * into FIFO. + */ + c = clkrate * 1500; + do_div(c, NSEC_PER_SEC); + + local_irq_save(flags); + val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR)); + + if (duty_cycles < imx->duty_cycle && (cr & MX3_PWMCR_EN)) { + if (period_us < 2) { /* 2us = 500 kHz */ + /* Best effort attempt to fix up >500 kHz case */ + udelay(3 * period_us); + writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); + writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); + } else if (val < MX3_PWMSR_FIFOAV_2WORDS) { + val = readl_relaxed(imx->mmio_base + MX3_PWMCNR); + /* + * If counter is close to period, controller may roll over when + * next IO write. + */ + if ((val + c >= duty_cycles && val < imx->duty_cycle) || + val + c >= period_cycles) + writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); + } + } + writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR); + local_irq_restore(flags); + writel(period_cycles, imx->mmio_base + MX3_PWMPR); /*