[PATCH v7 0/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[PATCH v7 0/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Taniya Das]

 [v7]
   * Updated the logic to check for related CPUs.

 [v6]
   * Renamed match table 'qcom_cpufreq_hw_match'.
   * Renamed 'qcom_read_lut' to 'qcom_cpufreq_hw_read_lut'.
   * Updated the logic to check for related CPUs at the beginning of the
     'qcom_cpu_resources_init'.
   * Use devm_ioremap_resource instead of devm_ioremap.
   * Update the use of of_node_put to handle error conditions.
   * Use policy->cached_resolved_idx in fast switch callback.
   * Keep precalculated offsets 'reg_bases'.
   * XO clock is taken from Device tree.
   * Update documentation binding for clocks/clock-names.
   * Minor comments in Kconfig.arm.
   * Comments to move dev_info to dev_dbg.

 [v5]
   * Remove mapping different register regions of perf/lut/enable,
     instead map the entire HW region.
   * Add reg_offset/cpufreq_qcom_std_offsets to be supplied as device data.
   * Check of src == 0 during lut read.
   * Add of_node_put(cpu_np) in qcom_get_related_cpus
   * Update the qcom_cpu_resources_init for register offset data,
     and cleanup the related cpus to keep a single copy of CPUfreq.
   * Replace FW with HW, update Kconfig, rename filename qcom-cpufreq-hw.c
   * Update the documentation binding to reflect the changes of mapping the
   * entire HW region.

 [v4]
   * Fixed console messages as per comments.
   * Return error from qcom_resources_init()
     in the cases where failed to get frequency domain.
   * Rename cpu_dev to cpu_np in qcom_resources_init,
     qcom_get_related_cpus(). Also use temp variable freq_np in
     qcom_get_related_cpus().
   * Update qcom_cpufreq_fw_get() to use the policy data to incoporate
     the hotplug use case.
   * Update code to use of fast_switching.
   * Check for !c->max_cores instead of cpumask_empty in
     qcom_get_related_cpus().
   * Update the logic of assigning 'c' to qcom_freq_domain_map[cpu].

 [v3]
   * Remove index check from 'qcom_cpufreq_fw_target_index'.
   * Update the Documentation binding to add the platform specific properties in
     the CPU nodes, node name "qcom,freq-domain".
   * Update return value to '0' from -ENODEV from 'qcom_cpufreq_fw_get'.
   * Update the logic for boost frequency to use local variables instead of
     cpufreq driver data in 'qcom_read_lut'.
   * Update the logic in 'qcom_get_related_cpus' to find the related cpus.
   * Update the reg-names to remove "_base" and also update the binding with the
     description of these registers.
   * Update the logic in 'qcom_resources_init' to address the new device tree
     notation of handling the frequency domain phandles.

 [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'.

Taniya Das (2):
  dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings
  cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

 .../bindings/cpufreq/cpufreq-qcom-hw.txt           | 172 ++++++++++
 drivers/cpufreq/Kconfig.arm                        |  11 +
 drivers/cpufreq/Makefile                           |   1 +
 drivers/cpufreq/qcom-cpufreq-hw.c                  | 348 +++++++++++++++++++++
 4 files changed, 532 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
 create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c

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

[PATCH v7 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings

[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 the hardware engine.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 .../bindings/cpufreq/cpufreq-qcom-hw.txt           | 172 +++++++++++++++++++++
 1 file changed, 172 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
new file mode 100644
index 0000000..22d4355
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
@@ -0,0 +1,172 @@
+Qualcomm Technologies, Inc. CPUFREQ Bindings
+
+CPUFREQ HW 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-hw".
+
+- clocks
+	Usage:		required
+	Value type:	<phandle> From common clock binding.
+	Definition:	clock handle for XO clock.
+
+- clock-names
+	Usage:		required
+	Value type:	<string> From common clock binding.
+	Definition:	must be "xo".
+
+* Property qcom,freq-domain
+Devices supporting freq-domain must set their "qcom,freq-domain" property with
+phandle to a freq_domain_table in their DT node.
+
+* Frequency Domain Table Node
+
+This describes the frequency domain belonging to a device.
+This node can have following properties:
+
+- reg
+	Usage:		required
+	Value type:	<prop-encoded-array>
+	Definition:	Addresses and sizes for the memory of the HW bases.
+
+Example:
+
+Example 1: Dual-cluster, Quad-core per cluster. CPUs within a cluster switch
+DCVS state together.
+
+/ {
+	cpus {
+		#address-cells = <2>;
+		#size-cells = <0>;
+
+		CPU0: cpu@0 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x0>;
+			enable-method = "psci";
+			next-level-cache = <&L2_0>;
+			qcom,freq-domain = <&freq_domain_table0>;
+			L2_0: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+				L3_0: l3-cache {
+				      compatible = "cache";
+				};
+			};
+		};
+
+		CPU1: cpu@100 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x100>;
+			enable-method = "psci";
+			next-level-cache = <&L2_100>;
+			qcom,freq-domain = <&freq_domain_table0>;
+			L2_100: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU2: cpu@200 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x200>;
+			enable-method = "psci";
+			next-level-cache = <&L2_200>;
+			qcom,freq-domain = <&freq_domain_table0>;
+			L2_200: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU3: cpu@300 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x300>;
+			enable-method = "psci";
+			next-level-cache = <&L2_300>;
+			qcom,freq-domain = <&freq_domain_table0>;
+			L2_300: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU4: cpu@400 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x400>;
+			enable-method = "psci";
+			next-level-cache = <&L2_400>;
+			qcom,freq-domain = <&freq_domain_table1>;
+			L2_400: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU5: cpu@500 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x500>;
+			enable-method = "psci";
+			next-level-cache = <&L2_500>;
+			qcom,freq-domain = <&freq_domain_table1>;
+			L2_500: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU6: cpu@600 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x600>;
+			enable-method = "psci";
+			next-level-cache = <&L2_600>;
+			qcom,freq-domain = <&freq_domain_table1>;
+			L2_600: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU7: cpu@700 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x700>;
+			enable-method = "psci";
+			next-level-cache = <&L2_700>;
+			qcom,freq-domain = <&freq_domain_table1>;
+			L2_700: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+	};
+
+	qcom,cpufreq-hw {
+		compatible = "qcom,cpufreq-hw";
+
+		clocks = <&rpmhcc RPMH_CXO_CLK>;
+		clock-names = "xo";
+
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+		freq_domain_table0: freq_table0 {
+			reg = <0 0x17d43000 0 0x1400>;
+		};
+
+		freq_domain_table1: freq_table1 {
+			reg = <0 0x17d45800 0 0x1400>;
+		};
+	};
+};
-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

[PATCH v7 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Taniya Das]

The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
for changing the frequency of CPUs. The driver implements the cpufreq
driver interface for this hardware engine.

Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/cpufreq/Kconfig.arm       |  11 ++
 drivers/cpufreq/Makefile          |   1 +
 drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 360 insertions(+)
 create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 0cd8eb7..93a9d72 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -298,3 +298,14 @@ config ARM_PXA2xx_CPUFREQ
 	  This add the CPUFreq driver support for Intel PXA2xx SOCs.

 	  If in doubt, say N.
+
+config ARM_QCOM_CPUFREQ_HW
+	bool "QCOM CPUFreq HW driver"
+	depends on ARCH_QCOM
+	help
+	 Support for the CPUFreq HW driver.
+	 Some QCOM chipsets have a HW engine to offload the steps
+	 necessary for changing the frequency of the CPUs. Firmware loaded
+	 in this engine exposes a programming interface to the OS.
+	 The driver implements the cpufreq interface for this HW engine.
+	 Say Y if you want to support CPUFreq HW.
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index c1ffeab..ca48a1d 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_HW)	+= qcom-cpufreq-hw.o


 ##################################################################################
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
new file mode 100644
index 0000000..ea8f7d1
--- /dev/null
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -0,0 +1,348 @@
+// 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 LUT_MAX_ENTRIES			40U
+#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
+#define LUT_ROW_SIZE			32
+
+enum {
+	REG_ENABLE,
+	REG_LUT_TABLE,
+	REG_PERF_STATE,
+
+	REG_ARRAY_SIZE,
+};
+
+struct cpufreq_qcom {
+	struct cpufreq_frequency_table *table;
+	struct device *dev;
+	void __iomem *reg_bases[REG_ARRAY_SIZE];
+	cpumask_t related_cpus;
+	unsigned int max_cores;
+	unsigned long xo_rate;
+};
+
+static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
+	[REG_ENABLE]		= 0x0,
+	[REG_LUT_TABLE]		= 0x110,
+	[REG_PERF_STATE]	= 0x920,
+};
+
+static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
+
+static int
+qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
+			     unsigned int index)
+{
+	struct cpufreq_qcom *c = policy->driver_data;
+
+	writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
+
+	return 0;
+}
+
+static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
+{
+	struct cpufreq_qcom *c;
+	struct cpufreq_policy *policy;
+	unsigned int index;
+
+	policy = cpufreq_cpu_get_raw(cpu);
+	if (!policy)
+		return 0;
+
+	c = policy->driver_data;
+
+	index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
+	index = min(index, LUT_MAX_ENTRIES - 1);
+
+	return policy->freq_table[index].frequency;
+}
+
+static unsigned int
+qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
+			    unsigned int target_freq)
+{
+	struct cpufreq_qcom *c = policy->driver_data;
+	int index;
+
+	index = policy->cached_resolved_idx;
+	if (index < 0)
+		return 0;
+
+	writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
+
+	return policy->freq_table[index].frequency;
+}
+
+static int qcom_cpufreq_hw_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->fast_switch_possible = true;
+	policy->freq_table = c->table;
+	policy->driver_data = c;
+
+	return 0;
+}
+
+static struct freq_attr *qcom_cpufreq_hw_attr[] = {
+	&cpufreq_freq_attr_scaling_available_freqs,
+	&cpufreq_freq_attr_scaling_boost_freqs,
+	NULL
+};
+
+static struct cpufreq_driver cpufreq_qcom_hw_driver = {
+	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
+			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
+	.verify		= cpufreq_generic_frequency_table_verify,
+	.target_index	= qcom_cpufreq_hw_target_index,
+	.get		= qcom_cpufreq_hw_get,
+	.init		= qcom_cpufreq_hw_cpu_init,
+	.fast_switch    = qcom_cpufreq_hw_fast_switch,
+	.name		= "qcom-cpufreq-hw",
+	.attr		= qcom_cpufreq_hw_attr,
+	.boost_enabled	= true,
+};
+
+static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
+				    struct cpufreq_qcom *c)
+{
+	struct device *dev = &pdev->dev;
+	void __iomem *base;
+	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
+
+	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
+				sizeof(*c->table), GFP_KERNEL);
+	if (!c->table)
+		return -ENOMEM;
+
+	base = c->reg_bases[REG_LUT_TABLE];
+
+	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
+		data = readl_relaxed(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 = c->xo_rate * lval / 1000;
+		else
+			c->table[i].frequency = INIT_RATE / 1000;
+
+		cur_freq = c->table[i].frequency;
+
+		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
+			i, c->table[i].frequency, core_count);
+
+		if (core_count != c->max_cores)
+			cur_freq = CPUFREQ_ENTRY_INVALID;
+
+		/*
+		 * Two of the same frequencies with the same core counts means
+		 * end of table.
+		 */
+		if (i > 0 && c->table[i - 1].frequency ==
+		   c->table[i].frequency && prev_cc == core_count) {
+			struct cpufreq_frequency_table *prev = &c->table[i - 1];
+
+			if (prev_freq == CPUFREQ_ENTRY_INVALID)
+				prev->flags = CPUFREQ_BOOST_FREQ;
+			break;
+		}
+		prev_cc = core_count;
+		prev_freq = cur_freq;
+	}
+
+	c->table[i].frequency = CPUFREQ_TABLE_END;
+
+	return 0;
+}
+
+static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
+{
+	struct device_node *cpu_np, *freq_np;
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		cpu_np = of_cpu_device_node_get(cpu);
+		if (!cpu_np)
+			continue;
+		freq_np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
+		of_node_put(cpu_np);
+		if (!freq_np)
+			continue;
+
+		if (freq_np == np)
+			cpumask_set_cpu(cpu, m);
+	}
+
+	return 0;
+}
+
+static int qcom_cpu_resources_init(struct platform_device *pdev,
+				   struct device_node *np, unsigned int cpu,
+				   unsigned long xo_rate)
+{
+	struct cpufreq_qcom *c;
+	struct resource res;
+	struct device *dev = &pdev->dev;
+	const u16 *offsets;
+	int ret, i, cpu_first, cpu_r;
+	void __iomem *base;
+
+	if (qcom_freq_domain_map[cpu])
+		return 0;
+
+	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+	if (!c)
+		return -ENOMEM;
+
+	offsets = of_device_get_match_data(&pdev->dev);
+	if (!offsets)
+		return -EINVAL;
+
+	if (of_address_to_resource(np, 0, &res))
+		return -ENOMEM;
+
+	base = devm_ioremap_resource(dev, &res);
+	if (!base)
+		return -ENOMEM;
+
+	for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
+		c->reg_bases[i] = base + offsets[i];
+
+	/* HW should be in enabled state to proceed */
+	if (!(readl_relaxed(c->reg_bases[REG_ENABLE]) & 0x1)) {
+		dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
+		return -ENODEV;
+	}
+
+	ret = qcom_get_related_cpus(np, &c->related_cpus);
+	if (ret) {
+		dev_err(dev, "%s failed to get related CPUs\n", np->name);
+		return ret;
+	}
+
+	c->max_cores = cpumask_weight(&c->related_cpus);
+	if (!c->max_cores)
+		return -ENOENT;
+
+	c->xo_rate = xo_rate;
+
+	ret = qcom_cpufreq_hw_read_lut(pdev, c);
+	if (ret) {
+		dev_err(dev, "%s failed to read LUT\n", np->name);
+		return ret;
+	}
+
+	qcom_freq_domain_map[cpu] = c;
+
+	/* Related CPUs */
+	cpu_first = cpumask_first(&c->related_cpus);
+
+	for_each_cpu(cpu_r, &c->related_cpus) {
+		if (cpu_r != cpu_first)
+			qcom_freq_domain_map[cpu_r] =
+				qcom_freq_domain_map[cpu_first];
+	}
+
+	return 0;
+}
+
+static int qcom_resources_init(struct platform_device *pdev)
+{
+	struct device_node *np, *cpu_np;
+	struct clk *clk;
+	unsigned int cpu;
+	int ret;
+
+	clk = devm_clk_get(&pdev->dev, "xo");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	for_each_possible_cpu(cpu) {
+		cpu_np = of_cpu_device_node_get(cpu);
+		if (!cpu_np) {
+			dev_dbg(&pdev->dev, "Failed to get cpu %d device\n",
+				cpu);
+			continue;
+		}
+
+		np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
+		of_node_put(cpu_np);
+		if (!np) {
+			dev_err(&pdev->dev, "Failed to get freq-domain device\n");
+			return -EINVAL;
+		}
+
+		ret = qcom_cpu_resources_init(pdev, np, cpu, clk_get_rate(clk));
+		if (ret)
+			return ret;
+	}
+
+	devm_clk_put(&pdev->dev, clk);
+
+	return 0;
+}
+
+static int qcom_cpufreq_hw_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_hw_driver);
+	if (rc) {
+		dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
+		return rc;
+	}
+
+	dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
+
+	return 0;
+}
+
+static const struct of_device_id qcom_cpufreq_hw_match[] = {
+	{ .compatible = "qcom,cpufreq-hw", .data = &cpufreq_qcom_std_offsets },
+	{}
+};
+
+static struct platform_driver qcom_cpufreq_hw_driver = {
+	.probe = qcom_cpufreq_hw_driver_probe,
+	.driver = {
+		.name = "qcom-cpufreq-hw",
+		.of_match_table = qcom_cpufreq_hw_match,
+	},
+};
+
+static int __init qcom_cpufreq_hw_init(void)
+{
+	return platform_driver_register(&qcom_cpufreq_hw_driver);
+}
+subsys_initcall(qcom_cpufreq_hw_init);
+
+MODULE_DESCRIPTION("QCOM firmware-based CPU Frequency driver");
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Re: [PATCH v7 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Evan Green]

Hi Taniya,

On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <tdas@codeaurora.org> wrote:
>
> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this hardware engine.
>
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  drivers/cpufreq/Kconfig.arm       |  11 ++
>  drivers/cpufreq/Makefile          |   1 +
>  drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 360 insertions(+)
>  create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>
...
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> new file mode 100644
> index 0000000..ea8f7d1
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
...
> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
> +                                   struct cpufreq_qcom *c)
> +{
> +       struct device *dev = &pdev->dev;
> +       void __iomem *base;
> +       u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
> +
> +       c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> +                               sizeof(*c->table), GFP_KERNEL);
> +       if (!c->table)
> +               return -ENOMEM;
> +
> +       base = c->reg_bases[REG_LUT_TABLE];
> +
> +       for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> +               data = readl_relaxed(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 = c->xo_rate * lval / 1000;
> +               else
> +                       c->table[i].frequency = INIT_RATE / 1000;

I don't know much about how this hardware works, but based on the
mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
and 3 all mean xo_rate?

Also, is INIT_RATE really constant? It sounds like gpll0 (or
gpll0_out_even?). You're already getting the xo clock, why not get
gpll0's real rate as well?

Re: [PATCH v7 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[skannan]

On 2018-08-03 12:40, Evan Green wrote:
> Hi Taniya,
> 
> On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <tdas@codeaurora.org> wrote:
>> 
>> The CPUfreq HW present in some QCOM chipsets offloads the steps 
>> necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this hardware engine.
>> 
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> ---
>>  drivers/cpufreq/Kconfig.arm       |  11 ++
>>  drivers/cpufreq/Makefile          |   1 +
>>  drivers/cpufreq/qcom-cpufreq-hw.c | 348 
>> ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 360 insertions(+)
>>  create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>> 
> ...
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c 
>> b/drivers/cpufreq/qcom-cpufreq-hw.c
>> new file mode 100644
>> index 0000000..ea8f7d1
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> ...
>> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
>> +                                   struct cpufreq_qcom *c)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       void __iomem *base;
>> +       u32 data, src, lval, i, core_count, prev_cc, prev_freq, 
>> cur_freq;
>> +
>> +       c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
>> +                               sizeof(*c->table), GFP_KERNEL);
>> +       if (!c->table)
>> +               return -ENOMEM;
>> +
>> +       base = c->reg_bases[REG_LUT_TABLE];
>> +
>> +       for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> +               data = readl_relaxed(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 = c->xo_rate * lval / 
>> 1000;
>> +               else
>> +                       c->table[i].frequency = INIT_RATE / 1000;
> 
> I don't know much about how this hardware works, but based on the
> mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
> and 3 all mean xo_rate?
> 
> Also, is INIT_RATE really constant? It sounds like gpll0 (or
> gpll0_out_even?). You're already getting the xo clock, why not get
> gpll0's real rate as well?

Actually I was about to comment and say NOT to get clocks just to get 
their rate. The XO_RATE is just a multiplication factor. This HW/FW can 
change in the future and make the multiplication factor to 1KHz. We also 
can't really control any of the clocks going to this block from Linux 
(it's all locked down). Adding clk_get significantly delays when this 
driver can be probed and increases boot up time. The INIT_RATE will 
always be 300 MHz independent of what source it's coming from (as in, if 
GPLL0 can't give 300, the HW design will be so that we'll find a 
different source).

So, I'd like to remove any clock bindings for this driver.

Thanks,
Saravana

Re: [PATCH v7 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Stephen Boyd]

Quoting skannan@codeaurora.org (2018-08-03 12:52:48)
> On 2018-08-03 12:40, Evan Green wrote:
> > Hi Taniya,
> > 
> > On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <tdas@codeaurora.org> wrote:
> >> 
> >> +               if (src)
> >> +                       c->table[i].frequency = c->xo_rate * lval / 
> >> 1000;
> >> +               else
> >> +                       c->table[i].frequency = INIT_RATE / 1000;
> > 
> > I don't know much about how this hardware works, but based on the
> > mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
> > and 3 all mean xo_rate?
> > 
> > Also, is INIT_RATE really constant? It sounds like gpll0 (or
> > gpll0_out_even?). You're already getting the xo clock, why not get
> > gpll0's real rate as well?
> 
> Actually I was about to comment and say NOT to get clocks just to get 
> their rate. The XO_RATE is just a multiplication factor. This HW/FW can 
> change in the future and make the multiplication factor to 1KHz.

So future changes to this hardware are going to make this register
return the final frequency that we should use? Sounds great! But that
isn't how it's working right now. We need to have XO in the binding here
so the driver can work with different XO frequencies in case that ever
happens. Same story for GPLL0. Hardcoding this stuff in the driver just
means we'll have to swizzle things in the driver when it changes.

> We also 
> can't really control any of the clocks going to this block from Linux 
> (it's all locked down).

This shouldn't matter. The clocks going to this hardware block are
described by the firmware to the OS by means of the DT node. If the
firmware or the hardware decides to change the input clks then the
binding can have different clk nodes used.

> Adding clk_get significantly delays when this 
> driver can be probed and increases boot up time.

Huh? Please fix your bootloader to increase the CPU frequency before
booting the kernel. I really doubt probe defer is going to happen if we
send GPLL0 here (XO is a DT clk so it's definitely registered before
this driver), and trying to work around probe defer because the CPU
won't go fast before that is papering over a larger problem with both
probe defer and bootloaders not supplying enough CPU speed for you.

> The INIT_RATE will 
> always be 300 MHz independent of what source it's coming from (as in, if 
> GPLL0 can't give 300, the HW design will be so that we'll find a 
> different source).
> 
> So, I'd like to remove any clock bindings for this driver.

No. Bindings describe how the hardware is connected. Please don't remove
clocks from the binding just because probe defer is a concern.

Re: [PATCH v7 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings

[Stephen Boyd]

Quoting Taniya Das (2018-07-24 03:42:49)
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> new file mode 100644
> index 0000000..22d4355
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> @@ -0,0 +1,172 @@
[...]
> +
> +               CPU7: cpu@700 {
> +                       device_type = "cpu";
> +                       compatible = "qcom,kryo385";
> +                       reg = <0x0 0x700>;
> +                       enable-method = "psci";
> +                       next-level-cache = <&L2_700>;
> +                       qcom,freq-domain = <&freq_domain_table1>;
> +                       L2_700: l2-cache {
> +                               compatible = "cache";
> +                               next-level-cache = <&L3_0>;
> +                       };
> +               };
> +       };
> +
> +       qcom,cpufreq-hw {
> +               compatible = "qcom,cpufreq-hw";
> +
> +               clocks = <&rpmhcc RPMH_CXO_CLK>;
> +               clock-names = "xo";
> +
> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +               ranges;
> +               freq_domain_table0: freq_table0 {
> +                       reg = <0 0x17d43000 0 0x1400>;
> +               };
> +
> +               freq_domain_table1: freq_table1 {
> +                       reg = <0 0x17d45800 0 0x1400>;
> +               };

Sorry, this is just not proper DT design. The whole node should have a
reg property, and it should contain two (or three if we're handling the
L3 clk domain?) different offsets for the different power clusters. The
problem seems to still be that we don't have a way to map the CPUs to
the clk domains they're in provided by this hardware block. Making
subnodes is not the solution.

Re: [PATCH v7 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[skannan]

On 2018-08-03 15:24, Stephen Boyd wrote:
> Quoting skannan@codeaurora.org (2018-08-03 12:52:48)
>> On 2018-08-03 12:40, Evan Green wrote:
>> > Hi Taniya,
>> >
>> > On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <tdas@codeaurora.org> wrote:
>> >>
>> >> +               if (src)
>> >> +                       c->table[i].frequency = c->xo_rate * lval /
>> >> 1000;
>> >> +               else
>> >> +                       c->table[i].frequency = INIT_RATE / 1000;
>> >
>> > I don't know much about how this hardware works, but based on the
>> > mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
>> > and 3 all mean xo_rate?
>> >
>> > Also, is INIT_RATE really constant? It sounds like gpll0 (or
>> > gpll0_out_even?). You're already getting the xo clock, why not get
>> > gpll0's real rate as well?
>> 
>> Actually I was about to comment and say NOT to get clocks just to get
>> their rate. The XO_RATE is just a multiplication factor. This HW/FW 
>> can
>> change in the future and make the multiplication factor to 1KHz.
> 
> So future changes to this hardware are going to make this register
> return the final frequency that we should use? Sounds great! But that
> isn't how it's working right now. We need to have XO in the binding 
> here
> so the driver can work with different XO frequencies in case that ever
> happens. Same story for GPLL0. Hardcoding this stuff in the driver just
> means we'll have to swizzle things in the driver when it changes.

XO being a different frequency in a Qualcomm chip is way way less likely 
than anything else we are discussing here. In the 10+years I've been 
there this has never changed.

So, how about making this binding optional? If it's not present we can 
make assumptions for XO rate and init rate. That'll handle your 
hypothetical use case while also being optimized.

>> We also
>> can't really control any of the clocks going to this block from Linux
>> (it's all locked down).
> 
> This shouldn't matter. The clocks going to this hardware block are
> described by the firmware to the OS by means of the DT node. If the
> firmware or the hardware decides to change the input clks then the
> binding can have different clk nodes used.

There are tons of clocks that are input to blocks in an SoC that are 
never controlled by Linux. Or are expose in DT because they are set up 
ahead of time and can't change. So, why make an exception here?

>> Adding clk_get significantly delays when this
>> driver can be probed and increases boot up time.
> 
> Huh? Please fix your bootloader to increase the CPU frequency before
> booting the kernel. I really doubt probe defer is going to happen if we
> send GPLL0 here (XO is a DT clk so it's definitely registered before
> this driver), and trying to work around probe defer because the CPU
> won't go fast before that is papering over a larger problem with both
> probe defer and bootloaders not supplying enough CPU speed for you.

There are multiple reason the bootloader can't run the CPU faster. For 
one, it doesn't have thermal handling, etc. And adding all those 
features into the bootloader doesn't make sense when Linux already has 
support for it.

>> The INIT_RATE will
>> always be 300 MHz independent of what source it's coming from (as in, 
>> if
>> GPLL0 can't give 300, the HW design will be so that we'll find a
>> different source).
>> 
>> So, I'd like to remove any clock bindings for this driver.
> 
> No. Bindings describe how the hardware is connected. Please don't 
> remove
> clocks from the binding just because probe defer is a concern.

Binding describes hardware controllable by the OS. That's the reality. 
Let's not add mandatory clock bindings for clocks that the OS can't do 
anything about.

-Saravana

Re: [PATCH v7 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings

[skannan]

On 2018-08-03 16:46, Stephen Boyd wrote:
> Quoting Taniya Das (2018-07-24 03:42:49)
>> diff --git 
>> a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt 
>> b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
>> new file mode 100644
>> index 0000000..22d4355
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
>> @@ -0,0 +1,172 @@
> [...]
>> +
>> +               CPU7: cpu@700 {
>> +                       device_type = "cpu";
>> +                       compatible = "qcom,kryo385";
>> +                       reg = <0x0 0x700>;
>> +                       enable-method = "psci";
>> +                       next-level-cache = <&L2_700>;
>> +                       qcom,freq-domain = <&freq_domain_table1>;
>> +                       L2_700: l2-cache {
>> +                               compatible = "cache";
>> +                               next-level-cache = <&L3_0>;
>> +                       };
>> +               };
>> +       };
>> +
>> +       qcom,cpufreq-hw {
>> +               compatible = "qcom,cpufreq-hw";
>> +
>> +               clocks = <&rpmhcc RPMH_CXO_CLK>;
>> +               clock-names = "xo";
>> +
>> +               #address-cells = <2>;
>> +               #size-cells = <2>;
>> +               ranges;
>> +               freq_domain_table0: freq_table0 {
>> +                       reg = <0 0x17d43000 0 0x1400>;
>> +               };
>> +
>> +               freq_domain_table1: freq_table1 {
>> +                       reg = <0 0x17d45800 0 0x1400>;
>> +               };
> 
> Sorry, this is just not proper DT design. The whole node should have a
> reg property, and it should contain two (or three if we're handling the
> L3 clk domain?) different offsets for the different power clusters. The
> problem seems to still be that we don't have a way to map the CPUs to
> the clk domains they're in provided by this hardware block. Making
> subnodes is not the solution.

The problem is mapping clock domains to logical CPUs that CPUfreq uses. 
The physical CPU to logical CPU mapping can be changed by the kernel 
(even through DT if I'm not mistaken). So we need to have a way to tell 
in DT which physical CPUs are connected to which CPU freq clock domain.

As for subnodes or not, we don't have any strong opinion, but couple of 
other points to consider. Two or more CPUfreq policies might have a 
common frequency table (read from HW), but separate control of 
frequency. So, you also need a way to group frequency table with CPU 
freq policies. If you have a better design, we are open to that 
suggestion.

Thanks,
Saravana

Re: [PATCH v7 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings

[Sudeep Holla]

On Mon, Aug 06, 2018 at 01:54:24PM -0700, skannan@codeaurora.org wrote:
> On 2018-08-03 16:46, Stephen Boyd wrote:
> >Quoting Taniya Das (2018-07-24 03:42:49)
> >>diff --git
> >>a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> >>b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> >>new file mode 100644
> >>index 0000000..22d4355
> >>--- /dev/null
> >>+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> >>@@ -0,0 +1,172 @@
> >[...]
> >>+
> >>+               CPU7: cpu@700 {
> >>+                       device_type = "cpu";
> >>+                       compatible = "qcom,kryo385";
> >>+                       reg = <0x0 0x700>;
> >>+                       enable-method = "psci";
> >>+                       next-level-cache = <&L2_700>;
> >>+                       qcom,freq-domain = <&freq_domain_table1>;
> >>+                       L2_700: l2-cache {
> >>+                               compatible = "cache";
> >>+                               next-level-cache = <&L3_0>;
> >>+                       };
> >>+               };
> >>+       };
> >>+
> >>+       qcom,cpufreq-hw {
> >>+               compatible = "qcom,cpufreq-hw";
> >>+
> >>+               clocks = <&rpmhcc RPMH_CXO_CLK>;
> >>+               clock-names = "xo";
> >>+
> >>+               #address-cells = <2>;
> >>+               #size-cells = <2>;
> >>+               ranges;
> >>+               freq_domain_table0: freq_table0 {
> >>+                       reg = <0 0x17d43000 0 0x1400>;
> >>+               };
> >>+
> >>+               freq_domain_table1: freq_table1 {
> >>+                       reg = <0 0x17d45800 0 0x1400>;
> >>+               };
> >
> >Sorry, this is just not proper DT design. The whole node should have a
> >reg property, and it should contain two (or three if we're handling the
> >L3 clk domain?) different offsets for the different power clusters. The
> >problem seems to still be that we don't have a way to map the CPUs to
> >the clk domains they're in provided by this hardware block. Making
> >subnodes is not the solution.
> 
> The problem is mapping clock domains to logical CPUs that CPUfreq uses. The
> physical CPU to logical CPU mapping can be changed by the kernel (even
> through DT if I'm not mistaken). So we need to have a way to tell in DT
> which physical CPUs are connected to which CPU freq clock domain.
> 

How about passing CPU freq clock domain id as along with phandle in
qcom,freq-domain ?

--
Regards,
Sudeep

Re: [PATCH v7 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings

[skannan]

On 2018-08-07 04:12, Sudeep Holla wrote:
> On Mon, Aug 06, 2018 at 01:54:24PM -0700, skannan@codeaurora.org wrote:
>> On 2018-08-03 16:46, Stephen Boyd wrote:
>> >Quoting Taniya Das (2018-07-24 03:42:49)
>> >>diff --git
>> >>a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
>> >>b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
>> >>new file mode 100644
>> >>index 0000000..22d4355
>> >>--- /dev/null
>> >>+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
>> >>@@ -0,0 +1,172 @@
>> >[...]
>> >>+
>> >>+               CPU7: cpu@700 {
>> >>+                       device_type = "cpu";
>> >>+                       compatible = "qcom,kryo385";
>> >>+                       reg = <0x0 0x700>;
>> >>+                       enable-method = "psci";
>> >>+                       next-level-cache = <&L2_700>;
>> >>+                       qcom,freq-domain = <&freq_domain_table1>;
>> >>+                       L2_700: l2-cache {
>> >>+                               compatible = "cache";
>> >>+                               next-level-cache = <&L3_0>;
>> >>+                       };
>> >>+               };
>> >>+       };
>> >>+
>> >>+       qcom,cpufreq-hw {
>> >>+               compatible = "qcom,cpufreq-hw";
>> >>+
>> >>+               clocks = <&rpmhcc RPMH_CXO_CLK>;
>> >>+               clock-names = "xo";
>> >>+
>> >>+               #address-cells = <2>;
>> >>+               #size-cells = <2>;
>> >>+               ranges;
>> >>+               freq_domain_table0: freq_table0 {
>> >>+                       reg = <0 0x17d43000 0 0x1400>;
>> >>+               };
>> >>+
>> >>+               freq_domain_table1: freq_table1 {
>> >>+                       reg = <0 0x17d45800 0 0x1400>;
>> >>+               };
>> >
>> >Sorry, this is just not proper DT design. The whole node should have a
>> >reg property, and it should contain two (or three if we're handling the
>> >L3 clk domain?) different offsets for the different power clusters. The
>> >problem seems to still be that we don't have a way to map the CPUs to
>> >the clk domains they're in provided by this hardware block. Making
>> >subnodes is not the solution.
>> 
>> The problem is mapping clock domains to logical CPUs that CPUfreq 
>> uses. The
>> physical CPU to logical CPU mapping can be changed by the kernel (even
>> through DT if I'm not mistaken). So we need to have a way to tell in 
>> DT
>> which physical CPUs are connected to which CPU freq clock domain.
>> 
> 
> How about passing CPU freq clock domain id as along with phandle in
> qcom,freq-domain ?

Now sure what you mean here. There's no such this as CPUfreq clock 
domain id. It has policies that are made up of logical CPU numbers. 
Logical CPU is not something that you can fix in DT.

-Saravana

Re: [PATCH v7 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings

[Taniya Das]

On 8/8/2018 12:54 AM, skannan@codeaurora.org wrote:
> On 2018-08-07 04:12, Sudeep Holla wrote:
>> On Mon, Aug 06, 2018 at 01:54:24PM -0700, skannan@codeaurora.org wrote:
>>> On 2018-08-03 16:46, Stephen Boyd wrote:
>>> >Quoting Taniya Das (2018-07-24 03:42:49)
>>> >>diff --git
>>> >>a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
>>> >>b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
>>> >>new file mode 100644
>>> >>index 0000000..22d4355
>>> >>--- /dev/null
>>> >>+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
>>> >>@@ -0,0 +1,172 @@
>>> >[...]
>>> >>+
>>> >>+               CPU7: cpu@700 {
>>> >>+                       device_type = "cpu";
>>> >>+                       compatible = "qcom,kryo385";
>>> >>+                       reg = <0x0 0x700>;
>>> >>+                       enable-method = "psci";
>>> >>+                       next-level-cache = <&L2_700>;
>>> >>+                       qcom,freq-domain = <&freq_domain_table1>;
>>> >>+                       L2_700: l2-cache {
>>> >>+                               compatible = "cache";
>>> >>+                               next-level-cache = <&L3_0>;
>>> >>+                       };
>>> >>+               };
>>> >>+       };
>>> >>+
>>> >>+       qcom,cpufreq-hw {
>>> >>+               compatible = "qcom,cpufreq-hw";
>>> >>+
>>> >>+               clocks = <&rpmhcc RPMH_CXO_CLK>;
>>> >>+               clock-names = "xo";
>>> >>+
>>> >>+               #address-cells = <2>;
>>> >>+               #size-cells = <2>;
>>> >>+               ranges;
>>> >>+               freq_domain_table0: freq_table0 {
>>> >>+                       reg = <0 0x17d43000 0 0x1400>;
>>> >>+               };
>>> >>+
>>> >>+               freq_domain_table1: freq_table1 {
>>> >>+                       reg = <0 0x17d45800 0 0x1400>;
>>> >>+               };
>>> >
>>> >Sorry, this is just not proper DT design. The whole node should have a
>>> >reg property, and it should contain two (or three if we're handling the
>>> >L3 clk domain?) different offsets for the different power clusters. The
>>> >problem seems to still be that we don't have a way to map the CPUs to
>>> >the clk domains they're in provided by this hardware block. Making
>>> >subnodes is not the solution.
>>>
>>> The problem is mapping clock domains to logical CPUs that CPUfreq 
>>> uses. The
>>> physical CPU to logical CPU mapping can be changed by the kernel (even
>>> through DT if I'm not mistaken). So we need to have a way to tell in DT
>>> which physical CPUs are connected to which CPU freq clock domain.
>>>
>>
>> How about passing CPU freq clock domain id as along with phandle in
>> qcom,freq-domain ?
> 
> Now sure what you mean here. There's no such this as CPUfreq clock 
> domain id. It has policies that are made up of logical CPU numbers. 
> Logical CPU is not something that you can fix in DT.
> 
> -Saravana

Sudeep,

Earlier the design was the freq_domain would take the CPU phandles

freq_domain:
   cpus = <&cpu0 &cpu1....>;

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

--

Re: [PATCH v7 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings

[Stephen Boyd]

Quoting Taniya Das (2018-08-07 19:46:01)
> 
> 
> On 8/8/2018 12:54 AM, skannan@codeaurora.org wrote:
> > On 2018-08-07 04:12, Sudeep Holla wrote:
> >> On Mon, Aug 06, 2018 at 01:54:24PM -0700, skannan@codeaurora.org wrote:
> >>> On 2018-08-03 16:46, Stephen Boyd wrote:
> >>> >Quoting Taniya Das (2018-07-24 03:42:49)
> >>> >>diff --git
> >>> >>a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> >>> >>b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> >>> >>new file mode 100644
> >>> >>index 0000000..22d4355
> >>> >>--- /dev/null
> >>> >>+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> >>> >>@@ -0,0 +1,172 @@
> >>> >[...]
> >>> >>+
> >>> >>+               CPU7: cpu@700 {
> >>> >>+                       device_type = "cpu";
> >>> >>+                       compatible = "qcom,kryo385";
> >>> >>+                       reg = <0x0 0x700>;
> >>> >>+                       enable-method = "psci";
> >>> >>+                       next-level-cache = <&L2_700>;
> >>> >>+                       qcom,freq-domain = <&freq_domain_table1>;
> >>> >>+                       L2_700: l2-cache {
> >>> >>+                               compatible = "cache";
> >>> >>+                               next-level-cache = <&L3_0>;
> >>> >>+                       };
> >>> >>+               };
> >>> >>+       };
> >>> >>+
> >>> >>+       qcom,cpufreq-hw {
> >>> >>+               compatible = "qcom,cpufreq-hw";
> >>> >>+
> >>> >>+               clocks = <&rpmhcc RPMH_CXO_CLK>;
> >>> >>+               clock-names = "xo";
> >>> >>+
> >>> >>+               #address-cells = <2>;
> >>> >>+               #size-cells = <2>;
> >>> >>+               ranges;
> >>> >>+               freq_domain_table0: freq_table0 {
> >>> >>+                       reg = <0 0x17d43000 0 0x1400>;
> >>> >>+               };
> >>> >>+
> >>> >>+               freq_domain_table1: freq_table1 {
> >>> >>+                       reg = <0 0x17d45800 0 0x1400>;
> >>> >>+               };
> >>> >
> >>> >Sorry, this is just not proper DT design. The whole node should have a
> >>> >reg property, and it should contain two (or three if we're handling the
> >>> >L3 clk domain?) different offsets for the different power clusters. The
> >>> >problem seems to still be that we don't have a way to map the CPUs to
> >>> >the clk domains they're in provided by this hardware block. Making
> >>> >subnodes is not the solution.
> >>>
> >>> The problem is mapping clock domains to logical CPUs that CPUfreq 
> >>> uses. The
> >>> physical CPU to logical CPU mapping can be changed by the kernel (even
> >>> through DT if I'm not mistaken). So we need to have a way to tell in DT
> >>> which physical CPUs are connected to which CPU freq clock domain.
> >>>
> >>
> >> How about passing CPU freq clock domain id as along with phandle in
> >> qcom,freq-domain ?
> > 
> > Now sure what you mean here. There's no such this as CPUfreq clock 
> > domain id. It has policies that are made up of logical CPU numbers. 
> > Logical CPU is not something that you can fix in DT.
> > 
> > -Saravana
> 
> Sudeep,
> 
> Earlier the design was the freq_domain would take the CPU phandles
> 
> freq_domain:
>    cpus = <&cpu0 &cpu1....>;
> 

I believe Sudeep is recommending something I recommended earlier. It
would look like:

   cpu7 {
	   qcom,freq-domain = <&cpufreq_hw 1>;
   }

to indicate that cpu7 is in cpufreq_hw's frequency domain #1. That
should probably be called clk domain BTW.

If that was done with a phandle and a single cell, then we should have
something similar on the cpufreq_hw node side indicating how to parse
the cells in qcom,freq-domain. A property like #qcom,freq-domain-cells =
<1> to indicate that one u32 follows the phandle.

Re: [PATCH v7 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Stephen Boyd]

Quoting skannan@codeaurora.org (2018-08-06 13:46:05)
> On 2018-08-03 15:24, Stephen Boyd wrote:
> > Quoting skannan@codeaurora.org (2018-08-03 12:52:48)
> >> On 2018-08-03 12:40, Evan Green wrote:
> >> > Hi Taniya,
> >> >
> >> > On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <tdas@codeaurora.org> wrote:
> >> >>
> >> >> +               if (src)
> >> >> +                       c->table[i].frequency = c->xo_rate * lval /
> >> >> 1000;
> >> >> +               else
> >> >> +                       c->table[i].frequency = INIT_RATE / 1000;
> >> >
> >> > I don't know much about how this hardware works, but based on the
> >> > mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
> >> > and 3 all mean xo_rate?
> >> >
> >> > Also, is INIT_RATE really constant? It sounds like gpll0 (or
> >> > gpll0_out_even?). You're already getting the xo clock, why not get
> >> > gpll0's real rate as well?
> >> 
> >> Actually I was about to comment and say NOT to get clocks just to get
> >> their rate. The XO_RATE is just a multiplication factor. This HW/FW 
> >> can
> >> change in the future and make the multiplication factor to 1KHz.
> > 
> > So future changes to this hardware are going to make this register
> > return the final frequency that we should use? Sounds great! But that
> > isn't how it's working right now. We need to have XO in the binding 
> > here
> > so the driver can work with different XO frequencies in case that ever
> > happens. Same story for GPLL0. Hardcoding this stuff in the driver just
> > means we'll have to swizzle things in the driver when it changes.
> 
> XO being a different frequency in a Qualcomm chip is way way less likely 
> than anything else we are discussing here. In the 10+years I've been 
> there this has never changed.
> 
> So, how about making this binding optional? If it's not present we can 
> make assumptions for XO rate and init rate. That'll handle your 
> hypothetical use case while also being optimized.

Optional properties are almost never correct for clks. Either the clk
goes there or it doesn't go there. The only time it's optional is if the
HW has the choice of generating the clk itself internally or to use an
external clk as input.

In this case, it's not optional, it's actually used so marking it
optional is highly confusing.

> 
> >> We also
> >> can't really control any of the clocks going to this block from Linux
> >> (it's all locked down).
> > 
> > This shouldn't matter. The clocks going to this hardware block are
> > described by the firmware to the OS by means of the DT node. If the
> > firmware or the hardware decides to change the input clks then the
> > binding can have different clk nodes used.
> 
> There are tons of clocks that are input to blocks in an SoC that are 
> never controlled by Linux. Or are expose in DT because they are set up 
> ahead of time and can't change. So, why make an exception here?

Because the driver doesn't have to hard-code some frequency that can
easily come from the DT, making it more flexible for frequency plan
changes. It doesn't matter about control of clks at all here, so this
comparison is just plain wrong.

> 
> >> The INIT_RATE will
> >> always be 300 MHz independent of what source it's coming from (as in, 
> >> if
> >> GPLL0 can't give 300, the HW design will be so that we'll find a
> >> different source).
> >> 
> >> So, I'd like to remove any clock bindings for this driver.
> > 
> > No. Bindings describe how the hardware is connected. Please don't 
> > remove
> > clocks from the binding just because probe defer is a concern.
> 
> Binding describes hardware controllable by the OS. That's the reality. 
> Let's not add mandatory clock bindings for clocks that the OS can't do 
> anything about.
> 

It seems that you believe clks should only be used to turn on/off and
control rates. That is not the whole truth. Sometimes clks are there
just to express the clk frequencies that are present in the design so
that drivers can figure out what to do.

Re: [PATCH v7 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings

[Sudeep Holla]

On Tue, Aug 07, 2018 at 11:15:02PM -0700, Stephen Boyd wrote:
> Quoting Taniya Das (2018-08-07 19:46:01)
[...]

> > Sudeep,
> > 
> > Earlier the design was the freq_domain would take the CPU phandles
> > 
> > freq_domain:
> >    cpus = <&cpu0 &cpu1....>;
> > 
> 
> I believe Sudeep is recommending something I recommended earlier. It
> would look like:
> 
>    cpu7 {
> 	   qcom,freq-domain = <&cpufreq_hw 1>;
>    }
> 
> to indicate that cpu7 is in cpufreq_hw's frequency domain #1. That
> should probably be called clk domain BTW.
>

Thanks Stephen, that's exactly what I meant. You have explained all the
details saving me time :)

> If that was done with a phandle and a single cell, then we should have
> something similar on the cpufreq_hw node side indicating how to parse
> the cells in qcom,freq-domain. A property like #qcom,freq-domain-cells =
> <1> to indicate that one u32 follows the phandle.
>

As Saravana mentions in the other email I can't believe it's just made up
of logical CPU numbers. If that's the case is it software configurable.

--
Regards,
Sudeep

Re: [PATCH v7 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Taniya Das]

On 8/8/2018 11:52 AM, Stephen Boyd wrote:
> Quoting skannan@codeaurora.org (2018-08-06 13:46:05)
>> On 2018-08-03 15:24, Stephen Boyd wrote:
>>> Quoting skannan@codeaurora.org (2018-08-03 12:52:48)
>>>> On 2018-08-03 12:40, Evan Green wrote:
>>>>> Hi Taniya,
>>>>>
>>>>> On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <tdas@codeaurora.org> wrote:
>>>>>>
>>>>>> +               if (src)
>>>>>> +                       c->table[i].frequency = c->xo_rate * lval /
>>>>>> 1000;
>>>>>> +               else
>>>>>> +                       c->table[i].frequency = INIT_RATE / 1000;
>>>>>
>>>>> I don't know much about how this hardware works, but based on the
>>>>> mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
>>>>> and 3 all mean xo_rate?
>>>>>
>>>>> Also, is INIT_RATE really constant? It sounds like gpll0 (or
>>>>> gpll0_out_even?). You're already getting the xo clock, why not get
>>>>> gpll0's real rate as well?
>>>>
>>>> Actually I was about to comment and say NOT to get clocks just to get
>>>> their rate. The XO_RATE is just a multiplication factor. This HW/FW
>>>> can
>>>> change in the future and make the multiplication factor to 1KHz.
>>>
>>> So future changes to this hardware are going to make this register
>>> return the final frequency that we should use? Sounds great! But that
>>> isn't how it's working right now. We need to have XO in the binding
>>> here
>>> so the driver can work with different XO frequencies in case that ever
>>> happens. Same story for GPLL0. Hardcoding this stuff in the driver just
>>> means we'll have to swizzle things in the driver when it changes.
>>
>> XO being a different frequency in a Qualcomm chip is way way less likely
>> than anything else we are discussing here. In the 10+years I've been
>> there this has never changed.
>>
>> So, how about making this binding optional? If it's not present we can
>> make assumptions for XO rate and init rate. That'll handle your
>> hypothetical use case while also being optimized.
> 
> Optional properties are almost never correct for clks. Either the clk
> goes there or it doesn't go there. The only time it's optional is if the
> HW has the choice of generating the clk itself internally or to use an
> external clk as input.
> 
> In this case, it's not optional, it's actually used so marking it
> optional is highly confusing.
> 
>>
>>>> We also
>>>> can't really control any of the clocks going to this block from Linux
>>>> (it's all locked down).
>>>
>>> This shouldn't matter. The clocks going to this hardware block are
>>> described by the firmware to the OS by means of the DT node. If the
>>> firmware or the hardware decides to change the input clks then the
>>> binding can have different clk nodes used.
>>
>> There are tons of clocks that are input to blocks in an SoC that are
>> never controlled by Linux. Or are expose in DT because they are set up
>> ahead of time and can't change. So, why make an exception here?
> 
> Because the driver doesn't have to hard-code some frequency that can
> easily come from the DT, making it more flexible for frequency plan
> changes. It doesn't matter about control of clks at all here, so this
> comparison is just plain wrong.
> 
>>
>>>> The INIT_RATE will
>>>> always be 300 MHz independent of what source it's coming from (as in,
>>>> if
>>>> GPLL0 can't give 300, the HW design will be so that we'll find a
>>>> different source).
>>>>
>>>> So, I'd like to remove any clock bindings for this driver.
>>>
>>> No. Bindings describe how the hardware is connected. Please don't
>>> remove
>>> clocks from the binding just because probe defer is a concern.
>>
>> Binding describes hardware controllable by the OS. That's the reality.
>> Let's not add mandatory clock bindings for clocks that the OS can't do
>> anything about.
>>
> 
> It seems that you believe clks should only be used to turn on/off and
> control rates. That is not the whole truth. Sometimes clks are there
> just to express the clk frequencies that are present in the design so
> that drivers can figure out what to do.
> 

Stephen,

As this clock is not configurable by linux clock drivers and we really 
do not care the parent src(as mentioned by Saravana) to generate the 
300MHz, would it be good to define a fixed rate clock so as to express 
the HW connectivity & frequency?

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

--

Re: [PATCH v7 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Stephen Boyd]

Quoting Taniya Das (2018-08-08 03:15:26)
> 
> 
> On 8/8/2018 11:52 AM, Stephen Boyd wrote:
> >>
> >> Binding describes hardware controllable by the OS. That's the reality.
> >> Let's not add mandatory clock bindings for clocks that the OS can't do
> >> anything about.
> >>
> > 
> > It seems that you believe clks should only be used to turn on/off and
> > control rates. That is not the whole truth. Sometimes clks are there
> > just to express the clk frequencies that are present in the design so
> > that drivers can figure out what to do.
> > 
> 
> Stephen,
> 
> As this clock is not configurable by linux clock drivers and we really 
> do not care the parent src(as mentioned by Saravana) to generate the 
> 300MHz, would it be good to define a fixed rate clock so as to express 
> the HW connectivity & frequency?
> 

As a hack that works great, but why do we need to workaround problems by
adding a fixed rate clk to DT for this PLL? The PLL is provided by GCC
node so it should be connected to the GCC node.

Re: [PATCH v7 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Matthias Kaehlcke]

Hi Taniya,

On Tue, Jul 24, 2018 at 04:12:50PM +0530, Taniya Das wrote:
> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this hardware engine.
> 
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  drivers/cpufreq/Kconfig.arm       |  11 ++
>  drivers/cpufreq/Makefile          |   1 +
>  drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 360 insertions(+)
>  create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 0cd8eb7..93a9d72 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -298,3 +298,14 @@ config ARM_PXA2xx_CPUFREQ
>  	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
> 
>  	  If in doubt, say N.
> +
> +config ARM_QCOM_CPUFREQ_HW
> +	bool "QCOM CPUFreq HW driver"
> +	depends on ARCH_QCOM
> +	help
> +	 Support for the CPUFreq HW driver.
> +	 Some QCOM chipsets have a HW engine to offload the steps
> +	 necessary for changing the frequency of the CPUs. Firmware loaded
> +	 in this engine exposes a programming interface to the OS.
> +	 The driver implements the cpufreq interface for this HW engine.
> +	 Say Y if you want to support CPUFreq HW.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index c1ffeab..ca48a1d 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_HW)	+= qcom-cpufreq-hw.o
> 
> 
>  ##################################################################################
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> new file mode 100644
> index 0000000..ea8f7d1
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -0,0 +1,348 @@
> +// 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 LUT_MAX_ENTRIES			40U
> +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
> +#define LUT_ROW_SIZE			32
> +
> +enum {
> +	REG_ENABLE,
> +	REG_LUT_TABLE,
> +	REG_PERF_STATE,
> +
> +	REG_ARRAY_SIZE,
> +};
> +
> +struct cpufreq_qcom {
> +	struct cpufreq_frequency_table *table;
> +	struct device *dev;

'dev' is not used and can be removed.

> ...
> 
> +static int qcom_cpu_resources_init(struct platform_device *pdev,
> +				   struct device_node *np, unsigned int cpu,
> +				   unsigned long xo_rate)
> +{
> +	struct cpufreq_qcom *c;
> +	struct resource res;
> +	struct device *dev = &pdev->dev;
> +	const u16 *offsets;
> +	int ret, i, cpu_first, cpu_r;
> +	void __iomem *base;
> +
> +	if (qcom_freq_domain_map[cpu])
> +		return 0;
> +
> +	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> +	if (!c)
> +		return -ENOMEM;
> +
> +	offsets = of_device_get_match_data(&pdev->dev);
> +	if (!offsets)
> +		return -EINVAL;
> +
> +	if (of_address_to_resource(np, 0, &res))
> +		return -ENOMEM;
> +
> +	base = devm_ioremap_resource(dev, &res);
> +	if (!base)
> +		return -ENOMEM;
> +
> +	for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
> +		c->reg_bases[i] = base + offsets[i];
> +
> +	/* HW should be in enabled state to proceed */
> +	if (!(readl_relaxed(c->reg_bases[REG_ENABLE]) & 0x1)) {
> +		dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
> +		return -ENODEV;
> +	}
> +
> +	ret = qcom_get_related_cpus(np, &c->related_cpus);
> +	if (ret) {
> +		dev_err(dev, "%s failed to get related CPUs\n", np->name);
> +		return ret;
> +	}
> +
> +	c->max_cores = cpumask_weight(&c->related_cpus);
> +	if (!c->max_cores)
> +		return -ENOENT;
> +
> +	c->xo_rate = xo_rate;
> +
> +	ret = qcom_cpufreq_hw_read_lut(pdev, c);
> +	if (ret) {
> +		dev_err(dev, "%s failed to read LUT\n", np->name);
> +		return ret;
> +	}
> +
> +	qcom_freq_domain_map[cpu] = c;
> +
> +	/* Related CPUs */
> +	cpu_first = cpumask_first(&c->related_cpus);
> +
> +	for_each_cpu(cpu_r, &c->related_cpus) {
> +		if (cpu_r != cpu_first)
> +			qcom_freq_domain_map[cpu_r] =
> +				qcom_freq_domain_map[cpu_first];
> +	}

The above ten lines could be simplified to:

	for_each_cpu(cpu_r, &c->related_cpus)
		qcom_freq_domain_map[cpu_r] = c;

> ...
> 
> +static int __init qcom_cpufreq_hw_init(void)
> +{
> +	return platform_driver_register(&qcom_cpufreq_hw_driver);
> +}
> +subsys_initcall(qcom_cpufreq_hw_init);

Is subsys_initcall used for a particular reason? It will cause
problems when registering cooling devices, since the thermal device
class is initialized through an fs_initcall, which are executed
later.

Most cpufreq drivers use module_init, device_initcall or
late_initcall, can't this driver use one of those?

Cheers

Matthias

Re: [PATCH v7 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Amit Kucheria]

Hi Taniya,

How much have you stressed this driver?

I tried it on top of an integration branch based on 4.19-rc2 that we
maintain[1] and was able to get the board to reboot fairly easily with
just a few "yes > /dev/null &" instances running in the background.

I even tried with interconnect framework disabled to be sure, and it
still reboots.

I'll continue trying to pinpoint the source of the problem, but it
would be nice to know what tree you're testing against.

Regards,
Amit
[1] https://git.linaro.org/people/amit.kucheria/kernel.git/log/?h=integration/qcomlt-automerge-result

On Tue, Jul 24, 2018 at 4:12 PM, Taniya Das <tdas@codeaurora.org> wrote:
> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this hardware engine.
>
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  drivers/cpufreq/Kconfig.arm       |  11 ++
>  drivers/cpufreq/Makefile          |   1 +
>  drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 360 insertions(+)
>  create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 0cd8eb7..93a9d72 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -298,3 +298,14 @@ config ARM_PXA2xx_CPUFREQ
>           This add the CPUFreq driver support for Intel PXA2xx SOCs.
>
>           If in doubt, say N.
> +
> +config ARM_QCOM_CPUFREQ_HW
> +       bool "QCOM CPUFreq HW driver"
> +       depends on ARCH_QCOM
> +       help
> +        Support for the CPUFreq HW driver.
> +        Some QCOM chipsets have a HW engine to offload the steps
> +        necessary for changing the frequency of the CPUs. Firmware loaded
> +        in this engine exposes a programming interface to the OS.
> +        The driver implements the cpufreq interface for this HW engine.
> +        Say Y if you want to support CPUFreq HW.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index c1ffeab..ca48a1d 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_HW)      += qcom-cpufreq-hw.o
>
>
>  ##################################################################################
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> new file mode 100644
> index 0000000..ea8f7d1
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -0,0 +1,348 @@
> +// 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 LUT_MAX_ENTRIES                        40U
> +#define CORE_COUNT_VAL(val)            (((val) & (GENMASK(18, 16))) >> 16)
> +#define LUT_ROW_SIZE                   32
> +
> +enum {
> +       REG_ENABLE,
> +       REG_LUT_TABLE,
> +       REG_PERF_STATE,
> +
> +       REG_ARRAY_SIZE,
> +};
> +
> +struct cpufreq_qcom {
> +       struct cpufreq_frequency_table *table;
> +       struct device *dev;
> +       void __iomem *reg_bases[REG_ARRAY_SIZE];
> +       cpumask_t related_cpus;
> +       unsigned int max_cores;
> +       unsigned long xo_rate;
> +};
> +
> +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
> +       [REG_ENABLE]            = 0x0,
> +       [REG_LUT_TABLE]         = 0x110,
> +       [REG_PERF_STATE]        = 0x920,
> +};
> +
> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> +
> +static int
> +qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> +                            unsigned int index)
> +{
> +       struct cpufreq_qcom *c = policy->driver_data;
> +
> +       writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> +
> +       return 0;
> +}
> +
> +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
> +{
> +       struct cpufreq_qcom *c;
> +       struct cpufreq_policy *policy;
> +       unsigned int index;
> +
> +       policy = cpufreq_cpu_get_raw(cpu);
> +       if (!policy)
> +               return 0;
> +
> +       c = policy->driver_data;
> +
> +       index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
> +       index = min(index, LUT_MAX_ENTRIES - 1);
> +
> +       return policy->freq_table[index].frequency;
> +}
> +
> +static unsigned int
> +qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
> +                           unsigned int target_freq)
> +{
> +       struct cpufreq_qcom *c = policy->driver_data;
> +       int index;
> +
> +       index = policy->cached_resolved_idx;
> +       if (index < 0)
> +               return 0;
> +
> +       writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> +
> +       return policy->freq_table[index].frequency;
> +}
> +
> +static int qcom_cpufreq_hw_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->fast_switch_possible = true;
> +       policy->freq_table = c->table;
> +       policy->driver_data = c;
> +
> +       return 0;
> +}
> +
> +static struct freq_attr *qcom_cpufreq_hw_attr[] = {
> +       &cpufreq_freq_attr_scaling_available_freqs,
> +       &cpufreq_freq_attr_scaling_boost_freqs,
> +       NULL
> +};
> +
> +static struct cpufreq_driver cpufreq_qcom_hw_driver = {
> +       .flags          = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
> +                         CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
> +       .verify         = cpufreq_generic_frequency_table_verify,
> +       .target_index   = qcom_cpufreq_hw_target_index,
> +       .get            = qcom_cpufreq_hw_get,
> +       .init           = qcom_cpufreq_hw_cpu_init,
> +       .fast_switch    = qcom_cpufreq_hw_fast_switch,
> +       .name           = "qcom-cpufreq-hw",
> +       .attr           = qcom_cpufreq_hw_attr,
> +       .boost_enabled  = true,
> +};
> +
> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
> +                                   struct cpufreq_qcom *c)
> +{
> +       struct device *dev = &pdev->dev;
> +       void __iomem *base;
> +       u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
> +
> +       c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> +                               sizeof(*c->table), GFP_KERNEL);
> +       if (!c->table)
> +               return -ENOMEM;
> +
> +       base = c->reg_bases[REG_LUT_TABLE];
> +
> +       for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> +               data = readl_relaxed(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 = c->xo_rate * lval / 1000;
> +               else
> +                       c->table[i].frequency = INIT_RATE / 1000;
> +
> +               cur_freq = c->table[i].frequency;
> +
> +               dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> +                       i, c->table[i].frequency, core_count);
> +
> +               if (core_count != c->max_cores)
> +                       cur_freq = CPUFREQ_ENTRY_INVALID;
> +
> +               /*
> +                * Two of the same frequencies with the same core counts means
> +                * end of table.
> +                */
> +               if (i > 0 && c->table[i - 1].frequency ==
> +                  c->table[i].frequency && prev_cc == core_count) {
> +                       struct cpufreq_frequency_table *prev = &c->table[i - 1];
> +
> +                       if (prev_freq == CPUFREQ_ENTRY_INVALID)
> +                               prev->flags = CPUFREQ_BOOST_FREQ;
> +                       break;
> +               }
> +               prev_cc = core_count;
> +               prev_freq = cur_freq;
> +       }
> +
> +       c->table[i].frequency = CPUFREQ_TABLE_END;
> +
> +       return 0;
> +}
> +
> +static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
> +{
> +       struct device_node *cpu_np, *freq_np;
> +       int cpu;
> +
> +       for_each_possible_cpu(cpu) {
> +               cpu_np = of_cpu_device_node_get(cpu);
> +               if (!cpu_np)
> +                       continue;
> +               freq_np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
> +               of_node_put(cpu_np);
> +               if (!freq_np)
> +                       continue;
> +
> +               if (freq_np == np)
> +                       cpumask_set_cpu(cpu, m);
> +       }
> +
> +       return 0;
> +}
> +
> +static int qcom_cpu_resources_init(struct platform_device *pdev,
> +                                  struct device_node *np, unsigned int cpu,
> +                                  unsigned long xo_rate)
> +{
> +       struct cpufreq_qcom *c;
> +       struct resource res;
> +       struct device *dev = &pdev->dev;
> +       const u16 *offsets;
> +       int ret, i, cpu_first, cpu_r;
> +       void __iomem *base;
> +
> +       if (qcom_freq_domain_map[cpu])
> +               return 0;
> +
> +       c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> +       if (!c)
> +               return -ENOMEM;
> +
> +       offsets = of_device_get_match_data(&pdev->dev);
> +       if (!offsets)
> +               return -EINVAL;
> +
> +       if (of_address_to_resource(np, 0, &res))
> +               return -ENOMEM;
> +
> +       base = devm_ioremap_resource(dev, &res);
> +       if (!base)
> +               return -ENOMEM;
> +
> +       for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
> +               c->reg_bases[i] = base + offsets[i];
> +
> +       /* HW should be in enabled state to proceed */
> +       if (!(readl_relaxed(c->reg_bases[REG_ENABLE]) & 0x1)) {
> +               dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
> +               return -ENODEV;
> +       }
> +
> +       ret = qcom_get_related_cpus(np, &c->related_cpus);
> +       if (ret) {
> +               dev_err(dev, "%s failed to get related CPUs\n", np->name);
> +               return ret;
> +       }
> +
> +       c->max_cores = cpumask_weight(&c->related_cpus);
> +       if (!c->max_cores)
> +               return -ENOENT;
> +
> +       c->xo_rate = xo_rate;
> +
> +       ret = qcom_cpufreq_hw_read_lut(pdev, c);
> +       if (ret) {
> +               dev_err(dev, "%s failed to read LUT\n", np->name);
> +               return ret;
> +       }
> +
> +       qcom_freq_domain_map[cpu] = c;
> +
> +       /* Related CPUs */
> +       cpu_first = cpumask_first(&c->related_cpus);
> +
> +       for_each_cpu(cpu_r, &c->related_cpus) {
> +               if (cpu_r != cpu_first)
> +                       qcom_freq_domain_map[cpu_r] =
> +                               qcom_freq_domain_map[cpu_first];
> +       }
> +
> +       return 0;
> +}
> +
> +static int qcom_resources_init(struct platform_device *pdev)
> +{
> +       struct device_node *np, *cpu_np;
> +       struct clk *clk;
> +       unsigned int cpu;
> +       int ret;
> +
> +       clk = devm_clk_get(&pdev->dev, "xo");
> +       if (IS_ERR(clk))
> +               return PTR_ERR(clk);
> +
> +       for_each_possible_cpu(cpu) {
> +               cpu_np = of_cpu_device_node_get(cpu);
> +               if (!cpu_np) {
> +                       dev_dbg(&pdev->dev, "Failed to get cpu %d device\n",
> +                               cpu);
> +                       continue;
> +               }
> +
> +               np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
> +               of_node_put(cpu_np);
> +               if (!np) {
> +                       dev_err(&pdev->dev, "Failed to get freq-domain device\n");
> +                       return -EINVAL;
> +               }
> +
> +               ret = qcom_cpu_resources_init(pdev, np, cpu, clk_get_rate(clk));
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       devm_clk_put(&pdev->dev, clk);
> +
> +       return 0;
> +}
> +
> +static int qcom_cpufreq_hw_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_hw_driver);
> +       if (rc) {
> +               dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
> +               return rc;
> +       }
> +
> +       dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id qcom_cpufreq_hw_match[] = {
> +       { .compatible = "qcom,cpufreq-hw", .data = &cpufreq_qcom_std_offsets },
> +       {}
> +};
> +
> +static struct platform_driver qcom_cpufreq_hw_driver = {
> +       .probe = qcom_cpufreq_hw_driver_probe,
> +       .driver = {
> +               .name = "qcom-cpufreq-hw",
> +               .of_match_table = qcom_cpufreq_hw_match,
> +       },
> +};
> +
> +static int __init qcom_cpufreq_hw_init(void)
> +{
> +       return platform_driver_register(&qcom_cpufreq_hw_driver);
> +}
> +subsys_initcall(qcom_cpufreq_hw_init);
> +
> +MODULE_DESCRIPTION("QCOM firmware-based CPU Frequency driver");
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
> of the Code Aurora Forum, hosted by the  Linux Foundation.
>

Re: [PATCH v7 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Matthias Kaehlcke]

On Tue, Jul 24, 2018 at 04:12:50PM +0530, Taniya Das wrote:
> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this hardware engine.
> 
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  drivers/cpufreq/Kconfig.arm       |  11 ++
>  drivers/cpufreq/Makefile          |   1 +
>  drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 360 insertions(+)
>  create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 0cd8eb7..93a9d72 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -298,3 +298,14 @@ config ARM_PXA2xx_CPUFREQ
>  	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
> 
>  	  If in doubt, say N.
> +
> +config ARM_QCOM_CPUFREQ_HW
> +	bool "QCOM CPUFreq HW driver"
> +	depends on ARCH_QCOM
> +	help
> +	 Support for the CPUFreq HW driver.
> +	 Some QCOM chipsets have a HW engine to offload the steps
> +	 necessary for changing the frequency of the CPUs. Firmware loaded
> +	 in this engine exposes a programming interface to the OS.
> +	 The driver implements the cpufreq interface for this HW engine.
> +	 Say Y if you want to support CPUFreq HW.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index c1ffeab..ca48a1d 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_HW)	+= qcom-cpufreq-hw.o
> 
> 
>  ##################################################################################
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> new file mode 100644
> index 0000000..ea8f7d1
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -0,0 +1,348 @@
> +// 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 LUT_MAX_ENTRIES			40U
> +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
> +#define LUT_ROW_SIZE			32
> +
> +enum {
> +	REG_ENABLE,
> +	REG_LUT_TABLE,
> +	REG_PERF_STATE,
> +
> +	REG_ARRAY_SIZE,
> +};
> +
> +struct cpufreq_qcom {
> +	struct cpufreq_frequency_table *table;
> +	struct device *dev;
> +	void __iomem *reg_bases[REG_ARRAY_SIZE];
> +	cpumask_t related_cpus;
> +	unsigned int max_cores;
> +	unsigned long xo_rate;
> +};
> +
> +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
> +	[REG_ENABLE]		= 0x0,
> +	[REG_LUT_TABLE]		= 0x110,
> +	[REG_PERF_STATE]	= 0x920,
> +};
> +
> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> +
> +static int
> +qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> +			     unsigned int index)
> +{
> +	struct cpufreq_qcom *c = policy->driver_data;
> +
> +	writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> +
> +	return 0;
> +}
> +
> +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
> +{
> +	struct cpufreq_qcom *c;
> +	struct cpufreq_policy *policy;
> +	unsigned int index;
> +
> +	policy = cpufreq_cpu_get_raw(cpu);
> +	if (!policy)
> +		return 0;
> +
> +	c = policy->driver_data;
> +
> +	index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
> +	index = min(index, LUT_MAX_ENTRIES - 1);
> +
> +	return policy->freq_table[index].frequency;
> +}
> +
> +static unsigned int
> +qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
> +			    unsigned int target_freq)
> +{
> +	struct cpufreq_qcom *c = policy->driver_data;
> +	int index;
> +
> +	index = policy->cached_resolved_idx;
> +	if (index < 0)
> +		return 0;
> +
> +	writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> +
> +	return policy->freq_table[index].frequency;
> +}
> +
> +static int qcom_cpufreq_hw_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->fast_switch_possible = true;
> +	policy->freq_table = c->table;
> +	policy->driver_data = c;
> +
> +	return 0;
> +}
> +
> +static struct freq_attr *qcom_cpufreq_hw_attr[] = {
> +	&cpufreq_freq_attr_scaling_available_freqs,
> +	&cpufreq_freq_attr_scaling_boost_freqs,
> +	NULL
> +};
> +
> +static struct cpufreq_driver cpufreq_qcom_hw_driver = {
> +	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
> +			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
> +	.verify		= cpufreq_generic_frequency_table_verify,
> +	.target_index	= qcom_cpufreq_hw_target_index,
> +	.get		= qcom_cpufreq_hw_get,
> +	.init		= qcom_cpufreq_hw_cpu_init,
> +	.fast_switch    = qcom_cpufreq_hw_fast_switch,
> +	.name		= "qcom-cpufreq-hw",
> +	.attr		= qcom_cpufreq_hw_attr,
> +	.boost_enabled	= true,
> +};
> +
> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
> +				    struct cpufreq_qcom *c)
> +{
> +	struct device *dev = &pdev->dev;
> +	void __iomem *base;
> +	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
> +
> +	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> +				sizeof(*c->table), GFP_KERNEL);
> +	if (!c->table)
> +		return -ENOMEM;
> +
> +	base = c->reg_bases[REG_LUT_TABLE];
> +
> +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> +		data = readl_relaxed(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 = c->xo_rate * lval / 1000;
> +		else
> +			c->table[i].frequency = INIT_RATE / 1000;
> +
> +		cur_freq = c->table[i].frequency;
> +
> +		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> +			i, c->table[i].frequency, core_count);
> +
> +		if (core_count != c->max_cores)
> +			cur_freq = CPUFREQ_ENTRY_INVALID;
> +

I noticed that the 'power_allocator' thermal governor currently can't
be used with this driver since there is no OPP table with frequency and
voltage information. Does the LUT contain information about the
voltage or is there another mechanism to retrieve it?

Re: [PATCH v7 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Taniya Das]

Hello Stephen,


On 8/24/2018 12:08 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-08-08 03:15:26)
>>
>>
>> On 8/8/2018 11:52 AM, Stephen Boyd wrote:
>>>>
>>>> Binding describes hardware controllable by the OS. That's the reality.
>>>> Let's not add mandatory clock bindings for clocks that the OS can't do
>>>> anything about.
>>>>
>>>
>>> It seems that you believe clks should only be used to turn on/off and
>>> control rates. That is not the whole truth. Sometimes clks are there
>>> just to express the clk frequencies that are present in the design so
>>> that drivers can figure out what to do.
>>>
>>
>> Stephen,
>>
>> As this clock is not configurable by linux clock drivers and we really
>> do not care the parent src(as mentioned by Saravana) to generate the
>> 300MHz, would it be good to define a fixed rate clock so as to express
>> the HW connectivity & frequency?
>>
> 
> As a hack that works great, but why do we need to workaround problems by
> adding a fixed rate clk to DT for this PLL? The PLL is provided by GCC
> node so it should be connected to the GCC node.
> 

Please help with review the next patch series which would take the PLL 
phandle from DT.

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

--

Re: [PATCH v7 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Taniya Das]

Hello Matthias,

Thanks for your review comments.

On 8/29/2018 11:31 PM, Matthias Kaehlcke wrote:
> Hi Taniya,
> 
> On Tue, Jul 24, 2018 at 04:12:50PM +0530, Taniya Das wrote:
>> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this hardware engine.
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> ---
>>   drivers/cpufreq/Kconfig.arm       |  11 ++
>>   drivers/cpufreq/Makefile          |   1 +
>>   drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 360 insertions(+)
>>   create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 0cd8eb7..93a9d72 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -298,3 +298,14 @@ config ARM_PXA2xx_CPUFREQ
>>   	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
>>
>>   	  If in doubt, say N.
>> +
>> +config ARM_QCOM_CPUFREQ_HW
>> +	bool "QCOM CPUFreq HW driver"
>> +	depends on ARCH_QCOM
>> +	help
>> +	 Support for the CPUFreq HW driver.
>> +	 Some QCOM chipsets have a HW engine to offload the steps
>> +	 necessary for changing the frequency of the CPUs. Firmware loaded
>> +	 in this engine exposes a programming interface to the OS.
>> +	 The driver implements the cpufreq interface for this HW engine.
>> +	 Say Y if you want to support CPUFreq HW.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index c1ffeab..ca48a1d 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_HW)	+= qcom-cpufreq-hw.o
>>
>>
>>   ##################################################################################
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>> new file mode 100644
>> index 0000000..ea8f7d1
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -0,0 +1,348 @@
>> +// 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 LUT_MAX_ENTRIES			40U
>> +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
>> +#define LUT_ROW_SIZE			32
>> +
>> +enum {
>> +	REG_ENABLE,
>> +	REG_LUT_TABLE,
>> +	REG_PERF_STATE,
>> +
>> +	REG_ARRAY_SIZE,
>> +};
>> +
>> +struct cpufreq_qcom {
>> +	struct cpufreq_frequency_table *table;
>> +	struct device *dev;
> 
> 'dev' is not used and can be removed.
> 

Thanks, would remove in the next patch.

>> ...
>>
>> +static int qcom_cpu_resources_init(struct platform_device *pdev,
>> +				   struct device_node *np, unsigned int cpu,
>> +				   unsigned long xo_rate)
>> +{
>> +	struct cpufreq_qcom *c;
>> +	struct resource res;
>> +	struct device *dev = &pdev->dev;
>> +	const u16 *offsets;
>> +	int ret, i, cpu_first, cpu_r;
>> +	void __iomem *base;
>> +
>> +	if (qcom_freq_domain_map[cpu])
>> +		return 0;
>> +
>> +	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
>> +	if (!c)
>> +		return -ENOMEM;
>> +
>> +	offsets = of_device_get_match_data(&pdev->dev);
>> +	if (!offsets)
>> +		return -EINVAL;
>> +
>> +	if (of_address_to_resource(np, 0, &res))
>> +		return -ENOMEM;
>> +
>> +	base = devm_ioremap_resource(dev, &res);
>> +	if (!base)
>> +		return -ENOMEM;
>> +
>> +	for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
>> +		c->reg_bases[i] = base + offsets[i];
>> +
>> +	/* HW should be in enabled state to proceed */
>> +	if (!(readl_relaxed(c->reg_bases[REG_ENABLE]) & 0x1)) {
>> +		dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = qcom_get_related_cpus(np, &c->related_cpus);
>> +	if (ret) {
>> +		dev_err(dev, "%s failed to get related CPUs\n", np->name);
>> +		return ret;
>> +	}
>> +
>> +	c->max_cores = cpumask_weight(&c->related_cpus);
>> +	if (!c->max_cores)
>> +		return -ENOENT;
>> +
>> +	c->xo_rate = xo_rate;
>> +
>> +	ret = qcom_cpufreq_hw_read_lut(pdev, c);
>> +	if (ret) {
>> +		dev_err(dev, "%s failed to read LUT\n", np->name);
>> +		return ret;
>> +	}
>> +
>> +	qcom_freq_domain_map[cpu] = c;
>> +
>> +	/* Related CPUs */
>> +	cpu_first = cpumask_first(&c->related_cpus);
>> +
>> +	for_each_cpu(cpu_r, &c->related_cpus) {
>> +		if (cpu_r != cpu_first)
>> +			qcom_freq_domain_map[cpu_r] =
>> +				qcom_freq_domain_map[cpu_first];
>> +	}
> 
> The above ten lines could be simplified to:
> 
> 	for_each_cpu(cpu_r, &c->related_cpus)
> 		qcom_freq_domain_map[cpu_r] = c;
> 

Would clean it up in the next patch.

>> ...
>>
>> +static int __init qcom_cpufreq_hw_init(void)
>> +{
>> +	return platform_driver_register(&qcom_cpufreq_hw_driver);
>> +}
>> +subsys_initcall(qcom_cpufreq_hw_init);
> 
> Is subsys_initcall used for a particular reason? It will cause
> problems when registering cooling devices, since the thermal device
> class is initialized through an fs_initcall, which are executed
> later.
> 
> Most cpufreq drivers use module_init, device_initcall or
> late_initcall, can't this driver use one of those?
> 

We want the CPU to be scaling to the highest frequency at the earliest.

> Cheers
> 
> Matthias
> 

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

--

Re: [PATCH v7 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Taniya Das]

Hi Amit,

On 9/9/2018 8:04 PM, Amit Kucheria wrote:
> Hi Taniya,
> 
> How much have you stressed this driver?
> 
> I tried it on top of an integration branch based on 4.19-rc2 that we
> maintain[1] and was able to get the board to reboot fairly easily with
> just a few "yes > /dev/null &" instances running in the background.
> 
> I even tried with interconnect framework disabled to be sure, and it
> still reboots.
> 

The driver is being tested with 4.14 kernel version and I have not yet 
got any reports of reboots. Though I would also try to check with teams 
internally.

> I'll continue trying to pinpoint the source of the problem, but it
> would be nice to know what tree you're testing against.
> 
> Regards,
> Amit
> [1] https://git.linaro.org/people/amit.kucheria/kernel.git/log/?h=integration/qcomlt-automerge-result
> 
> On Tue, Jul 24, 2018 at 4:12 PM, Taniya Das <tdas@codeaurora.org> wrote:
>> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this hardware engine.
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> ---
>>   drivers/cpufreq/Kconfig.arm       |  11 ++
>>   drivers/cpufreq/Makefile          |   1 +
>>   drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 360 insertions(+)
>>   create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 0cd8eb7..93a9d72 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -298,3 +298,14 @@ config ARM_PXA2xx_CPUFREQ
>>            This add the CPUFreq driver support for Intel PXA2xx SOCs.
>>
>>            If in doubt, say N.
>> +
>> +config ARM_QCOM_CPUFREQ_HW
>> +       bool "QCOM CPUFreq HW driver"
>> +       depends on ARCH_QCOM
>> +       help
>> +        Support for the CPUFreq HW driver.
>> +        Some QCOM chipsets have a HW engine to offload the steps
>> +        necessary for changing the frequency of the CPUs. Firmware loaded
>> +        in this engine exposes a programming interface to the OS.
>> +        The driver implements the cpufreq interface for this HW engine.
>> +        Say Y if you want to support CPUFreq HW.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index c1ffeab..ca48a1d 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_HW)      += qcom-cpufreq-hw.o
>>
>>
>>   ##################################################################################
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>> new file mode 100644
>> index 0000000..ea8f7d1
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -0,0 +1,348 @@
>> +// 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 LUT_MAX_ENTRIES                        40U
>> +#define CORE_COUNT_VAL(val)            (((val) & (GENMASK(18, 16))) >> 16)
>> +#define LUT_ROW_SIZE                   32
>> +
>> +enum {
>> +       REG_ENABLE,
>> +       REG_LUT_TABLE,
>> +       REG_PERF_STATE,
>> +
>> +       REG_ARRAY_SIZE,
>> +};
>> +
>> +struct cpufreq_qcom {
>> +       struct cpufreq_frequency_table *table;
>> +       struct device *dev;
>> +       void __iomem *reg_bases[REG_ARRAY_SIZE];
>> +       cpumask_t related_cpus;
>> +       unsigned int max_cores;
>> +       unsigned long xo_rate;
>> +};
>> +
>> +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
>> +       [REG_ENABLE]            = 0x0,
>> +       [REG_LUT_TABLE]         = 0x110,
>> +       [REG_PERF_STATE]        = 0x920,
>> +};
>> +
>> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
>> +
>> +static int
>> +qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
>> +                            unsigned int index)
>> +{
>> +       struct cpufreq_qcom *c = policy->driver_data;
>> +
>> +       writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
>> +
>> +       return 0;
>> +}
>> +
>> +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
>> +{
>> +       struct cpufreq_qcom *c;
>> +       struct cpufreq_policy *policy;
>> +       unsigned int index;
>> +
>> +       policy = cpufreq_cpu_get_raw(cpu);
>> +       if (!policy)
>> +               return 0;
>> +
>> +       c = policy->driver_data;
>> +
>> +       index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
>> +       index = min(index, LUT_MAX_ENTRIES - 1);
>> +
>> +       return policy->freq_table[index].frequency;
>> +}
>> +
>> +static unsigned int
>> +qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
>> +                           unsigned int target_freq)
>> +{
>> +       struct cpufreq_qcom *c = policy->driver_data;
>> +       int index;
>> +
>> +       index = policy->cached_resolved_idx;
>> +       if (index < 0)
>> +               return 0;
>> +
>> +       writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
>> +
>> +       return policy->freq_table[index].frequency;
>> +}
>> +
>> +static int qcom_cpufreq_hw_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->fast_switch_possible = true;
>> +       policy->freq_table = c->table;
>> +       policy->driver_data = c;
>> +
>> +       return 0;
>> +}
>> +
>> +static struct freq_attr *qcom_cpufreq_hw_attr[] = {
>> +       &cpufreq_freq_attr_scaling_available_freqs,
>> +       &cpufreq_freq_attr_scaling_boost_freqs,
>> +       NULL
>> +};
>> +
>> +static struct cpufreq_driver cpufreq_qcom_hw_driver = {
>> +       .flags          = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
>> +                         CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
>> +       .verify         = cpufreq_generic_frequency_table_verify,
>> +       .target_index   = qcom_cpufreq_hw_target_index,
>> +       .get            = qcom_cpufreq_hw_get,
>> +       .init           = qcom_cpufreq_hw_cpu_init,
>> +       .fast_switch    = qcom_cpufreq_hw_fast_switch,
>> +       .name           = "qcom-cpufreq-hw",
>> +       .attr           = qcom_cpufreq_hw_attr,
>> +       .boost_enabled  = true,
>> +};
>> +
>> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
>> +                                   struct cpufreq_qcom *c)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       void __iomem *base;
>> +       u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
>> +
>> +       c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
>> +                               sizeof(*c->table), GFP_KERNEL);
>> +       if (!c->table)
>> +               return -ENOMEM;
>> +
>> +       base = c->reg_bases[REG_LUT_TABLE];
>> +
>> +       for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> +               data = readl_relaxed(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 = c->xo_rate * lval / 1000;
>> +               else
>> +                       c->table[i].frequency = INIT_RATE / 1000;
>> +
>> +               cur_freq = c->table[i].frequency;
>> +
>> +               dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
>> +                       i, c->table[i].frequency, core_count);
>> +
>> +               if (core_count != c->max_cores)
>> +                       cur_freq = CPUFREQ_ENTRY_INVALID;
>> +
>> +               /*
>> +                * Two of the same frequencies with the same core counts means
>> +                * end of table.
>> +                */
>> +               if (i > 0 && c->table[i - 1].frequency ==
>> +                  c->table[i].frequency && prev_cc == core_count) {
>> +                       struct cpufreq_frequency_table *prev = &c->table[i - 1];
>> +
>> +                       if (prev_freq == CPUFREQ_ENTRY_INVALID)
>> +                               prev->flags = CPUFREQ_BOOST_FREQ;
>> +                       break;
>> +               }
>> +               prev_cc = core_count;
>> +               prev_freq = cur_freq;
>> +       }
>> +
>> +       c->table[i].frequency = CPUFREQ_TABLE_END;
>> +
>> +       return 0;
>> +}
>> +
>> +static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
>> +{
>> +       struct device_node *cpu_np, *freq_np;
>> +       int cpu;
>> +
>> +       for_each_possible_cpu(cpu) {
>> +               cpu_np = of_cpu_device_node_get(cpu);
>> +               if (!cpu_np)
>> +                       continue;
>> +               freq_np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
>> +               of_node_put(cpu_np);
>> +               if (!freq_np)
>> +                       continue;
>> +
>> +               if (freq_np == np)
>> +                       cpumask_set_cpu(cpu, m);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int qcom_cpu_resources_init(struct platform_device *pdev,
>> +                                  struct device_node *np, unsigned int cpu,
>> +                                  unsigned long xo_rate)
>> +{
>> +       struct cpufreq_qcom *c;
>> +       struct resource res;
>> +       struct device *dev = &pdev->dev;
>> +       const u16 *offsets;
>> +       int ret, i, cpu_first, cpu_r;
>> +       void __iomem *base;
>> +
>> +       if (qcom_freq_domain_map[cpu])
>> +               return 0;
>> +
>> +       c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
>> +       if (!c)
>> +               return -ENOMEM;
>> +
>> +       offsets = of_device_get_match_data(&pdev->dev);
>> +       if (!offsets)
>> +               return -EINVAL;
>> +
>> +       if (of_address_to_resource(np, 0, &res))
>> +               return -ENOMEM;
>> +
>> +       base = devm_ioremap_resource(dev, &res);
>> +       if (!base)
>> +               return -ENOMEM;
>> +
>> +       for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
>> +               c->reg_bases[i] = base + offsets[i];
>> +
>> +       /* HW should be in enabled state to proceed */
>> +       if (!(readl_relaxed(c->reg_bases[REG_ENABLE]) & 0x1)) {
>> +               dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
>> +               return -ENODEV;
>> +       }
>> +
>> +       ret = qcom_get_related_cpus(np, &c->related_cpus);
>> +       if (ret) {
>> +               dev_err(dev, "%s failed to get related CPUs\n", np->name);
>> +               return ret;
>> +       }
>> +
>> +       c->max_cores = cpumask_weight(&c->related_cpus);
>> +       if (!c->max_cores)
>> +               return -ENOENT;
>> +
>> +       c->xo_rate = xo_rate;
>> +
>> +       ret = qcom_cpufreq_hw_read_lut(pdev, c);
>> +       if (ret) {
>> +               dev_err(dev, "%s failed to read LUT\n", np->name);
>> +               return ret;
>> +       }
>> +
>> +       qcom_freq_domain_map[cpu] = c;
>> +
>> +       /* Related CPUs */
>> +       cpu_first = cpumask_first(&c->related_cpus);
>> +
>> +       for_each_cpu(cpu_r, &c->related_cpus) {
>> +               if (cpu_r != cpu_first)
>> +                       qcom_freq_domain_map[cpu_r] =
>> +                               qcom_freq_domain_map[cpu_first];
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int qcom_resources_init(struct platform_device *pdev)
>> +{
>> +       struct device_node *np, *cpu_np;
>> +       struct clk *clk;
>> +       unsigned int cpu;
>> +       int ret;
>> +
>> +       clk = devm_clk_get(&pdev->dev, "xo");
>> +       if (IS_ERR(clk))
>> +               return PTR_ERR(clk);
>> +
>> +       for_each_possible_cpu(cpu) {
>> +               cpu_np = of_cpu_device_node_get(cpu);
>> +               if (!cpu_np) {
>> +                       dev_dbg(&pdev->dev, "Failed to get cpu %d device\n",
>> +                               cpu);
>> +                       continue;
>> +               }
>> +
>> +               np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
>> +               of_node_put(cpu_np);
>> +               if (!np) {
>> +                       dev_err(&pdev->dev, "Failed to get freq-domain device\n");
>> +                       return -EINVAL;
>> +               }
>> +
>> +               ret = qcom_cpu_resources_init(pdev, np, cpu, clk_get_rate(clk));
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +
>> +       devm_clk_put(&pdev->dev, clk);
>> +
>> +       return 0;
>> +}
>> +
>> +static int qcom_cpufreq_hw_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_hw_driver);
>> +       if (rc) {
>> +               dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
>> +               return rc;
>> +       }
>> +
>> +       dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct of_device_id qcom_cpufreq_hw_match[] = {
>> +       { .compatible = "qcom,cpufreq-hw", .data = &cpufreq_qcom_std_offsets },
>> +       {}
>> +};
>> +
>> +static struct platform_driver qcom_cpufreq_hw_driver = {
>> +       .probe = qcom_cpufreq_hw_driver_probe,
>> +       .driver = {
>> +               .name = "qcom-cpufreq-hw",
>> +               .of_match_table = qcom_cpufreq_hw_match,
>> +       },
>> +};
>> +
>> +static int __init qcom_cpufreq_hw_init(void)
>> +{
>> +       return platform_driver_register(&qcom_cpufreq_hw_driver);
>> +}
>> +subsys_initcall(qcom_cpufreq_hw_init);
>> +
>> +MODULE_DESCRIPTION("QCOM firmware-based CPU Frequency driver");
>> --
>> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
>> of the Code Aurora Forum, hosted by the  Linux Foundation.
>>

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

--

Re: [PATCH v7 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Taniya Das]

On 9/11/2018 1:00 AM, Matthias Kaehlcke wrote:
> On Tue, Jul 24, 2018 at 04:12:50PM +0530, Taniya Das wrote:
>> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this hardware engine.
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> ---
>>   drivers/cpufreq/Kconfig.arm       |  11 ++
>>   drivers/cpufreq/Makefile          |   1 +
>>   drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 360 insertions(+)
>>   create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 0cd8eb7..93a9d72 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -298,3 +298,14 @@ config ARM_PXA2xx_CPUFREQ
>>   	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
>>
>>   	  If in doubt, say N.
>> +
>> +config ARM_QCOM_CPUFREQ_HW
>> +	bool "QCOM CPUFreq HW driver"
>> +	depends on ARCH_QCOM
>> +	help
>> +	 Support for the CPUFreq HW driver.
>> +	 Some QCOM chipsets have a HW engine to offload the steps
>> +	 necessary for changing the frequency of the CPUs. Firmware loaded
>> +	 in this engine exposes a programming interface to the OS.
>> +	 The driver implements the cpufreq interface for this HW engine.
>> +	 Say Y if you want to support CPUFreq HW.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index c1ffeab..ca48a1d 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_HW)	+= qcom-cpufreq-hw.o
>>
>>
>>   ##################################################################################
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>> new file mode 100644
>> index 0000000..ea8f7d1
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -0,0 +1,348 @@
>> +// 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 LUT_MAX_ENTRIES			40U
>> +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
>> +#define LUT_ROW_SIZE			32
>> +
>> +enum {
>> +	REG_ENABLE,
>> +	REG_LUT_TABLE,
>> +	REG_PERF_STATE,
>> +
>> +	REG_ARRAY_SIZE,
>> +};
>> +
>> +struct cpufreq_qcom {
>> +	struct cpufreq_frequency_table *table;
>> +	struct device *dev;
>> +	void __iomem *reg_bases[REG_ARRAY_SIZE];
>> +	cpumask_t related_cpus;
>> +	unsigned int max_cores;
>> +	unsigned long xo_rate;
>> +};
>> +
>> +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
>> +	[REG_ENABLE]		= 0x0,
>> +	[REG_LUT_TABLE]		= 0x110,
>> +	[REG_PERF_STATE]	= 0x920,
>> +};
>> +
>> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
>> +
>> +static int
>> +qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
>> +			     unsigned int index)
>> +{
>> +	struct cpufreq_qcom *c = policy->driver_data;
>> +
>> +	writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
>> +
>> +	return 0;
>> +}
>> +
>> +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
>> +{
>> +	struct cpufreq_qcom *c;
>> +	struct cpufreq_policy *policy;
>> +	unsigned int index;
>> +
>> +	policy = cpufreq_cpu_get_raw(cpu);
>> +	if (!policy)
>> +		return 0;
>> +
>> +	c = policy->driver_data;
>> +
>> +	index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
>> +	index = min(index, LUT_MAX_ENTRIES - 1);
>> +
>> +	return policy->freq_table[index].frequency;
>> +}
>> +
>> +static unsigned int
>> +qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
>> +			    unsigned int target_freq)
>> +{
>> +	struct cpufreq_qcom *c = policy->driver_data;
>> +	int index;
>> +
>> +	index = policy->cached_resolved_idx;
>> +	if (index < 0)
>> +		return 0;
>> +
>> +	writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
>> +
>> +	return policy->freq_table[index].frequency;
>> +}
>> +
>> +static int qcom_cpufreq_hw_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->fast_switch_possible = true;
>> +	policy->freq_table = c->table;
>> +	policy->driver_data = c;
>> +
>> +	return 0;
>> +}
>> +
>> +static struct freq_attr *qcom_cpufreq_hw_attr[] = {
>> +	&cpufreq_freq_attr_scaling_available_freqs,
>> +	&cpufreq_freq_attr_scaling_boost_freqs,
>> +	NULL
>> +};
>> +
>> +static struct cpufreq_driver cpufreq_qcom_hw_driver = {
>> +	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
>> +			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
>> +	.verify		= cpufreq_generic_frequency_table_verify,
>> +	.target_index	= qcom_cpufreq_hw_target_index,
>> +	.get		= qcom_cpufreq_hw_get,
>> +	.init		= qcom_cpufreq_hw_cpu_init,
>> +	.fast_switch    = qcom_cpufreq_hw_fast_switch,
>> +	.name		= "qcom-cpufreq-hw",
>> +	.attr		= qcom_cpufreq_hw_attr,
>> +	.boost_enabled	= true,
>> +};
>> +
>> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
>> +				    struct cpufreq_qcom *c)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	void __iomem *base;
>> +	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
>> +
>> +	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
>> +				sizeof(*c->table), GFP_KERNEL);
>> +	if (!c->table)
>> +		return -ENOMEM;
>> +
>> +	base = c->reg_bases[REG_LUT_TABLE];
>> +
>> +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> +		data = readl_relaxed(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 = c->xo_rate * lval / 1000;
>> +		else
>> +			c->table[i].frequency = INIT_RATE / 1000;
>> +
>> +		cur_freq = c->table[i].frequency;
>> +
>> +		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
>> +			i, c->table[i].frequency, core_count);
>> +
>> +		if (core_count != c->max_cores)
>> +			cur_freq = CPUFREQ_ENTRY_INVALID;
>> +
> 
> I noticed that the 'power_allocator' thermal governor currently can't
> be used with this driver since there is no OPP table with frequency and
> voltage information. Does the LUT contain information about the
> voltage or is there another mechanism to retrieve it?
> 

No, currently there is no way of reading the voltage information.

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

--

Re: [PATCH v7 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Matthias Kaehlcke]

On Sun, Sep 23, 2018 at 03:18:20PM +0530, Taniya Das wrote:
> 
> 
> On 9/11/2018 1:00 AM, Matthias Kaehlcke wrote:
> > On Tue, Jul 24, 2018 at 04:12:50PM +0530, Taniya Das wrote:
> > > The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> > > for changing the frequency of CPUs. The driver implements the cpufreq
> > > driver interface for this hardware engine.
> > > 
> > > Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> > > Signed-off-by: Taniya Das <tdas@codeaurora.org>
> > > ---
> > >   drivers/cpufreq/Kconfig.arm       |  11 ++
> > >   drivers/cpufreq/Makefile          |   1 +
> > >   drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 360 insertions(+)
> > >   create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
> > > 
> > > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > > index 0cd8eb7..93a9d72 100644
> > > --- a/drivers/cpufreq/Kconfig.arm
> > > +++ b/drivers/cpufreq/Kconfig.arm
> > > @@ -298,3 +298,14 @@ config ARM_PXA2xx_CPUFREQ
> > >   	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
> > > 
> > >   	  If in doubt, say N.
> > > +
> > > +config ARM_QCOM_CPUFREQ_HW
> > > +	bool "QCOM CPUFreq HW driver"
> > > +	depends on ARCH_QCOM
> > > +	help
> > > +	 Support for the CPUFreq HW driver.
> > > +	 Some QCOM chipsets have a HW engine to offload the steps
> > > +	 necessary for changing the frequency of the CPUs. Firmware loaded
> > > +	 in this engine exposes a programming interface to the OS.
> > > +	 The driver implements the cpufreq interface for this HW engine.
> > > +	 Say Y if you want to support CPUFreq HW.
> > > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> > > index c1ffeab..ca48a1d 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_HW)	+= qcom-cpufreq-hw.o
> > > 
> > > 
> > >   ##################################################################################
> > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > new file mode 100644
> > > index 0000000..ea8f7d1
> > > --- /dev/null
> > > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > @@ -0,0 +1,348 @@
> > > +// 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 LUT_MAX_ENTRIES			40U
> > > +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
> > > +#define LUT_ROW_SIZE			32
> > > +
> > > +enum {
> > > +	REG_ENABLE,
> > > +	REG_LUT_TABLE,
> > > +	REG_PERF_STATE,
> > > +
> > > +	REG_ARRAY_SIZE,
> > > +};
> > > +
> > > +struct cpufreq_qcom {
> > > +	struct cpufreq_frequency_table *table;
> > > +	struct device *dev;
> > > +	void __iomem *reg_bases[REG_ARRAY_SIZE];
> > > +	cpumask_t related_cpus;
> > > +	unsigned int max_cores;
> > > +	unsigned long xo_rate;
> > > +};
> > > +
> > > +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
> > > +	[REG_ENABLE]		= 0x0,
> > > +	[REG_LUT_TABLE]		= 0x110,
> > > +	[REG_PERF_STATE]	= 0x920,
> > > +};
> > > +
> > > +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> > > +
> > > +static int
> > > +qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> > > +			     unsigned int index)
> > > +{
> > > +	struct cpufreq_qcom *c = policy->driver_data;
> > > +
> > > +	writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
> > > +{
> > > +	struct cpufreq_qcom *c;
> > > +	struct cpufreq_policy *policy;
> > > +	unsigned int index;
> > > +
> > > +	policy = cpufreq_cpu_get_raw(cpu);
> > > +	if (!policy)
> > > +		return 0;
> > > +
> > > +	c = policy->driver_data;
> > > +
> > > +	index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
> > > +	index = min(index, LUT_MAX_ENTRIES - 1);
> > > +
> > > +	return policy->freq_table[index].frequency;
> > > +}
> > > +
> > > +static unsigned int
> > > +qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
> > > +			    unsigned int target_freq)
> > > +{
> > > +	struct cpufreq_qcom *c = policy->driver_data;
> > > +	int index;
> > > +
> > > +	index = policy->cached_resolved_idx;
> > > +	if (index < 0)
> > > +		return 0;
> > > +
> > > +	writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> > > +
> > > +	return policy->freq_table[index].frequency;
> > > +}
> > > +
> > > +static int qcom_cpufreq_hw_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->fast_switch_possible = true;
> > > +	policy->freq_table = c->table;
> > > +	policy->driver_data = c;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct freq_attr *qcom_cpufreq_hw_attr[] = {
> > > +	&cpufreq_freq_attr_scaling_available_freqs,
> > > +	&cpufreq_freq_attr_scaling_boost_freqs,
> > > +	NULL
> > > +};
> > > +
> > > +static struct cpufreq_driver cpufreq_qcom_hw_driver = {
> > > +	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
> > > +			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
> > > +	.verify		= cpufreq_generic_frequency_table_verify,
> > > +	.target_index	= qcom_cpufreq_hw_target_index,
> > > +	.get		= qcom_cpufreq_hw_get,
> > > +	.init		= qcom_cpufreq_hw_cpu_init,
> > > +	.fast_switch    = qcom_cpufreq_hw_fast_switch,
> > > +	.name		= "qcom-cpufreq-hw",
> > > +	.attr		= qcom_cpufreq_hw_attr,
> > > +	.boost_enabled	= true,
> > > +};
> > > +
> > > +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
> > > +				    struct cpufreq_qcom *c)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	void __iomem *base;
> > > +	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
> > > +
> > > +	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> > > +				sizeof(*c->table), GFP_KERNEL);
> > > +	if (!c->table)
> > > +		return -ENOMEM;
> > > +
> > > +	base = c->reg_bases[REG_LUT_TABLE];
> > > +
> > > +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> > > +		data = readl_relaxed(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 = c->xo_rate * lval / 1000;
> > > +		else
> > > +			c->table[i].frequency = INIT_RATE / 1000;
> > > +
> > > +		cur_freq = c->table[i].frequency;
> > > +
> > > +		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> > > +			i, c->table[i].frequency, core_count);
> > > +
> > > +		if (core_count != c->max_cores)
> > > +			cur_freq = CPUFREQ_ENTRY_INVALID;
> > > +
> > 
> > I noticed that the 'power_allocator' thermal governor currently can't
> > be used with this driver since there is no OPP table with frequency and
> > voltage information. Does the LUT contain information about the
> > voltage or is there another mechanism to retrieve it?
> > 
> 
> No, currently there is no way of reading the voltage information.

That leaves the 'power_allocator' out of question :(

Which thermal governor is/should typically be used on these systems?
Step wise and user space should work out of the box, however the
response of step wise could be slow (step by step) and user space
requires a thermal daemon (and could suffer from latencies). Fair
share could be an option if the thermal cooling devices are registered
with a 'weight', which could come from the device tree.

Re: [PATCH v7 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Matthias Kaehlcke]

On Sun, Sep 23, 2018 at 03:10:55PM +0530, Taniya Das wrote:
> Hello Matthias,
> 
> Thanks for your review comments.
> 
> On 8/29/2018 11:31 PM, Matthias Kaehlcke wrote:
> > Hi Taniya,
> > 
> > On Tue, Jul 24, 2018 at 04:12:50PM +0530, Taniya Das wrote:
> > > The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> > > for changing the frequency of CPUs. The driver implements the cpufreq
> > > driver interface for this hardware engine.
> > > 
> > > Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> > > Signed-off-by: Taniya Das <tdas@codeaurora.org>
> > > ---
> > >   drivers/cpufreq/Kconfig.arm       |  11 ++
> > >   drivers/cpufreq/Makefile          |   1 +
> > >   drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 360 insertions(+)
> > >   create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
> > > 
> > > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > > index 0cd8eb7..93a9d72 100644
> > > --- a/drivers/cpufreq/Kconfig.arm
> > > +++ b/drivers/cpufreq/Kconfig.arm
> > > @@ -298,3 +298,14 @@ config ARM_PXA2xx_CPUFREQ
> > >   	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
> > > 
> > >   	  If in doubt, say N.
> > > +
> > > +config ARM_QCOM_CPUFREQ_HW
> > > +	bool "QCOM CPUFreq HW driver"
> > > +	depends on ARCH_QCOM
> > > +	help
> > > +	 Support for the CPUFreq HW driver.
> > > +	 Some QCOM chipsets have a HW engine to offload the steps
> > > +	 necessary for changing the frequency of the CPUs. Firmware loaded
> > > +	 in this engine exposes a programming interface to the OS.
> > > +	 The driver implements the cpufreq interface for this HW engine.
> > > +	 Say Y if you want to support CPUFreq HW.
> > > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> > > index c1ffeab..ca48a1d 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_HW)	+= qcom-cpufreq-hw.o
> > > 
> > > 
> > >   ##################################################################################
> > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > new file mode 100644
> > > index 0000000..ea8f7d1
> > > --- /dev/null
> > > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > @@ -0,0 +1,348 @@
> > > +// 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 LUT_MAX_ENTRIES			40U
> > > +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
> > > +#define LUT_ROW_SIZE			32
> > > +
> > > +enum {
> > > +	REG_ENABLE,
> > > +	REG_LUT_TABLE,
> > > +	REG_PERF_STATE,
> > > +
> > > +	REG_ARRAY_SIZE,
> > > +};
> > > +
> > > +struct cpufreq_qcom {
> > > +	struct cpufreq_frequency_table *table;
> > > +	struct device *dev;
> > 
> > 'dev' is not used and can be removed.
> > 
> 
> Thanks, would remove in the next patch.
> 
> > > ...
> > > 
> > > +static int qcom_cpu_resources_init(struct platform_device *pdev,
> > > +				   struct device_node *np, unsigned int cpu,
> > > +				   unsigned long xo_rate)
> > > +{
> > > +	struct cpufreq_qcom *c;
> > > +	struct resource res;
> > > +	struct device *dev = &pdev->dev;
> > > +	const u16 *offsets;
> > > +	int ret, i, cpu_first, cpu_r;
> > > +	void __iomem *base;
> > > +
> > > +	if (qcom_freq_domain_map[cpu])
> > > +		return 0;
> > > +
> > > +	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> > > +	if (!c)
> > > +		return -ENOMEM;
> > > +
> > > +	offsets = of_device_get_match_data(&pdev->dev);
> > > +	if (!offsets)
> > > +		return -EINVAL;
> > > +
> > > +	if (of_address_to_resource(np, 0, &res))
> > > +		return -ENOMEM;
> > > +
> > > +	base = devm_ioremap_resource(dev, &res);
> > > +	if (!base)
> > > +		return -ENOMEM;
> > > +
> > > +	for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
> > > +		c->reg_bases[i] = base + offsets[i];
> > > +
> > > +	/* HW should be in enabled state to proceed */
> > > +	if (!(readl_relaxed(c->reg_bases[REG_ENABLE]) & 0x1)) {
> > > +		dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	ret = qcom_get_related_cpus(np, &c->related_cpus);
> > > +	if (ret) {
> > > +		dev_err(dev, "%s failed to get related CPUs\n", np->name);
> > > +		return ret;
> > > +	}
> > > +
> > > +	c->max_cores = cpumask_weight(&c->related_cpus);
> > > +	if (!c->max_cores)
> > > +		return -ENOENT;
> > > +
> > > +	c->xo_rate = xo_rate;
> > > +
> > > +	ret = qcom_cpufreq_hw_read_lut(pdev, c);
> > > +	if (ret) {
> > > +		dev_err(dev, "%s failed to read LUT\n", np->name);
> > > +		return ret;
> > > +	}
> > > +
> > > +	qcom_freq_domain_map[cpu] = c;
> > > +
> > > +	/* Related CPUs */
> > > +	cpu_first = cpumask_first(&c->related_cpus);
> > > +
> > > +	for_each_cpu(cpu_r, &c->related_cpus) {
> > > +		if (cpu_r != cpu_first)
> > > +			qcom_freq_domain_map[cpu_r] =
> > > +				qcom_freq_domain_map[cpu_first];
> > > +	}
> > 
> > The above ten lines could be simplified to:
> > 
> > 	for_each_cpu(cpu_r, &c->related_cpus)
> > 		qcom_freq_domain_map[cpu_r] = c;
> > 
> 
> Would clean it up in the next patch.
> 
> > > ...
> > > 
> > > +static int __init qcom_cpufreq_hw_init(void)
> > > +{
> > > +	return platform_driver_register(&qcom_cpufreq_hw_driver);
> > > +}
> > > +subsys_initcall(qcom_cpufreq_hw_init);
> > 
> > Is subsys_initcall used for a particular reason? It will cause
> > problems when registering cooling devices, since the thermal device
> > class is initialized through an fs_initcall, which are executed
> > later.
> > 
> > Most cpufreq drivers use module_init, device_initcall or
> > late_initcall, can't this driver use one of those?
> > 
> 
> We want the CPU to be scaling to the highest frequency at the
> earliest.

I guess you also want thermal management for the CPU. With the
subsys_initcall registration of cooling devices fails, as mentioned in
my earlier comment. Do you plan to defer the registration of cooling
devices?

Cheers

Matthias