[PATCH v5 0/3] remoteproc: imx_rproc: Support i.MX95

[PATCH v5 0/3] remoteproc: imx_rproc: Support i.MX95

[Peng Fan]

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 couldn't control it.
(2) M7 in a separate Logical Machine that Linux could control it using
    LMM protocol
(3) M7 runs in same Logical Machine as A55, so Linux could control it
    using CPU protocol

In patch 2, Use LMM and CPU protocol to manage M7. More info could be
found in the patch commit log

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.

Patchset was tested with below boot images when the patchset based on next-20250526:
imx-boot-variant-rpmsg-imx95-19x19-lpddr5-evk-sd.bin-flash_lpboot_sm_a55 (Use LMM protocol)
imx-boot-variant-alt-imx95-19x19-lpddr5-evk-sd.bin-flash_alt (Use CPU protocol)
imx-boot-imx95-19x19-lpddr5-evk-sd.bin-flash_a55 (M7 not under A55 control)
imx-boot-imx95-19x19-lpddr5-evk-sd.bin-flash_all (M7 not under A55 control)

Patchset was tested again with rebase on next-20250623
Patchset was tested again with rebase on next-20250710

Patchset is re-based on next-20250603.

Thanks for Daniel/Frank helping review the patchset before posting out to list.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Changes in v5:
- Rebased to next-20250820 to resolve minor conflict in patch 2, as below
  if (dcfg->method == IMX_RPROC_NONE || dcfg->method == IMX_RPROC_SCU_API)
	  =>
  if (dcfg->method == IMX_RPROC_NONE || dcfg->method == IMX_RPROC_SCU_API ||
      dcfg->method == IMX_RPROC_SM)
- Add Kconfig denpendencies in patch 2 following 514b2262ade4 ("firmware:
  arm_scmi: Fix i.MX build dependency")
- Drop patch 4,5 which are dts changes that enable cm7 node for i.MX95, but
  there are dtbs_check error because of "linux,code" property not laned in
  tree. To avoid false alarm on this patchset, so drop the two patches.
- Link to v4: https://lore.kernel.org/r/20250710-imx95-rproc-1-v4-0-a7123e857dfb@nxp.com

Changes in v4:
- Move the lmm permission check code to a separate
  function(imx_rproc_sm_lmm_prepare) in patch 3.
- Check return value of scmi_imx_cpu_started in patch 3
- Rebased to next-20250710 and tested on i.MX95-19x19-EVK
- Add R-b from Frank for patch 1-4 and A-b from Krzysztof for patch 1
- Drop mu7 from patch 5, because mu7 status was already okay.
- Link to v3: https://lore.kernel.org/r/20250625-imx95-rproc-1-v3-0-699031f5926d@nxp.com

Changes in v3:
- Drop fsl,lmm-id and fsl,cpu-id for binding in patch 1
- Add lmid and cpuid in driver patch 2.
- Add i.MX95 lmid and cpuid in patch 3
- Rebased to linux-next-6-23 and tested with this new rebased version
- Add dtsi/dts patch 4,5 to give people a view on how it is used per Krzysztof
- Daniel's R-b are still kept after talk with Daniel
- Link to v2: https://lore.kernel.org/r/20250606-imx95-rproc-1-v2-0-a2bd64438be9@nxp.com

Changes in v2:
- Typo fix in patch 2 commit message
- Move the m7 address mapping array from patch 2 to patch 3
- Add R-b from Daniel to patch 3
- Link to v1: https://lore.kernel.org/r/20250604-imx95-rproc-1-v1-0-a6e5f512731c@nxp.com

---
Peng Fan (3):
      dt-bindings: remoteproc: fsl,imx-rproc: Add support for i.MX95
      remoteproc: imx_rproc: Add support for System Manager API
      remoteproc: imx_rproc: Add support for i.MX95

 .../bindings/remoteproc/fsl,imx-rproc.yaml         |   1 +
 drivers/remoteproc/Kconfig                         |   2 +
 drivers/remoteproc/imx_rproc.c                     | 148 ++++++++++++++++++++-
 drivers/remoteproc/imx_rproc.h                     |   5 +
 4 files changed, 153 insertions(+), 3 deletions(-)
---
base-commit: c4b125bd6408789809eb57701eabb4cdc6407d75
change-id: 20250525-imx95-rproc-1-20bb74ddc8af

Best regards,
-- 
Peng Fan <peng.fan@nxp.com>

[PATCH v5 1/3] dt-bindings: remoteproc: fsl,imx-rproc: Add support for i.MX95

[Peng Fan]

Add compatible string for the Cortex-M7 core in i.MX95

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
index 57d75acb0b5e52ca49d1361176fdebc18a0bf7a2..ce8ec0119469c8fc0979a192b6e3d3a03108d7d2 100644
--- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
@@ -28,6 +28,7 @@ properties:
       - fsl,imx8qxp-cm4
       - fsl,imx8ulp-cm33
       - fsl,imx93-cm33
+      - fsl,imx95-cm7
 
   clocks:
     maxItems: 1

-- 
2.37.1

[PATCH v5 2/3] remoteproc: imx_rproc: Add support for System Manager API

[Peng Fan]

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@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Peng Fan <peng.fan@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;
+	} else if (ret == -EACCES) {
+		dev_info(priv->dev, "lmm(%d) not under Linux Control\n", dcfg->lmid);
+		/*
+		 * If remote cores boots up in detached mode, continue;
+		 * else linux has no permission, return -EACCES.
+		 */
+		if (priv->rproc->state != RPROC_DETACHED)
+			return -EACCES;
+
+		/* work in state RPROC_DETACHED */
+		ret = 0;
+	} else if (ret) {
+		dev_err(priv->dev, "Failed to power on lmm(%d): %d\n", ret, dcfg->lmid);
+	}
+
+	return ret;
+}
+
 static int imx_rproc_prepare(struct rproc *rproc)
 {
 	struct imx_rproc *priv = rproc->priv;
 	struct device_node *np = priv->dev->of_node;
+	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
 	struct of_phandle_iterator it;
 	struct rproc_mem_entry *mem;
 	struct reserved_mem *rmem;
@@ -593,7 +674,10 @@ static int imx_rproc_prepare(struct rproc *rproc)
 		rproc_add_carveout(rproc, mem);
 	}
 
-	return  0;
+	if (dcfg->method == IMX_RPROC_SM)
+		return imx_rproc_sm_lmm_prepare(rproc);
+
+	return 0;
 }
 
 static int imx_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
@@ -927,13 +1011,41 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
 	struct regmap_config config = { .name = "imx-rproc" };
 	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
 	struct device *dev = priv->dev;
+	struct scmi_imx_lmm_info info;
 	struct regmap *regmap;
 	struct arm_smccc_res res;
+	bool started = false;
 	int ret;
 	u32 val;
 	u8 pt;
 
 	switch (dcfg->method) {
+	case IMX_RPROC_SM:
+		/* 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.
+		 * If no, need use Logical Machine API to manage remote processor, and
+		 * set IMX_RPROC_FLAGS_SM_LMM_OP.
+		 * If yes, use CPU protocol API to manage remote processor.
+		 */
+		if (dcfg->lmid != info.lmid) {
+			priv->flags |= IMX_RPROC_FLAGS_SM_LMM_OP;
+			dev_info(dev, "Using LMM Protocol OPS\n");
+		} else {
+			dev_info(dev, "Using CPU Protocol OPS\n");
+		}
+
+		ret = scmi_imx_cpu_started(dcfg->cpuid, &started);
+		if (ret || started)
+			priv->rproc->state = RPROC_DETACHED;
+
+		return 0;
 	case IMX_RPROC_NONE:
 		priv->rproc->state = RPROC_DETACHED;
 		return 0;
@@ -1045,8 +1157,13 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv)
 	struct device *dev = priv->dev;
 	int ret;
 
-	/* Remote core is not under control of Linux or it is managed by SCU API */
-	if (dcfg->method == IMX_RPROC_NONE || dcfg->method == IMX_RPROC_SCU_API)
+	/*
+	 * Remote core is not under control of Linux or it is managed by SCU API.
+	 * System Manager(SM) firmware automatically configures clock, so also
+	 * bypass the clk settings for IMX_RPROC_SM.
+	 */
+	if (dcfg->method == IMX_RPROC_NONE || dcfg->method == IMX_RPROC_SCU_API ||
+	    dcfg->method == IMX_RPROC_SM)
 		return 0;
 
 	priv->clk = devm_clk_get(dev, NULL);
diff --git a/drivers/remoteproc/imx_rproc.h b/drivers/remoteproc/imx_rproc.h
index cfd38d37e1467d1d9e6f89be146c0b53262b92a0..6fe8d975ed302967f27b7a4319a899e6f0822976 100644
--- a/drivers/remoteproc/imx_rproc.h
+++ b/drivers/remoteproc/imx_rproc.h
@@ -26,6 +26,8 @@ enum imx_rproc_method {
 	IMX_RPROC_SCU_API,
 	/* Through Reset Controller API */
 	IMX_RPROC_RESET_CONTROLLER,
+	/* Through System Manager */
+	IMX_RPROC_SM,
 };
 
 /* dcfg flags */
@@ -42,6 +44,9 @@ struct imx_rproc_dcfg {
 	size_t				att_size;
 	enum imx_rproc_method		method;
 	u32				flags;
+	/* For System Manager(SM) based SoCs, the IDs are from SM firmware */
+	u32				cpuid;
+	u32				lmid;
 };
 
 #endif /* _IMX_RPROC_H */

-- 
2.37.1

Re: [PATCH v5 2/3] remoteproc: imx_rproc: Add support for System Manager API

[Mathieu Poirier]

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@nxp.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@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.

> +	} else if (ret == -EACCES) {
> +		dev_info(priv->dev, "lmm(%d) not under Linux Control\n", dcfg->lmid);
> +		/*
> +		 * If remote cores boots up in detached mode, continue;
> +		 * else linux has no permission, return -EACCES.
> +		 */
> +		if (priv->rproc->state != RPROC_DETACHED)
> +			return -EACCES;

The comment above scmi_imx_lmm_operation() mentions that calling
scmi_imx_lmm_operation() is done to make sure TCMs are available.  Is there a
point in calling scmi_imx_lmm_operation() if ->state == RPROC_DETACHED?  If not,
can't that check be move to the beginning of imx_rproc_sm_lmm_prepare()?  

> +
> +		/* work in state RPROC_DETACHED */
> +		ret = 0;
> +	} else if (ret) {
> +		dev_err(priv->dev, "Failed to power on lmm(%d): %d\n", ret, dcfg->lmid);
> +	}
> +
> +	return ret;
> +}
> +
>  static int imx_rproc_prepare(struct rproc *rproc)
>  {
>  	struct imx_rproc *priv = rproc->priv;
>  	struct device_node *np = priv->dev->of_node;
> +	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>  	struct of_phandle_iterator it;
>  	struct rproc_mem_entry *mem;
>  	struct reserved_mem *rmem;
> @@ -593,7 +674,10 @@ static int imx_rproc_prepare(struct rproc *rproc)
>  		rproc_add_carveout(rproc, mem);
>  	}
>  
> -	return  0;
> +	if (dcfg->method == IMX_RPROC_SM)
> +		return imx_rproc_sm_lmm_prepare(rproc);
> +
> +	return 0;
>  }
>  
>  static int imx_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> @@ -927,13 +1011,41 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
>  	struct regmap_config config = { .name = "imx-rproc" };
>  	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>  	struct device *dev = priv->dev;
> +	struct scmi_imx_lmm_info info;
>  	struct regmap *regmap;
>  	struct arm_smccc_res res;
> +	bool started = false;
>  	int ret;
>  	u32 val;
>  	u8 pt;
>  
>  	switch (dcfg->method) {
> +	case IMX_RPROC_SM:
> +		/* 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.
> +		 * If no, need use Logical Machine API to manage remote processor, and
> +		 * set IMX_RPROC_FLAGS_SM_LMM_OP.
> +		 * If yes, use CPU protocol API to manage remote processor.
> +		 */
> +		if (dcfg->lmid != info.lmid) {
> +			priv->flags |= IMX_RPROC_FLAGS_SM_LMM_OP;
> +			dev_info(dev, "Using LMM Protocol OPS\n");
> +		} else {
> +			dev_info(dev, "Using CPU Protocol OPS\n");
> +		}
> +
> +		ret = scmi_imx_cpu_started(dcfg->cpuid, &started);
> +		if (ret || started)
> +			priv->rproc->state = RPROC_DETACHED;

An error should be reported and the initialization process aborted if an error
occurs rather than defaulting to the default mode.  This will lead to additional
error conditions that will have to be handled elsewhere.

Thanks,
Mathieu

> +
> +		return 0;
>  	case IMX_RPROC_NONE:
>  		priv->rproc->state = RPROC_DETACHED;
>  		return 0;
> @@ -1045,8 +1157,13 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv)
>  	struct device *dev = priv->dev;
>  	int ret;
>  
> -	/* Remote core is not under control of Linux or it is managed by SCU API */
> -	if (dcfg->method == IMX_RPROC_NONE || dcfg->method == IMX_RPROC_SCU_API)
> +	/*
> +	 * Remote core is not under control of Linux or it is managed by SCU API.
> +	 * System Manager(SM) firmware automatically configures clock, so also
> +	 * bypass the clk settings for IMX_RPROC_SM.
> +	 */
> +	if (dcfg->method == IMX_RPROC_NONE || dcfg->method == IMX_RPROC_SCU_API ||
> +	    dcfg->method == IMX_RPROC_SM)
>  		return 0;
>  
>  	priv->clk = devm_clk_get(dev, NULL);
> diff --git a/drivers/remoteproc/imx_rproc.h b/drivers/remoteproc/imx_rproc.h
> index cfd38d37e1467d1d9e6f89be146c0b53262b92a0..6fe8d975ed302967f27b7a4319a899e6f0822976 100644
> --- a/drivers/remoteproc/imx_rproc.h
> +++ b/drivers/remoteproc/imx_rproc.h
> @@ -26,6 +26,8 @@ enum imx_rproc_method {
>  	IMX_RPROC_SCU_API,
>  	/* Through Reset Controller API */
>  	IMX_RPROC_RESET_CONTROLLER,
> +	/* Through System Manager */
> +	IMX_RPROC_SM,
>  };
>  
>  /* dcfg flags */
> @@ -42,6 +44,9 @@ struct imx_rproc_dcfg {
>  	size_t				att_size;
>  	enum imx_rproc_method		method;
>  	u32				flags;
> +	/* For System Manager(SM) based SoCs, the IDs are from SM firmware */
> +	u32				cpuid;
> +	u32				lmid;
>  };
>  
>  #endif /* _IMX_RPROC_H */
> 
> -- 
> 2.37.1
> 

Re: [PATCH v5 2/3] remoteproc: imx_rproc: Add support for System Manager API

[Peng Fan]

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@nxp.com>
>> Reviewed-by: Frank Li <Frank.Li@nxp.com>
>> Signed-off-by: Peng Fan <peng.fan@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.

>
>> +	} else if (ret == -EACCES) {
>> +		dev_info(priv->dev, "lmm(%d) not under Linux Control\n", dcfg->lmid);
>> +		/*
>> +		 * If remote cores boots up in detached mode, continue;
>> +		 * else linux has no permission, return -EACCES.
>> +		 */
>> +		if (priv->rproc->state != RPROC_DETACHED)
>> +			return -EACCES;
>
>The comment above scmi_imx_lmm_operation() mentions that calling
>scmi_imx_lmm_operation() is done to make sure TCMs are available.  Is there a
>point in calling scmi_imx_lmm_operation() if ->state == RPROC_DETACHED?  If not,
>can't that check be move to the beginning of imx_rproc_sm_lmm_prepare()?  

scmi_imx_lmm_operation also serves as permission check.

If ->state is RPROC_DETACHED, we still need to know M7 LM is or is not
under control of Linux LM.


>
>> +
>> +		/* work in state RPROC_DETACHED */
>> +		ret = 0;
>> +	} else if (ret) {
>> +		dev_err(priv->dev, "Failed to power on lmm(%d): %d\n", ret, dcfg->lmid);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  static int imx_rproc_prepare(struct rproc *rproc)
>>  {
>>  	struct imx_rproc *priv = rproc->priv;
>>  	struct device_node *np = priv->dev->of_node;
>> +	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>>  	struct of_phandle_iterator it;
>>  	struct rproc_mem_entry *mem;
>>  	struct reserved_mem *rmem;
>> @@ -593,7 +674,10 @@ static int imx_rproc_prepare(struct rproc *rproc)
>>  		rproc_add_carveout(rproc, mem);
>>  	}
>>  
>> -	return  0;
>> +	if (dcfg->method == IMX_RPROC_SM)
>> +		return imx_rproc_sm_lmm_prepare(rproc);
>> +
>> +	return 0;
>>  }
>>  
>>  static int imx_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
>> @@ -927,13 +1011,41 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
>>  	struct regmap_config config = { .name = "imx-rproc" };
>>  	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>>  	struct device *dev = priv->dev;
>> +	struct scmi_imx_lmm_info info;
>>  	struct regmap *regmap;
>>  	struct arm_smccc_res res;
>> +	bool started = false;
>>  	int ret;
>>  	u32 val;
>>  	u8 pt;
>>  
>>  	switch (dcfg->method) {
>> +	case IMX_RPROC_SM:
>> +		/* 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.
>> +		 * If no, need use Logical Machine API to manage remote processor, and
>> +		 * set IMX_RPROC_FLAGS_SM_LMM_OP.
>> +		 * If yes, use CPU protocol API to manage remote processor.
>> +		 */
>> +		if (dcfg->lmid != info.lmid) {
>> +			priv->flags |= IMX_RPROC_FLAGS_SM_LMM_OP;
>> +			dev_info(dev, "Using LMM Protocol OPS\n");
>> +		} else {
>> +			dev_info(dev, "Using CPU Protocol OPS\n");
>> +		}
>> +
>> +		ret = scmi_imx_cpu_started(dcfg->cpuid, &started);
>> +		if (ret || started)
>> +			priv->rproc->state = RPROC_DETACHED;
>
>An error should be reported and the initialization process aborted if an error
>occurs rather than defaulting to the default mode.  This will lead to additional
>error conditions that will have to be handled elsewhere.

ok. I could update to
if (ret)
	return ret;
if (started)
	priv->rproc->state = RPROC_DETACHED;



Thanks,
Peng

Re: [PATCH v5 2/3] remoteproc: imx_rproc: Add support for System Manager API

[Mathieu Poirier]

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@nxp.com>
> >> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> >> Signed-off-by: Peng Fan <peng.fan@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.

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.

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.

> 
> >
> >> +	} else if (ret == -EACCES) {
> >> +		dev_info(priv->dev, "lmm(%d) not under Linux Control\n", dcfg->lmid);
> >> +		/*
> >> +		 * If remote cores boots up in detached mode, continue;
> >> +		 * else linux has no permission, return -EACCES.
> >> +		 */
> >> +		if (priv->rproc->state != RPROC_DETACHED)
> >> +			return -EACCES;
> >
> >The comment above scmi_imx_lmm_operation() mentions that calling
> >scmi_imx_lmm_operation() is done to make sure TCMs are available.  Is there a
> >point in calling scmi_imx_lmm_operation() if ->state == RPROC_DETACHED?  If not,
> >can't that check be move to the beginning of imx_rproc_sm_lmm_prepare()?  
> 
> scmi_imx_lmm_operation also serves as permission check.
> 
> If ->state is RPROC_DETACHED, we still need to know M7 LM is or is not
> under control of Linux LM.
> 
> 
> >
> >> +
> >> +		/* work in state RPROC_DETACHED */
> >> +		ret = 0;
> >> +	} else if (ret) {
> >> +		dev_err(priv->dev, "Failed to power on lmm(%d): %d\n", ret, dcfg->lmid);
> >> +	}
> >> +
> >> +	return ret;
> >> +}
> >> +
> >>  static int imx_rproc_prepare(struct rproc *rproc)
> >>  {
> >>  	struct imx_rproc *priv = rproc->priv;
> >>  	struct device_node *np = priv->dev->of_node;
> >> +	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
> >>  	struct of_phandle_iterator it;
> >>  	struct rproc_mem_entry *mem;
> >>  	struct reserved_mem *rmem;
> >> @@ -593,7 +674,10 @@ static int imx_rproc_prepare(struct rproc *rproc)
> >>  		rproc_add_carveout(rproc, mem);
> >>  	}
> >>  
> >> -	return  0;
> >> +	if (dcfg->method == IMX_RPROC_SM)
> >> +		return imx_rproc_sm_lmm_prepare(rproc);
> >> +
> >> +	return 0;
> >>  }
> >>  
> >>  static int imx_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> >> @@ -927,13 +1011,41 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
> >>  	struct regmap_config config = { .name = "imx-rproc" };
> >>  	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
> >>  	struct device *dev = priv->dev;
> >> +	struct scmi_imx_lmm_info info;
> >>  	struct regmap *regmap;
> >>  	struct arm_smccc_res res;
> >> +	bool started = false;
> >>  	int ret;
> >>  	u32 val;
> >>  	u8 pt;
> >>  
> >>  	switch (dcfg->method) {
> >> +	case IMX_RPROC_SM:
> >> +		/* 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.
> >> +		 * If no, need use Logical Machine API to manage remote processor, and
> >> +		 * set IMX_RPROC_FLAGS_SM_LMM_OP.
> >> +		 * If yes, use CPU protocol API to manage remote processor.
> >> +		 */
> >> +		if (dcfg->lmid != info.lmid) {
> >> +			priv->flags |= IMX_RPROC_FLAGS_SM_LMM_OP;
> >> +			dev_info(dev, "Using LMM Protocol OPS\n");
> >> +		} else {
> >> +			dev_info(dev, "Using CPU Protocol OPS\n");
> >> +		}
> >> +
> >> +		ret = scmi_imx_cpu_started(dcfg->cpuid, &started);
> >> +		if (ret || started)
> >> +			priv->rproc->state = RPROC_DETACHED;
> >
> >An error should be reported and the initialization process aborted if an error
> >occurs rather than defaulting to the default mode.  This will lead to additional
> >error conditions that will have to be handled elsewhere.
> 
> ok. I could update to
> if (ret)
> 	return ret;
> if (started)
> 	priv->rproc->state = RPROC_DETACHED;
> 
> 
> 
> Thanks,
> Peng
> 

Re: [PATCH v5 2/3] remoteproc: imx_rproc: Add support for System Manager API

[Peng Fan]

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@nxp.com>
>> >> Reviewed-by: Frank Li <Frank.Li@nxp.com>
>> >> Signed-off-by: Peng Fan <peng.fan@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=ytmu2q90NyEpkoX9g@mail.gmail.com/

Thanks,
Peng
>

Re: [PATCH v5 2/3] remoteproc: imx_rproc: Add support for System Manager API

[Peng Fan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 10851 bytes --]

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@nxp.com>
>>> >> Reviewed-by: Frank Li <Frank.Li@nxp.com>
>>> >> Signed-off-by: Peng Fan <peng.fan@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.

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
>>
>

Re: [PATCH v5 2/3] remoteproc: imx_rproc: Add support for System Manager API

[Daniel Baluta]

<snip>

> >Please give detailed suggestions, but not a general comment.
>
>
> @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?”

Peng, please trim the message when replying so that you keep only
relevant part of the text.

I think you are both right here. I understand your frustration :)
but also Mathieu's point of view is reasonable.

For a person who doesn't know the internals of IMX remoteproc the code
is a little
bit hard to understand. Peng I've given you my R-b because even if the
feels like
it is getting complicated it is still manageable and you are the
maintainer for it.

Allow me a few days to see if we can simplify the logic.

thanks,
Daniel.

Re: [PATCH v5 2/3] remoteproc: imx_rproc: Add support for System Manager API

[Mathieu Poirier]

On Wed, 3 Sept 2025 at 01:20, Daniel Baluta <daniel.baluta@gmail.com> wrote:
>
> <snip>
>
> > >Please give detailed suggestions, but not a general comment.
> >
> >
> > @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?”
>
> Peng, please trim the message when replying so that you keep only
> relevant part of the text.
>
> I think you are both right here. I understand your frustration :)
> but also Mathieu's point of view is reasonable.
>
> For a person who doesn't know the internals of IMX remoteproc the code
> is a little
> bit hard to understand. Peng I've given you my R-b because even if the
> feels like
> it is getting complicated it is still manageable and you are the
> maintainer for it.
>
> Allow me a few days to see if we can simplify the logic.
>

Excellent.

> thanks,
> Daniel.

Re: [PATCH v5 2/3] remoteproc: imx_rproc: Add support for System Manager API

[Mathieu Poirier]

On Tue, 2 Sept 2025 at 23:28, Peng Fan <peng.fan@oss.nxp.com> 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 <daniel.baluta@nxp.com>
> >>> >> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> >>> >> Signed-off-by: Peng Fan <peng.fan@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.
>
> 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=ytmu2q90NyEpkoX9g@mail.gmail.com/
> >
> >Thanks,
> >Peng
> >>
> >

[PATCH v5 3/3] remoteproc: imx_rproc: Add support for i.MX95

[Peng Fan]

Add imx_rproc_cfg_imx95_m7 and address(TCM and DDR) mapping.
Add i.MX95 of_device_id entry.

Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 151b9ca34bac2dac9df0ed873f493791f2d1466e..6f83fcdcd8528e6b91269369e23d39a0395fa8d0 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -74,6 +74,10 @@
 
 #define IMX_SC_IRQ_GROUP_REBOOTED	5
 
+/* Must align with System Manager Firmware */
+#define IMX95_M7_CPUID			1
+#define IMX95_M7_LMID			1
+
 /**
  * struct imx_rproc_mem - slim internal memory structure
  * @cpu_addr: MPU virtual address of the memory region
@@ -127,6 +131,18 @@ struct imx_rproc {
 	u32				flags;
 };
 
+static const struct imx_rproc_att imx_rproc_att_imx95_m7[] = {
+	/* dev addr , sys addr  , size	    , flags */
+	/* TCM CODE NON-SECURE */
+	{ 0x00000000, 0x203C0000, 0x00040000, ATT_OWN | ATT_IOMEM },
+
+	/* TCM SYS NON-SECURE*/
+	{ 0x20000000, 0x20400000, 0x00040000, ATT_OWN | ATT_IOMEM },
+
+	/* DDR */
+	{ 0x80000000, 0x80000000, 0x50000000, 0 },
+};
+
 static const struct imx_rproc_att imx_rproc_att_imx93[] = {
 	/* dev addr , sys addr  , size	    , flags */
 	/* TCM CODE NON-SECURE */
@@ -373,6 +389,14 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx93 = {
 	.method		= IMX_RPROC_SMC,
 };
 
+static const struct imx_rproc_dcfg imx_rproc_cfg_imx95_m7 = {
+	.att		= imx_rproc_att_imx95_m7,
+	.att_size	= ARRAY_SIZE(imx_rproc_att_imx95_m7),
+	.method		= IMX_RPROC_SM,
+	.cpuid		= IMX95_M7_CPUID,
+	.lmid		= IMX95_M7_LMID,
+};
+
 static int imx_rproc_start(struct rproc *rproc)
 {
 	struct imx_rproc *priv = rproc->priv;
@@ -1338,6 +1362,7 @@ static const struct of_device_id imx_rproc_of_match[] = {
 	{ .compatible = "fsl,imx8qm-cm4", .data = &imx_rproc_cfg_imx8qm },
 	{ .compatible = "fsl,imx8ulp-cm33", .data = &imx_rproc_cfg_imx8ulp },
 	{ .compatible = "fsl,imx93-cm33", .data = &imx_rproc_cfg_imx93 },
+	{ .compatible = "fsl,imx95-cm7", .data = &imx_rproc_cfg_imx95_m7 },
 	{},
 };
 MODULE_DEVICE_TABLE(of, imx_rproc_of_match);

-- 
2.37.1