Re: [PATCH v9 6/9] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Lina Iyer <lina.iyer@linaro.org>]

On Mon, Nov 17 2014 at 10:39 -0700, Lorenzo Pieralisi wrote:
>On Sat, Oct 25, 2014 at 12:40:21AM +0100, Lina Iyer wrote:
>> Add cpuidle driver interface to allow cpus to go into C-States. Use the
>> cpuidle DT interface, common across ARM architectures, to provide the
>> C-State information to the cpuidle framework.
>
>"idle states", this is not ACPI.
>
Okay.
>> Supported modes at this time are Standby and Standalone Power Collapse.
>>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
[...]

> +static struct qcom_cpu_pm_ops *lpm_ops;
>> +
>> +static int qcom_cpu_stby(struct cpuidle_device *dev,
>> +				struct cpuidle_driver *drv, int index)
>> +{
>> +	lpm_ops->standby(NULL);
>> +
>> +	return index;
>> +}
>> +
>> +static int qcom_cpu_spc(struct cpuidle_device *dev,
>> +				struct cpuidle_driver *drv, int index)
>> +{
>> +	lpm_ops->spc(NULL);
>> +
>> +	return index;
>> +}
>> +
>
>I can't have a look at this and avoid thinking that this should look
>something like:
>
>static qcom_cpu_idle(...., int index)
>{
>	lpm_ops[index].enter_idle(...);
>	return index;
>}
>
>Before jumping to conclusions, see below.
>
>> +static struct cpuidle_driver qcom_cpuidle_driver = {
>> +	.name	= "qcom_cpuidle",
>> +};
>> +
>> +static const struct of_device_id qcom_idle_state_match[] = {
>> +	{ .compatible = "qcom,idle-state-stby", .data = qcom_cpu_stby},
>> +	{ .compatible = "qcom,idle-state-spc", .data = qcom_cpu_spc },
>> +	{ },
>> +};
>> +
>> +static int qcom_cpuidle_probe(struct platform_device *pdev)
>> +{
>> +	struct cpuidle_driver *drv = &qcom_cpuidle_driver;
>> +	int ret;
>> +
>> +	lpm_ops = pdev->dev.platform_data;
>> +
>> +	/* Probe for other states, including standby */
>> +	ret = dt_init_idle_driver(drv, qcom_idle_state_match, 0);
>
>This driver will be DT only. If an idle state is parsed correctly,
>it is initialized, otherwise it is skipped. Now, if we added glue
>code in arch arm (as arm64 does) that allows us to link an idle state
>index with the functions above (a DT idle state contains all information
>required to initialize its enter function, more so now that we are adding
>power domains to the picture), what would be the issue in defining a
>common API that just passes the index to the arch back-end ? No pointers
>to pass, no platform drivers required and still no arch/soc code in
>drivers/cpuidle.
>
>I am obviously talking about DT CPUidle drivers only.
>
>If the idle state is parsed correctly and the backend initializer (let's
>call it arm_cpu_init_idle(cpu)) is successful (which means that DT idle
>states contain valid information and the enter functions could be
>initialized from DT properly) I do not see what's the problem, give it
>some thought.
>

>Anyway, I will put together an RFC to start the discussion when patch is
>merged and patch the relevant code as an example, you can go ahead with
>current code, I am reviewing it.
>
OK. I understand your idea. Like it.

>Lorenzo
>