Re: [RFC v2 04/12] ARM: cpuidle: Add runtime PM support for CPUs

[Lina Iyer <lina.iyer@linaro.org>]

On Fri, Feb 26 2016 at 11:24 -0700, Stephen Boyd wrote:
>On 02/12, Lina Iyer wrote:
>> @@ -45,6 +48,8 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
>>
>>  	ret = cpu_pm_enter();
>>  	if (!ret) {
>> +		RCU_NONIDLE(pm_runtime_put_sync_suspend(cpu_dev));
>
>Can you add a comment on why we need to use RCU_NONIDLE here?
>It's not super obvious.
>
OK.

>> +
>>  		/*
>>  		 * Pass idle state index to cpu_suspend which in turn will
>>  		 * call the CPU ops suspend protocol with idle index as a
>> @@ -52,6 +57,7 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
>>  		 */
>>  		arm_cpuidle_suspend(idx);
>>
>> +		RCU_NONIDLE(pm_runtime_get_sync(cpu_dev));
>>  		cpu_pm_exit();
>>  	}
>>
>> @@ -84,6 +90,30 @@ static const struct of_device_id arm_idle_state_match[] __initconst = {
>>  	{ },
>>  };
>>
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +static int cpu_hotplug(struct notifier_block *nb,
>
>This function is pretty generically named. Maybe something more
>runtime PM specific or cpu idle specific?
>
OK

>> +			unsigned long action, void *data)
>> +{
>> +	struct device *cpu_dev = get_cpu_device(smp_processor_id());
>> +
>> +	/* Execute CPU runtime PM on that CPU */
>> +	switch (action) {
>
>We could do the & ~CPU_TASKS_FROZEN trick here to save a few cases.
>
OK

>> +	case CPU_DYING:
>> +	case CPU_DYING_FROZEN:
>> +		RCU_NONIDLE(pm_runtime_put_sync_suspend(cpu_dev));
>
>And do we actually need to use it for hotplug path? These
>notifiers don't run from idle context do they?
>
True. Will remove.

> +
>>  /*
>>   * arm_idle_init
>>   *
>> @@ -96,6 +126,7 @@ static int __init arm_idle_init(void)
>>  	int cpu, ret;
>>  	struct cpuidle_driver *drv = &arm_idle_driver;
>>  	struct cpuidle_device *dev;
>> +	struct device *cpu_dev;
>>
>>  	/*
>>  	 * Initialize idle states data, starting at index 1.
>> @@ -148,10 +189,17 @@ static int __init arm_idle_init(void)
>>  		}
>>  	}
>>
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +	/* Register for hotplug notifications for runtime PM */
>> +	hotcpu_notifier(cpu_hotplug, 0);
>
>Define an empty cpu_hotplug() function for !CONFIG_HOTPLUG_CPU
>and then always call this without the ifdef?
>
I did this so we dont even register a hotplug notifier. Will change.

Thanks,
Lina