[v0 0/2] Add support for QCOM cpufreq FW driver

[v0 0/2] Add support for QCOM cpufreq FW driver

[Taniya Das]

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

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

 .../bindings/cpufreq/cpufreq-qcom-fw.txt           |  68 +++++
 drivers/cpufreq/Kconfig.arm                        |   9 +
 drivers/cpufreq/Makefile                           |   1 +
 drivers/cpufreq/qcom-cpufreq-fw.c                  | 318 +++++++++++++++++++++
 4 files changed, 396 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.

[v0 1/2] dt-bindings: clock: Introduce QCOM CPUFREQ FW bindings

[Taniya Das]

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

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 .../bindings/cpufreq/cpufreq-qcom-fw.txt           | 68 ++++++++++++++++++++++
 1 file changed, 68 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..bc912f4
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
@@ -0,0 +1,68 @@
+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".
+
+Note that #address-cells, #size-cells, and ranges shall be present to ensure
+the cpufreq can address a freq-domain registers.
+
+A freq-domain sub-node would be defined for the cpus with the following
+properties:
+
+- compatible:
+	Usage:		required
+	Value type:	<string>
+	Definition:	must be "cpufreq".
+
+- reg
+	Usage:		required
+	Value type:	<prop-encoded-array>
+	Definition:	Addresses and sizes for the memory of the perf_base
+			, lut_base and en_base.
+- reg-names
+	Usage:		required
+	Value type:	<stringlist>
+	Definition:	Address names. Must be "perf_base", "lut_base",
+			"en_base".
+			Must be specified in the same order as the
+			corresponding addresses are specified in the reg
+			property.
+
+- qcom,cpulist
+	Usage:		required
+	Value type:	<phandles of CPU>
+	Definition:	List of related cpu handles which are under a cluster.
+
+Example:
+	qcom,cpufreq-fw {
+		compatible = "qcom,cpufreq-fw";
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		freq-domain-0 {
+			compatible = "cpufreq";
+			reg = <0x17d43920 0x4>,
+			     <0x17d43110 0x500>,
+			     <0x17d41000 0x4>;
+			reg-names = "perf_base", "lut_base", "en_base";
+			qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3>;
+		};
+
+		freq-domain-1 {
+			compatible = "cpufreq";
+			reg = <0x17d46120 0x4>,
+			    <0x17d45910 0x500>,
+			    <0x17d45800 0x4>;
+			reg-names = "perf_base", "lut_base", "en_base";
+			qcom,cpulist = <&CPU4 &CPU5 &CPU6 &CPU7>;
+		};
+	};
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

[v0 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

[Taniya Das]

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

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

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 96b35b8..a50aa6e 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -301,3 +301,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
+	tristate "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 8d24ade..a3edbce 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
 obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
 obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
 obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
+obj-$(CONFIG_ARM_QCOM_CPUFREQ_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..67996d5
--- /dev/null
+++ b/drivers/cpufreq/qcom-cpufreq-fw.c
@@ -0,0 +1,318 @@
+// 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;
+
+	if (index >= LUT_MAX_ENTRIES) {
+		dev_err(c->dev,
+		"Passing an index (%u) that's greater than max (%d)\n",
+					index, LUT_MAX_ENTRIES - 1);
+		return -EINVAL;
+	}
+
+	writel_relaxed(index, c->perf_base);
+
+	/* Make sure the write goes through before proceeding */
+	mb();
+	return 0;
+}
+
+static unsigned int qcom_cpufreq_fw_get(unsigned int cpu)
+{
+	struct cpufreq_qcom *c;
+	unsigned int index;
+
+	c = qcom_freq_domain_map[cpu];
+	if (!c)
+		return -ENODEV;
+
+	index = readl_relaxed(c->perf_base);
+	index = min(index, LUT_MAX_ENTRIES - 1);
+
+	return c->table[index].frequency;
+}
+
+static int qcom_cpufreq_fw_cpu_init(struct cpufreq_policy *policy)
+{
+	struct cpufreq_qcom *c;
+	int ret;
+
+	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->table = c->table;
+	policy->driver_data = c;
+
+	return ret;
+}
+
+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,
+	.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 = 0;
+
+	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;
+
+		c->table[i].driver_data = 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)
+			c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
+
+		/*
+		 * Two of the same frequencies with the same core counts means
+		 * end of table.
+		 */
+		if (i > 0 && c->table[i - 1].driver_data ==
+					c->table[i].driver_data
+					&& prev_cc == core_count) {
+			struct cpufreq_frequency_table *prev = &c->table[i - 1];
+
+			if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
+				prev->flags = CPUFREQ_BOOST_FREQ;
+				prev->frequency = prev->driver_data;
+			}
+
+			break;
+		}
+		prev_cc = core_count;
+	}
+	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 *dev_phandle;
+	struct device *cpu_dev;
+	int cpu, i = 0, ret = -ENOENT;
+
+	dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
+	while (dev_phandle) {
+		for_each_possible_cpu(cpu) {
+			cpu_dev = get_cpu_device(cpu);
+			if (cpu_dev && cpu_dev->of_node == dev_phandle) {
+				cpumask_set_cpu(cpu, m);
+				ret = 0;
+				break;
+			}
+		}
+		dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
+	}
+
+	return ret;
+}
+
+static int qcom_cpu_resources_init(struct platform_device *pdev,
+						struct device_node *np)
+{
+	struct cpufreq_qcom *c;
+	struct resource res;
+	struct device *dev = &pdev->dev;
+	void __iomem *en_base;
+	int cpu, index = 0, ret;
+
+	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+
+	res = platform_get_resource_byname(dev, IORESOURCE_MEM, "en_base");
+	if (!res) {
+		dev_err(dev, "Enable base not defined for %s\n", np->name);
+		return ret;
+	}
+
+	en_base = devm_ioremap(dev, res->start, resource_size(res));
+	if (!en_base) {
+		dev_err(dev, "Unable to map %s en-base\n", np->name);
+		return -ENOMEM;
+	}
+
+	/* FW should be 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_base");
+	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_base");
+	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 core phandles\n", np->name);
+		return ret;
+	}
+
+	c->max_cores = cpumask_weight(&c->related_cpus);
+
+	ret = qcom_read_lut(pdev, c);
+	if (ret) {
+		dev_err(dev, "%s failed to read LUT\n", np->name);
+		return ret;
+	}
+
+	for_each_cpu(cpu, &c->related_cpus)
+		qcom_freq_domain_map[cpu] = c;
+
+	return 0;
+}
+
+static int qcom_resources_init(struct platform_device *pdev)
+{
+	struct device_node *np;
+	int ret = -ENODEV;
+
+	for_each_available_child_of_node(pdev->dev.of_node, np) {
+		if (of_device_is_compatible(np, "cpufreq")) {
+			ret = qcom_cpu_resources_init(pdev, np);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return ret;
+}
+
+static int qcom_cpufreq_fw_driver_probe(struct platform_device *pdev)
+{
+	int rc = 0;
+
+	/* 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 inited\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);
+
+static void __exit qcom_cpufreq_fw_exit(void)
+{
+	platform_driver_unregister(&qcom_cpufreq_fw_driver);
+}
+module_exit(qcom_cpufreq_fw_exit);
+
+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.

Re: [v0 0/2] Add support for QCOM cpufreq FW driver

[Amit Kucheria]

On Thu, May 17, 2018 at 12:29 PM, Taniya Das <tdas@codeaurora.org> wrote:
> The CPUfreq FW present in some QCOM chipsets offloads the steps necessary
> for hanging the frequency of CPUs. The driver implements the cpufreq driver

s/hanging/changing :-)

> interface for this firmware.
>
> Taniya Das (2):
>   dt-bindings: clock: Introduce QCOM CPUFREQ FW bindings
>   cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver
>
>  .../bindings/cpufreq/cpufreq-qcom-fw.txt           |  68 +++++
>  drivers/cpufreq/Kconfig.arm                        |   9 +
>  drivers/cpufreq/Makefile                           |   1 +
>  drivers/cpufreq/qcom-cpufreq-fw.c                  | 318 +++++++++++++++++++++
>  4 files changed, 396 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.
>

Re: [v0 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

[Viresh Kumar]

On 17-05-18, 15:00, Taniya Das wrote:
> The CPUfreq FW present in some QCOM chipsets offloads the steps necessary
> for hanging the frequency of CPUs. The driver implements the cpufreq driver
> interface for this firmware.
> 
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  drivers/cpufreq/Kconfig.arm       |   9 ++
>  drivers/cpufreq/Makefile          |   1 +
>  drivers/cpufreq/qcom-cpufreq-fw.c | 318 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 328 insertions(+)
>  create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 96b35b8..a50aa6e 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -301,3 +301,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
> +	tristate "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 8d24ade..a3edbce 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
>  obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
>  obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
>  obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_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..67996d5
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-fw.c
> @@ -0,0 +1,318 @@
> +// 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;
> +
> +	if (index >= LUT_MAX_ENTRIES) {
> +		dev_err(c->dev,
> +		"Passing an index (%u) that's greater than max (%d)\n",

Alignment issues here. Run checkpatch --strict.

> +					index, LUT_MAX_ENTRIES - 1);
> +		return -EINVAL;
> +	}
> +
> +	writel_relaxed(index, c->perf_base);
> +
> +	/* Make sure the write goes through before proceeding */
> +	mb();
> +	return 0;
> +}
> +
> +static unsigned int qcom_cpufreq_fw_get(unsigned int cpu)
> +{
> +	struct cpufreq_qcom *c;
> +	unsigned int index;
> +
> +	c = qcom_freq_domain_map[cpu];
> +	if (!c)
> +		return -ENODEV;
> +
> +	index = readl_relaxed(c->perf_base);
> +	index = min(index, LUT_MAX_ENTRIES - 1);
> +
> +	return c->table[index].frequency;
> +}
> +
> +static int qcom_cpufreq_fw_cpu_init(struct cpufreq_policy *policy)
> +{
> +	struct cpufreq_qcom *c;
> +	int ret;
> +
> +	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->table = c->table;
> +	policy->driver_data = c;
> +
> +	return ret;
> +}
> +
> +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,
> +	.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 = 0;
> +
> +	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);

Why do you need this here ? And why do below in case this doesn't
match max-cores count ?

> +
> +		if (!src)
> +			c->table[i].frequency = INIT_RATE / 1000;
> +		else
> +			c->table[i].frequency = XO_RATE * lval / 1000;
> +
> +		c->table[i].driver_data = 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)
> +			c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
> +
> +		/*
> +		 * Two of the same frequencies with the same core counts means
> +		 * end of table.
> +		 */
> +		if (i > 0 && c->table[i - 1].driver_data ==
> +					c->table[i].driver_data
> +					&& prev_cc == core_count) {
> +			struct cpufreq_frequency_table *prev = &c->table[i - 1];
> +
> +			if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
> +				prev->flags = CPUFREQ_BOOST_FREQ;
> +				prev->frequency = prev->driver_data;
> +			}
> +
> +			break;
> +		}
> +		prev_cc = core_count;
> +	}
> +	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 *dev_phandle;
> +	struct device *cpu_dev;
> +	int cpu, i = 0, ret = -ENOENT;
> +
> +	dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);

TBH, I am not a great fan of the CPU phandle list you have created
here. Lets see what Rob has to say on this.

> +	while (dev_phandle) {
> +		for_each_possible_cpu(cpu) {
> +			cpu_dev = get_cpu_device(cpu);
> +			if (cpu_dev && cpu_dev->of_node == dev_phandle) {
> +				cpumask_set_cpu(cpu, m);
> +				ret = 0;

Maybe just remove this line ...

> +				break;
> +			}
> +		}
> +		dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> +	}
> +
> +	return ret;

and check for empty cpumask for an error here.

> +}
> +
> +static int qcom_cpu_resources_init(struct platform_device *pdev,
> +						struct device_node *np)

You may want to align these properly. Try running checkpatch with
--strict option.

> +{
> +	struct cpufreq_qcom *c;
> +	struct resource res;
> +	struct device *dev = &pdev->dev;
> +	void __iomem *en_base;
> +	int cpu, index = 0, ret;

Why initialize index here ?

> +
> +	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);

Check for a valid 'c' here ?

> +
> +	res = platform_get_resource_byname(dev, IORESOURCE_MEM, "en_base");

You are assigning a pointer to a structure here :(

> +	if (!res) {
> +		dev_err(dev, "Enable base not defined for %s\n", np->name);
> +		return ret;
> +	}
> +
> +	en_base = devm_ioremap(dev, res->start, resource_size(res));

You don't get a build error for doing res->start here ? Looks like you
sent a driver upstream which doesn't even build.

> +	if (!en_base) {
> +		dev_err(dev, "Unable to map %s en-base\n", np->name);
> +		return -ENOMEM;
> +	}
> +
> +	/* FW should be 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_base");
> +	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_base");
> +	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 core phandles\n", np->name);
> +		return ret;
> +	}
> +
> +	c->max_cores = cpumask_weight(&c->related_cpus);
> +
> +	ret = qcom_read_lut(pdev, c);
> +	if (ret) {
> +		dev_err(dev, "%s failed to read LUT\n", np->name);
> +		return ret;
> +	}
> +
> +	for_each_cpu(cpu, &c->related_cpus)
> +		qcom_freq_domain_map[cpu] = c;
> +
> +	return 0;
> +}
> +
> +static int qcom_resources_init(struct platform_device *pdev)
> +{
> +	struct device_node *np;
> +	int ret = -ENODEV;
> +
> +	for_each_available_child_of_node(pdev->dev.of_node, np) {
> +		if (of_device_is_compatible(np, "cpufreq")) {
> +			ret = qcom_cpu_resources_init(pdev, np);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	return ret;

Don't initialize ret to -ENODEV, rather return -ENODEV directly here.
That makes it more readable.

> +}
> +
> +static int qcom_cpufreq_fw_driver_probe(struct platform_device *pdev)
> +{
> +	int rc = 0;

Don't need to initialize to 0 here.

> +
> +	/* 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 inited\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);

Why this for a driver which can be built as a module ? You really want
it to be built as a module ?

> +
> +static void __exit qcom_cpufreq_fw_exit(void)
> +{
> +	platform_driver_unregister(&qcom_cpufreq_fw_driver);
> +}

But you don't unregister the cpufreq driver ?

> +module_exit(qcom_cpufreq_fw_exit);
> +
> +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.

-- 
viresh

Re: [v0 1/2] dt-bindings: clock: Introduce QCOM CPUFREQ FW bindings

[Viresh Kumar]

+ Rob.

On 17-05-18, 15:00, 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           | 68 ++++++++++++++++++++++
>  1 file changed, 68 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..bc912f4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
> @@ -0,0 +1,68 @@
> +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".
> +
> +Note that #address-cells, #size-cells, and ranges shall be present to ensure
> +the cpufreq can address a freq-domain registers.
> +
> +A freq-domain sub-node would be defined for the cpus with the following
> +properties:
> +
> +- compatible:
> +	Usage:		required
> +	Value type:	<string>
> +	Definition:	must be "cpufreq".
> +
> +- reg
> +	Usage:		required
> +	Value type:	<prop-encoded-array>
> +	Definition:	Addresses and sizes for the memory of the perf_base
> +			, lut_base and en_base.
> +- reg-names
> +	Usage:		required
> +	Value type:	<stringlist>
> +	Definition:	Address names. Must be "perf_base", "lut_base",
> +			"en_base".
> +			Must be specified in the same order as the
> +			corresponding addresses are specified in the reg
> +			property.
> +
> +- qcom,cpulist
> +	Usage:		required
> +	Value type:	<phandles of CPU>
> +	Definition:	List of related cpu handles which are under a cluster.
> +
> +Example:
> +	qcom,cpufreq-fw {
> +		compatible = "qcom,cpufreq-fw";
> +
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		freq-domain-0 {
> +			compatible = "cpufreq";
> +			reg = <0x17d43920 0x4>,
> +			     <0x17d43110 0x500>,
> +			     <0x17d41000 0x4>;
> +			reg-names = "perf_base", "lut_base", "en_base";
> +			qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3>;
> +		};
> +
> +		freq-domain-1 {
> +			compatible = "cpufreq";
> +			reg = <0x17d46120 0x4>,
> +			    <0x17d45910 0x500>,
> +			    <0x17d45800 0x4>;
> +			reg-names = "perf_base", "lut_base", "en_base";
> +			qcom,cpulist = <&CPU4 &CPU5 &CPU6 &CPU7>;
> +		};
> +	};
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
> of the Code Aurora Forum, hosted by the  Linux Foundation.

-- 
viresh

Re: [v0 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

[Amit Kucheria]

On Thu, May 17, 2018 at 12:30 PM, Taniya Das <tdas@codeaurora.org> wrote:
> The CPUfreq FW present in some QCOM chipsets offloads the steps necessary
> for hanging the frequency of CPUs. The driver implements the cpufreq driver

s/hanging/changing :-)

> interface for this firmware.
>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  drivers/cpufreq/Kconfig.arm       |   9 ++
>  drivers/cpufreq/Makefile          |   1 +
>  drivers/cpufreq/qcom-cpufreq-fw.c | 318 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 328 insertions(+)
>  create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 96b35b8..a50aa6e 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -301,3 +301,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
> +       tristate "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 8d24ade..a3edbce 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)    += tegra124-cpufreq.o
>  obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)     += tegra186-cpufreq.o
>  obj-$(CONFIG_ARM_TI_CPUFREQ)           += ti-cpufreq.o
>  obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_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..67996d5
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-fw.c
> @@ -0,0 +1,318 @@
> +// 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;
> +
> +       if (index >= LUT_MAX_ENTRIES) {
> +               dev_err(c->dev,
> +               "Passing an index (%u) that's greater than max (%d)\n",
> +                                       index, LUT_MAX_ENTRIES - 1);
> +               return -EINVAL;
> +       }
> +
> +       writel_relaxed(index, c->perf_base);
> +
> +       /* Make sure the write goes through before proceeding */
> +       mb();
> +       return 0;
> +}
> +
> +static unsigned int qcom_cpufreq_fw_get(unsigned int cpu)
> +{
> +       struct cpufreq_qcom *c;
> +       unsigned int index;
> +
> +       c = qcom_freq_domain_map[cpu];
> +       if (!c)
> +               return -ENODEV;
> +
> +       index = readl_relaxed(c->perf_base);
> +       index = min(index, LUT_MAX_ENTRIES - 1);
> +
> +       return c->table[index].frequency;
> +}
> +
> +static int qcom_cpufreq_fw_cpu_init(struct cpufreq_policy *policy)
> +{
> +       struct cpufreq_qcom *c;
> +       int ret;
> +
> +       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->table = c->table;
> +       policy->driver_data = c;
> +
> +       return ret;
> +}
> +
> +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,
> +       .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 = 0;


Get rid of prev_cc variable initialisation here and in other places
through out the code

> +
> +       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;
> +
> +               c->table[i].driver_data = 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)
> +                       c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
> +
> +               /*
> +                * Two of the same frequencies with the same core counts means
> +                * end of table.
> +                */
> +               if (i > 0 && c->table[i - 1].driver_data ==
> +                                       c->table[i].driver_data
> +                                       && prev_cc == core_count) {
> +                       struct cpufreq_frequency_table *prev = &c->table[i - 1];
> +
> +                       if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
> +                               prev->flags = CPUFREQ_BOOST_FREQ;
> +                               prev->frequency = prev->driver_data;
> +                       }
> +
> +                       break;
> +               }
> +               prev_cc = core_count;
> +       }
> +       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 *dev_phandle;
> +       struct device *cpu_dev;
> +       int cpu, i = 0, ret = -ENOENT;
> +
> +       dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> +       while (dev_phandle) {
> +               for_each_possible_cpu(cpu) {
> +                       cpu_dev = get_cpu_device(cpu);
> +                       if (cpu_dev && cpu_dev->of_node == dev_phandle) {
> +                               cpumask_set_cpu(cpu, m);
> +                               ret = 0;
> +                               break;
> +                       }
> +               }
> +               dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> +       }
> +
> +       return ret;
> +}
> +
> +static int qcom_cpu_resources_init(struct platform_device *pdev,
> +                                               struct device_node *np)
> +{
> +       struct cpufreq_qcom *c;
> +       struct resource res;
> +       struct device *dev = &pdev->dev;
> +       void __iomem *en_base;
> +       int cpu, index = 0, ret;
> +
> +       c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);

Check if memory allocated successfully?

> +
> +       res = platform_get_resource_byname(dev, IORESOURCE_MEM, "en_base");
> +       if (!res) {
> +               dev_err(dev, "Enable base not defined for %s\n", np->name);
> +               return ret;
> +       }


This is not going to build. Did you build-test this on a 4.17-rc kenel ?

> +
> +       en_base = devm_ioremap(dev, res->start, resource_size(res));
> +       if (!en_base) {
> +               dev_err(dev, "Unable to map %s en-base\n", np->name);
> +               return -ENOMEM;
> +       }
> +
> +       /* FW should be 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_base");
> +       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_base");
> +       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 core phandles\n", np->name);
> +               return ret;
> +       }
> +
> +       c->max_cores = cpumask_weight(&c->related_cpus);
> +
> +       ret = qcom_read_lut(pdev, c);
> +       if (ret) {
> +               dev_err(dev, "%s failed to read LUT\n", np->name);
> +               return ret;
> +       }
> +
> +       for_each_cpu(cpu, &c->related_cpus)
> +               qcom_freq_domain_map[cpu] = c;
> +
> +       return 0;
> +}
> +
> +static int qcom_resources_init(struct platform_device *pdev)
> +{
> +       struct device_node *np;
> +       int ret = -ENODEV;
> +
> +       for_each_available_child_of_node(pdev->dev.of_node, np) {
> +               if (of_device_is_compatible(np, "cpufreq")) {
> +                       ret = qcom_cpu_resources_init(pdev, np);
> +                       if (ret)
> +                               return ret;
> +               }
> +       }
> +
> +       return ret;
> +}
> +
> +static int qcom_cpufreq_fw_driver_probe(struct platform_device *pdev)
> +{
> +       int rc = 0;
> +
> +       /* 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 inited\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);
> +
> +static void __exit qcom_cpufreq_fw_exit(void)
> +{
> +       platform_driver_unregister(&qcom_cpufreq_fw_driver);
> +}
> +module_exit(qcom_cpufreq_fw_exit);
> +
> +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.
>

Re: [v0 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

[Taniya Das]

Hi Viresh,

Thanks for the review comments, I have already fixed the resource 
comment in the v1 series which I sent across. I will fix the rest of the 
comments and send it for review.

On 5/17/2018 3:44 PM, Viresh Kumar wrote:
> On 17-05-18, 15:00, Taniya Das wrote:
>> The CPUfreq FW present in some QCOM chipsets offloads the steps necessary
>> for hanging the frequency of CPUs. The driver implements the cpufreq driver
>> interface for this firmware.
>>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> ---
>>   drivers/cpufreq/Kconfig.arm       |   9 ++
>>   drivers/cpufreq/Makefile          |   1 +
>>   drivers/cpufreq/qcom-cpufreq-fw.c | 318 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 328 insertions(+)
>>   create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 96b35b8..a50aa6e 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -301,3 +301,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
>> +	tristate "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 8d24ade..a3edbce 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -85,6 +85,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
>>   obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
>>   obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
>>   obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
>> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_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..67996d5
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-fw.c
>> @@ -0,0 +1,318 @@
>> +// 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;
>> +
>> +	if (index >= LUT_MAX_ENTRIES) {
>> +		dev_err(c->dev,
>> +		"Passing an index (%u) that's greater than max (%d)\n",
> 
> Alignment issues here. Run checkpatch --strict.
> 
>> +					index, LUT_MAX_ENTRIES - 1);
>> +		return -EINVAL;
>> +	}
>> +
>> +	writel_relaxed(index, c->perf_base);
>> +
>> +	/* Make sure the write goes through before proceeding */
>> +	mb();
>> +	return 0;
>> +}
>> +
>> +static unsigned int qcom_cpufreq_fw_get(unsigned int cpu)
>> +{
>> +	struct cpufreq_qcom *c;
>> +	unsigned int index;
>> +
>> +	c = qcom_freq_domain_map[cpu];
>> +	if (!c)
>> +		return -ENODEV;
>> +
>> +	index = readl_relaxed(c->perf_base);
>> +	index = min(index, LUT_MAX_ENTRIES - 1);
>> +
>> +	return c->table[index].frequency;
>> +}
>> +
>> +static int qcom_cpufreq_fw_cpu_init(struct cpufreq_policy *policy)
>> +{
>> +	struct cpufreq_qcom *c;
>> +	int ret;
>> +
>> +	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->table = c->table;
>> +	policy->driver_data = c;
>> +
>> +	return ret;
>> +}
>> +
>> +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,
>> +	.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 = 0;
>> +
>> +	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);
> 
> Why do you need this here ? And why do below in case this doesn't
> match max-cores count ?
> 
>> +
>> +		if (!src)
>> +			c->table[i].frequency = INIT_RATE / 1000;
>> +		else
>> +			c->table[i].frequency = XO_RATE * lval / 1000;
>> +
>> +		c->table[i].driver_data = 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)
>> +			c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
>> +
>> +		/*
>> +		 * Two of the same frequencies with the same core counts means
>> +		 * end of table.
>> +		 */
>> +		if (i > 0 && c->table[i - 1].driver_data ==
>> +					c->table[i].driver_data
>> +					&& prev_cc == core_count) {
>> +			struct cpufreq_frequency_table *prev = &c->table[i - 1];
>> +
>> +			if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
>> +				prev->flags = CPUFREQ_BOOST_FREQ;
>> +				prev->frequency = prev->driver_data;
>> +			}
>> +
>> +			break;
>> +		}
>> +		prev_cc = core_count;
>> +	}
>> +	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 *dev_phandle;
>> +	struct device *cpu_dev;
>> +	int cpu, i = 0, ret = -ENOENT;
>> +
>> +	dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> 
> TBH, I am not a great fan of the CPU phandle list you have created
> here. Lets see what Rob has to say on this.
> 
>> +	while (dev_phandle) {
>> +		for_each_possible_cpu(cpu) {
>> +			cpu_dev = get_cpu_device(cpu);
>> +			if (cpu_dev && cpu_dev->of_node == dev_phandle) {
>> +				cpumask_set_cpu(cpu, m);
>> +				ret = 0;
> 
> Maybe just remove this line ...
> 
>> +				break;
>> +			}
>> +		}
>> +		dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
>> +	}
>> +
>> +	return ret;
> 
> and check for empty cpumask for an error here.
> 
>> +}
>> +
>> +static int qcom_cpu_resources_init(struct platform_device *pdev,
>> +						struct device_node *np)
> 
> You may want to align these properly. Try running checkpatch with
> --strict option.
> 
>> +{
>> +	struct cpufreq_qcom *c;
>> +	struct resource res;
>> +	struct device *dev = &pdev->dev;
>> +	void __iomem *en_base;
>> +	int cpu, index = 0, ret;
> 
> Why initialize index here ?
> 
>> +
>> +	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> 
> Check for a valid 'c' here ?
> 
>> +
>> +	res = platform_get_resource_byname(dev, IORESOURCE_MEM, "en_base");
> 
> You are assigning a pointer to a structure here :(
> 
>> +	if (!res) {
>> +		dev_err(dev, "Enable base not defined for %s\n", np->name);
>> +		return ret;
>> +	}
>> +
>> +	en_base = devm_ioremap(dev, res->start, resource_size(res));
> 
> You don't get a build error for doing res->start here ? Looks like you
> sent a driver upstream which doesn't even build.
> 
>> +	if (!en_base) {
>> +		dev_err(dev, "Unable to map %s en-base\n", np->name);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	/* FW should be 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_base");
>> +	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_base");
>> +	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 core phandles\n", np->name);
>> +		return ret;
>> +	}
>> +
>> +	c->max_cores = cpumask_weight(&c->related_cpus);
>> +
>> +	ret = qcom_read_lut(pdev, c);
>> +	if (ret) {
>> +		dev_err(dev, "%s failed to read LUT\n", np->name);
>> +		return ret;
>> +	}
>> +
>> +	for_each_cpu(cpu, &c->related_cpus)
>> +		qcom_freq_domain_map[cpu] = c;
>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_resources_init(struct platform_device *pdev)
>> +{
>> +	struct device_node *np;
>> +	int ret = -ENODEV;
>> +
>> +	for_each_available_child_of_node(pdev->dev.of_node, np) {
>> +		if (of_device_is_compatible(np, "cpufreq")) {
>> +			ret = qcom_cpu_resources_init(pdev, np);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +	}
>> +
>> +	return ret;
> 
> Don't initialize ret to -ENODEV, rather return -ENODEV directly here.
> That makes it more readable.
> 
>> +}
>> +
>> +static int qcom_cpufreq_fw_driver_probe(struct platform_device *pdev)
>> +{
>> +	int rc = 0;
> 
> Don't need to initialize to 0 here.
> 
>> +
>> +	/* 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 inited\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);
> 
> Why this for a driver which can be built as a module ? You really want
> it to be built as a module ?
> 
>> +
>> +static void __exit qcom_cpufreq_fw_exit(void)
>> +{
>> +	platform_driver_unregister(&qcom_cpufreq_fw_driver);
>> +}
> 
> But you don't unregister the cpufreq driver ?
> 
>> +module_exit(qcom_cpufreq_fw_exit);
>> +
>> +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.
> 

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

--

Re: [v0 1/2] dt-bindings: clock: Introduce QCOM CPUFREQ FW bindings

[Saravana Kannan]

On 05/17/2018 03:14 AM, Viresh Kumar wrote:
> + Rob.
>
> On 17-05-18, 15:00, 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           | 68 ++++++++++++++++++++++
>>   1 file changed, 68 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt

Isn't the prefix wrong? Shouldn't it be dt-bindings: cpufreq:?

-Saravana


-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [v0 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

[Saravana Kannan]

On 05/17/2018 03:14 AM, Viresh Kumar wrote:
> On 17-05-18, 15:00, Taniya Das wrote:
>> The CPUfreq FW present in some QCOM chipsets offloads the steps necessary
>> for hanging the frequency of CPUs. The driver implements the cpufreq driver
>> interface for this firmware.
>>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> ---
>>
>>   ##################################################################################
>> diff --git a/drivers/cpufreq/qcom-cpufreq-fw.c b/drivers/cpufreq/qcom-cpufreq-fw.c

>> +
>> +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 = 0;
>> +
>> +	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);
>
> Why do you need this here ? And why do below in case this doesn't
> match max-cores count ?

This is how we detect boost frequencies.

>> +
>> +		if (!src)
>> +			c->table[i].frequency = INIT_RATE / 1000;
>> +		else
>> +			c->table[i].frequency = XO_RATE * lval / 1000;
>> +
>> +		c->table[i].driver_data = 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)
>> +			c->table[i].frequency = CPUFREQ_ENTRY_INVALID;

The FW might has some frequencies marked as "boost frequencies" when 
there are higher non-boost frequencies. So, we mark them as invalid.

>> +
>> +		/*
>> +		 * Two of the same frequencies with the same core counts means
>> +		 * end of table.
>> +		 */
>> +		if (i > 0 && c->table[i - 1].driver_data ==
>> +					c->table[i].driver_data
>> +					&& prev_cc == core_count) {
>> +			struct cpufreq_frequency_table *prev = &c->table[i - 1];
>> +
>> +			if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
>> +				prev->flags = CPUFREQ_BOOST_FREQ;
>> +				prev->frequency = prev->driver_data;
>> +			}
>> +
>> +			break;
>> +		}
>> +		prev_cc = core_count;
>> +	}
>> +	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 *dev_phandle;
>> +	struct device *cpu_dev;
>> +	int cpu, i = 0, ret = -ENOENT;
>> +
>> +	dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
>
> TBH, I am not a great fan of the CPU phandle list you have created
> here. Lets see what Rob has to say on this.
>

Neither do we, but this is the only real way of mapping the logical CPU 
numbers to the real CPUs in HW that belong to the same freq domain. 
Because boot CPU is always going to be CPU0 if I'm not mistaken.

>> +	while (dev_phandle) {
>> +		for_each_possible_cpu(cpu) {
>> +			cpu_dev = get_cpu_device(cpu);
>> +			if (cpu_dev && cpu_dev->of_node == dev_phandle) {
>> +				cpumask_set_cpu(cpu, m);
>> +				ret = 0;
>
> Maybe just remove this line ...
>
>> +				break;
>> +			}
>> +		}
>> +		dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
>> +	}
>> +
>> +	return ret;


-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [v0 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 10763 bytes --]

Hi Taniya,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pm/linux-next]
[also build test ERROR on v4.17-rc5 next-20180517]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Taniya-Das/Add-support-for-QCOM-cpufreq-FW-driver/20180519-050902
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/cpufreq/qcom-cpufreq-fw.c: In function 'qcom_cpufreq_fw_cpu_init':
>> drivers/cpufreq/qcom-cpufreq-fw.c:77:8: error: 'struct cpufreq_policy' has no member named 'table'
     policy->table = c->table;
           ^~
   drivers/cpufreq/qcom-cpufreq-fw.c: In function 'qcom_cpu_resources_init':
>> drivers/cpufreq/qcom-cpufreq-fw.c:187:37: error: passing argument 1 of 'platform_get_resource_byname' from incompatible pointer type [-Werror=incompatible-pointer-types]
     res = platform_get_resource_byname(dev, IORESOURCE_MEM, "en_base");
                                        ^~~
   In file included from include/linux/of_device.h:6:0,
                    from include/linux/of_platform.h:12,
                    from drivers/cpufreq/qcom-cpufreq-fw.c:11:
   include/linux/platform_device.h:56:25: note: expected 'struct platform_device *' but argument is of type 'struct device *'
    extern struct resource *platform_get_resource_byname(struct platform_device *,
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/cpufreq/qcom-cpufreq-fw.c:187:6: error: incompatible types when assigning to type 'struct resource' from type 'struct resource *'
     res = platform_get_resource_byname(dev, IORESOURCE_MEM, "en_base");
         ^
>> drivers/cpufreq/qcom-cpufreq-fw.c:188:6: error: wrong type argument to unary exclamation mark
     if (!res) {
         ^
>> drivers/cpufreq/qcom-cpufreq-fw.c:193:33: error: invalid type argument of '->' (have 'struct resource')
     en_base = devm_ioremap(dev, res->start, resource_size(res));
                                    ^~
>> drivers/cpufreq/qcom-cpufreq-fw.c:193:56: error: incompatible type for argument 1 of 'resource_size'
     en_base = devm_ioremap(dev, res->start, resource_size(res));
                                                           ^~~
   In file included from include/linux/of_address.h:4:0,
                    from drivers/cpufreq/qcom-cpufreq-fw.c:10:
   include/linux/ioport.h:196:31: note: expected 'const struct resource *' but argument is of type 'struct resource'
    static inline resource_size_t resource_size(const struct resource *res)
                                  ^~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +77 drivers/cpufreq/qcom-cpufreq-fw.c

  > 11	#include <linux/of_platform.h>
    12	
    13	#define INIT_RATE			300000000UL
    14	#define XO_RATE				19200000UL
    15	#define LUT_MAX_ENTRIES			40U
    16	#define CORE_COUNT_VAL(val)		((val & GENMASK(18, 16)) >> 16)
    17	#define LUT_ROW_SIZE			32
    18	
    19	struct cpufreq_qcom {
    20		struct cpufreq_frequency_table *table;
    21		struct device *dev;
    22		void __iomem *perf_base;
    23		void __iomem *lut_base;
    24		cpumask_t related_cpus;
    25		unsigned int max_cores;
    26	};
    27	
    28	static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
    29	
    30	static int
    31	qcom_cpufreq_fw_target_index(struct cpufreq_policy *policy, unsigned int index)
    32	{
    33		struct cpufreq_qcom *c = policy->driver_data;
    34	
    35		if (index >= LUT_MAX_ENTRIES) {
    36			dev_err(c->dev,
    37			"Passing an index (%u) that's greater than max (%d)\n",
    38						index, LUT_MAX_ENTRIES - 1);
    39			return -EINVAL;
    40		}
    41	
    42		writel_relaxed(index, c->perf_base);
    43	
    44		/* Make sure the write goes through before proceeding */
    45		mb();
    46		return 0;
    47	}
    48	
    49	static unsigned int qcom_cpufreq_fw_get(unsigned int cpu)
    50	{
    51		struct cpufreq_qcom *c;
    52		unsigned int index;
    53	
    54		c = qcom_freq_domain_map[cpu];
    55		if (!c)
    56			return -ENODEV;
    57	
    58		index = readl_relaxed(c->perf_base);
    59		index = min(index, LUT_MAX_ENTRIES - 1);
    60	
    61		return c->table[index].frequency;
    62	}
    63	
    64	static int qcom_cpufreq_fw_cpu_init(struct cpufreq_policy *policy)
    65	{
    66		struct cpufreq_qcom *c;
    67		int ret;
    68	
    69		c = qcom_freq_domain_map[policy->cpu];
    70		if (!c) {
    71			pr_err("No scaling support for CPU%d\n", policy->cpu);
    72			return -ENODEV;
    73		}
    74	
    75		cpumask_copy(policy->cpus, &c->related_cpus);
    76	
  > 77		policy->table = c->table;
    78		policy->driver_data = c;
    79	
    80		return ret;
    81	}
    82	
    83	static struct freq_attr *qcom_cpufreq_fw_attr[] = {
    84		&cpufreq_freq_attr_scaling_available_freqs,
    85		&cpufreq_freq_attr_scaling_boost_freqs,
    86		NULL
    87	};
    88	
    89	static struct cpufreq_driver cpufreq_qcom_fw_driver = {
    90		.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
    91				  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
    92		.verify		= cpufreq_generic_frequency_table_verify,
    93		.target_index	= qcom_cpufreq_fw_target_index,
    94		.get		= qcom_cpufreq_fw_get,
    95		.init		= qcom_cpufreq_fw_cpu_init,
    96		.name		= "qcom-cpufreq-fw",
    97		.attr		= qcom_cpufreq_fw_attr,
    98		.boost_enabled	= true,
    99	};
   100	
   101	static int qcom_read_lut(struct platform_device *pdev,
   102				struct cpufreq_qcom *c)
   103	{
   104		struct device *dev = &pdev->dev;
   105		u32 data, src, lval, i, core_count, prev_cc = 0;
   106	
   107		c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
   108					sizeof(*c->table), GFP_KERNEL);
   109		if (!c->table)
   110			return -ENOMEM;
   111	
   112		for (i = 0; i < LUT_MAX_ENTRIES; i++) {
   113			data = readl_relaxed(c->lut_base + i * LUT_ROW_SIZE);
   114			src = ((data & GENMASK(31, 30)) >> 30);
   115			lval = (data & GENMASK(7, 0));
   116			core_count = CORE_COUNT_VAL(data);
   117	
   118			if (!src)
   119				c->table[i].frequency = INIT_RATE / 1000;
   120			else
   121				c->table[i].frequency = XO_RATE * lval / 1000;
   122	
   123			c->table[i].driver_data = c->table[i].frequency;
   124	
   125			dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
   126					i, c->table[i].frequency, core_count);
   127	
   128			if (core_count != c->max_cores)
   129				c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
   130	
   131			/*
   132			 * Two of the same frequencies with the same core counts means
   133			 * end of table.
   134			 */
   135			if (i > 0 && c->table[i - 1].driver_data ==
   136						c->table[i].driver_data
   137						&& prev_cc == core_count) {
   138				struct cpufreq_frequency_table *prev = &c->table[i - 1];
   139	
   140				if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
   141					prev->flags = CPUFREQ_BOOST_FREQ;
   142					prev->frequency = prev->driver_data;
   143				}
   144	
   145				break;
   146			}
   147			prev_cc = core_count;
   148		}
   149		c->table[i].frequency = CPUFREQ_TABLE_END;
   150	
   151		return 0;
   152	}
   153	
   154	static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
   155	{
   156		struct device_node *dev_phandle;
   157		struct device *cpu_dev;
   158		int cpu, i = 0, ret = -ENOENT;
   159	
   160		dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
   161		while (dev_phandle) {
   162			for_each_possible_cpu(cpu) {
   163				cpu_dev = get_cpu_device(cpu);
   164				if (cpu_dev && cpu_dev->of_node == dev_phandle) {
   165					cpumask_set_cpu(cpu, m);
   166					ret = 0;
   167					break;
   168				}
   169			}
   170			dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
   171		}
   172	
   173		return ret;
   174	}
   175	
   176	static int qcom_cpu_resources_init(struct platform_device *pdev,
   177							struct device_node *np)
   178	{
   179		struct cpufreq_qcom *c;
   180		struct resource res;
   181		struct device *dev = &pdev->dev;
   182		void __iomem *en_base;
   183		int cpu, index = 0, ret;
   184	
   185		c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
   186	
 > 187		res = platform_get_resource_byname(dev, IORESOURCE_MEM, "en_base");
 > 188		if (!res) {
   189			dev_err(dev, "Enable base not defined for %s\n", np->name);
   190			return ret;
   191		}
   192	
 > 193		en_base = devm_ioremap(dev, res->start, resource_size(res));
   194		if (!en_base) {
   195			dev_err(dev, "Unable to map %s en-base\n", np->name);
   196			return -ENOMEM;
   197		}
   198	
   199		/* FW should be enabled state to proceed */
   200		if (!(readl_relaxed(en_base) & 0x1)) {
   201			dev_err(dev, "%s firmware not enabled\n", np->name);
   202			return -ENODEV;
   203		}
   204	
   205		devm_iounmap(&pdev->dev, en_base);
   206	
   207		index = of_property_match_string(np, "reg-names", "perf_base");
   208		if (index < 0)
   209			return index;
   210	
   211		if (of_address_to_resource(np, index, &res))
   212			return -ENOMEM;
   213	
   214		c->perf_base = devm_ioremap(dev, res.start, resource_size(&res));
   215		if (!c->perf_base) {
   216			dev_err(dev, "Unable to map %s perf-base\n", np->name);
   217			return -ENOMEM;
   218		}
   219	
   220		index = of_property_match_string(np, "reg-names", "lut_base");
   221		if (index < 0)
   222			return index;
   223	
   224		if (of_address_to_resource(np, index, &res))
   225			return -ENOMEM;
   226	
   227		c->lut_base = devm_ioremap(dev, res.start, resource_size(&res));
   228		if (!c->lut_base) {
   229			dev_err(dev, "Unable to map %s lut-base\n", np->name);
   230			return -ENOMEM;
   231		}
   232	
   233		ret = qcom_get_related_cpus(np, &c->related_cpus);
   234		if (ret) {
   235			dev_err(dev, "%s failed to get core phandles\n", np->name);
   236			return ret;
   237		}
   238	
   239		c->max_cores = cpumask_weight(&c->related_cpus);
   240	
   241		ret = qcom_read_lut(pdev, c);
   242		if (ret) {
   243			dev_err(dev, "%s failed to read LUT\n", np->name);
   244			return ret;
   245		}
   246	
   247		for_each_cpu(cpu, &c->related_cpus)
   248			qcom_freq_domain_map[cpu] = c;
   249	
   250		return 0;
   251	}
   252	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65216 bytes --]