Re: [PATCH v2 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver
From: Taniya Das
Date: Tue May 22 2018 - 05:49:58 EST
Hello Viresh,
Thanks for your comments.
On 5/22/2018 12:36 AM, skannan@xxxxxxxxxxxxxx wrote:
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.
Yes, I would remove this in the next series.
+ÂÂÂ 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.
Just want to make sure it doesn't really sit in the write buffer before
return.
+ÂÂÂ 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.
Would update this in the next patch.
+
+ÂÂÂ 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 ?
The frequency field would be marked INVALID in case the core count does
not match and the frequency data would be lost.
+
+ÂÂÂÂÂÂÂ 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.
Please correct me, currently the driver_data is not used by cpufreq core
and that was the reason to use it. In case you still think it is not a
good way to handle it, I would try to handle it differently.
+ÂÂÂÂÂÂÂÂÂÂÂ }
+
+ÂÂÂÂÂÂÂÂÂÂÂ 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.
The table allocation is done for 'LUT_MAX_ENTRIES + 1'.
Yes in case we have all [0-39] (i.e 40 entries) read from the hardware,
would store the same and mark the 40th index as table end. Please
correct if I missed something in your comment.
-Saravana
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
--