All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/13] opp: Implement dev_pm_opp_set_opp()
@ 2021-01-21 11:17 ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Andy Gross, Bjorn Andersson, Chanwoo Choi,
	Jonathan Hunter, Kyungmin Park, MyungJoo Ham, Nishanth Menon,
	Rafael J. Wysocki, Rob Clark, Sean Paul, Stephen Boyd,
	Thierry Reding, Viresh Kumar, Viresh Kumar
  Cc: linux-pm, Vincent Guittot, Sibi Sankar, linux-kernel,
	linux-arm-kernel, dri-devel, freedreno, linux-arm-msm,
	linux-tegra

Hello,

This patchset implements a new API dev_pm_opp_set_opp(), which
configures the devices represented by an opp table to a particular opp.
The opp core supports a wide variety of devices now, some of them can
change frequency and other properties (like CPUs), while others can just
change their pstates or regulators (like power domains) and then there
are others which can change their bandwidth as well (interconnects).
Instead of having separate implementations for all of them, where all
will eventually lack something or the other, lets try to implement a
common solution for everyone. This takes care of setting regulators, bw,
required opps, etc for all device types.

Dmitry, please go ahead and try this series. This is based of opp tree's
linux-next branch.

Sibi, since you added dev_pm_opp_set_bw() earlier, it would be good if
you can give this a try. In case this breaks anything for you.

I have already tested this on hikey board for CPU devices.

To get this tested better and as early as possible, I have pushed it
here:

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next

This will be part of linux-next tomorrow.

Note, all the patches need to go through OPP tree here. Please provide
your Acks for platform specific bits.

--
Viresh

Viresh Kumar (13):
  opp: Rename _opp_set_rate_zero()
  opp: No need to check clk for errors
  opp: Keep track of currently programmed OPP
  opp: Split _set_opp() out of dev_pm_opp_set_rate()
  opp: Allow _set_opp() to work for non-freq devices
  opp: Allow _generic_set_opp_regulator() to work for non-freq devices
  opp: Allow _generic_set_opp_clk_only() to work for non-freq devices
  opp: Update parameters of  _set_opp_custom()
  opp: Implement dev_pm_opp_set_opp()
  cpufreq: qcom: Migrate to dev_pm_opp_set_opp()
  devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
  drm: msm: Migrate to dev_pm_opp_set_opp()
  opp: Remove dev_pm_opp_set_bw()

 drivers/cpufreq/qcom-cpufreq-hw.c     |   2 +-
 drivers/devfreq/tegra30-devfreq.c     |   2 +-
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c |   8 +-
 drivers/opp/core.c                    | 314 ++++++++++++++------------
 drivers/opp/opp.h                     |   2 +
 include/linux/pm_opp.h                |   6 +-
 6 files changed, 184 insertions(+), 150 deletions(-)

-- 
2.25.0.rc1.19.g042ed3e048af


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 00/13] opp: Implement dev_pm_opp_set_opp()
@ 2021-01-21 11:17 ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Andy Gross, Bjorn Andersson, Chanwoo Choi,
	Jonathan Hunter, Kyungmin Park, MyungJoo Ham, Nishanth Menon,
	Rafael J. Wysocki, Rob Clark, Sean Paul, Stephen Boyd,
	Thierry Reding, Viresh Kumar, Viresh Kumar
  Cc: Vincent Guittot, linux-pm, linux-arm-msm, linux-kernel,
	dri-devel, Sibi Sankar, linux-tegra, freedreno, linux-arm-kernel

Hello,

This patchset implements a new API dev_pm_opp_set_opp(), which
configures the devices represented by an opp table to a particular opp.
The opp core supports a wide variety of devices now, some of them can
change frequency and other properties (like CPUs), while others can just
change their pstates or regulators (like power domains) and then there
are others which can change their bandwidth as well (interconnects).
Instead of having separate implementations for all of them, where all
will eventually lack something or the other, lets try to implement a
common solution for everyone. This takes care of setting regulators, bw,
required opps, etc for all device types.

Dmitry, please go ahead and try this series. This is based of opp tree's
linux-next branch.

Sibi, since you added dev_pm_opp_set_bw() earlier, it would be good if
you can give this a try. In case this breaks anything for you.

I have already tested this on hikey board for CPU devices.

To get this tested better and as early as possible, I have pushed it
here:

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next

This will be part of linux-next tomorrow.

Note, all the patches need to go through OPP tree here. Please provide
your Acks for platform specific bits.

--
Viresh

Viresh Kumar (13):
  opp: Rename _opp_set_rate_zero()
  opp: No need to check clk for errors
  opp: Keep track of currently programmed OPP
  opp: Split _set_opp() out of dev_pm_opp_set_rate()
  opp: Allow _set_opp() to work for non-freq devices
  opp: Allow _generic_set_opp_regulator() to work for non-freq devices
  opp: Allow _generic_set_opp_clk_only() to work for non-freq devices
  opp: Update parameters of  _set_opp_custom()
  opp: Implement dev_pm_opp_set_opp()
  cpufreq: qcom: Migrate to dev_pm_opp_set_opp()
  devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
  drm: msm: Migrate to dev_pm_opp_set_opp()
  opp: Remove dev_pm_opp_set_bw()

 drivers/cpufreq/qcom-cpufreq-hw.c     |   2 +-
 drivers/devfreq/tegra30-devfreq.c     |   2 +-
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c |   8 +-
 drivers/opp/core.c                    | 314 ++++++++++++++------------
 drivers/opp/opp.h                     |   2 +
 include/linux/pm_opp.h                |   6 +-
 6 files changed, 184 insertions(+), 150 deletions(-)

-- 
2.25.0.rc1.19.g042ed3e048af


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 00/13] opp: Implement dev_pm_opp_set_opp()
@ 2021-01-21 11:17 ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Andy Gross, Bjorn Andersson, Chanwoo Choi,
	Jonathan Hunter, Kyungmin Park, MyungJoo Ham, Nishanth Menon,
	Rafael J. Wysocki, Rob Clark, Sean Paul, Stephen Boyd,
	Thierry Reding, Viresh Kumar, Viresh Kumar
  Cc: Vincent Guittot, linux-pm, linux-arm-msm, linux-kernel,
	dri-devel, Sibi Sankar, linux-tegra, freedreno, linux-arm-kernel

Hello,

This patchset implements a new API dev_pm_opp_set_opp(), which
configures the devices represented by an opp table to a particular opp.
The opp core supports a wide variety of devices now, some of them can
change frequency and other properties (like CPUs), while others can just
change their pstates or regulators (like power domains) and then there
are others which can change their bandwidth as well (interconnects).
Instead of having separate implementations for all of them, where all
will eventually lack something or the other, lets try to implement a
common solution for everyone. This takes care of setting regulators, bw,
required opps, etc for all device types.

Dmitry, please go ahead and try this series. This is based of opp tree's
linux-next branch.

Sibi, since you added dev_pm_opp_set_bw() earlier, it would be good if
you can give this a try. In case this breaks anything for you.

I have already tested this on hikey board for CPU devices.

To get this tested better and as early as possible, I have pushed it
here:

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next

This will be part of linux-next tomorrow.

Note, all the patches need to go through OPP tree here. Please provide
your Acks for platform specific bits.

--
Viresh

Viresh Kumar (13):
  opp: Rename _opp_set_rate_zero()
  opp: No need to check clk for errors
  opp: Keep track of currently programmed OPP
  opp: Split _set_opp() out of dev_pm_opp_set_rate()
  opp: Allow _set_opp() to work for non-freq devices
  opp: Allow _generic_set_opp_regulator() to work for non-freq devices
  opp: Allow _generic_set_opp_clk_only() to work for non-freq devices
  opp: Update parameters of  _set_opp_custom()
  opp: Implement dev_pm_opp_set_opp()
  cpufreq: qcom: Migrate to dev_pm_opp_set_opp()
  devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
  drm: msm: Migrate to dev_pm_opp_set_opp()
  opp: Remove dev_pm_opp_set_bw()

 drivers/cpufreq/qcom-cpufreq-hw.c     |   2 +-
 drivers/devfreq/tegra30-devfreq.c     |   2 +-
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c |   8 +-
 drivers/opp/core.c                    | 314 ++++++++++++++------------
 drivers/opp/opp.h                     |   2 +
 include/linux/pm_opp.h                |   6 +-
 6 files changed, 184 insertions(+), 150 deletions(-)

-- 
2.25.0.rc1.19.g042ed3e048af

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 01/13] opp: Rename _opp_set_rate_zero()
  2021-01-21 11:17 ` Viresh Kumar
@ 2021-01-21 11:17   ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki,
	Sibi Sankar, linux-kernel, linux-arm-kernel

This routine has nothing to do with frequency, it just disables all the
resources previously enabled. Rename it to match its purpose.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index aea0b9d7203c..9ec8e42981d0 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -943,7 +943,7 @@ int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_bw);
 
-static int _opp_set_rate_zero(struct device *dev, struct opp_table *opp_table)
+static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
 {
 	int ret;
 
@@ -997,7 +997,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	}
 
 	if (unlikely(!target_freq)) {
-		ret = _opp_set_rate_zero(dev, opp_table);
+		ret = _disable_opp_table(dev, opp_table);
 		goto put_opp_table;
 	}
 
-- 
2.25.0.rc1.19.g042ed3e048af


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 01/13] opp: Rename _opp_set_rate_zero()
@ 2021-01-21 11:17   ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Vincent Guittot, linux-pm, Viresh Kumar, Rafael Wysocki,
	linux-kernel, Sibi Sankar, linux-arm-kernel

This routine has nothing to do with frequency, it just disables all the
resources previously enabled. Rename it to match its purpose.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index aea0b9d7203c..9ec8e42981d0 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -943,7 +943,7 @@ int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_bw);
 
-static int _opp_set_rate_zero(struct device *dev, struct opp_table *opp_table)
+static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
 {
 	int ret;
 
@@ -997,7 +997,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	}
 
 	if (unlikely(!target_freq)) {
-		ret = _opp_set_rate_zero(dev, opp_table);
+		ret = _disable_opp_table(dev, opp_table);
 		goto put_opp_table;
 	}
 
-- 
2.25.0.rc1.19.g042ed3e048af


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 02/13] opp: No need to check clk for errors
  2021-01-21 11:17 ` Viresh Kumar
@ 2021-01-21 11:17   ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki,
	Sibi Sankar, linux-kernel, linux-arm-kernel

Clock is not optional for users who call into dev_pm_opp_set_rate().
Remove the unnecessary checks.

While at it also drop the local variable for clk and use opp_table->clk
instead.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 9ec8e42981d0..cb5b67ccf5cf 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -987,7 +987,6 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	struct opp_table *opp_table;
 	unsigned long freq, old_freq, temp_freq;
 	struct dev_pm_opp *old_opp, *opp;
-	struct clk *clk;
 	int ret;
 
 	opp_table = _find_opp_table(dev);
@@ -1001,19 +1000,11 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		goto put_opp_table;
 	}
 
-	clk = opp_table->clk;
-	if (IS_ERR(clk)) {
-		dev_err(dev, "%s: No clock available for the device\n",
-			__func__);
-		ret = PTR_ERR(clk);
-		goto put_opp_table;
-	}
-
-	freq = clk_round_rate(clk, target_freq);
+	freq = clk_round_rate(opp_table->clk, target_freq);
 	if ((long)freq <= 0)
 		freq = target_freq;
 
-	old_freq = clk_get_rate(clk);
+	old_freq = clk_get_rate(opp_table->clk);
 
 	/* Return early if nothing to do */
 	if (opp_table->enabled && old_freq == freq) {
@@ -1031,7 +1022,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	 * equivalent to a clk_set_rate()
 	 */
 	if (!_get_opp_count(opp_table)) {
-		ret = _generic_set_opp_clk_only(dev, clk, freq);
+		ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
 		goto put_opp_table;
 	}
 
@@ -1071,7 +1062,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 						 opp->supplies);
 	} else {
 		/* Only frequency scaling */
-		ret = _generic_set_opp_clk_only(dev, clk, freq);
+		ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
 	}
 
 	/* Scaling down? Configure required OPPs after frequency */
-- 
2.25.0.rc1.19.g042ed3e048af


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 02/13] opp: No need to check clk for errors
@ 2021-01-21 11:17   ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Vincent Guittot, linux-pm, Viresh Kumar, Rafael Wysocki,
	linux-kernel, Sibi Sankar, linux-arm-kernel

Clock is not optional for users who call into dev_pm_opp_set_rate().
Remove the unnecessary checks.

While at it also drop the local variable for clk and use opp_table->clk
instead.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 9ec8e42981d0..cb5b67ccf5cf 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -987,7 +987,6 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	struct opp_table *opp_table;
 	unsigned long freq, old_freq, temp_freq;
 	struct dev_pm_opp *old_opp, *opp;
-	struct clk *clk;
 	int ret;
 
 	opp_table = _find_opp_table(dev);
@@ -1001,19 +1000,11 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		goto put_opp_table;
 	}
 
-	clk = opp_table->clk;
-	if (IS_ERR(clk)) {
-		dev_err(dev, "%s: No clock available for the device\n",
-			__func__);
-		ret = PTR_ERR(clk);
-		goto put_opp_table;
-	}
-
-	freq = clk_round_rate(clk, target_freq);
+	freq = clk_round_rate(opp_table->clk, target_freq);
 	if ((long)freq <= 0)
 		freq = target_freq;
 
-	old_freq = clk_get_rate(clk);
+	old_freq = clk_get_rate(opp_table->clk);
 
 	/* Return early if nothing to do */
 	if (opp_table->enabled && old_freq == freq) {
@@ -1031,7 +1022,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	 * equivalent to a clk_set_rate()
 	 */
 	if (!_get_opp_count(opp_table)) {
-		ret = _generic_set_opp_clk_only(dev, clk, freq);
+		ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
 		goto put_opp_table;
 	}
 
@@ -1071,7 +1062,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 						 opp->supplies);
 	} else {
 		/* Only frequency scaling */
-		ret = _generic_set_opp_clk_only(dev, clk, freq);
+		ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
 	}
 
 	/* Scaling down? Configure required OPPs after frequency */
-- 
2.25.0.rc1.19.g042ed3e048af


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 03/13] opp: Keep track of currently programmed OPP
  2021-01-21 11:17 ` Viresh Kumar
@ 2021-01-21 11:17   ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki,
	Sibi Sankar, linux-kernel, linux-arm-kernel

The dev_pm_opp_set_rate() helper needs to know the currently programmed
OPP to make few decisions and currently we try to find it on every
invocation of this routine.

Lets start keeping track of the current_opp programmed for the devices
of the opp table, that will be quite useful going forward.

If we fail to find the current OPP, we pick the first one available in
the list, as the list is in ascending order of frequencies, level, or
bandwidth and that's the best guess we can make anyway.

Note that we used to do the frequency comparison a bit early in
dev_pm_opp_set_rate() previously, and now instead we check the target
opp, which shall be more accurate anyway.

We need to make sure that current_opp's memory doesn't get freed while
it is being used and so we keep a reference of it until the time it is
used.

Now that current_opp will always be set, we can drop some unnecessary
checks as well.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 83 +++++++++++++++++++++++++++++-----------------
 drivers/opp/opp.h  |  2 ++
 2 files changed, 55 insertions(+), 30 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index cb5b67ccf5cf..4ee598344e6a 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -788,8 +788,7 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
 			__func__, old_freq);
 restore_voltage:
 	/* This shouldn't harm even if the voltages weren't updated earlier */
-	if (old_supply)
-		_set_opp_voltage(dev, reg, old_supply);
+	_set_opp_voltage(dev, reg, old_supply);
 
 	return ret;
 }
@@ -839,10 +838,7 @@ static int _set_opp_custom(const struct opp_table *opp_table,
 
 	data->old_opp.rate = old_freq;
 	size = sizeof(*old_supply) * opp_table->regulator_count;
-	if (!old_supply)
-		memset(data->old_opp.supplies, 0, size);
-	else
-		memcpy(data->old_opp.supplies, old_supply, size);
+	memcpy(data->old_opp.supplies, old_supply, size);
 
 	data->new_opp.rate = freq;
 	memcpy(data->new_opp.supplies, new_supply, size);
@@ -943,6 +939,31 @@ int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_bw);
 
+static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
+{
+	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
+	unsigned long freq;
+
+	if (!IS_ERR(opp_table->clk)) {
+		freq = clk_get_rate(opp_table->clk);
+		opp = _find_freq_ceil(opp_table, &freq);
+	}
+
+	/*
+	 * Unable to find the current OPP ? Pick the first from the list since
+	 * it is in ascending order, otherwise rest of the code will need to
+	 * make special checks to validate current_opp.
+	 */
+	if (IS_ERR(opp)) {
+		mutex_lock(&opp_table->lock);
+		opp = list_first_entry(&opp_table->opp_list, struct dev_pm_opp, node);
+		dev_pm_opp_get(opp);
+		mutex_unlock(&opp_table->lock);
+	}
+
+	opp_table->current_opp = opp;
+}
+
 static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
 {
 	int ret;
@@ -1004,16 +1025,6 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	if ((long)freq <= 0)
 		freq = target_freq;
 
-	old_freq = clk_get_rate(opp_table->clk);
-
-	/* Return early if nothing to do */
-	if (opp_table->enabled && old_freq == freq) {
-		dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to do\n",
-			__func__, freq);
-		ret = 0;
-		goto put_opp_table;
-	}
-
 	/*
 	 * For IO devices which require an OPP on some platforms/SoCs
 	 * while just needing to scale the clock on some others
@@ -1026,12 +1037,9 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		goto put_opp_table;
 	}
 
-	temp_freq = old_freq;
-	old_opp = _find_freq_ceil(opp_table, &temp_freq);
-	if (IS_ERR(old_opp)) {
-		dev_err(dev, "%s: failed to find current OPP for freq %lu (%ld)\n",
-			__func__, old_freq, PTR_ERR(old_opp));
-	}
+	/* Find the currently set OPP if we don't know already */
+	if (unlikely(!opp_table->current_opp))
+		_find_current_opp(dev, opp_table);
 
 	temp_freq = freq;
 	opp = _find_freq_ceil(opp_table, &temp_freq);
@@ -1039,7 +1047,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		ret = PTR_ERR(opp);
 		dev_err(dev, "%s: failed to find OPP for freq %lu (%d)\n",
 			__func__, freq, ret);
-		goto put_old_opp;
+		goto put_opp_table;
+	}
+
+	old_opp = opp_table->current_opp;
+	old_freq = old_opp->rate;
+
+	/* Return early if nothing to do */
+	if (opp_table->enabled && old_opp == opp) {
+		dev_dbg(dev, "%s: OPPs are same, nothing to do\n", __func__);
+		ret = 0;
+		goto put_opp;
 	}
 
 	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__,
@@ -1054,11 +1072,10 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	if (opp_table->set_opp) {
 		ret = _set_opp_custom(opp_table, dev, old_freq, freq,
-				      IS_ERR(old_opp) ? NULL : old_opp->supplies,
-				      opp->supplies);
+				      old_opp->supplies, opp->supplies);
 	} else if (opp_table->regulators) {
 		ret = _generic_set_opp_regulator(opp_table, dev, old_freq, freq,
-						 IS_ERR(old_opp) ? NULL : old_opp->supplies,
+						 old_opp->supplies,
 						 opp->supplies);
 	} else {
 		/* Only frequency scaling */
@@ -1074,15 +1091,18 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	if (!ret) {
 		ret = _set_opp_bw(opp_table, opp, dev, false);
-		if (!ret)
+		if (!ret) {
 			opp_table->enabled = true;
+			dev_pm_opp_put(old_opp);
+
+			/* Make sure current_opp doesn't get freed */
+			dev_pm_opp_get(opp);
+			opp_table->current_opp = opp;
+		}
 	}
 
 put_opp:
 	dev_pm_opp_put(opp);
-put_old_opp:
-	if (!IS_ERR(old_opp))
-		dev_pm_opp_put(old_opp);
 put_opp_table:
 	dev_pm_opp_put_opp_table(opp_table);
 	return ret;
@@ -1276,6 +1296,9 @@ static void _opp_table_kref_release(struct kref *kref)
 	list_del(&opp_table->node);
 	mutex_unlock(&opp_table_lock);
 
+	if (opp_table->current_opp)
+		dev_pm_opp_put(opp_table->current_opp);
+
 	_of_clear_opp_table(opp_table);
 
 	/* Release clk */
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 4408cfcb0f31..359fd89d5770 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -135,6 +135,7 @@ enum opp_table_access {
  * @clock_latency_ns_max: Max clock latency in nanoseconds.
  * @parsed_static_opps: Count of devices for which OPPs are initialized from DT.
  * @shared_opp: OPP is shared between multiple devices.
+ * @current_opp: Currently configured OPP for the table.
  * @suspend_opp: Pointer to OPP to be used during device suspend.
  * @genpd_virt_dev_lock: Mutex protecting the genpd virtual device pointers.
  * @genpd_virt_devs: List of virtual devices for multiple genpd support.
@@ -183,6 +184,7 @@ struct opp_table {
 
 	unsigned int parsed_static_opps;
 	enum opp_table_access shared_opp;
+	struct dev_pm_opp *current_opp;
 	struct dev_pm_opp *suspend_opp;
 
 	struct mutex genpd_virt_dev_lock;
-- 
2.25.0.rc1.19.g042ed3e048af


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 03/13] opp: Keep track of currently programmed OPP
@ 2021-01-21 11:17   ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Vincent Guittot, linux-pm, Viresh Kumar, Rafael Wysocki,
	linux-kernel, Sibi Sankar, linux-arm-kernel

The dev_pm_opp_set_rate() helper needs to know the currently programmed
OPP to make few decisions and currently we try to find it on every
invocation of this routine.

Lets start keeping track of the current_opp programmed for the devices
of the opp table, that will be quite useful going forward.

If we fail to find the current OPP, we pick the first one available in
the list, as the list is in ascending order of frequencies, level, or
bandwidth and that's the best guess we can make anyway.

Note that we used to do the frequency comparison a bit early in
dev_pm_opp_set_rate() previously, and now instead we check the target
opp, which shall be more accurate anyway.

We need to make sure that current_opp's memory doesn't get freed while
it is being used and so we keep a reference of it until the time it is
used.

Now that current_opp will always be set, we can drop some unnecessary
checks as well.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 83 +++++++++++++++++++++++++++++-----------------
 drivers/opp/opp.h  |  2 ++
 2 files changed, 55 insertions(+), 30 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index cb5b67ccf5cf..4ee598344e6a 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -788,8 +788,7 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
 			__func__, old_freq);
 restore_voltage:
 	/* This shouldn't harm even if the voltages weren't updated earlier */
-	if (old_supply)
-		_set_opp_voltage(dev, reg, old_supply);
+	_set_opp_voltage(dev, reg, old_supply);
 
 	return ret;
 }
@@ -839,10 +838,7 @@ static int _set_opp_custom(const struct opp_table *opp_table,
 
 	data->old_opp.rate = old_freq;
 	size = sizeof(*old_supply) * opp_table->regulator_count;
-	if (!old_supply)
-		memset(data->old_opp.supplies, 0, size);
-	else
-		memcpy(data->old_opp.supplies, old_supply, size);
+	memcpy(data->old_opp.supplies, old_supply, size);
 
 	data->new_opp.rate = freq;
 	memcpy(data->new_opp.supplies, new_supply, size);
@@ -943,6 +939,31 @@ int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_bw);
 
+static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
+{
+	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
+	unsigned long freq;
+
+	if (!IS_ERR(opp_table->clk)) {
+		freq = clk_get_rate(opp_table->clk);
+		opp = _find_freq_ceil(opp_table, &freq);
+	}
+
+	/*
+	 * Unable to find the current OPP ? Pick the first from the list since
+	 * it is in ascending order, otherwise rest of the code will need to
+	 * make special checks to validate current_opp.
+	 */
+	if (IS_ERR(opp)) {
+		mutex_lock(&opp_table->lock);
+		opp = list_first_entry(&opp_table->opp_list, struct dev_pm_opp, node);
+		dev_pm_opp_get(opp);
+		mutex_unlock(&opp_table->lock);
+	}
+
+	opp_table->current_opp = opp;
+}
+
 static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
 {
 	int ret;
@@ -1004,16 +1025,6 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	if ((long)freq <= 0)
 		freq = target_freq;
 
-	old_freq = clk_get_rate(opp_table->clk);
-
-	/* Return early if nothing to do */
-	if (opp_table->enabled && old_freq == freq) {
-		dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to do\n",
-			__func__, freq);
-		ret = 0;
-		goto put_opp_table;
-	}
-
 	/*
 	 * For IO devices which require an OPP on some platforms/SoCs
 	 * while just needing to scale the clock on some others
@@ -1026,12 +1037,9 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		goto put_opp_table;
 	}
 
-	temp_freq = old_freq;
-	old_opp = _find_freq_ceil(opp_table, &temp_freq);
-	if (IS_ERR(old_opp)) {
-		dev_err(dev, "%s: failed to find current OPP for freq %lu (%ld)\n",
-			__func__, old_freq, PTR_ERR(old_opp));
-	}
+	/* Find the currently set OPP if we don't know already */
+	if (unlikely(!opp_table->current_opp))
+		_find_current_opp(dev, opp_table);
 
 	temp_freq = freq;
 	opp = _find_freq_ceil(opp_table, &temp_freq);
@@ -1039,7 +1047,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		ret = PTR_ERR(opp);
 		dev_err(dev, "%s: failed to find OPP for freq %lu (%d)\n",
 			__func__, freq, ret);
-		goto put_old_opp;
+		goto put_opp_table;
+	}
+
+	old_opp = opp_table->current_opp;
+	old_freq = old_opp->rate;
+
+	/* Return early if nothing to do */
+	if (opp_table->enabled && old_opp == opp) {
+		dev_dbg(dev, "%s: OPPs are same, nothing to do\n", __func__);
+		ret = 0;
+		goto put_opp;
 	}
 
 	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__,
@@ -1054,11 +1072,10 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	if (opp_table->set_opp) {
 		ret = _set_opp_custom(opp_table, dev, old_freq, freq,
-				      IS_ERR(old_opp) ? NULL : old_opp->supplies,
-				      opp->supplies);
+				      old_opp->supplies, opp->supplies);
 	} else if (opp_table->regulators) {
 		ret = _generic_set_opp_regulator(opp_table, dev, old_freq, freq,
-						 IS_ERR(old_opp) ? NULL : old_opp->supplies,
+						 old_opp->supplies,
 						 opp->supplies);
 	} else {
 		/* Only frequency scaling */
@@ -1074,15 +1091,18 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	if (!ret) {
 		ret = _set_opp_bw(opp_table, opp, dev, false);
-		if (!ret)
+		if (!ret) {
 			opp_table->enabled = true;
+			dev_pm_opp_put(old_opp);
+
+			/* Make sure current_opp doesn't get freed */
+			dev_pm_opp_get(opp);
+			opp_table->current_opp = opp;
+		}
 	}
 
 put_opp:
 	dev_pm_opp_put(opp);
-put_old_opp:
-	if (!IS_ERR(old_opp))
-		dev_pm_opp_put(old_opp);
 put_opp_table:
 	dev_pm_opp_put_opp_table(opp_table);
 	return ret;
@@ -1276,6 +1296,9 @@ static void _opp_table_kref_release(struct kref *kref)
 	list_del(&opp_table->node);
 	mutex_unlock(&opp_table_lock);
 
+	if (opp_table->current_opp)
+		dev_pm_opp_put(opp_table->current_opp);
+
 	_of_clear_opp_table(opp_table);
 
 	/* Release clk */
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 4408cfcb0f31..359fd89d5770 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -135,6 +135,7 @@ enum opp_table_access {
  * @clock_latency_ns_max: Max clock latency in nanoseconds.
  * @parsed_static_opps: Count of devices for which OPPs are initialized from DT.
  * @shared_opp: OPP is shared between multiple devices.
+ * @current_opp: Currently configured OPP for the table.
  * @suspend_opp: Pointer to OPP to be used during device suspend.
  * @genpd_virt_dev_lock: Mutex protecting the genpd virtual device pointers.
  * @genpd_virt_devs: List of virtual devices for multiple genpd support.
@@ -183,6 +184,7 @@ struct opp_table {
 
 	unsigned int parsed_static_opps;
 	enum opp_table_access shared_opp;
+	struct dev_pm_opp *current_opp;
 	struct dev_pm_opp *suspend_opp;
 
 	struct mutex genpd_virt_dev_lock;
-- 
2.25.0.rc1.19.g042ed3e048af


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 04/13] opp: Split _set_opp() out of dev_pm_opp_set_rate()
  2021-01-21 11:17 ` Viresh Kumar
@ 2021-01-21 11:17   ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki,
	Sibi Sankar, linux-kernel, linux-arm-kernel

The _set_opp() helper will be used for devices which don't change their
frequency (like power domains, etc.) later on, prepare for that by
breaking the generic part out of dev_pm_opp_set_rate().

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 126 +++++++++++++++++++++++++--------------------
 1 file changed, 71 insertions(+), 55 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 4ee598344e6a..5313dc322bdd 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -992,72 +992,27 @@ static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
 	return ret;
 }
 
-/**
- * dev_pm_opp_set_rate() - Configure new OPP based on frequency
- * @dev:	 device for which we do this operation
- * @target_freq: frequency to achieve
- *
- * This configures the power-supplies to the levels specified by the OPP
- * corresponding to the target_freq, and programs the clock to a value <=
- * target_freq, as rounded by clk_round_rate(). Device wanting to run at fmax
- * provided by the opp, should have already rounded to the target OPP's
- * frequency.
- */
-int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
+static int _set_opp(struct device *dev, struct opp_table *opp_table,
+		    struct dev_pm_opp *opp, unsigned long freq)
 {
-	struct opp_table *opp_table;
-	unsigned long freq, old_freq, temp_freq;
-	struct dev_pm_opp *old_opp, *opp;
+	struct dev_pm_opp *old_opp;
+	unsigned long old_freq;
 	int ret;
 
-	opp_table = _find_opp_table(dev);
-	if (IS_ERR(opp_table)) {
-		dev_err(dev, "%s: device opp doesn't exist\n", __func__);
-		return PTR_ERR(opp_table);
-	}
-
-	if (unlikely(!target_freq)) {
-		ret = _disable_opp_table(dev, opp_table);
-		goto put_opp_table;
-	}
-
-	freq = clk_round_rate(opp_table->clk, target_freq);
-	if ((long)freq <= 0)
-		freq = target_freq;
-
-	/*
-	 * For IO devices which require an OPP on some platforms/SoCs
-	 * while just needing to scale the clock on some others
-	 * we look for empty OPP tables with just a clock handle and
-	 * scale only the clk. This makes dev_pm_opp_set_rate()
-	 * equivalent to a clk_set_rate()
-	 */
-	if (!_get_opp_count(opp_table)) {
-		ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
-		goto put_opp_table;
-	}
+	if (unlikely(!opp))
+		return _disable_opp_table(dev, opp_table);
 
 	/* Find the currently set OPP if we don't know already */
 	if (unlikely(!opp_table->current_opp))
 		_find_current_opp(dev, opp_table);
 
-	temp_freq = freq;
-	opp = _find_freq_ceil(opp_table, &temp_freq);
-	if (IS_ERR(opp)) {
-		ret = PTR_ERR(opp);
-		dev_err(dev, "%s: failed to find OPP for freq %lu (%d)\n",
-			__func__, freq, ret);
-		goto put_opp_table;
-	}
-
 	old_opp = opp_table->current_opp;
 	old_freq = old_opp->rate;
 
 	/* Return early if nothing to do */
 	if (opp_table->enabled && old_opp == opp) {
 		dev_dbg(dev, "%s: OPPs are same, nothing to do\n", __func__);
-		ret = 0;
-		goto put_opp;
+		return 0;
 	}
 
 	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__,
@@ -1067,7 +1022,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	if (freq >= old_freq) {
 		ret = _set_required_opps(dev, opp_table, opp, true);
 		if (ret)
-			goto put_opp;
+			return ret;
 	}
 
 	if (opp_table->set_opp) {
@@ -1101,8 +1056,69 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		}
 	}
 
-put_opp:
-	dev_pm_opp_put(opp);
+	return ret;
+}
+
+/**
+ * dev_pm_opp_set_rate() - Configure new OPP based on frequency
+ * @dev:	 device for which we do this operation
+ * @target_freq: frequency to achieve
+ *
+ * This configures the power-supplies to the levels specified by the OPP
+ * corresponding to the target_freq, and programs the clock to a value <=
+ * target_freq, as rounded by clk_round_rate(). Device wanting to run at fmax
+ * provided by the opp, should have already rounded to the target OPP's
+ * frequency.
+ */
+int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
+{
+	struct opp_table *opp_table;
+	unsigned long freq = 0, temp_freq;
+	struct dev_pm_opp *opp = NULL;
+	int ret;
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table)) {
+		dev_err(dev, "%s: device's opp table doesn't exist\n", __func__);
+		return PTR_ERR(opp_table);
+	}
+
+	if (target_freq) {
+		/*
+		 * For IO devices which require an OPP on some platforms/SoCs
+		 * while just needing to scale the clock on some others
+		 * we look for empty OPP tables with just a clock handle and
+		 * scale only the clk. This makes dev_pm_opp_set_rate()
+		 * equivalent to a clk_set_rate()
+		 */
+		if (!_get_opp_count(opp_table)) {
+			ret = _generic_set_opp_clk_only(dev, opp_table->clk, target_freq);
+			goto put_opp_table;
+		}
+
+		freq = clk_round_rate(opp_table->clk, target_freq);
+		if ((long)freq <= 0)
+			freq = target_freq;
+
+		/*
+		 * The clock driver may support finer resolution of the
+		 * frequencies than the OPP table, don't update the frequency we
+		 * pass to clk_set_rate() here.
+		 */
+		temp_freq = freq;
+		opp = _find_freq_ceil(opp_table, &temp_freq);
+		if (IS_ERR(opp)) {
+			ret = PTR_ERR(opp);
+			dev_err(dev, "%s: failed to find OPP for freq %lu (%d)\n",
+				__func__, freq, ret);
+			goto put_opp_table;
+		}
+	}
+
+	ret = _set_opp(dev, opp_table, opp, freq);
+
+	if (opp)
+		dev_pm_opp_put(opp);
 put_opp_table:
 	dev_pm_opp_put_opp_table(opp_table);
 	return ret;
-- 
2.25.0.rc1.19.g042ed3e048af


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 04/13] opp: Split _set_opp() out of dev_pm_opp_set_rate()
@ 2021-01-21 11:17   ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Vincent Guittot, linux-pm, Viresh Kumar, Rafael Wysocki,
	linux-kernel, Sibi Sankar, linux-arm-kernel

The _set_opp() helper will be used for devices which don't change their
frequency (like power domains, etc.) later on, prepare for that by
breaking the generic part out of dev_pm_opp_set_rate().

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 126 +++++++++++++++++++++++++--------------------
 1 file changed, 71 insertions(+), 55 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 4ee598344e6a..5313dc322bdd 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -992,72 +992,27 @@ static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
 	return ret;
 }
 
-/**
- * dev_pm_opp_set_rate() - Configure new OPP based on frequency
- * @dev:	 device for which we do this operation
- * @target_freq: frequency to achieve
- *
- * This configures the power-supplies to the levels specified by the OPP
- * corresponding to the target_freq, and programs the clock to a value <=
- * target_freq, as rounded by clk_round_rate(). Device wanting to run at fmax
- * provided by the opp, should have already rounded to the target OPP's
- * frequency.
- */
-int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
+static int _set_opp(struct device *dev, struct opp_table *opp_table,
+		    struct dev_pm_opp *opp, unsigned long freq)
 {
-	struct opp_table *opp_table;
-	unsigned long freq, old_freq, temp_freq;
-	struct dev_pm_opp *old_opp, *opp;
+	struct dev_pm_opp *old_opp;
+	unsigned long old_freq;
 	int ret;
 
-	opp_table = _find_opp_table(dev);
-	if (IS_ERR(opp_table)) {
-		dev_err(dev, "%s: device opp doesn't exist\n", __func__);
-		return PTR_ERR(opp_table);
-	}
-
-	if (unlikely(!target_freq)) {
-		ret = _disable_opp_table(dev, opp_table);
-		goto put_opp_table;
-	}
-
-	freq = clk_round_rate(opp_table->clk, target_freq);
-	if ((long)freq <= 0)
-		freq = target_freq;
-
-	/*
-	 * For IO devices which require an OPP on some platforms/SoCs
-	 * while just needing to scale the clock on some others
-	 * we look for empty OPP tables with just a clock handle and
-	 * scale only the clk. This makes dev_pm_opp_set_rate()
-	 * equivalent to a clk_set_rate()
-	 */
-	if (!_get_opp_count(opp_table)) {
-		ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
-		goto put_opp_table;
-	}
+	if (unlikely(!opp))
+		return _disable_opp_table(dev, opp_table);
 
 	/* Find the currently set OPP if we don't know already */
 	if (unlikely(!opp_table->current_opp))
 		_find_current_opp(dev, opp_table);
 
-	temp_freq = freq;
-	opp = _find_freq_ceil(opp_table, &temp_freq);
-	if (IS_ERR(opp)) {
-		ret = PTR_ERR(opp);
-		dev_err(dev, "%s: failed to find OPP for freq %lu (%d)\n",
-			__func__, freq, ret);
-		goto put_opp_table;
-	}
-
 	old_opp = opp_table->current_opp;
 	old_freq = old_opp->rate;
 
 	/* Return early if nothing to do */
 	if (opp_table->enabled && old_opp == opp) {
 		dev_dbg(dev, "%s: OPPs are same, nothing to do\n", __func__);
-		ret = 0;
-		goto put_opp;
+		return 0;
 	}
 
 	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__,
@@ -1067,7 +1022,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	if (freq >= old_freq) {
 		ret = _set_required_opps(dev, opp_table, opp, true);
 		if (ret)
-			goto put_opp;
+			return ret;
 	}
 
 	if (opp_table->set_opp) {
@@ -1101,8 +1056,69 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		}
 	}
 
-put_opp:
-	dev_pm_opp_put(opp);
+	return ret;
+}
+
+/**
+ * dev_pm_opp_set_rate() - Configure new OPP based on frequency
+ * @dev:	 device for which we do this operation
+ * @target_freq: frequency to achieve
+ *
+ * This configures the power-supplies to the levels specified by the OPP
+ * corresponding to the target_freq, and programs the clock to a value <=
+ * target_freq, as rounded by clk_round_rate(). Device wanting to run at fmax
+ * provided by the opp, should have already rounded to the target OPP's
+ * frequency.
+ */
+int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
+{
+	struct opp_table *opp_table;
+	unsigned long freq = 0, temp_freq;
+	struct dev_pm_opp *opp = NULL;
+	int ret;
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table)) {
+		dev_err(dev, "%s: device's opp table doesn't exist\n", __func__);
+		return PTR_ERR(opp_table);
+	}
+
+	if (target_freq) {
+		/*
+		 * For IO devices which require an OPP on some platforms/SoCs
+		 * while just needing to scale the clock on some others
+		 * we look for empty OPP tables with just a clock handle and
+		 * scale only the clk. This makes dev_pm_opp_set_rate()
+		 * equivalent to a clk_set_rate()
+		 */
+		if (!_get_opp_count(opp_table)) {
+			ret = _generic_set_opp_clk_only(dev, opp_table->clk, target_freq);
+			goto put_opp_table;
+		}
+
+		freq = clk_round_rate(opp_table->clk, target_freq);
+		if ((long)freq <= 0)
+			freq = target_freq;
+
+		/*
+		 * The clock driver may support finer resolution of the
+		 * frequencies than the OPP table, don't update the frequency we
+		 * pass to clk_set_rate() here.
+		 */
+		temp_freq = freq;
+		opp = _find_freq_ceil(opp_table, &temp_freq);
+		if (IS_ERR(opp)) {
+			ret = PTR_ERR(opp);
+			dev_err(dev, "%s: failed to find OPP for freq %lu (%d)\n",
+				__func__, freq, ret);
+			goto put_opp_table;
+		}
+	}
+
+	ret = _set_opp(dev, opp_table, opp, freq);
+
+	if (opp)
+		dev_pm_opp_put(opp);
 put_opp_table:
 	dev_pm_opp_put_opp_table(opp_table);
 	return ret;
-- 
2.25.0.rc1.19.g042ed3e048af


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 05/13] opp: Allow _set_opp() to work for non-freq devices
  2021-01-21 11:17 ` Viresh Kumar
@ 2021-01-21 11:17   ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki,
	Sibi Sankar, linux-kernel, linux-arm-kernel

The _set_opp() helper will be used for devices which don't change frequency
(like power domains, etc.) later on, prepare for that by not relying on
frequency for making decisions here.

While at it, also update the debug print to contain all relevant
information.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 5313dc322bdd..64424dbd23c1 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -997,7 +997,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 {
 	struct dev_pm_opp *old_opp;
 	unsigned long old_freq;
-	int ret;
+	int scaling_down, ret;
 
 	if (unlikely(!opp))
 		return _disable_opp_table(dev, opp_table);
@@ -1015,11 +1015,17 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 		return 0;
 	}
 
-	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__,
-		old_freq, freq);
+	dev_dbg(dev, "%s: switching OPP: Freq %lu -> %lu Hz, Level %u -> %u, Bw %u -> %u\n",
+		__func__, old_freq, freq, old_opp->level, opp->level,
+		old_opp->bandwidth ? old_opp->bandwidth[0].peak : 0,
+		opp->bandwidth ? opp->bandwidth[0].peak : 0);
+
+	scaling_down = _opp_compare_key(old_opp, opp);
+	if (scaling_down == -1)
+		scaling_down = 0;
 
 	/* Scaling up? Configure required OPPs before frequency */
-	if (freq >= old_freq) {
+	if (!scaling_down) {
 		ret = _set_required_opps(dev, opp_table, opp, true);
 		if (ret)
 			return ret;
@@ -1038,7 +1044,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 	}
 
 	/* Scaling down? Configure required OPPs after frequency */
-	if (!ret && freq < old_freq) {
+	if (!ret && scaling_down) {
 		ret = _set_required_opps(dev, opp_table, opp, false);
 		if (ret)
 			dev_err(dev, "Failed to set required opps: %d\n", ret);
-- 
2.25.0.rc1.19.g042ed3e048af


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 05/13] opp: Allow _set_opp() to work for non-freq devices
@ 2021-01-21 11:17   ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Vincent Guittot, linux-pm, Viresh Kumar, Rafael Wysocki,
	linux-kernel, Sibi Sankar, linux-arm-kernel

The _set_opp() helper will be used for devices which don't change frequency
(like power domains, etc.) later on, prepare for that by not relying on
frequency for making decisions here.

While at it, also update the debug print to contain all relevant
information.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 5313dc322bdd..64424dbd23c1 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -997,7 +997,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 {
 	struct dev_pm_opp *old_opp;
 	unsigned long old_freq;
-	int ret;
+	int scaling_down, ret;
 
 	if (unlikely(!opp))
 		return _disable_opp_table(dev, opp_table);
@@ -1015,11 +1015,17 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 		return 0;
 	}
 
-	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__,
-		old_freq, freq);
+	dev_dbg(dev, "%s: switching OPP: Freq %lu -> %lu Hz, Level %u -> %u, Bw %u -> %u\n",
+		__func__, old_freq, freq, old_opp->level, opp->level,
+		old_opp->bandwidth ? old_opp->bandwidth[0].peak : 0,
+		opp->bandwidth ? opp->bandwidth[0].peak : 0);
+
+	scaling_down = _opp_compare_key(old_opp, opp);
+	if (scaling_down == -1)
+		scaling_down = 0;
 
 	/* Scaling up? Configure required OPPs before frequency */
-	if (freq >= old_freq) {
+	if (!scaling_down) {
 		ret = _set_required_opps(dev, opp_table, opp, true);
 		if (ret)
 			return ret;
@@ -1038,7 +1044,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 	}
 
 	/* Scaling down? Configure required OPPs after frequency */
-	if (!ret && freq < old_freq) {
+	if (!ret && scaling_down) {
 		ret = _set_required_opps(dev, opp_table, opp, false);
 		if (ret)
 			dev_err(dev, "Failed to set required opps: %d\n", ret);
-- 
2.25.0.rc1.19.g042ed3e048af


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 06/13] opp: Allow _generic_set_opp_regulator() to work for non-freq devices
  2021-01-21 11:17 ` Viresh Kumar
@ 2021-01-21 11:17   ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki,
	Sibi Sankar, linux-kernel, linux-arm-kernel

The _generic_set_opp_regulator() helper will be used for devices which
don't change frequency (like power domains, etc.) later on, prepare for
that by not relying on frequency for making decisions here.

While at it, update its parameters to pass only what is necessary.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 64424dbd23c1..a96ffd9051b1 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -737,12 +737,12 @@ static inline int _generic_set_opp_clk_only(struct device *dev, struct clk *clk,
 
 static int _generic_set_opp_regulator(struct opp_table *opp_table,
 				      struct device *dev,
-				      unsigned long old_freq,
+				      struct dev_pm_opp *opp,
 				      unsigned long freq,
-				      struct dev_pm_opp_supply *old_supply,
-				      struct dev_pm_opp_supply *new_supply)
+				      int scaling_down)
 {
 	struct regulator *reg = opp_table->regulators[0];
+	struct dev_pm_opp *old_opp = opp_table->current_opp;
 	int ret;
 
 	/* This function only supports single regulator per device */
@@ -752,8 +752,8 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
 	}
 
 	/* Scaling up? Scale voltage before frequency */
-	if (freq >= old_freq) {
-		ret = _set_opp_voltage(dev, reg, new_supply);
+	if (!scaling_down) {
+		ret = _set_opp_voltage(dev, reg, opp->supplies);
 		if (ret)
 			goto restore_voltage;
 	}
@@ -764,8 +764,8 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
 		goto restore_voltage;
 
 	/* Scaling down? Scale voltage after frequency */
-	if (freq < old_freq) {
-		ret = _set_opp_voltage(dev, reg, new_supply);
+	if (scaling_down) {
+		ret = _set_opp_voltage(dev, reg, opp->supplies);
 		if (ret)
 			goto restore_freq;
 	}
@@ -783,12 +783,12 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
 	return 0;
 
 restore_freq:
-	if (_generic_set_opp_clk_only(dev, opp_table->clk, old_freq))
+	if (_generic_set_opp_clk_only(dev, opp_table->clk, old_opp->rate))
 		dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
-			__func__, old_freq);
+			__func__, old_opp->rate);
 restore_voltage:
 	/* This shouldn't harm even if the voltages weren't updated earlier */
-	_set_opp_voltage(dev, reg, old_supply);
+	_set_opp_voltage(dev, reg, old_opp->supplies);
 
 	return ret;
 }
@@ -1035,9 +1035,8 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 		ret = _set_opp_custom(opp_table, dev, old_freq, freq,
 				      old_opp->supplies, opp->supplies);
 	} else if (opp_table->regulators) {
-		ret = _generic_set_opp_regulator(opp_table, dev, old_freq, freq,
-						 old_opp->supplies,
-						 opp->supplies);
+		ret = _generic_set_opp_regulator(opp_table, dev, opp, freq,
+						 scaling_down);
 	} else {
 		/* Only frequency scaling */
 		ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
-- 
2.25.0.rc1.19.g042ed3e048af


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 06/13] opp: Allow _generic_set_opp_regulator() to work for non-freq devices
@ 2021-01-21 11:17   ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Vincent Guittot, linux-pm, Viresh Kumar, Rafael Wysocki,
	linux-kernel, Sibi Sankar, linux-arm-kernel

The _generic_set_opp_regulator() helper will be used for devices which
don't change frequency (like power domains, etc.) later on, prepare for
that by not relying on frequency for making decisions here.

While at it, update its parameters to pass only what is necessary.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 64424dbd23c1..a96ffd9051b1 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -737,12 +737,12 @@ static inline int _generic_set_opp_clk_only(struct device *dev, struct clk *clk,
 
 static int _generic_set_opp_regulator(struct opp_table *opp_table,
 				      struct device *dev,
-				      unsigned long old_freq,
+				      struct dev_pm_opp *opp,
 				      unsigned long freq,
-				      struct dev_pm_opp_supply *old_supply,
-				      struct dev_pm_opp_supply *new_supply)
+				      int scaling_down)
 {
 	struct regulator *reg = opp_table->regulators[0];
+	struct dev_pm_opp *old_opp = opp_table->current_opp;
 	int ret;
 
 	/* This function only supports single regulator per device */
@@ -752,8 +752,8 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
 	}
 
 	/* Scaling up? Scale voltage before frequency */
-	if (freq >= old_freq) {
-		ret = _set_opp_voltage(dev, reg, new_supply);
+	if (!scaling_down) {
+		ret = _set_opp_voltage(dev, reg, opp->supplies);
 		if (ret)
 			goto restore_voltage;
 	}
@@ -764,8 +764,8 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
 		goto restore_voltage;
 
 	/* Scaling down? Scale voltage after frequency */
-	if (freq < old_freq) {
-		ret = _set_opp_voltage(dev, reg, new_supply);
+	if (scaling_down) {
+		ret = _set_opp_voltage(dev, reg, opp->supplies);
 		if (ret)
 			goto restore_freq;
 	}
@@ -783,12 +783,12 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
 	return 0;
 
 restore_freq:
-	if (_generic_set_opp_clk_only(dev, opp_table->clk, old_freq))
+	if (_generic_set_opp_clk_only(dev, opp_table->clk, old_opp->rate))
 		dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
-			__func__, old_freq);
+			__func__, old_opp->rate);
 restore_voltage:
 	/* This shouldn't harm even if the voltages weren't updated earlier */
-	_set_opp_voltage(dev, reg, old_supply);
+	_set_opp_voltage(dev, reg, old_opp->supplies);
 
 	return ret;
 }
@@ -1035,9 +1035,8 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 		ret = _set_opp_custom(opp_table, dev, old_freq, freq,
 				      old_opp->supplies, opp->supplies);
 	} else if (opp_table->regulators) {
-		ret = _generic_set_opp_regulator(opp_table, dev, old_freq, freq,
-						 old_opp->supplies,
-						 opp->supplies);
+		ret = _generic_set_opp_regulator(opp_table, dev, opp, freq,
+						 scaling_down);
 	} else {
 		/* Only frequency scaling */
 		ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
-- 
2.25.0.rc1.19.g042ed3e048af


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() to work for non-freq devices
  2021-01-21 11:17 ` Viresh Kumar
@ 2021-01-21 11:17   ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki,
	Sibi Sankar, linux-kernel, linux-arm-kernel

In order to avoid conditional statements at the caller site, this patch
updates _generic_set_opp_clk_only() to work for devices that don't
change frequency (like power domains, etc.). Return 0 if the clk pointer
passed to this routine is not valid.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index a96ffd9051b1..6b09d468d37a 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -726,6 +726,10 @@ static inline int _generic_set_opp_clk_only(struct device *dev, struct clk *clk,
 {
 	int ret;
 
+	/* We may reach here for devices which don't change frequency */
+	if (unlikely(!clk))
+		return 0;
+
 	ret = clk_set_rate(clk, freq);
 	if (ret) {
 		dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
-- 
2.25.0.rc1.19.g042ed3e048af


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() to work for non-freq devices
@ 2021-01-21 11:17   ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Vincent Guittot, linux-pm, Viresh Kumar, Rafael Wysocki,
	linux-kernel, Sibi Sankar, linux-arm-kernel

In order to avoid conditional statements at the caller site, this patch
updates _generic_set_opp_clk_only() to work for devices that don't
change frequency (like power domains, etc.). Return 0 if the clk pointer
passed to this routine is not valid.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index a96ffd9051b1..6b09d468d37a 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -726,6 +726,10 @@ static inline int _generic_set_opp_clk_only(struct device *dev, struct clk *clk,
 {
 	int ret;
 
+	/* We may reach here for devices which don't change frequency */
+	if (unlikely(!clk))
+		return 0;
+
 	ret = clk_set_rate(clk, freq);
 	if (ret) {
 		dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
-- 
2.25.0.rc1.19.g042ed3e048af


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 08/13] opp: Update parameters of  _set_opp_custom()
  2021-01-21 11:17 ` Viresh Kumar
@ 2021-01-21 11:17   ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki,
	Sibi Sankar, linux-kernel, linux-arm-kernel

Drop the unnecessary parameters and follow the pattern from
_generic_set_opp_regulator().

While at it, also remove the local variable old_freq.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 6b09d468d37a..3500cc9de66b 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -826,11 +826,10 @@ static int _set_opp_bw(const struct opp_table *opp_table,
 }
 
 static int _set_opp_custom(const struct opp_table *opp_table,
-			   struct device *dev, unsigned long old_freq,
-			   unsigned long freq,
-			   struct dev_pm_opp_supply *old_supply,
-			   struct dev_pm_opp_supply *new_supply)
+			   struct device *dev, struct dev_pm_opp *opp,
+			   unsigned long freq)
 {
+	struct dev_pm_opp *old_opp = opp_table->current_opp;
 	struct dev_pm_set_opp_data *data;
 	int size;
 
@@ -840,12 +839,12 @@ static int _set_opp_custom(const struct opp_table *opp_table,
 	data->clk = opp_table->clk;
 	data->dev = dev;
 
-	data->old_opp.rate = old_freq;
-	size = sizeof(*old_supply) * opp_table->regulator_count;
-	memcpy(data->old_opp.supplies, old_supply, size);
+	data->old_opp.rate = old_opp->rate;
+	size = sizeof(*old_opp->supplies) * opp_table->regulator_count;
+	memcpy(data->old_opp.supplies, old_opp->supplies, size);
 
 	data->new_opp.rate = freq;
-	memcpy(data->new_opp.supplies, new_supply, size);
+	memcpy(data->new_opp.supplies, opp->supplies, size);
 
 	return opp_table->set_opp(data);
 }
@@ -1000,7 +999,6 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 		    struct dev_pm_opp *opp, unsigned long freq)
 {
 	struct dev_pm_opp *old_opp;
-	unsigned long old_freq;
 	int scaling_down, ret;
 
 	if (unlikely(!opp))
@@ -1011,7 +1009,6 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 		_find_current_opp(dev, opp_table);
 
 	old_opp = opp_table->current_opp;
-	old_freq = old_opp->rate;
 
 	/* Return early if nothing to do */
 	if (opp_table->enabled && old_opp == opp) {
@@ -1020,7 +1017,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 	}
 
 	dev_dbg(dev, "%s: switching OPP: Freq %lu -> %lu Hz, Level %u -> %u, Bw %u -> %u\n",
-		__func__, old_freq, freq, old_opp->level, opp->level,
+		__func__, old_opp->rate, freq, old_opp->level, opp->level,
 		old_opp->bandwidth ? old_opp->bandwidth[0].peak : 0,
 		opp->bandwidth ? opp->bandwidth[0].peak : 0);
 
@@ -1036,8 +1033,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 	}
 
 	if (opp_table->set_opp) {
-		ret = _set_opp_custom(opp_table, dev, old_freq, freq,
-				      old_opp->supplies, opp->supplies);
+		ret = _set_opp_custom(opp_table, dev, opp, freq);
 	} else if (opp_table->regulators) {
 		ret = _generic_set_opp_regulator(opp_table, dev, opp, freq,
 						 scaling_down);
-- 
2.25.0.rc1.19.g042ed3e048af


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 08/13] opp: Update parameters of  _set_opp_custom()
@ 2021-01-21 11:17   ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Vincent Guittot, linux-pm, Viresh Kumar, Rafael Wysocki,
	linux-kernel, Sibi Sankar, linux-arm-kernel

Drop the unnecessary parameters and follow the pattern from
_generic_set_opp_regulator().

While at it, also remove the local variable old_freq.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 6b09d468d37a..3500cc9de66b 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -826,11 +826,10 @@ static int _set_opp_bw(const struct opp_table *opp_table,
 }
 
 static int _set_opp_custom(const struct opp_table *opp_table,
-			   struct device *dev, unsigned long old_freq,
-			   unsigned long freq,
-			   struct dev_pm_opp_supply *old_supply,
-			   struct dev_pm_opp_supply *new_supply)
+			   struct device *dev, struct dev_pm_opp *opp,
+			   unsigned long freq)
 {
+	struct dev_pm_opp *old_opp = opp_table->current_opp;
 	struct dev_pm_set_opp_data *data;
 	int size;
 
@@ -840,12 +839,12 @@ static int _set_opp_custom(const struct opp_table *opp_table,
 	data->clk = opp_table->clk;
 	data->dev = dev;
 
-	data->old_opp.rate = old_freq;
-	size = sizeof(*old_supply) * opp_table->regulator_count;
-	memcpy(data->old_opp.supplies, old_supply, size);
+	data->old_opp.rate = old_opp->rate;
+	size = sizeof(*old_opp->supplies) * opp_table->regulator_count;
+	memcpy(data->old_opp.supplies, old_opp->supplies, size);
 
 	data->new_opp.rate = freq;
-	memcpy(data->new_opp.supplies, new_supply, size);
+	memcpy(data->new_opp.supplies, opp->supplies, size);
 
 	return opp_table->set_opp(data);
 }
@@ -1000,7 +999,6 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 		    struct dev_pm_opp *opp, unsigned long freq)
 {
 	struct dev_pm_opp *old_opp;
-	unsigned long old_freq;
 	int scaling_down, ret;
 
 	if (unlikely(!opp))
@@ -1011,7 +1009,6 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 		_find_current_opp(dev, opp_table);
 
 	old_opp = opp_table->current_opp;
-	old_freq = old_opp->rate;
 
 	/* Return early if nothing to do */
 	if (opp_table->enabled && old_opp == opp) {
@@ -1020,7 +1017,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 	}
 
 	dev_dbg(dev, "%s: switching OPP: Freq %lu -> %lu Hz, Level %u -> %u, Bw %u -> %u\n",
-		__func__, old_freq, freq, old_opp->level, opp->level,
+		__func__, old_opp->rate, freq, old_opp->level, opp->level,
 		old_opp->bandwidth ? old_opp->bandwidth[0].peak : 0,
 		opp->bandwidth ? opp->bandwidth[0].peak : 0);
 
@@ -1036,8 +1033,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 	}
 
 	if (opp_table->set_opp) {
-		ret = _set_opp_custom(opp_table, dev, old_freq, freq,
-				      old_opp->supplies, opp->supplies);
+		ret = _set_opp_custom(opp_table, dev, opp, freq);
 	} else if (opp_table->regulators) {
 		ret = _generic_set_opp_regulator(opp_table, dev, opp, freq,
 						 scaling_down);
-- 
2.25.0.rc1.19.g042ed3e048af


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 09/13] opp: Implement dev_pm_opp_set_opp()
  2021-01-21 11:17 ` Viresh Kumar
@ 2021-01-21 11:17   ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd,
	Rafael J. Wysocki
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Sibi Sankar,
	linux-kernel, linux-arm-kernel

The new helper dev_pm_opp_set_opp() can be used for configuring the
devices for a particular OPP and can be used by different type of
devices, even the ones which don't change frequency (like power
domains).

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c     | 28 ++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  6 ++++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 3500cc9de66b..5a367ef02b92 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1130,6 +1130,34 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
 
+/**
+ * dev_pm_opp_set_opp() - Configure device for OPP
+ * @dev: device for which we do this operation
+ * @opp: OPP to set to
+ *
+ * This configures the device based on the properties of the OPP passed to this
+ * routine.
+ *
+ * Return: 0 on success, a negative error number otherwise.
+ */
+int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp)
+{
+	struct opp_table *opp_table;
+	int ret;
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table)) {
+		dev_err(dev, "%s: device opp doesn't exist\n", __func__);
+		return PTR_ERR(opp_table);
+	}
+
+	ret = _set_opp(dev, opp_table, opp, opp->rate);
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_opp);
+
 /* OPP-dev Helpers */
 static void _remove_opp_dev(struct opp_device *opp_dev,
 			    struct opp_table *opp_table)
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index f8e9a8e3eb59..2d8a706f8d00 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -158,6 +158,7 @@ void dev_pm_opp_detach_genpd(struct opp_table *opp_table);
 struct opp_table *devm_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs);
 int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
+int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp);
 int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp);
 int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
 int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
@@ -376,6 +377,11 @@ static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_f
 	return -ENOTSUPP;
 }
 
+static inline int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp)
+{
+	return -ENOTSUPP;
+}
+
 static inline int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp)
 {
 	return -EOPNOTSUPP;
-- 
2.25.0.rc1.19.g042ed3e048af


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 09/13] opp: Implement dev_pm_opp_set_opp()
@ 2021-01-21 11:17   ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd,
	Rafael J. Wysocki
  Cc: Vincent Guittot, linux-pm, Viresh Kumar, linux-kernel,
	Sibi Sankar, linux-arm-kernel

The new helper dev_pm_opp_set_opp() can be used for configuring the
devices for a particular OPP and can be used by different type of
devices, even the ones which don't change frequency (like power
domains).

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c     | 28 ++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  6 ++++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 3500cc9de66b..5a367ef02b92 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1130,6 +1130,34 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
 
+/**
+ * dev_pm_opp_set_opp() - Configure device for OPP
+ * @dev: device for which we do this operation
+ * @opp: OPP to set to
+ *
+ * This configures the device based on the properties of the OPP passed to this
+ * routine.
+ *
+ * Return: 0 on success, a negative error number otherwise.
+ */
+int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp)
+{
+	struct opp_table *opp_table;
+	int ret;
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table)) {
+		dev_err(dev, "%s: device opp doesn't exist\n", __func__);
+		return PTR_ERR(opp_table);
+	}
+
+	ret = _set_opp(dev, opp_table, opp, opp->rate);
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_opp);
+
 /* OPP-dev Helpers */
 static void _remove_opp_dev(struct opp_device *opp_dev,
 			    struct opp_table *opp_table)
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index f8e9a8e3eb59..2d8a706f8d00 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -158,6 +158,7 @@ void dev_pm_opp_detach_genpd(struct opp_table *opp_table);
 struct opp_table *devm_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs);
 int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
+int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp);
 int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp);
 int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
 int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
@@ -376,6 +377,11 @@ static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_f
 	return -ENOTSUPP;
 }
 
+static inline int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp)
+{
+	return -ENOTSUPP;
+}
+
 static inline int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp)
 {
 	return -EOPNOTSUPP;
-- 
2.25.0.rc1.19.g042ed3e048af


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 10/13] cpufreq: qcom: Migrate to dev_pm_opp_set_opp()
  2021-01-21 11:17 ` Viresh Kumar
@ 2021-01-21 11:17   ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Andy Gross, Bjorn Andersson, Rafael J. Wysocki,
	Viresh Kumar
  Cc: linux-pm, Vincent Guittot, Stephen Boyd, Nishanth Menon,
	Sibi Sankar, linux-kernel, linux-arm-kernel, linux-arm-msm

dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
be used instead. Migrate to the new API.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-hw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 9ed5341dc515..7df18903b66c 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -54,7 +54,7 @@ static int qcom_cpufreq_set_bw(struct cpufreq_policy *policy,
 	if (IS_ERR(opp))
 		return PTR_ERR(opp);
 
-	ret = dev_pm_opp_set_bw(dev, opp);
+	ret = dev_pm_opp_set_opp(dev, opp);
 	dev_pm_opp_put(opp);
 	return ret;
 }
-- 
2.25.0.rc1.19.g042ed3e048af


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 10/13] cpufreq: qcom: Migrate to dev_pm_opp_set_opp()
@ 2021-01-21 11:17   ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Andy Gross, Bjorn Andersson, Rafael J. Wysocki,
	Viresh Kumar
  Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd,
	linux-arm-msm, linux-kernel, Sibi Sankar, linux-arm-kernel

dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
be used instead. Migrate to the new API.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-hw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 9ed5341dc515..7df18903b66c 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -54,7 +54,7 @@ static int qcom_cpufreq_set_bw(struct cpufreq_policy *policy,
 	if (IS_ERR(opp))
 		return PTR_ERR(opp);
 
-	ret = dev_pm_opp_set_bw(dev, opp);
+	ret = dev_pm_opp_set_opp(dev, opp);
 	dev_pm_opp_put(opp);
 	return ret;
 }
-- 
2.25.0.rc1.19.g042ed3e048af


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
  2021-01-21 11:17 ` Viresh Kumar
@ 2021-01-21 11:17   ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Thierry Reding, Jonathan Hunter
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki,
	Stephen Boyd, Nishanth Menon, Sibi Sankar, linux-kernel,
	linux-arm-kernel, linux-tegra

dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
be used instead. Migrate to the new API.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/devfreq/tegra30-devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 117cad7968ab..d2477d7d1f66 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
 		return PTR_ERR(opp);
 	}
 
-	ret = dev_pm_opp_set_bw(dev, opp);
+	ret = dev_pm_opp_set_opp(dev, opp);
 	dev_pm_opp_put(opp);
 
 	return ret;
-- 
2.25.0.rc1.19.g042ed3e048af


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
@ 2021-01-21 11:17   ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Thierry Reding, Jonathan Hunter
  Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd,
	Viresh Kumar, Rafael Wysocki, linux-kernel, Sibi Sankar,
	linux-tegra, linux-arm-kernel

dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
be used instead. Migrate to the new API.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/devfreq/tegra30-devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 117cad7968ab..d2477d7d1f66 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
 		return PTR_ERR(opp);
 	}
 
-	ret = dev_pm_opp_set_bw(dev, opp);
+	ret = dev_pm_opp_set_opp(dev, opp);
 	dev_pm_opp_put(opp);
 
 	return ret;
-- 
2.25.0.rc1.19.g042ed3e048af


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 12/13] drm: msm: Migrate to dev_pm_opp_set_opp()
  2021-01-21 11:17 ` Viresh Kumar
  (?)
@ 2021-01-21 11:17   ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Rob Clark, Sean Paul
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki,
	Stephen Boyd, Nishanth Menon, Sibi Sankar, linux-kernel,
	linux-arm-kernel, linux-arm-msm, dri-devel, freedreno

dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
be used instead. Migrate to the new API.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index e6703ae98760..05e0ef58fe32 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -134,7 +134,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
 
 	if (!gmu->legacy) {
 		a6xx_hfi_set_freq(gmu, perf_index);
-		dev_pm_opp_set_bw(&gpu->pdev->dev, opp);
+		dev_pm_opp_set_opp(&gpu->pdev->dev, opp);
 		pm_runtime_put(gmu->dev);
 		return;
 	}
@@ -158,7 +158,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
 	if (ret)
 		dev_err(gmu->dev, "GMU set GPU frequency error: %d\n", ret);
 
-	dev_pm_opp_set_bw(&gpu->pdev->dev, opp);
+	dev_pm_opp_set_opp(&gpu->pdev->dev, opp);
 	pm_runtime_put(gmu->dev);
 }
 
@@ -866,7 +866,7 @@ static void a6xx_gmu_set_initial_bw(struct msm_gpu *gpu, struct a6xx_gmu *gmu)
 	if (IS_ERR_OR_NULL(gpu_opp))
 		return;
 
-	dev_pm_opp_set_bw(&gpu->pdev->dev, gpu_opp);
+	dev_pm_opp_set_opp(&gpu->pdev->dev, gpu_opp);
 	dev_pm_opp_put(gpu_opp);
 }
 
@@ -1072,7 +1072,7 @@ int a6xx_gmu_stop(struct a6xx_gpu *a6xx_gpu)
 		a6xx_gmu_shutdown(gmu);
 
 	/* Remove the bus vote */
-	dev_pm_opp_set_bw(&gpu->pdev->dev, NULL);
+	dev_pm_opp_set_opp(&gpu->pdev->dev, NULL);
 
 	/*
 	 * Make sure the GX domain is off before turning off the GMU (CX)
-- 
2.25.0.rc1.19.g042ed3e048af


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 12/13] drm: msm: Migrate to dev_pm_opp_set_opp()
@ 2021-01-21 11:17   ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Rob Clark, Sean Paul
  Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd,
	Viresh Kumar, Rafael Wysocki, linux-kernel, dri-devel,
	Sibi Sankar, linux-arm-msm, freedreno, linux-arm-kernel

dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
be used instead. Migrate to the new API.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index e6703ae98760..05e0ef58fe32 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -134,7 +134,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
 
 	if (!gmu->legacy) {
 		a6xx_hfi_set_freq(gmu, perf_index);
-		dev_pm_opp_set_bw(&gpu->pdev->dev, opp);
+		dev_pm_opp_set_opp(&gpu->pdev->dev, opp);
 		pm_runtime_put(gmu->dev);
 		return;
 	}
@@ -158,7 +158,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
 	if (ret)
 		dev_err(gmu->dev, "GMU set GPU frequency error: %d\n", ret);
 
-	dev_pm_opp_set_bw(&gpu->pdev->dev, opp);
+	dev_pm_opp_set_opp(&gpu->pdev->dev, opp);
 	pm_runtime_put(gmu->dev);
 }
 
@@ -866,7 +866,7 @@ static void a6xx_gmu_set_initial_bw(struct msm_gpu *gpu, struct a6xx_gmu *gmu)
 	if (IS_ERR_OR_NULL(gpu_opp))
 		return;
 
-	dev_pm_opp_set_bw(&gpu->pdev->dev, gpu_opp);
+	dev_pm_opp_set_opp(&gpu->pdev->dev, gpu_opp);
 	dev_pm_opp_put(gpu_opp);
 }
 
@@ -1072,7 +1072,7 @@ int a6xx_gmu_stop(struct a6xx_gpu *a6xx_gpu)
 		a6xx_gmu_shutdown(gmu);
 
 	/* Remove the bus vote */
-	dev_pm_opp_set_bw(&gpu->pdev->dev, NULL);
+	dev_pm_opp_set_opp(&gpu->pdev->dev, NULL);
 
 	/*
 	 * Make sure the GX domain is off before turning off the GMU (CX)
-- 
2.25.0.rc1.19.g042ed3e048af


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 12/13] drm: msm: Migrate to dev_pm_opp_set_opp()
@ 2021-01-21 11:17   ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Rob Clark, Sean Paul
  Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd,
	Viresh Kumar, Rafael Wysocki, linux-kernel, dri-devel,
	Sibi Sankar, linux-arm-msm, freedreno, linux-arm-kernel

dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
be used instead. Migrate to the new API.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index e6703ae98760..05e0ef58fe32 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -134,7 +134,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
 
 	if (!gmu->legacy) {
 		a6xx_hfi_set_freq(gmu, perf_index);
-		dev_pm_opp_set_bw(&gpu->pdev->dev, opp);
+		dev_pm_opp_set_opp(&gpu->pdev->dev, opp);
 		pm_runtime_put(gmu->dev);
 		return;
 	}
@@ -158,7 +158,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
 	if (ret)
 		dev_err(gmu->dev, "GMU set GPU frequency error: %d\n", ret);
 
-	dev_pm_opp_set_bw(&gpu->pdev->dev, opp);
+	dev_pm_opp_set_opp(&gpu->pdev->dev, opp);
 	pm_runtime_put(gmu->dev);
 }
 
@@ -866,7 +866,7 @@ static void a6xx_gmu_set_initial_bw(struct msm_gpu *gpu, struct a6xx_gmu *gmu)
 	if (IS_ERR_OR_NULL(gpu_opp))
 		return;
 
-	dev_pm_opp_set_bw(&gpu->pdev->dev, gpu_opp);
+	dev_pm_opp_set_opp(&gpu->pdev->dev, gpu_opp);
 	dev_pm_opp_put(gpu_opp);
 }
 
@@ -1072,7 +1072,7 @@ int a6xx_gmu_stop(struct a6xx_gpu *a6xx_gpu)
 		a6xx_gmu_shutdown(gmu);
 
 	/* Remove the bus vote */
-	dev_pm_opp_set_bw(&gpu->pdev->dev, NULL);
+	dev_pm_opp_set_opp(&gpu->pdev->dev, NULL);
 
 	/*
 	 * Make sure the GX domain is off before turning off the GMU (CX)
-- 
2.25.0.rc1.19.g042ed3e048af

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 13/13] opp: Remove dev_pm_opp_set_bw()
  2021-01-21 11:17 ` Viresh Kumar
@ 2021-01-21 11:17   ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd,
	Rafael J. Wysocki
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Sibi Sankar,
	linux-kernel, linux-arm-kernel

All the users have migrated to dev_pm_opp_set_opp() now, get rid of the
duplicate API, dev_pm_opp_set_bw(), which only performs a part of the new API.

While at it, remove the unnecessary parameter to _set_opp_bw().

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c     | 41 +++++------------------------------------
 include/linux/pm_opp.h |  6 ------
 2 files changed, 5 insertions(+), 42 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 5a367ef02b92..d8ca15d96ce9 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -798,7 +798,7 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
 }
 
 static int _set_opp_bw(const struct opp_table *opp_table,
-		       struct dev_pm_opp *opp, struct device *dev, bool remove)
+		       struct dev_pm_opp *opp, struct device *dev)
 {
 	u32 avg, peak;
 	int i, ret;
@@ -807,7 +807,7 @@ static int _set_opp_bw(const struct opp_table *opp_table,
 		return 0;
 
 	for (i = 0; i < opp_table->path_count; i++) {
-		if (remove) {
+		if (!opp) {
 			avg = 0;
 			peak = 0;
 		} else {
@@ -817,7 +817,7 @@ static int _set_opp_bw(const struct opp_table *opp_table,
 		ret = icc_set_bw(opp_table->paths[i], avg, peak);
 		if (ret) {
 			dev_err(dev, "Failed to %s bandwidth[%d]: %d\n",
-				remove ? "remove" : "set", i, ret);
+				opp ? "set" : "remove", i, ret);
 			return ret;
 		}
 	}
@@ -911,37 +911,6 @@ static int _set_required_opps(struct device *dev,
 	return ret;
 }
 
-/**
- * dev_pm_opp_set_bw() - sets bandwidth levels corresponding to an opp
- * @dev:	device for which we do this operation
- * @opp:	opp based on which the bandwidth levels are to be configured
- *
- * This configures the bandwidth to the levels specified by the OPP. However
- * if the OPP specified is NULL the bandwidth levels are cleared out.
- *
- * Return: 0 on success or a negative error value.
- */
-int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp)
-{
-	struct opp_table *opp_table;
-	int ret;
-
-	opp_table = _find_opp_table(dev);
-	if (IS_ERR(opp_table)) {
-		dev_err(dev, "%s: device opp table doesn't exist\n", __func__);
-		return PTR_ERR(opp_table);
-	}
-
-	if (opp)
-		ret = _set_opp_bw(opp_table, opp, dev, false);
-	else
-		ret = _set_opp_bw(opp_table, NULL, dev, true);
-
-	dev_pm_opp_put_opp_table(opp_table);
-	return ret;
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_set_bw);
-
 static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
 {
 	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
@@ -982,7 +951,7 @@ static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
 	if (!_get_opp_count(opp_table))
 		return 0;
 
-	ret = _set_opp_bw(opp_table, NULL, dev, true);
+	ret = _set_opp_bw(opp_table, NULL, dev);
 	if (ret)
 		return ret;
 
@@ -1050,7 +1019,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 	}
 
 	if (!ret) {
-		ret = _set_opp_bw(opp_table, opp, dev, false);
+		ret = _set_opp_bw(opp_table, opp, dev);
 		if (!ret) {
 			opp_table->enabled = true;
 			dev_pm_opp_put(old_opp);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 2d8a706f8d00..891276b46e97 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -159,7 +159,6 @@ struct opp_table *devm_pm_opp_attach_genpd(struct device *dev, const char **name
 int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
 int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp);
-int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp);
 int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
 int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
 void dev_pm_opp_remove_table(struct device *dev);
@@ -382,11 +381,6 @@ static inline int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp)
 	return -ENOTSUPP;
 }
 
-static inline int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp)
-{
-	return -EOPNOTSUPP;
-}
-
 static inline int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask)
 {
 	return -ENOTSUPP;
-- 
2.25.0.rc1.19.g042ed3e048af


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 13/13] opp: Remove dev_pm_opp_set_bw()
@ 2021-01-21 11:17   ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-21 11:17 UTC (permalink / raw)
  To: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd,
	Rafael J. Wysocki
  Cc: Vincent Guittot, linux-pm, Viresh Kumar, linux-kernel,
	Sibi Sankar, linux-arm-kernel

All the users have migrated to dev_pm_opp_set_opp() now, get rid of the
duplicate API, dev_pm_opp_set_bw(), which only performs a part of the new API.

While at it, remove the unnecessary parameter to _set_opp_bw().

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c     | 41 +++++------------------------------------
 include/linux/pm_opp.h |  6 ------
 2 files changed, 5 insertions(+), 42 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 5a367ef02b92..d8ca15d96ce9 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -798,7 +798,7 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
 }
 
 static int _set_opp_bw(const struct opp_table *opp_table,
-		       struct dev_pm_opp *opp, struct device *dev, bool remove)
+		       struct dev_pm_opp *opp, struct device *dev)
 {
 	u32 avg, peak;
 	int i, ret;
@@ -807,7 +807,7 @@ static int _set_opp_bw(const struct opp_table *opp_table,
 		return 0;
 
 	for (i = 0; i < opp_table->path_count; i++) {
-		if (remove) {
+		if (!opp) {
 			avg = 0;
 			peak = 0;
 		} else {
@@ -817,7 +817,7 @@ static int _set_opp_bw(const struct opp_table *opp_table,
 		ret = icc_set_bw(opp_table->paths[i], avg, peak);
 		if (ret) {
 			dev_err(dev, "Failed to %s bandwidth[%d]: %d\n",
-				remove ? "remove" : "set", i, ret);
+				opp ? "set" : "remove", i, ret);
 			return ret;
 		}
 	}
@@ -911,37 +911,6 @@ static int _set_required_opps(struct device *dev,
 	return ret;
 }
 
-/**
- * dev_pm_opp_set_bw() - sets bandwidth levels corresponding to an opp
- * @dev:	device for which we do this operation
- * @opp:	opp based on which the bandwidth levels are to be configured
- *
- * This configures the bandwidth to the levels specified by the OPP. However
- * if the OPP specified is NULL the bandwidth levels are cleared out.
- *
- * Return: 0 on success or a negative error value.
- */
-int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp)
-{
-	struct opp_table *opp_table;
-	int ret;
-
-	opp_table = _find_opp_table(dev);
-	if (IS_ERR(opp_table)) {
-		dev_err(dev, "%s: device opp table doesn't exist\n", __func__);
-		return PTR_ERR(opp_table);
-	}
-
-	if (opp)
-		ret = _set_opp_bw(opp_table, opp, dev, false);
-	else
-		ret = _set_opp_bw(opp_table, NULL, dev, true);
-
-	dev_pm_opp_put_opp_table(opp_table);
-	return ret;
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_set_bw);
-
 static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
 {
 	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
@@ -982,7 +951,7 @@ static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
 	if (!_get_opp_count(opp_table))
 		return 0;
 
-	ret = _set_opp_bw(opp_table, NULL, dev, true);
+	ret = _set_opp_bw(opp_table, NULL, dev);
 	if (ret)
 		return ret;
 
@@ -1050,7 +1019,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 	}
 
 	if (!ret) {
-		ret = _set_opp_bw(opp_table, opp, dev, false);
+		ret = _set_opp_bw(opp_table, opp, dev);
 		if (!ret) {
 			opp_table->enabled = true;
 			dev_pm_opp_put(old_opp);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 2d8a706f8d00..891276b46e97 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -159,7 +159,6 @@ struct opp_table *devm_pm_opp_attach_genpd(struct device *dev, const char **name
 int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
 int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp);
-int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp);
 int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
 int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
 void dev_pm_opp_remove_table(struct device *dev);
@@ -382,11 +381,6 @@ static inline int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp)
 	return -ENOTSUPP;
 }
 
-static inline int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp)
-{
-	return -EOPNOTSUPP;
-}
-
 static inline int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask)
 {
 	return -ENOTSUPP;
-- 
2.25.0.rc1.19.g042ed3e048af


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() to work for non-freq devices
  2021-01-21 11:17   ` Viresh Kumar
@ 2021-01-21 20:26     ` Dmitry Osipenko
  -1 siblings, 0 replies; 73+ messages in thread
From: Dmitry Osipenko @ 2021-01-21 20:26 UTC (permalink / raw)
  To: Viresh Kumar, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: linux-pm, Vincent Guittot, Rafael Wysocki, Sibi Sankar,
	linux-kernel, linux-arm-kernel, linux-tegra

21.01.2021 14:17, Viresh Kumar пишет:
> In order to avoid conditional statements at the caller site, this patch
> updates _generic_set_opp_clk_only() to work for devices that don't
> change frequency (like power domains, etc.). Return 0 if the clk pointer
> passed to this routine is not valid.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
...

Hello Viresh,

Thank you very much for yours effort! I gave a quick test to this series
and instantly found one small issue in this patch.

> +	/* We may reach here for devices which don't change frequency */
> +	if (unlikely(!clk))

I replaced dev_pm_opp_set_voltage() with dev_pm_opp_set_opp() in the
Tegra PD driver and got a crash, which happens because the above line
should be:

	if (IS_ERR(clk))

The opp_table->clk is initialized to ERR_PTR(-ENOENT) if device doesn't
have a clock, like a power domain device in my case.

Everything works good after fixing this patch. I'll keep testing and
will be taking a closer look at the rest of the patches over this weekend.

For the record, here is a backtrace of the crash:

Unable to handle kernel NULL pointer dereference at virtual address 00000016
...
(clk_set_rate) from (_set_opp)
(_set_opp) from (dev_pm_opp_set_opp)
(dev_pm_opp_set_opp) from (tegra_genpd_set_performance_state)
(tegra_genpd_set_performance_state) from (_genpd_set_performance_state)
(_genpd_set_performance_state) from (dev_pm_genpd_set_performance_state)
(dev_pm_genpd_set_performance_state) from (_set_required_opp)
(_set_required_opp) from (_set_opp)
(_set_opp) from (dev_pm_opp_set_rate)
(dev_pm_opp_set_rate) from (host1x_runtime_resume)
(host1x_runtime_resume) from (genpd_runtime_resume)
(genpd_runtime_resume) from (__rpm_callback)
(__rpm_callback) from (rpm_callback)
(rpm_callback) from (rpm_resume)
(rpm_resume) from (__pm_runtime_resume)
(__pm_runtime_resume) from (host1x_probe)
(host1x_probe) from (platform_probe)
(platform_probe) from (really_probe)
(really_probe) from (driver_probe_device)
(driver_probe_device) from (device_driver_attach)
(device_driver_attach) from (__driver_attach)
(__driver_attach) from (bus_for_each_dev)
(bus_for_each_dev) from (bus_add_driver)
(bus_add_driver) from (driver_register)
(driver_register) from (__platform_register_drivers)
(__platform_register_drivers) from (host1x_module_init)
(host1x_module_init) from (do_one_initcall)
(do_one_initcall) from (kernel_init_freeable)
(kernel_init_freeable) from (kernel_init)

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() to work for non-freq devices
@ 2021-01-21 20:26     ` Dmitry Osipenko
  0 siblings, 0 replies; 73+ messages in thread
From: Dmitry Osipenko @ 2021-01-21 20:26 UTC (permalink / raw)
  To: Viresh Kumar, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Vincent Guittot, linux-pm, Rafael Wysocki, linux-kernel,
	Sibi Sankar, linux-tegra, linux-arm-kernel

21.01.2021 14:17, Viresh Kumar пишет:
> In order to avoid conditional statements at the caller site, this patch
> updates _generic_set_opp_clk_only() to work for devices that don't
> change frequency (like power domains, etc.). Return 0 if the clk pointer
> passed to this routine is not valid.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
...

Hello Viresh,

Thank you very much for yours effort! I gave a quick test to this series
and instantly found one small issue in this patch.

> +	/* We may reach here for devices which don't change frequency */
> +	if (unlikely(!clk))

I replaced dev_pm_opp_set_voltage() with dev_pm_opp_set_opp() in the
Tegra PD driver and got a crash, which happens because the above line
should be:

	if (IS_ERR(clk))

The opp_table->clk is initialized to ERR_PTR(-ENOENT) if device doesn't
have a clock, like a power domain device in my case.

Everything works good after fixing this patch. I'll keep testing and
will be taking a closer look at the rest of the patches over this weekend.

For the record, here is a backtrace of the crash:

Unable to handle kernel NULL pointer dereference at virtual address 00000016
...
(clk_set_rate) from (_set_opp)
(_set_opp) from (dev_pm_opp_set_opp)
(dev_pm_opp_set_opp) from (tegra_genpd_set_performance_state)
(tegra_genpd_set_performance_state) from (_genpd_set_performance_state)
(_genpd_set_performance_state) from (dev_pm_genpd_set_performance_state)
(dev_pm_genpd_set_performance_state) from (_set_required_opp)
(_set_required_opp) from (_set_opp)
(_set_opp) from (dev_pm_opp_set_rate)
(dev_pm_opp_set_rate) from (host1x_runtime_resume)
(host1x_runtime_resume) from (genpd_runtime_resume)
(genpd_runtime_resume) from (__rpm_callback)
(__rpm_callback) from (rpm_callback)
(rpm_callback) from (rpm_resume)
(rpm_resume) from (__pm_runtime_resume)
(__pm_runtime_resume) from (host1x_probe)
(host1x_probe) from (platform_probe)
(platform_probe) from (really_probe)
(really_probe) from (driver_probe_device)
(driver_probe_device) from (device_driver_attach)
(device_driver_attach) from (__driver_attach)
(__driver_attach) from (bus_for_each_dev)
(bus_for_each_dev) from (bus_add_driver)
(bus_add_driver) from (driver_register)
(driver_register) from (__platform_register_drivers)
(__platform_register_drivers) from (host1x_module_init)
(host1x_module_init) from (do_one_initcall)
(do_one_initcall) from (kernel_init_freeable)
(kernel_init_freeable) from (kernel_init)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
  2021-01-21 11:17   ` Viresh Kumar
@ 2021-01-21 21:36     ` Dmitry Osipenko
  -1 siblings, 0 replies; 73+ messages in thread
From: Dmitry Osipenko @ 2021-01-21 21:36 UTC (permalink / raw)
  To: Viresh Kumar, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Thierry Reding, Jonathan Hunter
  Cc: linux-pm, Vincent Guittot, Rafael Wysocki, Stephen Boyd,
	Nishanth Menon, Sibi Sankar, linux-kernel, linux-arm-kernel,
	linux-tegra

21.01.2021 14:17, Viresh Kumar пишет:
> dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
> be used instead. Migrate to the new API.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/devfreq/tegra30-devfreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 117cad7968ab..d2477d7d1f66 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>  		return PTR_ERR(opp);
>  	}
>  
> -	ret = dev_pm_opp_set_bw(dev, opp);
> +	ret = dev_pm_opp_set_opp(dev, opp);
>  	dev_pm_opp_put(opp);
>  
>  	return ret;
> 

This patch introduces a very serious change that needs to be fixed.

Now dev_pm_opp_set_opp() changes both clock rate and bandwidth, this is
unacceptable for this driver because it shall not touch the clock rate.

I think dev_pm_opp_set_bw() can't be removed.

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
@ 2021-01-21 21:36     ` Dmitry Osipenko
  0 siblings, 0 replies; 73+ messages in thread
From: Dmitry Osipenko @ 2021-01-21 21:36 UTC (permalink / raw)
  To: Viresh Kumar, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Thierry Reding, Jonathan Hunter
  Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd,
	Rafael Wysocki, linux-kernel, Sibi Sankar, linux-tegra,
	linux-arm-kernel

21.01.2021 14:17, Viresh Kumar пишет:
> dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
> be used instead. Migrate to the new API.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/devfreq/tegra30-devfreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 117cad7968ab..d2477d7d1f66 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>  		return PTR_ERR(opp);
>  	}
>  
> -	ret = dev_pm_opp_set_bw(dev, opp);
> +	ret = dev_pm_opp_set_opp(dev, opp);
>  	dev_pm_opp_put(opp);
>  
>  	return ret;
> 

This patch introduces a very serious change that needs to be fixed.

Now dev_pm_opp_set_opp() changes both clock rate and bandwidth, this is
unacceptable for this driver because it shall not touch the clock rate.

I think dev_pm_opp_set_bw() can't be removed.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
  2021-01-21 11:17   ` Viresh Kumar
@ 2021-01-21 21:41     ` Dmitry Osipenko
  -1 siblings, 0 replies; 73+ messages in thread
From: Dmitry Osipenko @ 2021-01-21 21:41 UTC (permalink / raw)
  To: Viresh Kumar, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: linux-pm, Vincent Guittot, Rafael Wysocki, Sibi Sankar,
	linux-kernel, linux-arm-kernel

21.01.2021 14:17, Viresh Kumar пишет:
> @@ -1074,15 +1091,18 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  
>  	if (!ret) {
>  		ret = _set_opp_bw(opp_table, opp, dev, false);
> -		if (!ret)
> +		if (!ret) {
>  			opp_table->enabled = true;
> +			dev_pm_opp_put(old_opp);
> +
> +			/* Make sure current_opp doesn't get freed */
> +			dev_pm_opp_get(opp);
> +			opp_table->current_opp = opp;
> +		}
>  	}

I'm a bit surprised that _set_opp_bw() isn't used similarly to
_set_opp_voltage() in _generic_set_opp_regulator().

I'd expect the BW requirement to be raised before the clock rate goes UP.

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
@ 2021-01-21 21:41     ` Dmitry Osipenko
  0 siblings, 0 replies; 73+ messages in thread
From: Dmitry Osipenko @ 2021-01-21 21:41 UTC (permalink / raw)
  To: Viresh Kumar, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: Vincent Guittot, linux-pm, Rafael Wysocki, linux-kernel,
	Sibi Sankar, linux-arm-kernel

21.01.2021 14:17, Viresh Kumar пишет:
> @@ -1074,15 +1091,18 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  
>  	if (!ret) {
>  		ret = _set_opp_bw(opp_table, opp, dev, false);
> -		if (!ret)
> +		if (!ret) {
>  			opp_table->enabled = true;
> +			dev_pm_opp_put(old_opp);
> +
> +			/* Make sure current_opp doesn't get freed */
> +			dev_pm_opp_get(opp);
> +			opp_table->current_opp = opp;
> +		}
>  	}

I'm a bit surprised that _set_opp_bw() isn't used similarly to
_set_opp_voltage() in _generic_set_opp_regulator().

I'd expect the BW requirement to be raised before the clock rate goes UP.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() to work for non-freq devices
  2021-01-21 20:26     ` Dmitry Osipenko
@ 2021-01-22  4:35       ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-22  4:35 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
	Vincent Guittot, Rafael Wysocki, Sibi Sankar, linux-kernel,
	linux-arm-kernel, linux-tegra

On 21-01-21, 23:26, Dmitry Osipenko wrote:
> 21.01.2021 14:17, Viresh Kumar пишет:
> > In order to avoid conditional statements at the caller site, this patch
> > updates _generic_set_opp_clk_only() to work for devices that don't
> > change frequency (like power domains, etc.). Return 0 if the clk pointer
> > passed to this routine is not valid.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> ...
> 
> Hello Viresh,
> 
> Thank you very much for yours effort! I gave a quick test to this series
> and instantly found one small issue in this patch.
> 
> > +	/* We may reach here for devices which don't change frequency */
> > +	if (unlikely(!clk))
> 
> I replaced dev_pm_opp_set_voltage() with dev_pm_opp_set_opp() in the
> Tegra PD driver and got a crash, which happens because the above line
> should be:
> 
> 	if (IS_ERR(clk))

Fixed, thanks.

-- 
viresh

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() to work for non-freq devices
@ 2021-01-22  4:35       ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-22  4:35 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd,
	Viresh Kumar, Rafael Wysocki, linux-kernel, Sibi Sankar,
	linux-tegra, linux-arm-kernel

On 21-01-21, 23:26, Dmitry Osipenko wrote:
> 21.01.2021 14:17, Viresh Kumar пишет:
> > In order to avoid conditional statements at the caller site, this patch
> > updates _generic_set_opp_clk_only() to work for devices that don't
> > change frequency (like power domains, etc.). Return 0 if the clk pointer
> > passed to this routine is not valid.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> ...
> 
> Hello Viresh,
> 
> Thank you very much for yours effort! I gave a quick test to this series
> and instantly found one small issue in this patch.
> 
> > +	/* We may reach here for devices which don't change frequency */
> > +	if (unlikely(!clk))
> 
> I replaced dev_pm_opp_set_voltage() with dev_pm_opp_set_opp() in the
> Tegra PD driver and got a crash, which happens because the above line
> should be:
> 
> 	if (IS_ERR(clk))

Fixed, thanks.

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
  2021-01-21 21:41     ` Dmitry Osipenko
@ 2021-01-22  4:45       ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-22  4:45 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
	Vincent Guittot, Rafael Wysocki, Sibi Sankar, linux-kernel,
	linux-arm-kernel

On 22-01-21, 00:41, Dmitry Osipenko wrote:
> 21.01.2021 14:17, Viresh Kumar пишет:
> > @@ -1074,15 +1091,18 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> >  
> >  	if (!ret) {
> >  		ret = _set_opp_bw(opp_table, opp, dev, false);
> > -		if (!ret)
> > +		if (!ret) {
> >  			opp_table->enabled = true;
> > +			dev_pm_opp_put(old_opp);
> > +
> > +			/* Make sure current_opp doesn't get freed */
> > +			dev_pm_opp_get(opp);
> > +			opp_table->current_opp = opp;
> > +		}
> >  	}
> 
> I'm a bit surprised that _set_opp_bw() isn't used similarly to
> _set_opp_voltage() in _generic_set_opp_regulator().
> 
> I'd expect the BW requirement to be raised before the clock rate goes UP.

I remember discussing that earlier when this stuff came in, and this I
believe is the reason for that.

We need to scale regulators before/after frequency because when we
increase the frequency a regulator may _not_ be providing enough power
to sustain that (even for a short while) and this may have undesired
effects on the hardware and so it is important to prevent that
malfunction.

In case of bandwidth such issues will not happen (AFAIK) and doing it
just once is normally enough. It is just about allowing more data to
be transmitted, and won't make the hardware behave badly.

-- 
viresh

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
@ 2021-01-22  4:45       ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-22  4:45 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd,
	Viresh Kumar, Rafael Wysocki, linux-kernel, Sibi Sankar,
	linux-arm-kernel

On 22-01-21, 00:41, Dmitry Osipenko wrote:
> 21.01.2021 14:17, Viresh Kumar пишет:
> > @@ -1074,15 +1091,18 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> >  
> >  	if (!ret) {
> >  		ret = _set_opp_bw(opp_table, opp, dev, false);
> > -		if (!ret)
> > +		if (!ret) {
> >  			opp_table->enabled = true;
> > +			dev_pm_opp_put(old_opp);
> > +
> > +			/* Make sure current_opp doesn't get freed */
> > +			dev_pm_opp_get(opp);
> > +			opp_table->current_opp = opp;
> > +		}
> >  	}
> 
> I'm a bit surprised that _set_opp_bw() isn't used similarly to
> _set_opp_voltage() in _generic_set_opp_regulator().
> 
> I'd expect the BW requirement to be raised before the clock rate goes UP.

I remember discussing that earlier when this stuff came in, and this I
believe is the reason for that.

We need to scale regulators before/after frequency because when we
increase the frequency a regulator may _not_ be providing enough power
to sustain that (even for a short while) and this may have undesired
effects on the hardware and so it is important to prevent that
malfunction.

In case of bandwidth such issues will not happen (AFAIK) and doing it
just once is normally enough. It is just about allowing more data to
be transmitted, and won't make the hardware behave badly.

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
  2021-01-21 21:36     ` Dmitry Osipenko
@ 2021-01-22  6:26       ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-22  6:26 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Thierry Reding,
	Jonathan Hunter, linux-pm, Vincent Guittot, Rafael Wysocki,
	Stephen Boyd, Nishanth Menon, Sibi Sankar, linux-kernel,
	linux-arm-kernel, linux-tegra

On 22-01-21, 00:36, Dmitry Osipenko wrote:
> 21.01.2021 14:17, Viresh Kumar пишет:
> > dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
> > be used instead. Migrate to the new API.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  drivers/devfreq/tegra30-devfreq.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> > index 117cad7968ab..d2477d7d1f66 100644
> > --- a/drivers/devfreq/tegra30-devfreq.c
> > +++ b/drivers/devfreq/tegra30-devfreq.c
> > @@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
> >  		return PTR_ERR(opp);
> >  	}
> >  
> > -	ret = dev_pm_opp_set_bw(dev, opp);
> > +	ret = dev_pm_opp_set_opp(dev, opp);
> >  	dev_pm_opp_put(opp);
> >  
> >  	return ret;
> > 
> 
> This patch introduces a very serious change that needs to be fixed.
> 
> Now dev_pm_opp_set_opp() changes both clock rate and bandwidth, this is
> unacceptable for this driver because it shall not touch the clock rate.
> 
> I think dev_pm_opp_set_bw() can't be removed.

I am wondering here on what would be a better solution, do what you
said or introduce another helper like dev_pm_opp_clear_clk(), which
will make sure the OPP core doesn't play with device's clk.

-- 
viresh

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
@ 2021-01-22  6:26       ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-22  6:26 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd,
	Rafael Wysocki, linux-kernel, Jonathan Hunter, Chanwoo Choi,
	Kyungmin Park, MyungJoo Ham, Thierry Reding, Sibi Sankar,
	linux-tegra, linux-arm-kernel

On 22-01-21, 00:36, Dmitry Osipenko wrote:
> 21.01.2021 14:17, Viresh Kumar пишет:
> > dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
> > be used instead. Migrate to the new API.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  drivers/devfreq/tegra30-devfreq.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> > index 117cad7968ab..d2477d7d1f66 100644
> > --- a/drivers/devfreq/tegra30-devfreq.c
> > +++ b/drivers/devfreq/tegra30-devfreq.c
> > @@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
> >  		return PTR_ERR(opp);
> >  	}
> >  
> > -	ret = dev_pm_opp_set_bw(dev, opp);
> > +	ret = dev_pm_opp_set_opp(dev, opp);
> >  	dev_pm_opp_put(opp);
> >  
> >  	return ret;
> > 
> 
> This patch introduces a very serious change that needs to be fixed.
> 
> Now dev_pm_opp_set_opp() changes both clock rate and bandwidth, this is
> unacceptable for this driver because it shall not touch the clock rate.
> 
> I think dev_pm_opp_set_bw() can't be removed.

I am wondering here on what would be a better solution, do what you
said or introduce another helper like dev_pm_opp_clear_clk(), which
will make sure the OPP core doesn't play with device's clk.

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
  2021-01-22  4:45       ` Viresh Kumar
@ 2021-01-22 14:31         ` Dmitry Osipenko
  -1 siblings, 0 replies; 73+ messages in thread
From: Dmitry Osipenko @ 2021-01-22 14:31 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
	Vincent Guittot, Rafael Wysocki, Sibi Sankar, linux-kernel,
	linux-arm-kernel

22.01.2021 07:45, Viresh Kumar пишет:
> On 22-01-21, 00:41, Dmitry Osipenko wrote:
>> 21.01.2021 14:17, Viresh Kumar пишет:
>>> @@ -1074,15 +1091,18 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>>>  
>>>  	if (!ret) {
>>>  		ret = _set_opp_bw(opp_table, opp, dev, false);
>>> -		if (!ret)
>>> +		if (!ret) {
>>>  			opp_table->enabled = true;
>>> +			dev_pm_opp_put(old_opp);
>>> +
>>> +			/* Make sure current_opp doesn't get freed */
>>> +			dev_pm_opp_get(opp);
>>> +			opp_table->current_opp = opp;
>>> +		}
>>>  	}
>>
>> I'm a bit surprised that _set_opp_bw() isn't used similarly to
>> _set_opp_voltage() in _generic_set_opp_regulator().
>>
>> I'd expect the BW requirement to be raised before the clock rate goes UP.
> 
> I remember discussing that earlier when this stuff came in, and this I
> believe is the reason for that.
> 
> We need to scale regulators before/after frequency because when we
> increase the frequency a regulator may _not_ be providing enough power
> to sustain that (even for a short while) and this may have undesired
> effects on the hardware and so it is important to prevent that
> malfunction.
> 
> In case of bandwidth such issues will not happen (AFAIK) and doing it
> just once is normally enough. It is just about allowing more data to
> be transmitted, and won't make the hardware behave badly.
> 

This may not be true for all kinds of hardware, a display controller is
one example. If display's pixclock is raised before the memory bandwidth
of the display's memory client, then display controller may get a memory
underflow since it won't be able to fetch memory fast enough and it's
not possible to pause data transmission to display panel, hence display
panel may get out of sync and a full hardware reset will be needed in
order to recover. At least this is the case for NVIDIA Tegra SoCs.

I guess it's not a real problem for any of OPP API users right now, but
this is something to keep in mind.

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
@ 2021-01-22 14:31         ` Dmitry Osipenko
  0 siblings, 0 replies; 73+ messages in thread
From: Dmitry Osipenko @ 2021-01-22 14:31 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd,
	Viresh Kumar, Rafael Wysocki, linux-kernel, Sibi Sankar,
	linux-arm-kernel

22.01.2021 07:45, Viresh Kumar пишет:
> On 22-01-21, 00:41, Dmitry Osipenko wrote:
>> 21.01.2021 14:17, Viresh Kumar пишет:
>>> @@ -1074,15 +1091,18 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>>>  
>>>  	if (!ret) {
>>>  		ret = _set_opp_bw(opp_table, opp, dev, false);
>>> -		if (!ret)
>>> +		if (!ret) {
>>>  			opp_table->enabled = true;
>>> +			dev_pm_opp_put(old_opp);
>>> +
>>> +			/* Make sure current_opp doesn't get freed */
>>> +			dev_pm_opp_get(opp);
>>> +			opp_table->current_opp = opp;
>>> +		}
>>>  	}
>>
>> I'm a bit surprised that _set_opp_bw() isn't used similarly to
>> _set_opp_voltage() in _generic_set_opp_regulator().
>>
>> I'd expect the BW requirement to be raised before the clock rate goes UP.
> 
> I remember discussing that earlier when this stuff came in, and this I
> believe is the reason for that.
> 
> We need to scale regulators before/after frequency because when we
> increase the frequency a regulator may _not_ be providing enough power
> to sustain that (even for a short while) and this may have undesired
> effects on the hardware and so it is important to prevent that
> malfunction.
> 
> In case of bandwidth such issues will not happen (AFAIK) and doing it
> just once is normally enough. It is just about allowing more data to
> be transmitted, and won't make the hardware behave badly.
> 

This may not be true for all kinds of hardware, a display controller is
one example. If display's pixclock is raised before the memory bandwidth
of the display's memory client, then display controller may get a memory
underflow since it won't be able to fetch memory fast enough and it's
not possible to pause data transmission to display panel, hence display
panel may get out of sync and a full hardware reset will be needed in
order to recover. At least this is the case for NVIDIA Tegra SoCs.

I guess it's not a real problem for any of OPP API users right now, but
this is something to keep in mind.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
  2021-01-22  6:26       ` Viresh Kumar
@ 2021-01-22 15:28         ` Dmitry Osipenko
  -1 siblings, 0 replies; 73+ messages in thread
From: Dmitry Osipenko @ 2021-01-22 15:28 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Thierry Reding,
	Jonathan Hunter, linux-pm, Vincent Guittot, Rafael Wysocki,
	Stephen Boyd, Nishanth Menon, Sibi Sankar, linux-kernel,
	linux-arm-kernel, linux-tegra

22.01.2021 09:26, Viresh Kumar пишет:
> On 22-01-21, 00:36, Dmitry Osipenko wrote:
>> 21.01.2021 14:17, Viresh Kumar пишет:
>>> dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
>>> be used instead. Migrate to the new API.
>>>
>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>> ---
>>>  drivers/devfreq/tegra30-devfreq.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>>> index 117cad7968ab..d2477d7d1f66 100644
>>> --- a/drivers/devfreq/tegra30-devfreq.c
>>> +++ b/drivers/devfreq/tegra30-devfreq.c
>>> @@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>>>  		return PTR_ERR(opp);
>>>  	}
>>>  
>>> -	ret = dev_pm_opp_set_bw(dev, opp);
>>> +	ret = dev_pm_opp_set_opp(dev, opp);
>>>  	dev_pm_opp_put(opp);
>>>  
>>>  	return ret;
>>>
>>
>> This patch introduces a very serious change that needs to be fixed.
>>
>> Now dev_pm_opp_set_opp() changes both clock rate and bandwidth, this is
>> unacceptable for this driver because it shall not touch the clock rate.
>>
>> I think dev_pm_opp_set_bw() can't be removed.
> 
> I am wondering here on what would be a better solution, do what you
> said or introduce another helper like dev_pm_opp_clear_clk(), which
> will make sure the OPP core doesn't play with device's clk.
> 

Either way will work, but maybe keeping the dev_pm_opp_set_bw() is a bit
more straightforward variant for now since it will avoid the code's
changes and it's probably a bit more obvious variant for the OPP users.

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
@ 2021-01-22 15:28         ` Dmitry Osipenko
  0 siblings, 0 replies; 73+ messages in thread
From: Dmitry Osipenko @ 2021-01-22 15:28 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd,
	Rafael Wysocki, linux-kernel, Jonathan Hunter, Chanwoo Choi,
	Kyungmin Park, MyungJoo Ham, Thierry Reding, Sibi Sankar,
	linux-tegra, linux-arm-kernel

22.01.2021 09:26, Viresh Kumar пишет:
> On 22-01-21, 00:36, Dmitry Osipenko wrote:
>> 21.01.2021 14:17, Viresh Kumar пишет:
>>> dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
>>> be used instead. Migrate to the new API.
>>>
>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>> ---
>>>  drivers/devfreq/tegra30-devfreq.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>>> index 117cad7968ab..d2477d7d1f66 100644
>>> --- a/drivers/devfreq/tegra30-devfreq.c
>>> +++ b/drivers/devfreq/tegra30-devfreq.c
>>> @@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>>>  		return PTR_ERR(opp);
>>>  	}
>>>  
>>> -	ret = dev_pm_opp_set_bw(dev, opp);
>>> +	ret = dev_pm_opp_set_opp(dev, opp);
>>>  	dev_pm_opp_put(opp);
>>>  
>>>  	return ret;
>>>
>>
>> This patch introduces a very serious change that needs to be fixed.
>>
>> Now dev_pm_opp_set_opp() changes both clock rate and bandwidth, this is
>> unacceptable for this driver because it shall not touch the clock rate.
>>
>> I think dev_pm_opp_set_bw() can't be removed.
> 
> I am wondering here on what would be a better solution, do what you
> said or introduce another helper like dev_pm_opp_clear_clk(), which
> will make sure the OPP core doesn't play with device's clk.
> 

Either way will work, but maybe keeping the dev_pm_opp_set_bw() is a bit
more straightforward variant for now since it will avoid the code's
changes and it's probably a bit more obvious variant for the OPP users.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
  2021-01-22 14:31         ` Dmitry Osipenko
@ 2021-01-25  3:12           ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-25  3:12 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
	Vincent Guittot, Rafael Wysocki, Sibi Sankar, linux-kernel,
	linux-arm-kernel

On 22-01-21, 17:31, Dmitry Osipenko wrote:
> This may not be true for all kinds of hardware, a display controller is
> one example. If display's pixclock is raised before the memory bandwidth
> of the display's memory client, then display controller may get a memory
> underflow since it won't be able to fetch memory fast enough and it's
> not possible to pause data transmission to display panel, hence display
> panel may get out of sync and a full hardware reset will be needed in
> order to recover. At least this is the case for NVIDIA Tegra SoCs.

Hmm, but I expected that the request for more data will only come after the
opp-set-rate has finished and not in between. May be I am wrong. There is
nothing wrong in doing it the regulator way if required.

> I guess it's not a real problem for any of OPP API users right now, but
> this is something to keep in mind.

Sure, I am not against it. Just that we thought it isn't worth the code.

-- 
viresh

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
@ 2021-01-25  3:12           ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-25  3:12 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd,
	Viresh Kumar, Rafael Wysocki, linux-kernel, Sibi Sankar,
	linux-arm-kernel

On 22-01-21, 17:31, Dmitry Osipenko wrote:
> This may not be true for all kinds of hardware, a display controller is
> one example. If display's pixclock is raised before the memory bandwidth
> of the display's memory client, then display controller may get a memory
> underflow since it won't be able to fetch memory fast enough and it's
> not possible to pause data transmission to display panel, hence display
> panel may get out of sync and a full hardware reset will be needed in
> order to recover. At least this is the case for NVIDIA Tegra SoCs.

Hmm, but I expected that the request for more data will only come after the
opp-set-rate has finished and not in between. May be I am wrong. There is
nothing wrong in doing it the regulator way if required.

> I guess it's not a real problem for any of OPP API users right now, but
> this is something to keep in mind.

Sure, I am not against it. Just that we thought it isn't worth the code.

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
  2021-01-22 15:28         ` Dmitry Osipenko
@ 2021-01-25  3:14           ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-25  3:14 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Thierry Reding,
	Jonathan Hunter, linux-pm, Vincent Guittot, Rafael Wysocki,
	Stephen Boyd, Nishanth Menon, Sibi Sankar, linux-kernel,
	linux-arm-kernel, linux-tegra

On 22-01-21, 18:28, Dmitry Osipenko wrote:
> Either way will work, but maybe keeping the dev_pm_opp_set_bw() is a bit
> more straightforward variant for now since it will avoid the code's
> changes and it's probably a bit more obvious variant for the OPP users.

The problem is it creates unnecessary paths which we need to support. For
example, in case of bandwidth itself we may want to update regulator/pm
domain/required OPPs and this should all be done for everyone. I really do not
want to keep separate paths for such stuff, it makes it hard to maintain..

-- 
viresh

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
@ 2021-01-25  3:14           ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-25  3:14 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd,
	Rafael Wysocki, linux-kernel, Jonathan Hunter, Chanwoo Choi,
	Kyungmin Park, MyungJoo Ham, Thierry Reding, Sibi Sankar,
	linux-tegra, linux-arm-kernel

On 22-01-21, 18:28, Dmitry Osipenko wrote:
> Either way will work, but maybe keeping the dev_pm_opp_set_bw() is a bit
> more straightforward variant for now since it will avoid the code's
> changes and it's probably a bit more obvious variant for the OPP users.

The problem is it creates unnecessary paths which we need to support. For
example, in case of bandwidth itself we may want to update regulator/pm
domain/required OPPs and this should all be done for everyone. I really do not
want to keep separate paths for such stuff, it makes it hard to maintain..

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
  2021-01-25  3:14           ` Viresh Kumar
@ 2021-01-25 16:00             ` Dmitry Osipenko
  -1 siblings, 0 replies; 73+ messages in thread
From: Dmitry Osipenko @ 2021-01-25 16:00 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Thierry Reding,
	Jonathan Hunter, linux-pm, Vincent Guittot, Rafael Wysocki,
	Stephen Boyd, Nishanth Menon, Sibi Sankar, linux-kernel,
	linux-arm-kernel, linux-tegra

25.01.2021 06:14, Viresh Kumar пишет:
> On 22-01-21, 18:28, Dmitry Osipenko wrote:
>> Either way will work, but maybe keeping the dev_pm_opp_set_bw() is a bit
>> more straightforward variant for now since it will avoid the code's
>> changes and it's probably a bit more obvious variant for the OPP users.
> 
> The problem is it creates unnecessary paths which we need to support. For
> example, in case of bandwidth itself we may want to update regulator/pm
> domain/required OPPs and this should all be done for everyone. I really do not
> want to keep separate paths for such stuff, it makes it hard to maintain..
> 

Maybe we could add dev_pm_opp_of_add_table_without_clock(), at least
that should be a bit nicer from a drivers perspective than having to
care about dev_pm_opp_clear_clk(), IMO.

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
@ 2021-01-25 16:00             ` Dmitry Osipenko
  0 siblings, 0 replies; 73+ messages in thread
From: Dmitry Osipenko @ 2021-01-25 16:00 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd,
	Rafael Wysocki, linux-kernel, Jonathan Hunter, Chanwoo Choi,
	Kyungmin Park, MyungJoo Ham, Thierry Reding, Sibi Sankar,
	linux-tegra, linux-arm-kernel

25.01.2021 06:14, Viresh Kumar пишет:
> On 22-01-21, 18:28, Dmitry Osipenko wrote:
>> Either way will work, but maybe keeping the dev_pm_opp_set_bw() is a bit
>> more straightforward variant for now since it will avoid the code's
>> changes and it's probably a bit more obvious variant for the OPP users.
> 
> The problem is it creates unnecessary paths which we need to support. For
> example, in case of bandwidth itself we may want to update regulator/pm
> domain/required OPPs and this should all be done for everyone. I really do not
> want to keep separate paths for such stuff, it makes it hard to maintain..
> 

Maybe we could add dev_pm_opp_of_add_table_without_clock(), at least
that should be a bit nicer from a drivers perspective than having to
care about dev_pm_opp_clear_clk(), IMO.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() to work for non-freq devices
  2021-01-22  4:35       ` Viresh Kumar
@ 2021-01-25 21:09         ` Dmitry Osipenko
  -1 siblings, 0 replies; 73+ messages in thread
From: Dmitry Osipenko @ 2021-01-25 21:09 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
	Vincent Guittot, Rafael Wysocki, Sibi Sankar, linux-kernel,
	linux-arm-kernel, linux-tegra

22.01.2021 07:35, Viresh Kumar пишет:
> On 21-01-21, 23:26, Dmitry Osipenko wrote:
>> 21.01.2021 14:17, Viresh Kumar пишет:
>>> In order to avoid conditional statements at the caller site, this patch
>>> updates _generic_set_opp_clk_only() to work for devices that don't
>>> change frequency (like power domains, etc.). Return 0 if the clk pointer
>>> passed to this routine is not valid.
>>>
>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>> ---
>> ...
>>
>> Hello Viresh,
>>
>> Thank you very much for yours effort! I gave a quick test to this series
>> and instantly found one small issue in this patch.
>>
>>> +	/* We may reach here for devices which don't change frequency */
>>> +	if (unlikely(!clk))
>>
>> I replaced dev_pm_opp_set_voltage() with dev_pm_opp_set_opp() in the
>> Tegra PD driver and got a crash, which happens because the above line
>> should be:
>>
>> 	if (IS_ERR(clk))
> 
> Fixed, thanks.
> 

Please remove unlikely() around IS_ERR(), it already has the unlikely().

https://elixir.bootlin.com/linux/v5.11-rc4/source/include/linux/err.h#L22

I'd also recommend to remove all the unlikely() from OPP code since it
doesn't bring any value if not used in a very performance-critical code
path. OPP core doesn't have such code paths. The [un]likely() only make
code less readable and may result in a worse assembly.

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() to work for non-freq devices
@ 2021-01-25 21:09         ` Dmitry Osipenko
  0 siblings, 0 replies; 73+ messages in thread
From: Dmitry Osipenko @ 2021-01-25 21:09 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd,
	Viresh Kumar, Rafael Wysocki, linux-kernel, Sibi Sankar,
	linux-tegra, linux-arm-kernel

22.01.2021 07:35, Viresh Kumar пишет:
> On 21-01-21, 23:26, Dmitry Osipenko wrote:
>> 21.01.2021 14:17, Viresh Kumar пишет:
>>> In order to avoid conditional statements at the caller site, this patch
>>> updates _generic_set_opp_clk_only() to work for devices that don't
>>> change frequency (like power domains, etc.). Return 0 if the clk pointer
>>> passed to this routine is not valid.
>>>
>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>> ---
>> ...
>>
>> Hello Viresh,
>>
>> Thank you very much for yours effort! I gave a quick test to this series
>> and instantly found one small issue in this patch.
>>
>>> +	/* We may reach here for devices which don't change frequency */
>>> +	if (unlikely(!clk))
>>
>> I replaced dev_pm_opp_set_voltage() with dev_pm_opp_set_opp() in the
>> Tegra PD driver and got a crash, which happens because the above line
>> should be:
>>
>> 	if (IS_ERR(clk))
> 
> Fixed, thanks.
> 

Please remove unlikely() around IS_ERR(), it already has the unlikely().

https://elixir.bootlin.com/linux/v5.11-rc4/source/include/linux/err.h#L22

I'd also recommend to remove all the unlikely() from OPP code since it
doesn't bring any value if not used in a very performance-critical code
path. OPP core doesn't have such code paths. The [un]likely() only make
code less readable and may result in a worse assembly.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() to work for non-freq devices
  2021-01-25 21:09         ` Dmitry Osipenko
@ 2021-01-27  6:58           ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-27  6:58 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
	Vincent Guittot, Rafael Wysocki, Sibi Sankar, linux-kernel,
	linux-arm-kernel, linux-tegra

On 26-01-21, 00:09, Dmitry Osipenko wrote:
> Please remove unlikely() around IS_ERR(), it already has the unlikely().

Right.

> https://elixir.bootlin.com/linux/v5.11-rc4/source/include/linux/err.h#L22
> 
> I'd also recommend to remove all the unlikely() from OPP code since it
> doesn't bring any value if not used in a very performance-critical code
> path. OPP core doesn't have such code paths. The [un]likely() only make
> code less readable and may result in a worse assembly.

The likely/unlikely() stuff is to optimize code, not necessarily the stuff in
the hot path alone, therwise stuff like IS_ERR() would never have it. It surely
does bring value by optimizing the code, surely the result isn't significant
enough but that is fine, every effort counts.

AFAIK, if we are sure of path the code will almost always take, then we should
rather use these and so I am inclined towards keeping them. Though I understand
that using them may result in bad behavior (performance) if they fail.

-- 
viresh

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() to work for non-freq devices
@ 2021-01-27  6:58           ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-27  6:58 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd,
	Viresh Kumar, Rafael Wysocki, linux-kernel, Sibi Sankar,
	linux-tegra, linux-arm-kernel

On 26-01-21, 00:09, Dmitry Osipenko wrote:
> Please remove unlikely() around IS_ERR(), it already has the unlikely().

Right.

> https://elixir.bootlin.com/linux/v5.11-rc4/source/include/linux/err.h#L22
> 
> I'd also recommend to remove all the unlikely() from OPP code since it
> doesn't bring any value if not used in a very performance-critical code
> path. OPP core doesn't have such code paths. The [un]likely() only make
> code less readable and may result in a worse assembly.

The likely/unlikely() stuff is to optimize code, not necessarily the stuff in
the hot path alone, therwise stuff like IS_ERR() would never have it. It surely
does bring value by optimizing the code, surely the result isn't significant
enough but that is fine, every effort counts.

AFAIK, if we are sure of path the code will almost always take, then we should
rather use these and so I am inclined towards keeping them. Though I understand
that using them may result in bad behavior (performance) if they fail.

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH V2 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
  2021-01-21 11:17   ` Viresh Kumar
  (?)
  (?)
@ 2021-01-27  9:10   ` Viresh Kumar
  2021-01-27 10:02     ` Viresh Kumar
                       ` (2 more replies)
  -1 siblings, 3 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-27  9:10 UTC (permalink / raw)
  To: Dmitry Osipenko, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Thierry Reding, Jonathan Hunter
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki,
	Stephen Boyd, Nishanth Menon, linux-tegra, linux-kernel

dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
be used instead. Migrate to the new API.

We don't want the OPP core to manage the clk for this driver, migrate to
dev_pm_opp_of_add_table_noclk() to make sure dev_pm_opp_set_opp()
doesn't have any side effects.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Dmitry,

This is based over the patches sent here:

https://lore.kernel.org/lkml/6c2160ff30a8f421563793020264cf9f533f293c.1611738228.git.viresh.kumar@linaro.org/

This should fix the problem you mentioned earlier. Will push this for
linux-next unless you have any issues with it.

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

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 117cad7968ab..31f7dec5990b 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
 		return PTR_ERR(opp);
 	}
 
-	ret = dev_pm_opp_set_bw(dev, opp);
+	ret = dev_pm_opp_set_opp(dev, opp);
 	dev_pm_opp_put(opp);
 
 	return ret;
@@ -849,7 +849,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	err = dev_pm_opp_of_add_table(&pdev->dev);
+	err = dev_pm_opp_of_add_table_noclk(&pdev->dev);
 	if (err) {
 		dev_err(&pdev->dev, "Failed to add OPP table: %d\n", err);
 		goto put_hw;
-- 
2.25.0.rc1.19.g042ed3e048af


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH V2 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
  2021-01-27  9:10   ` [PATCH V2 " Viresh Kumar
@ 2021-01-27 10:02     ` Viresh Kumar
  2021-01-27 15:58       ` Dmitry Osipenko
  2021-02-01  0:21     ` Chanwoo Choi
  2021-02-01 19:57     ` Dmitry Osipenko
  2 siblings, 1 reply; 73+ messages in thread
From: Viresh Kumar @ 2021-01-27 10:02 UTC (permalink / raw)
  To: Dmitry Osipenko, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Thierry Reding, Jonathan Hunter
  Cc: linux-pm, Vincent Guittot, Rafael Wysocki, Stephen Boyd,
	Nishanth Menon, linux-tegra, linux-kernel

On 27-01-21, 14:40, Viresh Kumar wrote:
> dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
> be used instead. Migrate to the new API.
> 
> We don't want the OPP core to manage the clk for this driver, migrate to
> dev_pm_opp_of_add_table_noclk() to make sure dev_pm_opp_set_opp()
> doesn't have any side effects.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Dmitry,
> 
> This is based over the patches sent here:
> 
> https://lore.kernel.org/lkml/6c2160ff30a8f421563793020264cf9f533f293c.1611738228.git.viresh.kumar@linaro.org/
> 
> This should fix the problem you mentioned earlier. Will push this for
> linux-next unless you have any issues with it.
> 
>  drivers/devfreq/tegra30-devfreq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 117cad7968ab..31f7dec5990b 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>  		return PTR_ERR(opp);
>  	}
>  
> -	ret = dev_pm_opp_set_bw(dev, opp);
> +	ret = dev_pm_opp_set_opp(dev, opp);
>  	dev_pm_opp_put(opp);
>  
>  	return ret;
> @@ -849,7 +849,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> -	err = dev_pm_opp_of_add_table(&pdev->dev);
> +	err = dev_pm_opp_of_add_table_noclk(&pdev->dev);

Plus this, somehow was left uncommited in my tree :(

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 31f7dec5990b..ce83f883ca65 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -849,7 +849,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
                return err;
        }
 
-       err = dev_pm_opp_of_add_table_noclk(&pdev->dev);
+       err = dev_pm_opp_of_add_table_noclk(&pdev->dev, 0);
        if (err) {
                dev_err(&pdev->dev, "Failed to add OPP table: %d\n", err);
                goto put_hw;

-- 
viresh

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH V2 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
  2021-01-27 10:02     ` Viresh Kumar
@ 2021-01-27 15:58       ` Dmitry Osipenko
  2021-01-28  7:01         ` Viresh Kumar
  0 siblings, 1 reply; 73+ messages in thread
From: Dmitry Osipenko @ 2021-01-27 15:58 UTC (permalink / raw)
  To: Viresh Kumar, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Thierry Reding, Jonathan Hunter
  Cc: linux-pm, Vincent Guittot, Rafael Wysocki, Stephen Boyd,
	Nishanth Menon, linux-tegra, linux-kernel

27.01.2021 13:02, Viresh Kumar пишет:
> On 27-01-21, 14:40, Viresh Kumar wrote:
>> dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
>> be used instead. Migrate to the new API.
>>
>> We don't want the OPP core to manage the clk for this driver, migrate to
>> dev_pm_opp_of_add_table_noclk() to make sure dev_pm_opp_set_opp()
>> doesn't have any side effects.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>> Dmitry,
>>
>> This is based over the patches sent here:
>>
>> https://lore.kernel.org/lkml/6c2160ff30a8f421563793020264cf9f533f293c.1611738228.git.viresh.kumar@linaro.org/
>>
>> This should fix the problem you mentioned earlier. Will push this for
>> linux-next unless you have any issues with it.
>>
>>  drivers/devfreq/tegra30-devfreq.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>> index 117cad7968ab..31f7dec5990b 100644
>> --- a/drivers/devfreq/tegra30-devfreq.c
>> +++ b/drivers/devfreq/tegra30-devfreq.c
>> @@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>>  		return PTR_ERR(opp);
>>  	}
>>  
>> -	ret = dev_pm_opp_set_bw(dev, opp);
>> +	ret = dev_pm_opp_set_opp(dev, opp);
>>  	dev_pm_opp_put(opp);
>>  
>>  	return ret;
>> @@ -849,7 +849,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>  		return err;
>>  	}
>>  
>> -	err = dev_pm_opp_of_add_table(&pdev->dev);
>> +	err = dev_pm_opp_of_add_table_noclk(&pdev->dev);
> 
> Plus this, somehow was left uncommited in my tree :(
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 31f7dec5990b..ce83f883ca65 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -849,7 +849,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>                 return err;
>         }
>  
> -       err = dev_pm_opp_of_add_table_noclk(&pdev->dev);
> +       err = dev_pm_opp_of_add_table_noclk(&pdev->dev, 0);
>         if (err) {
>                 dev_err(&pdev->dev, "Failed to add OPP table: %d\n", err);
>                 goto put_hw;
> 

Sadly this doesn't work because we missed that clk is assigned to
opp_table when OPP table is allocated and not when it's added to device.

Hence we're now set back to the dev_pm_opp_clear_clk() variant.

What about to add a new OPP API which will allow OPP users to configure
behaviour that user wants from OPP core in a generic way, something like
this:

struct opp_config {
	bool no_clk;
	...
};

devm_pm_opp_set_config(dev, struct opp_config);
dev_pm_opp_set_config(dev, struct opp_config);
dev_pm_opp_unset_config(dev);

Or maybe even rename it dev_pm_opp_allocate_table(dev, struct
opp_config), which will allow users to directly allocate OPP table
instead of relying on the implicit allocations. Then there won't be a
need for drivers to use a dummy devm_pm_opp_set_clkname(dev, NULL) just
to allocate the table usable for dev_pm_opp_set_rate().

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
  2021-01-22  4:45       ` Viresh Kumar
  (?)
  (?)
@ 2021-01-27 16:31       ` Akhil P Oommen
  2021-01-28  4:14           ` Viresh Kumar
  -1 siblings, 1 reply; 73+ messages in thread
From: Akhil P Oommen @ 2021-01-27 16:31 UTC (permalink / raw)
  To: Viresh Kumar, Dmitry Osipenko
  Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
	Vincent Guittot, Rafael Wysocki, Sibi Sankar, linux-kernel,
	linux-arm-kernel

On 1/22/2021 10:15 AM, Viresh Kumar wrote:
> On 22-01-21, 00:41, Dmitry Osipenko wrote:
>> 21.01.2021 14:17, Viresh Kumar пишет:
>>> @@ -1074,15 +1091,18 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>>>   
>>>   	if (!ret) {
>>>   		ret = _set_opp_bw(opp_table, opp, dev, false);
>>> -		if (!ret)
>>> +		if (!ret) {
>>>   			opp_table->enabled = true;
>>> +			dev_pm_opp_put(old_opp);
>>> +
>>> +			/* Make sure current_opp doesn't get freed */
>>> +			dev_pm_opp_get(opp);
>>> +			opp_table->current_opp = opp;
>>> +		}
>>>   	}
>>
>> I'm a bit surprised that _set_opp_bw() isn't used similarly to
>> _set_opp_voltage() in _generic_set_opp_regulator().
>>
>> I'd expect the BW requirement to be raised before the clock rate goes UP.
> 
> I remember discussing that earlier when this stuff came in, and this I
> believe is the reason for that.
> 
> We need to scale regulators before/after frequency because when we
> increase the frequency a regulator may _not_ be providing enough power
> to sustain that (even for a short while) and this may have undesired
> effects on the hardware and so it is important to prevent that
> malfunction.
> 
> In case of bandwidth such issues will not happen (AFAIK) and doing it
> just once is normally enough. It is just about allowing more data to
> be transmitted, and won't make the hardware behave badly.
> 
I agree with Dmitry. BW is a shared resource in a lot of architectures. 
Raising clk before increasing the bw can lead to a scenario where this 
client saturate the entire BW for whatever small duration it may be. 
This will impact the latency requirements of other clients.

-Akhil.

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
  2021-01-27 16:31       ` Akhil P Oommen
@ 2021-01-28  4:14           ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-28  4:14 UTC (permalink / raw)
  To: Akhil P Oommen
  Cc: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd,
	linux-pm, Vincent Guittot, Rafael Wysocki, Sibi Sankar,
	linux-kernel, linux-arm-kernel

On 27-01-21, 22:01, Akhil P Oommen wrote:
> On 1/22/2021 10:15 AM, Viresh Kumar wrote:
> > On 22-01-21, 00:41, Dmitry Osipenko wrote:
> > > 21.01.2021 14:17, Viresh Kumar пишет:
> > > > @@ -1074,15 +1091,18 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> > > >   	if (!ret) {
> > > >   		ret = _set_opp_bw(opp_table, opp, dev, false);
> > > > -		if (!ret)
> > > > +		if (!ret) {
> > > >   			opp_table->enabled = true;
> > > > +			dev_pm_opp_put(old_opp);
> > > > +
> > > > +			/* Make sure current_opp doesn't get freed */
> > > > +			dev_pm_opp_get(opp);
> > > > +			opp_table->current_opp = opp;
> > > > +		}
> > > >   	}
> > > 
> > > I'm a bit surprised that _set_opp_bw() isn't used similarly to
> > > _set_opp_voltage() in _generic_set_opp_regulator().
> > > 
> > > I'd expect the BW requirement to be raised before the clock rate goes UP.
> > 
> > I remember discussing that earlier when this stuff came in, and this I
> > believe is the reason for that.
> > 
> > We need to scale regulators before/after frequency because when we
> > increase the frequency a regulator may _not_ be providing enough power
> > to sustain that (even for a short while) and this may have undesired
> > effects on the hardware and so it is important to prevent that
> > malfunction.
> > 
> > In case of bandwidth such issues will not happen (AFAIK) and doing it
> > just once is normally enough. It is just about allowing more data to
> > be transmitted, and won't make the hardware behave badly.
> > 
> I agree with Dmitry. BW is a shared resource in a lot of architectures.
> Raising clk before increasing the bw can lead to a scenario where this
> client saturate the entire BW for whatever small duration it may be. This
> will impact the latency requirements of other clients.

I see. I will make the necessary changes then to fix it. Thanks guys.

-- 
viresh

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
@ 2021-01-28  4:14           ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-28  4:14 UTC (permalink / raw)
  To: Akhil P Oommen
  Cc: Nishanth Menon, Vincent Guittot, linux-pm, Stephen Boyd,
	Viresh Kumar, Rafael Wysocki, linux-kernel, Sibi Sankar,
	Dmitry Osipenko, linux-arm-kernel

On 27-01-21, 22:01, Akhil P Oommen wrote:
> On 1/22/2021 10:15 AM, Viresh Kumar wrote:
> > On 22-01-21, 00:41, Dmitry Osipenko wrote:
> > > 21.01.2021 14:17, Viresh Kumar пишет:
> > > > @@ -1074,15 +1091,18 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> > > >   	if (!ret) {
> > > >   		ret = _set_opp_bw(opp_table, opp, dev, false);
> > > > -		if (!ret)
> > > > +		if (!ret) {
> > > >   			opp_table->enabled = true;
> > > > +			dev_pm_opp_put(old_opp);
> > > > +
> > > > +			/* Make sure current_opp doesn't get freed */
> > > > +			dev_pm_opp_get(opp);
> > > > +			opp_table->current_opp = opp;
> > > > +		}
> > > >   	}
> > > 
> > > I'm a bit surprised that _set_opp_bw() isn't used similarly to
> > > _set_opp_voltage() in _generic_set_opp_regulator().
> > > 
> > > I'd expect the BW requirement to be raised before the clock rate goes UP.
> > 
> > I remember discussing that earlier when this stuff came in, and this I
> > believe is the reason for that.
> > 
> > We need to scale regulators before/after frequency because when we
> > increase the frequency a regulator may _not_ be providing enough power
> > to sustain that (even for a short while) and this may have undesired
> > effects on the hardware and so it is important to prevent that
> > malfunction.
> > 
> > In case of bandwidth such issues will not happen (AFAIK) and doing it
> > just once is normally enough. It is just about allowing more data to
> > be transmitted, and won't make the hardware behave badly.
> > 
> I agree with Dmitry. BW is a shared resource in a lot of architectures.
> Raising clk before increasing the bw can lead to a scenario where this
> client saturate the entire BW for whatever small duration it may be. This
> will impact the latency requirements of other clients.

I see. I will make the necessary changes then to fix it. Thanks guys.

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH V2 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
  2021-01-27 15:58       ` Dmitry Osipenko
@ 2021-01-28  7:01         ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-01-28  7:01 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Thierry Reding,
	Jonathan Hunter, linux-pm, Vincent Guittot, Rafael Wysocki,
	Stephen Boyd, Nishanth Menon, linux-tegra, linux-kernel

On 27-01-21, 18:58, Dmitry Osipenko wrote:
> Sadly this doesn't work because we missed that clk is assigned to
> opp_table when OPP table is allocated and not when it's added to device.

Ahh, I missed that.

I have bumped up the other patchset to V2, that should work fine with
this patch for tegra30 (this shouldn't require any update).

Everything is pushed in opp/linux-next, fetch and try it. Thanks.

-- 
viresh

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH V2 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
  2021-01-27  9:10   ` [PATCH V2 " Viresh Kumar
  2021-01-27 10:02     ` Viresh Kumar
@ 2021-02-01  0:21     ` Chanwoo Choi
  2021-02-01 19:57     ` Dmitry Osipenko
  2 siblings, 0 replies; 73+ messages in thread
From: Chanwoo Choi @ 2021-02-01  0:21 UTC (permalink / raw)
  To: Viresh Kumar, Dmitry Osipenko, MyungJoo Ham, Kyungmin Park,
	Thierry Reding, Jonathan Hunter
  Cc: linux-pm, Vincent Guittot, Rafael Wysocki, Stephen Boyd,
	Nishanth Menon, linux-tegra, linux-kernel

On 1/27/21 6:10 PM, Viresh Kumar wrote:
> dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
> be used instead. Migrate to the new API.
> 
> We don't want the OPP core to manage the clk for this driver, migrate to
> dev_pm_opp_of_add_table_noclk() to make sure dev_pm_opp_set_opp()
> doesn't have any side effects.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Dmitry,
> 
> This is based over the patches sent here:
> 
> https://protect2.fireeye.com/v1/url?k=72d88562-2d43bc78-72d90e2d-000babff24ad-e4764030101eaedc&q=1&e=a25b5db7-346b-47b2-9c0a-3945e579f389&u=https%3A%2F%2Flore.kernel.org%2Flkml%2F6c2160ff30a8f421563793020264cf9f533f293c.1611738228.git.viresh.kumar%40linaro.org%2F
> 
> This should fix the problem you mentioned earlier. Will push this for
> linux-next unless you have any issues with it.
> 
>  drivers/devfreq/tegra30-devfreq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 117cad7968ab..31f7dec5990b 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>  		return PTR_ERR(opp);
>  	}
>  
> -	ret = dev_pm_opp_set_bw(dev, opp);
> +	ret = dev_pm_opp_set_opp(dev, opp);
>  	dev_pm_opp_put(opp);
>  
>  	return ret;
> @@ -849,7 +849,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> -	err = dev_pm_opp_of_add_table(&pdev->dev);
> +	err = dev_pm_opp_of_add_table_noclk(&pdev->dev);
>  	if (err) {
>  		dev_err(&pdev->dev, "Failed to add OPP table: %d\n", err);
>  		goto put_hw;
> 

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

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH V2 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
  2021-01-27  9:10   ` [PATCH V2 " Viresh Kumar
  2021-01-27 10:02     ` Viresh Kumar
  2021-02-01  0:21     ` Chanwoo Choi
@ 2021-02-01 19:57     ` Dmitry Osipenko
  2 siblings, 0 replies; 73+ messages in thread
From: Dmitry Osipenko @ 2021-02-01 19:57 UTC (permalink / raw)
  To: Viresh Kumar, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
	Thierry Reding, Jonathan Hunter
  Cc: linux-pm, Vincent Guittot, Rafael Wysocki, Stephen Boyd,
	Nishanth Menon, linux-tegra, linux-kernel

27.01.2021 12:10, Viresh Kumar пишет:
> dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
> be used instead. Migrate to the new API.
> 
> We don't want the OPP core to manage the clk for this driver, migrate to
> dev_pm_opp_of_add_table_noclk() to make sure dev_pm_opp_set_opp()
> doesn't have any side effects.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Dmitry,
> 
> This is based over the patches sent here:
> 
> https://lore.kernel.org/lkml/6c2160ff30a8f421563793020264cf9f533f293c.1611738228.git.viresh.kumar@linaro.org/
> 
> This should fix the problem you mentioned earlier. Will push this for
> linux-next unless you have any issues with it.
> 
>  drivers/devfreq/tegra30-devfreq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 117cad7968ab..31f7dec5990b 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>  		return PTR_ERR(opp);
>  	}
>  
> -	ret = dev_pm_opp_set_bw(dev, opp);
> +	ret = dev_pm_opp_set_opp(dev, opp);
>  	dev_pm_opp_put(opp);
>  
>  	return ret;
> @@ -849,7 +849,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> -	err = dev_pm_opp_of_add_table(&pdev->dev);
> +	err = dev_pm_opp_of_add_table_noclk(&pdev->dev);
>  	if (err) {
>  		dev_err(&pdev->dev, "Failed to add OPP table: %d\n", err);
>  		goto put_hw;
> 

Tested-by: Dmitry Osipenko <digetx@gmail.com>

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
  2021-01-21 11:17   ` Viresh Kumar
@ 2021-07-07 10:24     ` Ionela Voinescu
  -1 siblings, 0 replies; 73+ messages in thread
From: Ionela Voinescu @ 2021-07-07 10:24 UTC (permalink / raw)
  To: Viresh Kumar, zhongkaihua
  Cc: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd,
	linux-pm, Vincent Guittot, Rafael Wysocki, Sibi Sankar,
	linux-kernel, linux-arm-kernel

Hi,

On Thursday 21 Jan 2021 at 16:47:43 (+0530), Viresh Kumar wrote:
> The dev_pm_opp_set_rate() helper needs to know the currently programmed
> OPP to make few decisions and currently we try to find it on every
> invocation of this routine.
> 
> Lets start keeping track of the current_opp programmed for the devices
> of the opp table, that will be quite useful going forward.
> 
> If we fail to find the current OPP, we pick the first one available in
> the list, as the list is in ascending order of frequencies, level, or
> bandwidth and that's the best guess we can make anyway.
> 
> Note that we used to do the frequency comparison a bit early in
> dev_pm_opp_set_rate() previously, and now instead we check the target
> opp, which shall be more accurate anyway.
> 
> We need to make sure that current_opp's memory doesn't get freed while
> it is being used and so we keep a reference of it until the time it is
> used.
> 
> Now that current_opp will always be set, we can drop some unnecessary
> checks as well.
> 

I'm seeing some intermittent issues on Hikey960 after this patch,
which reproduces as follows. I've used v5.13 for my testing.


root@buildroot:~# while true; do \
>     cd /sys/devices/system/cpu/cpufreq/; \
>     for policy in policy*; do \
>            cd /sys/devices/system/cpu/cpufreq/$policy; \
>            for freq in $(cat scaling_available_frequencies); do \
>                 echo "userspace" > scaling_governor; \
>                 sleep 1; \
>                 echo $freq > scaling_setspeed; \
>                 sleep 1; \
>                 cpu="${policy: -1}"; \
>                 mask="0x$(printf '%x\n' $((1 << $cpu)))"; \
>                 sysev=$(~/taskset $mask ~/sysbench run --test=cpu --max-time=1 | grep "total number of events"); \
>                 delivered=$(cat cpuinfo_cur_freq); \
>                 if [ "$freq" != "$delivered" ]; then \
>                         echo "CPU$cpu - $freq setting failed: delivered $delivered, sysevents: $sysev"; \
>                 else \
>                         echo "CPU$cpu - $freq setting succeeded: delivered $delivered, sysevents: $sysev"; \
>                 fi; \
>                 echo "schedutil" > scaling_governor; \
>                 sleep 1; \
>                 done; done; done;

CPU0 - 533000 setting succeeded: delivered 533000, sysevents:     total number of events:              112
CPU0 - 999000 setting succeeded: delivered 999000, sysevents:     total number of events:              209
CPU0 - 1402000 setting succeeded: delivered 1402000, sysevents:     total number of events:              293
CPU0 - 1709000 setting succeeded: delivered 1709000, sysevents:     total number of events:              357
CPU0 - 1844000 setting succeeded: delivered 1844000, sysevents:     total number of events:              385
CPU4 - 903000 setting succeeded: delivered 903000, sysevents:     total number of events:              249
CPU4 - 1421000 setting succeeded: delivered 1421000, sysevents:     total number of events:              395
CPU4 - 1805000 setting succeeded: delivered 1805000, sysevents:     total number of events:              502
CPU4 - 2112000 setting succeeded: delivered 2112000, sysevents:     total number of events:              588
CPU4 - 2362000 setting succeeded: delivered 2362000, sysevents:     total number of events:              657

This is an example of good behavior of changing frequencies. I'm putting
this here first to show the sysbench results for each frequency, which is
helping me make sure that the performance matches the new set frequency.

Notes: the change to the schedutil governor after each userspace driven
frequency change was added because if the change is always to higher
frequencies, the issue does not reproduce as easily; the sleep commands
are added just to make sure the change gets the time to take effect.

From time to time (7/400 fail rate), I get the following failures:

CPU0 - 533000 setting failed: delivered 1402000, sysevents:     total number of events:              293
CPU0 - 1402000 setting failed: delivered 533000, sysevents:     total number of events:              112
CPU0 - 1402000 setting failed: delivered 533000, sysevents:     total number of events:              112
CPU4 - 903000 setting failed: delivered 1421000, sysevents:     total number of events:              394
CPU4 - 1805000 setting failed: delivered 903000, sysevents:     total number of events:              249
CPU0 - 533000 setting failed: delivered 1402000, sysevents:     total number of events:              293
CPU4 - 1805000 setting failed: delivered 903000, sysevents:     total number of events:              251

Now comes the interesting part: what seems to fix it is a call to
clk_get_rate(opp_table->clk) in _set_opp(), which is what basically
happened before this patch, as _find_current_opp() was always called.
I do not need to do anything with the returned frequency.

Therefore, by adding:
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e366218d6736..2fdaf97f7ded 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -987,6 +987,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 {
        struct dev_pm_opp *old_opp;
        int scaling_down, ret;
+       unsigned long cur_freq;

        if (unlikely(!opp))
                return _disable_opp_table(dev, opp_table);
@@ -994,6 +995,13 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
        /* Find the currently set OPP if we don't know already */
        if (unlikely(!opp_table->current_opp))
                _find_current_opp(dev, opp_table);
+       else if (!IS_ERR(opp_table->clk)) {
+                       cur_freq = clk_get_rate(opp_table->clk);
+                       if (opp_table->current_rate != cur_freq)
+                               pr_err("OPP mismatch: %lu vs %lu!",
+                                      opp_table->current_rate,
+                                      cur_freq);
+               }

        old_opp = opp_table->current_opp;

.. it does seem to solve the problem (no failures in 1000 frequency changes),
although I do get a few OPP mismatch logs:

[  667.495112] core: OPP mismatch: 1709000000 vs 1402000000!
[ 7260.656154] core: OPP mismatch: 1421000000 vs 903000000!
[ 7260.727717] core: OPP mismatch: 903000000 vs 1421000000!
[ 8847.304323] core: OPP mismatch: 1709000000 vs 1402000000!


I'm not sure what is happening here so I'm hoping you guys have more
knowledge to steer debugging in the right direction.

To be noted that I'm running an equivalent variant of this test on
multiple boards, and none of them have issues, except for Hikey960.

Thanks,
Ionela.


> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/opp/core.c | 83 +++++++++++++++++++++++++++++-----------------
>  drivers/opp/opp.h  |  2 ++
>  2 files changed, 55 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index cb5b67ccf5cf..4ee598344e6a 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -788,8 +788,7 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
>  			__func__, old_freq);
>  restore_voltage:
>  	/* This shouldn't harm even if the voltages weren't updated earlier */
> -	if (old_supply)
> -		_set_opp_voltage(dev, reg, old_supply);
> +	_set_opp_voltage(dev, reg, old_supply);
>  
>  	return ret;
>  }
> @@ -839,10 +838,7 @@ static int _set_opp_custom(const struct opp_table *opp_table,
>  
>  	data->old_opp.rate = old_freq;
>  	size = sizeof(*old_supply) * opp_table->regulator_count;
> -	if (!old_supply)
> -		memset(data->old_opp.supplies, 0, size);
> -	else
> -		memcpy(data->old_opp.supplies, old_supply, size);
> +	memcpy(data->old_opp.supplies, old_supply, size);
>  
>  	data->new_opp.rate = freq;
>  	memcpy(data->new_opp.supplies, new_supply, size);
> @@ -943,6 +939,31 @@ int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_set_bw);
>  
> +static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
> +{
> +	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
> +	unsigned long freq;
> +
> +	if (!IS_ERR(opp_table->clk)) {
> +		freq = clk_get_rate(opp_table->clk);
> +		opp = _find_freq_ceil(opp_table, &freq);
> +	}
> +
> +	/*
> +	 * Unable to find the current OPP ? Pick the first from the list since
> +	 * it is in ascending order, otherwise rest of the code will need to
> +	 * make special checks to validate current_opp.
> +	 */
> +	if (IS_ERR(opp)) {
> +		mutex_lock(&opp_table->lock);
> +		opp = list_first_entry(&opp_table->opp_list, struct dev_pm_opp, node);
> +		dev_pm_opp_get(opp);
> +		mutex_unlock(&opp_table->lock);
> +	}
> +
> +	opp_table->current_opp = opp;
> +}
> +
>  static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
>  {
>  	int ret;
> @@ -1004,16 +1025,6 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  	if ((long)freq <= 0)
>  		freq = target_freq;
>  
> -	old_freq = clk_get_rate(opp_table->clk);
> -
> -	/* Return early if nothing to do */
> -	if (opp_table->enabled && old_freq == freq) {
> -		dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to do\n",
> -			__func__, freq);
> -		ret = 0;
> -		goto put_opp_table;
> -	}
> -
>  	/*
>  	 * For IO devices which require an OPP on some platforms/SoCs
>  	 * while just needing to scale the clock on some others
> @@ -1026,12 +1037,9 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  		goto put_opp_table;
>  	}
>  
> -	temp_freq = old_freq;
> -	old_opp = _find_freq_ceil(opp_table, &temp_freq);
> -	if (IS_ERR(old_opp)) {
> -		dev_err(dev, "%s: failed to find current OPP for freq %lu (%ld)\n",
> -			__func__, old_freq, PTR_ERR(old_opp));
> -	}
> +	/* Find the currently set OPP if we don't know already */
> +	if (unlikely(!opp_table->current_opp))
> +		_find_current_opp(dev, opp_table);
>  
>  	temp_freq = freq;
>  	opp = _find_freq_ceil(opp_table, &temp_freq);
> @@ -1039,7 +1047,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  		ret = PTR_ERR(opp);
>  		dev_err(dev, "%s: failed to find OPP for freq %lu (%d)\n",
>  			__func__, freq, ret);
> -		goto put_old_opp;
> +		goto put_opp_table;
> +	}
> +
> +	old_opp = opp_table->current_opp;
> +	old_freq = old_opp->rate;
> +
> +	/* Return early if nothing to do */
> +	if (opp_table->enabled && old_opp == opp) {
> +		dev_dbg(dev, "%s: OPPs are same, nothing to do\n", __func__);
> +		ret = 0;
> +		goto put_opp;
>  	}
>  
>  	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__,
> @@ -1054,11 +1072,10 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  
>  	if (opp_table->set_opp) {
>  		ret = _set_opp_custom(opp_table, dev, old_freq, freq,
> -				      IS_ERR(old_opp) ? NULL : old_opp->supplies,
> -				      opp->supplies);
> +				      old_opp->supplies, opp->supplies);
>  	} else if (opp_table->regulators) {
>  		ret = _generic_set_opp_regulator(opp_table, dev, old_freq, freq,
> -						 IS_ERR(old_opp) ? NULL : old_opp->supplies,
> +						 old_opp->supplies,
>  						 opp->supplies);
>  	} else {
>  		/* Only frequency scaling */
> @@ -1074,15 +1091,18 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  
>  	if (!ret) {
>  		ret = _set_opp_bw(opp_table, opp, dev, false);
> -		if (!ret)
> +		if (!ret) {
>  			opp_table->enabled = true;
> +			dev_pm_opp_put(old_opp);
> +
> +			/* Make sure current_opp doesn't get freed */
> +			dev_pm_opp_get(opp);
> +			opp_table->current_opp = opp;
> +		}
>  	}
>  
>  put_opp:
>  	dev_pm_opp_put(opp);
> -put_old_opp:
> -	if (!IS_ERR(old_opp))
> -		dev_pm_opp_put(old_opp);
>  put_opp_table:
>  	dev_pm_opp_put_opp_table(opp_table);
>  	return ret;
> @@ -1276,6 +1296,9 @@ static void _opp_table_kref_release(struct kref *kref)
>  	list_del(&opp_table->node);
>  	mutex_unlock(&opp_table_lock);
>  
> +	if (opp_table->current_opp)
> +		dev_pm_opp_put(opp_table->current_opp);
> +
>  	_of_clear_opp_table(opp_table);
>  
>  	/* Release clk */
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 4408cfcb0f31..359fd89d5770 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -135,6 +135,7 @@ enum opp_table_access {
>   * @clock_latency_ns_max: Max clock latency in nanoseconds.
>   * @parsed_static_opps: Count of devices for which OPPs are initialized from DT.
>   * @shared_opp: OPP is shared between multiple devices.
> + * @current_opp: Currently configured OPP for the table.
>   * @suspend_opp: Pointer to OPP to be used during device suspend.
>   * @genpd_virt_dev_lock: Mutex protecting the genpd virtual device pointers.
>   * @genpd_virt_devs: List of virtual devices for multiple genpd support.
> @@ -183,6 +184,7 @@ struct opp_table {
>  
>  	unsigned int parsed_static_opps;
>  	enum opp_table_access shared_opp;
> +	struct dev_pm_opp *current_opp;
>  	struct dev_pm_opp *suspend_opp;
>  
>  	struct mutex genpd_virt_dev_lock;
> -- 
> 2.25.0.rc1.19.g042ed3e048af
> 
> 

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
@ 2021-07-07 10:24     ` Ionela Voinescu
  0 siblings, 0 replies; 73+ messages in thread
From: Ionela Voinescu @ 2021-07-07 10:24 UTC (permalink / raw)
  To: Viresh Kumar, zhongkaihua
  Cc: Dmitry Osipenko, Viresh Kumar, Nishanth Menon, Stephen Boyd,
	linux-pm, Vincent Guittot, Rafael Wysocki, Sibi Sankar,
	linux-kernel, linux-arm-kernel

Hi,

On Thursday 21 Jan 2021 at 16:47:43 (+0530), Viresh Kumar wrote:
> The dev_pm_opp_set_rate() helper needs to know the currently programmed
> OPP to make few decisions and currently we try to find it on every
> invocation of this routine.
> 
> Lets start keeping track of the current_opp programmed for the devices
> of the opp table, that will be quite useful going forward.
> 
> If we fail to find the current OPP, we pick the first one available in
> the list, as the list is in ascending order of frequencies, level, or
> bandwidth and that's the best guess we can make anyway.
> 
> Note that we used to do the frequency comparison a bit early in
> dev_pm_opp_set_rate() previously, and now instead we check the target
> opp, which shall be more accurate anyway.
> 
> We need to make sure that current_opp's memory doesn't get freed while
> it is being used and so we keep a reference of it until the time it is
> used.
> 
> Now that current_opp will always be set, we can drop some unnecessary
> checks as well.
> 

I'm seeing some intermittent issues on Hikey960 after this patch,
which reproduces as follows. I've used v5.13 for my testing.


root@buildroot:~# while true; do \
>     cd /sys/devices/system/cpu/cpufreq/; \
>     for policy in policy*; do \
>            cd /sys/devices/system/cpu/cpufreq/$policy; \
>            for freq in $(cat scaling_available_frequencies); do \
>                 echo "userspace" > scaling_governor; \
>                 sleep 1; \
>                 echo $freq > scaling_setspeed; \
>                 sleep 1; \
>                 cpu="${policy: -1}"; \
>                 mask="0x$(printf '%x\n' $((1 << $cpu)))"; \
>                 sysev=$(~/taskset $mask ~/sysbench run --test=cpu --max-time=1 | grep "total number of events"); \
>                 delivered=$(cat cpuinfo_cur_freq); \
>                 if [ "$freq" != "$delivered" ]; then \
>                         echo "CPU$cpu - $freq setting failed: delivered $delivered, sysevents: $sysev"; \
>                 else \
>                         echo "CPU$cpu - $freq setting succeeded: delivered $delivered, sysevents: $sysev"; \
>                 fi; \
>                 echo "schedutil" > scaling_governor; \
>                 sleep 1; \
>                 done; done; done;

CPU0 - 533000 setting succeeded: delivered 533000, sysevents:     total number of events:              112
CPU0 - 999000 setting succeeded: delivered 999000, sysevents:     total number of events:              209
CPU0 - 1402000 setting succeeded: delivered 1402000, sysevents:     total number of events:              293
CPU0 - 1709000 setting succeeded: delivered 1709000, sysevents:     total number of events:              357
CPU0 - 1844000 setting succeeded: delivered 1844000, sysevents:     total number of events:              385
CPU4 - 903000 setting succeeded: delivered 903000, sysevents:     total number of events:              249
CPU4 - 1421000 setting succeeded: delivered 1421000, sysevents:     total number of events:              395
CPU4 - 1805000 setting succeeded: delivered 1805000, sysevents:     total number of events:              502
CPU4 - 2112000 setting succeeded: delivered 2112000, sysevents:     total number of events:              588
CPU4 - 2362000 setting succeeded: delivered 2362000, sysevents:     total number of events:              657

This is an example of good behavior of changing frequencies. I'm putting
this here first to show the sysbench results for each frequency, which is
helping me make sure that the performance matches the new set frequency.

Notes: the change to the schedutil governor after each userspace driven
frequency change was added because if the change is always to higher
frequencies, the issue does not reproduce as easily; the sleep commands
are added just to make sure the change gets the time to take effect.

From time to time (7/400 fail rate), I get the following failures:

CPU0 - 533000 setting failed: delivered 1402000, sysevents:     total number of events:              293
CPU0 - 1402000 setting failed: delivered 533000, sysevents:     total number of events:              112
CPU0 - 1402000 setting failed: delivered 533000, sysevents:     total number of events:              112
CPU4 - 903000 setting failed: delivered 1421000, sysevents:     total number of events:              394
CPU4 - 1805000 setting failed: delivered 903000, sysevents:     total number of events:              249
CPU0 - 533000 setting failed: delivered 1402000, sysevents:     total number of events:              293
CPU4 - 1805000 setting failed: delivered 903000, sysevents:     total number of events:              251

Now comes the interesting part: what seems to fix it is a call to
clk_get_rate(opp_table->clk) in _set_opp(), which is what basically
happened before this patch, as _find_current_opp() was always called.
I do not need to do anything with the returned frequency.

Therefore, by adding:
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e366218d6736..2fdaf97f7ded 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -987,6 +987,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 {
        struct dev_pm_opp *old_opp;
        int scaling_down, ret;
+       unsigned long cur_freq;

        if (unlikely(!opp))
                return _disable_opp_table(dev, opp_table);
@@ -994,6 +995,13 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
        /* Find the currently set OPP if we don't know already */
        if (unlikely(!opp_table->current_opp))
                _find_current_opp(dev, opp_table);
+       else if (!IS_ERR(opp_table->clk)) {
+                       cur_freq = clk_get_rate(opp_table->clk);
+                       if (opp_table->current_rate != cur_freq)
+                               pr_err("OPP mismatch: %lu vs %lu!",
+                                      opp_table->current_rate,
+                                      cur_freq);
+               }

        old_opp = opp_table->current_opp;

.. it does seem to solve the problem (no failures in 1000 frequency changes),
although I do get a few OPP mismatch logs:

[  667.495112] core: OPP mismatch: 1709000000 vs 1402000000!
[ 7260.656154] core: OPP mismatch: 1421000000 vs 903000000!
[ 7260.727717] core: OPP mismatch: 903000000 vs 1421000000!
[ 8847.304323] core: OPP mismatch: 1709000000 vs 1402000000!


I'm not sure what is happening here so I'm hoping you guys have more
knowledge to steer debugging in the right direction.

To be noted that I'm running an equivalent variant of this test on
multiple boards, and none of them have issues, except for Hikey960.

Thanks,
Ionela.


> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/opp/core.c | 83 +++++++++++++++++++++++++++++-----------------
>  drivers/opp/opp.h  |  2 ++
>  2 files changed, 55 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index cb5b67ccf5cf..4ee598344e6a 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -788,8 +788,7 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
>  			__func__, old_freq);
>  restore_voltage:
>  	/* This shouldn't harm even if the voltages weren't updated earlier */
> -	if (old_supply)
> -		_set_opp_voltage(dev, reg, old_supply);
> +	_set_opp_voltage(dev, reg, old_supply);
>  
>  	return ret;
>  }
> @@ -839,10 +838,7 @@ static int _set_opp_custom(const struct opp_table *opp_table,
>  
>  	data->old_opp.rate = old_freq;
>  	size = sizeof(*old_supply) * opp_table->regulator_count;
> -	if (!old_supply)
> -		memset(data->old_opp.supplies, 0, size);
> -	else
> -		memcpy(data->old_opp.supplies, old_supply, size);
> +	memcpy(data->old_opp.supplies, old_supply, size);
>  
>  	data->new_opp.rate = freq;
>  	memcpy(data->new_opp.supplies, new_supply, size);
> @@ -943,6 +939,31 @@ int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_set_bw);
>  
> +static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
> +{
> +	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
> +	unsigned long freq;
> +
> +	if (!IS_ERR(opp_table->clk)) {
> +		freq = clk_get_rate(opp_table->clk);
> +		opp = _find_freq_ceil(opp_table, &freq);
> +	}
> +
> +	/*
> +	 * Unable to find the current OPP ? Pick the first from the list since
> +	 * it is in ascending order, otherwise rest of the code will need to
> +	 * make special checks to validate current_opp.
> +	 */
> +	if (IS_ERR(opp)) {
> +		mutex_lock(&opp_table->lock);
> +		opp = list_first_entry(&opp_table->opp_list, struct dev_pm_opp, node);
> +		dev_pm_opp_get(opp);
> +		mutex_unlock(&opp_table->lock);
> +	}
> +
> +	opp_table->current_opp = opp;
> +}
> +
>  static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
>  {
>  	int ret;
> @@ -1004,16 +1025,6 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  	if ((long)freq <= 0)
>  		freq = target_freq;
>  
> -	old_freq = clk_get_rate(opp_table->clk);
> -
> -	/* Return early if nothing to do */
> -	if (opp_table->enabled && old_freq == freq) {
> -		dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to do\n",
> -			__func__, freq);
> -		ret = 0;
> -		goto put_opp_table;
> -	}
> -
>  	/*
>  	 * For IO devices which require an OPP on some platforms/SoCs
>  	 * while just needing to scale the clock on some others
> @@ -1026,12 +1037,9 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  		goto put_opp_table;
>  	}
>  
> -	temp_freq = old_freq;
> -	old_opp = _find_freq_ceil(opp_table, &temp_freq);
> -	if (IS_ERR(old_opp)) {
> -		dev_err(dev, "%s: failed to find current OPP for freq %lu (%ld)\n",
> -			__func__, old_freq, PTR_ERR(old_opp));
> -	}
> +	/* Find the currently set OPP if we don't know already */
> +	if (unlikely(!opp_table->current_opp))
> +		_find_current_opp(dev, opp_table);
>  
>  	temp_freq = freq;
>  	opp = _find_freq_ceil(opp_table, &temp_freq);
> @@ -1039,7 +1047,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  		ret = PTR_ERR(opp);
>  		dev_err(dev, "%s: failed to find OPP for freq %lu (%d)\n",
>  			__func__, freq, ret);
> -		goto put_old_opp;
> +		goto put_opp_table;
> +	}
> +
> +	old_opp = opp_table->current_opp;
> +	old_freq = old_opp->rate;
> +
> +	/* Return early if nothing to do */
> +	if (opp_table->enabled && old_opp == opp) {
> +		dev_dbg(dev, "%s: OPPs are same, nothing to do\n", __func__);
> +		ret = 0;
> +		goto put_opp;
>  	}
>  
>  	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__,
> @@ -1054,11 +1072,10 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  
>  	if (opp_table->set_opp) {
>  		ret = _set_opp_custom(opp_table, dev, old_freq, freq,
> -				      IS_ERR(old_opp) ? NULL : old_opp->supplies,
> -				      opp->supplies);
> +				      old_opp->supplies, opp->supplies);
>  	} else if (opp_table->regulators) {
>  		ret = _generic_set_opp_regulator(opp_table, dev, old_freq, freq,
> -						 IS_ERR(old_opp) ? NULL : old_opp->supplies,
> +						 old_opp->supplies,
>  						 opp->supplies);
>  	} else {
>  		/* Only frequency scaling */
> @@ -1074,15 +1091,18 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  
>  	if (!ret) {
>  		ret = _set_opp_bw(opp_table, opp, dev, false);
> -		if (!ret)
> +		if (!ret) {
>  			opp_table->enabled = true;
> +			dev_pm_opp_put(old_opp);
> +
> +			/* Make sure current_opp doesn't get freed */
> +			dev_pm_opp_get(opp);
> +			opp_table->current_opp = opp;
> +		}
>  	}
>  
>  put_opp:
>  	dev_pm_opp_put(opp);
> -put_old_opp:
> -	if (!IS_ERR(old_opp))
> -		dev_pm_opp_put(old_opp);
>  put_opp_table:
>  	dev_pm_opp_put_opp_table(opp_table);
>  	return ret;
> @@ -1276,6 +1296,9 @@ static void _opp_table_kref_release(struct kref *kref)
>  	list_del(&opp_table->node);
>  	mutex_unlock(&opp_table_lock);
>  
> +	if (opp_table->current_opp)
> +		dev_pm_opp_put(opp_table->current_opp);
> +
>  	_of_clear_opp_table(opp_table);
>  
>  	/* Release clk */
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 4408cfcb0f31..359fd89d5770 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -135,6 +135,7 @@ enum opp_table_access {
>   * @clock_latency_ns_max: Max clock latency in nanoseconds.
>   * @parsed_static_opps: Count of devices for which OPPs are initialized from DT.
>   * @shared_opp: OPP is shared between multiple devices.
> + * @current_opp: Currently configured OPP for the table.
>   * @suspend_opp: Pointer to OPP to be used during device suspend.
>   * @genpd_virt_dev_lock: Mutex protecting the genpd virtual device pointers.
>   * @genpd_virt_devs: List of virtual devices for multiple genpd support.
> @@ -183,6 +184,7 @@ struct opp_table {
>  
>  	unsigned int parsed_static_opps;
>  	enum opp_table_access shared_opp;
> +	struct dev_pm_opp *current_opp;
>  	struct dev_pm_opp *suspend_opp;
>  
>  	struct mutex genpd_virt_dev_lock;
> -- 
> 2.25.0.rc1.19.g042ed3e048af
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
  2021-07-07 10:24     ` Ionela Voinescu
@ 2021-07-08  7:53       ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-07-08  7:53 UTC (permalink / raw)
  To: Ionela Voinescu, Kevin Wangtao, Leo Yan, Jassi Brar
  Cc: zhongkaihua, Dmitry Osipenko, Viresh Kumar, Nishanth Menon,
	Stephen Boyd, linux-pm, Vincent Guittot, Rafael Wysocki,
	Sibi Sankar, linux-kernel, linux-arm-kernel

On 07-07-21, 11:24, Ionela Voinescu wrote:
> Now comes the interesting part: what seems to fix it is a call to
> clk_get_rate(opp_table->clk) in _set_opp(), which is what basically
> happened before this patch, as _find_current_opp() was always called.
> I do not need to do anything with the returned frequency.

Wow, thanks for narrowing it down this far :)

I had a quick look and this is what I think is the problem here.

This platform uses mailbox API to send its frequency change requests to another
processor.  And the way it is written currently, I don't see any guarantee
whatsoever which say

  "once clk_set_rate() returns, the frequency would have already changed".

And this may exactly be the thing you are able to hit, luckily because of this
patchset :)

As a quick way of checking if that is right or not, this may make it work:

diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c
index 395ddc250828..9856c1c84dcf 100644
--- a/drivers/mailbox/hi3660-mailbox.c
+++ b/drivers/mailbox/hi3660-mailbox.c
@@ -201,6 +201,9 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)

        /* Trigger data transferring */
        writel(BIT(mchan->ack_irq), base + MBOX_SEND_REG);
+
+       hi3660_mbox_check_state(chan);
+
        return 0;
 }

-------------------------8<-------------------------

As a proper fix, something like this (not even compile tested) is required I
believe as I don't see the clients would know if the transfer is over. Cc'ing
mailbox guys to see what can be done.

diff --git a/drivers/clk/hisilicon/clk-hi3660-stub.c b/drivers/clk/hisilicon/clk-hi3660-stub.c
index 3a653d54bee0..c1e62ea4cf01 100644
--- a/drivers/clk/hisilicon/clk-hi3660-stub.c
+++ b/drivers/clk/hisilicon/clk-hi3660-stub.c
@@ -89,7 +89,6 @@ static int hi3660_stub_clk_set_rate(struct clk_hw *hw, unsigned long rate,
                stub_clk->msg[0], stub_clk->msg[1]);
 
        mbox_send_message(stub_clk_chan.mbox, stub_clk->msg);
-       mbox_client_txdone(stub_clk_chan.mbox, 0);
 
        stub_clk->rate = rate;
        return 0;
@@ -131,7 +130,7 @@ static int hi3660_stub_clk_probe(struct platform_device *pdev)
        /* Use mailbox client without blocking */
        stub_clk_chan.cl.dev = dev;
        stub_clk_chan.cl.tx_done = NULL;
-       stub_clk_chan.cl.tx_block = false;
+       stub_clk_chan.cl.tx_block = true;
        stub_clk_chan.cl.knows_txdone = false;
 
        /* Allocate mailbox channel */
diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c
index 395ddc250828..8f6b787c0aba 100644
--- a/drivers/mailbox/hi3660-mailbox.c
+++ b/drivers/mailbox/hi3660-mailbox.c
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
-// Copyright (c) 2017-2018 HiSilicon Limited.
+// Copyright (c) 2017-2018 Hisilicon Limited.
 // Copyright (c) 2017-2018 Linaro Limited.
 
 #include <linux/bitops.h>
@@ -83,7 +83,7 @@ static struct hi3660_mbox *to_hi3660_mbox(struct mbox_controller *mbox)
        return container_of(mbox, struct hi3660_mbox, controller);
 }
 
-static int hi3660_mbox_check_state(struct mbox_chan *chan)
+static bool hi3660_mbox_last_tx_done(struct mbox_chan *chan)
 {
        unsigned long ch = (unsigned long)chan->con_priv;
        struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox);
@@ -94,20 +94,20 @@ static int hi3660_mbox_check_state(struct mbox_chan *chan)
 
        /* Mailbox is ready to use */
        if (readl(base + MBOX_MODE_REG) & MBOX_STATE_READY)
-               return 0;
+               return true;
 
        /* Wait for acknowledge from remote */
        ret = readx_poll_timeout_atomic(readl, base + MBOX_MODE_REG,
                        val, (val & MBOX_STATE_ACK), 1000, 300000);
        if (ret) {
                dev_err(mbox->dev, "%s: timeout for receiving ack\n", __func__);
-               return ret;
+               return false;
        }
 
        /* clear ack state, mailbox will get back to ready state */
        writel(BIT(mchan->ack_irq), base + MBOX_ICLR_REG);
 
-       return 0;
+       return true;
 }
 
 static int hi3660_mbox_unlock(struct mbox_chan *chan)
@@ -182,10 +182,6 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
        unsigned int i;
        int ret;
 
-       ret = hi3660_mbox_check_state(chan);
-       if (ret)
-               return ret;
-
        /* Clear mask for destination interrupt */
        writel_relaxed(~BIT(mchan->dst_irq), base + MBOX_IMASK_REG);
 
@@ -207,6 +203,7 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
 static const struct mbox_chan_ops hi3660_mbox_ops = {
        .startup        = hi3660_mbox_startup,
        .send_data      = hi3660_mbox_send_data,
+       .last_tx_done   = hi3660_mbox_last_tx_done,
 };
 
 static struct mbox_chan *hi3660_mbox_xlate(struct mbox_controller *controller,
@@ -259,6 +256,7 @@ static int hi3660_mbox_probe(struct platform_device *pdev)
        mbox->controller.num_chans = MBOX_CHAN_MAX;
        mbox->controller.ops = &hi3660_mbox_ops;
        mbox->controller.of_xlate = hi3660_mbox_xlate;
+       mbox->controller.txdone_poll = true;
 
        /* Initialize mailbox channel data */
        chan = mbox->chan;

-- 
viresh

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
@ 2021-07-08  7:53       ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-07-08  7:53 UTC (permalink / raw)
  To: Ionela Voinescu, Kevin Wangtao, Leo Yan, Jassi Brar
  Cc: zhongkaihua, Dmitry Osipenko, Viresh Kumar, Nishanth Menon,
	Stephen Boyd, linux-pm, Vincent Guittot, Rafael Wysocki,
	Sibi Sankar, linux-kernel, linux-arm-kernel

On 07-07-21, 11:24, Ionela Voinescu wrote:
> Now comes the interesting part: what seems to fix it is a call to
> clk_get_rate(opp_table->clk) in _set_opp(), which is what basically
> happened before this patch, as _find_current_opp() was always called.
> I do not need to do anything with the returned frequency.

Wow, thanks for narrowing it down this far :)

I had a quick look and this is what I think is the problem here.

This platform uses mailbox API to send its frequency change requests to another
processor.  And the way it is written currently, I don't see any guarantee
whatsoever which say

  "once clk_set_rate() returns, the frequency would have already changed".

And this may exactly be the thing you are able to hit, luckily because of this
patchset :)

As a quick way of checking if that is right or not, this may make it work:

diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c
index 395ddc250828..9856c1c84dcf 100644
--- a/drivers/mailbox/hi3660-mailbox.c
+++ b/drivers/mailbox/hi3660-mailbox.c
@@ -201,6 +201,9 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)

        /* Trigger data transferring */
        writel(BIT(mchan->ack_irq), base + MBOX_SEND_REG);
+
+       hi3660_mbox_check_state(chan);
+
        return 0;
 }

-------------------------8<-------------------------

As a proper fix, something like this (not even compile tested) is required I
believe as I don't see the clients would know if the transfer is over. Cc'ing
mailbox guys to see what can be done.

diff --git a/drivers/clk/hisilicon/clk-hi3660-stub.c b/drivers/clk/hisilicon/clk-hi3660-stub.c
index 3a653d54bee0..c1e62ea4cf01 100644
--- a/drivers/clk/hisilicon/clk-hi3660-stub.c
+++ b/drivers/clk/hisilicon/clk-hi3660-stub.c
@@ -89,7 +89,6 @@ static int hi3660_stub_clk_set_rate(struct clk_hw *hw, unsigned long rate,
                stub_clk->msg[0], stub_clk->msg[1]);
 
        mbox_send_message(stub_clk_chan.mbox, stub_clk->msg);
-       mbox_client_txdone(stub_clk_chan.mbox, 0);
 
        stub_clk->rate = rate;
        return 0;
@@ -131,7 +130,7 @@ static int hi3660_stub_clk_probe(struct platform_device *pdev)
        /* Use mailbox client without blocking */
        stub_clk_chan.cl.dev = dev;
        stub_clk_chan.cl.tx_done = NULL;
-       stub_clk_chan.cl.tx_block = false;
+       stub_clk_chan.cl.tx_block = true;
        stub_clk_chan.cl.knows_txdone = false;
 
        /* Allocate mailbox channel */
diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c
index 395ddc250828..8f6b787c0aba 100644
--- a/drivers/mailbox/hi3660-mailbox.c
+++ b/drivers/mailbox/hi3660-mailbox.c
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
-// Copyright (c) 2017-2018 HiSilicon Limited.
+// Copyright (c) 2017-2018 Hisilicon Limited.
 // Copyright (c) 2017-2018 Linaro Limited.
 
 #include <linux/bitops.h>
@@ -83,7 +83,7 @@ static struct hi3660_mbox *to_hi3660_mbox(struct mbox_controller *mbox)
        return container_of(mbox, struct hi3660_mbox, controller);
 }
 
-static int hi3660_mbox_check_state(struct mbox_chan *chan)
+static bool hi3660_mbox_last_tx_done(struct mbox_chan *chan)
 {
        unsigned long ch = (unsigned long)chan->con_priv;
        struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox);
@@ -94,20 +94,20 @@ static int hi3660_mbox_check_state(struct mbox_chan *chan)
 
        /* Mailbox is ready to use */
        if (readl(base + MBOX_MODE_REG) & MBOX_STATE_READY)
-               return 0;
+               return true;
 
        /* Wait for acknowledge from remote */
        ret = readx_poll_timeout_atomic(readl, base + MBOX_MODE_REG,
                        val, (val & MBOX_STATE_ACK), 1000, 300000);
        if (ret) {
                dev_err(mbox->dev, "%s: timeout for receiving ack\n", __func__);
-               return ret;
+               return false;
        }
 
        /* clear ack state, mailbox will get back to ready state */
        writel(BIT(mchan->ack_irq), base + MBOX_ICLR_REG);
 
-       return 0;
+       return true;
 }
 
 static int hi3660_mbox_unlock(struct mbox_chan *chan)
@@ -182,10 +182,6 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
        unsigned int i;
        int ret;
 
-       ret = hi3660_mbox_check_state(chan);
-       if (ret)
-               return ret;
-
        /* Clear mask for destination interrupt */
        writel_relaxed(~BIT(mchan->dst_irq), base + MBOX_IMASK_REG);
 
@@ -207,6 +203,7 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
 static const struct mbox_chan_ops hi3660_mbox_ops = {
        .startup        = hi3660_mbox_startup,
        .send_data      = hi3660_mbox_send_data,
+       .last_tx_done   = hi3660_mbox_last_tx_done,
 };
 
 static struct mbox_chan *hi3660_mbox_xlate(struct mbox_controller *controller,
@@ -259,6 +256,7 @@ static int hi3660_mbox_probe(struct platform_device *pdev)
        mbox->controller.num_chans = MBOX_CHAN_MAX;
        mbox->controller.ops = &hi3660_mbox_ops;
        mbox->controller.of_xlate = hi3660_mbox_xlate;
+       mbox->controller.txdone_poll = true;
 
        /* Initialize mailbox channel data */
        chan = mbox->chan;

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
  2021-07-08  7:53       ` Viresh Kumar
@ 2021-07-09  8:57         ` Ionela Voinescu
  -1 siblings, 0 replies; 73+ messages in thread
From: Ionela Voinescu @ 2021-07-09  8:57 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Kevin Wangtao, Leo Yan, Jassi Brar, zhongkaihua, Dmitry Osipenko,
	Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
	Vincent Guittot, Rafael Wysocki, Sibi Sankar, linux-kernel,
	linux-arm-kernel

Hi Viresh,

On Thursday 08 Jul 2021 at 13:23:53 (+0530), Viresh Kumar wrote:
> On 07-07-21, 11:24, Ionela Voinescu wrote:
> > Now comes the interesting part: what seems to fix it is a call to
> > clk_get_rate(opp_table->clk) in _set_opp(), which is what basically
> > happened before this patch, as _find_current_opp() was always called.
> > I do not need to do anything with the returned frequency.
> 
> Wow, thanks for narrowing it down this far :)
> 
> I had a quick look and this is what I think is the problem here.
> 
> This platform uses mailbox API to send its frequency change requests to another
> processor.  And the way it is written currently, I don't see any guarantee
> whatsoever which say
> 
>   "once clk_set_rate() returns, the frequency would have already changed".
> 

I think what was strange to me was that the frequency never seems to
change, there isn't just a delay in the new frequency taking effect, as
I would expect in these cases. Or if there is a delay, that's quite large
- at least a second.

> And this may exactly be the thing you are able to hit, luckily because of this
> patchset :)
> 
> As a quick way of checking if that is right or not, this may make it work:
> 
> diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c
> index 395ddc250828..9856c1c84dcf 100644
> --- a/drivers/mailbox/hi3660-mailbox.c
> +++ b/drivers/mailbox/hi3660-mailbox.c
> @@ -201,6 +201,9 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
> 
>         /* Trigger data transferring */
>         writel(BIT(mchan->ack_irq), base + MBOX_SEND_REG);
> +
> +       hi3660_mbox_check_state(chan);
> +

I gave this a try an it does work for me.

>         return 0;
>  }
> 
> -------------------------8<-------------------------
> 
> As a proper fix, something like this (not even compile tested) is required I
> believe as I don't see the clients would know if the transfer is over. Cc'ing
> mailbox guys to see what can be done.
> 

I'll give this a try as well when there is consensus. I might even try to
review it, if the time allows.

Many thanks,
Ionela.

> diff --git a/drivers/clk/hisilicon/clk-hi3660-stub.c b/drivers/clk/hisilicon/clk-hi3660-stub.c
> index 3a653d54bee0..c1e62ea4cf01 100644
> --- a/drivers/clk/hisilicon/clk-hi3660-stub.c
> +++ b/drivers/clk/hisilicon/clk-hi3660-stub.c
> @@ -89,7 +89,6 @@ static int hi3660_stub_clk_set_rate(struct clk_hw *hw, unsigned long rate,
>                 stub_clk->msg[0], stub_clk->msg[1]);
>  
>         mbox_send_message(stub_clk_chan.mbox, stub_clk->msg);
> -       mbox_client_txdone(stub_clk_chan.mbox, 0);
>  
>         stub_clk->rate = rate;
>         return 0;
> @@ -131,7 +130,7 @@ static int hi3660_stub_clk_probe(struct platform_device *pdev)
>         /* Use mailbox client without blocking */
>         stub_clk_chan.cl.dev = dev;
>         stub_clk_chan.cl.tx_done = NULL;
> -       stub_clk_chan.cl.tx_block = false;
> +       stub_clk_chan.cl.tx_block = true;
>         stub_clk_chan.cl.knows_txdone = false;
>  
>         /* Allocate mailbox channel */
> diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c
> index 395ddc250828..8f6b787c0aba 100644
> --- a/drivers/mailbox/hi3660-mailbox.c
> +++ b/drivers/mailbox/hi3660-mailbox.c
> @@ -1,5 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0
> -// Copyright (c) 2017-2018 HiSilicon Limited.
> +// Copyright (c) 2017-2018 Hisilicon Limited.
>  // Copyright (c) 2017-2018 Linaro Limited.
>  
>  #include <linux/bitops.h>
> @@ -83,7 +83,7 @@ static struct hi3660_mbox *to_hi3660_mbox(struct mbox_controller *mbox)
>         return container_of(mbox, struct hi3660_mbox, controller);
>  }
>  
> -static int hi3660_mbox_check_state(struct mbox_chan *chan)
> +static bool hi3660_mbox_last_tx_done(struct mbox_chan *chan)
>  {
>         unsigned long ch = (unsigned long)chan->con_priv;
>         struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox);
> @@ -94,20 +94,20 @@ static int hi3660_mbox_check_state(struct mbox_chan *chan)
>  
>         /* Mailbox is ready to use */
>         if (readl(base + MBOX_MODE_REG) & MBOX_STATE_READY)
> -               return 0;
> +               return true;
>  
>         /* Wait for acknowledge from remote */
>         ret = readx_poll_timeout_atomic(readl, base + MBOX_MODE_REG,
>                         val, (val & MBOX_STATE_ACK), 1000, 300000);
>         if (ret) {
>                 dev_err(mbox->dev, "%s: timeout for receiving ack\n", __func__);
> -               return ret;
> +               return false;
>         }
>  
>         /* clear ack state, mailbox will get back to ready state */
>         writel(BIT(mchan->ack_irq), base + MBOX_ICLR_REG);
>  
> -       return 0;
> +       return true;
>  }
>  
>  static int hi3660_mbox_unlock(struct mbox_chan *chan)
> @@ -182,10 +182,6 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
>         unsigned int i;
>         int ret;
>  
> -       ret = hi3660_mbox_check_state(chan);
> -       if (ret)
> -               return ret;
> -
>         /* Clear mask for destination interrupt */
>         writel_relaxed(~BIT(mchan->dst_irq), base + MBOX_IMASK_REG);
>  
> @@ -207,6 +203,7 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
>  static const struct mbox_chan_ops hi3660_mbox_ops = {
>         .startup        = hi3660_mbox_startup,
>         .send_data      = hi3660_mbox_send_data,
> +       .last_tx_done   = hi3660_mbox_last_tx_done,
>  };
>  
>  static struct mbox_chan *hi3660_mbox_xlate(struct mbox_controller *controller,
> @@ -259,6 +256,7 @@ static int hi3660_mbox_probe(struct platform_device *pdev)
>         mbox->controller.num_chans = MBOX_CHAN_MAX;
>         mbox->controller.ops = &hi3660_mbox_ops;
>         mbox->controller.of_xlate = hi3660_mbox_xlate;
> +       mbox->controller.txdone_poll = true;
>  
>         /* Initialize mailbox channel data */
>         chan = mbox->chan;
> 
> -- 
> viresh

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
@ 2021-07-09  8:57         ` Ionela Voinescu
  0 siblings, 0 replies; 73+ messages in thread
From: Ionela Voinescu @ 2021-07-09  8:57 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Kevin Wangtao, Leo Yan, Jassi Brar, zhongkaihua, Dmitry Osipenko,
	Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
	Vincent Guittot, Rafael Wysocki, Sibi Sankar, linux-kernel,
	linux-arm-kernel

Hi Viresh,

On Thursday 08 Jul 2021 at 13:23:53 (+0530), Viresh Kumar wrote:
> On 07-07-21, 11:24, Ionela Voinescu wrote:
> > Now comes the interesting part: what seems to fix it is a call to
> > clk_get_rate(opp_table->clk) in _set_opp(), which is what basically
> > happened before this patch, as _find_current_opp() was always called.
> > I do not need to do anything with the returned frequency.
> 
> Wow, thanks for narrowing it down this far :)
> 
> I had a quick look and this is what I think is the problem here.
> 
> This platform uses mailbox API to send its frequency change requests to another
> processor.  And the way it is written currently, I don't see any guarantee
> whatsoever which say
> 
>   "once clk_set_rate() returns, the frequency would have already changed".
> 

I think what was strange to me was that the frequency never seems to
change, there isn't just a delay in the new frequency taking effect, as
I would expect in these cases. Or if there is a delay, that's quite large
- at least a second.

> And this may exactly be the thing you are able to hit, luckily because of this
> patchset :)
> 
> As a quick way of checking if that is right or not, this may make it work:
> 
> diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c
> index 395ddc250828..9856c1c84dcf 100644
> --- a/drivers/mailbox/hi3660-mailbox.c
> +++ b/drivers/mailbox/hi3660-mailbox.c
> @@ -201,6 +201,9 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
> 
>         /* Trigger data transferring */
>         writel(BIT(mchan->ack_irq), base + MBOX_SEND_REG);
> +
> +       hi3660_mbox_check_state(chan);
> +

I gave this a try an it does work for me.

>         return 0;
>  }
> 
> -------------------------8<-------------------------
> 
> As a proper fix, something like this (not even compile tested) is required I
> believe as I don't see the clients would know if the transfer is over. Cc'ing
> mailbox guys to see what can be done.
> 

I'll give this a try as well when there is consensus. I might even try to
review it, if the time allows.

Many thanks,
Ionela.

> diff --git a/drivers/clk/hisilicon/clk-hi3660-stub.c b/drivers/clk/hisilicon/clk-hi3660-stub.c
> index 3a653d54bee0..c1e62ea4cf01 100644
> --- a/drivers/clk/hisilicon/clk-hi3660-stub.c
> +++ b/drivers/clk/hisilicon/clk-hi3660-stub.c
> @@ -89,7 +89,6 @@ static int hi3660_stub_clk_set_rate(struct clk_hw *hw, unsigned long rate,
>                 stub_clk->msg[0], stub_clk->msg[1]);
>  
>         mbox_send_message(stub_clk_chan.mbox, stub_clk->msg);
> -       mbox_client_txdone(stub_clk_chan.mbox, 0);
>  
>         stub_clk->rate = rate;
>         return 0;
> @@ -131,7 +130,7 @@ static int hi3660_stub_clk_probe(struct platform_device *pdev)
>         /* Use mailbox client without blocking */
>         stub_clk_chan.cl.dev = dev;
>         stub_clk_chan.cl.tx_done = NULL;
> -       stub_clk_chan.cl.tx_block = false;
> +       stub_clk_chan.cl.tx_block = true;
>         stub_clk_chan.cl.knows_txdone = false;
>  
>         /* Allocate mailbox channel */
> diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c
> index 395ddc250828..8f6b787c0aba 100644
> --- a/drivers/mailbox/hi3660-mailbox.c
> +++ b/drivers/mailbox/hi3660-mailbox.c
> @@ -1,5 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0
> -// Copyright (c) 2017-2018 HiSilicon Limited.
> +// Copyright (c) 2017-2018 Hisilicon Limited.
>  // Copyright (c) 2017-2018 Linaro Limited.
>  
>  #include <linux/bitops.h>
> @@ -83,7 +83,7 @@ static struct hi3660_mbox *to_hi3660_mbox(struct mbox_controller *mbox)
>         return container_of(mbox, struct hi3660_mbox, controller);
>  }
>  
> -static int hi3660_mbox_check_state(struct mbox_chan *chan)
> +static bool hi3660_mbox_last_tx_done(struct mbox_chan *chan)
>  {
>         unsigned long ch = (unsigned long)chan->con_priv;
>         struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox);
> @@ -94,20 +94,20 @@ static int hi3660_mbox_check_state(struct mbox_chan *chan)
>  
>         /* Mailbox is ready to use */
>         if (readl(base + MBOX_MODE_REG) & MBOX_STATE_READY)
> -               return 0;
> +               return true;
>  
>         /* Wait for acknowledge from remote */
>         ret = readx_poll_timeout_atomic(readl, base + MBOX_MODE_REG,
>                         val, (val & MBOX_STATE_ACK), 1000, 300000);
>         if (ret) {
>                 dev_err(mbox->dev, "%s: timeout for receiving ack\n", __func__);
> -               return ret;
> +               return false;
>         }
>  
>         /* clear ack state, mailbox will get back to ready state */
>         writel(BIT(mchan->ack_irq), base + MBOX_ICLR_REG);
>  
> -       return 0;
> +       return true;
>  }
>  
>  static int hi3660_mbox_unlock(struct mbox_chan *chan)
> @@ -182,10 +182,6 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
>         unsigned int i;
>         int ret;
>  
> -       ret = hi3660_mbox_check_state(chan);
> -       if (ret)
> -               return ret;
> -
>         /* Clear mask for destination interrupt */
>         writel_relaxed(~BIT(mchan->dst_irq), base + MBOX_IMASK_REG);
>  
> @@ -207,6 +203,7 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
>  static const struct mbox_chan_ops hi3660_mbox_ops = {
>         .startup        = hi3660_mbox_startup,
>         .send_data      = hi3660_mbox_send_data,
> +       .last_tx_done   = hi3660_mbox_last_tx_done,
>  };
>  
>  static struct mbox_chan *hi3660_mbox_xlate(struct mbox_controller *controller,
> @@ -259,6 +256,7 @@ static int hi3660_mbox_probe(struct platform_device *pdev)
>         mbox->controller.num_chans = MBOX_CHAN_MAX;
>         mbox->controller.ops = &hi3660_mbox_ops;
>         mbox->controller.of_xlate = hi3660_mbox_xlate;
> +       mbox->controller.txdone_poll = true;
>  
>         /* Initialize mailbox channel data */
>         chan = mbox->chan;
> 
> -- 
> viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
  2021-07-09  8:57         ` Ionela Voinescu
@ 2021-07-12  4:14           ` Viresh Kumar
  -1 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-07-12  4:14 UTC (permalink / raw)
  To: Ionela Voinescu
  Cc: Kevin Wangtao, Leo Yan, Jassi Brar, zhongkaihua, Dmitry Osipenko,
	Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
	Vincent Guittot, Rafael Wysocki, Sibi Sankar, linux-kernel,
	linux-arm-kernel

On 09-07-21, 09:57, Ionela Voinescu wrote:
> On Thursday 08 Jul 2021 at 13:23:53 (+0530), Viresh Kumar wrote:
> > On 07-07-21, 11:24, Ionela Voinescu wrote:
> > > Now comes the interesting part: what seems to fix it is a call to
> > > clk_get_rate(opp_table->clk) in _set_opp(), which is what basically
> > > happened before this patch, as _find_current_opp() was always called.
> > > I do not need to do anything with the returned frequency.
> > 
> > Wow, thanks for narrowing it down this far :)
> > 
> > I had a quick look and this is what I think is the problem here.
> > 
> > This platform uses mailbox API to send its frequency change requests to another
> > processor.  And the way it is written currently, I don't see any guarantee
> > whatsoever which say
> > 
> >   "once clk_set_rate() returns, the frequency would have already changed".
> > 
> 
> I think what was strange to me was that the frequency never seems to
> change, there isn't just a delay in the new frequency taking effect, as
> I would expect in these cases. Or if there is a delay, that's quite large
> - at least a second.

No idea on what the firmware is doing behind the scene :)

> > And this may exactly be the thing you are able to hit, luckily because of this
> > patchset :)
> > 
> > As a quick way of checking if that is right or not, this may make it work:
> > 
> > diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c
> > index 395ddc250828..9856c1c84dcf 100644
> > --- a/drivers/mailbox/hi3660-mailbox.c
> > +++ b/drivers/mailbox/hi3660-mailbox.c
> > @@ -201,6 +201,9 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
> > 
> >         /* Trigger data transferring */
> >         writel(BIT(mchan->ack_irq), base + MBOX_SEND_REG);
> > +
> > +       hi3660_mbox_check_state(chan);
> > +
> 
> I gave this a try an it does work for me.

Good, so that kind of proves what I was suspecting. The mailbox driver looks
buggy here.

> > -------------------------8<-------------------------
> > 
> > As a proper fix, something like this (not even compile tested) is required I
> > believe as I don't see the clients would know if the transfer is over. Cc'ing
> > mailbox guys to see what can be done.
> > 
> 
> I'll give this a try as well when there is consensus. I might even try to
> review it, if the time allows.

Sure, lets see what the platform guys think about this first.

Kevin, Kaihua ?

-- 
viresh

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 03/13] opp: Keep track of currently programmed OPP
@ 2021-07-12  4:14           ` Viresh Kumar
  0 siblings, 0 replies; 73+ messages in thread
From: Viresh Kumar @ 2021-07-12  4:14 UTC (permalink / raw)
  To: Ionela Voinescu
  Cc: Kevin Wangtao, Leo Yan, Jassi Brar, zhongkaihua, Dmitry Osipenko,
	Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
	Vincent Guittot, Rafael Wysocki, Sibi Sankar, linux-kernel,
	linux-arm-kernel

On 09-07-21, 09:57, Ionela Voinescu wrote:
> On Thursday 08 Jul 2021 at 13:23:53 (+0530), Viresh Kumar wrote:
> > On 07-07-21, 11:24, Ionela Voinescu wrote:
> > > Now comes the interesting part: what seems to fix it is a call to
> > > clk_get_rate(opp_table->clk) in _set_opp(), which is what basically
> > > happened before this patch, as _find_current_opp() was always called.
> > > I do not need to do anything with the returned frequency.
> > 
> > Wow, thanks for narrowing it down this far :)
> > 
> > I had a quick look and this is what I think is the problem here.
> > 
> > This platform uses mailbox API to send its frequency change requests to another
> > processor.  And the way it is written currently, I don't see any guarantee
> > whatsoever which say
> > 
> >   "once clk_set_rate() returns, the frequency would have already changed".
> > 
> 
> I think what was strange to me was that the frequency never seems to
> change, there isn't just a delay in the new frequency taking effect, as
> I would expect in these cases. Or if there is a delay, that's quite large
> - at least a second.

No idea on what the firmware is doing behind the scene :)

> > And this may exactly be the thing you are able to hit, luckily because of this
> > patchset :)
> > 
> > As a quick way of checking if that is right or not, this may make it work:
> > 
> > diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c
> > index 395ddc250828..9856c1c84dcf 100644
> > --- a/drivers/mailbox/hi3660-mailbox.c
> > +++ b/drivers/mailbox/hi3660-mailbox.c
> > @@ -201,6 +201,9 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
> > 
> >         /* Trigger data transferring */
> >         writel(BIT(mchan->ack_irq), base + MBOX_SEND_REG);
> > +
> > +       hi3660_mbox_check_state(chan);
> > +
> 
> I gave this a try an it does work for me.

Good, so that kind of proves what I was suspecting. The mailbox driver looks
buggy here.

> > -------------------------8<-------------------------
> > 
> > As a proper fix, something like this (not even compile tested) is required I
> > believe as I don't see the clients would know if the transfer is over. Cc'ing
> > mailbox guys to see what can be done.
> > 
> 
> I'll give this a try as well when there is consensus. I might even try to
> review it, if the time allows.

Sure, lets see what the platform guys think about this first.

Kevin, Kaihua ?

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 73+ messages in thread

end of thread, other threads:[~2021-07-12  4:16 UTC | newest]

Thread overview: 73+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-21 11:17 [PATCH 00/13] opp: Implement dev_pm_opp_set_opp() Viresh Kumar
2021-01-21 11:17 ` Viresh Kumar
2021-01-21 11:17 ` Viresh Kumar
2021-01-21 11:17 ` [PATCH 01/13] opp: Rename _opp_set_rate_zero() Viresh Kumar
2021-01-21 11:17   ` Viresh Kumar
2021-01-21 11:17 ` [PATCH 02/13] opp: No need to check clk for errors Viresh Kumar
2021-01-21 11:17   ` Viresh Kumar
2021-01-21 11:17 ` [PATCH 03/13] opp: Keep track of currently programmed OPP Viresh Kumar
2021-01-21 11:17   ` Viresh Kumar
2021-01-21 21:41   ` Dmitry Osipenko
2021-01-21 21:41     ` Dmitry Osipenko
2021-01-22  4:45     ` Viresh Kumar
2021-01-22  4:45       ` Viresh Kumar
2021-01-22 14:31       ` Dmitry Osipenko
2021-01-22 14:31         ` Dmitry Osipenko
2021-01-25  3:12         ` Viresh Kumar
2021-01-25  3:12           ` Viresh Kumar
2021-01-27 16:31       ` Akhil P Oommen
2021-01-28  4:14         ` Viresh Kumar
2021-01-28  4:14           ` Viresh Kumar
2021-07-07 10:24   ` Ionela Voinescu
2021-07-07 10:24     ` Ionela Voinescu
2021-07-08  7:53     ` Viresh Kumar
2021-07-08  7:53       ` Viresh Kumar
2021-07-09  8:57       ` Ionela Voinescu
2021-07-09  8:57         ` Ionela Voinescu
2021-07-12  4:14         ` Viresh Kumar
2021-07-12  4:14           ` Viresh Kumar
2021-01-21 11:17 ` [PATCH 04/13] opp: Split _set_opp() out of dev_pm_opp_set_rate() Viresh Kumar
2021-01-21 11:17   ` Viresh Kumar
2021-01-21 11:17 ` [PATCH 05/13] opp: Allow _set_opp() to work for non-freq devices Viresh Kumar
2021-01-21 11:17   ` Viresh Kumar
2021-01-21 11:17 ` [PATCH 06/13] opp: Allow _generic_set_opp_regulator() " Viresh Kumar
2021-01-21 11:17   ` Viresh Kumar
2021-01-21 11:17 ` [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() " Viresh Kumar
2021-01-21 11:17   ` Viresh Kumar
2021-01-21 20:26   ` Dmitry Osipenko
2021-01-21 20:26     ` Dmitry Osipenko
2021-01-22  4:35     ` Viresh Kumar
2021-01-22  4:35       ` Viresh Kumar
2021-01-25 21:09       ` Dmitry Osipenko
2021-01-25 21:09         ` Dmitry Osipenko
2021-01-27  6:58         ` Viresh Kumar
2021-01-27  6:58           ` Viresh Kumar
2021-01-21 11:17 ` [PATCH 08/13] opp: Update parameters of _set_opp_custom() Viresh Kumar
2021-01-21 11:17   ` Viresh Kumar
2021-01-21 11:17 ` [PATCH 09/13] opp: Implement dev_pm_opp_set_opp() Viresh Kumar
2021-01-21 11:17   ` Viresh Kumar
2021-01-21 11:17 ` [PATCH 10/13] cpufreq: qcom: Migrate to dev_pm_opp_set_opp() Viresh Kumar
2021-01-21 11:17   ` Viresh Kumar
2021-01-21 11:17 ` [PATCH 11/13] devfreq: tegra30: " Viresh Kumar
2021-01-21 11:17   ` Viresh Kumar
2021-01-21 21:36   ` Dmitry Osipenko
2021-01-21 21:36     ` Dmitry Osipenko
2021-01-22  6:26     ` Viresh Kumar
2021-01-22  6:26       ` Viresh Kumar
2021-01-22 15:28       ` Dmitry Osipenko
2021-01-22 15:28         ` Dmitry Osipenko
2021-01-25  3:14         ` Viresh Kumar
2021-01-25  3:14           ` Viresh Kumar
2021-01-25 16:00           ` Dmitry Osipenko
2021-01-25 16:00             ` Dmitry Osipenko
2021-01-27  9:10   ` [PATCH V2 " Viresh Kumar
2021-01-27 10:02     ` Viresh Kumar
2021-01-27 15:58       ` Dmitry Osipenko
2021-01-28  7:01         ` Viresh Kumar
2021-02-01  0:21     ` Chanwoo Choi
2021-02-01 19:57     ` Dmitry Osipenko
2021-01-21 11:17 ` [PATCH 12/13] drm: msm: " Viresh Kumar
2021-01-21 11:17   ` Viresh Kumar
2021-01-21 11:17   ` Viresh Kumar
2021-01-21 11:17 ` [PATCH 13/13] opp: Remove dev_pm_opp_set_bw() Viresh Kumar
2021-01-21 11:17   ` Viresh Kumar

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.