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

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

[Eric Anholt]

From: Lubomir Rintel <lkundrak@v3.sk>

This patch was split out of Lubomir's original mailbox patch by Eric
Anholt, and the required properties documentation and examples have
been filled out more completely and updated for the driver being
changed to expose a single channel.

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.

v6: Add change description to commit message separate from this
    version section.  Update for #mbox-cells 0 change.

 .../bindings/mailbox/brcm,bcm2835-mbox.txt         | 26 ++++++++++++++++++++++
 1 file changed, 26 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..e893615
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
@@ -0,0 +1,26 @@
+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 0, since there is only one
+		  mailbox channel implemented by the device.
+
+Example:
+
+mailbox: mailbox@7e00b800 {
+	compatible = "brcm,bcm2835-mbox";
+	reg = <0x7e00b880 0x40>;
+	interrupts = <0 1>;
+	#mbox-cells = <0>;
+};
+
+firmware: firmware {
+	compatible = "raspberrypi,firmware";
+	mboxes = <&mailbox>;
+	#power-domain-cells = <1>;
+};
-- 
2.1.4

[PATCH 2/3 v8] 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.

The driver was originally submitted by Lubomir, based on the
out-of-tree 2708 mailbox driver.  Eric Anholt fixed it up for
upstreaming, with the major functional change being that it now has no
notion of multiple channels (since that is a firmware-dependent
concept) and instead the raspberrypi-firmware driver will do that
bit-twiddling in its own messages.

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>
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).

v8: Fix compiler warning from txdone check removal.  Add a summary of
    anholt's changes to the commit message.  Supply our own of_xlate
    to insist that 0 mbox-cells are provided.  Replace global *mbox
    with container_of()s. Drop our driver's extra "struct device
    *dev".

 drivers/mailbox/Kconfig           |   8 ++
 drivers/mailbox/Makefile          |   2 +
 drivers/mailbox/bcm2835-mailbox.c | 217 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 227 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..16c3453
--- /dev/null
+++ b/drivers/mailbox/bcm2835-mailbox.c
@@ -0,0 +1,217 @@
+/*
+ *  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 {
+	void __iomem *regs;
+	spinlock_t lock;
+	struct mbox_controller controller;
+};
+
+static struct bcm2835_mbox *bcm2835_link_mbox(struct mbox_chan *link)
+{
+	return container_of(link->mbox, struct bcm2835_mbox, controller);
+}
+
+static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id)
+{
+	struct bcm2835_mbox *mbox = dev_id;
+	struct device *dev = mbox->controller.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)
+{
+	struct bcm2835_mbox *mbox = bcm2835_link_mbox(link);
+	int ret = 0;
+	u32 msg = *(u32 *)data;
+
+	spin_lock(&mbox->lock);
+	writel(msg, mbox->regs + MAIL1_WRT);
+	dev_dbg(mbox->controller.dev, "Request 0x%08X\n", msg);
+	spin_unlock(&mbox->lock);
+	return ret;
+}
+
+static int bcm2835_startup(struct mbox_chan *link)
+{
+	struct bcm2835_mbox *mbox = bcm2835_link_mbox(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)
+{
+	struct bcm2835_mbox *mbox = bcm2835_link_mbox(link);
+
+	writel(0, mbox->regs + MAIL0_CNF);
+}
+
+static bool bcm2835_last_tx_done(struct mbox_chan *link)
+{
+	struct bcm2835_mbox *mbox = bcm2835_link_mbox(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 struct mbox_chan *bcm2835_mbox_index_xlate(struct mbox_controller *mbox,
+		    const struct of_phandle_args *sp)
+{
+	if (sp->args_count != 0)
+		return NULL;
+
+	return &mbox->chans[0];
+}
+
+static int bcm2835_mbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int ret = 0;
+	struct resource *iomem;
+	struct bcm2835_mbox *mbox;
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (mbox == NULL)
+		return -ENOMEM;
+	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);
+		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);
+		return ret;
+	}
+
+	mbox->controller.txdone_poll = true;
+	mbox->controller.txpoll_period = 5;
+	mbox->controller.ops = &bcm2835_mbox_chan_ops;
+	mbox->controller.of_xlate = &bcm2835_mbox_index_xlate;
+	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)
+		return -ENOMEM;
+
+	ret = mbox_controller_register(&mbox->controller);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, mbox);
+	dev_info(dev, "mailbox enabled\n");
+
+	return ret;
+}
+
+static int bcm2835_mbox_remove(struct platform_device *pdev)
+{
+	struct bcm2835_mbox *mbox = platform_get_drvdata(pdev);
+	mbox_controller_unregister(&mbox->controller);
+	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 3/3 v2] 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>
---

v2: Update for #mbox-cells 0 change in the driver.

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

Re: [PATCH 2/3 v8] mailbox: Enable BCM2835 mailbox support

[Noralf Trønnes]

Den 05.05.2015 22:27, skrev 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.
>
> The driver was originally submitted by Lubomir, based on the
> out-of-tree 2708 mailbox driver.  Eric Anholt fixed it up for
> upstreaming, with the major functional change being that it now has no
> notion of multiple channels (since that is a firmware-dependent
> concept) and instead the raspberrypi-firmware driver will do that
> bit-twiddling in its own messages.
...
> +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);

I have tested this driver and the firmware driver booting directly
from the VideoCore bootloader (no uboot).
The mailbox driver loads too late to turn on USB power:

[    0.754258] platform soc:firmware: Driver raspberrypi-firmware 
requests probe deferral

[    2.041136] dwc2 20980000.usb: 256 invalid for 
host_nperio_tx_fifo_size. Check HW configuration.
[    2.050058] dwc2 20980000.usb: 512 invalid for 
host_perio_tx_fifo_size. Check HW configuration.
[    4.140894] dwc2 20980000.usb: dwc2_core_reset() HANG! Soft Reset 
GRSTCTL=80000001
[    4.148583] dwc2 20980000.usb: dwc2_core_init(): Reset failed, aborting
[    4.155281] dwc2 20980000.usb: dwc2_hcd_init() FAILED, returning -16
[    4.161720] dwc2: probe of 20980000.usb failed with error -16

[    4.272027] bcm2835-mbox 2000b880.mailbox: mailbox enabled


When I move mailbox module init to arch_initcall, USB power is turned on.

[    0.548481] bcm2835-mbox 2000b880.mailbox: mailbox enabled

[    1.733669] raspberrypi_firmware_set_power  *** I have added this
[    2.242699] USB: Power-on latency exceeded, new value 509026000 ns
[    3.109748] dwc2 20980000.usb: DWC OTG Controller
[    3.114690] dwc2 20980000.usb: new USB bus registered, assigned bus 
number 1
[    3.121890] dwc2 20980000.usb: irq 33, io mem 0x00000000

This silences the warning:
struct raspberrypi_power_domain raspberrypi_power_domain_usb = {
     .base = {
         .power_on_latency_ns = 600000000,


Noralf.

Re: [PATCH 2/3 v8] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

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

Noralf Trønnes <noralf@tronnes.org> writes:

> Den 05.05.2015 22:27, skrev 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.
>>
>> The driver was originally submitted by Lubomir, based on the
>> out-of-tree 2708 mailbox driver.  Eric Anholt fixed it up for
>> upstreaming, with the major functional change being that it now has no
>> notion of multiple channels (since that is a firmware-dependent
>> concept) and instead the raspberrypi-firmware driver will do that
>> bit-twiddling in its own messages.
> ...
>> +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);
>
> I have tested this driver and the firmware driver booting directly
> from the VideoCore bootloader (no uboot).
> The mailbox driver loads too late to turn on USB power:

Yeah, I have a patch on my branches that returns -EPROBE_DEFER when
trying to get a power domain and not finding the provider.  It was
rejected by the maintainers in favor of a proposed solution whose
description I didn't quite follow.

> This silences the warning:
> struct raspberrypi_power_domain raspberrypi_power_domain_usb = {
>      .base = {
>          .power_on_latency_ns = 600000000,

Oh, nice.  Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: Re: [PATCH 2/3 v8] mailbox: Enable BCM2835 mailbox support

[Alexander Stein]

On Thursday 07 May 2015, 12:54:20 wrote Eric Anholt:
> Noralf Trønnes <noralf@tronnes.org> writes:
> 
> > Den 05.05.2015 22:27, skrev 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.
> >>
> >> The driver was originally submitted by Lubomir, based on the
> >> out-of-tree 2708 mailbox driver.  Eric Anholt fixed it up for
> >> upstreaming, with the major functional change being that it now has no
> >> notion of multiple channels (since that is a firmware-dependent
> >> concept) and instead the raspberrypi-firmware driver will do that
> >> bit-twiddling in its own messages.
> > ...
> >> +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);
> >
> > I have tested this driver and the firmware driver booting directly
> > from the VideoCore bootloader (no uboot).
> > The mailbox driver loads too late to turn on USB power:
> 
> Yeah, I have a patch on my branches that returns -EPROBE_DEFER when
> trying to get a power domain and not finding the provider.  It was
> rejected by the maintainers in favor of a proposed solution whose
> description I didn't quite follow.

Do you have a link for this thread?

> > This silences the warning:
> > struct raspberrypi_power_domain raspberrypi_power_domain_usb = {
> >      .base = {
> >          .power_on_latency_ns = 600000000,
> 
> Oh, nice.  Thanks!

Well, Using a timeout for dependencies seems odd to me.

Regards,
Alexander

Re: Re: [PATCH 2/3 v8] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

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

Alexander Stein <alexanders83@web.de> writes:

> On Thursday 07 May 2015, 12:54:20 wrote Eric Anholt:
>> Noralf Trønnes <noralf@tronnes.org> writes:
>> 
>> > Den 05.05.2015 22:27, skrev 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.
>> >>
>> >> The driver was originally submitted by Lubomir, based on the
>> >> out-of-tree 2708 mailbox driver.  Eric Anholt fixed it up for
>> >> upstreaming, with the major functional change being that it now has no
>> >> notion of multiple channels (since that is a firmware-dependent
>> >> concept) and instead the raspberrypi-firmware driver will do that
>> >> bit-twiddling in its own messages.
>> > ...
>> >> +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);
>> >
>> > I have tested this driver and the firmware driver booting directly
>> > from the VideoCore bootloader (no uboot).
>> > The mailbox driver loads too late to turn on USB power:
>> 
>> Yeah, I have a patch on my branches that returns -EPROBE_DEFER when
>> trying to get a power domain and not finding the provider.  It was
>> rejected by the maintainers in favor of a proposed solution whose
>> description I didn't quite follow.
>
> Do you have a link for this thread?

https://lkml.org/lkml/2015/3/11/483

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/3 v8] mailbox: Enable BCM2835 mailbox support

[Noralf Trønnes]

Den 08.05.2015 10:33, skrev Alexander Stein:
> On Thursday 07 May 2015, 12:54:20 wrote Eric Anholt:
>> Noralf Trønnes <noralf@tronnes.org> writes:
>>
>>> Den 05.05.2015 22:27, skrev 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.
>>>>
>>>> The driver was originally submitted by Lubomir, based on the
>>>> out-of-tree 2708 mailbox driver.  Eric Anholt fixed it up for
>>>> upstreaming, with the major functional change being that it now has no
>>>> notion of multiple channels (since that is a firmware-dependent
>>>> concept) and instead the raspberrypi-firmware driver will do that
>>>> bit-twiddling in its own messages.
>>> ...
>>>> +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);
>>> I have tested this driver and the firmware driver booting directly
>>> from the VideoCore bootloader (no uboot).
>>> The mailbox driver loads too late to turn on USB power:
>> Yeah, I have a patch on my branches that returns -EPROBE_DEFER when
>> trying to get a power domain and not finding the provider.  It was
>> rejected by the maintainers in favor of a proposed solution whose
>> description I didn't quite follow.
> Do you have a link for this thread?
>
>>> This silences the warning:
>>> struct raspberrypi_power_domain raspberrypi_power_domain_usb = {
>>>       .base = {
>>>           .power_on_latency_ns = 600000000,
>> Oh, nice.  Thanks!
> Well, Using a timeout for dependencies seems odd to me.

I can only find one place where power_on_latency_ns is set,
arch/arm/mach-imx/gpc.c:
static struct pu_domain imx6q_pu_domain = {
     .base = {
         .name = "PU",
         .power_off = imx6q_pm_pu_power_off,
         .power_on = imx6q_pm_pu_power_on,
         .power_off_latency_ns = 25000,
         .power_on_latency_ns = 2000000,
     },
};

power_on_latency_ns is not set by the core, so it defaults to zero.
So in the default case, genpd_power_on() will always issue a warning on
the first power on.

I would say that power_on_latency_ns is a characteristic of the power
domain, like enable_time for regulators.


Noralf.

Re: [PATCH 2/3 v8] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 05/08/2015 01:19 PM, Eric Anholt wrote:
> Alexander Stein <alexanders83@web.de> writes:
> 
>> On Thursday 07 May 2015, 12:54:20 wrote Eric Anholt:
>>> Noralf Trønnes <noralf@tronnes.org> writes:
>>> 
>>>> Den 05.05.2015 22:27, skrev 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.
>>>>> 
>>>>> The driver was originally submitted by Lubomir, based on
>>>>> the out-of-tree 2708 mailbox driver.  Eric Anholt fixed it
>>>>> up for upstreaming, with the major functional change being
>>>>> that it now has no notion of multiple channels (since that
>>>>> is a firmware-dependent concept) and instead the
>>>>> raspberrypi-firmware driver will do that bit-twiddling in
>>>>> its own messages.
>>>> ...
>>>>> +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);
>>>> 
>>>> I have tested this driver and the firmware driver booting
>>>> directly from the VideoCore bootloader (no uboot). The
>>>> mailbox driver loads too late to turn on USB power:
>>> 
>>> Yeah, I have a patch on my branches that returns -EPROBE_DEFER
>>> when trying to get a power domain and not finding the provider.
>>> It was rejected by the maintainers in favor of a proposed
>>> solution whose description I didn't quite follow.
>> 
>> Do you have a link for this thread?
> 
> https://lkml.org/lkml/2015/3/11/483

That's really odd; -EPROBE_DEFER was clearly invented exactly to
handle dependencies just like this. Playing with initcall levels
simply isn't scalable, and in the main people are actively working not
to use them for dependencies like this; they're far too implicit.
While the timeout mentioned earlier in the thread might work (I didn't
really look at the details), again it's far too indirect/accidental to
be a good solution.

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

[Jassi Brar]

On Wed, May 6, 2015 at 1:57 AM, Eric Anholt <eric@anholt.net> wrote:
> From: Lubomir Rintel <lkundrak@v3.sk>
>
> This patch was split out of Lubomir's original mailbox patch by Eric
> Anholt, and the required properties documentation and examples have
> been filled out more completely and updated for the driver being
> changed to expose a single channel.
>
> 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>
> ---
Thanks to the reviewers. Applied all 3 patches.

Thanks.

Re: [PATCH 2/3 v8] mailbox: Enable BCM2835 mailbox support

[Noralf Trønnes]

Den 05.05.2015 22:27, skrev 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.
>
> The driver was originally submitted by Lubomir, based on the
> out-of-tree 2708 mailbox driver.  Eric Anholt fixed it up for
> upstreaming, with the major functional change being that it now has no
> notion of multiple channels (since that is a firmware-dependent
> concept) and instead the raspberrypi-firmware driver will do that
> bit-twiddling in its own messages.
>
> 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>
> Acked-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
[...]
> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
[...]
> +static bool bcm2835_last_tx_done(struct mbox_chan *link)
> +{
> +	struct bcm2835_mbox *mbox = bcm2835_link_mbox(link);
> +	bool ret;
> +
> +	spin_lock(&mbox->lock);
> +	ret = !(readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL);

I don't know how the mailbox framework uses this function,
but MAIL0_STA might be wrong here.

Phil Elwell detected a bug in the downstream mailbox driver in
the mbox_write function where it waits for space to be available
in the FIFO. His observation:
With the VC reader blocked and the ARM writing, MAIL0_STA reads
empty permanently while MAIL1_STA goes from empty (0x40000000)
to non-empty (0x00000001-0x00000007) to full (0x80000008).

See:
https://github.com/raspberrypi/linux/commit/3018d8a0996ad2340ba1b3f473f705ef285b01b5

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

[Lee Jones]

On Tue, 12 May 2015, Jassi Brar wrote:
> On Wed, May 6, 2015 at 1:57 AM, Eric Anholt <eric@anholt.net> wrote:
> > From: Lubomir Rintel <lkundrak@v3.sk>
> >
> > This patch was split out of Lubomir's original mailbox patch by Eric
> > Anholt, and the required properties documentation and examples have
> > been filled out more completely and updated for the driver being
> > changed to expose a single channel.
> >
> > 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>
> > ---
> Thanks to the reviewers. Applied all 3 patches.

Jassi, can you remove patch 3/3 from your tree please?  Changes to
DTS(I) files should go in via ARM-SoC.  If SS Maintainers took DT
changes, there would be carnage.

I'm going to apply it now.

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

[Lee Jones]

On Tue, 05 May 2015, Eric Anholt wrote:

> Signed-off-by: Eric Anholt <eric@anholt.net>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> Acked-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
> 
> v2: Update for #mbox-cells 0 change in the driver.
> 
>  arch/arm/boot/dts/bcm2835.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)

Applied, thanks.

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

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

[Jassi Brar]

On Wed, May 13, 2015 at 1:27 PM, Lee Jones <lee@kernel.org> wrote:
> On Tue, 12 May 2015, Jassi Brar wrote:
>> On Wed, May 6, 2015 at 1:57 AM, Eric Anholt <eric@anholt.net> wrote:
>> > From: Lubomir Rintel <lkundrak@v3.sk>
>> >
>> > This patch was split out of Lubomir's original mailbox patch by Eric
>> > Anholt, and the required properties documentation and examples have
>> > been filled out more completely and updated for the driver being
>> > changed to expose a single channel.
>> >
>> > 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>
>> > ---
>> Thanks to the reviewers. Applied all 3 patches.
>
> Jassi, can you remove patch 3/3 from your tree please?  Changes to
> DTS(I) files should go in via ARM-SoC.  If SS Maintainers took DT
> changes, there would be carnage.
>
> I'm going to apply it now.
>
Done. Thanks for the heads up.