[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250831082839.GF17728@nxa18884-linux.ap.freescale.net>
Date: Sun, 31 Aug 2025 16:28:39 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Sudeep Holla <sudeep.holla@....com>
Cc: Peng Fan <peng.fan@....com>,
Cristian Marussi <cristian.marussi@....com>,
Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>, arm-scmi@...r.kernel.org,
imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 5/6] firmware: arm_scmi: imx: Support getting syslog
of MISC protocol
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@....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
Powered by blists - more mailing lists