diff mbox series

[v3,10/12] opp: Support set_opp() customization without requiring to use regulators

Message ID 20210118005524.27787-11-digetx@gmail.com (mailing list archive)
State New, archived
Delegated to: viresh kumar
Headers show
Series OPP API fixes and improvements | expand

Commit Message

Dmitry Osipenko Jan. 18, 2021, 12:55 a.m. UTC 
Support set_opp() customization without requiring to use regulators. This
is needed by drivers which want to use dev_pm_opp_set_rate() for changing
rates of a multiple clocks and don't need to touch regulator.

One example is NVIDIA Tegra30/114 SoCs which have two sibling 3D hardware
units which should be use to the same clock rate, meanwhile voltage
scaling is done using a power domain. In this case OPP table doesn't have
a regulator, causing a NULL dereference in _set_opp_custom().

Tested-by: Peter Geis <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Tested-by: Matt Merhar <mattmerhar@protonmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c     | 46 +++++++++++++++++++++++++++++++++++-------
 include/linux/pm_opp.h |  3 +++
 2 files changed, 42 insertions(+), 7 deletions(-)

Comments

Viresh Kumar Jan. 18, 2021, 11:44 a.m. UTC | #1 
On 18-01-21, 03:55, Dmitry Osipenko wrote:
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index eefd0b15890c..c98fd2add563 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -13,6 +13,7 @@
>  
>  #include <linux/energy_model.h>
>  #include <linux/err.h>
> +#include <linux/kref.h>
>  #include <linux/notifier.h>
>  
>  struct clk;
> @@ -74,6 +75,7 @@ struct dev_pm_opp_info {
>   * @regulator_count: Number of regulators
>   * @clk:	Pointer to clk
>   * @dev:	Pointer to the struct device
> + * @kref:	Reference counter
>   *
>   * This structure contains all information required for setting an OPP.
>   */
> @@ -85,6 +87,7 @@ struct dev_pm_set_opp_data {
>  	unsigned int regulator_count;
>  	struct clk *clk;
>  	struct device *dev;
> +	struct kref kref;
>  };

Instead of kref thing, allocate the memory for supplies from
dev_pm_opp_set_regulators() and store it in new entries in opp-table
and for rest of the data from dev_pm_opp_register_set_opp_helper(), to
which you can copy supplies pointers then.
Dmitry Osipenko Jan. 18, 2021, 6:48 p.m. UTC | #2 
18.01.2021 14:44, Viresh Kumar пишет:
> On 18-01-21, 03:55, Dmitry Osipenko wrote:
>> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
>> index eefd0b15890c..c98fd2add563 100644
>> --- a/include/linux/pm_opp.h
>> +++ b/include/linux/pm_opp.h
>> @@ -13,6 +13,7 @@
>>  
>>  #include <linux/energy_model.h>
>>  #include <linux/err.h>
>> +#include <linux/kref.h>
>>  #include <linux/notifier.h>
>>  
>>  struct clk;
>> @@ -74,6 +75,7 @@ struct dev_pm_opp_info {
>>   * @regulator_count: Number of regulators
>>   * @clk:	Pointer to clk
>>   * @dev:	Pointer to the struct device
>> + * @kref:	Reference counter
>>   *
>>   * This structure contains all information required for setting an OPP.
>>   */
>> @@ -85,6 +87,7 @@ struct dev_pm_set_opp_data {
>>  	unsigned int regulator_count;
>>  	struct clk *clk;
>>  	struct device *dev;
>> +	struct kref kref;
>>  };
> 
> Instead of kref thing, allocate the memory for supplies from
> dev_pm_opp_set_regulators() and store it in new entries in opp-table
> and for rest of the data from dev_pm_opp_register_set_opp_helper(), to
> which you can copy supplies pointers then.
> 

Could you please show a code sample?
Viresh Kumar Jan. 19, 2021, 6:36 a.m. UTC | #3 
On 18-01-21, 21:48, Dmitry Osipenko wrote:
> 18.01.2021 14:44, Viresh Kumar пишет:
> > On 18-01-21, 03:55, Dmitry Osipenko wrote:
> >> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> >> index eefd0b15890c..c98fd2add563 100644
> >> --- a/include/linux/pm_opp.h
> >> +++ b/include/linux/pm_opp.h
> >> @@ -13,6 +13,7 @@
> >>  
> >>  #include <linux/energy_model.h>
> >>  #include <linux/err.h>
> >> +#include <linux/kref.h>
> >>  #include <linux/notifier.h>
> >>  
> >>  struct clk;
> >> @@ -74,6 +75,7 @@ struct dev_pm_opp_info {
> >>   * @regulator_count: Number of regulators
> >>   * @clk:	Pointer to clk
> >>   * @dev:	Pointer to the struct device
> >> + * @kref:	Reference counter
> >>   *
> >>   * This structure contains all information required for setting an OPP.
> >>   */
> >> @@ -85,6 +87,7 @@ struct dev_pm_set_opp_data {
> >>  	unsigned int regulator_count;
> >>  	struct clk *clk;
> >>  	struct device *dev;
> >> +	struct kref kref;
> >>  };
> > 
> > Instead of kref thing, allocate the memory for supplies from
> > dev_pm_opp_set_regulators() and store it in new entries in opp-table
> > and for rest of the data from dev_pm_opp_register_set_opp_helper(), to
> > which you can copy supplies pointers then.
> > 
> 
> Could you please show a code sample?

Sent a patch to you.
diff mbox series

Patch

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 49419ab9fbb4..7726c4c40b53 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -829,16 +829,21 @@  static int _set_opp_custom(const struct opp_table *opp_table,
 			   struct dev_pm_opp_supply *new_supply)
 {
 	struct dev_pm_set_opp_data *data;
-	int size;
+	int size, count;
+
+	if (opp_table->regulators)
+		count = opp_table->regulator_count;
+	else
+		count = 0;
 
 	data = opp_table->set_opp_data;
 	data->regulators = opp_table->regulators;
-	data->regulator_count = opp_table->regulator_count;
+	data->regulator_count = count;
 	data->clk = opp_table->clk;
 	data->dev = dev;
 
 	data->old_opp.rate = old_freq;
-	size = sizeof(*old_supply) * opp_table->regulator_count;
+	size = sizeof(*old_supply) * count;
 	if (!old_supply)
 		memset(data->old_opp.supplies, 0, size);
 	else
@@ -1860,8 +1865,14 @@  static int _allocate_set_opp_data(struct opp_table *opp_table)
 	struct dev_pm_set_opp_data *data;
 	int len, count = opp_table->regulator_count;
 
-	if (WARN_ON(!opp_table->regulators))
-		return -EINVAL;
+	/* just bump the refcount if already allocated */
+	if (opp_table->set_opp_data) {
+		kref_get(&opp_table->set_opp_data->kref);
+		return 0;
+	}
+
+	/* allocate maximum number of regulators, for simplicity */
+	count = max(count, 1);
 
 	/* space for set_opp_data */
 	len = sizeof(*data);
@@ -1873,6 +1884,7 @@  static int _allocate_set_opp_data(struct opp_table *opp_table)
 	if (!data)
 		return -ENOMEM;
 
+	kref_init(&data->kref);
 	data->old_opp.supplies = (void *)(data + 1);
 	data->new_opp.supplies = data->old_opp.supplies + count;
 
@@ -1881,10 +1893,17 @@  static int _allocate_set_opp_data(struct opp_table *opp_table)
 	return 0;
 }
 
+static void _release_set_opp_data(struct kref *kref)
+{
+	kfree(container_of(kref, struct dev_pm_set_opp_data, kref));
+}
+
 static void _free_set_opp_data(struct opp_table *opp_table)
 {
-	kfree(opp_table->set_opp_data);
-	opp_table->set_opp_data = NULL;
+	struct dev_pm_set_opp_data *data = opp_table->set_opp_data;
+
+	if (kref_put(&data->kref, _release_set_opp_data))
+		opp_table->set_opp_data = NULL;
 }
 
 /**
@@ -2182,6 +2201,7 @@  struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,
 			int (*set_opp)(struct dev_pm_set_opp_data *data))
 {
 	struct opp_table *opp_table;
+	int ret;
 
 	if (!set_opp)
 		return ERR_PTR(-EINVAL);
@@ -2196,11 +2216,21 @@  struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,
 		return ERR_PTR(-EBUSY);
 	}
 
+	/* Allocate block to pass to set_opp() routines */
+	ret = _allocate_set_opp_data(opp_table);
+	if (ret)
+		goto err;
+
 	/* Another CPU that shares the OPP table has set the helper ? */
 	if (!opp_table->set_opp)
 		opp_table->set_opp = set_opp;
 
 	return opp_table;
+
+err:
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_register_set_opp_helper);
 
@@ -2219,6 +2249,8 @@  void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table)
 	/* Make sure there are no concurrent readers while updating opp_table */
 	WARN_ON(!list_empty(&opp_table->opp_list));
 
+	_free_set_opp_data(opp_table);
+
 	opp_table->set_opp = NULL;
 	dev_pm_opp_put_opp_table(opp_table);
 }
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index eefd0b15890c..c98fd2add563 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -13,6 +13,7 @@ 
 
 #include <linux/energy_model.h>
 #include <linux/err.h>
+#include <linux/kref.h>
 #include <linux/notifier.h>
 
 struct clk;
@@ -74,6 +75,7 @@  struct dev_pm_opp_info {
  * @regulator_count: Number of regulators
  * @clk:	Pointer to clk
  * @dev:	Pointer to the struct device
+ * @kref:	Reference counter
  *
  * This structure contains all information required for setting an OPP.
  */
@@ -85,6 +87,7 @@  struct dev_pm_set_opp_data {
 	unsigned int regulator_count;
 	struct clk *clk;
 	struct device *dev;
+	struct kref kref;
 };
 
 #if defined(CONFIG_PM_OPP)