[PATCH 1/2] dt-bindings: soc: qcom: Add devicetree binding for Qcom IPCC

[PATCH 1/2] dt-bindings: soc: qcom: Add devicetree binding for Qcom IPCC

[Manivannan Sadhasivam]

Add devicetree YAML binding for Qualcomm Inter-Processor Communication
Controller (IPCC) block.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 .../bindings/soc/qcom/qcom,ipcc.yaml          | 85 +++++++++++++++++++
 include/dt-bindings/soc/qcom,ipcc.h           | 38 +++++++++
 2 files changed, 123 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,ipcc.yaml
 create mode 100644 include/dt-bindings/soc/qcom,ipcc.h

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,ipcc.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,ipcc.yaml
new file mode 100644
index 000000000000..48b281181401
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,ipcc.yaml
@@ -0,0 +1,85 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/qcom/qcom,ipcc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. Inter-Processor Communication Controller
+
+maintainers:
+  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+
+description:
+  The Inter-Processor Communication Controller (IPCC) is a centralized hardware
+  to route the interrupts across various subsystems. It involves a three-level
+  addressing scheme called protocol, client and signal. For example, consider an
+  entity on the Application Processor Subsystem (APSS) that wants to listen to
+  Modem's interrupts via Shared Memory Point to Point (SMP2P) interface. In such
+  a case, the client would be Modem (client-id is 2) and the signal would be
+  SMP2P (signal-id is 2). The SMP2P itself falls under the Multiprocessor (MPROC)
+  protocol (protocol-id is 0). Refer include/dt-bindings/soc/qcom/qcom,ipcc.h
+  for the list of such IDs.
+
+  One of the duties of this interrupt controller driver is to forward the
+  interrupts to the correct entities on the APSS. The children inheriting the
+  interrupt-controller would be mentioning the client-id and signal-id it's
+  interested in.
+
+  On the other hand, sending an interrupt to a subsystem is done through the
+  mailbox interface, which again requires client-id and signal-id.
+
+properties:
+  compatible:
+    const: "qcom,ipcc"
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 3
+    description:
+      The first cell is the client-id, the second cell is the signal-id and the
+      third cell is the interrupt type.
+
+  "#mbox-cells":
+    const: 2
+    description:
+      The first cell is the client-id, and the second cell is the signal-id.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-controller
+  - "#interrupt-cells"
+  - "#mbox-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+        #include <dt-bindings/interrupt-controller/arm-gic.h>
+        #include <dt-bindings/soc/qcom,ipcc.h>
+
+        ipcc_mproc: qcom,ipcc@408000 {
+                compatible = "qcom,ipcc";
+                reg = <0x408000 0x1000>;
+                interrupts = <GIC_SPI 229 IRQ_TYPE_LEVEL_HIGH>;
+                interrupt-controller;
+                #interrupt-cells = <3>;
+                #mbox-cells = <2>;
+        };
+
+        smp2p-modem {
+                compatible = "qcom,smp2p";
+                interrupts-extended = <&ipcc_mproc IPCC_CLIENT_MPSS
+                                IPCC_MPROC_SIGNAL_SMP2P IRQ_TYPE_EDGE_RISING>;
+                mboxes = <&ipcc_mproc IPCC_CLIENT_MPSS IPCC_MPROC_SIGNAL_SMP2P>;
+
+                /* Other SMP2P fields */
+        };
diff --git a/include/dt-bindings/soc/qcom,ipcc.h b/include/dt-bindings/soc/qcom,ipcc.h
new file mode 100644
index 000000000000..2926cdb4cb48
--- /dev/null
+++ b/include/dt-bindings/soc/qcom,ipcc.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2018-2019, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __DT_BINDINGS_QCOM_IPCC_H
+#define __DT_BINDINGS_QCOM_IPCC_H
+
+/* Signal IDs for MPROC protocol */
+#define IPCC_MPROC_SIGNAL_GLINK_QMP	0
+#define IPCC_MPROC_SIGNAL_SMP2P		2
+#define IPCC_MPROC_SIGNAL_PING		3
+#define IPCC_MPROC_SIGNAL_MAX		4 /* Used by driver only */
+
+#define IPCC_COMPUTE_L0_SIGNAL_MAX	32 /* Used by driver only */
+#define IPCC_COMPUTE_L1_SIGNAL_MAX	32 /* Used by driver only */
+
+/* Client IDs */
+#define IPCC_CLIENT_AOP			0
+#define IPCC_CLIENT_TZ			1
+#define IPCC_CLIENT_MPSS		2
+#define IPCC_CLIENT_LPASS		3
+#define IPCC_CLIENT_SLPI		4
+#define IPCC_CLIENT_SDC			5
+#define IPCC_CLIENT_CDSP		6
+#define IPCC_CLIENT_NPU			7
+#define IPCC_CLIENT_APSS		8
+#define IPCC_CLIENT_GPU			9
+#define IPCC_CLIENT_CVP			10
+#define IPCC_CLIENT_CAM			11
+#define IPCC_CLIENT_VPU			12
+#define IPCC_CLIENT_PCIE0		13
+#define IPCC_CLIENT_PCIE1		14
+#define IPCC_CLIENT_PCIE2		15
+#define IPCC_CLIENT_SPSS		16
+#define IPCC_CLIENT_MAX			17 /* Used by driver only */
+
+#endif
-- 
2.17.1

[PATCH 2/2] soc: qcom: ipcc: Add support for IPCC controller

[Manivannan Sadhasivam]

From: Venkata Narendra Kumar Gutta <vnkgutta@codeaurora.org>

Add support for the Inter-Processor Communication Controller (IPCC)
driver that coordinates the interrupts (inbound & outbound) for
Multiprocessor (MPROC), COMPUTE-Level0 (COMPUTE-L0) & COMPUTE-Level1
(COMPUTE-L1) protocols for the Application Processor Subsystem (APSS).

As a part of its inbound communication, the driver would be responsible
for forwarding the interrupts from various clients, such as Modem, DSPs,
PCIe, and so on, to the entities running on the APPS. As a result, it's
implemented as an irq_chip driver.

On the other hand, the driver also registers as a mailbox controller
that provides a mailbox interface to interrupt other clients connected
to the IPCC, acting as an outbound communication channel.

Signed-off-by: Raghavendra Rao Ananta <rananta@codeaurora.org>
Signed-off-by: Venkata Narendra Kumar Gutta <vnkgutta@codeaurora.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
[mani: cleaned up for upstream]
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/soc/qcom/Kconfig     |  11 +
 drivers/soc/qcom/Makefile    |   1 +
 drivers/soc/qcom/qcom_ipcc.c | 410 +++++++++++++++++++++++++++++++++++
 3 files changed, 422 insertions(+)
 create mode 100644 drivers/soc/qcom/qcom_ipcc.c

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index bf42a17a45de..97c380c3fa09 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -53,6 +53,17 @@ config QCOM_GSBI
 	  functions for connecting the underlying serial UART, SPI, and I2C
 	  devices to the output pins.
 
+config QCOM_IPCC
+	tristate "Qualcomm Technologies, Inc. IPCC driver"
+	depends on MAILBOX
+	help
+	  Qualcomm Technologies, Inc. IPCC driver for MSM devices. The drivers
+	  acts as an interrupt controller for the clients interested in
+	  talking to the IPCC (inbound-communication). On the other hand, the
+	  driver also provides a mailbox channel for outbound-communications.
+	  Say Y here to compile the driver as a part of kernel or M to compile
+	  as a module.
+
 config QCOM_LLCC
 	tristate "Qualcomm Technologies, Inc. LLCC driver"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 5d6b83dc58e8..b7658b007040 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -24,5 +24,6 @@ obj-$(CONFIG_QCOM_SOCINFO)	+= socinfo.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
 obj-$(CONFIG_QCOM_APR) += apr.o
 obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
+obj-$(CONFIG_QCOM_IPCC) += qcom_ipcc.o
 obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
 obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
diff --git a/drivers/soc/qcom/qcom_ipcc.c b/drivers/soc/qcom/qcom_ipcc.c
new file mode 100644
index 000000000000..5906a70220e0
--- /dev/null
+++ b/drivers/soc/qcom/qcom_ipcc.c
@@ -0,0 +1,410 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include <dt-bindings/soc/qcom,ipcc.h>
+
+/* IPCC Register offsets */
+#define IPCC_REG_SEND_ID		0x0c
+#define IPCC_REG_RECV_ID		0x10
+#define IPCC_REG_RECV_SIGNAL_ENABLE	0x14
+#define IPCC_REG_RECV_SIGNAL_DISABLE	0x18
+#define IPCC_REG_RECV_SIGNAL_CLEAR	0x1c
+#define IPCC_REG_CLIENT_CLEAR		0x38
+
+#define IPCC_SIGNAL_ID_MASK		GENMASK(15, 0)
+#define IPCC_CLIENT_ID_MASK		GENMASK(31, 16)
+#define IPCC_CLIENT_ID_SHIFT		16
+
+#define IPCC_NO_PENDING_IRQ		0xffffffff
+
+/**
+ * struct qcom_ipcc_proto_data - Per-protocol data
+ * @irq_domain:		irq_domain associated with this protocol-id
+ * @mbox:		mailbox-controller interface
+ * @chans:		The mailbox clients channel array (created dynamically)
+ * @base:		Base address of the IPCC frame associated to APSS
+ * @dev:		Device associated with this instance
+ * @irq:		Summary irq
+ */
+struct qcom_ipcc_proto_data {
+	struct irq_domain *irq_domain;
+	struct mbox_controller mbox;
+	struct mbox_chan *chans;
+	void __iomem *base;
+	struct device *dev;
+	int irq;
+};
+
+/**
+ * struct qcom_ipcc_mbox_chan - Per-mailbox-channel data. Associated to
+ *				channel when requested by the clients
+ * @chan:	Points to this channel's array element for this protocol's
+ *		ipcc_protocol_data->chans[]
+ * @proto_data: The pointer to per-protocol data associated to this channel
+ * @client_id:	The client-id to which the interrupt has to be triggered
+ * @signal_id:	The signal-id to which the interrupt has to be triggered
+ */
+struct qcom_ipcc_mbox_chan {
+	struct mbox_chan *chan;
+	struct qcom_ipcc_proto_data *proto_data;
+	u16 client_id;
+	u16 signal_id;
+};
+
+static struct qcom_ipcc_proto_data *ipcc_proto_data;
+
+static inline u32 qcom_ipcc_get_packed_id(u16 client_id, u16 signal_id)
+{
+	return (client_id << IPCC_CLIENT_ID_SHIFT) | signal_id;
+}
+
+static inline u16 qcom_ipcc_get_client_id(u32 packed_id)
+{
+	return packed_id >> IPCC_CLIENT_ID_SHIFT;
+}
+
+static inline u16 qcom_ipcc_get_signal_id(u32 packed_id)
+{
+	return packed_id & IPCC_SIGNAL_ID_MASK;
+}
+
+static irqreturn_t qcom_ipcc_irq_fn(int irq, void *data)
+{
+	struct qcom_ipcc_proto_data *proto_data = data;
+	u32 packed_id;
+	int virq;
+
+	for (;;) {
+		packed_id = readl(proto_data->base + IPCC_REG_RECV_ID);
+		if (packed_id == IPCC_NO_PENDING_IRQ)
+			break;
+
+		virq = irq_find_mapping(proto_data->irq_domain, packed_id);
+
+		dev_dbg(proto_data->dev,
+			"IRQ for client_id: %u; signal_id: %u; virq: %d\n",
+			qcom_ipcc_get_client_id(packed_id),
+			qcom_ipcc_get_signal_id(packed_id), virq);
+
+		writel(packed_id,
+		       proto_data->base + IPCC_REG_RECV_SIGNAL_CLEAR);
+
+		generic_handle_irq(virq);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static void qcom_ipcc_mask_irq(struct irq_data *irqd)
+{
+	struct qcom_ipcc_proto_data *proto_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(irqd);
+	u16 sender_client_id = qcom_ipcc_get_client_id(hwirq);
+	u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq);
+
+	proto_data = irq_data_get_irq_chip_data(irqd);
+
+	dev_dbg(proto_data->dev,
+		"Disabling interrupts for: client_id: %u; signal_id: %u\n",
+		sender_client_id, sender_signal_id);
+
+	writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_DISABLE);
+}
+
+static void qcom_ipcc_unmask_irq(struct irq_data *irqd)
+{
+	struct qcom_ipcc_proto_data *proto_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(irqd);
+	u16 sender_client_id = qcom_ipcc_get_client_id(hwirq);
+	u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq);
+
+	proto_data = irq_data_get_irq_chip_data(irqd);
+
+	dev_dbg(proto_data->dev,
+		"Enabling interrupts for: client_id: %u; signal_id: %u\n",
+		sender_client_id, sender_signal_id);
+
+	writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_ENABLE);
+}
+
+static struct irq_chip qcom_ipcc_irq_chip = {
+	.name = "ipcc",
+	.irq_mask = qcom_ipcc_mask_irq,
+	.irq_unmask = qcom_ipcc_unmask_irq,
+	.flags = IRQCHIP_SKIP_SET_WAKE,
+};
+
+static int qcom_ipcc_domain_map(struct irq_domain *d, unsigned int irq,
+				irq_hw_number_t hw)
+{
+	struct qcom_ipcc_proto_data *proto_data = d->host_data;
+
+	irq_set_chip_and_handler(irq, &qcom_ipcc_irq_chip, handle_level_irq);
+	irq_set_chip_data(irq, proto_data);
+	irq_set_noprobe(irq);
+
+	return 0;
+}
+
+static int qcom_ipcc_domain_xlate(struct irq_domain *d,
+				  struct device_node *node, const u32 *intspec,
+				  unsigned int intsize,
+				  unsigned long *out_hwirq,
+				  unsigned int *out_type)
+{
+	struct qcom_ipcc_proto_data *proto_data = d->host_data;
+	struct device *dev = proto_data->dev;
+
+	if (intsize != 3)
+		return -EINVAL;
+
+	*out_hwirq = qcom_ipcc_get_packed_id(intspec[0], intspec[1]);
+	*out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
+
+	dev_dbg(dev, "hwirq: 0x%lx\n", *out_hwirq);
+
+	return 0;
+}
+
+static const struct irq_domain_ops qcom_ipcc_irq_ops = {
+	.map = qcom_ipcc_domain_map,
+	.xlate = qcom_ipcc_domain_xlate,
+};
+
+static int qcom_ipcc_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+	struct qcom_ipcc_mbox_chan *ipcc_mbox_chan = chan->con_priv;
+	struct qcom_ipcc_proto_data *proto_data = ipcc_mbox_chan->proto_data;
+	u32 packed_id;
+
+	dev_dbg(proto_data->dev,
+		"Generating IRQ for client_id: %u; signal_id: %u\n",
+		ipcc_mbox_chan->client_id, ipcc_mbox_chan->signal_id);
+
+	packed_id = qcom_ipcc_get_packed_id(ipcc_mbox_chan->client_id,
+					    ipcc_mbox_chan->signal_id);
+	writel(packed_id, proto_data->base + IPCC_REG_SEND_ID);
+
+	return 0;
+}
+
+static void qcom_ipcc_mbox_shutdown(struct mbox_chan *chan)
+{
+	struct qcom_ipcc_mbox_chan *ipcc_mbox_chan = chan->con_priv;
+
+	chan->con_priv = NULL;
+	kfree(ipcc_mbox_chan);
+}
+
+static struct mbox_chan *qcom_ipcc_mbox_xlate(struct mbox_controller *mbox,
+					const struct of_phandle_args *ph)
+{
+	struct device *dev;
+	struct qcom_ipcc_proto_data *proto_data;
+	struct qcom_ipcc_mbox_chan *ipcc_mbox_chan;
+	int chan_id;
+
+	proto_data = container_of(mbox, struct qcom_ipcc_proto_data, mbox);
+	if (WARN_ON(!proto_data))
+		return ERR_PTR(-EINVAL);
+
+	dev = proto_data->dev;
+
+	if (ph->args_count != 2)
+		return ERR_PTR(-EINVAL);
+
+	/* Check whether the mbox channel is allocated or not */
+	for (chan_id = 0; chan_id < mbox->num_chans; chan_id++) {
+		ipcc_mbox_chan = proto_data->chans[chan_id].con_priv;
+
+		if (!ipcc_mbox_chan)
+			break;
+		else if (ipcc_mbox_chan->client_id == ph->args[0] &&
+			 ipcc_mbox_chan->signal_id == ph->args[1])
+			return ERR_PTR(-EBUSY);
+	}
+
+	if (chan_id >= mbox->num_chans)
+		return ERR_PTR(-EBUSY);
+
+	ipcc_mbox_chan = kzalloc(sizeof(*ipcc_mbox_chan), GFP_KERNEL);
+	if (!ipcc_mbox_chan)
+		return ERR_PTR(-ENOMEM);
+
+	ipcc_mbox_chan->client_id = ph->args[0];
+	ipcc_mbox_chan->signal_id = ph->args[1];
+	ipcc_mbox_chan->chan = &proto_data->chans[chan_id];
+	ipcc_mbox_chan->proto_data = proto_data;
+	ipcc_mbox_chan->chan->con_priv = ipcc_mbox_chan;
+
+	dev_dbg(dev,
+		"New mailbox channel: %u for client_id: %u; signal_id: %u\n",
+		chan_id, ipcc_mbox_chan->client_id,
+		ipcc_mbox_chan->signal_id);
+
+	return ipcc_mbox_chan->chan;
+}
+
+static const struct mbox_chan_ops ipcc_mbox_chan_ops = {
+	.send_data = qcom_ipcc_mbox_send_data,
+	.shutdown = qcom_ipcc_mbox_shutdown
+};
+
+static int qcom_ipcc_setup_mbox(struct qcom_ipcc_proto_data *proto_data,
+				struct device_node *controller_dn)
+{
+	struct mbox_controller *mbox;
+	struct device_node *client_dn;
+	struct device *dev = proto_data->dev;
+	struct of_phandle_args curr_ph;
+	int i, j, ret;
+	int num_chans = 0;
+
+	/*
+	 * Find out the number of clients interested in this mailbox
+	 * and create channels accordingly.
+	 */
+	for_each_node_with_property(client_dn, "mboxes") {
+		if (!of_device_is_available(client_dn))
+			continue;
+		i = of_count_phandle_with_args(client_dn,
+					       "mboxes", "#mbox-cells");
+		for (j = 0; j < i; j++) {
+			ret = of_parse_phandle_with_args(client_dn, "mboxes",
+							 "#mbox-cells", j,
+							 &curr_ph);
+			of_node_put(curr_ph.np);
+			if (!ret && curr_ph.np == controller_dn) {
+				num_chans++;
+				break;
+			}
+		}
+	}
+
+	/* If no clients are found, skip registering as a mbox controller */
+	if (!num_chans)
+		return 0;
+
+	proto_data->chans = devm_kcalloc(dev, num_chans,
+					 sizeof(struct mbox_chan), GFP_KERNEL);
+	if (!proto_data->chans)
+		return -ENOMEM;
+
+	mbox = &proto_data->mbox;
+	mbox->dev = dev;
+	mbox->num_chans = num_chans;
+	mbox->chans = proto_data->chans;
+	mbox->ops = &ipcc_mbox_chan_ops;
+	mbox->of_xlate = qcom_ipcc_mbox_xlate;
+	mbox->txdone_irq = false;
+	mbox->txdone_poll = false;
+
+	return devm_mbox_controller_register(dev, mbox);
+}
+
+static int qcom_ipcc_probe(struct platform_device *pdev)
+{
+	struct qcom_ipcc_proto_data *proto_data;
+	int ret;
+
+	proto_data = devm_kzalloc(&pdev->dev, sizeof(*proto_data), GFP_KERNEL);
+	if (!proto_data)
+		return -ENOMEM;
+
+	ipcc_proto_data = proto_data;
+	proto_data->dev = &pdev->dev;
+
+	proto_data->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(proto_data->base)) {
+		dev_err(&pdev->dev, "Failed to ioremap the ipcc base addr\n");
+		return PTR_ERR(proto_data->base);
+	}
+
+	proto_data->irq = platform_get_irq(pdev, 0);
+	if (proto_data->irq < 0) {
+		dev_err(&pdev->dev, "Failed to get the IRQ\n");
+		return proto_data->irq;
+	}
+
+	/* Perform a SW reset on this client's protocol state */
+	writel(0x1, proto_data->base + IPCC_REG_CLIENT_CLEAR);
+
+	proto_data->irq_domain = irq_domain_add_tree(pdev->dev.of_node,
+						     &qcom_ipcc_irq_ops,
+						     proto_data);
+	if (!proto_data->irq_domain) {
+		dev_err(&pdev->dev, "Failed to add irq_domain\n");
+		return -ENOMEM;
+	}
+
+	ret = qcom_ipcc_setup_mbox(proto_data, pdev->dev.of_node);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to create mailbox\n");
+		goto err_mbox;
+	}
+
+	ret = devm_request_irq(&pdev->dev, proto_data->irq, qcom_ipcc_irq_fn,
+			       IRQF_TRIGGER_HIGH, "ipcc", proto_data);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret);
+		goto err_mbox;
+	}
+
+	enable_irq_wake(proto_data->irq);
+	platform_set_drvdata(pdev, proto_data);
+
+	return 0;
+
+err_mbox:
+	irq_domain_remove(proto_data->irq_domain);
+
+	return ret;
+}
+
+static int qcom_ipcc_remove(struct platform_device *pdev)
+{
+	struct qcom_ipcc_proto_data *proto_data = platform_get_drvdata(pdev);
+
+	disable_irq_wake(proto_data->irq);
+	irq_domain_remove(proto_data->irq_domain);
+
+	return 0;
+}
+
+static const struct of_device_id qcom_ipcc_of_match[] = {
+	{ .compatible = "qcom,ipcc"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, qcom_ipcc_of_match);
+
+static struct platform_driver qcom_ipcc_driver = {
+	.probe = qcom_ipcc_probe,
+	.remove = qcom_ipcc_remove,
+	.driver = {
+		.name = "qcom_ipcc",
+		.of_match_table = qcom_ipcc_of_match,
+	},
+};
+
+static int __init qcom_ipcc_init(void)
+{
+	return platform_driver_register(&qcom_ipcc_driver);
+}
+arch_initcall(qcom_ipcc_init);
+
+static void __exit qcom_ipcc_exit(void)
+{
+	platform_driver_unregister(&qcom_ipcc_driver);
+}
+module_exit(qcom_ipcc_exit);
+
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH 2/2] soc: qcom: ipcc: Add support for IPCC controller

[rananta]

On 2020-04-29 23:30, Manivannan Sadhasivam wrote:
> +static int qcom_ipcc_probe(struct platform_device *pdev)
> +{
> +	struct qcom_ipcc_proto_data *proto_data;
> +	int ret;
> +
> +	proto_data = devm_kzalloc(&pdev->dev, sizeof(*proto_data), 
> GFP_KERNEL);
> +	if (!proto_data)
> +		return -ENOMEM;
> +
> +	ipcc_proto_data = proto_data;
> +	proto_data->dev = &pdev->dev;
> +
> +	proto_data->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(proto_data->base)) {
> +		dev_err(&pdev->dev, "Failed to ioremap the ipcc base addr\n");
> +		return PTR_ERR(proto_data->base);
> +	}
> +
> +	proto_data->irq = platform_get_irq(pdev, 0);
> +	if (proto_data->irq < 0) {
> +		dev_err(&pdev->dev, "Failed to get the IRQ\n");
> +		return proto_data->irq;
> +	}
> +
> +	/* Perform a SW reset on this client's protocol state */
> +	writel(0x1, proto_data->base + IPCC_REG_CLIENT_CLEAR);
We can skip doing a SW reset here. Few of the subsystems may be brought 
out of reset via the bootloader and the interrupts from them might be 
pending. Doing a SW reset here would clear those interrupts.

Thank you.
Raghavendra

Re: [PATCH 2/2] soc: qcom: ipcc: Add support for IPCC controller

[Manivannan Sadhasivam]

Hi,

On Wed, Apr 29, 2020 at 11:45:27PM -0700, rananta@codeaurora.org wrote:
> On 2020-04-29 23:30, Manivannan Sadhasivam wrote:
> > +static int qcom_ipcc_probe(struct platform_device *pdev)
> > +{
> > +	struct qcom_ipcc_proto_data *proto_data;
> > +	int ret;
> > +
> > +	proto_data = devm_kzalloc(&pdev->dev, sizeof(*proto_data),
> > GFP_KERNEL);
> > +	if (!proto_data)
> > +		return -ENOMEM;
> > +
> > +	ipcc_proto_data = proto_data;
> > +	proto_data->dev = &pdev->dev;
> > +
> > +	proto_data->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(proto_data->base)) {
> > +		dev_err(&pdev->dev, "Failed to ioremap the ipcc base addr\n");
> > +		return PTR_ERR(proto_data->base);
> > +	}
> > +
> > +	proto_data->irq = platform_get_irq(pdev, 0);
> > +	if (proto_data->irq < 0) {
> > +		dev_err(&pdev->dev, "Failed to get the IRQ\n");
> > +		return proto_data->irq;
> > +	}
> > +
> > +	/* Perform a SW reset on this client's protocol state */
> > +	writel(0x1, proto_data->base + IPCC_REG_CLIENT_CLEAR);
> We can skip doing a SW reset here. Few of the subsystems may be brought out
> of reset via the bootloader and the interrupts from them might be pending.
> Doing a SW reset here would clear those interrupts.
> 

Okay, will remove it.

Thanks,
Mani

> Thank you.
> Raghavendra

Re: [PATCH 2/2] soc: qcom: ipcc: Add support for IPCC controller

[Vinod Koul]

On 30-04-20, 12:00, Manivannan Sadhasivam wrote:

> +#define IPCC_SIGNAL_ID_MASK		GENMASK(15, 0)
> +#define IPCC_CLIENT_ID_MASK		GENMASK(31, 16)
> +#define IPCC_CLIENT_ID_SHIFT		16
> +
> +#define IPCC_NO_PENDING_IRQ		0xffffffff

Why not GENMASK(31, 0)

> +static struct qcom_ipcc_proto_data *ipcc_proto_data;

why do we need a global which is used only once.

> +static void qcom_ipcc_mask_irq(struct irq_data *irqd)
> +{
> +	struct qcom_ipcc_proto_data *proto_data;
> +	irq_hw_number_t hwirq = irqd_to_hwirq(irqd);
> +	u16 sender_client_id = qcom_ipcc_get_client_id(hwirq);
> +	u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq);

last three are used for debug log, fn can be much simpler if we get rid
of noise.. Do we really need this to be production :)

> +
> +	proto_data = irq_data_get_irq_chip_data(irqd);
> +
> +	dev_dbg(proto_data->dev,
> +		"Disabling interrupts for: client_id: %u; signal_id: %u\n",
> +		sender_client_id, sender_signal_id);
> +
> +	writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_DISABLE);
> +}
> +
> +static void qcom_ipcc_unmask_irq(struct irq_data *irqd)
> +{
> +	struct qcom_ipcc_proto_data *proto_data;
> +	irq_hw_number_t hwirq = irqd_to_hwirq(irqd);
> +	u16 sender_client_id = qcom_ipcc_get_client_id(hwirq);
> +	u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq);

here as well

> +static int qcom_ipcc_domain_xlate(struct irq_domain *d,
> +				  struct device_node *node, const u32 *intspec,
> +				  unsigned int intsize,
> +				  unsigned long *out_hwirq,
> +				  unsigned int *out_type)

pls align these to match open parenthesis

> +static int qcom_ipcc_setup_mbox(struct qcom_ipcc_proto_data *proto_data,
> +				struct device_node *controller_dn)
> +{
> +	struct mbox_controller *mbox;
> +	struct device_node *client_dn;
> +	struct device *dev = proto_data->dev;
> +	struct of_phandle_args curr_ph;
> +	int i, j, ret;
> +	int num_chans = 0;
> +
> +	/*
> +	 * Find out the number of clients interested in this mailbox
> +	 * and create channels accordingly.
> +	 */
> +	for_each_node_with_property(client_dn, "mboxes") {
> +		if (!of_device_is_available(client_dn))
> +			continue;
> +		i = of_count_phandle_with_args(client_dn,
> +					       "mboxes", "#mbox-cells");
> +		for (j = 0; j < i; j++) {
> +			ret = of_parse_phandle_with_args(client_dn, "mboxes",
> +							 "#mbox-cells", j,
> +							 &curr_ph);

this sounds like something DT should do, not drivers :)

> +static int qcom_ipcc_probe(struct platform_device *pdev)
> +{
> +	struct qcom_ipcc_proto_data *proto_data;
> +	int ret;
> +
> +	proto_data = devm_kzalloc(&pdev->dev, sizeof(*proto_data), GFP_KERNEL);
> +	if (!proto_data)
> +		return -ENOMEM;
> +
> +	ipcc_proto_data = proto_data;
> +	proto_data->dev = &pdev->dev;
> +
> +	proto_data->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(proto_data->base)) {
> +		dev_err(&pdev->dev, "Failed to ioremap the ipcc base addr\n");
> +		return PTR_ERR(proto_data->base);
> +	}
> +
> +	proto_data->irq = platform_get_irq(pdev, 0);
> +	if (proto_data->irq < 0) {
> +		dev_err(&pdev->dev, "Failed to get the IRQ\n");
> +		return proto_data->irq;
> +	}
> +
> +	/* Perform a SW reset on this client's protocol state */
> +	writel(0x1, proto_data->base + IPCC_REG_CLIENT_CLEAR);
> +
> +	proto_data->irq_domain = irq_domain_add_tree(pdev->dev.of_node,
> +						     &qcom_ipcc_irq_ops,
> +						     proto_data);
> +	if (!proto_data->irq_domain) {
> +		dev_err(&pdev->dev, "Failed to add irq_domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = qcom_ipcc_setup_mbox(proto_data, pdev->dev.of_node);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to create mailbox\n");
> +		goto err_mbox;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, proto_data->irq, qcom_ipcc_irq_fn,
> +			       IRQF_TRIGGER_HIGH, "ipcc", proto_data);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret);

Should the qcom_ipcc_setup_mbox() not be unroller here?

> +		goto err_mbox;
> +	}
> +
> +	enable_irq_wake(proto_data->irq);
> +	platform_set_drvdata(pdev, proto_data);
> +
> +	return 0;
> +
> +err_mbox:
> +	irq_domain_remove(proto_data->irq_domain);
> +
> +	return ret;
> +}
> +
> +static int qcom_ipcc_remove(struct platform_device *pdev)
> +{
> +	struct qcom_ipcc_proto_data *proto_data = platform_get_drvdata(pdev);
> +
> +	disable_irq_wake(proto_data->irq);
> +	irq_domain_remove(proto_data->irq_domain);

So you are calling this when your isr is active, we have possibility of
race here. This function come with a warning:
"The caller must ensure that all mappings within the domain have been disposed"

Has that been done?

-- 
~Vinod

Re: [PATCH 2/2] soc: qcom: ipcc: Add support for IPCC controller

[Manivannan Sadhasivam]

Hi,

On Thu, Apr 30, 2020 at 12:54:48PM +0530, Vinod Koul wrote:
> On 30-04-20, 12:00, Manivannan Sadhasivam wrote:
> 
> > +#define IPCC_SIGNAL_ID_MASK		GENMASK(15, 0)
> > +#define IPCC_CLIENT_ID_MASK		GENMASK(31, 16)
> > +#define IPCC_CLIENT_ID_SHIFT		16
> > +
> > +#define IPCC_NO_PENDING_IRQ		0xffffffff
> 
> Why not GENMASK(31, 0)
> 

Hmm, I usually use GENMASK for mask defines. But yeah it can be used here.

> > +static struct qcom_ipcc_proto_data *ipcc_proto_data;
> 
> why do we need a global which is used only once.
> 

Ah, that's a left over. Will remove it.

> > +static void qcom_ipcc_mask_irq(struct irq_data *irqd)
> > +{
> > +	struct qcom_ipcc_proto_data *proto_data;
> > +	irq_hw_number_t hwirq = irqd_to_hwirq(irqd);
> > +	u16 sender_client_id = qcom_ipcc_get_client_id(hwirq);
> > +	u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq);
> 
> last three are used for debug log, fn can be much simpler if we get rid
> of noise.. Do we really need this to be production :)
> 

This is for debugging the production systems, that's why dev_dbg. So I don't
consider it as a noise :)

> > +
> > +	proto_data = irq_data_get_irq_chip_data(irqd);
> > +
> > +	dev_dbg(proto_data->dev,
> > +		"Disabling interrupts for: client_id: %u; signal_id: %u\n",
> > +		sender_client_id, sender_signal_id);
> > +
> > +	writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_DISABLE);
> > +}
> > +
> > +static void qcom_ipcc_unmask_irq(struct irq_data *irqd)
> > +{
> > +	struct qcom_ipcc_proto_data *proto_data;
> > +	irq_hw_number_t hwirq = irqd_to_hwirq(irqd);
> > +	u16 sender_client_id = qcom_ipcc_get_client_id(hwirq);
> > +	u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq);
> 
> here as well
> 
> > +static int qcom_ipcc_domain_xlate(struct irq_domain *d,
> > +				  struct device_node *node, const u32 *intspec,
> > +				  unsigned int intsize,
> > +				  unsigned long *out_hwirq,
> > +				  unsigned int *out_type)
> 
> pls align these to match open parenthesis
> 

It is aligned. Perhaps diff is showing it as mangled due to ignoring
whitespaces?

> > +static int qcom_ipcc_setup_mbox(struct qcom_ipcc_proto_data *proto_data,
> > +				struct device_node *controller_dn)
> > +{
> > +	struct mbox_controller *mbox;
> > +	struct device_node *client_dn;
> > +	struct device *dev = proto_data->dev;
> > +	struct of_phandle_args curr_ph;
> > +	int i, j, ret;
> > +	int num_chans = 0;
> > +
> > +	/*
> > +	 * Find out the number of clients interested in this mailbox
> > +	 * and create channels accordingly.
> > +	 */
> > +	for_each_node_with_property(client_dn, "mboxes") {
> > +		if (!of_device_is_available(client_dn))
> > +			continue;
> > +		i = of_count_phandle_with_args(client_dn,
> > +					       "mboxes", "#mbox-cells");
> > +		for (j = 0; j < i; j++) {
> > +			ret = of_parse_phandle_with_args(client_dn, "mboxes",
> > +							 "#mbox-cells", j,
> > +							 &curr_ph);
> 
> this sounds like something DT should do, not drivers :)
> 

Right. This is design discussion I'd like to have. Currently the driver checks
the DT and allocates the total number of mbox channels. But I think the more
cleaner way would be to have this driver allocating fixed number of channels
statically and let the clients use it.

Maybe Raghavendra/Venkat can comment here?

> > +static int qcom_ipcc_probe(struct platform_device *pdev)
> > +{
> > +	struct qcom_ipcc_proto_data *proto_data;
> > +	int ret;
> > +
> > +	proto_data = devm_kzalloc(&pdev->dev, sizeof(*proto_data), GFP_KERNEL);
> > +	if (!proto_data)
> > +		return -ENOMEM;
> > +
> > +	ipcc_proto_data = proto_data;
> > +	proto_data->dev = &pdev->dev;
> > +
> > +	proto_data->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(proto_data->base)) {
> > +		dev_err(&pdev->dev, "Failed to ioremap the ipcc base addr\n");
> > +		return PTR_ERR(proto_data->base);
> > +	}
> > +
> > +	proto_data->irq = platform_get_irq(pdev, 0);
> > +	if (proto_data->irq < 0) {
> > +		dev_err(&pdev->dev, "Failed to get the IRQ\n");
> > +		return proto_data->irq;
> > +	}
> > +
> > +	/* Perform a SW reset on this client's protocol state */
> > +	writel(0x1, proto_data->base + IPCC_REG_CLIENT_CLEAR);
> > +
> > +	proto_data->irq_domain = irq_domain_add_tree(pdev->dev.of_node,
> > +						     &qcom_ipcc_irq_ops,
> > +						     proto_data);
> > +	if (!proto_data->irq_domain) {
> > +		dev_err(&pdev->dev, "Failed to add irq_domain\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	ret = qcom_ipcc_setup_mbox(proto_data, pdev->dev.of_node);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to create mailbox\n");
> > +		goto err_mbox;
> > +	}
> > +
> > +	ret = devm_request_irq(&pdev->dev, proto_data->irq, qcom_ipcc_irq_fn,
> > +			       IRQF_TRIGGER_HIGH, "ipcc", proto_data);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret);
> 
> Should the qcom_ipcc_setup_mbox() not be unroller here?
> 

qcom_ipcc_setup_mbox() uses devm_ API for registering mbox controller. So what
is the issue?

> > +		goto err_mbox;
> > +	}
> > +
> > +	enable_irq_wake(proto_data->irq);
> > +	platform_set_drvdata(pdev, proto_data);
> > +
> > +	return 0;
> > +
> > +err_mbox:
> > +	irq_domain_remove(proto_data->irq_domain);
> > +
> > +	return ret;
> > +}
> > +
> > +static int qcom_ipcc_remove(struct platform_device *pdev)
> > +{
> > +	struct qcom_ipcc_proto_data *proto_data = platform_get_drvdata(pdev);
> > +
> > +	disable_irq_wake(proto_data->irq);
> > +	irq_domain_remove(proto_data->irq_domain);
> 
> So you are calling this when your isr is active, we have possibility of
> race here. This function come with a warning:
> "The caller must ensure that all mappings within the domain have been disposed"
> 

I thought it is not required since most of the interrupt controller drivers
don't do it. But checked with Marc Zyngier and he suggested to dispose the
mapping as that's the good practice. The usual pattern is an interrupt
controller is not built as a module and the assumption is it lives forever.

But one issue here is, currently we don't know the allocated irqs (in specific
hw irq numbers) as we don't create the mapping. It gets created when a client
calls platform_get_irq(). In the irq handler, we just read the current hw irq
number from a register. So, if we want to dispose the mapping then we need to
track the allocated irqs. Else we need to create the mapping for all possible
clients in this driver itself. I'm not sure which one is preferred.

Maybe Bjorn/qcom folks can comment what is preferred here?

Thanks,
Mani

> Has that been done?
> 
> -- 
> ~Vinod

Re: [PATCH 2/2] soc: qcom: ipcc: Add support for IPCC controller

[Vinod Koul]

On 30-04-20, 14:32, Manivannan Sadhasivam wrote:
> Hi,
> 
> On Thu, Apr 30, 2020 at 12:54:48PM +0530, Vinod Koul wrote:
> > On 30-04-20, 12:00, Manivannan Sadhasivam wrote:
> > 
> > > +#define IPCC_SIGNAL_ID_MASK		GENMASK(15, 0)
> > > +#define IPCC_CLIENT_ID_MASK		GENMASK(31, 16)
> > > +#define IPCC_CLIENT_ID_SHIFT		16
> > > +
> > > +#define IPCC_NO_PENDING_IRQ		0xffffffff
> > 
> > Why not GENMASK(31, 0)
> > 
> 
> Hmm, I usually use GENMASK for mask defines. But yeah it can be used here.

Well the idea behind genmask was to avoid coding mistakes which sounds
apt here as well :)

> 
> > > +static struct qcom_ipcc_proto_data *ipcc_proto_data;
> > 
> > why do we need a global which is used only once.
> > 
> 
> Ah, that's a left over. Will remove it.
> 
> > > +static void qcom_ipcc_mask_irq(struct irq_data *irqd)
> > > +{
> > > +	struct qcom_ipcc_proto_data *proto_data;
> > > +	irq_hw_number_t hwirq = irqd_to_hwirq(irqd);
> > > +	u16 sender_client_id = qcom_ipcc_get_client_id(hwirq);
> > > +	u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq);
> > 
> > last three are used for debug log, fn can be much simpler if we get rid
> > of noise.. Do we really need this to be production :)
> > 
> 
> This is for debugging the production systems, that's why dev_dbg. So I don't
> consider it as a noise :)

This in an irq chip, the debug code is much more than actual function!
Anyone who wants to debug can add these lines :)

> > > +	proto_data = irq_data_get_irq_chip_data(irqd);
> > > +
> > > +	dev_dbg(proto_data->dev,
> > > +		"Disabling interrupts for: client_id: %u; signal_id: %u\n",
> > > +		sender_client_id, sender_signal_id);
> > > +
> > > +	writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_DISABLE);
> > > +}
> > > +
> > > +static void qcom_ipcc_unmask_irq(struct irq_data *irqd)
> > > +{
> > > +	struct qcom_ipcc_proto_data *proto_data;
> > > +	irq_hw_number_t hwirq = irqd_to_hwirq(irqd);
> > > +	u16 sender_client_id = qcom_ipcc_get_client_id(hwirq);
> > > +	u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq);
> > 
> > here as well
> > 
> > > +static int qcom_ipcc_domain_xlate(struct irq_domain *d,
> > > +				  struct device_node *node, const u32 *intspec,
> > > +				  unsigned int intsize,
> > > +				  unsigned long *out_hwirq,
> > > +				  unsigned int *out_type)
> > 
> > pls align these to match open parenthesis
> > 
> 
> It is aligned. Perhaps diff is showing it as mangled due to ignoring
> whitespaces?

Not sure, even checkpatch seems to think so

> 
> > > +static int qcom_ipcc_setup_mbox(struct qcom_ipcc_proto_data *proto_data,
> > > +				struct device_node *controller_dn)
> > > +{
> > > +	struct mbox_controller *mbox;
> > > +	struct device_node *client_dn;
> > > +	struct device *dev = proto_data->dev;
> > > +	struct of_phandle_args curr_ph;
> > > +	int i, j, ret;
> > > +	int num_chans = 0;
> > > +
> > > +	/*
> > > +	 * Find out the number of clients interested in this mailbox
> > > +	 * and create channels accordingly.
> > > +	 */
> > > +	for_each_node_with_property(client_dn, "mboxes") {
> > > +		if (!of_device_is_available(client_dn))
> > > +			continue;
> > > +		i = of_count_phandle_with_args(client_dn,
> > > +					       "mboxes", "#mbox-cells");
> > > +		for (j = 0; j < i; j++) {
> > > +			ret = of_parse_phandle_with_args(client_dn, "mboxes",
> > > +							 "#mbox-cells", j,
> > > +							 &curr_ph);
> > 
> > this sounds like something DT should do, not drivers :)
> > 
> 
> Right. This is design discussion I'd like to have. Currently the driver checks
> the DT and allocates the total number of mbox channels. But I think the more
> cleaner way would be to have this driver allocating fixed number of channels
> statically and let the clients use it.

Sorry my point was about code of checking mboxes and #mbox-cells, these
seems generic enough and could be moved into of core code :)

But I think making fixed number of channels should not be done if DT can
provide this information.

> Maybe Raghavendra/Venkat can comment here?
> 
> > > +static int qcom_ipcc_probe(struct platform_device *pdev)
> > > +{
> > > +	struct qcom_ipcc_proto_data *proto_data;
> > > +	int ret;
> > > +
> > > +	proto_data = devm_kzalloc(&pdev->dev, sizeof(*proto_data), GFP_KERNEL);
> > > +	if (!proto_data)
> > > +		return -ENOMEM;
> > > +
> > > +	ipcc_proto_data = proto_data;
> > > +	proto_data->dev = &pdev->dev;
> > > +
> > > +	proto_data->base = devm_platform_ioremap_resource(pdev, 0);
> > > +	if (IS_ERR(proto_data->base)) {
> > > +		dev_err(&pdev->dev, "Failed to ioremap the ipcc base addr\n");
> > > +		return PTR_ERR(proto_data->base);
> > > +	}
> > > +
> > > +	proto_data->irq = platform_get_irq(pdev, 0);
> > > +	if (proto_data->irq < 0) {
> > > +		dev_err(&pdev->dev, "Failed to get the IRQ\n");
> > > +		return proto_data->irq;
> > > +	}
> > > +
> > > +	/* Perform a SW reset on this client's protocol state */
> > > +	writel(0x1, proto_data->base + IPCC_REG_CLIENT_CLEAR);
> > > +
> > > +	proto_data->irq_domain = irq_domain_add_tree(pdev->dev.of_node,
> > > +						     &qcom_ipcc_irq_ops,
> > > +						     proto_data);
> > > +	if (!proto_data->irq_domain) {
> > > +		dev_err(&pdev->dev, "Failed to add irq_domain\n");
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	ret = qcom_ipcc_setup_mbox(proto_data, pdev->dev.of_node);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "Failed to create mailbox\n");
> > > +		goto err_mbox;
> > > +	}
> > > +
> > > +	ret = devm_request_irq(&pdev->dev, proto_data->irq, qcom_ipcc_irq_fn,
> > > +			       IRQF_TRIGGER_HIGH, "ipcc", proto_data);
> > > +	if (ret < 0) {
> > > +		dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret);
> > 
> > Should the qcom_ipcc_setup_mbox() not be unroller here?
> 
> qcom_ipcc_setup_mbox() uses devm_ API for registering mbox controller. So what
> is the issue?

Ah missed the devm parts, i think no unroll required here

> > > +		goto err_mbox;
> > > +	}
> > > +
> > > +	enable_irq_wake(proto_data->irq);
> > > +	platform_set_drvdata(pdev, proto_data);
> > > +
> > > +	return 0;
> > > +
> > > +err_mbox:
> > > +	irq_domain_remove(proto_data->irq_domain);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int qcom_ipcc_remove(struct platform_device *pdev)
> > > +{
> > > +	struct qcom_ipcc_proto_data *proto_data = platform_get_drvdata(pdev);
> > > +
> > > +	disable_irq_wake(proto_data->irq);
> > > +	irq_domain_remove(proto_data->irq_domain);
> > 
> > So you are calling this when your isr is active, we have possibility of
> > race here. This function come with a warning:
> > "The caller must ensure that all mappings within the domain have been disposed"
> 
> I thought it is not required since most of the interrupt controller drivers
> don't do it. But checked with Marc Zyngier and he suggested to dispose the
> mapping as that's the good practice. The usual pattern is an interrupt
> controller is not built as a module and the assumption is it lives forever.
> 
> But one issue here is, currently we don't know the allocated irqs (in specific
> hw irq numbers) as we don't create the mapping. It gets created when a client
> calls platform_get_irq(). In the irq handler, we just read the current hw irq
> number from a register. So, if we want to dispose the mapping then we need to
> track the allocated irqs. Else we need to create the mapping for all possible
> clients in this driver itself. I'm not sure which one is preferred.
> 
> Maybe Bjorn/qcom folks can comment what is preferred here?

Maybe this should also be lives forever cases then! :)

-- 
~Vinod

Re: [PATCH 2/2] soc: qcom: ipcc: Add support for IPCC controller

[Manivannan Sadhasivam]

On Thu, Apr 30, 2020 at 02:53:58PM +0530, Vinod Koul wrote:
> On 30-04-20, 14:32, Manivannan Sadhasivam wrote:
> > Hi,
> > 
> > On Thu, Apr 30, 2020 at 12:54:48PM +0530, Vinod Koul wrote:
> > > On 30-04-20, 12:00, Manivannan Sadhasivam wrote:
> > > 
> > > > +#define IPCC_SIGNAL_ID_MASK		GENMASK(15, 0)
> > > > +#define IPCC_CLIENT_ID_MASK		GENMASK(31, 16)
> > > > +#define IPCC_CLIENT_ID_SHIFT		16
> > > > +
> > > > +#define IPCC_NO_PENDING_IRQ		0xffffffff
> > > 
> > > Why not GENMASK(31, 0)
> > > 
> > 
> > Hmm, I usually use GENMASK for mask defines. But yeah it can be used here.
> 
> Well the idea behind genmask was to avoid coding mistakes which sounds
> apt here as well :)
> 

Agree.

> > 
> > > > +static struct qcom_ipcc_proto_data *ipcc_proto_data;
> > > 
> > > why do we need a global which is used only once.
> > > 
> > 
> > Ah, that's a left over. Will remove it.
> > 
> > > > +static void qcom_ipcc_mask_irq(struct irq_data *irqd)
> > > > +{
> > > > +	struct qcom_ipcc_proto_data *proto_data;
> > > > +	irq_hw_number_t hwirq = irqd_to_hwirq(irqd);
> > > > +	u16 sender_client_id = qcom_ipcc_get_client_id(hwirq);
> > > > +	u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq);
> > > 
> > > last three are used for debug log, fn can be much simpler if we get rid
> > > of noise.. Do we really need this to be production :)
> > > 
> > 
> > This is for debugging the production systems, that's why dev_dbg. So I don't
> > consider it as a noise :)
> 
> This in an irq chip, the debug code is much more than actual function!
> Anyone who wants to debug can add these lines :)
> 

I don't have a strong feeling so will remove :)

> > > > +	proto_data = irq_data_get_irq_chip_data(irqd);
> > > > +
> > > > +	dev_dbg(proto_data->dev,
> > > > +		"Disabling interrupts for: client_id: %u; signal_id: %u\n",
> > > > +		sender_client_id, sender_signal_id);
> > > > +
> > > > +	writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_DISABLE);
> > > > +}
> > > > +
> > > > +static void qcom_ipcc_unmask_irq(struct irq_data *irqd)
> > > > +{
> > > > +	struct qcom_ipcc_proto_data *proto_data;
> > > > +	irq_hw_number_t hwirq = irqd_to_hwirq(irqd);
> > > > +	u16 sender_client_id = qcom_ipcc_get_client_id(hwirq);
> > > > +	u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq);
> > > 
> > > here as well
> > > 
> > > > +static int qcom_ipcc_domain_xlate(struct irq_domain *d,
> > > > +				  struct device_node *node, const u32 *intspec,
> > > > +				  unsigned int intsize,
> > > > +				  unsigned long *out_hwirq,
> > > > +				  unsigned int *out_type)
> > > 
> > > pls align these to match open parenthesis
> > > 
> > 
> > It is aligned. Perhaps diff is showing it as mangled due to ignoring
> > whitespaces?
> 
> Not sure, even checkpatch seems to think so
> 

I'm not sure too, this is what it says for me:

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#66: 
new file mode 100644

CHECK: Alignment should match open parenthesis
#279: FILE: drivers/soc/qcom/qcom_ipcc.c:209:
+static struct mbox_chan *qcom_ipcc_mbox_xlate(struct mbox_controller *mbox,
+					const struct of_phandle_args *ph)

total: 0 errors, 1 warnings, 1 checks, 433 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Commit 01c2e8a8a53d ("soc: qcom: ipcc: Add support for IPCC controller") has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

> > 
> > > > +static int qcom_ipcc_setup_mbox(struct qcom_ipcc_proto_data *proto_data,
> > > > +				struct device_node *controller_dn)
> > > > +{
> > > > +	struct mbox_controller *mbox;
> > > > +	struct device_node *client_dn;
> > > > +	struct device *dev = proto_data->dev;
> > > > +	struct of_phandle_args curr_ph;
> > > > +	int i, j, ret;
> > > > +	int num_chans = 0;
> > > > +
> > > > +	/*
> > > > +	 * Find out the number of clients interested in this mailbox
> > > > +	 * and create channels accordingly.
> > > > +	 */
> > > > +	for_each_node_with_property(client_dn, "mboxes") {
> > > > +		if (!of_device_is_available(client_dn))
> > > > +			continue;
> > > > +		i = of_count_phandle_with_args(client_dn,
> > > > +					       "mboxes", "#mbox-cells");
> > > > +		for (j = 0; j < i; j++) {
> > > > +			ret = of_parse_phandle_with_args(client_dn, "mboxes",
> > > > +							 "#mbox-cells", j,
> > > > +							 &curr_ph);
> > > 
> > > this sounds like something DT should do, not drivers :)
> > > 
> > 
> > Right. This is design discussion I'd like to have. Currently the driver checks
> > the DT and allocates the total number of mbox channels. But I think the more
> > cleaner way would be to have this driver allocating fixed number of channels
> > statically and let the clients use it.
> 
> Sorry my point was about code of checking mboxes and #mbox-cells, these
> seems generic enough and could be moved into of core code :)
> 
> But I think making fixed number of channels should not be done if DT can
> provide this information.
> 

That's not my call, it is Jassi :) All mailbox drivers statically declare the
number of channels in drivers and we don't have any API/property to fetch the
information from DT.

If Jassi is okay with it, I can add an API and corresponding DT property (and
ofcourse document it).

> > Maybe Raghavendra/Venkat can comment here?
> > 
> > > > +static int qcom_ipcc_probe(struct platform_device *pdev)
> > > > +{
> > > > +	struct qcom_ipcc_proto_data *proto_data;
> > > > +	int ret;
> > > > +
> > > > +	proto_data = devm_kzalloc(&pdev->dev, sizeof(*proto_data), GFP_KERNEL);
> > > > +	if (!proto_data)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ipcc_proto_data = proto_data;
> > > > +	proto_data->dev = &pdev->dev;
> > > > +
> > > > +	proto_data->base = devm_platform_ioremap_resource(pdev, 0);
> > > > +	if (IS_ERR(proto_data->base)) {
> > > > +		dev_err(&pdev->dev, "Failed to ioremap the ipcc base addr\n");
> > > > +		return PTR_ERR(proto_data->base);
> > > > +	}
> > > > +
> > > > +	proto_data->irq = platform_get_irq(pdev, 0);
> > > > +	if (proto_data->irq < 0) {
> > > > +		dev_err(&pdev->dev, "Failed to get the IRQ\n");
> > > > +		return proto_data->irq;
> > > > +	}
> > > > +
> > > > +	/* Perform a SW reset on this client's protocol state */
> > > > +	writel(0x1, proto_data->base + IPCC_REG_CLIENT_CLEAR);
> > > > +
> > > > +	proto_data->irq_domain = irq_domain_add_tree(pdev->dev.of_node,
> > > > +						     &qcom_ipcc_irq_ops,
> > > > +						     proto_data);
> > > > +	if (!proto_data->irq_domain) {
> > > > +		dev_err(&pdev->dev, "Failed to add irq_domain\n");
> > > > +		return -ENOMEM;
> > > > +	}
> > > > +
> > > > +	ret = qcom_ipcc_setup_mbox(proto_data, pdev->dev.of_node);
> > > > +	if (ret) {
> > > > +		dev_err(&pdev->dev, "Failed to create mailbox\n");
> > > > +		goto err_mbox;
> > > > +	}
> > > > +
> > > > +	ret = devm_request_irq(&pdev->dev, proto_data->irq, qcom_ipcc_irq_fn,
> > > > +			       IRQF_TRIGGER_HIGH, "ipcc", proto_data);
> > > > +	if (ret < 0) {
> > > > +		dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret);
> > > 
> > > Should the qcom_ipcc_setup_mbox() not be unroller here?
> > 
> > qcom_ipcc_setup_mbox() uses devm_ API for registering mbox controller. So what
> > is the issue?
> 
> Ah missed the devm parts, i think no unroll required here
> 
> > > > +		goto err_mbox;
> > > > +	}
> > > > +
> > > > +	enable_irq_wake(proto_data->irq);
> > > > +	platform_set_drvdata(pdev, proto_data);
> > > > +
> > > > +	return 0;
> > > > +
> > > > +err_mbox:
> > > > +	irq_domain_remove(proto_data->irq_domain);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int qcom_ipcc_remove(struct platform_device *pdev)
> > > > +{
> > > > +	struct qcom_ipcc_proto_data *proto_data = platform_get_drvdata(pdev);
> > > > +
> > > > +	disable_irq_wake(proto_data->irq);
> > > > +	irq_domain_remove(proto_data->irq_domain);
> > > 
> > > So you are calling this when your isr is active, we have possibility of
> > > race here. This function come with a warning:
> > > "The caller must ensure that all mappings within the domain have been disposed"
> > 
> > I thought it is not required since most of the interrupt controller drivers
> > don't do it. But checked with Marc Zyngier and he suggested to dispose the
> > mapping as that's the good practice. The usual pattern is an interrupt
> > controller is not built as a module and the assumption is it lives forever.
> > 
> > But one issue here is, currently we don't know the allocated irqs (in specific
> > hw irq numbers) as we don't create the mapping. It gets created when a client
> > calls platform_get_irq(). In the irq handler, we just read the current hw irq
> > number from a register. So, if we want to dispose the mapping then we need to
> > track the allocated irqs. Else we need to create the mapping for all possible
> > clients in this driver itself. I'm not sure which one is preferred.
> > 
> > Maybe Bjorn/qcom folks can comment what is preferred here?
> 
> Maybe this should also be lives forever cases then! :)
> 

Heh. Most of the drivers using irq_domain_add_tree() don't bother disposing the
interrupts. Currently this driver is defined as tristate. So either we should
dispose the interrupts or make it as built-in only. If this is ever going to get
removed, then there will be a warning from kernel saying irq tree is not empty!

Or there is also a case where we can make sure that all clients will dispose
their interrupts when they get unloaded. In that way, we can safely remove the
irq domain in this driver.

Thanks,
Mani

> -- 
> ~Vinod

Re: [PATCH 2/2] soc: qcom: ipcc: Add support for IPCC controller

[Bjorn Andersson]

On Wed 29 Apr 23:30 PDT 2020, Manivannan Sadhasivam wrote:

> From: Venkata Narendra Kumar Gutta <vnkgutta@codeaurora.org>
> 
> Add support for the Inter-Processor Communication Controller (IPCC)
> driver that coordinates the interrupts (inbound & outbound) for
> Multiprocessor (MPROC), COMPUTE-Level0 (COMPUTE-L0) & COMPUTE-Level1
> (COMPUTE-L1) protocols for the Application Processor Subsystem (APSS).
> 
> As a part of its inbound communication, the driver would be responsible
> for forwarding the interrupts from various clients, such as Modem, DSPs,
> PCIe, and so on, to the entities running on the APPS. As a result, it's
> implemented as an irq_chip driver.
> 
> On the other hand, the driver also registers as a mailbox controller
> that provides a mailbox interface to interrupt other clients connected
> to the IPCC, acting as an outbound communication channel.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@codeaurora.org>
> Signed-off-by: Venkata Narendra Kumar Gutta <vnkgutta@codeaurora.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> [mani: cleaned up for upstream]
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/soc/qcom/Kconfig     |  11 +
>  drivers/soc/qcom/Makefile    |   1 +
>  drivers/soc/qcom/qcom_ipcc.c | 410 +++++++++++++++++++++++++++++++++++

I think the driver should be moved to either drivers/irqchip or
drivers/mailbox.

>  3 files changed, 422 insertions(+)
>  create mode 100644 drivers/soc/qcom/qcom_ipcc.c
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index bf42a17a45de..97c380c3fa09 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -53,6 +53,17 @@ config QCOM_GSBI
>  	  functions for connecting the underlying serial UART, SPI, and I2C
>  	  devices to the output pins.
>  
> +config QCOM_IPCC
> +	tristate "Qualcomm Technologies, Inc. IPCC driver"
> +	depends on MAILBOX
> +	help
> +	  Qualcomm Technologies, Inc. IPCC driver for MSM devices. The drivers
> +	  acts as an interrupt controller for the clients interested in
> +	  talking to the IPCC (inbound-communication). On the other hand, the
> +	  driver also provides a mailbox channel for outbound-communications.
> +	  Say Y here to compile the driver as a part of kernel or M to compile
> +	  as a module.
> +
>  config QCOM_LLCC
>  	tristate "Qualcomm Technologies, Inc. LLCC driver"
>  	depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 5d6b83dc58e8..b7658b007040 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -24,5 +24,6 @@ obj-$(CONFIG_QCOM_SOCINFO)	+= socinfo.o
>  obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
>  obj-$(CONFIG_QCOM_APR) += apr.o
>  obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
> +obj-$(CONFIG_QCOM_IPCC) += qcom_ipcc.o
>  obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
>  obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
> diff --git a/drivers/soc/qcom/qcom_ipcc.c b/drivers/soc/qcom/qcom_ipcc.c
> new file mode 100644
> index 000000000000..5906a70220e0
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom_ipcc.c
> @@ -0,0 +1,410 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#include <dt-bindings/soc/qcom,ipcc.h>
> +
> +/* IPCC Register offsets */
> +#define IPCC_REG_SEND_ID		0x0c
> +#define IPCC_REG_RECV_ID		0x10
> +#define IPCC_REG_RECV_SIGNAL_ENABLE	0x14
> +#define IPCC_REG_RECV_SIGNAL_DISABLE	0x18
> +#define IPCC_REG_RECV_SIGNAL_CLEAR	0x1c
> +#define IPCC_REG_CLIENT_CLEAR		0x38
> +
> +#define IPCC_SIGNAL_ID_MASK		GENMASK(15, 0)
> +#define IPCC_CLIENT_ID_MASK		GENMASK(31, 16)
> +#define IPCC_CLIENT_ID_SHIFT		16
> +
> +#define IPCC_NO_PENDING_IRQ		0xffffffff
> +
> +/**
> + * struct qcom_ipcc_proto_data - Per-protocol data
> + * @irq_domain:		irq_domain associated with this protocol-id
> + * @mbox:		mailbox-controller interface
> + * @chans:		The mailbox clients channel array (created dynamically)
> + * @base:		Base address of the IPCC frame associated to APSS
> + * @dev:		Device associated with this instance
> + * @irq:		Summary irq
> + */
> +struct qcom_ipcc_proto_data {

There's only one of these per IPCC hardware block, so I would suggest
that you shorten in to just "struct qcom_ipcc" and then throughout the
implementation replace "proto_data" with just "ipcc".

> +	struct irq_domain *irq_domain;
> +	struct mbox_controller mbox;
> +	struct mbox_chan *chans;
> +	void __iomem *base;
> +	struct device *dev;
> +	int irq;
> +};
> +
> +/**
> + * struct qcom_ipcc_mbox_chan - Per-mailbox-channel data. Associated to
> + *				channel when requested by the clients
> + * @chan:	Points to this channel's array element for this protocol's
> + *		ipcc_protocol_data->chans[]
> + * @proto_data: The pointer to per-protocol data associated to this channel
> + * @client_id:	The client-id to which the interrupt has to be triggered
> + * @signal_id:	The signal-id to which the interrupt has to be triggered
> + */
> +struct qcom_ipcc_mbox_chan {
> +	struct mbox_chan *chan;
> +	struct qcom_ipcc_proto_data *proto_data;
> +	u16 client_id;
> +	u16 signal_id;
> +};
> +
> +static struct qcom_ipcc_proto_data *ipcc_proto_data;
> +
> +static inline u32 qcom_ipcc_get_packed_id(u16 client_id, u16 signal_id)

qcom_ipcc_pack_ids()

and please omit the "inline", the compiler will inline it if it's a good
thing to do.

> +{
> +	return (client_id << IPCC_CLIENT_ID_SHIFT) | signal_id;

How about:

	return FIELD_PREP(IPCC_CLIENT_ID_MASK, client_id) |
	       FIELD_PREP(IPCC_SIGNAL_ID_MASK, signal_id);

> +}
> +
> +static inline u16 qcom_ipcc_get_client_id(u32 packed_id)

This is used only to extract the field for a dev_dbg print. How about
dropping this for now and if this kind of debugging is beneficial lets
add some tracepoint that internally can extract the fields.

> +{
> +	return packed_id >> IPCC_CLIENT_ID_SHIFT;
> +}
> +
> +static inline u16 qcom_ipcc_get_signal_id(u32 packed_id)

Ditto.

> +{
> +	return packed_id & IPCC_SIGNAL_ID_MASK;
> +}
> +
> +static irqreturn_t qcom_ipcc_irq_fn(int irq, void *data)
> +{
> +	struct qcom_ipcc_proto_data *proto_data = data;
> +	u32 packed_id;
> +	int virq;
> +
> +	for (;;) {
> +		packed_id = readl(proto_data->base + IPCC_REG_RECV_ID);
> +		if (packed_id == IPCC_NO_PENDING_IRQ)
> +			break;
> +
> +		virq = irq_find_mapping(proto_data->irq_domain, packed_id);
> +
> +		dev_dbg(proto_data->dev,
> +			"IRQ for client_id: %u; signal_id: %u; virq: %d\n",
> +			qcom_ipcc_get_client_id(packed_id),
> +			qcom_ipcc_get_signal_id(packed_id), virq);

As above, I think this is useful either during development of the driver
(now done) or in a system debugging case as a tracepoint. So please omit
this debug print.

> +
> +		writel(packed_id,

If you're consistently with naming this "hwirq" you don't need to wrap
this line.

> +		       proto_data->base + IPCC_REG_RECV_SIGNAL_CLEAR);
> +
> +		generic_handle_irq(virq);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void qcom_ipcc_mask_irq(struct irq_data *irqd)
> +{
> +	struct qcom_ipcc_proto_data *proto_data;
> +	irq_hw_number_t hwirq = irqd_to_hwirq(irqd);
> +	u16 sender_client_id = qcom_ipcc_get_client_id(hwirq);
> +	u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq);
> +
> +	proto_data = irq_data_get_irq_chip_data(irqd);

	struct qcom_ipcc *ipcc = irq_data_get_irq_chip_data(irqd);
	irq_hw_number_t hwirq = irqd_to_hwirq(irqd);

Looks better.

> +
> +	dev_dbg(proto_data->dev,
> +		"Disabling interrupts for: client_id: %u; signal_id: %u\n",
> +		sender_client_id, sender_signal_id);

Pushing out field dissection and "logging" to a tracepoint would make
this function prettier as well.

> +
> +	writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_DISABLE);
> +}
> +
> +static void qcom_ipcc_unmask_irq(struct irq_data *irqd)
> +{
> +	struct qcom_ipcc_proto_data *proto_data;
> +	irq_hw_number_t hwirq = irqd_to_hwirq(irqd);
> +	u16 sender_client_id = qcom_ipcc_get_client_id(hwirq);
> +	u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq);
> +
> +	proto_data = irq_data_get_irq_chip_data(irqd);

As above.

> +
> +	dev_dbg(proto_data->dev,
> +		"Enabling interrupts for: client_id: %u; signal_id: %u\n",
> +		sender_client_id, sender_signal_id);
> +
> +	writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_ENABLE);
> +}
> +
> +static struct irq_chip qcom_ipcc_irq_chip = {
> +	.name = "ipcc",
> +	.irq_mask = qcom_ipcc_mask_irq,
> +	.irq_unmask = qcom_ipcc_unmask_irq,
> +	.flags = IRQCHIP_SKIP_SET_WAKE,
> +};
> +
> +static int qcom_ipcc_domain_map(struct irq_domain *d, unsigned int irq,
> +				irq_hw_number_t hw)
> +{
> +	struct qcom_ipcc_proto_data *proto_data = d->host_data;
> +
> +	irq_set_chip_and_handler(irq, &qcom_ipcc_irq_chip, handle_level_irq);
> +	irq_set_chip_data(irq, proto_data);
> +	irq_set_noprobe(irq);
> +
> +	return 0;
> +}
> +
> +static int qcom_ipcc_domain_xlate(struct irq_domain *d,
> +				  struct device_node *node, const u32 *intspec,
> +				  unsigned int intsize,
> +				  unsigned long *out_hwirq,
> +				  unsigned int *out_type)
> +{
> +	struct qcom_ipcc_proto_data *proto_data = d->host_data;
> +	struct device *dev = proto_data->dev;
> +
> +	if (intsize != 3)
> +		return -EINVAL;
> +
> +	*out_hwirq = qcom_ipcc_get_packed_id(intspec[0], intspec[1]);
> +	*out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
> +
> +	dev_dbg(dev, "hwirq: 0x%lx\n", *out_hwirq);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops qcom_ipcc_irq_ops = {
> +	.map = qcom_ipcc_domain_map,
> +	.xlate = qcom_ipcc_domain_xlate,
> +};
> +
> +static int qcom_ipcc_mbox_send_data(struct mbox_chan *chan, void *data)
> +{
> +	struct qcom_ipcc_mbox_chan *ipcc_mbox_chan = chan->con_priv;
> +	struct qcom_ipcc_proto_data *proto_data = ipcc_mbox_chan->proto_data;
> +	u32 packed_id;

Although we're in the "mailbox section", I still think it's reasonable
to name this hwirq for consistency (otherwise please shorten it to "id").

> +
> +	dev_dbg(proto_data->dev,
> +		"Generating IRQ for client_id: %u; signal_id: %u\n",
> +		ipcc_mbox_chan->client_id, ipcc_mbox_chan->signal_id);
> +
> +	packed_id = qcom_ipcc_get_packed_id(ipcc_mbox_chan->client_id,
> +					    ipcc_mbox_chan->signal_id);
> +	writel(packed_id, proto_data->base + IPCC_REG_SEND_ID);
> +
> +	return 0;
> +}
> +
> +static void qcom_ipcc_mbox_shutdown(struct mbox_chan *chan)
> +{
> +	struct qcom_ipcc_mbox_chan *ipcc_mbox_chan = chan->con_priv;
> +
> +	chan->con_priv = NULL;
> +	kfree(ipcc_mbox_chan);
> +}
> +
> +static struct mbox_chan *qcom_ipcc_mbox_xlate(struct mbox_controller *mbox,
> +					const struct of_phandle_args *ph)
> +{
> +	struct device *dev;
> +	struct qcom_ipcc_proto_data *proto_data;
> +	struct qcom_ipcc_mbox_chan *ipcc_mbox_chan;
> +	int chan_id;
> +
> +	proto_data = container_of(mbox, struct qcom_ipcc_proto_data, mbox);
> +	if (WARN_ON(!proto_data))

Is this possible?

> +		return ERR_PTR(-EINVAL);
> +
> +	dev = proto_data->dev;
> +
> +	if (ph->args_count != 2)
> +		return ERR_PTR(-EINVAL);
> +
> +	/* Check whether the mbox channel is allocated or not */
> +	for (chan_id = 0; chan_id < mbox->num_chans; chan_id++) {
> +		ipcc_mbox_chan = proto_data->chans[chan_id].con_priv;
> +
> +		if (!ipcc_mbox_chan)
> +			break;
> +		else if (ipcc_mbox_chan->client_id == ph->args[0] &&
> +			 ipcc_mbox_chan->signal_id == ph->args[1])
> +			return ERR_PTR(-EBUSY);

I think it would be better to just return the matching chan, and let
mbox_request_channel() deal with the fact that it's already in use.

> +	}
> +
> +	if (chan_id >= mbox->num_chans)
> +		return ERR_PTR(-EBUSY);
> +
> +	ipcc_mbox_chan = kzalloc(sizeof(*ipcc_mbox_chan), GFP_KERNEL);

Can we allocate these during initialization time instead of allocating
and releasing them in of_xlate and shutdown?

> +	if (!ipcc_mbox_chan)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ipcc_mbox_chan->client_id = ph->args[0];
> +	ipcc_mbox_chan->signal_id = ph->args[1];
> +	ipcc_mbox_chan->chan = &proto_data->chans[chan_id];
> +	ipcc_mbox_chan->proto_data = proto_data;
> +	ipcc_mbox_chan->chan->con_priv = ipcc_mbox_chan;
> +
> +	dev_dbg(dev,
> +		"New mailbox channel: %u for client_id: %u; signal_id: %u\n",
> +		chan_id, ipcc_mbox_chan->client_id,
> +		ipcc_mbox_chan->signal_id);
> +
> +	return ipcc_mbox_chan->chan;
> +}
> +
> +static const struct mbox_chan_ops ipcc_mbox_chan_ops = {
> +	.send_data = qcom_ipcc_mbox_send_data,
> +	.shutdown = qcom_ipcc_mbox_shutdown
> +};
> +
> +static int qcom_ipcc_setup_mbox(struct qcom_ipcc_proto_data *proto_data,
> +				struct device_node *controller_dn)
> +{
> +	struct mbox_controller *mbox;
> +	struct device_node *client_dn;
> +	struct device *dev = proto_data->dev;
> +	struct of_phandle_args curr_ph;
> +	int i, j, ret;
> +	int num_chans = 0;
> +
> +	/*
> +	 * Find out the number of clients interested in this mailbox
> +	 * and create channels accordingly.
> +	 */
> +	for_each_node_with_property(client_dn, "mboxes") {

How much memory do we save by doing this, in comparison to just picking
a max amount?

> +		if (!of_device_is_available(client_dn))
> +			continue;
> +		i = of_count_phandle_with_args(client_dn,
> +					       "mboxes", "#mbox-cells");
> +		for (j = 0; j < i; j++) {
> +			ret = of_parse_phandle_with_args(client_dn, "mboxes",
> +							 "#mbox-cells", j,
> +							 &curr_ph);
> +			of_node_put(curr_ph.np);
> +			if (!ret && curr_ph.np == controller_dn) {
> +				num_chans++;
> +				break;
> +			}
> +		}
> +	}
> +
> +	/* If no clients are found, skip registering as a mbox controller */
> +	if (!num_chans)
> +		return 0;
> +
> +	proto_data->chans = devm_kcalloc(dev, num_chans,
> +					 sizeof(struct mbox_chan), GFP_KERNEL);

sizeof(*proto_data->chans)

> +	if (!proto_data->chans)
> +		return -ENOMEM;
> +
> +	mbox = &proto_data->mbox;
> +	mbox->dev = dev;
> +	mbox->num_chans = num_chans;
> +	mbox->chans = proto_data->chans;
> +	mbox->ops = &ipcc_mbox_chan_ops;
> +	mbox->of_xlate = qcom_ipcc_mbox_xlate;
> +	mbox->txdone_irq = false;
> +	mbox->txdone_poll = false;
> +
> +	return devm_mbox_controller_register(dev, mbox);
> +}
> +
> +static int qcom_ipcc_probe(struct platform_device *pdev)
> +{
> +	struct qcom_ipcc_proto_data *proto_data;
> +	int ret;
> +
> +	proto_data = devm_kzalloc(&pdev->dev, sizeof(*proto_data), GFP_KERNEL);
> +	if (!proto_data)
> +		return -ENOMEM;
> +
> +	ipcc_proto_data = proto_data;

ipcc_protocol_data is unused.

> +	proto_data->dev = &pdev->dev;
> +
> +	proto_data->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(proto_data->base)) {
> +		dev_err(&pdev->dev, "Failed to ioremap the ipcc base addr\n");
> +		return PTR_ERR(proto_data->base);
> +	}
> +
> +	proto_data->irq = platform_get_irq(pdev, 0);
> +	if (proto_data->irq < 0) {
> +		dev_err(&pdev->dev, "Failed to get the IRQ\n");

platform_get_irq() already did print.

> +		return proto_data->irq;
> +	}
> +
> +	/* Perform a SW reset on this client's protocol state */
> +	writel(0x1, proto_data->base + IPCC_REG_CLIENT_CLEAR);
> +
> +	proto_data->irq_domain = irq_domain_add_tree(pdev->dev.of_node,
> +						     &qcom_ipcc_irq_ops,
> +						     proto_data);
> +	if (!proto_data->irq_domain) {
> +		dev_err(&pdev->dev, "Failed to add irq_domain\n");

Afaict this also prints errors.

> +		return -ENOMEM;
> +	}
> +
> +	ret = qcom_ipcc_setup_mbox(proto_data, pdev->dev.of_node);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to create mailbox\n");

Afaict this too will print errors in all code paths (except the ones
where you pass invalid data to mbox_controller_register() - but we're
already past that).

Regards,
Bjorn

> +		goto err_mbox;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, proto_data->irq, qcom_ipcc_irq_fn,
> +			       IRQF_TRIGGER_HIGH, "ipcc", proto_data);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret);
> +		goto err_mbox;
> +	}
> +
> +	enable_irq_wake(proto_data->irq);
> +	platform_set_drvdata(pdev, proto_data);
> +
> +	return 0;
> +
> +err_mbox:
> +	irq_domain_remove(proto_data->irq_domain);
> +
> +	return ret;
> +}
> +
> +static int qcom_ipcc_remove(struct platform_device *pdev)
> +{
> +	struct qcom_ipcc_proto_data *proto_data = platform_get_drvdata(pdev);
> +
> +	disable_irq_wake(proto_data->irq);
> +	irq_domain_remove(proto_data->irq_domain);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id qcom_ipcc_of_match[] = {
> +	{ .compatible = "qcom,ipcc"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, qcom_ipcc_of_match);
> +
> +static struct platform_driver qcom_ipcc_driver = {
> +	.probe = qcom_ipcc_probe,
> +	.remove = qcom_ipcc_remove,
> +	.driver = {
> +		.name = "qcom_ipcc",
> +		.of_match_table = qcom_ipcc_of_match,
> +	},
> +};
> +
> +static int __init qcom_ipcc_init(void)
> +{
> +	return platform_driver_register(&qcom_ipcc_driver);
> +}
> +arch_initcall(qcom_ipcc_init);
> +
> +static void __exit qcom_ipcc_exit(void)
> +{
> +	platform_driver_unregister(&qcom_ipcc_driver);
> +}
> +module_exit(qcom_ipcc_exit);
> +
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.17.1
> 

Re: [PATCH 2/2] soc: qcom: ipcc: Add support for IPCC controller

[Manivannan Sadhasivam]

On Thu, Apr 30, 2020 at 01:18:07PM -0700, Bjorn Andersson wrote:
> On Wed 29 Apr 23:30 PDT 2020, Manivannan Sadhasivam wrote:
> 
> > From: Venkata Narendra Kumar Gutta <vnkgutta@codeaurora.org>
> > 
> > Add support for the Inter-Processor Communication Controller (IPCC)
> > driver that coordinates the interrupts (inbound & outbound) for
> > Multiprocessor (MPROC), COMPUTE-Level0 (COMPUTE-L0) & COMPUTE-Level1
> > (COMPUTE-L1) protocols for the Application Processor Subsystem (APSS).
> > 
> > As a part of its inbound communication, the driver would be responsible
> > for forwarding the interrupts from various clients, such as Modem, DSPs,
> > PCIe, and so on, to the entities running on the APPS. As a result, it's
> > implemented as an irq_chip driver.
> > 
> > On the other hand, the driver also registers as a mailbox controller
> > that provides a mailbox interface to interrupt other clients connected
> > to the IPCC, acting as an outbound communication channel.
> > 
> > Signed-off-by: Raghavendra Rao Ananta <rananta@codeaurora.org>
> > Signed-off-by: Venkata Narendra Kumar Gutta <vnkgutta@codeaurora.org>
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > [mani: cleaned up for upstream]
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/soc/qcom/Kconfig     |  11 +
> >  drivers/soc/qcom/Makefile    |   1 +
> >  drivers/soc/qcom/qcom_ipcc.c | 410 +++++++++++++++++++++++++++++++++++
> 
> I think the driver should be moved to either drivers/irqchip or
> drivers/mailbox.
> 

As said in bindings patch, mailbox seems to be a better location if we don't
want this to be under drivers/soc.

> >  3 files changed, 422 insertions(+)
> >  create mode 100644 drivers/soc/qcom/qcom_ipcc.c
> > 
> > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> > index bf42a17a45de..97c380c3fa09 100644
> > --- a/drivers/soc/qcom/Kconfig
> > +++ b/drivers/soc/qcom/Kconfig
> > @@ -53,6 +53,17 @@ config QCOM_GSBI
> >  	  functions for connecting the underlying serial UART, SPI, and I2C
> >  	  devices to the output pins.
> >  
> > +config QCOM_IPCC
> > +	tristate "Qualcomm Technologies, Inc. IPCC driver"
> > +	depends on MAILBOX
> > +	help
> > +	  Qualcomm Technologies, Inc. IPCC driver for MSM devices. The drivers
> > +	  acts as an interrupt controller for the clients interested in
> > +	  talking to the IPCC (inbound-communication). On the other hand, the
> > +	  driver also provides a mailbox channel for outbound-communications.
> > +	  Say Y here to compile the driver as a part of kernel or M to compile
> > +	  as a module.
> > +
> >  config QCOM_LLCC
> >  	tristate "Qualcomm Technologies, Inc. LLCC driver"
> >  	depends on ARCH_QCOM || COMPILE_TEST
> > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> > index 5d6b83dc58e8..b7658b007040 100644
> > --- a/drivers/soc/qcom/Makefile
> > +++ b/drivers/soc/qcom/Makefile
> > @@ -24,5 +24,6 @@ obj-$(CONFIG_QCOM_SOCINFO)	+= socinfo.o
> >  obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
> >  obj-$(CONFIG_QCOM_APR) += apr.o
> >  obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
> > +obj-$(CONFIG_QCOM_IPCC) += qcom_ipcc.o
> >  obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
> >  obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
> > diff --git a/drivers/soc/qcom/qcom_ipcc.c b/drivers/soc/qcom/qcom_ipcc.c
> > new file mode 100644
> > index 000000000000..5906a70220e0
> > --- /dev/null
> > +++ b/drivers/soc/qcom/qcom_ipcc.c
> > @@ -0,0 +1,410 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/mailbox_controller.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <dt-bindings/soc/qcom,ipcc.h>
> > +
> > +/* IPCC Register offsets */
> > +#define IPCC_REG_SEND_ID		0x0c
> > +#define IPCC_REG_RECV_ID		0x10
> > +#define IPCC_REG_RECV_SIGNAL_ENABLE	0x14
> > +#define IPCC_REG_RECV_SIGNAL_DISABLE	0x18
> > +#define IPCC_REG_RECV_SIGNAL_CLEAR	0x1c
> > +#define IPCC_REG_CLIENT_CLEAR		0x38
> > +
> > +#define IPCC_SIGNAL_ID_MASK		GENMASK(15, 0)
> > +#define IPCC_CLIENT_ID_MASK		GENMASK(31, 16)
> > +#define IPCC_CLIENT_ID_SHIFT		16
> > +
> > +#define IPCC_NO_PENDING_IRQ		0xffffffff
> > +
> > +/**
> > + * struct qcom_ipcc_proto_data - Per-protocol data
> > + * @irq_domain:		irq_domain associated with this protocol-id
> > + * @mbox:		mailbox-controller interface
> > + * @chans:		The mailbox clients channel array (created dynamically)
> > + * @base:		Base address of the IPCC frame associated to APSS
> > + * @dev:		Device associated with this instance
> > + * @irq:		Summary irq
> > + */
> > +struct qcom_ipcc_proto_data {
> 
> There's only one of these per IPCC hardware block, so I would suggest
> that you shorten in to just "struct qcom_ipcc" and then throughout the
> implementation replace "proto_data" with just "ipcc".
> 

okay

> > +	struct irq_domain *irq_domain;
> > +	struct mbox_controller mbox;
> > +	struct mbox_chan *chans;
> > +	void __iomem *base;
> > +	struct device *dev;
> > +	int irq;
> > +};
> > +
> > +/**
> > + * struct qcom_ipcc_mbox_chan - Per-mailbox-channel data. Associated to
> > + *				channel when requested by the clients
> > + * @chan:	Points to this channel's array element for this protocol's
> > + *		ipcc_protocol_data->chans[]
> > + * @proto_data: The pointer to per-protocol data associated to this channel
> > + * @client_id:	The client-id to which the interrupt has to be triggered
> > + * @signal_id:	The signal-id to which the interrupt has to be triggered
> > + */
> > +struct qcom_ipcc_mbox_chan {
> > +	struct mbox_chan *chan;
> > +	struct qcom_ipcc_proto_data *proto_data;
> > +	u16 client_id;
> > +	u16 signal_id;
> > +};
> > +
> > +static struct qcom_ipcc_proto_data *ipcc_proto_data;
> > +
> > +static inline u32 qcom_ipcc_get_packed_id(u16 client_id, u16 signal_id)
> 
> qcom_ipcc_pack_ids()
> 
> and please omit the "inline", the compiler will inline it if it's a good
> thing to do.
> 

But the usual convention is to give the hint to compiler isn't it?

> > +{
> > +	return (client_id << IPCC_CLIENT_ID_SHIFT) | signal_id;
> 
> How about:
> 
> 	return FIELD_PREP(IPCC_CLIENT_ID_MASK, client_id) |
> 	       FIELD_PREP(IPCC_SIGNAL_ID_MASK, signal_id);
> 

okay

> > +}
> > +
> > +static inline u16 qcom_ipcc_get_client_id(u32 packed_id)
> 
> This is used only to extract the field for a dev_dbg print. How about
> dropping this for now and if this kind of debugging is beneficial lets
> add some tracepoint that internally can extract the fields.
> 

okay, will drop.

> > +{
> > +	return packed_id >> IPCC_CLIENT_ID_SHIFT;
> > +}
> > +
> > +static inline u16 qcom_ipcc_get_signal_id(u32 packed_id)
> 
> Ditto.
> 
> > +{
> > +	return packed_id & IPCC_SIGNAL_ID_MASK;
> > +}
> > +
> > +static irqreturn_t qcom_ipcc_irq_fn(int irq, void *data)
> > +{
> > +	struct qcom_ipcc_proto_data *proto_data = data;
> > +	u32 packed_id;
> > +	int virq;
> > +
> > +	for (;;) {
> > +		packed_id = readl(proto_data->base + IPCC_REG_RECV_ID);
> > +		if (packed_id == IPCC_NO_PENDING_IRQ)
> > +			break;
> > +
> > +		virq = irq_find_mapping(proto_data->irq_domain, packed_id);
> > +
> > +		dev_dbg(proto_data->dev,
> > +			"IRQ for client_id: %u; signal_id: %u; virq: %d\n",
> > +			qcom_ipcc_get_client_id(packed_id),
> > +			qcom_ipcc_get_signal_id(packed_id), virq);
> 
> As above, I think this is useful either during development of the driver
> (now done) or in a system debugging case as a tracepoint. So please omit
> this debug print.
> 

okay

> > +
> > +		writel(packed_id,
> 
> If you're consistently with naming this "hwirq" you don't need to wrap
> this line.
> 
> > +		       proto_data->base + IPCC_REG_RECV_SIGNAL_CLEAR);
> > +
> > +		generic_handle_irq(virq);
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static void qcom_ipcc_mask_irq(struct irq_data *irqd)
> > +{
> > +	struct qcom_ipcc_proto_data *proto_data;
> > +	irq_hw_number_t hwirq = irqd_to_hwirq(irqd);
> > +	u16 sender_client_id = qcom_ipcc_get_client_id(hwirq);
> > +	u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq);
> > +
> > +	proto_data = irq_data_get_irq_chip_data(irqd);
> 
> 	struct qcom_ipcc *ipcc = irq_data_get_irq_chip_data(irqd);
> 	irq_hw_number_t hwirq = irqd_to_hwirq(irqd);
> 
> Looks better.
> 
> > +
> > +	dev_dbg(proto_data->dev,
> > +		"Disabling interrupts for: client_id: %u; signal_id: %u\n",
> > +		sender_client_id, sender_signal_id);
> 
> Pushing out field dissection and "logging" to a tracepoint would make
> this function prettier as well.
> 

okay

> > +
> > +	writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_DISABLE);
> > +}
> > +
> > +static void qcom_ipcc_unmask_irq(struct irq_data *irqd)
> > +{
> > +	struct qcom_ipcc_proto_data *proto_data;
> > +	irq_hw_number_t hwirq = irqd_to_hwirq(irqd);
> > +	u16 sender_client_id = qcom_ipcc_get_client_id(hwirq);
> > +	u16 sender_signal_id = qcom_ipcc_get_signal_id(hwirq);
> > +
> > +	proto_data = irq_data_get_irq_chip_data(irqd);
> 
> As above.
> 
> > +
> > +	dev_dbg(proto_data->dev,
> > +		"Enabling interrupts for: client_id: %u; signal_id: %u\n",
> > +		sender_client_id, sender_signal_id);
> > +
> > +	writel(hwirq, proto_data->base + IPCC_REG_RECV_SIGNAL_ENABLE);
> > +}
> > +
> > +static struct irq_chip qcom_ipcc_irq_chip = {
> > +	.name = "ipcc",
> > +	.irq_mask = qcom_ipcc_mask_irq,
> > +	.irq_unmask = qcom_ipcc_unmask_irq,
> > +	.flags = IRQCHIP_SKIP_SET_WAKE,
> > +};
> > +
> > +static int qcom_ipcc_domain_map(struct irq_domain *d, unsigned int irq,
> > +				irq_hw_number_t hw)
> > +{
> > +	struct qcom_ipcc_proto_data *proto_data = d->host_data;
> > +
> > +	irq_set_chip_and_handler(irq, &qcom_ipcc_irq_chip, handle_level_irq);
> > +	irq_set_chip_data(irq, proto_data);
> > +	irq_set_noprobe(irq);
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_ipcc_domain_xlate(struct irq_domain *d,
> > +				  struct device_node *node, const u32 *intspec,
> > +				  unsigned int intsize,
> > +				  unsigned long *out_hwirq,
> > +				  unsigned int *out_type)
> > +{
> > +	struct qcom_ipcc_proto_data *proto_data = d->host_data;
> > +	struct device *dev = proto_data->dev;
> > +
> > +	if (intsize != 3)
> > +		return -EINVAL;
> > +
> > +	*out_hwirq = qcom_ipcc_get_packed_id(intspec[0], intspec[1]);
> > +	*out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
> > +
> > +	dev_dbg(dev, "hwirq: 0x%lx\n", *out_hwirq);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct irq_domain_ops qcom_ipcc_irq_ops = {
> > +	.map = qcom_ipcc_domain_map,
> > +	.xlate = qcom_ipcc_domain_xlate,
> > +};
> > +
> > +static int qcom_ipcc_mbox_send_data(struct mbox_chan *chan, void *data)
> > +{
> > +	struct qcom_ipcc_mbox_chan *ipcc_mbox_chan = chan->con_priv;
> > +	struct qcom_ipcc_proto_data *proto_data = ipcc_mbox_chan->proto_data;
> > +	u32 packed_id;
> 
> Although we're in the "mailbox section", I still think it's reasonable
> to name this hwirq for consistency (otherwise please shorten it to "id").
> 

okay, will name it as hwirq. 

> > +
> > +	dev_dbg(proto_data->dev,
> > +		"Generating IRQ for client_id: %u; signal_id: %u\n",
> > +		ipcc_mbox_chan->client_id, ipcc_mbox_chan->signal_id);
> > +
> > +	packed_id = qcom_ipcc_get_packed_id(ipcc_mbox_chan->client_id,
> > +					    ipcc_mbox_chan->signal_id);
> > +	writel(packed_id, proto_data->base + IPCC_REG_SEND_ID);
> > +
> > +	return 0;
> > +}
> > +
> > +static void qcom_ipcc_mbox_shutdown(struct mbox_chan *chan)
> > +{
> > +	struct qcom_ipcc_mbox_chan *ipcc_mbox_chan = chan->con_priv;
> > +
> > +	chan->con_priv = NULL;
> > +	kfree(ipcc_mbox_chan);
> > +}
> > +
> > +static struct mbox_chan *qcom_ipcc_mbox_xlate(struct mbox_controller *mbox,
> > +					const struct of_phandle_args *ph)
> > +{
> > +	struct device *dev;
> > +	struct qcom_ipcc_proto_data *proto_data;
> > +	struct qcom_ipcc_mbox_chan *ipcc_mbox_chan;
> > +	int chan_id;
> > +
> > +	proto_data = container_of(mbox, struct qcom_ipcc_proto_data, mbox);
> > +	if (WARN_ON(!proto_data))
> 
> Is this possible?
> 

hmm, not required.

> > +		return ERR_PTR(-EINVAL);
> > +
> > +	dev = proto_data->dev;
> > +
> > +	if (ph->args_count != 2)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	/* Check whether the mbox channel is allocated or not */
> > +	for (chan_id = 0; chan_id < mbox->num_chans; chan_id++) {
> > +		ipcc_mbox_chan = proto_data->chans[chan_id].con_priv;
> > +
> > +		if (!ipcc_mbox_chan)
> > +			break;
> > +		else if (ipcc_mbox_chan->client_id == ph->args[0] &&
> > +			 ipcc_mbox_chan->signal_id == ph->args[1])
> > +			return ERR_PTR(-EBUSY);
> 
> I think it would be better to just return the matching chan, and let
> mbox_request_channel() deal with the fact that it's already in use.
> 

okay

> > +	}
> > +
> > +	if (chan_id >= mbox->num_chans)
> > +		return ERR_PTR(-EBUSY);
> > +
> > +	ipcc_mbox_chan = kzalloc(sizeof(*ipcc_mbox_chan), GFP_KERNEL);
> 
> Can we allocate these during initialization time instead of allocating
> and releasing them in of_xlate and shutdown?
> 

We should know the max channels supported for allocating statically.

> > +	if (!ipcc_mbox_chan)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	ipcc_mbox_chan->client_id = ph->args[0];
> > +	ipcc_mbox_chan->signal_id = ph->args[1];
> > +	ipcc_mbox_chan->chan = &proto_data->chans[chan_id];
> > +	ipcc_mbox_chan->proto_data = proto_data;
> > +	ipcc_mbox_chan->chan->con_priv = ipcc_mbox_chan;
> > +
> > +	dev_dbg(dev,
> > +		"New mailbox channel: %u for client_id: %u; signal_id: %u\n",
> > +		chan_id, ipcc_mbox_chan->client_id,
> > +		ipcc_mbox_chan->signal_id);
> > +
> > +	return ipcc_mbox_chan->chan;
> > +}
> > +
> > +static const struct mbox_chan_ops ipcc_mbox_chan_ops = {
> > +	.send_data = qcom_ipcc_mbox_send_data,
> > +	.shutdown = qcom_ipcc_mbox_shutdown
> > +};
> > +
> > +static int qcom_ipcc_setup_mbox(struct qcom_ipcc_proto_data *proto_data,
> > +				struct device_node *controller_dn)
> > +{
> > +	struct mbox_controller *mbox;
> > +	struct device_node *client_dn;
> > +	struct device *dev = proto_data->dev;
> > +	struct of_phandle_args curr_ph;
> > +	int i, j, ret;
> > +	int num_chans = 0;
> > +
> > +	/*
> > +	 * Find out the number of clients interested in this mailbox
> > +	 * and create channels accordingly.
> > +	 */
> > +	for_each_node_with_property(client_dn, "mboxes") {
> 
> How much memory do we save by doing this, in comparison to just picking
> a max amount?
> 

Right, this is what I thought and was hoping that you'll get to it :)
But I don't know the max channels supported by IPCC.

Can someone help to find this?

> > +		if (!of_device_is_available(client_dn))
> > +			continue;
> > +		i = of_count_phandle_with_args(client_dn,
> > +					       "mboxes", "#mbox-cells");
> > +		for (j = 0; j < i; j++) {
> > +			ret = of_parse_phandle_with_args(client_dn, "mboxes",
> > +							 "#mbox-cells", j,
> > +							 &curr_ph);
> > +			of_node_put(curr_ph.np);
> > +			if (!ret && curr_ph.np == controller_dn) {
> > +				num_chans++;
> > +				break;
> > +			}
> > +		}
> > +	}
> > +
> > +	/* If no clients are found, skip registering as a mbox controller */
> > +	if (!num_chans)
> > +		return 0;
> > +
> > +	proto_data->chans = devm_kcalloc(dev, num_chans,
> > +					 sizeof(struct mbox_chan), GFP_KERNEL);
> 
> sizeof(*proto_data->chans)
> 
> > +	if (!proto_data->chans)
> > +		return -ENOMEM;
> > +
> > +	mbox = &proto_data->mbox;
> > +	mbox->dev = dev;
> > +	mbox->num_chans = num_chans;
> > +	mbox->chans = proto_data->chans;
> > +	mbox->ops = &ipcc_mbox_chan_ops;
> > +	mbox->of_xlate = qcom_ipcc_mbox_xlate;
> > +	mbox->txdone_irq = false;
> > +	mbox->txdone_poll = false;
> > +
> > +	return devm_mbox_controller_register(dev, mbox);
> > +}
> > +
> > +static int qcom_ipcc_probe(struct platform_device *pdev)
> > +{
> > +	struct qcom_ipcc_proto_data *proto_data;
> > +	int ret;
> > +
> > +	proto_data = devm_kzalloc(&pdev->dev, sizeof(*proto_data), GFP_KERNEL);
> > +	if (!proto_data)
> > +		return -ENOMEM;
> > +
> > +	ipcc_proto_data = proto_data;
> 
> ipcc_protocol_data is unused.
> 

Ack, this is a leftover.

> > +	proto_data->dev = &pdev->dev;
> > +
> > +	proto_data->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(proto_data->base)) {
> > +		dev_err(&pdev->dev, "Failed to ioremap the ipcc base addr\n");
> > +		return PTR_ERR(proto_data->base);
> > +	}
> > +
> > +	proto_data->irq = platform_get_irq(pdev, 0);
> > +	if (proto_data->irq < 0) {
> > +		dev_err(&pdev->dev, "Failed to get the IRQ\n");
> 
> platform_get_irq() already did print.
> 
> > +		return proto_data->irq;
> > +	}
> > +
> > +	/* Perform a SW reset on this client's protocol state */
> > +	writel(0x1, proto_data->base + IPCC_REG_CLIENT_CLEAR);
> > +
> > +	proto_data->irq_domain = irq_domain_add_tree(pdev->dev.of_node,
> > +						     &qcom_ipcc_irq_ops,
> > +						     proto_data);
> > +	if (!proto_data->irq_domain) {
> > +		dev_err(&pdev->dev, "Failed to add irq_domain\n");
> 
> Afaict this also prints errors.
> 
> > +		return -ENOMEM;
> > +	}
> > +
> > +	ret = qcom_ipcc_setup_mbox(proto_data, pdev->dev.of_node);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to create mailbox\n");
> 
> Afaict this too will print errors in all code paths (except the ones
> where you pass invalid data to mbox_controller_register() - but we're
> already past that).
> 

Okay, will remove.

Thanks,
Mani

> Regards,
> Bjorn
> 
> > +		goto err_mbox;
> > +	}
> > +
> > +	ret = devm_request_irq(&pdev->dev, proto_data->irq, qcom_ipcc_irq_fn,
> > +			       IRQF_TRIGGER_HIGH, "ipcc", proto_data);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret);
> > +		goto err_mbox;
> > +	}
> > +
> > +	enable_irq_wake(proto_data->irq);
> > +	platform_set_drvdata(pdev, proto_data);
> > +
> > +	return 0;
> > +
> > +err_mbox:
> > +	irq_domain_remove(proto_data->irq_domain);
> > +
> > +	return ret;
> > +}
> > +
> > +static int qcom_ipcc_remove(struct platform_device *pdev)
> > +{
> > +	struct qcom_ipcc_proto_data *proto_data = platform_get_drvdata(pdev);
> > +
> > +	disable_irq_wake(proto_data->irq);
> > +	irq_domain_remove(proto_data->irq_domain);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id qcom_ipcc_of_match[] = {
> > +	{ .compatible = "qcom,ipcc"},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, qcom_ipcc_of_match);
> > +
> > +static struct platform_driver qcom_ipcc_driver = {
> > +	.probe = qcom_ipcc_probe,
> > +	.remove = qcom_ipcc_remove,
> > +	.driver = {
> > +		.name = "qcom_ipcc",
> > +		.of_match_table = qcom_ipcc_of_match,
> > +	},
> > +};
> > +
> > +static int __init qcom_ipcc_init(void)
> > +{
> > +	return platform_driver_register(&qcom_ipcc_driver);
> > +}
> > +arch_initcall(qcom_ipcc_init);
> > +
> > +static void __exit qcom_ipcc_exit(void)
> > +{
> > +	platform_driver_unregister(&qcom_ipcc_driver);
> > +}
> > +module_exit(qcom_ipcc_exit);
> > +
> > +MODULE_LICENSE("GPL v2");
> > -- 
> > 2.17.1
> > 

Re: [PATCH 1/2] dt-bindings: soc: qcom: Add devicetree binding for Qcom IPCC

[Bjorn Andersson]

On Wed 29 Apr 23:30 PDT 2020, Manivannan Sadhasivam wrote:

> Add devicetree YAML binding for Qualcomm Inter-Processor Communication
> Controller (IPCC) block.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  .../bindings/soc/qcom/qcom,ipcc.yaml          | 85 +++++++++++++++++++

How about putting this in either interrupt-controller/ or mailbox/ instead?

>  include/dt-bindings/soc/qcom,ipcc.h           | 38 +++++++++
>  2 files changed, 123 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,ipcc.yaml
>  create mode 100644 include/dt-bindings/soc/qcom,ipcc.h
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,ipcc.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,ipcc.yaml
> new file mode 100644
> index 000000000000..48b281181401
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,ipcc.yaml
> @@ -0,0 +1,85 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/qcom/qcom,ipcc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies, Inc. Inter-Processor Communication Controller
> +
> +maintainers:
> +  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> +
> +description:
> +  The Inter-Processor Communication Controller (IPCC) is a centralized hardware
> +  to route the interrupts across various subsystems. It involves a three-level

s/the//

> +  addressing scheme called protocol, client and signal. For example, consider an
> +  entity on the Application Processor Subsystem (APSS) that wants to listen to
> +  Modem's interrupts via Shared Memory Point to Point (SMP2P) interface. In such
> +  a case, the client would be Modem (client-id is 2) and the signal would be
> +  SMP2P (signal-id is 2). The SMP2P itself falls under the Multiprocessor (MPROC)
> +  protocol (protocol-id is 0). Refer include/dt-bindings/soc/qcom/qcom,ipcc.h
> +  for the list of such IDs.
> +
> +  One of the duties of this interrupt controller driver is to forward the
> +  interrupts to the correct entities on the APSS. The children inheriting the

Clients using the...

> +  interrupt-controller would be mentioning the client-id and signal-id it's

s/would be mentioning/should specify/

> +  interested in.
> +
> +  On the other hand, sending an interrupt to a subsystem is done through the

"In the other direction," and add clarify subsystem by making it "remote
subsystem".

> +  mailbox interface, which again requires client-id and signal-id.
> +
> +properties:
> +  compatible:

It's uncertain how new vers

> +    const: "qcom,ipcc"
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 3
> +    description:
> +      The first cell is the client-id, the second cell is the signal-id and the
> +      third cell is the interrupt type.
> +
> +  "#mbox-cells":
> +    const: 2
> +    description:
> +      The first cell is the client-id, and the second cell is the signal-id.
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-controller
> +  - "#interrupt-cells"
> +  - "#mbox-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +        #include <dt-bindings/interrupt-controller/arm-gic.h>
> +        #include <dt-bindings/soc/qcom,ipcc.h>
> +
> +        ipcc_mproc: qcom,ipcc@408000 {

interrupt-controller@

Regards,
Bjorn

> +                compatible = "qcom,ipcc";
> +                reg = <0x408000 0x1000>;
> +                interrupts = <GIC_SPI 229 IRQ_TYPE_LEVEL_HIGH>;
> +                interrupt-controller;
> +                #interrupt-cells = <3>;
> +                #mbox-cells = <2>;
> +        };
> +
> +        smp2p-modem {
> +                compatible = "qcom,smp2p";
> +                interrupts-extended = <&ipcc_mproc IPCC_CLIENT_MPSS
> +                                IPCC_MPROC_SIGNAL_SMP2P IRQ_TYPE_EDGE_RISING>;
> +                mboxes = <&ipcc_mproc IPCC_CLIENT_MPSS IPCC_MPROC_SIGNAL_SMP2P>;
> +
> +                /* Other SMP2P fields */
> +        };
> diff --git a/include/dt-bindings/soc/qcom,ipcc.h b/include/dt-bindings/soc/qcom,ipcc.h
> new file mode 100644
> index 000000000000..2926cdb4cb48
> --- /dev/null
> +++ b/include/dt-bindings/soc/qcom,ipcc.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2018-2019, The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef __DT_BINDINGS_QCOM_IPCC_H
> +#define __DT_BINDINGS_QCOM_IPCC_H
> +
> +/* Signal IDs for MPROC protocol */
> +#define IPCC_MPROC_SIGNAL_GLINK_QMP	0
> +#define IPCC_MPROC_SIGNAL_SMP2P		2
> +#define IPCC_MPROC_SIGNAL_PING		3
> +#define IPCC_MPROC_SIGNAL_MAX		4 /* Used by driver only */
> +
> +#define IPCC_COMPUTE_L0_SIGNAL_MAX	32 /* Used by driver only */
> +#define IPCC_COMPUTE_L1_SIGNAL_MAX	32 /* Used by driver only */
> +
> +/* Client IDs */
> +#define IPCC_CLIENT_AOP			0
> +#define IPCC_CLIENT_TZ			1
> +#define IPCC_CLIENT_MPSS		2
> +#define IPCC_CLIENT_LPASS		3
> +#define IPCC_CLIENT_SLPI		4
> +#define IPCC_CLIENT_SDC			5
> +#define IPCC_CLIENT_CDSP		6
> +#define IPCC_CLIENT_NPU			7
> +#define IPCC_CLIENT_APSS		8
> +#define IPCC_CLIENT_GPU			9
> +#define IPCC_CLIENT_CVP			10
> +#define IPCC_CLIENT_CAM			11
> +#define IPCC_CLIENT_VPU			12
> +#define IPCC_CLIENT_PCIE0		13
> +#define IPCC_CLIENT_PCIE1		14
> +#define IPCC_CLIENT_PCIE2		15
> +#define IPCC_CLIENT_SPSS		16
> +#define IPCC_CLIENT_MAX			17 /* Used by driver only */
> +
> +#endif
> -- 
> 2.17.1
> 

Re: [PATCH 1/2] dt-bindings: soc: qcom: Add devicetree binding for Qcom IPCC

[Manivannan Sadhasivam]

On Thu, Apr 30, 2020 at 12:36:09PM -0700, Bjorn Andersson wrote:
> On Wed 29 Apr 23:30 PDT 2020, Manivannan Sadhasivam wrote:
> 
> > Add devicetree YAML binding for Qualcomm Inter-Processor Communication
> > Controller (IPCC) block.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  .../bindings/soc/qcom/qcom,ipcc.yaml          | 85 +++++++++++++++++++
> 
> How about putting this in either interrupt-controller/ or mailbox/ instead?
> 

I thought about it but was not sure. But if we want to move it to other
relevant location I think mailbox is a better one. Because, there are other
places where subsystem drivers expose irqchip functionality. So I think a
mailbox driver exposing irqchip is a relevant one for this.

> >  include/dt-bindings/soc/qcom,ipcc.h           | 38 +++++++++
> >  2 files changed, 123 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,ipcc.yaml
> >  create mode 100644 include/dt-bindings/soc/qcom,ipcc.h
> > 
> > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,ipcc.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,ipcc.yaml
> > new file mode 100644
> > index 000000000000..48b281181401
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,ipcc.yaml
> > @@ -0,0 +1,85 @@
> > +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/soc/qcom/qcom,ipcc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm Technologies, Inc. Inter-Processor Communication Controller
> > +
> > +maintainers:
> > +  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > +
> > +description:
> > +  The Inter-Processor Communication Controller (IPCC) is a centralized hardware
> > +  to route the interrupts across various subsystems. It involves a three-level
> 
> s/the//
> 
> > +  addressing scheme called protocol, client and signal. For example, consider an
> > +  entity on the Application Processor Subsystem (APSS) that wants to listen to
> > +  Modem's interrupts via Shared Memory Point to Point (SMP2P) interface. In such
> > +  a case, the client would be Modem (client-id is 2) and the signal would be
> > +  SMP2P (signal-id is 2). The SMP2P itself falls under the Multiprocessor (MPROC)
> > +  protocol (protocol-id is 0). Refer include/dt-bindings/soc/qcom/qcom,ipcc.h
> > +  for the list of such IDs.
> > +
> > +  One of the duties of this interrupt controller driver is to forward the
> > +  interrupts to the correct entities on the APSS. The children inheriting the
> 
> Clients using the...
> 
> > +  interrupt-controller would be mentioning the client-id and signal-id it's
> 
> s/would be mentioning/should specify/
> 
> > +  interested in.
> > +
> > +  On the other hand, sending an interrupt to a subsystem is done through the
> 
> "In the other direction," and add clarify subsystem by making it "remote
> subsystem".
> 
> > +  mailbox interface, which again requires client-id and signal-id.
> > +
> > +properties:
> > +  compatible:
> 
> It's uncertain how new vers
> 

lost?

> > +    const: "qcom,ipcc"
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-controller: true
> > +
> > +  "#interrupt-cells":
> > +    const: 3
> > +    description:
> > +      The first cell is the client-id, the second cell is the signal-id and the
> > +      third cell is the interrupt type.
> > +
> > +  "#mbox-cells":
> > +    const: 2
> > +    description:
> > +      The first cell is the client-id, and the second cell is the signal-id.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - interrupt-controller
> > +  - "#interrupt-cells"
> > +  - "#mbox-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +        #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +        #include <dt-bindings/soc/qcom,ipcc.h>
> > +
> > +        ipcc_mproc: qcom,ipcc@408000 {
> 
> interrupt-controller@
> 

mailbox?

Thanks,
Mani

> Regards,
> Bjorn
> 
> > +                compatible = "qcom,ipcc";
> > +                reg = <0x408000 0x1000>;
> > +                interrupts = <GIC_SPI 229 IRQ_TYPE_LEVEL_HIGH>;
> > +                interrupt-controller;
> > +                #interrupt-cells = <3>;
> > +                #mbox-cells = <2>;
> > +        };
> > +
> > +        smp2p-modem {
> > +                compatible = "qcom,smp2p";
> > +                interrupts-extended = <&ipcc_mproc IPCC_CLIENT_MPSS
> > +                                IPCC_MPROC_SIGNAL_SMP2P IRQ_TYPE_EDGE_RISING>;
> > +                mboxes = <&ipcc_mproc IPCC_CLIENT_MPSS IPCC_MPROC_SIGNAL_SMP2P>;
> > +
> > +                /* Other SMP2P fields */
> > +        };
> > diff --git a/include/dt-bindings/soc/qcom,ipcc.h b/include/dt-bindings/soc/qcom,ipcc.h
> > new file mode 100644
> > index 000000000000..2926cdb4cb48
> > --- /dev/null
> > +++ b/include/dt-bindings/soc/qcom,ipcc.h
> > @@ -0,0 +1,38 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2018-2019, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#ifndef __DT_BINDINGS_QCOM_IPCC_H
> > +#define __DT_BINDINGS_QCOM_IPCC_H
> > +
> > +/* Signal IDs for MPROC protocol */
> > +#define IPCC_MPROC_SIGNAL_GLINK_QMP	0
> > +#define IPCC_MPROC_SIGNAL_SMP2P		2
> > +#define IPCC_MPROC_SIGNAL_PING		3
> > +#define IPCC_MPROC_SIGNAL_MAX		4 /* Used by driver only */
> > +
> > +#define IPCC_COMPUTE_L0_SIGNAL_MAX	32 /* Used by driver only */
> > +#define IPCC_COMPUTE_L1_SIGNAL_MAX	32 /* Used by driver only */
> > +
> > +/* Client IDs */
> > +#define IPCC_CLIENT_AOP			0
> > +#define IPCC_CLIENT_TZ			1
> > +#define IPCC_CLIENT_MPSS		2
> > +#define IPCC_CLIENT_LPASS		3
> > +#define IPCC_CLIENT_SLPI		4
> > +#define IPCC_CLIENT_SDC			5
> > +#define IPCC_CLIENT_CDSP		6
> > +#define IPCC_CLIENT_NPU			7
> > +#define IPCC_CLIENT_APSS		8
> > +#define IPCC_CLIENT_GPU			9
> > +#define IPCC_CLIENT_CVP			10
> > +#define IPCC_CLIENT_CAM			11
> > +#define IPCC_CLIENT_VPU			12
> > +#define IPCC_CLIENT_PCIE0		13
> > +#define IPCC_CLIENT_PCIE1		14
> > +#define IPCC_CLIENT_PCIE2		15
> > +#define IPCC_CLIENT_SPSS		16
> > +#define IPCC_CLIENT_MAX			17 /* Used by driver only */
> > +
> > +#endif
> > -- 
> > 2.17.1
> >