Re: [PATCH v7 1/6] soc: qcom: cpr: Move common functions to new file
From: Vladimir Zapolskiy
Date: Fri Oct 15 2021 - 07:15:53 EST
Hi AngeloGioacchino,
On 9/1/21 6:57 PM, AngeloGioacchino Del Regno wrote:
In preparation for implementing a new driver that will be handling
CPRv3, CPRv4 and CPR-Hardened, format out common functions to a new
file.
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxxx>
---
drivers/soc/qcom/Makefile | 2 +-
drivers/soc/qcom/cpr-common.c | 362 ++++++++++++++++++++++++++++++
drivers/soc/qcom/cpr-common.h | 111 ++++++++++
drivers/soc/qcom/cpr.c | 399 ++--------------------------------
4 files changed, 497 insertions(+), 377 deletions(-)
create mode 100644 drivers/soc/qcom/cpr-common.c
create mode 100644 drivers/soc/qcom/cpr-common.h
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index ad675a6593d0..8d1262a2e23c 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -3,7 +3,7 @@ CFLAGS_rpmh-rsc.o := -I$(src)
obj-$(CONFIG_QCOM_AOSS_QMP) += qcom_aoss.o
obj-$(CONFIG_QCOM_GENI_SE) += qcom-geni-se.o
obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
-obj-$(CONFIG_QCOM_CPR) += cpr.o
+obj-$(CONFIG_QCOM_CPR) += cpr-common.o cpr.o
obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o
obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o
obj-$(CONFIG_QCOM_OCMEM) += ocmem.o
diff --git a/drivers/soc/qcom/cpr-common.c b/drivers/soc/qcom/cpr-common.c
new file mode 100644
index 000000000000..41fcc5863e72
--- /dev/null
+++ b/drivers/soc/qcom/cpr-common.c
@@ -0,0 +1,362 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2019, Linaro Limited
+ */
+
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/debugfs.h>
+#include <linux/string.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/bitops.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_opp.h>
+#include <linux/interrupt.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
Please consider to sort the list of included headers alphabetically,
also I find that some of them are not needed, for instance the two
includes above are definitely excessive.
I understand that the list have been just copied, nevertheless, it
might be a good opportunity to shrink it.
+#include <linux/regulator/consumer.h>
+#include <linux/clk.h>
+#include <linux/nvmem-consumer.h>
+#include "cpr-common.h"
+
+int cpr_populate_ring_osc_idx(struct device *dev,
+ struct fuse_corner *fuse_corner,
+ const struct cpr_fuse *cpr_fuse,
+ int num_fuse_corners)
+{
+ struct fuse_corner *end = fuse_corner + num_fuse_corners;
+ u32 data;
+ int ret;
+
+ for (; fuse_corner < end; fuse_corner++, cpr_fuse++) {
+ ret = nvmem_cell_read_variable_le_u32(dev, cpr_fuse->ring_osc, &data);
+ if (ret)
+ return ret;
+ fuse_corner->ring_osc_idx = data;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(cpr_populate_ring_osc_idx);
+
+int cpr_read_fuse_uV(int init_v_width, int step_size_uV, int ref_uV,
+ int adj, int step_volt, const char *init_v_efuse,
+ struct device *dev)
+{
+ int steps, uV;
+ u32 bits = 0;
+ int ret;
+
+ ret = nvmem_cell_read_variable_le_u32(dev, init_v_efuse, &bits);
+ if (ret)
+ return ret;
+
+ steps = bits & (BIT(init_v_width - 1) - 1);
+ /* Not two's complement.. instead highest bit is sign bit */
+ if (bits & BIT(init_v_width - 1))
+ steps = -steps;
+
+ uV = ref_uV + steps * step_size_uV;
+
+ /* Apply open-loop fixed adjustments to fused values */
+ uV += adj;
+
+ return DIV_ROUND_UP(uV, step_volt) * step_volt;
+}
+EXPORT_SYMBOL_GPL(cpr_read_fuse_uV);
This function should be static and unexported, since I see no its users
outside of the object scope.
+const struct cpr_fuse *cpr_get_fuses(struct device *dev, int tid,
+ int num_fuse_corners)
From struct cpr_thread_desc the field 'num_fuse_corners' is unsigned,
please consider to change the type here to unsigned also.
+{
+ struct cpr_fuse *fuses;
+ int i;
Then 'i' can be changed to unsigned also.
+
+ fuses = devm_kcalloc(dev, num_fuse_corners,
+ sizeof(struct cpr_fuse),
A common practice is to specify the size as 'sizeof(*fuses)'.
+ GFP_KERNEL);
+ if (!fuses)
+ return ERR_PTR(-ENOMEM);
+
+ for (i = 0; i < num_fuse_corners; i++) {
+ char tbuf[50];
+
+ snprintf(tbuf, sizeof(tbuf), "cpr%d_ring_osc%d", tid, i + 1);
+ fuses[i].ring_osc = devm_kstrdup(dev, tbuf, GFP_KERNEL);
+ if (!fuses[i].ring_osc)
+ return ERR_PTR(-ENOMEM);
+
+ snprintf(tbuf, sizeof(tbuf),
+ "cpr%d_init_voltage%d", tid, i + 1);
+ fuses[i].init_voltage = devm_kstrdup(dev, tbuf,
+ GFP_KERNEL);
It should fit into one line, no need to wrap.
+ if (!fuses[i].init_voltage)
+ return ERR_PTR(-ENOMEM);
+
+ snprintf(tbuf, sizeof(tbuf), "cpr%d_quotient%d", tid, i + 1);
+ fuses[i].quotient = devm_kstrdup(dev, tbuf, GFP_KERNEL);
+ if (!fuses[i].quotient)
+ return ERR_PTR(-ENOMEM);
+
+ snprintf(tbuf, sizeof(tbuf),
+ "cpr%d_quotient_offset%d", tid, i + 1);
+ fuses[i].quotient_offset = devm_kstrdup(dev, tbuf,
+ GFP_KERNEL);
Here as well.
+ if (!fuses[i].quotient_offset)
+ return ERR_PTR(-ENOMEM);
+ }
+
+ return fuses;
+}
+EXPORT_SYMBOL_GPL(cpr_get_fuses);
+
+int cpr_populate_fuse_common(struct device *dev,
+ struct fuse_corner_data *fdata,
+ const struct cpr_fuse *cpr_fuse,
+ struct fuse_corner *fuse_corner,
+ int step_volt, int init_v_width,
+ int init_v_step)
+{
+ int uV, ret;
+
+ /* Populate uV */
+ uV = cpr_read_fuse_uV(init_v_width, init_v_step,
+ fdata->ref_uV, fdata->volt_oloop_adjust,
+ step_volt, cpr_fuse->init_voltage, dev);
+ if (uV < 0)
+ return uV;
+
+ /*
+ * Update SoC voltages: platforms might choose a different
+ * regulators than the one used to characterize the algorithms
+ * (ie, init_voltage_step).
+ */
+ fdata->min_uV = roundup(fdata->min_uV, step_volt);
+ fdata->max_uV = roundup(fdata->max_uV, step_volt);
+
+ fuse_corner->min_uV = fdata->min_uV;
+ fuse_corner->max_uV = fdata->max_uV;
+ fuse_corner->uV = clamp(uV, fuse_corner->min_uV, fuse_corner->max_uV);
+
+ /* Populate target quotient by scaling */
+ ret = nvmem_cell_read_variable_le_u32(dev, cpr_fuse->quotient, &fuse_corner->quot);
+ if (ret)
+ return ret;
+
+ fuse_corner->quot *= fdata->quot_scale;
+ fuse_corner->quot += fdata->quot_offset;
+ fuse_corner->quot += fdata->quot_adjust;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(cpr_populate_fuse_common);
+
+/*
+ * Returns: Index of the initial corner or negative number for error.
+ */
+int cpr_find_initial_corner(struct device *dev, struct clk *cpu_clk,
+ struct corner *corners, int num_corners)
+{
+ unsigned long rate;
+ struct corner *iter, *corner;
+ const struct corner *end;
+ unsigned int ret = 0;
+
+ if (!cpu_clk)
+ return -EINVAL;
+
+ end = &corners[num_corners - 1];
+ rate = clk_get_rate(cpu_clk);
+
+ /*
+ * Some bootloaders set a CPU clock frequency that is not defined
+ * in the OPP table. When running at an unlisted frequency,
+ * cpufreq_online() will change to the OPP which has the lowest
+ * frequency, at or above the unlisted frequency.
+ * Since cpufreq_online() always "rounds up" in the case of an
+ * unlisted frequency, this function always "rounds down" in case
+ * of an unlisted frequency. That way, when cpufreq_online()
+ * triggers the first ever call to cpr_set_performance_state(),
+ * it will correctly determine the direction as UP.
+ */
+ for (iter = corners; iter <= end; iter++) {
+ if (iter->freq > rate)
+ break;
+ ret++;
+ if (iter->freq == rate) {
+ corner = iter;
+ break;
+ }
+ if (iter->freq < rate)
+ corner = iter;
+ }
+
+ if (!corner) {
+ dev_err(dev, "boot up corner not found\n");
+ return -EINVAL;
+ }
+
+ dev_dbg(dev, "boot up perf state: %u\n", ret);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(cpr_find_initial_corner);
+
+u32 cpr_get_fuse_corner(struct dev_pm_opp *opp, u32 tid)
+{
+ struct device_node *np;
+ u32 fc;
+
+ np = dev_pm_opp_get_of_node(opp);
+ if (of_property_read_u32_index(np, "qcom,opp-fuse-level", tid, &fc)) {
+ pr_debug("%s: missing 'qcom,opp-fuse-level' property\n",
+ __func__);
+ fc = 0;
+ }
+
+ of_node_put(np);
+
+ return fc;
+}
+EXPORT_SYMBOL_GPL(cpr_get_fuse_corner);
+
+unsigned long cpr_get_opp_hz_for_req(struct dev_pm_opp *ref,
+ struct device *cpu_dev)
+{
+ u64 rate = 0;
+ struct device_node *ref_np;
+ struct device_node *desc_np;
+ struct device_node *child_np = NULL;
+ struct device_node *child_req_np = NULL;
+
+ desc_np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
+ if (!desc_np)
+ return 0;
+
+ ref_np = dev_pm_opp_get_of_node(ref);
+ if (!ref_np)
+ goto out_ref;
+
+ do {
+ of_node_put(child_req_np);
+ child_np = of_get_next_available_child(desc_np, child_np);
+ child_req_np = of_parse_phandle(child_np, "required-opps", 0);
+ } while (child_np && child_req_np != ref_np);
There is a potential to reuse for_each_available_child_of_node() helper.
+
+ if (child_np && child_req_np == ref_np)
+ of_property_read_u64(child_np, "opp-hz", &rate);
+
+ of_node_put(child_req_np);
+ of_node_put(child_np);
+ of_node_put(ref_np);
+out_ref:
+ of_node_put(desc_np);
+
+ return (unsigned long) rate;
+}
+EXPORT_SYMBOL_GPL(cpr_get_opp_hz_for_req);
+
+int cpr_calculate_scaling(const char *quot_offset,
+ struct device *dev,
Please consider to make 'dev' argument as the first one in the list.
+ const struct fuse_corner_data *fdata,
+ const struct corner *corner)
+{
+ u32 quot_diff = 0;
+ unsigned long freq_diff;
+ int scaling;
+ const struct fuse_corner *fuse, *prev_fuse;
+ int ret;
I personally prefer a Reverse Christmas Tree Format ordering of local
variable declarations (see https://lwn.net/Articles/758552), not sure
if you'd prefer to follow it...
+
+ fuse = corner->fuse_corner;
+ prev_fuse = fuse - 1;
+
+ if (quot_offset) {
+ ret = nvmem_cell_read_variable_le_u32(dev, quot_offset, "_diff);
+ if (ret)
+ return ret;
+
+ quot_diff *= fdata->quot_offset_scale;
+ quot_diff += fdata->quot_offset_adjust;
+ } else {
+ quot_diff = fuse->quot - prev_fuse->quot;
+ }
+
+ freq_diff = fuse->max_freq - prev_fuse->max_freq;
+ freq_diff /= 1000000; /* Convert to MHz */
+ scaling = 1000 * quot_diff / freq_diff;
+ return min(scaling, fdata->max_quot_scale);
+}
+EXPORT_SYMBOL_GPL(cpr_calculate_scaling);
+
+int cpr_interpolate(const struct corner *corner, int step_volt,
+ const struct fuse_corner_data *fdata)
+{
+ unsigned long f_high, f_low, f_diff;
+ int uV_high, uV_low, uV;
+ u64 temp, temp_limit;
+ const struct fuse_corner *fuse, *prev_fuse;
+
+ fuse = corner->fuse_corner;
+ prev_fuse = fuse - 1;
+
+ f_high = fuse->max_freq;
+ f_low = prev_fuse->max_freq;
+ uV_high = fuse->uV;
+ uV_low = prev_fuse->uV;
+ f_diff = fuse->max_freq - corner->freq;
+
+ /*
+ * Don't interpolate in the wrong direction. This could happen
+ * if the adjusted fuse voltage overlaps with the previous fuse's
+ * adjusted voltage.
+ */
+ if (f_high <= f_low || uV_high <= uV_low || f_high <= corner->freq)
+ return corner->uV;
+
+ temp = f_diff * (uV_high - uV_low);
+ do_div(temp, f_high - f_low);
+
+ /*
+ * max_volt_scale has units of uV/MHz while freq values
+ * have units of Hz. Divide by 1000000 to convert to.
+ */
+ temp_limit = f_diff * fdata->max_volt_scale;
+ do_div(temp_limit, 1000000);
+
+ uV = uV_high - min(temp, temp_limit);
+ return roundup(uV, step_volt);
+}
+EXPORT_SYMBOL_GPL(cpr_interpolate);
+
+int cpr_check_vreg_constraints(struct device *dev, struct regulator *vreg,
+ struct fuse_corner *f)
+{
+ int ret;
+
+ ret = regulator_is_supported_voltage(vreg, f->min_uV, f->min_uV);
+ if (!ret) {
+ dev_err(dev, "min uV: %d not supported by regulator\n",
+ f->min_uV);
+ return -EINVAL;
+ }
+
+ ret = regulator_is_supported_voltage(vreg, f->max_uV, f->max_uV);
+ if (!ret) {
+ dev_err(dev, "max uV: %d not supported by regulator\n",
+ f->max_uV);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(cpr_check_vreg_constraints);
+
+MODULE_DESCRIPTION("Core Power Reduction (CPR) common");
+MODULE_LICENSE("GPL v2");
--
Best wishes,
Vladimir