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.
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/[email protected]/#r
Other relevant Links:
https://lore.kernel.org/lkml/[email protected]/
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 +++++
arch/arm64/boot/dts/qcom/x1e80100.dtsi | 91 ++++++---
drivers/mailbox/Kconfig | 8 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/qcom-cpucp-mbox.c | 188 ++++++++++++++++++
5 files changed, 313 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
Add the cpucp mailbox and sram nodes required by SCMI perf protocol
on X1E80100 SoCs.
Signed-off-by: Sibi Sankar <[email protected]>
---
v2:
* Rename sram sub-nodes according to schema. [Dmitry]
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..f40e95d49370 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>;
+
+ #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
Enable cpufreq on X1E80100 SoCs through the SCMI perf protocol node.
Signed-off-by: Sibi Sankar <[email protected]>
---
v2:
* Use power-domain instead of clocks. [Sudeep/Ulf]
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 f40e95d49370..42bbe58dfbf9 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
Resize the GICR register region as it currently seeps into the CPU Control
Processor mailbox RX region.
Reviewed-by: Dmitry Baryshkov <[email protected]>
Signed-off-by: Sibi Sankar <[email protected]>
---
v2:
* Pickup Rb from Dimitry.
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
Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox
controller.
Reviewed-by: Rob Herring <[email protected]>
Signed-off-by: Sibi Sankar <[email protected]>
---
v2:
* Pickup Rb from Dimitry.
.../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 <[email protected]>
+
+description:
+ The CPUSS Control Processor (CPUCP) mailbox controller enables communication
+ between AP and CPUCP by acting as a doorbell between them.
+
+properties:
+ compatible:
+ items:
+ - 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
Add support for CPUSS Control Processor (CPUCP) mailbox controller,
this driver enables communication between AP and CPUCP by acting as
a doorbell between them.
Signed-off-by: Sibi Sankar <[email protected]>
---
v2:
* 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.
drivers/mailbox/Kconfig | 8 ++
drivers/mailbox/Makefile | 2 +
drivers/mailbox/qcom-cpucp-mbox.c | 188 ++++++++++++++++++++++++++++++
3 files changed, 198 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..059eb25f217c
--- /dev/null
+++ b/drivers/mailbox/qcom-cpucp-mbox.c
@@ -0,0 +1,188 @@
+// 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(i) (0x100 + ((i) * 8))
+
+/* Rx Registers */
+#define APSS_CPUCP_RX_MBOX_IDR 0
+#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 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 & BIT(i)) {
+ val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
+ chan = &cpucp->chans[i];
+ 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 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)
+ return cpucp->irq;
+
+ ret = devm_request_irq(cpucp->dev, cpucp->irq, qcom_cpucp_mbox_irq_fn,
+ IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
+ if (ret < 0)
+ return dev_err_probe(cpucp->dev, ret, "Failed to register irq: %d\n", cpucp->irq);
+
+ 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 = devm_mbox_controller_register(cpucp->dev, mbox);
+ if (ret)
+ return dev_err_probe(cpucp->dev, ret, "Failed to create mailbox\n");
+
+ platform_set_drvdata(pdev, cpucp);
+
+ 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,
+ },
+};
+module_platform_driver(qcom_cpucp_mbox_driver);
+
+MODULE_DESCRIPTION("QTI CPUCP MBOX Driver");
+MODULE_LICENSE("GPL");
--
2.34.1
On Wed, 17 Apr 2024 at 16:29, Sibi Sankar <[email protected]> wrote:
>
> Add support for CPUSS Control Processor (CPUCP) mailbox controller,
> this driver enables communication between AP and CPUCP by acting as
> a doorbell between them.
>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
>
> v2:
> * 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.
>
> drivers/mailbox/Kconfig | 8 ++
> drivers/mailbox/Makefile | 2 +
> drivers/mailbox/qcom-cpucp-mbox.c | 188 ++++++++++++++++++++++++++++++
> 3 files changed, 198 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..059eb25f217c
> --- /dev/null
> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
> @@ -0,0 +1,188 @@
> +// 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
I don't see _IDR defines being used.
Other than that:
Reviewed-by: Dmitry Baryshkov <[email protected]>
> +#define APSS_CPUCP_TX_MBOX_CMD(i) (0x100 + ((i) * 8))
> +
> +/* Rx Registers */
> +#define APSS_CPUCP_RX_MBOX_IDR 0
> +#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 0xFFFFFFFFFFFFFFFF
--
With best wishes
Dmitry
On Wed, Apr 17, 2024 at 06:58:52PM +0530, Sibi Sankar wrote:
> Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox
> controller.
>
> Reviewed-by: Rob Herring <[email protected]>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
>
> v2:
> * Pickup Rb from Dimitry.
>
> .../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 <[email protected]>
That doesn't look like the correct domain.
Regards,
Bjorn
On Wed, Apr 17, 2024 at 06:58:54PM +0530, Sibi Sankar wrote:
> Resize the GICR register region as it currently seeps into the CPU Control
> Processor mailbox RX region.
>
Not that anyone is running a stable kernel here, but please make a habit
of adding Fixes: tags when correcting previous mistakes.
> Reviewed-by: Dmitry Baryshkov <[email protected]>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
>
> v2:
> * Pickup Rb from Dimitry.
>
> 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 */
>
The 12th GICR ends a bit before that, and per your commit message you're
just nudging it down to get this range out of the say of your other
range - rather than giving it a proper value.
Wouldn't 0x300000 be a better value here? Or am I perhaps missing
something in the difference? If so, I'd like the commit message to state
what, so someone doesn't get excited and correct/break it later.
Regards,
Bjorn
> interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
>
> --
> 2.34.1
>
On Wed, Apr 17, 2024 at 06:58:53PM +0530, Sibi Sankar wrote:
> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
> new file mode 100644
> index 000000000000..059eb25f217c
> --- /dev/null
> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
Nope.
> + */
> +
> +#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(i) (0x100 + ((i) * 8))
> +
> +/* Rx Registers */
> +#define APSS_CPUCP_RX_MBOX_IDR 0
> +#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
Can we have lower case hex digits, plz?
> +#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
@dev and @irq can be a local variables in qcom_cpucp_mbox_probe().
> + */
> +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;
This value is immediately overwritten (or unused).
> + if (status & BIT(i)) {
Can't you combine the for loop and this conditional into a
for_each_bit_set()?
> + val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
> + chan = &cpucp->chans[i];
> + spin_lock_irqsave(&chan->lock, flags);
Can you please add a comment here to document that the lock is taken
here to deal with races against client registration? (It wasn't obvious
to me...)
> + 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 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)
> + return cpucp->irq;
> +
> + ret = devm_request_irq(cpucp->dev, cpucp->irq, qcom_cpucp_mbox_irq_fn,
> + IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
> + if (ret < 0)
> + return dev_err_probe(cpucp->dev, ret, "Failed to register irq: %d\n", cpucp->irq);
> +
> + 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 = devm_mbox_controller_register(cpucp->dev, mbox);
> + if (ret)
> + return dev_err_probe(cpucp->dev, ret, "Failed to create mailbox\n");
> +
> + platform_set_drvdata(pdev, cpucp);
I don't see you using the drvdata anywhere, can we drop this?
> +
> + return 0;
> +}
> +
> +static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
> + { .compatible = "qcom,x1e80100-cpucp-mbox"},
A space after the final '"' would be good for aesthetics.
Regards,
Bjorn
> + {}
> +};
> +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,
> + },
> +};
> +module_platform_driver(qcom_cpucp_mbox_driver);
> +
> +MODULE_DESCRIPTION("QTI CPUCP MBOX Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
On 4/18/24 02:13, Bjorn Andersson wrote:
> On Wed, Apr 17, 2024 at 06:58:52PM +0530, Sibi Sankar wrote:
>> Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox
>> controller.
>>
>> Reviewed-by: Rob Herring <[email protected]>
>> Signed-off-by: Sibi Sankar <[email protected]>
>> ---
>>
>> v2:
>> * Pickup Rb from Dimitry.
>>
>> .../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 <[email protected]>
>
> That doesn't look like the correct domain.
Thanks, I don't know how I even constructed this chimera of an email-id
lol.
-Sibi
>
> Regards,
> Bjorn
On 4/18/24 02:56, Bjorn Andersson wrote:
> On Wed, Apr 17, 2024 at 06:58:53PM +0530, Sibi Sankar wrote:
>> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
>> new file mode 100644
>> index 000000000000..059eb25f217c
>> --- /dev/null
>> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
>> @@ -0,0 +1,188 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
Hey Bjorn,
Thanks for taking time to review the series :)
>> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
>
> Nope.
ack, artefact from the v1 of legacy driver :(
>
>> + */
>> +
>> +#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(i) (0x100 + ((i) * 8))
>> +
>> +/* Rx Registers */
>> +#define APSS_CPUCP_RX_MBOX_IDR 0
>> +#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
>
> Can we have lower case hex digits, plz?
Sure
>
>> +#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
>
> @dev and @irq can be a local variables in qcom_cpucp_mbox_probe().
Ack
>
>> + */
>> +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;
>
> This value is immediately overwritten (or unused).
Ack
>
>> + if (status & BIT(i)) {
>
> Can't you combine the for loop and this conditional into a
> for_each_bit_set()?
The only drawback I see here is if the number of channels increase to
it's full capacity of 64 since for_each_set_bit expects unsigned long.
>
>> + val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
>> + chan = &cpucp->chans[i];
>> + spin_lock_irqsave(&chan->lock, flags);
>
> Can you please add a comment here to document that the lock is taken
> here to deal with races against client registration? (It wasn't obvious
> to me...)
This is was put in to handle irqs after channel closure. Meaning we
don't want to send data on a closed/empty channel.
>
>> + 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 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)
>> + return cpucp->irq;
>> +
>> + ret = devm_request_irq(cpucp->dev, cpucp->irq, qcom_cpucp_mbox_irq_fn,
>> + IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
>> + if (ret < 0)
>> + return dev_err_probe(cpucp->dev, ret, "Failed to register irq: %d\n", cpucp->irq);
>> +
>> + 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 = devm_mbox_controller_register(cpucp->dev, mbox);
>> + if (ret)
>> + return dev_err_probe(cpucp->dev, ret, "Failed to create mailbox\n");
>> +
>> + platform_set_drvdata(pdev, cpucp);
>
> I don't see you using the drvdata anywhere, can we drop this?
Yeash I'll drop this in the next re-spin.
>
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
>> + { .compatible = "qcom,x1e80100-cpucp-mbox"},
>
> A space after the final '"' would be good for aesthetics.
Not sure how I missed it :(
-Sibi
>
> Regards,
> Bjorn
>
>> + {}
>> +};
>> +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,
>> + },
>> +};
>> +module_platform_driver(qcom_cpucp_mbox_driver);
>> +
>> +MODULE_DESCRIPTION("QTI CPUCP MBOX Driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.34.1
>>
On 4/18/24 02:26, Bjorn Andersson wrote:
> On Wed, Apr 17, 2024 at 06:58:54PM +0530, Sibi Sankar wrote:
>> Resize the GICR register region as it currently seeps into the CPU Control
>> Processor mailbox RX region.
>>
>
> Not that anyone is running a stable kernel here, but please make a habit
> of adding Fixes: tags when correcting previous mistakes.
Yeah, skipped adding fixed for ^^ reason but I'll add it in the next re-
spin.
>
>> Reviewed-by: Dmitry Baryshkov <[email protected]>
>> Signed-off-by: Sibi Sankar <[email protected]>
>> ---
>>
>> v2:
>> * Pickup Rb from Dimitry.
>>
>> 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 */
>>
>
> The 12th GICR ends a bit before that, and per your commit message you're
> just nudging it down to get this range out of the say of your other
> range - rather than giving it a proper value.
>
> Wouldn't 0x300000 be a better value here? Or am I perhaps missing
Yes, my calc was incorrect. It should be 0x300000.
-Sibi
> something in the difference? If so, I'd like the commit message to state
> what, so someone doesn't get excited and correct/break it later.
>
> Regards,
> Bjorn
>
>> interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
>>
>> --
>> 2.34.1
>>
On Thu, Apr 18, 2024 at 01:01:50PM +0530, Sibi Sankar wrote:
> On 4/18/24 02:56, Bjorn Andersson wrote:
> > On Wed, Apr 17, 2024 at 06:58:53PM +0530, Sibi Sankar wrote:
> > > diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
[..]
> >
> > > + if (status & BIT(i)) {
> >
> > Can't you combine the for loop and this conditional into a
> > for_each_bit_set()?
>
> The only drawback I see here is if the number of channels increase to
> it's full capacity of 64 since for_each_set_bit expects unsigned long.
>
It takes a unsigned long * and it can take a size > BITS_PER_LONG. But
I've not convinced myself that the bit order across two of those matches
the u64 bits.
> >
> > > + val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
> > > + chan = &cpucp->chans[i];
> > > + spin_lock_irqsave(&chan->lock, flags);
> >
> > Can you please add a comment here to document that the lock is taken
> > here to deal with races against client registration? (It wasn't obvious
> > to me...)
>
> This is was put in to handle irqs after channel closure. Meaning we
> don't want to send data on a closed/empty channel.
>
You're dealing with that through the chan->cl check below, not the lock
itself. So the lock here would be for synchronizing this code with
potentially concurrent execution of __mbox_bind_client() or e.g.
mbox_free_channel().
But if this is indeed the problem, then we're locking here to ensure
that mbox_chan_received_data() will not dereference chan->cl while it's
being modified elsewhere int he mailbox core.
If that's the case, I think this needs to be strongly documented in the
API, or perhaps better yet the lock being moved into
mbox_chan_received_data().
Regards,
Bjorn
Hi Sibi,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.9-rc4 next-20240419]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sibi-Sankar/dt-bindings-mailbox-qcom-Add-CPUCP-mailbox-controller-bindings/20240417-213339
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240417132856.1106250-3-quic_sibis%40quicinc.com
patch subject: [PATCH V3 2/5] mailbox: Add support for QTI CPUCP mailbox controller
config: hexagon-randconfig-r121-20240421 (https://download.01.org/0day-ci/archive/20240421/[email protected]/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce: (https://download.01.org/0day-ci/archive/20240421/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
In file included from drivers/mailbox/qcom-cpucp-mbox.c:6:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from drivers/mailbox/qcom-cpucp-mbox.c:6:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from drivers/mailbox/qcom-cpucp-mbox.c:6:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
>> drivers/mailbox/qcom-cpucp-mbox.c:61:11: error: call to undeclared function 'readq'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
^
>> drivers/mailbox/qcom-cpucp-mbox.c:71:4: error: call to undeclared function 'writeq'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
^
drivers/mailbox/qcom-cpucp-mbox.c:85:8: error: call to undeclared function 'readq'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
^
drivers/mailbox/qcom-cpucp-mbox.c:87:2: error: call to undeclared function 'writeq'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
writeq(val, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
^
drivers/mailbox/qcom-cpucp-mbox.c:98:8: error: call to undeclared function 'readq'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
^
drivers/mailbox/qcom-cpucp-mbox.c:100:2: error: call to undeclared function 'writeq'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
writeq(val, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
^
drivers/mailbox/qcom-cpucp-mbox.c:140:2: error: call to undeclared function 'writeq'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
^
6 warnings and 7 errors generated.
vim +/readq +61 drivers/mailbox/qcom-cpucp-mbox.c
51
52 static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
53 {
54 struct qcom_cpucp_mbox *cpucp = data;
55 struct mbox_chan *chan;
56 unsigned long flags;
57 u64 status;
58 u32 val;
59 int i;
60
> 61 status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
62
63 for (i = 0; i < APSS_CPUCP_IPC_CHAN_SUPPORTED; i++) {
64 val = 0;
65 if (status & BIT(i)) {
66 val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
67 chan = &cpucp->chans[i];
68 spin_lock_irqsave(&chan->lock, flags);
69 if (chan->cl)
70 mbox_chan_received_data(chan, &val);
> 71 writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
72 spin_unlock_irqrestore(&chan->lock, flags);
73 }
74 }
75
76 return IRQ_HANDLED;
77 }
78
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki