[PATCH v2 00/19] NVIDIA Tegra devfreq improvements and Tegra20/30 support

[PATCH v2 00/19] NVIDIA Tegra devfreq improvements and Tegra20/30 support

[Dmitry Osipenko]

Hello,

I tried to utilize the Tegra devfreq driver on Tegra30 and found out that
it doesn't work properly due to improper Hz<->kHz conversions made by the
driver. After fixing that problem and doing some more testing I noticed
that there are things that could be improved and in result here is this
patchset that fixes the problems, makes some improvements and adds support
for NVIDIA Tegra30 SoC's. This series was tested on Tegra30 and Tegra124
machines.

This series also adds support for the Tegra20 SoC's that do not have
the ACTMON hardware unit, but can use Memory Controller counters to make
decision about the required memory frequency. Please note that the patch
that instantiates a platform device for the driver isn't a part of this
series, I'll submit it later on because of interdependency with some other
ongoing patchset.

Please review, thanks.

Changelog:

v2: The patchset was quite heavily reworked since v1, few patches we
    dropped or squashed into the new ones and more patches we added.
    In a result more bugs and potential problems are fixed now, driver's
    code got more clean up.

    The Tegra20 driver-addition patch is now a part of this series, it has
    no changes since v1.

Dmitry Osipenko (19):
  PM / devfreq: tegra: Fix kHz to Hz conversion
  PM / devfreq: tegra: Replace readl-writel with relaxed versions
  PM / devfreq: tegra: Don't ignore clk errors
  PM / devfreq: tegra: Don't set EMC clock rate to maximum on probe
  PM / devfreq: tegra: Replace write memory barrier with the read
    barrier
  PM / devfreq: tegra: Fix missed error checking on devfreq
    initialization failure
  PM / devfreq: tegra: Register clk notifier in the end of driver's
    probe
  PM / devfreq: tegra: Remove OPP entries on driver removal
  PM / devfreq: tegra: Change interrupt request order
  PM / devfreq: tegra: Drop primary interrupt handler
  PM / devfreq: tegra: De-initialize properly on driver's probe error
  PM / devfreq: tegra: Avoid inconsistency of current frequency value
  PM / devfreq: tegra: Mark ACTMON's governor as immutable
  PM / devfreq: tegra: Move governor registration to driver's probe
  PM / devfreq: tegra: Synchronize IRQ after masking it in hardware
  PM / devfreq: tegra: Reconfigure hardware on governor's restart
  PM / devfreq: tegra: Support Tegra30
  PM / devfreq: tegra: Enable COMPILE_TEST for the driver
  PM / devfreq: Introduce driver for NVIDIA Tegra20

 MAINTAINERS                       |   8 +
 drivers/devfreq/Kconfig           |  15 +-
 drivers/devfreq/Makefile          |   1 +
 drivers/devfreq/tegra-devfreq.c   | 296 +++++++++++++-----------------
 drivers/devfreq/tegra20-devfreq.c | 177 ++++++++++++++++++
 5 files changed, 324 insertions(+), 173 deletions(-)
 create mode 100644 drivers/devfreq/tegra20-devfreq.c

-- 
2.21.0

[PATCH v2 01/19] PM / devfreq: tegra: Fix kHz to Hz conversion

[Dmitry Osipenko]

The kHz to Hz is incorrectly converted in a few places in the code,
this results in a wrong frequency being calculated because devfreq core
uses OPP frequencies that are given in Hz to clamp the rate, while
tegra-devfreq gives to the core value in kHz and then it also expects to
receive value in kHz from the core. In a result memory freq is always set
to a value which is close to ULONG_MAX because of the bug. Hence the EMC
frequency is always capped to the maximum and the driver doesn't do
anything useful. This patch was tested on Tegra30 and Tegra124 SoC's, EMC
frequency scaling works properly now.

Cc: <stable@vger.kernel.org> # 4.14+
Tested-by: Steev Klimaszewski <steev@kali.org>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra-devfreq.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index c89ba7b834ff..02905978abe1 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -486,9 +486,9 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
 {
 	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
 	struct dev_pm_opp *opp;
-	unsigned long rate = *freq * KHZ;
+	unsigned long rate;
 
-	opp = devfreq_recommended_opp(dev, &rate, flags);
+	opp = devfreq_recommended_opp(dev, freq, flags);
 	if (IS_ERR(opp)) {
 		dev_err(dev, "Failed to find opp for %lu KHz\n", *freq);
 		return PTR_ERR(opp);
@@ -499,8 +499,6 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
 	clk_set_min_rate(tegra->emc_clock, rate);
 	clk_set_rate(tegra->emc_clock, 0);
 
-	*freq = rate;
-
 	return 0;
 }
 
@@ -510,7 +508,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
 	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
 	struct tegra_devfreq_device *actmon_dev;
 
-	stat->current_frequency = tegra->cur_freq;
+	stat->current_frequency = tegra->cur_freq * KHZ;
 
 	/* To be used by the tegra governor */
 	stat->private_data = tegra;
@@ -565,7 +563,7 @@ static int tegra_governor_get_target(struct devfreq *devfreq,
 		target_freq = max(target_freq, dev->target_freq);
 	}
 
-	*freq = target_freq;
+	*freq = target_freq * KHZ;
 
 	return 0;
 }
-- 
2.21.0

[PATCH v2 02/19] PM / devfreq: tegra: Replace readl-writel with relaxed versions

[Dmitry Osipenko]

There is no need to insert memory barrier on each readl/writel
invocation, hence use the relaxed versions.

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

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 02905978abe1..0c0909fba545 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -191,23 +191,23 @@ static struct tegra_actmon_emc_ratio actmon_emc_ratios[] = {
 
 static u32 actmon_readl(struct tegra_devfreq *tegra, u32 offset)
 {
-	return readl(tegra->regs + offset);
+	return readl_relaxed(tegra->regs + offset);
 }
 
 static void actmon_writel(struct tegra_devfreq *tegra, u32 val, u32 offset)
 {
-	writel(val, tegra->regs + offset);
+	writel_relaxed(val, tegra->regs + offset);
 }
 
 static u32 device_readl(struct tegra_devfreq_device *dev, u32 offset)
 {
-	return readl(dev->regs + offset);
+	return readl_relaxed(dev->regs + offset);
 }
 
 static void device_writel(struct tegra_devfreq_device *dev, u32 val,
 			  u32 offset)
 {
-	writel(val, dev->regs + offset);
+	writel_relaxed(val, dev->regs + offset);
 }
 
 static unsigned long do_percent(unsigned long val, unsigned int pct)
-- 
2.21.0

[PATCH v2 03/19] PM / devfreq: tegra: Don't ignore clk errors

[Dmitry Osipenko]

The clk_set_min_rate() could fail and in this case clk_set_rate() sets
rate to 0, which may drop EMC rate to minimum and make machine very
difficult to use.

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

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 0c0909fba545..69581c9082d4 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -487,6 +487,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
 	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
 	struct dev_pm_opp *opp;
 	unsigned long rate;
+	int err;
 
 	opp = devfreq_recommended_opp(dev, freq, flags);
 	if (IS_ERR(opp)) {
@@ -496,8 +497,13 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
 	rate = dev_pm_opp_get_freq(opp);
 	dev_pm_opp_put(opp);
 
-	clk_set_min_rate(tegra->emc_clock, rate);
-	clk_set_rate(tegra->emc_clock, 0);
+	err = clk_set_min_rate(tegra->emc_clock, rate);
+	if (err)
+		return err;
+
+	err = clk_set_rate(tegra->emc_clock, 0);
+	if (err)
+		return err;
 
 	return 0;
 }
-- 
2.21.0

[PATCH v2 04/19] PM / devfreq: tegra: Don't set EMC clock rate to maximum on probe

[Dmitry Osipenko]

There is no real benefit from doing so, hence let's drop that rate setting
for consistency.

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

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 69581c9082d4..d62fb1b0d9bb 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -648,8 +648,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 		return PTR_ERR(tegra->emc_clock);
 	}
 
-	clk_set_rate(tegra->emc_clock, ULONG_MAX);
-
 	tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
 	err = clk_notifier_register(tegra->emc_clock, &tegra->rate_change_nb);
 	if (err) {
-- 
2.21.0

[PATCH v2 05/19] PM / devfreq: tegra: Replace write memory barrier with the read barrier

[Dmitry Osipenko]

The write memory barrier isn't needed because the BUS buffer is flushed
by read after write that happens after the removed wmb(), we will also
use readl() instead of the relaxed version to ensure that read is indeed
completed.

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

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index d62fb1b0d9bb..f0f0d78f6cbf 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -243,8 +243,7 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
 static void actmon_write_barrier(struct tegra_devfreq *tegra)
 {
 	/* ensure the update has reached the ACTMON */
-	wmb();
-	actmon_readl(tegra, ACTMON_GLB_STATUS);
+	readl(tegra->regs + ACTMON_GLB_STATUS);
 }
 
 static void actmon_isr_device(struct tegra_devfreq *tegra,
-- 
2.21.0

[PATCH v2 06/19] PM / devfreq: tegra: Fix missed error checking on devfreq initialization failure

[Dmitry Osipenko]

The code doesn't check for devfreq initialization failure, fix it.

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

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index f0f0d78f6cbf..aa0478464b35 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -707,6 +707,10 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 						 &tegra_devfreq_profile,
 						 "tegra_actmon",
 						 NULL);
+	if (IS_ERR(tegra->devfreq)) {
+		err = PTR_ERR(tegra->devfreq);
+		return err;
+	}
 
 	return 0;
 }
-- 
2.21.0

[PATCH v2 07/19] PM / devfreq: tegra: Register clk notifier in the end of driver's probe

[Dmitry Osipenko]

The notifier isn't unregistered if something fails after the registration,
move it to the end of probe to fix the potential use-after-free problem.

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

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index aa0478464b35..c248c18431d9 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -647,14 +647,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 		return PTR_ERR(tegra->emc_clock);
 	}
 
-	tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
-	err = clk_notifier_register(tegra->emc_clock, &tegra->rate_change_nb);
-	if (err) {
-		dev_err(&pdev->dev,
-			"Failed to register rate change notifier\n");
-		return err;
-	}
-
 	reset_control_assert(tegra->reset);
 
 	err = clk_prepare_enable(tegra->clock);
@@ -712,6 +704,14 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
+	err = clk_notifier_register(tegra->emc_clock, &tegra->rate_change_nb);
+	if (err) {
+		dev_err(&pdev->dev,
+			"Failed to register rate change notifier\n");
+		return err;
+	}
+
 	return 0;
 }
 
-- 
2.21.0

[PATCH v2 08/19] PM / devfreq: tegra: Remove OPP entries on driver removal

[Dmitry Osipenko]

This fixes "_opp_is_duplicate" warning messages on driver's module reload.

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

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index c248c18431d9..98ba27b1e2c4 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -701,7 +701,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 						 NULL);
 	if (IS_ERR(tegra->devfreq)) {
 		err = PTR_ERR(tegra->devfreq);
-		return err;
+		goto remove_opps;
 	}
 
 	tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
@@ -709,10 +709,15 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	if (err) {
 		dev_err(&pdev->dev,
 			"Failed to register rate change notifier\n");
-		return err;
+		goto remove_opps;
 	}
 
 	return 0;
+
+remove_opps:
+	dev_pm_opp_remove_all_dynamic(&pdev->dev);
+
+	return err;
 }
 
 static int tegra_devfreq_remove(struct platform_device *pdev)
@@ -722,6 +727,9 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
 	u32 val;
 	unsigned int i;
 
+	devm_devfreq_remove_device(&pdev->dev, tegra->devfreq);
+	dev_pm_opp_remove_all_dynamic(&pdev->dev);
+
 	for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
 		val = device_readl(&tegra->devices[i], ACTMON_DEV_CTRL);
 		val &= ~ACTMON_DEV_CTRL_ENB;
-- 
2.21.0

[PATCH v2 09/19] PM / devfreq: tegra: Change interrupt request order

[Dmitry Osipenko]

The interrupt should be requested after devfreq initialization because
interrupt handler uses the devfreq, otherwise interrupt handler will
crash if interrupt fires before the initialization. This shouldn't
happen in practice, but it's more correct to have interrupt available
after all of interrupt prerequisites are ready.

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

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 98ba27b1e2c4..2a1464098200 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -686,14 +686,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, tegra);
 
-	err = devm_request_threaded_irq(&pdev->dev, irq, actmon_isr,
-					actmon_thread_isr, IRQF_SHARED,
-					"tegra-devfreq", tegra);
-	if (err) {
-		dev_err(&pdev->dev, "Interrupt request failed\n");
-		return err;
-	}
-
 	tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
 	tegra->devfreq = devm_devfreq_add_device(&pdev->dev,
 						 &tegra_devfreq_profile,
@@ -701,6 +693,14 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 						 NULL);
 	if (IS_ERR(tegra->devfreq)) {
 		err = PTR_ERR(tegra->devfreq);
+		return err;
+	}
+
+	err = devm_request_threaded_irq(&pdev->dev, irq, actmon_isr,
+					actmon_thread_isr, IRQF_SHARED,
+					"tegra-devfreq", tegra);
+	if (err) {
+		dev_err(&pdev->dev, "Interrupt request failed\n");
 		goto remove_opps;
 	}
 
-- 
2.21.0

[PATCH v2 10/19] PM / devfreq: tegra: Drop primary interrupt handler

[Dmitry Osipenko]

There is no real need in the primary interrupt handler, hence move
everything to the secondary (threaded) handler. In a result locking
is consistent now and there are no potential races with the interrupt
handler because it is protected with the devfreq's mutex.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra-devfreq.c | 61 ++++++++++++---------------------
 1 file changed, 22 insertions(+), 39 deletions(-)

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 2a1464098200..69b557df5084 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -144,7 +144,6 @@ static struct tegra_devfreq_device_config actmon_device_configs[] = {
 struct tegra_devfreq_device {
 	const struct tegra_devfreq_device_config *config;
 	void __iomem *regs;
-	spinlock_t lock;
 
 	/* Average event count sampled in the last interrupt */
 	u32 avg_count;
@@ -249,11 +248,8 @@ static void actmon_write_barrier(struct tegra_devfreq *tegra)
 static void actmon_isr_device(struct tegra_devfreq *tegra,
 			      struct tegra_devfreq_device *dev)
 {
-	unsigned long flags;
 	u32 intr_status, dev_ctrl;
 
-	spin_lock_irqsave(&dev->lock, flags);
-
 	dev->avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
 	tegra_devfreq_update_avg_wmark(tegra, dev);
 
@@ -302,26 +298,6 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
 	device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
 
 	actmon_write_barrier(tegra);
-
-	spin_unlock_irqrestore(&dev->lock, flags);
-}
-
-static irqreturn_t actmon_isr(int irq, void *data)
-{
-	struct tegra_devfreq *tegra = data;
-	bool handled = false;
-	unsigned int i;
-	u32 val;
-
-	val = actmon_readl(tegra, ACTMON_GLB_STATUS);
-	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
-		if (val & tegra->devices[i].config->irq_mask) {
-			actmon_isr_device(tegra, tegra->devices + i);
-			handled = true;
-		}
-	}
-
-	return handled ? IRQ_WAKE_THREAD : IRQ_NONE;
 }
 
 static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra,
@@ -348,35 +324,46 @@ static void actmon_update_target(struct tegra_devfreq *tegra,
 	unsigned long cpu_freq = 0;
 	unsigned long static_cpu_emc_freq = 0;
 	unsigned int avg_sustain_coef;
-	unsigned long flags;
+	u32 avg_count;
 
 	if (dev->config->avg_dependency_threshold) {
 		cpu_freq = cpufreq_get(0);
 		static_cpu_emc_freq = actmon_cpu_to_emc_rate(tegra, cpu_freq);
 	}
 
-	spin_lock_irqsave(&dev->lock, flags);
-
-	dev->target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD;
+	avg_count = dev->avg_count;
+	dev->target_freq = 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;
 
-	if (dev->avg_count >= dev->config->avg_dependency_threshold)
+	if (avg_count >= dev->config->avg_dependency_threshold)
 		dev->target_freq = max(dev->target_freq, static_cpu_emc_freq);
-
-	spin_unlock_irqrestore(&dev->lock, flags);
 }
 
 static irqreturn_t actmon_thread_isr(int irq, void *data)
 {
 	struct tegra_devfreq *tegra = data;
+	bool handled = false;
+	unsigned int i;
+	u32 val;
 
 	mutex_lock(&tegra->devfreq->lock);
-	update_devfreq(tegra->devfreq);
+
+	val = actmon_readl(tegra, ACTMON_GLB_STATUS);
+	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
+		if (val & tegra->devices[i].config->irq_mask) {
+			actmon_isr_device(tegra, tegra->devices + i);
+			handled = true;
+		}
+	}
+
+	if (handled)
+		update_devfreq(tegra->devfreq);
+
 	mutex_unlock(&tegra->devfreq->lock);
 
-	return IRQ_HANDLED;
+	return handled ? IRQ_HANDLED : IRQ_NONE;
 }
 
 static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
@@ -386,7 +373,6 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
 	struct tegra_devfreq *tegra;
 	struct tegra_devfreq_device *dev;
 	unsigned int i;
-	unsigned long flags;
 
 	if (action != POST_RATE_CHANGE)
 		return NOTIFY_OK;
@@ -398,9 +384,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];
 
-		spin_lock_irqsave(&dev->lock, flags);
 		tegra_devfreq_update_wmark(tegra, dev);
-		spin_unlock_irqrestore(&dev->lock, flags);
 	}
 
 	actmon_write_barrier(tegra);
@@ -668,7 +652,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 		dev = tegra->devices + i;
 		dev->config = actmon_device_configs + i;
 		dev->regs = tegra->regs + dev->config->offset;
-		spin_lock_init(&dev->lock);
 
 		tegra_actmon_configure_device(tegra, dev);
 	}
@@ -696,8 +679,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	err = devm_request_threaded_irq(&pdev->dev, irq, actmon_isr,
-					actmon_thread_isr, IRQF_SHARED,
+	err = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+					actmon_thread_isr, IRQF_ONESHOT,
 					"tegra-devfreq", tegra);
 	if (err) {
 		dev_err(&pdev->dev, "Interrupt request failed\n");
-- 
2.21.0

[PATCH v2 11/19] PM / devfreq: tegra: De-initialize properly on driver's probe error

[Dmitry Osipenko]

Reset hardware, disable ACTMON clock, release OPP's and free IRQ before
removing devfreq device since there is no guarantee that interrupt
handling won't run after masking interrupt in hardware.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra-devfreq.c | 53 ++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 27 deletions(-)

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 69b557df5084..a668e4fbc874 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -663,28 +663,28 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
-		dev_err(&pdev->dev, "Failed to get IRQ: %d\n", irq);
-		return irq;
+		err = irq;
+		dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err);
+		goto remove_opps;
 	}
 
 	platform_set_drvdata(pdev, tegra);
 
 	tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
-	tegra->devfreq = devm_devfreq_add_device(&pdev->dev,
-						 &tegra_devfreq_profile,
-						 "tegra_actmon",
-						 NULL);
+	tegra->devfreq = devfreq_add_device(&pdev->dev,
+					    &tegra_devfreq_profile,
+					    "tegra_actmon",
+					    NULL);
 	if (IS_ERR(tegra->devfreq)) {
 		err = PTR_ERR(tegra->devfreq);
-		return err;
+		goto remove_opps;
 	}
 
-	err = devm_request_threaded_irq(&pdev->dev, irq, NULL,
-					actmon_thread_isr, IRQF_ONESHOT,
-					"tegra-devfreq", tegra);
+	err = request_threaded_irq(irq, NULL, actmon_thread_isr, IRQF_ONESHOT,
+				   "tegra-devfreq", tegra);
 	if (err) {
 		dev_err(&pdev->dev, "Interrupt request failed\n");
-		goto remove_opps;
+		goto remove_devfreq;
 	}
 
 	tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
@@ -692,14 +692,23 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	if (err) {
 		dev_err(&pdev->dev,
 			"Failed to register rate change notifier\n");
-		goto remove_opps;
+		goto disable_interrupt;
 	}
 
 	return 0;
 
+disable_interrupt:
+	free_irq(irq, tegra);
+
+remove_devfreq:
+	devfreq_remove_device(tegra->devfreq);
+
 remove_opps:
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);
 
+	reset_control_reset(tegra->reset);
+	clk_disable_unprepare(tegra->clock);
+
 	return err;
 }
 
@@ -707,24 +716,14 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
 {
 	struct tegra_devfreq *tegra = platform_get_drvdata(pdev);
 	int irq = platform_get_irq(pdev, 0);
-	u32 val;
-	unsigned int i;
-
-	devm_devfreq_remove_device(&pdev->dev, tegra->devfreq);
-	dev_pm_opp_remove_all_dynamic(&pdev->dev);
-
-	for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
-		val = device_readl(&tegra->devices[i], ACTMON_DEV_CTRL);
-		val &= ~ACTMON_DEV_CTRL_ENB;
-		device_writel(&tegra->devices[i], val, ACTMON_DEV_CTRL);
-	}
-
-	actmon_write_barrier(tegra);
-
-	devm_free_irq(&pdev->dev, irq, tegra);
 
 	clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb);
+	free_irq(irq, tegra);
+
+	devfreq_remove_device(tegra->devfreq);
+	dev_pm_opp_remove_all_dynamic(&pdev->dev);
 
+	reset_control_reset(tegra->reset);
 	clk_disable_unprepare(tegra->clock);
 
 	return 0;
-- 
2.21.0

[PATCH v2 12/19] PM / devfreq: tegra: Avoid inconsistency of current frequency value

[Dmitry Osipenko]

The frequency value potentially could change in-between. It doesn't
cause any real problem at all right now, but that could change in the
future. Hence let's avoid the inconsistency.

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

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index a668e4fbc874..f1a6f951813a 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -496,13 +496,15 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
 {
 	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
 	struct tegra_devfreq_device *actmon_dev;
+	unsigned long cur_freq;
 
-	stat->current_frequency = tegra->cur_freq * KHZ;
+	cur_freq = READ_ONCE(tegra->cur_freq);
 
 	/* 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;
 
 	actmon_dev = &tegra->devices[MCALL];
 
@@ -513,7 +515,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 * tegra->cur_freq;
+	stat->total_time = ACTMON_SAMPLING_PERIOD * cur_freq;
 
 	stat->busy_time = min(stat->busy_time, stat->total_time);
 
-- 
2.21.0

[PATCH v2 13/19] PM / devfreq: tegra: Mark ACTMON's governor as immutable

[Dmitry Osipenko]

The ACTMON's governor supports only the Tegra's devfreq device and there
is no need to use any other governor, hence let's mark Tegra governor as
immutable to permanently stick it with Tegra's devfreq device.

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

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 6a172d338f6d..a78dffe603c1 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -94,7 +94,6 @@ config ARM_EXYNOS_BUS_DEVFREQ
 config ARM_TEGRA_DEVFREQ
 	tristate "Tegra DEVFREQ Driver"
 	depends on ARCH_TEGRA_124_SOC
-	select DEVFREQ_GOV_SIMPLE_ONDEMAND
 	select PM_OPP
 	help
 	  This adds the DEVFREQ driver for the Tegra family of SoCs.
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index f1a6f951813a..832e4f5aa11b 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -593,6 +593,7 @@ static struct devfreq_governor tegra_devfreq_governor = {
 	.name = "tegra_actmon",
 	.get_target_freq = tegra_governor_get_target,
 	.event_handler = tegra_governor_event_handler,
+	.immutable = true,
 };
 
 static int tegra_devfreq_probe(struct platform_device *pdev)
-- 
2.21.0

[PATCH v2 14/19] PM / devfreq: tegra: Move governor registration to driver's probe

[Dmitry Osipenko]

There is no need to register the ACTMON's governor separately from
the driver, hence let's move the registration into the driver's probe
function for consistency and to make code cleaner a tad.

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

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 832e4f5aa11b..46c61af8ca33 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -673,6 +673,12 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, tegra);
 
+	err = devfreq_add_governor(&tegra_devfreq_governor);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to add governor: %d\n", err);
+		goto remove_opps;
+	}
+
 	tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
 	tegra->devfreq = devfreq_add_device(&pdev->dev,
 					    &tegra_devfreq_profile,
@@ -680,7 +686,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 					    NULL);
 	if (IS_ERR(tegra->devfreq)) {
 		err = PTR_ERR(tegra->devfreq);
-		goto remove_opps;
+		goto remove_governor;
 	}
 
 	err = request_threaded_irq(irq, NULL, actmon_thread_isr, IRQF_ONESHOT,
@@ -706,6 +712,9 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 remove_devfreq:
 	devfreq_remove_device(tegra->devfreq);
 
+remove_governor:
+	devfreq_remove_governor(&tegra_devfreq_governor);
+
 remove_opps:
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);
 
@@ -729,7 +738,7 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
 	reset_control_reset(tegra->reset);
 	clk_disable_unprepare(tegra->clock);
 
-	return 0;
+	return devfreq_remove_governor(&tegra_devfreq_governor);
 }
 
 static const struct of_device_id tegra_devfreq_of_match[] = {
@@ -747,36 +756,7 @@ static struct platform_driver tegra_devfreq_driver = {
 		.of_match_table = tegra_devfreq_of_match,
 	},
 };
-
-static int __init tegra_devfreq_init(void)
-{
-	int ret = 0;
-
-	ret = devfreq_add_governor(&tegra_devfreq_governor);
-	if (ret) {
-		pr_err("%s: failed to add governor: %d\n", __func__, ret);
-		return ret;
-	}
-
-	ret = platform_driver_register(&tegra_devfreq_driver);
-	if (ret)
-		devfreq_remove_governor(&tegra_devfreq_governor);
-
-	return ret;
-}
-module_init(tegra_devfreq_init)
-
-static void __exit tegra_devfreq_exit(void)
-{
-	int ret = 0;
-
-	platform_driver_unregister(&tegra_devfreq_driver);
-
-	ret = devfreq_remove_governor(&tegra_devfreq_governor);
-	if (ret)
-		pr_err("%s: failed to remove governor: %d\n", __func__, ret);
-}
-module_exit(tegra_devfreq_exit)
+module_platform_driver(tegra_devfreq_driver);
 
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("Tegra devfreq driver");
-- 
2.21.0

[PATCH v2 15/19] PM / devfreq: tegra: Synchronize IRQ after masking it in hardware

[Dmitry Osipenko]

There is no guarantee that interrupt handling isn't running in parallel,
hence it is necessary to synchronize IRQ in order to ensure that interrupt
is indeed disabled.

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

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 46c61af8ca33..cfbfbe28e7bf 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -171,6 +171,8 @@ struct tegra_devfreq {
 	struct notifier_block	rate_change_nb;
 
 	struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
+
+	int irq;
 };
 
 struct tegra_actmon_emc_ratio {
@@ -432,6 +434,8 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
 	}
 
 	actmon_write_barrier(tegra);
+
+	synchronize_irq(tegra->irq);
 }
 
 static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
@@ -603,7 +607,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	struct resource *res;
 	unsigned int i;
 	unsigned long rate;
-	int irq;
 	int err;
 
 	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
@@ -659,18 +662,18 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 		tegra_actmon_configure_device(tegra, dev);
 	}
 
+	tegra->irq = platform_get_irq(pdev, 0);
+	if (tegra->irq < 0) {
+		err = tegra->irq;
+		dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err);
+		return err;
+	}
+
 	for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
 		rate = clk_round_rate(tegra->emc_clock, rate);
 		dev_pm_opp_add(&pdev->dev, rate, 0);
 	}
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0) {
-		err = irq;
-		dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err);
-		goto remove_opps;
-	}
-
 	platform_set_drvdata(pdev, tegra);
 
 	err = devfreq_add_governor(&tegra_devfreq_governor);
@@ -689,10 +692,11 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 		goto remove_governor;
 	}
 
-	err = request_threaded_irq(irq, NULL, actmon_thread_isr, IRQF_ONESHOT,
-				   "tegra-devfreq", tegra);
+	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\n");
+		dev_err(&pdev->dev, "Interrupt request failed: %d\n", err);
 		goto remove_devfreq;
 	}
 
@@ -701,14 +705,11 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	if (err) {
 		dev_err(&pdev->dev,
 			"Failed to register rate change notifier\n");
-		goto disable_interrupt;
+		goto remove_devfreq;
 	}
 
 	return 0;
 
-disable_interrupt:
-	free_irq(irq, tegra);
-
 remove_devfreq:
 	devfreq_remove_device(tegra->devfreq);
 
@@ -727,10 +728,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 static int tegra_devfreq_remove(struct platform_device *pdev)
 {
 	struct tegra_devfreq *tegra = platform_get_drvdata(pdev);
-	int irq = platform_get_irq(pdev, 0);
 
 	clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb);
-	free_irq(irq, tegra);
 
 	devfreq_remove_device(tegra->devfreq);
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);
-- 
2.21.0

[PATCH v2 16/19] PM / devfreq: tegra: Reconfigure hardware on governor's restart

[Dmitry Osipenko]

Move hardware configuration to governor's start/resume methods.
This allows to re-initialize hardware counters and reconfigure
cleanly if governor was stopped/paused. That is needed because we
are not aware of all hardware changes that happened while governor
was stopped and the paused state may get out of sync with reality,
hence it's better to start with a clean slate after the pause. In
a result there is no memory bandwidth starvation after resume from
suspend-to-ram that results in display controller underflowing that
happens on resume because of improper decision made by devfreq about
the required memory frequency. This change also cleans up code a tad
by moving hardware-configuration code into a single location.

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

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index cfbfbe28e7bf..f0711d5ad27d 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -394,50 +394,6 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
-static void tegra_actmon_enable_interrupts(struct tegra_devfreq *tegra)
-{
-	struct tegra_devfreq_device *dev;
-	u32 val;
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
-		dev = &tegra->devices[i];
-
-		val = device_readl(dev, ACTMON_DEV_CTRL);
-		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;
-
-		device_writel(dev, val, ACTMON_DEV_CTRL);
-	}
-
-	actmon_write_barrier(tegra);
-}
-
-static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
-{
-	struct tegra_devfreq_device *dev;
-	u32 val;
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
-		dev = &tegra->devices[i];
-
-		val = device_readl(dev, ACTMON_DEV_CTRL);
-		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;
-
-		device_writel(dev, val, ACTMON_DEV_CTRL);
-	}
-
-	actmon_write_barrier(tegra);
-
-	synchronize_irq(tegra->irq);
-}
-
 static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
 					  struct tegra_devfreq_device *dev)
 {
@@ -461,13 +417,40 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
 		<< ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_NUM_SHIFT;
 	val |= (ACTMON_ABOVE_WMARK_WINDOW - 1)
 		<< 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;
 
 	device_writel(dev, val, ACTMON_DEV_CTRL);
+}
+
+static void tegra_actmon_start(struct tegra_devfreq *tegra)
+{
+	unsigned int i;
+
+	actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
+		      ACTMON_GLB_PERIOD_CTRL);
+
+	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
+		tegra_actmon_configure_device(tegra, &tegra->devices[i]);
 
 	actmon_write_barrier(tegra);
 }
 
+static void tegra_actmon_stop(struct tegra_devfreq *tegra)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
+		device_writel(&tegra->devices[i], 0x00000000, ACTMON_DEV_CTRL);
+
+	actmon_write_barrier(tegra);
+
+	synchronize_irq(tegra->irq);
+}
+
 static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
 				u32 flags)
 {
@@ -571,22 +554,22 @@ static int tegra_governor_event_handler(struct devfreq *devfreq,
 	switch (event) {
 	case DEVFREQ_GOV_START:
 		devfreq_monitor_start(devfreq);
-		tegra_actmon_enable_interrupts(tegra);
+		tegra_actmon_start(tegra);
 		break;
 
 	case DEVFREQ_GOV_STOP:
-		tegra_actmon_disable_interrupts(tegra);
+		tegra_actmon_stop(tegra);
 		devfreq_monitor_stop(devfreq);
 		break;
 
 	case DEVFREQ_GOV_SUSPEND:
-		tegra_actmon_disable_interrupts(tegra);
+		tegra_actmon_stop(tegra);
 		devfreq_monitor_suspend(devfreq);
 		break;
 
 	case DEVFREQ_GOV_RESUME:
 		devfreq_monitor_resume(devfreq);
-		tegra_actmon_enable_interrupts(tegra);
+		tegra_actmon_start(tegra);
 		break;
 	}
 
@@ -651,15 +634,10 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	tegra->max_freq = clk_round_rate(tegra->emc_clock, ULONG_MAX) / KHZ;
 	tegra->cur_freq = clk_get_rate(tegra->emc_clock) / KHZ;
 
-	actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
-		      ACTMON_GLB_PERIOD_CTRL);
-
 	for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
 		dev = tegra->devices + i;
 		dev->config = actmon_device_configs + i;
 		dev->regs = tegra->regs + dev->config->offset;
-
-		tegra_actmon_configure_device(tegra, dev);
 	}
 
 	tegra->irq = platform_get_irq(pdev, 0);
-- 
2.21.0

[PATCH v2 17/19] PM / devfreq: tegra: Support Tegra30

[Dmitry Osipenko]

The devfreq driver can be used on Tegra30 without any code change and
it works perfectly fine, the default Tegra124 parameters are good enough
for Tegra30.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/Kconfig         | 4 ++--
 drivers/devfreq/tegra-devfreq.c | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index a78dffe603c1..78c33ddd4eea 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -92,8 +92,8 @@ config ARM_EXYNOS_BUS_DEVFREQ
 	  This does not yet operate with optimal voltages.
 
 config ARM_TEGRA_DEVFREQ
-	tristate "Tegra DEVFREQ Driver"
-	depends on ARCH_TEGRA_124_SOC
+	tristate "NVIDIA Tegra30+ DEVFREQ Driver"
+	depends on ARCH_TEGRA
 	select PM_OPP
 	help
 	  This adds the DEVFREQ driver for the Tegra family of SoCs.
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index f0711d5ad27d..0a2465a58cf5 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -719,6 +719,7 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id tegra_devfreq_of_match[] = {
+	{ .compatible = "nvidia,tegra30-actmon" },
 	{ .compatible = "nvidia,tegra124-actmon" },
 	{ },
 };
-- 
2.21.0

[PATCH v2 18/19] PM / devfreq: tegra: Enable COMPILE_TEST for the driver

[Dmitry Osipenko]

The driver's compilation doesn't have any specific dependencies, hence
the COMPILE_TEST option can be supported in Kconfig.

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

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 78c33ddd4eea..bd6652863e7d 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
 
 config ARM_TEGRA_DEVFREQ
 	tristate "NVIDIA Tegra30+ DEVFREQ Driver"
-	depends on ARCH_TEGRA
+	depends on ARCH_TEGRA || COMPILE_TEST
 	select PM_OPP
 	help
 	  This adds the DEVFREQ driver for the Tegra family of SoCs.
-- 
2.21.0

[PATCH v2 19/19] PM / devfreq: Introduce driver for NVIDIA Tegra20

[Dmitry Osipenko]

Add devfreq driver for NVIDIA Tegra20 SoC's. The driver periodically
reads out Memory Controller counters and adjusts memory frequency based
on the memory clients activity.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 MAINTAINERS                       |   8 ++
 drivers/devfreq/Kconfig           |  10 ++
 drivers/devfreq/Makefile          |   1 +
 drivers/devfreq/tegra20-devfreq.c | 177 ++++++++++++++++++++++++++++++
 4 files changed, 196 insertions(+)
 create mode 100644 drivers/devfreq/tegra20-devfreq.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 80b59db1b6e4..91f475ec4545 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10056,6 +10056,14 @@ F:	include/linux/memblock.h
 F:	mm/memblock.c
 F:	Documentation/core-api/boot-time-mm.rst
 
+MEMORY FREQUENCY SCALING DRIVER FOR NVIDIA TEGRA20
+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
+
 MEMORY MANAGEMENT
 L:	linux-mm@kvack.org
 W:	http://www.linux-mm.org
diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index bd6652863e7d..af4c86c4e0f6 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -100,6 +100,16 @@ config ARM_TEGRA_DEVFREQ
 	  It reads ACTMON counters of memory controllers and adjusts the
 	  operating frequencies and voltages with OPP support.
 
+config ARM_TEGRA20_DEVFREQ
+	tristate "NVIDIA Tegra20 DEVFREQ Driver"
+	depends on (TEGRA_MC && TEGRA20_EMC) || COMPILE_TEST
+	select DEVFREQ_GOV_SIMPLE_ONDEMAND
+	select PM_OPP
+	help
+	  This adds the DEVFREQ driver for the Tegra20 family of SoCs.
+	  It reads Memory Controller counters and adjusts the operating
+	  frequencies and voltages with OPP support.
+
 config ARM_RK3399_DMC_DEVFREQ
 	tristate "ARM RK3399 DMC DEVFREQ Driver"
 	depends on ARCH_ROCKCHIP
diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
index 32b8d4d3f12c..6fcc5596b8b7 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
 obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
 obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
 obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
+obj-$(CONFIG_ARM_TEGRA20_DEVFREQ)	+= tegra20-devfreq.o
 
 # DEVFREQ Event Drivers
 obj-$(CONFIG_PM_DEVFREQ_EVENT)		+= event/
diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c
new file mode 100644
index 000000000000..18c9aad7a9d7
--- /dev/null
+++ b/drivers/devfreq/tegra20-devfreq.c
@@ -0,0 +1,177 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * NVIDIA Tegra20 devfreq driver
+ *
+ * Author: Dmitry Osipenko <digetx@gmail.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/devfreq.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_opp.h>
+#include <linux/slab.h>
+
+#include <soc/tegra/mc.h>
+
+#include "governor.h"
+
+#define MC_STAT_CONTROL				0x90
+#define MC_STAT_EMC_CLOCK_LIMIT			0xa0
+#define MC_STAT_EMC_CLOCKS			0xa4
+#define MC_STAT_EMC_CONTROL			0xa8
+#define MC_STAT_EMC_COUNT			0xb8
+
+#define EMC_GATHER_CLEAR			(1 << 8)
+#define EMC_GATHER_ENABLE			(3 << 8)
+
+struct tegra_devfreq {
+	struct devfreq *devfreq;
+	struct clk *clk;
+	void __iomem *regs;
+};
+
+static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
+				u32 flags)
+{
+	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
+	struct dev_pm_opp *opp;
+	unsigned long rate;
+	int err;
+
+	opp = devfreq_recommended_opp(dev, freq, flags);
+	if (IS_ERR(opp))
+		return PTR_ERR(opp);
+
+	rate = dev_pm_opp_get_freq(opp);
+	dev_pm_opp_put(opp);
+
+	err = clk_set_min_rate(tegra->clk, rate);
+	if (err)
+		return err;
+
+	err = clk_set_rate(tegra->clk, 0);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static int tegra_devfreq_get_dev_status(struct device *dev,
+					struct devfreq_dev_status *stat)
+{
+	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
+
+	stat->busy_time = readl_relaxed(tegra->regs + MC_STAT_EMC_COUNT);
+	stat->total_time = readl_relaxed(tegra->regs + MC_STAT_EMC_CLOCKS) / 8;
+	stat->current_frequency = clk_get_rate(tegra->clk);
+
+	writel_relaxed(EMC_GATHER_CLEAR, tegra->regs + MC_STAT_CONTROL);
+	writel_relaxed(EMC_GATHER_ENABLE, tegra->regs + MC_STAT_CONTROL);
+
+	return 0;
+}
+
+static struct devfreq_dev_profile tegra_devfreq_profile = {
+	.polling_ms	= 500,
+	.target		= tegra_devfreq_target,
+	.get_dev_status	= tegra_devfreq_get_dev_status,
+};
+
+static struct tegra_mc *tegra_get_memory_controller(void)
+{
+	struct platform_device *pdev;
+	struct device_node *np;
+	struct tegra_mc *mc;
+
+	np = of_find_compatible_node(NULL, NULL, "nvidia,tegra20-mc-gart");
+	if (!np)
+		return ERR_PTR(-ENOENT);
+
+	pdev = of_find_device_by_node(np);
+	of_node_put(np);
+	if (!pdev)
+		return ERR_PTR(-ENODEV);
+
+	mc = platform_get_drvdata(pdev);
+	if (!mc)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	return mc;
+}
+
+static int tegra_devfeq_probe(struct platform_device *pdev)
+{
+	struct tegra_devfreq *tegra;
+	struct tegra_mc *mc;
+	unsigned long max_rate;
+	unsigned long rate;
+	int err;
+
+	mc = tegra_get_memory_controller();
+	if (IS_ERR(mc)) {
+		err = PTR_ERR(mc);
+		dev_err(&pdev->dev, "failed to get mc: %d\n", err);
+		return err;
+	}
+
+	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
+	if (!tegra)
+		return -ENOMEM;
+
+	tegra->clk = devm_clk_get(&pdev->dev, "emc");
+	if (IS_ERR(tegra->clk)) {
+		err = PTR_ERR(tegra->clk);
+		dev_err(&pdev->dev, "failed to get emc clock: %d\n", err);
+		return err;
+	}
+
+	tegra->regs = mc->regs;
+
+	max_rate = clk_round_rate(tegra->clk, ULONG_MAX);
+
+	for (rate = 0; rate <= max_rate; rate++) {
+		rate = clk_round_rate(tegra->clk, rate);
+		dev_pm_opp_add(&pdev->dev, rate, 0);
+	}
+
+	writel_relaxed(0x00000000, tegra->regs + MC_STAT_CONTROL);
+	writel_relaxed(0x00000000, tegra->regs + MC_STAT_EMC_CONTROL);
+	writel_relaxed(0xffffffff, tegra->regs + MC_STAT_EMC_CLOCK_LIMIT);
+
+	platform_set_drvdata(pdev, tegra);
+
+	tegra->devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
+					    DEVFREQ_GOV_SIMPLE_ONDEMAND, NULL);
+	if (IS_ERR(tegra->devfreq))
+		return PTR_ERR(tegra->devfreq);
+
+	return 0;
+}
+
+static int tegra_devfreq_remove(struct platform_device *pdev)
+{
+	struct tegra_devfreq *tegra = platform_get_drvdata(pdev);
+
+	devfreq_remove_device(tegra->devfreq);
+	dev_pm_opp_remove_all_dynamic(&pdev->dev);
+
+	return 0;
+}
+
+static struct platform_driver tegra_devfeq_driver = {
+	.probe		= tegra_devfeq_probe,
+	.remove		= tegra_devfreq_remove,
+	.driver		= {
+		.name	= "tegra20-devfreq",
+	},
+};
+module_platform_driver(tegra_devfeq_driver);
+
+MODULE_ALIAS("platform:tegra20-devfreq");
+MODULE_AUTHOR("Dmitry Osipenko <digetx@gmail.com>");
+MODULE_DESCRIPTION("NVIDIA Tegra20 devfreq driver");
+MODULE_LICENSE("GPL v2");
-- 
2.21.0

Re: [PATCH v2 01/19] PM / devfreq: tegra: Fix kHz to Hz conversion

[Chanwoo Choi]

Hi,

I add one minor comment (KHZ -> hz).

On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
> The kHz to Hz is incorrectly converted in a few places in the code,
> this results in a wrong frequency being calculated because devfreq core
> uses OPP frequencies that are given in Hz to clamp the rate, while
> tegra-devfreq gives to the core value in kHz and then it also expects to
> receive value in kHz from the core. In a result memory freq is always set
> to a value which is close to ULONG_MAX because of the bug. Hence the EMC
> frequency is always capped to the maximum and the driver doesn't do
> anything useful. This patch was tested on Tegra30 and Tegra124 SoC's, EMC
> frequency scaling works properly now.
> 
> Cc: <stable@vger.kernel.org> # 4.14+
> Tested-by: Steev Klimaszewski <steev@kali.org>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra-devfreq.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index c89ba7b834ff..02905978abe1 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -486,9 +486,9 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>  {
>  	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>  	struct dev_pm_opp *opp;
> -	unsigned long rate = *freq * KHZ;
> +	unsigned long rate;
>  
> -	opp = devfreq_recommended_opp(dev, &rate, flags);
> +	opp = devfreq_recommended_opp(dev, freq, flags);
>  	if (IS_ERR(opp)) {
>  		dev_err(dev, "Failed to find opp for %lu KHz\n", *freq);

Need to change it as following:
- KHz -> Hz


>  		return PTR_ERR(opp);
> @@ -499,8 +499,6 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>  	clk_set_min_rate(tegra->emc_clock, rate);
>  	clk_set_rate(tegra->emc_clock, 0);
>  
> -	*freq = rate;
> -
>  	return 0;
>  }
>  
> @@ -510,7 +508,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>  	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>  	struct tegra_devfreq_device *actmon_dev;
>  
> -	stat->current_frequency = tegra->cur_freq;
> +	stat->current_frequency = tegra->cur_freq * KHZ;
>  
>  	/* To be used by the tegra governor */
>  	stat->private_data = tegra;
> @@ -565,7 +563,7 @@ static int tegra_governor_get_target(struct devfreq *devfreq,
>  		target_freq = max(target_freq, dev->target_freq);
>  	}
>  
> -	*freq = target_freq;
> +	*freq = target_freq * KHZ;
>  
>  	return 0;
>  }
> 

It looks good to me if modify it in accordance with my comment.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Regards,
Chanwoo Choi



-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 03/19] PM / devfreq: tegra: Don't ignore clk errors

[Chanwoo Choi]

Hi,

On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
> The clk_set_min_rate() could fail and in this case clk_set_rate() sets
> rate to 0, which may drop EMC rate to minimum and make machine very
> difficult to use.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra-devfreq.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index 0c0909fba545..69581c9082d4 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -487,6 +487,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>  	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>  	struct dev_pm_opp *opp;
>  	unsigned long rate;
> +	int err;
>  
>  	opp = devfreq_recommended_opp(dev, freq, flags);
>  	if (IS_ERR(opp)) {
> @@ -496,8 +497,13 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>  	rate = dev_pm_opp_get_freq(opp);
>  	dev_pm_opp_put(opp);
>  
> -	clk_set_min_rate(tegra->emc_clock, rate);
> -	clk_set_rate(tegra->emc_clock, 0);
> +	err = clk_set_min_rate(tegra->emc_clock, rate);
> +	if (err)
> +		return err;
> +
> +	err = clk_set_rate(tegra->emc_clock, 0);
> +	if (err)

Why don't you restore the previous minimum clock for tegra->emc_clock
when failed to execute 'clk_set_rate(tegra->emc_clock, 0);'?

I think that if you control the restoring the previous minimum clock,
it is more nice to handle the error handling.

> +		return err;
>  
>  	return 0;
>  }
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 04/19] PM / devfreq: tegra: Don't set EMC clock rate to maximum on probe

[Chanwoo Choi]

Hi,

On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
> There is no real benefit from doing so, hence let's drop that rate setting
> for consistency.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra-devfreq.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index 69581c9082d4..d62fb1b0d9bb 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -648,8 +648,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  		return PTR_ERR(tegra->emc_clock);
>  	}
>  
> -	clk_set_rate(tegra->emc_clock, ULONG_MAX);
> -

It seems like that initialize the emc_clock as the supported maximum clock.
But, if the rate for emc_clock initialized by either bootloader or clock driver
and it's well working until this code, actually, it is not necessary.

Looks good to me.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>


>  	tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
>  	err = clk_notifier_register(tegra->emc_clock, &tegra->rate_change_nb);
>  	if (err) {
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 06/19] PM / devfreq: tegra: Fix missed error checking on devfreq initialization failure

[Chanwoo Choi]

Hi,

patch6/7/8/9 are for handling of exception handling in probe() function.
Actually, I'm not sure that there are special reason to split out
the patches. I think that you can squash patch6/7/8/9 to only one patch.

Also, even if patch6/7/8/9 handle the exception handling in probe(),
the tegra_devfreq_probe() doesn't support the restoring sequence
when fail happen. I think that if you want to fix the fail case of probe(),
please add the restoring sequence about following function.
- clk_disable_unprepare()
- clk_notifier_unregister()
- dev_pm_opp_remove()


On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
> The code doesn't check for devfreq initialization failure, fix it.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra-devfreq.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index f0f0d78f6cbf..aa0478464b35 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -707,6 +707,10 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  						 &tegra_devfreq_profile,
>  						 "tegra_actmon",
>  						 NULL);
> +	if (IS_ERR(tegra->devfreq)) {
> +		err = PTR_ERR(tegra->devfreq);
> +		return err;
> +	}
>  
>  	return 0;
>  }
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 10/19] PM / devfreq: tegra: Drop primary interrupt handler

[Chanwoo Choi]

Hi,

It looks good to me to drop the primary interrupt handler
but I have some comments. Please check it.

On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
> There is no real need in the primary interrupt handler, hence move
> everything to the secondary (threaded) handler. In a result locking
> is consistent now and there are no potential races with the interrupt
> handler because it is protected with the devfreq's mutex.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra-devfreq.c | 61 ++++++++++++---------------------
>  1 file changed, 22 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index 2a1464098200..69b557df5084 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -144,7 +144,6 @@ static struct tegra_devfreq_device_config actmon_device_configs[] = {
>  struct tegra_devfreq_device {
>  	const struct tegra_devfreq_device_config *config;
>  	void __iomem *regs;
> -	spinlock_t lock;
>  
>  	/* Average event count sampled in the last interrupt */
>  	u32 avg_count;
> @@ -249,11 +248,8 @@ static void actmon_write_barrier(struct tegra_devfreq *tegra)
>  static void actmon_isr_device(struct tegra_devfreq *tegra,
>  			      struct tegra_devfreq_device *dev)
>  {
> -	unsigned long flags;
>  	u32 intr_status, dev_ctrl;
>  
> -	spin_lock_irqsave(&dev->lock, flags);
> -
>  	dev->avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
>  	tegra_devfreq_update_avg_wmark(tegra, dev);
>  
> @@ -302,26 +298,6 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>  	device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
>  
>  	actmon_write_barrier(tegra);
> -
> -	spin_unlock_irqrestore(&dev->lock, flags);
> -}
> -
> -static irqreturn_t actmon_isr(int irq, void *data)
> -{
> -	struct tegra_devfreq *tegra = data;
> -	bool handled = false;
> -	unsigned int i;
> -	u32 val;
> -
> -	val = actmon_readl(tegra, ACTMON_GLB_STATUS);
> -	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
> -		if (val & tegra->devices[i].config->irq_mask) {
> -			actmon_isr_device(tegra, tegra->devices + i);
> -			handled = true;
> -		}
> -	}
> -
> -	return handled ? IRQ_WAKE_THREAD : IRQ_NONE;
>  }
>  
>  static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra,
> @@ -348,35 +324,46 @@ static void actmon_update_target(struct tegra_devfreq *tegra,
>  	unsigned long cpu_freq = 0;
>  	unsigned long static_cpu_emc_freq = 0;
>  	unsigned int avg_sustain_coef;
> -	unsigned long flags;
> +	u32 avg_count;
>  
>  	if (dev->config->avg_dependency_threshold) {
>  		cpu_freq = cpufreq_get(0);
>  		static_cpu_emc_freq = actmon_cpu_to_emc_rate(tegra, cpu_freq);
>  	}
>  
> -	spin_lock_irqsave(&dev->lock, flags);
> -
> -	dev->target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD;
> +	avg_count = dev->avg_count;
> +	dev->target_freq = avg_count / ACTMON_SAMPLING_PERIOD;

Actually, this change is not related to this patch.
Please keep the original code.

>  	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;
>  
> -	if (dev->avg_count >= dev->config->avg_dependency_threshold)
> +	if (avg_count >= dev->config->avg_dependency_threshold)

ditto.

>  		dev->target_freq = max(dev->target_freq, static_cpu_emc_freq);
> -
> -	spin_unlock_irqrestore(&dev->lock, flags);
>  }
>  
>  static irqreturn_t actmon_thread_isr(int irq, void *data)
>  {
>  	struct tegra_devfreq *tegra = data;
> +	bool handled = false;
> +	unsigned int i;
> +	u32 val;
>  
>  	mutex_lock(&tegra->devfreq->lock);
> -	update_devfreq(tegra->devfreq);
> +
> +	val = actmon_readl(tegra, ACTMON_GLB_STATUS);
> +	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
> +		if (val & tegra->devices[i].config->irq_mask) {
> +			actmon_isr_device(tegra, tegra->devices + i);
> +			handled = true;
> +		}
> +	}
> +
> +	if (handled)
> +		update_devfreq(tegra->devfreq);
> +
>  	mutex_unlock(&tegra->devfreq->lock);
>  
> -	return IRQ_HANDLED;
> +	return handled ? IRQ_HANDLED : IRQ_NONE;
>  }
>  
>  static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
> @@ -386,7 +373,6 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
>  	struct tegra_devfreq *tegra;
>  	struct tegra_devfreq_device *dev;
>  	unsigned int i;
> -	unsigned long flags;
>  
>  	if (action != POST_RATE_CHANGE)
>  		return NOTIFY_OK;
> @@ -398,9 +384,7 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,

When I review this patch, I have a question
about why tegra_actmon_rate_notify_cb is needed.

tegra_actmon_rate_notify_cb() do something
when the clock rate of emc_clock is changed.
I think that 'emc_clock' is changed by this driver.
It means that the this driver can catch the change timing
of emc_clock rate without notifier. 

IMO, it is possible to call tegra_devfreq_update_wmark()
directly without clock notifier before calling
'clk_set_min_rate()/clk_set_rate()' in the tegra_devfreq_target().

With clock notifier, it cannot restore something for
tegra_devfreq_update_wmark(tegra, dev) when failed to
set the rate of emc_clk by 'clk_set_min_rate()/clk_set_rate()'.

>  	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
>  		dev = &tegra->devices[i];
>  
> -		spin_lock_irqsave(&dev->lock, flags);
>  		tegra_devfreq_update_wmark(tegra, dev);
> -		spin_unlock_irqrestore(&dev->lock, flags);
>  	}
>  
>  	actmon_write_barrier(tegra);
> @@ -668,7 +652,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  		dev = tegra->devices + i;
>  		dev->config = actmon_device_configs + i;
>  		dev->regs = tegra->regs + dev->config->offset;
> -		spin_lock_init(&dev->lock);
>  
>  		tegra_actmon_configure_device(tegra, dev);
>  	}
> @@ -696,8 +679,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> -	err = devm_request_threaded_irq(&pdev->dev, irq, actmon_isr,
> -					actmon_thread_isr, IRQF_SHARED,
> +	err = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +					actmon_thread_isr, IRQF_ONESHOT,
>  					"tegra-devfreq", tegra);
>  	if (err) {
>  		dev_err(&pdev->dev, "Interrupt request failed\n");
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 11/19] PM / devfreq: tegra: De-initialize properly on driver's probe error

[Chanwoo Choi]

Hi Dmitry,

I already replied against patch6 about the exception handling
of tegra_devfreq_probe(). This patchset split out the patch
related to error handling for probe(). I think that you can
squash the patches regarding of exception handling for probe()
to one patch instead of split out the multiple patches.

On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
> Reset hardware, disable ACTMON clock, release OPP's and free IRQ before
> removing devfreq device since there is no guarantee that interrupt
> handling won't run after masking interrupt in hardware.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra-devfreq.c | 53 ++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index 69b557df5084..a668e4fbc874 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -663,28 +663,28 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0) {
> -		dev_err(&pdev->dev, "Failed to get IRQ: %d\n", irq);
> -		return irq;
> +		err = irq;
> +		dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err);
> +		goto remove_opps;
>  	}
>  
>  	platform_set_drvdata(pdev, tegra);
>  
>  	tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
> -	tegra->devfreq = devm_devfreq_add_device(&pdev->dev,
> -						 &tegra_devfreq_profile,
> -						 "tegra_actmon",
> -						 NULL);
> +	tegra->devfreq = devfreq_add_device(&pdev->dev,
> +					    &tegra_devfreq_profile,
> +					    "tegra_actmon",
> +					    NULL);
>  	if (IS_ERR(tegra->devfreq)) {
>  		err = PTR_ERR(tegra->devfreq);
> -		return err;
> +		goto remove_opps;
>  	}
>  
> -	err = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> -					actmon_thread_isr, IRQF_ONESHOT,
> -					"tegra-devfreq", tegra);
> +	err = request_threaded_irq(irq, NULL, actmon_thread_isr, IRQF_ONESHOT,
> +				   "tegra-devfreq", tegra);
>  	if (err) {
>  		dev_err(&pdev->dev, "Interrupt request failed\n");
> -		goto remove_opps;
> +		goto remove_devfreq;
>  	}
>  
>  	tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb;
> @@ -692,14 +692,23 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  	if (err) {
>  		dev_err(&pdev->dev,
>  			"Failed to register rate change notifier\n");
> -		goto remove_opps;
> +		goto disable_interrupt;
>  	}
>  
>  	return 0;
>  
> +disable_interrupt:
> +	free_irq(irq, tegra);
> +
> +remove_devfreq:
> +	devfreq_remove_device(tegra->devfreq);
> +
>  remove_opps:
>  	dev_pm_opp_remove_all_dynamic(&pdev->dev);
>  
> +	reset_control_reset(tegra->reset);
> +	clk_disable_unprepare(tegra->clock);
> +
>  	return err;
>  }
>  
> @@ -707,24 +716,14 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
>  {
>  	struct tegra_devfreq *tegra = platform_get_drvdata(pdev);
>  	int irq = platform_get_irq(pdev, 0);
> -	u32 val;
> -	unsigned int i;
> -
> -	devm_devfreq_remove_device(&pdev->dev, tegra->devfreq);
> -	dev_pm_opp_remove_all_dynamic(&pdev->dev);
> -
> -	for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
> -		val = device_readl(&tegra->devices[i], ACTMON_DEV_CTRL);
> -		val &= ~ACTMON_DEV_CTRL_ENB;
> -		device_writel(&tegra->devices[i], val, ACTMON_DEV_CTRL);
> -	}
> -
> -	actmon_write_barrier(tegra);
> -
> -	devm_free_irq(&pdev->dev, irq, tegra);
>  
>  	clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb);
> +	free_irq(irq, tegra);
> +
> +	devfreq_remove_device(tegra->devfreq);
> +	dev_pm_opp_remove_all_dynamic(&pdev->dev);
>  
> +	reset_control_reset(tegra->reset);
>  	clk_disable_unprepare(tegra->clock);
>  
>  	return 0;
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 12/19] PM / devfreq: tegra: Avoid inconsistency of current frequency value

[Chanwoo Choi]

Hi,

On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
> The frequency value potentially could change in-between. It doesn't
> cause any real problem at all right now, but that could change in the
> future. Hence let's avoid the inconsistency.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra-devfreq.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index a668e4fbc874..f1a6f951813a 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -496,13 +496,15 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>  {
>  	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>  	struct tegra_devfreq_device *actmon_dev;
> +	unsigned long cur_freq;
>  
> -	stat->current_frequency = tegra->cur_freq * KHZ;
> +	cur_freq = READ_ONCE(tegra->cur_freq);
>  
>  	/* 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;
>  
>  	actmon_dev = &tegra->devices[MCALL];
>  
> @@ -513,7 +515,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 * tegra->cur_freq;
> +	stat->total_time = ACTMON_SAMPLING_PERIOD * cur_freq;
>  
>  	stat->busy_time = min(stat->busy_time, stat->total_time);
>  
> 

The read/write access of tegra->cur_freq is in the single routine
of update_devfreq() as following. I think that there are no any
potential problem about the inconsistency of tegra->cur_freq.

IMHO, if there are no any problem now, I'm not sure that we need
to apply this patch.

update_devfreq()
{
	devfreq->governor->get_target_freq()
		devfreq_update_stats(devfreq)
			tegra_devfreq_get_dev_status()
				stat->current_frequency = tegra->cur_freq * KHZ;

	devfreq_set_target()
		tegra_devfreq_target()
			clk_set_min_rate(emc_rate, )
				tegra_actmon_rate_notify_cb()
					tegra->cur_freq = data->new_rate / KHZ;
		
			clk_set_rate(emc_rate, )
				tegra_actmon_rate_notify_cb()
					tegra->cur_freq = data->new_rate / KHZ;
}


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 13/19] PM / devfreq: tegra: Mark ACTMON's governor as immutable

[Chanwoo Choi]

Hi,

On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
> The ACTMON's governor supports only the Tegra's devfreq device and there
> is no need to use any other governor, hence let's mark Tegra governor as
> immutable to permanently stick it with Tegra's devfreq device.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/Kconfig         | 1 -
>  drivers/devfreq/tegra-devfreq.c | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 6a172d338f6d..a78dffe603c1 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -94,7 +94,6 @@ config ARM_EXYNOS_BUS_DEVFREQ
>  config ARM_TEGRA_DEVFREQ
>  	tristate "Tegra DEVFREQ Driver"
>  	depends on ARCH_TEGRA_124_SOC
> -	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>  	select PM_OPP
>  	help
>  	  This adds the DEVFREQ driver for the Tegra family of SoCs.
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index f1a6f951813a..832e4f5aa11b 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -593,6 +593,7 @@ static struct devfreq_governor tegra_devfreq_governor = {
>  	.name = "tegra_actmon",
>  	.get_target_freq = tegra_governor_get_target,
>  	.event_handler = tegra_governor_event_handler,
> +	.immutable = true,
>  };
>  
>  static int tegra_devfreq_probe(struct platform_device *pdev)
> 

Looks good to me.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 14/19] PM / devfreq: tegra: Move governor registration to driver's probe

[Chanwoo Choi]

On 19. 4. 15. 오후 11:55, Dmitry Osipenko wrote:
> There is no need to register the ACTMON's governor separately from
> the driver, hence let's move the registration into the driver's probe
> function for consistency and to make code cleaner a tad.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra-devfreq.c | 44 +++++++++------------------------
>  1 file changed, 12 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index 832e4f5aa11b..46c61af8ca33 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -673,6 +673,12 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, tegra);
>  
> +	err = devfreq_add_governor(&tegra_devfreq_governor);
> +	if (err) {
> +		dev_err(&pdev->dev, "Failed to add governor: %d\n", err);
> +		goto remove_opps;
> +	}
> +
>  	tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
>  	tegra->devfreq = devfreq_add_device(&pdev->dev,
>  					    &tegra_devfreq_profile,
> @@ -680,7 +686,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  					    NULL);
>  	if (IS_ERR(tegra->devfreq)) {
>  		err = PTR_ERR(tegra->devfreq);
> -		goto remove_opps;
> +		goto remove_governor;
>  	}
>  
>  	err = request_threaded_irq(irq, NULL, actmon_thread_isr, IRQF_ONESHOT,
> @@ -706,6 +712,9 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  remove_devfreq:
>  	devfreq_remove_device(tegra->devfreq);
>  
> +remove_governor:
> +	devfreq_remove_governor(&tegra_devfreq_governor);
> +
>  remove_opps:
>  	dev_pm_opp_remove_all_dynamic(&pdev->dev);
>  
> @@ -729,7 +738,7 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
>  	reset_control_reset(tegra->reset);
>  	clk_disable_unprepare(tegra->clock);
>  
> -	return 0;
> +	return devfreq_remove_governor(&tegra_devfreq_governor);
>  }
>  
>  static const struct of_device_id tegra_devfreq_of_match[] = {
> @@ -747,36 +756,7 @@ static struct platform_driver tegra_devfreq_driver = {
>  		.of_match_table = tegra_devfreq_of_match,
>  	},
>  };
> -
> -static int __init tegra_devfreq_init(void)
> -{
> -	int ret = 0;
> -
> -	ret = devfreq_add_governor(&tegra_devfreq_governor);
> -	if (ret) {
> -		pr_err("%s: failed to add governor: %d\n", __func__, ret);
> -		return ret;
> -	}
> -
> -	ret = platform_driver_register(&tegra_devfreq_driver);
> -	if (ret)
> -		devfreq_remove_governor(&tegra_devfreq_governor);
> -
> -	return ret;
> -}
> -module_init(tegra_devfreq_init)
> -
> -static void __exit tegra_devfreq_exit(void)
> -{
> -	int ret = 0;
> -
> -	platform_driver_unregister(&tegra_devfreq_driver);
> -
> -	ret = devfreq_remove_governor(&tegra_devfreq_governor);
> -	if (ret)
> -		pr_err("%s: failed to remove governor: %d\n", __func__, ret);
> -}
> -module_exit(tegra_devfreq_exit)
> +module_platform_driver(tegra_devfreq_driver);
>  
>  MODULE_LICENSE("GPL v2");
>  MODULE_DESCRIPTION("Tegra devfreq driver");
> 

Looks good to me.
Reviewed-by : Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 15/19] PM / devfreq: tegra: Synchronize IRQ after masking it in hardware

[Chanwoo Choi]

Hi,

In this patchset,
the patch11 adds new 'disable_interrupt' goto statement
and then patch15 removes 'disable_interrupt' goto statement again.
Actually, it is inefficient.

If you change the order of patches,
you could remove this stuff.

On 19. 4. 15. 오후 11:55, Dmitry Osipenko wrote:
> There is no guarantee that interrupt handling isn't running in parallel,
> hence it is necessary to synchronize IRQ in order to ensure that interrupt
> is indeed disabled.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra-devfreq.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index 46c61af8ca33..cfbfbe28e7bf 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -171,6 +171,8 @@ struct tegra_devfreq {
>  	struct notifier_block	rate_change_nb;
>  
>  	struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
> +
> +	int irq;
>  };
>  
>  struct tegra_actmon_emc_ratio {
> @@ -432,6 +434,8 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
>  	}
>  
>  	actmon_write_barrier(tegra);
> +
> +	synchronize_irq(tegra->irq);
>  }
>  
>  static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
> @@ -603,7 +607,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	unsigned int i;
>  	unsigned long rate;
> -	int irq;
>  	int err;
>  
>  	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> @@ -659,18 +662,18 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  		tegra_actmon_configure_device(tegra, dev);
>  	}
>  
> +	tegra->irq = platform_get_irq(pdev, 0);
> +	if (tegra->irq < 0) {
> +		err = tegra->irq;
> +		dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err);
> +		return err;
> +	}
> +
>  	for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
>  		rate = clk_round_rate(tegra->emc_clock, rate);
>  		dev_pm_opp_add(&pdev->dev, rate, 0);
>  	}
>  
> -	irq = platform_get_irq(pdev, 0);
> -	if (irq < 0) {
> -		err = irq;
> -		dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err);
> -		goto remove_opps;
> -	}
> -
>  	platform_set_drvdata(pdev, tegra);
>  
>  	err = devfreq_add_governor(&tegra_devfreq_governor);
> @@ -689,10 +692,11 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  		goto remove_governor;
>  	}
>  
> -	err = request_threaded_irq(irq, NULL, actmon_thread_isr, IRQF_ONESHOT,
> -				   "tegra-devfreq", tegra);
> +	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\n");
> +		dev_err(&pdev->dev, "Interrupt request failed: %d\n", err);
>  		goto remove_devfreq;
>  	}
>  
> @@ -701,14 +705,11 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  	if (err) {
>  		dev_err(&pdev->dev,
>  			"Failed to register rate change notifier\n");
> -		goto disable_interrupt;
> +		goto remove_devfreq;
>  	}
>  
>  	return 0;
>  
> -disable_interrupt:
> -	free_irq(irq, tegra);
> -
>  remove_devfreq:
>  	devfreq_remove_device(tegra->devfreq);
>  
> @@ -727,10 +728,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  static int tegra_devfreq_remove(struct platform_device *pdev)
>  {
>  	struct tegra_devfreq *tegra = platform_get_drvdata(pdev);
> -	int irq = platform_get_irq(pdev, 0);
>  
>  	clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb);
> -	free_irq(irq, tegra);
>  
>  	devfreq_remove_device(tegra->devfreq);
>  	dev_pm_opp_remove_all_dynamic(&pdev->dev);
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 18/19] PM / devfreq: tegra: Enable COMPILE_TEST for the driver

[Chanwoo Choi]

On 19. 4. 15. 오후 11:55, Dmitry Osipenko wrote:
> The driver's compilation doesn't have any specific dependencies, hence
> the COMPILE_TEST option can be supported in Kconfig.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 78c33ddd4eea..bd6652863e7d 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
>  
>  config ARM_TEGRA_DEVFREQ
>  	tristate "NVIDIA Tegra30+ DEVFREQ Driver"
> -	depends on ARCH_TEGRA
> +	depends on ARCH_TEGRA || COMPILE_TEST
>  	select PM_OPP
>  	help
>  	  This adds the DEVFREQ driver for the Tegra family of SoCs.
> 

I think that it doesn't need to make it a separate patch.
You can merge this patch to patch17 and then drop this patch.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 17/19] PM / devfreq: tegra: Support Tegra30

[Chanwoo Choi]

Hi,

On 19. 4. 15. 오후 11:55, Dmitry Osipenko wrote:
> The devfreq driver can be used on Tegra30 without any code change and
> it works perfectly fine, the default Tegra124 parameters are good enough
> for Tegra30.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/Kconfig         | 4 ++--
>  drivers/devfreq/tegra-devfreq.c | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index a78dffe603c1..78c33ddd4eea 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -92,8 +92,8 @@ config ARM_EXYNOS_BUS_DEVFREQ
>  	  This does not yet operate with optimal voltages.
>  
>  config ARM_TEGRA_DEVFREQ
> -	tristate "Tegra DEVFREQ Driver"
> -	depends on ARCH_TEGRA_124_SOC
> +	tristate "NVIDIA Tegra30+ DEVFREQ Driver"

Looks good to me. But, I have a question.

'Tegra30+' expression is enough in order to indicate
the support for both tegra30 and tegra124?

In my case, it is difficult to catch the meaning
of which tegra30+ support the kind of tegra124.

> +	depends on ARCH_TEGRA
>  	select PM_OPP
>  	help
>  	  This adds the DEVFREQ driver for the Tegra family of SoCs.
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index f0711d5ad27d..0a2465a58cf5 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -719,6 +719,7 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id tegra_devfreq_of_match[] = {
> +	{ .compatible = "nvidia,tegra30-actmon" },
>  	{ .compatible = "nvidia,tegra124-actmon" },
>  	{ },
>  };
> 

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

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 05/19] PM / devfreq: tegra: Replace write memory barrier with the read barrier

[Chanwoo Choi]

Hi,

On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
> The write memory barrier isn't needed because the BUS buffer is flushed
> by read after write that happens after the removed wmb(), we will also
> use readl() instead of the relaxed version to ensure that read is indeed
> completed.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra-devfreq.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index d62fb1b0d9bb..f0f0d78f6cbf 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -243,8 +243,7 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
>  static void actmon_write_barrier(struct tegra_devfreq *tegra)
>  {
>  	/* ensure the update has reached the ACTMON */
> -	wmb();
> -	actmon_readl(tegra, ACTMON_GLB_STATUS);
> +	readl(tegra->regs + ACTMON_GLB_STATUS);

I think that this meaning of actmon_write_barrier() keeps
the order of 'store' assembly command without the execution change
from compiler optimization by using the wmb().

But, this patch edits it as following:
The result of the following two cases are same?

[original code]
	wmb()
	read_relaxed()

[new code by this patch]
	readl_relaxed()
	rmb()


>  }
>  
>  static void actmon_isr_device(struct tegra_devfreq *tegra,
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 19/19] PM / devfreq: Introduce driver for NVIDIA Tegra20

[Chanwoo Choi]

Hi,

On 19. 4. 15. 오후 11:55, Dmitry Osipenko wrote:
> Add devfreq driver for NVIDIA Tegra20 SoC's. The driver periodically
> reads out Memory Controller counters and adjusts memory frequency based
> on the memory clients activity.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  MAINTAINERS                       |   8 ++
>  drivers/devfreq/Kconfig           |  10 ++
>  drivers/devfreq/Makefile          |   1 +
>  drivers/devfreq/tegra20-devfreq.c | 177 ++++++++++++++++++++++++++++++
>  4 files changed, 196 insertions(+)
>  create mode 100644 drivers/devfreq/tegra20-devfreq.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 80b59db1b6e4..91f475ec4545 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10056,6 +10056,14 @@ F:	include/linux/memblock.h
>  F:	mm/memblock.c
>  F:	Documentation/core-api/boot-time-mm.rst
>  
> +MEMORY FREQUENCY SCALING DRIVER FOR NVIDIA TEGRA20
> +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
> +
>  MEMORY MANAGEMENT
>  L:	linux-mm@kvack.org
>  W:	http://www.linux-mm.org
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index bd6652863e7d..af4c86c4e0f6 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -100,6 +100,16 @@ config ARM_TEGRA_DEVFREQ
>  	  It reads ACTMON counters of memory controllers and adjusts the
>  	  operating frequencies and voltages with OPP support.
>  
> +config ARM_TEGRA20_DEVFREQ
> +	tristate "NVIDIA Tegra20 DEVFREQ Driver"
> +	depends on (TEGRA_MC && TEGRA20_EMC) || COMPILE_TEST
> +	select DEVFREQ_GOV_SIMPLE_ONDEMAND
> +	select PM_OPP
> +	help
> +	  This adds the DEVFREQ driver for the Tegra20 family of SoCs.
> +	  It reads Memory Controller counters and adjusts the operating
> +	  frequencies and voltages with OPP support.
> +
>  config ARM_RK3399_DMC_DEVFREQ
>  	tristate "ARM RK3399 DMC DEVFREQ Driver"
>  	depends on ARCH_ROCKCHIP
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 32b8d4d3f12c..6fcc5596b8b7 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
>  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
> +obj-$(CONFIG_ARM_TEGRA20_DEVFREQ)	+= tegra20-devfreq.o
>  
>  # DEVFREQ Event Drivers
>  obj-$(CONFIG_PM_DEVFREQ_EVENT)		+= event/
> diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c
> new file mode 100644
> index 000000000000..18c9aad7a9d7
> --- /dev/null
> +++ b/drivers/devfreq/tegra20-devfreq.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * NVIDIA Tegra20 devfreq driver
> + *
> + * Author: Dmitry Osipenko <digetx@gmail.com>
> + */

It doesn't any "Copyright (c) 2019 ..." sentence.

> +
> +#include <linux/clk.h>
> +#include <linux/devfreq.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/slab.h>
> +
> +#include <soc/tegra/mc.h>

I can find the '<soc/tegra/mc.h>' header file
on mainline branch. But mc.h is included in linux-next.git. 

If you don't share the patch related to mc.h,
the kernel build will be failed when apply it the devfreq.git
on final step. Actually, it should make the immutable branch
between two related maintainers in order to remove the build fail.

> +
> +#include "governor.h"
> +
> +#define MC_STAT_CONTROL				0x90
> +#define MC_STAT_EMC_CLOCK_LIMIT			0xa0
> +#define MC_STAT_EMC_CLOCKS			0xa4
> +#define MC_STAT_EMC_CONTROL			0xa8
> +#define MC_STAT_EMC_COUNT			0xb8
> +
> +#define EMC_GATHER_CLEAR			(1 << 8)
> +#define EMC_GATHER_ENABLE			(3 << 8)
> +
> +struct tegra_devfreq {
> +	struct devfreq *devfreq;
> +	struct clk *clk;
> +	void __iomem *regs;
> +};
> +
> +static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
> +				u32 flags)
> +{
> +	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
> +	struct dev_pm_opp *opp;
> +	unsigned long rate;
> +	int err;
> +
> +	opp = devfreq_recommended_opp(dev, freq, flags);
> +	if (IS_ERR(opp))
> +		return PTR_ERR(opp);
> +
> +	rate = dev_pm_opp_get_freq(opp);
> +	dev_pm_opp_put(opp);
> +
> +	err = clk_set_min_rate(tegra->clk, rate);
> +	if (err)
> +		return err;
> +
> +	err = clk_set_rate(tegra->clk, 0);
> +	if (err)

When fail happen, I think that you have to control
the restoring sequence for previous operation like clk_set_min_rate().

> +		return err;
> +
> +	return 0;
> +}
> +
> +static int tegra_devfreq_get_dev_status(struct device *dev,
> +					struct devfreq_dev_status *stat)
> +{
> +	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
> +
> +	stat->busy_time = readl_relaxed(tegra->regs + MC_STAT_EMC_COUNT);
> +	stat->total_time = readl_relaxed(tegra->regs + MC_STAT_EMC_CLOCKS) / 8;
> +	stat->current_frequency = clk_get_rate(tegra->clk);
> +
> +	writel_relaxed(EMC_GATHER_CLEAR, tegra->regs + MC_STAT_CONTROL);
> +	writel_relaxed(EMC_GATHER_ENABLE, tegra->regs + MC_STAT_CONTROL);
> +
> +	return 0;
> +}
> +
> +static struct devfreq_dev_profile tegra_devfreq_profile = {
> +	.polling_ms	= 500,
> +	.target		= tegra_devfreq_target,
> +	.get_dev_status	= tegra_devfreq_get_dev_status,
> +};
> +
> +static struct tegra_mc *tegra_get_memory_controller(void)
> +{
> +	struct platform_device *pdev;
> +	struct device_node *np;
> +	struct tegra_mc *mc;
> +
> +	np = of_find_compatible_node(NULL, NULL, "nvidia,tegra20-mc-gart");
> +	if (!np)
> +		return ERR_PTR(-ENOENT);
> +
> +	pdev = of_find_device_by_node(np);
> +	of_node_put(np);
> +	if (!pdev)
> +		return ERR_PTR(-ENODEV);
> +
> +	mc = platform_get_drvdata(pdev);
> +	if (!mc)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	return mc;
> +}
> +
> +static int tegra_devfeq_probe(struct platform_device *pdev)
> +{
> +	struct tegra_devfreq *tegra;
> +	struct tegra_mc *mc;
> +	unsigned long max_rate;
> +	unsigned long rate;
> +	int err;
> +
> +	mc = tegra_get_memory_controller();
> +	if (IS_ERR(mc)) {
> +		err = PTR_ERR(mc);
> +		dev_err(&pdev->dev, "failed to get mc: %d\n", err);

How about using 'memory controller' instead of 'mc'?
Because 'mc' is not standard expression.

> +		return err;
> +	}
> +
> +	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> +	if (!tegra)
> +		return -ENOMEM;
> +
> +	tegra->clk = devm_clk_get(&pdev->dev, "emc");
> +	if (IS_ERR(tegra->clk)) {
> +		err = PTR_ERR(tegra->clk);
> +		dev_err(&pdev->dev, "failed to get emc clock: %d\n", err);
> +		return err;
> +	}

Don't you need to enable the 'emc' clock'?
Because this patch doesn't enable this clock.

> +
> +	tegra->regs = mc->regs;
> +
> +	max_rate = clk_round_rate(tegra->clk, ULONG_MAX);
> +
> +	for (rate = 0; rate <= max_rate; rate++) {
> +		rate = clk_round_rate(tegra->clk, rate);
> +		dev_pm_opp_add(&pdev->dev, rate, 0);
> +	}
> +
> +	writel_relaxed(0x00000000, tegra->regs + MC_STAT_CONTROL);
> +	writel_relaxed(0x00000000, tegra->regs + MC_STAT_EMC_CONTROL);
> +	writel_relaxed(0xffffffff, tegra->regs + MC_STAT_EMC_CLOCK_LIMIT);

You better to add the comments of what are the meaning
of 0x00000000/0xffffffff. Without the detailed comments,
it is difficult to understand of meaning.

> +
> +	platform_set_drvdata(pdev, tegra);
> +
> +	tegra->devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
> +					    DEVFREQ_GOV_SIMPLE_ONDEMAND, NULL);
> +	if (IS_ERR(tegra->devfreq))
> +		return PTR_ERR(tegra->devfreq);
> +
> +	return 0;
> +}
> +
> +static int tegra_devfreq_remove(struct platform_device *pdev)
> +{
> +	struct tegra_devfreq *tegra = platform_get_drvdata(pdev);
> +
> +	devfreq_remove_device(tegra->devfreq);
> +	dev_pm_opp_remove_all_dynamic(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver tegra_devfeq_driver = {
> +	.probe		= tegra_devfeq_probe,
> +	.remove		= tegra_devfreq_remove,
> +	.driver		= {
> +		.name	= "tegra20-devfreq",

How can you bind this driver without compatible name for Devicetree?
And I tried to find the name ("tegra20-devfreq") in
the MFD drivers.

> +	},
> +};
> +module_platform_driver(tegra_devfeq_driver);
> +
> +MODULE_ALIAS("platform:tegra20-devfreq");
> +MODULE_AUTHOR("Dmitry Osipenko <digetx@gmail.com>");
> +MODULE_DESCRIPTION("NVIDIA Tegra20 devfreq driver");
> +MODULE_LICENSE("GPL v2");
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 01/19] PM / devfreq: tegra: Fix kHz to Hz conversion

[Dmitry Osipenko]

16.04.2019 4:45, Chanwoo Choi пишет:
> Hi,
> 
> I add one minor comment (KHZ -> hz).

Hello Chanwoo,

Thank you very much for the review!

> On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
>> The kHz to Hz is incorrectly converted in a few places in the code,
>> this results in a wrong frequency being calculated because devfreq core
>> uses OPP frequencies that are given in Hz to clamp the rate, while
>> tegra-devfreq gives to the core value in kHz and then it also expects to
>> receive value in kHz from the core. In a result memory freq is always set
>> to a value which is close to ULONG_MAX because of the bug. Hence the EMC
>> frequency is always capped to the maximum and the driver doesn't do
>> anything useful. This patch was tested on Tegra30 and Tegra124 SoC's, EMC
>> frequency scaling works properly now.
>>
>> Cc: <stable@vger.kernel.org> # 4.14+
>> Tested-by: Steev Klimaszewski <steev@kali.org>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/tegra-devfreq.c | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>> index c89ba7b834ff..02905978abe1 100644
>> --- a/drivers/devfreq/tegra-devfreq.c
>> +++ b/drivers/devfreq/tegra-devfreq.c
>> @@ -486,9 +486,9 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>>  {
>>  	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>>  	struct dev_pm_opp *opp;
>> -	unsigned long rate = *freq * KHZ;
>> +	unsigned long rate;
>>  
>> -	opp = devfreq_recommended_opp(dev, &rate, flags);
>> +	opp = devfreq_recommended_opp(dev, freq, flags);
>>  	if (IS_ERR(opp)) {
>>  		dev_err(dev, "Failed to find opp for %lu KHz\n", *freq);
> 
> Need to change it as following:
> - KHz -> Hz

Indeed, good catch.

> 
>>  		return PTR_ERR(opp);
>> @@ -499,8 +499,6 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>>  	clk_set_min_rate(tegra->emc_clock, rate);
>>  	clk_set_rate(tegra->emc_clock, 0);
>>  
>> -	*freq = rate;
>> -
>>  	return 0;
>>  }
>>  
>> @@ -510,7 +508,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>>  	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>>  	struct tegra_devfreq_device *actmon_dev;
>>  
>> -	stat->current_frequency = tegra->cur_freq;
>> +	stat->current_frequency = tegra->cur_freq * KHZ;
>>  
>>  	/* To be used by the tegra governor */
>>  	stat->private_data = tegra;
>> @@ -565,7 +563,7 @@ static int tegra_governor_get_target(struct devfreq *devfreq,
>>  		target_freq = max(target_freq, dev->target_freq);
>>  	}
>>  
>> -	*freq = target_freq;
>> +	*freq = target_freq * KHZ;
>>  
>>  	return 0;
>>  }
>>
> 
> It looks good to me if modify it in accordance with my comment.
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Okay, thanks.

Re: [PATCH v2 03/19] PM / devfreq: tegra: Don't ignore clk errors

[Dmitry Osipenko]

16.04.2019 4:52, Chanwoo Choi пишет:
> Hi,
> 
> On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
>> The clk_set_min_rate() could fail and in this case clk_set_rate() sets
>> rate to 0, which may drop EMC rate to minimum and make machine very
>> difficult to use.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/tegra-devfreq.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>> index 0c0909fba545..69581c9082d4 100644
>> --- a/drivers/devfreq/tegra-devfreq.c
>> +++ b/drivers/devfreq/tegra-devfreq.c
>> @@ -487,6 +487,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>>  	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>>  	struct dev_pm_opp *opp;
>>  	unsigned long rate;
>> +	int err;
>>  
>>  	opp = devfreq_recommended_opp(dev, freq, flags);
>>  	if (IS_ERR(opp)) {
>> @@ -496,8 +497,13 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>>  	rate = dev_pm_opp_get_freq(opp);
>>  	dev_pm_opp_put(opp);
>>  
>> -	clk_set_min_rate(tegra->emc_clock, rate);
>> -	clk_set_rate(tegra->emc_clock, 0);
>> +	err = clk_set_min_rate(tegra->emc_clock, rate);
>> +	if (err)
>> +		return err;
>> +
>> +	err = clk_set_rate(tegra->emc_clock, 0);
>> +	if (err)
> 
> Why don't you restore the previous minimum clock for tegra->emc_clock
> when failed to execute 'clk_set_rate(tegra->emc_clock, 0);'?
> 
> I think that if you control the restoring the previous minimum clock,
> it is more nice to handle the error handling.

I now see that devfreq has the previous_freq field and indeed it is possible to restore the min_rate value using it. Thanks for the suggestion.

Re: [PATCH v2 04/19] PM / devfreq: tegra: Don't set EMC clock rate to maximum on probe

[Dmitry Osipenko]

16.04.2019 4:59, Chanwoo Choi пишет:
> Hi,
> 
> On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
>> There is no real benefit from doing so, hence let's drop that rate setting
>> for consistency.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/tegra-devfreq.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>> index 69581c9082d4..d62fb1b0d9bb 100644
>> --- a/drivers/devfreq/tegra-devfreq.c
>> +++ b/drivers/devfreq/tegra-devfreq.c
>> @@ -648,8 +648,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>  		return PTR_ERR(tegra->emc_clock);
>>  	}
>>  
>> -	clk_set_rate(tegra->emc_clock, ULONG_MAX);
>> -
> 
> It seems like that initialize the emc_clock as the supported maximum clock.
> But, if the rate for emc_clock initialized by either bootloader or clock driver
> and it's well working until this code, actually, it is not necessary.
> 
> Looks good to me.
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

EMC clock rate is set properly at a boot time on all Tegra's, thanks.

Re: [PATCH v2 05/19] PM / devfreq: tegra: Replace write memory barrier with the read barrier

[Dmitry Osipenko]

16.04.2019 11:00, Chanwoo Choi пишет:
> Hi,
> 
> On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
>> The write memory barrier isn't needed because the BUS buffer is flushed
>> by read after write that happens after the removed wmb(), we will also
>> use readl() instead of the relaxed version to ensure that read is indeed
>> completed.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/tegra-devfreq.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>> index d62fb1b0d9bb..f0f0d78f6cbf 100644
>> --- a/drivers/devfreq/tegra-devfreq.c
>> +++ b/drivers/devfreq/tegra-devfreq.c
>> @@ -243,8 +243,7 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
>>  static void actmon_write_barrier(struct tegra_devfreq *tegra)
>>  {
>>  	/* ensure the update has reached the ACTMON */
>> -	wmb();
>> -	actmon_readl(tegra, ACTMON_GLB_STATUS);
>> +	readl(tegra->regs + ACTMON_GLB_STATUS);
> 
> I think that this meaning of actmon_write_barrier() keeps
> the order of 'store' assembly command without the execution change
> from compiler optimization by using the wmb().

The IO mapped memory is strongly-ordered on ARM, hence all readl/writel accesses are guaranteed to be ordered by default. I think wmb() here is just a cut-n-pasted relic from old downstream driver.

> But, this patch edits it as following:
> The result of the following two cases are same?
> 
> [original code]
> 	wmb()
> 	read_relaxed()
> 
> [new code by this patch]
> 	readl_relaxed()
> 	rmb()

Yes, the result is the same. The wmb() is not just about IO accesses, but about all kind of memory accesses and at least on Tegra30 it is quite expensive operation because it translates into L2XO cache syncing (arm_heavy_mb) + dsb(). It should be more efficient to flush out writes with a read-back and then wait for that read operation to be completed.

Re: [PATCH v2 06/19] PM / devfreq: tegra: Fix missed error checking on devfreq initialization failure

[Dmitry Osipenko]

16.04.2019 5:32, Chanwoo Choi пишет:
> Hi,
> 
> patch6/7/8/9 are for handling of exception handling in probe() function.
> Actually, I'm not sure that there are special reason to split out
> the patches. I think that you can squash patch6/7/8/9 to only one patch.

Indeed, I was rebasing and reordering patches multiple times and looks like there is no reason not to squash these patches now.

> Also, even if patch6/7/8/9 handle the exception handling in probe(),
> the tegra_devfreq_probe() doesn't support the restoring sequence
> when fail happen. I think that if you want to fix the fail case of probe(),
> please add the restoring sequence about following function.
> - clk_disable_unprepare()
> - clk_notifier_unregister()
> - dev_pm_opp_remove()

When all of 6/7/8/9 patches are applied, the clk_notifier_register() becomes the last invocation of the probe function and clk_enable() is kept at the first place of the probe order. Hence the sequence you're suggesting is already incorrect because error-unwinding order usually should be opposite to the probe order. It looks to me that the current final result of these patches is already correct.

Re: [PATCH v2 10/19] PM / devfreq: tegra: Drop primary interrupt handler

[Dmitry Osipenko]

16.04.2019 8:56, Chanwoo Choi пишет:
> Hi,
> 
> It looks good to me to drop the primary interrupt handler
> but I have some comments. Please check it.
> 
> On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
>> There is no real need in the primary interrupt handler, hence move
>> everything to the secondary (threaded) handler. In a result locking
>> is consistent now and there are no potential races with the interrupt
>> handler because it is protected with the devfreq's mutex.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/tegra-devfreq.c | 61 ++++++++++++---------------------
>>  1 file changed, 22 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>> index 2a1464098200..69b557df5084 100644
>> --- a/drivers/devfreq/tegra-devfreq.c
>> +++ b/drivers/devfreq/tegra-devfreq.c
>> @@ -144,7 +144,6 @@ static struct tegra_devfreq_device_config actmon_device_configs[] = {
>>  struct tegra_devfreq_device {
>>  	const struct tegra_devfreq_device_config *config;
>>  	void __iomem *regs;
>> -	spinlock_t lock;
>>  
>>  	/* Average event count sampled in the last interrupt */
>>  	u32 avg_count;
>> @@ -249,11 +248,8 @@ static void actmon_write_barrier(struct tegra_devfreq *tegra)
>>  static void actmon_isr_device(struct tegra_devfreq *tegra,
>>  			      struct tegra_devfreq_device *dev)
>>  {
>> -	unsigned long flags;
>>  	u32 intr_status, dev_ctrl;
>>  
>> -	spin_lock_irqsave(&dev->lock, flags);
>> -
>>  	dev->avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
>>  	tegra_devfreq_update_avg_wmark(tegra, dev);
>>  
>> @@ -302,26 +298,6 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>>  	device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
>>  
>>  	actmon_write_barrier(tegra);
>> -
>> -	spin_unlock_irqrestore(&dev->lock, flags);
>> -}
>> -
>> -static irqreturn_t actmon_isr(int irq, void *data)
>> -{
>> -	struct tegra_devfreq *tegra = data;
>> -	bool handled = false;
>> -	unsigned int i;
>> -	u32 val;
>> -
>> -	val = actmon_readl(tegra, ACTMON_GLB_STATUS);
>> -	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
>> -		if (val & tegra->devices[i].config->irq_mask) {
>> -			actmon_isr_device(tegra, tegra->devices + i);
>> -			handled = true;
>> -		}
>> -	}
>> -
>> -	return handled ? IRQ_WAKE_THREAD : IRQ_NONE;
>>  }
>>  
>>  static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra,
>> @@ -348,35 +324,46 @@ static void actmon_update_target(struct tegra_devfreq *tegra,
>>  	unsigned long cpu_freq = 0;
>>  	unsigned long static_cpu_emc_freq = 0;
>>  	unsigned int avg_sustain_coef;
>> -	unsigned long flags;
>> +	u32 avg_count;
>>  
>>  	if (dev->config->avg_dependency_threshold) {
>>  		cpu_freq = cpufreq_get(0);
>>  		static_cpu_emc_freq = actmon_cpu_to_emc_rate(tegra, cpu_freq);
>>  	}
>>  
>> -	spin_lock_irqsave(&dev->lock, flags);
>> -
>> -	dev->target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD;
>> +	avg_count = dev->avg_count;
>> +	dev->target_freq = avg_count / ACTMON_SAMPLING_PERIOD;
> 
> Actually, this change is not related to this patch.
> Please keep the original code.
> 
>>  	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;
>>  
>> -	if (dev->avg_count >= dev->config->avg_dependency_threshold)
>> +	if (avg_count >= dev->config->avg_dependency_threshold)
> 
> ditto.

Good catch, it's a leftover from v1 that I forgot to revert. Thank you.

[snip]

> 
> When I review this patch, I have a question
> about why tegra_actmon_rate_notify_cb is needed.
> 
> tegra_actmon_rate_notify_cb() do something
> when the clock rate of emc_clock is changed.
> I think that 'emc_clock' is changed by this driver.
> It means that the this driver can catch the change timing
> of emc_clock rate without notifier. 

The devfreq driver isn't the only driver of the EMC clock rate. The devfreq driver changes EMC freq dynamically based of on average memory usage activity, but for some hardware units (like display controller for example) there is a requirement for a minimum memory bandwidth (isochronous transactions) and hence when display is waking up from suspend it immediately requires an amount of memory bandwidth that could be higher than ACTMON hardware unit suggests and besides the ACTMON's reaction is delayed by the sampling period (12ms).

> IMO, it is possible to call tegra_devfreq_update_wmark()
> directly without clock notifier before calling
> 'clk_set_min_rate()/clk_set_rate()' in the tegra_devfreq_target().
> 
> With clock notifier, it cannot restore something for
> tegra_devfreq_update_wmark(tegra, dev) when failed to
> set the rate of emc_clk by 'clk_set_min_rate()/clk_set_rate()'.

The watermarks should be changed in accordance to the actual EMC clock rate. Given that EMC rate could be changed by something else than the devfreq driver, we need to re-adjust the watermarks to the actual values after the EMC rate-change completion. Please note that the clock notifier uses the POST_RATE_CHANGE event that happens only after successful completion of EMC clock rate change.

Re: [PATCH v2 12/19] PM / devfreq: tegra: Avoid inconsistency of current frequency value

[Dmitry Osipenko]

16.04.2019 10:15, Chanwoo Choi пишет:
> Hi,
> 
> On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
>> The frequency value potentially could change in-between. It doesn't
>> cause any real problem at all right now, but that could change in the
>> future. Hence let's avoid the inconsistency.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/tegra-devfreq.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>> index a668e4fbc874..f1a6f951813a 100644
>> --- a/drivers/devfreq/tegra-devfreq.c
>> +++ b/drivers/devfreq/tegra-devfreq.c
>> @@ -496,13 +496,15 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>>  {
>>  	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>>  	struct tegra_devfreq_device *actmon_dev;
>> +	unsigned long cur_freq;
>>  
>> -	stat->current_frequency = tegra->cur_freq * KHZ;
>> +	cur_freq = READ_ONCE(tegra->cur_freq);
>>  
>>  	/* 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;
>>  
>>  	actmon_dev = &tegra->devices[MCALL];
>>  
>> @@ -513,7 +515,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 * tegra->cur_freq;
>> +	stat->total_time = ACTMON_SAMPLING_PERIOD * cur_freq;
>>  
>>  	stat->busy_time = min(stat->busy_time, stat->total_time);
>>  
>>
> 
> The read/write access of tegra->cur_freq is in the single routine
> of update_devfreq() as following. I think that there are no any
> potential problem about the inconsistency of tegra->cur_freq.

No, that's wrong assumption. The tegra->cur_freq is changed by the clock notifier that runs asynchronously with the devfreq driver when EMC clock rate is changed by something else in the kernel. 

> IMHO, if there are no any problem now, I'm not sure that we need
> to apply this patch.
> 
> update_devfreq()
> {
> 	devfreq->governor->get_target_freq()
> 		devfreq_update_stats(devfreq)
> 			tegra_devfreq_get_dev_status()
> 				stat->current_frequency = tegra->cur_freq * KHZ;
> 
> 	devfreq_set_target()
> 		tegra_devfreq_target()
> 			clk_set_min_rate(emc_rate, )
> 				tegra_actmon_rate_notify_cb()
> 					tegra->cur_freq = data->new_rate / KHZ;
> 		
> 			clk_set_rate(emc_rate, )
> 				tegra_actmon_rate_notify_cb()
> 					tegra->cur_freq = data->new_rate / KHZ;
> }
> 
> 

The cur_freq value is changed by the clock notifier that runs asynchronously with the rest of the devfreq driver. Hence potentially compiler may generate two separate fetches of the cur_freq value, then the clock rate could be changed by other CPU core simultaneously with tegra_devfreq_get_dev_status() or kernel may re-schedule preemptively, changing the clock rate in-between of the two fetches.

Re: [PATCH v2 15/19] PM / devfreq: tegra: Synchronize IRQ after masking it in hardware

[Dmitry Osipenko]

16.04.2019 10:41, Chanwoo Choi пишет:
> Hi,
> 
> In this patchset,
> the patch11 adds new 'disable_interrupt' goto statement
> and then patch15 removes 'disable_interrupt' goto statement again.
> Actually, it is inefficient.
> 
> If you change the order of patches,
> you could remove this stuff.

Okay, I'll change that in v3. Thanks.

Re: [PATCH v2 17/19] PM / devfreq: tegra: Support Tegra30

[Dmitry Osipenko]

16.04.2019 10:48, Chanwoo Choi пишет:
> Hi,
> 
> On 19. 4. 15. 오후 11:55, Dmitry Osipenko wrote:
>> The devfreq driver can be used on Tegra30 without any code change and
>> it works perfectly fine, the default Tegra124 parameters are good enough
>> for Tegra30.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/Kconfig         | 4 ++--
>>  drivers/devfreq/tegra-devfreq.c | 1 +
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index a78dffe603c1..78c33ddd4eea 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -92,8 +92,8 @@ config ARM_EXYNOS_BUS_DEVFREQ
>>  	  This does not yet operate with optimal voltages.
>>  
>>  config ARM_TEGRA_DEVFREQ
>> -	tristate "Tegra DEVFREQ Driver"
>> -	depends on ARCH_TEGRA_124_SOC
>> +	tristate "NVIDIA Tegra30+ DEVFREQ Driver"
> 
> Looks good to me. But, I have a question.
> 
> 'Tegra30+' expression is enough in order to indicate
> the support for both tegra30 and tegra124?
> 
> In my case, it is difficult to catch the meaning
> of which tegra30+ support the kind of tegra124.

The ACTMON serves Tegra30, Tegra124 and Tegra210 SoC's. The later Tegra generations also have the ACTMON hardware unit, but most of actual drivers have been moved into firmware on the later gens, including the ACTMON driver. I'll change the wording to "Tegra30/124/210" for clarity. Note that Tegra210 support isn't implemented at the moment by the driver, but that may change in the future.

>> +	depends on ARCH_TEGRA
>>  	select PM_OPP
>>  	help
>>  	  This adds the DEVFREQ driver for the Tegra family of SoCs.
>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>> index f0711d5ad27d..0a2465a58cf5 100644
>> --- a/drivers/devfreq/tegra-devfreq.c
>> +++ b/drivers/devfreq/tegra-devfreq.c
>> @@ -719,6 +719,7 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
>>  }
>>  
>>  static const struct of_device_id tegra_devfreq_of_match[] = {
>> +	{ .compatible = "nvidia,tegra30-actmon" },
>>  	{ .compatible = "nvidia,tegra124-actmon" },
>>  	{ },
>>  };
>>
> 
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> 

Thanks.

Re: [PATCH v2 18/19] PM / devfreq: tegra: Enable COMPILE_TEST for the driver

[Dmitry Osipenko]

16.04.2019 10:43, Chanwoo Choi пишет:
> On 19. 4. 15. 오후 11:55, Dmitry Osipenko wrote:
>> The driver's compilation doesn't have any specific dependencies, hence
>> the COMPILE_TEST option can be supported in Kconfig.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/Kconfig | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index 78c33ddd4eea..bd6652863e7d 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
>>  
>>  config ARM_TEGRA_DEVFREQ
>>  	tristate "NVIDIA Tegra30+ DEVFREQ Driver"
>> -	depends on ARCH_TEGRA
>> +	depends on ARCH_TEGRA || COMPILE_TEST
>>  	select PM_OPP
>>  	help
>>  	  This adds the DEVFREQ driver for the Tegra family of SoCs.
>>
> 
> I think that it doesn't need to make it a separate patch.
> You can merge this patch to patch17 and then drop this patch.
> 

I'd prefer to have this change as a separate patch because this is a logically distinct change from the patch 17.

Re: [PATCH v2 19/19] PM / devfreq: Introduce driver for NVIDIA Tegra20

[Dmitry Osipenko]

16.04.2019 11:31, Chanwoo Choi пишет:
> Hi,
> 
> On 19. 4. 15. 오후 11:55, Dmitry Osipenko wrote:
>> Add devfreq driver for NVIDIA Tegra20 SoC's. The driver periodically
>> reads out Memory Controller counters and adjusts memory frequency based
>> on the memory clients activity.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  MAINTAINERS                       |   8 ++
>>  drivers/devfreq/Kconfig           |  10 ++
>>  drivers/devfreq/Makefile          |   1 +
>>  drivers/devfreq/tegra20-devfreq.c | 177 ++++++++++++++++++++++++++++++
>>  4 files changed, 196 insertions(+)
>>  create mode 100644 drivers/devfreq/tegra20-devfreq.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 80b59db1b6e4..91f475ec4545 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -10056,6 +10056,14 @@ F:	include/linux/memblock.h
>>  F:	mm/memblock.c
>>  F:	Documentation/core-api/boot-time-mm.rst
>>  
>> +MEMORY FREQUENCY SCALING DRIVER FOR NVIDIA TEGRA20
>> +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
>> +
>>  MEMORY MANAGEMENT
>>  L:	linux-mm@kvack.org
>>  W:	http://www.linux-mm.org
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index bd6652863e7d..af4c86c4e0f6 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -100,6 +100,16 @@ config ARM_TEGRA_DEVFREQ
>>  	  It reads ACTMON counters of memory controllers and adjusts the
>>  	  operating frequencies and voltages with OPP support.
>>  
>> +config ARM_TEGRA20_DEVFREQ
>> +	tristate "NVIDIA Tegra20 DEVFREQ Driver"
>> +	depends on (TEGRA_MC && TEGRA20_EMC) || COMPILE_TEST
>> +	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>> +	select PM_OPP
>> +	help
>> +	  This adds the DEVFREQ driver for the Tegra20 family of SoCs.
>> +	  It reads Memory Controller counters and adjusts the operating
>> +	  frequencies and voltages with OPP support.
>> +
>>  config ARM_RK3399_DMC_DEVFREQ
>>  	tristate "ARM RK3399 DMC DEVFREQ Driver"
>>  	depends on ARCH_ROCKCHIP
>> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
>> index 32b8d4d3f12c..6fcc5596b8b7 100644
>> --- a/drivers/devfreq/Makefile
>> +++ b/drivers/devfreq/Makefile
>> @@ -11,6 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
>>  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
>>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
>>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
>> +obj-$(CONFIG_ARM_TEGRA20_DEVFREQ)	+= tegra20-devfreq.o
>>  
>>  # DEVFREQ Event Drivers
>>  obj-$(CONFIG_PM_DEVFREQ_EVENT)		+= event/
>> diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c
>> new file mode 100644
>> index 000000000000..18c9aad7a9d7
>> --- /dev/null
>> +++ b/drivers/devfreq/tegra20-devfreq.c
>> @@ -0,0 +1,177 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * NVIDIA Tegra20 devfreq driver
>> + *
>> + * Author: Dmitry Osipenko <digetx@gmail.com>
>> + */
> 
> It doesn't any "Copyright (c) 2019 ..." sentence.

I'll add one in v3.

>> +
>> +#include <linux/clk.h>
>> +#include <linux/devfreq.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_opp.h>
>> +#include <linux/slab.h>
>> +
>> +#include <soc/tegra/mc.h>
> 
> I can find the '<soc/tegra/mc.h>' header file
> on mainline branch. But mc.h is included in linux-next.git. 
> 
> If you don't share the patch related to mc.h,
> the kernel build will be failed when apply it the devfreq.git
> on final step. Actually, it should make the immutable branch
> between two related maintainers in order to remove the build fail.

The '<soc/tegra/mc.h>' header file exists since v3.18 [0], seems you just missed something.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/soc/tegra/mc.h?h=v3.18&id=8918465163171322c77a19d5258a95f56d89d2e4

>> +
>> +#include "governor.h"
>> +
>> +#define MC_STAT_CONTROL				0x90
>> +#define MC_STAT_EMC_CLOCK_LIMIT			0xa0
>> +#define MC_STAT_EMC_CLOCKS			0xa4
>> +#define MC_STAT_EMC_CONTROL			0xa8
>> +#define MC_STAT_EMC_COUNT			0xb8
>> +
>> +#define EMC_GATHER_CLEAR			(1 << 8)
>> +#define EMC_GATHER_ENABLE			(3 << 8)
>> +
>> +struct tegra_devfreq {
>> +	struct devfreq *devfreq;
>> +	struct clk *clk;
>> +	void __iomem *regs;
>> +};
>> +
>> +static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>> +				u32 flags)
>> +{
>> +	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>> +	struct dev_pm_opp *opp;
>> +	unsigned long rate;
>> +	int err;
>> +
>> +	opp = devfreq_recommended_opp(dev, freq, flags);
>> +	if (IS_ERR(opp))
>> +		return PTR_ERR(opp);
>> +
>> +	rate = dev_pm_opp_get_freq(opp);
>> +	dev_pm_opp_put(opp);
>> +
>> +	err = clk_set_min_rate(tegra->clk, rate);
>> +	if (err)
>> +		return err;
>> +
>> +	err = clk_set_rate(tegra->clk, 0);
>> +	if (err)
> 
> When fail happen, I think that you have to control
> the restoring sequence for previous operation like clk_set_min_rate().

Okay, thanks.

>> +		return err;
>> +
>> +	return 0;
>> +}
>> +
>> +static int tegra_devfreq_get_dev_status(struct device *dev,
>> +					struct devfreq_dev_status *stat)
>> +{
>> +	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>> +
>> +	stat->busy_time = readl_relaxed(tegra->regs + MC_STAT_EMC_COUNT);
>> +	stat->total_time = readl_relaxed(tegra->regs + MC_STAT_EMC_CLOCKS) / 8;
>> +	stat->current_frequency = clk_get_rate(tegra->clk);
>> +
>> +	writel_relaxed(EMC_GATHER_CLEAR, tegra->regs + MC_STAT_CONTROL);
>> +	writel_relaxed(EMC_GATHER_ENABLE, tegra->regs + MC_STAT_CONTROL);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct devfreq_dev_profile tegra_devfreq_profile = {
>> +	.polling_ms	= 500,
>> +	.target		= tegra_devfreq_target,
>> +	.get_dev_status	= tegra_devfreq_get_dev_status,
>> +};
>> +
>> +static struct tegra_mc *tegra_get_memory_controller(void)
>> +{
>> +	struct platform_device *pdev;
>> +	struct device_node *np;
>> +	struct tegra_mc *mc;
>> +
>> +	np = of_find_compatible_node(NULL, NULL, "nvidia,tegra20-mc-gart");
>> +	if (!np)
>> +		return ERR_PTR(-ENOENT);
>> +
>> +	pdev = of_find_device_by_node(np);
>> +	of_node_put(np);
>> +	if (!pdev)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	mc = platform_get_drvdata(pdev);
>> +	if (!mc)
>> +		return ERR_PTR(-EPROBE_DEFER);
>> +
>> +	return mc;
>> +}
>> +
>> +static int tegra_devfeq_probe(struct platform_device *pdev)
>> +{
>> +	struct tegra_devfreq *tegra;
>> +	struct tegra_mc *mc;
>> +	unsigned long max_rate;
>> +	unsigned long rate;
>> +	int err;
>> +
>> +	mc = tegra_get_memory_controller();
>> +	if (IS_ERR(mc)) {
>> +		err = PTR_ERR(mc);
>> +		dev_err(&pdev->dev, "failed to get mc: %d\n", err);
> 
> How about using 'memory controller' instead of 'mc'?
> Because 'mc' is not standard expression.

Sounds good, thanks.

>> +		return err;
>> +	}
>> +
>> +	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
>> +	if (!tegra)
>> +		return -ENOMEM;
>> +
>> +	tegra->clk = devm_clk_get(&pdev->dev, "emc");
>> +	if (IS_ERR(tegra->clk)) {
>> +		err = PTR_ERR(tegra->clk);
>> +		dev_err(&pdev->dev, "failed to get emc clock: %d\n", err);
>> +		return err;
>> +	}
> 
> Don't you need to enable the 'emc' clock'?
> Because this patch doesn't enable this clock.

EMC is a system critical clock (marked as critical by the clock driver), it is guaranteed to be always enabled. I think it is fine to omit clock-enabling in this case.

>> +
>> +	tegra->regs = mc->regs;
>> +
>> +	max_rate = clk_round_rate(tegra->clk, ULONG_MAX);
>> +
>> +	for (rate = 0; rate <= max_rate; rate++) {
>> +		rate = clk_round_rate(tegra->clk, rate);
>> +		dev_pm_opp_add(&pdev->dev, rate, 0);
>> +	}
>> +
>> +	writel_relaxed(0x00000000, tegra->regs + MC_STAT_CONTROL);
>> +	writel_relaxed(0x00000000, tegra->regs + MC_STAT_EMC_CONTROL);
>> +	writel_relaxed(0xffffffff, tegra->regs + MC_STAT_EMC_CLOCK_LIMIT);
> 
> You better to add the comments of what are the meaning
> of 0x00000000/0xffffffff. Without the detailed comments,
> it is difficult to understand of meaning.

Okay. Although the complete registers description is available in public and someone really curious could just google for the full details. 

>> +
>> +	platform_set_drvdata(pdev, tegra);
>> +
>> +	tegra->devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
>> +					    DEVFREQ_GOV_SIMPLE_ONDEMAND, NULL);
>> +	if (IS_ERR(tegra->devfreq))
>> +		return PTR_ERR(tegra->devfreq);
>> +
>> +	return 0;
>> +}
>> +
>> +static int tegra_devfreq_remove(struct platform_device *pdev)
>> +{
>> +	struct tegra_devfreq *tegra = platform_get_drvdata(pdev);
>> +
>> +	devfreq_remove_device(tegra->devfreq);
>> +	dev_pm_opp_remove_all_dynamic(&pdev->dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver tegra_devfeq_driver = {
>> +	.probe		= tegra_devfeq_probe,
>> +	.remove		= tegra_devfreq_remove,
>> +	.driver		= {
>> +		.name	= "tegra20-devfreq",
> 
> How can you bind this driver without compatible name for Devicetree?
> And I tried to find the name ("tegra20-devfreq") in
> the MFD drivers.

As I wrote in the cover-letter, the device for the driver will be created by the EMC driver. I'll send out the patch that creates the device later on because of EMC driver patch-series interdependence, for now I have that patch here [1] if you're curious how it looks like.

[1] https://github.com/grate-driver/linux/commit/c34440402bb4fb365647bf43166e85562c124273

Re: [PATCH v2 19/19] PM / devfreq: Introduce driver for NVIDIA Tegra20

[Chanwoo Choi]

Hi,

On 19. 4. 17. 오전 1:11, Dmitry Osipenko wrote:
> 16.04.2019 11:31, Chanwoo Choi пишет:
>> Hi,
>>
>> On 19. 4. 15. 오후 11:55, Dmitry Osipenko wrote:
>>> Add devfreq driver for NVIDIA Tegra20 SoC's. The driver periodically
>>> reads out Memory Controller counters and adjusts memory frequency based
>>> on the memory clients activity.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  MAINTAINERS                       |   8 ++
>>>  drivers/devfreq/Kconfig           |  10 ++
>>>  drivers/devfreq/Makefile          |   1 +
>>>  drivers/devfreq/tegra20-devfreq.c | 177 ++++++++++++++++++++++++++++++
>>>  4 files changed, 196 insertions(+)
>>>  create mode 100644 drivers/devfreq/tegra20-devfreq.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 80b59db1b6e4..91f475ec4545 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -10056,6 +10056,14 @@ F:	include/linux/memblock.h
>>>  F:	mm/memblock.c
>>>  F:	Documentation/core-api/boot-time-mm.rst
>>>  
>>> +MEMORY FREQUENCY SCALING DRIVER FOR NVIDIA TEGRA20
>>> +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
>>> +
>>>  MEMORY MANAGEMENT
>>>  L:	linux-mm@kvack.org
>>>  W:	http://www.linux-mm.org
>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>> index bd6652863e7d..af4c86c4e0f6 100644
>>> --- a/drivers/devfreq/Kconfig
>>> +++ b/drivers/devfreq/Kconfig
>>> @@ -100,6 +100,16 @@ config ARM_TEGRA_DEVFREQ
>>>  	  It reads ACTMON counters of memory controllers and adjusts the
>>>  	  operating frequencies and voltages with OPP support.
>>>  
>>> +config ARM_TEGRA20_DEVFREQ
>>> +	tristate "NVIDIA Tegra20 DEVFREQ Driver"
>>> +	depends on (TEGRA_MC && TEGRA20_EMC) || COMPILE_TEST
>>> +	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>>> +	select PM_OPP
>>> +	help
>>> +	  This adds the DEVFREQ driver for the Tegra20 family of SoCs.
>>> +	  It reads Memory Controller counters and adjusts the operating
>>> +	  frequencies and voltages with OPP support.
>>> +
>>>  config ARM_RK3399_DMC_DEVFREQ
>>>  	tristate "ARM RK3399 DMC DEVFREQ Driver"
>>>  	depends on ARCH_ROCKCHIP
>>> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
>>> index 32b8d4d3f12c..6fcc5596b8b7 100644
>>> --- a/drivers/devfreq/Makefile
>>> +++ b/drivers/devfreq/Makefile
>>> @@ -11,6 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
>>>  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
>>>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
>>>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
>>> +obj-$(CONFIG_ARM_TEGRA20_DEVFREQ)	+= tegra20-devfreq.o
>>>  
>>>  # DEVFREQ Event Drivers
>>>  obj-$(CONFIG_PM_DEVFREQ_EVENT)		+= event/
>>> diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c
>>> new file mode 100644
>>> index 000000000000..18c9aad7a9d7
>>> --- /dev/null
>>> +++ b/drivers/devfreq/tegra20-devfreq.c
>>> @@ -0,0 +1,177 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * NVIDIA Tegra20 devfreq driver
>>> + *
>>> + * Author: Dmitry Osipenko <digetx@gmail.com>
>>> + */
>>
>> It doesn't any "Copyright (c) 2019 ..." sentence.
> 
> I'll add one in v3.
> 
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/devfreq.h>
>>> +#include <linux/io.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/pm_opp.h>
>>> +#include <linux/slab.h>
>>> +
>>> +#include <soc/tegra/mc.h>
>>
>> I can find the '<soc/tegra/mc.h>' header file
>> on mainline branch. But mc.h is included in linux-next.git. 
>>
>> If you don't share the patch related to mc.h,
>> the kernel build will be failed when apply it the devfreq.git
>> on final step. Actually, it should make the immutable branch
>> between two related maintainers in order to remove the build fail.
> 
> The '<soc/tegra/mc.h>' header file exists since v3.18 [0], seems you just missed something.
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/soc/tegra/mc.h?h=v3.18&id=8918465163171322c77a19d5258a95f56d89d2e4

Sorry. It is my missing point. When I tried to find it,
it is included in the mainline kernel.

> 
>>> +
>>> +#include "governor.h"
>>> +
>>> +#define MC_STAT_CONTROL				0x90
>>> +#define MC_STAT_EMC_CLOCK_LIMIT			0xa0
>>> +#define MC_STAT_EMC_CLOCKS			0xa4
>>> +#define MC_STAT_EMC_CONTROL			0xa8
>>> +#define MC_STAT_EMC_COUNT			0xb8
>>> +
>>> +#define EMC_GATHER_CLEAR			(1 << 8)
>>> +#define EMC_GATHER_ENABLE			(3 << 8)
>>> +
>>> +struct tegra_devfreq {
>>> +	struct devfreq *devfreq;
>>> +	struct clk *clk;
>>> +	void __iomem *regs;
>>> +};
>>> +
>>> +static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>>> +				u32 flags)
>>> +{
>>> +	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>>> +	struct dev_pm_opp *opp;
>>> +	unsigned long rate;
>>> +	int err;
>>> +
>>> +	opp = devfreq_recommended_opp(dev, freq, flags);
>>> +	if (IS_ERR(opp))
>>> +		return PTR_ERR(opp);
>>> +
>>> +	rate = dev_pm_opp_get_freq(opp);
>>> +	dev_pm_opp_put(opp);
>>> +
>>> +	err = clk_set_min_rate(tegra->clk, rate);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	err = clk_set_rate(tegra->clk, 0);
>>> +	if (err)
>>
>> When fail happen, I think that you have to control
>> the restoring sequence for previous operation like clk_set_min_rate().
> 
> Okay, thanks.
> 
>>> +		return err;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int tegra_devfreq_get_dev_status(struct device *dev,
>>> +					struct devfreq_dev_status *stat)
>>> +{
>>> +	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>>> +
>>> +	stat->busy_time = readl_relaxed(tegra->regs + MC_STAT_EMC_COUNT);
>>> +	stat->total_time = readl_relaxed(tegra->regs + MC_STAT_EMC_CLOCKS) / 8;
>>> +	stat->current_frequency = clk_get_rate(tegra->clk);
>>> +
>>> +	writel_relaxed(EMC_GATHER_CLEAR, tegra->regs + MC_STAT_CONTROL);
>>> +	writel_relaxed(EMC_GATHER_ENABLE, tegra->regs + MC_STAT_CONTROL);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct devfreq_dev_profile tegra_devfreq_profile = {
>>> +	.polling_ms	= 500,
>>> +	.target		= tegra_devfreq_target,
>>> +	.get_dev_status	= tegra_devfreq_get_dev_status,
>>> +};
>>> +
>>> +static struct tegra_mc *tegra_get_memory_controller(void)
>>> +{
>>> +	struct platform_device *pdev;
>>> +	struct device_node *np;
>>> +	struct tegra_mc *mc;
>>> +
>>> +	np = of_find_compatible_node(NULL, NULL, "nvidia,tegra20-mc-gart");
>>> +	if (!np)
>>> +		return ERR_PTR(-ENOENT);
>>> +
>>> +	pdev = of_find_device_by_node(np);
>>> +	of_node_put(np);
>>> +	if (!pdev)
>>> +		return ERR_PTR(-ENODEV);
>>> +
>>> +	mc = platform_get_drvdata(pdev);
>>> +	if (!mc)
>>> +		return ERR_PTR(-EPROBE_DEFER);
>>> +
>>> +	return mc;
>>> +}
>>> +
>>> +static int tegra_devfeq_probe(struct platform_device *pdev)
>>> +{
>>> +	struct tegra_devfreq *tegra;
>>> +	struct tegra_mc *mc;
>>> +	unsigned long max_rate;
>>> +	unsigned long rate;
>>> +	int err;
>>> +
>>> +	mc = tegra_get_memory_controller();
>>> +	if (IS_ERR(mc)) {
>>> +		err = PTR_ERR(mc);
>>> +		dev_err(&pdev->dev, "failed to get mc: %d\n", err);
>>
>> How about using 'memory controller' instead of 'mc'?
>> Because 'mc' is not standard expression.
> 
> Sounds good, thanks.
> 
>>> +		return err;
>>> +	}
>>> +
>>> +	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
>>> +	if (!tegra)
>>> +		return -ENOMEM;
>>> +
>>> +	tegra->clk = devm_clk_get(&pdev->dev, "emc");
>>> +	if (IS_ERR(tegra->clk)) {
>>> +		err = PTR_ERR(tegra->clk);
>>> +		dev_err(&pdev->dev, "failed to get emc clock: %d\n", err);
>>> +		return err;
>>> +	}
>>
>> Don't you need to enable the 'emc' clock'?
>> Because this patch doesn't enable this clock.
> 
> EMC is a system critical clock (marked as critical by the clock driver), it is guaranteed to be always enabled. I think it is fine to omit clock-enabling in this case.

If you think don't need to enable it due to the critical clock,
instead, please add the comment about that emc clock is critical.
In my case, it looks like the strange use-case because this driver
doesn't contain the any enable code for clock.

> 
>>> +
>>> +	tegra->regs = mc->regs;
>>> +
>>> +	max_rate = clk_round_rate(tegra->clk, ULONG_MAX);
>>> +
>>> +	for (rate = 0; rate <= max_rate; rate++) {
>>> +		rate = clk_round_rate(tegra->clk, rate);
>>> +		dev_pm_opp_add(&pdev->dev, rate, 0);
>>> +	}
>>> +
>>> +	writel_relaxed(0x00000000, tegra->regs + MC_STAT_CONTROL);
>>> +	writel_relaxed(0x00000000, tegra->regs + MC_STAT_EMC_CONTROL);
>>> +	writel_relaxed(0xffffffff, tegra->regs + MC_STAT_EMC_CLOCK_LIMIT);
>>
>> You better to add the comments of what are the meaning
>> of 0x00000000/0xffffffff. Without the detailed comments,
>> it is difficult to understand of meaning.
> 
> Okay. Although the complete registers description is available in public and someone really curious could just google for the full details. 
> 
>>> +
>>> +	platform_set_drvdata(pdev, tegra);
>>> +
>>> +	tegra->devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
>>> +					    DEVFREQ_GOV_SIMPLE_ONDEMAND, NULL);
>>> +	if (IS_ERR(tegra->devfreq))
>>> +		return PTR_ERR(tegra->devfreq);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int tegra_devfreq_remove(struct platform_device *pdev)
>>> +{
>>> +	struct tegra_devfreq *tegra = platform_get_drvdata(pdev);
>>> +
>>> +	devfreq_remove_device(tegra->devfreq);
>>> +	dev_pm_opp_remove_all_dynamic(&pdev->dev);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct platform_driver tegra_devfeq_driver = {
>>> +	.probe		= tegra_devfeq_probe,
>>> +	.remove		= tegra_devfreq_remove,
>>> +	.driver		= {
>>> +		.name	= "tegra20-devfreq",
>>
>> How can you bind this driver without compatible name for Devicetree?
>> And I tried to find the name ("tegra20-devfreq") in
>> the MFD drivers.
> 
> As I wrote in the cover-letter, the device for the driver will be created by the EMC driver. I'll 
send out the patch that creates the device later on because of EMC driver patch-series interdependence, for now I have that patch here [1] if you're curious how it looks like.

OK. I understand. Thanks.

> 
> [1] https://github.com/grate-driver/linux/commit/c34440402bb4fb365647bf43166e85562c124273
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 18/19] PM / devfreq: tegra: Enable COMPILE_TEST for the driver

[Chanwoo Choi]

On 19. 4. 17. 오전 12:52, Dmitry Osipenko wrote:
> 16.04.2019 10:43, Chanwoo Choi пишет:
>> On 19. 4. 15. 오후 11:55, Dmitry Osipenko wrote:
>>> The driver's compilation doesn't have any specific dependencies, hence
>>> the COMPILE_TEST option can be supported in Kconfig.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  drivers/devfreq/Kconfig | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>> index 78c33ddd4eea..bd6652863e7d 100644
>>> --- a/drivers/devfreq/Kconfig
>>> +++ b/drivers/devfreq/Kconfig
>>> @@ -93,7 +93,7 @@ config ARM_EXYNOS_BUS_DEVFREQ
>>>  
>>>  config ARM_TEGRA_DEVFREQ
>>>  	tristate "NVIDIA Tegra30+ DEVFREQ Driver"
>>> -	depends on ARCH_TEGRA
>>> +	depends on ARCH_TEGRA || COMPILE_TEST
>>>  	select PM_OPP
>>>  	help
>>>  	  This adds the DEVFREQ driver for the Tegra family of SoCs.
>>>
>>
>> I think that it doesn't need to make it a separate patch.
>> You can merge this patch to patch17 and then drop this patch.
>>
> 
> I'd prefer to have this change as a separate patch because this is a logically distinct change from the patch 17.
> 
> 

OK.

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

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 17/19] PM / devfreq: tegra: Support Tegra30

[Chanwoo Choi]

On 19. 4. 17. 오전 12:49, Dmitry Osipenko wrote:
> 16.04.2019 10:48, Chanwoo Choi пишет:
>> Hi,
>>
>> On 19. 4. 15. 오후 11:55, Dmitry Osipenko wrote:
>>> The devfreq driver can be used on Tegra30 without any code change and
>>> it works perfectly fine, the default Tegra124 parameters are good enough
>>> for Tegra30.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  drivers/devfreq/Kconfig         | 4 ++--
>>>  drivers/devfreq/tegra-devfreq.c | 1 +
>>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>> index a78dffe603c1..78c33ddd4eea 100644
>>> --- a/drivers/devfreq/Kconfig
>>> +++ b/drivers/devfreq/Kconfig
>>> @@ -92,8 +92,8 @@ config ARM_EXYNOS_BUS_DEVFREQ
>>>  	  This does not yet operate with optimal voltages.
>>>  
>>>  config ARM_TEGRA_DEVFREQ
>>> -	tristate "Tegra DEVFREQ Driver"
>>> -	depends on ARCH_TEGRA_124_SOC
>>> +	tristate "NVIDIA Tegra30+ DEVFREQ Driver"
>>
>> Looks good to me. But, I have a question.
>>
>> 'Tegra30+' expression is enough in order to indicate
>> the support for both tegra30 and tegra124?
>>
>> In my case, it is difficult to catch the meaning
>> of which tegra30+ support the kind of tegra124.
> 
> The ACTMON serves Tegra30, Tegra124 and Tegra210 SoC's. The later Tegra generations also have the ACTMON hardware unit, but most of actual drivers have been moved into firmware on the later gens, including the ACTMON driver. I'll change the wording to "Tegra30/124/210" for clarity. Note that Tegra210 support isn't implemented at the moment by the driver, but that may change in the future.

OK. Thanks.

> 
>>> +	depends on ARCH_TEGRA
>>>  	select PM_OPP
>>>  	help
>>>  	  This adds the DEVFREQ driver for the Tegra family of SoCs.
>>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>>> index f0711d5ad27d..0a2465a58cf5 100644
>>> --- a/drivers/devfreq/tegra-devfreq.c
>>> +++ b/drivers/devfreq/tegra-devfreq.c
>>> @@ -719,6 +719,7 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
>>>  }
>>>  
>>>  static const struct of_device_id tegra_devfreq_of_match[] = {
>>> +	{ .compatible = "nvidia,tegra30-actmon" },
>>>  	{ .compatible = "nvidia,tegra124-actmon" },
>>>  	{ },
>>>  };
>>>
>>
>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>
> 
> Thanks.
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 10/19] PM / devfreq: tegra: Drop primary interrupt handler

[Chanwoo Choi]

Hi,

On 19. 4. 17. 오전 12:23, Dmitry Osipenko wrote:
> 16.04.2019 8:56, Chanwoo Choi пишет:
>> Hi,
>>
>> It looks good to me to drop the primary interrupt handler
>> but I have some comments. Please check it.
>>
>> On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
>>> There is no real need in the primary interrupt handler, hence move
>>> everything to the secondary (threaded) handler. In a result locking
>>> is consistent now and there are no potential races with the interrupt
>>> handler because it is protected with the devfreq's mutex.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  drivers/devfreq/tegra-devfreq.c | 61 ++++++++++++---------------------
>>>  1 file changed, 22 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>>> index 2a1464098200..69b557df5084 100644
>>> --- a/drivers/devfreq/tegra-devfreq.c
>>> +++ b/drivers/devfreq/tegra-devfreq.c
>>> @@ -144,7 +144,6 @@ static struct tegra_devfreq_device_config actmon_device_configs[] = {
>>>  struct tegra_devfreq_device {
>>>  	const struct tegra_devfreq_device_config *config;
>>>  	void __iomem *regs;
>>> -	spinlock_t lock;
>>>  
>>>  	/* Average event count sampled in the last interrupt */
>>>  	u32 avg_count;
>>> @@ -249,11 +248,8 @@ static void actmon_write_barrier(struct tegra_devfreq *tegra)
>>>  static void actmon_isr_device(struct tegra_devfreq *tegra,
>>>  			      struct tegra_devfreq_device *dev)
>>>  {
>>> -	unsigned long flags;
>>>  	u32 intr_status, dev_ctrl;
>>>  
>>> -	spin_lock_irqsave(&dev->lock, flags);
>>> -
>>>  	dev->avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
>>>  	tegra_devfreq_update_avg_wmark(tegra, dev);
>>>  
>>> @@ -302,26 +298,6 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>>>  	device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
>>>  
>>>  	actmon_write_barrier(tegra);
>>> -
>>> -	spin_unlock_irqrestore(&dev->lock, flags);
>>> -}
>>> -
>>> -static irqreturn_t actmon_isr(int irq, void *data)
>>> -{
>>> -	struct tegra_devfreq *tegra = data;
>>> -	bool handled = false;
>>> -	unsigned int i;
>>> -	u32 val;
>>> -
>>> -	val = actmon_readl(tegra, ACTMON_GLB_STATUS);
>>> -	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
>>> -		if (val & tegra->devices[i].config->irq_mask) {
>>> -			actmon_isr_device(tegra, tegra->devices + i);
>>> -			handled = true;
>>> -		}
>>> -	}
>>> -
>>> -	return handled ? IRQ_WAKE_THREAD : IRQ_NONE;
>>>  }
>>>  
>>>  static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra,
>>> @@ -348,35 +324,46 @@ static void actmon_update_target(struct tegra_devfreq *tegra,
>>>  	unsigned long cpu_freq = 0;
>>>  	unsigned long static_cpu_emc_freq = 0;
>>>  	unsigned int avg_sustain_coef;
>>> -	unsigned long flags;
>>> +	u32 avg_count;
>>>  
>>>  	if (dev->config->avg_dependency_threshold) {
>>>  		cpu_freq = cpufreq_get(0);
>>>  		static_cpu_emc_freq = actmon_cpu_to_emc_rate(tegra, cpu_freq);
>>>  	}
>>>  
>>> -	spin_lock_irqsave(&dev->lock, flags);
>>> -
>>> -	dev->target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD;
>>> +	avg_count = dev->avg_count;
>>> +	dev->target_freq = avg_count / ACTMON_SAMPLING_PERIOD;
>>
>> Actually, this change is not related to this patch.
>> Please keep the original code.
>>
>>>  	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;
>>>  
>>> -	if (dev->avg_count >= dev->config->avg_dependency_threshold)
>>> +	if (avg_count >= dev->config->avg_dependency_threshold)
>>
>> ditto.
> 
> Good catch, it's a leftover from v1 that I forgot to revert. Thank you.
> 
> [snip]
> 
>>
>> When I review this patch, I have a question
>> about why tegra_actmon_rate_notify_cb is needed.
>>
>> tegra_actmon_rate_notify_cb() do something
>> when the clock rate of emc_clock is changed.
>> I think that 'emc_clock' is changed by this driver.
>> It means that the this driver can catch the change timing
>> of emc_clock rate without notifier. 
> 
> The devfreq driver isn't the only driver of the EMC clock rate. The devfreq driver changes EMC freq dynamically based of on average memory usage activity, but for some hardware units (like display controller for example) there is a requirement for a minimum memory bandwidth (isochronous transactions) and hence when display is waking up from suspend it immediately requires an amount of memory bandwidth that could be higher than ACTMON hardware unit suggests and besides the ACTMON's reaction is delayed by the sampling period (12ms).

Thanks for explaining it. If EMC clock is used on multiple point,
I understand why the clock notifier is necessary.

> 
>> IMO, it is possible to call tegra_devfreq_update_wmark()
>> directly without clock notifier before calling
>> 'clk_set_min_rate()/clk_set_rate()' in the tegra_devfreq_target().
>>
>> With clock notifier, it cannot restore something for
>> tegra_devfreq_update_wmark(tegra, dev) when failed to
>> set the rate of emc_clk by 'clk_set_min_rate()/clk_set_rate()'.
> 
> The watermarks should be changed in accordance to the actual EMC clock rate. Given that EMC rate could be changed by something else than the devfreq driver, we need to re-adjust the watermarks to the actual values after the EMC rate-change completion. Please note that the clock notifier uses the POST_RATE_CHANGE event that happens only after successful completion of EMC clock rate change.

I understand the needs of clock notifier. Thanks.

> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 12/19] PM / devfreq: tegra: Avoid inconsistency of current frequency value

[Chanwoo Choi]

Hi,

On 19. 4. 17. 오전 12:40, Dmitry Osipenko wrote:
> 16.04.2019 10:15, Chanwoo Choi пишет:
>> Hi,
>>
>> On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
>>> The frequency value potentially could change in-between. It doesn't
>>> cause any real problem at all right now, but that could change in the
>>> future. Hence let's avoid the inconsistency.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  drivers/devfreq/tegra-devfreq.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>>> index a668e4fbc874..f1a6f951813a 100644
>>> --- a/drivers/devfreq/tegra-devfreq.c
>>> +++ b/drivers/devfreq/tegra-devfreq.c
>>> @@ -496,13 +496,15 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>>>  {
>>>  	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>>>  	struct tegra_devfreq_device *actmon_dev;
>>> +	unsigned long cur_freq;
>>>  
>>> -	stat->current_frequency = tegra->cur_freq * KHZ;
>>> +	cur_freq = READ_ONCE(tegra->cur_freq);
>>>  
>>>  	/* 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;
>>>  
>>>  	actmon_dev = &tegra->devices[MCALL];
>>>  
>>> @@ -513,7 +515,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 * tegra->cur_freq;
>>> +	stat->total_time = ACTMON_SAMPLING_PERIOD * cur_freq;
>>>  
>>>  	stat->busy_time = min(stat->busy_time, stat->total_time);
>>>  
>>>
>>
>> The read/write access of tegra->cur_freq is in the single routine
>> of update_devfreq() as following. I think that there are no any
>> potential problem about the inconsistency of tegra->cur_freq.
> 
> No, that's wrong assumption. The tegra->cur_freq is changed by the clock notifier that runs asynchronously with the devfreq driver when EMC clock rate is changed by something else in the kernel. 
> 
>> IMHO, if there are no any problem now, I'm not sure that we need
>> to apply this patch.
>>
>> update_devfreq()
>> {
>> 	devfreq->governor->get_target_freq()
>> 		devfreq_update_stats(devfreq)
>> 			tegra_devfreq_get_dev_status()
>> 				stat->current_frequency = tegra->cur_freq * KHZ;
>>
>> 	devfreq_set_target()
>> 		tegra_devfreq_target()
>> 			clk_set_min_rate(emc_rate, )
>> 				tegra_actmon_rate_notify_cb()
>> 					tegra->cur_freq = data->new_rate / KHZ;
>> 		
>> 			clk_set_rate(emc_rate, )
>> 				tegra_actmon_rate_notify_cb()
>> 					tegra->cur_freq = data->new_rate / KHZ;
>> }
>>
>>
> 
> The cur_freq value is changed by the clock notifier that runs asynchronously with the rest of the devfreq driver. Hence potentially compiler may generate two separate fetches of the cur_freq value, then the clock rate could be changed by other CPU core simultaneously with tegra_devfreq_get_dev_status() or kernel may re-schedule preemptively, changing the clock rate in-between of the two fetches.
> 
> 

Thanks. I understand why have to consider the inconsistency of clock
which is used on the multiple points.

Looks good to me.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 05/19] PM / devfreq: tegra: Replace write memory barrier with the read barrier

[Chanwoo Choi]

On 19. 4. 16. 오후 10:57, Dmitry Osipenko wrote:
> 16.04.2019 11:00, Chanwoo Choi пишет:
>> Hi,
>>
>> On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
>>> The write memory barrier isn't needed because the BUS buffer is flushed
>>> by read after write that happens after the removed wmb(), we will also
>>> use readl() instead of the relaxed version to ensure that read is indeed
>>> completed.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  drivers/devfreq/tegra-devfreq.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>>> index d62fb1b0d9bb..f0f0d78f6cbf 100644
>>> --- a/drivers/devfreq/tegra-devfreq.c
>>> +++ b/drivers/devfreq/tegra-devfreq.c
>>> @@ -243,8 +243,7 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
>>>  static void actmon_write_barrier(struct tegra_devfreq *tegra)
>>>  {
>>>  	/* ensure the update has reached the ACTMON */
>>> -	wmb();
>>> -	actmon_readl(tegra, ACTMON_GLB_STATUS);
>>> +	readl(tegra->regs + ACTMON_GLB_STATUS);
>>
>> I think that this meaning of actmon_write_barrier() keeps
>> the order of 'store' assembly command without the execution change
>> from compiler optimization by using the wmb().
> 
> The IO mapped memory is strongly-ordered on ARM, hence all readl/writel accesses are guaranteed to be ordered by default. I think wmb() here is just a cut-n-pasted relic from old downstream driver.

OK.

> 
>> But, this patch edits it as following:
>> The result of the following two cases are same?
>>
>> [original code]
>> 	wmb()
>> 	read_relaxed()
>>
>> [new code by this patch]
>> 	readl_relaxed()
>> 	rmb()
> 
> Yes, the result is the same. The wmb() is not just about IO accesses, but about all kind of memory accesses and at least on Tegra30 it is quite expensive operation because it translates into L2XO cache syncing (arm_heavy_mb) + dsb(). It should be more efficient to flush out writes with a read-back and then wait for that read operation to be completed.
> 
> 

OK. Thanks for explanation.

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


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 06/19] PM / devfreq: tegra: Fix missed error checking on devfreq initialization failure

[Chanwoo Choi]

Hi,

On 19. 4. 16. 오후 11:29, Dmitry Osipenko wrote:
> 16.04.2019 5:32, Chanwoo Choi пишет:
>> Hi,
>>
>> patch6/7/8/9 are for handling of exception handling in probe() function.
>> Actually, I'm not sure that there are special reason to split out
>> the patches. I think that you can squash patch6/7/8/9 to only one patch.
> 
> Indeed, I was rebasing and reordering patches multiple times and looks like there is no reason not to squash these patches now.
> 
>> Also, even if patch6/7/8/9 handle the exception handling in probe(),
>> the tegra_devfreq_probe() doesn't support the restoring sequence
>> when fail happen. I think that if you want to fix the fail case of probe(),
>> please add the restoring sequence about following function.
>> - clk_disable_unprepare()
>> - clk_notifier_unregister()
>> - dev_pm_opp_remove()
> 
> When all of 6/7/8/9 patches are applied, the clk_notifier_register() becomes the last invocation of the probe function and clk_enable() is kept at the first place of the probe order. Hence the sequence you're suggesting is already incorrect because error-unwinding order usually should be opposite to the probe order. It looks to me that the current final result of these patches is already correct.

You're right. When I replied it, I have not considered the order.
Sorry, it made you some confusion.

> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 02/19] PM / devfreq: tegra: Replace readl-writel with relaxed versions

[Chanwoo Choi]

Hi,

On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
> There is no need to insert memory barrier on each readl/writel
> invocation, hence use the relaxed versions.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra-devfreq.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index 02905978abe1..0c0909fba545 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -191,23 +191,23 @@ static struct tegra_actmon_emc_ratio actmon_emc_ratios[] = {
>  
>  static u32 actmon_readl(struct tegra_devfreq *tegra, u32 offset)
>  {
> -	return readl(tegra->regs + offset);
> +	return readl_relaxed(tegra->regs + offset);
>  }
>  
>  static void actmon_writel(struct tegra_devfreq *tegra, u32 val, u32 offset)
>  {
> -	writel(val, tegra->regs + offset);
> +	writel_relaxed(val, tegra->regs + offset);
>  }
>  
>  static u32 device_readl(struct tegra_devfreq_device *dev, u32 offset)
>  {
> -	return readl(dev->regs + offset);
> +	return readl_relaxed(dev->regs + offset);
>  }
>  
>  static void device_writel(struct tegra_devfreq_device *dev, u32 val,
>  			  u32 offset)
>  {
> -	writel(val, dev->regs + offset);
> +	writel_relaxed(val, dev->regs + offset);
>  }
>  
>  static unsigned long do_percent(unsigned long val, unsigned int pct)
> 

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

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 19/19] PM / devfreq: Introduce driver for NVIDIA Tegra20

[Dmitry Osipenko]

17.04.2019 3:26, Chanwoo Choi пишет:
> Hi,
> 
> On 19. 4. 17. 오전 1:11, Dmitry Osipenko wrote:
>> 16.04.2019 11:31, Chanwoo Choi пишет:
>>> Hi,
>>>
>>> On 19. 4. 15. 오후 11:55, Dmitry Osipenko wrote:
>>>> Add devfreq driver for NVIDIA Tegra20 SoC's. The driver periodically
>>>> reads out Memory Controller counters and adjusts memory frequency based
>>>> on the memory clients activity.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>  MAINTAINERS                       |   8 ++
>>>>  drivers/devfreq/Kconfig           |  10 ++
>>>>  drivers/devfreq/Makefile          |   1 +
>>>>  drivers/devfreq/tegra20-devfreq.c | 177 ++++++++++++++++++++++++++++++
>>>>  4 files changed, 196 insertions(+)
>>>>  create mode 100644 drivers/devfreq/tegra20-devfreq.c
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 80b59db1b6e4..91f475ec4545 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -10056,6 +10056,14 @@ F:	include/linux/memblock.h
>>>>  F:	mm/memblock.c
>>>>  F:	Documentation/core-api/boot-time-mm.rst
>>>>  
>>>> +MEMORY FREQUENCY SCALING DRIVER FOR NVIDIA TEGRA20
>>>> +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
>>>> +
>>>>  MEMORY MANAGEMENT
>>>>  L:	linux-mm@kvack.org
>>>>  W:	http://www.linux-mm.org
>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>> index bd6652863e7d..af4c86c4e0f6 100644
>>>> --- a/drivers/devfreq/Kconfig
>>>> +++ b/drivers/devfreq/Kconfig
>>>> @@ -100,6 +100,16 @@ config ARM_TEGRA_DEVFREQ
>>>>  	  It reads ACTMON counters of memory controllers and adjusts the
>>>>  	  operating frequencies and voltages with OPP support.
>>>>  
>>>> +config ARM_TEGRA20_DEVFREQ
>>>> +	tristate "NVIDIA Tegra20 DEVFREQ Driver"
>>>> +	depends on (TEGRA_MC && TEGRA20_EMC) || COMPILE_TEST
>>>> +	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>>>> +	select PM_OPP
>>>> +	help
>>>> +	  This adds the DEVFREQ driver for the Tegra20 family of SoCs.
>>>> +	  It reads Memory Controller counters and adjusts the operating
>>>> +	  frequencies and voltages with OPP support.
>>>> +
>>>>  config ARM_RK3399_DMC_DEVFREQ
>>>>  	tristate "ARM RK3399 DMC DEVFREQ Driver"
>>>>  	depends on ARCH_ROCKCHIP
>>>> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
>>>> index 32b8d4d3f12c..6fcc5596b8b7 100644
>>>> --- a/drivers/devfreq/Makefile
>>>> +++ b/drivers/devfreq/Makefile
>>>> @@ -11,6 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
>>>>  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
>>>>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
>>>>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
>>>> +obj-$(CONFIG_ARM_TEGRA20_DEVFREQ)	+= tegra20-devfreq.o
>>>>  
>>>>  # DEVFREQ Event Drivers
>>>>  obj-$(CONFIG_PM_DEVFREQ_EVENT)		+= event/
>>>> diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c
>>>> new file mode 100644
>>>> index 000000000000..18c9aad7a9d7
>>>> --- /dev/null
>>>> +++ b/drivers/devfreq/tegra20-devfreq.c
>>>> @@ -0,0 +1,177 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * NVIDIA Tegra20 devfreq driver
>>>> + *
>>>> + * Author: Dmitry Osipenko <digetx@gmail.com>
>>>> + */
>>>
>>> It doesn't any "Copyright (c) 2019 ..." sentence.
>>
>> I'll add one in v3.
>>
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/devfreq.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/pm_opp.h>
>>>> +#include <linux/slab.h>
>>>> +
>>>> +#include <soc/tegra/mc.h>
>>>
>>> I can find the '<soc/tegra/mc.h>' header file
>>> on mainline branch. But mc.h is included in linux-next.git. 
>>>
>>> If you don't share the patch related to mc.h,
>>> the kernel build will be failed when apply it the devfreq.git
>>> on final step. Actually, it should make the immutable branch
>>> between two related maintainers in order to remove the build fail.
>>
>> The '<soc/tegra/mc.h>' header file exists since v3.18 [0], seems you just missed something.
>>
>> [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/soc/tegra/mc.h?h=v3.18&id=8918465163171322c77a19d5258a95f56d89d2e4
> 
> Sorry. It is my missing point. When I tried to find it,
> it is included in the mainline kernel.
> 
>>
>>>> +
>>>> +#include "governor.h"
>>>> +
>>>> +#define MC_STAT_CONTROL				0x90
>>>> +#define MC_STAT_EMC_CLOCK_LIMIT			0xa0
>>>> +#define MC_STAT_EMC_CLOCKS			0xa4
>>>> +#define MC_STAT_EMC_CONTROL			0xa8
>>>> +#define MC_STAT_EMC_COUNT			0xb8
>>>> +
>>>> +#define EMC_GATHER_CLEAR			(1 << 8)
>>>> +#define EMC_GATHER_ENABLE			(3 << 8)
>>>> +
>>>> +struct tegra_devfreq {
>>>> +	struct devfreq *devfreq;
>>>> +	struct clk *clk;
>>>> +	void __iomem *regs;
>>>> +};
>>>> +
>>>> +static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>>>> +				u32 flags)
>>>> +{
>>>> +	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>>>> +	struct dev_pm_opp *opp;
>>>> +	unsigned long rate;
>>>> +	int err;
>>>> +
>>>> +	opp = devfreq_recommended_opp(dev, freq, flags);
>>>> +	if (IS_ERR(opp))
>>>> +		return PTR_ERR(opp);
>>>> +
>>>> +	rate = dev_pm_opp_get_freq(opp);
>>>> +	dev_pm_opp_put(opp);
>>>> +
>>>> +	err = clk_set_min_rate(tegra->clk, rate);
>>>> +	if (err)
>>>> +		return err;
>>>> +
>>>> +	err = clk_set_rate(tegra->clk, 0);
>>>> +	if (err)
>>>
>>> When fail happen, I think that you have to control
>>> the restoring sequence for previous operation like clk_set_min_rate().
>>
>> Okay, thanks.
>>
>>>> +		return err;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int tegra_devfreq_get_dev_status(struct device *dev,
>>>> +					struct devfreq_dev_status *stat)
>>>> +{
>>>> +	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>>>> +
>>>> +	stat->busy_time = readl_relaxed(tegra->regs + MC_STAT_EMC_COUNT);
>>>> +	stat->total_time = readl_relaxed(tegra->regs + MC_STAT_EMC_CLOCKS) / 8;
>>>> +	stat->current_frequency = clk_get_rate(tegra->clk);
>>>> +
>>>> +	writel_relaxed(EMC_GATHER_CLEAR, tegra->regs + MC_STAT_CONTROL);
>>>> +	writel_relaxed(EMC_GATHER_ENABLE, tegra->regs + MC_STAT_CONTROL);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static struct devfreq_dev_profile tegra_devfreq_profile = {
>>>> +	.polling_ms	= 500,
>>>> +	.target		= tegra_devfreq_target,
>>>> +	.get_dev_status	= tegra_devfreq_get_dev_status,
>>>> +};
>>>> +
>>>> +static struct tegra_mc *tegra_get_memory_controller(void)
>>>> +{
>>>> +	struct platform_device *pdev;
>>>> +	struct device_node *np;
>>>> +	struct tegra_mc *mc;
>>>> +
>>>> +	np = of_find_compatible_node(NULL, NULL, "nvidia,tegra20-mc-gart");
>>>> +	if (!np)
>>>> +		return ERR_PTR(-ENOENT);
>>>> +
>>>> +	pdev = of_find_device_by_node(np);
>>>> +	of_node_put(np);
>>>> +	if (!pdev)
>>>> +		return ERR_PTR(-ENODEV);
>>>> +
>>>> +	mc = platform_get_drvdata(pdev);
>>>> +	if (!mc)
>>>> +		return ERR_PTR(-EPROBE_DEFER);
>>>> +
>>>> +	return mc;
>>>> +}
>>>> +
>>>> +static int tegra_devfeq_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct tegra_devfreq *tegra;
>>>> +	struct tegra_mc *mc;
>>>> +	unsigned long max_rate;
>>>> +	unsigned long rate;
>>>> +	int err;
>>>> +
>>>> +	mc = tegra_get_memory_controller();
>>>> +	if (IS_ERR(mc)) {
>>>> +		err = PTR_ERR(mc);
>>>> +		dev_err(&pdev->dev, "failed to get mc: %d\n", err);
>>>
>>> How about using 'memory controller' instead of 'mc'?
>>> Because 'mc' is not standard expression.
>>
>> Sounds good, thanks.
>>
>>>> +		return err;
>>>> +	}
>>>> +
>>>> +	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
>>>> +	if (!tegra)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	tegra->clk = devm_clk_get(&pdev->dev, "emc");
>>>> +	if (IS_ERR(tegra->clk)) {
>>>> +		err = PTR_ERR(tegra->clk);
>>>> +		dev_err(&pdev->dev, "failed to get emc clock: %d\n", err);
>>>> +		return err;
>>>> +	}
>>>
>>> Don't you need to enable the 'emc' clock'?
>>> Because this patch doesn't enable this clock.
>>
>> EMC is a system critical clock (marked as critical by the clock driver), it is guaranteed to be always enabled. I think it is fine to omit clock-enabling in this case.
> 
> If you think don't need to enable it due to the critical clock,
> instead, please add the comment about that emc clock is critical.
> In my case, it looks like the strange use-case because this driver
> doesn't contain the any enable code for clock.

Okay, thank you for the review again.

[snip]