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

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

[Peng Fan]

Cristian,
  I only kept your R-b for patch 1, dropped your other R-b tags, because
  there are some changes to address the comments from Sudeep. Most changes
  are only to dump the information in probe phase, update to single sysfs
  single value for syslog.

Sudeep,
  I saw you sent out PR to Arnd for 6.17, so no rush now for this patchset.
  Take your time.

Hope I not miss any comments from V1.

Thanks,
Peng

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 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    | 190 +++++++++++++++++++++
 drivers/firmware/arm_scmi/vendors/imx/imx95.rst    |  22 +++
 drivers/firmware/imx/sm-misc.c                     | 178 +++++++++++++++++++
 include/linux/scmi_imx_protocol.h                  |  19 +++
 4 files changed, 409 insertions(+)
---
base-commit: e88d17c7335a917e98b7c65776cbe3358210e8ab
change-id: 20250627-sm-misc-api-v1-85c030c670c6

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

[PATCH v2 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 v2 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.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 42 ++++++++++++++++++++++
 1 file changed, 42 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..b301f09ab6cc65d286edb8819031d7f4c02e8272 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,34 @@ 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)
+{
+	struct scmi_imx_misc_buildinfo_out *out;
+	char date[MISC_MAX_BUILDDATE] = {'\0'};
+	char time[MISC_MAX_BUILDTIME] = {'\0'};
+	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 +337,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 v2 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.

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 b301f09ab6cc65d286edb8819031d7f4c02e8272..dc6ea67255b8b1ac95dfb552fa88b65044de8d3f 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)
 {
@@ -310,6 +317,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] = {'\0'};
+	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,
@@ -341,6 +372,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 v2 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.

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 dc6ea67255b8b1ac95dfb552fa88b65044de8d3f..6b86c35c192d02e13f0d2a7d713bc447886b84bf 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
@@ -317,6 +324,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] = {'\0'};
+	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;
@@ -372,6 +403,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 v2 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    | 78 ++++++++++++++++++++++
 include/linux/scmi_imx_protocol.h                  | 19 ++++++
 2 files changed, 97 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 6b86c35c192d02e13f0d2a7d713bc447886b84bf..193a862cf9b807232f04a6dbbd6a8efd1b40ff73 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)
 {
@@ -372,10 +386,74 @@ static int scmi_imx_misc_cfg_info(const struct scmi_protocol_handle *ph)
 	return ret;
 }
 
+struct scmi_imx_misc_syslog_ipriv {
+	u32 *array;
+};
+
+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;
+
+	st->num_returned = RETURNED(r->numlogflags);
+	st->num_remaining = REMAINING(r->numlogflags);
+
+	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,
+	};
+	void *iter;
+
+	if (!array || !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..4283f09d7185e980c4fb18aadcba16f64cb341a4 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 v2 6/6] firmware: imx: sm-misc: Dump syslog info

[Peng Fan]

Add sysfs interface to read System Manager syslog info

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/imx/sm-misc.c | 178 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 178 insertions(+)

diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
index fc3ee12c2be878e0285183e3381c9514a63d5142..10aab5b127f65ecc57db9ab6c0c2c2b84d1dd864 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,178 @@ 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);
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
+	if (ret)
+		return ret;
+
+	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);
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
+	if (ret)
+		return ret;
+
+	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);
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
+	if (ret)
+		return ret;
+
+	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);
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
+	if (ret)
+		return ret;
+
+	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);
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
+	if (ret)
+		return ret;
+
+	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);
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
+	if (ret)
+		return ret;
+
+	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);
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
+	if (ret)
+		return ret;
+
+	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);
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
+	if (ret)
+		return ret;
+
+	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);
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
+	if (ret)
+		return ret;
+
+	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 +232,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 +283,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 v2 0/6] firmware: arm_scmi: imx: Dump syslog and system_info

[Peng Fan]

Hi Sudeep, Cristian

On Thu, Jul 10, 2025 at 04:33:25PM +0800, Peng Fan wrote:
>Cristian,
>  I only kept your R-b for patch 1, dropped your other R-b tags, because
>  there are some changes to address the comments from Sudeep. Most changes
>  are only to dump the information in probe phase, update to single sysfs
>  single value for syslog.
>
>Sudeep,
>  I saw you sent out PR to Arnd for 6.17, so no rush now for this patchset.
>  Take your time.

This patchset was originally submitted before v6.16-rc6. As the kernel has
now reached v6.17-rc2, I would greatly appreciate it if you could take some
time to review it when convenient. Thank you in advance!

Thanks,
Peng

>
>Hope I not miss any comments from V1.
>
>Thanks,
>Peng
>
>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 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    | 190 +++++++++++++++++++++
> drivers/firmware/arm_scmi/vendors/imx/imx95.rst    |  22 +++
> drivers/firmware/imx/sm-misc.c                     | 178 +++++++++++++++++++
> include/linux/scmi_imx_protocol.h                  |  19 +++
> 4 files changed, 409 insertions(+)
>---
>base-commit: e88d17c7335a917e98b7c65776cbe3358210e8ab
>change-id: 20250627-sm-misc-api-v1-85c030c670c6
>
>Best regards,
>-- 
>Peng Fan <peng.fan@nxp.com>
>

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

[Cristian Marussi]

On Thu, Jul 10, 2025 at 04:33:27PM +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.

Hi,

> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 42 ++++++++++++++++++++++
>  1 file changed, 42 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..b301f09ab6cc65d286edb8819031d7f4c02e8272 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,34 @@ 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)
> +{
> +	struct scmi_imx_misc_buildinfo_out *out;
> +	char date[MISC_MAX_BUILDDATE] = {'\0'};
> +	char time[MISC_MAX_BUILDTIME] = {'\0'};

...dont think that these 2 array needs to be NULL terminated given that
they are used as the dest buffer for strscpy() which always terminates
the dest buffer returning always a valid string.


> +	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);
> +	}
> +

...other than this...LGTM.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

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

[Cristian Marussi]

On Thu, Jul 10, 2025 at 04:33:28PM +0800, Peng Fan wrote:
> MISC protocol supports getting the System Manager(SM) mode selection
> and configuration name. Retrieve the information from SM.
> 
> 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 b301f09ab6cc65d286edb8819031d7f4c02e8272..dc6ea67255b8b1ac95dfb552fa88b65044de8d3f 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)
>  {
> @@ -310,6 +317,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] = {'\0'};

Same considerations here...

> +	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));

...other than this...LGTM.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

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

[Cristian Marussi]

On Thu, Jul 10, 2025 at 04:33:29PM +0800, Peng Fan wrote:
> MISC protocol supports getting board information. Retrieve the information
> from SM.
> 
> 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 dc6ea67255b8b1ac95dfb552fa88b65044de8d3f..6b86c35c192d02e13f0d2a7d713bc447886b84bf 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
> @@ -317,6 +324,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] = {'\0'};

same...

> +	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));
> +	}
> +

...other than this...LGTM.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

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

[Cristian Marussi]

On Thu, Jul 10, 2025 at 04:33:30PM +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    | 78 ++++++++++++++++++++++
>  include/linux/scmi_imx_protocol.h                  | 19 ++++++
>  2 files changed, 97 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 6b86c35c192d02e13f0d2a7d713bc447886b84bf..193a862cf9b807232f04a6dbbd6a8efd1b40ff73 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];
>  };

Hi,

one consideration down below...

>  
> +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)
>  {
> @@ -372,10 +386,74 @@ static int scmi_imx_misc_cfg_info(const struct scmi_protocol_handle *ph)
>  	return ret;
>  }
>  
> +struct scmi_imx_misc_syslog_ipriv {
> +	u32 *array;
> +};
> +

So, AFAIU, you basically use this generic u32 array to retrieve data words from
your FW in a generic way....

> +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;
> +
> +	st->num_returned = RETURNED(r->numlogflags);
> +	st->num_remaining = REMAINING(r->numlogflags);
> +
> +	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)

...and you provide a output array param and its size to use in the
iterators to retrieve your data...

...so you can use size to properly let iterators check bounds...

...so far so good BUT...

...is it not a possibility, especially with different FW versions in the
future, that the platform will return LESS than size data, because maybe
different platform can return different log data...I deduce this from
the fact that you are using a generic u32 array...

...so in this scenario wouldn't be useful to have the above size param as
being both an input and an output parameter using a pointer ? 

So that you can...

	static int scmi_imx_misc_syslog(const struct scmi_protocol_handle *ph,
					u16 *size, void *array)

...use *size as the max_resources for iterators as of now BUT also pass
it down to the iterators in ipriv->size so that you can easily once for
all in prepare_message

	*(ipriv->size) = st->num_returned + st->num_remaining;

...so that you can KNOW if the specific FW has returned less items than
the maximum *size slots provided in *array ?

I maybe overthinking...and also this scenario will assume that the FW
can return less items, BUT still contiguos items...i.e. no holes in the
array....

...thoughts ?

Thanks,
Cristian

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

[Cristian Marussi]

On Thu, Jul 10, 2025 at 04:33:31PM +0800, Peng Fan wrote:
> Add sysfs interface to read System Manager syslog info
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/firmware/imx/sm-misc.c | 178 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 178 insertions(+)
> 
> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
> index fc3ee12c2be878e0285183e3381c9514a63d5142..10aab5b127f65ecc57db9ab6c0c2c2b84d1dd864 100644
> --- a/drivers/firmware/imx/sm-misc.c
> +++ b/drivers/firmware/imx/sm-misc.c
> @@ -3,6 +3,7 @@
>   * Copyright 2024 NXP
>   */
>  

Hi,

I have not so much to say about this: you expose one value per-sysfs
entry as prescribe BUT this also means a lot of messages to exchage to
retrieve a whole new fresh log each time a single sysfs entry is
queried...a bit ugly but I wouldn't now how to handle this differently
with the current log command format indeed...

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

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

[Peng Fan]

Hi Cristian,

On Tue, Aug 19, 2025 at 06:09:52PM +0100, Cristian Marussi wrote:
>On Thu, Jul 10, 2025 at 04:33:30PM +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    | 78 ++++++++++++++++++++++
>>  include/linux/scmi_imx_protocol.h                  | 19 ++++++
>>  2 files changed, 97 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 6b86c35c192d02e13f0d2a7d713bc447886b84bf..193a862cf9b807232f04a6dbbd6a8efd1b40ff73 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];
>>  };
>
>Hi,
>
>one consideration down below...
>
>>  
>> +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)
>>  {
>> @@ -372,10 +386,74 @@ static int scmi_imx_misc_cfg_info(const struct scmi_protocol_handle *ph)
>>  	return ret;
>>  }
>>  
>> +struct scmi_imx_misc_syslog_ipriv {
>> +	u32 *array;
>> +};
>> +
>
>So, AFAIU, you basically use this generic u32 array to retrieve data words from
>your FW in a generic way....
>
>> +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;
>> +
>> +	st->num_returned = RETURNED(r->numlogflags);
>> +	st->num_remaining = REMAINING(r->numlogflags);
>> +
>> +	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)
>
>...and you provide a output array param and its size to use in the
>iterators to retrieve your data...
>
>...so you can use size to properly let iterators check bounds...
>
>...so far so good BUT...
>
>...is it not a possibility, especially with different FW versions in the
>future, that the platform will return LESS than size data, because maybe
>different platform can return different log data...I deduce this from
>the fact that you are using a generic u32 array...
>
>...so in this scenario wouldn't be useful to have the above size param as
>being both an input and an output parameter using a pointer ? 
>
>So that you can...
>
>	static int scmi_imx_misc_syslog(const struct scmi_protocol_handle *ph,
>					u16 *size, void *array)
>
>...use *size as the max_resources for iterators as of now BUT also pass
>it down to the iterators in ipriv->size so that you can easily once for
>all in prepare_message
>
>	*(ipriv->size) = st->num_returned + st->num_remaining;
>
>...so that you can KNOW if the specific FW has returned less items than
>the maximum *size slots provided in *array ?
>
>I maybe overthinking...and also this scenario will assume that the FW
>can return less items, BUT still contiguos items...i.e. no holes in the
>array....
>
>...thoughts ?

The current platforms that deployed i.MX System Manager firmware use
a fixed log structure and return them all when input log index is 0.
But you bring a valid point that the firmware may report less in future
or new platforms. I will follow you suggestion and update in V3.

I would send out V3 in early next week. Wait to see if Sudeep has
other comments.

Thanks for reviewing the patchset.

Thanks,
Peng

>
>Thanks,
>Cristian