[PATCH v2 0/6] mailbox: arm_mhu: add support for doorbell mode

[PATCH v2 0/6] mailbox: arm_mhu: add support for doorbell mode

[Sudeep Holla]

Hi,

This series adds doorbell support to ARM MHU mailbox controller driver.
Since we need to callback the different client based on the doorbel bits
triggered from the remote, we can manage with single channel for the set
of 32 doorbells.

Regards,
Sudeep

v1->v2:
	- Removed the notion od subchannels
	- Treat each bit in the MHU register as a doorbell and hence
	  different channel with respect to mailbox framework

v1: https://www.spinics.net/lists/kernel/msg2500461.html

Sudeep Holla (6):
  mailbox: arm_mhu: reorder header inclusion and drop unneeded ones
  Documentation: devicetree: add bindings to support ARM MHU doorbells
  mailbox: arm_mhu: migrate to threaded irq handler
  mailbox: arm_mhu: re-factor data structure to add doorbell support
  mailbox: arm_mhu: add full support for the doorbells
  mailbox: arm_mhu: add support to read and record mbox-name

 .../devicetree/bindings/mailbox/arm-mhu.txt        |  46 ++-
 drivers/mailbox/arm_mhu.c                          | 386 ++++++++++++++++++---
 2 files changed, 383 insertions(+), 49 deletions(-)

--
2.7.4

[PATCH v2 1/6] mailbox: arm_mhu: reorder header inclusion and drop unneeded ones

[Sudeep Holla]

This patch just re-orders some of the headers includes and also drop
the ones that are unnecessary.

Cc: Alexey Klimov <alexey.klimov@arm.com>
Cc: Jassi Brar <jaswinder.singh@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/arm_mhu.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index 99befa76e37c..be0f293a9457 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -13,16 +13,13 @@
  * GNU General Public License for more details.
  */
 
-#include <linux/interrupt.h>
-#include <linux/spinlock.h>
-#include <linux/mutex.h>
-#include <linux/delay.h>
-#include <linux/slab.h>
+#include <linux/amba/bus.h>
+#include <linux/device.h>
 #include <linux/err.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
-#include <linux/module.h>
-#include <linux/amba/bus.h>
 #include <linux/mailbox_controller.h>
+#include <linux/module.h>
 
 #define INTR_STAT_OFS	0x0
 #define INTR_SET_OFS	0x8
-- 
2.7.4

[PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Sudeep Holla]

The ARM MHU has mechanism to assert interrupt signals to facilitate
inter-processor message based communication. It drives the signal using
a 32-bit register, with all 32-bits logically ORed together. It also
enables software to set, clear and check the status of each of the bits
of this register independently. Each bit of the register can be
associated with a type of event that can contribute to raising the
interrupt thereby allowing it to be used as independent doorbells.

Since the first version of this binding can't support doorbells,
this patch extends the existing binding to support them.

Cc: Alexey Klimov <alexey.klimov@arm.com>
Cc: Jassi Brar <jaswinder.singh@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 .../devicetree/bindings/mailbox/arm-mhu.txt        | 46 ++++++++++++++++++++--
 1 file changed, 43 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
index 4971f03f0b33..bd9a3a267caf 100644
--- a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
+++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
@@ -10,21 +10,42 @@ 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.
 
+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.
+
 Mailbox Device Node:
 ====================
 
 Required properties:
 --------------------
-- compatible:		Shall be "arm,mhu" & "arm,primecell"
+- compatible:		Shall be "arm,primecell" and one of the below:
+			"arm,mhu" - if the controller doesn't support
+				    doorbell model
+			"arm,mhu-doorbell" - if the controller supports
+				    doorbell model
 - reg:			Contains the mailbox register address range (base
 			address and length)
-- #mbox-cells		Shall be 1 - the index of the channel needed.
+- #mbox-cells		Shall be 1 - the index of the channel needed when
+			compatible is "arm,mhu"
+			Shall be 2 - the index of the channel needed, and
+			the index of the doorbell bit with the channel when
+			compatible is "arm,mhu-doorbell"
 - interrupts:		Contains the interrupt information corresponding to
-			each of the 3 links of MHU.
+			each of the 3 physical channels of MHU namely low
+			priority non-secure, high priority non-secure and
+			secure channels.
 
 Example:
 --------
 
+1. Controller which doesn't support doorbells
+
 	mhu: mailbox@2b1f0000 {
 		#mbox-cells = <1>;
 		compatible = "arm,mhu", "arm,primecell";
@@ -41,3 +62,22 @@ used by Linux running NS.
 		reg = <0 0x2e000000 0x4000>;
 		mboxes = <&mhu 1>; /* HP-NonSecure */
 	};
+
+2. Controller which supports doorbells
+
+	mhu: mailbox@2b1f0000 {
+		#mbox-cells = <2>;
+		compatible = "arm,mhu-doorbell", "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 = "arm,scpi";
+		reg = <0 0x2e000000 0x200>;
+		mboxes = <&mhu 1 4>; /* HP-NonSecure 5th doorbell bit */
+	};
-- 
2.7.4

[PATCH v2 3/6] mailbox: arm_mhu: migrate to threaded irq handler

[Sudeep Holla]

In preparation to introduce support for doorbells which require the
interrupt handlers to be threaded, this patch moves the existing
interrupt handler to threaded handler.

Also it moves out the registering and freeing of the handlers from
the mailbox startup and shutdown methods. This also is required to
support doorbells.

Cc: Alexey Klimov <alexey.klimov@arm.com>
Cc: Jassi Brar <jaswinder.singh@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/arm_mhu.c | 41 ++++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index be0f293a9457..ebe17c097f43 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -84,27 +84,15 @@ static int mhu_startup(struct mbox_chan *chan)
 {
 	struct mhu_link *mlink = chan->con_priv;
 	u32 val;
-	int ret;
 
 	val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
 	writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS);
 
-	ret = request_irq(mlink->irq, mhu_rx_interrupt,
-			  IRQF_SHARED, "mhu_link", chan);
-	if (ret) {
-		dev_err(chan->mbox->dev,
-			"Unable to acquire IRQ %d\n", mlink->irq);
-		return ret;
-	}
-
 	return 0;
 }
 
 static void mhu_shutdown(struct mbox_chan *chan)
 {
-	struct mhu_link *mlink = chan->con_priv;
-
-	free_irq(mlink->irq, chan);
 }
 
 static const struct mbox_chan_ops mhu_ops = {
@@ -132,13 +120,6 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 		return PTR_ERR(mhu->base);
 	}
 
-	for (i = 0; i < MHU_CHANS; i++) {
-		mhu->chan[i].con_priv = &mhu->mlink[i];
-		mhu->mlink[i].irq = adev->irq[i];
-		mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i];
-		mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + TX_REG_OFFSET;
-	}
-
 	mhu->mbox.dev = dev;
 	mhu->mbox.chans = &mhu->chan[0];
 	mhu->mbox.num_chans = MHU_CHANS;
@@ -155,6 +136,28 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 		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->chan[i].con_priv = &mhu->mlink[i];
+		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_rx_interrupt, IRQF_ONESHOT,
+						"mhu_link", &mhu->chan[i]);
+		if (err) {
+			dev_err(dev, "Can't claim IRQ %d\n", irq);
+			mbox_controller_unregister(&mhu->mbox);
+			return err;
+		}
+	}
+
 	dev_info(dev, "ARM MHU Mailbox registered\n");
 	return 0;
 }
-- 
2.7.4

[PATCH v2 4/6] mailbox: arm_mhu: re-factor data structure to add doorbell support

[Sudeep Holla]

In order to support doorbells, we need a bit of reword around data
structures that are per-channel. Since the number of doorbells are
not fixed though restricted to maximum of 20, the channel assignment
and initialization is move to xlate function.

This patch also adds the platform data for the existing support of one
channel per physical channel.

Cc: Alexey Klimov <alexey.klimov@arm.com>
Cc: Jassi Brar <jaswinder.singh@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/arm_mhu.c | 208 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 186 insertions(+), 22 deletions(-)

diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index ebe17c097f43..ae06924eb6f4 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -20,6 +20,8 @@
 #include <linux/io.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
@@ -30,7 +32,8 @@
 #define MHU_SEC_OFFSET	0x200
 #define TX_REG_OFFSET	0x100
 
-#define MHU_CHANS	3
+#define MHU_NUM_PCHANS	3	/* Secure, Non-Secure High and Low Priority */
+#define MHU_CHAN_MAX	20	/* Max channels to save on unused RAM */
 
 struct mhu_link {
 	unsigned irq;
@@ -40,53 +43,175 @@ struct mhu_link {
 
 struct arm_mhu {
 	void __iomem *base;
-	struct mhu_link mlink[MHU_CHANS];
-	struct mbox_chan chan[MHU_CHANS];
+	struct mhu_link mlink[MHU_NUM_PCHANS];
 	struct mbox_controller mbox;
+	struct device *dev;
 };
 
+/**
+ * ARM MHU Mailbox platform specific configuration
+ *
+ * @num_pchans: Maximum number of physical channels
+ * @num_doorbells: Maximum number of doorbells per physical channel
+ */
+struct mhu_mbox_pdata {
+	unsigned int num_pchans;
+	unsigned int num_doorbells;
+	bool support_doorbells;
+};
+
+/**
+ * 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_channel {
+	struct arm_mhu *mhu;
+	unsigned int pchan;
+	unsigned int doorbell;
+};
+
+static inline struct mbox_chan *
+mhu_mbox_to_channel(struct mbox_controller *mbox,
+		    unsigned int pchan, unsigned int doorbell)
+{
+	int i;
+	struct mhu_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 unsigned int mhu_mbox_irq_to_pchan_num(struct arm_mhu *mhu, int irq)
+{
+	unsigned int pchan;
+	struct mhu_mbox_pdata *pdata = dev_get_platdata(mhu->dev);
+
+	for (pchan = 0; pchan < pdata->num_pchans; pchan++)
+		if (mhu->mlink[pchan].irq == irq)
+			break;
+	return pchan;
+}
+
+static struct mbox_chan *mhu_mbox_xlate(struct mbox_controller *mbox,
+					const struct of_phandle_args *spec)
+{
+	struct arm_mhu *mhu = dev_get_drvdata(mbox->dev);
+	struct mhu_mbox_pdata *pdata = dev_get_platdata(mhu->dev);
+	struct mhu_channel *chan_info;
+	struct mbox_chan *chan = NULL;
+	unsigned int pchan = spec->args[0];
+	unsigned int doorbell = pdata->support_doorbells ? spec->args[1] : 0;
+	int i;
+
+	/* Bounds checking */
+	if (pchan >= pdata->num_pchans || doorbell >= pdata->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 irqreturn_t mhu_rx_interrupt(int irq, void *p)
 {
-	struct mbox_chan *chan = p;
-	struct mhu_link *mlink = chan->con_priv;
+	struct arm_mhu *mhu = p;
+	unsigned int pchan = mhu_mbox_irq_to_pchan_num(mhu, irq);
+	struct mbox_chan *chan = mhu_mbox_to_channel(&mhu->mbox, pchan, 0);
+	void __iomem *base = mhu->mlink[pchan].rx_reg;
 	u32 val;
 
-	val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS);
+	val = readl_relaxed(base + INTR_STAT_OFS);
 	if (!val)
 		return IRQ_NONE;
 
 	mbox_chan_received_data(chan, (void *)&val);
 
-	writel_relaxed(val, mlink->rx_reg + INTR_CLR_OFS);
+	writel_relaxed(val, base + INTR_CLR_OFS);
 
 	return IRQ_HANDLED;
 }
 
 static bool mhu_last_tx_done(struct mbox_chan *chan)
 {
-	struct mhu_link *mlink = chan->con_priv;
-	u32 val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
+	struct mhu_channel *chan_info = chan->con_priv;
+	void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].tx_reg;
+	u32 val = readl_relaxed(base + INTR_STAT_OFS);
 
 	return (val == 0);
 }
 
 static int mhu_send_data(struct mbox_chan *chan, void *data)
 {
-	struct mhu_link *mlink = chan->con_priv;
+	struct mhu_channel *chan_info = chan->con_priv;
+	void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].tx_reg;
 	u32 *arg = data;
 
-	writel_relaxed(*arg, mlink->tx_reg + INTR_SET_OFS);
+	writel_relaxed(*arg, base + INTR_SET_OFS);
 
 	return 0;
 }
 
 static int mhu_startup(struct mbox_chan *chan)
 {
-	struct mhu_link *mlink = chan->con_priv;
+	struct mhu_channel *chan_info = chan->con_priv;
+	void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].tx_reg;
 	u32 val;
 
-	val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
-	writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS);
+	val = readl_relaxed(base + INTR_STAT_OFS);
+	writel_relaxed(val, base + INTR_CLR_OFS);
 
 	return 0;
 }
@@ -102,14 +227,46 @@ static const struct mbox_chan_ops mhu_ops = {
 	.last_tx_done = mhu_last_tx_done,
 };
 
+static const struct mhu_mbox_pdata arm_mhu_pdata = {
+	.num_pchans = 3,
+	.num_doorbells = 1,
+	.support_doorbells = false,
+};
+
+static const struct of_device_id mhu_mbox_match[] = {
+	{ .compatible = "arm,mhu", .data = (void *)&arm_mhu_pdata },
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, mhu_mbox_match);
+
 static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 {
-	int i, err;
+	int i, err, max_chans;
 	struct arm_mhu *mhu;
+	struct mbox_chan *chans;
+	struct mhu_mbox_pdata *pdata;
+	const struct of_device_id *match;
 	struct device *dev = &adev->dev;
-	int mhu_reg[MHU_CHANS] = {MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET};
+	int mhu_reg[MHU_NUM_PCHANS] = {
+		MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET,
+	};
+
+	match = of_match_device(mhu_mbox_match, dev);
+	if (!match) {
+		dev_err(dev, "No configuration found\n");
+		return -ENODEV;
+	}
+	pdata = (struct mhu_mbox_pdata *)match->data;
+
+	if (pdata->num_pchans > MHU_NUM_PCHANS) {
+		dev_err(dev, "Number of physical channel can't exceed %d\n",
+			MHU_NUM_PCHANS);
+		return -EINVAL;
+	}
+
+	max_chans = pdata->support_doorbells ? MHU_CHAN_MAX : MHU_NUM_PCHANS;
 
-	/* Allocate memory for device */
 	mhu = devm_kzalloc(dev, sizeof(*mhu), GFP_KERNEL);
 	if (!mhu)
 		return -ENOMEM;
@@ -120,14 +277,22 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 		return PTR_ERR(mhu->base);
 	}
 
+	chans = devm_kcalloc(dev, max_chans, sizeof(*chans), GFP_KERNEL);
+	if (!chans)
+		return -ENOMEM;
+
+	dev->platform_data = pdata;
+
+	mhu->dev = dev;
 	mhu->mbox.dev = dev;
-	mhu->mbox.chans = &mhu->chan[0];
-	mhu->mbox.num_chans = MHU_CHANS;
+	mhu->mbox.chans = chans;
+	mhu->mbox.num_chans = max_chans;
 	mhu->mbox.ops = &mhu_ops;
 	mhu->mbox.txdone_irq = false;
 	mhu->mbox.txdone_poll = true;
 	mhu->mbox.txpoll_period = 1;
 
+	mhu->mbox.of_xlate = mhu_mbox_xlate;
 	amba_set_drvdata(adev, mhu);
 
 	err = mbox_controller_register(&mhu->mbox);
@@ -136,7 +301,7 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 		return err;
 	}
 
-	for (i = 0; i < MHU_CHANS; i++) {
+	for (i = 0; i < pdata->num_pchans; i++) {
 		int irq = mhu->mlink[i].irq = adev->irq[i];
 
 		if (irq <= 0) {
@@ -144,13 +309,12 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 			continue;
 		}
 
-		mhu->chan[i].con_priv = &mhu->mlink[i];
 		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_rx_interrupt, IRQF_ONESHOT,
-						"mhu_link", &mhu->chan[i]);
+						"mhu_link", mhu);
 		if (err) {
 			dev_err(dev, "Can't claim IRQ %d\n", irq);
 			mbox_controller_unregister(&mhu->mbox);
-- 
2.7.4

[PATCH v2 5/6] mailbox: arm_mhu: add full support for the doorbells

[Sudeep Holla]

We now have the basic infrastructure and binding to support doorbells
on ARM MHU controller.

This patch adds all the necessary mailbox operations to add support for
the doorbells. Maximum of 32 doorbells are supported on each physical
channel, however the total number of doorbells is restricted to 20
in order to save memory. It can increased if required in future.

Cc: Alexey Klimov <alexey.klimov@arm.com>
Cc: Jassi Brar <jaswinder.singh@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/arm_mhu.c | 136 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 130 insertions(+), 6 deletions(-)

diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index ae06924eb6f4..79d2392d7f3b 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -18,6 +18,7 @@
 #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>
@@ -94,6 +95,14 @@ mhu_mbox_to_channel(struct mbox_controller *mbox,
 	return NULL;
 }
 
+static void mhu_mbox_clear_irq(struct mbox_chan *chan)
+{
+	struct mhu_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_mbox_irq_to_pchan_num(struct arm_mhu *mhu, int irq)
 {
 	unsigned int pchan;
@@ -105,6 +114,95 @@ static unsigned int mhu_mbox_irq_to_pchan_num(struct arm_mhu *mhu, int irq)
 	return pchan;
 }
 
+static struct mbox_chan *mhu_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_mbox_to_channel(mbox, pchan, doorbell);
+		if (chan)
+			break;
+	}
+
+	return chan;
+}
+
+static irqreturn_t mhu_mbox_thread_handler(int irq, void *data)
+{
+	struct mbox_chan *chan;
+	struct arm_mhu *mhu = data;
+	unsigned int pchan = mhu_mbox_irq_to_pchan_num(mhu, irq);
+
+	while (NULL != (chan = mhu_mbox_irq_to_channel(mhu, pchan))) {
+		mbox_chan_received_data(chan, NULL);
+		mhu_mbox_clear_irq(chan);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static bool mhu_doorbell_last_tx_done(struct mbox_chan *chan)
+{
+	struct mhu_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_doorbell_send_data(struct mbox_chan *chan, void *data)
+{
+	struct mhu_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_doorbell_startup(struct mbox_chan *chan)
+{
+	mhu_mbox_clear_irq(chan);
+	return 0;
+}
+
+static void mhu_doorbell_shutdown(struct mbox_chan *chan)
+{
+	struct mhu_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_mbox_clear_irq(chan);
+	chan->con_priv = NULL;
+}
+
 static struct mbox_chan *mhu_mbox_xlate(struct mbox_controller *mbox,
 					const struct of_phandle_args *spec)
 {
@@ -227,15 +325,34 @@ static const struct mbox_chan_ops mhu_ops = {
 	.last_tx_done = mhu_last_tx_done,
 };
 
+static const struct mbox_chan_ops mhu_doorbell_ops = {
+	.send_data = mhu_doorbell_send_data,
+	.startup = mhu_doorbell_startup,
+	.shutdown = mhu_doorbell_shutdown,
+	.last_tx_done = mhu_doorbell_last_tx_done,
+};
+
 static const struct mhu_mbox_pdata arm_mhu_pdata = {
 	.num_pchans = 3,
 	.num_doorbells = 1,
 	.support_doorbells = false,
 };
 
+static const struct mhu_mbox_pdata arm_mhu_doorbell_pdata = {
+	.num_pchans = 2,	/* Secure can't be used */
+	.num_doorbells = 32,
+	.support_doorbells = true,
+};
+
 static const struct of_device_id mhu_mbox_match[] = {
-	{ .compatible = "arm,mhu", .data = (void *)&arm_mhu_pdata },
-	{}
+	{
+		.compatible = "arm,mhu",
+		.data = (void *)&arm_mhu_pdata
+	}, {
+		.compatible = "arm,mhu-doorbell",
+		.data = (void *)&arm_mhu_doorbell_pdata
+	}, {
+	}
 };
 
 MODULE_DEVICE_TABLE(of, mhu_mbox_match);
@@ -243,6 +360,7 @@ MODULE_DEVICE_TABLE(of, mhu_mbox_match);
 static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 {
 	int i, err, max_chans;
+	irq_handler_t handler;
 	struct arm_mhu *mhu;
 	struct mbox_chan *chans;
 	struct mhu_mbox_pdata *pdata;
@@ -287,7 +405,6 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 	mhu->mbox.dev = dev;
 	mhu->mbox.chans = chans;
 	mhu->mbox.num_chans = max_chans;
-	mhu->mbox.ops = &mhu_ops;
 	mhu->mbox.txdone_irq = false;
 	mhu->mbox.txdone_poll = true;
 	mhu->mbox.txpoll_period = 1;
@@ -295,6 +412,14 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 	mhu->mbox.of_xlate = mhu_mbox_xlate;
 	amba_set_drvdata(adev, mhu);
 
+	if (pdata->support_doorbells) {
+		mhu->mbox.ops = &mhu_doorbell_ops;
+		handler = mhu_mbox_thread_handler;
+	} else {
+		mhu->mbox.ops = &mhu_ops;
+		handler = mhu_rx_interrupt;
+	}
+
 	err = mbox_controller_register(&mhu->mbox);
 	if (err) {
 		dev_err(dev, "Failed to register mailboxes %d\n", err);
@@ -312,9 +437,8 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 		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_rx_interrupt, IRQF_ONESHOT,
-						"mhu_link", mhu);
+		err = devm_request_threaded_irq(dev, irq, NULL, handler,
+						IRQF_ONESHOT, "mhu_link", mhu);
 		if (err) {
 			dev_err(dev, "Can't claim IRQ %d\n", irq);
 			mbox_controller_unregister(&mhu->mbox);
-- 
2.7.4

[PATCH v2 6/6] mailbox: arm_mhu: add support to read and record mbox-name

[Sudeep Holla]

It's sometimes useful to identify the mailbox controller with the name
as specified in the devicetree via mbox-name property especially in a
system with multiple controllers.

This patch adds support to read and record the mailbox controller name.

Cc: Alexey Klimov <alexey.klimov@arm.com>
Cc: Jassi Brar <jaswinder.singh@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/arm_mhu.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index 79d2392d7f3b..08ed22e9e347 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -47,6 +47,7 @@ struct arm_mhu {
 	struct mhu_link mlink[MHU_NUM_PCHANS];
 	struct mbox_controller mbox;
 	struct device *dev;
+	const char *name;
 };
 
 /**
@@ -257,8 +258,8 @@ static struct mbox_chan *mhu_mbox_xlate(struct mbox_controller *mbox,
 
 	chan->con_priv = chan_info;
 
-	dev_dbg(mbox->dev, "mbox: created channel phys: %d doorbell: %d\n",
-		pchan, doorbell);
+	dev_dbg(mbox->dev, "mbox: %s, created channel phys: %d doorbell: %d\n",
+		mhu->name, pchan, doorbell);
 
 	return chan;
 }
@@ -366,6 +367,7 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 	struct mhu_mbox_pdata *pdata;
 	const struct of_device_id *match;
 	struct device *dev = &adev->dev;
+	struct device_node *np = dev->of_node;
 	int mhu_reg[MHU_NUM_PCHANS] = {
 		MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET,
 	};
@@ -395,6 +397,10 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 		return PTR_ERR(mhu->base);
 	}
 
+	err = of_property_read_string(np, "mbox-name", &mhu->name);
+	if (err)
+		mhu->name = np->full_name;
+
 	chans = devm_kcalloc(dev, max_chans, sizeof(*chans), GFP_KERNEL);
 	if (!chans)
 		return -ENOMEM;
@@ -446,7 +452,7 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 		}
 	}
 
-	dev_info(dev, "ARM MHU Mailbox registered\n");
+	dev_info(dev, "%s mailbox registered\n", mhu->name);
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH v2 0/6] mailbox: arm_mhu: add support for doorbell mode

[Jassi Brar]

On Wed, May 24, 2017 at 3:46 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Hi,
>
> This series adds doorbell support to ARM MHU mailbox controller driver.
> Since we need to callback the different client based on the doorbel bits
> triggered from the remote, we can manage with single channel for the set
> of 32 doorbells.
>
> Regards,
> Sudeep
>
> v1->v2:
>         - Removed the notion od subchannels
>         - Treat each bit in the MHU register as a doorbell and hence
>           different channel with respect to mailbox framework
>
Whatever happened to the endless explanations I gave you, how the MHU
driver already supports your usecase?

Re: [PATCH v2 0/6] mailbox: arm_mhu: add support for doorbell mode

[Sudeep Holla]

On 24/05/17 11:56, Jassi Brar wrote:
> On Wed, May 24, 2017 at 3:46 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>> Hi,
>>
>> This series adds doorbell support to ARM MHU mailbox controller driver.
>> Since we need to callback the different client based on the doorbel bits
>> triggered from the remote, we can manage with single channel for the set
>> of 32 doorbells.
>>
>> Regards,
>> Sudeep
>>
>> v1->v2:
>>         - Removed the notion od subchannels
>>         - Treat each bit in the MHU register as a doorbell and hence
>>           different channel with respect to mailbox framework
>>
> Whatever happened to the endless explanations I gave you, how the MHU
> driver already supports your usecase?
> 

Yes but you didn't respond to my queries:
1. The client driver is generic and expects it to be doorbell like
   mailbox controller. I am referring to SCMI which will be released
   soon. We can't embed ARM MHU or any other mailbox controller info
   into that.

2. How do we call multiple clients from mhu_irq ? I have Slot/bit 0
   being used by SCPI protocol(already in mainline) and slot 1/2 or more
   will be used by SCMI ?

3. We already have mailbox-sti.c which implements exactly the same logic
   of doorbell. Why did you not push back to implement something like
   arm_mhu.c then ? I am confused as why you are so particular in this
   case ?

Few more things to note here:

1. Just because the platform you worked used MHU to pass the command
   doesn't mean that's the only one use-case and others have to
   workaround in the client drivers.

2. Read the specification again. It's clear that it's designed for
   doorbell kind of usage. All I am asking is to support that both in
   the binding and implementation. And lets not assume or make it work
   with one protocol. It's generic IP and can be used in either
   doorbell way or the way it's currently supported.

3. That's one of the reason for just have 2 set's of registers as it's
   possible to use them as 32 different doorbells. Otherwise 2 channels
   is too limited for any platform.

Please address my queries instead of claiming that you can workout a
solution. I simply want the mailbox and protocol independent and hence
the binding. I don't like the idea of you proposed(i.e. 32-bit data to
be written to the controller register).

-- 
Regards,
Sudeep

Re: [PATCH v2 0/6] mailbox: arm_mhu: add support for doorbell mode

[Jassi Brar]

On Thu, May 25, 2017 at 5:00 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 24/05/17 11:56, Jassi Brar wrote:
>> On Wed, May 24, 2017 at 3:46 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>
>>> Hi,
>>>
>>> This series adds doorbell support to ARM MHU mailbox controller driver.
>>> Since we need to callback the different client based on the doorbel bits
>>> triggered from the remote, we can manage with single channel for the set
>>> of 32 doorbells.
>>>
>>> Regards,
>>> Sudeep
>>>
>>> v1->v2:
>>>         - Removed the notion od subchannels
>>>         - Treat each bit in the MHU register as a doorbell and hence
>>>           different channel with respect to mailbox framework
>>>
>> Whatever happened to the endless explanations I gave you, how the MHU
>> driver already supports your usecase?
>>
>
> Yes but you didn't respond to my queries:
> 1. The client driver is generic and expects it to be doorbell like
>    mailbox controller. I am referring to SCMI which will be released
>    soon. We can't embed ARM MHU or any other mailbox controller info
>    into that.
>
If SCMI is to be usable over different platforms, there has to be 2
sub-parts of the SCMI - one platform agnostic high level protocol
implementation, and the other platform specific 'transport' layer
where actual message xfer is done.

For the Nth time:-
    The 'mssg' in mbox_send_message(struct mbox_chan *chan, void
*mssg) is platform specific. For MHU it is simple u32*, whereas for
other platform it will be like 'struct my_protocol_message *'

I can't make it any clearer.

> 2. How do we call multiple clients from mhu_irq ? I have Slot/bit 0
>    being used by SCPI protocol(already in mainline) and slot 1/2 or more
>    will be used by SCMI ?
>
Like other platforms do, have a common client that manages messages
to/from clients working on same channel.

> 3. We already have mailbox-sti.c which implements exactly the same logic
>    of doorbell. Why did you not push back to implement something like
>    arm_mhu.c then ? I am confused as why you are so particular in this
>    case ?
>
The way STI's controller works (or as I was told), it warrants that
design. I know MHU very well and I bet it needs no modification.

 I explained in more than one way how to use the current driver, but
you refuse to acknowledge. Then I offered to modify your code for you,
but you don't agree to that either. I am running out of ways to
respond and point you back to my old posts.

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Jassi Brar]

On Wed, May 24, 2017 at 3:46 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> The ARM MHU has mechanism to assert interrupt signals to facilitate
> inter-processor message based communication. It drives the signal using
> a 32-bit register, with all 32-bits logically ORed together. It also
> enables software to set, clear and check the status of each of the bits
> of this register independently. Each bit of the register can be
> associated with a type of event that can contribute to raising the
> interrupt thereby allowing it to be used as independent doorbells.
>
> Since the first version of this binding can't support doorbells,
> this patch extends the existing binding to support them.
>
> Cc: Alexey Klimov <alexey.klimov@arm.com>
> Cc: Jassi Brar <jaswinder.singh@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  .../devicetree/bindings/mailbox/arm-mhu.txt        | 46 ++++++++++++++++++++--
>  1 file changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
> index 4971f03f0b33..bd9a3a267caf 100644
> --- a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
> +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
> @@ -10,21 +10,42 @@ 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.
>
> +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.
> +
>  Mailbox Device Node:
>  ====================
>
>  Required properties:
>  --------------------
> -- compatible:          Shall be "arm,mhu" & "arm,primecell"
> +- compatible:          Shall be "arm,primecell" and one of the below:
> +                       "arm,mhu" - if the controller doesn't support
> +                                   doorbell model
> +                       "arm,mhu-doorbell" - if the controller supports
> +                                   doorbell model
>  - reg:                 Contains the mailbox register address range (base
>                         address and length)
> -- #mbox-cells          Shall be 1 - the index of the channel needed.
> +- #mbox-cells          Shall be 1 - the index of the channel needed when
> +                       compatible is "arm,mhu"
> +                       Shall be 2 - the index of the channel needed, and
> +                       the index of the doorbell bit with the channel when
> +                       compatible is "arm,mhu-doorbell"
>  - interrupts:          Contains the interrupt information corresponding to
> -                       each of the 3 links of MHU.
> +                       each of the 3 physical channels of MHU namely low
> +                       priority non-secure, high priority non-secure and
> +                       secure channels.
>
>  Example:
>  --------
>
> +1. Controller which doesn't support doorbells
> +
>         mhu: mailbox@2b1f0000 {
>                 #mbox-cells = <1>;
>                 compatible = "arm,mhu", "arm,primecell";
> @@ -41,3 +62,22 @@ used by Linux running NS.
>                 reg = <0 0x2e000000 0x4000>;
>                 mboxes = <&mhu 1>; /* HP-NonSecure */
>         };
> +
> +2. Controller which supports doorbells
> +
> +       mhu: mailbox@2b1f0000 {
> +               #mbox-cells = <2>;
> +               compatible = "arm,mhu-doorbell", "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 = "arm,scpi";
> +               reg = <0 0x2e000000 0x200>;
> +               mboxes = <&mhu 1 4>; /* HP-NonSecure 5th doorbell bit */
> +       };
>
Every MHU controller can by driven as "arm,mhu-doorbell" or "arm,mhu"
equally fine. So you are basically smuggling a s/w feature into DT.

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Sudeep Holla]

On 25/05/17 14:22, Jassi Brar wrote:
> On Wed, May 24, 2017 at 3:46 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> The ARM MHU has mechanism to assert interrupt signals to facilitate
>> inter-processor message based communication. It drives the signal using
>> a 32-bit register, with all 32-bits logically ORed together. It also
>> enables software to set, clear and check the status of each of the bits
>> of this register independently. Each bit of the register can be
>> associated with a type of event that can contribute to raising the
>> interrupt thereby allowing it to be used as independent doorbells.
>>
>> Since the first version of this binding can't support doorbells,
>> this patch extends the existing binding to support them.
>>
>> Cc: Alexey Klimov <alexey.klimov@arm.com>
>> Cc: Jassi Brar <jaswinder.singh@linaro.org>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  .../devicetree/bindings/mailbox/arm-mhu.txt        | 46 ++++++++++++++++++++--
>>  1 file changed, 43 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>> index 4971f03f0b33..bd9a3a267caf 100644
>> --- a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>> +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>> @@ -10,21 +10,42 @@ 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.
>>
>> +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.
>> +
>>  Mailbox Device Node:
>>  ====================
>>
>>  Required properties:
>>  --------------------
>> -- compatible:          Shall be "arm,mhu" & "arm,primecell"
>> +- compatible:          Shall be "arm,primecell" and one of the below:
>> +                       "arm,mhu" - if the controller doesn't support
>> +                                   doorbell model
>> +                       "arm,mhu-doorbell" - if the controller supports
>> +                                   doorbell model
>>  - reg:                 Contains the mailbox register address range (base
>>                         address and length)
>> -- #mbox-cells          Shall be 1 - the index of the channel needed.
>> +- #mbox-cells          Shall be 1 - the index of the channel needed when
>> +                       compatible is "arm,mhu"
>> +                       Shall be 2 - the index of the channel needed, and
>> +                       the index of the doorbell bit with the channel when
>> +                       compatible is "arm,mhu-doorbell"
>>  - interrupts:          Contains the interrupt information corresponding to
>> -                       each of the 3 links of MHU.
>> +                       each of the 3 physical channels of MHU namely low
>> +                       priority non-secure, high priority non-secure and
>> +                       secure channels.
>>
>>  Example:
>>  --------
>>
>> +1. Controller which doesn't support doorbells
>> +
>>         mhu: mailbox@2b1f0000 {
>>                 #mbox-cells = <1>;
>>                 compatible = "arm,mhu", "arm,primecell";
>> @@ -41,3 +62,22 @@ used by Linux running NS.
>>                 reg = <0 0x2e000000 0x4000>;
>>                 mboxes = <&mhu 1>; /* HP-NonSecure */
>>         };
>> +
>> +2. Controller which supports doorbells
>> +
>> +       mhu: mailbox@2b1f0000 {
>> +               #mbox-cells = <2>;
>> +               compatible = "arm,mhu-doorbell", "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 = "arm,scpi";
>> +               reg = <0 0x2e000000 0x200>;
>> +               mboxes = <&mhu 1 4>; /* HP-NonSecure 5th doorbell bit */
>> +       };
>>
> Every MHU controller can by driven as "arm,mhu-doorbell" or "arm,mhu"
> equally fine. So you are basically smuggling a s/w feature into DT.
> 

I disagree, the spec clearly says each bit can be used for different
event and hence we need a way to specify that in DT when used as doorbell.

-- 
Regards,
Sudeep

Re: [PATCH v2 0/6] mailbox: arm_mhu: add support for doorbell mode

[Sudeep Holla]

On 25/05/17 14:20, Jassi Brar wrote:
> On Thu, May 25, 2017 at 5:00 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>>
>> On 24/05/17 11:56, Jassi Brar wrote:
>>> On Wed, May 24, 2017 at 3:46 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> This series adds doorbell support to ARM MHU mailbox controller driver.
>>>> Since we need to callback the different client based on the doorbel bits
>>>> triggered from the remote, we can manage with single channel for the set
>>>> of 32 doorbells.
>>>>
>>>> Regards,
>>>> Sudeep
>>>>
>>>> v1->v2:
>>>>         - Removed the notion od subchannels
>>>>         - Treat each bit in the MHU register as a doorbell and hence
>>>>           different channel with respect to mailbox framework
>>>>
>>> Whatever happened to the endless explanations I gave you, how the MHU
>>> driver already supports your usecase?
>>>
>>
>> Yes but you didn't respond to my queries:
>> 1. The client driver is generic and expects it to be doorbell like
>>    mailbox controller. I am referring to SCMI which will be released
>>    soon. We can't embed ARM MHU or any other mailbox controller info
>>    into that.
>>
> If SCMI is to be usable over different platforms, there has to be 2
> sub-parts of the SCMI - one platform agnostic high level protocol
> implementation, and the other platform specific 'transport' layer
> where actual message xfer is done.
> 

It recommends doorbell kind of interface for the transport.

> For the Nth time:-
>     The 'mssg' in mbox_send_message(struct mbox_chan *chan, void
> *mssg) is platform specific. For MHU it is simple u32*, whereas for
> other platform it will be like 'struct my_protocol_message *'
> 
> I can't make it any clearer.
> 

Why is that ? Just because it was used on your platform like that ?
Sorry that's not a valid reason. I too repeat for the nth time that the
MHU is designed to have 32 doorbells which can be used *independent* of
each other as *clearly* stated in the specification. So don't make it
platform issue. It just happened that *your* platform chose to write
some 32bit data as a whole doesn't mean that's the only use.

>> 2. How do we call multiple clients from mhu_irq ? I have Slot/bit 0
>>    being used by SCPI protocol(already in mainline) and slot 1/2 or more
>>    will be used by SCMI ?
>>
> Like other platforms do, have a common client that manages messages
> to/from clients working on same channel.
> 

Not possible as the protocols are not related. Please accept the fact
that protocol are not just platform specific. SCMI is all about generic
protocol layer that can be used on any transport.

Also each slot or bit has a different shared memory, how do you
represent that ? You are simply missing my point when I say it's proper
channel with a bit in MHU register as doorbell.

>> 3. We already have mailbox-sti.c which implements exactly the same logic
>>    of doorbell. Why did you not push back to implement something like
>>    arm_mhu.c then ? I am confused as why you are so particular in this
>>    case ?
>>
> The way STI's controller works (or as I was told), it warrants that
> design. I know MHU very well and I bet it needs no modification.
> 

It's exactly the same. I bet not better than the MHU hardware designer
I spoke to.

>  I explained in more than one way how to use the current driver, but
> you refuse to acknowledge. Then I offered to modify your code for you,
> but you don't agree to that either. I am running out of ways to
> respond and point you back to my old posts.
> 

Yes, you are simply missing the point that both ARM MHU and whatever
protocol sits on it has to be platform agnostic.

OK, you can stop pointing me back and simply propose alternate binding.
The binding needs to be changed to handle my use-case. I will then see
what need to be done.

-- 
Regards,
Sudeep

Re: [PATCH v2 0/6] mailbox: arm_mhu: add support for doorbell mode

[Jassi Brar]

On Thu, May 25, 2017 at 7:05 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>> 1. The client driver is generic and expects it to be doorbell like
>>>    mailbox controller. I am referring to SCMI which will be released
>>>    soon. We can't embed ARM MHU or any other mailbox controller info
>>>    into that.
>>>
>> If SCMI is to be usable over different platforms, there has to be 2
>> sub-parts of the SCMI - one platform agnostic high level protocol
>> implementation, and the other platform specific 'transport' layer
>> where actual message xfer is done.
>>
>
> It recommends doorbell kind of interface for the transport.
>
>> For the Nth time:-
>>     The 'mssg' in mbox_send_message(struct mbox_chan *chan, void
>> *mssg) is platform specific. For MHU it is simple u32*, whereas for
>> other platform it will be like 'struct my_protocol_message *'
>>
>> I can't make it any clearer.
>>
>
> Why is that ? Just because it was used on your platform like that ?
> Sorry that's not a valid reason.
>
To be clear, by "other platform" I mean platforms with mailbox
controller other than MHU.  (Not to mean SCMI can't run on MHU as of
today).

Do you intend SCMI to run only on platforms that have MHU controller?
I hope not.

Now re-read my last post until you get it.

Thanks.

Re: [PATCH v2 0/6] mailbox: arm_mhu: add support for doorbell mode

[Sudeep Holla]

On 25/05/17 14:44, Jassi Brar wrote:
> On Thu, May 25, 2017 at 7:05 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>> 1. The client driver is generic and expects it to be doorbell like
>>>>    mailbox controller. I am referring to SCMI which will be released
>>>>    soon. We can't embed ARM MHU or any other mailbox controller info
>>>>    into that.
>>>>
>>> If SCMI is to be usable over different platforms, there has to be 2
>>> sub-parts of the SCMI - one platform agnostic high level protocol
>>> implementation, and the other platform specific 'transport' layer
>>> where actual message xfer is done.
>>>
>>
>> It recommends doorbell kind of interface for the transport.
>>
>>> For the Nth time:-
>>>     The 'mssg' in mbox_send_message(struct mbox_chan *chan, void
>>> *mssg) is platform specific. For MHU it is simple u32*, whereas for
>>> other platform it will be like 'struct my_protocol_message *'
>>>
>>> I can't make it any clearer.
>>>
>>
>> Why is that ? Just because it was used on your platform like that ?
>> Sorry that's not a valid reason.
>>
> To be clear, by "other platform" I mean platforms with mailbox
> controller other than MHU.  (Not to mean SCMI can't run on MHU as of
> today).
> 

It can't run along with existing SCPI without some hacking. You need to
add a layer that is platform specific which is *hacky*.

> Do you intend SCMI to run only on platforms that have MHU controller?
> I hope not.
> 

Definitely not, that's the whole point. It should work with any
controller that supports doorbell mode.

> Now re-read my last post until you get it.
> 
Just propose the binding yourself.

On juno, say we will use low priority channel

BIT(0) - SCPI
BIT(1) - SCMI (general)
BIT(2) - SCMI (notification)
BIT(3) - A totally new protocol
:
:
with each of the above with specific shared memory reserved for them.

Beware we don't need any of these info in either of the driver as it
may change with another platform. So this info has to come from DT.
So I am requesting you to propose the binding if you think you can
better than the one I have.

I can base my code on that.

-- 
Regards,
Sudeep

Re: [PATCH v2 0/6] mailbox: arm_mhu: add support for doorbell mode

[Jassi Brar]

On Thu, May 25, 2017 at 7:23 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

> Just propose the binding yourself.
>
My proposed binding is what already exists. Zero change.
I offer to adapt your driver for you, if you are too busy to do it yourself.

Re: [PATCH v2 0/6] mailbox: arm_mhu: add support for doorbell mode

[Sudeep Holla]

On 25/05/17 15:07, Jassi Brar wrote:
> On Thu, May 25, 2017 at 7:23 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> 
>> Just propose the binding yourself.
>>
> My proposed binding is what already exists. Zero change.
> I offer to adapt your driver for you, if you are too busy to do it yourself.
> 

Care to explain how with the example I gave ?

BIT(0) - SCPI
BIT(1) - SCMI (general)
BIT(2) - SCMI (notification)
BIT(3) - A totally new protocol
:
:

The protocols drivers *has* to be independent as they are used with
other mailbox controllers. I can do the changes, but I would like to
know how.

-- 
Regards,
Sudeep

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Sudeep Holla]

On 25/05/17 14:22, Jassi Brar wrote:
> On Wed, May 24, 2017 at 3:46 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> The ARM MHU has mechanism to assert interrupt signals to facilitate
>> inter-processor message based communication. It drives the signal using
>> a 32-bit register, with all 32-bits logically ORed together. It also
>> enables software to set, clear and check the status of each of the bits
>> of this register independently. Each bit of the register can be
>> associated with a type of event that can contribute to raising the
>> interrupt thereby allowing it to be used as independent doorbells.
>>
>> Since the first version of this binding can't support doorbells,
>> this patch extends the existing binding to support them.
>>
>> Cc: Alexey Klimov <alexey.klimov@arm.com>
>> Cc: Jassi Brar <jaswinder.singh@linaro.org>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  .../devicetree/bindings/mailbox/arm-mhu.txt        | 46 ++++++++++++++++++++--
>>  1 file changed, 43 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>> index 4971f03f0b33..bd9a3a267caf 100644
>> --- a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>> +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>> @@ -10,21 +10,42 @@ 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.
>>
>> +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.
>> +
>>  Mailbox Device Node:
>>  ====================
>>
>>  Required properties:
>>  --------------------
>> -- compatible:          Shall be "arm,mhu" & "arm,primecell"
>> +- compatible:          Shall be "arm,primecell" and one of the below:
>> +                       "arm,mhu" - if the controller doesn't support
>> +                                   doorbell model
>> +                       "arm,mhu-doorbell" - if the controller supports
>> +                                   doorbell model
>>  - reg:                 Contains the mailbox register address range (base
>>                         address and length)
>> -- #mbox-cells          Shall be 1 - the index of the channel needed.
>> +- #mbox-cells          Shall be 1 - the index of the channel needed when
>> +                       compatible is "arm,mhu"
>> +                       Shall be 2 - the index of the channel needed, and
>> +                       the index of the doorbell bit with the channel when
>> +                       compatible is "arm,mhu-doorbell"
>>  - interrupts:          Contains the interrupt information corresponding to
>> -                       each of the 3 links of MHU.
>> +                       each of the 3 physical channels of MHU namely low
>> +                       priority non-secure, high priority non-secure and
>> +                       secure channels.
>>
>>  Example:
>>  --------
>>
>> +1. Controller which doesn't support doorbells
>> +
>>         mhu: mailbox@2b1f0000 {
>>                 #mbox-cells = <1>;
>>                 compatible = "arm,mhu", "arm,primecell";
>> @@ -41,3 +62,22 @@ used by Linux running NS.
>>                 reg = <0 0x2e000000 0x4000>;
>>                 mboxes = <&mhu 1>; /* HP-NonSecure */
>>         };
>> +
>> +2. Controller which supports doorbells
>> +
>> +       mhu: mailbox@2b1f0000 {
>> +               #mbox-cells = <2>;
>> +               compatible = "arm,mhu-doorbell", "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 = "arm,scpi";
>> +               reg = <0 0x2e000000 0x200>;
>> +               mboxes = <&mhu 1 4>; /* HP-NonSecure 5th doorbell bit */
>> +       };
>>
> Every MHU controller can by driven as "arm,mhu-doorbell" or "arm,mhu"
> equally fine. So you are basically smuggling a s/w feature into DT.
> 

I have asked many times now, please explain how would you handle
multiple protocol on a single set of MHU registers.

BIT(0) - SCPI protocol
BIT(1) - SCMI (general)
BIT(2) - SCMI (notification)
BIT(3) - A totally new protocol
:
:
with each of the above with specific shared memory reserved for them.

I know you suggested to pass the values from DT, but that sounds
much horrible than sanely describing the bits as individual doorbells.
And also have that value as part of any protocol also makes no sense
as it's not clearly part of that protocol spec.

Further, we need another layer of indirection. All these protocol
will mbox_request_channel to start with as they need to be generic.
How do you propose to mux multiple requests of these into arm_mhu ?

Rob,

Do you have any objections to specify the doorbell bits in the mailbox
controller as opposed to client sending that value as some magic value ?

-- 
Regards,
Sudeep

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Rob Herring]

On Thu, May 25, 2017 at 02:23:44PM +0100, Sudeep Holla wrote:
> 
> 
> On 25/05/17 14:22, Jassi Brar wrote:
> > On Wed, May 24, 2017 at 3:46 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >> The ARM MHU has mechanism to assert interrupt signals to facilitate
> >> inter-processor message based communication. It drives the signal using
> >> a 32-bit register, with all 32-bits logically ORed together. It also
> >> enables software to set, clear and check the status of each of the bits
> >> of this register independently. Each bit of the register can be
> >> associated with a type of event that can contribute to raising the
> >> interrupt thereby allowing it to be used as independent doorbells.
> >>
> >> Since the first version of this binding can't support doorbells,
> >> this patch extends the existing binding to support them.
> >>
> >> Cc: Alexey Klimov <alexey.klimov@arm.com>
> >> Cc: Jassi Brar <jaswinder.singh@linaro.org>
> >> Cc: Rob Herring <robh+dt@kernel.org>
> >> Cc: devicetree@vger.kernel.org
> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >> ---
> >>  .../devicetree/bindings/mailbox/arm-mhu.txt        | 46 ++++++++++++++++++++--
> >>  1 file changed, 43 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
> >> index 4971f03f0b33..bd9a3a267caf 100644
> >> --- a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
> >> +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
> >> @@ -10,21 +10,42 @@ 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.
> >>
> >> +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.
> >> +
> >>  Mailbox Device Node:
> >>  ====================
> >>
> >>  Required properties:
> >>  --------------------
> >> -- compatible:          Shall be "arm,mhu" & "arm,primecell"
> >> +- compatible:          Shall be "arm,primecell" and one of the below:
> >> +                       "arm,mhu" - if the controller doesn't support
> >> +                                   doorbell model
> >> +                       "arm,mhu-doorbell" - if the controller supports
> >> +                                   doorbell model
> >>  - reg:                 Contains the mailbox register address range (base
> >>                         address and length)
> >> -- #mbox-cells          Shall be 1 - the index of the channel needed.
> >> +- #mbox-cells          Shall be 1 - the index of the channel needed when
> >> +                       compatible is "arm,mhu"
> >> +                       Shall be 2 - the index of the channel needed, and
> >> +                       the index of the doorbell bit with the channel when
> >> +                       compatible is "arm,mhu-doorbell"
> >>  - interrupts:          Contains the interrupt information corresponding to
> >> -                       each of the 3 links of MHU.
> >> +                       each of the 3 physical channels of MHU namely low
> >> +                       priority non-secure, high priority non-secure and
> >> +                       secure channels.
> >>
> >>  Example:
> >>  --------
> >>
> >> +1. Controller which doesn't support doorbells
> >> +
> >>         mhu: mailbox@2b1f0000 {
> >>                 #mbox-cells = <1>;
> >>                 compatible = "arm,mhu", "arm,primecell";
> >> @@ -41,3 +62,22 @@ used by Linux running NS.
> >>                 reg = <0 0x2e000000 0x4000>;
> >>                 mboxes = <&mhu 1>; /* HP-NonSecure */
> >>         };
> >> +
> >> +2. Controller which supports doorbells
> >> +
> >> +       mhu: mailbox@2b1f0000 {
> >> +               #mbox-cells = <2>;
> >> +               compatible = "arm,mhu-doorbell", "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 = "arm,scpi";
> >> +               reg = <0 0x2e000000 0x200>;
> >> +               mboxes = <&mhu 1 4>; /* HP-NonSecure 5th doorbell bit */
> >> +       };
> >>
> > Every MHU controller can by driven as "arm,mhu-doorbell" or "arm,mhu"
> > equally fine. So you are basically smuggling a s/w feature into DT.
> > 
> 
> I disagree, the spec clearly says each bit can be used for different
> event and hence we need a way to specify that in DT when used as doorbell.

I think the point is that you should not continue to use both. The 
single cell usage should be deprecated. Maybe you'll have to encode the 
2nd cell when not used as 0 means bit 0?

Arguably, you don't even need a new compatible. #mbox-cells tells you 
whether to use the old or new binding. I'm fine either way though.

Rob

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Sudeep Holla]

On 31/05/17 18:08, Rob Herring wrote:
> On Thu, May 25, 2017 at 02:23:44PM +0100, Sudeep Holla wrote:
>>
>>
>> On 25/05/17 14:22, Jassi Brar wrote:

[...]

>>> Every MHU controller can by driven as "arm,mhu-doorbell" or "arm,mhu"
>>> equally fine. So you are basically smuggling a s/w feature into DT.
>>>
>>
>> I disagree, the spec clearly says each bit can be used for different
>> event and hence we need a way to specify that in DT when used as doorbell.
> 
> I think the point is that you should not continue to use both. The 
> single cell usage should be deprecated. Maybe you'll have to encode the 
> 2nd cell when not used as 0 means bit 0?
> 

Instead of having special encoding, I like your below suggestion on
using #mbox-cells to distinguish the usage modes.

> Arguably, you don't even need a new compatible. #mbox-cells tells you 
> whether to use the old or new binding. I'm fine either way though.
> 

Ah good point, yes we can distinguish with #mbox-cells. I will drop the
new compatible.

-- 
Regards,
Sudeep

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Jassi Brar]

Hi Rob,

On Wed, May 31, 2017 at 10:38 PM, Rob Herring <robh@kernel.org> wrote:
> On Thu, May 25, 2017 at 02:23:44PM +0100, Sudeep Holla wrote:
>>
>> >>  .../devicetree/bindings/mailbox/arm-mhu.txt        | 46 ++++++++++++++++++++--
>> >>  1 file changed, 43 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>> >> index 4971f03f0b33..bd9a3a267caf 100644
>> >> --- a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>> >> +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>> >> @@ -10,21 +10,42 @@ 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.
>> >>
>> >> +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.
>> >> +
>> >>  Mailbox Device Node:
>> >>  ====================
>> >>
>> >>  Required properties:
>> >>  --------------------
>> >> -- compatible:          Shall be "arm,mhu" & "arm,primecell"
>> >> +- compatible:          Shall be "arm,primecell" and one of the below:
>> >> +                       "arm,mhu" - if the controller doesn't support
>> >> +                                   doorbell model
>> >> +                       "arm,mhu-doorbell" - if the controller supports
>> >> +                                   doorbell model
>> >>  - reg:                 Contains the mailbox register address range (base
>> >>                         address and length)
>> >> -- #mbox-cells          Shall be 1 - the index of the channel needed.
>> >> +- #mbox-cells          Shall be 1 - the index of the channel needed when
>> >> +                       compatible is "arm,mhu"
>> >> +                       Shall be 2 - the index of the channel needed, and
>> >> +                       the index of the doorbell bit with the channel when
>> >> +                       compatible is "arm,mhu-doorbell"
>> >>  - interrupts:          Contains the interrupt information corresponding to
>> >> -                       each of the 3 links of MHU.
>> >> +                       each of the 3 physical channels of MHU namely low
>> >> +                       priority non-secure, high priority non-secure and
>> >> +                       secure channels.
>> >>
>> >>  Example:
>> >>  --------
>> >>
>> >> +1. Controller which doesn't support doorbells
>> >> +
>> >>         mhu: mailbox@2b1f0000 {
>> >>                 #mbox-cells = <1>;
>> >>                 compatible = "arm,mhu", "arm,primecell";
>> >> @@ -41,3 +62,22 @@ used by Linux running NS.
>> >>                 reg = <0 0x2e000000 0x4000>;
>> >>                 mboxes = <&mhu 1>; /* HP-NonSecure */
>> >>         };
>> >> +
>> >> +2. Controller which supports doorbells
>> >> +
>> >> +       mhu: mailbox@2b1f0000 {
>> >> +               #mbox-cells = <2>;
>> >> +               compatible = "arm,mhu-doorbell", "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 = "arm,scpi";
>> >> +               reg = <0 0x2e000000 0x200>;
>> >> +               mboxes = <&mhu 1 4>; /* HP-NonSecure 5th doorbell bit */
>> >> +       };
>> >>
>> > Every MHU controller can by driven as "arm,mhu-doorbell" or "arm,mhu"
>> > equally fine. So you are basically smuggling a s/w feature into DT.
>> >
>>
>> I disagree, the spec clearly says each bit can be used for different
>> event and hence we need a way to specify that in DT when used as doorbell.
>
> I think the point is that you should not continue to use both. The
>
FYKI my point (here and in other threads) is that current MHU
driver/bindings can very well support Sudeep's usecase.

> single cell usage should be deprecated. Maybe you'll have to encode the
> 2nd cell when not used as 0 means bit 0?
>
> Arguably, you don't even need a new compatible. #mbox-cells tells you
> whether to use the old or new binding. I'm fine either way though.
>
In mailbox terminology, there is a channel and command(s) that we
send/recv over it. OOB data passing is optional.

MHU driver accepts a command as a u32. Sudeep's usecase has commands
of the form of BIT(x)  ... an instance of u32.

Instead of having his client driver pass BIT(x) like other MHU
clients, he wants to modify the driver and bindings to specify 'x' via
second cell of mboxes.  That would have made sense (and already
implemented) if the controller could send only 1 bit at a time and
every client/protocol used exactly 1 command - both of which are not
true.

I even offered Sudeep to share his client driver and I'll adapt it for
him, but her refuses to either see my point or share his code.

Cheers!

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Sudeep Holla]

On 02/06/17 06:45, Jassi Brar wrote:
> Hi Rob,
> 
> On Wed, May 31, 2017 at 10:38 PM, Rob Herring <robh@kernel.org> wrote:
>> On Thu, May 25, 2017 at 02:23:44PM +0100, Sudeep Holla wrote:
>>>
>>>>>  .../devicetree/bindings/mailbox/arm-mhu.txt        | 46 ++++++++++++++++++++--
>>>>>  1 file changed, 43 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>>>>> index 4971f03f0b33..bd9a3a267caf 100644
>>>>> --- a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>>>>> +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>>>>> @@ -10,21 +10,42 @@ 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.
>>>>>
>>>>> +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.
>>>>> +
>>>>>  Mailbox Device Node:
>>>>>  ====================
>>>>>
>>>>>  Required properties:
>>>>>  --------------------
>>>>> -- compatible:          Shall be "arm,mhu" & "arm,primecell"
>>>>> +- compatible:          Shall be "arm,primecell" and one of the below:
>>>>> +                       "arm,mhu" - if the controller doesn't support
>>>>> +                                   doorbell model
>>>>> +                       "arm,mhu-doorbell" - if the controller supports
>>>>> +                                   doorbell model
>>>>>  - reg:                 Contains the mailbox register address range (base
>>>>>                         address and length)
>>>>> -- #mbox-cells          Shall be 1 - the index of the channel needed.
>>>>> +- #mbox-cells          Shall be 1 - the index of the channel needed when
>>>>> +                       compatible is "arm,mhu"
>>>>> +                       Shall be 2 - the index of the channel needed, and
>>>>> +                       the index of the doorbell bit with the channel when
>>>>> +                       compatible is "arm,mhu-doorbell"
>>>>>  - interrupts:          Contains the interrupt information corresponding to
>>>>> -                       each of the 3 links of MHU.
>>>>> +                       each of the 3 physical channels of MHU namely low
>>>>> +                       priority non-secure, high priority non-secure and
>>>>> +                       secure channels.
>>>>>
>>>>>  Example:
>>>>>  --------
>>>>>
>>>>> +1. Controller which doesn't support doorbells
>>>>> +
>>>>>         mhu: mailbox@2b1f0000 {
>>>>>                 #mbox-cells = <1>;
>>>>>                 compatible = "arm,mhu", "arm,primecell";
>>>>> @@ -41,3 +62,22 @@ used by Linux running NS.
>>>>>                 reg = <0 0x2e000000 0x4000>;
>>>>>                 mboxes = <&mhu 1>; /* HP-NonSecure */
>>>>>         };
>>>>> +
>>>>> +2. Controller which supports doorbells
>>>>> +
>>>>> +       mhu: mailbox@2b1f0000 {
>>>>> +               #mbox-cells = <2>;
>>>>> +               compatible = "arm,mhu-doorbell", "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 = "arm,scpi";
>>>>> +               reg = <0 0x2e000000 0x200>;
>>>>> +               mboxes = <&mhu 1 4>; /* HP-NonSecure 5th doorbell bit */
>>>>> +       };
>>>>>
>>>> Every MHU controller can by driven as "arm,mhu-doorbell" or "arm,mhu"
>>>> equally fine. So you are basically smuggling a s/w feature into DT.
>>>>
>>>
>>> I disagree, the spec clearly says each bit can be used for different
>>> event and hence we need a way to specify that in DT when used as doorbell.
>>
>> I think the point is that you should not continue to use both. The
>>
> FYKI my point (here and in other threads) is that current MHU
> driver/bindings can very well support Sudeep's usecase.
> 
>> single cell usage should be deprecated. Maybe you'll have to encode the
>> 2nd cell when not used as 0 means bit 0?
>>
>> Arguably, you don't even need a new compatible. #mbox-cells tells you
>> whether to use the old or new binding. I'm fine either way though.
>>
> In mailbox terminology, there is a channel and command(s) that we
> send/recv over it. OOB data passing is optional.
> 
> MHU driver accepts a command as a u32. Sudeep's usecase has commands
> of the form of BIT(x)  ... an instance of u32.
> 

Again read the spec, it is designed to be used as doorbell

> Instead of having his client driver pass BIT(x) like other MHU

Client is generic and doesn't understand which controller it's working
with. It's not custom protocol. I have given enough links and details to
say it's generic protocol adapted by other vendors.

> clients, he wants to modify the driver and bindings to specify 'x' via
> second cell of mboxes.  That would have made sense (and already
> implemented) if the controller could send only 1 bit at a time and
> every client/protocol used exactly 1 command - both of which are not
> true.

Yes but the controller is designed to provide single bit doorbell
capability. Please understand that instead of pushing how you think it
needs to work.

> 
> I even offered Sudeep to share his client driver and I'll adapt it for
> him, but her refuses to either see my point or share his code.
> 

You have not answered my queries, you keep telling I can help but never
read and answer what I ask. Also I have told you the driver is in
mainline(drivers/firmware/arm_scpi.c) and there will be another similar
one. And theres are used by different platforms with different mailbox
controllers.
Please post patches to make 2 different protocols like SCPI work. Please
note these protocols are used by other platforms which have different
controllers.

-- 
Regards,
Sudeep

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Sudeep Holla]

On 02/06/17 10:32, Sudeep Holla wrote:
> 
> 
> On 02/06/17 06:45, Jassi Brar wrote:
>> Hi Rob,
>>
>> On Wed, May 31, 2017 at 10:38 PM, Rob Herring <robh@kernel.org> wrote:
>>> On Thu, May 25, 2017 at 02:23:44PM +0100, Sudeep Holla wrote:
>>>>
>>>>>>  .../devicetree/bindings/mailbox/arm-mhu.txt        | 46 ++++++++++++++++++++--
>>>>>>  1 file changed, 43 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>>>>>> index 4971f03f0b33..bd9a3a267caf 100644
>>>>>> --- a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>>>>>> +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>>>>>> @@ -10,21 +10,42 @@ 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.
>>>>>>
>>>>>> +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.
>>>>>> +
>>>>>>  Mailbox Device Node:
>>>>>>  ====================
>>>>>>
>>>>>>  Required properties:
>>>>>>  --------------------
>>>>>> -- compatible:          Shall be "arm,mhu" & "arm,primecell"
>>>>>> +- compatible:          Shall be "arm,primecell" and one of the below:
>>>>>> +                       "arm,mhu" - if the controller doesn't support
>>>>>> +                                   doorbell model
>>>>>> +                       "arm,mhu-doorbell" - if the controller supports
>>>>>> +                                   doorbell model
>>>>>>  - reg:                 Contains the mailbox register address range (base
>>>>>>                         address and length)
>>>>>> -- #mbox-cells          Shall be 1 - the index of the channel needed.
>>>>>> +- #mbox-cells          Shall be 1 - the index of the channel needed when
>>>>>> +                       compatible is "arm,mhu"
>>>>>> +                       Shall be 2 - the index of the channel needed, and
>>>>>> +                       the index of the doorbell bit with the channel when
>>>>>> +                       compatible is "arm,mhu-doorbell"
>>>>>>  - interrupts:          Contains the interrupt information corresponding to
>>>>>> -                       each of the 3 links of MHU.
>>>>>> +                       each of the 3 physical channels of MHU namely low
>>>>>> +                       priority non-secure, high priority non-secure and
>>>>>> +                       secure channels.
>>>>>>
>>>>>>  Example:
>>>>>>  --------
>>>>>>
>>>>>> +1. Controller which doesn't support doorbells
>>>>>> +
>>>>>>         mhu: mailbox@2b1f0000 {
>>>>>>                 #mbox-cells = <1>;
>>>>>>                 compatible = "arm,mhu", "arm,primecell";
>>>>>> @@ -41,3 +62,22 @@ used by Linux running NS.
>>>>>>                 reg = <0 0x2e000000 0x4000>;
>>>>>>                 mboxes = <&mhu 1>; /* HP-NonSecure */
>>>>>>         };
>>>>>> +
>>>>>> +2. Controller which supports doorbells
>>>>>> +
>>>>>> +       mhu: mailbox@2b1f0000 {
>>>>>> +               #mbox-cells = <2>;
>>>>>> +               compatible = "arm,mhu-doorbell", "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 = "arm,scpi";
>>>>>> +               reg = <0 0x2e000000 0x200>;
>>>>>> +               mboxes = <&mhu 1 4>; /* HP-NonSecure 5th doorbell bit */
>>>>>> +       };
>>>>>>
>>>>> Every MHU controller can by driven as "arm,mhu-doorbell" or "arm,mhu"
>>>>> equally fine. So you are basically smuggling a s/w feature into DT.
>>>>>
>>>>
>>>> I disagree, the spec clearly says each bit can be used for different
>>>> event and hence we need a way to specify that in DT when used as doorbell.
>>>
>>> I think the point is that you should not continue to use both. The
>>>
>> FYKI my point (here and in other threads) is that current MHU
>> driver/bindings can very well support Sudeep's usecase.
>>
>>> single cell usage should be deprecated. Maybe you'll have to encode the
>>> 2nd cell when not used as 0 means bit 0?
>>>
>>> Arguably, you don't even need a new compatible. #mbox-cells tells you
>>> whether to use the old or new binding. I'm fine either way though.
>>>
>> In mailbox terminology, there is a channel and command(s) that we
>> send/recv over it. OOB data passing is optional.
>>
>> MHU driver accepts a command as a u32. Sudeep's usecase has commands
>> of the form of BIT(x)  ... an instance of u32.
>>
> 
> Again read the spec, it is designed to be used as doorbell
> 
>> Instead of having his client driver pass BIT(x) like other MHU
> 
> Client is generic and doesn't understand which controller it's working
> with. It's not custom protocol. I have given enough links and details to
> say it's generic protocol adapted by other vendors.
> 
>> clients, he wants to modify the driver and bindings to specify 'x' via
>> second cell of mboxes.  That would have made sense (and already
>> implemented) if the controller could send only 1 bit at a time and
>> every client/protocol used exactly 1 command - both of which are not
>> true.
> 
> Yes but the controller is designed to provide single bit doorbell
> capability. Please understand that instead of pushing how you think it
> needs to work.
> 
>>
>> I even offered Sudeep to share his client driver and I'll adapt it for
>> him, but her refuses to either see my point or share his code.
>>
> 
> You have not answered my queries, you keep telling I can help but never
> read and answer what I ask. Also I have told you the driver is in
> mainline(drivers/firmware/arm_scpi.c) and there will be another similar
> one. And theres are used by different platforms with different mailbox
> controllers.
> Please post patches to make 2 different protocols like SCPI work. Please
> note these protocols are used by other platforms which have different
> controllers.
> 

I have posted the SCMI patches now[1], please let me know how to get
both SCPI and SCMI working together with different doorbell bits on the
same channel.

-- 
Regards,
Sudeep

[1] http://marc.info/?l=devicetree&m=149849482623492&w=2

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Jassi Brar]

On Wed, Jul 5, 2017 at 11:32 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

>
> I have posted the SCMI patches now[1],
>
I wish I was CC'ed on that. Now LKML seems too busy to forward it.

> please let me know how to get
> both SCPI and SCMI working together with different doorbell bits on the
> same channel.
>
You say in the cover letter :
"Let me begin admitting that we are introducing yet another protocol to
achieve same things as many existing protocols like ARM SCPI, TI SCI,
QCOM RPM, Nvidia Tegra BPMP, and so on"

 So SCMI is supposed to replace SCPI, SCI, RPM and BPMP   or   SCMI is
to be used for future platforms.
If SCPI and SCMI achieve the same, why have them both active simultaneously?

Assuming there really is some sane excuse :-
    SCPI and SCMI are two separate client working over an MHU channel.
So, as is the case with users relying on a common resource, you need a
shim arbitrator that serialises access to the resource.

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Sudeep Holla]

Hi Jassi,

On 06/07/17 07:28, Jassi Brar wrote:
> On Wed, Jul 5, 2017 at 11:32 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> 
>>
>> I have posted the SCMI patches now[1],
>>
> I wish I was CC'ed on that. Now LKML seems too busy to forward it.
> 

Yes, my mistake, I should have cc-ed you.

>> please let me know how to get
>> both SCPI and SCMI working together with different doorbell bits on the
>> same channel.
>>
> You say in the cover letter :
> "Let me begin admitting that we are introducing yet another protocol to
> achieve same things as many existing protocols like ARM SCPI, TI SCI,
> QCOM RPM, Nvidia Tegra BPMP, and so on"
> 
>  So SCMI is supposed to replace SCPI, SCI, RPM and BPMP   or   SCMI is
> to be used for future platforms.
> If SCPI and SCMI achieve the same, why have them both active simultaneously?
> 

Yes it may not be used, but the firmware might support both for backward
compatibility. E.g. on Juno, we still may continue supporting SCPI while
we transition to SCMI. So both old and new DTs must work.

> Assuming there really is some sane excuse :-

Yes as I mentioned above.

>     SCPI and SCMI are two separate client working over an MHU channel.
> So, as is the case with users relying on a common resource, you need a
> shim arbitrator that serialises access to the resource.>

Yes, that's what I have done with this series of patches retaining the
backward compatibility. I have made it generic to ARM MHU instead of
specifically addressing couple of protocols on a particular platform.

If I try to come up with an additional shim driver, I bet it will become
much ugly, bigger and platform specific.

The specification of MHU mentions on how to use it as doorbell bit and I
am doing that to provide cleaner and generic solution which I you seem
to disagree with. So please suggest/provide alternative solution.

A shim layer may get complicated as both SCMI and SCPI will call
standard mailbox APIs. Do you mean I need to replace those calls with
shim layer API ? If I do that it may break on other platforms which are
using SCPI.

-- 
Regards,
Sudeep

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Jassi Brar]

On Thu, Jul 6, 2017 at 2:48 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> Hi Jassi,
>
> On 06/07/17 07:28, Jassi Brar wrote:
>> On Wed, Jul 5, 2017 at 11:32 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>>>
>>> I have posted the SCMI patches now[1],
>>>
>> I wish I was CC'ed on that. Now LKML seems too busy to forward it.
>>
>
> Yes, my mistake, I should have cc-ed you.
>
>>> please let me know how to get
>>> both SCPI and SCMI working together with different doorbell bits on the
>>> same channel.
>>>
>> You say in the cover letter :
>> "Let me begin admitting that we are introducing yet another protocol to
>> achieve same things as many existing protocols like ARM SCPI, TI SCI,
>> QCOM RPM, Nvidia Tegra BPMP, and so on"
>>
>>  So SCMI is supposed to replace SCPI, SCI, RPM and BPMP   or   SCMI is
>> to be used for future platforms.
>> If SCPI and SCMI achieve the same, why have them both active simultaneously?
>>
>
> Yes it may not be used, but the firmware might support both for backward
> compatibility. E.g. on Juno, we still may continue supporting SCPI while
> we transition to SCMI. So both old and new DTs must work.
>
Sure, but still there is no reason to have both SCMI and SCPI active
during _runtime_.
Either SCMI or SCPI should be populated by DT, not both.

>> Assuming there really is some sane excuse :-
>
> Yes as I mentioned above.
>
If you specify only one of SCPI/SCMI, you wouldn't need the shim arbitrator.

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Sudeep Holla]

On 06/07/17 10:27, Jassi Brar wrote:
> On Thu, Jul 6, 2017 at 2:48 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> Hi Jassi,
>>
>> On 06/07/17 07:28, Jassi Brar wrote:
>>> On Wed, Jul 5, 2017 at 11:32 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>
>>>>
>>>> I have posted the SCMI patches now[1],
>>>>
>>> I wish I was CC'ed on that. Now LKML seems too busy to forward it.
>>>
>>
>> Yes, my mistake, I should have cc-ed you.
>>
>>>> please let me know how to get
>>>> both SCPI and SCMI working together with different doorbell bits on the
>>>> same channel.
>>>>
>>> You say in the cover letter :
>>> "Let me begin admitting that we are introducing yet another protocol to
>>> achieve same things as many existing protocols like ARM SCPI, TI SCI,
>>> QCOM RPM, Nvidia Tegra BPMP, and so on"
>>>
>>>  So SCMI is supposed to replace SCPI, SCI, RPM and BPMP   or   SCMI is
>>> to be used for future platforms.
>>> If SCPI and SCMI achieve the same, why have them both active simultaneously?
>>>
>>
>> Yes it may not be used, but the firmware might support both for backward
>> compatibility. E.g. on Juno, we still may continue supporting SCPI while
>> we transition to SCMI. So both old and new DTs must work.
>>
> Sure, but still there is no reason to have both SCMI and SCPI active
> during _runtime_.
> Either SCMI or SCPI should be populated by DT, not both.
> 
>>> Assuming there really is some sane excuse :-
>>
>> Yes as I mentioned above.
>>
> If you specify only one of SCPI/SCMI, you wouldn't need the shim arbitrator.
> 

I said it *may not be used*, currently it is used. Also can you please
answer other questions I had on mailbox API and not breaking SCPI
exiting users ? I need your suggestion to proceed on that. Similar to
SCPI, SCMI will be used by other platforms which must have doorbell
mailbox mechanism.

-- 
Regards,
Sudeep

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Jassi Brar]

On Thu, Jul 6, 2017 at 3:03 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 06/07/17 10:27, Jassi Brar wrote:
>> On Thu, Jul 6, 2017 at 2:48 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>> Hi Jassi,
>>>
>>> On 06/07/17 07:28, Jassi Brar wrote:
>>>> On Wed, Jul 5, 2017 at 11:32 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>>
>>>>>
>>>>> I have posted the SCMI patches now[1],
>>>>>
>>>> I wish I was CC'ed on that. Now LKML seems too busy to forward it.
>>>>
>>>
>>> Yes, my mistake, I should have cc-ed you.
>>>
>>>>> please let me know how to get
>>>>> both SCPI and SCMI working together with different doorbell bits on the
>>>>> same channel.
>>>>>
>>>> You say in the cover letter :
>>>> "Let me begin admitting that we are introducing yet another protocol to
>>>> achieve same things as many existing protocols like ARM SCPI, TI SCI,
>>>> QCOM RPM, Nvidia Tegra BPMP, and so on"
>>>>
>>>>  So SCMI is supposed to replace SCPI, SCI, RPM and BPMP   or   SCMI is
>>>> to be used for future platforms.
>>>> If SCPI and SCMI achieve the same, why have them both active simultaneously?
>>>>
>>>
>>> Yes it may not be used, but the firmware might support both for backward
>>> compatibility. E.g. on Juno, we still may continue supporting SCPI while
>>> we transition to SCMI. So both old and new DTs must work.
>>>
>> Sure, but still there is no reason to have both SCMI and SCPI active
>> during _runtime_.
>> Either SCMI or SCPI should be populated by DT, not both.
>>
>>>> Assuming there really is some sane excuse :-
>>>
>>> Yes as I mentioned above.
>>>
>> If you specify only one of SCPI/SCMI, you wouldn't need the shim arbitrator.
>>
>
> I said it *may not be used*, currently it is used.
>
SCPI provides more than what SCMI currently does - dvfs, clock, sensor.
I see no reason why you must have SCPI and SCMI both running.

And even then there is a solution - a shim arbitrator. Other
platforms, those share a channel, do that. No big deal.

 BTW, I hope you realise that we need a 'transport layer' which will
be the platform specific glue between mailbox controller specifics and
the generic SCMI code.
I see your confusion in the form of some issues in the SCMI
implementation, please CC me on the next revision.

Thanks.

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Sudeep Holla]

On 06/07/17 15:37, Jassi Brar wrote:
> On Thu, Jul 6, 2017 at 3:03 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

[...]

>>
>> I said it *may not be used*, currently it is used.
>>
> SCPI provides more than what SCMI currently does - dvfs, clock, sensor.

Not sure what you mean by that, but that's not true.

> I see no reason why you must have SCPI and SCMI both running.
> 

We can still have 2 different protocols using same MHU channel with
different doorbells, what's wrong with that ?

> And even then there is a solution - a shim arbitrator. Other
> platforms, those share a channel, do that. No big deal.
> 

Example please ? Please remember these protocols are generic and we
can't add any platform specific code into them.

>  BTW, I hope you realise that we need a 'transport layer' which will
> be the platform specific glue between mailbox controller specifics and
> the generic SCMI code.

Why ? Clearly you have not made a since technical argument so far as why
MHU doorbell is not correct way even when MHU specification is clearly
allows it. I have given example of ST mailbox which has this doorbell
kind of support.

> I see your confusion in the form of some issues in the SCMI
> implementation, please CC me on the next revision.
> 

Care to elaborate on what's my confusion or at-least what you think so ?

Also if you have concern on implementation, ok we can discuss further.
But can you make it clear as what your objections are for the doorbell
MHU binding. How will I get the bit assigned for different protocols
which are platform specific ? I still need some binding , right ?
-- 
Regards,
Sudeep

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Jassi Brar]

On Thu, Jul 6, 2017 at 10:14 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On 06/07/17 15:37, Jassi Brar wrote:
>> On Thu, Jul 6, 2017 at 3:03 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>

>> I see no reason why you must have SCPI and SCMI both running.
>>
>
> We can still have 2 different protocols using same MHU channel with
> different doorbells, what's wrong with that ?
>
Only for SCMI and SCPI - both written by same person to achieve the
same thing - I think it should not be necessary.

But yes, SCMI running alongside some complementary protocol (that
implements what SCMI doesn't provide) is a very solid usecase. And
that is the reason you need a platform specific 'transport layer'.

>> And even then there is a solution - a shim arbitrator. Other
>> platforms, those share a channel, do that. No big deal.
>>
>
> Example please? Please remember these protocols are generic and we
> can't add any platform specific code into them.
>
You do need to have platform specific glue as I said.

>>  BTW, I hope you realise that we need a 'transport layer' which will
>> be the platform specific glue between mailbox controller specifics and
>> the generic SCMI code.
>
> Why ?
>
Because you should not restrict the usage of SCMI to only platforms
with mailbox controller that does not require any information to
trigger a signal on the other side -- what you call "doorbell".

Why shouldn't Rockchip be able to implement SCMI? Just because it uses
different mechanism to signal the other end?

You are actually putting in effort to restrict the usage of SCMI. Its
like you buy a usb device and the first thing you do is solder it to
the plug.


> Clearly you have not made a since technical argument so far as why
> MHU doorbell is not correct way even when MHU specification is clearly
> allows it. I have given example of ST mailbox which has this doorbell
> kind of support.
>
>> I see your confusion in the form of some issues in the SCMI
>> implementation, please CC me on the next revision.
>>
>
> Care to elaborate on what's my confusion or at-least what you think so ?
>
See me last point.

> Also if you have concern on implementation, ok we can discuss further.
> But can you make it clear as what your objections are for the doorbell
> MHU binding. How will I get the bit assigned for different protocols
> which are platform specific ? I still need some binding , right ?
>
Sorry, No. I'll try to get lkml fwd me the patchset so I can explain
there further.

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Sudeep Holla]

On 06/07/17 19:37, Jassi Brar wrote:
> On Thu, Jul 6, 2017 at 10:14 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>> On 06/07/17 15:37, Jassi Brar wrote:
>>> On Thu, Jul 6, 2017 at 3:03 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
> 
>>> I see no reason why you must have SCPI and SCMI both running.
>>>
>>
>> We can still have 2 different protocols using same MHU channel with
>> different doorbells, what's wrong with that ?
>>
> Only for SCMI and SCPI - both written by same person to achieve the
> same thing - I think it should not be necessary.
> 

Really ? you decide based on that and repeatedly ignore what ARM MHU
specification offers. Wow, I am surprised.

> But yes, SCMI running alongside some complementary protocol (that
> implements what SCMI doesn't provide) is a very solid usecase. And
> that is the reason you need a platform specific 'transport layer'.
> 

The whole SCMI exercise is to reduce such platform specific code.
It's made to work with ACPI using doorbells via PCC channels. Now, you
say for DT we need per platform shim. Mailbox is my transport, why do I
need anything more as I don't care what is sent in mailbox controller as
long as the signal is sent to the other side.

>>> And even then there is a solution - a shim arbitrator. Other
>>> platforms, those share a channel, do that. No big deal.
>>>
>>
>> Example please? Please remember these protocols are generic and we
>> can't add any platform specific code into them.
>>
> You do need to have platform specific glue as I said.
> 

Yes, please propose along bindings to do that as you are objecting the
one in $subject. I have asked you many times the same thing.

>>>  BTW, I hope you realise that we need a 'transport layer' which will
>>> be the platform specific glue between mailbox controller specifics and
>>> the generic SCMI code.
>>
>> Why ?
>>
> Because you should not restrict the usage of SCMI to only platforms
> with mailbox controller that does not require any information to
> trigger a signal on the other side -- what you call "doorbell".
> 

Indeed, it's restricted, read the specification. We have all the
information in the channel shared memory and we just need doorbell
in the transport. It's clear in the specification. You seem to not read
any of the specification and continue to ignore.

> Why shouldn't Rockchip be able to implement SCMI? Just because it uses
> different mechanism to signal the other end?
> 

They can, all we need is just a mailbox channel, we don't care wants
sent in the controller as along as signal is sent. The shared memory has
to be as per the specification. I still don't know what I am missing to
convey you.

> You are actually putting in effort to restrict the usage of SCMI. Its
> like you buy a usb device and the first thing you do is solder it to
> the plug.
> 

I don't think you have read the SCMI specification, sorry.

> 
>> Clearly you have not made a since technical argument so far as why
>> MHU doorbell is not correct way even when MHU specification is clearly
>> allows it. I have given example of ST mailbox which has this doorbell
>> kind of support.
>>
>>> I see your confusion in the form of some issues in the SCMI
>>> implementation, please CC me on the next revision.
>>>
>>
>> Care to elaborate on what's my confusion or at-least what you think so ?
>>
> See me last point.
> 

I think you are just fixiated on one usage of ARM MHU and so hard to
convince otherwise..

>> Also if you have concern on implementation, ok we can discuss further.
>> But can you make it clear as what your objections are for the doorbell
>> MHU binding. How will I get the bit assigned for different protocols
>> which are platform specific ? I still need some binding , right ?
>>
> Sorry, No. I'll try to get lkml fwd me the patchset so I can explain
> there further.
> 

Sure, thanks.

-- 
Regards,
Sudeep

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Jassi Brar]

On Fri, Jul 7, 2017 at 5:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 06/07/17 19:37, Jassi Brar wrote:
>> On Thu, Jul 6, 2017 at 10:14 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>
>>> On 06/07/17 15:37, Jassi Brar wrote:
>>>> On Thu, Jul 6, 2017 at 3:03 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>
>>
>>>> I see no reason why you must have SCPI and SCMI both running.
>>>>
>>>
>>> We can still have 2 different protocols using same MHU channel with
>>> different doorbells, what's wrong with that ?
>>>
>> Only for SCMI and SCPI - both written by same person to achieve the
>> same thing - I think it should not be necessary.
>>
>
> Really ? you decide based on that and repeatedly ignore what ARM MHU
> specification offers. Wow, I am surprised.
>
>> But yes, SCMI running alongside some complementary protocol (that
>> implements what SCMI doesn't provide) is a very solid usecase. And
>> that is the reason you need a platform specific 'transport layer'.
>>
>
> The whole SCMI exercise is to reduce such platform specific code.
> It's made to work with ACPI using doorbells via PCC channels. Now, you
> say for DT we need per platform shim. Mailbox is my transport, why do I
> need anything more as I don't care what is sent in mailbox controller as
> long as the signal is sent to the other side.
>
>>>> And even then there is a solution - a shim arbitrator. Other
>>>> platforms, those share a channel, do that. No big deal.
>>>>
>>>
>>> Example please? Please remember these protocols are generic and we
>>> can't add any platform specific code into them.
>>>
>> You do need to have platform specific glue as I said.
>>
>
> Yes, please propose along bindings to do that as you are objecting the
> one in $subject. I have asked you many times the same thing.
>
"platform specific transport layer"  =>  no generic bindings for the controller.

The bindings, that already exist for mailbox controllers plus what
properties the SCMI need, are to be used by a thin platform specific
driver to provide an interface to the generic SCMI implementation to
send the "doorbell".

For example, look at how the common DWC3 driver is used by platforms
that have different h/w integrations.
   $ grep synopsys,dwc3 -rw -B15 arch/arm/boot/dts/

That is, SCMI node as a child of platform specific SCMI node. Or maybe
even SCMI as a library kinda thing.

I can't explain it any better.


>>>>  BTW, I hope you realise that we need a 'transport layer' which will
>>>> be the platform specific glue between mailbox controller specifics and
>>>> the generic SCMI code.
>>>
>>> Why ?
>>>
>> Because you should not restrict the usage of SCMI to only platforms
>> with mailbox controller that does not require any information to
>> trigger a signal on the other side -- what you call "doorbell".
>>
>
> Indeed, it's restricted, read the specification....
>
Its a shame to hear that.


>> Why shouldn't Rockchip be able to implement SCMI? Just because it uses
>> different mechanism to signal the other end?
>>
>
> They can, all we need is just a mailbox channel, we don't care wants
> sent in the controller as along as signal is sent. The shared memory has
> to be as per the specification. I still don't know what I am missing to
> convey you.
>

In scmi_do_xfer() you pass pointer to 'struct scmi_xfer' as 'data'. That is

    mbox_send_message(struct mbox_chan *, struct scmi_xfer*);

A controller driver defines is own format for the data it needs to
send a message.

For example, Rockchip driver expects
rockchip_mbox_send_data(struct mbox_chan *, struct rockchip_mbox_msg *)

Fow the huck will it work ?!

SCMI is purely a s/w protocol which can work over any physical link.
But it wouldn't because you have funny notions of a "doorbell".

Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Sudeep Holla]

On 07/07/17 14:12, Jassi Brar wrote:
> On Fri, Jul 7, 2017 at 5:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>>
>> On 06/07/17 19:37, Jassi Brar wrote:
>>> On Thu, Jul 6, 2017 at 10:14 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>>
>>>> On 06/07/17 15:37, Jassi Brar wrote:
>>>>> On Thu, Jul 6, 2017 at 3:03 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>>
>>>
>>>>> I see no reason why you must have SCPI and SCMI both running.
>>>>>
>>>>
>>>> We can still have 2 different protocols using same MHU channel with
>>>> different doorbells, what's wrong with that ?
>>>>
>>> Only for SCMI and SCPI - both written by same person to achieve the
>>> same thing - I think it should not be necessary.
>>>
>>
>> Really ? you decide based on that and repeatedly ignore what ARM MHU
>> specification offers. Wow, I am surprised.
>>
>>> But yes, SCMI running alongside some complementary protocol (that
>>> implements what SCMI doesn't provide) is a very solid usecase. And
>>> that is the reason you need a platform specific 'transport layer'.
>>>
>>
>> The whole SCMI exercise is to reduce such platform specific code.
>> It's made to work with ACPI using doorbells via PCC channels. Now, you
>> say for DT we need per platform shim. Mailbox is my transport, why do I
>> need anything more as I don't care what is sent in mailbox controller as
>> long as the signal is sent to the other side.
>>
>>>>> And even then there is a solution - a shim arbitrator. Other
>>>>> platforms, those share a channel, do that. No big deal.
>>>>>
>>>>
>>>> Example please? Please remember these protocols are generic and we
>>>> can't add any platform specific code into them.
>>>>
>>> You do need to have platform specific glue as I said.
>>>
>>
>> Yes, please propose along bindings to do that as you are objecting the
>> one in $subject. I have asked you many times the same thing.
>>
> "platform specific transport layer"  =>  no generic bindings for the controller.
> 

It's not platform specific. It's in MHU specification and please stop
making it platform specific just because your platform didn't use it.
STOP this nonsense argument and read the MHU specification. I am tired
of mentioning that again and again.

> The bindings, that already exist for mailbox controllers plus what
> properties the SCMI need, are to be used by a thin platform specific
> driver to provide an interface to the generic SCMI implementation to
> send the "doorbell".
> 

It's non-sense to provide such layer for each and every controller and
platform. MHU supports doorbells and I need to add that support. You
still fail to make any technical issues adding such feature. What is the
problem you see adding that feature. Please list them.

> For example, look at how the common DWC3 driver is used by platforms
> that have different h/w integrations.
>    $ grep synopsys,dwc3 -rw -B15 arch/arm/boot/dts/
> 
> That is, SCMI node as a child of platform specific SCMI node. Or maybe
> even SCMI as a library kinda thing.
> 

1. It's not topic of this patch
2. SCMI is discoverable and we want to have as minimum binding as
   possible and now you are asking to add unnecessary layers of binding
   which I don't agree with and I am fine if you propose and convince
   DT maintainers.

>>>>>  BTW, I hope you realise that we need a 'transport layer' which will
>>>>> be the platform specific glue between mailbox controller specifics and
>>>>> the generic SCMI code.
>>>>
>>>> Why ?
>>>>
>>> Because you should not restrict the usage of SCMI to only platforms
>>> with mailbox controller that does not require any information to
>>> trigger a signal on the other side -- what you call "doorbell".
>>>
>>
>> Indeed, it's restricted, read the specification....
>>
> Its a shame to hear that.
> 

Sorry, but I am not author of the specification. Also just FYI it was
reviewed and accepted by multiple vendors.

>>> Why shouldn't Rockchip be able to implement SCMI? Just because it uses
>>> different mechanism to signal the other end?
>>>
>>
>> They can, all we need is just a mailbox channel, we don't care wants
>> sent in the controller as along as signal is sent. The shared memory has
>> to be as per the specification. I still don't know what I am missing to
>> convey you.
>>
> 
> In scmi_do_xfer() you pass pointer to 'struct scmi_xfer' as 'data'. That is
> 
>     mbox_send_message(struct mbox_chan *, struct scmi_xfer*);
> 
> A controller driver defines is own format for the data it needs to
> send a message.
> 

Exactly the point and that's why I didn't bother much to drag you into
SCMI. The main reason why I am adding doorbel to MHU is not just SCMI.
It's to support multiple protocols implemented using different bit of
each channel as a doorbell. Please don't link that with SCMI. You are
the one who wanted to look at the code. Hence I pointed to SCMI, I am
*not* saying SCMI cares what MHU driver send.

But as another requirement, I have 4 different channel with 2 different
protocol. It may also extend to 6 different channels where we want to
reserve 2 for DVFS alone and which may not do interrupt based
communication. This is to achieve fast switching patch from the schedutil.

> For example, Rockchip driver expects
> rockchip_mbox_send_data(struct mbox_chan *, struct rockchip_mbox_msg *)
> 
> Fow the huck will it work ?!
> 

Yes, did I make any comment anywhere objecting that ? Sorry and I am
wrong if did.

> SCMI is purely a s/w protocol which can work over any physical link.
> But it wouldn't because you have funny notions of a "doorbell".
> 

Really ? Doorbell is very well know mechanism in hardware. SCMI didn't
invent it, it just *uses* it.

-- 
Regards,
Sudeep