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

[PATCH 0/7] 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.

The output as below:
root@imx95evk:~# cat /sys/devices/platform/arm-scmi.0.auto/scmi_dev.14/syslog
Wake Vector = 36
Sys sleep mode = 0
Sys sleep flags = 0x00000000
MIX power status = 0x00030017
MEM power status = 0x00014400
PLL power status = 0x00000020
Sleep latency = 764
Wake latency = 3728
Sleep count = 1
root@imx95evk:~# cat /sys/devices/platform/arm-scmi.0.auto/scmi_dev.14/system_info
SM Version    = Build 646, Commit 08707569f4
SM Config     = mx95evkrpmsg, mSel=0
Silicon       = i.MX95 B0
Board         = i.MX95 EVK, attr=0x00000000

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Peng Fan (7):
      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 silicon info of MISC protocol
      firmware: arm_scmi: imx: Support getting syslog of MISC protocol
      firmware: arm_scmi: imx: Support getting board info of MISC protocol
      firmware: imx: sm-misc: Dump syslog and system info

 .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 207 +++++++++++++++++++++
 drivers/firmware/arm_scmi/vendors/imx/imx95.rst    |  20 ++
 drivers/firmware/imx/sm-misc.c                     |  97 ++++++++++
 include/linux/scmi_imx_protocol.h                  |  49 +++++
 4 files changed, 373 insertions(+)
---
base-commit: ecb259c4f70dd5c83907809f45bf4dc6869961d7
change-id: 20250627-sm-misc-api-v1-85c030c670c6

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

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

[Peng Fan]

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

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

diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
index 4e246a78a042a79eb81be35632079c7626bbbe57..ac82da0d1e5ce5fa65a5771286aaebb748c8a4e6 100644
--- a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
+++ b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
@@ -1670,6 +1670,26 @@ 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                                |
++--------------------+---------------------------------------------------------+
+|uint8 boardname[16] |Board name. Null terminated ASCII string of up           |
+|                    |to 16 bytes in length                                    |
++--------------------+---------------------------------------------------------+
+
 NEGOTIATE_PROTOCOL_VERSION
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 

-- 
2.37.1

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

[Cristian Marussi]

On Fri, Jun 27, 2025 at 02:03:44PM +0800, Peng Fan wrote:
> System Manager Firmware supports getting board information, add
> documentation for this API
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/firmware/arm_scmi/vendors/imx/imx95.rst | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
> index 4e246a78a042a79eb81be35632079c7626bbbe57..ac82da0d1e5ce5fa65a5771286aaebb748c8a4e6 100644
> --- a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
> @@ -1670,6 +1670,26 @@ 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                                |

..what's in here ?

> ++--------------------+---------------------------------------------------------+
> +|uint8 boardname[16] |Board name. Null terminated ASCII string of up           |
> +|                    |to 16 bytes in length                                    |
> ++--------------------+---------------------------------------------------------+
> +
>  NEGOTIATE_PROTOCOL_VERSION
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~
>

..other than this, LGTM.
  
Reviewed;by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

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

[Peng Fan]

On Fri, Jun 27, 2025 at 01:46:03PM +0100, Cristian Marussi wrote:
>On Fri, Jun 27, 2025 at 02:03:44PM +0800, Peng Fan wrote:
>> System Manager Firmware supports getting board information, add
>> documentation for this API
>> 
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>  drivers/firmware/arm_scmi/vendors/imx/imx95.rst | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>> 
>> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
>> index 4e246a78a042a79eb81be35632079c7626bbbe57..ac82da0d1e5ce5fa65a5771286aaebb748c8a4e6 100644
>> --- a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
>> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
>> @@ -1670,6 +1670,26 @@ 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                                |
>
>..what's in here ?

It is 0 as of now, per my understanding, it could be to any value that
board owner wanna. Anyway, I need check with our Firmware owners, then
update you.

>
>> ++--------------------+---------------------------------------------------------+
>> +|uint8 boardname[16] |Board name. Null terminated ASCII string of up           |
>> +|                    |to 16 bytes in length                                    |
>> ++--------------------+---------------------------------------------------------+
>> +
>>  NEGOTIATE_PROTOCOL_VERSION
>>  ~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>
>..other than this, LGTM.
>  
>Reviewed;by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Peng

>
>Thanks,
>Cristian

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

[Peng Fan]

On Mon, Jun 30, 2025 at 10:45:58AM +0800, Peng Fan wrote:
>On Fri, Jun 27, 2025 at 01:46:03PM +0100, Cristian Marussi wrote:
>>On Fri, Jun 27, 2025 at 02:03:44PM +0800, Peng Fan wrote:
>>> System Manager Firmware supports getting board information, add
>>> documentation for this API
>>> 
>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>> ---
>>>  drivers/firmware/arm_scmi/vendors/imx/imx95.rst | 20 ++++++++++++++++++++
>>>  1 file changed, 20 insertions(+)
>>> 
>>> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
>>> index 4e246a78a042a79eb81be35632079c7626bbbe57..ac82da0d1e5ce5fa65a5771286aaebb748c8a4e6 100644
>>> --- a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
>>> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
>>> @@ -1670,6 +1670,26 @@ 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                                |
>>
>>..what's in here ?
>
>It is 0 as of now, per my understanding, it could be to any value that
>board owner wanna. Anyway, I need check with our Firmware owners, then
>update you.

It is for future expansion without breaking backwards compatibility.

I could add the information
"
 Board specific attributes(reserved for future
 expansion without breaking backwards compatibility)
"

Thanks,
Peng

>
>>
>>> ++--------------------+---------------------------------------------------------+
>>> +|uint8 boardname[16] |Board name. Null terminated ASCII string of up           |
>>> +|                    |to 16 bytes in length                                    |
>>> ++--------------------+---------------------------------------------------------+
>>> +
>>>  NEGOTIATE_PROTOCOL_VERSION
>>>  ~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>
>>..other than this, LGTM.
>>  
>>Reviewed;by: Cristian Marussi <cristian.marussi@arm.com>
>
>Thanks,
>Peng
>
>>
>>Thanks,
>>Cristian

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

[Sudeep Holla]

On Fri, Jun 27, 2025 at 02:03:44PM +0800, Peng Fan wrote:
> System Manager Firmware supports getting board information, add
> documentation for this API
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/firmware/arm_scmi/vendors/imx/imx95.rst | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
> index 4e246a78a042a79eb81be35632079c7626bbbe57..ac82da0d1e5ce5fa65a5771286aaebb748c8a4e6 100644
> --- a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
> @@ -1670,6 +1670,26 @@ 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                                |

As suggested, please add current definition of the values.

> ++--------------------+---------------------------------------------------------+
> +|uint8 boardname[16] |Board name. Null terminated ASCII string of up           |
> +|                    |to 16 bytes in length                                    |

How does this match with the information from the DT ? Will they be in sync ?
I understand NXP being silicon vendor, wants to have vendor extensions. But
any board information comes from the OEMs/ODMs. Do you expect them to change
their SCMI firmware. That's not the general expectation, so I am bit puzzled
on this whole BOARD_INFO interface. Please help me understand the motivation
for this new interface.

-- 
Regards,
Sudeep

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

[Peng Fan]

On Wed, Jul 02, 2025 at 04:21:24PM +0100, Sudeep Holla wrote:
>On Fri, Jun 27, 2025 at 02:03:44PM +0800, Peng Fan wrote:
>> System Manager Firmware supports getting board information, add
>> documentation for this API
>> 
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>  drivers/firmware/arm_scmi/vendors/imx/imx95.rst | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>> 
>> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
>> index 4e246a78a042a79eb81be35632079c7626bbbe57..ac82da0d1e5ce5fa65a5771286aaebb748c8a4e6 100644
>> --- a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
>> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
>> @@ -1670,6 +1670,26 @@ 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                                |
>
>As suggested, please add current definition of the values.

Per our firmware owner:
Future expansion without breaking backwards compatibility
(message size and parameters).


>
>> ++--------------------+---------------------------------------------------------+
>> +|uint8 boardname[16] |Board name. Null terminated ASCII string of up           |
>> +|                    |to 16 bytes in length                                    |
>
>How does this match with the information from the DT ? Will they be in sync ?
>I understand NXP being silicon vendor, wants to have vendor extensions. But
>any board information comes from the OEMs/ODMs. Do you expect them to change
>their SCMI firmware. That's not the general expectation, so I am bit puzzled
>on this whole BOARD_INFO interface. Please help me understand the motivation
>for this new interface.

For example, i.MX95 has two die size, 19x19 and 15x15. With the two variants,
there are two boards: i.MX95-19x19-EVK, i.MX95-15x15-EVK.

However we use one System Manager firmware binary to support both boards,
so System Manager reports board name "i.MX95 EVK". But in dts,
we use "fsl,imx95-19x19-evk" and "fsl,imx95-15x15-evk". So they are
different.

Our System Manager firmware is public and open-source, OEM/ODMs could
update the code as what they wanna, including board name.

Loop our firmware owner here, in case any he could help complement.

Thanks,
Peng

>
>-- 
>Regards,
>Sudeep

[PATCH 2/7] 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. 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    | 35 ++++++++++++++++++++++
 include/linux/scmi_imx_protocol.h                  | 12 ++++++++
 2 files changed, 47 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..1b24d070c6f4856b92f515fcdba5836fd6498ce6 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,13 @@ struct scmi_imx_misc_ctrl_get_out {
 	__le32 val[];
 };
 
+struct scmi_imx_misc_buildinfo_out {
+	__le32 buildnum;
+	__le32 buildcommit;
+	u8 builddate[MISC_MAX_BUILDDATE];
+	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,10 +280,37 @@ static int scmi_imx_misc_ctrl_set(const struct scmi_protocol_handle *ph,
 	return ret;
 }
 
+static int scmi_imx_discover_build_info(const struct scmi_protocol_handle *ph,
+					struct scmi_imx_misc_system_info *info)
+{
+	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;
+		info->buildnum = le32_to_cpu(out->buildnum);
+		info->buildcommit = le32_to_cpu(out->buildcommit);
+		strscpy(info->date, out->builddate, MISC_MAX_BUILDDATE);
+		strscpy(info->time, out->buildtime, MISC_MAX_BUILDTIME);
+	}
+
+	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,
 	.misc_ctrl_req_notify = scmi_imx_misc_ctrl_notify,
+	.misc_discover_build_info = scmi_imx_discover_build_info,
 };
 
 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..826402dfe6f4d3b9e6d2e93868d6699f989e9bcc 100644
--- a/include/linux/scmi_imx_protocol.h
+++ b/include/linux/scmi_imx_protocol.h
@@ -52,6 +52,16 @@ struct scmi_imx_misc_ctrl_notify_report {
 	unsigned int		flags;
 };
 
+#define MISC_MAX_BUILDDATE	16
+#define MISC_MAX_BUILDTIME	16
+
+struct scmi_imx_misc_system_info {
+	u32 buildnum;
+	u32 buildcommit;
+	u8 date[MISC_MAX_BUILDDATE];
+	u8 time[MISC_MAX_BUILDTIME];
+};
+
 struct scmi_imx_misc_proto_ops {
 	int (*misc_ctrl_set)(const struct scmi_protocol_handle *ph, u32 id,
 			     u32 num, u32 *val);
@@ -59,6 +69,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_discover_build_info)(const struct scmi_protocol_handle *ph,
+					struct scmi_imx_misc_system_info *info);
 };
 
 /* See LMM_ATTRIBUTES in imx95.rst */

-- 
2.37.1

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

[Cristian Marussi]

On Fri, Jun 27, 2025 at 02:03:45PM +0800, Peng Fan wrote:
> MISC protocol supports discovering the System Manager(SM) build
> information including build commit, build time and etc. Add the API
> for user to retrieve the information from SM.
> 

Hi,

> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 35 ++++++++++++++++++++++
>  include/linux/scmi_imx_protocol.h                  | 12 ++++++++
>  2 files changed, 47 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..1b24d070c6f4856b92f515fcdba5836fd6498ce6 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,13 @@ struct scmi_imx_misc_ctrl_get_out {
>  	__le32 val[];
>  };
>  
> +struct scmi_imx_misc_buildinfo_out {
> +	__le32 buildnum;
> +	__le32 buildcommit;
> +	u8 builddate[MISC_MAX_BUILDDATE];
> +	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,10 +280,37 @@ static int scmi_imx_misc_ctrl_set(const struct scmi_protocol_handle *ph,
>  	return ret;
>  }
>  
> +static int scmi_imx_discover_build_info(const struct scmi_protocol_handle *ph,
> +					struct scmi_imx_misc_system_info *info)
> +{
> +	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;
> +		info->buildnum = le32_to_cpu(out->buildnum);
> +		info->buildcommit = le32_to_cpu(out->buildcommit);
> +		strscpy(info->date, out->builddate, MISC_MAX_BUILDDATE);
> +		strscpy(info->time, out->buildtime, MISC_MAX_BUILDTIME);
> +	}
> +
> +	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,
>  	.misc_ctrl_req_notify = scmi_imx_misc_ctrl_notify,
> +	.misc_discover_build_info = scmi_imx_discover_build_info,
>  };
>  
>  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..826402dfe6f4d3b9e6d2e93868d6699f989e9bcc 100644
> --- a/include/linux/scmi_imx_protocol.h
> +++ b/include/linux/scmi_imx_protocol.h
> @@ -52,6 +52,16 @@ struct scmi_imx_misc_ctrl_notify_report {
>  	unsigned int		flags;
>  };
>  
> +#define MISC_MAX_BUILDDATE	16
> +#define MISC_MAX_BUILDTIME	16
> +
> +struct scmi_imx_misc_system_info {
> +	u32 buildnum;
> +	u32 buildcommit;
> +	u8 date[MISC_MAX_BUILDDATE];
> +	u8 time[MISC_MAX_BUILDTIME];
> +};
> +
>  struct scmi_imx_misc_proto_ops {
>  	int (*misc_ctrl_set)(const struct scmi_protocol_handle *ph, u32 id,
>  			     u32 num, u32 *val);
> @@ -59,6 +69,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_discover_build_info)(const struct scmi_protocol_handle *ph,
> +					struct scmi_imx_misc_system_info *info);
>  };

LGTM.

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

Thanks,
Cristian

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

[Sudeep Holla]

On Fri, Jun 27, 2025 at 02:03:45PM +0800, Peng Fan wrote:
> MISC protocol supports discovering the System Manager(SM) build
> information including build commit, build time 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    | 35 ++++++++++++++++++++++
>  include/linux/scmi_imx_protocol.h                  | 12 ++++++++
>  2 files changed, 47 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..1b24d070c6f4856b92f515fcdba5836fd6498ce6 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,

I clearly missed to raise this point when the documentation for this command
was added. Anyways I assume, you had explored all the options before adding
this as generic tools may not be able to pick this up. Instead, I would have
just stuck with vendor version in the standard protocol with build number
embedded into it. The date and other info must be implicit from the build.

I try to be more cautious and ask questions in the future as I don't want
vendor extensions to be dumping ground for really random things like this.

-- 
Regards,
Sudeep

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

[Peng Fan]

Hi Sudeep,

On Wed, Jul 02, 2025 at 04:21:40PM +0100, Sudeep Holla wrote:
>On Fri, Jun 27, 2025 at 02:03:45PM +0800, Peng Fan wrote:
>> MISC protocol supports discovering the System Manager(SM) build
>> information including build commit, build time 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    | 35 ++++++++++++++++++++++
>>  include/linux/scmi_imx_protocol.h                  | 12 ++++++++
>>  2 files changed, 47 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..1b24d070c6f4856b92f515fcdba5836fd6498ce6 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,
>
>I clearly missed to raise this point when the documentation for this command
>was added. Anyways I assume, you had explored all the options before adding
>this as generic tools may not be able to pick this up. Instead, I would have
>just stuck with vendor version in the standard protocol with build number
>embedded into it. The date and other info must be implicit from the build.
>
>I try to be more cautious and ask questions in the future as I don't want
>vendor extensions to be dumping ground for really random things like this.

+Souvik

And Loop our firmware owner to help comment. I just add what the firmware
supports here and allow linux to see the information when the firmware
does not have uart output in some builds.

From SCMI spec, it does not restrict what vendor extensions should be like
as I know. So I am not sure what we should do when we define vendor
extensions and what I should do next for this patch.

Please suggest.

Thanks,
Peng

>
>-- 
>Regards,
>Sudeep

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

[Sudeep Holla]

On Fri, Jul 04, 2025 at 01:12:04PM +0800, Peng Fan wrote:
> Hi Sudeep,
> 
> On Wed, Jul 02, 2025 at 04:21:40PM +0100, Sudeep Holla wrote:
> >On Fri, Jun 27, 2025 at 02:03:45PM +0800, Peng Fan wrote:
> >> MISC protocol supports discovering the System Manager(SM) build
> >> information including build commit, build time 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    | 35 ++++++++++++++++++++++
> >>  include/linux/scmi_imx_protocol.h                  | 12 ++++++++
> >>  2 files changed, 47 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..1b24d070c6f4856b92f515fcdba5836fd6498ce6 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,
> >
> >I clearly missed to raise this point when the documentation for this command
> >was added. Anyways I assume, you had explored all the options before adding
> >this as generic tools may not be able to pick this up. Instead, I would have
> >just stuck with vendor version in the standard protocol with build number
> >embedded into it. The date and other info must be implicit from the build.
> >
> >I try to be more cautious and ask questions in the future as I don't want
> >vendor extensions to be dumping ground for really random things like this.
> 
> +Souvik
> 
> And Loop our firmware owner to help comment. I just add what the firmware
> supports here and allow linux to see the information when the firmware
> does not have uart output in some builds.
> 
> From SCMI spec, it does not restrict what vendor extensions should be like
> as I know. So I am not sure what we should do when we define vendor
> extensions and what I should do next for this patch.
> 

Just to be clear, I am not against vendor extensions. I am just saying
this interface is not strictly needed. The vendor version could encode
this nicely and you could have a map. The only and main concern is having
such extensions will not help generic tools as these are very vendor specific.

It is good to have firmware version and other related details in a standard
format that anyone can understand without the need to dig deeper into vendor
specific extensions.

Again I am not saying to drop these interfaces, but you will get questioned
for its use in the kernel if that doesn't seem like the right approach.

-- 
Regards,
Sudeep

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

[Peng Fan]

Hi Sudeep,

Sorry for late reply.
On Fri, Jul 04, 2025 at 09:59:16AM +0100, Sudeep Holla wrote:
>On Fri, Jul 04, 2025 at 01:12:04PM +0800, Peng Fan wrote:
>> Hi Sudeep,
>> 
>> On Wed, Jul 02, 2025 at 04:21:40PM +0100, Sudeep Holla wrote:
>> >On Fri, Jun 27, 2025 at 02:03:45PM +0800, Peng Fan wrote:
>> >> MISC protocol supports discovering the System Manager(SM) build
>> >> information including build commit, build time 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    | 35 ++++++++++++++++++++++
>> >>  include/linux/scmi_imx_protocol.h                  | 12 ++++++++
>> >>  2 files changed, 47 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..1b24d070c6f4856b92f515fcdba5836fd6498ce6 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,
>> >
>> >I clearly missed to raise this point when the documentation for this command
>> >was added. Anyways I assume, you had explored all the options before adding
>> >this as generic tools may not be able to pick this up. Instead, I would have
>> >just stuck with vendor version in the standard protocol with build number
>> >embedded into it. The date and other info must be implicit from the build.
>> >
>> >I try to be more cautious and ask questions in the future as I don't want
>> >vendor extensions to be dumping ground for really random things like this.
>> 
>> +Souvik
>> 
>> And Loop our firmware owner to help comment. I just add what the firmware
>> supports here and allow linux to see the information when the firmware
>> does not have uart output in some builds.
>> 
>> From SCMI spec, it does not restrict what vendor extensions should be like
>> as I know. So I am not sure what we should do when we define vendor
>> extensions and what I should do next for this patch.
>> 
>
>Just to be clear, I am not against vendor extensions. I am just saying
>this interface is not strictly needed. The vendor version could encode
>this nicely and you could have a map. The only and main concern is having
>such extensions will not help generic tools as these are very vendor specific.
>
>It is good to have firmware version and other related details in a standard
>format that anyone can understand without the need to dig deeper into vendor
>specific extensions.
>
>Again I am not saying to drop these interfaces, but you will get questioned
>for its use in the kernel if that doesn't seem like the right approach.

This is mostly for debug purpose to export the build information to linux,
such as firmware commit hash.

Should I still keep current patch, or do you have any suggestions
with current SM API?  I think there is little chance to update SM to encode
vendor version to include build date, build num, and commit hash.

Thanks,
Peng

>
>-- 
>Regards,
>Sudeep

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

[Sudeep Holla]

On Wed, Jul 09, 2025 at 12:10:06AM +0800, Peng Fan wrote:
> Hi Sudeep,
> 
> Sorry for late reply.
> On Fri, Jul 04, 2025 at 09:59:16AM +0100, Sudeep Holla wrote:
> >On Fri, Jul 04, 2025 at 01:12:04PM +0800, Peng Fan wrote:
> >> Hi Sudeep,
> >> 
> >> On Wed, Jul 02, 2025 at 04:21:40PM +0100, Sudeep Holla wrote:
> >> >On Fri, Jun 27, 2025 at 02:03:45PM +0800, Peng Fan wrote:
> >> >> MISC protocol supports discovering the System Manager(SM) build
> >> >> information including build commit, build time 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    | 35 ++++++++++++++++++++++
> >> >>  include/linux/scmi_imx_protocol.h                  | 12 ++++++++
> >> >>  2 files changed, 47 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..1b24d070c6f4856b92f515fcdba5836fd6498ce6 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,
> >> >
> >> >I clearly missed to raise this point when the documentation for this command
> >> >was added. Anyways I assume, you had explored all the options before adding
> >> >this as generic tools may not be able to pick this up. Instead, I would have
> >> >just stuck with vendor version in the standard protocol with build number
> >> >embedded into it. The date and other info must be implicit from the build.
> >> >
> >> >I try to be more cautious and ask questions in the future as I don't want
> >> >vendor extensions to be dumping ground for really random things like this.
> >> 
> >> +Souvik
> >> 
> >> And Loop our firmware owner to help comment. I just add what the firmware
> >> supports here and allow linux to see the information when the firmware
> >> does not have uart output in some builds.
> >> 
> >> From SCMI spec, it does not restrict what vendor extensions should be like
> >> as I know. So I am not sure what we should do when we define vendor
> >> extensions and what I should do next for this patch.
> >> 
> >
> >Just to be clear, I am not against vendor extensions. I am just saying
> >this interface is not strictly needed. The vendor version could encode
> >this nicely and you could have a map. The only and main concern is having
> >such extensions will not help generic tools as these are very vendor specific.
> >
> >It is good to have firmware version and other related details in a standard
> >format that anyone can understand without the need to dig deeper into vendor
> >specific extensions.
> >
> >Again I am not saying to drop these interfaces, but you will get questioned
> >for its use in the kernel if that doesn't seem like the right approach.
> 
> This is mostly for debug purpose to export the build information to linux,
> such as firmware commit hash.
> 
> Should I still keep current patch, or do you have any suggestions
> with current SM API?  I think there is little chance to update SM to encode
> vendor version to include build date, build num, and commit hash.
> 

I am against using this in the way you are doing in 7/7(i.e. exposing to
the userspace). Just dump the info on the serial port from the init code.
You don't need to expose it scmi_imx_misc_proto_ops as well.

-- 
Regards,
Sudeep

[PATCH 3/7] 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. 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    | 30 ++++++++++++++++++++++
 include/linux/scmi_imx_protocol.h                  |  5 ++++
 2 files 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 1b24d070c6f4856b92f515fcdba5836fd6498ce6..8ce4bf92e6535af2f30d72a34717678613b35049 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,
 };
 
@@ -73,6 +74,11 @@ struct scmi_imx_misc_buildinfo_out {
 	u8 buildtime[MISC_MAX_BUILDTIME];
 };
 
+struct scmi_imx_misc_cfg_info_out {
+	__le32 msel;
+	u8 cfgname[MISC_MAX_CFGNAME];
+};
+
 static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
 					struct scmi_imx_misc_info *mi)
 {
@@ -306,7 +312,31 @@ static int scmi_imx_discover_build_info(const struct scmi_protocol_handle *ph,
 	return ret;
 }
 
+static int scmi_imx_misc_cfg_info(const struct scmi_protocol_handle *ph,
+				  struct scmi_imx_misc_system_info *info)
+{
+	struct scmi_imx_misc_cfg_info_out *out;
+	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;
+		info->msel = le32_to_cpu(out->msel);
+		strscpy(info->cfgname, out->cfgname, MISC_MAX_CFGNAME);
+	}
+
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
 static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
+	.misc_cfg_info = scmi_imx_misc_cfg_info,
 	.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,
diff --git a/include/linux/scmi_imx_protocol.h b/include/linux/scmi_imx_protocol.h
index 826402dfe6f4d3b9e6d2e93868d6699f989e9bcc..bb0c35b5d6705acddd6c83c31474482a2667b418 100644
--- a/include/linux/scmi_imx_protocol.h
+++ b/include/linux/scmi_imx_protocol.h
@@ -54,15 +54,20 @@ struct scmi_imx_misc_ctrl_notify_report {
 
 #define MISC_MAX_BUILDDATE	16
 #define MISC_MAX_BUILDTIME	16
+#define MISC_MAX_CFGNAME	16
 
 struct scmi_imx_misc_system_info {
 	u32 buildnum;
 	u32 buildcommit;
 	u8 date[MISC_MAX_BUILDDATE];
 	u8 time[MISC_MAX_BUILDTIME];
+	u32 msel;
+	u8 cfgname[MISC_MAX_CFGNAME];
 };
 
 struct scmi_imx_misc_proto_ops {
+	int (*misc_cfg_info)(const struct scmi_protocol_handle *ph,
+			     struct scmi_imx_misc_system_info *info);
 	int (*misc_ctrl_set)(const struct scmi_protocol_handle *ph, u32 id,
 			     u32 num, u32 *val);
 	int (*misc_ctrl_get)(const struct scmi_protocol_handle *ph, u32 id,

-- 
2.37.1

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

[Cristian Marussi]

On Fri, Jun 27, 2025 at 02:03:46PM +0800, Peng Fan wrote:
> MISC protocol supports getting the System Manager(SM) mode selection
> and configuration name. 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    | 30 ++++++++++++++++++++++
>  include/linux/scmi_imx_protocol.h                  |  5 ++++
>  2 files 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 1b24d070c6f4856b92f515fcdba5836fd6498ce6..8ce4bf92e6535af2f30d72a34717678613b35049 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,
>  };
>  
> @@ -73,6 +74,11 @@ struct scmi_imx_misc_buildinfo_out {
>  	u8 buildtime[MISC_MAX_BUILDTIME];
>  };
>  
> +struct scmi_imx_misc_cfg_info_out {
> +	__le32 msel;
> +	u8 cfgname[MISC_MAX_CFGNAME];
> +};
> +
>  static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
>  					struct scmi_imx_misc_info *mi)
>  {
> @@ -306,7 +312,31 @@ static int scmi_imx_discover_build_info(const struct scmi_protocol_handle *ph,
>  	return ret;
>  }
>  
> +static int scmi_imx_misc_cfg_info(const struct scmi_protocol_handle *ph,
> +				  struct scmi_imx_misc_system_info *info)
> +{
> +	struct scmi_imx_misc_cfg_info_out *out;
> +	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;
> +		info->msel = le32_to_cpu(out->msel);
> +		strscpy(info->cfgname, out->cfgname, MISC_MAX_CFGNAME);
> +	}
> +
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
>  static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
> +	.misc_cfg_info = scmi_imx_misc_cfg_info,
>  	.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,
> diff --git a/include/linux/scmi_imx_protocol.h b/include/linux/scmi_imx_protocol.h
> index 826402dfe6f4d3b9e6d2e93868d6699f989e9bcc..bb0c35b5d6705acddd6c83c31474482a2667b418 100644
> --- a/include/linux/scmi_imx_protocol.h
> +++ b/include/linux/scmi_imx_protocol.h
> @@ -54,15 +54,20 @@ struct scmi_imx_misc_ctrl_notify_report {
>  
>  #define MISC_MAX_BUILDDATE	16
>  #define MISC_MAX_BUILDTIME	16
> +#define MISC_MAX_CFGNAME	16
>  
>  struct scmi_imx_misc_system_info {
>  	u32 buildnum;
>  	u32 buildcommit;
>  	u8 date[MISC_MAX_BUILDDATE];
>  	u8 time[MISC_MAX_BUILDTIME];
> +	u32 msel;
> +	u8 cfgname[MISC_MAX_CFGNAME];
>  };
>

Bit odd that you use the same struct partially as output of one ops
and partially as outout of this ops....but indeed the 2 sets of data
have different lifetimes, with one set not changing at all after the
first call durring the same boot.... so I suppose it will be up to the
caller not to mess up stuff.  

maybe you could embed 2 different structures colelcting those different
data and then pass the pointers to such internal structs to the
caller...

>  struct scmi_imx_misc_proto_ops {
> +	int (*misc_cfg_info)(const struct scmi_protocol_handle *ph,
> +			     struct scmi_imx_misc_system_info *info);
>  	int (*misc_ctrl_set)(const struct scmi_protocol_handle *ph, u32 id,
>  			     u32 num, u32 *val);
>  	int (*misc_ctrl_get)(const struct scmi_protocol_handle *ph, u32 id,
> 

Anyway, LGTM.

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

Thanks,
Cristian

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

[Sudeep Holla]

On Fri, Jun 27, 2025 at 02:03:46PM +0800, Peng Fan wrote:
> MISC protocol supports getting the System Manager(SM) mode selection
> and configuration name. 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    | 30 ++++++++++++++++++++++
>  include/linux/scmi_imx_protocol.h                  |  5 ++++
>  2 files 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 1b24d070c6f4856b92f515fcdba5836fd6498ce6..8ce4bf92e6535af2f30d72a34717678613b35049 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,
>  };
>  
> @@ -73,6 +74,11 @@ struct scmi_imx_misc_buildinfo_out {
>  	u8 buildtime[MISC_MAX_BUILDTIME];
>  };
>  
> +struct scmi_imx_misc_cfg_info_out {
> +	__le32 msel;

Now, I realise, this mode select is not properly defined in the document.
Just 32-bit word. What are those values ? Any fixed list of values with
well defined modes or configurations ? If so, please add to the document.

-- 
Regards,
Sudeep

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

[Peng Fan]

On Wed, Jul 02, 2025 at 04:21:58PM +0100, Sudeep Holla wrote:
>On Fri, Jun 27, 2025 at 02:03:46PM +0800, Peng Fan wrote:
>> MISC protocol supports getting the System Manager(SM) mode selection
>> and configuration name. 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    | 30 ++++++++++++++++++++++
>>  include/linux/scmi_imx_protocol.h                  |  5 ++++
>>  2 files 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 1b24d070c6f4856b92f515fcdba5836fd6498ce6..8ce4bf92e6535af2f30d72a34717678613b35049 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,
>>  };
>>  
>> @@ -73,6 +74,11 @@ struct scmi_imx_misc_buildinfo_out {
>>  	u8 buildtime[MISC_MAX_BUILDTIME];
>>  };
>>  
>> +struct scmi_imx_misc_cfg_info_out {
>> +	__le32 msel;
>
>Now, I realise, this mode select is not properly defined in the document.
>Just 32-bit word. What are those values ? Any fixed list of values with
>well defined modes or configurations ? If so, please add to the document.

The current used value are 0,1,2. It is used to decide the logic machine
boot order, such as 0 means booting LM0, LM1, LM2, LM3...
Regarding 0,1,2 using which LM order, it could be defined by user in
i.MX9 System Manger cfg file. That means 1 could mean LM0, LM2, LM3, LM1
or LM0, LM3, LM1, LM2.

I will update doc with below:
msel is used to decide Logical Machine boot order, it could vary per board,
because the order is defined in System Manager board configuration file.

Regards,
Peng

>
>-- 
>Regards,
>Sudeep

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

[Sudeep Holla]

On Fri, Jul 04, 2025 at 06:07:41PM +0800, Peng Fan wrote:
> On Wed, Jul 02, 2025 at 04:21:58PM +0100, Sudeep Holla wrote:
> >On Fri, Jun 27, 2025 at 02:03:46PM +0800, Peng Fan wrote:
> >> MISC protocol supports getting the System Manager(SM) mode selection
> >> and configuration name. 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    | 30 ++++++++++++++++++++++
> >>  include/linux/scmi_imx_protocol.h                  |  5 ++++
> >>  2 files 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 1b24d070c6f4856b92f515fcdba5836fd6498ce6..8ce4bf92e6535af2f30d72a34717678613b35049 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,
> >>  };
> >>  
> >> @@ -73,6 +74,11 @@ struct scmi_imx_misc_buildinfo_out {
> >>  	u8 buildtime[MISC_MAX_BUILDTIME];
> >>  };
> >>  
> >> +struct scmi_imx_misc_cfg_info_out {
> >> +	__le32 msel;
> >
> >Now, I realise, this mode select is not properly defined in the document.
> >Just 32-bit word. What are those values ? Any fixed list of values with
> >well defined modes or configurations ? If so, please add to the document.
> 
> The current used value are 0,1,2. It is used to decide the logic machine
> boot order, such as 0 means booting LM0, LM1, LM2, LM3...
> Regarding 0,1,2 using which LM order, it could be defined by user in
> i.MX9 System Manger cfg file. That means 1 could mean LM0, LM2, LM3, LM1
> or LM0, LM3, LM1, LM2.
> 

This sounds like this is not well defined and could change on a different
platform ? If so, how will you manage this extension across i.MX platforms ?

Or the above order is actually fixed and will remain same across the
platforms ?

-- 
Regards,
Sudeep

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

[Peng Fan]

> Subject: Re: [PATCH 3/7] firmware: arm_scmi: imx: Support getting cfg
> info of MISC protocol
> 
> On Fri, Jul 04, 2025 at 06:07:41PM +0800, Peng Fan wrote:
> > On Wed, Jul 02, 2025 at 04:21:58PM +0100, Sudeep Holla wrote:
> > >On Fri, Jun 27, 2025 at 02:03:46PM +0800, Peng Fan wrote:
> > >> MISC protocol supports getting the System Manager(SM) mode
> > >> selection and configuration name. 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    | 30
> ++++++++++++++++++++++
> > >>  include/linux/scmi_imx_protocol.h                  |  5 ++++
> > >>  2 files 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
> > >>
> 1b24d070c6f4856b92f515fcdba5836fd6498ce6..8ce4bf92e6535af2f3
> 0d72a34
> > >> 717678613b35049 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,
> > >>  };
> > >>
> > >> @@ -73,6 +74,11 @@ struct scmi_imx_misc_buildinfo_out {
> > >>  	u8 buildtime[MISC_MAX_BUILDTIME];  };
> > >>
> > >> +struct scmi_imx_misc_cfg_info_out {
> > >> +	__le32 msel;
> > >
> > >Now, I realise, this mode select is not properly defined in the
> document.
> > >Just 32-bit word. What are those values ? Any fixed list of values
> > >with well defined modes or configurations ? If so, please add to the
> document.
> >
> > The current used value are 0,1,2. It is used to decide the logic
> > machine boot order, such as 0 means booting LM0, LM1, LM2, LM3...
> > Regarding 0,1,2 using which LM order, it could be defined by user in
> > i.MX9 System Manger cfg file. That means 1 could mean LM0, LM2,
> LM3,
> > LM1 or LM0, LM3, LM1, LM2.
> >
> 
> This sounds like this is not well defined and could change on a different
> platform ? If so, how will you manage this extension across i.MX
> platforms ?
> 
> Or the above order is actually fixed and will remain same across the
> platforms ?

+ Chuck who is the best person to answer this. I may not explain it clear.

Regards
Peng

> 
> --
> Regards,
> Sudeep

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

[Sudeep Holla]

On Fri, Jul 04, 2025 at 10:39:53AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH 3/7] firmware: arm_scmi: imx: Support getting cfg
> > info of MISC protocol
> > 
> > On Fri, Jul 04, 2025 at 06:07:41PM +0800, Peng Fan wrote:
> > > On Wed, Jul 02, 2025 at 04:21:58PM +0100, Sudeep Holla wrote:
> > > >On Fri, Jun 27, 2025 at 02:03:46PM +0800, Peng Fan wrote:
> > > >> MISC protocol supports getting the System Manager(SM) mode
> > > >> selection and configuration name. 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    | 30
> > ++++++++++++++++++++++
> > > >>  include/linux/scmi_imx_protocol.h                  |  5 ++++
> > > >>  2 files 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
> > > >>
> > 1b24d070c6f4856b92f515fcdba5836fd6498ce6..8ce4bf92e6535af2f3
> > 0d72a34
> > > >> 717678613b35049 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,
> > > >>  };
> > > >>
> > > >> @@ -73,6 +74,11 @@ struct scmi_imx_misc_buildinfo_out {
> > > >>  	u8 buildtime[MISC_MAX_BUILDTIME];  };
> > > >>
> > > >> +struct scmi_imx_misc_cfg_info_out {
> > > >> +	__le32 msel;
> > > >
> > > >Now, I realise, this mode select is not properly defined in the
> > document.
> > > >Just 32-bit word. What are those values ? Any fixed list of values
> > > >with well defined modes or configurations ? If so, please add to the
> > document.
> > >
> > > The current used value are 0,1,2. It is used to decide the logic
> > > machine boot order, such as 0 means booting LM0, LM1, LM2, LM3...
> > > Regarding 0,1,2 using which LM order, it could be defined by user in
> > > i.MX9 System Manger cfg file. That means 1 could mean LM0, LM2,
> > LM3,
> > > LM1 or LM0, LM3, LM1, LM2.
> > >
> > 
> > This sounds like this is not well defined and could change on a different
> > platform ? If so, how will you manage this extension across i.MX
> > platforms ?
> > 
> > Or the above order is actually fixed and will remain same across the
> > platforms ?
> 
> + Chuck who is the best person to answer this. I may not explain it clear.
> 

Sorry I missed the mention of i.MX9 System Manger cfg file. So the exact
order is specified in that file and this seems like an index to that entry.
It is better to mention that in the document at the least if nothing else
can be fixed and you need flexibility to keep that variable.

-- 
Regards,
Sudeep

[PATCH 4/7] firmware: arm_scmi: imx: Support getting silicon info of MISC protocol

[Peng Fan]

MISC protocol supports getting the silicon information including revision
number, part number 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    | 34 ++++++++++++++++++++++
 include/linux/scmi_imx_protocol.h                  |  8 +++++
 2 files 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 8ce4bf92e6535af2f30d72a34717678613b35049..d5b24bc4d4ca6c19f4cddfaea6e9d9b32a4c92f7 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_SI_INFO = 0xB,
 	SCMI_IMX_MISC_CFG_INFO = 0xC,
 	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
 };
@@ -79,6 +80,13 @@ struct scmi_imx_misc_cfg_info_out {
 	u8 cfgname[MISC_MAX_CFGNAME];
 };
 
+struct scmi_imx_misc_si_info_out {
+	__le32 deviceid;
+	__le32 sirev;
+	__le32 partnum;
+	u8 siname[MISC_MAX_SINAME];
+};
+
 static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
 					struct scmi_imx_misc_info *mi)
 {
@@ -335,12 +343,38 @@ static int scmi_imx_misc_cfg_info(const struct scmi_protocol_handle *ph,
 	return ret;
 }
 
+static int scmi_imx_misc_silicon_info(const struct scmi_protocol_handle *ph,
+				      struct scmi_imx_misc_system_info *info)
+{
+	struct scmi_imx_misc_si_info_out *out;
+	struct scmi_xfer *t;
+	int ret;
+
+	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_SI_INFO, 0, sizeof(*out), &t);
+	if (ret)
+		return ret;
+
+	ret = ph->xops->do_xfer(ph, t);
+	if (!ret) {
+		out = t->rx.buf;
+		info->deviceid = le32_to_cpu(out->deviceid);
+		info->sirev = le32_to_cpu(out->sirev);
+		info->partnum = le32_to_cpu(out->partnum);
+		strscpy(info->siname, out->siname, MISC_MAX_SINAME);
+	}
+
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
 static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
 	.misc_cfg_info = scmi_imx_misc_cfg_info,
 	.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_discover_build_info = scmi_imx_discover_build_info,
+	.misc_silicon_info = scmi_imx_misc_silicon_info,
 };
 
 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 bb0c35b5d6705acddd6c83c31474482a2667b418..0e639dfb5d16e281e2ccf006a63694b316c431f4 100644
--- a/include/linux/scmi_imx_protocol.h
+++ b/include/linux/scmi_imx_protocol.h
@@ -55,6 +55,7 @@ struct scmi_imx_misc_ctrl_notify_report {
 #define MISC_MAX_BUILDDATE	16
 #define MISC_MAX_BUILDTIME	16
 #define MISC_MAX_CFGNAME	16
+#define MISC_MAX_SINAME		16
 
 struct scmi_imx_misc_system_info {
 	u32 buildnum;
@@ -63,6 +64,11 @@ struct scmi_imx_misc_system_info {
 	u8 time[MISC_MAX_BUILDTIME];
 	u32 msel;
 	u8 cfgname[MISC_MAX_CFGNAME];
+	/* silicon */
+	u32 deviceid;
+	u32 sirev;
+	u32 partnum;
+	u8 siname[MISC_MAX_SINAME];
 };
 
 struct scmi_imx_misc_proto_ops {
@@ -76,6 +82,8 @@ struct scmi_imx_misc_proto_ops {
 				    u32 ctrl_id, u32 evt_id, u32 flags);
 	int (*misc_discover_build_info)(const struct scmi_protocol_handle *ph,
 					struct scmi_imx_misc_system_info *info);
+	int (*misc_silicon_info)(const struct scmi_protocol_handle *ph,
+				 struct scmi_imx_misc_system_info *info);
 };
 
 /* See LMM_ATTRIBUTES in imx95.rst */

-- 
2.37.1

Re: [PATCH 4/7] firmware: arm_scmi: imx: Support getting silicon info of MISC protocol

[Cristian Marussi]

On Fri, Jun 27, 2025 at 02:03:47PM +0800, Peng Fan wrote:
> MISC protocol supports getting the silicon information including revision
> number, part number 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    | 34 ++++++++++++++++++++++
>  include/linux/scmi_imx_protocol.h                  |  8 +++++
>  2 files 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 8ce4bf92e6535af2f30d72a34717678613b35049..d5b24bc4d4ca6c19f4cddfaea6e9d9b32a4c92f7 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_SI_INFO = 0xB,
>  	SCMI_IMX_MISC_CFG_INFO = 0xC,
>  	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
>  };
> @@ -79,6 +80,13 @@ struct scmi_imx_misc_cfg_info_out {
>  	u8 cfgname[MISC_MAX_CFGNAME];
>  };
>  
> +struct scmi_imx_misc_si_info_out {
> +	__le32 deviceid;
> +	__le32 sirev;
> +	__le32 partnum;
> +	u8 siname[MISC_MAX_SINAME];
> +};
> +
>  static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
>  					struct scmi_imx_misc_info *mi)
>  {
> @@ -335,12 +343,38 @@ static int scmi_imx_misc_cfg_info(const struct scmi_protocol_handle *ph,
>  	return ret;
>  }
>  
> +static int scmi_imx_misc_silicon_info(const struct scmi_protocol_handle *ph,
> +				      struct scmi_imx_misc_system_info *info)
> +{
> +	struct scmi_imx_misc_si_info_out *out;
> +	struct scmi_xfer *t;
> +	int ret;
> +
> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_SI_INFO, 0, sizeof(*out), &t);
> +	if (ret)
> +		return ret;
> +
> +	ret = ph->xops->do_xfer(ph, t);
> +	if (!ret) {
> +		out = t->rx.buf;
> +		info->deviceid = le32_to_cpu(out->deviceid);
> +		info->sirev = le32_to_cpu(out->sirev);
> +		info->partnum = le32_to_cpu(out->partnum);
> +		strscpy(info->siname, out->siname, MISC_MAX_SINAME);
> +	}
> +
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
>  static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
>  	.misc_cfg_info = scmi_imx_misc_cfg_info,
>  	.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_discover_build_info = scmi_imx_discover_build_info,
> +	.misc_silicon_info = scmi_imx_misc_silicon_info,
>  };
>  
>  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 bb0c35b5d6705acddd6c83c31474482a2667b418..0e639dfb5d16e281e2ccf006a63694b316c431f4 100644
> --- a/include/linux/scmi_imx_protocol.h
> +++ b/include/linux/scmi_imx_protocol.h
> @@ -55,6 +55,7 @@ struct scmi_imx_misc_ctrl_notify_report {
>  #define MISC_MAX_BUILDDATE	16
>  #define MISC_MAX_BUILDTIME	16
>  #define MISC_MAX_CFGNAME	16
> +#define MISC_MAX_SINAME		16
>  
>  struct scmi_imx_misc_system_info {
>  	u32 buildnum;
> @@ -63,6 +64,11 @@ struct scmi_imx_misc_system_info {
>  	u8 time[MISC_MAX_BUILDTIME];
>  	u32 msel;
>  	u8 cfgname[MISC_MAX_CFGNAME];
> +	/* silicon */
> +	u32 deviceid;
> +	u32 sirev;
> +	u32 partnum;
> +	u8 siname[MISC_MAX_SINAME];
>  };

Same observation here...maybe embed a struct dedicated to this....BUT in
this case the silicon_info are NOT meant to change during a boot (and
even across a reboot really) so why a distinct command from build_info
since both infos has the same lifetime ? (I understand the quality of
the info returned is drastically different HW vs SW)

>  
>  struct scmi_imx_misc_proto_ops {
> @@ -76,6 +82,8 @@ struct scmi_imx_misc_proto_ops {
>  				    u32 ctrl_id, u32 evt_id, u32 flags);
>  	int (*misc_discover_build_info)(const struct scmi_protocol_handle *ph,
>  					struct scmi_imx_misc_system_info *info);
> +	int (*misc_silicon_info)(const struct scmi_protocol_handle *ph,
> +				 struct scmi_imx_misc_system_info *info);
>  };
>  
>  /* See LMM_ATTRIBUTES in imx95.rst */


Other than this, no strong opinion anyway.

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

Thanks,
Cristian

Re: [PATCH 4/7] firmware: arm_scmi: imx: Support getting silicon info of MISC protocol

[Sudeep Holla]

On Fri, Jun 27, 2025 at 02:03:47PM +0800, Peng Fan wrote:
> MISC protocol supports getting the silicon information including revision
> number, part number 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    | 34 ++++++++++++++++++++++
>  include/linux/scmi_imx_protocol.h                  |  8 +++++
>  2 files 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 8ce4bf92e6535af2f30d72a34717678613b35049..d5b24bc4d4ca6c19f4cddfaea6e9d9b32a4c92f7 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_SI_INFO = 0xB,

Again, this seem to have slipped through in my initial review. How is this
different from SMCCC SOC_ID interface. I am OK to have it as part of your
vendor extensions and be here in the kernel documentation. But I won't
accept any users of this within the kernel. Please provide justification
as why you can't use the standard SMCCC SOC_ID.

So, clear NACK for adding this support in the kernel for now.

It is pretty useless to push this to userspace with custom interface.
Use the existing interface with SOC_ID. Also the way I would think this
interface may be used is from SMCCC interface implementation which could
retrieve info via this interface but that would be just platform specific
code in the firmware.

-- 
Regards,
Sudeep

Re: [PATCH 4/7] firmware: arm_scmi: imx: Support getting silicon info of MISC protocol

[Peng Fan]

On Wed, Jul 02, 2025 at 04:22:11PM +0100, Sudeep Holla wrote:
>On Fri, Jun 27, 2025 at 02:03:47PM +0800, Peng Fan wrote:
>> MISC protocol supports getting the silicon information including revision
>> number, part number 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    | 34 ++++++++++++++++++++++
>>  include/linux/scmi_imx_protocol.h                  |  8 +++++
>>  2 files 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 8ce4bf92e6535af2f30d72a34717678613b35049..d5b24bc4d4ca6c19f4cddfaea6e9d9b32a4c92f7 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_SI_INFO = 0xB,
>
>Again, this seem to have slipped through in my initial review. How is this
>different from SMCCC SOC_ID interface. I am OK to have it as part of your
>vendor extensions and be here in the kernel documentation. But I won't
>accept any users of this within the kernel. Please provide justification
>as why you can't use the standard SMCCC SOC_ID.
>
>So, clear NACK for adding this support in the kernel for now.

What I do here is just wanna to let
linux could print similar information as what SM shows in its console:

>$ info
SM Version    = Build 677, Commit 49a36aaf
SM Config     = mx95evk, mSel=0
Board         = i.MX95 EVK, attr=0x00000000
Silicon       = i.MX95 B0
Boot mode     = normal
Boot device   = SD2
Boot stage    = primary
Boot set      = 1
ECID          = 0x7BADEECC000001D40001300963E636F1
PMIC 0 (0x08) = 0x20, 0x09, 0x20, 0x00, 0x01
PMIC 1 (0x2A) = 0x54, 0x22, 0x00, 0x0B
PMIC 2 (0x29) = 0x55, 0x22, 0x00, 0x0A
Compiler      = gcc 14.2.1 20241119

With soc_device_register, dumping the silicon information needs use the
other sysfs interface. Here with this patchset, reading one sysfs file could
dump all the information.

But anyway, ok to drop this patch.
 
Thanks,
Peng

Re: [PATCH 4/7] firmware: arm_scmi: imx: Support getting silicon info of MISC protocol

[Sudeep Holla]

On Fri, Jul 04, 2025 at 06:20:57PM +0800, Peng Fan wrote:
> On Wed, Jul 02, 2025 at 04:22:11PM +0100, Sudeep Holla wrote:
> >On Fri, Jun 27, 2025 at 02:03:47PM +0800, Peng Fan wrote:
> >> MISC protocol supports getting the silicon information including revision
> >> number, part number 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    | 34 ++++++++++++++++++++++
> >>  include/linux/scmi_imx_protocol.h                  |  8 +++++
> >>  2 files 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 8ce4bf92e6535af2f30d72a34717678613b35049..d5b24bc4d4ca6c19f4cddfaea6e9d9b32a4c92f7 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_SI_INFO = 0xB,
> >
> >Again, this seem to have slipped through in my initial review. How is this
> >different from SMCCC SOC_ID interface. I am OK to have it as part of your
> >vendor extensions and be here in the kernel documentation. But I won't
> >accept any users of this within the kernel. Please provide justification
> >as why you can't use the standard SMCCC SOC_ID.
> >
> >So, clear NACK for adding this support in the kernel for now.
> 
> What I do here is just wanna to let
> linux could print similar information as what SM shows in its console:
> 

Sure you can dump whatever you want on the console. The main concern is
another user interface for very similar information that can be obtained
by SOC_ID or soc_device in the kernel. Just kernel internal use should
be fine.

> >$ info
> SM Version    = Build 677, Commit 49a36aaf
> SM Config     = mx95evk, mSel=0
> Board         = i.MX95 EVK, attr=0x00000000
> Silicon       = i.MX95 B0
> Boot mode     = normal
> Boot device   = SD2
> Boot stage    = primary
> Boot set      = 1
> ECID          = 0x7BADEECC000001D40001300963E636F1
> PMIC 0 (0x08) = 0x20, 0x09, 0x20, 0x00, 0x01
> PMIC 1 (0x2A) = 0x54, 0x22, 0x00, 0x0B
> PMIC 2 (0x29) = 0x55, 0x22, 0x00, 0x0A
> Compiler      = gcc 14.2.1 20241119
> 
> With soc_device_register, dumping the silicon information needs use the
> other sysfs interface.

So what ?

> Here with this patchset, reading one sysfs file could dump all the information.
> 

That goes completely again the sysfs policy. Single file single value.
Convince Greg otherwise, I will consider elsewhere not for this SOC_ID
for sure.

> But anyway, ok to drop this patch.
>  

Thanks!

-- 
Regards,
Sudeep

[PATCH 5/7] 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 d5b24bc4d4ca6c19f4cddfaea6e9d9b32a4c92f7..1a6d75357b76ce6bb7d06461999b368c27f1fa43 100644
--- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
+++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
@@ -28,6 +28,7 @@ enum scmi_imx_misc_protocol_cmd {
 	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
 	SCMI_IMX_MISC_SI_INFO = 0xB,
 	SCMI_IMX_MISC_CFG_INFO = 0xC,
+	SCMI_IMX_MISC_SYSLOG = 0xD,
 	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
 };
 
@@ -87,6 +88,19 @@ struct scmi_imx_misc_si_info_out {
 	u8 siname[MISC_MAX_SINAME];
 };
 
+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)
 {
@@ -368,6 +382,69 @@ static int scmi_imx_misc_silicon_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_cfg_info = scmi_imx_misc_cfg_info,
 	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
@@ -375,6 +452,7 @@ static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
 	.misc_ctrl_req_notify = scmi_imx_misc_ctrl_notify,
 	.misc_discover_build_info = scmi_imx_discover_build_info,
 	.misc_silicon_info = scmi_imx_misc_silicon_info,
+	.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 0e639dfb5d16e281e2ccf006a63694b316c431f4..ff34d974046aa982fa9f5d46fc673412e01a532d 100644
--- a/include/linux/scmi_imx_protocol.h
+++ b/include/linux/scmi_imx_protocol.h
@@ -71,6 +71,23 @@ struct scmi_imx_misc_system_info {
 	u8 siname[MISC_MAX_SINAME];
 };
 
+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_cfg_info)(const struct scmi_protocol_handle *ph,
 			     struct scmi_imx_misc_system_info *info);
@@ -84,6 +101,8 @@ struct scmi_imx_misc_proto_ops {
 					struct scmi_imx_misc_system_info *info);
 	int (*misc_silicon_info)(const struct scmi_protocol_handle *ph,
 				 struct scmi_imx_misc_system_info *info);
+	int (*misc_syslog)(const struct scmi_protocol_handle *ph, u16 size,
+			  void *array);
 };
 
 /* See LMM_ATTRIBUTES in imx95.rst */

-- 
2.37.1

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

[Cristian Marussi]

On Fri, Jun 27, 2025 at 02:03:48PM +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 d5b24bc4d4ca6c19f4cddfaea6e9d9b32a4c92f7..1a6d75357b76ce6bb7d06461999b368c27f1fa43 100644
> --- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> @@ -28,6 +28,7 @@ enum scmi_imx_misc_protocol_cmd {
>  	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
>  	SCMI_IMX_MISC_SI_INFO = 0xB,
>  	SCMI_IMX_MISC_CFG_INFO = 0xC,
> +	SCMI_IMX_MISC_SYSLOG = 0xD,
>  	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
>  };
>  
> @@ -87,6 +88,19 @@ struct scmi_imx_misc_si_info_out {
>  	u8 siname[MISC_MAX_SINAME];
>  };
>  
> +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)
>  {
> @@ -368,6 +382,69 @@ static int scmi_imx_misc_silicon_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)
> +{

...so this size...

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

...which cannot be zero and is passed down to the iterator as max_resources
is meant to repreent also the length of tthe array passed here as an
argument and filled-in by the iterators ?

...and so basically array bounds-checking is enforced by the iterators
core code, because no matter what, it is always enforced that

	(returned + remaining <= max_resources (size)

...I am fine with this, I am just trying to understand and see if I can
find a mishap :D

> +	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_cfg_info = scmi_imx_misc_cfg_info,
>  	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
> @@ -375,6 +452,7 @@ static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
>  	.misc_ctrl_req_notify = scmi_imx_misc_ctrl_notify,
>  	.misc_discover_build_info = scmi_imx_discover_build_info,
>  	.misc_silicon_info = scmi_imx_misc_silicon_info,
> +	.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 0e639dfb5d16e281e2ccf006a63694b316c431f4..ff34d974046aa982fa9f5d46fc673412e01a532d 100644
> --- a/include/linux/scmi_imx_protocol.h
> +++ b/include/linux/scmi_imx_protocol.h
> @@ -71,6 +71,23 @@ struct scmi_imx_misc_system_info {
>  	u8 siname[MISC_MAX_SINAME];
>  };
>  
> +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;
> +};

So where is this used ? later in the series ?
> +
> +struct scmi_imx_misc_syslog {
> +	struct scmi_imx_misc_sys_sleep_rec syssleeprecord;
> +	uint32_t deverrlog;
> +};
> +
>  struct scmi_imx_misc_proto_ops {
>  	int (*misc_cfg_info)(const struct scmi_protocol_handle *ph,
>  			     struct scmi_imx_misc_system_info *info);
> @@ -84,6 +101,8 @@ struct scmi_imx_misc_proto_ops {
>  					struct scmi_imx_misc_system_info *info);
>  	int (*misc_silicon_info)(const struct scmi_protocol_handle *ph,
>  				 struct scmi_imx_misc_system_info *info);
> +	int (*misc_syslog)(const struct scmi_protocol_handle *ph, u16 size,
> +			  void *array);
>  };
> 

Thanks,
Cristian

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

[Peng Fan]

Hi Cristian,

On Fri, Jun 27, 2025 at 02:44:00PM +0100, Cristian Marussi wrote:
>On Fri, Jun 27, 2025 at 02:03:48PM +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 d5b24bc4d4ca6c19f4cddfaea6e9d9b32a4c92f7..1a6d75357b76ce6bb7d06461999b368c27f1fa43 100644
>> --- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> @@ -28,6 +28,7 @@ enum scmi_imx_misc_protocol_cmd {
>>  	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
>>  	SCMI_IMX_MISC_SI_INFO = 0xB,
>>  	SCMI_IMX_MISC_CFG_INFO = 0xC,
>> +	SCMI_IMX_MISC_SYSLOG = 0xD,
>>  	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
>>  };
>>  
>> @@ -87,6 +88,19 @@ struct scmi_imx_misc_si_info_out {
>>  	u8 siname[MISC_MAX_SINAME];
>>  };
>>  
>> +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)
>>  {
>> @@ -368,6 +382,69 @@ static int scmi_imx_misc_silicon_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)
>> +{
>
>...so this size...
>
>> +	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;
>> +
>
>...which cannot be zero and is passed down to the iterator as max_resources
>is meant to repreent also the length of tthe array passed here as an
>argument and filled-in by the iterators ?
>
>...and so basically array bounds-checking is enforced by the iterators
>core code, because no matter what, it is always enforced that
>
>	(returned + remaining <= max_resources (size)

Right. I think set size to 0 does not make sense, so add a '(!size)' check

>
>...I am fine with this, I am just trying to understand and see if I can
>find a mishap :D
>
>> +	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_cfg_info = scmi_imx_misc_cfg_info,
>>  	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
>> @@ -375,6 +452,7 @@ static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
>>  	.misc_ctrl_req_notify = scmi_imx_misc_ctrl_notify,
>>  	.misc_discover_build_info = scmi_imx_discover_build_info,
>>  	.misc_silicon_info = scmi_imx_misc_silicon_info,
>> +	.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 0e639dfb5d16e281e2ccf006a63694b316c431f4..ff34d974046aa982fa9f5d46fc673412e01a532d 100644
>> --- a/include/linux/scmi_imx_protocol.h
>> +++ b/include/linux/scmi_imx_protocol.h
>> @@ -71,6 +71,23 @@ struct scmi_imx_misc_system_info {
>>  	u8 siname[MISC_MAX_SINAME];
>>  };
>>  
>> +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;
>> +};
>
>So where is this used ? later in the series ?
>> +
>> +struct scmi_imx_misc_syslog {
>> +	struct scmi_imx_misc_sys_sleep_rec syssleeprecord;

Included here, but used in last patch in the patchset.

>> +	uint32_t deverrlog;
>> +};
>> +
>>  struct scmi_imx_misc_proto_ops {
>>  	int (*misc_cfg_info)(const struct scmi_protocol_handle *ph,
>>  			     struct scmi_imx_misc_system_info *info);
>> @@ -84,6 +101,8 @@ struct scmi_imx_misc_proto_ops {
>>  					struct scmi_imx_misc_system_info *info);
>>  	int (*misc_silicon_info)(const struct scmi_protocol_handle *ph,
>>  				 struct scmi_imx_misc_system_info *info);
>> +	int (*misc_syslog)(const struct scmi_protocol_handle *ph, u16 size,
>> +			  void *array);
>>  };
>> 

Thanks,
Peng

>
>Thanks,
>Cristian

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

[Sudeep Holla]

On Fri, Jun 27, 2025 at 02:03:48PM +0800, Peng Fan wrote:
> MISC protocol supports getting system log regarding system sleep latency
> ,wakeup interrupt and etc.

We now have SCMI telemetry protocol to provide such information in a
standard protocol. You must consider using that or switching to that
in near future. I am OK with this but would like to warn use of custom
interface for things like this is not scalable for long term.

-- 
Regards,
Sudeep

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

[Peng Fan]

On Wed, Jul 02, 2025 at 04:22:30PM +0100, Sudeep Holla wrote:
>On Fri, Jun 27, 2025 at 02:03:48PM +0800, Peng Fan wrote:
>> MISC protocol supports getting system log regarding system sleep latency
>> ,wakeup interrupt and etc.
>
>We now have SCMI telemetry protocol to provide such information in a
>standard protocol. You must consider using that or switching to that
>in near future. I am OK with this but would like to warn use of custom
>interface for things like this is not scalable for long term.

I have shared latest 4.0 alpha release information to our firmware
owner. In future, we may use telemetry, but not up to me.

Thanks,
Peng

>
>-- 
>Regards,
>Sudeep

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

[Sudeep Holla]

On Fri, Jul 04, 2025 at 06:23:53PM +0800, Peng Fan wrote:
> On Wed, Jul 02, 2025 at 04:22:30PM +0100, Sudeep Holla wrote:
> >On Fri, Jun 27, 2025 at 02:03:48PM +0800, Peng Fan wrote:
> >> MISC protocol supports getting system log regarding system sleep latency
> >> ,wakeup interrupt and etc.
> >
> >We now have SCMI telemetry protocol to provide such information in a
> >standard protocol. You must consider using that or switching to that
> >in near future. I am OK with this but would like to warn use of custom
> >interface for things like this is not scalable for long term.
> 
> I have shared latest 4.0 alpha release information to our firmware
> owner. In future, we may use telemetry, but not up to me.
> 

Thanks for that. Much appreciated!

-- 
Regards,
Sudeep

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

[Peng Fan]

MISC protocol supports getting board information. 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    | 30 ++++++++++++++++++++++
 include/linux/scmi_imx_protocol.h                  |  5 ++++
 2 files 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 1a6d75357b76ce6bb7d06461999b368c27f1fa43..35c63e7cb189475807ed1e6723dbcb61ab66800a 100644
--- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
+++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
@@ -29,6 +29,7 @@ enum scmi_imx_misc_protocol_cmd {
 	SCMI_IMX_MISC_SI_INFO = 0xB,
 	SCMI_IMX_MISC_CFG_INFO = 0xC,
 	SCMI_IMX_MISC_SYSLOG = 0xD,
+	SCMI_IMX_MISC_BOARD_INFO = 0xE,
 	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
 };
 
@@ -76,6 +77,11 @@ struct scmi_imx_misc_buildinfo_out {
 	u8 buildtime[MISC_MAX_BUILDTIME];
 };
 
+struct scmi_imx_misc_board_info_out {
+	__le32 attributes;
+	u8 brdname[MISC_MAX_BRDNAME];
+};
+
 struct scmi_imx_misc_cfg_info_out {
 	__le32 msel;
 	u8 cfgname[MISC_MAX_CFGNAME];
@@ -334,6 +340,29 @@ static int scmi_imx_discover_build_info(const struct scmi_protocol_handle *ph,
 	return ret;
 }
 
+static int scmi_imx_misc_board_info(const struct scmi_protocol_handle *ph,
+				    struct scmi_imx_misc_system_info *info)
+{
+	struct scmi_imx_misc_board_info_out *out;
+	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;
+		info->brd_attributes = le32_to_cpu(out->attributes);
+		strscpy(info->brdname, out->brdname, MISC_MAX_BRDNAME);
+	}
+
+	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_system_info *info)
 {
@@ -446,6 +475,7 @@ static int scmi_imx_misc_syslog(const struct scmi_protocol_handle *ph, u16 size,
 }
 
 static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
+	.misc_board_info = scmi_imx_misc_board_info,
 	.misc_cfg_info = scmi_imx_misc_cfg_info,
 	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
 	.misc_ctrl_get = scmi_imx_misc_ctrl_get,
diff --git a/include/linux/scmi_imx_protocol.h b/include/linux/scmi_imx_protocol.h
index ff34d974046aa982fa9f5d46fc673412e01a532d..4950cd6f50aa7b3038bd519a7287e805f70e1cf5 100644
--- a/include/linux/scmi_imx_protocol.h
+++ b/include/linux/scmi_imx_protocol.h
@@ -56,6 +56,7 @@ struct scmi_imx_misc_ctrl_notify_report {
 #define MISC_MAX_BUILDTIME	16
 #define MISC_MAX_CFGNAME	16
 #define MISC_MAX_SINAME		16
+#define MISC_MAX_BRDNAME	16
 
 struct scmi_imx_misc_system_info {
 	u32 buildnum;
@@ -69,6 +70,8 @@ struct scmi_imx_misc_system_info {
 	u32 sirev;
 	u32 partnum;
 	u8 siname[MISC_MAX_SINAME];
+	u32 brd_attributes;
+	u8 brdname[MISC_MAX_BRDNAME];
 };
 
 struct scmi_imx_misc_sys_sleep_rec {
@@ -89,6 +92,8 @@ struct scmi_imx_misc_syslog {
 };
 
 struct scmi_imx_misc_proto_ops {
+	int (*misc_board_info)(const struct scmi_protocol_handle *ph,
+			       struct scmi_imx_misc_system_info *info);
 	int (*misc_cfg_info)(const struct scmi_protocol_handle *ph,
 			     struct scmi_imx_misc_system_info *info);
 	int (*misc_ctrl_set)(const struct scmi_protocol_handle *ph, u32 id,

-- 
2.37.1

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

[Cristian Marussi]

On Fri, Jun 27, 2025 at 02:03:49PM +0800, Peng Fan wrote:
> MISC protocol supports getting board information. 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    | 30 ++++++++++++++++++++++
>  include/linux/scmi_imx_protocol.h                  |  5 ++++
>  2 files 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 1a6d75357b76ce6bb7d06461999b368c27f1fa43..35c63e7cb189475807ed1e6723dbcb61ab66800a 100644
> --- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> @@ -29,6 +29,7 @@ enum scmi_imx_misc_protocol_cmd {
>  	SCMI_IMX_MISC_SI_INFO = 0xB,
>  	SCMI_IMX_MISC_CFG_INFO = 0xC,
>  	SCMI_IMX_MISC_SYSLOG = 0xD,
> +	SCMI_IMX_MISC_BOARD_INFO = 0xE,
>  	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
>  };
>  
> @@ -76,6 +77,11 @@ struct scmi_imx_misc_buildinfo_out {
>  	u8 buildtime[MISC_MAX_BUILDTIME];
>  };
>  
> +struct scmi_imx_misc_board_info_out {
> +	__le32 attributes;
> +	u8 brdname[MISC_MAX_BRDNAME];
> +};
> +
>  struct scmi_imx_misc_cfg_info_out {
>  	__le32 msel;
>  	u8 cfgname[MISC_MAX_CFGNAME];
> @@ -334,6 +340,29 @@ static int scmi_imx_discover_build_info(const struct scmi_protocol_handle *ph,
>  	return ret;
>  }
>  
> +static int scmi_imx_misc_board_info(const struct scmi_protocol_handle *ph,
> +				    struct scmi_imx_misc_system_info *info)
> +{
> +	struct scmi_imx_misc_board_info_out *out;
> +	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;
> +		info->brd_attributes = le32_to_cpu(out->attributes);
> +		strscpy(info->brdname, out->brdname, MISC_MAX_BRDNAME);
> +	}
> +
> +	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_system_info *info)
>  {
> @@ -446,6 +475,7 @@ static int scmi_imx_misc_syslog(const struct scmi_protocol_handle *ph, u16 size,
>  }
>  
>  static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
> +	.misc_board_info = scmi_imx_misc_board_info,
>  	.misc_cfg_info = scmi_imx_misc_cfg_info,
>  	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
>  	.misc_ctrl_get = scmi_imx_misc_ctrl_get,
> diff --git a/include/linux/scmi_imx_protocol.h b/include/linux/scmi_imx_protocol.h
> index ff34d974046aa982fa9f5d46fc673412e01a532d..4950cd6f50aa7b3038bd519a7287e805f70e1cf5 100644
> --- a/include/linux/scmi_imx_protocol.h
> +++ b/include/linux/scmi_imx_protocol.h
> @@ -56,6 +56,7 @@ struct scmi_imx_misc_ctrl_notify_report {
>  #define MISC_MAX_BUILDTIME	16
>  #define MISC_MAX_CFGNAME	16
>  #define MISC_MAX_SINAME		16
> +#define MISC_MAX_BRDNAME	16
>  
>  struct scmi_imx_misc_system_info {
>  	u32 buildnum;
> @@ -69,6 +70,8 @@ struct scmi_imx_misc_system_info {
>  	u32 sirev;
>  	u32 partnum;
>  	u8 siname[MISC_MAX_SINAME];
> +	u32 brd_attributes;
> +	u8 brdname[MISC_MAX_BRDNAME];
>  };

Same comment here as before...

>  
>  struct scmi_imx_misc_sys_sleep_rec {
> @@ -89,6 +92,8 @@ struct scmi_imx_misc_syslog {
>  };
>  
>  struct scmi_imx_misc_proto_ops {
> +	int (*misc_board_info)(const struct scmi_protocol_handle *ph,
> +			       struct scmi_imx_misc_system_info *info);
>  	int (*misc_cfg_info)(const struct scmi_protocol_handle *ph,
>  			     struct scmi_imx_misc_system_info *info);
>  	int (*misc_ctrl_set)(const struct scmi_protocol_handle *ph, u32 id,
>

Anyway, LGTM.
Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
 
Thanks,
Cristian

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

[Peng Fan]

On Fri, Jun 27, 2025 at 02:45:47PM +0100, Cristian Marussi wrote:
>On Fri, Jun 27, 2025 at 02:03:49PM +0800, Peng Fan wrote:
>> MISC protocol supports getting board information. 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    | 30 ++++++++++++++++++++++
>>  include/linux/scmi_imx_protocol.h                  |  5 ++++
>>  2 files 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 1a6d75357b76ce6bb7d06461999b368c27f1fa43..35c63e7cb189475807ed1e6723dbcb61ab66800a 100644
>> --- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> @@ -29,6 +29,7 @@ enum scmi_imx_misc_protocol_cmd {
>>  	SCMI_IMX_MISC_SI_INFO = 0xB,
>>  	SCMI_IMX_MISC_CFG_INFO = 0xC,
>>  	SCMI_IMX_MISC_SYSLOG = 0xD,
>> +	SCMI_IMX_MISC_BOARD_INFO = 0xE,
>>  	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
>>  };
>>  
>> @@ -76,6 +77,11 @@ struct scmi_imx_misc_buildinfo_out {
>>  	u8 buildtime[MISC_MAX_BUILDTIME];
>>  };
>>  
>> +struct scmi_imx_misc_board_info_out {
>> +	__le32 attributes;
>> +	u8 brdname[MISC_MAX_BRDNAME];
>> +};
>> +
>>  struct scmi_imx_misc_cfg_info_out {
>>  	__le32 msel;
>>  	u8 cfgname[MISC_MAX_CFGNAME];
>> @@ -334,6 +340,29 @@ static int scmi_imx_discover_build_info(const struct scmi_protocol_handle *ph,
>>  	return ret;
>>  }
>>  
>> +static int scmi_imx_misc_board_info(const struct scmi_protocol_handle *ph,
>> +				    struct scmi_imx_misc_system_info *info)
>> +{
>> +	struct scmi_imx_misc_board_info_out *out;
>> +	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;
>> +		info->brd_attributes = le32_to_cpu(out->attributes);
>> +		strscpy(info->brdname, out->brdname, MISC_MAX_BRDNAME);
>> +	}
>> +
>> +	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_system_info *info)
>>  {
>> @@ -446,6 +475,7 @@ static int scmi_imx_misc_syslog(const struct scmi_protocol_handle *ph, u16 size,
>>  }
>>  
>>  static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
>> +	.misc_board_info = scmi_imx_misc_board_info,
>>  	.misc_cfg_info = scmi_imx_misc_cfg_info,
>>  	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
>>  	.misc_ctrl_get = scmi_imx_misc_ctrl_get,
>> diff --git a/include/linux/scmi_imx_protocol.h b/include/linux/scmi_imx_protocol.h
>> index ff34d974046aa982fa9f5d46fc673412e01a532d..4950cd6f50aa7b3038bd519a7287e805f70e1cf5 100644
>> --- a/include/linux/scmi_imx_protocol.h
>> +++ b/include/linux/scmi_imx_protocol.h
>> @@ -56,6 +56,7 @@ struct scmi_imx_misc_ctrl_notify_report {
>>  #define MISC_MAX_BUILDTIME	16
>>  #define MISC_MAX_CFGNAME	16
>>  #define MISC_MAX_SINAME		16
>> +#define MISC_MAX_BRDNAME	16
>>  
>>  struct scmi_imx_misc_system_info {
>>  	u32 buildnum;
>> @@ -69,6 +70,8 @@ struct scmi_imx_misc_system_info {
>>  	u32 sirev;
>>  	u32 partnum;
>>  	u8 siname[MISC_MAX_SINAME];
>> +	u32 brd_attributes;
>> +	u8 brdname[MISC_MAX_BRDNAME];
>>  };
>
>Same comment here as before...

Just reply here for all comments regarding system_info.

I could add something below in next version:

struct scmi_imx_misc_firmware_info {
};
struct scmi_imx_misc_silicon_info {
};
struct scmi_imx_misc_board_info {
};

struct scm_imx_misc_system_info {
	struct scmi_imx_misc_firmware_info firmware_info;
	struct scmi_imx_misc_silicon_info si_info;
	struct scmi_imx_misc_board_info board_info;
};

>
>>  
>>  struct scmi_imx_misc_sys_sleep_rec {
>> @@ -89,6 +92,8 @@ struct scmi_imx_misc_syslog {
>>  };
>>  
>>  struct scmi_imx_misc_proto_ops {
>> +	int (*misc_board_info)(const struct scmi_protocol_handle *ph,
>> +			       struct scmi_imx_misc_system_info *info);
>>  	int (*misc_cfg_info)(const struct scmi_protocol_handle *ph,
>>  			     struct scmi_imx_misc_system_info *info);
>>  	int (*misc_ctrl_set)(const struct scmi_protocol_handle *ph, u32 id,
>>
>
>Anyway, LGTM.
>Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Peng
> 
>Thanks,
>Cristian

[PATCH 7/7] firmware: imx: sm-misc: Dump syslog and system info

[Peng Fan]

Add sysfs interface to read System Manager syslog and system info

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

diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
index fc3ee12c2be878e0285183e3381c9514a63d5142..55485a3c4a5c615102a377f41025a6911d746770 100644
--- a/drivers/firmware/imx/sm-misc.c
+++ b/drivers/firmware/imx/sm-misc.c
@@ -44,6 +44,100 @@ static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
 	return 0;
 }
 
+static ssize_t syslog_show(struct device *device, struct device_attribute *attr,
+			   char *buf)
+{
+	struct scmi_imx_misc_sys_sleep_rec *rec;
+	struct scmi_imx_misc_syslog *syslog;
+	int ret;
+	size_t len = 0;
+
+	if (!ph)
+		return 0;
+
+	syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
+	if (!syslog)
+		return -ENOMEM;
+
+	ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
+	if (ret) {
+		kfree(syslog);
+		return ret;
+	}
+
+	rec = &syslog->syssleeprecord;
+
+	len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec->wakesource);
+	len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec->syssleepmode);
+	len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec->syssleepflags);
+	len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n", rec->mixpwrstat);
+	len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n", rec->mempwrstat);
+	len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n", rec->pllpwrstat);
+	len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec->sleepentryusec);
+	len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec->sleepexitusec);
+	len += sysfs_emit_at(buf, len, "Sleep count = %u\n", rec->sleepcnt);
+
+	kfree(syslog);
+
+	return len;
+}
+
+static DEVICE_ATTR_RO(syslog);
+
+static ssize_t system_info_show(struct device *device, struct device_attribute *attr,
+				char *buf)
+{
+	struct scmi_imx_misc_system_info *info;
+	int len = 0;
+	int ret;
+
+	if (!ph)
+		return 0;
+
+	info = kmalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	ret = imx_misc_ctrl_ops->misc_discover_build_info(ph, info);
+	if (ret)
+		goto err;
+
+	ret = imx_misc_ctrl_ops->misc_cfg_info(ph, info);
+	if (ret)
+		goto err;
+
+	ret = imx_misc_ctrl_ops->misc_silicon_info(ph, info);
+	if (ret)
+		goto err;
+
+	ret = imx_misc_ctrl_ops->misc_board_info(ph, info);
+	if (ret)
+		goto err;
+
+	len += sysfs_emit_at(buf, len, "SM Version    = Build %u, Commit 08%x\n",
+			     info->buildnum, info->buildcommit);
+	len += sysfs_emit_at(buf, len, "SM Config     = %s, mSel=%u\n",
+			     info->cfgname, info->msel);
+	len += sysfs_emit_at(buf, len, "Silicon       = %s\n", info->siname);
+	len += sysfs_emit_at(buf, len, "Board         = %s, attr=0x%08x\n",
+			     info->brdname, info->brd_attributes);
+
+	ret = len;
+err:
+	kfree(info);
+	return ret;
+}
+
+static DEVICE_ATTR_RO(system_info);
+
+static struct attribute *sm_misc_attrs[] = {
+	&dev_attr_syslog.attr,
+	&dev_attr_system_info.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(sm_misc);
+
 static int scmi_imx_misc_ctrl_probe(struct scmi_device *sdev)
 {
 	const struct scmi_handle *handle = sdev->handle;
@@ -108,6 +202,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 7/7] firmware: imx: sm-misc: Dump syslog and system info

[Cristian Marussi]

On Fri, Jun 27, 2025 at 02:03:50PM +0800, Peng Fan wrote:
> Add sysfs interface to read System Manager syslog and system info
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/firmware/imx/sm-misc.c | 97 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
> index fc3ee12c2be878e0285183e3381c9514a63d5142..55485a3c4a5c615102a377f41025a6911d746770 100644
> --- a/drivers/firmware/imx/sm-misc.c
> +++ b/drivers/firmware/imx/sm-misc.c
> @@ -44,6 +44,100 @@ static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
>  	return 0;
>  }
>  
> +static ssize_t syslog_show(struct device *device, struct device_attribute *attr,
> +			   char *buf)
> +{
> +	struct scmi_imx_misc_sys_sleep_rec *rec;
> +	struct scmi_imx_misc_syslog *syslog;
> +	int ret;
> +	size_t len = 0;
> +
> +	if (!ph)
> +		return 0;
> +
> +	syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
> +	if (!syslog)
> +		return -ENOMEM;
> +
> +	ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);

...ah...so you basically cast to void a structure of u32 words and then
expect that the firmware perfectly aligned with how the struct is
defined here....size checks assures no out-of-bounds BUT the meaning of
he blob itself is entirely up to FW and kernel being aligned, since no
type checking is done of any kind and fields are NOT reference by
name...so may I ask why ? ..also because the scmi_imx_misc_syslog seems
pretty tiny to need iterators to parse back the reply...do you have so
tiny transpotr message size ?

..anyway I would suggest at least to add some sort of version-field to
the struct so that at least you can detect if the FW spits out something
that is not what your driver expect or is ready to handle...especially
if you plan to extend or modify the layout of the structs in the future.


> +	if (ret) {
> +		kfree(syslog);
> +		return ret;
> +	}
> +
> +	rec = &syslog->syssleeprecord;
> +
> +	len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec->wakesource);
> +	len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec->syssleepmode);
> +	len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec->syssleepflags);
> +	len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n", rec->mixpwrstat);
> +	len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n", rec->mempwrstat);
> +	len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n", rec->pllpwrstat);
> +	len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec->sleepentryusec);
> +	len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec->sleepexitusec);
> +	len += sysfs_emit_at(buf, len, "Sleep count = %u\n", rec->sleepcnt);
> +

... how wonder how much is still frowned upon to serve such big multiline
structured information payloads from sysfs ... (asking for a friend :P)


> +	kfree(syslog);
> +
> +	return len;
> +}
> +
> +static DEVICE_ATTR_RO(syslog);
> +
> +static ssize_t system_info_show(struct device *device, struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct scmi_imx_misc_system_info *info;
> +	int len = 0;
> +	int ret;
> +
> +	if (!ph)
> +		return 0;
> +
> +	info = kmalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	ret = imx_misc_ctrl_ops->misc_discover_build_info(ph, info);
> +	if (ret)
> +		goto err;
> +
> +	ret = imx_misc_ctrl_ops->misc_cfg_info(ph, info);
> +	if (ret)
> +		goto err;
> +
> +	ret = imx_misc_ctrl_ops->misc_silicon_info(ph, info);
> +	if (ret)
> +		goto err;
> +
> +	ret = imx_misc_ctrl_ops->misc_board_info(ph, info);
> +	if (ret)
> +		goto err;
> +
> +	len += sysfs_emit_at(buf, len, "SM Version    = Build %u, Commit 08%x\n",
> +			     info->buildnum, info->buildcommit);
> +	len += sysfs_emit_at(buf, len, "SM Config     = %s, mSel=%u\n",
> +			     info->cfgname, info->msel);
> +	len += sysfs_emit_at(buf, len, "Silicon       = %s\n", info->siname);
> +	len += sysfs_emit_at(buf, len, "Board         = %s, attr=0x%08x\n",
> +			     info->brdname, info->brd_attributes);

...so some of this stuff Build/Silicon/BoaRD info has pretty much
'forever' lifetime...are you sure you want to query them out of the FW
each time you issue a sysfs show ?

Cannot you simply dump this stuff once for all at probve time and
instead query misc_cfg_info() upon each show to refresh only the data
that will change ?

(this also corroborates my idea that you should somehow partition this
data into distinct structs by their lifetime...

Thanks,
Cristian

Re: [PATCH 7/7] firmware: imx: sm-misc: Dump syslog and system info

[Peng Fan]

On Fri, Jun 27, 2025 at 03:11:03PM +0100, Cristian Marussi wrote:
>On Fri, Jun 27, 2025 at 02:03:50PM +0800, Peng Fan wrote:
>> Add sysfs interface to read System Manager syslog and system info
>> 
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>  drivers/firmware/imx/sm-misc.c | 97 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 97 insertions(+)
>> 
>> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
>> index fc3ee12c2be878e0285183e3381c9514a63d5142..55485a3c4a5c615102a377f41025a6911d746770 100644
>> --- a/drivers/firmware/imx/sm-misc.c
>> +++ b/drivers/firmware/imx/sm-misc.c
>> @@ -44,6 +44,100 @@ static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
>>  	return 0;
>>  }
>>  
>> +static ssize_t syslog_show(struct device *device, struct device_attribute *attr,
>> +			   char *buf)
>> +{
>> +	struct scmi_imx_misc_sys_sleep_rec *rec;
>> +	struct scmi_imx_misc_syslog *syslog;
>> +	int ret;
>> +	size_t len = 0;
>> +
>> +	if (!ph)
>> +		return 0;
>> +
>> +	syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
>> +	if (!syslog)
>> +		return -ENOMEM;
>> +
>> +	ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
>
>...ah...so you basically cast to void a structure of u32 words and then
>expect that the firmware perfectly aligned with how the struct is
>defined here....size checks assures no out-of-bounds BUT the meaning of
>he blob itself is entirely up to FW and kernel being aligned, since no
>type checking is done of any kind and fields are NOT reference by
>name...so may I ask why ?

I thought to use "u32 *syslog", but this needs a cast in
misc_syslog(x,y,(u32 *)syslog), or I could directly change
the misc_syslog function prototype to use 'struct scmi_imx_misc_syslog *'.
No specific reason, just think 'void *' could avoid a cast.

..also because the scmi_imx_misc_syslog seems
>pretty tiny to need iterators to parse back the reply...do you have so
>tiny transpotr message size ?

The transport memory size is 0x80 bytes, it could cover the current
syslog, but I am not sure whether in future, the firmware could
extend to add more information.

In our firmware there is one more field that I not include in this patchset,
because it is default not built in in firmware:
typedef struct
{
    /*! System sleep record */
    dev_sm_sys_sleep_rec_t sysSleepRecord;

    /*! Device error log */
    uint32_t devErrLog;

#ifdef DEV_SM_MSG_PROF_CNT
    /*! Message profiling record */
    dev_sm_sys_msg_rec_t sysMsgRecord;
#endif
} dev_sm_syslog_t;

typedef struct
{
    uint32_t scmiChannel;   /*!< Caller SCMI channel */
    uint32_t chanType;      /*!< SCMI channel type */
    uint32_t protocolId;    /*!< SCMI protocol ID */
    uint32_t msgId;         /*!< SCMI message ID */
    uint32_t msgLatUsec;    /*!< Message latency */
} dev_sm_sys_msg_prof_t;

/*!
 * Message profile record
 */
typedef struct
{
    /*! MSG profile log */
    dev_sm_sys_msg_prof_t msgProf[DEV_SM_MSG_PROF_CNT];
} dev_sm_sys_msg_rec_t;

With profiling,  we need iterator to get all the information.

In our default FW build, DEV_SM_MSG_PROF_CNT is not defined, but
I could keep iterator in case DEV_SM_MSG_PROF_CNT enabled in future.

>
>..anyway I would suggest at least to add some sort of version-field to
>the struct so that at least you can detect if the FW spits out something
>that is not what your driver expect or is ready to handle...especially
>if you plan to extend or modify the layout of the structs in the future.

Would you please share more insights on what version is needed here?
You mean add a field for syslog protocol as below?
struct scmi_imx_misc_syslog {
        uint32_t version; //Set as protocol version and display it in sysfs show ops?
        struct scmi_imx_misc_sys_sleep_rec syssleeprecord;
        uint32_t deverrlog;
};

>
>
>> +	if (ret) {
>> +		kfree(syslog);
>> +		return ret;
>> +	}
>> +
>> +	rec = &syslog->syssleeprecord;
>> +
>> +	len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec->wakesource);
>> +	len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec->syssleepmode);
>> +	len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec->syssleepflags);
>> +	len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n", rec->mixpwrstat);
>> +	len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n", rec->mempwrstat);
>> +	len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n", rec->pllpwrstat);
>> +	len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec->sleepentryusec);
>> +	len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec->sleepexitusec);
>> +	len += sysfs_emit_at(buf, len, "Sleep count = %u\n", rec->sleepcnt);
>> +
>
>... how wonder how much is still frowned upon to serve such big multiline
>structured information payloads from sysfs ... (asking for a friend :P)

Just take what our firmware console displays.

And in case the firmware does not have a dedicated uart on some
boards, using syslog to show similar info would be prefered,
so ...

>
>
>> +	kfree(syslog);
>> +
>> +	return len;
>> +}
>> +
>> +static DEVICE_ATTR_RO(syslog);
>> +
>> +static ssize_t system_info_show(struct device *device, struct device_attribute *attr,
>> +				char *buf)
>> +{
>> +	struct scmi_imx_misc_system_info *info;
>> +	int len = 0;
>> +	int ret;
>> +
>> +	if (!ph)
>> +		return 0;
>> +
>> +	info = kmalloc(sizeof(*info), GFP_KERNEL);
>> +	if (!info)
>> +		return -ENOMEM;
>> +
>> +	ret = imx_misc_ctrl_ops->misc_discover_build_info(ph, info);
>> +	if (ret)
>> +		goto err;
>> +
>> +	ret = imx_misc_ctrl_ops->misc_cfg_info(ph, info);
>> +	if (ret)
>> +		goto err;
>> +
>> +	ret = imx_misc_ctrl_ops->misc_silicon_info(ph, info);
>> +	if (ret)
>> +		goto err;
>> +
>> +	ret = imx_misc_ctrl_ops->misc_board_info(ph, info);
>> +	if (ret)
>> +		goto err;
>> +
>> +	len += sysfs_emit_at(buf, len, "SM Version    = Build %u, Commit 08%x\n",
>> +			     info->buildnum, info->buildcommit);
>> +	len += sysfs_emit_at(buf, len, "SM Config     = %s, mSel=%u\n",
>> +			     info->cfgname, info->msel);
>> +	len += sysfs_emit_at(buf, len, "Silicon       = %s\n", info->siname);
>> +	len += sysfs_emit_at(buf, len, "Board         = %s, attr=0x%08x\n",
>> +			     info->brdname, info->brd_attributes);
>
>...so some of this stuff Build/Silicon/BoaRD info has pretty much
>'forever' lifetime...are you sure you want to query them out of the FW
>each time you issue a sysfs show ?
>
>Cannot you simply dump this stuff once for all at probve time and
>instead query misc_cfg_info() upon each show to refresh only the data
>that will change ?
>
>(this also corroborates my idea that you should somehow partition this
>data into distinct structs by their lifetime...

In next version, I will change to use probe to query the information and
use sysfs to give userspace an interface to get the queried information.

Thanks,
Peng

>
>Thanks,
>Cristian

Re: [PATCH 7/7] firmware: imx: sm-misc: Dump syslog and system info

[kernel test robot]

Hi Peng,

kernel test robot noticed the following build errors:

[auto build test ERROR on ecb259c4f70dd5c83907809f45bf4dc6869961d7]

url:    https://github.com/intel-lab-lkp/linux/commits/Peng-Fan/firmware-arm_scmi-imx-Add-documentation-for-MISC_BOARD_INFO/20250627-140736
base:   ecb259c4f70dd5c83907809f45bf4dc6869961d7
patch link:    https://lore.kernel.org/r/20250627-sm-misc-api-v1-v1-7-2b99481fe825%40nxp.com
patch subject: [PATCH 7/7] firmware: imx: sm-misc: Dump syslog and system info
config: i386-buildonly-randconfig-001-20250628 (https://download.01.org/0day-ci/archive/20250628/202506280217.aZih1pGT-lkp@intel.com/config)
compiler: clang version 20.1.7 (https://github.com/llvm/llvm-project 6146a88f60492b520a36f8f8f3231e15f3cc6082)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250628/202506280217.aZih1pGT-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506280217.aZih1pGT-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/firmware/imx/sm-misc.c:58:11: error: call to undeclared function 'kmalloc'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      58 |         syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
         |                  ^
   drivers/firmware/imx/sm-misc.c:58:11: note: did you mean 'mm_alloc'?
   include/linux/sched/mm.h:16:26: note: 'mm_alloc' declared here
      16 | extern struct mm_struct *mm_alloc(void);
         |                          ^
>> drivers/firmware/imx/sm-misc.c:58:9: error: incompatible integer to pointer conversion assigning to 'struct scmi_imx_misc_syslog *' from 'int' [-Wint-conversion]
      58 |         syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
         |                ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/firmware/imx/sm-misc.c:64:3: error: call to undeclared function 'kfree'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      64 |                 kfree(syslog);
         |                 ^
   drivers/firmware/imx/sm-misc.c:80:2: error: call to undeclared function 'kfree'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      80 |         kfree(syslog);
         |         ^
   drivers/firmware/imx/sm-misc.c:97:9: error: call to undeclared function 'kmalloc'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      97 |         info = kmalloc(sizeof(*info), GFP_KERNEL);
         |                ^
>> drivers/firmware/imx/sm-misc.c:97:7: error: incompatible integer to pointer conversion assigning to 'struct scmi_imx_misc_system_info *' from 'int' [-Wint-conversion]
      97 |         info = kmalloc(sizeof(*info), GFP_KERNEL);
         |              ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/firmware/imx/sm-misc.c:127:2: error: call to undeclared function 'kfree'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     127 |         kfree(info);
         |         ^
   7 errors generated.


vim +/kmalloc +58 drivers/firmware/imx/sm-misc.c

    46	
    47	static ssize_t syslog_show(struct device *device, struct device_attribute *attr,
    48				   char *buf)
    49	{
    50		struct scmi_imx_misc_sys_sleep_rec *rec;
    51		struct scmi_imx_misc_syslog *syslog;
    52		int ret;
    53		size_t len = 0;
    54	
    55		if (!ph)
    56			return 0;
    57	
  > 58		syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
    59		if (!syslog)
    60			return -ENOMEM;
    61	
    62		ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
    63		if (ret) {
  > 64			kfree(syslog);
    65			return ret;
    66		}
    67	
    68		rec = &syslog->syssleeprecord;
    69	
    70		len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec->wakesource);
    71		len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec->syssleepmode);
    72		len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec->syssleepflags);
    73		len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n", rec->mixpwrstat);
    74		len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n", rec->mempwrstat);
    75		len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n", rec->pllpwrstat);
    76		len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec->sleepentryusec);
    77		len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec->sleepexitusec);
    78		len += sysfs_emit_at(buf, len, "Sleep count = %u\n", rec->sleepcnt);
    79	
    80		kfree(syslog);
    81	
    82		return len;
    83	}
    84	
    85	static DEVICE_ATTR_RO(syslog);
    86	
    87	static ssize_t system_info_show(struct device *device, struct device_attribute *attr,
    88					char *buf)
    89	{
    90		struct scmi_imx_misc_system_info *info;
    91		int len = 0;
    92		int ret;
    93	
    94		if (!ph)
    95			return 0;
    96	
  > 97		info = kmalloc(sizeof(*info), GFP_KERNEL);
    98		if (!info)
    99			return -ENOMEM;
   100	
   101		ret = imx_misc_ctrl_ops->misc_discover_build_info(ph, info);
   102		if (ret)
   103			goto err;
   104	
   105		ret = imx_misc_ctrl_ops->misc_cfg_info(ph, info);
   106		if (ret)
   107			goto err;
   108	
   109		ret = imx_misc_ctrl_ops->misc_silicon_info(ph, info);
   110		if (ret)
   111			goto err;
   112	
   113		ret = imx_misc_ctrl_ops->misc_board_info(ph, info);
   114		if (ret)
   115			goto err;
   116	
   117		len += sysfs_emit_at(buf, len, "SM Version    = Build %u, Commit 08%x\n",
   118				     info->buildnum, info->buildcommit);
   119		len += sysfs_emit_at(buf, len, "SM Config     = %s, mSel=%u\n",
   120				     info->cfgname, info->msel);
   121		len += sysfs_emit_at(buf, len, "Silicon       = %s\n", info->siname);
   122		len += sysfs_emit_at(buf, len, "Board         = %s, attr=0x%08x\n",
   123				     info->brdname, info->brd_attributes);
   124	
   125		ret = len;
   126	err:
   127		kfree(info);
   128		return ret;
   129	}
   130	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 7/7] firmware: imx: sm-misc: Dump syslog and system info

[kernel test robot]

Hi Peng,

kernel test robot noticed the following build warnings:

[auto build test WARNING on ecb259c4f70dd5c83907809f45bf4dc6869961d7]

url:    https://github.com/intel-lab-lkp/linux/commits/Peng-Fan/firmware-arm_scmi-imx-Add-documentation-for-MISC_BOARD_INFO/20250627-140736
base:   ecb259c4f70dd5c83907809f45bf4dc6869961d7
patch link:    https://lore.kernel.org/r/20250627-sm-misc-api-v1-v1-7-2b99481fe825%40nxp.com
patch subject: [PATCH 7/7] firmware: imx: sm-misc: Dump syslog and system info
config: x86_64-buildonly-randconfig-005-20250627 (https://download.01.org/0day-ci/archive/20250628/202506282233.n54Z23oc-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250628/202506282233.n54Z23oc-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506282233.n54Z23oc-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/firmware/imx/sm-misc.c: In function 'syslog_show':
   drivers/firmware/imx/sm-misc.c:58:18: error: implicit declaration of function 'kmalloc'; did you mean 'mm_alloc'? [-Werror=implicit-function-declaration]
      58 |         syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
         |                  ^~~~~~~
         |                  mm_alloc
>> drivers/firmware/imx/sm-misc.c:58:16: warning: assignment to 'struct scmi_imx_misc_syslog *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
      58 |         syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
         |                ^
   drivers/firmware/imx/sm-misc.c:64:17: error: implicit declaration of function 'kfree' [-Werror=implicit-function-declaration]
      64 |                 kfree(syslog);
         |                 ^~~~~
   drivers/firmware/imx/sm-misc.c: In function 'system_info_show':
>> drivers/firmware/imx/sm-misc.c:97:14: warning: assignment to 'struct scmi_imx_misc_system_info *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
      97 |         info = kmalloc(sizeof(*info), GFP_KERNEL);
         |              ^
   cc1: some warnings being treated as errors


vim +58 drivers/firmware/imx/sm-misc.c

    46	
    47	static ssize_t syslog_show(struct device *device, struct device_attribute *attr,
    48				   char *buf)
    49	{
    50		struct scmi_imx_misc_sys_sleep_rec *rec;
    51		struct scmi_imx_misc_syslog *syslog;
    52		int ret;
    53		size_t len = 0;
    54	
    55		if (!ph)
    56			return 0;
    57	
  > 58		syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
    59		if (!syslog)
    60			return -ENOMEM;
    61	
    62		ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
    63		if (ret) {
    64			kfree(syslog);
    65			return ret;
    66		}
    67	
    68		rec = &syslog->syssleeprecord;
    69	
    70		len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec->wakesource);
    71		len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec->syssleepmode);
    72		len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec->syssleepflags);
    73		len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n", rec->mixpwrstat);
    74		len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n", rec->mempwrstat);
    75		len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n", rec->pllpwrstat);
    76		len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec->sleepentryusec);
    77		len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec->sleepexitusec);
    78		len += sysfs_emit_at(buf, len, "Sleep count = %u\n", rec->sleepcnt);
    79	
    80		kfree(syslog);
    81	
    82		return len;
    83	}
    84	
    85	static DEVICE_ATTR_RO(syslog);
    86	
    87	static ssize_t system_info_show(struct device *device, struct device_attribute *attr,
    88					char *buf)
    89	{
    90		struct scmi_imx_misc_system_info *info;
    91		int len = 0;
    92		int ret;
    93	
    94		if (!ph)
    95			return 0;
    96	
  > 97		info = kmalloc(sizeof(*info), GFP_KERNEL);
    98		if (!info)
    99			return -ENOMEM;
   100	
   101		ret = imx_misc_ctrl_ops->misc_discover_build_info(ph, info);
   102		if (ret)
   103			goto err;
   104	
   105		ret = imx_misc_ctrl_ops->misc_cfg_info(ph, info);
   106		if (ret)
   107			goto err;
   108	
   109		ret = imx_misc_ctrl_ops->misc_silicon_info(ph, info);
   110		if (ret)
   111			goto err;
   112	
   113		ret = imx_misc_ctrl_ops->misc_board_info(ph, info);
   114		if (ret)
   115			goto err;
   116	
   117		len += sysfs_emit_at(buf, len, "SM Version    = Build %u, Commit 08%x\n",
   118				     info->buildnum, info->buildcommit);
   119		len += sysfs_emit_at(buf, len, "SM Config     = %s, mSel=%u\n",
   120				     info->cfgname, info->msel);
   121		len += sysfs_emit_at(buf, len, "Silicon       = %s\n", info->siname);
   122		len += sysfs_emit_at(buf, len, "Board         = %s, attr=0x%08x\n",
   123				     info->brdname, info->brd_attributes);
   124	
   125		ret = len;
   126	err:
   127		kfree(info);
   128		return ret;
   129	}
   130	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 7/7] firmware: imx: sm-misc: Dump syslog and system info

[Sudeep Holla]

On Fri, Jun 27, 2025 at 02:03:50PM +0800, Peng Fan wrote:
> Add sysfs interface to read System Manager syslog and system info
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/firmware/imx/sm-misc.c | 97 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
> index fc3ee12c2be878e0285183e3381c9514a63d5142..55485a3c4a5c615102a377f41025a6911d746770 100644
> --- a/drivers/firmware/imx/sm-misc.c
> +++ b/drivers/firmware/imx/sm-misc.c
> @@ -44,6 +44,100 @@ static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
>  	return 0;
>  }
>  
> +static ssize_t syslog_show(struct device *device, struct device_attribute *attr,
> +			   char *buf)
> +{
> +	struct scmi_imx_misc_sys_sleep_rec *rec;
> +	struct scmi_imx_misc_syslog *syslog;
> +	int ret;
> +	size_t len = 0;
> +
> +	if (!ph)
> +		return 0;
> +
> +	syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
> +	if (!syslog)
> +		return -ENOMEM;
> +
> +	ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
> +	if (ret) {
> +		kfree(syslog);
> +		return ret;
> +	}
> +
> +	rec = &syslog->syssleeprecord;
> +
> +	len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec->wakesource);
> +	len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec->syssleepmode);
> +	len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec->syssleepflags);
> +	len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n", rec->mixpwrstat);
> +	len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n", rec->mempwrstat);
> +	len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n", rec->pllpwrstat);
> +	len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec->sleepentryusec);
> +	len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec->sleepexitusec);
> +	len += sysfs_emit_at(buf, len, "Sleep count = %u\n", rec->sleepcnt);
> +

Why can't this be individual files and values ?

-- 
Regards,
Sudeep

Re: [PATCH 7/7] firmware: imx: sm-misc: Dump syslog and system info

[Peng Fan]

On Wed, Jul 02, 2025 at 04:22:47PM +0100, Sudeep Holla wrote:
>On Fri, Jun 27, 2025 at 02:03:50PM +0800, Peng Fan wrote:
>> Add sysfs interface to read System Manager syslog and system info
>> 
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>  drivers/firmware/imx/sm-misc.c | 97 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 97 insertions(+)
>> 
>> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
>> index fc3ee12c2be878e0285183e3381c9514a63d5142..55485a3c4a5c615102a377f41025a6911d746770 100644
>> --- a/drivers/firmware/imx/sm-misc.c
>> +++ b/drivers/firmware/imx/sm-misc.c
>> @@ -44,6 +44,100 @@ static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
>>  	return 0;
>>  }
>>  
>> +static ssize_t syslog_show(struct device *device, struct device_attribute *attr,
>> +			   char *buf)
>> +{
>> +	struct scmi_imx_misc_sys_sleep_rec *rec;
>> +	struct scmi_imx_misc_syslog *syslog;
>> +	int ret;
>> +	size_t len = 0;
>> +
>> +	if (!ph)
>> +		return 0;
>> +
>> +	syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
>> +	if (!syslog)
>> +		return -ENOMEM;
>> +
>> +	ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
>> +	if (ret) {
>> +		kfree(syslog);
>> +		return ret;
>> +	}
>> +
>> +	rec = &syslog->syssleeprecord;
>> +
>> +	len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec->wakesource);
>> +	len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec->syssleepmode);
>> +	len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec->syssleepflags);
>> +	len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n", rec->mixpwrstat);
>> +	len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n", rec->mempwrstat);
>> +	len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n", rec->pllpwrstat);
>> +	len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec->sleepentryusec);
>> +	len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec->sleepexitusec);
>> +	len += sysfs_emit_at(buf, len, "Sleep count = %u\n", rec->sleepcnt);
>> +
>
>Why can't this be individual files and values ?

The System manager firmware provides a command "syslog" to dump
all the information, I just follow same.

With individual files, some values may not be consistent, because
system behaviour may change during reading two files.

Regards,
Peng

>
>-- 
>Regards,
>Sudeep

Re: [PATCH 7/7] firmware: imx: sm-misc: Dump syslog and system info

[Sudeep Holla]

On Fri, Jul 04, 2025 at 06:28:02PM +0800, Peng Fan wrote:
> On Wed, Jul 02, 2025 at 04:22:47PM +0100, Sudeep Holla wrote:
> >On Fri, Jun 27, 2025 at 02:03:50PM +0800, Peng Fan wrote:
> >> Add sysfs interface to read System Manager syslog and system info
> >> 
> >> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >> ---
> >>  drivers/firmware/imx/sm-misc.c | 97 ++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 97 insertions(+)
> >> 
> >> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
> >> index fc3ee12c2be878e0285183e3381c9514a63d5142..55485a3c4a5c615102a377f41025a6911d746770 100644
> >> --- a/drivers/firmware/imx/sm-misc.c
> >> +++ b/drivers/firmware/imx/sm-misc.c
> >> @@ -44,6 +44,100 @@ static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
> >>  	return 0;
> >>  }
> >>  
> >> +static ssize_t syslog_show(struct device *device, struct device_attribute *attr,
> >> +			   char *buf)
> >> +{
> >> +	struct scmi_imx_misc_sys_sleep_rec *rec;
> >> +	struct scmi_imx_misc_syslog *syslog;
> >> +	int ret;
> >> +	size_t len = 0;
> >> +
> >> +	if (!ph)
> >> +		return 0;
> >> +
> >> +	syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
> >> +	if (!syslog)
> >> +		return -ENOMEM;
> >> +
> >> +	ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
> >> +	if (ret) {
> >> +		kfree(syslog);
> >> +		return ret;
> >> +	}
> >> +
> >> +	rec = &syslog->syssleeprecord;
> >> +
> >> +	len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec->wakesource);
> >> +	len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec->syssleepmode);
> >> +	len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec->syssleepflags);
> >> +	len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n", rec->mixpwrstat);
> >> +	len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n", rec->mempwrstat);
> >> +	len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n", rec->pllpwrstat);
> >> +	len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec->sleepentryusec);
> >> +	len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec->sleepexitusec);
> >> +	len += sysfs_emit_at(buf, len, "Sleep count = %u\n", rec->sleepcnt);
> >> +
> >
> >Why can't this be individual files and values ?
> 
> The System manager firmware provides a command "syslog" to dump
> all the information, I just follow same.
> 
> With individual files, some values may not be consistent, because
> system behaviour may change during reading two files.
> 

You definitely need to convince Greg if you take this approach. I am sure
he prefers single value sysfs files.

-- 
Regards,
Sudeep

RE: [PATCH 7/7] firmware: imx: sm-misc: Dump syslog and system info

[Peng Fan]

> Subject: Re: [PATCH 7/7] firmware: imx: sm-misc: Dump syslog and
> system info
> 
> On Fri, Jul 04, 2025 at 06:28:02PM +0800, Peng Fan wrote:
> > On Wed, Jul 02, 2025 at 04:22:47PM +0100, Sudeep Holla wrote:
> > >On Fri, Jun 27, 2025 at 02:03:50PM +0800, Peng Fan wrote:
> > >> Add sysfs interface to read System Manager syslog and system
> info
> > >>
> > >> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > >> ---
> > >>  drivers/firmware/imx/sm-misc.c | 97
> > >> ++++++++++++++++++++++++++++++++++++++++++
> > >>  1 file changed, 97 insertions(+)
> > >>
> > >> diff --git a/drivers/firmware/imx/sm-misc.c
> > >> b/drivers/firmware/imx/sm-misc.c index
> > >>
> fc3ee12c2be878e0285183e3381c9514a63d5142..55485a3c4a5c6151
> 02a377f41
> > >> 025a6911d746770 100644
> > >> --- a/drivers/firmware/imx/sm-misc.c
> > >> +++ b/drivers/firmware/imx/sm-misc.c
> > >> @@ -44,6 +44,100 @@ static int
> scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
> > >>  	return 0;
> > >>  }
> > >>
> > >> +static ssize_t syslog_show(struct device *device, struct
> device_attribute *attr,
> > >> +			   char *buf)
> > >> +{
> > >> +	struct scmi_imx_misc_sys_sleep_rec *rec;
> > >> +	struct scmi_imx_misc_syslog *syslog;
> > >> +	int ret;
> > >> +	size_t len = 0;
> > >> +
> > >> +	if (!ph)
> > >> +		return 0;
> > >> +
> > >> +	syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
> > >> +	if (!syslog)
> > >> +		return -ENOMEM;
> > >> +
> > >> +	ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog),
> syslog);
> > >> +	if (ret) {
> > >> +		kfree(syslog);
> > >> +		return ret;
> > >> +	}
> > >> +
> > >> +	rec = &syslog->syssleeprecord;
> > >> +
> > >> +	len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec-
> >wakesource);
> > >> +	len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec-
> >syssleepmode);
> > >> +	len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec-
> >syssleepflags);
> > >> +	len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n",
> rec->mixpwrstat);
> > >> +	len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n",
> rec->mempwrstat);
> > >> +	len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n",
> rec->pllpwrstat);
> > >> +	len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec-
> >sleepentryusec);
> > >> +	len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec-
> >sleepexitusec);
> > >> +	len += sysfs_emit_at(buf, len, "Sleep count = %u\n",
> > >> +rec->sleepcnt);
> > >> +
> > >
> > >Why can't this be individual files and values ?
> >
> > The System manager firmware provides a command "syslog" to
> dump all
> > the information, I just follow same.
> >
> > With individual files, some values may not be consistent, because
> > system behaviour may change during reading two files.
> >
> 
> You definitely need to convince Greg if you take this approach. I am
> sure he prefers single value sysfs files.

ok. Thanks for sharing me the info. I was not aware of this.
Let me follow single value for sysfs files in next version.

Thanks,
Peng.

> 
> --
> Regards,
> Sudeep