From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sudeep Holla Subject: Re: [PATCH] ARM: cpuidle: Support asymmetric idle definition Date: Mon, 22 May 2017 11:11:37 +0100 Message-ID: <850fab54-d6e6-7b4c-631a-f4ee658c96fa@arm.com> References: <1495212343-24873-1-git-send-email-daniel.lezcano@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: Received: from foss.arm.com ([217.140.101.70]:34402 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751616AbdEVKLl (ORCPT ); Mon, 22 May 2017 06:11:41 -0400 In-Reply-To: <1495212343-24873-1-git-send-email-daniel.lezcano@linaro.org> Content-Language: en-US Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Daniel Lezcano Cc: rjw@rjwysocki.net, lorenzo.pieralisi@arm.com, Sudeep Holla , leo.yan@linaro.org, "open list:CPUIDLE DRIVERS" , open list On 19/05/17 17:45, Daniel Lezcano wrote: > Some hardware have clusters with different idle states. The current code does > not support this and fails as it expects all the idle states to be identical. > > Because of this, the Mediatek mtk8173 had to create the same idle state for a > big.Little system and now the Hisilicon 960 is facing the same situation. > While I agree the we don't support them today, it's better to benchmark and record/compare the gain we get with the support for cluster based idle states. > Solve this by simply assuming the multiple driver will be needed for all the > platforms using the ARM generic cpuidle driver which makes sense because of the > different topologies we can support with a single kernel for ARM32 or ARM64. > Unfortunately, it's not true always and for sure will break with the new ARM DynamIQ [1] > Tested on: > - 96boards: Hikey 620 > - 96boards: Hikey 960 > - 96boards: dragonboard410c > - Mediatek 8173 > > Tested-by: Leo Yan > Signed-off-by: Daniel Lezcano > --- > drivers/cpuidle/Kconfig.arm | 1 + > drivers/cpuidle/cpuidle-arm.c | 55 +++++++++++++++++++++++++++++-------------- > 2 files changed, 38 insertions(+), 18 deletions(-) > > diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm > index 21340e0..f521448 100644 > --- a/drivers/cpuidle/Kconfig.arm > +++ b/drivers/cpuidle/Kconfig.arm > @@ -4,6 +4,7 @@ > config ARM_CPUIDLE > bool "Generic ARM/ARM64 CPU idle Driver" > select DT_IDLE_STATES > + select CPU_IDLE_MULTIPLE_DRIVERS > help > Select this to enable generic cpuidle driver for ARM. > It provides a generic idle driver whose idle states are configured > diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c > index f440d38..bec31d5 100644 > --- a/drivers/cpuidle/cpuidle-arm.c > +++ b/drivers/cpuidle/cpuidle-arm.c > @@ -18,6 +18,7 @@ > #include > #include > #include > +#include > > #include > > @@ -44,7 +45,7 @@ static int arm_enter_idle_state(struct cpuidle_device *dev, > return CPU_PM_CPU_IDLE_ENTER(arm_cpuidle_suspend, idx); > } > > -static struct cpuidle_driver arm_idle_driver = { > +static struct cpuidle_driver arm_idle_driver __initdata = { > .name = "arm_idle", > .owner = THIS_MODULE, > /* > @@ -80,23 +81,40 @@ static const struct of_device_id arm_idle_state_match[] __initconst = { > static int __init arm_idle_init(void) > { > int cpu, ret; > - struct cpuidle_driver *drv = &arm_idle_driver; > + struct cpuidle_driver *drv = NULL; > struct cpuidle_device *dev; > > - /* > - * Initialize idle states data, starting at index 1. > - * This driver is DT only, if no DT idle states are detected (ret == 0) > - * let the driver initialization fail accordingly since there is no > - * reason to initialize the idle driver if only wfi is supported. > - */ > - ret = dt_init_idle_driver(drv, arm_idle_state_match, 1); > - if (ret <= 0) > - return ret ? : -ENODEV; > - > - ret = cpuidle_register_driver(drv); > - if (ret) { > - pr_err("Failed to register cpuidle driver\n"); > - return ret; > + for_each_possible_cpu(cpu) { > + > + if (drv && cpumask_test_cpu(cpu, drv->cpumask)) > + continue; > + > + ret = -ENOMEM; > + > + drv = kmemdup(&arm_idle_driver, sizeof(*drv), GFP_KERNEL); > + if (!drv) > + goto out_fail; > + > + drv->cpumask = &cpu_topology[cpu].core_sibling; > + This is not always true and not architecturally guaranteed. So instead of introducing this broken dependency, better to extract information from the device tree. -- Regards, Sudeep [1] https://community.arm.com/processors/b/blog/posts/arm-dynamiq-technology-for-the-next-era-of-compute