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

Based on work by many authors, available at codeaurora.org

Add interface layer to abstract and handle hardware specific
functionality for executing various cpu low power modes in QCOM
chipsets.

QCOM cpus support multiple low power modes. The C-States are defined as -

    * WFI (clock gating)
    * Retention (clock gating at lower power)
    * Standalone Power Collapse (Standalone PC or SPC) - The power to
    	the cpu is lost and the cpu warmboots.
    * Power Collapse (PC) - Same as SPC, but is a cognizant of the fact
    	that the SoC may do deeper sleep modes.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
[lina: simplify the driver for an initial submission, add commit text
description of idle states]
---
 drivers/soc/qcom/Makefile |   2 +-
 drivers/soc/qcom/pm.c     | 109 ++++++++++++++++++++++++++++++++++++++++++++++
 include/soc/qcom/pm.h     |  26 +++++++++++
 3 files changed, 136 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/qcom/pm.c
 create mode 100644 include/soc/qcom/pm.h

On 27-09-2014 06:28 AM, Lina Iyer wrote:
> Based on work by many authors, available at codeaurora.org
> 
> Add interface layer to abstract and handle hardware specific
> functionality for executing various cpu low power modes in QCOM
> chipsets.
> 

..

> +#include <soc/qcom/scm-boot.h>
> +#include <soc/qcom/spm.h>
> +
> +#define SCM_CMD_TERMINATE_PC	(0x2)
> +#define SCM_FLUSH_FLAG_MASK	(0x3)
> +#define SCM_L2_ON 		(0x0)
There is checkpatch warning here.

WARNING: please, no space before tabs
#75: FILE: drivers/soc/qcom/pm.c:28:
+#define SCM_L2_ON ^I^I(0x0)$

> +#define SCM_L2_OFF		(0x1)
> +
> +
..

> +
> +#endif  /* __QCOM_PM_H */
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 09/26/14 17:58, Lina Iyer wrote:
> Based on work by many authors, available at codeaurora.org
>
> Add interface layer to abstract and handle hardware specific
> functionality for executing various cpu low power modes in QCOM
> chipsets.
>
> QCOM cpus support multiple low power modes. The C-States are defined as -
>
>     * WFI (clock gating)
>     * Retention (clock gating at lower power)
>     * Standalone Power Collapse (Standalone PC or SPC) - The power to
>     	the cpu is lost and the cpu warmboots.
>     * Power Collapse (PC) - Same as SPC, but is a cognizant of the fact
>     	that the SoC may do deeper sleep modes.
>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> [lina: simplify the driver for an initial submission, add commit text
> description of idle states]

Maintainer tags don't really make sense unless there is another author.

> ---
>  drivers/soc/qcom/Makefile |   2 +-
>  drivers/soc/qcom/pm.c     | 109 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/soc/qcom/pm.h     |  26 +++++++++++
>  3 files changed, 136 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/soc/qcom/pm.c
>  create mode 100644 include/soc/qcom/pm.h
>
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 20b329f..19900ed 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -1,4 +1,4 @@
>  obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
> -obj-$(CONFIG_QCOM_PM)	+=	spm.o
> +obj-$(CONFIG_QCOM_PM)	+=	spm.o pm.o
>  CFLAGS_scm.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1)
>  obj-$(CONFIG_QCOM_SCM) += scm.o scm-boot.o
> 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 @@
> +/* Copyright (c) 2010-2014, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +
> +#include <asm/proc-fns.h>
> +#include <asm/suspend.h>
> +
> +#include <soc/qcom/pm.h>
> +#include <soc/qcom/scm.h>
> +#include <soc/qcom/scm-boot.h>
> +#include <soc/qcom/spm.h>
> +
> +#define SCM_CMD_TERMINATE_PC	(0x2)
> +#define SCM_FLUSH_FLAG_MASK	(0x3)
> +#define SCM_L2_ON 		(0x0)
> +#define SCM_L2_OFF		(0x1)
> +
> +
> +static int set_up_boot_address(void *entry, int cpu)
> +{
> +	static int flags[NR_CPUS] = {
> +		SCM_FLAG_WARMBOOT_CPU0,
> +		SCM_FLAG_WARMBOOT_CPU1,
> +		SCM_FLAG_WARMBOOT_CPU2,
> +		SCM_FLAG_WARMBOOT_CPU3,
> +	};
> +	static DEFINE_PER_CPU(void *, last_known_entry);
> +
> +	if (entry == per_cpu(last_known_entry, cpu))
> +		return 0;
> +
> +	per_cpu(last_known_entry, cpu) = entry;
> +	return scm_set_boot_addr(virt_to_phys(entry), flags[cpu]);
> +}
> +
> +static int qcom_pm_collapse(unsigned long int unused)
> +{
> +	int ret;
> +	u32 flag;
> +
> +	ret = set_up_boot_address(cpu_resume, raw_smp_processor_id());

Preemption better be off here, so why are we using raw_smp_processor_id()?

> +	if (ret) {
> +		pr_err("Failed to set warm boot address for cpu %d\n",
> +				raw_smp_processor_id());

Stuff cpu into a local variable?

> +		return ret;
> +	}
> +
> +	flag = SCM_L2_ON & SCM_FLUSH_FLAG_MASK;
> +	scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);
> +
> +	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);

Isn't it a one-to-one between PM_SLEEP_MODE_SPC and
SPM_MODE_POWER_COLLAPSE? The enum to enum map seems overly complicated.

> +		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();
> +		break;
> +	}
> +
> +	local_irq_enable();
> +
> +	return ret;
> +}
> +
> +static struct platform_device qcom_cpuidle_device = {
> +	.name              = "qcom_cpuidle",
> +	.id                = -1,
> +	.dev.platform_data = qcom_cpu_pm_enter_sleep,
> +};

This doesn't need to be static. Use platform_device_register_full() instead.

> +
> +static int __init qcom_pm_device_init(void)
> +{
> +	platform_device_register(&qcom_cpuidle_device);
> +
> +	return 0;
> +}
> +device_initcall(qcom_pm_device_init);

modules?

> diff --git a/include/soc/qcom/pm.h b/include/soc/qcom/pm.h
> new file mode 100644
> index 0000000..563b9c3
> --- /dev/null
> +++ b/include/soc/qcom/pm.h
> @@ -0,0 +1,26 @@
> +/*
> + * Copyright (c) 2009-2014, The Linux Foundation. All rights reserved.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef __QCOM_PM_H
> +#define __QCOM_PM_H
> +
> +enum pm_sleep_mode {
> +	PM_SLEEP_MODE_WFI,
> +	PM_SLEEP_MODE_RET,
> +	PM_SLEEP_MODE_SPC,
> +	PM_SLEEP_MODE_PC,
> +	PM_SLEEP_MODE_NR,
> +};
> +
> +#endif  /* __QCOM_PM_H */

Hopefully this header is not necessary.

On Mon, Sep 29 2014 at 17:37 -0600, Stephen Boyd wrote:
>On 09/26/14 17:58, Lina Iyer wrote:
>> Based on work by many authors, available at codeaurora.org
>>
>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> [lina: simplify the driver for an initial submission, add commit text
>> description of idle states]
>
>Maintainer tags don't really make sense unless there is another author.
>
Hmm.. Since this patch is a derivative work, I wanted to clarify, what
changed seems important. The work was done by many authors. Adding
signed-off from everybody who could have contributed to the patch
downstream is confusing.
I would be okay removing it.

> +
[...]
>> +static int qcom_pm_collapse(unsigned long int unused)
>> +{
>> +	int ret;
>> +	u32 flag;
>> +
>> +	ret = set_up_boot_address(cpu_resume, raw_smp_processor_id());
>
>Preemption better be off here, so why are we using raw_smp_processor_id()?
>
True, so raw_ returns without premeption disable, no?

>> +	if (ret) {
>> +		pr_err("Failed to set warm boot address for cpu %d\n",
>> +				raw_smp_processor_id());
>
>Stuff cpu into a local variable?
>
Yeah
>> +		return ret;
>> +	}
>> +
>> +	flag = SCM_L2_ON & SCM_FLUSH_FLAG_MASK;
>> +	scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);
>> +
>> +	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);
>
>Isn't it a one-to-one between PM_SLEEP_MODE_SPC and
>SPM_MODE_POWER_COLLAPSE? The enum to enum map seems overly complicated.
>
Not really. SPM modes, differ when the idle state has to notify RPM. It
does not have 2 enums for those modes, but uses an overloaded enum with
an additional flag.

>> +		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();
>> +		break;
>> +	}
>> +
>> +	local_irq_enable();
>> +
>> +	return ret;
>> +}
>> +
>> +static struct platform_device qcom_cpuidle_device = {
>> +	.name              = "qcom_cpuidle",
>> +	.id                = -1,
>> +	.dev.platform_data = qcom_cpu_pm_enter_sleep,
>> +};
>
>This doesn't need to be static. Use platform_device_register_full() instead.
>
Okay
>> +
>> +static int __init qcom_pm_device_init(void)
>> +{
>> +	platform_device_register(&qcom_cpuidle_device);
>> +
>> +	return 0;
>> +}
>> +device_initcall(qcom_pm_device_init);
>
>modules?
>
Why? An earlier initialization helps with power saving

>> diff --git a/include/soc/qcom/pm.h b/include/soc/qcom/pm.h
>> new file mode 100644
>> index 0000000..563b9c3
>> --- /dev/null
>> +++ b/include/soc/qcom/pm.h
>> @@ -0,0 +1,26 @@
>> +/*
>> + * Copyright (c) 2009-2014, The Linux Foundation. All rights reserved.
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#ifndef __QCOM_PM_H
>> +#define __QCOM_PM_H
>> +
>> +enum pm_sleep_mode {
>> +	PM_SLEEP_MODE_WFI,
>> +	PM_SLEEP_MODE_RET,
>> +	PM_SLEEP_MODE_SPC,
>> +	PM_SLEEP_MODE_PC,
>> +	PM_SLEEP_MODE_NR,
>> +};
>> +
>> +#endif  /* __QCOM_PM_H */
>
>Hopefully this header is not necessary.
>
>-- 
>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>hosted by The Linux Foundation
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lina Iyer <lina.iyer@linaro.org> writes:

> On Mon, Sep 29 2014 at 17:37 -0600, Stephen Boyd wrote:
>>On 09/26/14 17:58, Lina Iyer wrote:
>>> Based on work by many authors, available at codeaurora.org
>>>
>>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>> [lina: simplify the driver for an initial submission, add commit text
>>> description of idle states]
>>
>>Maintainer tags don't really make sense unless there is another author.
>>
> Hmm.. Since this patch is a derivative work, I wanted to clarify, what
> changed seems important. The work was done by many authors. Adding
> signed-off from everybody who could have contributed to the patch
> downstream is confusing.
> I would be okay removing it.

Yes, you should remove it, but move the "Based on work by many authors"
part to just before your signoff.  Also, it's considered good manners to
name any significant contributors if there are a few standouts.  Also,
the "available at codeaurora.org" is wanting a link, ideally to a
specific commit, or a branch so original code can easily be found.

Kevin
 
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 09/30/14 09:03, Lina Iyer wrote:
> On Mon, Sep 29 2014 at 17:37 -0600, Stephen Boyd wrote:
>> On 09/26/14 17:58, Lina Iyer wrote:
>>> Based on work by many authors, available at codeaurora.org
>>>
>>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>> [lina: simplify the driver for an initial submission, add commit text
>>> description of idle states]
>>
>> Maintainer tags don't really make sense unless there is another author.
>>
> Hmm.. Since this patch is a derivative work, I wanted to clarify, what
> changed seems important. The work was done by many authors. Adding
> signed-off from everybody who could have contributed to the patch
> downstream is confusing.
> I would be okay removing it.
>

Kevin's suggestion is good.

>> +
> [...]
>>> +static int qcom_pm_collapse(unsigned long int unused)
>>> +{
>>> +    int ret;
>>> +    u32 flag;
>>> +
>>> +    ret = set_up_boot_address(cpu_resume, raw_smp_processor_id());
>>
>> Preemption better be off here, so why are we using
>> raw_smp_processor_id()?
>>
> True, so raw_ returns without premeption disable, no?
>
>

No. raw_ just means "I know what I'm doing, go away preemption checks"
which usually is not the right thing if you actually care about the CPU
this code is running on staying the same after you call the function.

>>> +        return ret;
>>> +    }
>>> +
>>> +    flag = SCM_L2_ON & SCM_FLUSH_FLAG_MASK;
>>> +    scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);
>>> +
>>> +    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);
>>
>> Isn't it a one-to-one between PM_SLEEP_MODE_SPC and
>> SPM_MODE_POWER_COLLAPSE? The enum to enum map seems overly complicated.
>>
> Not really. SPM modes, differ when the idle state has to notify RPM. It
> does not have 2 enums for those modes, but uses an overloaded enum with
> an additional flag.
>

I imagine that the RPM notification will go into power domains? From the
cpuidle perspective we're putting the CPU into wfi, retention, gdhs,
power collapse and those correspond exactly to a flat mapping of the cpu
idle indices. Those idle states will tell the power domain "I have this
much time to sleep" and the power domain path will determine if we can
go notify the RPM or not based on the time and the state of the other
CPUs. At least this is my understanding of what Daniel was saying about
how power domains could be used to overcome the "hierarchical" idle states.

>>> +
>>> +static int __init qcom_pm_device_init(void)
>>> +{
>>> +    platform_device_register(&qcom_cpuidle_device);
>>> +
>>> +    return 0;
>>> +}
>>> +device_initcall(qcom_pm_device_init);
>>
>> modules?
>>
> Why? An earlier initialization helps with power saving

If that was true the bootloader should do it. The goal is to move away
from initcall ordering and use probe defer for these things.

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 @@
> +/* Copyright (c) 2010-2014, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +
> +#include <asm/proc-fns.h>
> +#include <asm/suspend.h>
> +
> +#include <soc/qcom/pm.h>
> +#include <soc/qcom/scm.h>
> +#include <soc/qcom/scm-boot.h>
> +#include <soc/qcom/spm.h>
> +
> +#define SCM_CMD_TERMINATE_PC	(0x2)
> +#define SCM_FLUSH_FLAG_MASK	(0x3)
> +#define SCM_L2_ON 		(0x0)
> +#define SCM_L2_OFF		(0x1)
> +
> +
> +static int set_up_boot_address(void *entry, int cpu)
> +{
> +	static int flags[NR_CPUS] = {
> +		SCM_FLAG_WARMBOOT_CPU0,
> +		SCM_FLAG_WARMBOOT_CPU1,
> +		SCM_FLAG_WARMBOOT_CPU2,
> +		SCM_FLAG_WARMBOOT_CPU3,
> +	};
> +	static DEFINE_PER_CPU(void *, last_known_entry);
> +
> +	if (entry == per_cpu(last_known_entry, cpu))
> +		return 0;
> +
> +	per_cpu(last_known_entry, cpu) = entry;
> +	return scm_set_boot_addr(virt_to_phys(entry), flags[cpu]);
> +}
> +
> +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.

> +
> +	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.

> +		break;
> +	}
> +
> +	local_irq_enable();

This looks wrong to me. You end up running the CPU PM notifiers with irqs
enabled, among other issues.

Lorenzo

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html