lkml.org 
[lkml]   [2020]   [Jun]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [TEGRA194_CPUFREQ Patch v3 3/4] cpufreq: Add Tegra194 cpufreq driver
From
Date
Hi Viresh,

Thank you for the review. please find my reply inline.


>> +++ b/drivers/cpufreq/tegra194-cpufreq.c
>> @@ -0,0 +1,403 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved
>
> 2020
>
>> + */
>> +
>> +#include <linux/cpu.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/delay.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +
>> +#include <asm/smp_plat.h>
>> +
>> +#include <soc/tegra/bpmp.h>
>> +#include <soc/tegra/bpmp-abi.h>
>> +
>> +#define KHZ 1000
>> +#define REF_CLK_MHZ 408 /* 408 MHz */
>> +#define US_DELAY 500
>> +#define US_DELAY_MIN 2
>> +#define CPUFREQ_TBL_STEP_HZ (50 * KHZ * KHZ)
>> +#define MAX_CNT ~0U
>> +
>> +/* cpufreq transisition latency */
>> +#define TEGRA_CPUFREQ_TRANSITION_LATENCY (300 * 1000) /* unit in nanoseconds */
>> +
>> +#define LOOP_FOR_EACH_CPU_OF_CLUSTER(cl) for (cpu = (cl * 2); \
>> + cpu < ((cl + 1) * 2); cpu++)
>
> Both latency and this loop are used only once in the code, maybe just open code
> it. Also you should have passed cpu as a parameter to the macro, even if it
> works fine without it, for better readability.
>
Ok, i will open code the loop in next version. For latency value, i feel
named macro makes readability better. So, prefer keeping it.

>> +
>> +u16 map_freq_to_ndiv(struct mrq_cpu_ndiv_limits_response *nltbl, u32 freq)
>
> Unused routine
>
Sure, will remove it.

>> +{
>> + return DIV_ROUND_UP(freq * nltbl->pdiv * nltbl->mdiv,
>> + nltbl->ref_clk_hz / KHZ);
>> +}
>
>> +static int tegra194_cpufreq_init(struct cpufreq_policy *policy)
>> +{
>> + struct tegra194_cpufreq_data *data = cpufreq_get_driver_data();
>> + int cl = get_cpu_cluster(policy->cpu);
>> + u32 cpu;
>> +
>> + if (cl >= data->num_clusters)
>> + return -EINVAL;
>> +
>> + policy->cur = tegra194_fast_get_speed(policy->cpu); /* boot freq */
>> +
>> + /* set same policy for all cpus in a cluster */
>> + LOOP_FOR_EACH_CPU_OF_CLUSTER(cl)
>> + cpumask_set_cpu(cpu, policy->cpus);
>> +
>> + policy->freq_table = data->tables[cl];
>> + policy->cpuinfo.transition_latency = TEGRA_CPUFREQ_TRANSITION_LATENCY;
>> +
>> + return 0;
>> +}
>
>> +static int tegra194_cpufreq_set_target(struct cpufreq_policy *policy,
>> + unsigned int index)
>> +{
>> + struct cpufreq_frequency_table *tbl = policy->freq_table + index;
>> +
>> + on_each_cpu_mask(policy->cpus, set_cpu_ndiv, tbl, true);
>
> I am still a bit confused. While setting the frequency you are calling this
> routine for each CPU of the policy (cluster). Does that mean that CPUs within a
> cluster can actually run at different frequencies at any given point of time ?
>
> If cpufreq terms, a cpufreq policy represents a group of CPUs that change
> frequency together, i.e. they share the clk line. If all CPUs in your system can
> do DVFS separately, then you must have policy per CPU, instead of cluster.
>
T194 supports four CPU clusters, each with two cores. Each CPU cluster
is capable of running at a specific frequency sourced by respective
NAFLL to provide cluster specific clocks. Individual cores within a
cluster write freq in per core register. Cluster h/w forwards the
max(core0, core1) request to per cluster NAFLL.

>> +static void tegra194_cpufreq_free_resources(void)
>> +{
>> + flush_workqueue(read_counters_wq);
>
> Why is this required exactly? I see that you add the work request and
> immediately flush it, then why would you need to do this separately ?
>
Ya, will remove flush_workqueue().

>> + destroy_workqueue(read_counters_wq);
>> +}
>> +
>> +static struct cpufreq_frequency_table *
>> +init_freq_table(struct platform_device *pdev, struct tegra_bpmp *bpmp,
>> + unsigned int cluster_id)
>> +{
>> + struct cpufreq_frequency_table *freq_table;
>> + struct mrq_cpu_ndiv_limits_response resp;
>> + unsigned int num_freqs, ndiv, delta_ndiv;
>> + struct mrq_cpu_ndiv_limits_request req;
>> + struct tegra_bpmp_message msg;
>> + u16 freq_table_step_size;
>> + int err, index;
>> +
>> + memset(&req, 0, sizeof(req));
>> + req.cluster_id = cluster_id;
>> +
>> + memset(&msg, 0, sizeof(msg));
>> + msg.mrq = MRQ_CPU_NDIV_LIMITS;
>> + msg.tx.data = &req;
>> + msg.tx.size = sizeof(req);
>> + msg.rx.data = &resp;
>> + msg.rx.size = sizeof(resp);
>> +
>> + err = tegra_bpmp_transfer(bpmp, &msg);
>
> So the firmware can actually return different frequency tables for the clusters,
> right ? Else you could have received the table only once and used it for all the
> CPUs.
>
Yes, frequency tables are returned per cluster by BPMP firmware. In T194
SOC, currently same table values are used for all clusters. This might
change in future.

>> + if (err)
>> + return ERR_PTR(err);
>> +
>> + /*
>> + * Make sure frequency table step is a multiple of mdiv to match
>> + * vhint table granularity.
>> + */
>> + freq_table_step_size = resp.mdiv *
>> + DIV_ROUND_UP(CPUFREQ_TBL_STEP_HZ, resp.ref_clk_hz);
>> +
>> + dev_dbg(&pdev->dev, "cluster %d: frequency table step size: %d\n",
>> + cluster_id, freq_table_step_size);
>> +
>> + delta_ndiv = resp.ndiv_max - resp.ndiv_min;
>> +
>> + if (unlikely(delta_ndiv == 0))
>> + num_freqs = 1;
>> + else
>> + /* We store both ndiv_min and ndiv_max hence the +1 */
>> + num_freqs = delta_ndiv / freq_table_step_size + 1;
>> +
>> + num_freqs += (delta_ndiv % freq_table_step_size) ? 1 : 0;
>> +
>> + freq_table = devm_kcalloc(&pdev->dev, num_freqs + 1,
>> + sizeof(*freq_table), GFP_KERNEL);
>> + if (!freq_table)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + for (index = 0, ndiv = resp.ndiv_min;
>> + ndiv < resp.ndiv_max;
>> + index++, ndiv += freq_table_step_size) {
>> + freq_table[index].driver_data = ndiv;
>> + freq_table[index].frequency = map_ndiv_to_freq(&resp, ndiv);
>> + }
>> +
>> + freq_table[index].driver_data = resp.ndiv_max;
>> + freq_table[index++].frequency = map_ndiv_to_freq(&resp, resp.ndiv_max);
>> + freq_table[index].frequency = CPUFREQ_TABLE_END;
>> +
>> + return freq_table;
>> +}
>
> --
> viresh
>

\
 
 \ /
  Last update: 2020-06-23 07:19    [W:0.083 / U:0.408 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site