[PATCH 0/3] remoteproc: imx_rproc: allow attaching to running core kicked by the bootloader

[PATCH 0/3] remoteproc: imx_rproc: allow attaching to running core kicked by the bootloader

[Hiago De Franco]

From: Hiago De Franco <hiago.franco@toradex.com>

For the i.MX8X and i.MX8 family SoCs, currently when the remotecore is
started by the bootloader and the M core and A core are in the same
partition, the driver is not capable to detect the remote core and
report the correct state of it.

This series of patches implement an API call to the SCU which will
return the power mode of a given resource (M core in this case) and if
it is already powered on, the driver will attach to it. This SCU API was
already being used inside pmdomain/imx/scu-pd.c driver, therefore it has
been moved to firmware/imx/misc.c so it can be accessed by imx_rproc
driver.

Finally, the imx_rproc_clk_enable() function was also changed to make it
return before dev_clk_get() is called, as it currently generates an SCU
fault reset if the remote core is already running and the kernel tries
to enable the clock again. These changes are a follow up from a v1 sent
to imx_rproc [1] and from a reported regression [2].

[1] https://lore.kernel.org/lkml/20250423155131.101473-1-hiagofranco@gmail.com/
[2] https://lore.kernel.org/lkml/20250404141713.ac2ntcsjsf7epdfa@hiago-nb/

Hiago De Franco (3):
  remoteproc: imx_rproc: skip clock enable when M-core is managed by the
    SCU
  firmware: imx: move get power mode function from scu-pd.c to misc.c
  remoteproc: imx_rproc: add power mode check for remote core attachment

 drivers/firmware/imx/misc.c           | 47 +++++++++++++++++++++++++++
 drivers/pmdomain/imx/scu-pd.c         | 29 ++++-------------
 drivers/remoteproc/imx_rproc.c        | 27 +++++++++++++--
 include/linux/firmware/imx/svc/misc.h |  8 +++++
 4 files changed, 87 insertions(+), 24 deletions(-)

-- 
2.39.5

[PATCH 1/3] remoteproc: imx_rproc: skip clock enable when M-core is managed by the SCU

[Hiago De Franco]

From: Hiago De Franco <hiago.franco@toradex.com>

For the i.MX8X and i.MX8 family SoCs, when the M-core is powered up
before Linux starts (e.g., by the bootloader) and it is being managed by
the SCU, the SCFW will not allow the kernel to enable the clock again.
This currently causes an SCU fault reset when the M-core is up and
running and the kernel boots, resetting the system.

Therefore, add a check in the clock enable function to not execute it if
the M-core is being managed by the SCU.

This change affects only the i.MX8X and i.MX8 family SoCs, as this is
under the IMX_RPROC_SCU_API method.

Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
Suggested-by: Peng Fan <peng.fan@oss.nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 74299af1d7f1..627e57a88db2 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -1029,8 +1029,8 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv)
 	struct device *dev = priv->dev;
 	int ret;
 
-	/* Remote core is not under control of Linux */
-	if (dcfg->method == IMX_RPROC_NONE)
+	/* 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;
 
 	priv->clk = devm_clk_get(dev, NULL);
-- 
2.39.5

[PATCH 2/3] firmware: imx: move get power mode function from scu-pd.c to misc.c

[Hiago De Franco]

From: Hiago De Franco <hiago.franco@toradex.com>

Move imx_sc_get_pd_power() from pmdomain/imx/scu-pd.c to
firmware/imx/misc.c and rename it to imx_sc_pm_get_resource_power_mode()
to maintain the same naming logic with other functions in misc.c.

This makes the API available for other use cases. For example,
remoteproc/imx_rproc.c can now use this function to check the power mode
of the remote core.

Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
---
 drivers/firmware/imx/misc.c           | 47 +++++++++++++++++++++++++++
 drivers/pmdomain/imx/scu-pd.c         | 29 ++++-------------
 include/linux/firmware/imx/svc/misc.h |  8 +++++
 3 files changed, 62 insertions(+), 22 deletions(-)

diff --git a/drivers/firmware/imx/misc.c b/drivers/firmware/imx/misc.c
index d073cb3ce699..61fcb0066ec9 100644
--- a/drivers/firmware/imx/misc.c
+++ b/drivers/firmware/imx/misc.c
@@ -37,6 +37,18 @@ struct imx_sc_msg_resp_misc_get_ctrl {
 	u32 val;
 } __packed __aligned(4);
 
+struct imx_sc_msg_req_get_resource_power_mode {
+	struct imx_sc_rpc_msg hdr;
+	union {
+		struct {
+			u16 resource;
+		} req;
+		struct {
+			u8 mode;
+		} resp;
+	} data;
+} __packed __aligned(4);
+
 /*
  * This function sets a miscellaneous control value.
  *
@@ -135,3 +147,38 @@ int imx_sc_pm_cpu_start(struct imx_sc_ipc *ipc, u32 resource,
 	return imx_scu_call_rpc(ipc, &msg, true);
 }
 EXPORT_SYMBOL(imx_sc_pm_cpu_start);
+
+/*
+ * This function gets the power mode from a given @resource
+ *
+ * @param[in]     ipc         IPC handle
+ * @param[in]     resource    resource to check the power mode
+ *
+ * @return Returns < 0 for errors or the following for success:
+ * IMX_SC_PM_PW_MODE_OFF  0	Power off
+ * IMX_SC_PM_PW_MODE_STBY 1	Power in standby
+ * IMX_SC_PM_PW_MODE_LP   2	Power in low-power
+ * IMX_SC_PM_PW_MODE_ON   3	Power on
+ *
+ * These are defined under firmware/imx/svc/pm.h
+ */
+int imx_sc_pm_get_resource_power_mode(struct imx_sc_ipc *ipc, u32 resource)
+{
+	struct imx_sc_msg_req_get_resource_power_mode msg;
+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
+	int ret;
+
+	hdr->ver = IMX_SC_RPC_VERSION;
+	hdr->svc = IMX_SC_RPC_SVC_PM;
+	hdr->func = IMX_SC_PM_FUNC_GET_RESOURCE_POWER_MODE;
+	hdr->size = 2;
+
+	msg.data.req.resource = resource;
+
+	ret = imx_scu_call_rpc(ipc, &msg, true);
+	if (ret)
+		return ret;
+
+	return msg.data.resp.mode;
+}
+EXPORT_SYMBOL(imx_sc_pm_get_resource_power_mode);
diff --git a/drivers/pmdomain/imx/scu-pd.c b/drivers/pmdomain/imx/scu-pd.c
index 01d465d88f60..945f972e636f 100644
--- a/drivers/pmdomain/imx/scu-pd.c
+++ b/drivers/pmdomain/imx/scu-pd.c
@@ -54,6 +54,7 @@
 #include <dt-bindings/firmware/imx/rsrc.h>
 #include <linux/console.h>
 #include <linux/firmware/imx/sci.h>
+#include <linux/firmware/imx/svc/misc.h>
 #include <linux/firmware/imx/svc/rm.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -328,27 +329,6 @@ static void imx_sc_pd_get_console_rsrc(void)
 	imx_con_rsrc = specs.args[0];
 }
 
-static int imx_sc_get_pd_power(struct device *dev, u32 rsrc)
-{
-	struct imx_sc_msg_req_get_resource_power_mode msg;
-	struct imx_sc_rpc_msg *hdr = &msg.hdr;
-	int ret;
-
-	hdr->ver = IMX_SC_RPC_VERSION;
-	hdr->svc = IMX_SC_RPC_SVC_PM;
-	hdr->func = IMX_SC_PM_FUNC_GET_RESOURCE_POWER_MODE;
-	hdr->size = 2;
-
-	msg.data.req.resource = rsrc;
-
-	ret = imx_scu_call_rpc(pm_ipc_handle, &msg, true);
-	if (ret)
-		dev_err(dev, "failed to get power resource %d mode, ret %d\n",
-			rsrc, ret);
-
-	return msg.data.resp.mode;
-}
-
 static int imx_sc_pd_power(struct generic_pm_domain *domain, bool power_on)
 {
 	struct imx_sc_msg_req_set_resource_power_mode msg;
@@ -438,7 +418,12 @@ imx_scu_add_pm_domain(struct device *dev, int idx,
 	if (imx_con_rsrc == sc_pd->rsrc)
 		sc_pd->pd.flags = GENPD_FLAG_RPM_ALWAYS_ON;
 
-	mode = imx_sc_get_pd_power(dev, pd_ranges->rsrc + idx);
+	mode = imx_sc_pm_get_resource_power_mode(pm_ipc_handle,
+						 pd_ranges->rsrc + idx);
+	if (mode < 0)
+		dev_err(dev, "failed to get power resource %d mode, ret %d\n",
+			pd_ranges->rsrc + idx, mode);
+
 	if (mode == IMX_SC_PM_PW_MODE_ON)
 		is_off = false;
 	else
diff --git a/include/linux/firmware/imx/svc/misc.h b/include/linux/firmware/imx/svc/misc.h
index 760db08a67fc..376c800a88d0 100644
--- a/include/linux/firmware/imx/svc/misc.h
+++ b/include/linux/firmware/imx/svc/misc.h
@@ -55,6 +55,8 @@ int imx_sc_misc_get_control(struct imx_sc_ipc *ipc, u32 resource,
 
 int imx_sc_pm_cpu_start(struct imx_sc_ipc *ipc, u32 resource,
 			bool enable, u64 phys_addr);
+
+int imx_sc_pm_get_resource_power_mode(struct imx_sc_ipc *ipc, u32 resource);
 #else
 static inline int imx_sc_misc_set_control(struct imx_sc_ipc *ipc,
 					  u32 resource, u8 ctrl, u32 val)
@@ -73,5 +75,11 @@ static inline int imx_sc_pm_cpu_start(struct imx_sc_ipc *ipc, u32 resource,
 {
 	return -ENOTSUPP;
 }
+
+static inline int imx_sc_pm_get_resource_power_mode(struct imx_sc_ipc *ipc,
+						    u32 resource)
+{
+	return -ENOTSUPP;
+}
 #endif
 #endif /* _SC_MISC_API_H */
-- 
2.39.5

[PATCH 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment

[Hiago De Franco]

From: Hiago De Franco <hiago.franco@toradex.com>

When the remote core is started before Linux boots (e.g., by the
bootloader), the driver currently is not able to attach because it only
checks for cores running in different partitions. If the core was kicked
by the bootloader, it is in the same partition as Linux and it is
already up and running.

This adds power mode verification through the SCU interface, enabling
the driver to detect when the remote core is already running and
properly attach to it.

Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
Suggested-by: Peng Fan <peng.fan@oss.nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 627e57a88db2..86541d9d8640 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -8,6 +8,7 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/firmware/imx/sci.h>
+#include <linux/firmware/imx/svc/misc.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/mailbox_client.h>
@@ -906,6 +907,21 @@ static int imx_rproc_attach_pd(struct imx_rproc *priv)
 	return ret < 0 ? ret : 0;
 }
 
+static bool imx_rproc_is_on(struct device *dev, struct imx_sc_ipc *ipc,
+			    u32 resource)
+{
+	int ret;
+
+	ret = imx_sc_pm_get_resource_power_mode(ipc, resource);
+	if (ret < 0) {
+		dev_err(dev, "failed to get power resource %d mode, ret %d\n",
+			resource, ret);
+		return false;
+	}
+
+	return ret == IMX_SC_PM_PW_MODE_ON;
+}
+
 static int imx_rproc_detect_mode(struct imx_rproc *priv)
 {
 	struct regmap_config config = { .name = "imx-rproc" };
@@ -949,6 +965,13 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
 			if (of_property_read_u32(dev->of_node, "fsl,entry-address", &priv->entry))
 				return -EINVAL;
 
+			/*
+			 * If remote core is already running (e.g. kicked by
+			 * the bootloader), attach to it.
+			 */
+			if (imx_rproc_is_on(dev, priv->ipc_handle, priv->rsrc_id))
+				priv->rproc->state = RPROC_DETACHED;
+
 			return imx_rproc_attach_pd(priv);
 		}
 
-- 
2.39.5

Re: [PATCH 1/3] remoteproc: imx_rproc: skip clock enable when M-core is managed by the SCU

[Peng Fan]

On Mon, May 05, 2025 at 12:48:47PM -0300, Hiago De Franco wrote:
>From: Hiago De Franco <hiago.franco@toradex.com>
>
>For the i.MX8X and i.MX8 family SoCs, when the M-core is powered up
>before Linux starts (e.g., by the bootloader) and it is being managed by
>the SCU, the SCFW will not allow the kernel to enable the clock again.
>This currently causes an SCU fault reset when the M-core is up and
>running and the kernel boots, resetting the system.
>
>Therefore, add a check in the clock enable function to not execute it if
>the M-core is being managed by the SCU.
>
>This change affects only the i.MX8X and i.MX8 family SoCs, as this is
>under the IMX_RPROC_SCU_API method.

I would rewrite as below: "

For the i.MX8X and i.MX8 family SoCs, when the M-core is powered up
by the bootloader, M-core and Linux are in same SCFW(System Controller
Firmware) partition, so linux has permission to control M-core.

But when M-core is started, the SCFW will automatically enable the clock
and configure the rate, and any users that wanna to enable the clock
will get error 'LOCKED' from SCFW. So current imx_rproc.c probe function
gets failure because clk_prepare_enable returns failure. Then
the power domain of M-core is powered off when M-core is still running,
SCU(System Controller Unit) will get a fault reset, and system restarts.

To address the issue, ignore handling the clk for i.MX8X and i.MX8 M-core,
because SCFW automatically enables and configures the clock.
"

You may update if you wanna.

>
>Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
>Suggested-by: Peng Fan <peng.fan@oss.nxp.com>

-> peng.fan@nxp.com

Thanks,
Peng

>---
> drivers/remoteproc/imx_rproc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
>index 74299af1d7f1..627e57a88db2 100644
>--- a/drivers/remoteproc/imx_rproc.c
>+++ b/drivers/remoteproc/imx_rproc.c
>@@ -1029,8 +1029,8 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv)
> 	struct device *dev = priv->dev;
> 	int ret;
> 
>-	/* Remote core is not under control of Linux */
>-	if (dcfg->method == IMX_RPROC_NONE)
>+	/* 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;
> 
> 	priv->clk = devm_clk_get(dev, NULL);
>-- 
>2.39.5
>

Re: [PATCH 2/3] firmware: imx: move get power mode function from scu-pd.c to misc.c

[Peng Fan]

On Mon, May 05, 2025 at 12:48:48PM -0300, Hiago De Franco wrote:
>From: Hiago De Franco <hiago.franco@toradex.com>
>
>Move imx_sc_get_pd_power() from pmdomain/imx/scu-pd.c to
>firmware/imx/misc.c and rename it to imx_sc_pm_get_resource_power_mode()
>to maintain the same naming logic with other functions in misc.c.
>
>This makes the API available for other use cases. For example,
>remoteproc/imx_rproc.c can now use this function to check the power mode
>of the remote core.

Better put this patch at the first I think.

To be simple, I think just export
imx_sc_get_pd_power in drivers/pmdomain/imx/scu-pd.c.
And add the function declaration in include/linux/firmware/imx/sci.h.

Not sure Ulf or Shawn is good with it.

Regards,
Peng

>
>Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
>---
> drivers/firmware/imx/misc.c           | 47 +++++++++++++++++++++++++++
> drivers/pmdomain/imx/scu-pd.c         | 29 ++++-------------
> include/linux/firmware/imx/svc/misc.h |  8 +++++
> 3 files changed, 62 insertions(+), 22 deletions(-)
>
>diff --git a/drivers/firmware/imx/misc.c b/drivers/firmware/imx/misc.c
>index d073cb3ce699..61fcb0066ec9 100644
>--- a/drivers/firmware/imx/misc.c
>+++ b/drivers/firmware/imx/misc.c
>@@ -37,6 +37,18 @@ struct imx_sc_msg_resp_misc_get_ctrl {
> 	u32 val;
> } __packed __aligned(4);
> 
>+struct imx_sc_msg_req_get_resource_power_mode {
>+	struct imx_sc_rpc_msg hdr;
>+	union {
>+		struct {
>+			u16 resource;
>+		} req;
>+		struct {
>+			u8 mode;
>+		} resp;
>+	} data;
>+} __packed __aligned(4);
>+
> /*
>  * This function sets a miscellaneous control value.
>  *
>@@ -135,3 +147,38 @@ int imx_sc_pm_cpu_start(struct imx_sc_ipc *ipc, u32 resource,
> 	return imx_scu_call_rpc(ipc, &msg, true);
> }
> EXPORT_SYMBOL(imx_sc_pm_cpu_start);
>+
>+/*
>+ * This function gets the power mode from a given @resource
>+ *
>+ * @param[in]     ipc         IPC handle
>+ * @param[in]     resource    resource to check the power mode
>+ *
>+ * @return Returns < 0 for errors or the following for success:
>+ * IMX_SC_PM_PW_MODE_OFF  0	Power off
>+ * IMX_SC_PM_PW_MODE_STBY 1	Power in standby
>+ * IMX_SC_PM_PW_MODE_LP   2	Power in low-power
>+ * IMX_SC_PM_PW_MODE_ON   3	Power on
>+ *
>+ * These are defined under firmware/imx/svc/pm.h
>+ */
>+int imx_sc_pm_get_resource_power_mode(struct imx_sc_ipc *ipc, u32 resource)
>+{
>+	struct imx_sc_msg_req_get_resource_power_mode msg;
>+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
>+	int ret;
>+
>+	hdr->ver = IMX_SC_RPC_VERSION;
>+	hdr->svc = IMX_SC_RPC_SVC_PM;
>+	hdr->func = IMX_SC_PM_FUNC_GET_RESOURCE_POWER_MODE;
>+	hdr->size = 2;
>+
>+	msg.data.req.resource = resource;
>+
>+	ret = imx_scu_call_rpc(ipc, &msg, true);
>+	if (ret)
>+		return ret;
>+
>+	return msg.data.resp.mode;
>+}
>+EXPORT_SYMBOL(imx_sc_pm_get_resource_power_mode);
>diff --git a/drivers/pmdomain/imx/scu-pd.c b/drivers/pmdomain/imx/scu-pd.c
>index 01d465d88f60..945f972e636f 100644
>--- a/drivers/pmdomain/imx/scu-pd.c
>+++ b/drivers/pmdomain/imx/scu-pd.c
>@@ -54,6 +54,7 @@
> #include <dt-bindings/firmware/imx/rsrc.h>
> #include <linux/console.h>
> #include <linux/firmware/imx/sci.h>
>+#include <linux/firmware/imx/svc/misc.h>
> #include <linux/firmware/imx/svc/rm.h>
> #include <linux/io.h>
> #include <linux/module.h>
>@@ -328,27 +329,6 @@ static void imx_sc_pd_get_console_rsrc(void)
> 	imx_con_rsrc = specs.args[0];
> }
> 
>-static int imx_sc_get_pd_power(struct device *dev, u32 rsrc)
>-{
>-	struct imx_sc_msg_req_get_resource_power_mode msg;
>-	struct imx_sc_rpc_msg *hdr = &msg.hdr;
>-	int ret;
>-
>-	hdr->ver = IMX_SC_RPC_VERSION;
>-	hdr->svc = IMX_SC_RPC_SVC_PM;
>-	hdr->func = IMX_SC_PM_FUNC_GET_RESOURCE_POWER_MODE;
>-	hdr->size = 2;
>-
>-	msg.data.req.resource = rsrc;
>-
>-	ret = imx_scu_call_rpc(pm_ipc_handle, &msg, true);
>-	if (ret)
>-		dev_err(dev, "failed to get power resource %d mode, ret %d\n",
>-			rsrc, ret);
>-
>-	return msg.data.resp.mode;
>-}
>-
> static int imx_sc_pd_power(struct generic_pm_domain *domain, bool power_on)
> {
> 	struct imx_sc_msg_req_set_resource_power_mode msg;
>@@ -438,7 +418,12 @@ imx_scu_add_pm_domain(struct device *dev, int idx,
> 	if (imx_con_rsrc == sc_pd->rsrc)
> 		sc_pd->pd.flags = GENPD_FLAG_RPM_ALWAYS_ON;
> 
>-	mode = imx_sc_get_pd_power(dev, pd_ranges->rsrc + idx);
>+	mode = imx_sc_pm_get_resource_power_mode(pm_ipc_handle,
>+						 pd_ranges->rsrc + idx);
>+	if (mode < 0)
>+		dev_err(dev, "failed to get power resource %d mode, ret %d\n",
>+			pd_ranges->rsrc + idx, mode);
>+
> 	if (mode == IMX_SC_PM_PW_MODE_ON)
> 		is_off = false;
> 	else
>diff --git a/include/linux/firmware/imx/svc/misc.h b/include/linux/firmware/imx/svc/misc.h
>index 760db08a67fc..376c800a88d0 100644
>--- a/include/linux/firmware/imx/svc/misc.h
>+++ b/include/linux/firmware/imx/svc/misc.h
>@@ -55,6 +55,8 @@ int imx_sc_misc_get_control(struct imx_sc_ipc *ipc, u32 resource,
> 
> int imx_sc_pm_cpu_start(struct imx_sc_ipc *ipc, u32 resource,
> 			bool enable, u64 phys_addr);
>+
>+int imx_sc_pm_get_resource_power_mode(struct imx_sc_ipc *ipc, u32 resource);
> #else
> static inline int imx_sc_misc_set_control(struct imx_sc_ipc *ipc,
> 					  u32 resource, u8 ctrl, u32 val)
>@@ -73,5 +75,11 @@ static inline int imx_sc_pm_cpu_start(struct imx_sc_ipc *ipc, u32 resource,
> {
> 	return -ENOTSUPP;
> }
>+
>+static inline int imx_sc_pm_get_resource_power_mode(struct imx_sc_ipc *ipc,
>+						    u32 resource)
>+{
>+	return -ENOTSUPP;
>+}
> #endif
> #endif /* _SC_MISC_API_H */
>-- 
>2.39.5
>

Re: [PATCH 3/3] remoteproc: imx_rproc: add power mode check for remote core attachment

[Peng Fan]

On Mon, May 05, 2025 at 12:48:49PM -0300, Hiago De Franco wrote:
>From: Hiago De Franco <hiago.franco@toradex.com>
>
>When the remote core is started before Linux boots (e.g., by the
>bootloader), the driver currently is not able to attach because it only
>checks for cores running in different partitions. If the core was kicked
>by the bootloader, it is in the same partition as Linux and it is
>already up and running.
>
>This adds power mode verification through the SCU interface, enabling
>the driver to detect when the remote core is already running and
>properly attach to it.
>
>Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
>Suggested-by: Peng Fan <peng.fan@oss.nxp.com>
>---
> drivers/remoteproc/imx_rproc.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
>diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
>index 627e57a88db2..86541d9d8640 100644
>--- a/drivers/remoteproc/imx_rproc.c
>+++ b/drivers/remoteproc/imx_rproc.c
>@@ -8,6 +8,7 @@
> #include <linux/clk.h>
> #include <linux/err.h>
> #include <linux/firmware/imx/sci.h>
>+#include <linux/firmware/imx/svc/misc.h>

Drop this line.

> #include <linux/interrupt.h>
> #include <linux/kernel.h>
> #include <linux/mailbox_client.h>
>@@ -906,6 +907,21 @@ static int imx_rproc_attach_pd(struct imx_rproc *priv)
> 	return ret < 0 ? ret : 0;
> }
> 
>+static bool imx_rproc_is_on(struct device *dev, struct imx_sc_ipc *ipc,
>+			    u32 resource)
>+{
>+	int ret;
>+
>+	ret = imx_sc_pm_get_resource_power_mode(ipc, resource);
>+	if (ret < 0) {
>+		dev_err(dev, "failed to get power resource %d mode, ret %d\n",
>+			resource, ret);
>+		return false;
>+	}
>+
>+	return ret == IMX_SC_PM_PW_MODE_ON;
>+}
>+
> static int imx_rproc_detect_mode(struct imx_rproc *priv)
> {
> 	struct regmap_config config = { .name = "imx-rproc" };
>@@ -949,6 +965,13 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
> 			if (of_property_read_u32(dev->of_node, "fsl,entry-address", &priv->entry))
> 				return -EINVAL;
> 
>+			/*
>+			 * If remote core is already running (e.g. kicked by
>+			 * the bootloader), attach to it.
>+			 */
>+			if (imx_rproc_is_on(dev, priv->ipc_handle, priv->rsrc_id))

I prefer "xyz == IMX_SC_PM_PW_MODE_ON"
But anyway, up to you. I not have strong opinion on this.

Regards,
Peng

>+				priv->rproc->state = RPROC_DETACHED;
>+
> 			return imx_rproc_attach_pd(priv);
> 		}
> 
>-- 
>2.39.5
>

Re: [PATCH 1/3] remoteproc: imx_rproc: skip clock enable when M-core is managed by the SCU

[Hiago De Franco]

Hi Peng,

On Tue, May 06, 2025 at 12:38:35PM +0800, Peng Fan wrote:
> On Mon, May 05, 2025 at 12:48:47PM -0300, Hiago De Franco wrote:
> >From: Hiago De Franco <hiago.franco@toradex.com>
> >
> >For the i.MX8X and i.MX8 family SoCs, when the M-core is powered up
> >before Linux starts (e.g., by the bootloader) and it is being managed by
> >the SCU, the SCFW will not allow the kernel to enable the clock again.
> >This currently causes an SCU fault reset when the M-core is up and
> >running and the kernel boots, resetting the system.
> >
> >Therefore, add a check in the clock enable function to not execute it if
> >the M-core is being managed by the SCU.
> >
> >This change affects only the i.MX8X and i.MX8 family SoCs, as this is
> >under the IMX_RPROC_SCU_API method.
> 
> I would rewrite as below: "
> 
> For the i.MX8X and i.MX8 family SoCs, when the M-core is powered up
> by the bootloader, M-core and Linux are in same SCFW(System Controller
> Firmware) partition, so linux has permission to control M-core.
> 
> But when M-core is started, the SCFW will automatically enable the clock
> and configure the rate, and any users that wanna to enable the clock
> will get error 'LOCKED' from SCFW. So current imx_rproc.c probe function
> gets failure because clk_prepare_enable returns failure. Then
> the power domain of M-core is powered off when M-core is still running,
> SCU(System Controller Unit) will get a fault reset, and system restarts.
> 
> To address the issue, ignore handling the clk for i.MX8X and i.MX8 M-core,
> because SCFW automatically enables and configures the clock.
> "
> 
> You may update if you wanna.
> 
> >
> >Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
> >Suggested-by: Peng Fan <peng.fan@oss.nxp.com>
> 
> -> peng.fan@nxp.com

Thanks for the review, I will update the suggestions on a v2. Meanwhile,
I will wait a little bit for other feedbacks.

> 
> Thanks,
> Peng
> 
> >---
> > drivers/remoteproc/imx_rproc.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> >index 74299af1d7f1..627e57a88db2 100644
> >--- a/drivers/remoteproc/imx_rproc.c
> >+++ b/drivers/remoteproc/imx_rproc.c
> >@@ -1029,8 +1029,8 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv)
> > 	struct device *dev = priv->dev;
> > 	int ret;
> > 
> >-	/* Remote core is not under control of Linux */
> >-	if (dcfg->method == IMX_RPROC_NONE)
> >+	/* 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;
> > 
> > 	priv->clk = devm_clk_get(dev, NULL);
> >-- 
> >2.39.5
> >

Cheers,
Hiago.

Re: [PATCH 1/3] remoteproc: imx_rproc: skip clock enable when M-core is managed by the SCU

[Mathieu Poirier]

On Tue, May 06, 2025 at 09:36:19AM -0300, Hiago De Franco wrote:
> Hi Peng,
> 
> On Tue, May 06, 2025 at 12:38:35PM +0800, Peng Fan wrote:
> > On Mon, May 05, 2025 at 12:48:47PM -0300, Hiago De Franco wrote:
> > >From: Hiago De Franco <hiago.franco@toradex.com>
> > >
> > >For the i.MX8X and i.MX8 family SoCs, when the M-core is powered up
> > >before Linux starts (e.g., by the bootloader) and it is being managed by
> > >the SCU, the SCFW will not allow the kernel to enable the clock again.
> > >This currently causes an SCU fault reset when the M-core is up and
> > >running and the kernel boots, resetting the system.
> > >
> > >Therefore, add a check in the clock enable function to not execute it if
> > >the M-core is being managed by the SCU.
> > >
> > >This change affects only the i.MX8X and i.MX8 family SoCs, as this is
> > >under the IMX_RPROC_SCU_API method.
> > 
> > I would rewrite as below: "
> > 
> > For the i.MX8X and i.MX8 family SoCs, when the M-core is powered up
> > by the bootloader, M-core and Linux are in same SCFW(System Controller
> > Firmware) partition, so linux has permission to control M-core.
> > 
> > But when M-core is started, the SCFW will automatically enable the clock
> > and configure the rate, and any users that wanna to enable the clock
> > will get error 'LOCKED' from SCFW. So current imx_rproc.c probe function
> > gets failure because clk_prepare_enable returns failure. Then
> > the power domain of M-core is powered off when M-core is still running,
> > SCU(System Controller Unit) will get a fault reset, and system restarts.
> > 
> > To address the issue, ignore handling the clk for i.MX8X and i.MX8 M-core,
> > because SCFW automatically enables and configures the clock.
> > "
> > 
> > You may update if you wanna.
> > 
> > >
> > >Signed-off-by: Hiago De Franco <hiago.franco@toradex.com>
> > >Suggested-by: Peng Fan <peng.fan@oss.nxp.com>
> > 
> > -> peng.fan@nxp.com
> 
> Thanks for the review, I will update the suggestions on a v2. Meanwhile,
> I will wait a little bit for other feedbacks.
>

I suggest you go ahead with a v2 - I have a fair amount of patches to review and
my time to do so is currently very limited.

> > 
> > Thanks,
> > Peng
> > 
> > >---
> > > drivers/remoteproc/imx_rproc.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > >diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> > >index 74299af1d7f1..627e57a88db2 100644
> > >--- a/drivers/remoteproc/imx_rproc.c
> > >+++ b/drivers/remoteproc/imx_rproc.c
> > >@@ -1029,8 +1029,8 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv)
> > > 	struct device *dev = priv->dev;
> > > 	int ret;
> > > 
> > >-	/* Remote core is not under control of Linux */
> > >-	if (dcfg->method == IMX_RPROC_NONE)
> > >+	/* 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;
> > > 
> > > 	priv->clk = devm_clk_get(dev, NULL);
> > >-- 
> > >2.39.5
> > >
> 
> Cheers,
> Hiago.

Re: [PATCH 2/3] firmware: imx: move get power mode function from scu-pd.c to misc.c

[Hiago De Franco]

Hi Peng,

On Tue, May 06, 2025 at 12:46:18PM +0800, Peng Fan wrote:
> On Mon, May 05, 2025 at 12:48:48PM -0300, Hiago De Franco wrote:
> >From: Hiago De Franco <hiago.franco@toradex.com>
> >
> >Move imx_sc_get_pd_power() from pmdomain/imx/scu-pd.c to
> >firmware/imx/misc.c and rename it to imx_sc_pm_get_resource_power_mode()
> >to maintain the same naming logic with other functions in misc.c.
> >
> >This makes the API available for other use cases. For example,
> >remoteproc/imx_rproc.c can now use this function to check the power mode
> >of the remote core.
>
> Better put this patch at the first I think.

Ok, I will do that.

>
> To be simple, I think just export
> imx_sc_get_pd_power in drivers/pmdomain/imx/scu-pd.c.
> And add the function declaration in include/linux/firmware/imx/sci.h.

I do not think this is correct, since it is specific to the scu-pd.c
driver and none of the functions there are exported. I believe the
correct implementation is moving to misc.c and export it there, just
like the other SCU API functions.

I will send a v2 today with the comments adressed, thanks.

>
> Not sure Ulf or Shawn is good with it.
>
> Regards,
> Peng
>

Cheers,
Hiago.