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

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

[Taniya Das]

 [v10]
  * Update Documentation binding for cpufreq node.
  * Make the driver 'tristate' instead of 'bool' and update code.
  * Update the clock name to reflect the hardware name.
  * Remove support for varying offset.

 [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           | 172 ++++++++++
 drivers/cpufreq/Kconfig.arm                        |  11 +
 drivers/cpufreq/Makefile                           |   1 +
 drivers/cpufreq/qcom-cpufreq-hw.c                  | 346 +++++++++++++++++++++
 4 files changed, 530 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 v10 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..90e396b
--- /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 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", "gcc_cpuss_gpll0_clk_src";
+
+		#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 v10 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 | 346 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 358 insertions(+)
 create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 4e1131e..688f102 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -114,6 +114,17 @@ config ARM_QCOM_CPUFREQ_KRYO

 	  If in doubt, say N.

+config ARM_QCOM_CPUFREQ_HW
+	tristate "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 d5ee456..789b2e0 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -62,6 +62,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..6390e85
--- /dev/null
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -0,0 +1,346 @@
+// 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 LUT_MAX_ENTRIES			40U
+#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
+#define LUT_ROW_SIZE			32
+#define CLK_HW_DIV			2
+
+/* Register offsets */
+#define REG_ENABLE			0x0
+#define REG_LUT_TABLE			0x110
+#define REG_PERF_STATE			0x920
+
+struct cpufreq_qcom {
+	struct cpufreq_frequency_table *table;
+	void __iomem *perf_base;
+	cpumask_t related_cpus;
+	unsigned int max_cores;
+	unsigned long xo_rate;
+	unsigned long cpu_hw_rate;
+};
+
+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->perf_base);
+
+	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->perf_base);
+	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->perf_base);
+
+	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, void __iomem *base)
+{
+	struct device *dev = &pdev->dev;
+	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;
+
+	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
+		data = readl_relaxed(base + REG_LUT_TABLE + 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;
+	void __iomem *base;
+	int ret, cpu_r;
+
+	if (qcom_freq_domain_map[cpu])
+		return 0;
+
+	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+	if (!c)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	/* HW should be in enabled state to proceed */
+	if (!(readl_relaxed(base + 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;
+	c->perf_base = base + REG_PERF_STATE;
+
+	ret = qcom_cpufreq_hw_read_lut(pdev, c, base);
+	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, "gcc_cpuss_gpll0_clk_src");
+	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" },
+	{}
+};
+
+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);
+
+static void __exit qcom_cpufreq_hw_exit(void)
+{
+	cpufreq_unregister_driver(&cpufreq_qcom_hw_driver);
+	platform_driver_unregister(&qcom_cpufreq_hw_driver);
+}
+module_exit(qcom_cpufreq_hw_exit);
+
+MODULE_DESCRIPTION("QTI CPUFREQ HW Driver");
+MODULE_LICENSE("GPL v2");
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

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

[Matthias Kaehlcke]

On Wed, Nov 21, 2018 at 04:12:46PM +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           | 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..90e396b
> --- /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 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", "gcc_cpuss_gpll0_clk_src";

I don't have/find the document with the clock configuration of this
IP block, but the 'clock-names' sound more like aliases for the actual
clocks specified in the 'clocks' property (especially
'gcc_cpuss_gpll0_clk_src') than input signals from the IP POV, which
I'd expect to have more generic names.

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

[Stephen Boyd]

Quoting Taniya Das (2018-11-21 02:42:47)
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index d5ee456..789b2e0 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -62,6 +62,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

This isn't alphabetically sorted.

>  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..6390e85
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -0,0 +1,346 @@
> +// 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 LUT_MAX_ENTRIES                        40U
> +#define CORE_COUNT_VAL(val)            (((val) & (GENMASK(18, 16))) >> 16)
> +#define LUT_ROW_SIZE                   32
> +#define CLK_HW_DIV                     2
> +
> +/* Register offsets */
> +#define REG_ENABLE                     0x0
> +#define REG_LUT_TABLE                  0x110
> +#define REG_PERF_STATE                 0x920
> +
> +struct cpufreq_qcom {
> +       struct cpufreq_frequency_table *table;
> +       void __iomem *perf_base;
> +       cpumask_t related_cpus;
> +       unsigned int max_cores;
> +       unsigned long xo_rate;
> +       unsigned long cpu_hw_rate;
> +};
> +
> +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->perf_base);
> +

This isn't using the pointer directly though.

> +       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->perf_base);
> +       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->perf_base);
> +
> +       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, void __iomem *base)
> +{
> +       struct device *dev = &pdev->dev;
> +       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;
> +
> +       for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> +               data = readl_relaxed(base + REG_LUT_TABLE + i * LUT_ROW_SIZE);
> +               src = (data & GENMASK(31, 30)) >> 30;
> +               lval = data & GENMASK(7, 0);
> +               core_count = CORE_COUNT_VAL(data);

CORE_COUNT_VAL is used in one place. Just inline it like the src and
lval ones? Or better yet, use FIELD_GET helpers.

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

Write this as:

		if (src)
			cur_freq = c->xo_rate * lval / 1000;
		else
			cur_freq = c->cpu_hw_rate / 1000;
		
		c->table[i].frequency = cur_freq;

for shorter lines.

> +
> +               dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> +                       i, c->table[i].frequency, core_count);

Use cur_freq instead of the long route?

> +
> +               if (core_count != c->max_cores)
> +                       cur_freq = CPUFREQ_ENTRY_INVALID;

Don't we want to knock out the intermediate frequencies that also don't
have the max_cores core_count? If so, I would expect there to be
CPUFREQ_ENTRY_INVALID in the frequency table, but this isn't doing that.

> +
> +               /*
> +                * 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;
> +               }

This is really hard to read.

> +               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;
> +       void __iomem *base;
> +       int ret, cpu_r;
> +
> +       if (qcom_freq_domain_map[cpu])
> +               return 0;
> +
> +       c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> +       if (!c)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> +       base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(base))
> +               return PTR_ERR(base);
> +
> +       /* HW should be in enabled state to proceed */
> +       if (!(readl_relaxed(base + 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;

Why do we need to pass around these local variables in the structure?

> +       c->perf_base = base + REG_PERF_STATE;
> +
> +       ret = qcom_cpufreq_hw_read_lut(pdev, c, base);
> +       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, "gcc_cpuss_gpll0_clk_src");

I didn't see this in the binding. And it doesn't make sense still
because now it's a global clk name. It can't be called "safe" or
"backup" or something like that?

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

Not sure why this is split out of the probe function.

> +
> +       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" },
> +       {}
> +};

Missing MODULE_DEVICE_TABLE here.

> +
> +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);
> +
> +static void __exit qcom_cpufreq_hw_exit(void)
> +{
> +       cpufreq_unregister_driver(&cpufreq_qcom_hw_driver);

Why isn't this done in driver remove?

> +       platform_driver_unregister(&qcom_cpufreq_hw_driver);
> +}
> +module_exit(qcom_cpufreq_hw_exit);
> +
> +MODULE_DESCRIPTION("QTI CPUFREQ HW Driver");
> +MODULE_LICENSE("GPL v2");

Here's a patch I tested out to do all this. Simple testing shows it
works. If you need my sign-off to fold it in here you go.

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

 drivers/cpufreq/Makefile          |   2 +-
 drivers/cpufreq/qcom-cpufreq-hw.c | 147 ++++++++++++++++++--------------------
 2 files changed, 69 insertions(+), 80 deletions(-)

diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 789b2e06f3ba..08c071be2491 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -61,8 +61,8 @@ obj-$(CONFIG_MACH_MVEBU_V7)		+= mvebu-cpufreq.o
 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_QCOM_CPUFREQ_KRYO)	+= qcom-cpufreq-kryo.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
index 6390e85df1b3..105c9fe69a64 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2018, The Linux Foundation. All rights reserved.
  */
 
+#include <linux/bitfield.h>
 #include <linux/cpufreq.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -11,7 +12,9 @@
 #include <linux/of_platform.h>
 
 #define LUT_MAX_ENTRIES			40U
-#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
+#define LUT_SRC				GENMASK(31, 30)
+#define LUT_L_VAL			GENMASK(7, 0)
+#define LUT_CORE_COUNT			GENMASK(18, 16)
 #define LUT_ROW_SIZE			32
 #define CLK_HW_DIV			2
 
@@ -24,27 +27,23 @@ struct cpufreq_qcom {
 	struct cpufreq_frequency_table *table;
 	void __iomem *perf_base;
 	cpumask_t related_cpus;
-	unsigned int max_cores;
-	unsigned long xo_rate;
-	unsigned long cpu_hw_rate;
 };
 
 static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
 
-static int
-qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
-			     unsigned int index)
+static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
+					unsigned int index)
 {
-	struct cpufreq_qcom *c = policy->driver_data;
+	void __iomem *perf_base = policy->driver_data;
 
-	writel_relaxed(index, c->perf_base);
+	writel_relaxed(index, perf_base);
 
 	return 0;
 }
 
 static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
 {
-	struct cpufreq_qcom *c;
+	void __iomem *perf_base;
 	struct cpufreq_policy *policy;
 	unsigned int index;
 
@@ -52,26 +51,25 @@ static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
 	if (!policy)
 		return 0;
 
-	c = policy->driver_data;
+	perf_base = policy->driver_data;
 
-	index = readl_relaxed(c->perf_base);
+	index = readl_relaxed(perf_base);
 	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)
+static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
+						unsigned int target_freq)
 {
-	struct cpufreq_qcom *c = policy->driver_data;
+	void __iomem *perf_base = policy->driver_data;
 	int index;
 
 	index = policy->cached_resolved_idx;
 	if (index < 0)
 		return 0;
 
-	writel_relaxed(index, c->perf_base);
+	writel_relaxed(index, perf_base);
 
 	return policy->freq_table[index].frequency;
 }
@@ -90,7 +88,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 
 	policy->fast_switch_possible = true;
 	policy->freq_table = c->table;
-	policy->driver_data = c;
+	policy->driver_data = c->perf_base;
 
 	return 0;
 }
@@ -114,11 +112,12 @@ static struct cpufreq_driver cpufreq_qcom_hw_driver = {
 	.boost_enabled	= true,
 };
 
-static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
-				    struct cpufreq_qcom *c, void __iomem *base)
+static int qcom_cpufreq_hw_read_lut(struct device *dev, struct cpufreq_qcom *c,
+				    void __iomem *base, unsigned long xo_rate,
+				    unsigned long cpu_hw_rate)
 {
-	struct device *dev = &pdev->dev;
-	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
+	u32 data, src, lval, i, core_count, prev_cc, prev_freq, freq;
+	unsigned int max_cores = cpumask_weight(&c->related_cpus);
 
 	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
 				sizeof(*c->table), GFP_KERNEL);
@@ -127,37 +126,45 @@ static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
 
 	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
 		data = readl_relaxed(base + REG_LUT_TABLE + i * LUT_ROW_SIZE);
-		src = (data & GENMASK(31, 30)) >> 30;
-		lval = data & GENMASK(7, 0);
-		core_count = CORE_COUNT_VAL(data);
+		src = FIELD_GET(LUT_SRC, data);
+		lval = FIELD_GET(LUT_L_VAL, data);
+		core_count = FIELD_GET(LUT_CORE_COUNT, data);
 
 		if (src)
-			c->table[i].frequency = c->xo_rate * lval / 1000;
+			freq = 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;
+			freq = cpu_hw_rate / 1000;
+
+		/* Ignore boosts in the middle of the table */
+		if (core_count != max_cores) {
+			c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
+		} else {
+			c->table[i].frequency = freq;
+			dev_dbg(dev, "index=%d freq=%d, core_count %d\n", i,
+				freq, core_count);
+		}
 
 		/*
 		 * Two of the same frequencies with the same core counts means
-		 * end of table.
+		 * end of table
 		 */
-		if (i > 0 && c->table[i - 1].frequency ==
-		   c->table[i].frequency && prev_cc == core_count) {
+		if (i > 0 && prev_freq == freq && prev_cc == core_count) {
 			struct cpufreq_frequency_table *prev = &c->table[i - 1];
 
-			if (prev_freq == CPUFREQ_ENTRY_INVALID)
+			/*
+			 * Only treat the last frequency that might be a boost
+			 * as the boost frequency
+			 */
+			if (prev_cc != max_cores) {
+				prev->frequency = prev_freq;
 				prev->flags = CPUFREQ_BOOST_FREQ;
+			}
+
 			break;
 		}
+
 		prev_cc = core_count;
-		prev_freq = cur_freq;
+		prev_freq = freq;
 	}
 
 	c->table[i].frequency = CPUFREQ_TABLE_END;
@@ -165,7 +172,7 @@ static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
 	return 0;
 }
 
-static int qcom_get_related_cpus(int index, struct cpumask *m)
+static void qcom_get_related_cpus(int index, struct cpumask *m)
 {
 	struct device_node *cpu_np;
 	struct of_phandle_args args;
@@ -186,8 +193,6 @@ static int qcom_get_related_cpus(int index, struct cpumask *m)
 		if (index == args.args[0])
 			cpumask_set_cpu(cpu, m);
 	}
-
-	return 0;
 }
 
 static int qcom_cpu_resources_init(struct platform_device *pdev,
@@ -201,9 +206,6 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
 	void __iomem *base;
 	int ret, cpu_r;
 
-	if (qcom_freq_domain_map[cpu])
-		return 0;
-
 	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
 	if (!c)
 		return -ENOMEM;
@@ -219,21 +221,15 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
 		return -ENODEV;
 	}
 
-	ret = qcom_get_related_cpus(index, &c->related_cpus);
-	if (ret) {
+	qcom_get_related_cpus(index, &c->related_cpus);
+	if (!cpumask_weight(&c->related_cpus)) {
 		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;
 	c->perf_base = base + REG_PERF_STATE;
 
-	ret = qcom_cpufreq_hw_read_lut(pdev, c, base);
+	ret = qcom_cpufreq_hw_read_lut(dev, c, base, xo_rate, cpu_hw_rate);
 	if (ret) {
 		dev_err(dev, "Domain-%d failed to read LUT\n", index);
 		return ret;
@@ -245,7 +241,7 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
 	return 0;
 }
 
-static int qcom_resources_init(struct platform_device *pdev)
+static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 {
 	struct device_node *cpu_np;
 	struct of_phandle_args args;
@@ -259,15 +255,13 @@ static int qcom_resources_init(struct platform_device *pdev)
 		return PTR_ERR(clk);
 
 	xo_rate = clk_get_rate(clk);
-
 	clk_put(clk);
 
-	clk = clk_get(&pdev->dev, "gcc_cpuss_gpll0_clk_src");
+	clk = clk_get(&pdev->dev, "alternate");
 	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) {
@@ -282,33 +276,22 @@ static int qcom_resources_init(struct platform_device *pdev)
 						 "#freq-domain-cells", 0,
 						  &args);
 		of_node_put(cpu_np);
-		if (ret < 0)
+		if (ret)
 			return ret;
 
+		if (qcom_freq_domain_map[cpu])
+			continue;
+
 		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) {
+	ret = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
+	if (ret) {
 		dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
-		return rc;
+		return ret;
 	}
 
 	dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
@@ -316,13 +299,20 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static int qcom_cpufreq_hw_driver_remove(struct platform_device *pdev)
+{
+	return cpufreq_unregister_driver(&cpufreq_qcom_hw_driver);
+}
+
 static const struct of_device_id qcom_cpufreq_hw_match[] = {
 	{ .compatible = "qcom,cpufreq-hw" },
 	{}
 };
+MODULE_DEVICE_TABLE(of, qcom_cpufreq_hw_match);
 
 static struct platform_driver qcom_cpufreq_hw_driver = {
 	.probe = qcom_cpufreq_hw_driver_probe,
+	.remove = qcom_cpufreq_hw_driver_remove,
 	.driver = {
 		.name = "qcom-cpufreq-hw",
 		.of_match_table = qcom_cpufreq_hw_match,
@@ -337,7 +327,6 @@ subsys_initcall(qcom_cpufreq_hw_init);
 
 static void __exit qcom_cpufreq_hw_exit(void)
 {
-	cpufreq_unregister_driver(&cpufreq_qcom_hw_driver);
 	platform_driver_unregister(&qcom_cpufreq_hw_driver);
 }
 module_exit(qcom_cpufreq_hw_exit);

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

[Matthias Kaehlcke]

Hi Taniya,

thanks for respinning, a few nits inline.

On Wed, Nov 21, 2018 at 04:12:47PM +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 | 346 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 358 insertions(+)
>  create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 4e1131e..688f102 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -114,6 +114,17 @@ config ARM_QCOM_CPUFREQ_KRYO
> 
>  	  If in doubt, say N.
> 
> +config ARM_QCOM_CPUFREQ_HW
> +	tristate "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 d5ee456..789b2e0 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -62,6 +62,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..6390e85
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -0,0 +1,346 @@
> +// 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 LUT_MAX_ENTRIES			40U
> +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
> +#define LUT_ROW_SIZE			32
> +#define CLK_HW_DIV			2
> +
> +/* Register offsets */
> +#define REG_ENABLE			0x0
> +#define REG_LUT_TABLE			0x110
> +#define REG_PERF_STATE			0x920
> +
> +struct cpufreq_qcom {
> +	struct cpufreq_frequency_table *table;
> +	void __iomem *perf_base;

nit: is this really a base address? It's the address of the perf state
register, right? Better name it 'perf_state_reg'/'reg_perf_state' or
similar (just 'perf_state' might be confusing, I'd expect a variable
with this name to hold a state, not an address).

> +	cpumask_t related_cpus;
> +	unsigned int max_cores;
> +	unsigned long xo_rate;
> +	unsigned long cpu_hw_rate;
> +};
> +
> +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->perf_base);
> +
> +	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->perf_base);
> +	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->perf_base);
> +
> +	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, void __iomem *base)
> +{
> +	struct device *dev = &pdev->dev;
> +	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;
> +
> +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> +		data = readl_relaxed(base + REG_LUT_TABLE + 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;
> +	void __iomem *base;
> +	int ret, cpu_r;
> +
> +	if (qcom_freq_domain_map[cpu])
> +		return 0;
> +
> +	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> +	if (!c)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	/* HW should be in enabled state to proceed */
> +	if (!(readl_relaxed(base + 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;
> +	c->perf_base = base + REG_PERF_STATE;
> +
> +	ret = qcom_cpufreq_hw_read_lut(pdev, c, base);
> +	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, "gcc_cpuss_gpll0_clk_src");

As commented on the binding patch, I'm not sure if this is the correct
name for this clock input from the POV of this IP block. Just a doubt
at this point, I don't have/find the hardware documentation to suggest
something better.

> +	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" },
> +	{}
> +};
> +
> +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);

I'm still not convinced that a subsys_initcall is needed (instead of
module_init or device_initcall), as mentioned in the review of v7
it causes problems when registering CPU cooling devices, but we can
also fix this when support for cooling devices is added ;-)

> +static void __exit qcom_cpufreq_hw_exit(void)
> +{
> +	cpufreq_unregister_driver(&cpufreq_qcom_hw_driver);
> +	platform_driver_unregister(&qcom_cpufreq_hw_driver);
> +}
> +module_exit(qcom_cpufreq_hw_exit);
> +
> +MODULE_DESCRIPTION("QTI CPUFREQ HW Driver");

nit: make it 'QCOM CPUFreq HW driver' for consistency?

Cheers

Matthias

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

[Matthias Kaehlcke]

On Wed, Nov 21, 2018 at 04:12:47PM +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 | 346 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 358 insertions(+)
>  create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 4e1131e..688f102 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -114,6 +114,17 @@ config ARM_QCOM_CPUFREQ_KRYO
> 
>  	  If in doubt, say N.
> 
> +config ARM_QCOM_CPUFREQ_HW
> +	tristate "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 d5ee456..789b2e0 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -62,6 +62,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..6390e85
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -0,0 +1,346 @@
> +// 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 LUT_MAX_ENTRIES			40U
> +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
> +#define LUT_ROW_SIZE			32
> +#define CLK_HW_DIV			2
> +
> +/* Register offsets */
> +#define REG_ENABLE			0x0
> +#define REG_LUT_TABLE			0x110
> +#define REG_PERF_STATE			0x920
> +
> +struct cpufreq_qcom {
> +	struct cpufreq_frequency_table *table;
> +	void __iomem *perf_base;
> +	cpumask_t related_cpus;
> +	unsigned int max_cores;
> +	unsigned long xo_rate;
> +	unsigned long cpu_hw_rate;
> +};
> +
> +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->perf_base);
> +
> +	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->perf_base);
> +	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->perf_base);
> +
> +	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,

I have no real expertise with cpufreq boost, but after reading a bit
through cpufreq code this seems wrong. Boost is enabled statically,
however the driver has neither a ->set_boost function nor does it call
cpufreq_enable_boost_support() which would use a default
implementation for ->set_boost. As a result boost support is
effectively disabled:

static bool cpufreq_boost_supported(void)
{
        return likely(cpufreq_driver) && cpufreq_driver->set_boost;
}

The driver should probably do the same as cpufreq-dt.c and call
cpufreq_enable_boost_support() if boost frequencies are available,
instead of 'enabling' boost statically.

Thanks

Matthias

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

[Viresh Kumar]

On 21-11-18, 14:06, Matthias Kaehlcke wrote:
> On Wed, Nov 21, 2018 at 04:12:47PM +0530, Taniya Das wrote:
> > +	.boost_enabled	= true,
> 
> I have no real expertise with cpufreq boost, but after reading a bit
> through cpufreq code this seems wrong. Boost is enabled statically,
> however the driver has neither a ->set_boost function nor does it call
> cpufreq_enable_boost_support() which would use a default
> implementation for ->set_boost. As a result boost support is
> effectively disabled:
> 
> static bool cpufreq_boost_supported(void)
> {
>         return likely(cpufreq_driver) && cpufreq_driver->set_boost;
> }
> 
> The driver should probably do the same as cpufreq-dt.c and call
> cpufreq_enable_boost_support() if boost frequencies are available,
> instead of 'enabling' boost statically.

Feels like I have written the boost support in cpufreq core few decades back as
I don't remember any of it :)

But reading through the code this is what I understood.

There are two parts of boosting.

- Sysfs file available or not to enable/disable boost frequencies on the go.
  This file gets created only when cpufreq_enable_boost_support() gets called.

- Will cpufreq core consider boost frequencies or not while checking target
  frequency again, this is governed by cpufreq_driver->boost field, which can be
  set from driver or using the sysfs file mentioned above.

In this driver, all we have done is to set the cpufreq_driver->boost field to
true, which would allow cpufreq core to use boost frequencies as well. But that
isn't any better than making them all normal frequencies and getting rid of
boost stuff. The boosting stuff will be useful only if you want to disable some
of them at runtime, based on heating, etc. And that is possible only after you
create a sysfs file.

-- 
viresh

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

[Rob Herring]

On Wed, Nov 21, 2018 at 10:02:36AM -0800, Matthias Kaehlcke wrote:
> On Wed, Nov 21, 2018 at 04:12:46PM +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           | 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..90e396b
> > --- /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 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", "gcc_cpuss_gpll0_clk_src";
> 
> I don't have/find the document with the clock configuration of this
> IP block, but the 'clock-names' sound more like aliases for the actual
> clocks specified in the 'clocks' property (especially
> 'gcc_cpuss_gpll0_clk_src') than input signals from the IP POV, which
> I'd expect to have more generic names.

Also, this doesn't match the documented value above.

Rob

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

[Matthias Kaehlcke]

On Thu, Nov 22, 2018 at 10:37:08AM +0530, Viresh Kumar wrote:
> On 21-11-18, 14:06, Matthias Kaehlcke wrote:
> > On Wed, Nov 21, 2018 at 04:12:47PM +0530, Taniya Das wrote:
> > > +	.boost_enabled	= true,
> > 
> > I have no real expertise with cpufreq boost, but after reading a bit
> > through cpufreq code this seems wrong. Boost is enabled statically,
> > however the driver has neither a ->set_boost function nor does it call
> > cpufreq_enable_boost_support() which would use a default
> > implementation for ->set_boost. As a result boost support is
> > effectively disabled:
> > 
> > static bool cpufreq_boost_supported(void)
> > {
> >         return likely(cpufreq_driver) && cpufreq_driver->set_boost;
> > }
> > 
> > The driver should probably do the same as cpufreq-dt.c and call
> > cpufreq_enable_boost_support() if boost frequencies are available,
> > instead of 'enabling' boost statically.
> 
> Feels like I have written the boost support in cpufreq core few decades back as
> I don't remember any of it :)
> 
> But reading through the code this is what I understood.

Thanks for digging into it!

> There are two parts of boosting.
> 
> - Sysfs file available or not to enable/disable boost frequencies on the go.
>   This file gets created only when cpufreq_enable_boost_support() gets called.
> 
> - Will cpufreq core consider boost frequencies or not while checking target
>   frequency again, this is governed by cpufreq_driver->boost field, which can be
>   set from driver or using the sysfs file mentioned above.
> 
> In this driver, all we have done is to set the cpufreq_driver->boost field to
> true, which would allow cpufreq core to use boost frequencies as well. But that
> isn't any better than making them all normal frequencies and getting rid of
> boost stuff. The boosting stuff will be useful only if you want to disable some
> of them at runtime, based on heating, etc. And that is possible only after you
> create a sysfs file.

That matches what Amit reported (and I confirmed) about the CPU
frequency "being stuck" at the boost frequency
(https://lore.kernel.org/patchwork/patch/998335/#1186701) on a loaded
system.

Taniya: I wonder if it would make sense to drop boost support for now
in order to land a first working version of the driver soon, instead
of keeping respinning this series. Boost support could be added as a
separate feature, just like cooling devices. If you have a working
quick fix now that's also fine, otherwise I'd suggest the iterative
approach, I'm sure you also want to see this landing ;-)

Cheers

Matthias

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

[Taniya Das]

Hello Rob,

On 11/27/2018 12:28 AM, Rob Herring wrote:
> On Wed, Nov 21, 2018 at 10:02:36AM -0800, Matthias Kaehlcke wrote:
>> On Wed, Nov 21, 2018 at 04:12:46PM +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           | 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..90e396b
>>> --- /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 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", "gcc_cpuss_gpll0_clk_src";
>>
>> I don't have/find the document with the clock configuration of this
>> IP block, but the 'clock-names' sound more like aliases for the actual
>> clocks specified in the 'clocks' property (especially
>> 'gcc_cpuss_gpll0_clk_src') than input signals from the IP POV, which
>> I'd expect to have more generic names.
> 
> Also, this doesn't match the documented value above.
> 
> Rob
> 

Thanks, updated it in the latest patch.

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

--

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

[Taniya Das]

Hello Matthias,

On 11/21/2018 11:32 PM, Matthias Kaehlcke wrote:
> On Wed, Nov 21, 2018 at 04:12:46PM +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           | 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..90e396b
>> --- /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 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", "gcc_cpuss_gpll0_clk_src";
> 
> I don't have/find the document with the clock configuration of this
> IP block, but the 'clock-names' sound more like aliases for the actual
> clocks specified in the 'clocks' property (especially
> 'gcc_cpuss_gpll0_clk_src') than input signals from the IP POV, which
> I'd expect to have more generic names.
> 

Updated the clock name to "alternate" in the latest series.

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

--

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

[Taniya Das]

Hello Matthias,

On 11/22/2018 12:11 AM, Matthias Kaehlcke wrote:
> Hi Taniya,
> 
> thanks for respinning, a few nits inline.
> 
> On Wed, Nov 21, 2018 at 04:12:47PM +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 | 346 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 358 insertions(+)
>>   create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 4e1131e..688f102 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -114,6 +114,17 @@ config ARM_QCOM_CPUFREQ_KRYO
>>
>>   	  If in doubt, say N.
>>
>> +config ARM_QCOM_CPUFREQ_HW
>> +	tristate "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 d5ee456..789b2e0 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -62,6 +62,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..6390e85
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -0,0 +1,346 @@
>> +// 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 LUT_MAX_ENTRIES			40U
>> +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
>> +#define LUT_ROW_SIZE			32
>> +#define CLK_HW_DIV			2
>> +
>> +/* Register offsets */
>> +#define REG_ENABLE			0x0
>> +#define REG_LUT_TABLE			0x110
>> +#define REG_PERF_STATE			0x920
>> +
>> +struct cpufreq_qcom {
>> +	struct cpufreq_frequency_table *table;
>> +	void __iomem *perf_base;
> 
> nit: is this really a base address? It's the address of the perf state
> register, right? Better name it 'perf_state_reg'/'reg_perf_state' or
> similar (just 'perf_state' might be confusing, I'd expect a variable
> with this name to hold a state, not an address).
> 

I have updated the name to "perf_state_reg".

>> +	cpumask_t related_cpus;
>> +	unsigned int max_cores;
>> +	unsigned long xo_rate;
>> +	unsigned long cpu_hw_rate;
>> +};
>> +
>> +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->perf_base);
>> +
>> +	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->perf_base);
>> +	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->perf_base);
>> +
>> +	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, void __iomem *base)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	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;
>> +
>> +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> +		data = readl_relaxed(base + REG_LUT_TABLE + 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;
>> +	void __iomem *base;
>> +	int ret, cpu_r;
>> +
>> +	if (qcom_freq_domain_map[cpu])
>> +		return 0;
>> +
>> +	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
>> +	if (!c)
>> +		return -ENOMEM;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
>> +	base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>> +
>> +	/* HW should be in enabled state to proceed */
>> +	if (!(readl_relaxed(base + 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;
>> +	c->perf_base = base + REG_PERF_STATE;
>> +
>> +	ret = qcom_cpufreq_hw_read_lut(pdev, c, base);
>> +	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, "gcc_cpuss_gpll0_clk_src");
> 
> As commented on the binding patch, I'm not sure if this is the correct
> name for this clock input from the POV of this IP block. Just a doubt
> at this point, I don't have/find the hardware documentation to suggest
> something better.
> 

The clock name used in the latest series is "alternate".

>> +	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" },
>> +	{}
>> +};
>> +
>> +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);
> 
> I'm still not convinced that a subsys_initcall is needed (instead of
> module_init or device_initcall), as mentioned in the review of v7
> it causes problems when registering CPU cooling devices, but we can
> also fix this when support for cooling devices is added ;-)
> 

Yes, sure, we could revisit this.

>> +static void __exit qcom_cpufreq_hw_exit(void)
>> +{
>> +	cpufreq_unregister_driver(&cpufreq_qcom_hw_driver);
>> +	platform_driver_unregister(&qcom_cpufreq_hw_driver);
>> +}
>> +module_exit(qcom_cpufreq_hw_exit);
>> +
>> +MODULE_DESCRIPTION("QTI CPUFREQ HW Driver");
> 
> nit: make it 'QCOM CPUFreq HW driver' for consistency?
> 

Thanks, have taken care in the latest patch.

> 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 v10 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Taniya Das]

Hello Stephen,

Thanks for the patch, I have updated the latest series with the patch 
and few comments from Matthias.

On 11/21/2018 11:53 PM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-11-21 02:42:47)
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index d5ee456..789b2e0 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -62,6 +62,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
> 
> This isn't alphabetically sorted.
> 
>>   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..6390e85
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -0,0 +1,346 @@
>> +// 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 LUT_MAX_ENTRIES                        40U
>> +#define CORE_COUNT_VAL(val)            (((val) & (GENMASK(18, 16))) >> 16)
>> +#define LUT_ROW_SIZE                   32
>> +#define CLK_HW_DIV                     2
>> +
>> +/* Register offsets */
>> +#define REG_ENABLE                     0x0
>> +#define REG_LUT_TABLE                  0x110
>> +#define REG_PERF_STATE                 0x920
>> +
>> +struct cpufreq_qcom {
>> +       struct cpufreq_frequency_table *table;
>> +       void __iomem *perf_base;
>> +       cpumask_t related_cpus;
>> +       unsigned int max_cores;
>> +       unsigned long xo_rate;
>> +       unsigned long cpu_hw_rate;
>> +};
>> +
>> +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->perf_base);
>> +
> 
> This isn't using the pointer directly though.
> 
>> +       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->perf_base);
>> +       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->perf_base);
>> +
>> +       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, void __iomem *base)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       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;
>> +
>> +       for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> +               data = readl_relaxed(base + REG_LUT_TABLE + i * LUT_ROW_SIZE);
>> +               src = (data & GENMASK(31, 30)) >> 30;
>> +               lval = data & GENMASK(7, 0);
>> +               core_count = CORE_COUNT_VAL(data);
> 
> CORE_COUNT_VAL is used in one place. Just inline it like the src and
> lval ones? Or better yet, use FIELD_GET helpers.
> 
>> +
>> +               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;
> 
> Write this as:
> 
> 		if (src)
> 			cur_freq = c->xo_rate * lval / 1000;
> 		else
> 			cur_freq = c->cpu_hw_rate / 1000;
> 		
> 		c->table[i].frequency = cur_freq;
> 
> for shorter lines.
> 
>> +
>> +               dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
>> +                       i, c->table[i].frequency, core_count);
> 
> Use cur_freq instead of the long route?
> 
>> +
>> +               if (core_count != c->max_cores)
>> +                       cur_freq = CPUFREQ_ENTRY_INVALID;
> 
> Don't we want to knock out the intermediate frequencies that also don't
> have the max_cores core_count? If so, I would expect there to be
> CPUFREQ_ENTRY_INVALID in the frequency table, but this isn't doing that.
> 
>> +
>> +               /*
>> +                * 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;
>> +               }
> 
> This is really hard to read.
> 
>> +               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;
>> +       void __iomem *base;
>> +       int ret, cpu_r;
>> +
>> +       if (qcom_freq_domain_map[cpu])
>> +               return 0;
>> +
>> +       c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
>> +       if (!c)
>> +               return -ENOMEM;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, index);
>> +       base = devm_ioremap_resource(dev, res);
>> +       if (IS_ERR(base))
>> +               return PTR_ERR(base);
>> +
>> +       /* HW should be in enabled state to proceed */
>> +       if (!(readl_relaxed(base + 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;
> 
> Why do we need to pass around these local variables in the structure?
> 
>> +       c->perf_base = base + REG_PERF_STATE;
>> +
>> +       ret = qcom_cpufreq_hw_read_lut(pdev, c, base);
>> +       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, "gcc_cpuss_gpll0_clk_src");
> 
> I didn't see this in the binding. And it doesn't make sense still
> because now it's a global clk name. It can't be called "safe" or
> "backup" or something like that?
> 
>> +       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;
>> +       }
> 
> Not sure why this is split out of the probe function.
> 
>> +
>> +       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" },
>> +       {}
>> +};
> 
> Missing MODULE_DEVICE_TABLE here.
> 
>> +
>> +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);
>> +
>> +static void __exit qcom_cpufreq_hw_exit(void)
>> +{
>> +       cpufreq_unregister_driver(&cpufreq_qcom_hw_driver);
> 
> Why isn't this done in driver remove?
> 
>> +       platform_driver_unregister(&qcom_cpufreq_hw_driver);
>> +}
>> +module_exit(qcom_cpufreq_hw_exit);
>> +
>> +MODULE_DESCRIPTION("QTI CPUFREQ HW Driver");
>> +MODULE_LICENSE("GPL v2");
> 
> Here's a patch I tested out to do all this. Simple testing shows it
> works. If you need my sign-off to fold it in here you go.
> 
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> 
>   drivers/cpufreq/Makefile          |   2 +-
>   drivers/cpufreq/qcom-cpufreq-hw.c | 147 ++++++++++++++++++--------------------
>   2 files changed, 69 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 789b2e06f3ba..08c071be2491 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -61,8 +61,8 @@ obj-$(CONFIG_MACH_MVEBU_V7)		+= mvebu-cpufreq.o
>   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_QCOM_CPUFREQ_KRYO)	+= qcom-cpufreq-kryo.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
> index 6390e85df1b3..105c9fe69a64 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -3,6 +3,7 @@
>    * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>    */
>   
> +#include <linux/bitfield.h>
>   #include <linux/cpufreq.h>
>   #include <linux/init.h>
>   #include <linux/kernel.h>
> @@ -11,7 +12,9 @@
>   #include <linux/of_platform.h>
>   
>   #define LUT_MAX_ENTRIES			40U
> -#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
> +#define LUT_SRC				GENMASK(31, 30)
> +#define LUT_L_VAL			GENMASK(7, 0)
> +#define LUT_CORE_COUNT			GENMASK(18, 16)
>   #define LUT_ROW_SIZE			32
>   #define CLK_HW_DIV			2
>   
> @@ -24,27 +27,23 @@ struct cpufreq_qcom {
>   	struct cpufreq_frequency_table *table;
>   	void __iomem *perf_base;
>   	cpumask_t related_cpus;
> -	unsigned int max_cores;
> -	unsigned long xo_rate;
> -	unsigned long cpu_hw_rate;
>   };
>   
>   static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
>   
> -static int
> -qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> -			     unsigned int index)
> +static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> +					unsigned int index)
>   {
> -	struct cpufreq_qcom *c = policy->driver_data;
> +	void __iomem *perf_base = policy->driver_data;
>   
> -	writel_relaxed(index, c->perf_base);
> +	writel_relaxed(index, perf_base);
>   
>   	return 0;
>   }
>   
>   static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
>   {
> -	struct cpufreq_qcom *c;
> +	void __iomem *perf_base;
>   	struct cpufreq_policy *policy;
>   	unsigned int index;
>   
> @@ -52,26 +51,25 @@ static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
>   	if (!policy)
>   		return 0;
>   
> -	c = policy->driver_data;
> +	perf_base = policy->driver_data;
>   
> -	index = readl_relaxed(c->perf_base);
> +	index = readl_relaxed(perf_base);
>   	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)
> +static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
> +						unsigned int target_freq)
>   {
> -	struct cpufreq_qcom *c = policy->driver_data;
> +	void __iomem *perf_base = policy->driver_data;
>   	int index;
>   
>   	index = policy->cached_resolved_idx;
>   	if (index < 0)
>   		return 0;
>   
> -	writel_relaxed(index, c->perf_base);
> +	writel_relaxed(index, perf_base);
>   
>   	return policy->freq_table[index].frequency;
>   }
> @@ -90,7 +88,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>   
>   	policy->fast_switch_possible = true;
>   	policy->freq_table = c->table;
> -	policy->driver_data = c;
> +	policy->driver_data = c->perf_base;
>   
>   	return 0;
>   }
> @@ -114,11 +112,12 @@ static struct cpufreq_driver cpufreq_qcom_hw_driver = {
>   	.boost_enabled	= true,
>   };
>   
> -static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
> -				    struct cpufreq_qcom *c, void __iomem *base)
> +static int qcom_cpufreq_hw_read_lut(struct device *dev, struct cpufreq_qcom *c,
> +				    void __iomem *base, unsigned long xo_rate,
> +				    unsigned long cpu_hw_rate)
>   {
> -	struct device *dev = &pdev->dev;
> -	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
> +	u32 data, src, lval, i, core_count, prev_cc, prev_freq, freq;
> +	unsigned int max_cores = cpumask_weight(&c->related_cpus);
>   
>   	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
>   				sizeof(*c->table), GFP_KERNEL);
> @@ -127,37 +126,45 @@ static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
>   
>   	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>   		data = readl_relaxed(base + REG_LUT_TABLE + i * LUT_ROW_SIZE);
> -		src = (data & GENMASK(31, 30)) >> 30;
> -		lval = data & GENMASK(7, 0);
> -		core_count = CORE_COUNT_VAL(data);
> +		src = FIELD_GET(LUT_SRC, data);
> +		lval = FIELD_GET(LUT_L_VAL, data);
> +		core_count = FIELD_GET(LUT_CORE_COUNT, data);
>   
>   		if (src)
> -			c->table[i].frequency = c->xo_rate * lval / 1000;
> +			freq = 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;
> +			freq = cpu_hw_rate / 1000;
> +
> +		/* Ignore boosts in the middle of the table */
> +		if (core_count != max_cores) {
> +			c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
> +		} else {
> +			c->table[i].frequency = freq;
> +			dev_dbg(dev, "index=%d freq=%d, core_count %d\n", i,
> +				freq, core_count);
> +		}
>   
>   		/*
>   		 * Two of the same frequencies with the same core counts means
> -		 * end of table.
> +		 * end of table
>   		 */
> -		if (i > 0 && c->table[i - 1].frequency ==
> -		   c->table[i].frequency && prev_cc == core_count) {
> +		if (i > 0 && prev_freq == freq && prev_cc == core_count) {
>   			struct cpufreq_frequency_table *prev = &c->table[i - 1];
>   
> -			if (prev_freq == CPUFREQ_ENTRY_INVALID)
> +			/*
> +			 * Only treat the last frequency that might be a boost
> +			 * as the boost frequency
> +			 */
> +			if (prev_cc != max_cores) {
> +				prev->frequency = prev_freq;
>   				prev->flags = CPUFREQ_BOOST_FREQ;
> +			}
> +
>   			break;
>   		}
> +
>   		prev_cc = core_count;
> -		prev_freq = cur_freq;
> +		prev_freq = freq;
>   	}
>   
>   	c->table[i].frequency = CPUFREQ_TABLE_END;
> @@ -165,7 +172,7 @@ static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
>   	return 0;
>   }
>   
> -static int qcom_get_related_cpus(int index, struct cpumask *m)
> +static void qcom_get_related_cpus(int index, struct cpumask *m)
>   {
>   	struct device_node *cpu_np;
>   	struct of_phandle_args args;
> @@ -186,8 +193,6 @@ static int qcom_get_related_cpus(int index, struct cpumask *m)
>   		if (index == args.args[0])
>   			cpumask_set_cpu(cpu, m);
>   	}
> -
> -	return 0;
>   }
>   
>   static int qcom_cpu_resources_init(struct platform_device *pdev,
> @@ -201,9 +206,6 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
>   	void __iomem *base;
>   	int ret, cpu_r;
>   
> -	if (qcom_freq_domain_map[cpu])
> -		return 0;
> -
>   	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
>   	if (!c)
>   		return -ENOMEM;
> @@ -219,21 +221,15 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
>   		return -ENODEV;
>   	}
>   
> -	ret = qcom_get_related_cpus(index, &c->related_cpus);
> -	if (ret) {
> +	qcom_get_related_cpus(index, &c->related_cpus);
> +	if (!cpumask_weight(&c->related_cpus)) {
>   		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;
>   	c->perf_base = base + REG_PERF_STATE;
>   
> -	ret = qcom_cpufreq_hw_read_lut(pdev, c, base);
> +	ret = qcom_cpufreq_hw_read_lut(dev, c, base, xo_rate, cpu_hw_rate);
>   	if (ret) {
>   		dev_err(dev, "Domain-%d failed to read LUT\n", index);
>   		return ret;
> @@ -245,7 +241,7 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
>   	return 0;
>   }
>   
> -static int qcom_resources_init(struct platform_device *pdev)
> +static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
>   {
>   	struct device_node *cpu_np;
>   	struct of_phandle_args args;
> @@ -259,15 +255,13 @@ static int qcom_resources_init(struct platform_device *pdev)
>   		return PTR_ERR(clk);
>   
>   	xo_rate = clk_get_rate(clk);
> -
>   	clk_put(clk);
>   
> -	clk = clk_get(&pdev->dev, "gcc_cpuss_gpll0_clk_src");
> +	clk = clk_get(&pdev->dev, "alternate");
>   	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) {
> @@ -282,33 +276,22 @@ static int qcom_resources_init(struct platform_device *pdev)
>   						 "#freq-domain-cells", 0,
>   						  &args);
>   		of_node_put(cpu_np);
> -		if (ret < 0)
> +		if (ret)
>   			return ret;
>   
> +		if (qcom_freq_domain_map[cpu])
> +			continue;
> +
>   		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) {
> +	ret = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
> +	if (ret) {
>   		dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
> -		return rc;
> +		return ret;
>   	}
>   
>   	dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
> @@ -316,13 +299,20 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +static int qcom_cpufreq_hw_driver_remove(struct platform_device *pdev)
> +{
> +	return cpufreq_unregister_driver(&cpufreq_qcom_hw_driver);
> +}
> +
>   static const struct of_device_id qcom_cpufreq_hw_match[] = {
>   	{ .compatible = "qcom,cpufreq-hw" },
>   	{}
>   };
> +MODULE_DEVICE_TABLE(of, qcom_cpufreq_hw_match);
>   
>   static struct platform_driver qcom_cpufreq_hw_driver = {
>   	.probe = qcom_cpufreq_hw_driver_probe,
> +	.remove = qcom_cpufreq_hw_driver_remove,
>   	.driver = {
>   		.name = "qcom-cpufreq-hw",
>   		.of_match_table = qcom_cpufreq_hw_match,
> @@ -337,7 +327,6 @@ subsys_initcall(qcom_cpufreq_hw_init);
>   
>   static void __exit qcom_cpufreq_hw_exit(void)
>   {
> -	cpufreq_unregister_driver(&cpufreq_qcom_hw_driver);
>   	platform_driver_unregister(&qcom_cpufreq_hw_driver);
>   }
>   module_exit(qcom_cpufreq_hw_exit);
> 

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

--