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.
> 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.
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.
Kevin
--
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
