Received-SPF: pass (google.com: domain of linux-kernel+bounces-61201-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <a777f419-05a5-fecc-b51c-fa442502ac47@quicinc.com>
Date: Mon, 12 Feb 2024 14:01:16 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [RFC 3/7] firmware: arm_scmi: Add QCOM vendor protocol
Content-Language: en-US
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC: <sudeep.holla@arm.com>, <cristian.marussi@arm.com>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <jassisinghbrar@gmail.com>,
        <robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <quic_rgottimu@quicinc.com>,
        <quic_kshivnan@quicinc.com>, <conor+dt@kernel.org>,
        Amir Vajid <avajid@quicinc.com>
References: <20240117173458.2312669-1-quic_sibis@quicinc.com>
 <20240117173458.2312669-4-quic_sibis@quicinc.com>
 <CAA8EJpqUgqHfMOZip58yGpt6S3XSBz+9BeXZSL_JmWar_JbO4g@mail.gmail.com>
From: Sibi Sankar <quic_sibis@quicinc.com>
In-Reply-To: <CAA8EJpqUgqHfMOZip58yGpt6S3XSBz+9BeXZSL_JmWar_JbO4g@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 1/18/24 00:39, Dmitry Baryshkov wrote:
> On Wed, 17 Jan 2024 at 19:36, Sibi Sankar <quic_sibis@quicinc.com> wrote:
>>
>> From: Shivnandan Kumar <quic_kshivnan@quicinc.com>
>>
>> SCMI QCOM vendor protocol provides interface to communicate with SCMI
>> controller and enable vendor specific features like bus scaling capable
>> of running on it.
>>

Hey Dmitry,

Thanks for taking time to review the series!

Will get all of these done in the next re-spin.

>> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
>> Co-developed-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com>
>> Signed-off-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com>
>> Co-developed-by: Amir Vajid <avajid@quicinc.com>
>> Signed-off-by: Amir Vajid <avajid@quicinc.com>
>> Co-developed-by: Sibi Sankar <quic_sibis@quicinc.com>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>   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
>>
>>
> 
>