diff mbox

[2/3,v5] mailbox: Enable BCM2835 mailbox support

Message ID 1430173644-19099-3-git-send-email-eric@anholt.net (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Anholt April 27, 2015, 10:27 p.m. UTC
From: Lubomir Rintel <lkundrak@v3.sk>

Implement BCM2835 mailbox support as a device registered with the
general purpose mailbox framework. Implementation based on commits by
Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base the
implementation.

[1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
[2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
Signed-off-by: Suman Anna <s-anna@ti.com>
Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com>
Signed-off-by: Eric Anholt <eric@anholt.net>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Jassi Brar <jassisinghbrar@gmail.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: linux-rpi-kernel@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org
---

v2: Squashed Craig's work for review, carried over to new version of
    Mailbox framework (changes by Lubomir)

v3: Fix multi-line comment style.  Refer to the documentation by
    filename.  Only declare one MODULE_AUTHOR.  Alphabetize includes.
    Drop some excessive dev_dbg()s (changes by anholt).

v4: Use the new bcm2835_peripheral_read_workaround(), drop the
    unnecessary wmb()s, make the messages be a pointer to u32, rather
    than u32-cast-as-pointer, fold in small static functions, drop
    extra error messages, clean up sizeof() arg for malloc, disable
    interrupts on unload (changes by anholt).

v5: Rewrite as a single-channel driver, where the old notion of
    channels will now be interpreted by an rpi-specific driver.  Minor
    style fix.  Drop peripheral_read_workaround() since it's been
    noted that readl() covers the non-interrupt half of it, and I
    found that we would need to resolve read ordering in the interrupt
    handler anyway (changes by anholt).

 drivers/mailbox/Kconfig           |   8 ++
 drivers/mailbox/Makefile          |   2 +
 drivers/mailbox/bcm2835-mailbox.c | 220 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 230 insertions(+)
 create mode 100644 drivers/mailbox/bcm2835-mailbox.c

Comments

Jassi Brar April 28, 2015, 5:43 a.m. UTC | #1
On Tue, Apr 28, 2015 at 3:57 AM, Eric Anholt <eric@anholt.net> wrote:
> From: Lubomir Rintel <lkundrak@v3.sk>
>
> Implement BCM2835 mailbox support as a device registered with the
> general purpose mailbox framework. Implementation based on commits by
> Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base the
> implementation.
>
> [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
> [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
>
We could probably drop the history from changelog. Just talk about
what the controller is and any specifics of the driver.

> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com>
>
How come it has my s-o-b?


> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
> new file mode 100644
> index 0000000..6910c71
> --- /dev/null
> +++ b/drivers/mailbox/bcm2835-mailbox.c
> @@ -0,0 +1,220 @@
> +/*
> + *  Copyright (C) 2010 Broadcom
> + *  Copyright (C) 2013-2014 Lubomir Rintel
> + *  Copyright (C) 2013 Craig McGeachie
> + *
You may want to make it 2015


> +
> +/* Status register: FIFO state. */
> +#define ARM_MS_FULL            0x80000000
> +#define ARM_MS_EMPTY           0x40000000
>
nit:  BIT(31) and BIT(30)

> +
> +/* Configuration register: Enable interrupts. */
> +#define ARM_MC_IHAVEDATAIRQEN  0x00000001
>
nit: BIT(0)

> +
> +struct bcm2835_mbox {
> +       struct device *dev;
> +       void __iomem *regs;
> +       spinlock_t lock;
> +       bool started;
> +       struct mbox_controller controller;
> +};
> +
> +struct bcm2835_mbox *mbox;
> +
Bad omen. You assume any platform will ever have just one instance of
the mailbox controller. Which may come true, but still is a taboo to
think of.


> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id)
> +{
> +       struct device *dev = mbox->dev;
>
.... and you are wasting 'dev_id' here, which could have been 'mbox'


> +       struct mbox_chan *link = &mbox->controller.chans[0];
> +
> +       while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
> +               u32 msg = readl(mbox->regs + MAIL0_RD);
> +
> +               if (!mbox->started) {
> +                       dev_err(dev, "Reply 0x%08x with no client attached\n",
> +                               msg);
>
This shouldn't happen unless the remote could send asynchronous
commands to Linux. And even if it does, we shouldn't be seeing them
before we are ready. Please move the "Enable the interrupt on data
reception" from probe to bcm2835_startup(), and disable interrupts in
bcm2835_shutdown()


> +static int bcm2835_send_data(struct mbox_chan *link, void *data)
> +{
> +       int ret = 0;
> +       u32 msg = *(u32 *)data;
> +
Sorry, this is seen as an abuse. Please pass pointer to a u32 and not
typecast the value.

> +       if (!mbox->started)
> +               return -ENODEV;
>
This 'started' flag is unnecessary. API won't call send_data() before
startup() or after shutdown().

> +       if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) {
> +               ret = -EBUSY;
> +               goto end;
> +       }
>
This check is unnecessary too. API would have already called
last_tx_done() already to make sure this never hits.

Cheers.
Lubomir Rintel April 28, 2015, 9:50 a.m. UTC | #2
On Tue, 2015-04-28 at 11:13 +0530, Jassi Brar wrote:
> On Tue, Apr 28, 2015 at 3:57 AM, Eric Anholt <eric@anholt.net> wrote:
> > From: Lubomir Rintel <lkundrak@v3.sk>
> > 
> > Implement BCM2835 mailbox support as a device registered with the
> > general purpose mailbox framework. Implementation based on commits 
> > by
> > Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base 
> > the
> > implementation.
> > 
> > [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013
> > -April/000528.html
> > [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013
> > -May/000546.html
> > 
> We could probably drop the history from changelog. Just talk about
> what the controller is and any specifics of the driver.
> 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
> > Signed-off-by: Suman Anna <s-anna@ti.com>
> > Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com>
> > 
> 
> How come it has my s-o-b?

A good question. Seems like I've added it, but I can't really remember
or explain that:

http://lists.infradead.org/pipermail/linux-rpi-kernel/2014
-October/001006.html

It doesn't seem like I've squashed any commit with S-o-b by Jassi Brar
or Suman Anna in. Maybe I intended to add a Cc: and pasted in a S-o-b
by accident? I'm sorry for that.

Please remove the two.

Thank you,
Lubo
Eric Anholt April 28, 2015, 7:49 p.m. UTC | #3
Jassi Brar <jassisinghbrar@gmail.com> writes:

> On Tue, Apr 28, 2015 at 3:57 AM, Eric Anholt <eric@anholt.net> wrote:
>> From: Lubomir Rintel <lkundrak@v3.sk>
>>
>> Implement BCM2835 mailbox support as a device registered with the
>> general purpose mailbox framework. Implementation based on commits by
>> Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base the
>> implementation.
>>
>> [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
>> [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
>>
> We could probably drop the history from changelog. Just talk about
> what the controller is and any specifics of the driver.

How about:

"This mailbox driver provides a single mailbox channel to write 32-bit
values to the VPU and get a 32-bit response.  The Raspberry Pi firmware
uses this mailbox channel to implement firmware calls, while Roku 2
(despite being derived from the same firmware source) doesn't."

>
>> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>> Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com>
>>
> How come it has my s-o-b?

The patch had your s-o-b on it when I started working on it:

http://lists.infradead.org/pipermail/linux-rpi-kernel/2014-October/001006.html

Presumably from:

http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-September/000692.html

I don't see your interaction in the cite for you, though, so if you'd
like the s-o-b removed, I'm happy to do so.  It's been an awfully long
time in development for this driver, with enough revisions, I figured I
just hadn't found where your involvement exactly was.

>> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
>> new file mode 100644
>> index 0000000..6910c71
>> --- /dev/null
>> +++ b/drivers/mailbox/bcm2835-mailbox.c
>> @@ -0,0 +1,220 @@
>> +/*
>> + *  Copyright (C) 2010 Broadcom
>> + *  Copyright (C) 2013-2014 Lubomir Rintel
>> + *  Copyright (C) 2013 Craig McGeachie
>> + *
> You may want to make it 2015

At this point I've probably done enough to merit adding a 2015 for
Broadcom.  Done.

>> +/* Status register: FIFO state. */
>> +#define ARM_MS_FULL            0x80000000
>> +#define ARM_MS_EMPTY           0x40000000
>>
> nit:  BIT(31) and BIT(30)
>
>> +
>> +/* Configuration register: Enable interrupts. */
>> +#define ARM_MC_IHAVEDATAIRQEN  0x00000001
>>
> nit: BIT(0)

Works for me.

>> +
>> +struct bcm2835_mbox {
>> +       struct device *dev;
>> +       void __iomem *regs;
>> +       spinlock_t lock;
>> +       bool started;
>> +       struct mbox_controller controller;
>> +};
>> +
>> +struct bcm2835_mbox *mbox;
>> +
> Bad omen. You assume any platform will ever have just one instance of
> the mailbox controller. Which may come true, but still is a taboo to
> think of.

On the other hand, when I've submitted to other subsystems people have
complained about how I have these extra lookups/container_ofs, instead
of just storing the obviously-only-one-of-them thing in a global.

I think a global makes definite sense for this driver.  But if I have to
go readd the code I had cleaned up, to act like there might be more than
one, I could.

>> +       struct mbox_chan *link = &mbox->controller.chans[0];
>> +
>> +       while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
>> +               u32 msg = readl(mbox->regs + MAIL0_RD);
>> +
>> +               if (!mbox->started) {
>> +                       dev_err(dev, "Reply 0x%08x with no client attached\n",
>> +                               msg);
>>
> This shouldn't happen unless the remote could send asynchronous
> commands to Linux. And even if it does, we shouldn't be seeing them
> before we are ready. Please move the "Enable the interrupt on data
> reception" from probe to bcm2835_startup(), and disable interrupts in
> bcm2835_shutdown()

Done.

>> +static int bcm2835_send_data(struct mbox_chan *link, void *data)
>> +{
>> +       int ret = 0;
>> +       u32 msg = *(u32 *)data;
>> +
> Sorry, this is seen as an abuse. Please pass pointer to a u32 and not
> typecast the value.

This is a pointer to a u32 being passed in.  I think you misread, or I'm
misunderstanding you.

>> +       if (!mbox->started)
>> +               return -ENODEV;
>>
> This 'started' flag is unnecessary. API won't call send_data() before
> startup() or after shutdown().

Dropped.

>> +       if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) {
>> +               ret = -EBUSY;
>> +               goto end;
>> +       }
>>
> This check is unnecessary too. API would have already called
> last_tx_done() already to make sure this never hits.

Dropped.
Jassi Brar April 29, 2015, 2:07 a.m. UTC | #4
On Wed, Apr 29, 2015 at 1:19 AM, Eric Anholt <eric@anholt.net> wrote:
> Jassi Brar <jassisinghbrar@gmail.com> writes:
>
>>> +
>>> +struct bcm2835_mbox {
>>> +       struct device *dev;
>>> +       void __iomem *regs;
>>> +       spinlock_t lock;
>>> +       bool started;
>>> +       struct mbox_controller controller;
>>> +};
>>> +
>>> +struct bcm2835_mbox *mbox;
>>> +
>> Bad omen. You assume any platform will ever have just one instance of
>> the mailbox controller. Which may come true, but still is a taboo to
>> think of.
>
> On the other hand, when I've submitted to other subsystems people have
> complained about how I have these extra lookups/container_ofs, instead
> of just storing the obviously-only-one-of-them thing in a global.
>
> I think a global makes definite sense for this driver.
>
0) Why is 'mbox' not even static?
1) It makes sense for system wide entities like stack/protocol
instance. But this is _one_ controller instance with _one_ mailbox
instance. You think any platform will ever only need one mailbox or
use two classes of controllers?
2) "Avoidable global statics are generally bad"

>>> +       if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) {
>>> +               ret = -EBUSY;
>>> +               goto end;
>>> +       }
>>>
>> This check is unnecessary too. API would have already called
>> last_tx_done() already to make sure this never hits.
>
> Dropped.
>
I see it's not yet.
Eric Anholt April 29, 2015, 5:05 p.m. UTC | #5
Jassi Brar <jassisinghbrar@gmail.com> writes:

> On Wed, Apr 29, 2015 at 1:19 AM, Eric Anholt <eric@anholt.net> wrote:
>> Jassi Brar <jassisinghbrar@gmail.com> writes:
>>
>>>> +
>>>> +struct bcm2835_mbox {
>>>> +       struct device *dev;
>>>> +       void __iomem *regs;
>>>> +       spinlock_t lock;
>>>> +       bool started;
>>>> +       struct mbox_controller controller;
>>>> +};
>>>> +
>>>> +struct bcm2835_mbox *mbox;
>>>> +
>>> Bad omen. You assume any platform will ever have just one instance of
>>> the mailbox controller. Which may come true, but still is a taboo to
>>> think of.
>>
>> On the other hand, when I've submitted to other subsystems people have
>> complained about how I have these extra lookups/container_ofs, instead
>> of just storing the obviously-only-one-of-them thing in a global.
>>
>> I think a global makes definite sense for this driver.
>>
> 0) Why is 'mbox' not even static?

Typo.

> 1) It makes sense for system wide entities like stack/protocol
> instance. But this is _one_ controller instance with _one_ mailbox
> instance. You think any platform will ever only need one mailbox or
> use two classes of controllers?

Yes, I think there will only ever be one or zero instances of this
driver.  I give it 10:1 odds.

>>>> +       if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) {
>>>> +               ret = -EBUSY;
>>>> +               goto end;
>>>> +       }
>>>>
>>> This check is unnecessary too. API would have already called
>>> last_tx_done() already to make sure this never hits.
>>
>> Dropped.
>>
> I see it's not yet.

Misread this while going through the ->stopped removals.  Fixed now.
diff mbox

Patch

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 84b0a2d..ab574da 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -60,4 +60,12 @@  config ALTERA_MBOX
 	  An implementation of the Altera Mailbox soft core. It is used
 	  to send message between processors. Say Y here if you want to use the
 	  Altera mailbox support.
+
+config BCM2835_MBOX
+	tristate "BCM2835 Mailbox"
+	depends on ARCH_BCM2835
+	help
+	  An implementation of the BCM2385 Mailbox.  It is used to invoke
+	  the services of the Videocore. Say Y here if you want to use the
+	  BCM2835 Mailbox.
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index b18201e..8e6d822 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -11,3 +11,5 @@  obj-$(CONFIG_OMAP2PLUS_MBOX)	+= omap-mailbox.o
 obj-$(CONFIG_PCC)		+= pcc.o
 
 obj-$(CONFIG_ALTERA_MBOX)	+= mailbox-altera.o
+
+obj-$(CONFIG_BCM2835_MBOX)	+= bcm2835-mailbox.o
diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
new file mode 100644
index 0000000..6910c71
--- /dev/null
+++ b/drivers/mailbox/bcm2835-mailbox.c
@@ -0,0 +1,220 @@ 
+/*
+ *  Copyright (C) 2010 Broadcom
+ *  Copyright (C) 2013-2014 Lubomir Rintel
+ *  Copyright (C) 2013 Craig McGeachie
+ *
+ * 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.
+ *
+ * This device provides a mechanism for writing to the mailboxes,
+ * that are shared between the ARM and the VideoCore processor
+ *
+ * Parts of the driver are based on:
+ *  - arch/arm/mach-bcm2708/vcio.c file written by Gray Girling that was
+ *    obtained from branch "rpi-3.6.y" of git://github.com/raspberrypi/
+ *    linux.git
+ *  - drivers/mailbox/bcm2835-ipc.c by Lubomir Rintel at
+ *    https://github.com/hackerspace/rpi-linux/blob/lr-raspberry-pi/drivers/
+ *    mailbox/bcm2835-ipc.c
+ *  - documentation available on the following web site:
+ *    https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
+ */
+
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+/* Mailboxes */
+#define ARM_0_MAIL0	0x00
+#define ARM_0_MAIL1	0x20
+
+/*
+ * Mailbox registers. We basically only support mailbox 0 & 1. We
+ * deliver to the VC in mailbox 1, it delivers to us in mailbox 0. See
+ * BCM2835-ARM-Peripherals.pdf section 1.3 for an explanation about
+ * the placement of memory barriers.
+ */
+#define MAIL0_RD	(ARM_0_MAIL0 + 0x00)
+#define MAIL0_POL	(ARM_0_MAIL0 + 0x10)
+#define MAIL0_STA	(ARM_0_MAIL0 + 0x18)
+#define MAIL0_CNF	(ARM_0_MAIL0 + 0x1C)
+#define MAIL1_WRT	(ARM_0_MAIL1 + 0x00)
+
+/* Status register: FIFO state. */
+#define ARM_MS_FULL		0x80000000
+#define ARM_MS_EMPTY		0x40000000
+
+/* Configuration register: Enable interrupts. */
+#define ARM_MC_IHAVEDATAIRQEN	0x00000001
+
+struct bcm2835_mbox {
+	struct device *dev;
+	void __iomem *regs;
+	spinlock_t lock;
+	bool started;
+	struct mbox_controller controller;
+};
+
+struct bcm2835_mbox *mbox;
+
+static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id)
+{
+	struct device *dev = mbox->dev;
+	struct mbox_chan *link = &mbox->controller.chans[0];
+
+	while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
+		u32 msg = readl(mbox->regs + MAIL0_RD);
+
+		if (!mbox->started) {
+			dev_err(dev, "Reply 0x%08x with no client attached\n",
+				msg);
+			continue;
+		}
+		dev_dbg(dev, "Reply 0x%08X\n", msg);
+		mbox_chan_received_data(link, &msg);
+	}
+	return IRQ_HANDLED;
+}
+
+static int bcm2835_send_data(struct mbox_chan *link, void *data)
+{
+	int ret = 0;
+	u32 msg = *(u32 *)data;
+
+	if (!mbox->started)
+		return -ENODEV;
+	spin_lock(&mbox->lock);
+	if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) {
+		ret = -EBUSY;
+		goto end;
+	}
+	writel(msg, mbox->regs + MAIL1_WRT);
+	dev_dbg(mbox->dev, "Request 0x%08X\n", msg);
+end:
+	spin_unlock(&mbox->lock);
+	return ret;
+}
+
+static int bcm2835_startup(struct mbox_chan *link)
+{
+	mbox->started = true;
+	return 0;
+}
+
+static void bcm2835_shutdown(struct mbox_chan *link)
+{
+	mbox->started = false;
+}
+
+static bool bcm2835_last_tx_done(struct mbox_chan *link)
+{
+	bool ret;
+
+	if (!mbox->started)
+		return false;
+	spin_lock(&mbox->lock);
+	ret = !(readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL);
+	spin_unlock(&mbox->lock);
+	return ret;
+}
+
+static struct mbox_chan_ops bcm2835_mbox_chan_ops = {
+	.send_data	= bcm2835_send_data,
+	.startup	= bcm2835_startup,
+	.shutdown	= bcm2835_shutdown,
+	.last_tx_done	= bcm2835_last_tx_done
+};
+
+static int bcm2835_mbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int ret = 0;
+	struct resource *iomem;
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (mbox == NULL)
+		return -ENOMEM;
+	mbox->dev = dev;
+	spin_lock_init(&mbox->lock);
+
+	ret = devm_request_irq(dev, irq_of_parse_and_map(dev->of_node, 0),
+			       bcm2835_mbox_irq, 0, dev_name(dev), mbox);
+	if (ret) {
+		dev_err(dev, "Failed to register a mailbox IRQ handler: %d\n",
+			ret);
+		mbox = NULL;
+		return -ENODEV;
+	}
+
+	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mbox->regs = devm_ioremap_resource(&pdev->dev, iomem);
+	if (IS_ERR(mbox->regs)) {
+		ret = PTR_ERR(mbox->regs);
+		dev_err(&pdev->dev, "Failed to remap mailbox regs: %d\n", ret);
+		mbox = NULL;
+		return ret;
+	}
+
+	mbox->controller.txdone_poll = true;
+	mbox->controller.txpoll_period = 5;
+	mbox->controller.ops = &bcm2835_mbox_chan_ops;
+	mbox->controller.dev = dev;
+	mbox->controller.num_chans = 1;
+	mbox->controller.chans = devm_kzalloc(dev,
+		sizeof(*mbox->controller.chans), GFP_KERNEL);
+	if (!mbox->controller.chans) {
+		mbox = NULL;
+		return -ENOMEM;
+	}
+
+	ret  = mbox_controller_register(&mbox->controller);
+	if (ret) {
+		mbox = NULL;
+		return ret;
+	}
+
+	/* Enable the interrupt on data reception */
+	writel(ARM_MC_IHAVEDATAIRQEN, mbox->regs + MAIL0_CNF);
+	dev_info(dev, "mailbox enabled\n");
+
+	return ret;
+}
+
+static int bcm2835_mbox_remove(struct platform_device *pdev)
+{
+	writel(0, mbox->regs + MAIL0_CNF);
+	mbox_controller_unregister(&mbox->controller);
+	mbox = NULL;
+	return 0;
+}
+
+static const struct of_device_id bcm2835_mbox_of_match[] = {
+	{ .compatible = "brcm,bcm2835-mbox", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm2835_mbox_of_match);
+
+static struct platform_driver bcm2835_mbox_driver = {
+	.driver = {
+		.name = "bcm2835-mbox",
+		.owner = THIS_MODULE,
+		.of_match_table = bcm2835_mbox_of_match,
+	},
+	.probe		= bcm2835_mbox_probe,
+	.remove		= bcm2835_mbox_remove,
+};
+module_platform_driver(bcm2835_mbox_driver);
+
+MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>");
+MODULE_DESCRIPTION("BCM2835 mailbox IPC driver");
+MODULE_LICENSE("GPL v2");