| Message ID | 20241004193531.673488-1-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v7,1/1] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle | expand |
On Fri, Oct 04, 2024 at 03:35:31PM -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 800000000 > duty_cycle # One full high plus will be seen by scope That should be "pulse" I guess ------------------^^^^ I would have expected a much lower value for the second write to duty_cycle. I guess it depends on the machine you run this on if this hits the race window. > 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 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 | 75 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 74 insertions(+), 1 deletion(-) > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > index 9e2bbf5b4a8ce..00a7189ba46ca 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) > @@ -223,6 +224,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; > > @@ -263,7 +266,77 @@ 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 Hmm, ok, so SAR is the register value that implements the duty cycle setting. And if a new SAR is written, it is directly applied to the hardware and this way it can happen (if SAR_new < counter < SAR_old) that no falling edge happens in the current period. Right? If so, I think the depicted PWM output is misleading. I'd describe and picture it as follows: /* * At each clock tick the hardware compares the SAR value with * the current counter. If they are equal the output is changed * to the inactive level. As a new SAR value is applied * immediately to the currently running period, it can happen * that no falling edge happens in a period and so the output is * active for a whole period. Consider a change from * ________ * / \______/ * ^ * ^ * to * ____ * / \__________/ * ^ ^ * * where SAR is written at the time marked by *. The counter * didn't reach the old (bigger) value because it was changed * before the counter reached that value and when the new value * becomes active it is already lower than the current counter * and so doesn't trigger either while the counter continues to * grow. So the resulting waveform looks as follows: * * ________ ____________________ * / \______/ \__________/ * ^ ^ * ^ ^ * |<-- old SAR -->| |<-- new SAR -->| * * that is the output is active for a whole period. */ > + * > + * 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) { > + if (state->period < 2000) { /* 2000ns = 500 kHz */ > + /* Best effort attempt to fix up >500 kHz case */ > + udelay(6); /* 2us per FIFO entry, 3 FIFO entries written => 6 us */ I don't understand the motivation to wait here. Wouldn't it be better to write the old value 3 - val times and not sleep? Or busy loop until MX3_PWMSR_FIFOAV becomes 0? > + writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR); > + writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR); With the comment above I would have expected __raw_writel here?! > + } 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); I didn't find the time yet to look into your other pwm-imx27 series. Does it conflict with this patch? Which should be applied first? Best regards Uwe
On Fri, Oct 04, 2024 at 10:58:49PM +0200, Uwe Kleine-König wrote: > On Fri, Oct 04, 2024 at 03:35:31PM -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 800000000 > duty_cycle # One full high plus will be seen by scope > > That should be "pulse" I guess ------------------^^^^ Yes, > > I would have expected a much lower value for the second write to > duty_cycle. I guess it depends on the machine you run this on if this > hits the race window. Yes, lower value can increase reproduce rate. I can change to 8000 at next version. > > > 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 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 | 75 ++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 74 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > > index 9e2bbf5b4a8ce..00a7189ba46ca 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) > > @@ -223,6 +224,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; > > > > @@ -263,7 +266,77 @@ 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 > > Hmm, ok, so SAR is the register value that implements the duty cycle > setting. And if a new SAR is written, it is directly applied to the > hardware and this way it can happen (if SAR_new < counter < SAR_old) > that no falling edge happens in the current period. Right? Yes > > If so, I think the depicted PWM output is misleading. I'd describe and > picture it as follows: > > /* > * At each clock tick the hardware compares the SAR value with > * the current counter. If they are equal the output is changed > * to the inactive level. As a new SAR value is applied > * immediately to the currently running period, it can happen > * that no falling edge happens in a period and so the output is > * active for a whole period. Consider a change from > * ________ > * / \______/ > * ^ * ^ > * to > * ____ > * / \__________/ > * ^ ^ > * > * where SAR is written at the time marked by *. The counter > * didn't reach the old (bigger) value because it was changed > * before the counter reached that value and when the new value > * becomes active it is already lower than the current counter > * and so doesn't trigger either while the counter continues to > * grow. So the resulting waveform looks as follows: > * > * ________ ____________________ > * / \______/ \__________/ > * ^ ^ * ^ ^ > * |<-- old SAR -->| |<-- new SAR -->| > * > * that is the output is active for a whole period. > */ Good. > > > + * > > + * 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) { > > + if (state->period < 2000) { /* 2000ns = 500 kHz */ > > + /* Best effort attempt to fix up >500 kHz case */ > > + udelay(6); /* 2us per FIFO entry, 3 FIFO entries written => 6 us */ > > I don't understand the motivation to wait here. Wouldn't it be better to > write the old value 3 - val times and not sleep? Or busy loop until > MX3_PWMSR_FIFOAV becomes 0? It is required by Marek Vasut. Read register is also quite slow. It is hard to hit this branch and can not 100% workaround this problem when period is short. Just choose simplest mathod here. > > > + writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR); > > + writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR); > > With the comment above I would have expected __raw_writel here?! I forget update comment. writel_relaxed() is wrap of __raw_writel(). > > > + } 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); > > I didn't find the time yet to look into your other pwm-imx27 series. > Does it conflict with this patch? Which should be applied first? No conflict, but let's work out this patch first. I think 32k patch may not necessary because driver have not use 32k clock source. It should work without 32k clk. Frank > > Best regards > Uwe
On 10/4/24 10:58 PM, Uwe Kleine-König wrote: [...] >> @@ -263,7 +266,77 @@ 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 > > Hmm, ok, so SAR is the register value that implements the duty cycle > setting. And if a new SAR is written, it is directly applied to the > hardware and this way it can happen (if SAR_new < counter < SAR_old) > that no falling edge happens in the current period. Right? Yes > If so, I think the depicted PWM output is misleading. I'd describe and > picture it as follows: Why not simply duplicate the ERRATA description for iMX8M Nano MX8MN_0N14Y errata sheet ? " 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%. " That is very clear to me. > /* > * At each clock tick the hardware compares the SAR value with > * the current counter. If they are equal the output is changed > * to the inactive level. I would skip this ^ part unless you can surely say the IP works exactly that way because you checked the RTL. > As a new SAR value is applied > * immediately to the currently running period, it can happen > * that no falling edge happens in a period and so the output is > * active for a whole period. Consider a change from > * ________ > * / \______/ > * ^ * ^ > * to > * ____ > * / \__________/ > * ^ ^ > * > * where SAR is written at the time marked by *. The counter > * didn't reach the old (bigger) value because it was changed > * before the counter reached that value and when the new value > * becomes active it is already lower than the current counter > * and so doesn't trigger either while the counter continues to > * grow. So the resulting waveform looks as follows: > * > * ________ ____________________ > * / \______/ \__________/ > * ^ ^ * ^ ^ > * |<-- old SAR -->| |<-- new SAR -->| > * > * that is the output is active for a whole period. The ascii/infographics is nice and would be good to keep, but regarding the description, frankly, the NXP errata description says the same thing in fewer words :) > */ > >> + * >> + * 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) { >> + if (state->period < 2000) { /* 2000ns = 500 kHz */ >> + /* Best effort attempt to fix up >500 kHz case */ >> + udelay(6); /* 2us per FIFO entry, 3 FIFO entries written => 6 us */ > > I don't understand the motivation to wait here. Wouldn't it be better to > write the old value 3 - val times and not sleep? No, because you would overflow the FIFO, see: 137fd45ffec1 ("pwm: imx: Avoid sample FIFO overflow for i.MX PWM version2") > Or busy loop until > MX3_PWMSR_FIFOAV becomes 0? Do we really want a busy wait here if we can avoid it ? We can do udelay(3 * state->period / 1000); so faster PWMs would wait shorter. The delay is here to basically wait until the FIFO is surely empty and has space for 3 consecutive writes (see the commit above wrt. FIFO overflow). [...]
On Sat, Oct 05, 2024 at 02:41:29AM +0200, Marek Vasut wrote: > On 10/4/24 10:58 PM, Uwe Kleine-König wrote: > > [...] > > Why not simply duplicate the ERRATA description for iMX8M Nano MX8MN_0N14Y > errata sheet ? > > " > [...] > " > > That is very clear to me. Fine for me. Frank, do you want to try creating the right mix of the NXP text, your and my description? > > /* > > * At each clock tick the hardware compares the SAR value with > > * the current counter. If they are equal the output is changed > > * to the inactive level. > > I would skip this ^ part unless you can surely say the IP works exactly that > way because you checked the RTL. That it works that way is clear from the errata text IMHO. > > As a new SAR value is applied > > * immediately to the currently running period, it can happen > > * that no falling edge happens in a period and so the output is > > * active for a whole period. Consider a change from > > * ________ > > * / \______/ > > * ^ * ^ > > * to > > * ____ > > * / \__________/ > > * ^ ^ > > * > > * where SAR is written at the time marked by *. The counter > > * didn't reach the old (bigger) value because it was changed > > * before the counter reached that value and when the new value > > * becomes active it is already lower than the current counter > > * and so doesn't trigger either while the counter continues to > > * grow. So the resulting waveform looks as follows: > > * > > * ________ ____________________ > > * / \______/ \__________/ > > * ^ ^ * ^ ^ > > * |<-- old SAR -->| |<-- new SAR -->| > > * > > * that is the output is active for a whole period. > > The ascii/infographics is nice and would be good to keep, but regarding the > description, frankly, the NXP errata description says the same thing in > fewer words :) > > > */ > > > > > + * > > > + * 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) { > > > + if (state->period < 2000) { /* 2000ns = 500 kHz */ > > > + /* Best effort attempt to fix up >500 kHz case */ > > > + udelay(6); /* 2us per FIFO entry, 3 FIFO entries written => 6 us */ > > > > I don't understand the motivation to wait here. Wouldn't it be better to > > write the old value 3 - val times and not sleep? > > No, because you would overflow the FIFO, see: > > 137fd45ffec1 ("pwm: imx: Avoid sample FIFO overflow for i.MX PWM version2") val holds the number of uses FIFO entries, so writing (3 - val) new items should be fine?! > > Or busy loop until > > MX3_PWMSR_FIFOAV becomes 0? > > Do we really want a busy wait here if we can avoid it ? udelay(6) is a busy loop, so we're already there. > We can do udelay(3 * state->period / 1000); so faster PWMs would wait > shorter. state->period is the new value (and you want the old, right?), but otherwise I agree > The delay is here to basically wait until the FIFO is surely empty and has > space for 3 consecutive writes (see the commit above wrt. FIFO overflow). Best regards Uwe
On 10/5/24 5:57 PM, Uwe Kleine-König wrote: > On Sat, Oct 05, 2024 at 02:41:29AM +0200, Marek Vasut wrote: >> On 10/4/24 10:58 PM, Uwe Kleine-König wrote: >> >> [...] >> >> Why not simply duplicate the ERRATA description for iMX8M Nano MX8MN_0N14Y >> errata sheet ? >> >> " >> [...] >> " >> >> That is very clear to me. > > Fine for me. Frank, do you want to try creating the right mix of the NXP > text, your and my description? > >>> /* >>> * At each clock tick the hardware compares the SAR value with >>> * the current counter. If they are equal the output is changed >>> * to the inactive level. >> >> I would skip this ^ part unless you can surely say the IP works exactly that >> way because you checked the RTL. > > That it works that way is clear from the errata text IMHO. The errata description does not say anything about comparing SAR value on each clock tick. Better stick to exactly what the errata does say. [...] >>>> + 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) { >>>> + if (state->period < 2000) { /* 2000ns = 500 kHz */ >>>> + /* Best effort attempt to fix up >500 kHz case */ >>>> + udelay(6); /* 2us per FIFO entry, 3 FIFO entries written => 6 us */ >>> >>> I don't understand the motivation to wait here. Wouldn't it be better to >>> write the old value 3 - val times and not sleep? >> >> No, because you would overflow the FIFO, see: >> >> 137fd45ffec1 ("pwm: imx: Avoid sample FIFO overflow for i.MX PWM version2") > > val holds the number of uses FIFO entries, so writing (3 - val) new > items should be fine?! Not necessarily, consider the case where: - The PWM is very fast - There are currently 3 entries in the FIFO according to driver state - The driver determines 3-val is 1 and performs 1 single write to FIFO => If the PWM consumed the FIFO (FIFO is empty) before the 1 single write arrives, then the aforementioned errata still occurs I believe the better option is to wait until the FIFO is surely depleted and then write three entries in short sequence -- OLD-OLD-NEW -- this way the FIFO would get updated with old value first and then switched to new value, hopefully mitigating the issue as best as possible even for fast PWM settings. btw. the two writes here should be writing the old value twice, now there are three new value writes in this patch version. >>> Or busy loop until >>> MX3_PWMSR_FIFOAV becomes 0? >> >> Do we really want a busy wait here if we can avoid it ? > > udelay(6) is a busy loop, so we're already there. > >> We can do udelay(3 * state->period / 1000); so faster PWMs would wait >> shorter. > > state->period is the new value (and you want the old, right?), but > otherwise I agree Right
diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c index 9e2bbf5b4a8ce..00a7189ba46ca 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) @@ -223,6 +224,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; @@ -263,7 +266,77 @@ 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) { + if (state->period < 2000) { /* 2000ns = 500 kHz */ + /* Best effort attempt to fix up >500 kHz case */ + udelay(6); /* 2us per FIFO entry, 3 FIFO entries written => 6 us */ + writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR); + writel_relaxed(duty_cycles, 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); /*