LinuxLists.cc - [PATCH v7 0/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[permalink] [raw]

Subject: [PATCH v7 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.

Signed-off-by: Taniya Das <[email protected]>
---
.../bindings/cpufreq/cpufreq-qcom-hw.txt | 172 +++++++++++++++++++++
1 file changed, 172 insertions(+)
create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
new file mode 100644
index 0000000..22d4355
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
@@ -0,0 +1,172 @@
+Qualcomm Technologies, Inc. CPUFREQ Bindings
+
+CPUFREQ HW is a hardware engine used by some Qualcomm Technologies, Inc. (QTI)
+SoCs to manage frequency in hardware. It is capable of controlling frequency
+for multiple clusters.
+
+Properties:
+- compatible
+ Usage: required
+ Value type: <string>
+ Definition: must be "qcom,cpufreq-hw".
+
+- clocks
+ Usage: required
+ Value type: <phandle> From common clock binding.
+ Definition: clock handle for XO clock.
+
+- clock-names
+ Usage: required
+ Value type: <string> From common clock binding.
+ Definition: must be "xo".
+
+* 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";
+
+ clocks = <&rpmhcc RPMH_CXO_CLK>;
+ clock-names = "xo";
+
+ #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.

2018-07-24 10:46:09

[permalink] [raw]

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

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

Signed-off-by: Saravana Kannan <[email protected]>
Signed-off-by: Taniya Das <[email protected]>
---
drivers/cpufreq/Kconfig.arm | 11 ++
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
3 files changed, 360 insertions(+)
create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 0cd8eb7..93a9d72 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -298,3 +298,14 @@ 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"
+ depends on ARCH_QCOM
+ help
+ Support for the CPUFreq HW driver.
+ Some QCOM chipsets have a HW engine to offload the steps
+ necessary for changing the frequency of the CPUs. Firmware loaded
+ in this engine exposes a programming interface to the OS.
+ The driver implements the cpufreq interface for this HW engine.
+ Say Y if you want to support CPUFreq HW.
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index c1ffeab..ca48a1d 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_HW) += qcom-cpufreq-hw.o

##################################################################################
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
new file mode 100644
index 0000000..ea8f7d1
--- /dev/null
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -0,0 +1,348 @@
+// 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 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;
+ void __iomem *reg_bases[REG_ARRAY_SIZE];
+ cpumask_t related_cpus;
+ unsigned int max_cores;
+ unsigned long xo_rate;
+};
+
+static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
+ [REG_ENABLE] = 0x0,
+ [REG_LUT_TABLE] = 0x110,
+ [REG_PERF_STATE] = 0x920,
+};
+
+static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
+
+static int
+qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
+ unsigned int index)
+{
+ struct cpufreq_qcom *c = policy->driver_data;
+
+ writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
+
+ return 0;
+}
+
+static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
+{
+ struct cpufreq_qcom *c;
+ struct cpufreq_policy *policy;
+ unsigned int index;
+
+ policy = cpufreq_cpu_get_raw(cpu);
+ if (!policy)
+ return 0;
+
+ c = policy->driver_data;
+
+ index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
+ index = min(index, LUT_MAX_ENTRIES - 1);
+
+ return policy->freq_table[index].frequency;
+}
+
+static unsigned int
+qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
+ unsigned int target_freq)
+{
+ struct cpufreq_qcom *c = policy->driver_data;
+ int index;
+
+ index = policy->cached_resolved_idx;
+ if (index < 0)
+ return 0;
+
+ writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
+
+ return policy->freq_table[index].frequency;
+}
+
+static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
+{
+ struct cpufreq_qcom *c;
+
+ c = qcom_freq_domain_map[policy->cpu];
+ if (!c) {
+ pr_err("No scaling support for CPU%d\n", policy->cpu);
+ return -ENODEV;
+ }
+
+ cpumask_copy(policy->cpus, &c->related_cpus);
+
+ policy->fast_switch_possible = true;
+ policy->freq_table = c->table;
+ policy->driver_data = c;
+
+ return 0;
+}
+
+static struct freq_attr *qcom_cpufreq_hw_attr[] = {
+ &cpufreq_freq_attr_scaling_available_freqs,
+ &cpufreq_freq_attr_scaling_boost_freqs,
+ NULL
+};
+
+static struct cpufreq_driver cpufreq_qcom_hw_driver = {
+ .flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
+ CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
+ .verify = cpufreq_generic_frequency_table_verify,
+ .target_index = qcom_cpufreq_hw_target_index,
+ .get = qcom_cpufreq_hw_get,
+ .init = qcom_cpufreq_hw_cpu_init,
+ .fast_switch = qcom_cpufreq_hw_fast_switch,
+ .name = "qcom-cpufreq-hw",
+ .attr = qcom_cpufreq_hw_attr,
+ .boost_enabled = true,
+};
+
+static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
+ struct cpufreq_qcom *c)
+{
+ struct device *dev = &pdev->dev;
+ void __iomem *base;
+ 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;
+
+ base = c->reg_bases[REG_LUT_TABLE];
+
+ for (i = 0; i < LUT_MAX_ENTRIES; i++) {
+ data = readl_relaxed(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 = c->xo_rate * lval / 1000;
+ else
+ c->table[i].frequency = INIT_RATE / 1000;
+
+ cur_freq = c->table[i].frequency;
+
+ dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
+ i, c->table[i].frequency, core_count);
+
+ if (core_count != c->max_cores)
+ cur_freq = CPUFREQ_ENTRY_INVALID;
+
+ /*
+ * Two of the same frequencies with the same core counts means
+ * end of table.
+ */
+ if (i > 0 && c->table[i - 1].frequency ==
+ c->table[i].frequency && prev_cc == core_count) {
+ struct cpufreq_frequency_table *prev = &c->table[i - 1];
+
+ if (prev_freq == CPUFREQ_ENTRY_INVALID)
+ prev->flags = CPUFREQ_BOOST_FREQ;
+ break;
+ }
+ prev_cc = core_count;
+ prev_freq = cur_freq;
+ }
+
+ c->table[i].frequency = CPUFREQ_TABLE_END;
+
+ return 0;
+}
+
+static int qcom_get_related_cpus(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);
+ of_node_put(cpu_np);
+ if (!freq_np)
+ continue;
+
+ if (freq_np == np)
+ cpumask_set_cpu(cpu, m);
+ }
+
+ return 0;
+}
+
+static int qcom_cpu_resources_init(struct platform_device *pdev,
+ struct device_node *np, unsigned int cpu,
+ unsigned long xo_rate)
+{
+ struct cpufreq_qcom *c;
+ struct resource res;
+ struct device *dev = &pdev->dev;
+ const u16 *offsets;
+ int ret, i, cpu_first, cpu_r;
+ void __iomem *base;
+
+ if (qcom_freq_domain_map[cpu])
+ return 0;
+
+ c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+ if (!c)
+ return -ENOMEM;
+
+ offsets = of_device_get_match_data(&pdev->dev);
+ if (!offsets)
+ return -EINVAL;
+
+ if (of_address_to_resource(np, 0, &res))
+ return -ENOMEM;
+
+ base = devm_ioremap_resource(dev, &res);
+ if (!base)
+ return -ENOMEM;
+
+ for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
+ c->reg_bases[i] = base + offsets[i];
+
+ /* HW should be in enabled state to proceed */
+ if (!(readl_relaxed(c->reg_bases[REG_ENABLE]) & 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;
+
+ c->xo_rate = xo_rate;
+
+ ret = qcom_cpufreq_hw_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;
+
+ /* Related CPUs */
+ cpu_first = cpumask_first(&c->related_cpus);
+
+ for_each_cpu(cpu_r, &c->related_cpus) {
+ if (cpu_r != cpu_first)
+ qcom_freq_domain_map[cpu_r] =
+ qcom_freq_domain_map[cpu_first];
+ }
+
+ return 0;
+}
+
+static int qcom_resources_init(struct platform_device *pdev)
+{
+ struct device_node *np, *cpu_np;
+ struct clk *clk;
+ unsigned int cpu;
+ int ret;
+
+ clk = devm_clk_get(&pdev->dev, "xo");
+ if (IS_ERR(clk))
+ return PTR_ERR(clk);
+
+ for_each_possible_cpu(cpu) {
+ cpu_np = of_cpu_device_node_get(cpu);
+ if (!cpu_np) {
+ dev_dbg(&pdev->dev, "Failed to get cpu %d device\n",
+ cpu);
+ continue;
+ }
+
+ np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
+ of_node_put(cpu_np);
+ if (!np) {
+ dev_err(&pdev->dev, "Failed to get freq-domain device\n");
+ return -EINVAL;
+ }
+
+ ret = qcom_cpu_resources_init(pdev, np, cpu, clk_get_rate(clk));
+ if (ret)
+ return ret;
+ }
+
+ devm_clk_put(&pdev->dev, clk);
+
+ return 0;
+}
+
+static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
+{
+ int rc;
+
+ /* Get the bases of cpufreq for domains */
+ rc = qcom_resources_init(pdev);
+ if (rc) {
+ dev_err(&pdev->dev, "CPUFreq resource init failed\n");
+ return rc;
+ }
+
+ rc = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
+ if (rc) {
+ dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
+ return rc;
+ }
+
+ dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
+
+ return 0;
+}
+
+static const struct of_device_id qcom_cpufreq_hw_match[] = {
+ { .compatible = "qcom,cpufreq-hw", .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 = qcom_cpufreq_hw_match,
+ },
+};
+
+static int __init qcom_cpufreq_hw_init(void)
+{
+ return platform_driver_register(&qcom_cpufreq_hw_driver);
+}
+subsys_initcall(qcom_cpufreq_hw_init);
+
+MODULE_DESCRIPTION("QCOM firmware-based CPU Frequency driver");
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the Linux Foundation.

2018-08-03 19:43:08

by Evan Green

[permalink] [raw]

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

Hi Taniya,

On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <[email protected]> wrote:
>
> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this hardware engine.
>
> Signed-off-by: Saravana Kannan <[email protected]>
> Signed-off-by: Taniya Das <[email protected]>
> ---
> drivers/cpufreq/Kconfig.arm | 11 ++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 360 insertions(+)
> create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>
...
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> new file mode 100644
> index 0000000..ea8f7d1
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
...
> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
> + struct cpufreq_qcom *c)
> +{
> + struct device *dev = &pdev->dev;
> + void __iomem *base;
> + 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;
> +
> + base = c->reg_bases[REG_LUT_TABLE];
> +
> + for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> + data = readl_relaxed(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 = c->xo_rate * lval / 1000;
> + else
> + c->table[i].frequency = INIT_RATE / 1000;

I don't know much about how this hardware works, but based on the
mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
and 3 all mean xo_rate?

Also, is INIT_RATE really constant? It sounds like gpll0 (or
gpll0_out_even?). You're already getting the xo clock, why not get
gpll0's real rate as well?

2018-08-03 19:53:54

[permalink] [raw]

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

On 2018-08-03 12:40, Evan Green wrote:
> Hi Taniya,
>
> On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <[email protected]> wrote:
>>
>> The CPUfreq HW present in some QCOM chipsets offloads the steps
>> necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this hardware engine.
>>
>> Signed-off-by: Saravana Kannan <[email protected]>
>> Signed-off-by: Taniya Das <[email protected]>
>> ---
>> drivers/cpufreq/Kconfig.arm | 11 ++
>> drivers/cpufreq/Makefile | 1 +
>> drivers/cpufreq/qcom-cpufreq-hw.c | 348
>> ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 360 insertions(+)
>> create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>>
> ...
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c
>> b/drivers/cpufreq/qcom-cpufreq-hw.c
>> new file mode 100644
>> index 0000000..ea8f7d1
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> ...
>> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
>> + struct cpufreq_qcom *c)
>> +{
>> + struct device *dev = &pdev->dev;
>> + void __iomem *base;
>> + 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;
>> +
>> + base = c->reg_bases[REG_LUT_TABLE];
>> +
>> + for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> + data = readl_relaxed(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 = c->xo_rate * lval /
>> 1000;
>> + else
>> + c->table[i].frequency = INIT_RATE / 1000;
>
> I don't know much about how this hardware works, but based on the
> mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
> and 3 all mean xo_rate?
>
> Also, is INIT_RATE really constant? It sounds like gpll0 (or
> gpll0_out_even?). You're already getting the xo clock, why not get
> gpll0's real rate as well?

Actually I was about to comment and say NOT to get clocks just to get
their rate. The XO_RATE is just a multiplication factor. This HW/FW can
change in the future and make the multiplication factor to 1KHz. We also
can't really control any of the clocks going to this block from Linux
(it's all locked down). Adding clk_get significantly delays when this
driver can be probed and increases boot up time. The INIT_RATE will
always be 300 MHz independent of what source it's coming from (as in, if
GPLL0 can't give 300, the HW design will be so that we'll find a
different source).

So, I'd like to remove any clock bindings for this driver.

Thanks,
Saravana

2018-08-03 22:25:11

[permalink] [raw]

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

Quoting [email protected] (2018-08-03 12:52:48)
> On 2018-08-03 12:40, Evan Green wrote:
> > Hi Taniya,
> >
> > On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <[email protected]> wrote:
> >>
> >> + if (src)
> >> + c->table[i].frequency = c->xo_rate * lval /
> >> 1000;
> >> + else
> >> + c->table[i].frequency = INIT_RATE / 1000;
> >
> > I don't know much about how this hardware works, but based on the
> > mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
> > and 3 all mean xo_rate?
> >
> > Also, is INIT_RATE really constant? It sounds like gpll0 (or
> > gpll0_out_even?). You're already getting the xo clock, why not get
> > gpll0's real rate as well?
>
> Actually I was about to comment and say NOT to get clocks just to get
> their rate. The XO_RATE is just a multiplication factor. This HW/FW can
> change in the future and make the multiplication factor to 1KHz.

So future changes to this hardware are going to make this register
return the final frequency that we should use? Sounds great! But that
isn't how it's working right now. We need to have XO in the binding here
so the driver can work with different XO frequencies in case that ever
happens. Same story for GPLL0. Hardcoding this stuff in the driver just
means we'll have to swizzle things in the driver when it changes.

> We also
> can't really control any of the clocks going to this block from Linux
> (it's all locked down).

This shouldn't matter. The clocks going to this hardware block are
described by the firmware to the OS by means of the DT node. If the
firmware or the hardware decides to change the input clks then the
binding can have different clk nodes used.

> Adding clk_get significantly delays when this
> driver can be probed and increases boot up time.

Huh? Please fix your bootloader to increase the CPU frequency before
booting the kernel. I really doubt probe defer is going to happen if we
send GPLL0 here (XO is a DT clk so it's definitely registered before
this driver), and trying to work around probe defer because the CPU
won't go fast before that is papering over a larger problem with both
probe defer and bootloaders not supplying enough CPU speed for you.

> The INIT_RATE will
> always be 300 MHz independent of what source it's coming from (as in, if
> GPLL0 can't give 300, the HW design will be so that we'll find a
> different source).
>
> So, I'd like to remove any clock bindings for this driver.

No. Bindings describe how the hardware is connected. Please don't remove
clocks from the binding just because probe defer is a concern.

2018-08-03 23:48:43

[permalink] [raw]

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

Quoting Taniya Das (2018-07-24 03:42:49)
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> new file mode 100644
> index 0000000..22d4355
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> @@ -0,0 +1,172 @@
[...]
> +
> + 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";
> +
> + clocks = <&rpmhcc RPMH_CXO_CLK>;
> + clock-names = "xo";
> +
> + #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>;
> + };

Sorry, this is just not proper DT design. The whole node should have a
reg property, and it should contain two (or three if we're handling the
L3 clk domain?) different offsets for the different power clusters. The
problem seems to still be that we don't have a way to map the CPUs to
the clk domains they're in provided by this hardware block. Making
subnodes is not the solution.

2018-08-06 20:56:01

[permalink] [raw]

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

On 2018-08-03 16:46, Stephen Boyd wrote:
> Quoting Taniya Das (2018-07-24 03:42:49)
>> diff --git
>> a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
>> b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
>> new file mode 100644
>> index 0000000..22d4355
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
>> @@ -0,0 +1,172 @@
> [...]
>> +
>> + 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";
>> +
>> + clocks = <&rpmhcc RPMH_CXO_CLK>;
>> + clock-names = "xo";
>> +
>> + #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>;
>> + };
>
> Sorry, this is just not proper DT design. The whole node should have a
> reg property, and it should contain two (or three if we're handling the
> L3 clk domain?) different offsets for the different power clusters. The
> problem seems to still be that we don't have a way to map the CPUs to
> the clk domains they're in provided by this hardware block. Making
> subnodes is not the solution.

The problem is mapping clock domains to logical CPUs that CPUfreq uses.
The physical CPU to logical CPU mapping can be changed by the kernel
(even through DT if I'm not mistaken). So we need to have a way to tell
in DT which physical CPUs are connected to which CPU freq clock domain.

As for subnodes or not, we don't have any strong opinion, but couple of
other points to consider. Two or more CPUfreq policies might have a
common frequency table (read from HW), but separate control of
frequency. So, you also need a way to group frequency table with CPU
freq policies. If you have a better design, we are open to that
suggestion.

Thanks,
Saravana

2018-08-06 21:31:41

[permalink] [raw]

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

On 2018-08-03 15:24, Stephen Boyd wrote:
> Quoting [email protected] (2018-08-03 12:52:48)
>> On 2018-08-03 12:40, Evan Green wrote:
>> > Hi Taniya,
>> >
>> > On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <[email protected]> wrote:
>> >>
>> >> + if (src)
>> >> + c->table[i].frequency = c->xo_rate * lval /
>> >> 1000;
>> >> + else
>> >> + c->table[i].frequency = INIT_RATE / 1000;
>> >
>> > I don't know much about how this hardware works, but based on the
>> > mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
>> > and 3 all mean xo_rate?
>> >
>> > Also, is INIT_RATE really constant? It sounds like gpll0 (or
>> > gpll0_out_even?). You're already getting the xo clock, why not get
>> > gpll0's real rate as well?
>>
>> Actually I was about to comment and say NOT to get clocks just to get
>> their rate. The XO_RATE is just a multiplication factor. This HW/FW
>> can
>> change in the future and make the multiplication factor to 1KHz.
>
> So future changes to this hardware are going to make this register
> return the final frequency that we should use? Sounds great! But that
> isn't how it's working right now. We need to have XO in the binding
> here
> so the driver can work with different XO frequencies in case that ever
> happens. Same story for GPLL0. Hardcoding this stuff in the driver just
> means we'll have to swizzle things in the driver when it changes.

XO being a different frequency in a Qualcomm chip is way way less likely
than anything else we are discussing here. In the 10+years I've been
there this has never changed.

So, how about making this binding optional? If it's not present we can
make assumptions for XO rate and init rate. That'll handle your
hypothetical use case while also being optimized.

>> We also
>> can't really control any of the clocks going to this block from Linux
>> (it's all locked down).
>
> This shouldn't matter. The clocks going to this hardware block are
> described by the firmware to the OS by means of the DT node. If the
> firmware or the hardware decides to change the input clks then the
> binding can have different clk nodes used.

There are tons of clocks that are input to blocks in an SoC that are
never controlled by Linux. Or are expose in DT because they are set up
ahead of time and can't change. So, why make an exception here?

>> Adding clk_get significantly delays when this
>> driver can be probed and increases boot up time.
>
> Huh? Please fix your bootloader to increase the CPU frequency before
> booting the kernel. I really doubt probe defer is going to happen if we
> send GPLL0 here (XO is a DT clk so it's definitely registered before
> this driver), and trying to work around probe defer because the CPU
> won't go fast before that is papering over a larger problem with both
> probe defer and bootloaders not supplying enough CPU speed for you.

There are multiple reason the bootloader can't run the CPU faster. For
one, it doesn't have thermal handling, etc. And adding all those
features into the bootloader doesn't make sense when Linux already has
support for it.

>> The INIT_RATE will
>> always be 300 MHz independent of what source it's coming from (as in,
>> if
>> GPLL0 can't give 300, the HW design will be so that we'll find a
>> different source).
>>
>> So, I'd like to remove any clock bindings for this driver.
>
> No. Bindings describe how the hardware is connected. Please don't
> remove
> clocks from the binding just because probe defer is a concern.

Binding describes hardware controllable by the OS. That's the reality.
Let's not add mandatory clock bindings for clocks that the OS can't do
anything about.

-Saravana

2018-08-07 11:13:51

by Sudeep Holla

[permalink] [raw]

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

On Mon, Aug 06, 2018 at 01:54:24PM -0700, [email protected] wrote:
> On 2018-08-03 16:46, Stephen Boyd wrote:
> >Quoting Taniya Das (2018-07-24 03:42:49)
> >>diff --git
> >>a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> >>b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> >>new file mode 100644
> >>index 0000000..22d4355
> >>--- /dev/null
> >>+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> >>@@ -0,0 +1,172 @@
> >[...]
> >>+
> >>+ 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";
> >>+
> >>+ clocks = <&rpmhcc RPMH_CXO_CLK>;
> >>+ clock-names = "xo";
> >>+
> >>+ #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>;
> >>+ };
> >
> >Sorry, this is just not proper DT design. The whole node should have a
> >reg property, and it should contain two (or three if we're handling the
> >L3 clk domain?) different offsets for the different power clusters. The
> >problem seems to still be that we don't have a way to map the CPUs to
> >the clk domains they're in provided by this hardware block. Making
> >subnodes is not the solution.
>
> The problem is mapping clock domains to logical CPUs that CPUfreq uses. The
> physical CPU to logical CPU mapping can be changed by the kernel (even
> through DT if I'm not mistaken). So we need to have a way to tell in DT
> which physical CPUs are connected to which CPU freq clock domain.
>

How about passing CPU freq clock domain id as along with phandle in
qcom,freq-domain ?

--
Regards,
Sudeep

2018-08-07 20:09:01

[permalink] [raw]

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

On 2018-08-07 04:12, Sudeep Holla wrote:
> On Mon, Aug 06, 2018 at 01:54:24PM -0700, [email protected] wrote:
>> On 2018-08-03 16:46, Stephen Boyd wrote:
>> >Quoting Taniya Das (2018-07-24 03:42:49)
>> >>diff --git
>> >>a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
>> >>b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
>> >>new file mode 100644
>> >>index 0000000..22d4355
>> >>--- /dev/null
>> >>+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
>> >>@@ -0,0 +1,172 @@
>> >[...]
>> >>+
>> >>+ 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";
>> >>+
>> >>+ clocks = <&rpmhcc RPMH_CXO_CLK>;
>> >>+ clock-names = "xo";
>> >>+
>> >>+ #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>;
>> >>+ };
>> >
>> >Sorry, this is just not proper DT design. The whole node should have a
>> >reg property, and it should contain two (or three if we're handling the
>> >L3 clk domain?) different offsets for the different power clusters. The
>> >problem seems to still be that we don't have a way to map the CPUs to
>> >the clk domains they're in provided by this hardware block. Making
>> >subnodes is not the solution.
>>
>> The problem is mapping clock domains to logical CPUs that CPUfreq
>> uses. The
>> physical CPU to logical CPU mapping can be changed by the kernel (even
>> through DT if I'm not mistaken). So we need to have a way to tell in
>> DT
>> which physical CPUs are connected to which CPU freq clock domain.
>>
>
> How about passing CPU freq clock domain id as along with phandle in
> qcom,freq-domain ?

Now sure what you mean here. There's no such this as CPUfreq clock
domain id. It has policies that are made up of logical CPU numbers.
Logical CPU is not something that you can fix in DT.

-Saravana

2018-08-08 02:47:25

[permalink] [raw]

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

On 8/8/2018 12:54 AM, [email protected] wrote:
> On 2018-08-07 04:12, Sudeep Holla wrote:
>> On Mon, Aug 06, 2018 at 01:54:24PM -0700, [email protected] wrote:
>>> On 2018-08-03 16:46, Stephen Boyd wrote:
>>> >Quoting Taniya Das (2018-07-24 03:42:49)
>>> >>diff --git
>>> >>a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
>>> >>b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
>>> >>new file mode 100644
>>> >>index 0000000..22d4355
>>> >>--- /dev/null
>>> >>+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
>>> >>@@ -0,0 +1,172 @@
>>> >[...]
>>> >>+
>>> >>+               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";
>>> >>+
>>> >>+               clocks = <&rpmhcc RPMH_CXO_CLK>;
>>> >>+               clock-names = "xo";
>>> >>+
>>> >>+               #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>;
>>> >>+               };
>>> >
>>> >Sorry, this is just not proper DT design. The whole node should have a
>>> >reg property, and it should contain two (or three if we're handling the
>>> >L3 clk domain?) different offsets for the different power clusters. The
>>> >problem seems to still be that we don't have a way to map the CPUs to
>>> >the clk domains they're in provided by this hardware block. Making
>>> >subnodes is not the solution.
>>>
>>> The problem is mapping clock domains to logical CPUs that CPUfreq
>>> uses. The
>>> physical CPU to logical CPU mapping can be changed by the kernel (even
>>> through DT if I'm not mistaken). So we need to have a way to tell in DT
>>> which physical CPUs are connected to which CPU freq clock domain.
>>>
>>
>> How about passing CPU freq clock domain id as along with phandle in
>> qcom,freq-domain ?
>
> Now sure what you mean here. There's no such this as CPUfreq clock
> domain id. It has policies that are made up of logical CPU numbers.
> Logical CPU is not something that you can fix in DT.
>
> -Saravana

Sudeep,

Earlier the design was the freq_domain would take the CPU phandles

freq_domain:
cpus = <&cpu0 &cpu1....>;

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

--

2018-08-08 06:16:16

[permalink] [raw]

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

Quoting Taniya Das (2018-08-07 19:46:01)
>
>
> On 8/8/2018 12:54 AM, [email protected] wrote:
> > On 2018-08-07 04:12, Sudeep Holla wrote:
> >> On Mon, Aug 06, 2018 at 01:54:24PM -0700, [email protected] wrote:
> >>> On 2018-08-03 16:46, Stephen Boyd wrote:
> >>> >Quoting Taniya Das (2018-07-24 03:42:49)
> >>> >>diff --git
> >>> >>a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> >>> >>b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> >>> >>new file mode 100644
> >>> >>index 0000000..22d4355
> >>> >>--- /dev/null
> >>> >>+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> >>> >>@@ -0,0 +1,172 @@
> >>> >[...]
> >>> >>+
> >>> >>+               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";
> >>> >>+
> >>> >>+               clocks = <&rpmhcc RPMH_CXO_CLK>;
> >>> >>+               clock-names = "xo";
> >>> >>+
> >>> >>+               #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>;
> >>> >>+               };
> >>> >
> >>> >Sorry, this is just not proper DT design. The whole node should have a
> >>> >reg property, and it should contain two (or three if we're handling the
> >>> >L3 clk domain?) different offsets for the different power clusters. The
> >>> >problem seems to still be that we don't have a way to map the CPUs to
> >>> >the clk domains they're in provided by this hardware block. Making
> >>> >subnodes is not the solution.
> >>>
> >>> The problem is mapping clock domains to logical CPUs that CPUfreq
> >>> uses. The
> >>> physical CPU to logical CPU mapping can be changed by the kernel (even
> >>> through DT if I'm not mistaken). So we need to have a way to tell in DT
> >>> which physical CPUs are connected to which CPU freq clock domain.
> >>>
> >>
> >> How about passing CPU freq clock domain id as along with phandle in
> >> qcom,freq-domain ?
> >
> > Now sure what you mean here. There's no such this as CPUfreq clock
> > domain id. It has policies that are made up of logical CPU numbers.
> > Logical CPU is not something that you can fix in DT.
> >
> > -Saravana
>
> Sudeep,
>
> Earlier the design was the freq_domain would take the CPU phandles
>
> freq_domain:
> cpus = <&cpu0 &cpu1....>;
>

I believe Sudeep is recommending something I recommended earlier. It
would look like:

cpu7 {
qcom,freq-domain = <&cpufreq_hw 1>;
}

to indicate that cpu7 is in cpufreq_hw's frequency domain #1. That
should probably be called clk domain BTW.

If that was done with a phandle and a single cell, then we should have
something similar on the cpufreq_hw node side indicating how to parse
the cells in qcom,freq-domain. A property like #qcom,freq-domain-cells =
<1> to indicate that one u32 follows the phandle.

2018-08-08 06:24:32

[permalink] [raw]

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

Quoting [email protected] (2018-08-06 13:46:05)
> On 2018-08-03 15:24, Stephen Boyd wrote:
> > Quoting [email protected] (2018-08-03 12:52:48)
> >> On 2018-08-03 12:40, Evan Green wrote:
> >> > Hi Taniya,
> >> >
> >> > On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <[email protected]> wrote:
> >> >>
> >> >> + if (src)
> >> >> + c->table[i].frequency = c->xo_rate * lval /
> >> >> 1000;
> >> >> + else
> >> >> + c->table[i].frequency = INIT_RATE / 1000;
> >> >
> >> > I don't know much about how this hardware works, but based on the
> >> > mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
> >> > and 3 all mean xo_rate?
> >> >
> >> > Also, is INIT_RATE really constant? It sounds like gpll0 (or
> >> > gpll0_out_even?). You're already getting the xo clock, why not get
> >> > gpll0's real rate as well?
> >>
> >> Actually I was about to comment and say NOT to get clocks just to get
> >> their rate. The XO_RATE is just a multiplication factor. This HW/FW
> >> can
> >> change in the future and make the multiplication factor to 1KHz.
> >
> > So future changes to this hardware are going to make this register
> > return the final frequency that we should use? Sounds great! But that
> > isn't how it's working right now. We need to have XO in the binding
> > here
> > so the driver can work with different XO frequencies in case that ever
> > happens. Same story for GPLL0. Hardcoding this stuff in the driver just
> > means we'll have to swizzle things in the driver when it changes.
>
> XO being a different frequency in a Qualcomm chip is way way less likely
> than anything else we are discussing here. In the 10+years I've been
> there this has never changed.
>
> So, how about making this binding optional? If it's not present we can
> make assumptions for XO rate and init rate. That'll handle your
> hypothetical use case while also being optimized.

Optional properties are almost never correct for clks. Either the clk
goes there or it doesn't go there. The only time it's optional is if the
HW has the choice of generating the clk itself internally or to use an
external clk as input.

In this case, it's not optional, it's actually used so marking it
optional is highly confusing.

>
> >> We also
> >> can't really control any of the clocks going to this block from Linux
> >> (it's all locked down).
> >
> > This shouldn't matter. The clocks going to this hardware block are
> > described by the firmware to the OS by means of the DT node. If the
> > firmware or the hardware decides to change the input clks then the
> > binding can have different clk nodes used.
>
> There are tons of clocks that are input to blocks in an SoC that are
> never controlled by Linux. Or are expose in DT because they are set up
> ahead of time and can't change. So, why make an exception here?

Because the driver doesn't have to hard-code some frequency that can
easily come from the DT, making it more flexible for frequency plan
changes. It doesn't matter about control of clks at all here, so this
comparison is just plain wrong.

>
> >> The INIT_RATE will
> >> always be 300 MHz independent of what source it's coming from (as in,
> >> if
> >> GPLL0 can't give 300, the HW design will be so that we'll find a
> >> different source).
> >>
> >> So, I'd like to remove any clock bindings for this driver.
> >
> > No. Bindings describe how the hardware is connected. Please don't
> > remove
> > clocks from the binding just because probe defer is a concern.
>
> Binding describes hardware controllable by the OS. That's the reality.
> Let's not add mandatory clock bindings for clocks that the OS can't do
> anything about.
>

It seems that you believe clks should only be used to turn on/off and
control rates. That is not the whole truth. Sometimes clks are there
just to express the clk frequencies that are present in the design so
that drivers can figure out what to do.

2018-08-08 08:39:04

by Sudeep Holla

[permalink] [raw]

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

On Tue, Aug 07, 2018 at 11:15:02PM -0700, Stephen Boyd wrote:
> Quoting Taniya Das (2018-08-07 19:46:01)
[...]

> > Sudeep,
> >
> > Earlier the design was the freq_domain would take the CPU phandles
> >
> > freq_domain:
> > cpus = <&cpu0 &cpu1....>;
> >
>
> I believe Sudeep is recommending something I recommended earlier. It
> would look like:
>
> cpu7 {
> qcom,freq-domain = <&cpufreq_hw 1>;
> }
>
> to indicate that cpu7 is in cpufreq_hw's frequency domain #1. That
> should probably be called clk domain BTW.
>

Thanks Stephen, that's exactly what I meant. You have explained all the
details saving me time :)

> If that was done with a phandle and a single cell, then we should have
> something similar on the cpufreq_hw node side indicating how to parse
> the cells in qcom,freq-domain. A property like #qcom,freq-domain-cells =
> <1> to indicate that one u32 follows the phandle.
>

As Saravana mentions in the other email I can't believe it's just made up
of logical CPU numbers. If that's the case is it software configurable.

--
Regards,
Sudeep

2018-08-08 10:16:38

[permalink] [raw]

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

On 8/8/2018 11:52 AM, Stephen Boyd wrote:
> Quoting [email protected] (2018-08-06 13:46:05)
>> On 2018-08-03 15:24, Stephen Boyd wrote:
>>> Quoting [email protected] (2018-08-03 12:52:48)
>>>> On 2018-08-03 12:40, Evan Green wrote:
>>>>> Hi Taniya,
>>>>>
>>>>> On Tue, Jul 24, 2018 at 3:44 AM Taniya Das <[email protected]> wrote:
>>>>>>
>>>>>> + if (src)
>>>>>> + c->table[i].frequency = c->xo_rate * lval /
>>>>>> 1000;
>>>>>> + else
>>>>>> + c->table[i].frequency = INIT_RATE / 1000;
>>>>>
>>>>> I don't know much about how this hardware works, but based on the
>>>>> mask, src has 4 possible values. So does 0 mean INIT_RATE, and 1, 2,
>>>>> and 3 all mean xo_rate?
>>>>>
>>>>> Also, is INIT_RATE really constant? It sounds like gpll0 (or
>>>>> gpll0_out_even?). You're already getting the xo clock, why not get
>>>>> gpll0's real rate as well?
>>>>
>>>> Actually I was about to comment and say NOT to get clocks just to get
>>>> their rate. The XO_RATE is just a multiplication factor. This HW/FW
>>>> can
>>>> change in the future and make the multiplication factor to 1KHz.
>>>
>>> So future changes to this hardware are going to make this register
>>> return the final frequency that we should use? Sounds great! But that
>>> isn't how it's working right now. We need to have XO in the binding
>>> here
>>> so the driver can work with different XO frequencies in case that ever
>>> happens. Same story for GPLL0. Hardcoding this stuff in the driver just
>>> means we'll have to swizzle things in the driver when it changes.
>>
>> XO being a different frequency in a Qualcomm chip is way way less likely
>> than anything else we are discussing here. In the 10+years I've been
>> there this has never changed.
>>
>> So, how about making this binding optional? If it's not present we can
>> make assumptions for XO rate and init rate. That'll handle your
>> hypothetical use case while also being optimized.
>
> Optional properties are almost never correct for clks. Either the clk
> goes there or it doesn't go there. The only time it's optional is if the
> HW has the choice of generating the clk itself internally or to use an
> external clk as input.
>
> In this case, it's not optional, it's actually used so marking it
> optional is highly confusing.
>
>>
>>>> We also
>>>> can't really control any of the clocks going to this block from Linux
>>>> (it's all locked down).
>>>
>>> This shouldn't matter. The clocks going to this hardware block are
>>> described by the firmware to the OS by means of the DT node. If the
>>> firmware or the hardware decides to change the input clks then the
>>> binding can have different clk nodes used.
>>
>> There are tons of clocks that are input to blocks in an SoC that are
>> never controlled by Linux. Or are expose in DT because they are set up
>> ahead of time and can't change. So, why make an exception here?
>
> Because the driver doesn't have to hard-code some frequency that can
> easily come from the DT, making it more flexible for frequency plan
> changes. It doesn't matter about control of clks at all here, so this
> comparison is just plain wrong.
>
>>
>>>> The INIT_RATE will
>>>> always be 300 MHz independent of what source it's coming from (as in,
>>>> if
>>>> GPLL0 can't give 300, the HW design will be so that we'll find a
>>>> different source).
>>>>
>>>> So, I'd like to remove any clock bindings for this driver.
>>>
>>> No. Bindings describe how the hardware is connected. Please don't
>>> remove
>>> clocks from the binding just because probe defer is a concern.
>>
>> Binding describes hardware controllable by the OS. That's the reality.
>> Let's not add mandatory clock bindings for clocks that the OS can't do
>> anything about.
>>
>
> It seems that you believe clks should only be used to turn on/off and
> control rates. That is not the whole truth. Sometimes clks are there
> just to express the clk frequencies that are present in the design so
> that drivers can figure out what to do.
>

Stephen,

As this clock is not configurable by linux clock drivers and we really
do not care the parent src(as mentioned by Saravana) to generate the
300MHz, would it be good to define a fixed rate clock so as to express
the HW connectivity & frequency?

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

--

2018-08-23 18:39:53

[permalink] [raw]

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

Quoting Taniya Das (2018-08-08 03:15:26)
>
>
> On 8/8/2018 11:52 AM, Stephen Boyd wrote:
> >>
> >> Binding describes hardware controllable by the OS. That's the reality.
> >> Let's not add mandatory clock bindings for clocks that the OS can't do
> >> anything about.
> >>
> >
> > It seems that you believe clks should only be used to turn on/off and
> > control rates. That is not the whole truth. Sometimes clks are there
> > just to express the clk frequencies that are present in the design so
> > that drivers can figure out what to do.
> >
>
> Stephen,
>
> As this clock is not configurable by linux clock drivers and we really
> do not care the parent src(as mentioned by Saravana) to generate the
> 300MHz, would it be good to define a fixed rate clock so as to express
> the HW connectivity & frequency?
>

As a hack that works great, but why do we need to workaround problems by
adding a fixed rate clk to DT for this PLL? The PLL is provided by GCC
node so it should be connected to the GCC node.

2018-08-29 18:02:48

[permalink] [raw]

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

Hi Taniya,

On Tue, Jul 24, 2018 at 04:12:50PM +0530, Taniya Das wrote:
> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this hardware engine.
>
> Signed-off-by: Saravana Kannan <[email protected]>
> Signed-off-by: Taniya Das <[email protected]>
> ---
> drivers/cpufreq/Kconfig.arm | 11 ++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 360 insertions(+)
> create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 0cd8eb7..93a9d72 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -298,3 +298,14 @@ 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"
> + depends on ARCH_QCOM
> + help
> + Support for the CPUFreq HW driver.
> + Some QCOM chipsets have a HW engine to offload the steps
> + necessary for changing the frequency of the CPUs. Firmware loaded
> + in this engine exposes a programming interface to the OS.
> + The driver implements the cpufreq interface for this HW engine.
> + Say Y if you want to support CPUFreq HW.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index c1ffeab..ca48a1d 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_HW) += qcom-cpufreq-hw.o
>
>
> ##################################################################################
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> new file mode 100644
> index 0000000..ea8f7d1
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -0,0 +1,348 @@
> +// 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 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;

'dev' is not used and can be removed.

> ...
>
> +static int qcom_cpu_resources_init(struct platform_device *pdev,
> + struct device_node *np, unsigned int cpu,
> + unsigned long xo_rate)
> +{
> + struct cpufreq_qcom *c;
> + struct resource res;
> + struct device *dev = &pdev->dev;
> + const u16 *offsets;
> + int ret, i, cpu_first, cpu_r;
> + void __iomem *base;
> +
> + if (qcom_freq_domain_map[cpu])
> + return 0;
> +
> + c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> + if (!c)
> + return -ENOMEM;
> +
> + offsets = of_device_get_match_data(&pdev->dev);
> + if (!offsets)
> + return -EINVAL;
> +
> + if (of_address_to_resource(np, 0, &res))
> + return -ENOMEM;
> +
> + base = devm_ioremap_resource(dev, &res);
> + if (!base)
> + return -ENOMEM;
> +
> + for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
> + c->reg_bases[i] = base + offsets[i];
> +
> + /* HW should be in enabled state to proceed */
> + if (!(readl_relaxed(c->reg_bases[REG_ENABLE]) & 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;
> +
> + c->xo_rate = xo_rate;
> +
> + ret = qcom_cpufreq_hw_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;
> +
> + /* Related CPUs */
> + cpu_first = cpumask_first(&c->related_cpus);
> +
> + for_each_cpu(cpu_r, &c->related_cpus) {
> + if (cpu_r != cpu_first)
> + qcom_freq_domain_map[cpu_r] =
> + qcom_freq_domain_map[cpu_first];
> + }

The above ten lines could be simplified to:

for_each_cpu(cpu_r, &c->related_cpus)
qcom_freq_domain_map[cpu_r] = c;

> ...
>
> +static int __init qcom_cpufreq_hw_init(void)
> +{
> + return platform_driver_register(&qcom_cpufreq_hw_driver);
> +}
> +subsys_initcall(qcom_cpufreq_hw_init);

Is subsys_initcall used for a particular reason? It will cause
problems when registering cooling devices, since the thermal device
class is initialized through an fs_initcall, which are executed
later.

Most cpufreq drivers use module_init, device_initcall or
late_initcall, can't this driver use one of those?

Cheers

Matthias

2018-09-09 14:36:52

by Amit Kucheria

[permalink] [raw]

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

Hi Taniya,

How much have you stressed this driver?

I tried it on top of an integration branch based on 4.19-rc2 that we
maintain[1] and was able to get the board to reboot fairly easily with
just a few "yes > /dev/null &" instances running in the background.

I even tried with interconnect framework disabled to be sure, and it
still reboots.

I'll continue trying to pinpoint the source of the problem, but it
would be nice to know what tree you're testing against.

Regards,
Amit
[1] https://git.linaro.org/people/amit.kucheria/kernel.git/log/?h=integration/qcomlt-automerge-result

On Tue, Jul 24, 2018 at 4:12 PM, Taniya Das <[email protected]> wrote:
> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this hardware engine.
>
> Signed-off-by: Saravana Kannan <[email protected]>
> Signed-off-by: Taniya Das <[email protected]>
> ---
> drivers/cpufreq/Kconfig.arm | 11 ++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 360 insertions(+)
> create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 0cd8eb7..93a9d72 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -298,3 +298,14 @@ 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"
> + depends on ARCH_QCOM
> + help
> + Support for the CPUFreq HW driver.
> + Some QCOM chipsets have a HW engine to offload the steps
> + necessary for changing the frequency of the CPUs. Firmware loaded
> + in this engine exposes a programming interface to the OS.
> + The driver implements the cpufreq interface for this HW engine.
> + Say Y if you want to support CPUFreq HW.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index c1ffeab..ca48a1d 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_HW) += qcom-cpufreq-hw.o
>
>
> ##################################################################################
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> new file mode 100644
> index 0000000..ea8f7d1
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -0,0 +1,348 @@
> +// 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 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;
> + void __iomem *reg_bases[REG_ARRAY_SIZE];
> + cpumask_t related_cpus;
> + unsigned int max_cores;
> + unsigned long xo_rate;
> +};
> +
> +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
> + [REG_ENABLE] = 0x0,
> + [REG_LUT_TABLE] = 0x110,
> + [REG_PERF_STATE] = 0x920,
> +};
> +
> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> +
> +static int
> +qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> + unsigned int index)
> +{
> + struct cpufreq_qcom *c = policy->driver_data;
> +
> + writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> +
> + return 0;
> +}
> +
> +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
> +{
> + struct cpufreq_qcom *c;
> + struct cpufreq_policy *policy;
> + unsigned int index;
> +
> + policy = cpufreq_cpu_get_raw(cpu);
> + if (!policy)
> + return 0;
> +
> + c = policy->driver_data;
> +
> + index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
> + index = min(index, LUT_MAX_ENTRIES - 1);
> +
> + return policy->freq_table[index].frequency;
> +}
> +
> +static unsigned int
> +qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
> + unsigned int target_freq)
> +{
> + struct cpufreq_qcom *c = policy->driver_data;
> + int index;
> +
> + index = policy->cached_resolved_idx;
> + if (index < 0)
> + return 0;
> +
> + writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> +
> + return policy->freq_table[index].frequency;
> +}
> +
> +static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> +{
> + struct cpufreq_qcom *c;
> +
> + c = qcom_freq_domain_map[policy->cpu];
> + if (!c) {
> + pr_err("No scaling support for CPU%d\n", policy->cpu);
> + return -ENODEV;
> + }
> +
> + cpumask_copy(policy->cpus, &c->related_cpus);
> +
> + policy->fast_switch_possible = true;
> + policy->freq_table = c->table;
> + policy->driver_data = c;
> +
> + return 0;
> +}
> +
> +static struct freq_attr *qcom_cpufreq_hw_attr[] = {
> + &cpufreq_freq_attr_scaling_available_freqs,
> + &cpufreq_freq_attr_scaling_boost_freqs,
> + NULL
> +};
> +
> +static struct cpufreq_driver cpufreq_qcom_hw_driver = {
> + .flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
> + CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
> + .verify = cpufreq_generic_frequency_table_verify,
> + .target_index = qcom_cpufreq_hw_target_index,
> + .get = qcom_cpufreq_hw_get,
> + .init = qcom_cpufreq_hw_cpu_init,
> + .fast_switch = qcom_cpufreq_hw_fast_switch,
> + .name = "qcom-cpufreq-hw",
> + .attr = qcom_cpufreq_hw_attr,
> + .boost_enabled = true,
> +};
> +
> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
> + struct cpufreq_qcom *c)
> +{
> + struct device *dev = &pdev->dev;
> + void __iomem *base;
> + 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;
> +
> + base = c->reg_bases[REG_LUT_TABLE];
> +
> + for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> + data = readl_relaxed(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 = c->xo_rate * lval / 1000;
> + else
> + c->table[i].frequency = INIT_RATE / 1000;
> +
> + cur_freq = c->table[i].frequency;
> +
> + dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> + i, c->table[i].frequency, core_count);
> +
> + if (core_count != c->max_cores)
> + cur_freq = CPUFREQ_ENTRY_INVALID;
> +
> + /*
> + * Two of the same frequencies with the same core counts means
> + * end of table.
> + */
> + if (i > 0 && c->table[i - 1].frequency ==
> + c->table[i].frequency && prev_cc == core_count) {
> + struct cpufreq_frequency_table *prev = &c->table[i - 1];
> +
> + if (prev_freq == CPUFREQ_ENTRY_INVALID)
> + prev->flags = CPUFREQ_BOOST_FREQ;
> + break;
> + }
> + prev_cc = core_count;
> + prev_freq = cur_freq;
> + }
> +
> + c->table[i].frequency = CPUFREQ_TABLE_END;
> +
> + return 0;
> +}
> +
> +static int qcom_get_related_cpus(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);
> + of_node_put(cpu_np);
> + if (!freq_np)
> + continue;
> +
> + if (freq_np == np)
> + cpumask_set_cpu(cpu, m);
> + }
> +
> + return 0;
> +}
> +
> +static int qcom_cpu_resources_init(struct platform_device *pdev,
> + struct device_node *np, unsigned int cpu,
> + unsigned long xo_rate)
> +{
> + struct cpufreq_qcom *c;
> + struct resource res;
> + struct device *dev = &pdev->dev;
> + const u16 *offsets;
> + int ret, i, cpu_first, cpu_r;
> + void __iomem *base;
> +
> + if (qcom_freq_domain_map[cpu])
> + return 0;
> +
> + c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> + if (!c)
> + return -ENOMEM;
> +
> + offsets = of_device_get_match_data(&pdev->dev);
> + if (!offsets)
> + return -EINVAL;
> +
> + if (of_address_to_resource(np, 0, &res))
> + return -ENOMEM;
> +
> + base = devm_ioremap_resource(dev, &res);
> + if (!base)
> + return -ENOMEM;
> +
> + for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
> + c->reg_bases[i] = base + offsets[i];
> +
> + /* HW should be in enabled state to proceed */
> + if (!(readl_relaxed(c->reg_bases[REG_ENABLE]) & 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;
> +
> + c->xo_rate = xo_rate;
> +
> + ret = qcom_cpufreq_hw_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;
> +
> + /* Related CPUs */
> + cpu_first = cpumask_first(&c->related_cpus);
> +
> + for_each_cpu(cpu_r, &c->related_cpus) {
> + if (cpu_r != cpu_first)
> + qcom_freq_domain_map[cpu_r] =
> + qcom_freq_domain_map[cpu_first];
> + }
> +
> + return 0;
> +}
> +
> +static int qcom_resources_init(struct platform_device *pdev)
> +{
> + struct device_node *np, *cpu_np;
> + struct clk *clk;
> + unsigned int cpu;
> + int ret;
> +
> + clk = devm_clk_get(&pdev->dev, "xo");
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> +
> + for_each_possible_cpu(cpu) {
> + cpu_np = of_cpu_device_node_get(cpu);
> + if (!cpu_np) {
> + dev_dbg(&pdev->dev, "Failed to get cpu %d device\n",
> + cpu);
> + continue;
> + }
> +
> + np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
> + of_node_put(cpu_np);
> + if (!np) {
> + dev_err(&pdev->dev, "Failed to get freq-domain device\n");
> + return -EINVAL;
> + }
> +
> + ret = qcom_cpu_resources_init(pdev, np, cpu, clk_get_rate(clk));
> + if (ret)
> + return ret;
> + }
> +
> + devm_clk_put(&pdev->dev, clk);
> +
> + return 0;
> +}
> +
> +static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
> +{
> + int rc;
> +
> + /* Get the bases of cpufreq for domains */
> + rc = qcom_resources_init(pdev);
> + if (rc) {
> + dev_err(&pdev->dev, "CPUFreq resource init failed\n");
> + return rc;
> + }
> +
> + rc = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
> + if (rc) {
> + dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
> + return rc;
> + }
> +
> + dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
> +
> + return 0;
> +}
> +
> +static const struct of_device_id qcom_cpufreq_hw_match[] = {
> + { .compatible = "qcom,cpufreq-hw", .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 = qcom_cpufreq_hw_match,
> + },
> +};
> +
> +static int __init qcom_cpufreq_hw_init(void)
> +{
> + return platform_driver_register(&qcom_cpufreq_hw_driver);
> +}
> +subsys_initcall(qcom_cpufreq_hw_init);
> +
> +MODULE_DESCRIPTION("QCOM firmware-based CPU Frequency driver");
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
> of the Code Aurora Forum, hosted by the Linux Foundation.
>

2018-09-10 19:31:15

[permalink] [raw]

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

On Tue, Jul 24, 2018 at 04:12:50PM +0530, Taniya Das wrote:
> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this hardware engine.
>
> Signed-off-by: Saravana Kannan <[email protected]>
> Signed-off-by: Taniya Das <[email protected]>
> ---
> drivers/cpufreq/Kconfig.arm | 11 ++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 360 insertions(+)
> create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 0cd8eb7..93a9d72 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -298,3 +298,14 @@ 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"
> + depends on ARCH_QCOM
> + help
> + Support for the CPUFreq HW driver.
> + Some QCOM chipsets have a HW engine to offload the steps
> + necessary for changing the frequency of the CPUs. Firmware loaded
> + in this engine exposes a programming interface to the OS.
> + The driver implements the cpufreq interface for this HW engine.
> + Say Y if you want to support CPUFreq HW.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index c1ffeab..ca48a1d 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_HW) += qcom-cpufreq-hw.o
>
>
> ##################################################################################
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> new file mode 100644
> index 0000000..ea8f7d1
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -0,0 +1,348 @@
> +// 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 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;
> + void __iomem *reg_bases[REG_ARRAY_SIZE];
> + cpumask_t related_cpus;
> + unsigned int max_cores;
> + unsigned long xo_rate;
> +};
> +
> +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
> + [REG_ENABLE] = 0x0,
> + [REG_LUT_TABLE] = 0x110,
> + [REG_PERF_STATE] = 0x920,
> +};
> +
> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> +
> +static int
> +qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> + unsigned int index)
> +{
> + struct cpufreq_qcom *c = policy->driver_data;
> +
> + writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> +
> + return 0;
> +}
> +
> +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
> +{
> + struct cpufreq_qcom *c;
> + struct cpufreq_policy *policy;
> + unsigned int index;
> +
> + policy = cpufreq_cpu_get_raw(cpu);
> + if (!policy)
> + return 0;
> +
> + c = policy->driver_data;
> +
> + index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
> + index = min(index, LUT_MAX_ENTRIES - 1);
> +
> + return policy->freq_table[index].frequency;
> +}
> +
> +static unsigned int
> +qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
> + unsigned int target_freq)
> +{
> + struct cpufreq_qcom *c = policy->driver_data;
> + int index;
> +
> + index = policy->cached_resolved_idx;
> + if (index < 0)
> + return 0;
> +
> + writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> +
> + return policy->freq_table[index].frequency;
> +}
> +
> +static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> +{
> + struct cpufreq_qcom *c;
> +
> + c = qcom_freq_domain_map[policy->cpu];
> + if (!c) {
> + pr_err("No scaling support for CPU%d\n", policy->cpu);
> + return -ENODEV;
> + }
> +
> + cpumask_copy(policy->cpus, &c->related_cpus);
> +
> + policy->fast_switch_possible = true;
> + policy->freq_table = c->table;
> + policy->driver_data = c;
> +
> + return 0;
> +}
> +
> +static struct freq_attr *qcom_cpufreq_hw_attr[] = {
> + &cpufreq_freq_attr_scaling_available_freqs,
> + &cpufreq_freq_attr_scaling_boost_freqs,
> + NULL
> +};
> +
> +static struct cpufreq_driver cpufreq_qcom_hw_driver = {
> + .flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
> + CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
> + .verify = cpufreq_generic_frequency_table_verify,
> + .target_index = qcom_cpufreq_hw_target_index,
> + .get = qcom_cpufreq_hw_get,
> + .init = qcom_cpufreq_hw_cpu_init,
> + .fast_switch = qcom_cpufreq_hw_fast_switch,
> + .name = "qcom-cpufreq-hw",
> + .attr = qcom_cpufreq_hw_attr,
> + .boost_enabled = true,
> +};
> +
> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
> + struct cpufreq_qcom *c)
> +{
> + struct device *dev = &pdev->dev;
> + void __iomem *base;
> + 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;
> +
> + base = c->reg_bases[REG_LUT_TABLE];
> +
> + for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> + data = readl_relaxed(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 = c->xo_rate * lval / 1000;
> + else
> + c->table[i].frequency = INIT_RATE / 1000;
> +
> + cur_freq = c->table[i].frequency;
> +
> + dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> + i, c->table[i].frequency, core_count);
> +
> + if (core_count != c->max_cores)
> + cur_freq = CPUFREQ_ENTRY_INVALID;
> +

I noticed that the 'power_allocator' thermal governor currently can't
be used with this driver since there is no OPP table with frequency and
voltage information. Does the LUT contain information about the
voltage or is there another mechanism to retrieve it?

2018-09-23 09:35:59

[permalink] [raw]

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

Hello Stephen,

On 8/24/2018 12:08 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-08-08 03:15:26)
>>
>>
>> On 8/8/2018 11:52 AM, Stephen Boyd wrote:
>>>>
>>>> Binding describes hardware controllable by the OS. That's the reality.
>>>> Let's not add mandatory clock bindings for clocks that the OS can't do
>>>> anything about.
>>>>
>>>
>>> It seems that you believe clks should only be used to turn on/off and
>>> control rates. That is not the whole truth. Sometimes clks are there
>>> just to express the clk frequencies that are present in the design so
>>> that drivers can figure out what to do.
>>>
>>
>> Stephen,
>>
>> As this clock is not configurable by linux clock drivers and we really
>> do not care the parent src(as mentioned by Saravana) to generate the
>> 300MHz, would it be good to define a fixed rate clock so as to express
>> the HW connectivity & frequency?
>>
>
> As a hack that works great, but why do we need to workaround problems by
> adding a fixed rate clk to DT for this PLL? The PLL is provided by GCC
> node so it should be connected to the GCC node.
>

Please help with review the next patch series which would take the PLL
phandle from DT.

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

--

2018-09-23 09:41:35

[permalink] [raw]

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

Hello Matthias,

Thanks for your review comments.

On 8/29/2018 11:31 PM, Matthias Kaehlcke wrote:
> Hi Taniya,
>
> On Tue, Jul 24, 2018 at 04:12:50PM +0530, Taniya Das wrote:
>> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this hardware engine.
>>
>> Signed-off-by: Saravana Kannan <[email protected]>
>> Signed-off-by: Taniya Das <[email protected]>
>> ---
>> drivers/cpufreq/Kconfig.arm | 11 ++
>> drivers/cpufreq/Makefile | 1 +
>> drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 360 insertions(+)
>> create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 0cd8eb7..93a9d72 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -298,3 +298,14 @@ 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"
>> + depends on ARCH_QCOM
>> + help
>> + Support for the CPUFreq HW driver.
>> + Some QCOM chipsets have a HW engine to offload the steps
>> + necessary for changing the frequency of the CPUs. Firmware loaded
>> + in this engine exposes a programming interface to the OS.
>> + The driver implements the cpufreq interface for this HW engine.
>> + Say Y if you want to support CPUFreq HW.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index c1ffeab..ca48a1d 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_HW) += qcom-cpufreq-hw.o
>>
>>
>> ##################################################################################
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>> new file mode 100644
>> index 0000000..ea8f7d1
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -0,0 +1,348 @@
>> +// 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 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;
>
> 'dev' is not used and can be removed.
>

Thanks, would remove in the next patch.

>> ...
>>
>> +static int qcom_cpu_resources_init(struct platform_device *pdev,
>> + struct device_node *np, unsigned int cpu,
>> + unsigned long xo_rate)
>> +{
>> + struct cpufreq_qcom *c;
>> + struct resource res;
>> + struct device *dev = &pdev->dev;
>> + const u16 *offsets;
>> + int ret, i, cpu_first, cpu_r;
>> + void __iomem *base;
>> +
>> + if (qcom_freq_domain_map[cpu])
>> + return 0;
>> +
>> + c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
>> + if (!c)
>> + return -ENOMEM;
>> +
>> + offsets = of_device_get_match_data(&pdev->dev);
>> + if (!offsets)
>> + return -EINVAL;
>> +
>> + if (of_address_to_resource(np, 0, &res))
>> + return -ENOMEM;
>> +
>> + base = devm_ioremap_resource(dev, &res);
>> + if (!base)
>> + return -ENOMEM;
>> +
>> + for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
>> + c->reg_bases[i] = base + offsets[i];
>> +
>> + /* HW should be in enabled state to proceed */
>> + if (!(readl_relaxed(c->reg_bases[REG_ENABLE]) & 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;
>> +
>> + c->xo_rate = xo_rate;
>> +
>> + ret = qcom_cpufreq_hw_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;
>> +
>> + /* Related CPUs */
>> + cpu_first = cpumask_first(&c->related_cpus);
>> +
>> + for_each_cpu(cpu_r, &c->related_cpus) {
>> + if (cpu_r != cpu_first)
>> + qcom_freq_domain_map[cpu_r] =
>> + qcom_freq_domain_map[cpu_first];
>> + }
>
> The above ten lines could be simplified to:
>
> for_each_cpu(cpu_r, &c->related_cpus)
> qcom_freq_domain_map[cpu_r] = c;
>

Would clean it up in the next patch.

>> ...
>>
>> +static int __init qcom_cpufreq_hw_init(void)
>> +{
>> + return platform_driver_register(&qcom_cpufreq_hw_driver);
>> +}
>> +subsys_initcall(qcom_cpufreq_hw_init);
>
> Is subsys_initcall used for a particular reason? It will cause
> problems when registering cooling devices, since the thermal device
> class is initialized through an fs_initcall, which are executed
> later.
>
> Most cpufreq drivers use module_init, device_initcall or
> late_initcall, can't this driver use one of those?
>

We want the CPU to be scaling to the highest frequency at the earliest.

> Cheers
>
> Matthias
>

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

--

2018-09-23 09:44:06

[permalink] [raw]

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

Hi Amit,

On 9/9/2018 8:04 PM, Amit Kucheria wrote:
> Hi Taniya,
>
> How much have you stressed this driver?
>
> I tried it on top of an integration branch based on 4.19-rc2 that we
> maintain[1] and was able to get the board to reboot fairly easily with
> just a few "yes > /dev/null &" instances running in the background.
>
> I even tried with interconnect framework disabled to be sure, and it
> still reboots.
>

The driver is being tested with 4.14 kernel version and I have not yet
got any reports of reboots. Though I would also try to check with teams
internally.

> I'll continue trying to pinpoint the source of the problem, but it
> would be nice to know what tree you're testing against.
>
> Regards,
> Amit
> [1] https://git.linaro.org/people/amit.kucheria/kernel.git/log/?h=integration/qcomlt-automerge-result
>
> On Tue, Jul 24, 2018 at 4:12 PM, Taniya Das <[email protected]> wrote:
>> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this hardware engine.
>>
>> Signed-off-by: Saravana Kannan <[email protected]>
>> Signed-off-by: Taniya Das <[email protected]>
>> ---
>> drivers/cpufreq/Kconfig.arm | 11 ++
>> drivers/cpufreq/Makefile | 1 +
>> drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 360 insertions(+)
>> create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 0cd8eb7..93a9d72 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -298,3 +298,14 @@ 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"
>> + depends on ARCH_QCOM
>> + help
>> + Support for the CPUFreq HW driver.
>> + Some QCOM chipsets have a HW engine to offload the steps
>> + necessary for changing the frequency of the CPUs. Firmware loaded
>> + in this engine exposes a programming interface to the OS.
>> + The driver implements the cpufreq interface for this HW engine.
>> + Say Y if you want to support CPUFreq HW.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index c1ffeab..ca48a1d 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_HW) += qcom-cpufreq-hw.o
>>
>>
>> ##################################################################################
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>> new file mode 100644
>> index 0000000..ea8f7d1
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -0,0 +1,348 @@
>> +// 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 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;
>> + void __iomem *reg_bases[REG_ARRAY_SIZE];
>> + cpumask_t related_cpus;
>> + unsigned int max_cores;
>> + unsigned long xo_rate;
>> +};
>> +
>> +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
>> + [REG_ENABLE] = 0x0,
>> + [REG_LUT_TABLE] = 0x110,
>> + [REG_PERF_STATE] = 0x920,
>> +};
>> +
>> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
>> +
>> +static int
>> +qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
>> + unsigned int index)
>> +{
>> + struct cpufreq_qcom *c = policy->driver_data;
>> +
>> + writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
>> +
>> + return 0;
>> +}
>> +
>> +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
>> +{
>> + struct cpufreq_qcom *c;
>> + struct cpufreq_policy *policy;
>> + unsigned int index;
>> +
>> + policy = cpufreq_cpu_get_raw(cpu);
>> + if (!policy)
>> + return 0;
>> +
>> + c = policy->driver_data;
>> +
>> + index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
>> + index = min(index, LUT_MAX_ENTRIES - 1);
>> +
>> + return policy->freq_table[index].frequency;
>> +}
>> +
>> +static unsigned int
>> +qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
>> + unsigned int target_freq)
>> +{
>> + struct cpufreq_qcom *c = policy->driver_data;
>> + int index;
>> +
>> + index = policy->cached_resolved_idx;
>> + if (index < 0)
>> + return 0;
>> +
>> + writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
>> +
>> + return policy->freq_table[index].frequency;
>> +}
>> +
>> +static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>> +{
>> + struct cpufreq_qcom *c;
>> +
>> + c = qcom_freq_domain_map[policy->cpu];
>> + if (!c) {
>> + pr_err("No scaling support for CPU%d\n", policy->cpu);
>> + return -ENODEV;
>> + }
>> +
>> + cpumask_copy(policy->cpus, &c->related_cpus);
>> +
>> + policy->fast_switch_possible = true;
>> + policy->freq_table = c->table;
>> + policy->driver_data = c;
>> +
>> + return 0;
>> +}
>> +
>> +static struct freq_attr *qcom_cpufreq_hw_attr[] = {
>> + &cpufreq_freq_attr_scaling_available_freqs,
>> + &cpufreq_freq_attr_scaling_boost_freqs,
>> + NULL
>> +};
>> +
>> +static struct cpufreq_driver cpufreq_qcom_hw_driver = {
>> + .flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
>> + CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
>> + .verify = cpufreq_generic_frequency_table_verify,
>> + .target_index = qcom_cpufreq_hw_target_index,
>> + .get = qcom_cpufreq_hw_get,
>> + .init = qcom_cpufreq_hw_cpu_init,
>> + .fast_switch = qcom_cpufreq_hw_fast_switch,
>> + .name = "qcom-cpufreq-hw",
>> + .attr = qcom_cpufreq_hw_attr,
>> + .boost_enabled = true,
>> +};
>> +
>> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
>> + struct cpufreq_qcom *c)
>> +{
>> + struct device *dev = &pdev->dev;
>> + void __iomem *base;
>> + 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;
>> +
>> + base = c->reg_bases[REG_LUT_TABLE];
>> +
>> + for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> + data = readl_relaxed(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 = c->xo_rate * lval / 1000;
>> + else
>> + c->table[i].frequency = INIT_RATE / 1000;
>> +
>> + cur_freq = c->table[i].frequency;
>> +
>> + dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
>> + i, c->table[i].frequency, core_count);
>> +
>> + if (core_count != c->max_cores)
>> + cur_freq = CPUFREQ_ENTRY_INVALID;
>> +
>> + /*
>> + * Two of the same frequencies with the same core counts means
>> + * end of table.
>> + */
>> + if (i > 0 && c->table[i - 1].frequency ==
>> + c->table[i].frequency && prev_cc == core_count) {
>> + struct cpufreq_frequency_table *prev = &c->table[i - 1];
>> +
>> + if (prev_freq == CPUFREQ_ENTRY_INVALID)
>> + prev->flags = CPUFREQ_BOOST_FREQ;
>> + break;
>> + }
>> + prev_cc = core_count;
>> + prev_freq = cur_freq;
>> + }
>> +
>> + c->table[i].frequency = CPUFREQ_TABLE_END;
>> +
>> + return 0;
>> +}
>> +
>> +static int qcom_get_related_cpus(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);
>> + of_node_put(cpu_np);
>> + if (!freq_np)
>> + continue;
>> +
>> + if (freq_np == np)
>> + cpumask_set_cpu(cpu, m);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int qcom_cpu_resources_init(struct platform_device *pdev,
>> + struct device_node *np, unsigned int cpu,
>> + unsigned long xo_rate)
>> +{
>> + struct cpufreq_qcom *c;
>> + struct resource res;
>> + struct device *dev = &pdev->dev;
>> + const u16 *offsets;
>> + int ret, i, cpu_first, cpu_r;
>> + void __iomem *base;
>> +
>> + if (qcom_freq_domain_map[cpu])
>> + return 0;
>> +
>> + c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
>> + if (!c)
>> + return -ENOMEM;
>> +
>> + offsets = of_device_get_match_data(&pdev->dev);
>> + if (!offsets)
>> + return -EINVAL;
>> +
>> + if (of_address_to_resource(np, 0, &res))
>> + return -ENOMEM;
>> +
>> + base = devm_ioremap_resource(dev, &res);
>> + if (!base)
>> + return -ENOMEM;
>> +
>> + for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
>> + c->reg_bases[i] = base + offsets[i];
>> +
>> + /* HW should be in enabled state to proceed */
>> + if (!(readl_relaxed(c->reg_bases[REG_ENABLE]) & 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;
>> +
>> + c->xo_rate = xo_rate;
>> +
>> + ret = qcom_cpufreq_hw_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;
>> +
>> + /* Related CPUs */
>> + cpu_first = cpumask_first(&c->related_cpus);
>> +
>> + for_each_cpu(cpu_r, &c->related_cpus) {
>> + if (cpu_r != cpu_first)
>> + qcom_freq_domain_map[cpu_r] =
>> + qcom_freq_domain_map[cpu_first];
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int qcom_resources_init(struct platform_device *pdev)
>> +{
>> + struct device_node *np, *cpu_np;
>> + struct clk *clk;
>> + unsigned int cpu;
>> + int ret;
>> +
>> + clk = devm_clk_get(&pdev->dev, "xo");
>> + if (IS_ERR(clk))
>> + return PTR_ERR(clk);
>> +
>> + for_each_possible_cpu(cpu) {
>> + cpu_np = of_cpu_device_node_get(cpu);
>> + if (!cpu_np) {
>> + dev_dbg(&pdev->dev, "Failed to get cpu %d device\n",
>> + cpu);
>> + continue;
>> + }
>> +
>> + np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
>> + of_node_put(cpu_np);
>> + if (!np) {
>> + dev_err(&pdev->dev, "Failed to get freq-domain device\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = qcom_cpu_resources_init(pdev, np, cpu, clk_get_rate(clk));
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + devm_clk_put(&pdev->dev, clk);
>> +
>> + return 0;
>> +}
>> +
>> +static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
>> +{
>> + int rc;
>> +
>> + /* Get the bases of cpufreq for domains */
>> + rc = qcom_resources_init(pdev);
>> + if (rc) {
>> + dev_err(&pdev->dev, "CPUFreq resource init failed\n");
>> + return rc;
>> + }
>> +
>> + rc = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
>> + if (rc) {
>> + dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
>> + return rc;
>> + }
>> +
>> + dev_dbg(&pdev->dev, "QCOM CPUFreq HW driver initialized\n");
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id qcom_cpufreq_hw_match[] = {
>> + { .compatible = "qcom,cpufreq-hw", .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 = qcom_cpufreq_hw_match,
>> + },
>> +};
>> +
>> +static int __init qcom_cpufreq_hw_init(void)
>> +{
>> + return platform_driver_register(&qcom_cpufreq_hw_driver);
>> +}
>> +subsys_initcall(qcom_cpufreq_hw_init);
>> +
>> +MODULE_DESCRIPTION("QCOM firmware-based CPU Frequency driver");
>> --
>> 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.

--

2018-09-23 09:50:24

[permalink] [raw]

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

On 9/11/2018 1:00 AM, Matthias Kaehlcke wrote:
> On Tue, Jul 24, 2018 at 04:12:50PM +0530, Taniya Das wrote:
>> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this hardware engine.
>>
>> Signed-off-by: Saravana Kannan <[email protected]>
>> Signed-off-by: Taniya Das <[email protected]>
>> ---
>> drivers/cpufreq/Kconfig.arm | 11 ++
>> drivers/cpufreq/Makefile | 1 +
>> drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 360 insertions(+)
>> create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 0cd8eb7..93a9d72 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -298,3 +298,14 @@ 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"
>> + depends on ARCH_QCOM
>> + help
>> + Support for the CPUFreq HW driver.
>> + Some QCOM chipsets have a HW engine to offload the steps
>> + necessary for changing the frequency of the CPUs. Firmware loaded
>> + in this engine exposes a programming interface to the OS.
>> + The driver implements the cpufreq interface for this HW engine.
>> + Say Y if you want to support CPUFreq HW.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index c1ffeab..ca48a1d 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_HW) += qcom-cpufreq-hw.o
>>
>>
>> ##################################################################################
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>> new file mode 100644
>> index 0000000..ea8f7d1
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -0,0 +1,348 @@
>> +// 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 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;
>> + void __iomem *reg_bases[REG_ARRAY_SIZE];
>> + cpumask_t related_cpus;
>> + unsigned int max_cores;
>> + unsigned long xo_rate;
>> +};
>> +
>> +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
>> + [REG_ENABLE] = 0x0,
>> + [REG_LUT_TABLE] = 0x110,
>> + [REG_PERF_STATE] = 0x920,
>> +};
>> +
>> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
>> +
>> +static int
>> +qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
>> + unsigned int index)
>> +{
>> + struct cpufreq_qcom *c = policy->driver_data;
>> +
>> + writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
>> +
>> + return 0;
>> +}
>> +
>> +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
>> +{
>> + struct cpufreq_qcom *c;
>> + struct cpufreq_policy *policy;
>> + unsigned int index;
>> +
>> + policy = cpufreq_cpu_get_raw(cpu);
>> + if (!policy)
>> + return 0;
>> +
>> + c = policy->driver_data;
>> +
>> + index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
>> + index = min(index, LUT_MAX_ENTRIES - 1);
>> +
>> + return policy->freq_table[index].frequency;
>> +}
>> +
>> +static unsigned int
>> +qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
>> + unsigned int target_freq)
>> +{
>> + struct cpufreq_qcom *c = policy->driver_data;
>> + int index;
>> +
>> + index = policy->cached_resolved_idx;
>> + if (index < 0)
>> + return 0;
>> +
>> + writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
>> +
>> + return policy->freq_table[index].frequency;
>> +}
>> +
>> +static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>> +{
>> + struct cpufreq_qcom *c;
>> +
>> + c = qcom_freq_domain_map[policy->cpu];
>> + if (!c) {
>> + pr_err("No scaling support for CPU%d\n", policy->cpu);
>> + return -ENODEV;
>> + }
>> +
>> + cpumask_copy(policy->cpus, &c->related_cpus);
>> +
>> + policy->fast_switch_possible = true;
>> + policy->freq_table = c->table;
>> + policy->driver_data = c;
>> +
>> + return 0;
>> +}
>> +
>> +static struct freq_attr *qcom_cpufreq_hw_attr[] = {
>> + &cpufreq_freq_attr_scaling_available_freqs,
>> + &cpufreq_freq_attr_scaling_boost_freqs,
>> + NULL
>> +};
>> +
>> +static struct cpufreq_driver cpufreq_qcom_hw_driver = {
>> + .flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
>> + CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
>> + .verify = cpufreq_generic_frequency_table_verify,
>> + .target_index = qcom_cpufreq_hw_target_index,
>> + .get = qcom_cpufreq_hw_get,
>> + .init = qcom_cpufreq_hw_cpu_init,
>> + .fast_switch = qcom_cpufreq_hw_fast_switch,
>> + .name = "qcom-cpufreq-hw",
>> + .attr = qcom_cpufreq_hw_attr,
>> + .boost_enabled = true,
>> +};
>> +
>> +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
>> + struct cpufreq_qcom *c)
>> +{
>> + struct device *dev = &pdev->dev;
>> + void __iomem *base;
>> + 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;
>> +
>> + base = c->reg_bases[REG_LUT_TABLE];
>> +
>> + for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> + data = readl_relaxed(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 = c->xo_rate * lval / 1000;
>> + else
>> + c->table[i].frequency = INIT_RATE / 1000;
>> +
>> + cur_freq = c->table[i].frequency;
>> +
>> + dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
>> + i, c->table[i].frequency, core_count);
>> +
>> + if (core_count != c->max_cores)
>> + cur_freq = CPUFREQ_ENTRY_INVALID;
>> +
>
> I noticed that the 'power_allocator' thermal governor currently can't
> be used with this driver since there is no OPP table with frequency and
> voltage information. Does the LUT contain information about the
> voltage or is there another mechanism to retrieve it?
>

No, currently there is no way of reading the voltage information.

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

--

2018-09-24 16:47:13

[permalink] [raw]

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

On Sun, Sep 23, 2018 at 03:18:20PM +0530, Taniya Das wrote:
>
>
> On 9/11/2018 1:00 AM, Matthias Kaehlcke wrote:
> > On Tue, Jul 24, 2018 at 04:12:50PM +0530, Taniya Das wrote:
> > > The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> > > for changing the frequency of CPUs. The driver implements the cpufreq
> > > driver interface for this hardware engine.
> > >
> > > Signed-off-by: Saravana Kannan <[email protected]>
> > > Signed-off-by: Taniya Das <[email protected]>
> > > ---
> > > drivers/cpufreq/Kconfig.arm | 11 ++
> > > drivers/cpufreq/Makefile | 1 +
> > > drivers/cpufreq/qcom-cpufreq-hw.c | 348 ++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 360 insertions(+)
> > > create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
> > >
> > > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > > index 0cd8eb7..93a9d72 100644
> > > --- a/drivers/cpufreq/Kconfig.arm
> > > +++ b/drivers/cpufreq/Kconfig.arm
> > > @@ -298,3 +298,14 @@ 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"
> > > + depends on ARCH_QCOM
> > > + help
> > > + Support for the CPUFreq HW driver.
> > > + Some QCOM chipsets have a HW engine to offload the steps
> > > + necessary for changing the frequency of the CPUs. Firmware loaded
> > > + in this engine exposes a programming interface to the OS.
> > > + The driver implements the cpufreq interface for this HW engine.
> > > + Say Y if you want to support CPUFreq HW.
> > > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> > > index c1ffeab..ca48a1d 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_HW) += qcom-cpufreq-hw.o
> > >
> > >
> > > ##################################################################################
> > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > new file mode 100644
> > > index 0000000..ea8f7d1
> > > --- /dev/null
> > > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > @@ -0,0 +1,348 @@
> > > +// 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 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;
> > > + void __iomem *reg_bases[REG_ARRAY_SIZE];
> > > + cpumask_t related_cpus;
> > > + unsigned int max_cores;
> > > + unsigned long xo_rate;
> > > +};
> > > +
> > > +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
> > > + [REG_ENABLE] = 0x0,
> > > + [REG_LUT_TABLE] = 0x110,
> > > + [REG_PERF_STATE] = 0x920,
> > > +};
> > > +
> > > +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> > > +
> > > +static int
> > > +qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> > > + unsigned int index)
> > > +{
> > > + struct cpufreq_qcom *c = policy->driver_data;
> > > +
> > > + writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
> > > +{
> > > + struct cpufreq_qcom *c;
> > > + struct cpufreq_policy *policy;
> > > + unsigned int index;
> > > +
> > > + policy = cpufreq_cpu_get_raw(cpu);
> > > + if (!policy)
> > > + return 0;
> > > +
> > > + c = policy->driver_data;
> > > +
> > > + index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
> > > + index = min(index, LUT_MAX_ENTRIES - 1);
> > > +
> > > + return policy->freq_table[index].frequency;
> > > +}
> > > +
> > > +static unsigned int
> > > +qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
> > > + unsigned int target_freq)
> > > +{
> > > + struct cpufreq_qcom *c = policy->driver_data;
> > > + int index;
> > > +
> > > + index = policy->cached_resolved_idx;
> > > + if (index < 0)
> > > + return 0;
> > > +
> > > + writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> > > +
> > > + return policy->freq_table[index].frequency;
> > > +}
> > > +
> > > +static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> > > +{
> > > + struct cpufreq_qcom *c;
> > > +
> > > + c = qcom_freq_domain_map[policy->cpu];
> > > + if (!c) {
> > > + pr_err("No scaling support for CPU%d\n", policy->cpu);
> > > + return -ENODEV;
> > > + }
> > > +
> > > + cpumask_copy(policy->cpus, &c->related_cpus);
> > > +
> > > + policy->fast_switch_possible = true;
> > > + policy->freq_table = c->table;
> > > + policy->driver_data = c;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static struct freq_attr *qcom_cpufreq_hw_attr[] = {
> > > + &cpufreq_freq_attr_scaling_available_freqs,
> > > + &cpufreq_freq_attr_scaling_boost_freqs,
> > > + NULL
> > > +};
> > > +
> > > +static struct cpufreq_driver cpufreq_qcom_hw_driver = {
> > > + .flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
> > > + CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
> > > + .verify = cpufreq_generic_frequency_table_verify,
> > > + .target_index = qcom_cpufreq_hw_target_index,
> > > + .get = qcom_cpufreq_hw_get,
> > > + .init = qcom_cpufreq_hw_cpu_init,
> > > + .fast_switch = qcom_cpufreq_hw_fast_switch,
> > > + .name = "qcom-cpufreq-hw",
> > > + .attr = qcom_cpufreq_hw_attr,
> > > + .boost_enabled = true,
> > > +};
> > > +
> > > +static int qcom_cpufreq_hw_read_lut(struct platform_device *pdev,
> > > + struct cpufreq_qcom *c)
> > > +{
> > > + struct device *dev = &pdev->dev;
> > > + void __iomem *base;
> > > + 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;
> > > +
> > > + base = c->reg_bases[REG_LUT_TABLE];
> > > +
> > > + for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> > > + data = readl_relaxed(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 = c->xo_rate * lval / 1000;
> > > + else
> > > + c->table[i].frequency = INIT_RATE / 1000;
> > > +
> > > + cur_freq = c->table[i].frequency;
> > > +
> > > + dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> > > + i, c->table[i].frequency, core_count);
> > > +
> > > + if (core_count != c->max_cores)
> > > + cur_freq = CPUFREQ_ENTRY_INVALID;
> > > +
> >
> > I noticed that the 'power_allocator' thermal governor currently can't
> > be used with this driver since there is no OPP table with frequency and
> > voltage information. Does the LUT contain information about the
> > voltage or is there another mechanism to retrieve it?
> >
>
> No, currently there is no way of reading the voltage information.

That leaves the 'power_allocator' out of question :(

Which thermal governor is/should typically be used on these systems?
Step wise and user space should work out of the box, however the
response of step wise could be slow (step by step) and user space
requires a thermal daemon (and could suffer from latencies). Fair
share could be an option if the thermal cooling devices are registered
with a 'weight', which could come from the device tree.

2018-09-24 17:03:01