Re: [PATCH v7 06/12] clk: Always set the rate on clk_set_range_rate

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

On 2/25/22 17:35, Maxime Ripard wrote:
> When we change a clock minimum or maximum using clk_set_rate_range(),
> clk_set_min_rate() or clk_set_max_rate(), the current code will only
> trigger a new rate change if the rate is outside of the new boundaries.
> 
> However, a clock driver might want to always keep the clock rate to
> one of its boundary, for example the minimum to keep the power
> consumption as low as possible.
> 
> Since they don't always get called though, clock providers don't have the
> opportunity to implement this behaviour.
> 
> Let's trigger a clk_set_rate() on the previous requested rate every time
> clk_set_rate_range() is called. That way, providers that care about the
> new boundaries have a chance to adjust the rate, while providers that
> don't care about those new boundaries will return the same rate than
> before, which will be ignored by clk_set_rate() and won't result in a
> new rate change.
> 
> Suggested-by: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/clk/clk.c      | 45 ++++++++++++++++----------------
>  drivers/clk/clk_test.c | 58 +++++++++++++++++++-----------------------
>  2 files changed, 49 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index c15ee5070f52..9bc8bf434b94 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2373,28 +2373,29 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
>  		goto out;
>  	}
>  
> -	rate = clk_core_get_rate_nolock(clk->core);
> -	if (rate < min || rate > max) {
> -		/*
> -		 * FIXME:
> -		 * We are in bit of trouble here, current rate is outside the
> -		 * the requested range. We are going try to request appropriate
> -		 * range boundary but there is a catch. It may fail for the
> -		 * usual reason (clock broken, clock protected, etc) but also
> -		 * because:
> -		 * - round_rate() was not favorable and fell on the wrong
> -		 *   side of the boundary
> -		 * - the determine_rate() callback does not really check for
> -		 *   this corner case when determining the rate
> -		 */
> -
> -		rate = clamp(clk->core->req_rate, min, max);
> -		ret = clk_core_set_rate_nolock(clk->core, rate);
> -		if (ret) {
> -			/* rollback the changes */
> -			clk->min_rate = old_min;
> -			clk->max_rate = old_max;
> -		}
> +	/*
> +	 * Since the boundaries have been changed, let's give the
> +	 * opportunity to the provider to adjust the clock rate based on
> +	 * the new boundaries.
> +	 *
> +	 * We also need to handle the case where the clock is currently
> +	 * outside of the boundaries. Clamping the last requested rate
> +	 * to the current minimum and maximum will also handle this.
> +	 *
> +	 * FIXME:
> +	 * There is a catch. It may fail for the usual reason (clock
> +	 * broken, clock protected, etc) but also because:
> +	 * - round_rate() was not favorable and fell on the wrong
> +	 *   side of the boundary
> +	 * - the determine_rate() callback does not really check for
> +	 *   this corner case when determining the rate
> +	 */
> +	rate = clamp(clk->core->req_rate, min, max);
> +	ret = clk_core_set_rate_nolock(clk->core, rate);
> +	if (ret) {
> +		/* rollback the changes */
> +		clk->min_rate = old_min;
> +		clk->max_rate = old_max;
>  	}
>  
>  out:

Hi,

NVIDIA Tegra30 no longer boots with this change.

You can't assume that rate was requested by clk_set_rate() before
clk_set_rate_range() is called, see what [1] does. T30 memory rate now
drops to min on boot when clk debug range is inited innocuously and CPU
no longer can make any progress because display controller takes out
whole memory bandwidth.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/memory/tegra/tegra30-emc.c#n1437

If clk_set_rate() wasn't ever invoked and req_rate=0, then you must not
change the clk rate if it's within the new range. Please revert this
patch, thanks.