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