[PATCHv10 0/4] Common Mailbox Framework

[PATCHv10 0/4] Common Mailbox Framework

[Jassi Brar]

Hello,
 Here's v10. Not much different from v9. The code and the generic
bindings looked good enough to relevant gods, but the patchset still
needs some formal loving in the form of Acked-by's.

Changes since v9:
 o Purely cosmetic changes - added headers and used BIT() 

Changes since v8:
 o Nits like spelling corrections and a couple symbol renames
 o Made 'mbox-names' optional in favor of index of mailbox
   specifier in 'mboxes' property. Which results in
     mbox_request_channel(struct mbox_client *cl)
        to
     mbox_request_channel(struct mbox_client *cl, int index)
 o Separate out Documentation and Bindings patches.

Changes since v7:
 o Added documentation and example usage.
 o Merged all patches into one that create api, bindings and
   documentation.

Changes since v6:
 o Separate out generic DT bindings patch.
 o Discard unnecessary aligned attributes.

Changes since v5:
 o Use standard error types instead of special type mbox_result.
 o Constify client struct in request_channel
 o Use reinit_completion instead of init_completion every time.
 o Improve commentary in bindings and code.

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

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

Thanks
-Jassi

Jassi Brar (3):
  mailbox: Introduce framework for mailbox
  doc: add documentation for mailbox framework
  dt: mailbox: add generic bindings

Suman Anna (1):
  mailbox: rename pl320-ipc specific mailbox.h

 .../devicetree/bindings/mailbox/mailbox.txt        |  36 ++
 Documentation/mailbox.txt                          | 122 ++++++
 MAINTAINERS                                        |   8 +
 arch/arm/mach-highbank/highbank.c                  |   2 +-
 drivers/cpufreq/highbank-cpufreq.c                 |   2 +-
 drivers/mailbox/Makefile                           |   4 +
 drivers/mailbox/mailbox.c                          | 466 +++++++++++++++++++++
 drivers/mailbox/pl320-ipc.c                        |   2 +-
 include/linux/mailbox_client.h                     |  46 ++
 include/linux/mailbox_controller.h                 | 135 ++++++
 include/linux/{mailbox.h => pl320-ipc.h}           |   0
 11 files changed, 820 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mailbox/mailbox.txt
 create mode 100644 Documentation/mailbox.txt
 create mode 100644 drivers/mailbox/mailbox.c
 create mode 100644 include/linux/mailbox_client.h
 create mode 100644 include/linux/mailbox_controller.h
 rename include/linux/{mailbox.h => pl320-ipc.h} (100%)

-- 
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv10 3/4] doc: add documentation for mailbox framework

[Jassi Brar]

 Some explanations with examples of how to write to implement users
and providers of the mailbox framework.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 Documentation/mailbox.txt | 122 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)
 create mode 100644 Documentation/mailbox.txt

diff --git a/Documentation/mailbox.txt b/Documentation/mailbox.txt
new file mode 100644
index 0000000..60f43ff
--- /dev/null
+++ b/Documentation/mailbox.txt
@@ -0,0 +1,122 @@
+		The Common Mailbox Framework
+		Jassi Brar <jaswinder.singh@linaro.org>
+
+ This document aims to help developers write client and controller
+drivers for the API. But before we start, let us note that the
+client (especially) and controller drivers are likely going to be
+very platform specific because the remote firmware is likely to be
+proprietary and implement non-standard protocol. So even if two
+platforms employ, say, PL320 controller, the client drivers can't
+be shared across them. Even the PL320 driver might need to accommodate
+some platform specific quirks. So the API is meant mainly to avoid
+similar copies of code written for each platform. Having said that,
+nothing prevents the remote f/w to also be Linux based and use the
+same api there. However none of that helps us locally because we only
+ever deal at client's protocol level.
+ Some of the choices made during implementation are the result of this
+peculiarity of this "common" framework.
+
+
+
+	Part 1 - Controller Driver (See include/linux/mailbox_controller.h)
+
+ Allocate mbox_controller and the array of mbox_chan.
+Populate mbox_chan_ops, except peek_data() all are mandatory.
+The controller driver might know a message has been consumed
+by the remote by getting an IRQ or polling some hardware flag
+or it can never know (the client knows by way of the protocol).
+The method in order of preference is IRQ -> Poll -> None, which
+the controller driver should set via 'txdone_irq' or 'txdone_poll'
+or neither.
+
+
+	Part 2 - Client Driver (See include/linux/mailbox_client.h)
+
+ The client might want to operate in blocking mode (synchronously
+send a message through before returning) or non-blocking/async mode (submit
+a message and a callback function to the API and return immediately).
+
+
+struct demo_client {
+	struct mbox_client cl;
+	struct mbox_chan *mbox;
+	struct completion c;
+	bool async;
+	/* ... */
+};
+
+/*
+ * This is the handler for data received from remote. The behaviour is purely
+ * dependent upon the protocol. This is just an example.
+ */
+static void message_from_remote(struct mbox_client *cl, void *mssg)
+{
+	struct demo_client *dc = container_of(mbox_client,
+						struct demo_client, cl);
+	if (dc->aysnc) {
+		if (is_an_ack(mssg)) {
+			/* An ACK to our last sample sent */
+			return; /* Or do something else here */
+		} else { /* A new message from remote */
+			queue_req(mssg);
+		}
+	} else {
+		/* Remote f/w sends only ACK packets on this channel */
+		return;
+	}
+}
+
+static void sample_sent(struct mbox_client *cl, void *mssg, int r)
+{
+	struct demo_client *dc = container_of(mbox_client,
+						struct demo_client, cl);
+	complete(&dc->c);
+}
+
+static void client_demo(struct platform_device *pdev)
+{
+	struct demo_client *dc_sync, *dc_async;
+	/* The controller already knows async_pkt and sync_pkt */
+	struct async_pkt ap;
+	struct sync_pkt sp;
+
+	dc_sync = kzalloc(sizeof(*dc_sync), GFP_KERNEL);
+	dc_async = kzalloc(sizeof(*dc_async), GFP_KERNEL);
+
+	/* Populate non-blocking mode client */
+	dc_async->cl.dev = &pdev->dev;
+	dc_async->cl.rx_callback = message_from_remote;
+	dc_async->cl.tx_done = sample_sent;
+	dc_async->cl.tx_block = false;
+	dc_async->cl.tx_tout = 0; /* doesn't matter here */
+	dc_async->cl.knows_txdone = false; /* depending upon protocol */
+	dc_async->async = true;
+	init_completion(&dc_async->c);
+
+	/* Populate blocking mode client */
+	dc_sync->cl.dev = &pdev->dev;
+	dc_sync->cl.rx_callback = message_from_remote;
+	dc_sync->cl.tx_done = NULL; /* operate in blocking mode */
+	dc_sync->cl.tx_block = true;
+	dc_sync->cl.tx_tout = 500; /* by half a second */
+	dc_sync->cl.knows_txdone = false; /* depending upon protocol */
+	dc_sync->async = false;
+
+	/* ASync mailbox is listed second in 'mboxes' property */
+	dc_async->mbox = mbox_request_channel(&dc_async->cl, 1);
+	/* Populate data packet */
+	/* ap.xxx = 123; etc */
+	/* Send async message to remote */
+	mbox_send_message(dc_async->mbox, &ap);
+
+	/* Sync mailbox is listed first in 'mboxes' property */
+	dc_sync->mbox = mbox_request_channel(&dc_sync->cl, 0);
+	/* Populate data packet */
+	/* sp.abc = 123; etc */
+	/* Send message to remote in blocking mode */
+	mbox_send_message(dc_sync->mbox, &sp);
+	/* At this point 'sp' has been sent */
+
+	/* Now wait for async chan to be done */
+	wait_for_completion(&dc_async->c);
+}
-- 
1.8.1.2

[PATCHv10 4/4] dt: mailbox: add generic bindings

[Jassi Brar]

Define generic bindings for the framework clients to
request mailbox channels.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 .../devicetree/bindings/mailbox/mailbox.txt        | 36 ++++++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/mailbox.txt

diff --git a/Documentation/devicetree/bindings/mailbox/mailbox.txt b/Documentation/devicetree/bindings/mailbox/mailbox.txt
new file mode 100644
index 0000000..bb79678
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/mailbox.txt
@@ -0,0 +1,36 @@
+* Generic Mailbox Controller and client driver bindings
+
+Generic binding to provide a way for Mailbox controller drivers to
+assign appropriate mailbox channel to client drivers.
+
+* Mailbox Controller
+
+Required property:
+- #mbox-cells: Must be at least 1. Number of cells in a mailbox
+		specifier.
+
+Example:
+	mailbox: mailbox {
+		...
+		#mbox-cells = <1>;
+	};
+
+
+* Mailbox Client
+
+Required property:
+- mboxes: List of phandle and mailbox channel specifiers.
+
+Optional property:
+- mbox-names: List of identifier strings for each mailbox channel
+		required by the client. The use of this property
+		is discouraged in favor of using index in list of
+		'mboxes' while requesting a mailbox.
+
+Example:
+	pwr_cntrl: power {
+		...
+		mbox-names = "pwr-ctrl", "rpc";
+		mboxes = <&mailbox 0
+			&mailbox 1>;
+	};
-- 
1.8.1.2

Re: [PATCHv10 4/4] dt: mailbox: add generic bindings

[Mark Brown]

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

On Fri, Aug 01, 2014 at 06:02:45PM +0530, Jassi Brar wrote:

> +Optional property:
> +- mbox-names: List of identifier strings for each mailbox channel
> +		required by the client. The use of this property
> +		is discouraged in favor of using index in list of
> +		'mboxes' while requesting a mailbox.

Can I suggest that if we do want to avoid strings for some reason (it's
not obvious to me why) then we include a recommendation to provide
defines in a header file in order to improve legibility.  That's pretty
minor, though.

Reviewed-by: Mark Brown <broonie-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

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

Re: [PATCHv10 4/4] dt: mailbox: add generic bindings

[Jassi Brar]

On 2 August 2014 00:13, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Aug 01, 2014 at 06:02:45PM +0530, Jassi Brar wrote:
>
>> +Optional property:
>> +- mbox-names: List of identifier strings for each mailbox channel
>> +             required by the client. The use of this property
>> +             is discouraged in favor of using index in list of
>> +             'mboxes' while requesting a mailbox.
>
> Can I suggest that if we do want to avoid strings for some reason (it's
> not obvious to me why)
>
The string and index are both to be taken from DT node of client.

    pwr_cntrl: power {
               ...
               mbox-names = "pwr-ctrl", "rpc";
               mboxes = <&mailbox 0
                       &mailbox 1>;
    };

The xlate() will map string onto index. So rather than the client
driver specify, say, "rpc" while requesting mailbox channel, it can
directly specify index 1.

> then we include a recommendation to provide
> defines in a header file in order to improve legibility.  That's pretty
> minor, though.
>
Sorry I don't understand.

> Reviewed-by: Mark Brown <broonie@linaro.org>
>
Thanks a lot!
-Jassi

Re: [PATCHv10 4/4] dt: mailbox: add generic bindings

[Mark Brown]

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

On Sat, Aug 02, 2014 at 01:17:18PM +0530, Jassi Brar wrote:
> On 2 August 2014 00:13, Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Aug 01, 2014 at 06:02:45PM +0530, Jassi Brar wrote:

> > Can I suggest that if we do want to avoid strings for some reason (it's
> > not obvious to me why)

> The string and index are both to be taken from DT node of client.

>     pwr_cntrl: power {
>                ...
>                mbox-names = "pwr-ctrl", "rpc";
>                mboxes = <&mailbox 0
>                        &mailbox 1>;
>     };

> The xlate() will map string onto index. So rather than the client
> driver specify, say, "rpc" while requesting mailbox channel, it can
> directly specify index 1.

The whole point with specifying an array of names as well as the array
of mboxes is that you don't then need to do that in the driver code, it
provides a layer of indirection between the array indexes and the
identification of the resources.  This improves the legibility of the
driver (since you're asking for things by name rather than random
number) and DT (since there's a bit less of counting through potentially
long lists and making sure you edit the right entry.  It also makes life
easier if a future version of the binding allows one of the existing
mailboxes to be omitted since you don't need to have blank entries.

> > then we include a recommendation to provide
> > defines in a header file in order to improve legibility.  That's pretty
> > minor, though.

> Sorry I don't understand.

If there's defines in the DT header files (which would hopefully go
along with the bindings in some future where we actually split the
bindings out) then the driver can use those names rather than having
numbers in it.

While I think about it it would be good to be able to encourage people
to have things like <&mailbox 0> being able to be written as <&mailbox
NAME>.

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

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

[Jassi Brar]

From: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>

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-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
---
 arch/arm/mach-highbank/highbank.c        | 2 +-
 drivers/cpufreq/highbank-cpufreq.c       | 2 +-
 drivers/mailbox/pl320-ipc.c              | 2 +-
 include/linux/{mailbox.h => pl320-ipc.h} | 0
 4 files changed, 3 insertions(+), 3 deletions(-)
 rename include/linux/{mailbox.h => pl320-ipc.h} (100%)

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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Brown]

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

On Fri, Aug 01, 2014 at 06:00:57PM +0530, Jassi Brar wrote:
> 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.

Reviewed-by: Mark Brown <broonie@linaro.org>

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

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

[Jassi Brar]

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

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

Signed-off-by: Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 MAINTAINERS                        |   8 +
 drivers/mailbox/Makefile           |   4 +
 drivers/mailbox/mailbox.c          | 466 +++++++++++++++++++++++++++++++++++++
 include/linux/mailbox_client.h     |  46 ++++
 include/linux/mailbox_controller.h | 135 +++++++++++
 5 files changed, 659 insertions(+)
 create mode 100644 drivers/mailbox/mailbox.c
 create mode 100644 include/linux/mailbox_client.h
 create mode 100644 include/linux/mailbox_controller.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 95990dd..7d3bd89 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5672,6 +5672,14 @@ S:	Maintained
 F:	drivers/net/macvlan.c
 F:	include/linux/if_macvlan.h
 
+MAILBOX API
+M:	Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+L:	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+S:	Maintained
+F:	drivers/mailbox/
+F:	include/linux/mailbox_client.h
+F:	include/linux/mailbox_controller.h
+
 MAN-PAGES: MANUAL PAGES FOR LINUX -- Sections 2, 3, 4, 5, and 7
 M:	Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
 W:	http://www.kernel.org/doc/man-pages
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..d026dba
--- /dev/null
+++ b/drivers/mailbox/mailbox.c
@@ -0,0 +1,466 @@
+/*
+ * Mailbox: Common code for Mailbox controllers and users
+ *
+ * Copyright (C) 2013-2014 Linaro Ltd.
+ * Author: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/spinlock.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/bitops.h>
+#include <linux/mailbox_client.h>
+#include <linux/mailbox_controller.h>
+
+#define TXDONE_BY_IRQ	BIT(0) /* controller has remote RTR irq */
+#define TXDONE_BY_POLL	BIT(1) /* controller can read status of last TX */
+#define TXDONE_BY_ACK	BIT(2) /* S/W ACK recevied by Client ticks the TX */
+
+static LIST_HEAD(mbox_cons);
+static DEFINE_MUTEX(con_mutex);
+
+static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
+{
+	int idx;
+	unsigned long flags;
+
+	spin_lock_irqsave(&chan->lock, flags);
+
+	/* See if there is any space left */
+	if (chan->msg_count == MBOX_TX_QUEUE_LEN) {
+		spin_unlock_irqrestore(&chan->lock, flags);
+		return -ENOBUFS;
+	}
+
+	idx = chan->msg_free;
+	chan->msg_data[idx] = mssg;
+	chan->msg_count++;
+
+	if (idx == MBOX_TX_QUEUE_LEN - 1)
+		chan->msg_free = 0;
+	else
+		chan->msg_free++;
+
+	spin_unlock_irqrestore(&chan->lock, flags);
+
+	return idx;
+}
+
+static void msg_submit(struct mbox_chan *chan)
+{
+	unsigned count, idx;
+	unsigned long flags;
+	void *data;
+	int err;
+
+	spin_lock_irqsave(&chan->lock, flags);
+
+	if (!chan->msg_count || chan->active_req)
+		goto exit;
+
+	count = chan->msg_count;
+	idx = chan->msg_free;
+	if (idx >= count)
+		idx -= count;
+	else
+		idx += MBOX_TX_QUEUE_LEN - count;
+
+	data = chan->msg_data[idx];
+
+	/* Try to submit a message to the MBOX controller */
+	err = chan->mbox->ops->send_data(chan, data);
+	if (!err) {
+		chan->active_req = data;
+		chan->msg_count--;
+	}
+exit:
+	spin_unlock_irqrestore(&chan->lock, flags);
+}
+
+static void tx_tick(struct mbox_chan *chan, int 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 (mssg && chan->cl->tx_done)
+		chan->cl->tx_done(chan->cl, mssg, r);
+
+	if (chan->cl->tx_block)
+		complete(&chan->tx_complete);
+}
+
+static void poll_txdone(unsigned long data)
+{
+	struct mbox_controller *mbox = (struct mbox_controller *)data;
+	bool txdone, resched = false;
+	int i;
+
+	for (i = 0; i < mbox->num_chans; i++) {
+		struct mbox_chan *chan = &mbox->chans[i];
+
+		if (chan->active_req && chan->cl) {
+			resched = true;
+			txdone = chan->mbox->ops->last_tx_done(chan);
+			if (txdone)
+				tx_tick(chan, 0);
+		}
+	}
+
+	if (resched)
+		mod_timer(&mbox->poll, jiffies +
+				msecs_to_jiffies(mbox->period));
+}
+
+/**
+ * mbox_chan_received_data - A way for controller driver to push data
+ *				received from remote to the upper layer.
+ * @chan: Pointer to the mailbox channel on which RX happened.
+ * @mssg: Client specific message typecasted as void *
+ *
+ * After startup and before shutdown any data received on the chan
+ * is passed on to the API via atomic mbox_chan_received_data().
+ * The controller should ACK the RX only after this call returns.
+ */
+void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
+{
+	/* No buffering the received data */
+	if (chan->cl->rx_callback)
+		chan->cl->rx_callback(chan->cl, mssg);
+}
+EXPORT_SYMBOL_GPL(mbox_chan_received_data);
+
+/**
+ * mbox_chan_txdone - A way for controller driver to notify the
+ *			framework that the last TX has completed.
+ * @chan: Pointer to the mailbox chan on which TX happened.
+ * @r: Status of last TX - OK or ERROR
+ *
+ * The controller that has IRQ for TX ACK calls this atomic API
+ * to tick the TX state machine. It works only if txdone_irq
+ * is set by the controller.
+ */
+void mbox_chan_txdone(struct mbox_chan *chan, int r)
+{
+	if (unlikely(!(chan->txdone_method & TXDONE_BY_IRQ))) {
+		pr_err("Controller can't run the TX ticker\n");
+		return;
+	}
+
+	tx_tick(chan, r);
+}
+EXPORT_SYMBOL_GPL(mbox_chan_txdone);
+
+/**
+ * mbox_client_txdone - The way for a client to run the TX state machine.
+ * @chan: Mailbox channel assigned to this client.
+ * @r: Success status of last transmission.
+ *
+ * The client/protocol had received some 'ACK' packet and it notifies
+ * the API that the last packet was sent successfully. This only works
+ * if the controller can't sense TX-Done.
+ */
+void mbox_client_txdone(struct mbox_chan *chan, int r)
+{
+	if (unlikely(!(chan->txdone_method & TXDONE_BY_ACK))) {
+		pr_err("Client can't run the TX ticker\n");
+		return;
+	}
+
+	tx_tick(chan, r);
+}
+EXPORT_SYMBOL_GPL(mbox_client_txdone);
+
+/**
+ * mbox_client_peek_data - A way for client driver to pull data
+ *			received from remote by the controller.
+ * @chan: Mailbox channel assigned to this client.
+ *
+ * A poke to controller driver for any received data.
+ * The data is actually passed onto client via the
+ * mbox_chan_received_data()
+ * The call can be made from atomic context, so the controller's
+ * implementation of peek_data() must not sleep.
+ *
+ * Return: True, if controller has, and is going to push after this,
+ *          some data.
+ *         False, if controller doesn't have any data to be read.
+ */
+bool mbox_client_peek_data(struct mbox_chan *chan)
+{
+	if (chan->mbox->ops->peek_data)
+		return chan->mbox->ops->peek_data(chan);
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(mbox_client_peek_data);
+
+/**
+ * mbox_send_message -	For client to submit a message to be
+ *				sent to the remote.
+ * @chan: Mailbox channel assigned to this client.
+ * @mssg: Client specific message typecasted.
+ *
+ * For client to submit data to the controller destined for a remote
+ * processor. If the client had set 'tx_block', the call will return
+ * either when the remote receives the data or when 'tx_tout' millisecs
+ * run out.
+ *  In non-blocking mode, the requests are buffered by the API and a
+ * non-negative token is returned for each queued request. If the request
+ * is not queued, a negative token is returned. Upon failure or successful
+ * TX, the API calls 'tx_done' from atomic context, from which the client
+ * could submit yet another request.
+ * The pointer to message should be preserved until it is sent
+ * over the chan, i.e, tx_done() is made.
+ * This function could be called from atomic context as it simply
+ * queues the data and returns a token against the request.
+ *
+ * Return: Non-negative integer for successful submission (non-blocking mode)
+ *	or transmission over chan (blocking mode).
+ *	Negative value denotes failure.
+ */
+int mbox_send_message(struct mbox_chan *chan, void *mssg)
+{
+	int t;
+
+	if (!chan || !chan->cl)
+		return -EINVAL;
+
+	t = add_to_rbuf(chan, mssg);
+	if (t < 0) {
+		pr_err("Try increasing MBOX_TX_QUEUE_LEN\n");
+		return t;
+	}
+
+	msg_submit(chan);
+
+	reinit_completion(&chan->tx_complete);
+
+	if (chan->txdone_method	== TXDONE_BY_POLL)
+		poll_txdone((unsigned long)chan->mbox);
+
+	if (chan->cl->tx_block && chan->active_req) {
+		unsigned long wait;
+		int ret;
+
+		if (!chan->cl->tx_tout) /* wait forever */
+			wait = msecs_to_jiffies(3600000);
+		else
+			wait = msecs_to_jiffies(chan->cl->tx_tout);
+
+		ret = wait_for_completion_timeout(&chan->tx_complete, wait);
+		if (ret == 0) {
+			t = -EIO;
+			tx_tick(chan, -EIO);
+		}
+	}
+
+	return t;
+}
+EXPORT_SYMBOL_GPL(mbox_send_message);
+
+/**
+ * mbox_request_channel - Request a mailbox channel.
+ * @cl: Identity of the client requesting the channel.
+ * @index: Index of mailbox specifier in 'mboxes' property.
+ *
+ * The Client specifies its requirements and capabilities while asking for
+ * a mailbox channel. It can't be called from atomic context.
+ * The channel is exclusively allocated and can't be used by another
+ * client before the owner calls mbox_free_channel.
+ * After assignment, any packet received on this channel will be
+ * handed over to the client via the 'rx_callback'.
+ * The framework holds reference to the client, so the mbox_client
+ * structure shouldn't be modified until the mbox_free_channel returns.
+ *
+ * Return: Pointer to the channel assigned to the client if successful.
+ *		ERR_PTR for request failure.
+ */
+struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
+{
+	struct device *dev = cl->dev;
+	struct mbox_controller *mbox;
+	struct of_phandle_args spec;
+	struct mbox_chan *chan;
+	unsigned long flags;
+	int ret;
+
+	if (!dev || !dev->of_node) {
+		pr_debug("%s: No owner device node\n", __func__);
+		return ERR_PTR(-ENODEV);
+	}
+
+	mutex_lock(&con_mutex);
+
+	if (of_parse_phandle_with_args(dev->of_node, "mboxes",
+				       "#mbox-cells", index, &spec)) {
+		pr_debug("%s: can't parse \"mboxes\" property\n", __func__);
+		mutex_unlock(&con_mutex);
+		return ERR_PTR(-ENODEV);
+	}
+
+	chan = NULL;
+	list_for_each_entry(mbox, &mbox_cons, node)
+		if (mbox->dev->of_node == spec.np) {
+			chan = mbox->of_xlate(mbox, &spec);
+			break;
+		}
+
+	of_node_put(spec.np);
+
+	if (!chan || chan->cl || !try_module_get(mbox->dev->driver->owner)) {
+		pr_debug("%s: mailbox not free\n", __func__);
+		mutex_unlock(&con_mutex);
+		return ERR_PTR(-EBUSY);
+	}
+
+	spin_lock_irqsave(&chan->lock, flags);
+	chan->msg_free = 0;
+	chan->msg_count = 0;
+	chan->active_req = NULL;
+	chan->cl = cl;
+	init_completion(&chan->tx_complete);
+
+	if (chan->txdone_method	== TXDONE_BY_POLL && cl->knows_txdone)
+		chan->txdone_method |= TXDONE_BY_ACK;
+
+	spin_unlock_irqrestore(&chan->lock, flags);
+
+	ret = chan->mbox->ops->startup(chan);
+	if (ret) {
+		pr_err("Unable to startup the chan (%d)\n", ret);
+		mbox_free_channel(chan);
+		chan = ERR_PTR(ret);
+	}
+
+	mutex_unlock(&con_mutex);
+	return chan;
+}
+EXPORT_SYMBOL_GPL(mbox_request_channel);
+
+/**
+ * mbox_free_channel - The client relinquishes control of a mailbox
+ *			channel by this call.
+ * @chan: The mailbox channel to be freed.
+ */
+void mbox_free_channel(struct mbox_chan *chan)
+{
+	unsigned long flags;
+
+	if (!chan || !chan->cl)
+		return;
+
+	chan->mbox->ops->shutdown(chan);
+
+	/* The queued TX requests are simply aborted, no callbacks are made */
+	spin_lock_irqsave(&chan->lock, flags);
+	chan->cl = NULL;
+	chan->active_req = NULL;
+	if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
+		chan->txdone_method = TXDONE_BY_POLL;
+
+	module_put(chan->mbox->dev->driver->owner);
+	spin_unlock_irqrestore(&chan->lock, flags);
+}
+EXPORT_SYMBOL_GPL(mbox_free_channel);
+
+static struct mbox_chan *
+of_mbox_index_xlate(struct mbox_controller *mbox,
+		    const struct of_phandle_args *sp)
+{
+	int ind = sp->args[0];
+
+	if (ind >= mbox->num_chans)
+		return NULL;
+
+	return &mbox->chans[ind];
+}
+
+/**
+ * mbox_controller_register - Register the mailbox controller
+ * @mbox:	Pointer to the mailbox controller.
+ *
+ * The controller driver registers its communication channels
+ */
+int mbox_controller_register(struct mbox_controller *mbox)
+{
+	int i, txdone;
+
+	/* Sanity check */
+	if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
+		return -EINVAL;
+
+	if (mbox->txdone_irq)
+		txdone = TXDONE_BY_IRQ;
+	else if (mbox->txdone_poll)
+		txdone = TXDONE_BY_POLL;
+	else /* It has to be ACK then */
+		txdone = TXDONE_BY_ACK;
+
+	if (txdone == TXDONE_BY_POLL) {
+		mbox->poll.function = &poll_txdone;
+		mbox->poll.data = (unsigned long)mbox;
+		init_timer(&mbox->poll);
+	}
+
+	for (i = 0; i < mbox->num_chans; i++) {
+		struct mbox_chan *chan = &mbox->chans[i];
+
+		chan->cl = NULL;
+		chan->mbox = mbox;
+		chan->txdone_method = txdone;
+		spin_lock_init(&chan->lock);
+	}
+
+	if (!mbox->of_xlate)
+		mbox->of_xlate = of_mbox_index_xlate;
+
+	mutex_lock(&con_mutex);
+	list_add_tail(&mbox->node, &mbox_cons);
+	mutex_unlock(&con_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mbox_controller_register);
+
+/**
+ * mbox_controller_unregister - Unregister the mailbox controller
+ * @mbox:	Pointer to the mailbox controller.
+ */
+void mbox_controller_unregister(struct mbox_controller *mbox)
+{
+	int i;
+
+	if (!mbox)
+		return;
+
+	mutex_lock(&con_mutex);
+
+	list_del(&mbox->node);
+
+	for (i = 0; i < mbox->num_chans; i++)
+		mbox_free_channel(&mbox->chans[i]);
+
+	if (mbox->txdone_poll)
+		del_timer_sync(&mbox->poll);
+
+	mutex_unlock(&con_mutex);
+}
+EXPORT_SYMBOL_GPL(mbox_controller_unregister);
diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
new file mode 100644
index 0000000..307d9ca
--- /dev/null
+++ b/include/linux/mailbox_client.h
@@ -0,0 +1,46 @@
+/*
+ * Copyright (C) 2013-2014 Linaro Ltd.
+ * Author: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *
+ * 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/of.h>
+#include <linux/device.h>
+
+struct mbox_chan;
+
+/**
+ * struct mbox_client - User of a mailbox
+ * @dev:		The client device
+ * @tx_block:		If the mbox_send_message should block until data is
+ *			transmitted.
+ * @tx_tout:		Max block period in ms before TX is assumed failure
+ * @knows_txdone:	If the client could run the TX state machine. Usually
+ *			if the client receives some ACK packet for transmission.
+ *			Unused if the controller already has TX_Done/RTR IRQ.
+ * @rx_callback:	Atomic callback to provide client the data received
+ * @tx_done:		Atomic callback to tell client of data transmission
+ */
+struct mbox_client {
+	struct device *dev;
+	bool tx_block;
+	unsigned long tx_tout;
+	bool knows_txdone;
+
+	void (*rx_callback)(struct mbox_client *cl, void *mssg);
+	void (*tx_done)(struct mbox_client *cl, void *mssg, int r);
+};
+
+struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index);
+int mbox_send_message(struct mbox_chan *chan, void *mssg);
+void mbox_client_txdone(struct mbox_chan *chan, int r); /* atomic */
+bool mbox_client_peek_data(struct mbox_chan *chan); /* atomic */
+void mbox_free_channel(struct mbox_chan *chan); /* may sleep */
+
+#endif /* __MAILBOX_CLIENT_H */
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
new file mode 100644
index 0000000..9ee195b
--- /dev/null
+++ b/include/linux/mailbox_controller.h
@@ -0,0 +1,135 @@
+/*
+ * 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/of.h>
+#include <linux/types.h>
+#include <linux/timer.h>
+#include <linux/device.h>
+#include <linux/completion.h>
+
+struct mbox_chan;
+
+/**
+ * struct mbox_chan_ops - methods to control mailbox channels
+ * @send_data:	The API asks the MBOX controller driver, in atomic
+ *		context try to transmit a message on the bus. Returns 0 if
+ *		data is accepted for transmission, -EBUSY while rejecting
+ *		if the remote hasn't yet read the last data sent. Actual
+ *		transmission of data is reported by the controller via
+ *		mbox_chan_txdone (if it has some TX ACK irq). It must not
+ *		sleep.
+ * @startup:	Called when a client requests the chan. The controller
+ *		could ask clients for additional parameters of communication
+ *		to be provided via client's chan_data. This call may
+ *		block. After this call the Controller must forward any
+ *		data received on the chan by calling mbox_chan_received_data.
+ *		The controller may do stuff that need to sleep.
+ * @shutdown:	Called when a client relinquishes control of a chan.
+ *		This call may block too. The controller must not forward
+ *		any received data anymore.
+ *		The controller may do stuff that need to sleep.
+ * @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.
+ *		  The controller may do stuff that need to sleep/block.
+ *		  Used only if txdone_poll:=true && txdone_irq:=false
+ * @peek_data: Atomic check for any received data. Return true if controller
+ *		  has some data to push to the client. False otherwise.
+ */
+struct mbox_chan_ops {
+	int (*send_data)(struct mbox_chan *chan, void *data);
+	int (*startup)(struct mbox_chan *chan);
+	void (*shutdown)(struct mbox_chan *chan);
+	bool (*last_tx_done)(struct mbox_chan *chan);
+	bool (*peek_data)(struct mbox_chan *chan);
+};
+
+/**
+ * struct mbox_controller - Controller of a class of communication channels
+ * @dev:		Device backing this controller
+ * @ops:		Operators that work on each communication chan
+ * @chans:		Array of channels
+ * @num_chans:		Number of channels in the 'chans' array.
+ * @txdone_irq:		Indicates if the controller can report to API when
+ *			the last transmitted data was read by the remote.
+ *			Eg, if it has some TX ACK irq.
+ * @txdone_poll:	If the controller can read but not report the TX
+ *			done. Ex, some register shows the TX status but
+ *			no interrupt rises. Ignored if 'txdone_irq' is set.
+ * @txpoll_period:	If 'txdone_poll' is in effect, the API polls for
+ *			last TX's status after these many millisecs
+ * @of_xlate:		Controller driver specific mapping of channel via DT
+ * @poll:		API private. Used to poll for TXDONE on all channels.
+ * @period:		API private. Polling period.
+ * @node:		API private. To hook into list of controllers.
+ */
+struct mbox_controller {
+	struct device *dev;
+	struct mbox_chan_ops *ops;
+	struct mbox_chan *chans;
+	int num_chans;
+	bool txdone_irq;
+	bool txdone_poll;
+	unsigned txpoll_period;
+	struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox,
+				      const struct of_phandle_args *sp);
+	/* Internal to API */
+	struct timer_list poll;
+	unsigned period;
+	struct list_head node;
+};
+
+/*
+ * 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 transfer is interrupt
+ * triggered and if we have lots of data to transfer, the interrupt
+ * latencies are going to be the bottleneck, not the buffer length.
+ * Besides, mbox_send_message could be called from atomic context and
+ * the client could also queue another message from the notifier 'tx_done'
+ * of the last transfer done.
+ * REVISIT: If too many platforms see the "Try increasing MBOX_TX_QUEUE_LEN"
+ * print, it needs to be taken from config option or somesuch.
+ */
+#define MBOX_TX_QUEUE_LEN	20
+
+/**
+ * struct mbox_chan - s/w representation of a communication chan
+ * @mbox:		Pointer to the parent/provider of this channel
+ * @txdone_method:	Way to detect TXDone chosen by the API
+ * @cl:			Pointer to the current owner of this channel
+ * @tx_complete:	Transmission completion
+ * @active_req:		Currently active request hook
+ * @msg_count:		No. of mssg currently queued
+ * @msg_free:		Index of next available mssg slot
+ * @msg_data:		Hook for data packet
+ * @lock:		Serialise access to the channel
+ * @con_priv:		Hook for controller driver to attach private data
+ */
+struct mbox_chan {
+	struct mbox_controller *mbox;
+	unsigned txdone_method;
+	struct mbox_client *cl;
+	struct completion tx_complete;
+	void *active_req;
+	unsigned msg_count, msg_free;
+	void *msg_data[MBOX_TX_QUEUE_LEN];
+	spinlock_t lock; /* Serialise access to the channel */
+	void *con_priv;
+};
+
+int mbox_controller_register(struct mbox_controller *mbox); /* can sleep */
+void mbox_controller_unregister(struct mbox_controller *mbox); /* can sleep */
+void mbox_chan_received_data(struct mbox_chan *chan, void *data); /* atomic */
+void mbox_chan_txdone(struct mbox_chan *chan, int r); /* atomic */
+
+#endif /* __MAILBOX_CONTROLLER_H */
-- 
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Brown]

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

On Fri, Aug 01, 2014 at 06:01:36PM +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

Reviewed-by: Mark Brown <broonie-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

One very minor thing:

> +	t = add_to_rbuf(chan, mssg);
> +	if (t < 0) {
> +		pr_err("Try increasing MBOX_TX_QUEUE_LEN\n");
> +		return t;
> +	}

In this and the other error messages it's probably worth doing something
like a dev_ print with the device in the mbox_controller - if there's
more than one mailbox controller in the system it may well make
someone's life easier to get a hint about which one is running into
trouble.  Similarly if there were some identification for the channels
that might be useful to display that (perhaps add a name field people
can initialize if they like in the struct?).

This is clearly not a blocker for merge though.

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

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

[Jassi Brar]

On 2 August 2014 00:07, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Aug 01, 2014 at 06:01:36PM +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
>
> Reviewed-by: Mark Brown <broonie@linaro.org>
>
> One very minor thing:
>
>> +     t = add_to_rbuf(chan, mssg);
>> +     if (t < 0) {
>> +             pr_err("Try increasing MBOX_TX_QUEUE_LEN\n");
>> +             return t;
>> +     }
>
> In this and the other error messages it's probably worth doing something
> like a dev_ print with the device in the mbox_controller - if there's
> more than one mailbox controller in the system it may well make
> someone's life easier to get a hint about which one is running into
> trouble.  Similarly if there were some identification for the channels
> that might be useful to display that (perhaps add a name field people
> can initialize if they like in the struct?).
>
> This is clearly not a blocker for merge though.
>
OK will do.

Thanks,
Jassi


-- 
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  -
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog

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

[Ashwin Chaugule]

Hi Jassi,

On 1 August 2014 08:31, Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  MAINTAINERS                        |   8 +
>  drivers/mailbox/Makefile           |   4 +
>  drivers/mailbox/mailbox.c          | 466 +++++++++++++++++++++++++++++++++++++
>  include/linux/mailbox_client.h     |  46 ++++
>  include/linux/mailbox_controller.h | 135 +++++++++++
>  5 files changed, 659 insertions(+)
>  create mode 100644 drivers/mailbox/mailbox.c
>  create mode 100644 include/linux/mailbox_client.h
>  create mode 100644 include/linux/mailbox_controller.h
>

[..]

> +
> +static void poll_txdone(unsigned long data)
> +{
> +       struct mbox_controller *mbox = (struct mbox_controller *)data;
> +       bool txdone, resched = false;
> +       int i;
> +
> +       for (i = 0; i < mbox->num_chans; i++) {
> +               struct mbox_chan *chan = &mbox->chans[i];
> +
> +               if (chan->active_req && chan->cl) {
> +                       resched = true;
> +                       txdone = chan->mbox->ops->last_tx_done(chan);
> +                       if (txdone)
> +                               tx_tick(chan, 0);
> +               }
> +       }
> +
> +       if (resched)
> +               mod_timer(&mbox->poll, jiffies +
> +                               msecs_to_jiffies(mbox->period));

While preparing a different patch which uses the Mbox framework, I
noticed that mbox->period might not be initialized anywhere. Also, how
is mbox->txpoll_period to be used? It appears from the description of
txpoll_period in mbox_controller.h that you'd want to use that value
in the mod_timer above, or equate the two somewhere in the controller
registration or eliminate one of the two. FWIW I also looked at your
code in [1].


Thanks,
Ashwin

[1] - http://git.linaro.org/landing-teams/working/fujitsu/integration.git/shortlog/refs/heads/mailbox-for-next
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sudeep Holla]

On 22/09/14 19:01, Ashwin Chaugule wrote:
> Hi Jassi,
>
> On 1 August 2014 08:31, Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>   MAINTAINERS                        |   8 +
>>   drivers/mailbox/Makefile           |   4 +
>>   drivers/mailbox/mailbox.c          | 466 +++++++++++++++++++++++++++++++++++++
>>   include/linux/mailbox_client.h     |  46 ++++
>>   include/linux/mailbox_controller.h | 135 +++++++++++
>>   5 files changed, 659 insertions(+)
>>   create mode 100644 drivers/mailbox/mailbox.c
>>   create mode 100644 include/linux/mailbox_client.h
>>   create mode 100644 include/linux/mailbox_controller.h
>>
>
> [..]
>
>> +
>> +static void poll_txdone(unsigned long data)
>> +{
>> +       struct mbox_controller *mbox = (struct mbox_controller *)data;
>> +       bool txdone, resched = false;
>> +       int i;
>> +
>> +       for (i = 0; i < mbox->num_chans; i++) {
>> +               struct mbox_chan *chan = &mbox->chans[i];
>> +
>> +               if (chan->active_req && chan->cl) {
>> +                       resched = true;
>> +                       txdone = chan->mbox->ops->last_tx_done(chan);
>> +                       if (txdone)
>> +                               tx_tick(chan, 0);
>> +               }
>> +       }
>> +
>> +       if (resched)
>> +               mod_timer(&mbox->poll, jiffies +
>> +                               msecs_to_jiffies(mbox->period));
>
> While preparing a different patch which uses the Mbox framework, I
> noticed that mbox->period might not be initialized anywhere. Also, how
> is mbox->txpoll_period to be used? It appears from the description of
> txpoll_period in mbox_controller.h that you'd want to use that value
> in the mod_timer above, or equate the two somewhere in the controller
> registration or eliminate one of the two. FWIW I also looked at your
> code in [1].
>

IIUC the controller needs to set the txpoll_period if it sets
txdone_poll, may be a sanity check for !0 would be good.

Regards,
Sudeep

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sudeep Holla]

On 22/09/14 19:15, Sudeep Holla wrote:
>
>
> On 22/09/14 19:01, Ashwin Chaugule wrote:
>> Hi Jassi,
>>
>> On 1 August 2014 08:31, Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> ---
>>>    MAINTAINERS                        |   8 +
>>>    drivers/mailbox/Makefile           |   4 +
>>>    drivers/mailbox/mailbox.c          | 466 +++++++++++++++++++++++++++++++++++++
>>>    include/linux/mailbox_client.h     |  46 ++++
>>>    include/linux/mailbox_controller.h | 135 +++++++++++
>>>    5 files changed, 659 insertions(+)
>>>    create mode 100644 drivers/mailbox/mailbox.c
>>>    create mode 100644 include/linux/mailbox_client.h
>>>    create mode 100644 include/linux/mailbox_controller.h
>>>
>>
>> [..]
>>
>>> +
>>> +static void poll_txdone(unsigned long data)
>>> +{
>>> +       struct mbox_controller *mbox = (struct mbox_controller *)data;
>>> +       bool txdone, resched = false;
>>> +       int i;
>>> +
>>> +       for (i = 0; i < mbox->num_chans; i++) {
>>> +               struct mbox_chan *chan = &mbox->chans[i];
>>> +
>>> +               if (chan->active_req && chan->cl) {
>>> +                       resched = true;
>>> +                       txdone = chan->mbox->ops->last_tx_done(chan);
>>> +                       if (txdone)
>>> +                               tx_tick(chan, 0);
>>> +               }
>>> +       }
>>> +
>>> +       if (resched)
>>> +               mod_timer(&mbox->poll, jiffies +
>>> +                               msecs_to_jiffies(mbox->period));
>>
>> While preparing a different patch which uses the Mbox framework, I
>> noticed that mbox->period might not be initialized anywhere. Also, how
>> is mbox->txpoll_period to be used? It appears from the description of
>> txpoll_period in mbox_controller.h that you'd want to use that value
>> in the mod_timer above, or equate the two somewhere in the controller
>> registration or eliminate one of the two. FWIW I also looked at your
>> code in [1].
>>
>
> IIUC the controller needs to set the txpoll_period if it sets
> txdone_poll, may be a sanity check for !0 would be good.
>

Ah, sorry I confused mbox->period to txpoll_period.
You are right mbox->period is not set, the header claims it to be
private, and hence I assume it needs to be handled only in core mailbox
library. Not sure if we need both mbox->period and txpoll_period though.

Regards,
Sudeep

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ashwin Chaugule]

On 22 September 2014 14:33, Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> wrote:
> On 22/09/14 19:15, Sudeep Holla wrote:
>> On 22/09/14 19:01, Ashwin Chaugule wrote:
>>>
>>> Hi Jassi,
>>>
>>> On 1 August 2014 08:31, Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>> ---
>>>>    MAINTAINERS                        |   8 +
>>>>    drivers/mailbox/Makefile           |   4 +
>>>>    drivers/mailbox/mailbox.c          | 466
>>>> +++++++++++++++++++++++++++++++++++++
>>>>    include/linux/mailbox_client.h     |  46 ++++
>>>>    include/linux/mailbox_controller.h | 135 +++++++++++
>>>>    5 files changed, 659 insertions(+)
>>>>    create mode 100644 drivers/mailbox/mailbox.c
>>>>    create mode 100644 include/linux/mailbox_client.h
>>>>    create mode 100644 include/linux/mailbox_controller.h
>>>>
>>>
>>> [..]
>>>
>>>> +
>>>> +static void poll_txdone(unsigned long data)
>>>> +{
>>>> +       struct mbox_controller *mbox = (struct mbox_controller *)data;
>>>> +       bool txdone, resched = false;
>>>> +       int i;
>>>> +
>>>> +       for (i = 0; i < mbox->num_chans; i++) {
>>>> +               struct mbox_chan *chan = &mbox->chans[i];
>>>> +
>>>> +               if (chan->active_req && chan->cl) {
>>>> +                       resched = true;
>>>> +                       txdone = chan->mbox->ops->last_tx_done(chan);
>>>> +                       if (txdone)
>>>> +                               tx_tick(chan, 0);
>>>> +               }
>>>> +       }
>>>> +
>>>> +       if (resched)
>>>> +               mod_timer(&mbox->poll, jiffies +
>>>> +                               msecs_to_jiffies(mbox->period));
>>>
>>>
>>> While preparing a different patch which uses the Mbox framework, I
>>> noticed that mbox->period might not be initialized anywhere. Also, how
>>> is mbox->txpoll_period to be used? It appears from the description of
>>> txpoll_period in mbox_controller.h that you'd want to use that value
>>> in the mod_timer above, or equate the two somewhere in the controller
>>> registration or eliminate one of the two. FWIW I also looked at your
>>> code in [1].
>>>
>>
>> IIUC the controller needs to set the txpoll_period if it sets
>> txdone_poll, may be a sanity check for !0 would be good.
>>
>
> Ah, sorry I confused mbox->period to txpoll_period.
> You are right mbox->period is not set, the header claims it to be
> private, and hence I assume it needs to be handled only in core mailbox
> library. Not sure if we need both mbox->period and txpoll_period though.

Right. I dont see the need for having both either. Unless the Mailbox
maintainer wants to fix this in some other way, I can send a patch for
replacing mbox->period with mbox->txpoll_period along with my PCC
work.

Thanks,
Ashwin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jassi Brar]

On 24 September 2014 09:14, Ashwin Chaugule <ashwin.chaugule-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 22 September 2014 14:33, Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> wrote:

>>>>> +static void poll_txdone(unsigned long data)
>>>>> +{
>>>>> +       struct mbox_controller *mbox = (struct mbox_controller *)data;
>>>>> +       bool txdone, resched = false;
>>>>> +       int i;
>>>>> +
>>>>> +       for (i = 0; i < mbox->num_chans; i++) {
>>>>> +               struct mbox_chan *chan = &mbox->chans[i];
>>>>> +
>>>>> +               if (chan->active_req && chan->cl) {
>>>>> +                       resched = true;
>>>>> +                       txdone = chan->mbox->ops->last_tx_done(chan);
>>>>> +                       if (txdone)
>>>>> +                               tx_tick(chan, 0);
>>>>> +               }
>>>>> +       }
>>>>> +
>>>>> +       if (resched)
>>>>> +               mod_timer(&mbox->poll, jiffies +
>>>>> +                               msecs_to_jiffies(mbox->period));
>>>>
>>>>
>>>> While preparing a different patch which uses the Mbox framework, I
>>>> noticed that mbox->period might not be initialized anywhere. Also, how
>>>> is mbox->txpoll_period to be used? It appears from the description of
>>>> txpoll_period in mbox_controller.h that you'd want to use that value
>>>> in the mod_timer above, or equate the two somewhere in the controller
>>>> registration or eliminate one of the two. FWIW I also looked at your
>>>> code in [1].
>>>>
>>>
>>> IIUC the controller needs to set the txpoll_period if it sets
>>> txdone_poll, may be a sanity check for !0 would be good.
>>>
>>
>> Ah, sorry I confused mbox->period to txpoll_period.
>> You are right mbox->period is not set, the header claims it to be
>> private, and hence I assume it needs to be handled only in core mailbox
>> library. Not sure if we need both mbox->period and txpoll_period though.
>
> Right. I dont see the need for having both either. Unless the Mailbox
> maintainer wants to fix this in some other way, I can send a patch for
> replacing mbox->period with mbox->txpoll_period along with my PCC
> work.
>
Yeah, probably we should just get rid of mbox_controller.period  I
have updated the same in for-next.

Thanks
Jassi
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ashwin Chaugule]

On 25 September 2014 20:57, Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 24 September 2014 09:14, Ashwin Chaugule <ashwin.chaugule-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On 22 September 2014 14:33, Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> wrote:
>
>>>>>> +static void poll_txdone(unsigned long data)
>>>>>> +{
>>>>>> +       struct mbox_controller *mbox = (struct mbox_controller *)data;
>>>>>> +       bool txdone, resched = false;
>>>>>> +       int i;
>>>>>> +
>>>>>> +       for (i = 0; i < mbox->num_chans; i++) {
>>>>>> +               struct mbox_chan *chan = &mbox->chans[i];
>>>>>> +
>>>>>> +               if (chan->active_req && chan->cl) {
>>>>>> +                       resched = true;
>>>>>> +                       txdone = chan->mbox->ops->last_tx_done(chan);
>>>>>> +                       if (txdone)
>>>>>> +                               tx_tick(chan, 0);
>>>>>> +               }
>>>>>> +       }
>>>>>> +
>>>>>> +       if (resched)
>>>>>> +               mod_timer(&mbox->poll, jiffies +
>>>>>> +                               msecs_to_jiffies(mbox->period));
>>>>>
>>>>>
>>>>> While preparing a different patch which uses the Mbox framework, I
>>>>> noticed that mbox->period might not be initialized anywhere. Also, how
>>>>> is mbox->txpoll_period to be used? It appears from the description of
>>>>> txpoll_period in mbox_controller.h that you'd want to use that value
>>>>> in the mod_timer above, or equate the two somewhere in the controller
>>>>> registration or eliminate one of the two. FWIW I also looked at your
>>>>> code in [1].
>>>>>
>>>>
>>>> IIUC the controller needs to set the txpoll_period if it sets
>>>> txdone_poll, may be a sanity check for !0 would be good.
>>>>
>>>
>>> Ah, sorry I confused mbox->period to txpoll_period.
>>> You are right mbox->period is not set, the header claims it to be
>>> private, and hence I assume it needs to be handled only in core mailbox
>>> library. Not sure if we need both mbox->period and txpoll_period though.
>>
>> Right. I dont see the need for having both either. Unless the Mailbox
>> maintainer wants to fix this in some other way, I can send a patch for
>> replacing mbox->period with mbox->txpoll_period along with my PCC
>> work.
>>
> Yeah, probably we should just get rid of mbox_controller.period  I
> have updated the same in for-next.

Looks like linux-next already has the older version[1]. Your for-next
branch seems to have the fix squashed into the previous commit.
Wouldnt you need to send this as a separate patch? Or ask for the
patch to be reverted in linux-next and then pull again from your
branch.

Thanks,
Ashwin

[1] - http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/mailbox/mailbox.c
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv10 0/4] Common Mailbox Framework

[Mark Brown]

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

On Fri, Aug 01, 2014 at 05:59:54PM +0530, Jassi Brar wrote:
> Hello,
>  Here's v10. Not much different from v9. The code and the generic
> bindings looked good enough to relevant gods, but the patchset still
> needs some formal loving in the form of Acked-by's.
> 
> Changes since v9:
>  o Purely cosmetic changes - added headers and used BIT() 

Given that this code is now getting very stable can I suggest setting up
a git tree and making sure it's included in -next?  Just send Stephen a
mail with a pointer to the relevant tree and branch and ask him to add it.

I'd say it's probably worth merging at this point though since it's not
been in -next yet and the merge window is likely to open this weekend it
might be a bit much to try to merge it this window.

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

Re: [PATCHv10 0/4] Common Mailbox Framework

[Jassi Brar]

On 1 August 2014 23:56, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Aug 01, 2014 at 05:59:54PM +0530, Jassi Brar wrote:
>> Hello,
>>  Here's v10. Not much different from v9. The code and the generic
>> bindings looked good enough to relevant gods, but the patchset still
>> needs some formal loving in the form of Acked-by's.
>>
>> Changes since v9:
>>  o Purely cosmetic changes - added headers and used BIT()
>
> Given that this code is now getting very stable can I suggest setting up
> a git tree and making sure it's included in -next?  Just send Stephen a
> mail with a pointer to the relevant tree and branch and ask him to add it.
>
> I'd say it's probably worth merging at this point though since it's not
> been in -next yet and the merge window is likely to open this weekend it
> might be a bit much to try to merge it this window.
>
Thanks, sounds good. Will do.

Regards,
Jassi
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv10 0/4] Common Mailbox Framework

[Suman Anna]

Jassi,

On 08/02/2014 02:48 AM, Jassi Brar wrote:
> On 1 August 2014 23:56, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> On Fri, Aug 01, 2014 at 05:59:54PM +0530, Jassi Brar wrote:
>>> Hello,
>>>  Here's v10. Not much different from v9. The code and the generic
>>> bindings looked good enough to relevant gods, but the patchset still
>>> needs some formal loving in the form of Acked-by's.
>>>
>>> Changes since v9:
>>>  o Purely cosmetic changes - added headers and used BIT()
>>
>> Given that this code is now getting very stable can I suggest setting up
>> a git tree and making sure it's included in -next?  Just send Stephen a
>> mail with a pointer to the relevant tree and branch and ask him to add it.
>>
>> I'd say it's probably worth merging at this point though since it's not
>> been in -next yet and the merge window is likely to open this weekend it
>> might be a bit much to try to merge it this window.
>>
> Thanks, sounds good. Will do.

Is there anything pending on this series to make it into 3.18?

This is one of the dependencies for our AM335/AM437 PM suspend support,
so it would be great to see this in 3.18, so that we can target 3.19 for
the PM series.

regards
Suman


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv10 0/4] Common Mailbox Framework

[Mark Brown]

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

On Tue, Sep 09, 2014 at 11:23:54AM -0500, Suman Anna wrote:
> On 08/02/2014 02:48 AM, Jassi Brar wrote:

> > Thanks, sounds good. Will do.

> Is there anything pending on this series to make it into 3.18?

> This is one of the dependencies for our AM335/AM437 PM suspend support,
> so it would be great to see this in 3.18, so that we can target 3.19 for
> the PM series.

It'd be good to get it in -next, I don't think that got chased after the
merge window.

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