| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAEnQRZC8PTbzNM056WUSR-kYqdf4Sgkr88z3S87ZFk+rc=q3=Q@mail.gmail.com>
Date: Fri, 7 Nov 2025 11:10:15 +0200
From: Daniel Baluta <daniel.baluta@...il.com>
To: Peng Fan <peng.fan@....com>
Cc: 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 Peng,
Some comments inline to make this easier to understand:
On Fri, Oct 31, 2025 at 4:26 AM Peng Fan <peng.fan@....com> 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.
>
> There are three cases for M7:
Here we should specify how M7 gets into these three situations, is it
via the ATF configuration?
e.g
Depending on ATF 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?
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?
> 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.
> - For above case , after using SCMI_IMX_LMM_POWER_ON to check
> permission, SCMI_IMX_LMM_SHUTDOWN API should be called to shutdown
> the M7 LM to save power only when M7 LM is going to be started by
> remoteproc framework. Otherwise bypass SCMI_IMX_LMM_SHUTDOWN API if
> M7 LM is started before booting Linux.
>
> Current setup relies on pre-Linux software(U-Boot) to do M7 TCM ECC
> initialization. In future, we could add the support in Linux to decouple
> U-Boot and Linux.
>
> Signed-off-by: Peng Fan <peng.fan@....com>
> ---
> drivers/remoteproc/Kconfig | 2 +
> drivers/remoteproc/imx_rproc.c | 192 +++++++++++++++++++++++++++++++++++++++++
> drivers/remoteproc/imx_rproc.h | 3 +
> 3 files changed, 197 insertions(+)
>
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 48a0d3a69ed08057716f1e7ea950899f60bbe0cf..ee54436fea5ad08a9c198ce74d44ce7a9aa206de 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -27,6 +27,8 @@ config IMX_REMOTEPROC
> tristate "i.MX remoteproc support"
> depends on ARCH_MXC
> depends on HAVE_ARM_SMCCC
> + depends on IMX_SCMI_CPU_DRV || !IMX_SCMI_CPU_DRV
> + depends on IMX_SCMI_LMM_DRV || !IMX_SCMI_LMM_DRV
> select MAILBOX
> help
> Say y here to support iMX's remote processors via the remote
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 232eb91e0b5dc2432131b1c140d6688b073fea1d..1fb17701964ca4ee4b73d343b5ec1be8e2ee5fda 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -8,6 +8,7 @@
> #include <linux/clk.h>
> #include <linux/err.h>
> #include <linux/firmware/imx/sci.h>
> +#include <linux/firmware/imx/sm.h>
> #include <linux/interrupt.h>
> #include <linux/kernel.h>
> #include <linux/mailbox_client.h>
> @@ -22,6 +23,7 @@
> #include <linux/reboot.h>
> #include <linux/regmap.h>
> #include <linux/remoteproc.h>
> +#include <linux/scmi_imx_protocol.h>
> #include <linux/workqueue.h>
>
> #include "imx_rproc.h"
> @@ -92,8 +94,12 @@ struct imx_rproc_mem {
> #define ATT_CORE_MASK 0xffff
> #define ATT_CORE(I) BIT((I))
>
> +/* Linux has permission to handle the Logical Machine of remote cores */
> +#define IMX_RPROC_FLAGS_SM_LMM_AVAIL BIT(0)
> +
> 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_detect_mode(struct rproc *rproc);
>
> struct imx_rproc {
> struct device *dev;
> @@ -117,6 +123,8 @@ struct imx_rproc {
> u32 core_index;
> struct dev_pm_domain_list *pd_list;
> const struct imx_rproc_plat_ops *ops;
> + /* For i.MX System Manager based systems */
> + u32 flags;
<snip>
> static int imx_rproc_prepare(struct rproc *rproc)
> {
> struct imx_rproc *priv = rproc->priv;
> @@ -994,6 +1090,102 @@ static int imx_rproc_scu_api_detect_mode(struct rproc *rproc)
> return 0;
> }
>
> +static const struct imx_rproc_plat_ops imx_rproc_ops_sm = {
I think this should be called imx_rproc_ops_sm_lmm.
> + .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,
> +};
> +
> +static const struct imx_rproc_plat_ops imx_rproc_ops_sm_cpu = {
> + .detect_mode = imx_rproc_sm_detect_mode,
> + .start = imx_rproc_sm_cpu_start,
> + .stop = imx_rproc_sm_cpu_stop,
> +};
> +
> +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.
> + */
> + 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.
> + 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?
Maybe we need a better explanation here.
> + return 0;
> + }
> +
> + dev_info(dev, "power on lmm(%d) failed: %d\n", dcfg->lmid, ret);
> + return ret;
> + }
> +
> + priv->flags |= IMX_RPROC_FLAGS_SM_LMM_AVAIL;
> +
> + /* rproc was started before boot Linux and under control of Linux, directly return */
> + if (started)
> + 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?
> + 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? So we can still use this even if Host core
and remote core are in different LMMs?
> + 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)
> +
> + 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?
> + 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.
<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)?
Powered by blists - more mailing lists