Re: [PATCH 5/5] firmware: imx: add i.MX95 MISC driver

[Cristian Marussi <cristian.marussi@arm.com>]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel