LinuxLists.cc - [PATCH 0/7] PM / OPP: per OPP node clock support and imx7ulp cpufreq driver

[permalink] [raw]

Subject: [PATCH 2/7] dt-bindings: PM / OPP: add clocks per OPP node support

It's used for platforms where different OPPs may use different clocks.
With this extended binding, user could specify the correct clock for each
OPP node.

Cc: Viresh Kumar <[email protected]>
Cc: Nishanth Menon <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Frank Rowand <[email protected]>
Cc: [email protected]
Signed-off-by: Dong Aisheng <[email protected]>
---
Documentation/devicetree/bindings/opp/opp.txt | 52 +++++++++++++++++++++++++++
1 file changed, 52 insertions(+)

diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index e36d261..40a4340 100644
--- a/Documentation/devicetree/bindings/opp/opp.txt
+++ b/Documentation/devicetree/bindings/opp/opp.txt
@@ -152,6 +152,11 @@ Optional properties:
hierarchy can be contained in multiple 32 bit values. i.e. <X Y Z1 Z2> in the
above example, Z1 & Z2 refer to the version hierarchy Z.

+- clocks: Clock phandle and specifier used for this opp.
+
+- clock-names: clock names for this opp. The valid clock names are platform
+ specific.
+
- status: Marks the node enabled/disabled.

Example 1: Single cluster Dual-core ARM cortex A9, switch DVFS states together.
@@ -528,3 +533,50 @@ Example 6: opp-microvolt-<name>, opp-microamp-<name>:
};
};
};
+
+Example 7: Single core ARM cortex A7, switch separate clocks for each OPP:
+
+/ {
+ cpus {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ cpu@0 {
+ compatible = "arm,cortex-a7";
+ reg = <0>;
+ next-level-cache = <&L2>;
+ clocks = <&clk_controller 0>;
+ clock-names = "cpu";
+ cpu-supply = <&cpu_supply0>;
+ operating-points-v2 = <&cpu0_opp_table>;
+ };
+ };
+
+ cpu0_opp_table: opp_table0 {
+ compatible = "operating-points-v2";
+ opp-shared;
+
+ opp-1000000000 {
+ opp-hz = /bits/ 64 <1000000000>;
+ opp-microvolt = <975000 970000 985000>;
+ opp-microamp = <70000>;
+ clock-latency-ns = <300000>;
+ clocks = <&clk_controller 0>;
+ opp-suspend;
+ };
+ opp-1100000000 {
+ opp-hz = /bits/ 64 <1100000000>;
+ opp-microvolt = <1000000 980000 1010000>;
+ opp-microamp = <80000>;
+ clocks = <&clk_controller 1>;
+ clock-latency-ns = <310000>;
+ };
+ opp-1200000000 {
+ opp-hz = /bits/ 64 <1200000000>;
+ opp-microvolt = <1025000>;
+ clocks = <&clk_controller 2>;
+ clock-latency-ns = <290000>;
+ turbo-mode;
+ };
+ };
+};
--
2.7.4

2017-08-23 16:10:39

[permalink] [raw]

Subject: [PATCH 4/7] PM / OPP: use OPP node clock to set CPU frequency

If OPP node has a valid clock, we use this corresponding OPP clock to
set the device clock frequency instead of the default opp_table->clk which
is shared by all OPPs.

Cc: Viresh Kumar <[email protected]>
Cc: Nishanth Menon <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
drivers/base/power/opp/core.c | 10 +++++++++-
drivers/base/power/opp/of.c | 8 ++++++++
drivers/base/power/opp/opp.h | 3 +++
3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 2579401..d4656cc 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -600,7 +600,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
struct opp_table *opp_table;
unsigned long freq, old_freq;
struct dev_pm_opp *old_opp, *opp;
- struct clk *clk;
+ struct clk *clk, *old_clk = NULL;
int ret, size;

if (unlikely(!target_freq)) {
@@ -651,6 +651,11 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
goto put_old_opp;
}

+ if (!IS_ERR_OR_NULL(opp->clk)) {
+ old_clk = opp_table->cur_clk;
+ opp_table->cur_clk = opp->clk;
+ }
+
dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__,
old_freq, freq);

@@ -683,6 +688,9 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
ret = opp_table->set_opp(data);
}

+ if (ret && !IS_ERR_OR_NULL(opp->clk))
+ opp_table->cur_clk = old_clk;
+
dev_pm_opp_put(opp);
put_old_opp:
if (!IS_ERR(old_opp))
diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index 57eec1c..64909e7 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -13,6 +13,7 @@

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+#include <linux/clk.h>
#include <linux/cpu.h>
#include <linux/errno.h>
#include <linux/device.h>
@@ -314,6 +315,13 @@ static int _opp_add_static_v2(struct opp_table *opp_table, struct device *dev,
new_opp->dynamic = false;
new_opp->available = true;

+ new_opp->clk = of_clk_get(np, 0);
+ if (IS_ERR(new_opp->clk)) {
+ ret = PTR_ERR(new_opp->clk);
+ if (ret == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+ }
+
if (!of_property_read_u32(np, "clock-latency-ns", &val))
new_opp->clock_latency_ns = val;

diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 30a637c..dbe8ec6 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -60,6 +60,7 @@ extern struct list_head opp_tables;
* @suspend: true if suspend OPP
* @rate: Frequency in hertz
* @supplies: Power supplies voltage/current values
+ * @clk: Clock used for this OPP
* @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
* frequency from any other OPP's frequency.
* @opp_table: points back to the opp_table struct this opp belongs to
@@ -80,6 +81,8 @@ struct dev_pm_opp {

struct dev_pm_opp_supply *supplies;

+ struct clk *clk;
+
unsigned long clock_latency_ns;

struct opp_table *opp_table;
--
2.7.4

2017-08-23 16:10:44

[permalink] [raw]

Subject: [PATCH 6/7] cpufreq: make cpufreq_generic_init transition_latency default to CPUFREQ_ETERNAL

If no valid transition_latency specified, let's make it default to
CPUFREQ_ETERNAL which is consistent with its definition.

This can save some of the same checkings like this:
transition_latency = dev_pm_opp_get_max_transition_latency(cpu_dev);
- if (!transition_latency)
- transition_latency = CPUFREQ_ETERNAL;
ret = cpufreq_generic_init(policy, freq_table, transition_latency);

Cc: Viresh Kumar <[email protected]>
Cc: Nishanth Menon <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
drivers/cpufreq/cpufreq.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 9bf97a3..da07de6 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -180,6 +180,8 @@ int cpufreq_generic_init(struct cpufreq_policy *policy,
return ret;
}

+ if (!transition_latency)
+ transition_latency = CPUFREQ_ETERNAL;
policy->cpuinfo.transition_latency = transition_latency;

/*
--
2.7.4

2017-08-23 16:10:53

[permalink] [raw]

Subject: [PATCH 5/7] PM / OPP: Add dev_pm_opp_get_cur_clk()

User may need to know the current OPP clock for updating policy clk
accordingly. e.g. cpufreq policy clock.
This function does the work.

Cc: Viresh Kumar <[email protected]>
Cc: Nishanth Menon <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
drivers/base/power/opp/core.c | 29 +++++++++++++++++++++++++++++
include/linux/pm_opp.h | 3 +++
2 files changed, 32 insertions(+)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index d4656cc..a425f98 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -1441,6 +1441,35 @@ void dev_pm_opp_put_clkname(struct opp_table *opp_table)
EXPORT_SYMBOL_GPL(dev_pm_opp_put_clkname);

/**
+ * dev_pm_opp_get_cur_clk() - Get the current OPP clock the device is running
+ * @dev: device for which we do this operation
+ *
+ * This must be called before any OPPs are initialized for the device.
+ */
+struct clk *dev_pm_opp_get_cur_clk(struct device *dev)
+{
+ struct opp_table *opp_table;
+ int ret;
+
+ opp_table = dev_pm_opp_get_opp_table(dev);
+ if (!opp_table)
+ return ERR_PTR(-ENOMEM);
+
+ if (IS_ERR(opp_table->cur_clk)) {
+ ret = -ENODEV;
+ goto err;
+ }
+
+ return opp_table->cur_clk;
+
+err:
+ dev_pm_opp_put_opp_table(opp_table);
+
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_cur_clk);
+
+/**
* dev_pm_opp_register_set_opp_helper() - Register custom set OPP helper
* @dev: Device for which the helper is getting registered.
* @set_opp: Custom set OPP helper.
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index a8b1e4d..e20d090 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -123,6 +123,7 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * con
void dev_pm_opp_put_regulators(struct opp_table *opp_table);
struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char * name);
void dev_pm_opp_put_clkname(struct opp_table *opp_table);
+struct clk *dev_pm_opp_get_cur_clk(struct device *dev);
struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data));
void dev_pm_opp_register_put_opp_helper(struct opp_table *opp_table);
struct opp_table *dev_pm_opp_register_set_clk_helper(struct device *dev, int (*set_clk)(struct device *dev, struct clk *clk,
@@ -279,6 +280,8 @@ static inline struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const

static inline void dev_pm_opp_put_clkname(struct opp_table *opp_table) {}

+static inline struct clk *dev_pm_opp_get_cur_clk(struct device *dev) {}
+
static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
{
return -ENOTSUPP;
--
2.7.4

2017-08-23 16:11:09

[permalink] [raw]

Subject: [PATCH 7/7] cpufreq: add imx7ulp cpufreq driver

MX7ULP supports HSRUN mode (528 Mhz), RUN mode (413 Mhz) and VLPR mode
(restricted to FIRC clock, usually 48 Mhz). This patch adds the cpufreq
driver support for HSRUN and RUN mode.
When in different modes, the A7 core is using different clocks:
RUN: clk run_divcore
HSRUN: clk hsrun_divcore

Thus, this driver replies on the newly added features in OPP core framework
in former patches:
"PM / OPP: use OPP node clock to set CPU frequency"

And since IMX7ULP CPU clock setting is different from the generic
set OPP clock, we also implemented a private set_clk function.

Cc: Viresh Kumar <[email protected]>
Cc: Nishanth Menon <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Anson Huang <[email protected]>
Cc: Bai Ping <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
drivers/cpufreq/Kconfig.arm | 8 ++
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/imx7ulp-cpufreq.c | 234 ++++++++++++++++++++++++++++++++++++++
3 files changed, 243 insertions(+)
create mode 100644 drivers/cpufreq/imx7ulp-cpufreq.c

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 2011fec..53664b5 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -90,6 +90,14 @@ config ARM_IMX6Q_CPUFREQ

If in doubt, say N.

+config ARM_IMX7ULP_CPUFREQ
+ tristate "NXP i.MX7ULP cpufreq support"
+ depends on ARCH_MXC
+ help
+ This adds cpufreq driver support for NXP i.MX7ULP series SoCs.
+
+ If in doubt, say N.
+
config ARM_KIRKWOOD_CPUFREQ
def_bool MACH_KIRKWOOD
help
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index ab3a42c..25a1ebd 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -57,6 +57,7 @@ obj-$(CONFIG_ARM_DB8500_CPUFREQ) += dbx500-cpufreq.o
obj-$(CONFIG_ARM_EXYNOS5440_CPUFREQ) += exynos5440-cpufreq.o
obj-$(CONFIG_ARM_HIGHBANK_CPUFREQ) += highbank-cpufreq.o
obj-$(CONFIG_ARM_IMX6Q_CPUFREQ) += imx6q-cpufreq.o
+obj-$(CONFIG_ARM_IMX7ULP_CPUFREQ) += imx7ulp-cpufreq.o
obj-$(CONFIG_ARM_KIRKWOOD_CPUFREQ) += kirkwood-cpufreq.o
obj-$(CONFIG_ARM_MT8173_CPUFREQ) += mt8173-cpufreq.o
obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ) += omap-cpufreq.o
diff --git a/drivers/cpufreq/imx7ulp-cpufreq.c b/drivers/cpufreq/imx7ulp-cpufreq.c
new file mode 100644
index 0000000..92374c6
--- /dev/null
+++ b/drivers/cpufreq/imx7ulp-cpufreq.c
@@ -0,0 +1,234 @@
+/*
+ * Copyright 2017 NXP.
+ *
+ * Author: Dong Aisheng <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/cpu.h>
+#include <linux/cpufreq.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/pm_opp.h>
+#include <linux/pm_qos.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/suspend.h>
+
+#define HSRUN_FREQ 528000000
+#define SMC_PMPROT 0x8
+#define SMC_PMCTRL 0x10
+
+static void __iomem *smc_base;
+
+static struct device *cpu_dev;
+static struct opp_table *opp_table;
+static struct cpufreq_frequency_table *freq_table;
+
+enum IMX7ULP_CPUFREQ_CLKS {
+ RUN_DIVCORE,
+ RUN_SCS_SEL,
+ HSRUN_DIVCORE,
+ HSRUN_SCS_SEL,
+ SPLL_PFD0,
+ SPLL_SEL,
+ FIRC,
+};
+
+static struct clk_bulk_data clks[] = {
+ { .id = "run_divcore" },
+ { .id = "run_scs_sel" },
+ { .id = "hsrun_divcore" },
+ { .id = "hsrun_scs_sel" },
+ { .id = "spll_pfd0" },
+ { .id = "spll_sel" },
+ { .id = "firc" },
+};
+
+static int imx7ulp_set_clk(struct device *dev, struct clk *clk,
+ unsigned long old_freq, unsigned long new_freq)
+{
+ u32 val;
+
+ /*
+ * Before changing the ARM core PLL, change the ARM clock soure
+ * to FIRC first.
+ */
+ if (new_freq >= HSRUN_FREQ) {
+ clk_set_parent(clks[RUN_SCS_SEL].clk, clks[FIRC].clk);
+
+ /* switch to HSRUN mode */
+ val = readl_relaxed(smc_base + SMC_PMCTRL);
+ val |= (0x3 << 8);
+ writel_relaxed(val, smc_base + SMC_PMCTRL);
+
+ /* change the clock rate in HSRUN */
+ clk_set_rate(clks[SPLL_PFD0].clk, new_freq);
+ clk_set_parent(clks[HSRUN_SCS_SEL].clk, clks[SPLL_SEL].clk);
+ } else {
+ /* change the HSRUN clock to firc */
+ clk_set_parent(clks[HSRUN_SCS_SEL].clk, clks[FIRC].clk);
+
+ /* switch to RUN mode */
+ val = readl_relaxed(smc_base + SMC_PMCTRL);
+ val &= ~(0x3 << 8);
+ writel_relaxed(val, smc_base + SMC_PMCTRL);
+
+ clk_set_rate(clks[SPLL_PFD0].clk, new_freq);
+ clk_set_parent(clks[RUN_SCS_SEL].clk, clks[SPLL_SEL].clk);
+ }
+
+ return 0;
+}
+
+static int imx7ulp_set_target(struct cpufreq_policy *policy, unsigned int index)
+{
+ int ret;
+
+ ret = dev_pm_opp_set_rate(cpu_dev,
+ policy->freq_table[index].frequency * 1000);
+ if (ret)
+ return ret;
+
+ policy->clk = dev_pm_opp_get_cur_clk(cpu_dev);
+ if (IS_ERR(policy->clk))
+ ret = PTR_ERR(policy->clk);
+
+ return ret;
+}
+
+static int imx7ulp_cpufreq_init(struct cpufreq_policy *policy)
+{
+ unsigned int transition_latency;
+ int ret;
+
+ policy->clk = clks[RUN_DIVCORE].clk;
+ policy->suspend_freq = dev_pm_opp_get_suspend_opp_freq(cpu_dev) / 1000;
+ transition_latency = dev_pm_opp_get_max_transition_latency(cpu_dev);
+ if (!transition_latency)
+ transition_latency = CPUFREQ_ETERNAL;
+
+ ret = cpufreq_generic_init(policy, freq_table, transition_latency);
+ if (ret) {
+ dev_err(cpu_dev, "imx7ulp cpufreq init failed\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static struct cpufreq_driver imx7ulp_cpufreq_driver = {
+ .flags = CPUFREQ_NEED_INITIAL_FREQ_CHECK,
+ .verify = cpufreq_generic_frequency_table_verify,
+ .target_index = imx7ulp_set_target,
+ .get = cpufreq_generic_get,
+ .init = imx7ulp_cpufreq_init,
+ .name = "imx7ulp-cpufreq",
+ .attr = cpufreq_generic_attr,
+ .suspend = cpufreq_generic_suspend,
+};
+
+static int imx7ulp_cpufreq_probe(struct platform_device *pdev)
+{
+ const char *name = "cpu";
+ struct device_node *np;
+ int ret;
+
+ cpu_dev = get_cpu_device(0);
+ if (!cpu_dev) {
+ pr_err("failed to get cpu0 device\n");
+ return -ENOENT;
+ }
+
+ np = of_find_compatible_node(NULL, NULL, "fsl,imx7ulp-smc1");
+ smc_base = of_iomap(np, 0);
+ of_node_put(np);
+ if (!smc_base)
+ return -ENOMEM;
+
+ ret = clk_bulk_get(cpu_dev, ARRAY_SIZE(clks), clks);
+ if (ret)
+ return ret;
+
+ opp_table = dev_pm_opp_set_regulators(cpu_dev, &name, 1);
+ if (IS_ERR(opp_table)) {
+ ret = PTR_ERR(opp_table);
+ dev_err(cpu_dev, "failed to set regulator %d\n", ret);
+ goto put_clk;
+ }
+
+ opp_table = dev_pm_opp_register_set_clk_helper(cpu_dev,
+ imx7ulp_set_clk);
+ if (IS_ERR(opp_table)) {
+ ret = PTR_ERR(opp_table);
+ dev_err(cpu_dev, "failed to set opp clk %d\n", ret);
+ goto put_reg;
+ }
+
+ ret = dev_pm_opp_of_add_table(cpu_dev);
+ if (ret < 0) {
+ dev_err(cpu_dev, "failed to init OPP table: %d\n", ret);
+ goto put_clk_helper;
+ }
+
+ ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
+ if (ret) {
+ dev_err(cpu_dev, "failed to init cpufreq table\n");
+ goto free_opp_table;
+ }
+
+ ret = cpufreq_register_driver(&imx7ulp_cpufreq_driver);
+ if (ret) {
+ dev_err(cpu_dev, "failed to register cpufreq driver\n");
+ goto free_freq_table;
+ }
+
+ return 0;
+
+free_freq_table:
+ dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
+free_opp_table:
+ dev_pm_opp_of_remove_table(cpu_dev);
+put_clk_helper:
+ dev_pm_opp_register_put_clk_helper(opp_table);
+put_reg:
+ dev_pm_opp_put_regulators(opp_table);
+put_clk:
+ clk_bulk_put(ARRAY_SIZE(clks), clks);
+
+ return ret;
+}
+
+static int imx7ulp_cpufreq_remove(struct platform_device *pdev)
+{
+ cpufreq_unregister_driver(&imx7ulp_cpufreq_driver);
+ dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
+ dev_pm_opp_of_remove_table(cpu_dev);
+ dev_pm_opp_register_put_clk_helper(opp_table);
+ dev_pm_opp_put_regulators(opp_table);
+ clk_bulk_put(ARRAY_SIZE(clks), clks);
+
+ return 0;
+}
+
+static struct platform_driver imx7ulp_cpufreq_platdrv = {
+ .driver = {
+ .name = "imx7ulp-cpufreq",
+ .owner = THIS_MODULE,
+ },
+ .probe = imx7ulp_cpufreq_probe,
+ .remove = imx7ulp_cpufreq_remove,
+};
+
+module_platform_driver(imx7ulp_cpufreq_platdrv);
+
+MODULE_AUTHOR("Dong Aisheng <[email protected]>");
+MODULE_DESCRIPTION("NXP i.MX7ULP cpufreq driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4

2017-08-23 16:11:37

[permalink] [raw]

Subject: [PATCH 3/7] PM / OPP: rename opp_table->clk to opp_table->cur_clk

Since we now support per OPP clocks, we rename opp_table->clk to
opp_table->cur_clk to make it less confusing as the opp_table->clk
will become only represent the current clock of the current OPP
on which the device is running.

Cc: Viresh Kumar <[email protected]>
Cc: Nishanth Menon <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
drivers/base/power/opp/core.c | 30 +++++++++++++++---------------
drivers/base/power/opp/opp.h | 4 ++--
2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 37cf970..2579401 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -527,9 +527,9 @@ _generic_set_opp_clk_only(const struct opp_table *opp_table, struct device *dev,
int ret;

if (opp_table->set_clk)
- return opp_table->set_clk(dev, opp_table->clk, old_freq, freq);
+ return opp_table->set_clk(dev, opp_table->cur_clk, old_freq, freq);

- ret = clk_set_rate(opp_table->clk, freq);
+ ret = clk_set_rate(opp_table->cur_clk, freq);
if (ret) {
dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
ret);
@@ -615,7 +615,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
return PTR_ERR(opp_table);
}

- clk = opp_table->clk;
+ clk = opp_table->cur_clk;
if (IS_ERR(clk)) {
dev_err(dev, "%s: No clock available for the device\n",
__func__);
@@ -750,9 +750,9 @@ static struct opp_table *_allocate_opp_table(struct device *dev)
_of_init_opp_table(opp_table, dev);

/* Find clk for the device */
- opp_table->clk = clk_get(dev, NULL);
- if (IS_ERR(opp_table->clk)) {
- ret = PTR_ERR(opp_table->clk);
+ opp_table->cur_clk = clk_get(dev, NULL);
+ if (IS_ERR(opp_table->cur_clk)) {
+ ret = PTR_ERR(opp_table->cur_clk);
if (ret != -EPROBE_DEFER)
dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__,
ret);
@@ -799,8 +799,8 @@ static void _opp_table_kref_release(struct kref *kref)
struct opp_device *opp_dev;

/* Release clk */
- if (!IS_ERR(opp_table->clk))
- clk_put(opp_table->clk);
+ if (!IS_ERR(opp_table->cur_clk))
+ clk_put(opp_table->cur_clk);

opp_dev = list_first_entry(&opp_table->dev_list, struct opp_device,
node);
@@ -1393,13 +1393,13 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
}

/* Already have default clk set, free it */
- if (!IS_ERR(opp_table->clk))
- clk_put(opp_table->clk);
+ if (!IS_ERR(opp_table->cur_clk))
+ clk_put(opp_table->cur_clk);

/* Find clk for the device */
- opp_table->clk = clk_get(dev, name);
- if (IS_ERR(opp_table->clk)) {
- ret = PTR_ERR(opp_table->clk);
+ opp_table->cur_clk = clk_get(dev, name);
+ if (IS_ERR(opp_table->cur_clk)) {
+ ret = PTR_ERR(opp_table->cur_clk);
if (ret != -EPROBE_DEFER) {
dev_err(dev, "%s: Couldn't find clock: %d\n", __func__,
ret);
@@ -1425,8 +1425,8 @@ void dev_pm_opp_put_clkname(struct opp_table *opp_table)
/* Make sure there are no concurrent readers while updating opp_table */
WARN_ON(!list_empty(&opp_table->opp_list));

- clk_put(opp_table->clk);
- opp_table->clk = ERR_PTR(-EINVAL);
+ clk_put(opp_table->cur_clk);
+ opp_table->cur_clk = ERR_PTR(-EINVAL);

dev_pm_opp_put_opp_table(opp_table);
}
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index c85405e..30a637c 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -132,7 +132,7 @@ enum opp_table_access {
* @supported_hw: Array of version number to support.
* @supported_hw_count: Number of elements in supported_hw array.
* @prop_name: A name to postfix to many DT properties, while parsing them.
- * @clk: Device's clock handle
+ * @cur_clk: Current device's clock handle
* @regulators: Supply regulators
* @regulator_count: Number of power supply regulators
* @set_opp: Platform specific set_opp callback
@@ -169,7 +169,7 @@ struct opp_table {
unsigned int *supported_hw;
unsigned int supported_hw_count;
const char *prop_name;
- struct clk *clk;
+ struct clk *cur_clk;
struct regulator **regulators;
unsigned int regulator_count;

--
2.7.4

2017-08-23 16:12:05

[permalink] [raw]

Subject: [PATCH 1/7] PM / OPP: Add platform specific set_clk function

This is useful to support platforms which only the clk setting is
different from the generic OPP set rate but others like voltage
setting are still the same.

Users can use this function to register a custom OPP set clk helper
working in place of the default simple clk setting in the generic
dev_pm_opp_set_rate(). Then user can still use dev_pm_opp_set_rate()
with .set_clk() to save a lot duplicated work.

Cc: Viresh Kumar <[email protected]>
Cc: Nishanth Menon <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
drivers/base/power/opp/core.c | 90 ++++++++++++++++++++++++++++++++++++++++---
drivers/base/power/opp/opp.h | 4 ++
include/linux/pm_opp.h | 13 +++++++
3 files changed, 102 insertions(+), 5 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index a8cc14f..37cf970 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -521,12 +521,15 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
}

static inline int
-_generic_set_opp_clk_only(struct device *dev, struct clk *clk,
+_generic_set_opp_clk_only(const struct opp_table *opp_table, struct device *dev,
unsigned long old_freq, unsigned long freq)
{
int ret;

- ret = clk_set_rate(clk, freq);
+ if (opp_table->set_clk)
+ return opp_table->set_clk(dev, opp_table->clk, old_freq, freq);
+
+ ret = clk_set_rate(opp_table->clk, freq);
if (ret) {
dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
ret);
@@ -559,7 +562,7 @@ static int _generic_set_opp_regulator(const struct opp_table *opp_table,
}

/* Change frequency */
- ret = _generic_set_opp_clk_only(dev, opp_table->clk, old_freq, freq);
+ ret = _generic_set_opp_clk_only(opp_table, dev, old_freq, freq);
if (ret)
goto restore_voltage;

@@ -573,7 +576,7 @@ static int _generic_set_opp_regulator(const struct opp_table *opp_table,
return 0;

restore_freq:
- if (_generic_set_opp_clk_only(dev, opp_table->clk, freq, old_freq))
+ if (_generic_set_opp_clk_only(opp_table, dev, freq, old_freq))
dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
__func__, old_freq);
restore_voltage:
@@ -653,7 +656,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)

/* Only frequency scaling */
if (!opp_table->regulators) {
- ret = _generic_set_opp_clk_only(dev, clk, old_freq, freq);
+ ret = _generic_set_opp_clk_only(opp_table, dev, old_freq, freq);
} else if (!opp_table->set_opp) {
ret = _generic_set_opp_regulator(opp_table, dev, old_freq, freq,
IS_ERR(old_opp) ? NULL : old_opp->supplies,
@@ -1500,6 +1503,83 @@ void dev_pm_opp_register_put_opp_helper(struct opp_table *opp_table)
EXPORT_SYMBOL_GPL(dev_pm_opp_register_put_opp_helper);

/**
+ * dev_pm_opp_register_set_clk_helper() - Register custom OPP set clk helper
+ * @dev: Device for which the helper is getting registered.
+ * @set_clk: Custom OPP set clk helper.
+ *
+ * This is useful to support complex platforms which only the clk setting
+ * is different from the generic OPP set rate but others like voltage
+ * setting are still the same.
+ *
+ * Users can use this function to register a custom OPP set clk helper
+ * working in place of the default simple clk setting in the generic
+ * dev_pm_opp_set_rate().
+ *
+ * This must be called before any OPPs are initialized for the device.
+ */
+struct opp_table *dev_pm_opp_register_set_clk_helper(struct device *dev,
+ int (*set_clk)(struct device *dev, struct clk *clk,
+ unsigned long old_freq, unsigned long freq))
+{
+ struct opp_table *opp_table;
+ int ret;
+
+ if (!set_clk)
+ return ERR_PTR(-EINVAL);
+
+ opp_table = dev_pm_opp_get_opp_table(dev);
+ if (!opp_table)
+ return ERR_PTR(-ENOMEM);
+
+ /* This should be called before OPPs are initialized */
+ if (WARN_ON(!list_empty(&opp_table->opp_list))) {
+ ret = -EBUSY;
+ goto err;
+ }
+
+ /* Already have custom set_clk helper */
+ if (WARN_ON(opp_table->set_clk)) {
+ ret = -EBUSY;
+ goto err;
+ }
+
+ opp_table->set_clk = set_clk;
+
+ return opp_table;
+
+err:
+ dev_pm_opp_put_opp_table(opp_table);
+
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_register_set_clk_helper);
+
+/**
+ * dev_pm_opp_register_put_clk_helper() - Releases resources blocked for
+ * set_clk helper
+ * @opp_table: OPP table returned from dev_pm_opp_register_set_clk_helper().
+ *
+ * Release resources blocked for platform specific set_clk helper.
+ */
+void dev_pm_opp_register_put_clk_helper(struct opp_table *opp_table)
+{
+ if (!opp_table->set_clk) {
+ pr_err("%s: Doesn't have custom set_clk helper set\n",
+ __func__);
+ return;
+ }
+
+ /* Make sure there are no concurrent readers while updating opp_table */
+ WARN_ON(!list_empty(&opp_table->opp_list));
+
+ opp_table->set_clk = NULL;
+
+ dev_pm_opp_put_opp_table(opp_table);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_register_put_clk_helper);
+
+
+/**
* dev_pm_opp_add() - Add an OPP table from a table definitions
* @dev: device for which we do this operation
* @freq: Frequency in Hz for this OPP
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 166eef9..c85405e 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -137,6 +137,8 @@ enum opp_table_access {
* @regulator_count: Number of power supply regulators
* @set_opp: Platform specific set_opp callback
* @set_opp_data: Data to be passed to set_opp callback
+ * @set_clk: Platform specific set_clk callback. Different from @set_opp,
+ @set_clk only handles the clk part setting.
* @dentry: debugfs dentry pointer of the real device directory (not links).
* @dentry_name: Name of the real dentry.
*
@@ -174,6 +176,8 @@ struct opp_table {
int (*set_opp)(struct dev_pm_set_opp_data *data);
struct dev_pm_set_opp_data *set_opp_data;

+ int (*set_clk)(struct device *dev, struct clk *clk, unsigned long old_freq, unsigned long freq);
+
#ifdef CONFIG_DEBUG_FS
struct dentry *dentry;
char dentry_name[NAME_MAX];
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 51ec727..a8b1e4d 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -125,6 +125,9 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char * name);
void dev_pm_opp_put_clkname(struct opp_table *opp_table);
struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data));
void dev_pm_opp_register_put_opp_helper(struct opp_table *opp_table);
+struct opp_table *dev_pm_opp_register_set_clk_helper(struct device *dev, int (*set_clk)(struct device *dev, struct clk *clk,
+ unsigned long old_freq, unsigned long freq));
+void dev_pm_opp_register_put_clk_helper(struct opp_table *opp_table);
int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
@@ -245,6 +248,16 @@ static inline struct opp_table *dev_pm_opp_register_set_opp_helper(struct device

static inline void dev_pm_opp_register_put_opp_helper(struct opp_table *opp_table) {}

+
+static inline struct opp_table *dev_pm_opp_register_set_clk_helper(struct device *dev,
+ int (*set_clk)(struct device *dev, struct clk *clk,
+ unsigned long old_freq, unsigned long freq))
+{
+ return ERR_PTR(-ENOTSUPP);
+}
+
+static inline void dev_pm_opp_register_put_clk_helper(struct opp_table *opp_table) {}
+
static inline struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name)
{
return ERR_PTR(-ENOTSUPP);
--
2.7.4

2017-08-31 17:39:23

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH 2/7] dt-bindings: PM / OPP: add clocks per OPP node support

On Thu, Aug 24, 2017 at 12:10:05AM +0800, Dong Aisheng wrote:
> It's used for platforms where different OPPs may use different clocks.
> With this extended binding, user could specify the correct clock for each
> OPP node.
>
> Cc: Viresh Kumar <[email protected]>
> Cc: Nishanth Menon <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Frank Rowand <[email protected]>
> Cc: [email protected]
> Signed-off-by: Dong Aisheng <[email protected]>
> ---
> Documentation/devicetree/bindings/opp/opp.txt | 52 +++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> index e36d261..40a4340 100644
> --- a/Documentation/devicetree/bindings/opp/opp.txt
> +++ b/Documentation/devicetree/bindings/opp/opp.txt
> @@ -152,6 +152,11 @@ Optional properties:
> hierarchy can be contained in multiple 32 bit values. i.e. <X Y Z1 Z2> in the
> above example, Z1 & Z2 refer to the version hierarchy Z.
>
> +- clocks: Clock phandle and specifier used for this opp.
> +
> +- clock-names: clock names for this opp. The valid clock names are platform
> + specific.

You don't need -names if there's only 1 clock. But then how long until
we have a list of any random clocks some how associated with an OPP.

I think this should really be solved within the clock framework. What
you really need is "set my parent clock to the source that can provide X
Hz". Could the assigned-clocks property work here (in the OPP nodes
rather than the cpu nodes)?

> +
> - status: Marks the node enabled/disabled.
>
> Example 1: Single cluster Dual-core ARM cortex A9, switch DVFS states together.
> @@ -528,3 +533,50 @@ Example 6: opp-microvolt-<name>, opp-microamp-<name>:
> };
> };
> };
> +
> +Example 7: Single core ARM cortex A7, switch separate clocks for each OPP:

Can't you add this to an existing example? We don't need to enumerate
every possible option.

Rob

2017-09-01 13:01:40

[permalink] [raw]

Subject: Re: [PATCH 2/7] dt-bindings: PM / OPP: add clocks per OPP node support

On Thu, Aug 31, 2017 at 12:39:20PM -0500, Rob Herring wrote:
> On Thu, Aug 24, 2017 at 12:10:05AM +0800, Dong Aisheng wrote:
> > It's used for platforms where different OPPs may use different clocks.
> > With this extended binding, user could specify the correct clock for each
> > OPP node.
> >
> > Cc: Viresh Kumar <[email protected]>
> > Cc: Nishanth Menon <[email protected]>
> > Cc: Stephen Boyd <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Frank Rowand <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Dong Aisheng <[email protected]>
> > ---
> > Documentation/devicetree/bindings/opp/opp.txt | 52 +++++++++++++++++++++++++++
> > 1 file changed, 52 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> > index e36d261..40a4340 100644
> > --- a/Documentation/devicetree/bindings/opp/opp.txt
> > +++ b/Documentation/devicetree/bindings/opp/opp.txt
> > @@ -152,6 +152,11 @@ Optional properties:
> > hierarchy can be contained in multiple 32 bit values. i.e. <X Y Z1 Z2> in the
> > above example, Z1 & Z2 refer to the version hierarchy Z.
> >
> > +- clocks: Clock phandle and specifier used for this opp.
> > +
> > +- clock-names: clock names for this opp. The valid clock names are platform
> > + specific.
>
> You don't need -names if there's only 1 clock.

Got it, will remove -name.

> But then how long until
> we have a list of any random clocks some how associated with an OPP.
>

Not sure whether it will really happen in the future. Even it happens,
as we usually only need the device parent clock, then probably better to
handle the left complicated things in clock driver or OPP device driver.

> I think this should really be solved within the clock framework. What
> you really need is "set my parent clock to the source that can provide X
> Hz". Could the assigned-clocks property work here (in the OPP nodes
> rather than the cpu nodes)?
>

I'm not quite sure i got your point. assigned-clocks seems a bit like function
the same as clocks property here, both are used to specifying the device
parent clock which provide X hz.

Or are you suggesting using assigned-clocks/assigned-clocks-rates instread
of clocks/opp-hz? Would you please clarify a bit more?

> > +
> > - status: Marks the node enabled/disabled.
> >
> > Example 1: Single cluster Dual-core ARM cortex A9, switch DVFS states together.
> > @@ -528,3 +533,50 @@ Example 6: opp-microvolt-<name>, opp-microamp-<name>:
> > };
> > };
> > };
> > +
> > +Example 7: Single core ARM cortex A7, switch separate clocks for each OPP:
>
> Can't you add this to an existing example? We don't need to enumerate
> every possible option.

Okay, Will merge it.

Thanks

Regards
Dong Aisheng

>
> Rob

2017-09-11 07:28:17

[permalink] [raw]

Subject: Re: [PATCH 0/7] PM / OPP: per OPP node clock support and imx7ulp cpufreq driver

Hi Viresh and Rafael,

On Thu, Aug 24, 2017 at 12:10:03AM +0800, Dong Aisheng wrote:
> This patch series does three things:
> 1) Add platform specific set_clk function
> 2) Add per OPP node clock support
> 3) Add imx7ulp cpufreq driver support
>
> 3 is depends on 1 & 2.
>
> MX7ULP supports HSRUN mode (528 Mhz), RUN mode (413 Mhz) and VLPR mode
> (restricted to FIRC clock, usually 48 Mhz). This patch adds the cpufreq
> driver support for HSRUN and RUN mode.
> When in different modes, the A7 core is using different clocks:
> RUN: clk run_divcore
> HSRUN: clk hsrun_divcore
>
> Thus, this driver replies on the newly added features in OPP core framework
> patch 1~6: e.g.
> "PM / OPP: use OPP node clock to set CPU frequency"
>
> And since IMX7ULP CPU clock setting is different from the generic
> set OPP clock, we also implemented a private set_clk function.
>
> Dong Aisheng (7):
> PM / OPP: Add platform specific set_clk function
> dt-bindings: PM / OPP: add clocks per OPP node support
> PM / OPP: rename opp_table->clk to opp_table->cur_clk
> PM / OPP: use OPP node clock to set CPU frequency
> PM / OPP: Add dev_pm_opp_get_cur_clk()
> cpufreq: make cpufreq_generic_init transition_latency default to
> CPUFREQ_ETERNAL
> cpufreq: add imx7ulp cpufreq driver
>

Do you have any comments about this series?

Regards
Dong Aisheng

> Documentation/devicetree/bindings/opp/opp.txt | 52 ++++++
> drivers/base/power/opp/core.c | 155 ++++++++++++++---
> drivers/base/power/opp/of.c | 8 +
> drivers/base/power/opp/opp.h | 11 +-
> drivers/cpufreq/Kconfig.arm | 8 +
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/cpufreq.c | 2 +
> drivers/cpufreq/imx7ulp-cpufreq.c | 234 ++++++++++++++++++++++++++
> include/linux/pm_opp.h | 16 ++
> 9 files changed, 466 insertions(+), 21 deletions(-)
> create mode 100644 drivers/cpufreq/imx7ulp-cpufreq.c
>
> --
> 2.7.4
>

2017-09-19 22:54:17

[permalink] [raw]

Subject: Re: [PATCH 0/7] PM / OPP: per OPP node clock support and imx7ulp cpufreq driver

On 11-09-17, 15:28, Dong Aisheng wrote:
> Do you have any comments about this series?

I wasn't around for last 3-4 weeks and so couldn't get to these.

--
viresh

2017-09-19 22:58:44

[permalink] [raw]

Subject: Re: [PATCH 1/7] PM / OPP: Add platform specific set_clk function

On 24-08-17, 00:10, Dong Aisheng wrote:
> This is useful to support platforms which only the clk setting is
> different from the generic OPP set rate but others like voltage
> setting are still the same.
>
> Users can use this function to register a custom OPP set clk helper
> working in place of the default simple clk setting in the generic
> dev_pm_opp_set_rate(). Then user can still use dev_pm_opp_set_rate()
> with .set_clk() to save a lot duplicated work.

I am not inclined to add this support really. What prevents you to
register a clock for the device (which is CPU in your case) and the
generic clk_set_rate() will eventually call into the platform specific
routine. That's what everyone else is doing.

--
viresh

2017-09-19 23:10:11

[permalink] [raw]

Subject: Re: [PATCH 6/7] cpufreq: make cpufreq_generic_init transition_latency default to CPUFREQ_ETERNAL

On 24-08-17, 00:10, Dong Aisheng wrote:
> If no valid transition_latency specified, let's make it default to
> CPUFREQ_ETERNAL which is consistent with its definition.
>
> This can save some of the same checkings like this:
> transition_latency = dev_pm_opp_get_max_transition_latency(cpu_dev);
> - if (!transition_latency)
> - transition_latency = CPUFREQ_ETERNAL;
> ret = cpufreq_generic_init(policy, freq_table, transition_latency);
>
> Cc: Viresh Kumar <[email protected]>
> Cc: Nishanth Menon <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Signed-off-by: Dong Aisheng <[email protected]>
> ---
> drivers/cpufreq/cpufreq.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 9bf97a3..da07de6 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -180,6 +180,8 @@ int cpufreq_generic_init(struct cpufreq_policy *policy,
> return ret;
> }
>
> + if (!transition_latency)
> + transition_latency = CPUFREQ_ETERNAL;
> policy->cpuinfo.transition_latency = transition_latency;
>
> /*

Can you update all the existing drivers as well (in the same patch)
who can benefit from it?

--
viresh

2017-09-20 07:03:58

[permalink] [raw]

Subject: Re: [PATCH 1/7] PM / OPP: Add platform specific set_clk function

Hi Viresh,

On Tue, Sep 19, 2017 at 03:58:40PM -0700, Viresh Kumar wrote:
> On 24-08-17, 00:10, Dong Aisheng wrote:
> > This is useful to support platforms which only the clk setting is
> > different from the generic OPP set rate but others like voltage
> > setting are still the same.
> >
> > Users can use this function to register a custom OPP set clk helper
> > working in place of the default simple clk setting in the generic
> > dev_pm_opp_set_rate(). Then user can still use dev_pm_opp_set_rate()
> > with .set_clk() to save a lot duplicated work.
>
> I am not inclined to add this support really. What prevents you to
> register a clock for the device (which is CPU in your case) and the
> generic clk_set_rate() will eventually call into the platform specific
> routine. That's what everyone else is doing.
>

I've been thinking of that before.
Actually IMX already does some similar thing for MX5 (no for MX6).
See: clk_cpu_set_rate() in drivers/clk/imx/clk-cpu.c.

After some diggings, it seems MX7ULP is a bit more complicated than before
mainly due to two reasons:
1) It requires to switch to different CPU mode accordingly when setting
clocks rate. That means we need handle this in clock driver as well
which looks not quite suitable although we could do if really want.

2) It uses different clocks for different CPU mode (RUN 416M or
HSRUN 528M), and those clocks have some dependency.
e.g. when setting HSRUN clock, we need change RUN clock parent to make sure
the SPLL_PFD is got disabled before changing rate, as both CPU mode using
the same parent SPLL_PFD clock. Doing this in clock driver also make things
a bit more complicated.

The whole follow would be something like below:
static int imx7ulp_set_clk(struct device *dev, struct clk *clk,
unsigned long old_freq, unsigned long new_freq)
{
u32 val;

/*
* Before changing the ARM core PLL, change the ARM clock soure
* to FIRC first.
*/
if (new_freq >= HSRUN_FREQ) {
clk_set_parent(clks[RUN_SCS_SEL].clk, clks[FIRC].clk);

/* switch to HSRUN mode */
val = readl_relaxed(smc_base + SMC_PMCTRL);
val |= (0x3 << 8);
writel_relaxed(val, smc_base + SMC_PMCTRL);

/* change the clock rate in HSRUN */
clk_set_rate(clks[SPLL_PFD0].clk, new_freq);
clk_set_parent(clks[HSRUN_SCS_SEL].clk, clks[SPLL_SEL].clk);
} else {
/* change the HSRUN clock to firc */
clk_set_parent(clks[HSRUN_SCS_SEL].clk, clks[FIRC].clk);

/* switch to RUN mode */
val = readl_relaxed(smc_base + SMC_PMCTRL);
val &= ~(0x3 << 8);
writel_relaxed(val, smc_base + SMC_PMCTRL);

clk_set_rate(clks[SPLL_PFD0].clk, new_freq);
clk_set_parent(clks[RUN_SCS_SEL].clk, clks[SPLL_SEL].clk);
}

return 0;
}

That's why i thought if we can make OPP core provide a way to handle such
complicated things in platform specific cpufreq driver.

How would you suggest for this issue?

Regards
Dong Aisheng

> --
> viresh

2017-09-20 07:05:01

[permalink] [raw]

Subject: Re: [PATCH 6/7] cpufreq: make cpufreq_generic_init transition_latency default to CPUFREQ_ETERNAL

On Tue, Sep 19, 2017 at 04:10:07PM -0700, Viresh Kumar wrote:
> On 24-08-17, 00:10, Dong Aisheng wrote:
> > If no valid transition_latency specified, let's make it default to
> > CPUFREQ_ETERNAL which is consistent with its definition.
> >
> > This can save some of the same checkings like this:
> > transition_latency = dev_pm_opp_get_max_transition_latency(cpu_dev);
> > - if (!transition_latency)
> > - transition_latency = CPUFREQ_ETERNAL;
> > ret = cpufreq_generic_init(policy, freq_table, transition_latency);
> >
> > Cc: Viresh Kumar <[email protected]>
> > Cc: Nishanth Menon <[email protected]>
> > Cc: Stephen Boyd <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Signed-off-by: Dong Aisheng <[email protected]>
> > ---
> > drivers/cpufreq/cpufreq.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 9bf97a3..da07de6 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -180,6 +180,8 @@ int cpufreq_generic_init(struct cpufreq_policy *policy,
> > return ret;
> > }
> >
> > + if (!transition_latency)
> > + transition_latency = CPUFREQ_ETERNAL;
> > policy->cpuinfo.transition_latency = transition_latency;
> >
> > /*
>
> Can you update all the existing drivers as well (in the same patch)
> who can benefit from it?

Yes, of course.
Will do it in the updated version later.

Thanks

Regards
Dong Aisheng

>
> --
> viresh

2017-09-20 14:45:43

[permalink] [raw]

Subject: Re: [PATCH 6/7] cpufreq: make cpufreq_generic_init transition_latency default to CPUFREQ_ETERNAL

On 20-09-17, 15:04, Dong Aisheng wrote:
> On Tue, Sep 19, 2017 at 04:10:07PM -0700, Viresh Kumar wrote:
> > On 24-08-17, 00:10, Dong Aisheng wrote:
> > > If no valid transition_latency specified, let's make it default to
> > > CPUFREQ_ETERNAL which is consistent with its definition.
> > >
> > > This can save some of the same checkings like this:
> > > transition_latency = dev_pm_opp_get_max_transition_latency(cpu_dev);
> > > - if (!transition_latency)
> > > - transition_latency = CPUFREQ_ETERNAL;
> > > ret = cpufreq_generic_init(policy, freq_table, transition_latency);
> > >
> > > Cc: Viresh Kumar <[email protected]>
> > > Cc: Nishanth Menon <[email protected]>
> > > Cc: Stephen Boyd <[email protected]>
> > > Cc: "Rafael J. Wysocki" <[email protected]>
> > > Signed-off-by: Dong Aisheng <[email protected]>
> > > ---
> > > drivers/cpufreq/cpufreq.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index 9bf97a3..da07de6 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -180,6 +180,8 @@ int cpufreq_generic_init(struct cpufreq_policy *policy,
> > > return ret;
> > > }
> > >
> > > + if (!transition_latency)
> > > + transition_latency = CPUFREQ_ETERNAL;
> > > policy->cpuinfo.transition_latency = transition_latency;
> > >
> > > /*
> >
> > Can you update all the existing drivers as well (in the same patch)
> > who can benefit from it?
>
> Yes, of course.
> Will do it in the updated version later.

Perhaps you can send it separately if you want as your series is
surely going to take some time to get merged.

--
viresh

2017-09-20 20:30:38

[permalink] [raw]

Subject: Re: [PATCH 1/7] PM / OPP: Add platform specific set_clk function

On 20-09-17, 15:03, Dong Aisheng wrote:
> I've been thinking of that before.
> Actually IMX already does some similar thing for MX5 (no for MX6).
> See: clk_cpu_set_rate() in drivers/clk/imx/clk-cpu.c.
>
> After some diggings, it seems MX7ULP is a bit more complicated than before
> mainly due to two reasons:
> 1) It requires to switch to different CPU mode accordingly when setting
> clocks rate. That means we need handle this in clock driver as well
> which looks not quite suitable although we could do if really want.
>
> 2) It uses different clocks for different CPU mode (RUN 416M or
> HSRUN 528M), and those clocks have some dependency.
> e.g. when setting HSRUN clock, we need change RUN clock parent to make sure
> the SPLL_PFD is got disabled before changing rate, as both CPU mode using
> the same parent SPLL_PFD clock. Doing this in clock driver also make things
> a bit more complicated.
>
> The whole follow would be something like below:
> static int imx7ulp_set_clk(struct device *dev, struct clk *clk,
> unsigned long old_freq, unsigned long new_freq)
> {
> u32 val;
>
> /*
> * Before changing the ARM core PLL, change the ARM clock soure
> * to FIRC first.
> */
> if (new_freq >= HSRUN_FREQ) {
> clk_set_parent(clks[RUN_SCS_SEL].clk, clks[FIRC].clk);
>
> /* switch to HSRUN mode */
> val = readl_relaxed(smc_base + SMC_PMCTRL);
> val |= (0x3 << 8);
> writel_relaxed(val, smc_base + SMC_PMCTRL);
>
> /* change the clock rate in HSRUN */
> clk_set_rate(clks[SPLL_PFD0].clk, new_freq);
> clk_set_parent(clks[HSRUN_SCS_SEL].clk, clks[SPLL_SEL].clk);
> } else {
> /* change the HSRUN clock to firc */
> clk_set_parent(clks[HSRUN_SCS_SEL].clk, clks[FIRC].clk);
>
> /* switch to RUN mode */
> val = readl_relaxed(smc_base + SMC_PMCTRL);
> val &= ~(0x3 << 8);
> writel_relaxed(val, smc_base + SMC_PMCTRL);
>
> clk_set_rate(clks[SPLL_PFD0].clk, new_freq);
> clk_set_parent(clks[RUN_SCS_SEL].clk, clks[SPLL_SEL].clk);
> }
>
> return 0;
> }

Right and we have the same thing in the cpufreq driver now. It will
stay at some place and we need to find the best one, keeping in mind
that we may or may not want to solve this problem in a generic way.

> That's why i thought if we can make OPP core provide a way to handle such
> complicated things in platform specific cpufreq driver.
>
> How would you suggest for this issue?

I wouldn't add an API into the OPP framework if I were you. There is
just too much code to add to the core to handle such platform specific
stuff, which you are anyway going to keep somewhere as it is. IMHO,
keeping that in the clock driver is a better thing to do than this.

--
viresh

2017-09-21 02:17:17