From: Chanwoo Choi <cw00.choi@samsung.com>
To: Dmitry Osipenko <digetx@gmail.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 09:02:30 +0900 [thread overview]
Message-ID: <2f7a0283-7433-b786-a8c3-7d711aafd721@samsung.com> (raw)
In-Reply-To: <20190811212315.12689-12-digetx@gmail.com>
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);
Except for this, looks good to me.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
(snip)
--
Best Regards,
Chanwoo Choi
Samsung Electronics
next prev parent reply other threads:[~2019-10-01 23:57 UTC|newest]
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 [this message]
2019-10-02 14:06 ` Dmitry Osipenko
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 publicly 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=2f7a0283-7433-b786-a8c3-7d711aafd721@samsung.com \
--to=cw00.choi@samsung.com \
--cc=digetx@gmail.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).