Re: [PATCH v2 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver
From: skannan
Date: Mon May 21 2018 - 14:11:28 EST
On 2018-05-21 02:01, Viresh Kumar wrote:
On 19-05-18, 23:04, Taniya Das wrote:
The CPUfreq FW present in some QCOM chipsets offloads the steps
necessary
for changing the frequency of CPUs. The driver implements the cpufreq
driver interface for this firmware.
Signed-off-by: Saravana Kannan <skannan@xxxxxxxxxxxxxx>
Signed-off-by: Taniya Das <tdas@xxxxxxxxxxxxxx>
---
drivers/cpufreq/Kconfig.arm | 9 ++
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/qcom-cpufreq-fw.c | 317
++++++++++++++++++++++++++++++++++++++
3 files changed, 327 insertions(+)
create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 96b35b8..571f6b4 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -301,3 +301,12 @@ config ARM_PXA2xx_CPUFREQ
This add the CPUFreq driver support for Intel PXA2xx SOCs.
If in doubt, say N.
+
+config ARM_QCOM_CPUFREQ_FW
+ bool "QCOM CPUFreq FW driver"
During last review I didn't say that this driver shouldn't be a
module, but that you need to fix things to make it a module. I am fine
though if you don't want this to be a module ever.
+ help
+ Support for the CPUFreq FW driver.
+ The CPUfreq FW preset in some QCOM chipsets offloads the steps
+ necessary for changing the frequency of CPUs. The driver
+ implements the cpufreq driver interface for this firmware.
+ Say Y if you want to support CPUFreq FW.
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 8d24ade..a3edbce 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ) +=
tegra124-cpufreq.o
obj-$(CONFIG_ARM_TEGRA186_CPUFREQ) += tegra186-cpufreq.o
obj-$(CONFIG_ARM_TI_CPUFREQ) += ti-cpufreq.o
obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
+obj-$(CONFIG_ARM_QCOM_CPUFREQ_FW) += qcom-cpufreq-fw.o
##################################################################################
diff --git a/drivers/cpufreq/qcom-cpufreq-fw.c
b/drivers/cpufreq/qcom-cpufreq-fw.c
new file mode 100644
index 0000000..0e66de0
--- /dev/null
+++ b/drivers/cpufreq/qcom-cpufreq-fw.c
@@ -0,0 +1,317 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/cpufreq.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+
+#define INIT_RATE 300000000UL
+#define XO_RATE 19200000UL
+#define LUT_MAX_ENTRIES 40U
+#define CORE_COUNT_VAL(val) ((val & GENMASK(18, 16)) >> 16)
+#define LUT_ROW_SIZE 32
+
+struct cpufreq_qcom {
+ struct cpufreq_frequency_table *table;
+ struct device *dev;
+ void __iomem *perf_base;
+ void __iomem *lut_base;
+ cpumask_t related_cpus;
+ unsigned int max_cores;
+};
+
+static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
+
+static int
+qcom_cpufreq_fw_target_index(struct cpufreq_policy *policy, unsigned
int index)
+{
+ struct cpufreq_qcom *c = policy->driver_data;
+
+ if (index >= LUT_MAX_ENTRIES) {
+ dev_err(c->dev,
+ "Passing an index (%u) that's greater than max (%d)\n",
+ index, LUT_MAX_ENTRIES - 1);
+ return -EINVAL;
+ }
This is never going to happen unless there is a bug in cpufreq core.
You are allocating only 40 entries for the cpufreq table and this will
always be 0-39. None of the other drivers is checking this I believe
and neither should you. This is the only routine which will get call
very frequently and we better not add unnecessary stuff here.
+ writel_relaxed(index, c->perf_base);
+
+ /* Make sure the write goes through before proceeding */
+ mb();
Btw what happens right after this is done ? Are we guaranteed that the
frequency is updated in the hardware after this ? What about enabling
fast-switch for your platform ? Look at drivers/cpufreq/scmi-cpufreq.c
to see how that is done.
Yeah, I don't think this is needed really.
+ return 0;
+}
+
+static unsigned int qcom_cpufreq_fw_get(unsigned int cpu)
+{
+ struct cpufreq_qcom *c;
+ unsigned int index;
+
+ c = qcom_freq_domain_map[cpu];
+ if (!c)
+ return -ENODEV;
Return 0 on error here.
+
+ index = readl_relaxed(c->perf_base);
+ index = min(index, LUT_MAX_ENTRIES - 1);
Will the hardware ever read a value over 39 here ?
The register could be initialized to whatever before the kernel is
brought up. Don't want to depend on it being correct to avoid out of
bounds access that could leak data.
+
+ return c->table[index].frequency;
+}
+
+static int qcom_cpufreq_fw_cpu_init(struct cpufreq_policy *policy)
+{
+ struct cpufreq_qcom *c;
+
+ c = qcom_freq_domain_map[policy->cpu];
+ if (!c) {
+ pr_err("No scaling support for CPU%d\n", policy->cpu);
+ return -ENODEV;
+ }
+
+ cpumask_copy(policy->cpus, &c->related_cpus);
+ policy->freq_table = c->table;
+ policy->driver_data = c;
+
+ return 0;
+}
+
+static struct freq_attr *qcom_cpufreq_fw_attr[] = {
+ &cpufreq_freq_attr_scaling_available_freqs,
+ &cpufreq_freq_attr_scaling_boost_freqs,
+ NULL
+};
+
+static struct cpufreq_driver cpufreq_qcom_fw_driver = {
+ .flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
+ CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
+ .verify = cpufreq_generic_frequency_table_verify,
+ .target_index = qcom_cpufreq_fw_target_index,
+ .get = qcom_cpufreq_fw_get,
+ .init = qcom_cpufreq_fw_cpu_init,
+ .name = "qcom-cpufreq-fw",
+ .attr = qcom_cpufreq_fw_attr,
+ .boost_enabled = true,
+};
+
+static int qcom_read_lut(struct platform_device *pdev,
+ struct cpufreq_qcom *c)
+{
+ struct device *dev = &pdev->dev;
+ u32 data, src, lval, i, core_count, prev_cc;
+
+ c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
+ sizeof(*c->table), GFP_KERNEL);
+ if (!c->table)
+ return -ENOMEM;
+
+ for (i = 0; i < LUT_MAX_ENTRIES; i++) {
+ data = readl_relaxed(c->lut_base + i * LUT_ROW_SIZE);
+ src = ((data & GENMASK(31, 30)) >> 30);
+ lval = (data & GENMASK(7, 0));
+ core_count = CORE_COUNT_VAL(data);
+
+ if (!src)
+ c->table[i].frequency = INIT_RATE / 1000;
+ else
+ c->table[i].frequency = XO_RATE * lval / 1000;
+
+ c->table[i].driver_data = c->table[i].frequency;
Why do you need to use driver_data here? Why can't you simple use
frequency field in the below conditional expressions ?
+
+ dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
+ i, c->table[i].frequency, core_count);
+
+ if (core_count != c->max_cores)
+ c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
+
+ /*
+ * Two of the same frequencies with the same core counts means
+ * end of table.
+ */
+ if (i > 0 && c->table[i - 1].driver_data ==
+ c->table[i].driver_data && prev_cc == core_count) {
+ struct cpufreq_frequency_table *prev = &c->table[i - 1];
+
+ if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
There can only be a single boost frequency at max ?
As of now, yes. If that changes, we'll change this code later.
+ prev->flags = CPUFREQ_BOOST_FREQ;
+ prev->frequency = prev->driver_data;
Okay you are using driver_data as a local variable to keep this value
safe which you might have overwritten. Maybe use a simple variable
prev_freq for this. It would be much more readable in that case and
you wouldn't end up abusing the driver_data field.
+ }
+
+ break;
+ }
+ prev_cc = core_count;
+ }
+ c->table[i].frequency = CPUFREQ_TABLE_END;
Wouldn't you end up writing on c->table[40].frequency here if there
are 40 frequency value present ?
Yeah, the loop condition needs to be fixed.
-Saravana