Re: [PATCH 2/5] firmware: arm_scmi: imx: Add i.MX95 CPU Protocol

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

On Wed, Jan 22, 2025 at 11:48:52AM +0300, Dan Carpenter wrote:
> On Tue, Jan 21, 2025 at 11:08:12PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> > 
> > This protocol allows an agent to start, stop a CPU or set reset vector. It
> > is used to manage auxiliary CPUs in an LM (e.g. additional cores in an AP
> > cluster).
> > 

Hi,

just a quick one down below.

> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/firmware/arm_scmi/vendors/imx/Kconfig      |  13 +-
> >  drivers/firmware/arm_scmi/vendors/imx/Makefile     |   1 +
> >  drivers/firmware/arm_scmi/vendors/imx/imx-sm-cpu.c | 283 +++++++++++++++++++++
> >  include/linux/scmi_imx_protocol.h                  |  10 +
> >  4 files changed, 306 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/firmware/arm_scmi/vendors/imx/Kconfig b/drivers/firmware/arm_scmi/vendors/imx/Kconfig
> > index 1a936fc87d2350e2a21bccd45dfbeebfa3b90286..9070522510e4d3f3d7276a7581f8676006d20f90 100644
> > --- a/drivers/firmware/arm_scmi/vendors/imx/Kconfig
> > +++ b/drivers/firmware/arm_scmi/vendors/imx/Kconfig
> > @@ -12,6 +12,17 @@ config IMX_SCMI_BBM_EXT
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called imx-sm-bbm.
> >  
> > +config IMX_SCMI_CPU_EXT
> > +	tristate "i.MX SCMI CPU EXTENSION"
> > +	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> > +	default y if ARCH_MXC
> > +	help
> > +	  This enables i.MX System CPU Protocol to manage cpu
> > +	  start, stop and etc.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called imx-sm-cpu.
> > +
> >  config IMX_SCMI_LMM_EXT
> >  	tristate "i.MX SCMI LMM EXTENSION"
> >  	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> > @@ -21,7 +32,7 @@ config IMX_SCMI_LMM_EXT
> >  	  manage Logical Machines boot, shutdown and etc.
> >  
> >  	  To compile this driver as a module, choose M here: the
> > -	  module will be called imx-sm-lmm.
> > +	  module will be called imx-sm-cpu.
> >  
> 
> It's supposed to be called imx-sm-lmm.
> 
> >  config IMX_SCMI_MISC_EXT
> >  	tristate "i.MX SCMI MISC EXTENSION"
> > diff --git a/drivers/firmware/arm_scmi/vendors/imx/Makefile b/drivers/firmware/arm_scmi/vendors/imx/Makefile
> > index f39a99ccaf9af757475e8b112d224669444d7ddc..e3a5ea46345c89da1afae25e55698044672b7c28 100644
> > --- a/drivers/firmware/arm_scmi/vendors/imx/Makefile
> > +++ b/drivers/firmware/arm_scmi/vendors/imx/Makefile
> > @@ -1,4 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  obj-$(CONFIG_IMX_SCMI_BBM_EXT) += imx-sm-bbm.o
> > +obj-$(CONFIG_IMX_SCMI_CPU_EXT) += imx-sm-cpu.o
> >  obj-$(CONFIG_IMX_SCMI_LMM_EXT) += imx-sm-lmm.o
> >  obj-$(CONFIG_IMX_SCMI_MISC_EXT) += imx-sm-misc.o
> > diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-cpu.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-cpu.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..e3f294c2cb69a5b5a916d55984f4a63539937d02
> > --- /dev/null
> > +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-cpu.c
> > @@ -0,0 +1,283 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * System control and Management Interface (SCMI) NXP CPU Protocol
> > + *
> > + * Copyright 2025 NXP
> > + */
> > +
> > +#include <linux/bits.h>
> > +#include <linux/io.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>
> > +
> > +#include "../../protocols.h"
> > +#include "../../notify.h"
> > +
> > +#define SCMI_PROTOCOL_SUPPORTED_VERSION		0x10000
> > +
> > +enum scmi_imx_cpu_protocol_cmd {
> > +	SCMI_IMX_CPU_ATTRIBUTES	= 0x3,
> > +	SCMI_IMX_CPU_START = 0x4,
> > +	SCMI_IMX_CPU_STOP = 0x5,
> > +	SCMI_IMX_CPU_RESET_VECTOR_SET = 0x6,
> > +	SCMI_IMX_CPU_INFO_GET = 0xC,
> > +};
> > +
> > +struct scmi_imx_cpu_info {
> > +	u32 nr_cpu;
> > +};
> > +
> > +#define SCMI_IMX_CPU_PROTO_ATTR_NUM_CPUS(x)  ((x) & 0xFFFF)
> > +struct scmi_msg_imx_cpu_protocol_attributes {
> > +	__le32 attributes;
> > +};
> > +
> > +struct scmi_msg_imx_cpu_attributes_out {
> > +	__le32 attributes;
> > +#define	CPU_MAX_NAME	16
> > +	u8 name[CPU_MAX_NAME];
> 
> char is always unsigned in the kernel these days but strings should
> still always be char.  Same thing in patch 1, there were a couple u8
> names.
> 

While it is certainly true that char is the way to go for strings and, as
such, it is used elsewhere to hold the resource names across all SCMI
protocols, in this context it is a field of structure representing
exactly the layout of message reply coming from the server, and defined
in the SCMI spec as a uint8 array, so, we have generally preferred to
used u8 to represent such fixed size array all across the SCMI stack
protocols implementation....

.... not saying that it is necessarily completelt right, but that is the
reason we are guilty :D

Thanks,
Cristian