On Mon, Feb 24, 2025 at 03:54:19PM +0000, Cristian Marussi wrote:
>On Wed, Feb 12, 2025 at 03:40:25PM +0800, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@xxxxxxx>
>>
>> Add Logical Machine Management(LMM) protocol which is intended for boot,
>> shutdown, and reset of other logical machines (LM). It is usually used to
>> allow one LM to manager another used as an offload or accelerator engine.
>>
>
>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-lmm.c | 270 +++++++++++++++++++++
>> include/linux/scmi_imx_protocol.h | 31 +++
>> 4 files changed, 313 insertions(+)
>>
>> diff --git a/drivers/firmware/arm_scmi/vendors/imx/Kconfig b/drivers/firmware/arm_scmi/vendors/imx/Kconfig
>> index a01bf5e47301d2f93c9bfc7eebc77e083ea4ed75..1a936fc87d2350e2a21bccd45dfbeebfa3b90286 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_LMM_EXT
>> + tristate "i.MX SCMI LMM EXTENSION"
>> + depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
>> + default y if ARCH_MXC
>> + help
>> + This enables i.MX System Logical Machine Protocol to
>> + 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.
>> +
>> config IMX_SCMI_MISC_EXT
>> tristate "i.MX SCMI MISC EXTENSION"
>> depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
>> diff --git a/drivers/firmware/arm_scmi/vendors/imx/Makefile b/drivers/firmware/arm_scmi/vendors/imx/Makefile
>> index d3ee6d5449244a4f5cdf6abcf1845f312c512325..f39a99ccaf9af757475e8b112d224669444d7ddc 100644
>> --- a/drivers/firmware/arm_scmi/vendors/imx/Makefile
>> +++ b/drivers/firmware/arm_scmi/vendors/imx/Makefile
>> @@ -1,3 +1,4 @@
>> # SPDX-License-Identifier: GPL-2.0-only
>> obj-$(CONFIG_IMX_SCMI_BBM_EXT) += imx-sm-bbm.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-lmm.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-lmm.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..4b9211df2329623fae0400039db91cb2b98cded1
>> --- /dev/null
>> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-lmm.c
>> @@ -0,0 +1,270 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * System control and Management Interface (SCMI) NXP LMM 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_lmm_protocol_cmd {
>> + SCMI_IMX_LMM_ATTRIBUTES = 0x3,
>> + SCMI_IMX_LMM_BOOT = 0x4,
>> + SCMI_IMX_LMM_RESET = 0x5,
>> + SCMI_IMX_LMM_SHUTDOWN = 0x6,
>> + SCMI_IMX_LMM_WAKE = 0x7,
>> + SCMI_IMX_LMM_SUSPEND = 0x8,
>> + SCMI_IMX_LMM_NOTIFY = 0x9,
>> + SCMI_IMX_LMM_RESET_REASON = 0xA,
>> + SCMI_IMX_LMM_POWER_ON = 0xB,
>> + SCMI_IMX_LMM_RESET_VECTOR_SET = 0xC,
>> +};
>> +
>> +struct scmi_imx_lmm_priv {
>> + u32 nr_lmm;
>> +};
>> +
>> +#define SCMI_IMX_LMM_PROTO_ATTR_NUM_LM(x) (((x) & 0xFFU))
>> +struct scmi_msg_imx_lmm_protocol_attributes {
>> + __le32 attributes;
>> +};
>> +
>> +struct scmi_msg_imx_lmm_attributes_out {
>> + __le32 lmid;
>> + __le32 attributes;
>> + __le32 state;
>> + __le32 errstatus;
>> + u8 name[LMM_MAX_NAME];
>> +};
>> +
>> +struct scmi_imx_lmm_reset_vector_set_in {
>> + __le32 lmid;
>> + __le32 cpuid;
>> +#define SCMI_LMM_VEC_FLAGS_TABLE BIT(0)
>> + __le32 flags;
>> + __le32 resetvectorlow;
>> + __le32 resetvectorhigh;
>> +};
>> +
>> +struct scmi_imx_lmm_shutdown_in {
>> + __le32 lmid;
>> + __le32 flags;
>> +};
>> +
>> +static int scmi_imx_lmm_validate_lmid(const struct scmi_protocol_handle *ph, u32 lmid)
>> +{
>> + struct scmi_imx_lmm_priv *priv = ph->get_priv(ph);
>> +
>> + if (lmid >= priv->nr_lmm)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static int scmi_imx_lmm_attributes(const struct scmi_protocol_handle *ph,
>> + u32 lmid, struct scmi_imx_lmm_info *info)
>> +{
>> + struct scmi_msg_imx_lmm_attributes_out *out;
>> + struct scmi_xfer *t;
>> + int ret;
>> +
>> + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_LMM_ATTRIBUTES, sizeof(u32), 0, &t);
>> + if (ret)
>> + return ret;
>> +
>> + put_unaligned_le32(lmid, t->tx.buf);
>> + ret = ph->xops->do_xfer(ph, t);
>> + if (!ret) {
>> + out = t->rx.buf;
>> + info->lmid = le32_to_cpu(out->lmid);
>> + info->state = le32_to_cpu(out->state);
>> + info->errstatus = le32_to_cpu(out->errstatus);
>> + strscpy(info->name, out->name);
>> + dev_dbg(ph->dev, "i.MX LMM: Logical Machine(%d), name: %s\n",
>> + info->lmid, out->name);
>> + } else {
>> + dev_err(ph->dev, "i.MX LMM: Failed to get info of Logical Machine(%u)\n", lmid);
>> + }
>> +
>> + ph->xops->xfer_put(ph, t);
>> +
>> + return ret;
>> +}
>> +
>> +static int scmi_imx_lmm_boot(const struct scmi_protocol_handle *ph, u32 lmid)
>> +{
>> + struct scmi_xfer *t;
...
>> +
>
>..this....
>
>> +static int scmi_imx_lmm_power_on(const struct scmi_protocol_handle *ph, u32 lmid)
>> +{
>> +
...
>> + return ret;
>> +}
>
>...can be refatcored to use one common workhorse fcuntion sued by both
>ops...
ok. I will merge power on and boot into one function.
>
>> +
>> +static int scmi_imx_lmm_reset_vector_set(const struct scmi_protocol_handle *ph,
>> + u32 lmid, u32 cpuid, u32 flags, u64 vector)
>> +{
>> + struct scmi_imx_lmm_reset_vector_set_in *in;
>> + struct scmi_xfer *t;
>> + int ret;
>> +
>> + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_LMM_RESET_VECTOR_SET, sizeof(*in),
>> + 0, &t);
>> + if (ret)
>> + return ret;
>> +
>> + in = t->tx.buf;
>> + in->lmid = cpu_to_le32(lmid);
>> + in->cpuid = cpu_to_le32(cpuid);
>> + in->flags = cpu_to_le32(flags);
>
>...bitfields in a flag field must not be enianity converted...only
>eventually subfields representing numbers (liek you did above)...
>
>..I feel FIELD_PREP should be fine...or even BIT(0) really given what
>these flags represents...
I will mark flags as reserved as updated doc, because this flag is not
used as of now.
>
>...again check issues with smatch....
yes, I will try this.
>
>> + 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_lmm_shutdown(const struct scmi_protocol_handle *ph, u32 lmid,
>> + u32 flags)
>> +{
>> + struct scmi_imx_lmm_shutdown_in *in;
>> + struct scmi_xfer *t;
>> + int ret;
>> +
>> + ret = scmi_imx_lmm_validate_lmid(ph, lmid);
>> + if (ret)
>> + return ret;
>> +
>> + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_LMM_SHUTDOWN, sizeof(*in),
>> + 0, &t);
>> + if (ret)
>> + return ret;
>> +
>> + in = t->tx.buf;
>> + in->lmid = cpu_to_le32(lmid);
>> + in->flags = cpu_to_le32(flags);
>
>Same here, smatch would complain if you remove straight away this
>cpu_to_le32(), BUT this is a bitfield and does NOT contain any
>longer-than-2 number value that needs to be endianess manipulated...you
>just have to be able to set the required BIT 0 and 1, whitouth pissing
>off smatch :P
Only BIT0 used as of now. I will update as you suggest.
>
>> + ret = ph->xops->do_xfer(ph, t);
>> +
>> + ph->xops->xfer_put(ph, t);
>> +
>> + return ret;
>> +}
>> +
>> +static const struct scmi_imx_lmm_proto_ops scmi_imx_lmm_proto_ops = {
>> + .lmm_boot = scmi_imx_lmm_boot,
>> + .lmm_info = scmi_imx_lmm_attributes,
>> + .lmm_power_on = scmi_imx_lmm_power_on,
>> + .lmm_reset_vector_set = scmi_imx_lmm_reset_vector_set,
>> + .lmm_shutdown = scmi_imx_lmm_shutdown,
>> +};
>> +
>> +static int scmi_imx_lmm_protocol_attributes_get(const struct scmi_protocol_handle *ph,
>> + struct scmi_imx_lmm_priv *priv)
>> +{
>> + struct scmi_msg_imx_lmm_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) {
>> + priv->nr_lmm = SCMI_IMX_LMM_PROTO_ATTR_NUM_LM(le32_to_cpu(attr->attributes));
>
>This seems wrong to me...you have an 8bit field to extract from an
>attribute field...so I would at first cut the byte out and then convert
>to LE, NOT the other way around like you are doing
>(i.e.: le32_to_cpu((attr->attributes & 0xFF))).
>
>Even BETTER to use:
>
> le32_get_bits((x), GENMASK(7, 0))
>
>...the thing is, being a single byte you really SHOULD NOT need to address
>any endianity concern (i.e. FIELD_GET(GENMASK(7, 0), (x))), BUT I fear that
>if you dont use some of the le32_ accessors smatch/sparse will complain since
>the message field is, correctly, declared as __le32.
>
>So le32_get_bits is the way to go (bit check with the static analyzer :P)
Yup, fix in next version.
>
>> + dev_info(ph->dev, "i.MX LMM: %d Logical Machines\n",
>> + priv->nr_lmm);
>> + }
>> +
...
>> +};
>> +module_scmi_protocol(scmi_imx_lmm);
>> +
>
>
>Should this also be added:
>
>MODULE_ALIAS("scmi-protocol-" __stringify(SCMI_PROTOCOL_IMX_LMM) "-" SCMI_IMX_VENDOR);
>
>...after this went in:
fix in next version.
>
>> };
....
>> +
>> +#define LMM_ID_DISCOVER 0xFFFFFFFFU
>
>What is this ? Documented anywhere ?
LMM_ATTRIBUTES if ID is 0xFFFFFFFF, it will return current lmid.
I will update in doc patch.
>
