Re: [RFC V3 2/4] firmware: arm_scmi: vendors: Add ARM SCMI QCOM vendor protocol v1.0

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 7/9/24 23:22, Cristian Marussi wrote:
On Wed, Jul 03, 2024 at 12:44:38AM +0530, Sibi Sankar wrote:
The ARM SCMI QCOM vendor protocol provides a generic way of exposing a
number of Qualcomm SoC specific features (like memory bus scaling) through
a mixture of pre-determined algorithm strings and param_id pairs hosted on
the SCMI controller.


Hi Sibi,

Co-developed-by: Shivnandan Kumar <quic_kshivnan@xxxxxxxxxxx>
Signed-off-by: Shivnandan Kumar <quic_kshivnan@xxxxxxxxxxx>
Co-developed-by: Ramakrishna Gottimukkula <quic_rgottimu@xxxxxxxxxxx>
Signed-off-by: Ramakrishna Gottimukkula <quic_rgottimu@xxxxxxxxxxx>
Co-developed-by: Amir Vajid <avajid@xxxxxxxxxxx>
Signed-off-by: Amir Vajid <avajid@xxxxxxxxxxx>
Signed-off-by: Sibi Sankar <quic_sibis@xxxxxxxxxxx>
---
  drivers/firmware/arm_scmi/vendors/Kconfig     |  12 ++
  drivers/firmware/arm_scmi/vendors/Makefile    |   2 +-
  .../arm_scmi/vendors/qcom_scmi_vendor.c       | 184 ++++++++++++++++++
  include/linux/qcom_scmi_vendor.h              |  39 ++++
  4 files changed, 236 insertions(+), 1 deletion(-)
  create mode 100644 drivers/firmware/arm_scmi/vendors/qcom_scmi_vendor.c
  create mode 100644 include/linux/qcom_scmi_vendor.h

diff --git a/drivers/firmware/arm_scmi/vendors/Kconfig b/drivers/firmware/arm_scmi/vendors/Kconfig
index 7c1ca7a12603..6bff4550fa25 100644
--- a/drivers/firmware/arm_scmi/vendors/Kconfig
+++ b/drivers/firmware/arm_scmi/vendors/Kconfig
@@ -1,4 +1,16 @@
  # SPDX-License-Identifier: GPL-2.0-only
  menu "ARM SCMI Vendor Protocols"
+config ARM_SCMI_PROTOCOL_VENDOR_QCOM
+	tristate "Qualcomm Technologies, Inc. Qcom SCMI vendor Protocol"
+	depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
+	help
+	  The SCMI QCOM vendor protocol provides a generic way of exposing a
+	  number of Qualcomm SoC specific features (like memory bus scaling)
+	  through a mixture of pre-determined algorithm strings and param_id
+	  pairs hosted on the SCMI controller.
+
+	  This driver defines/documents the message ID's used for this
+	  communication and also exposes the ops used by the clients.

operations

+
  endmenu
diff --git a/drivers/firmware/arm_scmi/vendors/Makefile b/drivers/firmware/arm_scmi/vendors/Makefile
index c6c214158dd8..c1d6a355f579 100644
--- a/drivers/firmware/arm_scmi/vendors/Makefile
[...]
+++ b/drivers/firmware/arm_scmi/vendors/Makefile
+	if (ret)
+		return ret;
+
+	msg = t->tx.buf;
+	msg->algo_low = cpu_to_le32(lower_32_bits(algo_str));
+	msg->algo_high = cpu_to_le32(upper_32_bits(algo_str));
+	msg->param_id = cpu_to_le32(param_id);
+
+	memcpy(msg->buf, buf, t->tx.len - sizeof(*msg));
+
+	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)
+{

Similarly...and looking at my past ramblings...this rx_size is the expected RX
size AND also the size of the provided *buf too, right ?

+	struct scmi_xfer *t;
+	struct qcom_scmi_msg *msg;
+	int ret;
+
+	ret = ph->xops->xfer_get_init(ph, QCOM_SCMI_GET_PARAM, tx_size + sizeof(*msg), rx_size, &t);
+	if (ret)
+		return ret;
+
+	msg = t->tx.buf;
+	msg->algo_low = cpu_to_le32(lower_32_bits(algo_str));
+	msg->algo_high = cpu_to_le32(upper_32_bits(algo_str));
+	msg->param_id = cpu_to_le32(param_id);
+	memcpy(msg->buf, buf, t->tx.len - sizeof(*msg));
+
+	ret = ph->xops->do_xfer(ph, t);
+	memcpy(buf, t->rx.buf, t->rx.len);

...so that this memcpy is safe since rx.len is equal to rx_size by construction
(if I read correctly my past review/mublings...)

...in that case maybe, for better clarity you could re-name the rx_size
param as buf_len and have it following *buf in the param list...


...sorry for not having spotted this naming/order niptick earlier ...

the only caveat being rx_size can be lower than the tx_size
and we dont't want to copy more that what we expect. Addressed
all your other comments from the series in V4. Sry it took a
while due to its dependency with scmi perf changes. Thanks
again.

-Sibi

[...]

Thanks,
Cristian




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux