Re: [Xen-devel] [PATCH v3 06/11] x86/intel_pstate: the main boby of the intel_pstate driver

["Wang, Wei W" <wei.w.wang@xxxxxxxxx>]

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