RE: [PATCH v4 4/6] firmware: arm_scmi: add initial support for i.MX MISC protocol

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux