| 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: <20250903063915.GA18615@nxa18884-linux.ap.freescale.net>
Date: Wed, 3 Sep 2025 14:39:15 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Mathieu Poirier <mathieu.poirier@...aro.org>
Cc: Peng Fan <peng.fan@....com>, Bjorn Andersson <andersson@...nel.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>, Frank Li <frank.li@....com>,
Daniel Baluta <daniel.baluta@....com>,
Iuliana Prodan <iuliana.prodan@....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 v5 2/3] remoteproc: imx_rproc: Add support for System
Manager API
On Wed, Sep 03, 2025 at 12:56:11PM +0800, Peng Fan wrote:
>On Tue, Sep 02, 2025 at 10:38:49AM -0600, Mathieu Poirier wrote:
>>On Sat, Aug 30, 2025 at 08:52:09PM +0800, Peng Fan wrote:
>>> On Fri, Aug 29, 2025 at 10:00:04AM -0600, Mathieu Poirier wrote:
>>> >Good day,
>>> >
>>> >On Thu, Aug 21, 2025 at 05:05:05PM +0800, Peng Fan 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:
>>> >> (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.
>>> >>
>>> >> 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.
>>> >>
>>> >> Reviewed-by: Daniel Baluta <daniel.baluta@....com>
>>> >> Reviewed-by: Frank Li <Frank.Li@....com>
>>> >> Signed-off-by: Peng Fan <peng.fan@....com>
>>> >> ---
>>> >> drivers/remoteproc/Kconfig | 2 +
>>> >> drivers/remoteproc/imx_rproc.c | 123 ++++++++++++++++++++++++++++++++++++++++-
>>> >> drivers/remoteproc/imx_rproc.h | 5 ++
>>> >> 3 files changed, 127 insertions(+), 3 deletions(-)
>>> >>
>>> >> 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 a6eef0080ca9e46efe60dcb3878b9efdbdc0f08e..151b9ca34bac2dac9df0ed873f493791f2d1466e 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,6 +94,11 @@ struct imx_rproc_mem {
>>> >> #define ATT_CORE_MASK 0xffff
>>> >> #define ATT_CORE(I) BIT((I))
>>> >>
>>> >> +/* Logical Machine Operation */
>>> >> +#define IMX_RPROC_FLAGS_SM_LMM_OP BIT(0)
>>> >> +/* Linux has permission to handle the Logical Machine of remote cores */
>>> >> +#define IMX_RPROC_FLAGS_SM_LMM_AVAIL BIT(1)
>>> >> +
>>> >> static int imx_rproc_xtr_mbox_init(struct rproc *rproc, bool tx_block);
>>> >> static void imx_rproc_free_mbox(struct rproc *rproc);
>>> >>
>>> >> @@ -116,6 +123,8 @@ struct imx_rproc {
>>> >> u32 entry; /* cpu start address */
>>> >> u32 core_index;
>>> >> struct dev_pm_domain_list *pd_list;
>>> >> + /* For i.MX System Manager based systems */
>>> >> + u32 flags;
>>> >> };
>>> >>
>>> >> static const struct imx_rproc_att imx_rproc_att_imx93[] = {
>>> >> @@ -394,6 +403,30 @@ static int imx_rproc_start(struct rproc *rproc)
>>> >> case IMX_RPROC_SCU_API:
>>> >> ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc_id, true, priv->entry);
>>> >> break;
>>> >> + case IMX_RPROC_SM:
>>> >> + if (priv->flags & IMX_RPROC_FLAGS_SM_LMM_OP) {
>>> >> + if (!(priv->flags & IMX_RPROC_FLAGS_SM_LMM_AVAIL))
>>> >> + return -EACCES;
>>> >> +
>>> >> + 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);
>>> >> + }
>>> >> +
>>> >> + ret = scmi_imx_lmm_operation(dcfg->lmid, SCMI_IMX_LMM_BOOT, 0);
>>> >> + if (ret)
>>> >> + dev_err(dev, "Failed to boot lmm(%d): %d\n", ret, dcfg->lmid);
>>> >> + } else {
>>> >> + ret = scmi_imx_cpu_reset_vector_set(dcfg->cpuid, 0, true, false, false);
>>> >> + if (ret) {
>>> >> + dev_err(dev, "Failed to set reset vector cpuid(%u): %d\n",
>>> >> + dcfg->cpuid, ret);
>>> >> + }
>>> >> +
>>> >> + ret = scmi_imx_cpu_start(dcfg->cpuid, true);
>>> >> + }
>>> >> + break;
>>> >> default:
>>> >> return -EOPNOTSUPP;
>>> >> }
>>> >> @@ -436,6 +469,16 @@ static int imx_rproc_stop(struct rproc *rproc)
>>> >> case IMX_RPROC_SCU_API:
>>> >> ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc_id, false, priv->entry);
>>> >> break;
>>> >> + case IMX_RPROC_SM:
>>> >> + if (priv->flags & IMX_RPROC_FLAGS_SM_LMM_OP) {
>>> >> + if (priv->flags & IMX_RPROC_FLAGS_SM_LMM_AVAIL)
>>> >> + ret = scmi_imx_lmm_operation(dcfg->lmid, SCMI_IMX_LMM_SHUTDOWN, 0);
>>> >> + else
>>> >> + ret = -EACCES;
>>> >> + } else {
>>> >> + ret = scmi_imx_cpu_start(dcfg->cpuid, false);
>>> >> + }
>>> >> + break;
>>> >> default:
>>> >> return -EOPNOTSUPP;
>>> >> }
>>> >> @@ -546,10 +589,48 @@ static int imx_rproc_mem_release(struct rproc *rproc,
>>> >> return 0;
>>> >> }
>>> >>
>>> >> +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;
>>> >> +
>>> >> + if (!(priv->flags & IMX_RPROC_FLAGS_SM_LMM_OP))
>>> >> + return 0;
>>> >> +
>>> >> + /*
>>> >> + * 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;
>>> >
>>> >Why is setting this flag needed? The check that is done on it in
>>> >imx_rproc_{start|stop} should be done here. If there is an error then we don't
>>> >move forward.
>>>
>>> The flag is to indicate M7 LM could be controlled by Linux LM(to save SCMI
>>> calls. without this flag, heavy SCMI calls will be runs into). The reason
>>> on why set it here:
>>> The prepare function will be invoked in two path: rproc_attach and rproc_fw_boot.
>>> When M7 LM works in detached state and not under control of Linux LM, rproc_stop
>>> could still be invoked, so we need to avoid Linux runs into scmi calls to
>>> stop M7 LM(even if scmi firmware will return -EACCESS, but with a flag, we could
>>> save a SCMI call), so there is a check in imx_rproc_stop and this is why
>>> we need a flag there.
>>>
>>> The flag check in start might be redundant, but to keep safe, I still keep
>>> it there.
>>
>>One of the (many) problem I see with this patch is that there is no
>>IMX_RPROC_FLAGS_SM_CPU_OP to complement IMX_RPROC_FLAGS_SM_LMM_OP in
>>imx_rproc_detect_mode(), leading to if/else statements that are hard to follow.
>
>There are only two methods as written in commit log.
>one is LMM_OP, the other is CPU_OP
>
>>
>>In imx_rproc_sm_lmm_prepare(), if scmi_imx_lmm_operation() is successful, return
>>0 immediately. If -EACCESS the LMM method is unavailable in both normal and
>>detached mode, so priv->flags &= ~IMX_RPROC_FLAGS_SM_LMM_OP.
>
>No. LMM in avaiable in normal and detach mode. I have written in commit log,
>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
>
>>
>>The main takeaway here is that the code introduced by this patch is difficult to
>>understand and maintain. I suggest you find a way to make things simpler.
>
>You asked Daniel to help review this patchset. Daniel and Frank both help
>reviewed this patchset and gave R-b.
>
>You also said this patchset "looks fine to you" Jul 21 [1].
>
>Now you overturn these and say "find a way to make things simpler.
>What's the point to ask my colleague to review? What should I do,
>redesign the patchset according to "make things simpler"?
>
>Please give detailed suggestions, but not a general comment.
I realize my previous message may have come across as frustrated — I truly
appreciate your time and feedback. I’m just trying to understand the
direction you’d prefer for this patchset, especially since it had prior
R-b’s and your earlier comment that it “looks fine.”
Could you please help clarify what specifically you’d like simplified?
I’m happy to revise accordingly, but I’d really appreciate concrete
suggestions so I can move forward effectively.
@Daniel, @Frank — since you've reviewed and R-b'd this patchset, do you
have thoughts on the latest feedback from Mathieu? Would you agree that
further simplification is needed, or is the current structure acceptable?”
Thanks again!
Thanks,
Peng
>
>[1] https://lore.kernel.org/all/CANLsYkwZz4xLOG25D6S-AEGFeUBWwyp1=ytmu2q90NyEpkoX9g@mail.gmail.com/
>
>Thanks,
>Peng
>>
>
Powered by blists - more mailing lists