RE: [PATCH 2/2] i7300_idle driver v1.55

[Pallipadi, Venkatesh]

Thanks for the review. Some comments below.

Thanks,
Venki

>-----Original Message-----
>From: Randy Dunlap [mailto:randy.dunlap@xxxxxxxxxx]
>Sent: Monday, October 20, 2008 10:32 AM
>To: Len Brown
>Cc: Ingo Molnar; linux-acpi@xxxxxxxxxxxxxxx; Linux Kernel
>Mailing List; Pallipadi, Venkatesh; Henroid, Andrew D; Linus
>Torvalds; Thomas Gleixner; H. Peter Anvin; Andi Kleen
>Subject: Re: [PATCH 2/2] i7300_idle driver v1.55
>
>On Thu, 16 Oct 2008 18:08:01 -0400 (EDT) Len Brown wrote:
>
>> Signed-off-by: Andy Henroid <andrew.d.henroid@xxxxxxxxx>
>> Signed-off-by: Len Brown <len.brown@xxxxxxxxx>
>> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@xxxxxxxxx>
>> ---
>>  MAINTAINERS               |    6 +
>>  arch/x86/Kconfig          |    2 +
>>  drivers/Makefile          |    1 +
>>  drivers/dma/ioat_dma.c    |    3 +
>>  drivers/idle/Kconfig      |   16 +
>>  drivers/idle/Makefile     |    2 +
>>  drivers/idle/i7300_idle.c |  674
>+++++++++++++++++++++++++++++++++++++++++++++
>
>Why not just use drivers/power/ instead of a new sub-directory?

As the driver was specific to idle system and specific to few
platforms/chipset, we decided to put it in a separate directory.
We had called the new directory as drivers/memory initially.
But, idle seemed to suit it better.

I have no issues moving this over to drivers/power. Will send a
incremental patch to move it if that suits better. But, if we
Expect other platforms to have similar platform specific idle
power savings through drivers like this, then having a separate
dir to deal with idle optimization should be better.

>> +/*
>> + * Value to set THRTLOW to when initiating throttling
>> + *  0 = No throttling
>> + *  1 = Throttle when > 4 activations per eval window
>(Maximum throttling)
>> + *  2 = Throttle when > 8 activations
>> + *  168 = Throttle when > 168 activations (Minimum throttling)
>
>Why do some numbers mean > their value (i.e., 168) but others don't?
>Is it a linear value or logarithmic or what?

That is a typo in the comment. These values are simple "multiply by 4".
So, 168 should be 672 activations, and that is the minimum
possible throttling.

>> + */
>> +#define MAX_THRTLWLIMIT              168
>> +static uint i7300_idle_thrtlowlm = 1;
>> +module_param_named(thrtlwlimit, i7300_idle_thrtlowlm, uint, 0644);
>> +MODULE_PARM_DESC(thrtlwlimit,
>> +             "Value for THRTLOWLM activation field "
>> +             "(0 = disable throttle, 1 = Max throttle, 168
>= Min throttle)");
>> +
>
>> +/* Stop I/O AT memory copy */
>> +static void i7300_idle_ioat_stop(void)
>> +{
>> +     int i;
>> +     u8 sts;
>> +
>> +     for (i = 0; i < 5; i++) {
>> +             writeb(IOAT_CHANCMD_RESET,
>> +                     ioat_chanbase + IOAT1_CHANCMD_OFFSET);
>> +
>> +             udelay(10);
>> +
>> +             sts = readq(ioat_chanbase + IOAT1_CHANSTS_OFFSET) &
>> +                     IOAT_CHANSTS_DMA_TRANSFER_STATUS;
>
>readq() returns 64 bits.  You only want to LS-byte of it here?

CHANSTS is a 64 bit register and we only use lower few bits
around here. But, agree with your comment and will change it
sts to u64 with an update patch. That will make it consistent
with other reads of this register in this file

>
>> +             if (sts != IOAT_CHANSTS_DMA_TRANSFER_STATUS_ACTIVE)
>> +                     break;
>> +
>> +     }
>> +
>> +     if (i == 5)
>> +             dprintk("failed to suspend+reset I/O AT after
>5 retries\n");
>> +
>> +}
>
>
>> +struct debugfs_file_info {
>> +     void *ptr;
>> +     char name[32];
>> +     struct dentry *file;
>> +} debugfs_file_list[] = {
>> +                             {&total_starts, "total_starts", NULL},
>> +                             {&total_us, "total_us", NULL},
>> +#ifdef DEBUG
>> +                             {&past_skip, "past_skip", NULL},
>> +#endif
>> +                             {NULL, "", NULL}
>> +                     };
>
>What is debugfs used for?  any docs for it?

It has some statistics about how many times we went
into "memory throttling" state and how long we stayed
there since the boot. And one tunable that tunes a
parameter changing the policy to start throttling.
Will document this with an update patch.

>
>Does this build/work if CONFIG_DEBUG_FS=n?

Yes. Did not see any breakage with DEBIG_FS=n

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/