| Message ID | 20240117173458.2312669-4-quic_sibis@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | firmware: arm_scmi: Qualcomm Vendor Protocol | expand |
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. > > 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 > >
On 1/17/24 18:34, 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. "QCOM protocol" sounds overly generic, especially given how many different vendor protocols have historically been present in QC firmware.. > > 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> > --- So, this is another 0x80 protocol, different to the one that has been shipping on devices that got released with msm-5.4, msm-5.10 and msm-5.15 [1][2]. They're totally incompatible (judging by the msg format), use the same protocol ID and they are (at a glance) providing access to the same HW/FW/tunables. I'm not sure if this can be trusted not to change again.. Unless we get a strong commitment that all platforms (compute, mobile, auto, iot, whatever) stick to this one.. That said, the spec (DEN0056C) says that protocol IDs 0x80-0xff are: "Reserved for vendor or platform-specific extensions to this interface.". So if perhaps there's a will to maintain multiple versions of this, with a way to discern between them.. Konrad [1] https://git.codelinaro.org/clo/la/kernel/msm-5.15/-/blob/KERNEL.PLATFORM.2.1.r5-05400-kernel.0/drivers/firmware/arm_scmi/memlat_vendor.c?ref_type=tags [2] https://git.codelinaro.org/clo/la/kernel/msm-5.15/-/blob/KERNEL.PLATFORM.2.1.r5-05400-kernel.0/include/linux/scmi_memlat.h#L16
On 1/17/24 18:34, 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. > > 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> > --- [...] > + > +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; After you apply Dmitry's suggestions on returning -EINVAL directly, you can also sort definitions in a reverse-Christmas- tree order throughout the file. > + 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); lower/upper_32_bits()? [...] > + 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; No other driver seems to be checking for this, should this: a) go to common code b) be ignored ? Konrad
On Wed, Jan 17, 2024 at 09:15:40PM +0100, Konrad Dybcio wrote: > > > On 1/17/24 18:34, Sibi Sankar wrote: > > From: Shivnandan Kumar <quic_kshivnan@quicinc.com> > > Hi, a few early remarks, I am gonna look at this better next week. > > SCMI QCOM vendor protocol provides interface to communicate with SCMI > > controller and enable vendor specific features like bus scaling capable > > of running on it. > > "QCOM protocol" sounds overly generic, especially given how many > different vendor protocols have historically been present in > QC firmware.. > I was going to raise the same point :D, usually the name identifies the aim of the protocol (and the vendor also in this case) > > > > 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> > > --- > > So, this is another 0x80 protocol, different to the one that has > been shipping on devices that got released with msm-5.4, msm-5.10 > and msm-5.15 [1][2]. They're totally incompatible (judging by the > msg format), use the same protocol ID and they are (at a glance) > providing access to the same HW/FW/tunables. > > I'm not sure if this can be trusted not to change again.. Unless > we get a strong commitment that all platforms (compute, mobile, > auto, iot, whatever) stick to this one.. > > That said, the spec (DEN0056C) says that protocol IDs 0x80-0xff > are: "Reserved for vendor or platform-specific extensions to > this interface.". So if perhaps there's a will to maintain > multiple versions of this, with a way to discern between them.. > Just recently we had a discussion with some other vendor about this possible clashing of vendor protocols numbers between different vendors/platforms, especially if aiming to just push all in defconfig. The basic idea to solve this, which I am going to post shortly in the next weeks, was to add a way to define and register your protocol number associated with a vendor identifier(s) of some kind, since vendor/subvendor/firmware versions are advertised by the Platform and are retrieved via Base protocol at probe time upfront; this way it 'should' be feasible to compile in any existent vendor protocol but allow at run-time only the registration with the SCMI core of the protocols whose vendor identity matches that of the identity advertised by the running firmware.... ...still not sure which of the IDs to use vendor/subvendor and still not have really experimented with this...so any feedback welcome. This would rule out, anyway, the capability of solving number clashes within the same vendor. Thanks, Cristian
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. > > 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. > 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. > +#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. > + > +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.
On 1/18/24 01:45, Konrad Dybcio wrote: > > > On 1/17/24 18:34, 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 Konrad, > "QCOM protocol" sounds overly generic, especially given how many > different vendor protocols have historically been present in > QC firmware.. Here it is specifically mentioned that way to communicate that this is the only vendor protocol exposed by Qualcomm. It handles all the other protocols which were usually handled separately on older SoCs. > >> >> 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> >> --- > > So, this is another 0x80 protocol, different to the one that has > been shipping on devices that got released with msm-5.4, msm-5.10 > and msm-5.15 [1][2]. They're totally incompatible (judging by the > msg format), use the same protocol ID and they are (at a glance) > providing access to the same HW/FW/tunables. Thanks for bringing this up but like I already explained the only SoC that was actually shipped with ^^ protocol was SC7180 and we already have an alternative arrangement for memory dvfs upstreamed on it. Further more it handles only L3 dvfs so it makes zero sense to try to upstream the older protocol given that working dvfs solution already exists upstream. All other SoCs don't have the 0x80 protocol enabled for memory dvfs in production. > > I'm not sure if this can be trusted not to change again.. Unless > we get a strong commitment that all platforms (compute, mobile, > auto, iot, whatever) stick to this one.. This is exactly that consolidation effort from Qualcomm. Here they expose just one vendor protocol and implement all the algorithms just through it. > > That said, the spec (DEN0056C) says that protocol IDs 0x80-0xff > are: "Reserved for vendor or platform-specific extensions to > this interface.". So if perhaps there's a will to maintain > multiple versions of this, with a way to discern between them.. > > Konrad > > [1] > https://git.codelinaro.org/clo/la/kernel/msm-5.15/-/blob/KERNEL.PLATFORM.2.1.r5-05400-kernel.0/drivers/firmware/arm_scmi/memlat_vendor.c?ref_type=tags > [2] > https://git.codelinaro.org/clo/la/kernel/msm-5.15/-/blob/KERNEL.PLATFORM.2.1.r5-05400-kernel.0/include/linux/scmi_memlat.h#L16
On 8.02.2024 12:44, Sibi Sankar wrote: > > > On 1/18/24 01:45, Konrad Dybcio wrote: >> >> >> On 1/17/24 18:34, 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 Konrad, > >> "QCOM protocol" sounds overly generic, especially given how many >> different vendor protocols have historically been present in >> QC firmware.. > > Here it is specifically mentioned that way to communicate that > this is the only vendor protocol exposed by Qualcomm. It handles > all the other protocols which were usually handled separately on > older SoCs. I'm no SCMI specialist but that's a rather.. peculiar design decision, I guess > >> >>> >>> 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> >>> --- >> >> So, this is another 0x80 protocol, different to the one that has >> been shipping on devices that got released with msm-5.4, msm-5.10 >> and msm-5.15 [1][2]. They're totally incompatible (judging by the >> msg format), use the same protocol ID and they are (at a glance) >> providing access to the same HW/FW/tunables. > > Thanks for bringing this up but like I already explained the only > SoC that was actually shipped with ^^ protocol was SC7180 and we > already have an alternative arrangement for memory dvfs upstreamed > on it. Ok, that makes sense. I took my 8550 phone, enabled some debug prints and it looks like the only SCMI protocol exposed is 0x19 (which doesn't seem to be defined). Not sure what other devices would spit out, but I assume what you said is true. For completeness, the reported rev is: arm-scmi firmware:scmi: SCMI Protocol v2.0 'Qualcomm:' Firmware version 0x10000 > Further more it handles only L3 dvfs so it makes zero sense > to try to upstream the older protocol given that working dvfs solution > already exists upstream. We don't have any sort of governor for it though, so I wouldn't go as far as calling it working :P > All other SoCs don't have the 0x80 protocol > enabled for memory dvfs in production. > >> >> I'm not sure if this can be trusted not to change again.. Unless >> we get a strong commitment that all platforms (compute, mobile, >> auto, iot, whatever) stick to this one.. > > This is exactly that consolidation effort from Qualcomm. Here they > expose just one vendor protocol and implement all the algorithms just > through it. And I'm very glad you're taking such consolidation steps.. Just a little worried that in case this protocol's extensibility is exhausted, the next one would need to be called.. "Qualcomm2"? Konrad
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 >> >> > >
On 2/10/24 04:15, Konrad Dybcio wrote: > On 8.02.2024 12:44, Sibi Sankar wrote: >> >> >> On 1/18/24 01:45, Konrad Dybcio wrote: >>> >>> >>> On 1/17/24 18:34, 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 Konrad, >> >>> "QCOM protocol" sounds overly generic, especially given how many >>> different vendor protocols have historically been present in >>> QC firmware.. >> >> Here it is specifically mentioned that way to communicate that >> this is the only vendor protocol exposed by Qualcomm. It handles >> all the other protocols which were usually handled separately on >> older SoCs. > > I'm no SCMI specialist but that's a rather.. peculiar design decision, > I guess > > >> >>> >>>> >>>> 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> >>>> --- >>> >>> So, this is another 0x80 protocol, different to the one that has >>> been shipping on devices that got released with msm-5.4, msm-5.10 >>> and msm-5.15 [1][2]. They're totally incompatible (judging by the >>> msg format), use the same protocol ID and they are (at a glance) >>> providing access to the same HW/FW/tunables. >> >> Thanks for bringing this up but like I already explained the only >> SoC that was actually shipped with ^^ protocol was SC7180 and we >> already have an alternative arrangement for memory dvfs upstreamed >> on it. > > Ok, that makes sense. > > I took my 8550 phone, enabled some debug prints and it looks like the > only SCMI protocol exposed is 0x19 (which doesn't seem to be defined). > > Not sure what other devices would spit out, but I assume what you said > is true. > > For completeness, the reported rev is: > > arm-scmi firmware:scmi: SCMI Protocol v2.0 'Qualcomm:' Firmware version 0x10000 > >> Further more it handles only L3 dvfs so it makes zero sense >> to try to upstream the older protocol given that working dvfs solution >> already exists upstream. > > We don't have any sort of governor for it though, so I wouldn't go as > far as calling it working :P It is a working solution (it is equivalent to the compute mon mapping in downstream implementation) but isn't feature complete ¯\_(ツ)_/¯. > >> All other SoCs don't have the 0x80 protocol >> enabled for memory dvfs in production. >> >>> >>> I'm not sure if this can be trusted not to change again.. Unless >>> we get a strong commitment that all platforms (compute, mobile, >>> auto, iot, whatever) stick to this one.. >> >> This is exactly that consolidation effort from Qualcomm. Here they >> expose just one vendor protocol and implement all the algorithms just >> through it. > > And I'm very glad you're taking such consolidation steps.. Just a little > worried that in case this protocol's extensibility is exhausted, the next > one would need to be called.. "Qualcomm2"? We don't see ^^ happening in the near future (meaning this doesn't apply to just X1E). The consolidation would still be better than spinning out n number of protocols per SoC. -Sibi > > Konrad
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 >
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. > Hi Sibi, a few comments down below. > 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 > I am starting to think to put vendor protocols in their own dedicated subdir given that a bunch of those appeared recently :D ....have to discuss with Sudeep...anyway not really an issue... any thoughts about this ? > 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; If you get to call this protocol operation, the protocol itself has to have been initialized already and registered with the SCMI core, and get assigned a protocol_handle *ph, so ph and ph->xops are definitely non-NULL here....if they are please report as a bug :P > + > + ret = ph->xops->xfer_get_init(ph, SET_PARAM, size + PROTOCOL_PAYLOAD_SIZE, > + SCMI_MAX_TX_RX_SIZE, &t); This parameter, which you set to SCMI_MAX_TX_RX_SIZE, is meant to carry the max RX payload size for the specific message you are sending, if you known it; if you do NOT known it you can set this to ZERO and the SCMI core will bump it to the maximum message size for the currently configured underlying transport AND check if the reply fits in. Here it seems that you are trying to somehow set the max RX to the max size you know the transport can support (which is indeed 128bytes for nmailbox/shmem), but you dont need to (as explained), it is something that does NOT belong to the protocol layer in fact (if you meant to use the transport layer max size), AND you wont be able in any case to override the underlying maximum RX payload size, since that is the size of the pre-allocated message buffers in the SCMI xore, and it is enforced by xfer_get_init(). So, in case somehow the underlying transport was or will be configured to be shorter than you requested here, you will fail the xfer_get_init() in teh future. > + 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); ...just shift as Konrad (I think) was saying...of use FIELD_GET() (probably overkill) ..moreover... if the message PROTOCOL_PAYLOAD that you use is always the same (surely the same in size) you should just define some sort of: (just making up names here) struct qc_msg { __le32 ext_id; __le32 algo_low; __le32 algo_high; __le32 param_id; __le32 buf[]; } ..so that you can easily write the above as: msg->ext_id = cpu_to_le32(EXTENDED_MSG_ID); ... which is more readable and MOST importantly can be checked by static analyzer like smatch for consistent usage of endianess macros...(that we all love...:P) > + *msg++ = cpu_to_le32(param_id); > + memcpy(msg, buf, size); ...mmm...this is correct at the end since you allocate a TX len buffer of (size + PROTOCOL_PAYLOAD_SIZE) and just move the dst_buf @msg by just PROTOCOL_PAYLOAD_SIZE before the memcpy, BUT the memcpy @size param should represent the maximum amount of bytes that fits into the dst_buf, and here it represent the src_buf size and it WORKS just fine since it is indeed the amount of space left in msg, BUT ONLY because of how you allocate the buffer above depending on the define PROTOCOL_PAYLOAD_SIZE... ...seems to me not so much future/error proof in these regards, what happens if by mistake the msg fields and the define get of sync ? ..what about instead something like (applying also all of the remarks above): struct qc_msg *msg; ret = ph->xops->xfer_get_init(ph, SET_PARAM, size + sizeof(*msg), 0 , &t); ... msg = t->tx.buf; msg->ext_id = cpu_to_le32(EXTENDED_MSG_ID); ... memcpy(msg->buf, buf, t->tx.len - sizeof(*msg)); ...thoughts ? > + 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; > + Ditto. ph and ph->xops checks not needed > + ret = ph->xops->xfer_get_init(ph, GET_PARAM, tx_size + PROTOCOL_PAYLOAD_SIZE, > + SCMI_MAX_TX_RX_SIZE, &t); Shouldn't this be simply: 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); Ditto. qc_msg + above remarks > + ret = ph->xops->do_xfer(ph, t); > + if (t->rx.len > rx_size) { ...if you use above rx_size for the desired payload max size, that will be also used as the t->rx.len by the SCMI core and the configured transport layer to enforce that the buffer RX payload size is not overflowed.... (see drivers/firmware/arm_scmi/shmem.c as an example) ...well you'll get you buffer silently truncated if it is too big than expected...to be honest... ...but in any case you wont need this check...maybe here you can just anyway warn if it is too small than expected (and was truncated)...if you want > + 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; Ditto. > + > + ret = ph->xops->xfer_get_init(ph, START_ACTIVITY, size + PROTOCOL_PAYLOAD_SIZE, > + SCMI_MAX_TX_RX_SIZE, &t); Ditto. > + 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); Ditto. > + 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; Ditto. > + > + ret = ph->xops->xfer_get_init(ph, STOP_ACTIVITY, size + PROTOCOL_PAYLOAD_SIZE, > + SCMI_MAX_TX_RX_SIZE, &t); Ditto. > + 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); Ditto. > + 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); As said already, I posted an RFC, which I am gonna cleanup and repost soon (probably within the week) in order to allow for multiple custom protocols from multipl distinct Vendors to co-exist within the same 0x80-0xFF protocols numbers space....in a nutshell you will have to populate here one or more fields to this struct at compile time so as to be able to identify this protocol as yours...so that we can then compile all vendors protocols into defconfig but then, at run-time, load only the ones matching the effective platform you are running in. I understand that you now have "your one and only protocol to rule them all (0x80)" :P... but this does not mean that other vendors cannot choose that same number of yours for their own protocols (..I think it is already happening), so we need a compile/runtime mechanism to properly select... > + > +MODULE_DESCRIPTION("QTI SCMI vendor protocol"); As already said, it seems a bit strange to have just one protocol where you channel all the current and future stuff...this protocol seems related to _MEMLAT configs at the moment only... ...consider that you can reserve/dedicate a channel to a protocol (if the underlying transport allows) for performance purposes BUT clearly if you stick all of your machinery into one single protocol you wont have this capability... (... and I dont charge for new protocol numbers :P .... joking ah...) > +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); ...what is this extern ? I maybe missing something... > + > +/** > + * 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); > +}; > + Thanks, Cristian
On Mon, Feb 12, 2024 at 05:39:19PM +0000, Cristian Marussi wrote: > > 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 > > > > I am starting to think to put vendor protocols in their own dedicated > subdir given that a bunch of those appeared recently :D > Yes I tend to agree with different subdir for each vendor. Not sure if we need new Kconfig entry or just use ARCH_<vendor/group of SoC> to build all subdir used by that vendor. > ....have to discuss with Sudeep...anyway not really an issue... > > any thoughts about this ? In general, I see lot of discussions on this thread when I was away for past 3 weeks. I will wait for newer version as that seems simpler for me than getting lost to follow the discussions so far.
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. > I would expect a proper description of the protocol specification either as part of the header file qcom_scmi_vendor.h or somewhere in the Documentation. It helps to understand the design instead of assuming and/or getting confused with the assumption while reviewing. I will point out at couple of individual place why I am asking for this. You can follow some pattern to describe the command using SCMI spec as reference. That will act as a contract for the software instead of changing the implementation every time someone thinks it should work in certain way. I have seen that quite a lot with the Qcom firmware lately with zero transparency provided for these firmware by Qcom. In short I don't trust just code to understand these vendor protocols. I need them to be documented and version where needed so that we can refer back and make maintenance smooth and easy.
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; + + 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, + .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 */ +