On 02/02/2013 03:16 AM, Len Brown wrote:
intel_idle already uses a driver-specific flag:
#define CPUIDLE_FLAG_TLB_FLUSHED 0x10000
This patch just uses 4 more bits along with that one.
Ok. In this case it would make sense to move this flag out of the generic core code to the intel_idle.c file ?
This flag is already local to intel_idle.c. If another architecture finds it useful, then yes, it would make sense to move it to the shared header.
Oh, right. Sorry I puzzled myself with the name. I was convinced it was in the shared header.
and move the [dec/en]coding macro flags_2_MWAIT_EAX and MWAIT_EAX_2_flags (with a more appropriate name for a generic code) to the cpuidle.h file ?
I think that a driver's private flag definitions should remain local to the driver. It makes no sense to pollute the name space of other drivers for stuff that doesn't mean anything to them. MWAIT is pretty specific to x86 -- and re-naming it to something 'generic' isn't going to make the code easier to read.
Ok, let me rephrase it because I think how it was presented was not clear.
As we want to use the half of the state flags for private purpose, I suggested to add the encoding/decoding function in the shared header file.
The mwait eax flags are not encoded and the CPUIDLE_FLAG_TLB_FLUSHED is encoded.
I suggested to unify both and to use an encoding function from the shared header file.
#define CPUIDLE_PRIVATE_FLAGS(_flags_) \ ((_flags_) << 16) & CPUIDLE_DRIVER_FLAGS_MASK
For example:
#define FLAG_TLB_FLUSHED CPUIDLE_PRIVATE_FLAGS(0x1) #define FLAG_MWAIT_C1 CPUIDLE_PRIVATE_FLAGS(0x0) #define FLAG_MWAIT_C2 CPUIDLE_PRIVATE_FLAGS(0x10) #define FLAG_MWAIT_C3 CPUIDLE_PRIVATE_FLAGS(0x20) #define FLAG_MWAIT_C4 CPUIDLE_PRIVATE_FLAGS(0x30) #define FLAG_MWAIT_C5 CPUIDLE_PRIVATE_FLAGS(0x40) #define FLAG_MWAIT_C6 CPUIDLE_PRIVATE_FLAGS(0x52)
And then:
...
.flags = FLAG_TLB_FLUSHED | FLAG_MWAIT_C2 | CPUIDLE_FLAG_TIME_VALID
...
But in the idle function, you need to retrieve the 'value' of the EAX not a flag, so there is the need for an extra macro conversion and mask the TLB flag.
Well, this is a detail, so feel free to ignore this suggestion :)
Thanks -- Daniel
I think that a driver's private flag definitions should remain local to the driver. It makes no sense to pollute the name space of other drivers for stuff that doesn't mean anything to them. MWAIT is pretty specific to x86 -- and re-naming it to something 'generic' isn't going to make the code easier to read.
Ok, let me rephrase it because I think how it was presented was not clear.
As we want to use the half of the state flags for private purpose, I suggested to add the encoding/decoding function in the shared header file.
The mwait eax flags are not encoded and the CPUIDLE_FLAG_TLB_FLUSHED is encoded.
I suggested to unify both and to use an encoding function from the shared header file.
#define CPUIDLE_PRIVATE_FLAGS(_flags_) \ ((_flags_) << 16) & CPUIDLE_DRIVER_FLAGS_MASK
For example:
#define FLAG_TLB_FLUSHED CPUIDLE_PRIVATE_FLAGS(0x1) #define FLAG_MWAIT_C1 CPUIDLE_PRIVATE_FLAGS(0x0) #define FLAG_MWAIT_C2 CPUIDLE_PRIVATE_FLAGS(0x10) #define FLAG_MWAIT_C3 CPUIDLE_PRIVATE_FLAGS(0x20) #define FLAG_MWAIT_C4 CPUIDLE_PRIVATE_FLAGS(0x30) #define FLAG_MWAIT_C5 CPUIDLE_PRIVATE_FLAGS(0x40) #define FLAG_MWAIT_C6 CPUIDLE_PRIVATE_FLAGS(0x52)
And then:
...
.flags = FLAG_TLB_FLUSHED | FLAG_MWAIT_C2 | CPUIDLE_FLAG_TIME_VALID
I like your syntax better than what I used -- thanks for suggesting it.
I can't use it as-is, however, because I really do need to be able to support unique parameters on each entry (eg. different flags for C3 on different processors). When some of the logical and functional changes in flux settle out I'll come back to re-visit the syntax.
thanks, -Len Brown, Intel Open Source Technology Center
-
Daniel Lezcano -
Len Brown