| 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: <ZtoWowx1Vja3yjXc@lizhi-Precision-Tower-5810>
Date: Thu, 5 Sep 2024 16:37:55 -0400
From: Frank Li <Frank.li@....com>
To: Marek Vasut <marex@...x.de>
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 Thu, Sep 05, 2024 at 09:01:06PM +0200, Marek Vasut wrote:
> 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
I will add it at next version. But I found a problem of your method,
especially when period is quite long, for example 2s. Assume FIFO is empty.
[old, old, new] will be written to FIFO, new value will takes 2s-6s to make
new value effect.
The currect method, most time, the new value will effect at next period.
Frank
Powered by blists - more mailing lists