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 <[email protected]> >>> >> Reviewed-by: Frank Li <[email protected]> >>> >> Signed-off-by: Peng Fan <[email protected]> >>> >> --- >>> >> 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=ytmu2q90nyepko...@mail.gmail.com/ > >Thanks, >Peng >> >
