Re: [PATCH v5 1/1] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle
From: Marek Vasut
Date: Wed Sep 11 2024 - 16:34:54 EST
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@xxxxxxx>
Signed-off-by: Clark Wang <xiaoning.wang@xxxxxxx>
Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
---
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@xxxxxxxxxxxxxx/
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@xxxxxxx/
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 !