| Message ID | 20240202-imx95-bbm-misc-v1-5-3cb743020933@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware: support i.MX95 SCMI BBM/MISC Extenstion | expand |
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... Alternnatively you will have to make this survive multiple probes...keeping track of multiple *ph for each probe() and handling that in your EXPORTED funcs...thing that seems awkard a bit...but you only know how this is an will be used. > +MODULE_AUTHOR("Peng Fan <peng.fan@nxp.com>"); > +MODULE_DESCRIPTION("IMX SM MISC driver"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/firmware/imx/sm.h b/include/linux/firmware/imx/sm.h > new file mode 100644 > index 000000000000..daad4bdf7d1c > --- /dev/null > +++ b/include/linux/firmware/imx/sm.h > @@ -0,0 +1,33 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +/* > + * Copyright 2024 NXP > + */ > + > +#ifndef _SCMI_IMX_H > +#define _SCMI_IMX_H > + > +#include <linux/bitfield.h> > +#include <linux/types.h> > + > +#define SCMI_IMX_CTRL_PDM_CLK_SEL 0 /* AON PDM clock sel */ > +#define SCMI_IMX_CTRL_MQS1_SETTINGS 1 /* AON MQS settings */ > +#define SCMI_IMX_CTRL_SAI1_MCLK 2 /* AON SAI1 MCLK */ > +#define SCMI_IMX_CTRL_SAI3_MCLK 3 /* WAKE SAI3 MCLK */ > +#define SCMI_IMX_CTRL_SAI4_MCLK 4 /* WAKE SAI4 MCLK */ > +#define SCMI_IMX_CTRL_SAI5_MCLK 5 /* WAKE SAI5 MCLK */ > + > +#if IS_ENABLED(CONFIG_IMX_SCMI_MISC_EXT) > +int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val); > +int scmi_imx_misc_ctrl_set(u32 id, u32 val); > +#else > +static inline int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline int scmi_imx_misc_ctrl_set(u32 id, u32 val); > +{ > + return -EOPNOTSUPP; > +} > +#endif > +#endif > Thanks, Cristian
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
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 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; +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); + +static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb, + unsigned long event, void *data) +{ + return 0; +} + +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); + 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); + +MODULE_AUTHOR("Peng Fan <peng.fan@nxp.com>"); +MODULE_DESCRIPTION("IMX SM MISC driver"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/firmware/imx/sm.h b/include/linux/firmware/imx/sm.h new file mode 100644 index 000000000000..daad4bdf7d1c --- /dev/null +++ b/include/linux/firmware/imx/sm.h @@ -0,0 +1,33 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Copyright 2024 NXP + */ + +#ifndef _SCMI_IMX_H +#define _SCMI_IMX_H + +#include <linux/bitfield.h> +#include <linux/types.h> + +#define SCMI_IMX_CTRL_PDM_CLK_SEL 0 /* AON PDM clock sel */ +#define SCMI_IMX_CTRL_MQS1_SETTINGS 1 /* AON MQS settings */ +#define SCMI_IMX_CTRL_SAI1_MCLK 2 /* AON SAI1 MCLK */ +#define SCMI_IMX_CTRL_SAI3_MCLK 3 /* WAKE SAI3 MCLK */ +#define SCMI_IMX_CTRL_SAI4_MCLK 4 /* WAKE SAI4 MCLK */ +#define SCMI_IMX_CTRL_SAI5_MCLK 5 /* WAKE SAI5 MCLK */ + +#if IS_ENABLED(CONFIG_IMX_SCMI_MISC_EXT) +int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val); +int scmi_imx_misc_ctrl_set(u32 id, u32 val); +#else +static inline int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val) +{ + return -EOPNOTSUPP; +} + +static inline int scmi_imx_misc_ctrl_set(u32 id, u32 val); +{ + return -EOPNOTSUPP; +} +#endif +#endif