| Message ID | 20230818020616.4748-1-chun-jen.tseng@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq: mediatek: change transition delay for MT8186 | expand |
On Mon, 2023-08-28 at 12:09 +0530, Viresh Kumar wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Hi Mark, > > I am not entirely clear by few things in the commit log. > > On 18-08-23, 10:06, Mark Tseng wrote: > > For MT8186, it has policy0 and policy6 by different governor > thread,so > > it may be call cpufreq->set_target_index() by different core. > > Why does this matter ? For MT8186 set freq must Big CPU -> Little CPU -> CCI like this order and it takes 10 ms. But in cpufreq & devfreq flow , when Big CPU or Little CPU change freq , it will call CCI setting by different policy. It will become Big CPU -> CCI setting or Little CPU -> CCI setting. Howevery, It will cause CCI setting to wrong value when per 1ms call governor and change freq. > > In general > > case, it must check BCPU, LCPU and CCI together then take about > 10ms. > > BCPU is Big CPU ? LCPU is Little CPU ? Yes, it is. > So are you saying that changing the frequency takes roughly 10 ms for > MT8186 ? > > > Atfer 44295af5019f this patch, it may call cpufreq_out_of_sync() by > > cpufreq_verify_current_freq() because current frequency is bigger > > than clk_get_rate() over 1 Mhz. By the same time, it may call > > s/ouver/over/ > s/1Mh/1 MHz/ > > > > cpufreq->set_target_index() again. > > Where was it called for the first time ? this patch (44295af5019f) , fixed cpufreq_out_of_sync() condition from 1000 Mhz to 1 Mhz. So, when cpufreq_verify_current_freq() call clk_get_rate() over 1 Mhz, it must to do freq sync by cpufreq_out_of_sync(). In MT8186, it offen over 1 Mhz when call clk_get_rate(), so I modify transition delay from 1 ms to 10 ms for make sure freq setting done. > > So, the CCI freq may be too lower for > > BCPU cause BCPU kernel panic. > > I am not sure how a low frequency causes kernel panic here. In MT8186, if CCI freq is lower than Bit CPU freq 70%, it will causes kernel panic on Big CPU. > > So, it should change the default transition delay 1ms to 10ms. It > can > > promise the next freq setting then governor trigger new freq > change. > > There are few typos as well here, please fix them. > > > Fixes: 44295af5019f ("cpufreq: use correct unit when verify cur > freq") > > I think you should drop this. The issue at hand may be visible now > after 44295af5019f is applied, but it certainly didn't cause it. > > -- > viresh >
On 29-08-23, 14:01, Viresh Kumar wrote:
> Why exactly does the kernel crash here ? Any idea ?
Also note that cpufreq core has enough locking in place to make sure
two ->target_index() function calls don't run in parallel for the same
policy.
What may be happening in your case is that you are configuring a
common entity (CCI) from both the policies and there is no locking in
place to take care of the races.
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c index a0a61919bc4c..5633a5357e8f 100644 --- a/drivers/cpufreq/mediatek-cpufreq.c +++ b/drivers/cpufreq/mediatek-cpufreq.c @@ -23,6 +23,7 @@ struct mtk_cpufreq_platform_data { int sram_min_volt; int sram_max_volt; bool ccifreq_supported; + unsigned int transition_delay_us; }; /* @@ -595,6 +596,7 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy) policy->freq_table = freq_table; policy->driver_data = info; policy->clk = info->cpu_clk; + policy->transition_delay_us = info->soc_data->transition_delay_us; return 0; } @@ -689,6 +691,7 @@ static const struct mtk_cpufreq_platform_data mt2701_platform_data = { .sram_min_volt = 0, .sram_max_volt = 1150000, .ccifreq_supported = false, + .transition_delay_us = 1000, }; static const struct mtk_cpufreq_platform_data mt7622_platform_data = { @@ -698,6 +701,7 @@ static const struct mtk_cpufreq_platform_data mt7622_platform_data = { .sram_min_volt = 0, .sram_max_volt = 1350000, .ccifreq_supported = false, + .transition_delay_us = 1000, }; static const struct mtk_cpufreq_platform_data mt7623_platform_data = { @@ -705,6 +709,7 @@ static const struct mtk_cpufreq_platform_data mt7623_platform_data = { .max_volt_shift = 200000, .proc_max_volt = 1300000, .ccifreq_supported = false, + .transition_delay_us = 1000, }; static const struct mtk_cpufreq_platform_data mt8183_platform_data = { @@ -714,6 +719,7 @@ static const struct mtk_cpufreq_platform_data mt8183_platform_data = { .sram_min_volt = 0, .sram_max_volt = 1150000, .ccifreq_supported = true, + .transition_delay_us = 1000, }; static const struct mtk_cpufreq_platform_data mt8186_platform_data = { @@ -723,6 +729,7 @@ static const struct mtk_cpufreq_platform_data mt8186_platform_data = { .sram_min_volt = 850000, .sram_max_volt = 1118750, .ccifreq_supported = true, + .transition_delay_us = 10000, }; static const struct mtk_cpufreq_platform_data mt8516_platform_data = { @@ -732,6 +739,7 @@ static const struct mtk_cpufreq_platform_data mt8516_platform_data = { .sram_min_volt = 0, .sram_max_volt = 1310000, .ccifreq_supported = false, + .transition_delay_us = 1000, }; /* List of machines supported by this driver */
For MT8186, it has policy0 and policy6 by different governor thread,so it may be call cpufreq->set_target_index() by different core. In general case, it must check BCPU, LCPU and CCI together then take about 10ms. Atfer 44295af5019f this patch, it may call cpufreq_out_of_sync() by cpufreq_verify_current_freq() because current frequency is bigger than clk_get_rate() ouver 1Mh. By the same time, it may call cpufreq->set_target_index() again. So, the CCI freq may be too lower for BCPU cause BCPU kernel panic. So, it should change the default transition delay 1ms to 10ms. It can promise the next freq setting then governor trigger new freq change. Fixes: 44295af5019f ("cpufreq: use correct unit when verify cur freq") Signed-off-by: Mark Tseng <chun-jen.tseng@mediatek.com> --- drivers/cpufreq/mediatek-cpufreq.c | 8 ++++++++ 1 file changed, 8 insertions(+)