[11/16] OPP: Add dev_pm_opp_add_dynamic() to allow more flexibility
Commit Message
The dev_pm_opp_add() API is limited to add dynamic OPPs with a frequency
and/or voltage level. To enable more flexibility, let's add a new
dev_pm_opp_add_dynamic() API, that's takes a struct dev_pm_opp_data*
instead of list of in-parameters.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/opp/core.c | 49 ++++++++++++++++++++++++++++++++----------
drivers/opp/of.c | 11 ++++++----
drivers/opp/opp.h | 2 +-
include/linux/pm_opp.h | 18 ++++++++++++++++
4 files changed, 64 insertions(+), 16 deletions(-)
Comments
On 07-06-23, 14:46, Ulf Hansson wrote:
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 954c94865cf5..0e6ee2980f88 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1921,8 +1921,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
> * _opp_add_v1() - Allocate a OPP based on v1 bindings.
> * @opp_table: OPP table
> * @dev: device for which we do this operation
> - * @freq: Frequency in Hz for this OPP
> - * @u_volt: Voltage in uVolts for this OPP
> + * @opp: The OPP to add
> * @dynamic: Dynamically added OPPs.
> *
> * This function adds an opp definition to the opp table and returns status.
> @@ -1940,10 +1939,10 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
> * -ENOMEM Memory allocation failure
> */
> int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
> - unsigned long freq, long u_volt, bool dynamic)
> + struct dev_pm_opp_data *opp, bool dynamic)
The name `opp` is mostly used for instances of `struct dev_pm_opp`. Can we use a
different name here please for the data ?
> +/**
> + * dev_pm_opp_add() - Add an OPP table from a table definitions
> + * @dev: device for which we do this operation
> + * @freq: Frequency in Hz for this OPP
> + * @u_volt: Voltage in uVolts for this OPP
> + *
> + * This function adds an opp definition to the opp table and returns status.
> + * The opp is made available by default and it can be controlled using
> + * dev_pm_opp_enable/disable functions.
> + *
> + * Return:
> + * 0 On success OR
> + * Duplicate OPPs (both freq and volt are same) and opp->available
> + * -EEXIST Freq are same and volt are different OR
> + * Duplicate OPPs (both freq and volt are same) and !opp->available
> + * -ENOMEM Memory allocation failure
> + */
> +int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
Maybe move this to include/linux/pm_opp.h and mark it static inline and get rid
of documentation too.
> +{
> + struct dev_pm_opp_data opp;
> +
> + memset(&opp, 0, sizeof(opp));
What about
struct dev_pm_opp_data data = {0};
I think it is guaranteed that all the fields will be 0 now, not the padding of
course, but we don't care about that here.
> + opp.freq = freq;
> + opp.u_volt = u_volt;
Or maybe just
struct dev_pm_opp_data data = {
.freq = freq,
.u_volt = u_volt,
};
Rest must be 0.
On Thu, 8 Jun 2023 at 07:29, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 07-06-23, 14:46, Ulf Hansson wrote:
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 954c94865cf5..0e6ee2980f88 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1921,8 +1921,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
> > * _opp_add_v1() - Allocate a OPP based on v1 bindings.
> > * @opp_table: OPP table
> > * @dev: device for which we do this operation
> > - * @freq: Frequency in Hz for this OPP
> > - * @u_volt: Voltage in uVolts for this OPP
> > + * @opp: The OPP to add
> > * @dynamic: Dynamically added OPPs.
> > *
> > * This function adds an opp definition to the opp table and returns status.
> > @@ -1940,10 +1939,10 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
> > * -ENOMEM Memory allocation failure
> > */
> > int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
> > - unsigned long freq, long u_volt, bool dynamic)
> > + struct dev_pm_opp_data *opp, bool dynamic)
>
> The name `opp` is mostly used for instances of `struct dev_pm_opp`. Can we use a
> different name here please for the data ?
Certainly, what do you suggest?
>
> > +/**
> > + * dev_pm_opp_add() - Add an OPP table from a table definitions
> > + * @dev: device for which we do this operation
> > + * @freq: Frequency in Hz for this OPP
> > + * @u_volt: Voltage in uVolts for this OPP
> > + *
> > + * This function adds an opp definition to the opp table and returns status.
> > + * The opp is made available by default and it can be controlled using
> > + * dev_pm_opp_enable/disable functions.
> > + *
> > + * Return:
> > + * 0 On success OR
> > + * Duplicate OPPs (both freq and volt are same) and opp->available
> > + * -EEXIST Freq are same and volt are different OR
> > + * Duplicate OPPs (both freq and volt are same) and !opp->available
> > + * -ENOMEM Memory allocation failure
> > + */
> > +int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>
> Maybe move this to include/linux/pm_opp.h and mark it static inline and get rid
> of documentation too.
Good idea!
>
> > +{
> > + struct dev_pm_opp_data opp;
> > +
> > + memset(&opp, 0, sizeof(opp));
>
> What about
> struct dev_pm_opp_data data = {0};
>
> I think it is guaranteed that all the fields will be 0 now, not the padding of
> course, but we don't care about that here.
>
> > + opp.freq = freq;
> > + opp.u_volt = u_volt;
>
> Or maybe just
>
> struct dev_pm_opp_data data = {
> .freq = freq,
> .u_volt = u_volt,
> };
>
> Rest must be 0.
Good suggestions both, I will change to whatever is best suitable!
Thanks for reviewing!
Kind regards
Uffe
On 08-06-23, 10:59, Ulf Hansson wrote:
> Certainly, what do you suggest?
`data` :)
On Thu, 8 Jun 2023 at 11:22, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 08-06-23, 10:59, Ulf Hansson wrote:
> > Certainly, what do you suggest?
>
> `data` :)
I thought you meant renaming the struct. :-)
Yes, I move to data instead!
Uffe
@@ -1921,8 +1921,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
* _opp_add_v1() - Allocate a OPP based on v1 bindings.
* @opp_table: OPP table
* @dev: device for which we do this operation
- * @freq: Frequency in Hz for this OPP
- * @u_volt: Voltage in uVolts for this OPP
+ * @opp: The OPP to add
* @dynamic: Dynamically added OPPs.
*
* This function adds an opp definition to the opp table and returns status.
@@ -1940,10 +1939,10 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
* -ENOMEM Memory allocation failure
*/
int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
- unsigned long freq, long u_volt, bool dynamic)
+ struct dev_pm_opp_data *opp, bool dynamic)
{
struct dev_pm_opp *new_opp;
- unsigned long tol;
+ unsigned long tol, u_volt = opp->u_volt;
int ret;
if (!assert_single_clk(opp_table))
@@ -1954,7 +1953,7 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
return -ENOMEM;
/* populate the opp table */
- new_opp->rates[0] = freq;
+ new_opp->rates[0] = opp->freq;
tol = u_volt * opp_table->voltage_tolerance_v1 / 100;
new_opp->supplies[0].u_volt = u_volt;
new_opp->supplies[0].u_volt_min = u_volt - tol;
@@ -2738,10 +2737,9 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
}
/**
- * dev_pm_opp_add() - Add an OPP table from a table definitions
- * @dev: device for which we do this operation
- * @freq: Frequency in Hz for this OPP
- * @u_volt: Voltage in uVolts for this OPP
+ * dev_pm_opp_add_dynamic() - Add an OPP table from a table definitions
+ * @dev: The device for which we do this operation
+ * @opp: The OPP to be added
*
* This function adds an opp definition to the opp table and returns status.
* The opp is made available by default and it can be controlled using
@@ -2754,7 +2752,7 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
* Duplicate OPPs (both freq and volt are same) and !opp->available
* -ENOMEM Memory allocation failure
*/
-int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
+int dev_pm_opp_add_dynamic(struct device *dev, struct dev_pm_opp_data *opp)
{
struct opp_table *opp_table;
int ret;
@@ -2766,12 +2764,41 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
/* Fix regulator count for dynamic OPPs */
opp_table->regulator_count = 1;
- ret = _opp_add_v1(opp_table, dev, freq, u_volt, true);
+ ret = _opp_add_v1(opp_table, dev, opp, true);
if (ret)
dev_pm_opp_put_opp_table(opp_table);
return ret;
}
+EXPORT_SYMBOL_GPL(dev_pm_opp_add_dynamic);
+
+/**
+ * dev_pm_opp_add() - Add an OPP table from a table definitions
+ * @dev: device for which we do this operation
+ * @freq: Frequency in Hz for this OPP
+ * @u_volt: Voltage in uVolts for this OPP
+ *
+ * This function adds an opp definition to the opp table and returns status.
+ * The opp is made available by default and it can be controlled using
+ * dev_pm_opp_enable/disable functions.
+ *
+ * Return:
+ * 0 On success OR
+ * Duplicate OPPs (both freq and volt are same) and opp->available
+ * -EEXIST Freq are same and volt are different OR
+ * Duplicate OPPs (both freq and volt are same) and !opp->available
+ * -ENOMEM Memory allocation failure
+ */
+int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
+{
+ struct dev_pm_opp_data opp;
+
+ memset(&opp, 0, sizeof(opp));
+ opp.freq = freq;
+ opp.u_volt = u_volt;
+
+ return dev_pm_opp_add_dynamic(dev, &opp);
+}
EXPORT_SYMBOL_GPL(dev_pm_opp_add);
/**
@@ -1079,13 +1079,16 @@ static int _of_add_opp_table_v1(struct device *dev, struct opp_table *opp_table)
val = prop->value;
while (nr) {
- unsigned long freq = be32_to_cpup(val++) * 1000;
- unsigned long volt = be32_to_cpup(val++);
+ struct dev_pm_opp_data opp;
- ret = _opp_add_v1(opp_table, dev, freq, volt, false);
+ memset(&opp, 0, sizeof(opp));
+ opp.freq = be32_to_cpup(val++) * 1000;
+ opp.u_volt = be32_to_cpup(val++);
+
+ ret = _opp_add_v1(opp_table, dev, &opp, false);
if (ret) {
dev_err(dev, "%s: Failed to add OPP %ld (%d)\n",
- __func__, freq, ret);
+ __func__, opp.freq, ret);
goto remove_static_opp;
}
nr -= 2;
@@ -255,7 +255,7 @@ struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table);
void _opp_free(struct dev_pm_opp *opp);
int _opp_compare_key(struct opp_table *opp_table, struct dev_pm_opp *opp1, struct dev_pm_opp *opp2);
int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table);
-int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned long freq, long u_volt, bool dynamic);
+int _opp_add_v1(struct opp_table *opp_table, struct device *dev, struct dev_pm_opp_data *opp, bool dynamic);
void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cpu);
struct opp_table *_add_opp_table_indexed(struct device *dev, int index, bool getclk);
void _put_opp_list_kref(struct opp_table *opp_table);
@@ -92,6 +92,16 @@ struct dev_pm_opp_config {
struct device ***virt_devs;
};
+/**
+ * struct dev_pm_opp_data - The data to use to initialize an OPP.
+ * @freq: The clock rate in Hz for the OPP.
+ * @u_volt: The voltage in uV for the OPP.
+ */
+struct dev_pm_opp_data {
+ unsigned long freq;
+ unsigned long u_volt;
+};
+
#if defined(CONFIG_PM_OPP)
struct opp_table *dev_pm_opp_get_opp_table(struct device *dev);
@@ -142,6 +152,8 @@ void dev_pm_opp_put(struct dev_pm_opp *opp);
int dev_pm_opp_add(struct device *dev, unsigned long freq,
unsigned long u_volt);
+int dev_pm_opp_add_dynamic(struct device *dev, struct dev_pm_opp_data *opp);
+
void dev_pm_opp_remove(struct device *dev, unsigned long freq);
void dev_pm_opp_remove_all_dynamic(struct device *dev);
@@ -297,6 +309,12 @@ static inline int dev_pm_opp_add(struct device *dev, unsigned long freq,
return -EOPNOTSUPP;
}
+static inline int
+dev_pm_opp_add_dynamic(struct device *dev, struct dev_pm_opp_data *opp)
+{
+ return -EOPNOTSUPP;
+}
+
static inline void dev_pm_opp_remove(struct device *dev, unsigned long freq)
{
}