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

[PATCH 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>
---
Clark Wang (1):
      pwm: imx27: workaround of the pwm output bug when decrease the duty cycle

Frank Li (1):
      dt-bindings: pwm: imx: Add compatible string imx8qxp-mipi-pwm

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

 Documentation/devicetree/bindings/pwm/imx-pwm.yaml | 29 ++++++-
 drivers/pwm/pwm-imx27.c                            | 88 ++++++++++++++++++++--
 2 files changed, 106 insertions(+), 11 deletions(-)
---
base-commit: 366b147e98c933f02176e84d73eea4b503bc5362
change-id: 20240708-pwm-5993e602c9b2

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

[PATCH 1/3] dt-bindings: pwm: imx: Add compatible string imx8qxp-mipi-pwm

[Frank Li]

The pwm in imx8qxp mipi subsystem require one extra '32k' clock. So add
compatible string 'imx8qxp-mipi-pwm'. Increase maxItems for clock and
clock-names. Add allOf check to make sure other compatible string keep the
same restriction.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 Documentation/devicetree/bindings/pwm/imx-pwm.yaml | 29 +++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.yaml b/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
index 04148198e34d0..750636985f937 100644
--- a/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
+++ b/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
@@ -9,9 +9,6 @@ title: Freescale i.MX PWM controller
 maintainers:
   - Philipp Zabel <p.zabel@pengutronix.de>
 
-allOf:
-  - $ref: pwm.yaml#
-
 properties:
   "#pwm-cells":
     description:
@@ -42,6 +39,7 @@ properties:
               - fsl,imx8mp-pwm
               - fsl,imx8mq-pwm
               - fsl,imx8qxp-pwm
+              - fsl,imx8qxp-mipi-pwm
           - const: fsl,imx27-pwm
 
   reg:
@@ -51,11 +49,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
@@ -69,6 +71,27 @@ required:
   - clocks
   - clock-names
 
+allOf:
+  - $ref: pwm.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - fsl,imx8qxp-mipi-pwm
+    then:
+      properties:
+        clock:
+          minItems: 3
+        clock-names:
+          minItems: 3
+    else:
+      properties:
+        clock:
+          maxItems: 2
+        clock-name:
+          maxItems: 2
+
 additionalProperties: false
 
 examples:

-- 
2.34.1

Re: [PATCH 1/3] dt-bindings: pwm: imx: Add compatible string imx8qxp-mipi-pwm

[Rob Herring]

On Thu, Jul 11, 2024 at 05:08:56PM -0400, Frank Li wrote:
> The pwm in imx8qxp mipi subsystem require one extra '32k' clock. So add
> compatible string 'imx8qxp-mipi-pwm'. Increase maxItems for clock and
> clock-names. Add allOf check to make sure other compatible string keep the
> same restriction.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  Documentation/devicetree/bindings/pwm/imx-pwm.yaml | 29 +++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.yaml b/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
> index 04148198e34d0..750636985f937 100644
> --- a/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
> +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
> @@ -9,9 +9,6 @@ title: Freescale i.MX PWM controller
>  maintainers:
>    - Philipp Zabel <p.zabel@pengutronix.de>
>  
> -allOf:
> -  - $ref: pwm.yaml#
> -
>  properties:
>    "#pwm-cells":
>      description:
> @@ -42,6 +39,7 @@ properties:
>                - fsl,imx8mp-pwm
>                - fsl,imx8mq-pwm
>                - fsl,imx8qxp-pwm
> +              - fsl,imx8qxp-mipi-pwm

I'm not sure I'd do a new compatible for this. Is the instance really 
different? It's fine if some instances have an optional clock.

Rob

[PATCH 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>
---
 drivers/pwm/pwm-imx27.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 9e2bbf5b4a8ce..032bce7d1fdd3 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,36 @@ static int pwm_imx27_clk_prepare_enable(struct pwm_imx27_chip *imx)
 {
 	int ret;
 
+	if (imx->clk_32k) {
+		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:
+	if (imx->clk_32k)
+		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);
+	if (imx->clk_32k)
+		clk_disable_unprepare(imx->clk_32k);
 }
 
 static int pwm_imx27_get_state(struct pwm_chip *chip,
@@ -223,6 +238,7 @@ 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;
+	int val;
 	int ret;
 	u32 cr;
 
@@ -325,6 +341,13 @@ 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)) {
+		dev_err(&pdev->dev, "getting 32k clock failed with %ld\n",
+				PTR_ERR(imx->clk_32k));
+		return PTR_ERR(imx->clk_32k);
+	}
+
 	chip->ops = &pwm_imx27_ops;
 
 	imx->mmio_base = devm_platform_ioremap_resource(pdev, 0);

-- 
2.34.1

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

[Rob Herring]

On Thu, Jul 11, 2024 at 05:08:57PM -0400, Frank Li wrote:
> 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>
> ---
>  drivers/pwm/pwm-imx27.c | 33 ++++++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 9e2bbf5b4a8ce..032bce7d1fdd3 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,36 @@ static int pwm_imx27_clk_prepare_enable(struct pwm_imx27_chip *imx)
>  {
>  	int ret;
>  
> +	if (imx->clk_32k) {
> +		ret = clk_prepare_enable(imx->clk_32k);

IIRC, no need for the 'if' here. clk_prepare_enable() returns success 
for a NULL clock which is what devm_clk_get_optional() returns if the 
clock doesn't exist.

> +		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:
> +	if (imx->clk_32k)
> +		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);
> +	if (imx->clk_32k)
> +		clk_disable_unprepare(imx->clk_32k);

Same here.

>  }
>  
>  static int pwm_imx27_get_state(struct pwm_chip *chip,
> @@ -223,6 +238,7 @@ 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;
> +	int val;

Goes in next patch.

>  	int ret;
>  	u32 cr;
>  
> @@ -325,6 +341,13 @@ 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)) {
> +		dev_err(&pdev->dev, "getting 32k clock failed with %ld\n",
> +				PTR_ERR(imx->clk_32k));

dev_err_probe.

> +		return PTR_ERR(imx->clk_32k);
> +	}
> +
>  	chip->ops = &pwm_imx27_ops;
>  
>  	imx->mmio_base = devm_platform_ioremap_resource(pdev, 0);
> 
> -- 
> 2.34.1
> 

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

[Frank Li]

From: Clark Wang <xiaoning.wang@nxp.com>

When the SAR FIFO is empty, the write value is directly applied to SAR even
though 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 flip the level. This result in a pulse with a 100% duty
cycle.

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.

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>
---
 drivers/pwm/pwm-imx27.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 52 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 032bce7d1fdd3..b817d73effc93 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -22,11 +22,13 @@
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 
 #define MX3_PWMCR			0x00    /* PWM Control Register */
 #define MX3_PWMSR			0x04    /* PWM Status Register */
 #define MX3_PWMSAR			0x0C    /* PWM Sample Register */
 #define MX3_PWMPR			0x10    /* PWM Period Register */
+#define MX3_PWMCNR			0x14    /* PWM Counter Register */
 
 #define MX3_PWMCR_FWM			GENMASK(27, 26)
 #define MX3_PWMCR_STOPEN		BIT(25)
@@ -92,6 +94,7 @@ struct pwm_imx27_chip {
 	 * value to return in that case.
 	 */
 	unsigned int duty_cycle;
+	spinlock_t lock;
 };
 
 static inline struct pwm_imx27_chip *to_pwm_imx27_chip(struct pwm_chip *chip)
@@ -220,10 +223,10 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
 
 	sr = readl(imx->mmio_base + MX3_PWMSR);
 	fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
-	if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
+	if (fifoav >= MX3_PWMSR_FIFOAV_3WORDS) {
 		period_ms = DIV_ROUND_UP_ULL(pwm->state.period,
 					     NSEC_PER_MSEC);
-		msleep(period_ms);
+		msleep(period_ms * 2);
 
 		sr = readl(imx->mmio_base + MX3_PWMSR);
 		if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr))
@@ -236,8 +239,10 @@ 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;
@@ -279,7 +284,50 @@ 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.
+	 */
+	if (duty_cycles < imx->duty_cycle) {
+		c = clkrate * 1500;
+		do_div(c, NSEC_PER_SEC);
+
+		spin_lock_irqsave(&imx->lock, flags);
+		if (state->period >= 2000)
+			readl_poll_timeout_atomic(imx->mmio_base + MX3_PWMCNR, val,
+						  period_cycles - val >= c, 0, 10);
+
+		if (!(MX3_PWMSR_FIFOAV & readl_relaxed(imx->mmio_base + MX3_PWMSR)))
+			__raw_writel(imx->duty_cycle, reg_sar);
+		__raw_writel(duty_cycles, reg_sar);
+		spin_unlock_irqrestore(&imx->lock, flags);
+	} else
+		writel(duty_cycles, reg_sar);
+
 	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
 
 	/*
@@ -348,6 +396,7 @@ static int pwm_imx27_probe(struct platform_device *pdev)
 		return PTR_ERR(imx->clk_32k);
 	}
 
+	spin_lock_init(&imx->lock);
 	chip->ops = &pwm_imx27_ops;
 
 	imx->mmio_base = devm_platform_ioremap_resource(pdev, 0);

-- 
2.34.1

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

[Stefan Wahren]

Hi Frank,

Am 11.07.24 um 23:08 schrieb Frank Li:
> From: Clark Wang <xiaoning.wang@nxp.com>
>
> When the SAR FIFO is empty, the write value is directly applied to SAR even
> though 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 flip the level. This result in a pulse with a 100% duty
> cycle.
>
> 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.
>
> 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>
the same patch has been submitted from other people in the past and they
received many review comments [1], [2].

Can you please explain which version of the patch this is and does it
address any review comments?

Best regards

[1] -
https://lore.kernel.org/linux-pwm/20211220073130.1429723-1-xiaoning.wang@nxp.com/
[2] -
https://lore.kernel.org/linux-pwm/20231229063013.1786-1-pratikmanvar09@gmail.com/

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

[Frank Li]

On Fri, Jul 12, 2024 at 08:26:17AM +0200, Stefan Wahren wrote:
> Hi Frank,
> 
> Am 11.07.24 um 23:08 schrieb Frank Li:
> > From: Clark Wang <xiaoning.wang@nxp.com>
> > 
> > When the SAR FIFO is empty, the write value is directly applied to SAR even
> > though 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 flip the level. This result in a pulse with a 100% duty
> > cycle.
> > 
> > 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.
> > 
> > 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>
> the same patch has been submitted from other people in the past and they
> received many review comments [1], [2].
> 
> Can you please explain which version of the patch this is and does it
> address any review comments?

Thank, I am not realize someone already submitted before. I fixed some by
common senses. Let me double check to make sure to fix all. 

Frank

> 
> Best regards
> 
> [1] -
> https://lore.kernel.org/linux-pwm/20211220073130.1429723-1-xiaoning.wang@nxp.com/
> [2] -
> https://lore.kernel.org/linux-pwm/20231229063013.1786-1-pratikmanvar09@gmail.com/