Skip to contentSkip to conversation

cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

PATCH Superseded Taniya Das 6 participants 13 messages Jul 12, 2018 – Jul 18, 2018

Subsystems (3)

v5 of 13 Superseded Latest is v13 →

lore
Share & tools
lore

Taniya Das

author
#0 lore raw

v5 cover cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[v5]
* Remove mapping different register regions of perf/lut/enable, instead map the entire HW region.
* Add reg_offset/cpufreq_qcom_std_offsets to be supplied as device data.
* Check of src == 0 during lut read.
* Add of_node_put(cpu_np) in qcom_get_related_cpus
* Update the qcom_cpu_resources_init for register offset data, and cleanup the related cpus to keep a single copy of CPUfreq.
* Replace FW with HW, update Kconfig, rename filename qcom-cpufreq-hw.c
* Update the documentation binding to reflect the changes of mapping the
* entire HW region.

[v4]
* Fixed console messages as per comments.
* Return error from qcom_resources_init()
in the cases where failed to get frequency domain.
* Rename cpu_dev to cpu_np in qcom_resources_init, qcom_get_related_cpus(). Also use temp variable freq_np in qcom_get_related_cpus().
* Update qcom_cpufreq_fw_get() to use the policy data to incoporate the hotplug use case.
* Update code to use of fast_switching.
* Check for !c->max_cores instead of cpumask_empty in qcom_get_related_cpus().
* Update the logic of assigning 'c' to qcom_freq_domain_map[cpu].

[v3]
* Remove index check from 'qcom_cpufreq_fw_target_index'.
* Update the Documentation binding to add the platform specific properties in the CPU nodes, node name "qcom,freq-domain".
* Update return value to '0' from -ENODEV from 'qcom_cpufreq_fw_get'.
* Update the logic for boost frequency to use local variables instead of cpufreq driver data in 'qcom_read_lut'.
* Update the logic in 'qcom_get_related_cpus' to find the related cpus.
* Update the reg-names to remove "_base" and also update the binding with the description of these registers.
* Update the logic in 'qcom_resources_init' to address the new device tree notation of handling the frequency domain phandles.

[v2]
* Fixed the alignment issues in "qcom_cpufreq_fw_target_index" for dev_err and also for "qcom_cpu_resources_init".
* Removed ret = 0 from qcom_get_related_cpus and added to check for cpu_mask_empty to return -ENOENT.
* Fixes qcom_cpu_resources_init function
* Remove initialization of 'index'
* Check for valid 'c'
* Removed initialization of 'prev_cc' from 'qcom_read_lut'.
* 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 Firmware bindings
cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

 .../bindings/cpufreq/cpufreq-qcom-hw.txt           | 158 ++++++++++
 drivers/cpufreq/Kconfig.arm                        |  10 +
 drivers/cpufreq/Makefile                           |   1 +
 drivers/cpufreq/qcom-cpufreq-hw.c                  | 344 +++++++++++++++++++++
 4 files changed, 513 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
 create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c

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

author
#1 lore raw

PATCH v5 1/2 dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings

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

---
 .../bindings/cpufreq/cpufreq-qcom-hw.txt           | 158 +++++++++++++++++++++
 1 file changed, 158 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt new file +158 -0
new file mode 100644
index 0000000..52a53e1
@@ -0,0 +1,158 @@
+Qualcomm Technologies, Inc. CPUFREQ Bindings
+
+CPUFREQ HW is a hardware engine used by some Qualcomm Technologies, Inc. (QTI)
+SoCs to manage frequency in hardware. It is capable of controlling frequency
+for multiple clusters.
+
+Properties:
+- compatible
+	Usage:		required
+	Value type:	<string>
+	Definition:	must be "qcom,cpufreq-hw".
+
+* 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 HW bases.
+
+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-hw {
+		compatible = "qcom,cpufreq-hw";
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+		freq_domain_table0: freq_table0 {
+			reg = <0 0x17d43000 0 0x1400>;
+		};
+
+		freq_domain_table1: freq_table1 {
+			reg = <0 0x17d45800 0 0x1400>;
+		};
+	};
+};

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

#2 lore raw

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

Quoting Taniya Das (2018-07-12 11:05:44)
[..]

Quoted 27 lines
Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt new file +27 -0
+                       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-hw {
+               compatible = "qcom,cpufreq-hw";

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt line 156

Quoted 10 lines
Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt new file +10 -0
+               #address-cells = <2>;
+               #size-cells = <2>;
+               ranges;
+               freq_domain_table0: freq_table0 {
+                       reg = <0 0x17d43000 0 0x1400>;
+               };
+
+               freq_domain_table1: freq_table1 {
+                       reg = <0 0x17d45800 0 0x1400>;
+               };

It seems that we need to map the CPUs in the cpus node to the frequency domains in the cpufreq-hw node. Wouldn't that be better served via a #foo-cells and <&phandle foo-cell> property in the CPU node? It's annoying that the cpufreq-hw node doesn't have a reg property, when it really should have one that goes over the whole register space (or is split across the frequency domains so that there are two reg properties here).

author
#3 lore raw

PATCH v5 2/2 cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

In reply to Taniya Das

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

---
 drivers/cpufreq/Kconfig.arm       |  10 ++
 drivers/cpufreq/Makefile          |   1 +
 drivers/cpufreq/qcom-cpufreq-hw.c | 344 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 355 insertions(+)
 create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
drivers/cpufreq/Kconfig.arm +10 -0
index 52f5f1a..141ec3e 100644
@@ -312,3 +312,13 @@ config ARM_PXA2xx_CPUFREQ
 	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
 
 	  If in doubt, say N.
+
+config ARM_QCOM_CPUFREQ_HW
+	bool "QCOM CPUFreq HW driver"
+	help
+	 Support for the CPUFreq HW driver.
+	 Some QCOM chipsets have a HW engine to offload the steps
+	 necessary for changing the frequency of the CPUs. Firmware loaded
+	 in this engine exposes a programming interafce to the High-level OS.
+	 The driver implements the cpufreq driver interface for this HW engine.
+	 Say Y if you want to support CPUFreq HW.
drivers/cpufreq/Makefile +1 -0
index fb4a2ec..1226a3e 100644
@@ -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_HW)	+= qcom-cpufreq-hw.o
 
 
 ##################################################################################
drivers/cpufreq/qcom-cpufreq-hw.c new file +344 -0

344 lines not on this page · raw message

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

#4 lore raw

Quoting Taniya Das (2018-07-12 11:05:45)

Quoted 11 lines

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

drivers/cpufreq/Kconfig.arm +0 -0
index 52f5f1a..141ec3e 100644

drivers/cpufreq/Kconfig.arm config ARM_PXA2xx_CPUFREQ · line 317

Quoted 6 lines
drivers/cpufreq/Kconfig.arm +3 -0
@@ -312,3 +312,13 @@ config ARM_PXA2xx_CPUFREQ
          This add the CPUFreq driver support for Intel PXA2xx SOCs.
 
          If in doubt, say N.
+
+config ARM_QCOM_CPUFREQ_HW
+       bool "QCOM CPUFreq HW driver"

Why can't it be a module?

drivers/cpufreq/Kconfig.arm config ARM_PXA2xx_CPUFREQ · line 322

Quoted 5 lines
drivers/cpufreq/Kconfig.arm +5 -0
+       help
+        Support for the CPUFreq HW driver.
+        Some QCOM chipsets have a HW engine to offload the steps
+        necessary for changing the frequency of the CPUs. Firmware loaded
+        in this engine exposes a programming interafce to the High-level OS.

typo on interface. Why is High capitalized? Just say OS?

drivers/cpufreq/Kconfig.arm config ARM_PXA2xx_CPUFREQ · line 323

Quoted 1 line
drivers/cpufreq/Kconfig.arm +1 -0
+        The driver implements the cpufreq driver interface for this HW engine.

So much 'driver'.

Quoted 10 lines
drivers/cpufreq/Kconfig.arm +1 -0
+        Say Y if you want to support CPUFreq HW.
drivers/cpufreq/qcom-cpufreq-hw.c new file +3 -0
new file mode 100644
index 0000000..fa25a95
@@ -0,0 +1,344 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.

drivers/cpufreq/qcom-cpufreq-hw.c line 13

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+ */
+
+#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

This doesn't need to be configured from DT? Or more likely be specified as some sort of PLL that is part of the clocks property so we know what the 'safe' or 'default' frequency is?

drivers/cpufreq/qcom-cpufreq-hw.c line 14

Quoted 1 line
drivers/cpufreq/qcom-cpufreq-hw.c new file +1 -0
+#define XO_RATE                                19200000UL

This should come from DT via some clocks property.

Quoted 12 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +12 -0
+#define LUT_MAX_ENTRIES                        40U
+#define CORE_COUNT_VAL(val)            (((val) & (GENMASK(18, 16))) >> 16)
+#define LUT_ROW_SIZE                   32
+
+enum {
+       REG_ENABLE,
+       REG_LUT_TABLE,
+       REG_PERF_STATE,
+
+       REG_ARRAY_SIZE,
+};
+

drivers/cpufreq/qcom-cpufreq-hw.c line 36

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+struct cpufreq_qcom {
+       struct cpufreq_frequency_table *table;
+       struct device *dev;
+       const u16 *reg_offset;
+       void __iomem *base;
+       cpumask_t related_cpus;
+       unsigned int max_cores;
+};
+
+static u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {

const?

drivers/cpufreq/qcom-cpufreq-hw.c line 39

Quoted 3 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +3 -0
+       [REG_ENABLE]            = 0x0,
+       [REG_LUT_TABLE]         = 0x110,
+       [REG_PERF_STATE]        = 0x920,

Is the register map going to change again for the next device? It may be better to precalculate the offset for the fast switch so that the addition isn't in the hotpath.

Quoted 34 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +34 -0
+};
+
+static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
+
+static int
+qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
+                            unsigned int index)
+{
+       struct cpufreq_qcom *c = policy->driver_data;
+       unsigned int offset = c->reg_offset[REG_PERF_STATE];
+
+       writel_relaxed(index, c->base + offset);
+
+       return 0;
+}
+
+static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
+{
+       struct cpufreq_qcom *c;
+       struct cpufreq_policy *policy;
+       unsigned int index, offset;
+
+       policy = cpufreq_cpu_get_raw(cpu);
+       if (!policy)
+               return 0;
+
+       c = policy->driver_data;
+       offset = c->reg_offset[REG_PERF_STATE];
+
+       index = readl_relaxed(c->base + offset);
+       index = min(index, LUT_MAX_ENTRIES - 1);
+
+       return policy->freq_table[index].frequency;
+}

drivers/cpufreq/qcom-cpufreq-hw.c line 83

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+
+static unsigned int
+qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
+                           unsigned int target_freq)
+{
+       struct cpufreq_qcom *c = policy->driver_data;
+       unsigned int offset;
+       int index;
+
+       index = cpufreq_table_find_index_l(policy, target_freq);

It's unfortunate that we have to search the table in software again. Why can't we use policy->cached_resolved_idx to avoid this search twice?

Quoted 55 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +55 -0
+       if (index < 0)
+               return 0;
+
+       offset = c->reg_offset[REG_PERF_STATE];
+
+       writel_relaxed(index, c->base + offset);
+
+       return policy->freq_table[index].frequency;
+}
+
+static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
+{
+       struct cpufreq_qcom *c;
+
+       c = qcom_freq_domain_map[policy->cpu];
+       if (!c) {
+               pr_err("No scaling support for CPU%d\n", policy->cpu);
+               return -ENODEV;
+       }
+
+       cpumask_copy(policy->cpus, &c->related_cpus);
+
+       policy->fast_switch_possible = true;
+       policy->freq_table = c->table;
+       policy->driver_data = c;
+
+       return 0;
+}
+
+static struct freq_attr *qcom_cpufreq_hw_attr[] = {
+       &cpufreq_freq_attr_scaling_available_freqs,
+       &cpufreq_freq_attr_scaling_boost_freqs,
+       NULL
+};
+
+static struct cpufreq_driver cpufreq_qcom_hw_driver = {
+       .flags          = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
+                         CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
+       .verify         = cpufreq_generic_frequency_table_verify,
+       .target_index   = qcom_cpufreq_hw_target_index,
+       .get            = qcom_cpufreq_hw_get,
+       .init           = qcom_cpufreq_hw_cpu_init,
+       .fast_switch    = qcom_cpufreq_hw_fast_switch,
+       .name           = "qcom-cpufreq-hw",
+       .attr           = qcom_cpufreq_hw_attr,
+       .boost_enabled  = true,
+};
+
+static int qcom_read_lut(struct platform_device *pdev,
+                        struct cpufreq_qcom *c)
+{
+       struct device *dev = &pdev->dev;
+       unsigned int offset;
+       u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
+

drivers/cpufreq/qcom-cpufreq-hw.c line 148

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+       c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
+                               sizeof(*c->table), GFP_KERNEL);
+       if (!c->table)
+               return -ENOMEM;
+
+       offset = c->reg_offset[REG_LUT_TABLE];
+
+       for (i = 0; i < LUT_MAX_ENTRIES; i++) {
+               data = readl_relaxed(c->base + offset + i * LUT_ROW_SIZE);
+               src = ((data & GENMASK(31, 30)) >> 30);

One too many parenthesis.

drivers/cpufreq/qcom-cpufreq-hw.c line 149

Quoted 1 line
drivers/cpufreq/qcom-cpufreq-hw.c new file +1 -0
+               lval = (data & GENMASK(7, 0));

One too many parenthesis.

Quoted 36 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +36 -0
+               core_count = CORE_COUNT_VAL(data);
+
+               if (src == 0)
+                       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;
+}
+

drivers/cpufreq/qcom-cpufreq-hw.c line 195

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -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);

Put the of_node_put(cpu_np) here? And then remove it from the other two places below?

Quoted 21 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +21 -0
+               if (!freq_np) {
+                       of_node_put(cpu_np);
+                       continue;
+               }
+               if (freq_np == np)
+                       cpumask_set_cpu(cpu, m);
+
+               of_node_put(cpu_np);
+       }
+
+       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;
+       unsigned int offset, cpu_r;
+       int ret;

drivers/cpufreq/qcom-cpufreq-hw.c line 226

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+
+       c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+       if (!c)
+               return -ENOMEM;
+
+       c->reg_offset = of_device_get_match_data(&pdev->dev);
+       if (!c->reg_offset)
+               return -EINVAL;
+
+       if (of_address_to_resource(np, 0, &res))

This is unfortunate that it can't use platform APIs.

drivers/cpufreq/qcom-cpufreq-hw.c line 229

Quoted 3 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +3 -0
+               return -ENOMEM;
+
+       c->base = devm_ioremap(dev, res.start, resource_size(&res));

No devm_ioremap_resource? And we don't put the reg properties in the top-level node?

drivers/cpufreq/qcom-cpufreq-hw.c line 231

Quoted 2 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +2 -0
+       if (!c->base) {
+               dev_err(dev, "Unable to map %s base\n", np->name);

We don't need error messages like this for mapping failures when it will spew a kmalloc error.

Quoted 12 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +12 -0
+               return -ENOMEM;
+       }
+
+       offset = c->reg_offset[REG_ENABLE];
+
+       /* HW should be in enabled state to proceed */
+       if (!(readl_relaxed(c->base + offset) & 0x1)) {
+               dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
+               return -ENODEV;
+       }
+
+       ret = qcom_get_related_cpus(np, &c->related_cpus);

drivers/cpufreq/qcom-cpufreq-hw.c line 253

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+       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);

qcom_cpufreq_hw_read_lut?

drivers/cpufreq/qcom-cpufreq-hw.c line 261

Quoted 8 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +8 -0
+       if (ret) {
+               dev_err(dev, "%s failed to read LUT\n", np->name);
+               return ret;
+       }
+
+       qcom_freq_domain_map[cpu] = c;
+
+       /* Related CPUs to keep a single copy */

What does this comment mean?

Quoted 11 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +11 -0
+       cpu_r = cpumask_first(&c->related_cpus);
+       if (cpu != cpu_r) {
+               qcom_freq_domain_map[cpu] = qcom_freq_domain_map[cpu_r];
+               devm_kfree(dev, c);
+       }
+
+       return 0;
+}
+
+static int qcom_resources_init(struct platform_device *pdev)
+{

drivers/cpufreq/qcom-cpufreq-hw.c line 282

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+       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;

An error, but we continue? Why not dev_dbg level?

Quoted 26 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +26 -0
+               }
+
+               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_hw_driver_probe(struct platform_device *pdev)
+{
+       int rc;
+
+       /* Get the bases of cpufreq for domains */
+       rc = qcom_resources_init(pdev);
+       if (rc) {
+               dev_err(&pdev->dev, "CPUFreq resource init failed\n");

drivers/cpufreq/qcom-cpufreq-hw.c line 318

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+               return rc;
+       }
+
+       rc = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
+       if (rc) {
+               dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
+               return rc;
+       }
+
+       dev_info(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");

Move to dev_dbg? We have other ways to know if a driver probes successfully so the whole line isn't really needed.

drivers/cpufreq/qcom-cpufreq-hw.c line 323

Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +5 -0
+
+       return 0;
+}
+
+static const struct of_device_id match_table[] = {

Please call it something besides 'match_table'. qcom_cpufreq_hw_match?

drivers/cpufreq/qcom-cpufreq-hw.c line 333

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+       { .compatible = "qcom,cpufreq-hw", .data = &cpufreq_qcom_std_offsets },
+       {}
+};
+
+static struct platform_driver qcom_cpufreq_hw_driver = {
+       .probe = qcom_cpufreq_hw_driver_probe,
+       .driver = {
+               .name = "qcom-cpufreq-hw",
+               .of_match_table = match_table,
+               .owner = THIS_MODULE,

platform_driver_register() already assigns this. This should be dropped from here.

drivers/cpufreq/qcom-cpufreq-hw.c line 344

Quoted 11 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +11 -0
+       },
+};
+
+static int __init qcom_cpufreq_hw_init(void)
+{
+       return platform_driver_register(&qcom_cpufreq_hw_driver);
+}
+subsys_initcall(qcom_cpufreq_hw_init);
+
+MODULE_DESCRIPTION("QCOM firmware-based CPU Frequency driver");
+MODULE_LICENSE("GPL v2");

It should be tristate then in the Kconfig.

author
#5 lore raw

Hello Stephen,

Thanks for the review comments.

Quoted 14 lines · Stephen Boyd

Quoting Taniya Das (2018-07-12 11:05:45)

Quoted 13 lines

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

drivers/cpufreq/Kconfig.arm +0 -0
index 52f5f1a..141ec3e 100644
@@ -312,3 +312,13 @@ config ARM_PXA2xx_CPUFREQ
           This add the CPUFreq driver support for Intel PXA2xx SOCs.
 
Quoted 7 lines

drivers/cpufreq/Kconfig.arm config ARM_PXA2xx_CPUFREQ · line 317

Quoted 4 lines
drivers/cpufreq/Kconfig.arm +3 -0
           If in doubt, say N.
+
+config ARM_QCOM_CPUFREQ_HW
+       bool "QCOM CPUFreq HW driver"

Why can't it be a module?

I am of the opinion to keep it in-built.

Quoted 6 lines

drivers/cpufreq/Kconfig.arm config ARM_PXA2xx_CPUFREQ · line 322

Quoted 5 lines
drivers/cpufreq/Kconfig.arm +5 -0
+       help
+        Support for the CPUFreq HW driver.
+        Some QCOM chipsets have a HW engine to offload the steps
+        necessary for changing the frequency of the CPUs. Firmware loaded
+        in this engine exposes a programming interafce to the High-level OS.
Quoted 7 lines

typo on interface. Why is High capitalized? Just say OS? Taken care in the next patch.

drivers/cpufreq/Kconfig.arm config ARM_PXA2xx_CPUFREQ · line 323

Quoted 1 line
drivers/cpufreq/Kconfig.arm +1 -0
+        The driver implements the cpufreq driver interface for this HW engine.

So much 'driver'.

Taken care in the next patch.

Quoted 16 lines
Quoted 16 lines
drivers/cpufreq/Kconfig.arm +1 -0
+        Say Y if you want to support CPUFreq HW.
drivers/cpufreq/qcom-cpufreq-hw.c new file +9 -0
new file mode 100644
index 0000000..fa25a95
@@ -0,0 +1,344 @@
+// 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>
Quoted 9 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 13

Quoted 4 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +4 -0
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+
+#define INIT_RATE                      300000000UL

This doesn't need to be configured from DT? Or more likely be specified as some sort of PLL that is part of the clocks property so we know what the 'safe' or 'default' frequency is?

The source is RCG which is pre-configured by HW and is not modeled in SW code. That is the reason to keep it as a macro.

Quoted 4 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 14

Quoted 1 line
drivers/cpufreq/qcom-cpufreq-hw.c new file +1 -0
+#define XO_RATE                                19200000UL

This should come from DT via some clocks property.

This would be taken as an input from DT clocks.

Quoted 16 lines
Quoted 16 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +16 -0
+#define LUT_MAX_ENTRIES                        40U
+#define CORE_COUNT_VAL(val)            (((val) & (GENMASK(18, 16))) >> 16)
+#define LUT_ROW_SIZE                   32
+
+enum {
+       REG_ENABLE,
+       REG_LUT_TABLE,
+       REG_PERF_STATE,
+
+       REG_ARRAY_SIZE,
+};
+
+struct cpufreq_qcom {
+       struct cpufreq_frequency_table *table;
+       struct device *dev;
+       const u16 *reg_offset;
Quoted 9 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 36

Quoted 6 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +6 -0
+       void __iomem *base;
+       cpumask_t related_cpus;
+       unsigned int max_cores;
+};
+
+static u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {

const?

Updated to use const.

Quoted 8 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 39

Quoted 3 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +3 -0
+       [REG_ENABLE]            = 0x0,
+       [REG_LUT_TABLE]         = 0x110,
+       [REG_PERF_STATE]        = 0x920,

Is the register map going to change again for the next device? It may be better to precalculate the offset for the fast switch so that the addition isn't in the hotpath.

Taken care in the next patch set.

Quoted 39 lines
Quoted 39 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +39 -0
+};
+
+static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
+
+static int
+qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
+                            unsigned int index)
+{
+       struct cpufreq_qcom *c = policy->driver_data;
+       unsigned int offset = c->reg_offset[REG_PERF_STATE];
+
+       writel_relaxed(index, c->base + offset);
+
+       return 0;
+}
+
+static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
+{
+       struct cpufreq_qcom *c;
+       struct cpufreq_policy *policy;
+       unsigned int index, offset;
+
+       policy = cpufreq_cpu_get_raw(cpu);
+       if (!policy)
+               return 0;
+
+       c = policy->driver_data;
+       offset = c->reg_offset[REG_PERF_STATE];
+
+       index = readl_relaxed(c->base + offset);
+       index = min(index, LUT_MAX_ENTRIES - 1);
+
+       return policy->freq_table[index].frequency;
+}
+
+static unsigned int
+qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
+                           unsigned int target_freq)
+{
Quoted 9 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 83

Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +5 -0
+       struct cpufreq_qcom *c = policy->driver_data;
+       unsigned int offset;
+       int index;
+
+       index = cpufreq_table_find_index_l(policy, target_freq);

It's unfortunate that we have to search the table in software again. Why can't we use policy->cached_resolved_idx to avoid this search twice?

Yeah, I just checked the call flow
get_next_freq(already determines the idx and keeps a cached copy of the index) and then invokes the 'sugov_update_commit' for fast switch.

My understanding is we could use policy->cached_resolved_idx instead of searching again.

Quoted 59 lines
Quoted 59 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +59 -0
+       if (index < 0)
+               return 0;
+
+       offset = c->reg_offset[REG_PERF_STATE];
+
+       writel_relaxed(index, c->base + offset);
+
+       return policy->freq_table[index].frequency;
+}
+
+static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
+{
+       struct cpufreq_qcom *c;
+
+       c = qcom_freq_domain_map[policy->cpu];
+       if (!c) {
+               pr_err("No scaling support for CPU%d\n", policy->cpu);
+               return -ENODEV;
+       }
+
+       cpumask_copy(policy->cpus, &c->related_cpus);
+
+       policy->fast_switch_possible = true;
+       policy->freq_table = c->table;
+       policy->driver_data = c;
+
+       return 0;
+}
+
+static struct freq_attr *qcom_cpufreq_hw_attr[] = {
+       &cpufreq_freq_attr_scaling_available_freqs,
+       &cpufreq_freq_attr_scaling_boost_freqs,
+       NULL
+};
+
+static struct cpufreq_driver cpufreq_qcom_hw_driver = {
+       .flags          = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
+                         CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
+       .verify         = cpufreq_generic_frequency_table_verify,
+       .target_index   = qcom_cpufreq_hw_target_index,
+       .get            = qcom_cpufreq_hw_get,
+       .init           = qcom_cpufreq_hw_cpu_init,
+       .fast_switch    = qcom_cpufreq_hw_fast_switch,
+       .name           = "qcom-cpufreq-hw",
+       .attr           = qcom_cpufreq_hw_attr,
+       .boost_enabled  = true,
+};
+
+static int qcom_read_lut(struct platform_device *pdev,
+                        struct cpufreq_qcom *c)
+{
+       struct device *dev = &pdev->dev;
+       unsigned int offset;
+       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;
Quoted 9 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 148

Quoted 6 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +6 -0
+
+       offset = c->reg_offset[REG_LUT_TABLE];
+
+       for (i = 0; i < LUT_MAX_ENTRIES; i++) {
+               data = readl_relaxed(c->base + offset + i * LUT_ROW_SIZE);
+               src = ((data & GENMASK(31, 30)) >> 30);

One too many parenthesis.

Removed.

Quoted 4 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 149

Quoted 1 line
drivers/cpufreq/qcom-cpufreq-hw.c new file +1 -0
+               lval = (data & GENMASK(7, 0));

One too many parenthesis.

Removed.

Quoted 41 lines
Quoted 41 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +41 -0
+               core_count = CORE_COUNT_VAL(data);
+
+               if (src == 0)
+                       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;
+
Quoted 9 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 195

Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +5 -0
+       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);

Put the of_node_put(cpu_np) here? And then remove it from the other two places below?

Fixed it in the next patch.

Quoted 34 lines
Quoted 21 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +21 -0
+               if (!freq_np) {
+                       of_node_put(cpu_np);
+                       continue;
+               }
+               if (freq_np == np)
+                       cpumask_set_cpu(cpu, m);
+
+               of_node_put(cpu_np);
+       }
+
+       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;
+       unsigned int offset, cpu_r;
+       int ret;

drivers/cpufreq/qcom-cpufreq-hw.c line 226

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+
+       c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+       if (!c)
+               return -ENOMEM;
+
+       c->reg_offset = of_device_get_match_data(&pdev->dev);
+       if (!c->reg_offset)
+               return -EINVAL;
+
+       if (of_address_to_resource(np, 0, &res))

This is unfortunate that it can't use platform APIs.

Quoted 7 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 229

Quoted 3 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +3 -0
+               return -ENOMEM;
+
+       c->base = devm_ioremap(dev, res.start, resource_size(&res));

No devm_ioremap_resource? And we don't put the reg properties in the top-level node?

Moved to use devm_ioremap_resource. There is no reg property in the top level node.

Quoted 6 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 231

Quoted 2 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +2 -0
+       if (!c->base) {
+               dev_err(dev, "Unable to map %s base\n", np->name);

We don't need error messages like this for mapping failures when it will spew a kmalloc error.

removed the error message.

Quoted 16 lines
Quoted 16 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +16 -0
+               return -ENOMEM;
+       }
+
+       offset = c->reg_offset[REG_ENABLE];
+
+       /* HW should be in enabled state to proceed */
+       if (!(readl_relaxed(c->base + offset) & 0x1)) {
+               dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
+               return -ENODEV;
+       }
+
+       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;
+       }
Quoted 9 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 253

Quoted 6 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +6 -0
+
+       c->max_cores = cpumask_weight(&c->related_cpus);
+       if (!c->max_cores)
+               return -ENOENT;
+
+       ret = qcom_read_lut(pdev, c);

qcom_cpufreq_hw_read_lut?

Renamed the function to 'qcom_cpufreq_hw_read_lut'.

Quoted 11 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 261

Quoted 8 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +8 -0
+       if (ret) {
+               dev_err(dev, "%s failed to read LUT\n", np->name);
+               return ret;
+       }
+
+       qcom_freq_domain_map[cpu] = c;
+
+       /* Related CPUs to keep a single copy */

What does this comment mean?

All related CPUs to use first cpu of the cluster structure.

Quoted 15 lines
Quoted 15 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +15 -0
+       cpu_r = cpumask_first(&c->related_cpus);
+       if (cpu != cpu_r) {
+               qcom_freq_domain_map[cpu] = qcom_freq_domain_map[cpu_r];
+               devm_kfree(dev, c);
+       }
+
+       return 0;
+}
+
+static int qcom_resources_init(struct platform_device *pdev)
+{
+       struct device_node *np, *cpu_np;
+       unsigned int cpu;
+       int ret;
+
Quoted 9 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 282

Quoted 6 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +6 -0
+       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;

An error, but we continue? Why not dev_dbg level?

Moved to dev_dbg.

Quoted 31 lines
Quoted 31 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +31 -0
+               }
+
+               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_hw_driver_probe(struct platform_device *pdev)
+{
+       int rc;
+
+       /* Get the bases of cpufreq for domains */
+       rc = qcom_resources_init(pdev);
+       if (rc) {
+               dev_err(&pdev->dev, "CPUFreq resource init failed\n");
+               return rc;
+       }
+
+       rc = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
+       if (rc) {
Quoted 9 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 318

Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +5 -0
+               dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
+               return rc;
+       }
+
+       dev_info(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");

Move to dev_dbg? We have other ways to know if a driver probes successfully so the whole line isn't really needed.

Moved it to dev_dbg.

Quoted 8 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 323

Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +5 -0
+
+       return 0;
+}
+
+static const struct of_device_id match_table[] = {

Please call it something besides 'match_table'. qcom_cpufreq_hw_match?

Renamed to use 'qcom_cpufreq_hw_match'.

Quoted 5 lines
Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +5 -0
+       { .compatible = "qcom,cpufreq-hw", .data = &cpufreq_qcom_std_offsets },
+       {}
+};
+
+static struct platform_driver qcom_cpufreq_hw_driver = {
Quoted 9 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 333

Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +5 -0
+       .probe = qcom_cpufreq_hw_driver_probe,
+       .driver = {
+               .name = "qcom-cpufreq-hw",
+               .of_match_table = match_table,
+               .owner = THIS_MODULE,

platform_driver_register() already assigns this. This should be dropped from here.

Removed.

Quoted 5 lines
Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +5 -0
+       },
+};
+
+static int __init qcom_cpufreq_hw_init(void)
+{
Quoted 9 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 344

Quoted 6 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +6 -0
+       return platform_driver_register(&qcom_cpufreq_hw_driver);
+}
+subsys_initcall(qcom_cpufreq_hw_init);
+
+MODULE_DESCRIPTION("QCOM firmware-based CPU Frequency driver");
+MODULE_LICENSE("GPL v2");

It should be tristate then in the Kconfig.

Removed the module license and want to keep it inbuilt.

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

--
#6 lore raw
In reply to Taniya Das

Hi,

Quoted 72 lines · Taniya Das

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

---
 drivers/cpufreq/Kconfig.arm       |  10 ++
 drivers/cpufreq/Makefile          |   1 +
 drivers/cpufreq/qcom-cpufreq-hw.c | 344 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 355 insertions(+)
 create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
drivers/cpufreq/Kconfig.arm +10 -0
index 52f5f1a..141ec3e 100644
@@ -312,3 +312,13 @@ config ARM_PXA2xx_CPUFREQ
 	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
 
 	  If in doubt, say N.
+
+config ARM_QCOM_CPUFREQ_HW
+	bool "QCOM CPUFreq HW driver"
+	help
+	 Support for the CPUFreq HW driver.
+	 Some QCOM chipsets have a HW engine to offload the steps
+	 necessary for changing the frequency of the CPUs. Firmware loaded
+	 in this engine exposes a programming interafce to the High-level OS.
+	 The driver implements the cpufreq driver interface for this HW engine.
+	 Say Y if you want to support CPUFreq HW.
drivers/cpufreq/Makefile +1 -0
index fb4a2ec..1226a3e 100644
@@ -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_HW)	+= qcom-cpufreq-hw.o
 
 
 ##################################################################################
drivers/cpufreq/qcom-cpufreq-hw.c new file +23 -0
new file mode 100644
index 0000000..fa25a95
@@ -0,0 +1,344 @@
+// 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
+
+enum {
+	REG_ENABLE,
+	REG_LUT_TABLE,
+	REG_PERF_STATE,
+

drivers/cpufreq/qcom-cpufreq-hw.c line 33

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+	REG_ARRAY_SIZE,
+};
+
+struct cpufreq_qcom {
+	struct cpufreq_frequency_table *table;
+	struct device *dev;
+	const u16 *reg_offset;
+	void __iomem *base;
+	cpumask_t related_cpus;
+	unsigned int max_cores;

Same comment as on v4:

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. Maybe it's the name from the datasheet? Should it still be 'num_cores' or similer?

drivers/cpufreq/qcom-cpufreq-hw.c line 42

Quoted 1 line
drivers/cpufreq/qcom-cpufreq-hw.c new file +1 -0
+static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];

It would be an option to limit this to the number of CPU clusters and allocate it dynamically when the driver is initialized (key = first core in the cluster). Probably not worth the hassle with the limited number of cores though.

Quoted 14 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +14 -0
+static int qcom_read_lut(struct platform_device *pdev,
+			 struct cpufreq_qcom *c)
+{
+	struct device *dev = &pdev->dev;
+	unsigned int offset;
+	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;
+
+	offset = c->reg_offset[REG_LUT_TABLE];
+

drivers/cpufreq/qcom-cpufreq-hw.c line 155

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
+		data = readl_relaxed(c->base + offset + i * LUT_ROW_SIZE);
+		src = ((data & GENMASK(31, 30)) >> 30);
+		lval = (data & GENMASK(7, 0));
+		core_count = CORE_COUNT_VAL(data);
+
+		if (src == 0)
+			c->table[i].frequency = INIT_RATE / 1000;
+		else
+			c->table[i].frequency = XO_RATE * lval / 1000;

You changed the condition from '!src' to 'src == 0'. My suggestion on v4 was in part about a negative condition, but also about the order. If it doesn't obstruct the code otherwise I think for an if-else branch it is good practice to handle the more common case first and then the 'exception'. I would expect most entries to have an actual rate. Just a nit in any case, feel free to ignore if you prefer as is.

Quoted 41 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +41 -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;
+	unsigned int offset, cpu_r;
+	int ret;
+
+	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+	if (!c)
+		return -ENOMEM;
+
+	c->reg_offset = of_device_get_match_data(&pdev->dev);
+	if (!c->reg_offset)
+		return -EINVAL;
+
+	if (of_address_to_resource(np, 0, &res))
+		return -ENOMEM;
+
+	c->base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!c->base) {
+		dev_err(dev, "Unable to map %s base\n", np->name);
+		return -ENOMEM;
+	}
+
+	offset = c->reg_offset[REG_ENABLE];
+
+	/* HW should be in enabled state to proceed */
+	if (!(readl_relaxed(c->base + offset) & 0x1)) {
+		dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
+		return -ENODEV;
+	}
+
+	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);

drivers/cpufreq/qcom-cpufreq-hw.c line 259

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+	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;

If the general code structure remains as is (see my comment below) the assignment could be done in a 'if (cpu == cpu_r)' branch instead of first assigning and then overwriting it for 'cpu != cpu_r'.

drivers/cpufreq/qcom-cpufreq-hw.c line 266

Quoted 7 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +7 -0
+
+	/* Related CPUs to keep a single copy */
+	cpu_r = cpumask_first(&c->related_cpus);
+	if (cpu != cpu_r) {
+		qcom_freq_domain_map[cpu] = qcom_freq_domain_map[cpu_r];
+		devm_kfree(dev, c);
+	}

Couldn't we do this at the beginning of the function instead of going through allocation, ioremap, read_lut for every core only to throw the information away later for the 'related' CPUs?

qcom_cpu_resources_init() is called with increasing 'cpu' values, hence the 'first' CPU of the cluster is already initialized when the 'related' ones are processed.

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+	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) {

drivers/cpufreq/qcom-cpufreq-hw.c line 287

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+		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);

drivers/cpufreq/qcom-cpufreq-hw.c line 299

Quoted 12 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +12 -0
+			return -EINVAL;
+		}
+
+		of_node_put(cpu_np);
+
+		ret = qcom_cpu_resources_init(pdev, np, cpu);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}

Thanks

Matthias

author
#7 lore raw

Hello Matthias,

Thanks for your review comments.

Quoted 88 lines · Matthias Kaehlcke

Hi,

Quoted 72 lines · Taniya Das

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

---
  drivers/cpufreq/Kconfig.arm       |  10 ++
  drivers/cpufreq/Makefile          |   1 +
  drivers/cpufreq/qcom-cpufreq-hw.c | 344 ++++++++++++++++++++++++++++++++++++++
  3 files changed, 355 insertions(+)

create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c

drivers/cpufreq/Kconfig.arm +10 -0
index 52f5f1a..141ec3e 100644
@@ -312,3 +312,13 @@ config ARM_PXA2xx_CPUFREQ
  	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
 
  	  If in doubt, say N.
+
+config ARM_QCOM_CPUFREQ_HW
+	bool "QCOM CPUFreq HW driver"
+	help
+	 Support for the CPUFreq HW driver.
+	 Some QCOM chipsets have a HW engine to offload the steps
+	 necessary for changing the frequency of the CPUs. Firmware loaded
+	 in this engine exposes a programming interafce to the High-level OS.
+	 The driver implements the cpufreq driver interface for this HW engine.
+	 Say Y if you want to support CPUFreq HW.
drivers/cpufreq/Makefile +1 -0
index fb4a2ec..1226a3e 100644
@@ -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_HW)	+= qcom-cpufreq-hw.o
 
 
  ##################################################################################
drivers/cpufreq/qcom-cpufreq-hw.c new file +23 -0
new file mode 100644
index 0000000..fa25a95
@@ -0,0 +1,344 @@
+// 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
+
+enum {
+	REG_ENABLE,
+	REG_LUT_TABLE,
+	REG_PERF_STATE,
+

drivers/cpufreq/qcom-cpufreq-hw.c line 33

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+	REG_ARRAY_SIZE,
+};
+
+struct cpufreq_qcom {
+	struct cpufreq_frequency_table *table;
+	struct device *dev;
+	const u16 *reg_offset;
+	void __iomem *base;
+	cpumask_t related_cpus;
+	unsigned int max_cores;

Same comment as on v4:

Quoted 5 lines

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. Maybe it's the name from the datasheet? Should it still be 'num_cores' or similer?

Your understanding is correct. I would prefer to leave the naming as 'max_cores'.

Quoted 30 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 42

Quoted 1 line
drivers/cpufreq/qcom-cpufreq-hw.c new file +1 -0
+static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];

It would be an option to limit this to the number of CPU clusters and allocate it dynamically when the driver is initialized (key = first core in the cluster). Probably not worth the hassle with the limited number of cores though.

Quoted 23 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +23 -0
+static int qcom_read_lut(struct platform_device *pdev,
+			 struct cpufreq_qcom *c)
+{
+	struct device *dev = &pdev->dev;
+	unsigned int offset;
+	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;
+
+	offset = c->reg_offset[REG_LUT_TABLE];
+
+	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
+		data = readl_relaxed(c->base + offset + i * LUT_ROW_SIZE);
+		src = ((data & GENMASK(31, 30)) >> 30);
+		lval = (data & GENMASK(7, 0));
+		core_count = CORE_COUNT_VAL(data);
+
+		if (src == 0)
+			c->table[i].frequency = INIT_RATE / 1000;
+		else
Quoted 9 lines

drivers/cpufreq/qcom-cpufreq-hw.c line 155

Quoted 1 line
drivers/cpufreq/qcom-cpufreq-hw.c new file +1 -0
+			c->table[i].frequency = XO_RATE * lval / 1000;

You changed the condition from '!src' to 'src == 0'. My suggestion on v4 was in part about a negative condition, but also about the order. If it doesn't obstruct the code otherwise I think for an if-else branch it is good practice to handle the more common case first and then the 'exception'. I would expect most entries to have an actual rate. Just a nit in any case, feel free to ignore if you prefer as is.

Thanks, Sure, I would take care of it in the next series.

Quoted 68 lines
Quoted 41 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +41 -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;
+	unsigned int offset, cpu_r;
+	int ret;
+
+	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+	if (!c)
+		return -ENOMEM;
+
+	c->reg_offset = of_device_get_match_data(&pdev->dev);
+	if (!c->reg_offset)
+		return -EINVAL;
+
+	if (of_address_to_resource(np, 0, &res))
+		return -ENOMEM;
+
+	c->base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!c->base) {
+		dev_err(dev, "Unable to map %s base\n", np->name);
+		return -ENOMEM;
+	}
+
+	offset = c->reg_offset[REG_ENABLE];
+
+	/* HW should be in enabled state to proceed */
+	if (!(readl_relaxed(c->base + offset) & 0x1)) {
+		dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
+		return -ENODEV;
+	}
+
+	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);

drivers/cpufreq/qcom-cpufreq-hw.c line 259

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+	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;

If the general code structure remains as is (see my comment below) the assignment could be done in a 'if (cpu == cpu_r)' branch instead of first assigning and then overwriting it for 'cpu != cpu_r'.

drivers/cpufreq/qcom-cpufreq-hw.c line 266

Quoted 7 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +7 -0
+
+	/* Related CPUs to keep a single copy */
+	cpu_r = cpumask_first(&c->related_cpus);
+	if (cpu != cpu_r) {
+		qcom_freq_domain_map[cpu] = qcom_freq_domain_map[cpu_r];
+		devm_kfree(dev, c);
+	}

Couldn't we do this at the beginning of the function instead of going through allocation, ioremap, read_lut for every core only to throw the information away later for the 'related' CPUs?

Quoted 4 lines

qcom_cpu_resources_init() is called with increasing 'cpu' values, hence the 'first' CPU of the cluster is already initialized when the 'related' ones are processed.

I would be moving the code to the beginning of the function.

Quoted 40 lines
Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+	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) {

drivers/cpufreq/qcom-cpufreq-hw.c line 287

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+		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);

drivers/cpufreq/qcom-cpufreq-hw.c line 299

Quoted 12 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +12 -0
+			return -EINVAL;
+		}
+
+		of_node_put(cpu_np);
+
+		ret = qcom_cpu_resources_init(pdev, np, cpu);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}

Thanks

Matthias

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

--
maintainer
#8 lore raw
In reply to Taniya Das
Quoted 4 lines · Taniya Das

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

Quoted 8 lines
---
 drivers/cpufreq/Kconfig.arm       |  10 ++
 drivers/cpufreq/Makefile          |   1 +
 drivers/cpufreq/qcom-cpufreq-hw.c | 344 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 355 insertions(+)
 create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c

Have you addressed my concerns that I raised for this patch in the previous version ? You never replied to me as well.

Signature · 1 line
viresh
maintainer
#9 lore raw
In reply to Taniya Das
Quoted 48 lines · Taniya Das
drivers/cpufreq/qcom-cpufreq-hw.c new file +48 -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;
+	unsigned int offset, cpu_r;
+	int ret;
+
+	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+	if (!c)
+		return -ENOMEM;
+
+	c->reg_offset = of_device_get_match_data(&pdev->dev);
+	if (!c->reg_offset)
+		return -EINVAL;
+
+	if (of_address_to_resource(np, 0, &res))
+		return -ENOMEM;
+
+	c->base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!c->base) {
+		dev_err(dev, "Unable to map %s base\n", np->name);
+		return -ENOMEM;
+	}
+
+	offset = c->reg_offset[REG_ENABLE];
+
+	/* HW should be in enabled state to proceed */
+	if (!(readl_relaxed(c->base + offset) & 0x1)) {
+		dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
+		return -ENODEV;
+	}
+
+	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;

drivers/cpufreq/qcom-cpufreq-hw.c line 266

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+	}
+
+	qcom_freq_domain_map[cpu] = c;
+
+	/* Related CPUs to keep a single copy */
+	cpu_r = cpumask_first(&c->related_cpus);
+	if (cpu != cpu_r) {
+		qcom_freq_domain_map[cpu] = qcom_freq_domain_map[cpu_r];
+		devm_kfree(dev, c);
+	}

Sorry about missing this, you have actually worked on my comments.

But I think this isn't the clever way of doing it. You allocate the structures, fill everything and then finally free them because we were related. Why can't we have similar check at the top of this routine and skip everything then ?

Signature · 1 line
viresh
author
#10 lore raw
Quoted 62 lines · Viresh Kumar
Quoted 48 lines · Taniya Das
drivers/cpufreq/qcom-cpufreq-hw.c new file +48 -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;
+	unsigned int offset, cpu_r;
+	int ret;
+
+	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+	if (!c)
+		return -ENOMEM;
+
+	c->reg_offset = of_device_get_match_data(&pdev->dev);
+	if (!c->reg_offset)
+		return -EINVAL;
+
+	if (of_address_to_resource(np, 0, &res))
+		return -ENOMEM;
+
+	c->base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!c->base) {
+		dev_err(dev, "Unable to map %s base\n", np->name);
+		return -ENOMEM;
+	}
+
+	offset = c->reg_offset[REG_ENABLE];
+
+	/* HW should be in enabled state to proceed */
+	if (!(readl_relaxed(c->base + offset) & 0x1)) {
+		dev_err(dev, "%s cpufreq hardware not enabled\n", np->name);
+		return -ENODEV;
+	}
+
+	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;

drivers/cpufreq/qcom-cpufreq-hw.c line 266

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+	}
+
+	qcom_freq_domain_map[cpu] = c;
+
+	/* Related CPUs to keep a single copy */
+	cpu_r = cpumask_first(&c->related_cpus);
+	if (cpu != cpu_r) {
+		qcom_freq_domain_map[cpu] = qcom_freq_domain_map[cpu_r];
+		devm_kfree(dev, c);
+	}

Sorry about missing this, you have actually worked on my comments.

Quoted 5 lines

But I think this isn't the clever way of doing it. You allocate the structures, fill everything and then finally free them because we were related. Why can't we have similar check at the top of this routine and skip everything then ?

Thanks Viresh, I actually replied to all the reviewers with the changes I have made in v5 series, sorry for not replying individually to all reviewers. Yes, I agree I should have put this check at the beginning of the function.

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

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

--
#11 lore raw
In reply to Taniya Das

Hi Taniya,

Quoted 14 lines · Taniya Das

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

---
 drivers/cpufreq/Kconfig.arm       |  10 ++
 drivers/cpufreq/Makefile          |   1 +
 drivers/cpufreq/qcom-cpufreq-hw.c | 344 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 355 insertions(+)
 create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c

...

Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +0 -0
new file mode 100644
index 0000000..fa25a95

...

Quoted 7 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +7 -0
+static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
+{
+       struct cpufreq_qcom *c;
+
+       c = qcom_freq_domain_map[policy->cpu];
+       if (!c) {
+               pr_err("No scaling support for CPU%d\n", policy->cpu);

drivers/cpufreq/qcom-cpufreq-hw.c line 110

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+               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;

I haven't looked at this driver in detail, but I have tested it. Instead of the line above, I needed:

return cpufreq_table_validate_and_show(policy, c->table);

Without this the framework thinks that the min and max frequencies are zero, and then you get a warning about an invalid table. I also removed "policy->freq_table = c->table", since validate_and_show does this.
-Evan

author
#12 lore raw

Hello Evan,

Thank you for testing the patch.

Quoted 43 lines · Evan Green

Hi Taniya,

Quoted 14 lines · Taniya Das

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

---
  drivers/cpufreq/Kconfig.arm       |  10 ++
  drivers/cpufreq/Makefile          |   1 +
  drivers/cpufreq/qcom-cpufreq-hw.c | 344 ++++++++++++++++++++++++++++++++++++++
  3 files changed, 355 insertions(+)

create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c

...

Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +0 -0
new file mode 100644
index 0000000..fa25a95

...

Quoted 7 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +7 -0
+static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
+{
+       struct cpufreq_qcom *c;
+
+       c = qcom_freq_domain_map[policy->cpu];
+       if (!c) {
+               pr_err("No scaling support for CPU%d\n", policy->cpu);

drivers/cpufreq/qcom-cpufreq-hw.c line 110

Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10 -0
+               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;

I haven't looked at this driver in detail, but I have tested it. Instead of the line above, I needed:

Quoted 8 lines

return cpufreq_table_validate_and_show(policy, c->table);

Without this the framework thinks that the min and max frequencies are zero, and then you get a warning about an invalid table. I also removed "policy->freq_table = c->table", since validate_and_show does this.
-Evan

The API is no longer supported,

https://patchwork.kernel.org/patch/10320897/

If you are using the 4.14 kernel, you do need to replace

-policy->freq_table = c->table; +cpufreq_table_validate_and_show(policy, c->table);

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

--