[PATCH v6 00/19] More improvements for Tegra30 devfreq driver

[PATCH v6 00/19] More improvements for Tegra30 devfreq driver

[Dmitry Osipenko]

Hello,

This series addresses some additional review comments that were made by
Thierry Reding to [1], makes several important changes to the driver,
fixing excessive interrupts activity, and adds new features. In the end
I'm proposing myself as a maintainer for the Tegra devfreq drivers.

[1] https://lore.kernel.org/lkml/0fb50eb1-a173-1756-6889-2526a10ac707@gmail.com/T/

Changelog:

v6:  Addressed review comment that was made by Chanwoo Choi to v5 by
     squashing "Define ACTMON_DEV_CTRL_STOP" patch into the "Use CPUFreq
     notifier" patch.

v5:  Addressed review comments that were made by Chanwoo Choi to v4 by
     squashing few patches, dropping some questionable patches, rewording
     comments to the code, restructuring the code and etc.

     These patches are now dropped from the series:

       PM / devfreq: tegra30: Use tracepoints for debugging
       PM / devfreq: tegra30: Inline all one-line functions

     The interrupt-optimization patches are squashed into a single patch:

       PM / devfreq: tegra30: Reduce unnecessary interrupts activity

     because it's better to keep the optimizations as a separate change and
     this also helps to reduce code churning, since the code changes depend
     on a previous patch in order to stay cleaner.

     Fixed a lockup bug that I spotted recently, which is caused by a
     clk-notifier->cpufreq_get()->clk_set_rate() sequence. Now a non-blocking
     variant of CPU's frequency retrieving is used, i.e. cpufreq_quick_get().

     Further optimized the CPUFreq notifier by postponing the delayed
     updating in accordance to the polling interval, this actually uncovered
     the above lockup bug.

     Implemented new minor driver feature in the new patch:

       PM / devfreq: tegra30: Support variable polling interval

v4:  Added two new patches to the series:

       PM / devfreq: tegra30: Synchronize average count on target's update
       PM / devfreq: tegra30: Increase sampling period to 16ms

     The first patch addresses problem where governor could get stuck due
     to outdated "average count" value which is snapshoted by ISR and there
     are cases where manual update of the value is required.

     The second patch is just a minor optimization.

v3:  Added support for tracepoints, replacing the debug messages.
     Fixed few more bugs with the help of tracepoints.

     New patches in this version:

       PM / devfreq: tegra30: Use tracepoints for debugging
       PM / devfreq: tegra30: Optimize CPUFreq notifier
       PM / devfreq: tegra30: Optimize upper consecutive watermark selection
       PM / devfreq: tegra30: Optimize upper average watermark selection
       PM / devfreq: tegra30: Include appropriate header

     Some of older patches of this series also got some extra minor polish.

v2:  Added more patches that are cleaning driver's code further and
     squashing another kHz conversion bug.

     The patch "Rework frequency management logic" of the v1 series is now
     converted to "Set up watermarks properly" because I found some problems
     in the original patch and then realized that there is no need to change
     the logic much. So the logic mostly preserved and only got improvements.

     The series is based on the today's linux-next (25 Jun) and takes into
     account minor changes that MyungJoo Ham made to the already queued
     patches from the first batch [1].

Dmitry Osipenko (19):
  PM / devfreq: tegra30: Change irq type to unsigned int
  PM / devfreq: tegra30: Keep interrupt disabled while governor is
    stopped
  PM / devfreq: tegra30: Handle possible round-rate error
  PM / devfreq: tegra30: Drop write-barrier
  PM / devfreq: tegra30: Set up watermarks properly
  PM / devfreq: tegra30: Tune up boosting thresholds
  PM / devfreq: tegra30: Fix integer overflow on CPU's freq max out
  PM / devfreq: tegra30: Ensure that target freq won't overflow
  PM / devfreq: tegra30: Use kHz units uniformly in the code
  PM / devfreq: tegra30: Reduce unnecessary interrupts activity
  PM / devfreq: tegra30: Use CPUFreq notifier
  PM / devfreq: tegra30: Move clk-notifier's registration to governor's
    start
  PM / devfreq: tegra30: Reset boosting on startup
  PM / devfreq: tegra30: Don't enable consecutive-down interrupt on
    startup
  PM / devfreq: tegra30: Constify structs
  PM / devfreq: tegra30: Include appropriate header
  PM / devfreq: tegra30: Increase sampling period to 16ms
  PM / devfreq: tegra30: Support variable polling interval
  PM / devfreq: tegra20/30: Add Dmitry as a maintainer

 MAINTAINERS                       |   9 +
 drivers/devfreq/tegra30-devfreq.c | 706 +++++++++++++++++++++++-------
 2 files changed, 555 insertions(+), 160 deletions(-)

-- 
2.22.0

[PATCH v6 01/19] PM / devfreq: tegra30: Change irq type to unsigned int

[Dmitry Osipenko]

IRQ numbers are always positive, hence the corresponding variable should
be unsigned to keep types consistent. This is a minor change that cleans
up code a tad more.

Suggested-by: Thierry Reding <thierry.reding@gmail.com>
Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index a6ba75f4106d..a27300f40b0b 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -160,7 +160,7 @@ struct tegra_devfreq {
 
 	struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
 
-	int irq;
+	unsigned int		irq;
 };
 
 struct tegra_actmon_emc_ratio {
@@ -618,12 +618,12 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 		return PTR_ERR(tegra->emc_clock);
 	}
 
-	tegra->irq = platform_get_irq(pdev, 0);
-	if (tegra->irq < 0) {
-		err = tegra->irq;
+	err = platform_get_irq(pdev, 0);
+	if (err < 0) {
 		dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err);
 		return err;
 	}
+	tegra->irq = err;
 
 	reset_control_assert(tegra->reset);
 
-- 
2.22.0

[PATCH v6 02/19] PM / devfreq: tegra30: Keep interrupt disabled while governor is stopped

[Dmitry Osipenko]

There is no real need to keep interrupt always-enabled, will be nicer
to keep it disabled while governor is inactive.

Suggested-by: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 47 ++++++++++++++++---------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index a27300f40b0b..8be6a33beb9c 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -11,6 +11,7 @@
 #include <linux/devfreq.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/irq.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
@@ -416,8 +417,6 @@ static void tegra_actmon_start(struct tegra_devfreq *tegra)
 {
 	unsigned int i;
 
-	disable_irq(tegra->irq);
-
 	actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
 		      ACTMON_GLB_PERIOD_CTRL);
 
@@ -442,8 +441,6 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
 	}
 
 	actmon_write_barrier(tegra);
-
-	enable_irq(tegra->irq);
 }
 
 static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
@@ -552,6 +549,12 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
 {
 	struct tegra_devfreq *tegra = dev_get_drvdata(devfreq->dev.parent);
 
+	/*
+	 * Couple device with the governor early as it is needed at
+	 * the moment of governor's start (used by ISR).
+	 */
+	tegra->devfreq = devfreq;
+
 	switch (event) {
 	case DEVFREQ_GOV_START:
 		devfreq_monitor_start(devfreq);
@@ -586,10 +589,11 @@ static struct devfreq_governor tegra_devfreq_governor = {
 
 static int tegra_devfreq_probe(struct platform_device *pdev)
 {
-	struct tegra_devfreq *tegra;
 	struct tegra_devfreq_device *dev;
-	unsigned int i;
+	struct tegra_devfreq *tegra;
+	struct devfreq *devfreq;
 	unsigned long rate;
+	unsigned int i;
 	int err;
 
 	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
@@ -625,6 +629,16 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	}
 	tegra->irq = err;
 
+	irq_set_status_flags(tegra->irq, IRQ_NOAUTOEN);
+
+	err = devm_request_threaded_irq(&pdev->dev, tegra->irq, NULL,
+					actmon_thread_isr, IRQF_ONESHOT,
+					"tegra-devfreq", tegra);
+	if (err) {
+		dev_err(&pdev->dev, "Interrupt request failed: %d\n", err);
+		return err;
+	}
+
 	reset_control_assert(tegra->reset);
 
 	err = clk_prepare_enable(tegra->clock);
@@ -672,28 +686,15 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	}
 
 	tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
-	tegra->devfreq = devfreq_add_device(&pdev->dev,
-					    &tegra_devfreq_profile,
-					    "tegra_actmon",
-					    NULL);
-	if (IS_ERR(tegra->devfreq)) {
-		err = PTR_ERR(tegra->devfreq);
+	devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
+				     "tegra_actmon", NULL);
+	if (IS_ERR(devfreq)) {
+		err = PTR_ERR(devfreq);
 		goto remove_governor;
 	}
 
-	err = devm_request_threaded_irq(&pdev->dev, tegra->irq, NULL,
-					actmon_thread_isr, IRQF_ONESHOT,
-					"tegra-devfreq", tegra);
-	if (err) {
-		dev_err(&pdev->dev, "Interrupt request failed: %d\n", err);
-		goto remove_devfreq;
-	}
-
 	return 0;
 
-remove_devfreq:
-	devfreq_remove_device(tegra->devfreq);
-
 remove_governor:
 	devfreq_remove_governor(&tegra_devfreq_governor);
 
-- 
2.22.0

Re: [PATCH v6 02/19] PM / devfreq: tegra30: Keep interrupt disabled while governor is stopped

[Chanwoo Choi]

Hi,

On 19. 8. 12. 오전 6:22, Dmitry Osipenko wrote:
> There is no real need to keep interrupt always-enabled, will be nicer
> to keep it disabled while governor is inactive.
> 
> Suggested-by: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra30-devfreq.c | 47 ++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index a27300f40b0b..8be6a33beb9c 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -11,6 +11,7 @@
>  #include <linux/devfreq.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> +#include <linux/irq.h>
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/platform_device.h>
> @@ -416,8 +417,6 @@ static void tegra_actmon_start(struct tegra_devfreq *tegra)
>  {
>  	unsigned int i;
>  
> -	disable_irq(tegra->irq);
> -
>  	actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
>  		      ACTMON_GLB_PERIOD_CTRL);
>  
> @@ -442,8 +441,6 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
>  	}
>  
>  	actmon_write_barrier(tegra);
> -
> -	enable_irq(tegra->irq);
>  }
>  
>  static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
> @@ -552,6 +549,12 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
>  {
>  	struct tegra_devfreq *tegra = dev_get_drvdata(devfreq->dev.parent);
>  
> +	/*
> +	 * Couple device with the governor early as it is needed at
> +	 * the moment of governor's start (used by ISR).
> +	 */
> +	tegra->devfreq = devfreq;
> +
>  	switch (event) {
>  	case DEVFREQ_GOV_START:
>  		devfreq_monitor_start(devfreq);
> @@ -586,10 +589,11 @@ static struct devfreq_governor tegra_devfreq_governor = {
>  
>  static int tegra_devfreq_probe(struct platform_device *pdev)
>  {
> -	struct tegra_devfreq *tegra;
>  	struct tegra_devfreq_device *dev;
> -	unsigned int i;
> +	struct tegra_devfreq *tegra;
> +	struct devfreq *devfreq;
>  	unsigned long rate;
> +	unsigned int i;
>  	int err;
>  
>  	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> @@ -625,6 +629,16 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  	}
>  	tegra->irq = err;
>  
> +	irq_set_status_flags(tegra->irq, IRQ_NOAUTOEN);
> +
> +	err = devm_request_threaded_irq(&pdev->dev, tegra->irq, NULL,
> +					actmon_thread_isr, IRQF_ONESHOT,
> +					"tegra-devfreq", tegra);
> +	if (err) {
> +		dev_err(&pdev->dev, "Interrupt request failed: %d\n", err);
> +		return err;
> +	}
> +
>  	reset_control_assert(tegra->reset);
>  
>  	err = clk_prepare_enable(tegra->clock);
> @@ -672,28 +686,15 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  	}
>  
>  	tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
> -	tegra->devfreq = devfreq_add_device(&pdev->dev,
> -					    &tegra_devfreq_profile,
> -					    "tegra_actmon",
> -					    NULL);
> -	if (IS_ERR(tegra->devfreq)) {
> -		err = PTR_ERR(tegra->devfreq);
> +	devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
> +				     "tegra_actmon", NULL);
> +	if (IS_ERR(devfreq)) {
> +		err = PTR_ERR(devfreq);
>  		goto remove_governor;
>  	}
>  
> -	err = devm_request_threaded_irq(&pdev->dev, tegra->irq, NULL,
> -					actmon_thread_isr, IRQF_ONESHOT,
> -					"tegra-devfreq", tegra);
> -	if (err) {
> -		dev_err(&pdev->dev, "Interrupt request failed: %d\n", err);
> -		goto remove_devfreq;
> -	}
> -
>  	return 0;
>  
> -remove_devfreq:
> -	devfreq_remove_device(tegra->devfreq);
> -
>  remove_governor:
>  	devfreq_remove_governor(&tegra_devfreq_governor);
>  
> 

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[PATCH v6 03/19] PM / devfreq: tegra30: Handle possible round-rate error

[Dmitry Osipenko]

The EMC clock rate rounding technically could fail, hence let's handle
the error cases properly.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 8be6a33beb9c..bfee9d43de1e 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -592,8 +592,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	struct tegra_devfreq_device *dev;
 	struct tegra_devfreq *tegra;
 	struct devfreq *devfreq;
-	unsigned long rate;
 	unsigned int i;
+	long rate;
 	int err;
 
 	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
@@ -650,8 +650,14 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 
 	reset_control_deassert(tegra->reset);
 
-	tegra->max_freq = clk_round_rate(tegra->emc_clock, ULONG_MAX) / KHZ;
+	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
+	if (rate < 0) {
+		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
+		return rate;
+	}
+
 	tegra->cur_freq = clk_get_rate(tegra->emc_clock) / KHZ;
+	tegra->max_freq = rate / KHZ;
 
 	for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
 		dev = tegra->devices + i;
@@ -662,6 +668,13 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
 		rate = clk_round_rate(tegra->emc_clock, rate);
 
+		if (rate < 0) {
+			dev_err(&pdev->dev,
+				"Failed to round clock rate: %ld\n", rate);
+			err = rate;
+			goto remove_opps;
+		}
+
 		err = dev_pm_opp_add(&pdev->dev, rate, 0);
 		if (err) {
 			dev_err(&pdev->dev, "Failed to add OPP: %d\n", err);
-- 
2.22.0

Re: [PATCH v6 03/19] PM / devfreq: tegra30: Handle possible round-rate error

[Chanwoo Choi]

Hi,

On 19. 8. 12. 오전 6:22, Dmitry Osipenko wrote:
> The EMC clock rate rounding technically could fail, hence let's handle
> the error cases properly.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra30-devfreq.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 8be6a33beb9c..bfee9d43de1e 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -592,8 +592,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  	struct tegra_devfreq_device *dev;
>  	struct tegra_devfreq *tegra;
>  	struct devfreq *devfreq;
> -	unsigned long rate;
>  	unsigned int i;
> +	long rate;
>  	int err;
>  
>  	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> @@ -650,8 +650,14 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  
>  	reset_control_deassert(tegra->reset);
>  
> -	tegra->max_freq = clk_round_rate(tegra->emc_clock, ULONG_MAX) / KHZ;
> +	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
> +	if (rate < 0) {
> +		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
> +		return rate;
> +	}
> +
>  	tegra->cur_freq = clk_get_rate(tegra->emc_clock) / KHZ;
> +	tegra->max_freq = rate / KHZ;
>  
>  	for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
>  		dev = tegra->devices + i;
> @@ -662,6 +668,13 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  	for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
>  		rate = clk_round_rate(tegra->emc_clock, rate);
>  
> +		if (rate < 0) {
> +			dev_err(&pdev->dev,
> +				"Failed to round clock rate: %ld\n", rate);
> +			err = rate;
> +			goto remove_opps;
> +		}
> +
>  		err = dev_pm_opp_add(&pdev->dev, rate, 0);
>  		if (err) {
>  			dev_err(&pdev->dev, "Failed to add OPP: %d\n", err);
> 

It supports the exception handling. Looks good to me.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[PATCH v6 04/19] PM / devfreq: tegra30: Drop write-barrier

[Dmitry Osipenko]

There is no need in a write-barrier now, given that interrupt masking is
handled by CPU's GIC now. Hence we know exactly that interrupt won't fire
after stopping the devfreq's governor. In other cases we don't care about
potential buffering of the writes to hardware and thus there is no need to
stall CPU.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index bfee9d43de1e..ee14bf534c0d 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -230,12 +230,6 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
 		      ACTMON_DEV_LOWER_WMARK);
 }
 
-static void actmon_write_barrier(struct tegra_devfreq *tegra)
-{
-	/* ensure the update has reached the ACTMON */
-	readl(tegra->regs + ACTMON_GLB_STATUS);
-}
-
 static void actmon_isr_device(struct tegra_devfreq *tegra,
 			      struct tegra_devfreq_device *dev)
 {
@@ -287,8 +281,6 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
 	device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
 
 	device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
-
-	actmon_write_barrier(tegra);
 }
 
 static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra,
@@ -376,8 +368,6 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
 		tegra_devfreq_update_wmark(tegra, dev);
 	}
 
-	actmon_write_barrier(tegra);
-
 	return NOTIFY_OK;
 }
 
@@ -423,8 +413,6 @@ 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]);
 
-	actmon_write_barrier(tegra);
-
 	enable_irq(tegra->irq);
 }
 
@@ -439,8 +427,6 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
 		device_writel(&tegra->devices[i], ACTMON_INTR_STATUS_CLEAR,
 			      ACTMON_DEV_INTR_STATUS);
 	}
-
-	actmon_write_barrier(tegra);
 }
 
 static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
-- 
2.22.0

[PATCH v6 05/19] PM / devfreq: tegra30: Set up watermarks properly

[Dmitry Osipenko]

The current implementation is inaccurate and results in very intensive
interrupt activity, which neglects the whole idea of polling offload to
hardware. The reason of the shortcoming is that watermarks are not set
up correctly and this results in ACTMON constantly asking to change freq
and then these requests are ignored. The end result of this patch is that
there are few hundreds of ACTMON's interrupts instead of tens thousands
after few minutes of a working devfreq, meanwhile the transitions activity
stays about the same and governor becomes more reactive.

Since watermarks are set precisely correct now, the boosting logic is
changed a tad to accommodate the change. The "average sustain coefficient"
multiplier is gone now since there is no need to compensate the improper
watermarks and EMC frequency-bump happens once boosting hits the upper
watermark enough times, depending on the per-device boosting threshold.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 293 +++++++++++++++++++++---------
 1 file changed, 209 insertions(+), 84 deletions(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index ee14bf534c0d..2331052fd8bd 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -47,6 +47,8 @@
 
 #define ACTMON_DEV_INTR_CONSECUTIVE_UPPER			BIT(31)
 #define ACTMON_DEV_INTR_CONSECUTIVE_LOWER			BIT(30)
+#define ACTMON_DEV_INTR_AVG_BELOW_WMARK				BIT(25)
+#define ACTMON_DEV_INTR_AVG_ABOVE_WMARK				BIT(24)
 
 #define ACTMON_ABOVE_WMARK_WINDOW				1
 #define ACTMON_BELOW_WMARK_WINDOW				3
@@ -63,9 +65,8 @@
  * ACTMON_AVERAGE_WINDOW_LOG2: default value for @DEV_CTRL_K_VAL, which
  * translates to 2 ^ (K_VAL + 1). ex: 2 ^ (6 + 1) = 128
  */
-#define ACTMON_AVERAGE_WINDOW_LOG2			6
-#define ACTMON_SAMPLING_PERIOD				12 /* ms */
-#define ACTMON_DEFAULT_AVG_BAND				6  /* 1/10 of % */
+#define ACTMON_AVERAGE_WINDOW_LOG2				6
+#define ACTMON_SAMPLING_PERIOD					12 /* ms */
 
 #define KHZ							1000
 
@@ -142,9 +143,6 @@ struct tegra_devfreq_device {
 	 * watermark breaches.
 	 */
 	unsigned long boost_freq;
-
-	/* Optimal frequency calculated from the stats for this device */
-	unsigned long target_freq;
 };
 
 struct tegra_devfreq {
@@ -156,7 +154,6 @@ struct tegra_devfreq {
 
 	struct clk		*emc_clock;
 	unsigned long		max_freq;
-	unsigned long		cur_freq;
 	struct notifier_block	rate_change_nb;
 
 	struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
@@ -205,42 +202,182 @@ 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)
+{
+	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++) {
+		if (cpu_freq >= ratio->cpu_freq) {
+			if (ratio->emc_freq >= tegra->max_freq)
+				return tegra->max_freq;
+			else
+				return ratio->emc_freq;
+		}
+	}
+
+	return 0;
+}
+
+static unsigned long
+tegra_actmon_account_cpu_freq(struct tegra_devfreq *tegra,
+			      struct tegra_devfreq_device *dev,
+			      unsigned long target_freq)
+{
+	unsigned long static_cpu_emc_freq;
+
+	if (dev->config->avg_dependency_threshold &&
+	    dev->config->avg_dependency_threshold < dev->avg_count) {
+		static_cpu_emc_freq = actmon_cpu_to_emc_rate(tegra);
+		target_freq = max(target_freq, static_cpu_emc_freq);
+	}
+
+	return target_freq;
+}
+
+static unsigned long tegra_actmon_lower_freq(struct tegra_devfreq *tegra,
+					     unsigned long target_freq)
+{
+	unsigned long lower = target_freq;
+	struct dev_pm_opp *opp;
+
+	opp = dev_pm_opp_find_freq_floor(tegra->devfreq->dev.parent, &lower);
+	if (IS_ERR(opp))
+		lower = 0;
+	else
+		dev_pm_opp_put(opp);
+
+	return lower;
+}
+
+static unsigned long tegra_actmon_upper_freq(struct tegra_devfreq *tegra,
+					     unsigned long target_freq)
+{
+	unsigned long upper = target_freq + 1;
+	struct dev_pm_opp *opp;
+
+	opp = dev_pm_opp_find_freq_ceil(tegra->devfreq->dev.parent, &upper);
+	if (IS_ERR(opp))
+		upper = ULONG_MAX;
+	else
+		dev_pm_opp_put(opp);
+
+	return upper;
+}
+
+static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
+					 struct tegra_devfreq_device *dev,
+					 unsigned long target_freq,
+					 unsigned long *lower,
+					 unsigned long *upper)
+{
+	/*
+	 * Memory frequencies are guaranteed to have 1MHz granularity
+	 * and thus we need this rounding down to get a proper watermarks
+	 * range in a case where target_freq falls into a range of
+	 * next_possible_opp_freq - 1MHz.
+	 */
+	target_freq = round_down(target_freq, 1000000);
+
+	/* watermarks are set at the borders of the corresponding OPPs */
+	*lower = tegra_actmon_lower_freq(tegra, target_freq);
+	*upper = tegra_actmon_upper_freq(tegra, target_freq);
+
+	*lower /= KHZ;
+	*upper /= KHZ;
+
+	/*
+	 * The upper watermark should take into account CPU's frequency
+	 * because cpu_to_emc_rate() may override the target_freq with
+	 * a higher value and thus upper watermark need to be set up
+	 * accordingly to avoid parasitic upper-events.
+	 */
+	*upper = tegra_actmon_account_cpu_freq(tegra, dev, *upper);
+
+	*lower *= ACTMON_SAMPLING_PERIOD;
+	*upper *= ACTMON_SAMPLING_PERIOD;
+}
+
 static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
 					   struct tegra_devfreq_device *dev)
 {
-	u32 avg = dev->avg_count;
-	u32 avg_band_freq = tegra->max_freq * ACTMON_DEFAULT_AVG_BAND / KHZ;
-	u32 band = avg_band_freq * ACTMON_SAMPLING_PERIOD;
+	unsigned long lower, upper, freq;
 
-	device_writel(dev, avg + band, ACTMON_DEV_AVG_UPPER_WMARK);
+	freq = dev->avg_count / ACTMON_SAMPLING_PERIOD * KHZ;
+	tegra_actmon_get_lower_upper(tegra, dev, freq, &lower, &upper);
 
-	avg = max(dev->avg_count, band);
-	device_writel(dev, avg - band, ACTMON_DEV_AVG_LOWER_WMARK);
+	/*
+	 * We want to get interrupts when MCCPU client crosses the
+	 * dependency threshold in order to take into / out of account
+	 * the CPU's freq.
+	 */
+	if (lower < dev->config->avg_dependency_threshold &&
+	    upper > dev->config->avg_dependency_threshold) {
+		if (dev->avg_count < dev->config->avg_dependency_threshold)
+			upper = dev->config->avg_dependency_threshold;
+		else
+			lower = dev->config->avg_dependency_threshold;
+	}
+
+	device_writel(dev, lower, ACTMON_DEV_AVG_LOWER_WMARK);
+	device_writel(dev, upper, ACTMON_DEV_AVG_UPPER_WMARK);
 }
 
 static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
-				       struct tegra_devfreq_device *dev)
+				       struct tegra_devfreq_device *dev,
+				       unsigned long freq)
 {
-	u32 val = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
+	unsigned long lower, upper, delta;
+
+	/*
+	 * Boosting logic kicks-in once lower / upper watermark is hit.
+	 * The watermarks are based on the updated EMC rate and the
+	 * average activity.
+	 *
+	 * The higher watermark is set in accordance to the EMC rate
+	 * because we want to set it to the highest mark here and EMC rate
+	 * represents that mark. The consecutive-upper interrupts are
+	 * always enabled and we don't want to receive them if they won't
+	 * do anything useful, hence the upper watermark is capped to maximum.
+	 * Note that the EMC rate is changed once boosting pushed the rate
+	 * too high, in that case boosting-up will be stopped because
+	 * upper watermark is much higher now and it is *important* to
+	 * stop the unwanted interrupts.
+	 */
+	tegra_actmon_get_lower_upper(tegra, dev, freq - 1, &lower, &upper);
+
+	delta = do_percent(upper - lower, dev->config->boost_up_threshold);
+	device_writel(dev, lower + delta, ACTMON_DEV_UPPER_WMARK);
 
-	device_writel(dev, do_percent(val, dev->config->boost_up_threshold),
-		      ACTMON_DEV_UPPER_WMARK);
+	/*
+	 * Meanwhile the lower mark is based on the average value
+	 * because it is the lowest possible consecutive-mark for this
+	 * device. Once that mark is hit and boosting is stopped, the
+	 * interrupt is disabled by ISR.
+	 */
+	freq = dev->avg_count / ACTMON_SAMPLING_PERIOD * KHZ;
+	tegra_actmon_get_lower_upper(tegra, dev, freq, &lower, &upper);
 
-	device_writel(dev, do_percent(val, dev->config->boost_down_threshold),
-		      ACTMON_DEV_LOWER_WMARK);
+	delta = do_percent(upper - lower, dev->config->boost_down_threshold);
+	device_writel(dev, lower + delta, ACTMON_DEV_LOWER_WMARK);
 }
 
 static void actmon_isr_device(struct tegra_devfreq *tegra,
 			      struct tegra_devfreq_device *dev)
 {
-	u32 intr_status, dev_ctrl;
+	u32 intr_status, dev_ctrl, avg_intr_mask;
 
 	dev->avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
-	tegra_devfreq_update_avg_wmark(tegra, dev);
-
 	intr_status = device_readl(dev, ACTMON_DEV_INTR_STATUS);
 	dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
 
+	avg_intr_mask = ACTMON_DEV_INTR_AVG_BELOW_WMARK |
+			ACTMON_DEV_INTR_AVG_ABOVE_WMARK;
+
+	if (intr_status & avg_intr_mask)
+		tegra_devfreq_update_avg_wmark(tegra, dev);
+
 	if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_UPPER) {
 		/*
 		 * new_boost = min(old_boost * up_coef + step, max_freq)
@@ -253,8 +390,6 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
 
 		if (dev->boost_freq >= tegra->max_freq)
 			dev->boost_freq = tegra->max_freq;
-		else
-			dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
 	} else if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_LOWER) {
 		/*
 		 * new_boost = old_boost * down_coef
@@ -263,63 +398,37 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
 		dev->boost_freq = do_percent(dev->boost_freq,
 					     dev->config->boost_down_coeff);
 
-		dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
-
 		if (dev->boost_freq < (ACTMON_BOOST_FREQ_STEP >> 1))
 			dev->boost_freq = 0;
-		else
-			dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
 	}
 
-	if (dev->config->avg_dependency_threshold) {
-		if (dev->avg_count >= dev->config->avg_dependency_threshold)
-			dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
-		else if (dev->boost_freq == 0)
-			dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+	if (intr_status & avg_intr_mask) {
+		/*
+		 * Once average watermark is hit, it means that the memory
+		 * activity changed significantly and thus boosting-up shall
+		 * be reset because EMC clock rate will be changed and
+		 * boosting will restart in this case.
+		 */
+		dev->boost_freq = 0;
 	}
 
-	device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
+	/* no boosting => no need for consecutive-down interrupt */
+	if (dev->boost_freq == 0)
+		dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
 
+	device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
 	device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
 }
 
-static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra,
-					    unsigned long cpu_freq)
-{
-	unsigned int i;
-	struct tegra_actmon_emc_ratio *ratio = actmon_emc_ratios;
-
-	for (i = 0; i < ARRAY_SIZE(actmon_emc_ratios); i++, ratio++) {
-		if (cpu_freq >= ratio->cpu_freq) {
-			if (ratio->emc_freq >= tegra->max_freq)
-				return tegra->max_freq;
-			else
-				return ratio->emc_freq;
-		}
-	}
-
-	return 0;
-}
-
-static void actmon_update_target(struct tegra_devfreq *tegra,
-				 struct tegra_devfreq_device *dev)
+static unsigned long actmon_update_target(struct tegra_devfreq *tegra,
+					  struct tegra_devfreq_device *dev)
 {
-	unsigned long cpu_freq = 0;
-	unsigned long static_cpu_emc_freq = 0;
-	unsigned int avg_sustain_coef;
-
-	if (dev->config->avg_dependency_threshold) {
-		cpu_freq = cpufreq_get(0);
-		static_cpu_emc_freq = actmon_cpu_to_emc_rate(tegra, cpu_freq);
-	}
+	unsigned long target_freq;
 
-	dev->target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD;
-	avg_sustain_coef = 100 * 100 / dev->config->boost_up_threshold;
-	dev->target_freq = do_percent(dev->target_freq, avg_sustain_coef);
-	dev->target_freq += dev->boost_freq;
+	target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD + dev->boost_freq;
+	target_freq = tegra_actmon_account_cpu_freq(tegra, dev, target_freq);
 
-	if (dev->avg_count >= dev->config->avg_dependency_threshold)
-		dev->target_freq = max(dev->target_freq, static_cpu_emc_freq);
+	return target_freq;
 }
 
 static irqreturn_t actmon_thread_isr(int irq, void *data)
@@ -351,8 +460,8 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
 				       unsigned long action, void *ptr)
 {
 	struct clk_notifier_data *data = ptr;
-	struct tegra_devfreq *tegra;
 	struct tegra_devfreq_device *dev;
+	struct tegra_devfreq *tegra;
 	unsigned int i;
 
 	if (action != POST_RATE_CHANGE)
@@ -360,12 +469,28 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
 
 	tegra = container_of(nb, struct tegra_devfreq, rate_change_nb);
 
-	tegra->cur_freq = data->new_rate / KHZ;
-
+	/*
+	 * EMC rate could change due to three reasons:
+	 *
+	 *    1. Average watermark hit
+	 *    2. Boosting overflow
+	 *    3. CPU freq change
+	 *
+	 * Once rate is changed, the consecutive watermarks need to be
+	 * updated in order for boosting to work properly and to avoid
+	 * unnecessary interrupts. Note that the consecutive range is set for
+	 * all of devices using the same rate, hence if CPU is doing much
+	 * less than the other memory clients, then its upper watermark will
+	 * be very high in comparison to the actual activity (lower watermark)
+	 * and thus unnecessary upper-interrupts will be suppressed.
+	 *
+	 * The average watermarks also should be updated because of 3.
+	 */
 	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
 		dev = &tegra->devices[i];
 
-		tegra_devfreq_update_wmark(tegra, dev);
+		tegra_devfreq_update_avg_wmark(tegra, dev);
+		tegra_devfreq_update_wmark(tegra, dev, data->new_rate);
 	}
 
 	return NOTIFY_OK;
@@ -374,15 +499,14 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
 static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
 					  struct tegra_devfreq_device *dev)
 {
-	u32 val = 0;
-
-	dev->target_freq = tegra->cur_freq;
+	u32 val = 0, target_freq;
 
-	dev->avg_count = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
+	target_freq = clk_get_rate(tegra->emc_clock) / KHZ;
+	dev->avg_count = target_freq * ACTMON_SAMPLING_PERIOD;
 	device_writel(dev, dev->avg_count, ACTMON_DEV_INIT_AVG);
 
 	tegra_devfreq_update_avg_wmark(tegra, dev);
-	tegra_devfreq_update_wmark(tegra, dev);
+	tegra_devfreq_update_wmark(tegra, dev, target_freq);
 
 	device_writel(dev, ACTMON_COUNT_WEIGHT, ACTMON_DEV_COUNT_WEIGHT);
 	device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
@@ -469,13 +593,13 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
 	struct tegra_devfreq_device *actmon_dev;
 	unsigned long cur_freq;
 
-	cur_freq = READ_ONCE(tegra->cur_freq);
+	cur_freq = clk_get_rate(tegra->emc_clock);
 
 	/* To be used by the tegra governor */
 	stat->private_data = tegra;
 
 	/* The below are to be used by the other governors */
-	stat->current_frequency = cur_freq * KHZ;
+	stat->current_frequency = cur_freq;
 
 	actmon_dev = &tegra->devices[MCALL];
 
@@ -486,7 +610,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
 	stat->busy_time *= 100 / BUS_SATURATION_RATIO;
 
 	/* Number of cycles in a sampling period */
-	stat->total_time = ACTMON_SAMPLING_PERIOD * cur_freq;
+	stat->total_time = cur_freq / KHZ * ACTMON_SAMPLING_PERIOD;
 
 	stat->busy_time = min(stat->busy_time, stat->total_time);
 
@@ -505,6 +629,7 @@ static int tegra_governor_get_target(struct devfreq *devfreq,
 	struct devfreq_dev_status *stat;
 	struct tegra_devfreq *tegra;
 	struct tegra_devfreq_device *dev;
+	unsigned long dev_target_freq;
 	unsigned long target_freq = 0;
 	unsigned int i;
 	int err;
@@ -520,9 +645,9 @@ static int tegra_governor_get_target(struct devfreq *devfreq,
 	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
 		dev = &tegra->devices[i];
 
-		actmon_update_target(tegra, dev);
+		dev_target_freq = actmon_update_target(tegra, dev);
 
-		target_freq = max(target_freq, dev->target_freq);
+		target_freq = max(target_freq, dev_target_freq);
 	}
 
 	*freq = target_freq * KHZ;
@@ -642,7 +767,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 		return rate;
 	}
 
-	tegra->cur_freq = clk_get_rate(tegra->emc_clock) / KHZ;
 	tegra->max_freq = rate / KHZ;
 
 	for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
@@ -671,7 +795,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, tegra);
 
 	tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
-	err = clk_notifier_register(tegra->emc_clock, &tegra->rate_change_nb);
+	err = clk_notifier_register(tegra->emc_clock,
+				    &tegra->rate_change_nb);
 	if (err) {
 		dev_err(&pdev->dev,
 			"Failed to register rate change notifier\n");
-- 
2.22.0

[PATCH v6 06/19] PM / devfreq: tegra30: Tune up boosting thresholds

[Dmitry Osipenko]

Now that average-sustain coefficient / multiplier is gone, it won't hurt
to re-tune the boosting thresholds to get a bit harder boosting for MCALL
clients, resulting in a more reactive governing in a case of multimedia
applications usage like 3d / video.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 2331052fd8bd..4525c051f85c 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -111,8 +111,8 @@ static struct tegra_devfreq_device_config actmon_device_configs[] = {
 		.irq_mask = 1 << 26,
 		.boost_up_coeff = 200,
 		.boost_down_coeff = 50,
-		.boost_up_threshold = 60,
-		.boost_down_threshold = 40,
+		.boost_up_threshold = 50,
+		.boost_down_threshold = 25,
 	},
 	{
 		/* MCCPU: memory accesses from the CPUs */
-- 
2.22.0

[PATCH v6 07/19] PM / devfreq: tegra30: Fix integer overflow on CPU's freq max out

[Dmitry Osipenko]

There is another kHz-conversion bug in the code, resulting in integer
overflow. Although, this time the resulting value is 4294966296 and it's
close to ULONG_MAX, which is okay in this case.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 4525c051f85c..70dce58212a4 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -70,6 +70,8 @@
 
 #define KHZ							1000
 
+#define KHZ_MAX						(ULONG_MAX / KHZ)
+
 /* Assume that the bus is saturated if the utilization is 25% */
 #define BUS_SATURATION_RATIO					25
 
@@ -167,7 +169,7 @@ struct tegra_actmon_emc_ratio {
 };
 
 static struct tegra_actmon_emc_ratio actmon_emc_ratios[] = {
-	{ 1400000, ULONG_MAX },
+	{ 1400000,    KHZ_MAX },
 	{ 1200000,    750000 },
 	{ 1100000,    600000 },
 	{ 1000000,    500000 },
-- 
2.22.0

[PATCH v6 08/19] PM / devfreq: tegra30: Ensure that target freq won't overflow

[Dmitry Osipenko]

We already had few integer overflow bugs, let's limit the freq for
consistency.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 70dce58212a4..ca499368ee81 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -430,7 +430,7 @@ static unsigned long actmon_update_target(struct tegra_devfreq *tegra,
 	target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD + dev->boost_freq;
 	target_freq = tegra_actmon_account_cpu_freq(tegra, dev, target_freq);
 
-	return target_freq;
+	return min(target_freq, tegra->max_freq);
 }
 
 static irqreturn_t actmon_thread_isr(int irq, void *data)
-- 
2.22.0

Re: [PATCH v6 08/19] PM / devfreq: tegra30: Ensure that target freq won't overflow

[Chanwoo Choi]

On 19. 8. 12. 오전 6:23, Dmitry Osipenko wrote:
> We already had few integer overflow bugs, let's limit the freq for
> consistency.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra30-devfreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 70dce58212a4..ca499368ee81 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -430,7 +430,7 @@ static unsigned long actmon_update_target(struct tegra_devfreq *tegra,
>  	target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD + dev->boost_freq;
>  	target_freq = tegra_actmon_account_cpu_freq(tegra, dev, target_freq);
>  
> -	return target_freq;
> +	return min(target_freq, tegra->max_freq);

Once again, did you meet this case sometimes?

Usually, we can prevent the overflow of target_freq
when calculating the target frequency or this style.

I think that if the overflow of target frequency happen frequently,
it might have the problem of calculation way. 

>  }
>  
>  static irqreturn_t actmon_thread_isr(int irq, void *data)
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v6 08/19] PM / devfreq: tegra30: Ensure that target freq won't overflow

[Dmitry Osipenko]

Hello Chanwoo,

20.08.2019 3:23, Chanwoo Choi пишет:
> On 19. 8. 12. 오전 6:23, Dmitry Osipenko wrote:
>> We already had few integer overflow bugs, let's limit the freq for
>> consistency.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/tegra30-devfreq.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>> index 70dce58212a4..ca499368ee81 100644
>> --- a/drivers/devfreq/tegra30-devfreq.c
>> +++ b/drivers/devfreq/tegra30-devfreq.c
>> @@ -430,7 +430,7 @@ static unsigned long actmon_update_target(struct tegra_devfreq *tegra,
>>  	target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD + dev->boost_freq;
>>  	target_freq = tegra_actmon_account_cpu_freq(tegra, dev, target_freq);
>>  
>> -	return target_freq;
>> +	return min(target_freq, tegra->max_freq);
> 
> Once again, did you meet this case sometimes?

This case happens at least whenever CPU's freq accounting returns
KHZ_MAX, please see actmon_emc_ratios[] and actmon_cpu_to_emc_rate().

> Usually, we can prevent the overflow of target_freq
> when calculating the target frequency or this style.
> 
> I think that if the overflow of target frequency happen frequently,
> it might have the problem of calculation way. 

It is possible to get sampled avg_count from hardware that results in
target_freq > max_freq because translating number of memory events to
memory frequency has some calculation error, although it is negligible.

In fact, it shouldn't matter that the returned target_freq > max_freq
with the current driver's code structure and in the commit's message I
wrote that this change is done for consistency. Hence it should be okay
to drop this patch as well.

[PATCH v6 09/19] PM / devfreq: tegra30: Use kHz units uniformly in the code

[Dmitry Osipenko]

Now that all kHz-conversion related bugs are fixed, we can use the kHz
uniformly. This makes code cleaner and avoids integer divisions in the
code, which is useful in a case of Tegra30 that has Cortex A9 CPU that
doesn't support integer division instructions, hence all divisions are
actually made in software mode. Another small benefit from this change
is that now powertop utility correctly displays devfreq's stats, for
some reason it expects them to be in kHz.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 81 +++++++++++++++++++------------
 1 file changed, 49 insertions(+), 32 deletions(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index ca499368ee81..43d50b4366dd 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -137,8 +137,11 @@ struct tegra_devfreq_device {
 	const struct tegra_devfreq_device_config *config;
 	void __iomem *regs;
 
-	/* Average event count sampled in the last interrupt */
-	u32 avg_count;
+	/*
+	 * Average event count sampled in the last interrupt and converted
+	 * to frequency value.
+	 */
+	u32 avg_freq;
 
 	/*
 	 * Extra frequency to increase the target by due to consecutive
@@ -222,6 +225,14 @@ static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra)
 	return 0;
 }
 
+static unsigned long
+tegra_actmon_dev_avg_dependency_freq(struct tegra_devfreq *tegra,
+				     struct tegra_devfreq_device *dev)
+{
+	return dev->config->avg_dependency_threshold /
+		ACTMON_SAMPLING_PERIOD;
+}
+
 static unsigned long
 tegra_actmon_account_cpu_freq(struct tegra_devfreq *tegra,
 			      struct tegra_devfreq_device *dev,
@@ -229,13 +240,15 @@ tegra_actmon_account_cpu_freq(struct tegra_devfreq *tegra,
 {
 	unsigned long static_cpu_emc_freq;
 
-	if (dev->config->avg_dependency_threshold &&
-	    dev->config->avg_dependency_threshold < dev->avg_count) {
+	if (!dev->config->avg_dependency_threshold)
+		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);
-		target_freq = max(target_freq, static_cpu_emc_freq);
-	}
+	else
+		static_cpu_emc_freq = 0;
 
-	return target_freq;
+	return max(target_freq, static_cpu_emc_freq);
 }
 
 static unsigned long tegra_actmon_lower_freq(struct tegra_devfreq *tegra,
@@ -261,7 +274,7 @@ static unsigned long tegra_actmon_upper_freq(struct tegra_devfreq *tegra,
 
 	opp = dev_pm_opp_find_freq_ceil(tegra->devfreq->dev.parent, &upper);
 	if (IS_ERR(opp))
-		upper = ULONG_MAX;
+		upper = KHZ_MAX;
 	else
 		dev_pm_opp_put(opp);
 
@@ -280,15 +293,12 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
 	 * range in a case where target_freq falls into a range of
 	 * next_possible_opp_freq - 1MHz.
 	 */
-	target_freq = round_down(target_freq, 1000000);
+	target_freq = round_down(target_freq, 1000);
 
 	/* watermarks are set at the borders of the corresponding OPPs */
 	*lower = tegra_actmon_lower_freq(tegra, target_freq);
 	*upper = tegra_actmon_upper_freq(tegra, target_freq);
 
-	*lower /= KHZ;
-	*upper /= KHZ;
-
 	/*
 	 * The upper watermark should take into account CPU's frequency
 	 * because cpu_to_emc_rate() may override the target_freq with
@@ -304,10 +314,11 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
 static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
 					   struct tegra_devfreq_device *dev)
 {
-	unsigned long lower, upper, freq;
+	unsigned long avg_dependency_freq, lower, upper;
+
+	tegra_actmon_get_lower_upper(tegra, dev, dev->avg_freq, &lower, &upper);
 
-	freq = dev->avg_count / ACTMON_SAMPLING_PERIOD * KHZ;
-	tegra_actmon_get_lower_upper(tegra, dev, freq, &lower, &upper);
+	avg_dependency_freq = tegra_actmon_dev_avg_dependency_freq(tegra, dev);
 
 	/*
 	 * We want to get interrupts when MCCPU client crosses the
@@ -316,7 +327,7 @@ static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
 	 */
 	if (lower < dev->config->avg_dependency_threshold &&
 	    upper > dev->config->avg_dependency_threshold) {
-		if (dev->avg_count < dev->config->avg_dependency_threshold)
+		if (dev->avg_freq < avg_dependency_freq)
 			upper = dev->config->avg_dependency_threshold;
 		else
 			lower = dev->config->avg_dependency_threshold;
@@ -358,8 +369,7 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
 	 * device. Once that mark is hit and boosting is stopped, the
 	 * interrupt is disabled by ISR.
 	 */
-	freq = dev->avg_count / ACTMON_SAMPLING_PERIOD * KHZ;
-	tegra_actmon_get_lower_upper(tegra, dev, freq, &lower, &upper);
+	tegra_actmon_get_lower_upper(tegra, dev, dev->avg_freq, &lower, &upper);
 
 	delta = do_percent(upper - lower, dev->config->boost_down_threshold);
 	device_writel(dev, lower + delta, ACTMON_DEV_LOWER_WMARK);
@@ -368,12 +378,14 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
 static void actmon_isr_device(struct tegra_devfreq *tegra,
 			      struct tegra_devfreq_device *dev)
 {
-	u32 intr_status, dev_ctrl, avg_intr_mask;
+	u32 intr_status, dev_ctrl, avg_intr_mask, avg_count;
 
-	dev->avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
 	intr_status = device_readl(dev, ACTMON_DEV_INTR_STATUS);
+	avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
 	dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
 
+	dev->avg_freq = avg_count / ACTMON_SAMPLING_PERIOD;
+
 	avg_intr_mask = ACTMON_DEV_INTR_AVG_BELOW_WMARK |
 			ACTMON_DEV_INTR_AVG_ABOVE_WMARK;
 
@@ -427,7 +439,7 @@ static unsigned long actmon_update_target(struct tegra_devfreq *tegra,
 {
 	unsigned long target_freq;
 
-	target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD + dev->boost_freq;
+	target_freq = dev->avg_freq + dev->boost_freq;
 	target_freq = tegra_actmon_account_cpu_freq(tegra, dev, target_freq);
 
 	return min(target_freq, tegra->max_freq);
@@ -464,6 +476,7 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
 	struct clk_notifier_data *data = ptr;
 	struct tegra_devfreq_device *dev;
 	struct tegra_devfreq *tegra;
+	unsigned long freq;
 	unsigned int i;
 
 	if (action != POST_RATE_CHANGE)
@@ -471,6 +484,8 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
 
 	tegra = container_of(nb, struct tegra_devfreq, rate_change_nb);
 
+	freq = data->new_rate / KHZ;
+
 	/*
 	 * EMC rate could change due to three reasons:
 	 *
@@ -492,7 +507,7 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
 		dev = &tegra->devices[i];
 
 		tegra_devfreq_update_avg_wmark(tegra, dev);
-		tegra_devfreq_update_wmark(tegra, dev, data->new_rate);
+		tegra_devfreq_update_wmark(tegra, dev, freq);
 	}
 
 	return NOTIFY_OK;
@@ -501,14 +516,14 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
 static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
 					  struct tegra_devfreq_device *dev)
 {
-	u32 val = 0, target_freq;
+	u32 val = 0;
 
-	target_freq = clk_get_rate(tegra->emc_clock) / KHZ;
-	dev->avg_count = target_freq * ACTMON_SAMPLING_PERIOD;
-	device_writel(dev, dev->avg_count, ACTMON_DEV_INIT_AVG);
+	dev->avg_freq = clk_get_rate(tegra->emc_clock) / KHZ;
+	device_writel(dev, dev->avg_freq * ACTMON_SAMPLING_PERIOD,
+		      ACTMON_DEV_INIT_AVG);
 
 	tegra_devfreq_update_avg_wmark(tegra, dev);
-	tegra_devfreq_update_wmark(tegra, dev, target_freq);
+	tegra_devfreq_update_wmark(tegra, dev, dev->avg_freq);
 
 	device_writel(dev, ACTMON_COUNT_WEIGHT, ACTMON_DEV_COUNT_WEIGHT);
 	device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
@@ -572,7 +587,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
 	rate = dev_pm_opp_get_freq(opp);
 	dev_pm_opp_put(opp);
 
-	err = clk_set_min_rate(tegra->emc_clock, rate);
+	err = clk_set_min_rate(tegra->emc_clock, rate * KHZ);
 	if (err)
 		return err;
 
@@ -595,7 +610,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
 	struct tegra_devfreq_device *actmon_dev;
 	unsigned long cur_freq;
 
-	cur_freq = clk_get_rate(tegra->emc_clock);
+	cur_freq = clk_get_rate(tegra->emc_clock) / KHZ;
 
 	/* To be used by the tegra governor */
 	stat->private_data = tegra;
@@ -612,7 +627,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
 	stat->busy_time *= 100 / BUS_SATURATION_RATIO;
 
 	/* Number of cycles in a sampling period */
-	stat->total_time = cur_freq / KHZ * ACTMON_SAMPLING_PERIOD;
+	stat->total_time = cur_freq * ACTMON_SAMPLING_PERIOD;
 
 	stat->busy_time = min(stat->busy_time, stat->total_time);
 
@@ -652,7 +667,7 @@ static int tegra_governor_get_target(struct devfreq *devfreq,
 		target_freq = max(target_freq, dev_target_freq);
 	}
 
-	*freq = target_freq * KHZ;
+	*freq = target_freq;
 
 	return 0;
 }
@@ -787,7 +802,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 			goto remove_opps;
 		}
 
-		err = dev_pm_opp_add(&pdev->dev, rate, 0);
+		err = dev_pm_opp_add(&pdev->dev, rate / KHZ, 0);
 		if (err) {
 			dev_err(&pdev->dev, "Failed to add OPP: %d\n", err);
 			goto remove_opps;
@@ -812,6 +827,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	}
 
 	tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
+	tegra_devfreq_profile.initial_freq /= KHZ;
+
 	devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
 				     "tegra_actmon", NULL);
 	if (IS_ERR(devfreq)) {
-- 
2.22.0

Re: [PATCH v6 09/19] PM / devfreq: tegra30: Use kHz units uniformly in the code

[Chanwoo Choi]

On 19. 8. 12. 오전 6:23, Dmitry Osipenko wrote:
> Now that all kHz-conversion related bugs are fixed, we can use the kHz
> uniformly. This makes code cleaner and avoids integer divisions in the
> code, which is useful in a case of Tegra30 that has Cortex A9 CPU that
> doesn't support integer division instructions, hence all divisions are
> actually made in software mode. Another small benefit from this change
> is that now powertop utility correctly displays devfreq's stats, for
> some reason it expects them to be in kHz.

If possible, please specify the benefit result on patch description.

And I have a question. Why do you fix the KHz-conversion issue on one patch?
Actually, in my case, it is difficult to understand that multiple patches
try to fix the KHz-conversion issue. I think that it is possible to
make one patch.

And, 
On these series, some codes wad added and then these codes are deleted
on later patch. It looks like that you made the issue and then you fix
the issue by yourself. I think that it is not proper.
Even if you developed the patches on your local environment sequentially
according to the sequence of your issue detection, you better to do
refactoring the patches.

Frankly, I cannot agree that some codes wad added on front patch
and then added codes are deleted on later patch in the same patchset.


> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra30-devfreq.c | 81 +++++++++++++++++++------------
>  1 file changed, 49 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index ca499368ee81..43d50b4366dd 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -137,8 +137,11 @@ struct tegra_devfreq_device {
>  	const struct tegra_devfreq_device_config *config;
>  	void __iomem *regs;
>  
> -	/* Average event count sampled in the last interrupt */
> -	u32 avg_count;
> +	/*
> +	 * Average event count sampled in the last interrupt and converted
> +	 * to frequency value.
> +	 */
> +	u32 avg_freq;
>  
>  	/*
>  	 * Extra frequency to increase the target by due to consecutive
> @@ -222,6 +225,14 @@ static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra)
>  	return 0;
>  }
>  
> +static unsigned long
> +tegra_actmon_dev_avg_dependency_freq(struct tegra_devfreq *tegra,
> +				     struct tegra_devfreq_device *dev)
> +{
> +	return dev->config->avg_dependency_threshold /
> +		ACTMON_SAMPLING_PERIOD;
> +}
> +
>  static unsigned long
>  tegra_actmon_account_cpu_freq(struct tegra_devfreq *tegra,
>  			      struct tegra_devfreq_device *dev,
> @@ -229,13 +240,15 @@ tegra_actmon_account_cpu_freq(struct tegra_devfreq *tegra,
>  {
>  	unsigned long static_cpu_emc_freq;
>  
> -	if (dev->config->avg_dependency_threshold &&
> -	    dev->config->avg_dependency_threshold < dev->avg_count) {
> +	if (!dev->config->avg_dependency_threshold)
> +		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);
> -		target_freq = max(target_freq, static_cpu_emc_freq);
> -	}
> +	else
> +		static_cpu_emc_freq = 0;
>  
> -	return target_freq;
> +	return max(target_freq, static_cpu_emc_freq);
>  }
>  
>  static unsigned long tegra_actmon_lower_freq(struct tegra_devfreq *tegra,
> @@ -261,7 +274,7 @@ static unsigned long tegra_actmon_upper_freq(struct tegra_devfreq *tegra,
>  
>  	opp = dev_pm_opp_find_freq_ceil(tegra->devfreq->dev.parent, &upper);
>  	if (IS_ERR(opp))
> -		upper = ULONG_MAX;
> +		upper = KHZ_MAX;
>  	else
>  		dev_pm_opp_put(opp);
>  
> @@ -280,15 +293,12 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
>  	 * range in a case where target_freq falls into a range of
>  	 * next_possible_opp_freq - 1MHz.
>  	 */
> -	target_freq = round_down(target_freq, 1000000);

This line was added on patch5. I think that you could fix the KHz-conversion
on patch5 instead of this patch. It is not good way to make the patches.
Because some codes wad added and then these codes are deleted on later patch.

It looks like that you made the issue and then you fix the issue by yourself.
It is difficult to make me to decide this patch is either proper or not.

> +	target_freq = round_down(target_freq, 1000);
>  
>  	/* watermarks are set at the borders of the corresponding OPPs */
>  	*lower = tegra_actmon_lower_freq(tegra, target_freq);
>  	*upper = tegra_actmon_upper_freq(tegra, target_freq);
>  
> -	*lower /= KHZ;
> -	*upper /= KHZ;
> -
>  	/*
>  	 * The upper watermark should take into account CPU's frequency
>  	 * because cpu_to_emc_rate() may override the target_freq with
> @@ -304,10 +314,11 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
>  static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
>  					   struct tegra_devfreq_device *dev)
>  {
> -	unsigned long lower, upper, freq;
> +	unsigned long avg_dependency_freq, lower, upper;
> +
> +	tegra_actmon_get_lower_upper(tegra, dev, dev->avg_freq, &lower, &upper);
>  
> -	freq = dev->avg_count / ACTMON_SAMPLING_PERIOD * KHZ;
> -	tegra_actmon_get_lower_upper(tegra, dev, freq, &lower, &upper);
> +	avg_dependency_freq = tegra_actmon_dev_avg_dependency_freq(tegra, dev);
>  
>  	/*
>  	 * We want to get interrupts when MCCPU client crosses the
> @@ -316,7 +327,7 @@ static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
>  	 */
>  	if (lower < dev->config->avg_dependency_threshold &&
>  	    upper > dev->config->avg_dependency_threshold) {
> -		if (dev->avg_count < dev->config->avg_dependency_threshold)
> +		if (dev->avg_freq < avg_dependency_freq)
>  			upper = dev->config->avg_dependency_threshold;
>  		else
>  			lower = dev->config->avg_dependency_threshold;
> @@ -358,8 +369,7 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
>  	 * device. Once that mark is hit and boosting is stopped, the
>  	 * interrupt is disabled by ISR.
>  	 */
> -	freq = dev->avg_count / ACTMON_SAMPLING_PERIOD * KHZ;
> -	tegra_actmon_get_lower_upper(tegra, dev, freq, &lower, &upper);
> +	tegra_actmon_get_lower_upper(tegra, dev, dev->avg_freq, &lower, &upper);

Also, patch5 newly defined this function and then you edit the function prototype
of this function. It is not proper way.

>  
>  	delta = do_percent(upper - lower, dev->config->boost_down_threshold);
>  	device_writel(dev, lower + delta, ACTMON_DEV_LOWER_WMARK);
> @@ -368,12 +378,14 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
>  static void actmon_isr_device(struct tegra_devfreq *tegra,
>  			      struct tegra_devfreq_device *dev)
>  {
> -	u32 intr_status, dev_ctrl, avg_intr_mask;
> +	u32 intr_status, dev_ctrl, avg_intr_mask, avg_count;
>  
> -	dev->avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
>  	intr_status = device_readl(dev, ACTMON_DEV_INTR_STATUS);
> +	avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
>  	dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
>  
> +	dev->avg_freq = avg_count / ACTMON_SAMPLING_PERIOD;
> +
>  	avg_intr_mask = ACTMON_DEV_INTR_AVG_BELOW_WMARK |
>  			ACTMON_DEV_INTR_AVG_ABOVE_WMARK;
>  
> @@ -427,7 +439,7 @@ static unsigned long actmon_update_target(struct tegra_devfreq *tegra,
>  {
>  	unsigned long target_freq;
>  
> -	target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD + dev->boost_freq;
> +	target_freq = dev->avg_freq + dev->boost_freq;
>  	target_freq = tegra_actmon_account_cpu_freq(tegra, dev, target_freq);
>  
>  	return min(target_freq, tegra->max_freq);
> @@ -464,6 +476,7 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
>  	struct clk_notifier_data *data = ptr;
>  	struct tegra_devfreq_device *dev;
>  	struct tegra_devfreq *tegra;
> +	unsigned long freq;
>  	unsigned int i;
>  
>  	if (action != POST_RATE_CHANGE)
> @@ -471,6 +484,8 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
>  
>  	tegra = container_of(nb, struct tegra_devfreq, rate_change_nb);
>  
> +	freq = data->new_rate / KHZ;
> +
>  	/*
>  	 * EMC rate could change due to three reasons:
>  	 *
> @@ -492,7 +507,7 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
>  		dev = &tegra->devices[i];
>  
>  		tegra_devfreq_update_avg_wmark(tegra, dev);
> -		tegra_devfreq_update_wmark(tegra, dev, data->new_rate);
> +		tegra_devfreq_update_wmark(tegra, dev, freq);
>  	}
>  
>  	return NOTIFY_OK;
> @@ -501,14 +516,14 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
>  static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
>  					  struct tegra_devfreq_device *dev)
>  {
> -	u32 val = 0, target_freq;
> +	u32 val = 0;
>  
> -	target_freq = clk_get_rate(tegra->emc_clock) / KHZ;
> -	dev->avg_count = target_freq * ACTMON_SAMPLING_PERIOD;
> -	device_writel(dev, dev->avg_count, ACTMON_DEV_INIT_AVG);
> +	dev->avg_freq = clk_get_rate(tegra->emc_clock) / KHZ;
> +	device_writel(dev, dev->avg_freq * ACTMON_SAMPLING_PERIOD,
> +		      ACTMON_DEV_INIT_AVG);
>  
>  	tegra_devfreq_update_avg_wmark(tegra, dev);
> -	tegra_devfreq_update_wmark(tegra, dev, target_freq);
> +	tegra_devfreq_update_wmark(tegra, dev, dev->avg_freq);
>  
>  	device_writel(dev, ACTMON_COUNT_WEIGHT, ACTMON_DEV_COUNT_WEIGHT);
>  	device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
> @@ -572,7 +587,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>  	rate = dev_pm_opp_get_freq(opp);
>  	dev_pm_opp_put(opp);
>  
> -	err = clk_set_min_rate(tegra->emc_clock, rate);
> +	err = clk_set_min_rate(tegra->emc_clock, rate * KHZ);
>  	if (err)
>  		return err;
>  
> @@ -595,7 +610,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>  	struct tegra_devfreq_device *actmon_dev;
>  	unsigned long cur_freq;
>  
> -	cur_freq = clk_get_rate(tegra->emc_clock);
> +	cur_freq = clk_get_rate(tegra->emc_clock) / KHZ;
>  
>  	/* To be used by the tegra governor */
>  	stat->private_data = tegra;
> @@ -612,7 +627,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>  	stat->busy_time *= 100 / BUS_SATURATION_RATIO;
>  
>  	/* Number of cycles in a sampling period */
> -	stat->total_time = cur_freq / KHZ * ACTMON_SAMPLING_PERIOD;
> +	stat->total_time = cur_freq * ACTMON_SAMPLING_PERIOD;
>  
>  	stat->busy_time = min(stat->busy_time, stat->total_time);
>  
> @@ -652,7 +667,7 @@ static int tegra_governor_get_target(struct devfreq *devfreq,
>  		target_freq = max(target_freq, dev_target_freq);
>  	}
>  
> -	*freq = target_freq * KHZ;
> +	*freq = target_freq;
>  
>  	return 0;
>  }
> @@ -787,7 +802,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  			goto remove_opps;
>  		}
>  
> -		err = dev_pm_opp_add(&pdev->dev, rate, 0);
> +		err = dev_pm_opp_add(&pdev->dev, rate / KHZ, 0);
>  		if (err) {
>  			dev_err(&pdev->dev, "Failed to add OPP: %d\n", err);
>  			goto remove_opps;
> @@ -812,6 +827,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  	}
>  
>  	tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
> +	tegra_devfreq_profile.initial_freq /= KHZ;
> +
>  	devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
>  				     "tegra_actmon", NULL);
>  	if (IS_ERR(devfreq)) {
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v6 09/19] PM / devfreq: tegra30: Use kHz units uniformly in the code

[Dmitry Osipenko]

02.10.2019 02:29, Chanwoo Choi пишет:
> On 19. 8. 12. 오전 6:23, Dmitry Osipenko wrote:
>> Now that all kHz-conversion related bugs are fixed, we can use the kHz
>> uniformly. This makes code cleaner and avoids integer divisions in the
>> code, which is useful in a case of Tegra30 that has Cortex A9 CPU that
>> doesn't support integer division instructions, hence all divisions are
>> actually made in software mode. Another small benefit from this change
>> is that now powertop utility correctly displays devfreq's stats, for
>> some reason it expects them to be in kHz.
> 
> If possible, please specify the benefit result on patch description.

As I wrote above, there are fewer divisions in the code as a result of this patch.

Then I also found that powertop expects KHz. This is actually something that devfreq core
potentially could improve by allowing drivers to specify what units are used for the freqs
such that sysfs interface could present freqs to userpspace in a predictable manner.

Lastly, this patch comes very handy for the patch11, because of the replacement of
avg_count with avg_freq which helps to keep code cleaner in further patches. Please note
that this patch doesn't change logic of the code.

> And I have a question. Why do you fix the KHz-conversion issue on one patch?
> Actually, in my case, it is difficult to understand that multiple patches
> try to fix the KHz-conversion issue. I think that it is possible to
> make one patch.

This driver used Hz units for the OPPs from the very beginning and then there were
Hz<->KHz conversion bugs that were fixed by previous patches. This patch doesn't fix any
KHz-conversion issue and merely makes a minor clean up by using KHz units everywhere in
the code, starting from OPPs that are created by dev_pm_opp_add().

> And, 
> On these series, some codes wad added and then these codes are deleted
> on later patch. It looks like that you made the issue and then you fix
> the issue by yourself. I think that it is not proper.
> Even if you developed the patches on your local environment sequentially
> according to the sequence of your issue detection, you better to do
> refactoring the patches.
> 
> Frankly, I cannot agree that some codes wad added on front patch
> and then added codes are deleted on later patch in the same patchset.

Alright, I understand that it makes easier for you to review patches without going back
and forth between patches, verifying every changed line of the previous patches of this
series.

>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/tegra30-devfreq.c | 81 +++++++++++++++++++------------
>>  1 file changed, 49 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>> index ca499368ee81..43d50b4366dd 100644
>> --- a/drivers/devfreq/tegra30-devfreq.c
>> +++ b/drivers/devfreq/tegra30-devfreq.c
>> @@ -137,8 +137,11 @@ struct tegra_devfreq_device {
>>  	const struct tegra_devfreq_device_config *config;
>>  	void __iomem *regs;
>>  
>> -	/* Average event count sampled in the last interrupt */
>> -	u32 avg_count;
>> +	/*
>> +	 * Average event count sampled in the last interrupt and converted
>> +	 * to frequency value.
>> +	 */
>> +	u32 avg_freq;
>>  
>>  	/*
>>  	 * Extra frequency to increase the target by due to consecutive
>> @@ -222,6 +225,14 @@ static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra)
>>  	return 0;
>>  }
>>  
>> +static unsigned long
>> +tegra_actmon_dev_avg_dependency_freq(struct tegra_devfreq *tegra,
>> +				     struct tegra_devfreq_device *dev)
>> +{
>> +	return dev->config->avg_dependency_threshold /
>> +		ACTMON_SAMPLING_PERIOD;
>> +}
>> +
>>  static unsigned long
>>  tegra_actmon_account_cpu_freq(struct tegra_devfreq *tegra,
>>  			      struct tegra_devfreq_device *dev,
>> @@ -229,13 +240,15 @@ tegra_actmon_account_cpu_freq(struct tegra_devfreq *tegra,
>>  {
>>  	unsigned long static_cpu_emc_freq;
>>  
>> -	if (dev->config->avg_dependency_threshold &&
>> -	    dev->config->avg_dependency_threshold < dev->avg_count) {
>> +	if (!dev->config->avg_dependency_threshold)
>> +		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);
>> -		target_freq = max(target_freq, static_cpu_emc_freq);
>> -	}
>> +	else
>> +		static_cpu_emc_freq = 0;
>>  
>> -	return target_freq;
>> +	return max(target_freq, static_cpu_emc_freq);
>>  }
>>  
>>  static unsigned long tegra_actmon_lower_freq(struct tegra_devfreq *tegra,
>> @@ -261,7 +274,7 @@ static unsigned long tegra_actmon_upper_freq(struct tegra_devfreq *tegra,
>>  
>>  	opp = dev_pm_opp_find_freq_ceil(tegra->devfreq->dev.parent, &upper);
>>  	if (IS_ERR(opp))
>> -		upper = ULONG_MAX;
>> +		upper = KHZ_MAX;
>>  	else
>>  		dev_pm_opp_put(opp);
>>  
>> @@ -280,15 +293,12 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
>>  	 * range in a case where target_freq falls into a range of
>>  	 * next_possible_opp_freq - 1MHz.
>>  	 */
>> -	target_freq = round_down(target_freq, 1000000);
> 
> This line was added on patch5. I think that you could fix the KHz-conversion
> on patch5 instead of this patch. It is not good way to make the patches.
> Because some codes wad added and then these codes are deleted on later patch.
> 
> It looks like that you made the issue and then you fix the issue by yourself.
> It is difficult to make me to decide this patch is either proper or not.

The OPPs were always defined in Hz units in this driver. The patch5 was created much
earlier than this patch and thus I'm making changes here to the code that was added in patch5.

Okay, I'll try to change the order of the patches. But usually it takes a lot if time and
effort to rebase patches and then re-test them, so I'm trying to avoid that when not
absolutely necessary.

>> +	target_freq = round_down(target_freq, 1000);
>>  
>>  	/* watermarks are set at the borders of the corresponding OPPs */
>>  	*lower = tegra_actmon_lower_freq(tegra, target_freq);
>>  	*upper = tegra_actmon_upper_freq(tegra, target_freq);
>>  
>> -	*lower /= KHZ;
>> -	*upper /= KHZ;
>> -
>>  	/*
>>  	 * The upper watermark should take into account CPU's frequency
>>  	 * because cpu_to_emc_rate() may override the target_freq with
>> @@ -304,10 +314,11 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
>>  static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
>>  					   struct tegra_devfreq_device *dev)
>>  {
>> -	unsigned long lower, upper, freq;
>> +	unsigned long avg_dependency_freq, lower, upper;
>> +
>> +	tegra_actmon_get_lower_upper(tegra, dev, dev->avg_freq, &lower, &upper);
>>  
>> -	freq = dev->avg_count / ACTMON_SAMPLING_PERIOD * KHZ;
>> -	tegra_actmon_get_lower_upper(tegra, dev, freq, &lower, &upper);
>> +	avg_dependency_freq = tegra_actmon_dev_avg_dependency_freq(tegra, dev);
>>  
>>  	/*
>>  	 * We want to get interrupts when MCCPU client crosses the
>> @@ -316,7 +327,7 @@ static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
>>  	 */
>>  	if (lower < dev->config->avg_dependency_threshold &&
>>  	    upper > dev->config->avg_dependency_threshold) {
>> -		if (dev->avg_count < dev->config->avg_dependency_threshold)
>> +		if (dev->avg_freq < avg_dependency_freq)
>>  			upper = dev->config->avg_dependency_threshold;
>>  		else
>>  			lower = dev->config->avg_dependency_threshold;
>> @@ -358,8 +369,7 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
>>  	 * device. Once that mark is hit and boosting is stopped, the
>>  	 * interrupt is disabled by ISR.
>>  	 */
>> -	freq = dev->avg_count / ACTMON_SAMPLING_PERIOD * KHZ;
>> -	tegra_actmon_get_lower_upper(tegra, dev, freq, &lower, &upper);
>> +	tegra_actmon_get_lower_upper(tegra, dev, dev->avg_freq, &lower, &upper);
> 
> Also, patch5 newly defined this function and then you edit the function prototype
> of this function. It is not proper way.

Sorry, I'm not sure what you're meaning by the "prototype" here. In case of this
particular hunk, the freq==dev->avg_freq and dev->avg_count is replaced with dev->avg_freq
in this patch as well. Hence this is a cosmetic change that doesn't change any logic of
the code. I'll see if this all could be made a bit more clear and easier for review.

[snip]

[PATCH v6 10/19] PM / devfreq: tegra30: Reduce unnecessary interrupts activity

[Dmitry Osipenko]

There are cases where unnecessary ACTMON interrupts could be avoided,
like when one memory client device requests higher clock rate than the
other or when clock rate is manually limited using sysfs devfreq
parameters. These cases could be avoided by tuning upper watermark or
disabling hardware events when min/max boosting thresholds are reached.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 87 ++++++++++++++++++++++++++++---
 1 file changed, 80 insertions(+), 7 deletions(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 43d50b4366dd..a2623de56d20 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -312,7 +312,8 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
 }
 
 static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
-					   struct tegra_devfreq_device *dev)
+					   struct tegra_devfreq_device *dev,
+					   unsigned long freq)
 {
 	unsigned long avg_dependency_freq, lower, upper;
 
@@ -320,6 +321,22 @@ static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
 
 	avg_dependency_freq = tegra_actmon_dev_avg_dependency_freq(tegra, dev);
 
+	/*
+	 * If cumulative EMC frequency selection (MCALL / min_freq) is
+	 * higher than the device's, then there is no need to set upper
+	 * watermark to a lower value because it will result in unnecessary
+	 * upper interrupts.
+	 *
+	 * Note that average watermarks are also updated after EMC
+	 * clock rate change, hence if clock rate goes down, then the
+	 * watermarks will be set in accordance to the new rate after
+	 * changing the rate. There are other ways to achieve the same
+	 * result, but this one is probably the least churning, although
+	 * it may look a bit convoluted.
+	 */
+	if (freq * ACTMON_SAMPLING_PERIOD > upper)
+		upper = freq * ACTMON_SAMPLING_PERIOD;
+
 	/*
 	 * We want to get interrupts when MCCPU client crosses the
 	 * dependency threshold in order to take into / out of account
@@ -361,7 +378,18 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
 	tegra_actmon_get_lower_upper(tegra, dev, freq - 1, &lower, &upper);
 
 	delta = do_percent(upper - lower, dev->config->boost_up_threshold);
-	device_writel(dev, lower + delta, ACTMON_DEV_UPPER_WMARK);
+
+	/*
+	 * The memory events count could go a bit higher than the maximum
+	 * defined by the OPPs, hence make the upper watermark infinitely
+	 * high to avoid unnecessary upper interrupts in that case.
+	 */
+	if (freq == tegra->max_freq)
+		upper = ULONG_MAX;
+	else
+		upper = lower + delta;
+
+	device_writel(dev, upper, ACTMON_DEV_UPPER_WMARK);
 
 	/*
 	 * Meanwhile the lower mark is based on the average value
@@ -379,6 +407,7 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
 			      struct tegra_devfreq_device *dev)
 {
 	u32 intr_status, dev_ctrl, avg_intr_mask, avg_count;
+	unsigned long freq;
 
 	intr_status = device_readl(dev, ACTMON_DEV_INTR_STATUS);
 	avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
@@ -389,8 +418,10 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
 	avg_intr_mask = ACTMON_DEV_INTR_AVG_BELOW_WMARK |
 			ACTMON_DEV_INTR_AVG_ABOVE_WMARK;
 
-	if (intr_status & avg_intr_mask)
-		tegra_devfreq_update_avg_wmark(tegra, dev);
+	if (intr_status & avg_intr_mask) {
+		freq = clk_get_rate(tegra->emc_clock) / KHZ;
+		tegra_devfreq_update_avg_wmark(tegra, dev, freq);
+	}
 
 	if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_UPPER) {
 		/*
@@ -412,6 +443,8 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
 		dev->boost_freq = do_percent(dev->boost_freq,
 					     dev->config->boost_down_coeff);
 
+		dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
+
 		if (dev->boost_freq < (ACTMON_BOOST_FREQ_STEP >> 1))
 			dev->boost_freq = 0;
 	}
@@ -427,8 +460,16 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
 	}
 
 	/* no boosting => no need for consecutive-down interrupt */
-	if (dev->boost_freq == 0)
+	if (dev->boost_freq == 0) {
 		dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+		dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
+	}
+
+	/* boosting max-out => no need for consecutive-up interrupt */
+	if (dev->boost_freq == tegra->max_freq) {
+		dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
+		dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+	}
 
 	device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
 	device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
@@ -437,8 +478,40 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
 static unsigned long actmon_update_target(struct tegra_devfreq *tegra,
 					  struct tegra_devfreq_device *dev)
 {
+	u32 avg_count, avg_freq, old_upper, new_upper, dev_ctrl;
 	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.
+	 */
+	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) {
+		if (dev->boost_freq == tegra->max_freq) {
+			dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
+
+			dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+			dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
+
+			device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
+		}
+
+		dev->avg_freq = avg_freq;
+		dev->boost_freq = 0;
+	}
+
 	target_freq = dev->avg_freq + dev->boost_freq;
 	target_freq = tegra_actmon_account_cpu_freq(tegra, dev, target_freq);
 
@@ -506,7 +579,7 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
 	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
 		dev = &tegra->devices[i];
 
-		tegra_devfreq_update_avg_wmark(tegra, dev);
+		tegra_devfreq_update_avg_wmark(tegra, dev, freq);
 		tegra_devfreq_update_wmark(tegra, dev, freq);
 	}
 
@@ -522,7 +595,7 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
 	device_writel(dev, dev->avg_freq * ACTMON_SAMPLING_PERIOD,
 		      ACTMON_DEV_INIT_AVG);
 
-	tegra_devfreq_update_avg_wmark(tegra, dev);
+	tegra_devfreq_update_avg_wmark(tegra, dev, dev->avg_freq);
 	tegra_devfreq_update_wmark(tegra, dev, dev->avg_freq);
 
 	device_writel(dev, ACTMON_COUNT_WEIGHT, ACTMON_DEV_COUNT_WEIGHT);
-- 
2.22.0

Re: [PATCH v6 10/19] PM / devfreq: tegra30: Reduce unnecessary interrupts activity

[Chanwoo Choi]

Hi,

On 19. 8. 12. 오전 6:23, Dmitry Osipenko wrote:
> There are cases where unnecessary ACTMON interrupts could be avoided,
> like when one memory client device requests higher clock rate than the
> other or when clock rate is manually limited using sysfs devfreq
> parameters. These cases could be avoided by tuning upper watermark or
> disabling hardware events when min/max boosting thresholds are reached.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra30-devfreq.c | 87 ++++++++++++++++++++++++++++---
>  1 file changed, 80 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 43d50b4366dd..a2623de56d20 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -312,7 +312,8 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
>  }
>  
>  static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
> -					   struct tegra_devfreq_device *dev)
> +					   struct tegra_devfreq_device *dev,
> +					   unsigned long freq)
>  {
>  	unsigned long avg_dependency_freq, lower, upper;
>  
> @@ -320,6 +321,22 @@ static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
>  
>  	avg_dependency_freq = tegra_actmon_dev_avg_dependency_freq(tegra, dev);
>  
> +	/*
> +	 * If cumulative EMC frequency selection (MCALL / min_freq) is
> +	 * higher than the device's, then there is no need to set upper
> +	 * watermark to a lower value because it will result in unnecessary
> +	 * upper interrupts.
> +	 *
> +	 * Note that average watermarks are also updated after EMC
> +	 * clock rate change, hence if clock rate goes down, then the
> +	 * watermarks will be set in accordance to the new rate after
> +	 * changing the rate. There are other ways to achieve the same
> +	 * result, but this one is probably the least churning, although
> +	 * it may look a bit convoluted.
> +	 */
> +	if (freq * ACTMON_SAMPLING_PERIOD > upper)
> +		upper = freq * ACTMON_SAMPLING_PERIOD;
> +
>  	/*
>  	 * We want to get interrupts when MCCPU client crosses the
>  	 * dependency threshold in order to take into / out of account
> @@ -361,7 +378,18 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
>  	tegra_actmon_get_lower_upper(tegra, dev, freq - 1, &lower, &upper);
>  
>  	delta = do_percent(upper - lower, dev->config->boost_up_threshold);
> -	device_writel(dev, lower + delta, ACTMON_DEV_UPPER_WMARK);


Also, this patch edits the added codes on front patch.
This code was added on patch5 and then delete it on this patch.
If it is not necessary, you can remove it on patch5 by refactoring.

> +
> +	/*
> +	 * The memory events count could go a bit higher than the maximum
> +	 * defined by the OPPs, hence make the upper watermark infinitely
> +	 * high to avoid unnecessary upper interrupts in that case.
> +	 */
> +	if (freq == tegra->max_freq)
> +		upper = ULONG_MAX;
> +	else
> +		upper = lower + delta;
> +
> +	device_writel(dev, upper, ACTMON_DEV_UPPER_WMARK);

I think that the changes of tegra_devfreq_update_avg_wmark() on this patch
can be merged to patch5.

>  
>  	/*
>  	 * Meanwhile the lower mark is based on the average value
> @@ -379,6 +407,7 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>  			      struct tegra_devfreq_device *dev)
>  {
>  	u32 intr_status, dev_ctrl, avg_intr_mask, avg_count;
> +	unsigned long freq;
>  
>  	intr_status = device_readl(dev, ACTMON_DEV_INTR_STATUS);
>  	avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
> @@ -389,8 +418,10 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>  	avg_intr_mask = ACTMON_DEV_INTR_AVG_BELOW_WMARK |
>  			ACTMON_DEV_INTR_AVG_ABOVE_WMARK;
>  
> -	if (intr_status & avg_intr_mask)
> -		tegra_devfreq_update_avg_wmark(tegra, dev);
> +	if (intr_status & avg_intr_mask) {
> +		freq = clk_get_rate(tegra->emc_clock) / KHZ;
> +		tegra_devfreq_update_avg_wmark(tegra, dev, freq);
> +	}
>  
>  	if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_UPPER) {
>  		/*
> @@ -412,6 +443,8 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>  		dev->boost_freq = do_percent(dev->boost_freq,
>  					     dev->config->boost_down_coeff);
>  
> +		dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
> +
>  		if (dev->boost_freq < (ACTMON_BOOST_FREQ_STEP >> 1))
>  			dev->boost_freq = 0;
>  	}
> @@ -427,8 +460,16 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>  	}
>  
>  	/* no boosting => no need for consecutive-down interrupt */
> -	if (dev->boost_freq == 0)
> +	if (dev->boost_freq == 0) {
>  		dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
> +		dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
> +	}
> +
> +	/* boosting max-out => no need for consecutive-up interrupt */
> +	if (dev->boost_freq == tegra->max_freq) {
> +		dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
> +		dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
> +	}
>  
>  	device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
>  	device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
> @@ -437,8 +478,40 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>  static unsigned long actmon_update_target(struct tegra_devfreq *tegra,
>  					  struct tegra_devfreq_device *dev)
>  {
> +	u32 avg_count, avg_freq, old_upper, new_upper, dev_ctrl;
>  	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.
> +	 */
> +	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) {
> +		if (dev->boost_freq == tegra->max_freq) {
> +			dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
> +
> +			dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
> +			dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
> +
> +			device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
> +		}
> +
> +		dev->avg_freq = avg_freq;
> +		dev->boost_freq = 0;
> +	}
> +
>  	target_freq = dev->avg_freq + dev->boost_freq;
>  	target_freq = tegra_actmon_account_cpu_freq(tegra, dev, target_freq);
>  
> @@ -506,7 +579,7 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
>  	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
>  		dev = &tegra->devices[i];
>  
> -		tegra_devfreq_update_avg_wmark(tegra, dev);
> +		tegra_devfreq_update_avg_wmark(tegra, dev, freq);
>  		tegra_devfreq_update_wmark(tegra, dev, freq);
>  	}
>  
> @@ -522,7 +595,7 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
>  	device_writel(dev, dev->avg_freq * ACTMON_SAMPLING_PERIOD,
>  		      ACTMON_DEV_INIT_AVG);
>  
> -	tegra_devfreq_update_avg_wmark(tegra, dev);
> +	tegra_devfreq_update_avg_wmark(tegra, dev, dev->avg_freq);
>  	tegra_devfreq_update_wmark(tegra, dev, dev->avg_freq);
>  
>  	device_writel(dev, ACTMON_COUNT_WEIGHT, ACTMON_DEV_COUNT_WEIGHT);
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v6 10/19] PM / devfreq: tegra30: Reduce unnecessary interrupts activity

[Dmitry Osipenko]

02.10.2019 02:35, Chanwoo Choi пишет:
> Hi,
> 
> On 19. 8. 12. 오전 6:23, Dmitry Osipenko wrote:
>> There are cases where unnecessary ACTMON interrupts could be avoided,
>> like when one memory client device requests higher clock rate than the
>> other or when clock rate is manually limited using sysfs devfreq
>> parameters. These cases could be avoided by tuning upper watermark or
>> disabling hardware events when min/max boosting thresholds are reached.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/tegra30-devfreq.c | 87 ++++++++++++++++++++++++++++---
>>  1 file changed, 80 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>> index 43d50b4366dd..a2623de56d20 100644
>> --- a/drivers/devfreq/tegra30-devfreq.c
>> +++ b/drivers/devfreq/tegra30-devfreq.c
>> @@ -312,7 +312,8 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
>>  }
>>  
>>  static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
>> -					   struct tegra_devfreq_device *dev)
>> +					   struct tegra_devfreq_device *dev,
>> +					   unsigned long freq)
>>  {
>>  	unsigned long avg_dependency_freq, lower, upper;
>>  
>> @@ -320,6 +321,22 @@ static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
>>  
>>  	avg_dependency_freq = tegra_actmon_dev_avg_dependency_freq(tegra, dev);
>>  
>> +	/*
>> +	 * If cumulative EMC frequency selection (MCALL / min_freq) is
>> +	 * higher than the device's, then there is no need to set upper
>> +	 * watermark to a lower value because it will result in unnecessary
>> +	 * upper interrupts.
>> +	 *
>> +	 * Note that average watermarks are also updated after EMC
>> +	 * clock rate change, hence if clock rate goes down, then the
>> +	 * watermarks will be set in accordance to the new rate after
>> +	 * changing the rate. There are other ways to achieve the same
>> +	 * result, but this one is probably the least churning, although
>> +	 * it may look a bit convoluted.
>> +	 */
>> +	if (freq * ACTMON_SAMPLING_PERIOD > upper)
>> +		upper = freq * ACTMON_SAMPLING_PERIOD;
>> +
>>  	/*
>>  	 * We want to get interrupts when MCCPU client crosses the
>>  	 * dependency threshold in order to take into / out of account
>> @@ -361,7 +378,18 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
>>  	tegra_actmon_get_lower_upper(tegra, dev, freq - 1, &lower, &upper);
>>  
>>  	delta = do_percent(upper - lower, dev->config->boost_up_threshold);
>> -	device_writel(dev, lower + delta, ACTMON_DEV_UPPER_WMARK);
> 
> 
> Also, this patch edits the added codes on front patch.
> This code was added on patch5 and then delete it on this patch.
> If it is not necessary, you can remove it on patch5 by refactoring.
> 
>> +
>> +	/*
>> +	 * The memory events count could go a bit higher than the maximum
>> +	 * defined by the OPPs, hence make the upper watermark infinitely
>> +	 * high to avoid unnecessary upper interrupts in that case.
>> +	 */
>> +	if (freq == tegra->max_freq)
>> +		upper = ULONG_MAX;
>> +	else
>> +		upper = lower + delta;
>> +
>> +	device_writel(dev, upper, ACTMON_DEV_UPPER_WMARK);
> 
> I think that the changes of tegra_devfreq_update_avg_wmark() on this patch
> can be merged to patch5.

Okay, I'll revisit these parts of tegra_devfreq_update_avg_wmark() and will move them to
patch5 if there won't be any major obstacles.

[PATCH v6 11/19] PM / devfreq: tegra30: Use CPUFreq notifier

[Dmitry Osipenko]

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]);
+
+	return err;
 }
 
 static void tegra_actmon_stop(struct tegra_devfreq *tegra)
@@ -636,11 +761,13 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
 
 	disable_irq(tegra->irq);
 
-	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
-		device_writel(&tegra->devices[i], 0x00000000, ACTMON_DEV_CTRL);
-		device_writel(&tegra->devices[i], ACTMON_INTR_STATUS_CLEAR,
-			      ACTMON_DEV_INTR_STATUS);
-	}
+	cpufreq_unregister_notifier(&tegra->cpu_rate_change_nb,
+				    CPUFREQ_TRANSITION_NOTIFIER);
+
+	cancel_delayed_work_sync(&tegra->cpufreq_update_work);
+
+	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
+		tegra_actmon_stop_device(&tegra->devices[i]);
 }
 
 static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
@@ -749,6 +876,7 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
 					unsigned int event, void *data)
 {
 	struct tegra_devfreq *tegra = dev_get_drvdata(devfreq->dev.parent);
+	int ret = 0;
 
 	/*
 	 * Couple device with the governor early as it is needed at
@@ -759,7 +887,7 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
 	switch (event) {
 	case DEVFREQ_GOV_START:
 		devfreq_monitor_start(devfreq);
-		tegra_actmon_start(tegra);
+		ret = tegra_actmon_start(tegra);
 		break;
 
 	case DEVFREQ_GOV_STOP:
@@ -774,11 +902,11 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
 
 	case DEVFREQ_GOV_RESUME:
 		devfreq_monitor_resume(devfreq);
-		tegra_actmon_start(tegra);
+		ret = tegra_actmon_start(tegra);
 		break;
 	}
 
-	return 0;
+	return ret;
 }
 
 static struct devfreq_governor tegra_devfreq_governor = {
@@ -884,9 +1012,14 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, tegra);
 
-	tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
+	tegra->cpu_rate_change_nb.notifier_call = tegra_actmon_cpu_notify_cb;
+
+	INIT_DELAYED_WORK(&tegra->cpufreq_update_work,
+			  tegra_actmon_delayed_update);
+
+	tegra->clk_rate_change_nb.notifier_call = tegra_actmon_clk_notify_cb;
 	err = clk_notifier_register(tegra->emc_clock,
-				    &tegra->rate_change_nb);
+				    &tegra->clk_rate_change_nb);
 	if (err) {
 		dev_err(&pdev->dev,
 			"Failed to register rate change notifier\n");
@@ -915,7 +1048,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	devfreq_remove_governor(&tegra_devfreq_governor);
 
 unreg_notifier:
-	clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb);
+	clk_notifier_unregister(tegra->emc_clock, &tegra->clk_rate_change_nb);
 
 remove_opps:
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);
@@ -933,7 +1066,7 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
 	devfreq_remove_device(tegra->devfreq);
 	devfreq_remove_governor(&tegra_devfreq_governor);
 
-	clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb);
+	clk_notifier_unregister(tegra->emc_clock, &tegra->clk_rate_change_nb);
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);
 
 	reset_control_reset(tegra->reset);
-- 
2.22.0

Re: [PATCH v6 11/19] PM / devfreq: tegra30: Use CPUFreq notifier

[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); 

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

(snip)


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v6 11/19] PM / devfreq: tegra30: Use CPUFreq notifier

[Dmitry Osipenko]

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!

[PATCH v6 12/19] PM / devfreq: tegra30: Move clk-notifier's registration to governor's start

[Dmitry Osipenko]

There is no point in receiving of the notifications while governor is
stopped, let's keep them disabled like we do for the CPU freq-change
notifications. This also fixes a potential use-after-free bug if
notification happens after device's removal.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 33 ++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index a260812f7744..bad9836b1eea 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -726,6 +726,19 @@ static int tegra_actmon_start(struct tegra_devfreq *tegra)
 	actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
 		      ACTMON_GLB_PERIOD_CTRL);
 
+	/*
+	 * CLK notifications are needed in order to reconfigure the upper
+	 * consecutive watermark in accordance to the actual clock rate
+	 * to avoid unnecessary upper interrupts.
+	 */
+	err = clk_notifier_register(tegra->emc_clock,
+				    &tegra->clk_rate_change_nb);
+	if (err) {
+		dev_err(tegra->devfreq->dev.parent,
+			"Failed to register rate change notifier\n");
+		return err;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
 		tegra_actmon_configure_device(tegra, &tegra->devices[i]);
 
@@ -752,6 +765,8 @@ static int tegra_actmon_start(struct tegra_devfreq *tegra)
 	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
 		tegra_actmon_stop_device(&tegra->devices[i]);
 
+	clk_notifier_unregister(tegra->emc_clock, &tegra->clk_rate_change_nb);
+
 	return err;
 }
 
@@ -768,6 +783,8 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
 
 	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
 		tegra_actmon_stop_device(&tegra->devices[i]);
+
+	clk_notifier_unregister(tegra->emc_clock, &tegra->clk_rate_change_nb);
 }
 
 static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
@@ -1012,24 +1029,16 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, tegra);
 
+	tegra->clk_rate_change_nb.notifier_call = tegra_actmon_clk_notify_cb;
 	tegra->cpu_rate_change_nb.notifier_call = tegra_actmon_cpu_notify_cb;
 
 	INIT_DELAYED_WORK(&tegra->cpufreq_update_work,
 			  tegra_actmon_delayed_update);
 
-	tegra->clk_rate_change_nb.notifier_call = tegra_actmon_clk_notify_cb;
-	err = clk_notifier_register(tegra->emc_clock,
-				    &tegra->clk_rate_change_nb);
-	if (err) {
-		dev_err(&pdev->dev,
-			"Failed to register rate change notifier\n");
-		goto remove_opps;
-	}
-
 	err = devfreq_add_governor(&tegra_devfreq_governor);
 	if (err) {
 		dev_err(&pdev->dev, "Failed to add governor: %d\n", err);
-		goto unreg_notifier;
+		goto remove_opps;
 	}
 
 	tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
@@ -1047,9 +1056,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 remove_governor:
 	devfreq_remove_governor(&tegra_devfreq_governor);
 
-unreg_notifier:
-	clk_notifier_unregister(tegra->emc_clock, &tegra->clk_rate_change_nb);
-
 remove_opps:
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);
 
@@ -1066,7 +1072,6 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
 	devfreq_remove_device(tegra->devfreq);
 	devfreq_remove_governor(&tegra_devfreq_governor);
 
-	clk_notifier_unregister(tegra->emc_clock, &tegra->clk_rate_change_nb);
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);
 
 	reset_control_reset(tegra->reset);
-- 
2.22.0

[PATCH v6 13/19] PM / devfreq: tegra30: Reset boosting on startup

[Dmitry Osipenko]

Governor could be stopped while boosting is active. We have assumption
that everything is reset on governor's restart, including the boosting
value, which was missed.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index bad9836b1eea..5002dca4c403 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -686,6 +686,9 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
 {
 	u32 val = 0;
 
+	/* reset boosting on governor's restart */
+	dev->boost_freq = 0;
+
 	dev->avg_freq = clk_get_rate(tegra->emc_clock) / KHZ;
 	device_writel(dev, dev->avg_freq * ACTMON_SAMPLING_PERIOD,
 		      ACTMON_DEV_INIT_AVG);
-- 
2.22.0

[PATCH v6 14/19] PM / devfreq: tegra30: Don't enable consecutive-down interrupt on startup

[Dmitry Osipenko]

The consecutive-down event tells that we should perform frequency
de-boosting, but boosting is in a reset state on start and hence the
event won't do anything useful for us and it will be just a dummy
interrupt request.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 5002dca4c403..a0a1ac09a824 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -708,7 +708,6 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
 		<< ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_NUM_SHIFT;
 	val |= ACTMON_DEV_CTRL_AVG_ABOVE_WMARK_EN;
 	val |= ACTMON_DEV_CTRL_AVG_BELOW_WMARK_EN;
-	val |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
 	val |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
 	val |= ACTMON_DEV_CTRL_ENB;
 
-- 
2.22.0

Re: [PATCH v6 14/19] PM / devfreq: tegra30: Don't enable consecutive-down interrupt on startup

[Chanwoo Choi]

On 19. 8. 12. 오전 6:23, Dmitry Osipenko wrote:
> The consecutive-down event tells that we should perform frequency
> de-boosting, but boosting is in a reset state on start and hence the
> event won't do anything useful for us and it will be just a dummy
> interrupt request.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra30-devfreq.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 5002dca4c403..a0a1ac09a824 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -708,7 +708,6 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
>  		<< ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_NUM_SHIFT;
>  	val |= ACTMON_DEV_CTRL_AVG_ABOVE_WMARK_EN;
>  	val |= ACTMON_DEV_CTRL_AVG_BELOW_WMARK_EN;
> -	val |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
>  	val |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
>  	val |= ACTMON_DEV_CTRL_ENB;
>  
> 

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[PATCH v6 15/19] PM / devfreq: tegra30: Constify structs

[Dmitry Osipenko]

Constify unmodifiable structs, for consistency.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index a0a1ac09a824..6446bf705b97 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -109,7 +109,7 @@ enum tegra_actmon_device {
 	MCCPU,
 };
 
-static struct tegra_devfreq_device_config actmon_device_configs[] = {
+static const struct tegra_devfreq_device_config actmon_device_configs[] = {
 	{
 		/* MCALL: All memory accesses (including from the CPUs) */
 		.offset = 0x1c0,
@@ -177,7 +177,7 @@ struct tegra_actmon_emc_ratio {
 	unsigned long emc_freq;
 };
 
-static struct tegra_actmon_emc_ratio actmon_emc_ratios[] = {
+static const struct tegra_actmon_emc_ratio actmon_emc_ratios[] = {
 	{ 1400000,    KHZ_MAX },
 	{ 1200000,    750000 },
 	{ 1100000,    600000 },
@@ -216,7 +216,7 @@ static unsigned long do_percent(unsigned long val, unsigned int pct)
 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;
+	const struct tegra_actmon_emc_ratio *ratio = actmon_emc_ratios;
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(actmon_emc_ratios); i++, ratio++) {
-- 
2.22.0

[PATCH v6 16/19] PM / devfreq: tegra30: Include appropriate header

[Dmitry Osipenko]

It's not very correct to include mod_devicetable.h for the OF device
drivers and of_device.h should be included instead.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 6446bf705b97..9753985c9131 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -13,7 +13,7 @@
 #include <linux/io.h>
 #include <linux/irq.h>
 #include <linux/module.h>
-#include <linux/mod_devicetable.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_opp.h>
 #include <linux/reset.h>
-- 
2.22.0

[PATCH v6 17/19] PM / devfreq: tegra30: Increase sampling period to 16ms

[Dmitry Osipenko]

Increase sampling period by 4ms to get a nicer pow2 value, converting
diving into shifts in the code. That's more preferable for Tegra30 that
doesn't have hardware divider instructions because of older Cortex-A9 CPU.
In a result boosting events are delayed by 4ms, which is not sensible in
practice at all.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 9753985c9131..8adc0ff48b45 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -69,7 +69,7 @@
  * translates to 2 ^ (K_VAL + 1). ex: 2 ^ (6 + 1) = 128
  */
 #define ACTMON_AVERAGE_WINDOW_LOG2				6
-#define ACTMON_SAMPLING_PERIOD					12 /* ms */
+#define ACTMON_SAMPLING_PERIOD					16 /* ms */
 
 #define KHZ							1000
 
-- 
2.22.0

[PATCH v6 18/19] PM / devfreq: tegra30: Support variable polling interval

[Dmitry Osipenko]

The ACTMON governor is interrupt-driven and currently hardware's polling
interval is fixed to 16ms in the driver. Devfreq supports variable polling
interval by the generic governors, let's re-use the generic interface for
changing of the polling interval. Now the polling interval can be changed
dynamically via /sys/class/devfreq/devfreq0/polling_interval.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 71 ++++++++++++++++++++++---------
 1 file changed, 50 insertions(+), 21 deletions(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 8adc0ff48b45..e30314bd571a 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -12,6 +12,7 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/irq.h>
+#include <linux/jiffies.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
@@ -236,7 +237,7 @@ tegra_actmon_dev_avg_dependency_freq(struct tegra_devfreq *tegra,
 				     struct tegra_devfreq_device *dev)
 {
 	return dev->config->avg_dependency_threshold /
-		ACTMON_SAMPLING_PERIOD;
+		tegra->devfreq->profile->polling_ms;
 }
 
 static unsigned long
@@ -314,8 +315,8 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
 	 */
 	*upper = tegra_actmon_account_cpu_freq(tegra, dev, *upper);
 
-	*lower *= ACTMON_SAMPLING_PERIOD;
-	*upper *= ACTMON_SAMPLING_PERIOD;
+	*lower *= tegra->devfreq->profile->polling_ms;
+	*upper *= tegra->devfreq->profile->polling_ms;
 }
 
 static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
@@ -341,8 +342,8 @@ static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
 	 * result, but this one is probably the least churning, although
 	 * it may look a bit convoluted.
 	 */
-	if (freq * ACTMON_SAMPLING_PERIOD > upper)
-		upper = freq * ACTMON_SAMPLING_PERIOD;
+	if (freq * tegra->devfreq->profile->polling_ms > upper)
+		upper = freq * tegra->devfreq->profile->polling_ms;
 
 	/*
 	 * We want to get interrupts when MCCPU client crosses the
@@ -420,7 +421,7 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
 	avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
 	dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
 
-	dev->avg_freq = avg_count / ACTMON_SAMPLING_PERIOD;
+	dev->avg_freq = avg_count / tegra->devfreq->profile->polling_ms;
 
 	avg_intr_mask = ACTMON_DEV_INTR_AVG_BELOW_WMARK |
 			ACTMON_DEV_INTR_AVG_ABOVE_WMARK;
@@ -499,7 +500,7 @@ static unsigned long actmon_update_target(struct tegra_devfreq *tegra,
 	 * outdated.
 	 */
 	avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
-	avg_freq = avg_count / ACTMON_SAMPLING_PERIOD;
+	avg_freq = avg_count / tegra->devfreq->profile->polling_ms;
 
 	old_upper = tegra_actmon_upper_freq(tegra, dev->avg_freq);
 	new_upper = tegra_actmon_upper_freq(tegra, avg_freq);
@@ -675,7 +676,7 @@ static int tegra_actmon_cpu_notify_cb(struct notifier_block *nb,
 	 * here for too long, otherwise CPUFreq's core will complain with a
 	 * warning splat.
 	 */
-	delay = msecs_to_jiffies(ACTMON_SAMPLING_PERIOD);
+	delay = msecs_to_jiffies(tegra->devfreq->profile->polling_ms);
 	schedule_delayed_work(&tegra->cpufreq_update_work, delay);
 
 	return NOTIFY_OK;
@@ -690,7 +691,7 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
 	dev->boost_freq = 0;
 
 	dev->avg_freq = clk_get_rate(tegra->emc_clock) / KHZ;
-	device_writel(dev, dev->avg_freq * ACTMON_SAMPLING_PERIOD,
+	device_writel(dev, dev->avg_freq * tegra->devfreq->profile->polling_ms,
 		      ACTMON_DEV_INIT_AVG);
 
 	tegra_devfreq_update_avg_wmark(tegra, dev, dev->avg_freq);
@@ -725,7 +726,14 @@ static int tegra_actmon_start(struct tegra_devfreq *tegra)
 	unsigned int i;
 	int err;
 
-	actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
+	if (!tegra->devfreq->stop_polling)
+		return 0;
+
+	tegra->devfreq->previous_freq = clk_get_rate(tegra->emc_clock) / KHZ;
+	tegra->devfreq->last_stat_updated = jiffies;
+	tegra->devfreq->stop_polling = false;
+
+	actmon_writel(tegra, tegra->devfreq->profile->polling_ms - 1,
 		      ACTMON_GLB_PERIOD_CTRL);
 
 	/*
@@ -776,6 +784,11 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
 {
 	unsigned int i;
 
+	if (tegra->devfreq->stop_polling)
+		return;
+
+	mutex_unlock(&tegra->devfreq->lock);
+
 	disable_irq(tegra->irq);
 
 	cpufreq_unregister_notifier(&tegra->cpu_rate_change_nb,
@@ -783,10 +796,16 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
 
 	cancel_delayed_work_sync(&tegra->cpufreq_update_work);
 
+	mutex_lock(&tegra->devfreq->lock);
+
 	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
 		tegra_actmon_stop_device(&tegra->devices[i]);
 
 	clk_notifier_unregister(tegra->emc_clock, &tegra->clk_rate_change_nb);
+
+	devfreq_update_status(tegra->devfreq, tegra->devfreq->previous_freq);
+
+	tegra->devfreq->stop_polling = true;
 }
 
 static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
@@ -846,7 +865,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
 	stat->busy_time *= 100 / BUS_SATURATION_RATIO;
 
 	/* Number of cycles in a sampling period */
-	stat->total_time = cur_freq * ACTMON_SAMPLING_PERIOD;
+	stat->total_time = cur_freq * tegra->devfreq->profile->polling_ms;
 
 	stat->busy_time = min(stat->busy_time, stat->total_time);
 
@@ -854,7 +873,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
 }
 
 static struct devfreq_dev_profile tegra_devfreq_profile = {
-	.polling_ms	= 0,
+	.polling_ms	= ACTMON_SAMPLING_PERIOD,
 	.target		= tegra_devfreq_target,
 	.get_dev_status	= tegra_devfreq_get_dev_status,
 };
@@ -895,8 +914,11 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
 					unsigned int event, void *data)
 {
 	struct tegra_devfreq *tegra = dev_get_drvdata(devfreq->dev.parent);
+	unsigned int new_delay, *delay_ptr = data;
 	int ret = 0;
 
+	mutex_lock(&devfreq->lock);
+
 	/*
 	 * Couple device with the governor early as it is needed at
 	 * the moment of governor's start (used by ISR).
@@ -905,26 +927,33 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
 
 	switch (event) {
 	case DEVFREQ_GOV_START:
-		devfreq_monitor_start(devfreq);
+		devfreq->stop_polling = true;
+		/* fall through */
+	case DEVFREQ_GOV_RESUME:
 		ret = tegra_actmon_start(tegra);
 		break;
 
 	case DEVFREQ_GOV_STOP:
-		tegra_actmon_stop(tegra);
-		devfreq_monitor_stop(devfreq);
-		break;
-
+		/* fall through */
 	case DEVFREQ_GOV_SUSPEND:
 		tegra_actmon_stop(tegra);
-		devfreq_monitor_suspend(devfreq);
 		break;
 
-	case DEVFREQ_GOV_RESUME:
-		devfreq_monitor_resume(devfreq);
-		ret = tegra_actmon_start(tegra);
+	case DEVFREQ_GOV_INTERVAL:
+		new_delay = *delay_ptr;
+
+		if (!new_delay || new_delay > 256) {
+			ret = -EINVAL;
+		} else {
+			tegra_actmon_stop(tegra);
+			devfreq->profile->polling_ms = new_delay;
+			ret = tegra_actmon_start(tegra);
+		}
 		break;
 	}
 
+	mutex_unlock(&devfreq->lock);
+
 	return ret;
 }
 
-- 
2.22.0

Re: [PATCH v6 18/19] PM / devfreq: tegra30: Support variable polling interval

[Chanwoo Choi]

Hi,

On 19. 8. 12. 오전 6:23, Dmitry Osipenko wrote:
> The ACTMON governor is interrupt-driven and currently hardware's polling
> interval is fixed to 16ms in the driver. Devfreq supports variable polling
> interval by the generic governors, let's re-use the generic interface for
> changing of the polling interval. Now the polling interval can be changed
> dynamically via /sys/class/devfreq/devfreq0/polling_interval.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra30-devfreq.c | 71 ++++++++++++++++++++++---------
>  1 file changed, 50 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 8adc0ff48b45..e30314bd571a 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -12,6 +12,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/irq.h>
> +#include <linux/jiffies.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
> @@ -236,7 +237,7 @@ tegra_actmon_dev_avg_dependency_freq(struct tegra_devfreq *tegra,
>  				     struct tegra_devfreq_device *dev)
>  {
>  	return dev->config->avg_dependency_threshold /
> -		ACTMON_SAMPLING_PERIOD;
> +		tegra->devfreq->profile->polling_ms;
>  }
>  
>  static unsigned long
> @@ -314,8 +315,8 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
>  	 */
>  	*upper = tegra_actmon_account_cpu_freq(tegra, dev, *upper);
>  
> -	*lower *= ACTMON_SAMPLING_PERIOD;
> -	*upper *= ACTMON_SAMPLING_PERIOD;
> +	*lower *= tegra->devfreq->profile->polling_ms;
> +	*upper *= tegra->devfreq->profile->polling_ms;
>  }
>  
>  static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
> @@ -341,8 +342,8 @@ static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
>  	 * result, but this one is probably the least churning, although
>  	 * it may look a bit convoluted.
>  	 */
> -	if (freq * ACTMON_SAMPLING_PERIOD > upper)
> -		upper = freq * ACTMON_SAMPLING_PERIOD;
> +	if (freq * tegra->devfreq->profile->polling_ms > upper)
> +		upper = freq * tegra->devfreq->profile->polling_ms;
>  
>  	/*
>  	 * We want to get interrupts when MCCPU client crosses the
> @@ -420,7 +421,7 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>  	avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
>  	dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
>  
> -	dev->avg_freq = avg_count / ACTMON_SAMPLING_PERIOD;
> +	dev->avg_freq = avg_count / tegra->devfreq->profile->polling_ms;
>  
>  	avg_intr_mask = ACTMON_DEV_INTR_AVG_BELOW_WMARK |
>  			ACTMON_DEV_INTR_AVG_ABOVE_WMARK;
> @@ -499,7 +500,7 @@ static unsigned long actmon_update_target(struct tegra_devfreq *tegra,
>  	 * outdated.
>  	 */
>  	avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
> -	avg_freq = avg_count / ACTMON_SAMPLING_PERIOD;
> +	avg_freq = avg_count / tegra->devfreq->profile->polling_ms;
>  
>  	old_upper = tegra_actmon_upper_freq(tegra, dev->avg_freq);
>  	new_upper = tegra_actmon_upper_freq(tegra, avg_freq);
> @@ -675,7 +676,7 @@ static int tegra_actmon_cpu_notify_cb(struct notifier_block *nb,
>  	 * here for too long, otherwise CPUFreq's core will complain with a
>  	 * warning splat.
>  	 */
> -	delay = msecs_to_jiffies(ACTMON_SAMPLING_PERIOD);
> +	delay = msecs_to_jiffies(tegra->devfreq->profile->polling_ms);
>  	schedule_delayed_work(&tegra->cpufreq_update_work, delay);
>  
>  	return NOTIFY_OK;
> @@ -690,7 +691,7 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
>  	dev->boost_freq = 0;
>  
>  	dev->avg_freq = clk_get_rate(tegra->emc_clock) / KHZ;
> -	device_writel(dev, dev->avg_freq * ACTMON_SAMPLING_PERIOD,
> +	device_writel(dev, dev->avg_freq * tegra->devfreq->profile->polling_ms,
>  		      ACTMON_DEV_INIT_AVG);
>  
>  	tegra_devfreq_update_avg_wmark(tegra, dev, dev->avg_freq);
> @@ -725,7 +726,14 @@ static int tegra_actmon_start(struct tegra_devfreq *tegra)
>  	unsigned int i;
>  	int err;
>  
> -	actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
> +	if (!tegra->devfreq->stop_polling)

I don't prefer to change the 'stop_polling' on each device driver direclty
without devfreq interface. Don't access it directly.

> +		return 0;
> +
> +	tegra->devfreq->previous_freq = clk_get_rate(tegra->emc_clock) / KHZ;
> +	tegra->devfreq->last_stat_updated = jiffies;
> +	tegra->devfreq->stop_polling = false;

ditto.

> +
> +	actmon_writel(tegra, tegra->devfreq->profile->polling_ms - 1,
>  		      ACTMON_GLB_PERIOD_CTRL);
>  
>  	/*
> @@ -776,6 +784,11 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
>  {
>  	unsigned int i;
>  
> +	if (tegra->devfreq->stop_polling)

ditto.

> +		return;
> +
> +	mutex_unlock(&tegra->devfreq->lock);
> +
>  	disable_irq(tegra->irq);
>  
>  	cpufreq_unregister_notifier(&tegra->cpu_rate_change_nb,
> @@ -783,10 +796,16 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
>  
>  	cancel_delayed_work_sync(&tegra->cpufreq_update_work);
>  
> +	mutex_lock(&tegra->devfreq->lock);
> +
>  	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
>  		tegra_actmon_stop_device(&tegra->devices[i]);
>  
>  	clk_notifier_unregister(tegra->emc_clock, &tegra->clk_rate_change_nb);
> +
> +	devfreq_update_status(tegra->devfreq, tegra->devfreq->previous_freq);
> +
> +	tegra->devfreq->stop_polling = true;

ditto.

>  }
>  
>  static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
> @@ -846,7 +865,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>  	stat->busy_time *= 100 / BUS_SATURATION_RATIO;
>  
>  	/* Number of cycles in a sampling period */
> -	stat->total_time = cur_freq * ACTMON_SAMPLING_PERIOD;
> +	stat->total_time = cur_freq * tegra->devfreq->profile->polling_ms;
>  
>  	stat->busy_time = min(stat->busy_time, stat->total_time);
>  
> @@ -854,7 +873,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>  }
>  
>  static struct devfreq_dev_profile tegra_devfreq_profile = {
> -	.polling_ms	= 0,
> +	.polling_ms	= ACTMON_SAMPLING_PERIOD,
>  	.target		= tegra_devfreq_target,
>  	.get_dev_status	= tegra_devfreq_get_dev_status,
>  };
> @@ -895,8 +914,11 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
>  					unsigned int event, void *data)
>  {
>  	struct tegra_devfreq *tegra = dev_get_drvdata(devfreq->dev.parent);
> +	unsigned int new_delay, *delay_ptr = data;
>  	int ret = 0;
>  
> +	mutex_lock(&devfreq->lock);
> +
>  	/*
>  	 * Couple device with the governor early as it is needed at
>  	 * the moment of governor's start (used by ISR).
> @@ -905,26 +927,33 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
>  
>  	switch (event) {
>  	case DEVFREQ_GOV_START:
> -		devfreq_monitor_start(devfreq);
> +		devfreq->stop_polling = true;

It is not proper to initialize the 'stop_polling' on device driver directly.

> +		/* fall through */
> +	case DEVFREQ_GOV_RESUME:
>  		ret = tegra_actmon_start(tegra);
>  		break;
>  
>  	case DEVFREQ_GOV_STOP:
> -		tegra_actmon_stop(tegra);
> -		devfreq_monitor_stop(devfreq);

Why do you remove them?

> -		break;
> -
> +		/* fall through */
>  	case DEVFREQ_GOV_SUSPEND:
>  		tegra_actmon_stop(tegra);
> -		devfreq_monitor_suspend(devfreq);

Why do you remove it?

>  		break;
>  
> -	case DEVFREQ_GOV_RESUME:
> -		devfreq_monitor_resume(devfreq);
> -		ret = tegra_actmon_start(tegra);
> +	case DEVFREQ_GOV_INTERVAL:
> +		new_delay = *delay_ptr;
> +
> +		if (!new_delay || new_delay > 256) {
> +			ret = -EINVAL;
> +		} else {
> +			tegra_actmon_stop(tegra);
> +			devfreq->profile->polling_ms = new_delay;
> +			ret = tegra_actmon_start(tegra);
> +		}
>  		break;
>  	}
>  
> +	mutex_unlock(&devfreq->lock);
> +
>  	return ret;
>  }
>  
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v6 18/19] PM / devfreq: tegra30: Support variable polling interval

[Dmitry Osipenko]

02.10.2019 03:18, Chanwoo Choi пишет:
> Hi,
> 
> On 19. 8. 12. 오전 6:23, Dmitry Osipenko wrote:
>> The ACTMON governor is interrupt-driven and currently hardware's polling
>> interval is fixed to 16ms in the driver. Devfreq supports variable polling
>> interval by the generic governors, let's re-use the generic interface for
>> changing of the polling interval. Now the polling interval can be changed
>> dynamically via /sys/class/devfreq/devfreq0/polling_interval.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/tegra30-devfreq.c | 71 ++++++++++++++++++++++---------
>>  1 file changed, 50 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>> index 8adc0ff48b45..e30314bd571a 100644
>> --- a/drivers/devfreq/tegra30-devfreq.c
>> +++ b/drivers/devfreq/tegra30-devfreq.c
>> @@ -12,6 +12,7 @@
>>  #include <linux/interrupt.h>
>>  #include <linux/io.h>
>>  #include <linux/irq.h>
>> +#include <linux/jiffies.h>
>>  #include <linux/module.h>
>>  #include <linux/of_device.h>
>>  #include <linux/platform_device.h>
>> @@ -236,7 +237,7 @@ tegra_actmon_dev_avg_dependency_freq(struct tegra_devfreq *tegra,
>>  				     struct tegra_devfreq_device *dev)
>>  {
>>  	return dev->config->avg_dependency_threshold /
>> -		ACTMON_SAMPLING_PERIOD;
>> +		tegra->devfreq->profile->polling_ms;
>>  }
>>  
>>  static unsigned long
>> @@ -314,8 +315,8 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
>>  	 */
>>  	*upper = tegra_actmon_account_cpu_freq(tegra, dev, *upper);
>>  
>> -	*lower *= ACTMON_SAMPLING_PERIOD;
>> -	*upper *= ACTMON_SAMPLING_PERIOD;
>> +	*lower *= tegra->devfreq->profile->polling_ms;
>> +	*upper *= tegra->devfreq->profile->polling_ms;
>>  }
>>  
>>  static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
>> @@ -341,8 +342,8 @@ static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
>>  	 * result, but this one is probably the least churning, although
>>  	 * it may look a bit convoluted.
>>  	 */
>> -	if (freq * ACTMON_SAMPLING_PERIOD > upper)
>> -		upper = freq * ACTMON_SAMPLING_PERIOD;
>> +	if (freq * tegra->devfreq->profile->polling_ms > upper)
>> +		upper = freq * tegra->devfreq->profile->polling_ms;
>>  
>>  	/*
>>  	 * We want to get interrupts when MCCPU client crosses the
>> @@ -420,7 +421,7 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>>  	avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
>>  	dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
>>  
>> -	dev->avg_freq = avg_count / ACTMON_SAMPLING_PERIOD;
>> +	dev->avg_freq = avg_count / tegra->devfreq->profile->polling_ms;
>>  
>>  	avg_intr_mask = ACTMON_DEV_INTR_AVG_BELOW_WMARK |
>>  			ACTMON_DEV_INTR_AVG_ABOVE_WMARK;
>> @@ -499,7 +500,7 @@ static unsigned long actmon_update_target(struct tegra_devfreq *tegra,
>>  	 * outdated.
>>  	 */
>>  	avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
>> -	avg_freq = avg_count / ACTMON_SAMPLING_PERIOD;
>> +	avg_freq = avg_count / tegra->devfreq->profile->polling_ms;
>>  
>>  	old_upper = tegra_actmon_upper_freq(tegra, dev->avg_freq);
>>  	new_upper = tegra_actmon_upper_freq(tegra, avg_freq);
>> @@ -675,7 +676,7 @@ static int tegra_actmon_cpu_notify_cb(struct notifier_block *nb,
>>  	 * here for too long, otherwise CPUFreq's core will complain with a
>>  	 * warning splat.
>>  	 */
>> -	delay = msecs_to_jiffies(ACTMON_SAMPLING_PERIOD);
>> +	delay = msecs_to_jiffies(tegra->devfreq->profile->polling_ms);
>>  	schedule_delayed_work(&tegra->cpufreq_update_work, delay);
>>  
>>  	return NOTIFY_OK;
>> @@ -690,7 +691,7 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
>>  	dev->boost_freq = 0;
>>  
>>  	dev->avg_freq = clk_get_rate(tegra->emc_clock) / KHZ;
>> -	device_writel(dev, dev->avg_freq * ACTMON_SAMPLING_PERIOD,
>> +	device_writel(dev, dev->avg_freq * tegra->devfreq->profile->polling_ms,
>>  		      ACTMON_DEV_INIT_AVG);
>>  
>>  	tegra_devfreq_update_avg_wmark(tegra, dev, dev->avg_freq);
>> @@ -725,7 +726,14 @@ static int tegra_actmon_start(struct tegra_devfreq *tegra)
>>  	unsigned int i;
>>  	int err;
>>  
>> -	actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
>> +	if (!tegra->devfreq->stop_polling)
> 
> I don't prefer to change the 'stop_polling' on each device driver direclty
> without devfreq interface. Don't access it directly.

Hmm.. okay. Please see more comments below.

>> +		return 0;
>> +
>> +	tegra->devfreq->previous_freq = clk_get_rate(tegra->emc_clock) / KHZ;
>> +	tegra->devfreq->last_stat_updated = jiffies;
>> +	tegra->devfreq->stop_polling = false;
> 
> ditto.
> 
>> +
>> +	actmon_writel(tegra, tegra->devfreq->profile->polling_ms - 1,
>>  		      ACTMON_GLB_PERIOD_CTRL);
>>  
>>  	/*
>> @@ -776,6 +784,11 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
>>  {
>>  	unsigned int i;
>>  
>> +	if (tegra->devfreq->stop_polling)
> 
> ditto.
> 
>> +		return;
>> +
>> +	mutex_unlock(&tegra->devfreq->lock);
>> +
>>  	disable_irq(tegra->irq);
>>  
>>  	cpufreq_unregister_notifier(&tegra->cpu_rate_change_nb,
>> @@ -783,10 +796,16 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
>>  
>>  	cancel_delayed_work_sync(&tegra->cpufreq_update_work);
>>  
>> +	mutex_lock(&tegra->devfreq->lock);
>> +
>>  	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
>>  		tegra_actmon_stop_device(&tegra->devices[i]);
>>  
>>  	clk_notifier_unregister(tegra->emc_clock, &tegra->clk_rate_change_nb);
>> +
>> +	devfreq_update_status(tegra->devfreq, tegra->devfreq->previous_freq);
>> +
>> +	tegra->devfreq->stop_polling = true;
> 
> ditto.
> 
>>  }
>>  
>>  static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>> @@ -846,7 +865,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>>  	stat->busy_time *= 100 / BUS_SATURATION_RATIO;
>>  
>>  	/* Number of cycles in a sampling period */
>> -	stat->total_time = cur_freq * ACTMON_SAMPLING_PERIOD;
>> +	stat->total_time = cur_freq * tegra->devfreq->profile->polling_ms;
>>  
>>  	stat->busy_time = min(stat->busy_time, stat->total_time);
>>  
>> @@ -854,7 +873,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>>  }
>>  
>>  static struct devfreq_dev_profile tegra_devfreq_profile = {
>> -	.polling_ms	= 0,
>> +	.polling_ms	= ACTMON_SAMPLING_PERIOD,
>>  	.target		= tegra_devfreq_target,
>>  	.get_dev_status	= tegra_devfreq_get_dev_status,
>>  };
>> @@ -895,8 +914,11 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
>>  					unsigned int event, void *data)
>>  {
>>  	struct tegra_devfreq *tegra = dev_get_drvdata(devfreq->dev.parent);
>> +	unsigned int new_delay, *delay_ptr = data;
>>  	int ret = 0;
>>  
>> +	mutex_lock(&devfreq->lock);
>> +
>>  	/*
>>  	 * Couple device with the governor early as it is needed at
>>  	 * the moment of governor's start (used by ISR).
>> @@ -905,26 +927,33 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
>>  
>>  	switch (event) {
>>  	case DEVFREQ_GOV_START:
>> -		devfreq_monitor_start(devfreq);
>> +		devfreq->stop_polling = true;
> 
> It is not proper to initialize the 'stop_polling' on device driver directly.
> 
>> +		/* fall through */
>> +	case DEVFREQ_GOV_RESUME:
>>  		ret = tegra_actmon_start(tegra);
>>  		break;
>>  
>>  	case DEVFREQ_GOV_STOP:
>> -		tegra_actmon_stop(tegra);
>> -		devfreq_monitor_stop(devfreq);
> 
> Why do you remove them?

Becasue DEVFREQ_GOV_STOP and DEVFREQ_GOV_SUSPEND are doing the same now:

	case DEVFREQ_GOV_STOP:
		/* fall through */
	case DEVFREQ_GOV_SUSPEND:
		tegra_actmon_stop(tegra);
		break;

>> -		break;
>> -
>> +		/* fall through */
>>  	case DEVFREQ_GOV_SUSPEND:
>>  		tegra_actmon_stop(tegra);
>> -		devfreq_monitor_suspend(devfreq);
> 
> Why do you remove it?

a) it takes the same mutex devfreq->lock as tegra_governor_event_handler()
b) the devfreq's manual polling isn't needed for this driver and thus
devfreq->work is unwanted
c) this patch makes tegra_actmon_stop() to replicate what
devfreq_monitor_suspend() does

It looks like it will be better to add new generic events for the
governors, like DEVFREQ_GOV_START/STOP_POLLING. Then this driver will be
able to customize start/stop of the polling, while software governors
will utilize some new default helpers that will start/stop the polling
work. What do you think?

[snip]

[PATCH v6 19/19] PM / devfreq: tegra20/30: Add Dmitry as a maintainer

[Dmitry Osipenko]

I was contributing to the NVIDIA Tegra20+ devfreq drivers recently and
want to help keep them working and evolving in the future.

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e60f5c361969..8a78abe4739e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10420,6 +10420,15 @@ F:	include/linux/memblock.h
 F:	mm/memblock.c
 F:	Documentation/core-api/boot-time-mm.rst
 
+MEMORY FREQUENCY SCALING DRIVERS FOR NVIDIA TEGRA
+M:	Dmitry Osipenko <digetx@gmail.com>
+L:	linux-pm@vger.kernel.org
+L:	linux-tegra@vger.kernel.org
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git
+S:	Maintained
+F:	drivers/devfreq/tegra20-devfreq.c
+F:	drivers/devfreq/tegra30-devfreq.c
+
 MEMORY MANAGEMENT
 L:	linux-mm@kvack.org
 W:	http://www.linux-mm.org
-- 
2.22.0

Re: [PATCH v6 00/19] More improvements for Tegra30 devfreq driver

[Dmitry Osipenko]

12.08.2019 00:22, Dmitry Osipenko пишет:
> Hello,
> 
> This series addresses some additional review comments that were made by
> Thierry Reding to [1], makes several important changes to the driver,
> fixing excessive interrupts activity, and adds new features. In the end
> I'm proposing myself as a maintainer for the Tegra devfreq drivers.
> 
> [1] https://lore.kernel.org/lkml/0fb50eb1-a173-1756-6889-2526a10ac707@gmail.com/T/
> 
> Changelog:
> 
> v6:  Addressed review comment that was made by Chanwoo Choi to v5 by
>      squashing "Define ACTMON_DEV_CTRL_STOP" patch into the "Use CPUFreq
>      notifier" patch.
> 
> v5:  Addressed review comments that were made by Chanwoo Choi to v4 by
>      squashing few patches, dropping some questionable patches, rewording
>      comments to the code, restructuring the code and etc.
> 
>      These patches are now dropped from the series:
> 
>        PM / devfreq: tegra30: Use tracepoints for debugging
>        PM / devfreq: tegra30: Inline all one-line functions
> 
>      The interrupt-optimization patches are squashed into a single patch:
> 
>        PM / devfreq: tegra30: Reduce unnecessary interrupts activity
> 
>      because it's better to keep the optimizations as a separate change and
>      this also helps to reduce code churning, since the code changes depend
>      on a previous patch in order to stay cleaner.
> 
>      Fixed a lockup bug that I spotted recently, which is caused by a
>      clk-notifier->cpufreq_get()->clk_set_rate() sequence. Now a non-blocking
>      variant of CPU's frequency retrieving is used, i.e. cpufreq_quick_get().
> 
>      Further optimized the CPUFreq notifier by postponing the delayed
>      updating in accordance to the polling interval, this actually uncovered
>      the above lockup bug.
> 
>      Implemented new minor driver feature in the new patch:
> 
>        PM / devfreq: tegra30: Support variable polling interval
> 
> v4:  Added two new patches to the series:
> 
>        PM / devfreq: tegra30: Synchronize average count on target's update
>        PM / devfreq: tegra30: Increase sampling period to 16ms
> 
>      The first patch addresses problem where governor could get stuck due
>      to outdated "average count" value which is snapshoted by ISR and there
>      are cases where manual update of the value is required.
> 
>      The second patch is just a minor optimization.
> 
> v3:  Added support for tracepoints, replacing the debug messages.
>      Fixed few more bugs with the help of tracepoints.
> 
>      New patches in this version:
> 
>        PM / devfreq: tegra30: Use tracepoints for debugging
>        PM / devfreq: tegra30: Optimize CPUFreq notifier
>        PM / devfreq: tegra30: Optimize upper consecutive watermark selection
>        PM / devfreq: tegra30: Optimize upper average watermark selection
>        PM / devfreq: tegra30: Include appropriate header
> 
>      Some of older patches of this series also got some extra minor polish.
> 
> v2:  Added more patches that are cleaning driver's code further and
>      squashing another kHz conversion bug.
> 
>      The patch "Rework frequency management logic" of the v1 series is now
>      converted to "Set up watermarks properly" because I found some problems
>      in the original patch and then realized that there is no need to change
>      the logic much. So the logic mostly preserved and only got improvements.
> 
>      The series is based on the today's linux-next (25 Jun) and takes into
>      account minor changes that MyungJoo Ham made to the already queued
>      patches from the first batch [1].
> 
> Dmitry Osipenko (19):
>   PM / devfreq: tegra30: Change irq type to unsigned int
>   PM / devfreq: tegra30: Keep interrupt disabled while governor is
>     stopped
>   PM / devfreq: tegra30: Handle possible round-rate error
>   PM / devfreq: tegra30: Drop write-barrier
>   PM / devfreq: tegra30: Set up watermarks properly
>   PM / devfreq: tegra30: Tune up boosting thresholds
>   PM / devfreq: tegra30: Fix integer overflow on CPU's freq max out
>   PM / devfreq: tegra30: Ensure that target freq won't overflow
>   PM / devfreq: tegra30: Use kHz units uniformly in the code
>   PM / devfreq: tegra30: Reduce unnecessary interrupts activity
>   PM / devfreq: tegra30: Use CPUFreq notifier
>   PM / devfreq: tegra30: Move clk-notifier's registration to governor's
>     start
>   PM / devfreq: tegra30: Reset boosting on startup
>   PM / devfreq: tegra30: Don't enable consecutive-down interrupt on
>     startup
>   PM / devfreq: tegra30: Constify structs
>   PM / devfreq: tegra30: Include appropriate header
>   PM / devfreq: tegra30: Increase sampling period to 16ms
>   PM / devfreq: tegra30: Support variable polling interval
>   PM / devfreq: tegra20/30: Add Dmitry as a maintainer
> 
>  MAINTAINERS                       |   9 +
>  drivers/devfreq/tegra30-devfreq.c | 706 +++++++++++++++++++++++-------
>  2 files changed, 555 insertions(+), 160 deletions(-)
> 

Hello Chanwoo,

I don't have any more updates in regards to this series, everything is
working flawlessly for now. Will be awesome if we could continue the
reviewing and then get the patches into linux-next to get some more testing.

Re: [PATCH v6 00/19] More improvements for Tegra30 devfreq driver

[Chanwoo Choi]

Hello Dmitry and Thierry,

On 19. 10. 2. 오전 6:15, Dmitry Osipenko wrote:
> 12.08.2019 00:22, Dmitry Osipenko пишет:
>> Hello,
>>
>> This series addresses some additional review comments that were made by
>> Thierry Reding to [1], makes several important changes to the driver,
>> fixing excessive interrupts activity, and adds new features. In the end
>> I'm proposing myself as a maintainer for the Tegra devfreq drivers.
>>
>> [1] https://lore.kernel.org/lkml/0fb50eb1-a173-1756-6889-2526a10ac707@gmail.com/T/
>>
>> Changelog:
>>
>> v6:  Addressed review comment that was made by Chanwoo Choi to v5 by
>>      squashing "Define ACTMON_DEV_CTRL_STOP" patch into the "Use CPUFreq
>>      notifier" patch.
>>
>> v5:  Addressed review comments that were made by Chanwoo Choi to v4 by
>>      squashing few patches, dropping some questionable patches, rewording
>>      comments to the code, restructuring the code and etc.
>>
>>      These patches are now dropped from the series:
>>
>>        PM / devfreq: tegra30: Use tracepoints for debugging
>>        PM / devfreq: tegra30: Inline all one-line functions
>>
>>      The interrupt-optimization patches are squashed into a single patch:
>>
>>        PM / devfreq: tegra30: Reduce unnecessary interrupts activity
>>
>>      because it's better to keep the optimizations as a separate change and
>>      this also helps to reduce code churning, since the code changes depend
>>      on a previous patch in order to stay cleaner.
>>
>>      Fixed a lockup bug that I spotted recently, which is caused by a
>>      clk-notifier->cpufreq_get()->clk_set_rate() sequence. Now a non-blocking
>>      variant of CPU's frequency retrieving is used, i.e. cpufreq_quick_get().
>>
>>      Further optimized the CPUFreq notifier by postponing the delayed
>>      updating in accordance to the polling interval, this actually uncovered
>>      the above lockup bug.
>>
>>      Implemented new minor driver feature in the new patch:
>>
>>        PM / devfreq: tegra30: Support variable polling interval
>>
>> v4:  Added two new patches to the series:
>>
>>        PM / devfreq: tegra30: Synchronize average count on target's update
>>        PM / devfreq: tegra30: Increase sampling period to 16ms
>>
>>      The first patch addresses problem where governor could get stuck due
>>      to outdated "average count" value which is snapshoted by ISR and there
>>      are cases where manual update of the value is required.
>>
>>      The second patch is just a minor optimization.
>>
>> v3:  Added support for tracepoints, replacing the debug messages.
>>      Fixed few more bugs with the help of tracepoints.
>>
>>      New patches in this version:
>>
>>        PM / devfreq: tegra30: Use tracepoints for debugging
>>        PM / devfreq: tegra30: Optimize CPUFreq notifier
>>        PM / devfreq: tegra30: Optimize upper consecutive watermark selection
>>        PM / devfreq: tegra30: Optimize upper average watermark selection
>>        PM / devfreq: tegra30: Include appropriate header
>>
>>      Some of older patches of this series also got some extra minor polish.
>>
>> v2:  Added more patches that are cleaning driver's code further and
>>      squashing another kHz conversion bug.
>>
>>      The patch "Rework frequency management logic" of the v1 series is now
>>      converted to "Set up watermarks properly" because I found some problems
>>      in the original patch and then realized that there is no need to change
>>      the logic much. So the logic mostly preserved and only got improvements.
>>
>>      The series is based on the today's linux-next (25 Jun) and takes into
>>      account minor changes that MyungJoo Ham made to the already queued
>>      patches from the first batch [1].
>>
>> Dmitry Osipenko (19):
>>   PM / devfreq: tegra30: Change irq type to unsigned int
>>   PM / devfreq: tegra30: Keep interrupt disabled while governor is
>>     stopped
>>   PM / devfreq: tegra30: Handle possible round-rate error
>>   PM / devfreq: tegra30: Drop write-barrier
>>   PM / devfreq: tegra30: Set up watermarks properly
>>   PM / devfreq: tegra30: Tune up boosting thresholds
>>   PM / devfreq: tegra30: Fix integer overflow on CPU's freq max out
>>   PM / devfreq: tegra30: Ensure that target freq won't overflow
>>   PM / devfreq: tegra30: Use kHz units uniformly in the code
>>   PM / devfreq: tegra30: Reduce unnecessary interrupts activity
>>   PM / devfreq: tegra30: Use CPUFreq notifier
>>   PM / devfreq: tegra30: Move clk-notifier's registration to governor's
>>     start
>>   PM / devfreq: tegra30: Reset boosting on startup
>>   PM / devfreq: tegra30: Don't enable consecutive-down interrupt on
>>     startup
>>   PM / devfreq: tegra30: Constify structs
>>   PM / devfreq: tegra30: Include appropriate header
>>   PM / devfreq: tegra30: Increase sampling period to 16ms
>>   PM / devfreq: tegra30: Support variable polling interval
>>   PM / devfreq: tegra20/30: Add Dmitry as a maintainer
>>
>>  MAINTAINERS                       |   9 +
>>  drivers/devfreq/tegra30-devfreq.c | 706 +++++++++++++++++++++++-------
>>  2 files changed, 555 insertions(+), 160 deletions(-)
>>
> 
> Hello Chanwoo,
> 
> I don't have any more updates in regards to this series, everything is
> working flawlessly for now. Will be awesome if we could continue the
> reviewing and then get the patches into linux-next to get some more testing.
> 
> 

Hello Dmitry,

I'm sorry for late reply. Except for patch5, I reviewed the patches.
Please check my comment. Actually, It is difficult to review the patch5
without any testing environment and detailed knowledge of watermark of tegra.
It is not familiar with me.


Hello Thierry,
If possible, Could you review the patch5 related to setting up the watermark
and other patches?

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v6 00/19] More improvements for Tegra30 devfreq driver

[Dmitry Osipenko]

02.10.2019 03:25, Chanwoo Choi пишет:
> Hello Dmitry and Thierry,
> 
> On 19. 10. 2. 오전 6:15, Dmitry Osipenko wrote:
>> 12.08.2019 00:22, Dmitry Osipenko пишет:
>>> Hello,
>>>
>>> This series addresses some additional review comments that were made by
>>> Thierry Reding to [1], makes several important changes to the driver,
>>> fixing excessive interrupts activity, and adds new features. In the end
>>> I'm proposing myself as a maintainer for the Tegra devfreq drivers.
>>>
>>> [1] https://lore.kernel.org/lkml/0fb50eb1-a173-1756-6889-2526a10ac707@gmail.com/T/
>>>
>>> Changelog:
>>>
>>> v6:  Addressed review comment that was made by Chanwoo Choi to v5 by
>>>      squashing "Define ACTMON_DEV_CTRL_STOP" patch into the "Use CPUFreq
>>>      notifier" patch.
>>>
>>> v5:  Addressed review comments that were made by Chanwoo Choi to v4 by
>>>      squashing few patches, dropping some questionable patches, rewording
>>>      comments to the code, restructuring the code and etc.
>>>
>>>      These patches are now dropped from the series:
>>>
>>>        PM / devfreq: tegra30: Use tracepoints for debugging
>>>        PM / devfreq: tegra30: Inline all one-line functions
>>>
>>>      The interrupt-optimization patches are squashed into a single patch:
>>>
>>>        PM / devfreq: tegra30: Reduce unnecessary interrupts activity
>>>
>>>      because it's better to keep the optimizations as a separate change and
>>>      this also helps to reduce code churning, since the code changes depend
>>>      on a previous patch in order to stay cleaner.
>>>
>>>      Fixed a lockup bug that I spotted recently, which is caused by a
>>>      clk-notifier->cpufreq_get()->clk_set_rate() sequence. Now a non-blocking
>>>      variant of CPU's frequency retrieving is used, i.e. cpufreq_quick_get().
>>>
>>>      Further optimized the CPUFreq notifier by postponing the delayed
>>>      updating in accordance to the polling interval, this actually uncovered
>>>      the above lockup bug.
>>>
>>>      Implemented new minor driver feature in the new patch:
>>>
>>>        PM / devfreq: tegra30: Support variable polling interval
>>>
>>> v4:  Added two new patches to the series:
>>>
>>>        PM / devfreq: tegra30: Synchronize average count on target's update
>>>        PM / devfreq: tegra30: Increase sampling period to 16ms
>>>
>>>      The first patch addresses problem where governor could get stuck due
>>>      to outdated "average count" value which is snapshoted by ISR and there
>>>      are cases where manual update of the value is required.
>>>
>>>      The second patch is just a minor optimization.
>>>
>>> v3:  Added support for tracepoints, replacing the debug messages.
>>>      Fixed few more bugs with the help of tracepoints.
>>>
>>>      New patches in this version:
>>>
>>>        PM / devfreq: tegra30: Use tracepoints for debugging
>>>        PM / devfreq: tegra30: Optimize CPUFreq notifier
>>>        PM / devfreq: tegra30: Optimize upper consecutive watermark selection
>>>        PM / devfreq: tegra30: Optimize upper average watermark selection
>>>        PM / devfreq: tegra30: Include appropriate header
>>>
>>>      Some of older patches of this series also got some extra minor polish.
>>>
>>> v2:  Added more patches that are cleaning driver's code further and
>>>      squashing another kHz conversion bug.
>>>
>>>      The patch "Rework frequency management logic" of the v1 series is now
>>>      converted to "Set up watermarks properly" because I found some problems
>>>      in the original patch and then realized that there is no need to change
>>>      the logic much. So the logic mostly preserved and only got improvements.
>>>
>>>      The series is based on the today's linux-next (25 Jun) and takes into
>>>      account minor changes that MyungJoo Ham made to the already queued
>>>      patches from the first batch [1].
>>>
>>> Dmitry Osipenko (19):
>>>   PM / devfreq: tegra30: Change irq type to unsigned int
>>>   PM / devfreq: tegra30: Keep interrupt disabled while governor is
>>>     stopped
>>>   PM / devfreq: tegra30: Handle possible round-rate error
>>>   PM / devfreq: tegra30: Drop write-barrier
>>>   PM / devfreq: tegra30: Set up watermarks properly
>>>   PM / devfreq: tegra30: Tune up boosting thresholds
>>>   PM / devfreq: tegra30: Fix integer overflow on CPU's freq max out
>>>   PM / devfreq: tegra30: Ensure that target freq won't overflow
>>>   PM / devfreq: tegra30: Use kHz units uniformly in the code
>>>   PM / devfreq: tegra30: Reduce unnecessary interrupts activity
>>>   PM / devfreq: tegra30: Use CPUFreq notifier
>>>   PM / devfreq: tegra30: Move clk-notifier's registration to governor's
>>>     start
>>>   PM / devfreq: tegra30: Reset boosting on startup
>>>   PM / devfreq: tegra30: Don't enable consecutive-down interrupt on
>>>     startup
>>>   PM / devfreq: tegra30: Constify structs
>>>   PM / devfreq: tegra30: Include appropriate header
>>>   PM / devfreq: tegra30: Increase sampling period to 16ms
>>>   PM / devfreq: tegra30: Support variable polling interval
>>>   PM / devfreq: tegra20/30: Add Dmitry as a maintainer
>>>
>>>  MAINTAINERS                       |   9 +
>>>  drivers/devfreq/tegra30-devfreq.c | 706 +++++++++++++++++++++++-------
>>>  2 files changed, 555 insertions(+), 160 deletions(-)
>>>
>>
>> Hello Chanwoo,
>>
>> I don't have any more updates in regards to this series, everything is
>> working flawlessly for now. Will be awesome if we could continue the
>> reviewing and then get the patches into linux-next to get some more testing.
>>
>>
> 
> Hello Dmitry,
> 
> I'm sorry for late reply. Except for patch5, I reviewed the patches.
> Please check my comment. Actually, It is difficult to review the patch5
> without any testing environment and detailed knowledge of watermark of tegra.
> It is not familiar with me.

Thank you very much! I'll go through yours comments and reply to them.

I understand that it's not easy for you to review patch5, but probably
you don't need to go into details and a brief-generic review of the code
will be enough in that case.

The hardware is actually very simple, there are watermarks that
correspond to a memory activity that hardware accounts over a given
period of time. Once watermark is reached, hardware generates interrupt.
There are two types of watermarks: average and consecutive. In case of
the average, the memory activity is collected over a larger window of
time. For the consecutive case, the memory activity is collected over
each period (16ms by default in the driver). Memory client may breach
average watermark very frequently, although that may not affect much the
average value and for some memory clients (like CPU) it is more
preferred to not completely ignore those short bursts of memory
activity. The consecutive watermarks are used in order to detect those
short bursts, which we account in the driver in a form of boosting. You
may notice that boost_up_coeff for the CPU's memory client is set to a
higher value in the driver.

> Hello Thierry,
> If possible, Could you review the patch5 related to setting up the watermark
> and other patches?
> 

Indeed, will be very nice if Thierry could also take a look at this
series. Although.. I could be wrong here, but it looks to me that
Thierry also isn't closely familiar with this driver and the hardware.

Thierry, at least please let us know if you're interested in taking a
look at the patches, I'm pretty sure that you're quite busy with other
things ;)

Re: [PATCH v6 00/19] More improvements for Tegra30 devfreq driver

[Peter Geis]

Tested on the Ouya (tegra30).

Tested-by: Peter Geis <pgwipeout@gmail.com>

On Wed, Oct 2, 2019 at 9:56 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 02.10.2019 03:25, Chanwoo Choi пишет:
> > Hello Dmitry and Thierry,
> >
> > On 19. 10. 2. 오전 6:15, Dmitry Osipenko wrote:
> >> 12.08.2019 00:22, Dmitry Osipenko пишет:
> >>> Hello,
> >>>
> >>> This series addresses some additional review comments that were made by
> >>> Thierry Reding to [1], makes several important changes to the driver,
> >>> fixing excessive interrupts activity, and adds new features. In the end
> >>> I'm proposing myself as a maintainer for the Tegra devfreq drivers.
> >>>
> >>> [1] https://lore.kernel.org/lkml/0fb50eb1-a173-1756-6889-2526a10ac707@gmail.com/T/
> >>>
> >>> Changelog:
> >>>
> >>> v6:  Addressed review comment that was made by Chanwoo Choi to v5 by
> >>>      squashing "Define ACTMON_DEV_CTRL_STOP" patch into the "Use CPUFreq
> >>>      notifier" patch.
> >>>
> >>> v5:  Addressed review comments that were made by Chanwoo Choi to v4 by
> >>>      squashing few patches, dropping some questionable patches, rewording
> >>>      comments to the code, restructuring the code and etc.
> >>>
> >>>      These patches are now dropped from the series:
> >>>
> >>>        PM / devfreq: tegra30: Use tracepoints for debugging
> >>>        PM / devfreq: tegra30: Inline all one-line functions
> >>>
> >>>      The interrupt-optimization patches are squashed into a single patch:
> >>>
> >>>        PM / devfreq: tegra30: Reduce unnecessary interrupts activity
> >>>
> >>>      because it's better to keep the optimizations as a separate change and
> >>>      this also helps to reduce code churning, since the code changes depend
> >>>      on a previous patch in order to stay cleaner.
> >>>
> >>>      Fixed a lockup bug that I spotted recently, which is caused by a
> >>>      clk-notifier->cpufreq_get()->clk_set_rate() sequence. Now a non-blocking
> >>>      variant of CPU's frequency retrieving is used, i.e. cpufreq_quick_get().
> >>>
> >>>      Further optimized the CPUFreq notifier by postponing the delayed
> >>>      updating in accordance to the polling interval, this actually uncovered
> >>>      the above lockup bug.
> >>>
> >>>      Implemented new minor driver feature in the new patch:
> >>>
> >>>        PM / devfreq: tegra30: Support variable polling interval
> >>>
> >>> v4:  Added two new patches to the series:
> >>>
> >>>        PM / devfreq: tegra30: Synchronize average count on target's update
> >>>        PM / devfreq: tegra30: Increase sampling period to 16ms
> >>>
> >>>      The first patch addresses problem where governor could get stuck due
> >>>      to outdated "average count" value which is snapshoted by ISR and there
> >>>      are cases where manual update of the value is required.
> >>>
> >>>      The second patch is just a minor optimization.
> >>>
> >>> v3:  Added support for tracepoints, replacing the debug messages.
> >>>      Fixed few more bugs with the help of tracepoints.
> >>>
> >>>      New patches in this version:
> >>>
> >>>        PM / devfreq: tegra30: Use tracepoints for debugging
> >>>        PM / devfreq: tegra30: Optimize CPUFreq notifier
> >>>        PM / devfreq: tegra30: Optimize upper consecutive watermark selection
> >>>        PM / devfreq: tegra30: Optimize upper average watermark selection
> >>>        PM / devfreq: tegra30: Include appropriate header
> >>>
> >>>      Some of older patches of this series also got some extra minor polish.
> >>>
> >>> v2:  Added more patches that are cleaning driver's code further and
> >>>      squashing another kHz conversion bug.
> >>>
> >>>      The patch "Rework frequency management logic" of the v1 series is now
> >>>      converted to "Set up watermarks properly" because I found some problems
> >>>      in the original patch and then realized that there is no need to change
> >>>      the logic much. So the logic mostly preserved and only got improvements.
> >>>
> >>>      The series is based on the today's linux-next (25 Jun) and takes into
> >>>      account minor changes that MyungJoo Ham made to the already queued
> >>>      patches from the first batch [1].
> >>>
> >>> Dmitry Osipenko (19):
> >>>   PM / devfreq: tegra30: Change irq type to unsigned int
> >>>   PM / devfreq: tegra30: Keep interrupt disabled while governor is
> >>>     stopped
> >>>   PM / devfreq: tegra30: Handle possible round-rate error
> >>>   PM / devfreq: tegra30: Drop write-barrier
> >>>   PM / devfreq: tegra30: Set up watermarks properly
> >>>   PM / devfreq: tegra30: Tune up boosting thresholds
> >>>   PM / devfreq: tegra30: Fix integer overflow on CPU's freq max out
> >>>   PM / devfreq: tegra30: Ensure that target freq won't overflow
> >>>   PM / devfreq: tegra30: Use kHz units uniformly in the code
> >>>   PM / devfreq: tegra30: Reduce unnecessary interrupts activity
> >>>   PM / devfreq: tegra30: Use CPUFreq notifier
> >>>   PM / devfreq: tegra30: Move clk-notifier's registration to governor's
> >>>     start
> >>>   PM / devfreq: tegra30: Reset boosting on startup
> >>>   PM / devfreq: tegra30: Don't enable consecutive-down interrupt on
> >>>     startup
> >>>   PM / devfreq: tegra30: Constify structs
> >>>   PM / devfreq: tegra30: Include appropriate header
> >>>   PM / devfreq: tegra30: Increase sampling period to 16ms
> >>>   PM / devfreq: tegra30: Support variable polling interval
> >>>   PM / devfreq: tegra20/30: Add Dmitry as a maintainer
> >>>
> >>>  MAINTAINERS                       |   9 +
> >>>  drivers/devfreq/tegra30-devfreq.c | 706 +++++++++++++++++++++++-------
> >>>  2 files changed, 555 insertions(+), 160 deletions(-)
> >>>
> >>
> >> Hello Chanwoo,
> >>
> >> I don't have any more updates in regards to this series, everything is
> >> working flawlessly for now. Will be awesome if we could continue the
> >> reviewing and then get the patches into linux-next to get some more testing.
> >>
> >>
> >
> > Hello Dmitry,
> >
> > I'm sorry for late reply. Except for patch5, I reviewed the patches.
> > Please check my comment. Actually, It is difficult to review the patch5
> > without any testing environment and detailed knowledge of watermark of tegra.
> > It is not familiar with me.
>
> Thank you very much! I'll go through yours comments and reply to them.
>
> I understand that it's not easy for you to review patch5, but probably
> you don't need to go into details and a brief-generic review of the code
> will be enough in that case.
>
> The hardware is actually very simple, there are watermarks that
> correspond to a memory activity that hardware accounts over a given
> period of time. Once watermark is reached, hardware generates interrupt.
> There are two types of watermarks: average and consecutive. In case of
> the average, the memory activity is collected over a larger window of
> time. For the consecutive case, the memory activity is collected over
> each period (16ms by default in the driver). Memory client may breach
> average watermark very frequently, although that may not affect much the
> average value and for some memory clients (like CPU) it is more
> preferred to not completely ignore those short bursts of memory
> activity. The consecutive watermarks are used in order to detect those
> short bursts, which we account in the driver in a form of boosting. You
> may notice that boost_up_coeff for the CPU's memory client is set to a
> higher value in the driver.
>
> > Hello Thierry,
> > If possible, Could you review the patch5 related to setting up the watermark
> > and other patches?
> >
>
> Indeed, will be very nice if Thierry could also take a look at this
> series. Although.. I could be wrong here, but it looks to me that
> Thierry also isn't closely familiar with this driver and the hardware.
>
> Thierry, at least please let us know if you're interested in taking a
> look at the patches, I'm pretty sure that you're quite busy with other
> things ;)