On Wed, Nov 24, 2010 at 09:39:18AM +0530, Rudramuni, Vishwesh M wrote:
First of all, it is customary to send out the entire patch series rather
than only those that you modified when you come up with a new version.
In addition, it would be better to name your new patch series [PATCH v2]
and have a short changelog in the very beginning outlining what changes
you made.
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> @@ -166,6 +172,24 @@ static struct cpuidle_state
> atom_cstates[MWAIT_MAX_NUM_CSTATES] = {
> .power_usage = 150,
> .target_residency = 4000,
Are you sure C6 has longer target residency than S0ix? What code base is
your patch against?
> .enter = &intel_idle },
> + { /* 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 = soc_s0i1_idle },
> + { /* S0i3 State*/
> + .name = "ATM-S0i3",
> + .desc = "MWAIT 0x52",
> + .driver_data = (void *) 0x60,
> + .flags = CPUIDLE_FLAG_TIME_VALID,
> + .exit_latency = 300,
> + .power_usage = 150,
> + .target_residency = 1200,
> + .enter = soc_s0i3_idle }
> };
> +static int soc_s0i1_idle(struct cpuidle_device *dev,
> + struct cpuidle_state *state)
> +{
> + int cpu = smp_processor_id();
> + struct cpuidle_state *next_state;
> + int ret;
> +
> + /* we don't support S0i1 in moorestown , fallback to C6 */
> + if (__mrst_cpu_chip != MRST_CPU_CHIP_PENWELL) {
> + next_state = &dev->states[4];
Why to fall back to C4 instead of C6?
> +static int soc_s0i3_idle(struct cpuidle_device *dev,
> + struct cpuidle_state *state)
> +{
> + unsigned long eax = (unsigned long)cpuidle_get_statedata(state);
> + ktime_t kt_before, kt_after;
> + s64 usec_delta;
> + int cpu = smp_processor_id();
> + struct cpuidle_state *next_state;
> + int ret;
> +
Here you also need to fall back to plain C states like below. Otherwise,
it won't work on other Atom based platforms without S0ix support. Keep
in mind that your code does not only run on mfld and mrst.
if (__mrst_cpu_chip != MRST_CPU_CHIP_LINCROFT &&
__mrst_cpu_chip != MRST_CPU_CHIP_PENWELL) {
fallback...
}
> + if (get_target_idle_state() == MID_S0I3_STATE) {
> + if (cpu == 0) {
> + local_irq_disable();
> + clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
> + &cpu);
> +
> + kt_before = ktime_get_real();
> +
> + stop_critical_timings();
> +#ifndef MODULE
> + trace_power_start(POWER_CSTATE, (eax >> 4) + 1);
> +#endif
> + /* mwait will be called inside mid_s0i3_enter() */
> + mid_s0i3_enter();
> +
> + start_critical_timings();
> +
> + kt_after = ktime_get_real();
> + usec_delta =
> + ktime_to_us(ktime_sub(kt_after, kt_before));
> +
> + local_irq_enable();
> +
> + clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
> + &cpu);
> +
> + return usec_delta;
> + }
> + }
> +
> + next_state = &dev->states[4];
> + ret = intel_idle(dev, next_state);
> + dev->last_state = &dev->states[4];
> + return ret;
> +
> +}
> diff --git a/drivers/idle/intel_idle.h b/drivers/idle/intel_idle.h
> +#ifndef CONFIG_INTEL_MID_POWER
> +static void mid_s0i3_enter(void) { }
> +static void mid_s0i1_prepare(void) { }
> +static int get_target_idle_state(void) { return 0; }
> +#endif
#else? Did you even compile test your patch? Ever?
> +extern void mid_s0i3_enter(void);
> +extern void mid_s0i1_prepare(void);
> +extern int get_target_idle_state(void);
> +#endif
> diff --git a/drivers/idle/intel_s0ix.c b/drivers/idle/intel_s0ix.c
...
> +static void mrst_apic_save(struct s0ix_data *dev_p)
> +{
> + int i;
> +
> + for (i = 0; i < LAPIC_MAX_REGS; i++) {
> + lapic_regs_val[i] = apic_read((u32) dev_p->apic_base_v + \
> + lapic_regs_offset[i]);
apic_read is not used this way. Please take a look at
arch/x86/kernel/apic/apic.c. Btw, this looks very much like
lapic_suspend which is the standard suspend callback for apic device.
Can you reuse that? In addition, this proves again that you did
not test your code. Please do test before submission.
> + }
> +}
> +
> +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++) {
> + apic_write(lapic_regs_val[i], (u32) (dev_p->apic_base_v + \
> + lapic_regs_offset[i]));
ditto.
> + }
> +}
> +
> +static int mrst_s0i3_env(struct s0ix_data *dev_p)
> +{
...
> + dev_p->apic_base_v = ioremap(0xFEE00000, PAGE_SIZE);
> + if (dev_p->apic_base_v == NULL) {
> + /* Unable to remap. Use a translation address */
> + dev_dbg(&pmu_dev->dev, "mrst_s0i3_env: Unable to remap \
> + APIC address\n");
> + dev_p->apic_base_v = phys_to_virt(0xFEE00000);
> + } else
> + dev_dbg(&pmu_dev->dev, "APIC mapped @ %p\n", \
> + dev_p->apic_base_v);
You do not need to do this which has been done by kernel. IIRC, this
will also result in kernel warnings of the same area being mapped for
multiple times.
> diff --git a/drivers/idle/intel_s0ix.h b/drivers/idle/intel_s0ix.h
...
> +/* MSR definitions needed for save restore */
> +#define IA32_MTRRCAP_MSR 254
You can find this defined as MSR_MTRRcap in msr.h as well.
> +
> +/* MSR definitions */
> +#define WAKE_VECTOR_OFFSET 24
> +#define WAKE_VECTOR_LENGTH 8
> +
> +/* Variable MTTR MSR's */
> +#define MTRRphysBase0 512
> +
You can find this defined as MTRRphysBase_MSR in <asm/mtrr.h>.
> +/* CAP_MSR definitions */
> +#define MTRR_FIXED 0x100
> +
> +/* 11 MTTR's of 64 bits each */
> +#define MTRR_FIXED_SIZE (11 * 8)
Looks like most of the mtrr save/restore code in suspend/resume path
can be eliminated by reusing the existing code under
arch/x86/kernel/cpu/mtrr.
> +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 */
> +};
Why is this still here? Won't you reuse 'struct x86_hw_tss' in
<asm/processor.h>?
> +
> +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;
> +};
> +
Why are these still here? Won't you reuse 'struct desc_struct' in
<asm/desc.h>?
> diff --git a/drivers/idle/intel_s0ix_resume.S
> b/drivers/idle/intel_s0ix_resume.S
...
> +
> +#ifdef CONFIG_X86_PAE
> +#define RSM_CR3_VAL 0x70
> +#else
> +#define RSM_CR3_VAL 0x50
> +#endif
Looks like your code still runs in both PAE and non-PAE modes. Is PAE a
hard requirement? If not, please remove the dependency on X86_PAE.
Otherwise, please clean up all non-PAE code.
-Yong
_______________________________________________
MeeGo-kernel mailing list
[email protected]
http://lists.meego.com/listinfo/meego-kernel
