Re: [PATCH 1/2] firmware: arm_scmi: imx: Support getting reset reason of MISC protocol

[Peng Fan]

Hi Daniel,

Thanks for giving a look.

On Thu, Mar 05, 2026 at 08:52:41AM +0200, Daniel Baluta wrote:
>On Thu, Mar 5, 2026 at 3:55 AM Peng Fan (OSS) <peng.fan@xxxxxxxxxxx> wrote:
>>
>> From: Peng Fan <peng.fan@xxxxxxx>
>>
>> MISC protocol supports getting reset reason per Logical Machine or
>> System. Add the API for user to retrieve the information from System
>> Manager.
>
>[..]
>
>> +struct scmi_imx_misc_reset_reason_in {
>> +#define MISC_REASON_FLAG_SYSTEM        BIT(0)
>> +       __le32 flags;
>> +};
>> +
>> +struct scmi_imx_misc_reset_reason_out {
>> +       /* Boot reason flags */
>> +#define MISC_BOOT_FLAG_VLD             BIT(31)
>> +#define MISC_BOOT_FLAG_ORG_VLD         BIT(28)
>> +#define MISC_BOOT_FLAG_ORIGIN          GENMASK(27, 24)
>> +#define MISC_BOOT_FLAG_O_SHIFT         24
>> +#define MISC_BOOT_FLAG_ERR_VLD         BIT(23)
>> +#define MISC_BOOT_FLAG_ERR_ID          GENMASK(22, 8)
>> +#define MISC_BOOT_FLAG_E_SHIFT         8
>> +#define MISC_BOOT_FLAG_REASON          GENMASK(7, 0)
>
>I would move this macros outside of the struct. Although the intention
>is good it makes everything hard to read.

I followed arm_scmi coding style, such as
drivers/firmware/arm_scmi/clock.c
drivers/firmware/arm_scmi/perf.c
and etc.

I would not break the rule here.
>
>Just do:
>
>struct scmi_imx_misc_reset_reason_out {
>__le32 b_flags; /* MISC_BOOT_FLAG_* flags */
>__le32 s_flags; /* MISC_SHUTDOWN_FLAG_* flags */
>      /* Array of extended info words */
>       __le32 extinfo[MISC_EXT_INFO_LEN_MAX];
>}
>
>[...]
>
>> +static int scmi_imx_misc_reset_reason(const struct scmi_protocol_handle *ph, bool system,
>> +                                     struct scmi_imx_misc_reset_reason *boot_r,
>> +                                     struct scmi_imx_misc_reset_reason *shut_r,
>> +                                     u32 *extinfo)
>> +{
>> +       struct scmi_imx_misc_reset_reason_in *in;
>> +       struct scmi_imx_misc_reset_reason_out *out;
>> +       struct scmi_xfer *t;
>> +       int ret;
>> +
>> +       ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_RESET_REASON_GET, sizeof(*in),
>> +                                     sizeof(*out), &t);
>> +       if (ret)
>> +               return ret;
>> +
>> +       in = t->tx.buf;
>> +       if (system)
>> +               in->flags = le32_encode_bits(1, MISC_REASON_FLAG_SYSTEM);
>> +       else
>> +               in->flags = cpu_to_le32(0);
>
>What does system = 0 mean? can you directly do in->flags = 0?

If system is false, that means to get LM(logial machine) reset reason.
If system is true, that means to get system reset reason.

grep "cpu_to_le32(0)" ./drivers/firmware/arm_scmi/ -rn

You will see other usage, I just follow same style here.

>
>
>> +
>> +       ret = ph->xops->do_xfer(ph, t);
>
>Is it mandatory to call  ph->xops->xfer_put(ph, t); even if ret ! = 0?
>Because we can get rid of one level of indentation with:

yes, xfer_put is mandatory.

Thanks,
Peng.

>
> ret = ph->xops->do_xfer(ph, t);
> if (ret)
>    return ret;