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

[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

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

[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

[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>

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

[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 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>

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

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