Re: [PATCH v2 1/5] cpu_pm: Add cpu power management notifiers

[Santosh <santosh.shilimkar@ti.com>]

On Saturday 10 September 2011 04:26 AM, Andrew Morton wrote:
> On Sat, 3 Sep 2011 20:09:11 +0530
> Santosh Shilimkar<santosh.shilimkar@ti.com>  wrote:
>
>> From: Colin Cross<ccross@android.com>
>>
>> During some CPU power modes entered during idle, hotplug and
>> suspend, peripherals located in the CPU power domain, such as
>> the GIC, localtimers, and VFP, may be powered down.  Add a
>> notifier chain that allows drivers for those peripherals to
>> be notified before and after they may be reset.
>
> Have you identified which indivudual you hope/expect to merge this into
> mainline?
>
> The code is presumably and hopefully applicable to architectures other
> than ARM, yes?  Can you suggest likely candidate architectures so we
> can go off and bug the relevant maintainers to review it?
>
I was planning to send the pull request to Russell.

>>
>> ...
>>
>> +/*
>> + * When a CPU goes to a low power state that turns off power to the CPU's
>> + * power domain, the contents of some blocks (floating point coprocessors,
>> + * interrutp controllers, caches, timers) in the same power domain can
>
> s/interrutp/interrupt/
ok.
>
>> + * be lost.  The cpm_pm notifiers provide a method for platform idle, suspend,
>> + * and hotplug implementations to notify the drivers for these blocks that
>> + * they may be reset.
>> + *
>> + * All cpu_pm notifications must be called with interrupts disabled.
>> + *
>> + * The notifications are split into two classes, CPU notifications and CPU
>
> s/,/:/
ok
>
>> + * cluster notifications.
>> + *
>> + * CPU notifications apply to a single CPU, and must be called on the affected
>
> s/,// ;)
>
ok
>> + * CPU.  They are used to save per-cpu context for affected blocks.
>> + *
>> + * CPU cluster notifications apply to all CPUs in a single power domain. They
>> + * are used to save any global context for affected blocks, and must be called
>> + * after all the CPUs in the power domain have been notified of the low power
>> + * state.
>> + *
>
> Remove this line.
>
ok.

>> + */
>> +
>>
>> ...
>>
>> +/*
>> + * cpm_pm_enter
>> + *
>> + * Notifies listeners that a single cpu is entering a low power state that may
>> + * cause some blocks in the same power domain as the cpu to reset.
>> + *
>> + * Must be called on the affected cpu with interrupts disabled.  Platform is
>> + * responsible for ensuring that cpu_pm_enter is not called twice on the same
>> + * cpu before cpu_pm_exit is called.
>> + */
>
> It's unconventional to put the documentation over the declarations in the
> .h file.  It's not a *bad* idea per-se, but we generally don't do it.
> People will look at the definition in .c for the documentation and it
> if isn't there, some will assume that documentation doesn't exist.
>
> Plus: I don't know about others, but I don't configure ctags to lead me
> to declarations.  So finding the documentation for cpm_pm_enter() is a
> single keystroke if it's in the .c file, and a big PITA if it is in the
> .h file.
>
Will move that to C file.

> Also, this documentation could trivially be converted into kerneldoc
> format - you may as well do this?
>
ok

>> +int cpu_pm_enter(void);
>
> An actual design question: the interface assumes that CPU PM is a
> boolean state: on or off.  "a CPU goes to a low power state that turns
> off power to the CPU's power domain".
>
> Will that always be true for all CPUs?  Or should the interface have
> the capability of notifying clients of multi-level power state
> transitions?
>
Yes. Those are CPU cluster events. We already use them for
interrupt controller which looses power only when CPU cluster
looses power.

>> +
>> +/*
>> + * cpm_pm_exit
>> + *
>> + * Notifies listeners that a single cpu is exiting a low power state that may
>> + * have caused some blocks in the same power domain as the cpu to reset.
>> + *
>> + * Must be called on the affected cpu with interrupts disabled.
>
> It's unobvious (to little old me) why all these things need to be
> called under local_irq_disable().  I suggest the addition of a code
> comment and changelog update so that others are not similarly
> mystified.
>
These notifiers are used in CPUIDLE and suspend code. We were
aslo disabling the interrupt controller. Will add more description
to it.

>> + */
>> +int cpu_pm_exit(void);
>>
>> ...
>>
>> +int cpu_cluster_pm_enter(void)
>> +{
>> +	int nr_calls;
>> +	int ret = 0;
>> +
>> +	read_lock(&cpu_pm_notifier_lock);
>> +	ret = cpu_pm_notify(CPU_CLUSTER_PM_ENTER, -1,&nr_calls);
>> +	if (ret)
>> +		cpu_pm_notify(CPU_CLUSTER_PM_ENTER_FAILED, nr_calls - 1, NULL);
>
> What's going on with nr_calls?  Avoiding calling the most recently
> registered callback?  It is unclear why.  Some explanation here would
> be good.
>
ok.

>> +	read_unlock(&cpu_pm_notifier_lock);
>> +
>> +	return ret;
>> +}
>>
>> ...
>>
>> --- a/kernel/power/Kconfig
>> +++ b/kernel/power/Kconfig
>> @@ -235,3 +235,7 @@ config PM_GENERIC_DOMAINS
>>   config PM_GENERIC_DOMAINS_RUNTIME
>>   	def_bool y
>>   	depends on PM_RUNTIME&&  PM_GENERIC_DOMAINS
>> +
>> +config CPU_PM
>> +	def_bool y
>> +	depends on SUSPEND || CPU_IDLE
>
> This will unconditionally include kernel/cpu_pm.o in x86 kernels, and
> it's all dead code.  Fix, please!
The idea was to make it not depend on any arch. I can make this default
n and then enabled it on ARCH_ARM. Same things needs to be done on other
arch's whoever wants to use it.

Regards
Santsoh