lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zw/maOfa12uoJlO6@lizhi-Precision-Tower-5810>
Date: Wed, 16 Oct 2024 12:14:32 -0400
From: Frank Li <Frank.li@....com>
To: u.kleine-koenig@...libre.com
Cc: conor+dt@...nel.org, devicetree@...r.kernel.org, festevam@...il.com,
	francesco@...cini.it, imx@...ts.linux.dev, jun.li@....com,
	kernel@...gutronix.de, krzk+dt@...nel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-pwm@...r.kernel.org, marex@...x.de, p.zabel@...gutronix.de,
	pratikmanvar09@...il.com, robh@...nel.org, s.hauer@...gutronix.de,
	shawnguo@...nel.org, xiaoning.wang@....com
Subject: Re: [PATCH v8 1/1] pwm: imx27: workaround of the pwm output bug when
 decrease the duty cycle

On Tue, Oct 08, 2024 at 03:41:23PM -0400, Frank Li wrote:
> From: Clark Wang <xiaoning.wang@....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@....com>
> Signed-off-by: Clark Wang <xiaoning.wang@....com>
> Signed-off-by: Frank Li <Frank.Li@....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
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ