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

[Dmitry Osipenko]

Ð 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?

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