From: Stefan Wahren <wahrenst@gmx.net>
To: pratikmanvar09@gmail.com
Cc: festevam@gmail.com, jun.li@nxp.com, kernel@pengutronix.de,
linux-arm-kernel@lists.infradead.org, linux-imx@nxp.com,
linux-kernel@vger.kernel.org, linux-pwm@vger.kernel.org,
lkp@intel.com, oe-kbuild-all@lists.linux.dev,
pratik.manvar@ifm.com, s.hauer@pengutronix.de,
shawnguo@kernel.org, thierry.reding@gmail.com,
u.kleine-koenig@pengutronix.de, xiaoning.wang@nxp.com
Subject: Re: [PATCH v2] pwm: imx27: workaround of the pwm output bug
Date: Sun, 4 Feb 2024 11:02:39 +0100 [thread overview]
Message-ID: <d72d1ae5-0378-4bac-8b77-0bb69f55accd@gmx.net> (raw)
In-Reply-To: <65bf307d.170a0220.4d544.f32b@mx.google.com>
Hi Pratik,
Am 04.02.24 um 07:36 schrieb pratikmanvar09@gmail.com:
> Hi Stefan,
>
> Thanks for your review.
> Please see my reply below inline.
>
>>> From: Clark Wang <xiaoning.wang@nxp.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.
Thanks, i think this ERR... reference is better than TKT... because it's
links to the errata documents and other Freescale/NXP drivers use them
too. So having this code in a comment would be great.
>
>> 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@nxp.com>
>>> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
>>> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317
>>> Tested-by: Pratik Manvar <pratik.manvar@ifm.com>
>>> ---
>>> V1 -> V2: fix sparse warnings reported-by: kernel test robot <lkp@intel.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!
Okay, AFAIR the coding style suggests braces for the else case.
>
>>> 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.
Could you also please look at Uwe's comments [1]?
Thanks
[1] -
https://lore.kernel.org/all/20211220105555.zwq22vip7onafrck@pengutronix.de/
>
>> Best regards
>>> imx->chip.ops = &pwm_imx27_ops;
>>> imx->chip.dev = &pdev->dev;
>>> imx->chip.npwm = 1;
next prev parent reply other threads:[~2024-02-04 10:03 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-29 6:30 [PATCH] pwm: imx27: workaround of the pwm output bug pratikmanvar09
2023-12-30 2:01 ` kernel test robot
2024-01-03 6:34 ` [PATCH v2] " pratikmanvar09
2024-01-03 11:00 ` Stefan Wahren
2024-02-04 6:36 ` pratikmanvar09
2024-02-04 10:02 ` Stefan Wahren [this message]
2024-03-05 9:24 ` pratikmanvar09
2024-01-03 10:34 ` [PATCH] " Francesco Dolcini
2024-01-03 11:02 ` [PATCH v3] " pratikmanvar09
2024-01-03 12:20 ` Francesco Dolcini
2024-01-24 11:48 ` Stefan Wahren
2024-01-25 21:28 ` Uwe Kleine-König
2024-03-05 9:27 ` pratikmanvar09
2024-01-03 12:18 ` [PATCH] " Uwe Kleine-König
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d72d1ae5-0378-4bac-8b77-0bb69f55accd@gmx.net \
--to=wahrenst@gmx.net \
--cc=festevam@gmail.com \
--cc=jun.li@nxp.com \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=lkp@intel.com \
--cc=oe-kbuild-all@lists.linux.dev \
--cc=pratik.manvar@ifm.com \
--cc=pratikmanvar09@gmail.com \
--cc=s.hauer@pengutronix.de \
--cc=shawnguo@kernel.org \
--cc=thierry.reding@gmail.com \
--cc=u.kleine-koenig@pengutronix.de \
--cc=xiaoning.wang@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).