DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org DDBF560D0C
Date: Wed, 12 Apr 2017 09:46:05 -0700
From: Stephen Boyd <sboyd@codeaurora.org>
To: Peter De Schrijver <pdeschrijver@nvidia.com>
Cc: Michael Turquette <mturquette@baylibre.com>, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] clk: Re-evaluate clock rate on min/max update
Message-ID: <20170412164605.GO7065@codeaurora.org>
References: <1490103807-21821-1-git-send-email-pdeschrijver@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1490103807-21821-1-git-send-email-pdeschrijver@nvidia.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2321
Lines: 73

On 03/21, Peter De Schrijver wrote:
> Whenever a user change its min or max rate limit of a clock, we need to
> re-evaluate the current clock rate and possibly change it if the new limits
> require so. To do this clk_set_rate_range() already calls
> clk_core_set_rate_nolock, however this won't have the intended effect
> because the core clock rate hasn't changed. To fix this, move the test to
> avoid setting the same core clock rate again, to clk_set_rate() so
> clk_core_set_rate_nolock() can change the clock rate when min or max have
> been updated, even when the core clock rate has not changed.

I'd expect some sort of Fixes: tag here? Or it never worked!?

> 
> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>

I seem to recall some problems here around rate aggregation that
we fixed after the patches merged. Sorry, but I have to go back
and look at those conversations to refresh my memory and make
sure this is all fine.

Are you relying on the rate setting op to be called with the new
min/max requirements if the aggregated rate is the same? I don't
understand why clk drivers care.

> ---
>  drivers/clk/clk.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 2fa2fb8..0b815d1 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1569,10 +1569,6 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>  	if (!core)
>  		return 0;
>  
> -	/* bail early if nothing to do */
> -	if (rate == clk_core_get_rate_nolock(core))
> -		return 0;
> -
>  	if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
>  		return -EBUSY;
>  
> @@ -1621,16 +1617,21 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>   */
>  int clk_set_rate(struct clk *clk, unsigned long rate)
>  {
> -	int ret;
> +	int ret = 0;
>  
>  	if (!clk)
> -		return 0;
> +		return ret;

Why? Noise?

>  
>  	/* prevent racing with updates to the clock topology */
>  	clk_prepare_lock();
>  
> +	/* bail early if nothing to do */
> +	if (rate == clk_core_get_rate_nolock(clk->core))
> +		goto out;
> +
>  	ret = clk_core_set_rate_nolock(clk->core, rate);
>  
> +out:
>  	clk_prepare_unlock();
>  

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project