| 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: <f42fefb6-2815-4f74-b403-fecd2aa79688@denx.de>
Date: Wed, 11 Sep 2024 22:26:38 +0200
From: Marek Vasut <marex@...x.de>
To: Frank Li <Frank.Li@....com>
Cc: conor+dt@...nel.org, devicetree@...r.kernel.org, festevam@...il.com,
francesco@...cini.it, imx@...ts.linux.dev, jun.li@....com,
kernel@...gutronix.de, krzk+dt@...nel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-pwm@...r.kernel.org, p.zabel@...gutronix.de, pratikmanvar09@...il.com,
robh@...nel.org, s.hauer@...gutronix.de, shawnguo@...nel.org,
ukleinek@...nel.org, xiaoning.wang@....com
Subject: Re: [PATCH v5 1/1] pwm: imx27: workaround of the pwm output bug when
decrease the duty cycle
On 9/10/24 5:24 PM, Frank Li wrote:
Hi,
purely nitpicking further below, one quick question right below.
> This only resolves the issue when the PWM period is longer than 2us
> (or <500KHz) because write register is not quick enough when PWM period is
> very short.
You did mention the IPS bus is slow. Do I understand it correctly that
the IPS bus write takes about 1us ? Because of the PWM consumes a sample
every 2us and we need to write 2 samples to avoid FIFO underrun, then to
safely write those 2 samples, we need to be able to write 1 sample per 1
us into the FIFO ?
Also, would writing more samples help with such "fast" use cases ?
Something like this:
if (clkrate > 500000) {
// This usleep() could use some further improvement, e.g. calculate
// precise delay for the FIFO to get empty based on PWM clkrate
usleep(2 * 5); // wait 2us for each of the 4 samples in FIFO and a bit
// Now the FIFO is surely empty, write all four FIFO slots
writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);
writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);
writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);
writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);
} else { // clock rate less than 500 kHz
// Do the workaround below
if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS
...
}
> Reproduce steps:
> cd /sys/class/pwm/pwmchip1/pwm0
> echo 2000000000 > period # It is easy to observe by using long period
> echo 1000000000 > duty_cycle
> echo 1 > enable
> echo 800000000 > duty_cycle # One full high plus will be seen by scope
>
> Fixes: 166091b1894d ("[ARM] MXC: add pwm driver for i.MX SoCs")
> Reviewed-by: Jun Li <jun.li@....com>
> Signed-off-by: Clark Wang <xiaoning.wang@....com>
> Signed-off-by: Frank Li <Frank.Li@....com>
> ---
> Change from v4 to v5
> - fix typo PMW & If
> - using imx->mmio_base + MX3_PWMSAR
>
> Change from v3 to v4
> - none, wrong bump version number
> Change from v2 to v3
> - simple workaround implement.
> - add reproduce steps.
>
> 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 | 67 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 66 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 253afe94c4776..713d368f03931 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)
> @@ -234,6 +235,8 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
> unsigned long long c;
> unsigned long long clkrate;
> + unsigned long flags;
> + int val;
> int ret;
> u32 cr;
>
> @@ -274,7 +277,69 @@ 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.
> + *
> + * ─────────────────────┐
> + * PWM OUTPUT │
> + * └─────────────────────────
> + *
> + * ┌──────────────────────────────────────────────┐
> + * Counter │ XXXXXXXXXXXXXX │
> + * └──────────────────────────────────────────────┘
> + * ▲ ▲
> + * │ │
> + * New SAR Old SAR
> + *
> + * XXXX Errata happen window
> + *
> + * If the new SAR value is less than the old one, and the counter is
> + * greater than the new SAR value (see above diagram XXXX), the current
> + * period will not flip the level. This will result in a pulse with a
> + * duty cycle of 100%.
> + *
> + * Check new sar less than old sar and current counter is in errata
Better do 's@\<sar\>@SAR@g' so all the instances of 'sar' in the text
are in uppercase. Currently, some are in lowercase and some are in
uppercase.
> + * windows, write extra old sar into FIFO and new sar will effect at
> + * next period.
> + *
> + * Sometime period is quite long, such as over 1 second. If add old sar
> + * into FIFO unconditional, new sar have to wait for next period. It
> + * may be too long.
> + *
> + * Turn off the interrupt to ensure that not irq and schedule happen
IRQ, in uppercase.
> + * during above operations. If any irq and schedule happen, counter
> + * in PWM will be out of data and take wrong action.
> + *
> + * Add a safety margin 1.5us because it needs some time to complete
> + * IO write.
> + *
> + * Use __raw_writel() to minimize the interval between two writes to
> + * the SAR register to increase the fastest pwm frequency supported.
PWM, in uppercase.
> + * When the PWM period is longer than 2us(or <500KHz), this workaround
kHz, kilo lowercase, Hz Hertz uppercase H lowercase z .
Also fix in the commit message.
> + * can solve this problem. No software workaround is available if PWM
> + * period is shorter than IO write.
> + */
> + c = clkrate * 1500;
> + do_div(c, NSEC_PER_SEC);
> +
> + local_irq_save(flags);
> + val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR));
> + if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS) {
> + val = readl_relaxed(imx->mmio_base + MX3_PWMCNR);
I would put the comment below above this conditional statement.
> + if ((val + c >= duty_cycles && val < imx->duty_cycle) ||
> + /*
> + * If counter is close to period, controller may roll over
> + * when next IO write.
> + */
> + val + c >= period_cycles)
> + writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);
> + }
> + writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> + local_irq_restore(flags);
> +
> writel(period_cycles, imx->mmio_base + MX3_PWMPR);
The patch looks good, the above are just trivial nitpicks.
Thanks !
Powered by blists - more mailing lists