Re: [PATCH 1/2] cpufreq: arm_big_little: check if the frequency is set correctly

From: Sudeep Holla <sudeep.holla@arm.com>
To: Michael Turquette <mike.turquette@linaro.org>,
	Viresh Kumar <viresh.kumar@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>
Subject: Re: [PATCH 1/2] cpufreq: arm_big_little: check if the frequency is set correctly
Date: Tue, 31 Mar 2015 10:24:29 +0100	[thread overview]
Message-ID: <551A67CD.50602@arm.com> (raw)
In-Reply-To: <20150331014854.25195.34023@quantum>

On 31/03/15 02:48, Michael Turquette wrote:
> Quoting Sudeep Holla (2015-03-30 06:39:00)
>> On 30/03/15 14:27, Viresh Kumar wrote:
>>> On 30 March 2015 at 17:57, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>> The actual frequency is set through "clk_change_rate" which is void
>>>> function. If the underlying hardware fails and returns error, the error
>>>> is lost in the clk layer. In order to track such failures, we need to
>>>> read back the frequency(just the cached value as clk_recalc called after
>>>> clk->ops->set_rate gets the frequency)

[...]
>>>
>>> This doesn't look to me the right place for fixing this.
>>>
>>
>> Yes I agree, after going through clk.c, I thought pre-/post- notifiers
>> are designed for such purpose. I tried using them but found it
>> unnecessary when it can be as simple as in this patch. However it's good
>> to hear from Mike as I seem to have assumed a lot here.
>
> Viresh & Sudeep,
>
> clk_set_rate returns an error (and always has), so it seems to me that
> this patch is unnecessary. bL_cpufreq_set_rate checks for an error from
> clk_set_rate and handles it.
>

No that's not correct, may be I was not clear earlier. Let me explain
with the stack trace.

bL_cpufreq_set_target(returns 0 even when clock driver returned error)
         |
         V
clk_set_rate(returns whatever it get from clk_core_set_rate_nolock)
         |
         V
clk_core_set_rate_nolock(always return 0 after calling clk_change_rate)
         |
         V
clk_change_rate(void function, so no return)
         |
         V
clk->ops->set_rate(i.e. <clock_driver_set_rate>)

Now for drivers/clk/clk.c IIUC, the return value from clk->ops->set_rate
is not checked. Now if <clock_driver_set_rate> returns error when h/w
fails to set the rate, I would like to know how the error returned by
<clock_driver_set_rate> is returned and received by clk_set_rate.
Correct me if I am missing anything in the above sequence.

In the current state of code, one can use notifier(basically
POST_RATE_CHANGE is called only if the clock rate changes), but since
the clk_recalc reads back the clock rate, I found this patch is simpler
compared to the notifiers.

Regards,
Sudeep