[PATCH v2 0/3] pwm: imx: add 32k clock for 8qm/qxp and workaround a chip issue

[PATCH v2 0/3] pwm: imx: add 32k clock for 8qm/qxp and workaround a chip issue

[Frank Li]

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Changes in v2:
- See each patch
- Link to v1: https://lore.kernel.org/r/20240711-pwm-v1-0-4d5766f99b8b@nxp.com

---
Clark Wang (1):
      pwm: imx27: workaround of the pwm output bug when decrease the duty cycle

Frank Li (1):
      dt-bindings: pwm: imx: Add optional clock '32k'

Liu Ying (1):
      pwm: imx27: Add 32k clock for pwm in i.MX8QXP MIPI subsystem

 Documentation/devicetree/bindings/pwm/imx-pwm.yaml |  4 ++
 drivers/pwm/pwm-imx27.c                            | 78 ++++++++++++++++++++--
 2 files changed, 76 insertions(+), 6 deletions(-)
---
base-commit: 472fa0e0d7d03574177fc83dc15ad9da15db4ce0
change-id: 20240708-pwm-5993e602c9b2

Best regards,
---
Frank Li <Frank.Li@nxp.com>

[PATCH v2 1/3] dt-bindings: pwm: imx: Add optional clock '32k'

[Frank Li]

The pwm in imx8qxp mipi subsystem require one extra '32k' clock. So
increase maxItems for clock and clock-names.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v1 to v2
- remove compatible string fsl,imx8qxp-mipi-pwm
---
 Documentation/devicetree/bindings/pwm/imx-pwm.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.yaml b/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
index 04148198e34d0..bc6991bd466e1 100644
--- a/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
+++ b/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
@@ -51,11 +51,15 @@ properties:
     items:
       - description: SoC PWM ipg clock
       - description: SoC PWM per clock
+      - description: 32k clock
+    minItems: 2
 
   clock-names:
     items:
       - const: ipg
       - const: per
+      - const: 32k
+    minItems: 2
 
   interrupts:
     maxItems: 1

-- 
2.34.1

[PATCH v2 2/3] pwm: imx27: Add 32k clock for pwm in i.MX8QXP MIPI subsystem

[Frank Li]

From: Liu Ying <victor.liu@nxp.com>

PWM in i.MX8QXP MIPI subsystem needs the clock '32k'. Use it if the DTS
provides that.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v1 to v2
- remove if check for clk
- use dev_err_probe
- remove int val
---
 drivers/pwm/pwm-imx27.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 9e2bbf5b4a8ce..253afe94c4776 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -15,6 +15,7 @@
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -82,6 +83,7 @@
 struct pwm_imx27_chip {
 	struct clk	*clk_ipg;
 	struct clk	*clk_per;
+	struct clk	*clk_32k;
 	void __iomem	*mmio_base;
 
 	/*
@@ -101,23 +103,32 @@ static int pwm_imx27_clk_prepare_enable(struct pwm_imx27_chip *imx)
 {
 	int ret;
 
+	ret = clk_prepare_enable(imx->clk_32k);
+	if (ret)
+		goto err1;
+
 	ret = clk_prepare_enable(imx->clk_ipg);
 	if (ret)
-		return ret;
+		goto err2;
 
 	ret = clk_prepare_enable(imx->clk_per);
-	if (ret) {
-		clk_disable_unprepare(imx->clk_ipg);
-		return ret;
-	}
+	if (ret)
+		goto err3;
 
 	return 0;
+err3:
+	clk_disable_unprepare(imx->clk_ipg);
+err2:
+	clk_disable_unprepare(imx->clk_32k);
+err1:
+	return ret;
 }
 
 static void pwm_imx27_clk_disable_unprepare(struct pwm_imx27_chip *imx)
 {
 	clk_disable_unprepare(imx->clk_per);
 	clk_disable_unprepare(imx->clk_ipg);
+	clk_disable_unprepare(imx->clk_32k);
 }
 
 static int pwm_imx27_get_state(struct pwm_chip *chip,
@@ -325,6 +336,11 @@ 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");
 
+	imx->clk_32k = devm_clk_get_optional(&pdev->dev, "32k");
+	if (IS_ERR(imx->clk_32k))
+		return dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_32k),
+				     "failed to get 32k clock\n");
+
 	chip->ops = &pwm_imx27_ops;
 
 	imx->mmio_base = devm_platform_ioremap_resource(pdev, 0);

-- 
2.34.1

[PATCH v2 3/3] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle

[Frank Li]

From: Clark Wang <xiaoning.wang@nxp.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.

Write the old SAR value before updating the new duty cycle to SAR. This
avoids writing the new value into an empty FIFO.

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.

Fixes: 166091b1894d ("[ARM] MXC: add pwm driver for i.MX SoCs")
Reviewed-by: Jun Li <jun.li@nxp.com>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
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);
+	}
+
 	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
 
 	/*

-- 
2.34.1

Re: [PATCH v2 1/3] dt-bindings: pwm: imx: Add optional clock '32k'

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 283 bytes --]

On Mon, Jul 15, 2024 at 04:29:50PM -0400, Frank Li wrote:
> The pwm in imx8qxp mipi subsystem require one extra '32k' clock. So
> increase maxItems for clock and clock-names.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 2/3] pwm: imx27: Add 32k clock for pwm in i.MX8QXP MIPI subsystem

[Christophe JAILLET]

Le 15/07/2024 à 22:29, Frank Li a écrit :
> From: Liu Ying <victor.liu-3arQi8VN3Tc@public.gmane.org>
> 
> PWM in i.MX8QXP MIPI subsystem needs the clock '32k'. Use it if the DTS
> provides that.
> 
> Signed-off-by: Liu Ying <victor.liu-3arQi8VN3Tc@public.gmane.org>
> Signed-off-by: Frank Li <Frank.Li-3arQi8VN3Tc@public.gmane.org>
> ---
> Change from v1 to v2
> - remove if check for clk
> - use dev_err_probe
> - remove int val

Hi,

maybe, switching to clk_bulk_get_all() and the clk_bulk API would 
simplify the code?

See [1] for a similar discussion related to devicetrees and binding if 
such a change is done.
I'm not familiar with it, so I just point it out.

Just my 2c.

CJ

[1]: 
https://lore.kernel.org/all/65b422fe-ecc1-4f57-bb72-f2fef3a5af28@collabora.com/

> ---
>   drivers/pwm/pwm-imx27.c | 26 +++++++++++++++++++++-----
>   1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 9e2bbf5b4a8ce..253afe94c4776 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -15,6 +15,7 @@
>   #include <linux/delay.h>
>   #include <linux/err.h>
>   #include <linux/io.h>
> +#include <linux/iopoll.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/of.h>
> @@ -82,6 +83,7 @@
>   struct pwm_imx27_chip {
>   	struct clk	*clk_ipg;
>   	struct clk	*clk_per;
> +	struct clk	*clk_32k;
>   	void __iomem	*mmio_base;
>   
>   	/*
> @@ -101,23 +103,32 @@ static int pwm_imx27_clk_prepare_enable(struct pwm_imx27_chip *imx)
>   {
>   	int ret;
>   
> +	ret = clk_prepare_enable(imx->clk_32k);
> +	if (ret)
> +		goto err1;
> +
>   	ret = clk_prepare_enable(imx->clk_ipg);
>   	if (ret)
> -		return ret;
> +		goto err2;
>   
>   	ret = clk_prepare_enable(imx->clk_per);
> -	if (ret) {
> -		clk_disable_unprepare(imx->clk_ipg);
> -		return ret;
> -	}
> +	if (ret)
> +		goto err3;
>   
>   	return 0;
> +err3:
> +	clk_disable_unprepare(imx->clk_ipg);
> +err2:
> +	clk_disable_unprepare(imx->clk_32k);
> +err1:
> +	return ret;
>   }
>   
>   static void pwm_imx27_clk_disable_unprepare(struct pwm_imx27_chip *imx)
>   {
>   	clk_disable_unprepare(imx->clk_per);
>   	clk_disable_unprepare(imx->clk_ipg);
> +	clk_disable_unprepare(imx->clk_32k);
>   }
>   
>   static int pwm_imx27_get_state(struct pwm_chip *chip,
> @@ -325,6 +336,11 @@ 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");
>   
> +	imx->clk_32k = devm_clk_get_optional(&pdev->dev, "32k");
> +	if (IS_ERR(imx->clk_32k))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_32k),
> +				     "failed to get 32k clock\n");
> +
>   	chip->ops = &pwm_imx27_ops;
>   
>   	imx->mmio_base = devm_platform_ioremap_resource(pdev, 0);
> 

Re: [PATCH v2 3/3] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle

[Fabio Estevam]

Adding Marek.

On Mon, Jul 15, 2024 at 5:30 PM Frank Li <Frank.Li@nxp.com> wrote:
>
> From: Clark Wang <xiaoning.wang@nxp.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.
>
> Write the old SAR value before updating the new duty cycle to SAR. This
> avoids writing the new value into an empty FIFO.
>
> 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.
>
> Fixes: 166091b1894d ("[ARM] MXC: add pwm driver for i.MX SoCs")
> Reviewed-by: Jun Li <jun.li@nxp.com>
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> 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);
> +       }
> +
>         writel(period_cycles, imx->mmio_base + MX3_PWMPR);
>
>         /*
>
> --
> 2.34.1
>

Re: [PATCH v2 3/3] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle

[Marek Vasut]

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@nxp.com> wrote:
>>
>> From: Clark Wang <xiaoning.wang@nxp.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 ?

>>          writel(period_cycles, imx->mmio_base + MX3_PWMPR);

Re: [PATCH v2 3/3] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle

[Frank Li]

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@nxp.com> wrote:
> > >
> > > From: Clark Wang <xiaoning.wang@nxp.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.

Frank

>
> > >          writel(period_cycles, imx->mmio_base + MX3_PWMPR);

Re: [PATCH v2 3/3] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle

[Marek Vasut]

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@nxp.com> wrote:
>>>>
>>>> From: Clark Wang <xiaoning.wang@nxp.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

Re: [PATCH v2 3/3] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle

[Frank Li]

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@nxp.com> wrote:
> > > > >
> > > > > From: Clark Wang <xiaoning.wang@nxp.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

Re: [PATCH v2 3/3] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle

[Marek Vasut]

On 9/5/24 10:37 PM, Frank Li wrote:
> 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@nxp.com> wrote:
>>>>>>
>>>>>> From: Clark Wang <xiaoning.wang@nxp.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.

You're right, for long PWM periods, the application of changes will take 
longer.

As far as I can tell, the method implemented in this patch may still 
suffer from the problem in case of short PWM periods, is that correct ? 
I think the writesl() might help cover some of that.

Maybe for longer PWM periods (like 500ms and longer?) where we can be 
sure the FIFO won't quickly consume the written data and underflow, we 
can do writesl() with only two longwords {old_sar, new_sar}, first 
longword to make sure the FIFO is not empty, second word with the new 
PWMSAR value ? That could speed the update up ?

> The currect method, most time, the new value will effect at next period.
Yes, right, I think we may have to handle the longer PWM periods somehow 
differently.

I would like to avoid local_irq_save()/readl_poll_timeout_atomic() if 
possible and let the hardware help avoid this, which I think is possible 
with creative loading of the FIFO with data, hence the writesl() idea.

What do you think about the option above ?

My usecase is mainly display PWM backlight dimming, so those periods 
there are in some 100..2000 Hz or so.

Re: [PATCH v2 3/3] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle

[Frank Li]

On Fri, Sep 06, 2024 at 06:33:45AM +0200, Marek Vasut wrote:
> On 9/5/24 10:37 PM, Frank Li wrote:
> > 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@nxp.com> wrote:
> > > > > > >
> > > > > > > From: Clark Wang <xiaoning.wang@nxp.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.
>
> You're right, for long PWM periods, the application of changes will take
> longer.
>
> As far as I can tell, the method implemented in this patch may still suffer
> from the problem in case of short PWM periods, is that correct ? I think the
> writesl() might help cover some of that.

No way can fix very short periods problem because period was shorter then
register write speed. The register write go through ips bus, which is
quit slow. writesl is equal to writel_relaxed and avoid a dmb(). This will
be over1M hz and user generally use pwm around hz to khz.

>
> Maybe for longer PWM periods (like 500ms and longer?) where we can be sure
> the FIFO won't quickly consume the written data and underflow, we can do
> writesl() with only two longwords {old_sar, new_sar}, first longword to make
> sure the FIFO is not empty, second word with the new PWMSAR value ? That
> could speed the update up ?

We should use this patch's method to read MX3_PWMCNR, if close enough,
write two data into fifo instead of wait for new peroid start.

>
> > The currect method, most time, the new value will effect at next period.
> Yes, right, I think we may have to handle the longer PWM periods somehow
> differently.
>
> I would like to avoid local_irq_save()/readl_poll_timeout_atomic() if
> possible and let the hardware help avoid this, which I think is possible
> with creative loading of the FIFO with data, hence the writesl() idea.

I think it hard to avoid local_irq_save() even use writesl(), writesl is
not atomic,  if irq happen after first raw_write,  FIFO may be empty at
next write.

actually, here have problem

	val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR));
	^^^ if irq happen here, schedule happen, when schedule back,

	FIFOAV value in hardware may less than MX3_PWMSR_FIFOAV_2WORDS, but
	previous read it bigger than MX3_PWMSR_FIFOAV_2WORDS, the below check
	will be false and skip workaround.

	if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS) {


See patch v4
https://lore.kernel.org/imx/20240909193855.2394830-1-Frank.Li@nxp.com/T/#u
It should be little bit better.

Frank

>
> What do you think about the option above ?
>
> My usecase is mainly display PWM backlight dimming, so those periods there
> are in some 100..2000 Hz or so.

Re: [PATCH v2 3/3] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle

[Marek Vasut]

On 9/9/24 9:41 PM, Frank Li wrote:

Hi,

[...]

>>> 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.
>>
>> You're right, for long PWM periods, the application of changes will take
>> longer.
>>
>> As far as I can tell, the method implemented in this patch may still suffer
>> from the problem in case of short PWM periods, is that correct ? I think the
>> writesl() might help cover some of that.
> 
> No way can fix very short periods problem because period was shorter then
> register write speed.

What kind of period are we talking about here ? Since the FIFO has 4 
slots, I would expect way 4-8 MHz to be fixable still ?

> The register write go through ips bus, which is
> quit slow. writesl is equal to writel_relaxed and avoid a dmb(). This will
> be over1M hz and user generally use pwm around hz to khz.

I just had a look at the implementation of writel_relaxed() and yes, 
this is basically a 'str', good point.

>> Maybe for longer PWM periods (like 500ms and longer?) where we can be sure
>> the FIFO won't quickly consume the written data and underflow, we can do
>> writesl() with only two longwords {old_sar, new_sar}, first longword to make
>> sure the FIFO is not empty, second word with the new PWMSAR value ? That
>> could speed the update up ?
> 
> We should use this patch's method to read MX3_PWMCNR, if close enough,
> write two data into fifo instead of wait for new peroid start.

Can we guarantee that there would never be any scheduling or other delay 
between the readout of PWMCNR and write of the FIFO, one which would 
trigger an underflow ?

>>> The currect method, most time, the new value will effect at next period.
>> Yes, right, I think we may have to handle the longer PWM periods somehow
>> differently.
>>
>> I would like to avoid local_irq_save()/readl_poll_timeout_atomic() if
>> possible and let the hardware help avoid this, which I think is possible
>> with creative loading of the FIFO with data, hence the writesl() idea.
> 
> I think it hard to avoid local_irq_save() even use writesl(), writesl is
> not atomic,  if irq happen after first raw_write,  FIFO may be empty at
> next write.
> 
> actually, here have problem
> 
> 	val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR));
> 	^^^ if irq happen here, schedule happen, when schedule back,
> 
> 	FIFOAV value in hardware may less than MX3_PWMSR_FIFOAV_2WORDS, but
> 	previous read it bigger than MX3_PWMSR_FIFOAV_2WORDS, the below check
> 	will be false and skip workaround.
> 
> 	if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS) {
> 
> 
> See patch v4
> https://lore.kernel.org/imx/20240909193855.2394830-1-Frank.Li@nxp.com/T/#u
> It should be little bit better.

I think the v4 is indeed better, thanks.