[v4,21/24] PM / devfreq: tegra30: Synchronize average count on target's update
diff mbox series

Message ID 20190707223303.6755-22-digetx@gmail.com
State New
Headers show
Series
  • More improvements for Tegra30 devfreq driver
Related show

Commit Message

Dmitry Osipenko July 7, 2019, 10:33 p.m. UTC
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@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

Comments

Chanwoo Choi July 18, 2019, 10:15 a.m. UTC | #1
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@gmail.com>
> ---
>  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.

> +
>  	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?
Dmitry Osipenko July 19, 2019, 12:31 a.m. UTC | #2
В Thu, 18 Jul 2019 19:15:54 +0900
Chanwoo Choi <cw00.choi@samsung.com> пишет:

> 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@gmail.com>
> > ---
> >  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.
Chanwoo Choi July 19, 2019, 1:40 a.m. UTC | #3
On 19. 7. 19. 오전 9:31, Dmitry Osipenko wrote:
> В Thu, 18 Jul 2019 19:15:54 +0900
> Chanwoo Choi <cw00.choi@samsung.com> пишет:
> 
>> 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@gmail.com>
>>> ---
>>>  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.
> 
> 
>
Dmitry Osipenko July 19, 2019, 4:46 p.m. UTC | #4
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@samsung.com> пишет:
>>
>>> 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@gmail.com>
>>>> ---
>>>>  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.
>>
>>
>>
> 
>

Patch
diff mbox series

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);
+
 	target_freq = min(dev->avg_freq + dev->boost_freq, KHZ_MAX);
 	target_freq = tegra_actmon_account_cpu_freq(tegra, dev, target_freq);