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@xxxxxxxxxx>
> Cc: Bjorn Andersson <andersson@xxxxxxxxxx>
> Cc: Shawn Guo <shawnguo@xxxxxxxxxx>
> Cc: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> Cc: <linux-remoteproc@xxxxxxxxxxxxxxx>
> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> ---
> 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
>
