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

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

On 10/08/2013 09:44 AM, Rongjun Ying wrote:
>
>
>> -----邮件原件-----
>> 发件人: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
>> 发送时间: Monday, October 07, 2013 4:24 PM
>> 收件人: Barry Song; rjw@sisk.pl
>> 抄送: linux-pm@vger.kernel.org; DL-SHA-WorkGroupLinux; Rongjun Ying; Barry
>> Song
>> 主题: Re: [PATCH] cpuidle: sirf : Add cpuidle for SiRFprimaII and SiRFatlasVI
>>
>>
>> Hi Barry,
>>
>> On 10/06/2013 06:26 AM, Barry Song wrote:
>>> From: Rongjun Ying <Rongjun.Ying@csr.com>
>>>
>>> This patch adds support cpuidle for CSR SiRFprimaII and SiRFatlasVI
>>> unicore ARM Cortex-A9 SoCs.
>>
>> A new driver deserves a better description.
>>
>>> Signed-off-by: Rongjun Ying <Rongjun.Ying@csr.com>
>>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>>> ---
>>
>> Is there a technical reference manual accessible somewhere ?
>
> No, I have not.

You haven't one ? How can you write a cpuidle driver without it ?

You didn't answer my question:

Is there a technical reference manual accessible somewhere ?

> If system need enter idle mode, Not need operate any SoC register. SiRF SoC enter cpuidle mode is very simple,
> Which only set the parent clock to OSC(26MHz)  and adjust voltage to min.

This is up to cpufreq.

[ ... ]

>>> +static int sirf_enter_idle(struct cpuidle_device *dev,
>>> +				struct cpuidle_driver *drv,
>>> +				int index)
>>> +{
>>> +	struct clk *parent_clk;
>>> +	unsigned long volt_old = 0, volt_new, freq;
>>> +	struct opp *opp;
>>> +
>>> +	local_irq_disable();
>>
>> Don't use local_irq_disable in the driver it is done by the caller.
>
> Yes, I will remove it.
>
>>
>>> +	parent_clk = clk_get_parent(sirf_cpuidle.cpu_clk);
>>> +	clk_set_parent(sirf_cpuidle.cpu_clk, sirf_cpuidle.osc_clk);
>>> +
>>> +	if (sirf_cpuidle.vcore_regulator) {
>>> +		volt_old = regulator_get_voltage(sirf_cpuidle.vcore_regulator);
>>> +
>>> +		freq = clk_get_rate(sirf_cpuidle.osc_clk);
>>> +		rcu_read_lock();
>>
>> Using rcu in the idle path is not allowed.
>
> Yes, I will remove it.
>
>>
>>> +		opp = opp_find_freq_ceil(sirf_cpuidle.cpu_dev, &freq);
>>> +		if (IS_ERR(opp)) {
>>> +			rcu_read_unlock();
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		volt_new = opp_get_voltage(opp);
>>> +		rcu_read_unlock();
>>> +
>>> +		regulator_set_voltage(sirf_cpuidle.vcore_regulator, volt_new,
>>> +				SIRFSOC_MAX_VOLTAGE);
>>> +	}
>>> +	/* Wait for interrupt state */
>>> +	cpu_do_idle();
>>> +
>>> +	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.

>>> +	local_irq_enable();
>>
>> Done by the caller.
>>
>
> Yes. I will remove it.
>
>>> +	return index;
>>> +}
>>> +
>>> +static struct cpuidle_driver sirf_cpuidle_driver = {
>>> +	.name = "sirf_cpuidle",
>>> +	.states = {
>>> +		ARM_CPUIDLE_WFI_STATE,
>>> +		{
>>> +			.name = "WFI-LP",
>>> +			.desc = "WFI low power",
>>> +			.flags = CPUIDLE_FLAG_TIME_VALID,
>>> +			.exit_latency = 10,
>>> +			.target_residency = 10000,

By the way, why 10000 ? Is it measured ?

>>> +			.enter = sirf_enter_idle,
>>> +		},
>>> +	},
>>> +	.state_count = 2,

[ cut ]

>>> +
>>> +	ret = cpuidle_register(&sirf_cpuidle_driver, NULL);
>>> +	if (!ret)
>>> +		return ret;
>>> +
>>> +out_put_clk:
>>> +	clk_put(sirf_cpuidle.cpu_clk);
>>> +out_put_osc_clk:
>>> +	clk_put(sirf_cpuidle.osc_clk);
>>> +	return ret;
>>> +}
>>> +late_initcall(sirfsoc_init_cpuidle);
>>
>> Please convert it to platform_driver as the other drivers are moving to (cf.
>> cpuidle-kirkwood, cpuidle-ux500).
>>
>
> I think cpuidle is not actual device which can't be added to device tree system.
> So where register this device?

I gave you a couple of examples above, you can rely on it to implement 
something similar.

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