linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v11 0/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW
@ 2018-12-02  3:55 Taniya Das
  2018-12-02  3:55 ` [PATCH v11 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings Taniya Das
  2018-12-02  3:55 ` [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver Taniya Das
  0 siblings, 2 replies; 21+ messages in thread
From: Taniya Das @ 2018-12-02  3:55 UTC (permalink / raw)
  To: Rafael J. Wysocki, Viresh Kumar, linux-kernel, linux-pm, Stephen Boyd
  Cc: Rajendra Nayak, devicetree, robh, skannan, linux-arm-msm,
	evgreen, Taniya Das

 [v11]
   * Updated the code logic as per Stephen.
   * Default boost enabled is removed.
   * Update the clock name to use "alternate" for GPLL0 source in code and
     Documentation binding.
   * Module description updated.
   * perf_base updated to perf_state_reg.

 [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                  | 334 +++++++++++++++++++++
 4 files changed, 518 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.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v11 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings
  2018-12-02  3:55 [PATCH v11 0/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW Taniya Das
@ 2018-12-02  3:55 ` Taniya Das
  2018-12-03 16:55   ` Stephen Boyd
  2018-12-03 23:09   ` Rob Herring
  2018-12-02  3:55 ` [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver Taniya Das
  1 sibling, 2 replies; 21+ messages in thread
From: Taniya Das @ 2018-12-02  3:55 UTC (permalink / raw)
  To: Rafael J. Wysocki, Viresh Kumar, linux-kernel, linux-pm, Stephen Boyd
  Cc: Rajendra Nayak, devicetree, robh, skannan, linux-arm-msm,
	evgreen, 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..2b82965
--- /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", "alternate".
+
+- 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", "alternate";
+
+		#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.


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
  2018-12-02  3:55 [PATCH v11 0/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW Taniya Das
  2018-12-02  3:55 ` [PATCH v11 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings Taniya Das
@ 2018-12-02  3:55 ` Taniya Das
  2018-12-03 16:57   ` Stephen Boyd
                     ` (2 more replies)
  1 sibling, 3 replies; 21+ messages in thread
From: Taniya Das @ 2018-12-02  3:55 UTC (permalink / raw)
  To: Rafael J. Wysocki, Viresh Kumar, linux-kernel, linux-pm, Stephen Boyd
  Cc: Rajendra Nayak, devicetree, robh, skannan, linux-arm-msm,
	evgreen, Taniya Das, Stephen Boyd

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: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/cpufreq/Kconfig.arm       |  11 ++
 drivers/cpufreq/Makefile          |   1 +
 drivers/cpufreq/qcom-cpufreq-hw.c | 334 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 346 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..08c071b 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -61,6 +61,7 @@ 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_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
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
new file mode 100644
index 0000000..8dc6b73
--- /dev/null
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -0,0 +1,334 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * 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>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+
+#define LUT_MAX_ENTRIES			40U
+#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
+
+/* 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_state_reg;
+	cpumask_t related_cpus;
+};
+
+static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
+
+static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
+					unsigned int index)
+{
+	void __iomem *perf_state_reg = policy->driver_data;
+
+	writel_relaxed(index, perf_state_reg);
+
+	return 0;
+}
+
+static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
+{
+	void __iomem *perf_state_reg;
+	struct cpufreq_policy *policy;
+	unsigned int index;
+
+	policy = cpufreq_cpu_get_raw(cpu);
+	if (!policy)
+		return 0;
+
+	perf_state_reg = policy->driver_data;
+
+	index = readl_relaxed(perf_state_reg);
+	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)
+{
+	void __iomem *perf_state_reg = policy->driver_data;
+	int index;
+
+	index = policy->cached_resolved_idx;
+	if (index < 0)
+		return 0;
+
+	writel_relaxed(index, perf_state_reg);
+
+	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->perf_state_reg;
+
+	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,
+};
+
+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)
+{
+	u32 data, src, lval, i, core_count, prev_cc = 0, prev_freq = 0, freq;
+	unsigned int max_cores = cpumask_weight(&c->related_cpus);
+
+	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 = FIELD_GET(LUT_SRC, data);
+		lval = FIELD_GET(LUT_L_VAL, data);
+		core_count = FIELD_GET(LUT_CORE_COUNT, data);
+
+		if (src)
+			freq = xo_rate * lval / 1000;
+		else
+			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
+		 */
+		if (i > 0 && prev_freq == freq && prev_cc == core_count) {
+			struct cpufreq_frequency_table *prev = &c->table[i - 1];
+
+			/*
+			 * 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 = freq;
+	}
+
+	c->table[i].frequency = CPUFREQ_TABLE_END;
+
+	return 0;
+}
+
+static void 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);
+	}
+}
+
+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;
+
+	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;
+	}
+
+	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 -ENOENT;
+	}
+
+	c->perf_state_reg = base + REG_PERF_STATE;
+
+	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;
+	}
+
+	for_each_cpu(cpu_r, &c->related_cpus)
+		qcom_freq_domain_map[cpu_r] = c;
+
+	return 0;
+}
+
+static int qcom_cpufreq_hw_driver_probe(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, "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)
+			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;
+	}
+
+	ret = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
+	if (ret) {
+		dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
+		return ret;
+	}
+
+	dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
+
+	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,
+	},
+};
+
+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)
+{
+	platform_driver_unregister(&qcom_cpufreq_hw_driver);
+}
+module_exit(qcom_cpufreq_hw_exit);
+
+MODULE_DESCRIPTION("QCOM 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.


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings
  2018-12-02  3:55 ` [PATCH v11 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings Taniya Das
@ 2018-12-03 16:55   ` Stephen Boyd
  2018-12-03 23:09   ` Rob Herring
  1 sibling, 0 replies; 21+ messages in thread
From: Stephen Boyd @ 2018-12-03 16:55 UTC (permalink / raw)
  To: Rafael J. Wysocki, Taniya Das, Viresh Kumar, linux-kernel, linux-pm
  Cc: Rajendra Nayak, devicetree, robh, skannan, linux-arm-msm,
	evgreen, Taniya Das

Quoting Taniya Das (2018-12-01 19:55:02)
> Add QCOM cpufreq firmware device bindings for Qualcomm Technology Inc's
> SoCs. This is required for managing the cpu frequency transitions which are
> controlled by the hardware engine.
> 
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
  2018-12-02  3:55 ` [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver Taniya Das
@ 2018-12-03 16:57   ` Stephen Boyd
  2018-12-03 17:46   ` Matthias Kaehlcke
  2018-12-04  5:12   ` Viresh Kumar
  2 siblings, 0 replies; 21+ messages in thread
From: Stephen Boyd @ 2018-12-03 16:57 UTC (permalink / raw)
  To: Rafael J. Wysocki, Taniya Das, Viresh Kumar, linux-kernel, linux-pm
  Cc: Rajendra Nayak, devicetree, robh, skannan, linux-arm-msm,
	evgreen, Taniya Das

Quoting Taniya Das (2018-12-01 19:55:03)
> 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: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
  2018-12-02  3:55 ` [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver Taniya Das
  2018-12-03 16:57   ` Stephen Boyd
@ 2018-12-03 17:46   ` Matthias Kaehlcke
  2018-12-04  5:12   ` Viresh Kumar
  2 siblings, 0 replies; 21+ messages in thread
From: Matthias Kaehlcke @ 2018-12-03 17:46 UTC (permalink / raw)
  To: Taniya Das
  Cc: Rafael J. Wysocki, Viresh Kumar, linux-kernel, linux-pm,
	Stephen Boyd, Rajendra Nayak, devicetree, robh, skannan,
	linux-arm-msm, evgreen, Stephen Boyd

On Sun, Dec 02, 2018 at 09:25:03AM +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: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  drivers/cpufreq/Kconfig.arm       |  11 ++
>  drivers/cpufreq/Makefile          |   1 +
>  drivers/cpufreq/qcom-cpufreq-hw.c | 334 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 346 insertions(+)
>  create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
> 
> ...
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> new file mode 100644
> index 0000000..8dc6b73
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> 
> ...
> 
> +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)
> +{
> +	u32 data, src, lval, i, core_count, prev_cc = 0, prev_freq = 0, freq;
> +	unsigned int max_cores = cpumask_weight(&c->related_cpus);
> +
> +	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 = FIELD_GET(LUT_SRC, data);
> +		lval = FIELD_GET(LUT_L_VAL, data);
> +		core_count = FIELD_GET(LUT_CORE_COUNT, data);
> +
> +		if (src)
> +			freq = xo_rate * lval / 1000;
> +		else
> +			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);
> +		}

nit: IMO it would be better to put the normal case ("core_count !=
max_cores") first and the exception in the else branch.

> +MODULE_DESCRIPTION("QCOM CPUFREQ HW Driver");

nit: my suggestion was 'QCOM CPUFreq HW driver', which is what's used
elsewhere in the driver.

Anyway, no need to respin just for the nits, we can address them (or
not) with follow-up patches.

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings
  2018-12-02  3:55 ` [PATCH v11 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings Taniya Das
  2018-12-03 16:55   ` Stephen Boyd
@ 2018-12-03 23:09   ` Rob Herring
  2018-12-03 23:57     ` Stephen Boyd
  1 sibling, 1 reply; 21+ messages in thread
From: Rob Herring @ 2018-12-03 23:09 UTC (permalink / raw)
  To: Taniya Das
  Cc: Rafael J. Wysocki, Viresh Kumar, linux-kernel, linux-pm,
	Stephen Boyd, Rajendra Nayak, devicetree, skannan, linux-arm-msm,
	evgreen

On Sun, Dec 02, 2018 at 09:25:02AM +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..2b82965
> --- /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", "alternate".
> +
> +- 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:

#freq-domain-cells

Otherwise,

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings
  2018-12-03 23:09   ` Rob Herring
@ 2018-12-03 23:57     ` Stephen Boyd
  0 siblings, 0 replies; 21+ messages in thread
From: Stephen Boyd @ 2018-12-03 23:57 UTC (permalink / raw)
  To: Rob Herring, Taniya Das
  Cc: Rafael J. Wysocki, Viresh Kumar, linux-kernel, linux-pm,
	Rajendra Nayak, devicetree, skannan, linux-arm-msm, evgreen

Quoting Rob Herring (2018-12-03 15:09:07)
> On Sun, Dec 02, 2018 at 09:25:02AM +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..2b82965
> > --- /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", "alternate".
> > +
> > +- 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:
> 
> #freq-domain-cells
> 
> Otherwise,
> 
> Reviewed-by: Rob Herring <robh@kernel.org>

Or should it be #qcom,freq-domain-cells? That would match the same stem
of the property used in the cpu node.



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
  2018-12-02  3:55 ` [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver Taniya Das
  2018-12-03 16:57   ` Stephen Boyd
  2018-12-03 17:46   ` Matthias Kaehlcke
@ 2018-12-04  5:12   ` Viresh Kumar
  2018-12-04  9:27     ` Taniya Das
  2018-12-04 22:28     ` Stephen Boyd
  2 siblings, 2 replies; 21+ messages in thread
From: Viresh Kumar @ 2018-12-04  5:12 UTC (permalink / raw)
  To: Taniya Das
  Cc: Rafael J. Wysocki, linux-kernel, linux-pm, Stephen Boyd,
	Rajendra Nayak, devicetree, robh, skannan, linux-arm-msm,
	evgreen, Stephen Boyd

Hi Taniya,

Sorry that I haven't been reviewing it much from last few iterations as I was
letting others get this into a better shape. Thanks for your efforts..

On 02-12-18, 09:25, Taniya Das wrote:
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c

> +struct cpufreq_qcom {
> +	struct cpufreq_frequency_table *table;
> +	void __iomem *perf_state_reg;
> +	cpumask_t related_cpus;
> +};
> +
> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];

Now that the code is much more simplified, I am not sure if you need this
per-cpu structure at all. The only place where you are using it is in
qcom_cpufreq_hw_cpu_init() and probe(). Why not merge qcom_cpu_resources_init()
completely into qcom_cpufreq_hw_cpu_init() and get rid of this structure
entirely ?

-- 
viresh

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
  2018-12-04  5:12   ` Viresh Kumar
@ 2018-12-04  9:27     ` Taniya Das
  2018-12-04  9:29       ` Viresh Kumar
  2018-12-04 22:28     ` Stephen Boyd
  1 sibling, 1 reply; 21+ messages in thread
From: Taniya Das @ 2018-12-04  9:27 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael J. Wysocki, linux-kernel, linux-pm, Stephen Boyd,
	Rajendra Nayak, devicetree, robh, skannan, linux-arm-msm,
	evgreen, Stephen Boyd

Hello Viresh,

On 12/4/2018 10:42 AM, Viresh Kumar wrote:
> Hi Taniya,
> 
> Sorry that I haven't been reviewing it much from last few iterations as I was
> letting others get this into a better shape. Thanks for your efforts..
> 
> On 02-12-18, 09:25, Taniya Das wrote:
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> 
>> +struct cpufreq_qcom {
>> +	struct cpufreq_frequency_table *table;
>> +	void __iomem *perf_state_reg;
>> +	cpumask_t related_cpus;
>> +};
>> +
>> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> 
> Now that the code is much more simplified, I am not sure if you need this
> per-cpu structure at all. The only place where you are using it is in
> qcom_cpufreq_hw_cpu_init() and probe(). Why not merge qcom_cpu_resources_init()
> completely into qcom_cpufreq_hw_cpu_init() and get rid of this structure
> entirely ?
> 

Yes, we still would require the per-cpu.

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

--

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
  2018-12-04  9:27     ` Taniya Das
@ 2018-12-04  9:29       ` Viresh Kumar
  0 siblings, 0 replies; 21+ messages in thread
From: Viresh Kumar @ 2018-12-04  9:29 UTC (permalink / raw)
  To: Taniya Das
  Cc: Rafael J. Wysocki, linux-kernel, linux-pm, Stephen Boyd,
	Rajendra Nayak, devicetree, robh, skannan, linux-arm-msm,
	evgreen, Stephen Boyd

On 04-12-18, 14:57, Taniya Das wrote:
> Hello Viresh,
> 
> On 12/4/2018 10:42 AM, Viresh Kumar wrote:
> > Hi Taniya,
> > 
> > Sorry that I haven't been reviewing it much from last few iterations as I was
> > letting others get this into a better shape. Thanks for your efforts..
> > 
> > On 02-12-18, 09:25, Taniya Das wrote:
> > > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > 
> > > +struct cpufreq_qcom {
> > > +	struct cpufreq_frequency_table *table;
> > > +	void __iomem *perf_state_reg;
> > > +	cpumask_t related_cpus;
> > > +};
> > > +
> > > +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> > 
> > Now that the code is much more simplified, I am not sure if you need this
> > per-cpu structure at all. The only place where you are using it is in
> > qcom_cpufreq_hw_cpu_init() and probe(). Why not merge qcom_cpu_resources_init()
> > completely into qcom_cpufreq_hw_cpu_init() and get rid of this structure
> > entirely ?
> > 
> 
> Yes, we still would require the per-cpu.

An explanation on why do you feel so would have been nice :)

I am sure I am missing something obvious here.

-- 
viresh

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
  2018-12-04  5:12   ` Viresh Kumar
  2018-12-04  9:27     ` Taniya Das
@ 2018-12-04 22:28     ` Stephen Boyd
  2018-12-04 23:05       ` Stephen Boyd
  1 sibling, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2018-12-04 22:28 UTC (permalink / raw)
  To: Taniya Das, Viresh Kumar
  Cc: Rafael J. Wysocki, linux-kernel, linux-pm, Rajendra Nayak,
	devicetree, robh, skannan, linux-arm-msm, evgreen

Quoting Viresh Kumar (2018-12-03 21:12:31)
> Hi Taniya,
> 
> Sorry that I haven't been reviewing it much from last few iterations as I was
> letting others get this into a better shape. Thanks for your efforts..
> 
> On 02-12-18, 09:25, Taniya Das wrote:
> > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> 
> > +struct cpufreq_qcom {
> > +     struct cpufreq_frequency_table *table;
> > +     void __iomem *perf_state_reg;
> > +     cpumask_t related_cpus;
> > +};
> > +
> > +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> 
> Now that the code is much more simplified, I am not sure if you need this
> per-cpu structure at all. The only place where you are using it is in
> qcom_cpufreq_hw_cpu_init() and probe(). Why not merge qcom_cpu_resources_init()
> completely into qcom_cpufreq_hw_cpu_init() and get rid of this structure
> entirely ?
> 

Good point. Here's an untested patch to handle that. It removes the
related functionality and makes an array of pointers to the domains that
are created each time qcom_cpu_resources_init() is called.

---8<----

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 8dc6b73c2f22..04e7cfd70316 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -23,14 +23,14 @@
 #define REG_LUT_TABLE			0x110
 #define REG_PERF_STATE			0x920
 
+#define MAX_FREQ_DOMAINS		2
+
 struct cpufreq_qcom {
 	struct cpufreq_frequency_table *table;
 	void __iomem *perf_state_reg;
 	cpumask_t related_cpus;
 };
 
-static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
-
 static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
 					unsigned int index)
 {
@@ -76,9 +76,14 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
 
 static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 {
+	struct cpufreq_qcom **freq_domain_map;
 	struct cpufreq_qcom *c;
 
-	c = qcom_freq_domain_map[policy->cpu];
+	freq_domain_map = cpufreq_get_driver_data();
+	if (!freq_domain_map)
+		return -ENODEV;
+
+	c = freq_domain_map[policy->cpu];
 	if (!c) {
 		pr_err("No scaling support for CPU%d\n", policy->cpu);
 		return -ENODEV;
@@ -171,39 +176,17 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev, struct cpufreq_qcom *c,
 	return 0;
 }
 
-static void 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);
-	}
-}
-
 static int qcom_cpu_resources_init(struct platform_device *pdev,
 				   unsigned int cpu, int index,
 				   unsigned long xo_rate,
-				   unsigned long cpu_hw_rate)
+				   unsigned long cpu_hw_rate,
+				   struct cpufreq_qcom **freq_domain_map)
 {
 	struct cpufreq_qcom *c;
 	struct resource *res;
 	struct device *dev = &pdev->dev;
 	void __iomem *base;
-	int ret, cpu_r;
+	int ret;
 
 	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
 	if (!c)
@@ -220,12 +203,6 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
 		return -ENODEV;
 	}
 
-	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 -ENOENT;
-	}
-
 	c->perf_state_reg = base + REG_PERF_STATE;
 
 	ret = qcom_cpufreq_hw_read_lut(dev, c, base, xo_rate, cpu_hw_rate);
@@ -234,8 +211,7 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
 		return ret;
 	}
 
-	for_each_cpu(cpu_r, &c->related_cpus)
-		qcom_freq_domain_map[cpu_r] = c;
+	freq_domain_map[index] = c;
 
 	return 0;
 }
@@ -245,9 +221,16 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 	struct device_node *cpu_np;
 	struct of_phandle_args args;
 	struct clk *clk;
-	unsigned int cpu;
+	unsigned int cpu, domain;
 	unsigned long xo_rate, cpu_hw_rate;
 	int ret;
+	struct cpufreq_qcom **freq_domain_map, *freq_domain;
+
+	freq_domain_map = devm_kcalloc(&pdev->dev, MAX_FREQ_DOMAINS,
+				       sizeof(*freq_domain_map), GFP_KERNEL);
+	cpufreq_qcom_hw_driver.driver_data = freq_domain_map;
+	if (!freq_domain_map)
+		return -ENOMEM;
 
 	clk = clk_get(&pdev->dev, "xo");
 	if (IS_ERR(clk))
@@ -273,16 +256,28 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 
 		ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
 						 "#freq-domain-cells", 0,
-						  &args);
+						 &args);
 		of_node_put(cpu_np);
 		if (ret)
 			return ret;
 
-		if (qcom_freq_domain_map[cpu])
+		domain = args.args[0];
+		if (domain >= MAX_FREQ_DOMAINS)
 			continue;
 
-		ret = qcom_cpu_resources_init(pdev, cpu, args.args[0],
-					      xo_rate, cpu_hw_rate);
+		/*
+		 * If we've already populated the frequency table for this domain
+		 * just mark it related and get out of here
+		 */
+		freq_domain = freq_domain_map[domain];
+		if (freq_domain) {
+			cpumask_set_cpu(cpu, &freq_domain->related_cpus);
+			continue;
+		}
+
+		ret = qcom_cpu_resources_init(pdev, cpu, domain,
+					      xo_rate, cpu_hw_rate,
+					      freq_domain_map);
 		if (ret)
 			return ret;
 	}

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
  2018-12-04 22:28     ` Stephen Boyd
@ 2018-12-04 23:05       ` Stephen Boyd
  2018-12-05  3:37         ` Taniya Das
  0 siblings, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2018-12-04 23:05 UTC (permalink / raw)
  To: Taniya Das, Viresh Kumar
  Cc: Rafael J. Wysocki, linux-kernel, linux-pm, Rajendra Nayak,
	devicetree, robh, skannan, linux-arm-msm, evgreen

Quoting Stephen Boyd (2018-12-04 14:28:11)
> Quoting Viresh Kumar (2018-12-03 21:12:31)
> > Hi Taniya,
> > 
> > Sorry that I haven't been reviewing it much from last few iterations as I was
> > letting others get this into a better shape. Thanks for your efforts..
> > 
> > On 02-12-18, 09:25, Taniya Das wrote:
> > > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > 
> > > +struct cpufreq_qcom {
> > > +     struct cpufreq_frequency_table *table;
> > > +     void __iomem *perf_state_reg;
> > > +     cpumask_t related_cpus;
> > > +};
> > > +
> > > +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> > 
> > Now that the code is much more simplified, I am not sure if you need this
> > per-cpu structure at all. The only place where you are using it is in
> > qcom_cpufreq_hw_cpu_init() and probe(). Why not merge qcom_cpu_resources_init()
> > completely into qcom_cpufreq_hw_cpu_init() and get rid of this structure
> > entirely ?
> > 
> 
> Good point. Here's an untested patch to handle that. It removes the
> related functionality and makes an array of pointers to the domains that
> are created each time qcom_cpu_resources_init() is called.
> 
> ---8<----
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index 8dc6b73c2f22..04e7cfd70316 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -23,14 +23,14 @@
>  #define REG_LUT_TABLE                  0x110
>  #define REG_PERF_STATE                 0x920
>  
> +#define MAX_FREQ_DOMAINS               2
> +
>  struct cpufreq_qcom {
>         struct cpufreq_frequency_table *table;
>         void __iomem *perf_state_reg;
>         cpumask_t related_cpus;
>  };
>  
> -static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> -
>  static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
>                                         unsigned int index)
>  {
> @@ -76,9 +76,14 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
>  
>  static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  {
> +       struct cpufreq_qcom **freq_domain_map;
>         struct cpufreq_qcom *c;
>  
> -       c = qcom_freq_domain_map[policy->cpu];
> +       freq_domain_map = cpufreq_get_driver_data();
> +       if (!freq_domain_map)
> +               return -ENODEV;
> +
> +       c = freq_domain_map[policy->cpu];

And this fails now because it indexes based on cpu number. We have to
parse the frequency domain out of the cpunode again to fix that.

Here's the updated (still untested) patch and I also just allocated the
freq domain array up front instead of in each domain init routine to
simplify some more.

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 8dc6b73c2f22..bc0d734f7e3c 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -23,14 +23,14 @@
 #define REG_LUT_TABLE			0x110
 #define REG_PERF_STATE			0x920
 
+#define MAX_FREQ_DOMAINS		2
+
 struct cpufreq_qcom {
 	struct cpufreq_frequency_table *table;
 	void __iomem *perf_state_reg;
 	cpumask_t related_cpus;
 };
 
-static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
-
 static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
 					unsigned int index)
 {
@@ -76,9 +76,26 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
 
 static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 {
-	struct cpufreq_qcom *c;
+	struct cpufreq_qcom *freq_domain_map, *c;
+	struct device_node *cpu_np;
+	struct of_phandle_args args;
+	int ret;
+
+	freq_domain_map = cpufreq_get_driver_data();
+	if (!freq_domain_map)
+		return -ENODEV;
+
+	cpu_np = of_cpu_device_node_get(policy->cpu);
+	if (!cpu_np)
+		return -ENODEV;
+
+	ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
+					 "#freq-domain-cells", 0, &args);
+	of_node_put(cpu_np);
+	if (ret)
+		return ret;
 
-	c = qcom_freq_domain_map[policy->cpu];
+	c = &freq_domain_map[args.args[0]];
 	if (!c) {
 		pr_err("No scaling support for CPU%d\n", policy->cpu);
 		return -ENODEV;
@@ -171,43 +188,15 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev, struct cpufreq_qcom *c,
 	return 0;
 }
 
-static void 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);
-	}
-}
-
 static int qcom_cpu_resources_init(struct platform_device *pdev,
-				   unsigned int cpu, int index,
-				   unsigned long xo_rate,
-				   unsigned long cpu_hw_rate)
+				   int index, unsigned long xo_rate,
+				   unsigned long cpu_hw_rate,
+				   struct cpufreq_qcom *c)
 {
-	struct cpufreq_qcom *c;
 	struct resource *res;
 	struct device *dev = &pdev->dev;
 	void __iomem *base;
-	int ret, cpu_r;
-
-	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
-	if (!c)
-		return -ENOMEM;
+	int ret;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
 	base = devm_ioremap_resource(dev, res);
@@ -220,12 +209,6 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
 		return -ENODEV;
 	}
 
-	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 -ENOENT;
-	}
-
 	c->perf_state_reg = base + REG_PERF_STATE;
 
 	ret = qcom_cpufreq_hw_read_lut(dev, c, base, xo_rate, cpu_hw_rate);
@@ -234,9 +217,6 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
 		return ret;
 	}
 
-	for_each_cpu(cpu_r, &c->related_cpus)
-		qcom_freq_domain_map[cpu_r] = c;
-
 	return 0;
 }
 
@@ -245,9 +225,16 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 	struct device_node *cpu_np;
 	struct of_phandle_args args;
 	struct clk *clk;
-	unsigned int cpu;
+	unsigned int cpu, domain;
 	unsigned long xo_rate, cpu_hw_rate;
 	int ret;
+	struct cpufreq_qcom *freq_domain_map, *freq_domain;
+
+	freq_domain_map = devm_kcalloc(&pdev->dev, MAX_FREQ_DOMAINS,
+				       sizeof(*freq_domain_map), GFP_KERNEL);
+	cpufreq_qcom_hw_driver.driver_data = freq_domain_map;
+	if (!freq_domain_map)
+		return -ENOMEM;
 
 	clk = clk_get(&pdev->dev, "xo");
 	if (IS_ERR(clk))
@@ -273,16 +260,26 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 
 		ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
 						 "#freq-domain-cells", 0,
-						  &args);
+						 &args);
 		of_node_put(cpu_np);
 		if (ret)
 			return ret;
 
-		if (qcom_freq_domain_map[cpu])
+		domain = args.args[0];
+		if (WARN_ON(domain >= MAX_FREQ_DOMAINS))
+			continue;
+
+		freq_domain = &freq_domain_map[domain];
+		cpumask_set_cpu(cpu, &freq_domain->related_cpus);
+		/*
+		 * If we've already populated the frequency table for this domain
+		 * just mark it related and get out of here
+		 */
+		if (cpumask_weight(&freq_domain->related_cpus) > 1)
 			continue;
 
-		ret = qcom_cpu_resources_init(pdev, cpu, args.args[0],
-					      xo_rate, cpu_hw_rate);
+		ret = qcom_cpu_resources_init(pdev, domain, xo_rate,
+					      cpu_hw_rate, freq_domain);
 		if (ret)
 			return ret;
 	}

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
  2018-12-04 23:05       ` Stephen Boyd
@ 2018-12-05  3:37         ` Taniya Das
  2018-12-05  6:16           ` Viresh Kumar
  2018-12-05  8:07           ` Stephen Boyd
  0 siblings, 2 replies; 21+ messages in thread
From: Taniya Das @ 2018-12-05  3:37 UTC (permalink / raw)
  To: Stephen Boyd, Viresh Kumar
  Cc: Rafael J. Wysocki, linux-kernel, linux-pm, Rajendra Nayak,
	devicetree, robh, skannan, linux-arm-msm, evgreen

Hello Stephen, Viresh

Thanks for the code and suggestions.

Having a NR_DOMAINS '2' makes the driver not scalable for re-use.
This assumption is only true for the current version of the HW and do 
not intend to update/clean-up this logic again. So want to stick keeping 
current logic of having the *qcom_freq_domain_map[NR_CPUS].

On 12/5/2018 4:35 AM, Stephen Boyd wrote:
> Quoting Stephen Boyd (2018-12-04 14:28:11)
>> Quoting Viresh Kumar (2018-12-03 21:12:31)
>>> Hi Taniya,
>>>
>>> Sorry that I haven't been reviewing it much from last few iterations as I was
>>> letting others get this into a better shape. Thanks for your efforts..
>>>
>>> On 02-12-18, 09:25, Taniya Das wrote:
>>>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>>>
>>>> +struct cpufreq_qcom {
>>>> +     struct cpufreq_frequency_table *table;
>>>> +     void __iomem *perf_state_reg;
>>>> +     cpumask_t related_cpus;
>>>> +};
>>>> +
>>>> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
>>>
>>> Now that the code is much more simplified, I am not sure if you need this
>>> per-cpu structure at all. The only place where you are using it is in
>>> qcom_cpufreq_hw_cpu_init() and probe(). Why not merge qcom_cpu_resources_init()
>>> completely into qcom_cpufreq_hw_cpu_init() and get rid of this structure
>>> entirely ?
>>>
>>
>> Good point. Here's an untested patch to handle that. It removes the
>> related functionality and makes an array of pointers to the domains that
>> are created each time qcom_cpu_resources_init() is called.
>>
>> ---8<----
>>
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>> index 8dc6b73c2f22..04e7cfd70316 100644
>> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -23,14 +23,14 @@
>>   #define REG_LUT_TABLE                  0x110
>>   #define REG_PERF_STATE                 0x920
>>   
>> +#define MAX_FREQ_DOMAINS               2
>> +
>>   struct cpufreq_qcom {
>>          struct cpufreq_frequency_table *table;
>>          void __iomem *perf_state_reg;
>>          cpumask_t related_cpus;
>>   };
>>   
>> -static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
>> -
>>   static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
>>                                          unsigned int index)
>>   {
>> @@ -76,9 +76,14 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
>>   
>>   static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>>   {
>> +       struct cpufreq_qcom **freq_domain_map;
>>          struct cpufreq_qcom *c;
>>   
>> -       c = qcom_freq_domain_map[policy->cpu];
>> +       freq_domain_map = cpufreq_get_driver_data();
>> +       if (!freq_domain_map)
>> +               return -ENODEV;
>> +
>> +       c = freq_domain_map[policy->cpu];
> 
> And this fails now because it indexes based on cpu number. We have to
> parse the frequency domain out of the cpunode again to fix that.
> 
> Here's the updated (still untested) patch and I also just allocated the
> freq domain array up front instead of in each domain init routine to
> simplify some more.
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index 8dc6b73c2f22..bc0d734f7e3c 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -23,14 +23,14 @@
>   #define REG_LUT_TABLE			0x110
>   #define REG_PERF_STATE			0x920
>   
> +#define MAX_FREQ_DOMAINS		2
> +
>   struct cpufreq_qcom {
>   	struct cpufreq_frequency_table *table;
>   	void __iomem *perf_state_reg;
>   	cpumask_t related_cpus;
>   };
>   
> -static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> -
>   static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
>   					unsigned int index)
>   {
> @@ -76,9 +76,26 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
>   
>   static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>   {
> -	struct cpufreq_qcom *c;
> +	struct cpufreq_qcom *freq_domain_map, *c;
> +	struct device_node *cpu_np;
> +	struct of_phandle_args args;
> +	int ret;
> +
> +	freq_domain_map = cpufreq_get_driver_data();
> +	if (!freq_domain_map)
> +		return -ENODEV;
> +
> +	cpu_np = of_cpu_device_node_get(policy->cpu);
> +	if (!cpu_np)
> +		return -ENODEV;
> +
> +	ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
> +					 "#freq-domain-cells", 0, &args);
> +	of_node_put(cpu_np);
> +	if (ret)
> +		return ret;
>   
> -	c = qcom_freq_domain_map[policy->cpu];
> +	c = &freq_domain_map[args.args[0]];
>   	if (!c) {
>   		pr_err("No scaling support for CPU%d\n", policy->cpu);
>   		return -ENODEV;
> @@ -171,43 +188,15 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev, struct cpufreq_qcom *c,
>   	return 0;
>   }
>   
> -static void 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);
> -	}
> -}
> -
>   static int qcom_cpu_resources_init(struct platform_device *pdev,
> -				   unsigned int cpu, int index,
> -				   unsigned long xo_rate,
> -				   unsigned long cpu_hw_rate)
> +				   int index, unsigned long xo_rate,
> +				   unsigned long cpu_hw_rate,
> +				   struct cpufreq_qcom *c)
>   {
> -	struct cpufreq_qcom *c;
>   	struct resource *res;
>   	struct device *dev = &pdev->dev;
>   	void __iomem *base;
> -	int ret, cpu_r;
> -
> -	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> -	if (!c)
> -		return -ENOMEM;
> +	int ret;
>   
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
>   	base = devm_ioremap_resource(dev, res);
> @@ -220,12 +209,6 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
>   		return -ENODEV;
>   	}
>   
> -	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 -ENOENT;
> -	}
> -
>   	c->perf_state_reg = base + REG_PERF_STATE;
>   
>   	ret = qcom_cpufreq_hw_read_lut(dev, c, base, xo_rate, cpu_hw_rate);
> @@ -234,9 +217,6 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
>   		return ret;
>   	}
>   
> -	for_each_cpu(cpu_r, &c->related_cpus)
> -		qcom_freq_domain_map[cpu_r] = c;
> -
>   	return 0;
>   }
>   
> @@ -245,9 +225,16 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
>   	struct device_node *cpu_np;
>   	struct of_phandle_args args;
>   	struct clk *clk;
> -	unsigned int cpu;
> +	unsigned int cpu, domain;
>   	unsigned long xo_rate, cpu_hw_rate;
>   	int ret;
> +	struct cpufreq_qcom *freq_domain_map, *freq_domain;
> +
> +	freq_domain_map = devm_kcalloc(&pdev->dev, MAX_FREQ_DOMAINS,
> +				       sizeof(*freq_domain_map), GFP_KERNEL);
> +	cpufreq_qcom_hw_driver.driver_data = freq_domain_map;
> +	if (!freq_domain_map)
> +		return -ENOMEM;
>   
>   	clk = clk_get(&pdev->dev, "xo");
>   	if (IS_ERR(clk))
> @@ -273,16 +260,26 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
>   
>   		ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
>   						 "#freq-domain-cells", 0,
> -						  &args);
> +						 &args);
>   		of_node_put(cpu_np);
>   		if (ret)
>   			return ret;
>   
> -		if (qcom_freq_domain_map[cpu])
> +		domain = args.args[0];
> +		if (WARN_ON(domain >= MAX_FREQ_DOMAINS))
> +			continue;
> +
> +		freq_domain = &freq_domain_map[domain];
> +		cpumask_set_cpu(cpu, &freq_domain->related_cpus);
> +		/*
> +		 * If we've already populated the frequency table for this domain
> +		 * just mark it related and get out of here
> +		 */
> +		if (cpumask_weight(&freq_domain->related_cpus) > 1)
>   			continue;
>   
> -		ret = qcom_cpu_resources_init(pdev, cpu, args.args[0],
> -					      xo_rate, cpu_hw_rate);
> +		ret = qcom_cpu_resources_init(pdev, domain, xo_rate,
> +					      cpu_hw_rate, freq_domain);
>   		if (ret)
>   			return ret;
>   	}
> 

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

--

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
  2018-12-05  3:37         ` Taniya Das
@ 2018-12-05  6:16           ` Viresh Kumar
  2018-12-05 17:00             ` Stephen Boyd
  2018-12-11 13:35             ` Taniya Das
  2018-12-05  8:07           ` Stephen Boyd
  1 sibling, 2 replies; 21+ messages in thread
From: Viresh Kumar @ 2018-12-05  6:16 UTC (permalink / raw)
  To: Taniya Das
  Cc: Stephen Boyd, Rafael J. Wysocki, linux-kernel, linux-pm,
	Rajendra Nayak, devicetree, robh, skannan, linux-arm-msm,
	evgreen

On 05-12-18, 09:07, Taniya Das wrote:
> Hello Stephen, Viresh
> 
> Thanks for the code and suggestions.
> 
> Having a NR_DOMAINS '2' makes the driver not scalable for re-use.

Sure, I didn't like it either and that wasn't really what I was suggesting in
the first place. I didn't wanted to write the code myself and pass it on, but I
have it now anyway :)

It may have a bug or two in there, but compiles just fine and is rebased over
your patch Taniya.

-- 
viresh

-------------------------8<-------------------------

 drivers/cpufreq/qcom-cpufreq-hw.c | 156 +++++++++++-------------------
 1 file changed, 57 insertions(+), 99 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 8dc6b73c2f22..bcf9bb37298a 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -23,13 +23,8 @@
 #define REG_LUT_TABLE			0x110
 #define REG_PERF_STATE			0x920
 
-struct cpufreq_qcom {
-	struct cpufreq_frequency_table *table;
-	void __iomem *perf_state_reg;
-	cpumask_t related_cpus;
-};
-
-static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
+static unsigned long cpu_hw_rate, xo_rate;
+static struct platform_device *global_pdev;
 
 static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
 					unsigned int index)
@@ -74,53 +69,17 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
 	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->perf_state_reg;
-
-	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,
-};
-
-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)
+static int qcom_cpufreq_hw_read_lut(struct device *dev,
+				    struct cpufreq_policy *policy,
+				    void __iomem *base)
 {
 	u32 data, src, lval, i, core_count, prev_cc = 0, prev_freq = 0, freq;
-	unsigned int max_cores = cpumask_weight(&c->related_cpus);
+	unsigned int max_cores = cpumask_weight(policy->cpus);
+	struct cpufreq_frequency_table	*table;
 
-	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
-				sizeof(*c->table), GFP_KERNEL);
-	if (!c->table)
+	table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
+				sizeof(*table), GFP_KERNEL);
+	if (!table)
 		return -ENOMEM;
 
 	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
@@ -136,9 +95,9 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev, struct cpufreq_qcom *c,
 
 		/* Ignore boosts in the middle of the table */
 		if (core_count != max_cores) {
-			c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
+			table[i].frequency = CPUFREQ_ENTRY_INVALID;
 		} else {
-			c->table[i].frequency = freq;
+			table[i].frequency = freq;
 			dev_dbg(dev, "index=%d freq=%d, core_count %d\n", i,
 				freq, core_count);
 		}
@@ -148,7 +107,7 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev, struct cpufreq_qcom *c,
 		 * end of table
 		 */
 		if (i > 0 && prev_freq == freq && prev_cc == core_count) {
-			struct cpufreq_frequency_table *prev = &c->table[i - 1];
+			struct cpufreq_frequency_table *prev = &table[i - 1];
 
 			/*
 			 * Only treat the last frequency that might be a boost
@@ -166,7 +125,8 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev, struct cpufreq_qcom *c,
 		prev_freq = freq;
 	}
 
-	c->table[i].frequency = CPUFREQ_TABLE_END;
+	table[i].frequency = CPUFREQ_TABLE_END;
+	policy->freq_table = table;
 
 	return 0;
 }
@@ -194,22 +154,29 @@ static void qcom_get_related_cpus(int index, struct cpumask *m)
 	}
 }
 
-static int qcom_cpu_resources_init(struct platform_device *pdev,
-				   unsigned int cpu, int index,
-				   unsigned long xo_rate,
-				   unsigned long cpu_hw_rate)
+static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 {
-	struct cpufreq_qcom *c;
+	struct device *dev = &global_pdev->dev;
+	struct of_phandle_args args;
+	struct device_node *cpu_np;
 	struct resource *res;
-	struct device *dev = &pdev->dev;
 	void __iomem *base;
-	int ret, cpu_r;
+	int ret, index;
 
-	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
-	if (!c)
-		return -ENOMEM;
+	cpu_np = of_cpu_device_node_get(policy->cpu);
+	if (!cpu_np)
+		return -EINVAL;
+
+	ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
+			"#freq-domain-cells", 0, &args);
+	of_node_put(cpu_np);
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
+	if (ret)
+		return ret;
+
+	index = args.args[0];
+
+	res = platform_get_resource(global_pdev, IORESOURCE_MEM, index);
 	base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
@@ -220,33 +187,46 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
 		return -ENODEV;
 	}
 
-	qcom_get_related_cpus(index, &c->related_cpus);
-	if (!cpumask_weight(&c->related_cpus)) {
+	qcom_get_related_cpus(index, policy->cpus);
+	if (!cpumask_weight(policy->cpus)) {
 		dev_err(dev, "Domain-%d failed to get related CPUs\n", index);
 		return -ENOENT;
 	}
 
-	c->perf_state_reg = base + REG_PERF_STATE;
+	policy->driver_data = base + REG_PERF_STATE;
 
-	ret = qcom_cpufreq_hw_read_lut(dev, c, base, xo_rate, cpu_hw_rate);
+	ret = qcom_cpufreq_hw_read_lut(dev, policy, 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;
+	policy->fast_switch_possible = true;
 
 	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,
+};
+
 static int qcom_cpufreq_hw_driver_probe(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");
@@ -263,29 +243,7 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 	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)
-			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;
-	}
+	global_pdev = pdev;
 
 	ret = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
 	if (ret) {

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
  2018-12-05  3:37         ` Taniya Das
  2018-12-05  6:16           ` Viresh Kumar
@ 2018-12-05  8:07           ` Stephen Boyd
  1 sibling, 0 replies; 21+ messages in thread
From: Stephen Boyd @ 2018-12-05  8:07 UTC (permalink / raw)
  To: Taniya Das, Viresh Kumar
  Cc: Rafael J. Wysocki, linux-kernel, linux-pm, Rajendra Nayak,
	devicetree, robh, skannan, linux-arm-msm, evgreen

Quoting Taniya Das (2018-12-04 19:37:00)
> Hello Stephen, Viresh
> 
> Thanks for the code and suggestions.
> 
> Having a NR_DOMAINS '2' makes the driver not scalable for re-use.
> This assumption is only true for the current version of the HW and do 
> not intend to update/clean-up this logic again. So want to stick keeping 
> current logic of having the *qcom_freq_domain_map[NR_CPUS].

Ok, so let's just count the number of reg properties there are in the
node and use that to know how many frequency domains exist. That already
maps exactly to how many frequency domains this driver needs to worry
about. Alternatively, we could count the number of named register
regions like 'freq_domain0', 'freq_domain1', etc. are listed in
reg-names so that other register regions in this device could be exposed
in DT when/if they are added. Here's the reg counting patch.

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 8dc6b73c2f22..c0f6a07eb3c5 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -29,8 +29,6 @@ struct cpufreq_qcom {
 	cpumask_t related_cpus;
 };
 
-static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
-
 static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
 					unsigned int index)
 {
@@ -76,9 +74,26 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
 
 static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 {
-	struct cpufreq_qcom *c;
+	struct cpufreq_qcom *freq_domains, *c;
+	struct device_node *cpu_np;
+	struct of_phandle_args args;
+	int ret;
+
+	freq_domains = cpufreq_get_driver_data();
+	if (!freq_domains)
+		return -ENODEV;
+
+	cpu_np = of_cpu_device_node_get(policy->cpu);
+	if (!cpu_np)
+		return -ENODEV;
+
+	ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
+					 "#freq-domain-cells", 0, &args);
+	of_node_put(cpu_np);
+	if (ret)
+		return ret;
 
-	c = qcom_freq_domain_map[policy->cpu];
+	c = &freq_domains[args.args[0]];
 	if (!c) {
 		pr_err("No scaling support for CPU%d\n", policy->cpu);
 		return -ENODEV;
@@ -171,43 +186,15 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev, struct cpufreq_qcom *c,
 	return 0;
 }
 
-static void 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);
-	}
-}
-
 static int qcom_cpu_resources_init(struct platform_device *pdev,
-				   unsigned int cpu, int index,
-				   unsigned long xo_rate,
-				   unsigned long cpu_hw_rate)
+				   int index, unsigned long xo_rate,
+				   unsigned long cpu_hw_rate,
+				   struct cpufreq_qcom *c)
 {
-	struct cpufreq_qcom *c;
 	struct resource *res;
 	struct device *dev = &pdev->dev;
 	void __iomem *base;
-	int ret, cpu_r;
-
-	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
-	if (!c)
-		return -ENOMEM;
+	int ret;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
 	base = devm_ioremap_resource(dev, res);
@@ -220,12 +207,6 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
 		return -ENODEV;
 	}
 
-	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 -ENOENT;
-	}
-
 	c->perf_state_reg = base + REG_PERF_STATE;
 
 	ret = qcom_cpufreq_hw_read_lut(dev, c, base, xo_rate, cpu_hw_rate);
@@ -234,9 +215,6 @@ static int qcom_cpu_resources_init(struct platform_device *pdev,
 		return ret;
 	}
 
-	for_each_cpu(cpu_r, &c->related_cpus)
-		qcom_freq_domain_map[cpu_r] = c;
-
 	return 0;
 }
 
@@ -245,9 +223,20 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 	struct device_node *cpu_np;
 	struct of_phandle_args args;
 	struct clk *clk;
-	unsigned int cpu;
+	unsigned int cpu, domain, max_domains;
 	unsigned long xo_rate, cpu_hw_rate;
 	int ret;
+	struct cpufreq_qcom *freq_domains, *freq_domain;
+
+	max_domains = 0;
+	while (platform_get_resource(pdev, IORESOURCE_MEM, max_domains))
+		max_domains++;
+
+	freq_domains = devm_kcalloc(&pdev->dev, max_domains,
+				    sizeof(*freq_domains), GFP_KERNEL);
+	cpufreq_qcom_hw_driver.driver_data = freq_domains;
+	if (!freq_domains)
+		return -ENOMEM;
 
 	clk = clk_get(&pdev->dev, "xo");
 	if (IS_ERR(clk))
@@ -273,16 +262,26 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 
 		ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
 						 "#freq-domain-cells", 0,
-						  &args);
+						 &args);
 		of_node_put(cpu_np);
 		if (ret)
 			return ret;
 
-		if (qcom_freq_domain_map[cpu])
+		domain = args.args[0];
+		if (WARN_ON(domain >= max_domains))
+			continue;
+
+		freq_domain = &freq_domains[domain];
+		cpumask_set_cpu(cpu, &freq_domain->related_cpus);
+		/*
+		 * If we've already populated the frequency table for this domain
+		 * just mark it related and get out of here
+		 */
+		if (cpumask_weight(&freq_domain->related_cpus) > 1)
 			continue;
 
-		ret = qcom_cpu_resources_init(pdev, cpu, args.args[0],
-					      xo_rate, cpu_hw_rate);
+		ret = qcom_cpu_resources_init(pdev, domain, xo_rate,
+					      cpu_hw_rate, freq_domain);
 		if (ret)
 			return ret;
 	}

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
  2018-12-05  6:16           ` Viresh Kumar
@ 2018-12-05 17:00             ` Stephen Boyd
  2018-12-06  4:22               ` Viresh Kumar
  2018-12-11 13:35             ` Taniya Das
  1 sibling, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2018-12-05 17:00 UTC (permalink / raw)
  To: Taniya Das, Viresh Kumar
  Cc: Rafael J. Wysocki, linux-kernel, linux-pm, Rajendra Nayak,
	devicetree, robh, skannan, linux-arm-msm, evgreen

Quoting Viresh Kumar (2018-12-04 22:16:00)
> On 05-12-18, 09:07, Taniya Das wrote:
> > Hello Stephen, Viresh
> > 
> > Thanks for the code and suggestions.
> > 
> > Having a NR_DOMAINS '2' makes the driver not scalable for re-use.
> 
> Sure, I didn't like it either and that wasn't really what I was suggesting in
> the first place. I didn't wanted to write the code myself and pass it on, but I
> have it now anyway :)
> 
> It may have a bug or two in there, but compiles just fine and is rebased over
> your patch Taniya.

If we move the ioremap of registers to the cpufreq_driver::init path
then we need to unmap it on cpufreq_driver::exit. In fact, all the
devm_*() code in the init path needs to change to non-devm. Otherwise
it's a nice simplification!

> +static int qcom_cpufreq_hw_read_lut(struct device *dev,
> +                                   struct cpufreq_policy *policy,
> +                                   void __iomem *base)
>  {
>         u32 data, src, lval, i, core_count, prev_cc = 0, prev_freq = 0, freq;
> -       unsigned int max_cores = cpumask_weight(&c->related_cpus);
> +       unsigned int max_cores = cpumask_weight(policy->cpus);
> +       struct cpufreq_frequency_table  *table;
>  
> -       c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> -                               sizeof(*c->table), GFP_KERNEL);
> -       if (!c->table)
> +       table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> +                               sizeof(*table), GFP_KERNEL);

This one.

> +       if (!table)
>                 return -ENOMEM;
>  
>         for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> @@ -194,22 +154,29 @@ static void qcom_get_related_cpus(int index, struct cpumask *m)
>         }
>  }
>  
> -static int qcom_cpu_resources_init(struct platform_device *pdev,
> -                                  unsigned int cpu, int index,
> -                                  unsigned long xo_rate,
> -                                  unsigned long cpu_hw_rate)
> +static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  {
> -       struct cpufreq_qcom *c;
> +       struct device *dev = &global_pdev->dev;
> +       struct of_phandle_args args;
> +       struct device_node *cpu_np;
>         struct resource *res;
> -       struct device *dev = &pdev->dev;
>         void __iomem *base;
> -       int ret, cpu_r;
> +       int ret, index;
>  
> -       c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> -       if (!c)
> -               return -ENOMEM;
> +       cpu_np = of_cpu_device_node_get(policy->cpu);
> +       if (!cpu_np)
> +               return -EINVAL;
> +
> +       ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
> +                       "#freq-domain-cells", 0, &args);
> +       of_node_put(cpu_np);
>  
> -       res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> +       if (ret)
> +               return ret;
> +
> +       index = args.args[0];
> +
> +       res = platform_get_resource(global_pdev, IORESOURCE_MEM, index);
>         base = devm_ioremap_resource(dev, res);

And this one.

>         if (IS_ERR(base))
>                 return PTR_ERR(base);

Here's a patch to squash in to fix those tiny problems. I left it as
devm_ioremap_resource() because that has all the nice features of
resource requesting inside and it didn't seem too bad to devm_iounmap()
on the exit path.

---------8<--------------

 drivers/cpufreq/qcom-cpufreq-hw.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index bcf9bb37298a..1e865e216839 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
+#include <linux/slab.h>
 
 #define LUT_MAX_ENTRIES			40U
 #define LUT_SRC				GENMASK(31, 30)
@@ -77,8 +78,7 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,
 	unsigned int max_cores = cpumask_weight(policy->cpus);
 	struct cpufreq_frequency_table	*table;
 
-	table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
-				sizeof(*table), GFP_KERNEL);
+	table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
 	if (!table)
 		return -ENOMEM;
 
@@ -144,7 +144,7 @@ static void qcom_get_related_cpus(int index, struct cpumask *m)
 
 		ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
 						 "#freq-domain-cells", 0,
-						  &args);
+						 &args);
 		of_node_put(cpu_np);
 		if (ret < 0)
 			continue;
@@ -170,7 +170,6 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 	ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
 			"#freq-domain-cells", 0, &args);
 	of_node_put(cpu_np);
-
 	if (ret)
 		return ret;
 
@@ -184,13 +183,15 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 	/* 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 = -ENODEV;
+		goto error;
 	}
 
 	qcom_get_related_cpus(index, policy->cpus);
 	if (!cpumask_weight(policy->cpus)) {
 		dev_err(dev, "Domain-%d failed to get related CPUs\n", index);
-		return -ENOENT;
+		ret = -ENOENT;
+		goto error;
 	}
 
 	policy->driver_data = base + REG_PERF_STATE;
@@ -198,11 +199,25 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 	ret = qcom_cpufreq_hw_read_lut(dev, policy, base);
 	if (ret) {
 		dev_err(dev, "Domain-%d failed to read LUT\n", index);
-		return ret;
+		goto error;
 	}
 
 	policy->fast_switch_possible = true;
 
+	return 0;
+
+error:
+	devm_iounmap(dev, base);
+	return ret;
+}
+
+static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
+{
+	void __iomem *base = policy->driver_data - REG_PERF_STATE;
+
+	kfree(policy->freq_table);
+	devm_iounmap(&global_pdev->dev, base);
+
 	return 0;
 }
 
@@ -219,6 +234,7 @@ static struct cpufreq_driver cpufreq_qcom_hw_driver = {
 	.target_index	= qcom_cpufreq_hw_target_index,
 	.get		= qcom_cpufreq_hw_get,
 	.init		= qcom_cpufreq_hw_cpu_init,
+	.exit		= qcom_cpufreq_hw_cpu_exit,
 	.fast_switch    = qcom_cpufreq_hw_fast_switch,
 	.name		= "qcom-cpufreq-hw",
 	.attr		= qcom_cpufreq_hw_attr,
@@ -248,12 +264,11 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 	ret = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
 	if (ret) {
 		dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
-		return ret;
+	} else {
+		dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
 	}
 
-	dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
-
-	return 0;
+	return ret;
 }
 
 static int qcom_cpufreq_hw_driver_remove(struct platform_device *pdev)

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
  2018-12-05 17:00             ` Stephen Boyd
@ 2018-12-06  4:22               ` Viresh Kumar
  0 siblings, 0 replies; 21+ messages in thread
From: Viresh Kumar @ 2018-12-06  4:22 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Taniya Das, Rafael J. Wysocki, linux-kernel, linux-pm,
	Rajendra Nayak, devicetree, robh, skannan, linux-arm-msm,
	evgreen

On 05-12-18, 09:00, Stephen Boyd wrote:
> Here's a patch to squash in to fix those tiny problems. I left it as
> devm_ioremap_resource() because that has all the nice features of
> resource requesting inside and it didn't seem too bad to devm_iounmap()
> on the exit path.
> 
> ---------8<--------------
> 
>  drivers/cpufreq/qcom-cpufreq-hw.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index bcf9bb37298a..1e865e216839 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -10,6 +10,7 @@
>  #include <linux/module.h>
>  #include <linux/of_address.h>
>  #include <linux/of_platform.h>
> +#include <linux/slab.h>
>  
>  #define LUT_MAX_ENTRIES			40U
>  #define LUT_SRC				GENMASK(31, 30)
> @@ -77,8 +78,7 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,
>  	unsigned int max_cores = cpumask_weight(policy->cpus);
>  	struct cpufreq_frequency_table	*table;
>  
> -	table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> -				sizeof(*table), GFP_KERNEL);
> +	table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
>  	if (!table)
>  		return -ENOMEM;
>  
> @@ -144,7 +144,7 @@ static void qcom_get_related_cpus(int index, struct cpumask *m)
>  
>  		ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
>  						 "#freq-domain-cells", 0,
> -						  &args);
> +						 &args);
>  		of_node_put(cpu_np);
>  		if (ret < 0)
>  			continue;
> @@ -170,7 +170,6 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  	ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
>  			"#freq-domain-cells", 0, &args);
>  	of_node_put(cpu_np);
> -
>  	if (ret)
>  		return ret;
>  
> @@ -184,13 +183,15 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  	/* 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 = -ENODEV;
> +		goto error;
>  	}
>  
>  	qcom_get_related_cpus(index, policy->cpus);
>  	if (!cpumask_weight(policy->cpus)) {
>  		dev_err(dev, "Domain-%d failed to get related CPUs\n", index);
> -		return -ENOENT;
> +		ret = -ENOENT;
> +		goto error;
>  	}
>  
>  	policy->driver_data = base + REG_PERF_STATE;
> @@ -198,11 +199,25 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  	ret = qcom_cpufreq_hw_read_lut(dev, policy, base);
>  	if (ret) {
>  		dev_err(dev, "Domain-%d failed to read LUT\n", index);
> -		return ret;
> +		goto error;
>  	}
>  
>  	policy->fast_switch_possible = true;
>  
> +	return 0;
> +
> +error:
> +	devm_iounmap(dev, base);
> +	return ret;
> +}
> +
> +static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
> +{
> +	void __iomem *base = policy->driver_data - REG_PERF_STATE;
> +
> +	kfree(policy->freq_table);
> +	devm_iounmap(&global_pdev->dev, base);
> +
>  	return 0;
>  }
>  
> @@ -219,6 +234,7 @@ static struct cpufreq_driver cpufreq_qcom_hw_driver = {
>  	.target_index	= qcom_cpufreq_hw_target_index,
>  	.get		= qcom_cpufreq_hw_get,
>  	.init		= qcom_cpufreq_hw_cpu_init,
> +	.exit		= qcom_cpufreq_hw_cpu_exit,
>  	.fast_switch    = qcom_cpufreq_hw_fast_switch,
>  	.name		= "qcom-cpufreq-hw",
>  	.attr		= qcom_cpufreq_hw_attr,
> @@ -248,12 +264,11 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
>  	ret = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
>  	if (ret) {
>  		dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
> -		return ret;
> +	} else {
> +		dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
>  	}
>  
> -	dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
> -
> -	return 0;
> +	return ret;
>  }
>  
>  static int qcom_cpufreq_hw_driver_remove(struct platform_device *pdev)

LGTM

-- 
viresh

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
  2018-12-05  6:16           ` Viresh Kumar
  2018-12-05 17:00             ` Stephen Boyd
@ 2018-12-11 13:35             ` Taniya Das
  2018-12-12  4:47               ` Viresh Kumar
  1 sibling, 1 reply; 21+ messages in thread
From: Taniya Das @ 2018-12-11 13:35 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Stephen Boyd, Rafael J. Wysocki, linux-kernel, linux-pm,
	Rajendra Nayak, devicetree, robh, skannan, linux-arm-msm,
	evgreen

Hi Viresh,

Sorry for the delayed response. Thanks for the patch.

On 12/5/2018 11:46 AM, Viresh Kumar wrote:
> On 05-12-18, 09:07, Taniya Das wrote:
>> Hello Stephen, Viresh
>>
>> Thanks for the code and suggestions.
>>
>> Having a NR_DOMAINS '2' makes the driver not scalable for re-use.
> 
> Sure, I didn't like it either and that wasn't really what I was suggesting in
> the first place. I didn't wanted to write the code myself and pass it on, but I
> have it now anyway :)
> 
> It may have a bug or two in there, but compiles just fine and is rebased over
> your patch Taniya.
> 

The design here assumes that there would not be any per-cpu/per-cluster 
based SW requirement for the HW during frequency transitions, which 
again makes me think that we would require to re-introduce these 
structures again in case we have such requirements in near future.

Also I think leaving the structures also helps us debug any boot up 
issues looking at the ram contents of the per-(cpu/cluster) structures 
with the contents from the firmware.

Hope these above helps us to go ahead with the current SW design.

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

--

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
  2018-12-11 13:35             ` Taniya Das
@ 2018-12-12  4:47               ` Viresh Kumar
  2018-12-13  7:48                 ` Taniya Das
  0 siblings, 1 reply; 21+ messages in thread
From: Viresh Kumar @ 2018-12-12  4:47 UTC (permalink / raw)
  To: Taniya Das
  Cc: Stephen Boyd, Rafael J. Wysocki, linux-kernel, linux-pm,
	Rajendra Nayak, devicetree, robh, skannan, linux-arm-msm,
	evgreen

On 11-12-18, 19:05, Taniya Das wrote:
> The design here assumes that there would not be any per-cpu/per-cluster
> based SW requirement for the HW during frequency transitions, which again
> makes me think that we would require to re-introduce these structures again
> in case we have such requirements in near future.

Firstly, even in such cases we can go ahead with the design we proposed. And I
am not at all concerned about some hardware which we don't have right now. We
will see what to do when such hardware comes, maybe reintroduce the structures,
but that doesn't matter right now.

> Also I think leaving the structures also helps us debug any boot up issues
> looking at the ram contents of the per-(cpu/cluster) structures with the
> contents from the firmware.

I don't see how debugging would be hard without those structures in place.

> Hope these above helps us to go ahead with the current SW design.

Sorry, but I don't see any satisfactory reason on why you shouldn't make the
suggested changes. We are trying to make your (and any other developer who will
work on that driver) life simple by simplifying the code. Nothing beyond that :)

-- 
viresh

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
  2018-12-12  4:47               ` Viresh Kumar
@ 2018-12-13  7:48                 ` Taniya Das
  0 siblings, 0 replies; 21+ messages in thread
From: Taniya Das @ 2018-12-13  7:48 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Stephen Boyd, Rafael J. Wysocki, linux-kernel, linux-pm,
	Rajendra Nayak, devicetree, robh, skannan, linux-arm-msm,
	evgreen

Hello Viresh,

On 12/12/2018 10:17 AM, Viresh Kumar wrote:
> On 11-12-18, 19:05, Taniya Das wrote:
>> The design here assumes that there would not be any per-cpu/per-cluster
>> based SW requirement for the HW during frequency transitions, which again
>> makes me think that we would require to re-introduce these structures again
>> in case we have such requirements in near future.
> 
> Firstly, even in such cases we can go ahead with the design we proposed. And I
> am not at all concerned about some hardware which we don't have right now. We
> will see what to do when such hardware comes, maybe reintroduce the structures,
> but that doesn't matter right now.
> 
>> Also I think leaving the structures also helps us debug any boot up issues
>> looking at the ram contents of the per-(cpu/cluster) structures with the
>> contents from the firmware.
> 
> I don't see how debugging would be hard without those structures in place.
> 
>> Hope these above helps us to go ahead with the current SW design.
> 
> Sorry, but I don't see any satisfactory reason on why you shouldn't make the
> suggested changes. We are trying to make your (and any other developer who will
> work on that driver) life simple by simplifying the code. Nothing beyond that :)
> 

Sure, Thanks, will submit the next patch series for your ACK :).

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

--

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2018-12-13  7:48 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-02  3:55 [PATCH v11 0/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW Taniya Das
2018-12-02  3:55 ` [PATCH v11 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings Taniya Das
2018-12-03 16:55   ` Stephen Boyd
2018-12-03 23:09   ` Rob Herring
2018-12-03 23:57     ` Stephen Boyd
2018-12-02  3:55 ` [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver Taniya Das
2018-12-03 16:57   ` Stephen Boyd
2018-12-03 17:46   ` Matthias Kaehlcke
2018-12-04  5:12   ` Viresh Kumar
2018-12-04  9:27     ` Taniya Das
2018-12-04  9:29       ` Viresh Kumar
2018-12-04 22:28     ` Stephen Boyd
2018-12-04 23:05       ` Stephen Boyd
2018-12-05  3:37         ` Taniya Das
2018-12-05  6:16           ` Viresh Kumar
2018-12-05 17:00             ` Stephen Boyd
2018-12-06  4:22               ` Viresh Kumar
2018-12-11 13:35             ` Taniya Das
2018-12-12  4:47               ` Viresh Kumar
2018-12-13  7:48                 ` Taniya Das
2018-12-05  8:07           ` Stephen Boyd

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).