On Tue, Jan 15, 2013 at 04:22:22PM +0000, Steve Bannister wrote:
> Thanks for this Mark,
>
> CCing Lorenzo as this doesn't seem to have come through on the linaro-devs
> list.
>
> Cheers,
>
> Steve
>
> On 15 Jan 2013, at 13:35, Mark Hambleton <mark.hamble...@broadcom.com> wrote:
>
> > Take the TC2 version of CPUIdle and remove TC2 specific line.
> > Move the file to drivers/cpuidle.
> > Introduce the concept of "arm,generic" compatible drivers to avoid
> > the need for a dummy dts node.
> >
> > Build the new CPUIdle based on CONFIG_BIG_LITTLE.
> >
> > Signed-off-by: mark <maham...@broadcom.com>
> > ---
> > Hi Nicolas / Dave,
> >
> > After plugging in our ASIC support to the new big.LITTLE frameworks I
> > realised that the TC2 CPUIdle driver was fundamentally generic, this patch
> > moves it from mach-vexpress into drivers/cpuidle and then checks for the
> > presence of a compatible node in the devicetree instead of checking for an
> > SPC.
> >
> > I haven't added a new config define as I figured it was specific to the
> > big.LITTLE framework, so just used that instead.
> >
> > I have tested the change on TC2 and am looking at extending the DCSCB code
> > to make it work on fastmodels.
> >
> > Any comments?
> >
> > Regards,
> >
> > Mark
[...]
> > diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> > index 03ee874..398ece7 100644
> > --- a/drivers/cpuidle/Makefile
> > +++ b/drivers/cpuidle/Makefile
> > @@ -4,5 +4,5 @@
> >
> > obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
> > obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
> > -
> > +obj-$(CONFIG_BIG_LITTLE) += arm_big_little.o
There is nothing big.LITTLE specific in all of this, so arm_idle.c would
be better.
> > obj-$(CONFIG_CPU_IDLE_CALXEDA) += cpuidle-calxeda.o
> > diff --git a/drivers/cpuidle/arm_big_little.c
> > b/drivers/cpuidle/arm_big_little.c
> > new file mode 100755
> > index 0000000..b97ebe0
> > --- /dev/null
> > +++ b/drivers/cpuidle/arm_big_little.c
> > @@ -0,0 +1,183 @@
> > +/*
> > + * big.LITTLE CPU idle driver.
See above.
> > + *
> > + * Copyright (C) 2012 ARM Ltd.
> > + * Author: Lorenzo Pieralisi <lorenzo.pieral...@arm.com>
Please copy me in next time.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/arm-cci.h>
> > +#include <linux/bitmap.h>
> > +#include <linux/cpuidle.h>
> > +#include <linux/cpu_pm.h>
> > +#include <linux/clockchips.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/hrtimer.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/tick.h>
> > +#include <linux/vexpress.h>
> > +#include <asm/bL_entry.h>
> > +#include <asm/cpuidle.h>
> > +#include <asm/cputype.h>
> > +#include <asm/idmap.h>
> > +#include <asm/proc-fns.h>
> > +#include <asm/suspend.h>
> > +#include <linux/of.h>
> > +
> > +static int bl_cpuidle_simple_enter(struct cpuidle_device *dev,
> > + struct cpuidle_driver *drv, int index)
> > +{
> > + ktime_t time_start, time_end;
> > + s64 diff;
> > +
> > + time_start = ktime_get();
> > +
> > + cpu_do_idle();
> > +
> > + 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;
> > +}
> > +
> > +static int bl_enter_powerdown(struct cpuidle_device *dev,
> > + struct cpuidle_driver *drv, int idx);
> > +
> > +static struct cpuidle_state bl_cpuidle_set[] __initdata = {
> > + [0] = {
> > + .enter = bl_cpuidle_simple_enter,
> > + .exit_latency = 1,
> > + .target_residency = 1,
> > + .power_usage = UINT_MAX,
> > + .flags = CPUIDLE_FLAG_TIME_VALID,
> > + .name = "WFI",
> > + .desc = "ARM WFI",
> > + },
> > + [1] = {
> > + .enter = bl_enter_powerdown,
> > + .exit_latency = 300,
> > + .target_residency = 1000,
> > + .flags = CPUIDLE_FLAG_TIME_VALID,
> > + .name = "C1",
> > + .desc = "ARM power down",
> > + },
> > +};
First problem, these states are not standard, in particular
target_residency, exit_latency and the enter function. We could
have a stab at initializing them from DT (or provide a way to probe
them from HW like Intel does), that's the only way to make this stuff
generic.
> > +struct cpuidle_driver bl_idle_driver = {
> > + .name = "bl_idle",
> > + .owner = THIS_MODULE,
> > + .safe_state_index = 0
> > +};
Different clusters have different drivers. This can be solved with
DT and a couple of bindings.
> > +
> > +static DEFINE_PER_CPU(struct cpuidle_device, bl_idle_dev);
> > +
> > +static int notrace bl_powerdown_finisher(unsigned long arg)
> > +{
> > + unsigned int mpidr = read_cpuid_mpidr();
> > + unsigned int cluster = (mpidr >> 8) & 0xf;
> > + unsigned int cpu = mpidr & 0xf;
> > +
> > + bL_set_entry_vector(cpu, cluster, cpu_resume);
This might not be what we want. We might want to resume from an
address different from cpu_resume (although it might well be
standardized since the power API allows to carry out platform
specific operations before jumping back to the kernel so I reckon
this can be and will be generic).
> > + bL_cpu_suspend(0); /* 0 should be replaced with better value here
> > */
> > + return 1;
> > +}
> > +
> > +/*
> > + * bl_enter_powerdown - Programs CPU to enter the specified state
> > + * @dev: cpuidle device
> > + * @drv: The target state to be programmed
> > + * @idx: state index
> > + *
> > + * Called from the CPUidle framework to program the device to the
> > + * specified target state selected by the governor.
> > + */
> > +static int bl_enter_powerdown(struct cpuidle_device *dev,
> > + struct cpuidle_driver *drv, int idx)
> > +{
> > + struct timespec ts_preidle, ts_postidle, ts_idle;
> > + int ret;
> > +
> > + /* Used to keep track of the total time in idle */
> > + getnstimeofday(&ts_preidle);
> > +
> > + BUG_ON(!irqs_disabled());
> > +
> > + cpu_pm_enter();
> > +
> > + clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
> > +
> > + ret = cpu_suspend((unsigned long) dev, bl_powerdown_finisher);
This is where things start getting a little more complicated. If we do
not know what the power state implies (e.g. L2 retained or not, global
timer or emulated local timers, GIC state lost in low-power or not) we
end up saving/restoring state or executing actions that may be useless.
In Intel world executing "mwait" does the trick for all C-states. Here
we need to define what a state is and what it implies to carry out the
required actions in a generic way, provided we can pull it off.
PSCI and the power API are a MAJOR step in this direction.
> > + if (ret)
> > + BUG();
> > +
> > + bL_cpu_powered_up();
> > +
> > + clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> > +
> > + cpu_pm_exit();
> > +
> > + getnstimeofday(&ts_postidle);
> > + local_irq_enable();
> > + ts_idle = timespec_sub(ts_postidle, ts_preidle);
> > +
> > + dev->last_residency = ts_idle.tv_nsec / NSEC_PER_USEC +
> > + ts_idle.tv_sec * USEC_PER_SEC;
> > + return idx;
> > +}
> > +
> > +/*
> > + * bl_idle_init
> > + *
> > + * Registers the bl specific cpuidle driver with the cpuidle
> > + * framework with the valid set of states.
> > + */
> > +int __init bl_idle_init(void)
> > +{
> > + struct cpuidle_device *dev;
> > + int i, cpu_id;
> > + struct cpuidle_driver *drv = &bl_idle_driver;
> > +
> > + if (!of_find_compatible_node(NULL, NULL, "arm,generic")) {
> > + pr_info("%s: No compatible node found\n", __func__);
> > + return -ENODEV;
> > + }
> > +
> > + drv->state_count = (sizeof(bl_cpuidle_set) /
> > + sizeof(struct cpuidle_state));
> > +
> > + for (i = 0; i < drv->state_count; i++) {
> > + memcpy(&drv->states[i], &bl_cpuidle_set[i],
> > + sizeof(struct cpuidle_state));
> > + }
> > +
> > + cpuidle_register_driver(drv);
> > +
> > + for_each_cpu(cpu_id, cpu_online_mask) {
> > + pr_err("CPUidle for CPU%d registered\n", cpu_id);
> > + dev = &per_cpu(bl_idle_dev, cpu_id);
> > + dev->cpu = cpu_id;
> > +
> > + dev->state_count = drv->state_count;
> > +
> > + if (cpuidle_register_device(dev)) {
> > + printk(KERN_ERR "%s: Cpuidle register device
> > failed\n",
> > + __func__);
> > + return -EIO;
> > + }
> > + }
This should be improved to cater for multiple drivers but that's not my
major concern.
Lorenzo
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
