[PATCHv5 0/4] Common Mailbox Framework

[PATCHv5 0/4] Common Mailbox Framework

[Jassi Brar]

Hi,
  Here is the next revision of Mailbox framwork.

Changes since v4:
 o Common DT binding for Controller and Client drivers
    As a result, discard string based channel matching
 o Provide for an atomic 'peek' api, that a client could
    call to trigger the controller driver push data upwards.
 o OMAP and Highbank conversion to new api is left out, which
    can be converted later by the developers.

Changes since v3:
 o Change name of symbols from ipc to mbox
 o Return real types instead of void *
 o Align structures
 o Change some symbol names
	rxcb -> rx_callback
	txcb -> tx_done
 o Added kernel-doc for exported API
 o Dropped the cl_id and use clients pointer for callbacks.
 o Fixed locking of channel pool
 o Return negative error code for unsuccessful ipc_send_message()
 o Module referencing during mailbox assignment to a client.
 o Made error code symbols specific to mailbox.

Thanks
Jassi

[PATCHv5 1/4] mailbox: rename pl320-ipc specific mailbox.h

[Jassi Brar]

From: Suman Anna <s-anna@ti.com>

The patch 30058677 "ARM / highbank: add support for pl320 IPC"
added a pl320 IPC specific header file as a generic mailbox.h.
This file has been renamed appropriately to allow the
introduction of the generic mailbox API framework.

Acked-by: Mark Langsdorf <mark.langsdorf@calxeda.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Suman Anna <s-anna@ti.com>
---
 arch/arm/mach-highbank/highbank.c        | 2 +-
 drivers/cpufreq/highbank-cpufreq.c       | 2 +-
 drivers/mailbox/pl320-ipc.c              | 2 +-
 include/linux/{mailbox.h => pl320-ipc.h} | 0
 4 files changed, 3 insertions(+), 3 deletions(-)
 rename include/linux/{mailbox.h => pl320-ipc.h} (100%)

diff --git a/arch/arm/mach-highbank/highbank.c b/arch/arm/mach-highbank/highbank.c
index c7de89b..f295cbb 100644
--- a/arch/arm/mach-highbank/highbank.c
+++ b/arch/arm/mach-highbank/highbank.c
@@ -20,7 +20,7 @@
 #include <linux/input.h>
 #include <linux/io.h>
 #include <linux/irqchip.h>
-#include <linux/mailbox.h>
+#include <linux/pl320-ipc.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
diff --git a/drivers/cpufreq/highbank-cpufreq.c b/drivers/cpufreq/highbank-cpufreq.c
index bf8902a..b464f29 100644
--- a/drivers/cpufreq/highbank-cpufreq.c
+++ b/drivers/cpufreq/highbank-cpufreq.c
@@ -19,7 +19,7 @@
 #include <linux/cpu.h>
 #include <linux/err.h>
 #include <linux/of.h>
-#include <linux/mailbox.h>
+#include <linux/pl320-ipc.h>
 #include <linux/platform_device.h>
 
 #define HB_CPUFREQ_CHANGE_NOTE	0x80000001
diff --git a/drivers/mailbox/pl320-ipc.c b/drivers/mailbox/pl320-ipc.c
index d873cba..f3755e0 100644
--- a/drivers/mailbox/pl320-ipc.c
+++ b/drivers/mailbox/pl320-ipc.c
@@ -26,7 +26,7 @@
 #include <linux/device.h>
 #include <linux/amba/bus.h>
 
-#include <linux/mailbox.h>
+#include <linux/pl320-ipc.h>
 
 #define IPCMxSOURCE(m)		((m) * 0x40)
 #define IPCMxDSET(m)		(((m) * 0x40) + 0x004)
diff --git a/include/linux/mailbox.h b/include/linux/pl320-ipc.h
similarity index 100%
rename from include/linux/mailbox.h
rename to include/linux/pl320-ipc.h
-- 
1.8.1.2

[PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Jassi Brar]

Introduce common framework for client/protocol drivers and
controller drivers of Inter-Processor-Communication (IPC).

Client driver developers should have a look at
 include/linux/mailbox_client.h to understand the part of
the API exposed to client drivers.
Similarly controller driver developers should have a look
at include/linux/mailbox_controller.h

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 .../devicetree/bindings/mailbox/mailbox.txt        |  30 ++
 drivers/mailbox/Makefile                           |   4 +
 drivers/mailbox/mailbox.c                          | 480 +++++++++++++++++++++
 include/linux/mailbox.h                            |  20 +
 include/linux/mailbox_client.h                     |  45 ++
 include/linux/mailbox_controller.h                 | 121 ++++++
 6 files changed, 700 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/mailbox.txt
 create mode 100644 drivers/mailbox/mailbox.c
 create mode 100644 include/linux/mailbox.h
 create mode 100644 include/linux/mailbox_client.h
 create mode 100644 include/linux/mailbox_controller.h

diff --git a/Documentation/devicetree/bindings/mailbox/mailbox.txt b/Documentation/devicetree/bindings/mailbox/mailbox.txt
new file mode 100644
index 0000000..0eb2fb0
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/mailbox.txt
@@ -0,0 +1,30 @@
+* Generic Mailbox Controller and client driver bindings
+
+Generic binding to provide a way for Mailbox controller drivers to assign appropriate mailbox channel to client drivers.
+
+* MAILBOX Controller
+
+Required property:
+- #mbox-cells: Must be at least 1. Number of cells in a mailbox specifier.
+
+Example:
+	mailbox: mailbox {
+		...
+		#mbox-cells = <1>;
+	};
+
+
+* MAILBOX Client
+
+Required property:
+- mbox: List of phandle and mailbox channel specifier.
+
+- mbox-names: List of identifier strings for each mailbox channel required by the client.
+
+Example:
+	pwr_cntrl: power {
+		...
+		mbox-names = "pwr-ctrl", "rpc";
+		mbox = <&mailbox 0
+			&mailbox 1>;
+	};
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index e0facb3..2fa343a 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -1,3 +1,7 @@
+# Generic MAILBOX API
+
+obj-$(CONFIG_MAILBOX)		+= mailbox.o
+
 obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
 
 obj-$(CONFIG_OMAP_MBOX)		+= omap-mailbox.o
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
new file mode 100644
index 0000000..921fedd3
--- /dev/null
+++ b/drivers/mailbox/mailbox.c
@@ -0,0 +1,480 @@
+/*
+ * Mailbox: Common code for Mailbox controllers and users
+ *
+ * Copyright (C) 2014 Linaro Ltd.
+ * Author: Jassi Brar <jassisinghbrar@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/spinlock.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/mailbox_client.h>
+#include <linux/mailbox_controller.h>
+
+#define TXDONE_BY_IRQ	(1 << 0) /* controller has remote RTR irq */
+#define TXDONE_BY_POLL	(1 << 1) /* controller can read status of last TX */
+#define TXDONE_BY_ACK	(1 << 2) /* S/W ACK recevied by Client ticks the TX */
+
+static LIST_HEAD(mbox_cons);
+static DEFINE_MUTEX(con_mutex);
+
+static int _add_to_rbuf(struct mbox_chan *chan, void *mssg)
+{
+	int idx;
+	unsigned long flags;
+
+	spin_lock_irqsave(&chan->lock, flags);
+
+	/* See if there is any space left */
+	if (chan->msg_count == MBOX_TX_QUEUE_LEN) {
+		spin_unlock_irqrestore(&chan->lock, flags);
+		return -ENOMEM;
+	}
+
+	idx = chan->msg_free;
+	chan->msg_data[idx] = mssg;
+	chan->msg_count++;
+
+	if (idx == MBOX_TX_QUEUE_LEN - 1)
+		chan->msg_free = 0;
+	else
+		chan->msg_free++;
+
+	spin_unlock_irqrestore(&chan->lock, flags);
+
+	return idx;
+}
+
+static void _msg_submit(struct mbox_chan *chan)
+{
+	unsigned count, idx;
+	unsigned long flags;
+	void *data;
+	int err;
+
+	spin_lock_irqsave(&chan->lock, flags);
+
+	if (!chan->msg_count || chan->active_req) {
+		spin_unlock_irqrestore(&chan->lock, flags);
+		return;
+	}
+
+	count = chan->msg_count;
+	idx = chan->msg_free;
+	if (idx >= count)
+		idx -= count;
+	else
+		idx += MBOX_TX_QUEUE_LEN - count;
+
+	data = chan->msg_data[idx];
+
+	/* Try to submit a message to the MBOX controller */
+	err = chan->mbox->ops->send_data(chan, data);
+	if (!err) {
+		chan->active_req = data;
+		chan->msg_count--;
+	}
+
+	spin_unlock_irqrestore(&chan->lock, flags);
+}
+
+static void tx_tick(struct mbox_chan *chan, enum mbox_result r)
+{
+	unsigned long flags;
+	void *mssg;
+
+	spin_lock_irqsave(&chan->lock, flags);
+	mssg = chan->active_req;
+	chan->active_req = NULL;
+	spin_unlock_irqrestore(&chan->lock, flags);
+
+	/* Submit next message */
+	_msg_submit(chan);
+
+	/* Notify the client */
+	if (chan->cl->tx_block)
+		complete(&chan->tx_complete);
+	else if (mssg && chan->cl->tx_done)
+		chan->cl->tx_done(chan->cl, mssg, r);
+}
+
+static void poll_txdone(unsigned long data)
+{
+	struct mbox_controller *mbox = (struct mbox_controller *)data;
+	bool txdone, resched = false;
+	int i;
+
+	for (i = 0; i < mbox->num_chans; i++) {
+		struct mbox_chan *chan = &mbox->chans[i];
+
+		if (chan->active_req && chan->cl) {
+			resched = true;
+			txdone = chan->mbox->ops->last_tx_done(chan);
+			if (txdone)
+				tx_tick(chan, MBOX_OK);
+		}
+	}
+
+	if (resched)
+		mod_timer(&mbox->poll,
+			jiffies + msecs_to_jiffies(mbox->period));
+}
+
+/**
+ * mbox_chan_received_data - A way for controller driver to push data
+ *				received from remote to the upper layer.
+ * @chan: Pointer to the mailbox channel on which RX happened.
+ * @data: Client specific message typecasted as void *
+ *
+ * After startup and before shutdown any data received on the chan
+ * is passed on to the API via atomic mbox_chan_received_data().
+ * The controller should ACK the RX only after this call returns.
+ */
+void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
+{
+	/* No buffering the received data */
+	if (chan->cl->rx_callback)
+		chan->cl->rx_callback(chan->cl, mssg);
+}
+EXPORT_SYMBOL_GPL(mbox_chan_received_data);
+
+/**
+ * mbox_chan_txdone - A way for controller driver to notify the
+ *			framework that the last TX has completed.
+ * @chan: Pointer to the mailbox chan on which TX happened.
+ * @r: Status of last TX - OK or ERROR
+ *
+ * The controller that has IRQ for TX ACK calls this atomic API
+ * to tick the TX state machine. It works only if txdone_irq
+ * is set by the controller.
+ */
+void mbox_chan_txdone(struct mbox_chan *chan, enum mbox_result r)
+{
+	if (unlikely(!(chan->txdone_method & TXDONE_BY_IRQ))) {
+		pr_err("Controller can't run the TX ticker\n");
+		return;
+	}
+
+	tx_tick(chan, r);
+}
+EXPORT_SYMBOL_GPL(mbox_chan_txdone);
+
+/**
+ * mbox_client_txdone - The way for a client to run the TX state machine.
+ * @chan: Mailbox channel assigned to this client.
+ * @r: Success status of last transmission.
+ *
+ * The client/protocol had received some 'ACK' packet and it notifies
+ * the API that the last packet was sent successfully. This only works
+ * if the controller can't sense TX-Done.
+ */
+void mbox_client_txdone(struct mbox_chan *chan, enum mbox_result r)
+{
+	if (unlikely(!(chan->txdone_method & TXDONE_BY_ACK))) {
+		pr_err("Client can't run the TX ticker\n");
+		return;
+	}
+
+	tx_tick(chan, r);
+}
+EXPORT_SYMBOL_GPL(mbox_client_txdone);
+
+/**
+ * mbox_client_peek_data - A way for client driver to pull data
+ *			received from remote by the controller.
+ * @chan: Mailbox channel assigned to this client.
+ *
+ * A poke to controller driver for any received data.
+ * The data is actually passed onto client via the
+ * mbox_chan_received_data()
+ * The call can be made from atomic context, so the controller's
+ * implementation of peek_data() must not sleep.
+ *
+ * Return: True, if controller has, and is going to push after this,
+ *          some data.
+ *         False, if controller doesn't have any data to be read.
+ */
+bool mbox_client_peek_data(struct mbox_chan *chan)
+{
+	if (chan->mbox->ops->peek_data)
+		return chan->mbox->ops->peek_data(chan);
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(mbox_client_peek_data);
+
+/**
+ * mbox_send_message -	For client to submit a message to be
+ *				sent to the remote.
+ * @chan: Mailbox channel assigned to this client.
+ * @mssg: Client specific message typecasted.
+ *
+ * For client to submit data to the controller destined for a remote
+ * processor. If the client had set 'tx_block', the call will return
+ * either when the remote receives the data or when 'tx_tout' millisecs
+ * run out.
+ *  In non-blocking mode, the requests are buffered by the API and a
+ * non-negative token is returned for each queued request. If the request
+ * is not queued, a negative token is returned. Upon failure or successful
+ * TX, the API calls 'tx_done' from atomic context, from which the client
+ * could submit yet another request.
+ *  In blocking mode, 'tx_done' is not called, effectively making the
+ * queue length 1.
+ * The pointer to message should be preserved until it is sent
+ * over the chan, i.e, tx_done() is made.
+ * This function could be called from atomic context as it simply
+ * queues the data and returns a token against the request.
+ *
+ * Return: Non-negative integer for successful submission (non-blocking mode)
+ *	or transmission over chan (blocking mode).
+ *	Negative value denotes failure.
+ */
+int mbox_send_message(struct mbox_chan *chan, void *mssg)
+{
+	int t;
+
+	if (!chan || !chan->cl)
+		return -EINVAL;
+
+	t = _add_to_rbuf(chan, mssg);
+	if (t < 0) {
+		pr_err("Try increasing MBOX_TX_QUEUE_LEN\n");
+		return t;
+	}
+
+	_msg_submit(chan);
+
+	if (chan->txdone_method	== TXDONE_BY_POLL)
+		poll_txdone((unsigned long)chan->mbox);
+
+	if (chan->cl->tx_block && chan->active_req) {
+		int ret;
+		init_completion(&chan->tx_complete);
+		ret = wait_for_completion_timeout(&chan->tx_complete,
+			chan->cl->tx_tout);
+		if (ret == 0) {
+			t = -EIO;
+			tx_tick(chan, MBOX_ERR);
+		}
+	}
+
+	return t;
+}
+EXPORT_SYMBOL_GPL(mbox_send_message);
+
+/**
+ * mbox_request_channel - Request a mailbox channel.
+ * @cl: Identity of the client requesting the channel.
+ *
+ * The Client specifies its requirements and capabilities while asking for
+ * a mailbox channel by name. It can't be called from atomic context.
+ * The channel is exclusively allocated and can't be used by another
+ * client before the owner calls mbox_free_channel.
+ * After assignment, any packet received on this channel will be
+ * handed over to the client via the 'rx_callback'.
+ *
+ * Return: Pointer to the channel assigned to the client if successful.
+ *		ERR_PTR for request failure.
+ */
+struct mbox_chan *mbox_request_channel(struct mbox_client *cl)
+{
+	struct device *dev = cl->dev;
+	struct mbox_controller *mbox;
+	struct of_phandle_args spec;
+	struct mbox_chan *chan;
+	unsigned long flags;
+	int count, i, ret;
+
+	if (!dev || !dev->of_node) {
+		pr_err("%s: No owner device node\n", __func__);
+		return ERR_PTR(-ENODEV);
+	}
+
+	count = of_property_count_strings(dev->of_node, "mbox-names");
+	if (count < 0) {
+		pr_err("%s: mbox-names property of node '%s' missing\n",
+			__func__, dev->of_node->full_name);
+		return ERR_PTR(-ENODEV);
+	}
+
+	mutex_lock(&con_mutex);
+
+	ret = -ENODEV;
+	for (i = 0; i < count; i++) {
+		const char *s;
+
+		if (of_property_read_string_index(dev->of_node,
+						"mbox-names", i, &s))
+			continue;
+
+		if (strcmp(cl->chan_name, s))
+			continue;
+
+		if (of_parse_phandle_with_args(dev->of_node,
+					 "mbox", "#mbox-cells",	i, &spec))
+			continue;
+
+		chan = NULL;
+		list_for_each_entry(mbox, &mbox_cons, node)
+			if (mbox->dev->of_node == spec.np) {
+				chan = mbox->of_xlate(mbox, &spec);
+				break;
+			}
+
+		of_node_put(spec.np);
+
+		if (!chan)
+			continue;
+
+		ret = -EBUSY;
+		if (!chan->cl && try_module_get(mbox->dev->driver->owner))
+			break;
+	}
+
+	if (i == count) {
+		mutex_unlock(&con_mutex);
+		return ERR_PTR(ret);
+	}
+
+	spin_lock_irqsave(&chan->lock, flags);
+	chan->msg_free = 0;
+	chan->msg_count = 0;
+	chan->active_req = NULL;
+	chan->cl = cl;
+	if (!cl->tx_tout) /* wait for ever */
+		cl->tx_tout = msecs_to_jiffies(3600000);
+	else
+		cl->tx_tout = msecs_to_jiffies(cl->tx_tout);
+	if (chan->txdone_method	== TXDONE_BY_POLL
+			&& cl->knows_txdone)
+		chan->txdone_method |= TXDONE_BY_ACK;
+	spin_unlock_irqrestore(&chan->lock, flags);
+
+	ret = chan->mbox->ops->startup(chan);
+	if (ret) {
+		pr_err("Unable to startup the chan\n");
+		mbox_free_channel(chan);
+		chan = ERR_PTR(ret);
+	}
+
+	mutex_unlock(&con_mutex);
+	return chan;
+}
+EXPORT_SYMBOL_GPL(mbox_request_channel);
+
+/**
+ * mbox_free_channel - The client relinquishes control of a mailbox
+ *			channel by this call.
+ * @chan: The mailbox channel to be freed.
+ */
+void mbox_free_channel(struct mbox_chan *chan)
+{
+	unsigned long flags;
+
+	if (!chan || !chan->cl)
+		return;
+
+	chan->mbox->ops->shutdown(chan);
+
+	/* The queued TX requests are simply aborted, no callbacks are made */
+	spin_lock_irqsave(&chan->lock, flags);
+	chan->cl = NULL;
+	chan->active_req = NULL;
+	if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
+		chan->txdone_method = TXDONE_BY_POLL;
+
+	module_put(chan->mbox->dev->driver->owner);
+	spin_unlock_irqrestore(&chan->lock, flags);
+}
+EXPORT_SYMBOL_GPL(mbox_free_channel);
+
+static struct mbox_chan *
+of_mbox_index_xlate(struct mbox_controller *mbox,
+				const struct of_phandle_args *sp)
+{
+	int ind = sp->args[0];
+
+	if (ind >= mbox->num_chans)
+		return NULL;
+
+	return &mbox->chans[ind];
+}
+
+/**
+ * mbox_controller_register - Register the mailbox controller
+ * @mbox:	Pointer to the mailbox controller.
+ *
+ * The controller driver registers its communication chans
+ */
+int mbox_controller_register(struct mbox_controller *mbox)
+{
+	int i, txdone;
+
+	/* Sanity check */
+	if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
+		return -EINVAL;
+
+	if (mbox->txdone_irq)
+		txdone = TXDONE_BY_IRQ;
+	else if (mbox->txdone_poll)
+		txdone = TXDONE_BY_POLL;
+	else /* It has to be ACK then */
+		txdone = TXDONE_BY_ACK;
+
+	if (txdone == TXDONE_BY_POLL) {
+		mbox->poll.function = &poll_txdone;
+		mbox->poll.data = (unsigned long)mbox;
+		init_timer(&mbox->poll);
+	}
+
+	for (i = 0; i < mbox->num_chans; i++) {
+		struct mbox_chan *chan = &mbox->chans[i];
+		chan->cl = NULL;
+		chan->mbox = mbox;
+		chan->txdone_method = txdone;
+		spin_lock_init(&chan->lock);
+	}
+
+	if (!mbox->of_xlate)
+		mbox->of_xlate = of_mbox_index_xlate;
+
+	mutex_lock(&con_mutex);
+	list_add_tail(&mbox->node, &mbox_cons);
+	mutex_unlock(&con_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mbox_controller_register);
+
+/**
+ * mbox_controller_unregister - UnRegister the mailbox controller
+ * @mbox:	Pointer to the mailbox controller.
+ */
+void mbox_controller_unregister(struct mbox_controller *mbox)
+{
+	int i;
+
+	if (!mbox)
+		return;
+
+	mutex_lock(&con_mutex);
+
+	list_del(&mbox->node);
+
+	for (i = 0; i < mbox->num_chans; i++)
+		mbox_free_channel(&mbox->chans[i]);
+
+	del_timer_sync(&mbox->poll);
+
+	mutex_unlock(&con_mutex);
+}
+EXPORT_SYMBOL_GPL(mbox_controller_unregister);
diff --git a/include/linux/mailbox.h b/include/linux/mailbox.h
new file mode 100644
index 0000000..fe7b806
--- /dev/null
+++ b/include/linux/mailbox.h
@@ -0,0 +1,20 @@
+/*
+ * Copyright (C) 2014 Linaro Ltd.
+ * Author: Jassi Brar <jassisinghbrar@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __MAILBOX_H
+#define __MAILBOX_H
+
+#include <linux/of.h>
+
+enum mbox_result {
+	MBOX_OK = 0,
+	MBOX_ERR,
+};
+
+#endif /* __MAILBOX_H */
diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
new file mode 100644
index 0000000..bbac2d2
--- /dev/null
+++ b/include/linux/mailbox_client.h
@@ -0,0 +1,45 @@
+/*
+ * Copyright (C) 2014 Linaro Ltd.
+ * Author: Jassi Brar <jassisinghbrar@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __MAILBOX_CLIENT_H
+#define __MAILBOX_CLIENT_H
+
+#include <linux/mailbox.h>
+
+struct mbox_chan;
+
+/**
+ * struct mbox_client - User of a mailbox
+ * @dev:		The client device
+ * @chan_name:		The "controller:channel" this client wants
+ * @rx_callback:	Atomic callback to provide client the data received
+ * @tx_done:		Atomic callback to tell client of data transmission
+ * @tx_block:		If the mbox_send_message should block until data is
+ *			transmitted.
+ * @tx_tout:		Max block period in ms before TX is assumed failure
+ * @knows_txdone:	if the client could run the TX state machine. Usually
+ *			if the client receives some ACK packet for transmission.
+ *			Unused if the controller already has TX_Done/RTR IRQ.
+ */
+struct mbox_client {
+	struct device *dev;
+	const char *chan_name;
+	void (*rx_callback)(struct mbox_client *cl, void *mssg);
+	void (*tx_done)(struct mbox_client *cl, void *mssg, enum mbox_result r);
+	bool tx_block;
+	unsigned long tx_tout;
+	bool knows_txdone;
+};
+
+struct mbox_chan *mbox_request_channel(struct mbox_client *cl);
+int mbox_send_message(struct mbox_chan *chan, void *mssg);
+void mbox_client_txdone(struct mbox_chan *chan, enum mbox_result r);
+void mbox_free_channel(struct mbox_chan *chan);
+
+#endif /* __MAILBOX_CLIENT_H */
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
new file mode 100644
index 0000000..cf81e80
--- /dev/null
+++ b/include/linux/mailbox_controller.h
@@ -0,0 +1,121 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __MAILBOX_CONTROLLER_H
+#define __MAILBOX_CONTROLLER_H
+
+#include <linux/mailbox.h>
+
+struct mbox_chan;
+
+/**
+ * struct mbox_chan_ops - s/w representation of a communication chan
+ * @send_data:	The API asks the MBOX controller driver, in atomic
+ *		context try to transmit a message on the bus. Returns 0 if
+ *		data is accepted for transmission, -EBUSY while rejecting
+ *		if the remote hasn't yet read the last data sent. Actual
+ *		transmission of data is reported by the controller via
+ *		mbox_chan_txdone (if it has some TX ACK irq). It must not
+ *		block.
+ * @startup:	Called when a client requests the chan. The controller
+ *		could ask clients for additional parameters of communication
+ *		to be provided via client's chan_data. This call may
+ *		block. After this call the Controller must forward any
+ *		data received on the chan by calling mbox_chan_received_data.
+ * @shutdown:	Called when a client relinquishes control of a chan.
+ *		This call may block too. The controller must not forwared
+ *		any received data anymore.
+ * @last_tx_done: If the controller sets 'txdone_poll', the API calls
+ *		  this to poll status of last TX. The controller must
+ *		  give priority to IRQ method over polling and never
+ *		  set both txdone_poll and txdone_irq. Only in polling
+ *		  mode 'send_data' is expected to return -EBUSY.
+ *		  Used only if txdone_poll:=true && txdone_irq:=false
+ * @peek_data: Atomic check for any received data. Return true if controller
+ *		  has some data to push to the client. False otherwise.
+ */
+struct mbox_chan_ops {
+	int (*send_data)(struct mbox_chan *chan, void *data);
+	int (*startup)(struct mbox_chan *chan);
+	void (*shutdown)(struct mbox_chan *chan);
+	bool (*last_tx_done)(struct mbox_chan *chan);
+	bool (*peek_data)(struct mbox_chan *chan);
+};
+
+/**
+ * struct mbox_controller - Controller of a class of communication chans
+ * @dev:		Device backing this controller
+ * @controller_name:	Literal name of the controller.
+ * @ops:		Operators that work on each communication chan
+ * @chans:		Null terminated array of chans.
+ * @txdone_irq:		Indicates if the controller can report to API when
+ *			the last transmitted data was read by the remote.
+ *			Eg, if it has some TX ACK irq.
+ * @txdone_poll:	If the controller can read but not report the TX
+ *			done. Ex, some register shows the TX status but
+ *			no interrupt rises. Ignored if 'txdone_irq' is set.
+ * @txpoll_period:	If 'txdone_poll' is in effect, the API polls for
+ *			last TX's status after these many millisecs
+ */
+struct mbox_controller {
+	struct device *dev;
+	struct mbox_chan_ops *ops;
+	struct mbox_chan *chans;
+	int num_chans;
+	bool txdone_irq;
+	bool txdone_poll;
+	unsigned txpoll_period;
+	struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox,
+					const struct of_phandle_args *sp);
+	/*
+	 * If the controller supports only TXDONE_BY_POLL,
+	 * this timer polls all the links for txdone.
+	 */
+	struct timer_list poll;
+	unsigned period;
+	/* Hook to add to the global controller list */
+	struct list_head node;
+} __aligned(32);
+
+/*
+ * The length of circular buffer for queuing messages from a client.
+ * 'msg_count' tracks the number of buffered messages while 'msg_free'
+ * is the index where the next message would be buffered.
+ * We shouldn't need it too big because every transferr is interrupt
+ * triggered and if we have lots of data to transfer, the interrupt
+ * latencies are going to be the bottleneck, not the buffer length.
+ * Besides, mbox_send_message could be called from atomic context and
+ * the client could also queue another message from the notifier 'tx_done'
+ * of the last transfer done.
+ * REVIST: If too many platforms see the "Try increasing MBOX_TX_QUEUE_LEN"
+ * print, it needs to be taken from config option or somesuch.
+ */
+#define MBOX_TX_QUEUE_LEN	20
+
+struct mbox_chan {
+	struct mbox_controller *mbox; /* Parent Controller */
+	unsigned txdone_method;
+
+	/* client */
+	struct mbox_client *cl;
+	struct completion tx_complete;
+
+	void *active_req;
+	unsigned msg_count, msg_free;
+	void *msg_data[MBOX_TX_QUEUE_LEN];
+	/* Access to the channel */
+	spinlock_t lock;
+
+	/* Private data for controller */
+	void *con_priv;
+} __aligned(32);
+
+int mbox_controller_register(struct mbox_controller *mbox);
+void mbox_chan_received_data(struct mbox_chan *chan, void *data);
+void mbox_chan_txdone(struct mbox_chan *chan, enum mbox_result r);
+void mbox_controller_unregister(struct mbox_controller *mbox);
+
+#endif /* __MAILBOX_CONTROLLER_H */
-- 
1.8.1.2

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Arnd Bergmann]

On Thursday 15 May 2014 11:41:00 Jassi Brar wrote:
> Introduce common framework for client/protocol drivers and
> controller drivers of Inter-Processor-Communication (IPC).
> 
> Client driver developers should have a look at
>  include/linux/mailbox_client.h to understand the part of
> the API exposed to client drivers.
> Similarly controller driver developers should have a look
> at include/linux/mailbox_controller.h
> 
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>

I hadn't followed all the previous versions, but this looks very nice!

I only have comments about tiny details.

> new file mode 100644
> index 0000000..0eb2fb0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/mailbox.txt
> @@ -0,0 +1,30 @@
> +* Generic Mailbox Controller and client driver bindings
> +
> +Generic binding to provide a way for Mailbox controller drivers to assign appropriate mailbox channel to client drivers.
> +
> +* MAILBOX Controller

Just formatting: wrap the lines after 70 characters, and don't use
capital letters for 'mailbox'.
> +
> +enum mbox_result {
> +	MBOX_OK = 0,
> +	MBOX_ERR,
> +};

How about using a standard error number?

> +/**
> + * struct mbox_client - User of a mailbox
> + * @dev:		The client device
> + * @chan_name:		The "controller:channel" this client wants
> + * @rx_callback:	Atomic callback to provide client the data received
> + * @tx_done:		Atomic callback to tell client of data transmission
> + * @tx_block:		If the mbox_send_message should block until data is
> + *			transmitted.
> + * @tx_tout:		Max block period in ms before TX is assumed failure
> + * @knows_txdone:	if the client could run the TX state machine. Usually
> + *			if the client receives some ACK packet for transmission.
> + *			Unused if the controller already has TX_Done/RTR IRQ.
> + */
> +struct mbox_client {
> +	struct device *dev;
> +	const char *chan_name;
> +	void (*rx_callback)(struct mbox_client *cl, void *mssg);
> +	void (*tx_done)(struct mbox_client *cl, void *mssg, enum mbox_result r);
> +	bool tx_block;
> +	unsigned long tx_tout;
> +	bool knows_txdone;
> +};
> +
> +struct mbox_chan *mbox_request_channel(struct mbox_client *cl);

Is it possible to make the argument 'const'?

Maybe document how you expect an mbox_client to be allocated:
- static const definition in driver
- dynamically allocated and embedded in some client specific struct
- on the stack and discarded after mbox_request_channel()

> +/**
> + * struct mbox_controller - Controller of a class of communication chans
> + * @dev:		Device backing this controller
> + * @controller_name:	Literal name of the controller.
> + * @ops:		Operators that work on each communication chan
> + * @chans:		Null terminated array of chans.
> + * @txdone_irq:		Indicates if the controller can report to API when
> + *			the last transmitted data was read by the remote.
> + *			Eg, if it has some TX ACK irq.
> + * @txdone_poll:	If the controller can read but not report the TX
> + *			done. Ex, some register shows the TX status but
> + *			no interrupt rises. Ignored if 'txdone_irq' is set.
> + * @txpoll_period:	If 'txdone_poll' is in effect, the API polls for
> + *			last TX's status after these many millisecs
> + */
> +struct mbox_controller {
> +	struct device *dev;
> +	struct mbox_chan_ops *ops;
> +	struct mbox_chan *chans;
> +	int num_chans;
> +	bool txdone_irq;
> +	bool txdone_poll;
> +	unsigned txpoll_period;
> +	struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox,
> +					const struct of_phandle_args *sp);
> +	/*
> +	 * If the controller supports only TXDONE_BY_POLL,
> +	 * this timer polls all the links for txdone.
> +	 */
> +	struct timer_list poll;
> +	unsigned period;
> +	/* Hook to add to the global controller list */
> +	struct list_head node;
> +} __aligned(32);

What is the __aligned(32) for?

	Arnd

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Jassi Brar]

On 15 May 2014 19:57, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 15 May 2014 11:41:00 Jassi Brar wrote:
>> Introduce common framework for client/protocol drivers and
>> controller drivers of Inter-Processor-Communication (IPC).
>>
>> Client driver developers should have a look at
>>  include/linux/mailbox_client.h to understand the part of
>> the API exposed to client drivers.
>> Similarly controller driver developers should have a look
>> at include/linux/mailbox_controller.h
>>
>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>
> I hadn't followed all the previous versions, but this looks very nice!
>
> I only have comments about tiny details.
>
>> new file mode 100644
>> index 0000000..0eb2fb0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/mailbox.txt
>> @@ -0,0 +1,30 @@
>> +* Generic Mailbox Controller and client driver bindings
>> +
>> +Generic binding to provide a way for Mailbox controller drivers to assign appropriate mailbox channel to client drivers.
>> +
>> +* MAILBOX Controller
>
> Just formatting: wrap the lines after 70 characters, and don't use
> capital letters for 'mailbox'.
>
OK, will fix. I usually do care about 80 char limits, but here
checkpatch didn't catch it either.

>> +
>> +enum mbox_result {
>> +     MBOX_OK = 0,
>> +     MBOX_ERR,
>> +};
>
> How about using a standard error number?
>
OK

>> +/**
>> + * struct mbox_client - User of a mailbox
>> + * @dev:             The client device
>> + * @chan_name:               The "controller:channel" this client wants
>> + * @rx_callback:     Atomic callback to provide client the data received
>> + * @tx_done:         Atomic callback to tell client of data transmission
>> + * @tx_block:                If the mbox_send_message should block until data is
>> + *                   transmitted.
>> + * @tx_tout:         Max block period in ms before TX is assumed failure
>> + * @knows_txdone:    if the client could run the TX state machine. Usually
>> + *                   if the client receives some ACK packet for transmission.
>> + *                   Unused if the controller already has TX_Done/RTR IRQ.
>> + */
>> +struct mbox_client {
>> +     struct device *dev;
>> +     const char *chan_name;
>> +     void (*rx_callback)(struct mbox_client *cl, void *mssg);
>> +     void (*tx_done)(struct mbox_client *cl, void *mssg, enum mbox_result r);
>> +     bool tx_block;
>> +     unsigned long tx_tout;
>> +     bool knows_txdone;
>> +};
>> +
>> +struct mbox_chan *mbox_request_channel(struct mbox_client *cl);
>
> Is it possible to make the argument 'const'?
>
Yes, will do.

> Maybe document how you expect an mbox_client to be allocated:
> - static const definition in driver
> - dynamically allocated and embedded in some client specific struct
> - on the stack and discarded after mbox_request_channel()
>
OK. The struct needs to be preserved until free_channel returns. So
clients may allocate on stack for quick single xfer or dynamically
allocated for 'server' type clients.

>> +/**
>> + * struct mbox_controller - Controller of a class of communication chans
>> + * @dev:             Device backing this controller
>> + * @controller_name: Literal name of the controller.
>> + * @ops:             Operators that work on each communication chan
>> + * @chans:           Null terminated array of chans.
>> + * @txdone_irq:              Indicates if the controller can report to API when
>> + *                   the last transmitted data was read by the remote.
>> + *                   Eg, if it has some TX ACK irq.
>> + * @txdone_poll:     If the controller can read but not report the TX
>> + *                   done. Ex, some register shows the TX status but
>> + *                   no interrupt rises. Ignored if 'txdone_irq' is set.
>> + * @txpoll_period:   If 'txdone_poll' is in effect, the API polls for
>> + *                   last TX's status after these many millisecs
>> + */
>> +struct mbox_controller {
>> +     struct device *dev;
>> +     struct mbox_chan_ops *ops;
>> +     struct mbox_chan *chans;
>> +     int num_chans;
>> +     bool txdone_irq;
>> +     bool txdone_poll;
>> +     unsigned txpoll_period;
>> +     struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox,
>> +                                     const struct of_phandle_args *sp);
>> +     /*
>> +      * If the controller supports only TXDONE_BY_POLL,
>> +      * this timer polls all the links for txdone.
>> +      */
>> +     struct timer_list poll;
>> +     unsigned period;
>> +     /* Hook to add to the global controller list */
>> +     struct list_head node;
>> +} __aligned(32);
>
> What is the __aligned(32) for?
>
Attempt to align access to mailbox?

I am still open to opinion about whether the mailbox ownership should
be exclusive or shared among clients. Need to handle async messages
from remote is one reason one might want more than one client to own a
channel. Allowing for RX via notifiers might be one option but that
breaks semantics of 'ownership' of a mailbox channel.
 Also, some platform might need to communicate with remote master
during very early boot like for initializing system timers and clocks.
The API isn't working then.

Thanks
Jassi

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Arnd Bergmann]

On Friday 16 May 2014 19:03:25 Jassi Brar wrote:
> >> +/**
> >> + * struct mbox_controller - Controller of a class of communication chans
> >> + * @dev:             Device backing this controller
> >> + * @controller_name: Literal name of the controller.
> >> + * @ops:             Operators that work on each communication chan
> >> + * @chans:           Null terminated array of chans.
> >> + * @txdone_irq:              Indicates if the controller can report to API when
> >> + *                   the last transmitted data was read by the remote.
> >> + *                   Eg, if it has some TX ACK irq.
> >> + * @txdone_poll:     If the controller can read but not report the TX
> >> + *                   done. Ex, some register shows the TX status but
> >> + *                   no interrupt rises. Ignored if 'txdone_irq' is set.
> >> + * @txpoll_period:   If 'txdone_poll' is in effect, the API polls for
> >> + *                   last TX's status after these many millisecs
> >> + */
> >> +struct mbox_controller {
> >> +     struct device *dev;
> >> +     struct mbox_chan_ops *ops;
> >> +     struct mbox_chan *chans;
> >> +     int num_chans;
> >> +     bool txdone_irq;
> >> +     bool txdone_poll;
> >> +     unsigned txpoll_period;
> >> +     struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox,
> >> +                                     const struct of_phandle_args *sp);
> >> +     /*
> >> +      * If the controller supports only TXDONE_BY_POLL,
> >> +      * this timer polls all the links for txdone.
> >> +      */
> >> +     struct timer_list poll;
> >> +     unsigned period;
> >> +     /* Hook to add to the global controller list */
> >> +     struct list_head node;
> >> +} __aligned(32);
> >
> > What is the __aligned(32) for?
> >
> Attempt to align access to mailbox?

I still don't understand why it matters. This data structure is internal
to the kernel, at least I don't see anything that is accessed by the
hardware here. Note that anything that allocates a mbox_controller through
kmalloc will not get the alignment from here anyway, but will get the
default slab alignment instead (which happens to also be 32 bytes on
ARM).

> I am still open to opinion about whether the mailbox ownership should
> be exclusive or shared among clients. Need to handle async messages
> from remote is one reason one might want more than one client to own a
> channel. Allowing for RX via notifiers might be one option but that
> breaks semantics of 'ownership' of a mailbox channel.

I don't have a strong opinion on that.

>  Also, some platform might need to communicate with remote master
> during very early boot like for initializing system timers and clocks.
> The API isn't working then.

Do you have an example for a platform like that? I'd expect that normally
we can have a boot loader that sets up the system timer to work good
enough for us to get into normal driver initialization.

	Arnd

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Jassi Brar]

On 19 May 2014 18:38, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 16 May 2014 19:03:25 Jassi Brar wrote:

>
>>  Also, some platform might need to communicate with remote master
>> during very early boot like for initializing system timers and clocks.
>> The API isn't working then.
>
> Do you have an example for a platform like that? I'd expect that normally
> we can have a boot loader that sets up the system timer to work good
> enough for us to get into normal driver initialization.
>
My platform. We choose to keep bootloader to the minimum and make
kernel not depend upon any goodies provided.
Second, which I don't think can be helped by a bootloader, the remote
master has gate & rate control of clocks to peripheral IPs. The
clk-api driver simply maps Linux requests onto mailbox commands. So
the mailbox is needed as early as CLK_OF_DECLARE (when kernel reads
the rate of every registered clock).   Any suggestions?

Thanks
Jassi

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Bjorn Andersson]

On Mon, May 19, 2014 at 11:03 AM, Jassi Brar <jaswinder.singh@linaro.org> wrote:
> On 19 May 2014 18:38, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Friday 16 May 2014 19:03:25 Jassi Brar wrote:
[...]
>> Do you have an example for a platform like that? I'd expect that normally
>> we can have a boot loader that sets up the system timer to work good
>> enough for us to get into normal driver initialization.
>>
> My platform. We choose to keep bootloader to the minimum and make
> kernel not depend upon any goodies provided.
> Second, which I don't think can be helped by a bootloader, the remote
> master has gate & rate control of clocks to peripheral IPs. The
> clk-api driver simply maps Linux requests onto mailbox commands. So
> the mailbox is needed as early as CLK_OF_DECLARE (when kernel reads
> the rate of every registered clock).   Any suggestions?

Hi Jessi

On the newer Qualcomm platform the root clocks are controlled by a "resource
power manager" system; all clocks and regulators accesses are done over a fifo
based communication link, over shared memory, with dependencies on e.g.
hwspinlocks.

There's nothing strange with this, you just have to bring up (probe) all those
drivers before you can initialize the clock driver so that you can
initialize any
peripheral device drivers. That's what you have EPROBE_DEFER for.


Even in your sparsely initialized system, you must have enough clocks to
bring up the mailbox driver, so that you can bring up your clock driver and
from that you're good to initialize the rest.

Regards,
Bjorn

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Arnd Bergmann]

On Monday 19 May 2014 12:55:51 Bjorn Andersson wrote:
> On Mon, May 19, 2014 at 11:03 AM, Jassi Brar <jaswinder.singh@linaro.org> wrote:
> > On 19 May 2014 18:38, Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Friday 16 May 2014 19:03:25 Jassi Brar wrote:
> [...]
> >> Do you have an example for a platform like that? I'd expect that normally
> >> we can have a boot loader that sets up the system timer to work good
> >> enough for us to get into normal driver initialization.
> >>
> > My platform. We choose to keep bootloader to the minimum and make
> > kernel not depend upon any goodies provided.
> > Second, which I don't think can be helped by a bootloader, the remote
> > master has gate & rate control of clocks to peripheral IPs. The
> > clk-api driver simply maps Linux requests onto mailbox commands. So
> > the mailbox is needed as early as CLK_OF_DECLARE (when kernel reads
> > the rate of every registered clock).   Any suggestions?
> 
> Hi Jessi
> 
> On the newer Qualcomm platform the root clocks are controlled by a "resource
> power manager" system; all clocks and regulators accesses are done over a fifo
> based communication link, over shared memory, with dependencies on e.g.
> hwspinlocks.
> 
> There's nothing strange with this, you just have to bring up (probe) all those
> drivers before you can initialize the clock driver so that you can
> initialize any
> peripheral device drivers. That's what you have EPROBE_DEFER for.
> 
> 
> Even in your sparsely initialized system, you must have enough clocks to
> bring up the mailbox driver, so that you can bring up your clock driver and
> from that you're good to initialize the rest.

The one dependency that is really hard to avoid is usually the clocksource.
If you can't enable that early, you don't get into the normal driver probe
and need some hack instead.

	Arnd

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Jassi Brar]

On Tue, May 20, 2014 at 1:25 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
> On Mon, May 19, 2014 at 11:03 AM, Jassi Brar <jaswinder.singh@linaro.org> wrote:
>> On 19 May 2014 18:38, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Friday 16 May 2014 19:03:25 Jassi Brar wrote:
> [...]
>>> Do you have an example for a platform like that? I'd expect that normally
>>> we can have a boot loader that sets up the system timer to work good
>>> enough for us to get into normal driver initialization.
>>>
>> My platform. We choose to keep bootloader to the minimum and make
>> kernel not depend upon any goodies provided.
>> Second, which I don't think can be helped by a bootloader, the remote
>> master has gate & rate control of clocks to peripheral IPs. The
>> clk-api driver simply maps Linux requests onto mailbox commands. So
>> the mailbox is needed as early as CLK_OF_DECLARE (when kernel reads
>> the rate of every registered clock).   Any suggestions?
>
> Hi Jessi
>
> On the newer Qualcomm platform the root clocks are controlled by a "resource
> power manager" system; all clocks and regulators accesses are done over a fifo
> based communication link, over shared memory, with dependencies on e.g.
> hwspinlocks.
>
> There's nothing strange with this, you just have to bring up (probe) all those
> drivers before you can initialize the clock driver so that you can
> initialize any
> peripheral device drivers. That's what you have EPROBE_DEFER for.
>
>
> Even in your sparsely initialized system, you must have enough clocks to
> bring up the mailbox driver, so that you can bring up your clock driver and
> from that you're good to initialize the rest.
>
As Arnd suspected, my clocksource SP804's clk as well is owned by the remote.
Another situation (again my platform) where EPROBE_DEFER won't help is
when CPU reset control is with the remote because smp_init() trumps
even a core_initcall.

Regards,
Jassi

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Matt Porter]

On Fri, May 16, 2014 at 07:03:25PM +0530, Jassi Brar wrote:
> On 15 May 2014 19:57, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Thursday 15 May 2014 11:41:00 Jassi Brar wrote:

...

> >> +struct mbox_controller {
> >> +     struct device *dev;
> >> +     struct mbox_chan_ops *ops;
> >> +     struct mbox_chan *chans;
> >> +     int num_chans;
> >> +     bool txdone_irq;
> >> +     bool txdone_poll;
> >> +     unsigned txpoll_period;
> >> +     struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox,
> >> +                                     const struct of_phandle_args *sp);
> >> +     /*
> >> +      * If the controller supports only TXDONE_BY_POLL,
> >> +      * this timer polls all the links for txdone.
> >> +      */
> >> +     struct timer_list poll;
> >> +     unsigned period;
> >> +     /* Hook to add to the global controller list */
> >> +     struct list_head node;
> >> +} __aligned(32);
> >
> > What is the __aligned(32) for?
> >
> Attempt to align access to mailbox?
> 
> I am still open to opinion about whether the mailbox ownership should
> be exclusive or shared among clients. Need to handle async messages
> from remote is one reason one might want more than one client to own a
> channel. Allowing for RX via notifiers might be one option but that
> breaks semantics of 'ownership' of a mailbox channel.

This is the case we have on a new family of Broadcom SoCs. One mailbox
channel is the "system" channel and is shared amongst many clients for
reception of unsolicited events from the coprocessor. The same channel
is also used commonly in drivers for debug/inspection of the M0 state.
Functionality for clock, pmu, pinmux, and cpu idle/suspend is implemented
via IPC with the M0 so all of those drivers need to share one mailbox.

There's a lot of common code necessary to construct/parse IPCs for
each of the drivers. I suppose this IPC driver/api could be the
sole owner of that system channel. However, we'd need to develop some
binding to represent IPC resources that devices need to operate. Coming
into this late...I wonder if I missed something about where these vendor
specific IPC layers should live and how, if it makes sense, they should
be represented in DT. In our case, the IPC is an integral part of the
hardware as it's loaded from ROM.

-Matt

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Jassi Brar]

On Thu, May 29, 2014 at 9:13 PM, Matt Porter <mporter@linaro.org> wrote:
> On Fri, May 16, 2014 at 07:03:25PM +0530, Jassi Brar wrote:
>> On 15 May 2014 19:57, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Thursday 15 May 2014 11:41:00 Jassi Brar wrote:
>
> ...
>
>> >> +struct mbox_controller {
>> >> +     struct device *dev;
>> >> +     struct mbox_chan_ops *ops;
>> >> +     struct mbox_chan *chans;
>> >> +     int num_chans;
>> >> +     bool txdone_irq;
>> >> +     bool txdone_poll;
>> >> +     unsigned txpoll_period;
>> >> +     struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox,
>> >> +                                     const struct of_phandle_args *sp);
>> >> +     /*
>> >> +      * If the controller supports only TXDONE_BY_POLL,
>> >> +      * this timer polls all the links for txdone.
>> >> +      */
>> >> +     struct timer_list poll;
>> >> +     unsigned period;
>> >> +     /* Hook to add to the global controller list */
>> >> +     struct list_head node;
>> >> +} __aligned(32);
>> >
>> > What is the __aligned(32) for?
>> >
>> Attempt to align access to mailbox?
>>
>> I am still open to opinion about whether the mailbox ownership should
>> be exclusive or shared among clients. Need to handle async messages
>> from remote is one reason one might want more than one client to own a
>> channel. Allowing for RX via notifiers might be one option but that
>> breaks semantics of 'ownership' of a mailbox channel.
>
> This is the case we have on a new family of Broadcom SoCs. One mailbox
> channel is the "system" channel and is shared amongst many clients for
> reception of unsolicited events from the coprocessor. The same channel
> is also used commonly in drivers for debug/inspection of the M0 state.
> Functionality for clock, pmu, pinmux, and cpu idle/suspend is implemented
> via IPC with the M0 so all of those drivers need to share one mailbox.
>
OK, so you have a single mailbox channel used for communication with
the remote master.

> There's a lot of common code necessary to construct/parse IPCs for
> each of the drivers. I suppose this IPC driver/api could be the
> sole owner of that system channel. However, we'd need to develop some
> binding to represent IPC resources that devices need to operate. Coming
> into this late...I wonder if I missed something about where these vendor
> specific IPC layers should live and how, if it makes sense, they should
> be represented in DT. In our case, the IPC is an integral part of the
> hardware as it's loaded from ROM.
>
Like other platforms, the IPC protocol is going to be very specific to
Broadcom, even if the mailbox controller is some popular third party
IP like PL320. So you/Broadcom have to implement parser code for IPC
(client) above the mailbox common api and the controller driver below
it. Any resource/feature specific to your client and your controller
should be passed via some Broadcom specific DT bindings. The common
mailbox api DT bindings provide only for channel-client mapping.

 Being more specific to your platform, I think you need some server
code (mailbox's client) that every driver (like clock, pmu, pinmux
etc) registers with to send messages to remote and receive commands
from remote ... perhaps by registering some filter to sort out
messages for each driver.

-Jassi

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Matt Porter]

On Fri, May 30, 2014 at 11:01:55AM +0530, Jassi Brar wrote:
> On Thu, May 29, 2014 at 9:13 PM, Matt Porter <mporter@linaro.org> wrote:
> > On Fri, May 16, 2014 at 07:03:25PM +0530, Jassi Brar wrote:
> >> On 15 May 2014 19:57, Arnd Bergmann <arnd@arndb.de> wrote:
> >> > On Thursday 15 May 2014 11:41:00 Jassi Brar wrote:
> >
> > ...
> >
> >> >> +struct mbox_controller {
> >> >> +     struct device *dev;
> >> >> +     struct mbox_chan_ops *ops;
> >> >> +     struct mbox_chan *chans;
> >> >> +     int num_chans;
> >> >> +     bool txdone_irq;
> >> >> +     bool txdone_poll;
> >> >> +     unsigned txpoll_period;
> >> >> +     struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox,
> >> >> +                                     const struct of_phandle_args *sp);
> >> >> +     /*
> >> >> +      * If the controller supports only TXDONE_BY_POLL,
> >> >> +      * this timer polls all the links for txdone.
> >> >> +      */
> >> >> +     struct timer_list poll;
> >> >> +     unsigned period;
> >> >> +     /* Hook to add to the global controller list */
> >> >> +     struct list_head node;
> >> >> +} __aligned(32);
> >> >
> >> > What is the __aligned(32) for?
> >> >
> >> Attempt to align access to mailbox?
> >>
> >> I am still open to opinion about whether the mailbox ownership should
> >> be exclusive or shared among clients. Need to handle async messages
> >> from remote is one reason one might want more than one client to own a
> >> channel. Allowing for RX via notifiers might be one option but that
> >> breaks semantics of 'ownership' of a mailbox channel.
> >
> > This is the case we have on a new family of Broadcom SoCs. One mailbox
> > channel is the "system" channel and is shared amongst many clients for
> > reception of unsolicited events from the coprocessor. The same channel
> > is also used commonly in drivers for debug/inspection of the M0 state.
> > Functionality for clock, pmu, pinmux, and cpu idle/suspend is implemented
> > via IPC with the M0 so all of those drivers need to share one mailbox.
> >
> OK, so you have a single mailbox channel used for communication with
> the remote master.

Yes, specifically, single mailbox channel is shared by many clients
for interrupt events.

> 
> > There's a lot of common code necessary to construct/parse IPCs for
> > each of the drivers. I suppose this IPC driver/api could be the
> > sole owner of that system channel. However, we'd need to develop some
> > binding to represent IPC resources that devices need to operate. Coming
> > into this late...I wonder if I missed something about where these vendor
> > specific IPC layers should live and how, if it makes sense, they should
> > be represented in DT. In our case, the IPC is an integral part of the
> > hardware as it's loaded from ROM.
> >
> Like other platforms, the IPC protocol is going to be very specific to
> Broadcom, even if the mailbox controller is some popular third party
> IP like PL320. So you/Broadcom have to implement parser code for IPC
> (client) above the mailbox common api and the controller driver below
> it. Any resource/feature specific to your client and your controller
> should be passed via some Broadcom specific DT bindings. The common
> mailbox api DT bindings provide only for channel-client mapping.

Agreed.

>  Being more specific to your platform, I think you need some server
> code (mailbox's client) that every driver (like clock, pmu, pinmux
> etc) registers with to send messages to remote and receive commands
> from remote ... perhaps by registering some filter to sort out
> messages for each driver.

Right, and here's where you hit on the problem. This server you mention
is not a piece of hardware, it would be a software construct. As such, it
doesn't fit into the DT binding as it exists. It's probably best to
illustrate in DT syntax.

If I were to represent the hardware relationship in the DT binding now
it would look like this:

---
cpm: mailbox@deadbeef {
        compatible = "brcm,bcm-cpm-mailbox";
        reg = <...>;
        #mbox-cells <1>;
        interrupts = <...>;
};

/* clock complex */
ccu {
        compatible = "brcm,bcm-foo-ccu";
        mbox = <&cpm CPM_SYSTEM_CHANNEL>;
	mbox-names = "system";
	/* leaving out other mailboxes for brevity */
        #clock-cells <1>;
        clock-output-names = "bar",
                             "baz";
};

pmu {
        compatible = "brcm,bcm-foo-pmu"
        mbox = <&cpm CPM_SYSTEM_CHANNEL>;
	mbox-names = "system";
};

pinmux {
	compatible = "brcm,bcm-foo-pinctrl";
        mbox = <&cpm CPM_SYSTEM_CHANNEL>;
	mbox-names = "system";
};
---

What we would need to do is completely ignore this information in each
of the of the client drivers associated with the clock, pmu, and pinmux
devices. This IPC server would need to be instantiated and get the
mailbox information from some source. mbox_request_channel() only works
when the client has an of_node with the mbox-names property present.
Let's say we follow this model and represent it in DT:

--
cpm: mailbox@deadbeef {
        compatible = "brcm,bcm-cpm-mailbox";
        reg = <...>;
        #mbox-cells <1>;
        interrupts = <...>;
};

cpm_ipc {
	compatible = "brcm,bcm-cpm-ipc";
	mbox = <&cpm CPM_SYSTEM_CHANNEL>;
	mbox-names = "system";
	/* leaving out other mailboxes for brevity */
};
---

This would allow an ipc driver to exclusively own this system channel,
but now we've invented a binding that doesn't reflect the hardware at
all. It's describing software so I don't believe the DT maintainers will
allow this type of construct.

One possibly is to bring back string-based channel matching, and allow
the controller node to optionally handle the mbox-names property. As in:

---
cpm: mailbox@deadbeef {
        compatible = "brcm,bcm-cpm-mailbox";
        reg = <...>;
        interrupts = <...>;
        #mbox-cells <1>;
	mbox-names = "foo",
		     "bar",
		     "baz",
		     "system";
};
---

and allow a non-DT based mbox_request_channel() path using string
matching. I know it's icky and the reasons for dropping that in the
first place but I'm just throwing out one option that would work here.

-Matt

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Jassi Brar]

On Mon, Jun 2, 2014 at 8:44 PM, Matt Porter <mporter@linaro.org> wrote:
> On Fri, May 30, 2014 at 11:01:55AM +0530, Jassi Brar wrote:
>
>>  Being more specific to your platform, I think you need some server
>> code (mailbox's client) that every driver (like clock, pmu, pinmux
>> etc) registers with to send messages to remote and receive commands
>> from remote ... perhaps by registering some filter to sort out
>> messages for each driver.
>
> Right, and here's where you hit on the problem. This server you mention
> is not a piece of hardware, it would be a software construct. As such, it
> doesn't fit into the DT binding as it exists. It's probably best to
> illustrate in DT syntax.
>
> If I were to represent the hardware relationship in the DT binding now
> it would look like this:
>
> ---
> cpm: mailbox@deadbeef {
>         compatible = "brcm,bcm-cpm-mailbox";
>         reg = <...>;
>         #mbox-cells <1>;
>         interrupts = <...>;
> };
>
> /* clock complex */
> ccu {
>         compatible = "brcm,bcm-foo-ccu";
>         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>         mbox-names = "system";
>         /* leaving out other mailboxes for brevity */
>         #clock-cells <1>;
>         clock-output-names = "bar",
>                              "baz";
> };
>
> pmu {
>         compatible = "brcm,bcm-foo-pmu"
>         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>         mbox-names = "system";
> };
>
> pinmux {
>         compatible = "brcm,bcm-foo-pinctrl";
>         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>         mbox-names = "system";
> };
> ---
Yeah, I too don't think its a good idea.


> What we would need to do is completely ignore this information in each
> of the of the client drivers associated with the clock, pmu, and pinmux
> devices. This IPC server would need to be instantiated and get the
> mailbox information from some source. mbox_request_channel() only works
> when the client has an of_node with the mbox-names property present.
> Let's say we follow this model and represent it in DT:
>
> --
> cpm: mailbox@deadbeef {
>         compatible = "brcm,bcm-cpm-mailbox";
>         reg = <...>;
>         #mbox-cells <1>;
>         interrupts = <...>;
> };
>
> cpm_ipc {
>         compatible = "brcm,bcm-cpm-ipc";
>         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>         mbox-names = "system";
>         /* leaving out other mailboxes for brevity */
> };
> ---
>
> This would allow an ipc driver to exclusively own this system channel,
> but now we've invented a binding that doesn't reflect the hardware at
> all. It's describing software so I don't believe the DT maintainers will
> allow this type of construct.
>
Must the server node specify MMIO and an IRQ, to be acceptable? Like ...

cpm_ipc : cpm@deadbeef {
         compatible = "brcm,bcm-cpm-ipc";
       /*  reg = <0xdeadbeef 0x100>; */
       /*  interrupts = <0 123 4>;  */
         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
         mbox-names = "system";
};

cpm_ipc already specifies a hardware resource (mbox) that its driver
needs, I think that should be enough reason. If it were some purely
soft property for the driver like
      mode = "poll";  //or "irq"
then the node wouldn't be justified because that is the job of a
build-time config or run-time module option.

Regards,
-Jassi

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Matt Porter]

[Adding devicetree list]

On Mon, Jun 02, 2014 at 10:41:44PM +0530, Jassi Brar wrote:
> On Mon, Jun 2, 2014 at 8:44 PM, Matt Porter <mporter@linaro.org> wrote:
> > On Fri, May 30, 2014 at 11:01:55AM +0530, Jassi Brar wrote:
> >
> >>  Being more specific to your platform, I think you need some server
> >> code (mailbox's client) that every driver (like clock, pmu, pinmux
> >> etc) registers with to send messages to remote and receive commands
> >> from remote ... perhaps by registering some filter to sort out
> >> messages for each driver.
> >
> > Right, and here's where you hit on the problem. This server you mention
> > is not a piece of hardware, it would be a software construct. As such, it
> > doesn't fit into the DT binding as it exists. It's probably best to
> > illustrate in DT syntax.
> >
> > If I were to represent the hardware relationship in the DT binding now
> > it would look like this:
> >
> > ---
> > cpm: mailbox@deadbeef {
> >         compatible = "brcm,bcm-cpm-mailbox";
> >         reg = <...>;
> >         #mbox-cells <1>;
> >         interrupts = <...>;
> > };
> >
> > /* clock complex */
> > ccu {
> >         compatible = "brcm,bcm-foo-ccu";
> >         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
> >         mbox-names = "system";
> >         /* leaving out other mailboxes for brevity */
> >         #clock-cells <1>;
> >         clock-output-names = "bar",
> >                              "baz";
> > };
> >
> > pmu {
> >         compatible = "brcm,bcm-foo-pmu"
> >         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
> >         mbox-names = "system";
> > };
> >
> > pinmux {
> >         compatible = "brcm,bcm-foo-pinctrl";
> >         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
> >         mbox-names = "system";
> > };
> > ---
> Yeah, I too don't think its a good idea.
> 
> 
> > What we would need to do is completely ignore this information in each
> > of the of the client drivers associated with the clock, pmu, and pinmux
> > devices. This IPC server would need to be instantiated and get the
> > mailbox information from some source. mbox_request_channel() only works
> > when the client has an of_node with the mbox-names property present.
> > Let's say we follow this model and represent it in DT:
> >
> > --
> > cpm: mailbox@deadbeef {
> >         compatible = "brcm,bcm-cpm-mailbox";
> >         reg = <...>;
> >         #mbox-cells <1>;
> >         interrupts = <...>;
> > };
> >
> > cpm_ipc {
> >         compatible = "brcm,bcm-cpm-ipc";
> >         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
> >         mbox-names = "system";
> >         /* leaving out other mailboxes for brevity */
> > };
> > ---
> >
> > This would allow an ipc driver to exclusively own this system channel,
> > but now we've invented a binding that doesn't reflect the hardware at
> > all. It's describing software so I don't believe the DT maintainers will
> > allow this type of construct.
> >
> Must the server node specify MMIO and an IRQ, to be acceptable? Like ...

My bad, that was a cut and paste typo..I intended to remove those
properties as you do below.

> 
> cpm_ipc : cpm@deadbeef {
>          compatible = "brcm,bcm-cpm-ipc";
>        /*  reg = <0xdeadbeef 0x100>; */
>        /*  interrupts = <0 123 4>;  */
>          mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>          mbox-names = "system";
> };

Correct, this should have been:

cpm_ipc {
	compatible = "brcm,bcm-cpm-ipc";
	mbox = <&cpm CPM_SYSTEM_CHANNEL>;
	mbox-names = "system";
};

> cpm_ipc already specifies a hardware resource (mbox) that its driver
> needs, I think that should be enough reason. If it were some purely
> soft property for the driver like
>       mode = "poll";  //or "irq"
> then the node wouldn't be justified because that is the job of a
> build-time config or run-time module option.

Hrm, this is where I'd like to hear from the DT maintainers since we
have to live with this generic binding. If they are ok with us creating
bindings for a virtual device that exists only to match with our ipc
driver then it will work. I'm not aware of other similar examples though.

-Matt

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Sudeep Holla]

Hi Jassi,

On Mon, Jun 2, 2014 at 6:11 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Mon, Jun 2, 2014 at 8:44 PM, Matt Porter <mporter@linaro.org> wrote:
>> On Fri, May 30, 2014 at 11:01:55AM +0530, Jassi Brar wrote:
>>
>>>  Being more specific to your platform, I think you need some server
>>> code (mailbox's client) that every driver (like clock, pmu, pinmux
>>> etc) registers with to send messages to remote and receive commands
>>> from remote ... perhaps by registering some filter to sort out
>>> messages for each driver.
>>
>> Right, and here's where you hit on the problem. This server you mention
>> is not a piece of hardware, it would be a software construct. As such, it
>> doesn't fit into the DT binding as it exists. It's probably best to
>> illustrate in DT syntax.
>>
>> If I were to represent the hardware relationship in the DT binding now
>> it would look like this:
>>
>> ---
>> cpm: mailbox@deadbeef {
>>         compatible = "brcm,bcm-cpm-mailbox";
>>         reg = <...>;
>>         #mbox-cells <1>;
>>         interrupts = <...>;
>> };
>>
>> /* clock complex */
>> ccu {
>>         compatible = "brcm,bcm-foo-ccu";
>>         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>>         mbox-names = "system";
>>         /* leaving out other mailboxes for brevity */
>>         #clock-cells <1>;
>>         clock-output-names = "bar",
>>                              "baz";
>> };
>>
>> pmu {
>>         compatible = "brcm,bcm-foo-pmu"
>>         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>>         mbox-names = "system";
>> };
>>
>> pinmux {
>>         compatible = "brcm,bcm-foo-pinctrl";
>>         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>>         mbox-names = "system";
>> };
>> ---
> Yeah, I too don't think its a good idea.
>
>
>> What we would need to do is completely ignore this information in each
>> of the of the client drivers associated with the clock, pmu, and pinmux
>> devices. This IPC server would need to be instantiated and get the
>> mailbox information from some source. mbox_request_channel() only works
>> when the client has an of_node with the mbox-names property present.
>> Let's say we follow this model and represent it in DT:
>>
>> --
>> cpm: mailbox@deadbeef {
>>         compatible = "brcm,bcm-cpm-mailbox";
>>         reg = <...>;
>>         #mbox-cells <1>;
>>         interrupts = <...>;
>> };
>>
>> cpm_ipc {
>>         compatible = "brcm,bcm-cpm-ipc";
>>         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>>         mbox-names = "system";
>>         /* leaving out other mailboxes for brevity */
>> };
>> ---
>>
>> This would allow an ipc driver to exclusively own this system channel,
>> but now we've invented a binding that doesn't reflect the hardware at
>> all. It's describing software so I don't believe the DT maintainers will
>> allow this type of construct.
>>
> Must the server node specify MMIO and an IRQ, to be acceptable? Like ...
>
> cpm_ipc : cpm@deadbeef {
>          compatible = "brcm,bcm-cpm-ipc";
>        /*  reg = <0xdeadbeef 0x100>; */
>        /*  interrupts = <0 123 4>;  */
>          mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>          mbox-names = "system";
> };
>
> cpm_ipc already specifies a hardware resource (mbox) that its driver
> needs, I think that should be enough reason. If it were some purely
> soft property for the driver like
>       mode = "poll";  //or "irq"
> then the node wouldn't be justified because that is the job of a
> build-time config or run-time module option.
>

Like Matt, I am also in similar situation where there's a lot of common
code necessary to construct/parse IPCs for each of the drivers using the
mailbox.

As per your suggestion if we have single DT node to specify both the
controller and the client, we might still have to pollute this node with
software specific compatibles.

One possible scenario I can think of is that if the mailbox controller is
a standard primecell like PL320 used in multiple SoCs, each SoC vendor
will develop their own protocol implemented in their firmware. This is true
even with single SoC vendor having same IP but changing the protocol to
talk to their firmware. We will need a way to identify that protocol mechanism.
Does it make sense to add that  ? Is that something acceptable ?

Regards,
Sudeep

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Jassi Brar]

On 3 June 2014 15:05, Sudeep Holla <sudeep.holla@arm.com> wrote:
> Hi Jassi,
>
> On Mon, Jun 2, 2014 at 6:11 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>> On Mon, Jun 2, 2014 at 8:44 PM, Matt Porter <mporter@linaro.org> wrote:
>>> On Fri, May 30, 2014 at 11:01:55AM +0530, Jassi Brar wrote:
>>>
>>>>  Being more specific to your platform, I think you need some server
>>>> code (mailbox's client) that every driver (like clock, pmu, pinmux
>>>> etc) registers with to send messages to remote and receive commands
>>>> from remote ... perhaps by registering some filter to sort out
>>>> messages for each driver.
>>>
>>> Right, and here's where you hit on the problem. This server you mention
>>> is not a piece of hardware, it would be a software construct. As such, it
>>> doesn't fit into the DT binding as it exists. It's probably best to
>>> illustrate in DT syntax.
>>>
>>> If I were to represent the hardware relationship in the DT binding now
>>> it would look like this:
>>>
>>> ---
>>> cpm: mailbox@deadbeef {
>>>         compatible = "brcm,bcm-cpm-mailbox";
>>>         reg = <...>;
>>>         #mbox-cells <1>;
>>>         interrupts = <...>;
>>> };
>>>
>>> /* clock complex */
>>> ccu {
>>>         compatible = "brcm,bcm-foo-ccu";
>>>         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>>>         mbox-names = "system";
>>>         /* leaving out other mailboxes for brevity */
>>>         #clock-cells <1>;
>>>         clock-output-names = "bar",
>>>                              "baz";
>>> };
>>>
>>> pmu {
>>>         compatible = "brcm,bcm-foo-pmu"
>>>         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>>>         mbox-names = "system";
>>> };
>>>
>>> pinmux {
>>>         compatible = "brcm,bcm-foo-pinctrl";
>>>         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>>>         mbox-names = "system";
>>> };
>>> ---
>> Yeah, I too don't think its a good idea.
>>
>>
>>> What we would need to do is completely ignore this information in each
>>> of the of the client drivers associated with the clock, pmu, and pinmux
>>> devices. This IPC server would need to be instantiated and get the
>>> mailbox information from some source. mbox_request_channel() only works
>>> when the client has an of_node with the mbox-names property present.
>>> Let's say we follow this model and represent it in DT:
>>>
>>> --
>>> cpm: mailbox@deadbeef {
>>>         compatible = "brcm,bcm-cpm-mailbox";
>>>         reg = <...>;
>>>         #mbox-cells <1>;
>>>         interrupts = <...>;
>>> };
>>>
>>> cpm_ipc {
>>>         compatible = "brcm,bcm-cpm-ipc";
>>>         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>>>         mbox-names = "system";
>>>         /* leaving out other mailboxes for brevity */
>>> };
>>> ---
>>>
>>> This would allow an ipc driver to exclusively own this system channel,
>>> but now we've invented a binding that doesn't reflect the hardware at
>>> all. It's describing software so I don't believe the DT maintainers will
>>> allow this type of construct.
>>>
>> Must the server node specify MMIO and an IRQ, to be acceptable? Like ...
>>
>> cpm_ipc : cpm@deadbeef {
>>          compatible = "brcm,bcm-cpm-ipc";
>>        /*  reg = <0xdeadbeef 0x100>; */
>>        /*  interrupts = <0 123 4>;  */
>>          mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>>          mbox-names = "system";
>> };
>>
>> cpm_ipc already specifies a hardware resource (mbox) that its driver
>> needs, I think that should be enough reason. If it were some purely
>> soft property for the driver like
>>       mode = "poll";  //or "irq"
>> then the node wouldn't be justified because that is the job of a
>> build-time config or run-time module option.
>>
>
> Like Matt, I am also in similar situation where there's a lot of common
> code necessary to construct/parse IPCs for each of the drivers using the
> mailbox.
>
> As per your suggestion if we have single DT node to specify both the
> controller and the client, we might still have to pollute this node with
> software specific compatibles.
>
I am afraid you misunderstood me. I don't suggest single node for
mailbox controller and client, and IIUC, neither did Matt. Please note
the controller is cpm and client is cpm_ipc.

BTW, here we at least have a hardware resource to specify in the DT
node, there are examples in kernel where the DT nodes are purely
virtual. For ex, grep for "linux,spdif-dit". So I think we should be
ok.

> One possible scenario I can think of is that if the mailbox controller is
> a standard primecell like PL320 used in multiple SoCs, each SoC vendor
> will develop their own protocol implemented in their firmware. This is true
> even with single SoC vendor having same IP but changing the protocol to
> talk to their firmware.
>
Yeah, people have noted that in previous threads.

> We will need a way to identify that protocol mechanism.
> Does it make sense to add that  ? Is that something acceptable ?
>
 IMO we can't help it more than _trying_ to write the controller
driver as versatile as possible. And still some protocol
version/peculiarity could make reuse of the controller driver worse
than write a new for the protocol version. Any minor change in
behavior could be flagged to controller and client in platform
specific way.

Regards,
-Jassi

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Sudeep Holla]

Hi Jassi,

On 03/06/14 11:21, Jassi Brar wrote:
> On 3 June 2014 15:05, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> Hi Jassi,
>>
>> On Mon, Jun 2, 2014 at 6:11 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>> On Mon, Jun 2, 2014 at 8:44 PM, Matt Porter <mporter@linaro.org> wrote:
>>>> On Fri, May 30, 2014 at 11:01:55AM +0530, Jassi Brar wrote:
>>>>
>>>>>   Being more specific to your platform, I think you need some server
>>>>> code (mailbox's client) that every driver (like clock, pmu, pinmux
>>>>> etc) registers with to send messages to remote and receive commands
>>>>> from remote ... perhaps by registering some filter to sort out
>>>>> messages for each driver.
>>>>
>>>> Right, and here's where you hit on the problem. This server you mention
>>>> is not a piece of hardware, it would be a software construct. As such, it
>>>> doesn't fit into the DT binding as it exists. It's probably best to
>>>> illustrate in DT syntax.
>>>>
>>>> If I were to represent the hardware relationship in the DT binding now
>>>> it would look like this:
>>>>
>>>> ---
>>>> cpm: mailbox@deadbeef {
>>>>          compatible = "brcm,bcm-cpm-mailbox";
>>>>          reg = <...>;
>>>>          #mbox-cells <1>;
>>>>          interrupts = <...>;
>>>> };
>>>>
>>>> /* clock complex */
>>>> ccu {
>>>>          compatible = "brcm,bcm-foo-ccu";
>>>>          mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>>>>          mbox-names = "system";
>>>>          /* leaving out other mailboxes for brevity */
>>>>          #clock-cells <1>;
>>>>          clock-output-names = "bar",
>>>>                               "baz";
>>>> };
>>>>
>>>> pmu {
>>>>          compatible = "brcm,bcm-foo-pmu"
>>>>          mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>>>>          mbox-names = "system";
>>>> };
>>>>
>>>> pinmux {
>>>>          compatible = "brcm,bcm-foo-pinctrl";
>>>>          mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>>>>          mbox-names = "system";
>>>> };
>>>> ---
>>> Yeah, I too don't think its a good idea.
>>>
>>>
>>>> What we would need to do is completely ignore this information in each
>>>> of the of the client drivers associated with the clock, pmu, and pinmux
>>>> devices. This IPC server would need to be instantiated and get the
>>>> mailbox information from some source. mbox_request_channel() only works
>>>> when the client has an of_node with the mbox-names property present.
>>>> Let's say we follow this model and represent it in DT:
>>>>
>>>> --
>>>> cpm: mailbox@deadbeef {
>>>>          compatible = "brcm,bcm-cpm-mailbox";
>>>>          reg = <...>;
>>>>          #mbox-cells <1>;
>>>>          interrupts = <...>;
>>>> };
>>>>
>>>> cpm_ipc {
>>>>          compatible = "brcm,bcm-cpm-ipc";
>>>>          mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>>>>          mbox-names = "system";
>>>>          /* leaving out other mailboxes for brevity */
>>>> };

I am assuming this is what you referring below: cpm(which is the controller)
and cpm_ipc(a virtual node referring to the client). If yes that's fine, but is
this virtual node acceptable ?

>>>> ---
>>>>
>>>> This would allow an ipc driver to exclusively own this system channel,
>>>> but now we've invented a binding that doesn't reflect the hardware at
>>>> all. It's describing software so I don't believe the DT maintainers will
>>>> allow this type of construct.
>>>>
>>> Must the server node specify MMIO and an IRQ, to be acceptable? Like ...
>>>
>>> cpm_ipc : cpm@deadbeef {
>>>           compatible = "brcm,bcm-cpm-ipc";
>>>         /*  reg = <0xdeadbeef 0x100>; */
>>>         /*  interrupts = <0 123 4>;  */
>>>           mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>>>           mbox-names = "system";
>>> };
>>>

Sorry got carried away with this example, assumed it to be accidental comment
and the proposal was to unify both controller and client. Now its clear, ignore
my noise earlier.

>>> cpm_ipc already specifies a hardware resource (mbox) that its driver
>>> needs, I think that should be enough reason. If it were some purely
>>> soft property for the driver like
>>>        mode = "poll";  //or "irq"
>>> then the node wouldn't be justified because that is the job of a
>>> build-time config or run-time module option.
>>>
>>
>> Like Matt, I am also in similar situation where there's a lot of common
>> code necessary to construct/parse IPCs for each of the drivers using the
>> mailbox.
>>
>> As per your suggestion if we have single DT node to specify both the
>> controller and the client, we might still have to pollute this node with
>> software specific compatibles.
>>
> I am afraid you misunderstood me. I don't suggest single node for
> mailbox controller and client, and IIUC, neither did Matt. Please note
> the controller is cpm and client is cpm_ipc.
>
> BTW, here we at least have a hardware resource to specify in the DT
> node, there are examples in kernel where the DT nodes are purely
> virtual. For ex, grep for "linux,spdif-dit". So I think we should be
> ok.
>

Yes while I agree with you, but will it be acceptable for a generic mailbox
binding ? I think this is what even Matt mentioned earlier.

>> One possible scenario I can think of is that if the mailbox controller is
>> a standard primecell like PL320 used in multiple SoCs, each SoC vendor
>> will develop their own protocol implemented in their firmware. This is true
>> even with single SoC vendor having same IP but changing the protocol to
>> talk to their firmware.
>>
> Yeah, people have noted that in previous threads.
>

Ok that's good. I tried to follow previous discussions, but couldn't. I followed
only your last postings(v4) after I started using this driver.

>> We will need a way to identify that protocol mechanism.
>> Does it make sense to add that  ? Is that something acceptable ?
>>
>   IMO we can't help it more than _trying_ to write the controller
> driver as versatile as possible. And still some protocol
> version/peculiarity could make reuse of the controller driver worse
> than write a new for the protocol version. Any minor change in
> behavior could be flagged to controller and client in platform
> specific way.

Yes I understand that. So I was worried about unifying both controller and
protocol in one node. The controller driver must just handle raw data and never
interpret it. Now we just need DT maintainers' view on this virtual DT node.

Regards,
Sudeep

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Matt Porter]

On Tue, Jun 03, 2014 at 03:51:55PM +0530, Jassi Brar wrote:
> On 3 June 2014 15:05, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > Hi Jassi,
> >
> > On Mon, Jun 2, 2014 at 6:11 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >> On Mon, Jun 2, 2014 at 8:44 PM, Matt Porter <mporter@linaro.org> wrote:
> >>> On Fri, May 30, 2014 at 11:01:55AM +0530, Jassi Brar wrote:
> >>>
> >>>>  Being more specific to your platform, I think you need some server
> >>>> code (mailbox's client) that every driver (like clock, pmu, pinmux
> >>>> etc) registers with to send messages to remote and receive commands
> >>>> from remote ... perhaps by registering some filter to sort out
> >>>> messages for each driver.
> >>>
> >>> Right, and here's where you hit on the problem. This server you mention
> >>> is not a piece of hardware, it would be a software construct. As such, it
> >>> doesn't fit into the DT binding as it exists. It's probably best to
> >>> illustrate in DT syntax.
> >>>
> >>> If I were to represent the hardware relationship in the DT binding now
> >>> it would look like this:
> >>>
> >>> ---
> >>> cpm: mailbox@deadbeef {
> >>>         compatible = "brcm,bcm-cpm-mailbox";
> >>>         reg = <...>;
> >>>         #mbox-cells <1>;
> >>>         interrupts = <...>;
> >>> };
> >>>
> >>> /* clock complex */
> >>> ccu {
> >>>         compatible = "brcm,bcm-foo-ccu";
> >>>         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
> >>>         mbox-names = "system";
> >>>         /* leaving out other mailboxes for brevity */
> >>>         #clock-cells <1>;
> >>>         clock-output-names = "bar",
> >>>                              "baz";
> >>> };
> >>>
> >>> pmu {
> >>>         compatible = "brcm,bcm-foo-pmu"
> >>>         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
> >>>         mbox-names = "system";
> >>> };
> >>>
> >>> pinmux {
> >>>         compatible = "brcm,bcm-foo-pinctrl";
> >>>         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
> >>>         mbox-names = "system";
> >>> };
> >>> ---
> >> Yeah, I too don't think its a good idea.
> >>
> >>
> >>> What we would need to do is completely ignore this information in each
> >>> of the of the client drivers associated with the clock, pmu, and pinmux
> >>> devices. This IPC server would need to be instantiated and get the
> >>> mailbox information from some source. mbox_request_channel() only works
> >>> when the client has an of_node with the mbox-names property present.
> >>> Let's say we follow this model and represent it in DT:
> >>>
> >>> --
> >>> cpm: mailbox@deadbeef {
> >>>         compatible = "brcm,bcm-cpm-mailbox";
> >>>         reg = <...>;
> >>>         #mbox-cells <1>;
> >>>         interrupts = <...>;
> >>> };
> >>>
> >>> cpm_ipc {
> >>>         compatible = "brcm,bcm-cpm-ipc";
> >>>         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
> >>>         mbox-names = "system";
> >>>         /* leaving out other mailboxes for brevity */
> >>> };
> >>> ---
> >>>
> >>> This would allow an ipc driver to exclusively own this system channel,
> >>> but now we've invented a binding that doesn't reflect the hardware at
> >>> all. It's describing software so I don't believe the DT maintainers will
> >>> allow this type of construct.
> >>>
> >> Must the server node specify MMIO and an IRQ, to be acceptable? Like ...
> >>
> >> cpm_ipc : cpm@deadbeef {
> >>          compatible = "brcm,bcm-cpm-ipc";
> >>        /*  reg = <0xdeadbeef 0x100>; */
> >>        /*  interrupts = <0 123 4>;  */
> >>          mbox = <&cpm CPM_SYSTEM_CHANNEL>;
> >>          mbox-names = "system";
> >> };
> >>
> >> cpm_ipc already specifies a hardware resource (mbox) that its driver
> >> needs, I think that should be enough reason. If it were some purely
> >> soft property for the driver like
> >>       mode = "poll";  //or "irq"
> >> then the node wouldn't be justified because that is the job of a
> >> build-time config or run-time module option.
> >>
> >
> > Like Matt, I am also in similar situation where there's a lot of common
> > code necessary to construct/parse IPCs for each of the drivers using the
> > mailbox.
> >
> > As per your suggestion if we have single DT node to specify both the
> > controller and the client, we might still have to pollute this node with
> > software specific compatibles.
> >
> I am afraid you misunderstood me. I don't suggest single node for
> mailbox controller and client, and IIUC, neither did Matt. Please note
> the controller is cpm and client is cpm_ipc.

Correct, I had separate controller and consumer nodes as written
above...to match the binding.

> BTW, here we at least have a hardware resource to specify in the DT
> node, there are examples in kernel where the DT nodes are purely
> virtual. For ex, grep for "linux,spdif-dit". So I think we should be
> ok.
> 

There's a bit of a difference between my concern over a virtual node and
this example you've cited. In the dummy spdif transmitter, it's defining
a virtual device that plugs in for a codec, a hardware concept well
defined in the audio bindings. I agree that there are many examples of
this type of virtual node, including dummy phys, but in all cases they
are stubbing out a real hardware concept.

I find it to be distinctly different to create a node that doesn't
represent the hardware's use of mailboxes. I'd be happy if a DT
maintainer could say that this is acceptable though. ;)

-Matt

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Jassi Brar]

On 5 June 2014 16:42, Matt Porter <mporter@linaro.org> wrote:
> On Tue, Jun 03, 2014 at 03:51:55PM +0530, Jassi Brar wrote:
>
>> BTW, here we at least have a hardware resource to specify in the DT
>> node, there are examples in kernel where the DT nodes are purely
>> virtual. For ex, grep for "linux,spdif-dit". So I think we should be
>> ok.
>>
>
> There's a bit of a difference between my concern over a virtual node and
> this example you've cited. In the dummy spdif transmitter, it's defining
> a virtual device that plugs in for a codec, a hardware concept well
> defined in the audio bindings. I agree that there are many examples of
> this type of virtual node, including dummy phys, but in all cases they
> are stubbing out a real hardware concept.
>
> I find it to be distinctly different to create a node that doesn't
> represent the hardware's use of mailboxes.
>
The way I see "cpm_ipc" is that it represents a device that doesn't
need MMIO or an IRQ, but only the mailbox hardware resource.
"linux,spdif-dit" needs no hardware resource at all. So if anything,
more "virtual" than cpm_ipc.

> I'd be happy if a DT
> maintainer could say that this is acceptable though. ;)
>
OK, though it becomes clear only after reading this ;)

Cheers
-Jassi

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1162 bytes --]

On Thu, Jun 05, 2014 at 07:12:05AM -0400, Matt Porter wrote:
> On Tue, Jun 03, 2014 at 03:51:55PM +0530, Jassi Brar wrote:

> > BTW, here we at least have a hardware resource to specify in the DT
> > node, there are examples in kernel where the DT nodes are purely
> > virtual. For ex, grep for "linux,spdif-dit". So I think we should be
> > ok.

> There's a bit of a difference between my concern over a virtual node and
> this example you've cited. In the dummy spdif transmitter, it's defining
> a virtual device that plugs in for a codec, a hardware concept well
> defined in the audio bindings. I agree that there are many examples of
> this type of virtual node, including dummy phys, but in all cases they
> are stubbing out a real hardware concept.

Well, really it's an actual physical thing - it's something that appears
in the schematic and can be pointed at on the board but just doesn't
have software control.  From that point of view if the software that's
being interacted with on the remote processor is in ROM/flash which
won't or can't be realistically be updated (which seems to be mostly the
case) then you can reasonably say the same thing.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1340 bytes --]

On Thu, May 15, 2014 at 11:41:00AM +0530, Jassi Brar wrote:
> Introduce common framework for client/protocol drivers and
> controller drivers of Inter-Processor-Communication (IPC).

This looks pretty nice, though I do have a few *very* small nits beyond
those Arnd had.

> +	if (chan->cl->tx_block && chan->active_req) {
> +		int ret;
> +		init_completion(&chan->tx_complete);

reinit_completion().

> +	if (!cl->tx_tout) /* wait for ever */
> +		cl->tx_tout = msecs_to_jiffies(3600000);
> +	else
> +		cl->tx_tout = msecs_to_jiffies(cl->tx_tout);

Is the default wait for ever the best timeout - I'm not sure it's best
from a defensiveness point of view.  It should be fine either way,
it's just a matter of taste.

> +	ret = chan->mbox->ops->startup(chan);
> +	if (ret) {
> +		pr_err("Unable to startup the chan\n");

Perhaps print the error codes?  Might be helpful to users.

> +	/* The queued TX requests are simply aborted, no callbacks are made */
> +	spin_lock_irqsave(&chan->lock, flags);
> +	chan->cl = NULL;
> +	chan->active_req = NULL;
> +	if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
> +		chan->txdone_method = TXDONE_BY_POLL;
> +
> +	module_put(chan->mbox->dev->driver->owner);
> +	spin_unlock_irqrestore(&chan->lock, flags);

Is the module_put() safe in atomic context?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Arnd Bergmann]

On Wednesday 21 May 2014 18:27:01 Mark Brown wrote:
> > +     /* The queued TX requests are simply aborted, no callbacks are made */
> > +     spin_lock_irqsave(&chan->lock, flags);
> > +     chan->cl = NULL;
> > +     chan->active_req = NULL;
> > +     if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
> > +             chan->txdone_method = TXDONE_BY_POLL;
> > +
> > +     module_put(chan->mbox->dev->driver->owner);
> > +     spin_unlock_irqrestore(&chan->lock, flags);
> 
> Is the module_put() safe in atomic context?
> 

I'm pretty sure it is:

void module_put(struct module *module)
{
        if (module) {
                preempt_disable();
                smp_wmb(); /* see comment in module_refcount */
                __this_cpu_inc(module->refptr->decs);

                trace_module_put(module, _RET_IP_);
                preempt_enable();
        }
}

This disables preemption around everything it does, so everything inside
is definitely safe in nonpreemptible context.

	Arnd

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Jassi Brar]

On Wed, May 21, 2014 at 10:57 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, May 15, 2014 at 11:41:00AM +0530, Jassi Brar wrote:
>> Introduce common framework for client/protocol drivers and
>> controller drivers of Inter-Processor-Communication (IPC).
>
> This looks pretty nice, though I do have a few *very* small nits beyond
> those Arnd had.
>
>> +     if (chan->cl->tx_block && chan->active_req) {
>> +             int ret;
>> +             init_completion(&chan->tx_complete);
>
> reinit_completion().
>
>> +     if (!cl->tx_tout) /* wait for ever */
>> +             cl->tx_tout = msecs_to_jiffies(3600000);
>> +     else
>> +             cl->tx_tout = msecs_to_jiffies(cl->tx_tout);
>
> Is the default wait for ever the best timeout - I'm not sure it's best
> from a defensiveness point of view.  It should be fine either way,
> it's just a matter of taste.
>
The client wants the call to be blocking. Out of 'zero', 'infinity'
and some 'valid' delay, it makes better sense to have 'infinity' than
zero or another value that might be valid for some platform. I assume
1hr to be 'infinity', though I am open to better suggestions. Maybe
put a WARN() ?


>> +     ret = chan->mbox->ops->startup(chan);
>> +     if (ret) {
>> +             pr_err("Unable to startup the chan\n");
>
> Perhaps print the error codes?  Might be helpful to users.
>
OK.


BTW, I have not converted Highbank's PL320 and OMAP's controller and
client drivers. I believe Highbank's can't be converted to DT now and
Suman would want to convert the OMAP himself.

Also, maybe mailbox patches could be upstreamed via, say, arm-soc tree?

Regards,
Jassi

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Suman Anna]

On 05/27/2014 11:20 PM, Jassi Brar wrote:
> On Wed, May 21, 2014 at 10:57 PM, Mark Brown <broonie@kernel.org> wrote:
>> On Thu, May 15, 2014 at 11:41:00AM +0530, Jassi Brar wrote:
>>> Introduce common framework for client/protocol drivers and
>>> controller drivers of Inter-Processor-Communication (IPC).
>>
>> This looks pretty nice, though I do have a few *very* small nits beyond
>> those Arnd had.
>>
>>> +     if (chan->cl->tx_block && chan->active_req) {
>>> +             int ret;
>>> +             init_completion(&chan->tx_complete);
>>
>> reinit_completion().
>>
>>> +     if (!cl->tx_tout) /* wait for ever */
>>> +             cl->tx_tout = msecs_to_jiffies(3600000);
>>> +     else
>>> +             cl->tx_tout = msecs_to_jiffies(cl->tx_tout);
>>
>> Is the default wait for ever the best timeout - I'm not sure it's best
>> from a defensiveness point of view.  It should be fine either way,
>> it's just a matter of taste.
>>
> The client wants the call to be blocking. Out of 'zero', 'infinity'
> and some 'valid' delay, it makes better sense to have 'infinity' than
> zero or another value that might be valid for some platform. I assume
> 1hr to be 'infinity', though I am open to better suggestions. Maybe
> put a WARN() ?
> 
> 
>>> +     ret = chan->mbox->ops->startup(chan);
>>> +     if (ret) {
>>> +             pr_err("Unable to startup the chan\n");
>>
>> Perhaps print the error codes?  Might be helpful to users.
>>
> OK.
> 
> 
> BTW, I have not converted Highbank's PL320 and OMAP's controller and
> client drivers. I believe Highbank's can't be converted to DT now and
> Suman would want to convert the OMAP himself.

Yes, I will get to this next week, especially as there are new SoCs like
DRA7 and AM437x that need special handling.

regards,
Suman




> 
> Also, maybe mailbox patches could be upstreamed via, say, arm-soc tree?
> 
> Regards,
> Jassi
> 

Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1544 bytes --]

On Wed, May 28, 2014 at 09:50:21AM +0530, Jassi Brar wrote:
> On Wed, May 21, 2014 at 10:57 PM, Mark Brown <broonie@kernel.org> wrote:

> >> +     if (!cl->tx_tout) /* wait for ever */
> >> +             cl->tx_tout = msecs_to_jiffies(3600000);
> >> +     else
> >> +             cl->tx_tout = msecs_to_jiffies(cl->tx_tout);

> > Is the default wait for ever the best timeout - I'm not sure it's best
> > from a defensiveness point of view.  It should be fine either way,
> > it's just a matter of taste.

> The client wants the call to be blocking. Out of 'zero', 'infinity'
> and some 'valid' delay, it makes better sense to have 'infinity' than
> zero or another value that might be valid for some platform. I assume
> 1hr to be 'infinity', though I am open to better suggestions. Maybe
> put a WARN() ?

I think my inclination would be to have the client required to specify
what it thinks is a sensible delay and complain loudly if nothing is
specified.  Having zero default to wait for ever (as opposed to
something like a negative number) means that if someone memset()s the
struct (as people tend to) then if things go wrong the system will tend
to run into long timeouts which tend to give an unpleasant experience if
things go wrong.  

Forcing the user to think what a sensible delay is seems likely to
ensure that sensible values are picked rather than people not thinking
about it at all since if everything is working well and messages get
answers it's OK.

Sorry about the delay in replying here.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCHv5 3/4] mailbox: Fix TX completion init

[Jassi Brar]

From: LeyFoon Tan <lftan.linux@gmail.com>

For fast TX the complete could be called before being initialized as follows
 mbox_send_message --> poll_txdone --> tx_tick --> complete(&chan->tx_complete)

Init the completion early enough to fix the race.

Signed-off-by: LeyFoon Tan <lftan.linux@gmail.com>
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 drivers/mailbox/mailbox.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 921fedd3..befb256 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -245,6 +245,9 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
 	if (!chan || !chan->cl)
 		return -EINVAL;
 
+	if (chan->cl->tx_block)
+		init_completion(&chan->tx_complete);
+
 	t = _add_to_rbuf(chan, mssg);
 	if (t < 0) {
 		pr_err("Try increasing MBOX_TX_QUEUE_LEN\n");
@@ -258,7 +261,6 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
 
 	if (chan->cl->tx_block && chan->active_req) {
 		int ret;
-		init_completion(&chan->tx_complete);
 		ret = wait_for_completion_timeout(&chan->tx_complete,
 			chan->cl->tx_tout);
 		if (ret == 0) {
-- 
1.8.1.2

[PATCHv5 4/4] mailbox: Fix deleteing poll timer

[Jassi Brar]

From: LeyFoon Tan <lftan.linux@gmail.com>

Try to delete the timer only if it was init/used.

Signed-off-by: LeyFoon Tan <lftan.linux@gmail.com>
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 drivers/mailbox/mailbox.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index befb256..d83d12c 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -475,7 +475,8 @@ void mbox_controller_unregister(struct mbox_controller *mbox)
 	for (i = 0; i < mbox->num_chans; i++)
 		mbox_free_channel(&mbox->chans[i]);
 
-	del_timer_sync(&mbox->poll);
+	if (mbox->txdone_poll)
+		del_timer_sync(&mbox->poll);
 
 	mutex_unlock(&con_mutex);
 }
-- 
1.8.1.2