LKML Archive on lore.kernel.org
 help / Atom feed
* [PATCH v4 0/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver
@ 2018-06-12 11:02 Taniya Das
  2018-06-12 11:02 ` [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings Taniya Das
  2018-06-12 11:02 ` [PATCH v4 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver Taniya Das
  0 siblings, 2 replies; 25+ messages in thread
From: Taniya Das @ 2018-06-12 11:02 UTC (permalink / raw)
  To: Rafael J. Wysocki, Viresh Kumar, linux-kernel, linux-pm, Stephen Boyd
  Cc: Rajendra Nayak, devicetree, robh, skannan, Taniya Das

 [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'.
 * Remove initialization of 'ret' from function qcom_resources_init and add
   return -ENODEV based on 'of_get_available_child_count'.
 * Removed initialization of 'rc' from qcom_cpufreq_fw_driver_probe
 * Removed module_exit as this driver would not be used as module, also updated
   the Kconfig to bool from tristate.
 * Updated the subsystem in device tree bindings.

 [v1]
   * Fixed compilation reported by Amit K.

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

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

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


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

* [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-12 11:02 [PATCH v4 0/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver Taniya Das
@ 2018-06-12 11:02 ` Taniya Das
  2018-06-13 11:26   ` Sudeep Holla
  2018-06-15 13:07   ` Amit Kucheria
  2018-06-12 11:02 ` [PATCH v4 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver Taniya Das
  1 sibling, 2 replies; 25+ messages in thread
From: Taniya Das @ 2018-06-12 11:02 UTC (permalink / raw)
  To: Rafael J. Wysocki, Viresh Kumar, linux-kernel, linux-pm, Stephen Boyd
  Cc: Rajendra Nayak, devicetree, robh, skannan, Taniya Das

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

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

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
new file mode 100644
index 0000000..e3087ec
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
@@ -0,0 +1,173 @@
+Qualcomm Technologies, Inc. CPUFREQ Bindings
+
+CPUFREQ FW is a hardware engine used by some Qualcomm Technologies, Inc. (QTI)
+SoCs to manage frequency in hardware. It is capable of controlling frequency
+for multiple clusters.
+
+Properties:
+- compatible
+	Usage:		required
+	Value type:	<string>
+	Definition:	must be "qcom,cpufreq-fw".
+
+* Property qcom,freq-domain
+Devices supporting freq-domain must set their "qcom,freq-domain" property with
+phandle to a freq_domain_table in their DT node.
+
+* Frequency Domain Table Node
+
+This describes the frequency domain belonging to a device.
+This node can have following properties:
+
+- reg
+	Usage:		required
+	Value type:	<prop-encoded-array>
+	Definition:	Addresses and sizes for the memory of the perf
+			, lut and enable bases.
+			perf - indicates the base address for the desired
+			performance state to be set.
+			lut - indicates the look up table base address for the
+			cpufreq	driver to read frequencies.
+			enable - indicates the enable register for firmware.
+- reg-names
+	Usage:		required
+	Value type:	<stringlist>
+	Definition:	Address names. Must be "perf", "lut", "enable".
+			Must be specified in the same order as the reg property.
+
+Example:
+
+Example 1: Dual-cluster, Quad-core per cluster. CPUs within a cluster switch
+DCVS state together.
+
+/ {
+	cpus {
+		#address-cells = <2>;
+		#size-cells = <0>;
+
+		CPU0: cpu@0 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x0>;
+			enable-method = "psci";
+			next-level-cache = <&L2_0>;
+			qcom,freq-domain = <&freq_domain_table0>;
+			L2_0: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+				L3_0: l3-cache {
+				      compatible = "cache";
+				};
+			};
+		};
+
+		CPU1: cpu@100 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x100>;
+			enable-method = "psci";
+			next-level-cache = <&L2_100>;
+			qcom,freq-domain = <&freq_domain_table0>;
+			L2_100: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU2: cpu@200 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x200>;
+			enable-method = "psci";
+			next-level-cache = <&L2_200>;
+			qcom,freq-domain = <&freq_domain_table0>;
+			L2_200: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU3: cpu@300 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x300>;
+			enable-method = "psci";
+			next-level-cache = <&L2_300>;
+			qcom,freq-domain = <&freq_domain_table0>;
+			L2_300: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU4: cpu@400 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x400>;
+			enable-method = "psci";
+			next-level-cache = <&L2_400>;
+			qcom,freq-domain = <&freq_domain_table1>;
+			L2_400: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU5: cpu@500 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x500>;
+			enable-method = "psci";
+			next-level-cache = <&L2_500>;
+			qcom,freq-domain = <&freq_domain_table1>;
+			L2_500: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU6: cpu@600 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x600>;
+			enable-method = "psci";
+			next-level-cache = <&L2_600>;
+			qcom,freq-domain = <&freq_domain_table1>;
+			L2_600: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU7: cpu@700 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x700>;
+			enable-method = "psci";
+			next-level-cache = <&L2_700>;
+			qcom,freq-domain = <&freq_domain_table1>;
+			L2_700: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+	};
+
+	qcom,cpufreq-fw {
+		compatible = "qcom,cpufreq-fw";
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		freq_domain_table0 : freq_table0 {
+			reg = <0x17d43920 0x4>, <0x17d43110 0x500>,
+				 <0x17d41000 0x4>;
+			reg-names = "perf", "lut", "enable";
+		};
+
+		freq_domain_table1 : freq_table1 {
+			reg = <0x17d46120 0x4>, <0x17d45910 0x500>,
+				<0x17d45800 0x4> ;
+			reg-names = "perf", "lut", "enable";
+		};
+	};
-- 
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] 25+ messages in thread

* [PATCH v4 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver
  2018-06-12 11:02 [PATCH v4 0/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver Taniya Das
  2018-06-12 11:02 ` [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings Taniya Das
@ 2018-06-12 11:02 ` Taniya Das
  2018-06-15 12:02   ` Amit Kucheria
                     ` (2 more replies)
  1 sibling, 3 replies; 25+ messages in thread
From: Taniya Das @ 2018-06-12 11:02 UTC (permalink / raw)
  To: Rafael J. Wysocki, Viresh Kumar, linux-kernel, linux-pm, Stephen Boyd
  Cc: Rajendra Nayak, devicetree, robh, skannan, Taniya Das

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

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

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 52f5f1a..2683716 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -312,3 +312,12 @@ config ARM_PXA2xx_CPUFREQ
 	  This add the CPUFreq driver support for Intel PXA2xx SOCs.

 	  If in doubt, say N.
+
+config ARM_QCOM_CPUFREQ_FW
+	bool "QCOM CPUFreq FW driver"
+	help
+	 Support for the CPUFreq FW driver.
+	 The CPUfreq FW preset in some QCOM chipsets offloads the steps
+	 necessary for changing the frequency of CPUs. The driver
+	 implements the cpufreq driver interface for this firmware.
+	 Say Y if you want to support CPUFreq FW.
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index fb4a2ec..34691a2 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -86,6 +86,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
 obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
 obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
 obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
+obj-$(CONFIG_ARM_QCOM_CPUFREQ_FW)	+= qcom-cpufreq-fw.o


 ##################################################################################
diff --git a/drivers/cpufreq/qcom-cpufreq-fw.c b/drivers/cpufreq/qcom-cpufreq-fw.c
new file mode 100644
index 0000000..62f4452
--- /dev/null
+++ b/drivers/cpufreq/qcom-cpufreq-fw.c
@@ -0,0 +1,336 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/cpufreq.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+
+#define INIT_RATE			300000000UL
+#define XO_RATE				19200000UL
+#define LUT_MAX_ENTRIES			40U
+#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
+#define LUT_ROW_SIZE			32
+
+struct cpufreq_qcom {
+	struct cpufreq_frequency_table *table;
+	struct device *dev;
+	void __iomem *perf_base;
+	void __iomem *lut_base;
+	cpumask_t related_cpus;
+	unsigned int max_cores;
+};
+
+static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
+
+static int
+qcom_cpufreq_fw_target_index(struct cpufreq_policy *policy,
+			     unsigned int index)
+{
+	struct cpufreq_qcom *c = policy->driver_data;
+
+	writel_relaxed(index, c->perf_base);
+
+	return 0;
+}
+
+static unsigned int qcom_cpufreq_fw_get(unsigned int cpu)
+{
+	struct cpufreq_qcom *c;
+	struct cpufreq_policy *policy;
+	unsigned int index;
+
+	policy = cpufreq_cpu_get_raw(cpu);
+	if (!policy)
+		return 0;
+
+	c = policy->driver_data;
+
+	index = readl_relaxed(c->perf_base);
+	index = min(index, LUT_MAX_ENTRIES - 1);
+
+	return policy->freq_table[index].frequency;
+}
+
+static unsigned int
+qcom_cpufreq_fw_fast_switch(struct cpufreq_policy *policy,
+			    unsigned int target_freq)
+{
+	struct cpufreq_qcom *c = policy->driver_data;
+	int index;
+
+	index = cpufreq_table_find_index_l(policy, target_freq);
+	if (index < 0)
+		return 0;
+
+	writel_relaxed(index, c->perf_base);
+
+	return policy->freq_table[index].frequency;
+}
+
+static int qcom_cpufreq_fw_cpu_init(struct cpufreq_policy *policy)
+{
+	struct cpufreq_qcom *c;
+
+	c = qcom_freq_domain_map[policy->cpu];
+	if (!c) {
+		pr_err("No scaling support for CPU%d\n", policy->cpu);
+		return -ENODEV;
+	}
+
+	cpumask_copy(policy->cpus, &c->related_cpus);
+
+	policy->fast_switch_possible = true;
+	policy->freq_table = c->table;
+	policy->driver_data = c;
+
+	return 0;
+}
+
+static struct freq_attr *qcom_cpufreq_fw_attr[] = {
+	&cpufreq_freq_attr_scaling_available_freqs,
+	&cpufreq_freq_attr_scaling_boost_freqs,
+	NULL
+};
+
+static struct cpufreq_driver cpufreq_qcom_fw_driver = {
+	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
+			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
+	.verify		= cpufreq_generic_frequency_table_verify,
+	.target_index	= qcom_cpufreq_fw_target_index,
+	.get		= qcom_cpufreq_fw_get,
+	.init		= qcom_cpufreq_fw_cpu_init,
+	.fast_switch    = qcom_cpufreq_fw_fast_switch,
+	.name		= "qcom-cpufreq-fw",
+	.attr		= qcom_cpufreq_fw_attr,
+	.boost_enabled	= true,
+};
+
+static int qcom_read_lut(struct platform_device *pdev,
+			 struct cpufreq_qcom *c)
+{
+	struct device *dev = &pdev->dev;
+	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
+
+	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
+				sizeof(*c->table), GFP_KERNEL);
+	if (!c->table)
+		return -ENOMEM;
+
+	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
+		data = readl_relaxed(c->lut_base + i * LUT_ROW_SIZE);
+		src = ((data & GENMASK(31, 30)) >> 30);
+		lval = (data & GENMASK(7, 0));
+		core_count = CORE_COUNT_VAL(data);
+
+		if (!src)
+			c->table[i].frequency = INIT_RATE / 1000;
+		else
+			c->table[i].frequency = XO_RATE * lval / 1000;
+
+		cur_freq = c->table[i].frequency;
+
+		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
+			i, c->table[i].frequency, core_count);
+
+		if (core_count != c->max_cores)
+			cur_freq = CPUFREQ_ENTRY_INVALID;
+
+		/*
+		 * Two of the same frequencies with the same core counts means
+		 * end of table.
+		 */
+		if (i > 0 && c->table[i - 1].frequency ==
+		   c->table[i].frequency && prev_cc == core_count) {
+			struct cpufreq_frequency_table *prev = &c->table[i - 1];
+
+			if (prev_freq == CPUFREQ_ENTRY_INVALID)
+				prev->flags = CPUFREQ_BOOST_FREQ;
+			break;
+		}
+		prev_cc = core_count;
+		prev_freq = cur_freq;
+	}
+
+	c->table[i].frequency = CPUFREQ_TABLE_END;
+
+	return 0;
+}
+
+static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
+{
+	struct device_node *cpu_np, *freq_np;
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		cpu_np = of_cpu_device_node_get(cpu);
+		if (!cpu_np)
+			continue;
+		freq_np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
+		if (!freq_np)
+			continue;
+		if (freq_np == np)
+			cpumask_set_cpu(cpu, m);
+	}
+
+	return 0;
+}
+
+static int qcom_cpu_resources_init(struct platform_device *pdev,
+				   struct device_node *np, unsigned int cpu)
+{
+	struct cpufreq_qcom *c;
+	struct resource res;
+	struct device *dev = &pdev->dev;
+	void __iomem *en_base;
+	int index, ret;
+
+	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+	if (!c)
+		return -ENOMEM;
+
+	index = of_property_match_string(np, "reg-names", "enable");
+	if (index < 0)
+		return index;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	en_base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!en_base) {
+		dev_err(dev, "Unable to map %s enable-base\n", np->name);
+		return -ENOMEM;
+	}
+
+	/* FW should be in enabled state to proceed */
+	if (!(readl_relaxed(en_base) & 0x1)) {
+		dev_err(dev, "%s firmware not enabled\n", np->name);
+		return -ENODEV;
+	}
+	devm_iounmap(&pdev->dev, en_base);
+
+	index = of_property_match_string(np, "reg-names", "perf");
+	if (index < 0)
+		return index;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	c->perf_base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!c->perf_base) {
+		dev_err(dev, "Unable to map %s perf-base\n", np->name);
+		return -ENOMEM;
+	}
+
+	index = of_property_match_string(np, "reg-names", "lut");
+	if (index < 0)
+		return index;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	c->lut_base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!c->lut_base) {
+		dev_err(dev, "Unable to map %s lut-base\n", np->name);
+		return -ENOMEM;
+	}
+
+	ret = qcom_get_related_cpus(np, &c->related_cpus);
+	if (ret) {
+		dev_err(dev, "%s failed to get related CPUs\n", np->name);
+		return ret;
+	}
+
+	c->max_cores = cpumask_weight(&c->related_cpus);
+	if (!c->max_cores)
+		return -ENOENT;
+
+	ret = qcom_read_lut(pdev, c);
+	if (ret) {
+		dev_err(dev, "%s failed to read LUT\n", np->name);
+		return ret;
+	}
+
+	qcom_freq_domain_map[cpu] = c;
+
+	return 0;
+}
+
+static int qcom_resources_init(struct platform_device *pdev)
+{
+	struct device_node *np, *cpu_np;
+	unsigned int cpu;
+	int ret;
+
+	for_each_possible_cpu(cpu) {
+		cpu_np = of_cpu_device_node_get(cpu);
+		if (!cpu_np) {
+			dev_err(&pdev->dev, "Failed to get cpu %d device\n",
+				cpu);
+			continue;
+		}
+
+		np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
+		if (!np) {
+			dev_err(&pdev->dev, "Failed to get freq-domain device\n");
+			return -EINVAL;
+		}
+
+		of_node_put(cpu_np);
+
+		ret = qcom_cpu_resources_init(pdev, np, cpu);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int qcom_cpufreq_fw_driver_probe(struct platform_device *pdev)
+{
+	int rc;
+
+	/* Get the bases of cpufreq for domains */
+	rc = qcom_resources_init(pdev);
+	if (rc) {
+		dev_err(&pdev->dev, "CPUFreq resource init failed\n");
+		return rc;
+	}
+
+	rc = cpufreq_register_driver(&cpufreq_qcom_fw_driver);
+	if (rc) {
+		dev_err(&pdev->dev, "CPUFreq FW driver failed to register\n");
+		return rc;
+	}
+
+	dev_info(&pdev->dev, "QCOM CPUFreq FW driver initialized\n");
+
+	return 0;
+}
+
+static const struct of_device_id match_table[] = {
+	{ .compatible = "qcom,cpufreq-fw" },
+	{}
+};
+
+static struct platform_driver qcom_cpufreq_fw_driver = {
+	.probe = qcom_cpufreq_fw_driver_probe,
+	.driver = {
+		.name = "qcom-cpufreq-fw",
+		.of_match_table = match_table,
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init qcom_cpufreq_fw_init(void)
+{
+	return platform_driver_register(&qcom_cpufreq_fw_driver);
+}
+subsys_initcall(qcom_cpufreq_fw_init);
+
+MODULE_DESCRIPTION("QCOM CPU Frequency FW");
+MODULE_LICENSE("GPL v2");
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.


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

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-12 11:02 ` [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings Taniya Das
@ 2018-06-13 11:26   ` Sudeep Holla
  2018-06-13 18:13     ` Taniya Das
  2018-06-15 13:07   ` Amit Kucheria
  1 sibling, 1 reply; 25+ messages in thread
From: Sudeep Holla @ 2018-06-13 11:26 UTC (permalink / raw)
  To: Taniya Das, linux-kernel, linux-pm
  Cc: Rafael J. Wysocki, Viresh Kumar, Stephen Boyd, Sudeep Holla,
	Rajendra Nayak, devicetree, robh, skannan



On 12/06/18 12:02, 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 firmware.
> 
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  .../bindings/cpufreq/cpufreq-qcom-fw.txt           | 173 +++++++++++++++++++++
>  1 file changed, 173 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
> new file mode 100644
> index 0000000..e3087ec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
> @@ -0,0 +1,173 @@
> +Qualcomm Technologies, Inc. CPUFREQ Bindings
> +
> +CPUFREQ FW is a hardware engine used by some Qualcomm Technologies, Inc. (QTI)
> +SoCs to manage frequency in hardware. It is capable of controlling frequency
> +for multiple clusters.
> +

You are bit inconsistent on the wordings. Some places you refer this as
hardware engine. If so, please drop all references to firmware/FW. If
it's firmware then update accordingly.

> +Properties:
> +- compatible
> +	Usage:		required
> +	Value type:	<string>
> +	Definition:	must be "qcom,cpufreq-fw".
> +
> +* Property qcom,freq-domain
> +Devices supporting freq-domain must set their "qcom,freq-domain" property with
> +phandle to a freq_domain_table in their DT node.
> +
> +* Frequency Domain Table Node
> +
> +This describes the frequency domain belonging to a device.
> +This node can have following properties:
> +
> +- reg
> +	Usage:		required
> +	Value type:	<prop-encoded-array>
> +	Definition:	Addresses and sizes for the memory of the perf
> +			, lut and enable bases.
> +			perf - indicates the base address for the desired
> +			performance state to be set.
> +			lut - indicates the look up table base address for the
> +			cpufreq	driver to read frequencies.
> +			enable - indicates the enable register for firmware.


You still didn't answer my earlier question.

OS might touch one or 2 registers in lots of IP blocks. I am not sure
why those are any different from these. Are you trying to align with any
other bindings or specification. Are you trying to make this binding
generic here ? I understand if it was trying to generalize the firmware
interface, but you also state it's a hardware engine. So I fail to see
the need for such specificity here. Why not define the whole IP block
and the driver knows where to access these specific ones as they are
specific to this hardware block. In that way if you decide to add more
data, it's extensible easily without the need for patching DT.

Eg. Suppose you need some information on power curve for EAS energy
model, I really hate to update DT for that or even do a mix with DT just
because f/w is no longer modifiable.

-- 
Regards,
Sudeep

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

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-13 11:26   ` Sudeep Holla
@ 2018-06-13 18:13     ` Taniya Das
  2018-06-14 10:47       ` Sudeep Holla
  0 siblings, 1 reply; 25+ messages in thread
From: Taniya Das @ 2018-06-13 18:13 UTC (permalink / raw)
  To: Sudeep Holla, linux-kernel, linux-pm
  Cc: Rafael J. Wysocki, Viresh Kumar, Stephen Boyd, Rajendra Nayak,
	devicetree, robh, skannan

Hello Sudeep,

Thanks for review comments.

On 6/13/2018 4:56 PM, Sudeep Holla wrote:
> 
> 
> On 12/06/18 12:02, 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 firmware.
>>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> ---
>>   .../bindings/cpufreq/cpufreq-qcom-fw.txt           | 173 +++++++++++++++++++++
>>   1 file changed, 173 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
>>
>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
>> new file mode 100644
>> index 0000000..e3087ec
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
>> @@ -0,0 +1,173 @@
>> +Qualcomm Technologies, Inc. CPUFREQ Bindings
>> +
>> +CPUFREQ FW is a hardware engine used by some Qualcomm Technologies, Inc. (QTI)
>> +SoCs to manage frequency in hardware. It is capable of controlling frequency
>> +for multiple clusters.
>> +
> 
> You are bit inconsistent on the wordings. Some places you refer this as
> hardware engine. If so, please drop all references to firmware/FW. If
> it's firmware then update accordingly.
> 

It is a hardware engine which has a firmware to take care of the
managing the frequency request from OS. That is reason to refer it as a 
firmware.

>> +Properties:
>> +- compatible
>> +	Usage:		required
>> +	Value type:	<string>
>> +	Definition:	must be "qcom,cpufreq-fw".
>> +
>> +* Property qcom,freq-domain
>> +Devices supporting freq-domain must set their "qcom,freq-domain" property with
>> +phandle to a freq_domain_table in their DT node.
>> +
>> +* Frequency Domain Table Node
>> +
>> +This describes the frequency domain belonging to a device.
>> +This node can have following properties:
>> +
>> +- reg
>> +	Usage:		required
>> +	Value type:	<prop-encoded-array>
>> +	Definition:	Addresses and sizes for the memory of the perf
>> +			, lut and enable bases.
>> +			perf - indicates the base address for the desired
>> +			performance state to be set.
>> +			lut - indicates the look up table base address for the
>> +			cpufreq	driver to read frequencies.
>> +			enable - indicates the enable register for firmware.
> 
> 
> You still didn't answer my earlier question.
> 
> OS might touch one or 2 registers in lots of IP blocks. I am not sure
> why those are any different from these. Are you trying to align with any
> other bindings or specification. Are you trying to make this binding
> generic here ? I understand if it was trying to generalize the firmware
> interface, but you also state it's a hardware engine. So I fail to see
> the need for such specificity here. Why not define the whole IP block
> and the driver knows where to access these specific ones as they are
> specific to this hardware block. In that way if you decide to add more
> data, it's extensible easily without the need for patching DT.
> 

Sorry Sudeep I missed replying to your earlier query.
The High level OS(HLOS) would require to access only these specific 
registers from this IP block and just mapping the whole block(huge 
region) is unnecessary from the OS point of View. As of now it is a 
generic binding for all using this IP block to manage frequency 
requests. The OS would only have to know the frequencies supported i.e 
to read the lookup table registers and put across the OS request using 
the performance state register.

> Eg. Suppose you need some information on power curve for EAS energy
> model, I really hate to update DT for that or even do a mix with DT just
> because f/w is no longer modifiable.
> 

For now we are safe.

-- 
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] 25+ messages in thread

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-13 18:13     ` Taniya Das
@ 2018-06-14 10:47       ` Sudeep Holla
  2018-06-14 18:24         ` Taniya Das
  0 siblings, 1 reply; 25+ messages in thread
From: Sudeep Holla @ 2018-06-14 10:47 UTC (permalink / raw)
  To: Taniya Das, linux-kernel, linux-pm
  Cc: Sudeep Holla, Rafael J. Wysocki, Viresh Kumar, Stephen Boyd,
	Rajendra Nayak, devicetree, robh, skannan



On 13/06/18 19:13, Taniya Das wrote:
> Hello Sudeep,
> 
> Thanks for review comments.
> 
> On 6/13/2018 4:56 PM, Sudeep Holla wrote:
>>
>>

[...]

>> You are bit inconsistent on the wordings. Some places you refer this as
>> hardware engine. If so, please drop all references to firmware/FW. If
>> it's firmware then update accordingly.
>>
> 
> It is a hardware engine which has a firmware to take care of the
> managing the frequency request from OS. That is reason to refer it as a
> firmware.
> 

Yes I did guess that initially, but I failed to understand when
different bindings were posted to deal with devfreq and cpufreq with the
same firmware. Ideally if it's the firmware you are talking to, place
all these under /firmware node and align all those with single binding.

Is there anything else that this firmware deals with ? If so all those
need to be put in one place.

>>> +Properties:
>>> +- compatible
>>> +    Usage:        required
>>> +    Value type:    <string>
>>> +    Definition:    must be "qcom,cpufreq-fw".
>>> +
>>> +* Property qcom,freq-domain
>>> +Devices supporting freq-domain must set their "qcom,freq-domain"
>>> property with
>>> +phandle to a freq_domain_table in their DT node.
>>> +
>>> +* Frequency Domain Table Node
>>> +
>>> +This describes the frequency domain belonging to a device.
>>> +This node can have following properties:
>>> +
>>> +- reg
>>> +    Usage:        required
>>> +    Value type:    <prop-encoded-array>
>>> +    Definition:    Addresses and sizes for the memory of the perf
>>> +            , lut and enable bases.
>>> +            perf - indicates the base address for the desired
>>> +            performance state to be set.
>>> +            lut - indicates the look up table base address for the
>>> +            cpufreq    driver to read frequencies.
>>> +            enable - indicates the enable register for firmware.
>>
>>
>> You still didn't answer my earlier question.
>>
>> OS might touch one or 2 registers in lots of IP blocks. I am not sure
>> why those are any different from these. Are you trying to align with any
>> other bindings or specification. Are you trying to make this binding
>> generic here ? I understand if it was trying to generalize the firmware
>> interface, but you also state it's a hardware engine. So I fail to see
>> the need for such specificity here. Why not define the whole IP block
>> and the driver knows where to access these specific ones as they are
>> specific to this hardware block. In that way if you decide to add more
>> data, it's extensible easily without the need for patching DT.
>>
> 
> Sorry Sudeep I missed replying to your earlier query.
> The High level OS(HLOS) would require to access only these specific
> registers from this IP block and just mapping the whole block(huge
> region) is unnecessary from the OS point of View. As of now it is a
> generic binding for all using this IP block to manage frequency
> requests. The OS would only have to know the frequencies supported i.e
> to read the lookup table registers and put across the OS request using
> the performance state register.
> 

I am not sure if you need to defining bindings to save OSPM IO mapping.
In-fact you may be adding more mapping unnecessarily. The mappings are
page aligned and spiting the registers and mapping them individually may
result in more mappings.

I just need to know the rational for such specific choice of registers.
I assume it's aligned to some other standard specifications like CPPC
though not identical.

>> Eg. Suppose you need some information on power curve for EAS energy
>> model, I really hate to update DT for that or even do a mix with DT just
>> because f/w is no longer modifiable.
>>
> 
> For now we are safe.
> 

What do you mean by that ? It should be easily extensible is what I am
trying to say. You can add more info and alter the information in the
driver with compatibles if you keep the register info as minimum as
possible. For now, you have enable, set and lut registers. What if you
want to provide power numbers ?

-- 
Regards,
Sudeep

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

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-14 10:47       ` Sudeep Holla
@ 2018-06-14 18:24         ` Taniya Das
  2018-06-15 11:59           ` Amit Kucheria
  2018-06-15 13:23           ` Sudeep Holla
  0 siblings, 2 replies; 25+ messages in thread
From: Taniya Das @ 2018-06-14 18:24 UTC (permalink / raw)
  To: Sudeep Holla, linux-kernel, linux-pm
  Cc: Rafael J. Wysocki, Viresh Kumar, Stephen Boyd, Rajendra Nayak,
	devicetree, robh, skannan

Hello Sudeep,

Thanks for your comments.

On 6/14/2018 4:17 PM, Sudeep Holla wrote:
> 
> 
> On 13/06/18 19:13, Taniya Das wrote:
>> Hello Sudeep,
>>
>> Thanks for review comments.
>>
>> On 6/13/2018 4:56 PM, Sudeep Holla wrote:
>>>
>>>
> 
> [...]
> 
>>> You are bit inconsistent on the wordings. Some places you refer this as
>>> hardware engine. If so, please drop all references to firmware/FW. If
>>> it's firmware then update accordingly.
>>>
>>
>> It is a hardware engine which has a firmware to take care of the
>> managing the frequency request from OS. That is reason to refer it as a
>> firmware.
>>
> 
> Yes I did guess that initially, but I failed to understand when
> different bindings were posted to deal with devfreq and cpufreq with the
> same firmware. Ideally if it's the firmware you are talking to, place
> all these under /firmware node and align all those with single binding.
> 

The OS is not aware of the firmware and OS only knows about the hardware 
engine and has to put forward it's request to the hardware engine using 
the "Perf" state register in both devfreq & cpufreq. So would it be 
still required to put under the /firmware node?

> Is there anything else that this firmware deals with ? If so all those
> need to be put in one place.
> 

We deal only with the CPU frequency and L3 frequency(devfreq).

>>>> +Properties:
>>>> +- compatible
>>>> +    Usage:        required
>>>> +    Value type:    <string>
>>>> +    Definition:    must be "qcom,cpufreq-fw".
>>>> +
>>>> +* Property qcom,freq-domain
>>>> +Devices supporting freq-domain must set their "qcom,freq-domain"
>>>> property with
>>>> +phandle to a freq_domain_table in their DT node.
>>>> +
>>>> +* Frequency Domain Table Node
>>>> +
>>>> +This describes the frequency domain belonging to a device.
>>>> +This node can have following properties:
>>>> +
>>>> +- reg
>>>> +    Usage:        required
>>>> +    Value type:    <prop-encoded-array>
>>>> +    Definition:    Addresses and sizes for the memory of the perf
>>>> +            , lut and enable bases.
>>>> +            perf - indicates the base address for the desired
>>>> +            performance state to be set.
>>>> +            lut - indicates the look up table base address for the
>>>> +            cpufreq    driver to read frequencies.
>>>> +            enable - indicates the enable register for firmware.
>>>
>>>
>>> You still didn't answer my earlier question.
>>>
>>> OS might touch one or 2 registers in lots of IP blocks. I am not sure
>>> why those are any different from these. Are you trying to align with any
>>> other bindings or specification. Are you trying to make this binding
>>> generic here ? I understand if it was trying to generalize the firmware
>>> interface, but you also state it's a hardware engine. So I fail to see
>>> the need for such specificity here. Why not define the whole IP block
>>> and the driver knows where to access these specific ones as they are
>>> specific to this hardware block. In that way if you decide to add more
>>> data, it's extensible easily without the need for patching DT.
>>>
>>
>> Sorry Sudeep I missed replying to your earlier query.
>> The High level OS(HLOS) would require to access only these specific
>> registers from this IP block and just mapping the whole block(huge
>> region) is unnecessary from the OS point of View. As of now it is a
>> generic binding for all using this IP block to manage frequency
>> requests. The OS would only have to know the frequencies supported i.e
>> to read the lookup table registers and put across the OS request using
>> the performance state register.
>>
> 
> I am not sure if you need to defining bindings to save OSPM IO mapping.
> In-fact you may be adding more mapping unnecessarily. The mappings are
> page aligned and spiting the registers and mapping them individually may
> result in more mappings.
> 
> I just need to know the rational for such specific choice of registers.
> I assume it's aligned to some other standard specifications like CPPC
> though not identical.
> 

I am not sure of the query but there is no other register that the OS is 
required to use other than the ones defined here.

>>> Eg. Suppose you need some information on power curve for EAS energy
>>> model, I really hate to update DT for that or even do a mix with DT just
>>> because f/w is no longer modifiable.
>>>
>>
>> For now we are safe.
>>
>
> What do you mean by that ?

I meant here was currently there is no such known case where the f/w is 
no longer modifiable and we need to extend device tree bindings.

> It should be easily extensible is what I am
> trying to say. You can add more info and alter the information in the
> driver with compatibles if you keep the register info as minimum as
> possible. For now, you have enable, set and lut registers. What if you
> want to provide power numbers ?
> 

Yes I do understand the intent of mapping the whole register space, but 
as per the HW specs these 3 registers would be the only ones required 
for now. I do not think this hardware engine has any information on the 
power numbers.

-- 
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] 25+ messages in thread

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-14 18:24         ` Taniya Das
@ 2018-06-15 11:59           ` Amit Kucheria
  2018-06-15 13:27             ` Sudeep Holla
  2018-06-15 17:40             ` Taniya Das
  2018-06-15 13:23           ` Sudeep Holla
  1 sibling, 2 replies; 25+ messages in thread
From: Amit Kucheria @ 2018-06-15 11:59 UTC (permalink / raw)
  To: Taniya Das
  Cc: Sudeep Holla, LKML, Linux PM list, Rafael J. Wysocki,
	Viresh Kumar, Stephen Boyd, Rajendra Nayak,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, Saravana Kannan

On Thu, Jun 14, 2018 at 9:24 PM, Taniya Das <tdas@codeaurora.org> wrote:

>>> Sorry Sudeep I missed replying to your earlier query.
>>> The High level OS(HLOS) would require to access only these specific
>>> registers from this IP block and just mapping the whole block(huge
>>> region) is unnecessary from the OS point of View. As of now it is a
>>> generic binding for all using this IP block to manage frequency
>>> requests. The OS would only have to know the frequencies supported i.e
>>> to read the lookup table registers and put across the OS request using
>>> the performance state register.
>>>
>>
>> I am not sure if you need to defining bindings to save OSPM IO mapping.
>> In-fact you may be adding more mapping unnecessarily. The mappings are
>> page aligned and spiting the registers and mapping them individually may
>> result in more mappings.
>>
>> I just need to know the rational for such specific choice of registers.
>> I assume it's aligned to some other standard specifications like CPPC
>> though not identical.
>>
>
> I am not sure of the query but there is no other register that the OS is
> required to use other than the ones defined here.
>
>>>> Eg. Suppose you need some information on power curve for EAS energy
>>>> model, I really hate to update DT for that or even do a mix with DT just
>>>> because f/w is no longer modifiable.
>>>>
>>>
>>> For now we are safe.
>>>
>>
>> What do you mean by that ?
>
>
> I meant here was currently there is no such known case where the f/w is no
> longer modifiable and we need to extend device tree bindings.
>
>> It should be easily extensible is what I am
>> trying to say. You can add more info and alter the information in the
>> driver with compatibles if you keep the register info as minimum as
>> possible. For now, you have enable, set and lut registers. What if you
>> want to provide power numbers ?
>>
>
> Yes I do understand the intent of mapping the whole register space, but as
> per the HW specs these 3 registers would be the only ones required for now.
> I do not think this hardware engine has any information on the power
> numbers.

"For now" - I think this is exactly the point that Sudeep is trying to make.

A future version of the HW engine, or more likely, a firmware
revision, will make more functionality available. Say, this needs
access to another register or two. This will require changing the DT
bindings. Instead, if you map the entire address space, you can just
add offsets to the new registers.

So in this case, I think you should define the following addresses
(size 0x1400) for the two frequency domains

0x17d43000, 0x1400 (power cluster)
0x17d45800, 0x1400 (perf cluster)

And in the driver simply add offsets as follows:

#define ENABLE_OFFSET               0x0
#define LUT_OFFSET                      0x110
#define PERF_DESIRED_OFFSET 0x920

This will allow you add any new registers in the future w/o modifying
the DT binding and reduce qcom_cpu_resources_init() to a handful of
lines since you no longer need so many OF string matches, and
devm_ioremap()s.

Regards,
Amit

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

* Re: [PATCH v4 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver
  2018-06-12 11:02 ` [PATCH v4 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver Taniya Das
@ 2018-06-15 12:02   ` Amit Kucheria
  2018-06-19  9:30   ` Viresh Kumar
  2018-07-11 20:37   ` Matthias Kaehlcke
  2 siblings, 0 replies; 25+ messages in thread
From: Amit Kucheria @ 2018-06-15 12:02 UTC (permalink / raw)
  To: Taniya Das
  Cc: Rafael J. Wysocki, Viresh Kumar, LKML, Linux PM list,
	Stephen Boyd, Rajendra Nayak,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, Saravana Kannan

On Tue, Jun 12, 2018 at 2:02 PM, Taniya Das <tdas@codeaurora.org> wrote:
> The CPUfreq FW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this firmware.
>
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  drivers/cpufreq/Kconfig.arm       |   9 +
>  drivers/cpufreq/Makefile          |   1 +
>  drivers/cpufreq/qcom-cpufreq-fw.c | 336 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 346 insertions(+)
>  create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 52f5f1a..2683716 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -312,3 +312,12 @@ config ARM_PXA2xx_CPUFREQ
>           This add the CPUFreq driver support for Intel PXA2xx SOCs.
>
>           If in doubt, say N.
> +
> +config ARM_QCOM_CPUFREQ_FW
> +       bool "QCOM CPUFreq FW driver"
> +       help
> +        Support for the CPUFreq FW driver.
> +        The CPUfreq FW preset in some QCOM chipsets offloads the steps

s/preset/present

> +        necessary for changing the frequency of CPUs. The driver

I'd rephrase this a bit to address Sudeep's comment. Something like:

"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 high-level OS. This driver
exposes a cpufreq abstraction for this HW engine. Say Y ....."

> +        implements the cpufreq driver interface for this firmware.
> +        Say Y if you want to support CPUFreq FW.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index fb4a2ec..34691a2 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -86,6 +86,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)    += tegra124-cpufreq.o
>  obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)     += tegra186-cpufreq.o
>  obj-$(CONFIG_ARM_TI_CPUFREQ)           += ti-cpufreq.o
>  obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_FW)      += qcom-cpufreq-fw.o
>

<snip>

> +static const struct of_device_id match_table[] = {
> +       { .compatible = "qcom,cpufreq-fw" },
> +       {}
> +};
> +
> +static struct platform_driver qcom_cpufreq_fw_driver = {
> +       .probe = qcom_cpufreq_fw_driver_probe,
> +       .driver = {
> +               .name = "qcom-cpufreq-fw",
> +               .of_match_table = match_table,
> +               .owner = THIS_MODULE,
> +       },
> +};
> +
> +static int __init qcom_cpufreq_fw_init(void)
> +{
> +       return platform_driver_register(&qcom_cpufreq_fw_driver);
> +}
> +subsys_initcall(qcom_cpufreq_fw_init);
> +
> +MODULE_DESCRIPTION("QCOM CPU Frequency FW");

This can be a bit more descriptive, e.g.

"QCOM firmware-based CPU Frequency  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	[flat|nested] 25+ messages in thread

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-12 11:02 ` [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings Taniya Das
  2018-06-13 11:26   ` Sudeep Holla
@ 2018-06-15 13:07   ` Amit Kucheria
  1 sibling, 0 replies; 25+ messages in thread
From: Amit Kucheria @ 2018-06-15 13:07 UTC (permalink / raw)
  To: Taniya Das
  Cc: Rafael J. Wysocki, Viresh Kumar, LKML, Linux PM list,
	Stephen Boyd, Rajendra Nayak,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, Saravana Kannan

On Tue, Jun 12, 2018 at 2:02 PM, Taniya Das <tdas@codeaurora.org> wrote:
> Add QCOM cpufreq firmware device bindings for Qualcomm Technology Inc's
> SoCs. This is required for managing the cpu frequency transitions which are
> controlled by firmware.
>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>


> +       qcom,cpufreq-fw {
> +               compatible = "qcom,cpufreq-fw";
> +
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +
> +               freq_domain_table0 : freq_table0 {
> +                       reg = <0x17d43920 0x4>, <0x17d43110 0x500>,
> +                                <0x17d41000 0x4>;

Changing the enable address to 0x17d43000 would make this a working
example, I think.

> +                       reg-names = "perf", "lut", "enable";
> +               };
> +
> +               freq_domain_table1 : freq_table1 {
> +                       reg = <0x17d46120 0x4>, <0x17d45910 0x500>,
> +                               <0x17d45800 0x4> ;
> +                       reg-names = "perf", "lut", "enable";
> +               };
> +       };

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

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-14 18:24         ` Taniya Das
  2018-06-15 11:59           ` Amit Kucheria
@ 2018-06-15 13:23           ` Sudeep Holla
  2018-06-15 17:31             ` Taniya Das
  1 sibling, 1 reply; 25+ messages in thread
From: Sudeep Holla @ 2018-06-15 13:23 UTC (permalink / raw)
  To: Taniya Das, linux-kernel, linux-pm
  Cc: Sudeep Holla, Rafael J. Wysocki, Viresh Kumar, Stephen Boyd,
	Rajendra Nayak, devicetree, robh, skannan



On 14/06/18 19:24, Taniya Das wrote:
> Hello Sudeep,
> 
> Thanks for your comments.
> 
> On 6/14/2018 4:17 PM, Sudeep Holla wrote:
>>
>>
>> On 13/06/18 19:13, Taniya Das wrote:
>>> Hello Sudeep,
>>>
>>> Thanks for review comments.
>>>
>>> On 6/13/2018 4:56 PM, Sudeep Holla wrote:
>>>>
>>>>
>>
>> [...]
>>
>>>> You are bit inconsistent on the wordings. Some places you refer this as
>>>> hardware engine. If so, please drop all references to firmware/FW. If
>>>> it's firmware then update accordingly.
>>>>
>>>
>>> It is a hardware engine which has a firmware to take care of the
>>> managing the frequency request from OS. That is reason to refer it as a
>>> firmware.
>>>
>>
>> Yes I did guess that initially, but I failed to understand when
>> different bindings were posted to deal with devfreq and cpufreq with the
>> same firmware. Ideally if it's the firmware you are talking to, place
>> all these under /firmware node and align all those with single binding.
>>
> 
> The OS is not aware of the firmware and OS only knows about the hardware
> engine and has to put forward it's request to the hardware engine using
> the "Perf" state register in both devfreq & cpufreq. So would it be
> still required to put under the /firmware node?
> 

Ah ok, then remove any references to firmware other than stating its
presence in the introduction. E.g. you have "Add cpufreq firmware device
bindings ...". So this is definitely not firmware binding. You are just
presenting the h/w as is and you need to deal with change of firmware in
DT and OS agnostic way.

>> Is there anything else that this firmware deals with ? If so all those
>> need to be put in one place.
>>
> 
> We deal only with the CPU frequency and L3 frequency(devfreq).
> 

OK

>>>>> +Properties:
>>>>> +- compatible
>>>>> +    Usage:        required
>>>>> +    Value type:    <string>
>>>>> +    Definition:    must be "qcom,cpufreq-fw".
>>>>> +
>>>>> +* Property qcom,freq-domain
>>>>> +Devices supporting freq-domain must set their "qcom,freq-domain"
>>>>> property with
>>>>> +phandle to a freq_domain_table in their DT node.
>>>>> +
>>>>> +* Frequency Domain Table Node
>>>>> +
>>>>> +This describes the frequency domain belonging to a device.
>>>>> +This node can have following properties:
>>>>> +
>>>>> +- reg
>>>>> +    Usage:        required
>>>>> +    Value type:    <prop-encoded-array>
>>>>> +    Definition:    Addresses and sizes for the memory of the perf
>>>>> +            , lut and enable bases.
>>>>> +            perf - indicates the base address for the desired
>>>>> +            performance state to be set.
>>>>> +            lut - indicates the look up table base address for the
>>>>> +            cpufreq    driver to read frequencies.
>>>>> +            enable - indicates the enable register for firmware.
>>>>
>>>>
>>>> You still didn't answer my earlier question.
>>>>
>>>> OS might touch one or 2 registers in lots of IP blocks. I am not sure
>>>> why those are any different from these. Are you trying to align with
>>>> any
>>>> other bindings or specification. Are you trying to make this binding
>>>> generic here ? I understand if it was trying to generalize the firmware
>>>> interface, but you also state it's a hardware engine. So I fail to see
>>>> the need for such specificity here. Why not define the whole IP block
>>>> and the driver knows where to access these specific ones as they are
>>>> specific to this hardware block. In that way if you decide to add more
>>>> data, it's extensible easily without the need for patching DT.
>>>>
>>>
>>> Sorry Sudeep I missed replying to your earlier query.
>>> The High level OS(HLOS) would require to access only these specific
>>> registers from this IP block and just mapping the whole block(huge
>>> region) is unnecessary from the OS point of View. As of now it is a
>>> generic binding for all using this IP block to manage frequency
>>> requests. The OS would only have to know the frequencies supported i.e
>>> to read the lookup table registers and put across the OS request using
>>> the performance state register.
>>>
>>
>> I am not sure if you need to defining bindings to save OSPM IO mapping.
>> In-fact you may be adding more mapping unnecessarily. The mappings are
>> page aligned and spiting the registers and mapping them individually may
>> result in more mappings.
>>
>> I just need to know the rational for such specific choice of registers.
>> I assume it's aligned to some other standard specifications like CPPC
>> though not identical.
>>
> 
> I am not sure of the query but there is no other register that the OS is
> required to use other than the ones defined here.
> 

The point is ever IP on the SoC may have 100s to 1000s of registers that
may or may not be used by OS. That's about to the OS to decide and you
just need to provide the hardware view to anyone using the device tree.
It *should not* _just_ represent what you think OS(Linux in particular)
"for now"

>>>> Eg. Suppose you need some information on power curve for EAS energy
>>>> model, I really hate to update DT for that or even do a mix with DT
>>>> just
>>>> because f/w is no longer modifiable.
>>>>
>>>
>>> For now we are safe.
>>>
>>
>> What do you mean by that ?
> 
> I meant here was currently there is no such known case where the f/w is
> no longer modifiable and we need to extend device tree bindings.
> 
>> It should be easily extensible is what I am
>> trying to say. You can add more info and alter the information in the
>> driver with compatibles if you keep the register info as minimum as
>> possible. For now, you have enable, set and lut registers. What if you
>> want to provide power numbers ?
>>
> 
> Yes I do understand the intent of mapping the whole register space, but
> as per the HW specs these 3 registers would be the only ones required
> for now. I do not think this hardware engine has any information on the
> power numbers.
> 

That's fine. So on this platform DT, will you list only the registers
touched by the OS for all the IP ? I am sure that will not be the case.

-- 
Regards,
Sudeep

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

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-15 11:59           ` Amit Kucheria
@ 2018-06-15 13:27             ` Sudeep Holla
  2018-06-15 17:40             ` Taniya Das
  1 sibling, 0 replies; 25+ messages in thread
From: Sudeep Holla @ 2018-06-15 13:27 UTC (permalink / raw)
  To: Amit Kucheria, Taniya Das
  Cc: Sudeep Holla, LKML, Linux PM list, Rafael J. Wysocki,
	Viresh Kumar, Stephen Boyd, Rajendra Nayak,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, Saravana Kannan



On 15/06/18 12:59, Amit Kucheria wrote:
> On Thu, Jun 14, 2018 at 9:24 PM, Taniya Das <tdas@codeaurora.org> wrote:
> 

[...]

>>
>> Yes I do understand the intent of mapping the whole register space, but as
>> per the HW specs these 3 registers would be the only ones required for now.
>> I do not think this hardware engine has any information on the power
>> numbers.
> 
> "For now" - I think this is exactly the point that Sudeep is trying to make.
> 
> A future version of the HW engine, or more likely, a firmware
> revision, will make more functionality available. Say, this needs
> access to another register or two. This will require changing the DT
> bindings. Instead, if you map the entire address space, you can just
> add offsets to the new registers.
> 
> So in this case, I think you should define the following addresses
> (size 0x1400) for the two frequency domains
> 
> 0x17d43000, 0x1400 (power cluster)
> 0x17d45800, 0x1400 (perf cluster)
> 
> And in the driver simply add offsets as follows:
> 
> #define ENABLE_OFFSET               0x0
> #define LUT_OFFSET                      0x110
> #define PERF_DESIRED_OFFSET 0x920
> 
> This will allow you add any new registers in the future w/o modifying
> the DT binding and reduce qcom_cpu_resources_init() to a handful of
> lines since you no longer need so many OF string matches, and
> devm_ioremap()s.
> 

Thanks Amit for such nice and detailed explanation. I was lazy to write
in such details, but was hoping Taniya to understand the point. Anyways
thanks again for doing this.

-- 
Regards,
Sudeep

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

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-15 13:23           ` Sudeep Holla
@ 2018-06-15 17:31             ` Taniya Das
  2018-06-15 17:42               ` Sudeep Holla
  0 siblings, 1 reply; 25+ messages in thread
From: Taniya Das @ 2018-06-15 17:31 UTC (permalink / raw)
  To: Sudeep Holla, linux-kernel, linux-pm
  Cc: Rafael J. Wysocki, Viresh Kumar, Stephen Boyd, Rajendra Nayak,
	devicetree, robh, skannan



On 6/15/2018 6:53 PM, Sudeep Holla wrote:
> 
> 
> On 14/06/18 19:24, Taniya Das wrote:
>> Hello Sudeep,
>>
>> Thanks for your comments.
>>
>> On 6/14/2018 4:17 PM, Sudeep Holla wrote:
>>>
>>>
>>> On 13/06/18 19:13, Taniya Das wrote:
>>>> Hello Sudeep,
>>>>
>>>> Thanks for review comments.
>>>>
>>>> On 6/13/2018 4:56 PM, Sudeep Holla wrote:
>>>>>
>>>>>
>>>
>>> [...]
>>>
>>>>> You are bit inconsistent on the wordings. Some places you refer this as
>>>>> hardware engine. If so, please drop all references to firmware/FW. If
>>>>> it's firmware then update accordingly.
>>>>>
>>>>
>>>> It is a hardware engine which has a firmware to take care of the
>>>> managing the frequency request from OS. That is reason to refer it as a
>>>> firmware.
>>>>
>>>
>>> Yes I did guess that initially, but I failed to understand when
>>> different bindings were posted to deal with devfreq and cpufreq with the
>>> same firmware. Ideally if it's the firmware you are talking to, place
>>> all these under /firmware node and align all those with single binding.
>>>
>>
>> The OS is not aware of the firmware and OS only knows about the hardware
>> engine and has to put forward it's request to the hardware engine using
>> the "Perf" state register in both devfreq & cpufreq. So would it be
>> still required to put under the /firmware node?
>>
> 
> Ah ok, then remove any references to firmware other than stating its
> presence in the introduction. E.g. you have "Add cpufreq firmware device
> bindings ...". So this is definitely not firmware binding. You are just
> presenting the h/w as is and you need to deal with change of firmware in
> DT and OS agnostic way.
> 

Sure Sudeep, I will update the references of firmware.

>>> Is there anything else that this firmware deals with ? If so all those
>>> need to be put in one place.
>>>
>>
>> We deal only with the CPU frequency and L3 frequency(devfreq).
>>
> 
> OK
> 
>>>>>> +Properties:
>>>>>> +- compatible
>>>>>> +    Usage:        required
>>>>>> +    Value type:    <string>
>>>>>> +    Definition:    must be "qcom,cpufreq-fw".
>>>>>> +
>>>>>> +* Property qcom,freq-domain
>>>>>> +Devices supporting freq-domain must set their "qcom,freq-domain"
>>>>>> property with
>>>>>> +phandle to a freq_domain_table in their DT node.
>>>>>> +
>>>>>> +* Frequency Domain Table Node
>>>>>> +
>>>>>> +This describes the frequency domain belonging to a device.
>>>>>> +This node can have following properties:
>>>>>> +
>>>>>> +- reg
>>>>>> +    Usage:        required
>>>>>> +    Value type:    <prop-encoded-array>
>>>>>> +    Definition:    Addresses and sizes for the memory of the perf
>>>>>> +            , lut and enable bases.
>>>>>> +            perf - indicates the base address for the desired
>>>>>> +            performance state to be set.
>>>>>> +            lut - indicates the look up table base address for the
>>>>>> +            cpufreq    driver to read frequencies.
>>>>>> +            enable - indicates the enable register for firmware.
>>>>>
>>>>>
>>>>> You still didn't answer my earlier question.
>>>>>
>>>>> OS might touch one or 2 registers in lots of IP blocks. I am not sure
>>>>> why those are any different from these. Are you trying to align with
>>>>> any
>>>>> other bindings or specification. Are you trying to make this binding
>>>>> generic here ? I understand if it was trying to generalize the firmware
>>>>> interface, but you also state it's a hardware engine. So I fail to see
>>>>> the need for such specificity here. Why not define the whole IP block
>>>>> and the driver knows where to access these specific ones as they are
>>>>> specific to this hardware block. In that way if you decide to add more
>>>>> data, it's extensible easily without the need for patching DT.
>>>>>
>>>>
>>>> Sorry Sudeep I missed replying to your earlier query.
>>>> The High level OS(HLOS) would require to access only these specific
>>>> registers from this IP block and just mapping the whole block(huge
>>>> region) is unnecessary from the OS point of View. As of now it is a
>>>> generic binding for all using this IP block to manage frequency
>>>> requests. The OS would only have to know the frequencies supported i.e
>>>> to read the lookup table registers and put across the OS request using
>>>> the performance state register.
>>>>
>>>
>>> I am not sure if you need to defining bindings to save OSPM IO mapping.
>>> In-fact you may be adding more mapping unnecessarily. The mappings are
>>> page aligned and spiting the registers and mapping them individually may
>>> result in more mappings.
>>>
>>> I just need to know the rational for such specific choice of registers.
>>> I assume it's aligned to some other standard specifications like CPPC
>>> though not identical.
>>>
>>
>> I am not sure of the query but there is no other register that the OS is
>> required to use other than the ones defined here.
>>
> 
> The point is ever IP on the SoC may have 100s to 1000s of registers that
> may or may not be used by OS. That's about to the OS to decide and you
> just need to provide the hardware view to anyone using the device tree.
> It *should not* _just_ represent what you think OS(Linux in particular)
> "for now"
> 
>>>>> Eg. Suppose you need some information on power curve for EAS energy
>>>>> model, I really hate to update DT for that or even do a mix with DT
>>>>> just
>>>>> because f/w is no longer modifiable.
>>>>>
>>>>
>>>> For now we are safe.
>>>>
>>>
>>> What do you mean by that ?
>>
>> I meant here was currently there is no such known case where the f/w is
>> no longer modifiable and we need to extend device tree bindings.
>>
>>> It should be easily extensible is what I am
>>> trying to say. You can add more info and alter the information in the
>>> driver with compatibles if you keep the register info as minimum as
>>> possible. For now, you have enable, set and lut registers. What if you
>>> want to provide power numbers ?
>>>
>>
>> Yes I do understand the intent of mapping the whole register space, but
>> as per the HW specs these 3 registers would be the only ones required
>> for now. I do not think this hardware engine has any information on the
>> power numbers.
>>
> 
> That's fine. So on this platform DT, will you list only the registers
> touched by the OS for all the IP ? I am sure that will not be the case.
> 

Yes, registers list those would be touched by OS only.

-- 
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] 25+ messages in thread

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-15 11:59           ` Amit Kucheria
  2018-06-15 13:27             ` Sudeep Holla
@ 2018-06-15 17:40             ` Taniya Das
  2018-06-15 17:45               ` Sudeep Holla
                                 ` (2 more replies)
  1 sibling, 3 replies; 25+ messages in thread
From: Taniya Das @ 2018-06-15 17:40 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: Sudeep Holla, LKML, Linux PM list, Rafael J. Wysocki,
	Viresh Kumar, Stephen Boyd, Rajendra Nayak,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, Saravana Kannan



On 6/15/2018 5:29 PM, Amit Kucheria wrote:
> On Thu, Jun 14, 2018 at 9:24 PM, Taniya Das <tdas@codeaurora.org> wrote:
> 
>>>> Sorry Sudeep I missed replying to your earlier query.
>>>> The High level OS(HLOS) would require to access only these specific
>>>> registers from this IP block and just mapping the whole block(huge
>>>> region) is unnecessary from the OS point of View. As of now it is a
>>>> generic binding for all using this IP block to manage frequency
>>>> requests. The OS would only have to know the frequencies supported i.e
>>>> to read the lookup table registers and put across the OS request using
>>>> the performance state register.
>>>>
>>>
>>> I am not sure if you need to defining bindings to save OSPM IO mapping.
>>> In-fact you may be adding more mapping unnecessarily. The mappings are
>>> page aligned and spiting the registers and mapping them individually may
>>> result in more mappings.
>>>
>>> I just need to know the rational for such specific choice of registers.
>>> I assume it's aligned to some other standard specifications like CPPC
>>> though not identical.
>>>
>>
>> I am not sure of the query but there is no other register that the OS is
>> required to use other than the ones defined here.
>>
>>>>> Eg. Suppose you need some information on power curve for EAS energy
>>>>> model, I really hate to update DT for that or even do a mix with DT just
>>>>> because f/w is no longer modifiable.
>>>>>
>>>>
>>>> For now we are safe.
>>>>
>>>
>>> What do you mean by that ?
>>
>>
>> I meant here was currently there is no such known case where the f/w is no
>> longer modifiable and we need to extend device tree bindings.
>>
>>> It should be easily extensible is what I am
>>> trying to say. You can add more info and alter the information in the
>>> driver with compatibles if you keep the register info as minimum as
>>> possible. For now, you have enable, set and lut registers. What if you
>>> want to provide power numbers ?
>>>
>>
>> Yes I do understand the intent of mapping the whole register space, but as
>> per the HW specs these 3 registers would be the only ones required for now.
>> I do not think this hardware engine has any information on the power
>> numbers.
> 
> "For now" - I think this is exactly the point that Sudeep is trying to make.
> 
> A future version of the HW engine, or more likely, a firmware
> revision, will make more functionality available. Say, this needs
> access to another register or two. This will require changing the DT
> bindings. Instead, if you map the entire address space, you can just
> add offsets to the new registers.
> 
> So in this case, I think you should define the following addresses
> (size 0x1400) for the two frequency domains
> 
> 0x17d43000, 0x1400 (power cluster)
> 0x17d45800, 0x1400 (perf cluster)
> 
> And in the driver simply add offsets as follows:
> 
> #define ENABLE_OFFSET               0x0
> #define LUT_OFFSET                      0x110
> #define PERF_DESIRED_OFFSET 0x920
> 

The offsets could vary across versions of this IP and that is the reason 
to provide them through the DT and not define any such offsets.

> This will allow you add any new registers in the future w/o modifying
> the DT binding and reduce qcom_cpu_resources_init() to a handful of
> lines since you no longer need so many OF string matches, and
> devm_ioremap()s.
> 
> Regards,
> Amit
> 

-- 
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] 25+ messages in thread

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-15 17:31             ` Taniya Das
@ 2018-06-15 17:42               ` Sudeep Holla
  0 siblings, 0 replies; 25+ messages in thread
From: Sudeep Holla @ 2018-06-15 17:42 UTC (permalink / raw)
  To: Taniya Das, linux-kernel, linux-pm
  Cc: Sudeep Holla, Rafael J. Wysocki, Viresh Kumar, Stephen Boyd,
	Rajendra Nayak, devicetree, robh, skannan



On 15/06/18 18:31, Taniya Das wrote:
> 
> 
> On 6/15/2018 6:53 PM, Sudeep Holla wrote:
>>

[...]

>>>
>>>> It should be easily extensible is what I am
>>>> trying to say. You can add more info and alter the information in the
>>>> driver with compatibles if you keep the register info as minimum as
>>>> possible. For now, you have enable, set and lut registers. What if you
>>>> want to provide power numbers ?
>>>>
>>>
>>> Yes I do understand the intent of mapping the whole register space, but
>>> as per the HW specs these 3 registers would be the only ones required
>>> for now. I do not think this hardware engine has any information on the
>>> power numbers.
>>>
>>
>> That's fine. So on this platform DT, will you list only the registers
>> touched by the OS for all the IP ? I am sure that will not be the case.
>>
> 
> Yes, registers list those would be touched by OS only.
> 

You are still missing the point.
Look at other IP blocks like pinmux/gpio/...(choose your pick).

E.g. Lets say gpio controller driver touches only status set and get
registers in a port, will you list then individually in the DT for 'n'
ports on the platform ?

-- 
Regards,
Sudeep

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

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-15 17:40             ` Taniya Das
@ 2018-06-15 17:45               ` Sudeep Holla
  2018-06-17  9:03               ` Amit Kucheria
  2018-06-18  9:21               ` Sudeep Holla
  2 siblings, 0 replies; 25+ messages in thread
From: Sudeep Holla @ 2018-06-15 17:45 UTC (permalink / raw)
  To: Taniya Das, Amit Kucheria
  Cc: Sudeep Holla, LKML, Linux PM list, Rafael J. Wysocki,
	Viresh Kumar, Stephen Boyd, Rajendra Nayak,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, Saravana Kannan



On 15/06/18 18:40, Taniya Das wrote:
> 
> 
> On 6/15/2018 5:29 PM, Amit Kucheria wrote:
>> On Thu, Jun 14, 2018 at 9:24 PM, Taniya Das <tdas@codeaurora.org> wrote:
>>
>>>>> Sorry Sudeep I missed replying to your earlier query.
>>>>> The High level OS(HLOS) would require to access only these specific
>>>>> registers from this IP block and just mapping the whole block(huge
>>>>> region) is unnecessary from the OS point of View. As of now it is a
>>>>> generic binding for all using this IP block to manage frequency
>>>>> requests. The OS would only have to know the frequencies supported i.e
>>>>> to read the lookup table registers and put across the OS request using
>>>>> the performance state register.
>>>>>
>>>>
>>>> I am not sure if you need to defining bindings to save OSPM IO mapping.
>>>> In-fact you may be adding more mapping unnecessarily. The mappings are
>>>> page aligned and spiting the registers and mapping them individually
>>>> may
>>>> result in more mappings.
>>>>
>>>> I just need to know the rational for such specific choice of registers.
>>>> I assume it's aligned to some other standard specifications like CPPC
>>>> though not identical.
>>>>
>>>
>>> I am not sure of the query but there is no other register that the OS is
>>> required to use other than the ones defined here.
>>>
>>>>>> Eg. Suppose you need some information on power curve for EAS energy
>>>>>> model, I really hate to update DT for that or even do a mix with
>>>>>> DT just
>>>>>> because f/w is no longer modifiable.
>>>>>>
>>>>>
>>>>> For now we are safe.
>>>>>
>>>>
>>>> What do you mean by that ?
>>>
>>>
>>> I meant here was currently there is no such known case where the f/w
>>> is no
>>> longer modifiable and we need to extend device tree bindings.
>>>
>>>> It should be easily extensible is what I am
>>>> trying to say. You can add more info and alter the information in the
>>>> driver with compatibles if you keep the register info as minimum as
>>>> possible. For now, you have enable, set and lut registers. What if you
>>>> want to provide power numbers ?
>>>>
>>>
>>> Yes I do understand the intent of mapping the whole register space,
>>> but as
>>> per the HW specs these 3 registers would be the only ones required
>>> for now.
>>> I do not think this hardware engine has any information on the power
>>> numbers.
>>
>> "For now" - I think this is exactly the point that Sudeep is trying to
>> make.
>>
>> A future version of the HW engine, or more likely, a firmware
>> revision, will make more functionality available. Say, this needs
>> access to another register or two. This will require changing the DT
>> bindings. Instead, if you map the entire address space, you can just
>> add offsets to the new registers.
>>
>> So in this case, I think you should define the following addresses
>> (size 0x1400) for the two frequency domains
>>
>> 0x17d43000, 0x1400 (power cluster)
>> 0x17d45800, 0x1400 (perf cluster)
>>
>> And in the driver simply add offsets as follows:
>>
>> #define ENABLE_OFFSET               0x0
>> #define LUT_OFFSET                      0x110
>> #define PERF_DESIRED_OFFSET 0x920
>>
> 
> The offsets could vary across versions of this IP and that is the reason
> to provide them through the DT and not define any such offsets.
> 

How many versions do you have and how much has it varied already ?
I am now getting a sense that it's mostly decided and fixed my the
firmware rather than at the time of hardware design.

-- 
Regards,
Sudeep

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

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-15 17:40             ` Taniya Das
  2018-06-15 17:45               ` Sudeep Holla
@ 2018-06-17  9:03               ` Amit Kucheria
  2018-06-18  9:21               ` Sudeep Holla
  2 siblings, 0 replies; 25+ messages in thread
From: Amit Kucheria @ 2018-06-17  9:03 UTC (permalink / raw)
  To: Taniya Das
  Cc: Sudeep Holla, LKML, Linux PM list, Rafael J. Wysocki,
	Viresh Kumar, Stephen Boyd, Rajendra Nayak,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, Saravana Kannan

On Fri, Jun 15, 2018 at 8:40 PM, Taniya Das <tdas@codeaurora.org> wrote:
>
>
> On 6/15/2018 5:29 PM, Amit Kucheria wrote:
>>
>> On Thu, Jun 14, 2018 at 9:24 PM, Taniya Das <tdas@codeaurora.org> wrote:
>>
>>>>> Sorry Sudeep I missed replying to your earlier query.
>>>>> The High level OS(HLOS) would require to access only these specific
>>>>> registers from this IP block and just mapping the whole block(huge
>>>>> region) is unnecessary from the OS point of View. As of now it is a
>>>>> generic binding for all using this IP block to manage frequency
>>>>> requests. The OS would only have to know the frequencies supported i.e
>>>>> to read the lookup table registers and put across the OS request using
>>>>> the performance state register.
>>>>>
>>>>
>>>> I am not sure if you need to defining bindings to save OSPM IO mapping.
>>>> In-fact you may be adding more mapping unnecessarily. The mappings are
>>>> page aligned and spiting the registers and mapping them individually may
>>>> result in more mappings.
>>>>
>>>> I just need to know the rational for such specific choice of registers.
>>>> I assume it's aligned to some other standard specifications like CPPC
>>>> though not identical.
>>>>
>>>
>>> I am not sure of the query but there is no other register that the OS is
>>> required to use other than the ones defined here.
>>>
>>>>>> Eg. Suppose you need some information on power curve for EAS energy
>>>>>> model, I really hate to update DT for that or even do a mix with DT
>>>>>> just
>>>>>> because f/w is no longer modifiable.
>>>>>>
>>>>>
>>>>> For now we are safe.
>>>>>
>>>>
>>>> What do you mean by that ?
>>>
>>>
>>>
>>> I meant here was currently there is no such known case where the f/w is
>>> no
>>> longer modifiable and we need to extend device tree bindings.
>>>
>>>> It should be easily extensible is what I am
>>>> trying to say. You can add more info and alter the information in the
>>>> driver with compatibles if you keep the register info as minimum as
>>>> possible. For now, you have enable, set and lut registers. What if you
>>>> want to provide power numbers ?
>>>>
>>>
>>> Yes I do understand the intent of mapping the whole register space, but
>>> as
>>> per the HW specs these 3 registers would be the only ones required for
>>> now.
>>> I do not think this hardware engine has any information on the power
>>> numbers.
>>
>>
>> "For now" - I think this is exactly the point that Sudeep is trying to
>> make.
>>
>> A future version of the HW engine, or more likely, a firmware
>> revision, will make more functionality available. Say, this needs
>> access to another register or two. This will require changing the DT
>> bindings. Instead, if you map the entire address space, you can just
>> add offsets to the new registers.
>>
>> So in this case, I think you should define the following addresses
>> (size 0x1400) for the two frequency domains
>>
>> 0x17d43000, 0x1400 (power cluster)
>> 0x17d45800, 0x1400 (perf cluster)
>>
>> And in the driver simply add offsets as follows:
>>
>> #define ENABLE_OFFSET               0x0
>> #define LUT_OFFSET                      0x110
>> #define PERF_DESIRED_OFFSET 0x920
>>
>
> The offsets could vary across versions of this IP and that is the reason to
> provide them through the DT and not define any such offsets.

If that is a known fact internally, you should already introduce
versioning information in the DT. e.g qcom,cpufreq-fw-v1. This will
give you the ability to deal with IP versions across SoC families.

We're currently trying to do the same thing for the TSENS IP.

Regards,
Amit

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

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-15 17:40             ` Taniya Das
  2018-06-15 17:45               ` Sudeep Holla
  2018-06-17  9:03               ` Amit Kucheria
@ 2018-06-18  9:21               ` Sudeep Holla
  2018-06-19  7:53                 ` Taniya Das
  2 siblings, 1 reply; 25+ messages in thread
From: Sudeep Holla @ 2018-06-18  9:21 UTC (permalink / raw)
  To: Taniya Das, Amit Kucheria
  Cc: Sudeep Holla, LKML, Linux PM list, Rafael J. Wysocki,
	Viresh Kumar, Stephen Boyd, Rajendra Nayak,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, Saravana Kannan



On 15/06/18 18:40, Taniya Das wrote:
> 
> 
> On 6/15/2018 5:29 PM, Amit Kucheria wrote:

[...]

>> A future version of the HW engine, or more likely, a firmware
>> revision, will make more functionality available. Say, this needs
>> access to another register or two. This will require changing the DT
>> bindings. Instead, if you map the entire address space, you can just
>> add offsets to the new registers.
>>
>> So in this case, I think you should define the following addresses
>> (size 0x1400) for the two frequency domains
>>
>> 0x17d43000, 0x1400 (power cluster)
>> 0x17d45800, 0x1400 (perf cluster)
>>
>> And in the driver simply add offsets as follows:
>>
>> #define ENABLE_OFFSET               0x0
>> #define LUT_OFFSET                      0x110
>> #define PERF_DESIRED_OFFSET 0x920
>>
> 
> The offsets could vary across versions of this IP and that is the reason
> to provide them through the DT and not define any such offsets.
> 

Just get compatibles to identify the version of the hardware if it can't
be probed and detected. Please don't use DT to get the addresses of each
register you use in the driver. That's neither scalable nor nice
solution to the problem.

-- 
Regards,
Sudeep

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

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-18  9:21               ` Sudeep Holla
@ 2018-06-19  7:53                 ` Taniya Das
  2018-06-19  9:21                   ` Viresh Kumar
  2018-06-19  9:34                   ` Sudeep Holla
  0 siblings, 2 replies; 25+ messages in thread
From: Taniya Das @ 2018-06-19  7:53 UTC (permalink / raw)
  To: Sudeep Holla, Amit Kucheria
  Cc: LKML, Linux PM list, Rafael J. Wysocki, Viresh Kumar,
	Stephen Boyd, Rajendra Nayak,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, Saravana Kannan



On 6/18/2018 2:51 PM, Sudeep Holla wrote:
> 
> 
> On 15/06/18 18:40, Taniya Das wrote:
>>
>>
>> On 6/15/2018 5:29 PM, Amit Kucheria wrote:
> 
> [...]
> 
>>> A future version of the HW engine, or more likely, a firmware
>>> revision, will make more functionality available. Say, this needs
>>> access to another register or two. This will require changing the DT
>>> bindings. Instead, if you map the entire address space, you can just
>>> add offsets to the new registers.
>>>
>>> So in this case, I think you should define the following addresses
>>> (size 0x1400) for the two frequency domains
>>>
>>> 0x17d43000, 0x1400 (power cluster)
>>> 0x17d45800, 0x1400 (perf cluster)
>>>
>>> And in the driver simply add offsets as follows:
>>>
>>> #define ENABLE_OFFSET               0x0
>>> #define LUT_OFFSET                      0x110
>>> #define PERF_DESIRED_OFFSET 0x920
>>>
>>
>> The offsets could vary across versions of this IP and that is the reason
>> to provide them through the DT and not define any such offsets.
>>
> 
> Just get compatibles to identify the version of the hardware if it can't
> be probed and detected. Please don't use DT to get the addresses of each
> register you use in the driver. That's neither scalable nor nice
> solution to the problem.
> Hello Sudeep and Amit,

Thanks for the comments, I am consolidating the understanding from the 
other emails in a single one.

I understand that you are looking for this IP to map the full region and 
define offsets according to access them.

But I still not sure how do you want this common driver to scale in the 
cases where the offsets could vary across version change.

  DT
====
   freq-node {
	reg = < X x_size>;   Where X is the start of the IP address.
   }

Driver code (The below representation is just for example).
=============

V1
#define ENABLE	0x0
#define LUT_V1	0x110
#define PERF_V1	0x920

V2
#define LUT_V2	0x150
#define PERF_V2	0x980

V3
#define LUT_V3	0x120
....

Do you want me to use "compatible" flag to

if (compatible == v1)
  enable =  readl_relaxed(X + LUT_V1);
else if (compatible == v2)
  enable = readl_relaxed(X + LUT_V2);
else if (compatible == v3)
  enable = readl_relaxed(X + LUT_V2);

With the current design I do not need such compatible checks and unmap 
the ones which are not required after probe. Please let me know your 
comments.

-- 
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] 25+ messages in thread

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-19  7:53                 ` Taniya Das
@ 2018-06-19  9:21                   ` Viresh Kumar
  2018-06-19  9:34                   ` Sudeep Holla
  1 sibling, 0 replies; 25+ messages in thread
From: Viresh Kumar @ 2018-06-19  9:21 UTC (permalink / raw)
  To: Taniya Das
  Cc: Sudeep Holla, Amit Kucheria, LKML, Linux PM list,
	Rafael J. Wysocki, Stephen Boyd, Rajendra Nayak,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, Saravana Kannan

On 19-06-18, 13:23, Taniya Das wrote:
> Driver code (The below representation is just for example).
> =============
> 
> V1
> #define ENABLE	0x0
> #define LUT_V1	0x110
> #define PERF_V1	0x920
> 
> V2
> #define LUT_V2	0x150
> #define PERF_V2	0x980
> 
> V3
> #define LUT_V3	0x120
> ....
> 
> Do you want me to use "compatible" flag to
> 
> if (compatible == v1)
>  enable =  readl_relaxed(X + LUT_V1);
> else if (compatible == v2)
>  enable = readl_relaxed(X + LUT_V2);
> else if (compatible == v3)
>  enable = readl_relaxed(X + LUT_V2);

You can have fields in a struct somewhere like enable_offset, which you can fill
based on compatible string only once during probe and then the rest of the code
would just do:

enable = readl_relaxed(X + struct->enable_offset);

-- 
viresh

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

* Re: [PATCH v4 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver
  2018-06-12 11:02 ` [PATCH v4 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver Taniya Das
  2018-06-15 12:02   ` Amit Kucheria
@ 2018-06-19  9:30   ` Viresh Kumar
  2018-07-11 20:37   ` Matthias Kaehlcke
  2 siblings, 0 replies; 25+ messages in thread
From: Viresh Kumar @ 2018-06-19  9:30 UTC (permalink / raw)
  To: Taniya Das
  Cc: Rafael J. Wysocki, linux-kernel, linux-pm, Stephen Boyd,
	Rajendra Nayak, devicetree, robh, skannan

On 12-06-18, 16:32, Taniya Das wrote:
> +static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
> +{
> +	struct device_node *cpu_np, *freq_np;
> +	int cpu;
> +
> +	for_each_possible_cpu(cpu) {
> +		cpu_np = of_cpu_device_node_get(cpu);
> +		if (!cpu_np)
> +			continue;
> +		freq_np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
> +		if (!freq_np)
> +			continue;
> +		if (freq_np == np)
> +			cpumask_set_cpu(cpu, m);
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_cpu_resources_init(struct platform_device *pdev,
> +				   struct device_node *np, unsigned int cpu)
> +{
> +	struct cpufreq_qcom *c;
> +	struct resource res;
> +	struct device *dev = &pdev->dev;
> +	void __iomem *en_base;
> +	int index, ret;
> +
> +	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> +	if (!c)
> +		return -ENOMEM;
> +
> +	index = of_property_match_string(np, "reg-names", "enable");
> +	if (index < 0)
> +		return index;
> +
> +	if (of_address_to_resource(np, index, &res))
> +		return -ENOMEM;
> +
> +	en_base = devm_ioremap(dev, res.start, resource_size(&res));
> +	if (!en_base) {
> +		dev_err(dev, "Unable to map %s enable-base\n", np->name);
> +		return -ENOMEM;
> +	}
> +
> +	/* FW should be in enabled state to proceed */
> +	if (!(readl_relaxed(en_base) & 0x1)) {
> +		dev_err(dev, "%s firmware not enabled\n", np->name);
> +		return -ENODEV;
> +	}
> +	devm_iounmap(&pdev->dev, en_base);
> +
> +	index = of_property_match_string(np, "reg-names", "perf");
> +	if (index < 0)
> +		return index;
> +
> +	if (of_address_to_resource(np, index, &res))
> +		return -ENOMEM;
> +
> +	c->perf_base = devm_ioremap(dev, res.start, resource_size(&res));
> +	if (!c->perf_base) {
> +		dev_err(dev, "Unable to map %s perf-base\n", np->name);
> +		return -ENOMEM;
> +	}
> +
> +	index = of_property_match_string(np, "reg-names", "lut");
> +	if (index < 0)
> +		return index;
> +
> +	if (of_address_to_resource(np, index, &res))
> +		return -ENOMEM;
> +
> +	c->lut_base = devm_ioremap(dev, res.start, resource_size(&res));
> +	if (!c->lut_base) {
> +		dev_err(dev, "Unable to map %s lut-base\n", np->name);
> +		return -ENOMEM;
> +	}
> +
> +	ret = qcom_get_related_cpus(np, &c->related_cpus);
> +	if (ret) {
> +		dev_err(dev, "%s failed to get related CPUs\n", np->name);
> +		return ret;
> +	}
> +
> +	c->max_cores = cpumask_weight(&c->related_cpus);
> +	if (!c->max_cores)
> +		return -ENOENT;
> +
> +	ret = qcom_read_lut(pdev, c);
> +	if (ret) {
> +		dev_err(dev, "%s failed to read LUT\n", np->name);
> +		return ret;
> +	}
> +
> +	qcom_freq_domain_map[cpu] = c;

This still looks wrong. You have removed the for-each-cpu loop here, so what
will happen now is that you will allocate a different "struct cpufreq_qcom" for
every CPU, even if they are related. Is that what you should be doing ? I think
there should still be a single copy of that structure which must be used by all
related CPUs.

> +
> +	return 0;
> +}
> +
> +static int qcom_resources_init(struct platform_device *pdev)
> +{
> +	struct device_node *np, *cpu_np;
> +	unsigned int cpu;
> +	int ret;
> +
> +	for_each_possible_cpu(cpu) {
> +		cpu_np = of_cpu_device_node_get(cpu);
> +		if (!cpu_np) {
> +			dev_err(&pdev->dev, "Failed to get cpu %d device\n",
> +				cpu);
> +			continue;
> +		}
> +
> +		np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
> +		if (!np) {
> +			dev_err(&pdev->dev, "Failed to get freq-domain device\n");
> +			return -EINVAL;
> +		}
> +
> +		of_node_put(cpu_np);
> +
> +		ret = qcom_cpu_resources_init(pdev, np, cpu);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

-- 
viresh

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

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-19  7:53                 ` Taniya Das
  2018-06-19  9:21                   ` Viresh Kumar
@ 2018-06-19  9:34                   ` Sudeep Holla
  2018-06-19 10:44                     ` Taniya Das
  1 sibling, 1 reply; 25+ messages in thread
From: Sudeep Holla @ 2018-06-19  9:34 UTC (permalink / raw)
  To: Taniya Das
  Cc: Amit Kucheria, Sudeep Holla, LKML, Linux PM list,
	Rafael J. Wysocki, Viresh Kumar, Stephen Boyd, Rajendra Nayak,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, Saravana Kannan



On 19/06/18 08:53, Taniya Das wrote:
> 
> 
> On 6/18/2018 2:51 PM, Sudeep Holla wrote:
>>
>>
>> On 15/06/18 18:40, Taniya Das wrote:
>>>
>>>
>>> On 6/15/2018 5:29 PM, Amit Kucheria wrote:
>>
>> [...]
>>
>>>> A future version of the HW engine, or more likely, a firmware
>>>> revision, will make more functionality available. Say, this needs
>>>> access to another register or two. This will require changing the DT
>>>> bindings. Instead, if you map the entire address space, you can just
>>>> add offsets to the new registers.
>>>>
>>>> So in this case, I think you should define the following addresses
>>>> (size 0x1400) for the two frequency domains
>>>>
>>>> 0x17d43000, 0x1400 (power cluster)
>>>> 0x17d45800, 0x1400 (perf cluster)
>>>>
>>>> And in the driver simply add offsets as follows:
>>>>
>>>> #define ENABLE_OFFSET               0x0
>>>> #define LUT_OFFSET                      0x110
>>>> #define PERF_DESIRED_OFFSET 0x920
>>>>
>>>
>>> The offsets could vary across versions of this IP and that is the reason
>>> to provide them through the DT and not define any such offsets.
>>>
>>
>> Just get compatibles to identify the version of the hardware if it can't
>> be probed and detected. Please don't use DT to get the addresses of each
>> register you use in the driver. That's neither scalable nor nice
>> solution to the problem.
>> Hello Sudeep and Amit,
> 
> Thanks for the comments, I am consolidating the understanding from the
> other emails in a single one.
> 
> I understand that you are looking for this IP to map the full region and
> define offsets according to access them.
> 
> But I still not sure how do you want this common driver to scale in the
> cases where the offsets could vary across version change.
> 

There are plenty of drivers that you can look at as example. TBH most of
the drivers implementing support for multiple versions of IP do
something on the similar lines.

>  DT
> ====
>   freq-node {
>     reg = < X x_size>;   Where X is the start of the IP address.
>   }
> 
> Driver code (The below representation is just for example).
> =============
> 
> V1
> #define ENABLE    0x0
> #define LUT_V1    0x110
> #define PERF_V1    0x920
> 
> V2
> #define LUT_V2    0x150
> #define PERF_V2    0x980
> 
> V3
> #define LUT_V3    0x120
> ....
> 
> Do you want me to use "compatible" flag to
> 
> if (compatible == v1)
>  enable =  readl_relaxed(X + LUT_V1);
> else if (compatible == v2)
>  enable = readl_relaxed(X + LUT_V2);
> else if (compatible == v3)
>  enable = readl_relaxed(X + LUT_V2);
> 

These are implementation details. But you should try to use compatibles
only in probe and just record the version in some variable or update the
offsets in some device specific structure so that you can use that
unconditionally for any access you make on that device.

> With the current design I do not need such compatible checks and unmap
> the ones which are not required after probe.

I am not sure what you mean by unmap after probe.

> Please let me know your comments.
> 

Please look at some drivers in the Linux tree for examples. Infact there
may be few drivers on QCOM SoC itself. What I am suggesting is the normal
practice in the drivers and you should see plenty of examples. Since I
was looking at some serial port patch, I can say you can have a look at
drivers/tty/serial/amba-pl011.c which supports multiple versions from
different vendors. I am sure there are many simpler examples but AMBA PL011
just stood out.

-- 
Regards,
Sudeep

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

* Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  2018-06-19  9:34                   ` Sudeep Holla
@ 2018-06-19 10:44                     ` Taniya Das
  0 siblings, 0 replies; 25+ messages in thread
From: Taniya Das @ 2018-06-19 10:44 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Amit Kucheria, LKML, Linux PM list, Rafael J. Wysocki,
	Viresh Kumar, Stephen Boyd, Rajendra Nayak,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, Saravana Kannan



On 6/19/2018 3:04 PM, Sudeep Holla wrote:
> 
> 
> On 19/06/18 08:53, Taniya Das wrote:
>>
>>
>> On 6/18/2018 2:51 PM, Sudeep Holla wrote:
>>>
>>>
>>> On 15/06/18 18:40, Taniya Das wrote:
>>>>
>>>>
>>>> On 6/15/2018 5:29 PM, Amit Kucheria wrote:
>>>
>>> [...]
>>>
>>>>> A future version of the HW engine, or more likely, a firmware
>>>>> revision, will make more functionality available. Say, this needs
>>>>> access to another register or two. This will require changing the DT
>>>>> bindings. Instead, if you map the entire address space, you can just
>>>>> add offsets to the new registers.
>>>>>
>>>>> So in this case, I think you should define the following addresses
>>>>> (size 0x1400) for the two frequency domains
>>>>>
>>>>> 0x17d43000, 0x1400 (power cluster)
>>>>> 0x17d45800, 0x1400 (perf cluster)
>>>>>
>>>>> And in the driver simply add offsets as follows:
>>>>>
>>>>> #define ENABLE_OFFSET               0x0
>>>>> #define LUT_OFFSET                      0x110
>>>>> #define PERF_DESIRED_OFFSET 0x920
>>>>>
>>>>
>>>> The offsets could vary across versions of this IP and that is the reason
>>>> to provide them through the DT and not define any such offsets.
>>>>
>>>
>>> Just get compatibles to identify the version of the hardware if it can't
>>> be probed and detected. Please don't use DT to get the addresses of each
>>> register you use in the driver. That's neither scalable nor nice
>>> solution to the problem.
>>> Hello Sudeep and Amit,
>>
>> Thanks for the comments, I am consolidating the understanding from the
>> other emails in a single one.
>>
>> I understand that you are looking for this IP to map the full region and
>> define offsets according to access them.
>>
>> But I still not sure how do you want this common driver to scale in the
>> cases where the offsets could vary across version change.
>>
> 
> There are plenty of drivers that you can look at as example. TBH most of
> the drivers implementing support for multiple versions of IP do
> something on the similar lines.
> 
>>   DT
>> ====
>>    freq-node {
>>      reg = < X x_size>;   Where X is the start of the IP address.
>>    }
>>
>> Driver code (The below representation is just for example).
>> =============
>>
>> V1
>> #define ENABLE    0x0
>> #define LUT_V1    0x110
>> #define PERF_V1    0x920
>>
>> V2
>> #define LUT_V2    0x150
>> #define PERF_V2    0x980
>>
>> V3
>> #define LUT_V3    0x120
>> ....
>>
>> Do you want me to use "compatible" flag to
>>
>> if (compatible == v1)
>>   enable =  readl_relaxed(X + LUT_V1);
>> else if (compatible == v2)
>>   enable = readl_relaxed(X + LUT_V2);
>> else if (compatible == v3)
>>   enable = readl_relaxed(X + LUT_V2);
>>
> 
> These are implementation details. But you should try to use compatibles
> only in probe and just record the version in some variable or update the
> offsets in some device specific structure so that you can use that
> unconditionally for any access you make on that device.
> 
>> With the current design I do not need such compatible checks and unmap
>> the ones which are not required after probe.
> 
> I am not sure what you mean by unmap after probe.
> 
>> Please let me know your comments.
>>
> 
> Please look at some drivers in the Linux tree for examples. Infact there
> may be few drivers on QCOM SoC itself. What I am suggesting is the normal
> practice in the drivers and you should see plenty of examples. Since I
> was looking at some serial port patch, I can say you can have a look at
> drivers/tty/serial/amba-pl011.c which supports multiple versions from
> different vendors. I am sure there are many simpler examples but AMBA PL011
> just stood out.
> 

Thanks Sudeep, let me take a look at the driver to see how I can 
associate data (offsets) based on compatible.

-- 
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] 25+ messages in thread

* Re: [PATCH v4 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver
  2018-06-12 11:02 ` [PATCH v4 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver Taniya Das
  2018-06-15 12:02   ` Amit Kucheria
  2018-06-19  9:30   ` Viresh Kumar
@ 2018-07-11 20:37   ` Matthias Kaehlcke
  2018-07-12 18:06     ` Taniya Das
  2 siblings, 1 reply; 25+ messages in thread
From: Matthias Kaehlcke @ 2018-07-11 20:37 UTC (permalink / raw)
  To: Taniya Das
  Cc: Rafael J. Wysocki, Viresh Kumar, linux-kernel, linux-pm,
	Stephen Boyd, Rajendra Nayak, devicetree, robh, skannan

Hi,

On Tue, Jun 12, 2018 at 04:32:35PM +0530, Taniya Das wrote:
> The CPUfreq FW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this firmware.
> 
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  drivers/cpufreq/Kconfig.arm       |   9 +
>  drivers/cpufreq/Makefile          |   1 +
>  drivers/cpufreq/qcom-cpufreq-fw.c | 336 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 346 insertions(+)
>  create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 52f5f1a..2683716 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -312,3 +312,12 @@ config ARM_PXA2xx_CPUFREQ
>  	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
> 
>  	  If in doubt, say N.
> +
> +config ARM_QCOM_CPUFREQ_FW
> +	bool "QCOM CPUFreq FW driver"
> +	help
> +	 Support for the CPUFreq FW driver.
> +	 The CPUfreq FW preset in some QCOM chipsets offloads the steps
> +	 necessary for changing the frequency of CPUs. The driver
> +	 implements the cpufreq driver interface for this firmware.
> +	 Say Y if you want to support CPUFreq FW.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index fb4a2ec..34691a2 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -86,6 +86,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
>  obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
>  obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
>  obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_FW)	+= qcom-cpufreq-fw.o
> 
> 
>  ##################################################################################
> diff --git a/drivers/cpufreq/qcom-cpufreq-fw.c b/drivers/cpufreq/qcom-cpufreq-fw.c
> new file mode 100644
> index 0000000..62f4452
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-fw.c
> @@ -0,0 +1,336 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/cpufreq.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +
> +#define INIT_RATE			300000000UL
> +#define XO_RATE				19200000UL
> +#define LUT_MAX_ENTRIES			40U
> +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
> +#define LUT_ROW_SIZE			32
> +
> +struct cpufreq_qcom {
> +	struct cpufreq_frequency_table *table;
> +	struct device *dev;
> +	void __iomem *perf_base;
> +	void __iomem *lut_base;
> +	cpumask_t related_cpus;
> +	unsigned int max_cores;

Why *max*_cores? This seems to be the number of CPUs in a cluster and
qcom_read_lut() expects the core count read from the LUT to match
exactly.

> +static int qcom_read_lut(struct platform_device *pdev,
> +			 struct cpufreq_qcom *c)
> +{
> +	struct device *dev = &pdev->dev;
> +	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
> +
> +	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> +				sizeof(*c->table), GFP_KERNEL);
> +	if (!c->table)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> +		data = readl_relaxed(c->lut_base + i * LUT_ROW_SIZE);
> +		src = ((data & GENMASK(31, 30)) >> 30);
> +		lval = (data & GENMASK(7, 0));
> +		core_count = CORE_COUNT_VAL(data);
> +
> +		if (!src)
> +			c->table[i].frequency = INIT_RATE / 1000;
> +		else
> +			c->table[i].frequency = XO_RATE * lval / 1000;

nit: any particular reason to use negative logic here? Why not check
for 'src[ != NULL]', which also seems to be the more common case.

> +static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
> +{
> +	struct device_node *cpu_np, *freq_np;
> +	int cpu;
> +
> +	for_each_possible_cpu(cpu) {
> +		cpu_np = of_cpu_device_node_get(cpu);
> +		if (!cpu_np)
> +			continue;
> +		freq_np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
> +		if (!freq_np)
> +			continue;
> +		if (freq_np == np)
> +			cpumask_set_cpu(cpu, m);

missing 'of_node_put(cpu_np)'. You might want to do it at the end of
the loop and use a 'goto' above instead of 'continue'.

> +static int qcom_cpu_resources_init(struct platform_device *pdev,
> +				   struct device_node *np, unsigned int cpu)
> +{
> +	struct cpufreq_qcom *c;
> +	struct resource res;
> +	struct device *dev = &pdev->dev;
> +	void __iomem *en_base;
> +	int index, ret;
> +
> +	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> +	if (!c)
> +		return -ENOMEM;
> +
> +	index = of_property_match_string(np, "reg-names", "enable");
> +	if (index < 0)
> +		return index;
> +
> +	if (of_address_to_resource(np, index, &res))
> +		return -ENOMEM;
> +
> +	en_base = devm_ioremap(dev, res.start, resource_size(&res));
> +	if (!en_base) {
> +		dev_err(dev, "Unable to map %s enable-base\n", np->name);
> +		return -ENOMEM;
> +	}
> +
> +	/* FW should be in enabled state to proceed */
> +	if (!(readl_relaxed(en_base) & 0x1)) {
> +		dev_err(dev, "%s firmware not enabled\n", np->name);
> +		return -ENODEV;
> +	}
> +	devm_iounmap(&pdev->dev, en_base);
> +
> +	index = of_property_match_string(np, "reg-names", "perf");
> +	if (index < 0)
> +		return index;
> +
> +	if (of_address_to_resource(np, index, &res))
> +		return -ENOMEM;
> +
> +	c->perf_base = devm_ioremap(dev, res.start, resource_size(&res));
> +	if (!c->perf_base) {
> +		dev_err(dev, "Unable to map %s perf-base\n", np->name);
> +		return -ENOMEM;
> +	}
> +
> +	index = of_property_match_string(np, "reg-names", "lut");
> +	if (index < 0)
> +		return index;
> +
> +	if (of_address_to_resource(np, index, &res))
> +		return -ENOMEM;
> +
> +	c->lut_base = devm_ioremap(dev, res.start, resource_size(&res));
> +	if (!c->lut_base) {
> +		dev_err(dev, "Unable to map %s lut-base\n", np->name);
> +		return -ENOMEM;
> +	}

The of_property_match_string() - of_address_to_resource() -
devm_ioremap() pattern is repeated 3x. In case the binding doesn't
change (there is discussion on the DT patch) you might want to move
this to a helper.

> +static int qcom_resources_init(struct platform_device *pdev)
> +{
> +	struct device_node *np, *cpu_np;
> +	unsigned int cpu;
> +	int ret;
> +
> +	for_each_possible_cpu(cpu) {
> +		cpu_np = of_cpu_device_node_get(cpu);
> +		if (!cpu_np) {
> +			dev_err(&pdev->dev, "Failed to get cpu %d device\n",
> +				cpu);
> +			continue;
> +		}
> +
> +		np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
> +		if (!np) {
> +			dev_err(&pdev->dev, "Failed to get freq-domain device\n");
			of_node_put(cpu_np);
> +			return -EINVAL;
> +		}
> +
> +		of_node_put(cpu_np);
> +
> +		ret = qcom_cpu_resources_init(pdev, np, cpu);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;

Cheers

Matthias

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

* Re: [PATCH v4 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver
  2018-07-11 20:37   ` Matthias Kaehlcke
@ 2018-07-12 18:06     ` Taniya Das
  0 siblings, 0 replies; 25+ messages in thread
From: Taniya Das @ 2018-07-12 18:06 UTC (permalink / raw)
  To: Matthias Kaehlcke, Viresh Kumar, robh, Sudeep Holla, Amit Kucheria
  Cc: Rafael J. Wysocki, linux-kernel, linux-pm, Stephen Boyd,
	Rajendra Nayak, devicetree, skannan

Please help review of the new series[v5] which takes care of the below.

- 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

On 7/12/2018 2:07 AM, Matthias Kaehlcke wrote:
> Hi,
> 
> On Tue, Jun 12, 2018 at 04:32:35PM +0530, Taniya Das wrote:
>> The CPUfreq FW present in some QCOM chipsets offloads the steps necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this firmware.
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> ---
>>   drivers/cpufreq/Kconfig.arm       |   9 +
>>   drivers/cpufreq/Makefile          |   1 +
>>   drivers/cpufreq/qcom-cpufreq-fw.c | 336 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 346 insertions(+)
>>   create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 52f5f1a..2683716 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -312,3 +312,12 @@ config ARM_PXA2xx_CPUFREQ
>>   	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
>>
>>   	  If in doubt, say N.
>> +
>> +config ARM_QCOM_CPUFREQ_FW
>> +	bool "QCOM CPUFreq FW driver"
>> +	help
>> +	 Support for the CPUFreq FW driver.
>> +	 The CPUfreq FW preset in some QCOM chipsets offloads the steps
>> +	 necessary for changing the frequency of CPUs. The driver
>> +	 implements the cpufreq driver interface for this firmware.
>> +	 Say Y if you want to support CPUFreq FW.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index fb4a2ec..34691a2 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -86,6 +86,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
>>   obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
>>   obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
>>   obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
>> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_FW)	+= qcom-cpufreq-fw.o
>>
>>
>>   ##################################################################################
>> diff --git a/drivers/cpufreq/qcom-cpufreq-fw.c b/drivers/cpufreq/qcom-cpufreq-fw.c
>> new file mode 100644
>> index 0000000..62f4452
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-fw.c
>> @@ -0,0 +1,336 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/cpufreq.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +
>> +#define INIT_RATE			300000000UL
>> +#define XO_RATE				19200000UL
>> +#define LUT_MAX_ENTRIES			40U
>> +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
>> +#define LUT_ROW_SIZE			32
>> +
>> +struct cpufreq_qcom {
>> +	struct cpufreq_frequency_table *table;
>> +	struct device *dev;
>> +	void __iomem *perf_base;
>> +	void __iomem *lut_base;
>> +	cpumask_t related_cpus;
>> +	unsigned int max_cores;
> 
> Why *max*_cores? This seems to be the number of CPUs in a cluster and
> qcom_read_lut() expects the core count read from the LUT to match
> exactly.
> 
>> +static int qcom_read_lut(struct platform_device *pdev,
>> +			 struct cpufreq_qcom *c)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
>> +
>> +	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
>> +				sizeof(*c->table), GFP_KERNEL);
>> +	if (!c->table)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> +		data = readl_relaxed(c->lut_base + i * LUT_ROW_SIZE);
>> +		src = ((data & GENMASK(31, 30)) >> 30);
>> +		lval = (data & GENMASK(7, 0));
>> +		core_count = CORE_COUNT_VAL(data);
>> +
>> +		if (!src)
>> +			c->table[i].frequency = INIT_RATE / 1000;
>> +		else
>> +			c->table[i].frequency = XO_RATE * lval / 1000;
> 
> nit: any particular reason to use negative logic here? Why not check
> for 'src[ != NULL]', which also seems to be the more common case.
> 
>> +static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
>> +{
>> +	struct device_node *cpu_np, *freq_np;
>> +	int cpu;
>> +
>> +	for_each_possible_cpu(cpu) {
>> +		cpu_np = of_cpu_device_node_get(cpu);
>> +		if (!cpu_np)
>> +			continue;
>> +		freq_np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
>> +		if (!freq_np)
>> +			continue;
>> +		if (freq_np == np)
>> +			cpumask_set_cpu(cpu, m);
> 
> missing 'of_node_put(cpu_np)'. You might want to do it at the end of
> the loop and use a 'goto' above instead of 'continue'.
> 
>> +static int qcom_cpu_resources_init(struct platform_device *pdev,
>> +				   struct device_node *np, unsigned int cpu)
>> +{
>> +	struct cpufreq_qcom *c;
>> +	struct resource res;
>> +	struct device *dev = &pdev->dev;
>> +	void __iomem *en_base;
>> +	int index, ret;
>> +
>> +	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
>> +	if (!c)
>> +		return -ENOMEM;
>> +
>> +	index = of_property_match_string(np, "reg-names", "enable");
>> +	if (index < 0)
>> +		return index;
>> +
>> +	if (of_address_to_resource(np, index, &res))
>> +		return -ENOMEM;
>> +
>> +	en_base = devm_ioremap(dev, res.start, resource_size(&res));
>> +	if (!en_base) {
>> +		dev_err(dev, "Unable to map %s enable-base\n", np->name);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	/* FW should be in enabled state to proceed */
>> +	if (!(readl_relaxed(en_base) & 0x1)) {
>> +		dev_err(dev, "%s firmware not enabled\n", np->name);
>> +		return -ENODEV;
>> +	}
>> +	devm_iounmap(&pdev->dev, en_base);
>> +
>> +	index = of_property_match_string(np, "reg-names", "perf");
>> +	if (index < 0)
>> +		return index;
>> +
>> +	if (of_address_to_resource(np, index, &res))
>> +		return -ENOMEM;
>> +
>> +	c->perf_base = devm_ioremap(dev, res.start, resource_size(&res));
>> +	if (!c->perf_base) {
>> +		dev_err(dev, "Unable to map %s perf-base\n", np->name);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	index = of_property_match_string(np, "reg-names", "lut");
>> +	if (index < 0)
>> +		return index;
>> +
>> +	if (of_address_to_resource(np, index, &res))
>> +		return -ENOMEM;
>> +
>> +	c->lut_base = devm_ioremap(dev, res.start, resource_size(&res));
>> +	if (!c->lut_base) {
>> +		dev_err(dev, "Unable to map %s lut-base\n", np->name);
>> +		return -ENOMEM;
>> +	}
> 
> The of_property_match_string() - of_address_to_resource() -
> devm_ioremap() pattern is repeated 3x. In case the binding doesn't
> change (there is discussion on the DT patch) you might want to move
> this to a helper.
> 
>> +static int qcom_resources_init(struct platform_device *pdev)
>> +{
>> +	struct device_node *np, *cpu_np;
>> +	unsigned int cpu;
>> +	int ret;
>> +
>> +	for_each_possible_cpu(cpu) {
>> +		cpu_np = of_cpu_device_node_get(cpu);
>> +		if (!cpu_np) {
>> +			dev_err(&pdev->dev, "Failed to get cpu %d device\n",
>> +				cpu);
>> +			continue;
>> +		}
>> +
>> +		np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
>> +		if (!np) {
>> +			dev_err(&pdev->dev, "Failed to get freq-domain device\n");
> 			of_node_put(cpu_np);
>> +			return -EINVAL;
>> +		}
>> +
>> +		of_node_put(cpu_np);
>> +
>> +		ret = qcom_cpu_resources_init(pdev, np, cpu);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
> 
> Cheers
> 
> Matthias
> 

-- 
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] 25+ messages in thread

end of thread, back to index

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-12 11:02 [PATCH v4 0/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver Taniya Das
2018-06-12 11:02 ` [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings Taniya Das
2018-06-13 11:26   ` Sudeep Holla
2018-06-13 18:13     ` Taniya Das
2018-06-14 10:47       ` Sudeep Holla
2018-06-14 18:24         ` Taniya Das
2018-06-15 11:59           ` Amit Kucheria
2018-06-15 13:27             ` Sudeep Holla
2018-06-15 17:40             ` Taniya Das
2018-06-15 17:45               ` Sudeep Holla
2018-06-17  9:03               ` Amit Kucheria
2018-06-18  9:21               ` Sudeep Holla
2018-06-19  7:53                 ` Taniya Das
2018-06-19  9:21                   ` Viresh Kumar
2018-06-19  9:34                   ` Sudeep Holla
2018-06-19 10:44                     ` Taniya Das
2018-06-15 13:23           ` Sudeep Holla
2018-06-15 17:31             ` Taniya Das
2018-06-15 17:42               ` Sudeep Holla
2018-06-15 13:07   ` Amit Kucheria
2018-06-12 11:02 ` [PATCH v4 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver Taniya Das
2018-06-15 12:02   ` Amit Kucheria
2018-06-19  9:30   ` Viresh Kumar
2018-07-11 20:37   ` Matthias Kaehlcke
2018-07-12 18:06     ` Taniya Das

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox