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

[Daniel Baluta]

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.

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?


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

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