On Fri, Nov 15, 2024 at 06:45:15AM +0530, Sibi Sankar wrote:
> The QCOM SCMI Generic Extensions Protocol provides a generic way of
> exposing a number of Qualcomm SoC specific features (like memory bus
> scaling) through a mixture of pre-determined algorithm strings and
> param_id pairs hosted on the SCMI controller.
>
Hi,
> Co-developed-by: Shivnandan Kumar <quic_kshivnan@xxxxxxxxxxx>
> Signed-off-by: Shivnandan Kumar <quic_kshivnan@xxxxxxxxxxx>
> Co-developed-by: Ramakrishna Gottimukkula <quic_rgottimu@xxxxxxxxxxx>
> Signed-off-by: Ramakrishna Gottimukkula <quic_rgottimu@xxxxxxxxxxx>
> Co-developed-by: Amir Vajid <avajid@xxxxxxxxxxx>
> Signed-off-by: Amir Vajid <avajid@xxxxxxxxxxx>
> Signed-off-by: Sibi Sankar <quic_sibis@xxxxxxxxxxx>
> Reviewed-by: Cristian Marussi <cristian.marussi@xxxxxxx>
You reworked/refactored quite a lot the internals since V4, you should
have dropped the Reviewed-by tag.
> ---
>
> v4:
> * Splitting the series into vendor protocol and memlat client.
> Also the move the memlat client implementation back to RFC
> due to multiple opens.
> * Use common xfer helper to avoid code duplication [Dmitry]
> * Update enum documentation without duplicate enum info [Dmitry]
>
> drivers/firmware/arm_scmi/Kconfig | 1 +
> drivers/firmware/arm_scmi/Makefile | 1 +
> .../firmware/arm_scmi/vendors/qcom/Kconfig | 15 ++
> .../firmware/arm_scmi/vendors/qcom/Makefile | 2 +
> .../arm_scmi/vendors/qcom/qcom-generic-ext.c | 139 ++++++++++++++++++
> include/linux/scmi_qcom_protocol.h | 37 +++++
> 6 files changed, 195 insertions(+)
> create mode 100644 drivers/firmware/arm_scmi/vendors/qcom/Kconfig
> create mode 100644 drivers/firmware/arm_scmi/vendors/qcom/Makefile
> create mode 100644 drivers/firmware/arm_scmi/vendors/qcom/qcom-generic-ext.c
> create mode 100644 include/linux/scmi_qcom_protocol.h
>
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index dabd874641d0..73128442d97b 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -71,6 +71,7 @@ config ARM_SCMI_DEBUG_COUNTERS
>
> source "drivers/firmware/arm_scmi/transports/Kconfig"
> source "drivers/firmware/arm_scmi/vendors/imx/Kconfig"
> +source "drivers/firmware/arm_scmi/vendors/qcom/Kconfig"
>
> endif #ARM_SCMI_PROTOCOL
>
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index 9ac81adff567..58cf4d656cbb 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -12,6 +12,7 @@ scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
>
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += transports/
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += vendors/imx/
> +obj-$(CONFIG_ARM_SCMI_PROTOCOL) += vendors/qcom/
>
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o
> diff --git a/drivers/firmware/arm_scmi/vendors/qcom/Kconfig b/drivers/firmware/arm_scmi/vendors/qcom/Kconfig
> new file mode 100644
> index 000000000000..5dd9e8a6b75f
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/vendors/qcom/Kconfig
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +menu "ARM SCMI QCOM Vendor Protocols"
> +
> +config QCOM_SCMI_GENERIC_EXT
> + tristate "Qualcomm Technologies, Inc. Qcom SCMI vendor Protocol"
> + depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> + help
> + The QCOM SCMI vendor protocol provides a generic way of exposing
> + a number of Qualcomm SoC specific features (like memory bus scaling)
> + through a mixture of pre-determined algorithm strings and param_id
> + pairs hosted on the SCMI controller.
> +
> + This driver defines/documents the message ID's used for this
> + communication and also exposes the operations used by the clients.
> +endmenu
> diff --git a/drivers/firmware/arm_scmi/vendors/qcom/Makefile b/drivers/firmware/arm_scmi/vendors/qcom/Makefile
> new file mode 100644
> index 000000000000..6b98fabbebb8
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/vendors/qcom/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_QCOM_SCMI_GENERIC_EXT) += qcom-generic-ext.o
> diff --git a/drivers/firmware/arm_scmi/vendors/qcom/qcom-generic-ext.c b/drivers/firmware/arm_scmi/vendors/qcom/qcom-generic-ext.c
> new file mode 100644
> index 000000000000..1b209093d275
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/vendors/qcom/qcom-generic-ext.c
> @@ -0,0 +1,139 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/scmi_qcom_protocol.h>
> +
> +#include "../../common.h"
> +
> +/**
> + * enum qcom_generic_ext_protocol_cmd - vendor specific commands supported by SCMI Qualcomm
> + * generic vendor protocol.
> + *
> + * This protocol is intended as a generic way of exposing a number of Qualcomm SoC
> + * specific features through a mixture of pre-determined algorithm string and param_id
> + * pairs hosted on the SCMI controller.
> + *
> + * The QCOM SCMI Vendor Protocol has the protocol id as 0x80 and vendor id set to
> + * Qualcomm and the implementation version set to 0x20000. The PROTOCOL_VERSION command
> + * returns version 1.0.
The chosen protocol ID is already defined in the protocol doc in the
series, and the note about the implementation version seems redundant,
it is indeed specified as needed in scmi_protocol.
> + *
> + * @QCOM_SCMI_SET_PARAM: is used to set the parameter of a specific algo_str hosted on
> + * QCOM SCMI Vendor Protocol. The tx len depends on the algo_str used.
> + * @QCOM_SCMI_GET_PARAM: is used to get parameter information of a specific algo_str
> + * hosted on QCOM SCMI Vendor Protocol. The tx and rx len depends
> + * on the algo_str used.
> + * @QCOM_SCMI_START_ACTIVITY: is used to start the activity performed by the algo_str.
> + * @QCOM_SCMI_STOP_ACTIVITY: is used to stop a pre-existing activity performed by the algo_str.
> + */
> +enum qcom_generic_ext_protocol_cmd {
> + QCOM_SCMI_SET_PARAM = 0x10,
> + QCOM_SCMI_GET_PARAM = 0x11,
> + QCOM_SCMI_START_ACTIVITY = 0x12,
> + QCOM_SCMI_STOP_ACTIVITY = 0x13,
> +};
> +
> +/**
> + * struct qcom_scmi_msg - represents the various parameters to be populated
> + * for using the QCOM SCMI Vendor Protocol
> + *
> + * @ext_id: reserved, must be zero
> + * @algo_low: lower 32 bits of the algo_str
> + * @algo_high: upper 32 bits of the algo_str
> + * @param_id: serves as token message id to the specific algo_str
> + * @buf: serves as the payload to the specified param_id and algo_str pair
> + */
> +struct qcom_scmi_msg {
> + __le32 ext_id;
> + __le32 algo_low;
> + __le32 algo_high;
> + __le32 param_id;
> + __le32 buf[];
> +};
> +
> +static int qcom_scmi_common_xfer(const struct scmi_protocol_handle *ph,
> + enum qcom_generic_ext_protocol_cmd cmd_id, void *buf,
> + size_t buf_len, u64 algo_str, u32 param_id, size_t rx_size)
> +{
> + struct scmi_xfer *t;
> + struct qcom_scmi_msg *msg;
> + int ret;
> +
> + ret = ph->xops->xfer_get_init(ph, cmd_id, buf_len + sizeof(*msg), rx_size, &t);
> + if (ret)
> + return ret;
> +
> + msg = t->tx.buf;
> + msg->algo_low = cpu_to_le32(lower_32_bits(algo_str));
> + msg->algo_high = cpu_to_le32(upper_32_bits(algo_str));
> + msg->param_id = cpu_to_le32(param_id);
> + memcpy(msg->buf, buf, buf_len);
> +
> + ret = ph->xops->do_xfer(ph, t);
> + if (rx_size)
You should check ret == 0 also here before considering to pick up the reply
if a non-zero rx_size was specified.
> + memcpy(buf, t->rx.buf, t->rx.len);
...you are using the same buf/buf_len to hold the TX request and get back
the reply content if a non-zero rx_size was provided but while you can
be sure that the reply payload rx.len <= rx_size by construction
(via xfer_get_init) you cannot be equally sure that rx_size <= buf_len
...it depends on te caller how this operation is invoked...so you could
end up oveflowing buf depending on the caller-provided params...
...please add some additional check at the start of this function like:
if (rx_size > buf_len)
return -EINVAL;
...to catch such bad invcations...
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int qcom_scmi_set_param(const struct scmi_protocol_handle *ph, void *buf, size_t buf_len,
> + u64 algo_str, u32 param_id)
> +{
> + return qcom_scmi_common_xfer(ph, QCOM_SCMI_SET_PARAM, buf, buf_len, algo_str,
> + param_id, 0);
> +}
> +
> +static int qcom_scmi_get_param(const struct scmi_protocol_handle *ph, void *buf, size_t buf_len,
> + u64 algo_str, u32 param_id, size_t rx_size)
> +{
> + return qcom_scmi_common_xfer(ph, QCOM_SCMI_GET_PARAM, buf, buf_len, algo_str,
> + param_id, rx_size);
> +}
> +
> +static int qcom_scmi_start_activity(const struct scmi_protocol_handle *ph, void *buf,
> + size_t buf_len, u64 algo_str, u32 param_id)
> +{
> + return qcom_scmi_common_xfer(ph, QCOM_SCMI_START_ACTIVITY, buf, buf_len, algo_str,
> + param_id, 0);
> +}
> +
> +static int qcom_scmi_stop_activity(const struct scmi_protocol_handle *ph, void *buf,
> + size_t buf_len, u64 algo_str, u32 param_id)
> +{
> + return qcom_scmi_common_xfer(ph, QCOM_SCMI_STOP_ACTIVITY, buf, buf_len, algo_str,
> + param_id, 0);
> +}
> +
> +static struct qcom_generic_ext_ops qcom_proto_ops = {
> + .set_param = qcom_scmi_set_param,
> + .get_param = qcom_scmi_get_param,
> + .start_activity = qcom_scmi_start_activity,
> + .stop_activity = qcom_scmi_stop_activity,
> +};
> +
> +static int qcom_generic_ext_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> + u32 version;
> +
> + ph->xops->version_get(ph, &version);
... please check retval and bailout on error when not even a basic version_get
succedeed...
> +
> + dev_dbg(ph->dev, "QCOM Generic Vendor Version %d.%d\n",
> + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> +
> + return 0;
> +}
> +
> +static const struct scmi_protocol qcom_generic_ext = {
> + .id = SCMI_PROTOCOL_QCOM_GENERIC,
> + .owner = THIS_MODULE,
> + .instance_init = &qcom_generic_ext_protocol_init,
> + .ops = &qcom_proto_ops,
> + .vendor_id = "Qualcomm",
> + .impl_ver = 0x20000,
> +};
> +module_scmi_protocol(qcom_generic_ext);
> +
You may have to add a proper MODULE_ALIAS, if the series on autoload
goes through
https://lore.kernel.org/linux-arm-kernel/20241203200038.3961090-1-cristian.marussi@xxxxxxx/
> +MODULE_DESCRIPTION("QCOM SCMI Generic Vendor protocol");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/scmi_qcom_protocol.h b/include/linux/scmi_qcom_protocol.h
> new file mode 100644
> index 000000000000..465b2522ca29
> --- /dev/null
> +++ b/include/linux/scmi_qcom_protocol.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * SCMI Message Protocol driver QCOM extension header
> + *
> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#ifndef _LINUX_SCMI_QCOM_PROTOCOL_H
> +#define _LINUX_SCMI_QCOM_PROTOCOL_H
> +
> +#include <linux/types.h>
> +
> +#define SCMI_PROTOCOL_QCOM_GENERIC 0x80
> +
> +struct scmi_protocol_handle;
> +
> +/**
> + * struct qcom_generic_ext_ops - represents the various operations provided
> + * by QCOM Generic Vendor Protocol
> + *
> + * @set_param: set parameter specified by param_id and algo_str pair.
> + * @get_param: retrieve parameter specified by param_id and algo_str pair.
> + * @start_activity: initiate a specific activity defined by algo_str.
> + * @stop_activity: halt previously initiated activity defined by algo_str.
> + */
> +struct qcom_generic_ext_ops {
> + int (*set_param)(const struct scmi_protocol_handle *ph, void *buf, size_t buf_len,
> + u64 algo_str, u32 param_id);
> + int (*get_param)(const struct scmi_protocol_handle *ph, void *buf, size_t buf_len,
> + u64 algo_str, u32 param_id, size_t rx_size);
> + int (*start_activity)(const struct scmi_protocol_handle *ph, void *buf, size_t buf_len,
> + u64 algo_str, u32 param_id);
> + int (*stop_activity)(const struct scmi_protocol_handle *ph, void *buf, size_t buf_len,
> + u64 algo_str, u32 param_id);
> +};
> +
> +#endif /* _LINUX_SCMI_QCOM_PROTOCOL_H */
> --
Thanks,
Cristian
