Message-ID: <1a5114c5-92d1-4f5a-9ad6-616475f3ba56@denx.de>
Date: Thu, 5 Sep 2024 21:01:06 +0200
From: Marek Vasut <marex@...x.de>
To: Frank Li <Frank.li@....com>
Cc: Fabio Estevam <festevam@...il.com>, Uwe Kleine-König
 <ukleinek@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
 Sascha Hauer <s.hauer@...gutronix.de>,
 Pengutronix Kernel Team <kernel@...gutronix.de>,
 Philipp Zabel <p.zabel@...gutronix.de>, linux-pwm@...r.kernel.org,
 devicetree@...r.kernel.org, imx@...ts.linux.dev,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 pratikmanvar09@...il.com, francesco@...cini.it,
 Clark Wang <xiaoning.wang@....com>, Jun Li <jun.li@....com>
Subject: Re: [PATCH v2 3/3] pwm: imx27: workaround of the pwm output bug when
 decrease the duty cycle

On 9/5/24 8:54 PM, Frank Li wrote:
> On Thu, Sep 05, 2024 at 08:26:34PM +0200, Marek Vasut wrote:
>> On 9/5/24 7:12 PM, Fabio Estevam wrote:
>>> Adding Marek.
>>>
>>> On Mon, Jul 15, 2024 at 5:30 PM Frank Li <Frank.Li@....com> 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.
>>
>> Frank, could you submit this patch separately ? The 1/3 and 2/3 are
>> unrelated as far as I can tell ?
>>
>>>> ---
>>>> 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 51 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
>>>> index 253afe94c4776..e12eaaebe3499 100644
>>>> --- a/drivers/pwm/pwm-imx27.c
>>>> +++ b/drivers/pwm/pwm-imx27.c
>>>> @@ -27,6 +27,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)
>>>> @@ -232,8 +233,11 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>>>    {
>>>>           unsigned long period_cycles, duty_cycles, prescale;
>>>>           struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
>>>> +       void __iomem *reg_sar = imx->mmio_base + MX3_PWMSAR;
>>>>           unsigned long long c;
>>>>           unsigned long long clkrate;
>>>> +       unsigned long flags;
>>>> +       int val;
>>>>           int ret;
>>>>           u32 cr;
>>>>
>>>> @@ -274,7 +278,53 @@ 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 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.
>>>> +        */
>>>> +       val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR));
>>>> +       if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS) {
>>>> +               c = clkrate * 1500;
>>>> +               do_div(c, NSEC_PER_SEC);
>>>> +
>>>> +               local_irq_save(flags);
>>>> +               if (state->period >= 2000)
>>>> +                       readl_poll_timeout_atomic(imx->mmio_base + MX3_PWMCNR, val,
>>>> +                                                 period_cycles - val >= c, 0, 10);
>>>> +
>>>> +               val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR));
>>>> +               if (!val)
>>>> +                       writel_relaxed(imx->duty_cycle, reg_sar);
>>>> +               writel_relaxed(duty_cycles, reg_sar);
>>>> +               local_irq_restore(flags);
>>>> +       } else {
>>>> +               writel_relaxed(duty_cycles, reg_sar);
>>>> +       }
>>
>> Why so complicated ? Can't this be simplified to this ?
>>
>> const u32 sar[3] = { old_sar, old_sar, new_sar };
>>
>> val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base +
>> MX3_PWMSR));
>>
>> switch (val) {
>> case MX3_PWMSR_FIFOAV_EMPTY:
>> case MX3_PWMSR_FIFOAV_1WORD:
>>    writesl(duty_cycles, sar, 3);
>>    break;
>> case MX3_PWMSR_FIFOAV_2WORDS:
>>    writesl(duty_cycles, sar + 1, 2);
>>    break;
>> default: // 3 words in FIFO
>>    writel(new_sar, duty_cycles);
>> }
>>
>> The MX3_PWMSR_FIFOAV_EMPTY/MX3_PWMSR_FIFOAV_1WORD case will effectively
>> trigger three consecutive 'str' instructions:
>>
>> 1: str PWMSAR, old_sar
>> 2: str PWMSAR, old_sar
>> 3: str PWMSAR, new_sar
>>
>> If the PWM cycle ends before 1:, then PWM will reload old value, then pick
>> old value from 1:, 2: and then new value from 3: -- the FIFO will never be
>> empty.
>>
>> If the PWM cycle ends after 1:, then PWM will pick up old value from 1:
>> which is now in FIFO, 2: and then new value from 3: -- the FIFO will never
>> be empty.
>>
>> The MX3_PWMSR_FIFOAV_2WORDS and default: case is there to prevent FIFO
>> overflow which may lock up the PWM. In case of MX3_PWMSR_FIFOAV_2WORDS there
>> are two words in the FIFO, write two more, old SAR value and new SAR value.
>> In case of default, there must be at least one free slot in the PWM FIFO
>> because pwm_imx27_wait_fifo_slot() which polled the FIFO for free slot
>> above, so there is no danger of FIFO being empty or FIFO overflow.
>>
>> Maybe this can still be isolated to "if (duty_cycles < imx->duty_cycle)" .
>>
>> What do you think ?
> 
> Reasonable. Let me test it.
Thank you.

I have MX8MN locally, so if you need RB/TB for V3, let me know.

Will I be able to reproduce it on another iMX too? Like MX8MP or MX8MM 
(they are a bit easier to work with for me) ?

Could you include "how to reproduce" in the commit message ? Something 
easy like:
- Write this and that sysfs attribute file with old value
- Write this and that sysfs attribute file with new value
- Observe this on a scope

Powered by blists - more mailing lists