| 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
| ||
|
Message-ID: <9b5f9e1b-645a-4b44-a30c-fe93e7bf3532@gmx.net>
Date: Wed, 3 Jan 2024 12:00:54 +0100
From: Stefan Wahren <wahrenst@....net>
To: pratikmanvar09@...il.com
Cc: festevam@...il.com, jun.li@....com, kernel@...gutronix.de,
linux-arm-kernel@...ts.infradead.org, linux-imx@....com,
linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
oe-kbuild-all@...ts.linux.dev, pratik.manvar@....com,
s.hauer@...gutronix.de, shawnguo@...nel.org, thierry.reding@...il.com,
u.kleine-koenig@...gutronix.de, xiaoning.wang@....com, lkp@...el.com
Subject: Re: [PATCH v2] pwm: imx27: workaround of the pwm output bug
Hi Pratik,
Am 03.01.24 um 07:34 schrieb pratikmanvar09@...il.com:
> From: Clark Wang <xiaoning.wang@....com>
>
> This fixes the pwm output bug when decrease the duty cycle.
> This is a limited workaround for the PWM IP issue TKT0577206.
this looks like a patch from the vendor tree.
Could you please provide a link to the origin or at least to the
document which describes TKT0577206?
As a i.MX6ULL user i couldn't find this issue in the chip errata. So are
you sure that every PWM IP handled by this driver is affected?
>
> Root cause:
> 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
s/filp/flip/ ?
> level. This will result in a pulse with a duty cycle of 100%.
>
> Workaround:
> Add an old value SAR write before updating the new duty cycle to SAR.
> This will keep the new value is always in a not empty fifo, and can be
> wait to update after a period finished.
>
> Limitation:
> This workaround can only solve this issue when the PWM period is longer
> than 2us(or <500KHz).
>
> Reviewed-by: Jun Li <jun.li@....com>
> Signed-off-by: Clark Wang <xiaoning.wang@....com>
> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
> Tested-by: Pratik Manvar <pratik.manvar@....com>
> ---
> V1 -> V2: fix sparse warnings reported-by: kernel test robot <lkp@...el.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202312300907.RGtYsKxb-lkp@intel.com/
>
> drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 62 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 7d9bc43f12b0..1e500a5bf564 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -21,11 +21,13 @@
> #include <linux/platform_device.h>
> #include <linux/pwm.h>
> #include <linux/slab.h>
> +#include <linux/spinlock.h>
>
> #define MX3_PWMCR 0x00 /* PWM Control Register */
> #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)
> @@ -91,6 +93,7 @@ struct pwm_imx27_chip {
> * value to return in that case.
> */
> unsigned int duty_cycle;
> + spinlock_t lock;
> };
>
> #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip)
> @@ -203,10 +206,10 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
>
> sr = readl(imx->mmio_base + MX3_PWMSR);
> fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
> - if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
> + if (fifoav >= MX3_PWMSR_FIFOAV_3WORDS) {
> period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm),
> NSEC_PER_MSEC);
> - msleep(period_ms);
> + msleep(period_ms * (fifoav - 2));
This touches a different workaround ("pwm: imx: Avoid sample FIFO
overflow for i.MX PWM version2") without any explanation.
>
> sr = readl(imx->mmio_base + MX3_PWMSR);
> if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr))
> @@ -217,13 +220,15 @@ 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, counter_check, flags;
> struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
> + void __iomem *reg_sar = imx->mmio_base + MX3_PWMSAR;
> + __force u32 sar_last, sar_current;
> struct pwm_state cstate;
> unsigned long long c;
> unsigned long long clkrate;
> int ret;
> - u32 cr;
> + u32 cr, timeout = 1000;
>
> pwm_get_state(pwm, &cstate);
>
> @@ -264,7 +269,57 @@ 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 same typo as in the commit message.
> + * 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.
> + */
> + if (duty_cycles < imx->duty_cycle) {
> + c = clkrate * 1500;
> + do_div(c, NSEC_PER_SEC);
> + counter_check = c;
> + sar_last = (__force u32) cpu_to_le32(imx->duty_cycle);
> + sar_current = (__force u32) cpu_to_le32(duty_cycles);
> +
> + spin_lock_irqsave(&imx->lock, flags);
> + if (state->period >= 2000) {
> + while ((period_cycles -
> + readl_relaxed(imx->mmio_base + MX3_PWMCNR))
> + < counter_check) {
> + if (!--timeout)
> + break;
> + };
> + }
> + if (!(MX3_PWMSR_FIFOAV &
> + readl_relaxed(imx->mmio_base + MX3_PWMSR)))
> + __raw_writel(sar_last, reg_sar);
> + __raw_writel(sar_current, reg_sar);
> + spin_unlock_irqrestore(&imx->lock, flags);
> + } else
> + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> +
This is hard to believe that checkpatch.pl is fine with this patch.
Please use it before submission.
> writel(period_cycles, imx->mmio_base + MX3_PWMPR);
>
> /*
> @@ -324,6 +379,8 @@ static int pwm_imx27_probe(struct platform_device *pdev)
> return dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_per),
> "failed to get peripheral clock\n");
>
> + spin_lock_init(&imx->lock);
> + imx->duty_cycle = 0;
This line looks unrelated and unnecessary.
Best regards
> imx->chip.ops = &pwm_imx27_ops;
> imx->chip.dev = &pdev->dev;
> imx->chip.npwm = 1;
Powered by blists - more mailing lists