Re: [PATCH v6] ARM: imx: Add basic imx6q thermal driver
On Tue, Jun 26, 2012 at 11:51 PM, Robert Lee rob@linaro.org wrote:
Add imx anatop peripheral thermal driver and use it for imx6q builds.
This driver hooks into the linux thermal framework which provides a
sysfs interface for temperature readings and other information and
a mechanism to shutdown the system upon crossing a critical
temperature trip point.
Only the sysfs interface and a critcial trip are supported by this
patch and not any active trip points or cooling devices.
The thermal driver is defaulted to be enabled which required the
anatopmfd driver to be defaulted to enabled.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/boot/dts/imx6q.dtsi |5 +
drivers/mfd/Kconfig |1 +
drivers/thermal/Kconfig |9 +
drivers/thermal/Makefile |1 +
drivers/thermal/imx_anatop_thermal.c | 465
++
5 files changed, 481 insertions(+)
create mode 100644 drivers/thermal/imx_anatop_thermal.c
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index d026f30..b53a16a 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -449,6 +449,10 @@
anatop-min-voltage = 725000;
anatop-max-voltage = 145;
};
+
+ thermal {
+ compatible =fsl,anatop-thermal;
+ };
};
usbphy@020c9000 { /* USBPHY1 */
@@ -666,6 +670,7 @@
};
ocotp@021bc000 {
+ compatible = fsl,imx6q-ocotp;
reg = 0x021bc000 0x4000;
};
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index e129c82..552fae3 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -915,6 +915,7 @@ config MFD_STA2X11
config MFD_ANATOP
bool Support for Freescale i.MX on-chip ANATOP controller
depends on SOC_IMX6Q
+ default y
help
Select this option to enable Freescale i.MX on-chip ANATOP
MFD controller. This controller embeds regulator and
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 04c6796..a35a35e 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -30,6 +30,15 @@ config CPU_THERMAL
and not the ACPI interface.
If you want this support, you should say Y or M here.
+config IMX_ANATOP_THERMAL
+ bool imx anatop soc thermal driver
+ depends on MFD_ANATOP CPU_THERMAL
I forgot to replace CPU_THERMAL with THERMAL
+ default y
+ help
+ Enable the on-chip temperature sensor and register the linux thermal
+ framework to provide SoC temperature data to sysfs and a basic
+ shutdown mechanism to prevent the SoC from overheating.
+
config SPEAR_THERMAL
bool SPEAr thermal sensor driver
depends on THERMAL
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 4636e35..4dd4570 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_THERMAL) += thermal_sys.o
obj-$(CONFIG_CPU_THERMAL) += cpu_cooling.o
obj-$(CONFIG_SPEAR_THERMAL)+= spear_thermal.o
obj-$(CONFIG_EXYNOS_THERMAL) += exynos_thermal.o
+obj-$(CONFIG_IMX_ANATOP_THERMAL) += imx_anatop_thermal.o
diff --git a/drivers/thermal/imx_anatop_thermal.c
b/drivers/thermal/imx_anatop_thermal.c
new file mode 100644
index 000..a932fe8
--- /dev/null
+++ b/drivers/thermal/imx_anatop_thermal.c
@@ -0,0 +1,465 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/*
+ * Thermal driver for i.MX anatop systems. Implented by hooking in to the
+ * linux thermal framework.
+ */
+
+#include linux/delay.h
+#include linux/device.h
+#include linux/dmi.h
+#include linux/init.h
+#include linux/io.h
+#include linux/kernel.h
+#include linux/mfd/anatop.h
+#include linux/module.h
+#include linux/of.h
+#include linux/of_address.h
+#include linux/platform_device.h
+#include linux/smp.h
+#include linux/slab.h
+#include linux/syscalls.h
+#include linux/thermal.h
+#include linux/types.h
+
+/* register define of anatop */
+#define HW_ANADIG_ANA_MISC00x0150
+#define HW_ANADIG_ANA_MISC0_SET0x0154
+#define HW_ANADIG_ANA_MISC0_CLR
Re: [PATCH v6] ARM: imx: Add basic imx6q thermal driver
On Wed, Jun 27, 2012 at 8:48 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Tue, Jun 26, 2012 at 11:51:55PM -0500, Robert Lee wrote:
+static inline int anatop_get_temp(int *temp, struct thermal_zone_device
*tzdev)
+{
+ unsigned int n_meas;
+ unsigned int reg;
+ struct imx_anatop_tsdata *sd;
+
+ sd = ((struct imx_anatop_thdata *)tzdev-devdata)-sensor_data;
+
+
+ do {
+ /*
+ * Every time we measure the temperature, we will power on the
+ * temperature sensor, enable measurements, take a reading,
+ * disable measurements, power off the temperature sensor.
+ */
+ sd-handle_suspend = false;
+
+ anatop_write_reg(sd-anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
+ BM_ANADIG_TEMPSENSE0_POWER_DOWN);
+ anatop_write_reg(sd-anatopmfd, HW_ANADIG_TEMPSENSE0,
+ BM_ANADIG_TEMPSENSE0_MEASURE_TEMP,
+ BM_ANADIG_TEMPSENSE0_MEASURE_TEMP);
+ /*
+ * According to the anatop temp sensor designers, it may
require
+ * up to ~17us to complete a measurement (imx6q).
+ * But this timing isn't checked on every part nor is it
+ * specified in the datasheet, so sleeping at least 1ms should
+ * provide plenty of time. Sleeping longer than 1ms is ok so
no
+ * need for usleep_range */
+ msleep(1);
+
+ reg = anatop_read_reg(sd-anatopmfd, HW_ANADIG_TEMPSENSE0);
+
+ anatop_write_reg(sd-anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
+ BM_ANADIG_TEMPSENSE0_MEASURE_TEMP);
+ anatop_write_reg(sd-anatopmfd, HW_ANADIG_TEMPSENSE0,
+ BM_ANADIG_TEMPSENSE0_POWER_DOWN,
+ BM_ANADIG_TEMPSENSE0_POWER_DOWN);
+
+ /* if we had a suspend and resume event, we will re-take the reading */
+ } while (sd-handle_suspend);
[...]
+static int anatop_thermal_suspend(struct platform_device *pdev,
+ pm_message_t state)
+{
+ struct imx_anatop_thdata *dd = platform_get_drvdata(pdev);
+ struct imx_anatop_tsdata *sd = dd-sensor_data;
+
+ /* power off the sensor during suspend */
+ anatop_write_reg(sd-anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
+ BM_ANADIG_TEMPSENSE0_MEASURE_TEMP);
+
+ anatop_write_reg(sd-anatopmfd, HW_ANADIG_TEMPSENSE0,
+ BM_ANADIG_TEMPSENSE0_POWER_DOWN,
+ BM_ANADIG_TEMPSENSE0_POWER_DOWN);
+ return 0;
+}
+
+static int anatop_thermal_resume(struct platform_device *pdev)
+{
+ struct imx_anatop_thdata *dd = platform_get_drvdata(pdev);
+ struct imx_anatop_tsdata *sd = dd-sensor_data;
+
+ sd-handle_suspend = true;
+ return 0;
+}
I don't know how to handle this properly or if it's really needed, but
the usage of this handle_suspend variable really looks suspicious. I've
never seen a driver looping around a while-suspended-in-between. Someone
else should have a look over this.
I'll add some more context that may help you and others in analyzing this.
As mentioned, this driver hooks into the linux thermal framework.
Upon registration with this is framework, the thermal framework
creates a kernel thread which periodically checks the temperature and
takes any necessary action. So anatop_get_temp periodically gets
called. Now if a suspend were to occur during the msleep() call of
this function for example, the temperature read afterword may not be
valid. So the loop check can detect that a suspend had occurred and
that the temperature sensor value read was invalid so it will try
again.
Another option would be for the resume function to return the
temperature sensor to its previous state that it was in right before
suspend. This would require the resume to re-enable the temperature
sensor and spin in a loop waiting for the temperature measurement to
complete (aka, checking a hardware bit for completion) which
theoretically may take up to ~17us. In this case, a timeout could be
added to this loop if we don't trust the hardware, and the looping in
the anatop_get_temp could be removed. This function already has some
handling if an invalid temp sensor reading is detected.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 1/5] thermal: Add generic cpufreq cooling implementation
Hey Amit, I was just re-organizing the imx thermal driver that uses this cpu cooling interface and noticed a couple of small issues here. If these suggestions are agreed upon and if it's too late for these issues be changed with this patchset, I can submit them separately unless you'd prefer to. On Sat, May 12, 2012 at 4:40 AM, Amit Daniel Kachhap amit.kach...@linaro.org wrote: This patch adds support for generic cpu thermal cooling low level implementations using frequency scaling up/down based on the registration parameters. Different cpu related cooling devices can be registered by the user and the binding of these cooling devices to the corresponding trip points can be easily done as the registration APIs return the cooling device pointer. The user of these APIs are responsible for passing clipping frequency . The drivers can also register to recieve notification about any cooling action called. Signed-off-by: Amit Daniel Kachhap amit.kach...@linaro.org --- Documentation/thermal/cpu-cooling-api.txt | 60 drivers/thermal/Kconfig | 11 + drivers/thermal/Makefile | 3 +- drivers/thermal/cpu_cooling.c | 483 + include/linux/cpu_cooling.h | 99 ++ 5 files changed, 655 insertions(+), 1 deletions(-) create mode 100644 Documentation/thermal/cpu-cooling-api.txt create mode 100644 drivers/thermal/cpu_cooling.c create mode 100644 include/linux/cpu_cooling.h diff --git a/Documentation/thermal/cpu-cooling-api.txt b/Documentation/thermal/cpu-cooling-api.txt new file mode 100644 index 000..557adb8 --- /dev/null +++ b/Documentation/thermal/cpu-cooling-api.txt @@ -0,0 +1,60 @@ +CPU cooling APIs How To +=== + +Written by Amit Daniel Kachhap amit.kach...@linaro.org + +Updated: 12 May 2012 + +Copyright (c) 2012 Samsung Electronics Co., Ltd(http://www.samsung.com) + +0. Introduction + +The generic cpu cooling(freq clipping, cpuhotplug etc) provides +registration/unregistration APIs to the caller. The binding of the cooling +devices to the trip point is left for the user. The registration APIs returns +the cooling device pointer. + +1. cpu cooling APIs + +1.1 cpufreq registration/unregistration APIs +1.1.1 struct thermal_cooling_device *cpufreq_cooling_register( + struct freq_clip_table *tab_ptr, unsigned int tab_size) + + This interface function registers the cpufreq cooling device with the name + thermal-cpufreq-%x. This api can support multiple instances of cpufreq + cooling devices. + + tab_ptr: The table containing the maximum value of frequency to be clipped + for each cooling state. + .freq_clip_max: Value of frequency to be clipped for each allowed + cpus. + .temp_level: Temperature level at which the frequency clamping will + happen. + .mask_val: cpumask of the allowed cpu's + tab_size: the total number of cpufreq cooling states. + +1.1.2 void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev) + + This interface function unregisters the thermal-cpufreq-%x cooling device. + + cdev: Cooling device pointer which has to be unregistered. + + +1.2 CPU cooling action notifier register/unregister interface +1.2.1 int cputherm_register_notifier(struct notifier_block *nb, + unsigned int list) + + This interface registers a driver with cpu cooling layer. The driver will + be notified when any cpu cooling action is called. + + nb: notifier function to register + list: CPUFREQ_COOLING_START or CPUFREQ_COOLING_STOP + +1.2.2 int cputherm_unregister_notifier(struct notifier_block *nb, + unsigned int list) + + This interface registers a driver with cpu cooling layer. The driver will + be notified when any cpu cooling action is called. + + nb: notifier function to register + list: CPUFREQ_COOLING_START or CPUFREQ_COOLING_STOP diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index 514a691..d9c529f 100644 --- a/drivers/thermal/Kconfig +++ b/drivers/thermal/Kconfig @@ -19,6 +19,17 @@ config THERMAL_HWMON depends on HWMON=y || HWMON=THERMAL default y +config CPU_THERMAL Perhaps the name CPU_COOLING or CPUFREQ_COOLING would more accurately describe this functionality? I like the latter since now this mechanism only supports cooling by using cpufreq. + bool generic cpu cooling support If we use CPUFREQ_COOLING, then perhaps this could say: bool cpu cooling through cpufreq frequency limiting + depends on THERMAL CPU_FREQ + help + This implements the generic cpu cooling mechanism through frequency + reduction, cpu hotplug and any other ways of reducing temperature. An + ACPI version of this already exists(drivers/acpi/processor_thermal.c). + This will be useful for platforms using
Re: [PATCH v5 0/7] cleanup imx5 idle, add imx5/6 cpuidle
On Wed, Jun 20, 2012 at 9:15 AM, Rob Lee rob@linaro.org wrote: Hello Sascha, On Wed, Jun 20, 2012 at 5:30 AM, Sascha Hauer s.ha...@pengutronix.de wrote: Hi Robert, On Mon, May 21, 2012 at 05:50:23PM -0500, Robert Lee wrote: Cleanup up imx5 idle code and enable imx5 low powe idle for imx53. Add common imx cpuidle initialization functionality and add a i.MX5 and i.MX6Q platform cpuidle implementation. I rebased this to 3.5-rc1 here: git.pengutronix.de/git/imx/linux-2.6.git imx/cpuidle Could you check if the result is ok with you? Certainly. I will let you know in a day or two. I did some basic testing and everything looks ok as far as idle and cpuidle goes. For imx51 and 53 I tested both device tree and non-device tree boot. On my imx51-babbage board, a generic imx_v6_v7_defconfig does not boot without enabling device tree and shows no output after the u-boot messages. But this also occurs on the kernel.org v3.5-rc3. Removing support for all boards except imx51-babbage makes it boot up ok. The idle changes don't seem to affect this behavior. Rob Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5] ARM: imx: Add basic imx6q cpu thermal management
On Wed, Jun 20, 2012 at 2:06 AM, Robert Lee rob@linaro.org wrote:
Add imx6q cpu thermal management driver using the new cpu cooling
interface which limits system performance via cpufreq to reduce
the cpu temperature. Temperature readings are taken using
the imx6q temperature sensor and this functionality was added
as part of this cpu thermal management driver.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/boot/dts/imx6q.dtsi | 5 +
drivers/thermal/Kconfig | 28 ++
drivers/thermal/Makefile | 1 +
drivers/thermal/imx6q_thermal.c | 593
+++
4 files changed, 627 insertions(+)
create mode 100644 drivers/thermal/imx6q_thermal.c
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 8c90cba..2650f65 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -442,6 +442,10 @@
anatop-min-voltage = 725000;
anatop-max-voltage = 145;
};
+
+ thermal {
+ compatible =fsl,anatop-thermal;
+ };
};
usbphy@020c9000 { /* USBPHY1 */
@@ -659,6 +663,7 @@
};
ocotp@021bc000 {
+ compatible = fsl,imx6q-ocotp;
reg = 0x021bc000 0x4000;
};
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 04c6796..b80c408 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -30,6 +30,34 @@ config CPU_THERMAL
and not the ACPI interface.
If you want this support, you should say Y or M here.
+config IMX6Q_THERMAL
+ bool IMX6Q Thermal interface support
+ depends on MFD_ANATOP CPU_THERMAL
+ help
+ Adds thermal management for IMX6Q.
+
+config IMX6Q_THERMAL_FAKE_CALIBRATION
+ bool IMX6Q fake temperature sensor calibration (FOR TESTING ONLY)
+ depends on IMX6Q_THERMAL
+ help
+ This enables a fake temp sensor calibration value for parts without
+ the correction calibration values burned into OCOTP. If the part
+ already has the calibrated values burned into OCOTP, enabling this
+ does nothing.
+ WARNING: Use of this feature is for testing only as it will cause
+ incorrect temperature readings which will result in incorrect system
+ thermal limiting behavior such as premature system performance
+ limiting or lack of proper performance reduction to reduce cpu
+ temperature
+
+config IMX6Q_THERMAL_FAKE_CAL_VAL
+ hex IMX6Q fake temperature sensor calibration value
+ depends on IMX6Q_THERMAL_FAKE_CALIBRATION
+ default 0x5704c67d
+ help
+ Refer to the temperature sensor section of the imx6q reference
manual
+ for more inforation on how this value is used.
+
config SPEAR_THERMAL
bool SPEAr thermal sensor driver
depends on THERMAL
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 4636e35..fc4004e 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_THERMAL) += thermal_sys.o
obj-$(CONFIG_CPU_THERMAL) += cpu_cooling.o
obj-$(CONFIG_SPEAR_THERMAL) += spear_thermal.o
obj-$(CONFIG_EXYNOS_THERMAL) += exynos_thermal.o
+obj-$(CONFIG_IMX6Q_THERMAL) += imx6q_thermal.o
diff --git a/drivers/thermal/imx6q_thermal.c b/drivers/thermal/imx6q_thermal.c
new file mode 100644
index 000..255d646
--- /dev/null
+++ b/drivers/thermal/imx6q_thermal.c
@@ -0,0 +1,593 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/* i.MX6Q Thermal Implementation */
+
+#include linux/kernel.h
+#include linux/device.h
+#include linux/module.h
+#include linux/dmi.h
+#include linux/init.h
+#include linux/slab.h
+#include linux/delay.h
+#include linux/types.h
+#include linux/thermal.h
+#include linux/io.h
+#include linux/syscalls.h
+#include linux/cpufreq.h
+#include linux/of.h
+#include linux/of_address.h
+#include linux/smp.h
+#include linux/cpu_cooling.h
+#include linux/platform_device.h
+#include linux/mfd/anatop.h
+
+/* register define of anatop */
+#define HW_ANADIG_ANA_MISC0 0x0150
+#define HW_ANADIG_ANA_MISC0_SET 0x0154
+#define HW_ANADIG_ANA_MISC0_CLR
Re: [PATCH v5] ARM: imx: Add basic imx6q cpu thermal management
Sascha, thanks for the review.
On Wed, Jun 20, 2012 at 5:11 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Wed, Jun 20, 2012 at 02:06:04AM -0500, Robert Lee wrote:
Add imx6q cpu thermal management driver using the new cpu cooling
interface which limits system performance via cpufreq to reduce
the cpu temperature. Temperature readings are taken using
the imx6q temperature sensor and this functionality was added
as part of this cpu thermal management driver.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/boot/dts/imx6q.dtsi | 5 +
drivers/thermal/Kconfig | 28 ++
drivers/thermal/Makefile | 1 +
drivers/thermal/imx6q_thermal.c | 593
+++
4 files changed, 627 insertions(+)
create mode 100644 drivers/thermal/imx6q_thermal.c
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 8c90cba..2650f65 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -442,6 +442,10 @@
anatop-min-voltage = 725000;
anatop-max-voltage = 145;
};
+
+ thermal {
+ compatible =fsl,anatop-thermal;
+ };
};
usbphy@020c9000 { /* USBPHY1 */
@@ -659,6 +663,7 @@
};
ocotp@021bc000 {
+ compatible = fsl,imx6q-ocotp;
reg = 0x021bc000 0x4000;
};
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 04c6796..b80c408 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -30,6 +30,34 @@ config CPU_THERMAL
and not the ACPI interface.
If you want this support, you should say Y or M here.
+config IMX6Q_THERMAL
+ bool IMX6Q Thermal interface support
+ depends on MFD_ANATOP CPU_THERMAL
+ help
+ Adds thermal management for IMX6Q.
+
+config IMX6Q_THERMAL_FAKE_CALIBRATION
+ bool IMX6Q fake temperature sensor calibration (FOR TESTING ONLY)
+ depends on IMX6Q_THERMAL
+ help
+ This enables a fake temp sensor calibration value for parts without
+ the correction calibration values burned into OCOTP. If the part
+ already has the calibrated values burned into OCOTP, enabling this
+ does nothing.
+ WARNING: Use of this feature is for testing only as it will cause
+ incorrect temperature readings which will result in incorrect system
+ thermal limiting behavior such as premature system performance
+ limiting or lack of proper performance reduction to reduce cpu
+ temperature
+
+config IMX6Q_THERMAL_FAKE_CAL_VAL
+ hex IMX6Q fake temperature sensor calibration value
+ depends on IMX6Q_THERMAL_FAKE_CALIBRATION
+ default 0x5704c67d
+ help
+ Refer to the temperature sensor section of the imx6q reference manual
+ for more inforation on how this value is used.
Don't add such stuff to Kconfig. If during runtime you detect that there
is no calibration data, then issue a warning and fall back to a safe
value. If you really think this should be configurable, add a sysfs
entry for it. FOR TESTING ONLY seems to imply though that it shouldn't
be configurable.
Ok, I'll remove this in v6.
+
config SPEAR_THERMAL
bool SPEAr thermal sensor driver
depends on THERMAL
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 4636e35..fc4004e 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_THERMAL) += thermal_sys.o
obj-$(CONFIG_CPU_THERMAL) += cpu_cooling.o
obj-$(CONFIG_SPEAR_THERMAL) += spear_thermal.o
obj-$(CONFIG_EXYNOS_THERMAL) += exynos_thermal.o
+obj-$(CONFIG_IMX6Q_THERMAL) += imx6q_thermal.o
diff --git a/drivers/thermal/imx6q_thermal.c
b/drivers/thermal/imx6q_thermal.c
new file mode 100644
index 000..255d646
--- /dev/null
+++ b/drivers/thermal/imx6q_thermal.c
@@ -0,0 +1,593 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/* i.MX6Q Thermal Implementation */
+
+#include linux/kernel.h
+#include linux/device.h
+#include linux/module.h
+#include linux/dmi.h
+#include linux/init.h
+#include linux/slab.h
+#include linux/delay.h
+#include linux/types.h
+#include linux/thermal.h
+#include linux/io.h
+#include linux/syscalls.h
+#include linux/cpufreq.h
+#include
Re: [PATCH v5 0/7] cleanup imx5 idle, add imx5/6 cpuidle
Hello Sascha, On Wed, Jun 20, 2012 at 5:30 AM, Sascha Hauer s.ha...@pengutronix.de wrote: Hi Robert, On Mon, May 21, 2012 at 05:50:23PM -0500, Robert Lee wrote: Cleanup up imx5 idle code and enable imx5 low powe idle for imx53. Add common imx cpuidle initialization functionality and add a i.MX5 and i.MX6Q platform cpuidle implementation. I rebased this to 3.5-rc1 here: git.pengutronix.de/git/imx/linux-2.6.git imx/cpuidle Could you check if the result is ok with you? Certainly. I will let you know in a day or two. Rob Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5] ARM: imx: Add basic imx6q cpu thermal management
On Wed, Jun 20, 2012 at 9:20 AM, Sascha Hauer s.ha...@pengutronix.de wrote: On Wed, Jun 20, 2012 at 09:12:51AM -0500, Rob Lee wrote: Sascha, thanks for the review. + +static struct imx6q_thermal_zone *th_zone; +static void __iomem *ocotp_base; This is a driver and drivers should generally be multi instance safe. I don't understand what this comment is referring to. Could you elaborate? Drivers can only be multi instance safe when all variables are inside a instance specific struct and you pass a pointer to this struct around. What if the i.MX7 has two different ocotp_base and you want to use this driver on both ocotp? Understood, thanks. I'll fix this in v6. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [linux-pm] [PATCH] cpuidle : use percpu cpuidle in the core code
Hey Daniel, Sorry for the late review/response but perhaps this will still be useful as your cpuidle work is ongoing. Most of the caller are in the boot-up code, in device_init or module_init. The other ones are doing some specific initialization on the cpuidle_device (cpuinit, like acpi) and can't use the cpuidle_register function. What about adding a callback pointer parameter to the cpuidle_register function that allows platform specific device initialization to occur before the cpuidle_device_register call is made? For reference, see my old cpuidle common init code that does this at git://git.linaro.org/people/rob_lee/linux.git cpuidle_init. With this callback, acpi and the other platforms that need to modify the cpuidle_device data can use this interface also. The necessary acpi cpuidle changes can also be found at the above git URL (although they haven't been reviewed so they are possibly flawed). Rob ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 3/7] ARM: imx: clean and consolidate imx5 suspend and idle code
On Wed, May 16, 2012 at 12:38 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
Hi Robert,
Overall this looks ok now, some comments inline.
Sascha
On Tue, May 15, 2012 at 09:33:32PM -0500, Robert Lee wrote:
The imx5 idle code that existed in mm-imx5.c is moved to pm-imx5.c.
The imx5_pm_init call is now exported and called during the
MACHINE_START late_init in supported imx5 platforms.
Remove various enabling/disabling of the gpc_dvfs clock and
enable it once during initialization. This is a very low
power clock that must be enabled during low power operations.
There are only two suspend_state_t imx5 low power modes ever
used. STOP_POWER_OFF for suspend to mem and
WAIT_UNCLOCKED_POWER_OFF for idle and suspend to standby. The
latter mode only requires 500 nanoseconds of extra hardware
exit time beyond a basic WFI operation (WAIT_CLOCKED mode) so
no other idle mode is necessary. Given this information, it
is more efficient to keep the registers in the often used
WAIT_UNCLOCKED_POWER_OFF state and only to and from the
STOP_POWER_OFF register state as needed when suspend to
mem is required.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/mach-imx/mm-imx5.c | 20 +-
arch/arm/mach-imx/pm-imx5.c | 63
++-
arch/arm/plat-mxc/include/mach/common.h | 3 +-
3 files changed, 40 insertions(+), 46 deletions(-)
diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
index d6b7e9f..bb38747 100644
--- a/arch/arm/mach-imx/mm-imx5.c
+++ b/arch/arm/mach-imx/mm-imx5.c
@@ -15,7 +15,6 @@
#include linux/init.h
#include linux/clk.h
-#include asm/system_misc.h
#include asm/mach/map.h
#include mach/hardware.h
@@ -23,23 +22,6 @@
#include mach/devices-common.h
#include mach/iomux-v3.h
-static struct clk *gpc_dvfs_clk;
-
-static void imx5_idle(void)
-{
- /* gpc clock is needed for SRPG */
- if (gpc_dvfs_clk == NULL) {
- gpc_dvfs_clk = clk_get(NULL, gpc_dvfs);
- if (IS_ERR(gpc_dvfs_clk))
- return;
- }
- clk_enable(gpc_dvfs_clk);
- mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF);
- if (!tzic_enable_wake())
- cpu_do_idle();
- clk_disable(gpc_dvfs_clk);
-}
-
/*
* Define the MX50 memory map.
*/
@@ -103,7 +85,6 @@ void __init imx51_init_early(void)
mxc_set_cpu_type(MXC_CPU_MX51);
mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR));
mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
- arm_pm_idle = imx5_idle;
}
void __init imx53_init_early(void)
@@ -238,4 +219,5 @@ void __init imx53_soc_init(void)
void __init imx51_init_late(void)
{
mx51_neon_fixup();
+ imx5_pm_init();
}
diff --git a/arch/arm/mach-imx/pm-imx5.c b/arch/arm/mach-imx/pm-imx5.c
index e26a9cb..6e62d79 100644
--- a/arch/arm/mach-imx/pm-imx5.c
+++ b/arch/arm/mach-imx/pm-imx5.c
@@ -13,18 +13,27 @@
#include linux/io.h
#include linux/err.h
#include asm/cacheflush.h
+#include asm/system_misc.h
#include asm/tlbflush.h
#include mach/common.h
#include mach/hardware.h
#include crm-regs-imx5.h
-static struct clk *gpc_dvfs_clk;
+/*
+ * The WAIT_UNCLOCKED_POWER_OFF state only requires = 500ns to exit.
+ * This is also the lowest power state possible without affecting
+ * non-cpu parts of the system. For these reasons, imx5 should default
+ * to always using this state for cpu idling. The PM_SUSPEND_STANDBY also
+ * uses this state and needs to take no action when registers remain
confgiured
+ * for this state.
+ */
+#define IMX5_DEFAULT_CPU_IDLE_STATE WAIT_UNCLOCKED_POWER_OFF
/*
* set cpu low power mode before WFI instruction. This function is called
* mx5 because it can be used for mx50, mx51, and mx53.
*/
-void mx5_cpu_lp_set(enum mxc_cpu_pwr_mode mode)
+static void mx5_cpu_lp_set(enum mxc_cpu_pwr_mode mode)
{
u32 plat_lpc, arm_srpgcr, ccm_clpcr;
u32 empgc0, empgc1;
@@ -87,11 +96,6 @@ void mx5_cpu_lp_set(enum mxc_cpu_pwr_mode mode)
}
}
-static int mx5_suspend_prepare(void)
-{
- return clk_prepare_enable(gpc_dvfs_clk);
-}
-
static int mx5_suspend_enter(suspend_state_t state)
{
switch (state) {
@@ -99,7 +103,7 @@ static int mx5_suspend_enter(suspend_state_t state)
mx5_cpu_lp_set(STOP_POWER_OFF);
break;
case PM_SUSPEND_STANDBY:
- mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF);
+ /* DEFAULT_IDLE_STATE already configured */
break;
default:
return -EINVAL;
@@ -114,12 +118,10 @@ static int mx5_suspend_enter(suspend_state_t state)
__raw_writel(0, MXC_SRPG_EMPGC1_SRPGCR);
}
cpu_do_idle();
- return 0;
-}
-static void mx5_suspend_finish(void)
-{
- clk_disable_unprepare(gpc_dvfs_clk);
+ /* return registers to default idle state */
+
Re: [PATCH v4 4/7] ARM: imx: Enable imx53 low power idle
On Wed, May 16, 2012 at 12:47 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Tue, May 15, 2012 at 09:33:33PM -0500, Robert Lee wrote:
Add various functionality needed to enable a imx53 low power idle
state. This includes adding the imx53 gpc_dvfs clock and making a
common imx5_late_init function and initializing all imx53
MACHINE_STATE late_init calls to imx5_late_init.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/mach-imx/clock-mx51-mx53.c | 1 +
arch/arm/mach-imx/imx53-dt.c | 1 +
arch/arm/mach-imx/mach-mx53_ard.c | 1 +
arch/arm/mach-imx/mach-mx53_evk.c | 1 +
arch/arm/mach-imx/mach-mx53_loco.c | 1 +
arch/arm/mach-imx/mach-mx53_smd.c | 1 +
arch/arm/mach-imx/mm-imx5.c | 7 ++-
arch/arm/plat-mxc/include/mach/common.h | 1 +
8 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-imx/clock-mx51-mx53.c
b/arch/arm/mach-imx/clock-mx51-mx53.c
index 0847050..decedc6 100644
--- a/arch/arm/mach-imx/clock-mx51-mx53.c
+++ b/arch/arm/mach-imx/clock-mx51-mx53.c
@@ -1529,6 +1529,7 @@ static struct clk_lookup mx53_lookups[] = {
_REGISTER_CLOCK(imx-ssi.1, NULL, ssi2_clk)
_REGISTER_CLOCK(imx-ssi.2, NULL, ssi3_clk)
_REGISTER_CLOCK(imx-keypad, NULL, dummy_clk)
+ _REGISTER_CLOCK(NULL, gpc_dvfs, gpc_dvfs_clk)
This has to be rebased against the common clock patches.
Ok.
.timer = mx53_smd_timer,
.init_machine = mx53_smd_board_init,
+ .init_late = imx5_init_late,
.restart = mxc_restart,
MACHINE_END
diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
index bb38747..7740739 100644
--- a/arch/arm/mach-imx/mm-imx5.c
+++ b/arch/arm/mach-imx/mm-imx5.c
@@ -216,8 +216,13 @@ void __init imx53_soc_init(void)
ARRAY_SIZE(imx53_audmux_res));
}
+void __init imx5_init_late(void)
+{
+ imx5_pm_init();
+}
+
void __init imx51_init_late(void)
{
mx51_neon_fixup();
- imx5_pm_init();
+ imx5_init_late();
}
Where would you add i.MX53 specific code above? Hint: imx5_init_late is
the wrong function name.
I added imx5_init_late for late_init functionality that is common
among all imx5. For example, in the future imx50 may use it as well.
But I can remove this and repeat the imx5_pm_init() calls for each
platform if you prefer that.
Thanks,
Rob
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 5/7] ARM: imx: Add common imx cpuidle init functionality.
On Wed, May 16, 2012 at 12:55 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Tue, May 15, 2012 at 09:33:34PM -0500, Robert Lee wrote:
Add common cpuidle init functionality that can be used by various
imx platforms.
Signed-off-by: Robert Lee rob@linaro.org
---
+
+#ifdef CONFIG_CPU_IDLE
+extern int imx_cpuidle_init(struct cpuidle_driver *drv);
+#else
+static inline int imx_cpuidle_init(struct cpuidle_driver *drv)
+{
+ return -ENODEV;
+}
+#endif
You should return succesfully here. Think about it, if imx_cpuidle_init
fails you basically do nothing except maybe printing an error message
which will be irritating when it appears on a kernel with cpuidle
disabled.
Oops, yeah, I missed that. Will fix in v5.
Thanks,
Rob
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] ARM: imx: Modify IMX_IO_P2V macro
Hello Uwe and Sascha, On Wed, May 9, 2012 at 7:24 PM, Robert Lee rob@linaro.org wrote: A change is needed in the IMX_IO_P2V macro to allow all imx5 platforms to use common definitions when accessing registers of peripherals on the AIPS2 bus. This change was tested for mapping conflicts using the iop2v script found at git://git.pengutronix.de/git/ukl/imx-iop2v.git and by performing a bootup of a default build using imx_v6_v7_defconfig on a imx51 babbage board and imx53 loco board. Signed-off-by: Robert Lee rob@linaro.org --- arch/arm/plat-mxc/include/mach/hardware.h | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/plat-mxc/include/mach/hardware.h b/arch/arm/plat-mxc/include/mach/hardware.h index 0630513..065cc04 100644 --- a/arch/arm/plat-mxc/include/mach/hardware.h +++ b/arch/arm/plat-mxc/include/mach/hardware.h @@ -96,6 +96,7 @@ */ #define IMX_IO_P2V(x) ( \ 0xf400 + \ + (((x) 0x8000) 7) + \ I doubled checked this today and this will result in some of the platform addresses being in the 0xf600 boundary. Instead, the '+' can be made an '|' and the addresses that get generated all appear to be acceptable without any conflicts. I'll re-submit this patch with the above fix and commet changes as part of a imx5 idle cleanup series unless I'm told that a change to this macro is unacceptable. Thanks, Rob (((x) 0x5000) 6) + \ (((x) 0x0b00) 4) + \ (((x) 0x000f))) -- 1.7.10 ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 2/3] ARM: imx: Add imx5 cpuidle driver
Sascha,
On Wed, May 9, 2012 at 3:02 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Mon, May 07, 2012 at 04:16:46PM -0500, Robert Lee wrote:
Add imx5 cpuidle driver.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/mach-imx/mm-imx5.c | 42
+++---
1 file changed, 39 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
index d6b7e9f..0b3a4cc 100644
--- a/arch/arm/mach-imx/mm-imx5.c
+++ b/arch/arm/mach-imx/mm-imx5.c
@@ -20,26 +20,61 @@
#include mach/hardware.h
#include mach/common.h
+#include mach/cpuidle.h
#include mach/devices-common.h
#include mach/iomux-v3.h
static struct clk *gpc_dvfs_clk;
-static void imx5_idle(void)
+static int imx5_idle(void)
{
+ int ret = 0;
+
/* gpc clock is needed for SRPG */
if (gpc_dvfs_clk == NULL) {
gpc_dvfs_clk = clk_get(NULL, gpc_dvfs);
This clk_get should go away here and be moved somewhere to
initialization. Also, if getting this clock fails we can still
do regular cpu_do_idle. Additionally, if clk_get fails, we'll
have a ERR_PTR value in gpc_dvfs_clk in which case the
gpc_dvfs_clk == NULL won't trigger next time you are here and
then you'll enable a nonexisting clock below.
Agree. I'd prefer to enable this clock during intialization and just
leave it running. It is supposed to be a very low power clock and I
couldn't measuring any power difference with and without it being
enabled
if (IS_ERR(gpc_dvfs_clk))
- return;
+ return -ENODEV;
}
clk_enable(gpc_dvfs_clk);
mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF);
if (!tzic_enable_wake())
cpu_do_idle();
+ else
+ ret = -EBUSY;
clk_disable(gpc_dvfs_clk);
+
+ return ret;
+}
+
+static int imx5_cpuidle_enter(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int idx)
+{
+ int ret;
+
+ ret = imx5_idle();
+
+ if (ret 0)
+ return ret;
+
+ return idx;
}
+static struct cpuidle_driver imx5_cpuidle_driver = {
+ .name = imx5_cpuidle,
+ .owner = THIS_MODULE,
+ .en_core_tk_irqen = 1,
+ .states[0] = {
+ .enter = imx5_cpuidle_enter,
+ .exit_latency = 20, /* max latency at 160MHz */
+ .target_residency = 1,
+ .flags = CPUIDLE_FLAG_TIME_VALID,
+ .name = IMX5 SRPG,
+ .desc = CPU state retained,powered off,
+ },
I wonder why you don't add the default ARM_CPUIDLE_WFI_STATE_PWR state.
The above is something different, right? It has a greater exit latency
than ARM_CPUIDLE_WFI_STATE_PWR, so why don't we add it here aswell?
Yes and no. Yes this is a different state but no, it doesn't have a
significantly greater exit latency, or at least a large enough exit
latency to warrant an extra state in my opinion. According to the
i.MX5 documentation, the extra exit time beyond basic WFI required for
the WAIT_UNCLOCKED_POWER_OFF state is 500ns (this is due to a
difference in i.MX5 hardware implementation compared to all other ARM
platforms). In reality, it did require a few more microseconds to
perform in my testing just based on the extra register writes in
mx5_cpu_lp_set(). I'd like to clean up mx5_cpu_lp_set() and add a
global variable to track the previous state and to just exit out if
the new state is the same as the old. I could do this cleanup as part
of this patchset if you prefer that.
+ .state_count = 1,
+};
+
/*
* Define the MX50 memory map.
*/
@@ -103,7 +138,7 @@ void __init imx51_init_early(void)
mxc_set_cpu_type(MXC_CPU_MX51);
mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR));
mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
- arm_pm_idle = imx5_idle;
+ arm_pm_idle = (void (*)(void))imx5_idle;
Still this looks suspicious. Reading this will lead to the question why
this prototype is casted. Please just add a imx5_pm_idle with the
correct prototype.
Ok.
Thanks,
Rob
}
void __init imx53_init_early(void)
@@ -238,4 +273,5 @@ void __init imx53_soc_init(void)
void __init imx51_init_late(void)
{
mx51_neon_fixup();
+ imx_cpuidle_init(imx5_cpuidle_driver);
}
--
1.7.10
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 3/3] ARM: imx: Add imx6q cpuidle driver
On Tue, May 1, 2012 at 10:23 PM, Shawn Guo shawn@linaro.org wrote:
On Tue, May 01, 2012 at 09:12:40PM -0500, Robert Lee wrote:
Add basic imx6q cpuidle driver. For now, only basic WFI state is
supported. Deeper idle states will be added in the future.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/mach-imx/cpuidle-imx6q.c | 33 +
So, this file is not needed any more, I guess.
Yes, I missed that. It shouldn't have been part of the patch.
arch/arm/mach-imx/mach-imx6q.c | 18 ++
2 files changed, 51 insertions(+)
create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c
diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c
b/arch/arm/mach-imx/cpuidle-imx6q.c
new file mode 100644
index 000..b74557f
--- /dev/null
+++ b/arch/arm/mach-imx/cpuidle-imx6q.c
@@ -0,0 +1,33 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include linux/kernel.h
+#include linux/init.h
+#include linux/cpuidle.h
+#include linux/export.h
+#include asm/cpuidle.h
+#include mach/cpuidle.h
+
+static struct cpuidle_driver imx6q_cpuidle_driver = {
+ .name = imx6q_cpuidle,
+ .owner = THIS_MODULE,
+ .en_core_tk_irqen = 1,
+ .states[0] = ARM_CPUIDLE_WFI_STATE,
+ .state_count = 1,
+};
+
+int __init imx6q_cpuidle_init(void)
+{
+ imx_cpuidle_set_driver(imx6q_cpuidle_driver);
+
+ return 0;
+}
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index da6c1d9..21e2051 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -21,6 +21,9 @@
#include linux/of_platform.h
#include linux/phy.h
#include linux/micrel_phy.h
+#include linux/export.h
+#include linux/cpuidle.h
+#include asm/cpuidle.h
#include asm/smp_twd.h
#include asm/hardware/cache-l2x0.h
#include asm/hardware/gic.h
@@ -29,6 +32,7 @@
#include asm/system_misc.h
#include mach/common.h
#include mach/hardware.h
+#include mach/cpuidle.h
The headers here are mostly sorted in names, so please ...
Do you mean sorted alphabetically per group? So the last three
includes should look like this instead?
...
#include mach/common.h
#include mach/cpuidle.h
#include mach/hardware.h
...
Thanks,
Rob
Regards,
Shawn
void imx6q_restart(char mode, const char *cmd)
{
@@ -86,6 +90,19 @@ static void __init imx6q_init_machine(void)
imx6q_pm_init();
}
+static struct cpuidle_driver imx6q_cpuidle_driver = {
+ .name = imx6q_cpuidle,
+ .owner = THIS_MODULE,
+ .en_core_tk_irqen = 1,
+ .states[0] = ARM_CPUIDLE_WFI_STATE,
+ .state_count = 1,
+};
+
+static void __init imx6q_init_late(void)
+{
+ imx_cpuidle_init(imx6q_cpuidle_driver);
+}
+
static void __init imx6q_map_io(void)
{
imx_lluart_map_io();
@@ -142,6 +159,7 @@ DT_MACHINE_START(IMX6Q, Freescale i.MX6 Quad (Device
Tree))
.handle_irq = imx6q_handle_irq,
.timer = imx6q_timer,
.init_machine = imx6q_init_machine,
+ .init_late = imx6q_init_late,
.dt_compat = imx6q_dt_compat,
.restart = imx6q_restart,
MACHINE_END
--
1.7.10
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.
Shawn,
On Tue, May 1, 2012 at 10:13 PM, Shawn Guo shawn@linaro.org wrote:
On Tue, May 01, 2012 at 09:12:38PM -0500, Robert Lee wrote:
Add common cpuidle init functionality that can be used by various
imx platforms.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/plat-mxc/Makefile | 1 +
arch/arm/plat-mxc/cpuidle.c | 80
++
arch/arm/plat-mxc/include/mach/cpuidle.h | 22
3 files changed, 103 insertions(+)
create mode 100644 arch/arm/plat-mxc/cpuidle.c
create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h
diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
index e81290c..63b064b 100644
--- a/arch/arm/plat-mxc/Makefile
+++ b/arch/arm/plat-mxc/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_MXC_ULPI) += ulpi.o
obj-$(CONFIG_MXC_USE_EPIT) += epit.o
obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o
obj-$(CONFIG_CPU_FREQ_IMX) += cpufreq.o
+obj-$(CONFIG_CPU_IDLE) += cpuidle.o
ifdef CONFIG_SND_IMX_SOC
obj-y += ssi-fiq.o
obj-y += ssi-fiq-ksym.o
diff --git a/arch/arm/plat-mxc/cpuidle.c b/arch/arm/plat-mxc/cpuidle.c
new file mode 100644
index 000..b7a5e1c
--- /dev/null
+++ b/arch/arm/plat-mxc/cpuidle.c
@@ -0,0 +1,80 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include linux/kernel.h
+#include linux/io.h
+#include linux/cpuidle.h
+#include linux/hrtimer.h
+#include linux/err.h
+#include linux/slab.h
+
+static struct cpuidle_device __percpu * imx_cpuidle_devices;
+
+void imx_cpuidle_devices_uninit(void)
+{
+ int cpu_id;
+ struct cpuidle_device *dev;
+
+ for_each_possible_cpu(cpu_id) {
+ dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
+ cpuidle_unregister_device(dev);
+ }
+
+ free_percpu(imx_cpuidle_devices);
+}
Does this function need to be exported? I haven't seen it being
used anywhere outside this file. Also, can it be __init?
Yes to both for now and can be changed back if necessary in the future.
+
+int __init imx_cpuidle_init(struct cpuidle_driver *drv)
+{
+ struct cpuidle_device *dev;
+ int cpu_id, ret;
+
+ if (!drv || drv-state_count CPUIDLE_STATE_MAX) {
+ pr_err(%s: Invalid Input\n, __func__);
+ return -EINVAL;
+ }
+
+ ret = cpuidle_register_driver(drv);
+ if (ret) {
+ pr_err(%s: Failed to register cpuidle driver with error:
%d\n,
+ __func__, ret);
+ return ret;
+ }
+
+ imx_cpuidle_devices = alloc_percpu(struct cpuidle_device);
+ if (imx_cpuidle_devices == NULL) {
+ ret = -ENOMEM;
+ goto unregister_drv;
+ }
+
+ /* initialize state data for each cpuidle_device */
+ for_each_possible_cpu(cpu_id) {
+ dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
+ dev-cpu = cpu_id;
+ dev-state_count = drv-state_count;
+
+ ret = cpuidle_register_device(dev);
+ if (ret) {
+ pr_err(%s: Failed to register cpu %u\n,
+ __func__, cpu_id);
Nit: print ret (error code) too?
I added the printing of the error code based on Sascha's suggestion in
v1 of this submission.
+ goto uninit;
+ }
+ }
+
+ return 0;
+
+uninit:
+ imx_cpuidle_devices_uninit();
+
+unregister_drv:
+ cpuidle_unregister_driver(drv);
+ return ret;
+}
diff --git a/arch/arm/plat-mxc/include/mach/cpuidle.h
b/arch/arm/plat-mxc/include/mach/cpuidle.h
new file mode 100644
index 000..8612510
--- /dev/null
+++ b/arch/arm/plat-mxc/include/mach/cpuidle.h
@@ -0,0 +1,22 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include linux/cpuidle.h
+
+#ifdef CONFIG_CPU_IDLE
+extern void imx_cpuidle_devices_uninit(void);
+extern int imx_cpuidle_init(struct cpuidle_driver *drv);
+#else
+static inline void imx_cpuidle_devices_uninit(void) {}
+static inline int imx_cpuidle_init(struct cpuidle_driver *drv)
+{ return -ENODEV; }
Nit: if it can not be in the same line with function name, we usually
have it be:
{
return -ENODEV;
}
Understood. I was just going by what I have seen used in other places
Re: [PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.
On Wed, May 2, 2012 at 9:07 AM, Shawn Guo shawn@linaro.org wrote:
On 2 May 2012 21:59, Rob Lee rob@linaro.org wrote:
+ ret = cpuidle_register_device(dev);
+ if (ret) {
+ pr_err(%s: Failed to register cpu %u\n,
+ __func__, cpu_id);
Nit: print ret (error code) too?
I added the printing of the error code based on Sascha's suggestion in
v1 of this submission.
But you did not print ret in above pr_err.
Argh, sorry, still trying to wake up I guess. Thanks.
Regards,
Shawn
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.
Sascha,
On Wed, May 2, 2012 at 2:27 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Tue, May 01, 2012 at 09:12:38PM -0500, Robert Lee wrote:
Add common cpuidle init functionality that can be used by various
imx platforms.
Signed-off-by: Robert Lee rob@linaro.org
---
+
+int __init imx_cpuidle_init(struct cpuidle_driver *drv)
+{
+ struct cpuidle_device *dev;
+ int cpu_id, ret;
+
+ if (!drv || drv-state_count CPUIDLE_STATE_MAX) {
Please don't check for !drv here. When someone calls this function with
a NULL pointer he should get a nive stack trace allowing him to figure
out what went wrong.
Ok, I will change this in v3. Given your statement, my understanding
is that I should avoid adding checks to make sure a valid driver
object was given as the stack trace information is all the handling
that is needed. If there is any further logic needed in that rule,
could you elaborate so that I don't make this mistake in the future,
or so that I don't add a check on a driver object in a case that I
should?
+ pr_err(%s: Invalid Input\n, __func__);
+ return -EINVAL;
+ }
+
+ ret = cpuidle_register_driver(drv);
+ if (ret) {
+ pr_err(%s: Failed to register cpuidle driver with error:
%d\n,
+ __func__, ret);
+ return ret;
+ }
+
+ imx_cpuidle_devices = alloc_percpu(struct cpuidle_device);
+ if (imx_cpuidle_devices == NULL) {
+ ret = -ENOMEM;
+ goto unregister_drv;
+ }
+
+ /* initialize state data for each cpuidle_device */
+ for_each_possible_cpu(cpu_id) {
+ dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
+ dev-cpu = cpu_id;
+ dev-state_count = drv-state_count;
+
+ ret = cpuidle_register_device(dev);
+ if (ret) {
+ pr_err(%s: Failed to register cpu %u\n,
+ __func__, cpu_id);
+ goto uninit;
You should only unregister the cpuidle devices you successfully
registered. Unregistering not yet registered cpuidle devices probably
has unwanted side effects.
I did not add in this handling because the cpuidle_unregister_device()
call already has a registered check so extra handling seemed
unnecessary. But if you still think it is needed just let me know.
cpuidle_unregister_device()
{
...
if (dev-registered == 0)
return;
...
Thanks,
Rob
Sascha
+ }
+ }
+
+ return 0;
+
+uninit:
+ imx_cpuidle_devices_uninit();
+
+unregister_drv:
+ cpuidle_unregister_driver(drv);
+ return ret;
+}
diff --git a/arch/arm/plat-mxc/include/mach/cpuidle.h
b/arch/arm/plat-mxc/include/mach/cpuidle.h
new file mode 100644
index 000..8612510
--- /dev/null
+++ b/arch/arm/plat-mxc/include/mach/cpuidle.h
@@ -0,0 +1,22 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include linux/cpuidle.h
+
+#ifdef CONFIG_CPU_IDLE
+extern void imx_cpuidle_devices_uninit(void);
+extern int imx_cpuidle_init(struct cpuidle_driver *drv);
+#else
+static inline void imx_cpuidle_devices_uninit(void) {}
+static inline int imx_cpuidle_init(struct cpuidle_driver *drv)
+{ return -ENODEV; }
+#endif
--
1.7.10
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 2/3] ARM: imx: Add imx5 cpuidle driver
Sascha,
On Wed, May 2, 2012 at 2:33 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Tue, May 01, 2012 at 09:12:39PM -0500, Robert Lee wrote:
Add imx5 cpuidle driver.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/mach-imx/mm-imx5.c | 42
+++---
1 file changed, 39 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
index d6b7e9f..cbd9bad 100644
--- a/arch/arm/mach-imx/mm-imx5.c
+++ b/arch/arm/mach-imx/mm-imx5.c
@@ -22,24 +22,59 @@
#include mach/common.h
#include mach/devices-common.h
#include mach/iomux-v3.h
+#include mach/cpuidle.h
static struct clk *gpc_dvfs_clk;
-static void imx5_idle(void)
+static int imx5_idle(void)
{
+ int ret = 0;
+
/* gpc clock is needed for SRPG */
if (gpc_dvfs_clk == NULL) {
gpc_dvfs_clk = clk_get(NULL, gpc_dvfs);
if (IS_ERR(gpc_dvfs_clk))
- return;
+ return -ENODEV;
}
clk_enable(gpc_dvfs_clk);
mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF);
if (!tzic_enable_wake())
cpu_do_idle();
+ else
+ ret = -EBUSY;
clk_disable(gpc_dvfs_clk);
+
+ return ret;
+}
+
+static int imx5_cpuidle_enter(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int idx)
+{
+ int ret;
+
+ ret = imx5_idle();
+
+ if (ret 0)
+ return ret;
+
+ return idx;
}
+static struct cpuidle_driver imx5_cpuidle_driver = {
+ .name = imx5_cpuidle,
+ .owner = THIS_MODULE,
+ .en_core_tk_irqen = 1,
+ .states[0] = {
+ .enter = imx5_cpuidle_enter,
+ .exit_latency = 20, /* max latency at 160MHz */
+ .target_residency = 1,
+ .flags = CPUIDLE_FLAG_TIME_VALID,
+ .name = IMX5 SRPG,
+ .desc = CPU state retained,powered off,
+ },
+ .state_count = 1,
+};
+
/*
* Define the MX50 memory map.
*/
@@ -103,7 +138,7 @@ void __init imx51_init_early(void)
mxc_set_cpu_type(MXC_CPU_MX51);
mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR));
mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
- arm_pm_idle = imx5_idle;
+ arm_pm_idle = (void *)imx5_idle;
I don't like this. It will cover all warnings when the prototype of
arm_pm_idle changes in future. Better add a static void imx5_idle
which calls a static int imx5_do_idle, then you have an idle function
which returns an int.
What about using the following:
arm_pm_idle = (void (*)(void))imx5_idle;
This will give warnings if arm_pm_idle prototype changes.
Thanks,
Rob
}
void __init imx53_init_early(void)
@@ -238,4 +273,5 @@ void __init imx53_soc_init(void)
void __init imx51_init_late(void)
{
mx51_neon_fixup();
+ imx_cpuidle_init(imx5_cpuidle_driver);
}
--
1.7.10
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] ARM: exynos: Adapt to cpuidle core time keeping and irq enable
On Wed, Apr 25, 2012 at 9:44 AM, Daniel Lezcano
daniel.lezc...@linaro.org wrote:
On 04/25/2012 02:11 PM, Amit Daniel Kachhap wrote:
This patch enables core cpuidle timekeeping and irq enabling and
remove those redundant parts from the exynos cpuidle drivers
CC: Daniel Lezcanodaniel.lezc...@linaro.org
CC: Robert Leerob@linaro.org
Signed-off-by: Amit Danielamit.kach...@linaro.org
---
arch/arm/mach-exynos/cpuidle.c | 53
---
1 files changed, 6 insertions(+), 47 deletions(-)
diff --git a/arch/arm/mach-exynos/cpuidle.c
b/arch/arm/mach-exynos/cpuidle.c
index 33ab4e7..26dac28 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -20,6 +20,7 @@
#includeasm/smp_scu.h
#includeasm/suspend.h
#includeasm/unified.h
+#includeasm/cpuidle.h
#includemach/regs-pmu.h
#includemach/pmu.h
@@ -34,22 +35,12 @@
#define S5P_CHECK_AFTR 0xFCBA0D10
-static int exynos4_enter_idle(struct cpuidle_device *dev,
- struct cpuidle_driver *drv,
- int index);
static int exynos4_enter_lowpower(struct cpuidle_device *dev,
struct cpuidle_driver *drv,
int index);
static struct cpuidle_state exynos4_cpuidle_set[] __initdata = {
- [0] = {
- .enter = exynos4_enter_idle,
- .exit_latency = 1,
- .target_residency = 10,
- .flags = CPUIDLE_FLAG_TIME_VALID,
- .name = C0,
- .desc = ARM clock gating(WFI),
- },
+ [0] = ARM_CPUIDLE_WFI_STATE,
[1] = {
.enter = exynos4_enter_lowpower,
.exit_latency = 300,
@@ -63,8 +54,9 @@ static struct cpuidle_state exynos4_cpuidle_set[]
__initdata = {
static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
static struct cpuidle_driver exynos4_idle_driver = {
- .name = exynos4_idle,
- .owner = THIS_MODULE,
+ .name = exynos4_idle,
+ .owner = THIS_MODULE,
+ .en_core_tk_irqen = 1,
};
/* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
@@ -103,13 +95,8 @@ static int exynos4_enter_core0_aftr(struct
cpuidle_device *dev,
struct cpuidle_driver *drv,
int index)
{
- struct timeval before, after;
- int idle_time;
unsigned long tmp;
- local_irq_disable();
- do_gettimeofday(before);
-
exynos4_set_wakeupmask();
/* Set value of power down register for aftr mode */
@@ -150,34 +137,6 @@ static int exynos4_enter_core0_aftr(struct
cpuidle_device *dev,
/* Clear wakeup state register */
__raw_writel(0x0, S5P_WAKEUP_STAT);
- do_gettimeofday(after);
-
- local_irq_enable();
- idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
- (after.tv_usec - before.tv_usec);
-
- dev-last_residency = idle_time;
- return index;
-}
-
-static int exynos4_enter_idle(struct cpuidle_device *dev,
- struct cpuidle_driver *drv,
- int index)
-{
- struct timeval before, after;
- int idle_time;
-
- local_irq_disable();
- do_gettimeofday(before);
-
- cpu_do_idle();
-
- do_gettimeofday(after);
- local_irq_enable();
- idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
- (after.tv_usec - before.tv_usec);
-
- dev-last_residency = idle_time;
return index;
}
@@ -192,7 +151,7 @@ static int exynos4_enter_lowpower(struct
cpuidle_device *dev,
new_index = drv-safe_state_index;
if (new_index == 0)
- return exynos4_enter_idle(dev, drv, new_index);
+ return arm_cpuidle_simple_enter(dev, drv, new_index);
else
return exynos4_enter_core0_aftr(dev, drv, new_index);
}
Acked-by: Robert Lee rob@linaro.org
Acked-by: Daniel Lezcano daniel.lezc...@linaro.org
--
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
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/3] ARM: imx: Add common imx cpuidle init functionality.
On Tue, Apr 24, 2012 at 3:36 AM, Shawn Guo shawn@linaro.org wrote: On Tue, Apr 24, 2012 at 08:54:26AM +0100, Russell King - ARM Linux wrote: On Tue, Apr 24, 2012 at 09:38:43AM +0800, Shawn Guo wrote: On Mon, Apr 23, 2012 at 10:45:02AM -0500, Rob Lee wrote: Let me try last time. What about having a late_initcall hook in machine_desc? Also fine with me. Shall I add Shawn's patch to my imx cpuidle patchset or should the arch/arm/kernel/setup.c and arch.h changes be submitted separately? If separately, Shawn, did you want to submit this patch or should I? Strange. Russell is not in the Cc list. I remember I added Russell into Cc when I propose the idea. Added him again. I didn't see any message in this thread cc'd to me, but that's not to say I hadn't already read this patch. I don't have any comment against it, but I do wonder how often this hook would be used. I guess mach-* that use common cpuidle will likely need this hook. We do seem to have quite a number of late_initcall()s in arch/arm/mach-*, so it seems to be a good idea - provided someone's willing to convert all those users of late_initcall()s. Agreed. The late_initcall()s in arch/arm/mach-* will not scale for long time, since we are moving toward single build. Thanks for the attention on this. From what I've understood, I will send another submission that includes the imx cpuidle patchset and Shawn's device tree late initcall patchset and I'll provide explanation of the two separate patchsets in the cover sheet. -- Regards, Shawn ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/3] ARM: imx: Add common imx cpuidle init functionality.
Let me try last time. What about having a late_initcall hook in
machine_desc?
Also fine with me.
Shall I add Shawn's patch to my imx cpuidle patchset or should the
arch/arm/kernel/setup.c and arch.h changes be submitted separately?
If separately, Shawn, did you want to submit this patch or should I?
Thanks,
Rob
Regards,
Shawn
8---
diff --git a/arch/arm/include/asm/mach/arch.h
b/arch/arm/include/asm/mach/arch.h
index d7692ca..0b1c94b 100644
--- a/arch/arm/include/asm/mach/arch.h
+++ b/arch/arm/include/asm/mach/arch.h
@@ -43,6 +43,7 @@ struct machine_desc {
void (*init_irq)(void);
struct sys_timer *timer; /* system tick timer */
void (*init_machine)(void);
+ void (*init_late)(void);
#ifdef CONFIG_MULTI_IRQ_HANDLER
void (*handle_irq)(struct pt_regs *);
#endif
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index ebfac78..549f036 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -800,6 +800,14 @@ static int __init customize_machine(void)
}
arch_initcall(customize_machine);
+static int __init init_machine_late(void)
+{
+ if (machine_desc-init_late)
+ machine_desc-init_late();
+ return 0;
+}
+late_initcall(init_machine_late);
+
#ifdef CONFIG_KEXEC
static inline unsigned long long get_total_mem(void)
{
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index da6c1d9..0e3640f 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -142,6 +142,7 @@ DT_MACHINE_START(IMX6Q, Freescale i.MX6 Quad (Device
Tree))
.handle_irq = imx6q_handle_irq,
.timer = imx6q_timer,
.init_machine = imx6q_init_machine,
+ .init_late = imx6q_init_late,
.dt_compat = imx6q_dt_compat,
.restart = imx6q_restart,
MACHINE_END
___
linux-arm-kernel mailing list
linux-arm-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/3] ARM: imx: Add common imx cpuidle init functionality.
If I called imx_cpuidle_init directly from imx5 or imx6q init routines, it would be getting called before the coreinit_call of core cpuidle causing a failure. There were various other directions to take and all seemed less desirable than this one. One alternative would be to add a function to return the pointer to the cpuidle driver object based on the machine type. Functionality exists to identify imx5 as a machine type but not imx6q, so I couldn't use that machine based method without adding that extra code. Another alternative would be to add a general platform lateinit_call function to each platforms that support cpuidle. Just put the initcall into mm-imx5.c and check the cpu type. Then you also don't have to make imx5_idle global. That solution is currently available for imx5 but for imx6q it implies adding the cpu type support for imx6q. Are you ok with that? Sascha or Shawn, any further comments on my question? Thanks, Rob ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/3] ARM: imx: Add common imx cpuidle init functionality.
On Tue, Apr 17, 2012 at 2:43 AM, Sascha Hauer s.ha...@pengutronix.de wrote:
On Mon, Apr 16, 2012 at 06:50:12PM -0500, Robert Lee wrote:
Add common cpuidle init functionality that can be used by various
imx platforms.
Signed-off-by: Robert Lee rob@linaro.org
---
diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
index e81290c..7c9e05f 100644
--- a/arch/arm/plat-mxc/Makefile
+++ b/arch/arm/plat-mxc/Makefile
@@ -16,6 +16,8 @@ obj-$(CONFIG_MXC_ULPI) += ulpi.o
obj-$(CONFIG_MXC_USE_EPIT) += epit.o
obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o
obj-$(CONFIG_CPU_FREQ_IMX) += cpufreq.o
+obj-$(CONFIG_CPU_IDLE) += cpuidle.o
+
ifdef CONFIG_SND_IMX_SOC
obj-y += ssi-fiq.o
obj-y += ssi-fiq-ksym.o
diff --git a/arch/arm/plat-mxc/cpuidle.c b/arch/arm/plat-mxc/cpuidle.c
new file mode 100644
index 000..d1c9301
--- /dev/null
+++ b/arch/arm/plat-mxc/cpuidle.c
@@ -0,0 +1,89 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include linux/kernel.h
+#include linux/io.h
+#include linux/cpuidle.h
+#include linux/hrtimer.h
+#include linux/err.h
+#include linux/slab.h
+
+static struct cpuidle_device __percpu * imx_cpuidle_devices;
+static struct cpuidle_driver *drv;
+
+void __init imx_cpuidle_set_driver(struct cpuidle_driver *p)
+{
+ drv = p;
+}
You like it complicated, eh? Why do you introduce a function which sets
a variable...
This complication is used to deal with the timing of various levels of
init calls. More explanation below.
+
+void imx_cpuidle_devices_uninit(void)
+{
+ int cpu_id;
+ struct cpuidle_device *dev;
+
+ for_each_possible_cpu(cpu_id) {
+ dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
+ cpuidle_unregister_device(dev);
+ }
+
+ free_percpu(imx_cpuidle_devices);
+}
+
+static int __init imx_cpuidle_init(void)
... instead of passing it directly to the function which uses it?
If I called imx_cpuidle_init directly from imx5 or imx6q init
routines, it would be getting called before the coreinit_call of core
cpuidle causing a failure. There were various other directions to
take and all seemed less desirable than this one.
One alternative would be to add a function to return the pointer to
the cpuidle driver object based on the machine type. Functionality
exists to identify imx5 as a machine type but not imx6q, so I couldn't
use that machine based method without adding that extra code.
Another alternative would be to add a general platform lateinit_call
function to each platforms that support cpuidle.
+{
+ struct cpuidle_device *dev;
+ int cpu_id, ret;
+
+ if (!drv || drv-state_count CPUIDLE_STATE_MAX) {
+ pr_err(%s: Invalid Input\n, __func__);
You introduce a pointless error message on SoCs not setting a cpuidle
driver. When will people learn that initcalls do not only run on
machines they have on their desk?
Will fix in next version.
+ return -EINVAL;
+ }
+
+ ret = cpuidle_register_driver(drv);
+
+ if (ret) {
+ pr_err(%s: Failed to register cpuidle driver\n, __func__);
It's always nice to add the return value to the error message to get a
clue *what* went wrong. The function name though is rather
uninteresting.
Will add the return value in the next version.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] ARM: imx: Fix imx5 idle logic bug
On Tue, Apr 17, 2012 at 3:10 AM, Sascha Hauer s.ha...@pengutronix.de wrote: On Mon, Apr 16, 2012 at 06:37:48PM -0500, Robert Lee wrote: The imx5_idle() check of the tzic_eanble_wake() return value uses incorrect (inverted) logic causing all attempt to idle to fail. Does this have influence on current kernels or does this only trigger with your cpuidle patches? This influences non-cpuidle kernels also as imx5_idle is what arm_pm_idle points to for imx51. Sascha Signed-off-by: Robert Lee rob@linaro.org --- arch/arm/mach-imx/mm-imx5.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c index 05250ae..e10f391 100644 --- a/arch/arm/mach-imx/mm-imx5.c +++ b/arch/arm/mach-imx/mm-imx5.c @@ -35,7 +35,7 @@ static void imx5_idle(void) } clk_enable(gpc_dvfs_clk); mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF); - if (tzic_enable_wake() != 0) + if (!tzic_enable_wake()) cpu_do_idle(); clk_disable(gpc_dvfs_clk); } -- 1.7.10 ___ linux-arm-kernel mailing list linux-arm-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/3] ARM: imx: Add common imx cpuidle init functionality.
On Tue, Apr 17, 2012 at 9:13 AM, Christian Robottom Reis
k...@linaro.org wrote:
On Tue, Apr 17, 2012 at 08:54:03AM -0500, Rob Lee wrote:
+void __init imx_cpuidle_set_driver(struct cpuidle_driver *p)
+{
+ drv = p;
+}
You like it complicated, eh? Why do you introduce a function which sets
a variable...
This complication is used to deal with the timing of various levels of
init calls. More explanation below.
Regardless of how you end up solving this, it's probably a good idea
to document the rationale, perhaps cribbing from what you describe
below..
Agree. Adding comments to the function itself seems to be the most
relevant location so I can add that information there if the function
remains.
If I called imx_cpuidle_init directly from imx5 or imx6q init
routines, it would be getting called before the coreinit_call of core
cpuidle causing a failure. There were various other directions to
take and all seemed less desirable than this one.
One alternative would be to add a function to return the pointer to
the cpuidle driver object based on the machine type. Functionality
exists to identify imx5 as a machine type but not imx6q, so I couldn't
use that machine based method without adding that extra code.
Another alternative would be to add a general platform lateinit_call
function to each platforms that support cpuidle.
..in a comment; without it, the code indeed looks bizarre.
--
Christian Robottom Reis, Engineering VP
Brazil (GMT-3) | [+55] 16 9112 6430 | [+1] 612 216 4935
Linaro.org: Open Source Software for ARM SoCs
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/3] ARM: imx: Add common imx cpuidle init functionality.
+void __init imx_cpuidle_set_driver(struct cpuidle_driver *p)
+{
+ drv = p;
+}
You like it complicated, eh? Why do you introduce a function which sets
a variable...
This complication is used to deal with the timing of various levels of
init calls. More explanation below.
+
+void imx_cpuidle_devices_uninit(void)
+{
+ int cpu_id;
+ struct cpuidle_device *dev;
+
+ for_each_possible_cpu(cpu_id) {
+ dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
+ cpuidle_unregister_device(dev);
+ }
+
+ free_percpu(imx_cpuidle_devices);
+}
+
+static int __init imx_cpuidle_init(void)
... instead of passing it directly to the function which uses it?
If I called imx_cpuidle_init directly from imx5 or imx6q init
routines, it would be getting called before the coreinit_call of core
cpuidle causing a failure. There were various other directions to
take and all seemed less desirable than this one.
One alternative would be to add a function to return the pointer to
the cpuidle driver object based on the machine type. Functionality
exists to identify imx5 as a machine type but not imx6q, so I couldn't
use that machine based method without adding that extra code.
Another alternative would be to add a general platform lateinit_call
function to each platforms that support cpuidle.
Just put the initcall into mm-imx5.c and check the cpu type. Then you
also don't have to make imx5_idle global.
That solution is currently available for imx5 but for imx6q it implies
adding the cpu type support for imx6q. Are you ok with that?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] ACPI-Thermal: Make Thermal trip points writeable
On Thu, Apr 5, 2012 at 12:52 AM, Amit Daniel Kachhap
amit.kach...@linaro.org wrote:
Hi Durgadoss,
Instead of making all the trip-points of a thermal zone writeable we should
only allow non-critical trip points to be writeable.
Thanks,
Amit Daniel
Agree, or even better, could the writeable attribute be made an
optional for any trip point?
Many SoCs now have built in thermal sensing and for cost reasons some
device makers may want to rely on the kernel thermal handling
capabilities as their primary thermal protection. Allowing userspace
to modify the trip points may increase their risk/exposure to lawsuits
or other undesirable results in some cases.
Best Regards,
Rob
-Original Message-
From: Durgadoss R durgados...@intel.com
Sent: Friday, January 27, 2012 9:58 PM
To: linux-a...@vger.kernel.org
Cc: l...@kernel.org; rui.zh...@intel.com; durgados...@intel.com
Subject: [PATCH] ACPI-Thermal: Make Thermal trip points writeable
Some of the thermal drivers using the Generic Thermal Framework
require (all/some) trip points to be writeable. This patch makes
the trip point temperatures writeable on a per-trip point basis,
and modifies the required function call in thermal.c. This patch
also updates the Documentation to reflect the new change.
Signed-off-by: Durgadoss R durgados...@intel.com
---
v1
* patch1/2: Code for making trip points writeable
* patch2/2: Change the callee in thermal.c
v2
* Make both the changes in a single patch
* Update Documentation/thermal/sysfs-api.txt
---
Documentation/thermal/sysfs-api.txt | 4 +-
drivers/acpi/thermal.c | 4 +-
drivers/thermal/thermal_sys.c | 127
++-
include/linux/thermal.h | 10 ++-
4 files changed, 91 insertions(+), 54 deletions(-)
diff --git a/Documentation/thermal/sysfs-api.txt
b/Documentation/thermal/sysfs-api.txt
index b61e46f..cd1476c 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -32,7 +32,8 @@ temperature) and throttle appropriate devices.
1.1 thermal zone device interface
1.1.1 struct thermal_zone_device *thermal_zone_device_register(char *name,
- int trips, void *devdata, struct thermal_zone_device_ops *ops)
+ int trips, int flag, void *devdata,
+ struct thermal_zone_device_ops *ops)
This interface function adds a new thermal zone device (sensor) to
/sys/class/thermal folder as thermal_zone[0-*]. It tries to bind all the
@@ -40,6 +41,7 @@ temperature) and throttle appropriate devices.
name: the thermal zone name.
trips: the total number of trip points this thermal zone supports.
+ flag: Bit string: If 'n'th bit is set, then trip point 'n' is writeable.
devdata: device private data
ops: thermal zone device call-backs.
.bind: bind the thermal zone device with a thermal cooling device.
diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 48fbc64..1c3133e 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -845,7 +845,7 @@ static int acpi_thermal_register_thermal_zone(struct
acpi_thermal *tz)
if (tz-trips.passive.flags.valid)
tz-thermal_zone =
- thermal_zone_device_register(acpitz, trips, tz,
+ thermal_zone_device_register(acpitz, trips, 0, tz,
acpi_thermal_zone_ops,
tz-trips.passive.tc1,
tz-trips.passive.tc2,
@@ -853,7 +853,7 @@ static int acpi_thermal_register_thermal_zone(struct
acpi_thermal *tz)
tz-polling_frequency*100);
else
tz-thermal_zone =
- thermal_zone_device_register(acpitz, trips, tz,
+ thermal_zone_device_register(acpitz, trips, 0, tz,
acpi_thermal_zone_ops,
0, 0, 0,
tz-polling_frequency*100);
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index dd9a574..3a4341e 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -220,6 +220,28 @@ trip_point_temp_show(struct device *dev, struct
device_attribute *attr,
}
static ssize_t
+trip_point_temp_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct thermal_zone_device *tz = to_thermal_zone(dev);
+ int trip, ret;
+ unsigned long temperature;
+
+ if (!tz-ops-set_trip_temp)
+ return -EPERM;
+
+ if (!sscanf(attr-attr.name, trip_point_%d_temp, trip))
+ return -EINVAL;
+
+ if (kstrtoul(buf, 10, temperature))
+ return -EINVAL;
+
+ ret = tz-ops-set_trip_temp(tz, trip,
Re: [PATCH] ACPI-Thermal: Make Thermal trip points writeable
Sorry, I just read Durgadoss last comment. Please ignore mine.
On Fri, Apr 6, 2012 at 8:01 AM, Rob Lee rob@linaro.org wrote:
On Thu, Apr 5, 2012 at 12:52 AM, Amit Daniel Kachhap
amit.kach...@linaro.org wrote:
Hi Durgadoss,
Instead of making all the trip-points of a thermal zone writeable we should
only allow non-critical trip points to be writeable.
Thanks,
Amit Daniel
Agree, or even better, could the writeable attribute be made an
optional for any trip point?
Many SoCs now have built in thermal sensing and for cost reasons some
device makers may want to rely on the kernel thermal handling
capabilities as their primary thermal protection. Allowing userspace
to modify the trip points may increase their risk/exposure to lawsuits
or other undesirable results in some cases.
Best Regards,
Rob
-Original Message-
From: Durgadoss R durgados...@intel.com
Sent: Friday, January 27, 2012 9:58 PM
To: linux-a...@vger.kernel.org
Cc: l...@kernel.org; rui.zh...@intel.com; durgados...@intel.com
Subject: [PATCH] ACPI-Thermal: Make Thermal trip points writeable
Some of the thermal drivers using the Generic Thermal Framework
require (all/some) trip points to be writeable. This patch makes
the trip point temperatures writeable on a per-trip point basis,
and modifies the required function call in thermal.c. This patch
also updates the Documentation to reflect the new change.
Signed-off-by: Durgadoss R durgados...@intel.com
---
v1
* patch1/2: Code for making trip points writeable
* patch2/2: Change the callee in thermal.c
v2
* Make both the changes in a single patch
* Update Documentation/thermal/sysfs-api.txt
---
Documentation/thermal/sysfs-api.txt | 4 +-
drivers/acpi/thermal.c | 4 +-
drivers/thermal/thermal_sys.c | 127
++-
include/linux/thermal.h | 10 ++-
4 files changed, 91 insertions(+), 54 deletions(-)
diff --git a/Documentation/thermal/sysfs-api.txt
b/Documentation/thermal/sysfs-api.txt
index b61e46f..cd1476c 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -32,7 +32,8 @@ temperature) and throttle appropriate devices.
1.1 thermal zone device interface
1.1.1 struct thermal_zone_device *thermal_zone_device_register(char *name,
- int trips, void *devdata, struct thermal_zone_device_ops *ops)
+ int trips, int flag, void *devdata,
+ struct thermal_zone_device_ops *ops)
This interface function adds a new thermal zone device (sensor) to
/sys/class/thermal folder as thermal_zone[0-*]. It tries to bind all the
@@ -40,6 +41,7 @@ temperature) and throttle appropriate devices.
name: the thermal zone name.
trips: the total number of trip points this thermal zone supports.
+ flag: Bit string: If 'n'th bit is set, then trip point 'n' is writeable.
devdata: device private data
ops: thermal zone device call-backs.
.bind: bind the thermal zone device with a thermal cooling device.
diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 48fbc64..1c3133e 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -845,7 +845,7 @@ static int acpi_thermal_register_thermal_zone(struct
acpi_thermal *tz)
if (tz-trips.passive.flags.valid)
tz-thermal_zone =
- thermal_zone_device_register(acpitz, trips, tz,
+ thermal_zone_device_register(acpitz, trips, 0, tz,
acpi_thermal_zone_ops,
tz-trips.passive.tc1,
tz-trips.passive.tc2,
@@ -853,7 +853,7 @@ static int acpi_thermal_register_thermal_zone(struct
acpi_thermal *tz)
tz-polling_frequency*100);
else
tz-thermal_zone =
- thermal_zone_device_register(acpitz, trips, tz,
+ thermal_zone_device_register(acpitz, trips, 0, tz,
acpi_thermal_zone_ops,
0, 0, 0,
tz-polling_frequency*100);
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index dd9a574..3a4341e 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -220,6 +220,28 @@ trip_point_temp_show(struct device *dev, struct
device_attribute *attr,
}
static ssize_t
+trip_point_temp_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct thermal_zone_device *tz = to_thermal_zone(dev);
+ int trip, ret;
+ unsigned long temperature;
+
+ if (!tz-ops-set_trip_temp)
+ return -EPERM;
+
+ if (!sscanf(attr-attr.name, trip_point_%d_temp, trip
Re: [PATCH v8 0/8] Consolidate cpuidle functionality
On Tue, Mar 20, 2012 at 7:10 PM, Kevin Hilman khil...@ti.com wrote: Hi Rob, Robert Lee rob@linaro.org writes: This patch series moves various functionality duplicated in platform cpuidle drivers to the core cpuidle driver. Also, the platform irq disabling was removed as it appears that all calls into cpuidle_call_idle will have already called local_irq_disable(). These changes have been pulled into linux-next. Len, Andrew, can a request be made for Linus to pull these changes? Acked-by: Jean Pihetj-pi...@ti.com (v6) Tested-by: Jean Pihetj-pi...@ti.com (v6, omap3) Tested-by: Amit Danielamit.kach...@linaro.org (v6, Exynos4) Tested-by: Robert Leerob@linaro.org (imx51, imx6q) Note that there's a space missing between the name and email in these tags (and for Deepthi's below also.) That seems to exist in all the patches. Thanks. This is now fixed in my pull tree. Reviewed-by: Kevin Hilman khil...@ti.com For my Reviewed-by, it only applies to the core code and the OMAP changes. I haven't reviewed the other platform-specific drivers. I believe the same applies to Jean Pihet who works with me on OMAP. Reviewed-by: Daniel Lezcano daniel.lezc...@linaro.org Reviewed-by: Deepthi Dharwardeep...@linux.vnet.ibm.com (core cpuidle only) Looks like you never heard from anyone actively working on at91, shmobile, kirwood or davinci. I'm not sure we should merge those platform-specific changes without an ack from those platform maintainers. For 3.4, maybe we should just merge the core code and the platforms that have been reviewed/ack'd, and for 3.5, spent some time nagging the other platform maintainers to review and test. Kevin ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v8 4/8] ARM: davinci: Consolidate time keeping and irq enable
On Wed, Mar 21, 2012 at 9:28 AM, Rob Lee rob@linaro.org wrote:
Sekhar tested this patch on Davinci last night and found a problem. I
looked at the code again and found a mindless omission on my part (see
below). Fix is trivial. I've check all other platforms and confirmed
this problem does not exist for those. Will resend a v9 of the
patchset shortly.
Kevin just informed me that Len already has v8 patchset queued so I'll
just send this fix out as a separate patch.
On Tue, Mar 20, 2012 at 3:22 PM, Robert Lee rob@linaro.org wrote:
Enable core cpuidle timekeeping and irq enabling and remove that
handling from this code.
Signed-off-by: Robert Lee rob@linaro.org
Reviewed-by: Kevin Hilman khil...@ti.com
Reviewed-by: Daniel Lezcano daniel.lezc...@linaro.org
Acked-by: Jean Pihetj-pi...@ti.com
---
arch/arm/mach-davinci/cpuidle.c | 82
---
1 file changed, 33 insertions(+), 49 deletions(-)
diff --git a/arch/arm/mach-davinci/cpuidle.c
b/arch/arm/mach-davinci/cpuidle.c
index a30c7c5..93ae096 100644
--- a/arch/arm/mach-davinci/cpuidle.c
+++ b/arch/arm/mach-davinci/cpuidle.c
@@ -18,6 +18,7 @@
#include linux/io.h
#include linux/export.h
#include asm/proc-fns.h
+#include asm/cpuidle.h
#include mach/cpuidle.h
#include mach/ddr2.h
@@ -30,12 +31,42 @@ struct davinci_ops {
u32 flags;
};
+/* Actual code that puts the SoC in different idle states */
+static int davinci_enter_idle(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv,
+ int index)
+{
+ struct cpuidle_state_usage *state_usage = dev-states_usage[index];
+ struct davinci_ops *ops = cpuidle_get_statedata(state_usage);
+
+ if (ops ops-enter)
+ ops-enter(ops-flags);
+
+ index = cpuidle_wrap_enter(dev, drv, index,
+ arm_cpuidle_simple_enter);
+
+ if (ops ops-exit)
+ ops-exit(ops-flags);
+
+ return index;
+}
+
/* fields in davinci_ops.flags */
#define DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN BIT(0)
static struct cpuidle_driver davinci_idle_driver = {
- .name = cpuidle-davinci,
- .owner = THIS_MODULE,
+ .name = cpuidle-davinci,
+ .owner = THIS_MODULE,
Argh! I am missing the .en_core_tk_irqen = 1 here.
+ .states[0] = ARM_CPUIDLE_WFI_STATE,
+ .states[1] = {
+ .enter = davinci_enter_idle,
+ .exit_latency = 10,
+ .target_residency = 10,
+ .flags = CPUIDLE_FLAG_TIME_VALID,
+ .name = DDR SR,
+ .desc = WFI and DDR Self Refresh,
+ },
+ .state_count = DAVINCI_CPUIDLE_MAX_STATES,
};
static DEFINE_PER_CPU(struct cpuidle_device, davinci_cpuidle_device);
@@ -77,41 +108,10 @@ static struct davinci_ops
davinci_states[DAVINCI_CPUIDLE_MAX_STATES] = {
},
};
-/* Actual code that puts the SoC in different idle states */
-static int davinci_enter_idle(struct cpuidle_device *dev,
- struct cpuidle_driver *drv,
- int index)
-{
- struct cpuidle_state_usage *state_usage = dev-states_usage[index];
- struct davinci_ops *ops = cpuidle_get_statedata(state_usage);
- struct timeval before, after;
- int idle_time;
-
- local_irq_disable();
- do_gettimeofday(before);
-
- if (ops ops-enter)
- ops-enter(ops-flags);
- /* Wait for interrupt state */
- cpu_do_idle();
- if (ops ops-exit)
- ops-exit(ops-flags);
-
- do_gettimeofday(after);
- local_irq_enable();
- idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
- (after.tv_usec - before.tv_usec);
-
- dev-last_residency = idle_time;
-
- return index;
-}
-
static int __init davinci_cpuidle_probe(struct platform_device *pdev)
{
int ret;
struct cpuidle_device *device;
- struct cpuidle_driver *driver = davinci_idle_driver;
struct davinci_cpuidle_config *pdata = pdev-dev.platform_data;
device = per_cpu(davinci_cpuidle_device, smp_processor_id());
@@ -123,27 +123,11 @@ static int __init davinci_cpuidle_probe(struct
platform_device *pdev)
ddr2_reg_base = pdata-ddr2_ctlr_base;
- /* Wait for interrupt state */
- driver-states[0].enter = davinci_enter_idle;
- driver-states[0].exit_latency = 1;
- driver-states[0].target_residency = 1;
- driver-states[0].flags = CPUIDLE_FLAG_TIME_VALID;
- strcpy(driver-states[0].name, WFI);
- strcpy(driver-states[0].desc, Wait for interrupt);
-
- /* Wait for interrupt and DDR self
Re: [PATCH v8 4/8] ARM: davinci: Consolidate time keeping and irq enable
Sekhar tested this patch on Davinci last night and found a problem. I
looked at the code again and found a mindless omission on my part (see
below). Fix is trivial. I've check all other platforms and confirmed
this problem does not exist for those. Will resend a v9 of the
patchset shortly.
On Tue, Mar 20, 2012 at 3:22 PM, Robert Lee rob@linaro.org wrote:
Enable core cpuidle timekeeping and irq enabling and remove that
handling from this code.
Signed-off-by: Robert Lee rob@linaro.org
Reviewed-by: Kevin Hilman khil...@ti.com
Reviewed-by: Daniel Lezcano daniel.lezc...@linaro.org
Acked-by: Jean Pihetj-pi...@ti.com
---
arch/arm/mach-davinci/cpuidle.c | 82
---
1 file changed, 33 insertions(+), 49 deletions(-)
diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c
index a30c7c5..93ae096 100644
--- a/arch/arm/mach-davinci/cpuidle.c
+++ b/arch/arm/mach-davinci/cpuidle.c
@@ -18,6 +18,7 @@
#include linux/io.h
#include linux/export.h
#include asm/proc-fns.h
+#include asm/cpuidle.h
#include mach/cpuidle.h
#include mach/ddr2.h
@@ -30,12 +31,42 @@ struct davinci_ops {
u32 flags;
};
+/* Actual code that puts the SoC in different idle states */
+static int davinci_enter_idle(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv,
+ int index)
+{
+ struct cpuidle_state_usage *state_usage = dev-states_usage[index];
+ struct davinci_ops *ops = cpuidle_get_statedata(state_usage);
+
+ if (ops ops-enter)
+ ops-enter(ops-flags);
+
+ index = cpuidle_wrap_enter(dev, drv, index,
+ arm_cpuidle_simple_enter);
+
+ if (ops ops-exit)
+ ops-exit(ops-flags);
+
+ return index;
+}
+
/* fields in davinci_ops.flags */
#define DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN BIT(0)
static struct cpuidle_driver davinci_idle_driver = {
- .name = cpuidle-davinci,
- .owner = THIS_MODULE,
+ .name = cpuidle-davinci,
+ .owner = THIS_MODULE,
Argh! I am missing the .en_core_tk_irqen = 1 here.
+ .states[0] = ARM_CPUIDLE_WFI_STATE,
+ .states[1] = {
+ .enter = davinci_enter_idle,
+ .exit_latency = 10,
+ .target_residency = 10,
+ .flags = CPUIDLE_FLAG_TIME_VALID,
+ .name = DDR SR,
+ .desc = WFI and DDR Self Refresh,
+ },
+ .state_count = DAVINCI_CPUIDLE_MAX_STATES,
};
static DEFINE_PER_CPU(struct cpuidle_device, davinci_cpuidle_device);
@@ -77,41 +108,10 @@ static struct davinci_ops
davinci_states[DAVINCI_CPUIDLE_MAX_STATES] = {
},
};
-/* Actual code that puts the SoC in different idle states */
-static int davinci_enter_idle(struct cpuidle_device *dev,
- struct cpuidle_driver *drv,
- int index)
-{
- struct cpuidle_state_usage *state_usage = dev-states_usage[index];
- struct davinci_ops *ops = cpuidle_get_statedata(state_usage);
- struct timeval before, after;
- int idle_time;
-
- local_irq_disable();
- do_gettimeofday(before);
-
- if (ops ops-enter)
- ops-enter(ops-flags);
- /* Wait for interrupt state */
- cpu_do_idle();
- if (ops ops-exit)
- ops-exit(ops-flags);
-
- do_gettimeofday(after);
- local_irq_enable();
- idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
- (after.tv_usec - before.tv_usec);
-
- dev-last_residency = idle_time;
-
- return index;
-}
-
static int __init davinci_cpuidle_probe(struct platform_device *pdev)
{
int ret;
struct cpuidle_device *device;
- struct cpuidle_driver *driver = davinci_idle_driver;
struct davinci_cpuidle_config *pdata = pdev-dev.platform_data;
device = per_cpu(davinci_cpuidle_device, smp_processor_id());
@@ -123,27 +123,11 @@ static int __init davinci_cpuidle_probe(struct
platform_device *pdev)
ddr2_reg_base = pdata-ddr2_ctlr_base;
- /* Wait for interrupt state */
- driver-states[0].enter = davinci_enter_idle;
- driver-states[0].exit_latency = 1;
- driver-states[0].target_residency = 1;
- driver-states[0].flags = CPUIDLE_FLAG_TIME_VALID;
- strcpy(driver-states[0].name, WFI);
- strcpy(driver-states[0].desc, Wait for interrupt);
-
- /* Wait for interrupt and DDR self refresh state */
- driver-states[1].enter = davinci_enter_idle;
- driver-states[1].exit_latency = 10;
- driver-states[1].target_residency = 1;
-
Re: [git pull] Consolidate cpuidle functionality
Len and Andrew, Please consider this an official merge request of this cpuidle patchset for v3.4. There were two small conflicts Stephen Rothwell found when merging to linux-next. The first conflict is with patch 1/9 in the drivers/cpuidle/cpuidle.c file which is trivially to resolve. I'm told this fixup is trivially enough to not require anyone to carry it. The second is with patch 2/9 in mach-at91/cpuidle.c due to some minor cleanup changes. The fix is also pretty simple but I'm waiting to hear back about who will carry it. Please let me know if you need any other action or information on my part. Thanks, Rob On Fri, Mar 9, 2012 at 12:40 AM, Stephen Rothwell s...@canb.auug.org.au wrote: Hi Rob, On Thu, 8 Mar 2012 19:58:23 -0600 Rob Lee rob@linaro.org wrote: git://git.linaro.org/people/rob_lee/linux.git cpuidle_consol_pull These changes move various functionality duplicated in platform cpuidle drivers to the core cpuidle driver and common arch arm code. Also, the irq disabling in the platform code was removed as all calls into cpuidle_call_idle() will have already called local_irq_disable(). This patchset is bisect safe. Also, the core cpuidle and arch changes of the first commit do not require any changes to the arch and platform cpuidle drivers, though those arch and platform change should be made to take advantage of the new consolidation function. Stephen, this patch has been reviewed, tested, and ACK'd per the list above but cpuidle maintainer Len Brown has been out on vacation for a couple of weeks so I am sending you this pull request as time is running out to get this into v3.4. I've had a brief communication with Andrew Morton about this as well so he is aware of this situation. I am fairly new to the community so please let me know if you see anything that needs my attention or anything I should be doing differently. I will add this tree from today. Lets see if anyone screams. Thanks for adding your subsystem tree as a participant of linux-next. As you may know, this is not a judgment of your code. The purpose of linux-next is for integration testing and to lower the impact of conflicts between subsystems in the next merge window. You will need to ensure that the patches/commits in your tree/series have been: * submitted under GPL v2 (or later) and include the Contributor's Signed-off-by, * posted to the relevant mailing list, * reviewed by you (or another maintainer of your subsystem tree), * successfully unit tested, and * destined for the current or next Linux merge window. Basically, this should be just what you would send to Linus (or ask him to fetch). It is allowed to be rebased if you deem it necessary. -- Cheers, Stephen Rothwell s...@canb.auug.org.au Legal Stuff: By participating in linux-next, your subsystem tree contributions are public and will be included in the linux-next trees. You may be sent e-mail messages indicating errors or other issues when the patches/commits from your subsystem tree are merged and tested in linux-next. These messages may also be cross-posted to the linux-next mailing list, the linux-kernel mailing list, etc. The linux-next tree project and IBM (my employer) make no warranties regarding the linux-next project, the testing procedures, the results, the e-mails, etc. If you don't agree to these ground rules, let me know and I'll remove your tree from participation in linux-next. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
[git pull] Consolidate cpuidle functionality
Hello Stephen, The following changes since commit 192cfd58774b4d17b2fe8bdc77d89c2ef4e0591d: Linux 3.3-rc6 (2012-03-03 17:08:09 -0800) are available in the git repository at: git://git.linaro.org/people/rob_lee/linux.git cpuidle_consol_pull Robert Lee (8): cpuidle: Add common time keeping and irq enabling ARM: at91: Consolidate time keeping and irq enable ARM: kirkwood: Consolidate time keeping and irq enable ARM: davinci: Consolidate time keeping and irq enable ARM: omap: Consolidate OMAP3 time keeping and irq enable ARM: omap: Consolidate OMAP4 time keeping and irq enable ARM: shmobile: Consolidate time keeping and irq enable SH: shmobile: Consolidate time keeping and irq enable arch/arm/include/asm/cpuidle.h| 22 + arch/arm/kernel/Makefile |2 +- arch/arm/kernel/cpuidle.c | 21 arch/arm/mach-at91/cpuidle.c | 67 ++- arch/arm/mach-davinci/cpuidle.c | 82 +--- arch/arm/mach-kirkwood/cpuidle.c | 72 arch/arm/mach-omap2/cpuidle34xx.c | 42 +++-- arch/arm/mach-omap2/cpuidle44xx.c | 21 +--- arch/arm/mach-shmobile/cpuidle.c | 31 +++-- arch/sh/kernel/cpu/shmobile/cpuidle.c | 10 +--- drivers/cpuidle/cpuidle.c | 79 +-- include/linux/cpuidle.h | 13 +- 12 files changed, 227 insertions(+), 235 deletions(-) create mode 100644 arch/arm/include/asm/cpuidle.h create mode 100644 arch/arm/kernel/cpuidle.c The top-commit id is bd7fd2ab3ee38df371ece6cd4997d6660b3e1c95 Acked-by: Jean Pihetj-pi...@ti.com (v6) Tested-by: Jean Pihetj-pi...@ti.com (v6, omap3) Tested-by: Amit Danielamit.kach...@linaro.org (v6, Exynos4) Tested-by: Robert Leerob@linaro.org (imx51, imx6q) Reviewed-by: Kevin Hilman khil...@ti.com Reviewed-by: Daniel Lezcano daniel.lezc...@linaro.org Reviewed-by: Deepthi Dharwardeep...@linux.vnet.ibm.com (core cpuidle only) These changes move various functionality duplicated in platform cpuidle drivers to the core cpuidle driver and common arch arm code. Also, the irq disabling in the platform code was removed as all calls into cpuidle_call_idle() will have already called local_irq_disable(). This patchset is bisect safe. Also, the core cpuidle and arch changes of the first commit do not require any changes to the arch and platform cpuidle drivers, though those arch and platform change should be made to take advantage of the new consolidation function. Stephen, this patch has been reviewed, tested, and ACK'd per the list above but cpuidle maintainer Len Brown has been out on vacation for a couple of weeks so I am sending you this pull request as time is running out to get this into v3.4. I've had a brief communication with Andrew Morton about this as well so he is aware of this situation. I am fairly new to the community so please let me know if you see anything that needs my attention or anything I should be doing differently. I've checked these changes against the recent linux-next tags and there is currently one small conflict due to a small change in arch/arm/mach-at91/cpuidle.c. I'd be happy to assist in resolving this issue if needed. Also, in my patchset submissions I include changes for a Samsung Exynos cpuidle platform driver patch that was accepted a couple of weeks ago but it hasn't yet made it to an rc so I am not including it in this request. --- patchset submission history: v7 submission can be found here: http://www.spinics.net/lists/arm-kernel/msg162290.html v7 changes: * Made some struct whitespace alignment changes. * Fixed a coding style violation (thanks Jean Pihet) * Fixed a bug in davinci cpuidle (thanks Jean Pihet) * Corrected the common ARM cpuidle WFI state description to be ARM platform agnostic (thanks Kevin Hilman) * Fixed the problem causing x86 and PPC builds to fail (thanks Deepthi) * Re-added a line of code that was mistakenly removed (thanks Deepthi) v6 submission can be found here: http://www.spinics.net/lists/arm-kernel/msg162018.html v6 changes: * Fixed mindless bug in CONFIG_ARCH_HAS_RELAX code in drivers/cpuidle/cpuidle.c * Removed inline from wrapper function (thanks Mike Turquette and Rob Herring) * Add zeroing out of last_residency if error value is returned (thanks Mike) * Made drivers/cpuidle/cpuidle.c more intelligently handle en_core_tk_irqen flag allowing removal of the if (en_core_tk_irqen) in cpuidle_idle_call (thanks Daniel Lezcano) * Moved CPUIDLE_ARM_WFI_STATE macro to arch/arm/include/asm/cpuidle.h (thanks Jean Pihet) * Cleaned up some comments and a stray change (thanks Jean) v5 submission can be found here: http://www.spinics.net/lists/arm-kernel/msg161596.html v5 changes: * Added common cpu_do_idle function to core cpuidle * Added time keep irq en wrapper to core cpuidle *
Re: [PATCH v7 0/9] Consolidate cpuidle functionality
On Wed, Feb 29, 2012 at 6:42 PM, Robert Lee rob@linaro.org wrote: This patch series moves various functionality duplicated in platform cpuidle drivers to the core cpuidle driver. Also, the platform irq disabling was removed as it appears that all calls into cpuidle_call_idle will have already called local_irq_disable(). I'm told that I forgot to add the Acks from the previous v6 to this version: Acked-by: Jean Pihet j-pi...@ti.com (v6) Tested-by: Jean Pihet j-pi...@ti.com (v6, omap3) Tested-by: Amit Daniel amit.kach...@linaro.org (v6, Exynos4) For the generic cpuidle changes: Reviewed-by: Deepthi Dharwar deep...@linux.vnet.ibm.com If anyone sees other omissions or has any suggested changes or improvements in my patch submissions semantics, please let me know. Thanks, Rob Rafael, Could you review this patchset and merge patch 1/9 once its ready? It seems pretty close to being acceptable. The get_maintainer script shows Len Brown as the cpuidle maintainer but I've been unable to get a response from him so far. If you are not the right person, could you suggest who I can make this request to? Thanks. Note to platform maintainers: Platform patches (2/9 to 9/9) in this patchset are not required to work with patch 1/9 but please review and push these platform changes as possible to allow this consolidation to occur. Based on 3.3-rc5 plus recent exynos cpuidle patch (affects exynos cpuidle only): http://www.spinics.net/lists/linux-samsung-soc/msg09467.html v6 submission tested successfully on Exynos (thanks Amit Kacchap) and OMAP3 (thanks Jean Pihet) platforms. v6 submission can be found here: http://www.spinics.net/lists/arm-kernel/msg162018.html Changes since v6: * Made some struct whitespace alignment changes. * Fixed a coding style violation (thanks Jean Pihet) * Fixed a bug in davinci cpuidle (thanks Jean Pihet) * Corrected the common ARM cpuidle WFI state description to be ARM platform agnostic (thanks Kevin Hilman) * Fixed the problem causing x86 and PPC builds to fail (thanks Deepthi) * Re-added a line of code that was mistakenly removed (thanks Deepthi) Robert Lee (9): cpuidle: Add common time keeping and irq enabling ARM: at91: Consolidate time keeping and irq enable ARM: exynos: Consolidate time keeping and irq enable ARM: kirkwood: Consolidate time keeping and irq enable ARM: davinci: Consolidate time keeping and irq enable ARM: omap: Consolidate OMAP3 time keeping and irq enable ARM: omap: Consolidate OMAP4 time keeping and irq enable ARM: shmobile: Consolidate time keeping and irq enable SH: shmobile: Consolidate time keeping and irq enable arch/arm/include/asm/cpuidle.h | 22 + arch/arm/kernel/Makefile | 2 +- arch/arm/kernel/cpuidle.c | 21 arch/arm/mach-at91/cpuidle.c | 67 ++- arch/arm/mach-davinci/cpuidle.c | 82 +--- arch/arm/mach-exynos/cpuidle.c | 53 ++--- arch/arm/mach-kirkwood/cpuidle.c | 72 arch/arm/mach-omap2/cpuidle34xx.c | 42 +++-- arch/arm/mach-omap2/cpuidle44xx.c | 21 +--- arch/arm/mach-shmobile/cpuidle.c | 31 +++-- arch/sh/kernel/cpu/shmobile/cpuidle.c | 10 +--- drivers/cpuidle/cpuidle.c | 79 +-- include/linux/cpuidle.h | 13 +- 13 files changed, 233 insertions(+), 282 deletions(-) create mode 100644 arch/arm/include/asm/cpuidle.h create mode 100644 arch/arm/kernel/cpuidle.c ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 5/9] ARM: davinci: Consolidate time keeping and irq enable
On Wed, Feb 29, 2012 at 2:36 AM, Jean Pihet jean.pi...@newoldbits.com wrote:
Rob,
On Wed, Feb 29, 2012 at 4:11 AM, Robert Lee rob@linaro.org wrote:
Enable core cpuidle timekeeping and irq enabling and remove that
handling from this code.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/mach-davinci/cpuidle.c | 78
+++---
1 files changed, 31 insertions(+), 47 deletions(-)
diff --git a/arch/arm/mach-davinci/cpuidle.c
b/arch/arm/mach-davinci/cpuidle.c
index a30c7c5..6f457f1 100644
--- a/arch/arm/mach-davinci/cpuidle.c
+++ b/arch/arm/mach-davinci/cpuidle.c
...
@@ -30,12 +31,42 @@ struct davinci_ops {
u32 flags;
};
+/* Actual code that puts the SoC in different idle states */
+static int davinci_enter_idle(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv,
+ int index)
+{
+ struct cpuidle_state_usage *state_usage = dev-states_usage[index];
+ struct davinci_ops *ops = cpuidle_get_statedata(state_usage);
+
+ if (ops ops-enter)
+ ops-enter(ops-flags);
+
+ return cpuidle_wrap_enter(dev, drv, index,
+ cpuidle_simple_enter);
This does not look right since ops-exit will never be called.
Yes, thanks. The 'return' should be 'index ='
+
+ if (ops ops-exit)
+ ops-exit(ops-flags);
+
+ return index;
+}
+
...
Regards,
Jean
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 1/9] cpuidle: Add common time keeping and irq enabling
Hello Deepthi,
On Wed, Feb 29, 2012 at 5:13 AM, Deepthi Dharwar
deep...@linux.vnet.ibm.com wrote:
Hi Rob,
On 02/29/2012 08:41 AM, Robert Lee wrote:
Make necessary changes to implement time keeping and irq enabling
in the core cpuidle code. This will allow the removal of these
functionalities from various platform cpuidle implementations whose
timekeeping and irq enabling follows the form in this common code.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/include/asm/cpuidle.h | 14 ++
drivers/cpuidle/cpuidle.c | 90
include/linux/cpuidle.h | 13 ++
3 files changed, 99 insertions(+), 18 deletions(-)
create mode 100644 arch/arm/include/asm/cpuidle.h
diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
new file mode 100644
index 000..1d2075b
--- /dev/null
+++ b/arch/arm/include/asm/cpuidle.h
@@ -0,0 +1,14 @@
+#ifndef __ASM_ARM_CPUIDLE_H
+#define __ASM_ARM_CPUIDLE_H
+
+/* Common ARM WFI state */
+#define CPUIDLE_ARM_WFI_STATE {\
+ .enter = cpuidle_simple_enter,\
+ .exit_latency = 1,\
+ .target_residency = 1,\
+ .flags = CPUIDLE_FLAG_TIME_VALID,\
+ .name = WFI,\
+ .desc = ARM core clock gating (WFI),\
+}
+
+#endif
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 59f4261..00b02f5 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -18,6 +18,7 @@
#include linux/ktime.h
#include linux/hrtimer.h
#include linux/module.h
+#include asm/proc-fns.h
#include trace/events/power.h
#include cpuidle.h
@@ -53,6 +54,24 @@ static void cpuidle_kick_cpus(void) {}
static int __cpuidle_register_device(struct cpuidle_device *dev);
+static inline int cpuidle_enter(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int index)
+{
+ struct cpuidle_state *target_state = drv-states[index];
+ return target_state-enter(dev, drv, index);
+}
+
+static inline int cpuidle_enter_tk(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int index)
+{
+ return cpuidle_wrap_enter(dev, drv, index, cpuidle_enter);
+}
+
+typedef int (*cpuidle_enter_t)(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int index);
+
+static cpuidle_enter_t cpuidle_enter_ops;
+
/**
* cpuidle_idle_call - the main idle loop
*
@@ -63,7 +82,6 @@ int cpuidle_idle_call(void)
{
struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
struct cpuidle_driver *drv = cpuidle_get_driver();
- struct cpuidle_state *target_state;
int next_state, entered_state;
if (off)
@@ -92,12 +110,10 @@ int cpuidle_idle_call(void)
return 0;
}
- target_state = drv-states[next_state];
-
trace_power_start(POWER_CSTATE, next_state, dev-cpu);
trace_cpu_idle(next_state, dev-cpu);
- entered_state = target_state-enter(dev, drv, next_state);
+ entered_state = cpuidle_enter_ops(dev, drv, next_state);
trace_power_end(dev-cpu);
trace_cpu_idle(PWR_EVENT_EXIT, dev-cpu);
@@ -110,7 +126,8 @@ int cpuidle_idle_call(void)
dev-states_usage[entered_state].time +=
(unsigned long long)dev-last_residency;
dev-states_usage[entered_state].usage++;
- }
+ } else
+ dev-last_residency = 0;
/* give the governor an opportunity to reflect on the outcome */
if (cpuidle_curr_governor-reflect)
@@ -164,20 +181,29 @@ void cpuidle_resume_and_unlock(void)
EXPORT_SYMBOL_GPL(cpuidle_resume_and_unlock);
-#ifdef CONFIG_ARCH_HAS_CPU_RELAX
-static int poll_idle(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int index)
+/**
+ * cpuidle_wrap_enter - performs timekeeping and irqen around enter function
+ * @dev: pointer to a valid cpuidle_device object
+ * @drv: pointer to a valid cpuidle_driver object
+ * @index: index of the target cpuidle state.
+ */
+int cpuidle_wrap_enter(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int index,
+ int (*enter)(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int index))
{
- ktime_t t1, t2;
+ ktime_t time_start, time_end;
s64 diff;
- t1 = ktime_get();
+ time_start = ktime_get();
+
+ index = enter(dev, drv, index);
+
+ time_end = ktime_get();
+
local_irq_enable();
- while (!need_resched())
- cpu_relax();
- t2 = ktime_get();
- diff = ktime_to_us(ktime_sub(t2, t1));
+ diff = ktime_to_us(ktime_sub(time_end, time_start));
if (diff INT_MAX)
diff = INT_MAX;
@@ -186,6 +212,31 @@ static int
Re: [PATCH v6 1/9] cpuidle: Add common time keeping and irq enabling
On Wed, Feb 29, 2012 at 2:30 AM, Jean Pihet jean.pi...@newoldbits.com wrote: Hi Rob, On Wed, Feb 29, 2012 at 4:11 AM, Robert Lee rob@linaro.org wrote: Make necessary changes to implement time keeping and irq enabling in the core cpuidle code. This will allow the removal of these functionalities from various platform cpuidle implementations whose timekeeping and irq enabling follows the form in this common code. Signed-off-by: Robert Lee rob@linaro.org --- arch/arm/include/asm/cpuidle.h | 14 ++ drivers/cpuidle/cpuidle.c | 90 include/linux/cpuidle.h | 13 ++ 3 files changed, 99 insertions(+), 18 deletions(-) create mode 100644 arch/arm/include/asm/cpuidle.h ... diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 59f4261..00b02f5 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c ... @@ -110,7 +126,8 @@ int cpuidle_idle_call(void) dev-states_usage[entered_state].time += (unsigned long long)dev-last_residency; dev-states_usage[entered_state].usage++; - } + } else + dev-last_residency = 0; Braces are required here, according to the coding style doc. Thanks. ... Regards, Jean ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 1/9] cpuidle: Add common time keeping and irq enabling
On Wed, Feb 29, 2012 at 12:43 PM, Kevin Hilman khil...@ti.com wrote:
Robert Lee rob@linaro.org writes:
Make necessary changes to implement time keeping and irq enabling
in the core cpuidle code. This will allow the removal of these
functionalities from various platform cpuidle implementations whose
timekeeping and irq enabling follows the form in this common code.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/include/asm/cpuidle.h | 14 ++
drivers/cpuidle/cpuidle.c | 90
include/linux/cpuidle.h | 13 ++
3 files changed, 99 insertions(+), 18 deletions(-)
create mode 100644 arch/arm/include/asm/cpuidle.h
diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
new file mode 100644
index 000..1d2075b
--- /dev/null
+++ b/arch/arm/include/asm/cpuidle.h
@@ -0,0 +1,14 @@
+#ifndef __ASM_ARM_CPUIDLE_H
+#define __ASM_ARM_CPUIDLE_H
+
+/* Common ARM WFI state */
+#define CPUIDLE_ARM_WFI_STATE {\
+ .enter = cpuidle_simple_enter,\
+ .exit_latency = 1,\
+ .target_residency = 1,\
+ .flags = CPUIDLE_FLAG_TIME_VALID,\
+ .name = WFI,\
+ .desc = ARM core clock gating (WFI),\
+}
nit: just name this ARM WFI. Clock gating is platform specific, and
core has platform-specific meanings, so in order to keep this truly
generic, I think hat ARM WFI is the best name.
Agree. I'll make that change.
Kevin
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
Hey Mike,
On Mon, Feb 27, 2012 at 6:06 PM, Turquette, Mike mturque...@ti.com wrote:
On Sun, Feb 26, 2012 at 8:47 PM, Robert Lee rob@linaro.org wrote:
+/**
+ * cpuidle_enter_wrap - performing timekeeping and irq around enter function
+ * @dev: pointer to a valid cpuidle_device object
+ * @drv: pointer to a valid cpuidle_driver object
+ * @index: index of the target cpuidle state.
+ */
+static inline int cpuidle_wrap_enter(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int index,
+ int (*enter)(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int
index))
+{
+ ktime_t time_start, time_end;
+ s64 diff;
+
+ time_start = ktime_get();
+
+ index = enter(dev, drv, index);
+
+ time_end = ktime_get();
+
+ local_irq_enable();
+
+ diff = ktime_to_us(ktime_sub(time_end, time_start));
+ if (diff INT_MAX)
+ diff = INT_MAX;
+
+ dev-last_residency = (int) diff;
+
+ return index;
+}
Any reason that this code is in the header? Why not in cpuidle.c?
Not a strong reason. I thought making it an inline would introduce
slightly less new execution when adding this code (realizing that
there are function calls immediately after, so the only benefit is the
reduce popping and pushing). But it does require an extra copy of
this code for any platform driver that does not enable
en_core_tk_irqen and instead makes calls to it directly (like omap3).
For this case, I don't think the inline implementation should add
extra code from what exists today as it should simply replace the
existing platform time keeping calls to a standard one defined by the
core cpuidle.
I don't have a strong preference with using the inline so if you or
others can give your opinion on which method to use and why, I'd be
glad to read it.
Regards,
Mike
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
On Mon, Feb 27, 2012 at 6:49 PM, Turquette, Mike mturque...@ti.com wrote:
On Sun, Feb 26, 2012 at 8:47 PM, Robert Lee rob@linaro.org wrote:
+/**
+ * cpuidle_enter_wrap - performing timekeeping and irq around enter function
+ * @dev: pointer to a valid cpuidle_device object
+ * @drv: pointer to a valid cpuidle_driver object
+ * @index: index of the target cpuidle state.
+ */
+static inline int cpuidle_wrap_enter(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int index,
+ int (*enter)(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int
index))
+{
+ ktime_t time_start, time_end;
+ s64 diff;
+
+ time_start = ktime_get();
+
+ index = enter(dev, drv, index);
+
+ time_end = ktime_get();
+
+ local_irq_enable();
+
+ diff = ktime_to_us(ktime_sub(time_end, time_start));
+ if (diff INT_MAX)
+ diff = INT_MAX;
+
+ dev-last_residency = (int) diff;
+
+ return index;
+}
Hi Rob,
In a previous series I brought up the idea of not accounting for time
if a C-state transition fails. My post on that thread can be found
here:
http://article.gmane.org/gmane.linux.ports.arm.kernel/149293/
How do you feel about adding something like the following?
if (IS_ERR(index))
dev-last_residency = 0;
return index;
Obviously it will up to the platforms to figure out how to propagate
that error up from their respective low power code.
To be completely clear on what you're asking for, from
cpuidle_idle_call in drivers/cpuidle/cpuidle.c:
...
target_state = drv-states[next_state];
trace_power_start(POWER_CSTATE, next_state, dev-cpu);
trace_cpu_idle(next_state, dev-cpu);
entered_state = target_state-enter(dev, drv, next_state);
trace_power_end(dev-cpu);
trace_cpu_idle(PWR_EVENT_EXIT, dev-cpu);
if (entered_state = 0) {
/* Update cpuidle counters */
/* This can be moved to within driver enter routine
* but that results in multiple copies of same code.
*/
dev-states_usage[entered_state].time +=
(unsigned long long)dev-last_residency;
dev-states_usage[entered_state].usage++;
}
...
Note the if (entered_state = 0). This ultimately prevents the
cpuidle device time accounting upon an negative value being returned.
So are you asking for the if(IS_ERR(index)) check to prevent the
unnecessary last_residency time calculation in the wrapper, or to make
sure a last_residency is zero upon failure? (or both?)
It seems like a bug (or lack or documentation at best) in the code
that exists today to not zero out dev-last_residency upon a negative
return value as this value is used by the governors upon the next
idle. So to ensure last_residency is 0 upon failure, I think it'd be
best to add that to an new else statement immediately following the
if (entered_state =)) so that any platform cpuidle driver that
returns a negative will have the last_residency zeroed out, not just
those that use en_core_tk_irqen.
Regards,
Mike
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
Any reason that this code is in the header? Why not in cpuidle.c? Not a strong reason. I thought making it an inline would introduce slightly less new execution when adding this code (realizing that there are function calls immediately after, so the only benefit is the reduce popping and pushing). But it does require an extra copy of this code for any platform driver that does not enable en_core_tk_irqen and instead makes calls to it directly (like omap3). For this case, I don't think the inline implementation should add extra code from what exists today as it should simply replace the existing platform time keeping calls to a standard one defined by the core cpuidle. But you will have multiple copies of the inlined code if platforms do use it. Or is it used only by the core cpuidle code? In that case, gcc can automatically inline static functions. Used by some platforms as well. It seems a bit long to inline and this isn't performance critical (at least for the enter side). Ok. Unless there are further comments supporting the inline method, I'll switch to non-inline for next version. Thanks Mike and Rob for the feedback. Rob I don't have a strong preference with using the inline so if you or others can give your opinion on which method to use and why, I'd be glad to read it. Regards, Mike ___ linux-arm-kernel mailing list linux-arm-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
I brought this topic up internally and Jon suggested that the 'usage' statistics that are reported in sysfs should also reflect failed versus successful C-state transitions, which is a great idea. This could simply be achieved by renaming the current 'usage' count to something like 'transitions_attempted' and then conditionally increment a new counter within the 'if (entered_state = 0)' block, perhaps named, 'transition_succeeded'. This way the old 'usage' count paradigm is as accurate as the new time-keeping code. Being able to easily observe which C-state tend to fail the most would be invaluable in tuning idle policy for maximum effectiveness. Thoughts? Sounds reasonable. In some cases it may be helpful to track state demotion as well. Since I'm still a noob and wearing my submission training wheels, I'm trying to minimize things that fall outside of this basic consolidation effort for this patch series. But I added Jon's suggestion to this cpuidle page which contains future cpuidle items to consider adding: https://wiki.linaro.org/WorkingGroups/PowerManagement/Doc/CPUIdle#Track_both_attempted_and_successful_enter_attempts Regards, Mike Regards, Mike ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
Sounds reasonable. In some cases it may be helpful to track state demotion as well. Since I'm still a noob and wearing my submission training wheels, I'm trying to minimize things that fall outside of this basic consolidation effort for this patch series. But I added Jon's suggestion to this cpuidle page which contains future cpuidle items to consider adding: https://wiki.linaro.org/WorkingGroups/PowerManagement/Doc/CPUIdle#Track_both_attempted_and_successful_enter_attempts Yeah, I don't want to feature-bloat your submission more than necessary. I'm happy for the usage counter stuff to get tackled at a later date, but you're still on board for setting last_residency to zero in this series, right? Yes. Regards, Mike ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 2/9] SH: shmobile: cpuidle consolidation
Adding sh mailing list and sh contributors I missed on the original
submission. SH folks, full patchset submission can be found here:
http://www.spinics.net/lists/arm-kernel/msg161596.html
Best Regards,
Rob
On Sun, Feb 26, 2012 at 10:47 PM, Robert Lee rob@linaro.org wrote:
Enable core cpuidle timekeeping and irq enabling and remove that
handling from this code.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/sh/kernel/cpu/shmobile/cpuidle.c | 10 +++---
1 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/arch/sh/kernel/cpu/shmobile/cpuidle.c
b/arch/sh/kernel/cpu/shmobile/cpuidle.c
index 6d62eb4..1ddc876 100644
--- a/arch/sh/kernel/cpu/shmobile/cpuidle.c
+++ b/arch/sh/kernel/cpu/shmobile/cpuidle.c
@@ -29,7 +29,6 @@ static int cpuidle_sleep_enter(struct cpuidle_device *dev,
int index)
{
unsigned long allowed_mode = SUSP_SH_SLEEP;
- ktime_t before, after;
int requested_state = index;
int allowed_state;
int k;
@@ -47,19 +46,16 @@ static int cpuidle_sleep_enter(struct cpuidle_device *dev,
*/
k = min_t(int, allowed_state, requested_state);
- before = ktime_get();
sh_mobile_call_standby(cpuidle_mode[k]);
- after = ktime_get();
-
- dev-last_residency = (int)ktime_to_ns(ktime_sub(after, before))
10;
return k;
}
static struct cpuidle_device cpuidle_dev;
static struct cpuidle_driver cpuidle_driver = {
- .name = sh_idle,
- .owner = THIS_MODULE,
+ .name = sh_idle,
+ .owner = THIS_MODULE,
+ .en_core_tk_irqen = 1,
};
void sh_mobile_setup_cpuidle(void)
--
1.7.1
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
On Mon, Feb 27, 2012 at 10:19 AM, Jean Pihet jean.pi...@newoldbits.com wrote:
Robert,
On Mon, Feb 27, 2012 at 5:47 AM, Robert Lee rob@linaro.org wrote:
Add functionality that is commonly duplicated by various platforms.
Signed-off-by: Robert Lee rob@linaro.org
---
drivers/cpuidle/cpuidle.c | 37 ++
include/linux/cpuidle.h | 55
+
2 files changed, 77 insertions(+), 15 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
...
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 712abcc..6563683 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -15,6 +15,7 @@
#include linux/list.h
#include linux/kobject.h
#include linux/completion.h
+#include linux/hrtimer.h
#define CPUIDLE_STATE_MAX 8
#define CPUIDLE_NAME_LEN 16
@@ -56,6 +57,16 @@ struct cpuidle_state {
#define CPUIDLE_DRIVER_FLAGS_MASK (0x)
+/* Common ARM WFI state */
+#define CPUIDLE_ARM_WFI_STATE {\
+ .enter = cpuidle_simple_enter,\
+ .exit_latency = 1,\
+ .target_residency = 1,\
+ .flags = CPUIDLE_FLAG_TIME_VALID,\
+ .name = WFI,\
+ .desc = ARM core clock gating (WFI),\
+}
This macro should belong to an ARM only header.
Thanks, I was wondering about that but wasn't which location was better.
+
/**
* cpuidle_get_statedata - retrieves private driver state data
* @st_usage: the state usage statistics
@@ -122,6 +133,14 @@ struct cpuidle_driver {
struct module *owner;
unsigned int power_specified:1;
+ /*
+ * use the core cpuidle time keeping. This has been implemented for
the
+ * entire driver instead of per state as currently the device enter
+ * fuction allows the entered state to change which requires handling
+ * that requires a (subjectively) unnecessary decrease to efficiency
+ * in this commonly called code.
+ */
+ unsigned int en_core_tk_irqen:1;
Does the description reads as 'the time accounting is only performed
if en_core_tk_irqen is set'?
Correct. I can make this clearer in the next version's comments.
Also it suggests that changing (i.e. demoting) the state is kind of a
problem, which is unclear. IIUC the state change is handled correctly
in cpuidle_idle_call, is that correct?
If en_core_tk_irqen was a cpuidle state option instead of a cpuidle
driver option, then if the platform enter function changed the state
to one that used a different en_core_tk_irqen value, a time keeping
problem could occur. Extra handling could be added, but for many SMP
systems, this case will be the most common case, and so I didn't think
it best to force this extra handling. Anyways, with the current
implementation, if a platform cpuidle driver chooses not to use
en_core_tk_irqen, they can still use the consolidated time keeping
functionality by using the exported inline function (e.g., the OMAP
34XX case).
struct cpuidle_state states[CPUIDLE_STATE_MAX];
int state_count;
int safe_state_index;
@@ -140,6 +159,39 @@ extern void cpuidle_pause_and_lock(void);
extern void cpuidle_resume_and_unlock(void);
extern int cpuidle_enable_device(struct cpuidle_device *dev);
extern void cpuidle_disable_device(struct cpuidle_device *dev);
+extern int cpuidle_simple_enter(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int index);
+
+/**
+ * cpuidle_enter_wrap - performing timekeeping and irq around enter function
+ * @dev: pointer to a valid cpuidle_device object
+ * @drv: pointer to a valid cpuidle_driver object
+ * @index: index of the target cpuidle state.
+ */
+static inline int cpuidle_wrap_enter(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int index,
+ int (*enter)(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int
index))
The function name does not match the description (cpuidle_enter_wrap
vs cpuidle_wrap_enter).
Oops, thanks.
+{
+ ktime_t time_start, time_end;
+ s64 diff;
+
+ time_start = ktime_get();
+
+ index = enter(dev, drv, index);
+
+ time_end = ktime_get();
+
+ local_irq_enable();
+
+ diff = ktime_to_us(ktime_sub(time_end, time_start));
+ if (diff INT_MAX)
+ diff = INT_MAX;
+
+ dev-last_residency = (int) diff;
+
+ return index;
+}
#else
static inline void disable_cpuidle(void) { }
...
Regards,
Jean
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 3/9] ARM: omap: Consolidate OMAP3 cpuidle time keeping and irq enable
On Mon, Feb 27, 2012 at 10:26 AM, Jean Pihet jean.pi...@newoldbits.com wrote:
On Mon, Feb 27, 2012 at 5:47 AM, Robert Lee rob@linaro.org wrote:
Use core cpuidle timekeeping and irqen wrapper and remove that
handling from this code.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/mach-omap2/cpuidle34xx.c | 43
+++-
1 files changed, 18 insertions(+), 25 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
b/arch/arm/mach-omap2/cpuidle34xx.c
index 464cffd..1f6123f 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
...
/**
+ * omap3_enter_idle - Programs OMAP3 to enter the specified state
+ * @dev: cpuidle device
+ * @drv: cpuidle driver
+ * @index: the index of state to be entered
+ *
+ * Called from the CPUidle framework to program the device to the
+ * specified target state selected by the governor.
+ */
+static int omap3_enter_idle(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv,
+ int index)
+{
+ return cpuidle_wrap_enter(dev, drv, index, __omap3_enter_idle);
Is it the intention to call cpuidle_wrap_enter from the non-common
code? Could the driver set en_core_tk_irqen to 1 and so let the core
call the idle function? Is it to have the time measurement code closer
to the low level idle code in __omap3_enter_idle?
Yes, Yes, and Yes.
+}
+
+/**
* next_valid_state - Find next valid C-state
* @dev: cpuidle device
* @drv: cpuidle driver
@@ -295,6 +287,7 @@ static int omap3_enter_idle_bm(struct cpuidle_device
*dev,
new_state_idx = next_valid_state(dev, drv, index);
select_state:
+
Stray change
Oops, thanks.
ret = omap3_enter_idle(dev, drv, new_state_idx);
/* Restore original PER state if it was modified */
--
1.7.1
Regards,
Jean
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC PATCH v4 0/4] Consolidate cpuidle timekeeping and irq enabling
On Wed, Feb 22, 2012 at 2:52 PM, Colin Cross ccr...@google.com wrote: On Fri, Feb 10, 2012 at 11:32 AM, Rob Lee rob@linaro.org wrote: Maintainers for drivers/cpuidle, do you have any comments/opinions about this patch? Intel cpuidle and acpi cpuidle maintainers, do you have any comments/opinions about this patch and the changes to your code? Any other review and comments welcome. Summary of positive and negatives as I understand them so far: version 1, 2, and 3 (Original wrapper method of consolidating timekeeping and interrupt enabling) + opportunistically provides consolidation for simple platform cpuidle implementations without disturbing the more complex implementations. By simple, I mean those at can be wrapped in the time keeping calls and interrupt enabling calls without significantly affecting idle time keeping accuracy or interrupt latency - Does not provide consolidation for the more complex platform cpuidle implementations - Adds an additional interface, perhaps unnecessarily if this consolidation could be done in cpuidle version 4 (modifications to drivers/cpuidle/cpuidle.c cpuidle_idle_call) + Adds consolidation work to cpuidle_idle_call which allows all platform timekeeping / interrupt handling to be consolidated. I think the question of what the timekeeping means needs to be considered. If the timekeeping is supposed to be a very accurate measurement of the time spent in the low power idle state, only the cpuidle driver can guarantee that - there may be significant time spent in the hardware transition or the very low level power code that cannot be split into pre_enter, but should not be counted in the timekeeping. Or there may be a long boot time out of the low power state that should not be counted. If it is just a rough estimate of how often the cpu is getting to idle, there is no need to split out the pre_enter time - just measure the time around the entire driver enter call. Either way, pre_enter doesn't seem useful. - Requires splitting up of more complex platform cpuidle implementations, adding further complexity and risk of breaking something. ? Allows both pre_enter or enter to change the idle state. Is there an objection to this? pre_enter (if it is kept) would probably have to support state demotion, because its actions may depend on the final state. For coupled SMP cpuidle, enter also has to support state demotion, because the final state will depend on actions of the other cpu after idle has been entered. ? Splitting up the enter functions can require additional function calls. Is there any concern that this is significant additional overhead? I don't think so, especially if you support NULL pre_enter and post_enter functions to allow drivers that care to skip them. Colin, thanks for your comments. For now, my plan is to release a v5 buy the end of this week that is a continuation of v3 (using an optional wrapper for consolidation of time keeping and irq enabling). The slightly non-positive feedback for the OMAP3 changes and the lack of feedback from the intel and acpi cpuidle maintainers have swayed me toward this direction. I can continue working towards a solution in cpuidle_idle_call after this patch series if folks voice a preference for doing so. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC PATCH v4 0/4] Consolidate cpuidle timekeeping and irq enabling
Maintainers for drivers/cpuidle, do you have any comments/opinions about this patch? Intel cpuidle and acpi cpuidle maintainers, do you have any comments/opinions about this patch and the changes to your code? Any other review and comments welcome. Summary of positive and negatives as I understand them so far: version 1, 2, and 3 (Original wrapper method of consolidating timekeeping and interrupt enabling) + opportunistically provides consolidation for simple platform cpuidle implementations without disturbing the more complex implementations. By simple, I mean those at can be wrapped in the time keeping calls and interrupt enabling calls without significantly affecting idle time keeping accuracy or interrupt latency - Does not provide consolidation for the more complex platform cpuidle implementations - Adds an additional interface, perhaps unnecessarily if this consolidation could be done in cpuidle version 4 (modifications to drivers/cpuidle/cpuidle.c cpuidle_idle_call) + Adds consolidation work to cpuidle_idle_call which allows all platform timekeeping / interrupt handling to be consolidated. - Requires splitting up of more complex platform cpuidle implementations, adding further complexity and risk of breaking something. ? Allows both pre_enter or enter to change the idle state. Is there an objection to this? ? Splitting up the enter functions can require additional function calls. Is there any concern that this is significant additional overhead? Thanks, Rob On Tue, Jan 31, 2012 at 7:00 PM, Robert Lee rob@linaro.org wrote: This patch series moves the timekeeping and irq enabling from the platform code to the core cpuidle driver. Also, the platform irq disabling was removed as it appears that all calls into cpuidle_call_idle will have already called local_irq_disable(). To save reviewers time, only a few platforms which required the most changes are included in this version. If these changes are approved, the next version will include the remaining platform code which should require minimal changes. For those who have followed the previous patch versions, as you know, the previous version of this patch series added some helper functionality which used a wrapper function to remove the time keeping and irq enabling/disabling from the platform code. There was also initialization helper functionality which has now been removed from this version. If the basic implementation in this version is approved, then a separate patch submission effort can be made to focus on consolidation of initialziation functionality. Based on 3.3-rc1 v3 submission can be found here: http://www.spinics.net/lists/arm-kernel/msg156751.html Changes since v3: * Removed drivers/cpuidle/common.c ** Removed the initialization helper functions ** Removed the wrapper used to consolidate time keeping and irq enable/disable * Add time keeping and local_irq_disable handling in cpuidle_call_idle(). * Made necessary modifications to a few platforms that required the most changes ** Note on omap3: changed structure of omap3_idle_drvdata and added per_next_state and per_saved_state vars to accomodate new framework. v2 submission can be found here: http://comments.gmane.org/gmane.linux.ports.arm.kernel/144199 Changes since v2: * Made various code organization and style changes as suggested in v1 review. * Removed at91 use of common code. A separate effort is underway to clean at91 code and the author has offered to convert to common interface as part of those changes (if this common interface is accepted in time). * Made platform cpuidle_driver objects __initdata and dynamically added one persistent instance of this object in common code. * Removed imx5 pm usage of gpc_dvfs clock as it is no longer needed after being enabled during clock initialization. * Re-organized patches. v1 submission can be found here: http://comments.gmane.org/gmane.linux.ports.arm.kernel/142791 Changes since v1: * Common interface moved to drivers/cpuidle and made non arch-specific. * Made various fixes and suggested additions to the common cpuidle code from v1 review. * Added callback for filling in driver_data field as needed. * Modified the various platforms with these changes. Robert Lee (4): cpuidle: Add time keeping and irq enabling ARM: omap: Remove cpuidle timekeeping and irq enable/disable acpi: Remove cpuidle timekeeping and irq enable/disable x86: Remove cpuidle timekeeping and irq enable/disable arch/arm/mach-omap2/cpuidle34xx.c | 96 -- drivers/acpi/processor_idle.c | 203 ++--- drivers/cpuidle/cpuidle.c | 75 +++--- drivers/idle/intel_idle.c | 110 ++-- include/linux/cpuidle.h | 26 +++-- 5 files changed, 317 insertions(+), 193 deletions(-) ___ linaro-dev mailing list linaro-dev@lists.linaro.org
Re: [RFC PATCH v4 1/4] cpuidle: Add time keeping and irq enabling
On Sat, Feb 4, 2012 at 4:06 PM, Turquette, Mike mturque...@ti.com wrote:
On Sat, Feb 4, 2012 at 11:02 AM, Colin Cross ccr...@google.com wrote:
On Tue, Jan 31, 2012 at 7:00 PM, Robert Lee rob@linaro.org wrote:
Make necessary changes to add implement time keepign and irq enabling
keeping
in the core cpuidle code. This will allow the remove of these
functionalities from the platform cpuidle implementations.
Signed-off-by: Robert Lee rob@linaro.org
---
drivers/cpuidle/cpuidle.c | 75
+++-
include/linux/cpuidle.h | 26 ++-
2 files changed, 76 insertions(+), 25 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 59f4261..8ea0fc3 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -57,14 +57,18 @@ static int __cpuidle_register_device(struct
cpuidle_device *dev);
* cpuidle_idle_call - the main idle loop
*
* NOTE: no locks or semaphores should be used here
+ * NOTE: Should only be called from a local irq disabled context
* return non-zero on failure
+ *
*/
int cpuidle_idle_call(void)
{
struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
struct cpuidle_driver *drv = cpuidle_get_driver();
struct cpuidle_state *target_state;
- int next_state, entered_state;
+ int idx, ret = 0;
+ ktime_t t1, t2;
+ s64 diff;
if (off)
return -ENODEV;
@@ -86,37 +90,76 @@ int cpuidle_idle_call(void)
#endif
/* ask the governor for the next state */
- next_state = cpuidle_curr_governor-select(drv, dev);
+ idx = cpuidle_curr_governor-select(drv, dev);
+
+ target_state = drv-states[idx];
+
+ /*
+ * Check with the device to see if it can enter this state or if
another
+ * state should be used.
+ */
+ if (target_state-pre_enter) {
+ idx = target_state-
+ pre_enter(dev, drv, idx);
Unnecessary line wrap and braces.
What's the point of the pre_enter call? This seems very similar to
the prepare call that was removed in 3.2. Drivers can already demote
Hi Colin,
I asked Rob to re-introduce the .prepare callback (not .pre_enter).
The short version of why I requested this is so that I can experiment
with modifying wake-up latency and theshold values dynamically based
on PM QoS constraints. For an OMAP-specific example, I'd like to see
our C-states no longer model both MPU and CORE power domains, and
instead only model the MPU. Then when entering idle the PM QoS
constraints against the CORE power domain's wake-up latency can be
considered in the .prepare callback which will affect the C-state
parameters as well as program the CORE low power target state.
.pre_enter isn't really right for the above case so I'm happy for it
to be dropped, but I'll probably re-introduce something like .prepare
in the future...
Hey Mike, yes, this pre_enter has it's own purpose that appears
separate from the reasoning for the prepare function you mention here.
Perhaps it would be best to re-add the prepare function and make use
of it in the OMAP code all in one patch series so that it doesn't get
removed again due to apparent lack of use in the upstream kernel. I'm
sure we'll discuss it further this week in person.
Regards,
Mike
the target state in their enter call. The only thing you do between
pre_enter and enter is trace and account for the time. Is there some
long call you don't want included in the idle time? Some
documentation would help, and you need to very clearly define the
semantics of when post_enter gets called when pre_enter or enter
return errors.
___
linux-arm-kernel mailing list
linux-arm-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line unsubscribe linux-pm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC PATCH v4 1/4] cpuidle: Add time keeping and irq enabling
Hello Colin, thanks for the review.
On Sat, Feb 4, 2012 at 1:02 PM, Colin Cross ccr...@google.com wrote:
On Tue, Jan 31, 2012 at 7:00 PM, Robert Lee rob@linaro.org wrote:
Make necessary changes to add implement time keepign and irq enabling
keeping
in the core cpuidle code. This will allow the remove of these
functionalities from the platform cpuidle implementations.
Signed-off-by: Robert Lee rob@linaro.org
---
drivers/cpuidle/cpuidle.c | 75
+++-
include/linux/cpuidle.h | 26 ++-
2 files changed, 76 insertions(+), 25 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 59f4261..8ea0fc3 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -57,14 +57,18 @@ static int __cpuidle_register_device(struct
cpuidle_device *dev);
* cpuidle_idle_call - the main idle loop
*
* NOTE: no locks or semaphores should be used here
+ * NOTE: Should only be called from a local irq disabled context
* return non-zero on failure
+ *
*/
int cpuidle_idle_call(void)
{
struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
struct cpuidle_driver *drv = cpuidle_get_driver();
struct cpuidle_state *target_state;
- int next_state, entered_state;
+ int idx, ret = 0;
+ ktime_t t1, t2;
+ s64 diff;
if (off)
return -ENODEV;
@@ -86,37 +90,76 @@ int cpuidle_idle_call(void)
#endif
/* ask the governor for the next state */
- next_state = cpuidle_curr_governor-select(drv, dev);
+ idx = cpuidle_curr_governor-select(drv, dev);
+
+ target_state = drv-states[idx];
+
+ /*
+ * Check with the device to see if it can enter this state or if
another
+ * state should be used.
+ */
+ if (target_state-pre_enter) {
+ idx = target_state-
+ pre_enter(dev, drv, idx);
Unnecessary line wrap and braces.
Thanks.
What's the point of the pre_enter call? This seems very similar to
the prepare call that was removed in 3.2. Drivers can already demote
the target state in their enter call. The only thing you do between
pre_enter and enter is trace and account for the time. Is there some
long call you don't want included in the idle time? Some
documentation would help, and you need to very clearly define the
semantics of when post_enter gets called when pre_enter or enter
return errors.
pre_enter's purpose is to perform any necessary platform technology
that can be performed before entering that actual idle or some
restricted idle context where only platform specific code can run. If
I try to consolidate the timekeeping in the core cpuidle driver
without pre_enter, then my idle time will incorrectly account for this
platform preparation time. Perhaps this small idle time accounting
error isn't of concern to anyone though.
The previous version of this patch submission used a different
approach by adding a wrapper that could be used by fairly simple
cpuidle implementations. In the v3 submission of this patch series,
Daniel asked about the possibility of just performing this
consolidation in the cpuidle_idle_call function itself. Instead of
debating the pros and cons of it, I thought it might be better to send
this version of the patch and discuss it further.
Another approach I thought about may be to add flags that indicate
whether or not the platform or the core driver should perform the time
keeping. Or, go back to the wrapper function approach. Please feel
free to give your opinion on the preferred approach.
Agree on the documentation. I was trying to just get this patch
series out there for discussion before spending more time on it in
case it is not the desired approach to timekeeping and irq disabling
consolidation.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC PATCH v4 2/4] ARM: omap: Remove cpuidle timekeeping and irq enable/disable
On Thu, Feb 2, 2012 at 10:21 AM, Jean Pihet jean.pi...@newoldbits.com wrote:
Rob,
On Wed, Feb 1, 2012 at 4:00 AM, Robert Lee rob@linaro.org wrote:
Now that the core cpuidle driver keeps time and handles irq enabling,
remove this functionality. Also, remove irq disabling as all paths to
cpuidle_idle_call already call local_irq_disable. Also, restructure
idle functions as needed by the cpuidle core driver changes.
Signed-off-by: Robert Lee rob@linaro.org
---
arch/arm/mach-omap2/cpuidle34xx.c | 96
1 files changed, 43 insertions(+), 53 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
b/arch/arm/mach-omap2/cpuidle34xx.c
index 464cffd..9ecded5 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
...
@@ -100,23 +107,20 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
struct cpuidle_driver *drv,
int index)
{
- struct omap3_idle_statedata *cx =
- cpuidle_get_statedata(dev-states_usage[index]);
- struct timespec ts_preidle, ts_postidle, ts_idle;
- u32 mpu_state = cx-mpu_state, core_state = cx-core_state;
- int idle_time;
+ struct omap3_idle_drvdata *dd = dev-states_usage[index].driver_data
A build error is triggered by the missing ;.
Argh, last minute change and I didn't build afterward.
...
@@ -147,18 +151,12 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
pwrdm_for_each_clkdm(core_pd, _cpuidle_allow_idle);
}
-return_sleep_time:
- getnstimeofday(ts_postidle);
- ts_idle = timespec_sub(ts_postidle, ts_preidle);
-
- local_irq_enable();
+leave:
local_fiq_enable();
- idle_time = ts_idle.tv_nsec / NSEC_PER_USEC + ts_idle.tv_sec * \
- USEC_PER_SEC;
-
- /* Update cpuidle counters */
- dev-last_residency = idle_time;
+ /* Restore original PER state if it was modified */
+ if (dd-per_next_state != dd-per_saved_state)
+ pwrdm_set_next_pwrst(per_pd, dd-per_saved_state);
This code is not necessarily balanced with the PER state change in
omap3_enter_idle_bm (cf. /* Are we changing PER target state? */
here below), since in the core cpuidle code there is no guarantee that
the calls to pre_enter and enter callbacks are balanced.
In general I fear that splitting the code in two functions introduces
a risk of programming non-coherent settings in the PRCM.
Agree, in general it does introduce that new risk. For the platform
code changes, I tried to keep the code paths the same as before. I
see now where I created a problem though:
...
if (target_state-pre_enter) {
idx = target_state-
pre_enter(dev, drv, idx);
}
if (idx 0) {
local_irq_enable();
return idx;
}
if (need_resched()) {
local_irq_enable();
return -EBUSY;
}
...
The only way the core cpuidle pre_enter can get called without enter
without enter is if the omap3 next_valid_state() returned a negative
value. If this ever happened in the existing omap3 code, it would
cause it to break anyway. But, this particular need_resched() call
could cause an exit that results in difference behavior than before.
One solution to that is just to move the need_resched check before the
pre_enter call.
...
@@ -255,8 +255,9 @@ static int omap3_enter_idle_bm(struct cpuidle_device
*dev,
int index)
{
int new_state_idx;
- u32 core_next_state, per_next_state = 0, per_saved_state = 0,
cam_state;
- struct omap3_idle_statedata *cx;
+ u32 core_next_state, cam_state;
+ struct omap3_idle_drvdata *dd = dev-states_usage[index].driver_data;
+ struct omap3_idle_statedata *cx = dd-state_data[index];
int ret;
The build throws a few warnings about unused variables:
arch/arm/mach-omap2/cpuidle34xx.c: In function 'omap3_enter_idle_bm':
arch/arm/mach-omap2/cpuidle34xx.c:261: warning: unused variable 'ret'
arch/arm/mach-omap2/cpuidle34xx.c:257: warning: unused variable
'new_state_idx'
...
Got it, not sure why I missed this.
DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
@@ -337,7 +329,8 @@ static inline void _fill_cstate(struct cpuidle_driver
*drv,
state-exit_latency = cpuidle_params_table[idx].exit_latency;
state-target_residency = cpuidle_params_table[idx].target_residency;
state-flags = CPUIDLE_FLAG_TIME_VALID;
- state-enter = omap3_enter_idle_bm;
+ state-pre_enter = omap3_enter_idle_bm;
+ state-enter = omap3_enter_idle;
sprintf(state-name, C%d, idx + 1);
strncpy(state-desc, descr,
Re: [RFC PATCH 2/2] thermal: Add generic cpu cooling implementation
Hey Amit, I was able to use your code on an i.MX6Q thermal implementation and it seemed to work pretty well. Thanks for adding this. A couple of comments below. On Tue, Dec 13, 2011 at 9:13 AM, Amit Daniel Kachhap amit.kach...@linaro.org wrote: This patch adds support for generic cpu thermal cooling low level implementations using frequency scaling and cpuhotplugg currently. Different cpu related cooling devices can be registered by the user and the binding of these cooling devices to the corresponding trip points can be easily done as the registration API's return the cooling device pointer. Signed-off-by: Amit Daniel Kachhap amit.kach...@linaro.org --- Documentation/thermal/cpu-cooling-api.txt | 52 + drivers/thermal/Kconfig | 11 + drivers/thermal/Makefile | 1 + drivers/thermal/cpu_cooling.c | 302 + include/linux/cpu_cooling.h | 45 + 5 files changed, 411 insertions(+), 0 deletions(-) create mode 100644 Documentation/thermal/cpu-cooling-api.txt create mode 100644 drivers/thermal/cpu_cooling.c create mode 100644 include/linux/cpu_cooling.h diff --git a/Documentation/thermal/cpu-cooling-api.txt b/Documentation/thermal/cpu-cooling-api.txt new file mode 100644 index 000..d30b4f2 --- /dev/null +++ b/Documentation/thermal/cpu-cooling-api.txt @@ -0,0 +1,52 @@ +CPU cooling api's How To +=== + +Written by Amit Daniel Kachhap amit.kach...@linaro.org + +Updated: 13 Dec 2011 + +Copyright (c) 2011 Samsung Electronics Co., Ltd(http://www.samsung.com) + +0. Introduction + +The generic cpu cooling(freq clipping, cpuhotplug) provides +registration/unregistration api's to the user. The binding of the cooling +devices to the trip types is left for the user. The registration api's returns +the cooling device pointer. + +1. cpufreq cooling api's + +1.1 cpufreq registration api's +1.1.1 struct thermal_cooling_device *cpufreq_cooling_register( + struct freq_pctg_table *tab_ptr, unsigned int tab_size, + const struct cpumask *mask_val) + + This interface function registers the cpufreq cooling device with the name + thermal-cpufreq. + + tab_ptr: The table containing the percentage of frequency to be clipped for + each cooling state. + .freq_clip_pctg[NR_CPUS]:Percentage of frequency to be clipped for each + cpu. + .polling_interval: polling interval for this cooling state. + tab_size: the total number of cooling state. + mask_val: all the allowed cpu's where frequency clipping can happen. + +1.1.2 void cpufreq_cooling_unregister(void) + + This interface function unregisters the thermal-cpufreq cooling device. + + +1.2 cpuhotplug registration api's + +1.2.1 struct thermal_cooling_device *cpuhotplug_cooling_register( + const struct cpumask *mask_val) + + This interface function registers the cpuhotplug cooling device with the name + thermal-cpuhotplug. + + mask_val: all the allowed cpu's which can be hotplugged out. + +1.1.2 void cpuhotplug_cooling_unregister(void) + + This interface function unregisters the thermal-cpuhotplug cooling device. diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index f7f71b2..298c1cd 100644 --- a/drivers/thermal/Kconfig +++ b/drivers/thermal/Kconfig @@ -18,3 +18,14 @@ config THERMAL_HWMON depends on THERMAL depends on HWMON=y || HWMON=THERMAL default y + +config CPU_THERMAL + bool generic cpu cooling support + depends on THERMAL + help + This implements the generic cpu cooling mechanism through frequency + reduction, cpu hotplug and any other ways of reducing temperature. An + ACPI version of this already exists(drivers/acpi/processor_thermal.c). + This will be useful for platforms using the generic thermal interface + and not the ACPI interface. + If you want this support, you should say Y or M here. diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile index 31108a0..655cbc4 100644 --- a/drivers/thermal/Makefile +++ b/drivers/thermal/Makefile @@ -3,3 +3,4 @@ # obj-$(CONFIG_THERMAL) += thermal_sys.o +obj-$(CONFIG_CPU_THERMAL) += cpu_cooling.o diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c new file mode 100644 index 000..cdd148c --- /dev/null +++ b/drivers/thermal/cpu_cooling.c @@ -0,0 +1,302 @@ +/* + * linux/drivers/thermal/cpu_cooling.c + * + * Copyright (C) 2011 Samsung Electronics Co., Ltd(http://www.samsung.com) + * Copyright (C) 2011 Amit Daniel amit.kach...@linaro.org + * + * ~~ + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General
Re: [linux-pm] [RFC PATCH 1/2] thermal: Add a new trip type to use cooling device instance number
Hey Amit/Vincent,
It appears that with this implementation the STATE_ACTIVE trip number
used will also be the index of the cool_freq_tab used. If that is
true, then perhaps a common structure would be beneficial that links
each STATE_ACTIVE trip point with its corresponding cooling data.
BR,
Rob
On Tue, Dec 20, 2011 at 11:11 PM, Amit Kachhap amit.kach...@linaro.org wrote:
Hi Vincent,
Thanks for the review.
Well actually your are correct that current temperature and last
temperature can be used to increase or decrease the cpu frequency. But
this has to be done again in cooling devices so to make the cooling
devices generic and to avoid the temperature comparison again this new
trip type passes the cooling device instance id.
Also about your queries that this may add dependency between trip
index and cooling state. This is actually needed and this dependency
is created when the cooling device is binded with trip points(For
cpufreq type cooling device just the instance of cooling device is
associated with trip points). More over the existing PASSIVE cooling
trip type does the same thing and iterates across all the cooling
state.
Thanks,
Amit Daniel
On 20 December 2011 18:07, Vincent Guittot vincent.guit...@linaro.org
wrote:
Hi Amit,
I'm not sure that using the trip index for setting the state of a
cooling device is a generic solution because you are adding a
dependency between the trip index and the cooling device state that
might be difficult to handle. This dependency implies that a cooling
device like cpufreq_cooling_device must be registered in the 1st trips
of a thermal_zone which is not possible when we want to register 2
cpufreq_cooling_devices in the same thermal_zone.
You should only rely on the current and last temperatures to detect if
a trip_temp has been crossed and you should increase or decrease the
current state of the cooling device accordingly.
something like below should work with cpufreq_cooling_device and will
not add any constraint on the trip index. The state of a cooling
device is increased/decreased once for each trip
+ case THERMAL_TRIP_STATE_ACTIVE:
+ list_for_each_entry(instance, tz-cooling_devices,
+ node) {
+ if (instance-trip != count)
+ continue;
+
+ cdev = instance-cdev;
+
+ if ((temp = trip_temp)
+ (trip_temp
tz-last_temperature)) {
+ cdev-ops-get_max_state(cdev,
+ max_state);
+ cdev-ops-get_cur_state(cdev,
+ current_state);
+ if (++current_state = max_state)
+
cdev-ops-set_cur_state(cdev,
+
current_state);
+ }
+ else if ((temp trip_temp)
+ (trip_temp =
tz-last_temperature)) {
+ cdev-ops-get_cur_state(cdev,
+ current_state);
+ if (current_state 0)
+
cdev-ops-set_cur_state(cdev,
+
--current_state);
+ }
+ break;
Regards,
Vincent
On 13 December 2011 16:13, Amit Daniel Kachhap amit.kach...@linaro.org
wrote:
This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This
trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling
device instance number. This helps the cooling device registered as
different instances to perform appropriate cooling action decision in
the set_cur_state call back function.
Also since the trip temperature's are in ascending order so some logic
is put in place to skip the un-necessary checks.
Signed-off-by: Amit Daniel Kachhap amit.kach...@linaro.org
---
Documentation/thermal/sysfs-api.txt | 4 ++--
drivers/thermal/thermal_sys.c | 27 ++-
include/linux/thermal.h | 1 +
3 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/Documentation/thermal/sysfs-api.txt
b/Documentation/thermal/sysfs-api.txt
index b61e46f..5c1d44e 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -184,8 +184,8 @@ trip_point_[0-*]_temp
trip_point_[0-*]_type
Strings which indicate the type of the trip point.
- E.g. it can be one of critical, hot, passive, active[0-*] for ACPI
Re: [PATCH] thermal: Add anatop thermal driver
Hey Paul, I'm already working on upstreaming a thermal driver for i.MX6. I'm currently on hold paused to upstream cpufreq for i.MX6 and do some cpuidle work but I predict that I'll be ready to submit a first version sometime next week. For all things power management related on i.MX(cpufreq, cpuidle, pm, thermal, etc.), you are welcome to discuss with me first so that we can coordinate things. Thanks, Rob On 4 December 2011 14:32, Amit Kucheria amit.kuche...@linaro.org wrote: Ying-Chun, Len signs-off several of the commits to the thermal framework[1]. So it is a good idea to cc him. Amit Daniel is currently working on patches to make the thermal framework less ACPI-centric. And Rob will be interested in the driver since he works at Freescale and is tasked with making sure the thermal management works on their platform. I've cc'ed them all. That said, unless Freescale is shipping ACPI on the i.MX6, this driver needs some serious work before it is ready for mainline. Some high-level comments inline. Regards, Amit [1] ./scripts/get_maintainer.pl -f drivers/thermal/thermal_sys.c On Sat, Dec 3, 2011 at 8:52 PM, Ying-Chun Liu (PaulLiu) paul@linaro.org wrote: From: Ying-Chun Liu (PaulLiu) paul@linaro.org Anatop thermal driver for Freescale imx6 system. This driver currently supports basic temperature reading. Signed-off-by: Anson Huang b20...@freescale.com Signed-off-by: Ying-Chun Liu (PaulLiu) paul@linaro.org --- drivers/thermal/Kconfig | 11 + drivers/thermal/Makefile | 1 + drivers/thermal/anatop_driver.h | 146 +++ drivers/thermal/anatop_thermal.c | 825 ++ 4 files changed, 983 insertions(+), 0 deletions(-) create mode 100644 drivers/thermal/anatop_driver.h create mode 100644 drivers/thermal/anatop_thermal.c diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index f7f71b2..792152e 100644 --- a/drivers/thermal/Kconfig +++ b/drivers/thermal/Kconfig @@ -18,3 +18,14 @@ config THERMAL_HWMON depends on THERMAL depends on HWMON=y || HWMON=THERMAL default y + +config THERMAL_ANATOP + tristate Anatop Thermal Zone + depends on THERMAL If thermal is really a requirement for MX6, as state below, it should be auto-selected when the SOC is enabled as follows: diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig index 5f7f9c2..41638c8 100644 --- a/arch/arm/mach-imx/Kconfig +++ b/arch/arm/mach-imx/Kconfig @@ -616,6 +616,7 @@ config SOC_IMX6Q select HAVE_IMX_MMDC select HAVE_IMX_SRC select USE_OF + select THERMAL_ANATOP help This enables support for Freescale i.MX6 Quad processor. + default y + help + This driver supports ANATOP thermal zones. It is HIGHLY + recommended that this option be enabled on MX6 platform, as + your processor(s) may be damaged without it. + + To compile this driver as a module, choose M here. diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile index 31108a0..2f88011 100644 --- a/drivers/thermal/Makefile +++ b/drivers/thermal/Makefile @@ -3,3 +3,4 @@ # obj-$(CONFIG_THERMAL) += thermal_sys.o +obj-$(CONFIG_THERMAL_ANATOP) += anatop_thermal.o diff --git a/drivers/thermal/anatop_driver.h b/drivers/thermal/anatop_driver.h new file mode 100644 index 000..5879fca --- /dev/null +++ b/drivers/thermal/anatop_driver.h @@ -0,0 +1,146 @@ +/* + * anatop_drivers.h + * + * Copyright (C) 2001, 2002 Andy Grover andrew.gro...@intel.com + * Copyright (C) 2001, 2002 Paul Diefenbaugh paul.s.diefenba...@intel.com + * I assume you copied an existing Intel driver and then hacked it to get it working on your platform. You don't need this copyright to Intel above since you're creating a new file. However, it is polite to add a line saying what file this driver was copied from in the commit description or at the top of the file. + * Copyright (C) 2011 Freescale Semiconductor, Inc. + * ~~ + * + * 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. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA. + * + *
imx cpuidle pull request
(Re-send from my Linaro email address this time) Requesting to add the commits in the attached patch to the October Linaro release. These commits add a common imx cpuidle driver, some common cpuidle mach-mx5 code, and the init call for i.MX51 SoCs. git://git.linaro.org/people/rob_lee/imx_cpuidle.git imx_mx5_mx51 A patch series containing this functionality was also submitted to lkml: http://patchwork.ozlabs.org/patch/115012/ Besides some minor requested changes, this submission was not accepted as there is some common functionality being duplicated by many of the ARM SoC cpuidle implementations and Russell King wants there to be common ARM cpuidle code to handle this. The commits contained in this pull request address the other minor issues discussed in the community submission. I am working on a common ARM implementation will occur but it will take longer. In the mean time, i.MX platform coul use this imx cpuidle implementation in Linaro kernels. Rob ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
imx cpuidle pull request
Requesting to add the commits in the attached patch to the October Linaro release. These commits add a common imx cpuidle driver, some common cpuidle mach-mx5 code, and the init call for i.MX51 SoCs. git://git.linaro.org/people/rob_lee/imx_cpuidle.git imx_mx5_mx51 A patch series containing this functionality was also submitted to lkml: http://patchwork.ozlabs.org/patch/115012/ Besides some minor requested changes, this submission was not accepted as there is some common functionality being duplicated by many of the ARM SoC cpuidle implementations and Russell King wants there to be common ARM cpuidle code to handle this. The commits contained in this pull request address the other minor issues discussed in the community submission. I am working on a common ARM implementation will occur but it will take longer. In the mean time, i.MX platform coul use this imx cpuidle implementation in Linaro kernels. Rob The following changes since commit 30ad23712ecc22a4d026a4b6af21ac65c303325e: ARM: kprobes: work around build errors (2011-10-17 12:08:00 -0400) are available in the git repository at: git://git.linaro.org/people/rob_lee/imx_cpuidle.git imx_mx5_mx51 Robert Lee (3): ARM: imx: Add imx cpuidle driver ARM: imx: Add cpuidle for mach-mx5 ARM: imx: Add cpuidle for i.MX51 arch/arm/mach-mx5/Makefile |3 +- arch/arm/mach-mx5/cpuidle.c | 82 +++ arch/arm/mach-mx5/cpuidle.h | 13 +++ arch/arm/mach-mx5/mm.c |4 + arch/arm/plat-mxc/Makefile |1 + arch/arm/plat-mxc/cpuidle.c | 159 ++ arch/arm/plat-mxc/include/mach/cpuidle.h | 56 +++ 7 files changed, 316 insertions(+), 2 deletions(-) create mode 100644 arch/arm/mach-mx5/cpuidle.c create mode 100644 arch/arm/mach-mx5/cpuidle.h create mode 100644 arch/arm/plat-mxc/cpuidle.c create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
