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

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

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


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