| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3b9a7392-8ebe-4d43-a111-68bb6d2f93b6@stanley.mountain>
Date: Wed, 22 Jan 2025 11:48:52 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: "Peng Fan (OSS)" <peng.fan@....nxp.com>
Cc: Sudeep Holla <sudeep.holla@....com>,
Cristian Marussi <cristian.marussi@....com>,
Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>, linux-kernel@...r.kernel.org,
arm-scmi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
imx@...ts.linux.dev, Peng Fan <peng.fan@....com>
Subject: Re: [PATCH 2/5] firmware: arm_scmi: imx: Add i.MX95 CPU Protocol
On Tue, Jan 21, 2025 at 11:08:12PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@....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).
>
> Signed-off-by: Peng Fan <peng.fan@....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.
> +};
> +
> +struct scmi_imx_cpu_reset_vector_set_in {
> + __le32 cpuid;
> +#define CPU_VEC_FLAGS_RESUME BIT(31)
> +#define CPU_VEC_FLAGS_START BIT(30)
> +#define CPU_VEC_FLAGS_BOOT BIT(29)
> + __le32 flags;
> + __le32 resetvectorlow;
> + __le32 resetvectorhigh;
> +};
> +
> +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;
> +}
> +
> +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;
> +
> + 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);
> + in->flags = start ? CPU_VEC_FLAGS_START : 0;
> + in->flags |= boot ? CPU_VEC_FLAGS_BOOT : 0;
> + in->flags |= resume ? CPU_VEC_FLAGS_BOOT : 0;
s/CPU_VEC_FLAGS_BOOT/CPU_VEC_FLAGS_RESUME/
> + 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;
> + int ret;
> +
> + *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;
> + if ((out->runmode == CPU_RUN_MODE_START) ||
> + (out->runmode == 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);
> + dev_info(ph->dev, "i.MX SM CPU: %d cpus\n",
> + info->nr_cpu);
This can fit in 80 characters:
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_imx_cpu_info *info)
No need to pass the "info" pointer.
> +{
> + struct scmi_msg_imx_cpu_attributes_out *out;
> + 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;
> + dev_info(ph->dev, "i.MX CPU: name: %s\n", out->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;
> +}
> +
> +
Multiple blank lines (checkpatch.pl --strict).
> +static int scmi_imx_cpu_protocol_init(const struct scmi_protocol_handle *ph)
> +{
regards,
dan carpenter
Powered by blists - more mailing lists