On Mon, May 13, 2019 at 03:54:10PM +0530, Viresh Kumar wrote:
> The OPP core requires the virtual device pointers to set performance
> state on behalf of the device, for the multiple power domain case. The
> genpd API (dev_pm_domain_attach_by_name()) has evolved now to support
> even the single power domain case and that lets us add common code for
> handling both the cases more efficiently.
>
> The virtual device structure returned by dev_pm_domain_attach_by_name()
> isn't normally used by the cpufreq drivers as they don't manage power
> on/off of the domains and so is only useful for the OPP core.
>
> This patch moves all the complexity into the OPP core to make the end
> drivers simple. The earlier APIs dev_pm_opp_{set|put}_genpd_virt_dev()
> are reworked into dev_pm_opp_{attach|detach}_genpd(). The new helper
> dev_pm_opp_attach_genpd() accepts a NULL terminated array of strings
> which contains names of all the genpd's to attach. It then attaches all
> the domains and saves the pointers to the virtual devices. The other
> helper undo the work done by this helper.
>
> Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
> ---
> @Niklas: Can you please try these patches and confirm they solve the
> issues you were facing ?
>
> drivers/opp/core.c | 128 ++++++++++++++++++++++++++---------------
> include/linux/pm_opp.h | 8 +--
> 2 files changed, 86 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0e7703fe733f..67d6b0caeab1 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1744,91 +1744,127 @@ void dev_pm_opp_unregister_set_opp_helper(struct
> opp_table *opp_table)
> }
> EXPORT_SYMBOL_GPL(dev_pm_opp_unregister_set_opp_helper);
>
> +static void _opp_detach_genpd(struct opp_table *opp_table)
> +{
> + int index;
> +
> + for (index = 0; index < opp_table->required_opp_count; index++) {
> + if (!opp_table->genpd_virt_devs[index])
> + continue;
> +
> + dev_pm_domain_detach(opp_table->genpd_virt_devs[index], false);
> + opp_table->genpd_virt_devs[index] = NULL;
> + }
> +}
> +
> /**
> - * dev_pm_opp_set_genpd_virt_dev - Set virtual genpd device for an index
> - * @dev: Consumer device for which the genpd device is getting set.
> - * @virt_dev: virtual genpd device.
> - * @index: index.
> + * dev_pm_opp_attach_genpd - Attach genpd(s) for the device and save virtual
> device pointer
> + * @dev: Consumer device for which the genpd is getting attached.
> + * @names: Null terminated array of pointers containing names of genpd to
> attach.
> *
> * Multiple generic power domains for a device are supported with the help of
> * virtual genpd devices, which are created for each consumer device - genpd
> * pair. These are the device structures which are attached to the power
> domain
> * and are required by the OPP core to set the performance state of the
> genpd.
> + * The same API also works for the case where single genpd is available and
> so
> + * we don't need to support that separately.
> *
> * This helper will normally be called by the consumer driver of the device
> - * "dev", as only that has details of the genpd devices.
> + * "dev", as only that has details of the genpd names.
> *
> - * This helper needs to be called once for each of those virtual devices, but
> - * only if multiple domains are available for a device. Otherwise the
> original
> - * device structure will be used instead by the OPP core.
> + * This helper needs to be called once with a list of all genpd to attach.
> + * Otherwise the original device structure will be used instead by the OPP
> core.
> */
> -struct opp_table *dev_pm_opp_set_genpd_virt_dev(struct device *dev,
> - struct device *virt_dev,
> - int index)
> +struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char
> **names)
> {
> struct opp_table *opp_table;
> + struct device *virt_dev;
> + int index, ret = -EINVAL;
> + const char **name = names;
>
> opp_table = dev_pm_opp_get_opp_table(dev);
> if (!opp_table)
> return ERR_PTR(-ENOMEM);
>
> + /*
> + * If the genpd's OPP table isn't already initialized, parsing of the
> + * required-opps fail for dev. We should retry this after genpd's OPP
> + * table is added.
> + */
> + if (!opp_table->required_opp_count) {
> + ret = -EPROBE_DEFER;
> + goto put_table;
> + }
> +
> mutex_lock(&opp_table->genpd_virt_dev_lock);
>
> - if (unlikely(!opp_table->genpd_virt_devs ||
> - index >= opp_table->required_opp_count ||
> - opp_table->genpd_virt_devs[index])) {
> + while (*name) {
> + index = of_property_match_string(dev->of_node,
> + "power-domain-names", *name);
> + if (index < 0) {
> + dev_err(dev, "Failed to find power domain: %s (%d)\n",
> + *name, index);
> + goto err;
> + }
>
> - dev_err(dev, "Invalid request to set required device\n");
> - dev_pm_opp_put_opp_table(opp_table);
> - mutex_unlock(&opp_table->genpd_virt_dev_lock);
> + if (index >= opp_table->required_opp_count) {
> + dev_err(dev, "Index can't be greater than
> required-opp-count - 1, %s (%d : %d)\n",
> + *name, opp_table->required_opp_count, index);
> + goto err;
> + }
>
> - return ERR_PTR(-EINVAL);
> + if (opp_table->genpd_virt_devs[index]) {
> + dev_err(dev, "Genpd virtual device already set %s\n",
> + *name);
> + goto err;
> + }
> +
> + virt_dev = dev_pm_domain_attach_by_name(dev, *name);
> + if (IS_ERR(virt_dev)) {
> + ret = PTR_ERR(virt_dev);
> + dev_err(dev, "Couldn't attach to pm_domain: %d\n", ret);
> + goto err;
> + }
> +
> + opp_table->genpd_virt_devs[index] = virt_dev;
> + name++;
> }
>
> - opp_table->genpd_virt_devs[index] = virt_dev;
> mutex_unlock(&opp_table->genpd_virt_dev_lock);
>
> return opp_table;
> +
> +err:
> + _opp_detach_genpd(opp_table);
> + mutex_unlock(&opp_table->genpd_virt_dev_lock);
> +
> +put_table:
> + dev_pm_opp_put_opp_table(opp_table);
> +
> + return ERR_PTR(ret);
> }
> +EXPORT_SYMBOL_GPL(dev_pm_opp_attach_genpd);
>
> /**
> - * dev_pm_opp_put_genpd_virt_dev() - Releases resources blocked for genpd
> device.
> - * @opp_table: OPP table returned by dev_pm_opp_set_genpd_virt_dev().
> - * @virt_dev: virtual genpd device.
> - *
> - * This releases the resource previously acquired with a call to
> - * dev_pm_opp_set_genpd_virt_dev(). The consumer driver shall call this
> helper
> - * if it doesn't want OPP core to update performance state of a power domain
> - * anymore.
> + * dev_pm_opp_detach_genpd() - Detach genpd(s) from the device.
> + * @opp_table: OPP table returned by dev_pm_opp_attach_genpd().
> + *
> + * This detaches the genpd(s), resets the virtual device pointers, and puts
> the
> + * OPP table.
> */
> -void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table,
> - struct device *virt_dev)
> +void dev_pm_opp_detach_genpd(struct opp_table *opp_table)
> {
> - int i;
> -
> /*
> * Acquire genpd_virt_dev_lock to make sure virt_dev isn't getting
> * used in parallel.
> */
> mutex_lock(&opp_table->genpd_virt_dev_lock);
> -
> - for (i = 0; i < opp_table->required_opp_count; i++) {
> - if (opp_table->genpd_virt_devs[i] != virt_dev)
> - continue;
> -
> - opp_table->genpd_virt_devs[i] = NULL;
> - dev_pm_opp_put_opp_table(opp_table);
> -
> - /* Drop the vote */
> - dev_pm_genpd_set_performance_state(virt_dev, 0);
> - break;
> - }
> -
> + _opp_detach_genpd(opp_table);
> mutex_unlock(&opp_table->genpd_virt_dev_lock);
>
> - if (unlikely(i == opp_table->required_opp_count))
> - dev_err(virt_dev, "Failed to find required device entry\n");
> + dev_pm_opp_put_opp_table(opp_table);
> }
> +EXPORT_SYMBOL_GPL(dev_pm_opp_detach_genpd);
>
> /**
> * dev_pm_opp_xlate_performance_state() - Find required OPP's pstate for
> src_table.
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index b150fe97ce5a..be570761b77a 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -131,8 +131,8 @@ struct opp_table *dev_pm_opp_set_clkname(struct device
> *dev, const char * name);
> void dev_pm_opp_put_clkname(struct opp_table *opp_table);
> struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int
> (*set_opp)(struct dev_pm_set_opp_data *data));
> void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
> -struct opp_table *dev_pm_opp_set_genpd_virt_dev(struct device *dev, struct
> device *virt_dev, int index);
> -void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table, struct
> device *virt_dev);
> +struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char
> **names);
> +void dev_pm_opp_detach_genpd(struct opp_table *opp_table);
> 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_sharing_cpus(struct device *cpu_dev, const struct cpumask
> *cpumask);
> @@ -295,12 +295,12 @@ static inline struct opp_table
> *dev_pm_opp_set_clkname(struct device *dev, const
>
> static inline void dev_pm_opp_put_clkname(struct opp_table *opp_table) {}
>
> -static inline struct opp_table *dev_pm_opp_set_genpd_virt_dev(struct device
> *dev, struct device *virt_dev, int index)
> +static inline struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
> const char **names)
> {
> return ERR_PTR(-ENOTSUPP);
> }
>
> -static inline void dev_pm_opp_put_genpd_virt_dev(struct opp_table
> *opp_table, struct device *virt_dev) {}
> +static inline void dev_pm_opp_detach_genpd(struct opp_table *opp_table) {}
>
> static inline int dev_pm_opp_xlate_performance_state(struct opp_table
> *src_table, struct opp_table *dst_table, unsigned int pstate)
> {
> --
> 2.21.0.rc0.269.g1a574e7a288b
>
Tested-by: Niklas Cassel <niklas.cas...@linaro.org>
