Re: [PATCH v2 4/7] firmware: arm_scmi: imx: Add i.MX95 CPU Protocol

[Peng Fan <peng.fan@oss.nxp.com>]

On Mon, Feb 24, 2025 at 06:10:48PM +0000, Cristian Marussi wrote:
>On Wed, Feb 12, 2025 at 03:40:26PM +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,
>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>  drivers/firmware/arm_scmi/vendors/imx/Kconfig      |  11 +
>>  drivers/firmware/arm_scmi/vendors/imx/Makefile     |   1 +
>>  drivers/firmware/arm_scmi/vendors/imx/imx-sm-cpu.c | 287 +++++++++++++++++++++
>>  include/linux/scmi_imx_protocol.h                  |  10 +
>>  4 files changed, 309 insertions(+)
>> 
>
>[snip]
>
>> +
>> +struct scmi_imx_cpu_info_get_out {
>> +#define	CPU_RUN_MODE_START	0
>> +#define	CPU_RUN_MODE_HOLD	1
>> +#define	CPU_RUN_MODE_STOP	2
>> +#define	CPU_RUN_MODE_SLEEP	3
>> +	__le32 runmode;
>> +	__le32 sleepmode;
>> +	__le32 resetvectorlow;
>> +	__le32 resetvectorhigh;
>> +};
>> +
>> +static int scmi_imx_cpu_validate_cpuid(const struct scmi_protocol_handle *ph,
>> +				       u32 cpuid)
>> +{
>> +	struct scmi_imx_cpu_info *info = ph->get_priv(ph);
>> +
>> +	if (cpuid >= info->nr_cpu)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int scmi_imx_cpu_start(const struct scmi_protocol_handle *ph, u32 cpuid)
>> +{
>> +	struct scmi_xfer *t;
>> +	int ret;
>> +
>> +	ret = scmi_imx_cpu_validate_cpuid(ph, cpuid);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_CPU_START, sizeof(u32),
>> +				      0, &t);
>> +	if (ret)
>> +		return ret;
>> +
>> +	put_unaligned_le32(cpuid, t->tx.buf);
>> +	ret = ph->xops->do_xfer(ph, t);
>> +
>> +	ph->xops->xfer_put(ph, t);
>> +
>> +	return ret;
>> +}
>> +
>> +static int scmi_imx_cpu_stop(const struct scmi_protocol_handle *ph, u32 cpuid)
>> +{
>> +	struct scmi_xfer *t;
>> +	int ret;
>> +
>> +	ret = scmi_imx_cpu_validate_cpuid(ph, cpuid);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_CPU_STOP, sizeof(u32),
>> +				      0, &t);
>> +	if (ret)
>> +		return ret;
>> +
>> +	put_unaligned_le32(cpuid, t->tx.buf);
>> +	ret = ph->xops->do_xfer(ph, t);
>> +
>> +	ph->xops->xfer_put(ph, t);
>> +
>> +	return ret;
>> +}
>> +
>
>Please refactor and unify this 2 seemingly identical start/stop funcs by
>defining a common helper...

ok. fix in next version.

>
>> +static int scmi_imx_cpu_reset_vector_set(const struct scmi_protocol_handle *ph,
>> +					 u32 cpuid, u64 vector, bool start,
>> +					 bool boot, bool resume)
>> +{
>> +	struct scmi_imx_cpu_reset_vector_set_in *in;
>> +	struct scmi_xfer *t;
>> +	int ret;
>> +	u32 flags;
>> +
>> +	ret = scmi_imx_cpu_validate_cpuid(ph, cpuid);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_CPU_RESET_VECTOR_SET, sizeof(*in),
>> +				      0, &t);
>> +	if (ret)
>> +		return ret;
>> +
>> +	in = t->tx.buf;
>> +	in->cpuid = cpu_to_le32(cpuid);
>> +	flags = start ? CPU_VEC_FLAGS_START : 0;
>> +	flags |= boot ? CPU_VEC_FLAGS_BOOT : 0;
>> +	flags |= resume ? CPU_VEC_FLAGS_RESUME : 0;
>> +	in->flags = cpu_to_le32(flags);
>
>...you should just avoid the endianess helper given that is a bitfield (I cannot
>exclude that there are other places where we wrongly endianIZE bitfields...) and
>I think that the best way to do it without cause smatch to cry would be
>to use le32_encode_bits() which should just produce the desired flags
>into an __le32
>
>le32_encode_bits and friends are used throughout the code base and added
>
>https://patchwork.ozlabs.org/project/ubuntu-kernel/patch/20190606034255.2192-2-aaron.ma@canonical.com/
>
>which seems to be the best (and only) documentation :P

Thanks for the pointer, I will fix in next version as you suggest.

>
>> +	in->resetvectorlow = cpu_to_le32(lower_32_bits(vector));
>> +	in->resetvectorhigh = cpu_to_le32(upper_32_bits(vector));
>> +	ret = ph->xops->do_xfer(ph, t);
>> +
>> +	ph->xops->xfer_put(ph, t);
>> +
>> +	return ret;
>> +}
>> +
>> +static int scmi_imx_cpu_started(const struct scmi_protocol_handle *ph, u32 cpuid,
>> +				bool *started)
>> +{
>> +	struct scmi_imx_cpu_info_get_out *out;
>> +	struct scmi_xfer *t;
>> +	u32 mode;
>> +	int ret;
>> +
>
>maybe overlay paranoid...but
>
>	if (!started)
>		return -EINVAL;
>
>...up to you, if you feel paranoid too
>
>> +	*started = false;
>> +	ret = scmi_imx_cpu_validate_cpuid(ph, cpuid);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_CPU_INFO_GET, sizeof(u32),
>> +				      0, &t);
>> +	if (ret)
>> +		return ret;
>> +
>> +	put_unaligned_le32(cpuid, t->tx.buf);
>> +	ret = ph->xops->do_xfer(ph, t);
>> +	if (!ret) {
>> +		out = t->rx.buf;
>> +		mode = le32_to_cpu(out->runmode);
>> +		if ((mode == CPU_RUN_MODE_START) || (mode == CPU_RUN_MODE_SLEEP))
>> +			*started = true;
>> +	}
>> +
>> +	ph->xops->xfer_put(ph, t);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct scmi_imx_cpu_proto_ops scmi_imx_cpu_proto_ops = {
>> +	.cpu_reset_vector_set = scmi_imx_cpu_reset_vector_set,
>> +	.cpu_start = scmi_imx_cpu_start,
>> +	.cpu_started = scmi_imx_cpu_started,
>> +	.cpu_stop = scmi_imx_cpu_stop,
>> +};
>> +
>> +static int scmi_imx_cpu_protocol_attributes_get(const struct scmi_protocol_handle *ph,
>> +						struct scmi_imx_cpu_info *info)
>> +{
>> +	struct scmi_msg_imx_cpu_protocol_attributes *attr;
>> +	struct scmi_xfer *t;
>> +	int ret;
>> +
>> +	ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0,
>> +				      sizeof(*attr), &t);
>> +	if (ret)
>> +		return ret;
>> +
>> +	attr = t->rx.buf;
>> +
>> +	ret = ph->xops->do_xfer(ph, t);
>> +	if (!ret) {
>> +		info->nr_cpu = SCMI_IMX_CPU_PROTO_ATTR_NUM_CPUS(attr->attributes);
>
>	
>	le32_get_bits(attr->attributes, GENMASK())

Fix in next version.

>
>> +		dev_info(ph->dev, "i.MX SM CPU: %d cpus\n",
>> +			 info->nr_cpu);
>> +	}
>> +
>> +	ph->xops->xfer_put(ph, t);
>> +
>> +	return ret;
>> +}
>> +
>> +static int scmi_imx_cpu_attributes_get(const struct scmi_protocol_handle *ph,
>> +				       u32 cpuid)
>> +{
>> +	struct scmi_msg_imx_cpu_attributes_out *out;
>> +	char name[SCMI_SHORT_NAME_MAX_SIZE] = {'\0'};
>> +	struct scmi_xfer *t;
>> +	int ret;
>> +
>> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_CPU_ATTRIBUTES, sizeof(u32), 0, &t);
>> +	if (ret)
>> +		return ret;
>> +
>> +	put_unaligned_le32(cpuid, t->tx.buf);
>> +	ret = ph->xops->do_xfer(ph, t);
>> +	if (!ret) {
>> +		out = t->rx.buf;
>> +		strscpy(name, out->name, SCMI_SHORT_NAME_MAX_SIZE);
>> +		dev_info(ph->dev, "i.MX CPU: name: %s\n", name);
>> +	} else {
>> +		dev_err(ph->dev, "i.MX cpu: Failed to get info of cpu(%u)\n", cpuid);
>> +	}
>> +
>> +	ph->xops->xfer_put(ph, t);
>> +
>> +	return ret;
>> +}
>> +
>> +static int scmi_imx_cpu_protocol_init(const struct scmi_protocol_handle *ph)
>> +{
>> +	struct scmi_imx_cpu_info *info;
>> +	u32 version;
>> +	int ret, i;
>> +
>> +	ret = ph->xops->version_get(ph, &version);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev_info(ph->dev, "NXP SM CPU Protocol Version %d.%d\n",
>> +		 PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
>> +
>> +	info = devm_kzalloc(ph->dev, sizeof(*info), GFP_KERNEL);
>> +	if (!info)
>> +		return -ENOMEM;
>> +
>> +	ret = scmi_imx_cpu_protocol_attributes_get(ph, info);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (i = 0; i < info->nr_cpu; i++) {
>> +		ret = scmi_imx_cpu_attributes_get(ph, i);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return ph->set_priv(ph, info, version);
>> +}
>> +
>> +static const struct scmi_protocol scmi_imx_cpu = {
>> +	.id = SCMI_PROTOCOL_IMX_CPU,
>> +	.owner = THIS_MODULE,
>> +	.instance_init = &scmi_imx_cpu_protocol_init,
>> +	.ops = &scmi_imx_cpu_proto_ops,
>> +	.supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
>> +	.vendor_id = SCMI_IMX_VENDOR,
>> +	.sub_vendor_id = SCMI_IMX_SUBVENDOR,
>> +};
>> +module_scmi_protocol(scmi_imx_cpu);
>
>similarly as LMM...
>
>MODULE_ALIAS("scmi-protocol-" __stringify(SCMI_PROTOCOL_IMX_CPU) "-" SCMI_IMX_VENDOR);

Fix in next version
Appreciate again for reviewing the large patchset.

Thanks,
Peng

>
>Thanks
>Cristian
>