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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

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 ?



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