Re: [RFC PATCH v3 10/12] cpufreq: add hwdom-cpufreq driver

[Julien Grall <julien.grall@linaro.org>]

Hi Oleksandr,

On 10/23/2014 04:07 PM, Oleksandr Dmytryshyn wrote:
> This driver uses hwdom to change frequencies on CPUs
> 
> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
> ---
>  xen/Rules.mk                        |   1 +
>  xen/drivers/cpufreq/Makefile        |   1 +
>  xen/drivers/cpufreq/hwdom-cpufreq.c | 220 ++++++++++++++++++++++++++++++++++++
>  xen/include/public/xen.h            |   1 +
>  4 files changed, 223 insertions(+)
>  create mode 100644 xen/drivers/cpufreq/hwdom-cpufreq.c
> 
> diff --git a/xen/Rules.mk b/xen/Rules.mk
> index 3b0b89b..cccbc72 100644
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -56,6 +56,7 @@ CFLAGS-$(perfc_arrays)  += -DPERF_ARRAYS
>  CFLAGS-$(lock_profile)  += -DLOCK_PROFILE
>  CFLAGS-$(HAS_ACPI)      += -DHAS_ACPI
>  CFLAGS-$(HAS_CPUFREQ)   += -DHAS_CPUFREQ
> +CFLAGS-$(HAS_HWDOM_CPUFREQ) += -DHAS_HWDOM_CPUFREQ
>  CFLAGS-$(HAS_PM)        += -DHAS_PM
>  CFLAGS-$(HAS_CPU_TURBO) += -DHAS_CPU_TURBO
>  CFLAGS-$(HAS_GDBSX)     += -DHAS_GDBSX
> diff --git a/xen/drivers/cpufreq/Makefile b/xen/drivers/cpufreq/Makefile
> index b87d127..891997c 100644
> --- a/xen/drivers/cpufreq/Makefile
> +++ b/xen/drivers/cpufreq/Makefile
> @@ -2,3 +2,4 @@ obj-y += cpufreq.o
>  obj-y += cpufreq_ondemand.o
>  obj-y += cpufreq_misc_governors.o
>  obj-y += utility.o
> +obj-$(HAS_HWDOM_CPUFREQ) += hwdom-cpufreq.o
> diff --git a/xen/drivers/cpufreq/hwdom-cpufreq.c b/xen/drivers/cpufreq/hwdom-cpufreq.c
> new file mode 100644
> index 0000000..67c9e1d
> --- /dev/null
> +++ b/xen/drivers/cpufreq/hwdom-cpufreq.c
> @@ -0,0 +1,220 @@
> +/*
> + *  Copyright (C) 2014 GlobalLogic Inc.

A part of this file has been copied from xen/arch/x86/acpi/cpufreq.c. I
would keep the copyright from this file and add yours.

Maybe we could share the initialization code (and others parts?) with
this file? For instance the structure looks the same...

> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or (at
> + *  your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +#include <xen/types.h>
> +#include <xen/errno.h>
> +#include <xen/sched.h>
> +#include <xen/event.h>
> +#include <xen/irq.h>
> +#include <xen/spinlock.h>
> +#include <xen/cpufreq.h>
> +#include <asm/current.h>
> +
> +struct hwdom_cpufreq_data {
> +    struct processor_performance *perf_data;
> +    struct cpufreq_frequency_table *freq_table;
> +};
> +
> +static struct hwdom_cpufreq_data *hwdom_cpufreq_drv_data[NR_CPUS];
> +
> +int cpufreq_cpu_init(unsigned int cpuid)
> +{
> +    return cpufreq_add_cpu(cpuid);
> +}
> +
> +static int hwdom_cpufreq_verify(struct cpufreq_policy *policy)
> +{
> +    struct hwdom_cpufreq_data *data;
> +    struct processor_performance *perf;
> +
> +    if ( !policy || !(data = hwdom_cpufreq_drv_data[policy->cpu]) ||
> +         !processor_pminfo[policy->cpu] )
> +        return -EINVAL;
> +
> +    perf = &processor_pminfo[policy->cpu]->perf;
> +
> +    cpufreq_verify_within_limits(policy, 0,
> +        perf->states[perf->platform_limit].core_frequency * 1000);

NIT: Missing space after the comma.

> +
> +    return cpufreq_frequency_table_verify(policy, data->freq_table);
> +}
> +
> +static int hwdom_cpufreq_target(struct cpufreq_policy *policy,
> +                               unsigned int target_freq, unsigned int relation)
> +{
> +    struct hwdom_cpufreq_data *data = hwdom_cpufreq_drv_data[policy->cpu];
> +    struct processor_performance *perf;
> +    struct cpufreq_freqs freqs;
> +    cpumask_t online_policy_cpus;
> +    unsigned int next_state = 0; /* Index into freq_table */
> +    unsigned int next_perf_state = 0; /* Index into perf table */
> +    unsigned int j;
> +    int ret = 0;
> +
> +    if ( unlikely(data == NULL ||
> +         data->perf_data == NULL || data->freq_table == NULL) )
> +    {
> +        return -ENODEV;
> +    }

NIT: The braces are not necessary.

> +
> +    perf = data->perf_data;
> +    ret = cpufreq_frequency_table_target(policy,
> +                                            data->freq_table,
> +                                            target_freq,
> +                                            relation, &next_state);

NIT: The alignment of the parameters don't look good.

> +    if ( unlikely(ret) )
> +        return -ENODEV;
> +
> +    cpumask_and(&online_policy_cpus, &cpu_online_map, policy->cpus);
> +
> +    next_perf_state = data->freq_table[next_state].index;
> +    if ( perf->state == next_perf_state )
> +    {
> +        if ( unlikely(policy->resume) )
> +            policy->resume = 0;
> +        else
> +            return 0;
> +    }
> +
> +    freqs.old = perf->states[perf->state].core_frequency * 1000;
> +    freqs.new = data->freq_table[next_state].frequency;
> +
> +    for_each_cpu( j, &online_policy_cpus )
> +        cpufreq_statistic_update(j, perf->state, next_perf_state);
> +
> +    perf->state = next_perf_state;
> +    policy->cur = freqs.new;
> +
> +    return ret;
> +}
> +
> +static int
> +hwdom_cpufreq_cpu_init(struct cpufreq_policy *policy)
> +{
> +    struct processor_performance *perf;
> +    struct hwdom_cpufreq_data *data;
> +    unsigned int cpu = policy->cpu;
> +    unsigned int valid_states = 0;
> +    int i;
> +    int ret = 0;
> +
> +    data = xzalloc(struct hwdom_cpufreq_data);
> +    if ( !data )
> +        return -ENOMEM;
> +
> +    hwdom_cpufreq_drv_data[cpu] = data;
> +
> +    data->perf_data = &processor_pminfo[cpu]->perf;
> +
> +    perf = data->perf_data;
> +    policy->shared_type = perf->shared_type;
> +
> +    data->freq_table = xmalloc_array(struct cpufreq_frequency_table,
> +                                    (perf->state_count+1));

NIT: Misaligned

> +    if ( !data->freq_table )
> +    {
> +        ret = -ENOMEM;
> +        goto err_unreg;
> +    }
> +
> +    /* detect transition latency */
> +    policy->cpuinfo.transition_latency = 0;
> +    for ( i=0; i<perf->state_count; i++ )

NIT: i < perf...

> +    {
> +        if ( (perf->states[i].transition_latency * 1000) >
> +             policy->cpuinfo.transition_latency )
> +            policy->cpuinfo.transition_latency =
> +                perf->states[i].transition_latency * 1000;
> +    }
> +
> +    policy->governor = cpufreq_opt_governor ? : CPUFREQ_DEFAULT_GOVERNOR;
> +
> +    /* table init */
> +    for ( i=0; i<perf->state_count; i++ )

Ditto

> +    {
> +        if ( i>0 && perf->states[i].core_frequency >=
> +            data->freq_table[valid_states-1].frequency / 1000 )
> +            continue;
> +
> +        data->freq_table[valid_states].index = i;
> +        data->freq_table[valid_states].frequency =
> +            perf->states[i].core_frequency * 1000;
> +        valid_states++;
> +    }
> +    data->freq_table[valid_states].frequency = CPUFREQ_TABLE_END;
> +    perf->state = 0;
> +
> +    ret = cpufreq_frequency_table_cpuinfo(policy, data->freq_table);
> +    if ( ret )
> +        goto err_freqfree;
> +
> +
> +    /*
> +     * the first call to ->target() should result in us actually
> +     * send command to the Dom0 to set frequency.
> +     */
> +    policy->resume = 1;
> +
> +    /* Set the minimal frequency */
> +    return hwdom_cpufreq_target(policy, policy->min, CPUFREQ_RELATION_L);
> +
> + err_freqfree:
> +    xfree(data->freq_table);
> + err_unreg:
> +    xfree(data);
> +    hwdom_cpufreq_drv_data[cpu] = NULL;
> +
> +    return ret;
> +}
> +
> +static int hwdom_cpufreq_cpu_exit(struct cpufreq_policy *policy)
> +{
> +    struct hwdom_cpufreq_data *data = hwdom_cpufreq_drv_data[policy->cpu];
> +
> +    if ( data )
> +    {
> +        hwdom_cpufreq_drv_data[policy->cpu] = NULL;
> +        xfree(data->freq_table);
> +        xfree(data);
> +    }
> +
> +    return 0;
> +}
> +
> +static struct cpufreq_driver hwdom_cpufreq_driver = {
> +    .name   = "hwdom-cpufreq",
> +    .verify = hwdom_cpufreq_verify,
> +    .target = hwdom_cpufreq_target,
> +    .init   = hwdom_cpufreq_cpu_init,
> +    .exit   = hwdom_cpufreq_cpu_exit,
> +};
> +
> +int __init hwdom_cpufreq_driver_init(void)

It looks like this function is only used for the __initcall. I would put
a static before.

Regards,

-- 
Julien Grall