[PATCH v3 0/6] firmware: arm_scmi: imx: Dump syslog and system_info

[PATCH v3 0/6] firmware: arm_scmi: imx: Dump syslog and system_info

[Peng Fan]

System Manager firmware provides API to dump board, silicon, firmware
information. It also provides API to dump system sleep, wakeup
information. So add the interface for Linux to retrieve the information:

patch 1 is to add doc for board information which was missed before.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Changes in v3:
- Add R-b from Cristian
- Store size, per Cristian's comments in patch 5: "*(ipriv->size) =
st->num_returned + st->num_remaining"
- Minor change in patch 6 to add size check
- Rebased to next-20250826
- Link to v2: https://lore.kernel.org/r/20250710-sm-misc-api-v1-v2-0-c3e953e34be6@nxp.com

Changes in v2:
- Add more info in patch 1 for documentation(attribute is reserved for
  future use, boardname may not align with the board name in device tree)
- Drop dump silicon revision API
- Update discover build info, cfg info, board info API to be only used in probe phase
- Update to use single sysfs single value for syslog.
- Link to v1: https://lore.kernel.org/r/20250627-sm-misc-api-v1-v1-0-2b99481fe825@nxp.com

---
Peng Fan (6):
      firmware: arm_scmi: imx: Add documentation for MISC_BOARD_INFO
      firmware: arm_scmi: imx: Support discovering buildinfo of MISC protocol
      firmware: arm_scmi: imx: Support getting cfg info of MISC protocol
      firmware: arm_scmi: imx: Support getting board info of MISC protocol
      firmware: arm_scmi: imx: Support getting syslog of MISC protocol
      firmware: imx: sm-misc: Dump syslog info

 .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 193 +++++++++++++++++
 drivers/firmware/arm_scmi/vendors/imx/imx95.rst    |  22 ++
 drivers/firmware/imx/sm-misc.c                     | 232 +++++++++++++++++++++
 include/linux/scmi_imx_protocol.h                  |  19 ++
 4 files changed, 466 insertions(+)
---
base-commit: 6180c21371050f261dd253947057bf46955548f7
change-id: 20250627-sm-misc-api-v1-85c030c670c6

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

[PATCH v3 1/6] firmware: arm_scmi: imx: Add documentation for MISC_BOARD_INFO

[Peng Fan]

System Manager Firmware supports getting board information, add
documentation for this API

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/arm_scmi/vendors/imx/imx95.rst | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
index 4e246a78a042a79eb81be35632079c7626bbbe57..0a4d157b3ea12c7735aa19d8d6c64ae8504d0c71 100644
--- a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
+++ b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
@@ -1670,6 +1670,28 @@ protocol_id: 0x84
 |uint32 syslog[N]    |Log data array, N is defined in bits[11:0] of numLogflags|
 +--------------------+---------------------------------------------------------+
 
+MISC_BOARD_INFO
+~~~~~~~~~~~~~~~
+
+message_id: 0xE
+protocol_id: 0x84
+
++--------------------+---------------------------------------------------------+
+|Return values                                                                 |
++--------------------+---------------------------------------------------------+
+|Name                |Description                                              |
++--------------------+---------------------------------------------------------+
+|int32 status        |SUCCESS: config name return                              |
+|                    |NOT_SUPPORTED: name not available                        |
++--------------------+---------------------------------------------------------+
+|uint32 attributes   |Board specific attributes reserved for future expansion  |
+|                    |without breaking backwards compatibility                 |
++--------------------+---------------------------------------------------------+
+|uint8 boardname[16] |Board name. Null terminated ASCII string of up           |
+|                    |to 16 bytes in length.(This is SM exported boardname     |
+|                    |which may not align with the boardname in device tree)   |
++--------------------+---------------------------------------------------------+
+
 NEGOTIATE_PROTOCOL_VERSION
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 

-- 
2.37.1

[PATCH v3 2/6] firmware: arm_scmi: imx: Support discovering buildinfo of MISC protocol

[Peng Fan]

MISC protocol supports discovering the System Manager(SM) build
information including build commit, build time and etc. Retrieve the
information from SM.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
index a8915d3b4df518719d56bfff38922625ad9b70f6..464afeae8267d8c1eca4c4d5e008eca6d741c6ff 100644
--- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
+++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
@@ -25,6 +25,7 @@
 enum scmi_imx_misc_protocol_cmd {
 	SCMI_IMX_MISC_CTRL_SET	= 0x3,
 	SCMI_IMX_MISC_CTRL_GET	= 0x4,
+	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
 	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
 };
 
@@ -65,6 +66,15 @@ struct scmi_imx_misc_ctrl_get_out {
 	__le32 val[];
 };
 
+struct scmi_imx_misc_buildinfo_out {
+	__le32 buildnum;
+	__le32 buildcommit;
+#define MISC_MAX_BUILDDATE	16
+	u8 builddate[MISC_MAX_BUILDDATE];
+#define MISC_MAX_BUILDTIME	16
+	u8 buildtime[MISC_MAX_BUILDTIME];
+};
+
 static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
 					struct scmi_imx_misc_info *mi)
 {
@@ -272,6 +282,33 @@ static int scmi_imx_misc_ctrl_set(const struct scmi_protocol_handle *ph,
 	return ret;
 }
 
+static int scmi_imx_misc_discover_build_info(const struct scmi_protocol_handle *ph)
+{
+	char date[MISC_MAX_BUILDDATE], time[MISC_MAX_BUILDTIME];
+	struct scmi_imx_misc_buildinfo_out *out;
+	struct scmi_xfer *t;
+	int ret;
+
+	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_DISCOVER_BUILDINFO, 0,
+				      sizeof(*out), &t);
+	if (ret)
+		return ret;
+
+	ret = ph->xops->do_xfer(ph, t);
+	if (!ret) {
+		out = t->rx.buf;
+		strscpy(date, out->builddate, MISC_MAX_BUILDDATE);
+		strscpy(time, out->buildtime, MISC_MAX_BUILDTIME);
+		dev_info(ph->dev, "SM Version\t= Build %u, Commit %08x %s %s\n",
+			le32_to_cpu(out->buildnum), le32_to_cpu(out->buildcommit),
+			date, time);
+	}
+
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
 static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
 	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
 	.misc_ctrl_get = scmi_imx_misc_ctrl_get,
@@ -299,6 +336,10 @@ static int scmi_imx_misc_protocol_init(const struct scmi_protocol_handle *ph)
 	if (ret)
 		return ret;
 
+	ret = scmi_imx_misc_discover_build_info(ph);
+	if (ret)
+		return ret;
+
 	return ph->set_priv(ph, minfo, version);
 }
 

-- 
2.37.1

[PATCH v3 3/6] firmware: arm_scmi: imx: Support getting cfg info of MISC protocol

[Peng Fan]

MISC protocol supports getting the System Manager(SM) mode selection
and configuration name. Retrieve the information from SM.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
index 464afeae8267d8c1eca4c4d5e008eca6d741c6ff..220b9369fb537306f9e1a105930ad4d65e6b10aa 100644
--- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
+++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
@@ -26,6 +26,7 @@ enum scmi_imx_misc_protocol_cmd {
 	SCMI_IMX_MISC_CTRL_SET	= 0x3,
 	SCMI_IMX_MISC_CTRL_GET	= 0x4,
 	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
+	SCMI_IMX_MISC_CFG_INFO = 0xC,
 	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
 };
 
@@ -75,6 +76,12 @@ struct scmi_imx_misc_buildinfo_out {
 	u8 buildtime[MISC_MAX_BUILDTIME];
 };
 
+struct scmi_imx_misc_cfg_info_out {
+	__le32 msel;
+#define MISC_MAX_CFGNAME	16
+	u8 cfgname[MISC_MAX_CFGNAME];
+};
+
 static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
 					struct scmi_imx_misc_info *mi)
 {
@@ -309,6 +316,30 @@ static int scmi_imx_misc_discover_build_info(const struct scmi_protocol_handle *
 	return ret;
 }
 
+static int scmi_imx_misc_cfg_info(const struct scmi_protocol_handle *ph)
+{
+	struct scmi_imx_misc_cfg_info_out *out;
+	char name[MISC_MAX_CFGNAME];
+	struct scmi_xfer *t;
+	int ret;
+
+	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CFG_INFO, 0, sizeof(*out), &t);
+	if (ret)
+		return ret;
+
+	ret = ph->xops->do_xfer(ph, t);
+	if (!ret) {
+		out = t->rx.buf;
+		strscpy(name, out->cfgname, MISC_MAX_CFGNAME);
+		dev_info(ph->dev, "SM Config\t= %s, mSel = %u\n",
+			 name, le32_to_cpu(out->msel));
+	}
+
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
 static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
 	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
 	.misc_ctrl_get = scmi_imx_misc_ctrl_get,
@@ -340,6 +371,10 @@ static int scmi_imx_misc_protocol_init(const struct scmi_protocol_handle *ph)
 	if (ret)
 		return ret;
 
+	ret = scmi_imx_misc_cfg_info(ph);
+	if (ret)
+		return ret;
+
 	return ph->set_priv(ph, minfo, version);
 }
 

-- 
2.37.1

[PATCH v3 4/6] firmware: arm_scmi: imx: Support getting board info of MISC protocol

[Peng Fan]

MISC protocol supports getting board information. Retrieve the information
from SM.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
index 220b9369fb537306f9e1a105930ad4d65e6b10aa..f934b4fbc6ec9f1e6b24d1c6c8cd07b45ce548e3 100644
--- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
+++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
@@ -27,6 +27,7 @@ enum scmi_imx_misc_protocol_cmd {
 	SCMI_IMX_MISC_CTRL_GET	= 0x4,
 	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
 	SCMI_IMX_MISC_CFG_INFO = 0xC,
+	SCMI_IMX_MISC_BOARD_INFO = 0xE,
 	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
 };
 
@@ -76,6 +77,12 @@ struct scmi_imx_misc_buildinfo_out {
 	u8 buildtime[MISC_MAX_BUILDTIME];
 };
 
+struct scmi_imx_misc_board_info_out {
+	__le32 attributes;
+#define MISC_MAX_BRDNAME	16
+	u8 brdname[MISC_MAX_BRDNAME];
+};
+
 struct scmi_imx_misc_cfg_info_out {
 	__le32 msel;
 #define MISC_MAX_CFGNAME	16
@@ -316,6 +323,30 @@ static int scmi_imx_misc_discover_build_info(const struct scmi_protocol_handle *
 	return ret;
 }
 
+static int scmi_imx_misc_board_info(const struct scmi_protocol_handle *ph)
+{
+	struct scmi_imx_misc_board_info_out *out;
+	char name[MISC_MAX_BRDNAME];
+	struct scmi_xfer *t;
+	int ret;
+
+	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_BOARD_INFO, 0, sizeof(*out), &t);
+	if (ret)
+		return ret;
+
+	ret = ph->xops->do_xfer(ph, t);
+	if (!ret) {
+		out = t->rx.buf;
+		strscpy(name, out->brdname, MISC_MAX_BRDNAME);
+		dev_info(ph->dev, "Board\t\t= %s, attr=0x%08x\n",
+			 name, le32_to_cpu(out->attributes));
+	}
+
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
 static int scmi_imx_misc_cfg_info(const struct scmi_protocol_handle *ph)
 {
 	struct scmi_imx_misc_cfg_info_out *out;
@@ -371,6 +402,10 @@ static int scmi_imx_misc_protocol_init(const struct scmi_protocol_handle *ph)
 	if (ret)
 		return ret;
 
+	ret = scmi_imx_misc_board_info(ph);
+	if (ret)
+		return ret;
+
 	ret = scmi_imx_misc_cfg_info(ph);
 	if (ret)
 		return ret;

-- 
2.37.1

[PATCH v3 5/6] firmware: arm_scmi: imx: Support getting syslog of MISC protocol

[Peng Fan]

MISC protocol supports getting system log regarding system sleep latency
,wakeup interrupt and etc. Add the API for user to retrieve the
information from SM.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 82 ++++++++++++++++++++++
 include/linux/scmi_imx_protocol.h                  | 19 +++++
 2 files changed, 101 insertions(+)

diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
index f934b4fbc6ec9f1e6b24d1c6c8cd07b45ce548e3..2d3423d83aed857329a9a367d0ec0681a1d77d0b 100644
--- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
+++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
@@ -27,6 +27,7 @@ enum scmi_imx_misc_protocol_cmd {
 	SCMI_IMX_MISC_CTRL_GET	= 0x4,
 	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
 	SCMI_IMX_MISC_CFG_INFO = 0xC,
+	SCMI_IMX_MISC_SYSLOG = 0xD,
 	SCMI_IMX_MISC_BOARD_INFO = 0xE,
 	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
 };
@@ -89,6 +90,19 @@ struct scmi_imx_misc_cfg_info_out {
 	u8 cfgname[MISC_MAX_CFGNAME];
 };
 
+struct scmi_imx_misc_syslog_in {
+	__le32 flags;
+	__le32 index;
+};
+
+#define REMAINING(x)	le32_get_bits((x), GENMASK(31, 20))
+#define RETURNED(x)	le32_get_bits((x), GENMASK(11, 0))
+
+struct scmi_imx_misc_syslog_out {
+	__le32 numlogflags;
+	__le32 syslog[];
+};
+
 static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
 					struct scmi_imx_misc_info *mi)
 {
@@ -371,10 +385,78 @@ static int scmi_imx_misc_cfg_info(const struct scmi_protocol_handle *ph)
 	return ret;
 }
 
+struct scmi_imx_misc_syslog_ipriv {
+	u32 *array;
+	u16 *size;
+};
+
+static void iter_misc_syslog_prepare_message(void *message, u32 desc_index,
+					     const void *priv)
+{
+	struct scmi_imx_misc_syslog_in *msg = message;
+
+	msg->flags = cpu_to_le32(0);
+	msg->index = cpu_to_le32(desc_index);
+}
+
+static int iter_misc_syslog_update_state(struct scmi_iterator_state *st,
+					 const void *response, void *priv)
+{
+	const struct scmi_imx_misc_syslog_out *r = response;
+	struct scmi_imx_misc_syslog_ipriv *p = priv;
+
+	st->num_returned = RETURNED(r->numlogflags);
+	st->num_remaining = REMAINING(r->numlogflags);
+	*(p->size) = st->num_returned + st->num_remaining;
+
+	return 0;
+}
+
+static int
+iter_misc_syslog_process_response(const struct scmi_protocol_handle *ph,
+				  const void *response,
+				  struct scmi_iterator_state *st, void *priv)
+{
+	const struct scmi_imx_misc_syslog_out *r = response;
+	struct scmi_imx_misc_syslog_ipriv *p = priv;
+
+	p->array[st->desc_index + st->loop_idx] =
+		le32_to_cpu(r->syslog[st->loop_idx]);
+
+	return 0;
+}
+
+static int scmi_imx_misc_syslog(const struct scmi_protocol_handle *ph, u16 *size,
+				void *array)
+{
+	struct scmi_iterator_ops ops = {
+		.prepare_message = iter_misc_syslog_prepare_message,
+		.update_state = iter_misc_syslog_update_state,
+		.process_response = iter_misc_syslog_process_response,
+	};
+	struct scmi_imx_misc_syslog_ipriv ipriv = {
+		.array = array,
+		.size = size,
+	};
+	void *iter;
+
+	if (!array || !size || !*size)
+		return -EINVAL;
+
+	iter = ph->hops->iter_response_init(ph, &ops, *size, SCMI_IMX_MISC_SYSLOG,
+					    sizeof(struct scmi_imx_misc_syslog_in),
+					    &ipriv);
+	if (IS_ERR(iter))
+		return PTR_ERR(iter);
+
+	return ph->hops->iter_response_run(iter);
+}
+
 static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
 	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
 	.misc_ctrl_get = scmi_imx_misc_ctrl_get,
 	.misc_ctrl_req_notify = scmi_imx_misc_ctrl_notify,
+	.misc_syslog = scmi_imx_misc_syslog,
 };
 
 static int scmi_imx_misc_protocol_init(const struct scmi_protocol_handle *ph)
diff --git a/include/linux/scmi_imx_protocol.h b/include/linux/scmi_imx_protocol.h
index 27bd372cbfb142b6acb0b1cf4b82f061529d0d45..6e18920aac56de67d388e985e3305745d3798c3e 100644
--- a/include/linux/scmi_imx_protocol.h
+++ b/include/linux/scmi_imx_protocol.h
@@ -52,6 +52,23 @@ struct scmi_imx_misc_ctrl_notify_report {
 	unsigned int		flags;
 };
 
+struct scmi_imx_misc_sys_sleep_rec {
+	u32 sleepentryusec;
+	u32 sleepexitusec;
+	u32 sleepcnt;
+	u32 wakesource;
+	u32 mixpwrstat;
+	u32 mempwrstat;
+	u32 pllpwrstat;
+	u32 syssleepmode;
+	u32 syssleepflags;
+};
+
+struct scmi_imx_misc_syslog {
+	struct scmi_imx_misc_sys_sleep_rec syssleeprecord;
+	uint32_t deverrlog;
+};
+
 struct scmi_imx_misc_proto_ops {
 	int (*misc_ctrl_set)(const struct scmi_protocol_handle *ph, u32 id,
 			     u32 num, u32 *val);
@@ -59,6 +76,8 @@ struct scmi_imx_misc_proto_ops {
 			     u32 *num, u32 *val);
 	int (*misc_ctrl_req_notify)(const struct scmi_protocol_handle *ph,
 				    u32 ctrl_id, u32 evt_id, u32 flags);
+	int (*misc_syslog)(const struct scmi_protocol_handle *ph, u16 *size,
+			   void *array);
 };
 
 /* See LMM_ATTRIBUTES in imx95.rst */

-- 
2.37.1

[PATCH v3 6/6] firmware: imx: sm-misc: Dump syslog info

[Peng Fan]

Add sysfs interface to read System Manager syslog info

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/imx/sm-misc.c | 232 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 232 insertions(+)

diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
index fc3ee12c2be878e0285183e3381c9514a63d5142..e742a2fff06b44ab7f07e9f97f723ef7ca1ff259 100644
--- a/drivers/firmware/imx/sm-misc.c
+++ b/drivers/firmware/imx/sm-misc.c
@@ -3,6 +3,7 @@
  * Copyright 2024 NXP
  */
 
+#include <linux/device/devres.h>
 #include <linux/firmware/imx/sm.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -44,10 +45,232 @@ static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
 	return 0;
 }
 
+static ssize_t
+wakevector_show(struct device *device, struct device_attribute *attr, char *buf)
+{
+	struct scmi_imx_misc_syslog *syslog = dev_get_drvdata(device);
+	u16 size = sizeof(*syslog) / 4;
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, &size, syslog);
+	if (ret)
+		return ret;
+
+	if (size * 4 < offsetofend(struct scmi_imx_misc_sys_sleep_rec, wakesource)) {
+		dev_err(device, "%s: returned truncated data\n", __func__);
+		return -EINVAL;
+	}
+
+	return sysfs_emit(buf, "%u\n", syslog->syssleeprecord.wakesource);
+}
+static DEVICE_ATTR_RO(wakevector);
+
+static ssize_t
+syssleepmode_show(struct device *device, struct device_attribute *attr, char *buf)
+{
+	struct scmi_imx_misc_syslog *syslog = dev_get_drvdata(device);
+	u16 size = sizeof(*syslog) / 4;
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, &size, syslog);
+	if (ret)
+		return ret;
+
+	if (size * 4 < offsetofend(struct scmi_imx_misc_sys_sleep_rec, syssleepmode)) {
+		dev_err(device, "%s: returned truncated data\n", __func__);
+		return -EINVAL;
+	}
+
+	return sysfs_emit(buf, "%u\n", syslog->syssleeprecord.syssleepmode);
+}
+static DEVICE_ATTR_RO(syssleepmode);
+
+static ssize_t
+syssleepflags_show(struct device *device, struct device_attribute *attr, char *buf)
+{
+	struct scmi_imx_misc_syslog *syslog = dev_get_drvdata(device);
+	u16 size = sizeof(*syslog) / 4;
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, &size, syslog);
+	if (ret)
+		return ret;
+
+	if (size * 4 < offsetofend(struct scmi_imx_misc_sys_sleep_rec, syssleepflags)) {
+		dev_err(device, "%s: returned truncated data\n", __func__);
+		return -EINVAL;
+	}
+
+	return sysfs_emit(buf, "0x%08x\n", syslog->syssleeprecord.syssleepflags);
+}
+static DEVICE_ATTR_RO(syssleepflags);
+
+static ssize_t
+mixpwrstat_show(struct device *device, struct device_attribute *attr, char *buf)
+{
+	struct scmi_imx_misc_syslog *syslog = dev_get_drvdata(device);
+	u16 size = sizeof(*syslog) / 4;
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, &size, syslog);
+	if (ret)
+		return ret;
+
+	if (size * 4 < offsetofend(struct scmi_imx_misc_sys_sleep_rec, mixpwrstat)) {
+		dev_err(device, "%s: returned truncated data\n", __func__);
+		return -EINVAL;
+	}
+
+	return sysfs_emit(buf, "0x%08x\n", syslog->syssleeprecord.mixpwrstat);
+}
+static DEVICE_ATTR_RO(mixpwrstat);
+
+static ssize_t
+mempwrstat_show(struct device *device, struct device_attribute *attr, char *buf)
+{
+	struct scmi_imx_misc_syslog *syslog = dev_get_drvdata(device);
+	u16 size = sizeof(*syslog) / 4;
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, &size, syslog);
+	if (ret)
+		return ret;
+
+	if (size * 4 < offsetofend(struct scmi_imx_misc_sys_sleep_rec, mempwrstat)) {
+		dev_err(device, "%s: returned truncated data\n", __func__);
+		return -EINVAL;
+	}
+
+	return sysfs_emit(buf, "0x%08x\n", syslog->syssleeprecord.mempwrstat);
+}
+static DEVICE_ATTR_RO(mempwrstat);
+
+static ssize_t
+pllpwrstat_show(struct device *device, struct device_attribute *attr, char *buf)
+{
+	struct scmi_imx_misc_syslog *syslog = dev_get_drvdata(device);
+	u16 size = sizeof(*syslog) / 4;
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, &size, syslog);
+	if (ret)
+		return ret;
+
+	if (size * 4 < offsetofend(struct scmi_imx_misc_sys_sleep_rec, pllpwrstat)) {
+		dev_err(device, "%s: returned truncated data\n", __func__);
+		return -EINVAL;
+	}
+
+	return sysfs_emit(buf, "0x%08x\n", syslog->syssleeprecord.pllpwrstat);
+}
+static DEVICE_ATTR_RO(pllpwrstat);
+
+static ssize_t
+sleepentryusec_show(struct device *device, struct device_attribute *attr, char *buf)
+{
+	struct scmi_imx_misc_syslog *syslog = dev_get_drvdata(device);
+	u16 size = sizeof(*syslog) / 4;
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, &size, syslog);
+	if (ret)
+		return ret;
+
+	if (size * 4 < offsetofend(struct scmi_imx_misc_sys_sleep_rec, sleepentryusec)) {
+		dev_err(device, "%s: returned truncated data\n", __func__);
+		return -EINVAL;
+	}
+
+	return sysfs_emit(buf, "%u\n", syslog->syssleeprecord.sleepentryusec);
+}
+static DEVICE_ATTR_RO(sleepentryusec);
+
+static ssize_t
+sleepexitusec_show(struct device *device, struct device_attribute *attr, char *buf)
+{
+	struct scmi_imx_misc_syslog *syslog = dev_get_drvdata(device);
+	u16 size = sizeof(*syslog) / 4;
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, &size, syslog);
+	if (ret)
+		return ret;
+
+	if (size * 4 < offsetofend(struct scmi_imx_misc_sys_sleep_rec, sleepexitusec)) {
+		dev_err(device, "%s: returned truncated data\n", __func__);
+		return -EINVAL;
+	}
+
+	return sysfs_emit(buf, "%u\n", syslog->syssleeprecord.sleepexitusec);
+}
+static DEVICE_ATTR_RO(sleepexitusec);
+
+static ssize_t
+sleepcnt_show(struct device *device, struct device_attribute *attr, char *buf)
+{
+	struct scmi_imx_misc_syslog *syslog = dev_get_drvdata(device);
+	u16 size = sizeof(*syslog) / 4;
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, &size, syslog);
+	if (ret)
+		return ret;
+
+	if (size * 4 < offsetofend(struct scmi_imx_misc_sys_sleep_rec, sleepcnt)) {
+		dev_err(device, "%s: returned truncated data\n", __func__);
+		return -EINVAL;
+	}
+
+	return sysfs_emit(buf, "%u\n", syslog->syssleeprecord.sleepcnt);
+}
+static DEVICE_ATTR_RO(sleepcnt);
+
+static struct attribute *sm_misc_attrs[] = {
+	&dev_attr_wakevector.attr,
+	&dev_attr_syssleepmode.attr,
+	&dev_attr_syssleepflags.attr,
+	&dev_attr_mixpwrstat.attr,
+	&dev_attr_mempwrstat.attr,
+	&dev_attr_pllpwrstat.attr,
+	&dev_attr_sleepentryusec.attr,
+	&dev_attr_sleepexitusec.attr,
+	&dev_attr_sleepcnt.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(sm_misc);
+
 static int scmi_imx_misc_ctrl_probe(struct scmi_device *sdev)
 {
 	const struct scmi_handle *handle = sdev->handle;
 	struct device_node *np = sdev->dev.of_node;
+	struct scmi_imx_misc_syslog *syslog;
 	u32 src_id, flags;
 	int ret, i, num;
 
@@ -63,6 +286,12 @@ static int scmi_imx_misc_ctrl_probe(struct scmi_device *sdev)
 	if (IS_ERR(imx_misc_ctrl_ops))
 		return PTR_ERR(imx_misc_ctrl_ops);
 
+	syslog = devm_kzalloc(&sdev->dev, sizeof(*syslog), GFP_KERNEL);
+	if (!syslog)
+		return -ENOMEM;
+
+	dev_set_drvdata(&sdev->dev, syslog);
+
 	num = of_property_count_u32_elems(np, "nxp,ctrl-ids");
 	if (num % 2) {
 		dev_err(&sdev->dev, "Invalid wakeup-sources\n");
@@ -108,6 +337,9 @@ static const struct scmi_device_id scmi_id_table[] = {
 MODULE_DEVICE_TABLE(scmi, scmi_id_table);
 
 static struct scmi_driver scmi_imx_misc_ctrl_driver = {
+	.driver = {
+		.dev_groups = sm_misc_groups,
+	},
 	.name = "scmi-imx-misc-ctrl",
 	.probe = scmi_imx_misc_ctrl_probe,
 	.id_table = scmi_id_table,

-- 
2.37.1

Re: [PATCH v3 1/6] firmware: arm_scmi: imx: Add documentation for MISC_BOARD_INFO

[Sudeep Holla]

On Wed, Aug 27, 2025 at 12:59:13PM +0800, Peng Fan wrote:
> System Manager Firmware supports getting board information, add
> documentation for this API

s/API/MISC_BOARD_INFO command/

> 
> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/firmware/arm_scmi/vendors/imx/imx95.rst | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
> index 4e246a78a042a79eb81be35632079c7626bbbe57..0a4d157b3ea12c7735aa19d8d6c64ae8504d0c71 100644
> --- a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
> @@ -1670,6 +1670,28 @@ protocol_id: 0x84
>  |uint32 syslog[N]    |Log data array, N is defined in bits[11:0] of numLogflags|
>  +--------------------+---------------------------------------------------------+
>  
> +MISC_BOARD_INFO
> +~~~~~~~~~~~~~~~
> +
> +message_id: 0xE
> +protocol_id: 0x84
> +
> ++--------------------+---------------------------------------------------------+
> +|Return values                                                                 |
> ++--------------------+---------------------------------------------------------+
> +|Name                |Description                                              |
> ++--------------------+---------------------------------------------------------+
> +|int32 status        |SUCCESS: config name return                              |
> +|                    |NOT_SUPPORTED: name not available                        |
> ++--------------------+---------------------------------------------------------+
> +|uint32 attributes   |Board specific attributes reserved for future expansion  |
> +|                    |without breaking backwards compatibility                 |

Is 0 the expected value in this version of the spec as it is just reserved ?
Please state the same explicitly here.

-- 
Regards,
Sudeep

Re: [PATCH v3 2/6] firmware: arm_scmi: imx: Support discovering buildinfo of MISC protocol

[Sudeep Holla]

On Wed, Aug 27, 2025 at 12:59:14PM +0800, Peng Fan wrote:
> MISC protocol supports discovering the System Manager(SM) build
> information including build commit, build time and etc. Retrieve the
> information from SM.
> 
> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> index a8915d3b4df518719d56bfff38922625ad9b70f6..464afeae8267d8c1eca4c4d5e008eca6d741c6ff 100644
> --- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> @@ -25,6 +25,7 @@
>  enum scmi_imx_misc_protocol_cmd {
>  	SCMI_IMX_MISC_CTRL_SET	= 0x3,
>  	SCMI_IMX_MISC_CTRL_GET	= 0x4,
> +	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
>  	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
>  };
>  
> @@ -65,6 +66,15 @@ struct scmi_imx_misc_ctrl_get_out {
>  	__le32 val[];
>  };
>  
> +struct scmi_imx_misc_buildinfo_out {
> +	__le32 buildnum;
> +	__le32 buildcommit;
> +#define MISC_MAX_BUILDDATE	16
> +	u8 builddate[MISC_MAX_BUILDDATE];
> +#define MISC_MAX_BUILDTIME	16
> +	u8 buildtime[MISC_MAX_BUILDTIME];
> +};
> +
>  static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
>  					struct scmi_imx_misc_info *mi)
>  {
> @@ -272,6 +282,33 @@ static int scmi_imx_misc_ctrl_set(const struct scmi_protocol_handle *ph,
>  	return ret;
>  }
>  
> +static int scmi_imx_misc_discover_build_info(const struct scmi_protocol_handle *ph)
> +{
> +	char date[MISC_MAX_BUILDDATE], time[MISC_MAX_BUILDTIME];
> +	struct scmi_imx_misc_buildinfo_out *out;
> +	struct scmi_xfer *t;
> +	int ret;
> +
> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_DISCOVER_BUILDINFO, 0,
> +				      sizeof(*out), &t);
> +	if (ret)
> +		return ret;
> +
> +	ret = ph->xops->do_xfer(ph, t);
> +	if (!ret) {
> +		out = t->rx.buf;
> +		strscpy(date, out->builddate, MISC_MAX_BUILDDATE);
> +		strscpy(time, out->buildtime, MISC_MAX_BUILDTIME);
> +		dev_info(ph->dev, "SM Version\t= Build %u, Commit %08x %s %s\n",
> +			le32_to_cpu(out->buildnum), le32_to_cpu(out->buildcommit),
> +			date, time);
> +	}
> +
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
>  static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
>  	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
>  	.misc_ctrl_get = scmi_imx_misc_ctrl_get,
> @@ -299,6 +336,10 @@ static int scmi_imx_misc_protocol_init(const struct scmi_protocol_handle *ph)
>  	if (ret)
>  		return ret;
>  
> +	ret = scmi_imx_misc_discover_build_info(ph);

To align with other naming,

s/scmi_imx_misc_discover_build_info/scmi_imx_misc_build_info_discover/

> +		return ret;

The document in 1/6 didn't state this as mandatory. IMO, it is not
fair to return error if the return is NOT_SUPPORTED.

-- 
Regards,
Sudeep

Re: [PATCH v3 2/6] firmware: arm_scmi: imx: Support discovering buildinfo of MISC protocol

[Sudeep Holla]

On Wed, Aug 27, 2025 at 12:59:14PM +0800, Peng Fan wrote:
> MISC protocol supports discovering the System Manager(SM) build
> information including build commit, build time and etc. Retrieve the
> information from SM.
> 
> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> index a8915d3b4df518719d56bfff38922625ad9b70f6..464afeae8267d8c1eca4c4d5e008eca6d741c6ff 100644
> --- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> @@ -25,6 +25,7 @@
>  enum scmi_imx_misc_protocol_cmd {
>  	SCMI_IMX_MISC_CTRL_SET	= 0x3,
>  	SCMI_IMX_MISC_CTRL_GET	= 0x4,
> +	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
>  	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
>  };
>  
> @@ -65,6 +66,15 @@ struct scmi_imx_misc_ctrl_get_out {
>  	__le32 val[];
>  };
>  
> +struct scmi_imx_misc_buildinfo_out {
> +	__le32 buildnum;
> +	__le32 buildcommit;
> +#define MISC_MAX_BUILDDATE	16
> +	u8 builddate[MISC_MAX_BUILDDATE];
> +#define MISC_MAX_BUILDTIME	16
> +	u8 buildtime[MISC_MAX_BUILDTIME];
> +};
> +
>  static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
>  					struct scmi_imx_misc_info *mi)
>  {
> @@ -272,6 +282,33 @@ static int scmi_imx_misc_ctrl_set(const struct scmi_protocol_handle *ph,
>  	return ret;
>  }
>  
> +static int scmi_imx_misc_discover_build_info(const struct scmi_protocol_handle *ph)
> +{
> +	char date[MISC_MAX_BUILDDATE], time[MISC_MAX_BUILDTIME];
> +	struct scmi_imx_misc_buildinfo_out *out;
> +	struct scmi_xfer *t;
> +	int ret;
> +
> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_DISCOVER_BUILDINFO, 0,
> +				      sizeof(*out), &t);
> +	if (ret)
> +		return ret;
> +
> +	ret = ph->xops->do_xfer(ph, t);
> +	if (!ret) {
> +		out = t->rx.buf;
> +		strscpy(date, out->builddate, MISC_MAX_BUILDDATE);
> +		strscpy(time, out->buildtime, MISC_MAX_BUILDTIME);
> +		dev_info(ph->dev, "SM Version\t= Build %u, Commit %08x %s %s\n",
> +			le32_to_cpu(out->buildnum), le32_to_cpu(out->buildcommit),
> +			date, time);

And fix the alignment above, just noticed after hitting enter.

-- 
Regards,
Sudeep

Re: [PATCH v3 3/6] firmware: arm_scmi: imx: Support getting cfg info of MISC protocol

[Sudeep Holla]

On Wed, Aug 27, 2025 at 12:59:15PM +0800, Peng Fan wrote:
> MISC protocol supports getting the System Manager(SM) mode selection
> and configuration name. Retrieve the information from SM.
> 
> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> index 464afeae8267d8c1eca4c4d5e008eca6d741c6ff..220b9369fb537306f9e1a105930ad4d65e6b10aa 100644
> --- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> @@ -26,6 +26,7 @@ enum scmi_imx_misc_protocol_cmd {
>  	SCMI_IMX_MISC_CTRL_SET	= 0x3,
>  	SCMI_IMX_MISC_CTRL_GET	= 0x4,
>  	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
> +	SCMI_IMX_MISC_CFG_INFO = 0xC,

1. Order it by command number
2. Be consistent with the document, MISC_CFG_INFO_GET is used there.

>  	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
>  };
>  
> @@ -75,6 +76,12 @@ struct scmi_imx_misc_buildinfo_out {
>  	u8 buildtime[MISC_MAX_BUILDTIME];
>  };
>  
> +struct scmi_imx_misc_cfg_info_out {
> +	__le32 msel;
> +#define MISC_MAX_CFGNAME	16
> +	u8 cfgname[MISC_MAX_CFGNAME];
> +};
> +
>  static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
>  					struct scmi_imx_misc_info *mi)
>  {
> @@ -309,6 +316,30 @@ static int scmi_imx_misc_discover_build_info(const struct scmi_protocol_handle *
>  	return ret;
>  }
>  
> +static int scmi_imx_misc_cfg_info(const struct scmi_protocol_handle *ph)
> +{
> +	struct scmi_imx_misc_cfg_info_out *out;
> +	char name[MISC_MAX_CFGNAME];
> +	struct scmi_xfer *t;
> +	int ret;
> +
> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CFG_INFO, 0, sizeof(*out), &t);
> +	if (ret)
> +		return ret;
> +
> +	ret = ph->xops->do_xfer(ph, t);
> +	if (!ret) {
> +		out = t->rx.buf;
> +		strscpy(name, out->cfgname, MISC_MAX_CFGNAME);
> +		dev_info(ph->dev, "SM Config\t= %s, mSel = %u\n",
> +			 name, le32_to_cpu(out->msel));
> +	}
> +
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
>  static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
>  	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
>  	.misc_ctrl_get = scmi_imx_misc_ctrl_get,
> @@ -340,6 +371,10 @@ static int scmi_imx_misc_protocol_init(const struct scmi_protocol_handle *ph)
>  	if (ret)
>  		return ret;
>  
> +	ret = scmi_imx_misc_cfg_info(ph);

s/scmi_imx_misc_cfg_info/scmi_imx_misc_cfg_info_get/

> +	if (ret)

Again the document hasn't mark it as mandatory, so handle NOT_SUPPORTED
gracefully.

-- 
Regards,
Sudeep

Re: [PATCH v3 4/6] firmware: arm_scmi: imx: Support getting board info of MISC protocol

[Sudeep Holla]

On Wed, Aug 27, 2025 at 12:59:16PM +0800, Peng Fan wrote:
> MISC protocol supports getting board information. Retrieve the information
> from SM.
> 
> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> index 220b9369fb537306f9e1a105930ad4d65e6b10aa..f934b4fbc6ec9f1e6b24d1c6c8cd07b45ce548e3 100644
> --- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> @@ -27,6 +27,7 @@ enum scmi_imx_misc_protocol_cmd {
>  	SCMI_IMX_MISC_CTRL_GET	= 0x4,
>  	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
>  	SCMI_IMX_MISC_CFG_INFO = 0xC,
> +	SCMI_IMX_MISC_BOARD_INFO = 0xE,

Again keep it ordered by command number ?

>  	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
>  };
>  
> @@ -76,6 +77,12 @@ struct scmi_imx_misc_buildinfo_out {
>  	u8 buildtime[MISC_MAX_BUILDTIME];
>  };
>  
> +struct scmi_imx_misc_board_info_out {
> +	__le32 attributes;
> +#define MISC_MAX_BRDNAME	16
> +	u8 brdname[MISC_MAX_BRDNAME];
> +};
> +
>  struct scmi_imx_misc_cfg_info_out {
>  	__le32 msel;
>  #define MISC_MAX_CFGNAME	16
> @@ -316,6 +323,30 @@ static int scmi_imx_misc_discover_build_info(const struct scmi_protocol_handle *
>  	return ret;
>  }
>  
> +static int scmi_imx_misc_board_info(const struct scmi_protocol_handle *ph)
> +{
> +	struct scmi_imx_misc_board_info_out *out;
> +	char name[MISC_MAX_BRDNAME];
> +	struct scmi_xfer *t;
> +	int ret;
> +
> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_BOARD_INFO, 0, sizeof(*out), &t);
> +	if (ret)
> +		return ret;
> +
> +	ret = ph->xops->do_xfer(ph, t);
> +	if (!ret) {
> +		out = t->rx.buf;
> +		strscpy(name, out->brdname, MISC_MAX_BRDNAME);
> +		dev_info(ph->dev, "Board\t\t= %s, attr=0x%08x\n",
> +			 name, le32_to_cpu(out->attributes));
> +	}
> +
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
>  static int scmi_imx_misc_cfg_info(const struct scmi_protocol_handle *ph)
>  {
>  	struct scmi_imx_misc_cfg_info_out *out;
> @@ -371,6 +402,10 @@ static int scmi_imx_misc_protocol_init(const struct scmi_protocol_handle *ph)
>  	if (ret)
>  		return ret;
>  
> +	ret = scmi_imx_misc_board_info(ph);
> +	if (ret)
> +		return ret;
> +

Ditto, not mandatory, graceful return on NOT_SUPPORTED please.

-- 
Regards,
Sudeep

Re: [PATCH v3 5/6] firmware: arm_scmi: imx: Support getting syslog of MISC protocol

[Sudeep Holla]

On Wed, Aug 27, 2025 at 12:59:17PM +0800, Peng Fan wrote:
> MISC protocol supports getting system log regarding system sleep latency
> ,wakeup interrupt and etc. Add the API for user to retrieve the
> information from SM.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 82 ++++++++++++++++++++++
>  include/linux/scmi_imx_protocol.h                  | 19 +++++
>  2 files changed, 101 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> index f934b4fbc6ec9f1e6b24d1c6c8cd07b45ce548e3..2d3423d83aed857329a9a367d0ec0681a1d77d0b 100644
> --- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> @@ -27,6 +27,7 @@ enum scmi_imx_misc_protocol_cmd {
>  	SCMI_IMX_MISC_CTRL_GET	= 0x4,
>  	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
>  	SCMI_IMX_MISC_CFG_INFO = 0xC,
> +	SCMI_IMX_MISC_SYSLOG = 0xD,

1. Not ordered
2. Inconsistent command name with the document.

>  	SCMI_IMX_MISC_BOARD_INFO = 0xE,
>  	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
>  };
> @@ -89,6 +90,19 @@ struct scmi_imx_misc_cfg_info_out {
>  	u8 cfgname[MISC_MAX_CFGNAME];
>  };
>  
> +struct scmi_imx_misc_syslog_in {
> +	__le32 flags;
> +	__le32 index;
> +};
> +
> +#define REMAINING(x)	le32_get_bits((x), GENMASK(31, 20))
> +#define RETURNED(x)	le32_get_bits((x), GENMASK(11, 0))
> +
> +struct scmi_imx_misc_syslog_out {
> +	__le32 numlogflags;
> +	__le32 syslog[];
> +};
> +
>  static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
>  					struct scmi_imx_misc_info *mi)
>  {
> @@ -371,10 +385,78 @@ static int scmi_imx_misc_cfg_info(const struct scmi_protocol_handle *ph)
>  	return ret;
>  }
>  
> +struct scmi_imx_misc_syslog_ipriv {
> +	u32 *array;
> +	u16 *size;
> +};
> +
> +static void iter_misc_syslog_prepare_message(void *message, u32 desc_index,
> +					     const void *priv)
> +{
> +	struct scmi_imx_misc_syslog_in *msg = message;
> +
> +	msg->flags = cpu_to_le32(0);
> +	msg->index = cpu_to_le32(desc_index);
> +}
> +
> +static int iter_misc_syslog_update_state(struct scmi_iterator_state *st,
> +					 const void *response, void *priv)
> +{
> +	const struct scmi_imx_misc_syslog_out *r = response;
> +	struct scmi_imx_misc_syslog_ipriv *p = priv;
> +
> +	st->num_returned = RETURNED(r->numlogflags);
> +	st->num_remaining = REMAINING(r->numlogflags);
> +	*(p->size) = st->num_returned + st->num_remaining;

I think you can drop () above.

> +
> +	return 0;
> +}
> +
> +static int
> +iter_misc_syslog_process_response(const struct scmi_protocol_handle *ph,
> +				  const void *response,
> +				  struct scmi_iterator_state *st, void *priv)
> +{
> +	const struct scmi_imx_misc_syslog_out *r = response;
> +	struct scmi_imx_misc_syslog_ipriv *p = priv;
> +
> +	p->array[st->desc_index + st->loop_idx] =
> +		le32_to_cpu(r->syslog[st->loop_idx]);
> +
> +	return 0;
> +}
> +
> +static int scmi_imx_misc_syslog(const struct scmi_protocol_handle *ph, u16 *size,
> +				void *array)
> +{
> +	struct scmi_iterator_ops ops = {
> +		.prepare_message = iter_misc_syslog_prepare_message,
> +		.update_state = iter_misc_syslog_update_state,
> +		.process_response = iter_misc_syslog_process_response,
> +	};
> +	struct scmi_imx_misc_syslog_ipriv ipriv = {
> +		.array = array,
> +		.size = size,
> +	};
> +	void *iter;
> +
> +	if (!array || !size || !*size)
> +		return -EINVAL;
> +
> +	iter = ph->hops->iter_response_init(ph, &ops, *size, SCMI_IMX_MISC_SYSLOG,
> +					    sizeof(struct scmi_imx_misc_syslog_in),
> +					    &ipriv);
> +	if (IS_ERR(iter))
> +		return PTR_ERR(iter);
> +

Handle NOT_SUPPORTED if not mandatory, may need update to the document to
add NOT_SUPPORTED as return value. Currently it's only success in which
case you don't need any error handling at all 😉.

> +	return ph->hops->iter_response_run(iter);
> +}
> +
>  static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
>  	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
>  	.misc_ctrl_get = scmi_imx_misc_ctrl_get,
>  	.misc_ctrl_req_notify = scmi_imx_misc_ctrl_notify,
> +	.misc_syslog = scmi_imx_misc_syslog,
>  };
>  
>  static int scmi_imx_misc_protocol_init(const struct scmi_protocol_handle *ph)
> diff --git a/include/linux/scmi_imx_protocol.h b/include/linux/scmi_imx_protocol.h
> index 27bd372cbfb142b6acb0b1cf4b82f061529d0d45..6e18920aac56de67d388e985e3305745d3798c3e 100644
> --- a/include/linux/scmi_imx_protocol.h
> +++ b/include/linux/scmi_imx_protocol.h
> @@ -52,6 +52,23 @@ struct scmi_imx_misc_ctrl_notify_report {
>  	unsigned int		flags;
>  };
>  
> +struct scmi_imx_misc_sys_sleep_rec {
> +	u32 sleepentryusec;
> +	u32 sleepexitusec;
> +	u32 sleepcnt;
> +	u32 wakesource;
> +	u32 mixpwrstat;
> +	u32 mempwrstat;
> +	u32 pllpwrstat;
> +	u32 syssleepmode;
> +	u32 syssleepflags;
> +};
> +
> +struct scmi_imx_misc_syslog {
> +	struct scmi_imx_misc_sys_sleep_rec syssleeprecord;
> +	uint32_t deverrlog;

s/uint32_t/u32/ for consistency ? Just look above 3-4 line e.g.

-- 
Regards,
Sudeep

Re: [PATCH v3 5/6] firmware: arm_scmi: imx: Support getting syslog of MISC protocol

[Sudeep Holla]

On Wed, Aug 27, 2025 at 12:59:17PM +0800, Peng Fan wrote:

[...]

>  static int scmi_imx_misc_protocol_init(const struct scmi_protocol_handle *ph)
> diff --git a/include/linux/scmi_imx_protocol.h b/include/linux/scmi_imx_protocol.h
> index 27bd372cbfb142b6acb0b1cf4b82f061529d0d45..6e18920aac56de67d388e985e3305745d3798c3e 100644
> --- a/include/linux/scmi_imx_protocol.h
> +++ b/include/linux/scmi_imx_protocol.h
> @@ -52,6 +52,23 @@ struct scmi_imx_misc_ctrl_notify_report {
>  	unsigned int		flags;
>  };
>  
> +struct scmi_imx_misc_sys_sleep_rec {
> +	u32 sleepentryusec;
> +	u32 sleepexitusec;
> +	u32 sleepcnt;
> +	u32 wakesource;
> +	u32 mixpwrstat;
> +	u32 mempwrstat;
> +	u32 pllpwrstat;
> +	u32 syssleepmode;
> +	u32 syssleepflags;
> +};

And since the document/spec doesn't cover details on above, better
to add comment for this to explain each of it. How do you ensure
the backward compatibility if you are assigning a structure to a log
buffer that is not so structured the spec/document. Will be end up
with one structure for each version if f/w screws up the structure ?

Can it be a raw dump of man 4KB instead ? Sorry just realised it now.

-- 
Regards,
Sudeep

RE: [PATCH v3 5/6] firmware: arm_scmi: imx: Support getting syslog of MISC protocol

[Peng Fan]

Hi Sudeep,

> Subject: Re: [PATCH v3 5/6] firmware: arm_scmi: imx: Support getting
> syslog of MISC protocol
> 
> On Wed, Aug 27, 2025 at 12:59:17PM +0800, Peng Fan wrote:
> 
> [...]
> 
> >  static int scmi_imx_misc_protocol_init(const struct
> > scmi_protocol_handle *ph) diff --git
> > a/include/linux/scmi_imx_protocol.h
> > b/include/linux/scmi_imx_protocol.h
> > index
> >
> 27bd372cbfb142b6acb0b1cf4b82f061529d0d45..6e18920aac56de67d
> 388e985e330
> > 5745d3798c3e 100644
> > --- a/include/linux/scmi_imx_protocol.h
> > +++ b/include/linux/scmi_imx_protocol.h
> > @@ -52,6 +52,23 @@ struct scmi_imx_misc_ctrl_notify_report {
> >  	unsigned int		flags;
> >  };
> >
> > +struct scmi_imx_misc_sys_sleep_rec {
> > +	u32 sleepentryusec;
> > +	u32 sleepexitusec;
> > +	u32 sleepcnt;
> > +	u32 wakesource;
> > +	u32 mixpwrstat;
> > +	u32 mempwrstat;
> > +	u32 pllpwrstat;
> > +	u32 syssleepmode;
> > +	u32 syssleepflags;
> > +};
> 
> And since the document/spec doesn't cover details on above, better to
> add comment for this to explain each of it. 

ok. I will add comment for each entry.

How do you ensure the
> backward compatibility if you are assigning a structure to a log buffer
> that is not so structured the spec/document. Will be end up with one
> structure for each version if f/w screws up the structure ?

What I could do is to ask firmware developer not to change
the structure to keep backward compatibility and add comment in
imx-sm firmware to note this structure should be kept backward
compatible.

> 
> Can it be a raw dump of man 4KB instead ? Sorry just realised it now.

4KB is quite a large area to dump, and current imx-sm only supports
to dump 40 bytes for syslog. The syslog size may expand in future,
but I think it may not reach to 4KB that large.

For raw dump, not sure you mean not parsing the data and just
export the raw data through sysfs interface or else.

But I would avoid user to directly get raw data. For
"diagnose message", raw data should be ok if future imx-sm
firmware supports diagnose from linux.

I am thinking to do a v4 in early next week if no big issues.
Then this patchset should have chance to catch your PR
in rc5 or rc6. Hope this is ok for you.

Thanks,
Peng.

> 
> --
> Regards,
> Sudeep

Re: [PATCH v3 1/6] firmware: arm_scmi: imx: Add documentation for MISC_BOARD_INFO

[Peng Fan]

On Fri, Aug 29, 2025 at 11:43:41AM +0100, Sudeep Holla wrote:
>On Wed, Aug 27, 2025 at 12:59:13PM +0800, Peng Fan wrote:
>> System Manager Firmware supports getting board information, add
>> documentation for this API
>
>s/API/MISC_BOARD_INFO command/
>

Fix in V4.

>> 
>> +protocol_id: 0x84
>> +
>> ++--------------------+---------------------------------------------------------+
>> +|Return values                                                                 |
>> ++--------------------+---------------------------------------------------------+
>> +|Name                |Description                                              |
>> ++--------------------+---------------------------------------------------------+
>> +|int32 status        |SUCCESS: config name return                              |
>> +|                    |NOT_SUPPORTED: name not available                        |
>> ++--------------------+---------------------------------------------------------+
>> +|uint32 attributes   |Board specific attributes reserved for future expansion  |
>> +|                    |without breaking backwards compatibility                 |
>
>Is 0 the expected value in this version of the spec as it is just reserved ?
>Please state the same explicitly here.

Yes. The SM firmware set it 0 by default.

I will add "The firmware set the value to 0 for now".

Thanks,
Peng

>
>-- 
>Regards,
>Sudeep

Re: [PATCH v3 2/6] firmware: arm_scmi: imx: Support discovering buildinfo of MISC protocol

[Peng Fan]

On Fri, Aug 29, 2025 at 11:48:56AM +0100, Sudeep Holla wrote:
>On Wed, Aug 27, 2025 at 12:59:14PM +0800, Peng Fan wrote:
>> MISC protocol supports discovering the System Manager(SM) build
>> information including build commit, build time and etc. Retrieve the
>> information from SM.
>> 
>> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>  .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 41 ++++++++++++++++++++++
>>  1 file changed, 41 insertions(+)
>> 
>> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> index a8915d3b4df518719d56bfff38922625ad9b70f6..464afeae8267d8c1eca4c4d5e008eca6d741c6ff 100644
>> --- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> @@ -25,6 +25,7 @@
>>  enum scmi_imx_misc_protocol_cmd {
>>  	SCMI_IMX_MISC_CTRL_SET	= 0x3,
>>  	SCMI_IMX_MISC_CTRL_GET	= 0x4,
>> +	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
>>  	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
>>  };
>>  
>> @@ -65,6 +66,15 @@ struct scmi_imx_misc_ctrl_get_out {
>>  	__le32 val[];
>>  };
>>  
>> +struct scmi_imx_misc_buildinfo_out {
>> +	__le32 buildnum;
>> +	__le32 buildcommit;
>> +#define MISC_MAX_BUILDDATE	16
>> +	u8 builddate[MISC_MAX_BUILDDATE];
>> +#define MISC_MAX_BUILDTIME	16
>> +	u8 buildtime[MISC_MAX_BUILDTIME];
>> +};
>> +
>>  static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
>>  					struct scmi_imx_misc_info *mi)
>>  {
>> @@ -272,6 +282,33 @@ static int scmi_imx_misc_ctrl_set(const struct scmi_protocol_handle *ph,
>>  	return ret;
>>  }
>>  
>> +static int scmi_imx_misc_discover_build_info(const struct scmi_protocol_handle *ph)
>> +{
>> +	char date[MISC_MAX_BUILDDATE], time[MISC_MAX_BUILDTIME];
>> +	struct scmi_imx_misc_buildinfo_out *out;
>> +	struct scmi_xfer *t;
>> +	int ret;
>> +
>> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_DISCOVER_BUILDINFO, 0,
>> +				      sizeof(*out), &t);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ph->xops->do_xfer(ph, t);
>> +	if (!ret) {
>> +		out = t->rx.buf;
>> +		strscpy(date, out->builddate, MISC_MAX_BUILDDATE);
>> +		strscpy(time, out->buildtime, MISC_MAX_BUILDTIME);
>> +		dev_info(ph->dev, "SM Version\t= Build %u, Commit %08x %s %s\n",
>> +			le32_to_cpu(out->buildnum), le32_to_cpu(out->buildcommit),
>> +			date, time);
>> +	}
>> +
>> +	ph->xops->xfer_put(ph, t);
>> +
>> +	return ret;
>> +}
>> +
>>  static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
>>  	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
>>  	.misc_ctrl_get = scmi_imx_misc_ctrl_get,
>> @@ -299,6 +336,10 @@ static int scmi_imx_misc_protocol_init(const struct scmi_protocol_handle *ph)
>>  	if (ret)
>>  		return ret;
>>  
>> +	ret = scmi_imx_misc_discover_build_info(ph);
>
>To align with other naming,
>
>s/scmi_imx_misc_discover_build_info/scmi_imx_misc_build_info_discover/

Fix in V4.

>
>> +		return ret;
>
>The document in 1/6 didn't state this as mandatory. IMO, it is not
>fair to return error if the return is NOT_SUPPORTED.

Indeed. I will use below:
        ret = scmi_imx_misc_build_info_discover(ph);
        if (ret)
                dev_err("Failed to get build info: %d\n", ret);

Thanks,
Peng

>
>-- 
>Regards,
>Sudeep

Re: [PATCH v3 2/6] firmware: arm_scmi: imx: Support discovering buildinfo of MISC protocol

[Peng Fan]

On Fri, Aug 29, 2025 at 11:50:20AM +0100, Sudeep Holla wrote:
>On Wed, Aug 27, 2025 at 12:59:14PM +0800, Peng Fan wrote:
>> MISC protocol supports discovering the System Manager(SM) build
>> information including build commit, build time and etc. Retrieve the
>> information from SM.
>> 
>> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>  .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 41 ++++++++++++++++++++++
>>  1 file changed, 41 insertions(+)
>> 
>> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> index a8915d3b4df518719d56bfff38922625ad9b70f6..464afeae8267d8c1eca4c4d5e008eca6d741c6ff 100644
>> --- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> @@ -25,6 +25,7 @@
>>  enum scmi_imx_misc_protocol_cmd {
>>  	SCMI_IMX_MISC_CTRL_SET	= 0x3,
>>  	SCMI_IMX_MISC_CTRL_GET	= 0x4,
>> +	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
>>  	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
>>  };
>>  
>> @@ -65,6 +66,15 @@ struct scmi_imx_misc_ctrl_get_out {
>>  	__le32 val[];
>>  };
>>  
>> +struct scmi_imx_misc_buildinfo_out {
>> +	__le32 buildnum;
>> +	__le32 buildcommit;
>> +#define MISC_MAX_BUILDDATE	16
>> +	u8 builddate[MISC_MAX_BUILDDATE];
>> +#define MISC_MAX_BUILDTIME	16
>> +	u8 buildtime[MISC_MAX_BUILDTIME];
>> +};
>> +
>>  static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
>>  					struct scmi_imx_misc_info *mi)
>>  {
>> @@ -272,6 +282,33 @@ static int scmi_imx_misc_ctrl_set(const struct scmi_protocol_handle *ph,
>>  	return ret;
>>  }
>>  
>> +static int scmi_imx_misc_discover_build_info(const struct scmi_protocol_handle *ph)
>> +{
>> +	char date[MISC_MAX_BUILDDATE], time[MISC_MAX_BUILDTIME];
>> +	struct scmi_imx_misc_buildinfo_out *out;
>> +	struct scmi_xfer *t;
>> +	int ret;
>> +
>> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_DISCOVER_BUILDINFO, 0,
>> +				      sizeof(*out), &t);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ph->xops->do_xfer(ph, t);
>> +	if (!ret) {
>> +		out = t->rx.buf;
>> +		strscpy(date, out->builddate, MISC_MAX_BUILDDATE);
>> +		strscpy(time, out->buildtime, MISC_MAX_BUILDTIME);
>> +		dev_info(ph->dev, "SM Version\t= Build %u, Commit %08x %s %s\n",
>> +			le32_to_cpu(out->buildnum), le32_to_cpu(out->buildcommit),
>> +			date, time);
>
>And fix the alignment above, just noticed after hitting enter.

My bad, just not sure why "b4 prep --check" not report warning for this.

Thanks,
Peng

>
>-- 
>Regards,
>Sudeep

Re: [PATCH v3 3/6] firmware: arm_scmi: imx: Support getting cfg info of MISC protocol

[Peng Fan]

On Fri, Aug 29, 2025 at 11:56:15AM +0100, Sudeep Holla wrote:
>On Wed, Aug 27, 2025 at 12:59:15PM +0800, Peng Fan wrote:
>> MISC protocol supports getting the System Manager(SM) mode selection
>> and configuration name. Retrieve the information from SM.
>> 
>> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>  .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 35 ++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>> 
>> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> index 464afeae8267d8c1eca4c4d5e008eca6d741c6ff..220b9369fb537306f9e1a105930ad4d65e6b10aa 100644
>> --- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> @@ -26,6 +26,7 @@ enum scmi_imx_misc_protocol_cmd {
>>  	SCMI_IMX_MISC_CTRL_SET	= 0x3,
>>  	SCMI_IMX_MISC_CTRL_GET	= 0x4,
>>  	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
>> +	SCMI_IMX_MISC_CFG_INFO = 0xC,
>
>1. Order it by command number
>2. Be consistent with the document, MISC_CFG_INFO_GET is used there.

Fix in V4.

>
>>  	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
>>  };
>>  
>> @@ -75,6 +76,12 @@ struct scmi_imx_misc_buildinfo_out {
>>  	u8 buildtime[MISC_MAX_BUILDTIME];
>>  };
>>  
>> +struct scmi_imx_misc_cfg_info_out {
>> +	__le32 msel;
>> +#define MISC_MAX_CFGNAME	16
>> +	u8 cfgname[MISC_MAX_CFGNAME];
>> +};
>> +
>>  static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
>>  					struct scmi_imx_misc_info *mi)
>>  {
>> @@ -309,6 +316,30 @@ static int scmi_imx_misc_discover_build_info(const struct scmi_protocol_handle *
>>  	return ret;
>>  }
>>  
>> +static int scmi_imx_misc_cfg_info(const struct scmi_protocol_handle *ph)
>> +{
>> +	struct scmi_imx_misc_cfg_info_out *out;
>> +	char name[MISC_MAX_CFGNAME];
>> +	struct scmi_xfer *t;
>> +	int ret;
>> +
>> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CFG_INFO, 0, sizeof(*out), &t);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ph->xops->do_xfer(ph, t);
>> +	if (!ret) {
>> +		out = t->rx.buf;
>> +		strscpy(name, out->cfgname, MISC_MAX_CFGNAME);
>> +		dev_info(ph->dev, "SM Config\t= %s, mSel = %u\n",
>> +			 name, le32_to_cpu(out->msel));
>> +	}
>> +
>> +	ph->xops->xfer_put(ph, t);
>> +
>> +	return ret;
>> +}
>> +
>>  static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
>>  	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
>>  	.misc_ctrl_get = scmi_imx_misc_ctrl_get,
>> @@ -340,6 +371,10 @@ static int scmi_imx_misc_protocol_init(const struct scmi_protocol_handle *ph)
>>  	if (ret)
>>  		return ret;
>>  
>> +	ret = scmi_imx_misc_cfg_info(ph);
>
>s/scmi_imx_misc_cfg_info/scmi_imx_misc_cfg_info_get/

Fix in V4.

>
>> +	if (ret)
>
>Again the document hasn't mark it as mandatory, so handle NOT_SUPPORTED
>gracefully.

Yeah,
"if (ret && ret != -EOPNOTSUPP)
	return ret;
"

Thanks,
Peng

>
>-- 
>Regards,
>Sudeep

Re: [PATCH v3 2/6] firmware: arm_scmi: imx: Support discovering buildinfo of MISC protocol

[Peng Fan]

On Fri, Aug 29, 2025 at 11:48:56AM +0100, Sudeep Holla wrote:
>On Wed, Aug 27, 2025 at 12:59:14PM +0800, Peng Fan wrote:
>> MISC protocol supports discovering the System Manager(SM) build
>> information including build commit, build time and etc. Retrieve the
>> information from SM.
>> 
>> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>  .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 41 ++++++++++++++++++++++
>>  1 file changed, 41 insertions(+)
>> 
>> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> index a8915d3b4df518719d56bfff38922625ad9b70f6..464afeae8267d8c1eca4c4d5e008eca6d741c6ff 100644
>> --- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> @@ -25,6 +25,7 @@
>>  enum scmi_imx_misc_protocol_cmd {
>>  	SCMI_IMX_MISC_CTRL_SET	= 0x3,
>>  	SCMI_IMX_MISC_CTRL_GET	= 0x4,
>> +	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
>>  	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
>>  };
>>  
>> @@ -65,6 +66,15 @@ struct scmi_imx_misc_ctrl_get_out {
>>  	__le32 val[];
>>  };
>>  
>> +struct scmi_imx_misc_buildinfo_out {
>> +	__le32 buildnum;
>> +	__le32 buildcommit;
>> +#define MISC_MAX_BUILDDATE	16
>> +	u8 builddate[MISC_MAX_BUILDDATE];
>> +#define MISC_MAX_BUILDTIME	16
>> +	u8 buildtime[MISC_MAX_BUILDTIME];
>> +};
>> +
>>  static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
>>  					struct scmi_imx_misc_info *mi)
>>  {
>> @@ -272,6 +282,33 @@ static int scmi_imx_misc_ctrl_set(const struct scmi_protocol_handle *ph,
>>  	return ret;
>>  }
>>  
>> +static int scmi_imx_misc_discover_build_info(const struct scmi_protocol_handle *ph)
>> +{
>> +	char date[MISC_MAX_BUILDDATE], time[MISC_MAX_BUILDTIME];
>> +	struct scmi_imx_misc_buildinfo_out *out;
>> +	struct scmi_xfer *t;
>> +	int ret;
>> +
>> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_DISCOVER_BUILDINFO, 0,
>> +				      sizeof(*out), &t);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ph->xops->do_xfer(ph, t);
>> +	if (!ret) {
>> +		out = t->rx.buf;
>> +		strscpy(date, out->builddate, MISC_MAX_BUILDDATE);
>> +		strscpy(time, out->buildtime, MISC_MAX_BUILDTIME);
>> +		dev_info(ph->dev, "SM Version\t= Build %u, Commit %08x %s %s\n",
>> +			le32_to_cpu(out->buildnum), le32_to_cpu(out->buildcommit),
>> +			date, time);
>> +	}
>> +
>> +	ph->xops->xfer_put(ph, t);
>> +
>> +	return ret;
>> +}
>> +
>>  static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
>>  	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
>>  	.misc_ctrl_get = scmi_imx_misc_ctrl_get,
>> @@ -299,6 +336,10 @@ static int scmi_imx_misc_protocol_init(const struct scmi_protocol_handle *ph)
>>  	if (ret)
>>  		return ret;
>>  
>> +	ret = scmi_imx_misc_discover_build_info(ph);
>
>To align with other naming,
>
>s/scmi_imx_misc_discover_build_info/scmi_imx_misc_build_info_discover/
>
>> +		return ret;
>
>The document in 1/6 didn't state this as mandatory. IMO, it is not
>fair to return error if the return is NOT_SUPPORTED.

I should use:
if (ret && ret != -EOPNOTSUPP) 
	return ret;

Thanks,
Peng
>
>-- 
>Regards,
>Sudeep

Re: [PATCH v3 5/6] firmware: arm_scmi: imx: Support getting syslog of MISC protocol

[Peng Fan]

On Fri, Aug 29, 2025 at 12:07:22PM +0100, Sudeep Holla wrote:
>On Wed, Aug 27, 2025 at 12:59:17PM +0800, Peng Fan wrote:
>> MISC protocol supports getting system log regarding system sleep latency
>> ,wakeup interrupt and etc. Add the API for user to retrieve the
>> information from SM.
>> 
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>  .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 82 ++++++++++++++++++++++
>>  include/linux/scmi_imx_protocol.h                  | 19 +++++
>>  2 files changed, 101 insertions(+)
>> 
>> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> index f934b4fbc6ec9f1e6b24d1c6c8cd07b45ce548e3..2d3423d83aed857329a9a367d0ec0681a1d77d0b 100644
>> --- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> @@ -27,6 +27,7 @@ enum scmi_imx_misc_protocol_cmd {
>>  	SCMI_IMX_MISC_CTRL_GET	= 0x4,
>>  	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
>>  	SCMI_IMX_MISC_CFG_INFO = 0xC,
>> +	SCMI_IMX_MISC_SYSLOG = 0xD,
>
>1. Not ordered
>2. Inconsistent command name with the document.

Fix in V4.

>
>> +	*(p->size) = st->num_returned + st->num_remaining;
>
>I think you can drop () above.

Sure. Fix in V4.

>
>> +
>> +	return 0;
>> +}
>> +
>> +static int
>> +iter_misc_syslog_process_response(const struct scmi_protocol_handle *ph,
>> +				  const void *response,
>> +				  struct scmi_iterator_state *st, void *priv)
>> +{
>> +	const struct scmi_imx_misc_syslog_out *r = response;
>> +	struct scmi_imx_misc_syslog_ipriv *p = priv;
>> +
>> +	p->array[st->desc_index + st->loop_idx] =
>> +		le32_to_cpu(r->syslog[st->loop_idx]);
>> +
>> +	return 0;
>> +}
>> +
>> +static int scmi_imx_misc_syslog(const struct scmi_protocol_handle *ph, u16 *size,
>> +				void *array)
>> +{
>> +	struct scmi_iterator_ops ops = {
>> +		.prepare_message = iter_misc_syslog_prepare_message,
>> +		.update_state = iter_misc_syslog_update_state,
>> +		.process_response = iter_misc_syslog_process_response,
>> +	};
>> +	struct scmi_imx_misc_syslog_ipriv ipriv = {
>> +		.array = array,
>> +		.size = size,
>> +	};
>> +	void *iter;
>> +
>> +	if (!array || !size || !*size)
>> +		return -EINVAL;
>> +
>> +	iter = ph->hops->iter_response_init(ph, &ops, *size, SCMI_IMX_MISC_SYSLOG,
>> +					    sizeof(struct scmi_imx_misc_syslog_in),
>> +					    &ipriv);
>> +	if (IS_ERR(iter))
>> +		return PTR_ERR(iter);
>> +
>
>Handle NOT_SUPPORTED if not mandatory, may need update to the document to
>add NOT_SUPPORTED as return value. Currently it's only success in which
>case you don't need any error handling at all ????.

Current imx-sm firmware implemented this API, but indeed it is optional, need
to handle NOT_SUPPORTED.

>
>> +	return ph->hops->iter_response_run(iter);
>> +}
>> +	uint32_t deverrlog;
>
>s/uint32_t/u32/ for consistency ? Just look above 3-4 line e.g.

Fix in V4

Thanks,
Peng

>
>-- 
>Regards,
>Sudeep