| Message ID | 20240910152455.2425152-1-Frank.Li@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5,1/1] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle | expand |
On 9/10/24 5:24 PM, Frank Li wrote: Hi, purely nitpicking further below, one quick question right below. > 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. You did mention the IPS bus is slow. Do I understand it correctly that the IPS bus write takes about 1us ? Because of the PWM consumes a sample every 2us and we need to write 2 samples to avoid FIFO underrun, then to safely write those 2 samples, we need to be able to write 1 sample per 1 us into the FIFO ? Also, would writing more samples help with such "fast" use cases ? Something like this: if (clkrate > 500000) { // This usleep() could use some further improvement, e.g. calculate // precise delay for the FIFO to get empty based on PWM clkrate usleep(2 * 5); // wait 2us for each of the 4 samples in FIFO and a bit // Now the FIFO is surely empty, write all four FIFO slots writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); } else { // clock rate less than 500 kHz // Do the workaround below if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS ... } > 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 800000000 > duty_cycle # One full high plus 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> > --- > 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 | 67 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 66 insertions(+), 1 deletion(-) > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > index 253afe94c4776..713d368f03931 100644 > --- a/drivers/pwm/pwm-imx27.c > +++ b/drivers/pwm/pwm-imx27.c > @@ -27,6 +27,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) > @@ -234,6 +235,8 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > 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; > > @@ -274,7 +277,69 @@ 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. > + * > + * ─────────────────────┐ > + * PWM OUTPUT │ > + * └───────────────────────── > + * > + * ┌──────────────────────────────────────────────┐ > + * Counter │ XXXXXXXXXXXXXX │ > + * └──────────────────────────────────────────────┘ > + * ▲ ▲ > + * │ │ > + * New SAR Old SAR > + * > + * XXXX Errata happen window > + * > + * If the new SAR value is less than the old one, and the counter is > + * greater than the new SAR value (see above diagram XXXX), the current > + * period will not flip the level. This will result in a pulse with a > + * duty cycle of 100%. > + * > + * Check new sar less than old sar and current counter is in errata Better do 's@\<sar\>@SAR@g' so all the instances of 'sar' in the text are in uppercase. Currently, some are in lowercase and some are in uppercase. > + * 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 IRQ, in uppercase. > + * 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 __raw_writel() to minimize the interval between two writes to > + * the SAR register to increase the fastest pwm frequency supported. PWM, in uppercase. > + * When the PWM period is longer than 2us(or <500KHz), this workaround kHz, kilo lowercase, Hz Hertz uppercase H lowercase z . Also fix in the commit message. > + * can solve this problem. No software workaround is available if PWM > + * period is shorter than IO write. > + */ > + 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 && val < MX3_PWMSR_FIFOAV_2WORDS) { > + val = readl_relaxed(imx->mmio_base + MX3_PWMCNR); I would put the comment below above this conditional statement. > + if ((val + c >= duty_cycles && val < imx->duty_cycle) || > + /* > + * If counter is close to period, controller may roll over > + * when next IO write. > + */ > + 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); The patch looks good, the above are just trivial nitpicks. Thanks !
On Wed, Sep 11, 2024 at 10:26:38PM +0200, Marek Vasut wrote: > On 9/10/24 5:24 PM, Frank Li wrote: > > Hi, > > purely nitpicking further below, one quick question right below. > > > 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. > > You did mention the IPS bus is slow. Do I understand it correctly that the > IPS bus write takes about 1us ? Because of the PWM consumes a sample every > 2us and we need to write 2 samples to avoid FIFO underrun, then to safely > write those 2 samples, we need to be able to write 1 sample per 1 us into > the FIFO ? The above time is just estimated, which variance at difference platform and impact by other IPs. If there are pending write/read from GPIO, PWM write have to wait for GPIO's write finish. It actually depend on IPS bus's loading. <500Khz is very less possiblity that write slower than PWM's consumes. > > Also, would writing more samples help with such "fast" use cases ? > Something like this: > > if (clkrate > 500000) { > // This usleep() could use some further improvement, e.g. calculate > // precise delay for the FIFO to get empty based on PWM clkrate > usleep(2 * 5); // wait 2us for each of the 4 samples in FIFO and a bit > // Now the FIFO is surely empty, write all four FIFO slots > writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); > writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); > writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); > writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); It can help at some possiblity, but still have problem if > 1Mhz, write will always less than consume. If errata happen, only 1 cycle is full high. I think it is quite less impact at such high frequency. We found this problem by observe a screen backlight flick when change ducty_cycle. I think we try fix it after a real user visible impact happen. Put code here can reduce some possiblity at certain freq range, but may miss-leading user the problem fixed when > 500k. Frank > } else { // clock rate less than 500 kHz > // Do the workaround below > if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS > ... > } > > > 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 800000000 > duty_cycle # One full high plus 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> > > --- > > 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 | 67 ++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 66 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > > index 253afe94c4776..713d368f03931 100644 > > --- a/drivers/pwm/pwm-imx27.c > > +++ b/drivers/pwm/pwm-imx27.c > > @@ -27,6 +27,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) > > @@ -234,6 +235,8 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > 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; > > @@ -274,7 +277,69 @@ 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. > > + * > > + * ─────────────────────┐ > > + * PWM OUTPUT │ > > + * └───────────────────────── > > + * > > + * ┌──────────────────────────────────────────────┐ > > + * Counter │ XXXXXXXXXXXXXX │ > > + * └──────────────────────────────────────────────┘ > > + * ▲ ▲ > > + * │ │ > > + * New SAR Old SAR > > + * > > + * XXXX Errata happen window > > + * > > + * If the new SAR value is less than the old one, and the counter is > > + * greater than the new SAR value (see above diagram XXXX), the current > > + * period will not flip the level. This will result in a pulse with a > > + * duty cycle of 100%. > > + * > > + * Check new sar less than old sar and current counter is in errata > > Better do 's@\<sar\>@SAR@g' so all the instances of 'sar' in the text are in > uppercase. Currently, some are in lowercase and some are in uppercase. > > > + * 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 > > IRQ, in uppercase. > > > + * 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 __raw_writel() to minimize the interval between two writes to > > + * the SAR register to increase the fastest pwm frequency supported. > > PWM, in uppercase. > > > + * When the PWM period is longer than 2us(or <500KHz), this workaround > > kHz, kilo lowercase, Hz Hertz uppercase H lowercase z . > > Also fix in the commit message. > > > + * can solve this problem. No software workaround is available if PWM > > + * period is shorter than IO write. > > + */ > > + 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 && val < MX3_PWMSR_FIFOAV_2WORDS) { > > + val = readl_relaxed(imx->mmio_base + MX3_PWMCNR); > > I would put the comment below above this conditional statement. > > > + if ((val + c >= duty_cycles && val < imx->duty_cycle) || > > + /* > > + * If counter is close to period, controller may roll over > > + * when next IO write. > > + */ > > + 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); > The patch looks good, the above are just trivial nitpicks. > > Thanks !
On 9/11/24 11:55 PM, Frank Li wrote: Hi, >>> 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. >> >> You did mention the IPS bus is slow. Do I understand it correctly that the >> IPS bus write takes about 1us ? Because of the PWM consumes a sample every >> 2us and we need to write 2 samples to avoid FIFO underrun, then to safely >> write those 2 samples, we need to be able to write 1 sample per 1 us into >> the FIFO ? > > The above time is just estimated, which variance at difference platform and > impact by other IPs. If there are pending write/read from GPIO, PWM write > have to wait for GPIO's write finish. It actually depend on IPS bus's > loading. > > <500Khz is very less possiblity that write slower than PWM's consumes. > >> >> Also, would writing more samples help with such "fast" use cases ? >> Something like this: >> >> if (clkrate > 500000) { >> // This usleep() could use some further improvement, e.g. calculate >> // precise delay for the FIFO to get empty based on PWM clkrate >> usleep(2 * 5); // wait 2us for each of the 4 samples in FIFO and a bit >> // Now the FIFO is surely empty, write all four FIFO slots >> writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); >> writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); >> writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); >> writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); > > It can help at some possiblity, but still have problem if > 1Mhz, write > will always less than consume. > > If errata happen, only 1 cycle is full high. I think it is quite less > impact at such high frequency. > > We found this problem by observe a screen backlight flick when change > ducty_cycle. I think we try fix it after a real user visible impact happen. Indeed, I observed similar problem. > Put code here can reduce some possiblity at certain freq range, but may > miss-leading user the problem fixed when > 500k. You already have good code comments, maybe expanding one would help clarify this issue cannot really be fully fixed with current hardware. I think the multi-write for > 500 kHz could further improve the patch, but let's wait for input from others, let's see what they think.
diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c index 253afe94c4776..713d368f03931 100644 --- a/drivers/pwm/pwm-imx27.c +++ b/drivers/pwm/pwm-imx27.c @@ -27,6 +27,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) @@ -234,6 +235,8 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, 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; @@ -274,7 +277,69 @@ 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. + * + * ─────────────────────┐ + * PWM OUTPUT │ + * └───────────────────────── + * + * ┌──────────────────────────────────────────────┐ + * Counter │ XXXXXXXXXXXXXX │ + * └──────────────────────────────────────────────┘ + * ▲ ▲ + * │ │ + * New SAR Old SAR + * + * XXXX Errata happen window + * + * If the new SAR value is less than the old one, and the counter is + * greater than the new SAR value (see above diagram XXXX), the current + * period will not flip the level. This will result in a pulse with a + * duty cycle of 100%. + * + * 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 __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. No software workaround is available if PWM + * period is shorter than IO write. + */ + 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 && val < MX3_PWMSR_FIFOAV_2WORDS) { + val = readl_relaxed(imx->mmio_base + MX3_PWMCNR); + if ((val + c >= duty_cycles && val < imx->duty_cycle) || + /* + * If counter is close to period, controller may roll over + * when next IO write. + */ + 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); /*