Add support for QCOM cpufreq FW driver

Add QCOM cpufreq firmware device bindings for Qualcomm Technology Inc's SoCs. This is required for managing the cpu frequency transitions which are controlled by firmware.

Signed-off-by Taniya Das <[email protected]>

---
 .../bindings/cpufreq/cpufreq-qcom-fw.txt           | 68 ++++++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +1 -0

new file mode 100644
index 0000000..bc912f4
@@ -0,0 +1,68 @@
+Qualcomm Technologies, Inc. CPUFREQ Bindings

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +19 -1

+
+		freq-domain-0 {
+			compatible = "cpufreq";
+			reg = <0x17d43920 0x4>,
+			     <0x17d43110 0x500>,
+			     <0x17d41000 0x4>;
+			reg-names = "perf_base", "lut_base", "en_base";
+			qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3>;
+		};
+
+		freq-domain-1 {
+			compatible = "cpufreq";
+			reg = <0x17d46120 0x4>,
+			    <0x17d45910 0x500>,
+			    <0x17d45800 0x4>;
+			reg-names = "perf_base", "lut_base", "en_base";
+			qcom,cpulist = <&CPU4 &CPU5 &CPU6 &CPU7>;
+		};
+	};
--

Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member of the Code Aurora Forum, hosted by the Linux Foundation.

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt line 57

Quoted 7 lines · Taniya Das

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +7 -0

+		freq-domain-0 {
+			compatible = "cpufreq";
+			reg = <0x17d43920 0x4>,
+			     <0x17d43110 0x500>,
+			     <0x17d41000 0x4>;
+			reg-names = "perf_base", "lut_base", "en_base";
+			qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3>;

Quoted 8 lines · Rob Herring

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt line 57

Quoted 7 lines · Taniya Das

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +7 -0

+           freq-domain-0 {
+                   compatible = "cpufreq";
+                   reg = <0x17d43920 0x4>,
+                        <0x17d43110 0x500>,
+                        <0x17d41000 0x4>;
+                   reg-names = "perf_base", "lut_base", "en_base";
+                   qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3>;

I was thinking, can't we add platform specific properties in the CPU nodes ? If yes, then we can point the phandle of fw node from the CPUs and this awkward list can go away.

Quoted 10 lines · Viresh Kumar

Quoted 8 lines · Rob Herring

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt line 57

Quoted 7 lines · Taniya Das

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +7 -0

+           freq-domain-0 {
+                   compatible = "cpufreq";
+                   reg = <0x17d43920 0x4>,
+                        <0x17d43110 0x500>,
+                        <0x17d41000 0x4>;
+                   reg-names = "perf_base", "lut_base", "en_base";
+                   qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3>;

Quoted 3 lines

I was thinking, can't we add platform specific properties in the CPU nodes ? If yes, then we can point the phandle of fw node from the CPUs and this awkward list can go away.

Yes, that's fine. That would be more like OPP binding in that the CPU points to the OPP table rather than the OPP pointing to the CPUs.

With that, you can get rid of the child nodes completely. Just make the parent reg property N sets of 3 addresses for N domains.

Rob

Hello Rob, Viresh,

Thank you for the comments. If I understand correctly, the device tree nodes should look something like the below.

Though I am not sure if any vendor name could be associated in the cpu nodes.

Please suggest if my understanding is wrong.

cpu@0 {
	qcom,freq-domain = <&freq_domain_table0>;
	…
};

same follows for cpu 1/2/3

cpu@400 {
	qcom,freq-domain = <&freq_domain_table1>;
	…
};

same follows for cpu 5/6/7

freq_domain_table0 : freq_table {
	reg = < >, < >, < > ;
	reg-names = "perf_base", "lut_base", "en_base";
};

freq_domain_table1 : freq_table {
	reg = < >, < >, < > ;
	reg-names = "perf_base", "lut_base", "en_base";
};

Quoted 7 lines · Taniya Das

Hello Rob, Viresh,

Thank you for the comments. If I understand correctly, the device tree nodes should look something like the below.

Though I am not sure if any vendor name could be associated in the cpu nodes.

Well Rob said Yes to that I think.

Quoted 20 lines

Please suggest if my understanding is wrong.

cpu@0 {
	qcom,freq-domain = <&freq_domain_table0>;
	…
};

same follows for cpu 1/2/3

cpu@400 {
	qcom,freq-domain = <&freq_domain_table1>;
	…
};

same follows for cpu 5/6/7

freq_domain_table0 : freq_table {
	reg = < >, < >, < > ;
	reg-names = "perf_base", "lut_base", "en_base";
};

Quoted 4 lines

freq_domain_table1 : freq_table {
	reg = < >, < >, < > ;
	reg-names = "perf_base", "lut_base", "en_base";
};

Mostly yes.

Add QCOM cpufreq firmware device bindings for Qualcomm Technology Inc's SoCs. This is required for managing the cpu frequency transitions which are controlled by firmware.

Signed-off-by Taniya Das <[email protected]>

---
 .../bindings/cpufreq/cpufreq-qcom-fw.txt           | 68 ++++++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +2 -0

new file mode 100644
index 0000000..bc912f4
@@ -0,0 +1,68 @@
+Qualcomm Technologies, Inc. CPUFREQ Bindings
+

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +6 -0

+Note that #address-cells, #size-cells, and ranges shall be present to ensure
+the cpufreq can address a freq-domain registers.
+
+A freq-domain sub-node would be defined for the cpus with the following
+properties:
+

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +4 -0

+- reg-names
+	Usage:		required
+	Value type:	<stringlist>
+	Definition:	Address names. Must be "perf_base", "lut_base",

Quoted 18 lines · Taniya Das

Add QCOM cpufreq firmware device bindings for Qualcomm Technology Inc's SoCs. This is required for managing the cpu frequency transitions which are controlled by firmware.

Signed-off-by Taniya Das <[email protected]>

---
  .../bindings/cpufreq/cpufreq-qcom-fw.txt           | 68 ++++++++++++++++++++++
  1 file changed, 68 insertions(+)

create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +2 -0

new file mode 100644
index 0000000..bc912f4
@@ -0,0 +1,68 @@
+Qualcomm Technologies, Inc. CPUFREQ Bindings
+

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt line 12

Quoted 10 lines

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +10 -0

+CPUFREQ FW is a hardware engine used by some Qualcomm Technologies, Inc. (QTI)
+SoCs to manage frequency in hardware. It is capable of controlling frequency
+for multiple clusters.
+
+Properties:
+- compatible
+	Usage:		required
+	Value type:	<string>
+	Definition:	must be "qcom,cpufreq-fw".
+

If the firmware is referred with some other name, better to use that than cpufreq

Quoted 10 lines

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +10 -0

+Note that #address-cells, #size-cells, and ranges shall be present to ensure
+the cpufreq can address a freq-domain registers.
+
+A freq-domain sub-node would be defined for the cpus with the following
+properties:
+
+- compatible:
+	Usage:		required
+	Value type:	<string>
+	Definition:	must be "cpufreq".

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt line 28

Quoted 6 lines

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +6 -0

+
+- reg
+	Usage:		required
+	Value type:	<prop-encoded-array>
+	Definition:	Addresses and sizes for the memory of the perf_base
+			, lut_base and en_base.

Can you explicitly define each one of them ? Either here or in reg-names.

Quoted 9 lines

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +9 -0

+- reg-names
+	Usage:		required
+	Value type:	<stringlist>
+	Definition:	Address names. Must be "perf_base", "lut_base",
+			"en_base".
+			Must be specified in the same order as the
+			corresponding addresses are specified in the reg
+			property.
+

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt line 42

Quoted 5 lines

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +5 -0

+- qcom,cpulist
+	Usage:		required
+	Value type:	<phandles of CPU>
+	Definition:	List of related cpu handles which are under a cluster.
+

As already mentioned by Rob and Viresh, better to align with OPP style to avoid phandle list of CPUs.

Also I see similar bindings for devfreq, can't they be aligned ? E.g. lut_base here while it's ftbl_base in devfreq.

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 <[email protected]>

Signed-off-by Taniya Das <[email protected]>

---
 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

drivers/cpufreq/Kconfig.arm +0 -0

index 96b35b8..571f6b4 100644

drivers/cpufreq/Kconfig.arm +6 -0

+	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.

drivers/cpufreq/Makefile +1 -0

index 8d24ade..a3edbce 100644
@@ -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
 
 
 ##################################################################################

drivers/cpufreq/qcom-cpufreq-fw.c new file +30 -0

new file mode 100644
index 0000000..0e66de0
@@ -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

drivers/cpufreq/qcom-cpufreq-fw.c new file +52 -0

+
+	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);

drivers/cpufreq/qcom-cpufreq-fw.c new file +6 -0

+
+		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;

drivers/cpufreq/qcom-cpufreq-fw.c new file +5 -0

+
+	return 0;
+}
+
+static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)

drivers/cpufreq/qcom-cpufreq-fw.c new file +156 -1

+				cpumask_set_cpu(cpu, m);
+				break;
+			}
+		}
+		dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
+	}
+
+	if (cpumask_empty(m))
+		return -ENOENT;
+
+	return 0;
+}
+
+static int qcom_cpu_resources_init(struct platform_device *pdev,
+				   struct device_node *np)
+{
+	struct cpufreq_qcom *c;
+	struct resource res;
+	struct device *dev = &pdev->dev;
+	void __iomem *en_base;
+	int cpu, index, ret;
+
+	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+	if (!c)
+		return -ENOMEM;
+
+	index = of_property_match_string(np, "reg-names", "en_base");
+	if (index < 0)
+		return index;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	en_base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!en_base) {
+		dev_err(dev, "Unable to map %s en-base\n", np->name);
+		return -ENOMEM;
+	}
+
+	/* FW should be in enabled state to proceed */
+	if (!(readl_relaxed(en_base) & 0x1)) {
+		dev_err(dev, "%s firmware not enabled\n", np->name);
+		return -ENODEV;
+	}
+
+	devm_iounmap(&pdev->dev, en_base);
+
+	index = of_property_match_string(np, "reg-names", "perf_base");
+	if (index < 0)
+		return index;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	c->perf_base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!c->perf_base) {
+		dev_err(dev, "Unable to map %s perf-base\n", np->name);
+		return -ENOMEM;
+	}
+
+	index = of_property_match_string(np, "reg-names", "lut_base");
+	if (index < 0)
+		return index;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	c->lut_base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!c->lut_base) {
+		dev_err(dev, "Unable to map %s lut-base\n", np->name);
+		return -ENOMEM;
+	}
+
+	ret = qcom_get_related_cpus(np, &c->related_cpus);
+	if (ret) {
+		dev_err(dev, "%s failed to get core phandles\n", np->name);
+		return ret;
+	}
+
+	c->max_cores = cpumask_weight(&c->related_cpus);
+
+	ret = qcom_read_lut(pdev, c);
+	if (ret) {
+		dev_err(dev, "%s failed to read LUT\n", np->name);
+		return ret;
+	}
+
+	for_each_cpu(cpu, &c->related_cpus)
+		qcom_freq_domain_map[cpu] = c;
+
+	return 0;
+}
+
+static int qcom_resources_init(struct platform_device *pdev)
+{
+	struct device_node *np;
+	int ret;
+
+	if (!of_get_available_child_count(pdev->dev.of_node))
+		return -ENODEV;
+
+	for_each_available_child_of_node(pdev->dev.of_node, np) {
+		if (of_device_is_compatible(np, "cpufreq")) {
+			ret = qcom_cpu_resources_init(pdev, np);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int qcom_cpufreq_fw_driver_probe(struct platform_device *pdev)
+{
+	int rc;
+
+	/* Get the bases of cpufreq for domains */
+	rc = qcom_resources_init(pdev);
+	if (rc) {
+		dev_err(&pdev->dev, "CPUFreq resource init failed\n");
+		return rc;
+	}
+
+	rc = cpufreq_register_driver(&cpufreq_qcom_fw_driver);
+	if (rc) {
+		dev_err(&pdev->dev, "CPUFreq FW driver failed to register\n");
+		return rc;
+	}
+
+	dev_info(&pdev->dev, "QCOM CPUFreq FW driver inited\n");
+
+	return 0;
+}
+
+static const struct of_device_id match_table[] = {
+	{ .compatible = "qcom,cpufreq-fw" },
+	{}
+};
+
+static struct platform_driver qcom_cpufreq_fw_driver = {
+	.probe = qcom_cpufreq_fw_driver_probe,
+	.driver = {
+		.name = "qcom-cpufreq-fw",
+		.of_match_table = match_table,
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init qcom_cpufreq_fw_init(void)
+{
+	return platform_driver_register(&qcom_cpufreq_fw_driver);
+}
+subsys_initcall(qcom_cpufreq_fw_init);
+
+MODULE_DESCRIPTION("QCOM CPU Frequency FW");
+MODULE_LICENSE("GPL v2");
--

Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member of the Code Aurora Forum, hosted by the Linux Foundation.

Quoted 19 lines · Taniya Das

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 <[email protected]>

Signed-off-by Taniya Das <[email protected]>

---
 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

drivers/cpufreq/Kconfig.arm +0 -0

index 96b35b8..571f6b4 100644

drivers/cpufreq/Kconfig.arm config ARM_PXA2xx_CPUFREQ · line 306

Quoted 6 lines

drivers/cpufreq/Kconfig.arm +3 -0

@@ -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.

Quoted 58 lines

drivers/cpufreq/Kconfig.arm +6 -0

+	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.

drivers/cpufreq/Makefile +0 -0

index 8d24ade..a3edbce 100644
@@ -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

##################################################################################

drivers/cpufreq/qcom-cpufreq-fw.c new file +31 -0

new file mode 100644
index 0000000..0e66de0
@@ -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)

drivers/cpufreq/qcom-cpufreq-fw.c line 40

Quoted 9 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +9 -0

+{
+	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.

drivers/cpufreq/qcom-cpufreq-fw.c line 45

Quoted 4 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +4 -0

+	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.

drivers/cpufreq/qcom-cpufreq-fw.c line 56

Quoted 12 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +11 -0

+	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.

drivers/cpufreq/qcom-cpufreq-fw.c line 59

Quoted 3 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +3 -0

+
+	index = readl_relaxed(c->perf_base);
+	index = min(index, LUT_MAX_ENTRIES - 1);

Will the hardware ever read a value over 39 here ?

Quoted 52 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +52 -0

+
+	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);

drivers/cpufreq/qcom-cpufreq-fw.c line 121

Quoted 10 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +10 -0

+		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 ?

Quoted 10 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +10 -0

+
+		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.

drivers/cpufreq/qcom-cpufreq-fw.c line 137

Quoted 6 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +6 -0

+		 */
+		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 ?

drivers/cpufreq/qcom-cpufreq-fw.c line 139

Quoted 2 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +2 -0

+				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.

drivers/cpufreq/qcom-cpufreq-fw.c line 146

Quoted 7 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +7 -0

+			}
+
+			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 ?

Quoted 98 lines · Viresh Kumar

Quoted 22 lines · Taniya Das

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 <[email protected]>

Signed-off-by Taniya Das <[email protected]>

---
 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

drivers/cpufreq/Kconfig.arm +0 -0

index 96b35b8..571f6b4 100644
@@ -301,3 +301,12 @@ config ARM_PXA2xx_CPUFREQ

       This add the CPUFreq driver support for Intel PXA2xx SOCs.

       If in doubt, say N.

drivers/cpufreq/Kconfig.arm config ARM_PXA2xx_CPUFREQ · line 306

Quoted 3 lines

drivers/cpufreq/Kconfig.arm +3 -0

+
+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.

Quoted 67 lines

+    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.

drivers/cpufreq/Makefile +0 -0

index 8d24ade..a3edbce 100644
@@ -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

##################################################################################

drivers/cpufreq/qcom-cpufreq-fw.c new file +31 -0

new file mode 100644
index 0000000..0e66de0
@@ -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)

drivers/cpufreq/qcom-cpufreq-fw.c new file +8 -0

+{
+    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;

Quoted 8 lines

Quoted 1 line

+    }

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.

Quoted 5 lines

Quoted 4 lines

+    writel_relaxed(index, c->perf_base);
+
+    /* Make sure the write goes through before proceeding */ +    mb();

Quoted 4 lines

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.

Quoted 15 lines

drivers/cpufreq/qcom-cpufreq-fw.c line 56

Quoted 12 lines

+    return 0;

drivers/cpufreq/qcom-cpufreq-fw.c new file +10 -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.

Quoted 4 lines

Quoted 3 lines

+
+    index = readl_relaxed(c->perf_base);
+    index = min(index, LUT_MAX_ENTRIES - 1);

Quoted 1 line

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.

Quoted 66 lines

Quoted 52 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +52 -0

+
+    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);

drivers/cpufreq/qcom-cpufreq-fw.c line 121

Quoted 10 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +10 -0

+        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 ?

Quoted 10 lines

Quoted 10 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +10 -0

+
+        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.

Quoted 8 lines

Quoted 6 lines

+         */
+        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.

Quoted 8 lines

Quoted 2 lines

+                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.

Quoted 8 lines

Quoted 7 lines

+            }
+
+            break;
+        }
+        prev_cc = core_count;
+    }
+    c->table[i].frequency = CPUFREQ_TABLE_END;

Quoted 2 lines

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

Quoted 11 lines · On 5/22/2018 12:36 AM, [email protected] wrote:

Quoted 9 lines · Viresh Kumar

Quoted 2 lines · Taniya Das

+    /* 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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

+    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.

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +19 -0

+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;

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

+        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.

Quoted 19 lines

Quoted 10 lines

Quoted 10 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +10 -0

+
+        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.

Quoted 8 lines

Quoted 6 lines

+         */
+        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 ?

Quoted 10 lines

As of now, yes. If that changes, we'll change this code later.

Quoted 8 lines

Quoted 2 lines

+                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.

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

+            }
+
+            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 ?

Quoted 2 lines

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.

[v2]
* Address comments given in v0 series.

[v1]
* Fixed compilation reported by Amit K.

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.

Taniya Das (2):
dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

 .../bindings/cpufreq/cpufreq-qcom-fw.txt           |  68 +++++
 drivers/cpufreq/Kconfig.arm                        |   9 +
 drivers/cpufreq/Makefile                           |   1 +
 drivers/cpufreq/qcom-cpufreq-fw.c                  | 317 +++++++++++++++++++++
 4 files changed, 395 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
 create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c

--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member of the Code Aurora Forum, hosted by the Linux Foundation.

Quoted 2 lines · Taniya Das

[v2]
* Address comments given in v0 series.

That's not how you do it. You need to explain every change in enough detail here so that the reviewers don't need to go to their previous emails to see what changed.

Quoted 22 lines

[v1]
* Fixed compilation reported by Amit K.

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.

Taniya Das (2):
dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

  .../bindings/cpufreq/cpufreq-qcom-fw.txt           |  68 +++++
  drivers/cpufreq/Kconfig.arm                        |   9 +
  drivers/cpufreq/Makefile                           |   1 +
  drivers/cpufreq/qcom-cpufreq-fw.c                  | 317 +++++++++++++++++++++
  4 files changed, 395 insertions(+)

create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c

--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member of the Code Aurora Forum, hosted by the Linux Foundation.

Hello Viresh,

Sure, will do it next time. Hope updating the fixes in this email is fine. Do let me know in case you need me to send it across again the series with cover letter updated.

Quoted 8 lines · Viresh Kumar

Quoted 2 lines · Taniya Das

[v2]
* Address comments given in v0 series.

That's not how you do it. You need to explain every change in enough detail here so that the reviewers don't need to go to their previous emails to see what changed.

Fixes in [v2]
* Fixed the alignment issues in "qcom_cpufreq_fw_target_index" for
dev_err and also for "qcom_cpu_resources_init".
* Removed ret = 0 from qcom_get_related_cpus and added to check for
cpu_mask_empty to return -ENOENT.
* Fixes qcom_cpu_resources_init function
* Remove initialization of 'index'
* Check for valid 'c'
* Removed initialization of 'prev_cc' from 'qcom_read_lut'.
* Remove initialization of 'ret' from function qcom_resources_init and add
return -ENODEV based on 'of_get_available_child_count'.
* Removed initialization of 'rc' from qcom_cpufreq_fw_driver_probe
* Removed module_exit as this driver would not be used as module, also
updated the Kconfig to bool from tristate.
* Updated the subsystem in device tree bindings.