* [PATCH 1/5] dt-bindings: mailbox: qcom: Add CPUCP mailbox controller bindings
2024-03-28 9:50 [PATCH 0/5] qcom: x1e80100: Enable CPUFreq Sibi Sankar
@ 2024-03-28 9:50 ` Sibi Sankar
2024-03-28 20:54 ` Rob Herring
2024-03-28 9:50 ` [PATCH 2/5] mailbox: Add support for QTI CPUCP mailbox controller Sibi Sankar
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Sibi Sankar @ 2024-03-28 9:50 UTC (permalink / raw)
To: sudeep.holla, cristian.marussi, andersson, konrad.dybcio,
jassisinghbrar, robh+dt, krzysztof.kozlowski+dt
Cc: linux-kernel, linux-arm-msm, devicetree, quic_rgottimu,
quic_kshivnan, quic_sibis, conor+dt, quic_gkohli, quic_nkela,
quic_psodagud
Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox
controller.
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
rfc:
* Use x1e80100 as the fallback for future SoCs using the cpucp-mbox
controller. [Krzysztoff/Konrad/Rob]
.../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..491b0a05e630
--- /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@qti.qualcomm.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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 1/5] dt-bindings: mailbox: qcom: Add CPUCP mailbox controller bindings
2024-03-28 9:50 ` [PATCH 1/5] dt-bindings: mailbox: qcom: Add CPUCP mailbox controller bindings Sibi Sankar
@ 2024-03-28 20:54 ` Rob Herring
0 siblings, 0 replies; 17+ messages in thread
From: Rob Herring @ 2024-03-28 20:54 UTC (permalink / raw)
To: Sibi Sankar
Cc: sudeep.holla, cristian.marussi, andersson, konrad.dybcio,
jassisinghbrar, krzysztof.kozlowski+dt, linux-kernel,
linux-arm-msm, devicetree, quic_rgottimu, quic_kshivnan, conor+dt,
quic_gkohli, quic_nkela, quic_psodagud
On Thu, Mar 28, 2024 at 03:20:40PM +0530, Sibi Sankar wrote:
> Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox
> controller.
>
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
>
> rfc:
rfc is not a version, but a "state of the patch" tag. This should be v2.
> * Use x1e80100 as the fallback for future SoCs using the cpucp-mbox
> controller. [Krzysztoff/Konrad/Rob]
>
> .../bindings/mailbox/qcom,cpucp-mbox.yaml | 49 +++++++++++++++++++
> 1 file changed, 49 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/5] mailbox: Add support for QTI CPUCP mailbox controller
2024-03-28 9:50 [PATCH 0/5] qcom: x1e80100: Enable CPUFreq Sibi Sankar
2024-03-28 9:50 ` [PATCH 1/5] dt-bindings: mailbox: qcom: Add CPUCP mailbox controller bindings Sibi Sankar
@ 2024-03-28 9:50 ` Sibi Sankar
2024-04-16 16:21 ` Dmitry Baryshkov
2024-03-28 9:50 ` [PATCH 3/5] arm64: dts: qcom: x1e80100: Resize GIC Redistributor register region Sibi Sankar
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Sibi Sankar @ 2024-03-28 9:50 UTC (permalink / raw)
To: sudeep.holla, cristian.marussi, andersson, konrad.dybcio,
jassisinghbrar, robh+dt, krzysztof.kozlowski+dt
Cc: linux-kernel, linux-arm-msm, devicetree, quic_rgottimu,
quic_kshivnan, quic_sibis, conor+dt, quic_gkohli, quic_nkela,
quic_psodagud
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 <quic_sibis@quicinc.com>
---
rfc:
* 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.
drivers/mailbox/Kconfig | 8 ++
drivers/mailbox/Makefile | 2 +
drivers/mailbox/qcom-cpucp-mbox.c | 205 ++++++++++++++++++++++++++++++
3 files changed, 215 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..e27a258fe57a
--- /dev/null
+++ b/drivers/mailbox/qcom-cpucp-mbox.c
@@ -0,0 +1,205 @@
+// 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;
+};
+
+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;
+ struct mbox_chan *chan;
+ unsigned long flags;
+ u64 status;
+ u32 val;
+ int i;
+
+ status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
+
+ for (i = 0; i < APSS_CPUCP_IPC_CHAN_SUPPORTED; 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);
+ chan = &cpucp->chans[i];
+ spin_lock_irqsave(&chan->lock, flags);
+ if (chan->cl)
+ mbox_chan_received_data(chan, &val);
+ spin_unlock_irqrestore(&chan->lock, flags);
+ 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 = 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 * 8) + 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 qcom_cpucp_mbox *cpucp;
+ struct mbox_controller *mbox;
+ int ret;
+
+ cpucp = devm_kzalloc(&pdev->dev, sizeof(*cpucp), GFP_KERNEL);
+ if (!cpucp)
+ return -ENOMEM;
+
+ cpucp->dev = &pdev->dev;
+
+ cpucp->rx_base = devm_of_iomap(cpucp->dev, cpucp->dev->of_node, 0, NULL);
+ if (IS_ERR(cpucp->rx_base))
+ return PTR_ERR(cpucp->rx_base);
+
+ cpucp->tx_base = devm_of_iomap(cpucp->dev, cpucp->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);
+
+ 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);
+
+ mbox = &cpucp->mbox;
+ mbox->dev = cpucp->dev;
+ mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
+ mbox->chans = cpucp->chans;
+ mbox->ops = &qcom_cpucp_mbox_chan_ops;
+ mbox->txdone_irq = false;
+ mbox->txdone_poll = false;
+
+ ret = mbox_controller_register(mbox);
+ 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,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,
+ .remove = qcom_cpucp_mbox_remove,
+ .driver = {
+ .name = "qcom_cpucp_mbox",
+ .of_match_table = qcom_cpucp_mbox_of_match,
+ .suppress_bind_attrs = true,
+ },
+};
+module_platform_driver(qcom_cpucp_mbox_driver);
+
+MODULE_DESCRIPTION("QTI CPUCP MBOX Driver");
+MODULE_LICENSE("GPL");
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 2/5] mailbox: Add support for QTI CPUCP mailbox controller
2024-03-28 9:50 ` [PATCH 2/5] mailbox: Add support for QTI CPUCP mailbox controller Sibi Sankar
@ 2024-04-16 16:21 ` Dmitry Baryshkov
2024-04-17 11:51 ` Sibi Sankar
0 siblings, 1 reply; 17+ messages in thread
From: Dmitry Baryshkov @ 2024-04-16 16:21 UTC (permalink / raw)
To: Sibi Sankar
Cc: sudeep.holla, cristian.marussi, andersson, konrad.dybcio,
jassisinghbrar, robh+dt, krzysztof.kozlowski+dt, linux-kernel,
linux-arm-msm, devicetree, quic_rgottimu, quic_kshivnan, conor+dt,
quic_gkohli, quic_nkela, quic_psodagud
On Thu, 28 Mar 2024 at 11:52, Sibi Sankar <quic_sibis@quicinc.com> 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 <quic_sibis@quicinc.com>
> ---
>
> rfc:
> * 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.
>
> drivers/mailbox/Kconfig | 8 ++
> drivers/mailbox/Makefile | 2 +
> drivers/mailbox/qcom-cpucp-mbox.c | 205 ++++++++++++++++++++++++++++++
> 3 files changed, 215 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..e27a258fe57a
> --- /dev/null
> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
> @@ -0,0 +1,205 @@
> +// 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;
> +};
> +
> +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;
> + struct mbox_chan *chan;
> + unsigned long flags;
> + u64 status;
> + u32 val;
> + int i;
> +
> + status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
> +
> + for (i = 0; i < APSS_CPUCP_IPC_CHAN_SUPPORTED; i++) {
> + val = 0;
> + if (status & ((u64)1 << i)) {
BIT() or test_bit()
> + val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD + (i * 8) + APSS_CPUCP_MBOX_CMD_OFF);
#define APSS_CPUCP_MBOX_CMD_OFF(i)
> + chan = &cpucp->chans[i];
> + spin_lock_irqsave(&chan->lock, flags);
> + if (chan->cl)
> + mbox_chan_received_data(chan, &val);
> + spin_unlock_irqrestore(&chan->lock, flags);
> + writeq(status, cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
Why is status written from inside the loop? If the bits are cleared by
writing 1, then you should be writing BIT(i) to that register. Also
make sure that it is written at the correct time, so that if there is
an event before notifying the driver, it doesn't get lost.
> + }
> + }
> +
> + 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 * 8) + 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 qcom_cpucp_mbox *cpucp;
> + struct mbox_controller *mbox;
> + int ret;
> +
> + cpucp = devm_kzalloc(&pdev->dev, sizeof(*cpucp), GFP_KERNEL);
> + if (!cpucp)
> + return -ENOMEM;
> +
> + cpucp->dev = &pdev->dev;
> +
> + cpucp->rx_base = devm_of_iomap(cpucp->dev, cpucp->dev->of_node, 0, NULL);
> + if (IS_ERR(cpucp->rx_base))
> + return PTR_ERR(cpucp->rx_base);
> +
> + cpucp->tx_base = devm_of_iomap(cpucp->dev, cpucp->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);
> +
> + cpucp->irq = platform_get_irq(pdev, 0);
> + if (cpucp->irq < 0) {
> + dev_err(&pdev->dev, "Failed to get the IRQ\n");
> + return cpucp->irq;
It already prints the error message.
> + }
> +
> + 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;
return dev_err_probe();
> + }
> +
> + writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
> +
> + mbox = &cpucp->mbox;
> + mbox->dev = cpucp->dev;
> + mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
> + mbox->chans = cpucp->chans;
> + mbox->ops = &qcom_cpucp_mbox_chan_ops;
> + mbox->txdone_irq = false;
> + mbox->txdone_poll = false;
> +
> + ret = mbox_controller_register(mbox);
Use devm_mbox_controller_register()
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to create mailbox\n");
> + return ret;
return dev_err_probe();
> + }
> +
> + 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);
This will be replaced by devm_mbox_controller_register().
> +
> + 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,
> + .remove = qcom_cpucp_mbox_remove,
> + .driver = {
> + .name = "qcom_cpucp_mbox",
> + .of_match_table = qcom_cpucp_mbox_of_match,
> + .suppress_bind_attrs = true,
No need to. Please drop.
> + },
> +};
> +module_platform_driver(qcom_cpucp_mbox_driver);
> +
> +MODULE_DESCRIPTION("QTI CPUCP MBOX Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/5] mailbox: Add support for QTI CPUCP mailbox controller
2024-04-16 16:21 ` Dmitry Baryshkov
@ 2024-04-17 11:51 ` Sibi Sankar
2024-04-17 11:54 ` Dmitry Baryshkov
0 siblings, 1 reply; 17+ messages in thread
From: Sibi Sankar @ 2024-04-17 11:51 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: sudeep.holla, cristian.marussi, andersson, konrad.dybcio,
jassisinghbrar, robh+dt, krzysztof.kozlowski+dt, linux-kernel,
linux-arm-msm, devicetree, quic_rgottimu, quic_kshivnan, conor+dt,
quic_gkohli, quic_nkela, quic_psodagud
On 4/16/24 21:51, Dmitry Baryshkov wrote:
> On Thu, 28 Mar 2024 at 11:52, Sibi Sankar <quic_sibis@quicinc.com> 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 <quic_sibis@quicinc.com>
>> ---
>>
>> rfc:
>> * 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.
Hey Dmitry,
Thanks for taking time to review the series.
>>
>> drivers/mailbox/Kconfig | 8 ++
>> drivers/mailbox/Makefile | 2 +
>> drivers/mailbox/qcom-cpucp-mbox.c | 205 ++++++++++++++++++++++++++++++
>> 3 files changed, 215 insertions(+)
>> create mode 100644 drivers/mailbox/qcom-cpucp-mbox.c
>>
[snip]
...
>> +
>> + status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
>> +
>> + for (i = 0; i < APSS_CPUCP_IPC_CHAN_SUPPORTED; i++) {
>> + val = 0;
>> + if (status & ((u64)1 << i)) {
>
> BIT() or test_bit()
I'll use BIT()
>
>> + val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD + (i * 8) + APSS_CPUCP_MBOX_CMD_OFF);
>
> #define APSS_CPUCP_MBOX_CMD_OFF(i)
ack
>
>> + chan = &cpucp->chans[i];
>> + spin_lock_irqsave(&chan->lock, flags);
>> + if (chan->cl)
>> + mbox_chan_received_data(chan, &val);
>> + spin_unlock_irqrestore(&chan->lock, flags);
>> + writeq(status, cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
>
> Why is status written from inside the loop? If the bits are cleared by
> writing 1, then you should be writing BIT(i) to that register. Also
> make sure that it is written at the correct time, so that if there is
> an event before notifying the driver, it doesn't get lost.
Thanks for catching this. I probably didn't run into this scenario
because of using just one channel at point any time. I'll move it
outside the loop.
>
>> + }
>> + }
>> +
>> + 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 * 8) + 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 qcom_cpucp_mbox *cpucp;
>> + struct mbox_controller *mbox;
>> + int ret;
>> +
>> + cpucp = devm_kzalloc(&pdev->dev, sizeof(*cpucp), GFP_KERNEL);
>> + if (!cpucp)
>> + return -ENOMEM;
>> +
>> + cpucp->dev = &pdev->dev;
>> +
>> + cpucp->rx_base = devm_of_iomap(cpucp->dev, cpucp->dev->of_node, 0, NULL);
>> + if (IS_ERR(cpucp->rx_base))
>> + return PTR_ERR(cpucp->rx_base);
>> +
>> + cpucp->tx_base = devm_of_iomap(cpucp->dev, cpucp->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);
>> +
>> + cpucp->irq = platform_get_irq(pdev, 0);
>> + if (cpucp->irq < 0) {
>> + dev_err(&pdev->dev, "Failed to get the IRQ\n");
>> + return cpucp->irq;
>
> It already prints the error message.
ack
>
>> + }
>> +
>> + 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;
>
> return dev_err_probe();
ack
>
>> + }
>> +
>> + writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
>> +
>> + mbox = &cpucp->mbox;
>> + mbox->dev = cpucp->dev;
>> + mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
>> + mbox->chans = cpucp->chans;
>> + mbox->ops = &qcom_cpucp_mbox_chan_ops;
>> + mbox->txdone_irq = false;
>> + mbox->txdone_poll = false;
>> +
>> + ret = mbox_controller_register(mbox);
>
> Use devm_mbox_controller_register()
ack
> >> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to create mailbox\n");
>> + return ret;
>
> return dev_err_probe();
I guess ^^ is a typo? Since devm_mbox_controller_register wouldn't
return -EPROBE_DEFER.
>
>> + }
>> +
>> + 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);
> > This will be replaced by devm_mbox_controller_register().
ack
>
>> +
>> + 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,
>> + .remove = qcom_cpucp_mbox_remove,
>> + .driver = {
>> + .name = "qcom_cpucp_mbox",
>> + .of_match_table = qcom_cpucp_mbox_of_match,
>> + .suppress_bind_attrs = true,
>
> No need to. Please drop.
ack
-Sibi
>
>> + },
>> +};
>> +module_platform_driver(qcom_cpucp_mbox_driver);
>> +
>> +MODULE_DESCRIPTION("QTI CPUCP MBOX Driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.34.1
>>
>>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/5] mailbox: Add support for QTI CPUCP mailbox controller
2024-04-17 11:51 ` Sibi Sankar
@ 2024-04-17 11:54 ` Dmitry Baryshkov
2024-04-17 13:28 ` Sibi Sankar
0 siblings, 1 reply; 17+ messages in thread
From: Dmitry Baryshkov @ 2024-04-17 11:54 UTC (permalink / raw)
To: Sibi Sankar
Cc: sudeep.holla, cristian.marussi, andersson, konrad.dybcio,
jassisinghbrar, robh+dt, krzysztof.kozlowski+dt, linux-kernel,
linux-arm-msm, devicetree, quic_rgottimu, quic_kshivnan, conor+dt,
quic_gkohli, quic_nkela, quic_psodagud
On Wed, 17 Apr 2024 at 14:51, Sibi Sankar <quic_sibis@quicinc.com> wrote:
>
>
>
> On 4/16/24 21:51, Dmitry Baryshkov wrote:
> > On Thu, 28 Mar 2024 at 11:52, Sibi Sankar <quic_sibis@quicinc.com> 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 <quic_sibis@quicinc.com>
> >> ---
> >>
> >> rfc:
> >> * 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.
>
> Hey Dmitry,
>
> Thanks for taking time to review the series.
>
> >>
> >> drivers/mailbox/Kconfig | 8 ++
> >> drivers/mailbox/Makefile | 2 +
> >> drivers/mailbox/qcom-cpucp-mbox.c | 205 ++++++++++++++++++++++++++++++
> >> 3 files changed, 215 insertions(+)
> >> create mode 100644 drivers/mailbox/qcom-cpucp-mbox.c
> >>
> [snip]
> ...
> >> +
> >> + status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
> >> +
> >> + for (i = 0; i < APSS_CPUCP_IPC_CHAN_SUPPORTED; i++) {
> >> + val = 0;
> >> + if (status & ((u64)1 << i)) {
> >
> > BIT() or test_bit()
>
> I'll use BIT()
>
> >
> >> + val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD + (i * 8) + APSS_CPUCP_MBOX_CMD_OFF);
> >
> > #define APSS_CPUCP_MBOX_CMD_OFF(i)
>
> ack
>
> >
> >> + chan = &cpucp->chans[i];
> >> + spin_lock_irqsave(&chan->lock, flags);
> >> + if (chan->cl)
> >> + mbox_chan_received_data(chan, &val);
> >> + spin_unlock_irqrestore(&chan->lock, flags);
> >> + writeq(status, cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
> >
> > Why is status written from inside the loop? If the bits are cleared by
> > writing 1, then you should be writing BIT(i) to that register. Also
> > make sure that it is written at the correct time, so that if there is
> > an event before notifying the driver, it doesn't get lost.
>
> Thanks for catching this. I probably didn't run into this scenario
> because of using just one channel at point any time. I'll move it
> outside the loop.
It might be better to write single bits from within the loop under the spinlock.
>
> >
> >> + }
> >> + }
> >> +
> >> + 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 * 8) + 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 qcom_cpucp_mbox *cpucp;
> >> + struct mbox_controller *mbox;
> >> + int ret;
> >> +
> >> + cpucp = devm_kzalloc(&pdev->dev, sizeof(*cpucp), GFP_KERNEL);
> >> + if (!cpucp)
> >> + return -ENOMEM;
> >> +
> >> + cpucp->dev = &pdev->dev;
> >> +
> >> + cpucp->rx_base = devm_of_iomap(cpucp->dev, cpucp->dev->of_node, 0, NULL);
> >> + if (IS_ERR(cpucp->rx_base))
> >> + return PTR_ERR(cpucp->rx_base);
> >> +
> >> + cpucp->tx_base = devm_of_iomap(cpucp->dev, cpucp->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);
> >> +
> >> + cpucp->irq = platform_get_irq(pdev, 0);
> >> + if (cpucp->irq < 0) {
> >> + dev_err(&pdev->dev, "Failed to get the IRQ\n");
> >> + return cpucp->irq;
> >
> > It already prints the error message.
>
> ack
>
> >
> >> + }
> >> +
> >> + 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;
> >
> > return dev_err_probe();
>
> ack
>
> >
> >> + }
> >> +
> >> + writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
> >> +
> >> + mbox = &cpucp->mbox;
> >> + mbox->dev = cpucp->dev;
> >> + mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
> >> + mbox->chans = cpucp->chans;
> >> + mbox->ops = &qcom_cpucp_mbox_chan_ops;
> >> + mbox->txdone_irq = false;
> >> + mbox->txdone_poll = false;
> >> +
> >> + ret = mbox_controller_register(mbox);
> >
> > Use devm_mbox_controller_register()
>
> ack
>
> > >> + if (ret) {
> >> + dev_err(&pdev->dev, "Failed to create mailbox\n");
> >> + return ret;
> >
> > return dev_err_probe();
>
> I guess ^^ is a typo? Since devm_mbox_controller_register wouldn't
> return -EPROBE_DEFER.
Anyway, using dev_err_probe is a simpler and better style. It's not a
question of returning -EPROBE_DEFER.
>
> >
> >> + }
> >> +
> >> + 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);
> > > This will be replaced by devm_mbox_controller_register().
>
> ack
>
> >
> >> +
> >> + 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,
> >> + .remove = qcom_cpucp_mbox_remove,
> >> + .driver = {
> >> + .name = "qcom_cpucp_mbox",
> >> + .of_match_table = qcom_cpucp_mbox_of_match,
> >> + .suppress_bind_attrs = true,
> >
> > No need to. Please drop.
>
> ack
>
> -Sibi
>
> >
> >> + },
> >> +};
> >> +module_platform_driver(qcom_cpucp_mbox_driver);
> >> +
> >> +MODULE_DESCRIPTION("QTI CPUCP MBOX Driver");
> >> +MODULE_LICENSE("GPL");
> >> --
> >> 2.34.1
> >>
> >>
> >
> >
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/5] mailbox: Add support for QTI CPUCP mailbox controller
2024-04-17 11:54 ` Dmitry Baryshkov
@ 2024-04-17 13:28 ` Sibi Sankar
0 siblings, 0 replies; 17+ messages in thread
From: Sibi Sankar @ 2024-04-17 13:28 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: sudeep.holla, cristian.marussi, andersson, konrad.dybcio,
jassisinghbrar, robh+dt, krzysztof.kozlowski+dt, linux-kernel,
linux-arm-msm, devicetree, quic_rgottimu, quic_kshivnan, conor+dt,
quic_gkohli, quic_nkela, quic_psodagud
On 4/17/24 17:24, Dmitry Baryshkov wrote:
> On Wed, 17 Apr 2024 at 14:51, Sibi Sankar <quic_sibis@quicinc.com> wrote:
>>
>>
>>
>> On 4/16/24 21:51, Dmitry Baryshkov wrote:
>>> On Thu, 28 Mar 2024 at 11:52, Sibi Sankar <quic_sibis@quicinc.com> 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 <quic_sibis@quicinc.com>
>>>> ---
>>>>
>>>> rfc:
>>>> * 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.
>>
>> Hey Dmitry,
>>
>> Thanks for taking time to review the series.
>>
>>>>
>>>> drivers/mailbox/Kconfig | 8 ++
>>>> drivers/mailbox/Makefile | 2 +
>>>> drivers/mailbox/qcom-cpucp-mbox.c | 205 ++++++++++++++++++++++++++++++
>>>> 3 files changed, 215 insertions(+)
>>>> create mode 100644 drivers/mailbox/qcom-cpucp-mbox.c
>>>>
>> [snip]
>> ...
>>>> +
>>>> + status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
>>>> +
>>>> + for (i = 0; i < APSS_CPUCP_IPC_CHAN_SUPPORTED; i++) {
>>>> + val = 0;
>>>> + if (status & ((u64)1 << i)) {
>>>
>>> BIT() or test_bit()
>>
>> I'll use BIT()
>>
>>>
>>>> + val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD + (i * 8) + APSS_CPUCP_MBOX_CMD_OFF);
>>>
>>> #define APSS_CPUCP_MBOX_CMD_OFF(i)
>>
>> ack
>>
>>>
>>>> + chan = &cpucp->chans[i];
>>>> + spin_lock_irqsave(&chan->lock, flags);
>>>> + if (chan->cl)
>>>> + mbox_chan_received_data(chan, &val);
>>>> + spin_unlock_irqrestore(&chan->lock, flags);
>>>> + writeq(status, cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
>>>
>>> Why is status written from inside the loop? If the bits are cleared by
>>> writing 1, then you should be writing BIT(i) to that register. Also
>>> make sure that it is written at the correct time, so that if there is
>>> an event before notifying the driver, it doesn't get lost.
>>
>> Thanks for catching this. I probably didn't run into this scenario
>> because of using just one channel at point any time. I'll move it
>> outside the loop.
>
> It might be better to write single bits from within the loop under the spinlock.
Sure, will do that instead.
>
>>
>>>
>>>> + }
>>>> + }
>>>> +
>>>> + 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 * 8) + 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 qcom_cpucp_mbox *cpucp;
>>>> + struct mbox_controller *mbox;
>>>> + int ret;
>>>> +
>>>> + cpucp = devm_kzalloc(&pdev->dev, sizeof(*cpucp), GFP_KERNEL);
>>>> + if (!cpucp)
>>>> + return -ENOMEM;
>>>> +
>>>> + cpucp->dev = &pdev->dev;
>>>> +
>>>> + cpucp->rx_base = devm_of_iomap(cpucp->dev, cpucp->dev->of_node, 0, NULL);
>>>> + if (IS_ERR(cpucp->rx_base))
>>>> + return PTR_ERR(cpucp->rx_base);
>>>> +
>>>> + cpucp->tx_base = devm_of_iomap(cpucp->dev, cpucp->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);
>>>> +
>>>> + cpucp->irq = platform_get_irq(pdev, 0);
>>>> + if (cpucp->irq < 0) {
>>>> + dev_err(&pdev->dev, "Failed to get the IRQ\n");
>>>> + return cpucp->irq;
>>>
>>> It already prints the error message.
>>
>> ack
>>
>>>
>>>> + }
>>>> +
>>>> + 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;
>>>
>>> return dev_err_probe();
>>
>> ack
>>
>>>
>>>> + }
>>>> +
>>>> + writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
>>>> +
>>>> + mbox = &cpucp->mbox;
>>>> + mbox->dev = cpucp->dev;
>>>> + mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
>>>> + mbox->chans = cpucp->chans;
>>>> + mbox->ops = &qcom_cpucp_mbox_chan_ops;
>>>> + mbox->txdone_irq = false;
>>>> + mbox->txdone_poll = false;
>>>> +
>>>> + ret = mbox_controller_register(mbox);
>>>
>>> Use devm_mbox_controller_register()
>>
>> ack
>>
>>> >> + if (ret) {
>>>> + dev_err(&pdev->dev, "Failed to create mailbox\n");
>>>> + return ret;
>>>
>>> return dev_err_probe();
>>
>> I guess ^^ is a typo? Since devm_mbox_controller_register wouldn't
>> return -EPROBE_DEFER.
>
> Anyway, using dev_err_probe is a simpler and better style. It's not a
> question of returning -EPROBE_DEFER.
ack
-Sibi
>
>>
>>>
>>>> + }
>>>> +
>>>> + 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);
>>> > This will be replaced by devm_mbox_controller_register().
>>
>> ack
>>
>>>
>>>> +
>>>> + 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,
>>>> + .remove = qcom_cpucp_mbox_remove,
>>>> + .driver = {
>>>> + .name = "qcom_cpucp_mbox",
>>>> + .of_match_table = qcom_cpucp_mbox_of_match,
>>>> + .suppress_bind_attrs = true,
>>>
>>> No need to. Please drop.
>>
>> ack
>>
>> -Sibi
>>
>>>
>>>> + },
>>>> +};
>>>> +module_platform_driver(qcom_cpucp_mbox_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("QTI CPUCP MBOX Driver");
>>>> +MODULE_LICENSE("GPL");
>>>> --
>>>> 2.34.1
>>>>
>>>>
>>>
>>>
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/5] arm64: dts: qcom: x1e80100: Resize GIC Redistributor register region
2024-03-28 9:50 [PATCH 0/5] qcom: x1e80100: Enable CPUFreq Sibi Sankar
2024-03-28 9:50 ` [PATCH 1/5] dt-bindings: mailbox: qcom: Add CPUCP mailbox controller bindings Sibi Sankar
2024-03-28 9:50 ` [PATCH 2/5] mailbox: Add support for QTI CPUCP mailbox controller Sibi Sankar
@ 2024-03-28 9:50 ` Sibi Sankar
2024-04-16 16:31 ` Dmitry Baryshkov
2024-03-28 9:50 ` [PATCH 4/5] arm64: dts: qcom: x1e80100: Add cpucp mailbox and sram nodes Sibi Sankar
2024-03-28 9:50 ` [PATCH 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq Sibi Sankar
4 siblings, 1 reply; 17+ messages in thread
From: Sibi Sankar @ 2024-03-28 9:50 UTC (permalink / raw)
To: sudeep.holla, cristian.marussi, andersson, konrad.dybcio,
jassisinghbrar, robh+dt, krzysztof.kozlowski+dt
Cc: linux-kernel, linux-arm-msm, devicetree, quic_rgottimu,
quic_kshivnan, quic_sibis, conor+dt, quic_gkohli, quic_nkela,
quic_psodagud
Resize the GICR register region as it currently seeps into the CPU Control
Processor mailbox RX region.
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
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 f5a3b39ae70e..28f65296781d 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
@@ -4949,7 +4949,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 0x380000>; /* GICR * 12 */
interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 3/5] arm64: dts: qcom: x1e80100: Resize GIC Redistributor register region
2024-03-28 9:50 ` [PATCH 3/5] arm64: dts: qcom: x1e80100: Resize GIC Redistributor register region Sibi Sankar
@ 2024-04-16 16:31 ` Dmitry Baryshkov
0 siblings, 0 replies; 17+ messages in thread
From: Dmitry Baryshkov @ 2024-04-16 16:31 UTC (permalink / raw)
To: Sibi Sankar
Cc: sudeep.holla, cristian.marussi, andersson, konrad.dybcio,
jassisinghbrar, robh+dt, krzysztof.kozlowski+dt, linux-kernel,
linux-arm-msm, devicetree, quic_rgottimu, quic_kshivnan, conor+dt,
quic_gkohli, quic_nkela, quic_psodagud
On Thu, 28 Mar 2024 at 11:53, Sibi Sankar <quic_sibis@quicinc.com> wrote:
>
> Resize the GICR register region as it currently seeps into the CPU Control
> Processor mailbox RX region.
>
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
> arch/arm64/boot/dts/qcom/x1e80100.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4/5] arm64: dts: qcom: x1e80100: Add cpucp mailbox and sram nodes
2024-03-28 9:50 [PATCH 0/5] qcom: x1e80100: Enable CPUFreq Sibi Sankar
` (2 preceding siblings ...)
2024-03-28 9:50 ` [PATCH 3/5] arm64: dts: qcom: x1e80100: Resize GIC Redistributor register region Sibi Sankar
@ 2024-03-28 9:50 ` Sibi Sankar
2024-04-16 16:30 ` Dmitry Baryshkov
2024-03-28 9:50 ` [PATCH 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq Sibi Sankar
4 siblings, 1 reply; 17+ messages in thread
From: Sibi Sankar @ 2024-03-28 9:50 UTC (permalink / raw)
To: sudeep.holla, cristian.marussi, andersson, konrad.dybcio,
jassisinghbrar, robh+dt, krzysztof.kozlowski+dt
Cc: linux-kernel, linux-arm-msm, devicetree, quic_rgottimu,
quic_kshivnan, quic_sibis, conor+dt, quic_gkohli, quic_nkela,
quic_psodagud
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 28f65296781d..4e0ec859ed61 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
@@ -4974,6 +4974,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>,
@@ -5157,6 +5164,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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 4/5] arm64: dts: qcom: x1e80100: Add cpucp mailbox and sram nodes
2024-03-28 9:50 ` [PATCH 4/5] arm64: dts: qcom: x1e80100: Add cpucp mailbox and sram nodes Sibi Sankar
@ 2024-04-16 16:30 ` Dmitry Baryshkov
2024-04-17 11:52 ` Sibi Sankar
0 siblings, 1 reply; 17+ messages in thread
From: Dmitry Baryshkov @ 2024-04-16 16:30 UTC (permalink / raw)
To: Sibi Sankar
Cc: sudeep.holla, cristian.marussi, andersson, konrad.dybcio,
jassisinghbrar, robh+dt, krzysztof.kozlowski+dt, linux-kernel,
linux-arm-msm, devicetree, quic_rgottimu, quic_kshivnan, conor+dt,
quic_gkohli, quic_nkela, quic_psodagud
On Thu, 28 Mar 2024 at 11:52, Sibi Sankar <quic_sibis@quicinc.com> wrote:
>
> 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 28f65296781d..4e0ec859ed61 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> @@ -4974,6 +4974,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>,
> @@ -5157,6 +5164,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 {
This doesn't seem to follow the schema.
> + 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
>
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 4/5] arm64: dts: qcom: x1e80100: Add cpucp mailbox and sram nodes
2024-04-16 16:30 ` Dmitry Baryshkov
@ 2024-04-17 11:52 ` Sibi Sankar
0 siblings, 0 replies; 17+ messages in thread
From: Sibi Sankar @ 2024-04-17 11:52 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: sudeep.holla, cristian.marussi, andersson, konrad.dybcio,
jassisinghbrar, robh+dt, krzysztof.kozlowski+dt, linux-kernel,
linux-arm-msm, devicetree, quic_rgottimu, quic_kshivnan, conor+dt,
quic_gkohli, quic_nkela, quic_psodagud
On 4/16/24 22:00, Dmitry Baryshkov wrote:
> On Thu, 28 Mar 2024 at 11:52, Sibi Sankar <quic_sibis@quicinc.com> wrote:
>>
>> 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 28f65296781d..4e0ec859ed61 100644
>> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> @@ -4974,6 +4974,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>,
>> @@ -5157,6 +5164,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 {
>
> This doesn't seem to follow the schema.
ack, will rename the nodes in the next re-spin.
-Sibi
>
>> + 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
>>
>>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq
2024-03-28 9:50 [PATCH 0/5] qcom: x1e80100: Enable CPUFreq Sibi Sankar
` (3 preceding siblings ...)
2024-03-28 9:50 ` [PATCH 4/5] arm64: dts: qcom: x1e80100: Add cpucp mailbox and sram nodes Sibi Sankar
@ 2024-03-28 9:50 ` Sibi Sankar
2024-04-02 11:09 ` Sudeep Holla
4 siblings, 1 reply; 17+ messages in thread
From: Sibi Sankar @ 2024-03-28 9:50 UTC (permalink / raw)
To: sudeep.holla, cristian.marussi, andersson, konrad.dybcio,
jassisinghbrar, robh+dt, krzysztof.kozlowski+dt
Cc: linux-kernel, linux-arm-msm, devicetree, quic_rgottimu,
quic_kshivnan, quic_sibis, conor+dt, quic_gkohli, quic_nkela,
quic_psodagud
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 | 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 4e0ec859ed61..d1d232cd1f25 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
@@ -68,6 +68,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";
@@ -85,6 +86,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";
@@ -96,6 +98,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";
@@ -107,6 +110,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";
@@ -118,6 +122,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";
@@ -135,6 +140,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";
@@ -146,6 +152,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";
@@ -157,6 +164,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";
@@ -168,6 +176,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";
@@ -185,6 +194,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";
@@ -196,6 +206,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";
@@ -207,6 +218,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";
@@ -309,6 +321,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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq
2024-03-28 9:50 ` [PATCH 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq Sibi Sankar
@ 2024-04-02 11:09 ` Sudeep Holla
2024-04-03 11:20 ` Ulf Hansson
0 siblings, 1 reply; 17+ messages in thread
From: Sudeep Holla @ 2024-04-02 11:09 UTC (permalink / raw)
To: Sibi Sankar
Cc: cristian.marussi, andersson, konrad.dybcio, jassisinghbrar,
robh+dt, krzysztof.kozlowski+dt, linux-kernel, linux-arm-msm,
devicetree, quic_rgottimu, quic_kshivnan, conor+dt, quic_gkohli,
quic_nkela, Ulf Hansson, quic_psodagud
On Thu, Mar 28, 2024 at 03:20:44PM +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 | 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 4e0ec859ed61..d1d232cd1f25 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> @@ -68,6 +68,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";
Any reason why you wouldn't want to use the new genpd based perf controls.
IIRC it was added based on mainly Qcom platform requirements.
- clocks = <&scmi_dvfs 0>;
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";
And the associated changes in the scmi dvfs node for cells property.
This change is OK but just wanted to check the reasoning for the choice.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq
2024-04-02 11:09 ` Sudeep Holla
@ 2024-04-03 11:20 ` Ulf Hansson
2024-04-04 6:32 ` Sibi Sankar
0 siblings, 1 reply; 17+ messages in thread
From: Ulf Hansson @ 2024-04-03 11:20 UTC (permalink / raw)
To: Sudeep Holla
Cc: Sibi Sankar, cristian.marussi, andersson, konrad.dybcio,
jassisinghbrar, robh+dt, krzysztof.kozlowski+dt, linux-kernel,
linux-arm-msm, devicetree, quic_rgottimu, quic_kshivnan, conor+dt,
quic_gkohli, quic_nkela, quic_psodagud
On Tue, 2 Apr 2024 at 13:10, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Mar 28, 2024 at 03:20:44PM +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 | 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 4e0ec859ed61..d1d232cd1f25 100644
> > --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> > @@ -68,6 +68,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";
>
>
> Any reason why you wouldn't want to use the new genpd based perf controls.
> IIRC it was added based on mainly Qcom platform requirements.
>
> - clocks = <&scmi_dvfs 0>;
> 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";
>
>
> And the associated changes in the scmi dvfs node for cells property.
>
> This change is OK but just wanted to check the reasoning for the choice.
To me, it seems reasonable to move to the new binding with
#power-domain-cells for protocol@13. This becomes more future proof,
as it can then easily be extended to be used beyond CPUs.
That said, I just submitted a patch [1] to update the examples in the
scmi DT doc to use #power-domain-cells in favor of #clock-cells. I
don't know if there is a better way to promote the new bindings?
Perhaps moving Juno to use this too?
Kind regards
Uffe
[1]
https://lore.kernel.org/all/20240403111106.1110940-1-ulf.hansson@linaro.org/
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq
2024-04-03 11:20 ` Ulf Hansson
@ 2024-04-04 6:32 ` Sibi Sankar
0 siblings, 0 replies; 17+ messages in thread
From: Sibi Sankar @ 2024-04-04 6:32 UTC (permalink / raw)
To: Ulf Hansson, Sudeep Holla
Cc: cristian.marussi, andersson, konrad.dybcio, jassisinghbrar,
robh+dt, krzysztof.kozlowski+dt, linux-kernel, linux-arm-msm,
devicetree, quic_rgottimu, quic_kshivnan, conor+dt, quic_gkohli,
quic_nkela, quic_psodagud
On 4/3/24 16:50, Ulf Hansson wrote:
> On Tue, 2 Apr 2024 at 13:10, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>> On Thu, Mar 28, 2024 at 03:20:44PM +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 | 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 4e0ec859ed61..d1d232cd1f25 100644
>>> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>>> @@ -68,6 +68,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";
>>
>>
>> Any reason why you wouldn't want to use the new genpd based perf controls.
>> IIRC it was added based on mainly Qcom platform requirements.
>>
>> - clocks = <&scmi_dvfs 0>;
>> 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";
>>
>>
>> And the associated changes in the scmi dvfs node for cells property.
>>
>> This change is OK but just wanted to check the reasoning for the choice.
>
> To me, it seems reasonable to move to the new binding with
> #power-domain-cells for protocol@13. This becomes more future proof,
> as it can then easily be extended to be used beyond CPUs.
>
> That said, I just submitted a patch [1] to update the examples in the
> scmi DT doc to use #power-domain-cells in favor of #clock-cells. I
> don't know if there is a better way to promote the new bindings?
> Perhaps moving Juno to use this too?
>
> Kind regards
> Uffe
Sudeep/Ulfe,
Thanks I'll move to the new recommendation.
-Sibi
>
> [1]
> https://lore.kernel.org/all/20240403111106.1110940-1-ulf.hansson@linaro.org/
^ permalink raw reply [flat|nested] 17+ messages in thread