[PATCH 0/4] mailbox: arm_mhu: Add ARM MHU doorbell controller driver

[PATCH 0/4] mailbox: arm_mhu: Add ARM MHU doorbell controller driver

[Sudeep Holla]

Hi,

These series adds ARM MHU doorbell controller driver based on the
discussion[1]. The DT patches are just repost from Viresh's last posting[2]

Regards,
Sudeep

[1] https://lore.kernel.org/lkml/20200909044618.deyx37pzocxiga7u@vireshk-i7
[2] https://lore.kernel.org/r/3874de094d193a08624a00a35067a3237e0b42b1.1600249102.git.viresh.kumar@linaro.org


Sudeep Holla (3):
  dt-bindings: mailbox: add doorbell support to ARM MHU
  mailbox: arm_mhu: Match only if compatible is "arm,mhu"
  mailbox: arm_mhu: Add ARM MHU doorbell driver

Viresh Kumar (1):
  dt-bindings: mailbox : arm,mhu: Convert to Json-schema

 .../devicetree/bindings/mailbox/arm,mhu.yaml  | 135 +++++++
 .../devicetree/bindings/mailbox/arm-mhu.txt   |  43 ---
 drivers/mailbox/Makefile                      |   2 +-
 drivers/mailbox/arm_mhu.c                     |   3 +
 drivers/mailbox/arm_mhu_db.c                  | 359 ++++++++++++++++++
 5 files changed, 498 insertions(+), 44 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mailbox/arm,mhu.yaml
 delete mode 100644 Documentation/devicetree/bindings/mailbox/arm-mhu.txt
 create mode 100644 drivers/mailbox/arm_mhu_db.c

-- 
2.17.1

[PATCH 1/4] dt-bindings: mailbox : arm,mhu: Convert to Json-schema

[Sudeep Holla]

From: Viresh Kumar <viresh.kumar@linaro.org>

Convert the DT binding over to Json-schema.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 .../devicetree/bindings/mailbox/arm,mhu.yaml  | 87 +++++++++++++++++++
 .../devicetree/bindings/mailbox/arm-mhu.txt   | 43 ---------
 2 files changed, 87 insertions(+), 43 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mailbox/arm,mhu.yaml
 delete mode 100644 Documentation/devicetree/bindings/mailbox/arm-mhu.txt

diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhu.yaml b/Documentation/devicetree/bindings/mailbox/arm,mhu.yaml
new file mode 100644
index 000000000000..2c8df7979c22
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/arm,mhu.yaml
@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/arm,mhu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ARM MHU Mailbox Controller
+
+maintainers:
+  - Jassi Brar <jaswinder.singh@linaro.org>
+
+description: |
+  The ARM's Message-Handling-Unit (MHU) is a mailbox controller that has 3
+  independent channels/links to communicate with remote processor(s).  MHU links
+  are hardwired on a platform. A link raises interrupt for any received data.
+  However, there is no specified way of knowing if the sent data has been read
+  by the remote. This driver assumes the sender polls STAT register and the
+  remote clears it after having read the data.  The last channel is specified to
+  be a 'Secure' resource, hence can't be used by Linux running NS.
+
+# We need a select here so we don't match all nodes with 'arm,primecell'
+select:
+  properties:
+    compatible:
+      contains:
+        const: arm,mhu
+  required:
+    - compatible
+
+properties:
+  compatible:
+    items:
+      - const: arm,mhu
+      - const: arm,primecell
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    items:
+      - description: low-priority non-secure
+      - description: high-priority non-secure
+      - description: Secure
+    maxItems: 3
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: apb_pclk
+
+  '#mbox-cells':
+    description: Index of the channel.
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - '#mbox-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        mhuA: mailbox@2b1f0000 {
+            #mbox-cells = <1>;
+            compatible = "arm,mhu", "arm,primecell";
+            reg = <0 0x2b1f0000 0 0x1000>;
+            interrupts = <0 36 4>, /* LP-NonSecure */
+                         <0 35 4>, /* HP-NonSecure */
+                         <0 37 4>; /* Secure */
+            clocks = <&clock 0 2 1>;
+            clock-names = "apb_pclk";
+        };
+
+        mhu_client_scb: scb@2e000000 {
+            compatible = "fujitsu,mb86s70-scb-1.0";
+            reg = <0 0x2e000000 0 0x4000>;
+            mboxes = <&mhuA 1>; /* HP-NonSecure */
+        };
+    };
diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
deleted file mode 100644
index 4971f03f0b33..000000000000
--- a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
+++ /dev/null
@@ -1,43 +0,0 @@
-ARM MHU Mailbox Driver
-======================
-
-The ARM's Message-Handling-Unit (MHU) is a mailbox controller that has
-3 independent channels/links to communicate with remote processor(s).
- MHU links are hardwired on a platform. A link raises interrupt for any
-received data. However, there is no specified way of knowing if the sent
-data has been read by the remote. This driver assumes the sender polls
-STAT register and the remote clears it after having read the data.
-The last channel is specified to be a 'Secure' resource, hence can't be
-used by Linux running NS.
-
-Mailbox Device Node:
-====================
-
-Required properties:
---------------------
-- compatible:		Shall be "arm,mhu" & "arm,primecell"
-- reg:			Contains the mailbox register address range (base
-			address and length)
-- #mbox-cells		Shall be 1 - the index of the channel needed.
-- interrupts:		Contains the interrupt information corresponding to
-			each of the 3 links of MHU.
-
-Example:
---------
-
-	mhu: mailbox@2b1f0000 {
-		#mbox-cells = <1>;
-		compatible = "arm,mhu", "arm,primecell";
-		reg = <0 0x2b1f0000 0x1000>;
-		interrupts = <0 36 4>, /* LP-NonSecure */
-			     <0 35 4>, /* HP-NonSecure */
-			     <0 37 4>; /* Secure */
-		clocks = <&clock 0 2 1>;
-		clock-names = "apb_pclk";
-	};
-
-	mhu_client: scb@2e000000 {
-		compatible = "fujitsu,mb86s70-scb-1.0";
-		reg = <0 0x2e000000 0x4000>;
-		mboxes = <&mhu 1>; /* HP-NonSecure */
-	};
-- 
2.17.1

[PATCH 2/4] dt-bindings: mailbox: add doorbell support to ARM MHU

[Sudeep Holla]

The ARM MHU's reference manual states following:

"The MHU drives the signal using a 32-bit register, with all 32 bits
logically ORed together. The MHU provides a set of registers to enable
software to set, clear, and check the status of each of the bits of this
register independently.  The use of 32 bits for each interrupt line
enables software to provide more information about the source of the
interrupt. For example, each bit of the register can be associated with
a type of event that can contribute to raising the interrupt."

This patch thus extends the MHU controller's DT binding to add support
for doorbell mode.

Though the same MHU hardware controller is used in the two modes, A new
compatible string is added here to represent the combination of the MHU
hardware and the firmware sitting on the other side (which expects each
bit to represent a different signal now).

Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Co-developed-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 .../devicetree/bindings/mailbox/arm,mhu.yaml  | 60 +++++++++++++++++--
 1 file changed, 54 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhu.yaml b/Documentation/devicetree/bindings/mailbox/arm,mhu.yaml
index 2c8df7979c22..d43791a2dde7 100644
--- a/Documentation/devicetree/bindings/mailbox/arm,mhu.yaml
+++ b/Documentation/devicetree/bindings/mailbox/arm,mhu.yaml
@@ -18,20 +18,40 @@ description: |
   remote clears it after having read the data.  The last channel is specified to
   be a 'Secure' resource, hence can't be used by Linux running NS.
 
+  The MHU hardware also allows operations in doorbell mode. The MHU drives the
+  interrupt signal using a 32-bit register, with all 32-bits logically ORed
+  together. It provides a set of registers to enable software to set, clear and
+  check the status of each of the bits of this register independently. The use
+  of 32 bits per interrupt line enables software to provide more information
+  about the source of the interrupt. For example, each bit of the register can
+  be associated with a type of event that can contribute to raising the
+  interrupt. Each of the 32-bits can be used as "doorbell" to alert the remote
+  processor.
+
 # We need a select here so we don't match all nodes with 'arm,primecell'
 select:
   properties:
     compatible:
       contains:
-        const: arm,mhu
+        enum:
+          - arm,mhu
+          - arm,mhu-doorbell
   required:
     - compatible
 
 properties:
   compatible:
-    items:
-      - const: arm,mhu
-      - const: arm,primecell
+    oneOf:
+      - description: Data transfer mode
+        items:
+          - const: arm,mhu
+          - const: arm,primecell
+
+      - description: Doorbell mode
+        items:
+          - const: arm,mhu-doorbell
+          - const: arm,primecell
+
 
   reg:
     maxItems: 1
@@ -51,8 +71,11 @@ description: |
       - const: apb_pclk
 
   '#mbox-cells':
-    description: Index of the channel.
-    const: 1
+    description: |
+      Set to 1 in data transfer mode and represents index of the channel.
+      Set to 2 in doorbell mode and represents index of the channel and doorbell
+      number.
+    enum: [ 1, 2 ]
 
 required:
   - compatible
@@ -63,6 +86,7 @@ description: |
 additionalProperties: false
 
 examples:
+  # Data transfer mode.
   - |
     soc {
         #address-cells = <2>;
@@ -85,3 +109,27 @@ additionalProperties: false
             mboxes = <&mhuA 1>; /* HP-NonSecure */
         };
     };
+
+  # Doorbell mode.
+  - |
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        mhuB: mailbox@2b2f0000 {
+            #mbox-cells = <2>;
+            compatible = "arm,mhu-doorbell", "arm,primecell";
+            reg = <0 0x2b2f0000 0 0x1000>;
+            interrupts = <0 36 4>, /* LP-NonSecure */
+                         <0 35 4>, /* HP-NonSecure */
+                         <0 37 4>; /* Secure */
+            clocks = <&clock 0 2 1>;
+            clock-names = "apb_pclk";
+        };
+
+        mhu_client_scpi: scpi@2f000000 {
+            compatible = "arm,scpi";
+            reg = <0 0x2f000000 0 0x200>;
+            mboxes = <&mhuB 1 4>; /* HP-NonSecure, 5th doorbell */
+        };
+    };
-- 
2.17.1

[PATCH 3/4] mailbox: arm_mhu: Match only if compatible is "arm,mhu"

[Sudeep Holla]

Since we will be soon adding a separate driver based on this ARM MHU
driver to support doorbell mode, let us add explicit check to match
the default compatible for this driver. This is needed as the probe
and match reuses the AMBA device ids currently and don't have any
explicit compatible check.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/arm_mhu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index 9da236552bd7..b7fbf276eb62 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -113,6 +113,9 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 	struct device *dev = &adev->dev;
 	int mhu_reg[MHU_CHANS] = {MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET};
 
+	if (!of_device_is_compatible(dev->of_node, "arm,mhu"))
+		return -ENODEV;
+
 	/* Allocate memory for device */
 	mhu = devm_kzalloc(dev, sizeof(*mhu), GFP_KERNEL);
 	if (!mhu)
-- 
2.17.1

[PATCH 4/4] mailbox: arm_mhu: Add ARM MHU doorbell driver

[Sudeep Holla]

The MHU drives the signal using a 32-bit register, with all 32 bits
logically ORed together. The MHU provides a set of registers to enable
software to set, clear, and check the status of each of the bits of this
register independently. The use of 32 bits for each interrupt line
enables software to provide more information about the source of the
interrupt. For example, each bit of the register can be associated with
a type of event that can contribute to raising the interrupt.

This patch adds a separate the MHU controller driver for doorbel mode
of operation using the extended DT binding to add support the same.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/Makefile     |   2 +-
 drivers/mailbox/arm_mhu_db.c | 359 +++++++++++++++++++++++++++++++++++
 2 files changed, 360 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mailbox/arm_mhu_db.c

diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 60d224b723a1..2e06e02b2e03 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -5,7 +5,7 @@ obj-$(CONFIG_MAILBOX)		+= mailbox.o
 
 obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
 
-obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
+obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o arm_mhu_db.o
 
 obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
 
diff --git a/drivers/mailbox/arm_mhu_db.c b/drivers/mailbox/arm_mhu_db.c
new file mode 100644
index 000000000000..ef5fba4ed54c
--- /dev/null
+++ b/drivers/mailbox/arm_mhu_db.c
@@ -0,0 +1,359 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2013-2015 Fujitsu Semiconductor Ltd.
+ * Copyright (C) 2015 Linaro Ltd.
+ * Based on ARM MHU driver by Jassi Brar <jaswinder.singh@linaro.org>
+ * Copyright (C) 2020 ARM Ltd.
+ */
+
+#include <linux/amba/bus.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+
+#define INTR_STAT_OFS	0x0
+#define INTR_SET_OFS	0x8
+#define INTR_CLR_OFS	0x10
+
+#define MHU_LP_OFFSET	0x0
+#define MHU_HP_OFFSET	0x20
+#define MHU_SEC_OFFSET	0x200
+#define TX_REG_OFFSET	0x100
+
+#define MHU_CHANS	3	/* Secure, Non-Secure High and Low Priority */
+#define MHU_CHAN_MAX	20	/* Max channels to save on unused RAM */
+#define MHU_NUM_DOORBELLS	32
+
+struct mhu_db_link {
+	unsigned int irq;
+	void __iomem *tx_reg;
+	void __iomem *rx_reg;
+};
+
+struct arm_mhu {
+	void __iomem *base;
+	struct mhu_db_link mlink[MHU_CHANS];
+	struct mbox_controller mbox;
+	struct device *dev;
+};
+
+/**
+ * ARM MHU Mailbox allocated channel information
+ *
+ * @mhu: Pointer to parent mailbox device
+ * @pchan: Physical channel within which this doorbell resides in
+ * @doorbell: doorbell number pertaining to this channel
+ */
+struct mhu_db_channel {
+	struct arm_mhu *mhu;
+	unsigned int pchan;
+	unsigned int doorbell;
+};
+
+static inline struct mbox_chan *
+mhu_db_mbox_to_channel(struct mbox_controller *mbox, unsigned int pchan,
+		       unsigned int doorbell)
+{
+	int i;
+	struct mhu_db_channel *chan_info;
+
+	for (i = 0; i < mbox->num_chans; i++) {
+		chan_info = mbox->chans[i].con_priv;
+		if (chan_info && chan_info->pchan == pchan &&
+		    chan_info->doorbell == doorbell)
+			return &mbox->chans[i];
+	}
+
+	dev_err(mbox->dev,
+		"Channel not registered: physical channel: %d doorbell: %d\n",
+		pchan, doorbell);
+
+	return NULL;
+}
+
+static void mhu_db_mbox_clear_irq(struct mbox_chan *chan)
+{
+	struct mhu_db_channel *chan_info = chan->con_priv;
+	void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].rx_reg;
+
+	writel_relaxed(BIT(chan_info->doorbell), base + INTR_CLR_OFS);
+}
+
+static unsigned int mhu_db_mbox_irq_to_pchan_num(struct arm_mhu *mhu, int irq)
+{
+	unsigned int pchan;
+
+	for (pchan = 0; pchan < MHU_CHANS; pchan++)
+		if (mhu->mlink[pchan].irq == irq)
+			break;
+	return pchan;
+}
+
+static struct mbox_chan *
+mhu_db_mbox_irq_to_channel(struct arm_mhu *mhu, unsigned int pchan)
+{
+	unsigned long bits;
+	unsigned int doorbell;
+	struct mbox_chan *chan = NULL;
+	struct mbox_controller *mbox = &mhu->mbox;
+	void __iomem *base = mhu->mlink[pchan].rx_reg;
+
+	bits = readl_relaxed(base + INTR_STAT_OFS);
+	if (!bits)
+		/* No IRQs fired in specified physical channel */
+		return NULL;
+
+	/* An IRQ has fired, find the associated channel */
+	for (doorbell = 0; bits; doorbell++) {
+		if (!test_and_clear_bit(doorbell, &bits))
+			continue;
+
+		chan = mhu_db_mbox_to_channel(mbox, pchan, doorbell);
+		if (chan)
+			break;
+	}
+
+	return chan;
+}
+
+static irqreturn_t mhu_db_mbox_rx_handler(int irq, void *data)
+{
+	struct mbox_chan *chan;
+	struct arm_mhu *mhu = data;
+	unsigned int pchan = mhu_db_mbox_irq_to_pchan_num(mhu, irq);
+
+	while (NULL != (chan = mhu_db_mbox_irq_to_channel(mhu, pchan))) {
+		mbox_chan_received_data(chan, NULL);
+		mhu_db_mbox_clear_irq(chan);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static bool mhu_db_last_tx_done(struct mbox_chan *chan)
+{
+	struct mhu_db_channel *chan_info = chan->con_priv;
+	void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].tx_reg;
+
+	if (readl_relaxed(base + INTR_STAT_OFS) & BIT(chan_info->doorbell))
+		return false;
+
+	return true;
+}
+
+static int mhu_db_send_data(struct mbox_chan *chan, void *data)
+{
+	struct mhu_db_channel *chan_info = chan->con_priv;
+	void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].tx_reg;
+
+	/* Send event to co-processor */
+	writel_relaxed(BIT(chan_info->doorbell), base + INTR_SET_OFS);
+
+	return 0;
+}
+
+static int mhu_db_startup(struct mbox_chan *chan)
+{
+	mhu_db_mbox_clear_irq(chan);
+	return 0;
+}
+
+static void mhu_db_shutdown(struct mbox_chan *chan)
+{
+	struct mhu_db_channel *chan_info = chan->con_priv;
+	struct mbox_controller *mbox = &chan_info->mhu->mbox;
+	int i;
+
+	for (i = 0; i < mbox->num_chans; i++)
+		if (chan == &mbox->chans[i])
+			break;
+
+	if (mbox->num_chans == i) {
+		dev_warn(mbox->dev, "Request to free non-existent channel\n");
+		return;
+	}
+
+	/* Reset channel */
+	mhu_db_mbox_clear_irq(chan);
+	chan->con_priv = NULL;
+}
+
+static struct mbox_chan *mhu_db_mbox_xlate(struct mbox_controller *mbox,
+					   const struct of_phandle_args *spec)
+{
+	struct arm_mhu *mhu = dev_get_drvdata(mbox->dev);
+	struct mhu_db_channel *chan_info;
+	struct mbox_chan *chan = NULL;
+	unsigned int pchan = spec->args[0];
+	unsigned int doorbell = spec->args[1];
+	int i;
+
+	/* Bounds checking */
+	if (pchan >= MHU_CHANS || doorbell >= MHU_NUM_DOORBELLS) {
+		dev_err(mbox->dev,
+			"Invalid channel requested pchan: %d doorbell: %d\n",
+			pchan, doorbell);
+		return ERR_PTR(-EINVAL);
+	}
+
+	for (i = 0; i < mbox->num_chans; i++) {
+		chan_info = mbox->chans[i].con_priv;
+
+		/* Is requested channel free? */
+		if (chan_info &&
+		    mbox->dev == chan_info->mhu->dev &&
+		    pchan == chan_info->pchan &&
+		    doorbell == chan_info->doorbell) {
+			dev_err(mbox->dev, "Channel in use\n");
+			return ERR_PTR(-EBUSY);
+		}
+
+		/*
+		 * Find the first free slot, then continue checking
+		 * to see if requested channel is in use
+		 */
+		if (!chan && !chan_info)
+			chan = &mbox->chans[i];
+	}
+
+	if (!chan) {
+		dev_err(mbox->dev, "No free channels left\n");
+		return ERR_PTR(-EBUSY);
+	}
+
+	chan_info = devm_kzalloc(mbox->dev, sizeof(*chan_info), GFP_KERNEL);
+	if (!chan_info)
+		return ERR_PTR(-ENOMEM);
+
+	chan_info->mhu = mhu;
+	chan_info->pchan = pchan;
+	chan_info->doorbell = doorbell;
+
+	chan->con_priv = chan_info;
+
+	dev_dbg(mbox->dev, "mbox: created channel phys: %d doorbell: %d\n",
+		pchan, doorbell);
+
+	return chan;
+}
+
+static const struct mbox_chan_ops mhu_db_ops = {
+	.send_data = mhu_db_send_data,
+	.startup = mhu_db_startup,
+	.shutdown = mhu_db_shutdown,
+	.last_tx_done = mhu_db_last_tx_done,
+};
+
+static int mhu_db_probe(struct amba_device *adev, const struct amba_id *id)
+{
+	u32 cell_count;
+	int i, err, max_chans;
+	struct arm_mhu *mhu;
+	struct mbox_chan *chans;
+	struct device *dev = &adev->dev;
+	struct device_node *np = dev->of_node;
+	int mhu_reg[MHU_CHANS] = {
+		MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET,
+	};
+
+	if (!of_device_is_compatible(np, "arm,mhu-doorbell"))
+		return -ENODEV;
+
+	err = of_property_read_u32(np, "#mbox-cells", &cell_count);
+	if (err) {
+		dev_err(dev, "failed to read #mbox-cells in '%pOF'\n", np);
+		return err;
+	}
+
+	if (cell_count == 2) {
+		max_chans = MHU_CHAN_MAX;
+	} else {
+		dev_err(dev, "incorrect value of #mbox-cells in '%pOF'\n", np);
+		return -EINVAL;
+	}
+
+	mhu = devm_kzalloc(dev, sizeof(*mhu), GFP_KERNEL);
+	if (!mhu)
+		return -ENOMEM;
+
+	mhu->base = devm_ioremap_resource(dev, &adev->res);
+	if (IS_ERR(mhu->base)) {
+		dev_err(dev, "ioremap failed\n");
+		return PTR_ERR(mhu->base);
+	}
+
+	chans = devm_kcalloc(dev, max_chans, sizeof(*chans), GFP_KERNEL);
+	if (!chans)
+		return -ENOMEM;
+
+	mhu->dev = dev;
+	mhu->mbox.dev = dev;
+	mhu->mbox.chans = chans;
+	mhu->mbox.num_chans = max_chans;
+	mhu->mbox.txdone_irq = false;
+	mhu->mbox.txdone_poll = true;
+	mhu->mbox.txpoll_period = 1;
+
+	mhu->mbox.of_xlate = mhu_db_mbox_xlate;
+	amba_set_drvdata(adev, mhu);
+
+	mhu->mbox.ops = &mhu_db_ops;
+
+	err = devm_mbox_controller_register(dev, &mhu->mbox);
+	if (err) {
+		dev_err(dev, "Failed to register mailboxes %d\n", err);
+		return err;
+	}
+
+	for (i = 0; i < MHU_CHANS; i++) {
+		int irq = mhu->mlink[i].irq = adev->irq[i];
+
+		if (irq <= 0) {
+			dev_dbg(dev, "No IRQ found for Channel %d\n", i);
+			continue;
+		}
+
+		mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i];
+		mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + TX_REG_OFFSET;
+
+		err = devm_request_threaded_irq(dev, irq, NULL,
+						mhu_db_mbox_rx_handler,
+						IRQF_ONESHOT, "mhu_db_link", mhu);
+		if (err) {
+			dev_err(dev, "Can't claim IRQ %d\n", irq);
+			mbox_controller_unregister(&mhu->mbox);
+			return err;
+		}
+	}
+
+	dev_info(dev, "ARM MHU Doorbell mailbox registered\n");
+	return 0;
+}
+
+static struct amba_id mhu_ids[] = {
+	{
+		.id	= 0x1bb098,
+		.mask	= 0xffffff,
+	},
+	{ 0, 0 },
+};
+MODULE_DEVICE_TABLE(amba, mhu_ids);
+
+static struct amba_driver arm_mhu_db_driver = {
+	.drv = {
+		.name	= "mhu-doorbell",
+	},
+	.id_table	= mhu_ids,
+	.probe		= mhu_db_probe,
+};
+module_amba_driver(arm_mhu_db_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("ARM MHU Doorbell Driver");
+MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
-- 
2.17.1

Re: [PATCH 4/4] mailbox: arm_mhu: Add ARM MHU doorbell driver

[Jassi Brar]

On Mon, Sep 28, 2020 at 6:45 AM Sudeep Holla <sudeep.holla@arm.com> wrote:

> +
> +static void mhu_db_shutdown(struct mbox_chan *chan)
> +{
> +       struct mhu_db_channel *chan_info = chan->con_priv;
> +       struct mbox_controller *mbox = &chan_info->mhu->mbox;
> +       int i;
> +
> +       for (i = 0; i < mbox->num_chans; i++)
> +               if (chan == &mbox->chans[i])
> +                       break;
> +
> +       if (mbox->num_chans == i) {
> +               dev_warn(mbox->dev, "Request to free non-existent channel\n");
> +               return;
> +       }
> +
> +       /* Reset channel */
> +       mhu_db_mbox_clear_irq(chan);
> +       chan->con_priv = NULL;
>
request->free->request will fail because of this NULL assignment.
Maybe add a 'taken' flag in mhu_db_channel, which should also be
checked before calling mbox_chan_received_data because the data may
arrive for a now relinquished channel.

> +
> +static struct mbox_chan *mhu_db_mbox_xlate(struct mbox_controller *mbox,
> +                                          const struct of_phandle_args *spec)
> +{
> +       struct arm_mhu *mhu = dev_get_drvdata(mbox->dev);
> +       struct mhu_db_channel *chan_info;
> +       struct mbox_chan *chan = NULL;
> +       unsigned int pchan = spec->args[0];
> +       unsigned int doorbell = spec->args[1];
> +       int i;
> +
> +       /* Bounds checking */
> +       if (pchan >= MHU_CHANS || doorbell >= MHU_NUM_DOORBELLS) {
> +               dev_err(mbox->dev,
> +                       "Invalid channel requested pchan: %d doorbell: %d\n",
> +                       pchan, doorbell);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       for (i = 0; i < mbox->num_chans; i++) {
> +               chan_info = mbox->chans[i].con_priv;
> +
> +               /* Is requested channel free? */
> +               if (chan_info &&
> +                   mbox->dev == chan_info->mhu->dev &&
> +                   pchan == chan_info->pchan &&
> +                   doorbell == chan_info->doorbell) {
> +                       dev_err(mbox->dev, "Channel in use\n");
> +                       return ERR_PTR(-EBUSY);
> +               }
> +
You may want to reuse mhu_db_mbox_to_channel.

Re: [PATCH 4/4] mailbox: arm_mhu: Add ARM MHU doorbell driver

[Viresh Kumar]

On 28-09-20, 12:44, Sudeep Holla wrote:
> The MHU drives the signal using a 32-bit register, with all 32 bits
> logically ORed together. The MHU provides a set of registers to enable
> software to set, clear, and check the status of each of the bits of this
> register independently. The use of 32 bits for each interrupt line
> enables software to provide more information about the source of the
> interrupt. For example, each bit of the register can be associated with
> a type of event that can contribute to raising the interrupt.
> 
> This patch adds a separate the MHU controller driver for doorbel mode
> of operation using the extended DT binding to add support the same.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/mailbox/Makefile     |   2 +-
>  drivers/mailbox/arm_mhu_db.c | 359 +++++++++++++++++++++++++++++++++++

Please put an entry in MAINTAINERS as well for this.

-- 
viresh

Re: [PATCH 4/4] mailbox: arm_mhu: Add ARM MHU doorbell driver

[Sudeep Holla]

On Fri, Oct 02, 2020 at 02:42:37PM -0500, Jassi Brar wrote:
> On Mon, Sep 28, 2020 at 6:45 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> 
> > +
> > +static void mhu_db_shutdown(struct mbox_chan *chan)
> > +{
> > +       struct mhu_db_channel *chan_info = chan->con_priv;
> > +       struct mbox_controller *mbox = &chan_info->mhu->mbox;
> > +       int i;
> > +
> > +       for (i = 0; i < mbox->num_chans; i++)
> > +               if (chan == &mbox->chans[i])
> > +                       break;
> > +
> > +       if (mbox->num_chans == i) {
> > +               dev_warn(mbox->dev, "Request to free non-existent channel\n");
> > +               return;
> > +       }
> > +
> > +       /* Reset channel */
> > +       mhu_db_mbox_clear_irq(chan);
> > +       chan->con_priv = NULL;
> >
> request->free->request will fail because of this NULL assignment.
> Maybe add a 'taken' flag in mhu_db_channel, which should also be
> checked before calling mbox_chan_received_data because the data may
> arrive for a now relinquished channel.
>

Good point, but the new 'taken' flag will have the same race as con_priv.
We need a lock here and can we use chan->lock or do you prefer this
driver maintains it own for this purpose.

mbox_request_channel releases the lock before calling startup and
mbox_free_channel acquires the after shutdown returns, so technically
we can reuse the same lock.

> > +
> > +static struct mbox_chan *mhu_db_mbox_xlate(struct mbox_controller *mbox,
> > +                                          const struct of_phandle_args *spec)
> > +{
> > +       struct arm_mhu *mhu = dev_get_drvdata(mbox->dev);
> > +       struct mhu_db_channel *chan_info;
> > +       struct mbox_chan *chan = NULL;
> > +       unsigned int pchan = spec->args[0];
> > +       unsigned int doorbell = spec->args[1];
> > +       int i;
> > +
> > +       /* Bounds checking */
> > +       if (pchan >= MHU_CHANS || doorbell >= MHU_NUM_DOORBELLS) {
> > +               dev_err(mbox->dev,
> > +                       "Invalid channel requested pchan: %d doorbell: %d\n",
> > +                       pchan, doorbell);
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       for (i = 0; i < mbox->num_chans; i++) {
> > +               chan_info = mbox->chans[i].con_priv;
> > +
> > +               /* Is requested channel free? */
> > +               if (chan_info &&
> > +                   mbox->dev == chan_info->mhu->dev &&
> > +                   pchan == chan_info->pchan &&
> > +                   doorbell == chan_info->doorbell) {
> > +                       dev_err(mbox->dev, "Channel in use\n");
> > +                       return ERR_PTR(-EBUSY);
> > +               }
> > +
> You may want to reuse mhu_db_mbox_to_channel.

Good point, thanks for pointing that out, will update.

-- 
Regards,
Sudeep

Re: [PATCH 4/4] mailbox: arm_mhu: Add ARM MHU doorbell driver

[Sudeep Holla]

On Wed, Oct 07, 2020 at 11:35:25AM +0530, Viresh Kumar wrote:
> On 28-09-20, 12:44, Sudeep Holla wrote:
> > The MHU drives the signal using a 32-bit register, with all 32 bits
> > logically ORed together. The MHU provides a set of registers to enable
> > software to set, clear, and check the status of each of the bits of this
> > register independently. The use of 32 bits for each interrupt line
> > enables software to provide more information about the source of the
> > interrupt. For example, each bit of the register can be associated with
> > a type of event that can contribute to raising the interrupt.
> > 
> > This patch adds a separate the MHU controller driver for doorbel mode
> > of operation using the extended DT binding to add support the same.
> > 
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/mailbox/Makefile     |   2 +-
> >  drivers/mailbox/arm_mhu_db.c | 359 +++++++++++++++++++++++++++++++++++
> 
> Please put an entry in MAINTAINERS as well for this.

Sure.

-- 
Regards,
Sudeep

Re: [PATCH 4/4] mailbox: arm_mhu: Add ARM MHU doorbell driver

[Jassi Brar]

On Wed, Oct 7, 2020 at 6:40 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Oct 02, 2020 at 02:42:37PM -0500, Jassi Brar wrote:
> > On Mon, Sep 28, 2020 at 6:45 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > > +
> > > +static void mhu_db_shutdown(struct mbox_chan *chan)
> > > +{
> > > +       struct mhu_db_channel *chan_info = chan->con_priv;
> > > +       struct mbox_controller *mbox = &chan_info->mhu->mbox;
> > > +       int i;
> > > +
> > > +       for (i = 0; i < mbox->num_chans; i++)
> > > +               if (chan == &mbox->chans[i])
> > > +                       break;
> > > +
> > > +       if (mbox->num_chans == i) {
> > > +               dev_warn(mbox->dev, "Request to free non-existent channel\n");
> > > +               return;
> > > +       }
> > > +
> > > +       /* Reset channel */
> > > +       mhu_db_mbox_clear_irq(chan);
> > > +       chan->con_priv = NULL;
> > >
> > request->free->request will fail because of this NULL assignment.
> > Maybe add a 'taken' flag in mhu_db_channel, which should also be
> > checked before calling mbox_chan_received_data because the data may
> > arrive for a now relinquished channel.
> >
>
> Good point, but the new 'taken' flag will have the same race as con_priv.
> We need a lock here and can we use chan->lock or do you prefer this
> driver maintains it own for this purpose.
>
I meant the con_priv is allocated in mhu_db_mbox_xlate and simply
assigning it NULL leaks memory, if not a crash by some other path. At
least free it before.

-j

Re: [PATCH 4/4] mailbox: arm_mhu: Add ARM MHU doorbell driver

[Sudeep Holla]

On Wed, Oct 07, 2020 at 10:43:19AM -0500, Jassi Brar wrote:
> On Wed, Oct 7, 2020 at 6:40 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Fri, Oct 02, 2020 at 02:42:37PM -0500, Jassi Brar wrote:
> > > On Mon, Sep 28, 2020 at 6:45 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > > +
> > > > +static void mhu_db_shutdown(struct mbox_chan *chan)
> > > > +{
> > > > +       struct mhu_db_channel *chan_info = chan->con_priv;
> > > > +       struct mbox_controller *mbox = &chan_info->mhu->mbox;
> > > > +       int i;
> > > > +
> > > > +       for (i = 0; i < mbox->num_chans; i++)
> > > > +               if (chan == &mbox->chans[i])
> > > > +                       break;
> > > > +
> > > > +       if (mbox->num_chans == i) {
> > > > +               dev_warn(mbox->dev, "Request to free non-existent channel\n");
> > > > +               return;
> > > > +       }
> > > > +
> > > > +       /* Reset channel */
> > > > +       mhu_db_mbox_clear_irq(chan);
> > > > +       chan->con_priv = NULL;
> > > >
> > > request->free->request will fail because of this NULL assignment.
> > > Maybe add a 'taken' flag in mhu_db_channel, which should also be
> > > checked before calling mbox_chan_received_data because the data may
> > > arrive for a now relinquished channel.
> > >
> >
> > Good point, but the new 'taken' flag will have the same race as con_priv.
> > We need a lock here and can we use chan->lock or do you prefer this
> > driver maintains it own for this purpose.
> >
> I meant the con_priv is allocated in mhu_db_mbox_xlate and simply
> assigning it NULL leaks memory, if not a crash by some other path. At
> least free it before.
>

Ah right, sorry got confused. Too much reliance on devm_* apis and I didn't
realise it was not in probe but in xlate which is mbox_startup path. Fixed
now, will post v2 with both issues addressed.

--
Regards,
Sudeep