[PATCH] cpuidle: sirf : Add cpuidle for SiRFprimaII and SiRFatlasVI

[PATCH] cpuidle: sirf : Add cpuidle for SiRFprimaII and SiRFatlasVI

[Barry Song]

From: Rongjun Ying <Rongjun.Ying@csr.com>

This patch adds support cpuidle for CSR SiRFprimaII and SiRFatlasVI unicore
ARM Cortex-A9 SoCs.

Signed-off-by: Rongjun Ying <Rongjun.Ying@csr.com>
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
 - this patch depens on  Rongjun Ying's "[PATCH] cpufreq: sirf: Add cpufreq
 for SiRFprimaII and SiRFatlasVI" at:
 http://marc.info/?l=linux-pm&m=138103042106089&w=2

 arch/arm/configs/prima2_defconfig |   2 +
 drivers/cpuidle/Kconfig.arm       |   6 ++
 drivers/cpuidle/Makefile          |   1 +
 drivers/cpuidle/cpuidle-sirf.c    | 131 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 140 insertions(+)
 create mode 100644 drivers/cpuidle/cpuidle-sirf.c

diff --git a/arch/arm/configs/prima2_defconfig b/arch/arm/configs/prima2_defconfig
index 513d75e..bc0bf1f 100644
--- a/arch/arm/configs/prima2_defconfig
+++ b/arch/arm/configs/prima2_defconfig
@@ -18,6 +18,8 @@ CONFIG_PREEMPT=y
 CONFIG_AEABI=y
 CONFIG_KEXEC=y
 CONFIG_CPU_FREQ=y
+CONFIG_CPU_IDLE=y
+CONFIG_ARM_SIRF_CPUIDLE=y
 CONFIG_BINFMT_MISC=y
 CONFIG_PM_RUNTIME=y
 CONFIG_UEVENT_HELPER_PATH="/sbin/hotplug"
diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index 8e36603..c1f5fbf 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -15,6 +15,12 @@ config ARM_KIRKWOOD_CPUIDLE
 	help
 	  This adds the CPU Idle driver for Marvell Kirkwood SoCs.
 
+config ARM_SIRF_CPUIDLE
+	bool "CPU Idle Driver for CSR SiRFprimaII and CSRatlasVI SoCs"
+	depends on ARCH_SIRF
+	help
+	  This adds the CPU Idle driver for CSR SiRF SoCs.
+
 config ARM_ZYNQ_CPUIDLE
 	bool "CPU Idle Driver for Xilinx Zynq processors"
 	depends on ARCH_ZYNQ
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index cea5ef5..f8cce16 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
 # ARM SoC drivers
 obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE)	+= cpuidle-calxeda.o
 obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
+obj-$(CONFIG_ARM_SIRF_CPUIDLE)		+= cpuidle-sirf.o
 obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o
 obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
 obj-$(CONFIG_CPU_IDLE_BIG_LITTLE)	+= cpuidle-big_little.o
diff --git a/drivers/cpuidle/cpuidle-sirf.c b/drivers/cpuidle/cpuidle-sirf.c
new file mode 100644
index 0000000..fc1f71e
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-sirf.c
@@ -0,0 +1,131 @@
+/*
+ * CPU idle drivers for CSR SiRFprimaII
+ *
+ * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company.
+ *
+ * Licensed under GPLv2 or later.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/cpuidle.h>
+#include <linux/io.h>
+#include <linux/time.h>
+#include <asm/proc-fns.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <asm/cpuidle.h>
+#include <linux/regulator/consumer.h>
+#include <linux/cpu.h>
+#include <linux/opp.h>
+
+#define SIRFSOC_MAX_VOLTAGE	1200000
+
+/*
+ * now we have only 2 C state WFI
+ * 1. ARM WFI
+ * 2. WFI + switch to 26Mhz clock source + lower volatge
+ */
+
+static struct {
+	struct clk		*cpu_clk;
+	struct clk		*osc_clk;
+	struct regulator	*vcore_regulator;
+	struct device		*cpu_dev;
+} sirf_cpuidle;
+
+static int sirf_enter_idle(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv,
+				int index)
+{
+	struct clk *parent_clk;
+	unsigned long volt_old = 0, volt_new, freq;
+	struct opp *opp;
+
+	local_irq_disable();
+	parent_clk = clk_get_parent(sirf_cpuidle.cpu_clk);
+	clk_set_parent(sirf_cpuidle.cpu_clk, sirf_cpuidle.osc_clk);
+
+	if (sirf_cpuidle.vcore_regulator) {
+		volt_old = regulator_get_voltage(sirf_cpuidle.vcore_regulator);
+
+		freq = clk_get_rate(sirf_cpuidle.osc_clk);
+		rcu_read_lock();
+		opp = opp_find_freq_ceil(sirf_cpuidle.cpu_dev, &freq);
+		if (IS_ERR(opp)) {
+			rcu_read_unlock();
+			return -EINVAL;
+		}
+
+		volt_new = opp_get_voltage(opp);
+		rcu_read_unlock();
+
+		regulator_set_voltage(sirf_cpuidle.vcore_regulator, volt_new,
+				SIRFSOC_MAX_VOLTAGE);
+	}
+	/* Wait for interrupt state */
+	cpu_do_idle();
+
+	if (sirf_cpuidle.vcore_regulator)
+		regulator_set_voltage(sirf_cpuidle.vcore_regulator, volt_old,
+				SIRFSOC_MAX_VOLTAGE);
+
+	clk_set_parent(sirf_cpuidle.cpu_clk, parent_clk);
+	/* Todo: other C states */
+
+	local_irq_enable();
+	return index;
+}
+
+static struct cpuidle_driver sirf_cpuidle_driver = {
+	.name = "sirf_cpuidle",
+	.states = {
+		ARM_CPUIDLE_WFI_STATE,
+		{
+			.name = "WFI-LP",
+			.desc = "WFI low power",
+			.flags = CPUIDLE_FLAG_TIME_VALID,
+			.exit_latency = 10,
+			.target_residency = 10000,
+			.enter = sirf_enter_idle,
+		},
+	},
+	.state_count = 2,
+};
+
+/* Initialize CPU idle by registering the idle states */
+static int sirfsoc_init_cpuidle(void)
+{
+	int ret = 0;
+
+	sirf_cpuidle.cpu_clk = clk_get_sys("cpu", NULL);
+	if (IS_ERR(sirf_cpuidle.cpu_clk))
+		return PTR_ERR(sirf_cpuidle.cpu_clk);
+
+	sirf_cpuidle.osc_clk = clk_get_sys("osc", NULL);
+	if (IS_ERR(sirf_cpuidle.osc_clk)) {
+		ret = PTR_ERR(sirf_cpuidle.osc_clk);
+		goto out_put_osc_clk;
+	}
+
+	sirf_cpuidle.vcore_regulator = regulator_get(NULL, "vcore");
+	if (IS_ERR(sirf_cpuidle.vcore_regulator))
+		sirf_cpuidle.vcore_regulator = NULL;
+
+	sirf_cpuidle.cpu_dev = get_cpu_device(0);
+	if (IS_ERR(sirf_cpuidle.cpu_dev)) {
+		ret = PTR_ERR(sirf_cpuidle.cpu_dev);
+		goto out_put_clk;
+	}
+
+	ret = cpuidle_register(&sirf_cpuidle_driver, NULL);
+	if (!ret)
+		return ret;
+
+out_put_clk:
+	clk_put(sirf_cpuidle.cpu_clk);
+out_put_osc_clk:
+	clk_put(sirf_cpuidle.osc_clk);
+	return ret;
+}
+late_initcall(sirfsoc_init_cpuidle);
-- 
1.8.2.3

Re: [PATCH] cpuidle: sirf : Add cpuidle for SiRFprimaII and SiRFatlasVI

[Daniel Lezcano]

Hi Barry,

On 10/06/2013 06:26 AM, Barry Song wrote:
> From: Rongjun Ying <Rongjun.Ying@csr.com>
>
> This patch adds support cpuidle for CSR SiRFprimaII and SiRFatlasVI unicore
> ARM Cortex-A9 SoCs.

A new driver deserves a better description.

> Signed-off-by: Rongjun Ying <Rongjun.Ying@csr.com>
> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> ---

Is there a technical reference manual accessible somewhere ?

>   - this patch depens on  Rongjun Ying's "[PATCH] cpufreq: sirf: Add cpufreq
>   for SiRFprimaII and SiRFatlasVI" at:
>   http://marc.info/?l=linux-pm&m=138103042106089&w=2
>
>   arch/arm/configs/prima2_defconfig |   2 +
>   drivers/cpuidle/Kconfig.arm       |   6 ++
>   drivers/cpuidle/Makefile          |   1 +
>   drivers/cpuidle/cpuidle-sirf.c    | 131 ++++++++++++++++++++++++++++++++++++++
>   4 files changed, 140 insertions(+)
>   create mode 100644 drivers/cpuidle/cpuidle-sirf.c
>
> diff --git a/arch/arm/configs/prima2_defconfig b/arch/arm/configs/prima2_defconfig
> index 513d75e..bc0bf1f 100644
> --- a/arch/arm/configs/prima2_defconfig
> +++ b/arch/arm/configs/prima2_defconfig
> @@ -18,6 +18,8 @@ CONFIG_PREEMPT=y
>   CONFIG_AEABI=y
>   CONFIG_KEXEC=y
>   CONFIG_CPU_FREQ=y
> +CONFIG_CPU_IDLE=y
> +CONFIG_ARM_SIRF_CPUIDLE=y
>   CONFIG_BINFMT_MISC=y
>   CONFIG_PM_RUNTIME=y
>   CONFIG_UEVENT_HELPER_PATH="/sbin/hotplug"
> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> index 8e36603..c1f5fbf 100644
> --- a/drivers/cpuidle/Kconfig.arm
> +++ b/drivers/cpuidle/Kconfig.arm
> @@ -15,6 +15,12 @@ config ARM_KIRKWOOD_CPUIDLE
>   	help
>   	  This adds the CPU Idle driver for Marvell Kirkwood SoCs.
>
> +config ARM_SIRF_CPUIDLE
> +	bool "CPU Idle Driver for CSR SiRFprimaII and CSRatlasVI SoCs"
> +	depends on ARCH_SIRF
> +	help
> +	  This adds the CPU Idle driver for CSR SiRF SoCs.
> +
>   config ARM_ZYNQ_CPUIDLE
>   	bool "CPU Idle Driver for Xilinx Zynq processors"
>   	depends on ARCH_ZYNQ
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index cea5ef5..f8cce16 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
>   # ARM SoC drivers
>   obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE)	+= cpuidle-calxeda.o
>   obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
> +obj-$(CONFIG_ARM_SIRF_CPUIDLE)		+= cpuidle-sirf.o
>   obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o
>   obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
>   obj-$(CONFIG_CPU_IDLE_BIG_LITTLE)	+= cpuidle-big_little.o
> diff --git a/drivers/cpuidle/cpuidle-sirf.c b/drivers/cpuidle/cpuidle-sirf.c
> new file mode 100644
> index 0000000..fc1f71e
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-sirf.c
> @@ -0,0 +1,131 @@
> +/*
> + * CPU idle drivers for CSR SiRFprimaII
> + *
> + * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company.
> + *
> + * Licensed under GPLv2 or later.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/cpuidle.h>
> +#include <linux/io.h>
> +#include <linux/time.h>
> +#include <asm/proc-fns.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <asm/cpuidle.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/cpu.h>
> +#include <linux/opp.h>
> +
> +#define SIRFSOC_MAX_VOLTAGE	1200000
> +
> +/*
> + * now we have only 2 C state WFI
> + * 1. ARM WFI
> + * 2. WFI + switch to 26Mhz clock source + lower volatge
> + */
> +
> +static struct {
> +	struct clk		*cpu_clk;
> +	struct clk		*osc_clk;
> +	struct regulator	*vcore_regulator;
> +	struct device		*cpu_dev;
> +} sirf_cpuidle;
> +
> +static int sirf_enter_idle(struct cpuidle_device *dev,
> +				struct cpuidle_driver *drv,
> +				int index)
> +{
> +	struct clk *parent_clk;
> +	unsigned long volt_old = 0, volt_new, freq;
> +	struct opp *opp;
> +
> +	local_irq_disable();

Don't use local_irq_disable in the driver it is done by the caller.

> +	parent_clk = clk_get_parent(sirf_cpuidle.cpu_clk);
> +	clk_set_parent(sirf_cpuidle.cpu_clk, sirf_cpuidle.osc_clk);
> +
> +	if (sirf_cpuidle.vcore_regulator) {
> +		volt_old = regulator_get_voltage(sirf_cpuidle.vcore_regulator);
> +
> +		freq = clk_get_rate(sirf_cpuidle.osc_clk);
> +		rcu_read_lock();

Using rcu in the idle path is not allowed.

> +		opp = opp_find_freq_ceil(sirf_cpuidle.cpu_dev, &freq);
> +		if (IS_ERR(opp)) {
> +			rcu_read_unlock();
> +			return -EINVAL;
> +		}
> +
> +		volt_new = opp_get_voltage(opp);
> +		rcu_read_unlock();
> +
> +		regulator_set_voltage(sirf_cpuidle.vcore_regulator, volt_new,
> +				SIRFSOC_MAX_VOLTAGE);
> +	}
> +	/* Wait for interrupt state */
> +	cpu_do_idle();
> +
> +	if (sirf_cpuidle.vcore_regulator)
> +		regulator_set_voltage(sirf_cpuidle.vcore_regulator, volt_old,
> +				SIRFSOC_MAX_VOLTAGE);
> +
> +	clk_set_parent(sirf_cpuidle.cpu_clk, parent_clk);
> +	/* Todo: other C states */

It sounds very weird you have this freq/volt/opp code here.

If you hit idle, the cpufreq driver didn't put the cpu in the state you 
are trying to bring here ?

There isn't a power management unit on this board ?

> +	local_irq_enable();

Done by the caller.

> +	return index;
> +}
> +
> +static struct cpuidle_driver sirf_cpuidle_driver = {
> +	.name = "sirf_cpuidle",
> +	.states = {
> +		ARM_CPUIDLE_WFI_STATE,
> +		{
> +			.name = "WFI-LP",
> +			.desc = "WFI low power",
> +			.flags = CPUIDLE_FLAG_TIME_VALID,
> +			.exit_latency = 10,
> +			.target_residency = 10000,
> +			.enter = sirf_enter_idle,
> +		},
> +	},
> +	.state_count = 2,
> +};
> +
> +/* Initialize CPU idle by registering the idle states */
> +static int sirfsoc_init_cpuidle(void)
> +{
> +	int ret = 0;
> +
> +	sirf_cpuidle.cpu_clk = clk_get_sys("cpu", NULL);
> +	if (IS_ERR(sirf_cpuidle.cpu_clk))
> +		return PTR_ERR(sirf_cpuidle.cpu_clk);
> +
> +	sirf_cpuidle.osc_clk = clk_get_sys("osc", NULL);
> +	if (IS_ERR(sirf_cpuidle.osc_clk)) {
> +		ret = PTR_ERR(sirf_cpuidle.osc_clk);
> +		goto out_put_osc_clk;
> +	}
> +
> +	sirf_cpuidle.vcore_regulator = regulator_get(NULL, "vcore");
> +	if (IS_ERR(sirf_cpuidle.vcore_regulator))
> +		sirf_cpuidle.vcore_regulator = NULL;
> +
> +	sirf_cpuidle.cpu_dev = get_cpu_device(0);
> +	if (IS_ERR(sirf_cpuidle.cpu_dev)) {
> +		ret = PTR_ERR(sirf_cpuidle.cpu_dev);
> +		goto out_put_clk;
> +	}
> +
> +	ret = cpuidle_register(&sirf_cpuidle_driver, NULL);
> +	if (!ret)
> +		return ret;
> +
> +out_put_clk:
> +	clk_put(sirf_cpuidle.cpu_clk);
> +out_put_osc_clk:
> +	clk_put(sirf_cpuidle.osc_clk);
> +	return ret;
> +}
> +late_initcall(sirfsoc_init_cpuidle);

Please convert it to platform_driver as the other drivers are moving to 
(cf. cpuidle-kirkwood, cpuidle-ux500).

Thanks
   -- Daniel

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

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

答复: [PATCH] cpuidle: sirf : Add cpuidle for SiRFprimaII and SiRFatlasVI

[Rongjun Ying]

> -----邮件原件-----
> 发件人: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
> 发送时间: Monday, October 07, 2013 4:24 PM
> 收件人: Barry Song; rjw@sisk.pl
> 抄送: linux-pm@vger.kernel.org; DL-SHA-WorkGroupLinux; Rongjun Ying; Barry
> Song
> 主题: Re: [PATCH] cpuidle: sirf : Add cpuidle for SiRFprimaII and SiRFatlasVI
> 
> 
> Hi Barry,
> 
> On 10/06/2013 06:26 AM, Barry Song wrote:
> > From: Rongjun Ying <Rongjun.Ying@csr.com>
> >
> > This patch adds support cpuidle for CSR SiRFprimaII and SiRFatlasVI
> > unicore ARM Cortex-A9 SoCs.
> 
> A new driver deserves a better description.
> 
> > Signed-off-by: Rongjun Ying <Rongjun.Ying@csr.com>
> > Signed-off-by: Barry Song <Baohua.Song@csr.com>
> > ---
> 
> Is there a technical reference manual accessible somewhere ?

No, I have not. If system need enter idle mode, Not need operate any SoC register. SiRF SoC enter cpuidle mode is very simple,
Which only set the parent clock to OSC(26MHz)  and adjust voltage to min.

> 
> >   - this patch depens on  Rongjun Ying's "[PATCH] cpufreq: sirf: Add cpufreq
> >   for SiRFprimaII and SiRFatlasVI" at:
> >   http://marc.info/?l=linux-pm&m=138103042106089&w=2
> >
> >   arch/arm/configs/prima2_defconfig |   2 +
> >   drivers/cpuidle/Kconfig.arm       |   6 ++
> >   drivers/cpuidle/Makefile          |   1 +
> >   drivers/cpuidle/cpuidle-sirf.c    | 131
> ++++++++++++++++++++++++++++++++++++++
> >   4 files changed, 140 insertions(+)
> >   create mode 100644 drivers/cpuidle/cpuidle-sirf.c
> >
> > diff --git a/arch/arm/configs/prima2_defconfig
> > b/arch/arm/configs/prima2_defconfig
> > index 513d75e..bc0bf1f 100644
> > --- a/arch/arm/configs/prima2_defconfig
> > +++ b/arch/arm/configs/prima2_defconfig
> > @@ -18,6 +18,8 @@ CONFIG_PREEMPT=y
> >   CONFIG_AEABI=y
> >   CONFIG_KEXEC=y
> >   CONFIG_CPU_FREQ=y
> > +CONFIG_CPU_IDLE=y
> > +CONFIG_ARM_SIRF_CPUIDLE=y
> >   CONFIG_BINFMT_MISC=y
> >   CONFIG_PM_RUNTIME=y
> >   CONFIG_UEVENT_HELPER_PATH="/sbin/hotplug"
> > diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> > index 8e36603..c1f5fbf 100644
> > --- a/drivers/cpuidle/Kconfig.arm
> > +++ b/drivers/cpuidle/Kconfig.arm
> > @@ -15,6 +15,12 @@ config ARM_KIRKWOOD_CPUIDLE
> >   	help
> >   	  This adds the CPU Idle driver for Marvell Kirkwood SoCs.
> >
> > +config ARM_SIRF_CPUIDLE
> > +	bool "CPU Idle Driver for CSR SiRFprimaII and CSRatlasVI SoCs"
> > +	depends on ARCH_SIRF
> > +	help
> > +	  This adds the CPU Idle driver for CSR SiRF SoCs.
> > +
> >   config ARM_ZYNQ_CPUIDLE
> >   	bool "CPU Idle Driver for Xilinx Zynq processors"
> >   	depends on ARCH_ZYNQ
> > diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile index
> > cea5ef5..f8cce16 100644
> > --- a/drivers/cpuidle/Makefile
> > +++ b/drivers/cpuidle/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) +=
> coupled.o
> >   # ARM SoC drivers
> >   obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE)	+= cpuidle-calxeda.o
> >   obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
> > +obj-$(CONFIG_ARM_SIRF_CPUIDLE)		+= cpuidle-sirf.o
> >   obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o
> >   obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
> >   obj-$(CONFIG_CPU_IDLE_BIG_LITTLE)	+= cpuidle-big_little.o
> > diff --git a/drivers/cpuidle/cpuidle-sirf.c
> > b/drivers/cpuidle/cpuidle-sirf.c new file mode 100644 index
> > 0000000..fc1f71e
> > --- /dev/null
> > +++ b/drivers/cpuidle/cpuidle-sirf.c
> > @@ -0,0 +1,131 @@
> > +/*
> > + * CPU idle drivers for CSR SiRFprimaII
> > + *
> > + * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group
> company.
> > + *
> > + * Licensed under GPLv2 or later.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/init.h>
> > +#include <linux/cpuidle.h>
> > +#include <linux/io.h>
> > +#include <linux/time.h>
> > +#include <asm/proc-fns.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <asm/cpuidle.h>
> > +#include <linux/regulator/consumer.h> #include <linux/cpu.h> #include
> > +<linux/opp.h>
> > +
> > +#define SIRFSOC_MAX_VOLTAGE	1200000
> > +
> > +/*
> > + * now we have only 2 C state WFI
> > + * 1. ARM WFI
> > + * 2. WFI + switch to 26Mhz clock source + lower volatge  */
> > +
> > +static struct {
> > +	struct clk		*cpu_clk;
> > +	struct clk		*osc_clk;
> > +	struct regulator	*vcore_regulator;
> > +	struct device		*cpu_dev;
> > +} sirf_cpuidle;
> > +
> > +static int sirf_enter_idle(struct cpuidle_device *dev,
> > +				struct cpuidle_driver *drv,
> > +				int index)
> > +{
> > +	struct clk *parent_clk;
> > +	unsigned long volt_old = 0, volt_new, freq;
> > +	struct opp *opp;
> > +
> > +	local_irq_disable();
> 
> Don't use local_irq_disable in the driver it is done by the caller.

Yes, I will remove it.

> 
> > +	parent_clk = clk_get_parent(sirf_cpuidle.cpu_clk);
> > +	clk_set_parent(sirf_cpuidle.cpu_clk, sirf_cpuidle.osc_clk);
> > +
> > +	if (sirf_cpuidle.vcore_regulator) {
> > +		volt_old = regulator_get_voltage(sirf_cpuidle.vcore_regulator);
> > +
> > +		freq = clk_get_rate(sirf_cpuidle.osc_clk);
> > +		rcu_read_lock();
> 
> Using rcu in the idle path is not allowed.

Yes, I will remove it.

> 
> > +		opp = opp_find_freq_ceil(sirf_cpuidle.cpu_dev, &freq);
> > +		if (IS_ERR(opp)) {
> > +			rcu_read_unlock();
> > +			return -EINVAL;
> > +		}
> > +
> > +		volt_new = opp_get_voltage(opp);
> > +		rcu_read_unlock();
> > +
> > +		regulator_set_voltage(sirf_cpuidle.vcore_regulator, volt_new,
> > +				SIRFSOC_MAX_VOLTAGE);
> > +	}
> > +	/* Wait for interrupt state */
> > +	cpu_do_idle();
> > +
> > +	if (sirf_cpuidle.vcore_regulator)
> > +		regulator_set_voltage(sirf_cpuidle.vcore_regulator, volt_old,
> > +				SIRFSOC_MAX_VOLTAGE);
> > +
> > +	clk_set_parent(sirf_cpuidle.cpu_clk, parent_clk);
> > +	/* Todo: other C states */
> 
> It sounds very weird you have this freq/volt/opp code here.
> 
> If you hit idle, the cpufreq driver didn't put the cpu in the state you are trying to
> bring here ?
> 
> There isn't a power management unit on this board ?
> 

By SiRF SoC, when enter idle mode, I set the freq to 26MHz and min voltage. 
That's will save the power consume and reduce leakage current.

> > +	local_irq_enable();
> 
> Done by the caller.
> 

Yes. I will remove it.

> > +	return index;
> > +}
> > +
> > +static struct cpuidle_driver sirf_cpuidle_driver = {
> > +	.name = "sirf_cpuidle",
> > +	.states = {
> > +		ARM_CPUIDLE_WFI_STATE,
> > +		{
> > +			.name = "WFI-LP",
> > +			.desc = "WFI low power",
> > +			.flags = CPUIDLE_FLAG_TIME_VALID,
> > +			.exit_latency = 10,
> > +			.target_residency = 10000,
> > +			.enter = sirf_enter_idle,
> > +		},
> > +	},
> > +	.state_count = 2,
> > +};
> > +
> > +/* Initialize CPU idle by registering the idle states */ static int
> > +sirfsoc_init_cpuidle(void) {
> > +	int ret = 0;
> > +
> > +	sirf_cpuidle.cpu_clk = clk_get_sys("cpu", NULL);
> > +	if (IS_ERR(sirf_cpuidle.cpu_clk))
> > +		return PTR_ERR(sirf_cpuidle.cpu_clk);
> > +
> > +	sirf_cpuidle.osc_clk = clk_get_sys("osc", NULL);
> > +	if (IS_ERR(sirf_cpuidle.osc_clk)) {
> > +		ret = PTR_ERR(sirf_cpuidle.osc_clk);
> > +		goto out_put_osc_clk;
> > +	}
> > +
> > +	sirf_cpuidle.vcore_regulator = regulator_get(NULL, "vcore");
> > +	if (IS_ERR(sirf_cpuidle.vcore_regulator))
> > +		sirf_cpuidle.vcore_regulator = NULL;
> > +
> > +	sirf_cpuidle.cpu_dev = get_cpu_device(0);
> > +	if (IS_ERR(sirf_cpuidle.cpu_dev)) {
> > +		ret = PTR_ERR(sirf_cpuidle.cpu_dev);
> > +		goto out_put_clk;
> > +	}
> > +
> > +	ret = cpuidle_register(&sirf_cpuidle_driver, NULL);
> > +	if (!ret)
> > +		return ret;
> > +
> > +out_put_clk:
> > +	clk_put(sirf_cpuidle.cpu_clk);
> > +out_put_osc_clk:
> > +	clk_put(sirf_cpuidle.osc_clk);
> > +	return ret;
> > +}
> > +late_initcall(sirfsoc_init_cpuidle);
> 
> Please convert it to platform_driver as the other drivers are moving to (cf.
> cpuidle-kirkwood, cpuidle-ux500).
> 

I think cpuidle is not actual device which can't be added to device tree system.
So where register this device?

> Thanks
>    -- Daniel
> 
> --
>   <http://www.linaro.org/> Linaro.org │ Open source software for ARM
> SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
> 
> 
> 
>  To report this email as spam click
> https://www.mailcontrol.com/sr/MZbqvYs5QwJvpeaetUwhCQ==
> SfbLvsGngckguWXFmltKBzrZwLHxUaTRDUX55WOZtQUzGWYsdQ== .

Thanks
RongJun Ying


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

Re: 答复: [PATCH] cpuidle: sirf : Add cpuidle for SiRFprimaII and SiRFatlasVI

[Daniel Lezcano]

On 10/08/2013 09:44 AM, Rongjun Ying wrote:
>
>
>> -----邮件原件-----
>> 发件人: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
>> 发送时间: Monday, October 07, 2013 4:24 PM
>> 收件人: Barry Song; rjw@sisk.pl
>> 抄送: linux-pm@vger.kernel.org; DL-SHA-WorkGroupLinux; Rongjun Ying; Barry
>> Song
>> 主题: Re: [PATCH] cpuidle: sirf : Add cpuidle for SiRFprimaII and SiRFatlasVI
>>
>>
>> Hi Barry,
>>
>> On 10/06/2013 06:26 AM, Barry Song wrote:
>>> From: Rongjun Ying <Rongjun.Ying@csr.com>
>>>
>>> This patch adds support cpuidle for CSR SiRFprimaII and SiRFatlasVI
>>> unicore ARM Cortex-A9 SoCs.
>>
>> A new driver deserves a better description.
>>
>>> Signed-off-by: Rongjun Ying <Rongjun.Ying@csr.com>
>>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>>> ---
>>
>> Is there a technical reference manual accessible somewhere ?
>
> No, I have not.

You haven't one ? How can you write a cpuidle driver without it ?

You didn't answer my question:

Is there a technical reference manual accessible somewhere ?

> If system need enter idle mode, Not need operate any SoC register. SiRF SoC enter cpuidle mode is very simple,
> Which only set the parent clock to OSC(26MHz)  and adjust voltage to min.

This is up to cpufreq.

[ ... ]

>>> +static int sirf_enter_idle(struct cpuidle_device *dev,
>>> +				struct cpuidle_driver *drv,
>>> +				int index)
>>> +{
>>> +	struct clk *parent_clk;
>>> +	unsigned long volt_old = 0, volt_new, freq;
>>> +	struct opp *opp;
>>> +
>>> +	local_irq_disable();
>>
>> Don't use local_irq_disable in the driver it is done by the caller.
>
> Yes, I will remove it.
>
>>
>>> +	parent_clk = clk_get_parent(sirf_cpuidle.cpu_clk);
>>> +	clk_set_parent(sirf_cpuidle.cpu_clk, sirf_cpuidle.osc_clk);
>>> +
>>> +	if (sirf_cpuidle.vcore_regulator) {
>>> +		volt_old = regulator_get_voltage(sirf_cpuidle.vcore_regulator);
>>> +
>>> +		freq = clk_get_rate(sirf_cpuidle.osc_clk);
>>> +		rcu_read_lock();
>>
>> Using rcu in the idle path is not allowed.
>
> Yes, I will remove it.
>
>>
>>> +		opp = opp_find_freq_ceil(sirf_cpuidle.cpu_dev, &freq);
>>> +		if (IS_ERR(opp)) {
>>> +			rcu_read_unlock();
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		volt_new = opp_get_voltage(opp);
>>> +		rcu_read_unlock();
>>> +
>>> +		regulator_set_voltage(sirf_cpuidle.vcore_regulator, volt_new,
>>> +				SIRFSOC_MAX_VOLTAGE);
>>> +	}
>>> +	/* Wait for interrupt state */
>>> +	cpu_do_idle();
>>> +
>>> +	if (sirf_cpuidle.vcore_regulator)
>>> +		regulator_set_voltage(sirf_cpuidle.vcore_regulator, volt_old,
>>> +				SIRFSOC_MAX_VOLTAGE);
>>> +
>>> +	clk_set_parent(sirf_cpuidle.cpu_clk, parent_clk);
>>> +	/* Todo: other C states */
>>
>> It sounds very weird you have this freq/volt/opp code here.
>>
>> If you hit idle, the cpufreq driver didn't put the cpu in the state you are trying to
>> bring here ?
>>
>> There isn't a power management unit on this board ?
>>
>
> By SiRF SoC, when enter idle mode, I set the freq to 26MHz and min voltage.
> That's will save the power consume and reduce leakage current.

Really ? :)

You didn't answer the questions.

1. this code shouldn't be here but in cpufreq which is already the case 
with the other patch you sent with this one. So I am convinced if you 
remove the OPP here and let cpufreq to handle that, and keep the 
cpu_do_idle, you won't see any difference with the current code.

2. Moreover, removing the OPP, there is only the WFI state which is the 
default idle function. Hence the cpuidle driver itself has no interest 
and you can simply remove it, except if you bring another idle state 
with the driver.

3. More idle states are often handled through a PMU (Power Management 
Unit) giving cpu power off, retention, cluster power down, etc ... Is 
there one on this board ?

The TRM can help to answer these questions.

>>> +	local_irq_enable();
>>
>> Done by the caller.
>>
>
> Yes. I will remove it.
>
>>> +	return index;
>>> +}
>>> +
>>> +static struct cpuidle_driver sirf_cpuidle_driver = {
>>> +	.name = "sirf_cpuidle",
>>> +	.states = {
>>> +		ARM_CPUIDLE_WFI_STATE,
>>> +		{
>>> +			.name = "WFI-LP",
>>> +			.desc = "WFI low power",
>>> +			.flags = CPUIDLE_FLAG_TIME_VALID,
>>> +			.exit_latency = 10,
>>> +			.target_residency = 10000,

By the way, why 10000 ? Is it measured ?

>>> +			.enter = sirf_enter_idle,
>>> +		},
>>> +	},
>>> +	.state_count = 2,

[ cut ]

>>> +
>>> +	ret = cpuidle_register(&sirf_cpuidle_driver, NULL);
>>> +	if (!ret)
>>> +		return ret;
>>> +
>>> +out_put_clk:
>>> +	clk_put(sirf_cpuidle.cpu_clk);
>>> +out_put_osc_clk:
>>> +	clk_put(sirf_cpuidle.osc_clk);
>>> +	return ret;
>>> +}
>>> +late_initcall(sirfsoc_init_cpuidle);
>>
>> Please convert it to platform_driver as the other drivers are moving to (cf.
>> cpuidle-kirkwood, cpuidle-ux500).
>>
>
> I think cpuidle is not actual device which can't be added to device tree system.
> So where register this device?

I gave you a couple of examples above, you can rely on it to implement 
something similar.

Thanks
   -- Daniel

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

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

Re: 答复: [PATCH] cpuidle: sirf : Add cpuidle for SiRFprimaII and SiRFatlasVI

[Barry Song]

>>>> +       if (sirf_cpuidle.vcore_regulator)
>>>> +               regulator_set_voltage(sirf_cpuidle.vcore_regulator,
>>>> volt_old,
>>>> +                               SIRFSOC_MAX_VOLTAGE);
>>>> +
>>>> +       clk_set_parent(sirf_cpuidle.cpu_clk, parent_clk);
>>>> +       /* Todo: other C states */
>>>
>>>
>>> It sounds very weird you have this freq/volt/opp code here.
>>>
>>> If you hit idle, the cpufreq driver didn't put the cpu in the state you
>>> are trying to
>>> bring here ?
>>>
>>> There isn't a power management unit on this board ?
>>>
>>
>> By SiRF SoC, when enter idle mode, I set the freq to 26MHz and min
>> voltage.
>> That's will save the power consume and reduce leakage current.
>
>
> Really ? :)
>
> You didn't answer the questions.
>
> 1. this code shouldn't be here but in cpufreq which is already the case with
> the other patch you sent with this one. So I am convinced if you remove the
> OPP here and let cpufreq to handle that, and keep the cpu_do_idle, you won't
> see any difference with the current code.
>
> 2. Moreover, removing the OPP, there is only the WFI state which is the
> default idle function. Hence the cpuidle driver itself has no interest and
> you can simply remove it, except if you bring another idle state with the
> driver.
>
> 3. More idle states are often handled through a PMU (Power Management Unit)
> giving cpu power off, retention, cluster power down, etc ... Is there one on
> this board ?
>
> The TRM can help to answer these questions.
>
Hi Daniel,
here the codes actually want to put cpu into lowest freq and lowest
voltage. because according to our test, power leakage will be much
lower when cpu is in the status than we put it into WFI directly in a
higher freq.

here it is difficult to say cpufreq will do that automatically as it
is decided by cpufreq policy.

there is no power management unit which can put prima2 into a
different idle status except WFI.

any suggestion from you about this chip issue?

-barry

Re: 答复: [PATCH] cpuidle: sirf : Add cpuidle for SiRFprimaII and SiRFatlasVI

[Daniel Lezcano]

On 10/08/2013 03:27 PM, Barry Song wrote:
>>>>> +       if (sirf_cpuidle.vcore_regulator)
>>>>> +               regulator_set_voltage(sirf_cpuidle.vcore_regulator,
>>>>> volt_old,
>>>>> +                               SIRFSOC_MAX_VOLTAGE);
>>>>> +
>>>>> +       clk_set_parent(sirf_cpuidle.cpu_clk, parent_clk);
>>>>> +       /* Todo: other C states */
>>>>
>>>>
>>>> It sounds very weird you have this freq/volt/opp code here.
>>>>
>>>> If you hit idle, the cpufreq driver didn't put the cpu in the state you
>>>> are trying to
>>>> bring here ?
>>>>
>>>> There isn't a power management unit on this board ?
>>>>
>>>
>>> By SiRF SoC, when enter idle mode, I set the freq to 26MHz and min
>>> voltage.
>>> That's will save the power consume and reduce leakage current.
>>
>>
>> Really ? :)
>>
>> You didn't answer the questions.
>>
>> 1. this code shouldn't be here but in cpufreq which is already the case with
>> the other patch you sent with this one. So I am convinced if you remove the
>> OPP here and let cpufreq to handle that, and keep the cpu_do_idle, you won't
>> see any difference with the current code.
>>
>> 2. Moreover, removing the OPP, there is only the WFI state which is the
>> default idle function. Hence the cpuidle driver itself has no interest and
>> you can simply remove it, except if you bring another idle state with the
>> driver.
>>
>> 3. More idle states are often handled through a PMU (Power Management Unit)
>> giving cpu power off, retention, cluster power down, etc ... Is there one on
>> this board ?
>>
>> The TRM can help to answer these questions.
>>
> Hi Daniel,
> here the codes actually want to put cpu into lowest freq and lowest
> voltage. because according to our test, power leakage will be much
> lower when cpu is in the status than we put it into WFI directly in a
> higher freq.
>
> here it is difficult to say cpufreq will do that automatically as it
> is decided by cpufreq policy.
>
> there is no power management unit which can put prima2 into a
> different idle status except WFI.
>
> any suggestion from you about this chip issue?

Not so much without further information. Definitively without the TRM it 
is hard to give any hints on how to go further.

Do you have the ability to do RAM self refresh ? If not, without a power 
management unit on your board, in my opinion you can drop the cpuidle 
driver because it means you can't do retention or poweroff the core.

Moreover, the OPP stuff, from a POV design, is tied with cpufreq and 
changing that from cpuidle will violate the decisions of the cpufreq 
governor.

Did you do some measurements without cpuidle but with the cpufreq 
governor 'ondemand' and compare that with the cpuidle driver you are 
submitting ?

Hopefully, Viresh (Cc'ed) can give its opinion.

Thanks
   -- Daniel


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

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

Re: 答复: [PATCH] cpuidle: sirf : Add cpuidle for SiRFprimaII and SiRFatlasVI

[Barry Song]

2013/10/9 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 10/08/2013 03:27 PM, Barry Song wrote:
>>>>>>
>>>>>> +       if (sirf_cpuidle.vcore_regulator)
>>>>>> +               regulator_set_voltage(sirf_cpuidle.vcore_regulator,
>>>>>> volt_old,
>>>>>> +                               SIRFSOC_MAX_VOLTAGE);
>>>>>> +
>>>>>> +       clk_set_parent(sirf_cpuidle.cpu_clk, parent_clk);
>>>>>> +       /* Todo: other C states */
>>>>>
>>>>>
>>>>>
>>>>> It sounds very weird you have this freq/volt/opp code here.
>>>>>
>>>>> If you hit idle, the cpufreq driver didn't put the cpu in the state you
>>>>> are trying to
>>>>> bring here ?
>>>>>
>>>>> There isn't a power management unit on this board ?
>>>>>
>>>>
>>>> By SiRF SoC, when enter idle mode, I set the freq to 26MHz and min
>>>> voltage.
>>>> That's will save the power consume and reduce leakage current.
>>>
>>>
>>>
>>> Really ? :)
>>>
>>> You didn't answer the questions.
>>>
>>> 1. this code shouldn't be here but in cpufreq which is already the case
>>> with
>>> the other patch you sent with this one. So I am convinced if you remove
>>> the
>>> OPP here and let cpufreq to handle that, and keep the cpu_do_idle, you
>>> won't
>>> see any difference with the current code.
>>>
>>> 2. Moreover, removing the OPP, there is only the WFI state which is the
>>> default idle function. Hence the cpuidle driver itself has no interest
>>> and
>>> you can simply remove it, except if you bring another idle state with the
>>> driver.
>>>
>>> 3. More idle states are often handled through a PMU (Power Management
>>> Unit)
>>> giving cpu power off, retention, cluster power down, etc ... Is there one
>>> on
>>> this board ?
>>>
>>> The TRM can help to answer these questions.
>>>
>> Hi Daniel,
>> here the codes actually want to put cpu into lowest freq and lowest
>> voltage. because according to our test, power leakage will be much
>> lower when cpu is in the status than we put it into WFI directly in a
>> higher freq.
>>
>> here it is difficult to say cpufreq will do that automatically as it
>> is decided by cpufreq policy.
>>
>> there is no power management unit which can put prima2 into a
>> different idle status except WFI.
>>
>> any suggestion from you about this chip issue?
>
>
> Not so much without further information. Definitively without the TRM it is
> hard to give any hints on how to go further.
>
> Do you have the ability to do RAM self refresh ? If not, without a power
> management unit on your board, in my opinion you can drop the cpuidle driver
> because it means you can't do retention or poweroff the core.

if dropping the cpuidle, actually the only left mode is WFI. but here
the problem is that WFI is not enough for us due to our chips have
some current leak in WFI. so here the driver is specific to sirf to
fix the current leak in WFI.

and i also don't think cpufreq will put hardware to low vol and low
freq automatically. at least for performance and user, it is
impossible. for other policies like ondemand, i'd like to have rongjun
collect some proof by doing some run+sleep +run+sleep... loop to check
the actual behaviour.

>
> Moreover, the OPP stuff, from a POV design, is tied with cpufreq and
> changing that from cpuidle will violate the decisions of the cpufreq
> governor.
>
> Did you do some measurements without cpuidle but with the cpufreq governor
> 'ondemand' and compare that with the cpuidle driver you are submitting ?

i'd like rongjun will provide measured data on real boards we have
done before. i think the WFI+low vol+low freq comes from some real
measurement before.

>
> Hopefully, Viresh (Cc'ed) can give its opinion.
>
>
> Thanks
>   -- Daniel

rongjun, pls keep one eye on this thread until we provide enough
persuasiveness and all problems are clarified here.

-barry

Re: 答复: [PATCH] cpuidle: sirf : Add cpuidle for SiRFprimaII and SiRFatlasVI

[Daniel Lezcano]

On 10/10/2013 12:24 PM, Barry Song wrote:
> 2013/10/9 Daniel Lezcano <daniel.lezcano@linaro.org>:
>> On 10/08/2013 03:27 PM, Barry Song wrote:
>>>>>>>
>>>>>>> +       if (sirf_cpuidle.vcore_regulator)
>>>>>>> +               regulator_set_voltage(sirf_cpuidle.vcore_regulator,
>>>>>>> volt_old,
>>>>>>> +                               SIRFSOC_MAX_VOLTAGE);
>>>>>>> +
>>>>>>> +       clk_set_parent(sirf_cpuidle.cpu_clk, parent_clk);
>>>>>>> +       /* Todo: other C states */
>>>>>>
>>>>>>
>>>>>>
>>>>>> It sounds very weird you have this freq/volt/opp code here.
>>>>>>
>>>>>> If you hit idle, the cpufreq driver didn't put the cpu in the state you
>>>>>> are trying to
>>>>>> bring here ?
>>>>>>
>>>>>> There isn't a power management unit on this board ?
>>>>>>
>>>>>
>>>>> By SiRF SoC, when enter idle mode, I set the freq to 26MHz and min
>>>>> voltage.
>>>>> That's will save the power consume and reduce leakage current.
>>>>
>>>>
>>>>
>>>> Really ? :)
>>>>
>>>> You didn't answer the questions.
>>>>
>>>> 1. this code shouldn't be here but in cpufreq which is already the case
>>>> with
>>>> the other patch you sent with this one. So I am convinced if you remove
>>>> the
>>>> OPP here and let cpufreq to handle that, and keep the cpu_do_idle, you
>>>> won't
>>>> see any difference with the current code.
>>>>
>>>> 2. Moreover, removing the OPP, there is only the WFI state which is the
>>>> default idle function. Hence the cpuidle driver itself has no interest
>>>> and
>>>> you can simply remove it, except if you bring another idle state with the
>>>> driver.
>>>>
>>>> 3. More idle states are often handled through a PMU (Power Management
>>>> Unit)
>>>> giving cpu power off, retention, cluster power down, etc ... Is there one
>>>> on
>>>> this board ?
>>>>
>>>> The TRM can help to answer these questions.
>>>>
>>> Hi Daniel,
>>> here the codes actually want to put cpu into lowest freq and lowest
>>> voltage. because according to our test, power leakage will be much
>>> lower when cpu is in the status than we put it into WFI directly in a
>>> higher freq.
>>>
>>> here it is difficult to say cpufreq will do that automatically as it
>>> is decided by cpufreq policy.
>>>
>>> there is no power management unit which can put prima2 into a
>>> different idle status except WFI.
>>>
>>> any suggestion from you about this chip issue?
>>
>>
>> Not so much without further information. Definitively without the TRM it is
>> hard to give any hints on how to go further.
>>
>> Do you have the ability to do RAM self refresh ? If not, without a power
>> management unit on your board, in my opinion you can drop the cpuidle driver
>> because it means you can't do retention or poweroff the core.
>
> if dropping the cpuidle, actually the only left mode is WFI. but here
> the problem is that WFI is not enough for us due to our chips have
> some current leak in WFI. so here the driver is specific to sirf to
> fix the current leak in WFI.
>
> and i also don't think cpufreq will put hardware to low vol and low
> freq automatically. at least for performance and user, it is
> impossible.

Yes and that is the proof the OPP in cpuidle violates the cpufreq 
governor decision. Let's pick an example:

I don't know your board so I will assume we have 300, 600, 900 MHz.

The user set the policy to 'userspace' and sets the freq to 600MHz, the 
cpuidle will change that to 900MHz when exiting idle.

The user set the policy to 'performance', cpuidle switch to 300MHz 
despite the cpufreq governor told 900MHz.

The user set the policy to 'ondemand', the cpufreq governor set the OPP 
to 600MHz but cpuidle change it to 300MHz, then 900MHz.

I am not a cpufreq expert but IMHO that will mess up the cpufreq 
governor, without talking about the freq changes notifiers.

Don't you have warning in dmesg ?

"Warning: CPU frequency out of sync: cpufreq and timing ..."

> for other policies like ondemand, i'd like to have rongjun
> collect some proof by doing some run+sleep +run+sleep... loop to check
> the actual behaviour.

Yeah, that would be nice.

>> Moreover, the OPP stuff, from a POV design, is tied with cpufreq and
>> changing that from cpuidle will violate the decisions of the cpufreq
>> governor.
>>
>> Did you do some measurements without cpuidle but with the cpufreq governor
>> 'ondemand' and compare that with the cpuidle driver you are submitting ?
>
> i'd like rongjun will provide measured data on real boards we have
> done before. i think the WFI+low vol+low freq comes from some real
> measurement before.
>
>>
>> Hopefully, Viresh (Cc'ed) can give its opinion.
>>
>>
>> Thanks
>>    -- Daniel
>
> rongjun, pls keep one eye on this thread until we provide enough
> persuasiveness and all problems are clarified here.
>
> -barry
>


-- 
  <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] cpuidle: sirf : Add cpuidle for SiRFprimaII and SiRFatlasVI

[Barry Song]

2013/10/10 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 10/10/2013 12:24 PM, Barry Song wrote:
>>
>> 2013/10/9 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>>
>>> On 10/08/2013 03:27 PM, Barry Song wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> +       if (sirf_cpuidle.vcore_regulator)
>>>>>>>> +               regulator_set_voltage(sirf_cpuidle.vcore_regulator,
>>>>>>>> volt_old,
>>>>>>>> +                               SIRFSOC_MAX_VOLTAGE);
>>>>>>>> +
>>>>>>>> +       clk_set_parent(sirf_cpuidle.cpu_clk, parent_clk);
>>>>>>>> +       /* Todo: other C states */
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> It sounds very weird you have this freq/volt/opp code here.
>>>>>>>
>>>>>>> If you hit idle, the cpufreq driver didn't put the cpu in the state
>>>>>>> you
>>>>>>> are trying to
>>>>>>> bring here ?
>>>>>>>
>>>>>>> There isn't a power management unit on this board ?
>>>>>>>
>>>>>>
>>>>>> By SiRF SoC, when enter idle mode, I set the freq to 26MHz and min
>>>>>> voltage.
>>>>>> That's will save the power consume and reduce leakage current.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Really ? :)
>>>>>
>>>>> You didn't answer the questions.
>>>>>
>>>>> 1. this code shouldn't be here but in cpufreq which is already the case
>>>>> with
>>>>> the other patch you sent with this one. So I am convinced if you remove
>>>>> the
>>>>> OPP here and let cpufreq to handle that, and keep the cpu_do_idle, you
>>>>> won't
>>>>> see any difference with the current code.
>>>>>
>>>>> 2. Moreover, removing the OPP, there is only the WFI state which is the
>>>>> default idle function. Hence the cpuidle driver itself has no interest
>>>>> and
>>>>> you can simply remove it, except if you bring another idle state with
>>>>> the
>>>>> driver.
>>>>>
>>>>> 3. More idle states are often handled through a PMU (Power Management
>>>>> Unit)
>>>>> giving cpu power off, retention, cluster power down, etc ... Is there
>>>>> one
>>>>> on
>>>>> this board ?
>>>>>
>>>>> The TRM can help to answer these questions.
>>>>>
>>>> Hi Daniel,
>>>> here the codes actually want to put cpu into lowest freq and lowest
>>>> voltage. because according to our test, power leakage will be much
>>>> lower when cpu is in the status than we put it into WFI directly in a
>>>> higher freq.
>>>>
>>>> here it is difficult to say cpufreq will do that automatically as it
>>>> is decided by cpufreq policy.
>>>>
>>>> there is no power management unit which can put prima2 into a
>>>> different idle status except WFI.
>>>>
>>>> any suggestion from you about this chip issue?
>>>
>>>
>>>
>>> Not so much without further information. Definitively without the TRM it
>>> is
>>> hard to give any hints on how to go further.
>>>
>>> Do you have the ability to do RAM self refresh ? If not, without a power
>>> management unit on your board, in my opinion you can drop the cpuidle
>>> driver
>>> because it means you can't do retention or poweroff the core.
>>
>>
>> if dropping the cpuidle, actually the only left mode is WFI. but here
>> the problem is that WFI is not enough for us due to our chips have
>> some current leak in WFI. so here the driver is specific to sirf to
>> fix the current leak in WFI.
>>
>> and i also don't think cpufreq will put hardware to low vol and low
>> freq automatically. at least for performance and user, it is
>> impossible.
>
>
> Yes and that is the proof the OPP in cpuidle violates the cpufreq governor
> decision. Let's pick an example:
>
> I don't know your board so I will assume we have 300, 600, 900 MHz.
>
> The user set the policy to 'userspace' and sets the freq to 600MHz, the
> cpuidle will change that to 900MHz when exiting idle.
>
> The user set the policy to 'performance', cpuidle switch to 300MHz despite
> the cpufreq governor told 900MHz.
>
> The user set the policy to 'ondemand', the cpufreq governor set the OPP to
> 600MHz but cpuidle change it to 300MHz, then 900MHz.

it seems not. cpuidle is the final task. when cpuidle is entering,
actually other tasks have sleeped.
btw, rongjun's codes actually save the current cpufreq -> enter lowest
vol/freq -> WFI -> restore saved cpufreq.

so if the current cpufreq is 600MHz, then the story is
1. saving 600MHz
2. goto 300MHz
3. WFI
4. restoring to 600MHz

so there is no conflict with cpufreq considering the whole "save the
current cpufreq -> enter lowest vol/freq -> WFI -> restore saved
cpufreq" is in a atomic context.


>
> I am not a cpufreq expert but IMHO that will mess up the cpufreq governor,
> without talking about the freq changes notifiers.
>
> Don't you have warning in dmesg ?
>
> "Warning: CPU frequency out of sync: cpufreq and timing ..."

i think no.

>
>
>> for other policies like ondemand, i'd like to have rongjun
>> collect some proof by doing some run+sleep +run+sleep... loop to check
>> the actual behaviour.
>
>
> Yeah, that would be nice.

i would hope the test can cover things like "run xx ms -> sleep yy ms
-> run xx ms -> sleep yy ms" and we change the xx/yy in different test
cases to check the result.


>
>

-barry

Re: 答复: [PATCH] cpuidle: sirf : Add cpuidle for SiRFprimaII and SiRFatlasVI

[Daniel Lezcano]

On 10/10/2013 03:07 PM, Barry Song wrote:
> 2013/10/10 Daniel Lezcano <daniel.lezcano@linaro.org>:
>> On 10/10/2013 12:24 PM, Barry Song wrote:
>>>
>>> 2013/10/9 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>>>
>>>> On 10/08/2013 03:27 PM, Barry Song wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> +       if (sirf_cpuidle.vcore_regulator)
>>>>>>>>> +               regulator_set_voltage(sirf_cpuidle.vcore_regulator,
>>>>>>>>> volt_old,
>>>>>>>>> +                               SIRFSOC_MAX_VOLTAGE);
>>>>>>>>> +
>>>>>>>>> +       clk_set_parent(sirf_cpuidle.cpu_clk, parent_clk);
>>>>>>>>> +       /* Todo: other C states */
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> It sounds very weird you have this freq/volt/opp code here.
>>>>>>>>
>>>>>>>> If you hit idle, the cpufreq driver didn't put the cpu in the state
>>>>>>>> you
>>>>>>>> are trying to
>>>>>>>> bring here ?
>>>>>>>>
>>>>>>>> There isn't a power management unit on this board ?
>>>>>>>>
>>>>>>>
>>>>>>> By SiRF SoC, when enter idle mode, I set the freq to 26MHz and min
>>>>>>> voltage.
>>>>>>> That's will save the power consume and reduce leakage current.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Really ? :)
>>>>>>
>>>>>> You didn't answer the questions.
>>>>>>
>>>>>> 1. this code shouldn't be here but in cpufreq which is already the case
>>>>>> with
>>>>>> the other patch you sent with this one. So I am convinced if you remove
>>>>>> the
>>>>>> OPP here and let cpufreq to handle that, and keep the cpu_do_idle, you
>>>>>> won't
>>>>>> see any difference with the current code.
>>>>>>
>>>>>> 2. Moreover, removing the OPP, there is only the WFI state which is the
>>>>>> default idle function. Hence the cpuidle driver itself has no interest
>>>>>> and
>>>>>> you can simply remove it, except if you bring another idle state with
>>>>>> the
>>>>>> driver.
>>>>>>
>>>>>> 3. More idle states are often handled through a PMU (Power Management
>>>>>> Unit)
>>>>>> giving cpu power off, retention, cluster power down, etc ... Is there
>>>>>> one
>>>>>> on
>>>>>> this board ?
>>>>>>
>>>>>> The TRM can help to answer these questions.
>>>>>>
>>>>> Hi Daniel,
>>>>> here the codes actually want to put cpu into lowest freq and lowest
>>>>> voltage. because according to our test, power leakage will be much
>>>>> lower when cpu is in the status than we put it into WFI directly in a
>>>>> higher freq.
>>>>>
>>>>> here it is difficult to say cpufreq will do that automatically as it
>>>>> is decided by cpufreq policy.
>>>>>
>>>>> there is no power management unit which can put prima2 into a
>>>>> different idle status except WFI.
>>>>>
>>>>> any suggestion from you about this chip issue?
>>>>
>>>>
>>>>
>>>> Not so much without further information. Definitively without the TRM it
>>>> is
>>>> hard to give any hints on how to go further.
>>>>
>>>> Do you have the ability to do RAM self refresh ? If not, without a power
>>>> management unit on your board, in my opinion you can drop the cpuidle
>>>> driver
>>>> because it means you can't do retention or poweroff the core.
>>>
>>>
>>> if dropping the cpuidle, actually the only left mode is WFI. but here
>>> the problem is that WFI is not enough for us due to our chips have
>>> some current leak in WFI. so here the driver is specific to sirf to
>>> fix the current leak in WFI.
>>>
>>> and i also don't think cpufreq will put hardware to low vol and low
>>> freq automatically. at least for performance and user, it is
>>> impossible.
>>
>>
>> Yes and that is the proof the OPP in cpuidle violates the cpufreq governor
>> decision. Let's pick an example:
>>
>> I don't know your board so I will assume we have 300, 600, 900 MHz.
>>
>> The user set the policy to 'userspace' and sets the freq to 600MHz, the
>> cpuidle will change that to 900MHz when exiting idle.
>>
>> The user set the policy to 'performance', cpuidle switch to 300MHz despite
>> the cpufreq governor told 900MHz.
>>
>> The user set the policy to 'ondemand', the cpufreq governor set the OPP to
>> 600MHz but cpuidle change it to 300MHz, then 900MHz.
>
> it seems not. cpuidle is the final task. when cpuidle is entering,
> actually other tasks have sleeped.
> btw, rongjun's codes actually save the current cpufreq -> enter lowest
> vol/freq -> WFI -> restore saved cpufreq.
>
> so if the current cpufreq is 600MHz, then the story is
> 1. saving 600MHz
> 2. goto 300MHz
> 3. WFI
> 4. restoring to 600MHz
>
> so there is no conflict with cpufreq considering the whole "save the
> current cpufreq -> enter lowest vol/freq -> WFI -> restore saved
> cpufreq" is in a atomic context.

Ok, may be my comment is inappropriate. I see you change the clock 
parent to 'osc' to 26MHz. If the cpu is WFI it is clock gated, no ?

>> I am not a cpufreq expert but IMHO that will mess up the cpufreq governor,
>> without talking about the freq changes notifiers.
>>
>> Don't you have warning in dmesg ?
>>
>> "Warning: CPU frequency out of sync: cpufreq and timing ..."
>
> i think no.
>
>>
>>
>>> for other policies like ondemand, i'd like to have rongjun
>>> collect some proof by doing some run+sleep +run+sleep... loop to check
>>> the actual behaviour.
>>
>>
>> Yeah, that would be nice.
>
> i would hope the test can cover things like "run xx ms -> sleep yy ms
> -> run xx ms -> sleep yy ms" and we change the xx/yy in different test
> cases to check the result.
>
>
>>
>>
>
> -barry
>


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