* [PATCH] PM / Domains: Align in-parameter names for some genpd functions
From: Ulf Hansson @ 2019-07-16 9:43 UTC (permalink / raw)
To: Rafael J . Wysocki, linux-pm; +Cc: Ulf Hansson, Viresh Kumar, Kevin Hilman
Align in-parameter names for the declarations of pm_genpd_add|
remove_subdomain() and of_genpd_add_subdomain() according to their
implementations, as to improve consistency.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
include/linux/pm_domain.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 91d9bf497071..baf02ff91a31 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -197,9 +197,9 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev);
int pm_genpd_remove_device(struct device *dev);
int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
- struct generic_pm_domain *new_subdomain);
+ struct generic_pm_domain *subdomain);
int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
- struct generic_pm_domain *target);
+ struct generic_pm_domain *subdomain);
int pm_genpd_init(struct generic_pm_domain *genpd,
struct dev_power_governor *gov, bool is_off);
int pm_genpd_remove(struct generic_pm_domain *genpd);
@@ -226,12 +226,12 @@ static inline int pm_genpd_remove_device(struct device *dev)
return -ENOSYS;
}
static inline int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
- struct generic_pm_domain *new_sd)
+ struct generic_pm_domain *subdomain)
{
return -ENOSYS;
}
static inline int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
- struct generic_pm_domain *target)
+ struct generic_pm_domain *subdomain)
{
return -ENOSYS;
}
@@ -282,8 +282,8 @@ int of_genpd_add_provider_onecell(struct device_node *np,
struct genpd_onecell_data *data);
void of_genpd_del_provider(struct device_node *np);
int of_genpd_add_device(struct of_phandle_args *args, struct device *dev);
-int of_genpd_add_subdomain(struct of_phandle_args *parent,
- struct of_phandle_args *new_subdomain);
+int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,
+ struct of_phandle_args *subdomain_spec);
struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
int of_genpd_parse_idle_states(struct device_node *dn,
struct genpd_power_state **states, int *n);
@@ -316,8 +316,8 @@ static inline int of_genpd_add_device(struct of_phandle_args *args,
return -ENODEV;
}
-static inline int of_genpd_add_subdomain(struct of_phandle_args *parent,
- struct of_phandle_args *new_subdomain)
+static inline int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,
+ struct of_phandle_args *subdomain_spec)
{
return -ENODEV;
}
--
2.17.1
^ permalink raw reply related
* [PATCH 00/10] cpufreq: Migrate users of policy notifiers to QoS requests
From: Viresh Kumar @ 2019-07-16 9:48 UTC (permalink / raw)
To: Rafael Wysocki, Amit Daniel Kachhap, Benjamin Herrenschmidt,
Daniel Lezcano, Eduardo Valentin, Erik Schmauss,
Greg Kroah-Hartman, Javi Merino, Len Brown, Rafael J. Wysocki,
Robert Moore, Zhang Rui
Cc: Viresh Kumar, linux-pm, Vincent Guittot, devel, dri-devel,
linux-acpi, linux-doc, linux-fbdev, linux-kernel, linuxppc-dev
Hello,
Now that cpufreq core supports taking QoS requests for min/max cpu
frequencies, lets migrate rest of the users to using them instead of the
policy notifiers.
The CPUFREQ_NOTIFY and CPUFREQ_ADJUST events of the policy notifiers are
removed as a result, but we have to add CPUFREQ_CREATE_POLICY and
CPUFREQ_REMOVE_POLICY events to it for the acpi stuff specifically. So
the policy notifiers aren't completely removed.
Boot tested on my x86 PC and ARM hikey board. Nothing looked broken :)
This has already gone through build bot for a few days now.
--
viresh
Viresh Kumar (10):
cpufreq: Add policy create/remove notifiers
video: sa1100fb: Remove cpufreq policy notifier
video: pxafb: Remove cpufreq policy notifier
arch_topology: Use CPUFREQ_CREATE_POLICY instead of CPUFREQ_NOTIFY
thermal: cpu_cooling: Switch to QoS requests instead of cpufreq
notifier
powerpc: macintosh: Switch to QoS requests instead of cpufreq notifier
cpufreq: powerpc_cbe: Switch to QoS requests instead of cpufreq
notifier
ACPI: cpufreq: Switch to QoS requests instead of cpufreq notifier
cpufreq: Remove CPUFREQ_ADJUST and CPUFREQ_NOTIFY policy notifier
events
Documentation: cpufreq: Update policy notifier documentation
Documentation/cpu-freq/core.txt | 16 +--
drivers/acpi/processor_driver.c | 44 ++++++++-
drivers/acpi/processor_perflib.c | 106 +++++++++-----------
drivers/acpi/processor_thermal.c | 81 ++++++++-------
drivers/base/arch_topology.c | 2 +-
drivers/cpufreq/cpufreq.c | 51 ++++------
drivers/cpufreq/ppc_cbe_cpufreq.c | 19 +++-
drivers/cpufreq/ppc_cbe_cpufreq.h | 8 ++
drivers/cpufreq/ppc_cbe_cpufreq_pmi.c | 96 +++++++++++-------
drivers/macintosh/windfarm_cpufreq_clamp.c | 77 ++++++++++-----
drivers/thermal/cpu_cooling.c | 110 +++++----------------
drivers/video/fbdev/pxafb.c | 21 ----
drivers/video/fbdev/pxafb.h | 1 -
drivers/video/fbdev/sa1100fb.c | 27 -----
drivers/video/fbdev/sa1100fb.h | 1 -
include/acpi/processor.h | 22 +++--
include/linux/cpufreq.h | 4 +-
17 files changed, 327 insertions(+), 359 deletions(-)
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply
* [PATCH 01/10] cpufreq: Add policy create/remove notifiers
From: Viresh Kumar @ 2019-07-16 9:48 UTC (permalink / raw)
To: Rafael Wysocki; +Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel
In-Reply-To: <cover.1563269894.git.viresh.kumar@linaro.org>
This reverts commit f9f41e3ef99ac9d4e91b07634362e393fb929aad.
We have a new use case for policy create/remove notifiers (for
allocating/freeing QoS requests per policy), lets add them back.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 15 ++++++++++++++-
include/linux/cpufreq.h | 2 ++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 4d6043ee7834..8a7eff2a3771 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1268,7 +1268,17 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
DEV_PM_QOS_MAX_FREQUENCY);
dev_pm_qos_remove_notifier(dev, &policy->nb_min,
DEV_PM_QOS_MIN_FREQUENCY);
- dev_pm_qos_remove_request(policy->max_freq_req);
+
+ if (policy->max_freq_req) {
+ /*
+ * CPUFREQ_CREATE_POLICY notification is sent only after
+ * successfully adding max_freq_req request.
+ */
+ blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
+ CPUFREQ_REMOVE_POLICY, policy);
+ dev_pm_qos_remove_request(policy->max_freq_req);
+ }
+
dev_pm_qos_remove_request(policy->min_freq_req);
kfree(policy->min_freq_req);
@@ -1393,6 +1403,9 @@ static int cpufreq_online(unsigned int cpu)
ret);
goto out_destroy_policy;
}
+
+ blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
+ CPUFREQ_CREATE_POLICY, policy);
}
if (cpufreq_driver->get && has_target()) {
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d757a56a74dc..e28c8af697d2 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -458,6 +458,8 @@ static inline void cpufreq_resume(void) {}
/* Policy Notifiers */
#define CPUFREQ_ADJUST (0)
#define CPUFREQ_NOTIFY (1)
+#define CPUFREQ_CREATE_POLICY (2)
+#define CPUFREQ_REMOVE_POLICY (3)
#ifdef CONFIG_CPU_FREQ
int cpufreq_register_notifier(struct notifier_block *nb, unsigned int list);
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH 04/10] arch_topology: Use CPUFREQ_CREATE_POLICY instead of CPUFREQ_NOTIFY
From: Viresh Kumar @ 2019-07-16 9:54 UTC (permalink / raw)
To: Rafael Wysocki, Greg Kroah-Hartman, Rafael J. Wysocki
Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel
In-Reply-To: <cover.1563270828.git.viresh.kumar@linaro.org>
CPUFREQ_NOTIFY is going to get removed soon, lets use
CPUFREQ_CREATE_POLICY instead of that here. CPUFREQ_CREATE_POLICY is
called only once (which is exactly what we want here) for each cpufreq
policy when it is first created.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/base/arch_topology.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 9b09e31ae82f..49f2884fe77f 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -175,7 +175,7 @@ init_cpu_capacity_callback(struct notifier_block *nb,
if (!raw_capacity)
return 0;
- if (val != CPUFREQ_NOTIFY)
+ if (val != CPUFREQ_CREATE_POLICY)
return 0;
pr_debug("cpu_capacity: init cpu capacity for CPUs [%*pbl] (to_visit=%*pbl)\n",
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH 07/10] cpufreq: powerpc_cbe: Switch to QoS requests instead of cpufreq notifier
From: Viresh Kumar @ 2019-07-16 9:54 UTC (permalink / raw)
To: Rafael Wysocki, Viresh Kumar; +Cc: linux-pm, Vincent Guittot, linux-kernel
In-Reply-To: <cover.1563270828.git.viresh.kumar@linaro.org>
The cpufreq core now takes the min/max frequency constraints via QoS
requests and the CPUFREQ_ADJUST notifier shall get removed later on.
Switch over to using the QoS request for maximum frequency constraint
for ppc_cbe_cpufreq driver.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/ppc_cbe_cpufreq.c | 19 +++++-
drivers/cpufreq/ppc_cbe_cpufreq.h | 8 +++
drivers/cpufreq/ppc_cbe_cpufreq_pmi.c | 96 +++++++++++++++++----------
3 files changed, 86 insertions(+), 37 deletions(-)
diff --git a/drivers/cpufreq/ppc_cbe_cpufreq.c b/drivers/cpufreq/ppc_cbe_cpufreq.c
index b83f36febf03..c58abb4cca3a 100644
--- a/drivers/cpufreq/ppc_cbe_cpufreq.c
+++ b/drivers/cpufreq/ppc_cbe_cpufreq.c
@@ -110,6 +110,13 @@ static int cbe_cpufreq_cpu_init(struct cpufreq_policy *policy)
#endif
policy->freq_table = cbe_freqs;
+ cbe_cpufreq_pmi_policy_init(policy);
+ return 0;
+}
+
+static int cbe_cpufreq_cpu_exit(struct cpufreq_policy *policy)
+{
+ cbe_cpufreq_pmi_policy_exit(policy);
return 0;
}
@@ -129,6 +136,7 @@ static struct cpufreq_driver cbe_cpufreq_driver = {
.verify = cpufreq_generic_frequency_table_verify,
.target_index = cbe_cpufreq_target,
.init = cbe_cpufreq_cpu_init,
+ .exit = cbe_cpufreq_cpu_exit,
.name = "cbe-cpufreq",
.flags = CPUFREQ_CONST_LOOPS,
};
@@ -139,15 +147,24 @@ static struct cpufreq_driver cbe_cpufreq_driver = {
static int __init cbe_cpufreq_init(void)
{
+ int ret;
+
if (!machine_is(cell))
return -ENODEV;
- return cpufreq_register_driver(&cbe_cpufreq_driver);
+ cbe_cpufreq_pmi_init();
+
+ ret = cpufreq_register_driver(&cbe_cpufreq_driver);
+ if (ret)
+ cbe_cpufreq_pmi_exit();
+
+ return ret;
}
static void __exit cbe_cpufreq_exit(void)
{
cpufreq_unregister_driver(&cbe_cpufreq_driver);
+ cbe_cpufreq_pmi_exit();
}
module_init(cbe_cpufreq_init);
diff --git a/drivers/cpufreq/ppc_cbe_cpufreq.h b/drivers/cpufreq/ppc_cbe_cpufreq.h
index 9d973519d669..00cd8633b0d9 100644
--- a/drivers/cpufreq/ppc_cbe_cpufreq.h
+++ b/drivers/cpufreq/ppc_cbe_cpufreq.h
@@ -20,6 +20,14 @@ int cbe_cpufreq_set_pmode_pmi(int cpu, unsigned int pmode);
#if IS_ENABLED(CONFIG_CPU_FREQ_CBE_PMI)
extern bool cbe_cpufreq_has_pmi;
+void cbe_cpufreq_pmi_policy_init(struct cpufreq_policy *policy);
+void cbe_cpufreq_pmi_policy_exit(struct cpufreq_policy *policy);
+void cbe_cpufreq_pmi_init(void);
+void cbe_cpufreq_pmi_exit(void);
#else
#define cbe_cpufreq_has_pmi (0)
+static inline void cbe_cpufreq_pmi_policy_init(struct cpufreq_policy *policy) {}
+static inline void cbe_cpufreq_pmi_policy_exit(struct cpufreq_policy *policy) {}
+static inline void cbe_cpufreq_pmi_init(void) {}
+static inline void cbe_cpufreq_pmi_exit(void) {}
#endif
diff --git a/drivers/cpufreq/ppc_cbe_cpufreq_pmi.c b/drivers/cpufreq/ppc_cbe_cpufreq_pmi.c
index 97c8ee4614b7..0babb27c1c72 100644
--- a/drivers/cpufreq/ppc_cbe_cpufreq_pmi.c
+++ b/drivers/cpufreq/ppc_cbe_cpufreq_pmi.c
@@ -12,6 +12,7 @@
#include <linux/timer.h>
#include <linux/init.h>
#include <linux/of_platform.h>
+#include <linux/pm_qos.h>
#include <asm/processor.h>
#include <asm/prom.h>
@@ -24,8 +25,6 @@
#include "ppc_cbe_cpufreq.h"
-static u8 pmi_slow_mode_limit[MAX_CBE];
-
bool cbe_cpufreq_has_pmi = false;
EXPORT_SYMBOL_GPL(cbe_cpufreq_has_pmi);
@@ -65,64 +64,89 @@ EXPORT_SYMBOL_GPL(cbe_cpufreq_set_pmode_pmi);
static void cbe_cpufreq_handle_pmi(pmi_message_t pmi_msg)
{
+ struct cpufreq_policy *policy;
+ struct dev_pm_qos_request *req;
u8 node, slow_mode;
+ int cpu, ret;
BUG_ON(pmi_msg.type != PMI_TYPE_FREQ_CHANGE);
node = pmi_msg.data1;
slow_mode = pmi_msg.data2;
- pmi_slow_mode_limit[node] = slow_mode;
+ cpu = cbe_node_to_cpu(node);
pr_debug("cbe_handle_pmi: node: %d max_freq: %d\n", node, slow_mode);
-}
-
-static int pmi_notifier(struct notifier_block *nb,
- unsigned long event, void *data)
-{
- struct cpufreq_policy *policy = data;
- struct cpufreq_frequency_table *cbe_freqs = policy->freq_table;
- u8 node;
-
- /* Should this really be called for CPUFREQ_ADJUST and CPUFREQ_NOTIFY
- * policy events?)
- */
- node = cbe_cpu_to_node(policy->cpu);
-
- pr_debug("got notified, event=%lu, node=%u\n", event, node);
- if (pmi_slow_mode_limit[node] != 0) {
- pr_debug("limiting node %d to slow mode %d\n",
- node, pmi_slow_mode_limit[node]);
+ policy = cpufreq_cpu_get(cpu);
+ if (!policy) {
+ pr_warn("cpufreq policy not found cpu%d\n", cpu);
+ return;
+ }
- cpufreq_verify_within_limits(policy, 0,
+ req = policy->driver_data;
- cbe_freqs[pmi_slow_mode_limit[node]].frequency);
- }
+ ret = dev_pm_qos_update_request(req,
+ policy->freq_table[slow_mode].frequency);
+ if (ret)
+ pr_warn("Failed to update freq constraint: %d\n", ret);
+ else
+ pr_debug("limiting node %d to slow mode %d\n", node, slow_mode);
- return 0;
+ cpufreq_cpu_put(policy);
}
-static struct notifier_block pmi_notifier_block = {
- .notifier_call = pmi_notifier,
-};
-
static struct pmi_handler cbe_pmi_handler = {
.type = PMI_TYPE_FREQ_CHANGE,
.handle_pmi_message = cbe_cpufreq_handle_pmi,
};
+void cbe_cpufreq_pmi_policy_init(struct cpufreq_policy *policy)
+{
+ struct dev_pm_qos_request *req;
+ int ret;
+
+ if (!cbe_cpufreq_has_pmi)
+ return;
+
+ req = kzalloc(sizeof(*req), GFP_KERNEL);
+ if (!req)
+ return;
+
+ ret = dev_pm_qos_add_request(get_cpu_device(policy->cpu), req,
+ DEV_PM_QOS_MAX_FREQUENCY,
+ policy->freq_table[0].frequency);
+ if (ret < 0) {
+ pr_err("Failed to add freq constraint (%d)\n", ret);
+ kfree(req);
+ return;
+ }
+ policy->driver_data = req;
+}
+EXPORT_SYMBOL_GPL(cbe_cpufreq_pmi_policy_init);
-static int __init cbe_cpufreq_pmi_init(void)
+void cbe_cpufreq_pmi_policy_exit(struct cpufreq_policy *policy)
{
- cbe_cpufreq_has_pmi = pmi_register_handler(&cbe_pmi_handler) == 0;
+ struct dev_pm_qos_request *req = policy->driver_data;
- if (!cbe_cpufreq_has_pmi)
- return -ENODEV;
+ if (cbe_cpufreq_has_pmi) {
+ dev_pm_qos_remove_request(req);
+ kfree(req);
+ }
+}
+EXPORT_SYMBOL_GPL(cbe_cpufreq_pmi_policy_exit);
- cpufreq_register_notifier(&pmi_notifier_block, CPUFREQ_POLICY_NOTIFIER);
+void cbe_cpufreq_pmi_init(void)
+{
+ if (!pmi_register_handler(&cbe_pmi_handler))
+ cbe_cpufreq_has_pmi = true;
+}
+EXPORT_SYMBOL_GPL(cbe_cpufreq_pmi_init);
- return 0;
+void cbe_cpufreq_pmi_exit(void)
+{
+ pmi_unregister_handler(&cbe_pmi_handler);
+ cbe_cpufreq_has_pmi = false;
}
-device_initcall(cbe_cpufreq_pmi_init);
+EXPORT_SYMBOL_GPL(cbe_cpufreq_pmi_exit);
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH 09/10] cpufreq: Remove CPUFREQ_ADJUST and CPUFREQ_NOTIFY policy notifier events
From: Viresh Kumar @ 2019-07-16 9:54 UTC (permalink / raw)
To: Rafael Wysocki, Viresh Kumar; +Cc: linux-pm, Vincent Guittot, linux-kernel
In-Reply-To: <cover.1563270828.git.viresh.kumar@linaro.org>
No driver makes reference to these events now, remove them and the code
related to them.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 36 +++++++-----------------------------
include/linux/cpufreq.h | 6 ++----
2 files changed, 9 insertions(+), 33 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 8a7eff2a3771..bd91091ed6b8 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2362,15 +2362,13 @@ EXPORT_SYMBOL(cpufreq_get_policy);
* @policy: Policy object to modify.
* @new_policy: New policy data.
*
- * Pass @new_policy to the cpufreq driver's ->verify() callback, run the
- * installed policy notifiers for it with the CPUFREQ_ADJUST value, pass it to
- * the driver's ->verify() callback again and run the notifiers for it again
- * with the CPUFREQ_NOTIFY value. Next, copy the min and max parameters
- * of @new_policy to @policy and either invoke the driver's ->setpolicy()
- * callback (if present) or carry out a governor update for @policy. That is,
- * run the current governor's ->limits() callback (if the governor field in
- * @new_policy points to the same object as the one in @policy) or replace the
- * governor for @policy with the new one stored in @new_policy.
+ * Pass @new_policy to the cpufreq driver's ->verify() callback. Next, copy the
+ * min and max parameters of @new_policy to @policy and either invoke the
+ * driver's ->setpolicy() callback (if present) or carry out a governor update
+ * for @policy. That is, run the current governor's ->limits() callback (if the
+ * governor field in @new_policy points to the same object as the one in
+ * @policy) or replace the governor for @policy with the new one stored in
+ * @new_policy.
*
* The cpuinfo part of @policy is not updated by this function.
*/
@@ -2398,26 +2396,6 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
if (ret)
return ret;
- /*
- * The notifier-chain shall be removed once all the users of
- * CPUFREQ_ADJUST are moved to use the QoS framework.
- */
- /* adjust if necessary - all reasons */
- blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
- CPUFREQ_ADJUST, new_policy);
-
- /*
- * verify the cpu speed can be set within this limit, which might be
- * different to the first one
- */
- ret = cpufreq_driver->verify(new_policy);
- if (ret)
- return ret;
-
- /* notification of the new policy */
- blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
- CPUFREQ_NOTIFY, new_policy);
-
policy->min = new_policy->min;
policy->max = new_policy->max;
trace_cpu_frequency_limits(policy);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index e28c8af697d2..cb1e86a14c7a 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -456,10 +456,8 @@ static inline void cpufreq_resume(void) {}
#define CPUFREQ_POSTCHANGE (1)
/* Policy Notifiers */
-#define CPUFREQ_ADJUST (0)
-#define CPUFREQ_NOTIFY (1)
-#define CPUFREQ_CREATE_POLICY (2)
-#define CPUFREQ_REMOVE_POLICY (3)
+#define CPUFREQ_CREATE_POLICY (0)
+#define CPUFREQ_REMOVE_POLICY (1)
#ifdef CONFIG_CPU_FREQ
int cpufreq_register_notifier(struct notifier_block *nb, unsigned int list);
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH 10/10] Documentation: cpufreq: Update policy notifier documentation
From: Viresh Kumar @ 2019-07-16 9:54 UTC (permalink / raw)
To: Rafael Wysocki, Viresh Kumar
Cc: linux-pm, Vincent Guittot, linux-doc, linux-kernel
In-Reply-To: <cover.1563270828.git.viresh.kumar@linaro.org>
Update documentation with the recent policy notifier updates.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Documentation/cpu-freq/core.txt | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/Documentation/cpu-freq/core.txt b/Documentation/cpu-freq/core.txt
index 073f128af5a7..af5c0d60b466 100644
--- a/Documentation/cpu-freq/core.txt
+++ b/Documentation/cpu-freq/core.txt
@@ -57,19 +57,11 @@ transition notifiers.
2.1 CPUFreq policy notifiers
----------------------------
-These are notified when a new policy is intended to be set. Each
-CPUFreq policy notifier is called twice for a policy transition:
+These are notified when a new policy is created or removed.
-1.) During CPUFREQ_ADJUST all CPUFreq notifiers may change the limit if
- they see a need for this - may it be thermal considerations or
- hardware limitations.
-
-2.) And during CPUFREQ_NOTIFY all notifiers are informed of the new policy
- - if two hardware drivers failed to agree on a new policy before this
- stage, the incompatible hardware shall be shut down, and the user
- informed of this.
-
-The phase is specified in the second argument to the notifier.
+The phase is specified in the second argument to the notifier. The phase is
+CPUFREQ_CREATE_POLICY when the policy is first created and it is
+CPUFREQ_REMOVE_POLICY when the policy is removed.
The third argument, a void *pointer, points to a struct cpufreq_policy
consisting of several values, including min, max (the lower and upper
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH 08/10] ACPI: cpufreq: Switch to QoS requests instead of cpufreq notifier
From: Viresh Kumar @ 2019-07-16 9:54 UTC (permalink / raw)
To: Rafael Wysocki, Len Brown, Zhang Rui, Robert Moore, Erik Schmauss
Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-acpi, linux-kernel,
devel
In-Reply-To: <cover.1563270828.git.viresh.kumar@linaro.org>
The cpufreq core now takes the min/max frequency constraints via QoS
requests and the CPUFREQ_ADJUST notifier shall get removed later on.
Switch over to using the QoS request for maximum frequency constraint
for acpi driver.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/acpi/processor_driver.c | 44 +++++++++++--
drivers/acpi/processor_perflib.c | 106 +++++++++++++------------------
drivers/acpi/processor_thermal.c | 81 ++++++++++++-----------
include/acpi/processor.h | 22 ++++---
4 files changed, 137 insertions(+), 116 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index aea8d674a33d..e7a3f07e9879 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -284,6 +284,35 @@ static int acpi_processor_stop(struct device *dev)
return 0;
}
+bool acpi_processor_cpufreq_init;
+
+static int acpi_processor_notifier(struct notifier_block *nb,
+ unsigned long event, void *data)
+{
+ struct cpufreq_policy *policy = data;
+ int cpu;
+
+ if (event == CPUFREQ_CREATE_POLICY) {
+ for_each_cpu(cpu, policy->cpus)
+ per_cpu(processors, cpu)->policy = policy;
+
+ acpi_thermal_cpufreq_init(policy);
+ acpi_processor_ppc_init(policy);
+ } else if (event == CPUFREQ_REMOVE_POLICY) {
+ acpi_processor_ppc_exit(policy);
+ acpi_thermal_cpufreq_exit(policy);
+
+ for_each_cpu(cpu, policy->cpus)
+ per_cpu(processors, cpu)->policy = NULL;
+ }
+
+ return 0;
+}
+
+static struct notifier_block acpi_processor_notifier_block = {
+ .notifier_call = acpi_processor_notifier,
+};
+
/*
* We keep the driver loaded even when ACPI is not running.
* This is needed for the powernow-k8 driver, that works even without
@@ -310,8 +339,11 @@ static int __init acpi_processor_driver_init(void)
cpuhp_setup_state_nocalls(CPUHP_ACPI_CPUDRV_DEAD, "acpi/cpu-drv:dead",
NULL, acpi_soft_cpu_dead);
- acpi_thermal_cpufreq_init();
- acpi_processor_ppc_init();
+ if (!cpufreq_register_notifier(&acpi_processor_notifier_block,
+ CPUFREQ_POLICY_NOTIFIER)) {
+ acpi_processor_cpufreq_init = true;
+ }
+
acpi_processor_throttling_init();
return 0;
err:
@@ -324,8 +356,12 @@ static void __exit acpi_processor_driver_exit(void)
if (acpi_disabled)
return;
- acpi_processor_ppc_exit();
- acpi_thermal_cpufreq_exit();
+ if (acpi_processor_cpufreq_init) {
+ cpufreq_unregister_notifier(&acpi_processor_notifier_block,
+ CPUFREQ_POLICY_NOTIFIER);
+ acpi_processor_cpufreq_init = false;
+ }
+
cpuhp_remove_state_nocalls(hp_online);
cpuhp_remove_state_nocalls(CPUHP_ACPI_CPUDRV_DEAD);
driver_unregister(&acpi_processor_driver);
diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
index ee87cb6f6e59..1a22b2415a8b 100644
--- a/drivers/acpi/processor_perflib.c
+++ b/drivers/acpi/processor_perflib.c
@@ -50,57 +50,13 @@ module_param(ignore_ppc, int, 0644);
MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \
"limited by BIOS, this should help");
-#define PPC_REGISTERED 1
-#define PPC_IN_USE 2
-
-static int acpi_processor_ppc_status;
-
-static int acpi_processor_ppc_notifier(struct notifier_block *nb,
- unsigned long event, void *data)
-{
- struct cpufreq_policy *policy = data;
- struct acpi_processor *pr;
- unsigned int ppc = 0;
-
- if (ignore_ppc < 0)
- ignore_ppc = 0;
-
- if (ignore_ppc)
- return 0;
-
- if (event != CPUFREQ_ADJUST)
- return 0;
-
- mutex_lock(&performance_mutex);
-
- pr = per_cpu(processors, policy->cpu);
- if (!pr || !pr->performance)
- goto out;
-
- ppc = (unsigned int)pr->performance_platform_limit;
-
- if (ppc >= pr->performance->state_count)
- goto out;
-
- cpufreq_verify_within_limits(policy, 0,
- pr->performance->states[ppc].
- core_frequency * 1000);
-
- out:
- mutex_unlock(&performance_mutex);
-
- return 0;
-}
-
-static struct notifier_block acpi_ppc_notifier_block = {
- .notifier_call = acpi_processor_ppc_notifier,
-};
+static int acpi_processor_ppc_in_use;
static int acpi_processor_get_platform_limit(struct acpi_processor *pr)
{
acpi_status status = 0;
unsigned long long ppc = 0;
-
+ int ret;
if (!pr)
return -EINVAL;
@@ -112,7 +68,7 @@ static int acpi_processor_get_platform_limit(struct acpi_processor *pr)
status = acpi_evaluate_integer(pr->handle, "_PPC", NULL, &ppc);
if (status != AE_NOT_FOUND)
- acpi_processor_ppc_status |= PPC_IN_USE;
+ acpi_processor_ppc_in_use = 1;
if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
ACPI_EXCEPTION((AE_INFO, status, "Evaluating _PPC"));
@@ -124,6 +80,16 @@ static int acpi_processor_get_platform_limit(struct acpi_processor *pr)
pr->performance_platform_limit = (int)ppc;
+ if (ignore_ppc || ppc >= pr->performance->state_count)
+ return 0;
+
+ ret = dev_pm_qos_update_request(pr->perflib_req,
+ pr->performance->states[ppc].core_frequency * 1000);
+ if (ret) {
+ pr_warn("Failed to update perflib freq constraint: cpu%d (%d)\n",
+ pr->id, ret);
+ }
+
return 0;
}
@@ -184,23 +150,39 @@ int acpi_processor_get_bios_limit(int cpu, unsigned int *limit)
}
EXPORT_SYMBOL(acpi_processor_get_bios_limit);
-void acpi_processor_ppc_init(void)
+void acpi_processor_ppc_init(struct cpufreq_policy *policy)
{
- if (!cpufreq_register_notifier
- (&acpi_ppc_notifier_block, CPUFREQ_POLICY_NOTIFIER))
- acpi_processor_ppc_status |= PPC_REGISTERED;
- else
- printk(KERN_DEBUG
- "Warning: Processor Platform Limit not supported.\n");
+ struct acpi_processor *pr = per_cpu(processors, policy->cpu);
+ struct dev_pm_qos_request *req;
+ int ret;
+
+ req = kzalloc(sizeof(*req), GFP_KERNEL);
+ if (!req)
+ return;
+
+ ret = dev_pm_qos_add_request(get_cpu_device(policy->cpu),
+ req, DEV_PM_QOS_MAX_FREQUENCY,
+ policy->cpuinfo.max_freq);
+ if (ret < 0) {
+ pr_err("Failed to add freq constraint for cpu%d (%d)\n",
+ policy->cpu, ret);
+ kfree(req);
+ return;
+ }
+
+ pr->perflib_req = req;
+
+ if (ignore_ppc < 0)
+ ignore_ppc = 0;
}
-void acpi_processor_ppc_exit(void)
+void acpi_processor_ppc_exit(struct cpufreq_policy *policy)
{
- if (acpi_processor_ppc_status & PPC_REGISTERED)
- cpufreq_unregister_notifier(&acpi_ppc_notifier_block,
- CPUFREQ_POLICY_NOTIFIER);
+ struct acpi_processor *pr = per_cpu(processors, policy->cpu);
- acpi_processor_ppc_status &= ~PPC_REGISTERED;
+ dev_pm_qos_remove_request(pr->perflib_req);
+ kfree(pr->perflib_req);
+ pr->perflib_req = NULL;
}
static int acpi_processor_get_performance_control(struct acpi_processor *pr)
@@ -477,7 +459,7 @@ int acpi_processor_notify_smm(struct module *calling_module)
static int is_done = 0;
int result;
- if (!(acpi_processor_ppc_status & PPC_REGISTERED))
+ if (!acpi_processor_cpufreq_init)
return -EBUSY;
if (!try_module_get(calling_module))
@@ -513,7 +495,7 @@ int acpi_processor_notify_smm(struct module *calling_module)
* we can allow the cpufreq driver to be rmmod'ed. */
is_done = 1;
- if (!(acpi_processor_ppc_status & PPC_IN_USE))
+ if (!acpi_processor_ppc_in_use)
module_put(calling_module);
return 0;
@@ -742,7 +724,7 @@ acpi_processor_register_performance(struct acpi_processor_performance
{
struct acpi_processor *pr;
- if (!(acpi_processor_ppc_status & PPC_REGISTERED))
+ if (!acpi_processor_cpufreq_init)
return -EINVAL;
mutex_lock(&performance_mutex);
diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
index 50fb0107375e..02407b33b874 100644
--- a/drivers/acpi/processor_thermal.c
+++ b/drivers/acpi/processor_thermal.c
@@ -35,7 +35,6 @@ ACPI_MODULE_NAME("processor_thermal");
#define CPUFREQ_THERMAL_MAX_STEP 3
static DEFINE_PER_CPU(unsigned int, cpufreq_thermal_reduction_pctg);
-static unsigned int acpi_thermal_cpufreq_is_init = 0;
#define reduction_pctg(cpu) \
per_cpu(cpufreq_thermal_reduction_pctg, phys_package_first_cpu(cpu))
@@ -61,35 +60,11 @@ static int phys_package_first_cpu(int cpu)
static int cpu_has_cpufreq(unsigned int cpu)
{
struct cpufreq_policy policy;
- if (!acpi_thermal_cpufreq_is_init || cpufreq_get_policy(&policy, cpu))
+ if (!acpi_processor_cpufreq_init || cpufreq_get_policy(&policy, cpu))
return 0;
return 1;
}
-static int acpi_thermal_cpufreq_notifier(struct notifier_block *nb,
- unsigned long event, void *data)
-{
- struct cpufreq_policy *policy = data;
- unsigned long max_freq = 0;
-
- if (event != CPUFREQ_ADJUST)
- goto out;
-
- max_freq = (
- policy->cpuinfo.max_freq *
- (100 - reduction_pctg(policy->cpu) * 20)
- ) / 100;
-
- cpufreq_verify_within_limits(policy, 0, max_freq);
-
- out:
- return 0;
-}
-
-static struct notifier_block acpi_thermal_cpufreq_notifier_block = {
- .notifier_call = acpi_thermal_cpufreq_notifier,
-};
-
static int cpufreq_get_max_state(unsigned int cpu)
{
if (!cpu_has_cpufreq(cpu))
@@ -108,7 +83,9 @@ static int cpufreq_get_cur_state(unsigned int cpu)
static int cpufreq_set_cur_state(unsigned int cpu, int state)
{
- int i;
+ struct acpi_processor *pr;
+ unsigned long max_freq;
+ int i, ret;
if (!cpu_has_cpufreq(cpu))
return 0;
@@ -121,33 +98,53 @@ static int cpufreq_set_cur_state(unsigned int cpu, int state)
* frequency.
*/
for_each_online_cpu(i) {
- if (topology_physical_package_id(i) ==
+ if (topology_physical_package_id(i) !=
topology_physical_package_id(cpu))
- cpufreq_update_policy(i);
+ continue;
+
+ pr = per_cpu(processors, i);
+ max_freq = (pr->policy->cpuinfo.max_freq * (100 - reduction_pctg(i) * 20)) / 100;
+
+ ret = dev_pm_qos_update_request(pr->thermal_req, max_freq);
+ if (ret) {
+ pr_warn("Failed to update thermal freq constraint: cpu%d (%d)\n",
+ pr->id, ret);
+ }
}
return 0;
}
-void acpi_thermal_cpufreq_init(void)
+void acpi_thermal_cpufreq_init(struct cpufreq_policy *policy)
{
- int i;
+ struct acpi_processor *pr = per_cpu(processors, policy->cpu);
+ struct dev_pm_qos_request *req;
+ int ret;
+
+ req = kzalloc(sizeof(*req), GFP_KERNEL);
+ if (!req)
+ return;
+
+ ret = dev_pm_qos_add_request(get_cpu_device(policy->cpu),
+ req, DEV_PM_QOS_MAX_FREQUENCY,
+ policy->cpuinfo.max_freq);
+ if (ret < 0) {
+ pr_err("Failed to add freq constraint for cpu%d (%d)\n",
+ policy->cpu, ret);
+ kfree(req);
+ return;
+ }
- i = cpufreq_register_notifier(&acpi_thermal_cpufreq_notifier_block,
- CPUFREQ_POLICY_NOTIFIER);
- if (!i)
- acpi_thermal_cpufreq_is_init = 1;
+ pr->thermal_req = req;
}
-void acpi_thermal_cpufreq_exit(void)
+void acpi_thermal_cpufreq_exit(struct cpufreq_policy *policy)
{
- if (acpi_thermal_cpufreq_is_init)
- cpufreq_unregister_notifier
- (&acpi_thermal_cpufreq_notifier_block,
- CPUFREQ_POLICY_NOTIFIER);
+ struct acpi_processor *pr = per_cpu(processors, policy->cpu);
- acpi_thermal_cpufreq_is_init = 0;
+ dev_pm_qos_remove_request(pr->thermal_req);
+ kfree(pr->thermal_req);
+ pr->thermal_req = NULL;
}
-
#else /* ! CONFIG_CPU_FREQ */
static int cpufreq_get_max_state(unsigned int cpu)
{
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 1194a4c78d55..a1a7966bb755 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -4,6 +4,8 @@
#include <linux/kernel.h>
#include <linux/cpu.h>
+#include <linux/cpufreq.h>
+#include <linux/pm_qos.h>
#include <linux/thermal.h>
#include <asm/acpi.h>
@@ -230,6 +232,9 @@ struct acpi_processor {
struct acpi_processor_limit limit;
struct thermal_cooling_device *cdev;
struct device *dev; /* Processor device. */
+ struct cpufreq_policy *policy;
+ struct dev_pm_qos_request *perflib_req;
+ struct dev_pm_qos_request *thermal_req;
};
struct acpi_processor_errata {
@@ -296,16 +301,17 @@ static inline void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx
/* in processor_perflib.c */
#ifdef CONFIG_CPU_FREQ
-void acpi_processor_ppc_init(void);
-void acpi_processor_ppc_exit(void);
+extern bool acpi_processor_cpufreq_init;
+void acpi_processor_ppc_init(struct cpufreq_policy *policy);
+void acpi_processor_ppc_exit(struct cpufreq_policy *policy);
void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag);
extern int acpi_processor_get_bios_limit(int cpu, unsigned int *limit);
#else
-static inline void acpi_processor_ppc_init(void)
+static inline void acpi_processor_ppc_init(struct cpufreq_policy *policy)
{
return;
}
-static inline void acpi_processor_ppc_exit(void)
+static inline void acpi_processor_ppc_exit(struct cpufreq_policy *policy)
{
return;
}
@@ -421,14 +427,14 @@ static inline int acpi_processor_hotplug(struct acpi_processor *pr)
int acpi_processor_get_limit_info(struct acpi_processor *pr);
extern const struct thermal_cooling_device_ops processor_cooling_ops;
#if defined(CONFIG_ACPI_CPU_FREQ_PSS) & defined(CONFIG_CPU_FREQ)
-void acpi_thermal_cpufreq_init(void);
-void acpi_thermal_cpufreq_exit(void);
+void acpi_thermal_cpufreq_init(struct cpufreq_policy *policy);
+void acpi_thermal_cpufreq_exit(struct cpufreq_policy *policy);
#else
-static inline void acpi_thermal_cpufreq_init(void)
+static inline void acpi_thermal_cpufreq_init(struct cpufreq_policy *policy)
{
return;
}
-static inline void acpi_thermal_cpufreq_exit(void)
+static inline void acpi_thermal_cpufreq_exit(struct cpufreq_policy *policy)
{
return;
}
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH 06/10] powerpc: macintosh: Switch to QoS requests instead of cpufreq notifier
From: Viresh Kumar @ 2019-07-16 9:54 UTC (permalink / raw)
To: Rafael Wysocki, Benjamin Herrenschmidt
Cc: Viresh Kumar, linux-pm, Vincent Guittot, linuxppc-dev,
linux-kernel
In-Reply-To: <cover.1563270828.git.viresh.kumar@linaro.org>
The cpufreq core now takes the min/max frequency constraints via QoS
requests and the CPUFREQ_ADJUST notifier shall get removed later on.
Switch over to using the QoS request for maximum frequency constraint
for windfarm_cpufreq_clamp driver.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/macintosh/windfarm_cpufreq_clamp.c | 77 ++++++++++++++--------
1 file changed, 50 insertions(+), 27 deletions(-)
diff --git a/drivers/macintosh/windfarm_cpufreq_clamp.c b/drivers/macintosh/windfarm_cpufreq_clamp.c
index 52fd5fca89a0..705c6200814b 100644
--- a/drivers/macintosh/windfarm_cpufreq_clamp.c
+++ b/drivers/macintosh/windfarm_cpufreq_clamp.c
@@ -3,9 +3,11 @@
#include <linux/errno.h>
#include <linux/kernel.h>
#include <linux/delay.h>
+#include <linux/pm_qos.h>
#include <linux/slab.h>
#include <linux/init.h>
#include <linux/wait.h>
+#include <linux/cpu.h>
#include <linux/cpufreq.h>
#include <asm/prom.h>
@@ -16,36 +18,24 @@
static int clamped;
static struct wf_control *clamp_control;
-
-static int clamp_notifier_call(struct notifier_block *self,
- unsigned long event, void *data)
-{
- struct cpufreq_policy *p = data;
- unsigned long max_freq;
-
- if (event != CPUFREQ_ADJUST)
- return 0;
-
- max_freq = clamped ? (p->cpuinfo.min_freq) : (p->cpuinfo.max_freq);
- cpufreq_verify_within_limits(p, 0, max_freq);
-
- return 0;
-}
-
-static struct notifier_block clamp_notifier = {
- .notifier_call = clamp_notifier_call,
-};
+static struct dev_pm_qos_request qos_req;
+static unsigned int min_freq, max_freq;
static int clamp_set(struct wf_control *ct, s32 value)
{
- if (value)
+ unsigned int freq;
+
+ if (value) {
+ freq = min_freq;
printk(KERN_INFO "windfarm: Clamping CPU frequency to "
"minimum !\n");
- else
+ } else {
+ freq = max_freq;
printk(KERN_INFO "windfarm: CPU frequency unclamped !\n");
+ }
clamped = value;
- cpufreq_update_policy(0);
- return 0;
+
+ return dev_pm_qos_update_request(&qos_req, freq);
}
static int clamp_get(struct wf_control *ct, s32 *value)
@@ -74,27 +64,60 @@ static const struct wf_control_ops clamp_ops = {
static int __init wf_cpufreq_clamp_init(void)
{
+ struct cpufreq_policy *policy;
struct wf_control *clamp;
+ struct device *dev;
+ int ret;
+
+ policy = cpufreq_cpu_get(0);
+ if (!policy) {
+ pr_warn("%s: cpufreq policy not found cpu0\n", __func__);
+ return -EPROBE_DEFER;
+ }
+
+ min_freq = policy->cpuinfo.min_freq;
+ max_freq = policy->cpuinfo.max_freq;
+ cpufreq_cpu_put(policy);
+
+ dev = get_cpu_device(0);
+ if (unlikely(!dev)) {
+ pr_warn("%s: No cpu device for cpu0\n", __func__);
+ return -ENODEV;
+ }
clamp = kmalloc(sizeof(struct wf_control), GFP_KERNEL);
if (clamp == NULL)
return -ENOMEM;
- cpufreq_register_notifier(&clamp_notifier, CPUFREQ_POLICY_NOTIFIER);
+
+ ret = dev_pm_qos_add_request(dev, &qos_req, DEV_PM_QOS_MAX_FREQUENCY,
+ max_freq);
+ if (ret < 0) {
+ pr_err("%s: Failed to add freq constraint (%d)\n", __func__,
+ ret);
+ goto free;
+ }
+
clamp->ops = &clamp_ops;
clamp->name = "cpufreq-clamp";
- if (wf_register_control(clamp))
+ ret = wf_register_control(clamp);
+ if (ret)
goto fail;
clamp_control = clamp;
return 0;
fail:
+ dev_pm_qos_remove_request(&qos_req);
+
+ free:
kfree(clamp);
- return -ENODEV;
+ return ret;
}
static void __exit wf_cpufreq_clamp_exit(void)
{
- if (clamp_control)
+ if (clamp_control) {
wf_unregister_control(clamp_control);
+ dev_pm_qos_remove_request(&qos_req);
+ }
}
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH 05/10] thermal: cpu_cooling: Switch to QoS requests instead of cpufreq notifier
From: Viresh Kumar @ 2019-07-16 9:54 UTC (permalink / raw)
To: Rafael Wysocki, Amit Daniel Kachhap, Viresh Kumar, Javi Merino,
Zhang Rui, Eduardo Valentin, Daniel Lezcano
Cc: linux-pm, Vincent Guittot, linux-kernel
In-Reply-To: <cover.1563270828.git.viresh.kumar@linaro.org>
The cpufreq core now takes the min/max frequency constraints via QoS
requests and the CPUFREQ_ADJUST notifier shall get removed later on.
Switch over to using the QoS request for maximum frequency constraint
for cpu_cooling driver.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/thermal/cpu_cooling.c | 110 ++++++++--------------------------
1 file changed, 26 insertions(+), 84 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 4c5db59a619b..391f39776c6a 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -16,6 +16,7 @@
#include <linux/err.h>
#include <linux/idr.h>
#include <linux/pm_opp.h>
+#include <linux/pm_qos.h>
#include <linux/slab.h>
#include <linux/cpu.h>
#include <linux/cpu_cooling.h>
@@ -66,8 +67,6 @@ struct time_in_idle {
* @last_load: load measured by the latest call to cpufreq_get_requested_power()
* @cpufreq_state: integer value representing the current state of cpufreq
* cooling devices.
- * @clipped_freq: integer value representing the absolute value of the clipped
- * frequency.
* @max_level: maximum cooling level. One less than total number of valid
* cpufreq frequencies.
* @freq_table: Freq table in descending order of frequencies
@@ -84,12 +83,12 @@ struct cpufreq_cooling_device {
int id;
u32 last_load;
unsigned int cpufreq_state;
- unsigned int clipped_freq;
unsigned int max_level;
struct freq_table *freq_table; /* In descending order */
struct cpufreq_policy *policy;
struct list_head node;
struct time_in_idle *idle_time;
+ struct dev_pm_qos_request qos_req;
};
static DEFINE_IDA(cpufreq_ida);
@@ -118,59 +117,6 @@ static unsigned long get_level(struct cpufreq_cooling_device *cpufreq_cdev,
return level - 1;
}
-/**
- * cpufreq_thermal_notifier - notifier callback for cpufreq policy change.
- * @nb: struct notifier_block * with callback info.
- * @event: value showing cpufreq event for which this function invoked.
- * @data: callback-specific data
- *
- * Callback to hijack the notification on cpufreq policy transition.
- * Every time there is a change in policy, we will intercept and
- * update the cpufreq policy with thermal constraints.
- *
- * Return: 0 (success)
- */
-static int cpufreq_thermal_notifier(struct notifier_block *nb,
- unsigned long event, void *data)
-{
- struct cpufreq_policy *policy = data;
- unsigned long clipped_freq;
- struct cpufreq_cooling_device *cpufreq_cdev;
-
- if (event != CPUFREQ_ADJUST)
- return NOTIFY_DONE;
-
- mutex_lock(&cooling_list_lock);
- list_for_each_entry(cpufreq_cdev, &cpufreq_cdev_list, node) {
- /*
- * A new copy of the policy is sent to the notifier and can't
- * compare that directly.
- */
- if (policy->cpu != cpufreq_cdev->policy->cpu)
- continue;
-
- /*
- * policy->max is the maximum allowed frequency defined by user
- * and clipped_freq is the maximum that thermal constraints
- * allow.
- *
- * If clipped_freq is lower than policy->max, then we need to
- * readjust policy->max.
- *
- * But, if clipped_freq is greater than policy->max, we don't
- * need to do anything.
- */
- clipped_freq = cpufreq_cdev->clipped_freq;
-
- if (policy->max > clipped_freq)
- cpufreq_verify_within_limits(policy, 0, clipped_freq);
- break;
- }
- mutex_unlock(&cooling_list_lock);
-
- return NOTIFY_OK;
-}
-
/**
* update_freq_table() - Update the freq table with power numbers
* @cpufreq_cdev: the cpufreq cooling device in which to update the table
@@ -374,7 +320,6 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
unsigned long state)
{
struct cpufreq_cooling_device *cpufreq_cdev = cdev->devdata;
- unsigned int clip_freq;
/* Request state should be less than max_level */
if (WARN_ON(state > cpufreq_cdev->max_level))
@@ -384,13 +329,10 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
if (cpufreq_cdev->cpufreq_state == state)
return 0;
- clip_freq = cpufreq_cdev->freq_table[state].frequency;
cpufreq_cdev->cpufreq_state = state;
- cpufreq_cdev->clipped_freq = clip_freq;
-
- cpufreq_update_policy(cpufreq_cdev->policy->cpu);
- return 0;
+ return dev_pm_qos_update_request(&cpufreq_cdev->qos_req,
+ cpufreq_cdev->freq_table[state].frequency);
}
/**
@@ -554,11 +496,6 @@ static struct thermal_cooling_device_ops cpufreq_power_cooling_ops = {
.power2state = cpufreq_power2state,
};
-/* Notifier for cpufreq policy change */
-static struct notifier_block thermal_cpufreq_notifier_block = {
- .notifier_call = cpufreq_thermal_notifier,
-};
-
static unsigned int find_next_max(struct cpufreq_frequency_table *table,
unsigned int prev_max)
{
@@ -596,9 +533,16 @@ __cpufreq_cooling_register(struct device_node *np,
struct cpufreq_cooling_device *cpufreq_cdev;
char dev_name[THERMAL_NAME_LENGTH];
unsigned int freq, i, num_cpus;
+ struct device *dev;
int ret;
struct thermal_cooling_device_ops *cooling_ops;
- bool first;
+
+ dev = get_cpu_device(policy->cpu);
+ if (unlikely(!dev)) {
+ pr_warn("No cpu device for cpu %d\n", policy->cpu);
+ return ERR_PTR(-ENODEV);
+ }
+
if (IS_ERR_OR_NULL(policy)) {
pr_err("%s: cpufreq policy isn't valid: %p\n", __func__, policy);
@@ -671,25 +615,29 @@ __cpufreq_cooling_register(struct device_node *np,
cooling_ops = &cpufreq_cooling_ops;
}
+ ret = dev_pm_qos_add_request(dev, &cpufreq_cdev->qos_req,
+ DEV_PM_QOS_MAX_FREQUENCY,
+ cpufreq_cdev->freq_table[0].frequency);
+ if (ret < 0) {
+ pr_err("%s: Failed to add freq constraint (%d)\n", __func__,
+ ret);
+ cdev = ERR_PTR(ret);
+ goto remove_ida;
+ }
+
cdev = thermal_of_cooling_device_register(np, dev_name, cpufreq_cdev,
cooling_ops);
if (IS_ERR(cdev))
- goto remove_ida;
-
- cpufreq_cdev->clipped_freq = cpufreq_cdev->freq_table[0].frequency;
+ goto remove_qos_req;
mutex_lock(&cooling_list_lock);
- /* Register the notifier for first cpufreq cooling device */
- first = list_empty(&cpufreq_cdev_list);
list_add(&cpufreq_cdev->node, &cpufreq_cdev_list);
mutex_unlock(&cooling_list_lock);
- if (first)
- cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
- CPUFREQ_POLICY_NOTIFIER);
-
return cdev;
+remove_qos_req:
+ dev_pm_qos_remove_request(&cpufreq_cdev->qos_req);
remove_ida:
ida_simple_remove(&cpufreq_ida, cpufreq_cdev->id);
free_table:
@@ -777,7 +725,6 @@ EXPORT_SYMBOL_GPL(of_cpufreq_cooling_register);
void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
{
struct cpufreq_cooling_device *cpufreq_cdev;
- bool last;
if (!cdev)
return;
@@ -786,15 +733,10 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
mutex_lock(&cooling_list_lock);
list_del(&cpufreq_cdev->node);
- /* Unregister the notifier for the last cpufreq cooling device */
- last = list_empty(&cpufreq_cdev_list);
mutex_unlock(&cooling_list_lock);
- if (last)
- cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
- CPUFREQ_POLICY_NOTIFIER);
-
thermal_cooling_device_unregister(cdev);
+ dev_pm_qos_remove_request(&cpufreq_cdev->qos_req);
ida_simple_remove(&cpufreq_ida, cpufreq_cdev->id);
kfree(cpufreq_cdev->idle_time);
kfree(cpufreq_cdev->freq_table);
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH 03/10] video: pxafb: Remove cpufreq policy notifier
From: Viresh Kumar @ 2019-07-16 9:54 UTC (permalink / raw)
To: Rafael Wysocki
Cc: Viresh Kumar, linux-pm, Vincent Guittot, dri-devel, linux-fbdev,
linux-kernel
In-Reply-To: <cover.1563270828.git.viresh.kumar@linaro.org>
The cpufreq policy notifier's CPUFREQ_ADJUST notification is going to
get removed soon.
The notifier callback pxafb_freq_policy() isn't doing anything apart
from printing a debug message on CPUFREQ_ADJUST notification. There is
no point in keeping an otherwise empty callback and registering the
notifier.
Remove it.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/video/fbdev/pxafb.c | 21 ---------------------
drivers/video/fbdev/pxafb.h | 1 -
2 files changed, 22 deletions(-)
diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
index 4282cb117b92..f70c9f79622e 100644
--- a/drivers/video/fbdev/pxafb.c
+++ b/drivers/video/fbdev/pxafb.c
@@ -1678,24 +1678,6 @@ pxafb_freq_transition(struct notifier_block *nb, unsigned long val, void *data)
}
return 0;
}
-
-static int
-pxafb_freq_policy(struct notifier_block *nb, unsigned long val, void *data)
-{
- struct pxafb_info *fbi = TO_INF(nb, freq_policy);
- struct fb_var_screeninfo *var = &fbi->fb.var;
- struct cpufreq_policy *policy = data;
-
- switch (val) {
- case CPUFREQ_ADJUST:
- pr_debug("min dma period: %d ps, "
- "new clock %d kHz\n", pxafb_display_dma_period(var),
- policy->max);
- /* TODO: fill in min/max values */
- break;
- }
- return 0;
-}
#endif
#ifdef CONFIG_PM
@@ -2400,11 +2382,8 @@ static int pxafb_probe(struct platform_device *dev)
#ifdef CONFIG_CPU_FREQ
fbi->freq_transition.notifier_call = pxafb_freq_transition;
- fbi->freq_policy.notifier_call = pxafb_freq_policy;
cpufreq_register_notifier(&fbi->freq_transition,
CPUFREQ_TRANSITION_NOTIFIER);
- cpufreq_register_notifier(&fbi->freq_policy,
- CPUFREQ_POLICY_NOTIFIER);
#endif
/*
diff --git a/drivers/video/fbdev/pxafb.h b/drivers/video/fbdev/pxafb.h
index b641289c8a99..86b1e9ab1a38 100644
--- a/drivers/video/fbdev/pxafb.h
+++ b/drivers/video/fbdev/pxafb.h
@@ -162,7 +162,6 @@ struct pxafb_info {
#ifdef CONFIG_CPU_FREQ
struct notifier_block freq_transition;
- struct notifier_block freq_policy;
#endif
struct regulator *lcd_supply;
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH 02/10] video: sa1100fb: Remove cpufreq policy notifier
From: Viresh Kumar @ 2019-07-16 9:54 UTC (permalink / raw)
To: Rafael Wysocki
Cc: Viresh Kumar, linux-pm, Vincent Guittot, dri-devel, linux-fbdev,
linux-kernel
In-Reply-To: <cover.1563270828.git.viresh.kumar@linaro.org>
The cpufreq policy notifier's CPUFREQ_ADJUST notification is going to
get removed soon.
The notifier callback sa1100fb_freq_policy() isn't doing anything apart
from printing a debug message on CPUFREQ_ADJUST notification. There is
no point in keeping an otherwise empty callback and registering the
notifier.
Remove it.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/video/fbdev/sa1100fb.c | 27 ---------------------------
drivers/video/fbdev/sa1100fb.h | 1 -
2 files changed, 28 deletions(-)
diff --git a/drivers/video/fbdev/sa1100fb.c b/drivers/video/fbdev/sa1100fb.c
index f7f8dee044b1..ae2bcfee338a 100644
--- a/drivers/video/fbdev/sa1100fb.c
+++ b/drivers/video/fbdev/sa1100fb.c
@@ -1005,31 +1005,6 @@ sa1100fb_freq_transition(struct notifier_block *nb, unsigned long val,
}
return 0;
}
-
-static int
-sa1100fb_freq_policy(struct notifier_block *nb, unsigned long val,
- void *data)
-{
- struct sa1100fb_info *fbi = TO_INF(nb, freq_policy);
- struct cpufreq_policy *policy = data;
-
- switch (val) {
- case CPUFREQ_ADJUST:
- dev_dbg(fbi->dev, "min dma period: %d ps, "
- "new clock %d kHz\n", sa1100fb_min_dma_period(fbi),
- policy->max);
- /* todo: fill in min/max values */
- break;
- case CPUFREQ_NOTIFY:
- do {} while(0);
- /* todo: panic if min/max values aren't fulfilled
- * [can't really happen unless there's a bug in the
- * CPU policy verififcation process *
- */
- break;
- }
- return 0;
-}
#endif
#ifdef CONFIG_PM
@@ -1242,9 +1217,7 @@ static int sa1100fb_probe(struct platform_device *pdev)
#ifdef CONFIG_CPU_FREQ
fbi->freq_transition.notifier_call = sa1100fb_freq_transition;
- fbi->freq_policy.notifier_call = sa1100fb_freq_policy;
cpufreq_register_notifier(&fbi->freq_transition, CPUFREQ_TRANSITION_NOTIFIER);
- cpufreq_register_notifier(&fbi->freq_policy, CPUFREQ_POLICY_NOTIFIER);
#endif
/* This driver cannot be unloaded at the moment */
diff --git a/drivers/video/fbdev/sa1100fb.h b/drivers/video/fbdev/sa1100fb.h
index 7a1a9ca33cec..d0aa33b0b88a 100644
--- a/drivers/video/fbdev/sa1100fb.h
+++ b/drivers/video/fbdev/sa1100fb.h
@@ -64,7 +64,6 @@ struct sa1100fb_info {
#ifdef CONFIG_CPU_FREQ
struct notifier_block freq_transition;
- struct notifier_block freq_policy;
#endif
const struct sa1100fb_mach_info *inf;
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* Re: [PATCH v2 1/4] opp: core: add regulators enable and disable
From: Viresh Kumar @ 2019-07-16 10:05 UTC (permalink / raw)
To: Kamil Konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Chanwoo Choi,
Krzysztof Kozlowski, Kukjin Kim, Kyungmin Park, Mark Rutland,
MyungJoo Ham, Nishanth Menon, Rob Herring, Stephen Boyd,
Viresh Kumar, devicetree, linux-arm-kernel, linux-kernel,
linux-pm, linux-samsung-soc
In-Reply-To: <20190715120416.3561-2-k.konieczny@partner.samsung.com>
On 15-07-19, 14:04, Kamil Konieczny wrote:
> Add enable regulators to dev_pm_opp_set_regulators() and disable
> regulators to dev_pm_opp_put_regulators(). This prepares for
> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
>
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> --
> Changes in v2:
>
> - move regulator enable and disable into loop
>
> ---
> drivers/opp/core.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0e7703fe733f..069c5cf8827e 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
> goto free_regulators;
> }
>
> + ret = regulator_enable(reg);
> + if (ret < 0)
> + goto disable;
The name of this label is logically incorrect because we won't disable
the regulator from there but put it. Over that, I would rather prefer
to remove the label and add regulator_put() here itself.
> +
> opp_table->regulators[i] = reg;
> }
>
> @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>
> return opp_table;
>
> +disable:
> + regulator_put(reg);
> + --i;
> +
> free_regulators:
> - while (i != 0)
> - regulator_put(opp_table->regulators[--i]);
> + for (; i >= 0; --i) {
> + regulator_disable(opp_table->regulators[i]);
> + regulator_put(opp_table->regulators[i]);
This is incorrect as this will now try to put/disable the regulator
which we failed to acquire. As --i happens only after the loop has run
once. You can rather do:
while (i--) {
regulator_disable(opp_table->regulators[i]);
regulator_put(opp_table->regulators[i]);
}
> + }
>
> kfree(opp_table->regulators);
> opp_table->regulators = NULL;
> @@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
> /* Make sure there are no concurrent readers while updating opp_table */
> WARN_ON(!list_empty(&opp_table->opp_list));
>
> - for (i = opp_table->regulator_count - 1; i >= 0; i--)
> + for (i = opp_table->regulator_count - 1; i >= 0; i--) {
> + regulator_disable(opp_table->regulators[i]);
> regulator_put(opp_table->regulators[i]);
> + }
>
> _free_set_opp_data(opp_table);
>
> --
> 2.22.0
--
viresh
^ permalink raw reply
* Re: [PATCH 00/10] cpufreq: Migrate users of policy notifiers to QoS requests
From: Rafael J. Wysocki @ 2019-07-16 10:06 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, Amit Daniel Kachhap, Benjamin Herrenschmidt,
Daniel Lezcano, Eduardo Valentin, Erik Schmauss,
Greg Kroah-Hartman, Javi Merino, Len Brown, Rafael J. Wysocki,
Robert Moore, Zhang Rui, Linux PM, Vincent Guittot,
open list:ACPI COMPONENT ARCHITECTURE (ACPICA), dri-devel,
ACPI Devel Maling List, open list:DOCUMENTATION,
open list:FRAMEBUFFER LAYER, Linux Kernel Mailing List,
linuxppc-dev
In-Reply-To: <cover.1563269894.git.viresh.kumar@linaro.org>
On Tue, Jul 16, 2019 at 11:49 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Hello,
>
> Now that cpufreq core supports taking QoS requests for min/max cpu
> frequencies, lets migrate rest of the users to using them instead of the
> policy notifiers.
Technically, this still is linux-next only. :-)
> The CPUFREQ_NOTIFY and CPUFREQ_ADJUST events of the policy notifiers are
> removed as a result, but we have to add CPUFREQ_CREATE_POLICY and
> CPUFREQ_REMOVE_POLICY events to it for the acpi stuff specifically. So
> the policy notifiers aren't completely removed.
That's not entirely accurate, because arch_topology is going to use
CPUFREQ_CREATE_POLICY now too.
> Boot tested on my x86 PC and ARM hikey board. Nothing looked broken :)
>
> This has already gone through build bot for a few days now.
So I'd prefer patches [5-8] to go right after the first one and then
do the cleanups on top of that, as somebody may want to backport the
essential changes without the cleanups.
^ permalink raw reply
* Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
From: Bartlomiej Zolnierkiewicz @ 2019-07-16 10:13 UTC (permalink / raw)
To: Chanwoo Choi
Cc: Kamil Konieczny, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <7f7cf551-005a-c647-d571-77eb5426478a@samsung.com>
Hi Chanwoo,
On 7/16/19 5:56 AM, Chanwoo Choi wrote:
> Hi Kamil,
>
> Looks good to me. But, this patch has some issue.
> I added the detailed reviews.
>
> I recommend that you make the separate patches as following
> in order to clarify the role of which apply the dev_pm_opp_* function.
>
> First patch,
> Need to consolidate the following two function into one function.
> because the original exynos-bus.c has the problem that the regulator
> of parent devfreq device have to be enabled before enabling the clock.
> This issue did not happen because bootloader enables the bus-related
> regulators before kernel booting.
> - exynos_bus_parse_of()
> - exynos_bus_parent_parse_of()
> > Second patch,
> Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()
>
>
> On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
>> Reuse opp core code for setting bus clock and voltage. As a side
>> effect this allow useage of coupled regulators feature (required
>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>> uses regulator_set_voltage_triplet() for setting regulator voltage
>> while the old code used regulator_set_voltage_tol() with fixed
>> tolerance. This patch also removes no longer needed parsing of DT
>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>> it).
>>
>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>> ---
>> drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
>> 1 file changed, 66 insertions(+), 106 deletions(-)
>>
>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>> index 486cc5b422f1..7fc4f76bd848 100644
>> --- a/drivers/devfreq/exynos-bus.c
>> +++ b/drivers/devfreq/exynos-bus.c
>> @@ -25,7 +25,6 @@
>> #include <linux/slab.h>
>>
>> #define DEFAULT_SATURATION_RATIO 40
>> -#define DEFAULT_VOLTAGE_TOLERANCE 2
>>
>> struct exynos_bus {
>> struct device *dev;
>> @@ -37,9 +36,9 @@ struct exynos_bus {
>>
>> unsigned long curr_freq;
>>
>> - struct regulator *regulator;
>> + struct opp_table *opp_table;
>> +
>> struct clk *clk;
>> - unsigned int voltage_tolerance;
>> unsigned int ratio;
>> };
>>
>> @@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>> {
>> struct exynos_bus *bus = dev_get_drvdata(dev);
>> struct dev_pm_opp *new_opp;
>> - unsigned long old_freq, new_freq, new_volt, tol;
>> int ret = 0;
>> -
>> - /* Get new opp-bus instance according to new bus clock */
>> + /*
>> + * New frequency for bus may not be exactly matched to opp, adjust
>> + * *freq to correct value.
>> + */
>
> You better to change this comment with following styles
> to keep the consistency:
>
> /* Get correct frequency for bus ... */
>
>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>> if (IS_ERR(new_opp)) {
>> dev_err(dev, "failed to get recommended opp instance\n");
>> return PTR_ERR(new_opp);
>> }
>>
>> - new_freq = dev_pm_opp_get_freq(new_opp);
>> - new_volt = dev_pm_opp_get_voltage(new_opp);
>> dev_pm_opp_put(new_opp);
>>
>> - old_freq = bus->curr_freq;
>> -
>> - if (old_freq == new_freq)
>> - return 0;
>> - tol = new_volt * bus->voltage_tolerance / 100;
>> -
>> /* Change voltage and frequency according to new OPP level */
>> mutex_lock(&bus->lock);
>> + ret = dev_pm_opp_set_rate(dev, *freq);
>> + if (!ret)
>> + bus->curr_freq = *freq;
>
> Have to print the error log if ret has minus error value.
dev_pm_opp_set_rate() should print the error message on all
errors so wouldn't printing the error log also here be superfluous?
[ Please also note that the other user of dev_pm_opp_set_rate()
(cpufreq-dt cpufreq driver) doesn't do this. ]
> Modify it as following:
>
> if (ret < 0) {
> dev_err(dev, "failed to set bus rate\n");
> goto err:
> }
> bus->curr_freq = *freq;
>
> err:
> mutex_unlock(&bus->lock);
>
> return ret;
>
>>
>> - if (old_freq < new_freq) {
>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>> - if (ret < 0) {
>> - dev_err(bus->dev, "failed to set voltage\n");
>> - goto out;
>> - }
>> - }
>> -
>> - ret = clk_set_rate(bus->clk, new_freq);
>> - if (ret < 0) {
>> - dev_err(dev, "failed to change clock of bus\n");
>> - clk_set_rate(bus->clk, old_freq);
>> - goto out;
>> - }
>> -
>> - if (old_freq > new_freq) {
>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>> - if (ret < 0) {
>> - dev_err(bus->dev, "failed to set voltage\n");
>> - goto out;
>> - }
>> - }
>> - bus->curr_freq = new_freq;
>> -
>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>> - old_freq, new_freq, clk_get_rate(bus->clk));
>> -out:
>> mutex_unlock(&bus->lock);
>>
>> return ret;
>> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
>> if (ret < 0)
>> dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>
>> - if (bus->regulator)
>> - regulator_disable(bus->regulator);
>> + if (bus->opp_table)
>> + dev_pm_opp_put_regulators(bus->opp_table);
>
> Have to disable regulator after disabling the clock
> to prevent the h/w fault.
>
> I think that you should call them with following sequence:
>
> clk_disable_unprepare(bus->clk);
> if (bus->opp_table)
> dev_pm_opp_put_regulators(bus->opp_table);
> dev_pm_opp_of_remove_table(dev);
>
>>
>> dev_pm_opp_of_remove_table(dev);
>> +
>> clk_disable_unprepare(bus->clk);
>> }
>>
>> @@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>> {
>> struct exynos_bus *bus = dev_get_drvdata(dev);
>> struct dev_pm_opp *new_opp;
>> - unsigned long old_freq, new_freq;
>> - int ret = 0;
>> + int ret;
>>
>> - /* Get new opp-bus instance according to new bus clock */
>> + /*
>> + * New frequency for bus may not be exactly matched to opp, adjust
>> + * *freq to correct value.
>> + */
>
> You better to change this comment with following styles
> to keep the consistency:
>
> /* Get correct frequency for bus ... */
>
>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>> if (IS_ERR(new_opp)) {
>> dev_err(dev, "failed to get recommended opp instance\n");
>> return PTR_ERR(new_opp);
>> }
>>
>> - new_freq = dev_pm_opp_get_freq(new_opp);
>> dev_pm_opp_put(new_opp);
>>
>> - old_freq = bus->curr_freq;
>> -
>> - if (old_freq == new_freq)
>> - return 0;
>> -
>> /* Change the frequency according to new OPP level */
>> mutex_lock(&bus->lock);
>> + ret = dev_pm_opp_set_rate(dev, *freq);
>> + if (!ret)
>> + bus->curr_freq = *freq;
>
> ditto. Have to print the error log, check above comment.
>
>>
>> - ret = clk_set_rate(bus->clk, new_freq);
>> - if (ret < 0) {
>> - dev_err(dev, "failed to set the clock of bus\n");
>> - goto out;
>> - }
>> -
>> - *freq = new_freq;
>> - bus->curr_freq = new_freq;
>> -
>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>> - old_freq, new_freq, clk_get_rate(bus->clk));
>> -out:
>> mutex_unlock(&bus->lock);
>>
>> return ret;
>> @@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>> struct exynos_bus *bus)
>> {
>> struct device *dev = bus->dev;
>> - int i, ret, count, size;
>> -
>> - /* Get the regulator to provide each bus with the power */
>> - bus->regulator = devm_regulator_get(dev, "vdd");
>> - if (IS_ERR(bus->regulator)) {
>> - dev_err(dev, "failed to get VDD regulator\n");
>> - return PTR_ERR(bus->regulator);
>> - }
>> -
>> - ret = regulator_enable(bus->regulator);
>> - if (ret < 0) {
>> - dev_err(dev, "failed to enable VDD regulator\n");
>> - return ret;
>> - }
>> + int i, count, size;
>>
>> /*
>> * Get the devfreq-event devices to get the current utilization of
>> @@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>> count = devfreq_event_get_edev_count(dev);
>> if (count < 0) {
>> dev_err(dev, "failed to get the count of devfreq-event dev\n");
>> - ret = count;
>> - goto err_regulator;
>> + return count;
>> }
>> +
>> bus->edev_count = count;
>>
>> size = sizeof(*bus->edev) * count;
>> bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>> - if (!bus->edev) {
>> - ret = -ENOMEM;
>> - goto err_regulator;
>> - }
>> + if (!bus->edev)
>> + return -ENOMEM;
>>
>> for (i = 0; i < count; i++) {
>> bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
>> - if (IS_ERR(bus->edev[i])) {
>> - ret = -EPROBE_DEFER;
>> - goto err_regulator;
>> - }
>> + if (IS_ERR(bus->edev[i]))
>> + return -EPROBE_DEFER;
>> }
>>
>> /*
>> @@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>> if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>> bus->ratio = DEFAULT_SATURATION_RATIO;
>>
>> - if (of_property_read_u32(np, "exynos,voltage-tolerance",
>> - &bus->voltage_tolerance))
>> - bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
>> -
>> return 0;
>> -
>> -err_regulator:
>> - regulator_disable(bus->regulator);
>> -
>> - return ret;
>> }
>>
>> static int exynos_bus_parse_of(struct device_node *np,
>> - struct exynos_bus *bus)
>> + struct exynos_bus *bus, bool passive)
>> {
>> struct device *dev = bus->dev;
>> + struct opp_table *opp_table;
>> + const char *vdd = "vdd";
>> struct dev_pm_opp *opp;
>> unsigned long rate;
>> int ret;
>> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
>> return ret;
>> }
>>
>> + if (!passive) {
>> + opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>> + if (IS_ERR(opp_table)) {
>> + ret = PTR_ERR(opp_table);
>> + dev_err(dev, "failed to set regulators %d\n", ret);
>> + goto err_clk;/
>> + }
>> +
>> + bus->opp_table = opp_table;
>> + }
>
> This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
> dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
> because the regulator is only used by parent devfreq device.
exynos_bus_parse_of() is called for all devfreq devices (including
parent) and (as you've noticed) the regulator should be enabled before
enabling clock (which is done in exynos_bus_parse_of()) so adding
extra argument to exynos_bus_parse_of() (like it is done currently in
the patch) makes it possible to do the setup correctly without the need
of merging both functions into one huge function (which would be more
difficult to follow than two simpler functions IMHO). Is that approach
acceptable or do you prefer one big function?
>> +
>> /* Get the freq and voltage from OPP table to scale the bus freq */
>> ret = dev_pm_opp_of_add_table(dev);
>> if (ret < 0) {
>> dev_err(dev, "failed to get OPP table\n");
>> - goto err_clk;
>> + goto err_regulator;
>> }
>>
>> rate = clk_get_rate(bus->clk);
>> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>> ret = PTR_ERR(opp);
>> goto err_opp;
>> }
>> +
>> bus->curr_freq = dev_pm_opp_get_freq(opp);
>> dev_pm_opp_put(opp);
>>
>> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>>
>> err_opp:
>> dev_pm_opp_of_remove_table(dev);
>> +
>> +err_regulator:
>> + if (bus->opp_table) {
>> + dev_pm_opp_put_regulators(bus->opp_table);
>> + bus->opp_table = NULL;
>> + }
>
> As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
> after removing the opp_table by dev_pm_opp_of_remove_table().
>
>> +
>> err_clk:
>> clk_disable_unprepare(bus->clk);
>>
>> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>> struct exynos_bus *bus;
>> int ret, max_state;
>> unsigned long min_freq, max_freq;
>> + bool passive = false;
>>
>> if (!np) {
>> dev_err(dev, "failed to find devicetree node\n");
>> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>> bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>> if (!bus)
>> return -ENOMEM;
>> +
>> mutex_init(&bus->lock);
>> bus->dev = &pdev->dev;
>> platform_set_drvdata(pdev, bus);
>> + node = of_parse_phandle(dev->of_node, "devfreq", 0);
>> + if (node) {
>> + of_node_put(node);
>> + passive = true;
>> + }
>>
>> /* Parse the device-tree to get the resource information */
>> - ret = exynos_bus_parse_of(np, bus);
>> + ret = exynos_bus_parse_of(np, bus, passive);
>> if (ret < 0)
>> return ret;
>>
>> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>> goto err;
>> }
>>
>> - node = of_parse_phandle(dev->of_node, "devfreq", 0);
>> - if (node) {
>> - of_node_put(node);
>> + if (passive)
>> goto passive;
>> - } else {
>> - ret = exynos_bus_parent_parse_of(np, bus);
>> - }
>> +
>> + ret = exynos_bus_parent_parse_of(np, bus);
>>
>
> Remove unneeded blank line.
>
>> if (ret < 0)
>> goto err;
>> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>
>> err:
>> dev_pm_opp_of_remove_table(dev);
>> + if (bus->opp_table) {
>> + dev_pm_opp_put_regulators(bus->opp_table);
>> + bus->opp_table = NULL;
>> + }
>> +
>
> ditto.
> Have to disable regulator after disabling the clock
> to prevent the h/w fault.
>
> I think that you should call them with following sequence:
>
> clk_disable_unprepare(bus->clk);
> if (bus->opp_table)
> dev_pm_opp_put_regulators(bus->opp_table);
> dev_pm_opp_of_remove_table(dev);
>
>> clk_disable_unprepare(bus->clk);
>>
>> return ret;
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 00/10] cpufreq: Migrate users of policy notifiers to QoS requests
From: Viresh Kumar @ 2019-07-16 10:14 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael Wysocki, Amit Daniel Kachhap, Benjamin Herrenschmidt,
Daniel Lezcano, Eduardo Valentin, Erik Schmauss,
Greg Kroah-Hartman, Javi Merino, Len Brown, Robert Moore,
Zhang Rui, Linux PM, Vincent Guittot,
open list:ACPI COMPONENT ARCHITECTURE (ACPICA), dri-devel,
ACPI Devel Maling List, open list:DOCUMENTATION,
open list:FRAMEBUFFER LAYER, Linux Kernel Mailing List,
linuxppc-dev
In-Reply-To: <CAJZ5v0iqYHNt6NQy3Fi1B=XtjNOm2x0mX3+7eWBREgFZRpUS+w@mail.gmail.com>
On 16-07-19, 12:06, Rafael J. Wysocki wrote:
> On Tue, Jul 16, 2019 at 11:49 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > Hello,
> >
> > Now that cpufreq core supports taking QoS requests for min/max cpu
> > frequencies, lets migrate rest of the users to using them instead of the
> > policy notifiers.
>
> Technically, this still is linux-next only. :-)
True :)
> > The CPUFREQ_NOTIFY and CPUFREQ_ADJUST events of the policy notifiers are
> > removed as a result, but we have to add CPUFREQ_CREATE_POLICY and
> > CPUFREQ_REMOVE_POLICY events to it for the acpi stuff specifically. So
> > the policy notifiers aren't completely removed.
>
> That's not entirely accurate, because arch_topology is going to use
> CPUFREQ_CREATE_POLICY now too.
Yeah, I thought about that while writing this patchset and
coverletter. But had it not been required for ACPI, I would have done
it differently for the arch-topology code. Maybe direct calling of
arch-topology routine from cpufreq core. I wanted to get rid of the
policy notifiers completely but I couldn't find a better way of doing
it for ACPI stuff.
> > Boot tested on my x86 PC and ARM hikey board. Nothing looked broken :)
> >
> > This has already gone through build bot for a few days now.
>
> So I'd prefer patches [5-8] to go right after the first one and then
> do the cleanups on top of that, as somebody may want to backport the
> essential changes without the cleanups.
In the exceptional case where nobody finds anything wrong with the
patches (highly unlikely), do you want me to resend with reordering or
you can reorder them while applying? There are no dependencies between
those patches anyway.
--
viresh
^ permalink raw reply
* Re: [PATCH 00/10] cpufreq: Migrate users of policy notifiers to QoS requests
From: Rafael J. Wysocki @ 2019-07-16 10:27 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Rafael Wysocki, Amit Daniel Kachhap,
Benjamin Herrenschmidt, Daniel Lezcano, Eduardo Valentin,
Erik Schmauss, Greg Kroah-Hartman, Javi Merino, Len Brown,
Robert Moore, Zhang Rui, Linux PM, Vincent Guittot,
open list:ACPI COMPONENT ARCHITECTURE (ACPICA), dri-devel,
ACPI Devel Maling List, open list:DOCUMENTATION,
open list:FRAMEBUFFER LAYER, Linux Kernel Mailing List,
linuxppc-dev
In-Reply-To: <20190716101416.ntk353cfnrcykoek@vireshk-i7>
On Tue, Jul 16, 2019 at 12:14 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 16-07-19, 12:06, Rafael J. Wysocki wrote:
> > On Tue, Jul 16, 2019 at 11:49 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > Hello,
> > >
> > > Now that cpufreq core supports taking QoS requests for min/max cpu
> > > frequencies, lets migrate rest of the users to using them instead of the
> > > policy notifiers.
> >
> > Technically, this still is linux-next only. :-)
>
> True :)
>
> > > The CPUFREQ_NOTIFY and CPUFREQ_ADJUST events of the policy notifiers are
> > > removed as a result, but we have to add CPUFREQ_CREATE_POLICY and
> > > CPUFREQ_REMOVE_POLICY events to it for the acpi stuff specifically. So
> > > the policy notifiers aren't completely removed.
> >
> > That's not entirely accurate, because arch_topology is going to use
> > CPUFREQ_CREATE_POLICY now too.
>
> Yeah, I thought about that while writing this patchset and
> coverletter. But had it not been required for ACPI, I would have done
> it differently for the arch-topology code. Maybe direct calling of
> arch-topology routine from cpufreq core. I wanted to get rid of the
> policy notifiers completely but I couldn't find a better way of doing
> it for ACPI stuff.
>
> > > Boot tested on my x86 PC and ARM hikey board. Nothing looked broken :)
> > >
> > > This has already gone through build bot for a few days now.
> >
> > So I'd prefer patches [5-8] to go right after the first one and then
> > do the cleanups on top of that, as somebody may want to backport the
> > essential changes without the cleanups.
>
> In the exceptional case where nobody finds anything wrong with the
> patches (highly unlikely), do you want me to resend with reordering or
> you can reorder them while applying? There are no dependencies between
> those patches anyway.
Please resend the reordered set when the merge window closes.
^ permalink raw reply
* Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
From: Chanwoo Choi @ 2019-07-16 10:33 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Kamil Konieczny, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <3d1687b7-4825-ad82-2706-a712c30e530b@samsung.com>
Hi Bartlomiej,
On 19. 7. 16. 오후 7:13, Bartlomiej Zolnierkiewicz wrote:
>
> Hi Chanwoo,
>
> On 7/16/19 5:56 AM, Chanwoo Choi wrote:
>> Hi Kamil,
>>
>> Looks good to me. But, this patch has some issue.
>> I added the detailed reviews.
>>
>> I recommend that you make the separate patches as following
>> in order to clarify the role of which apply the dev_pm_opp_* function.
>>
>> First patch,
>> Need to consolidate the following two function into one function.
>> because the original exynos-bus.c has the problem that the regulator
>> of parent devfreq device have to be enabled before enabling the clock.
>> This issue did not happen because bootloader enables the bus-related
>> regulators before kernel booting.
>> - exynos_bus_parse_of()
>> - exynos_bus_parent_parse_of()
>>> Second patch,
>> Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()
>>
>>
>> On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
>>> Reuse opp core code for setting bus clock and voltage. As a side
>>> effect this allow useage of coupled regulators feature (required
>>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>>> uses regulator_set_voltage_triplet() for setting regulator voltage
>>> while the old code used regulator_set_voltage_tol() with fixed
>>> tolerance. This patch also removes no longer needed parsing of DT
>>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>>> it).
>>>
>>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>> ---
>>> drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
>>> 1 file changed, 66 insertions(+), 106 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>> index 486cc5b422f1..7fc4f76bd848 100644
>>> --- a/drivers/devfreq/exynos-bus.c
>>> +++ b/drivers/devfreq/exynos-bus.c
>>> @@ -25,7 +25,6 @@
>>> #include <linux/slab.h>
>>>
>>> #define DEFAULT_SATURATION_RATIO 40
>>> -#define DEFAULT_VOLTAGE_TOLERANCE 2
>>>
>>> struct exynos_bus {
>>> struct device *dev;
>>> @@ -37,9 +36,9 @@ struct exynos_bus {
>>>
>>> unsigned long curr_freq;
>>>
>>> - struct regulator *regulator;
>>> + struct opp_table *opp_table;
>>> +
>>> struct clk *clk;
>>> - unsigned int voltage_tolerance;
>>> unsigned int ratio;
>>> };
>>>
>>> @@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>>> {
>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>> struct dev_pm_opp *new_opp;
>>> - unsigned long old_freq, new_freq, new_volt, tol;
>>> int ret = 0;
>>> -
>>> - /* Get new opp-bus instance according to new bus clock */
>>> + /*
>>> + * New frequency for bus may not be exactly matched to opp, adjust
>>> + * *freq to correct value.
>>> + */
>>
>> You better to change this comment with following styles
>> to keep the consistency:
>>
>> /* Get correct frequency for bus ... */
>>
>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>> if (IS_ERR(new_opp)) {
>>> dev_err(dev, "failed to get recommended opp instance\n");
>>> return PTR_ERR(new_opp);
>>> }
>>>
>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>> - new_volt = dev_pm_opp_get_voltage(new_opp);
>>> dev_pm_opp_put(new_opp);
>>>
>>> - old_freq = bus->curr_freq;
>>> -
>>> - if (old_freq == new_freq)
>>> - return 0;
>>> - tol = new_volt * bus->voltage_tolerance / 100;
>>> -
>>> /* Change voltage and frequency according to new OPP level */
>>> mutex_lock(&bus->lock);
>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>> + if (!ret)
>>> + bus->curr_freq = *freq;
>>
>> Have to print the error log if ret has minus error value.
>
> dev_pm_opp_set_rate() should print the error message on all
> errors so wouldn't printing the error log also here be superfluous?
>
> [ Please also note that the other user of dev_pm_opp_set_rate()
> (cpufreq-dt cpufreq driver) doesn't do this. ]
OK. Thanks for the explanation.
>
>> Modify it as following:
>>
>> if (ret < 0) {
>> dev_err(dev, "failed to set bus rate\n");
>> goto err:
>> }
>> bus->curr_freq = *freq;
>>
>> err:
>> mutex_unlock(&bus->lock);
>>
>> return ret;
>>
>>>
>>> - if (old_freq < new_freq) {
>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>> - if (ret < 0) {
>>> - dev_err(bus->dev, "failed to set voltage\n");
>>> - goto out;
>>> - }
>>> - }
>>> -
>>> - ret = clk_set_rate(bus->clk, new_freq);
>>> - if (ret < 0) {
>>> - dev_err(dev, "failed to change clock of bus\n");
>>> - clk_set_rate(bus->clk, old_freq);
>>> - goto out;
>>> - }
>>> -
>>> - if (old_freq > new_freq) {
>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>> - if (ret < 0) {
>>> - dev_err(bus->dev, "failed to set voltage\n");
>>> - goto out;
>>> - }
>>> - }
>>> - bus->curr_freq = new_freq;
>>> -
>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>> -out:
>>> mutex_unlock(&bus->lock);
>>>
>>> return ret;
>>> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
>>> if (ret < 0)
>>> dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>>
>>> - if (bus->regulator)
>>> - regulator_disable(bus->regulator);
>>> + if (bus->opp_table)
>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>
>> Have to disable regulator after disabling the clock
>> to prevent the h/w fault.
>>
>> I think that you should call them with following sequence:
>>
>> clk_disable_unprepare(bus->clk);
>> if (bus->opp_table)
>> dev_pm_opp_put_regulators(bus->opp_table);
>> dev_pm_opp_of_remove_table(dev);
>>
>>>
>>> dev_pm_opp_of_remove_table(dev);
>>> +
>>> clk_disable_unprepare(bus->clk);
>>> }
>>>
>>> @@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>>> {
>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>> struct dev_pm_opp *new_opp;
>>> - unsigned long old_freq, new_freq;
>>> - int ret = 0;
>>> + int ret;
>>>
>>> - /* Get new opp-bus instance according to new bus clock */
>>> + /*
>>> + * New frequency for bus may not be exactly matched to opp, adjust
>>> + * *freq to correct value.
>>> + */
>>
>> You better to change this comment with following styles
>> to keep the consistency:
>>
>> /* Get correct frequency for bus ... */
>>
>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>> if (IS_ERR(new_opp)) {
>>> dev_err(dev, "failed to get recommended opp instance\n");
>>> return PTR_ERR(new_opp);
>>> }
>>>
>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>> dev_pm_opp_put(new_opp);
>>>
>>> - old_freq = bus->curr_freq;
>>> -
>>> - if (old_freq == new_freq)
>>> - return 0;
>>> -
>>> /* Change the frequency according to new OPP level */
>>> mutex_lock(&bus->lock);
>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>> + if (!ret)
>>> + bus->curr_freq = *freq;
>>
>> ditto. Have to print the error log, check above comment.
>>
>>>
>>> - ret = clk_set_rate(bus->clk, new_freq);
>>> - if (ret < 0) {
>>> - dev_err(dev, "failed to set the clock of bus\n");
>>> - goto out;
>>> - }
>>> -
>>> - *freq = new_freq;
>>> - bus->curr_freq = new_freq;
>>> -
>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>> -out:
>>> mutex_unlock(&bus->lock);
>>>
>>> return ret;
>>> @@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>> struct exynos_bus *bus)
>>> {
>>> struct device *dev = bus->dev;
>>> - int i, ret, count, size;
>>> -
>>> - /* Get the regulator to provide each bus with the power */
>>> - bus->regulator = devm_regulator_get(dev, "vdd");
>>> - if (IS_ERR(bus->regulator)) {
>>> - dev_err(dev, "failed to get VDD regulator\n");
>>> - return PTR_ERR(bus->regulator);
>>> - }
>>> -
>>> - ret = regulator_enable(bus->regulator);
>>> - if (ret < 0) {
>>> - dev_err(dev, "failed to enable VDD regulator\n");
>>> - return ret;
>>> - }
>>> + int i, count, size;
>>>
>>> /*
>>> * Get the devfreq-event devices to get the current utilization of
>>> @@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>> count = devfreq_event_get_edev_count(dev);
>>> if (count < 0) {
>>> dev_err(dev, "failed to get the count of devfreq-event dev\n");
>>> - ret = count;
>>> - goto err_regulator;
>>> + return count;
>>> }
>>> +
>>> bus->edev_count = count;
>>>
>>> size = sizeof(*bus->edev) * count;
>>> bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>>> - if (!bus->edev) {
>>> - ret = -ENOMEM;
>>> - goto err_regulator;
>>> - }
>>> + if (!bus->edev)
>>> + return -ENOMEM;
>>>
>>> for (i = 0; i < count; i++) {
>>> bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
>>> - if (IS_ERR(bus->edev[i])) {
>>> - ret = -EPROBE_DEFER;
>>> - goto err_regulator;
>>> - }
>>> + if (IS_ERR(bus->edev[i]))
>>> + return -EPROBE_DEFER;
>>> }
>>>
>>> /*
>>> @@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>> if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>>> bus->ratio = DEFAULT_SATURATION_RATIO;
>>>
>>> - if (of_property_read_u32(np, "exynos,voltage-tolerance",
>>> - &bus->voltage_tolerance))
>>> - bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
>>> -
>>> return 0;
>>> -
>>> -err_regulator:
>>> - regulator_disable(bus->regulator);
>>> -
>>> - return ret;
>>> }
>>>
>>> static int exynos_bus_parse_of(struct device_node *np,
>>> - struct exynos_bus *bus)
>>> + struct exynos_bus *bus, bool passive)
>>> {
>>> struct device *dev = bus->dev;
>>> + struct opp_table *opp_table;
>>> + const char *vdd = "vdd";
>>> struct dev_pm_opp *opp;
>>> unsigned long rate;
>>> int ret;
>>> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
>>> return ret;
>>> }
>>>
>>> + if (!passive) {
>>> + opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>> + if (IS_ERR(opp_table)) {
>>> + ret = PTR_ERR(opp_table);
>>> + dev_err(dev, "failed to set regulators %d\n", ret);
>>> + goto err_clk;/
>>> + }
>>> +
>>> + bus->opp_table = opp_table;
>>> + }
>>
>> This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
>> dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
>> because the regulator is only used by parent devfreq device.
>
> exynos_bus_parse_of() is called for all devfreq devices (including
> parent) and (as you've noticed) the regulator should be enabled before
> enabling clock (which is done in exynos_bus_parse_of()) so adding
> extra argument to exynos_bus_parse_of() (like it is done currently in
> the patch)
I think that this patch has still the problem about call sequence
between clock and regulator as following:
273 ret = clk_prepare_enable(bus->clk);
274 if (ret < 0) {
275 dev_err(dev, "failed to get enable clock\n");
276 return ret;
277 }
278
279 if (!passive) {
280 opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
281 if (IS_ERR(opp_table)) {
282 ret = PTR_ERR(opp_table);
283 dev_err(dev, "failed to set regulators %d\n", ret);
284 goto err_clk;
285 }
286
287 bus->opp_table = opp_table;
288 }
makes it possible to do the setup correctly without the need
> of merging both functions into one huge function (which would be more
> difficult to follow than two simpler functions IMHO). Is that approach
> acceptable or do you prefer one big function?
Actually, I don't force to make one function for both
exynos_bus_parse_of() and exynos_bus_parent_parse_of().
If we just keep this code, dev_pm_opp_set_regulators()
should be handled in exynos_bus_parent_parse_of()
because only parent devfreq device controls the regulator.
In order to keep the two functions, maybe have to change
the call the sequence between exynos_bus_parse_of() and
exynos_bus_parent_parse_of().
Once again, I don't force any fixed method. I want to fix them
with correct way.
>
>>> +
>>> /* Get the freq and voltage from OPP table to scale the bus freq */
>>> ret = dev_pm_opp_of_add_table(dev);
>>> if (ret < 0) {
>>> dev_err(dev, "failed to get OPP table\n");
>>> - goto err_clk;
>>> + goto err_regulator;
>>> }
>>>
>>> rate = clk_get_rate(bus->clk);
>>> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>> ret = PTR_ERR(opp);
>>> goto err_opp;
>>> }
>>> +
>>> bus->curr_freq = dev_pm_opp_get_freq(opp);
>>> dev_pm_opp_put(opp);
>>>
>>> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>
>>> err_opp:
>>> dev_pm_opp_of_remove_table(dev);
>>> +
>>> +err_regulator:
>>> + if (bus->opp_table) {
>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>> + bus->opp_table = NULL;
>>> + }
>>
>> As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
>> after removing the opp_table by dev_pm_opp_of_remove_table().
>>
>>> +
>>> err_clk:
>>> clk_disable_unprepare(bus->clk);
>>>
>>> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>> struct exynos_bus *bus;
>>> int ret, max_state;
>>> unsigned long min_freq, max_freq;
>>> + bool passive = false;
>>>
>>> if (!np) {
>>> dev_err(dev, "failed to find devicetree node\n");
>>> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>> bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>>> if (!bus)
>>> return -ENOMEM;
>>> +
>>> mutex_init(&bus->lock);
>>> bus->dev = &pdev->dev;
>>> platform_set_drvdata(pdev, bus);
>>> + node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>> + if (node) {
>>> + of_node_put(node);
>>> + passive = true;
>>> + }
>>>
>>> /* Parse the device-tree to get the resource information */
>>> - ret = exynos_bus_parse_of(np, bus);
>>> + ret = exynos_bus_parse_of(np, bus, passive);
>>> if (ret < 0)
>>> return ret;
>>>
>>> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>> goto err;
>>> }
>>>
>>> - node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>> - if (node) {
>>> - of_node_put(node);
>>> + if (passive)
>>> goto passive;
>>> - } else {
>>> - ret = exynos_bus_parent_parse_of(np, bus);
>>> - }
>>> +
>>> + ret = exynos_bus_parent_parse_of(np, bus);
>>>
>>
>> Remove unneeded blank line.
>>
>>> if (ret < 0)
>>> goto err;
>>> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>
>>> err:
>>> dev_pm_opp_of_remove_table(dev);
>>> + if (bus->opp_table) {
>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>> + bus->opp_table = NULL;
>>> + }
>>> +
>>
>> ditto.
>> Have to disable regulator after disabling the clock
>> to prevent the h/w fault.
>>
>> I think that you should call them with following sequence:
>>
>> clk_disable_unprepare(bus->clk);
>> if (bus->opp_table)
>> dev_pm_opp_put_regulators(bus->opp_table);
>> dev_pm_opp_of_remove_table(dev);
>>
>>> clk_disable_unprepare(bus->clk);
>>>
>>> return ret;
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v2 3/4] ARM: dts: exynos: add initial data for coupled regulators for Exynos5422/5800
From: Bartlomiej Zolnierkiewicz @ 2019-07-16 10:30 UTC (permalink / raw)
To: Krzysztof Kozlowski, Kamil Konieczny
Cc: Marek Szyprowski, Chanwoo Choi, Kukjin Kim, Kyungmin Park,
Mark Rutland, MyungJoo Ham, Nishanth Menon, Rob Herring,
Stephen Boyd, Viresh Kumar, devicetree, linux-arm-kernel,
linux-kernel, linux-pm, linux-samsung-soc@vger.kernel.org
In-Reply-To: <CAJKOXPd6a5aLf1CEhx9m7khPQOwruSuA22efkJb41BsaWXjM3A@mail.gmail.com>
On 7/16/19 11:22 AM, Krzysztof Kozlowski wrote:
> On Mon, 15 Jul 2019 at 14:04, Kamil Konieczny
> <k.konieczny@partner.samsung.com> wrote:
>>
>> Declare Exynos5422/5800 voltage ranges for opp points for big cpu core and
>> bus wcore and couple their voltage supllies as vdd_arm and vdd_int should
>> be in 300mV range.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>
> This one was previously from Marek, now it is from you. Any changes here?
Hmmm, it seems that "From:" tag somehow got lost in v2 compared to v1?
Also the note about adding patch description (which was the only update to
original Marek's patch IIRC) should be added, something like:
...
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
[k.konieczny: add missing patch description]
Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH] opp: Return genpd virtual devices from dev_pm_opp_attach_genpd()
From: Niklas Cassel @ 2019-07-16 10:43 UTC (permalink / raw)
To: Viresh Kumar
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
linux-pm, Vincent Guittot, Rajendra Nayak, linux-kernel
In-Reply-To: <027985ce35873cd218298302a1408da06d48458b.1562565567.git.viresh.kumar@linaro.org>
On Mon, Jul 08, 2019 at 11:30:11AM +0530, Viresh Kumar wrote:
> The cpufreq drivers don't need to do runtime PM operations on the
> virtual devices returned by dev_pm_domain_attach_by_name() and so the
> virtual devices weren't shared with the callers of
> dev_pm_opp_attach_genpd() earlier.
>
> But the IO device drivers would want to do that. This patch updates the
> prototype of dev_pm_opp_attach_genpd() to accept another argument to
> return the pointer to the array of genpd virtual devices.
>
> Reported-by: Rajendra Nayak <rnayak@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> @Rajendra: Can you please test this one ? I have only compile tested it.
>
> drivers/opp/core.c | 5 ++++-
> include/linux/pm_opp.h | 4 ++--
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 2958cc7bbb58..07b6f1187b3b 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1775,6 +1775,7 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
> * dev_pm_opp_attach_genpd - Attach genpd(s) for the device and save virtual device pointer
> * @dev: Consumer device for which the genpd is getting attached.
> * @names: Null terminated array of pointers containing names of genpd to attach.
> + * @virt_devs: Pointer to return the array of virtual devices.
> *
> * Multiple generic power domains for a device are supported with the help of
> * virtual genpd devices, which are created for each consumer device - genpd
> @@ -1789,7 +1790,8 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
> * This helper needs to be called once with a list of all genpd to attach.
> * Otherwise the original device structure will be used instead by the OPP core.
> */
> -struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names)
> +struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
> + const char **names, struct device ***virt_devs)
> {
> struct opp_table *opp_table;
> struct device *virt_dev;
> @@ -1850,6 +1852,7 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names
> name++;
> }
>
> + *virt_devs = opp_table->genpd_virt_devs;
Could we perhaps only do this if (virt_devs), that way callers can send in
NULL if they don't care about the genpd virtual devices.
Kind regards,
Niklas
> mutex_unlock(&opp_table->genpd_virt_dev_lock);
>
> return opp_table;
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index be570761b77a..7c2fe2952f40 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -131,7 +131,7 @@ 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_unregister_set_opp_helper(struct opp_table *opp_table);
> -struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names);
> +struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs);
> void dev_pm_opp_detach_genpd(struct opp_table *opp_table);
> int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
> int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
> @@ -295,7 +295,7 @@ 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 opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names)
> +static inline struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs)
> {
> return ERR_PTR(-ENOTSUPP);
> }
> --
> 2.21.0.rc0.269.g1a574e7a288b
>
^ permalink raw reply
* Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
From: Bartlomiej Zolnierkiewicz @ 2019-07-16 10:59 UTC (permalink / raw)
To: Chanwoo Choi
Cc: Kamil Konieczny, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <5612547b-47c8-0dc4-cb3c-e972782d5a26@samsung.com>
On 7/16/19 12:33 PM, Chanwoo Choi wrote:
> Hi Bartlomiej,
>
> On 19. 7. 16. 오후 7:13, Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi Chanwoo,
>>
>> On 7/16/19 5:56 AM, Chanwoo Choi wrote:
>>> Hi Kamil,
>>>
>>> Looks good to me. But, this patch has some issue.
>>> I added the detailed reviews.
>>>
>>> I recommend that you make the separate patches as following
>>> in order to clarify the role of which apply the dev_pm_opp_* function.
>>>
>>> First patch,
>>> Need to consolidate the following two function into one function.
>>> because the original exynos-bus.c has the problem that the regulator
>>> of parent devfreq device have to be enabled before enabling the clock.
>>> This issue did not happen because bootloader enables the bus-related
>>> regulators before kernel booting.
>>> - exynos_bus_parse_of()
>>> - exynos_bus_parent_parse_of()
>>>> Second patch,
>>> Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()
>>>
>>>
>>> On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
>>>> Reuse opp core code for setting bus clock and voltage. As a side
>>>> effect this allow useage of coupled regulators feature (required
>>>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>>>> uses regulator_set_voltage_triplet() for setting regulator voltage
>>>> while the old code used regulator_set_voltage_tol() with fixed
>>>> tolerance. This patch also removes no longer needed parsing of DT
>>>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>>>> it).
>>>>
>>>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>>> ---
>>>> drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
>>>> 1 file changed, 66 insertions(+), 106 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>>> index 486cc5b422f1..7fc4f76bd848 100644
>>>> --- a/drivers/devfreq/exynos-bus.c
>>>> +++ b/drivers/devfreq/exynos-bus.c
>>>> @@ -25,7 +25,6 @@
>>>> #include <linux/slab.h>
>>>>
>>>> #define DEFAULT_SATURATION_RATIO 40
>>>> -#define DEFAULT_VOLTAGE_TOLERANCE 2
>>>>
>>>> struct exynos_bus {
>>>> struct device *dev;
>>>> @@ -37,9 +36,9 @@ struct exynos_bus {
>>>>
>>>> unsigned long curr_freq;
>>>>
>>>> - struct regulator *regulator;
>>>> + struct opp_table *opp_table;
>>>> +
>>>> struct clk *clk;
>>>> - unsigned int voltage_tolerance;
>>>> unsigned int ratio;
>>>> };
>>>>
>>>> @@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>>>> {
>>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>>> struct dev_pm_opp *new_opp;
>>>> - unsigned long old_freq, new_freq, new_volt, tol;
>>>> int ret = 0;
>>>> -
>>>> - /* Get new opp-bus instance according to new bus clock */
>>>> + /*
>>>> + * New frequency for bus may not be exactly matched to opp, adjust
>>>> + * *freq to correct value.
>>>> + */
>>>
>>> You better to change this comment with following styles
>>> to keep the consistency:
>>>
>>> /* Get correct frequency for bus ... */
>>>
>>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>> if (IS_ERR(new_opp)) {
>>>> dev_err(dev, "failed to get recommended opp instance\n");
>>>> return PTR_ERR(new_opp);
>>>> }
>>>>
>>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>>> - new_volt = dev_pm_opp_get_voltage(new_opp);
>>>> dev_pm_opp_put(new_opp);
>>>>
>>>> - old_freq = bus->curr_freq;
>>>> -
>>>> - if (old_freq == new_freq)
>>>> - return 0;
>>>> - tol = new_volt * bus->voltage_tolerance / 100;
>>>> -
>>>> /* Change voltage and frequency according to new OPP level */
>>>> mutex_lock(&bus->lock);
>>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>>> + if (!ret)
>>>> + bus->curr_freq = *freq;
>>>
>>> Have to print the error log if ret has minus error value.
>>
>> dev_pm_opp_set_rate() should print the error message on all
>> errors so wouldn't printing the error log also here be superfluous?
>>
>> [ Please also note that the other user of dev_pm_opp_set_rate()
>> (cpufreq-dt cpufreq driver) doesn't do this. ]
>
> OK. Thanks for the explanation.
>
>>
>>> Modify it as following:
>>>
>>> if (ret < 0) {
>>> dev_err(dev, "failed to set bus rate\n");
>>> goto err:
>>> }
>>> bus->curr_freq = *freq;
>>>
>>> err:
>>> mutex_unlock(&bus->lock);
>>>
>>> return ret;
>>>
>>>>
>>>> - if (old_freq < new_freq) {
>>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>> - if (ret < 0) {
>>>> - dev_err(bus->dev, "failed to set voltage\n");
>>>> - goto out;
>>>> - }
>>>> - }
>>>> -
>>>> - ret = clk_set_rate(bus->clk, new_freq);
>>>> - if (ret < 0) {
>>>> - dev_err(dev, "failed to change clock of bus\n");
>>>> - clk_set_rate(bus->clk, old_freq);
>>>> - goto out;
>>>> - }
>>>> -
>>>> - if (old_freq > new_freq) {
>>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>> - if (ret < 0) {
>>>> - dev_err(bus->dev, "failed to set voltage\n");
>>>> - goto out;
>>>> - }
>>>> - }
>>>> - bus->curr_freq = new_freq;
>>>> -
>>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>>> -out:
>>>> mutex_unlock(&bus->lock);
>>>>
>>>> return ret;
>>>> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
>>>> if (ret < 0)
>>>> dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>>>
>>>> - if (bus->regulator)
>>>> - regulator_disable(bus->regulator);
>>>> + if (bus->opp_table)
>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>
>>> Have to disable regulator after disabling the clock
>>> to prevent the h/w fault.
>>>
>>> I think that you should call them with following sequence:
>>>
>>> clk_disable_unprepare(bus->clk);
>>> if (bus->opp_table)
>>> dev_pm_opp_put_regulators(bus->opp_table);
>>> dev_pm_opp_of_remove_table(dev);
>>>
>>>>
>>>> dev_pm_opp_of_remove_table(dev);
>>>> +
>>>> clk_disable_unprepare(bus->clk);
>>>> }
>>>>
>>>> @@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>>>> {
>>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>>> struct dev_pm_opp *new_opp;
>>>> - unsigned long old_freq, new_freq;
>>>> - int ret = 0;
>>>> + int ret;
>>>>
>>>> - /* Get new opp-bus instance according to new bus clock */
>>>> + /*
>>>> + * New frequency for bus may not be exactly matched to opp, adjust
>>>> + * *freq to correct value.
>>>> + */
>>>
>>> You better to change this comment with following styles
>>> to keep the consistency:
>>>
>>> /* Get correct frequency for bus ... */
>>>
>>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>> if (IS_ERR(new_opp)) {
>>>> dev_err(dev, "failed to get recommended opp instance\n");
>>>> return PTR_ERR(new_opp);
>>>> }
>>>>
>>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>>> dev_pm_opp_put(new_opp);
>>>>
>>>> - old_freq = bus->curr_freq;
>>>> -
>>>> - if (old_freq == new_freq)
>>>> - return 0;
>>>> -
>>>> /* Change the frequency according to new OPP level */
>>>> mutex_lock(&bus->lock);
>>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>>> + if (!ret)
>>>> + bus->curr_freq = *freq;
>>>
>>> ditto. Have to print the error log, check above comment.
>>>
>>>>
>>>> - ret = clk_set_rate(bus->clk, new_freq);
>>>> - if (ret < 0) {
>>>> - dev_err(dev, "failed to set the clock of bus\n");
>>>> - goto out;
>>>> - }
>>>> -
>>>> - *freq = new_freq;
>>>> - bus->curr_freq = new_freq;
>>>> -
>>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>>> -out:
>>>> mutex_unlock(&bus->lock);
>>>>
>>>> return ret;
>>>> @@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>> struct exynos_bus *bus)
>>>> {
>>>> struct device *dev = bus->dev;
>>>> - int i, ret, count, size;
>>>> -
>>>> - /* Get the regulator to provide each bus with the power */
>>>> - bus->regulator = devm_regulator_get(dev, "vdd");
>>>> - if (IS_ERR(bus->regulator)) {
>>>> - dev_err(dev, "failed to get VDD regulator\n");
>>>> - return PTR_ERR(bus->regulator);
>>>> - }
>>>> -
>>>> - ret = regulator_enable(bus->regulator);
>>>> - if (ret < 0) {
>>>> - dev_err(dev, "failed to enable VDD regulator\n");
>>>> - return ret;
>>>> - }
>>>> + int i, count, size;
>>>>
>>>> /*
>>>> * Get the devfreq-event devices to get the current utilization of
>>>> @@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>> count = devfreq_event_get_edev_count(dev);
>>>> if (count < 0) {
>>>> dev_err(dev, "failed to get the count of devfreq-event dev\n");
>>>> - ret = count;
>>>> - goto err_regulator;
>>>> + return count;
>>>> }
>>>> +
>>>> bus->edev_count = count;
>>>>
>>>> size = sizeof(*bus->edev) * count;
>>>> bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>>>> - if (!bus->edev) {
>>>> - ret = -ENOMEM;
>>>> - goto err_regulator;
>>>> - }
>>>> + if (!bus->edev)
>>>> + return -ENOMEM;
>>>>
>>>> for (i = 0; i < count; i++) {
>>>> bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
>>>> - if (IS_ERR(bus->edev[i])) {
>>>> - ret = -EPROBE_DEFER;
>>>> - goto err_regulator;
>>>> - }
>>>> + if (IS_ERR(bus->edev[i]))
>>>> + return -EPROBE_DEFER;
>>>> }
>>>>
>>>> /*
>>>> @@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>> if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>>>> bus->ratio = DEFAULT_SATURATION_RATIO;
>>>>
>>>> - if (of_property_read_u32(np, "exynos,voltage-tolerance",
>>>> - &bus->voltage_tolerance))
>>>> - bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
>>>> -
>>>> return 0;
>>>> -
>>>> -err_regulator:
>>>> - regulator_disable(bus->regulator);
>>>> -
>>>> - return ret;
>>>> }
>>>>
>>>> static int exynos_bus_parse_of(struct device_node *np,
>>>> - struct exynos_bus *bus)
>>>> + struct exynos_bus *bus, bool passive)
>>>> {
>>>> struct device *dev = bus->dev;
>>>> + struct opp_table *opp_table;
>>>> + const char *vdd = "vdd";
>>>> struct dev_pm_opp *opp;
>>>> unsigned long rate;
>>>> int ret;
>>>> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>> return ret;
>>>> }
>>>>
>>>> + if (!passive) {
>>>> + opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>>> + if (IS_ERR(opp_table)) {
>>>> + ret = PTR_ERR(opp_table);
>>>> + dev_err(dev, "failed to set regulators %d\n", ret);
>>>> + goto err_clk;/
>>>> + }
>>>> +
>>>> + bus->opp_table = opp_table;
>>>> + }
>>>
>>> This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
>>> dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
>>> because the regulator is only used by parent devfreq device.
>>
>> exynos_bus_parse_of() is called for all devfreq devices (including
>> parent) and (as you've noticed) the regulator should be enabled before
>> enabling clock (which is done in exynos_bus_parse_of()) so adding
>> extra argument to exynos_bus_parse_of() (like it is done currently in
>> the patch)
>
> I think that this patch has still the problem about call sequence
> between clock and regulator as following:
Yes, this should be fixed (though the wrong sequence between regulator
and clock handling is not introduced by the patchset itself and is present
in the original driver code).
> 273 ret = clk_prepare_enable(bus->clk);
> 274 if (ret < 0) {
> 275 dev_err(dev, "failed to get enable clock\n");
> 276 return ret;
> 277 }
> 278
> 279 if (!passive) {
> 280 opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
> 281 if (IS_ERR(opp_table)) {
> 282 ret = PTR_ERR(opp_table);
> 283 dev_err(dev, "failed to set regulators %d\n", ret);
> 284 goto err_clk;
> 285 }
> 286
> 287 bus->opp_table = opp_table;
> 288 }
>
> makes it possible to do the setup correctly without the need
>> of merging both functions into one huge function (which would be more
>> difficult to follow than two simpler functions IMHO). Is that approach
>> acceptable or do you prefer one big function?
>
> Actually, I don't force to make one function for both
> exynos_bus_parse_of() and exynos_bus_parent_parse_of().
>
> If we just keep this code, dev_pm_opp_set_regulators()
> should be handled in exynos_bus_parent_parse_of()
> because only parent devfreq device controls the regulator.
Could your please explain rationale for this requirement (besides
function name)?
The patch adds 'bool passive' argument (which is set to false for
parent devfreq device and true for child devfreq device) to
exynos_bus_parse_of() (which is called for *all* devfreq devices
and is called before exynos_bus_parent_parse_of()) and there is
no hard requirement to call dev_pm_opp_set_regulators() in
exynos_bus_parent_parse_of() so after only changing the ordering
between regulator and clock handling the setup code should be
correct.
[ Please note that this patch moves parent/child detection before
exynos_bus_parse_of() call. ]
> In order to keep the two functions, maybe have to change
> the call the sequence between exynos_bus_parse_of() and
> exynos_bus_parent_parse_of().
Doesn't seem to be needed, care to explain it more?
> Once again, I don't force any fixed method. I want to fix them
> with correct way.
>
>>
>>>> +
>>>> /* Get the freq and voltage from OPP table to scale the bus freq */
>>>> ret = dev_pm_opp_of_add_table(dev);
>>>> if (ret < 0) {
>>>> dev_err(dev, "failed to get OPP table\n");
>>>> - goto err_clk;
>>>> + goto err_regulator;
>>>> }
>>>>
>>>> rate = clk_get_rate(bus->clk);
>>>> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>> ret = PTR_ERR(opp);
>>>> goto err_opp;
>>>> }
>>>> +
>>>> bus->curr_freq = dev_pm_opp_get_freq(opp);
>>>> dev_pm_opp_put(opp);
>>>>
>>>> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>
>>>> err_opp:
>>>> dev_pm_opp_of_remove_table(dev);
>>>> +
>>>> +err_regulator:
>>>> + if (bus->opp_table) {
>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>> + bus->opp_table = NULL;
>>>> + }
>>>
>>> As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
>>> after removing the opp_table by dev_pm_opp_of_remove_table().
>>>
>>>> +
>>>> err_clk:
>>>> clk_disable_unprepare(bus->clk);
>>>>
>>>> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>> struct exynos_bus *bus;
>>>> int ret, max_state;
>>>> unsigned long min_freq, max_freq;
>>>> + bool passive = false;
>>>>
>>>> if (!np) {
>>>> dev_err(dev, "failed to find devicetree node\n");
>>>> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>> bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>>>> if (!bus)
>>>> return -ENOMEM;
>>>> +
>>>> mutex_init(&bus->lock);
>>>> bus->dev = &pdev->dev;
>>>> platform_set_drvdata(pdev, bus);
>>>> + node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>> + if (node) {
>>>> + of_node_put(node);
>>>> + passive = true;
>>>> + }
>>>>
>>>> /* Parse the device-tree to get the resource information */
>>>> - ret = exynos_bus_parse_of(np, bus);
>>>> + ret = exynos_bus_parse_of(np, bus, passive);
>>>> if (ret < 0)
>>>> return ret;
>>>>
>>>> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>> goto err;
>>>> }
>>>>
>>>> - node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>> - if (node) {
>>>> - of_node_put(node);
>>>> + if (passive)
>>>> goto passive;
>>>> - } else {
>>>> - ret = exynos_bus_parent_parse_of(np, bus);
>>>> - }
>>>> +
>>>> + ret = exynos_bus_parent_parse_of(np, bus);
>>>>
>>>
>>> Remove unneeded blank line.
>>>
>>>> if (ret < 0)
>>>> goto err;
>>>> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>
>>>> err:
>>>> dev_pm_opp_of_remove_table(dev);
>>>> + if (bus->opp_table) {
>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>> + bus->opp_table = NULL;
>>>> + }
>>>> +
>>>
>>> ditto.
>>> Have to disable regulator after disabling the clock
>>> to prevent the h/w fault.
>>>
>>> I think that you should call them with following sequence:
>>>
>>> clk_disable_unprepare(bus->clk);
>>> if (bus->opp_table)
>>> dev_pm_opp_put_regulators(bus->opp_table);
>>> dev_pm_opp_of_remove_table(dev);
>>>
>>>> clk_disable_unprepare(bus->clk);
>>>>
>>>> return ret;
>>
>> Best regards,
>> --
>> Bartlomiej Zolnierkiewicz
>> Samsung R&D Institute Poland
>> Samsung Electronics
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 04/13] cpufreq: qcom: Refactor the driver to make it easier to extend
From: Niklas Cassel @ 2019-07-16 11:11 UTC (permalink / raw)
To: Viresh Kumar
Cc: Andy Gross, Ilia Lin, Rafael J. Wysocki, linux-arm-msm,
jorge.ramirez-ortiz, sboyd, vireshk, bjorn.andersson, ulf.hansson,
linux-pm, linux-kernel
In-Reply-To: <20190710063026.7yyoxjsqp2ck3z6n@vireshk-i7>
On Wed, Jul 10, 2019 at 12:00:26PM +0530, Viresh Kumar wrote:
> On 05-07-19, 11:57, Niklas Cassel wrote:
> > + drv->opp_tables = kcalloc(num_possible_cpus(), sizeof(*drv->opp_tables),
> > + GFP_KERNEL);
> > + if (!drv->opp_tables) {
> > + ret = -ENOMEM;
> > + goto free_drv;
> > + }
> >
> > for_each_possible_cpu(cpu) {
> > cpu_dev = get_cpu_device(cpu);
> > @@ -166,19 +195,23 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
> > goto free_opp;
> > }
> >
> > - opp_tables[cpu] = dev_pm_opp_set_supported_hw(cpu_dev,
> > - &versions, 1);
> > - if (IS_ERR(opp_tables[cpu])) {
> > - ret = PTR_ERR(opp_tables[cpu]);
> > - dev_err(cpu_dev, "Failed to set supported hardware\n");
> > - goto free_opp;
> > + if (drv->data->get_version) {
>
> Why depend on get_version here ? The OPP table is already allocated
> unconditionally.
Since the reading of the speedbin efuse is now optional,
it is now inside "if (drv->data->get_version)".
So if I don't also protect "dev_pm_opp_set_supported_hw()"
with "if (drv->data->get_version)" I get:
[ 3.135092] cpu cpu0: _opp_is_supported: failed to read opp-supported-hw property at index 0: -22
[ 3.139364] cpu cpu0: _opp_is_supported: failed to read opp-supported-hw property at index 0: -22
[ 3.148330] cpu cpu0: _opp_is_supported: failed to read opp-supported-hw property at index 0: -22
Probably since drv->versions is initialized to 0,
and if there is no opp-supported-hw in device tree,
OPP framework prints failures.
So it feels safest to only call dev_pm_opp_set_supported_hw()
if we know that we are supposed to parse the speedbin efuse.
Kind regards,
Niklas
>
> > + drv->opp_tables[cpu] =
> > + dev_pm_opp_set_supported_hw(cpu_dev,
> > + &drv->versions, 1);
> > + if (IS_ERR(drv->opp_tables[cpu])) {
> > + ret = PTR_ERR(drv->opp_tables[cpu]);
> > + dev_err(cpu_dev,
> > + "Failed to set supported hardware\n");
> > + goto free_opp;
> > + }
> > }
> > }
>
> --
> viresh
^ permalink raw reply
* Re: [PATCH 02/13] cpufreq: qcom: Re-organise kryo cpufreq to use it for other nvmem based qcom socs
From: Niklas Cassel @ 2019-07-16 11:11 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Andy Gross, Ilia Lin, linux-arm-msm,
jorge.ramirez-ortiz, sboyd, vireshk, bjorn.andersson, ulf.hansson,
Sricharan R, linux-kernel, linux-pm
In-Reply-To: <20190710061839.quncnis6pjyjntip@vireshk-i7>
On Wed, Jul 10, 2019 at 11:48:39AM +0530, Viresh Kumar wrote:
> On 05-07-19, 11:57, Niklas Cassel wrote:
> > -static struct platform_driver qcom_cpufreq_kryo_driver = {
> > - .probe = qcom_cpufreq_kryo_probe,
> > - .remove = qcom_cpufreq_kryo_remove,
> > +static struct platform_driver qcom_cpufreq_driver = {
> > + .probe = qcom_cpufreq_probe,
> > + .remove = qcom_cpufreq_remove,
> > .driver = {
> > - .name = "qcom-cpufreq-kryo",
> > + .name = "qcom-cpufreq",
>
> Should we still name it "qcom-cpufreq-nvmem" here ? Only the string
> here.
Sure, I can fix this in next version.
Kind regards,
Niklas
>
> > },
> > };
>
> --
> viresh
^ permalink raw reply
* Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
From: Chanwoo Choi @ 2019-07-16 11:26 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Kamil Konieczny, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <3ba736fa-832c-a72c-e60b-f4328e54c524@samsung.com>
Hi,
On 19. 7. 16. 오후 7:59, Bartlomiej Zolnierkiewicz wrote:
>
> On 7/16/19 12:33 PM, Chanwoo Choi wrote:
>> Hi Bartlomiej,
>>
>> On 19. 7. 16. 오후 7:13, Bartlomiej Zolnierkiewicz wrote:
>>>
>>> Hi Chanwoo,
>>>
>>> On 7/16/19 5:56 AM, Chanwoo Choi wrote:
>>>> Hi Kamil,
>>>>
>>>> Looks good to me. But, this patch has some issue.
>>>> I added the detailed reviews.
>>>>
>>>> I recommend that you make the separate patches as following
>>>> in order to clarify the role of which apply the dev_pm_opp_* function.
>>>>
>>>> First patch,
>>>> Need to consolidate the following two function into one function.
>>>> because the original exynos-bus.c has the problem that the regulator
>>>> of parent devfreq device have to be enabled before enabling the clock.
>>>> This issue did not happen because bootloader enables the bus-related
>>>> regulators before kernel booting.
>>>> - exynos_bus_parse_of()
>>>> - exynos_bus_parent_parse_of()
>>>>> Second patch,
>>>> Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()
>>>>
>>>>
>>>> On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
>>>>> Reuse opp core code for setting bus clock and voltage. As a side
>>>>> effect this allow useage of coupled regulators feature (required
>>>>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>>>>> uses regulator_set_voltage_triplet() for setting regulator voltage
>>>>> while the old code used regulator_set_voltage_tol() with fixed
>>>>> tolerance. This patch also removes no longer needed parsing of DT
>>>>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>>>>> it).
>>>>>
>>>>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>>>> ---
>>>>> drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
>>>>> 1 file changed, 66 insertions(+), 106 deletions(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>>>> index 486cc5b422f1..7fc4f76bd848 100644
>>>>> --- a/drivers/devfreq/exynos-bus.c
>>>>> +++ b/drivers/devfreq/exynos-bus.c
>>>>> @@ -25,7 +25,6 @@
>>>>> #include <linux/slab.h>
>>>>>
>>>>> #define DEFAULT_SATURATION_RATIO 40
>>>>> -#define DEFAULT_VOLTAGE_TOLERANCE 2
>>>>>
>>>>> struct exynos_bus {
>>>>> struct device *dev;
>>>>> @@ -37,9 +36,9 @@ struct exynos_bus {
>>>>>
>>>>> unsigned long curr_freq;
>>>>>
>>>>> - struct regulator *regulator;
>>>>> + struct opp_table *opp_table;
>>>>> +
>>>>> struct clk *clk;
>>>>> - unsigned int voltage_tolerance;
>>>>> unsigned int ratio;
>>>>> };
>>>>>
>>>>> @@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>>>>> {
>>>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>>>> struct dev_pm_opp *new_opp;
>>>>> - unsigned long old_freq, new_freq, new_volt, tol;
>>>>> int ret = 0;
>>>>> -
>>>>> - /* Get new opp-bus instance according to new bus clock */
>>>>> + /*
>>>>> + * New frequency for bus may not be exactly matched to opp, adjust
>>>>> + * *freq to correct value.
>>>>> + */
>>>>
>>>> You better to change this comment with following styles
>>>> to keep the consistency:
>>>>
>>>> /* Get correct frequency for bus ... */
>>>>
>>>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>>> if (IS_ERR(new_opp)) {
>>>>> dev_err(dev, "failed to get recommended opp instance\n");
>>>>> return PTR_ERR(new_opp);
>>>>> }
>>>>>
>>>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>>>> - new_volt = dev_pm_opp_get_voltage(new_opp);
>>>>> dev_pm_opp_put(new_opp);
>>>>>
>>>>> - old_freq = bus->curr_freq;
>>>>> -
>>>>> - if (old_freq == new_freq)
>>>>> - return 0;
>>>>> - tol = new_volt * bus->voltage_tolerance / 100;
>>>>> -
>>>>> /* Change voltage and frequency according to new OPP level */
>>>>> mutex_lock(&bus->lock);
>>>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>>>> + if (!ret)
>>>>> + bus->curr_freq = *freq;
>>>>
>>>> Have to print the error log if ret has minus error value.
>>>
>>> dev_pm_opp_set_rate() should print the error message on all
>>> errors so wouldn't printing the error log also here be superfluous?
>>>
>>> [ Please also note that the other user of dev_pm_opp_set_rate()
>>> (cpufreq-dt cpufreq driver) doesn't do this. ]
>>
>> OK. Thanks for the explanation.
>>
>>>
>>>> Modify it as following:
>>>>
>>>> if (ret < 0) {
>>>> dev_err(dev, "failed to set bus rate\n");
>>>> goto err:
>>>> }
>>>> bus->curr_freq = *freq;
>>>>
>>>> err:
>>>> mutex_unlock(&bus->lock);
>>>>
>>>> return ret;
>>>>
>>>>>
>>>>> - if (old_freq < new_freq) {
>>>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>>> - if (ret < 0) {
>>>>> - dev_err(bus->dev, "failed to set voltage\n");
>>>>> - goto out;
>>>>> - }
>>>>> - }
>>>>> -
>>>>> - ret = clk_set_rate(bus->clk, new_freq);
>>>>> - if (ret < 0) {
>>>>> - dev_err(dev, "failed to change clock of bus\n");
>>>>> - clk_set_rate(bus->clk, old_freq);
>>>>> - goto out;
>>>>> - }
>>>>> -
>>>>> - if (old_freq > new_freq) {
>>>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>>> - if (ret < 0) {
>>>>> - dev_err(bus->dev, "failed to set voltage\n");
>>>>> - goto out;
>>>>> - }
>>>>> - }
>>>>> - bus->curr_freq = new_freq;
>>>>> -
>>>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>>>> -out:
>>>>> mutex_unlock(&bus->lock);
>>>>>
>>>>> return ret;
>>>>> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
>>>>> if (ret < 0)
>>>>> dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>>>>
>>>>> - if (bus->regulator)
>>>>> - regulator_disable(bus->regulator);
>>>>> + if (bus->opp_table)
>>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>>
>>>> Have to disable regulator after disabling the clock
>>>> to prevent the h/w fault.
>>>>
>>>> I think that you should call them with following sequence:
>>>>
>>>> clk_disable_unprepare(bus->clk);
>>>> if (bus->opp_table)
>>>> dev_pm_opp_put_regulators(bus->opp_table);
>>>> dev_pm_opp_of_remove_table(dev);
>>>>
>>>>>
>>>>> dev_pm_opp_of_remove_table(dev);
>>>>> +
>>>>> clk_disable_unprepare(bus->clk);
>>>>> }
>>>>>
>>>>> @@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>>>>> {
>>>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>>>> struct dev_pm_opp *new_opp;
>>>>> - unsigned long old_freq, new_freq;
>>>>> - int ret = 0;
>>>>> + int ret;
>>>>>
>>>>> - /* Get new opp-bus instance according to new bus clock */
>>>>> + /*
>>>>> + * New frequency for bus may not be exactly matched to opp, adjust
>>>>> + * *freq to correct value.
>>>>> + */
>>>>
>>>> You better to change this comment with following styles
>>>> to keep the consistency:
>>>>
>>>> /* Get correct frequency for bus ... */
>>>>
>>>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>>> if (IS_ERR(new_opp)) {
>>>>> dev_err(dev, "failed to get recommended opp instance\n");
>>>>> return PTR_ERR(new_opp);
>>>>> }
>>>>>
>>>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>>>> dev_pm_opp_put(new_opp);
>>>>>
>>>>> - old_freq = bus->curr_freq;
>>>>> -
>>>>> - if (old_freq == new_freq)
>>>>> - return 0;
>>>>> -
>>>>> /* Change the frequency according to new OPP level */
>>>>> mutex_lock(&bus->lock);
>>>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>>>> + if (!ret)
>>>>> + bus->curr_freq = *freq;
>>>>
>>>> ditto. Have to print the error log, check above comment.
>>>>
>>>>>
>>>>> - ret = clk_set_rate(bus->clk, new_freq);
>>>>> - if (ret < 0) {
>>>>> - dev_err(dev, "failed to set the clock of bus\n");
>>>>> - goto out;
>>>>> - }
>>>>> -
>>>>> - *freq = new_freq;
>>>>> - bus->curr_freq = new_freq;
>>>>> -
>>>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>>>> -out:
>>>>> mutex_unlock(&bus->lock);
>>>>>
>>>>> return ret;
>>>>> @@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>> struct exynos_bus *bus)
>>>>> {
>>>>> struct device *dev = bus->dev;
>>>>> - int i, ret, count, size;
>>>>> -
>>>>> - /* Get the regulator to provide each bus with the power */
>>>>> - bus->regulator = devm_regulator_get(dev, "vdd");
>>>>> - if (IS_ERR(bus->regulator)) {
>>>>> - dev_err(dev, "failed to get VDD regulator\n");
>>>>> - return PTR_ERR(bus->regulator);
>>>>> - }
>>>>> -
>>>>> - ret = regulator_enable(bus->regulator);
>>>>> - if (ret < 0) {
>>>>> - dev_err(dev, "failed to enable VDD regulator\n");
>>>>> - return ret;
>>>>> - }
>>>>> + int i, count, size;
>>>>>
>>>>> /*
>>>>> * Get the devfreq-event devices to get the current utilization of
>>>>> @@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>> count = devfreq_event_get_edev_count(dev);
>>>>> if (count < 0) {
>>>>> dev_err(dev, "failed to get the count of devfreq-event dev\n");
>>>>> - ret = count;
>>>>> - goto err_regulator;
>>>>> + return count;
>>>>> }
>>>>> +
>>>>> bus->edev_count = count;
>>>>>
>>>>> size = sizeof(*bus->edev) * count;
>>>>> bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>>>>> - if (!bus->edev) {
>>>>> - ret = -ENOMEM;
>>>>> - goto err_regulator;
>>>>> - }
>>>>> + if (!bus->edev)
>>>>> + return -ENOMEM;
>>>>>
>>>>> for (i = 0; i < count; i++) {
>>>>> bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
>>>>> - if (IS_ERR(bus->edev[i])) {
>>>>> - ret = -EPROBE_DEFER;
>>>>> - goto err_regulator;
>>>>> - }
>>>>> + if (IS_ERR(bus->edev[i]))
>>>>> + return -EPROBE_DEFER;
>>>>> }
>>>>>
>>>>> /*
>>>>> @@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>> if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>>>>> bus->ratio = DEFAULT_SATURATION_RATIO;
>>>>>
>>>>> - if (of_property_read_u32(np, "exynos,voltage-tolerance",
>>>>> - &bus->voltage_tolerance))
>>>>> - bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
>>>>> -
>>>>> return 0;
>>>>> -
>>>>> -err_regulator:
>>>>> - regulator_disable(bus->regulator);
>>>>> -
>>>>> - return ret;
>>>>> }
>>>>>
>>>>> static int exynos_bus_parse_of(struct device_node *np,
>>>>> - struct exynos_bus *bus)
>>>>> + struct exynos_bus *bus, bool passive)
>>>>> {
>>>>> struct device *dev = bus->dev;
>>>>> + struct opp_table *opp_table;
>>>>> + const char *vdd = "vdd";
>>>>> struct dev_pm_opp *opp;
>>>>> unsigned long rate;
>>>>> int ret;
>>>>> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>> return ret;
>>>>> }
>>>>>
>>>>> + if (!passive) {
>>>>> + opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>>>> + if (IS_ERR(opp_table)) {
>>>>> + ret = PTR_ERR(opp_table);
>>>>> + dev_err(dev, "failed to set regulators %d\n", ret);
>>>>> + goto err_clk;/
>>>>> + }
>>>>> +
>>>>> + bus->opp_table = opp_table;
>>>>> + }
>>>>
>>>> This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
>>>> dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
>>>> because the regulator is only used by parent devfreq device.
>>>
>>> exynos_bus_parse_of() is called for all devfreq devices (including
>>> parent) and (as you've noticed) the regulator should be enabled before
>>> enabling clock (which is done in exynos_bus_parse_of()) so adding
>>> extra argument to exynos_bus_parse_of() (like it is done currently in
>>> the patch)
>>
>> I think that this patch has still the problem about call sequence
>> between clock and regulator as following:
>
> Yes, this should be fixed (though the wrong sequence between regulator
> and clock handling is not introduced by the patchset itself and is present
> in the original driver code).
>
>> 273 ret = clk_prepare_enable(bus->clk);
>> 274 if (ret < 0) {
>> 275 dev_err(dev, "failed to get enable clock\n");
>> 276 return ret;
>> 277 }
>> 278
>> 279 if (!passive) {
>> 280 opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>> 281 if (IS_ERR(opp_table)) {
>> 282 ret = PTR_ERR(opp_table);
>> 283 dev_err(dev, "failed to set regulators %d\n", ret);
>> 284 goto err_clk;
>> 285 }
>> 286
>> 287 bus->opp_table = opp_table;
>> 288 }
>>
>> makes it possible to do the setup correctly without the need
>>> of merging both functions into one huge function (which would be more
>>> difficult to follow than two simpler functions IMHO). Is that approach
>>> acceptable or do you prefer one big function?
>>
>> Actually, I don't force to make one function for both
>> exynos_bus_parse_of() and exynos_bus_parent_parse_of().
>>
>> If we just keep this code, dev_pm_opp_set_regulators()
>> should be handled in exynos_bus_parent_parse_of()
>> because only parent devfreq device controls the regulator.
>
> Could your please explain rationale for this requirement (besides
> function name)?
OK. I hope to satisfy the following requirements:
1. Fix the sequence problem between clock and regulator for enabling them.
2. dev_pm_opp_set_regulator() have to be handled in exynos_bus_parent_parse_of()
instead of exynos_bus_parse_of() for only parent devfreq device.
3. exynos_bus_parse_of() have to handle the only common properties
of both parent devfreq device and passive devfreq device.
>
> The patch adds 'bool passive' argument (which is set to false for
> parent devfreq device and true for child devfreq device) to
> exynos_bus_parse_of() (which is called for *all* devfreq devices
As I menteiond, exynos_bus_parse_of have to handle the only common
properties of both parent device and passive device.
I gathered the properties for parent device into exynos_bus_parent_parse_of()
This way using 'bool passive' argument is not proper in exynos_bus_parse_of().
> and is called before exynos_bus_parent_parse_of()) and there is
> no hard requirement to call dev_pm_opp_set_regulators() in
> exynos_bus_parent_parse_of() so after only changing the ordering
> between regulator and clock handling the setup code should be
> correct.
>
> [ Please note that this patch moves parent/child detection before
> exynos_bus_parse_of() call. ]
>
>> In order to keep the two functions, maybe have to change
>> the call the sequence between exynos_bus_parse_of() and
>> exynos_bus_parent_parse_of().
>
> Doesn't seem to be needed, care to explain it more?
In order to fix the sequence problem between clock and regulator
with dev_pm_opp_set_regualtor() and want to keep two functions
(exynos_bus_parent_parse_of() and exynos_bus_parse_of()),
have to change the call order as following and then modify
the exception handling code when error happen.
node = of_parse_phandle(dev->of_node, "devfreq", 0);
if (node) {
of_node_put(node);
passive = true
}
if (!passive)
exynos_bus_parent_parse_of()
dev_pm_opp_set_regulator
exynos_bus_parse_of()
>
>> Once again, I don't force any fixed method. I want to fix them
>> with correct way.
>>
>>>
>>>>> +
>>>>> /* Get the freq and voltage from OPP table to scale the bus freq */
>>>>> ret = dev_pm_opp_of_add_table(dev);
>>>>> if (ret < 0) {
>>>>> dev_err(dev, "failed to get OPP table\n");
>>>>> - goto err_clk;
>>>>> + goto err_regulator;
>>>>> }
>>>>>
>>>>> rate = clk_get_rate(bus->clk);
>>>>> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>> ret = PTR_ERR(opp);
>>>>> goto err_opp;
>>>>> }
>>>>> +
>>>>> bus->curr_freq = dev_pm_opp_get_freq(opp);
>>>>> dev_pm_opp_put(opp);
>>>>>
>>>>> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>
>>>>> err_opp:
>>>>> dev_pm_opp_of_remove_table(dev);
>>>>> +
>>>>> +err_regulator:
>>>>> + if (bus->opp_table) {
>>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>>> + bus->opp_table = NULL;
>>>>> + }
>>>>
>>>> As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
>>>> after removing the opp_table by dev_pm_opp_of_remove_table().
>>>>
>>>>> +
>>>>> err_clk:
>>>>> clk_disable_unprepare(bus->clk);
>>>>>
>>>>> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>> struct exynos_bus *bus;
>>>>> int ret, max_state;
>>>>> unsigned long min_freq, max_freq;
>>>>> + bool passive = false;
>>>>>
>>>>> if (!np) {
>>>>> dev_err(dev, "failed to find devicetree node\n");
>>>>> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>> bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>>>>> if (!bus)
>>>>> return -ENOMEM;
>>>>> +
>>>>> mutex_init(&bus->lock);
>>>>> bus->dev = &pdev->dev;
>>>>> platform_set_drvdata(pdev, bus);
>>>>> + node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>>> + if (node) {
>>>>> + of_node_put(node);
>>>>> + passive = true;
>>>>> + }
>>>>>
>>>>> /* Parse the device-tree to get the resource information */
>>>>> - ret = exynos_bus_parse_of(np, bus);
>>>>> + ret = exynos_bus_parse_of(np, bus, passive);
>>>>> if (ret < 0)
>>>>> return ret;
>>>>>
>>>>> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>> goto err;
>>>>> }
>>>>>
>>>>> - node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>>> - if (node) {
>>>>> - of_node_put(node);
>>>>> + if (passive)
>>>>> goto passive;
>>>>> - } else {
>>>>> - ret = exynos_bus_parent_parse_of(np, bus);
>>>>> - }
>>>>> +
>>>>> + ret = exynos_bus_parent_parse_of(np, bus);
>>>>>
>>>>
>>>> Remove unneeded blank line.
>>>>
>>>>> if (ret < 0)
>>>>> goto err;
>>>>> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>
>>>>> err:
>>>>> dev_pm_opp_of_remove_table(dev);
>>>>> + if (bus->opp_table) {
>>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>>> + bus->opp_table = NULL;
>>>>> + }
>>>>> +
>>>>
>>>> ditto.
>>>> Have to disable regulator after disabling the clock
>>>> to prevent the h/w fault.
>>>>
>>>> I think that you should call them with following sequence:
>>>>
>>>> clk_disable_unprepare(bus->clk);
>>>> if (bus->opp_table)
>>>> dev_pm_opp_put_regulators(bus->opp_table);
>>>> dev_pm_opp_of_remove_table(dev);
>>>>
>>>>> clk_disable_unprepare(bus->clk);
>>>>>
>>>>> return ret;
>>>
>>> Best regards,
>>> --
>>> Bartlomiej Zolnierkiewicz
>>> Samsung R&D Institute Poland
>>> Samsung Electronics
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v4 01/24] PM / devfreq: tegra30: Change irq type to unsigned int
From: Chanwoo Choi @ 2019-07-16 11:35 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, MyungJoo Ham, Kyungmin Park,
Jonathan Hunter, Tomeu Vizoso
Cc: linux-pm, linux-tegra, linux-kernel
In-Reply-To: <20190707223303.6755-2-digetx@gmail.com>
Hi Dmitry,
On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> IRQ numbers are always positive, hence the corresponding variable should
> be unsigned to keep types consistent. This is a minor change that cleans
> up code a tad more.
>
> Suggested-by: Thierry Reding <thierry.reding@gmail.com>
> Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> drivers/devfreq/tegra30-devfreq.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index a6ba75f4106d..a27300f40b0b 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -160,7 +160,7 @@ struct tegra_devfreq {
>
> struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
>
> - int irq;
> + unsigned int irq;
> };
>
> struct tegra_actmon_emc_ratio {
> @@ -618,12 +618,12 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> return PTR_ERR(tegra->emc_clock);
> }
>
> - tegra->irq = platform_get_irq(pdev, 0);
> - if (tegra->irq < 0) {
> - err = tegra->irq;
> + err = platform_get_irq(pdev, 0);
> + if (err < 0) {
> dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err);
> return err;
> }
> + tegra->irq = err;
>
> reset_control_assert(tegra->reset);
>
>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox