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(-)
--
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.
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 <[email protected]>
---
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)
+ 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;
--
2.34.1
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 <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: <[email protected]>
Signed-off-by: Ulf Hansson <[email protected]>
---
drivers/remoteproc/imx_dsp_rproc.c | 82 ++++--------------------------
1 file changed, 9 insertions(+), 73 deletions(-)
diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
index 8fcda9b74545..0409b7c47d5c 100644
--- a/drivers/remoteproc/imx_dsp_rproc.c
+++ b/drivers/remoteproc/imx_dsp_rproc.c
@@ -103,12 +103,10 @@ enum imx_dsp_rp_mbox_messages {
* @tx_ch: mailbox tx channel handle
* @rx_ch: mailbox rx channel handle
* @rxdb_ch: mailbox rx doorbell channel handle
- * @pd_dev: power domain device
- * @pd_dev_link: power domain device link
+ * @pd_list: power domain list
* @ipc_handle: System Control Unit ipc handle
* @rproc_work: work for processing virtio interrupts
* @pm_comp: completion primitive to sync for suspend response
- * @num_domains: power domain number
* @flags: control flags
*/
struct imx_dsp_rproc {
@@ -121,12 +119,10 @@ struct imx_dsp_rproc {
struct mbox_chan *tx_ch;
struct mbox_chan *rx_ch;
struct mbox_chan *rxdb_ch;
- struct device **pd_dev;
- struct device_link **pd_dev_link;
+ struct dev_pm_domain_list *pd_list;
struct imx_sc_ipc *ipc_handle;
struct work_struct rproc_work;
struct completion pm_comp;
- int num_domains;
u32 flags;
};
@@ -954,74 +950,14 @@ static const struct rproc_ops imx_dsp_rproc_ops = {
static int imx_dsp_attach_pm_domains(struct imx_dsp_rproc *priv)
{
struct device *dev = priv->rproc->dev.parent;
- int ret, i;
-
- priv->num_domains = of_count_phandle_with_args(dev->of_node,
- "power-domains",
- "#power-domain-cells");
-
- /* If only one domain, then no need to link the device */
- if (priv->num_domains <= 1)
- return 0;
-
- priv->pd_dev = devm_kmalloc_array(dev, priv->num_domains,
- sizeof(*priv->pd_dev),
- GFP_KERNEL);
- if (!priv->pd_dev)
- return -ENOMEM;
-
- priv->pd_dev_link = devm_kmalloc_array(dev, priv->num_domains,
- sizeof(*priv->pd_dev_link),
- GFP_KERNEL);
- if (!priv->pd_dev_link)
- return -ENOMEM;
-
- for (i = 0; i < priv->num_domains; 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_pm;
- }
-
- /*
- * device_link_add will check priv->pd_dev[i], if it is
- * NULL, then will break.
- */
- priv->pd_dev_link[i] = device_link_add(dev,
- priv->pd_dev[i],
- DL_FLAG_STATELESS |
- DL_FLAG_PM_RUNTIME);
- if (!priv->pd_dev_link[i]) {
- dev_pm_domain_detach(priv->pd_dev[i], false);
- ret = -EINVAL;
- goto detach_pm;
- }
- }
-
- return 0;
-
-detach_pm:
- 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_dsp_detach_pm_domains(struct imx_dsp_rproc *priv)
-{
- int i;
+ int ret;
- if (priv->num_domains <= 1)
+ /* A single PM domain is already attached. */
+ if (dev->pm_domain)
return 0;
- for (i = 0; i < priv->num_domains; 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, NULL, &priv->pd_list);
+ return ret < 0 ? ret : 0;
}
/**
@@ -1153,7 +1089,7 @@ static int imx_dsp_rproc_probe(struct platform_device *pdev)
return 0;
err_detach_domains:
- imx_dsp_detach_pm_domains(priv);
+ dev_pm_domain_detach_list(priv->pd_list);
err_put_rproc:
rproc_free(rproc);
@@ -1167,7 +1103,7 @@ static void imx_dsp_rproc_remove(struct platform_device *pdev)
pm_runtime_disable(&pdev->dev);
rproc_del(rproc);
- imx_dsp_detach_pm_domains(priv);
+ dev_pm_domain_detach_list(priv->pd_list);
rproc_free(rproc);
}
--
2.34.1
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 <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: <[email protected]>
Signed-off-by: Ulf Hansson <[email protected]>
---
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;
-
- 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
Let's avoid some of the boilerplate code to manage the various PM domain
cases, by converting into using dev_pm_domain_attach|detach_list().
As a part of the conversion, we are moving over to use device_links, which
simplifies the runtime PM support too. Moreover, while attaching let's
trust that an already attached single PM domain is the correct one.
Cc: Mathieu Poirier <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Konrad Dybcio <[email protected]>
Cc: <[email protected]>
Signed-off-by: Ulf Hansson <[email protected]>
---
drivers/remoteproc/qcom_q6v5_adsp.c | 160 +++++++++++++---------------
1 file changed, 73 insertions(+), 87 deletions(-)
diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
index 6c67514cc493..93f9a1537ec6 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -55,8 +55,6 @@
#define QDSP6SS_CORE_CBCR 0x20
#define QDSP6SS_SLEEP_CBCR 0x3c
-#define QCOM_Q6V5_RPROC_PROXY_PD_MAX 3
-
#define LPASS_BOOT_CORE_START BIT(0)
#define LPASS_BOOT_CMD_START BIT(0)
#define LPASS_EFUSE_Q6SS_EVB_SEL 0x0
@@ -74,7 +72,8 @@ struct adsp_pil_data {
const char **clk_ids;
int num_clks;
- const char **proxy_pd_names;
+ const char **pd_names;
+ unsigned int num_pds;
const char *load_state;
};
@@ -110,8 +109,7 @@ struct qcom_adsp {
size_t mem_size;
bool has_iommu;
- struct device *proxy_pds[QCOM_Q6V5_RPROC_PROXY_PD_MAX];
- size_t proxy_pd_count;
+ struct dev_pm_domain_list *pd_list;
struct qcom_rproc_glink glink_subdev;
struct qcom_rproc_ssr ssr_subdev;
@@ -120,98 +118,92 @@ struct qcom_adsp {
int (*shutdown)(struct qcom_adsp *adsp);
};
-static int qcom_rproc_pds_attach(struct device *dev, struct qcom_adsp *adsp,
- const char **pd_names)
+static int qcom_rproc_pds_attach(struct qcom_adsp *adsp, const char **pd_names,
+ unsigned int num_pds)
{
- struct device **devs = adsp->proxy_pds;
- size_t num_pds = 0;
+ struct device *dev = adsp->dev;
+ struct dev_pm_domain_attach_data pd_data = {
+ .pd_names = pd_names,
+ .num_pd_names = num_pds,
+ };
int ret;
- int i;
-
- if (!pd_names)
- return 0;
/* Handle single power domain */
- if (dev->pm_domain) {
- devs[0] = dev;
- pm_runtime_enable(dev);
- return 1;
- }
+ if (dev->pm_domain)
+ goto out;
- while (pd_names[num_pds])
- num_pds++;
+ if (!pd_names)
+ return 0;
- if (num_pds > ARRAY_SIZE(adsp->proxy_pds))
- return -E2BIG;
+ ret = dev_pm_domain_attach_list(dev, &pd_data, &adsp->pd_list);
+ if (ret < 0)
+ return ret;
- for (i = 0; i < num_pds; i++) {
- devs[i] = dev_pm_domain_attach_by_name(dev, pd_names[i]);
- if (IS_ERR_OR_NULL(devs[i])) {
- ret = PTR_ERR(devs[i]) ? : -ENODATA;
- goto unroll_attach;
- }
- }
+out:
+ pm_runtime_enable(dev);
+ return 0;
+}
- return num_pds;
+static void qcom_rproc_pds_detach(struct qcom_adsp *adsp)
+{
+ struct device *dev = adsp->dev;
+ struct dev_pm_domain_list *pds = adsp->pd_list;
-unroll_attach:
- for (i--; i >= 0; i--)
- dev_pm_domain_detach(devs[i], false);
+ dev_pm_domain_detach_list(pds);
- return ret;
+ if (dev->pm_domain || pds)
+ pm_runtime_disable(adsp->dev);
}
-static void qcom_rproc_pds_detach(struct qcom_adsp *adsp, struct device **pds,
- size_t pd_count)
+static int qcom_rproc_pds_enable(struct qcom_adsp *adsp)
{
struct device *dev = adsp->dev;
- int i;
+ struct dev_pm_domain_list *pds = adsp->pd_list;
+ int ret, i = 0;
- /* Handle single power domain */
- if (dev->pm_domain && pd_count) {
- pm_runtime_disable(dev);
- return;
- }
+ if (!dev->pm_domain && !pds)
+ return 0;
- for (i = 0; i < pd_count; i++)
- dev_pm_domain_detach(pds[i], false);
-}
+ if (dev->pm_domain)
+ dev_pm_genpd_set_performance_state(dev, INT_MAX);
-static int qcom_rproc_pds_enable(struct qcom_adsp *adsp, struct device **pds,
- size_t pd_count)
-{
- int ret;
- int i;
-
- for (i = 0; i < pd_count; i++) {
- dev_pm_genpd_set_performance_state(pds[i], INT_MAX);
- ret = pm_runtime_resume_and_get(pds[i]);
- if (ret < 0) {
- dev_pm_genpd_set_performance_state(pds[i], 0);
- goto unroll_pd_votes;
- }
+ while (pds && i < pds->num_pds) {
+ dev_pm_genpd_set_performance_state(pds->pd_devs[i], INT_MAX);
+ i++;
}
- return 0;
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret < 0) {
+ while (pds && i > 0) {
+ i--;
+ dev_pm_genpd_set_performance_state(pds->pd_devs[i], 0);
+ }
-unroll_pd_votes:
- for (i--; i >= 0; i--) {
- dev_pm_genpd_set_performance_state(pds[i], 0);
- pm_runtime_put(pds[i]);
+ if (dev->pm_domain)
+ dev_pm_genpd_set_performance_state(dev, 0);
}
return ret;
}
-static void qcom_rproc_pds_disable(struct qcom_adsp *adsp, struct device **pds,
- size_t pd_count)
+static void qcom_rproc_pds_disable(struct qcom_adsp *adsp)
{
- int i;
+ struct device *dev = adsp->dev;
+ struct dev_pm_domain_list *pds = adsp->pd_list;
+ int i = 0;
+
+ if (!dev->pm_domain && !pds)
+ return;
+
+ if (dev->pm_domain)
+ dev_pm_genpd_set_performance_state(dev, 0);
- for (i = 0; i < pd_count; i++) {
- dev_pm_genpd_set_performance_state(pds[i], 0);
- pm_runtime_put(pds[i]);
+ while (pds && i < pds->num_pds) {
+ dev_pm_genpd_set_performance_state(pds->pd_devs[i], 0);
+ i++;
}
+
+ pm_runtime_put(dev);
}
static int qcom_wpss_shutdown(struct qcom_adsp *adsp)
@@ -397,8 +389,7 @@ static int adsp_start(struct rproc *rproc)
if (ret)
goto adsp_smmu_unmap;
- ret = qcom_rproc_pds_enable(adsp, adsp->proxy_pds,
- adsp->proxy_pd_count);
+ ret = qcom_rproc_pds_enable(adsp);
if (ret < 0)
goto disable_xo_clk;
@@ -448,7 +439,7 @@ static int adsp_start(struct rproc *rproc)
disable_adsp_clks:
clk_bulk_disable_unprepare(adsp->num_clks, adsp->clks);
disable_power_domain:
- qcom_rproc_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
+ qcom_rproc_pds_disable(adsp);
disable_xo_clk:
clk_disable_unprepare(adsp->xo);
adsp_smmu_unmap:
@@ -464,7 +455,7 @@ static void qcom_adsp_pil_handover(struct qcom_q6v5 *q6v5)
struct qcom_adsp *adsp = container_of(q6v5, struct qcom_adsp, q6v5);
clk_disable_unprepare(adsp->xo);
- qcom_rproc_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
+ qcom_rproc_pds_disable(adsp);
}
static int adsp_stop(struct rproc *rproc)
@@ -715,13 +706,11 @@ static int adsp_probe(struct platform_device *pdev)
if (ret)
goto free_rproc;
- ret = qcom_rproc_pds_attach(adsp->dev, adsp,
- desc->proxy_pd_names);
+ ret = qcom_rproc_pds_attach(adsp, desc->pd_names, desc->num_pds);
if (ret < 0) {
dev_err(&pdev->dev, "Failed to attach proxy power domains\n");
goto free_rproc;
}
- adsp->proxy_pd_count = ret;
ret = adsp_init_reset(adsp);
if (ret)
@@ -753,7 +742,7 @@ static int adsp_probe(struct platform_device *pdev)
return 0;
disable_pm:
- qcom_rproc_pds_detach(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
+ qcom_rproc_pds_detach(adsp);
free_rproc:
rproc_free(rproc);
@@ -771,7 +760,7 @@ static void adsp_remove(struct platform_device *pdev)
qcom_remove_glink_subdev(adsp->rproc, &adsp->glink_subdev);
qcom_remove_sysmon_subdev(adsp->sysmon);
qcom_remove_ssr_subdev(adsp->rproc, &adsp->ssr_subdev);
- qcom_rproc_pds_detach(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
+ qcom_rproc_pds_detach(adsp);
rproc_free(adsp->rproc);
}
@@ -788,9 +777,8 @@ static const struct adsp_pil_data adsp_resource_init = {
"qdsp6ss_xo", "qdsp6ss_sleep", "qdsp6ss_core", NULL
},
.num_clks = 7,
- .proxy_pd_names = (const char*[]) {
- "cx", NULL
- },
+ .pd_names = (const char*[]) { "cx" },
+ .num_pds = 1,
};
static const struct adsp_pil_data adsp_sc7280_resource_init = {
@@ -821,9 +809,8 @@ static const struct adsp_pil_data cdsp_resource_init = {
"q6_axim", NULL
},
.num_clks = 7,
- .proxy_pd_names = (const char*[]) {
- "cx", NULL
- },
+ .pd_names = (const char*[]) { "cx" },
+ .num_pds = 1,
};
static const struct adsp_pil_data wpss_resource_init = {
@@ -839,9 +826,8 @@ static const struct adsp_pil_data wpss_resource_init = {
"ahb_bdg", "ahb", "rscp", NULL
},
.num_clks = 3,
- .proxy_pd_names = (const char*[]) {
- "cx", "mx", NULL
- },
+ .pd_names = (const char*[]) { "cx", "mx" },
+ .num_pds = 2,
};
static const struct of_device_id adsp_of_match[] = {
--
2.34.1
Let's avoid some of the boilerplate code to manage the vcodec PM domains,
by converting into using dev_pm_domain_attach|detach_list().
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Stanimir Varbanov <[email protected]>
Cc: Vikash Garodia <[email protected]>
Cc: "Bryan O'Donoghue" <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Konrad Dybcio <[email protected]>
Cc: <[email protected]>
Signed-off-by: Ulf Hansson <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 12 +++--
drivers/media/platform/qcom/venus/core.h | 7 ++-
.../media/platform/qcom/venus/pm_helpers.c | 48 +++++++------------
3 files changed, 26 insertions(+), 41 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 9cffe975581b..bd9b474280e4 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -16,6 +16,7 @@
#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/types.h>
+#include <linux/pm_domain.h>
#include <linux/pm_runtime.h>
#include <media/videobuf2-v4l2.h>
#include <media/v4l2-mem2mem.h>
@@ -114,7 +115,8 @@ static void venus_sys_error_handler(struct work_struct *work)
pm_runtime_put_sync(core->dev);
for (i = 0; i < max_attempts; i++) {
- if (!core->pmdomains[0] || !pm_runtime_active(core->pmdomains[0]))
+ if (!core->pmdomains ||
+ !pm_runtime_active(core->pmdomains->pd_devs[0]))
break;
usleep_range(1000, 1500);
}
@@ -705,7 +707,7 @@ static const struct venus_resources sdm845_res_v2 = {
.vcodec0_clks = { "vcodec0_core", "vcodec0_bus" },
.vcodec1_clks = { "vcodec1_core", "vcodec1_bus" },
.vcodec_clks_num = 2,
- .vcodec_pmdomains = { "venus", "vcodec0", "vcodec1" },
+ .vcodec_pmdomains = (const char *[]) { "venus", "vcodec0", "vcodec1" },
.vcodec_pmdomains_num = 3,
.opp_pmdomain = (const char *[]) { "cx", NULL },
.vcodec_num = 2,
@@ -754,7 +756,7 @@ static const struct venus_resources sc7180_res = {
.clks_num = 3,
.vcodec0_clks = { "vcodec0_core", "vcodec0_bus" },
.vcodec_clks_num = 2,
- .vcodec_pmdomains = { "venus", "vcodec0" },
+ .vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
.vcodec_pmdomains_num = 2,
.opp_pmdomain = (const char *[]) { "cx", NULL },
.vcodec_num = 1,
@@ -811,7 +813,7 @@ static const struct venus_resources sm8250_res = {
.resets_num = 2,
.vcodec0_clks = { "vcodec0_core" },
.vcodec_clks_num = 1,
- .vcodec_pmdomains = { "venus", "vcodec0" },
+ .vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
.vcodec_pmdomains_num = 2,
.opp_pmdomain = (const char *[]) { "mx", NULL },
.vcodec_num = 1,
@@ -870,7 +872,7 @@ static const struct venus_resources sc7280_res = {
.clks_num = 3,
.vcodec0_clks = {"vcodec_core", "vcodec_bus"},
.vcodec_clks_num = 2,
- .vcodec_pmdomains = { "venus", "vcodec0" },
+ .vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
.vcodec_pmdomains_num = 2,
.opp_pmdomain = (const char *[]) { "cx", NULL },
.vcodec_num = 1,
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 4a633261ece4..7ef341bf21cc 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -25,7 +25,6 @@
#define VIDC_CLKS_NUM_MAX 4
#define VIDC_VCODEC_CLKS_NUM_MAX 2
-#define VIDC_PMDOMAINS_NUM_MAX 3
#define VIDC_RESETS_NUM_MAX 2
extern int venus_fw_debug;
@@ -72,7 +71,7 @@ struct venus_resources {
const char * const vcodec0_clks[VIDC_VCODEC_CLKS_NUM_MAX];
const char * const vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
unsigned int vcodec_clks_num;
- const char * const vcodec_pmdomains[VIDC_PMDOMAINS_NUM_MAX];
+ const char **vcodec_pmdomains;
unsigned int vcodec_pmdomains_num;
const char **opp_pmdomain;
unsigned int vcodec_num;
@@ -134,7 +133,7 @@ struct venus_format {
* @video_path: an interconnect handle to video to/from memory path
* @cpucfg_path: an interconnect handle to cpu configuration path
* @has_opp_table: does OPP table exist
- * @pmdomains: an array of pmdomains struct device pointers
+ * @pmdomains: a pointer to a list of pmdomains
* @opp_dl_venus: an device-link for device OPP
* @opp_pmdomain: an OPP power-domain
* @resets: an array of reset signals
@@ -187,7 +186,7 @@ struct venus_core {
struct icc_path *video_path;
struct icc_path *cpucfg_path;
bool has_opp_table;
- struct device *pmdomains[VIDC_PMDOMAINS_NUM_MAX];
+ struct dev_pm_domain_list *pmdomains;
struct device_link *opp_dl_venus;
struct device *opp_pmdomain;
struct reset_control *resets[VIDC_RESETS_NUM_MAX];
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index a1b127caa90a..502822059498 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -455,7 +455,7 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
if (ret)
return ret;
- ret = pm_runtime_put_sync(core->pmdomains[1]);
+ ret = pm_runtime_put_sync(core->pmdomains->pd_devs[1]);
if (ret < 0)
return ret;
}
@@ -471,7 +471,7 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
if (ret)
return ret;
- ret = pm_runtime_put_sync(core->pmdomains[2]);
+ ret = pm_runtime_put_sync(core->pmdomains->pd_devs[2]);
if (ret < 0)
return ret;
}
@@ -484,7 +484,7 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
int ret;
if (coreid_mask & VIDC_CORE_ID_1) {
- ret = pm_runtime_get_sync(core->pmdomains[1]);
+ ret = pm_runtime_get_sync(core->pmdomains->pd_devs[1]);
if (ret < 0)
return ret;
@@ -502,7 +502,7 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
}
if (coreid_mask & VIDC_CORE_ID_2) {
- ret = pm_runtime_get_sync(core->pmdomains[2]);
+ ret = pm_runtime_get_sync(core->pmdomains->pd_devs[2]);
if (ret < 0)
return ret;
@@ -860,19 +860,18 @@ static int vcodec_domains_get(struct venus_core *core)
struct device **opp_virt_dev;
struct device *dev = core->dev;
const struct venus_resources *res = core->res;
- struct device *pd;
- unsigned int i;
+ struct dev_pm_domain_attach_data vcodec_data = {
+ .pd_names = res->vcodec_pmdomains,
+ .num_pd_names = res->vcodec_pmdomains_num,
+ .pd_flags = PD_FLAG_NO_DEV_LINK,
+ };
if (!res->vcodec_pmdomains_num)
goto skip_pmdomains;
- for (i = 0; i < res->vcodec_pmdomains_num; i++) {
- pd = dev_pm_domain_attach_by_name(dev,
- res->vcodec_pmdomains[i]);
- if (IS_ERR_OR_NULL(pd))
- return pd ? PTR_ERR(pd) : -ENODATA;
- core->pmdomains[i] = pd;
- }
+ ret = dev_pm_domain_attach_list(dev, &vcodec_data, &core->pmdomains);
+ if (ret < 0)
+ return ret;
skip_pmdomains:
if (!core->res->opp_pmdomain)
@@ -896,30 +895,14 @@ static int vcodec_domains_get(struct venus_core *core)
return 0;
opp_attach_err:
- for (i = 0; i < res->vcodec_pmdomains_num; i++) {
- if (IS_ERR_OR_NULL(core->pmdomains[i]))
- continue;
- dev_pm_domain_detach(core->pmdomains[i], true);
- }
-
+ dev_pm_domain_detach_list(core->pmdomains);
return ret;
}
static void vcodec_domains_put(struct venus_core *core)
{
- const struct venus_resources *res = core->res;
- unsigned int i;
+ dev_pm_domain_detach_list(core->pmdomains);
- if (!res->vcodec_pmdomains_num)
- goto skip_pmdomains;
-
- for (i = 0; i < res->vcodec_pmdomains_num; i++) {
- if (IS_ERR_OR_NULL(core->pmdomains[i]))
- continue;
- dev_pm_domain_detach(core->pmdomains[i], true);
- }
-
-skip_pmdomains:
if (!core->has_opp_table)
return;
@@ -1035,7 +1018,8 @@ static void core_put_v4(struct venus_core *core)
static int core_power_v4(struct venus_core *core, int on)
{
struct device *dev = core->dev;
- struct device *pmctrl = core->pmdomains[0];
+ struct device *pmctrl = core->pmdomains ?
+ core->pmdomains->pd_devs[0] : NULL;
int ret = 0;
if (on == POWER_ON) {
--
2.34.1
On 28/12/2023 11:41, Ulf Hansson wrote:
> Let's avoid some of the boilerplate code to manage the vcodec PM domains,
> by converting into using dev_pm_domain_attach|detach_list().
>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Stanimir Varbanov <[email protected]>
> Cc: Vikash Garodia <[email protected]>
> Cc: "Bryan O'Donoghue" <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Konrad Dybcio <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Ulf Hansson <[email protected]>
On top of 39676dfe52331 - (tag: next-20231222, linux-next/master) Add
linux-next specific files for 20231222 (7 days ago)
Tested-by: Bryan O'Donoghue <[email protected]>
Reviewed-by: Bryan O'Donoghue <[email protected]>
---
bod
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 <[email protected]>
> ---
> 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 :)
--
viresh
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 <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Shawn Guo <[email protected]>
> Cc: Sascha Hauer <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Ulf Hansson <[email protected]>
> ---
> 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
>
On Tue, 2 Jan 2024 at 19:41, Mathieu Poirier <[email protected]> wrote:
>
> 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 <[email protected]>
> > Cc: Bjorn Andersson <[email protected]>
> > Cc: Shawn Guo <[email protected]>
> > Cc: Sascha Hauer <[email protected]>
> > Cc: <[email protected]>
> > Signed-off-by: Ulf Hansson <[email protected]>
> > ---
> > 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 for reviewing!
To cover the the single PM domain case, imx_rproc_attach_pd() is
returning 0 when dev->pm_domain has been assigned. Moreover,
dev_pm_domain_attach_list() doesn't allow attaching in the single PM
domain case, as it returns -EEXIST if "dev->pm_domain" is already
assigned.
Did that make sense to you?
[...]
Kind regards
Uffe
On Fri, 29 Dec 2023 at 21:21, Nikunj Kela <[email protected]> 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 <[email protected]>
> > ---
> > 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
On Wed, 3 Jan 2024 at 03:11, Ulf Hansson <[email protected]> wrote:
>
> On Tue, 2 Jan 2024 at 19:41, Mathieu Poirier <[email protected]> wrote:
> >
> > 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 <[email protected]>
> > > Cc: Bjorn Andersson <[email protected]>
> > > Cc: Shawn Guo <[email protected]>
> > > Cc: Sascha Hauer <[email protected]>
> > > Cc: <[email protected]>
> > > Signed-off-by: Ulf Hansson <[email protected]>
> > > ---
> > > 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 for reviewing!
>
> To cover the the single PM domain case, imx_rproc_attach_pd() is
> returning 0 when dev->pm_domain has been assigned. Moreover,
> dev_pm_domain_attach_list() doesn't allow attaching in the single PM
> domain case, as it returns -EEXIST if "dev->pm_domain" is already
> assigned.
>
> Did that make sense to you?
>
Ah yes! The correlation between dev->pm_domain and the magic done by
the core framework was lost on me.
Once you respin to address Nikunj's comment I will ask the NXP team in
Romania to test this set. With the holiday season still floating in
the air it may take a little while for them to get to it.
> [...]
>
> Kind regards
> Uffe