Re: [PATCH v3 7/9] powerpc/powernv: Add platform support for stop instruction

[Shreyas B Prabhu]

On 05/24/2016 03:54 PM, Gautham R Shenoy wrote:
> Hi Shreyas,
> 
> On Mon, May 23, 2016 at 08:48:40PM +0530, Shreyas B. Prabhu wrote:
>> @@ -412,7 +517,8 @@ subcore_state_restored:
>>  first_thread_in_core:
>>
>>  	/*
>> -	 * First thread in the core waking up from fastsleep. It needs to
>> +	 * First thread in the core waking up from any state which can cause
>> +	 * partial or complete hypervisor state loss. It needs to
>>  	 * call the fastsleep workaround code if the platform requires it.
>>  	 * Call it unconditionally here. The below branch instruction will
>>  	 * be patched out when the idle states are discovered if platform
> 
> Please update the comment to 
> 
> "The below branch instruction will be patched out if the platform does
> not have fastsleep or does not require the workaround. Patching will
> be performed during the discovery of idle-states."

I've updated this in v4.
> 
>> @@ -423,8 +529,10 @@ pnv_fastsleep_workaround_at_exit:
>>  	b	fastsleep_workaround_at_exit
>>
>>  timebase_resync:
>> -	/* Do timebase resync if we are waking up from sleep. Use cr3 value
>> -	 * set in exceptions-64s.S */
>> +	/*
>> +	 * Use cr3 which indicates that we are waking up with atleast partial
>> +	 * hypervisor state loss to determine if TIMEBASE RESYNC is needed.
>> +	 */
>>  	ble	cr3,clear_lock
>>  	/* Time base re-sync */
>>  	li	r0,OPAL_RESYNC_TIMEBASE
> 
> 
> [..snip..]
>> @@ -264,6 +275,32 @@ static int __init pnv_init_idle_states(void)
>>  		goto out_free;
>>  	}
>>
>> +	if (cpu_has_feature(CPU_FTR_ARCH_300)) {
>> +		psscr_val = kcalloc(dt_idle_states, sizeof(*psscr_val),
>> +					GFP_KERNEL);
>> +		if (!psscr_val)
>> +			goto out_free;
>> +		if (of_property_read_u64_array(power_mgt,
>> +			"ibm,cpu-idle-state-psscr",
>> +			psscr_val, dt_idle_states)) {
>> +			pr_warn("cpuidle-powernv: missing ibm,cpu-idle-states-psscr in DT\n");
>> +			goto out_free_psscr;
>> +		}
>> +
>> +		/*
>> +		 * Set pnv_first_deep_stop_state to the first stop level
>> +		 * to cause hypervisor state loss
>> +		 */
>> +		pnv_first_deep_stop_state = 0xF;
>   		
> #define MAX_STOP_STATES	0xF ?

I changed this to MAX_POSSIBLE_STOP_STATE since 0xF is the max possible
stop state allowed by the ISA.

Thanks for the review,

-Shreyas