diff mbox series

[1/5] PM: domains: Add helper functions to attach/detach multiple PM domains

Message ID 20231228114157.104822-2-ulf.hansson@linaro.org (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series PM: domains: Add helpers for multi PM domains to avoid open-coding | expand

Commit Message

Ulf Hansson Dec. 28, 2023, 11:41 a.m. UTC
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(+)

Comments

Nikunj Kela Dec. 29, 2023, 8:21 p.m. UTC | #1
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;
Ulf Hansson Jan. 3, 2024, 12:49 p.m. UTC | #2
On Fri, 29 Dec 2023 at 21:21, Nikunj Kela <quic_nkela@quicinc.com> wrote:
>
>
> 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

Thanks for spotting this! I certainly tested that option too, but must
have been on some earlier internal version. :-)

I will iterate the series tomorrow to fix this up.

[...]

Kind regards
Uffe
diff mbox series

Patch

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)
+		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;