[PATCH 08/10] MIPS: support use of cpuidle

[PATCH 08/10] MIPS: support use of cpuidle

[Paul Burton]

This patch lays the groundwork for MIPS platforms to make use of the
cpuidle framework. The arch_cpu_idle function simply calls cpuidle &
falls back to the regular cpu_wait path if cpuidle should fail (eg. if
it's not selected or no driver is registered). A generic cpuidle state
for the wait instruction is introduced, intended to ease use of the wait
instruction from cpuidle drivers and reduce code duplication.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-pm@vger.kernel.org
---
 arch/mips/Kconfig            |  2 ++
 arch/mips/include/asm/idle.h | 14 ++++++++++++++
 arch/mips/kernel/idle.c      | 20 +++++++++++++++++++-
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 5bc27c0..95f2f11 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -1838,6 +1838,8 @@ config CPU_R4K_CACHE_TLB
 	bool
 	default y if !(CPU_R3000 || CPU_R8000 || CPU_SB1 || CPU_TX39XX || CPU_CAVIUM_OCTEON)
 
+source "drivers/cpuidle/Kconfig"
+
 choice
 	prompt "MIPS MT options"
 
diff --git a/arch/mips/include/asm/idle.h b/arch/mips/include/asm/idle.h
index d192158..d9f932d 100644
--- a/arch/mips/include/asm/idle.h
+++ b/arch/mips/include/asm/idle.h
@@ -1,6 +1,7 @@
 #ifndef __ASM_IDLE_H
 #define __ASM_IDLE_H
 
+#include <linux/cpuidle.h>
 #include <linux/linkage.h>
 
 extern void (*cpu_wait)(void);
@@ -20,4 +21,17 @@ static inline int address_is_in_r4k_wait_irqoff(unsigned long addr)
 	       addr < (unsigned long)__pastwait;
 }
 
+extern int mips_cpuidle_wait_enter(struct cpuidle_device *dev,
+				   struct cpuidle_driver *drv, int index);
+
+#define MIPS_CPUIDLE_WAIT_STATE {\
+	.enter			= mips_cpuidle_wait_enter,\
+	.exit_latency		= 1,\
+	.target_residency	= 1,\
+	.power_usage		= UINT_MAX,\
+	.flags			= CPUIDLE_FLAG_TIME_VALID,\
+	.name			= "wait",\
+	.desc			= "MIPS wait",\
+}
+
 #endif /* __ASM_IDLE_H  */
diff --git a/arch/mips/kernel/idle.c b/arch/mips/kernel/idle.c
index 3553243..64e91e4 100644
--- a/arch/mips/kernel/idle.c
+++ b/arch/mips/kernel/idle.c
@@ -11,6 +11,7 @@
  * as published by the Free Software Foundation; either version
  * 2 of the License, or (at your option) any later version.
  */
+#include <linux/cpuidle.h>
 #include <linux/export.h>
 #include <linux/init.h>
 #include <linux/irqflags.h>
@@ -239,7 +240,7 @@ static void smtc_idle_hook(void)
 #endif
 }
 
-void arch_cpu_idle(void)
+static void mips_cpu_idle(void)
 {
 	smtc_idle_hook();
 	if (cpu_wait)
@@ -247,3 +248,20 @@ void arch_cpu_idle(void)
 	else
 		local_irq_enable();
 }
+
+void arch_cpu_idle(void)
+{
+	if (cpuidle_idle_call())
+		mips_cpu_idle();
+}
+
+#ifdef CONFIG_CPU_IDLE
+
+int mips_cpuidle_wait_enter(struct cpuidle_device *dev,
+			    struct cpuidle_driver *drv, int index)
+{
+	mips_cpu_idle();
+	return index;
+}
+
+#endif
-- 
1.8.4.2

Re: [PATCH 08/10] MIPS: support use of cpuidle

[Daniel Lezcano]

On 01/15/2014 02:55 PM, Paul Burton wrote:
> This patch lays the groundwork for MIPS platforms to make use of the
> cpuidle framework. The arch_cpu_idle function simply calls cpuidle &
> falls back to the regular cpu_wait path if cpuidle should fail (eg. if
> it's not selected or no driver is registered). A generic cpuidle state
> for the wait instruction is introduced, intended to ease use of the wait
> instruction from cpuidle drivers and reduce code duplication.

Hi,

What is the status of this patchset ? Still need comments ?

Thanks
   -- Daniel

>
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: linux-pm@vger.kernel.org
> ---
>   arch/mips/Kconfig            |  2 ++
>   arch/mips/include/asm/idle.h | 14 ++++++++++++++
>   arch/mips/kernel/idle.c      | 20 +++++++++++++++++++-
>   3 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 5bc27c0..95f2f11 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -1838,6 +1838,8 @@ config CPU_R4K_CACHE_TLB
>   	bool
>   	default y if !(CPU_R3000 || CPU_R8000 || CPU_SB1 || CPU_TX39XX || CPU_CAVIUM_OCTEON)
>
> +source "drivers/cpuidle/Kconfig"
> +
>   choice
>   	prompt "MIPS MT options"
>
> diff --git a/arch/mips/include/asm/idle.h b/arch/mips/include/asm/idle.h
> index d192158..d9f932d 100644
> --- a/arch/mips/include/asm/idle.h
> +++ b/arch/mips/include/asm/idle.h
> @@ -1,6 +1,7 @@
>   #ifndef __ASM_IDLE_H
>   #define __ASM_IDLE_H
>
> +#include <linux/cpuidle.h>
>   #include <linux/linkage.h>
>
>   extern void (*cpu_wait)(void);
> @@ -20,4 +21,17 @@ static inline int address_is_in_r4k_wait_irqoff(unsigned long addr)
>   	       addr < (unsigned long)__pastwait;
>   }
>
> +extern int mips_cpuidle_wait_enter(struct cpuidle_device *dev,
> +				   struct cpuidle_driver *drv, int index);
> +
> +#define MIPS_CPUIDLE_WAIT_STATE {\
> +	.enter			= mips_cpuidle_wait_enter,\
> +	.exit_latency		= 1,\
> +	.target_residency	= 1,\
> +	.power_usage		= UINT_MAX,\
> +	.flags			= CPUIDLE_FLAG_TIME_VALID,\
> +	.name			= "wait",\
> +	.desc			= "MIPS wait",\
> +}
> +
>   #endif /* __ASM_IDLE_H  */
> diff --git a/arch/mips/kernel/idle.c b/arch/mips/kernel/idle.c
> index 3553243..64e91e4 100644
> --- a/arch/mips/kernel/idle.c
> +++ b/arch/mips/kernel/idle.c
> @@ -11,6 +11,7 @@
>    * as published by the Free Software Foundation; either version
>    * 2 of the License, or (at your option) any later version.
>    */
> +#include <linux/cpuidle.h>
>   #include <linux/export.h>
>   #include <linux/init.h>
>   #include <linux/irqflags.h>
> @@ -239,7 +240,7 @@ static void smtc_idle_hook(void)
>   #endif
>   }
>
> -void arch_cpu_idle(void)
> +static void mips_cpu_idle(void)
>   {
>   	smtc_idle_hook();
>   	if (cpu_wait)
> @@ -247,3 +248,20 @@ void arch_cpu_idle(void)
>   	else
>   		local_irq_enable();
>   }
> +
> +void arch_cpu_idle(void)
> +{
> +	if (cpuidle_idle_call())
> +		mips_cpu_idle();
> +}
> +
> +#ifdef CONFIG_CPU_IDLE
> +
> +int mips_cpuidle_wait_enter(struct cpuidle_device *dev,
> +			    struct cpuidle_driver *drv, int index)
> +{
> +	mips_cpu_idle();
> +	return index;
> +}
> +
> +#endif
>


-- 
  <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

Re: [PATCH 08/10] MIPS: support use of cpuidle

[Paul Burton]

On Thu, Feb 20, 2014 at 09:52:26AM +0100, Daniel Lezcano wrote:
> On 01/15/2014 02:55 PM, Paul Burton wrote:
> >This patch lays the groundwork for MIPS platforms to make use of the
> >cpuidle framework. The arch_cpu_idle function simply calls cpuidle &
> >falls back to the regular cpu_wait path if cpuidle should fail (eg. if
> >it's not selected or no driver is registered). A generic cpuidle state
> >for the wait instruction is introduced, intended to ease use of the wait
> >instruction from cpuidle drivers and reduce code duplication.
> 
> Hi,
> 
> What is the status of this patchset ? Still need comments ?
> 
> Thanks
>   -- Daniel
> 

It's present in Ralf's current mips-for-linux-next branch, and in
linux-next from next-20140210 up to todays next-20140220. So I presume
barring any problems it should make it into 3.15.

Thanks,
    Paul

> >
> >Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> >Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> >Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> >Cc: linux-pm@vger.kernel.org
> >---
> >  arch/mips/Kconfig            |  2 ++
> >  arch/mips/include/asm/idle.h | 14 ++++++++++++++
> >  arch/mips/kernel/idle.c      | 20 +++++++++++++++++++-
> >  3 files changed, 35 insertions(+), 1 deletion(-)
> >
> >diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> >index 5bc27c0..95f2f11 100644
> >--- a/arch/mips/Kconfig
> >+++ b/arch/mips/Kconfig
> >@@ -1838,6 +1838,8 @@ config CPU_R4K_CACHE_TLB
> >  	bool
> >  	default y if !(CPU_R3000 || CPU_R8000 || CPU_SB1 || CPU_TX39XX || CPU_CAVIUM_OCTEON)
> >
> >+source "drivers/cpuidle/Kconfig"
> >+
> >  choice
> >  	prompt "MIPS MT options"
> >
> >diff --git a/arch/mips/include/asm/idle.h b/arch/mips/include/asm/idle.h
> >index d192158..d9f932d 100644
> >--- a/arch/mips/include/asm/idle.h
> >+++ b/arch/mips/include/asm/idle.h
> >@@ -1,6 +1,7 @@
> >  #ifndef __ASM_IDLE_H
> >  #define __ASM_IDLE_H
> >
> >+#include <linux/cpuidle.h>
> >  #include <linux/linkage.h>
> >
> >  extern void (*cpu_wait)(void);
> >@@ -20,4 +21,17 @@ static inline int address_is_in_r4k_wait_irqoff(unsigned long addr)
> >  	       addr < (unsigned long)__pastwait;
> >  }
> >
> >+extern int mips_cpuidle_wait_enter(struct cpuidle_device *dev,
> >+				   struct cpuidle_driver *drv, int index);
> >+
> >+#define MIPS_CPUIDLE_WAIT_STATE {\
> >+	.enter			= mips_cpuidle_wait_enter,\
> >+	.exit_latency		= 1,\
> >+	.target_residency	= 1,\
> >+	.power_usage		= UINT_MAX,\
> >+	.flags			= CPUIDLE_FLAG_TIME_VALID,\
> >+	.name			= "wait",\
> >+	.desc			= "MIPS wait",\
> >+}
> >+
> >  #endif /* __ASM_IDLE_H  */
> >diff --git a/arch/mips/kernel/idle.c b/arch/mips/kernel/idle.c
> >index 3553243..64e91e4 100644
> >--- a/arch/mips/kernel/idle.c
> >+++ b/arch/mips/kernel/idle.c
> >@@ -11,6 +11,7 @@
> >   * as published by the Free Software Foundation; either version
> >   * 2 of the License, or (at your option) any later version.
> >   */
> >+#include <linux/cpuidle.h>
> >  #include <linux/export.h>
> >  #include <linux/init.h>
> >  #include <linux/irqflags.h>
> >@@ -239,7 +240,7 @@ static void smtc_idle_hook(void)
> >  #endif
> >  }
> >
> >-void arch_cpu_idle(void)
> >+static void mips_cpu_idle(void)
> >  {
> >  	smtc_idle_hook();
> >  	if (cpu_wait)
> >@@ -247,3 +248,20 @@ void arch_cpu_idle(void)
> >  	else
> >  		local_irq_enable();
> >  }
> >+
> >+void arch_cpu_idle(void)
> >+{
> >+	if (cpuidle_idle_call())
> >+		mips_cpu_idle();
> >+}
> >+
> >+#ifdef CONFIG_CPU_IDLE
> >+
> >+int mips_cpuidle_wait_enter(struct cpuidle_device *dev,
> >+			    struct cpuidle_driver *drv, int index)
> >+{
> >+	mips_cpu_idle();
> >+	return index;
> >+}
> >+
> >+#endif
> >
> 
> 
> -- 
>  <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
> 

[PATCH 09/10] cpuidle: declare cpuidle_dev in cpuidle.h

[Paul Burton]

Declaring this allows drivers which need to initialise each struct
cpuidle_device at initialisation time to make use of the structures
already defined in cpuidle.c, rather than having to wastefully define
their own.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-pm@vger.kernel.org
---
 include/linux/cpuidle.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 50fcbb0..bab4f33 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -84,6 +84,7 @@ struct cpuidle_device {
 };
 
 DECLARE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
+DECLARE_PER_CPU(struct cpuidle_device, cpuidle_dev);
 
 /**
  * cpuidle_get_last_residency - retrieves the last state's residency time
-- 
1.8.4.2

Re: [PATCH 09/10] cpuidle: declare cpuidle_dev in cpuidle.h

[Daniel Lezcano]

On 01/15/2014 02:55 PM, Paul Burton wrote:
> Declaring this allows drivers which need to initialise each struct
> cpuidle_device at initialisation time to make use of the structures
> already defined in cpuidle.c, rather than having to wastefully define
> their own.
>
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: linux-pm@vger.kernel.org
> ---
>   include/linux/cpuidle.h | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 50fcbb0..bab4f33 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -84,6 +84,7 @@ struct cpuidle_device {
>   };
>
>   DECLARE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
> +DECLARE_PER_CPU(struct cpuidle_device, cpuidle_dev);


Nak. When a device is registered, it is assigned to the cpuidle_devices 
pointer and the backend driver should use it.



-- 
  <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

Re: [PATCH 09/10] cpuidle: declare cpuidle_dev in cpuidle.h

[Paul Burton]

On Thu, Feb 20, 2014 at 02:35:18PM +0100, Daniel Lezcano wrote:
> On 01/15/2014 02:55 PM, Paul Burton wrote:
> >Declaring this allows drivers which need to initialise each struct
> >cpuidle_device at initialisation time to make use of the structures
> >already defined in cpuidle.c, rather than having to wastefully define
> >their own.
> >
> >Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> >Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> >Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> >Cc: linux-pm@vger.kernel.org
> >---
> >  include/linux/cpuidle.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> >diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> >index 50fcbb0..bab4f33 100644
> >--- a/include/linux/cpuidle.h
> >+++ b/include/linux/cpuidle.h
> >@@ -84,6 +84,7 @@ struct cpuidle_device {
> >  };
> >
> >  DECLARE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
> >+DECLARE_PER_CPU(struct cpuidle_device, cpuidle_dev);
> 
> 
> Nak. When a device is registered, it is assigned to the cpuidle_devices
> pointer and the backend driver should use it.
> 

Yes, but then if the driver needs to initialise the coupled_cpus mask
then it cannot do so until after the device has been registered. During
registration the cpuidle_coupled_register_device will then see the empty
coupled_cpus mask & do nothing. The only other ways around this would be
for the driver to define its own per-cpu struct cpuidle_device (which as
I state in the commit message seems wasteful when cpuidle already
defined them), or for cpuidle_coupled_register_device to be called later
after the driver had a chance to modify devices via the cpuidle_devices
pointers.

Paul

> 
> 
> -- 
>  <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
> 

Re: [PATCH 09/10] cpuidle: declare cpuidle_dev in cpuidle.h

[Daniel Lezcano]

On 02/20/2014 02:41 PM, Paul Burton wrote:
> On Thu, Feb 20, 2014 at 02:35:18PM +0100, Daniel Lezcano wrote:
>> On 01/15/2014 02:55 PM, Paul Burton wrote:
>>> Declaring this allows drivers which need to initialise each struct
>>> cpuidle_device at initialisation time to make use of the structures
>>> already defined in cpuidle.c, rather than having to wastefully define
>>> their own.
>>>
>>> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Cc: linux-pm@vger.kernel.org
>>> ---
>>>   include/linux/cpuidle.h | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
>>> index 50fcbb0..bab4f33 100644
>>> --- a/include/linux/cpuidle.h
>>> +++ b/include/linux/cpuidle.h
>>> @@ -84,6 +84,7 @@ struct cpuidle_device {
>>>   };
>>>
>>>   DECLARE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
>>> +DECLARE_PER_CPU(struct cpuidle_device, cpuidle_dev);
>>
>>
>> Nak. When a device is registered, it is assigned to the cpuidle_devices
>> pointer and the backend driver should use it.
>>
>
> Yes, but then if the driver needs to initialise the coupled_cpus mask
> then it cannot do so until after the device has been registered. During
> registration the cpuidle_coupled_register_device will then see the empty
> coupled_cpus mask & do nothing. The only other ways around this would be
> for the driver to define its own per-cpu struct cpuidle_device (which as
> I state in the commit message seems wasteful when cpuidle already
> defined them), or for cpuidle_coupled_register_device to be called later
> after the driver had a chance to modify devices via the cpuidle_devices
> pointers.

Yeah. I understand why you wanted to declare these cpu variables.

The mips cpuidle driver sounds like a bit particular. I believe I need 
some clarification on the behavior of the hardware to understand 
correctly the driver. Could you explain how the couples act vs the cpu ? 
And why cpu_sibling is used instead of cpu_possible_mask ?



>> --
>>   <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
>>
>


-- 
  <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

Re: [PATCH 09/10] cpuidle: declare cpuidle_dev in cpuidle.h

[Paul Burton]

On Thu, Feb 20, 2014 at 02:53:06PM +0100, Daniel Lezcano wrote:
> On 02/20/2014 02:41 PM, Paul Burton wrote:
> >On Thu, Feb 20, 2014 at 02:35:18PM +0100, Daniel Lezcano wrote:
> >>On 01/15/2014 02:55 PM, Paul Burton wrote:
> >>>Declaring this allows drivers which need to initialise each struct
> >>>cpuidle_device at initialisation time to make use of the structures
> >>>already defined in cpuidle.c, rather than having to wastefully define
> >>>their own.
> >>>
> >>>Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> >>>Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> >>>Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>>Cc: linux-pm@vger.kernel.org
> >>>---
> >>>  include/linux/cpuidle.h | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>>diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> >>>index 50fcbb0..bab4f33 100644
> >>>--- a/include/linux/cpuidle.h
> >>>+++ b/include/linux/cpuidle.h
> >>>@@ -84,6 +84,7 @@ struct cpuidle_device {
> >>>  };
> >>>
> >>>  DECLARE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
> >>>+DECLARE_PER_CPU(struct cpuidle_device, cpuidle_dev);
> >>
> >>
> >>Nak. When a device is registered, it is assigned to the cpuidle_devices
> >>pointer and the backend driver should use it.
> >>
> >
> >Yes, but then if the driver needs to initialise the coupled_cpus mask
> >then it cannot do so until after the device has been registered. During
> >registration the cpuidle_coupled_register_device will then see the empty
> >coupled_cpus mask & do nothing. The only other ways around this would be
> >for the driver to define its own per-cpu struct cpuidle_device (which as
> >I state in the commit message seems wasteful when cpuidle already
> >defined them), or for cpuidle_coupled_register_device to be called later
> >after the driver had a chance to modify devices via the cpuidle_devices
> >pointers.
> 
> Yeah. I understand why you wanted to declare these cpu variables.
> 
> The mips cpuidle driver sounds like a bit particular. I believe I need some
> clarification on the behavior of the hardware to understand correctly the
> driver. Could you explain how the couples act vs the cpu ? And why
> cpu_sibling is used instead of cpu_possible_mask ?
> 
> 

Sure. The CPUs that are coupled are actually VPEs (Virtual Processor
Elements) within a single core. They share the compute resource (ALUs
etc) of the core but have their own register state, ie. they're a form
of simultaneous multithreading.

Coherence within a MIPS Coherent Processing System is a property of the
core rather than of an individual VPE (since the VPEs within a core
share the same L1 caches). So for idle states which are non-coherent the
VPEs within the core are coupled. That covers all idle states beyond a
simple "wait" instruction - clock gating or powering down a core
requires it to become non-coherent first.

cpu_sibling_mask is already setup to indicate which CPUs (VPEs) are
within the same core as each other, which is why it is simply copied for
coupled_cpus.

Paul

Re: [PATCH 09/10] cpuidle: declare cpuidle_dev in cpuidle.h

[Daniel Lezcano]

On 02/20/2014 03:00 PM, Paul Burton wrote:
> On Thu, Feb 20, 2014 at 02:53:06PM +0100, Daniel Lezcano wrote:
>> On 02/20/2014 02:41 PM, Paul Burton wrote:
>>> On Thu, Feb 20, 2014 at 02:35:18PM +0100, Daniel Lezcano wrote:
>>>> On 01/15/2014 02:55 PM, Paul Burton wrote:
>>>>> Declaring this allows drivers which need to initialise each struct
>>>>> cpuidle_device at initialisation time to make use of the structures
>>>>> already defined in cpuidle.c, rather than having to wastefully define
>>>>> their own.
>>>>>
>>>>> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
>>>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>>>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>>> Cc: linux-pm@vger.kernel.org
>>>>> ---
>>>>>   include/linux/cpuidle.h | 1 +
>>>>>   1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
>>>>> index 50fcbb0..bab4f33 100644
>>>>> --- a/include/linux/cpuidle.h
>>>>> +++ b/include/linux/cpuidle.h
>>>>> @@ -84,6 +84,7 @@ struct cpuidle_device {
>>>>>   };
>>>>>
>>>>>   DECLARE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
>>>>> +DECLARE_PER_CPU(struct cpuidle_device, cpuidle_dev);
>>>>
>>>>
>>>> Nak. When a device is registered, it is assigned to the cpuidle_devices
>>>> pointer and the backend driver should use it.
>>>>
>>>
>>> Yes, but then if the driver needs to initialise the coupled_cpus mask
>>> then it cannot do so until after the device has been registered. During
>>> registration the cpuidle_coupled_register_device will then see the empty
>>> coupled_cpus mask & do nothing. The only other ways around this would be
>>> for the driver to define its own per-cpu struct cpuidle_device (which as
>>> I state in the commit message seems wasteful when cpuidle already
>>> defined them), or for cpuidle_coupled_register_device to be called later
>>> after the driver had a chance to modify devices via the cpuidle_devices
>>> pointers.
>>
>> Yeah. I understand why you wanted to declare these cpu variables.
>>
>> The mips cpuidle driver sounds like a bit particular. I believe I need some
>> clarification on the behavior of the hardware to understand correctly the
>> driver. Could you explain how the couples act vs the cpu ? And why
>> cpu_sibling is used instead of cpu_possible_mask ?
>>
>>
>
> Sure. The CPUs that are coupled are actually VPEs (Virtual Processor
> Elements) within a single core. They share the compute resource (ALUs
> etc) of the core but have their own register state, ie. they're a form
> of simultaneous multithreading.
>
> Coherence within a MIPS Coherent Processing System is a property of the
> core rather than of an individual VPE (since the VPEs within a core
> share the same L1 caches). So for idle states which are non-coherent the
> VPEs within the core are coupled. That covers all idle states beyond a
> simple "wait" instruction - clock gating or powering down a core
> requires it to become non-coherent first.
>
> cpu_sibling_mask is already setup to indicate which CPUs (VPEs) are
> within the same core as each other, which is why it is simply copied for
> coupled_cpus.

Hi Paul,

thanks for the explanation.

   -- 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

[PATCH 10/10] cpuidle: cpuidle driver for MIPS CPS

[Paul Burton]

This patch introduces a cpuidle driver implementation for the MIPS
Coherent Processing System (ie. Coherence Manager, Cluster Power
Controller). It allows for use of the following idle states:

 - Coherent wait. This is the usual MIPS wait instruction.

 - Non-coherent wait. In this state a core will disable coherency with
   the rest of the system before running the wait instruction. This
   eliminates coherence interventions which would typically be used to
   keep cores coherent.

These two states lay the groundwork for deeper states to be implemented
later, since all deeper states require the core to become non-coherent.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-pm@vger.kernel.org
---
 drivers/cpuidle/Kconfig            |   5 +
 drivers/cpuidle/Kconfig.mips       |  14 +
 drivers/cpuidle/Makefile           |   3 +
 drivers/cpuidle/cpuidle-mips-cps.c | 545 +++++++++++++++++++++++++++++++++++++
 4 files changed, 567 insertions(+)
 create mode 100644 drivers/cpuidle/Kconfig.mips
 create mode 100644 drivers/cpuidle/cpuidle-mips-cps.c

diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
index b3fb81d..11ff281 100644
--- a/drivers/cpuidle/Kconfig
+++ b/drivers/cpuidle/Kconfig
@@ -35,6 +35,11 @@ depends on ARM
 source "drivers/cpuidle/Kconfig.arm"
 endmenu
 
+menu "MIPS CPU Idle Drivers"
+depends on MIPS
+source "drivers/cpuidle/Kconfig.mips"
+endmenu
+
 endif
 
 config ARCH_NEEDS_CPU_IDLE_COUPLED
diff --git a/drivers/cpuidle/Kconfig.mips b/drivers/cpuidle/Kconfig.mips
new file mode 100644
index 0000000..dc96691
--- /dev/null
+++ b/drivers/cpuidle/Kconfig.mips
@@ -0,0 +1,14 @@
+#
+# MIPS CPU Idle drivers
+#
+
+config MIPS_CPS_CPUIDLE
+	bool "Support for MIPS Coherent Processing Systems"
+	depends on SYS_SUPPORTS_MIPS_CPS && CPU_MIPSR2 && !MIPS_MT_SMTC
+	select ARCH_NEEDS_CPU_IDLE_COUPLED if MIPS_MT
+	select MIPS_CM
+	help
+	  Select this option to enable CPU idle driver for systems based
+	  around the MIPS Coherent Processing System architecture - that
+	  is, those with a Coherence Manager & optionally a Cluster
+	  Power Controller.
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index 527be28..693cd95 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -13,3 +13,6 @@ obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
 obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o
 obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
 obj-$(CONFIG_ARM_AT91_CPUIDLE)          += cpuidle-at91.o
+
+# MIPS SoC drivers
+obj-$(CONFIG_MIPS_CPS_CPUIDLE)		+= cpuidle-mips-cps.o
diff --git a/drivers/cpuidle/cpuidle-mips-cps.c b/drivers/cpuidle/cpuidle-mips-cps.c
new file mode 100644
index 0000000..a78bfb4
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-mips-cps.c
@@ -0,0 +1,545 @@
+/*
+ * Copyright (C) 2013 Imagination Technologies
+ * Author: Paul Burton <paul.burton@imgtec.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/cpuidle.h>
+#include <linux/init.h>
+#include <linux/kconfig.h>
+#include <linux/slab.h>
+
+#include <asm/cacheflush.h>
+#include <asm/cacheops.h>
+#include <asm/idle.h>
+#include <asm/mips-cm.h>
+#include <asm/mipsmtregs.h>
+#include <asm/uasm.h>
+
+/*
+ * The CM & CPC can only handle coherence & power control on a per-core basis,
+ * thus in an MT system the VPEs within each core are coupled and can only
+ * enter or exit states requiring CM or CPC assistance in unison.
+ */
+#ifdef CONFIG_MIPS_MT
+# define coupled_coherence cpu_has_mipsmt
+#else
+# define coupled_coherence 0
+#endif
+
+/*
+ * cps_nc_entry_fn - type of a generated non-coherent state entry function
+ * @vpe_mask: a bitmap of online coupled VPEs, excluding this one
+ * @online: the count of online coupled VPEs (weight of vpe_mask + 1)
+ *
+ * The code entering & exiting non-coherent states is generated at runtime
+ * using uasm, in order to ensure that the compiler cannot insert a stray
+ * memory access at an unfortunate time and to allow the generation of optimal
+ * core-specific code particularly for cache routines. If coupled_coherence
+ * is non-zero, returns the number of VPEs that were in the wait state at the
+ * point this VPE left it. Returns garbage if coupled_coherence is zero.
+ */
+typedef unsigned (*cps_nc_entry_fn)(unsigned vpe_mask, unsigned online);
+
+/*
+ * The entry point of the generated non-coherent wait entry/exit function.
+ * Actually per-core rather than per-CPU.
+ */
+static DEFINE_PER_CPU_READ_MOSTLY(cps_nc_entry_fn, ncwait_asm_enter);
+
+/*
+ * Indicates the number of coupled VPEs ready to operate in a non-coherent
+ * state. Actually per-core rather than per-CPU.
+ */
+static DEFINE_PER_CPU_ALIGNED(u32, nc_ready_count);
+
+/* A somewhat arbitrary number of labels & relocs for uasm */
+static struct uasm_label labels[32] __initdata;
+static struct uasm_reloc relocs[32] __initdata;
+
+/* CPU dependant sync types */
+static unsigned stype_intervention;
+static unsigned stype_memory;
+static unsigned stype_ordering;
+
+enum mips_reg {
+	zero, at, v0, v1, a0, a1, a2, a3,
+	t0, t1, t2, t3, t4, t5, t6, t7,
+	s0, s1, s2, s3, s4, s5, s6, s7,
+	t8, t9, k0, k1, gp, sp, fp, ra,
+};
+
+static int cps_ncwait_enter(struct cpuidle_device *dev,
+			    struct cpuidle_driver *drv, int index)
+{
+	unsigned core = cpu_data[dev->cpu].core;
+	unsigned online, first_cpu, num_left;
+	cpumask_var_t coupled_mask, vpe_mask;
+
+	if (!alloc_cpumask_var(&coupled_mask, GFP_KERNEL))
+		return -ENOMEM;
+
+	if (!alloc_cpumask_var(&vpe_mask, GFP_KERNEL)) {
+		free_cpumask_var(coupled_mask);
+		return -ENOMEM;
+	}
+
+	/* Calculate which coupled CPUs (VPEs) are online */
+#ifdef CONFIG_MIPS_MT
+	cpumask_and(coupled_mask, cpu_online_mask, &dev->coupled_cpus);
+	first_cpu = cpumask_first(coupled_mask);
+	online = cpumask_weight(coupled_mask);
+	cpumask_clear_cpu(dev->cpu, coupled_mask);
+	cpumask_shift_right(vpe_mask, coupled_mask,
+			    cpumask_first(&dev->coupled_cpus));
+#else
+	cpumask_clear(coupled_mask);
+	cpumask_clear(vpe_mask);
+	first_cpu = dev->cpu;
+	online = 1;
+#endif
+
+	/*
+	 * Run the generated entry code. Note that we assume the number of VPEs
+	 * within this core does not exceed the width in bits of a long. Since
+	 * MVPConf0.PVPE is 4 bits wide this seems like a safe assumption.
+	 */
+	num_left = per_cpu(ncwait_asm_enter, core)(vpe_mask->bits[0], online);
+
+	/*
+	 * If this VPE is the first to leave the non-coherent wait state then
+	 * it needs to wake up any coupled VPEs still running their wait
+	 * instruction so that they return to cpuidle, which can then complete
+	 * coordination between the coupled VPEs & provide the governor with
+	 * a chance to reflect on the length of time the VPEs were in the
+	 * idle state.
+	 */
+	if (coupled_coherence && (num_left == online))
+		arch_send_call_function_ipi_mask(coupled_mask);
+
+	free_cpumask_var(vpe_mask);
+	free_cpumask_var(coupled_mask);
+	return index;
+}
+
+static struct cpuidle_driver cps_driver = {
+	.name			= "cpc_cpuidle",
+	.owner			= THIS_MODULE,
+	.states = {
+		MIPS_CPUIDLE_WAIT_STATE,
+		{
+			.enter	= cps_ncwait_enter,
+			.exit_latency		= 200,
+			.target_residency	= 450,
+			.flags	= CPUIDLE_FLAG_TIME_VALID,
+			.name	= "nc-wait",
+			.desc	= "non-coherent MIPS wait",
+		},
+	},
+	.state_count		= 2,
+	.safe_state_index	= 0,
+};
+
+static void __init cps_gen_cache_routine(u32 **pp, struct uasm_label **pl,
+					 struct uasm_reloc **pr,
+					 const struct cache_desc *cache,
+					 unsigned op, int lbl)
+{
+	unsigned cache_size = cache->ways << cache->waybit;
+	unsigned i;
+	const unsigned unroll_lines = 32;
+
+	/* If the cache isn't present this function has it easy */
+	if (cache->flags & MIPS_CACHE_NOT_PRESENT)
+		return;
+
+	/* Load base address */
+	UASM_i_LA(pp, t0, (long)CKSEG0);
+
+	/* Calculate end address */
+	if (cache_size < 0x8000)
+		uasm_i_addiu(pp, t1, t0, cache_size);
+	else
+		UASM_i_LA(pp, t1, (long)(CKSEG0 + cache_size));
+
+	/* Start of cache op loop */
+	uasm_build_label(pl, *pp, lbl);
+
+	/* Generate the cache ops */
+	for (i = 0; i < unroll_lines; i++)
+		uasm_i_cache(pp, op, i * cache->linesz, t0);
+
+	/* Update the base address */
+	uasm_i_addiu(pp, t0, t0, unroll_lines * cache->linesz);
+
+	/* Loop if we haven't reached the end address yet */
+	uasm_il_bne(pp, pr, t0, t1, lbl);
+	uasm_i_nop(pp);
+}
+
+static void * __init cps_gen_entry_code(struct cpuidle_device *dev)
+{
+	unsigned core = cpu_data[dev->cpu].core;
+	struct uasm_label *l = labels;
+	struct uasm_reloc *r = relocs;
+	u32 *buf, *p;
+	const unsigned r_vpemask = a0;
+	const unsigned r_online = a1;
+	const unsigned r_pcount = t6;
+	const unsigned r_pcohctl = t7;
+	const unsigned max_instrs = 256;
+	enum {
+		lbl_incready = 1,
+		lbl_lastvpe,
+		lbl_vpehalt_loop,
+		lbl_vpehalt_poll,
+		lbl_vpehalt_next,
+		lbl_disable_coherence,
+		lbl_invicache,
+		lbl_flushdcache,
+		lbl_vpeactivate_loop,
+		lbl_vpeactivate_next,
+		lbl_wait,
+		lbl_decready,
+	};
+
+	/* Allocate a buffer to hold the generated code */
+	p = buf = kcalloc(max_instrs, sizeof(u32), GFP_KERNEL);
+	if (!buf)
+		return NULL;
+
+	/* Clear labels & relocs ready for (re)use */
+	memset(labels, 0, sizeof(labels));
+	memset(relocs, 0, sizeof(relocs));
+
+	/*
+	 * Load address of the CM GCR_CL_COHERENCE register. This is done early
+	 * because it's needed in both the enable & disable coherence steps but
+	 * in the coupled case the enable step will only run on one VPE.
+	 */
+	UASM_i_LA(&p, r_pcohctl, (long)addr_gcr_cl_coherence());
+
+	if (coupled_coherence) {
+		/* Load address of nc_ready_count */
+		UASM_i_LA(&p, r_pcount, (long)&per_cpu(nc_ready_count, core));
+
+		/* Increment nc_ready_count */
+		uasm_build_label(&l, p, lbl_incready);
+		uasm_i_sync(&p, stype_ordering);
+		uasm_i_ll(&p, t1, 0, r_pcount);
+		uasm_i_addiu(&p, t2, t1, 1);
+		uasm_i_sc(&p, t2, 0, r_pcount);
+		uasm_il_beqz(&p, &r, t2, lbl_incready);
+		uasm_i_addiu(&p, t1, t1, 1);
+
+		/*
+		 * If this is the last VPE to become ready for non-coherence
+		 * then it should branch below.
+		 */
+		uasm_il_beq(&p, &r, t1, r_online, lbl_lastvpe);
+		uasm_i_nop(&p);
+
+		/*
+		 * Otherwise this is not the last VPE to become ready for
+		 * non-coherence. It needs to wait until coherence has been
+		 * disabled before executing a wait instruction, otherwise it
+		 * may return from wait quickly & re-enable coherence causing
+		 * a race with the VPE disabling coherence. It can't simply
+		 * poll the CPC sequencer for a non-coherent state as that
+		 * would race with any other VPE which may spot the
+		 * non-coherent state, run wait, return quickly & re-enable
+		 * coherence before this VPE ever saw the non-coherent state.
+		 * Instead this VPE will halt its TC such that it ceases to
+		 * execute for the moment.
+		 */
+		uasm_i_addiu(&p, t0, zero, TCHALT_H);
+		uasm_i_mtc0(&p, t0, 2, 4); /* TCHalt */
+
+		/* instruction_hazard(), to ensure the TC halts */
+		UASM_i_LA(&p, t0, (long)p + 12);
+		uasm_i_jr_hb(&p, t0);
+		uasm_i_nop(&p);
+
+		/*
+		 * The VPE which disables coherence will then clear the halt
+		 * bit for this VPE's TC once coherence has been disabled and
+		 * it can safely proceed to execute the wait instruction.
+		 */
+		uasm_il_b(&p, &r, lbl_wait);
+		uasm_i_nop(&p);
+
+		/*
+		 * The last VPE to increment nc_ready_count will continue from
+		 * here and must spin until all other VPEs within the core have
+		 * been halted, at which point it can be sure that it is safe
+		 * to disable coherence.
+		 *
+		 *   t0: number of VPEs left to handle
+		 *   t1: (shifted) mask of online VPEs
+		 *   t2: current VPE index
+		 */
+		uasm_build_label(&l, p, lbl_lastvpe);
+		uasm_i_addiu(&p, t0, r_online, -1);
+		uasm_il_beqz(&p, &r, t0, lbl_disable_coherence);
+		uasm_i_move(&p, t1, r_vpemask);
+		uasm_i_move(&p, t2, zero);
+
+		/*
+		 * Now loop through all VPEs within the core checking whether
+		 * they are online & not this VPE, which can be determined by
+		 * checking the vpe_mask argument. If a VPE is offline or is
+		 * this VPE, skip it.
+		 */
+		uasm_build_label(&l, p, lbl_vpehalt_loop);
+		uasm_i_andi(&p, t3, t1, 1);
+		uasm_il_beqz(&p, &r, t3, lbl_vpehalt_next);
+
+		/* settc(vpe) */
+		uasm_i_mfc0(&p, t3, 1, 1); /* VPEControl */
+		uasm_i_ins(&p, t3, t2, 0, 8);
+		uasm_i_mtc0(&p, t3, 1, 1); /* VPEControl */
+		uasm_i_ehb(&p);
+
+		/*
+		 * It's very likely that the VPE has already halted itself
+		 * by now, but there's theoretically a chance that it may not
+		 * have. Wait until the VPE's TC is halted.
+		 */
+		uasm_build_label(&l, p, lbl_vpehalt_poll);
+		uasm_i_mftc0(&p, t3, 2, 4); /* TCHalt */
+		uasm_il_beqz(&p, &r, t3, lbl_vpehalt_poll);
+		uasm_i_nop(&p);
+
+		/* Decrement the count of VPEs to be handled */
+		uasm_i_addiu(&p, t0, t0, -1);
+
+		/* Proceed to the next VPE, if there is one */
+		uasm_build_label(&l, p, lbl_vpehalt_next);
+		uasm_i_srl(&p, t1, t1, 1);
+		uasm_il_bnez(&p, &r, t0, lbl_vpehalt_loop);
+		uasm_i_addiu(&p, t2, t2, 1);
+	}
+
+	/*
+	 * This is the point of no return - this VPE will now proceed to
+	 * disable coherence. At this point we *must* be sure that no other
+	 * VPE within the core will interfere with the L1 dcache.
+	 */
+	uasm_build_label(&l, p, lbl_disable_coherence);
+
+	/* Completion barrier */
+	uasm_i_sync(&p, stype_memory);
+
+	/* Invalidate the L1 icache */
+	cps_gen_cache_routine(&p, &l, &r, &cpu_data[dev->cpu].icache,
+			      Index_Invalidate_I, lbl_invicache);
+
+	/* Writeback & invalidate the L1 dcache */
+	cps_gen_cache_routine(&p, &l, &r, &cpu_data[dev->cpu].dcache,
+			      Index_Writeback_Inv_D, lbl_flushdcache);
+
+	/*
+	 * Disable all but self interventions. The load from COHCTL is defined
+	 * by the interAptiv & proAptiv SUMs as ensuring that the operation
+	 * resulting from the preceeding store is complete.
+	 */
+	uasm_i_addiu(&p, t0, zero, 1 << cpu_data[dev->cpu].core);
+	uasm_i_sw(&p, t0, 0, r_pcohctl);
+	uasm_i_lw(&p, t0, 0, r_pcohctl);
+
+	/* Sync to ensure previous interventions are complete */
+	uasm_i_sync(&p, stype_intervention);
+
+	/* Disable coherence */
+	uasm_i_sw(&p, zero, 0, r_pcohctl);
+	uasm_i_lw(&p, t0, 0, r_pcohctl);
+
+	if (coupled_coherence) {
+		/*
+		 * Now that coherence is disabled it is safe for all VPEs to
+		 * proceed with executing their wait instruction, so this VPE
+		 * will go ahead and clear the halt bit of the TCs associated
+		 * with all other online VPEs within the core. Start by
+		 * initialising variables used throughout the loop, and
+		 * skipping the loop entirely if there are no VPEs to handle.
+		 *
+		 *   t0: number of VPEs left to handle
+		 *   t1: (shifted) mask of online VPEs
+		 *   t2: current VPE index
+		 */
+		uasm_i_addiu(&p, t0, r_online, -1);
+		uasm_il_beqz(&p, &r, t0, lbl_wait);
+		uasm_i_move(&p, t1, r_vpemask);
+		uasm_i_move(&p, t2, zero);
+
+		/*
+		 * Now loop through all VPEs within the core checking whether
+		 * they are online & not this VPE, which can be determined by
+		 * checking the vpe_mask argument. If a VPE is offline or is
+		 * this VPE, skip it.
+		 */
+		uasm_build_label(&l, p, lbl_vpeactivate_loop);
+		uasm_i_andi(&p, t3, t1, 1);
+		uasm_il_beqz(&p, &r, t3, lbl_vpeactivate_next);
+
+		/* settc(vpe) */
+		uasm_i_mfc0(&p, t3, 1, 1); /* VPEControl */
+		uasm_i_ins(&p, t3, t2, 0, 8);
+		uasm_i_mtc0(&p, t3, 1, 1); /* VPEControl */
+		uasm_i_ehb(&p);
+
+		/* Clear TCHalt */
+		uasm_i_mttc0(&p, zero, 2, 4); /* TCHalt */
+
+		/* Decrement the count of VPEs to be handled */
+		uasm_i_addiu(&p, t0, t0, -1);
+
+		/* Proceed to the next VPE, if there is one */
+		uasm_build_label(&l, p, lbl_vpeactivate_next);
+		uasm_i_srl(&p, t1, t1, 1);
+		uasm_il_bnez(&p, &r, t0, lbl_vpeactivate_loop);
+		uasm_i_addiu(&p, t2, t2, 1);
+	}
+
+	/* Now perform our wait */
+	uasm_build_label(&l, p, lbl_wait);
+	uasm_i_wait(&p, 0);
+
+	/*
+	 * Re-enable coherence. Note that all coupled VPEs will run this, the
+	 * first will actually re-enable coherence & the rest will just be
+	 * performing a rather unusual nop.
+	 */
+	uasm_i_addiu(&p, t0, zero, CM_GCR_Cx_COHERENCE_COHDOMAINEN_MSK);
+	uasm_i_sw(&p, t0, 0, r_pcohctl);
+	uasm_i_lw(&p, t0, 0, r_pcohctl);
+
+	/* Ordering barrier */
+	uasm_i_sync(&p, stype_ordering);
+
+	if (coupled_coherence) {
+		/* Decrement nc_ready_count */
+		uasm_build_label(&l, p, lbl_decready);
+		uasm_i_sync(&p, stype_ordering);
+		uasm_i_ll(&p, t1, 0, r_pcount);
+		uasm_i_addiu(&p, t2, t1, -1);
+		uasm_i_sc(&p, t2, 0, r_pcount);
+		uasm_il_beqz(&p, &r, t2, lbl_decready);
+		uasm_i_move(&p, v0, t1);
+	}
+
+	/* The core is coherent, time to return to C code */
+	uasm_i_jr(&p, ra);
+	uasm_i_nop(&p);
+
+	/* Ensure the code didn't exceed the resources allocated for it */
+	BUG_ON((p - buf) > max_instrs);
+	BUG_ON((l - labels) > ARRAY_SIZE(labels));
+	BUG_ON((r - relocs) > ARRAY_SIZE(relocs));
+
+	/* Patch branch offsets */
+	uasm_resolve_relocs(relocs, labels);
+
+	/* Flush the icache */
+	local_flush_icache_range((unsigned long)buf, (unsigned long)p);
+
+	return buf;
+}
+
+static void __init cps_cpuidle_unregister(void)
+{
+	int cpu;
+	struct cpuidle_device *device;
+	cps_nc_entry_fn *fn;
+
+	for_each_possible_cpu(cpu) {
+		device = &per_cpu(cpuidle_dev, cpu);
+		cpuidle_unregister_device(device);
+
+		/* Free entry code */
+		fn = &per_cpu(ncwait_asm_enter, cpu_data[cpu].core);
+		kfree(*fn);
+		*fn = NULL;
+	}
+
+	cpuidle_unregister_driver(&cps_driver);
+}
+
+static int __init cps_cpuidle_init(void)
+{
+	int err, cpu, core, i;
+	struct cpuidle_device *device;
+	void *core_entry;
+
+	/*
+	 * If interrupts were enabled whilst running the wait instruction then
+	 * the VPE may end up processing interrupts whilst non-coherent.
+	 */
+	if (cpu_wait != r4k_wait_irqoff) {
+		pr_warn("cpuidle-cps requires that masked interrupts restart the CPU pipeline following a wait\n");
+		return -ENODEV;
+	}
+
+	/* Detect appropriate sync types for the system */
+	switch (current_cpu_data.cputype) {
+	case CPU_INTERAPTIV:
+	case CPU_PROAPTIV:
+		stype_intervention = 0x2;
+		stype_memory = 0x3;
+		stype_ordering = 0x10;
+		break;
+
+	default:
+		pr_warn("cpuidle-cps using heavyweight sync 0\n");
+	}
+
+	/*
+	 * Set the coupled flag on the appropriate states if this system
+	 * requires it.
+	 */
+	if (coupled_coherence)
+		for (i = 1; i < cps_driver.state_count; i++)
+			cps_driver.states[i].flags |= CPUIDLE_FLAG_COUPLED;
+
+	err = cpuidle_register_driver(&cps_driver);
+	if (err) {
+		pr_err("Failed to register CPS cpuidle driver\n");
+		return err;
+	}
+
+	for_each_possible_cpu(cpu) {
+		core = cpu_data[cpu].core;
+		device = &per_cpu(cpuidle_dev, cpu);
+		device->cpu = cpu;
+#ifdef CONFIG_MIPS_MT
+		cpumask_copy(&device->coupled_cpus, &cpu_sibling_map[cpu]);
+#endif
+		if (!per_cpu(ncwait_asm_enter, core)) {
+			core_entry = cps_gen_entry_code(device);
+			if (!core_entry) {
+				pr_err("Failed to generate core %u entry\n",
+				       core);
+				err = -ENOMEM;
+				goto err_out;
+			}
+			per_cpu(ncwait_asm_enter, core) = core_entry;
+		}
+
+		err = cpuidle_register_device(device);
+		if (err) {
+			pr_err("Failed to register CPU%d cpuidle device\n",
+			       cpu);
+			goto err_out;
+		}
+	}
+
+	return 0;
+err_out:
+	cps_cpuidle_unregister();
+	return err;
+}
+device_initcall(cps_cpuidle_init);
-- 
1.8.4.2

Re: [PATCH 10/10] cpuidle: cpuidle driver for MIPS CPS

[Daniel Lezcano]

On 01/15/2014 02:55 PM, Paul Burton wrote:
> This patch introduces a cpuidle driver implementation for the MIPS
> Coherent Processing System (ie. Coherence Manager, Cluster Power
> Controller). It allows for use of the following idle states:
>
>   - Coherent wait. This is the usual MIPS wait instruction.
>
>   - Non-coherent wait. In this state a core will disable coherency with
>     the rest of the system before running the wait instruction. This
>     eliminates coherence interventions which would typically be used to
>     keep cores coherent.
>
> These two states lay the groundwork for deeper states to be implemented
> later, since all deeper states require the core to become non-coherent.
>
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: linux-pm@vger.kernel.org
> ---
>   drivers/cpuidle/Kconfig            |   5 +
>   drivers/cpuidle/Kconfig.mips       |  14 +
>   drivers/cpuidle/Makefile           |   3 +
>   drivers/cpuidle/cpuidle-mips-cps.c | 545 +++++++++++++++++++++++++++++++++++++
>   4 files changed, 567 insertions(+)
>   create mode 100644 drivers/cpuidle/Kconfig.mips
>   create mode 100644 drivers/cpuidle/cpuidle-mips-cps.c
>
> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
> index b3fb81d..11ff281 100644
> --- a/drivers/cpuidle/Kconfig
> +++ b/drivers/cpuidle/Kconfig
> @@ -35,6 +35,11 @@ depends on ARM
>   source "drivers/cpuidle/Kconfig.arm"
>   endmenu
>
> +menu "MIPS CPU Idle Drivers"
> +depends on MIPS
> +source "drivers/cpuidle/Kconfig.mips"
> +endmenu
> +
>   endif
>
>   config ARCH_NEEDS_CPU_IDLE_COUPLED
> diff --git a/drivers/cpuidle/Kconfig.mips b/drivers/cpuidle/Kconfig.mips
> new file mode 100644
> index 0000000..dc96691
> --- /dev/null
> +++ b/drivers/cpuidle/Kconfig.mips
> @@ -0,0 +1,14 @@
> +#
> +# MIPS CPU Idle drivers
> +#
> +
> +config MIPS_CPS_CPUIDLE
> +	bool "Support for MIPS Coherent Processing Systems"
> +	depends on SYS_SUPPORTS_MIPS_CPS && CPU_MIPSR2 && !MIPS_MT_SMTC
> +	select ARCH_NEEDS_CPU_IDLE_COUPLED if MIPS_MT
> +	select MIPS_CM
> +	help
> +	  Select this option to enable CPU idle driver for systems based
> +	  around the MIPS Coherent Processing System architecture - that
> +	  is, those with a Coherence Manager & optionally a Cluster
> +	  Power Controller.
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index 527be28..693cd95 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -13,3 +13,6 @@ obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
>   obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o
>   obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
>   obj-$(CONFIG_ARM_AT91_CPUIDLE)          += cpuidle-at91.o
> +
> +# MIPS SoC drivers
> +obj-$(CONFIG_MIPS_CPS_CPUIDLE)		+= cpuidle-mips-cps.o
> diff --git a/drivers/cpuidle/cpuidle-mips-cps.c b/drivers/cpuidle/cpuidle-mips-cps.c
> new file mode 100644
> index 0000000..a78bfb4
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-mips-cps.c
> @@ -0,0 +1,545 @@
> +/*
> + * Copyright (C) 2013 Imagination Technologies
> + * Author: Paul Burton <paul.burton@imgtec.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/cpuidle.h>
> +#include <linux/init.h>
> +#include <linux/kconfig.h>
> +#include <linux/slab.h>
> +
> +#include <asm/cacheflush.h>
> +#include <asm/cacheops.h>
> +#include <asm/idle.h>
> +#include <asm/mips-cm.h>
> +#include <asm/mipsmtregs.h>
> +#include <asm/uasm.h>

The convention is to not include headers from arch. These headers 
shouldn't appear in this driver.

> +/*
> + * The CM & CPC can only handle coherence & power control on a per-core basis,
> + * thus in an MT system the VPEs within each core are coupled and can only
> + * enter or exit states requiring CM or CPC assistance in unison.
> + */
> +#ifdef CONFIG_MIPS_MT
> +# define coupled_coherence cpu_has_mipsmt
> +#else
> +# define coupled_coherence 0
> +#endif
> +
> +/*
> + * cps_nc_entry_fn - type of a generated non-coherent state entry function
> + * @vpe_mask: a bitmap of online coupled VPEs, excluding this one
> + * @online: the count of online coupled VPEs (weight of vpe_mask + 1)
> + *
> + * The code entering & exiting non-coherent states is generated at runtime
> + * using uasm, in order to ensure that the compiler cannot insert a stray
> + * memory access at an unfortunate time and to allow the generation of optimal
> + * core-specific code particularly for cache routines. If coupled_coherence
> + * is non-zero, returns the number of VPEs that were in the wait state at the
> + * point this VPE left it. Returns garbage if coupled_coherence is zero.
> + */
> +typedef unsigned (*cps_nc_entry_fn)(unsigned vpe_mask, unsigned online);
> +
> +/*
> + * The entry point of the generated non-coherent wait entry/exit function.
> + * Actually per-core rather than per-CPU.
> + */
> +static DEFINE_PER_CPU_READ_MOSTLY(cps_nc_entry_fn, ncwait_asm_enter);
> +
> +/*
> + * Indicates the number of coupled VPEs ready to operate in a non-coherent
> + * state. Actually per-core rather than per-CPU.
> + */
> +static DEFINE_PER_CPU_ALIGNED(u32, nc_ready_count);
> +
> +/* A somewhat arbitrary number of labels & relocs for uasm */
> +static struct uasm_label labels[32] __initdata;
> +static struct uasm_reloc relocs[32] __initdata;
> +
> +/* CPU dependant sync types */
> +static unsigned stype_intervention;
> +static unsigned stype_memory;
> +static unsigned stype_ordering;
> +
> +enum mips_reg {
> +	zero, at, v0, v1, a0, a1, a2, a3,
> +	t0, t1, t2, t3, t4, t5, t6, t7,
> +	s0, s1, s2, s3, s4, s5, s6, s7,
> +	t8, t9, k0, k1, gp, sp, fp, ra,
> +};
> +
> +static int cps_ncwait_enter(struct cpuidle_device *dev,
> +			    struct cpuidle_driver *drv, int index)
> +{
> +	unsigned core = cpu_data[dev->cpu].core;
> +	unsigned online, first_cpu, num_left;
> +	cpumask_var_t coupled_mask, vpe_mask;
> +
> +	if (!alloc_cpumask_var(&coupled_mask, GFP_KERNEL))
> +		return -ENOMEM;
> +
> +	if (!alloc_cpumask_var(&vpe_mask, GFP_KERNEL)) {
> +		free_cpumask_var(coupled_mask);
> +		return -ENOMEM;
> +	}

You can't do that in this function where the local irqs are disabled. 
IMO, if you set CONFIG_CPUMASK_OFFSTACK and CONFIG_DEBUG_ATOMIC_SLEEP, 
you should see a kernel warning.

> +	/* Calculate which coupled CPUs (VPEs) are online */
> +#ifdef CONFIG_MIPS_MT
> +	cpumask_and(coupled_mask, cpu_online_mask, &dev->coupled_cpus);
> +	first_cpu = cpumask_first(coupled_mask);
> +	online = cpumask_weight(coupled_mask);
> +	cpumask_clear_cpu(dev->cpu, coupled_mask);
> +	cpumask_shift_right(vpe_mask, coupled_mask,
> +			    cpumask_first(&dev->coupled_cpus));

What is the purpose of this computation ?

> +#else
> +	cpumask_clear(coupled_mask);
> +	cpumask_clear(vpe_mask);
> +	first_cpu = dev->cpu;
> +	online = 1;
> +#endif

first_cpu is not used.

> +	/*
> +	 * Run the generated entry code. Note that we assume the number of VPEs
> +	 * within this core does not exceed the width in bits of a long. Since
> +	 * MVPConf0.PVPE is 4 bits wide this seems like a safe assumption.
> +	 */
> +	num_left = per_cpu(ncwait_asm_enter, core)(vpe_mask->bits[0], online);
> +
> +	/*
> +	 * If this VPE is the first to leave the non-coherent wait state then
> +	 * it needs to wake up any coupled VPEs still running their wait
> +	 * instruction so that they return to cpuidle,

Why is it needed ? Can't the other cpus stay idle ?

>        * which can then complete
> +	 * coordination between the coupled VPEs & provide the governor with
> +	 * a chance to reflect on the length of time the VPEs were in the
> +	 * idle state.
> +	 */
> +	if (coupled_coherence && (num_left == online))
> +		arch_send_call_function_ipi_mask(coupled_mask);

Except there is no choice due to hardware limitations, I don't think 
this is valid.

> +	free_cpumask_var(vpe_mask);
> +	free_cpumask_var(coupled_mask);
> +	return index;
> +}
> +
> +static struct cpuidle_driver cps_driver = {
> +	.name			= "cpc_cpuidle",
> +	.owner			= THIS_MODULE,
> +	.states = {
> +		MIPS_CPUIDLE_WAIT_STATE,
> +		{
> +			.enter	= cps_ncwait_enter,
> +			.exit_latency		= 200,
> +			.target_residency	= 450,
> +			.flags	= CPUIDLE_FLAG_TIME_VALID,
> +			.name	= "nc-wait",
> +			.desc	= "non-coherent MIPS wait",
> +		},
> +	},
> +	.state_count		= 2,
> +	.safe_state_index	= 0,
> +};



> +static void __init cps_gen_cache_routine(u32 **pp, struct uasm_label **pl,
> +					 struct uasm_reloc **pr,
> +					 const struct cache_desc *cache,
> +					 unsigned op, int lbl)
> +{
> +	unsigned cache_size = cache->ways << cache->waybit;
> +	unsigned i;
> +	const unsigned unroll_lines = 32;
> +
> +	/* If the cache isn't present this function has it easy */
> +	if (cache->flags & MIPS_CACHE_NOT_PRESENT)
> +		return;
> +
> +	/* Load base address */
> +	UASM_i_LA(pp, t0, (long)CKSEG0);
> +
> +	/* Calculate end address */
> +	if (cache_size < 0x8000)
> +		uasm_i_addiu(pp, t1, t0, cache_size);
> +	else
> +		UASM_i_LA(pp, t1, (long)(CKSEG0 + cache_size));
> +
> +	/* Start of cache op loop */
> +	uasm_build_label(pl, *pp, lbl);
> +
> +	/* Generate the cache ops */
> +	for (i = 0; i < unroll_lines; i++)
> +		uasm_i_cache(pp, op, i * cache->linesz, t0);
> +
> +	/* Update the base address */
> +	uasm_i_addiu(pp, t0, t0, unroll_lines * cache->linesz);
> +
> +	/* Loop if we haven't reached the end address yet */
> +	uasm_il_bne(pp, pr, t0, t1, lbl);
> +	uasm_i_nop(pp);
> +}
> +
> +static void * __init cps_gen_entry_code(struct cpuidle_device *dev)
> +{
> +	unsigned core = cpu_data[dev->cpu].core;
> +	struct uasm_label *l = labels;
> +	struct uasm_reloc *r = relocs;
> +	u32 *buf, *p;
> +	const unsigned r_vpemask = a0;
> +	const unsigned r_online = a1;
> +	const unsigned r_pcount = t6;
> +	const unsigned r_pcohctl = t7;
> +	const unsigned max_instrs = 256;
> +	enum {
> +		lbl_incready = 1,
> +		lbl_lastvpe,
> +		lbl_vpehalt_loop,
> +		lbl_vpehalt_poll,
> +		lbl_vpehalt_next,
> +		lbl_disable_coherence,
> +		lbl_invicache,
> +		lbl_flushdcache,
> +		lbl_vpeactivate_loop,
> +		lbl_vpeactivate_next,
> +		lbl_wait,
> +		lbl_decready,
> +	};
> +
> +	/* Allocate a buffer to hold the generated code */
> +	p = buf = kcalloc(max_instrs, sizeof(u32), GFP_KERNEL);
> +	if (!buf)
> +		return NULL;
> +
> +	/* Clear labels & relocs ready for (re)use */
> +	memset(labels, 0, sizeof(labels));
> +	memset(relocs, 0, sizeof(relocs));
> +
> +	/*
> +	 * Load address of the CM GCR_CL_COHERENCE register. This is done early
> +	 * because it's needed in both the enable & disable coherence steps but
> +	 * in the coupled case the enable step will only run on one VPE.
> +	 */
> +	UASM_i_LA(&p, r_pcohctl, (long)addr_gcr_cl_coherence());
> +
> +	if (coupled_coherence) {
> +		/* Load address of nc_ready_count */
> +		UASM_i_LA(&p, r_pcount, (long)&per_cpu(nc_ready_count, core));
> +
> +		/* Increment nc_ready_count */
> +		uasm_build_label(&l, p, lbl_incready);
> +		uasm_i_sync(&p, stype_ordering);
> +		uasm_i_ll(&p, t1, 0, r_pcount);
> +		uasm_i_addiu(&p, t2, t1, 1);
> +		uasm_i_sc(&p, t2, 0, r_pcount);
> +		uasm_il_beqz(&p, &r, t2, lbl_incready);
> +		uasm_i_addiu(&p, t1, t1, 1);
> +
> +		/*
> +		 * If this is the last VPE to become ready for non-coherence
> +		 * then it should branch below.
> +		 */
> +		uasm_il_beq(&p, &r, t1, r_online, lbl_lastvpe);
> +		uasm_i_nop(&p);
> +
> +		/*
> +		 * Otherwise this is not the last VPE to become ready for
> +		 * non-coherence. It needs to wait until coherence has been
> +		 * disabled before executing a wait instruction, otherwise it
> +		 * may return from wait quickly & re-enable coherence causing
> +		 * a race with the VPE disabling coherence. It can't simply
> +		 * poll the CPC sequencer for a non-coherent state as that
> +		 * would race with any other VPE which may spot the
> +		 * non-coherent state, run wait, return quickly & re-enable
> +		 * coherence before this VPE ever saw the non-coherent state.
> +		 * Instead this VPE will halt its TC such that it ceases to
> +		 * execute for the moment.
> +		 */
> +		uasm_i_addiu(&p, t0, zero, TCHALT_H);
> +		uasm_i_mtc0(&p, t0, 2, 4); /* TCHalt */
> +
> +		/* instruction_hazard(), to ensure the TC halts */
> +		UASM_i_LA(&p, t0, (long)p + 12);
> +		uasm_i_jr_hb(&p, t0);
> +		uasm_i_nop(&p);
> +
> +		/*
> +		 * The VPE which disables coherence will then clear the halt
> +		 * bit for this VPE's TC once coherence has been disabled and
> +		 * it can safely proceed to execute the wait instruction.
> +		 */
> +		uasm_il_b(&p, &r, lbl_wait);
> +		uasm_i_nop(&p);
> +
> +		/*
> +		 * The last VPE to increment nc_ready_count will continue from
> +		 * here and must spin until all other VPEs within the core have
> +		 * been halted, at which point it can be sure that it is safe
> +		 * to disable coherence.
> +		 *
> +		 *   t0: number of VPEs left to handle
> +		 *   t1: (shifted) mask of online VPEs
> +		 *   t2: current VPE index
> +		 */
> +		uasm_build_label(&l, p, lbl_lastvpe);
> +		uasm_i_addiu(&p, t0, r_online, -1);
> +		uasm_il_beqz(&p, &r, t0, lbl_disable_coherence);
> +		uasm_i_move(&p, t1, r_vpemask);
> +		uasm_i_move(&p, t2, zero);
> +
> +		/*
> +		 * Now loop through all VPEs within the core checking whether
> +		 * they are online & not this VPE, which can be determined by
> +		 * checking the vpe_mask argument. If a VPE is offline or is
> +		 * this VPE, skip it.
> +		 */
> +		uasm_build_label(&l, p, lbl_vpehalt_loop);
> +		uasm_i_andi(&p, t3, t1, 1);
> +		uasm_il_beqz(&p, &r, t3, lbl_vpehalt_next);
> +
> +		/* settc(vpe) */
> +		uasm_i_mfc0(&p, t3, 1, 1); /* VPEControl */
> +		uasm_i_ins(&p, t3, t2, 0, 8);
> +		uasm_i_mtc0(&p, t3, 1, 1); /* VPEControl */
> +		uasm_i_ehb(&p);
> +
> +		/*
> +		 * It's very likely that the VPE has already halted itself
> +		 * by now, but there's theoretically a chance that it may not
> +		 * have. Wait until the VPE's TC is halted.
> +		 */
> +		uasm_build_label(&l, p, lbl_vpehalt_poll);
> +		uasm_i_mftc0(&p, t3, 2, 4); /* TCHalt */
> +		uasm_il_beqz(&p, &r, t3, lbl_vpehalt_poll);
> +		uasm_i_nop(&p);
> +
> +		/* Decrement the count of VPEs to be handled */
> +		uasm_i_addiu(&p, t0, t0, -1);
> +
> +		/* Proceed to the next VPE, if there is one */
> +		uasm_build_label(&l, p, lbl_vpehalt_next);
> +		uasm_i_srl(&p, t1, t1, 1);
> +		uasm_il_bnez(&p, &r, t0, lbl_vpehalt_loop);
> +		uasm_i_addiu(&p, t2, t2, 1);
> +	}
> +
> +	/*
> +	 * This is the point of no return - this VPE will now proceed to
> +	 * disable coherence. At this point we *must* be sure that no other
> +	 * VPE within the core will interfere with the L1 dcache.
> +	 */
> +	uasm_build_label(&l, p, lbl_disable_coherence);
> +
> +	/* Completion barrier */
> +	uasm_i_sync(&p, stype_memory);
> +
> +	/* Invalidate the L1 icache */
> +	cps_gen_cache_routine(&p, &l, &r, &cpu_data[dev->cpu].icache,
> +			      Index_Invalidate_I, lbl_invicache);
> +
> +	/* Writeback & invalidate the L1 dcache */
> +	cps_gen_cache_routine(&p, &l, &r, &cpu_data[dev->cpu].dcache,
> +			      Index_Writeback_Inv_D, lbl_flushdcache);
> +
> +	/*
> +	 * Disable all but self interventions. The load from COHCTL is defined
> +	 * by the interAptiv & proAptiv SUMs as ensuring that the operation
> +	 * resulting from the preceeding store is complete.
> +	 */
> +	uasm_i_addiu(&p, t0, zero, 1 << cpu_data[dev->cpu].core);
> +	uasm_i_sw(&p, t0, 0, r_pcohctl);
> +	uasm_i_lw(&p, t0, 0, r_pcohctl);
> +
> +	/* Sync to ensure previous interventions are complete */
> +	uasm_i_sync(&p, stype_intervention);
> +
> +	/* Disable coherence */
> +	uasm_i_sw(&p, zero, 0, r_pcohctl);
> +	uasm_i_lw(&p, t0, 0, r_pcohctl);
> +
> +	if (coupled_coherence) {
> +		/*
> +		 * Now that coherence is disabled it is safe for all VPEs to
> +		 * proceed with executing their wait instruction, so this VPE
> +		 * will go ahead and clear the halt bit of the TCs associated
> +		 * with all other online VPEs within the core. Start by
> +		 * initialising variables used throughout the loop, and
> +		 * skipping the loop entirely if there are no VPEs to handle.
> +		 *
> +		 *   t0: number of VPEs left to handle
> +		 *   t1: (shifted) mask of online VPEs
> +		 *   t2: current VPE index
> +		 */
> +		uasm_i_addiu(&p, t0, r_online, -1);
> +		uasm_il_beqz(&p, &r, t0, lbl_wait);
> +		uasm_i_move(&p, t1, r_vpemask);
> +		uasm_i_move(&p, t2, zero);
> +
> +		/*
> +		 * Now loop through all VPEs within the core checking whether
> +		 * they are online & not this VPE, which can be determined by
> +		 * checking the vpe_mask argument. If a VPE is offline or is
> +		 * this VPE, skip it.
> +		 */
> +		uasm_build_label(&l, p, lbl_vpeactivate_loop);
> +		uasm_i_andi(&p, t3, t1, 1);
> +		uasm_il_beqz(&p, &r, t3, lbl_vpeactivate_next);
> +
> +		/* settc(vpe) */
> +		uasm_i_mfc0(&p, t3, 1, 1); /* VPEControl */
> +		uasm_i_ins(&p, t3, t2, 0, 8);
> +		uasm_i_mtc0(&p, t3, 1, 1); /* VPEControl */
> +		uasm_i_ehb(&p);
> +
> +		/* Clear TCHalt */
> +		uasm_i_mttc0(&p, zero, 2, 4); /* TCHalt */
> +
> +		/* Decrement the count of VPEs to be handled */
> +		uasm_i_addiu(&p, t0, t0, -1);
> +
> +		/* Proceed to the next VPE, if there is one */
> +		uasm_build_label(&l, p, lbl_vpeactivate_next);
> +		uasm_i_srl(&p, t1, t1, 1);
> +		uasm_il_bnez(&p, &r, t0, lbl_vpeactivate_loop);
> +		uasm_i_addiu(&p, t2, t2, 1);
> +	}
> +
> +	/* Now perform our wait */
> +	uasm_build_label(&l, p, lbl_wait);
> +	uasm_i_wait(&p, 0);
> +
> +	/*
> +	 * Re-enable coherence. Note that all coupled VPEs will run this, the
> +	 * first will actually re-enable coherence & the rest will just be
> +	 * performing a rather unusual nop.
> +	 */
> +	uasm_i_addiu(&p, t0, zero, CM_GCR_Cx_COHERENCE_COHDOMAINEN_MSK);
> +	uasm_i_sw(&p, t0, 0, r_pcohctl);
> +	uasm_i_lw(&p, t0, 0, r_pcohctl);
> +
> +	/* Ordering barrier */
> +	uasm_i_sync(&p, stype_ordering);
> +
> +	if (coupled_coherence) {
> +		/* Decrement nc_ready_count */
> +		uasm_build_label(&l, p, lbl_decready);
> +		uasm_i_sync(&p, stype_ordering);
> +		uasm_i_ll(&p, t1, 0, r_pcount);
> +		uasm_i_addiu(&p, t2, t1, -1);
> +		uasm_i_sc(&p, t2, 0, r_pcount);
> +		uasm_il_beqz(&p, &r, t2, lbl_decready);
> +		uasm_i_move(&p, v0, t1);
> +	}
> +
> +	/* The core is coherent, time to return to C code */
> +	uasm_i_jr(&p, ra);
> +	uasm_i_nop(&p);
> +
> +	/* Ensure the code didn't exceed the resources allocated for it */
> +	BUG_ON((p - buf) > max_instrs);
> +	BUG_ON((l - labels) > ARRAY_SIZE(labels));
> +	BUG_ON((r - relocs) > ARRAY_SIZE(relocs));
> +
> +	/* Patch branch offsets */
> +	uasm_resolve_relocs(relocs, labels);
> +
> +	/* Flush the icache */
> +	local_flush_icache_range((unsigned long)buf, (unsigned long)p);
> +
> +	return buf;
> +}

The two functions above should go to somewhere in arch/mips.

> +
> +static void __init cps_cpuidle_unregister(void)
> +{
> +	int cpu;
> +	struct cpuidle_device *device;
> +	cps_nc_entry_fn *fn;
> +
> +	for_each_possible_cpu(cpu) {
> +		device = &per_cpu(cpuidle_dev, cpu);
> +		cpuidle_unregister_device(device);
> +
> +		/* Free entry code */
> +		fn = &per_cpu(ncwait_asm_enter, cpu_data[cpu].core);
> +		kfree(*fn);
> +		*fn = NULL;
> +	}
> +
> +	cpuidle_unregister_driver(&cps_driver);
> +}
> +
> +static int __init cps_cpuidle_init(void)
> +{
> +	int err, cpu, core, i;
> +	struct cpuidle_device *device;
> +	void *core_entry;
> +
> +	/*
> +	 * If interrupts were enabled whilst running the wait instruction then
> +	 * the VPE may end up processing interrupts whilst non-coherent.
> +	 */
> +	if (cpu_wait != r4k_wait_irqoff) {
> +		pr_warn("cpuidle-cps requires that masked interrupts restart the CPU pipeline following a wait\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Detect appropriate sync types for the system */
> +	switch (current_cpu_data.cputype) {
> +	case CPU_INTERAPTIV:
> +	case CPU_PROAPTIV:
> +		stype_intervention = 0x2;
> +		stype_memory = 0x3;
> +		stype_ordering = 0x10;
> +		break;
> +
> +	default:
> +		pr_warn("cpuidle-cps using heavyweight sync 0\n");
> +	}
> +
> +	/*
> +	 * Set the coupled flag on the appropriate states if this system
> +	 * requires it.
> +	 */
> +	if (coupled_coherence)
> +		for (i = 1; i < cps_driver.state_count; i++)
> +			cps_driver.states[i].flags |= CPUIDLE_FLAG_COUPLED;

I am not sure CPUIDLE_FLAG_COUPLED is the solution for this driver. 
IIUC, with the IPI above and the wakeup sync with the couple states, 
this driver is waking up everybody instead of sleeping as much as possible.

I would recommend to have a look at a different approach, like the one 
used for cpuidle-ux500.c.

> +
> +	err = cpuidle_register_driver(&cps_driver);
> +	if (err) {
> +		pr_err("Failed to register CPS cpuidle driver\n");
> +		return err;
> +	}
> +
> +	for_each_possible_cpu(cpu) {
> +		core = cpu_data[cpu].core;
> +		device = &per_cpu(cpuidle_dev, cpu);
> +		device->cpu = cpu;
> +#ifdef CONFIG_MIPS_MT
> +		cpumask_copy(&device->coupled_cpus, &cpu_sibling_map[cpu]);
> +#endif
> +		if (!per_cpu(ncwait_asm_enter, core)) {
> +			core_entry = cps_gen_entry_code(device);
> +			if (!core_entry) {
> +				pr_err("Failed to generate core %u entry\n",
> +				       core);
> +				err = -ENOMEM;
> +				goto err_out;
> +			}
> +			per_cpu(ncwait_asm_enter, core) = core_entry;
> +		}
> +
> +		err = cpuidle_register_device(device);
> +		if (err) {
> +			pr_err("Failed to register CPU%d cpuidle device\n",
> +			       cpu);
> +			goto err_out;
> +		}
> +	}
> +
> +	return 0;
> +err_out:
> +	cps_cpuidle_unregister();
> +	return err;
> +}
> +device_initcall(cps_cpuidle_init);
>


-- 
  <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

Re: [PATCH 10/10] cpuidle: cpuidle driver for MIPS CPS

[Paul Burton]

On Tue, Feb 25, 2014 at 04:33:46PM +0100, Daniel Lezcano wrote:
> On 01/15/2014 02:55 PM, Paul Burton wrote:
> >This patch introduces a cpuidle driver implementation for the MIPS
> >Coherent Processing System (ie. Coherence Manager, Cluster Power
> >Controller). It allows for use of the following idle states:
> >
> >  - Coherent wait. This is the usual MIPS wait instruction.
> >
> >  - Non-coherent wait. In this state a core will disable coherency with
> >    the rest of the system before running the wait instruction. This
> >    eliminates coherence interventions which would typically be used to
> >    keep cores coherent.
> >
> >These two states lay the groundwork for deeper states to be implemented
> >later, since all deeper states require the core to become non-coherent.
> >
> >Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> >Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> >Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> >Cc: linux-pm@vger.kernel.org
> >---
> >  drivers/cpuidle/Kconfig            |   5 +
> >  drivers/cpuidle/Kconfig.mips       |  14 +
> >  drivers/cpuidle/Makefile           |   3 +
> >  drivers/cpuidle/cpuidle-mips-cps.c | 545 +++++++++++++++++++++++++++++++++++++
> >  4 files changed, 567 insertions(+)
> >  create mode 100644 drivers/cpuidle/Kconfig.mips
> >  create mode 100644 drivers/cpuidle/cpuidle-mips-cps.c
> >
> >diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
> >index b3fb81d..11ff281 100644
> >--- a/drivers/cpuidle/Kconfig
> >+++ b/drivers/cpuidle/Kconfig
> >@@ -35,6 +35,11 @@ depends on ARM
> >  source "drivers/cpuidle/Kconfig.arm"
> >  endmenu
> >
> >+menu "MIPS CPU Idle Drivers"
> >+depends on MIPS
> >+source "drivers/cpuidle/Kconfig.mips"
> >+endmenu
> >+
> >  endif
> >
> >  config ARCH_NEEDS_CPU_IDLE_COUPLED
> >diff --git a/drivers/cpuidle/Kconfig.mips b/drivers/cpuidle/Kconfig.mips
> >new file mode 100644
> >index 0000000..dc96691
> >--- /dev/null
> >+++ b/drivers/cpuidle/Kconfig.mips
> >@@ -0,0 +1,14 @@
> >+#
> >+# MIPS CPU Idle drivers
> >+#
> >+
> >+config MIPS_CPS_CPUIDLE
> >+	bool "Support for MIPS Coherent Processing Systems"
> >+	depends on SYS_SUPPORTS_MIPS_CPS && CPU_MIPSR2 && !MIPS_MT_SMTC
> >+	select ARCH_NEEDS_CPU_IDLE_COUPLED if MIPS_MT
> >+	select MIPS_CM
> >+	help
> >+	  Select this option to enable CPU idle driver for systems based
> >+	  around the MIPS Coherent Processing System architecture - that
> >+	  is, those with a Coherence Manager & optionally a Cluster
> >+	  Power Controller.
> >diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> >index 527be28..693cd95 100644
> >--- a/drivers/cpuidle/Makefile
> >+++ b/drivers/cpuidle/Makefile
> >@@ -13,3 +13,6 @@ obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
> >  obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o
> >  obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
> >  obj-$(CONFIG_ARM_AT91_CPUIDLE)          += cpuidle-at91.o
> >+
> >+# MIPS SoC drivers
> >+obj-$(CONFIG_MIPS_CPS_CPUIDLE)		+= cpuidle-mips-cps.o
> >diff --git a/drivers/cpuidle/cpuidle-mips-cps.c b/drivers/cpuidle/cpuidle-mips-cps.c
> >new file mode 100644
> >index 0000000..a78bfb4
> >--- /dev/null
> >+++ b/drivers/cpuidle/cpuidle-mips-cps.c
> >@@ -0,0 +1,545 @@
> >+/*
> >+ * Copyright (C) 2013 Imagination Technologies
> >+ * Author: Paul Burton <paul.burton@imgtec.com>
> >+ *
> >+ * This program is free software; you can redistribute it and/or modify it
> >+ * under the terms of the GNU General Public License as published by the
> >+ * Free Software Foundation;  either version 2 of the  License, or (at your
> >+ * option) any later version.
> >+ */
> >+
> >+#include <linux/cpuidle.h>
> >+#include <linux/init.h>
> >+#include <linux/kconfig.h>
> >+#include <linux/slab.h>
> >+
> >+#include <asm/cacheflush.h>
> >+#include <asm/cacheops.h>
> >+#include <asm/idle.h>
> >+#include <asm/mips-cm.h>
> >+#include <asm/mipsmtregs.h>
> >+#include <asm/uasm.h>
> 
> The convention is to not include headers from arch. These headers shouldn't
> appear in this driver.
> 

Without accessing those headers I can't really implement anything useful
- entering these idle states by their nature involves architecture
specifics. Would you rather the bulk of the driver is implemented under
arch/mips & the code in drivers/cpuidle simply calls elsewhere to do the
real work?

> >+/*
> >+ * The CM & CPC can only handle coherence & power control on a per-core basis,
> >+ * thus in an MT system the VPEs within each core are coupled and can only
> >+ * enter or exit states requiring CM or CPC assistance in unison.
> >+ */
> >+#ifdef CONFIG_MIPS_MT
> >+# define coupled_coherence cpu_has_mipsmt
> >+#else
> >+# define coupled_coherence 0
> >+#endif
> >+
> >+/*
> >+ * cps_nc_entry_fn - type of a generated non-coherent state entry function
> >+ * @vpe_mask: a bitmap of online coupled VPEs, excluding this one
> >+ * @online: the count of online coupled VPEs (weight of vpe_mask + 1)
> >+ *
> >+ * The code entering & exiting non-coherent states is generated at runtime
> >+ * using uasm, in order to ensure that the compiler cannot insert a stray
> >+ * memory access at an unfortunate time and to allow the generation of optimal
> >+ * core-specific code particularly for cache routines. If coupled_coherence
> >+ * is non-zero, returns the number of VPEs that were in the wait state at the
> >+ * point this VPE left it. Returns garbage if coupled_coherence is zero.
> >+ */
> >+typedef unsigned (*cps_nc_entry_fn)(unsigned vpe_mask, unsigned online);
> >+
> >+/*
> >+ * The entry point of the generated non-coherent wait entry/exit function.
> >+ * Actually per-core rather than per-CPU.
> >+ */
> >+static DEFINE_PER_CPU_READ_MOSTLY(cps_nc_entry_fn, ncwait_asm_enter);
> >+
> >+/*
> >+ * Indicates the number of coupled VPEs ready to operate in a non-coherent
> >+ * state. Actually per-core rather than per-CPU.
> >+ */
> >+static DEFINE_PER_CPU_ALIGNED(u32, nc_ready_count);
> >+
> >+/* A somewhat arbitrary number of labels & relocs for uasm */
> >+static struct uasm_label labels[32] __initdata;
> >+static struct uasm_reloc relocs[32] __initdata;
> >+
> >+/* CPU dependant sync types */
> >+static unsigned stype_intervention;
> >+static unsigned stype_memory;
> >+static unsigned stype_ordering;
> >+
> >+enum mips_reg {
> >+	zero, at, v0, v1, a0, a1, a2, a3,
> >+	t0, t1, t2, t3, t4, t5, t6, t7,
> >+	s0, s1, s2, s3, s4, s5, s6, s7,
> >+	t8, t9, k0, k1, gp, sp, fp, ra,
> >+};
> >+
> >+static int cps_ncwait_enter(struct cpuidle_device *dev,
> >+			    struct cpuidle_driver *drv, int index)
> >+{
> >+	unsigned core = cpu_data[dev->cpu].core;
> >+	unsigned online, first_cpu, num_left;
> >+	cpumask_var_t coupled_mask, vpe_mask;
> >+
> >+	if (!alloc_cpumask_var(&coupled_mask, GFP_KERNEL))
> >+		return -ENOMEM;
> >+
> >+	if (!alloc_cpumask_var(&vpe_mask, GFP_KERNEL)) {
> >+		free_cpumask_var(coupled_mask);
> >+		return -ENOMEM;
> >+	}
> 
> You can't do that in this function where the local irqs are disabled. IMO,
> if you set CONFIG_CPUMASK_OFFSTACK and CONFIG_DEBUG_ATOMIC_SLEEP, you should
> see a kernel warning.

Right you are, it should either use GFP_ATOMIC or just not handle the
off-stack case (which isn't currently used).

> 
> >+	/* Calculate which coupled CPUs (VPEs) are online */
> >+#ifdef CONFIG_MIPS_MT
> >+	cpumask_and(coupled_mask, cpu_online_mask, &dev->coupled_cpus);
> >+	first_cpu = cpumask_first(coupled_mask);
> >+	online = cpumask_weight(coupled_mask);
> >+	cpumask_clear_cpu(dev->cpu, coupled_mask);
> >+	cpumask_shift_right(vpe_mask, coupled_mask,
> >+			    cpumask_first(&dev->coupled_cpus));
> 
> What is the purpose of this computation ?

If you read through the code generated in cps_gen_entry_code, the
vpe_mask is used to indicate the VPEs within the current core which are
both online & not the VPE currently running the code.

> 
> >+#else
> >+	cpumask_clear(coupled_mask);
> >+	cpumask_clear(vpe_mask);
> >+	first_cpu = dev->cpu;
> >+	online = 1;
> >+#endif
> 
> first_cpu is not used.

Right you are. It's used in further work-in-progress patches but I'll
remove it from this one.

> 
> >+	/*
> >+	 * Run the generated entry code. Note that we assume the number of VPEs
> >+	 * within this core does not exceed the width in bits of a long. Since
> >+	 * MVPConf0.PVPE is 4 bits wide this seems like a safe assumption.
> >+	 */
> >+	num_left = per_cpu(ncwait_asm_enter, core)(vpe_mask->bits[0], online);
> >+
> >+	/*
> >+	 * If this VPE is the first to leave the non-coherent wait state then
> >+	 * it needs to wake up any coupled VPEs still running their wait
> >+	 * instruction so that they return to cpuidle,
> 
> Why is it needed ? Can't the other cpus stay idle ?

Not with the current cpuidle code. Please see the end of
cpuidle_enter_state_coupled in drivers/cpuidle/coupled.c. The code waits
for all coupled CPUs to exit the idle state before any of them proceed.
Whilst I suppose it would be possible to modify cpuidle to not require
that, it would leave you with inaccurate residence statistics mentioned
below.

> 
> >       * which can then complete
> >+	 * coordination between the coupled VPEs & provide the governor with
> >+	 * a chance to reflect on the length of time the VPEs were in the
> >+	 * idle state.
> >+	 */
> >+	if (coupled_coherence && (num_left == online))
> >+		arch_send_call_function_ipi_mask(coupled_mask);
> 
> Except there is no choice due to hardware limitations, I don't think this is
> valid.

By nature when one CPU (VPE) within a core leaves a non-coherent state
the rest do too, because as I mentioned coherence is a property of the
core not of individual VPEs. It would be possible to leave the other
VPEs idle if we didn't differentiate between the coherent & non-coherent
wait states, but again not with cpuidle as it is today (due to the
waiting for all CPUs at the end of cpuidle_enter_state_coupled).

> 
> >+	free_cpumask_var(vpe_mask);
> >+	free_cpumask_var(coupled_mask);
> >+	return index;
> >+}
> >+
> >+static struct cpuidle_driver cps_driver = {
> >+	.name			= "cpc_cpuidle",
> >+	.owner			= THIS_MODULE,
> >+	.states = {
> >+		MIPS_CPUIDLE_WAIT_STATE,
> >+		{
> >+			.enter	= cps_ncwait_enter,
> >+			.exit_latency		= 200,
> >+			.target_residency	= 450,
> >+			.flags	= CPUIDLE_FLAG_TIME_VALID,
> >+			.name	= "nc-wait",
> >+			.desc	= "non-coherent MIPS wait",
> >+		},
> >+	},
> >+	.state_count		= 2,
> >+	.safe_state_index	= 0,
> >+};
> 
> 
> 
> >+static void __init cps_gen_cache_routine(u32 **pp, struct uasm_label **pl,
> >+					 struct uasm_reloc **pr,
> >+					 const struct cache_desc *cache,
> >+					 unsigned op, int lbl)
> >+{
> >+	unsigned cache_size = cache->ways << cache->waybit;
> >+	unsigned i;
> >+	const unsigned unroll_lines = 32;
> >+
> >+	/* If the cache isn't present this function has it easy */
> >+	if (cache->flags & MIPS_CACHE_NOT_PRESENT)
> >+		return;
> >+
> >+	/* Load base address */
> >+	UASM_i_LA(pp, t0, (long)CKSEG0);
> >+
> >+	/* Calculate end address */
> >+	if (cache_size < 0x8000)
> >+		uasm_i_addiu(pp, t1, t0, cache_size);
> >+	else
> >+		UASM_i_LA(pp, t1, (long)(CKSEG0 + cache_size));
> >+
> >+	/* Start of cache op loop */
> >+	uasm_build_label(pl, *pp, lbl);
> >+
> >+	/* Generate the cache ops */
> >+	for (i = 0; i < unroll_lines; i++)
> >+		uasm_i_cache(pp, op, i * cache->linesz, t0);
> >+
> >+	/* Update the base address */
> >+	uasm_i_addiu(pp, t0, t0, unroll_lines * cache->linesz);
> >+
> >+	/* Loop if we haven't reached the end address yet */
> >+	uasm_il_bne(pp, pr, t0, t1, lbl);
> >+	uasm_i_nop(pp);
> >+}
> >+
> >+static void * __init cps_gen_entry_code(struct cpuidle_device *dev)
> >+{
> >+	unsigned core = cpu_data[dev->cpu].core;
> >+	struct uasm_label *l = labels;
> >+	struct uasm_reloc *r = relocs;
> >+	u32 *buf, *p;
> >+	const unsigned r_vpemask = a0;
> >+	const unsigned r_online = a1;
> >+	const unsigned r_pcount = t6;
> >+	const unsigned r_pcohctl = t7;
> >+	const unsigned max_instrs = 256;
> >+	enum {
> >+		lbl_incready = 1,
> >+		lbl_lastvpe,
> >+		lbl_vpehalt_loop,
> >+		lbl_vpehalt_poll,
> >+		lbl_vpehalt_next,
> >+		lbl_disable_coherence,
> >+		lbl_invicache,
> >+		lbl_flushdcache,
> >+		lbl_vpeactivate_loop,
> >+		lbl_vpeactivate_next,
> >+		lbl_wait,
> >+		lbl_decready,
> >+	};
> >+
> >+	/* Allocate a buffer to hold the generated code */
> >+	p = buf = kcalloc(max_instrs, sizeof(u32), GFP_KERNEL);
> >+	if (!buf)
> >+		return NULL;
> >+
> >+	/* Clear labels & relocs ready for (re)use */
> >+	memset(labels, 0, sizeof(labels));
> >+	memset(relocs, 0, sizeof(relocs));
> >+
> >+	/*
> >+	 * Load address of the CM GCR_CL_COHERENCE register. This is done early
> >+	 * because it's needed in both the enable & disable coherence steps but
> >+	 * in the coupled case the enable step will only run on one VPE.
> >+	 */
> >+	UASM_i_LA(&p, r_pcohctl, (long)addr_gcr_cl_coherence());
> >+
> >+	if (coupled_coherence) {
> >+		/* Load address of nc_ready_count */
> >+		UASM_i_LA(&p, r_pcount, (long)&per_cpu(nc_ready_count, core));
> >+
> >+		/* Increment nc_ready_count */
> >+		uasm_build_label(&l, p, lbl_incready);
> >+		uasm_i_sync(&p, stype_ordering);
> >+		uasm_i_ll(&p, t1, 0, r_pcount);
> >+		uasm_i_addiu(&p, t2, t1, 1);
> >+		uasm_i_sc(&p, t2, 0, r_pcount);
> >+		uasm_il_beqz(&p, &r, t2, lbl_incready);
> >+		uasm_i_addiu(&p, t1, t1, 1);
> >+
> >+		/*
> >+		 * If this is the last VPE to become ready for non-coherence
> >+		 * then it should branch below.
> >+		 */
> >+		uasm_il_beq(&p, &r, t1, r_online, lbl_lastvpe);
> >+		uasm_i_nop(&p);
> >+
> >+		/*
> >+		 * Otherwise this is not the last VPE to become ready for
> >+		 * non-coherence. It needs to wait until coherence has been
> >+		 * disabled before executing a wait instruction, otherwise it
> >+		 * may return from wait quickly & re-enable coherence causing
> >+		 * a race with the VPE disabling coherence. It can't simply
> >+		 * poll the CPC sequencer for a non-coherent state as that
> >+		 * would race with any other VPE which may spot the
> >+		 * non-coherent state, run wait, return quickly & re-enable
> >+		 * coherence before this VPE ever saw the non-coherent state.
> >+		 * Instead this VPE will halt its TC such that it ceases to
> >+		 * execute for the moment.
> >+		 */
> >+		uasm_i_addiu(&p, t0, zero, TCHALT_H);
> >+		uasm_i_mtc0(&p, t0, 2, 4); /* TCHalt */
> >+
> >+		/* instruction_hazard(), to ensure the TC halts */
> >+		UASM_i_LA(&p, t0, (long)p + 12);
> >+		uasm_i_jr_hb(&p, t0);
> >+		uasm_i_nop(&p);
> >+
> >+		/*
> >+		 * The VPE which disables coherence will then clear the halt
> >+		 * bit for this VPE's TC once coherence has been disabled and
> >+		 * it can safely proceed to execute the wait instruction.
> >+		 */
> >+		uasm_il_b(&p, &r, lbl_wait);
> >+		uasm_i_nop(&p);
> >+
> >+		/*
> >+		 * The last VPE to increment nc_ready_count will continue from
> >+		 * here and must spin until all other VPEs within the core have
> >+		 * been halted, at which point it can be sure that it is safe
> >+		 * to disable coherence.
> >+		 *
> >+		 *   t0: number of VPEs left to handle
> >+		 *   t1: (shifted) mask of online VPEs
> >+		 *   t2: current VPE index
> >+		 */
> >+		uasm_build_label(&l, p, lbl_lastvpe);
> >+		uasm_i_addiu(&p, t0, r_online, -1);
> >+		uasm_il_beqz(&p, &r, t0, lbl_disable_coherence);
> >+		uasm_i_move(&p, t1, r_vpemask);
> >+		uasm_i_move(&p, t2, zero);
> >+
> >+		/*
> >+		 * Now loop through all VPEs within the core checking whether
> >+		 * they are online & not this VPE, which can be determined by
> >+		 * checking the vpe_mask argument. If a VPE is offline or is
> >+		 * this VPE, skip it.
> >+		 */
> >+		uasm_build_label(&l, p, lbl_vpehalt_loop);
> >+		uasm_i_andi(&p, t3, t1, 1);
> >+		uasm_il_beqz(&p, &r, t3, lbl_vpehalt_next);
> >+
> >+		/* settc(vpe) */
> >+		uasm_i_mfc0(&p, t3, 1, 1); /* VPEControl */
> >+		uasm_i_ins(&p, t3, t2, 0, 8);
> >+		uasm_i_mtc0(&p, t3, 1, 1); /* VPEControl */
> >+		uasm_i_ehb(&p);
> >+
> >+		/*
> >+		 * It's very likely that the VPE has already halted itself
> >+		 * by now, but there's theoretically a chance that it may not
> >+		 * have. Wait until the VPE's TC is halted.
> >+		 */
> >+		uasm_build_label(&l, p, lbl_vpehalt_poll);
> >+		uasm_i_mftc0(&p, t3, 2, 4); /* TCHalt */
> >+		uasm_il_beqz(&p, &r, t3, lbl_vpehalt_poll);
> >+		uasm_i_nop(&p);
> >+
> >+		/* Decrement the count of VPEs to be handled */
> >+		uasm_i_addiu(&p, t0, t0, -1);
> >+
> >+		/* Proceed to the next VPE, if there is one */
> >+		uasm_build_label(&l, p, lbl_vpehalt_next);
> >+		uasm_i_srl(&p, t1, t1, 1);
> >+		uasm_il_bnez(&p, &r, t0, lbl_vpehalt_loop);
> >+		uasm_i_addiu(&p, t2, t2, 1);
> >+	}
> >+
> >+	/*
> >+	 * This is the point of no return - this VPE will now proceed to
> >+	 * disable coherence. At this point we *must* be sure that no other
> >+	 * VPE within the core will interfere with the L1 dcache.
> >+	 */
> >+	uasm_build_label(&l, p, lbl_disable_coherence);
> >+
> >+	/* Completion barrier */
> >+	uasm_i_sync(&p, stype_memory);
> >+
> >+	/* Invalidate the L1 icache */
> >+	cps_gen_cache_routine(&p, &l, &r, &cpu_data[dev->cpu].icache,
> >+			      Index_Invalidate_I, lbl_invicache);
> >+
> >+	/* Writeback & invalidate the L1 dcache */
> >+	cps_gen_cache_routine(&p, &l, &r, &cpu_data[dev->cpu].dcache,
> >+			      Index_Writeback_Inv_D, lbl_flushdcache);
> >+
> >+	/*
> >+	 * Disable all but self interventions. The load from COHCTL is defined
> >+	 * by the interAptiv & proAptiv SUMs as ensuring that the operation
> >+	 * resulting from the preceeding store is complete.
> >+	 */
> >+	uasm_i_addiu(&p, t0, zero, 1 << cpu_data[dev->cpu].core);
> >+	uasm_i_sw(&p, t0, 0, r_pcohctl);
> >+	uasm_i_lw(&p, t0, 0, r_pcohctl);
> >+
> >+	/* Sync to ensure previous interventions are complete */
> >+	uasm_i_sync(&p, stype_intervention);
> >+
> >+	/* Disable coherence */
> >+	uasm_i_sw(&p, zero, 0, r_pcohctl);
> >+	uasm_i_lw(&p, t0, 0, r_pcohctl);
> >+
> >+	if (coupled_coherence) {
> >+		/*
> >+		 * Now that coherence is disabled it is safe for all VPEs to
> >+		 * proceed with executing their wait instruction, so this VPE
> >+		 * will go ahead and clear the halt bit of the TCs associated
> >+		 * with all other online VPEs within the core. Start by
> >+		 * initialising variables used throughout the loop, and
> >+		 * skipping the loop entirely if there are no VPEs to handle.
> >+		 *
> >+		 *   t0: number of VPEs left to handle
> >+		 *   t1: (shifted) mask of online VPEs
> >+		 *   t2: current VPE index
> >+		 */
> >+		uasm_i_addiu(&p, t0, r_online, -1);
> >+		uasm_il_beqz(&p, &r, t0, lbl_wait);
> >+		uasm_i_move(&p, t1, r_vpemask);
> >+		uasm_i_move(&p, t2, zero);
> >+
> >+		/*
> >+		 * Now loop through all VPEs within the core checking whether
> >+		 * they are online & not this VPE, which can be determined by
> >+		 * checking the vpe_mask argument. If a VPE is offline or is
> >+		 * this VPE, skip it.
> >+		 */
> >+		uasm_build_label(&l, p, lbl_vpeactivate_loop);
> >+		uasm_i_andi(&p, t3, t1, 1);
> >+		uasm_il_beqz(&p, &r, t3, lbl_vpeactivate_next);
> >+
> >+		/* settc(vpe) */
> >+		uasm_i_mfc0(&p, t3, 1, 1); /* VPEControl */
> >+		uasm_i_ins(&p, t3, t2, 0, 8);
> >+		uasm_i_mtc0(&p, t3, 1, 1); /* VPEControl */
> >+		uasm_i_ehb(&p);
> >+
> >+		/* Clear TCHalt */
> >+		uasm_i_mttc0(&p, zero, 2, 4); /* TCHalt */
> >+
> >+		/* Decrement the count of VPEs to be handled */
> >+		uasm_i_addiu(&p, t0, t0, -1);
> >+
> >+		/* Proceed to the next VPE, if there is one */
> >+		uasm_build_label(&l, p, lbl_vpeactivate_next);
> >+		uasm_i_srl(&p, t1, t1, 1);
> >+		uasm_il_bnez(&p, &r, t0, lbl_vpeactivate_loop);
> >+		uasm_i_addiu(&p, t2, t2, 1);
> >+	}
> >+
> >+	/* Now perform our wait */
> >+	uasm_build_label(&l, p, lbl_wait);
> >+	uasm_i_wait(&p, 0);
> >+
> >+	/*
> >+	 * Re-enable coherence. Note that all coupled VPEs will run this, the
> >+	 * first will actually re-enable coherence & the rest will just be
> >+	 * performing a rather unusual nop.
> >+	 */
> >+	uasm_i_addiu(&p, t0, zero, CM_GCR_Cx_COHERENCE_COHDOMAINEN_MSK);
> >+	uasm_i_sw(&p, t0, 0, r_pcohctl);
> >+	uasm_i_lw(&p, t0, 0, r_pcohctl);
> >+
> >+	/* Ordering barrier */
> >+	uasm_i_sync(&p, stype_ordering);
> >+
> >+	if (coupled_coherence) {
> >+		/* Decrement nc_ready_count */
> >+		uasm_build_label(&l, p, lbl_decready);
> >+		uasm_i_sync(&p, stype_ordering);
> >+		uasm_i_ll(&p, t1, 0, r_pcount);
> >+		uasm_i_addiu(&p, t2, t1, -1);
> >+		uasm_i_sc(&p, t2, 0, r_pcount);
> >+		uasm_il_beqz(&p, &r, t2, lbl_decready);
> >+		uasm_i_move(&p, v0, t1);
> >+	}
> >+
> >+	/* The core is coherent, time to return to C code */
> >+	uasm_i_jr(&p, ra);
> >+	uasm_i_nop(&p);
> >+
> >+	/* Ensure the code didn't exceed the resources allocated for it */
> >+	BUG_ON((p - buf) > max_instrs);
> >+	BUG_ON((l - labels) > ARRAY_SIZE(labels));
> >+	BUG_ON((r - relocs) > ARRAY_SIZE(relocs));
> >+
> >+	/* Patch branch offsets */
> >+	uasm_resolve_relocs(relocs, labels);
> >+
> >+	/* Flush the icache */
> >+	local_flush_icache_range((unsigned long)buf, (unsigned long)p);
> >+
> >+	return buf;
> >+}
> 
> The two functions above should go to somewhere in arch/mips.

Well I guess they can, but they only make any sense in the context of
cpuidle which is why I implemented them here.

> 
> >+
> >+static void __init cps_cpuidle_unregister(void)
> >+{
> >+	int cpu;
> >+	struct cpuidle_device *device;
> >+	cps_nc_entry_fn *fn;
> >+
> >+	for_each_possible_cpu(cpu) {
> >+		device = &per_cpu(cpuidle_dev, cpu);
> >+		cpuidle_unregister_device(device);
> >+
> >+		/* Free entry code */
> >+		fn = &per_cpu(ncwait_asm_enter, cpu_data[cpu].core);
> >+		kfree(*fn);
> >+		*fn = NULL;
> >+	}
> >+
> >+	cpuidle_unregister_driver(&cps_driver);
> >+}
> >+
> >+static int __init cps_cpuidle_init(void)
> >+{
> >+	int err, cpu, core, i;
> >+	struct cpuidle_device *device;
> >+	void *core_entry;
> >+
> >+	/*
> >+	 * If interrupts were enabled whilst running the wait instruction then
> >+	 * the VPE may end up processing interrupts whilst non-coherent.
> >+	 */
> >+	if (cpu_wait != r4k_wait_irqoff) {
> >+		pr_warn("cpuidle-cps requires that masked interrupts restart the CPU pipeline following a wait\n");
> >+		return -ENODEV;
> >+	}
> >+
> >+	/* Detect appropriate sync types for the system */
> >+	switch (current_cpu_data.cputype) {
> >+	case CPU_INTERAPTIV:
> >+	case CPU_PROAPTIV:
> >+		stype_intervention = 0x2;
> >+		stype_memory = 0x3;
> >+		stype_ordering = 0x10;
> >+		break;
> >+
> >+	default:
> >+		pr_warn("cpuidle-cps using heavyweight sync 0\n");
> >+	}
> >+
> >+	/*
> >+	 * Set the coupled flag on the appropriate states if this system
> >+	 * requires it.
> >+	 */
> >+	if (coupled_coherence)
> >+		for (i = 1; i < cps_driver.state_count; i++)
> >+			cps_driver.states[i].flags |= CPUIDLE_FLAG_COUPLED;
> 
> I am not sure CPUIDLE_FLAG_COUPLED is the solution for this driver. IIUC,
> with the IPI above and the wakeup sync with the couple states, this driver
> is waking up everybody instead of sleeping as much as possible.
> 
> I would recommend to have a look at a different approach, like the one used
> for cpuidle-ux500.c.

Ok it looks like that just counts & performs work only on the last
online CPU to run the code. As before that could work but only by
disregarding any differentiation between coherent & non-coherent wait
states at levels above the driver.

Paul

> 
> >+
> >+	err = cpuidle_register_driver(&cps_driver);
> >+	if (err) {
> >+		pr_err("Failed to register CPS cpuidle driver\n");
> >+		return err;
> >+	}
> >+
> >+	for_each_possible_cpu(cpu) {
> >+		core = cpu_data[cpu].core;
> >+		device = &per_cpu(cpuidle_dev, cpu);
> >+		device->cpu = cpu;
> >+#ifdef CONFIG_MIPS_MT
> >+		cpumask_copy(&device->coupled_cpus, &cpu_sibling_map[cpu]);
> >+#endif
> >+		if (!per_cpu(ncwait_asm_enter, core)) {
> >+			core_entry = cps_gen_entry_code(device);
> >+			if (!core_entry) {
> >+				pr_err("Failed to generate core %u entry\n",
> >+				       core);
> >+				err = -ENOMEM;
> >+				goto err_out;
> >+			}
> >+			per_cpu(ncwait_asm_enter, core) = core_entry;
> >+		}
> >+
> >+		err = cpuidle_register_device(device);
> >+		if (err) {
> >+			pr_err("Failed to register CPU%d cpuidle device\n",
> >+			       cpu);
> >+			goto err_out;
> >+		}
> >+	}
> >+
> >+	return 0;
> >+err_out:
> >+	cps_cpuidle_unregister();
> >+	return err;
> >+}
> >+device_initcall(cps_cpuidle_init);
> >
> 
> 
> -- 
>  <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
> 

Re: [PATCH 10/10] cpuidle: cpuidle driver for MIPS CPS

[Daniel Lezcano]

On 02/25/2014 11:12 PM, Paul Burton wrote:
> On Tue, Feb 25, 2014 at 04:33:46PM +0100, Daniel Lezcano wrote:
>> On 01/15/2014 02:55 PM, Paul Burton wrote:
>>> This patch introduces a cpuidle driver implementation for the MIPS
>>> Coherent Processing System (ie. Coherence Manager, Cluster Power
>>> Controller). It allows for use of the following idle states:
>>>
>>>   - Coherent wait. This is the usual MIPS wait instruction.
>>>
>>>   - Non-coherent wait. In this state a core will disable coherency with
>>>     the rest of the system before running the wait instruction. This
>>>     eliminates coherence interventions which would typically be used to
>>>     keep cores coherent.
>>>
>>> These two states lay the groundwork for deeper states to be implemented
>>> later, since all deeper states require the core to become non-coherent.
>>>
>>> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Cc: linux-pm@vger.kernel.org
>>> ---
>>>   drivers/cpuidle/Kconfig            |   5 +
>>>   drivers/cpuidle/Kconfig.mips       |  14 +
>>>   drivers/cpuidle/Makefile           |   3 +
>>>   drivers/cpuidle/cpuidle-mips-cps.c | 545 +++++++++++++++++++++++++++++++++++++
>>>   4 files changed, 567 insertions(+)
>>>   create mode 100644 drivers/cpuidle/Kconfig.mips
>>>   create mode 100644 drivers/cpuidle/cpuidle-mips-cps.c
>>>
>>> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
>>> index b3fb81d..11ff281 100644
>>> --- a/drivers/cpuidle/Kconfig
>>> +++ b/drivers/cpuidle/Kconfig
>>> @@ -35,6 +35,11 @@ depends on ARM
>>>   source "drivers/cpuidle/Kconfig.arm"
>>>   endmenu
>>>
>>> +menu "MIPS CPU Idle Drivers"
>>> +depends on MIPS
>>> +source "drivers/cpuidle/Kconfig.mips"
>>> +endmenu
>>> +
>>>   endif
>>>
>>>   config ARCH_NEEDS_CPU_IDLE_COUPLED
>>> diff --git a/drivers/cpuidle/Kconfig.mips b/drivers/cpuidle/Kconfig.mips
>>> new file mode 100644
>>> index 0000000..dc96691
>>> --- /dev/null
>>> +++ b/drivers/cpuidle/Kconfig.mips
>>> @@ -0,0 +1,14 @@
>>> +#
>>> +# MIPS CPU Idle drivers
>>> +#
>>> +
>>> +config MIPS_CPS_CPUIDLE
>>> +	bool "Support for MIPS Coherent Processing Systems"
>>> +	depends on SYS_SUPPORTS_MIPS_CPS && CPU_MIPSR2 && !MIPS_MT_SMTC
>>> +	select ARCH_NEEDS_CPU_IDLE_COUPLED if MIPS_MT
>>> +	select MIPS_CM
>>> +	help
>>> +	  Select this option to enable CPU idle driver for systems based
>>> +	  around the MIPS Coherent Processing System architecture - that
>>> +	  is, those with a Coherence Manager & optionally a Cluster
>>> +	  Power Controller.
>>> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
>>> index 527be28..693cd95 100644
>>> --- a/drivers/cpuidle/Makefile
>>> +++ b/drivers/cpuidle/Makefile
>>> @@ -13,3 +13,6 @@ obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
>>>   obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o
>>>   obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
>>>   obj-$(CONFIG_ARM_AT91_CPUIDLE)          += cpuidle-at91.o
>>> +
>>> +# MIPS SoC drivers
>>> +obj-$(CONFIG_MIPS_CPS_CPUIDLE)		+= cpuidle-mips-cps.o
>>> diff --git a/drivers/cpuidle/cpuidle-mips-cps.c b/drivers/cpuidle/cpuidle-mips-cps.c
>>> new file mode 100644
>>> index 0000000..a78bfb4
>>> --- /dev/null
>>> +++ b/drivers/cpuidle/cpuidle-mips-cps.c
>>> @@ -0,0 +1,545 @@
>>> +/*
>>> + * Copyright (C) 2013 Imagination Technologies
>>> + * Author: Paul Burton <paul.burton@imgtec.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU General Public License as published by the
>>> + * Free Software Foundation;  either version 2 of the  License, or (at your
>>> + * option) any later version.
>>> + */
>>> +
>>> +#include <linux/cpuidle.h>
>>> +#include <linux/init.h>
>>> +#include <linux/kconfig.h>
>>> +#include <linux/slab.h>
>>> +
>>> +#include <asm/cacheflush.h>
>>> +#include <asm/cacheops.h>
>>> +#include <asm/idle.h>
>>> +#include <asm/mips-cm.h>
>>> +#include <asm/mipsmtregs.h>
>>> +#include <asm/uasm.h>
>>
>> The convention is to not include headers from arch. These headers shouldn't
>> appear in this driver.
>>
>
> Without accessing those headers I can't really implement anything useful
> - entering these idle states by their nature involves architecture
> specifics. Would you rather the bulk of the driver is implemented under
> arch/mips & the code in drivers/cpuidle simply calls elsewhere to do the
> real work?

Usually, this is how it is done. If you can move the 'asm' code inside 
eg. pm.c somewhere in arch/mips and invoke from pm functions from 
cpuidle through a simple header that would be great.

>>> +/*
>>> + * The CM & CPC can only handle coherence & power control on a per-core basis,
>>> + * thus in an MT system the VPEs within each core are coupled and can only
>>> + * enter or exit states requiring CM or CPC assistance in unison.
>>> + */
>>> +#ifdef CONFIG_MIPS_MT
>>> +# define coupled_coherence cpu_has_mipsmt
>>> +#else
>>> +# define coupled_coherence 0
>>> +#endif
>>> +
>>> +/*
>>> + * cps_nc_entry_fn - type of a generated non-coherent state entry function
>>> + * @vpe_mask: a bitmap of online coupled VPEs, excluding this one
>>> + * @online: the count of online coupled VPEs (weight of vpe_mask + 1)
>>> + *
>>> + * The code entering & exiting non-coherent states is generated at runtime
>>> + * using uasm, in order to ensure that the compiler cannot insert a stray
>>> + * memory access at an unfortunate time and to allow the generation of optimal
>>> + * core-specific code particularly for cache routines. If coupled_coherence
>>> + * is non-zero, returns the number of VPEs that were in the wait state at the
>>> + * point this VPE left it. Returns garbage if coupled_coherence is zero.
>>> + */
>>> +typedef unsigned (*cps_nc_entry_fn)(unsigned vpe_mask, unsigned online);
>>> +
>>> +/*
>>> + * The entry point of the generated non-coherent wait entry/exit function.
>>> + * Actually per-core rather than per-CPU.
>>> + */
>>> +static DEFINE_PER_CPU_READ_MOSTLY(cps_nc_entry_fn, ncwait_asm_enter);
>>> +
>>> +/*
>>> + * Indicates the number of coupled VPEs ready to operate in a non-coherent
>>> + * state. Actually per-core rather than per-CPU.
>>> + */
>>> +static DEFINE_PER_CPU_ALIGNED(u32, nc_ready_count);
>>> +
>>> +/* A somewhat arbitrary number of labels & relocs for uasm */
>>> +static struct uasm_label labels[32] __initdata;
>>> +static struct uasm_reloc relocs[32] __initdata;
>>> +
>>> +/* CPU dependant sync types */
>>> +static unsigned stype_intervention;
>>> +static unsigned stype_memory;
>>> +static unsigned stype_ordering;
>>> +
>>> +enum mips_reg {
>>> +	zero, at, v0, v1, a0, a1, a2, a3,
>>> +	t0, t1, t2, t3, t4, t5, t6, t7,
>>> +	s0, s1, s2, s3, s4, s5, s6, s7,
>>> +	t8, t9, k0, k1, gp, sp, fp, ra,
>>> +};
>>> +
>>> +static int cps_ncwait_enter(struct cpuidle_device *dev,
>>> +			    struct cpuidle_driver *drv, int index)
>>> +{
>>> +	unsigned core = cpu_data[dev->cpu].core;
>>> +	unsigned online, first_cpu, num_left;
>>> +	cpumask_var_t coupled_mask, vpe_mask;
>>> +
>>> +	if (!alloc_cpumask_var(&coupled_mask, GFP_KERNEL))
>>> +		return -ENOMEM;
>>> +
>>> +	if (!alloc_cpumask_var(&vpe_mask, GFP_KERNEL)) {
>>> +		free_cpumask_var(coupled_mask);
>>> +		return -ENOMEM;
>>> +	}
>>
>> You can't do that in this function where the local irqs are disabled. IMO,
>> if you set CONFIG_CPUMASK_OFFSTACK and CONFIG_DEBUG_ATOMIC_SLEEP, you should
>> see a kernel warning.
>
> Right you are, it should either use GFP_ATOMIC or just not handle the
> off-stack case (which isn't currently used).

Why don't you just allocate these cpumasks at init time instead of 
allocating / freeing them in the fast path ?

>>> +	/* Calculate which coupled CPUs (VPEs) are online */
>>> +#ifdef CONFIG_MIPS_MT
>>> +	cpumask_and(coupled_mask, cpu_online_mask, &dev->coupled_cpus);
>>> +	first_cpu = cpumask_first(coupled_mask);
>>> +	online = cpumask_weight(coupled_mask);
>>> +	cpumask_clear_cpu(dev->cpu, coupled_mask);
>>> +	cpumask_shift_right(vpe_mask, coupled_mask,
>>> +			    cpumask_first(&dev->coupled_cpus));
>>
>> What is the purpose of this computation ?
>
> If you read through the code generated in cps_gen_entry_code, the
> vpe_mask is used to indicate the VPEs within the current core which are
> both online & not the VPE currently running the code.

Ok, thanks for the clarification.

Shouldn't 'online = cpumask_weight(coupled_mask)' be after clearing the 
current cpu ? Otherwise, online has also the current cpu which, IIUC, 
shouldn't be included, no ?

>>> +#else
>>> +	cpumask_clear(coupled_mask);
>>> +	cpumask_clear(vpe_mask);
>>> +	first_cpu = dev->cpu;
>>> +	online = 1;
>>> +#endif
>>
>> first_cpu is not used.
>
> Right you are. It's used in further work-in-progress patches but I'll
> remove it from this one.
>
>>
>>> +	/*
>>> +	 * Run the generated entry code. Note that we assume the number of VPEs
>>> +	 * within this core does not exceed the width in bits of a long. Since
>>> +	 * MVPConf0.PVPE is 4 bits wide this seems like a safe assumption.
>>> +	 */
>>> +	num_left = per_cpu(ncwait_asm_enter, core)(vpe_mask->bits[0], online);
>>> +
>>> +	/*
>>> +	 * If this VPE is the first to leave the non-coherent wait state then
>>> +	 * it needs to wake up any coupled VPEs still running their wait
>>> +	 * instruction so that they return to cpuidle,
>>
>> Why is it needed ? Can't the other cpus stay idle ?
>
> Not with the current cpuidle code. Please see the end of
> cpuidle_enter_state_coupled in drivers/cpuidle/coupled.c. The code waits
> for all coupled CPUs to exit the idle state before any of them proceed.
> Whilst I suppose it would be possible to modify cpuidle to not require
> that, it would leave you with inaccurate residence statistics mentioned
> below.
>
>>
>>>        * which can then complete
>>> +	 * coordination between the coupled VPEs & provide the governor with
>>> +	 * a chance to reflect on the length of time the VPEs were in the
>>> +	 * idle state.
>>> +	 */
>>> +	if (coupled_coherence && (num_left == online))
>>> +		arch_send_call_function_ipi_mask(coupled_mask);
>>
>> Except there is no choice due to hardware limitations, I don't think this is
>> valid.
>
> By nature when one CPU (VPE) within a core leaves a non-coherent state
> the rest do too, because as I mentioned coherence is a property of the
> core not of individual VPEs. It would be possible to leave the other
> VPEs idle if we didn't differentiate between the coherent & non-coherent
> wait states, but again not with cpuidle as it is today (due to the
> waiting for all CPUs at the end of cpuidle_enter_state_coupled).

[ ... ]

>>> +
>>> +	/*
>>> +	 * Set the coupled flag on the appropriate states if this system
>>> +	 * requires it.
>>> +	 */
>>> +	if (coupled_coherence)
>>> +		for (i = 1; i < cps_driver.state_count; i++)
>>> +			cps_driver.states[i].flags |= CPUIDLE_FLAG_COUPLED;
>>
>> I am not sure CPUIDLE_FLAG_COUPLED is the solution for this driver. IIUC,
>> with the IPI above and the wakeup sync with the couple states, this driver
>> is waking up everybody instead of sleeping as much as possible.
>>
>> I would recommend to have a look at a different approach, like the one used
>> for cpuidle-ux500.c.
>
> Ok it looks like that just counts & performs work only on the last
> online CPU to run the code.

Yes, the last-man-standing approach. Usually, the couple idle states are 
used when the processors need to do some extra work or only the cpu0 can 
invoke PM routine for security reason.

In your case, IIUC, we don't care who will call the PM routine and if a 
cpu wakes up, the other ones (read the VPEs belonging to the same core), 
can stay idle until an interrupt occurs.

> As before that could work but only by
> disregarding any differentiation between coherent & non-coherent wait
> states at levels above the driver.

May be you can use the arch private flags for the idle states to 
differentiate coherent or non-coherent wait states ? Or alternatively 
create two drivers and register the right one.

>> --
>>   <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
>>


-- 
  <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

Re: [PATCH 10/10] cpuidle: cpuidle driver for MIPS CPS

[Paul Burton]

On Wed, Feb 26, 2014 at 03:37:39PM +0100, Daniel Lezcano wrote:
> >>The convention is to not include headers from arch. These headers shouldn't
> >>appear in this driver.
> >>
> >
> >Without accessing those headers I can't really implement anything useful
> >- entering these idle states by their nature involves architecture
> >specifics. Would you rather the bulk of the driver is implemented under
> >arch/mips & the code in drivers/cpuidle simply calls elsewhere to do the
> >real work?
> 
> Usually, this is how it is done. If you can move the 'asm' code inside eg.
> pm.c somewhere in arch/mips and invoke from pm functions from cpuidle
> through a simple header that would be great.

Ok.

> >Right you are, it should either use GFP_ATOMIC or just not handle the
> >off-stack case (which isn't currently used).
> 
> Why don't you just allocate these cpumasks at init time instead of
> allocating / freeing them in the fast path ?

Also a possibility. Right now nothing using this will have off-stack cpu
masks I was just attempting to be cautious by handling the case anyway.
I'll change it one way or another for v2.

> >>>+	/* Calculate which coupled CPUs (VPEs) are online */
> >>>+#ifdef CONFIG_MIPS_MT
> >>>+	cpumask_and(coupled_mask, cpu_online_mask, &dev->coupled_cpus);
> >>>+	first_cpu = cpumask_first(coupled_mask);
> >>>+	online = cpumask_weight(coupled_mask);
> >>>+	cpumask_clear_cpu(dev->cpu, coupled_mask);
> >>>+	cpumask_shift_right(vpe_mask, coupled_mask,
> >>>+			    cpumask_first(&dev->coupled_cpus));
> >>
> >>What is the purpose of this computation ?
> >
> >If you read through the code generated in cps_gen_entry_code, the
> >vpe_mask is used to indicate the VPEs within the current core which are
> >both online & not the VPE currently running the code.
> 
> Ok, thanks for the clarification.
> 
> Shouldn't 'online = cpumask_weight(coupled_mask)' be after clearing the
> current cpu ? Otherwise, online has also the current cpu which, IIUC,
> shouldn't be included, no ?

No, the generated code actually counts the number of VPEs which have
entered the generated code so that it knows it's safe to proceed to
disable coherence (which would of course be bad if any VPE was still
running C code). That count (nc_ready_count) has to equal online
(including the current CPU/VPE) & the final VPE goes on to disable
coherence whilst the others just halt.

> 
> >>>+#else
> >>>+	cpumask_clear(coupled_mask);
> >>>+	cpumask_clear(vpe_mask);
> >>>+	first_cpu = dev->cpu;
> >>>+	online = 1;
> >>>+#endif
> >>
> >>first_cpu is not used.
> >
> >Right you are. It's used in further work-in-progress patches but I'll
> >remove it from this one.
> >
> >>
> >>>+	/*
> >>>+	 * Run the generated entry code. Note that we assume the number of VPEs
> >>>+	 * within this core does not exceed the width in bits of a long. Since
> >>>+	 * MVPConf0.PVPE is 4 bits wide this seems like a safe assumption.
> >>>+	 */
> >>>+	num_left = per_cpu(ncwait_asm_enter, core)(vpe_mask->bits[0], online);
> >>>+
> >>>+	/*
> >>>+	 * If this VPE is the first to leave the non-coherent wait state then
> >>>+	 * it needs to wake up any coupled VPEs still running their wait
> >>>+	 * instruction so that they return to cpuidle,
> >>
> >>Why is it needed ? Can't the other cpus stay idle ?
> >
> >Not with the current cpuidle code. Please see the end of
> >cpuidle_enter_state_coupled in drivers/cpuidle/coupled.c. The code waits
> >for all coupled CPUs to exit the idle state before any of them proceed.
> >Whilst I suppose it would be possible to modify cpuidle to not require
> >that, it would leave you with inaccurate residence statistics mentioned
> >below.
> >
> >>
> >>>       * which can then complete
> >>>+	 * coordination between the coupled VPEs & provide the governor with
> >>>+	 * a chance to reflect on the length of time the VPEs were in the
> >>>+	 * idle state.
> >>>+	 */
> >>>+	if (coupled_coherence && (num_left == online))
> >>>+		arch_send_call_function_ipi_mask(coupled_mask);
> >>
> >>Except there is no choice due to hardware limitations, I don't think this is
> >>valid.
> >
> >By nature when one CPU (VPE) within a core leaves a non-coherent state
> >the rest do too, because as I mentioned coherence is a property of the
> >core not of individual VPEs. It would be possible to leave the other
> >VPEs idle if we didn't differentiate between the coherent & non-coherent
> >wait states, but again not with cpuidle as it is today (due to the
> >waiting for all CPUs at the end of cpuidle_enter_state_coupled).
> 
> [ ... ]
> 
> >>>+
> >>>+	/*
> >>>+	 * Set the coupled flag on the appropriate states if this system
> >>>+	 * requires it.
> >>>+	 */
> >>>+	if (coupled_coherence)
> >>>+		for (i = 1; i < cps_driver.state_count; i++)
> >>>+			cps_driver.states[i].flags |= CPUIDLE_FLAG_COUPLED;
> >>
> >>I am not sure CPUIDLE_FLAG_COUPLED is the solution for this driver. IIUC,
> >>with the IPI above and the wakeup sync with the couple states, this driver
> >>is waking up everybody instead of sleeping as much as possible.
> >>
> >>I would recommend to have a look at a different approach, like the one used
> >>for cpuidle-ux500.c.
> >
> >Ok it looks like that just counts & performs work only on the last
> >online CPU to run the code.
> 
> Yes, the last-man-standing approach. Usually, the couple idle states are
> used when the processors need to do some extra work or only the cpu0 can
> invoke PM routine for security reason.
> 
> In your case, IIUC, we don't care who will call the PM routine and if a cpu
> wakes up, the other ones (read the VPEs belonging to the same core), can
> stay idle until an interrupt occurs.

Right, we don't care which CPU/VPE disables coherence or which one
re-enables coherence. However when one of them does so it does affect
the rest. So although they could stay idle it's a question of *which*
idle state they're now in. As soon as one VPE has left the idle state
the rest would now be in a coherent wait state. If they don't wake up
then cpuidle continues to believe they are in non-coherent wait.

> 
> >As before that could work but only by
> >disregarding any differentiation between coherent & non-coherent wait
> >states at levels above the driver.
> 
> May be you can use the arch private flags for the idle states to
> differentiate coherent or non-coherent wait states ?

I'm sure the driver could keep the coherent vs non-coherent
differentiation internally, but my point is that the governor would no
longer have access to it. That may or may not be a big issue, but one
other thing to bear in mind is that as I mentioned in the commit message
non-coherent wait is very much just a stepping stone towards entering
lower power states (clock gating or power gating the cores). Those lower
power states also require treating VPEs within a core as coupled, and
whilst the secondary ones could run a wait instruction on resume that
would be to further detriment of the residency statistics.

> Or alternatively create
> two drivers and register the right one.

I'm unsure how that would help here - could you explain what you mean?

Paul