From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 06/11] x86/intel_pstate: the main boby of the intel_pstate driver Date: Fri, 12 Jun 2015 07:29:55 -0400 Message-ID: <557AC2B3.5020200@citrix.com> References: <1434011265-17256-1-git-send-email-wei.w.wang@intel.com> <5579948D.5060105@citrix.com> <286AC319A985734F985F78AFA26841F798D0CE@shsmsx102.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <286AC319A985734F985F78AFA26841F798D0CE@shsmsx102.ccr.corp.intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Wang, Wei W" , "xen-devel@lists.xen.org" , "jbeulich@suse.com" Cc: "andrew.cooper3@citrix.com" List-Id: xen-devel@lists.xenproject.org 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... >>> 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. Regards, -- Julien Grall