Changeset View
Standalone View
sys/x86/cpufreq/est.c
Context not available. | |||||
#include <dev/acpica/acpivar.h> | #include <dev/acpica/acpivar.h> | ||||
#include "acpi_if.h" | #include "acpi_if.h" | ||||
#include <x86/cpufreq/hwpstate_intel_internal.h> | |||||
cem: Would it be possible to define this in a shared header instead of multiple `extern` references… | |||||
Done Inline Actionsyeah... jhb@ recommended this way. I'm fine with whatever though, recommendations? bwidawsk: yeah... jhb@ recommended this way. I'm fine with whatever though, recommendations? | |||||
Not Done Inline ActionsLeave it be for now, I guess. This variable is a gross hack, hopefully it isn't used in additional places. cem: Leave it be for now, I guess. This variable is a gross hack, hopefully it isn't used in… | |||||
Not Done Inline ActionsI mostly can't think of a non-crappy header to put it in. jhb: I mostly can't think of a non-crappy header to put it in. | |||||
/* Status/control registers (from the IA-32 System Programming Guide). */ | /* Status/control registers (from the IA-32 System Programming Guide). */ | ||||
#define MSR_PERF_STATUS 0x198 | #define MSR_PERF_STATUS 0x198 | ||||
#define MSR_PERF_CTL 0x199 | #define MSR_PERF_CTL 0x199 | ||||
Done Inline ActionsProbably put this in a header (it can be a totally trivial intel_cpufreq_internal.h, for example) rather than declaring it in both drivers. cem: Probably put this in a header (it can be a totally trivial `intel_cpufreq_internal.h`, for… | |||||
Context not available. | |||||
static devclass_t est_devclass; | static devclass_t est_devclass; | ||||
DRIVER_MODULE(est, cpu, est_driver, est_devclass, 0, 0); | DRIVER_MODULE(est, cpu, est_driver, est_devclass, 0, 0); | ||||
MODULE_DEPEND(est, hwpstate_intel, 1, 1, 1); | |||||
static int | static int | ||||
est_features(driver_t *driver, u_int *features) | est_features(driver_t *driver, u_int *features) | ||||
Context not available. | |||||
{ | { | ||||
device_t child; | device_t child; | ||||
/* | |||||
Done Inline ActionsOh, I see. This is the hack that is preventing multiple freq drivers from registering on intel. Doesn't this break est on hardware too old for HWP support? And the kludge only works for Intel devices? cem: Oh, I see. This is the hack that is preventing multiple freq drivers from registering on intel. | |||||
Done Inline ActionsIt shouldn't. In the hwpstate_intel code, if the hardware is too old, it sets the bool to false. Same for basically any unsupported hardware. bwidawsk: It shouldn't. In the hwpstate_intel code, if the hardware is too old, it sets the bool to false. | |||||
Done Inline ActionsSure, but what controls the order in which est_identify / intel_hwpstate_identify run such that the latter is guaranteed to run first? cem: Sure, but what controls the order in which est_identify / intel_hwpstate_identify run such that… | |||||
Done Inline ActionsI have wondered if we don't want to effectively merge the two drivers in some way. jhb: I have wondered if we don't want to effectively merge the two drivers in some way. | |||||
Done Inline ActionsI can't think of an immediate downside to this idea (and like it better than the current approach). Ben, is it objectionable to you? cem: I can't think of an immediate downside to this idea (and like it better than the current… | |||||
Done Inline ActionsThe main downside is that suppose after this is in I do the cpufreq rework I sketched out in my overall comment last week. In that world, it's quite easy to have the 'speedshift' driver use a higher probe priority than est and have them be separate drivers. Given that they work quite differently I think this is cleaner long term. I had typed this comment above before my general comment (so it was "earlier" in my thinking). jhb: The main downside is that suppose after this is in I do the cpufreq rework I sketched out in my… | |||||
Done Inline ActionsWhen and if you get a chance to do that rework (I don't think it's happened yet?), the drivers could be pulled apart again. It's one of those "the tree isn't written in stone, we can always change it later" plus "don't block working things on good ideas no one has time for" deals, IMO. cem: When and if you get a chance to do that rework (I don't think it's happened yet?), the drivers… | |||||
Done Inline ActionsDoes this seem like a reasonable way to intertwine the two identify routines? scottph: Does this seem like a reasonable way to intertwine the two identify routines? | |||||
Not Done Inline ActionsIt's kinda funky, but tolerable IMO. If John has strong objections, he overrules me :-). Newbus doesn't really make this easy/clean on us. cem: It's kinda funky, but tolerable IMO. If John has strong objections, he overrules me :-). | |||||
Not Done Inline ActionsThis is fine for now. jhb: This is fine for now. | |||||
* Defer to hwpstate if it is present. This priority logic | |||||
* should be replaced with normal newbus probing in the | |||||
* future. | |||||
*/ | |||||
intel_hwpstate_identify(NULL, parent); | |||||
if (device_find_child(parent, "hwpstate_intel", -1) != NULL) | |||||
return; | |||||
/* Make sure we're not being doubly invoked. */ | /* Make sure we're not being doubly invoked. */ | ||||
if (device_find_child(parent, "est", -1) != NULL) | if (device_find_child(parent, "est", -1) != NULL) | ||||
return; | return; | ||||
Context not available. |
Would it be possible to define this in a shared header instead of multiple extern references in .c files?