From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 28296C2BB85 for ; Mon, 17 Jun 2024 18:15:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=qQXgy6rFzG5xTKw7SI3Yy+3E9HThwENwTjqgv93eox0=; b=XxAl589T3d3Cxld4DI0DW6H6Zx 1eU2ftQwJH1H4uLlfMlZHvUpI0DLaFTceGO1Pjb9pZYjYM7Dl89fzfbNTwNifVGgwLWO9V2vxL7Jv JhciRLbWjUf7oktyST1RQgVbFlbv1TlrORkDqof9oilUoquR4d/c/sqFA1ByPTmFK1JZPZZCmvvrB EzvEt6b+aAZRV/Gp1YtaxlLLGsmpA5TIQIvcNQXMDJTjCnkikByB6zBbd59z4IrSu/ZhzXh9Faiyy j8pQOJJq8e3WKVzMM7ssg2xe13jIVKfe4BxwMbQDar3WGbKDArvzQNLXS4cLTcW4Ff/0eoKKTqatm QT4Ro1FA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sJGsl-0000000C7Zu-1866; Mon, 17 Jun 2024 18:15:07 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sJGsc-0000000C7Wo-3Vpo for linux-arm-kernel@lists.infradead.org; Mon, 17 Jun 2024 18:15:05 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 9829EDA7; Mon, 17 Jun 2024 11:15:21 -0700 (PDT) Received: from pluto (usa-sjc-mx-foss1.foss.arm.com [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7CFDE3F64C; Mon, 17 Jun 2024 11:14:41 -0700 (PDT) Date: Mon, 17 Jun 2024 19:14:24 +0100 From: Cristian Marussi To: "Peng Fan (OSS)" Cc: Jonathan Corbet , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , Sudeep Holla , Cristian Marussi , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Peng Fan , linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org Subject: Re: [PATCH v4 4/6] firmware: arm_scmi: add initial support for i.MX MISC protocol Message-ID: References: <20240524-imx95-bbm-misc-v2-v4-0-dc456995d590@nxp.com> <20240524-imx95-bbm-misc-v2-v4-4-dc456995d590@nxp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240524-imx95-bbm-misc-v2-v4-4-dc456995d590@nxp.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240617_111459_000067_E86039E0 X-CRM114-Status: GOOD ( 34.21 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, May 24, 2024 at 04:56:46PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan > > 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. > > Signed-off-by: Peng Fan > --- > drivers/firmware/arm_scmi/imx/Kconfig | 9 + > drivers/firmware/arm_scmi/imx/Makefile | 1 + > drivers/firmware/arm_scmi/imx/imx-sm-misc.c | 303 ++++++++++++++++++++++++++++ > include/linux/scmi_imx_protocol.h | 22 ++ > 4 files changed, 335 insertions(+) > Hi, > diff --git a/drivers/firmware/arm_scmi/imx/Kconfig b/drivers/firmware/arm_scmi/imx/Kconfig > index 4b6ac7febe8f..e9d015859eaa 100644 > --- a/drivers/firmware/arm_scmi/imx/Kconfig > +++ b/drivers/firmware/arm_scmi/imx/Kconfig > @@ -11,4 +11,13 @@ config IMX_SCMI_BBM_EXT > > This driver can also be built as a module. > > +config IMX_SCMI_MISC_EXT > + tristate "i.MX SCMI MISC EXTENSION" > + depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF) > + default y if ARCH_MXC > + help > + This enables i.MX System MISC control logic such as gpio expander > + wakeup > + > + This driver can also be built as a module. > endmenu > diff --git a/drivers/firmware/arm_scmi/imx/Makefile b/drivers/firmware/arm_scmi/imx/Makefile > index a7dbdd20dbb9..d3ee6d544924 100644 > --- a/drivers/firmware/arm_scmi/imx/Makefile > +++ b/drivers/firmware/arm_scmi/imx/Makefile > @@ -1,2 +1,3 @@ > # SPDX-License-Identifier: GPL-2.0-only > obj-$(CONFIG_IMX_SCMI_BBM_EXT) += imx-sm-bbm.o > +obj-$(CONFIG_IMX_SCMI_MISC_EXT) += imx-sm-misc.o > diff --git a/drivers/firmware/arm_scmi/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/imx/imx-sm-misc.c > new file mode 100644 > index 000000000000..9d0063299310 > --- /dev/null > +++ b/drivers/firmware/arm_scmi/imx/imx-sm-misc.c > @@ -0,0 +1,303 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * System control and Management Interface (SCMI) NXP MISC Protocol > + * > + * Copyright 2024 NXP > + */ > + > +#define pr_fmt(fmt) "SCMI Notifications MISC - " fmt > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "../protocols.h" > +#include "../notify.h" > + > +#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x10000 > + > +enum scmi_imx_misc_protocol_cmd { > + SCMI_IMX_MISC_CTRL_SET = 0x3, > + SCMI_IMX_MISC_CTRL_GET = 0x4, > + SCMI_IMX_MISC_CTRL_NOTIFY = 0x8, > +}; > + > +struct scmi_imx_misc_info { > + u32 version; > + u32 nr_dev_ctrl; > + u32 nr_brd_ctrl; > + u32 nr_reason; > +}; > + > +struct scmi_msg_imx_misc_protocol_attributes { > + __le32 attributes; > +}; > + > +#define GET_BRD_CTRLS_NR(x) le32_get_bits((x), GENMASK(31, 24)) > +#define GET_REASONS_NR(x) le32_get_bits((x), GENMASK(23, 16)) > +#define GET_DEV_CTRLS_NR(x) le32_get_bits((x), GENMASK(15, 0)) > +#define BRD_CTRL_START_ID BIT(15) > + > +struct scmi_imx_misc_ctrl_set_in { > + __le32 id; > + __le32 num; > + __le32 value[MISC_MAX_VAL]; > +}; > + > +struct scmi_imx_misc_ctrl_notify_in { > + __le32 ctrl_id; > + __le32 flags; > +}; > + > +struct scmi_imx_misc_ctrl_notify_payld { > + __le32 ctrl_id; > + __le32 flags; > +}; > + > +struct scmi_imx_misc_ctrl_get_out { > + __le32 num; > + __le32 val[MISC_MAX_VAL]; > +}; > + > +static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph, > + struct scmi_imx_misc_info *mi) > +{ > + int ret; > + struct scmi_xfer *t; > + struct scmi_msg_imx_misc_protocol_attributes *attr; > + > + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, > + sizeof(*attr), &t); > + if (ret) > + return ret; > + > + attr = t->rx.buf; > + > + ret = ph->xops->do_xfer(ph, t); > + if (!ret) { > + mi->nr_dev_ctrl = GET_DEV_CTRLS_NR(attr->attributes); > + mi->nr_brd_ctrl = GET_BRD_CTRLS_NR(attr->attributes); > + mi->nr_reason = GET_REASONS_NR(attr->attributes); > + dev_info(ph->dev, "i.MX MISC NUM DEV CTRL: %d, NUM BRD CTRL: %d,NUM Reason: %d\n", > + mi->nr_dev_ctrl, mi->nr_brd_ctrl, mi->nr_reason); > + } > + > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +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 ? 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 ? > + > + 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 > + > +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 > + > +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++) > + 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 > #include > > +#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 ? Thanks, Cristian