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

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

On 10/10/2013 03:07 PM, Barry Song wrote:
> 2013/10/10 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.
>
> it seems not. cpuidle is the final task. when cpuidle is entering,
> actually other tasks have sleeped.
> btw, rongjun's codes actually save the current cpufreq -> enter lowest
> vol/freq -> WFI -> restore saved cpufreq.
>
> so if the current cpufreq is 600MHz, then the story is
> 1. saving 600MHz
> 2. goto 300MHz
> 3. WFI
> 4. restoring to 600MHz
>
> so there is no conflict with cpufreq considering the whole "save the
> current cpufreq -> enter lowest vol/freq -> WFI -> restore saved
> cpufreq" is in a atomic context.

Ok, may be my comment is inappropriate. I see you change the clock 
parent to 'osc' to 26MHz. If the cpu is WFI it is clock gated, no ?

>> 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 ..."
>
> i think no.
>
>>
>>
>>> 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.
>
> i would hope the test can cover things like "run xx ms -> sleep yy ms
> -> run xx ms -> sleep yy ms" and we change the xx/yy in different test
> cases to check the result.
>
>
>>
>>
>
> -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