Re: [Xen-devel] [RFC PATCH 29/31] xen/arm: Introduce CPUFreq Interface component

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

Hi Stefano

On Wed, Dec 6, 2017 at 12:25 AM, Stefano Stabellini
<sstabellini@xxxxxxxxxx> wrote:
> On Thu, 9 Nov 2017, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>>
>> This patch adds an interface component which performs following steps:
>> 1. Initialize everything needed SCPI based CPUFreq driver to be functional
>>    (SCPI Message protocol, mailbox to communicate with SCP, etc).
>>    Also preliminary check if SCPI DVFS clock nodes offered by SCP are
>>    present in a device tree.
>> 2. Register SCPI based CPUFreq driver.
>> 3. Populate CPUs. Get DVFS info (OPP list and the latency information)
>>    for all DVFS capable CPUs using SCPI protocol, convert these capabilities
>>    into PM data the CPUFreq framework expects to see followed by
>>    uploading it.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Julien Grall <julien.grall@xxxxxxxxxx>
>> ---
>>  xen/arch/arm/cpufreq/cpufreq_if.c | 522 
>> ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 522 insertions(+)
>>  create mode 100644 xen/arch/arm/cpufreq/cpufreq_if.c
>>
>> diff --git a/xen/arch/arm/cpufreq/cpufreq_if.c 
>> b/xen/arch/arm/cpufreq/cpufreq_if.c
>> new file mode 100644
>> index 0000000..2451d00
>> --- /dev/null
>> +++ b/xen/arch/arm/cpufreq/cpufreq_if.c
>> @@ -0,0 +1,522 @@
>> +/*
>> + * xen/arch/arm/cpufreq/cpufreq_if.c
>> + *
>> + * CPUFreq interface component
>> + *
>> + * Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>> + * Copyright (c) 2017 EPAM Systems.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * 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, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <xen/device_tree.h>
>> +#include <xen/err.h>
>> +#include <xen/sched.h>
>> +#include <xen/cpufreq.h>
>> +#include <xen/pmstat.h>
>> +#include <xen/guest_access.h>
>> +
>> +#include "scpi_protocol.h"
>> +
>> +/*
>> + * TODO:
>> + * 1. Add __init to required funcs
>> + * 2. Put get_cpu_device() into common place
>> + */
>> +
>> +static struct scpi_ops *scpi_ops;
>> +
>> +extern int scpi_cpufreq_register_driver(void);
>> +
>> +#define dev_name(dev) dt_node_full_name(dev_to_dt(dev))
>> +
>> +struct device *get_cpu_device(unsigned int cpu)
>> +{
>> +    if ( cpu < nr_cpu_ids && cpu_possible(cpu) )
>> +        return dt_to_dev(cpu_dt_nodes[cpu]);
>> +    else
>> +        return NULL;
>> +}
>> +
>> +static bool is_dvfs_capable(unsigned int cpu)
>> +{
>> +    static const struct dt_device_match scpi_dvfs_clock_match[] =
>> +    {
>> +        DT_MATCH_COMPATIBLE("arm,scpi-dvfs-clocks"),
>> +        { /* sentinel */ },
>> +    };
>> +    struct device *cpu_dev;
>> +    struct dt_phandle_args clock_spec;
>> +    struct scpi_dvfs_info *info;
>> +    u32 domain;
>> +    int i, ret, count;
>> +
>> +    cpu_dev = get_cpu_device(cpu);
>> +    if ( !cpu_dev )
>> +    {
>> +        printk("cpu%d: failed to get device\n", cpu);
>> +        return false;
>> +    }
>> +
>> +    /* First of all find a clock node this CPU is a consumer of */
>> +    ret = dt_parse_phandle_with_args(cpu_dev->of_node,
>> +                                     "clocks",
>> +                                     "#clock-cells",
>> +                                     0,
>> +                                     &clock_spec);
>> +    if ( ret )
>> +    {
>> +        printk("cpu%d: failed to get clock node\n", cpu);
>> +        return false;
>> +    }
>> +
>> +    /* Make sure it is an available DVFS clock node */
>> +    if ( !dt_match_node(scpi_dvfs_clock_match, clock_spec.np) ||
>> +         !dt_device_is_available(clock_spec.np) )
>> +    {
>> +        printk("cpu%d: clock node '%s' is either non-DVFS or 
>> non-available\n",
>> +               cpu, dev_name(&clock_spec.np->dev));
>> +        return false;
>> +    }
>> +
>> +    /*
>> +     * Actually we already have a power domain id this CPU belongs to,
>> +     * it is a stored in args[0] CPU clock specifier, so we could ask SCP
>> +     * to provide its DVFS info. But we want to dig a little bit deeper
>> +     * to make sure that everything is correct.
>> +     */
>> +
>> +    /* Check how many clock ids a DVFS clock node has */
>> +    ret = dt_property_count_elems_of_size(clock_spec.np,
>> +                                          "clock-indices",
>> +                                          sizeof(u32));
>> +    if ( ret < 0 )
>> +    {
>> +        printk("cpu%d: failed to get clock-indices count in '%s'\n",
>> +               cpu, dev_name(&clock_spec.np->dev));
>> +        return false;
>> +    }
>> +    count = ret;
>> +
>> +    /* Check if a clock id the CPU clock specifier points to is present */
>> +    for ( i = 0; i < count; i++ )
>> +    {
>> +        ret = dt_property_read_u32_index(clock_spec.np,
>> +                                         "clock-indices",
>> +                                         i,
>> +                                         &domain);
>> +        if ( ret )
>> +        {
>> +            printk("cpu%d: failed to get clock index in '%s'\n",
>> +                   cpu, dev_name(&clock_spec.np->dev));
>> +            return false;
>> +        }
>> +
>> +        /* Match found */
>> +        if ( clock_spec.args[0] == domain )
>> +            break;
>> +    }
>> +
>> +    if ( i == count )
>> +    {
>> +        printk("cpu%d: failed to find matching clk_id (pd) %d\n",
>> +               cpu, clock_spec.args[0]);
>> +        return false;
>> +    }
>> +
>> +    /*
>> +     * Check if a SCP is aware of this power domain. SCPI Message protocol
>> +     * driver will populate power domain's DVFS info then.
>> +     */
>> +    info = scpi_ops->dvfs_get_info(domain);
>> +    if ( IS_ERR(info) )
>> +    {
>> +        printk("cpu%d: failed to get DVFS info of pd%u\n", cpu, domain);
>> +        return false;
>> +    }
>> +
>> +    printk(XENLOG_DEBUG "cpu%d: is DVFS capable, belongs to pd%u\n",
>> +           cpu, domain);
>> +
>> +    return true;
>> +}
>> +
>> +static int get_sharing_cpus(unsigned int cpu, cpumask_t *mask)
>> +{
>> +    struct device *cpu_dev = get_cpu_device(cpu), *tcpu_dev;
>> +    unsigned int tcpu;
>> +    int domain, tdomain;
>> +
>> +    BUG_ON(!cpu_dev);
>> +
>> +    domain = scpi_ops->device_domain_id(cpu_dev);
>> +    if ( domain < 0 )
>> +        return domain;
>> +
>> +    cpumask_clear(mask);
>> +    cpumask_set_cpu(cpu, mask);
>> +
>> +    for_each_online_cpu( tcpu )
>> +    {
>> +        if ( tcpu == cpu )
>> +            continue;
>> +
>> +        tcpu_dev = get_cpu_device(tcpu);
>> +        if ( !tcpu_dev )
>> +            continue;
>> +
>> +        tdomain = scpi_ops->device_domain_id(tcpu_dev);
>> +        if ( tdomain == domain )
>> +            cpumask_set_cpu(tcpu, mask);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int get_transition_latency(struct device *cpu_dev)
>> +{
>> +    return scpi_ops->get_transition_latency(cpu_dev);
>> +}
>> +
>> +static struct scpi_dvfs_info *get_dvfs_info(struct device *cpu_dev)
>> +{
>> +    int domain;
>> +
>> +    domain = scpi_ops->device_domain_id(cpu_dev);
>> +    if ( domain < 0 )
>> +        return ERR_PTR(-EINVAL);
>> +
>> +    return scpi_ops->dvfs_get_info(domain);
>> +}
>> +
>> +static int init_cpufreq_table(unsigned int cpu,
>> +                              struct cpufreq_frequency_table **table)
>> +{
>> +    struct cpufreq_frequency_table *freq_table = NULL;
>> +    struct device *cpu_dev = get_cpu_device(cpu);
>> +    struct scpi_dvfs_info *info;
>> +    struct scpi_opp *opp;
>> +    int i;
>> +
>> +    BUG_ON(!cpu_dev);
>> +
>> +    info = get_dvfs_info(cpu_dev);
>> +    if ( IS_ERR(info) )
>> +        return PTR_ERR(info);
>> +
>> +    if ( !info->opps )
>> +        return -EIO;
>> +
>> +    freq_table = xzalloc_array(struct cpufreq_frequency_table, info->count 
>> + 1);
>> +    if ( !freq_table )
>> +        return -ENOMEM;
>> +
>> +    for ( opp = info->opps, i = 0; i < info->count; i++, opp++ )
>> +    {
>> +        freq_table[i].index = i;
>> +        /* Convert Hz -> kHz */
>> +        freq_table[i].frequency = opp->freq / 1000;
>> +    }
>> +
>> +    freq_table[i].index = i;
>> +    freq_table[i].frequency = CPUFREQ_TABLE_END;
>> +
>> +    *table = &freq_table[0];
>> +
>> +    return 0;
>> +}
>> +
>> +static void free_cpufreq_table(struct cpufreq_frequency_table **table)
>> +{
>> +    if ( !table )
>> +        return;
>> +
>> +    xfree(*table);
>> +    *table = NULL;
>> +}
>> +
>> +static int upload_cpufreq_data(cpumask_t *mask,
>> +                               struct cpufreq_frequency_table *table)
>> +{
>> +    struct xen_processor_performance *perf;
>> +    struct xen_processor_px *states;
>> +    uint32_t platform_limit = 0, state_count = 0;
>> +    unsigned int max_freq = 0, prev_freq = 0, cpu = cpumask_first(mask);
>> +    int i, latency, ret = 0;
>> +
>> +    perf = xzalloc(struct xen_processor_performance);
>> +    if ( !perf )
>> +        return -ENOMEM;
>> +
>> +    /* Check frequency table and find max frequency */
>> +    for ( i = 0; (table[i].frequency != CPUFREQ_TABLE_END); i++ )
>> +    {
>> +        unsigned int freq = table[i].frequency;
>> +
>> +        if ( freq == CPUFREQ_ENTRY_INVALID )
>> +            continue;
>> +
>> +        if ( table[i].index != state_count || freq <= prev_freq )
>> +        {
>> +            printk("cpu%d: frequency table format error\n", cpu);
>> +            ret = -EINVAL;
>> +            goto out;
>> +        }
>> +
>> +        prev_freq = freq;
>> +        state_count++;
>> +        if ( freq > max_freq )
>> +            max_freq = freq;
>> +    }
>> +
>> +    /*
>> +     * The frequency table we have is just a temporary place for storing
>> +     * provided by SCP DVFS info. Create performance states array.
>> +     */
>> +    if ( !state_count )
>> +    {
>> +        printk("cpu%d: no available performance states\n", cpu);
>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
>> +
>> +    states = xzalloc_array(struct xen_processor_px, state_count);
>> +    if ( !states )
>> +    {
>> +        ret = -ENOMEM;
>> +        goto out;
>> +    }
>> +
>> +    set_xen_guest_handle(perf->states, states);
>
> this is the bit that should go away

Yes. To add some glue I put references:

This patch must be reworked:
https://www.mail-archive.com/xen-devel@xxxxxxxxxxxxx/msg128410.html

Here we have started discussion how to rework it:
https://marc.info/?l=xen-devel&m=151250698607186

>
>
>> +    perf->state_count = state_count;
>> +
>> +    latency = get_transition_latency(get_cpu_device(cpu));
>> +
>> +    /* Performance states must start from higher values */
>> +    for ( i = 0; (table[i].frequency != CPUFREQ_TABLE_END); i++ )
>> +    {
>> +        unsigned int freq = table[i].frequency;
>> +        unsigned int index = state_count - 1 - table[i].index;
>> +
>> +        if ( freq == CPUFREQ_ENTRY_INVALID )
>> +            continue;
>> +
>> +        if ( freq == max_freq )
>> +            platform_limit = index;
>> +
>> +        /* Convert kHz -> MHz */
>> +        states[index].core_frequency = freq / 1000;
>> +        /* Convert ns -> us */
>> +        states[index].transition_latency = DIV_ROUND_UP(latency, 1000);
>
> Why are we using DIV_ROUND_UP here and not in all the other frequency
> conversions?

I decided to use DIV_ROUND_UP here, since the latency theoretically
might be less then 1000 ns and
we might end up with 0 us using the simple division.

>
>
>> +    }
>> +
>> +    perf->flags = XEN_PX_DATA; /* all info in a one-shot */
>
> Please use existing flags

Yes, sure. As we have already agreed here:
https://marc.info/?l=xen-devel&m=151250698607186

>
>
>> +    perf->platform_limit = platform_limit;
>> +    perf->shared_type = CPUFREQ_SHARED_TYPE_ANY;
>> +    perf->domain_info.domain = cpumask_first(mask);
>> +    perf->domain_info.num_processors = cpumask_weight(mask);
>> +
>> +    /* Iterate through all CPUs which are on the same boat */
>> +    for_each_cpu( cpu, mask )
>> +    {
>> +        ret = set_px_pminfo(cpu, perf);
>> +        if ( ret )
>> +        {
>> +            printk("cpu%d: failed to set Px states (%d)\n", cpu, ret);
>> +            break;
>> +        }
>> +
>> +        printk(XENLOG_DEBUG "cpu%d: set Px states\n", cpu);
>> +    }
>> +
>> +    xfree(states);
>> +out:
>> +    xfree(perf);
>> +
>> +    return ret;
>> +}
>> +
>> +static int __init scpi_cpufreq_postinit(void)
>> +{
>> +    struct cpufreq_frequency_table *freq_table = NULL;
>> +    cpumask_t processed_cpus, shared_cpus;
>> +    unsigned int cpu;
>> +    int ret = -ENODEV;
>> +
>> +    cpumask_clear(&processed_cpus);
>> +
>> +    for_each_online_cpu( cpu )
>> +    {
>> +        if ( cpumask_test_cpu(cpu, &processed_cpus) )
>> +            continue;
>> +
>> +        if ( !is_dvfs_capable(cpu) )
>> +            continue;
>> +
>> +        ret = get_sharing_cpus(cpu, &shared_cpus);
>> +        if ( ret )
>> +        {
>> +            printk("cpu%d: failed to get sharing cpumask (%d)\n", cpu, ret);
>> +            return ret;
>> +        }
>> +
>> +        BUG_ON(cpumask_empty(&shared_cpus));
>> +        cpumask_or(&processed_cpus, &processed_cpus, &shared_cpus);
>> +
>> +        /* Create intermediate frequency table */
>> +        ret = init_cpufreq_table(cpu, &freq_table);
>> +        if ( ret )
>> +        {
>> +            printk("cpu%d: failed to initialize frequency table (%d)\n",
>> +                   cpu, ret);
>> +            return ret;
>> +        }
>> +
>> +        ret = upload_cpufreq_data(&shared_cpus, freq_table);
>> +        /* Destroy intermediate frequency table */
>> +        free_cpufreq_table(&freq_table);
>> +        if ( ret )
>> +        {
>> +            printk("cpu%d: failed to upload cpufreq data (%d)\n", cpu, ret);
>> +            return ret;
>> +        }
>> +
>> +        printk(XENLOG_DEBUG "cpu%d: uploaded cpufreq data\n", cpu);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static int __init scpi_cpufreq_preinit(void)
>> +{
>> +    struct dt_device_node *scpi, *clk, *dvfs_clk;
>> +    int ret;
>> +
>> +    /* Initialize SCPI Message protocol */
>> +    ret = scpi_init();
>> +    if ( ret )
>> +    {
>> +        printk("failed to initialize SCPI (%d)\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    /* Sanity check */
>> +    if ( !get_scpi_ops() || !get_scpi_dev() )
>> +        return -ENXIO;
>> +
>> +    scpi = get_scpi_dev()->of_node;
>> +    scpi_ops = get_scpi_ops();
>> +
>> +    ret = -ENODEV;
>> +
>> +    /*
>> +     * Check for clock related nodes for now. But it might additional nodes,
>> +     * like thermal sensor, etc.
>> +     */
>> +    dt_for_each_child_node( scpi, clk )
>
> Wouldn't it make sense to have a proper:
>
> DT_DEVICE_START
> ...
> DT_DEVICE_END
>
> block and register the driver that way?

I am not sure that I got your question completely.
Which driver need to be registered in a such way?
If we had separate dt-related driver to manage clocks we would have to
register it in a proposed way.
Here we just iterating through all SCPI child in order to be sure that
DVFS clock sub-node is present.
Let say, preliminary check.

BTW, in a proposed way I register ARM SMC triggered mailbox driver:
https://www.mail-archive.com/xen-devel@xxxxxxxxxxxxx/msg128411.html
With adding new DEVICE_MAILBOX class:
https://www.mail-archive.com/xen-devel@xxxxxxxxxxxxx/msg128402.html

>
>
>> +    {
>> +        /*
>> +         * First of all there must be a container node which contains all
>> +         * clocks provided by SCP.
>> +         */
>> +        if ( !dt_device_is_compatible(clk, "arm,scpi-clocks") )
>> +            continue;
>> +
>> +        /*
>> +         * As we are interested in DVFS feature only, check for DVFS clock
>> +         * sub-node. At the current stage check for it presence only.
>> +         * Without it there is no point to register SCPI based CPUFreq. We 
>> will
>> +         * perform a thorough check later when populating DVFS clock 
>> consumers.
>> +         */
>> +        dt_for_each_child_node( clk, dvfs_clk )
>> +        {
>> +            if ( !dt_device_is_compatible(dvfs_clk, "arm,scpi-dvfs-clocks") 
>> )
>> +                continue;
>> +
>> +            return 0;
>> +        }
>> +
>> +        break;
>> +    }
>> +
>> +    printk("failed to find SCPI DVFS clocks (%d)\n", ret);
>> +
>> +    return ret;
>> +}
>> +
>> +/* TODO Implement me */
>
> :-)
>
>
>> +static void scpi_cpufreq_deinit(void)
>> +{
>> +
>> +}
>> +
>> +static int __init cpufreq_driver_init(void)
>> +{
>> +    int ret;
>> +
>> +    if ( cpufreq_controller != FREQCTL_xen )
>> +        return 0;
>> +
>> +    /*
>> +     * Initialize everything needed SCPI based CPUFreq driver to be 
>> functional
>> +     * (SCPI Message protocol, mailbox to communicate with SCP, etc).
>> +     * Also preliminary check if SCPI DVFS clock nodes offered by SCP are
>> +     * present in a device tree.
>> +     */
>> +    ret = scpi_cpufreq_preinit();
>> +    if ( ret )
>> +        goto out;
>> +
>> +    /* Register SCPI based CPUFreq driver */
>> +    ret = scpi_cpufreq_register_driver();
>> +    if ( ret )
>> +        goto out;
>> +
>> +    /*
>> +     * Populate CPUs. Get DVFS info (OPP list and the latency information)
>> +     * for all DVFS capable CPUs using SCPI protocol, convert these 
>> capabilities
>> +     * into PM data the CPUFreq framework expects to see followed by
>> +     * uploading it.
>> +     *
>> +     * Actually it is almost the same PM data which hwdom uploads in case of
>> +     * x86 via platform hypercall after parsing ACPI tables. In our case we
>> +     * don't need hwdom to be involved in, since we already have everything 
>> in
>> +     * hand. Moreover, the hwdom doesn't even know anything about physical 
>> CPUs.
>> +     * Not completely sure that it is the best place to do so, but certainly
>> +     * it must be after driver registration.
>> +     */
>> +    ret = scpi_cpufreq_postinit();
>> +
>> +out:
>> +    if ( ret )
>> +    {
>> +        printk("failed to initialize SCPI based CPUFreq (%d)\n", ret);
>> +        scpi_cpufreq_deinit();
>> +        return ret;
>> +    }
>> +
>> +    printk("initialized SCPI based CPUFreq\n");
>> +
>> +    return 0;
>> +}
>> +__initcall(cpufreq_driver_init);
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel