* [PATCH v2 0/4] fixed mediatek-cpufreq has multi policy concurrency issue
@ 2024-11-08 6:39 Mark Tseng
2024-11-08 6:39 ` [PATCH v2 1/4] cpufreq: mediatek: CCI support SoC , the transition_delay set to 10 ms Mark Tseng
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Mark Tseng @ 2024-11-08 6:39 UTC (permalink / raw)
To: Rafael J . Wysocki, Viresh Kumar, MyungJoo Ham, Kyungmin Park,
Chanwoo Choi, Matthias Brugger, AngeloGioacchino Del Regno
Cc: linux-pm, linux-kernel, linux-arm-kernel, linux-mediatek,
Project_Global_Chrome_Upstream_Group, chun-jen.tseng
For multi cluster SoC, the cpufreq->target_index() is re-enter function
for each policy to change CPU frequency. In the cirtical session must
use glocal mutex lock to avoid get wrong OPP.
Changes since v1:
- seperate more patch for detail change.
Mark Tseng (4):
cpufreq: mediatek: CCI support SoC , the transition_delay set to 10 ms
cpufreq: mediatek: using global lock avoid race condition
cpufreq: mediatek: Add CPUFREQ_ASYNC_NOTIFICATION flag
cpufreq: mediatek: data safety protect
drivers/cpufreq/mediatek-cpufreq.c | 65 ++++++++++++++++++++++--------
1 file changed, 49 insertions(+), 16 deletions(-)
--
2.45.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/4] cpufreq: mediatek: CCI support SoC , the transition_delay set to 10 ms
2024-11-08 6:39 [PATCH v2 0/4] fixed mediatek-cpufreq has multi policy concurrency issue Mark Tseng
@ 2024-11-08 6:39 ` Mark Tseng
2024-11-14 10:22 ` AngeloGioacchino Del Regno
2024-11-08 6:39 ` [PATCH v2 2/4] cpufreq: mediatek: using global lock avoid race condition Mark Tseng
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Mark Tseng @ 2024-11-08 6:39 UTC (permalink / raw)
To: Rafael J . Wysocki, Viresh Kumar, MyungJoo Ham, Kyungmin Park,
Chanwoo Choi, Matthias Brugger, AngeloGioacchino Del Regno
Cc: linux-pm, linux-kernel, linux-arm-kernel, linux-mediatek,
Project_Global_Chrome_Upstream_Group, chun-jen.tseng
SoC with CCI architecture should set transition_delay to 10 ms because
cpufreq need to call devfreq notifier in async mode. if delay less than
10 ms, it may get wrong OPP-level in devfreq passive governor.
Signed-off-by: Mark Tseng <chun-jen.tseng@mediatek.com>
---
drivers/cpufreq/mediatek-cpufreq.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index 663f61565cf7..f63183154e9a 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -597,6 +597,9 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
policy->driver_data = info;
policy->clk = info->cpu_clk;
+ if (info->soc_data->ccifreq_supported)
+ policy->transition_delay_us = 10000;
+
return 0;
}
--
2.45.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/4] cpufreq: mediatek: using global lock avoid race condition
2024-11-08 6:39 [PATCH v2 0/4] fixed mediatek-cpufreq has multi policy concurrency issue Mark Tseng
2024-11-08 6:39 ` [PATCH v2 1/4] cpufreq: mediatek: CCI support SoC , the transition_delay set to 10 ms Mark Tseng
@ 2024-11-08 6:39 ` Mark Tseng
2024-11-08 6:39 ` [PATCH v2 3/4] cpufreq: mediatek: Add CPUFREQ_ASYNC_NOTIFICATION flag Mark Tseng
2024-11-08 6:39 ` [PATCH v2 4/4] cpufreq: mediatek: data safety protect Mark Tseng
3 siblings, 0 replies; 7+ messages in thread
From: Mark Tseng @ 2024-11-08 6:39 UTC (permalink / raw)
To: Rafael J . Wysocki, Viresh Kumar, MyungJoo Ham, Kyungmin Park,
Chanwoo Choi, Matthias Brugger, AngeloGioacchino Del Regno
Cc: linux-pm, linux-kernel, linux-arm-kernel, linux-mediatek,
Project_Global_Chrome_Upstream_Group, chun-jen.tseng
In mtk_cpufreq_set_target() is re-enter function but the mutex lock
decalre in mtk_cpu_dvfs_info structure for each policy. It should
change to global variable for critical session avoid race condition
with 2 or more policy.
add mtk_cpufreq_get() replace cpufreq_generic_get() and use global lock
avoid return wrong clock.
Signed-off-by: Mark Tseng <chun-jen.tseng@mediatek.com>
---
drivers/cpufreq/mediatek-cpufreq.c | 39 ++++++++++++++++++++++--------
1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index f63183154e9a..6b3cd2b803bf 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -49,8 +49,6 @@ struct mtk_cpu_dvfs_info {
bool need_voltage_tracking;
int vproc_on_boot;
int pre_vproc;
- /* Avoid race condition for regulators between notify and policy */
- struct mutex reg_lock;
struct notifier_block opp_nb;
unsigned int opp_cpu;
unsigned long current_freq;
@@ -59,6 +57,9 @@ struct mtk_cpu_dvfs_info {
bool ccifreq_bound;
};
+/* Avoid race condition for regulators between notify and policy */
+static DEFINE_MUTEX(mtk_policy_lock);
+
static struct platform_device *cpufreq_pdev;
static LIST_HEAD(dvfs_info_list);
@@ -209,12 +210,12 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
long freq_hz, pre_freq_hz;
int vproc, pre_vproc, inter_vproc, target_vproc, ret;
+ mutex_lock(&mtk_policy_lock);
+
inter_vproc = info->intermediate_voltage;
pre_freq_hz = clk_get_rate(cpu_clk);
- mutex_lock(&info->reg_lock);
-
if (unlikely(info->pre_vproc <= 0))
pre_vproc = regulator_get_voltage(info->proc_reg);
else
@@ -308,7 +309,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
info->current_freq = freq_hz;
out:
- mutex_unlock(&info->reg_lock);
+ mutex_unlock(&mtk_policy_lock);
return ret;
}
@@ -316,19 +317,20 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
static int mtk_cpufreq_opp_notifier(struct notifier_block *nb,
unsigned long event, void *data)
{
- struct dev_pm_opp *opp = data;
+ struct dev_pm_opp *opp;
struct dev_pm_opp *new_opp;
struct mtk_cpu_dvfs_info *info;
unsigned long freq, volt;
struct cpufreq_policy *policy;
int ret = 0;
+ mutex_lock(&mtk_policy_lock);
+ opp = data;
info = container_of(nb, struct mtk_cpu_dvfs_info, opp_nb);
if (event == OPP_EVENT_ADJUST_VOLTAGE) {
freq = dev_pm_opp_get_freq(opp);
- mutex_lock(&info->reg_lock);
if (info->current_freq == freq) {
volt = dev_pm_opp_get_voltage(opp);
ret = mtk_cpufreq_set_voltage(info, volt);
@@ -336,7 +338,6 @@ static int mtk_cpufreq_opp_notifier(struct notifier_block *nb,
dev_err(info->cpu_dev,
"failed to scale voltage: %d\n", ret);
}
- mutex_unlock(&info->reg_lock);
} else if (event == OPP_EVENT_DISABLE) {
freq = dev_pm_opp_get_freq(opp);
@@ -361,6 +362,7 @@ static int mtk_cpufreq_opp_notifier(struct notifier_block *nb,
}
}
}
+ mutex_unlock(&mtk_policy_lock);
return notifier_from_errno(ret);
}
@@ -495,7 +497,6 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
info->intermediate_voltage = dev_pm_opp_get_voltage(opp);
dev_pm_opp_put(opp);
- mutex_init(&info->reg_lock);
info->current_freq = clk_get_rate(info->cpu_clk);
info->opp_cpu = cpu;
@@ -610,13 +611,31 @@ static void mtk_cpufreq_exit(struct cpufreq_policy *policy)
dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table);
}
+static unsigned int mtk_cpufreq_get(unsigned int cpu)
+{
+ struct mtk_cpu_dvfs_info *info;
+ unsigned long current_freq;
+
+ mutex_lock(&mtk_policy_lock);
+ info = mtk_cpu_dvfs_info_lookup(cpu);
+ if (!info) {
+ mutex_unlock(&mtk_policy_lock);
+ return 0;
+ }
+
+ current_freq = info->current_freq / 1000;
+ mutex_unlock(&mtk_policy_lock);
+
+ return current_freq;
+}
+
static struct cpufreq_driver mtk_cpufreq_driver = {
.flags = CPUFREQ_NEED_INITIAL_FREQ_CHECK |
CPUFREQ_HAVE_GOVERNOR_PER_POLICY |
CPUFREQ_IS_COOLING_DEV,
.verify = cpufreq_generic_frequency_table_verify,
.target_index = mtk_cpufreq_set_target,
- .get = cpufreq_generic_get,
+ .get = mtk_cpufreq_get,
.init = mtk_cpufreq_init,
.exit = mtk_cpufreq_exit,
.register_em = cpufreq_register_em_with_opp,
--
2.45.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 3/4] cpufreq: mediatek: Add CPUFREQ_ASYNC_NOTIFICATION flag
2024-11-08 6:39 [PATCH v2 0/4] fixed mediatek-cpufreq has multi policy concurrency issue Mark Tseng
2024-11-08 6:39 ` [PATCH v2 1/4] cpufreq: mediatek: CCI support SoC , the transition_delay set to 10 ms Mark Tseng
2024-11-08 6:39 ` [PATCH v2 2/4] cpufreq: mediatek: using global lock avoid race condition Mark Tseng
@ 2024-11-08 6:39 ` Mark Tseng
2024-11-08 6:39 ` [PATCH v2 4/4] cpufreq: mediatek: data safety protect Mark Tseng
3 siblings, 0 replies; 7+ messages in thread
From: Mark Tseng @ 2024-11-08 6:39 UTC (permalink / raw)
To: Rafael J . Wysocki, Viresh Kumar, MyungJoo Ham, Kyungmin Park,
Chanwoo Choi, Matthias Brugger, AngeloGioacchino Del Regno
Cc: linux-pm, linux-kernel, linux-arm-kernel, linux-mediatek,
Project_Global_Chrome_Upstream_Group, chun-jen.tseng
Add CPUFREQ_ASYNC_NOTIFICATION flages for cpufreq policy because some of
process will get CPU frequency by cpufreq sysfs node. It may get wrong
frequency then call cpufreq_out_of_sync() to fixed frequency.
Signed-off-by: Mark Tseng <chun-jen.tseng@mediatek.com>
---
drivers/cpufreq/mediatek-cpufreq.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index 6b3cd2b803bf..3369bdd9a348 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -209,12 +209,16 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
struct dev_pm_opp *opp;
long freq_hz, pre_freq_hz;
int vproc, pre_vproc, inter_vproc, target_vproc, ret;
+ struct cpufreq_freqs freqs;
mutex_lock(&mtk_policy_lock);
inter_vproc = info->intermediate_voltage;
+ pre_freq_hz = policy->cur * 1000;
- pre_freq_hz = clk_get_rate(cpu_clk);
+ freqs.old = policy->cur;
+ freqs.new = freq_table[index].frequency;
+ cpufreq_freq_transition_begin(policy, &freqs);
if (unlikely(info->pre_vproc <= 0))
pre_vproc = regulator_get_voltage(info->proc_reg);
@@ -309,6 +313,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
info->current_freq = freq_hz;
out:
+ cpufreq_freq_transition_end(policy, &freqs, false);
mutex_unlock(&mtk_policy_lock);
return ret;
@@ -632,6 +637,7 @@ static unsigned int mtk_cpufreq_get(unsigned int cpu)
static struct cpufreq_driver mtk_cpufreq_driver = {
.flags = CPUFREQ_NEED_INITIAL_FREQ_CHECK |
CPUFREQ_HAVE_GOVERNOR_PER_POLICY |
+ CPUFREQ_ASYNC_NOTIFICATION |
CPUFREQ_IS_COOLING_DEV,
.verify = cpufreq_generic_frequency_table_verify,
.target_index = mtk_cpufreq_set_target,
--
2.45.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 4/4] cpufreq: mediatek: data safety protect
2024-11-08 6:39 [PATCH v2 0/4] fixed mediatek-cpufreq has multi policy concurrency issue Mark Tseng
` (2 preceding siblings ...)
2024-11-08 6:39 ` [PATCH v2 3/4] cpufreq: mediatek: Add CPUFREQ_ASYNC_NOTIFICATION flag Mark Tseng
@ 2024-11-08 6:39 ` Mark Tseng
3 siblings, 0 replies; 7+ messages in thread
From: Mark Tseng @ 2024-11-08 6:39 UTC (permalink / raw)
To: Rafael J . Wysocki, Viresh Kumar, MyungJoo Ham, Kyungmin Park,
Chanwoo Choi, Matthias Brugger, AngeloGioacchino Del Regno
Cc: linux-pm, linux-kernel, linux-arm-kernel, linux-mediatek,
Project_Global_Chrome_Upstream_Group, chun-jen.tseng
get policy data in global lock session avoid get wrong data.
Signed-off-by: Mark Tseng <chun-jen.tseng@mediatek.com>
---
drivers/cpufreq/mediatek-cpufreq.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index 3369bdd9a348..3303b6d72ea7 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -201,11 +201,11 @@ static bool is_ccifreq_ready(struct mtk_cpu_dvfs_info *info)
static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
unsigned int index)
{
- struct cpufreq_frequency_table *freq_table = policy->freq_table;
- struct clk *cpu_clk = policy->clk;
- struct clk *armpll = clk_get_parent(cpu_clk);
- struct mtk_cpu_dvfs_info *info = policy->driver_data;
- struct device *cpu_dev = info->cpu_dev;
+ struct cpufreq_frequency_table *freq_table;
+ struct clk *cpu_clk;
+ struct clk *armpll;
+ struct mtk_cpu_dvfs_info *info;
+ struct device *cpu_dev;
struct dev_pm_opp *opp;
long freq_hz, pre_freq_hz;
int vproc, pre_vproc, inter_vproc, target_vproc, ret;
@@ -213,6 +213,11 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
mutex_lock(&mtk_policy_lock);
+ freq_table = policy->freq_table;
+ cpu_clk = policy->clk;
+ armpll = clk_get_parent(cpu_clk);
+ info = policy->driver_data;
+ cpu_dev = info->cpu_dev;
inter_vproc = info->intermediate_voltage;
pre_freq_hz = policy->cur * 1000;
--
2.45.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/4] cpufreq: mediatek: CCI support SoC , the transition_delay set to 10 ms
2024-11-08 6:39 ` [PATCH v2 1/4] cpufreq: mediatek: CCI support SoC , the transition_delay set to 10 ms Mark Tseng
@ 2024-11-14 10:22 ` AngeloGioacchino Del Regno
2025-02-14 7:41 ` Chun-Jen Tseng (曾俊仁)
0 siblings, 1 reply; 7+ messages in thread
From: AngeloGioacchino Del Regno @ 2024-11-14 10:22 UTC (permalink / raw)
To: Mark Tseng, Rafael J . Wysocki, Viresh Kumar, MyungJoo Ham,
Kyungmin Park, Chanwoo Choi, Matthias Brugger
Cc: linux-pm, linux-kernel, linux-arm-kernel, linux-mediatek,
Project_Global_Chrome_Upstream_Group
Il 08/11/24 07:39, Mark Tseng ha scritto:
> SoC with CCI architecture should set transition_delay to 10 ms because
> cpufreq need to call devfreq notifier in async mode. if delay less than
> 10 ms, it may get wrong OPP-level in devfreq passive governor.
>
This means that MediaTek SoCs can change their CPU frequency once every
10 milliseconds?!?!?!
I don't think that's really the case.
Besides, are you aware that this will have a *huge* impact on either power
consumption or performance?
We're going from a bunch of microseconds to *multiple* milliseconds here.
Regards,
Angelo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/4] cpufreq: mediatek: CCI support SoC , the transition_delay set to 10 ms
2024-11-14 10:22 ` AngeloGioacchino Del Regno
@ 2025-02-14 7:41 ` Chun-Jen Tseng (曾俊仁)
0 siblings, 0 replies; 7+ messages in thread
From: Chun-Jen Tseng (曾俊仁) @ 2025-02-14 7:41 UTC (permalink / raw)
To: viresh.kumar@linaro.org, AngeloGioacchino Del Regno,
rafael@kernel.org, myungjoo.ham@samsung.com,
kyungmin.park@samsung.com, cw00.choi@samsung.com,
matthias.bgg@gmail.com
Cc: linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org, linux-pm@vger.kernel.org,
Project_Global_Chrome_Upstream_Group,
linux-kernel@vger.kernel.org
On Thu, 2024-11-14 at 11:22 +0100, AngeloGioacchino Del Regno wrote:
hi Angelo,
Thanks your review and recommendation.
I will fix this issue on next patch.
BRs,
Mark Tseng
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> Il 08/11/24 07:39, Mark Tseng ha scritto:
> > SoC with CCI architecture should set transition_delay to 10 ms
> > because
> > cpufreq need to call devfreq notifier in async mode. if delay less
> > than
> > 10 ms, it may get wrong OPP-level in devfreq passive governor.
> >
>
> This means that MediaTek SoCs can change their CPU frequency once
> every
> 10 milliseconds?!?!?!
>
> I don't think that's really the case.
>
> Besides, are you aware that this will have a *huge* impact on either
> power
> consumption or performance?
> We're going from a bunch of microseconds to *multiple* milliseconds
> here.
>
> Regards,
> Angelo
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-02-14 7:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-08 6:39 [PATCH v2 0/4] fixed mediatek-cpufreq has multi policy concurrency issue Mark Tseng
2024-11-08 6:39 ` [PATCH v2 1/4] cpufreq: mediatek: CCI support SoC , the transition_delay set to 10 ms Mark Tseng
2024-11-14 10:22 ` AngeloGioacchino Del Regno
2025-02-14 7:41 ` Chun-Jen Tseng (曾俊仁)
2024-11-08 6:39 ` [PATCH v2 2/4] cpufreq: mediatek: using global lock avoid race condition Mark Tseng
2024-11-08 6:39 ` [PATCH v2 3/4] cpufreq: mediatek: Add CPUFREQ_ASYNC_NOTIFICATION flag Mark Tseng
2024-11-08 6:39 ` [PATCH v2 4/4] cpufreq: mediatek: data safety protect Mark Tseng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.