> Subject: Re: [PATCH v4 4/6] firmware: arm_scmi: add initial support for > i.MX MISC protocol > > On Fri, May 24, 2024 at 04:56:46PM +0800, Peng Fan (OSS) wrote: > > From: Peng Fan <peng.fan@xxxxxxx> > > > > i.MX95 System Manager(SM) firmware includes a SCMI vendor > protocol, > > SCMI MISC protocol which includes controls that are misc > > settings/actions that must be exposed from the SM to agents. They > are > > device specific and are usually define to access bit fields in various > > mix block control modules, IOMUX_GPR, and other General Purpose > > registers, Control Status Registers owned by the SM. > > ..... > > + > > +static int scmi_imx_misc_ctrl_validate_id(const struct > scmi_protocol_handle *ph, > > + u32 ctrl_id) > > +{ > > + struct scmi_imx_misc_info *mi = ph->get_priv(ph); > > + > > + if ((ctrl_id < BRD_CTRL_START_ID) && (ctrl_id > mi- > >nr_dev_ctrl)) > > + return -EINVAL; > > + if (ctrl_id >= BRD_CTRL_START_ID + mi->nr_brd_ctrl) > > + return -EINVAL; > > ...are these conditions fine ? Yes. they are correct. just checking because they seem a bit > odd...but I am certainly missing something...in case they are ok, is it > possible to add a comment explaining why those conds lead to - > EINVAL ? We have a flag "MISC_CTRL_FLAG_BRD 0x8000U" to indicate it is board ctrl or non-board ctrl > > > + > > + return 0; > > +} > > + > > +static int scmi_imx_misc_ctrl_notify(const struct > scmi_protocol_handle *ph, > > + u32 ctrl_id, u32 evt_id, u32 flags) > { > > + struct scmi_imx_misc_ctrl_notify_in *in; > > + struct scmi_xfer *t; > > + int ret; > > + > > + ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id); > > + if (ret) > > + return ret; > > + > > + ret = ph->xops->xfer_get_init(ph, > SCMI_IMX_MISC_CTRL_NOTIFY, > > + sizeof(*in), 0, &t); > > + if (ret) > > + return ret; > > + > > + in = t->tx.buf; > > + in->ctrl_id = cpu_to_le32(ctrl_id); > > + in->flags = cpu_to_le32(flags); > > + > > + ret = ph->xops->do_xfer(ph, t); > > + > > + ph->xops->xfer_put(ph, t); > > + > > + return ret; > > +} > > + > > +static int > > +scmi_imx_misc_ctrl_set_notify_enabled(const struct > scmi_protocol_handle *ph, > > + u8 evt_id, u32 src_id, bool enable) > { > > + int ret; > > + > > + /* misc_ctrl_req_notify is for enablement */ > > + if (enable) > > + return 0; > > + > > + ret = scmi_imx_misc_ctrl_notify(ph, src_id, evt_id, 0); > > + if (ret) > > + dev_err(ph->dev, "FAIL_ENABLED - evt[%X] src[%d] - > ret:%d\n", > > + evt_id, src_id, ret); > > + > > + return ret; > > +} > > + > > +static int scmi_imx_misc_ctrl_get_num_sources(const struct > > +scmi_protocol_handle *ph) { > > + return GENMASK(15, 0); > > +} > > This is statically defined at compile time..you dont need to provide this > method, which is just for discover number of possible event sources at > runtime....just drop it and use .num_sources in scmi_protocol_events > I see. Fix in v5. > > + > > +static void * > > +scmi_imx_misc_ctrl_fill_custom_report(const struct > scmi_protocol_handle *ph, > > + u8 evt_id, ktime_t timestamp, > > + const void *payld, size_t payld_sz, > > + void *report, u32 *src_id) > > +{ > > + const struct scmi_imx_misc_ctrl_notify_payld *p = payld; > > + struct scmi_imx_misc_ctrl_notify_report *r = report; > > + > > + if (sizeof(*p) != payld_sz) > > + return NULL; > > + > > + r->timestamp = timestamp; > > + r->ctrl_id = p->ctrl_id; > > + r->flags = p->flags; > > + if (src_id) > > + *src_id = r->ctrl_id; > > + dev_dbg(ph->dev, "%s: ctrl_id: %d flags: %d\n", __func__, > > + r->ctrl_id, r->flags); > > + > > + return r; > > +} > > + > > +static const struct scmi_event_ops scmi_imx_misc_event_ops = { > > + .get_num_sources = scmi_imx_misc_ctrl_get_num_sources, > drop > > > + .set_notify_enabled = scmi_imx_misc_ctrl_set_notify_enabled, > > + .fill_custom_report = scmi_imx_misc_ctrl_fill_custom_report, > > +}; > > + > > +static const struct scmi_event scmi_imx_misc_events[] = { > > + { > > + .id = SCMI_EVENT_IMX_MISC_CONTROL, > > + .max_payld_sz = sizeof(struct > scmi_imx_misc_ctrl_notify_payld), > > + .max_report_sz = sizeof(struct > scmi_imx_misc_ctrl_notify_report), > > + }, > > +}; > > + > > +static struct scmi_protocol_events scmi_imx_misc_protocol_events > = { > > + .queue_sz = SCMI_PROTO_QUEUE_SZ, > > + .ops = &scmi_imx_misc_event_ops, > > + .evts = scmi_imx_misc_events, > > + .num_events = ARRAY_SIZE(scmi_imx_misc_events), > > .num_sources = MAX_MISC_CTRL_SOURCES, // GENMASK(15, > 0) > > > +}; > > + > > +static int scmi_imx_misc_protocol_init(const struct > > +scmi_protocol_handle *ph) { > > + struct scmi_imx_misc_info *minfo; > > + u32 version; > > + int ret; > > + > > + ret = ph->xops->version_get(ph, &version); > > + if (ret) > > + return ret; > > + > > + dev_info(ph->dev, "NXP SM MISC Version %d.%d\n", > > + PROTOCOL_REV_MAJOR(version), > PROTOCOL_REV_MINOR(version)); > > + > > + minfo = devm_kzalloc(ph->dev, sizeof(*minfo), GFP_KERNEL); > > + if (!minfo) > > + return -ENOMEM; > > + > > + ret = scmi_imx_misc_attributes_get(ph, minfo); > > + if (ret) > > + return ret; > > + > > + return ph->set_priv(ph, minfo, version); } > > Same as previous patch please move the init downb below near the > scmi_protocol struct right after the ops > Yeah. Fix in v5. > > + > > +static int scmi_imx_misc_ctrl_get(const struct scmi_protocol_handle > *ph, > > + u32 ctrl_id, u32 *num, u32 *val) { > > + struct scmi_imx_misc_ctrl_get_out *out; > > + struct scmi_xfer *t; > > + int ret, i; > > + > > + ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id); > > + if (ret) > > + return ret; > > + > > + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_GET, > sizeof(u32), > > + 0, &t); > > + if (ret) > > + return ret; > > + > > + put_unaligned_le32(ctrl_id, t->tx.buf); > > + ret = ph->xops->do_xfer(ph, t); > > + if (!ret) { > > + out = t->rx.buf; > > + *num = le32_to_cpu(out->num); > > To stay even more safer, by guarding from malformed *num fields and > just bail out upfront with an error > > if (*num >= MISC_MAX_VAL || > *num * sizeof(__le32) > t->rx.len - sizeof(__le32)) > > and then just > > for (i = 0; i < *num; i++) > ok. looks good. I will fix in v5. > > + for (i = 0; i < *num && i < MISC_MAX_VAL; i++) > > + val[i] = le32_to_cpu(out->val[i]); > > + } > > + > > + ph->xops->xfer_put(ph, t); > > + > > + return ret; > > +} > > + > > +static int scmi_imx_misc_ctrl_set(const struct scmi_protocol_handle > *ph, > > + u32 ctrl_id, u32 num, u32 *val) { > > + struct scmi_imx_misc_ctrl_set_in *in; > > + struct scmi_xfer *t; > > + int ret, i; > > + > > + ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id); > > + if (ret) > > + return ret; > > + > > + if (num > MISC_MAX_VAL) > > + return -EINVAL; > > + > > + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_SET, > sizeof(*in), > > + 0, &t); > > + if (ret) > > + return ret; > > + > > + in = t->tx.buf; > > + in->id = cpu_to_le32(ctrl_id); > > + in->num = cpu_to_le32(num); > > + for (i = 0; i < num; i++) > > + in->value[i] = cpu_to_le32(val[i]); > > + > > + ret = ph->xops->do_xfer(ph, t); > > + > > + 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, }; > > + > > +static const struct scmi_protocol scmi_imx_misc = { > > + .id = SCMI_PROTOCOL_IMX_MISC, > > + .owner = THIS_MODULE, > > + .instance_init = &scmi_imx_misc_protocol_init, > > + .ops = &scmi_imx_misc_proto_ops, > > + .events = &scmi_imx_misc_protocol_events, > > + .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION, > > + .vendor_id = "NXP", > > + .sub_vendor_id = "i.MX95 EVK", > > +}; > > +module_scmi_protocol(scmi_imx_misc); > > diff --git a/include/linux/scmi_imx_protocol.h > > b/include/linux/scmi_imx_protocol.h > > index e59aedaa4aec..e9285abfc191 100644 > > --- a/include/linux/scmi_imx_protocol.h > > +++ b/include/linux/scmi_imx_protocol.h > > @@ -13,8 +13,14 @@ > > #include <linux/notifier.h> > > #include <linux/types.h> > > > > +#define SCMI_PAYLOAD_LEN 100 > > + > > +#define SCMI_ARRAY(X, Y) ((SCMI_PAYLOAD_LEN - (X)) / > sizeof(Y)) > > +#define MISC_MAX_VAL SCMI_ARRAY(8, uint32_t) > > > You base all of this on this fixed payload length, but the payload really > depends on the configured underlying transport: you can use the > ph->hops->get_max_msg_size to retrieve the configured max payload > length > for the platform you are running on....nad maybe bailout if the > minimum size required by your protocol is not available with the > currently configured transport...wouldnt't be more robust and reliable > then builtin fixing some payload ? Your point is valid. But I need check our firmware design and see if feasible and update. Thanks, Peng. > > Thanks, > Cristian