diff mbox series

[v2,3/3] pwm: rockchip: Do not start PWMs not already running

Message ID 0acdf3a78f670a2678e03b0bbbb01aa58a11ce9a.1608407584.git.simon@simonsouth.net (mailing list archive)
State New
Headers show
Series pwm: rockchip: Eliminate potential race condition when probing | expand

Commit Message

Simon South Dec. 19, 2020, 8:44 p.m. UTC
Currently the Rockchip PWM driver enables the signal ("bus") clock for
every PWM device it finds during probing, then disables it for any device
that was not already enabled (such as by a bootloader) when the kernel
started.

Instead of starting PWMs unnecessarily, check first to see whether a device
has already been enabled and if not, do not enable its signal clock.

Signed-off-by: Simon South <simon@simonsouth.net>
---
 drivers/pwm/pwm-rockchip.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

Comments

Uwe Kleine-König Dec. 21, 2020, 9:05 a.m. UTC | #1
On Sat, Dec 19, 2020 at 03:44:10PM -0500, Simon South wrote:
> Currently the Rockchip PWM driver enables the signal ("bus") clock for
> every PWM device it finds during probing, then disables it for any device
> that was not already enabled (such as by a bootloader) when the kernel
> started.
> 
> Instead of starting PWMs unnecessarily, check first to see whether a device

"starting PWM" here means enabling their clocks, right? I wouldn't
expect that this has any effect on the output, am I right?

> has already been enabled and if not, do not enable its signal clock.
> 
> Signed-off-by: Simon South <simon@simonsouth.net>
> ---
>  drivers/pwm/pwm-rockchip.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index f286a498b82c..b9faef3e9954 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -327,19 +327,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	ret = clk_prepare_enable(pc->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Can't prepare enable bus clk: %d\n", ret);
> -		return ret;
> -	}
> -
> -	ret = clk_prepare_enable(pc->pclk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);
> -		clk_disable_unprepare(pc->clk);
> -		return ret;
> -	}

Just for my understanding: That you moved clk_prepare_enable(pc->pclk)
further down is not strictly necessary for your change, right?

> -
>  	platform_set_drvdata(pdev, pc);
>  
>  	pc->data = id->data;
> @@ -353,12 +340,23 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  		pc->chip.of_pwm_n_cells = 3;
>  	}
>  
> +	ret = clk_prepare_enable(pc->pclk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);
> +		return ret;
> +	}
> +
>  	/* Keep the PWM clk enabled if the PWM appears to be up and running. */
>  	enable_conf = pc->data->enable_conf;
>  	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
>  	enabled = ((ctrl & enable_conf) == enable_conf);
> -	if (!enabled)
> -		clk_disable(pc->clk);
> +
> +	ret = enabled ? clk_prepare_enable(pc->clk) : clk_prepare(pc->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret);
> +		clk_disable_unprepare(pc->pclk);
> +		return ret;
> +	}

I'm not a big fan of this ?: construct. I'd prefer

	ret = clk_prepare(pc->clk);
	if (ret)
		...

	/* Keep the PWM clk enabled ... */
	enabled = ...
	if (enabled) {
		ret = clk_enable(pc->clk);
		if (ret)
			...
	}

even though it is less compact. A small benefit is that the error
message can be more accurate. (You wrote "Can't prepare bus clk" while
the problem might well be in the enable part, but mentioning "enable"
might also be misleading for the enabled = false case.)

Best regards
Uwe
Simon South Dec. 22, 2020, 4:32 p.m. UTC | #2
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> "starting PWM" here means enabling their clocks, right? I wouldn't
> expect that this has any effect on the output, am I right?

Right, it does not; another misunderstanding on my part. I'll fix the
commit message.

> Just for my understanding: That you moved clk_prepare_enable(pc->pclk)
> further down is not strictly necessary for your change, right?

That's right. My thought was to keep the code that enables this clock
close to the code that relies on it, and to minimize the duration the
clock is enabled.

Would it be better to leave the code where it is?

> I'm not a big fan of this ?: construct...

Neither was I, actually, but I guessed the shorter version would be
preferred. I'm happy to change it back to match what you described.
Robin Murphy Dec. 22, 2020, 5:23 p.m. UTC | #3
On 2020-12-19 20:44, Simon South wrote:
> Currently the Rockchip PWM driver enables the signal ("bus") clock for
> every PWM device it finds during probing, then disables it for any device
> that was not already enabled (such as by a bootloader) when the kernel
> started.
> 
> Instead of starting PWMs unnecessarily, check first to see whether a device
> has already been enabled and if not, do not enable its signal clock.
> 
> Signed-off-by: Simon South <simon@simonsouth.net>
> ---
>   drivers/pwm/pwm-rockchip.c | 28 +++++++++++++---------------
>   1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index f286a498b82c..b9faef3e9954 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -327,19 +327,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>   
> -	ret = clk_prepare_enable(pc->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Can't prepare enable bus clk: %d\n", ret);
> -		return ret;
> -	}
> -
> -	ret = clk_prepare_enable(pc->pclk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);
> -		clk_disable_unprepare(pc->clk);
> -		return ret;
> -	}
> -
>   	platform_set_drvdata(pdev, pc);
>   
>   	pc->data = id->data;
> @@ -353,12 +340,23 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>   		pc->chip.of_pwm_n_cells = 3;
>   	}
>   
> +	ret = clk_prepare_enable(pc->pclk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);
> +		return ret;
> +	}
> +
>   	/* Keep the PWM clk enabled if the PWM appears to be up and running. */
>   	enable_conf = pc->data->enable_conf;
>   	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
>   	enabled = ((ctrl & enable_conf) == enable_conf);
> -	if (!enabled)
> -		clk_disable(pc->clk);
> +
> +	ret = enabled ? clk_prepare_enable(pc->clk) : clk_prepare(pc->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret);

Since you're touching it, I guess it might be a good idea to update this 
message to say "PWM clk" for clarity.

I suspect there might also have been some historical confounding in the 
fact that when there is only one clock (pclk_pwm), it's merely a gate 
whose parent is pclk_bus, which serves all the peripherals in the 
usefully-named PD_BUS domain...

Robin.

> +		clk_disable_unprepare(pc->pclk);
> +		return ret;
> +	}
>   
>   	clk_disable(pc->pclk);
>   
>
Simon South Dec. 22, 2020, 5:43 p.m. UTC | #4
Robin Murphy <robin.murphy@arm.com> writes:
> Since you're touching it, I guess it might be a good idea to update
> this message to say "PWM clk" for clarity.

Sure, I'll be happy to remove another source of confusion.
diff mbox series

Patch

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index f286a498b82c..b9faef3e9954 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -327,19 +327,6 @@  static int rockchip_pwm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = clk_prepare_enable(pc->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "Can't prepare enable bus clk: %d\n", ret);
-		return ret;
-	}
-
-	ret = clk_prepare_enable(pc->pclk);
-	if (ret) {
-		dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);
-		clk_disable_unprepare(pc->clk);
-		return ret;
-	}
-
 	platform_set_drvdata(pdev, pc);
 
 	pc->data = id->data;
@@ -353,12 +340,23 @@  static int rockchip_pwm_probe(struct platform_device *pdev)
 		pc->chip.of_pwm_n_cells = 3;
 	}
 
+	ret = clk_prepare_enable(pc->pclk);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);
+		return ret;
+	}
+
 	/* Keep the PWM clk enabled if the PWM appears to be up and running. */
 	enable_conf = pc->data->enable_conf;
 	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
 	enabled = ((ctrl & enable_conf) == enable_conf);
-	if (!enabled)
-		clk_disable(pc->clk);
+
+	ret = enabled ? clk_prepare_enable(pc->clk) : clk_prepare(pc->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret);
+		clk_disable_unprepare(pc->pclk);
+		return ret;
+	}
 
 	clk_disable(pc->pclk);