[PATCH 0/3] BCM2835 core mailbox support

[PATCH 0/3] BCM2835 core mailbox support

[Eric Anholt]

Following srwarren's suggestion, I've re-split the mailbox driver I
had previously submitted so that it only exposes a single channel and
leaves the previous "channel" notion up to a firmware-specific device
driver.

I'll be submitting the firmware-specific driver in a separate mail
thread -- hopefully this code is ready now, while I expect to get
feeback on the other driver.

This code can also be found at https://github.com/anholt/linux/tree/rpi-mbox-only

[PATCH 1/3 v4] dt/bindings: Add binding for the BCM2835 mailbox driver

[Eric Anholt]

From: Lubomir Rintel <lkundrak@v3.sk>

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
Signed-off-by: Eric Anholt <eric@anholt.net>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Stephen Warren <swarren@wwwdotorg.org>
---

v2: Split into a separate patch for submitting to the devicetree list.
    Consistently start node docs with a capital letter. device's
    address in the example shouldn't have "0x". Drop machine-specific
    interrupt numbers from the docs.  (changes by anholt).

v3: Move the file to just bcm2835-mbox.txt, clean up formatting
    (changes by anholt, from review by Lee Jones).

v4: Move file back by consensus from various Broadcom platform
    maintainers (changes by anholt, acked by Lee Jones).

 .../devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt

diff --git a/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
new file mode 100644
index 0000000..0bb2b9d
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
@@ -0,0 +1,19 @@
+Broadcom BCM2835 VideoCore mailbox IPC
+
+Required properties:
+
+- compatible:	Should be "brcm,bcm2835-mbox"
+- reg:		Specifies base physical address and size of the registers
+- interrupts:	The interrupt number
+		  See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
+- #mbox-cells:	Specifies the number of cells needed to encode a mailbox
+		  channel. The value shall be 1
+
+Example:
+
+mailbox: mailbox at 7e00b800 {
+	compatible = "brcm,bcm2835-mbox";
+	reg = <0x7e00b880 0x40>;
+	interrupts = <0 1>;
+	#mbox-cells = <1>;
+};
-- 
2.1.4

[PATCH 1/3 v4] dt/bindings: Add binding for the BCM2835 mailbox driver

[Jassi Brar]

On Tue, Apr 28, 2015 at 3:57 AM, Eric Anholt <eric@anholt.net> wrote:
> From: Lubomir Rintel <lkundrak@v3.sk>
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> Acked-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
>
> v2: Split into a separate patch for submitting to the devicetree list.
>     Consistently start node docs with a capital letter. device's
>     address in the example shouldn't have "0x". Drop machine-specific
>     interrupt numbers from the docs.  (changes by anholt).
>
> v3: Move the file to just bcm2835-mbox.txt, clean up formatting
>     (changes by anholt, from review by Lee Jones).
>
> v4: Move file back by consensus from various Broadcom platform
>     maintainers (changes by anholt, acked by Lee Jones).
>
>  .../devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
>
> diff --git a/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
> new file mode 100644
> index 0000000..0bb2b9d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
> @@ -0,0 +1,19 @@
> +Broadcom BCM2835 VideoCore mailbox IPC
> +
> +Required properties:
> +
> +- compatible:  Should be "brcm,bcm2835-mbox"
> +- reg:         Specifies base physical address and size of the registers
> +- interrupts:  The interrupt number
> +                 See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
> +- #mbox-cells: Specifies the number of cells needed to encode a mailbox
> +                 channel. The value shall be 1
>
Probably also specify what that one-cell value would stand for.

> +
> +Example:
> +
> +mailbox: mailbox at 7e00b800 {
> +       compatible = "brcm,bcm2835-mbox";
> +       reg = <0x7e00b880 0x40>;
> +       interrupts = <0 1>;
> +       #mbox-cells = <1>;
> +};
Usually it comes with an example user node as well.

[PATCH 1/3 v5] dt/bindings: Add binding for the BCM2835 mailbox driver

[Eric Anholt]

From: Lubomir Rintel <lkundrak@v3.sk>

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
Signed-off-by: Eric Anholt <eric@anholt.net>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Stephen Warren <swarren@wwwdotorg.org>
---

v2: Split into a separate patch for submitting to the devicetree list.
    Consistently start node docs with a capital letter. device's
    address in the example shouldn't have "0x". Drop machine-specific
    interrupt numbers from the docs.  (changes by anholt).

v3: Move the file to just bcm2835-mbox.txt, clean up formatting
    (changes by anholt, from review by Lee Jones).

v4: Move file back by consensus from various Broadcom platform
    maintainers (changes by anholt, acked by Lee Jones).

v5: Document that the mailbox cell should be 0 in clients, and add an
    example of a client (changes by anholt, from review by Jassi).

 .../bindings/mailbox/brcm,bcm2835-mbox.txt         | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt

diff --git a/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
new file mode 100644
index 0000000..731de34
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
@@ -0,0 +1,27 @@
+Broadcom BCM2835 VideoCore mailbox IPC
+
+Required properties:
+
+- compatible:	Should be "brcm,bcm2835-mbox"
+- reg:		Specifies base physical address and size of the registers
+- interrupts:	The interrupt number
+		  See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
+- #mbox-cells:	Specifies the number of cells needed to encode a mailbox
+		  channel. The value shall be 1.  Clients should supply a value
+		  of 0 for the mailbox channel, because there is only one
+		  exposed
+
+Example:
+
+mailbox: mailbox at 7e00b800 {
+	compatible = "brcm,bcm2835-mbox";
+	reg = <0x7e00b880 0x40>;
+	interrupts = <0 1>;
+	#mbox-cells = <1>;
+};
+
+firmware: firmware {
+	compatible = "raspberrypi,firmware";
+	mboxes = <&mailbox 0>;
+	#power-domain-cells = <1>;
+};
-- 
2.1.4

[PATCH 1/3 v5] dt/bindings: Add binding for the BCM2835 mailbox driver

[Stephen Warren]

On 04/28/2015 02:44 PM, Eric Anholt wrote:
> From: Lubomir Rintel <lkundrak@v3.sk>
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> Acked-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
> 
> v2: Split into a separate patch for submitting to the devicetree list.
>     Consistently start node docs with a capital letter. device's
>     address in the example shouldn't have "0x". Drop machine-specific
>     interrupt numbers from the docs.  (changes by anholt).
> 
> v3: Move the file to just bcm2835-mbox.txt, clean up formatting
>     (changes by anholt, from review by Lee Jones).
> 
> v4: Move file back by consensus from various Broadcom platform
>     maintainers (changes by anholt, acked by Lee Jones).
> 
> v5: Document that the mailbox cell should be 0 in clients, and add an
>     example of a client (changes by anholt, from review by Jassi).

Some mention of what you changed in the patch might be nice, since it
was originally authored by someone else, and there's quite a changelog.

> diff --git a/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt

> +- #mbox-cells:	Specifies the number of cells needed to encode a mailbox
> +		  channel. The value shall be 1.  Clients should supply a value
> +		  of 0 for the mailbox channel, because there is only one
> +		  exposed

Can't you use #mbox-cells = <0> if there's no data to provide?

[PATCH 1/3 v5] dt/bindings: Add binding for the BCM2835 mailbox driver

[Eric Anholt]

Stephen Warren <swarren@wwwdotorg.org> writes:

> On 04/28/2015 02:44 PM, Eric Anholt wrote:
>> From: Lubomir Rintel <lkundrak@v3.sk>
>> 
>> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>> Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> Acked-by: Lee Jones <lee.jones@linaro.org>
>> Acked-by: Stephen Warren <swarren@wwwdotorg.org>
>> ---
>> 
>> v2: Split into a separate patch for submitting to the devicetree list.
>>     Consistently start node docs with a capital letter. device's
>>     address in the example shouldn't have "0x". Drop machine-specific
>>     interrupt numbers from the docs.  (changes by anholt).
>> 
>> v3: Move the file to just bcm2835-mbox.txt, clean up formatting
>>     (changes by anholt, from review by Lee Jones).
>> 
>> v4: Move file back by consensus from various Broadcom platform
>>     maintainers (changes by anholt, acked by Lee Jones).
>> 
>> v5: Document that the mailbox cell should be 0 in clients, and add an
>>     example of a client (changes by anholt, from review by Jassi).
>
> Some mention of what you changed in the patch might be nice, since it
> was originally authored by someone else, and there's quite a changelog.

I've been taking this changelog stuff out of the patch and putting it
below '---' because when I was submitting this series before, I got
chided for putting it in the patch!

>> diff --git a/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
>
>> +- #mbox-cells:	Specifies the number of cells needed to encode a mailbox
>> +		  channel. The value shall be 1.  Clients should supply a value
>> +		  of 0 for the mailbox channel, because there is only one
>> +		  exposed
>
> Can't you use #mbox-cells = <0> if there's no data to provide?

of_mbox_index_xlate looks like it's always dereferencing the first arg.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150429/6c9852e1/attachment-0001.sig>

[PATCH 1/3 v5] dt/bindings: Add binding for the BCM2835 mailbox driver

[Stephen Warren]

On 04/29/15 10:39, Eric Anholt wrote:
> Stephen Warren <swarren@wwwdotorg.org> writes:
>
>> On 04/28/2015 02:44 PM, Eric Anholt wrote:
>>> From: Lubomir Rintel <lkundrak@v3.sk>
>>>
>>> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>>> Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>> Acked-by: Lee Jones <lee.jones@linaro.org>
>>> Acked-by: Stephen Warren <swarren@wwwdotorg.org>
>>> ---
>>>
>>> v2: Split into a separate patch for submitting to the devicetree list.
>>>      Consistently start node docs with a capital letter. device's
>>>      address in the example shouldn't have "0x". Drop machine-specific
>>>      interrupt numbers from the docs.  (changes by anholt).
>>>
>>> v3: Move the file to just bcm2835-mbox.txt, clean up formatting
>>>      (changes by anholt, from review by Lee Jones).
>>>
>>> v4: Move file back by consensus from various Broadcom platform
>>>      maintainers (changes by anholt, acked by Lee Jones).
>>>
>>> v5: Document that the mailbox cell should be 0 in clients, and add an
>>>      example of a client (changes by anholt, from review by Jassi).
>>
>> Some mention of what you changed in the patch might be nice, since it
>> was originally authored by someone else, and there's quite a changelog.
>
> I've been taking this changelog stuff out of the patch and putting it
> below '---' because when I was submitting this series before, I got
> chided for putting it in the patch!

A changelog and a description of the changes you've made aren't quite 
the same thing.

There should be some mention in the commit description of what you 
changed relative to the patch that someone else authored, so they don't 
get blaimed/praised for any of your changes. The format of that 
description wouldn't usually be a detailed patch revision changelog 
though, just a description of the overall changes that were left in the 
final version.

>>> diff --git a/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
>>
>>> +- #mbox-cells:	Specifies the number of cells needed to encode a mailbox
>>> +		  channel. The value shall be 1.  Clients should supply a value
>>> +		  of 0 for the mailbox channel, because there is only one
>>> +		  exposed
>>
>> Can't you use #mbox-cells = <0> if there's no data to provide?
>
> of_mbox_index_xlate looks like it's always dereferencing the first arg.

Is it possible to fix that?

That said, last I looked at the HW I thought there were actually 
multiple channels possible, so perhaps having a cell for future 
expansion is reasonable.

[PATCH 1/3 v5] dt/bindings: Add binding for the BCM2835 mailbox driver

[Jassi Brar]

On Thu, Apr 30, 2015 at 5:11 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/29/15 10:39, Eric Anholt wrote:
>>
>> Stephen Warren <swarren@wwwdotorg.org> writes:
>>
>>> On 04/28/2015 02:44 PM, Eric Anholt wrote:
>>>>
>>>> From: Lubomir Rintel <lkundrak@v3.sk>
>>>>
>>>> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>>>> Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
>>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>>> Acked-by: Lee Jones <lee.jones@linaro.org>
>>>> Acked-by: Stephen Warren <swarren@wwwdotorg.org>
>>>> ---
>>>>
>>>> v2: Split into a separate patch for submitting to the devicetree list.
>>>>      Consistently start node docs with a capital letter. device's
>>>>      address in the example shouldn't have "0x". Drop machine-specific
>>>>      interrupt numbers from the docs.  (changes by anholt).
>>>>
>>>> v3: Move the file to just bcm2835-mbox.txt, clean up formatting
>>>>      (changes by anholt, from review by Lee Jones).
>>>>
>>>> v4: Move file back by consensus from various Broadcom platform
>>>>      maintainers (changes by anholt, acked by Lee Jones).
>>>>
>>>> v5: Document that the mailbox cell should be 0 in clients, and add an
>>>>      example of a client (changes by anholt, from review by Jassi).
>>>
>>>
>>> Some mention of what you changed in the patch might be nice, since it
>>> was originally authored by someone else, and there's quite a changelog.
>>
>>
>> I've been taking this changelog stuff out of the patch and putting it
>> below '---' because when I was submitting this series before, I got
>> chided for putting it in the patch!
>
>
> A changelog and a description of the changes you've made aren't quite the
> same thing.
>
> There should be some mention in the commit description of what you changed
> relative to the patch that someone else authored, so they don't get
> blaimed/praised for any of your changes. The format of that description
> wouldn't usually be a detailed patch revision changelog though, just a
> description of the overall changes that were left in the final version.
>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
>>>> b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
>>>
>>>
>>>> +- #mbox-cells: Specifies the number of cells needed to encode a mailbox
>>>> +                 channel. The value shall be 1.  Clients should supply
>>>> a value
>>>> +                 of 0 for the mailbox channel, because there is only
>>>> one
>>>> +                 exposed
>>>
>>>
>>> Can't you use #mbox-cells = <0> if there's no data to provide?
>>
>>
>> of_mbox_index_xlate looks like it's always dereferencing the first arg.
>
>
> Is it possible to fix that?
>
The driver specific of_xlate() is meant to be provided during
mbox_controller_register() in which case the default 1-cell xlate
won't be used.

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

[Eric Anholt]

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 at lists.infradead.org
Cc: linux-arm-kernel at 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

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");
-- 
2.1.4

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

[Jassi Brar]

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.

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

[Lubomir Rintel]

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

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

[Eric Anholt]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150428/7a449b9e/attachment.sig>

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

[Jassi Brar]

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.

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

[Eric Anholt]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150429/bc4157e5/attachment.sig>

[PATCH 2/3 v6] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

From: Lubomir Rintel <lkundrak@v3.sk>

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 tree) doesn't.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
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 at lists.infradead.org
Cc: linux-arm-kernel at 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).

v6: rewrite commit message, only turn on interrupts when a client is
    present, drop the started flag, use BIT() macro, update Broadcom
    copyright (changes by anholt, from Jassi's review).  Drop s-o-b of
    Jassi and Suman, who appear to have been accidentally added by
    Craig (noted by Lubomir).

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

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..3d85d89
--- /dev/null
+++ b/drivers/mailbox/bcm2835-mailbox.c
@@ -0,0 +1,208 @@
+/*
+ *  Copyright (C) 2010,2015 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		BIT(31)
+#define ARM_MS_EMPTY		BIT(30)
+
+/* Configuration register: Enable interrupts. */
+#define ARM_MC_IHAVEDATAIRQEN	BIT(0)
+
+struct bcm2835_mbox {
+	struct device *dev;
+	void __iomem *regs;
+	spinlock_t lock;
+	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);
+		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;
+
+	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)
+{
+	/* Enable the interrupt on data reception */
+	writel(ARM_MC_IHAVEDATAIRQEN, mbox->regs + MAIL0_CNF);
+
+	return 0;
+}
+
+static void bcm2835_shutdown(struct mbox_chan *link)
+{
+	writel(0, mbox->regs + MAIL0_CNF);
+}
+
+static bool bcm2835_last_tx_done(struct mbox_chan *link)
+{
+	bool ret;
+
+	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;
+	}
+
+	dev_info(dev, "mailbox enabled\n");
+
+	return ret;
+}
+
+static int bcm2835_mbox_remove(struct platform_device *pdev)
+{
+	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");
-- 
2.1.4

[PATCH 2/3 v6] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 04/28/2015 02:54 PM, Eric Anholt wrote:
> From: Lubomir Rintel <lkundrak@v3.sk>
> 
> 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 tree) doesn't.

Acked-by: Stephen Warren <swarren@wwwdotorg.org>

[PATCH 2/3 v7] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

From: Lubomir Rintel <lkundrak@v3.sk>

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 tree) doesn't.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
Signed-off-by: Eric Anholt <eric@anholt.net>
Cc: Jassi Brar <jassisinghbrar@gmail.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: linux-rpi-kernel at lists.infradead.org
Cc: linux-arm-kernel at lists.infradead.org
Acked-by: Stephen Warren <swarren@wwwdotorg.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).

v6: rewrite commit message, only turn on interrupts when a client is
    present, drop the started flag, use BIT() macro, update Broadcom
    copyright (changes by anholt, from Jassi's review).  Drop s-o-b of
    Jassi and Suman, who appear to have been accidentally added by
    Craig (noted by Lubomir).

v7: Drop the txdone-style check in send_data, since the core will make
    sure we're not called until the previous one is done.  Fix "mbox"
    to be static (changes by anholt, from review by Jassi).

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

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..f8b9c51
--- /dev/null
+++ b/drivers/mailbox/bcm2835-mailbox.c
@@ -0,0 +1,204 @@
+/*
+ *  Copyright (C) 2010,2015 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		BIT(31)
+#define ARM_MS_EMPTY		BIT(30)
+
+/* Configuration register: Enable interrupts. */
+#define ARM_MC_IHAVEDATAIRQEN	BIT(0)
+
+struct bcm2835_mbox {
+	struct device *dev;
+	void __iomem *regs;
+	spinlock_t lock;
+	struct mbox_controller controller;
+};
+
+static 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);
+		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;
+
+	spin_lock(&mbox->lock);
+	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)
+{
+	/* Enable the interrupt on data reception */
+	writel(ARM_MC_IHAVEDATAIRQEN, mbox->regs + MAIL0_CNF);
+
+	return 0;
+}
+
+static void bcm2835_shutdown(struct mbox_chan *link)
+{
+	writel(0, mbox->regs + MAIL0_CNF);
+}
+
+static bool bcm2835_last_tx_done(struct mbox_chan *link)
+{
+	bool ret;
+
+	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;
+	}
+
+	dev_info(dev, "mailbox enabled\n");
+
+	return ret;
+}
+
+static int bcm2835_mbox_remove(struct platform_device *pdev)
+{
+	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");
-- 
2.1.4

[PATCH 2/3 v7] mailbox: Enable BCM2835 mailbox support

[Jassi Brar]

On Wed, Apr 29, 2015 at 10:39 PM, Eric Anholt <eric@anholt.net> wrote:

> +
> +struct bcm2835_mbox {
> +       struct device *dev;
> +       void __iomem *regs;
> +       spinlock_t lock;
> +       struct mbox_controller controller;
> +};
> +
> +static 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];
> +
I learn from Stephen's other post that the controller could have
multiple channels. In which case this driver is poorly setup. Actually
if the driver was designed properly there isn't anything special to be
done.
 Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0

> +       while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
> +               u32 msg = readl(mbox->regs + MAIL0_RD);
> +               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;
> +
> +       spin_lock(&mbox->lock);
> +       writel(msg, mbox->regs + MAIL1_WRT);
> +       dev_dbg(mbox->dev, "Request 0x%08X\n", msg);
> +end:
>
Did you compile check the driver for errors and warnings?

> +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);
>
Why even pass 'mbox' when you insist on ignoring 'dev_id' and access
the global variable again directly :D

[PATCH 2/3 v7] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 04/29/2015 11:26 PM, Jassi Brar wrote:
> On Wed, Apr 29, 2015 at 10:39 PM, Eric Anholt <eric@anholt.net> wrote:
> 
>> +
>> +struct bcm2835_mbox {
>> +       struct device *dev;
>> +       void __iomem *regs;
>> +       spinlock_t lock;
>> +       struct mbox_controller controller;
>> +};
>> +
>> +static 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];
>> +
> I learn from Stephen's other post that the controller could have
> multiple channels. In which case this driver is poorly setup. Actually
> if the driver was designed properly there isn't anything special to be
> done.
>  Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0

Now that I look a bit harder at the registers, I think there are 2
mailbox register sets, but each is uni-directional, so a pair makes up
the typical bi-directional mailbox. There are multiple "owner" areas (or
sets of registers); I'm not quite sure what that implies.

As such, limiting this driver to a single mailbox is probably correct.

I would expect to see #mbox-cells=<0> in the DT, which would require a
custom of_xlate though (or modifying the default to work with a 0 cell
count; IIRC the default of_xlate for some other subsystems will work in
that scenario).

(As an aside, if we ever did find the need to expand the driver to cover
more mailboxes in the future, extending it and the DT to support
#mbox-cells=<0> or #mbox-cells=<1> at run-time should be trivial).

[PATCH 2/3 v7] mailbox: Enable BCM2835 mailbox support

[Jassi Brar]

On Fri, May 1, 2015 at 8:31 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/29/2015 11:26 PM, Jassi Brar wrote:
>> On Wed, Apr 29, 2015 at 10:39 PM, Eric Anholt <eric@anholt.net> wrote:
>>
>>> +
>>> +struct bcm2835_mbox {
>>> +       struct device *dev;
>>> +       void __iomem *regs;
>>> +       spinlock_t lock;
>>> +       struct mbox_controller controller;
>>> +};
>>> +
>>> +static 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];
>>> +
>> I learn from Stephen's other post that the controller could have
>> multiple channels. In which case this driver is poorly setup. Actually
>> if the driver was designed properly there isn't anything special to be
>> done.
>>  Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0
>
> Now that I look a bit harder at the registers, I think there are 2
> mailbox register sets, but each is uni-directional, so a pair makes up
> the typical bi-directional mailbox. There are multiple "owner" areas (or
> sets of registers); I'm not quite sure what that implies.
>
Probably a mailbox is the send-recv pair, and the controller could
have many such pairs.

> As such, limiting this driver to a single mailbox is probably correct.
>
OK

> I would expect to see #mbox-cells=<0> in the DT, which would require a
> custom of_xlate though (or modifying the default to work with a 0 cell
> count; IIRC the default of_xlate for some other subsystems will work in
> that scenario).
>
Yeah the api could simply return the first free channel for 0-cell spec.

[PATCH 2/3 v7] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

Jassi Brar <jassisinghbrar@gmail.com> writes:

> On Wed, Apr 29, 2015 at 10:39 PM, Eric Anholt <eric@anholt.net> wrote:
>
>> +
>> +struct bcm2835_mbox {
>> +       struct device *dev;
>> +       void __iomem *regs;
>> +       spinlock_t lock;
>> +       struct mbox_controller controller;
>> +};
>> +
>> +static 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];
>> +
> I learn from Stephen's other post that the controller could have
> multiple channels. In which case this driver is poorly setup. Actually
> if the driver was designed properly there isn't anything special to be
> done.
>  Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0

There's only the one channel according to the docs.  I wish we wouldn't
get derailed by speculation on the list when the documentation is
available. :(

>> +       while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
>> +               u32 msg = readl(mbox->regs + MAIL0_RD);
>> +               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;
>> +
>> +       spin_lock(&mbox->lock);
>> +       writel(msg, mbox->regs + MAIL1_WRT);
>> +       dev_dbg(mbox->dev, "Request 0x%08X\n", msg);
>> +end:
>>
> Did you compile check the driver for errors and warnings?

At this point I'm so burned out on repainting this bikeshed that I
missed a spot of the current color.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150504/4ef0cb46/attachment-0001.sig>

[PATCH 2/3 v7] mailbox: Enable BCM2835 mailbox support

[Jassi Brar]

On Tue, May 5, 2015 at 6:27 AM, Eric Anholt <eric@anholt.net> wrote:
> Jassi Brar <jassisinghbrar@gmail.com> writes:
>
>> On Wed, Apr 29, 2015 at 10:39 PM, Eric Anholt <eric@anholt.net> wrote:
>>
>>> +
>>> +struct bcm2835_mbox {
>>> +       struct device *dev;
>>> +       void __iomem *regs;
>>> +       spinlock_t lock;
>>> +       struct mbox_controller controller;
>>> +};
>>> +
>>> +static 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];
>>> +
>> I learn from Stephen's other post that the controller could have
>> multiple channels. In which case this driver is poorly setup. Actually
>> if the driver was designed properly there isn't anything special to be
>> done.
>>  Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0
>
> There's only the one channel according to the docs.  I wish we wouldn't
> get derailed by speculation on the list when the documentation is
> available. :(
>
Can I have the pointer to the doc please, if its publicly available.

>>> +       while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
>>> +               u32 msg = readl(mbox->regs + MAIL0_RD);
>>> +               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;
>>> +
>>> +       spin_lock(&mbox->lock);
>>> +       writel(msg, mbox->regs + MAIL1_WRT);
>>> +       dev_dbg(mbox->dev, "Request 0x%08X\n", msg);
>>> +end:
>>>
>> Did you compile check the driver for errors and warnings?
>
> At this point I'm so burned out on repainting this bikeshed that I
> missed a spot of the current color.
>
I have just rechecked the thread and I didn't find any revision that
had only nits picked. Subsystems even dictate how the files are named
and ordered in Makefile, and you have been insisting on hardcoding
values.

[PATCH 2/3 v7] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 05/04/2015 08:30 PM, Jassi Brar wrote:
> On Tue, May 5, 2015 at 6:27 AM, Eric Anholt <eric@anholt.net> wrote:
>> Jassi Brar <jassisinghbrar@gmail.com> writes:
>>
>>> On Wed, Apr 29, 2015 at 10:39 PM, Eric Anholt <eric@anholt.net> wrote:
>>>
>>>> +
>>>> +struct bcm2835_mbox {
>>>> +       struct device *dev;
>>>> +       void __iomem *regs;
>>>> +       spinlock_t lock;
>>>> +       struct mbox_controller controller;
>>>> +};
>>>> +
>>>> +static 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];
>>>> +
>>> I learn from Stephen's other post that the controller could have
>>> multiple channels. In which case this driver is poorly setup. Actually
>>> if the driver was designed properly there isn't anything special to be
>>> done.
>>>   Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0
>>
>> There's only the one channel according to the docs.  I wish we wouldn't
>> get derailed by speculation on the list when the documentation is
>> available. :(
>>
> Can I have the pointer to the doc please, if its publicly available.

https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf

[PATCH 3/3] ARM: bcm2835: Add the mailbox to the device tree

[Eric Anholt]

Signed-off-by: Eric Anholt <eric@anholt.net>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Stephen Warren <swarren@wwwdotorg.org>
---
 arch/arm/boot/dts/bcm2835.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index 4d4c129..664aa25 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -61,6 +61,13 @@
 			reg = <0x7e104000 0x10>;
 		};
 
+		mailbox: mailbox at 7e00b800 {
+			compatible = "brcm,bcm2835-mbox";
+			reg = <0x7e00b880 0x40>;
+			interrupts = <0 1>;
+			#mbox-cells = <1>;
+		};
+
 		gpio: gpio at 7e200000 {
 			compatible = "brcm,bcm2835-gpio";
 			reg = <0x7e200000 0xb4>;
-- 
2.1.4