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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel