[PATCHv3] Generic Mailbox API

[PATCHv3] Generic Mailbox API

[Jassi Brar]

Hello,

  Here is what the generic mailbox api looks like after modifications
to make it work with 5 different platforms.
 Major feedback from Suman Anna(TI), LeyFoon Tan(Intel),
Craig McGeachie(Broadcom) and Loic Pallardy(ST).

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>

[PATCHv3 1/6] 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>
---
 drivers/cpufreq/highbank-cpufreq.c |  2 +-
 drivers/mailbox/pl320-ipc.c        |  2 +-
 include/linux/mailbox.h            | 17 -----------------
 include/linux/pl320-ipc.h          | 17 +++++++++++++++++
 4 files changed, 19 insertions(+), 19 deletions(-)
 delete mode 100644 include/linux/mailbox.h
 create mode 100644 include/linux/pl320-ipc.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/mailbox.h
deleted file mode 100644
index 5161f63..0000000
--- a/include/linux/mailbox.h
+++ /dev/null
@@ -1,17 +0,0 @@
-/*
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-int pl320_ipc_transmit(u32 *data);
-int pl320_ipc_register_notifier(struct notifier_block *nb);
-int pl320_ipc_unregister_notifier(struct notifier_block *nb);
diff --git a/include/linux/pl320-ipc.h b/include/linux/pl320-ipc.h
new file mode 100644
index 0000000..5161f63
--- /dev/null
+++ b/include/linux/pl320-ipc.h
@@ -0,0 +1,17 @@
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+int pl320_ipc_transmit(u32 *data);
+int pl320_ipc_register_notifier(struct notifier_block *nb);
+int pl320_ipc_unregister_notifier(struct notifier_block *nb);
-- 
1.8.1.2

[PATCHv3 2/6] mailbox: Introduce a new common API

[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>
---
 drivers/mailbox/Makefile           |   4 +
 drivers/mailbox/mailbox.c          | 534 +++++++++++++++++++++++++++++++++++++
 include/linux/mailbox.h            |  17 ++
 include/linux/mailbox_client.h     |  87 ++++++
 include/linux/mailbox_controller.h | 102 +++++++
 5 files changed, 744 insertions(+)
 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/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..3082f08
--- /dev/null
+++ b/drivers/mailbox/mailbox.c
@@ -0,0 +1,534 @@
+/*
+ * 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/mailbox_client.h>
+#include <linux/mailbox_controller.h>
+
+/*
+ * 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, ipc_send_message could be called from atomic context and
+ * the client could also queue another message from the notifier 'txcb'
+ * 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
+
+#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 */
+
+struct ipc_chan {
+	char name[16]; /* link_name */
+	struct ipc_con *con; /* Parent Controller */
+	unsigned txdone_method;
+
+	/* Cached values from controller */
+	struct ipc_link *link;
+	struct ipc_link_ops *link_ops;
+
+	/* Cached values from client */
+	void *cl_id;
+	void (*rxcb)(void *cl_id, void *mssg);
+	void (*txcb)(void *cl_id, void *mssg, enum xfer_result r);
+	bool tx_block;
+	unsigned long tx_tout;
+	struct completion tx_complete;
+
+	void *active_req;
+	unsigned msg_count, msg_free;
+	void *msg_data[MBOX_TX_QUEUE_LEN];
+	bool assigned;
+	/* Serialize access to the channel */
+	spinlock_t lock;
+	/* Hook to add to the controller's list of channels */
+	struct list_head node;
+	/* Notifier to all clients waiting on aquiring this channel */
+	struct blocking_notifier_head avail;
+};
+
+/* Internal representation of a controller */
+struct ipc_con {
+	char name[16]; /* controller_name */
+	struct list_head channels;
+	/*
+	 * 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;
+};
+
+static LIST_HEAD(ipc_cons);
+static DEFINE_MUTEX(con_mutex);
+
+static request_token_t _add_to_rbuf(struct ipc_chan *chan, void *mssg)
+{
+	request_token_t 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 0;
+	}
+
+	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);
+
+	/* To aid debugging, we return 'idx+1' instead of 1 */
+	return idx + 1;
+}
+
+static void _msg_submit(struct ipc_chan *chan)
+{
+	struct ipc_link *link = chan->link;
+	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 IPC controller */
+	err = chan->link_ops->send_data(link, data);
+	if (!err) {
+		chan->active_req = data;
+		chan->msg_count--;
+	}
+
+	spin_unlock_irqrestore(&chan->lock, flags);
+}
+
+static void tx_tick(struct ipc_chan *chan, enum xfer_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->tx_block)
+		complete(&chan->tx_complete);
+	else if (mssg && chan->txcb)
+		chan->txcb(chan->cl_id, mssg, r);
+}
+
+static void poll_txdone(unsigned long data)
+{
+	struct ipc_con *con = (struct ipc_con *)data;
+	bool txdone, resched = false;
+	struct ipc_chan *chan;
+
+	list_for_each_entry(chan, &con->channels, node) {
+		if (chan->active_req && chan->assigned) {
+			resched = true;
+			txdone = chan->link_ops->last_tx_done(chan->link);
+			if (txdone)
+				tx_tick(chan, XFER_OK);
+		}
+	}
+
+	if (resched)
+		mod_timer(&con->poll,
+			jiffies + msecs_to_jiffies(con->period));
+}
+
+/*
+ * After 'startup' and before 'shutdown', the IPC controller driver
+ * notifies the API of data received over the link.
+ * The controller driver should make sure the 'RTR' is de-asserted since
+ * reception of the packet and until after this call returns.
+ * This call could be made from atomic context.
+ */
+void ipc_link_received_data(struct ipc_link *link, void *mssg)
+{
+	struct ipc_chan *chan = (struct ipc_chan *)link->api_priv;
+
+	/* No buffering the received data */
+	if (chan->rxcb)
+		chan->rxcb(chan->cl_id, mssg);
+}
+EXPORT_SYMBOL(ipc_link_received_data);
+
+/*
+ * The IPC controller driver notifies the API that the remote has
+ * asserted RTR and it could now send another message on the link.
+ */
+void ipc_link_txdone(struct ipc_link *link, enum xfer_result r)
+{
+	struct ipc_chan *chan = (struct ipc_chan *)link->api_priv;
+
+	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(ipc_link_txdone);
+
+/*
+ * 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 doesn't get IRQ for TX done.
+ */
+void ipc_client_txdone(void *channel, enum xfer_result r)
+{
+	struct ipc_chan *chan = (struct ipc_chan *)channel;
+	bool txdone = true;
+
+	if (unlikely(!(chan->txdone_method & TXDONE_BY_ACK))) {
+		pr_err("Client can't run the TX ticker\n");
+		return;
+	}
+
+	if (chan->txdone_method & TXDONE_BY_POLL)
+		txdone = chan->link_ops->last_tx_done(chan->link);
+
+	if (txdone)
+		tx_tick(chan, r);
+}
+EXPORT_SYMBOL(ipc_client_txdone);
+
+/*
+ * Called by a client to "put data on the h/w channel" so that if
+ * everything else is fine we don't need to do anything more locally
+ * for the remote to receive the data intact.
+ * In reality, the remote may receive it intact, corrupted or not at all.
+ * This could be called from atomic context as it simply
+ * queues the data and returns a token (request_token_t)
+ * against the request.
+ * The client is later notified of successful transmission of
+ * data over the channel via the 'txcb'. The client could in
+ * turn queue more messages from txcb.
+ */
+request_token_t ipc_send_message(void *channel, void *mssg)
+{
+	struct ipc_chan *chan = (struct ipc_chan *)channel;
+	request_token_t t;
+
+	if (!chan || !chan->assigned)
+		return 0;
+
+	t = _add_to_rbuf(chan, mssg);
+	if (!t)
+		pr_err("Try increasing MBOX_TX_QUEUE_LEN\n");
+
+	_msg_submit(chan);
+
+	if (chan->txdone_method	== TXDONE_BY_POLL)
+		poll_txdone((unsigned long)chan->con);
+
+	if (chan->tx_block && chan->active_req) {
+		int ret;
+		init_completion(&chan->tx_complete);
+		ret = wait_for_completion_timeout(&chan->tx_complete,
+			chan->tx_tout);
+		if (ret == 0) {
+			t = 0;
+			tx_tick(chan, XFER_ERR);
+		}
+	}
+
+	return t;
+}
+EXPORT_SYMBOL(ipc_send_message);
+
+/*
+ * A client driver asks for exclusive use of a channel/mailbox.
+ * If assigned, the channel has to be 'freed' before it could
+ * be assigned to some other client.
+ * After assignment, any packet received on this channel will be
+ * handed over to the client via the 'rxcb' callback.
+ * The 'txcb' callback is used to notify client upon sending the
+ * packet over the channel, which may or may not have been yet
+ * read by the remote processor.
+ */
+void *ipc_request_channel(struct ipc_client *cl)
+{
+	struct ipc_chan *chan;
+	struct ipc_con *con;
+	unsigned long flags;
+	char *con_name;
+	int len, ret;
+
+	con_name = cl->chan_name;
+	len = strcspn(cl->chan_name, ":");
+
+	ret = 0;
+	mutex_lock(&con_mutex);
+	list_for_each_entry(con, &ipc_cons, node)
+		if (!strncmp(con->name, con_name, len)) {
+			ret = 1;
+			break;
+		}
+	mutex_unlock(&con_mutex);
+
+	if (!ret) {
+		pr_info("Channel(%s) not found!\n", cl->chan_name);
+		return NULL;
+	}
+
+	ret = 0;
+	list_for_each_entry(chan, &con->channels, node) {
+		if (!strcmp(con_name + len + 1, chan->name)
+				&& !chan->assigned) {
+			spin_lock_irqsave(&chan->lock, flags);
+			chan->msg_free = 0;
+			chan->msg_count = 0;
+			chan->active_req = NULL;
+			chan->rxcb = cl->rxcb;
+			chan->txcb = cl->txcb;
+			chan->cl_id = cl->cl_id;
+			chan->assigned = true;
+			chan->tx_block = cl->tx_block;
+			if (!cl->tx_tout)
+				chan->tx_tout = msecs_to_jiffies(3600000);
+			else
+				chan->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 = 1;
+			break;
+		}
+	}
+
+	if (!ret) {
+		pr_err("Unable to assign mailbox(%s)\n", cl->chan_name);
+		return NULL;
+	}
+
+	ret = chan->link_ops->startup(chan->link, cl->link_data);
+	if (ret) {
+		pr_err("Unable to startup the link\n");
+		ipc_free_channel((void *)chan);
+		return NULL;
+	}
+
+	return (void *)chan;
+}
+EXPORT_SYMBOL(ipc_request_channel);
+
+/* Drop any messages queued and release the channel */
+void ipc_free_channel(void *ch)
+{
+	struct ipc_chan *chan = (struct ipc_chan *)ch;
+	unsigned long flags;
+
+	if (!chan || !chan->assigned)
+		return;
+
+	chan->link_ops->shutdown(chan->link);
+
+	/* The queued TX requests are simply aborted, no callbacks are made */
+	spin_lock_irqsave(&chan->lock, flags);
+	chan->assigned = false;
+	chan->active_req = NULL;
+	if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
+		chan->txdone_method = TXDONE_BY_POLL;
+	spin_unlock_irqrestore(&chan->lock, flags);
+
+	blocking_notifier_call_chain(&chan->avail, 0, NULL);
+}
+EXPORT_SYMBOL(ipc_free_channel);
+
+static struct ipc_chan *name_to_chan(const char *name)
+{
+	struct ipc_chan *chan = NULL;
+	struct ipc_con *con;
+	int len, found = 0;
+
+	len = strcspn(name, ":");
+
+	mutex_lock(&con_mutex);
+
+	list_for_each_entry(con, &ipc_cons, node) {
+		if (!strncmp(con->name, name, len)) {
+			list_for_each_entry(chan, &con->channels, node) {
+				if (!strcmp(name + len + 1, chan->name)) {
+					found = 1;
+					goto done;
+				}
+			}
+		}
+	}
+done:
+	mutex_unlock(&con_mutex);
+
+	if (!found)
+		return NULL;
+
+	return chan;
+}
+
+int ipc_notify_chan_register(const char *name, struct notifier_block *nb)
+{
+	struct ipc_chan *chan = name_to_chan(name);
+
+	if (chan && nb)
+		return blocking_notifier_chain_register(&chan->avail, nb);
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(ipc_notify_chan_register);
+
+void ipc_notify_chan_unregister(const char *name, struct notifier_block *nb)
+{
+	struct ipc_chan *chan = name_to_chan(name);
+
+	if (chan && nb)
+		blocking_notifier_chain_unregister(&chan->avail, nb);
+}
+EXPORT_SYMBOL(ipc_notify_chan_unregister);
+
+/*
+ * Call for IPC controller drivers to register a controller, adding
+ * its channels/mailboxes to the global pool.
+ */
+int ipc_links_register(struct ipc_controller *ipc)
+{
+	int i, num_links, txdone;
+	struct ipc_chan *chan;
+	struct ipc_con *con;
+
+	/* Sanity check */
+	if (!ipc || !ipc->ops)
+		return -EINVAL;
+
+	for (i = 0; ipc->links[i]; i++)
+		;
+	if (!i)
+		return -EINVAL;
+	num_links = i;
+
+	mutex_lock(&con_mutex);
+	/* Check if already populated */
+	list_for_each_entry(con, &ipc_cons, node)
+		if (!strcmp(ipc->controller_name, con->name)) {
+			mutex_unlock(&con_mutex);
+			return -EINVAL;
+		}
+	mutex_unlock(&con_mutex);
+
+	con = kzalloc(sizeof(*con) + sizeof(*chan) * num_links, GFP_KERNEL);
+	if (!con)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&con->channels);
+	snprintf(con->name, 16, "%s", ipc->controller_name);
+
+	if (ipc->txdone_irq)
+		txdone = TXDONE_BY_IRQ;
+	else if (ipc->txdone_poll)
+		txdone = TXDONE_BY_POLL;
+	else /* It has to be ACK then */
+		txdone = TXDONE_BY_ACK;
+
+	if (txdone == TXDONE_BY_POLL) {
+		con->period = ipc->txpoll_period;
+		con->poll.function = &poll_txdone;
+		con->poll.data = (unsigned long)con;
+		init_timer(&con->poll);
+	}
+
+	chan = (void *)con + sizeof(*con);
+	for (i = 0; i < num_links; i++) {
+		chan[i].con = con;
+		chan[i].assigned = false;
+		chan[i].link_ops = ipc->ops;
+		chan[i].link = ipc->links[i];
+		chan[i].txdone_method = txdone;
+		chan[i].link->api_priv = &chan[i];
+		spin_lock_init(&chan[i].lock);
+		BLOCKING_INIT_NOTIFIER_HEAD(&chan[i].avail);
+		list_add_tail(&chan[i].node, &con->channels);
+		snprintf(chan[i].name, 16, "%s", ipc->links[i]->link_name);
+	}
+
+	mutex_lock(&con_mutex);
+	list_add_tail(&con->node, &ipc_cons);
+	mutex_unlock(&con_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL(ipc_links_register);
+
+void ipc_links_unregister(struct ipc_controller *ipc)
+{
+	struct ipc_con *t, *con = NULL;
+	struct ipc_chan *chan;
+
+	mutex_lock(&con_mutex);
+
+	list_for_each_entry(t, &ipc_cons, node)
+		if (!strcmp(ipc->controller_name, t->name)) {
+			con = t;
+			break;
+		}
+
+	if (con)
+		list_del(&con->node);
+
+	mutex_unlock(&con_mutex);
+
+	if (!con)
+		return;
+
+	list_for_each_entry(chan, &con->channels, node)
+		ipc_free_channel((void *)chan);
+
+	del_timer_sync(&con->poll);
+
+	kfree(con);
+}
+EXPORT_SYMBOL(ipc_links_unregister);
diff --git a/include/linux/mailbox.h b/include/linux/mailbox.h
new file mode 100644
index 0000000..232e2c4
--- /dev/null
+++ b/include/linux/mailbox.h
@@ -0,0 +1,17 @@
+/*
+ * 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
+
+enum xfer_result {
+	XFER_OK = 0,
+	XFER_ERR,
+};
+
+typedef unsigned request_token_t;
+
+#endif /* __MAILBOX_H */
diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
new file mode 100644
index 0000000..c43f2c7
--- /dev/null
+++ b/include/linux/mailbox_client.h
@@ -0,0 +1,87 @@
+/*
+ * 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 ipc_client - User of a mailbox
+ * @chan_name: the "controller:channel" this client wants
+ * @cl_id: The identity to associate any tx/rx data with
+ * @rxcb: atomic callback to provide client the data received
+ * @txcb: atomic callback to tell client of data transmission
+ * @tx_block: if the ipc_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.
+ * @link_data: Optional controller specific parameters during channel request
+ */
+struct ipc_client {
+	char *chan_name;
+	void *cl_id;
+	void (*rxcb)(void *cl_id, void *mssg);
+	void (*txcb)(void *cl_id, void *mssg, enum xfer_result r);
+	bool tx_block;
+	unsigned long tx_tout;
+	bool knows_txdone;
+	void *link_data;
+};
+
+/**
+ * The Client specifies its requirements and capabilities while asking for
+ * a channel/mailbox 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 ipc_free_channel.
+ */
+void *ipc_request_channel(struct ipc_client *cl);
+
+/**
+ * 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-zero token is returned for each queued request. If the queue
+ * was full the returned token will be 0. Upon failure or successful
+ * TX, the API calls 'txcb' from atomic context, from which the client
+ * could submit yet another request.
+ *  In blocking mode, 'txcb' is not called, effectively making the
+ * queue length 1. The returned value is 0 if TX timed out, some
+ * non-zero value upon success.
+ */
+request_token_t ipc_send_message(void *channel, void *mssg);
+
+/**
+ * The way for a client to run the TX state machine. This works
+ * only if the client sets 'knows_txdone' and the IPC controller
+ * doesn't get an IRQ for TX_Done/Remote_RTR.
+ */
+void ipc_client_txdone(void *channel, enum xfer_result r);
+
+/**
+ * The client relinquishes control of a mailbox by this call,
+ * make it available to other clients.
+ * The ipc_request/free_channel are light weight calls, so the
+ * client should avoid holding it when it doesn't need to
+ * transfer data.
+ */
+void ipc_free_channel(void *channel);
+
+/**
+ * The client make ask the API to be notified when a particular channel
+ * becomes available to be acquired again.
+ */
+int ipc_notify_chan_register(const char *name, struct notifier_block *nb);
+
+/**
+ * The client is no more interested in acquiring the channel.
+ */
+void ipc_notify_chan_unregister(const char *name, struct notifier_block *nb);
+
+#endif /* __MAILBOX_CLIENT_H */
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
new file mode 100644
index 0000000..a907467
--- /dev/null
+++ b/include/linux/mailbox_controller.h
@@ -0,0 +1,102 @@
+/*
+ * 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 ipc_link - s/w representation of a communication link
+ * @link_name: Literal name assigned to the link. Physically
+ *    identical channels may have the same name.
+ * @api_priv: hook for the API to map its private data on the link
+ *    Controller driver must not touch it.
+ */
+struct ipc_link {
+	char link_name[16];
+	void *api_priv;
+};
+
+/**
+ * struct ipc_link - s/w representation of a communication link
+ * @send_data: The API asks the IPC 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
+ *    ipc_link_txdone (if it has some TX ACK irq). It must not
+ *    block.
+ * @startup: Called when a client requests the link. The controller
+ *    could ask clients for additional parameters of communication
+ *    to be provided via client's link_data. This call may block.
+ *    After this call the Controller must forward any data received
+ *    on the link by calling ipc_link_received_data (which won't block)
+ * @shutdown: Called when a client relinquishes control of a link.
+ *    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
+ */
+struct ipc_link_ops {
+	int (*send_data)(struct ipc_link *link, void *data);
+	int (*startup)(struct ipc_link *link, void *params);
+	void (*shutdown)(struct ipc_link *link);
+	bool (*last_tx_done)(struct ipc_link *link);
+};
+
+/**
+ * struct ipc_controller - Controller of a class of communication links
+ * @controller_name: Literal name of the controller.
+ * @ops: Operators that work on each communication link
+ * @links: Null terminated array of links.
+ * @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.
+ *    Eg, is 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 ipc_controller {
+	char controller_name[16];
+	struct ipc_link_ops *ops;
+	struct ipc_link **links;
+	bool txdone_irq;
+	bool txdone_poll;
+	unsigned txpoll_period;
+};
+
+/**
+ * The controller driver registers its communication links to the
+ * global pool managed by the API.
+ */
+int ipc_links_register(struct ipc_controller *ipc_con);
+
+/**
+ * After startup and before shutdown any data received on the link
+ * is pused to the API via atomic ipc_link_received_data() API.
+ * The controller should ACK the RX only after this call returns.
+ */
+void ipc_link_received_data(struct ipc_link *link, void *data);
+
+/**
+ * The controller the 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 ipc_link_txdone(struct ipc_link *link, enum xfer_result r);
+
+/**
+ * Purge the links from the global pool maintained by the API.
+ */
+void ipc_links_unregister(struct ipc_controller *ipc_con);
+
+#endif /* __MAILBOX_CONTROLLER_H */
-- 
1.8.1.2

[PATCHv3 3/6] mailbox: pl320: Introduce common API driver

[Jassi Brar]

Convert the PL320 controller driver to work with the common
mailbox API. Also convert the only user of PL320, highbank-cpufreq.c
to work with thee API. Drop the obsoleted driver pl320-ipc.c

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 drivers/cpufreq/highbank-cpufreq.c |  22 +++-
 drivers/mailbox/Makefile           |   2 +-
 drivers/mailbox/pl320-ipc.c        | 198 ----------------------------------
 drivers/mailbox/pl320.c            | 212 +++++++++++++++++++++++++++++++++++++
 include/linux/pl320-ipc.h          |  17 ---
 5 files changed, 233 insertions(+), 218 deletions(-)
 delete mode 100644 drivers/mailbox/pl320-ipc.c
 create mode 100644 drivers/mailbox/pl320.c
 delete mode 100644 include/linux/pl320-ipc.h

diff --git a/drivers/cpufreq/highbank-cpufreq.c b/drivers/cpufreq/highbank-cpufreq.c
index b464f29..b1a6b2e 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/pl320-ipc.h>
+#include <linux/mailbox_client.h>
 #include <linux/platform_device.h>
 
 #define HB_CPUFREQ_CHANGE_NOTE	0x80000001
@@ -29,8 +29,26 @@
 static int hb_voltage_change(unsigned int freq)
 {
 	u32 msg[HB_CPUFREQ_IPC_LEN] = {HB_CPUFREQ_CHANGE_NOTE, freq / 1000000};
+	struct ipc_client cl;
+	int ret = -ETIMEDOUT;
+	void *chan;
 
-	return pl320_ipc_transmit(msg);
+	cl.rxcb = NULL;
+	cl.txcb = NULL;
+	cl.tx_block = true;
+	cl.tx_tout = 1000; /* 1 sec */
+	cl.link_data = NULL;
+	cl.knows_txdone = false;
+	cl.chan_name = "pl320:A9_to_M3";
+
+	chan = ipc_request_channel(&cl);
+
+	if (ipc_send_message(chan, (void *)msg))
+		ret = msg[1]; /* PL320 updates buffer with FIFO after ACK */
+
+	ipc_free_channel(chan);
+
+	return ret;
 }
 
 static int hb_cpufreq_clk_notify(struct notifier_block *nb,
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 2fa343a..f3f4ab7 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -2,7 +2,7 @@
 
 obj-$(CONFIG_MAILBOX)		+= mailbox.o
 
-obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
+obj-$(CONFIG_PL320_MBOX)	+= pl320.o
 
 obj-$(CONFIG_OMAP_MBOX)		+= omap-mailbox.o
 obj-$(CONFIG_OMAP1_MBOX)	+= mailbox_omap1.o
diff --git a/drivers/mailbox/pl320-ipc.c b/drivers/mailbox/pl320-ipc.c
deleted file mode 100644
index f3755e0..0000000
--- a/drivers/mailbox/pl320-ipc.c
+++ /dev/null
@@ -1,198 +0,0 @@
-/*
- * Copyright 2012 Calxeda, Inc.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-#include <linux/types.h>
-#include <linux/err.h>
-#include <linux/delay.h>
-#include <linux/export.h>
-#include <linux/io.h>
-#include <linux/interrupt.h>
-#include <linux/completion.h>
-#include <linux/mutex.h>
-#include <linux/notifier.h>
-#include <linux/spinlock.h>
-#include <linux/device.h>
-#include <linux/amba/bus.h>
-
-#include <linux/pl320-ipc.h>
-
-#define IPCMxSOURCE(m)		((m) * 0x40)
-#define IPCMxDSET(m)		(((m) * 0x40) + 0x004)
-#define IPCMxDCLEAR(m)		(((m) * 0x40) + 0x008)
-#define IPCMxDSTATUS(m)		(((m) * 0x40) + 0x00C)
-#define IPCMxMODE(m)		(((m) * 0x40) + 0x010)
-#define IPCMxMSET(m)		(((m) * 0x40) + 0x014)
-#define IPCMxMCLEAR(m)		(((m) * 0x40) + 0x018)
-#define IPCMxMSTATUS(m)		(((m) * 0x40) + 0x01C)
-#define IPCMxSEND(m)		(((m) * 0x40) + 0x020)
-#define IPCMxDR(m, dr)		(((m) * 0x40) + ((dr) * 4) + 0x024)
-
-#define IPCMMIS(irq)		(((irq) * 8) + 0x800)
-#define IPCMRIS(irq)		(((irq) * 8) + 0x804)
-
-#define MBOX_MASK(n)		(1 << (n))
-#define IPC_TX_MBOX		1
-#define IPC_RX_MBOX		2
-
-#define CHAN_MASK(n)		(1 << (n))
-#define A9_SOURCE		1
-#define M3_SOURCE		0
-
-static void __iomem *ipc_base;
-static int ipc_irq;
-static DEFINE_MUTEX(ipc_m1_lock);
-static DECLARE_COMPLETION(ipc_completion);
-static ATOMIC_NOTIFIER_HEAD(ipc_notifier);
-
-static inline void set_destination(int source, int mbox)
-{
-	__raw_writel(CHAN_MASK(source), ipc_base + IPCMxDSET(mbox));
-	__raw_writel(CHAN_MASK(source), ipc_base + IPCMxMSET(mbox));
-}
-
-static inline void clear_destination(int source, int mbox)
-{
-	__raw_writel(CHAN_MASK(source), ipc_base + IPCMxDCLEAR(mbox));
-	__raw_writel(CHAN_MASK(source), ipc_base + IPCMxMCLEAR(mbox));
-}
-
-static void __ipc_send(int mbox, u32 *data)
-{
-	int i;
-	for (i = 0; i < 7; i++)
-		__raw_writel(data[i], ipc_base + IPCMxDR(mbox, i));
-	__raw_writel(0x1, ipc_base + IPCMxSEND(mbox));
-}
-
-static u32 __ipc_rcv(int mbox, u32 *data)
-{
-	int i;
-	for (i = 0; i < 7; i++)
-		data[i] = __raw_readl(ipc_base + IPCMxDR(mbox, i));
-	return data[1];
-}
-
-/* blocking implmentation from the A9 side, not usuable in interrupts! */
-int pl320_ipc_transmit(u32 *data)
-{
-	int ret;
-
-	mutex_lock(&ipc_m1_lock);
-
-	init_completion(&ipc_completion);
-	__ipc_send(IPC_TX_MBOX, data);
-	ret = wait_for_completion_timeout(&ipc_completion,
-					  msecs_to_jiffies(1000));
-	if (ret == 0) {
-		ret = -ETIMEDOUT;
-		goto out;
-	}
-
-	ret = __ipc_rcv(IPC_TX_MBOX, data);
-out:
-	mutex_unlock(&ipc_m1_lock);
-	return ret;
-}
-EXPORT_SYMBOL_GPL(pl320_ipc_transmit);
-
-static irqreturn_t ipc_handler(int irq, void *dev)
-{
-	u32 irq_stat;
-	u32 data[7];
-
-	irq_stat = __raw_readl(ipc_base + IPCMMIS(1));
-	if (irq_stat & MBOX_MASK(IPC_TX_MBOX)) {
-		__raw_writel(0, ipc_base + IPCMxSEND(IPC_TX_MBOX));
-		complete(&ipc_completion);
-	}
-	if (irq_stat & MBOX_MASK(IPC_RX_MBOX)) {
-		__ipc_rcv(IPC_RX_MBOX, data);
-		atomic_notifier_call_chain(&ipc_notifier, data[0], data + 1);
-		__raw_writel(2, ipc_base + IPCMxSEND(IPC_RX_MBOX));
-	}
-
-	return IRQ_HANDLED;
-}
-
-int pl320_ipc_register_notifier(struct notifier_block *nb)
-{
-	return atomic_notifier_chain_register(&ipc_notifier, nb);
-}
-EXPORT_SYMBOL_GPL(pl320_ipc_register_notifier);
-
-int pl320_ipc_unregister_notifier(struct notifier_block *nb)
-{
-	return atomic_notifier_chain_unregister(&ipc_notifier, nb);
-}
-EXPORT_SYMBOL_GPL(pl320_ipc_unregister_notifier);
-
-static int pl320_probe(struct amba_device *adev, const struct amba_id *id)
-{
-	int ret;
-
-	ipc_base = ioremap(adev->res.start, resource_size(&adev->res));
-	if (ipc_base == NULL)
-		return -ENOMEM;
-
-	__raw_writel(0, ipc_base + IPCMxSEND(IPC_TX_MBOX));
-
-	ipc_irq = adev->irq[0];
-	ret = request_irq(ipc_irq, ipc_handler, 0, dev_name(&adev->dev), NULL);
-	if (ret < 0)
-		goto err;
-
-	/* Init slow mailbox */
-	__raw_writel(CHAN_MASK(A9_SOURCE),
-			ipc_base + IPCMxSOURCE(IPC_TX_MBOX));
-	__raw_writel(CHAN_MASK(M3_SOURCE),
-			ipc_base + IPCMxDSET(IPC_TX_MBOX));
-	__raw_writel(CHAN_MASK(M3_SOURCE) | CHAN_MASK(A9_SOURCE),
-		     ipc_base + IPCMxMSET(IPC_TX_MBOX));
-
-	/* Init receive mailbox */
-	__raw_writel(CHAN_MASK(M3_SOURCE),
-			ipc_base + IPCMxSOURCE(IPC_RX_MBOX));
-	__raw_writel(CHAN_MASK(A9_SOURCE),
-			ipc_base + IPCMxDSET(IPC_RX_MBOX));
-	__raw_writel(CHAN_MASK(M3_SOURCE) | CHAN_MASK(A9_SOURCE),
-		     ipc_base + IPCMxMSET(IPC_RX_MBOX));
-
-	return 0;
-err:
-	iounmap(ipc_base);
-	return ret;
-}
-
-static struct amba_id pl320_ids[] = {
-	{
-		.id	= 0x00041320,
-		.mask	= 0x000fffff,
-	},
-	{ 0, 0 },
-};
-
-static struct amba_driver pl320_driver = {
-	.drv = {
-		.name	= "pl320",
-	},
-	.id_table	= pl320_ids,
-	.probe		= pl320_probe,
-};
-
-static int __init ipc_init(void)
-{
-	return amba_driver_register(&pl320_driver);
-}
-module_init(ipc_init);
diff --git a/drivers/mailbox/pl320.c b/drivers/mailbox/pl320.c
new file mode 100644
index 0000000..7ddae5c
--- /dev/null
+++ b/drivers/mailbox/pl320.c
@@ -0,0 +1,212 @@
+/*
+ * Copyright 2012 Calxeda, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/types.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/export.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/completion.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/spinlock.h>
+#include <linux/device.h>
+#include <linux/amba/bus.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/mailbox_controller.h>
+
+#define IPCMSOURCE(m)		((m) * 0x40)
+#define IPCMDSET(m)		(((m) * 0x40) + 0x004)
+#define IPCMDCLEAR(m)		(((m) * 0x40) + 0x008)
+#define IPCMDSTATUS(m)		(((m) * 0x40) + 0x00C)
+#define IPCMMODE(m)		(((m) * 0x40) + 0x010)
+#define IPCMMSET(m)		(((m) * 0x40) + 0x014)
+#define IPCMMCLEAR(m)		(((m) * 0x40) + 0x018)
+#define IPCMMSTATUS(m)		(((m) * 0x40) + 0x01C)
+#define IPCMSEND(m)		(((m) * 0x40) + 0x020)
+#define IPCMDR(m, dr)		(((m) * 0x40) + ((dr) * 4) + 0x024)
+
+#define IPCMMIS(irq)		(((irq) * 8) + 0x800)
+#define IPCMRIS(irq)		(((irq) * 8) + 0x804)
+
+#define MBOX_MASK(n)		(1 << (n))
+#define IPC_TX_MBOX		1
+
+#define CHAN_MASK(n)		(1 << (n))
+#define A9_SOURCE		1
+#define M3_SOURCE		0
+
+struct pl320_con {
+	u32 *data;
+	int ipc_irq;
+	struct device *dev;
+	struct ipc_link link;
+	void __iomem *ipc_base;
+	struct ipc_controller ipc_con;
+};
+
+static inline struct pl320_con *to_pl320(struct ipc_link *l)
+{
+	if (!l)
+		return NULL;
+
+	return container_of(l, struct pl320_con, link);
+}
+
+static irqreturn_t ipc_handler(int irq, void *p)
+{
+	struct ipc_link *link = (struct ipc_link *)p;
+	struct pl320_con *pl320 = to_pl320(link);
+	void __iomem *ipc_base = pl320->ipc_base;
+	u32 irq_stat;
+
+	irq_stat = __raw_readl(ipc_base + IPCMMIS(1));
+	if (irq_stat & MBOX_MASK(IPC_TX_MBOX)) {
+		u32 *data = pl320->data;
+		int i;
+
+		__raw_writel(0, ipc_base + IPCMSEND(IPC_TX_MBOX));
+
+		/*
+		 * The PL320 driver specifies that the send buffer
+		 * will be overwritten by same fifo upon TX ACK.
+		 */
+		for (i = 0; i < 7; i++)
+			data[i] = __raw_readl(ipc_base
+					 + IPCMDR(IPC_TX_MBOX, i));
+
+		ipc_link_txdone(link, XFER_OK);
+
+		pl320->data = NULL;
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int pl320_send_data(struct ipc_link *link, void *msg)
+{
+	struct pl320_con *pl320 = to_pl320(link);
+	void __iomem *ipc_base = pl320->ipc_base;
+	u32 *data = (u32 *)msg;
+	int i;
+
+	pl320->data = data;
+
+	for (i = 0; i < 7; i++)
+		__raw_writel(data[i], ipc_base + IPCMDR(IPC_TX_MBOX, i));
+
+	__raw_writel(0x1, ipc_base + IPCMSEND(IPC_TX_MBOX));
+
+	return 0;
+}
+
+static int pl320_startup(struct ipc_link *link, void *ignored)
+{
+	struct pl320_con *pl320 = to_pl320(link);
+	void __iomem *ipc_base = pl320->ipc_base;
+	int err, ipc_irq = pl320->ipc_irq;
+
+	__raw_writel(0, ipc_base + IPCMSEND(IPC_TX_MBOX));
+
+	err = request_irq(ipc_irq, ipc_handler, 0, dev_name(pl320->dev), link);
+	if (err)
+		return err;
+
+	/* Init slow mailbox */
+	__raw_writel(CHAN_MASK(A9_SOURCE),
+			ipc_base + IPCMSOURCE(IPC_TX_MBOX));
+	__raw_writel(CHAN_MASK(M3_SOURCE),
+			ipc_base + IPCMDSET(IPC_TX_MBOX));
+	__raw_writel(CHAN_MASK(M3_SOURCE) | CHAN_MASK(A9_SOURCE),
+		     ipc_base + IPCMMSET(IPC_TX_MBOX));
+
+	pl320->data = NULL;
+	return 0;
+}
+
+static void pl320_shutdown(struct ipc_link *link)
+{
+	struct pl320_con *pl320 = to_pl320(link);
+
+	pl320->data = NULL;
+	free_irq(pl320->ipc_irq, link);
+}
+
+static struct ipc_link_ops pl320_ops = {
+	.send_data = pl320_send_data,
+	.startup = pl320_startup,
+	.shutdown = pl320_shutdown,
+};
+
+static int pl320_probe(struct amba_device *adev, const struct amba_id *id)
+{
+	struct pl320_con *pl320;
+	struct ipc_link *l[2];
+	int ret;
+
+	pl320 = kzalloc(sizeof(struct pl320_con), GFP_KERNEL);
+	if (!pl320)
+		return -ENOMEM;
+
+	pl320->ipc_base = ioremap(adev->res.start, resource_size(&adev->res));
+	if (pl320->ipc_base == NULL) {
+		kfree(pl320);
+		return -ENOMEM;
+	}
+
+	pl320->dev = &adev->dev;
+	pl320->ipc_irq = adev->irq[0];
+	amba_set_drvdata(adev, pl320);
+
+	l[0] = &pl320->link;
+	l[1] = NULL;
+	pl320->ipc_con.links = l;
+	pl320->ipc_con.txdone_irq = true;
+	pl320->ipc_con.ops = &pl320_ops;
+	snprintf(pl320->link.link_name, 16, "A9_to_M3");
+	snprintf(pl320->ipc_con.controller_name, 16, "pl320");
+
+	ret = ipc_links_register(&pl320->ipc_con);
+	if (ret) {
+		iounmap(pl320->ipc_base);
+		kfree(pl320);
+	}
+
+	return ret;
+}
+
+static struct amba_id pl320_ids[] = {
+	{
+		.id	= 0x00041320,
+		.mask	= 0x000fffff,
+	},
+	{ 0, 0 },
+};
+
+static struct amba_driver pl320_driver = {
+	.drv = {
+		.name	= "pl320",
+	},
+	.id_table	= pl320_ids,
+	.probe		= pl320_probe,
+};
+
+static int __init ipc_init(void)
+{
+	return amba_driver_register(&pl320_driver);
+}
+module_init(ipc_init);
diff --git a/include/linux/pl320-ipc.h b/include/linux/pl320-ipc.h
deleted file mode 100644
index 5161f63..0000000
--- a/include/linux/pl320-ipc.h
+++ /dev/null
@@ -1,17 +0,0 @@
-/*
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-int pl320_ipc_transmit(u32 *data);
-int pl320_ipc_register_notifier(struct notifier_block *nb);
-int pl320_ipc_unregister_notifier(struct notifier_block *nb);
-- 
1.8.1.2

[PATCHv3 4/6] 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
 ipc_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 3082f08..f8b31da 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -258,6 +258,9 @@ request_token_t ipc_send_message(void *channel, void *mssg)
 	if (!chan || !chan->assigned)
 		return 0;
 
+	if (chan->tx_block)
+		init_completion(&chan->tx_complete);
+
 	t = _add_to_rbuf(chan, mssg);
 	if (!t)
 		pr_err("Try increasing MBOX_TX_QUEUE_LEN\n");
@@ -269,7 +272,6 @@ request_token_t ipc_send_message(void *channel, void *mssg)
 
 	if (chan->tx_block && chan->active_req) {
 		int ret;
-		init_completion(&chan->tx_complete);
 		ret = wait_for_completion_timeout(&chan->tx_complete,
 			chan->tx_tout);
 		if (ret == 0) {
-- 
1.8.1.2

[PATCHv3 5/6] 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 f8b31da..bac767e 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -529,7 +529,8 @@ void ipc_links_unregister(struct ipc_controller *ipc)
 	list_for_each_entry(chan, &con->channels, node)
 		ipc_free_channel((void *)chan);
 
-	del_timer_sync(&con->poll);
+	if (ipc->txdone_poll)
+		del_timer_sync(&con->poll);
 
 	kfree(con);
 }
-- 
1.8.1.2

[PATCHv3 6/6] mailbox: move the internal definitions into a private file

[Jassi Brar]

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

This is needed for extracting the omap_mbox. The OMAP mailbox
code has a need for exporting some pre-existing API to not
break the current clients.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/mailbox/mailbox.c          | 62 +-----------------------------
 drivers/mailbox/mailbox_internal.h | 77 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 61 deletions(-)
 create mode 100644 drivers/mailbox/mailbox_internal.h

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index bac767e..b0e7126 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -14,67 +14,7 @@
 #include <linux/mailbox_client.h>
 #include <linux/mailbox_controller.h>
 
-/*
- * 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, ipc_send_message could be called from atomic context and
- * the client could also queue another message from the notifier 'txcb'
- * 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
-
-#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 */
-
-struct ipc_chan {
-	char name[16]; /* link_name */
-	struct ipc_con *con; /* Parent Controller */
-	unsigned txdone_method;
-
-	/* Cached values from controller */
-	struct ipc_link *link;
-	struct ipc_link_ops *link_ops;
-
-	/* Cached values from client */
-	void *cl_id;
-	void (*rxcb)(void *cl_id, void *mssg);
-	void (*txcb)(void *cl_id, void *mssg, enum xfer_result r);
-	bool tx_block;
-	unsigned long tx_tout;
-	struct completion tx_complete;
-
-	void *active_req;
-	unsigned msg_count, msg_free;
-	void *msg_data[MBOX_TX_QUEUE_LEN];
-	bool assigned;
-	/* Serialize access to the channel */
-	spinlock_t lock;
-	/* Hook to add to the controller's list of channels */
-	struct list_head node;
-	/* Notifier to all clients waiting on aquiring this channel */
-	struct blocking_notifier_head avail;
-};
-
-/* Internal representation of a controller */
-struct ipc_con {
-	char name[16]; /* controller_name */
-	struct list_head channels;
-	/*
-	 * 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;
-};
+#include "mailbox_internal.h"
 
 static LIST_HEAD(ipc_cons);
 static DEFINE_MUTEX(con_mutex);
diff --git a/drivers/mailbox/mailbox_internal.h b/drivers/mailbox/mailbox_internal.h
new file mode 100644
index 0000000..a39dcb7
--- /dev/null
+++ b/drivers/mailbox/mailbox_internal.h
@@ -0,0 +1,77 @@
+/*
+ * mailbox: interprocessor communication module
+ *
+ * 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_INTERNAL_H
+#define MAILBOX_INTERNAL_H
+
+#include <linux/device.h>
+#include <linux/mailbox_controller.h>
+
+/*
+ * 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, ipc_send_message could be called from atomic context and
+ * the client could also queue another message from the notifier 'txcb'
+ * 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
+
+#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 */
+
+struct ipc_chan {
+	char name[16]; /* link_name */
+	struct ipc_con *con; /* Parent Controller */
+	unsigned txdone_method;
+
+	/* Cached values from controller */
+	struct ipc_link *link;
+	struct ipc_link_ops *link_ops;
+
+	/* Cached values from client */
+	void *cl_id;
+	void (*rxcb)(void *cl_id, void *mssg);
+	void (*txcb)(void *cl_id, void *mssg, enum xfer_result r);
+	bool tx_block;
+	unsigned long tx_tout;
+	struct completion tx_complete;
+
+	void *active_req;
+	unsigned msg_count, msg_free;
+	void *msg_data[MBOX_TX_QUEUE_LEN];
+	bool assigned;
+	/* Serialize access to the channel */
+	spinlock_t lock;
+	/* Hook to add to the controller's list of channels */
+	struct list_head node;
+	/* Notifier to all clients waiting on aquiring this channel */
+	struct blocking_notifier_head avail;
+};
+
+/* Internal representation of a controller */
+struct ipc_con {
+	char name[16]; /* controller_name */
+	struct list_head channels;
+	/*
+	 * 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;
+};
+
+#endif /* MAILBOX_INTERNAL_H */
-- 
1.8.1.2

Re: [PATCHv3 2/6] mailbox: Introduce a new common API

[Greg KH]

On Sat, Feb 15, 2014 at 11:55:27PM +0530, 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>
> ---
>  drivers/mailbox/Makefile           |   4 +
>  drivers/mailbox/mailbox.c          | 534 +++++++++++++++++++++++++++++++++++++
>  include/linux/mailbox.h            |  17 ++
>  include/linux/mailbox_client.h     |  87 ++++++
>  include/linux/mailbox_controller.h | 102 +++++++
>  5 files changed, 744 insertions(+)
>  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

Did you forget a Kconfig file here?  How can this code be built?

Re: [PATCHv3 2/6] mailbox: Introduce a new common API

[Greg KH]

On Sat, Feb 15, 2014 at 11:55:27PM +0530, Jassi Brar wrote:
> +typedef unsigned request_token_t;

Ick.  Why add a new typedef?  And if you do need this, drop the "_t" on
the end please.

Why not just rely on an unsigned int?  Or long?  Do you really need a
new type?

thanks,

greg k-h

Re: [PATCHv3 2/6] mailbox: Introduce a new common API

[Greg KH]

On Sat, Feb 15, 2014 at 11:55:27PM +0530, Jassi Brar wrote:
> +/*
> + * After 'startup' and before 'shutdown', the IPC controller driver
> + * notifies the API of data received over the link.
> + * The controller driver should make sure the 'RTR' is de-asserted since
> + * reception of the packet and until after this call returns.
> + * This call could be made from atomic context.
> + */
> +void ipc_link_received_data(struct ipc_link *link, void *mssg)
> +{
> +	struct ipc_chan *chan = (struct ipc_chan *)link->api_priv;
> +
> +	/* No buffering the received data */
> +	if (chan->rxcb)
> +		chan->rxcb(chan->cl_id, mssg);
> +}
> +EXPORT_SYMBOL(ipc_link_received_data);

EXPORT_SYMBOL_GPL() on all of these perhaps?

Re: [PATCHv3 2/6] mailbox: Introduce a new common API

[Greg KH]

On Sat, Feb 15, 2014 at 11:55:27PM +0530, Jassi Brar wrote:
> +/*
> + * Call for IPC controller drivers to register a controller, adding
> + * its channels/mailboxes to the global pool.
> + */
> +int ipc_links_register(struct ipc_controller *ipc)
> +{
> +	int i, num_links, txdone;
> +	struct ipc_chan *chan;
> +	struct ipc_con *con;
> +
> +	/* Sanity check */
> +	if (!ipc || !ipc->ops)
> +		return -EINVAL;
> +
> +	for (i = 0; ipc->links[i]; i++)
> +		;
> +	if (!i)
> +		return -EINVAL;

So you have to have links?  You should document this in the function
definition.  Actually, why no kerneldoc for the public functions?

> +	num_links = i;
> +
> +	mutex_lock(&con_mutex);
> +	/* Check if already populated */
> +	list_for_each_entry(con, &ipc_cons, node)
> +		if (!strcmp(ipc->controller_name, con->name)) {
> +			mutex_unlock(&con_mutex);
> +			return -EINVAL;
> +		}
> +	mutex_unlock(&con_mutex);

Why drop the lock here?  Shouldn't you grab it for the whole function,
as this could race if two callers want to register the same name.

> +	con = kzalloc(sizeof(*con) + sizeof(*chan) * num_links, GFP_KERNEL);

Are you ok with structures on unaligned boundries?  That might really
slow down some processors if your pointers are unaligned...

> +	if (!con)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&con->channels);
> +	snprintf(con->name, 16, "%s", ipc->controller_name);

Magic name size :(

> +
> +	if (ipc->txdone_irq)
> +		txdone = TXDONE_BY_IRQ;
> +	else if (ipc->txdone_poll)
> +		txdone = TXDONE_BY_POLL;
> +	else /* It has to be ACK then */
> +		txdone = TXDONE_BY_ACK;
> +
> +	if (txdone == TXDONE_BY_POLL) {
> +		con->period = ipc->txpoll_period;
> +		con->poll.function = &poll_txdone;
> +		con->poll.data = (unsigned long)con;
> +		init_timer(&con->poll);
> +	}
> +
> +	chan = (void *)con + sizeof(*con);
> +	for (i = 0; i < num_links; i++) {
> +		chan[i].con = con;
> +		chan[i].assigned = false;
> +		chan[i].link_ops = ipc->ops;
> +		chan[i].link = ipc->links[i];
> +		chan[i].txdone_method = txdone;
> +		chan[i].link->api_priv = &chan[i];
> +		spin_lock_init(&chan[i].lock);
> +		BLOCKING_INIT_NOTIFIER_HEAD(&chan[i].avail);
> +		list_add_tail(&chan[i].node, &con->channels);
> +		snprintf(chan[i].name, 16, "%s", ipc->links[i]->link_name);

Magic name size :(

> +	}
> +
> +	mutex_lock(&con_mutex);
> +	list_add_tail(&con->node, &ipc_cons);
> +	mutex_unlock(&con_mutex);

You could have raced with above, please just grab the lock for the
whole call to be safe.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ipc_links_register);
> +
> +void ipc_links_unregister(struct ipc_controller *ipc)
> +{
> +	struct ipc_con *t, *con = NULL;
> +	struct ipc_chan *chan;
> +
> +	mutex_lock(&con_mutex);
> +
> +	list_for_each_entry(t, &ipc_cons, node)
> +		if (!strcmp(ipc->controller_name, t->name)) {
> +			con = t;
> +			break;
> +		}
> +
> +	if (con)
> +		list_del(&con->node);
> +
> +	mutex_unlock(&con_mutex);
> +
> +	if (!con)
> +		return;
> +
> +	list_for_each_entry(chan, &con->channels, node)
> +		ipc_free_channel((void *)chan);

Why does this function take a void *?  Shouldn't it take a "real"
structure pointer?

> +
> +	del_timer_sync(&con->poll);
> +
> +	kfree(con);
> +}
> +EXPORT_SYMBOL(ipc_links_unregister);

> +struct ipc_client {
> +	char *chan_name;
> +	void *cl_id;

Why a void *?  Can't you have a "real" type here?

> +	void (*rxcb)(void *cl_id, void *mssg);
> +	void (*txcb)(void *cl_id, void *mssg, enum xfer_result r);
> +	bool tx_block;
> +	unsigned long tx_tout;
> +	bool knows_txdone;
> +	void *link_data;
> +};
> +
> +/**
> + * The Client specifies its requirements and capabilities while asking for
> + * a channel/mailbox 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 ipc_free_channel.
> + */
> +void *ipc_request_channel(struct ipc_client *cl);

Can't you return a real type, and use it everywhere?  That's much
"safer" and nicer.  This isn't other operating systems that have void *
everywhere and handles, we have real types in Linux :)

thanks,

greg k-h

Re: [PATCHv3 6/6] mailbox: move the internal definitions into a private file

[Greg KH]

On Sat, Feb 15, 2014 at 11:57:02PM +0530, Jassi Brar wrote:
> From: Suman Anna <s-anna@ti.com>
> 
> This is needed for extracting the omap_mbox. The OMAP mailbox
> code has a need for exporting some pre-existing API to not
> break the current clients.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---

You don't agree with this patch and sign off on it?

>  drivers/mailbox/mailbox.c          | 62 +-----------------------------
>  drivers/mailbox/mailbox_internal.h | 77 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 78 insertions(+), 61 deletions(-)
>  create mode 100644 drivers/mailbox/mailbox_internal.h

Why not just do this in the first patch(s)?  Why wait for later in the
series to "fix" things up?

thanks,

greg k-h

Re: [PATCHv3 6/6] mailbox: move the internal definitions into a private file

[Greg KH]

On Sat, Feb 15, 2014 at 11:57:02PM +0530, Jassi Brar wrote:
> From: Suman Anna <s-anna@ti.com>
> 
> This is needed for extracting the omap_mbox. The OMAP mailbox
> code has a need for exporting some pre-existing API to not
> break the current clients.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
>  drivers/mailbox/mailbox.c          | 62 +-----------------------------
>  drivers/mailbox/mailbox_internal.h | 77 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 78 insertions(+), 61 deletions(-)
>  create mode 100644 drivers/mailbox/mailbox_internal.h

Also, this patch doesn't seem to be needed at all here, nothing breaks
without it, so why include it in this series?

thanks,

greg k-h

Re: [PATCHv3 2/6] mailbox: Introduce a new common API

[Jassi Brar]

[merging replies in one post]

On Sun, Feb 16, 2014 at 12:45 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sat, Feb 15, 2014 at 11:55:27PM +0530, Jassi Brar wrote:
>> +/*
>> + * Call for IPC controller drivers to register a controller, adding
>> + * its channels/mailboxes to the global pool.
>> + */
>> +int ipc_links_register(struct ipc_controller *ipc)
>> +{
>> +     int i, num_links, txdone;
>> +     struct ipc_chan *chan;
>> +     struct ipc_con *con;
>> +
>> +     /* Sanity check */
>> +     if (!ipc || !ipc->ops)
>> +             return -EINVAL;
>> +
>> +     for (i = 0; ipc->links[i]; i++)
>> +             ;
>> +     if (!i)
>> +             return -EINVAL;
>
> So you have to have links?  You should document this in the function
> definition.  Actually, why no kerneldoc for the public functions?
>
Yes a controller registers a bunch of links/mailboxes that can be used
by client drivers to send/recv messages. I'll add kerneldoc for the
api as well.

>> +     num_links = i;
>> +
>> +     mutex_lock(&con_mutex);
>> +     /* Check if already populated */
>> +     list_for_each_entry(con, &ipc_cons, node)
>> +             if (!strcmp(ipc->controller_name, con->name)) {
>> +                     mutex_unlock(&con_mutex);
>> +                     return -EINVAL;
>> +             }
>> +     mutex_unlock(&con_mutex);
>
> Why drop the lock here?  Shouldn't you grab it for the whole function,
> as this could race if two callers want to register the same name.
>
Yes, thanks.

>> +     con = kzalloc(sizeof(*con) + sizeof(*chan) * num_links, GFP_KERNEL);
>
> Are you ok with structures on unaligned boundries?  That might really
> slow down some processors if your pointers are unaligned...
>
OK, I'll align allocation.

>> +     if (!con)
>> +             return -ENOMEM;
>> +
>> +     INIT_LIST_HEAD(&con->channels);
>> +     snprintf(con->name, 16, "%s", ipc->controller_name);
>
> Magic name size :(
>
Yeah I know. I tried to keep the implementation simple.

>> +
>> +     if (ipc->txdone_irq)
>> +             txdone = TXDONE_BY_IRQ;
>> +     else if (ipc->txdone_poll)
>> +             txdone = TXDONE_BY_POLL;
>> +     else /* It has to be ACK then */
>> +             txdone = TXDONE_BY_ACK;
>> +
>> +     if (txdone == TXDONE_BY_POLL) {
>> +             con->period = ipc->txpoll_period;
>> +             con->poll.function = &poll_txdone;
>> +             con->poll.data = (unsigned long)con;
>> +             init_timer(&con->poll);
>> +     }
>> +
>> +     chan = (void *)con + sizeof(*con);
>> +     for (i = 0; i < num_links; i++) {
>> +             chan[i].con = con;
>> +             chan[i].assigned = false;
>> +             chan[i].link_ops = ipc->ops;
>> +             chan[i].link = ipc->links[i];
>> +             chan[i].txdone_method = txdone;
>> +             chan[i].link->api_priv = &chan[i];
>> +             spin_lock_init(&chan[i].lock);
>> +             BLOCKING_INIT_NOTIFIER_HEAD(&chan[i].avail);
>> +             list_add_tail(&chan[i].node, &con->channels);
>> +             snprintf(chan[i].name, 16, "%s", ipc->links[i]->link_name);
>
> Magic name size :(
>
"Controller:Link" specify the 32byte identity of a link.
I haven't yet opened the can of worms that is generic naming/identity
scheme like DMAEngine. Because the local controller and the remote f/w
together represents an entity that the local client driver deals
with.... so many common client drivers.are unlikely.

>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL(ipc_links_register);
>> +
>> +void ipc_links_unregister(struct ipc_controller *ipc)
>> +{
>> +     struct ipc_con *t, *con = NULL;
>> +     struct ipc_chan *chan;
>> +
>> +     mutex_lock(&con_mutex);
>> +
>> +     list_for_each_entry(t, &ipc_cons, node)
>> +             if (!strcmp(ipc->controller_name, t->name)) {
>> +                     con = t;
>> +                     break;
>> +             }
>> +
>> +     if (con)
>> +             list_del(&con->node);
>> +
>> +     mutex_unlock(&con_mutex);
>> +
>> +     if (!con)
>> +             return;
>> +
>> +     list_for_each_entry(chan, &con->channels, node)
>> +             ipc_free_channel((void *)chan);
>
> Why does this function take a void *?  Shouldn't it take a "real"
> structure pointer?
>
ipc_request/free_channel() is the api for client drivers. I have tried
to make the return channel opaque object to the clients and yet be
able to reuse the object within the api implementation. For that
reason we have mailbox_client.h and mailbox_controller.h so no side
can abuse what's on the other side. Only the api(mailbox.c) includes
both the headers.

>> +
>> +     del_timer_sync(&con->poll);
>> +
>> +     kfree(con);
>> +}
>> +EXPORT_SYMBOL(ipc_links_unregister);
>
>> +struct ipc_client {
>> +     char *chan_name;
>> +     void *cl_id;
>
> Why a void *?  Can't you have a "real" type here?
>
That is for client driver to specify how the controller driver is to
behave .... the api simply passes it on to the underlying controller
driver. We couldn't have defined some global real type because the
same controller behaves differently if the remote f/w changes.

>> +     void (*rxcb)(void *cl_id, void *mssg);
>> +     void (*txcb)(void *cl_id, void *mssg, enum xfer_result r);
>> +     bool tx_block;
>> +     unsigned long tx_tout;
>> +     bool knows_txdone;
>> +     void *link_data;
>> +};
>> +
>> +/**
>> + * The Client specifies its requirements and capabilities while asking for
>> + * a channel/mailbox 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 ipc_free_channel.
>> + */
>> +void *ipc_request_channel(struct ipc_client *cl);
>
> Can't you return a real type, and use it everywhere?  That's much
> "safer" and nicer.  This isn't other operating systems that have void *
> everywhere and handles, we have real types in Linux :)
>
As I said, we don't want the client driver to interpret the channel it
has been assigned. For clients a channel assigned is just an opaque
token that can't be used anyway other than request/release the channel
from the api.

> Did you forget a Kconfig file here?  How can this code be built?
Yup, sorry. The patch somehow got that change dropped.

>> +typedef unsigned request_token_t;
>
> Ick.  Why add a new typedef?  And if you do need this,
> drop the "_t" on the end please.
> Why not just rely on an unsigned int?  Or long?
> Do you really need a new type?
>
We can live without the typedef, it is only to impress that the cookie
returned is not to be used just like some unsigned... but an opaque object.

>> +EXPORT_SYMBOL(ipc_link_received_data);
>
> EXPORT_SYMBOL_GPL() on all of these perhaps?
>
Yes of course :)

Thanks,
Jassi

Re: [PATCHv3 6/6] mailbox: move the internal definitions into a private file

[Jassi Brar]

On Sun, Feb 16, 2014 at 12:46 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sat, Feb 15, 2014 at 11:57:02PM +0530, Jassi Brar wrote:
>> From: Suman Anna <s-anna@ti.com>
>>
>> This is needed for extracting the omap_mbox. The OMAP mailbox
>> code has a need for exporting some pre-existing API to not
>> break the current clients.
>>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>>  drivers/mailbox/mailbox.c          | 62 +-----------------------------
>>  drivers/mailbox/mailbox_internal.h | 77 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 78 insertions(+), 61 deletions(-)
>>  create mode 100644 drivers/mailbox/mailbox_internal.h
>
> Also, this patch doesn't seem to be needed at all here, nothing breaks
> without it, so why include it in this series?
>
Suman had to segregate the definitions temporarily while the TI
drivers were sorted out. I hope Suman says the patch isn't needed
anymore :)

Thanks,
Jassi

Re: [PATCHv3 2/6] mailbox: Introduce a new common API

[Greg KH]

On Sun, Feb 16, 2014 at 12:06:44PM +0530, Jassi Brar wrote:
> >> +void ipc_links_unregister(struct ipc_controller *ipc)
> >> +{
> >> +     struct ipc_con *t, *con = NULL;
> >> +     struct ipc_chan *chan;
> >> +
> >> +     mutex_lock(&con_mutex);
> >> +
> >> +     list_for_each_entry(t, &ipc_cons, node)
> >> +             if (!strcmp(ipc->controller_name, t->name)) {
> >> +                     con = t;
> >> +                     break;
> >> +             }
> >> +
> >> +     if (con)
> >> +             list_del(&con->node);
> >> +
> >> +     mutex_unlock(&con_mutex);
> >> +
> >> +     if (!con)
> >> +             return;
> >> +
> >> +     list_for_each_entry(chan, &con->channels, node)
> >> +             ipc_free_channel((void *)chan);
> >
> > Why does this function take a void *?  Shouldn't it take a "real"
> > structure pointer?
> >
> ipc_request/free_channel() is the api for client drivers. I have tried
> to make the return channel opaque object to the clients and yet be
> able to reuse the object within the api implementation. For that
> reason we have mailbox_client.h and mailbox_controller.h so no side
> can abuse what's on the other side. Only the api(mailbox.c) includes
> both the headers.

That's fine, then just "pre-declare" the structure you are going to be
using/calling it in the public header files:
	struct foo;
and then use that, not a void *, which is horrible.  We have a typesafe
language, use it :)

> >> +
> >> +     del_timer_sync(&con->poll);
> >> +
> >> +     kfree(con);
> >> +}
> >> +EXPORT_SYMBOL(ipc_links_unregister);
> >
> >> +struct ipc_client {
> >> +     char *chan_name;
> >> +     void *cl_id;
> >
> > Why a void *?  Can't you have a "real" type here?
> >
> That is for client driver to specify how the controller driver is to
> behave .... the api simply passes it on to the underlying controller
> driver. We couldn't have defined some global real type because the
> same controller behaves differently if the remote f/w changes.

Then call it something like "client_data", as that's more like what it
is, right?

> >> +     void (*rxcb)(void *cl_id, void *mssg);
> >> +     void (*txcb)(void *cl_id, void *mssg, enum xfer_result r);
> >> +     bool tx_block;
> >> +     unsigned long tx_tout;
> >> +     bool knows_txdone;
> >> +     void *link_data;
> >> +};
> >> +
> >> +/**
> >> + * The Client specifies its requirements and capabilities while asking for
> >> + * a channel/mailbox 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 ipc_free_channel.
> >> + */
> >> +void *ipc_request_channel(struct ipc_client *cl);
> >
> > Can't you return a real type, and use it everywhere?  That's much
> > "safer" and nicer.  This isn't other operating systems that have void *
> > everywhere and handles, we have real types in Linux :)
> >
> As I said, we don't want the client driver to interpret the channel it
> has been assigned. For clients a channel assigned is just an opaque
> token that can't be used anyway other than request/release the channel
> from the api.

See above for how to fix that.

> >> +typedef unsigned request_token_t;
> >
> > Ick.  Why add a new typedef?  And if you do need this,
> > drop the "_t" on the end please.
> > Why not just rely on an unsigned int?  Or long?
> > Do you really need a new type?
> >
> We can live without the typedef, it is only to impress that the cookie
> returned is not to be used just like some unsigned... but an opaque object.

Then make it a structure, not a typedef please.

thanks,

greg k-h

Re: [PATCHv3] Generic Mailbox API

[Craig McGeachie]

On 16/02/14 07:22, Jassi Brar wrote:
> Hello,
>
>    Here is what the generic mailbox api looks like after modifications
> to make it work with 5 different platforms.
>   Major feedback from Suman Anna(TI), LeyFoon Tan(Intel),
> Craig McGeachie(Broadcom) and Loic Pallardy(ST).
>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>

Important point of clarification.  I don't work for Broadcom.  I have no 
current or past affiliation with Broadcom.  I was fairly certain that 
I'd given no indication that I did.  I'm just some guy who purchased a 
single board computer based on one of their products.  I do work for an 
IT company, but that isn't relevant to what I do after hours on my own time.

Cheers,
Craig.

Re: [PATCHv3] Generic Mailbox API

[Jassi Brar]

On 17 February 2014 11:27, Craig McGeachie <slapdau@yahoo.com.au> wrote:
> On 16/02/14 07:22, Jassi Brar wrote:
>>
>> Hello,
>>
>>    Here is what the generic mailbox api looks like after modifications
>> to make it work with 5 different platforms.
>>   Major feedback from Suman Anna(TI), LeyFoon Tan(Intel),
>> Craig McGeachie(Broadcom) and Loic Pallardy(ST).
>>
>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>>
>
> Important point of clarification.  I don't work for Broadcom.  I have no
> current or past affiliation with Broadcom.  I was fairly certain that I'd
> given no indication that I did.  I'm just some guy who purchased a single
> board computer based on one of their products.  I do work for an IT company,
> but that isn't relevant to what I do after hours on my own time.
>
I saw the Broadcom driver being worked upon by you and assumed you
either work for or on contract with Broadcom. It doesn't matter much
anyways by virtue of the beauty of open-source :)
 So you still have a say on the mailbox code.

Thanks
Jassi

Re: [PATCHv3] Generic Mailbox API

[Craig McGeachie]

On 16/02/14 07:22, Jassi Brar wrote:
> Hello,
>
>    Here is what the generic mailbox api looks like after modifications
> to make it work with 5 different platforms.
>   Major feedback from Suman Anna(TI), LeyFoon Tan(Intel),
> Craig McGeachie(Broadcom) and Loic Pallardy(ST).
>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>

Um, sorry for not having included this in the previous email.  I've just 
looked over the patch set and I don't believe it contains anything that 
I did, so I don't think my name is appropriate here at all.

Cheers,
Craig.

Re: [PATCHv3] Generic Mailbox API

[Jassi Brar]

On Mon, Feb 17, 2014 at 11:33 AM, Craig McGeachie <slapdau@yahoo.com.au> wrote:
> On 16/02/14 07:22, Jassi Brar wrote:
>>
>> Hello,
>>
>>    Here is what the generic mailbox api looks like after modifications
>> to make it work with 5 different platforms.
>>   Major feedback from Suman Anna(TI), LeyFoon Tan(Intel),
>> Craig McGeachie(Broadcom) and Loic Pallardy(ST).
>>
>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>>
>
> Um, sorry for not having included this in the previous email.  I've just
> looked over the patch set and I don't believe it contains anything that I
> did, so I don't think my name is appropriate here at all.
>
I did not assign you copyright/ownership of any code. The patches say
who did what.
I just mentioned on the record that I got very useful feedback from
these guys to improve the api. I do not 'accuse' you of writing any
code ;)  but I am thankful that you in a way tested the api by writing
a driver for it.

Thanks,
Jassi

Re: [PATCHv3 2/6] mailbox: Introduce a new common API

[Courtney Cavin]

On Sat, Feb 15, 2014 at 07:25:27PM +0100, 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>
> ---
>  drivers/mailbox/Makefile           |   4 +
>  drivers/mailbox/mailbox.c          | 534 +++++++++++++++++++++++++++++++++++++
>  include/linux/mailbox.h            |  17 ++
>  include/linux/mailbox_client.h     |  87 ++++++
>  include/linux/mailbox_controller.h | 102 +++++++
>  5 files changed, 744 insertions(+)
>  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/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> new file mode 100644
> index 0000000..3082f08
> --- /dev/null
> +++ b/drivers/mailbox/mailbox.c
> @@ -0,0 +1,534 @@
> +/*

No copyright?

> + * 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.
> + */
[...]
> +request_token_t ipc_send_message(void *channel, void *mssg)
> +{
> +       struct ipc_chan *chan = (struct ipc_chan *)channel;
> +       request_token_t t;
> +
> +       if (!chan || !chan->assigned)
> +               return 0;

0 is not an error in my book.  Please make this more obvious, or define
something which categorizes this as an error.

> +
> +       t = _add_to_rbuf(chan, mssg);
> +       if (!t)
> +               pr_err("Try increasing MBOX_TX_QUEUE_LEN\n");
> +
> +       _msg_submit(chan);
> +
> +       if (chan->txdone_method == TXDONE_BY_POLL)
> +               poll_txdone((unsigned long)chan->con);
> +
> +       if (chan->tx_block && chan->active_req) {
> +               int ret;
> +               init_completion(&chan->tx_complete);
> +               ret = wait_for_completion_timeout(&chan->tx_complete,
> +                       chan->tx_tout);
> +               if (ret == 0) {
> +                       t = 0;
> +                       tx_tick(chan, XFER_ERR);
> +               }
> +       }
> +
> +       return t;
> +}
[...]
> +void *ipc_request_channel(struct ipc_client *cl)
> +{
> +       struct ipc_chan *chan;
> +       struct ipc_con *con;
> +       unsigned long flags;
> +       char *con_name;
> +       int len, ret;
> +
> +       con_name = cl->chan_name;
> +       len = strcspn(cl->chan_name, ":");
> +
> +       ret = 0;
> +       mutex_lock(&con_mutex);
> +       list_for_each_entry(con, &ipc_cons, node)
> +               if (!strncmp(con->name, con_name, len)) {
> +                       ret = 1;
> +                       break;
> +               }
> +       mutex_unlock(&con_mutex);
> +
> +       if (!ret) {
> +               pr_info("Channel(%s) not found!\n", cl->chan_name);
> +               return NULL;

ERR_PTR(-ENODEV) would be better.

> +       }
> +
> +       ret = 0;
> +       list_for_each_entry(chan, &con->channels, node) {
> +               if (!strcmp(con_name + len + 1, chan->name)
> +                               && !chan->assigned) {

Move the !chan->assigned check first, so we can skip the strcmp if not
needed.

> +                       spin_lock_irqsave(&chan->lock, flags);
> +                       chan->msg_free = 0;
> +                       chan->msg_count = 0;
> +                       chan->active_req = NULL;
> +                       chan->rxcb = cl->rxcb;
> +                       chan->txcb = cl->txcb;
> +                       chan->cl_id = cl->cl_id;
> +                       chan->assigned = true;
> +                       chan->tx_block = cl->tx_block;

Copying all of this data is unnecessary, and prone to error.  Just point
to 'cl' in the client struct.

> +                       if (!cl->tx_tout)
> +                               chan->tx_tout = msecs_to_jiffies(3600000);

An hour!?  Arbitrary, and extremely long.  Either this should be
indefinite, or there should be a reasonably small value here.

> +                       else
> +                               chan->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 = 1;
> +                       break;
> +               }
> +       }
> +
> +       if (!ret) {
> +               pr_err("Unable to assign mailbox(%s)\n", cl->chan_name);
> +               return NULL;

ERR_PTR(-EBUSY) if busy, ERR_PTR(-ENODEV) if unavailable.

> +       }
> +
> +       ret = chan->link_ops->startup(chan->link, cl->link_data);
> +       if (ret) {
> +               pr_err("Unable to startup the link\n");
> +               ipc_free_channel((void *)chan);
> +               return NULL;

ERR_PTR(ret);

> +       }
> +
> +       return (void *)chan;
> +}
[...]
> +int ipc_links_register(struct ipc_controller *ipc)
> +{
> +       int i, num_links, txdone;
> +       struct ipc_chan *chan;
> +       struct ipc_con *con;
> +
> +       /* Sanity check */
> +       if (!ipc || !ipc->ops)

You probably want to check for !ipc->links here too, as you immediately
dereference it.

> +               return -EINVAL;
> +
> +       for (i = 0; ipc->links[i]; i++)

So you require a NULL node at the end?  Why not have a nlinks/num_links
in the controller struct?  It would save having to count here.

> +               ;
> +       if (!i)
> +               return -EINVAL;
> +       num_links = i;
> +
> +       mutex_lock(&con_mutex);
> +       /* Check if already populated */
> +       list_for_each_entry(con, &ipc_cons, node)
> +               if (!strcmp(ipc->controller_name, con->name)) {
> +                       mutex_unlock(&con_mutex);
> +                       return -EINVAL;
> +               }
> +       mutex_unlock(&con_mutex);
> +
> +       con = kzalloc(sizeof(*con) + sizeof(*chan) * num_links, GFP_KERNEL);
> +       if (!con)
> +               return -ENOMEM;
> +
> +       INIT_LIST_HEAD(&con->channels);
> +       snprintf(con->name, 16, "%s", ipc->controller_name);
> +
> +       if (ipc->txdone_irq)
> +               txdone = TXDONE_BY_IRQ;
> +       else if (ipc->txdone_poll)
> +               txdone = TXDONE_BY_POLL;
> +       else /* It has to be ACK then */
> +               txdone = TXDONE_BY_ACK;
> +
> +       if (txdone == TXDONE_BY_POLL) {
> +               con->period = ipc->txpoll_period;
> +               con->poll.function = &poll_txdone;
> +               con->poll.data = (unsigned long)con;
> +               init_timer(&con->poll);
> +       }
> +
> +       chan = (void *)con + sizeof(*con);
> +       for (i = 0; i < num_links; i++) {
> +               chan[i].con = con;
> +               chan[i].assigned = false;
> +               chan[i].link_ops = ipc->ops;
> +               chan[i].link = ipc->links[i];
> +               chan[i].txdone_method = txdone;
> +               chan[i].link->api_priv = &chan[i];
> +               spin_lock_init(&chan[i].lock);
> +               BLOCKING_INIT_NOTIFIER_HEAD(&chan[i].avail);
> +               list_add_tail(&chan[i].node, &con->channels);

Why not have all of this data exposed in the link definition, so you
don't need this list, and can represent it as a pre-populated array?

> +               snprintf(chan[i].name, 16, "%s", ipc->links[i]->link_name);
> +       }
> +
> +       mutex_lock(&con_mutex);
> +       list_add_tail(&con->node, &ipc_cons);
> +       mutex_unlock(&con_mutex);
> +
> +       return 0;

You should have a try_module_get() somewhere in this function, and
module_put() somewhere in unregister.

> +}
[...]
> diff --git a/include/linux/mailbox.h b/include/linux/mailbox.h
> new file mode 100644
> index 0000000..232e2c4
> --- /dev/null
> +++ b/include/linux/mailbox.h
> @@ -0,0 +1,17 @@
> +/*
> + * 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
> +
> +enum xfer_result {
> +       XFER_OK = 0,
> +       XFER_ERR,
> +};

This is not the only framework which does 'xfer's, please add a prefix
to this enum and values.

> +
> +typedef unsigned request_token_t;
> +

Likewise, if needed.

> +#endif /* __MAILBOX_H */
> diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
> new file mode 100644
> index 0000000..c43f2c7
> --- /dev/null
> +++ b/include/linux/mailbox_client.h
> @@ -0,0 +1,87 @@
> +/*
> + * 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 ipc_client - User of a mailbox
> + * @chan_name: the "controller:channel" this client wants

Uh, not a consumer name?  See comment on ipc_request_channel().

> + * @cl_id: The identity to associate any tx/rx data with
> + * @rxcb: atomic callback to provide client the data received
> + * @txcb: atomic callback to tell client of data transmission
> + * @tx_block: if the ipc_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.
> + * @link_data: Optional controller specific parameters during channel request
> + */
> +struct ipc_client {

I'm not so sure about the naming scheme here.  This header is
mailbox_client.h, and drivers are expected to go in drivers/mailbox, yet the
structs and functions have an ipc_ prefix.  Please standardize this,
preferably to mbox_ or mailbox_.

> +       char *chan_name;

const?

> +       void *cl_id;

Like Greg already mentioned, this is ugly.  My recommendation would be
to not have a "client_data" pointer here at all, and instead store a
pointer to this struct in the ipc_chan struct.  Then you could just use
a pointer to this struct in the callback function.

> +       void (*rxcb)(void *cl_id, void *mssg);
> +       void (*txcb)(void *cl_id, void *mssg, enum xfer_result r);

I don't believe we've run out of vowels....  Please make these names
more verbose.

Additionally, is there an actual use-case for having an async tx-done
callback?  Couldn't this be entirely solved by either:
	a) calling the blocking API, since you would have to manage that
	   in your own context, just like tx-done
	b) not caring about tx-done?  It seems your code supports this.

> +       bool tx_block;

This should be a parameter to ipc_send_message()... see comment below.

> +       unsigned long tx_tout;

'tx_timeout' I think would be a more descriptive name.  Although, I
don't quite understand why the client/consumer would specify this.

> +       bool knows_txdone;

If the client is running the state-machine, this is neither a mailbox,
nor much of a useful framework for whatever it is the client has
implemented.  Could you please explain the motivation here?

> +       void *link_data;

Random opaque data to pass straight through the framework?  This seems like
a hack.  If this is needed, clearly the framework has not implemented
something generic enough.

> +};
> +
> +/**
> + * The Client specifies its requirements and capabilities while asking for
> + * a channel/mailbox 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 ipc_free_channel.
> + */
> +void *ipc_request_channel(struct ipc_client *cl);

Greg has already commented on the return type here, but there are a few
further things I don't quite understand.

The lookup key here is the name, which is embedded in the struct.
While most of the struct seems to define the client, the name defines
something which seems to be an attribute of the controller.

The organization of this request/free seems all wrong.  If you look at
other frameworks (e.g clk, regulators, pwm), you'll see that this
doesn't quite fit in.  The accepted behavior seems to be to have a
'consumer name' which *is* an attribute of the client's device, and
simply maps to an index, which then may be used to look up a
mbox/channel in the controller.  The controller then has no need to name
its own channels, as these are usually better named by the communication
for which they are used (i.e. whatever the client driver expects it to
do).

Essentially, this is what I would expect to see here:

struct ipc_chan *ipc_request_channel(struct device *dev,
		const char *con_id, struct ipc_client *client);

Typically, the device and connection id are even optional and can be
NULL under certain conditions.

This would also map very cleanly into devicetree bindings, which are
really needed for this framework.  I would also like to see a
devm_ variant of this.

> +
> +/**
> + * 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-zero token is returned for each queued request. If the queue
> + * was full the returned token will be 0. Upon failure or successful
> + * TX, the API calls 'txcb' from atomic context, from which the client
> + * could submit yet another request.

The doc for this function should probably also mention that it is
expected that 'mssg' be available for consumption up until the point in
which tx-done is triggered.  

> + *  In blocking mode, 'txcb' is not called, effectively making the
> + * queue length 1. The returned value is 0 if TX timed out, some
> + * non-zero value upon success.
> + */
> +request_token_t ipc_send_message(void *channel, void *mssg);

In the case of the blocking use-case, the function's return type is odd.
Perhaps, instead of my recommendation above, this could be turned into
two separate functions: ipc_send_message() and ipc_send_message_async()?

> +
> +/**
> + * The way for a client to run the TX state machine. This works
> + * only if the client sets 'knows_txdone' and the IPC controller
> + * doesn't get an IRQ for TX_Done/Remote_RTR.
> + */
> +void ipc_client_txdone(void *channel, enum xfer_result r);
> +
> +/**
> + * The client relinquishes control of a mailbox by this call,
> + * make it available to other clients.
> + * The ipc_request/free_channel are light weight calls, so the
> + * client should avoid holding it when it doesn't need to
> + * transfer data.
> + */
> +void ipc_free_channel(void *channel);

I would tend to disagree with the comment that request/free are
light-weight, as a mutex lock is (well, should be) held around two
list lookups for request.  Additionally, it would appear to me that
since it calls the controllers startup/shutdown callbacks, there's no
guarantee that bringing-up/down the 'links' is a simple or quick
procedure.

With this in mind, I also don't believe that it would be a good idea to
avoid holding a channel, as having drivers "trade" channels can very
easily result in nasty race conditions.

> +
> +/**
> + * The client make ask the API to be notified when a particular channel
> + * becomes available to be acquired again.
> + */
> +int ipc_notify_chan_register(const char *name, struct notifier_block *nb);
> +
> +/**
> + * The client is no more interested in acquiring the channel.
> + */
> +void ipc_notify_chan_unregister(const char *name, struct notifier_block *nb);
> +

I don't understand the purpose of this functionality.  It would seem to
me that it would either be an error to need a channel that is already
being used, or the API needs to support having multiple clients per-channel.

> +#endif /* __MAILBOX_CLIENT_H */
> diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
> new file mode 100644
> index 0000000..a907467
> --- /dev/null
> +++ b/include/linux/mailbox_controller.h
> @@ -0,0 +1,102 @@
> +/*
> + * 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 ipc_link - s/w representation of a communication link
> + * @link_name: Literal name assigned to the link. Physically
> + *    identical channels may have the same name.

I do not think "physically identical" is enough here.  One would need to
know that the endpoint does not care about which channel you use.
Unfortunately, I think that this would be known more by the client, who
is attempting to communicate with the other side, then the controller,
which should just opaquely represent a mailbox chip.  I additionally
think that it would be more appropriate to just represent these channels
by index, rather than name, for the same reasons.

> + * @api_priv: hook for the API to map its private data on the link
> + *    Controller driver must not touch it.
> + */
> +struct ipc_link {
> +       char link_name[16];
> +       void *api_priv;

There's no reason here to hide things from the controller; it is not
normal that we assume driver developers are malicious.  Documentation
describing that the data is for internal use only is sufficient.

> +};
[...]
> +struct ipc_controller {
> +       char controller_name[16];

const char *name; should be sufficient here, this shouldn't change.

> +       struct ipc_link_ops *ops;

const

> +       struct ipc_link **links;

const?

> +       bool txdone_irq;
> +       bool txdone_poll;
> +       unsigned txpoll_period;
> +};
> +
> +/**
> + * The controller driver registers its communication links to the
> + * global pool managed by the API.
> + */
> +int ipc_links_register(struct ipc_controller *ipc_con);

I'm not sure about 'links' here, doesn't this function actually register
a controller, including its links?  Perhaps ipc_controller_register() ?

> +/**
> + * After startup and before shutdown any data received on the link
> + * is pused to the API via atomic ipc_link_received_data() API.
> + * The controller should ACK the RX only after this call returns.
> + */
> +void ipc_link_received_data(struct ipc_link *link, void *data);
> +
> +/**
> + * The controller the 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 ipc_link_txdone(struct ipc_link *link, enum xfer_result r);

Representing this all as a state machine seems odd, as it removes
considerable usefulness of the API if the controllers (and in the
polling case, the clients) need to know about the state machine.

> +/**
> + * Purge the links from the global pool maintained by the API.
> + */
> +void ipc_links_unregister(struct ipc_controller *ipc_con);
> +
> +#endif /* __MAILBOX_CONTROLLER_H */


I have added relevant parties from other series related to this.

Although I will admit that I didn't go looking at random code on the
internet before submitting my series, I did take a look at the previous
iterations of this submitted for upstream.  I tried to take the comments
and considerations in mind when designing my version of this framework.

I have tried to be fair with this code review, but as you can see, most
of the issues I have are resolved in my series.  It is possible that I
have missed something, as I was not privy to the off-list conversations
about this particular code, but other than tx-done callbacks and
polling--which I'm not convinced are necessary--it would appear that my
series (with the async work) covers all of the intended use-cases here.
Obviously I am somewhat biased, so I welcome arguments otherwise.

-Courtney

Re: [PATCHv3 2/6] mailbox: Introduce a new common API

[Jassi Brar]

Hi Courtney,

On 18 February 2014 06:22, Courtney Cavin <courtney.cavin@sonymobile.com> wrote:
> On Sat, Feb 15, 2014 at 07:25:27PM +0100, Jassi Brar wrote:

>> +request_token_t ipc_send_message(void *channel, void *mssg)
>> +{
>> +       struct ipc_chan *chan = (struct ipc_chan *)channel;
>> +       request_token_t t;
>> +
>> +       if (!chan || !chan->assigned)
>> +               return 0;
>
> 0 is not an error in my book.  Please make this more obvious, or define
> something which categorizes this as an error.
>
The returned value is index+1 of the slot assigned for message. Yeah
we could mention that in kerneldoc for the api or make it something <0

>> +void *ipc_request_channel(struct ipc_client *cl)
>> +{
>> +       struct ipc_chan *chan;
>> +       struct ipc_con *con;
>> +       unsigned long flags;
>> +       char *con_name;
>> +       int len, ret;
>> +
>> +       con_name = cl->chan_name;
>> +       len = strcspn(cl->chan_name, ":");
>> +
>> +       ret = 0;
>> +       mutex_lock(&con_mutex);
>> +       list_for_each_entry(con, &ipc_cons, node)
>> +               if (!strncmp(con->name, con_name, len)) {
>> +                       ret = 1;
>> +                       break;
>> +               }
>> +       mutex_unlock(&con_mutex);
>> +
>> +       if (!ret) {
>> +               pr_info("Channel(%s) not found!\n", cl->chan_name);
>> +               return NULL;
>
> ERR_PTR(-ENODEV) would be better.
>
Thanks, good point.

>> +       }
>> +
>> +       ret = 0;
>> +       list_for_each_entry(chan, &con->channels, node) {
>> +               if (!strcmp(con_name + len + 1, chan->name)
>> +                               && !chan->assigned) {
>
> Move the !chan->assigned check first, so we can skip the strcmp if not
> needed.
>
It was indeed that way in previous revision, somehow overlooked that
in haste to resubmit by that day. Thanks.

>> +                       spin_lock_irqsave(&chan->lock, flags);
>> +                       chan->msg_free = 0;
>> +                       chan->msg_count = 0;
>> +                       chan->active_req = NULL;
>> +                       chan->rxcb = cl->rxcb;
>> +                       chan->txcb = cl->txcb;
>> +                       chan->cl_id = cl->cl_id;
>> +                       chan->assigned = true;
>> +                       chan->tx_block = cl->tx_block;
>
> Copying all of this data is unnecessary, and prone to error.  Just point
> to 'cl' in the client struct.
>
That did occur to me. But I chose to have controller specify its
attributes from lower layer and the client from upper layer. The api
basically works on channels... which get their
functionality/limitations from controllers+clients and save them in
their own structure. That also helps 'submit message and moveon' type
clients.

>> +                       if (!cl->tx_tout)
>> +                               chan->tx_tout = msecs_to_jiffies(3600000);
>
> An hour!?  Arbitrary, and extremely long.  Either this should be
> indefinite, or there should be a reasonably small value here.
>
Ideally the client _should_ specify the timeout. Here the default 1hr
is infinity, roughly speaking.
What value do you suggest?

>> +                       else
>> +                               chan->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 = 1;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       if (!ret) {
>> +               pr_err("Unable to assign mailbox(%s)\n", cl->chan_name);
>> +               return NULL;
>
> ERR_PTR(-EBUSY) if busy, ERR_PTR(-ENODEV) if unavailable.
>
Yeah that'll be better.

>> +       }
>> +
>> +       ret = chan->link_ops->startup(chan->link, cl->link_data);
>> +       if (ret) {
>> +               pr_err("Unable to startup the link\n");
>> +               ipc_free_channel((void *)chan);
>> +               return NULL;
>
> ERR_PTR(ret);
>
Yup

>> +int ipc_links_register(struct ipc_controller *ipc)
>> +{
>> +       int i, num_links, txdone;
>> +       struct ipc_chan *chan;
>> +       struct ipc_con *con;
>> +
>> +       /* Sanity check */
>> +       if (!ipc || !ipc->ops)
>
> You probably want to check for !ipc->links here too, as you immediately
> dereference it.
>
Yeah, right. Thanks.

>> +               return -EINVAL;
>> +
>> +       for (i = 0; ipc->links[i]; i++)
>
> So you require a NULL node at the end?  Why not have a nlinks/num_links
> in the controller struct?  It would save having to count here.
>
Rather I find that noisy. Why add variables that we can do without?
Especially when the variable would be used just once.

>> +
>> +       chan = (void *)con + sizeof(*con);
>> +       for (i = 0; i < num_links; i++) {
>> +               chan[i].con = con;
>> +               chan[i].assigned = false;
>> +               chan[i].link_ops = ipc->ops;
>> +               chan[i].link = ipc->links[i];
>> +               chan[i].txdone_method = txdone;
>> +               chan[i].link->api_priv = &chan[i];
>> +               spin_lock_init(&chan[i].lock);
>> +               BLOCKING_INIT_NOTIFIER_HEAD(&chan[i].avail);
>> +               list_add_tail(&chan[i].node, &con->channels);
>
> Why not have all of this data exposed in the link definition, so you
> don't need this list, and can represent it as a pre-populated array?
>
Because the api choose to work only in terms of ipc channels. Infact
in previous revision there was just one global pool of channels that
the api managed. Suman Anna insisted we manage channels in controllers
so it makes searching easier.

>> +               snprintf(chan[i].name, 16, "%s", ipc->links[i]->link_name);
>> +       }
>> +
>> +       mutex_lock(&con_mutex);
>> +       list_add_tail(&con->node, &ipc_cons);
>> +       mutex_unlock(&con_mutex);
>> +
>> +       return 0;
>
> You should have a try_module_get() somewhere in this function, and
> module_put() somewhere in unregister.
>
Thanks. Yes we need to keep reference held.

>> +
>> +#ifndef __MAILBOX_H
>> +#define __MAILBOX_H
>> +
>> +enum xfer_result {
>> +       XFER_OK = 0,
>> +       XFER_ERR,
>> +};
>
> This is not the only framework which does 'xfer's, please add a prefix
> to this enum and values.
>
OK, will do.

>> +
>> +typedef unsigned request_token_t;
>> +
>
> Likewise, if needed.
>
OK, will do.

>> +/**
>> + * struct ipc_client - User of a mailbox
>> + * @chan_name: the "controller:channel" this client wants
>
> Uh, not a consumer name?  See comment on ipc_request_channel().
>
Yeah no consumer name. The controller driver gets to name its
channels, a client simply uses them.

>> + * @cl_id: The identity to associate any tx/rx data with
>> + * @rxcb: atomic callback to provide client the data received
>> + * @txcb: atomic callback to tell client of data transmission
>> + * @tx_block: if the ipc_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.
>> + * @link_data: Optional controller specific parameters during channel request
>> + */
>> +struct ipc_client {
>
> I'm not so sure about the naming scheme here.  This header is
> mailbox_client.h, and drivers are expected to go in drivers/mailbox, yet the
> structs and functions have an ipc_ prefix.  Please standardize this,
> preferably to mbox_ or mailbox_.
>
Yeah Loic already pointed that out.
The term 'mailbox' is specific to what ARM calls the IPC links in
their manuals. Perhaps we should rename the directory to drivers/ipc/.
 'Mailbox' is too symbolic. Though I am OK either way.

>> +       char *chan_name;
>
> const?
>
OK.

>> +       void *cl_id;
>
> Like Greg already mentioned, this is ugly.  My recommendation would be
> to not have a "client_data" pointer here at all, and instead store a
> pointer to this struct in the ipc_chan struct.  Then you could just use
> a pointer to this struct in the callback function.
>
I realize I replied to Greg the role of link_data. Sorry Greg.
'cl_id' is what the client would like to associate the TX/RX callbacks
with. Thanks to Suman for bringing up the use-case where he needed
common callback functions for different clients.
'void *' is because the generic api could never know the type of
structure the client would embed its private data in.

>> +       void (*rxcb)(void *cl_id, void *mssg);
>> +       void (*txcb)(void *cl_id, void *mssg, enum xfer_result r);
>
> I don't believe we've run out of vowels....  Please make these names
> more verbose.
>
Reading rxcb/txcb in this code, did you think of anything other than
'receive_callback/transmit_callback'? What was it?

> Additionally, is there an actual use-case for having an async tx-done
> callback?
>
Yes, some client might need to submit a 'beacon' type message.

>         a) calling the blocking API, since you would have to manage that
>            in your own context, just like tx-done
>         b) not caring about tx-done?  It seems your code supports this.
>
>> +       bool tx_block;
>
> This should be a parameter to ipc_send_message()... see comment below.
>
>> +       unsigned long tx_tout;
>
> 'tx_timeout' I think would be a more descriptive name.  Although, I
> don't quite understand why the client/consumer would specify this.
>
Perhaps you haven't read the threads of previous revisions.

>> +       bool knows_txdone;
>
> If the client is running the state-machine, this is neither a mailbox,
> nor much of a useful framework for whatever it is the client has
> implemented.  Could you please explain the motivation here?
>
You could have read the code and previous threads that had lots of
use-cases and rationale explained.

Anyways... consider my _real_ use-case that has come to use it.
 My IPC controller can not generate interrupt for TX-Done (when the
message/reply was read by the remote), but it does interrupt when the
remote has sent a message/reply. The client does not run the
state-machine, but it sure can save us from polling for TX-Done when
it know the received packet is reply to its last TX... so it tells the
API the last TX was done and the callback is done much sooner than the
next poll. This feature is solely responsible for keeping the IPC
latency within acceptable limits for my platform.

>> +       void *link_data;
>
> Random opaque data to pass straight through the framework?  This seems like
> a hack.  If this is needed, clearly the framework has not implemented
> something generic enough.
>
Nopes! I strongly suggest you read previous threads.

Here's the rant... in IPC a client doesn't behave only according to
the underlying controller. But to the combination of controller plus
the remote f/w(or the protocol).  For example, one protocol may
specify the remote will 'clear' the packet-location after reading it
while another protocol will not clear it but sends a particular
reply-packet. Since we can not have two drivers for the same
controller, someone has to tell the controller driver how to behave
for the current protocol... and that 'someone' can only be the client.
There can be no generic code that could enumerate all possible
behaviors.

>> +/**
>> + * The Client specifies its requirements and capabilities while asking for
>> + * a channel/mailbox 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 ipc_free_channel.
>> + */
>> +void *ipc_request_channel(struct ipc_client *cl);
>
> Greg has already commented on the return type here, but there are a few
> further things I don't quite understand.
>
> The lookup key here is the name, which is embedded in the struct.
> While most of the struct seems to define the client, the name defines
> something which seems to be an attribute of the controller.
>
I am not sure you understand. The client specifies which channel of
which controller does it want. The api keeps channels hooked up in
controller bunch. And can figure out which channel to return.

>
> This would also map very cleanly into devicetree bindings, which are
> really needed for this framework.  I would also like to see a
> devm_ variant of this.
>
Yeah we need to move onto device tree mappings if things gets fancy.
Right now we all had simple clients and unique controllers.

>> +
>> +/**
>> + * 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-zero token is returned for each queued request. If the queue
>> + * was full the returned token will be 0. Upon failure or successful
>> + * TX, the API calls 'txcb' from atomic context, from which the client
>> + * could submit yet another request.
>
> The doc for this function should probably also mention that it is
> expected that 'mssg' be available for consumption up until the point in
> which tx-done is triggered.
>
OK, I will update the comment.

>> + *  In blocking mode, 'txcb' is not called, effectively making the
>> + * queue length 1. The returned value is 0 if TX timed out, some
>> + * non-zero value upon success.
>> + */
>> +request_token_t ipc_send_message(void *channel, void *mssg);
>
> In the case of the blocking use-case, the function's return type is odd.
> Perhaps, instead of my recommendation above, this could be turned into
> two separate functions: ipc_send_message() and ipc_send_message_async()?
>
In blocking use case, the call will return success (a valid token).
The client sees a valid token and understands that the request was
accepted. And because the client asked it to be a blocking call, it
also understands the TX is successfully completed.  What's odd here?

>> +
>> +/**
>> + * The way for a client to run the TX state machine. This works
>> + * only if the client sets 'knows_txdone' and the IPC controller
>> + * doesn't get an IRQ for TX_Done/Remote_RTR.
>> + */
>> +void ipc_client_txdone(void *channel, enum xfer_result r);
>> +
>> +/**
>> + * The client relinquishes control of a mailbox by this call,
>> + * make it available to other clients.
>> + * The ipc_request/free_channel are light weight calls, so the
>> + * client should avoid holding it when it doesn't need to
>> + * transfer data.
>> + */
>> +void ipc_free_channel(void *channel);
>
> I would tend to disagree with the comment that request/free are
> light-weight, as a mutex lock is (well, should be) held around two
> list lookups for request.  Additionally, it would appear to me that
> since it calls the controllers startup/shutdown callbacks, there's no
> guarantee that bringing-up/down the 'links' is a simple or quick
> procedure.
>
The 'light-weight' comment is not to mean it's atomic. It was to
suggest the call doesn't wait for submitted xfers to complete and any
other api related waits.

If you problem is with the 'light-weight' part, I'll remove it. But
still the clients will behave just as their use-cases are!

> With this in mind, I also don't believe that it would be a good idea to
> avoid holding a channel, as having drivers "trade" channels can very
> easily result in nasty race conditions.
>
There are use-cases that require holding onto a channel (server model)
and there are use-cases that require a client to relieve a channel
immediately after it's done. Seems you are not aware of use-case out
there in the wild (pls read old threads).

>> +
>> +/**
>> + * The client is no more interested in acquiring the channel.
>> + */
>> +void ipc_notify_chan_unregister(const char *name, struct notifier_block *nb);
>> +
>
> I don't understand the purpose of this functionality.  It would seem to
> me that it would either be an error to need a channel that is already
> being used
>
Again ... you are not aware of enough use-cases.

> or the API needs to support having multiple clients per-channel.
>
For platforms with 2 or more clients and each client having to do it's
own transfer... a channel may be busy when a client needs it. So
instead of implementing its own poll mechanism for availability of the
channel, the client could (un)register to be notified when a channel
become available.
BTW, precisely on such platforms a client would like to relieve a
channel as soon as its done... which is anyway a good practice of not
hogging resources.

There are use-cases where a client needs exclusive access to the
channel... no other client should be able to send a message in the
middle of some 'atomic-sequence' that is already in progress. Which is
not possible if the API worked promiscuously. Whereas if the platform
wants it could still emulate the 'server' model(multiple clients per
channel) by having a global client submitting messages on behalf of
other drivers. Again, pls read old threads.

>> +
>> +/**
>> + * struct ipc_link - s/w representation of a communication link
>> + * @link_name: Literal name assigned to the link. Physically
>> + *    identical channels may have the same name.
>
> I do not think "physically identical" is enough here.  One would need to
> know that the endpoint does not care about which channel you use.
> Unfortunately, I think that this would be known more by the client, who
> is attempting to communicate with the other side, then the controller,
>
That part is what 'link_data' is for (provided by the client and
passed onto the controller).

'physically identical' is for controller driver and implies if the
links are equally capable. For example the PL320 has identical links.
A properly written PL320 driver will manage links as floating
resources and assign whichever is free. If the platform makes some
assumptions that need to be communicated via link_data.

>> + * @api_priv: hook for the API to map its private data on the link
>> + *    Controller driver must not touch it.
>> + */
>> +struct ipc_link {
>> +       char link_name[16];
>> +       void *api_priv;
>
> There's no reason here to hide things from the controller; it is not
> normal that we assume driver developers are malicious.  Documentation
> describing that the data is for internal use only is sufficient.
>
It is common practice for an api to provide hooks for private data of
lower layer.

>> +/**
>> + * The controller driver registers its communication links to the
>> + * global pool managed by the API.
>> + */
>> +int ipc_links_register(struct ipc_controller *ipc_con);
>
> I'm not sure about 'links' here, doesn't this function actually register
> a controller, including its links?  Perhaps ipc_controller_register() ?
>
OK, I'll rename it to ipc_controller_register()

>> +/**
>> + * The controller the 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 ipc_link_txdone(struct ipc_link *link, enum xfer_result r);
>
> Representing this all as a state machine seems odd, as it removes
> considerable usefulness of the API if the controllers (and in the
> polling case, the clients) need to know about the state machine.
>
This is an _important_ part of API. The controller driver(usually from
irq) need to notify the API that the last tx is done. Please read the
code and make yourself aware of the limitations/functionality of
controllers and protocols out there.

>> +/**
>> + * Purge the links from the global pool maintained by the API.
>> + */
>> +void ipc_links_unregister(struct ipc_controller *ipc_con);
>> +
>> +#endif /* __MAILBOX_CONTROLLER_H */
>
>
> I have added relevant parties from other series related to this.
>
> Although I will admit that I didn't go looking at random code on the
> internet before submitting my series, I did take a look at the previous
> iterations of this submitted for upstream.
>
But you objections (except cosmetic ones) sound like you are not aware
of all use-cases that have been taken care of.

>  I tried to take the comments
> and considerations in mind when designing my version of this framework.
>
> I have tried to be fair with this code review, but as you can see, most
> of the issues I have are resolved in my series.  It is possible that I
> have missed something, as I was not privy to the off-list conversations
> about this particular code, but other than tx-done callbacks and
> polling--which I'm not convinced are necessary--it would appear that my
> series (with the async work) covers all of the intended use-cases here.
> Obviously I am somewhat biased, so I welcome arguments otherwise.
>
Thanks for reviewing the code so critically, but it would have been
much greater help if you had explained if/how your controller/protocol
doesn't work with this api.

Thanks
Jassi

Re: [PATCHv3 2/6] mailbox: Introduce a new common API

[Bjorn Andersson]

On Mon, Feb 17, 2014 at 11:06 PM, Jassi Brar <jaswinder.singh@linaro.org> wrote:
> Hi Courtney,
>
> On 18 February 2014 06:22, Courtney Cavin <courtney.cavin@sonymobile.com> wrote:
>> On Sat, Feb 15, 2014 at 07:25:27PM +0100, Jassi Brar wrote:
[...]
>>> +struct ipc_client {
>>
>> I'm not so sure about the naming scheme here.  This header is
>> mailbox_client.h, and drivers are expected to go in drivers/mailbox, yet the
>> structs and functions have an ipc_ prefix.  Please standardize this,
>> preferably to mbox_ or mailbox_.
>>
> Yeah Loic already pointed that out.
> The term 'mailbox' is specific to what ARM calls the IPC links in
> their manuals. Perhaps we should rename the directory to drivers/ipc/.
>  'Mailbox' is too symbolic. Though I am OK either way.

In this world IPC means "Inter-process communication", so I'm afraid it's taken.

[...]
>>> +request_token_t ipc_send_message(void *channel, void *mssg);
>>
>> In the case of the blocking use-case, the function's return type is odd.
>> Perhaps, instead of my recommendation above, this could be turned into
>> two separate functions: ipc_send_message() and ipc_send_message_async()?
>>
> In blocking use case, the call will return success (a valid token).
> The client sees a valid token and understands that the request was
> accepted. And because the client asked it to be a blocking call, it
> also understands the TX is successfully completed.  What's odd here?
>

I've tried to figure out why you return your magic type here, from
what I can see it's just indicating to the consumer if the tx
succeeded (and maybe was put on the fifo?). When constructing this
number you have a comment that says "aid for debugging", which
indicates that it really doesn't hold any value...

There is already a convention for this in the kernel, return
descriptive negative numbers on failures; 0 on success.


How do you plan to support probe deferral here?

What is the plan to support references in Device Tree?

[...]
>> Although I will admit that I didn't go looking at random code on the
>> internet before submitting my series, I did take a look at the previous
>> iterations of this submitted for upstream.
>>
> But you objections (except cosmetic ones) sound like you are not aware
> of all use-cases that have been taken care of.

Your argument in the discussion of Courtneys proposal was "I have code
that depends on my code, let me show it to you first".

So please enlighten us on what use cases it is that you do support;
that are actually seen in real life.

Regards,
Bjorn

Re: [PATCHv3 2/6] mailbox: Introduce a new common API

[Jassi Brar]

On Tue, Feb 18, 2014 at 11:00 PM, Bjorn Andersson <bjorn@kryo.se> wrote:
> On Mon, Feb 17, 2014 at 11:06 PM, Jassi Brar <jaswinder.singh@linaro.org> wrote:
>> Hi Courtney,
>>
>> On 18 February 2014 06:22, Courtney Cavin <courtney.cavin@sonymobile.com> wrote:
>>> On Sat, Feb 15, 2014 at 07:25:27PM +0100, Jassi Brar wrote:
> [...]
>>>> +struct ipc_client {
>>>
>>> I'm not so sure about the naming scheme here.  This header is
>>> mailbox_client.h, and drivers are expected to go in drivers/mailbox, yet the
>>> structs and functions have an ipc_ prefix.  Please standardize this,
>>> preferably to mbox_ or mailbox_.
>>>
>> Yeah Loic already pointed that out.
>> The term 'mailbox' is specific to what ARM calls the IPC links in
>> their manuals. Perhaps we should rename the directory to drivers/ipc/.
>>  'Mailbox' is too symbolic. Though I am OK either way.
>
> In this world IPC means "Inter-process communication", so I'm afraid it's taken.
>
In this very world IPC also stands for "Inter-Processor-Communication" :)
But OK let's call it mailbox.

> [...]
>>>> +request_token_t ipc_send_message(void *channel, void *mssg);
>>>
>>> In the case of the blocking use-case, the function's return type is odd.
>>> Perhaps, instead of my recommendation above, this could be turned into
>>> two separate functions: ipc_send_message() and ipc_send_message_async()?
>>>
>> In blocking use case, the call will return success (a valid token).
>> The client sees a valid token and understands that the request was
>> accepted. And because the client asked it to be a blocking call, it
>> also understands the TX is successfully completed.  What's odd here?
>>
>
> I've tried to figure out why you return your magic type here, from
> what I can see it's just indicating to the consumer if the tx
> succeeded (and maybe was put on the fifo?). When constructing this
> number you have a comment that says "aid for debugging", which
> indicates that it really doesn't hold any value...
>
> There is already a convention for this in the kernel, return
> descriptive negative numbers on failures; 0 on success.
>
Courtney isn't talking about '0 as error code' here... which I said I
am ok making that a negative value.

>
> How do you plan to support probe deferral here?
>
What exactly do you mean? Aren't the drivers' probe supposed to do that?

> What is the plan to support references in Device Tree?
>
As I said right now channel assignment via DT isn't there. Nothing against that.

>>> Although I will admit that I didn't go looking at random code on the
>>> internet before submitting my series, I did take a look at the previous
>>> iterations of this submitted for upstream.
>>>
>> But you objections (except cosmetic ones) sound like you are not aware
>> of all use-cases that have been taken care of.
>
> Your argument in the discussion of Courtneys proposal was "I have code
> that depends on my code, let me show it to you first".
>
> So please enlighten us on what use cases it is that you do support;
> that are actually seen in real life.
>
Please read my last reply... I have mentioned many use-cases. All
those are supported and more. Courtney's asking and my giving
use-cases suggests Courtney wasn't even aware such cases existed.
Again, please read up the old threads.

  I, Suman Anna and Loic went through a lot of exchanges to cover
'all' variations in each step of inter-processor-communication. For
example some controllers have TX-Done irq, some have RX-Done irq, some
have both and some neither. Some clients want blocking and some async
xfer. Shared channel vs exclusive ownership support etc. No wonder the
last two platforms to join us found the api just worked for them.
  Please point me to your mailbox controller's manual, and I am sure I
can cook some code for you atleast as good as for your api. And I
haven't yet even got into reviewing Courtney's implementation. It is
sad it has come to yours-versus-mine ... Courtney should have picked
up our original implementation, which has the juice of long
discussions, and maybe 'fixed' it to taste and submitted.

-Jassi

Re: [PATCHv3 2/6] mailbox: Introduce a new common API

[Courtney Cavin]

On Tue, Feb 18, 2014 at 08:06:55AM +0100, Jassi Brar wrote:
> On 18 February 2014 06:22, Courtney Cavin <courtney.cavin@sonymobile.com> wrote:
> > On Sat, Feb 15, 2014 at 07:25:27PM +0100, Jassi Brar wrote:
> 
> >> +request_token_t ipc_send_message(void *channel, void *mssg)
> >> +{
> >> +       struct ipc_chan *chan = (struct ipc_chan *)channel;
> >> +       request_token_t t;
> >> +
> >> +       if (!chan || !chan->assigned)
> >> +               return 0;
> >
> > 0 is not an error in my book.  Please make this more obvious, or define
> > something which categorizes this as an error.
> >
> The returned value is index+1 of the slot assigned for message. Yeah
> we could mention that in kerneldoc for the api or make it something <0

Please do.

> >> +void *ipc_request_channel(struct ipc_client *cl)
> >> +{
[...]
> >> +                       spin_lock_irqsave(&chan->lock, flags);
> >> +                       chan->msg_free = 0;
> >> +                       chan->msg_count = 0;
> >> +                       chan->active_req = NULL;
> >> +                       chan->rxcb = cl->rxcb;
> >> +                       chan->txcb = cl->txcb;
> >> +                       chan->cl_id = cl->cl_id;
> >> +                       chan->assigned = true;
> >> +                       chan->tx_block = cl->tx_block;
> >
> > Copying all of this data is unnecessary, and prone to error.  Just point
> > to 'cl' in the client struct.
> >
> That did occur to me. But I chose to have controller specify its
> attributes from lower layer and the client from upper layer. The api
> basically works on channels... which get their
> functionality/limitations from controllers+clients and save them in
> their own structure. That also helps 'submit message and moveon' type
> clients.

I understand how this currently works, but I see no reason why one
couldn't simply point to the client struct here, instead of duplicating
the client definition, and copying the data.

I don't see an argument for fire-and-forget clients here.  The lifetime
of the callback and private data must persist, so why not the containing
struct?

> >> +                       if (!cl->tx_tout)
> >> +                               chan->tx_tout = msecs_to_jiffies(3600000);
> >
> > An hour!?  Arbitrary, and extremely long.  Either this should be
> > indefinite, or there should be a reasonably small value here.
> >
> Ideally the client _should_ specify the timeout. Here the default 1hr
> is infinity, roughly speaking.
> What value do you suggest?

An hour is by definition uncountably far from infinity.  The fact that
this may seem like it is infinite in a test setup indicates that this
will cause problems in the future, when/if the timeout occurs.  I
suggest using wait_for_completion() if this is 0, or alternatively a
timeout short enough to trigger easily via testing (e.g. 1 second).

> >> +int ipc_links_register(struct ipc_controller *ipc)
> >> +{
> >> +       int i, num_links, txdone;
> >> +       struct ipc_chan *chan;
> >> +       struct ipc_con *con;
> >> +
> >> +       /* Sanity check */
> >> +       if (!ipc || !ipc->ops)
> >
> > You probably want to check for !ipc->links here too, as you immediately
> > dereference it.
> >
> Yeah, right. Thanks.
> 
> >> +               return -EINVAL;
> >> +
> >> +       for (i = 0; ipc->links[i]; i++)
> >
> > So you require a NULL node at the end?  Why not have a nlinks/num_links
> > in the controller struct?  It would save having to count here.
> >
> Rather I find that noisy. Why add variables that we can do without?
> Especially when the variable would be used just once.

Just like the marker link you need at the end of your array?  It's more
flexible, and easier to understand from an API perspective.

> >> +
> >> +       chan = (void *)con + sizeof(*con);
> >> +       for (i = 0; i < num_links; i++) {
> >> +               chan[i].con = con;
> >> +               chan[i].assigned = false;
> >> +               chan[i].link_ops = ipc->ops;
> >> +               chan[i].link = ipc->links[i];
> >> +               chan[i].txdone_method = txdone;
> >> +               chan[i].link->api_priv = &chan[i];
> >> +               spin_lock_init(&chan[i].lock);
> >> +               BLOCKING_INIT_NOTIFIER_HEAD(&chan[i].avail);
> >> +               list_add_tail(&chan[i].node, &con->channels);
> >
> > Why not have all of this data exposed in the link definition, so you
> > don't need this list, and can represent it as a pre-populated array?
> >
> Because the api choose to work only in terms of ipc channels. Infact
> in previous revision there was just one global pool of channels that
> the api managed. Suman Anna insisted we manage channels in controllers
> so it makes searching easier.

"Legacy reasons" is not a good argument here.  Clearly this code knows
about the relationship between a client and a controller.  Additionally,
it has all the data it needs provided by the controller in an array. Why
not continue using that array?

> >> +               snprintf(chan[i].name, 16, "%s", ipc->links[i]->link_name);
> >> +       }
> >> +
> >> +       mutex_lock(&con_mutex);
> >> +       list_add_tail(&con->node, &ipc_cons);
> >> +       mutex_unlock(&con_mutex);
> >> +
> >> +       return 0;
> >
> > You should have a try_module_get() somewhere in this function, and
> > module_put() somewhere in unregister.
> >
> Thanks. Yes we need to keep reference held.

Sorry, I'm not sure what I was thinking here.  The try_module_get()
should be in request_channel(), and module_put() should be in
free_channel().  Additionally, unregister should return failure if a
channel is currently requested.

> >> +/**
> >> + * struct ipc_client - User of a mailbox
> >> + * @chan_name: the "controller:channel" this client wants
> >
> > Uh, not a consumer name?  See comment on ipc_request_channel().
> >
> Yeah no consumer name. The controller driver gets to name its
> channels, a client simply uses them.

Yes.  I simply don't think this conforms to "typical" kernel frameworks.

> >> +struct ipc_client {
> >
> > I'm not so sure about the naming scheme here.  This header is
> > mailbox_client.h, and drivers are expected to go in drivers/mailbox, yet the
> > structs and functions have an ipc_ prefix.  Please standardize this,
> > preferably to mbox_ or mailbox_.
> >
> Yeah Loic already pointed that out.
> The term 'mailbox' is specific to what ARM calls the IPC links in
> their manuals. Perhaps we should rename the directory to drivers/ipc/.
>  'Mailbox' is too symbolic. Though I am OK either way.

The name 'mailbox' is commonly used to refer to message-queues and is by
no means associated with ARM.  Additionally, this is very clearly not a
generic IPC framework, regardless of your definition of IPC.

> >> +       void *cl_id;
> >
> > Like Greg already mentioned, this is ugly.  My recommendation would be
> > to not have a "client_data" pointer here at all, and instead store a
> > pointer to this struct in the ipc_chan struct.  Then you could just use
> > a pointer to this struct in the callback function.
> >
> I realize I replied to Greg the role of link_data. Sorry Greg.
> 'cl_id' is what the client would like to associate the TX/RX callbacks
> with. Thanks to Suman for bringing up the use-case where he needed
> common callback functions for different clients.
> 'void *' is because the generic api could never know the type of
> structure the client would embed its private data in.

I understand the purpose of the pointer here, I'm just saying it's
unnecessary. Here's my recommendation:

struct ipc_client {
	void (*rxcb)(struct ipc_client *, void *mssg);
	...
};

struct ipc_chan {
	struct ipc_client *client;
	...
};

void call_rx_callback(struct ipc_chan *chan, void *mssg)
{
	chan->client->rxcb(chan->client, mssg);
}

Where a client can implement something like:

struct my_struct {
	struct ipc_client ipc;
	...
};

void my_callback(struct ipc_client *client, void *mssg)
{
	struct my_struct *my = container_of(client, struct my_struct, ipc);
	...
}

> >> +       void (*rxcb)(void *cl_id, void *mssg);
> >> +       void (*txcb)(void *cl_id, void *mssg, enum xfer_result r);
> >
> > I don't believe we've run out of vowels....  Please make these names
> > more verbose.
> >
> Reading rxcb/txcb in this code, did you think of anything other than
> 'receive_callback/transmit_callback'? What was it?

I thought "What the hell is 'rxcb' supposed to represent?"  Shortly
thereafter, I thought "Why the hell is the client receiving TX calls?"
Just because a name may be clear to you, doesn't mean that other people
interpret it the same way.  Even an underscore between 'rx' and 'cb'
would have helped here.  Still, in the efforts of making this code easily
understood, I would recommend something like 'rx_callback' and
'tx_done'.

> > Additionally, is there an actual use-case for having an async tx-done
> > callback?
> >
> Yes, some client might need to submit a 'beacon' type message.

Wouldn't this categorize as condition 'b' below?

> >         a) calling the blocking API, since you would have to manage that
> >            in your own context, just like tx-done
> >         b) not caring about tx-done?  It seems your code supports this.
[...]
> >> +       bool knows_txdone;
> >
> > If the client is running the state-machine, this is neither a mailbox,
> > nor much of a useful framework for whatever it is the client has
> > implemented.  Could you please explain the motivation here?
> >
> You could have read the code and previous threads that had lots of
> use-cases and rationale explained.
> 
> Anyways... consider my _real_ use-case that has come to use it.
>  My IPC controller can not generate interrupt for TX-Done (when the
> message/reply was read by the remote), but it does interrupt when the
> remote has sent a message/reply. The client does not run the
> state-machine, but it sure can save us from polling for TX-Done when
> it know the received packet is reply to its last TX... so it tells the
> API the last TX was done and the callback is done much sooner than the
> next poll. This feature is solely responsible for keeping the IPC
> latency within acceptable limits for my platform.

It sounds to me that at least part of the wire protocol over which your
client communicates is the actual mailbox here, and thus should be
represented in the controller driver itself.  What's underneath seems
more like some generic fifo without any useful mailbox bits.

> >> +       void *link_data;
> >
> > Random opaque data to pass straight through the framework?  This seems like
> > a hack.  If this is needed, clearly the framework has not implemented
> > something generic enough.
> >
> Nopes! I strongly suggest you read previous threads.
> 
> Here's the rant... in IPC a client doesn't behave only according to
> the underlying controller. But to the combination of controller plus
> the remote f/w(or the protocol).  For example, one protocol may
> specify the remote will 'clear' the packet-location after reading it
> while another protocol will not clear it but sends a particular
> reply-packet. Since we can not have two drivers for the same
> controller, someone has to tell the controller driver how to behave
> for the current protocol... and that 'someone' can only be the client.
> There can be no generic code that could enumerate all possible
> behaviors.

Please stop describing this as generic IPC, because it is far from that.
What you are describing is the difference between a driver for a
chip/block, and a wire protocol to use across with/it.  In many cases,
these are completely separate implementations, but they can come in many
different structures:
	- Single driver for generic mailbox controller
	- Driver for bus communication + driver for controller (e.g over i2c)
	- Driver for bus communication +
	  driver for packeted data + driver for controller

The "controller" in the case of this driver should be exposing whatever
combination makes it clearly defined as a "mailbox provider". This is
why it is so very important that this API represents mailboxes.

> >> +/**
> >> + * The Client specifies its requirements and capabilities while asking for
> >> + * a channel/mailbox 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 ipc_free_channel.
> >> + */
> >> +void *ipc_request_channel(struct ipc_client *cl);
> >
> > Greg has already commented on the return type here, but there are a few
> > further things I don't quite understand.
> >
> > The lookup key here is the name, which is embedded in the struct.
> > While most of the struct seems to define the client, the name defines
> > something which seems to be an attribute of the controller.
> >
> I am not sure you understand. The client specifies which channel of
> which controller does it want. The api keeps channels hooked up in
> controller bunch. And can figure out which channel to return.

I do understand.  My point is that naming the channels/links from the
controller isn't the normal method for doing this exact thing.  There's
no reason to expect that a controller can name its channels better than
a consumer can.  Neither should we expect that a client should know the
name of a channel without a binding of some sort.

What I'm recommending here is simply to use the same procedure here many
of the existing frameworks do:
	- controller maintains indexed list of channels
	- client device maintains name -> index mappings (from bindings)
	- client device maintains reference to controller (bindings)

And for platforms without DT support, a global lookup-list similar to
that in regulators and PWM frameworks, based on client device name (and
consumer id).

> >
> > This would also map very cleanly into devicetree bindings, which are
> > really needed for this framework.  I would also like to see a
> > devm_ variant of this.
> >
> Yeah we need to move onto device tree mappings if things gets fancy.
> Right now we all had simple clients and unique controllers.

Please don't underestimate the importance of devicetree support here.
Without it, most new platform/arch support will not be able to use this
framework.

> >> +request_token_t ipc_send_message(void *channel, void *mssg);
> >
> > In the case of the blocking use-case, the function's return type is odd.
> > Perhaps, instead of my recommendation above, this could be turned into
> > two separate functions: ipc_send_message() and ipc_send_message_async()?
> >
> In blocking use case, the call will return success (a valid token).
> The client sees a valid token and understands that the request was
> accepted. And because the client asked it to be a blocking call, it
> also understands the TX is successfully completed.  What's odd here?

The odd part is that one would expect a error/success state, not an id
which can be discarded, or a "nope."

> >> +/**
> >> + * The client relinquishes control of a mailbox by this call,
> >> + * make it available to other clients.
> >> + * The ipc_request/free_channel are light weight calls, so the
> >> + * client should avoid holding it when it doesn't need to
> >> + * transfer data.
> >> + */
> >> +void ipc_free_channel(void *channel);
> >
> > I would tend to disagree with the comment that request/free are
> > light-weight, as a mutex lock is (well, should be) held around two
> > list lookups for request.  Additionally, it would appear to me that
> > since it calls the controllers startup/shutdown callbacks, there's no
> > guarantee that bringing-up/down the 'links' is a simple or quick
> > procedure.
> >
> The 'light-weight' comment is not to mean it's atomic. It was to
> suggest the call doesn't wait for submitted xfers to complete and any
> other api related waits.

Then mention that instead, please.

> If you problem is with the 'light-weight' part, I'll remove it. But
> still the clients will behave just as their use-cases are!
> 
> > With this in mind, I also don't believe that it would be a good idea to
> > avoid holding a channel, as having drivers "trade" channels can very
> > easily result in nasty race conditions.
> >
> There are use-cases that require holding onto a channel (server model)
> and there are use-cases that require a client to relieve a channel
> immediately after it's done. Seems you are not aware of use-case out
> there in the wild (pls read old threads).

Or perhaps I just disagree that these are valid use-cases....

> >> +
> >> +/**
> >> + * The client is no more interested in acquiring the channel.
> >> + */
> >> +void ipc_notify_chan_unregister(const char *name, struct notifier_block *nb);
> >> +
> >
> > I don't understand the purpose of this functionality.  It would seem to
> > me that it would either be an error to need a channel that is already
> > being used
> >
> Again ... you are not aware of enough use-cases.

Thanks.

> > or the API needs to support having multiple clients per-channel.
> >
> For platforms with 2 or more clients and each client having to do it's
> own transfer... a channel may be busy when a client needs it. So
> instead of implementing its own poll mechanism for availability of the
> channel, the client could (un)register to be notified when a channel
> become available.
> BTW, precisely on such platforms a client would like to relieve a
> channel as soon as its done... which is anyway a good practice of not
> hogging resources.
> 
> There are use-cases where a client needs exclusive access to the
> channel... no other client should be able to send a message in the
> middle of some 'atomic-sequence' that is already in progress. Which is
> not possible if the API worked promiscuously. Whereas if the platform
> wants it could still emulate the 'server' model(multiple clients per
> channel) by having a global client submitting messages on behalf of
> other drivers. Again, pls read old threads.

If this server model suits multiple consumers, why would it not suit ...
multiple consumers with cooperative channel sharing?  It's not common
practice in the kernel to continually request/release exclusive access
to a resource.  Exposing a model that encourages this is not a good
idea either as it is very racy, depending on what drivers you have
compiled in.

> >> +
> >> +/**
> >> + * struct ipc_link - s/w representation of a communication link
> >> + * @link_name: Literal name assigned to the link. Physically
> >> + *    identical channels may have the same name.
> >
> > I do not think "physically identical" is enough here.  One would need to
> > know that the endpoint does not care about which channel you use.
> > Unfortunately, I think that this would be known more by the client, who
> > is attempting to communicate with the other side, then the controller,
> >
> That part is what 'link_data' is for (provided by the client and
> passed onto the controller).
> 
> 'physically identical' is for controller driver and implies if the
> links are equally capable. For example the PL320 has identical links.
> A properly written PL320 driver will manage links as floating
> resources and assign whichever is free. If the platform makes some
> assumptions that need to be communicated via link_data.

Here's the key. If the platform is making assumptions, then those
assumptions should be passed directly to the controller driver via
platform data, not through the channel request.

> >> + * @api_priv: hook for the API to map its private data on the link
> >> + *    Controller driver must not touch it.
> >> + */
> >> +struct ipc_link {
> >> +       char link_name[16];
> >> +       void *api_priv;
> >
> > There's no reason here to hide things from the controller; it is not
> > normal that we assume driver developers are malicious.  Documentation
> > describing that the data is for internal use only is sufficient.
> >
> It is common practice for an api to provide hooks for private data of
> lower layer.

Not in the kernel.

> >> +/**
> >> + * The controller driver registers its communication links to the
> >> + * global pool managed by the API.
> >> + */
> >> +int ipc_links_register(struct ipc_controller *ipc_con);
> >
> > I'm not sure about 'links' here, doesn't this function actually register
> > a controller, including its links?  Perhaps ipc_controller_register() ?
> >
> OK, I'll rename it to ipc_controller_register()

I should also mention that this should be attached to a device, as well.
Whether you put a pointer in the ipc_controller struct or in the function
parameters is irrelevant.

> >> +/**
> >> + * The controller the 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 ipc_link_txdone(struct ipc_link *link, enum xfer_result r);
> >
> > Representing this all as a state machine seems odd, as it removes
> > considerable usefulness of the API if the controllers (and in the
> > polling case, the clients) need to know about the state machine.
> >
> This is an _important_ part of API. The controller driver(usually from
> irq) need to notify the API that the last tx is done. Please read the
> code and make yourself aware of the limitations/functionality of
> controllers and protocols out there.

I understand the usefulness of this function call in your design, I was
more commenting on how it is exposed, and especially how it is described
in comment.  It would be nicer if the internal implementation details weren't
necessary to understand to interpret how the API should be used.

> >> +/**
> >> + * Purge the links from the global pool maintained by the API.
> >> + */
> >> +void ipc_links_unregister(struct ipc_controller *ipc_con);
> >> +
> >> +#endif /* __MAILBOX_CONTROLLER_H */
> >
> >
> > I have added relevant parties from other series related to this.
> >
> > Although I will admit that I didn't go looking at random code on the
> > internet before submitting my series, I did take a look at the previous
> > iterations of this submitted for upstream.
> >
> But you objections (except cosmetic ones) sound like you are not aware
> of all use-cases that have been taken care of.

Considering most of my comments are actually cosmetic?

Granted, I do have some concerns about use-cases which I am not convinced are
valid, and I do think that these should be challenged before upstreaming
code to support them.  Even if the use-cases are valid, it might still
be worth discarding them, if we find that they are unique and could
theoretically be implemented better elsewhere.

> >  I tried to take the comments
> > and considerations in mind when designing my version of this framework.
> >
> > I have tried to be fair with this code review, but as you can see, most
> > of the issues I have are resolved in my series.  It is possible that I
> > have missed something, as I was not privy to the off-list conversations
> > about this particular code, but other than tx-done callbacks and
> > polling--which I'm not convinced are necessary--it would appear that my
> > series (with the async work) covers all of the intended use-cases here.
> > Obviously I am somewhat biased, so I welcome arguments otherwise.
> >
> Thanks for reviewing the code so critically, but it would have been
> much greater help if you had explained if/how your controller/protocol
> doesn't work with this api.

DT support...

My main complaint is not that this driver won't work, but rather that
the architecture/design for it doesn't fit with similar APIs.

-Courtney

Re: [PATCHv3 2/6] mailbox: Introduce a new common API

[Kumar Gala]

On Feb 15, 2014, at 12:25 PM, Jassi Brar <jassisinghbrar@gmail.com> 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>
> ---
> drivers/mailbox/Makefile           |   4 +
> drivers/mailbox/mailbox.c          | 534 +++++++++++++++++++++++++++++++++++++
> include/linux/mailbox.h            |  17 ++
> include/linux/mailbox_client.h     |  87 ++++++
> include/linux/mailbox_controller.h | 102 +++++++
> 5 files changed, 744 insertions(+)
> 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

What’s the intent of trying to provide a unified interface here?  I’m trying to understand what benefit we are going for, I get possibly wanting something to reduce duplication in drivers (help functions, library, etc).  But do we really see benefit in a common interface for clients?

Are we really going to mix a OMAP mailbox controller with a client developed for some other SoC vendor?

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCHv3 2/6] mailbox: Introduce a new common API

[Jassi Brar]

Hi Kumar,

On 19 February 2014 03:02, Kumar Gala <galak@codeaurora.org> wrote:
>
> On Feb 15, 2014, at 12:25 PM, Jassi Brar <jassisinghbrar@gmail.com> 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>
>> ---
>> drivers/mailbox/Makefile           |   4 +
>> drivers/mailbox/mailbox.c          | 534 +++++++++++++++++++++++++++++++++++++
>> include/linux/mailbox.h            |  17 ++
>> include/linux/mailbox_client.h     |  87 ++++++
>> include/linux/mailbox_controller.h | 102 +++++++
>> 5 files changed, 744 insertions(+)
>> 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
>
> What's the intent of trying to provide a unified interface here?  I'm trying to understand what benefit we are going for, I get possibly wanting something to reduce duplication in drivers (help functions, library, etc).  But do we really see benefit in a common interface for clients?
>
> Are we really going to mix a OMAP mailbox controller with a client developed for some other SoC vendor?
>
Yeah :)  we may never a client reused on some other platform and that
is why I didn't yet bother implementing DT bindings to map random
clients onto mailboxes. Clients are going to be _very_ specific to
platforms(controller + remote f/w) and so we have the 'luxury' of
hardcoding that info(exact id of the mailbox) into client's code.

 Apart from controllers and clients sharing helper functions in the
api, we'll also soon run into various platforms using the same mailbox
controller (or a slightly modified version) and having to support
completely different protocols, and hence clients. Clearly we don't
want to have pl320_highbank.c, pl320_abc.c and pl320_xyz.c for three
different platforms. So we need the common/shared code to also help a
controller driver to behave differently on different platforms
(clients could pass platform specific info to the common controller
driver).

-jassi

Re: [PATCHv3 2/6] mailbox: Introduce a new common API

[Jassi Brar]

On 19 February 2014 01:17, Courtney Cavin <courtney.cavin@sonymobile.com> wrote:
> On Tue, Feb 18, 2014 at 08:06:55AM +0100, Jassi Brar wrote:
>>
>> >> +void *ipc_request_channel(struct ipc_client *cl)
>> >> +{
> [...]
>> >> +                       spin_lock_irqsave(&chan->lock, flags);
>> >> +                       chan->msg_free = 0;
>> >> +                       chan->msg_count = 0;
>> >> +                       chan->active_req = NULL;
>> >> +                       chan->rxcb = cl->rxcb;
>> >> +                       chan->txcb = cl->txcb;
>> >> +                       chan->cl_id = cl->cl_id;
>> >> +                       chan->assigned = true;
>> >> +                       chan->tx_block = cl->tx_block;
>> >
>> > Copying all of this data is unnecessary, and prone to error.  Just point
>> > to 'cl' in the client struct.
>> >
>> That did occur to me. But I chose to have controller specify its
>> attributes from lower layer and the client from upper layer. The api
>> basically works on channels... which get their
>> functionality/limitations from controllers+clients and save them in
>> their own structure. That also helps 'submit message and moveon' type
>> clients.
>
> I understand how this currently works, but I see no reason why one
> couldn't simply point to the client struct here, instead of duplicating
> the client definition, and copying the data.
>
> I don't see an argument for fire-and-forget clients here.  The lifetime
> of the callback and private data must persist, so why not the containing
> struct?
>
>> >> +                       if (!cl->tx_tout)
>> >> +                               chan->tx_tout = msecs_to_jiffies(3600000);
>> >
>> > An hour!?  Arbitrary, and extremely long.  Either this should be
>> > indefinite, or there should be a reasonably small value here.
>> >
>> Ideally the client _should_ specify the timeout. Here the default 1hr
>> is infinity, roughly speaking.
>> What value do you suggest?
>
> An hour is by definition uncountably far from infinity.  The fact that
> this may seem like it is infinite in a test setup indicates that this
> will cause problems in the future, when/if the timeout occurs.  I
> suggest using wait_for_completion() if this is 0, or alternatively a
> timeout short enough to trigger easily via testing (e.g. 1 second).
>
>> >> +int ipc_links_register(struct ipc_controller *ipc)
>> >> +{
>> >> +       int i, num_links, txdone;
>> >> +       struct ipc_chan *chan;
>> >> +       struct ipc_con *con;
>> >> +
>> >> +       /* Sanity check */
>> >> +       if (!ipc || !ipc->ops)
>> >
>> > You probably want to check for !ipc->links here too, as you immediately
>> > dereference it.
>> >
>> Yeah, right. Thanks.
>>
>> >> +               return -EINVAL;
>> >> +
>> >> +       for (i = 0; ipc->links[i]; i++)
>> >
>> > So you require a NULL node at the end?  Why not have a nlinks/num_links
>> > in the controller struct?  It would save having to count here.
>> >
>> Rather I find that noisy. Why add variables that we can do without?
>> Especially when the variable would be used just once.
>
> Just like the marker link you need at the end of your array?  It's more
> flexible, and easier to understand from an API perspective.
>
>> >> +
>> >> +       chan = (void *)con + sizeof(*con);
>> >> +       for (i = 0; i < num_links; i++) {
>> >> +               chan[i].con = con;
>> >> +               chan[i].assigned = false;
>> >> +               chan[i].link_ops = ipc->ops;
>> >> +               chan[i].link = ipc->links[i];
>> >> +               chan[i].txdone_method = txdone;
>> >> +               chan[i].link->api_priv = &chan[i];
>> >> +               spin_lock_init(&chan[i].lock);
>> >> +               BLOCKING_INIT_NOTIFIER_HEAD(&chan[i].avail);
>> >> +               list_add_tail(&chan[i].node, &con->channels);
>> >
>> > Why not have all of this data exposed in the link definition, so you
>> > don't need this list, and can represent it as a pre-populated array?
>> >
>> Because the api choose to work only in terms of ipc channels. Infact
>> in previous revision there was just one global pool of channels that
>> the api managed. Suman Anna insisted we manage channels in controllers
>> so it makes searching easier.
>
> "Legacy reasons" is not a good argument here.
>
The reason is not legacy... its that a developer of another platform
suggested a way to make searching a bit more efficient.

>> >> +/**
>> >> + * struct ipc_client - User of a mailbox
>> >> + * @chan_name: the "controller:channel" this client wants
>> >
>> > Uh, not a consumer name?  See comment on ipc_request_channel().
>> >
>> Yeah no consumer name. The controller driver gets to name its
>> channels, a client simply uses them.
>
> Yes.  I simply don't think this conforms to "typical" kernel frameworks.
>
Are you sure you are talking about the Linux kernel here?!
 Go look how dmaengine names the channels internally for example. Go
look into ASoC code how the controllers' and codec chips' names are
not decided by the machine drivers(clients) that use them.

>> > Additionally, is there an actual use-case for having an async tx-done
>> > callback?
>> >
>> Yes, some client might need to submit a 'beacon' type message.
>
> Wouldn't this categorize as condition 'b' below?
>
My implementation does support async-tx and async doesn't mean client
doesn't care.

>> >         a) calling the blocking API, since you would have to manage that
>> >            in your own context, just like tx-done
>> >         b) not caring about tx-done?  It seems your code supports this.
> [...]
>> >> +       bool knows_txdone;
>> >
>> > If the client is running the state-machine, this is neither a mailbox,
>> > nor much of a useful framework for whatever it is the client has
>> > implemented.  Could you please explain the motivation here?
>> >
>> You could have read the code and previous threads that had lots of
>> use-cases and rationale explained.
>>
>> Anyways... consider my _real_ use-case that has come to use it.
>>  My IPC controller can not generate interrupt for TX-Done (when the
>> message/reply was read by the remote), but it does interrupt when the
>> remote has sent a message/reply. The client does not run the
>> state-machine, but it sure can save us from polling for TX-Done when
>> it know the received packet is reply to its last TX... so it tells the
>> API the last TX was done and the callback is done much sooner than the
>> next poll. This feature is solely responsible for keeping the IPC
>> latency within acceptable limits for my platform.
>
> It sounds to me that at least part of the wire protocol over which your
> client communicates is the actual mailbox here, and thus should be
> represented in the controller driver itself.  What's underneath seems
> more like some generic fifo without any useful mailbox bits.
>
Please don't assume you know my hardware better than I do. The TX-poll
is done by checking the remote has cleared-off the message register.
The f/w from another source might not even do that and the only way to
detect TX-done would be the client receiving a reply.

>> >> +       void *link_data;
>> >
>> > Random opaque data to pass straight through the framework?  This seems like
>> > a hack.  If this is needed, clearly the framework has not implemented
>> > something generic enough.
>> >
>> Nopes! I strongly suggest you read previous threads.
>>
>> Here's the rant... in IPC a client doesn't behave only according to
>> the underlying controller. But to the combination of controller plus
>> the remote f/w(or the protocol).  For example, one protocol may
>> specify the remote will 'clear' the packet-location after reading it
>> while another protocol will not clear it but sends a particular
>> reply-packet. Since we can not have two drivers for the same
>> controller, someone has to tell the controller driver how to behave
>> for the current protocol... and that 'someone' can only be the client.
>> There can be no generic code that could enumerate all possible
>> behaviors.
>
> Please stop describing this as generic IPC, because it is far from that.
> What you are describing is the difference between a driver for a
> chip/block, and a wire protocol to use across with/it.  In many cases,
> these are completely separate implementations, but they can come in many
> different structures:
>         - Single driver for generic mailbox controller
>         - Driver for bus communication + driver for controller (e.g over i2c)
>         - Driver for bus communication +
>           driver for packeted data + driver for controller
>
> The "controller" in the case of this driver should be exposing whatever
> combination makes it clearly defined as a "mailbox provider". This is
> why it is so very important that this API represents mailboxes.
>
Don't worry, what's important has been done. 5 platforms have drivers
for it (albeit closed so far). And you are still to tell me how this
api wouldn't work for your controller?


>> >> +/**
>> >> + * The Client specifies its requirements and capabilities while asking for
>> >> + * a channel/mailbox 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 ipc_free_channel.
>> >> + */
>> >> +void *ipc_request_channel(struct ipc_client *cl);
>> >
>> > Greg has already commented on the return type here, but there are a few
>> > further things I don't quite understand.
>> >
>> > The lookup key here is the name, which is embedded in the struct.
>> > While most of the struct seems to define the client, the name defines
>> > something which seems to be an attribute of the controller.
>> >
>> I am not sure you understand. The client specifies which channel of
>> which controller does it want. The api keeps channels hooked up in
>> controller bunch. And can figure out which channel to return.
>
> I do understand.  My point is that naming the channels/links from the
> controller isn't the normal method for doing this exact thing.  There's
> no reason to expect that a controller can name its channels better than
> a consumer can.  Neither should we expect that a client should know the
> name of a channel without a binding of some sort.
>
> What I'm recommending here is simply to use the same procedure here many
> of the existing frameworks do:
>         - controller maintains indexed list of channels
>         - client device maintains name -> index mappings (from bindings)
>         - client device maintains reference to controller (bindings)
>
> And for platforms without DT support, a global lookup-list similar to
> that in regulators and PWM frameworks, based on client device name (and
> consumer id).
>
>> >
>> > This would also map very cleanly into devicetree bindings, which are
>> > really needed for this framework.  I would also like to see a
>> > devm_ variant of this.
>> >
>> Yeah we need to move onto device tree mappings if things gets fancy.
>> Right now we all had simple clients and unique controllers.
>
> Please don't underestimate the importance of devicetree support here.
> Without it, most new platform/arch support will not be able to use this
> framework.
>
I don't underestimate, I just don't see the pressing need from
platform-specific client drivers. I am OK with DT bindings though.

>> If you problem is with the 'light-weight' part, I'll remove it. But
>> still the clients will behave just as their use-cases are!
>>
>> > With this in mind, I also don't believe that it would be a good idea to
>> > avoid holding a channel, as having drivers "trade" channels can very
>> > easily result in nasty race conditions.
>> >
>> There are use-cases that require holding onto a channel (server model)
>> and there are use-cases that require a client to relieve a channel
>> immediately after it's done. Seems you are not aware of use-case out
>> there in the wild (pls read old threads).
>
> Or perhaps I just disagree that these are valid use-cases....
>
OK I take it on record that I presented you with my use-case (server
model) that's needed to make the platform work.... and you declare it
'invalid'.
Care to grep for QUIRK in kernel ?


>> > or the API needs to support having multiple clients per-channel.
>> >
>> For platforms with 2 or more clients and each client having to do it's
>> own transfer... a channel may be busy when a client needs it. So
>> instead of implementing its own poll mechanism for availability of the
>> channel, the client could (un)register to be notified when a channel
>> become available.
>> BTW, precisely on such platforms a client would like to relieve a
>> channel as soon as its done... which is anyway a good practice of not
>> hogging resources.
>>
>> There are use-cases where a client needs exclusive access to the
>> channel... no other client should be able to send a message in the
>> middle of some 'atomic-sequence' that is already in progress. Which is
>> not possible if the API worked promiscuously. Whereas if the platform
>> wants it could still emulate the 'server' model(multiple clients per
>> channel) by having a global client submitting messages on behalf of
>> other drivers. Again, pls read old threads.
>
> If this server model suits multiple consumers, why would it not suit ...
> multiple consumers with cooperative channel sharing?  It's not common
> practice in the kernel to continually request/release exclusive access
> to a resource.
>
Again, are you sure you are talking about the Linux kernel?
 IRQs, DmaChannels, GPIOs and almost every resource is taken and
released on need basis! It's wrong to hold onto resources when they
are not needed.  What do you think about RPM?

>> >> +
>> >> +/**
>> >> + * struct ipc_link - s/w representation of a communication link
>> >> + * @link_name: Literal name assigned to the link. Physically
>> >> + *    identical channels may have the same name.
>> >
>> > I do not think "physically identical" is enough here.  One would need to
>> > know that the endpoint does not care about which channel you use.
>> > Unfortunately, I think that this would be known more by the client, who
>> > is attempting to communicate with the other side, then the controller,
>> >
>> That part is what 'link_data' is for (provided by the client and
>> passed onto the controller).
>>
>> 'physically identical' is for controller driver and implies if the
>> links are equally capable. For example the PL320 has identical links.
>> A properly written PL320 driver will manage links as floating
>> resources and assign whichever is free. If the platform makes some
>> assumptions that need to be communicated via link_data.
>
> Here's the key. If the platform is making assumptions, then those
> assumptions should be passed directly to the controller driver via
> platform data, not through the channel request.
>
Of course controller driver could take info from platform data... but
not all. There maybe some dynamic configuration needed depending on
the remotes status.

>> >> + * @api_priv: hook for the API to map its private data on the link
>> >> + *    Controller driver must not touch it.
>> >> + */
>> >> +struct ipc_link {
>> >> +       char link_name[16];
>> >> +       void *api_priv;
>> >
>> > There's no reason here to hide things from the controller; it is not
>> > normal that we assume driver developers are malicious.  Documentation
>> > describing that the data is for internal use only is sufficient.
>> >
>> It is common practice for an api to provide hooks for private data of
>> lower layer.
>
> Not in the kernel.
>
Please don't troll!   Just grep  for private_data, priv_data and count hits!


>> >
>> > I have added relevant parties from other series related to this.
>> >
>> > Although I will admit that I didn't go looking at random code on the
>> > internet before submitting my series, I did take a look at the previous
>> > iterations of this submitted for upstream.
>> >
>> But you objections (except cosmetic ones) sound like you are not aware
>> of all use-cases that have been taken care of.
>
> Considering most of my comments are actually cosmetic?
>
> Granted, I do have some concerns about use-cases which I am not convinced are
> valid, and I do think that these should be challenged before upstreaming
> code to support them.  Even if the use-cases are valid, it might still
> be worth discarding them, if we find that they are unique and could
> theoretically be implemented better elsewhere.
>
Denying doesn't help. The 'inconvenient' platforms and use-cases are
out there... what do we say? "Linux won't support your platform... go
fire your designers and come back with a platform that's easy on my
api?"
And we are not talking about supporting some QUIRK mechanism. We
layout 'all' possible combinations of the IPC chain and try to support
them all.

Re: [PATCHv3] Generic Mailbox API

[Jassi Brar]

On Sun, Feb 16, 2014 at 2:22 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> Hello,
>
>   Here is what the generic mailbox api looks like after modifications
> to make it work with 5 different platforms.
>  Major feedback from Suman Anna(TI), LeyFoon Tan(Intel),
> Craig McGeachie(Broadcom) and Loic Pallardy(ST).
>
If there are no more comments, I plan to implement suggestion on this
submission and issue v4 early next week (right now I am at Linaro
Connect).

Thanks
Jassi