This patch series introduces the Qualcomm SCMI Vendor protocol and adds a
client driver that interacts with the vendor protocol and passes on the
required tuneables to start various features running on the SCMI controller.
The series specifically enables (LLCC/DDR) dvfs on X1E80100 SoC by passing
several tuneables including the IPM ratio (Instructions Per Miss),
cpu frequency to memory/bus frequency tables, CPU mapping to the vendor
protocol which in turn will enable the memory latency governor running
on the SCMI controller.
Depends on:
limits changed notification v2: https://patchwork.kernel.org/project/linux-arm-msm/cover/[email protected]/
Turbo support: https://patchwork.kernel.org/project/linux-arm-msm/cover/[email protected]/
Shivnandan Kumar (2):
firmware: arm_scmi: Add QCOM vendor protocol
soc: qcom: Utilize qcom scmi vendor protocol for bus dvfs
Sibi Sankar (5):
dt-bindings: mailbox: qcom: Add CPUCP mailbox controller bindings
mailbox: Add support for QTI CPUCP mailbox controller
arm64: dts: qcom: x1e80100: Add cpucp mailbox and sram nodes
arm64: dts: qcom: x1e80100: Enable cpufreq
arm64: dts: qcom: x1e80100: Enable LLCC/DDR dvfs
.../bindings/mailbox/qcom,cpucp-mbox.yaml | 51 ++
arch/arm64/boot/dts/qcom/x1e80100.dtsi | 101 ++++
drivers/firmware/arm_scmi/Kconfig | 11 +
drivers/firmware/arm_scmi/Makefile | 1 +
drivers/firmware/arm_scmi/qcom_scmi_vendor.c | 160 ++++++
drivers/mailbox/Kconfig | 8 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/qcom-cpucp-mbox.c | 265 ++++++++++
drivers/soc/qcom/Kconfig | 10 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/qcom_scmi_client.c | 486 ++++++++++++++++++
include/linux/qcom_scmi_vendor.h | 36 ++
12 files changed, 1132 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
create mode 100644 drivers/firmware/arm_scmi/qcom_scmi_vendor.c
create mode 100644 drivers/mailbox/qcom-cpucp-mbox.c
create mode 100644 drivers/soc/qcom/qcom_scmi_client.c
create mode 100644 include/linux/qcom_scmi_vendor.h
--
2.34.1
Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox
controller.
Signed-off-by: Sibi Sankar <[email protected]>
---
.../bindings/mailbox/qcom,cpucp-mbox.yaml | 51 +++++++++++++++++++
1 file changed, 51 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
diff --git a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
new file mode 100644
index 000000000000..2617e5555acb
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/qcom,cpucp-mbox.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. CPUCP Mailbox Controller
+
+maintainers:
+ - Sibi Sankar <[email protected]>
+
+description:
+ The CPUSS Control Processor (CPUCP) mailbox controller enables communication
+ between AP and CPUCP by acting as a doorbell between them.
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - qcom,x1e80100-cpucp-mbox
+ - const: qcom,cpucp-mbox
+
+ reg:
+ items:
+ - description: CPUCP rx register region
+ - description: CPUCP tx register region
+
+ interrupts:
+ maxItems: 1
+
+ "#mbox-cells":
+ const: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - "#mbox-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+ mailbox@17430000 {
+ compatible = "qcom,x1e80100-cpucp-mbox", "qcom,cpucp-mbox";
+ reg = <0x17430000 0x10000>, <0x18830000 0x300>;
+ interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
+ #mbox-cells = <1>;
+ };
--
2.34.1
Add support for CPUSS Control Processor (CPUCP) mailbox controller,
this driver enables communication between AP and CPUCP by acting as
a doorbell between them.
Signed-off-by: Sibi Sankar <[email protected]>
---
drivers/mailbox/Kconfig | 8 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/qcom-cpucp-mbox.c | 265 ++++++++++++++++++++++++++++++
3 files changed, 275 insertions(+)
create mode 100644 drivers/mailbox/qcom-cpucp-mbox.c
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 42940108a187..23741a6f054e 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -273,6 +273,14 @@ config SPRD_MBOX
to send message between application processors and MCU. Say Y here if
you want to build the Spreatrum mailbox controller driver.
+config QCOM_CPUCP_MBOX
+ tristate "Qualcomm Technologies, Inc. CPUCP mailbox driver"
+ depends on ARCH_QCOM || COMPILE_TEST
+ help
+ Qualcomm Technologies, Inc. CPUSS Control Processor (CPUCP) mailbox
+ controller driver enables communication between AP and CPUCP. Say
+ Y here if you want to build this driver.
+
config QCOM_IPCC
tristate "Qualcomm Technologies, Inc. IPCC driver"
depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 18793e6caa2f..53b512800bde 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -59,4 +59,6 @@ obj-$(CONFIG_SUN6I_MSGBOX) += sun6i-msgbox.o
obj-$(CONFIG_SPRD_MBOX) += sprd-mailbox.o
+obj-$(CONFIG_QCOM_CPUCP_MBOX) += qcom-cpucp-mbox.o
+
obj-$(CONFIG_QCOM_IPCC) += qcom-ipcc.o
diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
new file mode 100644
index 000000000000..22ea6c802286
--- /dev/null
+++ b/drivers/mailbox/qcom-cpucp-mbox.c
@@ -0,0 +1,265 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/platform_device.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+
+#define APSS_CPUCP_IPC_CHAN_SUPPORTED 3
+#define APSS_CPUCP_MBOX_CMD_OFF 0x4
+
+/* Tx Registers */
+#define APSS_CPUCP_TX_MBOX_IDR 0
+#define APSS_CPUCP_TX_MBOX_CMD 0x100
+
+/* Rx Registers */
+#define APSS_CPUCP_RX_MBOX_IDR 0
+#define APSS_CPUCP_RX_MBOX_CMD 0x100
+#define APSS_CPUCP_RX_MBOX_MAP 0x4000
+#define APSS_CPUCP_RX_MBOX_STAT 0x4400
+#define APSS_CPUCP_RX_MBOX_CLEAR 0x4800
+#define APSS_CPUCP_RX_MBOX_EN 0x4C00
+#define APSS_CPUCP_RX_MBOX_CMD_MASK 0xFFFFFFFFFFFFFFFF
+
+/**
+ * struct qcom_cpucp_mbox - Holder for the mailbox driver
+ * @chans: The mailbox channel
+ * @mbox: The mailbox controller
+ * @tx_base: Base address of the CPUCP tx registers
+ * @rx_base: Base address of the CPUCP rx registers
+ * @dev: Device associated with this instance
+ * @irq: CPUCP to AP irq
+ */
+struct qcom_cpucp_mbox {
+ struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
+ struct mbox_controller mbox;
+ void __iomem *tx_base;
+ void __iomem *rx_base;
+ struct device *dev;
+ int irq;
+ int num_chan;
+};
+
+static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
+{
+ struct qcom_cpucp_mbox *cpucp = data;
+ u64 status;
+ u32 val;
+ int i;
+
+ status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
+
+ for (i = 0; i < cpucp->num_chan; i++) {
+ val = 0;
+ if (status & ((u64)1 << i)) {
+ val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD + (i * 8) + APSS_CPUCP_MBOX_CMD_OFF);
+ if (!IS_ERR(cpucp->chans[i].con_priv))
+ mbox_chan_received_data(&cpucp->chans[i], &val);
+ writeq(status, cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
+ }
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
+{
+ struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
+ unsigned long chan_id = (unsigned long)chan->con_priv;
+ u64 val;
+
+ val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
+ val |= ((u64)1 << chan_id);
+ writeq(val, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
+
+ return 0;
+}
+
+static void qcom_cpucp_mbox_shutdown(struct mbox_chan *chan)
+{
+ struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
+ unsigned long chan_id = (unsigned long)chan->con_priv;
+ u64 val;
+
+ val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
+ val &= ~((u64)1 << chan_id);
+ writeq(val, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
+
+ chan->con_priv = ERR_PTR(-EINVAL);
+}
+
+static int qcom_cpucp_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+ struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
+ unsigned long chan_id = (unsigned long)chan->con_priv;
+ u32 val = (unsigned long)data;
+
+ writel(val, cpucp->tx_base + APSS_CPUCP_TX_MBOX_CMD + (chan_id * 8) + APSS_CPUCP_MBOX_CMD_OFF);
+
+ return 0;
+}
+
+static struct mbox_chan *qcom_cpucp_mbox_xlate(struct mbox_controller *mbox,
+ const struct of_phandle_args *sp)
+{
+ unsigned long ind = sp->args[0];
+
+ if (sp->args_count != 1)
+ return ERR_PTR(-EINVAL);
+
+ if (ind >= mbox->num_chans)
+ return ERR_PTR(-EINVAL);
+
+ if (!IS_ERR(mbox->chans[ind].con_priv))
+ return ERR_PTR(-EBUSY);
+
+ mbox->chans[ind].con_priv = (void *)ind;
+
+ return &mbox->chans[ind];
+}
+
+static const struct mbox_chan_ops qcom_cpucp_mbox_chan_ops = {
+ .startup = qcom_cpucp_mbox_startup,
+ .send_data = qcom_cpucp_mbox_send_data,
+ .shutdown = qcom_cpucp_mbox_shutdown
+};
+
+static int qcom_cpucp_setup_mbox(struct qcom_cpucp_mbox *cpucp)
+{
+ struct device *dev = cpucp->dev;
+ struct mbox_controller *mbox;
+ unsigned long i;
+
+ /* Initialize channel identifiers */
+ for (i = 0; i < ARRAY_SIZE(cpucp->chans); i++)
+ cpucp->chans[i].con_priv = ERR_PTR(-EINVAL);
+
+ mbox = &cpucp->mbox;
+ mbox->dev = dev;
+ mbox->num_chans = cpucp->num_chan;
+ mbox->chans = cpucp->chans;
+ mbox->ops = &qcom_cpucp_mbox_chan_ops;
+ mbox->of_xlate = qcom_cpucp_mbox_xlate;
+ mbox->txdone_irq = false;
+ mbox->txdone_poll = false;
+
+ return mbox_controller_register(mbox);
+}
+
+static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
+{
+ struct qcom_cpucp_mbox *cpucp;
+ struct resource *res;
+ int ret;
+
+ cpucp = devm_kzalloc(&pdev->dev, sizeof(*cpucp), GFP_KERNEL);
+ if (!cpucp)
+ return -ENOMEM;
+
+ cpucp->dev = &pdev->dev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(&pdev->dev, "Failed to get the cpucp rx base address\n");
+ return -ENODEV;
+ }
+
+ cpucp->rx_base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+ if (!cpucp->rx_base) {
+ dev_err(&pdev->dev, "Failed to ioremap cpucp tx base\n");
+ return -ENOMEM;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (!res) {
+ dev_err(&pdev->dev, "Failed to get the cpucp tx base address\n");
+ return -ENODEV;
+ }
+
+ cpucp->tx_base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+ if (!cpucp->tx_base) {
+ dev_err(&pdev->dev, "Failed to ioremap cpucp tx base\n");
+ return -ENOMEM;
+ }
+
+ writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
+ writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
+ writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
+
+ cpucp->irq = platform_get_irq(pdev, 0);
+ if (cpucp->irq < 0) {
+ dev_err(&pdev->dev, "Failed to get the IRQ\n");
+ return cpucp->irq;
+ }
+
+ ret = devm_request_irq(&pdev->dev, cpucp->irq, qcom_cpucp_mbox_irq_fn,
+ IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret);
+ return ret;
+ }
+
+ writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
+
+ cpucp->num_chan = APSS_CPUCP_IPC_CHAN_SUPPORTED;
+ ret = qcom_cpucp_setup_mbox(cpucp);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to create mailbox\n");
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, cpucp);
+
+ return 0;
+}
+
+static int qcom_cpucp_mbox_remove(struct platform_device *pdev)
+{
+ struct qcom_cpucp_mbox *cpucp = platform_get_drvdata(pdev);
+
+ mbox_controller_unregister(&cpucp->mbox);
+
+ return 0;
+}
+
+static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
+ { .compatible = "qcom,cpucp-mbox"},
+ {}
+};
+MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
+
+static struct platform_driver qcom_cpucp_mbox_driver = {
+ .probe = qcom_cpucp_mbox_probe,
+ .remove = qcom_cpucp_mbox_remove,
+ .driver = {
+ .name = "qcom_cpucp_mbox",
+ .of_match_table = qcom_cpucp_mbox_of_match,
+ .suppress_bind_attrs = true,
+ },
+};
+
+static int __init qcom_cpucp_mbox_init(void)
+{
+ int ret;
+
+ ret = platform_driver_register(&qcom_cpucp_mbox_driver);
+ if (ret)
+ pr_err("%s: qcom_cpucp_mbox register failed %d\n", __func__, ret);
+
+ return ret;
+}
+module_init(qcom_cpucp_mbox_init);
+
+static __exit void qcom_cpucp_mbox_exit(void)
+{
+ platform_driver_unregister(&qcom_cpucp_mbox_driver);
+}
+module_exit(qcom_cpucp_mbox_exit);
+
+MODULE_DESCRIPTION("QTI CPUCP MBOX Driver");
+MODULE_LICENSE("GPL");
--
2.34.1
From: Shivnandan Kumar <[email protected]>
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 <[email protected]>
Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
Co-developed-by: Amir Vajid <[email protected]>
Signed-off-by: Amir Vajid <[email protected]>
Co-developed-by: Sibi Sankar <[email protected]>
Signed-off-by: Sibi Sankar <[email protected]>
---
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;
+
+ 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 */
+
--
2.34.1
From: Shivnandan Kumar <[email protected]>
This patch introduces a client driver that interacts with the SCMI QCOM
vendor protocol and passes on the required tuneables to start various
features running on the SCMI controller.
Signed-off-by: Shivnandan Kumar <[email protected]>
Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
Co-developed-by: Amir Vajid <[email protected]>
Signed-off-by: Amir Vajid <[email protected]>
Co-developed-by: Sibi Sankar <[email protected]>
Signed-off-by: Sibi Sankar <[email protected]>
---
drivers/soc/qcom/Kconfig | 10 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/qcom_scmi_client.c | 486 ++++++++++++++++++++++++++++
3 files changed, 497 insertions(+)
create mode 100644 drivers/soc/qcom/qcom_scmi_client.c
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index c6ca4de42586..1530558aebfb 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -264,6 +264,16 @@ config QCOM_ICC_BWMON
the fixed bandwidth votes from cpufreq (CPU nodes) thus achieve high
memory throughput even with lower CPU frequencies.
+config QCOM_SCMI_CLIENT
+ tristate "Qualcomm Technologies Inc. SCMI client driver"
+ depends on QCOM_SCMI_VENDOR_PROTOCOL || COMPILE_TEST
+ default n
+ help
+ SCMI client driver registers for SCMI QCOM vendor protocol.
+
+ This driver interacts with the vendor protocol and passes on the required
+ tuneables to start various features running on the SCMI controller.
+
config QCOM_INLINE_CRYPTO_ENGINE
tristate
select QCOM_SCM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 05b3d54e8dc9..c2a51293c886 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -32,5 +32,6 @@ obj-$(CONFIG_QCOM_APR) += apr.o
obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
+obj-$(CONFIG_QCOM_SCMI_CLIENT) += qcom_scmi_client.o
qcom_ice-objs += ice.o
obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom_ice.o
diff --git a/drivers/soc/qcom/qcom_scmi_client.c b/drivers/soc/qcom/qcom_scmi_client.c
new file mode 100644
index 000000000000..418aa7900496
--- /dev/null
+++ b/drivers/soc/qcom/qcom_scmi_client.c
@@ -0,0 +1,486 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/cpu.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/qcom_scmi_vendor.h>
+#include <linux/scmi_protocol.h>
+
+#define MAX_MEMORY_TYPES 3
+#define MEMLAT_ALGO_STR 0x74616C6D656D /* "memlat" */
+#define INVALID_IDX 0xFF
+#define MAX_NAME_LEN 20
+#define MAX_MAP_ENTRIES 6
+#define MAX_MONITOR_CNT 4
+#define SCMI_VENDOR_MSG_START 3
+#define SCMI_VENDOR_MSG_MODULE_START 16
+
+enum scmi_memlat_protocol_cmd {
+ MEMLAT_SET_LOG_LEVEL = SCMI_VENDOR_MSG_START,
+ MEMLAT_FLUSH_LOGBUF,
+ MEMLAT_SET_MEM_GROUP = SCMI_VENDOR_MSG_MODULE_START,
+ MEMLAT_SET_MONITOR,
+ MEMLAT_SET_COMMON_EV_MAP,
+ MEMLAT_SET_GRP_EV_MAP,
+ MEMLAT_ADAPTIVE_LOW_FREQ,
+ MEMLAT_ADAPTIVE_HIGH_FREQ,
+ MEMLAT_GET_ADAPTIVE_CUR_FREQ,
+ MEMLAT_IPM_CEIL,
+ MEMLAT_FE_STALL_FLOOR,
+ MEMLAT_BE_STALL_FLOOR,
+ MEMLAT_WB_PCT,
+ MEMLAT_IPM_FILTER,
+ MEMLAT_FREQ_SCALE_PCT,
+ MEMLAT_FREQ_SCALE_CEIL_MHZ,
+ MEMLAT_FREQ_SCALE_FLOOR_MHZ,
+ MEMLAT_SAMPLE_MS,
+ MEMLAT_MON_FREQ_MAP,
+ MEMLAT_SET_MIN_FREQ,
+ MEMLAT_SET_MAX_FREQ,
+ MEMLAT_GET_CUR_FREQ,
+ MEMLAT_START_TIMER,
+ MEMLAT_STOP_TIMER,
+ MEMLAT_GET_TIMESTAMP,
+ MEMLAT_MAX_MSG
+};
+
+struct map_table {
+ u16 v1;
+ u16 v2;
+};
+
+struct map_param_msg {
+ u32 hw_type;
+ u32 mon_idx;
+ u32 nr_rows;
+ struct map_table tbl[MAX_MAP_ENTRIES];
+} __packed;
+
+struct node_msg {
+ u32 cpumask;
+ u32 hw_type;
+ u32 mon_type;
+ u32 mon_idx;
+ char mon_name[MAX_NAME_LEN];
+};
+
+struct scalar_param_msg {
+ u32 hw_type;
+ u32 mon_idx;
+ u32 val;
+};
+
+enum common_ev_idx {
+ INST_IDX,
+ CYC_IDX,
+ FE_STALL_IDX,
+ BE_STALL_IDX,
+ NUM_COMMON_EVS
+};
+
+enum grp_ev_idx {
+ MISS_IDX,
+ WB_IDX,
+ ACC_IDX,
+ NUM_GRP_EVS
+};
+
+#define EV_CPU_CYCLES 0
+#define EV_INST_RETIRED 2
+#define EV_L2_D_RFILL 5
+
+struct ev_map_msg {
+ u32 num_evs;
+ u32 hw_type;
+ u32 cid[NUM_COMMON_EVS];
+};
+
+struct cpufreq_memfreq_map {
+ unsigned int cpufreq_mhz;
+ unsigned int memfreq_khz;
+};
+
+struct scmi_monitor_info {
+ struct cpufreq_memfreq_map *freq_map;
+ char mon_name[MAX_NAME_LEN];
+ u32 mon_idx;
+ u32 mon_type;
+ u32 ipm_ceil;
+ u32 mask;
+ u32 freq_map_len;
+};
+
+struct scmi_memory_info {
+ struct scmi_monitor_info *monitor[MAX_MONITOR_CNT];
+ u32 hw_type;
+ int monitor_cnt;
+ u32 min_freq;
+ u32 max_freq;
+};
+
+struct scmi_memlat_info {
+ struct scmi_protocol_handle *ph;
+ const struct qcom_scmi_vendor_ops *ops;
+ struct scmi_memory_info *memory[MAX_MEMORY_TYPES];
+ int memory_cnt;
+};
+
+static int get_mask(struct device_node *np, u32 *mask)
+{
+ struct device_node *dev_phandle;
+ struct device *cpu_dev;
+ int cpu, i = 0;
+ int ret = -ENODEV;
+
+ dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
+ while (dev_phandle) {
+ for_each_possible_cpu(cpu) {
+ cpu_dev = get_cpu_device(cpu);
+ if (cpu_dev && cpu_dev->of_node == dev_phandle) {
+ *mask |= BIT(cpu);
+ ret = 0;
+ break;
+ }
+ }
+ dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
+ }
+
+ return ret;
+}
+
+static struct cpufreq_memfreq_map *init_cpufreq_memfreq_map(struct device *dev,
+ struct device_node *of_node,
+ u32 *cnt)
+{
+ int len, nf, i, j;
+ u32 data;
+ struct cpufreq_memfreq_map *tbl;
+ int ret;
+
+ if (!of_find_property(of_node, "qcom,cpufreq-memfreq-tbl", &len))
+ return NULL;
+ len /= sizeof(data);
+
+ if (len % 2 || len == 0)
+ return NULL;
+ nf = len / 2;
+
+ tbl = devm_kzalloc(dev, (nf + 1) * sizeof(struct cpufreq_memfreq_map),
+ GFP_KERNEL);
+ if (!tbl)
+ return NULL;
+
+ for (i = 0, j = 0; i < nf; i++, j += 2) {
+ ret = of_property_read_u32_index(of_node, "qcom,cpufreq-memfreq-tbl",
+ j, &data);
+ if (ret < 0)
+ return NULL;
+ tbl[i].cpufreq_mhz = data / 1000;
+
+ ret = of_property_read_u32_index(of_node, "qcom,cpufreq-memfreq-tbl",
+ j + 1, &data);
+ if (ret < 0)
+ return NULL;
+
+ tbl[i].memfreq_khz = data;
+ pr_debug("Entry%d CPU:%u, Mem:%u\n", i, tbl[i].cpufreq_mhz,
+ tbl[i].memfreq_khz);
+ }
+ *cnt = nf;
+ tbl[i].cpufreq_mhz = 0;
+
+ return tbl;
+}
+
+static int process_scmi_memlat_of_node(struct scmi_device *sdev, struct scmi_memlat_info *info)
+{
+ struct device_node *memlat_np, *memory_np, *monitor_np;
+ struct scmi_memory_info *memory;
+ struct scmi_monitor_info *monitor;
+ int ret = 0, i = 0, j;
+ u32 memfreq[2];
+
+ of_node_get(sdev->handle->dev->of_node);
+ memlat_np = of_find_node_by_name(sdev->handle->dev->of_node, "memlat");
+
+ info->memory_cnt = of_get_child_count(memlat_np);
+ if (info->memory_cnt <= 0)
+ pr_err("No memory nodes present\n");
+
+ for_each_child_of_node(memlat_np, memory_np) {
+ memory = devm_kzalloc(&sdev->dev, sizeof(*memory), GFP_KERNEL);
+ if (!memory) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ ret = of_property_read_u32(memory_np, "reg", &memory->hw_type);
+ if (ret) {
+ pr_err("Failed to read memory type\n");
+ goto err;
+ }
+
+ memory->monitor_cnt = of_get_child_count(memory_np);
+ if (memory->monitor_cnt <= 0) {
+ pr_err("No monitor nodes present\n");
+ ret = -EINVAL;
+ goto err;
+ }
+
+ ret = of_property_read_u32_array(memory_np, "freq-table-khz", memfreq, 2);
+ if (ret && (ret != -EINVAL)) {
+ pr_err("Failed to read min/max freq %d\n", ret);
+ goto err;
+ }
+
+ memory->min_freq = memfreq[0];
+ memory->max_freq = memfreq[1];
+ info->memory[i] = memory;
+ j = 0;
+ i++;
+
+ for_each_child_of_node(memory_np, monitor_np) {
+ monitor = devm_kzalloc(&sdev->dev, sizeof(*monitor), GFP_KERNEL);
+ if (!monitor) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ monitor->mon_type = (of_property_read_bool(monitor_np, "qcom,compute-mon")) ? 1 : 0;
+ monitor->ipm_ceil = (of_property_read_bool(monitor_np, "qcom,compute-mon")) ? 0 : 20000000;
+
+ if (get_mask(monitor_np, &monitor->mask)) {
+ pr_err("Failed to populate cpu mask %d\n", ret);
+ goto err;
+ }
+
+ monitor->freq_map = init_cpufreq_memfreq_map(&sdev->dev, monitor_np,
+ &monitor->freq_map_len);
+ snprintf(monitor->mon_name, MAX_NAME_LEN, "monitor-%d", j);
+ monitor->mon_idx = j;
+
+ memory->monitor[j] = monitor;
+ j++;
+ }
+ }
+
+ return 0;
+
+err:
+ of_node_put(memlat_np);
+
+ return ret;
+}
+
+static int configure_cpucp_common_events(struct scmi_memlat_info *info)
+{
+ const struct qcom_scmi_vendor_ops *ops = info->ops;
+ u8 ev_map[NUM_COMMON_EVS];
+ struct ev_map_msg msg;
+ int ret;
+
+ memset(ev_map, 0xFF, NUM_COMMON_EVS);
+
+ msg.num_evs = NUM_COMMON_EVS;
+ msg.hw_type = INVALID_IDX;
+ msg.cid[INST_IDX] = EV_INST_RETIRED;
+ msg.cid[CYC_IDX] = EV_CPU_CYCLES;
+ msg.cid[FE_STALL_IDX] = INVALID_IDX;
+ msg.cid[BE_STALL_IDX] = INVALID_IDX;
+
+ ret = ops->set_param(info->ph, &msg, MEMLAT_ALGO_STR, MEMLAT_SET_COMMON_EV_MAP,
+ sizeof(msg));
+ return ret;
+}
+
+static int configure_cpucp_grp(struct scmi_memlat_info *info, int memory_index)
+{
+ const struct qcom_scmi_vendor_ops *ops = info->ops;
+ struct scmi_memory_info *memory = info->memory[memory_index];
+ struct ev_map_msg ev_msg;
+ u8 ev_map[NUM_GRP_EVS];
+ struct node_msg msg;
+ int ret;
+
+ msg.cpumask = 0;
+ msg.hw_type = memory->hw_type;
+ msg.mon_type = 0;
+ msg.mon_idx = 0;
+ ret = ops->set_param(info->ph, &msg, MEMLAT_ALGO_STR, MEMLAT_SET_MEM_GROUP, sizeof(msg));
+ if (ret < 0) {
+ pr_err("Failed to configure mem type %d\n", memory->hw_type);
+ return ret;
+ }
+
+ memset(ev_map, 0xFF, NUM_GRP_EVS);
+ ev_msg.num_evs = NUM_GRP_EVS;
+ ev_msg.hw_type = memory->hw_type;
+ ev_msg.cid[MISS_IDX] = EV_L2_D_RFILL;
+ ev_msg.cid[WB_IDX] = INVALID_IDX;
+ ev_msg.cid[ACC_IDX] = INVALID_IDX;
+ ret = ops->set_param(info->ph, &ev_msg, MEMLAT_ALGO_STR, MEMLAT_SET_GRP_EV_MAP,
+ sizeof(ev_msg));
+ if (ret < 0) {
+ pr_err("Failed to configure event map for mem type %d\n", memory->hw_type);
+ return ret;
+ }
+
+ return ret;
+}
+
+static int configure_cpucp_mon(struct scmi_memlat_info *info, int memory_index, int monitor_index)
+{
+ const struct qcom_scmi_vendor_ops *ops = info->ops;
+ struct scmi_memory_info *memory = info->memory[memory_index];
+ struct scmi_monitor_info *monitor = memory->monitor[monitor_index];
+ struct scalar_param_msg scalar_msg;
+ struct map_param_msg map_msg;
+ struct node_msg msg;
+ int ret;
+ int i;
+
+ msg.cpumask = monitor->mask;
+ msg.hw_type = memory->hw_type;
+ msg.mon_type = monitor->mon_type;
+ msg.mon_idx = monitor->mon_idx;
+ strscpy(msg.mon_name, monitor->mon_name, sizeof(msg.mon_name));
+ ret = ops->set_param(info->ph, &msg, MEMLAT_ALGO_STR, MEMLAT_SET_MONITOR, sizeof(msg));
+ if (ret < 0) {
+ pr_err("Failed to configure monitor %s\n", monitor->mon_name);
+ return ret;
+ }
+
+ scalar_msg.hw_type = memory->hw_type;
+ scalar_msg.mon_idx = monitor->mon_idx;
+ scalar_msg.val = monitor->ipm_ceil;
+ ret = ops->set_param(info->ph, &scalar_msg, MEMLAT_ALGO_STR, MEMLAT_IPM_CEIL,
+ sizeof(scalar_msg));
+ if (ret < 0) {
+ pr_err("Failed to set ipm ceil for %s\n", monitor->mon_name);
+ return ret;
+ }
+
+ map_msg.hw_type = memory->hw_type;
+ map_msg.mon_idx = monitor->mon_idx;
+ map_msg.nr_rows = monitor->freq_map_len;
+ for (i = 0; i < monitor->freq_map_len; i++) {
+ map_msg.tbl[i].v1 = monitor->freq_map[i].cpufreq_mhz;
+ map_msg.tbl[i].v2 = monitor->freq_map[i].memfreq_khz / 1000;
+ }
+ ret = ops->set_param(info->ph, &map_msg, MEMLAT_ALGO_STR, MEMLAT_MON_FREQ_MAP,
+ sizeof(map_msg));
+ if (ret < 0) {
+ pr_err("Failed to configure freq_map for %s\n", monitor->mon_name);
+ return ret;
+ }
+
+ scalar_msg.hw_type = memory->hw_type;
+ scalar_msg.mon_idx = monitor->mon_idx;
+ scalar_msg.val = memory->min_freq;
+ ret = ops->set_param(info->ph, &scalar_msg, MEMLAT_ALGO_STR, MEMLAT_SET_MIN_FREQ,
+ sizeof(scalar_msg));
+ if (ret < 0) {
+ pr_err("Failed to set min_freq for %s\n", monitor->mon_name);
+ return ret;
+ }
+
+ scalar_msg.hw_type = memory->hw_type;
+ scalar_msg.mon_idx = monitor->mon_idx;
+ scalar_msg.val = memory->max_freq;
+ ret = ops->set_param(info->ph, &scalar_msg, MEMLAT_ALGO_STR, MEMLAT_SET_MAX_FREQ,
+ sizeof(scalar_msg));
+ if (ret < 0)
+ pr_err("Failed to set max_freq for %s\n", monitor->mon_name);
+
+ return ret;
+}
+
+static int cpucp_memlat_init(struct scmi_device *sdev)
+{
+ const struct scmi_handle *handle = sdev->handle;
+ const struct qcom_scmi_vendor_ops *ops;
+ struct scmi_protocol_handle *ph;
+ struct scmi_memlat_info *info;
+ u32 cpucp_sample_ms = 8;
+ int ret, i, j;
+
+ if (!handle)
+ return -ENODEV;
+
+ ops = handle->devm_protocol_get(sdev, QCOM_SCMI_VENDOR_PROTOCOL, &ph);
+ if (IS_ERR(ops))
+ return PTR_ERR(ops);
+
+ info = devm_kzalloc(&sdev->dev, sizeof(*info), GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
+
+ ret = process_scmi_memlat_of_node(sdev, info);
+ if (ret)
+ pr_err("Failed to configure common events: %d\n", ret);
+
+ info->ph = ph;
+ info->ops = ops;
+
+ ret = configure_cpucp_common_events(info);
+ if (ret < 0)
+ pr_err("Failed to configure common events: %d\n", ret);
+
+ for (i = 0; i < info->memory_cnt; i++) {
+ ret = configure_cpucp_grp(info, i);
+ if (ret < 0)
+ pr_err("Failed to configure mem group: %d\n", ret);
+
+ for (j = 0; j < info->memory[i]->monitor_cnt; j++) {
+ /* Configure per monitor parameters */
+ ret = configure_cpucp_mon(info, i, j);
+ if (ret < 0)
+ pr_err("Failed to configure monitor: %d\n", ret);
+ }
+ }
+
+ ret = ops->set_param(ph, &cpucp_sample_ms, MEMLAT_ALGO_STR, MEMLAT_SAMPLE_MS,
+ sizeof(cpucp_sample_ms));
+ if (ret < 0)
+ pr_err("Failed to set cpucp sample_ms ret = %d\n", ret);
+
+ /* Start sampling and voting timer */
+ ret = ops->start_activity(ph, NULL, MEMLAT_ALGO_STR, MEMLAT_START_TIMER, 0);
+ if (ret < 0)
+ pr_err("Error in starting the mem group timer %d\n", ret);
+
+ dev_set_drvdata(&sdev->dev, info);
+
+ return ret;
+}
+
+static int scmi_client_probe(struct scmi_device *sdev)
+{
+ cpucp_memlat_init(sdev);
+
+ return 0;
+}
+
+static const struct scmi_device_id scmi_id_table[] = {
+ { .protocol_id = QCOM_SCMI_VENDOR_PROTOCOL, .name = "qcom_scmi_vendor_protocol" },
+ { },
+};
+MODULE_DEVICE_TABLE(scmi, scmi_id_table);
+
+static struct scmi_driver qcom_scmi_client_drv = {
+ .name = "qcom-scmi-driver",
+ .probe = scmi_client_probe,
+ .id_table = scmi_id_table,
+};
+module_scmi_driver(qcom_scmi_client_drv);
+
+MODULE_DESCRIPTION("QTI SCMI client driver");
+MODULE_LICENSE("GPL");
--
2.34.1
Add the cpucp mailbox and sram nodes required by SCMI perf protocol
on X1E80100 SoCs.
Signed-off-by: Sibi Sankar <[email protected]>
---
arch/arm64/boot/dts/qcom/x1e80100.dtsi | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
index 6f75fc342ceb..afdbd27f8346 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
@@ -3309,6 +3309,13 @@ gic_its: msi-controller@17040000 {
};
};
+ cpucp_mbox: mailbox@17430000 {
+ compatible = "qcom,x1e80100-cpucp-mbox", "qcom,cpucp-mbox";
+ reg = <0 0x17430000 0 0x10000>, <0 0x18830000 0 0x300>;
+ interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>;
+ #mbox-cells = <1>;
+ };
+
apps_rsc: rsc@17500000 {
compatible = "qcom,rpmh-rsc";
reg = <0 0x17500000 0 0x10000>,
@@ -3492,6 +3499,25 @@ frame@1780d000 {
};
};
+ sram: sram@18b4e000 {
+ compatible = "mmio-sram";
+ reg = <0x0 0x18b4e000 0x0 0x400>;
+ ranges = <0x0 0x0 0x18b4e000 0x400>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ cpu_scp_lpri0: scmi-shmem@0 {
+ compatible = "arm,scmi-shmem";
+ reg = <0x0 0x200>;
+ };
+
+ cpu_scp_lpri1: scmi-shmem@200 {
+ compatible = "arm,scmi-shmem";
+ reg = <0x200 0x200>;
+ };
+ };
+
system-cache-controller@25000000 {
compatible = "qcom,x1e80100-llcc";
reg = <0 0x25000000 0 0x200000>,
--
2.34.1
Enable cpufreq on X1E80100 SoCs through the SCMI perf protocol node.
Signed-off-by: Sibi Sankar <[email protected]>
---
arch/arm64/boot/dts/qcom/x1e80100.dtsi | 27 ++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
index afdbd27f8346..6856a206f7fc 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
@@ -62,6 +62,7 @@ CPU0: cpu@0 {
compatible = "qcom,oryon";
reg = <0x0 0x0>;
enable-method = "psci";
+ clocks = <&scmi_dvfs 0>;
next-level-cache = <&L2_0>;
power-domains = <&CPU_PD0>;
power-domain-names = "psci";
@@ -79,6 +80,7 @@ CPU1: cpu@100 {
compatible = "qcom,oryon";
reg = <0x0 0x100>;
enable-method = "psci";
+ clocks = <&scmi_dvfs 0>;
next-level-cache = <&L2_0>;
power-domains = <&CPU_PD1>;
power-domain-names = "psci";
@@ -90,6 +92,7 @@ CPU2: cpu@200 {
compatible = "qcom,oryon";
reg = <0x0 0x200>;
enable-method = "psci";
+ clocks = <&scmi_dvfs 0>;
next-level-cache = <&L2_0>;
power-domains = <&CPU_PD2>;
power-domain-names = "psci";
@@ -101,6 +104,7 @@ CPU3: cpu@300 {
compatible = "qcom,oryon";
reg = <0x0 0x300>;
enable-method = "psci";
+ clocks = <&scmi_dvfs 0>;
next-level-cache = <&L2_0>;
power-domains = <&CPU_PD3>;
power-domain-names = "psci";
@@ -112,6 +116,7 @@ CPU4: cpu@10000 {
compatible = "qcom,oryon";
reg = <0x0 0x10000>;
enable-method = "psci";
+ clocks = <&scmi_dvfs 1>;
next-level-cache = <&L2_1>;
power-domains = <&CPU_PD4>;
power-domain-names = "psci";
@@ -129,6 +134,7 @@ CPU5: cpu@10100 {
compatible = "qcom,oryon";
reg = <0x0 0x10100>;
enable-method = "psci";
+ clocks = <&scmi_dvfs 1>;
next-level-cache = <&L2_1>;
power-domains = <&CPU_PD5>;
power-domain-names = "psci";
@@ -140,6 +146,7 @@ CPU6: cpu@10200 {
compatible = "qcom,oryon";
reg = <0x0 0x10200>;
enable-method = "psci";
+ clocks = <&scmi_dvfs 1>;
next-level-cache = <&L2_1>;
power-domains = <&CPU_PD6>;
power-domain-names = "psci";
@@ -151,6 +158,7 @@ CPU7: cpu@10300 {
compatible = "qcom,oryon";
reg = <0x0 0x10300>;
enable-method = "psci";
+ clocks = <&scmi_dvfs 1>;
next-level-cache = <&L2_1>;
power-domains = <&CPU_PD7>;
power-domain-names = "psci";
@@ -162,6 +170,7 @@ CPU8: cpu@20000 {
compatible = "qcom,oryon";
reg = <0x0 0x20000>;
enable-method = "psci";
+ clocks = <&scmi_dvfs 2>;
next-level-cache = <&L2_2>;
power-domains = <&CPU_PD8>;
power-domain-names = "psci";
@@ -179,6 +188,7 @@ CPU9: cpu@20100 {
compatible = "qcom,oryon";
reg = <0x0 0x20100>;
enable-method = "psci";
+ clocks = <&scmi_dvfs 2>;
next-level-cache = <&L2_2>;
power-domains = <&CPU_PD9>;
power-domain-names = "psci";
@@ -190,6 +200,7 @@ CPU10: cpu@20200 {
compatible = "qcom,oryon";
reg = <0x0 0x20200>;
enable-method = "psci";
+ clocks = <&scmi_dvfs 2>;
next-level-cache = <&L2_2>;
power-domains = <&CPU_PD10>;
power-domain-names = "psci";
@@ -201,6 +212,7 @@ CPU11: cpu@20300 {
compatible = "qcom,oryon";
reg = <0x0 0x20300>;
enable-method = "psci";
+ clocks = <&scmi_dvfs 2>;
next-level-cache = <&L2_2>;
power-domains = <&CPU_PD11>;
power-domain-names = "psci";
@@ -303,6 +315,21 @@ scm: scm {
interconnects = <&aggre2_noc MASTER_CRYPTO QCOM_ICC_TAG_ALWAYS
&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>;
};
+
+ scmi {
+ compatible = "arm,scmi";
+ mboxes = <&cpucp_mbox 0>, <&cpucp_mbox 2>;
+ mbox-names = "tx", "rx";
+ shmem = <&cpu_scp_lpri0>, <&cpu_scp_lpri1>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ scmi_dvfs: protocol@13 {
+ reg = <0x13>;
+ #clock-cells = <1>;
+ };
+ };
};
clk_virt: interconnect-0 {
--
2.34.1
Enable LLCC/DDR dvfs through the Qualcomm's SCMI vendor protocol.
Signed-off-by: Sibi Sankar <[email protected]>
---
arch/arm64/boot/dts/qcom/x1e80100.dtsi | 48 ++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
index 6856a206f7fc..3dc6f32fbb4c 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
@@ -329,6 +329,54 @@ scmi_dvfs: protocol@13 {
reg = <0x13>;
#clock-cells = <1>;
};
+
+ scmi_vendor: protocol@80 {
+ reg = <0x80>;
+
+ memlat {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ memory@0 {
+ reg = <0x0>; /* Memory Type DDR */
+ freq-table-khz = <200000 4224000>;
+
+ monitor-0 {
+ qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>;
+ qcom,cpufreq-memfreq-tbl = < 999000 547000 >,
+ < 1440000 768000 >,
+ < 1671000 1555000 >,
+ < 2189000 2092000 >,
+ < 2156000 3187000 >,
+ < 3860000 4224000 >;
+ };
+
+ monitor-1 {
+ qcom,compute-mon;
+ qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>;
+ qcom,cpufreq-memfreq-tbl = < 1440000 200000 >,
+ < 2189000 768000 >,
+ < 2156000 1555000 >,
+ < 3860000 2092000 >;
+ };
+ };
+
+ memory@1 {
+ reg = <0x1>; /* Memory Type LLCC */
+ freq-table-khz = <300000 1067000>;
+
+ monitor-0 {
+ qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>;
+ qcom,cpufreq-memfreq-tbl = < 999000 300000 >,
+ < 1440000 466000 >,
+ < 1671000 600000 >,
+ < 2189000 806000 >,
+ < 2156000 933000 >,
+ < 3860000 1066000 >;
+ };
+ };
+ };
+ };
};
};
--
2.34.1
On Wed, 17 Jan 2024 at 19:36, Sibi Sankar <[email protected]> wrote:
>
> Add support for CPUSS Control Processor (CPUCP) mailbox controller,
> this driver enables communication between AP and CPUCP by acting as
> a doorbell between them.
>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
> drivers/mailbox/Kconfig | 8 +
> drivers/mailbox/Makefile | 2 +
> drivers/mailbox/qcom-cpucp-mbox.c | 265 ++++++++++++++++++++++++++++++
> 3 files changed, 275 insertions(+)
> create mode 100644 drivers/mailbox/qcom-cpucp-mbox.c
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 42940108a187..23741a6f054e 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -273,6 +273,14 @@ config SPRD_MBOX
> to send message between application processors and MCU. Say Y here if
> you want to build the Spreatrum mailbox controller driver.
>
> +config QCOM_CPUCP_MBOX
> + tristate "Qualcomm Technologies, Inc. CPUCP mailbox driver"
> + depends on ARCH_QCOM || COMPILE_TEST
> + help
> + Qualcomm Technologies, Inc. CPUSS Control Processor (CPUCP) mailbox
> + controller driver enables communication between AP and CPUCP. Say
> + Y here if you want to build this driver.
> +
> config QCOM_IPCC
> tristate "Qualcomm Technologies, Inc. IPCC driver"
> depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index 18793e6caa2f..53b512800bde 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -59,4 +59,6 @@ obj-$(CONFIG_SUN6I_MSGBOX) += sun6i-msgbox.o
>
> obj-$(CONFIG_SPRD_MBOX) += sprd-mailbox.o
>
> +obj-$(CONFIG_QCOM_CPUCP_MBOX) += qcom-cpucp-mbox.o
> +
> obj-$(CONFIG_QCOM_IPCC) += qcom-ipcc.o
> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
> new file mode 100644
> index 000000000000..22ea6c802286
> --- /dev/null
> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
> @@ -0,0 +1,265 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/platform_device.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +
> +#define APSS_CPUCP_IPC_CHAN_SUPPORTED 3
> +#define APSS_CPUCP_MBOX_CMD_OFF 0x4
> +
> +/* Tx Registers */
> +#define APSS_CPUCP_TX_MBOX_IDR 0
> +#define APSS_CPUCP_TX_MBOX_CMD 0x100
> +
> +/* Rx Registers */
> +#define APSS_CPUCP_RX_MBOX_IDR 0
> +#define APSS_CPUCP_RX_MBOX_CMD 0x100
> +#define APSS_CPUCP_RX_MBOX_MAP 0x4000
> +#define APSS_CPUCP_RX_MBOX_STAT 0x4400
> +#define APSS_CPUCP_RX_MBOX_CLEAR 0x4800
> +#define APSS_CPUCP_RX_MBOX_EN 0x4C00
> +#define APSS_CPUCP_RX_MBOX_CMD_MASK 0xFFFFFFFFFFFFFFFF
> +
> +/**
> + * struct qcom_cpucp_mbox - Holder for the mailbox driver
> + * @chans: The mailbox channel
> + * @mbox: The mailbox controller
> + * @tx_base: Base address of the CPUCP tx registers
> + * @rx_base: Base address of the CPUCP rx registers
> + * @dev: Device associated with this instance
> + * @irq: CPUCP to AP irq
> + */
> +struct qcom_cpucp_mbox {
> + struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
> + struct mbox_controller mbox;
> + void __iomem *tx_base;
> + void __iomem *rx_base;
> + struct device *dev;
> + int irq;
> + int num_chan;
> +};
> +
> +static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
> +{
> + struct qcom_cpucp_mbox *cpucp = data;
> + u64 status;
> + u32 val;
> + int i;
> +
> + status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
> +
> + for (i = 0; i < cpucp->num_chan; i++) {
> + val = 0;
> + if (status & ((u64)1 << i)) {
> + val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD + (i * 8) + APSS_CPUCP_MBOX_CMD_OFF);
> + if (!IS_ERR(cpucp->chans[i].con_priv))
> + mbox_chan_received_data(&cpucp->chans[i], &val);
Why do you need a check? Can mailbox cope with unallocated channel instead?
> + writeq(status, cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
> + }
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
> +{
> + struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
> + unsigned long chan_id = (unsigned long)chan->con_priv;
> + u64 val;
> +
> + val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
> + val |= ((u64)1 << chan_id);
BIT()
> + writeq(val, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
> +
> + return 0;
> +}
> +
> +static void qcom_cpucp_mbox_shutdown(struct mbox_chan *chan)
> +{
> + struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
> + unsigned long chan_id = (unsigned long)chan->con_priv;
> + u64 val;
> +
> + val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
> + val &= ~((u64)1 << chan_id);
BIT()
> + writeq(val, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
> +
> + chan->con_priv = ERR_PTR(-EINVAL);
> +}
> +
> +static int qcom_cpucp_mbox_send_data(struct mbox_chan *chan, void *data)
> +{
> + struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
> + unsigned long chan_id = (unsigned long)chan->con_priv;
> + u32 val = (unsigned long)data;
Please don't pass an integer as a pointer value.
> +
> + writel(val, cpucp->tx_base + APSS_CPUCP_TX_MBOX_CMD + (chan_id * 8) + APSS_CPUCP_MBOX_CMD_OFF);
> +
> + return 0;
> +}
> +
> +static struct mbox_chan *qcom_cpucp_mbox_xlate(struct mbox_controller *mbox,
> + const struct of_phandle_args *sp)
> +{
> + unsigned long ind = sp->args[0];
> +
> + if (sp->args_count != 1)
> + return ERR_PTR(-EINVAL);
> +
> + if (ind >= mbox->num_chans)
> + return ERR_PTR(-EINVAL);
> +
> + if (!IS_ERR(mbox->chans[ind].con_priv))
> + return ERR_PTR(-EBUSY);
> +
> + mbox->chans[ind].con_priv = (void *)ind;
This seems to be static enough. Can you set it in
qcom_cpucp_setup_mbox() instead?
Then you can use of_mbox_index_xlate() here.
> +
> + return &mbox->chans[ind];
> +}
> +
> +static const struct mbox_chan_ops qcom_cpucp_mbox_chan_ops = {
> + .startup = qcom_cpucp_mbox_startup,
> + .send_data = qcom_cpucp_mbox_send_data,
> + .shutdown = qcom_cpucp_mbox_shutdown
> +};
> +
> +static int qcom_cpucp_setup_mbox(struct qcom_cpucp_mbox *cpucp)
> +{
> + struct device *dev = cpucp->dev;
> + struct mbox_controller *mbox;
> + unsigned long i;
> +
> + /* Initialize channel identifiers */
> + for (i = 0; i < ARRAY_SIZE(cpucp->chans); i++)
> + cpucp->chans[i].con_priv = ERR_PTR(-EINVAL);
> +
> + mbox = &cpucp->mbox;
> + mbox->dev = dev;
> + mbox->num_chans = cpucp->num_chan;
> + mbox->chans = cpucp->chans;
> + mbox->ops = &qcom_cpucp_mbox_chan_ops;
> + mbox->of_xlate = qcom_cpucp_mbox_xlate;
> + mbox->txdone_irq = false;
> + mbox->txdone_poll = false;
> +
> + return mbox_controller_register(mbox);
> +}
> +
> +static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
> +{
> + struct qcom_cpucp_mbox *cpucp;
> + struct resource *res;
> + int ret;
> +
> + cpucp = devm_kzalloc(&pdev->dev, sizeof(*cpucp), GFP_KERNEL);
> + if (!cpucp)
> + return -ENOMEM;
> +
> + cpucp->dev = &pdev->dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "Failed to get the cpucp rx base address\n");
> + return -ENODEV;
> + }
> +
> + cpucp->rx_base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
devm_of_iomap() here and below.
> + if (!cpucp->rx_base) {
> + dev_err(&pdev->dev, "Failed to ioremap cpucp tx base\n");
> + return -ENOMEM;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!res) {
> + dev_err(&pdev->dev, "Failed to get the cpucp tx base address\n");
> + return -ENODEV;
> + }
> +
> + cpucp->tx_base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> + if (!cpucp->tx_base) {
> + dev_err(&pdev->dev, "Failed to ioremap cpucp tx base\n");
> + return -ENOMEM;
> + }
> +
> + writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
> + writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
> + writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
> +
> + cpucp->irq = platform_get_irq(pdev, 0);
> + if (cpucp->irq < 0) {
> + dev_err(&pdev->dev, "Failed to get the IRQ\n");
> + return cpucp->irq;
> + }
> +
> + ret = devm_request_irq(&pdev->dev, cpucp->irq, qcom_cpucp_mbox_irq_fn,
> + IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret);
> + return ret;
> + }
> +
> + writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
> +
> + cpucp->num_chan = APSS_CPUCP_IPC_CHAN_SUPPORTED;
> + ret = qcom_cpucp_setup_mbox(cpucp);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to create mailbox\n");
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, cpucp);
> +
> + return 0;
> +}
> +
> +static int qcom_cpucp_mbox_remove(struct platform_device *pdev)
> +{
> + struct qcom_cpucp_mbox *cpucp = platform_get_drvdata(pdev);
> +
> + mbox_controller_unregister(&cpucp->mbox);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
> + { .compatible = "qcom,cpucp-mbox"},
Is there a guarantee that there will be no SoC-specifcs in this driver
in future?
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
> +
> +static struct platform_driver qcom_cpucp_mbox_driver = {
> + .probe = qcom_cpucp_mbox_probe,
> + .remove = qcom_cpucp_mbox_remove,
> + .driver = {
> + .name = "qcom_cpucp_mbox",
> + .of_match_table = qcom_cpucp_mbox_of_match,
> + .suppress_bind_attrs = true,
> + },
> +};
> +
> +static int __init qcom_cpucp_mbox_init(void)
> +{
> + int ret;
> +
> + ret = platform_driver_register(&qcom_cpucp_mbox_driver);
> + if (ret)
> + pr_err("%s: qcom_cpucp_mbox register failed %d\n", __func__, ret);
> +
> + return ret;
> +}
> +module_init(qcom_cpucp_mbox_init);
module_platform_init()
> +
> +static __exit void qcom_cpucp_mbox_exit(void)
> +{
> + platform_driver_unregister(&qcom_cpucp_mbox_driver);
> +}
> +module_exit(qcom_cpucp_mbox_exit);
> +
> +MODULE_DESCRIPTION("QTI CPUCP MBOX Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
>
--
With best wishes
Dmitry
On Wed, 17 Jan 2024 at 19:36, Sibi Sankar <[email protected]> wrote:
>
> From: Shivnandan Kumar <[email protected]>
>
> 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 <[email protected]>
> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
> Co-developed-by: Amir Vajid <[email protected]>
> Signed-off-by: Amir Vajid <[email protected]>
> Co-developed-by: Sibi Sankar <[email protected]>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
> drivers/firmware/arm_scmi/Kconfig | 11 ++
> drivers/firmware/arm_scmi/Makefile | 1 +
> drivers/firmware/arm_scmi/qcom_scmi_vendor.c | 160 +++++++++++++++++++
> include/linux/qcom_scmi_vendor.h | 36 +++++
> 4 files changed, 208 insertions(+)
> create mode 100644 drivers/firmware/arm_scmi/qcom_scmi_vendor.c
> create mode 100644 include/linux/qcom_scmi_vendor.h
>
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index aa5842be19b2..86b5d6c18ec4 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -180,4 +180,15 @@ config ARM_SCMI_POWER_CONTROL
> called scmi_power_control. Note this may needed early in boot to catch
> early shutdown/reboot SCMI requests.
>
> +config QCOM_SCMI_VENDOR_PROTOCOL
> + tristate "Qualcomm Technologies, Inc. Qcom SCMI vendor Protocol"
> + depends on ARM || ARM64 || COMPILE_TEST
> + depends on ARM_SCMI_PROTOCOL
> + help
> + The SCMI QCOM vendor protocol provides interface to communicate with SCMI
> + controller and enable vendor specific features like bus scaling.
> +
> + This driver defines the commands or message ID's used for this
> + communication and also exposes the ops used by the clients.
> +
> endmenu
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index a7bc4796519c..eaeb788b93c6 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o
>
> obj-$(CONFIG_ARM_SCMI_POWER_CONTROL) += scmi_power_control.o
> +obj-$(CONFIG_QCOM_SCMI_VENDOR_PROTOCOL) += qcom_scmi_vendor.o
>
> ifeq ($(CONFIG_THUMB2_KERNEL)$(CONFIG_CC_IS_CLANG),yy)
> # The use of R7 in the SMCCC conflicts with the compiler's use of R7 as a frame
> diff --git a/drivers/firmware/arm_scmi/qcom_scmi_vendor.c b/drivers/firmware/arm_scmi/qcom_scmi_vendor.c
> new file mode 100644
> index 000000000000..878b99f0d1ef
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/qcom_scmi_vendor.c
> @@ -0,0 +1,160 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/qcom_scmi_vendor.h>
> +
> +#include "common.h"
> +
> +#define EXTENDED_MSG_ID 0
> +#define SCMI_MAX_TX_RX_SIZE 128
> +#define PROTOCOL_PAYLOAD_SIZE 16
> +#define SET_PARAM 0x10
> +#define GET_PARAM 0x11
> +#define START_ACTIVITY 0x12
> +#define STOP_ACTIVITY 0x13
> +
> +static int qcom_scmi_set_param(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
> + u32 param_id, size_t size)
> +{
> + int ret = -EINVAL;
> + struct scmi_xfer *t;
> + u32 *msg;
> +
> + if (!ph || !ph->xops)
> + return ret;
Drop init of ret, return -EINVAL directly here.
> +
> + ret = ph->xops->xfer_get_init(ph, SET_PARAM, size + PROTOCOL_PAYLOAD_SIZE,
> + SCMI_MAX_TX_RX_SIZE, &t);
> + if (ret)
> + return ret;
> +
> + msg = t->tx.buf;
> + *msg++ = cpu_to_le32(EXTENDED_MSG_ID);
> + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0));
> + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32);
> + *msg++ = cpu_to_le32(param_id);
First, this header ops looks like a generic code which can be extracted.
Second, using GENMASK here in the ops doesn't make any sense. The
values will be limited to u32 anyway.
> + memcpy(msg, buf, size);
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int qcom_scmi_get_param(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
> + u32 param_id, size_t tx_size, size_t rx_size)
> +{
> + int ret = -EINVAL;
> + struct scmi_xfer *t;
> + u32 *msg;
> +
> + if (!ph || !ph->xops || !buf)
> + return ret;
Drop init of ret, return -EINVAL directly here.
> +
> + ret = ph->xops->xfer_get_init(ph, GET_PARAM, tx_size + PROTOCOL_PAYLOAD_SIZE,
> + SCMI_MAX_TX_RX_SIZE, &t);
> + if (ret)
> + return ret;
> +
> + msg = t->tx.buf;
> + *msg++ = cpu_to_le32(EXTENDED_MSG_ID);
> + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0));
> + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32);
> + *msg++ = cpu_to_le32(param_id);
> + memcpy(msg, buf, tx_size);
> + ret = ph->xops->do_xfer(ph, t);
> + if (t->rx.len > rx_size) {
> + pr_err("SCMI received buffer size %zu is more than expected size %zu\n",
> + t->rx.len, rx_size);
> + return -EMSGSIZE;
> + }
> + memcpy(buf, t->rx.buf, t->rx.len);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int qcom_scmi_start_activity(const struct scmi_protocol_handle *ph,
> + void *buf, u64 algo_str, u32 param_id, size_t size)
> +{
> + int ret = -EINVAL;
> + struct scmi_xfer *t;
> + u32 *msg;
> +
> + if (!ph || !ph->xops)
> + return ret;
You can guess the comment here.
> +
> + ret = ph->xops->xfer_get_init(ph, START_ACTIVITY, size + PROTOCOL_PAYLOAD_SIZE,
> + SCMI_MAX_TX_RX_SIZE, &t);
> + if (ret)
> + return ret;
> +
> + msg = t->tx.buf;
> + *msg++ = cpu_to_le32(EXTENDED_MSG_ID);
> + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0));
> + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32);
> + *msg++ = cpu_to_le32(param_id);
> + memcpy(msg, buf, size);
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int qcom_scmi_stop_activity(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
> + u32 param_id, size_t size)
> +{
> + int ret = -EINVAL;
> + struct scmi_xfer *t;
> + u32 *msg;
> +
> + if (!ph || !ph->xops)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, STOP_ACTIVITY, size + PROTOCOL_PAYLOAD_SIZE,
> + SCMI_MAX_TX_RX_SIZE, &t);
> + if (ret)
> + return ret;
> +
> + msg = t->tx.buf;
> + *msg++ = cpu_to_le32(EXTENDED_MSG_ID);
> + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0));
> + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32);
> + *msg++ = cpu_to_le32(param_id);
> + memcpy(msg, buf, size);
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static struct qcom_scmi_vendor_ops qcom_proto_ops = {
> + .set_param = qcom_scmi_set_param,
> + .get_param = qcom_scmi_get_param,
> + .start_activity = qcom_scmi_start_activity,
> + .stop_activity = qcom_scmi_stop_activity,
> +};
> +
> +static int qcom_scmi_vendor_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> + u32 version;
> +
> + ph->xops->version_get(ph, &version);
> +
> + dev_info(ph->dev, "qcom scmi version %d.%d\n",
> + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> +
> + return 0;
> +}
> +
> +static const struct scmi_protocol qcom_scmi_vendor = {
> + .id = QCOM_SCMI_VENDOR_PROTOCOL,
> + .owner = THIS_MODULE,
> + .instance_init = &qcom_scmi_vendor_protocol_init,
> + .ops = &qcom_proto_ops,
> +};
> +module_scmi_protocol(qcom_scmi_vendor);
> +
> +MODULE_DESCRIPTION("QTI SCMI vendor protocol");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/qcom_scmi_vendor.h b/include/linux/qcom_scmi_vendor.h
> new file mode 100644
> index 000000000000..bde57bb18367
> --- /dev/null
> +++ b/include/linux/qcom_scmi_vendor.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * QTI SCMI vendor protocol's header
> + *
> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef _QCOM_SCMI_VENDOR_H
> +#define _QCOM_SCMI_VENDOR_H
> +
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/types.h>
> +
> +#define QCOM_SCMI_VENDOR_PROTOCOL 0x80
> +
> +struct scmi_protocol_handle;
> +extern struct scmi_device *get_qcom_scmi_device(void);
> +
> +/**
> + * struct qcom_scmi_vendor_ops - represents the various operations provided
> + * by qcom scmi vendor protocol
> + */
> +struct qcom_scmi_vendor_ops {
> + int (*set_param)(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
> + u32 param_id, size_t size);
> + int (*get_param)(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
> + u32 param_id, size_t tx_size, size_t rx_size);
> + int (*start_activity)(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
> + u32 param_id, size_t size);
> + int (*stop_activity)(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
> + u32 param_id, size_t size);
> +};
> +
> +#endif /* _QCOM_SCMI_VENDOR_H */
> +
> --
> 2.34.1
>
>
--
With best wishes
Dmitry
On 1/17/24 18:34, Sibi Sankar wrote:
> Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox
> controller.
>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
[...]
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> + mailbox@17430000 {
> + compatible = "qcom,x1e80100-cpucp-mbox", "qcom,cpucp-mbox";
> + reg = <0x17430000 0x10000>, <0x18830000 0x300>;
These reg spaces are quite far apart.. On 7280-8550, a similar
mailbox exists, although it's dubbed RIMPS-mbox instead. In
that case, I separated the mbox into tx (via
qcom-apcs-ipc-mailbox.c) and rx (with a simple driver). Still
haven't pushed or posted that anywhere, I'd need to access
another machine..
On (some of) these SoCs, one of the channels (rx[1], iirc?) clearly
bleeds into the CPUFREQ_HW/OSM register region, which gives an
impression of misrepresenting the hardware. X1E doesn't have a
node for cpufreq_hw defined, so I can't tell whether it's also the
case here.
Konrad
On 1/17/24 18:34, Sibi Sankar wrote:
> From: Shivnandan Kumar <[email protected]>
>
> 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 <[email protected]>
> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
> Co-developed-by: Amir Vajid <[email protected]>
> Signed-off-by: Amir Vajid <[email protected]>
> Co-developed-by: Sibi Sankar <[email protected]>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
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 <[email protected]>
>
> 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 <[email protected]>
> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
> Co-developed-by: Amir Vajid <[email protected]>
> Signed-off-by: Amir Vajid <[email protected]>
> Co-developed-by: Sibi Sankar <[email protected]>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
[...]
> +
> +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 1/17/24 18:34, Sibi Sankar wrote:
> From: Shivnandan Kumar <[email protected]>
>
> This patch introduces a client driver that interacts with the SCMI QCOM
> vendor protocol and passes on the required tuneables to start various
> features running on the SCMI controller.
>
> Signed-off-by: Shivnandan Kumar <[email protected]>
> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
> Co-developed-by: Amir Vajid <[email protected]>
> Signed-off-by: Amir Vajid <[email protected]>
> Co-developed-by: Sibi Sankar <[email protected]>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
[...]
> +
> +struct cpufreq_memfreq_map {
> + unsigned int cpufreq_mhz;
> + unsigned int memfreq_khz;
> +};
Weird use of tabs
[...]
> +static int get_mask(struct device_node *np, u32 *mask)
> +{
> + struct device_node *dev_phandle;
> + struct device *cpu_dev;
> + int cpu, i = 0;
> + int ret = -ENODEV;
Don't initialize ret here, return 0 instead of breaking and return
enodev otherwise.
> +
> + dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> + while (dev_phandle) {
> + for_each_possible_cpu(cpu) {
> + cpu_dev = get_cpu_device(cpu);
> + if (cpu_dev && cpu_dev->of_node == dev_phandle) {
> + *mask |= BIT(cpu);
> + ret = 0;
> + break;
> + }
> + }
of_cpu_node_to_id()
> + dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> + }
> +
> + return ret;
> +}
> +
> +static struct cpufreq_memfreq_map *init_cpufreq_memfreq_map(struct device *dev,
> + struct device_node *of_node,
> + u32 *cnt)
I really feel like this is trying to reinvent OPP..
if you structure your entries like so:
opp-0 {
opp-hz = /bits/ 64 <12341234 43214321>;
};
you'll be able to use all the fantastic APIs that have been
created over the years!
[...]
> + monitor->mon_type = (of_property_read_bool(monitor_np, "qcom,compute-mon")) ? 1 : 0;
> + monitor->ipm_ceil = (of_property_read_bool(monitor_np, "qcom,compute-mon")) ? 0 : 20000000;
What does it even mean for a monitor to be a compute mon?
There seem to be no dt-bindings for properties referenced in this
driver, neither in the series nor in the dependencies. This is
strictly required.
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 <[email protected]>
> >
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 <[email protected]>
> > Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
> > Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
> > Co-developed-by: Amir Vajid <[email protected]>
> > Signed-off-by: Amir Vajid <[email protected]>
> > Co-developed-by: Sibi Sankar <[email protected]>
> > Signed-off-by: Sibi Sankar <[email protected]>
> > ---
>
> 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 1/17/24 18:34, Sibi Sankar wrote:
> Enable LLCC/DDR dvfs through the Qualcomm's SCMI vendor protocol.
>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/x1e80100.dtsi | 48 ++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> index 6856a206f7fc..3dc6f32fbb4c 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> @@ -329,6 +329,54 @@ scmi_dvfs: protocol@13 {
> reg = <0x13>;
> #clock-cells = <1>;
> };
> +
> + scmi_vendor: protocol@80 {
> + reg = <0x80>;
> +
> + memlat {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + memory@0 {
> + reg = <0x0>; /* Memory Type DDR */
I'm not sure reg is the best property to (ab)use..
You could very well define a new one, like qcom,memory type,
then the subnodes could look like:
memory-0 {
qcom,memory-type = <QCOM_MEM_TYPE_DDR>;
[...]
};
> + freq-table-khz = <200000 4224000>;
> +
> + monitor-0 {
> + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>;
I fail to see the usefulness in checking which CPUs make use of
the same DRAM or LLC pool. If that's something that may not be
obvious in future designs like on dual-socket x86 servers,
I think it can be deferred until then and for now, AFAIU you
can just unconditionally assume all CPUs count.
> + qcom,cpufreq-memfreq-tbl = < 999000 547000 >,
> + < 1440000 768000 >,
> + < 1671000 1555000 >,
> + < 2189000 2092000 >,
> + < 2156000 3187000 >,
> + < 3860000 4224000 >;
I.. can't seem to think of a future where this doesn't explode.
When you release a different bin/SKU/fuse config of this SoC where
the CPU frequencies are different, this will likely also need to be
updated. We don't want that manual cruft in the devicetree.
Since both previously cpufreq-hw and now cpufreq-scmi generally
operate on levels that map to some frequencies in the firmware,
could it be bound to that instead?
Konrad
On Wed, 17 Jan 2024 at 19:36, Sibi Sankar <[email protected]> wrote:
>
> From: Shivnandan Kumar <[email protected]>
>
> This patch introduces a client driver that interacts with the SCMI QCOM
git grep This.patch Documentation/process/
> vendor protocol and passes on the required tuneables to start various
> features running on the SCMI controller.
Is there any word about the 'memlat'? No. Unless one reads into the
patch, one can not come up with the idea of what is being introduced.
>
> Signed-off-by: Shivnandan Kumar <[email protected]>
> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
> Co-developed-by: Amir Vajid <[email protected]>
> Signed-off-by: Amir Vajid <[email protected]>
> Co-developed-by: Sibi Sankar <[email protected]>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
> drivers/soc/qcom/Kconfig | 10 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/qcom_scmi_client.c | 486 ++++++++++++++++++++++++++++
Should it go to drivers/firmware/arm_scmi instead? Or maybe to drivers/devfreq?
> 3 files changed, 497 insertions(+)
> create mode 100644 drivers/soc/qcom/qcom_scmi_client.c
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index c6ca4de42586..1530558aebfb 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -264,6 +264,16 @@ config QCOM_ICC_BWMON
> the fixed bandwidth votes from cpufreq (CPU nodes) thus achieve high
> memory throughput even with lower CPU frequencies.
>
> +config QCOM_SCMI_CLIENT
> + tristate "Qualcomm Technologies Inc. SCMI client driver"
> + depends on QCOM_SCMI_VENDOR_PROTOCOL || COMPILE_TEST
> + default n
> + help
> + SCMI client driver registers for SCMI QCOM vendor protocol.
> +
> + This driver interacts with the vendor protocol and passes on the required
> + tuneables to start various features running on the SCMI controller.
> +
> config QCOM_INLINE_CRYPTO_ENGINE
> tristate
> select QCOM_SCM
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 05b3d54e8dc9..c2a51293c886 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -32,5 +32,6 @@ obj-$(CONFIG_QCOM_APR) += apr.o
> obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
> obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
> obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
> +obj-$(CONFIG_QCOM_SCMI_CLIENT) += qcom_scmi_client.o
> qcom_ice-objs += ice.o
> obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom_ice.o
> diff --git a/drivers/soc/qcom/qcom_scmi_client.c b/drivers/soc/qcom/qcom_scmi_client.c
> new file mode 100644
> index 000000000000..418aa7900496
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom_scmi_client.c
> @@ -0,0 +1,486 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/cpu.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/qcom_scmi_vendor.h>
> +#include <linux/scmi_protocol.h>
> +
> +#define MAX_MEMORY_TYPES 3
> +#define MEMLAT_ALGO_STR 0x74616C6D656D /* "memlat" */
> +#define INVALID_IDX 0xFF
> +#define MAX_NAME_LEN 20
> +#define MAX_MAP_ENTRIES 6
> +#define MAX_MONITOR_CNT 4
> +#define SCMI_VENDOR_MSG_START 3
> +#define SCMI_VENDOR_MSG_MODULE_START 16
> +
> +enum scmi_memlat_protocol_cmd {
> + MEMLAT_SET_LOG_LEVEL = SCMI_VENDOR_MSG_START,
> + MEMLAT_FLUSH_LOGBUF,
> + MEMLAT_SET_MEM_GROUP = SCMI_VENDOR_MSG_MODULE_START,
> + MEMLAT_SET_MONITOR,
> + MEMLAT_SET_COMMON_EV_MAP,
> + MEMLAT_SET_GRP_EV_MAP,
> + MEMLAT_ADAPTIVE_LOW_FREQ,
> + MEMLAT_ADAPTIVE_HIGH_FREQ,
> + MEMLAT_GET_ADAPTIVE_CUR_FREQ,
> + MEMLAT_IPM_CEIL,
> + MEMLAT_FE_STALL_FLOOR,
> + MEMLAT_BE_STALL_FLOOR,
> + MEMLAT_WB_PCT,
> + MEMLAT_IPM_FILTER,
> + MEMLAT_FREQ_SCALE_PCT,
> + MEMLAT_FREQ_SCALE_CEIL_MHZ,
> + MEMLAT_FREQ_SCALE_FLOOR_MHZ,
> + MEMLAT_SAMPLE_MS,
> + MEMLAT_MON_FREQ_MAP,
> + MEMLAT_SET_MIN_FREQ,
> + MEMLAT_SET_MAX_FREQ,
> + MEMLAT_GET_CUR_FREQ,
> + MEMLAT_START_TIMER,
> + MEMLAT_STOP_TIMER,
> + MEMLAT_GET_TIMESTAMP,
> + MEMLAT_MAX_MSG
> +};
> +
> +struct map_table {
> + u16 v1;
> + u16 v2;
> +};
> +
Any documentation for these structures? It won't bite you, but it will
help reviewers and other developers.
> +struct map_param_msg {
> + u32 hw_type;
> + u32 mon_idx;
> + u32 nr_rows;
> + struct map_table tbl[MAX_MAP_ENTRIES];
> +} __packed;
> +
> +struct node_msg {
> + u32 cpumask;
> + u32 hw_type;
> + u32 mon_type;
> + u32 mon_idx;
> + char mon_name[MAX_NAME_LEN];
> +};
> +
> +struct scalar_param_msg {
> + u32 hw_type;
> + u32 mon_idx;
> + u32 val;
> +};
> +
> +enum common_ev_idx {
> + INST_IDX,
> + CYC_IDX,
> + FE_STALL_IDX,
> + BE_STALL_IDX,
> + NUM_COMMON_EVS
> +};
> +
> +enum grp_ev_idx {
> + MISS_IDX,
> + WB_IDX,
> + ACC_IDX,
> + NUM_GRP_EVS
> +};
> +
> +#define EV_CPU_CYCLES 0
> +#define EV_INST_RETIRED 2
> +#define EV_L2_D_RFILL 5
> +
> +struct ev_map_msg {
> + u32 num_evs;
> + u32 hw_type;
> + u32 cid[NUM_COMMON_EVS];
> +};
> +
> +struct cpufreq_memfreq_map {
> + unsigned int cpufreq_mhz;
> + unsigned int memfreq_khz;
> +};
> +
> +struct scmi_monitor_info {
> + struct cpufreq_memfreq_map *freq_map;
> + char mon_name[MAX_NAME_LEN];
> + u32 mon_idx;
> + u32 mon_type;
> + u32 ipm_ceil;
> + u32 mask;
> + u32 freq_map_len;
> +};
> +
> +struct scmi_memory_info {
> + struct scmi_monitor_info *monitor[MAX_MONITOR_CNT];
> + u32 hw_type;
> + int monitor_cnt;
> + u32 min_freq;
> + u32 max_freq;
> +};
> +
> +struct scmi_memlat_info {
> + struct scmi_protocol_handle *ph;
> + const struct qcom_scmi_vendor_ops *ops;
> + struct scmi_memory_info *memory[MAX_MEMORY_TYPES];
> + int memory_cnt;
> +};
> +
> +static int get_mask(struct device_node *np, u32 *mask)
> +{
> + struct device_node *dev_phandle;
> + struct device *cpu_dev;
> + int cpu, i = 0;
> + int ret = -ENODEV;
> +
> + dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> + while (dev_phandle) {
> + for_each_possible_cpu(cpu) {
> + cpu_dev = get_cpu_device(cpu);
> + if (cpu_dev && cpu_dev->of_node == dev_phandle) {
> + *mask |= BIT(cpu);
> + ret = 0;
> + break;
> + }
> + }
There is of_cpu_node_to_id(). No need to reinvent it.
> + dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> + }
> +
> + return ret;
> +}
> +
> +static struct cpufreq_memfreq_map *init_cpufreq_memfreq_map(struct device *dev,
> + struct device_node *of_node,
> + u32 *cnt)
> +{
> + int len, nf, i, j;
> + u32 data;
> + struct cpufreq_memfreq_map *tbl;
> + int ret;
> +
> + if (!of_find_property(of_node, "qcom,cpufreq-memfreq-tbl", &len))
> + return NULL;
> + len /= sizeof(data);
> +
> + if (len % 2 || len == 0)
> + return NULL;
> + nf = len / 2;
> +
> + tbl = devm_kzalloc(dev, (nf + 1) * sizeof(struct cpufreq_memfreq_map),
> + GFP_KERNEL);
> + if (!tbl)
> + return NULL;
> +
> + for (i = 0, j = 0; i < nf; i++, j += 2) {
> + ret = of_property_read_u32_index(of_node, "qcom,cpufreq-memfreq-tbl",
> + j, &data);
> + if (ret < 0)
> + return NULL;
> + tbl[i].cpufreq_mhz = data / 1000;
> +
> + ret = of_property_read_u32_index(of_node, "qcom,cpufreq-memfreq-tbl",
> + j + 1, &data);
> + if (ret < 0)
> + return NULL;
> +
> + tbl[i].memfreq_khz = data;
> + pr_debug("Entry%d CPU:%u, Mem:%u\n", i, tbl[i].cpufreq_mhz,
> + tbl[i].memfreq_khz);
> + }
> + *cnt = nf;
> + tbl[i].cpufreq_mhz = 0;
This looks like a lame version of the OPP table.
> +
> + return tbl;
> +}
> +
> +static int process_scmi_memlat_of_node(struct scmi_device *sdev, struct scmi_memlat_info *info)
> +{
> + struct device_node *memlat_np, *memory_np, *monitor_np;
> + struct scmi_memory_info *memory;
> + struct scmi_monitor_info *monitor;
> + int ret = 0, i = 0, j;
> + u32 memfreq[2];
> +
> + of_node_get(sdev->handle->dev->of_node);
> + memlat_np = of_find_node_by_name(sdev->handle->dev->of_node, "memlat");
> +
> + info->memory_cnt = of_get_child_count(memlat_np);
> + if (info->memory_cnt <= 0)
> + pr_err("No memory nodes present\n");
> +
> + for_each_child_of_node(memlat_np, memory_np) {
> + memory = devm_kzalloc(&sdev->dev, sizeof(*memory), GFP_KERNEL);
> + if (!memory) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + ret = of_property_read_u32(memory_np, "reg", &memory->hw_type);
> + if (ret) {
> + pr_err("Failed to read memory type\n");
> + goto err;
> + }
> +
> + memory->monitor_cnt = of_get_child_count(memory_np);
> + if (memory->monitor_cnt <= 0) {
> + pr_err("No monitor nodes present\n");
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + ret = of_property_read_u32_array(memory_np, "freq-table-khz", memfreq, 2);
> + if (ret && (ret != -EINVAL)) {
> + pr_err("Failed to read min/max freq %d\n", ret);
> + goto err;
> + }
foo-min-frequency and foo-max-frequency please.
> +
> + memory->min_freq = memfreq[0];
> + memory->max_freq = memfreq[1];
> + info->memory[i] = memory;
> + j = 0;
> + i++;
> +
> + for_each_child_of_node(memory_np, monitor_np) {
> + monitor = devm_kzalloc(&sdev->dev, sizeof(*monitor), GFP_KERNEL);
> + if (!monitor) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + monitor->mon_type = (of_property_read_bool(monitor_np, "qcom,compute-mon")) ? 1 : 0;
> + monitor->ipm_ceil = (of_property_read_bool(monitor_np, "qcom,compute-mon")) ? 0 : 20000000;
Huh?
> +
> + if (get_mask(monitor_np, &monitor->mask)) {
> + pr_err("Failed to populate cpu mask %d\n", ret);
> + goto err;
> + }
> +
> + monitor->freq_map = init_cpufreq_memfreq_map(&sdev->dev, monitor_np,
> + &monitor->freq_map_len);
> + snprintf(monitor->mon_name, MAX_NAME_LEN, "monitor-%d", j);
> + monitor->mon_idx = j;
> +
> + memory->monitor[j] = monitor;
> + j++;
> + }
> + }
> +
> + return 0;
> +
> +err:
> + of_node_put(memlat_np);
> +
> + return ret;
> +}
> +
> +static int configure_cpucp_common_events(struct scmi_memlat_info *info)
> +{
> + const struct qcom_scmi_vendor_ops *ops = info->ops;
> + u8 ev_map[NUM_COMMON_EVS];
> + struct ev_map_msg msg;
> + int ret;
> +
> + memset(ev_map, 0xFF, NUM_COMMON_EVS);
> +
> + msg.num_evs = NUM_COMMON_EVS;
> + msg.hw_type = INVALID_IDX;
> + msg.cid[INST_IDX] = EV_INST_RETIRED;
> + msg.cid[CYC_IDX] = EV_CPU_CYCLES;
> + msg.cid[FE_STALL_IDX] = INVALID_IDX;
> + msg.cid[BE_STALL_IDX] = INVALID_IDX;
> +
> + ret = ops->set_param(info->ph, &msg, MEMLAT_ALGO_STR, MEMLAT_SET_COMMON_EV_MAP,
> + sizeof(msg));
> + return ret;
> +}
> +
> +static int configure_cpucp_grp(struct scmi_memlat_info *info, int memory_index)
> +{
> + const struct qcom_scmi_vendor_ops *ops = info->ops;
> + struct scmi_memory_info *memory = info->memory[memory_index];
> + struct ev_map_msg ev_msg;
> + u8 ev_map[NUM_GRP_EVS];
> + struct node_msg msg;
> + int ret;
> +
> + msg.cpumask = 0;
> + msg.hw_type = memory->hw_type;
> + msg.mon_type = 0;
> + msg.mon_idx = 0;
> + ret = ops->set_param(info->ph, &msg, MEMLAT_ALGO_STR, MEMLAT_SET_MEM_GROUP, sizeof(msg));
> + if (ret < 0) {
> + pr_err("Failed to configure mem type %d\n", memory->hw_type);
> + return ret;
> + }
> +
> + memset(ev_map, 0xFF, NUM_GRP_EVS);
> + ev_msg.num_evs = NUM_GRP_EVS;
> + ev_msg.hw_type = memory->hw_type;
> + ev_msg.cid[MISS_IDX] = EV_L2_D_RFILL;
> + ev_msg.cid[WB_IDX] = INVALID_IDX;
> + ev_msg.cid[ACC_IDX] = INVALID_IDX;
> + ret = ops->set_param(info->ph, &ev_msg, MEMLAT_ALGO_STR, MEMLAT_SET_GRP_EV_MAP,
> + sizeof(ev_msg));
> + if (ret < 0) {
> + pr_err("Failed to configure event map for mem type %d\n", memory->hw_type);
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +static int configure_cpucp_mon(struct scmi_memlat_info *info, int memory_index, int monitor_index)
> +{
> + const struct qcom_scmi_vendor_ops *ops = info->ops;
> + struct scmi_memory_info *memory = info->memory[memory_index];
> + struct scmi_monitor_info *monitor = memory->monitor[monitor_index];
> + struct scalar_param_msg scalar_msg;
> + struct map_param_msg map_msg;
> + struct node_msg msg;
> + int ret;
> + int i;
> +
> + msg.cpumask = monitor->mask;
> + msg.hw_type = memory->hw_type;
> + msg.mon_type = monitor->mon_type;
> + msg.mon_idx = monitor->mon_idx;
> + strscpy(msg.mon_name, monitor->mon_name, sizeof(msg.mon_name));
> + ret = ops->set_param(info->ph, &msg, MEMLAT_ALGO_STR, MEMLAT_SET_MONITOR, sizeof(msg));
> + if (ret < 0) {
> + pr_err("Failed to configure monitor %s\n", monitor->mon_name);
> + return ret;
> + }
> +
> + scalar_msg.hw_type = memory->hw_type;
> + scalar_msg.mon_idx = monitor->mon_idx;
> + scalar_msg.val = monitor->ipm_ceil;
> + ret = ops->set_param(info->ph, &scalar_msg, MEMLAT_ALGO_STR, MEMLAT_IPM_CEIL,
> + sizeof(scalar_msg));
> + if (ret < 0) {
> + pr_err("Failed to set ipm ceil for %s\n", monitor->mon_name);
> + return ret;
> + }
> +
> + map_msg.hw_type = memory->hw_type;
> + map_msg.mon_idx = monitor->mon_idx;
> + map_msg.nr_rows = monitor->freq_map_len;
> + for (i = 0; i < monitor->freq_map_len; i++) {
> + map_msg.tbl[i].v1 = monitor->freq_map[i].cpufreq_mhz;
> + map_msg.tbl[i].v2 = monitor->freq_map[i].memfreq_khz / 1000;
> + }
So this table goes 1:1 to the firmware? Is it going to be the same for
all versions of the SoC? If so, it might be better to turn it into the
static data inside the driver. If it doesn't change, there is no need
to put it to DT.
> + ret = ops->set_param(info->ph, &map_msg, MEMLAT_ALGO_STR, MEMLAT_MON_FREQ_MAP,
> + sizeof(map_msg));
> + if (ret < 0) {
> + pr_err("Failed to configure freq_map for %s\n", monitor->mon_name);
> + return ret;
> + }
> +
> + scalar_msg.hw_type = memory->hw_type;
> + scalar_msg.mon_idx = monitor->mon_idx;
> + scalar_msg.val = memory->min_freq;
> + ret = ops->set_param(info->ph, &scalar_msg, MEMLAT_ALGO_STR, MEMLAT_SET_MIN_FREQ,
> + sizeof(scalar_msg));
> + if (ret < 0) {
> + pr_err("Failed to set min_freq for %s\n", monitor->mon_name);
> + return ret;
> + }
> +
> + scalar_msg.hw_type = memory->hw_type;
> + scalar_msg.mon_idx = monitor->mon_idx;
> + scalar_msg.val = memory->max_freq;
> + ret = ops->set_param(info->ph, &scalar_msg, MEMLAT_ALGO_STR, MEMLAT_SET_MAX_FREQ,
> + sizeof(scalar_msg));
> + if (ret < 0)
> + pr_err("Failed to set max_freq for %s\n", monitor->mon_name);
> +
> + return ret;
> +}
> +
> +static int cpucp_memlat_init(struct scmi_device *sdev)
> +{
> + const struct scmi_handle *handle = sdev->handle;
> + const struct qcom_scmi_vendor_ops *ops;
> + struct scmi_protocol_handle *ph;
> + struct scmi_memlat_info *info;
> + u32 cpucp_sample_ms = 8;
> + int ret, i, j;
> +
> + if (!handle)
> + return -ENODEV;
> +
> + ops = handle->devm_protocol_get(sdev, QCOM_SCMI_VENDOR_PROTOCOL, &ph);
> + if (IS_ERR(ops))
> + return PTR_ERR(ops);
> +
> + info = devm_kzalloc(&sdev->dev, sizeof(*info), GFP_KERNEL);
> + if (!info)
> + return -ENOMEM;
> +
> + ret = process_scmi_memlat_of_node(sdev, info);
> + if (ret)
> + pr_err("Failed to configure common events: %d\n", ret);
> +
> + info->ph = ph;
> + info->ops = ops;
> +
> + ret = configure_cpucp_common_events(info);
> + if (ret < 0)
> + pr_err("Failed to configure common events: %d\n", ret);
> +
> + for (i = 0; i < info->memory_cnt; i++) {
> + ret = configure_cpucp_grp(info, i);
> + if (ret < 0)
> + pr_err("Failed to configure mem group: %d\n", ret);
> +
> + for (j = 0; j < info->memory[i]->monitor_cnt; j++) {
> + /* Configure per monitor parameters */
> + ret = configure_cpucp_mon(info, i, j);
> + if (ret < 0)
> + pr_err("Failed to configure monitor: %d\n", ret);
> + }
> + }
> +
> + ret = ops->set_param(ph, &cpucp_sample_ms, MEMLAT_ALGO_STR, MEMLAT_SAMPLE_MS,
> + sizeof(cpucp_sample_ms));
> + if (ret < 0)
> + pr_err("Failed to set cpucp sample_ms ret = %d\n", ret);
> +
> + /* Start sampling and voting timer */
> + ret = ops->start_activity(ph, NULL, MEMLAT_ALGO_STR, MEMLAT_START_TIMER, 0);
> + if (ret < 0)
> + pr_err("Error in starting the mem group timer %d\n", ret);
> +
> + dev_set_drvdata(&sdev->dev, info);
> +
> + return ret;
> +}
> +
> +static int scmi_client_probe(struct scmi_device *sdev)
> +{
> + cpucp_memlat_init(sdev);
> +
> + return 0;
> +}
> +
> +static const struct scmi_device_id scmi_id_table[] = {
> + { .protocol_id = QCOM_SCMI_VENDOR_PROTOCOL, .name = "qcom_scmi_vendor_protocol" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> +
> +static struct scmi_driver qcom_scmi_client_drv = {
> + .name = "qcom-scmi-driver",
> + .probe = scmi_client_probe,
> + .id_table = scmi_id_table,
> +};
> +module_scmi_driver(qcom_scmi_client_drv);
> +
> +MODULE_DESCRIPTION("QTI SCMI client driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
>
--
With best wishes
Dmitry
On Wed, 17 Jan 2024 at 19:37, Sibi Sankar <[email protected]> wrote:
>
> Enable LLCC/DDR dvfs through the Qualcomm's SCMI vendor protocol.
Could you please post DT bindings?
>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/x1e80100.dtsi | 48 ++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> index 6856a206f7fc..3dc6f32fbb4c 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> @@ -329,6 +329,54 @@ scmi_dvfs: protocol@13 {
> reg = <0x13>;
> #clock-cells = <1>;
> };
> +
> + scmi_vendor: protocol@80 {
> + reg = <0x80>;
> +
> + memlat {
This doesn't look like a generic node name.
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + memory@0 {
> + reg = <0x0>; /* Memory Type DDR */
> + freq-table-khz = <200000 4224000>;
> +
> + monitor-0 {
> + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>;
> + qcom,cpufreq-memfreq-tbl = < 999000 547000 >,
> + < 1440000 768000 >,
> + < 1671000 1555000 >,
> + < 2189000 2092000 >,
> + < 2156000 3187000 >,
> + < 3860000 4224000 >;
These tables should be rewritten as OPP tables.
> + };
> +
> + monitor-1 {
> + qcom,compute-mon;
> + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>;
> + qcom,cpufreq-memfreq-tbl = < 1440000 200000 >,
> + < 2189000 768000 >,
> + < 2156000 1555000 >,
> + < 3860000 2092000 >;
> + };
> + };
> +
> + memory@1 {
> + reg = <0x1>; /* Memory Type LLCC */
> + freq-table-khz = <300000 1067000>;
> +
> + monitor-0 {
> + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>;
> + qcom,cpufreq-memfreq-tbl = < 999000 300000 >,
> + < 1440000 466000 >,
> + < 1671000 600000 >,
> + < 2189000 806000 >,
> + < 2156000 933000 >,
> + < 3860000 1066000 >;
> + };
> + };
> + };
> + };
> };
> };
>
> --
> 2.34.1
>
>
--
With best wishes
Dmitry
(Generic note: It is middle of merge window and I have seen multiple
series posted by you. Since I am mainly looking for bug fixes only ATM,
I may miss to look at few. You may have to ping or repost after the merge
window, just responding to this for now as it caught my attention)
On Wed, Jan 17, 2024 at 11:04:57PM +0530, Sibi Sankar wrote:
> Enable cpufreq on X1E80100 SoCs through the SCMI perf protocol node.
>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/x1e80100.dtsi | 27 ++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> index afdbd27f8346..6856a206f7fc 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> @@ -62,6 +62,7 @@ CPU0: cpu@0 {
> compatible = "qcom,oryon";
> reg = <0x0 0x0>;
> enable-method = "psci";
> + clocks = <&scmi_dvfs 0>;
I would use genpd bindings Ulf added recently. The reason I ask is I remember
one of the Qcom platform had both clocks and qcom,freq-domain and each one
served different purpose with latter one being used for cpufreq. So will
that be an issue here ?
--
Regards,
Sudeep
On Wed, Jan 17, 2024 at 11:04:54PM +0530, Sibi Sankar wrote:
> From: Shivnandan Kumar <[email protected]>
>
> 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 <[email protected]>
> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
> Co-developed-by: Amir Vajid <[email protected]>
> Signed-off-by: Amir Vajid <[email protected]>
> Co-developed-by: Sibi Sankar <[email protected]>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
> 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.
--
Regards,
Sudeep
On Wed, Jan 17, 2024 at 11:04:52PM +0530, Sibi Sankar wrote:
> Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox
> controller.
>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
> .../bindings/mailbox/qcom,cpucp-mbox.yaml | 51 +++++++++++++++++++
> 1 file changed, 51 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
>
> diff --git a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
> new file mode 100644
> index 000000000000..2617e5555acb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/qcom,cpucp-mbox.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies, Inc. CPUCP Mailbox Controller
> +
> +maintainers:
> + - Sibi Sankar <[email protected]>
> +
> +description:
> + The CPUSS Control Processor (CPUCP) mailbox controller enables communication
> + between AP and CPUCP by acting as a doorbell between them.
> +
> +properties:
> + compatible:
> + items:
> + - enum:
> + - qcom,x1e80100-cpucp-mbox
> + - const: qcom,cpucp-mbox
A generic fallback implies multiple devices use the same unchanged
block. That seems doubtful given you have not defined any others and
given Konrad's comments.
Rob
On 1/18/24 01:23, Konrad Dybcio wrote:
>
>
> On 1/17/24 18:34, Sibi Sankar wrote:
>> Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox
>> controller.
>>
>> Signed-off-by: Sibi Sankar <[email protected]>
>> ---
>
Hey Konrad,
Thanks for taking time to review the series.
> [...]
>
>> +Â - |
>> +Â Â Â #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +Â Â Â mailbox@17430000 {
>> +Â Â Â Â Â Â Â compatible = "qcom,x1e80100-cpucp-mbox", "qcom,cpucp-mbox";
>> +Â Â Â Â Â Â Â reg = <0x17430000 0x10000>, <0x18830000 0x300>;
>
> These reg spaces are quite far apart.. On 7280-8550, a similar
> mailbox exists, although it's dubbed RIMPS-mbox instead. In
> that case, I separated the mbox into tx (via
> qcom-apcs-ipc-mailbox.c) and rx (with a simple driver). Still
> haven't pushed or posted that anywhere, I'd need to access
> another machine..
>
> On (some of) these SoCs, one of the channels (rx[1], iirc?) clearly
> bleeds into the CPUFREQ_HW/OSM register region, which gives an
> impression of misrepresenting the hardware. X1E doesn't have a
> node for cpufreq_hw defined, so I can't tell whether it's also the
> case here.
I am aware of ^^ discussion and the X1E doesn't have this problem.
Both the regions described are only used for mailbox communication.
X1E uses the scmi perf protocol for cpu dvfs.
-Sibi
>
> Konrad
On 1/30/24 22:42, Rob Herring wrote:
> On Wed, Jan 17, 2024 at 11:04:52PM +0530, Sibi Sankar wrote:
>> Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox
>> controller.
Hey Rob,
Thanks for taking time to review the series.
>>
>> Signed-off-by: Sibi Sankar <[email protected]>
>> ---
>> .../bindings/mailbox/qcom,cpucp-mbox.yaml | 51 +++++++++++++++++++
>> 1 file changed, 51 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
>> new file mode 100644
>> index 000000000000..2617e5555acb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
>> @@ -0,0 +1,51 @@
>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mailbox/qcom,cpucp-mbox.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Technologies, Inc. CPUCP Mailbox Controller
>> +
>> +maintainers:
>> + - Sibi Sankar <[email protected]>
>> +
>> +description:
>> + The CPUSS Control Processor (CPUCP) mailbox controller enables communication
>> + between AP and CPUCP by acting as a doorbell between them.
>> +
>> +properties:
>> + compatible:
>> + items:
>> + - enum:
>> + - qcom,x1e80100-cpucp-mbox
>> + - const: qcom,cpucp-mbox
>
> A generic fallback implies multiple devices use the same unchanged
> block. That seems doubtful given you have not defined any others and
> given Konrad's comments.
This mbox is expected to be used as is on a number of future SoCs,
that's the only reason I added the generic fallback. I can drop it
in the next re-spin if you want.
-Sibi
>
> Rob
On 1/18/24 01:45, Konrad Dybcio wrote:
>
>
> On 1/17/24 18:34, Sibi Sankar wrote:
>> From: Shivnandan Kumar <[email protected]>
>>
>> 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 <[email protected]>
>> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
>> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
>> Co-developed-by: Amir Vajid <[email protected]>
>> Signed-off-by: Amir Vajid <[email protected]>
>> Co-developed-by: Sibi Sankar <[email protected]>
>> Signed-off-by: Sibi Sankar <[email protected]>
>> ---
>
> 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 08/02/2024 11:28, Sibi Sankar wrote:
>>> +properties:
>>> + compatible:
>>> + items:
>>> + - enum:
>>> + - qcom,x1e80100-cpucp-mbox
>>> + - const: qcom,cpucp-mbox
>>
>> A generic fallback implies multiple devices use the same unchanged
>> block. That seems doubtful given you have not defined any others and
>> given Konrad's comments.
>
> This mbox is expected to be used as is on a number of future SoCs,
> that's the only reason I added the generic fallback. I can drop it
> in the next re-spin if you want.
Given that, if you ever have compatible devices, just use
device-specific compatible as fallback.
Best regards,
Krzysztof
On 8.02.2024 11:22, Sibi Sankar wrote:
>
>
> On 1/18/24 01:23, Konrad Dybcio wrote:
>>
>>
>> On 1/17/24 18:34, Sibi Sankar wrote:
>>> Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox
>>> controller.
>>>
>>> Signed-off-by: Sibi Sankar <[email protected]>
>>> ---
>>
>
> Hey Konrad,
>
> Thanks for taking time to review the series.
>
>> [...]
>>
>>> +Â - |
>>> +Â Â Â #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +
>>> +Â Â Â mailbox@17430000 {
>>> +Â Â Â Â Â Â Â compatible = "qcom,x1e80100-cpucp-mbox", "qcom,cpucp-mbox";
>>> +Â Â Â Â Â Â Â reg = <0x17430000 0x10000>, <0x18830000 0x300>;
>>
>> These reg spaces are quite far apart.. On 7280-8550, a similar
>> mailbox exists, although it's dubbed RIMPS-mbox instead. In
>> that case, I separated the mbox into tx (via
>> qcom-apcs-ipc-mailbox.c) and rx (with a simple driver). Still
>> haven't pushed or posted that anywhere, I'd need to access
>> another machine..
>>
>> On (some of) these SoCs, one of the channels (rx[1], iirc?) clearly
>> bleeds into the CPUFREQ_HW/OSM register region, which gives an
>> impression of misrepresenting the hardware. X1E doesn't have a
>> node for cpufreq_hw defined, so I can't tell whether it's also the
>> case here.
>
> I am aware of ^^ discussion and the X1E doesn't have this problem.
> Both the regions described are only used for mailbox communication.
> X1E uses the scmi perf protocol for cpu dvfs.
Yes, that's clear.
I am however asking for something different: I presume the CPUSS
IP hasn't changed too much on this SoC, other than having new cores and
OSM now being controlled through a different firmware interface, and I'd
like to keep the hardware description in our DT as close to the metal as
possible.
In other words, if the good ol' OSM hardware is indeed there under however
many layers of firmware, and if RX does indeed bleed into its register
space, I'd prefer there be at least a syscon node describing the actual
block, and not a magic hwio entry that's many zeroes away.
Konrad
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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
>>> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
>>> Co-developed-by: Amir Vajid <[email protected]>
>>> Signed-off-by: Amir Vajid <[email protected]>
>>> Co-developed-by: Sibi Sankar <[email protected]>
>>> Signed-off-by: Sibi Sankar <[email protected]>
>>> ---
>>
>> 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 2/9/24 04:44, Konrad Dybcio wrote:
> On 8.02.2024 11:22, Sibi Sankar wrote:
>>
>>
>> On 1/18/24 01:23, Konrad Dybcio wrote:
>>>
>>>
>>> On 1/17/24 18:34, Sibi Sankar wrote:
>>>> Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox
>>>> controller.
>>>>
>>>> Signed-off-by: Sibi Sankar <[email protected]>
>>>> ---
>>>
>>
>> Hey Konrad,
>>
>> Thanks for taking time to review the series.
>>
>>> [...]
>>>
>>>> +Â - |
>>>> +Â Â Â #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>> +
>>>> +Â Â Â mailbox@17430000 {
>>>> +Â Â Â Â Â Â Â compatible = "qcom,x1e80100-cpucp-mbox", "qcom,cpucp-mbox";
>>>> +Â Â Â Â Â Â Â reg = <0x17430000 0x10000>, <0x18830000 0x300>;
>>>
>>> These reg spaces are quite far apart.. On 7280-8550, a similar
>>> mailbox exists, although it's dubbed RIMPS-mbox instead. In
>>> that case, I separated the mbox into tx (via
>>> qcom-apcs-ipc-mailbox.c) and rx (with a simple driver). Still
>>> haven't pushed or posted that anywhere, I'd need to access
>>> another machine..
>>>
>>> On (some of) these SoCs, one of the channels (rx[1], iirc?) clearly
>>> bleeds into the CPUFREQ_HW/OSM register region, which gives an
>>> impression of misrepresenting the hardware. X1E doesn't have a
>>> node for cpufreq_hw defined, so I can't tell whether it's also the
>>> case here.
>>
>> I am aware of ^^ discussion and the X1E doesn't have this problem.
>> Both the regions described are only used for mailbox communication.
>> X1E uses the scmi perf protocol for cpu dvfs.
>
> Yes, that's clear.
>
> I am however asking for something different: I presume the CPUSS
> IP hasn't changed too much on this SoC, other than having new cores and
> OSM now being controlled through a different firmware interface, and I'd
> like to keep the hardware description in our DT as close to the metal as
> possible.
>
> In other words, if the good ol' OSM hardware is indeed there under however
> many layers of firmware, and if RX does indeed bleed into its register
> space, I'd prefer there be at least a syscon node describing the actual
> block, and not a magic hwio entry that's many zeroes away.
>
With the new cores X1E does not have any artifacts from the legacy
OSM way that Qualcomm has followed till now. If it indeed existed it
would make zero sense to vote for CPU frequencies through a mailbox than
vote for it directly.
-Sibi
> Konrad
>
On 1/18/24 00:39, Dmitry Baryshkov wrote:
> On Wed, 17 Jan 2024 at 19:36, Sibi Sankar <[email protected]> wrote:
>>
>> From: Shivnandan Kumar <[email protected]>
>>
>> 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 <[email protected]>
>> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
>> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
>> Co-developed-by: Amir Vajid <[email protected]>
>> Signed-off-by: Amir Vajid <[email protected]>
>> Co-developed-by: Sibi Sankar <[email protected]>
>> Signed-off-by: Sibi Sankar <[email protected]>
>> ---
>> 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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
>>>> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
>>>> Co-developed-by: Amir Vajid <[email protected]>
>>>> Signed-off-by: Amir Vajid <[email protected]>
>>>> Co-developed-by: Sibi Sankar <[email protected]>
>>>> Signed-off-by: Sibi Sankar <[email protected]>
>>>> ---
>>>
>>> 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 <[email protected]>
>>
>> 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 <[email protected]>
>> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
>> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
>> Co-developed-by: Amir Vajid <[email protected]>
>> Signed-off-by: Amir Vajid <[email protected]>
>> Co-developed-by: Sibi Sankar <[email protected]>
>> Signed-off-by: Sibi Sankar <[email protected]>
>> ---
>> 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 1/18/24 20:55, Sudeep Holla wrote:
> (Generic note: It is middle of merge window and I have seen multiple
> series posted by you. Since I am mainly looking for bug fixes only ATM,
> I may miss to look at few. You may have to ping or repost after the merge
> window, just responding to this for now as it caught my attention)
ack
>
> On Wed, Jan 17, 2024 at 11:04:57PM +0530, Sibi Sankar wrote:
>> Enable cpufreq on X1E80100 SoCs through the SCMI perf protocol node.
>>
>> Signed-off-by: Sibi Sankar <[email protected]>
>> ---
>> arch/arm64/boot/dts/qcom/x1e80100.dtsi | 27 ++++++++++++++++++++++++++
>> 1 file changed, 27 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> index afdbd27f8346..6856a206f7fc 100644
>> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> @@ -62,6 +62,7 @@ CPU0: cpu@0 {
>> compatible = "qcom,oryon";
>> reg = <0x0 0x0>;
>> enable-method = "psci";
>> + clocks = <&scmi_dvfs 0>;
>
> I would use genpd bindings Ulf added recently. The reason I ask is I remember
> one of the Qcom platform had both clocks and qcom,freq-domain and each one
> served different purpose with latter one being used for cpufreq. So will
> that be an issue here ?
The cpufreq-hw node that Qualcomm used had a opp-table associated with
it to vote for various buses which in turn required both clock and freq-
domain. However the memory buses voting is done by the vendor protocol
0x80 on X1E and hence won't be an issue here.
-Sibi
>
On 1/18/24 02:17, Dmitry Baryshkov wrote:
> On Wed, 17 Jan 2024 at 19:37, Sibi Sankar <[email protected]> wrote:
>>
>> Enable LLCC/DDR dvfs through the Qualcomm's SCMI vendor protocol.
>
> Could you please post DT bindings?
ack, will include it in the next re-spin.
>
>>
>> Signed-off-by: Sibi Sankar <[email protected]>
>> ---
>> arch/arm64/boot/dts/qcom/x1e80100.dtsi | 48 ++++++++++++++++++++++++++
>> 1 file changed, 48 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> index 6856a206f7fc..3dc6f32fbb4c 100644
>> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> @@ -329,6 +329,54 @@ scmi_dvfs: protocol@13 {
>> reg = <0x13>;
>> #clock-cells = <1>;
>> };
>> +
>> + scmi_vendor: protocol@80 {
>> + reg = <0x80>;
>> +
>> + memlat {
>
> This doesn't look like a generic node name.
>
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>
>> +
>> + memory@0 {
>> + reg = <0x0>; /* Memory Type DDR */
>> + freq-table-khz = <200000 4224000>;
>> +
>> + monitor-0 {
>> + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>;
>
>
>
>> + qcom,cpufreq-memfreq-tbl = < 999000 547000 >,
>> + < 1440000 768000 >,
>> + < 1671000 1555000 >,
>> + < 2189000 2092000 >,
>> + < 2156000 3187000 >,
>> + < 3860000 4224000 >;
>
> These tables should be rewritten as OPP tables. >
>
>> + };
>> +
>> + monitor-1 {
>> + qcom,compute-mon;
>> + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>;
>> + qcom,cpufreq-memfreq-tbl = < 1440000 200000 >,
>> + < 2189000 768000 >,
>> + < 2156000 1555000 >,
>> + < 3860000 2092000 >;
>> + };
>> + };
>> +
>> + memory@1 {
>> + reg = <0x1>; /* Memory Type LLCC */
>> + freq-table-khz = <300000 1067000>;
>> +
>> + monitor-0 {
>> + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>;
>> + qcom,cpufreq-memfreq-tbl = < 999000 300000 >,
>> + < 1440000 466000 >,
>> + < 1671000 600000 >,
>> + < 2189000 806000 >,
>> + < 2156000 933000 >,
>> + < 3860000 1066000 >;
>> + };
>> + };
>> + };
>> + };
>> };
>> };
>>
>> --
>> 2.34.1
>>
>>
>
>
> --
> With best wishes
> Dmitry
On 1/18/24 02:08, Konrad Dybcio wrote:
>
>
> On 1/17/24 18:34, Sibi Sankar wrote:
>> Enable LLCC/DDR dvfs through the Qualcomm's SCMI vendor protocol.
>>
>> Signed-off-by: Sibi Sankar <[email protected]>
>> ---
>> Â arch/arm64/boot/dts/qcom/x1e80100.dtsi | 48 ++++++++++++++++++++++++++
>> Â 1 file changed, 48 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> index 6856a206f7fc..3dc6f32fbb4c 100644
>> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> @@ -329,6 +329,54 @@ scmi_dvfs: protocol@13 {
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â reg = <0x13>;
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â #clock-cells = <1>;
>> Â Â Â Â Â Â Â Â Â Â Â Â Â };
>> +
>> +Â Â Â Â Â Â Â Â Â Â Â scmi_vendor: protocol@80 {
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â reg = <0x80>;
>> +
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â memlat {
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â #address-cells = <1>;
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â #size-cells = <0>;
>> +
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â memory@0 {
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â reg = <0x0>; /* Memory Type DDR */
>
> I'm not sure reg is the best property to (ab)use..
I'm ok with introducing a custom property as well. I went
ahead with reg mainly because the overall structure looked
similar to audio apr.
>
> You could very well define a new one, like qcom,memory type,
> then the subnodes could look like:
>
> memory-0 {
> Â Â Â Â qcom,memory-type = <QCOM_MEM_TYPE_DDR>;
> Â Â Â Â [...]
> };
>
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â freq-table-khz = <200000 4224000>;
>> +
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â monitor-0 {
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3
>> &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>;
>
> I fail to see the usefulness in checking which CPUs make use of
> the same DRAM or LLC pool. If that's something that may not be
> obvious in future designs like on dual-socket x86 servers,
> I think it can be deferred until then and for now, AFAIU you
> can just unconditionally assume all CPUs count.
we list all the cpus here because on X1E they are identical
and have the same cpu frequency to memory frequency mapping.
But doesn't really apply to other SoCs in general. But dropping
this would mean that driver assumes a table applies to all
cpus by default.
>
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â qcom,cpufreq-memfreq-tbl = < 999000
>> 547000 >,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â < 1440000 768000 >,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â < 1671000 1555000 >,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â < 2189000 2092000 >,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â < 2156000 3187000 >,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â < 3860000 4224000 >;
>
> I.. can't seem to think of a future where this doesn't explode.
Not really ... You can already see a more or less standard table
being used across various skus on older SoCs that uses memlat
running from the kernel downstream. So that should count for
something.
>
> When you release a different bin/SKU/fuse config of this SoC where
> the CPU frequencies are different, this will likely also need to be
> updated. We don't want that manual cruft in the devicetree.
Also unlike cpufreq map, if you notice this table doesn't list all
possible cpu frequencies but list broad ranges instead. This way
the table rarely needs updates unless we want to scale to max llcc/ddr
at a lower CPU frequency for a particular SKU.
>
> Since both previously cpufreq-hw and now cpufreq-scmi generally
> operate on levels that map to some frequencies in the firmware,
> could it be bound to that instead?
At this point the only decision is whether the table lies in dt
or in the driver. But driver wouldn't even have a way to distinguish
between various skus so the dt looks like the only option.
-Sibi
>
> Konrad
>
On 1/18/24 02:11, Dmitry Baryshkov wrote:
> On Wed, 17 Jan 2024 at 19:36, Sibi Sankar <[email protected]> wrote:
>>
>> From: Shivnandan Kumar <[email protected]>
>>
>> This patch introduces a client driver that interacts with the SCMI QCOM
>
> git grep This.patch Documentation/process/
>
>> vendor protocol and passes on the required tuneables to start various
>> features running on the SCMI controller.
>
> Is there any word about the 'memlat'? No. Unless one reads into the
> patch, one can not come up with the idea of what is being introduced.
ack, will fix it in the re-spin.
>
>>
>> Signed-off-by: Shivnandan Kumar <[email protected]>
>> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
>> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
>> Co-developed-by: Amir Vajid <[email protected]>
>> Signed-off-by: Amir Vajid <[email protected]>
>> Co-developed-by: Sibi Sankar <[email protected]>
>> Signed-off-by: Sibi Sankar <[email protected]>
>> ---
>> drivers/soc/qcom/Kconfig | 10 +
>> drivers/soc/qcom/Makefile | 1 +
>> drivers/soc/qcom/qcom_scmi_client.c | 486 ++++++++++++++++++++++++++++
>
> Should it go to drivers/firmware/arm_scmi instead? Or maybe to drivers/devfreq?
I don't think it should go into arm_scmi unless Sudeep wants it there.
As for drivers/devfreq, I would have moved it there if this driver
benfitted being classified as a devfreq device. We can't use any of
the available governors on it and the tuneables appear way too custom.
These are the reasons why I placed it in drivers/soc/qcom instead.
>
>> 3 files changed, 497 insertions(+)
>> create mode 100644 drivers/soc/qcom/qcom_scmi_client.c
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index c6ca4de42586..1530558aebfb 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -264,6 +264,16 @@ config QCOM_ICC_BWMON
>> the fixed bandwidth votes from cpufreq (CPU nodes) thus achieve high
>> memory throughput even with lower CPU frequencies.
>>
>> +config QCOM_SCMI_CLIENT
>> + tristate "Qualcomm Technologies Inc. SCMI client driver"
>> + depends on QCOM_SCMI_VENDOR_PROTOCOL || COMPILE_TEST
>> + default n
>> + help
>> + SCMI client driver registers for SCMI QCOM vendor protocol.
>> +
>> + This driver interacts with the vendor protocol and passes on the required
>> + tuneables to start various features running on the SCMI controller.
>> +
>> config QCOM_INLINE_CRYPTO_ENGINE
>> tristate
>> select QCOM_SCM
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 05b3d54e8dc9..c2a51293c886 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -32,5 +32,6 @@ obj-$(CONFIG_QCOM_APR) += apr.o
>> obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
>> obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
>> obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
>> +obj-$(CONFIG_QCOM_SCMI_CLIENT) += qcom_scmi_client.o
>> qcom_ice-objs += ice.o
>> obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom_ice.o
>> diff --git a/drivers/soc/qcom/qcom_scmi_client.c b/drivers/soc/qcom/qcom_scmi_client.c
>> new file mode 100644
>> index 000000000000..418aa7900496
>> --- /dev/null
>> +++ b/drivers/soc/qcom/qcom_scmi_client.c
>> @@ -0,0 +1,486 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/cpu.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/qcom_scmi_vendor.h>
>> +#include <linux/scmi_protocol.h>
>> +
>> +#define MAX_MEMORY_TYPES 3
>> +#define MEMLAT_ALGO_STR 0x74616C6D656D /* "memlat" */
>> +#define INVALID_IDX 0xFF
>> +#define MAX_NAME_LEN 20
>> +#define MAX_MAP_ENTRIES 6
>> +#define MAX_MONITOR_CNT 4
>> +#define SCMI_VENDOR_MSG_START 3
>> +#define SCMI_VENDOR_MSG_MODULE_START 16
>> +
>> +enum scmi_memlat_protocol_cmd {
>> + MEMLAT_SET_LOG_LEVEL = SCMI_VENDOR_MSG_START,
>> + MEMLAT_FLUSH_LOGBUF,
>> + MEMLAT_SET_MEM_GROUP = SCMI_VENDOR_MSG_MODULE_START,
>> + MEMLAT_SET_MONITOR,
>> + MEMLAT_SET_COMMON_EV_MAP,
>> + MEMLAT_SET_GRP_EV_MAP,
>> + MEMLAT_ADAPTIVE_LOW_FREQ,
>> + MEMLAT_ADAPTIVE_HIGH_FREQ,
>> + MEMLAT_GET_ADAPTIVE_CUR_FREQ,
>> + MEMLAT_IPM_CEIL,
>> + MEMLAT_FE_STALL_FLOOR,
>> + MEMLAT_BE_STALL_FLOOR,
>> + MEMLAT_WB_PCT,
>> + MEMLAT_IPM_FILTER,
>> + MEMLAT_FREQ_SCALE_PCT,
>> + MEMLAT_FREQ_SCALE_CEIL_MHZ,
>> + MEMLAT_FREQ_SCALE_FLOOR_MHZ,
>> + MEMLAT_SAMPLE_MS,
>> + MEMLAT_MON_FREQ_MAP,
>> + MEMLAT_SET_MIN_FREQ,
>> + MEMLAT_SET_MAX_FREQ,
>> + MEMLAT_GET_CUR_FREQ,
>> + MEMLAT_START_TIMER,
>> + MEMLAT_STOP_TIMER,
>> + MEMLAT_GET_TIMESTAMP,
>> + MEMLAT_MAX_MSG
>> +};
>> +
>> +struct map_table {
>> + u16 v1;
>> + u16 v2;
>> +};
>> +
>
> Any documentation for these structures? It won't bite you, but it will
> help reviewers and other developers.
ack
>
>> +struct map_param_msg {
>> + u32 hw_type;
>> + u32 mon_idx;
>> + u32 nr_rows;
>> + struct map_table tbl[MAX_MAP_ENTRIES];
>> +} __packed;
>> +
>> +struct node_msg {
>> + u32 cpumask;
>> + u32 hw_type;
>> + u32 mon_type;
>> + u32 mon_idx;
>> + char mon_name[MAX_NAME_LEN];
>> +};
>> +
>> +struct scalar_param_msg {
>> + u32 hw_type;
>> + u32 mon_idx;
>> + u32 val;
>> +};
>> +
>> +enum common_ev_idx {
>> + INST_IDX,
>> + CYC_IDX,
>> + FE_STALL_IDX,
>> + BE_STALL_IDX,
>> + NUM_COMMON_EVS
>> +};
>> +
>> +enum grp_ev_idx {
>> + MISS_IDX,
>> + WB_IDX,
>> + ACC_IDX,
>> + NUM_GRP_EVS
>> +};
>> +
>> +#define EV_CPU_CYCLES 0
>> +#define EV_INST_RETIRED 2
>> +#define EV_L2_D_RFILL 5
>> +
>> +struct ev_map_msg {
>> + u32 num_evs;
>> + u32 hw_type;
>> + u32 cid[NUM_COMMON_EVS];
>> +};
>> +
>> +struct cpufreq_memfreq_map {
>> + unsigned int cpufreq_mhz;
>> + unsigned int memfreq_khz;
>> +};
>> +
>> +struct scmi_monitor_info {
>> + struct cpufreq_memfreq_map *freq_map;
>> + char mon_name[MAX_NAME_LEN];
>> + u32 mon_idx;
>> + u32 mon_type;
>> + u32 ipm_ceil;
>> + u32 mask;
>> + u32 freq_map_len;
>> +};
>> +
>> +struct scmi_memory_info {
>> + struct scmi_monitor_info *monitor[MAX_MONITOR_CNT];
>> + u32 hw_type;
>> + int monitor_cnt;
>> + u32 min_freq;
>> + u32 max_freq;
>> +};
>> +
>> +struct scmi_memlat_info {
>> + struct scmi_protocol_handle *ph;
>> + const struct qcom_scmi_vendor_ops *ops;
>> + struct scmi_memory_info *memory[MAX_MEMORY_TYPES];
>> + int memory_cnt;
>> +};
>> +
>> +static int get_mask(struct device_node *np, u32 *mask)
>> +{
>> + struct device_node *dev_phandle;
>> + struct device *cpu_dev;
>> + int cpu, i = 0;
>> + int ret = -ENODEV;
>> +
>> + dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
>> + while (dev_phandle) {
>> + for_each_possible_cpu(cpu) {
>> + cpu_dev = get_cpu_device(cpu);
>> + if (cpu_dev && cpu_dev->of_node == dev_phandle) {
>> + *mask |= BIT(cpu);
>> + ret = 0;
>> + break;
>> + }
>> + }
>
> There is of_cpu_node_to_id(). No need to reinvent it.
ack
>
>> + dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static struct cpufreq_memfreq_map *init_cpufreq_memfreq_map(struct device *dev,
>> + struct device_node *of_node,
>> + u32 *cnt)
>> +{
>> + int len, nf, i, j;
>> + u32 data;
>> + struct cpufreq_memfreq_map *tbl;
>> + int ret;
>> +
>> + if (!of_find_property(of_node, "qcom,cpufreq-memfreq-tbl", &len))
>> + return NULL;
>> + len /= sizeof(data);
>> +
>> + if (len % 2 || len == 0)
>> + return NULL;
>> + nf = len / 2;
>> +
>> + tbl = devm_kzalloc(dev, (nf + 1) * sizeof(struct cpufreq_memfreq_map),
>> + GFP_KERNEL);
>> + if (!tbl)
>> + return NULL;
>> +
>> + for (i = 0, j = 0; i < nf; i++, j += 2) {
>> + ret = of_property_read_u32_index(of_node, "qcom,cpufreq-memfreq-tbl",
>> + j, &data);
>> + if (ret < 0)
>> + return NULL;
>> + tbl[i].cpufreq_mhz = data / 1000;
>> +
>> + ret = of_property_read_u32_index(of_node, "qcom,cpufreq-memfreq-tbl",
>> + j + 1, &data);
>> + if (ret < 0)
>> + return NULL;
>> +
>> + tbl[i].memfreq_khz = data;
>> + pr_debug("Entry%d CPU:%u, Mem:%u\n", i, tbl[i].cpufreq_mhz,
>> + tbl[i].memfreq_khz);
>> + }
>> + *cnt = nf;
>> + tbl[i].cpufreq_mhz = 0;
>
> This looks like a lame version of the OPP table.
I didn't know listing multiple frequencies in a opp was allowed. We can
probably get away with it here since we just parse the data here and not
populate data in the opp core.
>
>> +
>> + return tbl;
>> +}
>> +
>> +static int process_scmi_memlat_of_node(struct scmi_device *sdev, struct scmi_memlat_info *info)
>> +{
>> + struct device_node *memlat_np, *memory_np, *monitor_np;
>> + struct scmi_memory_info *memory;
>> + struct scmi_monitor_info *monitor;
>> + int ret = 0, i = 0, j;
>> + u32 memfreq[2];
>> +
>> + of_node_get(sdev->handle->dev->of_node);
>> + memlat_np = of_find_node_by_name(sdev->handle->dev->of_node, "memlat");
>> +
>> + info->memory_cnt = of_get_child_count(memlat_np);
>> + if (info->memory_cnt <= 0)
>> + pr_err("No memory nodes present\n");
>> +
>> + for_each_child_of_node(memlat_np, memory_np) {
>> + memory = devm_kzalloc(&sdev->dev, sizeof(*memory), GFP_KERNEL);
>> + if (!memory) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> +
>> + ret = of_property_read_u32(memory_np, "reg", &memory->hw_type);
>> + if (ret) {
>> + pr_err("Failed to read memory type\n");
>> + goto err;
>> + }
>> +
>> + memory->monitor_cnt = of_get_child_count(memory_np);
>> + if (memory->monitor_cnt <= 0) {
>> + pr_err("No monitor nodes present\n");
>> + ret = -EINVAL;
>> + goto err;
>> + }
>> +
>> + ret = of_property_read_u32_array(memory_np, "freq-table-khz", memfreq, 2);
>> + if (ret && (ret != -EINVAL)) {
>> + pr_err("Failed to read min/max freq %d\n", ret);
>> + goto err;
>> + }
>
> foo-min-frequency and foo-max-frequency please.
ack
>
>> +
>> + memory->min_freq = memfreq[0];
>> + memory->max_freq = memfreq[1];
>> + info->memory[i] = memory;
>> + j = 0;
>> + i++;
>> +
>> + for_each_child_of_node(memory_np, monitor_np) {
>> + monitor = devm_kzalloc(&sdev->dev, sizeof(*monitor), GFP_KERNEL);
>> + if (!monitor) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> +
>> + monitor->mon_type = (of_property_read_bool(monitor_np, "qcom,compute-mon")) ? 1 : 0;
>> + monitor->ipm_ceil = (of_property_read_bool(monitor_np, "qcom,compute-mon")) ? 0 : 20000000;
>
> Huh?
lol, will add more comments.
>
>> +
>> + if (get_mask(monitor_np, &monitor->mask)) {
>> + pr_err("Failed to populate cpu mask %d\n", ret);
>> + goto err;
>> + }
>> +
>> + monitor->freq_map = init_cpufreq_memfreq_map(&sdev->dev, monitor_np,
>> + &monitor->freq_map_len);
>> + snprintf(monitor->mon_name, MAX_NAME_LEN, "monitor-%d", j);
>> + monitor->mon_idx = j;
>> +
>> + memory->monitor[j] = monitor;
>> + j++;
>> + }
>> + }
>> +
>> + return 0;
>> +
>> +err:
>> + of_node_put(memlat_np);
>> +
>> + return ret;
>> +}
>> +
>> +static int configure_cpucp_common_events(struct scmi_memlat_info *info)
>> +{
>> + const struct qcom_scmi_vendor_ops *ops = info->ops;
>> + u8 ev_map[NUM_COMMON_EVS];
>> + struct ev_map_msg msg;
>> + int ret;
>> +
>> + memset(ev_map, 0xFF, NUM_COMMON_EVS);
>> +
>> + msg.num_evs = NUM_COMMON_EVS;
>> + msg.hw_type = INVALID_IDX;
>> + msg.cid[INST_IDX] = EV_INST_RETIRED;
>> + msg.cid[CYC_IDX] = EV_CPU_CYCLES;
>> + msg.cid[FE_STALL_IDX] = INVALID_IDX;
>> + msg.cid[BE_STALL_IDX] = INVALID_IDX;
>> +
>> + ret = ops->set_param(info->ph, &msg, MEMLAT_ALGO_STR, MEMLAT_SET_COMMON_EV_MAP,
>> + sizeof(msg));
>> + return ret;
>> +}
>> +
>> +static int configure_cpucp_grp(struct scmi_memlat_info *info, int memory_index)
>> +{
>> + const struct qcom_scmi_vendor_ops *ops = info->ops;
>> + struct scmi_memory_info *memory = info->memory[memory_index];
>> + struct ev_map_msg ev_msg;
>> + u8 ev_map[NUM_GRP_EVS];
>> + struct node_msg msg;
>> + int ret;
>> +
>> + msg.cpumask = 0;
>> + msg.hw_type = memory->hw_type;
>> + msg.mon_type = 0;
>> + msg.mon_idx = 0;
>> + ret = ops->set_param(info->ph, &msg, MEMLAT_ALGO_STR, MEMLAT_SET_MEM_GROUP, sizeof(msg));
>> + if (ret < 0) {
>> + pr_err("Failed to configure mem type %d\n", memory->hw_type);
>> + return ret;
>> + }
>> +
>> + memset(ev_map, 0xFF, NUM_GRP_EVS);
>> + ev_msg.num_evs = NUM_GRP_EVS;
>> + ev_msg.hw_type = memory->hw_type;
>> + ev_msg.cid[MISS_IDX] = EV_L2_D_RFILL;
>> + ev_msg.cid[WB_IDX] = INVALID_IDX;
>> + ev_msg.cid[ACC_IDX] = INVALID_IDX;
>> + ret = ops->set_param(info->ph, &ev_msg, MEMLAT_ALGO_STR, MEMLAT_SET_GRP_EV_MAP,
>> + sizeof(ev_msg));
>> + if (ret < 0) {
>> + pr_err("Failed to configure event map for mem type %d\n", memory->hw_type);
>> + return ret;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int configure_cpucp_mon(struct scmi_memlat_info *info, int memory_index, int monitor_index)
>> +{
>> + const struct qcom_scmi_vendor_ops *ops = info->ops;
>> + struct scmi_memory_info *memory = info->memory[memory_index];
>> + struct scmi_monitor_info *monitor = memory->monitor[monitor_index];
>> + struct scalar_param_msg scalar_msg;
>> + struct map_param_msg map_msg;
>> + struct node_msg msg;
>> + int ret;
>> + int i;
>> +
>> + msg.cpumask = monitor->mask;
>> + msg.hw_type = memory->hw_type;
>> + msg.mon_type = monitor->mon_type;
>> + msg.mon_idx = monitor->mon_idx;
>> + strscpy(msg.mon_name, monitor->mon_name, sizeof(msg.mon_name));
>> + ret = ops->set_param(info->ph, &msg, MEMLAT_ALGO_STR, MEMLAT_SET_MONITOR, sizeof(msg));
>> + if (ret < 0) {
>> + pr_err("Failed to configure monitor %s\n", monitor->mon_name);
>> + return ret;
>> + }
>> +
>> + scalar_msg.hw_type = memory->hw_type;
>> + scalar_msg.mon_idx = monitor->mon_idx;
>> + scalar_msg.val = monitor->ipm_ceil;
>> + ret = ops->set_param(info->ph, &scalar_msg, MEMLAT_ALGO_STR, MEMLAT_IPM_CEIL,
>> + sizeof(scalar_msg));
>> + if (ret < 0) {
>> + pr_err("Failed to set ipm ceil for %s\n", monitor->mon_name);
>> + return ret;
>> + }
>> +
>> + map_msg.hw_type = memory->hw_type;
>> + map_msg.mon_idx = monitor->mon_idx;
>> + map_msg.nr_rows = monitor->freq_map_len;
>> + for (i = 0; i < monitor->freq_map_len; i++) {
>> + map_msg.tbl[i].v1 = monitor->freq_map[i].cpufreq_mhz;
>> + map_msg.tbl[i].v2 = monitor->freq_map[i].memfreq_khz / 1000;
>> + }
>
> So this table goes 1:1 to the firmware? Is it going to be the same for
> all versions of the SoC? If so, it might be better to turn it into the
> static data inside the driver. If it doesn't change, there is no need
> to put it to DT.
The table does go directly to the firmware but obviously varies across
SoCs. Also since it's a SCMI client driver we don't have a way to
distinguish between SoCs based on compatibles. So it made more sense to
move it to the device tree instead.
-Sibi
>
>> + ret = ops->set_param(info->ph, &map_msg, MEMLAT_ALGO_STR, MEMLAT_MON_FREQ_MAP,
>> + sizeof(map_msg));
>> + if (ret < 0) {
>> + pr_err("Failed to configure freq_map for %s\n", monitor->mon_name);
>> + return ret;
>> + }
>> +
>> + scalar_msg.hw_type = memory->hw_type;
>> + scalar_msg.mon_idx = monitor->mon_idx;
>> + scalar_msg.val = memory->min_freq;
>> + ret = ops->set_param(info->ph, &scalar_msg, MEMLAT_ALGO_STR, MEMLAT_SET_MIN_FREQ,
>> + sizeof(scalar_msg));
>> + if (ret < 0) {
>> + pr_err("Failed to set min_freq for %s\n", monitor->mon_name);
>> + return ret;
>> + }
>> +
>> + scalar_msg.hw_type = memory->hw_type;
>> + scalar_msg.mon_idx = monitor->mon_idx;
>> + scalar_msg.val = memory->max_freq;
>> + ret = ops->set_param(info->ph, &scalar_msg, MEMLAT_ALGO_STR, MEMLAT_SET_MAX_FREQ,
>> + sizeof(scalar_msg));
>> + if (ret < 0)
>> + pr_err("Failed to set max_freq for %s\n", monitor->mon_name);
>> +
>> + return ret;
>> +}
>> +
>> +static int cpucp_memlat_init(struct scmi_device *sdev)
>> +{
>> + const struct scmi_handle *handle = sdev->handle;
>> + const struct qcom_scmi_vendor_ops *ops;
>> + struct scmi_protocol_handle *ph;
>> + struct scmi_memlat_info *info;
>> + u32 cpucp_sample_ms = 8;
>> + int ret, i, j;
>> +
>> + if (!handle)
>> + return -ENODEV;
>> +
>> + ops = handle->devm_protocol_get(sdev, QCOM_SCMI_VENDOR_PROTOCOL, &ph);
>> + if (IS_ERR(ops))
>> + return PTR_ERR(ops);
>> +
>> + info = devm_kzalloc(&sdev->dev, sizeof(*info), GFP_KERNEL);
>> + if (!info)
>> + return -ENOMEM;
>> +
>> + ret = process_scmi_memlat_of_node(sdev, info);
>> + if (ret)
>> + pr_err("Failed to configure common events: %d\n", ret);
>> +
>> + info->ph = ph;
>> + info->ops = ops;
>> +
>> + ret = configure_cpucp_common_events(info);
>> + if (ret < 0)
>> + pr_err("Failed to configure common events: %d\n", ret);
>> +
>> + for (i = 0; i < info->memory_cnt; i++) {
>> + ret = configure_cpucp_grp(info, i);
>> + if (ret < 0)
>> + pr_err("Failed to configure mem group: %d\n", ret);
>> +
>> + for (j = 0; j < info->memory[i]->monitor_cnt; j++) {
>> + /* Configure per monitor parameters */
>> + ret = configure_cpucp_mon(info, i, j);
>> + if (ret < 0)
>> + pr_err("Failed to configure monitor: %d\n", ret);
>> + }
>> + }
>> +
>> + ret = ops->set_param(ph, &cpucp_sample_ms, MEMLAT_ALGO_STR, MEMLAT_SAMPLE_MS,
>> + sizeof(cpucp_sample_ms));
>> + if (ret < 0)
>> + pr_err("Failed to set cpucp sample_ms ret = %d\n", ret);
>> +
>> + /* Start sampling and voting timer */
>> + ret = ops->start_activity(ph, NULL, MEMLAT_ALGO_STR, MEMLAT_START_TIMER, 0);
>> + if (ret < 0)
>> + pr_err("Error in starting the mem group timer %d\n", ret);
>> +
>> + dev_set_drvdata(&sdev->dev, info);
>> +
>> + return ret;
>> +}
>> +
>> +static int scmi_client_probe(struct scmi_device *sdev)
>> +{
>> + cpucp_memlat_init(sdev);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct scmi_device_id scmi_id_table[] = {
>> + { .protocol_id = QCOM_SCMI_VENDOR_PROTOCOL, .name = "qcom_scmi_vendor_protocol" },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
>> +
>> +static struct scmi_driver qcom_scmi_client_drv = {
>> + .name = "qcom-scmi-driver",
>> + .probe = scmi_client_probe,
>> + .id_table = scmi_id_table,
>> +};
>> +module_scmi_driver(qcom_scmi_client_drv);
>> +
>> +MODULE_DESCRIPTION("QTI SCMI client driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.34.1
>>
>>
>
>
On 1/18/24 01:58, Konrad Dybcio wrote:
>
>
> On 1/17/24 18:34, Sibi Sankar wrote:
>> From: Shivnandan Kumar <[email protected]>
>>
>> This patch introduces a client driver that interacts with the SCMI QCOM
>> vendor protocol and passes on the required tuneables to start various
>> features running on the SCMI controller.
>>
>> Signed-off-by: Shivnandan Kumar <[email protected]>
>> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
>> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
>> Co-developed-by: Amir Vajid <[email protected]>
>> Signed-off-by: Amir Vajid <[email protected]>
>> Co-developed-by: Sibi Sankar <[email protected]>
>> Signed-off-by: Sibi Sankar <[email protected]>
>> ---
>
> [...]
>
>
>> +
>> +struct cpufreq_memfreq_map {
>> +   unsigned int           cpufreq_mhz;
>> +   unsigned int           memfreq_khz;
>> +};
>
> Weird use of tabs
will fix it in the next re-spin.
>
> [...]
>
>> +static int get_mask(struct device_node *np, u32 *mask)
>> +{
>> +Â Â Â struct device_node *dev_phandle;
>> +Â Â Â struct device *cpu_dev;
>> +Â Â Â int cpu, i = 0;
>> +Â Â Â int ret = -ENODEV;
>
> Don't initialize ret here, return 0 instead of breaking and return
> enodev otherwise.
ack
>
>> +
>> +Â Â Â dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
>> +Â Â Â while (dev_phandle) {
>> +Â Â Â Â Â Â Â for_each_possible_cpu(cpu) {
>> +Â Â Â Â Â Â Â Â Â Â Â cpu_dev = get_cpu_device(cpu);
>> +Â Â Â Â Â Â Â Â Â Â Â if (cpu_dev && cpu_dev->of_node == dev_phandle) {
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â *mask |= BIT(cpu);
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ret = 0;
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â break;
>> +Â Â Â Â Â Â Â Â Â Â Â }
>> +Â Â Â Â Â Â Â }
>
> of_cpu_node_to_id()
ack
>
>> +Â Â Â Â Â Â Â dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
>> +Â Â Â }
>> +
>> +Â Â Â return ret;
>> +}
>
>
>> +
>> +static struct cpufreq_memfreq_map *init_cpufreq_memfreq_map(struct
>> device *dev,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct device_node *of_node,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â u32 *cnt)
>
> I really feel like this is trying to reinvent OPP..
>
> if you structure your entries like so:
>
> opp-0 {
> Â Â Â Â opp-hz = /bits/ 64 <12341234 43214321>;
> };
>
> you'll be able to use all the fantastic APIs that have been
> created over the years!
I didn't know listing multiple frequencies in a opp was allowed. We can
probably get away with it here since we just parse the data here and not
populate data in the opp core.
>
> [...]
>
>> +Â Â Â Â Â Â Â Â Â Â Â monitor->mon_type = (of_property_read_bool(monitor_np,
>> "qcom,compute-mon")) ? 1 : 0;
>> +Â Â Â Â Â Â Â Â Â Â Â monitor->ipm_ceil = (of_property_read_bool(monitor_np,
>> "qcom,compute-mon")) ? 0 : 20000000;
>
> What does it even mean for a monitor to be a compute mon?
>
When a monitor is marked compute-mon it means that the table is
followed religiously irrespective whether the instruction per miss
count threshold (ipm) is exceeded or not. Equivalent to having
a cpufreq map -> l3/DDR bw mapping upstream.
> There seem to be no dt-bindings for properties referenced in this
> driver, neither in the series nor in the dependencies. This is
> strictly required.
Ack
Thanks again for reviewing the series. :)
-Sibi
>
> Konrad
On Mon, 12 Feb 2024 at 12:24, Sibi Sankar <[email protected]> wrote:
>
>
>
> On 1/18/24 02:11, Dmitry Baryshkov wrote:
> > On Wed, 17 Jan 2024 at 19:36, Sibi Sankar <[email protected]> wrote:
> >>
> >> From: Shivnandan Kumar <[email protected]>
> >>
> >> This patch introduces a client driver that interacts with the SCMI QCOM
> >
> > git grep This.patch Documentation/process/
> >
> >> vendor protocol and passes on the required tuneables to start various
> >> features running on the SCMI controller.
> >
> > Is there any word about the 'memlat'? No. Unless one reads into the
> > patch, one can not come up with the idea of what is being introduced.
>
> ack, will fix it in the re-spin.
>
> >
> >>
> >> Signed-off-by: Shivnandan Kumar <[email protected]>
> >> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
> >> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
> >> Co-developed-by: Amir Vajid <[email protected]>
> >> Signed-off-by: Amir Vajid <[email protected]>
> >> Co-developed-by: Sibi Sankar <[email protected]>
> >> Signed-off-by: Sibi Sankar <[email protected]>
> >> ---
> >> drivers/soc/qcom/Kconfig | 10 +
> >> drivers/soc/qcom/Makefile | 1 +
> >> drivers/soc/qcom/qcom_scmi_client.c | 486 ++++++++++++++++++++++++++++
> >
> > Should it go to drivers/firmware/arm_scmi instead? Or maybe to drivers/devfreq?
>
> I don't think it should go into arm_scmi unless Sudeep wants it there.
> As for drivers/devfreq, I would have moved it there if this driver
> benfitted being classified as a devfreq device. We can't use any of
> the available governors on it and the tuneables appear way too custom.
> These are the reasons why I placed it in drivers/soc/qcom instead.
>
> >
> >> 3 files changed, 497 insertions(+)
> >> create mode 100644 drivers/soc/qcom/qcom_scmi_client.c
> >>
> >> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> >> index c6ca4de42586..1530558aebfb 100644
> >> --- a/drivers/soc/qcom/Kconfig
> >> +++ b/drivers/soc/qcom/Kconfig
> >> @@ -264,6 +264,16 @@ config QCOM_ICC_BWMON
> >> the fixed bandwidth votes from cpufreq (CPU nodes) thus achieve high
> >> memory throughput even with lower CPU frequencies.
> >>
> >> +config QCOM_SCMI_CLIENT
> >> + tristate "Qualcomm Technologies Inc. SCMI client driver"
> >> + depends on QCOM_SCMI_VENDOR_PROTOCOL || COMPILE_TEST
> >> + default n
> >> + help
> >> + SCMI client driver registers for SCMI QCOM vendor protocol.
> >> +
> >> + This driver interacts with the vendor protocol and passes on the required
> >> + tuneables to start various features running on the SCMI controller.
> >> +
> >> config QCOM_INLINE_CRYPTO_ENGINE
> >> tristate
> >> select QCOM_SCM
> >> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> >> index 05b3d54e8dc9..c2a51293c886 100644
> >> --- a/drivers/soc/qcom/Makefile
> >> +++ b/drivers/soc/qcom/Makefile
> >> @@ -32,5 +32,6 @@ obj-$(CONFIG_QCOM_APR) += apr.o
> >> obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
> >> obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
> >> obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
> >> +obj-$(CONFIG_QCOM_SCMI_CLIENT) += qcom_scmi_client.o
> >> qcom_ice-objs += ice.o
> >> obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom_ice.o
> >> diff --git a/drivers/soc/qcom/qcom_scmi_client.c b/drivers/soc/qcom/qcom_scmi_client.c
> >> new file mode 100644
> >> index 000000000000..418aa7900496
> >> --- /dev/null
> >> +++ b/drivers/soc/qcom/qcom_scmi_client.c
> >> @@ -0,0 +1,486 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
> >> + */
> >> +
> >> +#include <linux/cpu.h>
> >> +#include <linux/err.h>
> >> +#include <linux/errno.h>
> >> +#include <linux/init.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/qcom_scmi_vendor.h>
> >> +#include <linux/scmi_protocol.h>
> >> +
> >> +#define MAX_MEMORY_TYPES 3
> >> +#define MEMLAT_ALGO_STR 0x74616C6D656D /* "memlat" */
> >> +#define INVALID_IDX 0xFF
> >> +#define MAX_NAME_LEN 20
> >> +#define MAX_MAP_ENTRIES 6
> >> +#define MAX_MONITOR_CNT 4
> >> +#define SCMI_VENDOR_MSG_START 3
> >> +#define SCMI_VENDOR_MSG_MODULE_START 16
> >> +
> >> +enum scmi_memlat_protocol_cmd {
> >> + MEMLAT_SET_LOG_LEVEL = SCMI_VENDOR_MSG_START,
> >> + MEMLAT_FLUSH_LOGBUF,
> >> + MEMLAT_SET_MEM_GROUP = SCMI_VENDOR_MSG_MODULE_START,
> >> + MEMLAT_SET_MONITOR,
> >> + MEMLAT_SET_COMMON_EV_MAP,
> >> + MEMLAT_SET_GRP_EV_MAP,
> >> + MEMLAT_ADAPTIVE_LOW_FREQ,
> >> + MEMLAT_ADAPTIVE_HIGH_FREQ,
> >> + MEMLAT_GET_ADAPTIVE_CUR_FREQ,
> >> + MEMLAT_IPM_CEIL,
> >> + MEMLAT_FE_STALL_FLOOR,
> >> + MEMLAT_BE_STALL_FLOOR,
> >> + MEMLAT_WB_PCT,
> >> + MEMLAT_IPM_FILTER,
> >> + MEMLAT_FREQ_SCALE_PCT,
> >> + MEMLAT_FREQ_SCALE_CEIL_MHZ,
> >> + MEMLAT_FREQ_SCALE_FLOOR_MHZ,
> >> + MEMLAT_SAMPLE_MS,
> >> + MEMLAT_MON_FREQ_MAP,
> >> + MEMLAT_SET_MIN_FREQ,
> >> + MEMLAT_SET_MAX_FREQ,
> >> + MEMLAT_GET_CUR_FREQ,
> >> + MEMLAT_START_TIMER,
> >> + MEMLAT_STOP_TIMER,
> >> + MEMLAT_GET_TIMESTAMP,
> >> + MEMLAT_MAX_MSG
> >> +};
> >> +
> >> +struct map_table {
> >> + u16 v1;
> >> + u16 v2;
> >> +};
> >> +
> >
> > Any documentation for these structures? It won't bite you, but it will
> > help reviewers and other developers.
>
> ack
>
> >
> >> +struct map_param_msg {
> >> + u32 hw_type;
> >> + u32 mon_idx;
> >> + u32 nr_rows;
> >> + struct map_table tbl[MAX_MAP_ENTRIES];
> >> +} __packed;
> >> +
> >> +struct node_msg {
> >> + u32 cpumask;
> >> + u32 hw_type;
> >> + u32 mon_type;
> >> + u32 mon_idx;
> >> + char mon_name[MAX_NAME_LEN];
> >> +};
> >> +
> >> +struct scalar_param_msg {
> >> + u32 hw_type;
> >> + u32 mon_idx;
> >> + u32 val;
> >> +};
> >> +
> >> +enum common_ev_idx {
> >> + INST_IDX,
> >> + CYC_IDX,
> >> + FE_STALL_IDX,
> >> + BE_STALL_IDX,
> >> + NUM_COMMON_EVS
> >> +};
> >> +
> >> +enum grp_ev_idx {
> >> + MISS_IDX,
> >> + WB_IDX,
> >> + ACC_IDX,
> >> + NUM_GRP_EVS
> >> +};
> >> +
> >> +#define EV_CPU_CYCLES 0
> >> +#define EV_INST_RETIRED 2
> >> +#define EV_L2_D_RFILL 5
> >> +
> >> +struct ev_map_msg {
> >> + u32 num_evs;
> >> + u32 hw_type;
> >> + u32 cid[NUM_COMMON_EVS];
> >> +};
> >> +
> >> +struct cpufreq_memfreq_map {
> >> + unsigned int cpufreq_mhz;
> >> + unsigned int memfreq_khz;
> >> +};
> >> +
> >> +struct scmi_monitor_info {
> >> + struct cpufreq_memfreq_map *freq_map;
> >> + char mon_name[MAX_NAME_LEN];
> >> + u32 mon_idx;
> >> + u32 mon_type;
> >> + u32 ipm_ceil;
> >> + u32 mask;
> >> + u32 freq_map_len;
> >> +};
> >> +
> >> +struct scmi_memory_info {
> >> + struct scmi_monitor_info *monitor[MAX_MONITOR_CNT];
> >> + u32 hw_type;
> >> + int monitor_cnt;
> >> + u32 min_freq;
> >> + u32 max_freq;
> >> +};
> >> +
> >> +struct scmi_memlat_info {
> >> + struct scmi_protocol_handle *ph;
> >> + const struct qcom_scmi_vendor_ops *ops;
> >> + struct scmi_memory_info *memory[MAX_MEMORY_TYPES];
> >> + int memory_cnt;
> >> +};
> >> +
> >> +static int get_mask(struct device_node *np, u32 *mask)
> >> +{
> >> + struct device_node *dev_phandle;
> >> + struct device *cpu_dev;
> >> + int cpu, i = 0;
> >> + int ret = -ENODEV;
> >> +
> >> + dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> >> + while (dev_phandle) {
> >> + for_each_possible_cpu(cpu) {
> >> + cpu_dev = get_cpu_device(cpu);
> >> + if (cpu_dev && cpu_dev->of_node == dev_phandle) {
> >> + *mask |= BIT(cpu);
> >> + ret = 0;
> >> + break;
> >> + }
> >> + }
> >
> > There is of_cpu_node_to_id(). No need to reinvent it.
>
> ack
>
> >
> >> + dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static struct cpufreq_memfreq_map *init_cpufreq_memfreq_map(struct device *dev,
> >> + struct device_node *of_node,
> >> + u32 *cnt)
> >> +{
> >> + int len, nf, i, j;
> >> + u32 data;
> >> + struct cpufreq_memfreq_map *tbl;
> >> + int ret;
> >> +
> >> + if (!of_find_property(of_node, "qcom,cpufreq-memfreq-tbl", &len))
> >> + return NULL;
> >> + len /= sizeof(data);
> >> +
> >> + if (len % 2 || len == 0)
> >> + return NULL;
> >> + nf = len / 2;
> >> +
> >> + tbl = devm_kzalloc(dev, (nf + 1) * sizeof(struct cpufreq_memfreq_map),
> >> + GFP_KERNEL);
> >> + if (!tbl)
> >> + return NULL;
> >> +
> >> + for (i = 0, j = 0; i < nf; i++, j += 2) {
> >> + ret = of_property_read_u32_index(of_node, "qcom,cpufreq-memfreq-tbl",
> >> + j, &data);
> >> + if (ret < 0)
> >> + return NULL;
> >> + tbl[i].cpufreq_mhz = data / 1000;
> >> +
> >> + ret = of_property_read_u32_index(of_node, "qcom,cpufreq-memfreq-tbl",
> >> + j + 1, &data);
> >> + if (ret < 0)
> >> + return NULL;
> >> +
> >> + tbl[i].memfreq_khz = data;
> >> + pr_debug("Entry%d CPU:%u, Mem:%u\n", i, tbl[i].cpufreq_mhz,
> >> + tbl[i].memfreq_khz);
> >> + }
> >> + *cnt = nf;
> >> + tbl[i].cpufreq_mhz = 0;
> >
> > This looks like a lame version of the OPP table.
>
> I didn't know listing multiple frequencies in a opp was allowed. We can
> probably get away with it here since we just parse the data here and not
> populate data in the opp core.
You are describing driver behaviour. DT describes hardware. So, the
proper way to describe this kind of data is the OPP table.
--
With best wishes
Dmitry
On Wed, Jan 17, 2024 at 11:04:54PM +0530, Sibi Sankar wrote:
> From: Shivnandan Kumar <[email protected]>
>
> 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 <[email protected]>
> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
> Co-developed-by: Amir Vajid <[email protected]>
> Signed-off-by: Amir Vajid <[email protected]>
> Co-developed-by: Sibi Sankar <[email protected]>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
> 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 Wed, Jan 17, 2024 at 11:04:51PM +0530, Sibi Sankar wrote:
> This patch series introduces the Qualcomm SCMI Vendor protocol and adds a
> client driver that interacts with the vendor protocol and passes on the
> required tuneables to start various features running on the SCMI controller.
>
Hi Sibi,
> The series specifically enables (LLCC/DDR) dvfs on X1E80100 SoC by passing
> several tuneables including the IPM ratio (Instructions Per Miss),
> cpu frequency to memory/bus frequency tables, CPU mapping to the vendor
> protocol which in turn will enable the memory latency governor running
> on the SCMI controller.
>
As a side note, before I forget (and I got lost again searching for this
thread), next time you post this, please CC also linux-arm-kernel, being
this series SCMI-related this way can be seen by other non-MSM/QC
SCMI-interested people. (as it is advised by get_maintainer.pl too)
Thanks,
Cristian
On Mon, Feb 12, 2024 at 03:54:27PM +0530, Sibi Sankar wrote:
>
>
> On 1/18/24 02:11, Dmitry Baryshkov wrote:
> > On Wed, 17 Jan 2024 at 19:36, Sibi Sankar <[email protected]> wrote:
Hi,
I'll comment this patch fully, just a remark down below about this
mail-thread.
> > >
> > > From: Shivnandan Kumar <[email protected]>
> > >
> > > This patch introduces a client driver that interacts with the SCMI QCOM
> >
> > git grep This.patch Documentation/process/
> >
> > > vendor protocol and passes on the required tuneables to start various
> > > features running on the SCMI controller.
> >
> > Is there any word about the 'memlat'? No. Unless one reads into the
> > patch, one can not come up with the idea of what is being introduced.
>
> ack, will fix it in the re-spin.
>
> >
> > >
> > > Signed-off-by: Shivnandan Kumar <[email protected]>
> > > Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
> > > Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
> > > Co-developed-by: Amir Vajid <[email protected]>
> > > Signed-off-by: Amir Vajid <[email protected]>
> > > Co-developed-by: Sibi Sankar <[email protected]>
> > > Signed-off-by: Sibi Sankar <[email protected]>
> > > ---
> > > drivers/soc/qcom/Kconfig | 10 +
> > > drivers/soc/qcom/Makefile | 1 +
> > > drivers/soc/qcom/qcom_scmi_client.c | 486 ++++++++++++++++++++++++++++
> >
> > Should it go to drivers/firmware/arm_scmi instead? Or maybe to drivers/devfreq?
>
> I don't think it should go into arm_scmi unless Sudeep wants it there.
> As for drivers/devfreq, I would have moved it there if this driver
> benfitted being classified as a devfreq device. We can't use any of
> the available governors on it and the tuneables appear way too custom.
> These are the reasons why I placed it in drivers/soc/qcom instead.
>
I think we used to host a couple of generic SCMI driver related to
standard protocols but they have been moved out of driver/firmware/arm_scmi
into the related subsystem...not sure if Sudeep thinks otherwise but I
suppose we want to host only SCMI drivers that are clearly lacking a
place where to stay...
> >
> > > 3 files changed, 497 insertions(+)
> > > create mode 100644 drivers/soc/qcom/qcom_scmi_client.c
[snip]
> > > +static int configure_cpucp_mon(struct scmi_memlat_info *info, int memory_index, int monitor_index)
> > > +{
> > > + const struct qcom_scmi_vendor_ops *ops = info->ops;
> > > + struct scmi_memory_info *memory = info->memory[memory_index];
> > > + struct scmi_monitor_info *monitor = memory->monitor[monitor_index];
> > > + struct scalar_param_msg scalar_msg;
> > > + struct map_param_msg map_msg;
> > > + struct node_msg msg;
> > > + int ret;
> > > + int i;
> > > +
> > > + msg.cpumask = monitor->mask;
> > > + msg.hw_type = memory->hw_type;
> > > + msg.mon_type = monitor->mon_type;
> > > + msg.mon_idx = monitor->mon_idx;
> > > + strscpy(msg.mon_name, monitor->mon_name, sizeof(msg.mon_name));
> > > + ret = ops->set_param(info->ph, &msg, MEMLAT_ALGO_STR, MEMLAT_SET_MONITOR, sizeof(msg));
> > > + if (ret < 0) {
> > > + pr_err("Failed to configure monitor %s\n", monitor->mon_name);
> > > + return ret;
> > > + }
> > > +
> > > + scalar_msg.hw_type = memory->hw_type;
> > > + scalar_msg.mon_idx = monitor->mon_idx;
> > > + scalar_msg.val = monitor->ipm_ceil;
> > > + ret = ops->set_param(info->ph, &scalar_msg, MEMLAT_ALGO_STR, MEMLAT_IPM_CEIL,
> > > + sizeof(scalar_msg));
> > > + if (ret < 0) {
> > > + pr_err("Failed to set ipm ceil for %s\n", monitor->mon_name);
> > > + return ret;
> > > + }
> > > +
> > > + map_msg.hw_type = memory->hw_type;
> > > + map_msg.mon_idx = monitor->mon_idx;
> > > + map_msg.nr_rows = monitor->freq_map_len;
> > > + for (i = 0; i < monitor->freq_map_len; i++) {
> > > + map_msg.tbl[i].v1 = monitor->freq_map[i].cpufreq_mhz;
> > > + map_msg.tbl[i].v2 = monitor->freq_map[i].memfreq_khz / 1000;
> > > + }
> >
> > So this table goes 1:1 to the firmware? Is it going to be the same for
> > all versions of the SoC? If so, it might be better to turn it into the
> > static data inside the driver. If it doesn't change, there is no need
> > to put it to DT.
>
> The table does go directly to the firmware but obviously varies across
> SoCs. Also since it's a SCMI client driver we don't have a way to
> distinguish between SoCs based on compatibles. So it made more sense to
> move it to the device tree instead.
>
Well, the SCMI fw running the server DOES know where it is running right ?
So, if you have multiple fixed config tables to feed into the firmware
that vary based on the SoC you are running on, you could add an SCMI command
to your QCOM SCMI vendor protocol and expose a related operation in ops to get
the actual SoC model, so that you can embed the tableS in the driver here (as
suggested) and then choose at runtime which one to use based on the reported
platform...this is clearly config stuff (sa said by others) so it just
does not belong to DT descriptions.
Thanks,
Cristian
On Wed, Jan 17, 2024 at 11:04:55PM +0530, Sibi Sankar wrote:
> From: Shivnandan Kumar <[email protected]>
Hi
>
> This patch introduces a client driver that interacts with the SCMI QCOM
> vendor protocol and passes on the required tuneables to start various
> features running on the SCMI controller.
>
> Signed-off-by: Shivnandan Kumar <[email protected]>
> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
> Co-developed-by: Amir Vajid <[email protected]>
> Signed-off-by: Amir Vajid <[email protected]>
> Co-developed-by: Sibi Sankar <[email protected]>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
> drivers/soc/qcom/Kconfig | 10 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/qcom_scmi_client.c | 486 ++++++++++++++++++++++++++++
> 3 files changed, 497 insertions(+)
> create mode 100644 drivers/soc/qcom/qcom_scmi_client.c
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index c6ca4de42586..1530558aebfb 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -264,6 +264,16 @@ config QCOM_ICC_BWMON
> the fixed bandwidth votes from cpufreq (CPU nodes) thus achieve high
> memory throughput even with lower CPU frequencies.
>
> +config QCOM_SCMI_CLIENT
> + tristate "Qualcomm Technologies Inc. SCMI client driver"
> + depends on QCOM_SCMI_VENDOR_PROTOCOL || COMPILE_TEST
> + default n
> + help
> + SCMI client driver registers for SCMI QCOM vendor protocol.
> +
> + This driver interacts with the vendor protocol and passes on the required
> + tuneables to start various features running on the SCMI controller.
> +
> config QCOM_INLINE_CRYPTO_ENGINE
> tristate
> select QCOM_SCM
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 05b3d54e8dc9..c2a51293c886 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -32,5 +32,6 @@ obj-$(CONFIG_QCOM_APR) += apr.o
> obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
> obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
> obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
> +obj-$(CONFIG_QCOM_SCMI_CLIENT) += qcom_scmi_client.o
> qcom_ice-objs += ice.o
> obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom_ice.o
> diff --git a/drivers/soc/qcom/qcom_scmi_client.c b/drivers/soc/qcom/qcom_scmi_client.c
> new file mode 100644
> index 000000000000..418aa7900496
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom_scmi_client.c
> @@ -0,0 +1,486 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/cpu.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/qcom_scmi_vendor.h>
> +#include <linux/scmi_protocol.h>
> +
> +#define MAX_MEMORY_TYPES 3
> +#define MEMLAT_ALGO_STR 0x74616C6D656D /* "memlat" */
> +#define INVALID_IDX 0xFF
> +#define MAX_NAME_LEN 20
> +#define MAX_MAP_ENTRIES 6
> +#define MAX_MONITOR_CNT 4
> +#define SCMI_VENDOR_MSG_START 3
> +#define SCMI_VENDOR_MSG_MODULE_START 16
> +
> +enum scmi_memlat_protocol_cmd {
> + MEMLAT_SET_LOG_LEVEL = SCMI_VENDOR_MSG_START,
> + MEMLAT_FLUSH_LOGBUF,
> + MEMLAT_SET_MEM_GROUP = SCMI_VENDOR_MSG_MODULE_START,
> + MEMLAT_SET_MONITOR,
> + MEMLAT_SET_COMMON_EV_MAP,
> + MEMLAT_SET_GRP_EV_MAP,
> + MEMLAT_ADAPTIVE_LOW_FREQ,
> + MEMLAT_ADAPTIVE_HIGH_FREQ,
> + MEMLAT_GET_ADAPTIVE_CUR_FREQ,
> + MEMLAT_IPM_CEIL,
> + MEMLAT_FE_STALL_FLOOR,
> + MEMLAT_BE_STALL_FLOOR,
> + MEMLAT_WB_PCT,
> + MEMLAT_IPM_FILTER,
> + MEMLAT_FREQ_SCALE_PCT,
> + MEMLAT_FREQ_SCALE_CEIL_MHZ,
> + MEMLAT_FREQ_SCALE_FLOOR_MHZ,
> + MEMLAT_SAMPLE_MS,
> + MEMLAT_MON_FREQ_MAP,
> + MEMLAT_SET_MIN_FREQ,
> + MEMLAT_SET_MAX_FREQ,
> + MEMLAT_GET_CUR_FREQ,
> + MEMLAT_START_TIMER,
> + MEMLAT_STOP_TIMER,
> + MEMLAT_GET_TIMESTAMP,
> + MEMLAT_MAX_MSG
> +};
So the reason why you can have just a single qualcomm vendor SCMI protocol is
that it really implements and expose just a couple of set/get generic commands
and exposes a few related ops so that you can piggyback any kind of real messages
into this new sort of transport layer and at the end the full final message payloads
are built here in the client driver...and any future further QC SCMI client driver
will implement its own set of payloads for different protocols.
Seems a bit odd to me (but certainly a creative way to abuse the SCMI stack), anyway
I have personally nothing against it, if you are happy with this design, but the spec
says that the protocol messages have to be little endian-encoded so I suppose that you
should, down below, define your smuggled (:P) payloads with __le32 & friends and use
the proper helpers to be sure that the values tranferred are properly interpreted, from
the endianess point-of-view, on both sides of the channel.
..but Sudeep could think differently...I would wait for his feedback...
> +
> +struct map_table {
> + u16 v1;
> + u16 v2;
> +};
> +
> +struct map_param_msg {
> + u32 hw_type;
> + u32 mon_idx;
> + u32 nr_rows;
> + struct map_table tbl[MAX_MAP_ENTRIES];
> +} __packed;
> +
> +struct node_msg {
> + u32 cpumask;
> + u32 hw_type;
> + u32 mon_type;
> + u32 mon_idx;
> + char mon_name[MAX_NAME_LEN];
> +};
> +
> +struct scalar_param_msg {
> + u32 hw_type;
> + u32 mon_idx;
> + u32 val;
> +};
> +
> +enum common_ev_idx {
> + INST_IDX,
> + CYC_IDX,
> + FE_STALL_IDX,
> + BE_STALL_IDX,
> + NUM_COMMON_EVS
> +};
> +
> +enum grp_ev_idx {
> + MISS_IDX,
> + WB_IDX,
> + ACC_IDX,
> + NUM_GRP_EVS
> +};
> +
> +#define EV_CPU_CYCLES 0
> +#define EV_INST_RETIRED 2
> +#define EV_L2_D_RFILL 5
> +
> +struct ev_map_msg {
> + u32 num_evs;
> + u32 hw_type;
> + u32 cid[NUM_COMMON_EVS];
> +};
> +
> +struct cpufreq_memfreq_map {
> + unsigned int cpufreq_mhz;
> + unsigned int memfreq_khz;
> +};
> +
> +struct scmi_monitor_info {
> + struct cpufreq_memfreq_map *freq_map;
> + char mon_name[MAX_NAME_LEN];
> + u32 mon_idx;
> + u32 mon_type;
> + u32 ipm_ceil;
> + u32 mask;
> + u32 freq_map_len;
> +};
> +
> +struct scmi_memory_info {
> + struct scmi_monitor_info *monitor[MAX_MONITOR_CNT];
> + u32 hw_type;
> + int monitor_cnt;
> + u32 min_freq;
> + u32 max_freq;
> +};
> +
> +struct scmi_memlat_info {
> + struct scmi_protocol_handle *ph;
> + const struct qcom_scmi_vendor_ops *ops;
> + struct scmi_memory_info *memory[MAX_MEMORY_TYPES];
> + int memory_cnt;
> +};
> +
> +static int get_mask(struct device_node *np, u32 *mask)
> +{
> + struct device_node *dev_phandle;
> + struct device *cpu_dev;
> + int cpu, i = 0;
> + int ret = -ENODEV;
> +
> + dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> + while (dev_phandle) {
> + for_each_possible_cpu(cpu) {
> + cpu_dev = get_cpu_device(cpu);
> + if (cpu_dev && cpu_dev->of_node == dev_phandle) {
> + *mask |= BIT(cpu);
> + ret = 0;
> + break;
> + }
> + }
> + dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> + }
> +
> + return ret;
> +}
> +
> +static struct cpufreq_memfreq_map *init_cpufreq_memfreq_map(struct device *dev,
> + struct device_node *of_node,
> + u32 *cnt)
> +{
> + int len, nf, i, j;
> + u32 data;
> + struct cpufreq_memfreq_map *tbl;
> + int ret;
> +
> + if (!of_find_property(of_node, "qcom,cpufreq-memfreq-tbl", &len))
> + return NULL;
> + len /= sizeof(data);
> +
> + if (len % 2 || len == 0)
> + return NULL;
> + nf = len / 2;
> +
> + tbl = devm_kzalloc(dev, (nf + 1) * sizeof(struct cpufreq_memfreq_map),
> + GFP_KERNEL);
> + if (!tbl)
> + return NULL;
> +
> + for (i = 0, j = 0; i < nf; i++, j += 2) {
> + ret = of_property_read_u32_index(of_node, "qcom,cpufreq-memfreq-tbl",
> + j, &data);
> + if (ret < 0)
> + return NULL;
Bailing out here and down below you are not releasing the devm_
allocated memory AND what is worst is that the caller of this function
does not check either for a NULL return value (see below)
> + tbl[i].cpufreq_mhz = data / 1000;
> +
> + ret = of_property_read_u32_index(of_node, "qcom,cpufreq-memfreq-tbl",
> + j + 1, &data);
> + if (ret < 0)
> + return NULL;
> +
Ditto.
> + tbl[i].memfreq_khz = data;
> + pr_debug("Entry%d CPU:%u, Mem:%u\n", i, tbl[i].cpufreq_mhz,
> + tbl[i].memfreq_khz);
> + }
> + *cnt = nf;
> + tbl[i].cpufreq_mhz = 0;
> +
> + return tbl;
> +}
> +
> +static int process_scmi_memlat_of_node(struct scmi_device *sdev, struct scmi_memlat_info *info)
> +{
> + struct device_node *memlat_np, *memory_np, *monitor_np;
> + struct scmi_memory_info *memory;
> + struct scmi_monitor_info *monitor;
> + int ret = 0, i = 0, j;
> + u32 memfreq[2];
> +
> + of_node_get(sdev->handle->dev->of_node);
> + memlat_np = of_find_node_by_name(sdev->handle->dev->of_node, "memlat");
> +
> + info->memory_cnt = of_get_child_count(memlat_np);
> + if (info->memory_cnt <= 0)
> + pr_err("No memory nodes present\n");
> +
> + for_each_child_of_node(memlat_np, memory_np) {
> + memory = devm_kzalloc(&sdev->dev, sizeof(*memory), GFP_KERNEL);
> + if (!memory) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + ret = of_property_read_u32(memory_np, "reg", &memory->hw_type);
> + if (ret) {
No devm_free see below.
> + pr_err("Failed to read memory type\n");
> + goto err;
> + }
> +
> + memory->monitor_cnt = of_get_child_count(memory_np);
> + if (memory->monitor_cnt <= 0) {
> + pr_err("No monitor nodes present\n");
> + ret = -EINVAL;
No devm_free see below.
> + goto err;
> + }
> +
> + ret = of_property_read_u32_array(memory_np, "freq-table-khz", memfreq, 2);
> + if (ret && (ret != -EINVAL)) {
> + pr_err("Failed to read min/max freq %d\n", ret);
No devm_free see below.
> + goto err;
> + }
> +
> + memory->min_freq = memfreq[0];
> + memory->max_freq = memfreq[1];
> + info->memory[i] = memory;
> + j = 0;
> + i++;
> +
> + for_each_child_of_node(memory_np, monitor_np) {
> + monitor = devm_kzalloc(&sdev->dev, sizeof(*monitor), GFP_KERNEL);
> + if (!monitor) {
> + ret = -ENOMEM;
No devm_free see below.
> + goto err;
> + }
> +
> + monitor->mon_type = (of_property_read_bool(monitor_np, "qcom,compute-mon")) ? 1 : 0;
> + monitor->ipm_ceil = (of_property_read_bool(monitor_np, "qcom,compute-mon")) ? 0 : 20000000;
> +
> + if (get_mask(monitor_np, &monitor->mask)) {
> + pr_err("Failed to populate cpu mask %d\n", ret);
No devm_free see below.
> + goto err;
> + }
> +
> + monitor->freq_map = init_cpufreq_memfreq_map(&sdev->dev, monitor_np,
> + &monitor->freq_map_len);
Return can be NULL here and it is not checked, but seems to me that is
then accessed without any NULL-check....
> + snprintf(monitor->mon_name, MAX_NAME_LEN, "monitor-%d", j);
> + monitor->mon_idx = j;
> +
> + memory->monitor[j] = monitor;
> + j++;
> + }
> + }
> +
> + return 0;
> +
> +err:
> + of_node_put(memlat_np);
> +
Here you bailout with an -ENOMEM or other errors but without releasing devm_
allocated stuff AND in the caller of this you carry on despite any error and
you neither did release this memory in the caller, so this unused mem-areas
will be eventually freed only on driver removal.
> + return ret;
> +}
> +
> +static int configure_cpucp_common_events(struct scmi_memlat_info *info)
> +{
> + const struct qcom_scmi_vendor_ops *ops = info->ops;
> + u8 ev_map[NUM_COMMON_EVS];
> + struct ev_map_msg msg;
> + int ret;
> +
> + memset(ev_map, 0xFF, NUM_COMMON_EVS);
> +
> + msg.num_evs = NUM_COMMON_EVS;
> + msg.hw_type = INVALID_IDX;
> + msg.cid[INST_IDX] = EV_INST_RETIRED;
> + msg.cid[CYC_IDX] = EV_CPU_CYCLES;
> + msg.cid[FE_STALL_IDX] = INVALID_IDX;
> + msg.cid[BE_STALL_IDX] = INVALID_IDX;
> +
> + ret = ops->set_param(info->ph, &msg, MEMLAT_ALGO_STR, MEMLAT_SET_COMMON_EV_MAP,
> + sizeof(msg));
> + return ret;
> +}
> +
> +static int configure_cpucp_grp(struct scmi_memlat_info *info, int memory_index)
> +{
> + const struct qcom_scmi_vendor_ops *ops = info->ops;
> + struct scmi_memory_info *memory = info->memory[memory_index];
> + struct ev_map_msg ev_msg;
> + u8 ev_map[NUM_GRP_EVS];
> + struct node_msg msg;
> + int ret;
> +
> + msg.cpumask = 0;
> + msg.hw_type = memory->hw_type;
> + msg.mon_type = 0;
> + msg.mon_idx = 0;
> + ret = ops->set_param(info->ph, &msg, MEMLAT_ALGO_STR, MEMLAT_SET_MEM_GROUP, sizeof(msg));
> + if (ret < 0) {
> + pr_err("Failed to configure mem type %d\n", memory->hw_type);
> + return ret;
> + }
> +
> + memset(ev_map, 0xFF, NUM_GRP_EVS);
> + ev_msg.num_evs = NUM_GRP_EVS;
> + ev_msg.hw_type = memory->hw_type;
> + ev_msg.cid[MISS_IDX] = EV_L2_D_RFILL;
> + ev_msg.cid[WB_IDX] = INVALID_IDX;
> + ev_msg.cid[ACC_IDX] = INVALID_IDX;
> + ret = ops->set_param(info->ph, &ev_msg, MEMLAT_ALGO_STR, MEMLAT_SET_GRP_EV_MAP,
> + sizeof(ev_msg));
> + if (ret < 0) {
> + pr_err("Failed to configure event map for mem type %d\n", memory->hw_type);
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +static int configure_cpucp_mon(struct scmi_memlat_info *info, int memory_index, int monitor_index)
> +{
> + const struct qcom_scmi_vendor_ops *ops = info->ops;
> + struct scmi_memory_info *memory = info->memory[memory_index];
> + struct scmi_monitor_info *monitor = memory->monitor[monitor_index];
> + struct scalar_param_msg scalar_msg;
> + struct map_param_msg map_msg;
> + struct node_msg msg;
> + int ret;
> + int i;
> +
> + msg.cpumask = monitor->mask;
> + msg.hw_type = memory->hw_type;
> + msg.mon_type = monitor->mon_type;
> + msg.mon_idx = monitor->mon_idx;
> + strscpy(msg.mon_name, monitor->mon_name, sizeof(msg.mon_name));
> + ret = ops->set_param(info->ph, &msg, MEMLAT_ALGO_STR, MEMLAT_SET_MONITOR, sizeof(msg));
> + if (ret < 0) {
> + pr_err("Failed to configure monitor %s\n", monitor->mon_name);
> + return ret;
> + }
> +
> + scalar_msg.hw_type = memory->hw_type;
> + scalar_msg.mon_idx = monitor->mon_idx;
> + scalar_msg.val = monitor->ipm_ceil;
> + ret = ops->set_param(info->ph, &scalar_msg, MEMLAT_ALGO_STR, MEMLAT_IPM_CEIL,
> + sizeof(scalar_msg));
> + if (ret < 0) {
> + pr_err("Failed to set ipm ceil for %s\n", monitor->mon_name);
> + return ret;
> + }
> +
> + map_msg.hw_type = memory->hw_type;
> + map_msg.mon_idx = monitor->mon_idx;
> + map_msg.nr_rows = monitor->freq_map_len;
> + for (i = 0; i < monitor->freq_map_len; i++) {
> + map_msg.tbl[i].v1 = monitor->freq_map[i].cpufreq_mhz;
> + map_msg.tbl[i].v2 = monitor->freq_map[i].memfreq_khz / 1000;
> + }
> + ret = ops->set_param(info->ph, &map_msg, MEMLAT_ALGO_STR, MEMLAT_MON_FREQ_MAP,
> + sizeof(map_msg));
> + if (ret < 0) {
> + pr_err("Failed to configure freq_map for %s\n", monitor->mon_name);
> + return ret;
> + }
> +
> + scalar_msg.hw_type = memory->hw_type;
> + scalar_msg.mon_idx = monitor->mon_idx;
> + scalar_msg.val = memory->min_freq;
> + ret = ops->set_param(info->ph, &scalar_msg, MEMLAT_ALGO_STR, MEMLAT_SET_MIN_FREQ,
> + sizeof(scalar_msg));
> + if (ret < 0) {
> + pr_err("Failed to set min_freq for %s\n", monitor->mon_name);
> + return ret;
> + }
> +
> + scalar_msg.hw_type = memory->hw_type;
> + scalar_msg.mon_idx = monitor->mon_idx;
> + scalar_msg.val = memory->max_freq;
> + ret = ops->set_param(info->ph, &scalar_msg, MEMLAT_ALGO_STR, MEMLAT_SET_MAX_FREQ,
> + sizeof(scalar_msg));
> + if (ret < 0)
> + pr_err("Failed to set max_freq for %s\n", monitor->mon_name);
> +
> + return ret;
> +}
> +
> +static int cpucp_memlat_init(struct scmi_device *sdev)
> +{
> + const struct scmi_handle *handle = sdev->handle;
> + const struct qcom_scmi_vendor_ops *ops;
> + struct scmi_protocol_handle *ph;
> + struct scmi_memlat_info *info;
> + u32 cpucp_sample_ms = 8;
> + int ret, i, j;
> +
> + if (!handle)
> + return -ENODEV;
> +
> + ops = handle->devm_protocol_get(sdev, QCOM_SCMI_VENDOR_PROTOCOL, &ph);
> + if (IS_ERR(ops))
> + return PTR_ERR(ops);
> +
> + info = devm_kzalloc(&sdev->dev, sizeof(*info), GFP_KERNEL);
> + if (!info)
> + return -ENOMEM;
> +
> + ret = process_scmi_memlat_of_node(sdev, info);
> + if (ret)
> + pr_err("Failed to configure common events: %d\n", ret);
Carry on without cleaning up stuff allocated by process_scmi_memlat_of_node()
Thanks,
Cristian
On 2/20/24 20:37, Cristian Marussi wrote:
> On Mon, Feb 12, 2024 at 03:54:27PM +0530, Sibi Sankar wrote:
>>
>>
>> On 1/18/24 02:11, Dmitry Baryshkov wrote:
>>> On Wed, 17 Jan 2024 at 19:36, Sibi Sankar <[email protected]> wrote:
>
> Hi,
>
> I'll comment this patch fully, just a remark down below about this
> mail-thread.
>
>>>>
>>>> From: Shivnandan Kumar <[email protected]>
>>>>
>>>> This patch introduces a client driver that interacts with the SCMI QCOM
>>>
>>> git grep This.patch Documentation/process/
>>>
>>>> vendor protocol and passes on the required tuneables to start various
>>>> features running on the SCMI controller.
>>>
>>> Is there any word about the 'memlat'? No. Unless one reads into the
>>> patch, one can not come up with the idea of what is being introduced.
>>
>> ack, will fix it in the re-spin.
>>
>>>
>>>>
>>>> Signed-off-by: Shivnandan Kumar <[email protected]>
>>>> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
>>>> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
>>>> Co-developed-by: Amir Vajid <[email protected]>
>>>> Signed-off-by: Amir Vajid <[email protected]>
>>>> Co-developed-by: Sibi Sankar <[email protected]>
>>>> Signed-off-by: Sibi Sankar <[email protected]>
>>>> ---
>>>> drivers/soc/qcom/Kconfig | 10 +
>>>> drivers/soc/qcom/Makefile | 1 +
>>>> drivers/soc/qcom/qcom_scmi_client.c | 486 ++++++++++++++++++++++++++++
>>>
>>> Should it go to drivers/firmware/arm_scmi instead? Or maybe to drivers/devfreq?
>>
>> I don't think it should go into arm_scmi unless Sudeep wants it there.
>> As for drivers/devfreq, I would have moved it there if this driver
>> benfitted being classified as a devfreq device. We can't use any of
>> the available governors on it and the tuneables appear way too custom.
>> These are the reasons why I placed it in drivers/soc/qcom instead.
>>
>
> I think we used to host a couple of generic SCMI driver related to
> standard protocols but they have been moved out of driver/firmware/arm_scmi
> into the related subsystem...not sure if Sudeep thinks otherwise but I
> suppose we want to host only SCMI drivers that are clearly lacking a
> place where to stay...
I see and it makes sense, since it would go through an additional layer
of review from the corresponding sub-system maintainers and only after
sufficient push back it gets to land in driver/firmware/arm_scmi.
>
>>>
>>>> 3 files changed, 497 insertions(+)
>>>> create mode 100644 drivers/soc/qcom/qcom_scmi_client.c
>
> [snip]
>
>>>> +static int configure_cpucp_mon(struct scmi_memlat_info *info, int memory_index, int monitor_index)
>>>> +{
>>>> + const struct qcom_scmi_vendor_ops *ops = info->ops;
>>>> + struct scmi_memory_info *memory = info->memory[memory_index];
>>>> + struct scmi_monitor_info *monitor = memory->monitor[monitor_index];
>>>> + struct scalar_param_msg scalar_msg;
>>>> + struct map_param_msg map_msg;
>>>> + struct node_msg msg;
>>>> + int ret;
>>>> + int i;
>>>> +
>>>> + msg.cpumask = monitor->mask;
>>>> + msg.hw_type = memory->hw_type;
>>>> + msg.mon_type = monitor->mon_type;
>>>> + msg.mon_idx = monitor->mon_idx;
>>>> + strscpy(msg.mon_name, monitor->mon_name, sizeof(msg.mon_name));
>>>> + ret = ops->set_param(info->ph, &msg, MEMLAT_ALGO_STR, MEMLAT_SET_MONITOR, sizeof(msg));
>>>> + if (ret < 0) {
>>>> + pr_err("Failed to configure monitor %s\n", monitor->mon_name);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + scalar_msg.hw_type = memory->hw_type;
>>>> + scalar_msg.mon_idx = monitor->mon_idx;
>>>> + scalar_msg.val = monitor->ipm_ceil;
>>>> + ret = ops->set_param(info->ph, &scalar_msg, MEMLAT_ALGO_STR, MEMLAT_IPM_CEIL,
>>>> + sizeof(scalar_msg));
>>>> + if (ret < 0) {
>>>> + pr_err("Failed to set ipm ceil for %s\n", monitor->mon_name);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + map_msg.hw_type = memory->hw_type;
>>>> + map_msg.mon_idx = monitor->mon_idx;
>>>> + map_msg.nr_rows = monitor->freq_map_len;
>>>> + for (i = 0; i < monitor->freq_map_len; i++) {
>>>> + map_msg.tbl[i].v1 = monitor->freq_map[i].cpufreq_mhz;
>>>> + map_msg.tbl[i].v2 = monitor->freq_map[i].memfreq_khz / 1000;
>>>> + }
>>>
>>> So this table goes 1:1 to the firmware? Is it going to be the same for
>>> all versions of the SoC? If so, it might be better to turn it into the
>>> static data inside the driver. If it doesn't change, there is no need
>>> to put it to DT.
>>
>> The table does go directly to the firmware but obviously varies across
>> SoCs. Also since it's a SCMI client driver we don't have a way to
>> distinguish between SoCs based on compatibles. So it made more sense to
>> move it to the device tree instead.
>>
>
> Well, the SCMI fw running the server DOES know where it is running right ?
>
> So, if you have multiple fixed config tables to feed into the firmware
> that vary based on the SoC you are running on, you could add an SCMI command
> to your QCOM SCMI vendor protocol and expose a related operation in ops to get
> the actual SoC model, so that you can embed the tableS in the driver here (as
> suggested) and then choose at runtime which one to use based on the reported
> platform...this is clearly config stuff (sa said by others) so it just
> does not belong to DT descriptions.
I still think changing it to opp-tables from qcom,cpufreq-memfreq-tbl
like Konrad/Dmitry suggested is the the right way to go for the
following reason. I would expect the SCP FW running on the SoC to be
board invariant i.e. a SoC can support multiple variants of a memory
that we are trying to scale like ram (lpddr4/lpddr5) so the having them
in the device tree actually allows us to configure the supported ddr
frequencies by overriding those in the board files. Thus closer to the
actual representation. Also like I explained just getting the SoC info
won't be sufficient since the tables are expected to change across
boards when there is a change in type or supported frequencies.
-Sibi
>
> Thanks,
> Cristian
On 1/30/24 18:12, Rob Herring wrote:
> On Wed, Jan 17, 2024 at 11:04:52PM +0530, Sibi Sankar wrote:
>> Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox
>> controller.
>>
>> Signed-off-by: Sibi Sankar <[email protected]>
>> ---
>> .../bindings/mailbox/qcom,cpucp-mbox.yaml | 51 +++++++++++++++++++
>> 1 file changed, 51 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
>> new file mode 100644
>> index 000000000000..2617e5555acb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
>> @@ -0,0 +1,51 @@
>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mailbox/qcom,cpucp-mbox.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Technologies, Inc. CPUCP Mailbox Controller
>> +
>> +maintainers:
>> + - Sibi Sankar <[email protected]>
>> +
>> +description:
>> + The CPUSS Control Processor (CPUCP) mailbox controller enables communication
>> + between AP and CPUCP by acting as a doorbell between them.
>> +
>> +properties:
>> + compatible:
>> + items:
>> + - enum:
>> + - qcom,x1e80100-cpucp-mbox
>> + - const: qcom,cpucp-mbox
>
> A generic fallback implies multiple devices use the same unchanged
> block. That seems doubtful given you have not defined any others and
> given Konrad's comments.
FWIW Sibi and I talked about this a bit off-list, this mailbox is
apparently new and has nothing to do with what I mentioned on other
platforms
Konrad
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.
--
Regards,
Sudeep
On Wed, Jan 17, 2024 at 11:04:54PM +0530, Sibi Sankar wrote:
> From: Shivnandan Kumar <[email protected]>
>
> 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.
--
Regards,
Sudeep
On Mon, Feb 12, 2024 at 03:54:27PM +0530, Sibi Sankar wrote:
>
>
> On 1/18/24 02:11, Dmitry Baryshkov wrote:
> > On Wed, 17 Jan 2024 at 19:36, Sibi Sankar <[email protected]> wrote:
> > >
> > > From: Shivnandan Kumar <[email protected]>
> > >
> > > This patch introduces a client driver that interacts with the SCMI QCOM
> >
> > git grep This.patch Documentation/process/
> >
> > > vendor protocol and passes on the required tuneables to start various
> > > features running on the SCMI controller.
> >
> > Is there any word about the 'memlat'? No. Unless one reads into the
> > patch, one can not come up with the idea of what is being introduced.
>
> ack, will fix it in the re-spin.
>
> >
> > >
> > > Signed-off-by: Shivnandan Kumar <[email protected]>
> > > Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
> > > Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
> > > Co-developed-by: Amir Vajid <[email protected]>
> > > Signed-off-by: Amir Vajid <[email protected]>
> > > Co-developed-by: Sibi Sankar <[email protected]>
> > > Signed-off-by: Sibi Sankar <[email protected]>
> > > ---
> > > drivers/soc/qcom/Kconfig | 10 +
> > > drivers/soc/qcom/Makefile | 1 +
> > > drivers/soc/qcom/qcom_scmi_client.c | 486 ++++++++++++++++++++++++++++
> >
> > Should it go to drivers/firmware/arm_scmi instead? Or maybe to drivers/devfreq?
>
> I don't think it should go into arm_scmi unless Sudeep wants it there.
I won't comment or worry about those silly details yet. I would like to
understand the design better first and all these can be sorted when we
get closer to getting this merged.
--
Regards,
Sudeep
On Tue, Feb 20, 2024 at 04:19:02PM +0000, Cristian Marussi wrote:
> On Wed, Jan 17, 2024 at 11:04:55PM +0530, Sibi Sankar wrote:
> > From: Shivnandan Kumar <[email protected]>
> >
> > This patch introduces a client driver that interacts with the SCMI QCOM
> > vendor protocol and passes on the required tuneables to start various
> > features running on the SCMI controller.
> >
> > Signed-off-by: Shivnandan Kumar <[email protected]>
> > Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
> > Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
> > Co-developed-by: Amir Vajid <[email protected]>
> > Signed-off-by: Amir Vajid <[email protected]>
> > Co-developed-by: Sibi Sankar <[email protected]>
> > Signed-off-by: Sibi Sankar <[email protected]>
> > ---
> > drivers/soc/qcom/Kconfig | 10 +
> > drivers/soc/qcom/Makefile | 1 +
> > drivers/soc/qcom/qcom_scmi_client.c | 486 ++++++++++++++++++++++++++++
> > 3 files changed, 497 insertions(+)
> > create mode 100644 drivers/soc/qcom/qcom_scmi_client.c
> >
> > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> > index c6ca4de42586..1530558aebfb 100644
> > --- a/drivers/soc/qcom/Kconfig
> > +++ b/drivers/soc/qcom/Kconfig
> > @@ -264,6 +264,16 @@ config QCOM_ICC_BWMON
> > the fixed bandwidth votes from cpufreq (CPU nodes) thus achieve high
> > memory throughput even with lower CPU frequencies.
> >
> > +config QCOM_SCMI_CLIENT
> > + tristate "Qualcomm Technologies Inc. SCMI client driver"
> > + depends on QCOM_SCMI_VENDOR_PROTOCOL || COMPILE_TEST
> > + default n
> > + help
> > + SCMI client driver registers for SCMI QCOM vendor protocol.
> > +
> > + This driver interacts with the vendor protocol and passes on the required
> > + tuneables to start various features running on the SCMI controller.
> > +
> > config QCOM_INLINE_CRYPTO_ENGINE
> > tristate
> > select QCOM_SCM
> > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> > index 05b3d54e8dc9..c2a51293c886 100644
> > --- a/drivers/soc/qcom/Makefile
> > +++ b/drivers/soc/qcom/Makefile
> > @@ -32,5 +32,6 @@ obj-$(CONFIG_QCOM_APR) += apr.o
> > obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
> > obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
> > obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
> > +obj-$(CONFIG_QCOM_SCMI_CLIENT) += qcom_scmi_client.o
> > qcom_ice-objs += ice.o
> > obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom_ice.o
> > diff --git a/drivers/soc/qcom/qcom_scmi_client.c b/drivers/soc/qcom/qcom_scmi_client.c
> > new file mode 100644
> > index 000000000000..418aa7900496
> > --- /dev/null
> > +++ b/drivers/soc/qcom/qcom_scmi_client.c
> > @@ -0,0 +1,486 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#include <linux/cpu.h>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/qcom_scmi_vendor.h>
> > +#include <linux/scmi_protocol.h>
> > +
> > +#define MAX_MEMORY_TYPES 3
> > +#define MEMLAT_ALGO_STR 0x74616C6D656D /* "memlat" */
> > +#define INVALID_IDX 0xFF
> > +#define MAX_NAME_LEN 20
> > +#define MAX_MAP_ENTRIES 6
> > +#define MAX_MONITOR_CNT 4
> > +#define SCMI_VENDOR_MSG_START 3
> > +#define SCMI_VENDOR_MSG_MODULE_START 16
> > +
> > +enum scmi_memlat_protocol_cmd {
> > + MEMLAT_SET_LOG_LEVEL = SCMI_VENDOR_MSG_START,
> > + MEMLAT_FLUSH_LOGBUF,
> > + MEMLAT_SET_MEM_GROUP = SCMI_VENDOR_MSG_MODULE_START,
> > + MEMLAT_SET_MONITOR,
> > + MEMLAT_SET_COMMON_EV_MAP,
> > + MEMLAT_SET_GRP_EV_MAP,
> > + MEMLAT_ADAPTIVE_LOW_FREQ,
> > + MEMLAT_ADAPTIVE_HIGH_FREQ,
> > + MEMLAT_GET_ADAPTIVE_CUR_FREQ,
> > + MEMLAT_IPM_CEIL,
> > + MEMLAT_FE_STALL_FLOOR,
> > + MEMLAT_BE_STALL_FLOOR,
> > + MEMLAT_WB_PCT,
> > + MEMLAT_IPM_FILTER,
> > + MEMLAT_FREQ_SCALE_PCT,
> > + MEMLAT_FREQ_SCALE_CEIL_MHZ,
> > + MEMLAT_FREQ_SCALE_FLOOR_MHZ,
> > + MEMLAT_SAMPLE_MS,
> > + MEMLAT_MON_FREQ_MAP,
> > + MEMLAT_SET_MIN_FREQ,
> > + MEMLAT_SET_MAX_FREQ,
> > + MEMLAT_GET_CUR_FREQ,
> > + MEMLAT_START_TIMER,
> > + MEMLAT_STOP_TIMER,
> > + MEMLAT_GET_TIMESTAMP,
> > + MEMLAT_MAX_MSG
> > +};
>
> So the reason why you can have just a single qualcomm vendor SCMI protocol is
> that it really implements and expose just a couple of set/get generic commands
> and exposes a few related ops so that you can piggyback any kind of real messages
> into this new sort of transport layer and at the end the full final message payloads
> are built here in the client driver...and any future further QC SCMI client driver
> will implement its own set of payloads for different protocols.
>
> Seems a bit odd to me (but certainly a creative way to abuse the SCMI stack), anyway
> I have personally nothing against it, if you are happy with this design, but the spec
> says that the protocol messages have to be little endian-encoded so I suppose that you
> should, down below, define your smuggled (:P) payloads with __le32 & friends and use
> the proper helpers to be sure that the values tranferred are properly interpreted, from
> the endianess point-of-view, on both sides of the channel.
>
> ...but Sudeep could think differently...I would wait for his feedback...
No not really. I agree. I have more fundamental question which I expected to
find some reasoning somewhere. It could be me not having followed the patches
properly, but
1. Why this bus/memlat dvfs can't be done using perf protocol ?
2. What std perf protocol lacks that is needed here ?
3. Why it needs to be vendor specific ?
4. If so, why MEMLAT alone doesn't deserve a separate vendor protocol ?
IOW why is vendor protocol is implemented as more generic one for all
qcom needs(I am assuming here) and MEMLAT is just command extension
of that protocol(as Cristian said protocol on top of the generic qcom
vendor protocol)
These are few details that helps to understand and review the patches. So
please add as much as documentation. If it is too much, we can remove it
(but I am sure we may not reach that point).
--
Regards,
Sudeep