remoteproc: imx_rproc: misc clean up

[PATCH 1/5] remoteproc: imx_rproc: Simplify clock enable logic using dcfg flags

Posted by Peng Fan 1 week, 6 days ago

Simplify the clock enable logic by removing the dedicated
imx_rproc_clk_enable() function and integrate the clock handling directly
into the probe function to simplify the code.

Add a new IMX_RPROC_NEED_CLKS flag in dcfg to indicate whether clock
management is required for a given SoC. Update probe logic to conditionally
enable clocks based on the new flag.

Set the flag for applicable SoCs (e.g., i.MX7D, i.MX8MQ, i.MX93, etc.).

No functional changes.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 37 ++++++++++++-------------------------
 drivers/remoteproc/imx_rproc.h |  1 +
 2 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 694fbbb2f34061de22a3a815f8a6114159585f9e..71617b20f9d0ae698e7f655aae22e8895434f32c 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -1002,28 +1002,6 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
 	return dcfg->ops->detect_mode(priv->rproc);
 }
 
-static int imx_rproc_clk_enable(struct imx_rproc *priv)
-{
-	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
-	struct device *dev = priv->dev;
-
-	/* Remote core is not under control of Linux or it is managed by SCU API */
-	if (dcfg->method == IMX_RPROC_NONE || dcfg->method == IMX_RPROC_SCU_API)
-		return 0;
-
-	/*
-	 * clk for M4 block including memory. Should be
-	 * enabled before .start for FW transfer.
-	 */
-	priv->clk = devm_clk_get_enabled(dev, NULL);
-	if (IS_ERR(priv->clk)) {
-		dev_err(dev, "Failed to enable clock\n");
-		return PTR_ERR(priv->clk);
-	}
-
-	return 0;
-}
-
 static int imx_rproc_sys_off_handler(struct sys_off_data *data)
 {
 	struct rproc *rproc = data->cb_data;
@@ -1108,9 +1086,12 @@ static int imx_rproc_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "failed on detect mode\n");
 
-	ret = imx_rproc_clk_enable(priv);
-	if (ret)
-		return dev_err_probe(dev, ret, "failed to enable clks\n");
+	/* Remote core is under control of Linux or clock is not managed by firmware */
+	if (dcfg->flags & IMX_RPROC_NEED_CLKS) {
+		priv->clk = devm_clk_get_enabled(dev, NULL);
+		if (IS_ERR(priv->clk))
+			return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to enable clock\n");
+	}
 
 	if (rproc->state != RPROC_DETACHED)
 		rproc->auto_boot = of_property_read_bool(np, "fsl,auto-boot");
@@ -1185,6 +1166,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mn_mmio = {
 	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8mn),
 	.method		= IMX_RPROC_MMIO,
 	.ops		= &imx_rproc_ops_mmio,
+	.flags		= IMX_RPROC_NEED_CLKS,
 };
 
 static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mn = {
@@ -1192,6 +1174,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mn = {
 	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8mn),
 	.method		= IMX_RPROC_SMC,
 	.ops		= &imx_rproc_ops_arm_smc,
+	.flags		= IMX_RPROC_NEED_CLKS,
 };
 
 static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mq = {
@@ -1203,6 +1186,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mq = {
 	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8mq),
 	.method		= IMX_RPROC_MMIO,
 	.ops		= &imx_rproc_ops_mmio,
+	.flags		= IMX_RPROC_NEED_CLKS,
 };
 
 static const struct imx_rproc_dcfg imx_rproc_cfg_imx8qm = {
@@ -1241,6 +1225,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
 	.att_size	= ARRAY_SIZE(imx_rproc_att_imx7d),
 	.method		= IMX_RPROC_MMIO,
 	.ops		= &imx_rproc_ops_mmio,
+	.flags		= IMX_RPROC_NEED_CLKS,
 };
 
 static const struct imx_rproc_dcfg imx_rproc_cfg_imx6sx = {
@@ -1252,6 +1237,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx6sx = {
 	.att_size	= ARRAY_SIZE(imx_rproc_att_imx6sx),
 	.method		= IMX_RPROC_MMIO,
 	.ops		= &imx_rproc_ops_mmio,
+	.flags		= IMX_RPROC_NEED_CLKS,
 };
 
 static const struct imx_rproc_dcfg imx_rproc_cfg_imx93 = {
@@ -1259,6 +1245,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx93 = {
 	.att_size	= ARRAY_SIZE(imx_rproc_att_imx93),
 	.method		= IMX_RPROC_SMC,
 	.ops		= &imx_rproc_ops_arm_smc,
+	.flags		= IMX_RPROC_NEED_CLKS,
 };
 
 static const struct of_device_id imx_rproc_of_match[] = {
diff --git a/drivers/remoteproc/imx_rproc.h b/drivers/remoteproc/imx_rproc.h
index 3a9adaaf048b396102feeb45488cd2ff125a807a..a9cba623560c85ea37e47401c392c06dada500aa 100644
--- a/drivers/remoteproc/imx_rproc.h
+++ b/drivers/remoteproc/imx_rproc.h
@@ -30,6 +30,7 @@ enum imx_rproc_method {
 
 /* dcfg flags */
 #define IMX_RPROC_NEED_SYSTEM_OFF	BIT(0)
+#define IMX_RPROC_NEED_CLKS		BIT(1)
 
 struct imx_rproc_plat_ops {
 	int (*start)(struct rproc *rproc);

-- 
2.37.1

Re: [PATCH 1/5] remoteproc: imx_rproc: Simplify clock enable logic using dcfg flags

Posted by Frank Li 1 week, 6 days ago

On Thu, Sep 18, 2025 at 07:51:16PM +0800, Peng Fan wrote:
> Simplify the clock enable logic by removing the dedicated
> imx_rproc_clk_enable() function and integrate the clock handling directly
> into the probe function to simplify the code.
>
> Add a new IMX_RPROC_NEED_CLKS flag in dcfg to indicate whether clock
> management is required for a given SoC. Update probe logic to conditionally
> enable clocks based on the new flag.
>
> Set the flag for applicable SoCs (e.g., i.MX7D, i.MX8MQ, i.MX93, etc.).
>
> No functional changes.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/remoteproc/imx_rproc.c | 37 ++++++++++++-------------------------
>  drivers/remoteproc/imx_rproc.h |  1 +
>  2 files changed, 13 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 694fbbb2f34061de22a3a815f8a6114159585f9e..71617b20f9d0ae698e7f655aae22e8895434f32c 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -1002,28 +1002,6 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
>  	return dcfg->ops->detect_mode(priv->rproc);
>  }
>
> -static int imx_rproc_clk_enable(struct imx_rproc *priv)
> -{
> -	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
> -	struct device *dev = priv->dev;
> -
> -	/* Remote core is not under control of Linux or it is managed by SCU API */
> -	if (dcfg->method == IMX_RPROC_NONE || dcfg->method == IMX_RPROC_SCU_API)
> -		return 0;
> -
> -	/*
> -	 * clk for M4 block including memory. Should be
> -	 * enabled before .start for FW transfer.
> -	 */
> -	priv->clk = devm_clk_get_enabled(dev, NULL);
> -	if (IS_ERR(priv->clk)) {
> -		dev_err(dev, "Failed to enable clock\n");
> -		return PTR_ERR(priv->clk);
> -	}
> -
> -	return 0;
> -}
> -
>  static int imx_rproc_sys_off_handler(struct sys_off_data *data)
>  {
>  	struct rproc *rproc = data->cb_data;
> @@ -1108,9 +1086,12 @@ static int imx_rproc_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed on detect mode\n");
>
> -	ret = imx_rproc_clk_enable(priv);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "failed to enable clks\n");
> +	/* Remote core is under control of Linux or clock is not managed by firmware */
> +	if (dcfg->flags & IMX_RPROC_NEED_CLKS) {
> +		priv->clk = devm_clk_get_enabled(dev, NULL);
> +		if (IS_ERR(priv->clk))
> +			return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to enable clock\n");
> +	}
>
>  	if (rproc->state != RPROC_DETACHED)
>  		rproc->auto_boot = of_property_read_bool(np, "fsl,auto-boot");
> @@ -1185,6 +1166,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mn_mmio = {
>  	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8mn),
>  	.method		= IMX_RPROC_MMIO,
>  	.ops		= &imx_rproc_ops_mmio,
> +	.flags		= IMX_RPROC_NEED_CLKS,
>  };
>
>  static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mn = {
> @@ -1192,6 +1174,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mn = {
>  	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8mn),
>  	.method		= IMX_RPROC_SMC,
>  	.ops		= &imx_rproc_ops_arm_smc,
> +	.flags		= IMX_RPROC_NEED_CLKS,
>  };
>
>  static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mq = {
> @@ -1203,6 +1186,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mq = {
>  	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8mq),
>  	.method		= IMX_RPROC_MMIO,
>  	.ops		= &imx_rproc_ops_mmio,
> +	.flags		= IMX_RPROC_NEED_CLKS,
>  };
>
>  static const struct imx_rproc_dcfg imx_rproc_cfg_imx8qm = {
> @@ -1241,6 +1225,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
>  	.att_size	= ARRAY_SIZE(imx_rproc_att_imx7d),
>  	.method		= IMX_RPROC_MMIO,
>  	.ops		= &imx_rproc_ops_mmio,
> +	.flags		= IMX_RPROC_NEED_CLKS,
>  };
>
>  static const struct imx_rproc_dcfg imx_rproc_cfg_imx6sx = {
> @@ -1252,6 +1237,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx6sx = {
>  	.att_size	= ARRAY_SIZE(imx_rproc_att_imx6sx),
>  	.method		= IMX_RPROC_MMIO,
>  	.ops		= &imx_rproc_ops_mmio,
> +	.flags		= IMX_RPROC_NEED_CLKS,
>  };
>
>  static const struct imx_rproc_dcfg imx_rproc_cfg_imx93 = {
> @@ -1259,6 +1245,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx93 = {
>  	.att_size	= ARRAY_SIZE(imx_rproc_att_imx93),
>  	.method		= IMX_RPROC_SMC,
>  	.ops		= &imx_rproc_ops_arm_smc,
> +	.flags		= IMX_RPROC_NEED_CLKS,
>  };
>
>  static const struct of_device_id imx_rproc_of_match[] = {
> diff --git a/drivers/remoteproc/imx_rproc.h b/drivers/remoteproc/imx_rproc.h
> index 3a9adaaf048b396102feeb45488cd2ff125a807a..a9cba623560c85ea37e47401c392c06dada500aa 100644
> --- a/drivers/remoteproc/imx_rproc.h
> +++ b/drivers/remoteproc/imx_rproc.h
> @@ -30,6 +30,7 @@ enum imx_rproc_method {
>
>  /* dcfg flags */
>  #define IMX_RPROC_NEED_SYSTEM_OFF	BIT(0)
> +#define IMX_RPROC_NEED_CLKS		BIT(1)
>
>  struct imx_rproc_plat_ops {
>  	int (*start)(struct rproc *rproc);
>
> --
> 2.37.1
>

Re: [PATCH 1/5] remoteproc: imx_rproc: Simplify clock enable logic using dcfg flags

Posted by Daniel Baluta 1 week, 6 days ago

On Thu, Sep 18, 2025 at 2:51 PM Peng Fan <peng.fan@nxp.com> wrote:
>
> Simplify the clock enable logic by removing the dedicated
> imx_rproc_clk_enable() function and integrate the clock handling directly
> into the probe function to simplify the code.
>
> Add a new IMX_RPROC_NEED_CLKS flag in dcfg to indicate whether clock
> management is required for a given SoC. Update probe logic to conditionally
> enable clocks based on the new flag.
>

<snip>

> -               return dev_err_probe(dev, ret, "failed to enable clks\n");
> +       /* Remote core is under control of Linux or clock is not managed by firmware */

I see that you negate the comment from imx_rproc_clk_enable but with
the negation
OR becomes AND.

So, the comment should be:

/* Handle clocks when remote core is under control of Linux AND the
clocks are not managed by remote side  FW */

Also, do we really need this flag?
Shouldn't we just make a decision based on the fact that clk is in the
device tree or not?

> +       if (dcfg->flags & IMX_RPROC_NEED_CLKS) {
> +               priv->clk = devm_clk_get_enabled(dev, NULL);
> +               if (IS_ERR(priv->clk))
> +                       return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to enable clock\n");

[PATCH 1/5] remoteproc: imx_rproc: Simplify clock enable logic using dcfg flags
[PATCH 2/5] remoteproc: imx_rproc: Make detach operation platform-specific
[PATCH 3/5] remoteproc: imx_rproc: Enable PM runtime support unconditionally
[PATCH 4/5] remoteproc: imx_rproc: Remove the assignement to method
[PATCH 5/5] MAINTAINERS: Add an entry for i.MX remoteproc driver