More improvements for Tegra30 devfreq driver

---
 drivers/devfreq/tegra30-devfreq.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/devfreq/tegra30-devfreq.c +4 -4

index a6ba75f4106d..a27300f40b0b 100644
@@ -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);

Signature · 1 line

2.23.0

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #2 lore raw

PATCH v7 2/19 PM / devfreq: tegra30: Keep interrupt disabled while governor is stopped

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

---
 drivers/devfreq/tegra30-devfreq.c | 47 ++++++++++++++++---------------
 1 file changed, 24 insertions(+), 23 deletions(-)

drivers/devfreq/tegra30-devfreq.c +24 ~~-23~~

index a27300f40b0b..8be6a33beb9c 100644
@@ -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);

Signature · 1 line

2.23.0

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #3 lore raw

PATCH v7 3/19 PM / devfreq: tegra30: Handle possible round-rate error

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

---
 drivers/devfreq/tegra30-devfreq.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

drivers/devfreq/tegra30-devfreq.c +15 -2

index 8be6a33beb9c..bfee9d43de1e 100644
@@ -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);

Signature · 1 line

2.23.0

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #4 lore raw

PATCH v7 4/19 PM / devfreq: tegra30: Drop write-barrier

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.

---
 drivers/devfreq/tegra30-devfreq.c | 14 --------------
 1 file changed, 14 deletions(-)

drivers/devfreq/tegra30-devfreq.c +0 ~~-14~~

index bfee9d43de1e..ee14bf534c0d 100644
@@ -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,

Signature · 1 line

2.23.0

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #5 lore raw

PATCH v7 5/19 PM / devfreq: tegra30: Fix integer overflow on CPU's freq max out

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.

---
 drivers/devfreq/tegra30-devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/devfreq/tegra30-devfreq.c +3 -1

index ee14bf534c0d..1d22f5239cd5 100644
@@ -69,6 +69,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

@@ -170,7 +172,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 },

Signature · 1 line

2.23.0

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #6 lore raw

PATCH v7 6/19 PM / devfreq: tegra30: Use kHz units uniformly in the code

Part of the code uses Hz units and the other kHz, let's switch to kHz everywhere for consistency. A small benefit from this change (besides code's cleanup) is that now powertop utility correctly displays devfreq's stats, for some reason it expects them to be in kHz.

---
 drivers/devfreq/tegra30-devfreq.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

drivers/devfreq/tegra30-devfreq.c +6 -5

index 1d22f5239cd5..06c5376a7201 100644
@@ -448,7 +448,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;

@@ -477,7 +477,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
 	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];

@@ -527,7 +527,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;
 }

@@ -663,7 +663,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;

@@ -686,7 +686,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 		goto unreg_notifier;
 	}
 
-	tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
+	tegra_devfreq_profile.initial_freq = tegra->cur_freq;
+
 	devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
 				     "tegra_actmon", NULL);
 	if (IS_ERR(devfreq)) {

Signature · 1 line

2.23.0

Chanwoo Choi

maintainer Reviewed-by on v7 6/19

2019-10-31 04:39 UTC #7 lore raw

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

Quoted 53 lines · Dmitry Osipenko

---
 drivers/devfreq/tegra30-devfreq.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

drivers/devfreq/tegra30-devfreq.c +4 -4

index 1d22f5239cd5..06c5376a7201 100644
@@ -448,7 +448,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;

@@ -477,7 +477,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
 	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];

@@ -527,7 +527,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;
 }

@@ -663,7 +663,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;

drivers/devfreq/tegra30-devfreq.c static int tegra_devfreq_probe(struct platform_device *pdev) · line 693

Quoted 9 lines

drivers/devfreq/tegra30-devfreq.c +2 -1

@@ -686,7 +686,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 		goto unreg_notifier;
 	}
 
-	tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
+	tegra_devfreq_profile.initial_freq = tegra->cur_freq;
+
 	devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
 				     "tegra_actmon", NULL);
 	if (IS_ERR(devfreq)) {

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Dmitry Osipenko

author maintainer

2019-10-31 23:08 UTC #8 lore raw

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

31.10.2019 07:44, Chanwoo Choi пишет:

Quoted 56 lines · Dmitry Osipenko

Quoted 55 lines

---
 drivers/devfreq/tegra30-devfreq.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

drivers/devfreq/tegra30-devfreq.c +4 -4

index 1d22f5239cd5..06c5376a7201 100644
@@ -448,7 +448,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;

@@ -477,7 +477,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
 	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];

@@ -527,7 +527,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;
 }

@@ -663,7 +663,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;

@@ -686,7 +686,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 		goto unreg_notifier;
 	}

Quoted 10 lines

drivers/devfreq/tegra30-devfreq.c static int tegra_devfreq_probe(struct platform_device *pdev) · line 693
Quoted 7 lines
drivers/devfreq/tegra30-devfreq.c +2 -1
-	tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
+	tegra_devfreq_profile.initial_freq = tegra->cur_freq;
+
 	devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
 				     "tegra_actmon", NULL);
 	if (IS_ERR(devfreq)) {
Reviewed-by Chanwoo Choi <[email protected]>

Thank you, looking forward to the comments to the rest of the patches. Will be nice if this series could get ready for 5.5.

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #9 lore raw

PATCH v7 7/19 PM / devfreq: tegra30: Use CPUFreq notifier

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.

---
 drivers/devfreq/tegra30-devfreq.c | 180 +++++++++++++++++++++++++-----
 1 file changed, 155 insertions(+), 25 deletions(-)

drivers/devfreq/tegra30-devfreq.c +155 ~~-25~~

index 06c5376a7201..3f2f2223da8c 100644
@@ -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;
 	unsigned long		cur_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)];

@@ -303,22 +309,32 @@ static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra,
 	return 0;
 }
 
+static unsigned long actmon_device_target_freq(struct tegra_devfreq *tegra,
+					       struct tegra_devfreq_device *dev)
+{
+	unsigned int avg_sustain_coef;
+	unsigned long target_freq;
+
+	target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD;
+	avg_sustain_coef = 100 * 100 / dev->config->boost_up_threshold;
+	target_freq = do_percent(target_freq, avg_sustain_coef);
+	target_freq += dev->boost_freq;
+
+	return target_freq;
+}
+
 static void 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);
+		cpu_freq = cpufreq_quick_get(0);
 		static_cpu_emc_freq = actmon_cpu_to_emc_rate(tegra, cpu_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;
+	dev->target_freq = actmon_device_target_freq(tegra, dev);
 
 	if (dev->avg_count >= dev->config->avg_dependency_threshold)
 		dev->target_freq = max(dev->target_freq, static_cpu_emc_freq);

@@ -349,8 +365,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 *tegra;

@@ -360,7 +376,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);
 
 	tegra->cur_freq = data->new_rate / KHZ;

@@ -373,6 +389,79 @@ 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 static_cpu_emc_freq, dev_freq;
+
+	/* check whether CPU's freq is taken into account at all */
+	if (tegra->devices[MCCPU].avg_count <
+	    tegra->devices[MCCPU].config->avg_dependency_threshold)
+		return 0;
+
+	static_cpu_emc_freq = actmon_cpu_to_emc_rate(tegra, cpu_freq);
+	dev_freq = actmon_device_target_freq(tegra, &tegra->devices[MCCPU]);
+
+	if (dev_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)
 {

@@ -405,9 +494,22 @@ 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_devices(struct tegra_devfreq *tegra)
+{
+	struct tegra_devfreq_device *dev = tegra->devices;
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++, 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);

@@ -415,20 +517,41 @@ 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:
+	tegra_actmon_stop_devices(tegra);
+
+	return err;
 }
 
 static void tegra_actmon_stop(struct tegra_devfreq *tegra)
 {
-	unsigned int i;
-
 	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);
+
+	tegra_actmon_stop_devices(tegra);
 }
 
 static int tegra_devfreq_target(struct device *dev, unsigned long *freq,

@@ -536,6 +659,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

@@ -546,7 +670,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:

@@ -561,11 +685,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 = {

@@ -672,8 +796,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;
-	err = clk_notifier_register(tegra->emc_clock, &tegra->rate_change_nb);
+	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");

@@ -701,7 +831,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);

@@ -719,7 +849,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);

Signature · 1 line

2.23.0

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #10 lore raw

PATCH v7 8/19 PM / devfreq: tegra30: Move clk-notifier's registration to governor's start

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.

---
 drivers/devfreq/tegra30-devfreq.c | 33 ++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 14 deletions(-)

drivers/devfreq/tegra30-devfreq.c +19 ~~-14~~

index 3f2f2223da8c..1934b4eb4e78 100644
@@ -514,6 +514,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]);

@@ -539,6 +552,8 @@ static int tegra_actmon_start(struct tegra_devfreq *tegra)
 err_stop:
 	tegra_actmon_stop_devices(tegra);
 
+	clk_notifier_unregister(tegra->emc_clock, &tegra->clk_rate_change_nb);
+
 	return err;
 }

@@ -552,6 +567,8 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
 	cancel_delayed_work_sync(&tegra->cpufreq_update_work);
 
 	tegra_actmon_stop_devices(tegra);
+
+	clk_notifier_unregister(tegra->emc_clock, &tegra->clk_rate_change_nb);
 }
 
 static int tegra_devfreq_target(struct device *dev, unsigned long *freq,

@@ -796,24 +813,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 = tegra->cur_freq;

@@ -830,9 +839,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);

@@ -849,7 +855,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);

Signature · 1 line

2.23.0

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #11 lore raw

PATCH v7 9/19 PM / devfreq: tegra30: Reset boosting on startup

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.

---
 drivers/devfreq/tegra30-devfreq.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/devfreq/tegra30-devfreq.c +3 -0

index 1934b4eb4e78..fb1163aaaae7 100644
@@ -467,6 +467,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->target_freq = tegra->cur_freq;
 
 	dev->avg_count = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;

Signature · 1 line

2.23.0

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #12 lore raw

PATCH v7 10/19 PM / devfreq: tegra30: Don't enable consecutive-down interrupt on startup

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.

---
 drivers/devfreq/tegra30-devfreq.c | 1 -
 1 file changed, 1 deletion(-)

drivers/devfreq/tegra30-devfreq.c +0 -1

index fb1163aaaae7..e9cb0ea54ee7 100644
@@ -490,7 +490,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;

Signature · 1 line

2.23.0

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #13 lore raw

PATCH v7 11/19 PM / devfreq: tegra30: Constify structs

Constify unmodifiable structs, for consistency.

---
 drivers/devfreq/tegra30-devfreq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/devfreq/tegra30-devfreq.c +3 -3

index e9cb0ea54ee7..f658b0b4a845 100644
@@ -108,7 +108,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 },

@@ -295,7 +295,7 @@ 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;
+	const 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) {

Signature · 1 line

2.23.0

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #14 lore raw

PATCH v7 12/19 PM / devfreq: tegra30: Include appropriate header

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

---
 drivers/devfreq/tegra30-devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/devfreq/tegra30-devfreq.c +1 -1

index f658b0b4a845..9da62f695859 100644
@@ -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>

Signature · 1 line

2.23.0

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #15 lore raw

PATCH v7 13/19 PM / devfreq: tegra30: Increase sampling period to 16ms

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.

---
 drivers/devfreq/tegra30-devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/devfreq/tegra30-devfreq.c +1 -1

index 9da62f695859..9cbee82880ff 100644
@@ -67,7 +67,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 ACTMON_DEFAULT_AVG_BAND				6  /* 1/10 of % */
 
 #define KHZ							1000

Signature · 1 line

2.23.0

Michał Mirosław

2019-11-01 21:53 UTC #16 lore raw

Re: [PATCH v7 13/19] PM / devfreq: tegra30: Increase sampling period to 16ms

Quoted 5 lines · 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.

This is made irrelevant by patch 17.

Best Regards,
Michał Mirosław

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #17 lore raw

PATCH v7 14/19 PM / devfreq: tegra30: Don't enable already enabled consecutive interrupts

Consecutive up/down interrupt-bit is set in the interrupt status register only if that interrupt was previously enabled. Thus enabling the already enabled interrupt doesn't do much for us.

---
 drivers/devfreq/tegra30-devfreq.c | 4 ----
 1 file changed, 4 deletions(-)

drivers/devfreq/tegra30-devfreq.c +0 -4

index 9cbee82880ff..a9336cf4b37a 100644
@@ -261,8 +261,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

@@ -275,8 +273,6 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
 
 		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) {

Signature · 1 line

2.23.0

Chanwoo Choi

maintainer Reviewed-by on v7 14/19

2019-11-01 06:55 UTC #18 lore raw

Re: [PATCH v7 14/19] PM / devfreq: tegra30: Don't enable already enabled consecutive interrupts

Quoted 24 lines · Dmitry Osipenko

Consecutive up/down interrupt-bit is set in the interrupt status register only if that interrupt was previously enabled. Thus enabling the already enabled interrupt doesn't do much for us.
Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/devfreq/tegra30-devfreq.c | 4 ----
 1 file changed, 4 deletions(-)
drivers/devfreq/tegra30-devfreq.c +0 -2
index 9cbee82880ff..a9336cf4b37a 100644
@@ -261,8 +261,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

drivers/devfreq/tegra30-devfreq.c static void actmon_isr_device(struct tegra_devfreq *tegra, · line 278

Quoted 8 lines

drivers/devfreq/tegra30-devfreq.c +0 -2

@@ -275,8 +273,6 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
 
 		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) {

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #19 lore raw

PATCH v7 15/19 PM / devfreq: tegra30: Disable consecutive interrupts when appropriate

Consecutive interrupts should be disabled when boosting is completed.

Currently the disabling of "lower" interrupt happens only for MCCPU monitor that uses dependency threshold, but even in a case of MCCPU the interrupt isn't getting disabled if CPU's activity is above the threshold. This results in a lot of dummy interrupt requests. The boosting feature is used by both MCCPU and MCALL, boosting should be stopped once it reaches 0 for both of the monitors and regardless of the activity level.

The boosting stops to grow once the maximum limit is hit and thus the "upper" interrupt needs to be disabled when the limit is reached.

---
 drivers/devfreq/tegra30-devfreq.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

drivers/devfreq/tegra30-devfreq.c +6 -9

index a9336cf4b37a..b745a973c35a 100644
@@ -259,8 +259,10 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
 
 		dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
 
-		if (dev->boost_freq >= tegra->max_freq)
+		if (dev->boost_freq >= tegra->max_freq) {
+			dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
 			dev->boost_freq = tegra->max_freq;
+		}
 	} else if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_LOWER) {
 		/*
 		 * new_boost = old_boost * down_coef

@@ -271,15 +273,10 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
 
 		dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
 
-		if (dev->boost_freq < (ACTMON_BOOST_FREQ_STEP >> 1))
-			dev->boost_freq = 0;
-	}
-
-	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)
+		if (dev->boost_freq < (ACTMON_BOOST_FREQ_STEP >> 1)) {
 			dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+			dev->boost_freq = 0;
+		}
 	}
 
 	device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);

Signature · 1 line

2.23.0

Chanwoo Choi

maintainer Reviewed-by on v7 15/19

2019-11-01 06:55 UTC #20 lore raw

Re: [PATCH v7 15/19] PM / devfreq: tegra30: Disable consecutive interrupts when appropriate

Quoted 43 lines · Dmitry Osipenko

Consecutive interrupts should be disabled when boosting is completed.
Currently the disabling of "lower" interrupt happens only for MCCPU monitor that uses dependency threshold, but even in a case of MCCPU the interrupt isn't getting disabled if CPU's activity is above the threshold. This results in a lot of dummy interrupt requests. The boosting feature is used by both MCCPU and MCALL, boosting should be stopped once it reaches 0 for both of the monitors and regardless of the activity level.
The boosting stops to grow once the maximum limit is hit and thus the "upper" interrupt needs to be disabled when the limit is reached.
Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/devfreq/tegra30-devfreq.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)
drivers/devfreq/tegra30-devfreq.c +3 -6
index a9336cf4b37a..b745a973c35a 100644
@@ -259,8 +259,10 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
 
 		dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
 
-		if (dev->boost_freq >= tegra->max_freq)
+		if (dev->boost_freq >= tegra->max_freq) {
+			dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
 			dev->boost_freq = tegra->max_freq;
+		}
 	} else if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_LOWER) {
 		/*
 		 * new_boost = old_boost * down_coef
@@ -271,15 +273,10 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
 
 		dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
 
-		if (dev->boost_freq < (ACTMON_BOOST_FREQ_STEP >> 1))
-			dev->boost_freq = 0;
-	}
-
-	if (dev->config->avg_dependency_threshold) {

drivers/devfreq/tegra30-devfreq.c static void actmon_isr_device(struct tegra_devfreq *tegra, · line 282

Quoted 10 lines

drivers/devfreq/tegra30-devfreq.c +3 -3

-		if (dev->avg_count >= dev->config->avg_dependency_threshold)
-			dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
-		else if (dev->boost_freq == 0)
+		if (dev->boost_freq < (ACTMON_BOOST_FREQ_STEP >> 1)) {
 			dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
+			dev->boost_freq = 0;
+		}
 	}
 
 	device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #21 lore raw

PATCH v7 16/19 PM / devfreq: Add new interrupt_driven flag for governors

Currently interrupt-driven governors (like NVIDIA Tegra30 ACTMON governor) are used to set polling_ms=0 in order to avoid periodic polling of device status by devfreq core. This means that polling interval can't be changed by userspace for such governors.

The new governor flag allows interrupt-driven governors to convey that devfreq core shouldn't perform polling of device status and thus generic devfreq polling interval could be supported by these governors now.

---
 drivers/devfreq/devfreq.c  | 14 +++++++++-----
 drivers/devfreq/governor.h |  3 +++
 2 files changed, 12 insertions(+), 5 deletions(-)

drivers/devfreq/devfreq.c +9 -5

index b905963cea7d..0ef972264841 100644
@@ -410,7 +410,8 @@ static void devfreq_monitor(struct work_struct *work)
 void devfreq_monitor_start(struct devfreq *devfreq)
 {
 	INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor);
-	if (devfreq->profile->polling_ms)
+	if (devfreq->profile->polling_ms &&
+	    !devfreq->governor->interrupt_driven)
 		queue_delayed_work(devfreq_wq, &devfreq->work,
 			msecs_to_jiffies(devfreq->profile->polling_ms));
 }

@@ -474,7 +475,8 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
 		goto out;
 
 	if (!delayed_work_pending(&devfreq->work) &&
-			devfreq->profile->polling_ms)
+			devfreq->profile->polling_ms &&
+				!devfreq->governor->interrupt_driven)
 		queue_delayed_work(devfreq_wq, &devfreq->work,
 			msecs_to_jiffies(devfreq->profile->polling_ms));

@@ -518,8 +520,9 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
 
 	/* if current delay is zero, start polling with new delay */
 	if (!cur_delay) {
-		queue_delayed_work(devfreq_wq, &devfreq->work,
-			msecs_to_jiffies(devfreq->profile->polling_ms));
+		if (!devfreq->governor->interrupt_driven)
+			queue_delayed_work(devfreq_wq, &devfreq->work,
+				msecs_to_jiffies(devfreq->profile->polling_ms));
 		goto out;
 	}

@@ -528,7 +531,8 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
 		mutex_unlock(&devfreq->lock);
 		cancel_delayed_work_sync(&devfreq->work);
 		mutex_lock(&devfreq->lock);
-		if (!devfreq->stop_polling)
+		if (!devfreq->stop_polling &&
+		    !devfreq->governor->interrupt_driven)
 			queue_delayed_work(devfreq_wq, &devfreq->work,
 				msecs_to_jiffies(devfreq->profile->polling_ms));
 	}

drivers/devfreq/governor.h +3 -0

index bbe5ff9fcecf..dc7533ccc3db 100644
@@ -31,6 +31,8 @@
  * @name:		Governor's name
  * @immutable:		Immutable flag for governor. If the value is 1,
  *			this govenror is never changeable to other governor.
+ * @interrupt_driven:	Devfreq core won't schedule polling work for this
+ *			governor if value is set to 1.
  * @get_target_freq:	Returns desired operating frequency for the device.
  *			Basically, get_target_freq will run
  *			devfreq_dev_profile.get_dev_status() to get the

@@ -49,6 +51,7 @@ struct devfreq_governor {
 
 	const char name[DEVFREQ_NAME_LEN];
 	const unsigned int immutable;
+	const unsigned int interrupt_driven;
 	int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
 	int (*event_handler)(struct devfreq *devfreq,
 				unsigned int event, void *data);

Signature · 1 line

2.23.0

Chanwoo Choi

maintainer

2019-11-01 07:26 UTC #22 lore raw

Re: [PATCH v7 16/19] PM / devfreq: Add new interrupt_driven flag for governors

Hi,

Quoted 30 lines · Dmitry Osipenko

Currently interrupt-driven governors (like NVIDIA Tegra30 ACTMON governor) are used to set polling_ms=0 in order to avoid periodic polling of device status by devfreq core. This means that polling interval can't be changed by userspace for such governors.
The new governor flag allows interrupt-driven governors to convey that devfreq core shouldn't perform polling of device status and thus generic devfreq polling interval could be supported by these governors now.
Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/devfreq/devfreq.c  | 14 +++++++++-----
 drivers/devfreq/governor.h |  3 +++
 2 files changed, 12 insertions(+), 5 deletions(-)
drivers/devfreq/devfreq.c +2 -1
index b905963cea7d..0ef972264841 100644
@@ -410,7 +410,8 @@ static void devfreq_monitor(struct work_struct *work)
 void devfreq_monitor_start(struct devfreq *devfreq)
 {
 	INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor);
-	if (devfreq->profile->polling_ms)
+	if (devfreq->profile->polling_ms &&
+	    !devfreq->governor->interrupt_driven)
 		queue_delayed_work(devfreq_wq, &devfreq->work,
 			msecs_to_jiffies(devfreq->profile->polling_ms));
 }

drivers/devfreq/devfreq.c void devfreq_monitor_resume(struct devfreq *devfreq) · line 479

Quoted 6 lines

drivers/devfreq/devfreq.c +2 -1

@@ -474,7 +475,8 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
 		goto out;
 
 	if (!delayed_work_pending(&devfreq->work) &&
-			devfreq->profile->polling_ms)
+			devfreq->profile->polling_ms &&
+				!devfreq->governor->interrupt_driven)

Better to edit it as following for the indentation.

drivers/devfreq/devfreq.c +1 -1

index cea05b43225f..60c5540b2a55 100644
@@ -477,7 +477,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
 
        if (!delayed_work_pending(&devfreq->work) &&
                        devfreq->profile->polling_ms &&
-                               !devfreq->governor->interrupt_driven)
+                       !devfreq->governor->interrupt_driven)

Quoted 16 lines

drivers/devfreq/devfreq.c +3 -2

 		queue_delayed_work(devfreq_wq, &devfreq->work,
 			msecs_to_jiffies(devfreq->profile->polling_ms));

@@ -518,8 +520,9 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
 
 	/* if current delay is zero, start polling with new delay */
 	if (!cur_delay) {
-		queue_delayed_work(devfreq_wq, &devfreq->work,
-			msecs_to_jiffies(devfreq->profile->polling_ms));
+		if (!devfreq->governor->interrupt_driven)
+			queue_delayed_work(devfreq_wq, &devfreq->work,
+				msecs_to_jiffies(devfreq->profile->polling_ms));
 		goto out;
 	}

drivers/devfreq/devfreq.c void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) · line 538

Quoted 9 lines

drivers/devfreq/devfreq.c +2 -1

@@ -528,7 +531,8 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
 		mutex_unlock(&devfreq->lock);
 		cancel_delayed_work_sync(&devfreq->work);
 		mutex_lock(&devfreq->lock);
-		if (!devfreq->stop_polling)
+		if (!devfreq->stop_polling &&
+		    !devfreq->governor->interrupt_driven)
 			queue_delayed_work(devfreq_wq, &devfreq->work,
 				msecs_to_jiffies(devfreq->profile->polling_ms));
 	}

In the devfreq_interval_update(), you better to modify this function as following:
It is more simple.

drivers/devfreq/devfreq.c +3 -0

index d3d12ed0ed29..80acb55d1686 100644
@@ -510,6 +510,9 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
        if (devfreq->stop_polling)
                goto out;
 
+       if (!devfreq->governor->interrupt_driven)
+               goto out;
+
        /* if new delay is zero, stop polling */
        if (!new_delay) {
                mutex_unlock(&devfreq->lock);

Quoted 22 lines

drivers/devfreq/governor.h +3 -0

index bbe5ff9fcecf..dc7533ccc3db 100644
@@ -31,6 +31,8 @@
  * @name:		Governor's name
  * @immutable:		Immutable flag for governor. If the value is 1,
  *			this govenror is never changeable to other governor.
+ * @interrupt_driven:	Devfreq core won't schedule polling work for this
+ *			governor if value is set to 1.
  * @get_target_freq:	Returns desired operating frequency for the device.
  *			Basically, get_target_freq will run
  *			devfreq_dev_profile.get_dev_status() to get the

@@ -49,6 +51,7 @@ struct devfreq_governor {
 
 	const char name[DEVFREQ_NAME_LEN];
 	const unsigned int immutable;
+	const unsigned int interrupt_driven;
 	int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
 	int (*event_handler)(struct devfreq *devfreq,
 				unsigned int event, void *data);

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Dmitry Osipenko

author maintainer

2019-11-01 13:56 UTC #23 lore raw

Re: [PATCH v7 16/19] PM / devfreq: Add new interrupt_driven flag for governors

01.11.2019 10:32, Chanwoo Choi пишет:

Quoted 116 lines

Hi,

Quoted 30 lines · Dmitry Osipenko

Currently interrupt-driven governors (like NVIDIA Tegra30 ACTMON governor) are used to set polling_ms=0 in order to avoid periodic polling of device status by devfreq core. This means that polling interval can't be changed by userspace for such governors.
The new governor flag allows interrupt-driven governors to convey that devfreq core shouldn't perform polling of device status and thus generic devfreq polling interval could be supported by these governors now.
Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/devfreq/devfreq.c  | 14 +++++++++-----
 drivers/devfreq/governor.h |  3 +++
 2 files changed, 12 insertions(+), 5 deletions(-)
drivers/devfreq/devfreq.c +2 -1
index b905963cea7d..0ef972264841 100644
@@ -410,7 +410,8 @@ static void devfreq_monitor(struct work_struct *work)
 void devfreq_monitor_start(struct devfreq *devfreq)
 {
 	INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor);
-	if (devfreq->profile->polling_ms)
+	if (devfreq->profile->polling_ms &&
+	    !devfreq->governor->interrupt_driven)
 		queue_delayed_work(devfreq_wq, &devfreq->work,
 			msecs_to_jiffies(devfreq->profile->polling_ms));
 }

drivers/devfreq/devfreq.c void devfreq_monitor_resume(struct devfreq *devfreq) · line 479

Quoted 6 lines

drivers/devfreq/devfreq.c +2 -1

@@ -474,7 +475,8 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
 		goto out;
 
 	if (!delayed_work_pending(&devfreq->work) &&
-			devfreq->profile->polling_ms)
+			devfreq->profile->polling_ms &&
+				!devfreq->governor->interrupt_driven)

Better to edit it as following for the indentation.

drivers/devfreq/devfreq.c +1 -1

index cea05b43225f..60c5540b2a55 100644
@@ -477,7 +477,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
 
        if (!delayed_work_pending(&devfreq->work) &&
                        devfreq->profile->polling_ms &&
-                               !devfreq->governor->interrupt_driven)
+                       !devfreq->governor->interrupt_driven)

Quoted 16 lines

drivers/devfreq/devfreq.c +3 -2

 		queue_delayed_work(devfreq_wq, &devfreq->work,
 			msecs_to_jiffies(devfreq->profile->polling_ms));

@@ -518,8 +520,9 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
 
 	/* if current delay is zero, start polling with new delay */
 	if (!cur_delay) {
-		queue_delayed_work(devfreq_wq, &devfreq->work,
-			msecs_to_jiffies(devfreq->profile->polling_ms));
+		if (!devfreq->governor->interrupt_driven)
+			queue_delayed_work(devfreq_wq, &devfreq->work,
+				msecs_to_jiffies(devfreq->profile->polling_ms));
 		goto out;
 	}

drivers/devfreq/devfreq.c void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) · line 538

Quoted 9 lines

drivers/devfreq/devfreq.c +2 -1

@@ -528,7 +531,8 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
 		mutex_unlock(&devfreq->lock);
 		cancel_delayed_work_sync(&devfreq->work);
 		mutex_lock(&devfreq->lock);
-		if (!devfreq->stop_polling)
+		if (!devfreq->stop_polling &&
+		    !devfreq->governor->interrupt_driven)
 			queue_delayed_work(devfreq_wq, &devfreq->work,
 				msecs_to_jiffies(devfreq->profile->polling_ms));
 	}

In the devfreq_interval_update(), you better to modify this function as following:
It is more simple.

drivers/devfreq/devfreq.c +3 -0

index d3d12ed0ed29..80acb55d1686 100644
@@ -510,6 +510,9 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
        if (devfreq->stop_polling)
                goto out;
 
+       if (!devfreq->governor->interrupt_driven)
+               goto out;
+
        /* if new delay is zero, stop polling */
        if (!new_delay) {
                mutex_unlock(&devfreq->lock);

Quoted 18 lines

drivers/devfreq/governor.h +2 -0

index bbe5ff9fcecf..dc7533ccc3db 100644
@@ -31,6 +31,8 @@
  * @name:		Governor's name
  * @immutable:		Immutable flag for governor. If the value is 1,
  *			this govenror is never changeable to other governor.
+ * @interrupt_driven:	Devfreq core won't schedule polling work for this
+ *			governor if value is set to 1.
  * @get_target_freq:	Returns desired operating frequency for the device.
  *			Basically, get_target_freq will run
  *			devfreq_dev_profile.get_dev_status() to get the

@@ -49,6 +51,7 @@ struct devfreq_governor {

Quoted 9 lines

drivers/devfreq/governor.h struct devfreq_governor { · line 57

Quoted 7 lines

drivers/devfreq/governor.h +1 -0

 	const char name[DEVFREQ_NAME_LEN];
 	const unsigned int immutable;
+	const unsigned int interrupt_driven;
 	int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
 	int (*event_handler)(struct devfreq *devfreq,
 				unsigned int event, void *data);

Okay, I'll update this patch.

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #24 lore raw

PATCH v7 17/19 PM / devfreq: tegra30: Support variable polling interval

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.

---
 drivers/devfreq/tegra30-devfreq.c | 35 ++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

drivers/devfreq/tegra30-devfreq.c +28 -7

index b745a973c35a..d0dd42856e5b 100644
@@ -218,7 +218,7 @@ static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
 {
 	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;
+	u32 band = avg_band_freq * tegra->devfreq->profile->polling_ms;
 
 	device_writel(dev, avg + band, ACTMON_DEV_AVG_UPPER_WMARK);

@@ -229,7 +229,7 @@ static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
 static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
 				       struct tegra_devfreq_device *dev)
 {
-	u32 val = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
+	u32 val = tegra->cur_freq * tegra->devfreq->profile->polling_ms;
 
 	device_writel(dev, do_percent(val, dev->config->boost_up_threshold),
 		      ACTMON_DEV_UPPER_WMARK);

@@ -308,7 +308,7 @@ static unsigned long actmon_device_target_freq(struct tegra_devfreq *tegra,
 	unsigned int avg_sustain_coef;
 	unsigned long target_freq;
 
-	target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD;
+	target_freq = dev->avg_count / tegra->devfreq->profile->polling_ms;
 	avg_sustain_coef = 100 * 100 / dev->config->boost_up_threshold;
 	target_freq = do_percent(target_freq, avg_sustain_coef);
 	target_freq += dev->boost_freq;

@@ -465,7 +465,7 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
 
 	dev->target_freq = tegra->cur_freq;
 
-	dev->avg_count = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
+	dev->avg_count = tegra->cur_freq * tegra->devfreq->profile->polling_ms;
 	device_writel(dev, dev->avg_count, ACTMON_DEV_INIT_AVG);
 
 	tegra_devfreq_update_avg_wmark(tegra, dev);

@@ -506,7 +506,11 @@ static int tegra_actmon_start(struct tegra_devfreq *tegra)
 	unsigned int i;
 	int err;
 
-	actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
+	if (!tegra->devfreq->profile->polling_ms ||
+	    tegra->devfreq->stop_polling)
+		return 0;
+
+	actmon_writel(tegra, tegra->devfreq->profile->polling_ms - 1,
 		      ACTMON_GLB_PERIOD_CTRL);
 
 	/*

@@ -554,6 +558,10 @@ static int tegra_actmon_start(struct tegra_devfreq *tegra)
 
 static void tegra_actmon_stop(struct tegra_devfreq *tegra)
 {
+	if (!tegra->devfreq->profile->polling_ms ||
+	    tegra->devfreq->stop_polling)
+		return;
+
 	disable_irq(tegra->irq);
 
 	cpufreq_unregister_notifier(&tegra->cpu_rate_change_nb,

@@ -623,7 +631,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 = tegra->devfreq->profile->polling_ms * cur_freq;
 
 	stat->busy_time = min(stat->busy_time, stat->total_time);

@@ -631,7 +639,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,
 };

@@ -671,6 +679,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);
+	unsigned int *new_delay = data;
 	int ret = 0;
 
 	/*

@@ -690,6 +699,17 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
 		devfreq_monitor_stop(devfreq);
 		break;
 
+	case DEVFREQ_GOV_INTERVAL:
+		if (*new_delay > 256) {
+			ret = -EINVAL;
+			break;
+		}
+
+		tegra_actmon_stop(tegra);
+		devfreq_interval_update(devfreq, new_delay);
+		ret = tegra_actmon_start(tegra);
+		break;
+
 	case DEVFREQ_GOV_SUSPEND:
 		tegra_actmon_stop(tegra);
 		devfreq_monitor_suspend(devfreq);

@@ -709,6 +729,7 @@ static struct devfreq_governor tegra_devfreq_governor = {
 	.get_target_freq = tegra_governor_get_target,
 	.event_handler = tegra_governor_event_handler,
 	.immutable = true,
+	.interrupt_driven = true,
 };
 
 static int tegra_devfreq_probe(struct platform_device *pdev)

Signature · 1 line

2.23.0

Chanwoo Choi

maintainer

2019-11-01 07:36 UTC #25 lore raw

Re: [PATCH v7 17/19] PM / devfreq: tegra30: Support variable polling interval

Quoted 52 lines · Dmitry Osipenko

---
 drivers/devfreq/tegra30-devfreq.c | 35 ++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

drivers/devfreq/tegra30-devfreq.c +4 -4

index b745a973c35a..d0dd42856e5b 100644
@@ -218,7 +218,7 @@ static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
 {
 	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;
+	u32 band = avg_band_freq * tegra->devfreq->profile->polling_ms;
 
 	device_writel(dev, avg + band, ACTMON_DEV_AVG_UPPER_WMARK);

@@ -229,7 +229,7 @@ static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
 static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
 				       struct tegra_devfreq_device *dev)
 {
-	u32 val = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
+	u32 val = tegra->cur_freq * tegra->devfreq->profile->polling_ms;
 
 	device_writel(dev, do_percent(val, dev->config->boost_up_threshold),
 		      ACTMON_DEV_UPPER_WMARK);

@@ -308,7 +308,7 @@ static unsigned long actmon_device_target_freq(struct tegra_devfreq *tegra,
 	unsigned int avg_sustain_coef;
 	unsigned long target_freq;
 
-	target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD;
+	target_freq = dev->avg_count / tegra->devfreq->profile->polling_ms;
 	avg_sustain_coef = 100 * 100 / dev->config->boost_up_threshold;
 	target_freq = do_percent(target_freq, avg_sustain_coef);
 	target_freq += dev->boost_freq;

@@ -465,7 +465,7 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
 
 	dev->target_freq = tegra->cur_freq;
 
-	dev->avg_count = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
+	dev->avg_count = tegra->cur_freq * tegra->devfreq->profile->polling_ms;
 	device_writel(dev, dev->avg_count, ACTMON_DEV_INIT_AVG);
 
 	tegra_devfreq_update_avg_wmark(tegra, dev);

drivers/devfreq/tegra30-devfreq.c static int tegra_actmon_start(struct tegra_devfreq *tegra) · line 510

Quoted 6 lines

drivers/devfreq/tegra30-devfreq.c +2 -1

@@ -506,7 +506,11 @@ static int tegra_actmon_start(struct tegra_devfreq *tegra)
 	unsigned int i;
 	int err;
 
-	actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
+	if (!tegra->devfreq->profile->polling_ms ||
+	    tegra->devfreq->stop_polling)

I think that the access of 'devfreq->stop_polling' is not good
on governor. It is possible to alter with DEVFREQ_GOV_START/STOP/SUSPEND/RESUME notification.

drivers/devfreq/tegra30-devfreq.c static int tegra_actmon_start(struct tegra_devfreq *tegra) · line 562

Quoted 11 lines

drivers/devfreq/tegra30-devfreq.c +4 -0

+		return 0;> +
+	actmon_writel(tegra, tegra->devfreq->profile->polling_ms - 1,
 		      ACTMON_GLB_PERIOD_CTRL);
 
 	/*

@@ -554,6 +558,10 @@ static int tegra_actmon_start(struct tegra_devfreq *tegra)
 
 static void tegra_actmon_stop(struct tegra_devfreq *tegra)
 {
+	if (!tegra->devfreq->profile->polling_ms ||
+	    tegra->devfreq->stop_polling)

ditto.

Quoted 32 lines

drivers/devfreq/tegra30-devfreq.c +5 -2

+		return;
+
 	disable_irq(tegra->irq);
 
 	cpufreq_unregister_notifier(&tegra->cpu_rate_change_nb,

@@ -623,7 +631,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 = tegra->devfreq->profile->polling_ms * cur_freq;
 
 	stat->busy_time = min(stat->busy_time, stat->total_time);

@@ -631,7 +639,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,
 };

@@ -671,6 +679,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);
+	unsigned int *new_delay = data;
 	int ret = 0;
 
 	/*

drivers/devfreq/tegra30-devfreq.c static int tegra_governor_event_handler(struct devfreq *devfreq, · line 703

Quoted 5 lines

drivers/devfreq/tegra30-devfreq.c +2 -0

@@ -690,6 +699,17 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
 		devfreq_monitor_stop(devfreq);
 		break;
 
+	case DEVFREQ_GOV_INTERVAL:
+		if (*new_delay > 256) {

Need to add the comment why the maximum delay is under 256 ms.

Quoted 21 lines

drivers/devfreq/tegra30-devfreq.c +10 -0

+			ret = -EINVAL;
+			break;
+		}
+
+		tegra_actmon_stop(tegra);
+		devfreq_interval_update(devfreq, new_delay);
+		ret = tegra_actmon_start(tegra);
+		break;
+
 	case DEVFREQ_GOV_SUSPEND:
 		tegra_actmon_stop(tegra);
 		devfreq_monitor_suspend(devfreq);

@@ -709,6 +729,7 @@ static struct devfreq_governor tegra_devfreq_governor = {
 	.get_target_freq = tegra_governor_get_target,
 	.event_handler = tegra_governor_event_handler,
 	.immutable = true,
+	.interrupt_driven = true,
 };
 
 static int tegra_devfreq_probe(struct platform_device *pdev)

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Dmitry Osipenko

author maintainer

2019-11-01 13:55 UTC #26 lore raw

Re: [PATCH v7 17/19] PM / devfreq: tegra30: Support variable polling interval

01.11.2019 10:41, Chanwoo Choi пишет:

Quoted 56 lines · Dmitry Osipenko

Quoted 55 lines

---
 drivers/devfreq/tegra30-devfreq.c | 35 ++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

drivers/devfreq/tegra30-devfreq.c +4 -4

index b745a973c35a..d0dd42856e5b 100644
@@ -218,7 +218,7 @@ static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
 {
 	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;
+	u32 band = avg_band_freq * tegra->devfreq->profile->polling_ms;
 
 	device_writel(dev, avg + band, ACTMON_DEV_AVG_UPPER_WMARK);

@@ -229,7 +229,7 @@ static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
 static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
 				       struct tegra_devfreq_device *dev)
 {
-	u32 val = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
+	u32 val = tegra->cur_freq * tegra->devfreq->profile->polling_ms;
 
 	device_writel(dev, do_percent(val, dev->config->boost_up_threshold),
 		      ACTMON_DEV_UPPER_WMARK);

@@ -308,7 +308,7 @@ static unsigned long actmon_device_target_freq(struct tegra_devfreq *tegra,
 	unsigned int avg_sustain_coef;
 	unsigned long target_freq;
 
-	target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD;
+	target_freq = dev->avg_count / tegra->devfreq->profile->polling_ms;
 	avg_sustain_coef = 100 * 100 / dev->config->boost_up_threshold;
 	target_freq = do_percent(target_freq, avg_sustain_coef);
 	target_freq += dev->boost_freq;

@@ -465,7 +465,7 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
 
 	dev->target_freq = tegra->cur_freq;
 
-	dev->avg_count = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
+	dev->avg_count = tegra->cur_freq * tegra->devfreq->profile->polling_ms;
 	device_writel(dev, dev->avg_count, ACTMON_DEV_INIT_AVG);
 
 	tegra_devfreq_update_avg_wmark(tegra, dev);

@@ -506,7 +506,11 @@ static int tegra_actmon_start(struct tegra_devfreq *tegra)
 	unsigned int i;
 	int err;

Quoted 7 lines

drivers/devfreq/tegra30-devfreq.c static int tegra_actmon_start(struct tegra_devfreq *tegra) · line 510
Quoted 3 lines
drivers/devfreq/tegra30-devfreq.c +2 -1
-	actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
+	if (!tegra->devfreq->profile->polling_ms ||
+	    tegra->devfreq->stop_polling)
I think that the access of 'devfreq->stop_polling' is not good
on governor. It is possible to alter with DEVFREQ_GOV_START/STOP/SUSPEND/RESUME notification.

Could you please clarify what you're meaning by "alter with notification"?

Do you mean to have the start/stop state tracking done by the tegra30-devfreq driver itself?

Quoted 49 lines

drivers/devfreq/tegra30-devfreq.c static int tegra_actmon_start(struct tegra_devfreq *tegra) · line 562

Quoted 11 lines

drivers/devfreq/tegra30-devfreq.c +4 -0

+		return 0;> +
+	actmon_writel(tegra, tegra->devfreq->profile->polling_ms - 1,
 		      ACTMON_GLB_PERIOD_CTRL);
 
 	/*

@@ -554,6 +558,10 @@ static int tegra_actmon_start(struct tegra_devfreq *tegra)
 
 static void tegra_actmon_stop(struct tegra_devfreq *tegra)
 {
+	if (!tegra->devfreq->profile->polling_ms ||
+	    tegra->devfreq->stop_polling)

ditto.

Quoted 35 lines

drivers/devfreq/tegra30-devfreq.c +5 -2

+		return;
+
 	disable_irq(tegra->irq);
 
 	cpufreq_unregister_notifier(&tegra->cpu_rate_change_nb,

@@ -623,7 +631,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 = tegra->devfreq->profile->polling_ms * cur_freq;
 
 	stat->busy_time = min(stat->busy_time, stat->total_time);

@@ -631,7 +639,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,
 };

@@ -671,6 +679,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);
+	unsigned int *new_delay = data;
 	int ret = 0;
 
 	/*

@@ -690,6 +699,17 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
 		devfreq_monitor_stop(devfreq);
 		break;

Quoted 4 lines

drivers/devfreq/tegra30-devfreq.c static int tegra_governor_event_handler(struct devfreq *devfreq, · line 703
Quoted 2 lines
drivers/devfreq/tegra30-devfreq.c +2 -0
+	case DEVFREQ_GOV_INTERVAL:
+		if (*new_delay > 256) {
Need to add the comment why the maximum delay is under 256 ms.

Okay, will add comment in v8. The 256ms max is the hardware's capability.

Michał Mirosław

2019-11-01 21:58 UTC #27 lore raw

Re: [PATCH v7 17/19] PM / devfreq: tegra30: Support variable polling interval

Quoted 5 lines · 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.

[...]

drivers/devfreq/tegra30-devfreq.c static unsigned long actmon_device_target_freq(struct tegra_devfreq *tegra, · line 314

Quoted 9 lines

drivers/devfreq/tegra30-devfreq.c +1 -1

@@ -308,7 +308,7 @@ static unsigned long actmon_device_target_freq(struct tegra_devfreq *tegra,
 	unsigned int avg_sustain_coef;
 	unsigned long target_freq;
 
-	target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD;
+	target_freq = dev->avg_count / tegra->devfreq->profile->polling_ms;
 	avg_sustain_coef = 100 * 100 / dev->config->boost_up_threshold;
 	target_freq = do_percent(target_freq, avg_sustain_coef);
 	target_freq += dev->boost_freq;

Noting a comment in patch 13, if this is hot path you could try reciprocal_divide().

Best Regards,
Michał Mirosław

Dmitry Osipenko

author maintainer

2019-11-01 22:23 UTC #28 lore raw

Re: [PATCH v7 17/19] PM / devfreq: tegra30: Support variable polling interval

02.11.2019 00:58, Michał Mirosław пишет:

Quoted 11 lines · Dmitry Osipenko

Quoted 5 lines
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.
[...]
Quoted 4 lines
drivers/devfreq/tegra30-devfreq.c +0 -0
@@ -308,7 +308,7 @@ static unsigned long actmon_device_target_freq(struct tegra_devfreq *tegra,
 	unsigned int avg_sustain_coef;
 	unsigned long target_freq;
 

Quoted 7 lines

drivers/devfreq/tegra30-devfreq.c static unsigned long actmon_device_target_freq(struct tegra_devfreq *tegra, · line 314
Quoted 5 lines
drivers/devfreq/tegra30-devfreq.c +1 -1
-	target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD;
+	target_freq = dev->avg_count / tegra->devfreq->profile->polling_ms;
 	avg_sustain_coef = 100 * 100 / dev->config->boost_up_threshold;
 	target_freq = do_percent(target_freq, avg_sustain_coef);
 	target_freq += dev->boost_freq;
Noting a comment in patch 13, if this is hot path you could try reciprocal_divide().

Hello Michał,

This not really a hot path, I just wanted to optimize that case to keep things a bit nicer.

Please take a look at the arch/arm/boot/compressed/lib1funcs.S, firstly it checks whether divisor is a power of 2 value and then takes optimized code path that uses a single shift. Hence the patch 13 still applies here.

Dmitry Osipenko

author maintainer

2019-11-03 15:25 UTC #29 lore raw

Re: [PATCH v7 17/19] PM / devfreq: tegra30: Support variable polling interval

02.11.2019 01:23, Dmitry Osipenko пишет:

Quoted 20 lines

02.11.2019 00:58, Michał Mirosław пишет:
Quoted 11 lines · Dmitry Osipenko
Quoted 5 lines
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.
[...]
Quoted 4 lines
drivers/devfreq/tegra30-devfreq.c +0 -0
@@ -308,7 +308,7 @@ static unsigned long actmon_device_target_freq(struct tegra_devfreq *tegra,
 	unsigned int avg_sustain_coef;
 	unsigned long target_freq;
 
Quoted 7 lines
drivers/devfreq/tegra30-devfreq.c static unsigned long actmon_device_target_freq(struct tegra_devfreq *tegra, · line 314
Quoted 5 lines
drivers/devfreq/tegra30-devfreq.c +1 -1
-	target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD;
+	target_freq = dev->avg_count / tegra->devfreq->profile->polling_ms;
 	avg_sustain_coef = 100 * 100 / dev->config->boost_up_threshold;
 	target_freq = do_percent(target_freq, avg_sustain_coef);
 	target_freq += dev->boost_freq;
Noting a comment in patch 13, if this is hot path you could try reciprocal_divide().

Quoted 8 lines

Hello Michał,
This not really a hot path, I just wanted to optimize that case to keep things a bit nicer.
Please take a look at the arch/arm/boot/compressed/lib1funcs.S, firstly it checks whether divisor is a power of 2 value and then takes optimized code path that uses a single shift. Hence the patch 13 still applies here.

On the other hand, there is now only a single case of the division by polling_ms in the driver which won't bring much benefit, so it indeed makes sense to skip the patch 13 for now.

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #30 lore raw

PATCH v7 18/19 PM / devfreq: tegra30: Tune up MCCPU boost-down coefficient

MCCPU boosts up very aggressively by 800% and boosts down very mildly by 10%. This doesn't work well when system is idling because the very slow de-boosting results in lots of consecutive-down interrupts, in result memory stays clocked high and CPU doesn't enter deepest idling state instead of keeping memory at lowest freq and having CPU cluster turned off. A more faster de-boosting fixes the case of idling system and doesn't affect the case of an active system.

---
 drivers/devfreq/tegra30-devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/devfreq/tegra30-devfreq.c +1 -1

index d0dd42856e5b..9a21a29198ee 100644
@@ -123,7 +123,7 @@ static const struct tegra_devfreq_device_config actmon_device_configs[] = {
 		.offset = 0x200,
 		.irq_mask = 1 << 25,
 		.boost_up_coeff = 800,
-		.boost_down_coeff = 90,
+		.boost_down_coeff = 40,
 		.boost_up_threshold = 27,
 		.boost_down_threshold = 10,
 		.avg_dependency_threshold = 50000,

Signature · 1 line

2.23.0

Chanwoo Choi

maintainer Reviewed-by on v7 18/19

2019-11-01 07:40 UTC #31 lore raw

Re: [PATCH v7 18/19] PM / devfreq: tegra30: Tune up MCCPU boost-down coefficient

Quoted 19 lines · Dmitry Osipenko

MCCPU boosts up very aggressively by 800% and boosts down very mildly by 10%. This doesn't work well when system is idling because the very slow de-boosting results in lots of consecutive-down interrupts, in result memory stays clocked high and CPU doesn't enter deepest idling state instead of keeping memory at lowest freq and having CPU cluster turned off. A more faster de-boosting fixes the case of idling system and doesn't affect the case of an active system.
Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/devfreq/tegra30-devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/devfreq/tegra30-devfreq.c +0 -0
index d0dd42856e5b..9a21a29198ee 100644

drivers/devfreq/tegra30-devfreq.c static const struct tegra_devfreq_device_config actmon_device_configs[] = { · line 129

Quoted 8 lines

drivers/devfreq/tegra30-devfreq.c +1 -1

@@ -123,7 +123,7 @@ static const struct tegra_devfreq_device_config actmon_device_configs[] = {
 		.offset = 0x200,
 		.irq_mask = 1 << 25,
 		.boost_up_coeff = 800,
-		.boost_down_coeff = 90,
+		.boost_down_coeff = 40,
 		.boost_up_threshold = 27,
 		.boost_down_threshold = 10,
 		.avg_dependency_threshold = 50000,

IMO, I think that it is not good to change the threshold value on device driver directly when some requirement happen. Instead, better to get the threshold value from device-tree file.

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Dmitry Osipenko

author maintainer

2019-11-01 14:14 UTC #32 lore raw

Re: [PATCH v7 18/19] PM / devfreq: tegra30: Tune up MCCPU boost-down coefficient

01.11.2019 10:45, Chanwoo Choi пишет:

Quoted 31 lines · Dmitry Osipenko

Quoted 19 lines
MCCPU boosts up very aggressively by 800% and boosts down very mildly by 10%. This doesn't work well when system is idling because the very slow de-boosting results in lots of consecutive-down interrupts, in result memory stays clocked high and CPU doesn't enter deepest idling state instead of keeping memory at lowest freq and having CPU cluster turned off. A more faster de-boosting fixes the case of idling system and doesn't affect the case of an active system.
Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/devfreq/tegra30-devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/devfreq/tegra30-devfreq.c +0 -0
index d0dd42856e5b..9a21a29198ee 100644
drivers/devfreq/tegra30-devfreq.c static const struct tegra_devfreq_device_config actmon_device_configs[] = { · line 129
Quoted 8 lines
drivers/devfreq/tegra30-devfreq.c +1 -1
@@ -123,7 +123,7 @@ static const struct tegra_devfreq_device_config actmon_device_configs[] = {
 		.offset = 0x200,
 		.irq_mask = 1 << 25,
 		.boost_up_coeff = 800,
-		.boost_down_coeff = 90,
+		.boost_down_coeff = 40,
 		.boost_up_threshold = 27,
 		.boost_down_threshold = 10,
 		.avg_dependency_threshold = 50000,
Reviewed-by Chanwoo Choi <[email protected]>

Quoted 3 lines

IMO, I think that it is not good to change the threshold value on device driver directly when some requirement happen. Instead, better to get the threshold value from device-tree file.

I think in worst case these values could be tuned for a hardware generation. The current hardcoded values should be good enough for everyone.

If we'll find that some differentiation is needed, then we could consider making the pre-defined config per hardware generation by specifying "data" fields of the of_device_id table.

It also should be possible add sysfs interface for userpace to change the boosting values. Or make these values a driver's module parameters.

There are different variants of what could be done in order to differentiate the configuration. But again, this is not needed for.

Dmitry Osipenko

author maintainer

2019-10-29 22:00 UTC #33 lore raw

PATCH v7 19/19 PM / devfreq: tegra20/30: Add Dmitry as a maintainer

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 <[email protected]>