| Message ID | 1497282910-19085-2-git-send-email-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Delegated to: | Rafael Wysocki |
| Headers | show |
On Monday, June 12, 2017 05:55:10 PM 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. > > 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. > > Every CPU has its own driver, so every single CPU can specify in the DT the > idle states. > > This simple approach allows to support the future dynamIQ system, current SMP > and HMP. > > Tested on: > - 96boards: Hikey 620 > - 96boards: Hikey 960 > - 96boards: dragonboard410c > - Mediatek 8173 > > Cc: Sudeep Holla <sudeep.holla@arm.com> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Tested-by: Leo Yan <leo.yan@linaro.org> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> There seems to have been quite some discussion regarding this one and I'm not sure about the resolution of it. I'd feel more comfortable with an ACK or Reviewed-by from Sudeep or Lorenzo here. > --- > drivers/cpuidle/Kconfig.arm | 1 + > drivers/cpuidle/cpuidle-arm.c | 62 ++++++++++++++++++++++++++----------------- > 2 files changed, 39 insertions(+), 24 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..7080c38 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,30 +81,42 @@ 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; > 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; > - } > - > - /* > - * Call arch CPU operations in order to initialize > - * idle states suspend back-end specific data > - */ > for_each_possible_cpu(cpu) { > + > + drv = kmemdup(&arm_idle_driver, sizeof(*drv), GFP_KERNEL); > + if (!drv) { > + ret = -ENOMEM; > + goto out_fail; > + } > + > + drv->cpumask = (struct cpumask *)cpumask_of(cpu); > + > + /* > + * 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) { > + ret = ret ? : -ENODEV; > + goto out_fail; > + } > + > + ret = cpuidle_register_driver(drv); > + if (ret) { > + pr_err("Failed to register cpuidle driver\n"); > + goto out_fail; > + } > + > + /* > + * Call arch CPU operations in order to initialize > + * idle states suspend back-end specific data > + */ > ret = arm_cpuidle_init(cpu); > > /* > @@ -141,10 +154,11 @@ static int __init arm_idle_init(void) > dev = per_cpu(cpuidle_devices, cpu); > cpuidle_unregister_device(dev); > kfree(dev); > + drv = cpuidle_get_driver(); > + cpuidle_unregister_driver(drv); > + kfree(drv); > } > > - cpuidle_unregister_driver(drv); > - > return ret; > } > device_initcall(arm_idle_init); > Thanks, Rafael
On Mon, Jun 12, 2017 at 08:49:39PM +0200, Rafael J. Wysocki wrote: > On Monday, June 12, 2017 05:55:10 PM 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. > > > > 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. > > > > Every CPU has its own driver, so every single CPU can specify in the DT the > > idle states. > > > > This simple approach allows to support the future dynamIQ system, current SMP > > and HMP. > > > > Tested on: > > - 96boards: Hikey 620 > > - 96boards: Hikey 960 > > - 96boards: dragonboard410c > > - Mediatek 8173 > > > > Cc: Sudeep Holla <sudeep.holla@arm.com> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Tested-by: Leo Yan <leo.yan@linaro.org> > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > There seems to have been quite some discussion regarding this one and I'm not > sure about the resolution of it. > > I'd feel more comfortable with an ACK or Reviewed-by from Sudeep or Lorenzo here. I understand. Sudeep it is ok with the patch [1] without an explicit acked-by. -- Daniel [1] https://www.spinics.net/lists/kernel/msg2525980.html
Hi Daniel, On 12/06/17 16:55, 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. > > 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. > > Every CPU has its own driver, so every single CPU can specify in the DT the > idle states. > > This simple approach allows to support the future dynamIQ system, current SMP > and HMP. > > Tested on: > - 96boards: Hikey 620 > - 96boards: Hikey 960 > - 96boards: dragonboard410c > - Mediatek 8173 > > Cc: Sudeep Holla <sudeep.holla@arm.com> Sorry for the delay, as I mentioned earlier I would like to add the minimum change to avoid this on platforms that don't require this. But that can be done later, I will try to come up with simple solution when I get time. Though I am not 100% happy ;), I am fine with this change for now: Acked-by: Sudeep Holla <sudeep.holla@arm.com>
On 12/06/2017 20:49, Rafael J. Wysocki wrote: > On Monday, June 12, 2017 05:55:10 PM 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. >> >> 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. >> >> Every CPU has its own driver, so every single CPU can specify in the DT the >> idle states. >> >> This simple approach allows to support the future dynamIQ system, current SMP >> and HMP. >> >> Tested on: >> - 96boards: Hikey 620 >> - 96boards: Hikey 960 >> - 96boards: dragonboard410c >> - Mediatek 8173 >> >> Cc: Sudeep Holla <sudeep.holla@arm.com> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> Tested-by: Leo Yan <leo.yan@linaro.org> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > There seems to have been quite some discussion regarding this one and I'm not > sure about the resolution of it. > > I'd feel more comfortable with an ACK or Reviewed-by from Sudeep or Lorenzo here. Hi Rafael, just a gentle reminder, Sudeep acked the patch. Thanks. -- Daniel
On Thursday, June 22, 2017 02:25:19 PM Daniel Lezcano wrote: > On 12/06/2017 20:49, Rafael J. Wysocki wrote: > > On Monday, June 12, 2017 05:55:10 PM 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. > >> > >> 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. > >> > >> Every CPU has its own driver, so every single CPU can specify in the DT the > >> idle states. > >> > >> This simple approach allows to support the future dynamIQ system, current SMP > >> and HMP. > >> > >> Tested on: > >> - 96boards: Hikey 620 > >> - 96boards: Hikey 960 > >> - 96boards: dragonboard410c > >> - Mediatek 8173 > >> > >> Cc: Sudeep Holla <sudeep.holla@arm.com> > >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > >> Tested-by: Leo Yan <leo.yan@linaro.org> > >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > > > There seems to have been quite some discussion regarding this one and I'm not > > sure about the resolution of it. > > > > I'd feel more comfortable with an ACK or Reviewed-by from Sudeep or Lorenzo here. > > > Hi Rafael, > > just a gentle reminder, Sudeep acked the patch. Yes, I'll get to it later today, most likely. Thanks, Rafael
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..7080c38 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,30 +81,42 @@ 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; 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; - } - - /* - * Call arch CPU operations in order to initialize - * idle states suspend back-end specific data - */ for_each_possible_cpu(cpu) { + + drv = kmemdup(&arm_idle_driver, sizeof(*drv), GFP_KERNEL); + if (!drv) { + ret = -ENOMEM; + goto out_fail; + } + + drv->cpumask = (struct cpumask *)cpumask_of(cpu); + + /* + * 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) { + ret = ret ? : -ENODEV; + goto out_fail; + } + + ret = cpuidle_register_driver(drv); + if (ret) { + pr_err("Failed to register cpuidle driver\n"); + goto out_fail; + } + + /* + * Call arch CPU operations in order to initialize + * idle states suspend back-end specific data + */ ret = arm_cpuidle_init(cpu); /* @@ -141,10 +154,11 @@ static int __init arm_idle_init(void) dev = per_cpu(cpuidle_devices, cpu); cpuidle_unregister_device(dev); kfree(dev); + drv = cpuidle_get_driver(); + cpuidle_unregister_driver(drv); + kfree(drv); } - cpuidle_unregister_driver(drv); - return ret; } device_initcall(arm_idle_init);