On Wed, Jul 09, 2025 at 05:02:31PM +0800, Peng Fan wrote:
> Hi Mathieu,
>
> On Tue, Jul 08, 2025 at 10:32:34AM -0600, Mathieu Poirier wrote:
> >Good day,
>
> Thanks, good day.
>
> >
> >On Wed, Jun 25, 2025 at 10:23:28AM +0800, Peng Fan (OSS) wrote:
> >> From: Peng Fan <peng....@nxp.com>
> >>
> >> 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.
> >>
> >> There are three cases for M7:
> >> (1) M7 in a separate Logical Machine(LM) that Linux can't control it.
> >> (2) M7 in a separate Logical Machine that Linux can control it 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.
> >> - Add IMX_RPROC_SM to indicate the remote core runs on a SoC that
> >> has System Manager.
> >> - 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),
> >> if Linux LM ID equals M7 LM ID(linux and M7 in same LM), use CPU
> >> protocol to start/stop. Otherwise, use LMM protocol to start/stop.
> >> Whether using CPU or LMM protocol to start/stop, the M7 status
> >> detection could use CPU protocol to detect started or not. So
> >> in imx_rproc_detect_mode, use scmi_imx_cpu_started to check the
> >> status of M7.
> >> - For above case 1 and 2, Use SCMI_IMX_LMM_POWER_ON to detect whether
> >> the M7 LM is under control of A55 LM.
> >>
> >
> >Thanks for the context, it really helps.
>
> Glad that helps.
>
> >
>
> [....]
>
> >> @@ -592,6 +637,38 @@ static int imx_rproc_prepare(struct rproc *rproc)
> >> rproc_add_carveout(rproc, mem);
> >> }
> >>
> >> + switch (dcfg->method) {
> >> + case IMX_RPROC_SM:
> >> + if (!(priv->flags & IMX_RPROC_FLAGS_SM_LMM_OP))
> >> + break;
> >> + /*
> >> + * Power on the Logical Machine to make sure TCM is available.
> >> + * Also serve as permission check. If 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) {
> >> + dev_info(priv->dev, "lmm(%d) powered on\n", dcfg->lmid);
> >> + priv->flags |= IMX_RPROC_FLAGS_SM_LMM_AVAIL;
> >
> >This is set all the time imx_rproc_prepare() is called - isn't there a way to
> >set it once at initialisation time?
>
> Yeah. Moving this to probe path should be ok, such as in
> imx_rproc_detect_mode.
> The drawback is M7 logical machine will be left in powered up state if
> moving to probe path before user starts M7 LM. Leaving in here means M7 logcal
> machine will be only powered up when user does
> "echo start > /xx/remoteproc-y/state", but needs to set
> IMX_RPROC_FLAGS_SM_LMM_AVAIL flags each time do preparing.
>
> If you prefer moving this logic to probe, I could give a try to move
> to imx_rproc_detect_mode which is probe path.
>
> How do you think?
Just leave it where it is.
Mathieu
>
> >
> >> + } else if (ret == -EACCES) {
> >> + dev_info(priv->dev, "lmm(%d) not under Linux
> >> Control\n", dcfg->lmid);
> >> + /*
> >> + * If remote cores boots up in detached mode, continue;
> >> + * else linux has no permission, return -EACCES.
> >> + */
> >> + if (priv->rproc->state != RPROC_DETACHED)
> >> + return -EACCES;
> >> + } else if (ret) {
> >> + dev_err(priv->dev, "Failed to power on lmm(%d): %d\n",
> >> ret, dcfg->lmid);
> >> + return ret;
> >> + }
> >> +
> >> + break;
> >> + default:
> >> + break;
> >> + };
> >> +
> >
> >Please put this in a function and get rid of the switch{}.
>
> ok. Fix in v4
>
> >
> >> return 0;
> >> }
> >>
> >> @@ -911,13 +988,41 @@ static int imx_rproc_detect_mode(struct imx_rproc
> >> *priv)
> >> struct regmap_config config = { .name = "imx-rproc" };
> >> const struct imx_rproc_dcfg *dcfg = priv->dcfg;
> >> struct device *dev = priv->dev;
> >> + struct scmi_imx_lmm_info info;
> >> struct regmap *regmap;
> >> struct arm_smccc_res res;
> >> + bool started = false;
> >> int ret;
> >> u32 val;
> >> u8 pt;
> >>
> >> switch (dcfg->method) {
> >> + case IMX_RPROC_SM:
> >> + /* 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.
> >> + * If no, need use Logical Machine API to manage remote
> >> processor, and
> >> + * set IMX_RPROC_FLAGS_SM_LMM_OP.
> >> + * If yes, use CPU protocol API to manage remote processor.
> >> + */
> >> + if (dcfg->lmid != info.lmid) {
> >> + priv->flags |= IMX_RPROC_FLAGS_SM_LMM_OP;
> >> + dev_info(dev, "Using LMM Protocol OPS\n");
> >> + } else {
> >> + dev_info(dev, "Using CPU Protocol OPS\n");
> >> + }
> >> +
> >> + scmi_imx_cpu_started(dcfg->cpuid, &started);
> >
> >Function scmi_imx_cpu_started() returns a value that is not taken into
> >account.
> >
> >> + if (started)
>
> I will update to "if (started || ret)" in v4, with ret assigned the return
> value of scmi_imx_cpu_started.
>
> Thanks,
> Peng
