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

[Peng Fan <peng.fan@oss.nxp.com>]

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:
>> From: Peng Fan <peng.fan@nxp.com>
>> 
...
>> +/* 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.

>
>>  static int imx_rproc_xtr_mbox_init(struct rproc *rproc, bool tx_block);
>>  static void imx_rproc_free_mbox(void *data);
>>  
...
>> +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().
         */

>
>> +	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;
>> +	}
>> +
>>  
...
>> +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;
>> +
>
>>>>>>>>>>>>
>
>> +	/*
>> +	 * 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.

>> +
>> +	/* 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)

>
>> +	ret = scmi_imx_lmm_operation(dcfg->lmid, SCMI_IMX_LMM_POWER_ON, 0);
>> +	if (ret) {
...
>> +
>> +
>
>>>>>>>>>>>>
>
>> +	/* 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.