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 (0xFFFF0000)
>>
>> +/* 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
