lkml.org 
[lkml]   [2024]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 5/5] firmware: imx: add i.MX95 MISC driver
    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 <peng.fan@nxp.com>
    > >
    > > 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 <peng.fan@nxp.com>
    > > ---
    > > 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 <linux/firmware/imx/sm.h>
    > > +#include <linux/module.h>
    > > +#include <linux/of.h>
    > > +#include <linux/platform_device.h>
    > > +#include <linux/scmi_protocol.h>
    > > +#include <linux/scmi_nxp_protocol.h>
    > > +
    > > +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

    \
     
     \ /
      Last update: 2024-05-27 15:22    [W:5.431 / U:0.052 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site