Re: [PATCH v7 4/7] qcom: pm: Add cpu low power mode functions

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

On Thu, Oct 02 2014 at 03:50 -0600, Lorenzo Pieralisi wrote:
>On Sat, Sep 27, 2014 at 01:58:12AM +0100, Lina Iyer wrote:
>
>[...]
>
>> diff --git a/drivers/soc/qcom/pm.c b/drivers/soc/qcom/pm.c
>> new file mode 100644
>> index 0000000..a2f7d72
>> --- /dev/null
>> +++ b/drivers/soc/qcom/pm.c
>> @@ -0,0 +1,109 @@
>> +static int qcom_pm_collapse(unsigned long int unused)
>> +{
>> +	int ret;
>> +	u32 flag;
>> +
>> +	ret = set_up_boot_address(cpu_resume, raw_smp_processor_id());
>> +	if (ret) {
>> +		pr_err("Failed to set warm boot address for cpu %d\n",
>> +				raw_smp_processor_id());
>> +		return ret;
>> +	}
>> +
>> +	flag = SCM_L2_ON & SCM_FLUSH_FLAG_MASK;
>> +	scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);
>
>Function call above does not return IIUC (ie it returns through cpu_resume),
>add a comment to clarify please.
>
Sure.
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * qcom_cpu_pm_enter_sleep(): Enter a low power mode on current cpu
>> + *
>> + * @mode - sleep mode to enter
>> + *
>> + * The code should be called with interrupts disabled and on the core on
>> + * which the low power mode is to be executed.
>> + *
>> + */
>> +static int qcom_cpu_pm_enter_sleep(enum pm_sleep_mode mode)
>> +{
>> +	int ret;
>> +
>> +	switch (mode) {
>> +	case PM_SLEEP_MODE_SPC:
>> +		qcom_spm_set_low_power_mode(SPM_MODE_POWER_COLLAPSE);
>> +		ret = cpu_suspend(0, qcom_pm_collapse);
>> +		break;
>> +	default:
>> +	case PM_SLEEP_MODE_WFI:
>> +		qcom_spm_set_low_power_mode(SPM_MODE_CLOCK_GATING);
>> +		ret = cpu_do_idle();
>
>Hmmm...are we sure the ret value you get from cpu_do_idle() is correct ?
>
>Maybe I am missing something but I do not think that the cpu_do_idle
>implementation (in assembly files mm/proc-* called through cpu_do_idle()
>macro) returns a proper value, it seems to me that it "returns" whatever is
>present in r0 at the time of the call, which might not be what you want.
>
>Am I missing something here ? I do not think you should rely on the
>cpu_do_idle() return value, at least for the moment.
>
Okay. I dont use it beyond this function. I shall remove it from this
function.

>> +		break;
>> +	}
>> +
>> +	local_irq_enable();
>
>This looks wrong to me. You end up running the CPU PM notifiers with irqs
>enabled, among other issues.
>
Good catch. Thanks!

Lina