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 <[email protected]>
>>
...
>> +/* 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.
