Re: [PATCH v2 5/7] firmware: imx: Add i.MX95 SCMI LMM driver

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

On Wed, Feb 12, 2025 at 03:40:27PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> The i.MX95 System manager exports SCMI LMM protocol for linux to manage
> Logical Machines. The driver is to use the LMM Protocol interface to
> boot, shutdown a LM.
> 

Hi,

> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/firmware/imx/Kconfig    | 11 +++++
>  drivers/firmware/imx/Makefile   |  1 +
>  drivers/firmware/imx/sm-lmm.c   | 98 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/firmware/imx/sm.h | 33 ++++++++++++++
>  4 files changed, 143 insertions(+)
> 
> diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig
> index 907cd149c40a8b5f1b14c53e1c315ff4a28f32ac..c3e344d6ecc645df1f0e3ee8078934c47f347fd7 100644
> --- a/drivers/firmware/imx/Kconfig
> +++ b/drivers/firmware/imx/Kconfig
> @@ -23,6 +23,17 @@ config IMX_SCU
>  	  This driver manages the IPC interface between host CPU and the
>  	  SCU firmware running on M4.
>  
> +config IMX_SCMI_LMM_DRV
> +	tristate "IMX SCMI LMM Protocol driver"
> +	depends on IMX_SCMI_LMM_EXT || COMPILE_TEST
> +	default y if ARCH_MXC
> +	help
> +	  The System Controller Management Interface firmware (SCMI FW) is
> +	  a low-level system function which runs on a dedicated Cortex-M
> +	  core that could provide Logical Machine management features.
> +
> +	  This driver can also be built as a module.
> +
>  config IMX_SCMI_MISC_DRV
>  	tristate "IMX SCMI MISC Protocol driver"
>  	default y if ARCH_MXC
> diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile
> index 8d046c341be878bb6dd1e6277992ff66ae90e292..7762855d2a771169d4f1867d27e0d51be7c9ad03 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_MISC_DRV}	+= sm-misc.o
> +obj-${CONFIG_IMX_SCMI_LMM_DRV}	+= sm-lmm.o
> diff --git a/drivers/firmware/imx/sm-lmm.c b/drivers/firmware/imx/sm-lmm.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..ed9650a8cb7ca878874e8609f0a5c83b5e46204a
> --- /dev/null
> +++ b/drivers/firmware/imx/sm-lmm.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2025 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_imx_protocol.h>
> +
> +static const struct scmi_imx_lmm_proto_ops *imx_lmm_ops;
> +static struct scmi_protocol_handle *ph;
> +
> +int scmi_imx_lmm_boot(u32 lmid)
> +{
> +	if (!ph)
> +		return -EPROBE_DEFER;
> +
> +	return imx_lmm_ops->lmm_boot(ph, lmid);
> +};
> +EXPORT_SYMBOL(scmi_imx_lmm_boot);


...cant you unify this...

> +
> +int scmi_imx_lmm_info(u32 lmid, struct scmi_imx_lmm_info *info)
> +{
> +	if (!ph)
> +		return -EPROBE_DEFER;
> +
> +	if (!info)
> +		return -EINVAL;
> +
> +	return imx_lmm_ops->lmm_info(ph, lmid, info);
> +};
> +EXPORT_SYMBOL(scmi_imx_lmm_info);
> +
> +int scmi_imx_lmm_reset_vector_set(u32 lmid, u32 cpuid, u32 flags, u64 vector)
> +{
> +	if (!ph)
> +		return -EPROBE_DEFER;
> +
> +	return imx_lmm_ops->lmm_reset_vector_set(ph, lmid, cpuid, flags, vector);
> +}
> +EXPORT_SYMBOL(scmi_imx_lmm_reset_vector_set);
> +
> +int scmi_imx_lmm_power_on(u32 lmid)
> +{
> +	if (!ph)
> +		return -EPROBE_DEFER;
> +
> +	return imx_lmm_ops->lmm_power_on(ph, lmid);
> +};
> +EXPORT_SYMBOL(scmi_imx_lmm_power_on);

...this...

> +
> +int scmi_imx_lmm_shutdown(u32 lmid, u32 flags)
> +{
> +	if (!ph)
> +		return -EPROBE_DEFER;
> +
> +	return imx_lmm_ops->lmm_shutdown(ph, lmid, flags);
> +};
> +EXPORT_SYMBOL(scmi_imx_lmm_shutdown);
> +

...and this into some common operation that accepts parameters to
specifiy what to do...like (just making up name)

	scmi_imx_operation(lmid, flags, <OP>)

...so exporting only ONE symbol ?

> +static int scmi_imx_lmm_probe(struct scmi_device *sdev)
> +{
> +	const struct scmi_handle *handle = sdev->handle;
> +
> +	if (!handle)
> +		return -ENODEV;
> +
> +	if (imx_lmm_ops) {
> +		dev_err(&sdev->dev, "lmm already initialized\n");
> +		return -EEXIST;
> +	}
> +
> +	imx_lmm_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_IMX_LMM, &ph);
> +	if (IS_ERR(imx_lmm_ops))
> +		return PTR_ERR(imx_lmm_ops);
> +
> +	return 0;
> +}
> +
> +static const struct scmi_device_id scmi_id_table[] = {
> +	{ SCMI_PROTOCOL_IMX_LMM, "imx-lmm" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> +
> +static struct scmi_driver scmi_imx_lmm_driver = {
> +	.name = "scmi-imx-lmm",
> +	.probe = scmi_imx_lmm_probe,
> +	.id_table = scmi_id_table,
> +};
> +module_scmi_driver(scmi_imx_lmm_driver);
> +
> +MODULE_AUTHOR("Peng Fan <peng.fan@nxp.com>");
> +MODULE_DESCRIPTION("IMX SM LMM driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/firmware/imx/sm.h b/include/linux/firmware/imx/sm.h
> index 9b85a3f028d1b0a5287b453eb3ad8412a363fe6c..e02b7b558afb6f430f6fbeeaf3ee1f59feea3c1b 100644
> --- a/include/linux/firmware/imx/sm.h
> +++ b/include/linux/firmware/imx/sm.h
> @@ -8,6 +8,7 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/errno.h>
> +#include <linux/scmi_imx_protocol.h>
>  #include <linux/types.h>
>  
>  #define SCMI_IMX_CTRL_PDM_CLK_SEL	0	/* AON PDM clock sel */
> @@ -20,4 +21,36 @@
>  int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val);
>  int scmi_imx_misc_ctrl_set(u32 id, u32 val);
>

This down below seems fine to me (maybe simoplfied as stated above) and
I guess we should see if we dont hit some weird Kconfig combination that
breaks some builds on some combination of modules/builtin: in the
past I vaguely remember Arnd posting the build error...and in fact the
above scmi_imx_misc_ctrl_set/get have NO ifdeffery and aer handeld in
that reversed way in which the protocol depends on the driver

config IMX_SCMI_MISC_EXT                                                         
        tristate "i.MX SCMI MISC EXTENSION"                                      
        depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)                     
        depends on IMX_SCMI_MISC_DRV                                             
        default y if ARCH_MXC                         

...which weird but was needed by the build bots combinations....(I
think)...so just a heads up that you could have to deal with that
again...

> +#if IS_ENABLED(CONFIG_IMX_SCMI_LMM_DRV) || IS_ENABLED(CONFIG_COMPILE_TEST)
> +int scmi_imx_lmm_boot(u32 lmid);
> +int scmi_imx_lmm_info(u32 lmid, struct scmi_imx_lmm_info *info);
> +int scmi_imx_lmm_reset_vector_set(u32 lmid, u32 cpuid, u32 flags, u64 vector);
> +int scmi_imx_lmm_power_on(u32 lmid);
> +int scmi_imx_lmm_shutdown(u32 lmid, u32 flags);
> +#else
> +static inline int scmi_imx_lmm_boot(u32 lmid)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int scmi_imx_lmm_info(u32 lmid, struct scmi_imx_lmm_info *info)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int scmi_imx_lmm_reset_vector_set(u32 lmid, u32 cpuid, u32 flags, u64 vector)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int scmi_imx_lmm_power_on(u32 lmid)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int scmi_imx_lmm_shutdown(u32 lmid, u32 flags)
> +{
> +	return -EOPNOTSUPP;
> +}
> +#endif
>  #endif
> 

Other thean the above unification:

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian