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.bal...@nxp.com>
>> >> Reviewed-by: Frank Li <frank...@nxp.com>
>> >> Signed-off-by: Peng Fan <peng....@nxp.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.
[1]
https://lore.kernel.org/all/CANLsYkwZz4xLOG25D6S-AEGFeUBWwyp1=ytmu2q90nyepko...@mail.gmail.com/
Thanks,
Peng
>
