[PATCH] OPP: Remove the unused argument to config_clks_t

[PATCH] OPP: Remove the unused argument to config_clks_t

[Viresh Kumar]

The OPP core needs to take care of a special case, where the OPPs aren't
available for a device, but in order to keep the same unified interface
for the driver, the same OPP core API must take care of performing a
simple clk_set_rate() for the device.

This required the extra argument, but that is used only within the OPP
core and the drivers don't need to take care of that.

Simplify the external API and handle it differently within the OPP core.

This shouldn't result in any functional change.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
I will be taking this through the PM tree for the upcoming merge window.
Hopefully this won't create any issues for the ufs and devfreq driver as there
is no functional change for them.

 drivers/devfreq/tegra30-devfreq.c |  2 +-
 drivers/opp/core.c                | 37 +++++++++++++------------------
 drivers/ufs/core/ufshcd.c         |  3 +--
 include/linux/pm_opp.h            |  5 ++---
 include/ufs/ufshcd.h              |  3 +--
 5 files changed, 20 insertions(+), 30 deletions(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 4a4f0106ab9d..730c6618abc5 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -823,7 +823,7 @@ static int devm_tegra_devfreq_init_hw(struct device *dev,
 
 static int tegra_devfreq_config_clks_nop(struct device *dev,
 					 struct opp_table *opp_table,
-					 struct dev_pm_opp *opp, void *data,
+					 struct dev_pm_opp *opp,
 					 bool scaling_down)
 {
 	/* We want to skip clk configuration via dev_pm_opp_set_opp() */
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 29f8160c3e38..ba5f692e2161 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -940,24 +940,11 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
 	return ret;
 }
 
-static int
-_opp_config_clk_single(struct device *dev, struct opp_table *opp_table,
-		       struct dev_pm_opp *opp, void *data, bool scaling_down)
+static int _opp_clk_set_rate(struct device *dev, struct opp_table *opp_table,
+			     unsigned long freq)
 {
-	unsigned long *target = data;
-	unsigned long freq;
 	int ret;
 
-	/* One of target and opp must be available */
-	if (target) {
-		freq = *target;
-	} else if (opp) {
-		freq = opp->rates[0];
-	} else {
-		WARN_ON(1);
-		return -EINVAL;
-	}
-
 	ret = clk_set_rate(opp_table->clk, freq);
 	if (ret) {
 		dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
@@ -969,12 +956,19 @@ _opp_config_clk_single(struct device *dev, struct opp_table *opp_table,
 	return ret;
 }
 
+static int
+_opp_config_clk_single(struct device *dev, struct opp_table *opp_table,
+		       struct dev_pm_opp *opp, bool scaling_down)
+{
+	return _opp_clk_set_rate(dev, opp_table, opp->rates[0]);
+}
+
 /*
  * Simple implementation for configuring multiple clocks. Configure clocks in
  * the order in which they are present in the array while scaling up.
  */
 int dev_pm_opp_config_clks_simple(struct device *dev,
-		struct opp_table *opp_table, struct dev_pm_opp *opp, void *data,
+		struct opp_table *opp_table, struct dev_pm_opp *opp,
 		bool scaling_down)
 {
 	int ret, i;
@@ -1183,7 +1177,7 @@ static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
 }
 
 static int _set_opp(struct device *dev, struct opp_table *opp_table,
-		    struct dev_pm_opp *opp, void *clk_data, bool forced)
+		    struct dev_pm_opp *opp, bool forced)
 {
 	struct dev_pm_opp *old_opp;
 	int scaling_down, ret;
@@ -1243,7 +1237,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 	}
 
 	if (opp_table->config_clks) {
-		ret = opp_table->config_clks(dev, opp_table, opp, clk_data, scaling_down);
+		ret = opp_table->config_clks(dev, opp_table, opp, scaling_down);
 		if (ret)
 			return ret;
 	}
@@ -1322,8 +1316,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 = opp_table->config_clks(dev, opp_table, NULL,
-						     &target_freq, false);
+			ret = _opp_clk_set_rate(dev, opp_table, target_freq);
 			goto put_opp_table;
 		}
 
@@ -1355,7 +1348,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		forced = opp_table->rate_clk_single != target_freq;
 	}
 
-	ret = _set_opp(dev, opp_table, opp, &target_freq, forced);
+	ret = _set_opp(dev, opp_table, opp, forced);
 
 	if (target_freq)
 		dev_pm_opp_put(opp);
@@ -1387,7 +1380,7 @@ int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp)
 		return PTR_ERR(opp_table);
 	}
 
-	ret = _set_opp(dev, opp_table, opp, NULL, false);
+	ret = _set_opp(dev, opp_table, opp, false);
 	dev_pm_opp_put_opp_table(opp_table);
 
 	return ret;
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index bce0d2a9a7f3..51d6c8567189 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1064,8 +1064,7 @@ static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
 }
 
 int ufshcd_opp_config_clks(struct device *dev, struct opp_table *opp_table,
-			   struct dev_pm_opp *opp, void *data,
-			   bool scaling_down)
+			   struct dev_pm_opp *opp, bool scaling_down)
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);
 	struct list_head *head = &hba->clk_list_head;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 76dcb7f37bcd..c99a66e88e78 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -50,7 +50,7 @@ typedef int (*config_regulators_t)(struct device *dev,
 			struct regulator **regulators, unsigned int count);
 
 typedef int (*config_clks_t)(struct device *dev, struct opp_table *opp_table,
-			struct dev_pm_opp *opp, void *data, bool scaling_down);
+			struct dev_pm_opp *opp, bool scaling_down);
 
 /**
  * struct dev_pm_opp_config - Device OPP configuration values
@@ -181,8 +181,7 @@ int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config);
 int devm_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config);
 void dev_pm_opp_clear_config(int token);
 int dev_pm_opp_config_clks_simple(struct device *dev,
-		struct opp_table *opp_table, struct dev_pm_opp *opp, void *data,
-		bool scaling_down);
+		struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down);
 
 struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table *src_table, struct opp_table *dst_table, struct dev_pm_opp *src_opp);
 int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 7f0b2c5599cd..156e47dd4d9c 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1255,8 +1255,7 @@ void ufshcd_mcq_enable_esi(struct ufs_hba *hba);
 void ufshcd_mcq_config_esi(struct ufs_hba *hba, struct msi_msg *msg);
 
 int ufshcd_opp_config_clks(struct device *dev, struct opp_table *opp_table,
-			   struct dev_pm_opp *opp, void *data,
-			   bool scaling_down);
+			   struct dev_pm_opp *opp, bool scaling_down);
 /**
  * ufshcd_set_variant - set variant specific data to the hba
  * @hba: per adapter instance
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH] OPP: Remove the unused argument to config_clks_t

[Konrad Dybcio]

On 3.01.2024 11:48, Viresh Kumar wrote:
> The OPP core needs to take care of a special case, where the OPPs aren't
> available for a device, but in order to keep the same unified interface
> for the driver, the same OPP core API must take care of performing a
> simple clk_set_rate() for the device.
> 
> This required the extra argument, but that is used only within the OPP
> core and the drivers don't need to take care of that.
> 
> Simplify the external API and handle it differently within the OPP core.
> 
> This shouldn't result in any functional change.

Hi, so this apparently breaks serial on Qualcomm platforms using
"qcom,geni-debug-uart".. I'm seeing garbage on the console, likely
meaning that ratesetting wasn't done.

Konrad

Re: [PATCH] OPP: Remove the unused argument to config_clks_t

[Konrad Dybcio]

On 4.01.2024 13:53, Konrad Dybcio wrote:
> 
> On 3.01.2024 11:48, Viresh Kumar wrote:
>> The OPP core needs to take care of a special case, where the OPPs aren't
>> available for a device, but in order to keep the same unified interface
>> for the driver, the same OPP core API must take care of performing a
>> simple clk_set_rate() for the device.
>>
>> This required the extra argument, but that is used only within the OPP
>> core and the drivers don't need to take care of that.
>>
>> Simplify the external API and handle it differently within the OPP core.
>>
>> This shouldn't result in any functional change.
> Hi, so this apparently breaks serial on Qualcomm platforms using
> "qcom,geni-debug-uart".. I'm seeing garbage on the console, likely
> meaning that ratesetting wasn't done.

+CC Bjorn, Dmitry

Probably also worth noting it only happens when an OPP table is present
in the device tree.

Konrad

Re: [PATCH] OPP: Remove the unused argument to config_clks_t

[Viresh Kumar]

On 04-01-24, 13:56, Konrad Dybcio wrote:
> 
> 
> On 4.01.2024 13:53, Konrad Dybcio wrote:
> > 
> > On 3.01.2024 11:48, Viresh Kumar wrote:
> >> The OPP core needs to take care of a special case, where the OPPs aren't
> >> available for a device, but in order to keep the same unified interface
> >> for the driver, the same OPP core API must take care of performing a
> >> simple clk_set_rate() for the device.
> >>
> >> This required the extra argument, but that is used only within the OPP
> >> core and the drivers don't need to take care of that.
> >>
> >> Simplify the external API and handle it differently within the OPP core.
> >>
> >> This shouldn't result in any functional change.
> > Hi, so this apparently breaks serial on Qualcomm platforms using
> > "qcom,geni-debug-uart".. I'm seeing garbage on the console, likely
> > meaning that ratesetting wasn't done.
> 
> +CC Bjorn, Dmitry
> 
> Probably also worth noting it only happens when an OPP table is present
> in the device tree.

Found the issue. Dropped the patch for now. Not sure if there is a
clean way of handling it right now.

-- 
viresh