Message-ID: <aQ35BvxIwMetA5fm@shlinux89>
Date: Fri, 7 Nov 2025 21:49:58 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Daniel Baluta <daniel.baluta@...il.com>
Cc: Peng Fan <peng.fan@....com>, Bjorn Andersson <andersson@...nel.org>,
	Mathieu Poirier <mathieu.poirier@...aro.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	Daniel Baluta <daniel.baluta@....com>, Frank Li <frank.li@....com>,
	linux-remoteproc@...r.kernel.org, devicetree@...r.kernel.org,
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/5] remoteproc: imx_rproc: Add support for System
 Manager API

Hi Daniel,

On Fri, Nov 07, 2025 at 11:10:15AM +0200, Daniel Baluta wrote:
>> There are three cases for M7:
>
>Here we should specify how M7 gets into these three situations, is it
>via the ATF configuration?

It is not ATF configuration. This is configured in System Manager.

>
>e.g
>
>Depending on ATF configuration, M7 can be used as follows:

I will use:
"Depending on System Manager configuration, M7 can be used as follows:"

>
>>  (1) M7 in a separate Logical Machine(LM) that Linux can't control it.
>
>Here we should make it clear from who is M7 separate.
>
>e.g
>
>(1) M7 in a separate Logical Machine (LM) from A55 cores, that Linux
>can't control
>
>>  (2) M7 in a separate Logical Machine that Linux can control it using
>>      LMM protocol
>
>(2) M7 in a separate LM from A55 cores that Linux can control using
>LMM protocol.
>
>>  (3) M7 runs in same Logical Machine as A55, so Linux can control it
>>      using CPU protocol
>>
>
>
>> So extend the driver to using LMM and CPU protocol to manage the M7 core.
>
>
>>  - Compare linux LM ID(got using scmi_imx_lmm_info) and M7 LM ID(the ID
>>    is fixed as 1 in SM firmware if M7 is in a seprate LM),
>
>s/seprate/separate
>
>One question here: Is it OK to call scmi_imx_lmm_info no matter the
>context are we in?

Yes. With LMM_ID_DISCOVER as parameter, this API will not fail in permission
check. It will always return current LM ID.

>
>If this call fails is it safe to assume that we are in the case (1)
>describe above? E.g Linux
>cannot reach the M7 core through LMM protocol  or CPU protocol?

Sadly no. See above.

>
>
>>
...
>> +static const struct imx_rproc_plat_ops imx_rproc_ops_sm = {
>
>I think this should be called imx_rproc_ops_sm_lmm.

ok.

>
>> +       .detect_mode    = imx_rproc_sm_detect_mode,
>> +       .prepare        = imx_rproc_sm_lmm_prepare,
>> +       .start          = imx_rproc_sm_lmm_start,
>> +       .stop           = imx_rproc_sm_lmm_stop,
>> +};
>> +
...
>> +       /*
>> +        * 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.
>> +        */
>> +       ret = scmi_imx_lmm_operation(dcfg->lmid, SCMI_IMX_LMM_POWER_ON, 0);
>
>I wonder if there is a better call to check if Linux has permissions
>to handle to other LMM. This is a bit
>strange but if no other option we can go witi it.

Per doc drivers/firmware/arm_scmi/vendors/imx95.rst, this is the only one
that could be used for runtime detection.

>> +       if (ret != 0) {
>> +               if (ret == -EACCES) {
>> +                       /* Not under Linux Control, so only do IPC between rproc and Linux */
>> +                       dev_info(dev, "lmm(%d) not under Linux Control\n", dcfg->lmid);
>
>Would this be an error? So if Linux cannot interact with the other LMM
>via LMM API how IPC is possible?
>via CPU protocol?

When M7 is in a different LM, this means it is booted by ROM/SM, so
there is only IPC between M7 and Linux.

>
>Maybe we need a better explanation here.

"RPROC LM is booted before Linux and not under Linux Control, so only
do IPC between RPROC and Linux, not return failure"

>
>
>> +                       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);
>
>do we care to restore the flags field here on case of error?

keep or clear, both should be fine. But restore the flags should be better
code practice. I will add that.

>
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int imx_rproc_sm_detect_mode(struct rproc *rproc)
>> +{
>
><snip>
>
>> +
>> +       ret = scmi_imx_cpu_started(dcfg->cpuid, &started);
>
>Is this CPU protocol call?

Yes.

>So we can still use this even if Host core
>and remote core are in different LMMs?

Yes. SM supports this.

>
>> +       if (ret) {
>> +               dev_err(dev, "Failed to detect cpu(%d) status: %d\n", dcfg->cpuid, ret);
>> +               return ret;
>> +       }
>> +
>> +       if (started)
>> +               priv->rproc->state = RPROC_DETACHED;
>> +
>> +       /* Get current Linux Logical Machine ID */
>> +       ret = scmi_imx_lmm_info(LMM_ID_DISCOVER, &info);
>> +       if (ret) {
>> +               dev_err(dev, "Failed to get current LMM ID err: %d\n", ret);
>> +               return ret;
>> +       }
>> +
>> +       /*
>> +        * Check whether remote processor is in same Logical Machine as Linux.
>
>Is in same -> is in the same. We need to always try to be consistent.
>
>Remote processor is a hardware part while Linux is a software part.
>
>So always use the same object types: e.g /*check whether remote
>processor is in the same LM as host core (running Linux) */
>
>> +        * If yes, use CPU protocol API to manage remote processor.
>> +        * If no, use Logical Machine API to manage remote processor.
>> +        */
>> +       is_cpu_ops = dcfg->lmid == info.lmid;
>
>No need for is_cpu_ops.
>
>Just go if(dcfg->lmid == info.lmid)

ok.

>
>
>> +
>> +       if (is_cpu_ops) {
>> +               priv->ops = &imx_rproc_ops_sm_cpu;
>> +               dev_info(dev, "Using CPU Protocol OPS\n");
>
>I'm not sure we want to go with dev_info here. It it pollute the log
>and at least confuse people.
>But if you feel a strong need for this you can keep it.
>
>Also, shouldn't be here an else case where priv->ops gets set to LMM ops?

Frank has same comment :)
This LMM ops is assigned to dcfg->ops, see patch 5.

But since both of you asked, adding else seems make it clear.

>
>> +               return 0;
>> +       }
>> +
>> +       dev_info(dev, "Using LMM Protocol OPS\n");
>> +
>> +       return imx_rproc_sm_lmm_check(rproc, started);
>
>If this check fails is the info message above still valid? It will
>confuse people.

I prefer to keep the message above, imx_rproc_sm_lmm_check() will
print out error message if there are any failures.

>
><snip>
>
>> @@ -52,6 +52,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 */
>
>Keep here:
>
>/* For System Manager(SM) based SoCs */
>
>Then comment for each of the fields:
>> +       u32                             cpuid; /* TODO.... CPU Id of the remote core? */
>
>> +       u32                             lmid;
>
>But how these fields are set? Are they the cpuid and lmid of the
>remote core or local core (a55)?

Both are for remote core. These fields are hard-coded in System Manager.

Thanks
Peng

Powered by blists - more mailing lists