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

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(-)

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

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.

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);
>   
>

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.