On Thu, Jan 08, 2026 at 06:29:36PM +0800, Peng Fan wrote:
> 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
The remoteproc core starts a remote processor, which in this case is the M7.
I'm not sure why you are referring to the logical machine (LM).
> * 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 */
Same comment as above.
> 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.
Please split.
>
> Thanks,
> Peng.
