[v4,05/16] PM / devfreq: tegra: Don't set EMC clock rate to maximum on probe
diff mbox series

Message ID 20190501233815.32643-6-digetx@gmail.com
State New
Headers show
Series
  • NVIDIA Tegra devfreq improvements and Tegra20/30 support
Related show

Commit Message

Dmitry Osipenko May 1, 2019, 11:38 p.m. UTC
There is no real benefit from doing so, hence let's drop that rate setting
for consistency.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra-devfreq.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Thierry Reding June 4, 2019, 11 a.m. UTC | #1
On Thu, May 02, 2019 at 02:38:04AM +0300, Dmitry Osipenko wrote:
> There is no real benefit from doing so, hence let's drop that rate setting
> for consistency.
> 
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra-devfreq.c | 2 --
>  1 file changed, 2 deletions(-)

Do you have any numbers to tell how long it would take for the EMC rate
to get incremented? My understanding is that ACTMON basically reacts to
system load, so I could imagine that not setting to the maximum
frequency after this is loaded might make the system slow in the short
term. Only after ACTMON has collected enough data to determine that it
needs to clock the EMC higher would system speed resume normal.

I guess technically this patch doesn't change anything if the system
already boots at the highest EMC frequency anyway, which I think it does
on many (although not all) devices.

Anyway, you said you've tested this and are satisfied with the
performance, so it can't be that bad:

Acked-by: Thierry Reding <treding@nvidia.com>

> 
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index c7428c5eee23..24ec65556c39 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -653,8 +653,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  		return PTR_ERR(tegra->emc_clock);
>  	}
>  
> -	clk_set_rate(tegra->emc_clock, ULONG_MAX);
> -
>  	tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
>  	err = clk_notifier_register(tegra->emc_clock, &tegra->rate_change_nb);
>  	if (err) {
> -- 
> 2.21.0
>
Dmitry Osipenko June 4, 2019, 1:05 p.m. UTC | #2
04.06.2019 14:00, Thierry Reding пишет:
> On Thu, May 02, 2019 at 02:38:04AM +0300, Dmitry Osipenko wrote:
>> There is no real benefit from doing so, hence let's drop that rate setting
>> for consistency.
>>
>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/tegra-devfreq.c | 2 --
>>  1 file changed, 2 deletions(-)
> 
> Do you have any numbers to tell how long it would take for the EMC rate
> to get incremented? My understanding is that ACTMON basically reacts to
> system load, so I could imagine that not setting to the maximum
> frequency after this is loaded might make the system slow in the short
> term. Only after ACTMON has collected enough data to determine that it
> needs to clock the EMC higher would system speed resume normal.
> 
> I guess technically this patch doesn't change anything if the system
> already boots at the highest EMC frequency anyway, which I think it does
> on many (although not all) devices.
> 
> Anyway, you said you've tested this and are satisfied with the
> performance, so it can't be that bad:
> 
> Acked-by: Thierry Reding <treding@nvidia.com>

It takes 12ms for ACTMON to react and then about (couple) hundred
microseconds to change memory freq. This is a very short period of time
that human being can't notice.

AFAIK, in practice there are no devices in the wild that boot up with
DRAM clocked at lowest rate. Most devices have a video output and thus
require significant memory bandwidth at a boot time already. Secondly,
higher memory bandwidth is only really needed for a cases like
multimedia, in most generic cases CPU is hitting cache and not utilizing
DRAM a lot.

Patch
diff mbox series

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index c7428c5eee23..24ec65556c39 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -653,8 +653,6 @@  static int tegra_devfreq_probe(struct platform_device *pdev)
 		return PTR_ERR(tegra->emc_clock);
 	}
 
-	clk_set_rate(tegra->emc_clock, ULONG_MAX);
-
 	tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
 	err = clk_notifier_register(tegra->emc_clock, &tegra->rate_change_nb);
 	if (err) {