>-----Original Message-----
>From: Yong Wang [mailto:[email protected]]
>Sent: Wednesday, November 24, 2010 2:15 PM
>To: Rudramuni, Vishwesh M; Brown, Len; Anvin, H Peter
>Cc: [email protected]; Bhimarao, Vijay; Muralidhar, Rajeev D; Seshadri,
>Harinarayanan
>Subject: Re: [Meego-kernel] [Meego-Kernel][PATCH 0/3] intel idle driver with
>s0ix hooks
>
>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?
I have not changed anything here. Its default set to this.
>
>> .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?
>
This is indeed C6 & not C4!!!
>> +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...
> }
>
Will add this.
>> + 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
>
Will change it.
>> 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.
>
>> + }
>> +}
>> +
>
Will look into it.
>> +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.
>
Will remove this.
>> 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>?
We had some issues using the linux tss. All the validation till now is done
With code & it would result in lot more debug with linux tss. I will work
On the linux tss but until then will retain this code.
>
>> 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.
>
Will remove this.
>-Yong
_______________________________________________
MeeGo-kernel mailing list
[email protected]
http://lists.meego.com/listinfo/meego-kernel
