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

[Dmitry Osipenko]

19.07.2019 4:40, 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.

That's another personal preference, but I'm fine with yours variant as
well. Will change this in the next version.

>>
>>>> +
>>>>  	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.
>>
>>
>>
> 
>