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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
