On Tue, Jan 15, 2013 at 06:17:37PM +0000, Mark Hambleton wrote:
Hi Lorenzo,
+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.
I figured that because the current version calls into the big.little platform power framework (bL_entry.c) and makes calls into that framework that this wasn't totally generic and is dependant upon that code. The version of the cpuidle driver won't build unless that code is built in, so I still think this is more appropriate naming, I could call it bL_* but I suspect someone will object to that upstream because of the mixed case.
Well, calling it big.LITTLE, bL or whatever else describing a big and a LITTLE is wrong IMHO because it makes people think this a driver for big.LITTLE ARM platforms. And it is not, if we ever manage to make it generic.
+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.
Ok, I will extend with a DT node to retrieve the states, it means I no longer need the generic compatibility node.
Extending the states require a standard and we need to adapt kernel code to comply and carry out required actions, it is not just a matter of adding some latencies in DT bindings.
I fail to understand why we want to make this code generic NOW, ARM kernel is not ready for that and to be honest I do not see why it has to be done now.
+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.
Not sure I understand this one, I am looking at the latest 3.8-rc3 tree and there is only 1 driver registered. Do I need to look at another version?
A cpuidle driver basically is an array of C-states. I am merging code that defines different states so different drivers for A15 and A7 clusters. cpuidle framework now supports multiple drivers, that's what we want for big.LITTLE platforms (but the generic idle driver should support as many drivers as required by DT bindings, which is 1 for symmetric systems.
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).
Ok, this was a patch to the linaro tree, it wasn't intended as a re-write of the driver.
I still do not understand why this file move has to be done now.
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.
Same again, this was based on what was already there... if you are re-working for PSCI please just treat this as a request to be more generic where possible.
To be generic we need multiple platforms and understand what are they doing with the respective C-states. I will update the code for PSCI.
This should be improved to cater for multiple drivers but that's not my major concern.
Ok, as I said this was just a move of tc2-cpuidle.c from mach-vexpress into drivers/cpuidle, if that driver is deficient then I suspect someone else is looking to fix it?
The driver is not deficient it is just a perfectly working cpuidle driver, TC2 specific, as tens of other cpuidle ARM drivers. It should be improved and made generic in due course, when we have a standard for C-states and the kernel save/restore and cache flushing mechanism is coded to follow suit.
Lorenzo
-
Lorenzo Pieralisi