| 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: <65bf307d.170a0220.4d544.f32b@mx.google.com>
Date: Sun, 4 Feb 2024 12:06:26 +0530
From: pratikmanvar09@...il.com
To: wahrenst@....net
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,
lkp@...el.com,
oe-kbuild-all@...ts.linux.dev,
pratik.manvar@....com,
pratikmanvar09@...il.com,
s.hauer@...gutronix.de,
shawnguo@...nel.org,
thierry.reding@...il.com,
u.kleine-koenig@...gutronix.de,
xiaoning.wang@....com
Subject: Re: [PATCH v2] pwm: imx27: workaround of the pwm output bug
Hi Stefan,
Thanks for your review.
Please see my reply below inline.
>> 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.
[Pratik]: Yes, this is the patch from NXP. Please see original link of the patch https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
>Could you please provide a link to the origin or at least to the
>document which describes TKT0577206?
[Pratik]: Please refer i.MX8MN errata #ERR051198 in https://www.nxp.com/docs/en/errata/IMX8MN_0N14Y.pdf.
>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?
[Pratik]: Yes, looks like this issue is on all platforms which uses this PWM IP.
>> 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.
[Pratik]: Sure, I will look into this. Thanks!
>>
>> 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.
[Pratik]: I used the checkpatch.pl in this patch and that runs without any warnings/errors!
>> 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.
[Pratik]: Right. I will remove this line in next patch version.
>Best regards
>> imx->chip.ops = &pwm_imx27_ops;
>> imx->chip.dev = &pdev->dev;
>> imx->chip.npwm = 1;
Powered by blists - more mailing lists