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

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

Hi Frank,

On Tue, Oct 28, 2025 at 12:16:56PM -0400, Frank Li wrote:
>On Tue, Oct 28, 2025 at 04:18:03PM +0800, Peng Fan wrote:
>> i.MX95 features a Cortex-M33 core, six Cortex-A55 cores, and
>> one Cortex-M7 core. The System Control Management Interface(SCMI)
>> firmware runs on the M33 core. The i.MX95 SCMI firmware named System
>> Manager(SM) includes vendor extension protocols, Logical Machine
>> Management(LMM) protocol and CPU protocol and etc.
>>
...
>> +	depends on IMX_SCMI_CPU_DRV || !IMX_SCMI_CPU_DRV
>> +	depends on IMX_SCMI_LMM_DRV || !IMX_SCMI_LMM_DRV
>
>what's means IMX_SCMI_LMM_DRV || !IMX_SCMI_LMM_DRV here, which is the same
>config

This is to make sure when IMX_SCMI_CPU_DRV or IMX_SCMI_LMM_DRV is module built,
imx_rproc should also be built as module.

>
>>  	select MAILBOX
>>  	help
...
>> +	 */
>> +	if (priv->flags & IMX_RPROC_FLAGS_SM_CPU_OP)
>> +		return 0;
>
>You already have imx_rproc_sm_lmm_stat[stop](),
>why not use imx_rproc_sm_lmm_prepare();
>
>imx_rproc_sm_cpu_prepare() should be empty. So needn't check
>IMX_RPROC_FLAGS_SM_CPU_OP.

I will add below in detect_mode(), and no need imx_rproc_sm_cpu_prepare.

priv->ops.prepare = &imx_rproc_sm_lmm_prepare;

>
>> +
>> +	/*
>>  static int imx_rproc_prepare(struct rproc *rproc)
>>  {
>>  	struct imx_rproc *priv = rproc->priv;
>> @@ -532,7 +639,10 @@ static int imx_rproc_prepare(struct rproc *rproc)
>>  		rproc_add_carveout(rproc, mem);
>>  	}
>>
>> -	return  0;
>> +	if (priv->ops.detect_mode == imx_rproc_sm_detect_mode)
>> +		return imx_rproc_sm_prepare(rproc);

Here I will change to:
if (priv->ops.prepare)
    return priv->ops.prepare();

>> +
>> +	return 0;
>>  }
>>
>>  static int imx_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
>> @@ -991,6 +1101,83 @@ static int imx_rproc_scu_api_detect_mode(struct rproc *rproc)
>>  	return 0;
>>  }
>>
>> +	 */
>> +	if (dcfg->lmid == info.lmid) {
>> +		priv->ops.start = &imx_rproc_sm_cpu_start;
>> +		priv->ops.stop = &imx_rproc_sm_cpu_stop;
>
>you have switch callback here, also swtich imx_rproc_sm_prepare()

Yes, as above.
priv->ops.prepare = &imx_rproc_sm_lmm_prepare;

>
>> +		priv->flags |= IMX_RPROC_FLAGS_SM_CPU_OP;
>> +		dev_info(dev, "Using CPU Protocol OPS\n");
>> +
>> +		return 0;
>> +	}
>> +
>> +	dev_info(dev, "Using LMM Protocol OPS\n");
>> +	priv->ops.start = &imx_rproc_sm_lmm_start;
>> +	priv->ops.stop = &imx_rproc_sm_lmm_stop;
>> +	priv->flags |= IMX_RPROC_FLAGS_SM_LMM_OP;
>
>Most likely
>
>	bool b = (dcfg->lmid == info.lmid);
>	priv->ops.start = b ? &imx_rproc_sm_cpu_start : &imx_rproc_sm_lmm_start;
>	...
>or
>	if (b)	{
>	} else {
>	}
>
>	to do ops switch.

ok.

>
>> +
>> +	/*
>> +	 * Use power on to do permission check. If rproc is in different Logical Machine,
>> +	 * and linux has permission to handle the Logical Machine, set
>> +	 * IMX_RPROC_FLAGS_SM_LMM_AVAIL.
>> +	 */
>> +	ret = scmi_imx_lmm_operation(dcfg->lmid, SCMI_IMX_LMM_POWER_ON, 0);
>> +	if (ret != 0) {
>> +		if (ret == -EACCES) {
>> +			/* Not under Linux Control, so only do IPC between rproc and Linux */
>> +			dev_info(priv->dev, "lmm(%d) not under Linux Control\n", dcfg->lmid);
>> +			return 0;
>> +		}
>> +
>> +		dev_info(priv->dev, "power on lmm(%d) failed: %d\n", dcfg->lmid, ret);
>> +		return ret;
>> +	}
>> +
>> +	priv->flags |= IMX_RPROC_FLAGS_SM_LMM_AVAIL;
>> +
>> +	/* rproc is started before boot Linux, so do NOT shutdown the LM */
>> +	if (started)
>> +		return 0;
>> +
>> +	/* Permission check finished, shutdown the LM to save power */
>> +	ret = scmi_imx_lmm_operation(dcfg->lmid, SCMI_IMX_LMM_SHUTDOWN, 0);
>> +	if (ret) {
>> +		dev_err(priv->dev, "shutdown lmm(%d) failed: %d\n", dcfg->lmid, ret);
>> +		return ret;
>> +	}
>
>look like this block should be in imx_rproc_sm_prepare()?

This should be kept here. permission check flow:

power on m7 lm
  |
  | success(indicating linux control rproc LM)
         -> check m7 booted by U-Boot
	              |-> If yes, keep power.
		      |-> If no, shutdown to save power.
                                                     
Thanks,
Peng

>
>Frank
>> +
>> +	return 0;
>> +}
>> +
>>  static int imx_rproc_detect_mode(struct imx_rproc *priv)
>>  {
>>  	/*
>> @@ -1166,6 +1353,10 @@ static const struct imx_rproc_plat_ops imx_rproc_ops_scu_api = {
>>  	.detect_mode	= imx_rproc_scu_api_detect_mode,
>>  };
>>
>> +static const struct imx_rproc_plat_ops imx_rproc_ops_sm = {
>> +	.detect_mode	= imx_rproc_sm_detect_mode,
>> +};
>> +
>>  static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mn_mmio = {
>>  	.src_reg	= IMX7D_SRC_SCR,
>>  	.src_mask	= IMX7D_M4_RST_MASK,
>> diff --git a/drivers/remoteproc/imx_rproc.h b/drivers/remoteproc/imx_rproc.h
>> index aeed08bdfb5619c7afd7201589f417cfd6745818..b8a30df92d3bdeb915e33551235d555b947d0b16 100644
>> --- a/drivers/remoteproc/imx_rproc.h
>> +++ b/drivers/remoteproc/imx_rproc.h
>> @@ -51,6 +51,9 @@ struct imx_rproc_dcfg {
>>  	enum imx_rproc_method		method;
>>  	u32				flags;
>>  	const struct imx_rproc_plat_ops	*ops;
>> +	/* For System Manager(SM) based SoCs, the IDs are from SM firmware */
>> +	u32				cpuid;
>> +	u32				lmid;
>>  };
>>
>>  #endif /* _IMX_RPROC_H */
>>
>> --
>> 2.37.1
>>