From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752228AbbJNIsm (ORCPT ); Wed, 14 Oct 2015 04:48:42 -0400 Received: from foss.arm.com ([217.140.101.70]:56727 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751329AbbJNIsf (ORCPT ); Wed, 14 Oct 2015 04:48:35 -0400 Subject: Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL switcher is active To: "Jon Medhurst (Tixy)" References: <1443807532.2845.25.camel@linaro.org> <20151007173920.GG4557@linux> <1444296229.2847.9.camel@linaro.org> <561BB39A.4020400@arm.com> <1444720744.2686.10.camel@linaro.org> <561CDEA9.7020700@arm.com> <1444806720.2691.15.camel@linaro.org> Cc: Sudeep Holla , Viresh Kumar , "Rafael J. Wysocki" , linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org From: Sudeep Holla Message-ID: <561E16E0.8030906@arm.com> Date: Wed, 14 Oct 2015 09:48:32 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <1444806720.2691.15.camel@linaro.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 14/10/15 08:12, Jon Medhurst (Tixy) wrote: > On Tue, 2015-10-13 at 11:36 +0100, Sudeep Holla wrote: >> >> On 13/10/15 08:19, Jon Medhurst (Tixy) wrote: > [...] >>> But then we wouldn't get the WARN_ON and pr_err triggered when we detect >>> the clock rate isn't set, which surely is half the reason for the check >>> in the first place? >>> >> >> Not sure if I understand what you mean or may be I was not clear, so >> thought I will put the delta here. Let me know if and how its still a >> problem. >> >> diff --git i/drivers/cpufreq/arm_big_little.c >> w/drivers/cpufreq/arm_big_little.c >> index f1e42f8ce0fc..05e850f80f39 100644 >> --- i/drivers/cpufreq/arm_big_little.c >> +++ w/drivers/cpufreq/arm_big_little.c >> @@ -164,6 +164,16 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 >> new_cluster, u32 rate) >> >> mutex_unlock(&cluster_lock[new_cluster]); >> >> + /* >> + * FIXME: clk_set_rate has to handle the case where clk_change_rate >> + * can fail due to hardware or firmware issues. Until the clk core >> + * layer is fixed, we can check here. In most of the cases we will >> + * be reading only the cached value anyway. This needs to be >> removed >> + * once clk core is fixed. >> + */ >> + if (bL_cpufreq_get_rate(cpu) != new_rate) >> + return -EIO; >> + >> /* Recalc freq for old cluster when switching clusters */ >> if (old_cluster != new_cluster) { >> pr_debug("%s: cpu: %d, old cluster: %d, new cluster: %d\n", > > That's what I though you meant, and I can't see why you would want to do > that and bypass the error reporting for clk_get_rate failing. After all, > the code we're moving around is explicitly there to workaround the fact > that clk_set_rate doesn't actually pass through all errors, so it's > doing additional error checking. (At least, that's what the comment > says). So this looks more logical to me. > OK, I understand what you mean now. I don't have a strong opinion, but here is the reason why I prefer the approach I said earlier: clk_set_rate doesn't return error if the h/w or f/w return error which is usually the last step. So calling clk_get_rate when clk_set_rate return error quite early makes no sense to me. -- Regards, Sudeep