| Message ID | 20240604093406.2448552-1-Sergiy_Kibrik@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN,v1] x86/cpufreq: separate powernow/hwp cpufreq code | expand |
On 04.06.2024 11:34, Sergiy Kibrik wrote: > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c > @@ -657,7 +657,7 @@ static int __init cf_check cpufreq_driver_init(void) > > case X86_VENDOR_AMD: > case X86_VENDOR_HYGON: > - ret = powernow_register_driver(); > + ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -ENODEV; > break; > } What about the Intel-specific code immediately up from here? Dealing with that as well may likely permit to reduce ... > --- a/xen/include/acpi/cpufreq/cpufreq.h > +++ b/xen/include/acpi/cpufreq/cpufreq.h > @@ -252,6 +252,7 @@ void cpufreq_dbs_timer_resume(void); > > void intel_feature_detect(struct cpufreq_policy *policy); > > +#ifdef CONFIG_INTEL > int hwp_cmdline_parse(const char *s, const char *e); > int hwp_register_driver(void); > bool hwp_active(void); > @@ -260,4 +261,35 @@ int get_hwp_para(unsigned int cpu, > int set_hwp_para(struct cpufreq_policy *policy, > struct xen_set_cppc_para *set_cppc); > > +#else > + > +static inline int hwp_cmdline_parse(const char *s, const char *e) > +{ > + return -EINVAL; > +} > + > +static inline int hwp_register_driver(void) > +{ > + return -ENODEV; > +} > + > +static inline bool hwp_active(void) > +{ > + return false; > +} > + > +static inline int get_hwp_para(unsigned int cpu, > + struct xen_cppc_para *cppc_para) > +{ > + return -EINVAL; > +} > + > +static inline int set_hwp_para(struct cpufreq_policy *policy, > + struct xen_set_cppc_para *set_cppc) > +{ > + return -EINVAL; > +} > + > +#endif /* CONFIG_INTEL */ ... the number of stubs you're adding here. Jan
06.06.24 10:08, Jan Beulich: > On 04.06.2024 11:34, Sergiy Kibrik wrote: >> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c >> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c >> @@ -657,7 +657,7 @@ static int __init cf_check cpufreq_driver_init(void) >> >> case X86_VENDOR_AMD: >> case X86_VENDOR_HYGON: >> - ret = powernow_register_driver(); >> + ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -ENODEV; >> break; >> } > > What about the Intel-specific code immediately up from here? > Dealing with that as well may likely permit to reduce ... > you mean to guard a call to hwp_register_driver() the same way as for powernow_register_driver(), and save one stub? ? -Sergiy
On 06.06.2024 09:30, Sergiy Kibrik wrote: > 06.06.24 10:08, Jan Beulich: >> On 04.06.2024 11:34, Sergiy Kibrik wrote: >>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c >>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c >>> @@ -657,7 +657,7 @@ static int __init cf_check cpufreq_driver_init(void) >>> >>> case X86_VENDOR_AMD: >>> case X86_VENDOR_HYGON: >>> - ret = powernow_register_driver(); >>> + ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -ENODEV; >>> break; >>> } >> >> What about the Intel-specific code immediately up from here? >> Dealing with that as well may likely permit to reduce ... > > you mean to guard a call to hwp_register_driver() the same way as for > powernow_register_driver(), and save one stub? ? Yes, and perhaps more. Maybe more stubs can be avoided? And acpi_cpufreq_driver doesn't need registering either, and hence would presumably be left unreferenced when !INTEL? Jan
06.06.24 10:54, Jan Beulich: > On 06.06.2024 09:30, Sergiy Kibrik wrote: >> 06.06.24 10:08, Jan Beulich: >>> On 04.06.2024 11:34, Sergiy Kibrik wrote: >>>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c >>>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c >>>> @@ -657,7 +657,7 @@ static int __init cf_check cpufreq_driver_init(void) >>>> >>>> case X86_VENDOR_AMD: >>>> case X86_VENDOR_HYGON: >>>> - ret = powernow_register_driver(); >>>> + ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -ENODEV; >>>> break; >>>> } >>> >>> What about the Intel-specific code immediately up from here? >>> Dealing with that as well may likely permit to reduce ... >> >> you mean to guard a call to hwp_register_driver() the same way as for >> powernow_register_driver(), and save one stub? ? > > Yes, and perhaps more. Maybe more stubs can be avoided? And > acpi_cpufreq_driver doesn't need registering either, and hence > would presumably be left unreferenced when !INTEL? > {get,set}_hwp_para() can be avoided, as they're being called just once and may be guarded by IS_ENABLED(CONFIG_INTEL). The same for hwp_cmdline_parse(). As for hwp_active() it's being used many times by generic cpufreq code and even outside of cpufreq, so probably it has to be either a stub, or be moved outside of hwp.c and become smth, like this: bool hwp_active(void) { return IS_ENABLED(CONFIG_INTEL) && hwp_in_use; } Though I'm not sure such movement would be any better than a stub. acpi_cpufreq_driver, i.e. the most of code in cpufreq.c file, can probably be separated into acpi.c and put under CONFIG_INTEL as well. What you think of this? -Sergiy
On 07.06.2024 11:14, Sergiy Kibrik wrote: > 06.06.24 10:54, Jan Beulich: >> On 06.06.2024 09:30, Sergiy Kibrik wrote: >>> 06.06.24 10:08, Jan Beulich: >>>> On 04.06.2024 11:34, Sergiy Kibrik wrote: >>>>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c >>>>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c >>>>> @@ -657,7 +657,7 @@ static int __init cf_check cpufreq_driver_init(void) >>>>> >>>>> case X86_VENDOR_AMD: >>>>> case X86_VENDOR_HYGON: >>>>> - ret = powernow_register_driver(); >>>>> + ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -ENODEV; >>>>> break; >>>>> } >>>> >>>> What about the Intel-specific code immediately up from here? >>>> Dealing with that as well may likely permit to reduce ... >>> >>> you mean to guard a call to hwp_register_driver() the same way as for >>> powernow_register_driver(), and save one stub? ? >> >> Yes, and perhaps more. Maybe more stubs can be avoided? And >> acpi_cpufreq_driver doesn't need registering either, and hence >> would presumably be left unreferenced when !INTEL? >> > > {get,set}_hwp_para() can be avoided, as they're being called just once > and may be guarded by IS_ENABLED(CONFIG_INTEL). > The same for hwp_cmdline_parse(). > As for hwp_active() it's being used many times by generic cpufreq code > and even outside of cpufreq, so probably it has to be either a stub, or > be moved outside of hwp.c and become smth, like this: > > bool hwp_active(void) > { > return IS_ENABLED(CONFIG_INTEL) && hwp_in_use; > } > > Though I'm not sure such movement would be any better than a stub. > > acpi_cpufreq_driver, i.e. the most of code in cpufreq.c file, can > probably be separated into acpi.c and put under CONFIG_INTEL as well. > What you think of this? Sounds like the direction I think we want to be following. Jan
>> Though I'm not sure such movement would be any better than a stub. >> >> acpi_cpufreq_driver, i.e. the most of code in cpufreq.c file, can >> probably be separated into acpi.c and put under CONFIG_INTEL as well. >> What you think of this? > > Sounds like the direction I think we want to be following. > Sure, then I'll make a series for that. -Sergiy
diff --git a/xen/arch/x86/acpi/cpufreq/Makefile b/xen/arch/x86/acpi/cpufreq/Makefile index db83aa6b14..527ff20f5a 100644 --- a/xen/arch/x86/acpi/cpufreq/Makefile +++ b/xen/arch/x86/acpi/cpufreq/Makefile @@ -1,3 +1,3 @@ obj-y += cpufreq.o -obj-y += hwp.o -obj-y += powernow.o +obj-$(CONFIG_INTEL) += hwp.o +obj-$(CONFIG_AMD) += powernow.o diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c index a341f2f020..a89f3ed03a 100644 --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c @@ -657,7 +657,7 @@ static int __init cf_check cpufreq_driver_init(void) case X86_VENDOR_AMD: case X86_VENDOR_HYGON: - ret = powernow_register_driver(); + ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -ENODEV; break; } } diff --git a/xen/include/acpi/cpufreq/cpufreq.h b/xen/include/acpi/cpufreq/cpufreq.h index 443427153b..bc0c9a2b9f 100644 --- a/xen/include/acpi/cpufreq/cpufreq.h +++ b/xen/include/acpi/cpufreq/cpufreq.h @@ -252,6 +252,7 @@ void cpufreq_dbs_timer_resume(void); void intel_feature_detect(struct cpufreq_policy *policy); +#ifdef CONFIG_INTEL int hwp_cmdline_parse(const char *s, const char *e); int hwp_register_driver(void); bool hwp_active(void); @@ -260,4 +261,35 @@ int get_hwp_para(unsigned int cpu, int set_hwp_para(struct cpufreq_policy *policy, struct xen_set_cppc_para *set_cppc); +#else + +static inline int hwp_cmdline_parse(const char *s, const char *e) +{ + return -EINVAL; +} + +static inline int hwp_register_driver(void) +{ + return -ENODEV; +} + +static inline bool hwp_active(void) +{ + return false; +} + +static inline int get_hwp_para(unsigned int cpu, + struct xen_cppc_para *cppc_para) +{ + return -EINVAL; +} + +static inline int set_hwp_para(struct cpufreq_policy *policy, + struct xen_set_cppc_para *set_cppc) +{ + return -EINVAL; +} + +#endif /* CONFIG_INTEL */ + #endif /* __XEN_CPUFREQ_PM_H__ */
Build AMD Architectural P-state driver when CONFIG_AMD is on, and Intel Hardware P-States driver when CONFIG_INTEL is on respectively, allowing for a plaftorm-specific build. Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> CC: Jason Andryuk <jason.andryuk@amd.com> --- xen/arch/x86/acpi/cpufreq/Makefile | 4 ++-- xen/arch/x86/acpi/cpufreq/cpufreq.c | 2 +- xen/include/acpi/cpufreq/cpufreq.h | 32 +++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 3 deletions(-)