[v2,07/10] qcom: msm-pm: Add cpu low power mode functions

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

Signed-off-by: Venkat Devarasetty <vdevaras@codeaurora.org>
Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/soc/qcom/Makefile |   2 +-
 drivers/soc/qcom/msm-pm.c | 219 ++++++++++++++++++++++++++++++++++++++++++++++
 include/soc/qcom/pm.h     |  39 +++++++++
 3 files changed, 259 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/qcom/msm-pm.c
 create mode 100644 include/soc/qcom/pm.h

On 08/12/2014 09:43 PM, Lina Iyer wrote:
> Add interface layer to abstract and handle hardware specific
> functionality for executing various cpu low power modes in QCOM
> chipsets.
>
> Signed-off-by: Venkat Devarasetty <vdevaras@codeaurora.org>
> Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>   drivers/soc/qcom/Makefile |   2 +-
>   drivers/soc/qcom/msm-pm.c | 219 ++++++++++++++++++++++++++++++++++++++++++++++
>   include/soc/qcom/pm.h     |  39 +++++++++
>   3 files changed, 259 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/soc/qcom/msm-pm.c
>   create mode 100644 include/soc/qcom/pm.h
>
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index d7ae93b..7925f83 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -1,5 +1,5 @@
>   obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
> -obj-$(CONFIG_QCOM_PM) +=	spm-devices.o spm.o
> +obj-$(CONFIG_QCOM_PM) +=	spm-devices.o spm.o msm-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/msm-pm.c b/drivers/soc/qcom/msm-pm.c
> new file mode 100644
> index 0000000..f2f15b8
> --- /dev/null
> +++ b/drivers/soc/qcom/msm-pm.c
> @@ -0,0 +1,219 @@
> +/* 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/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/smp.h>
> +#include <linux/tick.h>
> +#include <linux/platform_device.h>
> +#include <linux/cpu_pm.h>
> +#include <linux/uaccess.h>
> +
> +#include <soc/qcom/spm.h>
> +#include <soc/qcom/pm.h>
> +#include <soc/qcom/scm.h>
> +#include <soc/qcom/scm-boot.h>
> +
> +#include <asm/suspend.h>
> +#include <asm/cacheflush.h>
> +#include <asm/cputype.h>
> +#include <asm/system_misc.h>
> +
> +#define SCM_CMD_TERMINATE_PC	(0x2)
> +#define SCM_CMD_CORE_HOTPLUGGED (0x10)
> +#define SCM_FLUSH_FLAG_MASK	(0x3)
> +
> +static bool msm_pm_is_L1_writeback(void)
> +{
> +	u32 cache_id = 0;
> +
> +#if defined(CONFIG_CPU_V7)
> +	u32 sel = 0;
> +
> +	asm volatile ("mcr p15, 2, %[ccselr], c0, c0, 0\n\t"
> +		      "isb\n\t"
> +		      "mrc p15, 1, %[ccsidr], c0, c0, 0\n\t"
> +		      :[ccsidr]"=r" (cache_id)
> +		      :[ccselr]"r" (sel)
> +		     );
> +	return cache_id & BIT(30);
> +#elif defined(CONFIG_ARM64)
> +	u32 sel = 0;
> +	asm volatile("msr csselr_el1, %[ccselr]\n\t"
> +		     "isb\n\t"
> +		     "mrs %[ccsidr],ccsidr_el1\n\t"
> +		     :[ccsidr]"=r" (cache_id)
> +		     :[ccselr]"r" (sel)
> +		    );
> +	return cache_id & BIT(30);
> +#else
> +#error No valid CPU arch selected
> +#endif
> +}
> +
> +static inline void msm_arch_idle(void)
> +{
> +	/* Flush and clock-gate */
> +	mb();

Why is needed this memory barrier ?

> +	wfi();
> +}
> +
> +static bool msm_pm_swfi(bool from_idle)
> +{
> +	msm_arch_idle();
> +	return true;
> +}
> +
> +static bool msm_pm_retention(bool from_idle)
> +{
> +	int ret = 0;
> +
> +	ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_RETENTION, false);
> +	WARN_ON(ret);
> +
> +	msm_arch_idle();
> +
> +	ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_CLOCK_GATING, false);
> +	WARN_ON(ret);

Why do you need to set the clock gating mode each time you exit the 
retention mode ?

> +	return true;
> +}
> +
> +static int msm_pm_collapse(unsigned long from_idle)
> +{
> +	enum msm_pm_l2_scm_flag flag = MSM_SCM_L2_ON;
> +
> +	/**
> +	 * Single core processors need to have L2
> +	 * flushed when powering down the core.
> +	 * Notify SCM to flush secure L2 lines.
> +	 */
> +	if (num_possible_cpus() == 1)
> +		flag = MSM_SCM_L2_OFF;

I am wondering if this shouldn't be handle by a mcpm driver.

Cc nico.

> +	if (flag == MSM_SCM_L2_OFF)
> +		flush_cache_all();
> +	else if (msm_pm_is_L1_writeback())
> +		flush_cache_louis();
> +
> +	flag &= SCM_FLUSH_FLAG_MASK;
> +	if (!from_idle)
> +		flag |= SCM_CMD_CORE_HOTPLUGGED;
> +
> +	scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);
> +
> +	return 0;
> +}
> +
> +static void 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;
> +
> +	per_cpu(last_known_entry, cpu) = entry;
> +	scm_set_boot_addr(virt_to_phys(entry), flags[cpu]);
> +}
> +
> +static bool __ref msm_pm_spm_power_collapse(unsigned int cpu, bool from_idle)
> +{
> +	void *entry;
> +	bool collapsed = 0;
> +	int ret;
> +	bool save_cpu_regs = (cpu_online(cpu) || from_idle);
> +
> +	if (from_idle)
> +		cpu_pm_enter();
> +
> +	ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_POWER_COLLAPSE, false);
> +	WARN_ON(ret);
> +
> +	entry = save_cpu_regs ? cpu_resume : secondary_startup;
> +	set_up_boot_address(entry, cpu);
> +
> +#ifdef CONFIG_CPU_V7
> +	collapsed = !cpu_suspend(from_idle, msm_pm_collapse);
> +#else
> +	collapsed = !cpu_suspend(0);
> +#endif
> +
> +	if (collapsed)
> +		local_fiq_enable();
> +
> +	if (from_idle)
> +		cpu_pm_exit();
> +
> +	ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_CLOCK_GATING, false);
> +	WARN_ON(ret);
> +
> +	return collapsed;
> +}
> +
> +static bool msm_pm_power_collapse_standalone(bool from_idle)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	bool collapsed;
> +
> +	collapsed = msm_pm_spm_power_collapse(cpu, from_idle);
> +
> +	return collapsed;
> +}
> +
> +static bool msm_pm_power_collapse(bool from_idle)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	bool collapsed;
> +
> +	collapsed = msm_pm_spm_power_collapse(cpu, from_idle);
> +
> +	return collapsed;
> +}
> +
> +static bool (*execute[MSM_PM_SLEEP_MODE_NR])(bool idle) = {
> +	[MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT] = msm_pm_swfi,
> +	[MSM_PM_SLEEP_MODE_POWER_COLLAPSE_STANDALONE] =
> +		msm_pm_power_collapse_standalone,
> +	[MSM_PM_SLEEP_MODE_RETENTION] = msm_pm_retention,
> +	[MSM_PM_SLEEP_MODE_POWER_COLLAPSE] = msm_pm_power_collapse,
> +};
> +
> +/**
> + * msm_cpu_pm_enter_sleep(): Enter a low power mode on current cpu
> + *
> + * @mode - sleep mode to enter
> + * @from_idle - bool to indicate that the mode is exercised during idle/suspend
> + *
> + * The code should be with interrupts disabled and on the core on which the
> + * low power is to be executed.
> + *
> + */
> +bool msm_cpu_pm_enter_sleep(enum msm_pm_sleep_mode mode, bool from_idle)
> +{
> +	bool exit_stat = false;
> +
> +	if (execute[mode])
> +		exit_stat = execute[mode](from_idle);
> +
> +	local_irq_enable();
> +	return exit_stat;
> +}
> +EXPORT_SYMBOL(msm_cpu_pm_enter_sleep);
> diff --git a/include/soc/qcom/pm.h b/include/soc/qcom/pm.h
> new file mode 100644
> index 0000000..01872ad
> --- /dev/null
> +++ b/include/soc/qcom/pm.h
> @@ -0,0 +1,39 @@
> +/*
> + * 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 msm_pm_sleep_mode {
> +	MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT,
> +	MSM_PM_SLEEP_MODE_RETENTION,
> +	MSM_PM_SLEEP_MODE_POWER_COLLAPSE_STANDALONE,
> +	MSM_PM_SLEEP_MODE_POWER_COLLAPSE,
> +	MSM_PM_SLEEP_MODE_NR,
> +};
> +
> +enum msm_pm_l2_scm_flag {
> +	MSM_SCM_L2_ON = 0,
> +	MSM_SCM_L2_OFF = 1,
> +};
> +
> +#ifdef CONFIG_QCOM_PM
> +bool msm_cpu_pm_enter_sleep(enum msm_pm_sleep_mode mode, bool from_idle);
> +#else
> +static inline bool msm_cpu_pm_enter_sleep(enum msm_pm_sleep_mode mode,
> +						bool from_idle)
> +{ return true; }
> +#endif
> +
> +#endif  /* __QCOM_PM_H */
>

On Wed, Aug 13, 2014 at 01:18:01PM +0200, Daniel Lezcano wrote:
>On 08/12/2014 09:43 PM, Lina Iyer wrote:
>>Add interface layer to abstract and handle hardware specific
>>functionality for executing various cpu low power modes in QCOM
>>chipsets.
>>
>>Signed-off-by: Venkat Devarasetty <vdevaras@codeaurora.org>
>>Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
>>Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>---
>>  drivers/soc/qcom/Makefile |   2 +-
>>  drivers/soc/qcom/msm-pm.c | 219 ++++++++++++++++++++++++++++++++++++++++++++++
>>  include/soc/qcom/pm.h     |  39 +++++++++
>>  3 files changed, 259 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/soc/qcom/msm-pm.c
>>  create mode 100644 include/soc/qcom/pm.h
>>
>>diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>>index d7ae93b..7925f83 100644
>>--- a/drivers/soc/qcom/Makefile
>>+++ b/drivers/soc/qcom/Makefile
>>@@ -1,5 +1,5 @@
>>  obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
>>-obj-$(CONFIG_QCOM_PM) +=	spm-devices.o spm.o
>>+obj-$(CONFIG_QCOM_PM) +=	spm-devices.o spm.o msm-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/msm-pm.c b/drivers/soc/qcom/msm-pm.c
>>new file mode 100644
>>index 0000000..f2f15b8
>>--- /dev/null
>>+++ b/drivers/soc/qcom/msm-pm.c
>>@@ -0,0 +1,219 @@
>>+/* 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/module.h>
>>+#include <linux/kernel.h>
>>+#include <linux/init.h>
>>+#include <linux/io.h>
>>+#include <linux/smp.h>
>>+#include <linux/tick.h>
>>+#include <linux/platform_device.h>
>>+#include <linux/cpu_pm.h>
>>+#include <linux/uaccess.h>
>>+
>>+#include <soc/qcom/spm.h>
>>+#include <soc/qcom/pm.h>
>>+#include <soc/qcom/scm.h>
>>+#include <soc/qcom/scm-boot.h>
>>+
>>+#include <asm/suspend.h>
>>+#include <asm/cacheflush.h>
>>+#include <asm/cputype.h>
>>+#include <asm/system_misc.h>
>>+
>>+#define SCM_CMD_TERMINATE_PC	(0x2)
>>+#define SCM_CMD_CORE_HOTPLUGGED (0x10)
>>+#define SCM_FLUSH_FLAG_MASK	(0x3)
>>+
>>+static bool msm_pm_is_L1_writeback(void)
>>+{
>>+	u32 cache_id = 0;
>>+
>>+#if defined(CONFIG_CPU_V7)
>>+	u32 sel = 0;
>>+
>>+	asm volatile ("mcr p15, 2, %[ccselr], c0, c0, 0\n\t"
>>+		      "isb\n\t"
>>+		      "mrc p15, 1, %[ccsidr], c0, c0, 0\n\t"
>>+		      :[ccsidr]"=r" (cache_id)
>>+		      :[ccselr]"r" (sel)
>>+		     );
>>+	return cache_id & BIT(30);
>>+#elif defined(CONFIG_ARM64)
>>+	u32 sel = 0;
>>+	asm volatile("msr csselr_el1, %[ccselr]\n\t"
>>+		     "isb\n\t"
>>+		     "mrs %[ccsidr],ccsidr_el1\n\t"
>>+		     :[ccsidr]"=r" (cache_id)
>>+		     :[ccselr]"r" (sel)
>>+		    );
>>+	return cache_id & BIT(30);
>>+#else
>>+#error No valid CPU arch selected
>>+#endif
>>+}
>>+
>>+static inline void msm_arch_idle(void)
>>+{
>>+	/* Flush and clock-gate */
>>+	mb();
>
>Why is needed this memory barrier ?
Some QCOM SoCs needed this. I am not sure which one anymore. :(
>
>>+	wfi();
>>+}
>>+
>>+static bool msm_pm_swfi(bool from_idle)
>>+{
>>+	msm_arch_idle();
>>+	return true;
>>+}
>>+
>>+static bool msm_pm_retention(bool from_idle)
>>+{
>>+	int ret = 0;
>>+
>>+	ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_RETENTION, false);
>>+	WARN_ON(ret);
>>+
>>+	msm_arch_idle();
>>+
>>+	ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_CLOCK_GATING, false);
>>+	WARN_ON(ret);
>
>Why do you need to set the clock gating mode each time you exit the 
>retention mode ?
So if the SPM did not reset to clockgating, we would not do retention
when we intended to do clockgating. Btw, we dont set clockgating
everytime we do clockgating, helps reduce the latency in doing WFI.
>
>>+	return true;
>>+}
>>+
>>+static int msm_pm_collapse(unsigned long from_idle)
>>+{
>>+	enum msm_pm_l2_scm_flag flag = MSM_SCM_L2_ON;
>>+
>>+	/**
>>+	 * Single core processors need to have L2
>>+	 * flushed when powering down the core.
>>+	 * Notify SCM to flush secure L2 lines.
>>+	 */
>>+	if (num_possible_cpus() == 1)
>>+		flag = MSM_SCM_L2_OFF;
>
>I am wondering if this shouldn't be handle by a mcpm driver.
>
>Cc nico.

Well, possibly, sorry, not sure what features of the mcpm driver you
think I need here? 


--
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 Wednesday 13 August 2014 01:13 AM, Lina Iyer wrote:
> Add interface layer to abstract and handle hardware specific
> functionality for executing various cpu low power modes in QCOM
> chipsets.
> 
> Signed-off-by: Venkat Devarasetty <vdevaras@codeaurora.org>
> Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>  drivers/soc/qcom/Makefile |   2 +-
>  drivers/soc/qcom/msm-pm.c | 219 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/soc/qcom/pm.h     |  39 +++++++++

<snip>

> +{
> +	u32 cache_id = 0;
> +
> +#if defined(CONFIG_CPU_V7)
> +	u32 sel = 0;
> +
> +	asm volatile ("mcr p15, 2, %[ccselr], c0, c0, 0\n\t"
> +		      "isb\n\t"
> +		      "mrc p15, 1, %[ccsidr], c0, c0, 0\n\t"
> +		      :[ccsidr]"=r" (cache_id)
> +		      :[ccselr]"r" (sel)

Space after ':' is what checkpatch asks. :)

> +		     );
> +	return cache_id & BIT(30);
> +#elif defined(CONFIG_ARM64)
> +	u32 sel = 0;

new line missing after declaration.

> +	asm volatile("msr csselr_el1, %[ccselr]\n\t"
> +		     "isb\n\t"
> +		     "mrs %[ccsidr],ccsidr_el1\n\t"
> +		     :[ccsidr]"=r" (cache_id)
> +		     :[ccselr]"r" (sel)

Space after ':' is what checkpatch asks. :)

> +		    );
> +	return cache_id & BIT(30);
> +#else
> +#error No valid CPU arch selected
> +#endif
> +}
> +

> 
--
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 08/13/2014 04:16 PM, Lina Iyer wrote:
> On Wed, Aug 13, 2014 at 01:18:01PM +0200, Daniel Lezcano wrote:
>> On 08/12/2014 09:43 PM, Lina Iyer wrote:
>>> Add interface layer to abstract and handle hardware specific
>>> functionality for executing various cpu low power modes in QCOM
>>> chipsets.
>>>
>>> Signed-off-by: Venkat Devarasetty <vdevaras@codeaurora.org>
>>> Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>> ---
>>>  drivers/soc/qcom/Makefile |   2 +-
>>>  drivers/soc/qcom/msm-pm.c | 219
>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/soc/qcom/pm.h     |  39 +++++++++
>>>  3 files changed, 259 insertions(+), 1 deletion(-)
>>>  create mode 100644 drivers/soc/qcom/msm-pm.c
>>>  create mode 100644 include/soc/qcom/pm.h
>>>
>>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>>> index d7ae93b..7925f83 100644
>>> --- a/drivers/soc/qcom/Makefile
>>> +++ b/drivers/soc/qcom/Makefile
>>> @@ -1,5 +1,5 @@
>>>  obj-$(CONFIG_QCOM_GSBI)    +=    qcom_gsbi.o
>>> -obj-$(CONFIG_QCOM_PM) +=    spm-devices.o spm.o
>>> +obj-$(CONFIG_QCOM_PM) +=    spm-devices.o spm.o msm-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/msm-pm.c b/drivers/soc/qcom/msm-pm.c
>>> new file mode 100644
>>> index 0000000..f2f15b8
>>> --- /dev/null
>>> +++ b/drivers/soc/qcom/msm-pm.c
>>> @@ -0,0 +1,219 @@
>>> +/* 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/module.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/init.h>
>>> +#include <linux/io.h>
>>> +#include <linux/smp.h>
>>> +#include <linux/tick.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/cpu_pm.h>
>>> +#include <linux/uaccess.h>
>>> +
>>> +#include <soc/qcom/spm.h>
>>> +#include <soc/qcom/pm.h>
>>> +#include <soc/qcom/scm.h>
>>> +#include <soc/qcom/scm-boot.h>
>>> +
>>> +#include <asm/suspend.h>
>>> +#include <asm/cacheflush.h>
>>> +#include <asm/cputype.h>
>>> +#include <asm/system_misc.h>
>>> +
>>> +#define SCM_CMD_TERMINATE_PC    (0x2)
>>> +#define SCM_CMD_CORE_HOTPLUGGED (0x10)
>>> +#define SCM_FLUSH_FLAG_MASK    (0x3)
>>> +
>>> +static bool msm_pm_is_L1_writeback(void)
>>> +{
>>> +    u32 cache_id = 0;
>>> +
>>> +#if defined(CONFIG_CPU_V7)
>>> +    u32 sel = 0;
>>> +
>>> +    asm volatile ("mcr p15, 2, %[ccselr], c0, c0, 0\n\t"
>>> +              "isb\n\t"
>>> +              "mrc p15, 1, %[ccsidr], c0, c0, 0\n\t"
>>> +              :[ccsidr]"=r" (cache_id)
>>> +              :[ccselr]"r" (sel)
>>> +             );
>>> +    return cache_id & BIT(30);
>>> +#elif defined(CONFIG_ARM64)
>>> +    u32 sel = 0;
>>> +    asm volatile("msr csselr_el1, %[ccselr]\n\t"
>>> +             "isb\n\t"
>>> +             "mrs %[ccsidr],ccsidr_el1\n\t"
>>> +             :[ccsidr]"=r" (cache_id)
>>> +             :[ccselr]"r" (sel)
>>> +            );
>>> +    return cache_id & BIT(30);
>>> +#else
>>> +#error No valid CPU arch selected
>>> +#endif
>>> +}
>>> +
>>> +static inline void msm_arch_idle(void)
>>> +{
>>> +    /* Flush and clock-gate */
>>> +    mb();
>>
>> Why is needed this memory barrier ?
> Some QCOM SoCs needed this. I am not sure which one anymore. :(
>>
>>> +    wfi();
>>> +}
>>> +
>>> +static bool msm_pm_swfi(bool from_idle)
>>> +{
>>> +    msm_arch_idle();
>>> +    return true;
>>> +}
>>> +
>>> +static bool msm_pm_retention(bool from_idle)
>>> +{
>>> +    int ret = 0;
>>> +
>>> +    ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_RETENTION, false);
>>> +    WARN_ON(ret);
>>> +
>>> +    msm_arch_idle();
>>> +
>>> +    ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_CLOCK_GATING, false);
>>> +    WARN_ON(ret);
>>
>> Why do you need to set the clock gating mode each time you exit the
>> retention mode ?
> So if the SPM did not reset to clockgating, we would not do retention
> when we intended to do clockgating. Btw, we dont set clockgating
> everytime we do clockgating, helps reduce the latency in doing WFI.

Can you elaborate ? Or may be just describe what is the doing the 
function because I don't get the connection between your explanation and 
the code.

>>> +    return true;
>>> +}
>>> +
>>> +static int msm_pm_collapse(unsigned long from_idle)
>>> +{
>>> +    enum msm_pm_l2_scm_flag flag = MSM_SCM_L2_ON;
>>> +
>>> +    /**
>>> +     * Single core processors need to have L2
>>> +     * flushed when powering down the core.
>>> +     * Notify SCM to flush secure L2 lines.
>>> +     */
>>> +    if (num_possible_cpus() == 1)
>>> +        flag = MSM_SCM_L2_OFF;
>>
>> I am wondering if this shouldn't be handle by a mcpm driver.
>>
>> Cc nico.
>
> Well, possibly, sorry, not sure what features of the mcpm driver you
> think I need here?

Please correct me if I am wrong. IIUC, this function is checking the 
number of the cpus of the cluster in order to flush the L2 cache because 
the SCM will power down the cluster if it is the last one, right ?

On Thu, Aug 14, 2014 at 07:08:21PM +0530, Pramod Gurav wrote:
>On Wednesday 13 August 2014 01:13 AM, Lina Iyer wrote:
>> Add interface layer to abstract and handle hardware specific
>> functionality for executing various cpu low power modes in QCOM
>> chipsets.
>>
>> Signed-off-by: Venkat Devarasetty <vdevaras@codeaurora.org>
>> Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
>>  drivers/soc/qcom/Makefile |   2 +-
>>  drivers/soc/qcom/msm-pm.c | 219 ++++++++++++++++++++++++++++++++++++++++++++++
>>  include/soc/qcom/pm.h     |  39 +++++++++
>
><snip>
>
>> +{
>> +	u32 cache_id = 0;
>> +
>> +#if defined(CONFIG_CPU_V7)
>> +	u32 sel = 0;
>> +
>> +	asm volatile ("mcr p15, 2, %[ccselr], c0, c0, 0\n\t"
>> +		      "isb\n\t"
>> +		      "mrc p15, 1, %[ccsidr], c0, c0, 0\n\t"
>> +		      :[ccsidr]"=r" (cache_id)
>> +		      :[ccselr]"r" (sel)
>
>Space after ':' is what checkpatch asks. :)
See below
>
>> +		     );
>> +	return cache_id & BIT(30);
>> +#elif defined(CONFIG_ARM64)
>> +	u32 sel = 0;
>
>new line missing after declaration.
Ok.
>
>> +	asm volatile("msr csselr_el1, %[ccselr]\n\t"
>> +		     "isb\n\t"
>> +		     "mrs %[ccsidr],ccsidr_el1\n\t"
>> +		     :[ccsidr]"=r" (cache_id)
>> +		     :[ccselr]"r" (sel)
>
>Space after ':' is what checkpatch asks. :)
Yes, checkpatch seems to complain eitherways, whether you have a space
or not. The advice was to ignore it.

>
>> +		    );
>> +	return cache_id & BIT(30);
>> +#else
>> +#error No valid CPU arch selected
>> +#endif
>> +}
>> +
>
>>
--
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, Aug 14, 2014 at 04:24:10PM +0200, Daniel Lezcano wrote:
>On 08/13/2014 04:16 PM, Lina Iyer wrote:
>>On Wed, Aug 13, 2014 at 01:18:01PM +0200, Daniel Lezcano wrote:
>>>On 08/12/2014 09:43 PM, Lina Iyer wrote:

>>>+static bool msm_pm_retention(bool from_idle)
>>>>+{
>>>>+    int ret = 0;
>>>>+
>>>>+    ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_RETENTION, false);
>>>>+    WARN_ON(ret);
>>>>+
>>>>+    msm_arch_idle();
>>>>+
>>>>+    ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_CLOCK_GATING, false);
>>>>+    WARN_ON(ret);
>>>
>>>Why do you need to set the clock gating mode each time you exit the
>>>retention mode ?
>>So if the SPM did not reset to clockgating, we would not do retention
>>when we intended to do clockgating. Btw, we dont set clockgating
>>everytime we do clockgating, helps reduce the latency in doing WFI.
>
>Can you elaborate ? Or may be just describe what is the doing the 
>function because I don't get the connection between your explanation 
>and the code.
>

Retention still has an higher latency than clock gating. Retention gets
triggered with the core executes wfi() instruction. The entry into a
steady state retention mode is higher than just gating clocks, which may
be bad for power, if we do not stay in the low power modes for a minimum
of the residency period.

If the current idle state was retention and SPM was configured to do
retention and we came out the retention state and the next idle, we
decided to do clockgating we should configure the SPM to do clock
gating. Since we want to speed up clockgating as its one of the state
most commonly entered, we set the configuration of SPM as soon as we
come out any low power mode back to WFI.

>>>>+    return true;
>>>>+}
>>>>+
>>>>+static int msm_pm_collapse(unsigned long from_idle)
>>>>+{
>>>>+    enum msm_pm_l2_scm_flag flag = MSM_SCM_L2_ON;
>>>>+
>>>>+    /**
>>>>+     * Single core processors need to have L2
>>>>+     * flushed when powering down the core.
>>>>+     * Notify SCM to flush secure L2 lines.
>>>>+     */
>>>>+    if (num_possible_cpus() == 1)
>>>>+        flag = MSM_SCM_L2_OFF;
>>>
>>>I am wondering if this shouldn't be handle by a mcpm driver.
>>>
>>>Cc nico.
>>
>>Well, possibly, sorry, not sure what features of the mcpm driver you
>>think I need here?
>
>Please correct me if I am wrong. IIUC, this function is checking the 
>number of the cpus of the cluster in order to flush the L2 cache 
>because the SCM will power down the cluster if it is the last one, 
>right ?
>
>
>
>-- 
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
>Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
><http://twitter.com/#!/linaroorg> Twitter |
><http://www.linaro.org/linaro-blog/> Blog
>
--
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 08/13/2014 04:16 PM, Lina Iyer wrote:
> On Wed, Aug 13, 2014 at 01:18:01PM +0200, Daniel Lezcano wrote:
>> On 08/12/2014 09:43 PM, Lina Iyer wrote:
>>> Add interface layer to abstract and handle hardware specific
>>> functionality for executing various cpu low power modes in QCOM
>>> chipsets.
>>>
>>> Signed-off-by: Venkat Devarasetty <vdevaras@codeaurora.org>
>>> Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>> ---
>>>  drivers/soc/qcom/Makefile |   2 +-
>>>  drivers/soc/qcom/msm-pm.c | 219
>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/soc/qcom/pm.h     |  39 +++++++++
>>>  3 files changed, 259 insertions(+), 1 deletion(-)
>>>  create mode 100644 drivers/soc/qcom/msm-pm.c
>>>  create mode 100644 include/soc/qcom/pm.h
>>>
>>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>>> index d7ae93b..7925f83 100644
>>> --- a/drivers/soc/qcom/Makefile
>>> +++ b/drivers/soc/qcom/Makefile
>>> @@ -1,5 +1,5 @@
>>>  obj-$(CONFIG_QCOM_GSBI)    +=    qcom_gsbi.o
>>> -obj-$(CONFIG_QCOM_PM) +=    spm-devices.o spm.o
>>> +obj-$(CONFIG_QCOM_PM) +=    spm-devices.o spm.o msm-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/msm-pm.c b/drivers/soc/qcom/msm-pm.c
>>> new file mode 100644
>>> index 0000000..f2f15b8
>>> --- /dev/null
>>> +++ b/drivers/soc/qcom/msm-pm.c
>>> @@ -0,0 +1,219 @@
>>> +/* 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/module.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/init.h>
>>> +#include <linux/io.h>
>>> +#include <linux/smp.h>
>>> +#include <linux/tick.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/cpu_pm.h>
>>> +#include <linux/uaccess.h>
>>> +
>>> +#include <soc/qcom/spm.h>
>>> +#include <soc/qcom/pm.h>
>>> +#include <soc/qcom/scm.h>
>>> +#include <soc/qcom/scm-boot.h>
>>> +
>>> +#include <asm/suspend.h>
>>> +#include <asm/cacheflush.h>
>>> +#include <asm/cputype.h>
>>> +#include <asm/system_misc.h>
>>> +
>>> +#define SCM_CMD_TERMINATE_PC    (0x2)
>>> +#define SCM_CMD_CORE_HOTPLUGGED (0x10)
>>> +#define SCM_FLUSH_FLAG_MASK    (0x3)
>>> +
>>> +static bool msm_pm_is_L1_writeback(void)
>>> +{
>>> +    u32 cache_id = 0;
>>> +
>>> +#if defined(CONFIG_CPU_V7)
>>> +    u32 sel = 0;
>>> +
>>> +    asm volatile ("mcr p15, 2, %[ccselr], c0, c0, 0\n\t"
>>> +              "isb\n\t"
>>> +              "mrc p15, 1, %[ccsidr], c0, c0, 0\n\t"
>>> +              :[ccsidr]"=r" (cache_id)
>>> +              :[ccselr]"r" (sel)
>>> +             );
>>> +    return cache_id & BIT(30);
>>> +#elif defined(CONFIG_ARM64)
>>> +    u32 sel = 0;
>>> +    asm volatile("msr csselr_el1, %[ccselr]\n\t"
>>> +             "isb\n\t"
>>> +             "mrs %[ccsidr],ccsidr_el1\n\t"
>>> +             :[ccsidr]"=r" (cache_id)
>>> +             :[ccselr]"r" (sel)
>>> +            );
>>> +    return cache_id & BIT(30);
>>> +#else
>>> +#error No valid CPU arch selected
>>> +#endif
>>> +}
>>> +
>>> +static inline void msm_arch_idle(void)
>>> +{
>>> +    /* Flush and clock-gate */
>>> +    mb();
>>
>> Why is needed this memory barrier ?
> Some QCOM SoCs needed this. I am not sure which one anymore. :(

I guess this is to flush the L1 cache when the core is going down. 
Regarding the kernel option, it seems mb() is as dsb(), so I am 
wondering if this function could be simply replaced by cpu_do_idle().

>>> +    wfi();
>>> +}
>>> +
>>> +static bool msm_pm_swfi(bool from_idle)
>>> +{
>>> +    msm_arch_idle();
>>> +    return true;
>>> +}

Same here, could be replaced by cpu_do_idle(), I think.

>>> +static bool msm_pm_retention(bool from_idle)
>>> +{
>>> +    int ret = 0;
>>> +
>>> +    ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_RETENTION, false);
>>> +    WARN_ON(ret);
>>> +
>>> +    msm_arch_idle();
>>> +
>>> +    ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_CLOCK_GATING, false);
>>> +    WARN_ON(ret);
>>
>> Why do you need to set the clock gating mode each time you exit the
>> retention mode ?
> So if the SPM did not reset to clockgating, we would not do retention
> when we intended to do clockgating. Btw, we dont set clockgating
> everytime we do clockgating, helps reduce the latency in doing WFI.

Thanks for the explanation in the other email. So IIUC, the SCM keeps 
the last state configuration and we have to set it back to clock gating, 
right ?

I don't think it is up to this function to do this but the clock gating 
function.

Also, this function prototype looks a bit weird. Just for the sake of 
using callbacks.

And finally, the WARN_ON is not desirable here, except if the goal is to 
flood the terminal :)

What not using first simple functions ?

void qcom_do_idle(void)
{
	myfirmware_call(MSM_SPM_MODE_CLOCK_GATING);
	wfi();
}

void qcom_cpu_retention(void)
{
	myfirmware_call(MSM_SPM_MODE_RETENTION);
	dsb();
	wfi();
}

void qcom_cpu_powerdown(int flags)
{
	scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);
}

and then you build on top of that the cpuidle driver.

The patchset adds all the features in one shot and for someone not used 
with the platform it is really hard to follow all the code.

I suggest you write a simple cpuidle driver based on the DT Lorenzo 
patches bringing the clock gating, then another patchset with the 
retention mode, etc ...

>>> +    return true;
>>> +}
>>> +
>>> +static int msm_pm_collapse(unsigned long from_idle)
>>> +{
>>> +    enum msm_pm_l2_scm_flag flag = MSM_SCM_L2_ON;
>>> +
>>> +    /**
>>> +     * Single core processors need to have L2
>>> +     * flushed when powering down the core.
>>> +     * Notify SCM to flush secure L2 lines.
>>> +     */
>>> +    if (num_possible_cpus() == 1)
>>> +        flag = MSM_SCM_L2_OFF;
>>
>> I am wondering if this shouldn't be handle by a mcpm driver.
>>
>> Cc nico.
>
> Well, possibly, sorry, not sure what features of the mcpm driver you
> think I need here?

Please correct me if I am wrong. IIUC, this function is checking the
number of the cpus of the cluster in order to flush the L2 cache
because the SCM will power down the cluster if it is the last one,
right ?

On Thu, Aug 14, 2014 at 06:11:43PM +0200, Daniel Lezcano wrote:
>On 08/13/2014 04:16 PM, Lina Iyer wrote:
>>On Wed, Aug 13, 2014 at 01:18:01PM +0200, Daniel Lezcano wrote:
>>>On 08/12/2014 09:43 PM, Lina Iyer wrote:
>>>>Add interface layer to abstract and handle hardware specific
>>>>functionality for executing various cpu low power modes in QCOM
>>>>chipsets.
>>>>
>>>>Signed-off-by: Venkat Devarasetty <vdevaras@codeaurora.org>
>>>>Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
>>>>Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>>>---
>>>> drivers/soc/qcom/Makefile |   2 +-
>>>> drivers/soc/qcom/msm-pm.c | 219
>>>>++++++++++++++++++++++++++++++++++++++++++++++
>>>> include/soc/qcom/pm.h     |  39 +++++++++
>>>> 3 files changed, 259 insertions(+), 1 deletion(-)
>>>> create mode 100644 drivers/soc/qcom/msm-pm.c
>>>> create mode 100644 include/soc/qcom/pm.h
>>>>
>>>>diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>>>>index d7ae93b..7925f83 100644
>>>>--- a/drivers/soc/qcom/Makefile
>>>>+++ b/drivers/soc/qcom/Makefile
>>>>@@ -1,5 +1,5 @@
>>>> obj-$(CONFIG_QCOM_GSBI)    +=    qcom_gsbi.o
>>>>-obj-$(CONFIG_QCOM_PM) +=    spm-devices.o spm.o
>>>>+obj-$(CONFIG_QCOM_PM) +=    spm-devices.o spm.o msm-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/msm-pm.c b/drivers/soc/qcom/msm-pm.c
>>>>new file mode 100644
>>>>index 0000000..f2f15b8
>>>>--- /dev/null
>>>>+++ b/drivers/soc/qcom/msm-pm.c
>>>>@@ -0,0 +1,219 @@
>>>>+/* 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/module.h>
>>>>+#include <linux/kernel.h>
>>>>+#include <linux/init.h>
>>>>+#include <linux/io.h>
>>>>+#include <linux/smp.h>
>>>>+#include <linux/tick.h>
>>>>+#include <linux/platform_device.h>
>>>>+#include <linux/cpu_pm.h>
>>>>+#include <linux/uaccess.h>
>>>>+
>>>>+#include <soc/qcom/spm.h>
>>>>+#include <soc/qcom/pm.h>
>>>>+#include <soc/qcom/scm.h>
>>>>+#include <soc/qcom/scm-boot.h>
>>>>+
>>>>+#include <asm/suspend.h>
>>>>+#include <asm/cacheflush.h>
>>>>+#include <asm/cputype.h>
>>>>+#include <asm/system_misc.h>
>>>>+
>>>>+#define SCM_CMD_TERMINATE_PC    (0x2)
>>>>+#define SCM_CMD_CORE_HOTPLUGGED (0x10)
>>>>+#define SCM_FLUSH_FLAG_MASK    (0x3)
>>>>+
>>>>+static bool msm_pm_is_L1_writeback(void)
>>>>+{
>>>>+    u32 cache_id = 0;
>>>>+
>>>>+#if defined(CONFIG_CPU_V7)
>>>>+    u32 sel = 0;
>>>>+
>>>>+    asm volatile ("mcr p15, 2, %[ccselr], c0, c0, 0\n\t"
>>>>+              "isb\n\t"
>>>>+              "mrc p15, 1, %[ccsidr], c0, c0, 0\n\t"
>>>>+              :[ccsidr]"=r" (cache_id)
>>>>+              :[ccselr]"r" (sel)
>>>>+             );
>>>>+    return cache_id & BIT(30);
>>>>+#elif defined(CONFIG_ARM64)
>>>>+    u32 sel = 0;
>>>>+    asm volatile("msr csselr_el1, %[ccselr]\n\t"
>>>>+             "isb\n\t"
>>>>+             "mrs %[ccsidr],ccsidr_el1\n\t"
>>>>+             :[ccsidr]"=r" (cache_id)
>>>>+             :[ccselr]"r" (sel)
>>>>+            );
>>>>+    return cache_id & BIT(30);
>>>>+#else
>>>>+#error No valid CPU arch selected
>>>>+#endif
>>>>+}
>>>>+
>>>>+static inline void msm_arch_idle(void)
>>>>+{
>>>>+    /* Flush and clock-gate */
>>>>+    mb();
>>>
>>>Why is needed this memory barrier ?
>>Some QCOM SoCs needed this. I am not sure which one anymore. :(
>
>I guess this is to flush the L1 cache when the core is going down. 
>Regarding the kernel option, it seems mb() is as dsb(), so I am 
>wondering if this function could be simply replaced by cpu_do_idle().
>
Possibly could. I will do that.

>>>>+    wfi();
>>>>+}
>>>>+
>>>>+static bool msm_pm_swfi(bool from_idle)
>>>>+{
>>>>+    msm_arch_idle();
>>>>+    return true;
>>>>+}
>
>Same here, could be replaced by cpu_do_idle(), I think.
>
>>>>+static bool msm_pm_retention(bool from_idle)
>>>>+{
>>>>+    int ret = 0;
>>>>+
>>>>+    ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_RETENTION, false);
>>>>+    WARN_ON(ret);
>>>>+
>>>>+    msm_arch_idle();
>>>>+
>>>>+    ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_CLOCK_GATING, false);
>>>>+    WARN_ON(ret);
>>>
>>>Why do you need to set the clock gating mode each time you exit the
>>>retention mode ?
>>So if the SPM did not reset to clockgating, we would not do retention
>>when we intended to do clockgating. Btw, we dont set clockgating
>>everytime we do clockgating, helps reduce the latency in doing WFI.
>
>Thanks for the explanation in the other email. So IIUC, the SCM keeps 
>the last state configuration and we have to set it back to clock 
>gating, right ?
Correct.
>
>I don't think it is up to this function to do this but the clock 
>gating function.
>
>Also, this function prototype looks a bit weird. Just for the sake of 
>using callbacks.
>
>And finally, the WARN_ON is not desirable here, except if the goal is 
>to flood the terminal :)
Was debating the use of it myself. Will remove it.
>
>What not using first simple functions ?
>
>void qcom_do_idle(void)
>{
>	myfirmware_call(MSM_SPM_MODE_CLOCK_GATING);
>	wfi();
>}
>
>void qcom_cpu_retention(void)
>{
>	myfirmware_call(MSM_SPM_MODE_RETENTION);
>	dsb();
>	wfi();
>}
>
>void qcom_cpu_powerdown(int flags)
>{
>	scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);
>}
>
>and then you build on top of that the cpuidle driver.
Okay. Will do this
>
>The patchset adds all the features in one shot and for someone not 
>used with the platform it is really hard to follow all the code.
>
>I suggest you write a simple cpuidle driver based on the DT Lorenzo 
>patches bringing the clock gating, then another patchset with the 
>retention mode, etc ...
>
>>>>+    return true;
>>>>+}
>>>>+
>>>>+static int msm_pm_collapse(unsigned long from_idle)
>>>>+{
>>>>+    enum msm_pm_l2_scm_flag flag = MSM_SCM_L2_ON;
>>>>+
>>>>+    /**
>>>>+     * Single core processors need to have L2
>>>>+     * flushed when powering down the core.
>>>>+     * Notify SCM to flush secure L2 lines.
>>>>+     */
>>>>+    if (num_possible_cpus() == 1)
>>>>+        flag = MSM_SCM_L2_OFF;
>>>
>>>I am wondering if this shouldn't be handle by a mcpm driver.
>>>
>>>Cc nico.
>>
>>Well, possibly, sorry, not sure what features of the mcpm driver you
>>think I need here?
>
>Please correct me if I am wrong. IIUC, this function is checking the
>number of the cpus of the cluster in order to flush the L2 cache
>because the SCM will power down the cluster if it is the last one,
>right ?
Nope. Some QCOM variants which have a single CPU, cannot be powered down
without flushing the caches. Warm boot of the cpu resets the L2
logic as well. The cluster core is lot more complex than this :)

>
>
>-- 
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
>Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
><http://twitter.com/#!/linaroorg> Twitter |
><http://www.linaro.org/linaro-blog/> Blog
>
--
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 08/14/2014 09:22 PM, Lina Iyer wrote:
> On Thu, Aug 14, 2014 at 06:11:43PM +0200, Daniel Lezcano wrote:
>> On 08/13/2014 04:16 PM, Lina Iyer wrote:
>>> On Wed, Aug 13, 2014 at 01:18:01PM +0200, Daniel Lezcano wrote:
>>>> On 08/12/2014 09:43 PM, Lina Iyer wrote:
>>>>> Add interface layer to abstract and handle hardware specific
>>>>> functionality for executing various cpu low power modes in QCOM
>>>>> chipsets.
>>>>>
>>>>> Signed-off-by: Venkat Devarasetty <vdevaras@codeaurora.org>
>>>>> Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
>>>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>>>> ---

[ ... ]

>>>>> +static bool msm_pm_retention(bool from_idle)
>>>>> +{
>>>>> +    int ret = 0;
>>>>> +
>>>>> +    ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_RETENTION, false);
>>>>> +    WARN_ON(ret);
>>>>> +
>>>>> +    msm_arch_idle();
>>>>> +
>>>>> +    ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_CLOCK_GATING,
>>>>> false);
>>>>> +    WARN_ON(ret);
>>>>
>>>> Why do you need to set the clock gating mode each time you exit the
>>>> retention mode ?
>>> So if the SPM did not reset to clockgating, we would not do retention
>>> when we intended to do clockgating. Btw, we dont set clockgating
>>> everytime we do clockgating, helps reduce the latency in doing WFI.
>>
>> Thanks for the explanation in the other email. So IIUC, the SCM keeps
>> the last state configuration and we have to set it back to clock
>> gating, right ?
> Correct.
>>
>> I don't think it is up to this function to do this but the clock
>> gating function.
>>
>> Also, this function prototype looks a bit weird. Just for the sake of
>> using callbacks.
>>
>> And finally, the WARN_ON is not desirable here, except if the goal is
>> to flood the terminal :)
> Was debating the use of it myself. Will remove it.
>>
>> What not using first simple functions ?
>>
>> void qcom_do_idle(void)
>> {
>>     myfirmware_call(MSM_SPM_MODE_CLOCK_GATING);
>>     wfi();
>> }
>>
>> void qcom_cpu_retention(void)
>> {
>>     myfirmware_call(MSM_SPM_MODE_RETENTION);
>>     dsb();
>>     wfi();
>> }
>>
>> void qcom_cpu_powerdown(int flags)
>> {
>>     scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);
>> }
>>
>> and then you build on top of that the cpuidle driver.
> Okay. Will do this
>>
>> The patchset adds all the features in one shot and for someone not
>> used with the platform it is really hard to follow all the code.
>>
>> I suggest you write a simple cpuidle driver based on the DT Lorenzo
>> patches bringing the clock gating, then another patchset with the
>> retention mode, etc ...
 >>

Do you agree with this approach ?

>>>>> +    return true;
>>>>> +}
>>>>> +
>>>>> +static int msm_pm_collapse(unsigned long from_idle)
>>>>> +{
>>>>> +    enum msm_pm_l2_scm_flag flag = MSM_SCM_L2_ON;
>>>>> +
>>>>> +    /**
>>>>> +     * Single core processors need to have L2
>>>>> +     * flushed when powering down the core.
>>>>> +     * Notify SCM to flush secure L2 lines.
>>>>> +     */
>>>>> +    if (num_possible_cpus() == 1)
>>>>> +        flag = MSM_SCM_L2_OFF;
>>>>
>>>> I am wondering if this shouldn't be handle by a mcpm driver.
>>>>
>>>> Cc nico.
>>>
>>> Well, possibly, sorry, not sure what features of the mcpm driver you
>>> think I need here?
>>
>> Please correct me if I am wrong. IIUC, this function is checking the
>> number of the cpus of the cluster in order to flush the L2 cache
>> because the SCM will power down the cluster if it is the last one,
>> right ?
> Nope. Some QCOM variants which have a single CPU, cannot be powered down
> without flushing the caches. Warm boot of the cpu resets the L2
> logic as well. The cluster core is lot more complex than this :)

Ok, probably we can discuss this later when we reach this state in the 
incremental implementation.

Thanks

   -- Daniel

>> --
>> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>
>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>> <http://twitter.com/#!/linaroorg> Twitter |
>> <http://www.linaro.org/linaro-blog/> Blog
>>

On Fri, Aug 15, 2014 at 02:01:47AM +0200, Daniel Lezcano wrote:
>On 08/14/2014 09:22 PM, Lina Iyer wrote:
>>On Thu, Aug 14, 2014 at 06:11:43PM +0200, Daniel Lezcano wrote:
>>>On 08/13/2014 04:16 PM, Lina Iyer wrote:
>>>>On Wed, Aug 13, 2014 at 01:18:01PM +0200, Daniel Lezcano wrote:
>>>>>On 08/12/2014 09:43 PM, Lina Iyer wrote:
>>>>>>Add interface layer to abstract and handle hardware specific
>>>>>>functionality for executing various cpu low power modes in QCOM
>>>>>>chipsets.
>>>>>>
>>>>>>Signed-off-by: Venkat Devarasetty <vdevaras@codeaurora.org>
>>>>>>Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
>>>>>>Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>>>>>---
>
>[ ... ]
>
>>>>>>+static bool msm_pm_retention(bool from_idle)
>>>>>>+{
>>>>>>+    int ret = 0;
>>>>>>+
>>>>>>+    ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_RETENTION, false);
>>>>>>+    WARN_ON(ret);
>>>>>>+
>>>>>>+    msm_arch_idle();
>>>>>>+
>>>>>>+    ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_CLOCK_GATING,
>>>>>>false);
>>>>>>+    WARN_ON(ret);
>>>>>
>>>>>Why do you need to set the clock gating mode each time you exit the
>>>>>retention mode ?
>>>>So if the SPM did not reset to clockgating, we would not do retention
>>>>when we intended to do clockgating. Btw, we dont set clockgating
>>>>everytime we do clockgating, helps reduce the latency in doing WFI.
>>>
>>>Thanks for the explanation in the other email. So IIUC, the SCM keeps
>>>the last state configuration and we have to set it back to clock
>>>gating, right ?
>>Correct.
>>>
>>>I don't think it is up to this function to do this but the clock
>>>gating function.
>>>
>>>Also, this function prototype looks a bit weird. Just for the sake of
>>>using callbacks.
>>>
>>>And finally, the WARN_ON is not desirable here, except if the goal is
>>>to flood the terminal :)
>>Was debating the use of it myself. Will remove it.
>>>
>>>What not using first simple functions ?
>>>
>>>void qcom_do_idle(void)
>>>{
>>>    myfirmware_call(MSM_SPM_MODE_CLOCK_GATING);
>>>    wfi();
>>>}
>>>
>>>void qcom_cpu_retention(void)
>>>{
>>>    myfirmware_call(MSM_SPM_MODE_RETENTION);
>>>    dsb();
>>>    wfi();
>>>}
>>>
>>>void qcom_cpu_powerdown(int flags)
>>>{
>>>    scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);
>>>}
>>>
>>>and then you build on top of that the cpuidle driver.
>>Okay. Will do this
>>>
>>>The patchset adds all the features in one shot and for someone not
>>>used with the platform it is really hard to follow all the code.
>>>
>>>I suggest you write a simple cpuidle driver based on the DT Lorenzo
>>>patches bringing the clock gating, then another patchset with the
>>>retention mode, etc ...
>>>
>
>Do you agree with this approach ?
Yes, have a working patch with the code cleaned out as you suggested.
Looking at Lorenzo's DT changes and a few SPM driver clean ups. Will
submit a patch revision soon.

>
>>>>>>+    return true;
>>>>>>+}
>>>>>>+
>>>>>>+static int msm_pm_collapse(unsigned long from_idle)
>>>>>>+{
>>>>>>+    enum msm_pm_l2_scm_flag flag = MSM_SCM_L2_ON;
>>>>>>+
>>>>>>+    /**
>>>>>>+     * Single core processors need to have L2
>>>>>>+     * flushed when powering down the core.
>>>>>>+     * Notify SCM to flush secure L2 lines.
>>>>>>+     */
>>>>>>+    if (num_possible_cpus() == 1)
>>>>>>+        flag = MSM_SCM_L2_OFF;
>>>>>
>>>>>I am wondering if this shouldn't be handle by a mcpm driver.
>>>>>
>>>>>Cc nico.
>>>>
>>>>Well, possibly, sorry, not sure what features of the mcpm driver you
>>>>think I need here?
>>>
>>>Please correct me if I am wrong. IIUC, this function is checking the
>>>number of the cpus of the cluster in order to flush the L2 cache
>>>because the SCM will power down the cluster if it is the last one,
>>>right ?
>>Nope. Some QCOM variants which have a single CPU, cannot be powered down
>>without flushing the caches. Warm boot of the cpu resets the L2
>>logic as well. The cluster core is lot more complex than this :)
>
>Ok, probably we can discuss this later when we reach this state in the 
>incremental implementation.
Hmm, yeah.

>
>Thanks
>
>  -- Daniel
>
>>>--
>>><http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>>
>>>Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>>><http://twitter.com/#!/linaroorg> Twitter |
>>><http://www.linaro.org/linaro-blog/> Blog
>>>
>
>
>-- 
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
>Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
><http://twitter.com/#!/linaroorg> Twitter |
><http://www.linaro.org/linaro-blog/> Blog
>
--
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