On Tue, 2 Sept 2025 at 23:28, Peng Fan <[email protected]> wrote: > > 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.” >
I simply started taking a closer look and quickly found problems. > 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. > I did give suggestions. The core of this patch is 123 lines - even after spending two full hours reviewing it I still don't have a clear picture of what is happening. When that is the case, something is not right. When I look at a patchset, I usually ask myself if I'll remember what it does when looking at it again in 6 months. The answer was a clear "no" with this one. > @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 > >> > >
