On Wed, Nov 24, 2010 at 03:16:07PM +0530, Rudramuni, Vishwesh M wrote:
> >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.
>
Then you might be working on the wrong code base. S0ix will never be
selected the way your patch adds them into the state table.
> >> +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!!!
>
Oh you are right. I forgot the null states were skipped. But this is
really not quite readable. Maybe we should add a cpuidle_get_state
function which returns the state of interest based on the name of the
state.
> >> 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 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>?
>
What about the above four comments?
> 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.
>
Will look into the oddities and try to understand what they are.
> >
> >> 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.
Are you sure PAE is a hard requirement?
_______________________________________________
MeeGo-kernel mailing list
[email protected]
http://lists.meego.com/listinfo/meego-kernel
