Re: [RFC 3/7] firmware: arm_scmi: Add QCOM vendor protocol
From: Dmitry Baryshkov
Date: Wed Jan 17 2024 - 14:10:10 EST
On Wed, 17 Jan 2024 at 19:36, Sibi Sankar <quic_sibis@xxxxxxxxxxx> wrote:
>
> From: Shivnandan Kumar <quic_kshivnan@xxxxxxxxxxx>
>
> SCMI QCOM vendor protocol provides interface to communicate with SCMI
> controller and enable vendor specific features like bus scaling capable
> of running on it.
>
> 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>
> Co-developed-by: Sibi Sankar <quic_sibis@xxxxxxxxxxx>
> Signed-off-by: Sibi Sankar <quic_sibis@xxxxxxxxxxx>
> ---
> drivers/firmware/arm_scmi/Kconfig | 11 ++
> drivers/firmware/arm_scmi/Makefile | 1 +
> drivers/firmware/arm_scmi/qcom_scmi_vendor.c | 160 +++++++++++++++++++
> include/linux/qcom_scmi_vendor.h | 36 +++++
> 4 files changed, 208 insertions(+)
> create mode 100644 drivers/firmware/arm_scmi/qcom_scmi_vendor.c
> create mode 100644 include/linux/qcom_scmi_vendor.h
>
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index aa5842be19b2..86b5d6c18ec4 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -180,4 +180,15 @@ config ARM_SCMI_POWER_CONTROL
> called scmi_power_control. Note this may needed early in boot to catch
> early shutdown/reboot SCMI requests.
>
> +config QCOM_SCMI_VENDOR_PROTOCOL
> + tristate "Qualcomm Technologies, Inc. Qcom SCMI vendor Protocol"
> + depends on ARM || ARM64 || COMPILE_TEST
> + depends on ARM_SCMI_PROTOCOL
> + help
> + The SCMI QCOM vendor protocol provides interface to communicate with SCMI
> + controller and enable vendor specific features like bus scaling.
> +
> + This driver defines the commands or message ID's used for this
> + communication and also exposes the ops used by the clients.
> +
> endmenu
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index a7bc4796519c..eaeb788b93c6 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o
>
> obj-$(CONFIG_ARM_SCMI_POWER_CONTROL) += scmi_power_control.o
> +obj-$(CONFIG_QCOM_SCMI_VENDOR_PROTOCOL) += qcom_scmi_vendor.o
>
> ifeq ($(CONFIG_THUMB2_KERNEL)$(CONFIG_CC_IS_CLANG),yy)
> # The use of R7 in the SMCCC conflicts with the compiler's use of R7 as a frame
> diff --git a/drivers/firmware/arm_scmi/qcom_scmi_vendor.c b/drivers/firmware/arm_scmi/qcom_scmi_vendor.c
> new file mode 100644
> index 000000000000..878b99f0d1ef
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/qcom_scmi_vendor.c
> @@ -0,0 +1,160 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/qcom_scmi_vendor.h>
> +
> +#include "common.h"
> +
> +#define EXTENDED_MSG_ID 0
> +#define SCMI_MAX_TX_RX_SIZE 128
> +#define PROTOCOL_PAYLOAD_SIZE 16
> +#define SET_PARAM 0x10
> +#define GET_PARAM 0x11
> +#define START_ACTIVITY 0x12
> +#define STOP_ACTIVITY 0x13
> +
> +static int qcom_scmi_set_param(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
> + u32 param_id, size_t size)
> +{
> + int ret = -EINVAL;
> + struct scmi_xfer *t;
> + u32 *msg;
> +
> + if (!ph || !ph->xops)
> + return ret;
Drop init of ret, return -EINVAL directly here.
> +
> + ret = ph->xops->xfer_get_init(ph, SET_PARAM, size + PROTOCOL_PAYLOAD_SIZE,
> + SCMI_MAX_TX_RX_SIZE, &t);
> + if (ret)
> + return ret;
> +
> + msg = t->tx.buf;
> + *msg++ = cpu_to_le32(EXTENDED_MSG_ID);
> + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0));
> + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32);
> + *msg++ = cpu_to_le32(param_id);
First, this header ops looks like a generic code which can be extracted.
Second, using GENMASK here in the ops doesn't make any sense. The
values will be limited to u32 anyway.
> + memcpy(msg, buf, size);
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int qcom_scmi_get_param(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
> + u32 param_id, size_t tx_size, size_t rx_size)
> +{
> + int ret = -EINVAL;
> + struct scmi_xfer *t;
> + u32 *msg;
> +
> + if (!ph || !ph->xops || !buf)
> + return ret;
Drop init of ret, return -EINVAL directly here.
> +
> + ret = ph->xops->xfer_get_init(ph, GET_PARAM, tx_size + PROTOCOL_PAYLOAD_SIZE,
> + SCMI_MAX_TX_RX_SIZE, &t);
> + if (ret)
> + return ret;
> +
> + msg = t->tx.buf;
> + *msg++ = cpu_to_le32(EXTENDED_MSG_ID);
> + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0));
> + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32);
> + *msg++ = cpu_to_le32(param_id);
> + memcpy(msg, buf, tx_size);
> + ret = ph->xops->do_xfer(ph, t);
> + if (t->rx.len > rx_size) {
> + pr_err("SCMI received buffer size %zu is more than expected size %zu\n",
> + t->rx.len, rx_size);
> + return -EMSGSIZE;
> + }
> + memcpy(buf, t->rx.buf, t->rx.len);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int qcom_scmi_start_activity(const struct scmi_protocol_handle *ph,
> + void *buf, u64 algo_str, u32 param_id, size_t size)
> +{
> + int ret = -EINVAL;
> + struct scmi_xfer *t;
> + u32 *msg;
> +
> + if (!ph || !ph->xops)
> + return ret;
You can guess the comment here.
> +
> + ret = ph->xops->xfer_get_init(ph, START_ACTIVITY, size + PROTOCOL_PAYLOAD_SIZE,
> + SCMI_MAX_TX_RX_SIZE, &t);
> + if (ret)
> + return ret;
> +
> + msg = t->tx.buf;
> + *msg++ = cpu_to_le32(EXTENDED_MSG_ID);
> + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0));
> + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32);
> + *msg++ = cpu_to_le32(param_id);
> + memcpy(msg, buf, size);
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int qcom_scmi_stop_activity(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
> + u32 param_id, size_t size)
> +{
> + int ret = -EINVAL;
> + struct scmi_xfer *t;
> + u32 *msg;
> +
> + if (!ph || !ph->xops)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, STOP_ACTIVITY, size + PROTOCOL_PAYLOAD_SIZE,
> + SCMI_MAX_TX_RX_SIZE, &t);
> + if (ret)
> + return ret;
> +
> + msg = t->tx.buf;
> + *msg++ = cpu_to_le32(EXTENDED_MSG_ID);
> + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0));
> + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32);
> + *msg++ = cpu_to_le32(param_id);
> + memcpy(msg, buf, size);
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static struct qcom_scmi_vendor_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_scmi_vendor_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> + u32 version;
> +
> + ph->xops->version_get(ph, &version);
> +
> + dev_info(ph->dev, "qcom scmi version %d.%d\n",
> + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> +
> + return 0;
> +}
> +
> +static const struct scmi_protocol qcom_scmi_vendor = {
> + .id = QCOM_SCMI_VENDOR_PROTOCOL,
> + .owner = THIS_MODULE,
> + .instance_init = &qcom_scmi_vendor_protocol_init,
> + .ops = &qcom_proto_ops,
> +};
> +module_scmi_protocol(qcom_scmi_vendor);
> +
> +MODULE_DESCRIPTION("QTI SCMI vendor protocol");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/qcom_scmi_vendor.h b/include/linux/qcom_scmi_vendor.h
> new file mode 100644
> index 000000000000..bde57bb18367
> --- /dev/null
> +++ b/include/linux/qcom_scmi_vendor.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * QTI SCMI vendor protocol's header
> + *
> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef _QCOM_SCMI_VENDOR_H
> +#define _QCOM_SCMI_VENDOR_H
> +
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/types.h>
> +
> +#define QCOM_SCMI_VENDOR_PROTOCOL 0x80
> +
> +struct scmi_protocol_handle;
> +extern struct scmi_device *get_qcom_scmi_device(void);
> +
> +/**
> + * struct qcom_scmi_vendor_ops - represents the various operations provided
> + * by qcom scmi vendor protocol
> + */
> +struct qcom_scmi_vendor_ops {
> + int (*set_param)(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
> + u32 param_id, size_t size);
> + int (*get_param)(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
> + u32 param_id, size_t tx_size, size_t rx_size);
> + int (*start_activity)(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
> + u32 param_id, size_t size);
> + int (*stop_activity)(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
> + u32 param_id, size_t size);
> +};
> +
> +#endif /* _QCOM_SCMI_VENDOR_H */
> +
> --
> 2.34.1
>
>
--
With best wishes
Dmitry