On 03/21/2012 10:54 PM, Kevin Hilman wrote:
Daniel Lezcano<daniel.lezcano@xxxxxxxxxx> writes:
On 03/21/2012 02:43 PM, Jean Pihet wrote:
On Wed, Mar 21, 2012 at 11:07 AM, Santosh Shilimkar
<santosh.shilimkar@xxxxxx> wrote:
Daniel,
On Wednesday 21 March 2012 02:57 PM, Daniel Lezcano wrote:
This patchset is a proposition to improve a bit the code.
The changes are code cleanup and does not change the behavior of the
driver itself.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only
and why the WFI is not used ?
Daniel Lezcano (7):
ARM: OMAP4: cpuidle - Remove unused valid field
ARM: OMAP4: cpuidle - Declare the states with the driver declaration
ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table
ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration
ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time
ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly
ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot
time
The series looks fine to me in general. This clean-up is applicable
for OMAP3 cpuidle code as well.
Great!
However OMAP3 has a few specific things that cannot be removed as easily:
- the 'valid' flag is used because only certain combinations of power
domains states are possible,
When I look the board-rx51 code I see:
static struct cpuidle_params rx51_cpuidle_params[] = {
/* C1 */
{110 + 162, 5 , 1},
/* C2 */
{106 + 180, 309, 1},
/* C3 */
{107 + 410, 46057, 0},
/* C4 */
{121 + 3374, 46057, 0},
/* C5 */
{855 + 1146, 46057, 1},
/* C6 */
{7580 + 4134, 484329, 0},
/* C7 */
{7505 + 15274, 484329, 1},
};
If C3, C4, C6 are not valid, so AFAICS never used in the cpuidle code
why the values are 'exit_latency' and 'target_residency' specified ? I
mean why don't we have { 0, 0, 0 } ? Is it just for information ?
Yes, because getting those numbers were based on some board-specific
measurements, and we don't want those values to be lost. Also, some
usage patterns might want to (re)enable those states.
When you say re-enable you mean for a custom kernel ?
I understand the purpose of this code but it looks a bit tricky and
hard to factor out. Is it acceptable to create a new cpuidle driver
for rx51 then we factor out the code between omap3, omap4 and rx51
when all the code consistent ?
I don't like that idea. All the code is the same, the only thing we
need is the ability to pass in board-specific latency numbers for the
C-states.
Sorry I was not clear, I was not saying duplicating the code. What I
meant is to create a driver:
struct cpuidle_driver rx51_idle_driver = {
.name = "rx51_idle",
.owner = THIS_MODULE,
.states = {
{
/* What is in rx51_cpuidle_params */
}
};
We put in there only the valid fields and we keep in a comment the table
with the latency numbers.
Assuming the valid field is for handling the table overwritting, we can
then remove it. By this way, we simplify the next_valid_state function.
Depending if we have rx51 or not, we register the rx51 driver or the
omap3 driver in the init function. That has also the benefit to group
the cpuidle code in the cpuidle34xx file.
These latency numbers are obviously quite significant when it comes to
the final power consumption, and these values often depend on
board-specific settings. Until there are generic frameworks for
defining all the latencies involved, passing these values in from board
files is absolutly needed.
Yes but before creating the generic framework, we have to do a
transversal cpuidle consolidation across the SoC, factor out the code
when possible, and ensure the consistency between the different platform
and see a common pattern to emerge from this work.
--
<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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
