On Tue, Nov 16, 2010 at 05:36:24PM +0530, Rudramuni, Vishwesh M wrote:
> >From 8b151bc47bdfa4d4e52ac5db637d7a4162665217 Mon Sep 17 00:00:00 2001
> From: Vishwesh M Rudramuni <[email protected]>
> Date: Tue, 16 Nov 2010 22:13:59 +0530
> Subject: [PATCH 0/4] intel idle driver with s0ix hooks
>
[PATCH 0/n] is typically the cover letter created by git-format-patch
--cover-leter. The real patches start from [PATCH 1/n].
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
...
> + { /* S0i1 State*/
> + .name = "ATM-S0i1",
> + .desc = "MWAIT 0x52",
> + .driver_data = (void *) 0x50,
> + .flags = CPUIDLE_FLAG_TIME_VALID,
> + .exit_latency = 250,
> + .power_usage = 150,
> + .target_residency = 1000,
> + .enter = NULL},
compiler will spit a warning if you define soc_s0i1_idle but do not use
it.
> + { /* S0i3 State*/
> + .name = "ATM-S0i3",
> + .desc = "MWAIT 0x52",
> + .driver_data = (void *) 0x60,
What is this? Does Atom CPU support C7? At least I do not see C7 in
mwait leaf on LNC.
> +static int soc_s0i1_idle(struct cpuidle_device *dev,
> + struct cpuidle_state *state)
...
> + if ((cpu == 0) && (thread1_c6_state == 1))
> + mid_s0i1_prepare();
...
> @@ -230,6 +338,10 @@ static int intel_idle(struct cpuidle_device *dev, struct
> cpuidle_state *state)
> kt_before = ktime_get_real();
>
> stop_critical_timings();
> +
> + if ((cpu == 1) && (eax == 0x40))
> + thread1_c6_state = 1;
> +
Please be noted that Peter gave you some comments about such pieces of
code. Could you reply?
> diff --git a/drivers/idle/intel_s0ix.c b/drivers/idle/intel_s0ix.c
> new file mode 100644
> index 0000000..7fdfef5
> --- /dev/null
> +++ b/drivers/idle/intel_s0ix.c
> @@ -0,0 +1,1726 @@
> +/*
> + * sfi_processor_idle.c - sfi based c-state driver
> + * Copyright (c) 010, Intel Corporation.
s/010/2010/
> +static void mrst_apic_save(struct s0ix_data *dev_p)
> +{
> + int i;
> +
> + for (i = 0; i < LAPIC_MAX_REGS; i++) {
> + lapic_regs_val[i] = __raw_readl(dev_p->apic_base_v + \
> + lapic_regs_offset[i]);
Please use the apic_read in <asm/apic.h> instead.
> + }
> +}
> +
> +static void mrst_apic_restore(struct s0ix_data *dev_p)
> +{
> + int i;
> +
> + /* TODO: need to revisit this code not sure if we need to restore
> + * all Lapic registers after S0ix state
> + */
> + for (i = 0; i < LAPIC_MAX_REGS; i++) {
> + __raw_writel(lapic_regs_val[i], dev_p->apic_base_v + \
> + lapic_regs_offset[i]);
Please use the apic_write in <asm/apic.h> instead.
> diff --git a/drivers/idle/intel_s0ix.h b/drivers/idle/intel_s0ix.h
> +
> +#include <asm/apicdef.h>
Please include <asm/apic.h> instead so that you get not only the macro
definitions but also the related functions.
> +#include <asm/pgtable_types.h>
ditto. Please include <asm/pgtable.h> and use functions in the header
file as much as you can.
> +#include <asm/msr-index.h>
ditto. Include <asm/msr.h> instead. As peter pointed out, there is no
reason to reimplement the msr functions kernel already provides.
> +/* ID's associated with each of the helpers */
> +#define CTX_ID_INVALID 0
> +#define CTX_ID_MTTR 1
MTRR? There are still some appearances of mttr in your new patch, e.g.
CTX_ID_MTTR and in code comments. Please fix them all.
> +/* MSR definitions needed for save restore */
> +#define IA32_MTRRCAP_MSR 254
> +
> +/* MSR definitions */
> +#define WAKE_VECTOR_OFFSET 24
> +#define WAKE_VECTOR_LENGTH 8
> +#define IA32_TIME_STAMP_COUNTER 0x10
> +#define IA32_APIC_BASE 0x1B
> +#define MSR_EBL_CR_POWERON 0x2A
> +#define IA32_FEATURE_CONTROL 0x3A
> +#define IA32_PMC0 0xC1
> +#define IA32_PMC1 0xC2
> +#define IA32_MPERF 0xE7
> +#define IA32_SYSENTER_CS 0x174
> +#define IA32_SYSENTER_ESP 0x175
> +#define IA32_SYSENTER_EIP 0x176
> +#define IA32_THERM_INTERRUPT 0x19B
> +#define IA32_THERM_STATUS 0x19C
> +#define IA32_MISC_ENABLE 0x1A0
> +#define IA32_MTRR_DEF_TYPE 0x2FF
> +#define IA32_CR_PAT 0x277
> +#define IA32_FIXED_CTR0 0x309
> +#define IA32_FIXED_CTR1 0x30A
> +#define IA32_FIXED_CTR2 0x30B
> +#define IA32_DS_AREA 0x600
> +#define IA32_EFER 0xC0000080
> +#define IA32_STAR 0xC0000081
> +#define IA32_LSTAR 0xC0000082
> +#define IA32_FMASK 0xC0000084
> +#define IA32_GS_BASE 0xC0000101
> +#define IA32_KERNEL_GS_BASE 0xC0000102
> +
> +/* Variable MTTR MSR's */
> +#define MTRRphysBase0 512
> +
> +/* CAP_MSR definitions */
> +#define MTRR_FIXED 0x100
> +
> +/* 11 MTTR's of 64 bits each */
> +#define MTRR_FIXED_SIZE (11 * 8)
Why do you still have to define these MSRs by yourself if you have
already included the standard msr header?
> +struct task_state_structure {
> + u16 prev_task_link; /* Offset = 0; */
> + u16 reserved_0; /* 2 */
> + u32 esp0; /* 4 */
> + u16 ss0; /* 8 */
> + u16 reserved_1; /* 10 */
> + u32 esp1; /* 12 */
> + u16 ss1; /* 16 */
> + u16 reserved_2; /* 18 */
> + u32 esp2; /* 20 */
> + u16 ss2; /* 24 */
> + u16 reserved_3; /* 26 */
> + u32 cr3; /* 28 */
> + u32 eip; /* 32 */
> + u32 eflags; /* 36 */
> + u32 eax; /* 40 */
> + u32 ecx; /* 44 */
> + u32 edx; /* 48 */
> + u32 ebx; /* 52 */
> + u32 esp; /* 56 */
> + u32 ebp; /* 60 */
> + u32 esi; /* 64 */
> + u32 edi; /* 68 */
> + u16 es; /* 72 */
> + u16 reserved_4; /* 74 */
> + u16 cs; /* 76 */
> + u16 reserved_5; /* 78 */
> + u16 ss; /* 80 */
> + u16 reserved_6; /* 82 */
> + u16 ds; /* 84 */
> + u16 reserved_7; /* 86 */
> + u16 fs; /* 88 */
> + u16 reserved_8; /* 90 */
> + u16 gs; /* 92 */
> + u16 reserved_9; /* 94 */
> + u16 ldt_sel; /* 96 */
> + u16 reserved_10; /* 98 */
> + u16 reserved_11; /* 100 */
> + u16 io_map_base; /* 102 */
> + u8 io_map[IO_MAP_SIZE]; /* Rounds up to 128 bytes */
> +};
Could you take a look at struct x86_hw_tss in <asm/processor.h>?
> +
> +struct desc_entry {
> + u32 size;
> + u32 address;
> +};
> +
> +struct dt_entry_bit {
> + u16 limit_15_0;
> + u16 base_15_0;
> + u8 base_23_16;
> + u8 type;
> + unsigned limit_19_16:4;
> + unsigned attr:4;
> + u8 base_31_24;
> +};
> +
> +union dt_entry {
> + u32 d[2];
> + struct dt_entry_bit b;
> +};
...
> +
> +#define DESCTYPE_TSS 0x89 /* present, system, DPL-0, 32-bit TSS */
> +
> +#if 0
> +#ifndef write_gdt_entry
> +
> +#define write_ldt_entry(dt, entry, a, b) write_dt_entry(dt, entry, a, b)
> +#define write_gdt_entry(dt, entry, a, b) write_dt_entry(dt, entry, a, b)
> +#define write_idt_entry(dt, entry, a, b) write_dt_entry(dt, entry, a, b)
> +
> +#ifndef store_gdt
> +struct Xgt_desc_struct {
> + unsigned short size;
> + unsigned long address __attribute__ ((packed));
> + unsigned short pad;
> +} __attribute__ ((packed));
> +
> +#define store_gdt(dtr) native_store_gdt(dtr)
> +#define store_idt(dtr) native_store_idt(dtr)
> +#endif /* store_gdt */
> +
> +static inline void write_dt_entry(struct desc_struct *dt,
> + int entry, u32 entry_low, u32 entry_high)
> +{
> + pr_debug("Inside write_dt_entry dt:%x, entry:%d, \
> + entry_low:%d entry_high:%d \n", dt, entry, entry_low, entry_high);
> + dt[entry].a = entry_low;
> + dt[entry].b = entry_high;
> + pr_debug("Exiting write_dt_entry\n");
> +}
> +
> +#ifndef store_gdt
> +static inline void native_store_gdt(struct Xgt_desc_struct *dtr)
> +{
> +asm("sgdt %0" : "=m"(*dtr));
> +}
> +
> +static inline void native_store_idt(struct Xgt_desc_struct *dtr)
> +{
> +asm("sidt %0" : "=m"(*dtr));
> +}
Please take a look at <asm/desc.h> as I said earlier. Much code can be
eliminated if you reuse existing kernel code.
> diff --git a/drivers/idle/intel_s0ix_util.S b/drivers/idle/intel_s0ix_util.S
> +ENTRY(mrst_read_msr)
> +ENTRY(mrst_write_msr)
> +ENTRY(mrst_read_msr_block)
> +ENTRY(mrst_write_msr_block)
Please get rid of these as explained above.
-Yong
_______________________________________________
MeeGo-kernel mailing list
[email protected]
http://lists.meego.com/listinfo/meego-kernel
