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

[Peng Fan <peng.fan@oss.nxp.com>]

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

Fix in V4.

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

Sure. Fix in V4.

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

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

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

Fix in V4

Thanks,
Peng

>
>-- 
>Regards,
>Sudeep