Re: [RFC PATCH v2 3/4] cpuidle: (POWER) Enable cpuidle and directly call cpuidle_idle_call() for pSeries

[Deepthi Dharwar <deepthi@linux.vnet.ibm.com>]

On 11/28/2011 04:35 AM, Benjamin Herrenschmidt wrote:

> On Thu, 2011-11-17 at 16:58 +0530, Deepthi Dharwar wrote:
>> This patch enables cpuidle for pSeries and cpuidle_idle_call() is
>> directly called from the idle loop. As a result pseries_idle cpuidle
>> driver registered with cpuidle subsystem comes into action. This patch
>> also removes the routines pseries_shared_idle_sleep and
>> pseries_dedicated_idle_sleep as they are now implemented as part of
>> pseries_idle cpuidle driver.
>>
>> Signed-off-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
>> Signed-off-by: Trinabh Gupta <g.trinabh@gmail.com>
>> Signed-off-by: Arun R Bharadwaj <arun.r.bharadwaj@gmail.com>
>> ---
>>  arch/powerpc/platforms/Kconfig         |    6 ++
>>  arch/powerpc/platforms/pseries/setup.c |   86 +-------------------------------
>>  include/linux/cpuidle.h                |    2 -
>>  3 files changed, 8 insertions(+), 86 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
>> index e458872..0d2a028 100644
>> --- a/arch/powerpc/platforms/Kconfig
>> +++ b/arch/powerpc/platforms/Kconfig
>> @@ -211,6 +211,12 @@ config PPC_PASEMI_CPUFREQ
>>  
>>  endmenu
>>  
>> +menu "CPUIdle driver"
>> +
>> +source "drivers/cpuidle/Kconfig"
>> +
>> +endmenu
>> +
>>  config PPC601_SYNC_FIX
>>  	bool "Workarounds for PPC601 bugs"
>>  	depends on 6xx && (PPC_PREP || PPC_PMAC)
>> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
>> index 9c6716a..f624e74 100644
>> --- a/arch/powerpc/platforms/pseries/setup.c
>> +++ b/arch/powerpc/platforms/pseries/setup.c
>> @@ -39,6 +39,7 @@
>>  #include <linux/irq.h>
>>  #include <linux/seq_file.h>
>>  #include <linux/root_dev.h>
>> +#include <linux/cpuidle.h>
>>  
>>  #include <asm/mmu.h>
>>  #include <asm/processor.h>
>> @@ -74,9 +75,6 @@ EXPORT_SYMBOL(CMO_PageSize);
>>  
>>  int fwnmi_active;  /* TRUE if an FWNMI handler is present */
>>  
>> -static void pseries_shared_idle_sleep(void);
>> -static void pseries_dedicated_idle_sleep(void);
>> -
>>  static struct device_node *pSeries_mpic_node;
>>  
>>  static void pSeries_show_cpuinfo(struct seq_file *m)
>> @@ -374,18 +372,9 @@ static void __init pSeries_setup_arch(void)
>>  
>>  	pSeries_nvram_init();
>>  
>> -	/* Choose an idle loop */
>>  	if (firmware_has_feature(FW_FEATURE_SPLPAR)) {
>>  		vpa_init(boot_cpuid);
>> -		if (get_lppaca()->shared_proc) {
>> -			printk(KERN_DEBUG "Using shared processor idle loop\n");
>> -			ppc_md.power_save = pseries_shared_idle_sleep;
>> -		} else {
>> -			printk(KERN_DEBUG "Using dedicated idle loop\n");
>> -			ppc_md.power_save = pseries_dedicated_idle_sleep;
>> -		}
>> -	} else {
>> -		printk(KERN_DEBUG "Using default idle loop\n");
>> +		ppc_md.power_save = (void *)cpuidle_idle_call;
>>  	}
> 
> I very very much dislike that cast. You should not have to cast a
> function pointer ... EVER.



Yes, I ll fix this.
This actually bought out a design flaw with the current pseries idle as
mentioned by you in the next patch of the series.

> 
>>  	if (firmware_has_feature(FW_FEATURE_LPAR))
>> @@ -586,77 +575,6 @@ static int __init pSeries_probe(void)
>>  	return 1;
>>  }
>>  
> 
> Cheers,
> Ben.
> 
> 


Regards,
Deepthi