| Message ID | 20230607124628.157465-12-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm_scmi/opp/dvfs: Add generic performance scaling support | expand |
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
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) { 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); /** diff --git a/drivers/opp/of.c b/drivers/opp/of.c index 8246e9b7afe7..b96f6304f497 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -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; diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h index 2a057c42ddf4..b15770b2305e 100644 --- a/drivers/opp/opp.h +++ b/drivers/opp/opp.h @@ -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); diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index dc1fb5890792..305cd87b394c 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -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) { }
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(-)