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 <[email protected]> > Signed-off-by: Daniel Lezcano <[email protected]> > --- > 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 <linux/module.h> > #include <linux/of.h> > #include <linux/slab.h> > +#include <linux/topology.h> > > #include <asm/cpuidle.h> > > @@ -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
