lkml.org 
[lkml]   [2024]   [Feb]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [RFC 3/7] firmware: arm_scmi: Add QCOM vendor protocol
    From


    On 1/18/24 22:52, Sudeep Holla wrote:
    > On Wed, Jan 17, 2024 at 11:04:54PM +0530, Sibi Sankar 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 Sudeep,

    Thanks for taking time to review the series!

    >>
    >> 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.
    >> +
    >
    > I assume it will include all the Qualcomm specific vendor protocol
    > handling here. Not sure how it it implemented across different platforms
    > and but I already assume different platforms will use same protocol id
    > for different things and this implementation will abstract all those
    > details.

    Yes, that's what we are going for.

    >
    >> 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
    >
    > This gives me no clue what this means ?
    >
    >> +#define SCMI_MAX_TX_RX_SIZE 128
    >> +#define PROTOCOL_PAYLOAD_SIZE 16
    >> +#define SET_PARAM 0x10
    >
    > I assume these are the actual message IDs ? Any idea why 0x0-0xF is skipped ?
    > I assume atleast the required 0x0-0x2 are implemented.

    Yup 0x0-0x2 should be implemented. I'll have to get info on why the rest
    were skipped. Will add comments detailing the extended msg id as well.

    >
    >> +#define GET_PARAM 0x11
    >> +#define START_ACTIVITY 0x12
    >> +#define STOP_ACTIVITY 0x13
    >
    > In general, good to add description of these in the implementation here
    > or under Documentation or a pointer to the url where I can get the info.
    > If documenting within the kernel, please use SCMI spec format as it may
    > be easy to follow the same pattern even in the vendor protocols.
    >

    ack

    >> +
    >> +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;
    >> +
    >> + 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);
    >> + 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;
    >> +
    >> + 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;
    >> +
    >> + 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,
    >
    > As Cristian might have pointed out, this will conflict and we need better
    > matching to ensure each vendor and protocols with each implementation has
    > unique matching mechanism so that only one match occurs per protocol on
    > any platform.

    Ack.

    Also as mentioned in another thread this will be the only implementation
    of the 0x80 vendor protocol upstream given that no other SoC actually
    shipped with it enabled (expect for sc7180 which already has an
    alternative dvfs solution upstream).

    -Sibi

    >

    \
     
     \ /
      Last update: 2024-05-27 14:57    [W:4.592 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site