On Thu, Sep 4, 2014 at 7:00 PM, Kevin Hilman khilman@kernel.org wrote:
Thomas Abraham ta.omasab@gmail.com writes:
On Thu, Sep 4, 2014 at 4:45 AM, Kevin Hilman khilman@kernel.org wrote:
Hi Thomas,
Thomas Abraham ta.omasab@gmail.com writes:
[...]
A new branch [1] has been created using commits from exynos5-v3.17-rc1 branch + cpufreq + regulator + temp fixes. I have tested this branch on Exynos5800 Chromebook2 and cpufreq works fine with both ondemand and performance governors. Please let me know if there are any issues with this new branch. It is based on v3.17-rc3.
Excellent! Thank you.
The only thing missing now is the CPUidle support for 5800, and all that's needed for that is the compatible string patch[1] which Daniel has queued up.
With that patch, display + CPUidle + CPUfreq are working pretty well on my exynos5800/chromebook2 with the big.LITTLE switcher disabled. If I turn on the switcher, it boots OK, but as soon as I try to run powertop (upstream v2.6.1) it gets stuck. Have you tried this branch with the switcher enabled?
Yes, I have tested switcher + cpufreq + cpuidle with this branch and there are no issues found. I haven't tested with powertop yet. I will try and do that and let you know the result.
You mentioned that when you run powertop, it gets stuck. When that happens, is there any log on the console or does system just turn unresponsive?
The console is not responsive, but kernel seems busy because I see periodic timeout messages from the samsung clock driver.
Ok, I haven't tried to recreate this issue. I will try and do that.
Note that I see these messages when things are functioning normally also.
I did notice this a few times before but today I was able to reproduce this consistently with other test cases. This timeout is because the CPU clock blocks of the cluster that has been turned down are being reconfigured, which on Exynos will not work. The following is a temporary patch which solves this issue.
From b0c4057d428134fe12446431ede1d9a579fd1d05 Mon Sep 17 00:00:00 2001
From: Thomas Abraham thomas.ab@samsung.com Date: Fri, 5 Sep 2014 17:11:10 +0530 Subject: [PATCH] TEMP: cpufreq/bL: let the CPU switch complete before scaling frequency
On Exynos5420/Exyons5800, the clock blocks that make up the CPU clock supply do no operate when the cluster in which they belong is powered off. The CPU clock supply path is PLL -> Mux/Dividers -> CPU_clock.
In the arm_big_little CPUfreq driver, the frequency is scaled first and then the CPU is switched to the new cluster. In case the switch was for the first-man CPU in that new cluster, the frequency scaling step in arm_big_little CPUfreq driver would not work for Exynos since the in-bound cluster is powered off at that point. Note: On Exynos, the cluster is powered off when all the CPUs in that cluster are powered off which implies that the CPU clock blocks for that cluster do not operate anymore.
So when using arm_big_little CPUfreq driver for Exynos, two changes are required. The first change is to let the CPU to switch to the new cluster before scaling the frequency. The second change is to ensure that the switch has been completed before scaling the frequency.
With these changes, the message "wait_until_divider_stable: timeout in divider stabilization" is not seen anymore.
Signed-off-by: Thomas Abraham thomas.ab@samsung.com --- arch/arm/include/asm/bL_switcher.h | 15 ++++++++++++++- drivers/cpufreq/arm_big_little.c | 9 ++++++++- 2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/arch/arm/include/asm/bL_switcher.h b/arch/arm/include/asm/bL_switcher.h index 1714800..d609b86 100644 --- a/arch/arm/include/asm/bL_switcher.h +++ b/arch/arm/include/asm/bL_switcher.h @@ -20,9 +20,22 @@ typedef void (*bL_switch_completion_handler)(void *cookie); int bL_switch_request_cb(unsigned int cpu, unsigned int new_cluster_id, bL_switch_completion_handler completer, void *completer_cookie); + +static void bL_switch_complete_cb(void *cookie) +{ + struct completion *switch_complete = (struct completion *) cookie; + complete(switch_complete); +} + static inline int bL_switch_request(unsigned int cpu, unsigned int new_cluster_id) { - return bL_switch_request_cb(cpu, new_cluster_id, NULL, NULL); + struct completion complete; + + init_completion(&complete); + bL_switch_request_cb(cpu, new_cluster_id, &bL_switch_complete_cb, + &complete); + wait_for_completion(&complete); + return 0; }
/* diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c index a46c223..baeff47 100644 --- a/drivers/cpufreq/arm_big_little.c +++ b/drivers/cpufreq/arm_big_little.c @@ -129,6 +129,13 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate) int ret; bool bLs = is_bL_switching_enabled();
+ /* Switch cluster */ + if (old_cluster != new_cluster) { + pr_debug("%s: cpu: %d, old cluster: %d, new cluster: %d\n", + __func__, cpu, old_cluster, new_cluster); + bL_switch_request(cpu, new_cluster); + } + mutex_lock(&cluster_lock[new_cluster]);
if (bLs) { @@ -167,7 +174,7 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate) __func__, cpu, old_cluster, new_cluster);
/* Switch cluster */ - bL_switch_request(cpu, new_cluster); + /*bL_switch_request(cpu, new_cluster);*/
mutex_lock(&cluster_lock[old_cluster]);