[PATCH V6 0/5] qcom: x1e80100: Enable CPUFreq

[PATCH V6 0/5] qcom: x1e80100: Enable CPUFreq

[Sibi Sankar]

This series enables CPUFreq support on the X1E SoC using the SCMI perf
protocol. This was originally part of the RFC: firmware: arm_scmi:
Qualcomm Vendor Protocol [1]. I've split it up so that this part can
land earlier.

V5:
* Fix build error reported by kernel test robot by adding 64BIT requirement
  to COMPILE_TEST
* Pick Rbs

V4:
* Move val, flag and chan to local loop variables. [Jassi]
* Add cpucp mailbox to the MAINTAINERS file. [Jassi]
* Move to core_initcall. [Konrad]
* Skip explicitly setting txdone_irq/txdone_poll to zero. [Konrad]

V3:
* Fix Maintainer info in cpucp mbox bindings. [Bjorn]
* Fix copyright info in cpucp driver. [Bjorn]
* Drop unused APSS_CPUCP_TX_MBOX_IDR, value init and drv_data. [Bjorn/Dmitry]
* Convert to lower case hex. [Bjorn]
* Convert irq and dev to local variables. [Bjorn]
* Replace for and if with for_each_set_bit. [Bjorn]
* Document the need for spinlock. [Bjorn]
* Add space after " for aesthetics. [Bjorn]
* Fix err in calc and add fixes tag. [Bjorn]
* Include io.h and re-order platform_device.h
* Use GENMASK_ULL to generate APSS_CPUCP_RX_MBOX_CMD_MASK.

V2:
* Fix series version number [Rob]
* Pickup Rbs from Dimitry and Rob.
* Use power-domain instead of clocks. [Sudeep/Ulf]
* Rename sram sub-nodes according to schema. [Dmitry]
* Use BIT() instead of manual shift. [Dmitry]
* Define RX_MBOX_CMD to account for chan calculation. [Dmitry]
* Clear the bit instead of the entire status within the spinlock. [Dmitry]
* Use dev_err_probe instead. [Dmitry]
* Drop superfluous error message while handling errors from get_irq. [Dmitry]
* Use devm_mbox_controller_register and drop remove path. [Dmitry]
* Define TX_MBOX_CMD to account for chan calculation.
* Use cpucp->dev in probe path for conformity.

RFC V1:
* Use x1e80100 as the fallback for future SoCs using the cpucp-mbox
  controller. [Krzysztoff/Konrad/Rob]
* Use chan->lock and chan->cl to detect if the channel is no longer
  Available. [Dmitry]
* Use BIT() instead of using manual shifts. [Dmitry]
* Don't use integer as a pointer value. [Dmitry]
* Allow it to default to of_mbox_index_xlate. [Dmitry]
* Use devm_of_iomap. [Dmitry]
* Use module_platform_driver instead of module init/exit. [Dmitry]
* Get channel number using mailbox core (like other drivers) and
  further simplify the driver by dropping setup_mbox func.

[1]: https://lore.kernel.org/lkml/20240117173458.2312669-1-quic_sibis@quicinc.com/#r

Other relevant Links:
https://lore.kernel.org/lkml/be2e475a-349f-4e98-b238-262dd7117a4e@linaro.org/

Sibi Sankar (5):
  dt-bindings: mailbox: qcom: Add CPUCP mailbox controller bindings
  mailbox: Add support for QTI CPUCP mailbox controller
  arm64: dts: qcom: x1e80100: Resize GIC Redistributor register region
  arm64: dts: qcom: x1e80100: Add cpucp mailbox and sram nodes
  arm64: dts: qcom: x1e80100: Enable cpufreq

 .../bindings/mailbox/qcom,cpucp-mbox.yaml     |  49 +++++
 MAINTAINERS                                   |   7 +
 arch/arm64/boot/dts/qcom/x1e80100.dtsi        |  91 ++++++---
 drivers/mailbox/Kconfig                       |   8 +
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/qcom-cpucp-mbox.c             | 187 ++++++++++++++++++
 6 files changed, 319 insertions(+), 25 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
 create mode 100644 drivers/mailbox/qcom-cpucp-mbox.c

-- 
2.34.1

[PATCH V6 1/5] dt-bindings: mailbox: qcom: Add CPUCP mailbox controller bindings

[Sibi Sankar]

Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox
controller.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 .../bindings/mailbox/qcom,cpucp-mbox.yaml     | 49 +++++++++++++++++++
 1 file changed, 49 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..f7342d04beec
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
@@ -0,0 +1,49 @@
+# 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 <quic_sibis@quicinc.com>
+
+description:
+  The CPUSS Control Processor (CPUCP) mailbox controller enables communication
+  between AP and CPUCP by acting as a doorbell between them.
+
+properties:
+  compatible:
+    items:
+      - const: qcom,x1e80100-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";
+        reg = <0x17430000 0x10000>, <0x18830000 0x10000>;
+        interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>;
+        #mbox-cells = <1>;
+    };
-- 
2.34.1

[PATCH V6 2/5] mailbox: Add support for QTI CPUCP mailbox controller

[Sibi Sankar]

Add support for CPUSS Control Processor (CPUCP) mailbox controller,
this driver enables communication between AP and CPUCP by acting as
a doorbell between them.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---

v5:
* Fix build error reported by kernel test robot by adding 64BIT requirement
  to COMPILE_TEST

 MAINTAINERS                       |   7 ++
 drivers/mailbox/Kconfig           |   8 ++
 drivers/mailbox/Makefile          |   2 +
 drivers/mailbox/qcom-cpucp-mbox.c | 187 ++++++++++++++++++++++++++++++
 4 files changed, 204 insertions(+)
 create mode 100644 drivers/mailbox/qcom-cpucp-mbox.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e04f583780c5..d7c00abe2f93 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18533,6 +18533,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/power/avs/qcom,cpr.yaml
 F:	drivers/pmdomain/qcom/cpr.c
 
+QUALCOMM CPUCP MAILBOX DRIVER
+M:	Sibi Sankar <quic_sibis@quicinc.com>
+L:	linux-arm-msm@vger.kernel.org
+S:	Supported
+F:	Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
+F:	drivers/mailbox/qcom-cpucp-mbox.c
+
 QUALCOMM CPUFREQ DRIVER MSM8996/APQ8096
 M:	Ilia Lin <ilia.lin@kernel.org>
 L:	linux-pm@vger.kernel.org
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 3b8842c4a340..d1f6c758b5e8 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -276,6 +276,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 && 64BIT)
+	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 5cf2f54debaf..3c3c27d54c13 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -61,4 +61,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..e5437c294803
--- /dev/null
+++ b/drivers/mailbox/qcom-cpucp-mbox.c
@@ -0,0 +1,187 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/bitops.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define APSS_CPUCP_IPC_CHAN_SUPPORTED		3
+#define APSS_CPUCP_MBOX_CMD_OFF			0x4
+
+/* Tx Registers */
+#define APSS_CPUCP_TX_MBOX_CMD(i)		(0x100 + ((i) * 8))
+
+/* Rx Registers */
+#define APSS_CPUCP_RX_MBOX_CMD(i)		(0x100 + ((i) * 8))
+#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		GENMASK_ULL(63, 0)
+
+/**
+ * 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
+ */
+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;
+};
+
+static inline int channel_number(struct mbox_chan *chan)
+{
+	return chan - chan->mbox->chans;
+}
+
+static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
+{
+	struct qcom_cpucp_mbox *cpucp = data;
+	u64 status;
+	int i;
+
+	status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
+
+	for_each_set_bit(i, (unsigned long *)&status, APSS_CPUCP_IPC_CHAN_SUPPORTED) {
+		u32 val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
+		struct mbox_chan *chan = &cpucp->chans[i];
+		unsigned long flags;
+
+		/* Provide mutual exclusion with changes to chan->cl */
+		spin_lock_irqsave(&chan->lock, flags);
+		if (chan->cl)
+			mbox_chan_received_data(chan, &val);
+		writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
+		spin_unlock_irqrestore(&chan->lock, flags);
+	}
+
+	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 = channel_number(chan);
+	u64 val;
+
+	val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
+	val |= BIT(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 = channel_number(chan);
+	u64 val;
+
+	val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
+	val &= ~BIT(chan_id);
+	writeq(val, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
+}
+
+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 = channel_number(chan);
+	u32 *val = data;
+
+	writel(*val, cpucp->tx_base + APSS_CPUCP_TX_MBOX_CMD(chan_id) + APSS_CPUCP_MBOX_CMD_OFF);
+
+	return 0;
+}
+
+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_mbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct qcom_cpucp_mbox *cpucp;
+	struct mbox_controller *mbox;
+	int irq, ret;
+
+	cpucp = devm_kzalloc(dev, sizeof(*cpucp), GFP_KERNEL);
+	if (!cpucp)
+		return -ENOMEM;
+
+	cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
+	if (IS_ERR(cpucp->rx_base))
+		return PTR_ERR(cpucp->rx_base);
+
+	cpucp->tx_base = devm_of_iomap(dev, dev->of_node, 1, NULL);
+	if (IS_ERR(cpucp->tx_base))
+		return PTR_ERR(cpucp->tx_base);
+
+	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);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_irq(dev, irq, qcom_cpucp_mbox_irq_fn,
+			       IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to register irq: %d\n", irq);
+
+	writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
+
+	mbox = &cpucp->mbox;
+	mbox->dev = dev;
+	mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
+	mbox->chans = cpucp->chans;
+	mbox->ops = &qcom_cpucp_mbox_chan_ops;
+
+	ret = devm_mbox_controller_register(dev, mbox);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to create mailbox\n");
+
+	return 0;
+}
+
+static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
+	{ .compatible = "qcom,x1e80100-cpucp-mbox" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
+
+static struct platform_driver qcom_cpucp_mbox_driver = {
+	.probe = qcom_cpucp_mbox_probe,
+	.driver = {
+		.name = "qcom_cpucp_mbox",
+		.of_match_table = qcom_cpucp_mbox_of_match,
+	},
+};
+
+static int __init qcom_cpucp_mbox_init(void)
+{
+	return platform_driver_register(&qcom_cpucp_mbox_driver);
+}
+core_initcall(qcom_cpucp_mbox_init);
+
+static void __exit 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

Re: [PATCH V6 2/5] mailbox: Add support for QTI CPUCP mailbox controller

[Bjorn Andersson]

On Wed, Jun 12, 2024 at 06:10:53PM GMT, Sibi Sankar 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.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>

Reviewed-by: Bjorn Andersson <andersson@kernel.org>

Regards,
Bjorn

> ---
> 
> v5:
> * Fix build error reported by kernel test robot by adding 64BIT requirement
>   to COMPILE_TEST
> 
>  MAINTAINERS                       |   7 ++
>  drivers/mailbox/Kconfig           |   8 ++
>  drivers/mailbox/Makefile          |   2 +
>  drivers/mailbox/qcom-cpucp-mbox.c | 187 ++++++++++++++++++++++++++++++
>  4 files changed, 204 insertions(+)
>  create mode 100644 drivers/mailbox/qcom-cpucp-mbox.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e04f583780c5..d7c00abe2f93 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18533,6 +18533,13 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/power/avs/qcom,cpr.yaml
>  F:	drivers/pmdomain/qcom/cpr.c
>  
> +QUALCOMM CPUCP MAILBOX DRIVER
> +M:	Sibi Sankar <quic_sibis@quicinc.com>
> +L:	linux-arm-msm@vger.kernel.org
> +S:	Supported
> +F:	Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
> +F:	drivers/mailbox/qcom-cpucp-mbox.c
> +
>  QUALCOMM CPUFREQ DRIVER MSM8996/APQ8096
>  M:	Ilia Lin <ilia.lin@kernel.org>
>  L:	linux-pm@vger.kernel.org
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 3b8842c4a340..d1f6c758b5e8 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -276,6 +276,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 && 64BIT)
> +	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 5cf2f54debaf..3c3c27d54c13 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -61,4 +61,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..e5437c294803
> --- /dev/null
> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
> @@ -0,0 +1,187 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#define APSS_CPUCP_IPC_CHAN_SUPPORTED		3
> +#define APSS_CPUCP_MBOX_CMD_OFF			0x4
> +
> +/* Tx Registers */
> +#define APSS_CPUCP_TX_MBOX_CMD(i)		(0x100 + ((i) * 8))
> +
> +/* Rx Registers */
> +#define APSS_CPUCP_RX_MBOX_CMD(i)		(0x100 + ((i) * 8))
> +#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		GENMASK_ULL(63, 0)
> +
> +/**
> + * 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
> + */
> +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;
> +};
> +
> +static inline int channel_number(struct mbox_chan *chan)
> +{
> +	return chan - chan->mbox->chans;
> +}
> +
> +static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
> +{
> +	struct qcom_cpucp_mbox *cpucp = data;
> +	u64 status;
> +	int i;
> +
> +	status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
> +
> +	for_each_set_bit(i, (unsigned long *)&status, APSS_CPUCP_IPC_CHAN_SUPPORTED) {
> +		u32 val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
> +		struct mbox_chan *chan = &cpucp->chans[i];
> +		unsigned long flags;
> +
> +		/* Provide mutual exclusion with changes to chan->cl */
> +		spin_lock_irqsave(&chan->lock, flags);
> +		if (chan->cl)
> +			mbox_chan_received_data(chan, &val);
> +		writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
> +		spin_unlock_irqrestore(&chan->lock, flags);
> +	}
> +
> +	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 = channel_number(chan);
> +	u64 val;
> +
> +	val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
> +	val |= BIT(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 = channel_number(chan);
> +	u64 val;
> +
> +	val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
> +	val &= ~BIT(chan_id);
> +	writeq(val, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
> +}
> +
> +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 = channel_number(chan);
> +	u32 *val = data;
> +
> +	writel(*val, cpucp->tx_base + APSS_CPUCP_TX_MBOX_CMD(chan_id) + APSS_CPUCP_MBOX_CMD_OFF);
> +
> +	return 0;
> +}
> +
> +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_mbox_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qcom_cpucp_mbox *cpucp;
> +	struct mbox_controller *mbox;
> +	int irq, ret;
> +
> +	cpucp = devm_kzalloc(dev, sizeof(*cpucp), GFP_KERNEL);
> +	if (!cpucp)
> +		return -ENOMEM;
> +
> +	cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
> +	if (IS_ERR(cpucp->rx_base))
> +		return PTR_ERR(cpucp->rx_base);
> +
> +	cpucp->tx_base = devm_of_iomap(dev, dev->of_node, 1, NULL);
> +	if (IS_ERR(cpucp->tx_base))
> +		return PTR_ERR(cpucp->tx_base);
> +
> +	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);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = devm_request_irq(dev, irq, qcom_cpucp_mbox_irq_fn,
> +			       IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to register irq: %d\n", irq);
> +
> +	writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
> +
> +	mbox = &cpucp->mbox;
> +	mbox->dev = dev;
> +	mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
> +	mbox->chans = cpucp->chans;
> +	mbox->ops = &qcom_cpucp_mbox_chan_ops;
> +
> +	ret = devm_mbox_controller_register(dev, mbox);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to create mailbox\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
> +	{ .compatible = "qcom,x1e80100-cpucp-mbox" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
> +
> +static struct platform_driver qcom_cpucp_mbox_driver = {
> +	.probe = qcom_cpucp_mbox_probe,
> +	.driver = {
> +		.name = "qcom_cpucp_mbox",
> +		.of_match_table = qcom_cpucp_mbox_of_match,
> +	},
> +};
> +
> +static int __init qcom_cpucp_mbox_init(void)
> +{
> +	return platform_driver_register(&qcom_cpucp_mbox_driver);
> +}
> +core_initcall(qcom_cpucp_mbox_init);
> +
> +static void __exit 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
> 

Re: [PATCH V6 2/5] mailbox: Add support for QTI CPUCP mailbox controller

[Konrad Dybcio]

On 12.06.2024 2:40 PM, Sibi Sankar 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.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH V6 2/5] mailbox: Add support for QTI CPUCP mailbox controller

[Nathan Chancellor]

Hi Sibi,

On Wed, Jun 12, 2024 at 06:10:53PM +0530, Sibi Sankar 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.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
> 
> v5:
> * Fix build error reported by kernel test robot by adding 64BIT requirement
>   to COMPILE_TEST
...
> +config QCOM_CPUCP_MBOX
> +	tristate "Qualcomm Technologies, Inc. CPUCP mailbox driver"
> +	depends on ARCH_QCOM || (COMPILE_TEST && 64BIT)

This doesn't work, ARCH=arm allmodconfig is still broken with:

  drivers/mailbox/qcom-cpucp-mbox.c: In function 'qcom_cpucp_mbox_irq_fn':
  drivers/mailbox/qcom-cpucp-mbox.c:54:18: error: implicit declaration of function 'readq'; did you mean 'readb'? [-Wimplicit-function-declaration]
     54 |         status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
        |                  ^~~~~
        |                  readb
  drivers/mailbox/qcom-cpucp-mbox.c:65:17: error: implicit declaration of function 'writeq'; did you mean 'writel'? [-Wimplicit-function-declaration]
     65 |                 writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
        |                 ^~~~~~
        |                 writel

because there is ARCH_QCOM for that architecture as well.

You could resolve this by just including either io-64-nonatomic-hi-lo.h
or io-64-nonatomic-lo-hi.h or shuffling the dependencies to require
64BIT unconditionally:

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index d1f6c758b5e8..8d46b76c23fd 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -278,7 +278,7 @@ config SPRD_MBOX
 
 config QCOM_CPUCP_MBOX
 	tristate "Qualcomm Technologies, Inc. CPUCP mailbox driver"
-	depends on ARCH_QCOM || (COMPILE_TEST && 64BIT)
+	depends on 64BIT && (ARCH_QCOM || COMPILE_TEST)
 	help
 	  Qualcomm Technologies, Inc. CPUSS Control Processor (CPUCP) mailbox
 	  controller driver enables communication between AP and CPUCP. Say

Cheers,
Nathan

[PATCH V6 3/5] arm64: dts: qcom: x1e80100: Resize GIC Redistributor register region

[Sibi Sankar]

Resize the GICR register region as it currently seeps into the CPU Control
Processor mailbox RX region.

Fixes: af16b00578a7 ("arm64: dts: qcom: Add base X1E80100 dtsi and the QCP dts")
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---

v5:
* Pick Rbs

 arch/arm64/boot/dts/qcom/x1e80100.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
index 9944c654851e..28ae10c24a5f 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
@@ -5091,7 +5091,7 @@ apps_smmu: iommu@15000000 {
 		intc: interrupt-controller@17000000 {
 			compatible = "arm,gic-v3";
 			reg = <0 0x17000000 0 0x10000>,     /* GICD */
-			      <0 0x17080000 0 0x480000>;    /* GICR * 12 */
+			      <0 0x17080000 0 0x300000>;    /* GICR * 12 */
 
 			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
 
-- 
2.34.1

[PATCH V6 4/5] arm64: dts: qcom: x1e80100: Add cpucp mailbox and sram nodes

[Sibi Sankar]

Add the cpucp mailbox and sram nodes required by SCMI perf protocol
on X1E80100 SoCs.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 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 28ae10c24a5f..7b619db07694 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
@@ -5116,6 +5116,13 @@ gic_its: msi-controller@17040000 {
 			};
 		};
 
+		cpucp_mbox: mailbox@17430000 {
+			compatible = "qcom,x1e80100-cpucp-mbox";
+			reg = <0 0x17430000 0 0x10000>, <0 0x18830000 0 0x10000>;
+			interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>;
+			#mbox-cells = <1>;
+		};
+
 		apps_rsc: rsc@17500000 {
 			compatible = "qcom,rpmh-rsc";
 			reg = <0 0x17500000 0 0x10000>,
@@ -5299,6 +5306,25 @@ frame@1780d000 {
 			};
 		};
 
+		sram: sram@18b4e000 {
+			compatible = "mmio-sram";
+			reg = <0x0 0x18b4e000 0x0 0x400>;
+
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0x0 0x0 0x18b4e000 0x400>;
+
+			cpu_scp_lpri0: scp-sram-section@0 {
+				compatible = "arm,scmi-shmem";
+				reg = <0x0 0x200>;
+			};
+
+			cpu_scp_lpri1: scp-sram-section@200 {
+				compatible = "arm,scmi-shmem";
+				reg = <0x200 0x200>;
+			};
+		};
+
 		system-cache-controller@25000000 {
 			compatible = "qcom,x1e80100-llcc";
 			reg = <0 0x25000000 0 0x200000>,
-- 
2.34.1

[PATCH V6 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq

[Sibi Sankar]

Enable cpufreq on X1E80100 SoCs through the SCMI perf protocol node.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 arch/arm64/boot/dts/qcom/x1e80100.dtsi | 63 ++++++++++++++++----------
 1 file changed, 39 insertions(+), 24 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
index 7b619db07694..d134dc4c7425 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
@@ -69,8 +69,8 @@ CPU0: cpu@0 {
 			reg = <0x0 0x0>;
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
-			power-domains = <&CPU_PD0>;
-			power-domain-names = "psci";
+			power-domains = <&CPU_PD0>, <&scmi_dvfs 0>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&CLUSTER_C4>;
 
 			L2_0: l2-cache {
@@ -86,8 +86,8 @@ CPU1: cpu@100 {
 			reg = <0x0 0x100>;
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
-			power-domains = <&CPU_PD1>;
-			power-domain-names = "psci";
+			power-domains = <&CPU_PD1>, <&scmi_dvfs 0>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&CLUSTER_C4>;
 		};
 
@@ -97,8 +97,8 @@ CPU2: cpu@200 {
 			reg = <0x0 0x200>;
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
-			power-domains = <&CPU_PD2>;
-			power-domain-names = "psci";
+			power-domains = <&CPU_PD2>, <&scmi_dvfs 0>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&CLUSTER_C4>;
 		};
 
@@ -108,8 +108,8 @@ CPU3: cpu@300 {
 			reg = <0x0 0x300>;
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
-			power-domains = <&CPU_PD3>;
-			power-domain-names = "psci";
+			power-domains = <&CPU_PD3>, <&scmi_dvfs 0>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&CLUSTER_C4>;
 		};
 
@@ -119,8 +119,8 @@ CPU4: cpu@10000 {
 			reg = <0x0 0x10000>;
 			enable-method = "psci";
 			next-level-cache = <&L2_1>;
-			power-domains = <&CPU_PD4>;
-			power-domain-names = "psci";
+			power-domains = <&CPU_PD4>, <&scmi_dvfs 1>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&CLUSTER_C4>;
 
 			L2_1: l2-cache {
@@ -136,8 +136,8 @@ CPU5: cpu@10100 {
 			reg = <0x0 0x10100>;
 			enable-method = "psci";
 			next-level-cache = <&L2_1>;
-			power-domains = <&CPU_PD5>;
-			power-domain-names = "psci";
+			power-domains = <&CPU_PD5>, <&scmi_dvfs 1>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&CLUSTER_C4>;
 		};
 
@@ -147,8 +147,8 @@ CPU6: cpu@10200 {
 			reg = <0x0 0x10200>;
 			enable-method = "psci";
 			next-level-cache = <&L2_1>;
-			power-domains = <&CPU_PD6>;
-			power-domain-names = "psci";
+			power-domains = <&CPU_PD6>, <&scmi_dvfs 1>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&CLUSTER_C4>;
 		};
 
@@ -158,8 +158,8 @@ CPU7: cpu@10300 {
 			reg = <0x0 0x10300>;
 			enable-method = "psci";
 			next-level-cache = <&L2_1>;
-			power-domains = <&CPU_PD7>;
-			power-domain-names = "psci";
+			power-domains = <&CPU_PD7>, <&scmi_dvfs 1>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&CLUSTER_C4>;
 		};
 
@@ -169,8 +169,8 @@ CPU8: cpu@20000 {
 			reg = <0x0 0x20000>;
 			enable-method = "psci";
 			next-level-cache = <&L2_2>;
-			power-domains = <&CPU_PD8>;
-			power-domain-names = "psci";
+			power-domains = <&CPU_PD8>, <&scmi_dvfs 2>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&CLUSTER_C4>;
 
 			L2_2: l2-cache {
@@ -186,8 +186,8 @@ CPU9: cpu@20100 {
 			reg = <0x0 0x20100>;
 			enable-method = "psci";
 			next-level-cache = <&L2_2>;
-			power-domains = <&CPU_PD9>;
-			power-domain-names = "psci";
+			power-domains = <&CPU_PD9>, <&scmi_dvfs 2>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&CLUSTER_C4>;
 		};
 
@@ -197,8 +197,8 @@ CPU10: cpu@20200 {
 			reg = <0x0 0x20200>;
 			enable-method = "psci";
 			next-level-cache = <&L2_2>;
-			power-domains = <&CPU_PD10>;
-			power-domain-names = "psci";
+			power-domains = <&CPU_PD10>, <&scmi_dvfs 2>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&CLUSTER_C4>;
 		};
 
@@ -208,8 +208,8 @@ CPU11: cpu@20300 {
 			reg = <0x0 0x20300>;
 			enable-method = "psci";
 			next-level-cache = <&L2_2>;
-			power-domains = <&CPU_PD11>;
-			power-domain-names = "psci";
+			power-domains = <&CPU_PD11>, <&scmi_dvfs 2>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&CLUSTER_C4>;
 		};
 
@@ -309,6 +309,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>;
+				#power-domain-cells = <1>;
+			};
+		};
 	};
 
 	clk_virt: interconnect-0 {
-- 
2.34.1

Re: [PATCH V6 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq

[Johan Hovold]

On Wed, Jun 12, 2024 at 06:10:56PM +0530, Sibi Sankar wrote:
> Enable cpufreq on X1E80100 SoCs through the SCMI perf protocol node.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/x1e80100.dtsi | 63 ++++++++++++++++----------
>  1 file changed, 39 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> index 7b619db07694..d134dc4c7425 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> @@ -69,8 +69,8 @@ CPU0: cpu@0 {
>  			reg = <0x0 0x0>;
>  			enable-method = "psci";
>  			next-level-cache = <&L2_0>;
> -			power-domains = <&CPU_PD0>;
> -			power-domain-names = "psci";
> +			power-domains = <&CPU_PD0>, <&scmi_dvfs 0>;
> +			power-domain-names = "psci", "perf";
>  			cpu-idle-states = <&CLUSTER_C4>;

> +		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>;
> +				#power-domain-cells = <1>;
> +			};
> +		};
>  	};

This series gives a nice performance boost on the x1e80100 CRD, but I'm
seeing a bunch of warnings and errors that need to be addressed:

[    9.533053] arm-scmi firmware:scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:0] - ret:-95. Using regular messaging.
[    9.549458] arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
[    9.563925] arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
[    9.572835] arm-scmi firmware:scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:1] - ret:-95. Using regular messaging.
[    9.609471] arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
[    9.633341] arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
[    9.650000] arm-scmi firmware:scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:2] - ret:-95. Using regular messaging.
[    9.727098] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
[    9.737157] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
[    9.875039] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
[    9.888428] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
[    9.913506] debugfs: Directory 'NCC' with parent 'pm_genpd' already present!
[    9.922198] debugfs: Directory 'NCC' with parent 'pm_genpd' already present!

Johan

Re: [PATCH V6 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq

[Sibi Sankar]

On 7/2/24 21:25, Johan Hovold wrote:
> On Wed, Jun 12, 2024 at 06:10:56PM +0530, Sibi Sankar wrote:
>> Enable cpufreq on X1E80100 SoCs through the SCMI perf protocol node.
>>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/x1e80100.dtsi | 63 ++++++++++++++++----------
>>   1 file changed, 39 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> index 7b619db07694..d134dc4c7425 100644
>> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> @@ -69,8 +69,8 @@ CPU0: cpu@0 {
>>   			reg = <0x0 0x0>;
>>   			enable-method = "psci";
>>   			next-level-cache = <&L2_0>;
>> -			power-domains = <&CPU_PD0>;
>> -			power-domain-names = "psci";
>> +			power-domains = <&CPU_PD0>, <&scmi_dvfs 0>;
>> +			power-domain-names = "psci", "perf";
>>   			cpu-idle-states = <&CLUSTER_C4>;
> 
>> +		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>;
>> +				#power-domain-cells = <1>;
>> +			};
>> +		};
>>   	};
> 

Hey Johan,

Thanks for trying out the series.

> This series gives a nice performance boost on the x1e80100 CRD, but I'm
> seeing a bunch of warnings and errors that need to be addressed:
> 
> [    9.533053] arm-scmi firmware:scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:0] - ret:-95. Using regular messaging.
> [    9.549458] arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
> [    9.563925] arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
> [    9.572835] arm-scmi firmware:scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:1] - ret:-95. Using regular messaging.
> [    9.609471] arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
> [    9.633341] arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
> [    9.650000] arm-scmi firmware:scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:2] - ret:-95. Using regular messaging.

X1E uses fast channels only for message-id: 7 (level set) and regular
channels for all the other messages. The spec doesn't mandate fast
channels for any of the supported message ids for the perf protocol.
So nothing to fix here.

> [    9.727098] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
> [    9.737157] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
> [    9.875039] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
> [    9.888428] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1

The duplicate entries reported by the perf protocol come directly from
the speed bins. I was told the duplicate entry with volt 0 is meant to
indicate a lower power way of achieving the said frequency at a lower
core count. We have no way of using it in the kernel and it gets safely
discarded. So again nothing to fix in the kernel.

> [    9.913506] debugfs: Directory 'NCC' with parent 'pm_genpd' already present!
> [    9.922198] debugfs: Directory 'NCC' with parent 'pm_genpd' already present!

Yeah I did notice ^^ during dev, the series isn't the one introducing it
so it shouldn't block the series acceptance. Meanwhile I'll spend some
cycles to get this warn fixed.

-Sibi

> 
> Johan
> 

Re: [PATCH V6 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq

[Nikunj Kela]

On 7/2/2024 12:59 PM, Sibi Sankar wrote:
>
>
> On 7/2/24 21:25, Johan Hovold wrote:
>> On Wed, Jun 12, 2024 at 06:10:56PM +0530, Sibi Sankar wrote:
>>> Enable cpufreq on X1E80100 SoCs through the SCMI perf protocol node.
>>>
>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>>> ---
>>>   arch/arm64/boot/dts/qcom/x1e80100.dtsi | 63
>>> ++++++++++++++++----------
>>>   1 file changed, 39 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>> b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>> index 7b619db07694..d134dc4c7425 100644
>>> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>> @@ -69,8 +69,8 @@ CPU0: cpu@0 {
>>>               reg = <0x0 0x0>;
>>>               enable-method = "psci";
>>>               next-level-cache = <&L2_0>;
>>> -            power-domains = <&CPU_PD0>;
>>> -            power-domain-names = "psci";
>>> +            power-domains = <&CPU_PD0>, <&scmi_dvfs 0>;
>>> +            power-domain-names = "psci", "perf";
>>>               cpu-idle-states = <&CLUSTER_C4>;
>>
>>> +        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>;
>>> +                #power-domain-cells = <1>;
>>> +            };
>>> +        };
>>>       };
>>
>
> Hey Johan,
>
> Thanks for trying out the series.
>
>> This series gives a nice performance boost on the x1e80100 CRD, but I'm
>> seeing a bunch of warnings and errors that need to be addressed:
>>
>> [    9.533053] arm-scmi firmware:scmi: Failed to get FC for protocol
>> 13 [MSG_ID:6 / RES_ID:0] - ret:-95. Using regular messaging.
>> [    9.549458] arm-scmi firmware:scmi: Failed to add opps_by_lvl at
>> 3417600 for NCC - ret:-16
>> [    9.563925] arm-scmi firmware:scmi: Failed to add opps_by_lvl at
>> 3417600 for NCC - ret:-16
>> [    9.572835] arm-scmi firmware:scmi: Failed to get FC for protocol
>> 13 [MSG_ID:6 / RES_ID:1] - ret:-95. Using regular messaging.
>> [    9.609471] arm-scmi firmware:scmi: Failed to add opps_by_lvl at
>> 3417600 for NCC - ret:-16
>> [    9.633341] arm-scmi firmware:scmi: Failed to add opps_by_lvl at
>> 3417600 for NCC - ret:-16
>> [    9.650000] arm-scmi firmware:scmi: Failed to get FC for protocol
>> 13 [MSG_ID:6 / RES_ID:2] - ret:-95. Using regular messaging.
>
> X1E uses fast channels only for message-id: 7 (level set) and regular
> channels for all the other messages. The spec doesn't mandate fast
> channels for any of the supported message ids for the perf protocol.
> So nothing to fix here.
>
>> [    9.727098] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected.
>> Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq:
>> 3417600000, volt: 0, enabled: 1
>> [    9.737157] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected.
>> Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq:
>> 3417600000, volt: 0, enabled: 1
>> [    9.875039] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected.
>> Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq:
>> 3417600000, volt: 0, enabled: 1
>> [    9.888428] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected.
>> Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq:
>> 3417600000, volt: 0, enabled: 1
>
> The duplicate entries reported by the perf protocol come directly from
> the speed bins. I was told the duplicate entry with volt 0 is meant to
> indicate a lower power way of achieving the said frequency at a lower
> core count. We have no way of using it in the kernel and it gets safely
> discarded. So again nothing to fix in the kernel.

Hi Sibi,

Can you try increasing the max_msg_size to 256 bytes in mailbox
transport? We saw the same issue but got resolved by increasing the
max_msg_size for the transport(obviously, I reduced the max_msg to 10 to
keep the total shmem size same). Even the opps_by_lvl warning went away
with this for us.

Thanks,

-Nikunj

>
>> [    9.913506] debugfs: Directory 'NCC' with parent 'pm_genpd'
>> already present!
>> [    9.922198] debugfs: Directory 'NCC' with parent 'pm_genpd'
>> already present!
>
> Yeah I did notice ^^ during dev, the series isn't the one introducing it
> so it shouldn't block the series acceptance. Meanwhile I'll spend some
> cycles to get this warn fixed.
>
> -Sibi
>
>>
>> Johan
>>

Re: [PATCH V6 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq

[Sibi Sankar]

On 7/3/24 01:43, Nikunj Kela wrote:
> 
> On 7/2/2024 12:59 PM, Sibi Sankar wrote:
>>
>>
>> On 7/2/24 21:25, Johan Hovold wrote:
>>> On Wed, Jun 12, 2024 at 06:10:56PM +0530, Sibi Sankar wrote:
>>>> Enable cpufreq on X1E80100 SoCs through the SCMI perf protocol node.
>>>>
>>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>>>> ---
>>>>    arch/arm64/boot/dts/qcom/x1e80100.dtsi | 63
>>>> ++++++++++++++++----------
>>>>    1 file changed, 39 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>>> b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>>> index 7b619db07694..d134dc4c7425 100644
>>>> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>>> @@ -69,8 +69,8 @@ CPU0: cpu@0 {
>>>>                reg = <0x0 0x0>;
>>>>                enable-method = "psci";
>>>>                next-level-cache = <&L2_0>;
>>>> -            power-domains = <&CPU_PD0>;
>>>> -            power-domain-names = "psci";
>>>> +            power-domains = <&CPU_PD0>, <&scmi_dvfs 0>;
>>>> +            power-domain-names = "psci", "perf";
>>>>                cpu-idle-states = <&CLUSTER_C4>;
>>>
>>>> +        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>;
>>>> +                #power-domain-cells = <1>;
>>>> +            };
>>>> +        };
>>>>        };
>>>
>>
>> Hey Johan,
>>
>> Thanks for trying out the series.
>>
>>> This series gives a nice performance boost on the x1e80100 CRD, but I'm
>>> seeing a bunch of warnings and errors that need to be addressed:
>>>
>>> [    9.533053] arm-scmi firmware:scmi: Failed to get FC for protocol
>>> 13 [MSG_ID:6 / RES_ID:0] - ret:-95. Using regular messaging.
>>> [    9.549458] arm-scmi firmware:scmi: Failed to add opps_by_lvl at
>>> 3417600 for NCC - ret:-16
>>> [    9.563925] arm-scmi firmware:scmi: Failed to add opps_by_lvl at
>>> 3417600 for NCC - ret:-16
>>> [    9.572835] arm-scmi firmware:scmi: Failed to get FC for protocol
>>> 13 [MSG_ID:6 / RES_ID:1] - ret:-95. Using regular messaging.
>>> [    9.609471] arm-scmi firmware:scmi: Failed to add opps_by_lvl at
>>> 3417600 for NCC - ret:-16
>>> [    9.633341] arm-scmi firmware:scmi: Failed to add opps_by_lvl at
>>> 3417600 for NCC - ret:-16
>>> [    9.650000] arm-scmi firmware:scmi: Failed to get FC for protocol
>>> 13 [MSG_ID:6 / RES_ID:2] - ret:-95. Using regular messaging.
>>
>> X1E uses fast channels only for message-id: 7 (level set) and regular
>> channels for all the other messages. The spec doesn't mandate fast
>> channels for any of the supported message ids for the perf protocol.
>> So nothing to fix here.
>>
>>> [    9.727098] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected.
>>> Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq:
>>> 3417600000, volt: 0, enabled: 1
>>> [    9.737157] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected.
>>> Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq:
>>> 3417600000, volt: 0, enabled: 1
>>> [    9.875039] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected.
>>> Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq:
>>> 3417600000, volt: 0, enabled: 1
>>> [    9.888428] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected.
>>> Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq:
>>> 3417600000, volt: 0, enabled: 1
>>
>> The duplicate entries reported by the perf protocol come directly from
>> the speed bins. I was told the duplicate entry with volt 0 is meant to
>> indicate a lower power way of achieving the said frequency at a lower
>> core count. We have no way of using it in the kernel and it gets safely
>> discarded. So again nothing to fix in the kernel.
> 
> Hi Sibi,
> 
> Can you try increasing the max_msg_size to 256 bytes in mailbox
> transport? We saw the same issue but got resolved by increasing the
> max_msg_size for the transport(obviously, I reduced the max_msg to 10 to
> keep the total shmem size same). Even the opps_by_lvl warning went away
> with this for us.

Nikunj,
Thanks for taking time to review the series :)

Not sure if we are talking about the same things here, are you
suggesting that tweaking with the max_msg size will stop the SCMI
controller from reporting duplicate OPPs? Even if it does go away
magically wouldn't it mean you are dropping messages? Also opps_by_lvl
failing with -16 and duplicate opps detected in the opp core have the
same root cause i.e. duplicate entries reported by the controller.

> 
> Thanks,
> 
> -Nikunj
> 
>>
>>> [    9.913506] debugfs: Directory 'NCC' with parent 'pm_genpd'
>>> already present!
>>> [    9.922198] debugfs: Directory 'NCC' with parent 'pm_genpd'
>>> already present!
>>
>> Yeah I did notice ^^ during dev, the series isn't the one introducing it
>> so it shouldn't block the series acceptance. Meanwhile I'll spend some
>> cycles to get this warn fixed.

Johan,

https://lore.kernel.org/lkml/20240703110741.2668800-1-quic_sibis@quicinc.com/

Posted a fix for the warn ^^

>>
>> -Sibi
>>
>>>
>>> Johan
>>>

Re: [PATCH V6 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq

[Nikunj Kela]

On 7/3/2024 4:23 AM, Sibi Sankar wrote:
>
>
> On 7/3/24 01:43, Nikunj Kela wrote:
>>
>> On 7/2/2024 12:59 PM, Sibi Sankar wrote:
>>>
>>>
>>> On 7/2/24 21:25, Johan Hovold wrote:
>>>> On Wed, Jun 12, 2024 at 06:10:56PM +0530, Sibi Sankar wrote:
>>>>> Enable cpufreq on X1E80100 SoCs through the SCMI perf protocol node.
>>>>>
>>>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>>>>> ---
>>>>>    arch/arm64/boot/dts/qcom/x1e80100.dtsi | 63
>>>>> ++++++++++++++++----------
>>>>>    1 file changed, 39 insertions(+), 24 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>>>> b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>>>> index 7b619db07694..d134dc4c7425 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>>>> @@ -69,8 +69,8 @@ CPU0: cpu@0 {
>>>>>                reg = <0x0 0x0>;
>>>>>                enable-method = "psci";
>>>>>                next-level-cache = <&L2_0>;
>>>>> -            power-domains = <&CPU_PD0>;
>>>>> -            power-domain-names = "psci";
>>>>> +            power-domains = <&CPU_PD0>, <&scmi_dvfs 0>;
>>>>> +            power-domain-names = "psci", "perf";
>>>>>                cpu-idle-states = <&CLUSTER_C4>;
>>>>
>>>>> +        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>;
>>>>> +                #power-domain-cells = <1>;
>>>>> +            };
>>>>> +        };
>>>>>        };
>>>>
>>>
>>> Hey Johan,
>>>
>>> Thanks for trying out the series.
>>>
>>>> This series gives a nice performance boost on the x1e80100 CRD, but
>>>> I'm
>>>> seeing a bunch of warnings and errors that need to be addressed:
>>>>
>>>> [    9.533053] arm-scmi firmware:scmi: Failed to get FC for protocol
>>>> 13 [MSG_ID:6 / RES_ID:0] - ret:-95. Using regular messaging.
>>>> [    9.549458] arm-scmi firmware:scmi: Failed to add opps_by_lvl at
>>>> 3417600 for NCC - ret:-16
>>>> [    9.563925] arm-scmi firmware:scmi: Failed to add opps_by_lvl at
>>>> 3417600 for NCC - ret:-16
>>>> [    9.572835] arm-scmi firmware:scmi: Failed to get FC for protocol
>>>> 13 [MSG_ID:6 / RES_ID:1] - ret:-95. Using regular messaging.
>>>> [    9.609471] arm-scmi firmware:scmi: Failed to add opps_by_lvl at
>>>> 3417600 for NCC - ret:-16
>>>> [    9.633341] arm-scmi firmware:scmi: Failed to add opps_by_lvl at
>>>> 3417600 for NCC - ret:-16
>>>> [    9.650000] arm-scmi firmware:scmi: Failed to get FC for protocol
>>>> 13 [MSG_ID:6 / RES_ID:2] - ret:-95. Using regular messaging.
>>>
>>> X1E uses fast channels only for message-id: 7 (level set) and regular
>>> channels for all the other messages. The spec doesn't mandate fast
>>> channels for any of the supported message ids for the perf protocol.
>>> So nothing to fix here.
>>>
>>>> [    9.727098] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected.
>>>> Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq:
>>>> 3417600000, volt: 0, enabled: 1
>>>> [    9.737157] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected.
>>>> Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq:
>>>> 3417600000, volt: 0, enabled: 1
>>>> [    9.875039] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected.
>>>> Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq:
>>>> 3417600000, volt: 0, enabled: 1
>>>> [    9.888428] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected.
>>>> Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq:
>>>> 3417600000, volt: 0, enabled: 1
>>>
>>> The duplicate entries reported by the perf protocol come directly from
>>> the speed bins. I was told the duplicate entry with volt 0 is meant to
>>> indicate a lower power way of achieving the said frequency at a lower
>>> core count. We have no way of using it in the kernel and it gets safely
>>> discarded. So again nothing to fix in the kernel.
>>
>> Hi Sibi,
>>
>> Can you try increasing the max_msg_size to 256 bytes in mailbox
>> transport? We saw the same issue but got resolved by increasing the
>> max_msg_size for the transport(obviously, I reduced the max_msg to 10 to
>> keep the total shmem size same). Even the opps_by_lvl warning went away
>> with this for us.
>
> Nikunj,
> Thanks for taking time to review the series :)
>
> Not sure if we are talking about the same things here, are you
> suggesting that tweaking with the max_msg size will stop the SCMI
> controller from reporting duplicate OPPs? Even if it does go away
> magically wouldn't it mean you are dropping messages? Also opps_by_lvl
> failing with -16 and duplicate opps detected in the opp core have the
> same root cause i.e. duplicate entries reported by the controller.


Sibi,

My observation was that only 12 OPPs could fit it 128bytes msg_size and
our platform was sending 16 OPPs in one go. OPPs above 12 were getting
clobbered so the duplicate warning/error were not genuine. You may need
to tweak platform to send only 12(or less) OPPs in one go.


>
>>
>> Thanks,
>>
>> -Nikunj
>>
>>>
>>>> [    9.913506] debugfs: Directory 'NCC' with parent 'pm_genpd'
>>>> already present!
>>>> [    9.922198] debugfs: Directory 'NCC' with parent 'pm_genpd'
>>>> already present!
>>>
>>> Yeah I did notice ^^ during dev, the series isn't the one
>>> introducing it
>>> so it shouldn't block the series acceptance. Meanwhile I'll spend some
>>> cycles to get this warn fixed.
>
> Johan,
>
> https://lore.kernel.org/lkml/20240703110741.2668800-1-quic_sibis@quicinc.com/
>
>
> Posted a fix for the warn ^^
>
>>>
>>> -Sibi
>>>
>>>>
>>>> Johan
>>>>

Re: [PATCH V6 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq

[Sibi Sankar]

On 7/3/24 19:35, Nikunj Kela wrote:
> 
> On 7/3/2024 4:23 AM, Sibi Sankar wrote:
>>
>>
>> On 7/3/24 01:43, Nikunj Kela wrote:
>>>
>>> On 7/2/2024 12:59 PM, Sibi Sankar wrote:
>>>>
>>>>
>>>> On 7/2/24 21:25, Johan Hovold wrote:
>>>>> On Wed, Jun 12, 2024 at 06:10:56PM +0530, Sibi Sankar wrote:
>>>>>> Enable cpufreq on X1E80100 SoCs through the SCMI perf protocol node.
>>>>>>
>>>>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>>>>>> ---
>>>>>>     arch/arm64/boot/dts/qcom/x1e80100.dtsi | 63
>>>>>> ++++++++++++++++----------
>>>>>>     1 file changed, 39 insertions(+), 24 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>>>>> b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>>>>> index 7b619db07694..d134dc4c7425 100644
>>>>>> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>>>>> @@ -69,8 +69,8 @@ CPU0: cpu@0 {
>>>>>>                 reg = <0x0 0x0>;
>>>>>>                 enable-method = "psci";
>>>>>>                 next-level-cache = <&L2_0>;
>>>>>> -            power-domains = <&CPU_PD0>;
>>>>>> -            power-domain-names = "psci";
>>>>>> +            power-domains = <&CPU_PD0>, <&scmi_dvfs 0>;
>>>>>> +            power-domain-names = "psci", "perf";
>>>>>>                 cpu-idle-states = <&CLUSTER_C4>;
>>>>>
>>>>>> +        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>;
>>>>>> +                #power-domain-cells = <1>;
>>>>>> +            };
>>>>>> +        };
>>>>>>         };
>>>>>
>>>>
>>>> Hey Johan,
>>>>
>>>> Thanks for trying out the series.
>>>>
>>>>> This series gives a nice performance boost on the x1e80100 CRD, but
>>>>> I'm
>>>>> seeing a bunch of warnings and errors that need to be addressed:
>>>>>
>>>>> [    9.533053] arm-scmi firmware:scmi: Failed to get FC for protocol
>>>>> 13 [MSG_ID:6 / RES_ID:0] - ret:-95. Using regular messaging.
>>>>> [    9.549458] arm-scmi firmware:scmi: Failed to add opps_by_lvl at
>>>>> 3417600 for NCC - ret:-16
>>>>> [    9.563925] arm-scmi firmware:scmi: Failed to add opps_by_lvl at
>>>>> 3417600 for NCC - ret:-16
>>>>> [    9.572835] arm-scmi firmware:scmi: Failed to get FC for protocol
>>>>> 13 [MSG_ID:6 / RES_ID:1] - ret:-95. Using regular messaging.
>>>>> [    9.609471] arm-scmi firmware:scmi: Failed to add opps_by_lvl at
>>>>> 3417600 for NCC - ret:-16
>>>>> [    9.633341] arm-scmi firmware:scmi: Failed to add opps_by_lvl at
>>>>> 3417600 for NCC - ret:-16
>>>>> [    9.650000] arm-scmi firmware:scmi: Failed to get FC for protocol
>>>>> 13 [MSG_ID:6 / RES_ID:2] - ret:-95. Using regular messaging.
>>>>
>>>> X1E uses fast channels only for message-id: 7 (level set) and regular
>>>> channels for all the other messages. The spec doesn't mandate fast
>>>> channels for any of the supported message ids for the perf protocol.
>>>> So nothing to fix here.
>>>>
>>>>> [    9.727098] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected.
>>>>> Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq:
>>>>> 3417600000, volt: 0, enabled: 1
>>>>> [    9.737157] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected.
>>>>> Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq:
>>>>> 3417600000, volt: 0, enabled: 1
>>>>> [    9.875039] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected.
>>>>> Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq:
>>>>> 3417600000, volt: 0, enabled: 1
>>>>> [    9.888428] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected.
>>>>> Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq:
>>>>> 3417600000, volt: 0, enabled: 1
>>>>
>>>> The duplicate entries reported by the perf protocol come directly from
>>>> the speed bins. I was told the duplicate entry with volt 0 is meant to
>>>> indicate a lower power way of achieving the said frequency at a lower
>>>> core count. We have no way of using it in the kernel and it gets safely
>>>> discarded. So again nothing to fix in the kernel.
>>>
>>> Hi Sibi,
>>>
>>> Can you try increasing the max_msg_size to 256 bytes in mailbox
>>> transport? We saw the same issue but got resolved by increasing the
>>> max_msg_size for the transport(obviously, I reduced the max_msg to 10 to
>>> keep the total shmem size same). Even the opps_by_lvl warning went away
>>> with this for us.
>>
>> Nikunj,
>> Thanks for taking time to review the series :)
>>
>> Not sure if we are talking about the same things here, are you
>> suggesting that tweaking with the max_msg size will stop the SCMI
>> controller from reporting duplicate OPPs? Even if it does go away
>> magically wouldn't it mean you are dropping messages? Also opps_by_lvl
>> failing with -16 and duplicate opps detected in the opp core have the
>> same root cause i.e. duplicate entries reported by the controller.
> 
> 
> Sibi,
> 
> My observation was that only 12 OPPs could fit it 128bytes msg_size and
> our platform was sending 16 OPPs in one go. OPPs above 12 were getting
> clobbered so the duplicate warning/error were not genuine. You may need
> to tweak platform to send only 12(or less) OPPs in one go.

Nikunj,

The platform we are talking abt in this thread is X1E and the number
of performance levels returned by the PERFORMANCE_DESCRIBE_LEVELS
is just one. I relies on the skip_index and iterator ops to get
all the available levels. So the clobbering you are talking abt
in whatever platform you are referring to does not apply here.
Please find the logs below for Domain 1. Hope this clears up
whatever misunderstanding you had about X1E.

Logs Domain -1:
arm-scmi: iter_perf_levels_update_state num_returned: 1 num_remaining: 15
arm-scmi firmware:scmi: Level 710400 Power 23243 Latency 30us Ifreq 
710400 Index 0
...
[snip]
...
arm-scmi: iter_perf_levels_update_state num_returned: 1 num_remaining: 3
arm-scmi firmware:scmi: Level 3417600 Power 307141 Latency 30us Ifreq 
3417600 Index 12
arm-scmi: iter_perf_levels_update_state num_returned: 1 num_remaining: 2
arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - 
ret:-16
arm-scmi firmware:scmi: Level 3417600 Power 307141 Latency 30us Ifreq 
3417600 Index 13
arm-scmi: iter_perf_levels_update_state num_returned: 1 num_remaining: 1
arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - 
ret:-16
arm-scmi firmware:scmi: Level 3417600 Power 307141 Latency 30us Ifreq 
3417600 Index 14
arm-scmi: iter_perf_levels_update_state num_returned: 1 num_remaining: 0
arm-scmi firmware:scmi: Level 4012800 Power 539962 Latency 30us Ifreq 
4012800 Index 15


-Sibi

> 
> 
>>
>>>
>>> Thanks,
>>>
>>> -Nikunj
>>>
>>>>
>>>>> [    9.913506] debugfs: Directory 'NCC' with parent 'pm_genpd'
>>>>> already present!
>>>>> [    9.922198] debugfs: Directory 'NCC' with parent 'pm_genpd'
>>>>> already present!
>>>>
>>>> Yeah I did notice ^^ during dev, the series isn't the one
>>>> introducing it
>>>> so it shouldn't block the series acceptance. Meanwhile I'll spend some
>>>> cycles to get this warn fixed.
>>
>> Johan,
>>
>> https://lore.kernel.org/lkml/20240703110741.2668800-1-quic_sibis@quicinc.com/
>>
>>
>> Posted a fix for the warn ^^
>>
>>>>
>>>> -Sibi
>>>>
>>>>>
>>>>> Johan
>>>>>

Re: [PATCH V6 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq

[Johan Hovold]

Hi Sibi,

On Wed, Jul 03, 2024 at 01:29:11AM +0530, Sibi Sankar wrote:
> On 7/2/24 21:25, Johan Hovold wrote:
> > On Wed, Jun 12, 2024 at 06:10:56PM +0530, Sibi Sankar wrote:
> >> Enable cpufreq on X1E80100 SoCs through the SCMI perf protocol node.

> > This series gives a nice performance boost on the x1e80100 CRD, but I'm
> > seeing a bunch of warnings and errors that need to be addressed:
> > 
> > [    9.533053] arm-scmi firmware:scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:0] - ret:-95. Using regular messaging.
> > [    9.549458] arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
> > [    9.563925] arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
> > [    9.572835] arm-scmi firmware:scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:1] - ret:-95. Using regular messaging.
> > [    9.609471] arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
> > [    9.633341] arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
> > [    9.650000] arm-scmi firmware:scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:2] - ret:-95. Using regular messaging.
> 
> X1E uses fast channels only for message-id: 7 (level set) and regular
> channels for all the other messages. The spec doesn't mandate fast
> channels for any of the supported message ids for the perf protocol.
> So nothing to fix here.

I didn't look at this in any detail, but if the firmware is spec
compliant you should not be spamming the logs with warnings. Not sure
how best to address that, but you could, for example, add a quirk for
qcom fw or at a minimum demote this mess to info level.

Also the failure to add oops_by_lvl appears to be a separate issue (e.g.
related to the duplicate entries).

> > [    9.727098] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
> > [    9.737157] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
> > [    9.875039] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
> > [    9.888428] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
> 
> The duplicate entries reported by the perf protocol come directly from
> the speed bins. I was told the duplicate entry with volt 0 is meant to
> indicate a lower power way of achieving the said frequency at a lower
> core count. We have no way of using it in the kernel and it gets safely
> discarded. So again nothing to fix in the kernel.

Again, you should not be spamming the logs with warnings for things are
benign (e.g. as it may prevent people from noticing real issues).

Also these duplicate entries do not seem to get safely discarded as they
result in a bunch of operations failing loudly at boot (e.g. the
oops_by_lvl warning above) and similarly at resume as I recently
noticed:

[   42.690569] CPU4: Booted secondary processor 0x0000010000 [0x511f0011]
[   42.704360] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
[   42.737865] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
[   42.752943] debugfs: File 'cpu5' in directory 'opp' already present!
[   42.759956] debugfs: File 'cpu6' in directory 'opp' already present!
[   42.766641] debugfs: File 'cpu7' in directory 'opp' already present!
...
[   42.855520] CPU8: Booted secondary processor 0x0000020000 [0x511f0011]
[   42.865188] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
[   42.898494] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
[   42.913559] debugfs: File 'cpu9' in directory 'opp' already present!
[   42.920265] debugfs: File 'cpu10' in directory 'opp' already present!
[   42.927029] debugfs: File 'cpu11' in directory 'opp' already present!

Perhaps you can find some way to filter out the unused, duplicate
entries for qualcomm fw so that all of these issues go away.

> > [    9.913506] debugfs: Directory 'NCC' with parent 'pm_genpd' already present!
> > [    9.922198] debugfs: Directory 'NCC' with parent 'pm_genpd' already present!
> 
> Yeah I did notice ^^ during dev, the series isn't the one introducing it
> so it shouldn't block the series acceptance. Meanwhile I'll spend some
> cycles to get this warn fixed.

I didn't try to track down where this comes from, but figured it could
be related to the duplicate entries. Either way, these are actually
errors (not just warnings) that need to be addressed in some way.

Johan

Re: [PATCH V6 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq

[Konrad Dybcio]

On 9.07.2024 11:13 AM, Johan Hovold wrote:
> Hi Sibi,
> 
> On Wed, Jul 03, 2024 at 01:29:11AM +0530, Sibi Sankar wrote:
>> On 7/2/24 21:25, Johan Hovold wrote:
>>> On Wed, Jun 12, 2024 at 06:10:56PM +0530, Sibi Sankar wrote:
>>>> Enable cpufreq on X1E80100 SoCs through the SCMI perf protocol node.
> 
>>> This series gives a nice performance boost on the x1e80100 CRD, but I'm
>>> seeing a bunch of warnings and errors that need to be addressed:
>>>
>>> [    9.533053] arm-scmi firmware:scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:0] - ret:-95. Using regular messaging.
>>> [    9.549458] arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
>>> [    9.563925] arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
>>> [    9.572835] arm-scmi firmware:scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:1] - ret:-95. Using regular messaging.
>>> [    9.609471] arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
>>> [    9.633341] arm-scmi firmware:scmi: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
>>> [    9.650000] arm-scmi firmware:scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:2] - ret:-95. Using regular messaging.
>>
>> X1E uses fast channels only for message-id: 7 (level set) and regular
>> channels for all the other messages. The spec doesn't mandate fast
>> channels for any of the supported message ids for the perf protocol.
>> So nothing to fix here.
> 
> I didn't look at this in any detail, but if the firmware is spec
> compliant you should not be spamming the logs with warnings. Not sure
> how best to address that, but you could, for example, add a quirk for
> qcom fw or at a minimum demote this mess to info level.
> 
> Also the failure to add oops_by_lvl appears to be a separate issue (e.g.
> related to the duplicate entries).
> 
>>> [    9.727098] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
>>> [    9.737157] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
>>> [    9.875039] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
>>> [    9.888428] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
>>
>> The duplicate entries reported by the perf protocol come directly from
>> the speed bins. I was told the duplicate entry with volt 0 is meant to
>> indicate a lower power way of achieving the said frequency at a lower
>> core count. We have no way of using it in the kernel and it gets safely
>> discarded. So again nothing to fix in the kernel.
> 
> Again, you should not be spamming the logs with warnings for things are
> benign (e.g. as it may prevent people from noticing real issues).
> 
> Also these duplicate entries do not seem to get safely discarded as they
> result in a bunch of operations failing loudly at boot (e.g. the
> oops_by_lvl warning above) and similarly at resume as I recently
> noticed:
> 
> [   42.690569] CPU4: Booted secondary processor 0x0000010000 [0x511f0011]
> [   42.704360] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
> [   42.737865] cpu cpu4: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
> [   42.752943] debugfs: File 'cpu5' in directory 'opp' already present!
> [   42.759956] debugfs: File 'cpu6' in directory 'opp' already present!
> [   42.766641] debugfs: File 'cpu7' in directory 'opp' already present!
> ...
> [   42.855520] CPU8: Booted secondary processor 0x0000020000 [0x511f0011]
> [   42.865188] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
> [   42.898494] cpu cpu8: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 3417600000, volt: 0, enabled: 1. New: freq: 3417600000, volt: 0, enabled: 1
> [   42.913559] debugfs: File 'cpu9' in directory 'opp' already present!
> [   42.920265] debugfs: File 'cpu10' in directory 'opp' already present!
> [   42.927029] debugfs: File 'cpu11' in directory 'opp' already present!
> 
> Perhaps you can find some way to filter out the unused, duplicate
> entries for qualcomm fw so that all of these issues go away.

I would say that the firmware should probably change the PSTATEs'
"enabled" state based on availability and report that to the OS..
Or the OS should know the conditions (enabled core count as you mentioned)
and decide whether it makes sense to shut down these cores, based on
workloads.. The latter sounds more sane..

The SCMI perf protocol already exposes power metrics (through opp->power)
for EAS purposes, so perhaps additional field could be added (cpu mask /
cpu count, depending on whether the specific cores being off is meaningful)
so that the OS can make more educated choices here.. otherwise this almost
looks like a hack that made it into the firmware because there was no
time left or something..

You mentioned that "We have no way of using it in the kernel", but is that
actually true? Can you not set that OPP if the conditions are met?

Konrad

Re: [PATCH V6 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq

[Konrad Dybcio]

On 12.06.2024 2:40 PM, Sibi Sankar wrote:
> Enable cpufreq on X1E80100 SoCs through the SCMI perf protocol node.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH V6 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq

[Konrad Dybcio]

On 16.07.2024 12:45 PM, Konrad Dybcio wrote:
> On 12.06.2024 2:40 PM, Sibi Sankar wrote:
>> Enable cpufreq on X1E80100 SoCs through the SCMI perf protocol node.
>>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
> 
> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Taking this back.. forgot about <ZoQjAWse2YxwyRJv@hovoldconsulting.com>

Konrad

Re: (subset) [PATCH V6 0/5] qcom: x1e80100: Enable CPUFreq

[Bjorn Andersson]

On Wed, 12 Jun 2024 18:10:51 +0530, Sibi Sankar wrote:
> This series enables CPUFreq support on the X1E SoC using the SCMI perf
> protocol. This was originally part of the RFC: firmware: arm_scmi:
> Qualcomm Vendor Protocol [1]. I've split it up so that this part can
> land earlier.
> 
> V5:
> * Fix build error reported by kernel test robot by adding 64BIT requirement
>   to COMPILE_TEST
> * Pick Rbs
> 
> [...]

Applied, thanks!

[3/5] arm64: dts: qcom: x1e80100: Resize GIC Redistributor register region
      commit: 9ed1a2b8784262e85ec300792a1a37ebd8473be2

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>