| Message ID | 20231228114157.104822-1-ulf.hansson@linaro.org |
|---|---|
| Headers | show |
| Series | PM: domains: Add helpers for multi PM domains to avoid open-coding | expand |
On 12/28/2023 3:41 AM, Ulf Hansson wrote: > Attaching/detaching of a device to multiple PM domains has started to > become a common operation for many drivers, typically during ->probe() and > ->remove(). In most cases, this has lead to lots of boilerplate code in the > drivers. > > To fixup up the situation, let's introduce a pair of helper functions, > dev_pm_domain_attach|detach_list(), that driver can use instead of the > open-coding. Note that, it seems reasonable to limit the support for these > helpers to DT based platforms, at it's the only valid use case for now. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/base/power/common.c | 133 ++++++++++++++++++++++++++++++++++++ > include/linux/pm_domain.h | 38 +++++++++++ > 2 files changed, 171 insertions(+) > > diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c > index 44ec20918a4d..1ef51889fc6f 100644 > --- a/drivers/base/power/common.c > +++ b/drivers/base/power/common.c > @@ -167,6 +167,114 @@ struct device *dev_pm_domain_attach_by_name(struct device *dev, > } > EXPORT_SYMBOL_GPL(dev_pm_domain_attach_by_name); > > +/** > + * dev_pm_domain_attach_list - Associate a device with its PM domains. > + * @dev: The device used to lookup the PM domains for. > + * @data: The data used for attaching to the PM domains. > + * @list: An out-parameter with an allocated list of attached PM domains. > + * > + * This function helps to attach a device to its multiple PM domains. The > + * caller, which is typically a driver's probe function, may provide a list of > + * names for the PM domains that we should try to attach the device to, but it > + * may also provide an empty list, in case the attach should be done for all of > + * the available PM domains. > + * > + * Callers must ensure proper synchronization of this function with power > + * management callbacks. > + * > + * Returns the number of attached PM domains or a negative error code in case of > + * a failure. Note that, to detach the list of PM domains, the driver shall call > + * dev_pm_domain_detach_list(), typically during the remove phase. > + */ > +int dev_pm_domain_attach_list(struct device *dev, > + const struct dev_pm_domain_attach_data *data, > + struct dev_pm_domain_list **list) > +{ > + struct device_node *np = dev->of_node; > + struct dev_pm_domain_list *pds; > + struct device *pd_dev = NULL; > + int ret, i, num_pds = 0; > + bool by_id = true; > + u32 link_flags = data && data->pd_flags & PD_FLAG_NO_DEV_LINK ? 0 : > + DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME; > + > + if (dev->pm_domain) > + return -EEXIST; > + > + /* For now this is limited to OF based platforms. */ > + if (!np) > + return 0; > + > + if (data && data->pd_names) { > + num_pds = data->num_pd_names; > + by_id = false; > + } else { > + num_pds = of_count_phandle_with_args(np, "power-domains", > + "#power-domain-cells"); > + } > + > + if (num_pds <= 0) > + return 0; > + > + pds = devm_kzalloc(dev, sizeof(*pds), GFP_KERNEL); > + if (!pds) > + return -ENOMEM; > + > + pds->pd_devs = devm_kcalloc(dev, num_pds, sizeof(*pds->pd_devs), > + GFP_KERNEL); > + if (!pds->pd_devs) > + return -ENOMEM; > + > + pds->pd_links = devm_kcalloc(dev, num_pds, sizeof(*pds->pd_links), > + GFP_KERNEL); > + if (!pds->pd_links) > + return -ENOMEM; > + > + if (link_flags && data->pd_flags & PD_FLAG_DEV_LINK_ON) Since data is optional, this check results in crash if data is NULL. Thanks > + link_flags |= DL_FLAG_RPM_ACTIVE; > + > + for (i = 0; i < num_pds; i++) { > + if (by_id) > + pd_dev = dev_pm_domain_attach_by_id(dev, i); > + else > + pd_dev = dev_pm_domain_attach_by_name(dev, > + data->pd_names[i]); > + if (IS_ERR_OR_NULL(pd_dev)) { > + ret = pd_dev ? PTR_ERR(pd_dev) : -ENODEV; > + goto err_attach; > + } > + > + if (link_flags) { > + struct device_link *link; > + > + link = device_link_add(dev, pd_dev, link_flags); > + if (!link) { > + ret = -ENODEV; > + goto err_link; > + } > + > + pds->pd_links[i] = link; > + } > + > + pds->pd_devs[i] = pd_dev; > + } > + > + pds->num_pds = num_pds; > + *list = pds; > + return num_pds; > + > +err_link: > + dev_pm_domain_detach(pd_dev, true); > +err_attach: > + while (--i >= 0) { > + if (pds->pd_links[i]) > + device_link_del(pds->pd_links[i]); > + dev_pm_domain_detach(pds->pd_devs[i], true); > + } > + return ret; > +} > +EXPORT_SYMBOL_GPL(dev_pm_domain_attach_list); > + > /** > * dev_pm_domain_detach - Detach a device from its PM domain. > * @dev: Device to detach. > @@ -187,6 +295,31 @@ void dev_pm_domain_detach(struct device *dev, bool power_off) > } > EXPORT_SYMBOL_GPL(dev_pm_domain_detach); > > +/** > + * dev_pm_domain_detach_list - Detach a list of PM domains. > + * @list: The list of PM domains to detach. > + * > + * This function reverse the actions from dev_pm_domain_attach_list(). > + * Typically it should be invoked during the remove phase from drivers. > + * > + * Callers must ensure proper synchronization of this function with power > + * management callbacks. > + */ > +void dev_pm_domain_detach_list(struct dev_pm_domain_list *list) > +{ > + int i; > + > + if (!list) > + return; > + > + for (i = 0; i < list->num_pds; i++) { > + if (list->pd_links[i]) > + device_link_del(list->pd_links[i]); > + dev_pm_domain_detach(list->pd_devs[i], true); > + } > +} > +EXPORT_SYMBOL_GPL(dev_pm_domain_detach_list); > + > /** > * dev_pm_domain_start - Start the device through its PM domain. > * @dev: Device to start. > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 34663d0d5c55..6b71fb69c349 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -19,6 +19,33 @@ > #include <linux/cpumask.h> > #include <linux/time64.h> > > +/* > + * Flags to control the behaviour when attaching a device to its PM domains. > + * > + * PD_FLAG_NO_DEV_LINK: As the default behaviour creates a device-link > + * for every PM domain that gets attached, this > + * flag can be used to skip that. > + * > + * PD_FLAG_DEV_LINK_ON: Add the DL_FLAG_RPM_ACTIVE to power-on the > + * supplier and its PM domain when creating the > + * device-links. > + * > + */ > +#define PD_FLAG_NO_DEV_LINK BIT(0) > +#define PD_FLAG_DEV_LINK_ON BIT(1) > + > +struct dev_pm_domain_attach_data { > + const char * const *pd_names; > + const u32 num_pd_names; > + const u32 pd_flags; > +}; > + > +struct dev_pm_domain_list { > + struct device **pd_devs; > + struct device_link **pd_links; > + u32 num_pds; > +}; > + > /* > * Flags to control the behaviour of a genpd. > * > @@ -432,7 +459,11 @@ struct device *dev_pm_domain_attach_by_id(struct device *dev, > unsigned int index); > struct device *dev_pm_domain_attach_by_name(struct device *dev, > const char *name); > +int dev_pm_domain_attach_list(struct device *dev, > + const struct dev_pm_domain_attach_data *data, > + struct dev_pm_domain_list **list); > void dev_pm_domain_detach(struct device *dev, bool power_off); > +void dev_pm_domain_detach_list(struct dev_pm_domain_list *list); > int dev_pm_domain_start(struct device *dev); > void dev_pm_domain_set(struct device *dev, struct dev_pm_domain *pd); > int dev_pm_domain_set_performance_state(struct device *dev, unsigned int state); > @@ -451,7 +482,14 @@ static inline struct device *dev_pm_domain_attach_by_name(struct device *dev, > { > return NULL; > } > +static inline int dev_pm_domain_attach_list(struct device *dev, > + const struct dev_pm_domain_attach_data *data, > + struct dev_pm_domain_list **list) > +{ > + return 0; > +} > static inline void dev_pm_domain_detach(struct device *dev, bool power_off) {} > +static inline void dev_pm_domain_detach_list(struct dev_pm_domain_list *list) {} > static inline int dev_pm_domain_start(struct device *dev) > { > return 0;
On 28-12-23, 12:41, Ulf Hansson wrote: > For OPP integration, as a follow up I am striving to make the > dev_pm_opp_attach_genpd() redundant. Instead I think we should move towards > using dev_pm_opp_set_config()->_opp_set_required_devs(), which would allow us to > use the helpers that $subject series is adding. Big thanks for that :)
Hi Ulf, I'm in agreement with the modifications done to imx_rproc.c and imx_dsp_rproc.c. There is one thing I am ambivalent on, please see below. On Thu, Dec 28, 2023 at 12:41:55PM +0100, Ulf Hansson wrote: > Let's avoid the boilerplate code to manage the multiple PM domain case, by > converting into using dev_pm_domain_attach|detach_list(). > > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > Cc: Bjorn Andersson <andersson@kernel.org> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: <linux-remoteproc@vger.kernel.org> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/remoteproc/imx_rproc.c | 73 +++++----------------------------- > 1 file changed, 9 insertions(+), 64 deletions(-) > > diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c > index 8bb293b9f327..3161f14442bc 100644 > --- a/drivers/remoteproc/imx_rproc.c > +++ b/drivers/remoteproc/imx_rproc.c > @@ -92,7 +92,6 @@ struct imx_rproc_mem { > > static int imx_rproc_xtr_mbox_init(struct rproc *rproc); > static void imx_rproc_free_mbox(struct rproc *rproc); > -static int imx_rproc_detach_pd(struct rproc *rproc); > > struct imx_rproc { > struct device *dev; > @@ -113,10 +112,8 @@ struct imx_rproc { > u32 rproc_pt; /* partition id */ > u32 rsrc_id; /* resource id */ > u32 entry; /* cpu start address */ > - int num_pd; > u32 core_index; > - struct device **pd_dev; > - struct device_link **pd_dev_link; > + struct dev_pm_domain_list *pd_list; > }; > > static const struct imx_rproc_att imx_rproc_att_imx93[] = { > @@ -853,7 +850,7 @@ static void imx_rproc_put_scu(struct rproc *rproc) > return; > > if (imx_sc_rm_is_resource_owned(priv->ipc_handle, priv->rsrc_id)) { > - imx_rproc_detach_pd(rproc); > + dev_pm_domain_detach_list(priv->pd_list); > return; > } > > @@ -880,72 +877,20 @@ static int imx_rproc_partition_notify(struct notifier_block *nb, > static int imx_rproc_attach_pd(struct imx_rproc *priv) > { > struct device *dev = priv->dev; > - int ret, i; > - > - /* > - * If there is only one power-domain entry, the platform driver framework > - * will handle it, no need handle it in this driver. > - */ > - priv->num_pd = of_count_phandle_with_args(dev->of_node, "power-domains", > - "#power-domain-cells"); > - if (priv->num_pd <= 1) > - return 0; In function dev_pm_domain_attach_list(), this condition is "<= 0" rather than "<= 1". As such the association between the device and power domain will be done twice when there is a single power domain, i.e once by the core and once in dev_pm_domain_attach_list(). I am assuming the runtime PM subsystem is smart enough to deal with this kind of situation but would like a confirmation. Thanks, Mathieu > - > - priv->pd_dev = devm_kmalloc_array(dev, priv->num_pd, sizeof(*priv->pd_dev), GFP_KERNEL); > - if (!priv->pd_dev) > - return -ENOMEM; > - > - priv->pd_dev_link = devm_kmalloc_array(dev, priv->num_pd, sizeof(*priv->pd_dev_link), > - GFP_KERNEL); > - > - if (!priv->pd_dev_link) > - return -ENOMEM; > - > - for (i = 0; i < priv->num_pd; i++) { > - priv->pd_dev[i] = dev_pm_domain_attach_by_id(dev, i); > - if (IS_ERR(priv->pd_dev[i])) { > - ret = PTR_ERR(priv->pd_dev[i]); > - goto detach_pd; > - } > - > - priv->pd_dev_link[i] = device_link_add(dev, priv->pd_dev[i], DL_FLAG_STATELESS | > - DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE); > - if (!priv->pd_dev_link[i]) { > - dev_pm_domain_detach(priv->pd_dev[i], false); > - ret = -EINVAL; > - goto detach_pd; > - } > - } > - > - return 0; > - > -detach_pd: > - while (--i >= 0) { > - device_link_del(priv->pd_dev_link[i]); > - dev_pm_domain_detach(priv->pd_dev[i], false); > - } > - > - return ret; > -} > - > -static int imx_rproc_detach_pd(struct rproc *rproc) > -{ > - struct imx_rproc *priv = rproc->priv; > - int i; > + int ret; > + struct dev_pm_domain_attach_data pd_data = { > + .pd_flags = PD_FLAG_DEV_LINK_ON, > + }; > > /* > * If there is only one power-domain entry, the platform driver framework > * will handle it, no need handle it in this driver. > */ > - if (priv->num_pd <= 1) > + if (dev->pm_domain) > return 0; > > - for (i = 0; i < priv->num_pd; i++) { > - device_link_del(priv->pd_dev_link[i]); > - dev_pm_domain_detach(priv->pd_dev[i], false); > - } > - > - return 0; > + ret = dev_pm_domain_attach_list(dev, &pd_data, &priv->pd_list); > + return ret < 0 ? ret : 0; > } > > static int imx_rproc_detect_mode(struct imx_rproc *priv) > -- > 2.34.1 >
Attaching/detaching of a device to multiple PM domains has started to become a common operation for many drivers, typically during ->probe() and ->remove(). In most cases, this has lead to lots of boilerplate code in the drivers. This series adds a pair of helper functions to manage the attach/detach of a device to its multiple PM domains. Moreover, a couple of drivers have been converted to use the new helpers as a proof of concept. Note 1) The changes in the drivers have only been compile tested, while the helpers have been tested along with a couple of local dummy drivers that I have hacked up to model both genpd providers and genpd consumers. Note 2) I was struggling to make up mind if we should have a separate helper to attach all available power-domains described in DT, rather than providing "NULL" to the dev_pm_domain_attach_list(). I decided not to, but please let me know if you prefer the other option. Note 3) For OPP integration, as a follow up I am striving to make the dev_pm_opp_attach_genpd() redundant. Instead I think we should move towards using dev_pm_opp_set_config()->_opp_set_required_devs(), which would allow us to use the helpers that $subject series is adding. Kind regards Ulf Hansson Ulf Hansson (5): PM: domains: Add helper functions to attach/detach multiple PM domains remoteproc: imx_dsp_rproc: Convert to dev_pm_domain_attach|detach_list() remoteproc: imx_rproc: Convert to dev_pm_domain_attach|detach_list() remoteproc: qcom_q6v5_adsp: Convert to dev_pm_domain_attach|detach_list() media: venus: Convert to dev_pm_domain_attach|detach_list() for vcodec drivers/base/power/common.c | 133 +++++++++++++++ drivers/media/platform/qcom/venus/core.c | 12 +- drivers/media/platform/qcom/venus/core.h | 7 +- .../media/platform/qcom/venus/pm_helpers.c | 48 ++---- drivers/remoteproc/imx_dsp_rproc.c | 82 +-------- drivers/remoteproc/imx_rproc.c | 73 +------- drivers/remoteproc/qcom_q6v5_adsp.c | 160 ++++++++---------- include/linux/pm_domain.h | 38 +++++ 8 files changed, 288 insertions(+), 265 deletions(-)