All of lore.kernel.org
 help / color / mirror / Atom feed
* [GIT PULL] ARM: cpuidle for 4.13
@ 2017-06-12 15:54 Daniel Lezcano
  2017-06-12 15:55   ` Daniel Lezcano
  0 siblings, 1 reply; 12+ messages in thread
From: Daniel Lezcano @ 2017-06-12 15:54 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: linux-pm

Hi Rafael,

this is a pull request for a fix already and the asymmetric idle state support
for HMP systems. It is based on V4.12-rc5.

 - Make per cpuidle driver for the ARM cpuidle driver in order to support SMP,
   HMP and future DynamIQ cpus (Daniel Lezcano)

 - Release of node on error (Christophe Jaillet)

Thanks

  -- Daniel

The following changes since commit 32c1431eea4881a6b17bd7c639315010aeefa452:

  Linux 4.12-rc5 (2017-06-11 16:48:20 -0700)

are available in the git repository at:

  http://git@git.linaro.org/people/daniel.lezcano/linux.git cpuidle/4.13

for you to fetch changes up to 610c28be6046786ccec22c827481fd865d4db8e0:

  ARM: cpuidle: Support asymmetric idle definition (2017-06-12 14:57:21 +0200)

----------------------------------------------------------------
Christophe Jaillet (1):
      cpuidle: dt: Add missing 'of_node_put()'

Daniel Lezcano (1):
      ARM: cpuidle: Support asymmetric idle definition

 drivers/cpuidle/Kconfig.arm      |  1 +
 drivers/cpuidle/cpuidle-arm.c    | 62 ++++++++++++++++++++++++++++++++++++++------------------------
 drivers/cpuidle/dt_idle_states.c |  4 +++-
 3 files changed, 42 insertions(+), 25 deletions(-)



-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] cpuidle: dt: Add missing 'of_node_put()'
  2017-06-12 15:54 [GIT PULL] ARM: cpuidle for 4.13 Daniel Lezcano
@ 2017-06-12 15:55   ` Daniel Lezcano
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel Lezcano @ 2017-06-12 15:55 UTC (permalink / raw)
  To: rjw; +Cc: linux-pm, Christophe Jaillet, open list

From: Christophe Jaillet <christophe.jaillet@wanadoo.fr>

'of_node_put()' should be called on pointer returned by
'of_parse_phandle()' when done. In this function this is done in all path
except this 'continue', so add it.

Fixes: 97735da074fd ("drivers: cpuidle: Add status property to ARM idle states")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/dt_idle_states.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
index ffca4fc..ae8eb03 100644
--- a/drivers/cpuidle/dt_idle_states.c
+++ b/drivers/cpuidle/dt_idle_states.c
@@ -180,8 +180,10 @@ int dt_init_idle_driver(struct cpuidle_driver *drv,
 		if (!state_node)
 			break;
 
-		if (!of_device_is_available(state_node))
+		if (!of_device_is_available(state_node)) {
+			of_node_put(state_node);
 			continue;
+		}
 
 		if (!idle_state_valid(state_node, i, cpumask)) {
 			pr_warn("%s idle state not valid, bailing out\n",
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 1/2] cpuidle: dt: Add missing 'of_node_put()'
@ 2017-06-12 15:55   ` Daniel Lezcano
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel Lezcano @ 2017-06-12 15:55 UTC (permalink / raw)
  To: rjw; +Cc: linux-pm, Christophe Jaillet, open list

From: Christophe Jaillet <christophe.jaillet@wanadoo.fr>

'of_node_put()' should be called on pointer returned by
'of_parse_phandle()' when done. In this function this is done in all path
except this 'continue', so add it.

Fixes: 97735da074fd ("drivers: cpuidle: Add status property to ARM idle states")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/dt_idle_states.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
index ffca4fc..ae8eb03 100644
--- a/drivers/cpuidle/dt_idle_states.c
+++ b/drivers/cpuidle/dt_idle_states.c
@@ -180,8 +180,10 @@ int dt_init_idle_driver(struct cpuidle_driver *drv,
 		if (!state_node)
 			break;
 
-		if (!of_device_is_available(state_node))
+		if (!of_device_is_available(state_node)) {
+			of_node_put(state_node);
 			continue;
+		}
 
 		if (!idle_state_valid(state_node, i, cpumask)) {
 			pr_warn("%s idle state not valid, bailing out\n",
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] ARM: cpuidle: Support asymmetric idle definition
  2017-06-12 15:55   ` Daniel Lezcano
@ 2017-06-12 15:55     ` Daniel Lezcano
  -1 siblings, 0 replies; 12+ messages in thread
From: Daniel Lezcano @ 2017-06-12 15:55 UTC (permalink / raw)
  To: rjw; +Cc: linux-pm, Sudeep Holla, Lorenzo Pieralisi, Leo Yan, open list

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>
---
 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);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] ARM: cpuidle: Support asymmetric idle definition
@ 2017-06-12 15:55     ` Daniel Lezcano
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel Lezcano @ 2017-06-12 15:55 UTC (permalink / raw)
  To: rjw; +Cc: linux-pm, Sudeep Holla, Lorenzo Pieralisi, Leo Yan, open list

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>
---
 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);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] cpuidle: dt: Add missing 'of_node_put()'
  2017-06-12 15:55   ` Daniel Lezcano
  (?)
  (?)
@ 2017-06-12 16:42   ` Lorenzo Pieralisi
  -1 siblings, 0 replies; 12+ messages in thread
From: Lorenzo Pieralisi @ 2017-06-12 16:42 UTC (permalink / raw)
  To: Daniel Lezcano; +Cc: rjw, linux-pm, Christophe Jaillet, open list

On Mon, Jun 12, 2017 at 05:55:09PM +0200, Daniel Lezcano wrote:
> From: Christophe Jaillet <christophe.jaillet@wanadoo.fr>
> 
> 'of_node_put()' should be called on pointer returned by
> 'of_parse_phandle()' when done. In this function this is done in all path
> except this 'continue', so add it.
> 
> Fixes: 97735da074fd ("drivers: cpuidle: Add status property to ARM idle states")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> ---
>  drivers/cpuidle/dt_idle_states.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
> index ffca4fc..ae8eb03 100644
> --- a/drivers/cpuidle/dt_idle_states.c
> +++ b/drivers/cpuidle/dt_idle_states.c
> @@ -180,8 +180,10 @@ int dt_init_idle_driver(struct cpuidle_driver *drv,
>  		if (!state_node)
>  			break;
>  
> -		if (!of_device_is_available(state_node))
> +		if (!of_device_is_available(state_node)) {
> +			of_node_put(state_node);
>  			continue;
> +		}
>  
>  		if (!idle_state_valid(state_node, i, cpumask)) {
>  			pr_warn("%s idle state not valid, bailing out\n",
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] cpuidle: dt: Add missing 'of_node_put()'
  2017-06-12 15:55   ` Daniel Lezcano
                     ` (2 preceding siblings ...)
  (?)
@ 2017-06-12 18:46   ` Rafael J. Wysocki
  -1 siblings, 0 replies; 12+ messages in thread
From: Rafael J. Wysocki @ 2017-06-12 18:46 UTC (permalink / raw)
  To: Daniel Lezcano; +Cc: linux-pm, Christophe Jaillet, open list

On Monday, June 12, 2017 05:55:09 PM Daniel Lezcano wrote:
> From: Christophe Jaillet <christophe.jaillet@wanadoo.fr>
> 
> 'of_node_put()' should be called on pointer returned by
> 'of_parse_phandle()' when done. In this function this is done in all path
> except this 'continue', so add it.
> 
> Fixes: 97735da074fd ("drivers: cpuidle: Add status property to ARM idle states")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

I've already applied this one.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] ARM: cpuidle: Support asymmetric idle definition
  2017-06-12 15:55     ` Daniel Lezcano
  (?)
@ 2017-06-12 18:49     ` Rafael J. Wysocki
  2017-06-12 19:17       ` Daniel Lezcano
  2017-06-22 12:25       ` Daniel Lezcano
  -1 siblings, 2 replies; 12+ messages in thread
From: Rafael J. Wysocki @ 2017-06-12 18:49 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: linux-pm, Sudeep Holla, Lorenzo Pieralisi, Leo Yan, open list

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] ARM: cpuidle: Support asymmetric idle definition
  2017-06-12 18:49     ` Rafael J. Wysocki
@ 2017-06-12 19:17       ` Daniel Lezcano
  2017-06-22 12:25       ` Daniel Lezcano
  1 sibling, 0 replies; 12+ messages in thread
From: Daniel Lezcano @ 2017-06-12 19:17 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: linux-pm, Sudeep Holla, Lorenzo Pieralisi, Leo Yan, open list

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] ARM: cpuidle: Support asymmetric idle definition
  2017-06-12 15:55     ` Daniel Lezcano
  (?)
  (?)
@ 2017-06-13  9:46     ` Sudeep Holla
  -1 siblings, 0 replies; 12+ messages in thread
From: Sudeep Holla @ 2017-06-13  9:46 UTC (permalink / raw)
  To: Daniel Lezcano, rjw
  Cc: Sudeep Holla, linux-pm, Lorenzo Pieralisi, Leo Yan, open list

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>

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] ARM: cpuidle: Support asymmetric idle definition
  2017-06-12 18:49     ` Rafael J. Wysocki
  2017-06-12 19:17       ` Daniel Lezcano
@ 2017-06-22 12:25       ` Daniel Lezcano
  2017-06-22 14:27         ` Rafael J. Wysocki
  1 sibling, 1 reply; 12+ messages in thread
From: Daniel Lezcano @ 2017-06-22 12:25 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: linux-pm, Sudeep Holla, Lorenzo Pieralisi, Leo Yan, open list

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


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] ARM: cpuidle: Support asymmetric idle definition
  2017-06-22 12:25       ` Daniel Lezcano
@ 2017-06-22 14:27         ` Rafael J. Wysocki
  0 siblings, 0 replies; 12+ messages in thread
From: Rafael J. Wysocki @ 2017-06-22 14:27 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: linux-pm, Sudeep Holla, Lorenzo Pieralisi, Leo Yan, open list

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-06-22 14:35 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-12 15:54 [GIT PULL] ARM: cpuidle for 4.13 Daniel Lezcano
2017-06-12 15:55 ` [PATCH 1/2] cpuidle: dt: Add missing 'of_node_put()' Daniel Lezcano
2017-06-12 15:55   ` Daniel Lezcano
2017-06-12 15:55   ` [PATCH 2/2] ARM: cpuidle: Support asymmetric idle definition Daniel Lezcano
2017-06-12 15:55     ` Daniel Lezcano
2017-06-12 18:49     ` Rafael J. Wysocki
2017-06-12 19:17       ` Daniel Lezcano
2017-06-22 12:25       ` Daniel Lezcano
2017-06-22 14:27         ` Rafael J. Wysocki
2017-06-13  9:46     ` Sudeep Holla
2017-06-12 16:42   ` [PATCH 1/2] cpuidle: dt: Add missing 'of_node_put()' Lorenzo Pieralisi
2017-06-12 18:46   ` Rafael J. Wysocki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.