Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: Dmitry Osipenko <digetx@gmail.com>
To: Chanwoo Choi <cw00.choi@samsung.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	MyungJoo Ham <myungjoo.ham@samsung.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: linux-pm@vger.kernel.org, linux-tegra@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 11/19] PM / devfreq: tegra30: Use CPUFreq notifier
Date: Wed, 2 Oct 2019 17:06:24 +0300
Message-ID: <57b2f772-8849-5180-262f-0ef8936e63bc@gmail.com> (raw)
In-Reply-To: <2f7a0283-7433-b786-a8c3-7d711aafd721@samsung.com>

02.10.2019 03:02, Chanwoo Choi пишет:
> Hi,
> 
> On 19. 8. 12. 오전 6:23, Dmitry Osipenko wrote:
>> The CPU's client need to take into account that CPUFreq may change
>> while memory activity not, staying high. Thus an appropriate frequency
>> notifier should be used in addition to the clk-notifier.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/tegra30-devfreq.c | 173 ++++++++++++++++++++++++++----
>>  1 file changed, 153 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>> index a2623de56d20..a260812f7744 100644
>> --- a/drivers/devfreq/tegra30-devfreq.c
>> +++ b/drivers/devfreq/tegra30-devfreq.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/pm_opp.h>
>>  #include <linux/reset.h>
>> +#include <linux/workqueue.h>
>>  
>>  #include "governor.h"
>>  
>> @@ -34,6 +35,8 @@
>>  #define ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN		BIT(30)
>>  #define ACTMON_DEV_CTRL_ENB					BIT(31)
>>  
>> +#define ACTMON_DEV_CTRL_STOP					0x00000000
>> +
>>  #define ACTMON_DEV_UPPER_WMARK					0x4
>>  #define ACTMON_DEV_LOWER_WMARK					0x8
>>  #define ACTMON_DEV_INIT_AVG					0xc
>> @@ -159,7 +162,10 @@ struct tegra_devfreq {
>>  
>>  	struct clk		*emc_clock;
>>  	unsigned long		max_freq;
>> -	struct notifier_block	rate_change_nb;
>> +	struct notifier_block	clk_rate_change_nb;
>> +
>> +	struct delayed_work	cpufreq_update_work;
>> +	struct notifier_block	cpu_rate_change_nb;
>>  
>>  	struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
>>  
>> @@ -207,10 +213,10 @@ static unsigned long do_percent(unsigned long val, unsigned int pct)
>>  	return val * pct / 100;
>>  }
>>  
>> -static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra)
>> +static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra,
>> +					    unsigned int cpu_freq)
>>  {
>>  	struct tegra_actmon_emc_ratio *ratio = actmon_emc_ratios;
>> -	unsigned int cpu_freq = cpufreq_quick_get(0);
>>  	unsigned int i;
>>  
>>  	for (i = 0; i < ARRAY_SIZE(actmon_emc_ratios); i++, ratio++) {
>> @@ -244,7 +250,8 @@ tegra_actmon_account_cpu_freq(struct tegra_devfreq *tegra,
>>  		return target_freq;
>>  
>>  	if (dev->avg_freq > tegra_actmon_dev_avg_dependency_freq(tegra, dev))
>> -		static_cpu_emc_freq = actmon_cpu_to_emc_rate(tegra);
>> +		static_cpu_emc_freq = actmon_cpu_to_emc_rate(
>> +						tegra, cpufreq_quick_get(0));
>>  	else
>>  		static_cpu_emc_freq = 0;
>>  
>> @@ -543,8 +550,8 @@ static irqreturn_t actmon_thread_isr(int irq, void *data)
>>  	return handled ? IRQ_HANDLED : IRQ_NONE;
>>  }
>>  
>> -static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
>> -				       unsigned long action, void *ptr)
>> +static int tegra_actmon_clk_notify_cb(struct notifier_block *nb,
>> +				      unsigned long action, void *ptr)
>>  {
>>  	struct clk_notifier_data *data = ptr;
>>  	struct tegra_devfreq_device *dev;
>> @@ -555,7 +562,7 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
>>  	if (action != POST_RATE_CHANGE)
>>  		return NOTIFY_OK;
>>  
>> -	tegra = container_of(nb, struct tegra_devfreq, rate_change_nb);
>> +	tegra = container_of(nb, struct tegra_devfreq, clk_rate_change_nb);
>>  
>>  	freq = data->new_rate / KHZ;
>>  
>> @@ -586,6 +593,94 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
>>  	return NOTIFY_OK;
>>  }
>>  
>> +static void tegra_actmon_delayed_update(struct work_struct *work)
>> +{
>> +	struct tegra_devfreq *tegra = container_of(work, struct tegra_devfreq,
>> +						   cpufreq_update_work.work);
>> +
>> +	mutex_lock(&tegra->devfreq->lock);
>> +	update_devfreq(tegra->devfreq);
>> +	mutex_unlock(&tegra->devfreq->lock);
>> +}
>> +
>> +static unsigned long
>> +tegra_actmon_cpufreq_contribution(struct tegra_devfreq *tegra,
>> +				  unsigned int cpu_freq)
>> +{
>> +	unsigned long freq, static_cpu_emc_freq;
>> +
>> +	/* check whether CPU's freq is taken into account at all */
>> +	freq = tegra_actmon_dev_avg_dependency_freq(tegra,
>> +						    &tegra->devices[MCCPU]);
>> +	if (tegra->devices[MCCPU].avg_freq <= freq)
>> +		return 0;
>> +
>> +	static_cpu_emc_freq = actmon_cpu_to_emc_rate(tegra, cpu_freq);
>> +
>> +	/* compare static CPU-EMC freq with MCALL */
>> +	freq = tegra->devices[MCALL].avg_freq +
>> +	       tegra->devices[MCALL].boost_freq;
>> +
>> +	freq = tegra_actmon_upper_freq(tegra, freq);
>> +
>> +	if (freq == tegra->max_freq || freq >= static_cpu_emc_freq)
>> +		return 0;
>> +
>> +	/* compare static CPU-EMC freq with MCCPU */
>> +	freq = tegra->devices[MCCPU].avg_freq +
>> +	       tegra->devices[MCCPU].boost_freq;
>> +
>> +	freq = tegra_actmon_upper_freq(tegra, freq);
>> +
>> +	if (freq == tegra->max_freq || freq >= static_cpu_emc_freq)
>> +		return 0;
>> +
>> +	return static_cpu_emc_freq;
>> +}
>> +
>> +static int tegra_actmon_cpu_notify_cb(struct notifier_block *nb,
>> +				      unsigned long action, void *ptr)
>> +{
>> +	struct cpufreq_freqs *freqs = ptr;
>> +	struct tegra_devfreq *tegra;
>> +	unsigned long old, new, delay;
>> +
>> +	if (action != CPUFREQ_POSTCHANGE)
>> +		return NOTIFY_OK;
>> +
>> +	tegra = container_of(nb, struct tegra_devfreq, cpu_rate_change_nb);
>> +
>> +	/*
>> +	 * Quickly check whether CPU frequency should be taken into account
>> +	 * at all, without blocking CPUFreq's core.
>> +	 */
>> +	if (mutex_trylock(&tegra->devfreq->lock)) {
>> +		old = tegra_actmon_cpufreq_contribution(tegra, freqs->old);
>> +		new = tegra_actmon_cpufreq_contribution(tegra, freqs->new);
>> +		mutex_unlock(&tegra->devfreq->lock);
>> +
>> +		/*
>> +		 * If CPU's frequency shouldn't be taken into account at
>> +		 * the moment, then there is no need to update the devfreq's
>> +		 * state because ISR will re-check CPU's frequency on the
>> +		 * next interrupt.
>> +		 */
>> +		if (old == new)
>> +			return NOTIFY_OK;
>> +	}
>> +
>> +	/*
>> +	 * CPUFreq driver should support CPUFREQ_ASYNC_NOTIFICATION in order
>> +	 * to allow asynchronous notifications. This means we can't block
>> +	 * here for too long, otherwise CPUFreq's core will complain with a
>> +	 * warning splat.
>> +	 */
>> +	delay = msecs_to_jiffies(ACTMON_SAMPLING_PERIOD);
>> +	schedule_delayed_work(&tegra->cpufreq_update_work, delay);
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
>>  static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
>>  					  struct tegra_devfreq_device *dev)
>>  {
>> @@ -617,9 +712,16 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
>>  	device_writel(dev, val, ACTMON_DEV_CTRL);
>>  }
>>  
>> -static void tegra_actmon_start(struct tegra_devfreq *tegra)
>> +static void tegra_actmon_stop_device(struct tegra_devfreq_device *dev)
>> +{
>> +	device_writel(dev, ACTMON_DEV_CTRL_STOP, ACTMON_DEV_CTRL);
>> +	device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
>> +}
>> +
>> +static int tegra_actmon_start(struct tegra_devfreq *tegra)
>>  {
>>  	unsigned int i;
>> +	int err;
>>  
>>  	actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
>>  		      ACTMON_GLB_PERIOD_CTRL);
>> @@ -627,7 +729,30 @@ static void tegra_actmon_start(struct tegra_devfreq *tegra)
>>  	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
>>  		tegra_actmon_configure_device(tegra, &tegra->devices[i]);
>>  
>> +	/*
>> +	 * We are estimating CPU's memory bandwidth requirement based on
>> +	 * amount of memory accesses and system's load, judging by CPU's
>> +	 * frequency. We also don't want to receive events about CPU's
>> +	 * frequency transaction when governor is stopped, hence notifier
>> +	 * is registered dynamically.
>> +	 */
>> +	err = cpufreq_register_notifier(&tegra->cpu_rate_change_nb,
>> +					CPUFREQ_TRANSITION_NOTIFIER);
>> +	if (err) {
>> +		dev_err(tegra->devfreq->dev.parent,
>> +			"Failed to register rate change notifier: %d\n", err);
>> +		goto err_stop;
>> +	}
>> +
>>  	enable_irq(tegra->irq);
>> +
>> +	return 0;
>> +
>> +err_stop:
>> +	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
>> +		tegra_actmon_stop_device(&tegra->devices[i]);
> 
> nitpick:
> Why don't you contain this for loop statement in the tegra_actmon_stop_device()?
> I think that you can make it as following:
> 
> 	tegra_actmon_stop_device(struct tegra_devfreq *target);
> 		for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)> +				
> 			device_writel(dev, ACTMON_DEV_CTRL_STOP, ACTMON_DEV_CTRL);              
> 			device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS); 

I'll change that in v7.

> Except for this, looks good to me. 
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Thanks!

  reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-11 21:22 [PATCH v6 00/19] More improvements for Tegra30 devfreq driver Dmitry Osipenko
2019-08-11 21:22 ` [PATCH v6 01/19] PM / devfreq: tegra30: Change irq type to unsigned int Dmitry Osipenko
2019-08-11 21:22 ` [PATCH v6 02/19] PM / devfreq: tegra30: Keep interrupt disabled while governor is stopped Dmitry Osipenko
2019-08-20  0:02   ` Chanwoo Choi
2019-08-11 21:22 ` [PATCH v6 03/19] PM / devfreq: tegra30: Handle possible round-rate error Dmitry Osipenko
2019-08-20  0:06   ` Chanwoo Choi
2019-08-11 21:23 ` [PATCH v6 04/19] PM / devfreq: tegra30: Drop write-barrier Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 05/19] PM / devfreq: tegra30: Set up watermarks properly Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 06/19] PM / devfreq: tegra30: Tune up boosting thresholds Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 07/19] PM / devfreq: tegra30: Fix integer overflow on CPU's freq max out Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 08/19] PM / devfreq: tegra30: Ensure that target freq won't overflow Dmitry Osipenko
2019-08-20  0:23   ` Chanwoo Choi
2019-08-20 23:19     ` Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 09/19] PM / devfreq: tegra30: Use kHz units uniformly in the code Dmitry Osipenko
2019-10-01 23:29   ` Chanwoo Choi
2019-10-02 18:26     ` Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 10/19] PM / devfreq: tegra30: Reduce unnecessary interrupts activity Dmitry Osipenko
2019-10-01 23:35   ` Chanwoo Choi
2019-10-02 18:40     ` Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 11/19] PM / devfreq: tegra30: Use CPUFreq notifier Dmitry Osipenko
2019-10-02  0:02   ` Chanwoo Choi
2019-10-02 14:06     ` Dmitry Osipenko [this message]
2019-08-11 21:23 ` [PATCH v6 12/19] PM / devfreq: tegra30: Move clk-notifier's registration to governor's start Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 13/19] PM / devfreq: tegra30: Reset boosting on startup Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 14/19] PM / devfreq: tegra30: Don't enable consecutive-down interrupt " Dmitry Osipenko
2019-10-02  0:04   ` Chanwoo Choi
2019-08-11 21:23 ` [PATCH v6 15/19] PM / devfreq: tegra30: Constify structs Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 16/19] PM / devfreq: tegra30: Include appropriate header Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 17/19] PM / devfreq: tegra30: Increase sampling period to 16ms Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 18/19] PM / devfreq: tegra30: Support variable polling interval Dmitry Osipenko
2019-10-02  0:18   ` Chanwoo Choi
2019-10-02 15:27     ` Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 19/19] PM / devfreq: tegra20/30: Add Dmitry as a maintainer Dmitry Osipenko
2019-10-01 21:15 ` [PATCH v6 00/19] More improvements for Tegra30 devfreq driver Dmitry Osipenko
2019-10-02  0:25   ` Chanwoo Choi
2019-10-02 13:54     ` Dmitry Osipenko
2019-10-05 16:29       ` Peter Geis

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=57b2f772-8849-5180-262f-0ef8936e63bc@gmail.com \
    --to=digetx@gmail.com \
    --cc=cw00.choi@samsung.com \
    --cc=jonathanh@nvidia.com \
    --cc=kyungmin.park@samsung.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=myungjoo.ham@samsung.com \
    --cc=thierry.reding@gmail.com \
    --cc=tomeu.vizoso@collabora.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git