[PATCH v3 1/7] cpuidle: Add common init interface and idle functionality

Mike, thanks for your review.  Comments below.

On Tue, Jan 24, 2012 at 5:46 PM, Turquette, Mike [off-list ref] wrote:

On Mon, Jan 23, 2012 at 8:37 PM, Robert Lee [off-list ref] wrote:

quoted

diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c
new file mode 100644
index 0000000..dafa758
--- /dev/null
+++ b/drivers/cpuidle/common.c

@@ -0,0 +1,152 @@
+/*
+ * Copyright 2011 Freescale Semiconductor, Inc.
+ * Copyright 2011 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
+ */
+
+/*
+ * This code performs provides some commonly used cpuidle setup functionality

s/performs //

quoted

+static struct cpuidle_device __percpu * common_cpuidle_devices;

You can use DECLARE_PER_CPU here.

Ok.  Do you mean that DECLARE_PER_CPU is preferred in this case?

Is there any particular reason to allocate these dynamically? ?You can
replace the code above with,

static DEFINE_PER_CPU(struct cpuidle_device, common_cpuidle_devices);

I was thinking of the single kernel with multiple platform support
case.  In that particular case, it seems better to create the number
of device objects you need at run time.

I might change the variable name to "cpu_cpuidle_device" in that case
since you are addressing a single CPU when using the per cpu accessor
functions and "common_cpuidle_devices" sounds like an array or a list
or something. ?No big deal to keep the current name though.

quoted

+static int simple_enter(struct cpuidle_device *dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_driver *drv, int index)
+{
+ ? ? ? ktime_t time_start, time_end;
+
+ ? ? ? local_irq_disable();
+
+ ? ? ? time_start = ktime_get();
+
+ ? ? ? index = do_idle[index](dev, drv, index);
+
+ ? ? ? time_end = ktime_get();
+
+ ? ? ? local_irq_enable();
+
+ ? ? ? dev->last_residency =
+ ? ? ? ? ? ? ? (int)ktime_to_us(ktime_sub(time_end, time_start));

Sometimes an attempt to enter some C-state fails and the do_idle will
return immediately. ?What do you think about having do_idle return
-EERROR in this case and the conditionally setting last_residency to
zero in those cases? ?The point is that a C-state's total residency
time should not increase in the case where the hardware did not
successfully transition into that C-state. ?I've observed many times
where a specific low power state was actually achieved in the hardware
but /sys/devices/system/cpu/cpuN/cpuidle/stateM/time keeps
incrementing (albeit in very tiny increments). ?Something like,

if (IS_ERR(index))
? ? ? ?dev->last_residency = 0;
else
? ? ? ?...

Note: I haven't been through the CPUidle core in a while so maybe the
above suggestion violates some other requirements/assumptions...

Good suggestion.  I'll look into adding this to v4.

quoted

+
+ ? ? ? return index;
+}
+
+void common_cpuidle_devices_uninit(void)
+{
+ ? ? ? int cpu_id;
+ ? ? ? struct cpuidle_device *dev;
+
+ ? ? ? for_each_possible_cpu(cpu_id) {
+ ? ? ? ? ? ? ? dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
+ ? ? ? ? ? ? ? cpuidle_unregister_device(dev);
+ ? ? ? }
+
+ ? ? ? free_percpu(common_cpuidle_devices);

See note above about statically allocating the per-cpu variables.

quoted

+}
+
+/**
+ * common_cpuidle_init() - Provides some commonly used init functionality.
+ * @pdrv ? ? ? ? ? ? ? Pointer to your cpuidle_driver object.
+ * @simple ? ? ? ? ? ? Use the common simple_enter wrapper?
+ * @driver_data_init ? Pointer to your platform function to initialize your
+ * ? ? ? ? ? ? ? ? ? ? platform specific driver data. ?Use NULL if platform
+ * ? ? ? ? ? ? ? ? ? ? specific data is not needed.
+ *
+ * Common cpuidle init interface to provide common cpuidle functionality
+ * used by many platforms.
+ */
+int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple,
+ ? ? ? ? ? ? ? ? ? ? ? ?void (*driver_data_init)(struct cpuidle_device *dev))
+{
+ ? ? ? struct cpuidle_device *dev;
+ ? ? ? struct cpuidle_driver *drv;
+ ? ? ? int i, cpu_id, ret;
+
+ ? ? ? if (!pdrv || pdrv->state_count > CPUIDLE_STATE_MAX) {
+ ? ? ? ? ? ? ? pr_err("%s: Invalid Input\n", __func__);
+ ? ? ? ? ? ? ? return -EINVAL;
+ ? ? ? }
+
+ ? ? ? drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL);
+ ? ? ? if (!drv) {
+ ? ? ? ? ? ? ? pr_err("%s: no memory for cpuidle driver\n", __func__);
+ ? ? ? ? ? ? ? return -ENOMEM;
+ ? ? ? }
+
+ ? ? ? *drv = *pdrv;
+
+ ? ? ? for (i = 0; simple && (i < drv->state_count); i++) {
+ ? ? ? ? ? ? ? do_idle[i] = drv->states[i].enter;
+ ? ? ? ? ? ? ? drv->states[i].enter = simple_enter;
+ ? ? ? }
+
+ ? ? ? ret = cpuidle_register_driver(drv);
+ ? ? ? if (ret) {
+ ? ? ? ? ? ? ? pr_err("%s: Failed to register cpuidle driver\n", __func__);
+ ? ? ? ? ? ? ? goto free_drv;
+ ? ? ? }
+
+ ? ? ? common_cpuidle_devices = alloc_percpu(struct cpuidle_device);
+ ? ? ? if (common_cpuidle_devices == NULL) {
+ ? ? ? ? ? ? ? ret = -ENOMEM;
+ ? ? ? ? ? ? ? goto unregister_drv;
+ ? ? ? }

See note above about statically allocating these.

Regards,
Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at ?http://vger.kernel.org/majordomo-info.html