[03/10] pwm: sunxi: Yield some time to the pwm-block to become ready

Message ID 1445895161-2317-4-git-send-email-o.schinagl@ultimaker.com
State New
Headers show

Commit Message

Olliver Schinagl Oct. 26, 2015, 9:32 p.m.
The pwm-block of some of the sunxi chips feature a 'ready' flag to
indicate the software that it is ready for new commands.

Right now, when we call pwm_config and set the period, we write the
values to the registers, and turn off the clock to the IP. Because of
this, the hardware does not have time to configure the hardware and set
the 'ready' flag.

By running the clock just before making new changes and before checking
if the hardware is ready, the hardware has time to reconfigure itself
and set the clear the flag appropriately.

Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
---
 drivers/pwm/pwm-sun4i.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

Comments

Thierry Reding Nov. 6, 2015, 4:12 p.m. | #1
On Mon, Oct 26, 2015 at 10:32:34PM +0100, Olliver Schinagl wrote:
> The pwm-block of some of the sunxi chips feature a 'ready' flag to
> indicate the software that it is ready for new commands.
> 
> Right now, when we call pwm_config and set the period, we write the
> values to the registers, and turn off the clock to the IP. Because of
> this, the hardware does not have time to configure the hardware and set
> the 'ready' flag.
> 
> By running the clock just before making new changes and before checking
> if the hardware is ready, the hardware has time to reconfigure itself
> and set the clear the flag appropriately.
> 
> Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
> ---
>  drivers/pwm/pwm-sun4i.c | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)

This looks okay to me (except for one minor thing I noticed, see below),
but I'd like an Acked-by from one of the sunxi people. Maxime, any
comments on this?

> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 58ff424..4d84d9d 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -104,6 +104,22 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	u64 clk_rate, div = 0;
>  	unsigned int prescaler = 0;
>  	int err;
> +	int ret = 0;

Why not reuse err?

Thierry
Chen-Yu Tsai Nov. 6, 2015, 4:34 p.m. | #2
On Tue, Oct 27, 2015 at 5:32 AM, Olliver Schinagl
<o.schinagl@ultimaker.com> wrote:
> The pwm-block of some of the sunxi chips feature a 'ready' flag to
> indicate the software that it is ready for new commands.
>
> Right now, when we call pwm_config and set the period, we write the
> values to the registers, and turn off the clock to the IP. Because of
> this, the hardware does not have time to configure the hardware and set
> the 'ready' flag.
>
> By running the clock just before making new changes and before checking
> if the hardware is ready, the hardware has time to reconfigure itself
> and set the clear the flag appropriately.
>
> Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
> ---
>  drivers/pwm/pwm-sun4i.c | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 58ff424..4d84d9d 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -104,6 +104,22 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>         u64 clk_rate, div = 0;
>         unsigned int prescaler = 0;
>         int err;
> +       int ret = 0;
> +
> +       /* Let the PWM hardware run before making any changes. We do this to
> +        * allow the hardware to have some time to clear the 'ready' flag.
> +        */
> +       err = clk_prepare_enable(sun4i_pwm->clk);
> +       if (err) {
> +               dev_err(chip->dev, "failed to enable PWM clock\n");
> +               return err;
> +       }
> +       spin_lock(&sun4i_pwm->ctrl_lock);
> +       val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> +       clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> +       val |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> +       sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> +       spin_unlock(&sun4i_pwm->ctrl_lock);
>
>         clk_rate = clk_get_rate(sun4i_pwm->clk);
>
> @@ -136,7 +152,9 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>
>                 if (div - 1 > PWM_PRD_MASK) {
>                         dev_err(chip->dev, "period exceeds the maximum value\n");
> -                       return -EINVAL;
> +                       ret = -EINVAL;
> +                       spin_lock(&sun4i_pwm->ctrl_lock);
> +                       goto out;
>                 }
>         }
>
> @@ -145,26 +163,14 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>         do_div(div, period_ns);
>         dty = div;
>
> -       err = clk_prepare_enable(sun4i_pwm->clk);
> -       if (err) {
> -               dev_err(chip->dev, "failed to enable PWM clock\n");
> -               return err;
> -       }
> -
>         spin_lock(&sun4i_pwm->ctrl_lock);
>         val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> -
>         if (sun4i_pwm->data->has_rdy && (val & PWM_RDY(pwm->hwpwm))) {

Instead of moving the code around to try to give the hardware some unspecified
time to run, could we use a tight busy loop with a timeout to read the register
and check if it's been cleared? I think that works better with cpufreq as well.

Thanks.

ChenYu

> -               spin_unlock(&sun4i_pwm->ctrl_lock);
> -               clk_disable_unprepare(sun4i_pwm->clk);
> -               return -EBUSY;
> -       }
> -
> -       clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> -       if (clk_gate) {
> -               val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> -               sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> +               ret = -EBUSY;
> +               goto out;
>         }
> +       val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> +       sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
>
>         val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>         val &= ~BIT_CH(PWM_PRESCAL_MASK, pwm->hwpwm);
> @@ -174,6 +180,7 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>         val = (dty & PWM_DTY_MASK) | PWM_PRD(prd);
>         sun4i_pwm_writel(sun4i_pwm, val, PWM_CH_PRD(pwm->hwpwm));
>
> +out:
>         if (clk_gate) {
>                 val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>                 val |= clk_gate;
> @@ -183,7 +190,7 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>         spin_unlock(&sun4i_pwm->ctrl_lock);
>         clk_disable_unprepare(sun4i_pwm->clk);
>
> -       return 0;
> +       return ret;
>  }
>
>  static int sun4i_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> --
> 2.6.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 58ff424..4d84d9d 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -104,6 +104,22 @@  static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	u64 clk_rate, div = 0;
 	unsigned int prescaler = 0;
 	int err;
+	int ret = 0;
+
+	/* Let the PWM hardware run before making any changes. We do this to
+	 * allow the hardware to have some time to clear the 'ready' flag.
+	 */
+	err = clk_prepare_enable(sun4i_pwm->clk);
+	if (err) {
+		dev_err(chip->dev, "failed to enable PWM clock\n");
+		return err;
+	}
+	spin_lock(&sun4i_pwm->ctrl_lock);
+	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+	clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+	val |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+	spin_unlock(&sun4i_pwm->ctrl_lock);
 
 	clk_rate = clk_get_rate(sun4i_pwm->clk);
 
@@ -136,7 +152,9 @@  static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 		if (div - 1 > PWM_PRD_MASK) {
 			dev_err(chip->dev, "period exceeds the maximum value\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			spin_lock(&sun4i_pwm->ctrl_lock);
+			goto out;
 		}
 	}
 
@@ -145,26 +163,14 @@  static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	do_div(div, period_ns);
 	dty = div;
 
-	err = clk_prepare_enable(sun4i_pwm->clk);
-	if (err) {
-		dev_err(chip->dev, "failed to enable PWM clock\n");
-		return err;
-	}
-
 	spin_lock(&sun4i_pwm->ctrl_lock);
 	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
-
 	if (sun4i_pwm->data->has_rdy && (val & PWM_RDY(pwm->hwpwm))) {
-		spin_unlock(&sun4i_pwm->ctrl_lock);
-		clk_disable_unprepare(sun4i_pwm->clk);
-		return -EBUSY;
-	}
-
-	clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-	if (clk_gate) {
-		val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-		sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+		ret = -EBUSY;
+		goto out;
 	}
+	val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
 
 	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 	val &= ~BIT_CH(PWM_PRESCAL_MASK, pwm->hwpwm);
@@ -174,6 +180,7 @@  static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	val = (dty & PWM_DTY_MASK) | PWM_PRD(prd);
 	sun4i_pwm_writel(sun4i_pwm, val, PWM_CH_PRD(pwm->hwpwm));
 
+out:
 	if (clk_gate) {
 		val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 		val |= clk_gate;
@@ -183,7 +190,7 @@  static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	spin_unlock(&sun4i_pwm->ctrl_lock);
 	clk_disable_unprepare(sun4i_pwm->clk);
 
-	return 0;
+	return ret;
 }
 
 static int sun4i_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,