diff mbox series

[3/5] remoteproc: imx_rproc: Convert to dev_pm_domain_attach|detach_list()

Message ID 20231228114157.104822-4-ulf.hansson@linaro.org
State Superseded
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
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(-)

Comments

Mathieu Poirier Jan. 2, 2024, 6:41 p.m. UTC | #1
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
>
Ulf Hansson Jan. 3, 2024, 10:11 a.m. UTC | #2
On Tue, 2 Jan 2024 at 19:41, Mathieu Poirier <mathieu.poirier@linaro.org> 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 <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 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
Mathieu Poirier Jan. 3, 2024, 4:19 p.m. UTC | #3
On Wed, 3 Jan 2024 at 03:11, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 2 Jan 2024 at 19:41, Mathieu Poirier <mathieu.poirier@linaro.org> 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 <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 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
diff mbox series

Patch

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)