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

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

[Taniya Das]

 [v9]
  * Update the Documentation binding for freq-domain-cells & cpufreq node.
  * Address comments in Kconfig.arm & Makefile.
  * Remove include file & MODULE_DESCRIPTION not required.
  * Fix the code for 'of_node_put(cpu_np)'.

 [v8]
   * Address comments to update code to take cpufreq_hw phandle and index from
     the CPU nodes.
   * Updated the Documentation for the above change in DT.
   * Updated logic for assigning 'qcom_freq_domain_map' for related CPUs.
   * Clock input to the HW block is taken from DT which has been updated in
     code and Device tree documentation.

 [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           | 173 ++++++++++
 drivers/cpufreq/Kconfig.arm                        |  11 +
 drivers/cpufreq/Makefile                           |   1 +
 drivers/cpufreq/qcom-cpufreq-hw.c                  | 354 +++++++++++++++++++++
 4 files changed, 539 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 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           | 173 +++++++++++++++++++++
 1 file changed, 173 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..712643f
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
@@ -0,0 +1,173 @@
+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 and GPLL0 clock.
+
+- clock-names
+	Usage:		required
+	Value type:	<string> From common clock binding.
+	Definition:	must be "xo", "cpu_clk".
+
+- reg
+	Usage:		required
+	Value type:	<prop-encoded-array>
+	Definition:	Addresses and sizes for the memory of the HW bases in
+			each frequency domain.
+- reg-names
+	Usage:		Optional
+	Value type:	<string>
+	Definition:	Frequency domain name i.e.
+			"freq-domain0", "freq-domain1".
+
+- freq-domain-cells:
+	Usage:		required.
+	Definition:	Number of cells in a freqency domain specifier.
+
+* Property qcom,freq-domain
+Devices supporting freq-domain must set their "qcom,freq-domain" property with
+phandle to a cpufreq_hw followed by the Domain ID(0/1) in the CPU DT node.
+
+
+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 = <&cpufreq_hw 0>;
+			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 = <&cpufreq_hw 0>;
+			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 = <&cpufreq_hw 0>;
+			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 = <&cpufreq_hw 0>;
+			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 = <&cpufreq_hw 1>;
+			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 = <&cpufreq_hw 1>;
+			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 = <&cpufreq_hw 1>;
+			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 = <&cpufreq_hw 1>;
+			L2_700: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+	};
+
+ soc {
+	cpufreq_hw: cpufreq@17d43000 {
+		compatible = "qcom,cpufreq-hw";
+		reg = <0x17d43000 0x1400>, <0x17d45800 0x1400>;
+		reg-names = "freq-domain0", "freq-domain1";
+
+		clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
+		clock-names = "xo", "cpu_clk";
+
+		#freq-domain-cells = <1>
+
+	};
+}
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

[PATCH 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 | 354 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 366 insertions(+)
 create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 0cd8eb7..c379c7f 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -121,6 +121,17 @@ config ARM_QCOM_CPUFREQ_KRYO

 	  If in doubt, say N.

+config ARM_QCOM_CPUFREQ_HW
+	bool "QCOM CPUFreq HW driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	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.
+
 config ARM_S3C_CPUFREQ
 	bool
 	help
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index c1ffeab..3882908 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ)	+= omap-cpufreq.o
 obj-$(CONFIG_ARM_PXA2xx_CPUFREQ)	+= pxa2xx-cpufreq.o
 obj-$(CONFIG_PXA3xx)			+= pxa3xx-cpufreq.o
 obj-$(CONFIG_ARM_QCOM_CPUFREQ_KRYO)	+= qcom-cpufreq-kryo.o
+obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW)	+= qcom-cpufreq-hw.o
 obj-$(CONFIG_ARM_S3C2410_CPUFREQ)	+= s3c2410-cpufreq.o
 obj-$(CONFIG_ARM_S3C2412_CPUFREQ)	+= s3c2412-cpufreq.o
 obj-$(CONFIG_ARM_S3C2416_CPUFREQ)	+= s3c2416-cpufreq.o
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
new file mode 100644
index 0000000..fe1c264
--- /dev/null
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -0,0 +1,354 @@
+// 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/of_address.h>
+#include <linux/of_platform.h>
+
+#define LUT_MAX_ENTRIES			40U
+#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
+#define LUT_ROW_SIZE			32
+#define CLK_HW_DIV			2
+
+enum {
+	REG_ENABLE,
+	REG_LUT_TABLE,
+	REG_PERF_STATE,
+
+	REG_ARRAY_SIZE,
+};
+
+struct cpufreq_qcom {
+	struct cpufreq_frequency_table *table;
+	void __iomem *reg_bases[REG_ARRAY_SIZE];
+	cpumask_t related_cpus;
+	unsigned int max_cores;
+	unsigned long xo_rate;
+	unsigned long cpu_hw_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 = c->cpu_hw_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(int index, struct cpumask *m)
+{
+	struct device_node *cpu_np;
+	struct of_phandle_args args;
+	int cpu, ret;
+
+	for_each_possible_cpu(cpu) {
+		cpu_np = of_cpu_device_node_get(cpu);
+		if (!cpu_np)
+			continue;
+
+		ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
+						 "#freq-domain-cells", 0,
+						  &args);
+		of_node_put(cpu_np);
+		if (ret < 0)
+			continue;
+
+		if (index == args.args[0])
+			cpumask_set_cpu(cpu, m);
+	}
+
+	return 0;
+}
+
+static int qcom_cpu_resources_init(struct platform_device *pdev,
+				   unsigned int cpu, int index,
+				   unsigned long xo_rate,
+				   unsigned long cpu_hw_rate)
+{
+	struct cpufreq_qcom *c;
+	struct resource *res;
+	struct device *dev = &pdev->dev;
+	const u16 *offsets;
+	int ret, i, 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;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	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, "Domain-%d cpufreq hardware not enabled\n", index);
+		return -ENODEV;
+	}
+
+	ret = qcom_get_related_cpus(index, &c->related_cpus);
+	if (ret) {
+		dev_err(dev, "Domain-%d failed to get related CPUs\n", index);
+		return ret;
+	}
+
+	c->max_cores = cpumask_weight(&c->related_cpus);
+	if (!c->max_cores)
+		return -ENOENT;
+
+	c->xo_rate = xo_rate;
+	c->cpu_hw_rate = cpu_hw_rate;
+
+	ret = qcom_cpufreq_hw_read_lut(pdev, c);
+	if (ret) {
+		dev_err(dev, "Domain-%d failed to read LUT\n", index);
+		return ret;
+	}
+
+	for_each_cpu(cpu_r, &c->related_cpus)
+		qcom_freq_domain_map[cpu_r] = c;
+
+	return 0;
+}
+
+static int qcom_resources_init(struct platform_device *pdev)
+{
+	struct device_node *cpu_np;
+	struct of_phandle_args args;
+	struct clk *clk;
+	unsigned int cpu;
+	unsigned long xo_rate, cpu_hw_rate;
+	int ret;
+
+	clk = clk_get(&pdev->dev, "xo");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	xo_rate = clk_get_rate(clk);
+
+	clk_put(clk);
+
+	clk = clk_get(&pdev->dev, "cpu_clk");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	cpu_hw_rate = clk_get_rate(clk) / CLK_HW_DIV;
+
+	clk_put(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;
+		}
+
+		ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
+						 "#freq-domain-cells", 0,
+						  &args);
+		of_node_put(cpu_np);
+		if (ret < 0)
+			return ret;
+
+		ret = qcom_cpu_resources_init(pdev, cpu, args.args[0],
+					      xo_rate, cpu_hw_rate);
+		if (ret)
+			return ret;
+	}
+
+	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);
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

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

[Rob Herring]

On Thu, 11 Oct 2018 17:06:00 +0530, Taniya Das wrote:
> 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           | 173 +++++++++++++++++++++
>  1 file changed, 173 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>

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

[Stephen Boyd]

Quoting Taniya Das (2018-10-11 04:36:00)
> 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>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

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

[Stephen Boyd]

Quoting Taniya Das (2018-10-11 04:36:01)
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -121,6 +121,17 @@ config ARM_QCOM_CPUFREQ_KRYO
> 
>           If in doubt, say N.
> 
> +config ARM_QCOM_CPUFREQ_HW
> +       bool "QCOM CPUFreq HW driver"

Is there any reason this can't be a module?

> +       depends on ARCH_QCOM || COMPILE_TEST
> +       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.
> +
>  config ARM_S3C_CPUFREQ
>         bool
>         help
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> new file mode 100644
> index 0000000..fe1c264
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -0,0 +1,354 @@
> +// 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/of_address.h>
> +#include <linux/of_platform.h>
> +
> +#define LUT_MAX_ENTRIES                        40U
> +#define CORE_COUNT_VAL(val)            (((val) & (GENMASK(18, 16))) >> 16)
> +#define LUT_ROW_SIZE                   32
> +#define CLK_HW_DIV                     2
> +
> +enum {
> +       REG_ENABLE,
> +       REG_LUT_TABLE,
> +       REG_PERF_STATE,
> +
> +       REG_ARRAY_SIZE,
> +};
> +
> +struct cpufreq_qcom {
> +       struct cpufreq_frequency_table *table;
> +       void __iomem *reg_bases[REG_ARRAY_SIZE];
> +       cpumask_t related_cpus;
> +       unsigned int max_cores;
> +       unsigned long xo_rate;
> +       unsigned long cpu_hw_rate;
> +};
> +
> +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {

Is this going to change in the future?

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

Why can't we avoid the indirection here and store the perf_state pointer
in probe? Then we don't have to indirect through a table to perform the
register write.

> +
> +       return 0;
> +}
> +
[..]
> +static int qcom_resources_init(struct platform_device *pdev)
> +{
> +       struct device_node *cpu_np;
> +       struct of_phandle_args args;
> +       struct clk *clk;
> +       unsigned int cpu;
> +       unsigned long xo_rate, cpu_hw_rate;
> +       int ret;
> +
> +       clk = clk_get(&pdev->dev, "xo");
> +       if (IS_ERR(clk))
> +               return PTR_ERR(clk);
> +
> +       xo_rate = clk_get_rate(clk);
> +
> +       clk_put(clk);
> +
> +       clk = clk_get(&pdev->dev, "cpu_clk");

Sad that the name is cpu_clk, instead of something like 'backup' or
whatever the name really is in hardware.

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

[Matthias Kaehlcke]

On Thu, Oct 11, 2018 at 05:06:00PM +0530, Taniya Das wrote:
> 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           | 173 +++++++++++++++++++++
>  1 file changed, 173 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..712643f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> @@ -0,0 +1,173 @@
> +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 and GPLL0 clock.
> +
> +- clock-names
> +	Usage:		required
> +	Value type:	<string> From common clock binding.
> +	Definition:	must be "xo", "cpu_clk".
> +
> +- reg
> +	Usage:		required
> +	Value type:	<prop-encoded-array>
> +	Definition:	Addresses and sizes for the memory of the HW bases in
> +			each frequency domain.
> +- reg-names
> +	Usage:		Optional
> +	Value type:	<string>
> +	Definition:	Frequency domain name i.e.
> +			"freq-domain0", "freq-domain1".
> +
> +- freq-domain-cells:
> +	Usage:		required.
> +	Definition:	Number of cells in a freqency domain specifier.
> +
> +* Property qcom,freq-domain
> +Devices supporting freq-domain must set their "qcom,freq-domain" property with
> +phandle to a cpufreq_hw followed by the Domain ID(0/1) in the CPU DT node.
> +
> +
> +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 = <&cpufreq_hw 0>;
> +			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 = <&cpufreq_hw 0>;
> +			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 = <&cpufreq_hw 0>;
> +			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 = <&cpufreq_hw 0>;
> +			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 = <&cpufreq_hw 1>;
> +			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 = <&cpufreq_hw 1>;
> +			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 = <&cpufreq_hw 1>;
> +			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 = <&cpufreq_hw 1>;
> +			L2_700: l2-cache {
> +				compatible = "cache";
> +				next-level-cache = <&L3_0>;
> +			};
> +		};
> +	};
> +
> + soc {
> +	cpufreq_hw: cpufreq@17d43000 {
> +		compatible = "qcom,cpufreq-hw";
> +		reg = <0x17d43000 0x1400>, <0x17d45800 0x1400>;
> +		reg-names = "freq-domain0", "freq-domain1";
> +
> +		clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
> +		clock-names = "xo", "cpu_clk";
> +
> +		#freq-domain-cells = <1>

semicolon is missing.

> +

delete empty line

> +	};
> +}

Thanks

Matthias

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

[Amit Kucheria]

Hi Taniya,

Both the patches are missing v9 in their subject line - this threw off
patchwork when trying to download the patches.

On Thu, Oct 11, 2018 at 5:06 PM Taniya Das <tdas@codeaurora.org> wrote:
>
> 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.

I tested these patches on the sdm845-mtp against 4.19 and found that
the frequency gets stuck at the highest opp (the boost frequency)
after running a couple of 'yes > /dev/null &' instances. Have you
tested these against a mainline kernel?

See cpufreq statistics below:

linaro-test [rc=0]# cat policy?/scaling_cur_freq
300000
2803200

linaro-test [rc=0]# cat policy?/stats/time_in_state
300000 100840
403200 388
480000 71
576000 54
652800 22
748800 11
825600 5
902400 5
979200 9
1056000 3
1132800 2
1228800 5
1324800 8
1420800 2
1516800 1
1612800 0
1689600 0
1766400 392
825600 22048
902400 21
979200 4
1056000 15
1209600 6
1286400 0
1363200 1
1459200 0
1536000 0
1612800 1
1689600 0
1766400 0
1843200 2
1920000 2
1996800 0
2092800 0
2169600 0
2246400 0
2323200 0
2400000 0
2476800 0
2553600 0
2649600 0
2707200 0
2764800 0
2784000 0
2803200 79718


> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  .../bindings/cpufreq/cpufreq-qcom-hw.txt           | 173 +++++++++++++++++++++
>  1 file changed, 173 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..712643f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> @@ -0,0 +1,173 @@
> +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 and GPLL0 clock.
> +
> +- clock-names
> +       Usage:          required
> +       Value type:     <string> From common clock binding.
> +       Definition:     must be "xo", "cpu_clk".
> +
> +- reg
> +       Usage:          required
> +       Value type:     <prop-encoded-array>
> +       Definition:     Addresses and sizes for the memory of the HW bases in
> +                       each frequency domain.
> +- reg-names
> +       Usage:          Optional
> +       Value type:     <string>
> +       Definition:     Frequency domain name i.e.
> +                       "freq-domain0", "freq-domain1".
> +
> +- freq-domain-cells:
> +       Usage:          required.
> +       Definition:     Number of cells in a freqency domain specifier.
> +
> +* Property qcom,freq-domain
> +Devices supporting freq-domain must set their "qcom,freq-domain" property with
> +phandle to a cpufreq_hw followed by the Domain ID(0/1) in the CPU DT node.
> +
> +
> +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 = <&cpufreq_hw 0>;
> +                       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 = <&cpufreq_hw 0>;
> +                       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 = <&cpufreq_hw 0>;
> +                       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 = <&cpufreq_hw 0>;
> +                       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 = <&cpufreq_hw 1>;
> +                       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 = <&cpufreq_hw 1>;
> +                       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 = <&cpufreq_hw 1>;
> +                       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 = <&cpufreq_hw 1>;
> +                       L2_700: l2-cache {
> +                               compatible = "cache";
> +                               next-level-cache = <&L3_0>;
> +                       };
> +               };
> +       };
> +
> + soc {
> +       cpufreq_hw: cpufreq@17d43000 {
> +               compatible = "qcom,cpufreq-hw";
> +               reg = <0x17d43000 0x1400>, <0x17d45800 0x1400>;
> +               reg-names = "freq-domain0", "freq-domain1";
> +
> +               clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
> +               clock-names = "xo", "cpu_clk";
> +
> +               #freq-domain-cells = <1>
> +
> +       };
> +}
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
> of the Code Aurora Forum, hosted by the  Linux Foundation.
>

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

[Matthias Kaehlcke]

Hi,

On Tue, Oct 23, 2018 at 05:23:34PM +0530, Amit Kucheria wrote:
> Hi Taniya,
> 
> Both the patches are missing v9 in their subject line - this threw off
> patchwork when trying to download the patches.
> 
> On Thu, Oct 11, 2018 at 5:06 PM Taniya Das <tdas@codeaurora.org> wrote:
> >
> > 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.
> 
> I tested these patches on the sdm845-mtp against 4.19 and found that
> the frequency gets stuck at the highest opp (the boost frequency)
> after running a couple of 'yes > /dev/null &' instances. Have you
> tested these against a mainline kernel?
> 
> See cpufreq statistics below:
> 
> linaro-test [rc=0]# cat policy?/scaling_cur_freq
> 300000
> 2803200
> 
> linaro-test [rc=0]# cat policy?/stats/time_in_state
> 300000 100840
> 403200 388
> 480000 71
> 576000 54
> 652800 22
> 748800 11
> 825600 5
> 902400 5
> 979200 9
> 1056000 3
> 1132800 2
> 1228800 5
> 1324800 8
> 1420800 2
> 1516800 1
> 1612800 0
> 1689600 0
> 1766400 392
> 825600 22048
> 902400 21
> 979200 4
> 1056000 15
> 1209600 6
> 1286400 0
> 1363200 1
> 1459200 0
> 1536000 0
> 1612800 1
> 1689600 0
> 1766400 0
> 1843200 2
> 1920000 2
> 1996800 0
> 2092800 0
> 2169600 0
> 2246400 0
> 2323200 0
> 2400000 0
> 2476800 0
> 2553600 0
> 2649600 0
> 2707200 0
> 2764800 0
> 2784000 0
> 2803200 79718

I can repro this on SDM845 with a v4.19 kernel.

Since the little cores don't have a boost frequency I think maxing out
can be expected with a high workload and no thermal throttling.
However the big cores have a boost frequency (2.803 MHz), so the
driver shouldn't be stuck at it. Though in practice I also wonder if
the ~1% 'boost' makes a big difference in terms of performance or CPU
overload ...

Cheers

Matthias

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

[Taniya Das]

Hello Stephen,

On 10/18/2018 5:02 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-10-11 04:36:01)
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -121,6 +121,17 @@ config ARM_QCOM_CPUFREQ_KRYO
>>
>>            If in doubt, say N.
>>
>> +config ARM_QCOM_CPUFREQ_HW
>> +       bool "QCOM CPUFreq HW driver"
> 
> Is there any reason this can't be a module?
> 

We do not have any use cases where we need to support it as module.

>> +       depends on ARCH_QCOM || COMPILE_TEST
>> +       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.
>> +
>>   config ARM_S3C_CPUFREQ
>>          bool
>>          help
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>> new file mode 100644
>> index 0000000..fe1c264
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -0,0 +1,354 @@
>> +// 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/of_address.h>
>> +#include <linux/of_platform.h>
>> +
>> +#define LUT_MAX_ENTRIES                        40U
>> +#define CORE_COUNT_VAL(val)            (((val) & (GENMASK(18, 16))) >> 16)
>> +#define LUT_ROW_SIZE                   32
>> +#define CLK_HW_DIV                     2
>> +
>> +enum {
>> +       REG_ENABLE,
>> +       REG_LUT_TABLE,
>> +       REG_PERF_STATE,
>> +
>> +       REG_ARRAY_SIZE,
>> +};
>> +
>> +struct cpufreq_qcom {
>> +       struct cpufreq_frequency_table *table;
>> +       void __iomem *reg_bases[REG_ARRAY_SIZE];
>> +       cpumask_t related_cpus;
>> +       unsigned int max_cores;
>> +       unsigned long xo_rate;
>> +       unsigned long cpu_hw_rate;
>> +};
>> +
>> +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
> 
> Is this going to change in the future?
> 

Yes, they could change and that was the reason to introduce the offsets. 
This was discussed earlier too with Sudeep and was to add them.

>> +       [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]);
> 
> Why can't we avoid the indirection here and store the perf_state pointer
> in probe? Then we don't have to indirect through a table to perform the
> register write.
> 

As the offsets could change and that was the reason to add this.

>> +
>> +       return 0;
>> +}
>> +
> [..]
>> +static int qcom_resources_init(struct platform_device *pdev)
>> +{
>> +       struct device_node *cpu_np;
>> +       struct of_phandle_args args;
>> +       struct clk *clk;
>> +       unsigned int cpu;
>> +       unsigned long xo_rate, cpu_hw_rate;
>> +       int ret;
>> +
>> +       clk = clk_get(&pdev->dev, "xo");
>> +       if (IS_ERR(clk))
>> +               return PTR_ERR(clk);
>> +
>> +       xo_rate = clk_get_rate(clk);
>> +
>> +       clk_put(clk);
>> +
>> +       clk = clk_get(&pdev->dev, "cpu_clk");
> 
> Sad that the name is cpu_clk, instead of something like 'backup' or
> whatever the name really is in hardware.
> 

Sure, would update it.

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

--

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

[Stephen Boyd]

Quoting Taniya Das (2018-11-02 20:06:00)
> Hello Stephen,
> 
> On 10/18/2018 5:02 AM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-10-11 04:36:01)
> >> --- a/drivers/cpufreq/Kconfig.arm
> >> +++ b/drivers/cpufreq/Kconfig.arm
> >> @@ -121,6 +121,17 @@ config ARM_QCOM_CPUFREQ_KRYO
> >>
> >>            If in doubt, say N.
> >>
> >> +config ARM_QCOM_CPUFREQ_HW
> >> +       bool "QCOM CPUFreq HW driver"
> > 
> > Is there any reason this can't be a module?
> > 
> 
> We do not have any use cases where we need to support it as module.

Ok, so it could easily be tristate then? Why not allow it?

> 
> >> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> >> new file mode 100644
> >> index 0000000..fe1c264
> >> --- /dev/null
> >> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> >> @@ -0,0 +1,354 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> >> + */
[...]
> >> +
> >> +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
> > 
> > Is this going to change in the future?
> > 
> 
> Yes, they could change and that was the reason to introduce the offsets. 
> This was discussed earlier too with Sudeep and was to add them.
> 
> >> +       [REG_ENABLE]            = 0x0,

This is only used once? Maybe it could be removed.

> >> +       [REG_LUT_TABLE]         = 0x110,

And this is only used during probe to figure out the supported
frequencies. So we definitely don't need to store around the registers
after probe in an array of iomem pointers. The only one that we need
after probe is the one below.

> >> +       [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]);
> > 
> > Why can't we avoid the indirection here and store the perf_state pointer
> > in probe? Then we don't have to indirect through a table to perform the
> > register write.
> > 
> 
> As the offsets could change and that was the reason to add this.

With fast switching we can avoid incurring any extra instructions, so
please make another iomem pointer in the cpufreq_qcom struct just for
writing the index or if possible, just pass the iomem pointer that
points to the REG_PERF_STATE as the policy->driver_data variable here.
Then we have the address in hand without any extra load. If my
understanding is correct, we don't need to keep around anything besides
this register address anyway so we should be able to just load it and
write it immediately.

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

[Sudeep Holla]

On Sat, Nov 03, 2018 at 08:36:00AM +0530, Taniya Das wrote:
> Hello Stephen,
>
> On 10/18/2018 5:02 AM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-10-11 04:36:01)
> > > --- a/drivers/cpufreq/Kconfig.arm
> > > +++ b/drivers/cpufreq/Kconfig.arm

[...]

> > > +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
> >
> > Is this going to change in the future?
> >
>
> Yes, they could change and that was the reason to introduce the offsets.
> This was discussed earlier too with Sudeep and was to add them.

Sorry, I didn't like these registers to be coming from DT and I had the
same question: will this keep changing ? And IIRC, the answer was yes.
But I agree with Stephen, if and when we see the change, you can
introduce the array and keep it simple until then.

--
Regards,
Sudeep

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

[Taniya Das]

Hello Stephen,

Thanks for your comments.

On 11/4/2018 9:50 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-11-02 20:06:00)
>> Hello Stephen,
>>
>> On 10/18/2018 5:02 AM, Stephen Boyd wrote:
>>> Quoting Taniya Das (2018-10-11 04:36:01)
>>>> --- a/drivers/cpufreq/Kconfig.arm
>>>> +++ b/drivers/cpufreq/Kconfig.arm
>>>> @@ -121,6 +121,17 @@ config ARM_QCOM_CPUFREQ_KRYO
>>>>
>>>>             If in doubt, say N.
>>>>
>>>> +config ARM_QCOM_CPUFREQ_HW
>>>> +       bool "QCOM CPUFreq HW driver"
>>>
>>> Is there any reason this can't be a module?
>>>
>>
>> We do not have any use cases where we need to support it as module.
> 
> Ok, so it could easily be tristate then? Why not allow it?
> 

I have checked other vendors CPUfreq drivers and those too support only 
"bool".

>>
>>>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>>>> new file mode 100644
>>>> index 0000000..fe1c264
>>>> --- /dev/null
>>>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>>>> @@ -0,0 +1,354 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>>>> + */
> [...]
>>>> +
>>>> +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
>>>
>>> Is this going to change in the future?
>>>
>>
>> Yes, they could change and that was the reason to introduce the offsets.
>> This was discussed earlier too with Sudeep and was to add them.
>>
>>>> +       [REG_ENABLE]            = 0x0,
> 
> This is only used once? Maybe it could be removed.
> 
>>>> +       [REG_LUT_TABLE]         = 0x110,
> 
> And this is only used during probe to figure out the supported
> frequencies. So we definitely don't need to store around the registers
> after probe in an array of iomem pointers. The only one that we need
> after probe is the one below.
> 
>>>> +       [REG_PERF_STATE]        = 0x920,
>>>> +};
>>>> +

As these address offsets could change, so I am of the opinion to leave 
them as it is.

>>>> +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]);
>>>
>>> Why can't we avoid the indirection here and store the perf_state pointer
>>> in probe? Then we don't have to indirect through a table to perform the
>>> register write.
>>>
>>
>> As the offsets could change and that was the reason to add this.
> 
> With fast switching we can avoid incurring any extra instructions, so
> please make another iomem pointer in the cpufreq_qcom struct just for
> writing the index or if possible, just pass the iomem pointer that
> points to the REG_PERF_STATE as the policy->driver_data variable here.
> Then we have the address in hand without any extra load. If my
> understanding is correct, we don't need to keep around anything besides
> this register address anyway so we should be able to just load it and
> write it immediately.
>

The c->reg_bases[] is just an index to the updated bases addresses. I am 
not clear as to why it would incur an extra instruction.

The below code would already take care of it.

+	for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
+		c->reg_bases[i] = base + offsets[i];
+

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

--

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

[Matthias Kaehlcke]

On Thu, Oct 25, 2018 at 03:43:39PM -0700, Matthias Kaehlcke wrote:
> Hi,
> 
> On Tue, Oct 23, 2018 at 05:23:34PM +0530, Amit Kucheria wrote:
> > Hi Taniya,
> > 
> > Both the patches are missing v9 in their subject line - this threw off
> > patchwork when trying to download the patches.
> > 
> > On Thu, Oct 11, 2018 at 5:06 PM Taniya Das <tdas@codeaurora.org> wrote:
> > >
> > > 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.
> > 
> > I tested these patches on the sdm845-mtp against 4.19 and found that
> > the frequency gets stuck at the highest opp (the boost frequency)
> > after running a couple of 'yes > /dev/null &' instances. Have you
> > tested these against a mainline kernel?
> > 
> > See cpufreq statistics below:
> > 
> > linaro-test [rc=0]# cat policy?/scaling_cur_freq
> > 300000
> > 2803200
> > 
> > linaro-test [rc=0]# cat policy?/stats/time_in_state
> > 300000 100840
> > 403200 388
> > 480000 71
> > 576000 54
> > 652800 22
> > 748800 11
> > 825600 5
> > 902400 5
> > 979200 9
> > 1056000 3
> > 1132800 2
> > 1228800 5
> > 1324800 8
> > 1420800 2
> > 1516800 1
> > 1612800 0
> > 1689600 0
> > 1766400 392
> > 825600 22048
> > 902400 21
> > 979200 4
> > 1056000 15
> > 1209600 6
> > 1286400 0
> > 1363200 1
> > 1459200 0
> > 1536000 0
> > 1612800 1
> > 1689600 0
> > 1766400 0
> > 1843200 2
> > 1920000 2
> > 1996800 0
> > 2092800 0
> > 2169600 0
> > 2246400 0
> > 2323200 0
> > 2400000 0
> > 2476800 0
> > 2553600 0
> > 2649600 0
> > 2707200 0
> > 2764800 0
> > 2784000 0
> > 2803200 79718
> 
> I can repro this on SDM845 with a v4.19 kernel.
> 
> Since the little cores don't have a boost frequency I think maxing out
> can be expected with a high workload and no thermal throttling.
> However the big cores have a boost frequency (2.803 MHz), so the
> driver shouldn't be stuck at it. Though in practice I also wonder if
> the ~1% 'boost' makes a big difference in terms of performance or CPU
> overload ...

From Documentation/cpu-freq/boost.txt:

"Some CPUs support a functionality to raise the operating frequency of
some cores in a multi-core package if certain conditions apply, mostly
if the whole chip is not fully utilized and below it's intended thermal
budget."

According to this it is not per se an issue that the cores are
operating at the boost frequency for a prolonged time. The use of the
highest frequency can be expected with a certain system load and
the/a boost frequency may be used unless a thermal or other conditions
prevents it.

I think the real question is: why is the last frequency automatically
considered a boost frequency? On (my) SDM845 it is only about 1%
higher than the previous one (2.803 GHz vs 2.784 GHz), that hardly
seems like a 'boost'.

Thanks

Matthias

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

[Matthias Kaehlcke]

On Sun, Nov 11, 2018 at 06:12:29PM +0530, Taniya Das wrote:
> Hello Stephen,
> 
> Thanks for your comments.
> 
> On 11/4/2018 9:50 AM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-11-02 20:06:00)
> > > Hello Stephen,
> > > 
> > > On 10/18/2018 5:02 AM, Stephen Boyd wrote:
> > > > Quoting Taniya Das (2018-10-11 04:36:01)
> > > > > --- a/drivers/cpufreq/Kconfig.arm
> > > > > +++ b/drivers/cpufreq/Kconfig.arm
> > > > > @@ -121,6 +121,17 @@ config ARM_QCOM_CPUFREQ_KRYO
> > > > > 
> > > > >             If in doubt, say N.
> > > > > 
> > > > > +config ARM_QCOM_CPUFREQ_HW
> > > > > +       bool "QCOM CPUFreq HW driver"
> > > > 
> > > > Is there any reason this can't be a module?
> > > > 
> > > 
> > > We do not have any use cases where we need to support it as module.
> > 
> > Ok, so it could easily be tristate then? Why not allow it?
> > 
> 
> I have checked other vendors CPUfreq drivers and those too support only
> "bool".

That's not entirely correct. Most drivers in Kconfig are 'tristate'
and about 50% of those in KConfig.arm are. I'd say make it 'tristate'
unless there are good reasons not to do so.

> > > > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > > > new file mode 100644
> > > > > index 0000000..fe1c264
> > > > > --- /dev/null
> > > > > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > > > @@ -0,0 +1,354 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> > > > > + */
> > [...]
> > > > > +
> > > > > +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
> > > > 
> > > > Is this going to change in the future?
> > > > 
> > > 
> > > Yes, they could change and that was the reason to introduce the offsets.
> > > This was discussed earlier too with Sudeep and was to add them.
> > > 
> > > > > +       [REG_ENABLE]            = 0x0,
> > 
> > This is only used once? Maybe it could be removed.
> > 
> > > > > +       [REG_LUT_TABLE]         = 0x110,
> > 
> > And this is only used during probe to figure out the supported
> > frequencies. So we definitely don't need to store around the registers
> > after probe in an array of iomem pointers. The only one that we need
> > after probe is the one below.
> > 
> > > > > +       [REG_PERF_STATE]        = 0x920,
> > > > > +};
> > > > > +
> 
> As these address offsets could change, so I am of the opinion to leave them
> as it is.

As of now there is only one set of offsets. Let's just keep the code
simple while this is the case and address different offsets when it is
actually needed, as suggested by Stephen and Sudeep.

> > > > > +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]);
> > > > 
> > > > Why can't we avoid the indirection here and store the perf_state pointer
> > > > in probe? Then we don't have to indirect through a table to perform the
> > > > register write.
> > > > 
> > > 
> > > As the offsets could change and that was the reason to add this.
> > 
> > With fast switching we can avoid incurring any extra instructions, so
> > please make another iomem pointer in the cpufreq_qcom struct just for
> > writing the index or if possible, just pass the iomem pointer that
> > points to the REG_PERF_STATE as the policy->driver_data variable here.
> > Then we have the address in hand without any extra load. If my
> > understanding is correct, we don't need to keep around anything besides
> > this register address anyway so we should be able to just load it and
> > write it immediately.
> > 
> 
> The c->reg_bases[] is just an index to the updated bases addresses. I am not
> clear as to why it would incur an extra instruction.
> 
> The below code would already take care of it.
> 
> +	for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
> +		c->reg_bases[i] = base + offsets[i];
> +

From a performance point of view using a direct iomem pointer
seems like a micro-optimization that probably doesn't have a
measurable impact. However I think the code shouldn't be more complex
than necessary, and at this point the indirection isn't needed.

Cheers

Matthias

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

[Stephen Boyd]

Quoting Matthias Kaehlcke (2018-11-15 16:23:37)
> On Sun, Nov 11, 2018 at 06:12:29PM +0530, Taniya Das wrote:
> > On 11/4/2018 9:50 AM, Stephen Boyd wrote:
> > > Quoting Taniya Das (2018-11-02 20:06:00)
> > > > On 10/18/2018 5:02 AM, Stephen Boyd wrote:
> > > > > Quoting Taniya Das (2018-10-11 04:36:01)
> > > > > > --- a/drivers/cpufreq/Kconfig.arm
> > > > > > +++ b/drivers/cpufreq/Kconfig.arm
> > > > > > @@ -121,6 +121,17 @@ config ARM_QCOM_CPUFREQ_KRYO
> > > > > > 
> > > > > >             If in doubt, say N.
> > > > > > 
> > > > > > +config ARM_QCOM_CPUFREQ_HW
> > > > > > +       bool "QCOM CPUFreq HW driver"
> > > > > 
> > > > > Is there any reason this can't be a module?
> > > > > 
> > > > 
> > > > We do not have any use cases where we need to support it as module.
> > > 
> > > Ok, so it could easily be tristate then? Why not allow it?
> > > 
> > 
> > I have checked other vendors CPUfreq drivers and those too support only
> > "bool".
> 
> That's not entirely correct. Most drivers in Kconfig are 'tristate'
> and about 50% of those in KConfig.arm are. I'd say make it 'tristate'
> unless there are good reasons not to do so.

Yes, please make tristate.

> 
> > > > > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > > > > new file mode 100644
> > > > > > index 0000000..fe1c264
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > > > > @@ -0,0 +1,354 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > +/*
> > > > > > + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> > > > > > + */
> > > [...]
> > > > > > +
> > > > > > +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
> > > > > 
> > > > > Is this going to change in the future?
> > > > > 
> > > > 
> > > > Yes, they could change and that was the reason to introduce the offsets.
> > > > This was discussed earlier too with Sudeep and was to add them.
> > > > 
> > > > > > +       [REG_ENABLE]            = 0x0,
> > > 
> > > This is only used once? Maybe it could be removed.
> > > 
> > > > > > +       [REG_LUT_TABLE]         = 0x110,
> > > 
> > > And this is only used during probe to figure out the supported
> > > frequencies. So we definitely don't need to store around the registers
> > > after probe in an array of iomem pointers. The only one that we need
> > > after probe is the one below.
> > > 
> > > > > > +       [REG_PERF_STATE]        = 0x920,
> > > > > > +};
> > > > > > +
> > 
> > As these address offsets could change, so I am of the opinion to leave them
> > as it is.
> 
> As of now there is only one set of offsets. Let's just keep the code
> simple while this is the case and address different offsets when it is
> actually needed, as suggested by Stephen and Sudeep.

Yes, please simplify by getting rid of this and not storing anything in
the struct that's only used during probe.

> 
> > > 
> > > With fast switching we can avoid incurring any extra instructions, so
> > > please make another iomem pointer in the cpufreq_qcom struct just for
> > > writing the index or if possible, just pass the iomem pointer that
> > > points to the REG_PERF_STATE as the policy->driver_data variable here.
> > > Then we have the address in hand without any extra load. If my
> > > understanding is correct, we don't need to keep around anything besides
> > > this register address anyway so we should be able to just load it and
> > > write it immediately.
> > > 
> > 
> > The c->reg_bases[] is just an index to the updated bases addresses. I am not
> > clear as to why it would incur an extra instruction.
> > 
> > The below code would already take care of it.
> > 
> > +     for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
> > +             c->reg_bases[i] = base + offsets[i];
> > +
> 
> From a performance point of view using a direct iomem pointer
> seems like a micro-optimization that probably doesn't have a
> measurable impact. However I think the code shouldn't be more complex
> than necessary, and at this point the indirection isn't needed.
> 

Yes it's a micro-optimization for sure, in the task switching path so it
may actually be useful. Either way, I think we can greatly simplify by
just having the iomem pointer be the only pointer that is stored in the
policy driver_data.