Re: [PATCH v4 21/24] PM / devfreq: tegra30: Synchronize average count on target's update

[Chanwoo Choi]

On 19. 7. 19. ìì 9:31, Dmitry Osipenko wrote:
> Ð Thu, 18 Jul 2019 19:15:54 +0900
> Chanwoo Choi <cw00.choi@xxxxxxxxxxx> ÐÐÑÐÑ:
> 
>> On 19. 7. 8. ìì 7:33, Dmitry Osipenko wrote:
>>> The average count may get out of sync if interrupt was disabled /
>>> avoided for a long time due to upper watermark optimization, hence
>>> it should be re-synced on each target's update to ensure that
>>> watermarks are set up correctly on EMC rate-change notification and
>>> that a correct frequency is selected for device.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>>> ---
>>>  drivers/devfreq/tegra30-devfreq.c | 30
>>> ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)
>>>
>>> diff --git a/drivers/devfreq/tegra30-devfreq.c
>>> b/drivers/devfreq/tegra30-devfreq.c index
>>> 4d582809acb6..8a674fad26be 100644 ---
>>> a/drivers/devfreq/tegra30-devfreq.c +++
>>> b/drivers/devfreq/tegra30-devfreq.c @@ -466,11 +466,41 @@ static
>>> void actmon_isr_device(struct tegra_devfreq *tegra,
>>> dev->boost_freq, cpufreq_get(0)); }
>>>  
>>> +static void tegra_devfreq_sync_avg_count(struct tegra_devfreq
>>> *tegra,
>>> +					 struct
>>> tegra_devfreq_device *dev) +{
>>> +	u32 avg_count, avg_freq, old_upper, new_upper;
>>> +
>>> +	avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
>>> +	avg_freq = avg_count / ACTMON_SAMPLING_PERIOD;
>>> +
>>> +	old_upper = tegra_actmon_upper_freq(tegra, dev->avg_freq);
>>> +	new_upper = tegra_actmon_upper_freq(tegra, avg_freq);
>>> +
>>> +	/* similar to ISR, see comments in actmon_isr_device() */
>>> +	if (old_upper != new_upper) {
>>> +		dev->avg_freq = avg_freq;
>>> +		dev->boost_freq = 0;
>>> +	}
>>> +}
>>> +
>>>  static unsigned long actmon_update_target(struct tegra_devfreq
>>> *tegra, struct tegra_devfreq_device *dev)
>>>  {
>>>  	unsigned long target_freq;
>>>  
>>> +	/*
>>> +	 * The avg_count / avg_freq is getting snapshoted on
>>> device's
>>> +	 * interrupt, but there are cases where actual value need
>>> to
>>> +	 * be utilized on target's update, like CPUFreq boosting
>>> and
>>> +	 * overriding the min freq
>>> via /sys/class/devfreq/devfreq0/min_freq
>>> +	 * because we're optimizing the upper watermark based on
>>> the
>>> +	 * actual EMC frequency. This means that interrupt may be
>>> +	 * inactive for a long time and thus making snapshoted
>>> value
>>> +	 * outdated.
>>> +	 */
>>> +	tegra_devfreq_sync_avg_count(tegra, dev);  
>>
>> I think that you don't need to add the separate function to calculate
>> the 'dev->avg_freq'. It is enough with your detailed comment to add
>> this code in this function.
> 
> The separate function is indeed not mandatory here, but I'm finding that
> it usually makes easier to read and follow the code when it is properly
> split up into logical blocks. Don't you agree?

It is right to make the separate function if function is too long or 
function is called on the multiple points.

But, in this case, I think that it is enough to add this code
to the actmon_update_target() because you already added the detailed comment. 

It is enough to understand the role of this code with your comment.

> 
>>> +
>>>  	target_freq = min(dev->avg_freq + dev->boost_freq,
>>> KHZ_MAX); target_freq = tegra_actmon_account_cpu_freq(tegra, dev,
>>> target_freq); 
>>>   
>>
>> And also, is it impossible to squash this patch with patch19/patch20?
>>
> 
> It should be possible to squash this patch with #20, but wouldn't
> be better to keep changes in the chronological order? It's also better
> to keep changes separate simply to aid bisection in case of a problem.
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics