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 E668FC48BF6 for ; Mon, 26 Feb 2024 13:32:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=xnE2VSQf8GXrK6stKZxQulNQtrHTBfEd18XJzpjhETk=; b=1GZCR6ljquid2a kapXEi9bbkYJf3bFu4Pi1lcfJ+dEYb7ze7cS89iy8/DBk2gFlXxAsUBEBksZ623EsH5ajFB10m5Qb aG8nf1iEvQw174ayiJnmRGaKKo6B1NQs9IfDqZt9mB4oD80PAu4HB+K/eVWOKCW0KyuPF5s1wm36W ixVdaYMZAklfpmIsd5Cpp0KM90pOLBtNYAfbSLCjlMBQyEzhxvd9PZjaFMoDOMS+N9ej1eFdkoWKr j2RxUrTZYHLwx7RxKz6mSl7PuuwnuCxFV5l6ZZy6sGxRIr5Hi6ArJbfrpNmDmgIRZsQC57XLEsfj/ xQ//CvgWKUzai0+N9CPg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1reb5U-00000000qFE-1CQt; Mon, 26 Feb 2024 13:32:08 +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 1reb5Q-00000000qEp-3RGy for linux-arm-kernel@lists.infradead.org; Mon, 26 Feb 2024 13:32:06 +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 80E17DA7; Mon, 26 Feb 2024 05:32:40 -0800 (PST) Received: from pluto (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 195D03F762; Mon, 26 Feb 2024 05:31:59 -0800 (PST) Date: Mon, 26 Feb 2024 13:31:58 +0000 From: Cristian Marussi To: "Peng Fan (OSS)" Cc: Rob Herring , Krzysztof Kozlowski , Conor Dooley , Sudeep Holla , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Peng Fan Subject: Re: [PATCH 5/5] firmware: imx: add i.MX95 MISC driver Message-ID: References: <20240202-imx95-bbm-misc-v1-0-3cb743020933@nxp.com> <20240202-imx95-bbm-misc-v1-5-3cb743020933@nxp.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240226_053204_975219_431BFC0F X-CRM114-Status: GOOD ( 41.04 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Feb 23, 2024 at 06:35:26PM +0000, Cristian Marussi wrote: > On Fri, Feb 02, 2024 at 02:34:43PM +0800, Peng Fan (OSS) wrote: > > From: Peng Fan > > > > The i.MX95 System manager exports SCMI MISC protocol for linux to do > > various settings, such as set board gpio expander as wakeup source. > > > > Hi, > > > The driver is to add the support. > > > > Signed-off-by: Peng Fan > > --- > > drivers/firmware/imx/Makefile | 1 + > > drivers/firmware/imx/sm-misc.c | 92 +++++++++++++++++++++++++++++++++++++++++ > > include/linux/firmware/imx/sm.h | 33 +++++++++++++++ > > 3 files changed, 126 insertions(+) > > > > diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile > > index fb20e22074e1..cb9c361d9b81 100644 > > --- a/drivers/firmware/imx/Makefile > > +++ b/drivers/firmware/imx/Makefile > > @@ -2,3 +2,4 @@ > > obj-$(CONFIG_IMX_DSP) += imx-dsp.o > > obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o imx-scu-irq.o rm.o imx-scu-soc.o > > obj-${CONFIG_IMX_SCMI_BBM_EXT} += sm-bbm.o > > +obj-${CONFIG_IMX_SCMI_MISC_EXT} += sm-misc.o > > Same considerations about missing Kconfig as in BBM and implicit > dependency on the NXP MISC vendor module...this way also you cannot even > NOT compile this module when the Vendor protocol is compiled in for, > say, testing purposes... > > > diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c > > new file mode 100644 > > index 000000000000..4410e69d256b > > --- /dev/null > > +++ b/drivers/firmware/imx/sm-misc.c > > @@ -0,0 +1,92 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright 2024 NXP. > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +static const struct scmi_imx_misc_proto_ops *imx_misc_ctrl_ops; > > +static struct scmi_protocol_handle *ph; > > This global does NOT sound right...if there are multiple SCMI instances > defined in the DT this can be probed multiple times, and the MISC > protocol will be initialized multuple times, each instance will have > its distinct protocol_handle *ph...so store it somewhere like you did in > the BBM driver > > > +struct notifier_block scmi_imx_misc_ctrl_nb; > > + > > +int scmi_imx_misc_ctrl_set(u32 id, u32 val) > > +{ > > + if (!ph) > > + return -EPROBE_DEFER; > > + > > + return imx_misc_ctrl_ops->misc_ctrl_set(ph, id, 1, &val); > > +}; > > +EXPORT_SYMBOL(scmi_imx_misc_ctrl_set); > > + > > +int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val) > > +{ > > + if (!ph) > > + return -EPROBE_DEFER; > > + > > + return imx_misc_ctrl_ops->misc_ctrl_get(ph, id, num, val); > > +} > > +EXPORT_SYMBOL(scmi_imx_misc_ctrl_get); > > + > > Ok, now I suppose that you want to be sure to run just one instance if > this driver... > > > +static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb, > > + unsigned long event, void *data) > > +{ > > + return 0; > > +} > > What is the point of this ? > > > + > > +static int scmi_imx_misc_ctrl_probe(struct scmi_device *sdev) > > +{ > > + const struct scmi_handle *handle = sdev->handle; > > + struct device_node *np = sdev->dev.of_node; > > + u32 src_id, evt_id, wu_num; > > + int ret, i; > > + > > + if (!handle) > > + return -ENODEV; > > + > > + imx_misc_ctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_IMX_MISC, &ph); > > + if (IS_ERR(imx_misc_ctrl_ops)) > > + return PTR_ERR(imx_misc_ctrl_ops); > > + > > + scmi_imx_misc_ctrl_nb.notifier_call = &scmi_imx_misc_ctrl_notifier; > > + wu_num = of_property_count_u32_elems(np, "wakeup-sources"); > > + if (wu_num % 2) { > > + dev_err(&sdev->dev, "Invalid wakeup-sources\n"); > > + return -EINVAL; > > + } > > + > > + for (i = 0; i < wu_num; i += 2) { > > + WARN_ON(of_property_read_u32_index(np, "wakeup-sources", i, &src_id)); > > + WARN_ON(of_property_read_u32_index(np, "wakeup-sources", i + 1, &evt_id)); > > + ret = handle->notify_ops->devm_event_notifier_register(sdev, SCMI_PROTOCOL_IMX_MISC, > > + evt_id, > > + &src_id, > > + &scmi_imx_misc_ctrl_nb); > > ...when probed more than once this will lead to the same global nb registered on 2 > different notification chains.... > > > + if (ret) > > + dev_err(&sdev->dev, "Failed to register scmi misc event: %d\n", src_id); > > + } > > + > > + return 0; > > + > > +} > > + > > +static const struct scmi_device_id scmi_id_table[] = { > > + { SCMI_PROTOCOL_IMX_MISC, "imx-misc-ctrl" }, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(scmi, scmi_id_table); > > + > > +static struct scmi_driver scmi_imx_misc_ctrl_driver = { > > + .name = "scmi-imx-misc-ctrl", > > + .probe = scmi_imx_misc_ctrl_probe, > > + .id_table = scmi_id_table, > > +}; > > +module_scmi_driver(scmi_imx_misc_ctrl_driver); > > + > > All in all, I suppose the main thing to reason about this driver is if you > want/plan to allow for multiple instances of it to be loaded/probed on the same > running system or not... > > If you think that this driver HAS STRICTLY to be probed once, and having > 2 DT protocol nodes for MISC it is certainly an error, we will have to > add some mechianism in the SCMI core to be able to mark this as single > instance and refuse to create more than one device for this protocol...a > sort of generalization of what is done in a custom way by the core for > SYSTEM_POWER, since we dont want to have multiple sources of shutdown > events... An easier solution would be of course for this driver to just check if any global ph was already retrieved on a previous probe and just bail out, but I want to have a chat with Sudeep about this approach. Thanks, Cristian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel