Re: [PATCH v5 4/5] remoteproc: imx_rproc: Add support for System Manager API

Hi Mathieu,

On Wed, Jan 07, 2026 at 11:41:07AM -0700, Mathieu Poirier wrote:

On Thu, Dec 18, 2025 at 01:17:38PM +0800, Peng Fan (OSS) wrote:

quoted

From: Peng Fan <peng.fan@nxp.com>

...

quoted

+/* Linux has permission to handle the Logical Machine of remote cores */
+#define IMX_RPROC_FLAGS_SM_LMM_AVAIL	BIT(0)
+

This should be named IMX_RPROC_FLAGS_SM_LMM_CTRL.

Fix in V6.

quoted

 static int imx_rproc_xtr_mbox_init(struct rproc *rproc, bool tx_block);
 static void imx_rproc_free_mbox(void *data);
 

...

quoted

+static int imx_rproc_sm_lmm_start(struct rproc *rproc)
+{
+	struct imx_rproc *priv = rproc->priv;
+	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
+	struct device *dev = priv->dev;
+	int ret;
+

A comment is needed here to say that if the remoteproc core can't start the M7,
it will already be handled in imx_rproc_sm_lmm_prepare()

Add in V6:
        /*
         * If the remoteproc core can't start the LM, it will already be
         * handled in imx_rproc_sm_lmm_prepare().
         */

quoted

+	ret = scmi_imx_lmm_reset_vector_set(dcfg->lmid, dcfg->cpuid, 0, 0);
+	if (ret) {
+		dev_err(dev, "Failed to set reset vector lmid(%u), cpuid(%u): %d\n",
+			dcfg->lmid, dcfg->cpuid, ret);
+		return ret;
+	}
+
 

...

quoted

+static int imx_rproc_sm_lmm_prepare(struct rproc *rproc)
+{
+	struct imx_rproc *priv = rproc->priv;
+	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
+	int ret;
+

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

+	/*
+	 * IMX_RPROC_FLAGS_SM_LMM_AVAIL not set indicates Linux is not able
+	 * to start/stop rproc LM, then if rproc is not in detached state,
+	 * prepare should fail. If in detached state, this is in rproc_attach()
+	 * path.
+	 */
+	if (!(priv->flags & IMX_RPROC_FLAGS_SM_LMM_AVAIL))
+		return rproc->state == RPROC_DETACHED ? 0 : -EACCES;

<<<<<<<<<<<

       if (rproc->state == RPROC_DETACHED)
               return 0;

       if (!(priv->flags & IMX_RPROC_FLAGS_SM_LMM_AVAIL))
               return -EACCES;

<<<<<<<<<<<

Update in v6 with your code snippets.

quoted

+
+	/* Power on the Logical Machine to make sure TCM is available. */
+	ret = scmi_imx_lmm_operation(dcfg->lmid, SCMI_IMX_LMM_POWER_ON, 0);
+	if (ret) {
+		dev_err(priv->dev, "Failed to power on lmm(%d): %d\n", dcfg->lmid, ret);
+		return ret;
+	}
+
+	dev_info(priv->dev, "lmm(%d) powered on by Linux\n", dcfg->lmid);
+
+	return 0;
+}
+
 static int imx_rproc_prepare(struct rproc *rproc)
 {
 	struct imx_rproc *priv = rproc->priv;

@@ -980,6 +1078,93 @@ static int imx_rproc_scu_api_detect_mode(struct rproc *rproc)
 	return 0;
 }
 
+static int imx_rproc_sm_lmm_check(struct rproc *rproc, bool started)
+{
+	struct imx_rproc *priv = rproc->priv;
+	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
+	struct device *dev = priv->dev;
+	int ret;
+
+	/*
+	 * Use power on to do permission check. If rproc is in different LM,
+	 * and linux has permission to handle the LM, set IMX_RPROC_FLAGS_SM_LMM_AVAIL.
+	 */

Two things here:
(1) This whole comment describes what this function does rather than what the
code is doing in the next few lines.  As such it needs to be moved above the
function declaration.
(2) We know the M7 is in a different LM, otherwise "dcfg->lmid == info.lmid" in
imx_rproc_sm_detect_mode() and we would not be here.  As such the comment is
wrong.  The only thing this code is doing is check if the remoteproc core is
responsible for the M7 lifecycle.

Update in v6:
/* Check whether remoteproc core is responsible for LM lifecycle */
static int imx_rproc_sm_lmm_check(struct rproc *rproc, bool started)

quoted

+	ret = scmi_imx_lmm_operation(dcfg->lmid, SCMI_IMX_LMM_POWER_ON, 0);
+	if (ret) {

...

quoted

+
+

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

+	/* rproc was started before boot Linux and under control of Linux, directly return */
+	if (started) {
+		priv->flags |= IMX_RPROC_FLAGS_SM_LMM_AVAIL;
+		return 0;
+	}
+
+	/* else shutdown the LM to save power */
+	ret = scmi_imx_lmm_operation(dcfg->lmid, SCMI_IMX_LMM_SHUTDOWN, 0);
+	if (ret) {
+		dev_err(dev, "shutdown lmm(%d) failed: %d\n", dcfg->lmid, ret);
+		return ret;
+	}
+
+	priv->flags |= IMX_RPROC_FLAGS_SM_LMM_AVAIL;

<<<<<<<<<<<

       /* Shutdown remote processor if not started */
       if (!started) {
        ret = scmi_imx_lmm_operation(dcfg->lmid, SCMI_IMX_LMM_SHUTDOWN, 0);
        if (ret) {
	        dev_err(dev, "shutdown lmm(%d) failed: %d\n", dcfg->lmid, ret);
	        return ret;
        }
       }

priv->flags |= IMX_RPROC_FLAGS_SM_LMM_AVAIL;

<<<<<<<<<<<

Thanks for your code snippets. Update in v6.

This patchset would be a lot easier to digest if you had split the support for
CPU and LMM protocols in diffent patches.

I appreciate your detailed review and the code snippets you provided. Please
let me know if you'd prefer that I split the support for LMM
and CPU into separate patches in v6, or keep them combined as they are.

Thanks,
Peng.