On Thu, Sep 05, 2024 at 09:01:06PM +0200, Marek Vasut wrote: > On 9/5/24 8:54 PM, Frank Li wrote: > > On Thu, Sep 05, 2024 at 08:26:34PM +0200, Marek Vasut wrote: > > > On 9/5/24 7:12 PM, Fabio Estevam wrote: > > > > Adding Marek. > > > > > > > > On Mon, Jul 15, 2024 at 5:30 PM Frank Li <Frank.Li@xxxxxxx> wrote: > > > > > > > > > > From: Clark Wang <xiaoning.wang@xxxxxxx> > > > > > > > > > > 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. > > > > > > Frank, could you submit this patch separately ? The 1/3 and 2/3 are > > > unrelated as far as I can tell ? > > > > > > > > --- > > > > > 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++- > > > > > 1 file changed, 51 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > > > > > index 253afe94c4776..e12eaaebe3499 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) > > > > > @@ -232,8 +233,11 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > > { > > > > > unsigned long period_cycles, duty_cycles, prescale; > > > > > struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip); > > > > > + void __iomem *reg_sar = imx->mmio_base + MX3_PWMSAR; > > > > > unsigned long long c; > > > > > unsigned long long clkrate; > > > > > + unsigned long flags; > > > > > + int val; > > > > > int ret; > > > > > u32 cr; > > > > > > > > > > @@ -274,7 +278,53 @@ 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. > > > > > + */ > > > > > + val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR)); > > > > > + if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS) { > > > > > + c = clkrate * 1500; > > > > > + do_div(c, NSEC_PER_SEC); > > > > > + > > > > > + local_irq_save(flags); > > > > > + if (state->period >= 2000) > > > > > + readl_poll_timeout_atomic(imx->mmio_base + MX3_PWMCNR, val, > > > > > + period_cycles - val >= c, 0, 10); > > > > > + > > > > > + val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR)); > > > > > + if (!val) > > > > > + writel_relaxed(imx->duty_cycle, reg_sar); > > > > > + writel_relaxed(duty_cycles, reg_sar); > > > > > + local_irq_restore(flags); > > > > > + } else { > > > > > + writel_relaxed(duty_cycles, reg_sar); > > > > > + } > > > > > > Why so complicated ? Can't this be simplified to this ? > > > > > > const u32 sar[3] = { old_sar, old_sar, new_sar }; > > > > > > val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + > > > MX3_PWMSR)); > > > > > > switch (val) { > > > case MX3_PWMSR_FIFOAV_EMPTY: > > > case MX3_PWMSR_FIFOAV_1WORD: > > > writesl(duty_cycles, sar, 3); > > > break; > > > case MX3_PWMSR_FIFOAV_2WORDS: > > > writesl(duty_cycles, sar + 1, 2); > > > break; > > > default: // 3 words in FIFO > > > writel(new_sar, duty_cycles); > > > } > > > > > > The MX3_PWMSR_FIFOAV_EMPTY/MX3_PWMSR_FIFOAV_1WORD case will effectively > > > trigger three consecutive 'str' instructions: > > > > > > 1: str PWMSAR, old_sar > > > 2: str PWMSAR, old_sar > > > 3: str PWMSAR, new_sar > > > > > > If the PWM cycle ends before 1:, then PWM will reload old value, then pick > > > old value from 1:, 2: and then new value from 3: -- the FIFO will never be > > > empty. > > > > > > If the PWM cycle ends after 1:, then PWM will pick up old value from 1: > > > which is now in FIFO, 2: and then new value from 3: -- the FIFO will never > > > be empty. > > > > > > The MX3_PWMSR_FIFOAV_2WORDS and default: case is there to prevent FIFO > > > overflow which may lock up the PWM. In case of MX3_PWMSR_FIFOAV_2WORDS there > > > are two words in the FIFO, write two more, old SAR value and new SAR value. > > > In case of default, there must be at least one free slot in the PWM FIFO > > > because pwm_imx27_wait_fifo_slot() which polled the FIFO for free slot > > > above, so there is no danger of FIFO being empty or FIFO overflow. > > > > > > Maybe this can still be isolated to "if (duty_cycles < imx->duty_cycle)" . > > > > > > What do you think ? > > > > Reasonable. Let me test it. > Thank you. > > I have MX8MN locally, so if you need RB/TB for V3, let me know. > > Will I be able to reproduce it on another iMX too? Like MX8MP or MX8MM (they > are a bit easier to work with for me) ? > > Could you include "how to reproduce" in the commit message ? Something easy > like: > - Write this and that sysfs attribute file with old value > - Write this and that sysfs attribute file with new value > - Observe this on a scope I will add it at next version. But I found a problem of your method, especially when period is quite long, for example 2s. Assume FIFO is empty. [old, old, new] will be written to FIFO, new value will takes 2s-6s to make new value effect. The currect method, most time, the new value will effect at next period. Frank
