On Wed, Feb 12, 2025 at 03:40:26PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@xxxxxxx>
>
> 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@xxxxxxx>
> ---
> 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...
> +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@xxxxxxxxxxxxx/
which seems to be the best (and only) documentation :P
> + 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())
> + 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);
Thanks
Cristian
