Re: 答复: [PATCH] cpuidle: sirf : Add cpuidle for SiRFprimaII and SiRFatlasVI

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

On 10/10/2013 12:24 PM, Barry Song wrote:
> 2013/10/9 Daniel Lezcano <daniel.lezcano@linaro.org>:
>> On 10/08/2013 03:27 PM, Barry Song wrote:
>>>>>>>
>>>>>>> +       if (sirf_cpuidle.vcore_regulator)
>>>>>>> +               regulator_set_voltage(sirf_cpuidle.vcore_regulator,
>>>>>>> volt_old,
>>>>>>> +                               SIRFSOC_MAX_VOLTAGE);
>>>>>>> +
>>>>>>> +       clk_set_parent(sirf_cpuidle.cpu_clk, parent_clk);
>>>>>>> +       /* Todo: other C states */
>>>>>>
>>>>>>
>>>>>>
>>>>>> It sounds very weird you have this freq/volt/opp code here.
>>>>>>
>>>>>> If you hit idle, the cpufreq driver didn't put the cpu in the state you
>>>>>> are trying to
>>>>>> bring here ?
>>>>>>
>>>>>> There isn't a power management unit on this board ?
>>>>>>
>>>>>
>>>>> By SiRF SoC, when enter idle mode, I set the freq to 26MHz and min
>>>>> voltage.
>>>>> That's will save the power consume and reduce leakage current.
>>>>
>>>>
>>>>
>>>> Really ? :)
>>>>
>>>> You didn't answer the questions.
>>>>
>>>> 1. this code shouldn't be here but in cpufreq which is already the case
>>>> with
>>>> the other patch you sent with this one. So I am convinced if you remove
>>>> the
>>>> OPP here and let cpufreq to handle that, and keep the cpu_do_idle, you
>>>> won't
>>>> see any difference with the current code.
>>>>
>>>> 2. Moreover, removing the OPP, there is only the WFI state which is the
>>>> default idle function. Hence the cpuidle driver itself has no interest
>>>> and
>>>> you can simply remove it, except if you bring another idle state with the
>>>> driver.
>>>>
>>>> 3. More idle states are often handled through a PMU (Power Management
>>>> Unit)
>>>> giving cpu power off, retention, cluster power down, etc ... Is there one
>>>> on
>>>> this board ?
>>>>
>>>> The TRM can help to answer these questions.
>>>>
>>> Hi Daniel,
>>> here the codes actually want to put cpu into lowest freq and lowest
>>> voltage. because according to our test, power leakage will be much
>>> lower when cpu is in the status than we put it into WFI directly in a
>>> higher freq.
>>>
>>> here it is difficult to say cpufreq will do that automatically as it
>>> is decided by cpufreq policy.
>>>
>>> there is no power management unit which can put prima2 into a
>>> different idle status except WFI.
>>>
>>> any suggestion from you about this chip issue?
>>
>>
>> Not so much without further information. Definitively without the TRM it is
>> hard to give any hints on how to go further.
>>
>> Do you have the ability to do RAM self refresh ? If not, without a power
>> management unit on your board, in my opinion you can drop the cpuidle driver
>> because it means you can't do retention or poweroff the core.
>
> if dropping the cpuidle, actually the only left mode is WFI. but here
> the problem is that WFI is not enough for us due to our chips have
> some current leak in WFI. so here the driver is specific to sirf to
> fix the current leak in WFI.
>
> and i also don't think cpufreq will put hardware to low vol and low
> freq automatically. at least for performance and user, it is
> impossible.

Yes and that is the proof the OPP in cpuidle violates the cpufreq 
governor decision. Let's pick an example:

I don't know your board so I will assume we have 300, 600, 900 MHz.

The user set the policy to 'userspace' and sets the freq to 600MHz, the 
cpuidle will change that to 900MHz when exiting idle.

The user set the policy to 'performance', cpuidle switch to 300MHz 
despite the cpufreq governor told 900MHz.

The user set the policy to 'ondemand', the cpufreq governor set the OPP 
to 600MHz but cpuidle change it to 300MHz, then 900MHz.

I am not a cpufreq expert but IMHO that will mess up the cpufreq 
governor, without talking about the freq changes notifiers.

Don't you have warning in dmesg ?

"Warning: CPU frequency out of sync: cpufreq and timing ..."

> for other policies like ondemand, i'd like to have rongjun
> collect some proof by doing some run+sleep +run+sleep... loop to check
> the actual behaviour.

Yeah, that would be nice.

>> Moreover, the OPP stuff, from a POV design, is tied with cpufreq and
>> changing that from cpuidle will violate the decisions of the cpufreq
>> governor.
>>
>> Did you do some measurements without cpuidle but with the cpufreq governor
>> 'ondemand' and compare that with the cpuidle driver you are submitting ?
>
> i'd like rongjun will provide measured data on real boards we have
> done before. i think the WFI+low vol+low freq comes from some real
> measurement before.
>
>>
>> Hopefully, Viresh (Cc'ed) can give its opinion.
>>
>>
>> Thanks
>>    -- Daniel
>
> rongjun, pls keep one eye on this thread until we provide enough
> persuasiveness and all problems are clarified here.
>
> -barry
>


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