[PATCH v8 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Lina Iyer]

On Thu, Oct 23 2014 at 06:43 -0600, Lorenzo Pieralisi wrote:
>On Thu, Oct 23, 2014 at 12:05:39PM +0100, Daniel Lezcano wrote:
>
>[...]
>
>> > diff --git a/drivers/cpuidle/cpuidle-qcom.c b/drivers/cpuidle/cpuidle-qcom.c
>> > new file mode 100644
>> > index 0000000..0a65065
>> > --- /dev/null
>> > +++ b/drivers/cpuidle/cpuidle-qcom.c
>> > @@ -0,0 +1,79 @@
>> > +/*
>> > + * Copyright (c) 2014, Linaro Limited.
>> > + *
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License version 2 and
>> > + * only version 2 as published by the Free Software Foundation.
>> > + *
>> > + * This program is distributed in the hope that it will be useful,
>> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> > + * GNU General Public License for more details.
>> > + *
>> > + */
>> > +
>> > +#include <linux/cpu_pm.h>
>> > +#include <linux/cpuidle.h>
>> > +#include <linux/module.h>
>> > +#include <linux/platform_device.h>
>> > +
>> > +#include <soc/qcom/pm.h>
>> > +#include "dt_idle_states.h"
>> > +
>> > +static void (*qcom_idle_enter)(enum pm_sleep_mode);
>> > +
>> > +static int qcom_lpm_enter_stby(struct cpuidle_device *dev,
>> > +                             struct cpuidle_driver *drv, int index)
>> > +{
>> > +     qcom_idle_enter(PM_SLEEP_MODE_STBY);
>>
>> Could you replace this function by a generic one ?
>>
>> It would be nice to have qcom_cpu_standby(void) and
>> qcom_cpu_powerdown(void) and let behind the mysterious words 'Single
>> Power Collapse' in the low level code which is qcom specific :)
>>
>> I guess you had to create a single "qcom_idle_enter" because of a single
>> pointer in the platform data. I am working on a common structure to be
>> shared across the drivers as a common way to pass the different
>> callbacks without including a soc specific header.
>>
>> struct cpuidle_ops {
>>         int (*standby)(void *data);
>>         int (*retention)(void *data);
>>         int (*poweroff)(void *data);
>> };
>>
>> So maybe you can either:
>>
>> 1. wait I post this structure and provide the driver with this one
>> 2. create a similar structure and I will take care to upgrade when I
>> post the patchset with the common structure.
>>
>> The issue I see with this common structure is the initialization of the
>> qcom_idle_state_match array.
>
>Because you do not know which function to call when right ? That's why
>I think it is up to the CPUidle back-end to make that decision and why
>I think that the contract between the CPUidle driver and the back-end
>should be the idle index. Even if you have pointers to functions,
>the CPUidle driver do not know what parameter it has to chuck into
>the void data, which is likely to be platform specific too. Granted,
>you can make those cpuidle_ops a set of pairs, {function, parameter}
>associated with an idle index, but then you will end up implementing
>what I suggest below.
>
>If you have a look at how the MCPM wrapper works on bL driver, that's
>exactly the same problem. At the moment we are supporting only cluster
>shutdown. If we were to add a core gating idle state, how could the
>MCPM back-end differentiate between core and cluster states ? At the
>moment the only way would consist in passing the residency through
>mcpm_suspend() parameter. We could pass the idle state index, it is the
>same story.
>
>That's the reasoning behind cpu_suspend on ARM64, the index determines
>what should be done, it is up to the platform back end to execute the
>required actions (and it is not because we have PSCI on ARM64, that
>concept is generic and should be made similar on ARM32 IMHO).
>
>DT idle states are sorted by definition, and they can be parsed by
>the arch back-end too to determine the actions required by an idle
>state index (eg most likely information coming from power domains).

This is where it makes it difficult for me. QCOM SoC's can differ from
each other in the states supported. For example here, I dont have
retention state, while other SoC's would have it. And to have cpuidle
states with increasing order of power saving (or latency), retention
would creep in between standby and power collpase. This makes the
mapping, a real issue to figure out what the SoC cpuidle driver chose to
define and to what functions they map to. I was thinking in lines of the 
ops structure, which could work for PSCI as well.

>
>The glue code on arm64 is cpu_init_idle(), which takes charge of
>initializing the idle state platform specific actions/parameters/etc.
>
>Everything is open to debate, as long as we nail down an interface on
>arm32 and we stick to that from now onwards, I do not think you are far
>from achieving that at all.
>
Thanks, I will try and get the QCOM idle drivers conform to the common
ideas.

>Cheers,
>Lorenzo
>