[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 06/11] x86/intel_pstate: the main boby of the intel_pstate driver
On 12/06/2015 19:30, Julien Grall wrote: > On 11/06/2015 21:41, Wang, Wei W wrote: > > On 11/06/2015 22:02, Julien Grall wrote: > >> On 11/06/2015 04:27, Wei Wang wrote: > >>> diff --git a/xen/include/acpi/cpufreq/cpufreq.h > >> b/xen/include/acpi/cpufreq/cpufreq.h > >>> index d10e4c7..71bb45c 100644 > >>> --- a/xen/include/acpi/cpufreq/cpufreq.h > >>> +++ b/xen/include/acpi/cpufreq/cpufreq.h > >>> @@ -34,6 +34,12 @@ struct acpi_cpufreq_data { > >>> > >>> extern struct acpi_cpufreq_data *cpufreq_drv_data[NR_CPUS]; > >>> > >>> +/* > >>> + * Maximum transition latency is in nanoseconds - if it's unknown, > >>> + * CPUFREQ_ETERNAL shall be used. > >>> + */ > >>> +#define CPUFREQ_ETERNAL (-1) > >>> + > >>> struct cpufreq_cpuinfo { > >>> unsigned int max_freq; > >>> unsigned int second_max_freq; /* P1 if Turbo Mode is on > >>> */ > >>> @@ -77,6 +83,8 @@ struct cpufreq_policy { > >>> }; > >>> DECLARE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_policy); > >>> > >>> +extern int intel_pstate_init(void); > >>> + > >> > >> As said on a previous version [1], intel_pstate_init is x86 specific. > >> Although xen/include/acpi contains common headers. > > > > Please see our latest discussion here (the bottom of the link): > > http://lists.xen.org/archives/html/xen-devel/2015-06/msg00047.html > > Well we are planning to move cpufreq.h out of acpi in order to use for device > tree based platform. Most of these declaration is common. > > Although any x86 specific function would have to go out in a separate header. > > Please avoid to add new one when it's possible. I don't see why a new asm- > x86/cpufreq.h can't be added... I don't have an objection to creating a new header like that. Jan, what's your opinion? > >>> extern int __cpufreq_set_policy(struct cpufreq_policy *data, > >>> struct cpufreq_policy *policy); > >>> > >>> @@ -101,6 +109,12 @@ struct cpufreq_freqs { > >>> * CPUFREQ GOVERNORS * > >>> > >> > ********************************************************** > >> ***********/ > >>> > >>> +/* The four internal governors used in intel_pstate */ > >>> +#define CPUFREQ_POLICY_POWERSAVE (1) > >>> +#define CPUFREQ_POLICY_PERFORMANCE (2) > >>> +#define CPUFREQ_POLICY_USERSPACE (3) > >>> +#define CPUFREQ_POLICY_ONDEMAND (4) > >>> + > >> > >> From the comment, this looks like x86 specific. Maybe even intel_pstate? > > > > > > Yes. It's currently only used by the intel_pstate driver. > > After looking to this series, this statement looks wrong to me... You are > using > all these defines in the common cpufreq code (parameters, pmstat,...). > > The cpufreq framework should be agnostic to any cpufreq driver > implementation. > > So it looks like to me that we want CPUFREQ_* to be exposed for anyone. > And specifying the behavior when policy = 0 would be great too rather than > relying on a future developer to not define 0. How about renaming them to "INTEL_PSTATE_POLICY_XXXX" ? Best, Wei _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |