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@xxxxxxx>
> >
> > 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@xxxxxxx>
> > ---
> > 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
