Raspberry Pi mailbox drivers

Raspberry Pi mailbox drivers

[Eric Anholt]

This series can also be found at:

https://github.com/anholt/linux/tree/rpi-mailbox

The generic mailbox stuff was seen on the list last October:

http://marc.info/?l=linux-arm-kernel&m=141415695904815&w=2

which got review comments and then, as far as I can see, never got
resubmitted.  I think I've dealt with those comments except for the
following:

- Kconfig should say "Y or M" instead of "Y"

It looks like the majority behavior in Kconfig is to just say "Y"
generally, even for module things.

- Alphabetizing Makefile / Kconfig additions

In the time since the submission, everything else has been added in
submission order rather than alphabetizing, so I'm following that.

- Authorship (Lubomir or Craig) on the patch

Lubomir's patch (the one being reviewed) said it was squash of Craig's
onto previous work by Lubomir.  Craig's patch
(http://marc.info/?l=linux-arm-kernel&m=137907209606316&w=2) looked
like it was a replacement of a previous driver by Lubomir with a
rewrite onto the generic mailbox support.  Enough got rewritten in
Craig's rework that I could easily see his name going on the patch,
but I think it's a derivative of Lubomir's.  However, Lubomir did go
on to do some significant cleanup of Craig's work, as well.  I've left
Lubomir's name on the patch and made the MODULE_AUTHOR be him.  If
Craig is bothered by this, I'd be happy to try to work something out,
but this is one of those places where a certain style of development
is hard to fairly represent given the way we like history to happen in
the kernel tree.

My two driver bits haven't been seen on the list before.  I did read
some other drivers in the process of writing them (particularly the
downstream Raspberry Pi vcio.c and power.c).  I've tested the property
channel with some ad-hoc code to dump a few properties, and the
results look good.

I haven't added a MAINTAINERS entry for this series.  I'm personally
happy with the maintainers being the 2835 arch maintainers, but I'd
also be willing to add myself as a maintainer if that was desired.  I
should also be able to answer questions about the firmware behavior,
which would probably be useful to anyone trying to build more drivers
using this code.

[PATCH 02/10] 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

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

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
Signed-off-by: Suman Anna <s-anna@ti.com>
Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com>
Signed-off-by: Eric Anholt <eric@anholt.net>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Jassi Brar <jassisinghbrar@gmail.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: linux-rpi-kernel@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/mailbox/Kconfig           |   8 ++
 drivers/mailbox/Makefile          |   2 +
 drivers/mailbox/bcm2835-mailbox.c | 284 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 294 insertions(+)
 create mode 100644 drivers/mailbox/bcm2835-mailbox.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 84325f2..2873a03 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -51,4 +51,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 2e79231..7feb8da 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -9,3 +9,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..604beb7
--- /dev/null
+++ b/drivers/mailbox/bcm2835-mailbox.c
@@ -0,0 +1,284 @@
+/*
+ *  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)
+
+#define MBOX_CHAN_COUNT		16
+
+/* Status register: FIFO state. */
+#define ARM_MS_FULL		0x80000000
+#define ARM_MS_EMPTY		0x40000000
+
+/* Configuration register: Enable interrupts. */
+#define ARM_MC_IHAVEDATAIRQEN	0x00000001
+
+#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) & 0xf))
+#define MBOX_CHAN(msg)			((msg) & 0xf)
+#define MBOX_DATA28(msg)		((msg) & ~0xf)
+
+struct bcm2835_mbox;
+
+struct bcm2835_channel {
+	struct bcm2835_mbox *mbox;
+	struct mbox_chan *link;
+	u32 chan_num;
+	bool started;
+};
+
+struct bcm2835_mbox {
+	struct platform_device *pdev;
+	struct device *dev;
+	void __iomem *regs;
+	spinlock_t lock;
+	struct bcm2835_channel channel[MBOX_CHAN_COUNT];
+	struct mbox_controller controller;
+};
+
+#define to_channel(link) ((struct bcm2835_channel *)link->con_priv)
+
+static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id)
+{
+	struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id;
+	struct device *dev = mbox->dev;
+
+	while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
+		u32 msg = readl(mbox->regs + MAIL0_RD);
+		unsigned int chan = MBOX_CHAN(msg);
+
+		if (!mbox->channel[chan].started) {
+			dev_err(dev, "Reply on stopped channel %d\n", chan);
+			continue;
+		}
+		dev_dbg(dev, "Reply 0x%08X\n", msg);
+		mbox_chan_received_data(mbox->channel[chan].link,
+			(void *) MBOX_DATA28(msg));
+	}
+	rmb(); /* Finished last mailbox read. */
+	return IRQ_HANDLED;
+}
+
+static int bcm2835_send_data(struct mbox_chan *link, void *data)
+{
+	struct bcm2835_channel *chan = to_channel(link);
+	struct bcm2835_mbox *mbox = chan->mbox;
+	int ret = 0;
+
+	if (!chan->started)
+		return -ENODEV;
+	spin_lock(&mbox->lock);
+	if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) {
+		rmb(); /* Finished last mailbox read. */
+		ret = -EBUSY;
+		goto end;
+	}
+	wmb(); /* About to write to the mail box. */
+	writel(MBOX_MSG(chan->chan_num, (u32) data), mbox->regs + MAIL1_WRT);
+	dev_dbg(mbox->dev, "Request 0x%08X\n", MBOX_MSG(chan->chan_num,
+		(u32) data));
+end:
+	spin_unlock(&mbox->lock);
+	return ret;
+}
+
+static int bcm2835_startup(struct mbox_chan *link)
+{
+	struct bcm2835_channel *chan = to_channel(link);
+
+	chan->started = true;
+	return 0;
+}
+
+static void bcm2835_shutdown(struct mbox_chan *link)
+{
+	struct bcm2835_channel *chan = to_channel(link);
+
+	chan->started = false;
+}
+
+static bool bcm2835_last_tx_done(struct mbox_chan *link)
+{
+	struct bcm2835_channel *chan = to_channel(link);
+	struct bcm2835_mbox *mbox = chan->mbox;
+	bool ret;
+
+	if (!chan->started)
+		return false;
+	spin_lock(&mbox->lock);
+	ret = !(readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL);
+	rmb(); /* Finished last mailbox read. */
+	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 request_mailbox_iomem(struct bcm2835_mbox *mbox)
+{
+	struct platform_device *pdev = mbox->pdev;
+	struct resource *iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	dev_dbg(&pdev->dev, "iomem 0x%08X-0x%08X\n", iomem->start, iomem->end);
+	mbox->regs = devm_ioremap_resource(&pdev->dev, iomem);
+	if (IS_ERR(mbox->regs)) {
+		dev_err(&pdev->dev, "Failed to remap mailbox regs\n");
+		return PTR_ERR(mbox->regs);
+	}
+	return 0;
+}
+
+static int request_mailbox_irq(struct bcm2835_mbox *mbox)
+{
+	int ret;
+	struct device *dev = mbox->dev;
+	struct device_node *np = dev->of_node;
+	int irq = irq_of_parse_and_map(np, 0);
+
+	if (irq <= 0) {
+		dev_err(dev, "Can't get IRQ number for mailbox\n");
+		return -ENODEV;
+	}
+	ret = devm_request_irq(dev, irq, bcm2835_mbox_irq, 0, dev_name(dev),
+		mbox);
+	if (ret) {
+		dev_err(dev, "Failed to register a mailbox IRQ handler\n");
+		return -ENODEV;
+	}
+	return 0;
+}
+
+static int bcm2835_mbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct bcm2835_mbox *mbox;
+	int i;
+	int ret = 0;
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (mbox == NULL) {
+		dev_err(dev, "Failed to allocate mailbox memory\n");
+		return -ENOMEM;
+	}
+	platform_set_drvdata(pdev, mbox);
+	mbox->pdev = pdev;
+	mbox->dev = dev;
+	spin_lock_init(&mbox->lock);
+
+	ret = request_mailbox_irq(mbox);
+	if (ret)
+		return ret;
+	ret = request_mailbox_iomem(mbox);
+	if (ret)
+		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 = MBOX_CHAN_COUNT;
+	mbox->controller.chans = devm_kzalloc(dev,
+		sizeof(struct mbox_chan) * MBOX_CHAN_COUNT,
+		GFP_KERNEL);
+	if (!mbox->controller.chans) {
+		dev_err(dev, "Failed to alloc mbox_chans\n");
+		return -ENOMEM;
+	}
+
+	for (i = 0; i != MBOX_CHAN_COUNT; ++i) {
+		mbox->channel[i].mbox = mbox;
+		mbox->channel[i].link = &mbox->controller.chans[i];
+		mbox->channel[i].chan_num = i;
+		mbox->controller.chans[i].con_priv =
+			(void *)&mbox->channel[i];
+	}
+
+	ret  = mbox_controller_register(&mbox->controller);
+	if (ret)
+		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)
+{
+	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 02/10] 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

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

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
---
 drivers/mailbox/Kconfig           |   8 ++
 drivers/mailbox/Makefile          |   2 +
 drivers/mailbox/bcm2835-mailbox.c | 284 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 294 insertions(+)
 create mode 100644 drivers/mailbox/bcm2835-mailbox.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 84325f2..2873a03 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -51,4 +51,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 2e79231..7feb8da 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -9,3 +9,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..604beb7
--- /dev/null
+++ b/drivers/mailbox/bcm2835-mailbox.c
@@ -0,0 +1,284 @@
+/*
+ *  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)
+
+#define MBOX_CHAN_COUNT		16
+
+/* Status register: FIFO state. */
+#define ARM_MS_FULL		0x80000000
+#define ARM_MS_EMPTY		0x40000000
+
+/* Configuration register: Enable interrupts. */
+#define ARM_MC_IHAVEDATAIRQEN	0x00000001
+
+#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) & 0xf))
+#define MBOX_CHAN(msg)			((msg) & 0xf)
+#define MBOX_DATA28(msg)		((msg) & ~0xf)
+
+struct bcm2835_mbox;
+
+struct bcm2835_channel {
+	struct bcm2835_mbox *mbox;
+	struct mbox_chan *link;
+	u32 chan_num;
+	bool started;
+};
+
+struct bcm2835_mbox {
+	struct platform_device *pdev;
+	struct device *dev;
+	void __iomem *regs;
+	spinlock_t lock;
+	struct bcm2835_channel channel[MBOX_CHAN_COUNT];
+	struct mbox_controller controller;
+};
+
+#define to_channel(link) ((struct bcm2835_channel *)link->con_priv)
+
+static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id)
+{
+	struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id;
+	struct device *dev = mbox->dev;
+
+	while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
+		u32 msg = readl(mbox->regs + MAIL0_RD);
+		unsigned int chan = MBOX_CHAN(msg);
+
+		if (!mbox->channel[chan].started) {
+			dev_err(dev, "Reply on stopped channel %d\n", chan);
+			continue;
+		}
+		dev_dbg(dev, "Reply 0x%08X\n", msg);
+		mbox_chan_received_data(mbox->channel[chan].link,
+			(void *) MBOX_DATA28(msg));
+	}
+	rmb(); /* Finished last mailbox read. */
+	return IRQ_HANDLED;
+}
+
+static int bcm2835_send_data(struct mbox_chan *link, void *data)
+{
+	struct bcm2835_channel *chan = to_channel(link);
+	struct bcm2835_mbox *mbox = chan->mbox;
+	int ret = 0;
+
+	if (!chan->started)
+		return -ENODEV;
+	spin_lock(&mbox->lock);
+	if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) {
+		rmb(); /* Finished last mailbox read. */
+		ret = -EBUSY;
+		goto end;
+	}
+	wmb(); /* About to write to the mail box. */
+	writel(MBOX_MSG(chan->chan_num, (u32) data), mbox->regs + MAIL1_WRT);
+	dev_dbg(mbox->dev, "Request 0x%08X\n", MBOX_MSG(chan->chan_num,
+		(u32) data));
+end:
+	spin_unlock(&mbox->lock);
+	return ret;
+}
+
+static int bcm2835_startup(struct mbox_chan *link)
+{
+	struct bcm2835_channel *chan = to_channel(link);
+
+	chan->started = true;
+	return 0;
+}
+
+static void bcm2835_shutdown(struct mbox_chan *link)
+{
+	struct bcm2835_channel *chan = to_channel(link);
+
+	chan->started = false;
+}
+
+static bool bcm2835_last_tx_done(struct mbox_chan *link)
+{
+	struct bcm2835_channel *chan = to_channel(link);
+	struct bcm2835_mbox *mbox = chan->mbox;
+	bool ret;
+
+	if (!chan->started)
+		return false;
+	spin_lock(&mbox->lock);
+	ret = !(readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL);
+	rmb(); /* Finished last mailbox read. */
+	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 request_mailbox_iomem(struct bcm2835_mbox *mbox)
+{
+	struct platform_device *pdev = mbox->pdev;
+	struct resource *iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	dev_dbg(&pdev->dev, "iomem 0x%08X-0x%08X\n", iomem->start, iomem->end);
+	mbox->regs = devm_ioremap_resource(&pdev->dev, iomem);
+	if (IS_ERR(mbox->regs)) {
+		dev_err(&pdev->dev, "Failed to remap mailbox regs\n");
+		return PTR_ERR(mbox->regs);
+	}
+	return 0;
+}
+
+static int request_mailbox_irq(struct bcm2835_mbox *mbox)
+{
+	int ret;
+	struct device *dev = mbox->dev;
+	struct device_node *np = dev->of_node;
+	int irq = irq_of_parse_and_map(np, 0);
+
+	if (irq <= 0) {
+		dev_err(dev, "Can't get IRQ number for mailbox\n");
+		return -ENODEV;
+	}
+	ret = devm_request_irq(dev, irq, bcm2835_mbox_irq, 0, dev_name(dev),
+		mbox);
+	if (ret) {
+		dev_err(dev, "Failed to register a mailbox IRQ handler\n");
+		return -ENODEV;
+	}
+	return 0;
+}
+
+static int bcm2835_mbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct bcm2835_mbox *mbox;
+	int i;
+	int ret = 0;
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (mbox == NULL) {
+		dev_err(dev, "Failed to allocate mailbox memory\n");
+		return -ENOMEM;
+	}
+	platform_set_drvdata(pdev, mbox);
+	mbox->pdev = pdev;
+	mbox->dev = dev;
+	spin_lock_init(&mbox->lock);
+
+	ret = request_mailbox_irq(mbox);
+	if (ret)
+		return ret;
+	ret = request_mailbox_iomem(mbox);
+	if (ret)
+		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 = MBOX_CHAN_COUNT;
+	mbox->controller.chans = devm_kzalloc(dev,
+		sizeof(struct mbox_chan) * MBOX_CHAN_COUNT,
+		GFP_KERNEL);
+	if (!mbox->controller.chans) {
+		dev_err(dev, "Failed to alloc mbox_chans\n");
+		return -ENOMEM;
+	}
+
+	for (i = 0; i != MBOX_CHAN_COUNT; ++i) {
+		mbox->channel[i].mbox = mbox;
+		mbox->channel[i].link = &mbox->controller.chans[i];
+		mbox->channel[i].chan_num = i;
+		mbox->controller.chans[i].con_priv =
+			(void *)&mbox->channel[i];
+	}
+
+	ret  = mbox_controller_register(&mbox->controller);
+	if (ret)
+		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)
+{
+	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

Re: [PATCH 02/10] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/02/2015 01:54 PM, Eric Anholt wrote:
> From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
> 
> 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.

> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c

> +/* 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)

That implies there are more mailboxes. I wonder if we should
parameterize which to use via some DT properties? I guess we can defer
that though; we can default to the current values and add properties
later if we want to use something else.

> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id)
> +{
> +	struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id;
> +	struct device *dev = mbox->dev;
> +
> +	while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
> +		u32 msg = readl(mbox->regs + MAIL0_RD);
> +		unsigned int chan = MBOX_CHAN(msg);
> +
> +		if (!mbox->channel[chan].started) {
> +			dev_err(dev, "Reply on stopped channel %d\n", chan);
> +			continue;
> +		}
> +		dev_dbg(dev, "Reply 0x%08X\n", msg);
> +		mbox_chan_received_data(mbox->channel[chan].link,
> +			(void *) MBOX_DATA28(msg));
> +	}
> +	rmb(); /* Finished last mailbox read. */

I know the PDF mentioned in the comment earlier in the patch says to put
in barriers between accesses to different peripherals, which this seems
compliant with, but I don't see quite what this barrier achieves. I
think the PDF is talking generalities, not imposing a rule that must be
blindly followed. Besides, if there's a context-switch you can't
actually implement the rules the PDF suggests. What read operation is
this barrier attempting to ensure happens after reading all mailbox
messages and any associated DRAM buffer?

If any barrier is needed, shouldn't it be between the mailbox read and
the processing of that message (which could at least in some cases read
an SDRAM buffer). So, the producer would do roughly:

p1) Fill in DRAM buffer
p2) Write memory barrier so the MBOX write happens after the above
p3) Send mbox message to tell the consumer to process the buffer

... and the consumer:

c1) Read MBOX register to know which DRAM buffer to handle
c2) rmb() to make sure we read from the DRAM buffer after the MBOX read
c3) Read the DRAM buffer

Even then, since (c3) is data-dependent on (c1), I don't think the rmb()
in (c2) there actually does anything useful.

> +static int bcm2835_send_data(struct mbox_chan *link, void *data)
> +{
> +	struct bcm2835_channel *chan = to_channel(link);
> +	struct bcm2835_mbox *mbox = chan->mbox;
> +	int ret = 0;
> +
> +	if (!chan->started)
> +		return -ENODEV;
> +	spin_lock(&mbox->lock);

Is it safe to read chan->started without the channel lock held?

> +	if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) {
> +		rmb(); /* Finished last mailbox read. */

This also doesn't seem useful?

> +		ret = -EBUSY;
> +		goto end;
> +	}
> +	wmb(); /* About to write to the mail box. */
> +	writel(MBOX_MSG(chan->chan_num, (u32) data), mbox->regs + MAIL1_WRT);

This one I agree with, at least if MBOX messages contain pointers to
DRAM buffers.

> +static int bcm2835_startup(struct mbox_chan *link)
> +{
> +	struct bcm2835_channel *chan = to_channel(link);
> +
> +	chan->started = true;
> +	return 0;
> +}
> +
> +static void bcm2835_shutdown(struct mbox_chan *link)
> +{
> +	struct bcm2835_channel *chan = to_channel(link);
> +
> +	chan->started = false;
> +}

Don't we need to hold chan->lock when adjusting chan->started? Or is
start/stop intended to be asynchronous to any operations currently in
progress on the channel?

> +static bool bcm2835_last_tx_done(struct mbox_chan *link)
> +{
> +	struct bcm2835_channel *chan = to_channel(link);
> +	struct bcm2835_mbox *mbox = chan->mbox;
> +	bool ret;
> +
> +	if (!chan->started)
> +		return false;
> +	spin_lock(&mbox->lock);
> +	ret = !(readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL);
> +	rmb(); /* Finished last mailbox read. */

That barrier doesn't seem useful?

What are the semantics of "tx done"? This seems to be testing that the
TX mailbox isn't completely full, which is more about whether the
consumer side is backed up rather than whether our producer-side TX
operations are done.

If the idea is to wait for the consumer to have consumed everything in
our TX direction, shouldn't this check for empty not !full?

> +static int request_mailbox_irq(struct bcm2835_mbox *mbox)

> +	if (irq <= 0) {
> +		dev_err(dev, "Can't get IRQ number for mailbox\n");
> +		return -ENODEV;
> +	}

I expect devm_request_irq() checkes that condition.

> +	ret = devm_request_irq(dev, irq, bcm2835_mbox_irq, 0, dev_name(dev),
> +		mbox);
> +	if (ret) {
> +		dev_err(dev, "Failed to register a mailbox IRQ handler\n");

Printing ret might be useful to know why.

Are you sure devm_request_irq() is appropriate? The IRQ handler will be
unregistered *after* bcm2835_mbox_remove() is called, and I think
without any guarantee re: the order that other devm_ allocations are
cleaned up. If bcm2835_mbox_remove() can't guarantee that no IRQ will
fire after it exits, then bcm2835_mbox_irq() might just get called after
some allocations are torn down, thus causing the IRQ handler to touch
free'd memory.

Both request_mailbox_iomem and request_mailbox_irq are small enough
they're typically written inline into the main probe() function.

> +static int bcm2835_mbox_probe(struct platform_device *pdev)

> +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> +	if (mbox == NULL) {
> +		dev_err(dev, "Failed to allocate mailbox memory\n");

devm_kzalloc() already prints an error, so no need to add another here,
even if it does nicely document the fact that you remembered error
messages:-)

> +	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 = MBOX_CHAN_COUNT;
> +	mbox->controller.chans = devm_kzalloc(dev,
> +		sizeof(struct mbox_chan) * MBOX_CHAN_COUNT,
> +		GFP_KERNEL);

It'd be common to say "sizeof(*mbox->controller.chans) so the type can't
mismatch what's being assigned to.

> +	if (!mbox->controller.chans) {
> +		dev_err(dev, "Failed to alloc mbox_chans\n");

Same comment about error messages here.

> +	/* Enable the interrupt on data reception */
> +	writel(ARM_MC_IHAVEDATAIRQEN, mbox->regs + MAIL0_CNF);
> +	dev_info(dev, "mailbox enabled\n");

There's no interrupt for "TX space available"? Oh well. I guess that's
why mbox->controller.txdone_poll = true.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/10] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/02/2015 01:54 PM, Eric Anholt 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.

> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c

> +/* 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)

That implies there are more mailboxes. I wonder if we should
parameterize which to use via some DT properties? I guess we can defer
that though; we can default to the current values and add properties
later if we want to use something else.

> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id)
> +{
> +	struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id;
> +	struct device *dev = mbox->dev;
> +
> +	while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
> +		u32 msg = readl(mbox->regs + MAIL0_RD);
> +		unsigned int chan = MBOX_CHAN(msg);
> +
> +		if (!mbox->channel[chan].started) {
> +			dev_err(dev, "Reply on stopped channel %d\n", chan);
> +			continue;
> +		}
> +		dev_dbg(dev, "Reply 0x%08X\n", msg);
> +		mbox_chan_received_data(mbox->channel[chan].link,
> +			(void *) MBOX_DATA28(msg));
> +	}
> +	rmb(); /* Finished last mailbox read. */

I know the PDF mentioned in the comment earlier in the patch says to put
in barriers between accesses to different peripherals, which this seems
compliant with, but I don't see quite what this barrier achieves. I
think the PDF is talking generalities, not imposing a rule that must be
blindly followed. Besides, if there's a context-switch you can't
actually implement the rules the PDF suggests. What read operation is
this barrier attempting to ensure happens after reading all mailbox
messages and any associated DRAM buffer?

If any barrier is needed, shouldn't it be between the mailbox read and
the processing of that message (which could at least in some cases read
an SDRAM buffer). So, the producer would do roughly:

p1) Fill in DRAM buffer
p2) Write memory barrier so the MBOX write happens after the above
p3) Send mbox message to tell the consumer to process the buffer

... and the consumer:

c1) Read MBOX register to know which DRAM buffer to handle
c2) rmb() to make sure we read from the DRAM buffer after the MBOX read
c3) Read the DRAM buffer

Even then, since (c3) is data-dependent on (c1), I don't think the rmb()
in (c2) there actually does anything useful.

> +static int bcm2835_send_data(struct mbox_chan *link, void *data)
> +{
> +	struct bcm2835_channel *chan = to_channel(link);
> +	struct bcm2835_mbox *mbox = chan->mbox;
> +	int ret = 0;
> +
> +	if (!chan->started)
> +		return -ENODEV;
> +	spin_lock(&mbox->lock);

Is it safe to read chan->started without the channel lock held?

> +	if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) {
> +		rmb(); /* Finished last mailbox read. */

This also doesn't seem useful?

> +		ret = -EBUSY;
> +		goto end;
> +	}
> +	wmb(); /* About to write to the mail box. */
> +	writel(MBOX_MSG(chan->chan_num, (u32) data), mbox->regs + MAIL1_WRT);

This one I agree with, at least if MBOX messages contain pointers to
DRAM buffers.

> +static int bcm2835_startup(struct mbox_chan *link)
> +{
> +	struct bcm2835_channel *chan = to_channel(link);
> +
> +	chan->started = true;
> +	return 0;
> +}
> +
> +static void bcm2835_shutdown(struct mbox_chan *link)
> +{
> +	struct bcm2835_channel *chan = to_channel(link);
> +
> +	chan->started = false;
> +}

Don't we need to hold chan->lock when adjusting chan->started? Or is
start/stop intended to be asynchronous to any operations currently in
progress on the channel?

> +static bool bcm2835_last_tx_done(struct mbox_chan *link)
> +{
> +	struct bcm2835_channel *chan = to_channel(link);
> +	struct bcm2835_mbox *mbox = chan->mbox;
> +	bool ret;
> +
> +	if (!chan->started)
> +		return false;
> +	spin_lock(&mbox->lock);
> +	ret = !(readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL);
> +	rmb(); /* Finished last mailbox read. */

That barrier doesn't seem useful?

What are the semantics of "tx done"? This seems to be testing that the
TX mailbox isn't completely full, which is more about whether the
consumer side is backed up rather than whether our producer-side TX
operations are done.

If the idea is to wait for the consumer to have consumed everything in
our TX direction, shouldn't this check for empty not !full?

> +static int request_mailbox_irq(struct bcm2835_mbox *mbox)

> +	if (irq <= 0) {
> +		dev_err(dev, "Can't get IRQ number for mailbox\n");
> +		return -ENODEV;
> +	}

I expect devm_request_irq() checkes that condition.

> +	ret = devm_request_irq(dev, irq, bcm2835_mbox_irq, 0, dev_name(dev),
> +		mbox);
> +	if (ret) {
> +		dev_err(dev, "Failed to register a mailbox IRQ handler\n");

Printing ret might be useful to know why.

Are you sure devm_request_irq() is appropriate? The IRQ handler will be
unregistered *after* bcm2835_mbox_remove() is called, and I think
without any guarantee re: the order that other devm_ allocations are
cleaned up. If bcm2835_mbox_remove() can't guarantee that no IRQ will
fire after it exits, then bcm2835_mbox_irq() might just get called after
some allocations are torn down, thus causing the IRQ handler to touch
free'd memory.

Both request_mailbox_iomem and request_mailbox_irq are small enough
they're typically written inline into the main probe() function.

> +static int bcm2835_mbox_probe(struct platform_device *pdev)

> +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> +	if (mbox == NULL) {
> +		dev_err(dev, "Failed to allocate mailbox memory\n");

devm_kzalloc() already prints an error, so no need to add another here,
even if it does nicely document the fact that you remembered error
messages:-)

> +	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 = MBOX_CHAN_COUNT;
> +	mbox->controller.chans = devm_kzalloc(dev,
> +		sizeof(struct mbox_chan) * MBOX_CHAN_COUNT,
> +		GFP_KERNEL);

It'd be common to say "sizeof(*mbox->controller.chans) so the type can't
mismatch what's being assigned to.

> +	if (!mbox->controller.chans) {
> +		dev_err(dev, "Failed to alloc mbox_chans\n");

Same comment about error messages here.

> +	/* Enable the interrupt on data reception */
> +	writel(ARM_MC_IHAVEDATAIRQEN, mbox->regs + MAIL0_CNF);
> +	dev_info(dev, "mailbox enabled\n");

There's no interrupt for "TX space available"? Oh well. I guess that's
why mbox->controller.txdone_poll = true.

Re: [PATCH 02/10] mailbox: Enable BCM2835 mailbox support

[Arnd Bergmann]

On Tuesday 03 March 2015 20:03:13 Stephen Warren wrote:
> > +
> > +/*
> > + * 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)
> 
> That implies there are more mailboxes. I wonder if we should
> parameterize which to use via some DT properties? I guess we can defer
> that though; we can default to the current values and add properties
> later if we want to use something else.

How about changing #mbox-cells to <2> and using the first cell to
identify the mailbox and the second to identify the channel?

The binding isn't very clear on the meaning of the one argument
cell for the mailbox reference, but I assume it's used for the
mailbox channel rather than the mailbox id.

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

[PATCH 02/10] mailbox: Enable BCM2835 mailbox support

[Arnd Bergmann]

On Tuesday 03 March 2015 20:03:13 Stephen Warren wrote:
> > +
> > +/*
> > + * 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)
> 
> That implies there are more mailboxes. I wonder if we should
> parameterize which to use via some DT properties? I guess we can defer
> that though; we can default to the current values and add properties
> later if we want to use something else.

How about changing #mbox-cells to <2> and using the first cell to
identify the mailbox and the second to identify the channel?

The binding isn't very clear on the meaning of the one argument
cell for the mailbox reference, but I assume it's used for the
mailbox channel rather than the mailbox id.

	Arnd

Re: [PATCH 02/10] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

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

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:

> On 03/02/2015 01:54 PM, Eric Anholt wrote:
>> From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
>> 
>> 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.
>
>> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
>
>> +/* 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)
>
> That implies there are more mailboxes. I wonder if we should
> parameterize which to use via some DT properties? I guess we can defer
> that though; we can default to the current values and add properties
> later if we want to use something else.

Yeah, until there's something to talk to in another mailbox, this seems
fine.

>> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id)
>> +{
>> +	struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id;
>> +	struct device *dev = mbox->dev;
>> +
>> +	while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
>> +		u32 msg = readl(mbox->regs + MAIL0_RD);
>> +		unsigned int chan = MBOX_CHAN(msg);
>> +
>> +		if (!mbox->channel[chan].started) {
>> +			dev_err(dev, "Reply on stopped channel %d\n", chan);
>> +			continue;
>> +		}
>> +		dev_dbg(dev, "Reply 0x%08X\n", msg);
>> +		mbox_chan_received_data(mbox->channel[chan].link,
>> +			(void *) MBOX_DATA28(msg));
>> +	}
>> +	rmb(); /* Finished last mailbox read. */
>
> I know the PDF mentioned in the comment earlier in the patch says to put
> in barriers between accesses to different peripherals, which this seems
> compliant with, but I don't see quite what this barrier achieves. I
> think the PDF is talking generalities, not imposing a rule that must be
> blindly followed. Besides, if there's a context-switch you can't
> actually implement the rules the PDF suggests. What read operation is
> this barrier attempting to ensure happens after reading all mailbox
> messages and any associated DRAM buffer?

Looking at this bit of code in particular:

"As interrupts can appear anywhere in the code so you should safeguard
those. If an interrupt routine reads from a peripheral the routine
should start with a memory read barrier. If an interrupt routine writes
to a peripheral the routine should end with a memory write barrier."

So it seems like the IRQ handler at least wants:

diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
index 604beb7..7e528f6 100644
--- a/drivers/mailbox/bcm2835-mailbox.c
+++ b/drivers/mailbox/bcm2835-mailbox.c
@@ -88,6 +88,13 @@ static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id)
 	struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id;
 	struct device *dev = mbox->dev;
 
+	/*
+	 * BCM2835-ARM-Peripherals.pdf says "If an interrupt routine
+	 * reads from a peripheral the routine should start with a
+	 * memory read barrier."
+	 */
+	rmb();
+
 	while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
 		u32 msg = readl(mbox->regs + MAIL0_RD);
 		unsigned int chan = MBOX_CHAN(msg);
@@ -100,7 +107,6 @@ static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id)
 		mbox_chan_received_data(mbox->channel[chan].link,
 			(void *) MBOX_DATA28(msg));
 	}
-	rmb(); /* Finished last mailbox read. */
 	return IRQ_HANDLED;
 }
 
-- 

> If any barrier is needed, shouldn't it be between the mailbox read and
> the processing of that message (which could at least in some cases read
> an SDRAM buffer). So, the producer would do roughly:
>
> p1) Fill in DRAM buffer
> p2) Write memory barrier so the MBOX write happens after the above
> p3) Send mbox message to tell the consumer to process the buffer
>
> ... and the consumer:
>
> c1) Read MBOX register to know which DRAM buffer to handle
> c2) rmb() to make sure we read from the DRAM buffer after the MBOX read
> c3) Read the DRAM buffer
>
> Even then, since (c3) is data-dependent on (c1), I don't think the rmb()
> in (c2) there actually does anything useful.

I'm not sure if this is already covered by "The GPU has special logic to
cope with data arriving out-of-order", but I think I agree we should
probably do it for safety.

>> +static int bcm2835_send_data(struct mbox_chan *link, void *data)
>> +{
>> +	struct bcm2835_channel *chan = to_channel(link);
>> +	struct bcm2835_mbox *mbox = chan->mbox;
>> +	int ret = 0;
>> +
>> +	if (!chan->started)
>> +		return -ENODEV;
>> +	spin_lock(&mbox->lock);
>
> Is it safe to read chan->started without the channel lock held?

The channel's lock is held by our caller (msg_submit() of mailbox.c).

>> +	if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) {
>> +		rmb(); /* Finished last mailbox read. */
>
> This also doesn't seem useful?

This (and the next one) seems to fall under:

"You should place:
• A memory write barrier before the first write to a peripheral.
• A memory read barrier after the last read of a peripheral.

It is not required to put a memory barrier instruction after each read
or write access. Only at those places in the code where it is possible
that a peripheral read or write may be followed by a read or write of a
different peripheral. This is normally at the entry and exit points of
the peripheral service code."

>> +static int bcm2835_startup(struct mbox_chan *link)
>> +{
>> +	struct bcm2835_channel *chan = to_channel(link);
>> +
>> +	chan->started = true;
>> +	return 0;
>> +}
>> +
>> +static void bcm2835_shutdown(struct mbox_chan *link)
>> +{
>> +	struct bcm2835_channel *chan = to_channel(link);
>> +
>> +	chan->started = false;
>> +}
>
> Don't we need to hold chan->lock when adjusting chan->started? Or is
> start/stop intended to be asynchronous to any operations currently in
> progress on the channel?

Only one client can be on a channel at a time, which is controlled by
con_mutex at channel request time, and these hooks are when the client
appears/disappears.

The started check in the irq handler is necessary so that we drop any
stray mbox messages instead of NULL dereffing in
mbox_chan_received_data().  I think the check in bcm2846_send_data()
could just be dropped (we know we have a client if a client is trying to
send a message).  I haven't followed quite what bcm2835_last_tx_done()
is doing.

>> +static bool bcm2835_last_tx_done(struct mbox_chan *link)
>> +{
>> +	struct bcm2835_channel *chan = to_channel(link);
>> +	struct bcm2835_mbox *mbox = chan->mbox;
>> +	bool ret;
>> +
>> +	if (!chan->started)
>> +		return false;
>> +	spin_lock(&mbox->lock);
>> +	ret = !(readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL);
>> +	rmb(); /* Finished last mailbox read. */
>
> That barrier doesn't seem useful?

Same barrier comment.

> What are the semantics of "tx done"? This seems to be testing that the
> TX mailbox isn't completely full, which is more about whether the
> consumer side is backed up rather than whether our producer-side TX
> operations are done.

Take a look at poll_txdone() -- it does look like we're doing the right
thing here, just that the method would be better named as
"ready_to_send" or something.

>> +static int request_mailbox_irq(struct bcm2835_mbox *mbox)
>
>> +	if (irq <= 0) {
>> +		dev_err(dev, "Can't get IRQ number for mailbox\n");
>> +		return -ENODEV;
>> +	}
>
> I expect devm_request_irq() checkes that condition.

Chasing things down, it looks like you'd get a silent error, but then
we've already got a whine if devm_request_irq fails anyway.

>> +	ret = devm_request_irq(dev, irq, bcm2835_mbox_irq, 0, dev_name(dev),
>> +		mbox);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to register a mailbox IRQ handler\n");
>
> Printing ret might be useful to know why.

Yeah.

> Are you sure devm_request_irq() is appropriate? The IRQ handler will be
> unregistered *after* bcm2835_mbox_remove() is called, and I think
> without any guarantee re: the order that other devm_ allocations are
> cleaned up. If bcm2835_mbox_remove() can't guarantee that no IRQ will
> fire after it exits, then bcm2835_mbox_irq() might just get called after
> some allocations are torn down, thus causing the IRQ handler to touch
> free'd memory.

It looks like we should

writel(0, mbox->regs + MAIL0_CNF);

in the unload.

> Both request_mailbox_iomem and request_mailbox_irq are small enough
> they're typically written inline into the main probe() function.

Sounds good.

>> +static int bcm2835_mbox_probe(struct platform_device *pdev)
>
>> +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
>> +	if (mbox == NULL) {
>> +		dev_err(dev, "Failed to allocate mailbox memory\n");
>
> devm_kzalloc() already prints an error, so no need to add another here,
> even if it does nicely document the fact that you remembered error
> messages:-)

Wait, it does?  I couldn't find where it would, when I was looking into
the checkpatch warnings.

>> +	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 = MBOX_CHAN_COUNT;
>> +	mbox->controller.chans = devm_kzalloc(dev,
>> +		sizeof(struct mbox_chan) * MBOX_CHAN_COUNT,
>> +		GFP_KERNEL);
>
> It'd be common to say "sizeof(*mbox->controller.chans) so the type can't
> mismatch what's being assigned to.

Agreed.

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

[PATCH 02/10] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

Stephen Warren <swarren@wwwdotorg.org> writes:

> On 03/02/2015 01:54 PM, Eric Anholt 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.
>
>> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
>
>> +/* 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)
>
> That implies there are more mailboxes. I wonder if we should
> parameterize which to use via some DT properties? I guess we can defer
> that though; we can default to the current values and add properties
> later if we want to use something else.

Yeah, until there's something to talk to in another mailbox, this seems
fine.

>> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id)
>> +{
>> +	struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id;
>> +	struct device *dev = mbox->dev;
>> +
>> +	while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
>> +		u32 msg = readl(mbox->regs + MAIL0_RD);
>> +		unsigned int chan = MBOX_CHAN(msg);
>> +
>> +		if (!mbox->channel[chan].started) {
>> +			dev_err(dev, "Reply on stopped channel %d\n", chan);
>> +			continue;
>> +		}
>> +		dev_dbg(dev, "Reply 0x%08X\n", msg);
>> +		mbox_chan_received_data(mbox->channel[chan].link,
>> +			(void *) MBOX_DATA28(msg));
>> +	}
>> +	rmb(); /* Finished last mailbox read. */
>
> I know the PDF mentioned in the comment earlier in the patch says to put
> in barriers between accesses to different peripherals, which this seems
> compliant with, but I don't see quite what this barrier achieves. I
> think the PDF is talking generalities, not imposing a rule that must be
> blindly followed. Besides, if there's a context-switch you can't
> actually implement the rules the PDF suggests. What read operation is
> this barrier attempting to ensure happens after reading all mailbox
> messages and any associated DRAM buffer?

Looking at this bit of code in particular:

"As interrupts can appear anywhere in the code so you should safeguard
those. If an interrupt routine reads from a peripheral the routine
should start with a memory read barrier. If an interrupt routine writes
to a peripheral the routine should end with a memory write barrier."

So it seems like the IRQ handler at least wants:

diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
index 604beb7..7e528f6 100644
--- a/drivers/mailbox/bcm2835-mailbox.c
+++ b/drivers/mailbox/bcm2835-mailbox.c
@@ -88,6 +88,13 @@ static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id)
 	struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id;
 	struct device *dev = mbox->dev;
 
+	/*
+	 * BCM2835-ARM-Peripherals.pdf says "If an interrupt routine
+	 * reads from a peripheral the routine should start with a
+	 * memory read barrier."
+	 */
+	rmb();
+
 	while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
 		u32 msg = readl(mbox->regs + MAIL0_RD);
 		unsigned int chan = MBOX_CHAN(msg);
@@ -100,7 +107,6 @@ static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id)
 		mbox_chan_received_data(mbox->channel[chan].link,
 			(void *) MBOX_DATA28(msg));
 	}
-	rmb(); /* Finished last mailbox read. */
 	return IRQ_HANDLED;
 }
 
-- 

> If any barrier is needed, shouldn't it be between the mailbox read and
> the processing of that message (which could at least in some cases read
> an SDRAM buffer). So, the producer would do roughly:
>
> p1) Fill in DRAM buffer
> p2) Write memory barrier so the MBOX write happens after the above
> p3) Send mbox message to tell the consumer to process the buffer
>
> ... and the consumer:
>
> c1) Read MBOX register to know which DRAM buffer to handle
> c2) rmb() to make sure we read from the DRAM buffer after the MBOX read
> c3) Read the DRAM buffer
>
> Even then, since (c3) is data-dependent on (c1), I don't think the rmb()
> in (c2) there actually does anything useful.

I'm not sure if this is already covered by "The GPU has special logic to
cope with data arriving out-of-order", but I think I agree we should
probably do it for safety.

>> +static int bcm2835_send_data(struct mbox_chan *link, void *data)
>> +{
>> +	struct bcm2835_channel *chan = to_channel(link);
>> +	struct bcm2835_mbox *mbox = chan->mbox;
>> +	int ret = 0;
>> +
>> +	if (!chan->started)
>> +		return -ENODEV;
>> +	spin_lock(&mbox->lock);
>
> Is it safe to read chan->started without the channel lock held?

The channel's lock is held by our caller (msg_submit() of mailbox.c).

>> +	if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) {
>> +		rmb(); /* Finished last mailbox read. */
>
> This also doesn't seem useful?

This (and the next one) seems to fall under:

"You should place:
? A memory write barrier before the first write to a peripheral.
? A memory read barrier after the last read of a peripheral.

It is not required to put a memory barrier instruction after each read
or write access. Only at those places in the code where it is possible
that a peripheral read or write may be followed by a read or write of a
different peripheral. This is normally at the entry and exit points of
the peripheral service code."

>> +static int bcm2835_startup(struct mbox_chan *link)
>> +{
>> +	struct bcm2835_channel *chan = to_channel(link);
>> +
>> +	chan->started = true;
>> +	return 0;
>> +}
>> +
>> +static void bcm2835_shutdown(struct mbox_chan *link)
>> +{
>> +	struct bcm2835_channel *chan = to_channel(link);
>> +
>> +	chan->started = false;
>> +}
>
> Don't we need to hold chan->lock when adjusting chan->started? Or is
> start/stop intended to be asynchronous to any operations currently in
> progress on the channel?

Only one client can be on a channel at a time, which is controlled by
con_mutex at channel request time, and these hooks are when the client
appears/disappears.

The started check in the irq handler is necessary so that we drop any
stray mbox messages instead of NULL dereffing in
mbox_chan_received_data().  I think the check in bcm2846_send_data()
could just be dropped (we know we have a client if a client is trying to
send a message).  I haven't followed quite what bcm2835_last_tx_done()
is doing.

>> +static bool bcm2835_last_tx_done(struct mbox_chan *link)
>> +{
>> +	struct bcm2835_channel *chan = to_channel(link);
>> +	struct bcm2835_mbox *mbox = chan->mbox;
>> +	bool ret;
>> +
>> +	if (!chan->started)
>> +		return false;
>> +	spin_lock(&mbox->lock);
>> +	ret = !(readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL);
>> +	rmb(); /* Finished last mailbox read. */
>
> That barrier doesn't seem useful?

Same barrier comment.

> What are the semantics of "tx done"? This seems to be testing that the
> TX mailbox isn't completely full, which is more about whether the
> consumer side is backed up rather than whether our producer-side TX
> operations are done.

Take a look at poll_txdone() -- it does look like we're doing the right
thing here, just that the method would be better named as
"ready_to_send" or something.

>> +static int request_mailbox_irq(struct bcm2835_mbox *mbox)
>
>> +	if (irq <= 0) {
>> +		dev_err(dev, "Can't get IRQ number for mailbox\n");
>> +		return -ENODEV;
>> +	}
>
> I expect devm_request_irq() checkes that condition.

Chasing things down, it looks like you'd get a silent error, but then
we've already got a whine if devm_request_irq fails anyway.

>> +	ret = devm_request_irq(dev, irq, bcm2835_mbox_irq, 0, dev_name(dev),
>> +		mbox);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to register a mailbox IRQ handler\n");
>
> Printing ret might be useful to know why.

Yeah.

> Are you sure devm_request_irq() is appropriate? The IRQ handler will be
> unregistered *after* bcm2835_mbox_remove() is called, and I think
> without any guarantee re: the order that other devm_ allocations are
> cleaned up. If bcm2835_mbox_remove() can't guarantee that no IRQ will
> fire after it exits, then bcm2835_mbox_irq() might just get called after
> some allocations are torn down, thus causing the IRQ handler to touch
> free'd memory.

It looks like we should

writel(0, mbox->regs + MAIL0_CNF);

in the unload.

> Both request_mailbox_iomem and request_mailbox_irq are small enough
> they're typically written inline into the main probe() function.

Sounds good.

>> +static int bcm2835_mbox_probe(struct platform_device *pdev)
>
>> +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
>> +	if (mbox == NULL) {
>> +		dev_err(dev, "Failed to allocate mailbox memory\n");
>
> devm_kzalloc() already prints an error, so no need to add another here,
> even if it does nicely document the fact that you remembered error
> messages:-)

Wait, it does?  I couldn't find where it would, when I was looking into
the checkpatch warnings.

>> +	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 = MBOX_CHAN_COUNT;
>> +	mbox->controller.chans = devm_kzalloc(dev,
>> +		sizeof(struct mbox_chan) * MBOX_CHAN_COUNT,
>> +		GFP_KERNEL);
>
> It'd be common to say "sizeof(*mbox->controller.chans) so the type can't
> mismatch what's being assigned to.

Agreed.
-------------- 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/20150304/55955f1c/attachment.sig>

Re: [PATCH 02/10] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/04/2015 11:20 AM, Eric Anholt wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:
> 
>> On 03/02/2015 01:54 PM, Eric Anholt wrote:
>>> From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
>>> 
>>> 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.
>> 
>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>> b/drivers/mailbox/bcm2835-mailbox.c

>>> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) +{ 
>>> +	struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; +
>>> struct device *dev = mbox->dev; + +	while (!(readl(mbox->regs +
>>> MAIL0_STA) & ARM_MS_EMPTY)) { +		u32 msg = readl(mbox->regs +
>>> MAIL0_RD); +		unsigned int chan = MBOX_CHAN(msg); + +		if
>>> (!mbox->channel[chan].started) { +			dev_err(dev, "Reply on
>>> stopped channel %d\n", chan); +			continue; +		} +
>>> dev_dbg(dev, "Reply 0x%08X\n", msg); +
>>> mbox_chan_received_data(mbox->channel[chan].link, +			(void *)
>>> MBOX_DATA28(msg)); +	} +	rmb(); /* Finished last mailbox read.
>>> */
>> 
>> I know the PDF mentioned in the comment earlier in the patch says
>> to put in barriers between accesses to different peripherals,
>> which this seems compliant with, but I don't see quite what this
>> barrier achieves. I think the PDF is talking generalities, not
>> imposing a rule that must be blindly followed. Besides, if
>> there's a context-switch you can't actually implement the rules
>> the PDF suggests. What read operation is this barrier attempting
>> to ensure happens after reading all mailbox messages and any
>> associated DRAM buffer?
> 
> Looking at this bit of code in particular:
> 
> "As interrupts can appear anywhere in the code so you should
> safeguard those. If an interrupt routine reads from a peripheral
> the routine should start with a memory read barrier. If an
> interrupt routine writes to a peripheral the routine should end
> with a memory write barrier."

I can see that doing that would be safe, but I don't think following
those rules is actually necessary in the general case. Following those
rules would cause lots of unnecessary barriers in code.

Barriers shouldn't be used arbitrarily at the start/end of ISRs, but
rather at specific chosen locations in the code that actually need to
enforce some kind of memory ordering.

For example, if the CPU writes a buffer to RAM, then programs a DMA
peripheral to read from that RAM buffer, you'd need to make sure the
RAM writes were visible to the DMA controller before kicking it off.
That's often achieved by wmb() between the last write to the memory
buffer (and perhaps some cache management) and the write to the DMA
controller to kick off the DMA read.

Not all ISRs will have any kind of interaction between DMA peripherals
and peripheral registers. For example, an I2C driver that only gets
data into/out-of the I2C controller HW via CPU PIO register
reads/writes rather than via DMA to/from RAM likely wouldn't ever need
any barriers.

> So it seems like the IRQ handler at least wants:
> 
> diff --git a/drivers/mailbox/bcm2835-mailbox.c
> b/drivers/mailbox/bcm2835-mailbox.c index 604beb7..7e528f6 100644 
> --- a/drivers/mailbox/bcm2835-mailbox.c +++
> b/drivers/mailbox/bcm2835-mailbox.c @@ -88,6 +88,13 @@ static
> irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) struct
> bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; struct device
> *dev = mbox->dev;
> 
> +	/* +	 * BCM2835-ARM-Peripherals.pdf says "If an interrupt
> routine +	 * reads from a peripheral the routine should start with
> a +	 * memory read barrier." +	 */ +	rmb(); + while
> (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) { u32 msg =
> readl(mbox->regs + MAIL0_RD); unsigned int chan = MBOX_CHAN(msg); 
> @@ -100,7 +107,6 @@ static irqreturn_t bcm2835_mbox_irq(int irq,
> void *dev_id) mbox_chan_received_data(mbox->channel[chan].link, 
> (void *) MBOX_DATA28(msg)); } -	rmb(); /* Finished last mailbox
> read. */ return IRQ_HANDLED; }

In this case, I don't think neither the original barrier is needed,
nor the extra one you added.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/10] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/04/2015 11:20 AM, Eric Anholt wrote:
> Stephen Warren <swarren@wwwdotorg.org> writes:
> 
>> On 03/02/2015 01:54 PM, Eric Anholt 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.
>> 
>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>> b/drivers/mailbox/bcm2835-mailbox.c

>>> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) +{ 
>>> +	struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; +
>>> struct device *dev = mbox->dev; + +	while (!(readl(mbox->regs +
>>> MAIL0_STA) & ARM_MS_EMPTY)) { +		u32 msg = readl(mbox->regs +
>>> MAIL0_RD); +		unsigned int chan = MBOX_CHAN(msg); + +		if
>>> (!mbox->channel[chan].started) { +			dev_err(dev, "Reply on
>>> stopped channel %d\n", chan); +			continue; +		} +
>>> dev_dbg(dev, "Reply 0x%08X\n", msg); +
>>> mbox_chan_received_data(mbox->channel[chan].link, +			(void *)
>>> MBOX_DATA28(msg)); +	} +	rmb(); /* Finished last mailbox read.
>>> */
>> 
>> I know the PDF mentioned in the comment earlier in the patch says
>> to put in barriers between accesses to different peripherals,
>> which this seems compliant with, but I don't see quite what this
>> barrier achieves. I think the PDF is talking generalities, not
>> imposing a rule that must be blindly followed. Besides, if
>> there's a context-switch you can't actually implement the rules
>> the PDF suggests. What read operation is this barrier attempting
>> to ensure happens after reading all mailbox messages and any
>> associated DRAM buffer?
> 
> Looking at this bit of code in particular:
> 
> "As interrupts can appear anywhere in the code so you should
> safeguard those. If an interrupt routine reads from a peripheral
> the routine should start with a memory read barrier. If an
> interrupt routine writes to a peripheral the routine should end
> with a memory write barrier."

I can see that doing that would be safe, but I don't think following
those rules is actually necessary in the general case. Following those
rules would cause lots of unnecessary barriers in code.

Barriers shouldn't be used arbitrarily at the start/end of ISRs, but
rather at specific chosen locations in the code that actually need to
enforce some kind of memory ordering.

For example, if the CPU writes a buffer to RAM, then programs a DMA
peripheral to read from that RAM buffer, you'd need to make sure the
RAM writes were visible to the DMA controller before kicking it off.
That's often achieved by wmb() between the last write to the memory
buffer (and perhaps some cache management) and the write to the DMA
controller to kick off the DMA read.

Not all ISRs will have any kind of interaction between DMA peripherals
and peripheral registers. For example, an I2C driver that only gets
data into/out-of the I2C controller HW via CPU PIO register
reads/writes rather than via DMA to/from RAM likely wouldn't ever need
any barriers.

> So it seems like the IRQ handler at least wants:
> 
> diff --git a/drivers/mailbox/bcm2835-mailbox.c
> b/drivers/mailbox/bcm2835-mailbox.c index 604beb7..7e528f6 100644 
> --- a/drivers/mailbox/bcm2835-mailbox.c +++
> b/drivers/mailbox/bcm2835-mailbox.c @@ -88,6 +88,13 @@ static
> irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) struct
> bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; struct device
> *dev = mbox->dev;
> 
> +	/* +	 * BCM2835-ARM-Peripherals.pdf says "If an interrupt
> routine +	 * reads from a peripheral the routine should start with
> a +	 * memory read barrier." +	 */ +	rmb(); + while
> (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) { u32 msg =
> readl(mbox->regs + MAIL0_RD); unsigned int chan = MBOX_CHAN(msg); 
> @@ -100,7 +107,6 @@ static irqreturn_t bcm2835_mbox_irq(int irq,
> void *dev_id) mbox_chan_received_data(mbox->channel[chan].link, 
> (void *) MBOX_DATA28(msg)); } -	rmb(); /* Finished last mailbox
> read. */ return IRQ_HANDLED; }

In this case, I don't think neither the original barrier is needed,
nor the extra one you added.

Re: [PATCH 02/10] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

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

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:

> On 03/04/2015 11:20 AM, Eric Anholt wrote:
>> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:
>> 
>>> On 03/02/2015 01:54 PM, Eric Anholt wrote:
>>>> From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
>>>> 
>>>> 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.
>>> 
>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>> b/drivers/mailbox/bcm2835-mailbox.c
>
>>>> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) +{ 
>>>> +	struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; +
>>>> struct device *dev = mbox->dev; + +	while (!(readl(mbox->regs +
>>>> MAIL0_STA) & ARM_MS_EMPTY)) { +		u32 msg = readl(mbox->regs +
>>>> MAIL0_RD); +		unsigned int chan = MBOX_CHAN(msg); + +		if
>>>> (!mbox->channel[chan].started) { +			dev_err(dev, "Reply on
>>>> stopped channel %d\n", chan); +			continue; +		} +
>>>> dev_dbg(dev, "Reply 0x%08X\n", msg); +
>>>> mbox_chan_received_data(mbox->channel[chan].link, +			(void *)
>>>> MBOX_DATA28(msg)); +	} +	rmb(); /* Finished last mailbox read.
>>>> */
>>> 
>>> I know the PDF mentioned in the comment earlier in the patch says
>>> to put in barriers between accesses to different peripherals,
>>> which this seems compliant with, but I don't see quite what this
>>> barrier achieves. I think the PDF is talking generalities, not
>>> imposing a rule that must be blindly followed. Besides, if
>>> there's a context-switch you can't actually implement the rules
>>> the PDF suggests. What read operation is this barrier attempting
>>> to ensure happens after reading all mailbox messages and any
>>> associated DRAM buffer?
>> 
>> Looking at this bit of code in particular:
>> 
>> "As interrupts can appear anywhere in the code so you should
>> safeguard those. If an interrupt routine reads from a peripheral
>> the routine should start with a memory read barrier. If an
>> interrupt routine writes to a peripheral the routine should end
>> with a memory write barrier."
>
> I can see that doing that would be safe, but I don't think following
> those rules is actually necessary in the general case. Following those
> rules would cause lots of unnecessary barriers in code.
>
> Barriers shouldn't be used arbitrarily at the start/end of ISRs, but
> rather at specific chosen locations in the code that actually need to
> enforce some kind of memory ordering.

According to the architecture docs, if you don't RMB at the start of
your ISR, then the following timeline could get the wrong values:

some other device driver               our isr
------------------------               -------
rmb()
read from device
                                       read from device
                                       examine value read
                                       exit isr
examine value raed.

The ISR could get the device driver's value.  This is made explicit in
footnote 2 on page 7.

>> So it seems like the IRQ handler at least wants:
>> 
>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>> b/drivers/mailbox/bcm2835-mailbox.c index 604beb7..7e528f6 100644 
>> --- a/drivers/mailbox/bcm2835-mailbox.c +++
>> b/drivers/mailbox/bcm2835-mailbox.c @@ -88,6 +88,13 @@ static
>> irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) struct
>> bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; struct device
>> *dev = mbox->dev;
>> 
>> +	/* +	 * BCM2835-ARM-Peripherals.pdf says "If an interrupt
>> routine +	 * reads from a peripheral the routine should start with
>> a +	 * memory read barrier." +	 */ +	rmb(); + while
>> (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) { u32 msg =
>> readl(mbox->regs + MAIL0_RD); unsigned int chan = MBOX_CHAN(msg); 
>> @@ -100,7 +107,6 @@ static irqreturn_t bcm2835_mbox_irq(int irq,
>> void *dev_id) mbox_chan_received_data(mbox->channel[chan].link, 
>> (void *) MBOX_DATA28(msg)); } -	rmb(); /* Finished last mailbox
>> read. */ return IRQ_HANDLED; }
>
> In this case, I don't think neither the original barrier is needed,
> nor the extra one you added.

Your formatting got completely destroyed here.

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

[PATCH 02/10] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

Stephen Warren <swarren@wwwdotorg.org> writes:

> On 03/04/2015 11:20 AM, Eric Anholt wrote:
>> Stephen Warren <swarren@wwwdotorg.org> writes:
>> 
>>> On 03/02/2015 01:54 PM, Eric Anholt 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.
>>> 
>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>> b/drivers/mailbox/bcm2835-mailbox.c
>
>>>> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) +{ 
>>>> +	struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; +
>>>> struct device *dev = mbox->dev; + +	while (!(readl(mbox->regs +
>>>> MAIL0_STA) & ARM_MS_EMPTY)) { +		u32 msg = readl(mbox->regs +
>>>> MAIL0_RD); +		unsigned int chan = MBOX_CHAN(msg); + +		if
>>>> (!mbox->channel[chan].started) { +			dev_err(dev, "Reply on
>>>> stopped channel %d\n", chan); +			continue; +		} +
>>>> dev_dbg(dev, "Reply 0x%08X\n", msg); +
>>>> mbox_chan_received_data(mbox->channel[chan].link, +			(void *)
>>>> MBOX_DATA28(msg)); +	} +	rmb(); /* Finished last mailbox read.
>>>> */
>>> 
>>> I know the PDF mentioned in the comment earlier in the patch says
>>> to put in barriers between accesses to different peripherals,
>>> which this seems compliant with, but I don't see quite what this
>>> barrier achieves. I think the PDF is talking generalities, not
>>> imposing a rule that must be blindly followed. Besides, if
>>> there's a context-switch you can't actually implement the rules
>>> the PDF suggests. What read operation is this barrier attempting
>>> to ensure happens after reading all mailbox messages and any
>>> associated DRAM buffer?
>> 
>> Looking at this bit of code in particular:
>> 
>> "As interrupts can appear anywhere in the code so you should
>> safeguard those. If an interrupt routine reads from a peripheral
>> the routine should start with a memory read barrier. If an
>> interrupt routine writes to a peripheral the routine should end
>> with a memory write barrier."
>
> I can see that doing that would be safe, but I don't think following
> those rules is actually necessary in the general case. Following those
> rules would cause lots of unnecessary barriers in code.
>
> Barriers shouldn't be used arbitrarily at the start/end of ISRs, but
> rather at specific chosen locations in the code that actually need to
> enforce some kind of memory ordering.

According to the architecture docs, if you don't RMB at the start of
your ISR, then the following timeline could get the wrong values:

some other device driver               our isr
------------------------               -------
rmb()
read from device
                                       read from device
                                       examine value read
                                       exit isr
examine value raed.

The ISR could get the device driver's value.  This is made explicit in
footnote 2 on page 7.

>> So it seems like the IRQ handler at least wants:
>> 
>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>> b/drivers/mailbox/bcm2835-mailbox.c index 604beb7..7e528f6 100644 
>> --- a/drivers/mailbox/bcm2835-mailbox.c +++
>> b/drivers/mailbox/bcm2835-mailbox.c @@ -88,6 +88,13 @@ static
>> irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) struct
>> bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; struct device
>> *dev = mbox->dev;
>> 
>> +	/* +	 * BCM2835-ARM-Peripherals.pdf says "If an interrupt
>> routine +	 * reads from a peripheral the routine should start with
>> a +	 * memory read barrier." +	 */ +	rmb(); + while
>> (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) { u32 msg =
>> readl(mbox->regs + MAIL0_RD); unsigned int chan = MBOX_CHAN(msg); 
>> @@ -100,7 +107,6 @@ static irqreturn_t bcm2835_mbox_irq(int irq,
>> void *dev_id) mbox_chan_received_data(mbox->channel[chan].link, 
>> (void *) MBOX_DATA28(msg)); } -	rmb(); /* Finished last mailbox
>> read. */ return IRQ_HANDLED; }
>
> In this case, I don't think neither the original barrier is needed,
> nor the extra one you added.

Your formatting got completely destroyed here.
-------------- 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/20150306/6a66dddf/attachment.sig>

Re: [PATCH 02/10] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/06/2015 01:00 PM, Eric Anholt wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:
>
>> On 03/04/2015 11:20 AM, Eric Anholt wrote:
>>> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:
>>>
>>>> On 03/02/2015 01:54 PM, Eric Anholt wrote:
>>>>> From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
>>>>>
>>>>> 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.
>>>>
>>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>>> b/drivers/mailbox/bcm2835-mailbox.c
>>
>>>>> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) +{
>>>>> +	struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; +
>>>>> struct device *dev = mbox->dev; + +	while (!(readl(mbox->regs +
>>>>> MAIL0_STA) & ARM_MS_EMPTY)) { +		u32 msg = readl(mbox->regs +
>>>>> MAIL0_RD); +		unsigned int chan = MBOX_CHAN(msg); + +		if
>>>>> (!mbox->channel[chan].started) { +			dev_err(dev, "Reply on
>>>>> stopped channel %d\n", chan); +			continue; +		} +
>>>>> dev_dbg(dev, "Reply 0x%08X\n", msg); +
>>>>> mbox_chan_received_data(mbox->channel[chan].link, +			(void *)
>>>>> MBOX_DATA28(msg)); +	} +	rmb(); /* Finished last mailbox read.
>>>>> */
>>>>
>>>> I know the PDF mentioned in the comment earlier in the patch says
>>>> to put in barriers between accesses to different peripherals,
>>>> which this seems compliant with, but I don't see quite what this
>>>> barrier achieves. I think the PDF is talking generalities, not
>>>> imposing a rule that must be blindly followed. Besides, if
>>>> there's a context-switch you can't actually implement the rules
>>>> the PDF suggests. What read operation is this barrier attempting
>>>> to ensure happens after reading all mailbox messages and any
>>>> associated DRAM buffer?
>>>
>>> Looking at this bit of code in particular:
>>>
>>> "As interrupts can appear anywhere in the code so you should
>>> safeguard those. If an interrupt routine reads from a peripheral
>>> the routine should start with a memory read barrier. If an
>>> interrupt routine writes to a peripheral the routine should end
>>> with a memory write barrier."
>>
>> I can see that doing that would be safe, but I don't think following
>> those rules is actually necessary in the general case. Following those
>> rules would cause lots of unnecessary barriers in code.
>>
>> Barriers shouldn't be used arbitrarily at the start/end of ISRs, but
>> rather at specific chosen locations in the code that actually need to
>> enforce some kind of memory ordering.
>
> According to the architecture docs, if you don't RMB at the start of
> your ISR, then the following timeline could get the wrong values:

Which architecture doc and section/... specifically?

> some other device driver               our isr
> ------------------------               -------
> rmb()
> read from device
>                                         read from device
>                                         examine value read
>                                         exit isr
> examine value raed.
>
> The ISR could get the device driver's value.  This is made explicit in
> footnote 2 on page 7.

Are you saying that the "read from device" in the right -hand "our isr" 
column could end up returning the value actually read during "read from 
device" in the left-hand "some other device driver" column, or vice-versa?

That doesn't make any sense.

Barriers are about ensuring that accesses happen (are visible or 
complete) in the desired order, not about ensuring that the results of 
two unrelated reads don't get swapped.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/10] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/06/2015 01:00 PM, Eric Anholt wrote:
> Stephen Warren <swarren@wwwdotorg.org> writes:
>
>> On 03/04/2015 11:20 AM, Eric Anholt wrote:
>>> Stephen Warren <swarren@wwwdotorg.org> writes:
>>>
>>>> On 03/02/2015 01:54 PM, Eric Anholt 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.
>>>>
>>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>>> b/drivers/mailbox/bcm2835-mailbox.c
>>
>>>>> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) +{
>>>>> +	struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; +
>>>>> struct device *dev = mbox->dev; + +	while (!(readl(mbox->regs +
>>>>> MAIL0_STA) & ARM_MS_EMPTY)) { +		u32 msg = readl(mbox->regs +
>>>>> MAIL0_RD); +		unsigned int chan = MBOX_CHAN(msg); + +		if
>>>>> (!mbox->channel[chan].started) { +			dev_err(dev, "Reply on
>>>>> stopped channel %d\n", chan); +			continue; +		} +
>>>>> dev_dbg(dev, "Reply 0x%08X\n", msg); +
>>>>> mbox_chan_received_data(mbox->channel[chan].link, +			(void *)
>>>>> MBOX_DATA28(msg)); +	} +	rmb(); /* Finished last mailbox read.
>>>>> */
>>>>
>>>> I know the PDF mentioned in the comment earlier in the patch says
>>>> to put in barriers between accesses to different peripherals,
>>>> which this seems compliant with, but I don't see quite what this
>>>> barrier achieves. I think the PDF is talking generalities, not
>>>> imposing a rule that must be blindly followed. Besides, if
>>>> there's a context-switch you can't actually implement the rules
>>>> the PDF suggests. What read operation is this barrier attempting
>>>> to ensure happens after reading all mailbox messages and any
>>>> associated DRAM buffer?
>>>
>>> Looking at this bit of code in particular:
>>>
>>> "As interrupts can appear anywhere in the code so you should
>>> safeguard those. If an interrupt routine reads from a peripheral
>>> the routine should start with a memory read barrier. If an
>>> interrupt routine writes to a peripheral the routine should end
>>> with a memory write barrier."
>>
>> I can see that doing that would be safe, but I don't think following
>> those rules is actually necessary in the general case. Following those
>> rules would cause lots of unnecessary barriers in code.
>>
>> Barriers shouldn't be used arbitrarily at the start/end of ISRs, but
>> rather at specific chosen locations in the code that actually need to
>> enforce some kind of memory ordering.
>
> According to the architecture docs, if you don't RMB at the start of
> your ISR, then the following timeline could get the wrong values:

Which architecture doc and section/... specifically?

> some other device driver               our isr
> ------------------------               -------
> rmb()
> read from device
>                                         read from device
>                                         examine value read
>                                         exit isr
> examine value raed.
>
> The ISR could get the device driver's value.  This is made explicit in
> footnote 2 on page 7.

Are you saying that the "read from device" in the right -hand "our isr" 
column could end up returning the value actually read during "read from 
device" in the left-hand "some other device driver" column, or vice-versa?

That doesn't make any sense.

Barriers are about ensuring that accesses happen (are visible or 
complete) in the desired order, not about ensuring that the results of 
two unrelated reads don't get swapped.

Re: [PATCH 02/10] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/06/2015 01:29 PM, Stephen Warren wrote:
> On 03/06/2015 01:00 PM, Eric Anholt wrote:
>> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:
>>
>>> On 03/04/2015 11:20 AM, Eric Anholt wrote:
>>>> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:
>>>>
>>>>> On 03/02/2015 01:54 PM, Eric Anholt wrote:
>>>>>> From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
>>>>>>
>>>>>> 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.
>>>>>
>>>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>>>> b/drivers/mailbox/bcm2835-mailbox.c
>>>
>>>>>> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) +{
>>>>>> +    struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; +
>>>>>> struct device *dev = mbox->dev; + +    while (!(readl(mbox->regs +
>>>>>> MAIL0_STA) & ARM_MS_EMPTY)) { +        u32 msg = readl(mbox->regs +
>>>>>> MAIL0_RD); +        unsigned int chan = MBOX_CHAN(msg); + +        if
>>>>>> (!mbox->channel[chan].started) { +            dev_err(dev, "Reply on
>>>>>> stopped channel %d\n", chan); +            continue; +        } +
>>>>>> dev_dbg(dev, "Reply 0x%08X\n", msg); +
>>>>>> mbox_chan_received_data(mbox->channel[chan].link, +
>>>>>> (void *)
>>>>>> MBOX_DATA28(msg)); +    } +    rmb(); /* Finished last mailbox read.
>>>>>> */
>>>>>
>>>>> I know the PDF mentioned in the comment earlier in the patch says
>>>>> to put in barriers between accesses to different peripherals,
>>>>> which this seems compliant with, but I don't see quite what this
>>>>> barrier achieves. I think the PDF is talking generalities, not
>>>>> imposing a rule that must be blindly followed. Besides, if
>>>>> there's a context-switch you can't actually implement the rules
>>>>> the PDF suggests. What read operation is this barrier attempting
>>>>> to ensure happens after reading all mailbox messages and any
>>>>> associated DRAM buffer?
>>>>
>>>> Looking at this bit of code in particular:
>>>>
>>>> "As interrupts can appear anywhere in the code so you should
>>>> safeguard those. If an interrupt routine reads from a peripheral
>>>> the routine should start with a memory read barrier. If an
>>>> interrupt routine writes to a peripheral the routine should end
>>>> with a memory write barrier."
>>>
>>> I can see that doing that would be safe, but I don't think following
>>> those rules is actually necessary in the general case. Following those
>>> rules would cause lots of unnecessary barriers in code.
>>>
>>> Barriers shouldn't be used arbitrarily at the start/end of ISRs, but
>>> rather at specific chosen locations in the code that actually need to
>>> enforce some kind of memory ordering.
>>
>> According to the architecture docs, if you don't RMB at the start of
>> your ISR, then the following timeline could get the wrong values:
>
> Which architecture doc and section/... specifically?

Ah, this is BCM2835-ARM-Peripherals.pdf still. I found the footnote you 
mentioned.

>> some other device driver               our isr
>> ------------------------               -------
>> rmb()
>> read from device
>>                                         read from device
>>                                         examine value read
>>                                         exit isr
>> examine value raed.
>>
>> The ISR could get the device driver's value.  This is made explicit in
>> footnote 2 on page 7.
>
> Are you saying that the "read from device" in the right -hand "our isr"
> column could end up returning the value actually read during "read from
> device" in the left-hand "some other device driver" column, or vice-versa?
>
> That doesn't make any sense.
>
> Barriers are about ensuring that accesses happen (are visible or
> complete) in the desired order, not about ensuring that the results of
> two unrelated reads don't get swapped.

Ah. I've been thinking about the typical uses of barriers to ensure 
ordering of transactions on a standards-compliant ARM CPU.

It was pointed out on IRC that perhaps the barriers are to work around a 
CPU/SoC bug. In which case, if the CPU/bus really can swap results, then 
I can see how barriers might be required and how they might solve the 
problem. If this is the real reason, it sounds truly massively scary.

I think this deserves a complete description in each driver file and in 
the commit description for any commit that introduces these barriers. 
Otherwise someone else will just want to rip them out.

I'm also concerned that we need to add barriers in many more places than 
just that start/end of ISRs. For example, what if the ISR for the 
mailbox ends up calling back into some other driver that goes and 
touches a different HW module. Now we need to put barriers either at the 
start/end of that callback function, or immediately before/after we call 
that callback function. This has potential to need barriers in a whole 
bunch of places one wouldn't expect.

I sure hope this was fixed on bcm2836, or that the ordering issue 
appears separately within each CPU core and there's no interaction 
between multiple CPU cores. Otherwise, two separate pieces of code 
running on two separate CPU cores are going to have trouble when they 
start touching different peripherals.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/10] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/06/2015 01:29 PM, Stephen Warren wrote:
> On 03/06/2015 01:00 PM, Eric Anholt wrote:
>> Stephen Warren <swarren@wwwdotorg.org> writes:
>>
>>> On 03/04/2015 11:20 AM, Eric Anholt wrote:
>>>> Stephen Warren <swarren@wwwdotorg.org> writes:
>>>>
>>>>> On 03/02/2015 01:54 PM, Eric Anholt 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.
>>>>>
>>>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>>>> b/drivers/mailbox/bcm2835-mailbox.c
>>>
>>>>>> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) +{
>>>>>> +    struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; +
>>>>>> struct device *dev = mbox->dev; + +    while (!(readl(mbox->regs +
>>>>>> MAIL0_STA) & ARM_MS_EMPTY)) { +        u32 msg = readl(mbox->regs +
>>>>>> MAIL0_RD); +        unsigned int chan = MBOX_CHAN(msg); + +        if
>>>>>> (!mbox->channel[chan].started) { +            dev_err(dev, "Reply on
>>>>>> stopped channel %d\n", chan); +            continue; +        } +
>>>>>> dev_dbg(dev, "Reply 0x%08X\n", msg); +
>>>>>> mbox_chan_received_data(mbox->channel[chan].link, +
>>>>>> (void *)
>>>>>> MBOX_DATA28(msg)); +    } +    rmb(); /* Finished last mailbox read.
>>>>>> */
>>>>>
>>>>> I know the PDF mentioned in the comment earlier in the patch says
>>>>> to put in barriers between accesses to different peripherals,
>>>>> which this seems compliant with, but I don't see quite what this
>>>>> barrier achieves. I think the PDF is talking generalities, not
>>>>> imposing a rule that must be blindly followed. Besides, if
>>>>> there's a context-switch you can't actually implement the rules
>>>>> the PDF suggests. What read operation is this barrier attempting
>>>>> to ensure happens after reading all mailbox messages and any
>>>>> associated DRAM buffer?
>>>>
>>>> Looking at this bit of code in particular:
>>>>
>>>> "As interrupts can appear anywhere in the code so you should
>>>> safeguard those. If an interrupt routine reads from a peripheral
>>>> the routine should start with a memory read barrier. If an
>>>> interrupt routine writes to a peripheral the routine should end
>>>> with a memory write barrier."
>>>
>>> I can see that doing that would be safe, but I don't think following
>>> those rules is actually necessary in the general case. Following those
>>> rules would cause lots of unnecessary barriers in code.
>>>
>>> Barriers shouldn't be used arbitrarily at the start/end of ISRs, but
>>> rather at specific chosen locations in the code that actually need to
>>> enforce some kind of memory ordering.
>>
>> According to the architecture docs, if you don't RMB at the start of
>> your ISR, then the following timeline could get the wrong values:
>
> Which architecture doc and section/... specifically?

Ah, this is BCM2835-ARM-Peripherals.pdf still. I found the footnote you 
mentioned.

>> some other device driver               our isr
>> ------------------------               -------
>> rmb()
>> read from device
>>                                         read from device
>>                                         examine value read
>>                                         exit isr
>> examine value raed.
>>
>> The ISR could get the device driver's value.  This is made explicit in
>> footnote 2 on page 7.
>
> Are you saying that the "read from device" in the right -hand "our isr"
> column could end up returning the value actually read during "read from
> device" in the left-hand "some other device driver" column, or vice-versa?
>
> That doesn't make any sense.
>
> Barriers are about ensuring that accesses happen (are visible or
> complete) in the desired order, not about ensuring that the results of
> two unrelated reads don't get swapped.

Ah. I've been thinking about the typical uses of barriers to ensure 
ordering of transactions on a standards-compliant ARM CPU.

It was pointed out on IRC that perhaps the barriers are to work around a 
CPU/SoC bug. In which case, if the CPU/bus really can swap results, then 
I can see how barriers might be required and how they might solve the 
problem. If this is the real reason, it sounds truly massively scary.

I think this deserves a complete description in each driver file and in 
the commit description for any commit that introduces these barriers. 
Otherwise someone else will just want to rip them out.

I'm also concerned that we need to add barriers in many more places than 
just that start/end of ISRs. For example, what if the ISR for the 
mailbox ends up calling back into some other driver that goes and 
touches a different HW module. Now we need to put barriers either at the 
start/end of that callback function, or immediately before/after we call 
that callback function. This has potential to need barriers in a whole 
bunch of places one wouldn't expect.

I sure hope this was fixed on bcm2836, or that the ordering issue 
appears separately within each CPU core and there's no interaction 
between multiple CPU cores. Otherwise, two separate pieces of code 
running on two separate CPU cores are going to have trouble when they 
start touching different peripherals.

Re: [PATCH 02/10] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

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

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:

> On 03/02/2015 01:54 PM, Eric Anholt wrote:
>> From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
>> 
>> 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.
>
>> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
>
>> +/* 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)
>
> That implies there are more mailboxes. I wonder if we should
> parameterize which to use via some DT properties? I guess we can defer
> that though; we can default to the current values and add properties
> later if we want to use something else.

BCM2835-ARM-Peripherals.pdf:

"Default the interrupts from doorbell 0,1 and mailbox 0 go to the ARM
this means that these resources should be written by the GPU and read by
the ARM. The opposite holds for doorbells 2, 3 and mailbox 1."

I don't see any references to more mailboxes than 0 and 1.

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

[PATCH 02/10] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

Stephen Warren <swarren@wwwdotorg.org> writes:

> On 03/02/2015 01:54 PM, Eric Anholt 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.
>
>> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
>
>> +/* 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)
>
> That implies there are more mailboxes. I wonder if we should
> parameterize which to use via some DT properties? I guess we can defer
> that though; we can default to the current values and add properties
> later if we want to use something else.

BCM2835-ARM-Peripherals.pdf:

"Default the interrupts from doorbell 0,1 and mailbox 0 go to the ARM
this means that these resources should be written by the GPU and read by
the ARM. The opposite holds for doorbells 2, 3 and mailbox 1."

I don't see any references to more mailboxes than 0 and 1.
-------------- 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/20150304/0aa1d5b5/attachment.sig>

[PATCH 01/10] dt/bindings: Add binding for BCM2835 mailbox driver

[Eric Anholt]

From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.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).

Signed-off-by: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
Signed-off-by: Craig McGeachie <slapdau-/E1597aS9LT0CCvOHzKKcA@public.gmane.org>
Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
---
 .../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..f5741a0
--- /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@7e00b800 {
+	compatible = "brcm,bcm2835-mbox";
+	reg = <0x7e00b880 0x40>;
+	interrupts = <0 1>;
+	#mbox-cells = <1>;
+};
-- 
2.1.4

Re: [PATCH 01/10] dt/bindings: Add binding for BCM2835 mailbox driver

[Lee Jones]

On Mon, 02 Mar 2015, Eric Anholt wrote:

> From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
> 
> v2: Split into a separate patch for submitting to the devicetree list.

When you say that you've split them, do you mean each DT doc, as I
don't think this is the way to go.  I'm happy to listen to other
people's opinions, but I think all of the .../mailbox/brcm,bcm2835-
files should be amalgamated.

>     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).
> 
> Signed-off-by: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
> Signed-off-by: Craig McGeachie <slapdau-/E1597aS9LT0CCvOHzKKcA@public.gmane.org>
> Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> ---
>  .../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..f5741a0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt

Rename these files to conform to the current naming convention.  In
-next we currently have 'altera-mailbox.txt' and 'omap-mailbox.txt',
so 'bcm2835-mbox.txt' seems appropriate.

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

Binding documents are much easier to read if the property names and
descriptions are seperated with tabs.

- compatible		: Should be "brcm,bcm2835-mbox"
- reg			: Specifies base physical address and size of the registers
- interrupts 		: The interrupt number
			  [See ../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

... don't you think?  Also notice the consistency of no '.'s and the
bracketing of the [See ...] statement.

> +Example:
> +
> +mailbox: mailbox@7e00b800 {
> +	compatible = "brcm,bcm2835-mbox";
> +	reg = <0x7e00b880 0x40>;
> +	interrupts = <0 1>;
> +	#mbox-cells = <1>;
> +};

It would be good to see the client examples here as well.  Please consider
pulling in brcm,bcm2835-mbox-power.txt and brcm,bcm2835-mbox-property.txt.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/10] dt/bindings: Add binding for BCM2835 mailbox driver

[Eric Anholt]

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

Lee Jones <lee-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> writes:

> On Mon, 02 Mar 2015, Eric Anholt wrote:
>
>> From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
>> 
>> v2: Split into a separate patch for submitting to the devicetree list.
>
> When you say that you've split them, do you mean each DT doc, as I
> don't think this is the way to go.  I'm happy to listen to other
> people's opinions, but I think all of the .../mailbox/brcm,bcm2835-
> files should be amalgamated.

I just meant that I split this patch out of the patch that followed
(because of review feeback and
Documentation/devicetree/bindings/submitting-patches.txt rules).

>> ---
>>  .../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..f5741a0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
>
> Rename these files to conform to the current naming convention.  In
> -next we currently have 'altera-mailbox.txt' and 'omap-mailbox.txt',
> so 'bcm2835-mbox.txt' seems appropriate.

Will do.

>> @@ -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.
>
> Binding documents are much easier to read if the property names and
> descriptions are seperated with tabs.
>
> - compatible		: Should be "brcm,bcm2835-mbox"
> - reg			: Specifies base physical address and size of the registers
> - interrupts 		: The interrupt number
> 			  [See ../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
>
> ... don't you think?  Also notice the consistency of no '.'s and the
> bracketing of the [See ...] statement.

The tree seems inconsistent on formatting of these files, but I like
your suggestion for nicer formatting so I'll do it.

>> +Example:
>> +
>> +mailbox: mailbox@7e00b800 {
>> +	compatible = "brcm,bcm2835-mbox";
>> +	reg = <0x7e00b880 0x40>;
>> +	interrupts = <0 1>;
>> +	#mbox-cells = <1>;
>> +};
>
> It would be good to see the client examples here as well.  Please consider
> pulling in brcm,bcm2835-mbox-power.txt and brcm,bcm2835-mbox-property.txt.

Oh, so have those two just smashed into this file as one set of
documentation for everything to do with mailbox on bcm2835?  That seems
good to me.  When I was adding the client drivers, the fact that the
other brcm file was named after the compatible string made me generate
new files under then new compatible strings, but the other drivers
already in the tree obviously aren't formatted that way.

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

Re: [PATCH 01/10] dt/bindings: Add binding for BCM2835 mailbox driver

[Stephen Warren]

On 03/03/2015 12:28 PM, Eric Anholt wrote:
> Lee Jones <lee-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> writes:
> 
>> On Mon, 02 Mar 2015, Eric Anholt wrote:
>> 
>>> From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
>>> 
>>> v2: Split into a separate patch for submitting to the 
>>> devicetree list.
...
>>> ---

Generally, the changelog should go below the --- since most people
don't want to see the changelog committed into the source.

>>> .../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..f5741a0 --- /dev/null +++ 
>>> b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
>>
>>
>>>
>>> 
Rename these files to conform to the current naming convention.  In
>> -next we currently have 'altera-mailbox.txt' and 
>> 'omap-mailbox.txt', so 'bcm2835-mbox.txt' seems appropriate.
> 
> Will do.

I believe all the current bcm2835 bindings use the compatible value as
the filename. I personally prefer this to picking a different "random"
name for the filenames. It means you only have to name the thing once,
and then use the same value for the compatible property and binding
document.

>>> +Example: + +mailbox: mailbox@7e00b800 { +	compatible = 
>>> "brcm,bcm2835-mbox"; +	reg = <0x7e00b880 0x40>; +	interrupts = 
>>> <0 1>; +	#mbox-cells = <1>; +};
>> 
>> It would be good to see the client examples here as well.
>> Please consider pulling in brcm,bcm2835-mbox-power.txt and 
>> brcm,bcm2835-mbox-property.txt.
> 
> Oh, so have those two just smashed into this file as one set of 
> documentation for everything to do with mailbox on bcm2835?  That 
> seems good to me.  When I was adding the client drivers, the fact 
> that the other brcm file was named after the compatible string
> made me generate new files under then new compatible strings, but
> the other drivers already in the tree obviously aren't formatted
> that way.

The HW mailbox seems like a different process to the upper-layer
protocols/message formats running over the top of it. Sure right now
the Pi has a single firmware, but do all bcm2835-based devices share
the same firmware? Is so, we'd be warranted in lumping the HW and
firmware protocol together, but I rather wonder whether e.g. the
bcm2835-based Roku uses the same firmware protocol?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/10] dt/bindings: Add binding for BCM2835 mailbox driver

[Eric Anholt]

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

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:

> On 03/03/2015 12:28 PM, Eric Anholt wrote:
>> Lee Jones <lee-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> writes:
>> 
>>> On Mon, 02 Mar 2015, Eric Anholt wrote:
>>> 
>>>> From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
>>>> 
>>>> v2: Split into a separate patch for submitting to the 
>>>> devicetree list.
> ...
>>>> ---
>
> Generally, the changelog should go below the --- since most people
> don't want to see the changelog committed into the source.

Huh.  I'm coming from drivers/gpu/drm/ (and non-kernel communities)
where people are expected to keep changelog in the commit message.  I'll
strip it out of this series.

>>>> .../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..f5741a0 --- /dev/null +++ 
>>>> b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
>>>
>>>
>>>>
>>>> 
> Rename these files to conform to the current naming convention.  In
>>> -next we currently have 'altera-mailbox.txt' and 
>>> 'omap-mailbox.txt', so 'bcm2835-mbox.txt' seems appropriate.
>> 
>> Will do.
>
> I believe all the current bcm2835 bindings use the compatible value as
> the filename. I personally prefer this to picking a different "random"
> name for the filenames. It means you only have to name the thing once,
> and then use the same value for the compatible property and binding
> document.

"git grep brcm | grep 2835" thinks that we're quite confused on whether
you put the "brcm," in the filename or not.  And then there's some that
don't follow either convention:

watchdog/brcm,bcm2835-pm-wdog.txt:	compatible = "brcm,bcm2835-pm-wdt";
pwm/pwm-bcm2835.txt:	compatible = "brcm,bcm2835-pwm";
rng/brcm,bcm2835.txt:        compatible = "brcm,bcm2835-rng";

>>>> +Example: + +mailbox: mailbox@7e00b800 { +	compatible = 
>>>> "brcm,bcm2835-mbox"; +	reg = <0x7e00b880 0x40>; +	interrupts = 
>>>> <0 1>; +	#mbox-cells = <1>; +};
>>> 
>>> It would be good to see the client examples here as well.
>>> Please consider pulling in brcm,bcm2835-mbox-power.txt and 
>>> brcm,bcm2835-mbox-property.txt.
>> 
>> Oh, so have those two just smashed into this file as one set of 
>> documentation for everything to do with mailbox on bcm2835?  That 
>> seems good to me.  When I was adding the client drivers, the fact 
>> that the other brcm file was named after the compatible string
>> made me generate new files under then new compatible strings, but
>> the other drivers already in the tree obviously aren't formatted
>> that way.
>
> The HW mailbox seems like a different process to the upper-layer
> protocols/message formats running over the top of it. Sure right now
> the Pi has a single firmware, but do all bcm2835-based devices share
> the same firmware? Is so, we'd be warranted in lumping the HW and
> firmware protocol together, but I rather wonder whether e.g. the
> bcm2835-based Roku uses the same firmware protocol?

I don't know the answer to that one, and I've only got RPi firmware
myself.

Ultimately, my hope is to get enough merged that we can basically get
off the firmware, other than the bits of clock management that are only
doable from the VPU.

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

Re: [PATCH 01/10] dt/bindings: Add binding for BCM2835 mailbox driver

[Eric Anholt]

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

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:

> On 03/03/2015 12:28 PM, Eric Anholt wrote:
>> Lee Jones <lee-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> writes:
>> 
>>> On Mon, 02 Mar 2015, Eric Anholt wrote:
>>>> +Example: + +mailbox: mailbox@7e00b800 { +	compatible = 
>>>> "brcm,bcm2835-mbox"; +	reg = <0x7e00b880 0x40>; +	interrupts = 
>>>> <0 1>; +	#mbox-cells = <1>; +};
>>> 
>>> It would be good to see the client examples here as well.
>>> Please consider pulling in brcm,bcm2835-mbox-power.txt and 
>>> brcm,bcm2835-mbox-property.txt.
>> 
>> Oh, so have those two just smashed into this file as one set of 
>> documentation for everything to do with mailbox on bcm2835?  That 
>> seems good to me.  When I was adding the client drivers, the fact 
>> that the other brcm file was named after the compatible string
>> made me generate new files under then new compatible strings, but
>> the other drivers already in the tree obviously aren't formatted
>> that way.
>
> The HW mailbox seems like a different process to the upper-layer
> protocols/message formats running over the top of it. Sure right now
> the Pi has a single firmware, but do all bcm2835-based devices share
> the same firmware? Is so, we'd be warranted in lumping the HW and
> firmware protocol together, but I rather wonder whether e.g. the
> bcm2835-based Roku uses the same firmware protocol?

I've confirmed: While their firmware would have been forked from the
same source, there's no reason to expect their shipped firmware
protocols (like my power domain or property channel drivers use) to
match RPi's.

The power-domain rework ended up not working out -- it needs the
power-domain part of device/base to support throwing -EPROBE_DEFER if
the driver hasn't probed yet, unless we're willing to just bake in the
power domain driver in static init ordering.  Device base maintainers
weren't excited about my patch for -EPROBE_DEFER, because then a new DT
would mean we start failing to probe the USB driver in an older kernel,
which whould be a regression in the case that the user had U-Boot
setting up USB for them.

Meanwhile, it looks like the thing being done in the firmware for USB is
not just a power domain, but also configuration of the USB PHY.  This
led me to wonder if I could just write native support for the PHY and
the power domains instead of asking the firmware to do it, but this is
going to take more investigation.

As a result, I'm going to shelve my power domain and property channel
work for the moment, and just submit the core mailbox support that I
feel pretty confident about for now.

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

Re: [PATCH 01/10] dt/bindings: Add binding for BCM2835 mailbox driver

[Stephen Warren]

On 03/12/2015 05:23 PM, Eric Anholt wrote:
...
> The power-domain rework ended up not working out -- it needs the 
> power-domain part of device/base to support throwing -EPROBE_DEFER
> if the driver hasn't probed yet,

That makese sense.

> unless we're willing to just bake in the power domain driver in
> static init ordering.

Indeed, that's quite unlikely to be acceptable.

> Device base maintainers weren't excited about my patch for
> -EPROBE_DEFER, because then a new DT would mean we start failing to
> probe the USB driver in an older kernel, which whould be a
> regression in the case that the user had U-Boot setting up USB for
> them.

The main ABI issue is that old DTs should work with new kernels. The
other way around is nice, but certainly not as strict a requirement. I
don't think that should block a change. Do you have a link to the
thread; I don't think I noticed it.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/10] dt/bindings: Add binding for BCM2835 mailbox driver

[Eric Anholt]

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

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:

> On 03/12/2015 05:23 PM, Eric Anholt wrote:
>> Device base maintainers weren't excited about my patch for
>> -EPROBE_DEFER, because then a new DT would mean we start failing to
>> probe the USB driver in an older kernel, which whould be a
>> regression in the case that the user had U-Boot setting up USB for
>> them.
>
> The main ABI issue is that old DTs should work with new kernels. The
> other way around is nice, but certainly not as strict a requirement. I
> don't think that should block a change. Do you have a link to the
> thread; I don't think I noticed it.

http://comments.gmane.org/gmane.linux.kernel/1906119

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

Re: [PATCH 01/10] dt/bindings: Add binding for BCM2835 mailbox driver

[Stephen Warren]

On 03/17/2015 01:10 PM, Eric Anholt wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:
> 
>> On 03/12/2015 05:23 PM, Eric Anholt wrote:
>>> Device base maintainers weren't excited about my patch for 
>>> -EPROBE_DEFER, because then a new DT would mean we start
>>> failing to probe the USB driver in an older kernel, which
>>> whould be a regression in the case that the user had U-Boot
>>> setting up USB for them.
>> 
>> The main ABI issue is that old DTs should work with new kernels.
>> The other way around is nice, but certainly not as strict a
>> requirement. I don't think that should block a change. Do you
>> have a link to the thread; I don't think I noticed it.
> 
> http://comments.gmane.org/gmane.linux.kernel/1906119

Hmm. I think that in general, the Linux kernel has already chosen
deferred probe as the ordering mechanism over fixed init levels, which
have demonstrably been unable to solve all ordering problems in the
past. As Kevin pointed out, new DT with old kernel isn't the most
important use-case. The only other objection I saw was Ulf not wanting
to introduce another "probe scenario", but I guess he changed his mind
since he outlines the steps to make deferred probe work in a later email.

I guess/hope the way forward is to either wait for PM domains to work
with deferred probe, or push that work forward (i.e. implement part
(1) mentioned in Ulf's Mar 16 email).
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/10] ARM: bcm2835: Add the mailbox to the device tree.

[Eric Anholt]

Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.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 3342cb1..6b5c3a1 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -60,6 +60,13 @@
 			reg = <0x7e104000 0x10>;
 		};
 
+		mailbox: mailbox@7e00b800 {
+			compatible = "brcm,bcm2835-mbox";
+			reg = <0x7e00b880 0x40>;
+			interrupts = <0 1>;
+			#mbox-cells = <1>;
+		};
+
 		gpio: gpio@7e200000 {
 			compatible = "brcm,bcm2835-gpio";
 			reg = <0x7e200000 0xb4>;
-- 
2.1.4

Re: [PATCH 03/10] ARM: bcm2835: Add the mailbox to the device tree.

[Lee Jones]

On Mon, 02 Mar 2015, Eric Anholt wrote:

> Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> ---
>  arch/arm/boot/dts/bcm2835.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)

Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>


> diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
> index 3342cb1..6b5c3a1 100644
> --- a/arch/arm/boot/dts/bcm2835.dtsi
> +++ b/arch/arm/boot/dts/bcm2835.dtsi
> @@ -60,6 +60,13 @@
>  			reg = <0x7e104000 0x10>;
>  		};
>  
> +		mailbox: mailbox@7e00b800 {
> +			compatible = "brcm,bcm2835-mbox";
> +			reg = <0x7e00b880 0x40>;
> +			interrupts = <0 1>;
> +			#mbox-cells = <1>;
> +		};
> +
>  		gpio: gpio@7e200000 {
>  			compatible = "brcm,bcm2835-gpio";
>  			reg = <0x7e200000 0xb4>;
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 04/10] dt/bindings: Add binding for BCM2835 mailbox power channel driver

[Eric Anholt]

I was tempted to have the mailbox power channel support just be in the
2835 mailbox driver itself, but mbox_request_channel() wants its
device to have the "mboxes" node, and that appears to be only intended
for mailbox clients, not controllers.

Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
---
 .../bindings/mailbox/brcm,bcm2835-mbox-power.txt           | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox-power.txt

diff --git a/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox-power.txt b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox-power.txt
new file mode 100644
index 0000000..83446ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox-power.txt
@@ -0,0 +1,14 @@
+Broadcom BCM2835 VideoCore mailbox power channel IPC
+
+Required properties:
+
+- compatible : Should be "brcm,bcm2835-mbox-power"
+- mboxes: Single-entry list which specifies which mailbox controller
+  and channel is the power channel.
+
+Example:
+
+mailbox-power {
+	compatible = "brcm,bcm2835-mbox-power";
+	mboxes = <&mailbox 0>
+};
-- 
2.1.4

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

Re: [PATCH 04/10] dt/bindings: Add binding for BCM2835 mailbox power channel driver

[Stephen Warren]

On 03/02/2015 01:54 PM, Eric Anholt wrote:
> I was tempted to have the mailbox power channel support just be in the
> 2835 mailbox driver itself, but mbox_request_channel() wants its
> device to have the "mboxes" node, and that appears to be only intended
> for mailbox clients, not controllers.

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

> +Broadcom BCM2835 VideoCore mailbox power channel IPC
> +
> +Required properties:
> +
> +- compatible : Should be "brcm,bcm2835-mbox-power"
> +- mboxes: Single-entry list which specifies which mailbox controller
> +  and channel is the power channel.
> +
> +Example:
> +
> +mailbox-power {
> +	compatible = "brcm,bcm2835-mbox-power";
> +	mboxes = <&mailbox 0>
> +};

I think the driver for this device should provide its services to other
drivers using some standard in-kernel API (power domains, regulators?),
which in turn would presumably have some subsystem-level DT binding.

So, I would expect this DT node to implement the server-side of that
binding, and probably have a property like "#power-domain-cells = <1>",
and the client nodes to "consume" this service via a property like
"power-domain = <&mbox_power N>" where N is the ID of the client's power
domain within the firmware protocol.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/10] ARM: bcm2835: Add the mailbox power channel driver.

[Eric Anholt]

This just enables the power to the USB controller, so that DWC2 can
initialize.

The downstream tree has an interface to this channel for tracking
enables from multiple clients, except it doesn't have any clients as
far as I can see.  For now, just make the simplest thing that gets USB
working.

Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
---
 drivers/mailbox/Makefile                |   1 +
 drivers/mailbox/bcm2835-mailbox-power.c | 127 ++++++++++++++++++++++++++++++++
 2 files changed, 128 insertions(+)
 create mode 100644 drivers/mailbox/bcm2835-mailbox-power.c

diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 7feb8da..619d815 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_PCC)		+= pcc.o
 obj-$(CONFIG_ALTERA_MBOX)	+= mailbox-altera.o
 
 obj-$(CONFIG_BCM2835_MBOX)	+= bcm2835-mailbox.o
+obj-$(CONFIG_BCM2835_MBOX)	+= bcm2835-mailbox-power.o
diff --git a/drivers/mailbox/bcm2835-mailbox-power.c b/drivers/mailbox/bcm2835-mailbox-power.c
new file mode 100644
index 0000000..f09c855
--- /dev/null
+++ b/drivers/mailbox/bcm2835-mailbox-power.c
@@ -0,0 +1,127 @@
+/*
+ *  Copyright © 2015 Broadcom
+ *
+ * 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.
+ */
+
+/*
+ * Defines a module for accessing the power channel of the
+ * BCM2835 mailbox.
+ *
+ * For now, we just turn on the USB power so that DWC2 can initialize.
+ * At a later time we may extend this into a driver that actually does
+ * runtime power management on the other channels.
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define BCM_POWER_USB (1 << 3)
+#define BCM_MBOX_DATA_SHIFT 4
+
+struct bcm_mbox_power {
+	struct device *dev;
+	struct mbox_client cl;
+	struct mbox_chan *chan;
+	struct completion c;
+	uint32_t response;
+};
+
+struct bcm_mbox_power *mbox_power;
+
+static void response_callback(struct mbox_client *cl, void *mssg)
+{
+	mbox_power->response = (uint32_t)mssg;
+	complete(&mbox_power->c);
+}
+
+/*
+ * Submits a set of concatenated tags to the VPU firmware through the
+ * mailbox power interface.
+ *
+ * The buffer header and the ending tag are added by this function and
+ * don't need to be supplied, just the actual tags for your operation.
+ * See struct bcm_mbox_power_tag_header for the per-tag structure.
+ */
+static int bcm_mbox_set_power(uint32_t power_enables)
+{
+	int ret;
+
+	reinit_completion(&mbox_power->c);
+	ret = mbox_send_message(mbox_power->chan,
+				(void *)(power_enables << BCM_MBOX_DATA_SHIFT));
+	if (ret >= 0)
+		wait_for_completion(&mbox_power->c);
+
+	return ret;
+}
+
+static int bcm2835_mbox_power_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int ret = 0;
+
+	mbox_power = devm_kzalloc(dev, sizeof(*mbox_power),
+				       GFP_KERNEL);
+	if (!mbox_power) {
+		dev_err(dev, "Failed to allocate device memory\n");
+		return -ENOMEM;
+	}
+
+	mbox_power->cl.dev = dev;
+	mbox_power->cl.rx_callback = response_callback;
+	mbox_power->cl.tx_block = true;
+
+	mbox_power->chan = mbox_request_channel(&mbox_power->cl, 0);
+	if (!mbox_power->chan) {
+		dev_err(dev, "Failed to get mbox channel\n");
+		return -ENODEV;
+	}
+
+	init_completion(&mbox_power->c);
+
+	platform_set_drvdata(pdev, mbox_power);
+	mbox_power->dev = dev;
+
+	/* Enable power to the USB phy. */
+	if (IS_ENABLED(CONFIG_USB_DWC2)) {
+		bcm_mbox_set_power(BCM_POWER_USB);
+		bcm2835_mbox_power_initialized = true;
+	}
+
+	return ret;
+}
+
+static int bcm2835_mbox_power_remove(struct platform_device *pdev)
+{
+	bcm_mbox_set_power(0);
+
+	mbox_free_channel(mbox_power->chan);
+
+	return 0;
+}
+
+static const struct of_device_id bcm2835_mbox_power_of_match[] = {
+	{ .compatible = "brcm,bcm2835-mbox-power", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm2835_mbox_power_of_match);
+
+static struct platform_driver bcm2835_mbox_power_driver = {
+	.driver = {
+		.name = "bcm2835-mbox-power",
+		.owner = THIS_MODULE,
+		.of_match_table = bcm2835_mbox_power_of_match,
+	},
+	.probe		= bcm2835_mbox_power_probe,
+	.remove		= bcm2835_mbox_power_remove,
+};
+module_platform_driver(bcm2835_mbox_power_driver);
+
+MODULE_AUTHOR("Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>");
+MODULE_DESCRIPTION("BCM2835 mailbox power channel");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

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

Re: [PATCH 05/10] ARM: bcm2835: Add the mailbox power channel driver.

[Arnd Bergmann]

On Monday 02 March 2015 12:54:39 Eric Anholt wrote:
> +/*
> + * Submits a set of concatenated tags to the VPU firmware through the
> + * mailbox power interface.
> + *
> + * The buffer header and the ending tag are added by this function and
> + * don't need to be supplied, just the actual tags for your operation.
> + * See struct bcm_mbox_power_tag_header for the per-tag structure.
> + */
> +static int bcm_mbox_set_power(uint32_t power_enables)
> +{
> +       int ret;
> +
> +       reinit_completion(&mbox_power->c);
> +       ret = mbox_send_message(mbox_power->chan,
> +                               (void *)(power_enables << BCM_MBOX_DATA_SHIFT));
> +       if (ret >= 0)
> +               wait_for_completion(&mbox_power->c);
> +
> +       return ret;
> 

Please don't abuse the pointer argument to send an integer.

Instead, pass a pointer to the data argument as intended. As this
is a recurring problem, we may want to add a different interface
to pass fixed-length inline data, maybe

mbox_send_message_u32(struct mbox_chan *chan, u32 msg);

and possibly add a length argument to the existing mbox_send_message.

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

Re: [PATCH 05/10] ARM: bcm2835: Add the mailbox power channel driver.

[Eric Anholt]

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

Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> writes:

> On Monday 02 March 2015 12:54:39 Eric Anholt wrote:
>> +/*
>> + * Submits a set of concatenated tags to the VPU firmware through the
>> + * mailbox power interface.
>> + *
>> + * The buffer header and the ending tag are added by this function and
>> + * don't need to be supplied, just the actual tags for your operation.
>> + * See struct bcm_mbox_power_tag_header for the per-tag structure.
>> + */
>> +static int bcm_mbox_set_power(uint32_t power_enables)
>> +{
>> +       int ret;
>> +
>> +       reinit_completion(&mbox_power->c);
>> +       ret = mbox_send_message(mbox_power->chan,
>> +                               (void *)(power_enables << BCM_MBOX_DATA_SHIFT));
>> +       if (ret >= 0)
>> +               wait_for_completion(&mbox_power->c);
>> +
>> +       return ret;
>> 
>
> Please don't abuse the pointer argument to send an integer.
>
> Instead, pass a pointer to the data argument as intended. As this
> is a recurring problem, we may want to add a different interface
> to pass fixed-length inline data, maybe
>
> mbox_send_message_u32(struct mbox_chan *chan, u32 msg);
>
> and possibly add a length argument to the existing mbox_send_message.

Good point.  I've adjusted this code to do this (and to use "msg"
instead of "mssg" -- is "mssg" supposed to mean something besides an
unusual abbreviation of "message"?), and since it was the same number of
lines of code, I don't think it makes sense to define a new API.  I
suspect this awkward style just arose from following the lead of the
omap driver.

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

Re: [PATCH 05/10] ARM: bcm2835: Add the mailbox power channel driver.

[Stephen Warren]

On 03/02/2015 01:54 PM, Eric Anholt wrote:
> This just enables the power to the USB controller, so that DWC2 can
> initialize.
> 
> The downstream tree has an interface to this channel for tracking
> enables from multiple clients, except it doesn't have any clients as
> far as I can see.  For now, just make the simplest thing that gets USB
> working.

>  drivers/mailbox/Makefile                |   1 +
>  drivers/mailbox/bcm2835-mailbox-power.c | 127 ++++++++++++++++++++++++++++++++

Along the lines of my comments on the previous patch, I would expect
this driver to show up within the directory for the subsystem/API that
it implements (power domains, regulators?)

> diff --git a/drivers/mailbox/bcm2835-mailbox-power.c b/drivers/mailbox/bcm2835-mailbox-power.c

> +#define BCM_POWER_USB (1 << 3)

I'd expect that to be encoded in DT, in the manner I mentioned in reply
to patch 4, so that this driver can work for arbitrary clients.

> +#define BCM_MBOX_DATA_SHIFT 4

I'd expect that to be defined in some public header that's part of patch
2, so that clients don't have to duplicate the definition.

> +/*
> + * Submits a set of concatenated tags to the VPU firmware through the
> + * mailbox power interface.
> + *
> + * The buffer header and the ending tag are added by this function and
> + * don't need to be supplied, just the actual tags for your operation.
> + * See struct bcm_mbox_power_tag_header for the per-tag structure.
> + */
> +static int bcm_mbox_set_power(uint32_t power_enables)
> +{
> +	int ret;
> +
> +	reinit_completion(&mbox_power->c);
> +	ret = mbox_send_message(mbox_power->chan,
> +				(void *)(power_enables << BCM_MBOX_DATA_SHIFT));
> +	if (ret >= 0)
> +		wait_for_completion(&mbox_power->c);
> +
> +	return ret;
> +}

The comment sounds more like the property mailbox interface/channel,
whereas the code appears to be using the non-property channel. In
particular, the code only appears to be sending one "tag"/message,
without the header or ending tag mentioned in the comment.

> +static int bcm2835_mbox_power_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret = 0;
> +
> +	mbox_power = devm_kzalloc(dev, sizeof(*mbox_power),
> +				       GFP_KERNEL);
> +	if (!mbox_power) {
> +		dev_err(dev, "Failed to allocate device memory\n");

Duplicate error message.

> +	/* Enable power to the USB phy. */
> +	if (IS_ENABLED(CONFIG_USB_DWC2)) {
> +		bcm_mbox_set_power(BCM_POWER_USB);
> +		bcm2835_mbox_power_initialized = true;
> +	}

Hmm. Shouldn't the DWC2 driver make a call to do that?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 06/10] ARM: bcm2835: Add the mailbox power channel to the device tree.

[Eric Anholt]

Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
---
 arch/arm/boot/dts/bcm2835.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index 6b5c3a1..2eeeb20 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -67,6 +67,11 @@
 			#mbox-cells = <1>;
 		};
 
+		mailbox-power {
+			compatible = "brcm,bcm2835-mbox-power";
+			mboxes = <&mailbox 0>;
+		};
+
 		gpio: gpio@7e200000 {
 			compatible = "brcm,bcm2835-gpio";
 			reg = <0x7e200000 0xb4>;
-- 
2.1.4

[PATCH 07/10] usb: Make sure that DWC2 initializes after the power channel mailbox driver.

[Eric Anholt]

This gets USB working on the Raspberry Pi without relying on U-Boot to
send the power message for us.  Without it, you would get warnings
about fifo sizes and "dwc2_core_reset() HANG! Soft Reset
GRSTCTL=80000001" leading to a failed probe.

Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
Cc: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
---
 drivers/mailbox/bcm2835-mailbox-power.c |  5 +++++
 drivers/usb/dwc2/platform.c             | 20 ++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/mailbox/bcm2835-mailbox-power.c b/drivers/mailbox/bcm2835-mailbox-power.c
index f09c855..d0faca2 100644
--- a/drivers/mailbox/bcm2835-mailbox-power.c
+++ b/drivers/mailbox/bcm2835-mailbox-power.c
@@ -31,6 +31,9 @@ struct bcm_mbox_power {
 	uint32_t response;
 };
 
+bool bcm2835_mbox_power_initialized;
+EXPORT_SYMBOL_GPL(bcm2835_mbox_power_initialized);
+
 struct bcm_mbox_power *mbox_power;
 
 static void response_callback(struct mbox_client *cl, void *mssg)
@@ -92,6 +95,7 @@ static int bcm2835_mbox_power_probe(struct platform_device *pdev)
 		bcm_mbox_set_power(BCM_POWER_USB);
 		bcm2835_mbox_power_initialized = true;
 	}
+	bcm2835_mbox_power_initialized = true;
 
 	return ret;
 }
@@ -99,6 +103,7 @@ static int bcm2835_mbox_power_probe(struct platform_device *pdev)
 static int bcm2835_mbox_power_remove(struct platform_device *pdev)
 {
 	bcm_mbox_set_power(0);
+	bcm2835_mbox_power_initialized = false;
 
 	mbox_free_channel(mbox_power->chan);
 
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index ae095f0..2a4a3a6 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -136,6 +136,20 @@ static const struct of_device_id dwc2_of_match_table[] = {
 };
 MODULE_DEVICE_TABLE(of, dwc2_of_match_table);
 
+extern bool bcm2835_mbox_power_initialized;
+
+bool bcm2835_usb_power_enabled(void)
+{
+#ifdef CONFIG_BCM2835_MBOX
+	return bcm2835_mbox_power_initialized;
+#else
+	/* Assume that somebody else has enabled the power for us
+	 * (like U-Boot).
+	 */
+	return true;
+#endif
+}
+
 /**
  * dwc2_driver_probe() - Called when the DWC_otg core is bound to the DWC_otg
  * driver
@@ -175,6 +189,12 @@ static int dwc2_driver_probe(struct platform_device *dev)
 		defparams.dma_desc_enable = 0;
 	}
 
+	/* Make sure the platform driver for enabling the power has
+	 * initialized before we do.
+	 */
+	if (params == &params_bcm2835 && !bcm2835_usb_power_enabled())
+		return -EPROBE_DEFER;
+
 	hsotg = devm_kzalloc(&dev->dev, sizeof(*hsotg), GFP_KERNEL);
 	if (!hsotg)
 		return -ENOMEM;
-- 
2.1.4

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

Re: [PATCH 07/10] usb: Make sure that DWC2 initializes after the power channel mailbox driver.

[Lee Jones]

On Mon, 02 Mar 2015, Eric Anholt wrote:

> This gets USB working on the Raspberry Pi without relying on U-Boot to
> send the power message for us.  Without it, you would get warnings
> about fifo sizes and "dwc2_core_reset() HANG! Soft Reset
> GRSTCTL=80000001" leading to a failed probe.
> 
> Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> Cc: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/mailbox/bcm2835-mailbox-power.c |  5 +++++
>  drivers/usb/dwc2/platform.c             | 20 ++++++++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/mailbox/bcm2835-mailbox-power.c b/drivers/mailbox/bcm2835-mailbox-power.c
> index f09c855..d0faca2 100644
> --- a/drivers/mailbox/bcm2835-mailbox-power.c
> +++ b/drivers/mailbox/bcm2835-mailbox-power.c
> @@ -31,6 +31,9 @@ struct bcm_mbox_power {
>  	uint32_t response;
>  };
>  
> +bool bcm2835_mbox_power_initialized;
> +EXPORT_SYMBOL_GPL(bcm2835_mbox_power_initialized);

This is pretty ugly.  What is it used for?  Is there any way you can
use an API for it instead?

>  struct bcm_mbox_power *mbox_power;
>  
>  static void response_callback(struct mbox_client *cl, void *mssg)
> @@ -92,6 +95,7 @@ static int bcm2835_mbox_power_probe(struct platform_device *pdev)
>  		bcm_mbox_set_power(BCM_POWER_USB);
>  		bcm2835_mbox_power_initialized = true;
>  	}
> +	bcm2835_mbox_power_initialized = true;
>  
>  	return ret;
>  }
> @@ -99,6 +103,7 @@ static int bcm2835_mbox_power_probe(struct platform_device *pdev)
>  static int bcm2835_mbox_power_remove(struct platform_device *pdev)
>  {
>  	bcm_mbox_set_power(0);
> +	bcm2835_mbox_power_initialized = false;
>  
>  	mbox_free_channel(mbox_power->chan);
>  
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index ae095f0..2a4a3a6 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -136,6 +136,20 @@ static const struct of_device_id dwc2_of_match_table[] = {
>  };
>  MODULE_DEVICE_TABLE(of, dwc2_of_match_table);
>  
> +extern bool bcm2835_mbox_power_initialized;

Ughhh, *shudder*.

> +bool bcm2835_usb_power_enabled(void)
> +{
> +#ifdef CONFIG_BCM2835_MBOX

Please don't put #ifdeffery in *.c files.  If you have to match on
CONFIG_* options, please do so using inlines in a header file
somewhere.

> +	return bcm2835_mbox_power_initialized;
> +#else
> +	/* Assume that somebody else has enabled the power for us
> +	 * (like U-Boot).
> +	 */
> +	return true;
> +#endif
> +}
> +
>  /**
>   * dwc2_driver_probe() - Called when the DWC_otg core is bound to the DWC_otg
>   * driver
> @@ -175,6 +189,12 @@ static int dwc2_driver_probe(struct platform_device *dev)
>  		defparams.dma_desc_enable = 0;
>  	}
>  
> +	/* Make sure the platform driver for enabling the power has
> +	 * initialized before we do.
> +	 */

This multi-line comment is unconventional.

See: Documentation/CodingStyle

> +	if (params == &params_bcm2835 && !bcm2835_usb_power_enabled())
> +		return -EPROBE_DEFER;

Why don't you cut out the middle man and get rid of all of these awful
exports?

include/linux/platform/bcm2835.h

#ifdef CONFIG_BCM2835_MBOX
static inline bool bcm2835_is_usb_powered()
{
	return bcm2835_mbox_is_usb_powered();
}
#else
static inline bool bcm2835_is_usb_powered()
{
	/* 
	 * If BCM2835 isn't using its Mailbox(es), the
	 * bootloader is charged with powering up USB.
	 */
	return true;
}
#endif 

>  	hsotg = devm_kzalloc(&dev->dev, sizeof(*hsotg), GFP_KERNEL);
>  	if (!hsotg)
>  		return -ENOMEM;
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 07/10 v2] usb: Make sure that DWC2 initializes after the power channel mailbox driver.

[Eric Anholt]

This gets USB working on the Raspberry Pi without relying on U-Boot to
send the power message for us.  Without it, you would get warnings
about fifo sizes and "dwc2_core_reset() HANG! Soft Reset
GRSTCTL=80000001" leading to a failed probe.

v2: Make the export of the has-the-mailbox-driver-loaded bool be
    through a function call defined in include/linux/platform/

Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
Cc: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
---

This would be the first include file in include/linux/platform/.  Is
that the right place?  I see a few
arch/arm/mach-*/include/mach/platform.h files -- would that or
arch/arm/mach-bcm/include/mach/bcm2835-platform.h possibly be the right
place instead?

Also, Lee, since you mostly typed the contents of that header, should
your SoB go on this?

drivers/mailbox/bcm2835-mailbox-power.c | 11 +++++++++++
 drivers/usb/dwc2/platform.c             |  8 ++++++++
 include/linux/platform/bcm2835.h        | 18 ++++++++++++++++++
 3 files changed, 37 insertions(+)
 create mode 100644 include/linux/platform/bcm2835.h

diff --git a/drivers/mailbox/bcm2835-mailbox-power.c b/drivers/mailbox/bcm2835-mailbox-power.c
index f09c855..a3ed773 100644
--- a/drivers/mailbox/bcm2835-mailbox-power.c
+++ b/drivers/mailbox/bcm2835-mailbox-power.c
@@ -19,6 +19,7 @@
 #include <linux/mailbox_client.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/platform/bcm2835.h>
 
 #define BCM_POWER_USB (1 << 3)
 #define BCM_MBOX_DATA_SHIFT 4
@@ -31,6 +32,14 @@ struct bcm_mbox_power {
 	uint32_t response;
 };
 
+static bool bcm2835_mbox_power_initialized;
+
+bool bcm2835_mbox_is_usb_powered(void)
+{
+	return bcm2835_mbox_power_initialized;
+}
+EXPORT_SYMBOL_GPL(bcm2835_mbox_is_usb_powered);
+
 struct bcm_mbox_power *mbox_power;
 
 static void response_callback(struct mbox_client *cl, void *mssg)
@@ -92,6 +101,7 @@ static int bcm2835_mbox_power_probe(struct platform_device *pdev)
 		bcm_mbox_set_power(BCM_POWER_USB);
 		bcm2835_mbox_power_initialized = true;
 	}
+	bcm2835_mbox_power_initialized = true;
 
 	return ret;
 }
@@ -99,6 +109,7 @@ static int bcm2835_mbox_power_probe(struct platform_device *pdev)
 static int bcm2835_mbox_power_remove(struct platform_device *pdev)
 {
 	bcm_mbox_set_power(0);
+	bcm2835_mbox_power_initialized = false;
 
 	mbox_free_channel(mbox_power->chan);
 
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index ae095f0..caca216 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -42,6 +42,7 @@
 #include <linux/of_device.h>
 #include <linux/mutex.h>
 #include <linux/platform_device.h>
+#include <linux/platform/bcm2835.h>
 
 #include <linux/usb/of.h>
 
@@ -175,6 +176,13 @@ static int dwc2_driver_probe(struct platform_device *dev)
 		defparams.dma_desc_enable = 0;
 	}
 
+	/*
+	 * Make sure the platform driver for enabling the power has
+	 * initialized before we do.
+	 */
+	if (params == &params_bcm2835 && !bcm2835_is_usb_powered())
+		return -EPROBE_DEFER;
+
 	hsotg = devm_kzalloc(&dev->dev, sizeof(*hsotg), GFP_KERNEL);
 	if (!hsotg)
 		return -ENOMEM;
diff --git a/include/linux/platform/bcm2835.h b/include/linux/platform/bcm2835.h
new file mode 100644
index 0000000..a97d938
--- /dev/null
+++ b/include/linux/platform/bcm2835.h
@@ -0,0 +1,18 @@
+#ifdef CONFIG_BCM2835_MBOX
+bool bcm2835_mbox_is_usb_powered(void);
+
+static inline bool bcm2835_is_usb_powered(void)
+{
+	return bcm2835_mbox_is_usb_powered();
+}
+#else
+static inline bool bcm2835_is_usb_powered(void)
+{
+	/*
+	 * If BCM2835 isn't using its Mailbox driver, the bootloader
+	 * is charged with powering up USB.
+	 */
+	return true;
+}
+#endif
+
-- 
2.1.4

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

Re: [PATCH 07/10 v2] usb: Make sure that DWC2 initializes after the power channel mailbox driver.

[Stephen Warren]

On 03/03/2015 01:32 PM, Eric Anholt wrote:
> This gets USB working on the Raspberry Pi without relying on U-Boot to
> send the power message for us.  Without it, you would get warnings
> about fifo sizes and "dwc2_core_reset() HANG! Soft Reset
> GRSTCTL=80000001" leading to a failed probe.
> 
> v2: Make the export of the has-the-mailbox-driver-loaded bool be
>     through a function call defined in include/linux/platform/
> 
> Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> Cc: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> ---
> 
> This would be the first include file in include/linux/platform/.  Is
> that the right place?  I see a few
> arch/arm/mach-*/include/mach/platform.h files -- would that or
> arch/arm/mach-bcm/include/mach/bcm2835-platform.h possibly be the right
> place instead?

include/linux/soc might be better?

Even better would be to make this work through a standard kernel
subsystem API. Then, dwc2's probe would do something like:

// probe:
pd = power_domain_get(&pdev->dev, "power-domain");
if (IS_ERR(pd))
    return PTR_ERR(pd);
// perhaps in probe, or in PM callbacks, etc.
ret = power_domain_on(pd);

That hypothetical function will go out to DT, find the power-domain
property, look at the DT phandle in the property's value, search for a
power domain provider that was instantiated from the DT node the phandle
points at, and obtain a handle to it.

The benefits of this approach are:

a) Uses a standard API rather than something custom.

b) power_domain_get() will return ERR_PTR(-EPROBE_DEFERRED) if the
"power domain provider (driver) instantiated from the DT node the
phandle points at" has not yet probed. In turn this will prevent DWC2
from probing until the power domain can be turned on. The driver core
will call DWC2's probe over and over until it returns something other
than -EPROBE_DEFERRED (i.e. success, or some other failure).

> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c

> @@ -175,6 +176,13 @@ static int dwc2_driver_probe(struct platform_device *dev)
>  		defparams.dma_desc_enable = 0;
>  	}
>  
> +	/*
> +	 * Make sure the platform driver for enabling the power has
> +	 * initialized before we do.
> +	 */
> +	if (params == &params_bcm2835 && !bcm2835_is_usb_powered())
> +		return -EPROBE_DEFER;

This should be something more like:

if dt_property_exists("power-domain") {
  // The power_domain_get code I wrote above
}
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/10] usb: Make sure that DWC2 initializes after the power channel mailbox driver.

[Stephen Warren]

On 03/03/2015 01:32 AM, Lee Jones wrote:
> On Mon, 02 Mar 2015, Eric Anholt wrote:
>> This gets USB working on the Raspberry Pi without relying on U-Boot to
>> send the power message for us.  Without it, you would get warnings
>> about fifo sizes and "dwc2_core_reset() HANG! Soft Reset
>> GRSTCTL=80000001" leading to a failed probe.

>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c

>> +bool bcm2835_usb_power_enabled(void)
>> +{
>> +#ifdef CONFIG_BCM2835_MBOX
> 
> Please don't put #ifdeffery in *.c files.  If you have to match on
> CONFIG_* options, please do so using inlines in a header file
> somewhere.

I haven't heard of that restriction before. I'm sure there are many
ifdefs in C files in the kernel. What benefit does moving the ifdefs
into headers have?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/10] usb: Make sure that DWC2 initializes after the power channel mailbox driver.

[Arnd Bergmann]

On Tuesday 03 March 2015 20:17:24 Stephen Warren wrote:
> On 03/03/2015 01:32 AM, Lee Jones wrote:
> > On Mon, 02 Mar 2015, Eric Anholt wrote:
> >> This gets USB working on the Raspberry Pi without relying on U-Boot to
> >> send the power message for us.  Without it, you would get warnings
> >> about fifo sizes and "dwc2_core_reset() HANG! Soft Reset
> >> GRSTCTL=80000001" leading to a failed probe.
> 
> >> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> 
> >> +bool bcm2835_usb_power_enabled(void)
> >> +{
> >> +#ifdef CONFIG_BCM2835_MBOX
> > 
> > Please don't put #ifdeffery in *.c files.  If you have to match on
> > CONFIG_* options, please do so using inlines in a header file
> > somewhere.
> 
> I haven't heard of that restriction before. I'm sure there are many
> ifdefs in C files in the kernel. What benefit does moving the ifdefs
> into headers have?
> 

It's the common convention.

However, I don't think this should depend on the specific mailbox
implementation at all. The whole point of the mailbox abstraction is
to shield the drivers from the details, and you can rely on the
right mailbox driver being used based on the phandle that is used
in the dt.

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

[PATCH 08/10] dt/bindings: Add binding for BCM2835 mailbox property channel driver

[Eric Anholt]

I was tempted to have the mailbox property channel support just be in
the 2835 mailbox driver itself, but mbox_request_channel() wants its
device to have the "mboxes" node, and that appears to be only intended
for mailbox clients, not controllers.

Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
---
 .../bindings/mailbox/brcm,bcm2835-mbox-property.txt        | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox-property.txt

diff --git a/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox-property.txt b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox-property.txt
new file mode 100644
index 0000000..daed1cc
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox-property.txt
@@ -0,0 +1,14 @@
+Broadcom BCM2835 VideoCore mailbox property channel IPC
+
+Required properties:
+
+- compatible : Should be "brcm,bcm2835-mbox-property"
+- mboxes: Single-entry list which specifies which mailbox controller
+  and channel is the property channel.
+
+Example:
+
+mailbox-property {
+	compatible = "brcm,bcm2835-mbox-property";
+	mboxes = <&mailbox 8>
+};
-- 
2.1.4

Re: [PATCH 08/10] dt/bindings: Add binding for BCM2835 mailbox property channel driver

[Stephen Warren]

On 03/02/2015 01:54 PM, Eric Anholt wrote:
> I was tempted to have the mailbox property channel support just be in
> the 2835 mailbox driver itself, but mbox_request_channel() wants its
> device to have the "mboxes" node, and that appears to be only intended
> for mailbox clients, not controllers.

This is more of a particular format/protocol of messages you can send
over the mailbox HW than a device. I wonder if it actually makes sense
to represent it in DT as a device at all?

If we do represent this as a device in DT, shouldn't it also look like a
mailbox device so that other drivers (clock, display, ...) can bind to
it and send messages using the mailbox API?

I might have expected to just add property support directly into the
basic bcm2835 mailbox driver itself. Perhaps some attempt might be made
to isolate the HW register level access in one file/driver, and expose
both the regular and property mailbox protocols as a higher level that
uses the low-level mailbox functionality? The concept of the lower 4
bits of mailbox data being a channel ID might belong in the higher
protocol level rather than the lower HW layer?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/10] dt/bindings: Add binding for BCM2835 mailbox property channel driver

[Eric Anholt]

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

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:

> On 03/02/2015 01:54 PM, Eric Anholt wrote:
>> I was tempted to have the mailbox property channel support just be in
>> the 2835 mailbox driver itself, but mbox_request_channel() wants its
>> device to have the "mboxes" node, and that appears to be only intended
>> for mailbox clients, not controllers.
>
> This is more of a particular format/protocol of messages you can send
> over the mailbox HW than a device. I wonder if it actually makes sense
> to represent it in DT as a device at all?
>
> If we do represent this as a device in DT, shouldn't it also look like a
> mailbox device so that other drivers (clock, display, ...) can bind to
> it and send messages using the mailbox API?

I don't think it makes sense as a mailbox device.  mailbox devices can
only have one client per channel, while there's no reason to have that
limitation on the property channel.  You could imagine having the
individual tags be channels, except that again there's no reason to have
the one-client limitation, but more importantly the indivudual messages
to the firmware are composed of N tags.

> I might have expected to just add property support directly into the
> basic bcm2835 mailbox driver itself. Perhaps some attempt might be made
> to isolate the HW register level access in one file/driver, and expose
> both the regular and property mailbox protocols as a higher level that
> uses the low-level mailbox functionality? The concept of the lower 4
> bits of mailbox data being a channel ID might belong in the higher
> protocol level rather than the lower HW layer?

Yes, the lower 4 bits being the channel is firmware protocol.

The reason I didn't have it in the same device as the mailbox is that
you need the channel reference in order to set up a mailbox client, and
I didn't think it made sense to have the mailbox device dt reference
itself.

The higher-level interface I think might make sense would be a send_data
replacement that took your device and mbox channel index, and a u32 of
data (low 4 bits unused), and synchronously returned a u32 of response.
It would do the client setup/send/wait_for_completion/teardown for you.

I'm skeptical of putting much more work into mailboxes on this platform,
though.  All we've got for channels are:

0: power (present in this series)
1: fb (Won't be used in Linux)
2: unused
3: VCHIQ (I've heard skepticism that the kernel community would accept this
          interface)
4: unused
5: unused
6: unused
7: settings (not sure if there's anything here not exposed in properties)
8: property (present in this series)
9-15: unused

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

Re: [PATCH 08/10] dt/bindings: Add binding for BCM2835 mailbox property channel driver

[Stephen Warren]

On 03/05/2015 12:50 PM, Eric Anholt wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:
> 
>> On 03/02/2015 01:54 PM, Eric Anholt wrote:
>>> I was tempted to have the mailbox property channel support just
>>> be in the 2835 mailbox driver itself, but
>>> mbox_request_channel() wants its device to have the "mboxes"
>>> node, and that appears to be only intended for mailbox clients,
>>> not controllers.
>> 
>> This is more of a particular format/protocol of messages you can
>> send over the mailbox HW than a device. I wonder if it actually
>> makes sense to represent it in DT as a device at all?
>> 
>> If we do represent this as a device in DT, shouldn't it also look
>> like a mailbox device so that other drivers (clock, display, ...)
>> can bind to it and send messages using the mailbox API?
> 
> I don't think it makes sense as a mailbox device.  mailbox devices
> can only have one client per channel, while there's no reason to
> have that limitation on the property channel.  You could imagine
> having the individual tags be channels, except that again there's
> no reason to have the one-client limitation, but more importantly
> the indivudual messages to the firmware are composed of N tags.

My inclination would be to structure everything as:

a) A mailbox driver/device. This doesn't implement any of the protocol
code. The device appears in DT, since it's real HW.

b) A RPi firmware protocol implementation. This is the only code that
talks directly to the mailbox driver. This would implement support for
all aspects of the RPi firmware protocol; both the non-property and
property channels; they're just different aspects of the one RPi firmware.

I guess there could be a DT node that represents the existence of the
RPi firmware. It would be a bit of a "virtual device" rather than an
actual piece of physical HW, but since firmware is an interface that
the SW interacts with, I guess a DT node makes sense.

This DT node would contain a property pointing at the physical mailbox
provider, so it could send messages.

c) Client drivers (clocks, power domains, ...) All client drivers talk
to (b) not (a). The DT nodes for this code might contain a property
that points at (b), but the API from c->b would likely be something
custom rather than the mailbox API, since the RPi firmware protocol
implements something custom rather than standard.

I'd be interested to hear opinions from people much more familiar with
other mailbox HW and protocol drivers. Perhaps they're all lumped
together on other platforms?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/10] dt/bindings: Add binding for BCM2835 mailbox property channel driver

[Jassi Brar]

On Fri, Mar 6, 2015 at 10:45 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 03/05/2015 12:50 PM, Eric Anholt wrote:
>> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:
>>
>>> On 03/02/2015 01:54 PM, Eric Anholt wrote:
>>>> I was tempted to have the mailbox property channel support just
>>>> be in the 2835 mailbox driver itself, but
>>>> mbox_request_channel() wants its device to have the "mboxes"
>>>> node, and that appears to be only intended for mailbox clients,
>>>> not controllers.
>>>
>>> This is more of a particular format/protocol of messages you can
>>> send over the mailbox HW than a device. I wonder if it actually
>>> makes sense to represent it in DT as a device at all?
>>>
>>> If we do represent this as a device in DT, shouldn't it also look
>>> like a mailbox device so that other drivers (clock, display, ...)
>>> can bind to it and send messages using the mailbox API?
>>
>> I don't think it makes sense as a mailbox device.  mailbox devices
>> can only have one client per channel, while there's no reason to
>> have that limitation on the property channel.  You could imagine
>> having the individual tags be channels, except that again there's
>> no reason to have the one-client limitation, but more importantly
>> the indivudual messages to the firmware are composed of N tags.
>
> My inclination would be to structure everything as:
>
> a) A mailbox driver/device. This doesn't implement any of the protocol
> code. The device appears in DT, since it's real HW.
>
Exactly.

> b) A RPi firmware protocol implementation. This is the only code that
> talks directly to the mailbox driver. This would implement support for
> all aspects of the RPi firmware protocol; both the non-property and
> property channels; they're just different aspects of the one RPi firmware.
>
Yes, I usually call that the 'server' driver - a gobal client driver
that serves other client subsystems.

> I guess there could be a DT node that represents the existence of the
> RPi firmware. It would be a bit of a "virtual device" rather than an
> actual piece of physical HW, but since firmware is an interface that
> the SW interacts with, I guess a DT node makes sense.
>
> This DT node would contain a property pointing at the physical mailbox
> provider, so it could send messages.
>
Yes, as long as a device has one physical resource IRQ, register,
clock I think it has the right to be in DT.
It will be a physical mailbox channel, so it qualifies.

> c) Client drivers (clocks, power domains, ...) All client drivers talk
> to (b) not (a). The DT nodes for this code might contain a property
> that points at (b), but the API from c->b would likely be something
> custom rather than the mailbox API, since the RPi firmware protocol
> implements something custom rather than standard.
>
Yes, c<->b protocol will be Linux specific.  b<->a will be
pre-determined by the remote firmware.

I haven't had the time to look deeply but from quick overview:
1) bcm2835-mailbox-power.c and bcm2835-mailbox-property.c are client
drivers and shouldn't reside in drivers/mailbox/

2) The bcm2835-mailbox-power should probably be a part of platform's
USB support - which should try to call remote to enable power upon
init. Or even better, let the 'server' driver (b) manage that mailbox
as well, if it could be used for other purposes on some other
platform.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/10] ARM: bcm2835: Add the mailbox property channel driver.

[Eric Anholt]

Many of the operations with the firmware are done through this mailbox
channel pair with a specific packet format.  Notably, it's used for
clock control, which is apparently not actually totally possible to do
from the ARM side (some regs aren't addressable).  I need clock
control for the VC4 DRM driver, to turn on the 3D engine.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/mailbox/Makefile                         |   1 +
 drivers/mailbox/bcm2835-mailbox-property.c       | 172 +++++++++++++++++++++++
 include/linux/mailbox/bcm2835-mailbox-property.h | 106 ++++++++++++++
 3 files changed, 279 insertions(+)
 create mode 100644 drivers/mailbox/bcm2835-mailbox-property.c
 create mode 100644 include/linux/mailbox/bcm2835-mailbox-property.h

diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 619d815..0318966 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_ALTERA_MBOX)	+= mailbox-altera.o
 
 obj-$(CONFIG_BCM2835_MBOX)	+= bcm2835-mailbox.o
 obj-$(CONFIG_BCM2835_MBOX)	+= bcm2835-mailbox-power.o
+obj-$(CONFIG_BCM2835_MBOX)	+= bcm2835-mailbox-property.o
diff --git a/drivers/mailbox/bcm2835-mailbox-property.c b/drivers/mailbox/bcm2835-mailbox-property.c
new file mode 100644
index 0000000..d3590a9
--- /dev/null
+++ b/drivers/mailbox/bcm2835-mailbox-property.c
@@ -0,0 +1,172 @@
+/*
+ *  Copyright © 2015 Broadcom
+ *
+ * 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.
+ */
+
+/*
+ * Defines a module for accessing the property channel of the
+ * BCM2835 mailbox.
+ *
+ * The property interface lets you submit a bus address for a
+ * sequence of command packets to the firmware.
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/mailbox_client.h>
+#include <linux/mailbox/bcm2835-mailbox-property.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+static DEFINE_MUTEX(property_lock);
+
+struct bcm_mbox_property {
+	struct device *dev;
+	struct mbox_client cl;
+	struct mbox_chan *chan;
+	struct completion c;
+};
+
+struct bcm_mbox_property *mbox_property;
+
+static void response_callback(struct mbox_client *cl, void *mssg)
+{
+	complete(&mbox_property->c);
+}
+
+/*
+ * Requests the actual property transaction through the BCM2835
+ * mailbox driver.
+ */
+static int
+bcm_mbox_property_transaction(dma_addr_t bus_addr)
+{
+	int ret;
+
+	reinit_completion(&mbox_property->c);
+	ret = mbox_send_message(mbox_property->chan, (void *)bus_addr);
+	if (ret >= 0) {
+		wait_for_completion(&mbox_property->c);
+		ret = 0;
+	} else {
+		dev_err(mbox_property->dev, "mbox_send_message returned %d\n",
+			ret);
+	}
+
+	return ret;
+}
+
+/*
+ * Submits a set of concatenated tags to the VPU firmware through the
+ * mailbox property interface.
+ *
+ * The buffer header and the ending tag are added by this function and
+ * don't need to be supplied, just the actual tags for your operation.
+ * See struct bcm_mbox_property_tag_header for the per-tag structure.
+ */
+int bcm_mbox_property(void *data, size_t tag_size)
+{
+	size_t size = tag_size + 12;
+	void __iomem *buf;
+	dma_addr_t bus_addr;
+	int ret = 0;
+
+	/* Packets are processed a dword at a time. */
+	if (size & 3)
+		return -EINVAL;
+
+	buf = dma_alloc_coherent(NULL, PAGE_ALIGN(size), &bus_addr, GFP_ATOMIC);
+	if (!buf)
+		return -ENOMEM;
+
+	/* The firmware will error out without parsing in this case. */
+	WARN_ON(size >= 1024 * 1024);
+
+	writel(size, buf);
+	writel(bcm_mbox_status_request, buf + 4);
+	memcpy_toio(buf + 8, data, tag_size);
+	writel(bcm_mbox_property_end, buf + size - 4);
+
+	mutex_lock(&property_lock);
+	ret = bcm_mbox_property_transaction(bus_addr);
+	mutex_unlock(&property_lock);
+
+	memcpy_fromio(data, buf + 8, tag_size);
+	if (readl(buf + 4) != bcm_mbox_status_success) {
+		/*
+		 * The tag name here might not be the one causing the
+		 * error, if there were multiple tags in the request.
+		 * But single-tag is the most common, so go with it.
+		 */
+		dev_err(mbox_property->dev,
+			"Request 0x%08x returned status 0x%08x\n",
+			readl(buf + 8), readl(buf + 4));
+		ret = -EINVAL;
+	}
+
+	dma_free_coherent(NULL, PAGE_ALIGN(size), buf, bus_addr);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(bcm_mbox_property);
+
+
+static int bcm2835_mbox_property_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int ret = 0;
+
+	mbox_property = devm_kzalloc(dev, sizeof(*mbox_property),
+				       GFP_KERNEL);
+	if (!mbox_property) {
+		dev_err(dev, "Failed to allocate device memory\n");
+		return -ENOMEM;
+	}
+
+	mbox_property->cl.dev = dev;
+	mbox_property->cl.rx_callback = response_callback;
+	mbox_property->cl.tx_block = true;
+
+	mbox_property->chan = mbox_request_channel(&mbox_property->cl, 0);
+	if (!mbox_property->chan) {
+		dev_err(dev, "Failed to get mbox channel\n");
+		return -ENODEV;
+	}
+
+	init_completion(&mbox_property->c);
+
+	platform_set_drvdata(pdev, mbox_property);
+	mbox_property->dev = dev;
+
+	return ret;
+}
+
+static int bcm2835_mbox_property_remove(struct platform_device *pdev)
+{
+	mbox_free_channel(mbox_property->chan);
+
+	return 0;
+}
+
+static const struct of_device_id bcm2835_mbox_property_of_match[] = {
+	{ .compatible = "brcm,bcm2835-mbox-property", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm2835_mbox_property_of_match);
+
+static struct platform_driver bcm2835_mbox_property_driver = {
+	.driver = {
+		.name = "bcm2835-mbox-property",
+		.owner = THIS_MODULE,
+		.of_match_table = bcm2835_mbox_property_of_match,
+	},
+	.probe		= bcm2835_mbox_property_probe,
+	.remove		= bcm2835_mbox_property_remove,
+};
+module_platform_driver(bcm2835_mbox_property_driver);
+
+MODULE_AUTHOR("Eric Anholt <eric@anholt.net>");
+MODULE_DESCRIPTION("BCM2835 mailbox property channel");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mailbox/bcm2835-mailbox-property.h b/include/linux/mailbox/bcm2835-mailbox-property.h
new file mode 100644
index 0000000..783c069
--- /dev/null
+++ b/include/linux/mailbox/bcm2835-mailbox-property.h
@@ -0,0 +1,106 @@
+/*
+ *  Copyright © 2015 Broadcom
+ *
+ * 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.
+ */
+
+enum bcm_mbox_property_status {
+	bcm_mbox_status_request = 0,
+	bcm_mbox_status_success = 0x80000000,
+	bcm_mbox_status_error =   0x80000001,
+};
+
+struct bcm_mbox_property_tag_header {
+	/* One of enum_mbox_property_tag. */
+	u32 tag;
+
+	/* The number of bytes in the value buffer following this
+	 * struct.
+	 */
+	u32 buf_size;
+
+	/*
+	 * On submit, the length of the request (though it doesn't
+	 * appear to be currently used by the firmware).  On return,
+	 * the length of the response (always 4 byte aligned), with
+	 * the low bit set.
+	 */
+	u32 req_resp_size;
+};
+
+enum bcm_mbox_property_tag {
+	bcm_mbox_property_end = 0,
+	bcm_mbox_get_firmware_revision =                  0x00000001,
+
+	bcm_mbox_set_cursor_info =                        0x00008010,
+	bcm_mbox_set_cursor_state =                       0x00008011,
+
+	bcm_mbox_get_board_model =                        0x00010001,
+	bcm_mbox_get_board_revision =                     0x00010002,
+	bcm_mbox_get_board_mac_address =                  0x00010003,
+	bcm_mbox_get_board_serial =                       0x00010004,
+	bcm_mbox_get_arm_memory =                         0x00010005,
+	bcm_mbox_get_vc_memory =                          0x00010006,
+	bcm_mbox_get_clocks =                             0x00010007,
+	bcm_mbox_get_power_state =                        0x00020001,
+	bcm_mbox_get_timing =                             0x00020002,
+	bcm_mbox_set_power_state =                        0x00028001,
+	bcm_mbox_get_clock_state =                        0x00030001,
+	bcm_mbox_get_clock_rate =                         0x00030002,
+	bcm_mbox_get_voltage =                            0x00030003,
+	bcm_mbox_get_max_clock_rate =                     0x00030004,
+	bcm_mbox_get_max_voltage =                        0x00030005,
+	bcm_mbox_get_temperature =                        0x00030006,
+	bcm_mbox_get_min_clock_rate =                     0x00030007,
+	bcm_mbox_get_min_voltage =                        0x00030008,
+	bcm_mbox_get_turbo =                              0x00030009,
+	bcm_mbox_get_max_temperature =                    0x0003000a,
+	bcm_mbox_allocate_memory =                        0x0003000c,
+	bcm_mbox_lock_memory =                            0x0003000d,
+	bcm_mbox_unlock_memory =                          0x0003000e,
+	bcm_mbox_release_memory =                         0x0003000f,
+	bcm_mbox_execute_code =                           0x00030010,
+	bcm_mbox_get_dispmanx_resource_mem_handle =       0x00030014,
+	bcm_mbox_get_edid_block =                         0x00030020,
+	bcm_mbox_set_clock_state =                        0x00038001,
+	bcm_mbox_set_clock_rate =                         0x00038002,
+	bcm_mbox_set_voltage =                            0x00038003,
+	bcm_mbox_set_turbo =                              0x00038009,
+
+	/* Dispmanx tags */
+	bcm_mbox_framebuffer_allocate =                   0x00040001,
+	bcm_mbox_framebuffer_blank =                      0x00040002,
+	bcm_mbox_framebuffer_get_physical_width_height =  0x00040003,
+	bcm_mbox_framebuffer_get_virtual_width_height =   0x00040004,
+	bcm_mbox_framebuffer_get_depth =                  0x00040005,
+	bcm_mbox_framebuffer_get_pixel_order =            0x00040006,
+	bcm_mbox_framebuffer_get_alpha_mode =             0x00040007,
+	bcm_mbox_framebuffer_get_pitch =                  0x00040008,
+	bcm_mbox_framebuffer_get_virtual_offset =         0x00040009,
+	bcm_mbox_framebuffer_get_overscan =               0x0004000a,
+	bcm_mbox_framebuffer_get_palette =                0x0004000b,
+	bcm_mbox_framebuffer_release =                    0x00048001,
+	bcm_mbox_framebuffer_test_physical_width_height = 0x00044003,
+	bcm_mbox_framebuffer_test_virtual_width_height =  0x00044004,
+	bcm_mbox_framebuffer_test_depth =                 0x00044005,
+	bcm_mbox_framebuffer_test_pixel_order =           0x00044006,
+	bcm_mbox_framebuffer_test_alpha_mode =            0x00044007,
+	bcm_mbox_framebuffer_test_virtual_offset =        0x00044009,
+	bcm_mbox_framebuffer_test_overscan =              0x0004400a,
+	bcm_mbox_framebuffer_test_palette =               0x0004400b,
+	bcm_mbox_framebuffer_set_physical_width_height =  0x00048003,
+	bcm_mbox_framebuffer_set_virtual_width_height =   0x00048004,
+	bcm_mbox_framebuffer_set_depth =                  0x00048005,
+	bcm_mbox_framebuffer_set_pixel_order =            0x00048006,
+	bcm_mbox_framebuffer_set_alpha_mode =             0x00048007,
+	bcm_mbox_framebuffer_set_virtual_offset =         0x00048009,
+	bcm_mbox_framebuffer_set_overscan =               0x0004800a,
+	bcm_mbox_framebuffer_set_palette =                0x0004800b,
+
+	bcm_mbox_get_command_line =                       0x00050001,
+	bcm_mbox_get_dma_channels =                       0x00060001,
+};
+
+int bcm_mbox_property(void *data, size_t tag_size);
-- 
2.1.4


_______________________________________________
linux-rpi-kernel mailing list
linux-rpi-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel

Re: [PATCH 09/10] ARM: bcm2835: Add the mailbox property channel driver.

[Stephen Warren]

On 03/02/2015 01:54 PM, Eric Anholt wrote:
> Many of the operations with the firmware are done through this mailbox
> channel pair with a specific packet format.  Notably, it's used for
> clock control, which is apparently not actually totally possible to do
> from the ARM side (some regs aren't addressable).  I need clock
> control for the VC4 DRM driver, to turn on the 3D engine.

> diff --git a/drivers/mailbox/bcm2835-mailbox-property.c b/drivers/mailbox/bcm2835-mailbox-property.c
> +int bcm_mbox_property(void *data, size_t tag_size)

> +	buf = dma_alloc_coherent(NULL, PAGE_ALIGN(size), &bus_addr, GFP_ATOMIC);
> +	if (!buf)
> +		return -ENOMEM;

Can't the driver (this one or the client) maintain some persistent
buffer rather than allocating/freeing a new one each time. It seems like
the alloc/free might introduce quite some overhead?

> +	writel(size, buf);
> +	writel(bcm_mbox_status_request, buf + 4);
> +	memcpy_toio(buf + 8, data, tag_size);
> +	writel(bcm_mbox_property_end, buf + size - 4);

Since this is just a regular chunk of RAM, can't the code just use
regular memory writes and memcpy()?

> +static int bcm2835_mbox_property_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret = 0;
> +
> +	mbox_property = devm_kzalloc(dev, sizeof(*mbox_property),
> +				       GFP_KERNEL);
> +	if (!mbox_property) {
> +		dev_err(dev, "Failed to allocate device memory\n");

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

Re: [PATCH 09/10] ARM: bcm2835: Add the mailbox property channel driver.

[Eric Anholt]

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

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:

> On 03/02/2015 01:54 PM, Eric Anholt wrote:
>> Many of the operations with the firmware are done through this mailbox
>> channel pair with a specific packet format.  Notably, it's used for
>> clock control, which is apparently not actually totally possible to do
>> from the ARM side (some regs aren't addressable).  I need clock
>> control for the VC4 DRM driver, to turn on the 3D engine.
>
>> diff --git a/drivers/mailbox/bcm2835-mailbox-property.c b/drivers/mailbox/bcm2835-mailbox-property.c
>> +int bcm_mbox_property(void *data, size_t tag_size)
>
>> +	buf = dma_alloc_coherent(NULL, PAGE_ALIGN(size), &bus_addr, GFP_ATOMIC);
>> +	if (!buf)
>> +		return -ENOMEM;
>
> Can't the driver (this one or the client) maintain some persistent
> buffer rather than allocating/freeing a new one each time. It seems like
> the alloc/free might introduce quite some overhead?

The size of the buffer is arbitrary (up to 1MB), the frequency of
requessts is low, and the hardware's pretty starved for RAM.  However,
we're probably only ever going to see single page allocations, so the
RAM cost is probably low and the allocation time is probably also
correspondingly low (not like when I was trying to do 256k allocations
in the vc4 driver.  ouch).

>> +	writel(size, buf);
>> +	writel(bcm_mbox_status_request, buf + 4);
>> +	memcpy_toio(buf + 8, data, tag_size);
>> +	writel(bcm_mbox_property_end, buf + size - 4);
>
> Since this is just a regular chunk of RAM, can't the code just use
> regular memory writes and memcpy()?

will wmb() guarantee that the compiler won't optimize out my write to
buf[0] and corresponding read from buf[0]?  I was originally treating it
as RAM and using volatile, but then reread the old "don't use volatile"
doc.

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

Re: [PATCH 09/10] ARM: bcm2835: Add the mailbox property channel driver.

[Stephen Warren]

On 03/05/2015 12:54 PM, Eric Anholt wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:
> 
>> On 03/02/2015 01:54 PM, Eric Anholt wrote:
>>> Many of the operations with the firmware are done through this
>>> mailbox channel pair with a specific packet format.  Notably,
>>> it's used for clock control, which is apparently not actually
>>> totally possible to do from the ARM side (some regs aren't
>>> addressable).  I need clock control for the VC4 DRM driver, to
>>> turn on the 3D engine.
>> 
>>> diff --git a/drivers/mailbox/bcm2835-mailbox-property.c
>>> b/drivers/mailbox/bcm2835-mailbox-property.c +int
>>> bcm_mbox_property(void *data, size_t tag_size)
>> 
>>> +	buf = dma_alloc_coherent(NULL, PAGE_ALIGN(size), &bus_addr,
>>> GFP_ATOMIC); +	if (!buf) +		return -ENOMEM;
>> 
>> Can't the driver (this one or the client) maintain some
>> persistent buffer rather than allocating/freeing a new one each
>> time. It seems like the alloc/free might introduce quite some
>> overhead?
> 
> The size of the buffer is arbitrary (up to 1MB), the frequency of 
> requessts is low, and the hardware's pretty starved for RAM.
> However, we're probably only ever going to see single page
> allocations, so the RAM cost is probably low and the allocation
> time is probably also correspondingly low (not like when I was
> trying to do 256k allocations in the vc4 driver.  ouch).

OK. Since this is an implementation detail with no effect on DT ABI,
it would be easy to change later if it did turn out to be an issue.

>>> +	writel(size, buf); +	writel(bcm_mbox_status_request, buf +
>>> 4); +	memcpy_toio(buf + 8, data, tag_size); +
>>> writel(bcm_mbox_property_end, buf + size - 4);
>> 
>> Since this is just a regular chunk of RAM, can't the code just
>> use regular memory writes and memcpy()?
> 
> will wmb() guarantee that the compiler won't optimize out my write
> to buf[0] and corresponding read from buf[0]?  I was originally
> treating it as RAM and using volatile, but then reread the old
> "don't use volatile" doc.

Yes, all of the barrier implementations boil down to one of:

#define isb(x) __asm__ __volatile__ ("mcr p15, 0, %0, c7, c5, 4" \
                                    : : "r" (0) : "memory")
#define dsb(x) __asm__ __volatile__ ("mcr p15, 0, %0, c7, c10, 4" \
                                    : : "r" (0) : "memory")
#define dmb(x) __asm__ __volatile__ ("mcr p15, 0, %0, c7, c10, 5" \
                                    : : "r" (0) : "memory")

The final :"memory" tells the compiler that the __asm__ statement may
change memory, so the compiler isn't allowed to "cache" anything over
the statement.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/10] ARM: bcm2835: Add the mailbox property channel driver.

[Stephen Warren]

On 03/05/2015 12:54 PM, Eric Anholt wrote:
> Stephen Warren <swarren@wwwdotorg.org> writes:
> 
>> On 03/02/2015 01:54 PM, Eric Anholt wrote:
>>> Many of the operations with the firmware are done through this
>>> mailbox channel pair with a specific packet format.  Notably,
>>> it's used for clock control, which is apparently not actually
>>> totally possible to do from the ARM side (some regs aren't
>>> addressable).  I need clock control for the VC4 DRM driver, to
>>> turn on the 3D engine.
>> 
>>> diff --git a/drivers/mailbox/bcm2835-mailbox-property.c
>>> b/drivers/mailbox/bcm2835-mailbox-property.c +int
>>> bcm_mbox_property(void *data, size_t tag_size)
>> 
>>> +	buf = dma_alloc_coherent(NULL, PAGE_ALIGN(size), &bus_addr,
>>> GFP_ATOMIC); +	if (!buf) +		return -ENOMEM;
>> 
>> Can't the driver (this one or the client) maintain some
>> persistent buffer rather than allocating/freeing a new one each
>> time. It seems like the alloc/free might introduce quite some
>> overhead?
> 
> The size of the buffer is arbitrary (up to 1MB), the frequency of 
> requessts is low, and the hardware's pretty starved for RAM.
> However, we're probably only ever going to see single page
> allocations, so the RAM cost is probably low and the allocation
> time is probably also correspondingly low (not like when I was
> trying to do 256k allocations in the vc4 driver.  ouch).

OK. Since this is an implementation detail with no effect on DT ABI,
it would be easy to change later if it did turn out to be an issue.

>>> +	writel(size, buf); +	writel(bcm_mbox_status_request, buf +
>>> 4); +	memcpy_toio(buf + 8, data, tag_size); +
>>> writel(bcm_mbox_property_end, buf + size - 4);
>> 
>> Since this is just a regular chunk of RAM, can't the code just
>> use regular memory writes and memcpy()?
> 
> will wmb() guarantee that the compiler won't optimize out my write
> to buf[0] and corresponding read from buf[0]?  I was originally
> treating it as RAM and using volatile, but then reread the old
> "don't use volatile" doc.

Yes, all of the barrier implementations boil down to one of:

#define isb(x) __asm__ __volatile__ ("mcr p15, 0, %0, c7, c5, 4" \
                                    : : "r" (0) : "memory")
#define dsb(x) __asm__ __volatile__ ("mcr p15, 0, %0, c7, c10, 4" \
                                    : : "r" (0) : "memory")
#define dmb(x) __asm__ __volatile__ ("mcr p15, 0, %0, c7, c10, 5" \
                                    : : "r" (0) : "memory")

The final :"memory" tells the compiler that the __asm__ statement may
change memory, so the compiler isn't allowed to "cache" anything over
the statement.

[PATCH 10/10] ARM: bcm2835: Add the mailbox property channel to the device tree.

[Eric Anholt]

Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
---
 arch/arm/boot/dts/bcm2835.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index 2eeeb20..82c6947 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -72,6 +72,11 @@
 			mboxes = <&mailbox 0>;
 		};
 
+		mailbox-property {
+			compatible = "brcm,bcm2835-mbox-property";
+			mboxes = <&mailbox 8>;
+		};
+
 		gpio: gpio@7e200000 {
 			compatible = "brcm,bcm2835-gpio";
 			reg = <0x7e200000 0xb4>;
-- 
2.1.4