[PATCH 1/4] ARM: BCM2835: Add a function for doing an rmb() between device reads.

[PATCH 1/4] ARM: BCM2835: Add a function for doing an rmb() between device reads.

[Eric Anholt]

Stephen Warren was concerned that the rmb() present in the new mailbox
driver was unnecessary, and after seeing the docs, that it was just so
surprising that somebody would come along and remove it later.  The
explanation for the need for the rmb() is long enough that we won't
want to place it at every callsite.  Make a wrapper with the whole
explanation in it, so that anyone wondering what's going on sees the
docs right there.

Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
---
 include/soc/bcm2835/peripheral-workaround.h | 75 +++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 include/soc/bcm2835/peripheral-workaround.h

diff --git a/include/soc/bcm2835/peripheral-workaround.h b/include/soc/bcm2835/peripheral-workaround.h
new file mode 100644
index 0000000..4541a13
--- /dev/null
+++ b/include/soc/bcm2835/peripheral-workaround.h
@@ -0,0 +1,75 @@
+/*
+ * 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.
+ */
+
+static inline void bcm2835_peripheral_read_workaround(void)
+{
+#ifdef CONFIG_ARCH_BCM2835
+	/*
+	 * The BCM2835 bus is unusual in that it doesn't guarantee
+	 * ordering between reads from different peripherals (where
+	 * peripherals roughly correspond to Linux devices).  From
+	 * BCM2835 ARM Peripherals.pdf, page 7:
+	 *
+	 *     "In order to keep the system complexity low and data
+	 *      throughput high, the BCM2835 AXI system does not
+	 *      always return read data in-order. The GPU has special
+	 *      logic to cope with data arriving out-of-order; however
+	 *      the ARM core does not contain such logic. Therefore
+	 *      some precautions must be taken when using the ARM to
+	 *      access peripherals.
+	 *
+	 *      Accesses to the same peripheral will always arrive and
+	 *      return in-order. It is only when switching from one
+	 *      peripheral to another that data can arrive
+	 *      out-of-order. The simplest way to make sure that data
+	 *      is processed in-order is to place a memory barrier
+	 *      instruction at critical positions in the code. 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."
+	 *
+	 * The footnote explicitly says that:
+	 *
+	 *      "For example:
+	 *
+	 *         a_status = *pointer_to_peripheral_a;
+	 *         b_status = *pointer_to_peripheral_b;
+	 *
+	 *       Without precuations the values ending up in the
+	 *       variables a_status and b_status can be swapped
+	 *       around."
+	 *
+	 * However, it also notes that, somewhat contrary to the first
+	 * bullet point:
+	 *
+	 *     "It is theoretical possible for writes to go ‘wrong’
+	 *      but that is far more difficult to achieve. The AXI
+	 *      system makes sure the data always arrives in-order at
+	 *      its intended destination. So:
+	 *
+	 *        *pointer_to_peripheral_a = value_a;
+	 *        *pointer_to_peripheral_b = value_b;
+	 *
+	 *      will always give the expected result. The only time
+	 *      write data can arrive out-of-order is if two different
+	 *      peripherals are connected to the same external
+	 *      equipment"
+	 *
+	 * Since we aren't interacting with multiple peripherals
+	 * connected to the same external equipment as far as we know,
+	 * that means that we only need to handle the read workaround
+	 * case.  We do so by placing an rmb() at the first device
+	 * read acceess in a given driver path, including the
+	 * interrupt handlers.
+	 */
+	rmb();
+#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

[PATCH 2/4 v3] dt/bindings: Add binding for BCM2835 mailbox driver

[Eric Anholt]

From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>

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


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

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

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

diff --git a/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt
new file mode 100644
index 0000000..0bb2b9d
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/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

--
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 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

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.

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

Signed-off-by: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
Signed-off-by: Craig McGeachie <slapdau-/E1597aS9LT0CCvOHzKKcA@public.gmane.org>
Signed-off-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
Signed-off-by: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
Cc: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---


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

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

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

 drivers/mailbox/Kconfig           |   8 ++
 drivers/mailbox/Makefile          |   2 +
 drivers/mailbox/bcm2835-mailbox.c | 259 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 269 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..01ddb53
--- /dev/null
+++ b/drivers/mailbox/bcm2835-mailbox.c
@@ -0,0 +1,259 @@
+/*
+ *  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>
+#include <soc/bcm2835/peripheral-workaround.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;
+
+	bcm2835_peripheral_read_workaround();
+
+	while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
+		u32 msg = readl(mbox->regs + MAIL0_RD);
+		unsigned int chan = MBOX_CHAN(msg);
+		u32 data = MBOX_DATA28(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, &data);
+	}
+	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;
+	u32 msg = MBOX_MSG(chan->chan_num, *(u32 *)data);
+
+	if (!chan->started)
+		return -ENODEV;
+	spin_lock(&mbox->lock);
+	bcm2835_peripheral_read_workaround();
+	if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) {
+		ret = -EBUSY;
+		goto end;
+	}
+	writel(msg, mbox->regs + MAIL1_WRT);
+	dev_dbg(mbox->dev, "Request 0x%08X\n", msg);
+end:
+	spin_unlock(&mbox->lock);
+	return ret;
+}
+
+static int bcm2835_startup(struct mbox_chan *link)
+{
+	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);
+	bcm2835_peripheral_read_workaround();
+	ret = !(readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL);
+	spin_unlock(&mbox->lock);
+	return ret;
+}
+
+static struct mbox_chan_ops bcm2835_mbox_chan_ops = {
+	.send_data	= bcm2835_send_data,
+	.startup	= bcm2835_startup,
+	.shutdown	= bcm2835_shutdown,
+	.last_tx_done	= bcm2835_last_tx_done
+};
+
+static int bcm2835_mbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct bcm2835_mbox *mbox;
+	int i;
+	int ret = 0;
+	struct resource *iomem;
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (mbox == NULL)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, mbox);
+	mbox->pdev = pdev;
+	mbox->dev = dev;
+	spin_lock_init(&mbox->lock);
+
+	ret = devm_request_irq(dev, irq_of_parse_and_map(dev->of_node, 0),
+			       bcm2835_mbox_irq, 0, dev_name(dev),
+		mbox);
+	if (ret) {
+		dev_err(dev, "Failed to register a mailbox IRQ handler: %d\n",
+			ret);
+		return -ENODEV;
+	}
+
+	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mbox->regs = devm_ioremap_resource(&pdev->dev, iomem);
+	if (IS_ERR(mbox->regs)) {
+		ret = PTR_ERR(mbox->regs);
+		dev_err(&pdev->dev, "Failed to remap mailbox regs: %d\n", ret);
+		return ret;
+	}
+
+	mbox->controller.txdone_poll = true;
+	mbox->controller.txpoll_period = 5;
+	mbox->controller.ops = &bcm2835_mbox_chan_ops;
+	mbox->controller.dev = dev;
+	mbox->controller.num_chans = MBOX_CHAN_COUNT;
+	mbox->controller.chans = devm_kzalloc(dev,
+		sizeof(*mbox->controller.chans) * MBOX_CHAN_COUNT,
+		GFP_KERNEL);
+	if (!mbox->controller.chans)
+		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);
+
+	writel(0, mbox->regs + MAIL0_CNF);
+	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-NGH9Lh4a5iE@public.gmane.org>");
+MODULE_DESCRIPTION("BCM2835 mailbox IPC driver");
+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

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

[Eric Anholt]

Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@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

--
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 1/4] ARM: BCM2835: Add a function for doing an rmb() between device reads.

[Stephen Warren]

On 03/12/2015 08:32 PM, Eric Anholt wrote:
> Stephen Warren was concerned that the rmb() present in the new mailbox
> driver was unnecessary, and after seeing the docs, that it was just so
> surprising that somebody would come along and remove it later.  The
> explanation for the need for the rmb() is long enough that we won't
> want to place it at every callsite.  Make a wrapper with the whole
> explanation in it, so that anyone wondering what's going on sees the
> docs right there.

> diff --git a/include/soc/bcm2835/peripheral-workaround.h b/include/soc/bcm2835/peripheral-workaround.h

> +static inline void bcm2835_peripheral_read_workaround(void)
> +{
> +#ifdef CONFIG_ARCH_BCM2835

Would this header be included if that wasn't defined? Perhaps that'll be
answered by a later patch...

> +	/*
> +	 * The BCM2835 bus is unusual in that it doesn't guarantee
> +	 * ordering between reads from different peripherals (where
> +	 * peripherals roughly correspond to Linux devices).  From
> +	 * BCM2835 ARM Peripherals.pdf, page 7:

Many buses don't guarantee ordering; that's quite common. The issue is
that the CPU then doesn't match up the correct read request and
response, thus causing it to swap the results of read requests. That's
the unusual part. It would be useful to spell that out more explicitly
in this introduction, even though it is called out in the example below.

BTW, the ARM mailing list is linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org not
linux-arm-kernel-u79uwXL29TaiAVqoAR/hOA@public.gmane.org
--
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 1/4] ARM: BCM2835: Add a function for doing an rmb() between device reads.

[Stephen Warren]

On 03/12/2015 08:32 PM, Eric Anholt wrote:
> Stephen Warren was concerned that the rmb() present in the new mailbox
> driver was unnecessary, and after seeing the docs, that it was just so
> surprising that somebody would come along and remove it later.  The
> explanation for the need for the rmb() is long enough that we won't
> want to place it at every callsite.  Make a wrapper with the whole
> explanation in it, so that anyone wondering what's going on sees the
> docs right there.

> diff --git a/include/soc/bcm2835/peripheral-workaround.h b/include/soc/bcm2835/peripheral-workaround.h

> +static inline void bcm2835_peripheral_read_workaround(void)
> +{
> +#ifdef CONFIG_ARCH_BCM2835

Would this header be included if that wasn't defined? Perhaps that'll be
answered by a later patch...

> +	/*
> +	 * The BCM2835 bus is unusual in that it doesn't guarantee
> +	 * ordering between reads from different peripherals (where
> +	 * peripherals roughly correspond to Linux devices).  From
> +	 * BCM2835 ARM Peripherals.pdf, page 7:

Many buses don't guarantee ordering; that's quite common. The issue is
that the CPU then doesn't match up the correct read request and
response, thus causing it to swap the results of read requests. That's
the unusual part. It would be useful to spell that out more explicitly
in this introduction, even though it is called out in the example below.

BTW, the ARM mailing list is linux-arm-kernel at lists.infradead.org not
linux-arm-kernel at vger.kernel.org.

Re: [PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/12/2015 08:32 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.
> 
> [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
> [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
> 
> Signed-off-by: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
> Signed-off-by: Craig McGeachie <slapdau-/E1597aS9LT0CCvOHzKKcA@public.gmane.org>
> Signed-off-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> Cc: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Acks often don't carry over when there are significant changes.

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

> +#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) & 0xf))
> +#define MBOX_CHAN(msg)			((msg) & 0xf)
> +#define MBOX_DATA28(msg)		((msg) & ~0xf)

Even the concept of storing channel IDs in the LSBs feels like it might
be RPi-firmware-specific rather than HW-specific?

> +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_peripheral_read_workaround();
> +
> +	while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
> +		u32 msg = readl(mbox->regs + MAIL0_RD);
> +		unsigned int chan = MBOX_CHAN(msg);
> +		u32 data = MBOX_DATA28(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);

I think for complete safety, you'd need to put the peripheral workaround
call here too. Consider what happens if some module registers to receive
mailbox responses, then e.g. changes a GPIO right in the callback. Are
we going to add the workaround call to the bcm2835 GPIO, SDHCI, and USB
drivers too? If so, that might obviate the need to perform the
workaround here.

Either way though, wouldn't we need to at least move the workaround
that's already present in this function so that it happens before the
readl() inside the while() above every time, to handle the switch from
any HW access inside mbox_chan_received_data() to the HW access to the
mbox module here?

> +		mbox_chan_received_data(mbox->channel[chan].link, &data);
> +	}
> +	return IRQ_HANDLED;
> +}

> +static int bcm2835_mbox_probe(struct platform_device *pdev)
...
> +	/* Enable the interrupt on data reception */
> +	writel(ARM_MC_IHAVEDATAIRQEN, mbox->regs + MAIL0_CNF);

I think that bcm2835_mbox_remove() needs to undo that, so that it
guarantees the IRQ handler won't be called after the function returns,
but before the devm IRQ unregister occurs.
--
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 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/12/2015 08:32 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.
> 
> [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
> [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Cc: Jassi Brar <jassisinghbrar@gmail.com>
> Acked-by: Lee Jones <lee.jones@linaro.org>

Acks often don't carry over when there are significant changes.

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

> +#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) & 0xf))
> +#define MBOX_CHAN(msg)			((msg) & 0xf)
> +#define MBOX_DATA28(msg)		((msg) & ~0xf)

Even the concept of storing channel IDs in the LSBs feels like it might
be RPi-firmware-specific rather than HW-specific?

> +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_peripheral_read_workaround();
> +
> +	while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
> +		u32 msg = readl(mbox->regs + MAIL0_RD);
> +		unsigned int chan = MBOX_CHAN(msg);
> +		u32 data = MBOX_DATA28(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);

I think for complete safety, you'd need to put the peripheral workaround
call here too. Consider what happens if some module registers to receive
mailbox responses, then e.g. changes a GPIO right in the callback. Are
we going to add the workaround call to the bcm2835 GPIO, SDHCI, and USB
drivers too? If so, that might obviate the need to perform the
workaround here.

Either way though, wouldn't we need to at least move the workaround
that's already present in this function so that it happens before the
readl() inside the while() above every time, to handle the switch from
any HW access inside mbox_chan_received_data() to the HW access to the
mbox module here?

> +		mbox_chan_received_data(mbox->channel[chan].link, &data);
> +	}
> +	return IRQ_HANDLED;
> +}

> +static int bcm2835_mbox_probe(struct platform_device *pdev)
...
> +	/* Enable the interrupt on data reception */
> +	writel(ARM_MC_IHAVEDATAIRQEN, mbox->regs + MAIL0_CNF);

I think that bcm2835_mbox_remove() needs to undo that, so that it
guarantees the IRQ handler won't be called after the function returns,
but before the devm IRQ unregister occurs.

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

[Stephen Warren]

On 03/12/2015 08:32 PM, Eric Anholt wrote:
> Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Patches 2, 4,
Acked-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>

--
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 4/4] ARM: bcm2835: Add the mailbox to the device tree.

[Stephen Warren]

On 03/12/2015 08:32 PM, Eric Anholt wrote:
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Acked-by: Lee Jones <lee.jones@linaro.org>

Patches 2, 4,
Acked-by: Stephen Warren <swarren@wwwdotorg.org>

Re: [PATCH 2/4 v3] dt/bindings: Add binding for BCM2835 mailbox driver

[Lee Jones]

On Thu, 12 Mar 2015, Eric Anholt wrote:

> From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
> 
> 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>
> ---
> 
> 
> v2: Split into a separate patch for submitting to the devicetree list.
>     Consistently start node docs with a capital letter. device's
>     address in the example shouldn't have "0x". Drop machine-specific
>     interrupt numbers from the docs.  (changes by anholt).
> 
> v3: Move the file to just bcm2835-mbox.txt, clean up formatting
>     (changes by anholt, from review by Lee Jones).

Thanks for fixing up.

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

I don't see any unruliness or causes of controversy.

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

> diff --git a/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt
> new file mode 100644
> index 0000000..0bb2b9d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/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>;
> +};
--
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 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Lee Jones]

On Mon, 16 Mar 2015, Stephen Warren wrote:

> On 03/12/2015 08:32 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.
> > 
> > [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
> > [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
> > Signed-off-by: Craig McGeachie <slapdau-/E1597aS9LT0CCvOHzKKcA@public.gmane.org>
> > Signed-off-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
> > Signed-off-by: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> > Cc: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> Acks often don't carry over when there are significant changes.

Did I even Ack this?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Lee Jones]

On Mon, 16 Mar 2015, Stephen Warren wrote:

> On 03/12/2015 08:32 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.
> > 
> > [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
> > [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
> > Signed-off-by: Suman Anna <s-anna@ti.com>
> > Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com>
> > Signed-off-by: Eric Anholt <eric@anholt.net>
> > Cc: Jassi Brar <jassisinghbrar@gmail.com>
> > Acked-by: Lee Jones <lee.jones@linaro.org>
> 
> Acks often don't carry over when there are significant changes.

Did I even Ack this?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

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

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

> On Mon, 16 Mar 2015, Stephen Warren wrote:
>
>> On 03/12/2015 08:32 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.
>> > 
>> > [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
>> > [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
>> > 
>> > Signed-off-by: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
>> > Signed-off-by: Craig McGeachie <slapdau-/E1597aS9LT0CCvOHzKKcA@public.gmane.org>
>> > Signed-off-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
>> > Signed-off-by: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> > Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
>> > Cc: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> > Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> 
>> Acks often don't carry over when there are significant changes.
>
> Did I even Ack this?

Yeah, I don't see any record of it, looks like I misread the mail
thread.

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

[PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

Lee Jones <lee.jones@linaro.org> writes:

> On Mon, 16 Mar 2015, Stephen Warren wrote:
>
>> On 03/12/2015 08:32 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.
>> > 
>> > [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
>> > [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
>> > 
>> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>> > Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
>> > Signed-off-by: Suman Anna <s-anna@ti.com>
>> > Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com>
>> > Signed-off-by: Eric Anholt <eric@anholt.net>
>> > Cc: Jassi Brar <jassisinghbrar@gmail.com>
>> > Acked-by: Lee Jones <lee.jones@linaro.org>
>> 
>> Acks often don't carry over when there are significant changes.
>
> Did I even Ack this?

Yeah, I don't see any record of it, looks like I misread the mail
thread.
-------------- 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/20150317/68198f4c/attachment.sig>

Re: [PATCH 1/4] ARM: BCM2835: Add a function for doing an rmb() between device reads.

[Eric Anholt]

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

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

> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>> Stephen Warren was concerned that the rmb() present in the new mailbox
>> driver was unnecessary, and after seeing the docs, that it was just so
>> surprising that somebody would come along and remove it later.  The
>> explanation for the need for the rmb() is long enough that we won't
>> want to place it at every callsite.  Make a wrapper with the whole
>> explanation in it, so that anyone wondering what's going on sees the
>> docs right there.
>
>> diff --git a/include/soc/bcm2835/peripheral-workaround.h b/include/soc/bcm2835/peripheral-workaround.h
>
>> +static inline void bcm2835_peripheral_read_workaround(void)
>> +{
>> +#ifdef CONFIG_ARCH_BCM2835
>
> Would this header be included if that wasn't defined? Perhaps that'll be
> answered by a later patch...

Well, we may find we need workaround rmb()s in, say, dwc2.  I don't
think we'd want to have them unconditional on other architectures when
bcm2835 isn't included in the build.

>> +	/*
>> +	 * The BCM2835 bus is unusual in that it doesn't guarantee
>> +	 * ordering between reads from different peripherals (where
>> +	 * peripherals roughly correspond to Linux devices).  From
>> +	 * BCM2835 ARM Peripherals.pdf, page 7:
>
> Many buses don't guarantee ordering; that's quite common. The issue is
> that the CPU then doesn't match up the correct read request and
> response, thus causing it to swap the results of read requests. That's
> the unusual part. It would be useful to spell that out more explicitly
> in this introduction, even though it is called out in the example below.
>
> BTW, the ARM mailing list is linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org not
> linux-arm-kernel-u79uwXL29TaiAVqoAR/hOA@public.gmane.org

Fixed in my send-email script, thanks!

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

[PATCH 1/4] ARM: BCM2835: Add a function for doing an rmb() between device reads.

[Eric Anholt]

Stephen Warren <swarren@wwwdotorg.org> writes:

> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>> Stephen Warren was concerned that the rmb() present in the new mailbox
>> driver was unnecessary, and after seeing the docs, that it was just so
>> surprising that somebody would come along and remove it later.  The
>> explanation for the need for the rmb() is long enough that we won't
>> want to place it at every callsite.  Make a wrapper with the whole
>> explanation in it, so that anyone wondering what's going on sees the
>> docs right there.
>
>> diff --git a/include/soc/bcm2835/peripheral-workaround.h b/include/soc/bcm2835/peripheral-workaround.h
>
>> +static inline void bcm2835_peripheral_read_workaround(void)
>> +{
>> +#ifdef CONFIG_ARCH_BCM2835
>
> Would this header be included if that wasn't defined? Perhaps that'll be
> answered by a later patch...

Well, we may find we need workaround rmb()s in, say, dwc2.  I don't
think we'd want to have them unconditional on other architectures when
bcm2835 isn't included in the build.

>> +	/*
>> +	 * The BCM2835 bus is unusual in that it doesn't guarantee
>> +	 * ordering between reads from different peripherals (where
>> +	 * peripherals roughly correspond to Linux devices).  From
>> +	 * BCM2835 ARM Peripherals.pdf, page 7:
>
> Many buses don't guarantee ordering; that's quite common. The issue is
> that the CPU then doesn't match up the correct read request and
> response, thus causing it to swap the results of read requests. That's
> the unusual part. It would be useful to spell that out more explicitly
> in this introduction, even though it is called out in the example below.
>
> BTW, the ARM mailing list is linux-arm-kernel at lists.infradead.org not
> linux-arm-kernel at vger.kernel.org.

Fixed in my send-email script, thanks!
-------------- 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/20150317/34fcb781/attachment-0001.sig>

Re: [PATCH 2/4 v3] dt/bindings: Add binding for BCM2835 mailbox driver

[Scott Branden]

Hi Lubomir,


On 15-03-17 10:27 AM, Lee Jones wrote:
> On Thu, 12 Mar 2015, Eric Anholt wrote:
>
>> From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
>>
>> 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>
>> ---
>>
>>
>> v2: Split into a separate patch for submitting to the devicetree list.
>>      Consistently start node docs with a capital letter. device's
>>      address in the example shouldn't have "0x". Drop machine-specific
>>      interrupt numbers from the docs.  (changes by anholt).
>>
>> v3: Move the file to just bcm2835-mbox.txt, clean up formatting
>>      (changes by anholt, from review by Lee Jones).
>
> Thanks for fixing up.
>
>> .../devicetree/bindings/mailbox/bcm2835-mbox.txt      | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt
>
> I don't see any unruliness or causes of controversy.
>
> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
>> diff --git a/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt
>> new file mode 100644
>> index 0000000..0bb2b9d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt
I am trying to standarize all broadcom device tree bindings in the 
format "brcm,binding.txt".

Could you please rename this file to brcm,bcm2835-mbox.txt for 
consistency in bindings?

Regards,
  Scott
--
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 2/4 v3] dt/bindings: Add binding for BCM2835 mailbox driver

[Eric Anholt]

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

Scott Branden <sbranden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> writes:

> Hi Lubomir,
>
>
> On 15-03-17 10:27 AM, Lee Jones wrote:
>> On Thu, 12 Mar 2015, Eric Anholt wrote:
>>> diff --git a/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt
>>> new file mode 100644
>>> index 0000000..0bb2b9d
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt
> I am trying to standarize all broadcom device tree bindings in the 
> format "brcm,binding.txt".
>
> Could you please rename this file to brcm,bcm2835-mbox.txt for 
> consistency in bindings?

The filename was at the request of Lee Jones, to match the convention of
the other mailbox drivers.  I don't care what the filename is as long as
people can get agreement on what it should be.

(Right now it seems to be 2:1 in favor of using the compatible string?)

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

Re: [PATCH 2/4 v3] dt/bindings: Add binding for BCM2835 mailbox driver

[Lee Jones]

On Tue, 17 Mar 2015, Scott Branden wrote:
> On 15-03-17 10:27 AM, Lee Jones wrote:
> >On Thu, 12 Mar 2015, Eric Anholt wrote:
> >
> >>From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
> >>
> >>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>
> >>---
> >>
> >>
> >>v2: Split into a separate patch for submitting to the devicetree list.
> >>     Consistently start node docs with a capital letter. device's
> >>     address in the example shouldn't have "0x". Drop machine-specific
> >>     interrupt numbers from the docs.  (changes by anholt).
> >>
> >>v3: Move the file to just bcm2835-mbox.txt, clean up formatting
> >>     (changes by anholt, from review by Lee Jones).
> >
> >Thanks for fixing up.
> >
> >>.../devicetree/bindings/mailbox/bcm2835-mbox.txt      | 19 +++++++++++++++++++
> >>  1 file changed, 19 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt
> >
> >I don't see any unruliness or causes of controversy.
> >
> >Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >
> >>diff --git a/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt
> >>new file mode 100644
> >>index 0000000..0bb2b9d
> >>--- /dev/null
> >>+++ b/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt
> I am trying to standarize all broadcom device tree bindings in the
> format "brcm,binding.txt".
> 
> Could you please rename this file to brcm,bcm2835-mbox.txt for
> consistency in bindings?

The file name is governed by the maintainer of the subsystem you're
applying documentation for.  If this were an MFD submission, I would
not accept a the format matching the compatible string for instance.
I like consistency and commas in file names creeps me out.

This however, is Jassi's call.
--
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 1/4] ARM: BCM2835: Add a function for doing an rmb() between device reads.

[Lee Jones]

On Thu, 12 Mar 2015, Eric Anholt wrote:

> Stephen Warren was concerned that the rmb() present in the new mailbox
> driver was unnecessary, and after seeing the docs, that it was just so
> surprising that somebody would come along and remove it later.  The
> explanation for the need for the rmb() is long enough that we won't
> want to place it at every callsite.  Make a wrapper with the whole
> explanation in it, so that anyone wondering what's going on sees the
> docs right there.
> 
> Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> ---
>  include/soc/bcm2835/peripheral-workaround.h | 75 +++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100644 include/soc/bcm2835/peripheral-workaround.h
> 
> diff --git a/include/soc/bcm2835/peripheral-workaround.h b/include/soc/bcm2835/peripheral-workaround.h
> new file mode 100644
> index 0000000..4541a13
> --- /dev/null
> +++ b/include/soc/bcm2835/peripheral-workaround.h
> @@ -0,0 +1,75 @@
> +/*
> + * 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.
> + */
> +
> +static inline void bcm2835_peripheral_read_workaround(void)
> +{
> +#ifdef CONFIG_ARCH_BCM2835
> +	/*
> +	 * The BCM2835 bus is unusual in that it doesn't guarantee
> +	 * ordering between reads from different peripherals (where
> +	 * peripherals roughly correspond to Linux devices).  From
> +	 * BCM2835 ARM Peripherals.pdf, page 7:
> +	 *
> +	 *     "In order to keep the system complexity low and data
> +	 *      throughput high, the BCM2835 AXI system does not
> +	 *      always return read data in-order. The GPU has special
> +	 *      logic to cope with data arriving out-of-order; however
> +	 *      the ARM core does not contain such logic. Therefore
> +	 *      some precautions must be taken when using the ARM to
> +	 *      access peripherals.
> +	 *
> +	 *      Accesses to the same peripheral will always arrive and
> +	 *      return in-order. It is only when switching from one
> +	 *      peripheral to another that data can arrive
> +	 *      out-of-order. The simplest way to make sure that data
> +	 *      is processed in-order is to place a memory barrier
> +	 *      instruction at critical positions in the code. 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."
> +	 *
> +	 * The footnote explicitly says that:
> +	 *
> +	 *      "For example:
> +	 *
> +	 *         a_status = *pointer_to_peripheral_a;
> +	 *         b_status = *pointer_to_peripheral_b;
> +	 *
> +	 *       Without precuations the values ending up in the
> +	 *       variables a_status and b_status can be swapped
> +	 *       around."
> +	 *
> +	 * However, it also notes that, somewhat contrary to the first
> +	 * bullet point:
> +	 *
> +	 *     "It is theoretical possible for writes to go ‘wrong’
> +	 *      but that is far more difficult to achieve. The AXI
> +	 *      system makes sure the data always arrives in-order at
> +	 *      its intended destination. So:
> +	 *
> +	 *        *pointer_to_peripheral_a = value_a;
> +	 *        *pointer_to_peripheral_b = value_b;
> +	 *
> +	 *      will always give the expected result. The only time
> +	 *      write data can arrive out-of-order is if two different
> +	 *      peripherals are connected to the same external
> +	 *      equipment"
> +	 *
> +	 * Since we aren't interacting with multiple peripherals
> +	 * connected to the same external equipment as far as we know,
> +	 * that means that we only need to handle the read workaround
> +	 * case.  We do so by placing an rmb() at the first device
> +	 * read acceess in a given driver path, including the
> +	 * interrupt handlers.
> +	 */
> +	rmb();
> +#endif
> +}

The format:

#ifdef CONFIG_ARCH_BCM2835
static inline void bcm2835_peripheral_read_workaround(void)
{
        <stuff>
}
#else
static inline void bcm2835_peripheral_read_workaround(void) {}
#endif

... is more traditional in the Linux kernel.
--
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 2/4 v3] dt/bindings: Add binding for BCM2835 mailbox driver

[Jassi Brar]

On Wed, Mar 18, 2015 at 1:53 PM, Lee Jones <lee-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, 17 Mar 2015, Scott Branden wrote:
>> On 15-03-17 10:27 AM, Lee Jones wrote:
>> >On Thu, 12 Mar 2015, Eric Anholt wrote:
>> >
>> >>From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
>> >>
>> >>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>
>> >>---
>> >>
>> >>
>> >>v2: Split into a separate patch for submitting to the devicetree list.
>> >>     Consistently start node docs with a capital letter. device's
>> >>     address in the example shouldn't have "0x". Drop machine-specific
>> >>     interrupt numbers from the docs.  (changes by anholt).
>> >>
>> >>v3: Move the file to just bcm2835-mbox.txt, clean up formatting
>> >>     (changes by anholt, from review by Lee Jones).
>> >
>> >Thanks for fixing up.
>> >
>> >>.../devicetree/bindings/mailbox/bcm2835-mbox.txt      | 19 +++++++++++++++++++
>> >>  1 file changed, 19 insertions(+)
>> >>  create mode 100644 Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt
>> >
>> >I don't see any unruliness or causes of controversy.
>> >
>> >Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> >
>> >>diff --git a/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt
>> >>new file mode 100644
>> >>index 0000000..0bb2b9d
>> >>--- /dev/null
>> >>+++ b/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt
>> I am trying to standarize all broadcom device tree bindings in the
>> format "brcm,binding.txt".
>>
>> Could you please rename this file to brcm,bcm2835-mbox.txt for
>> consistency in bindings?
>
> The file name is governed by the maintainer of the subsystem you're
> applying documentation for.  If this were an MFD submission, I would
> not accept a the format matching the compatible string for instance.
> I like consistency and commas in file names creeps me out.
>
> This however, is Jassi's call.
>
I don't really have a strong opinion here. I tend to respect Broadcom
platforms' convention of using "brcm,binding.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 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Lee Jones]

On Thu, 12 Mar 2015, 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.
> 
> [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
> [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
> 
> Signed-off-by: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
> Signed-off-by: Craig McGeachie <slapdau-/E1597aS9LT0CCvOHzKKcA@public.gmane.org>
> Signed-off-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> Cc: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> 
> 
> v2: Squashed Craig's work for review, carried over to new version of
>     Mailbox framework (changes by Lubomir)
> 
> v3: Fix multi-line comment style.  Refer to the documentation by
>     filename.  Only declare one MODULE_AUTHOR.  Alphabetize includes.
>     Drop some excessive dev_dbg()s (changes by anholt).
> 
> v4: Use the new bcm2835_peripheral_read_workaround(), drop the

Can you explain to me why this is required (and don't just point me in
the direction of the other patch ;) ).  You appear to be using the
non-relaxed variants of readl and writel, which already do memory
barriers, so I'm a little perplexed as to how the problem can arise.

>     unnecessary wmb()s, make the messages be a pointer to u32, rather
>     than u32-cast-as-pointer, fold in small static functions, drop
>     extra error messages, clean up sizeof() arg for malloc, disable
>     interrupts on unload.
> 
>  drivers/mailbox/Kconfig           |   8 ++
>  drivers/mailbox/Makefile          |   2 +
>  drivers/mailbox/bcm2835-mailbox.c | 259 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 269 insertions(+)
>  create mode 100644 drivers/mailbox/bcm2835-mailbox.c
--
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 2/4 v3] dt/bindings: Add binding for BCM2835 mailbox driver

[Lee Jones]

On Wed, 18 Mar 2015, Jassi Brar wrote:
> On Wed, Mar 18, 2015 at 1:53 PM, Lee Jones <lee-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Tue, 17 Mar 2015, Scott Branden wrote:
> >> On 15-03-17 10:27 AM, Lee Jones wrote:
> >> >On Thu, 12 Mar 2015, Eric Anholt wrote:
> >> >
> >> >>From: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
> >> >>
> >> >>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>
> >> >>---
> >> >>
> >> >>
> >> >>v2: Split into a separate patch for submitting to the devicetree list.
> >> >>     Consistently start node docs with a capital letter. device's
> >> >>     address in the example shouldn't have "0x". Drop machine-specific
> >> >>     interrupt numbers from the docs.  (changes by anholt).
> >> >>
> >> >>v3: Move the file to just bcm2835-mbox.txt, clean up formatting
> >> >>     (changes by anholt, from review by Lee Jones).
> >> >
> >> >Thanks for fixing up.
> >> >
> >> >>.../devicetree/bindings/mailbox/bcm2835-mbox.txt      | 19 +++++++++++++++++++
> >> >>  1 file changed, 19 insertions(+)
> >> >>  create mode 100644 Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt
> >> >
> >> >I don't see any unruliness or causes of controversy.
> >> >
> >> >Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> >
> >> >>diff --git a/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt
> >> >>new file mode 100644
> >> >>index 0000000..0bb2b9d
> >> >>--- /dev/null
> >> >>+++ b/Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt
> >> I am trying to standarize all broadcom device tree bindings in the
> >> format "brcm,binding.txt".
> >>
> >> Could you please rename this file to brcm,bcm2835-mbox.txt for
> >> consistency in bindings?
> >
> > The file name is governed by the maintainer of the subsystem you're
> > applying documentation for.  If this were an MFD submission, I would
> > not accept a the format matching the compatible string for instance.
> > I like consistency and commas in file names creeps me out.
> >
> > This however, is Jassi's call.
> >
> I don't really have a strong opinion here. I tend to respect Broadcom
> platforms' convention of using "brcm,binding.txt"

I'm fine with this Eric.

If you change it back my Ack can reside.
--
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 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

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

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

> On Thu, 12 Mar 2015, 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.
>> 
>> [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
>> [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
>> 
>> Signed-off-by: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
>> Signed-off-by: Craig McGeachie <slapdau-/E1597aS9LT0CCvOHzKKcA@public.gmane.org>
>> Signed-off-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
>> Signed-off-by: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
>> Cc: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>> 
>> 
>> v2: Squashed Craig's work for review, carried over to new version of
>>     Mailbox framework (changes by Lubomir)
>> 
>> v3: Fix multi-line comment style.  Refer to the documentation by
>>     filename.  Only declare one MODULE_AUTHOR.  Alphabetize includes.
>>     Drop some excessive dev_dbg()s (changes by anholt).
>> 
>> v4: Use the new bcm2835_peripheral_read_workaround(), drop the
>
> Can you explain to me why this is required (and don't just point me in
> the direction of the other patch ;) ).  You appear to be using the
> non-relaxed variants of readl and writel, which already do memory
> barriers, so I'm a little perplexed as to how the problem can arise.

Hmm.

A shorter restatement of the architecture requirement would be, I think,
"Don't let there be two outstanding reads of different peripherals on
the AXI bus, or the CPU might mis-assign the read results.  Use rmb() to
wait for the previous bus reads when you need to prevent this"

arch/arm/include/asm/io.h's readl() does __iormb() after each
__raw_readl().  Imagine taking an interrupt for a new peripheral between
the driver's __raw_readl() and the following __iormb(): Now you've got
two __raw_readl()s in between iormb()s and you can theoretically get
unordered reads.

We could hope that the architecture IRQ handler would happen to do an
incidental rmb(), resolving the need to protect from interrupt handling
inside of device drivers.  The interrupt controller's presence at
0x7e00b200 sounds like it's an AXI peripheral, so it would need to be
ensuring ordering of reads.  However, it's doing readl_relaxed()s.  So
my rmb() at the start of my irq handler is silly -- if somebody got
interrupted between readl and rmb, we've already had a chance to get the
wrong result inside of the IRQ chip's status read.

My new idea for handling this would be to:

1) Assume drivers don't exit with reads outstanding.  This means they
don't do a readl_relaxed() from an AXI peripheral at the end of a path
without doing something with the result.

2) Make bcm2835_handle_irq() do this rmb() at the top, with the big
explanation, to avoid a race against the interrupted code device being
inside a readl() before the __iormb().  We don't worry about the 1-2
readl_relaxed()s inside of bcm2835_handle_irq(), because their return
values get waited on before continuing on to calling the device driver,
so the device driver knows its IRQ handler is being entered with no AXI
reads outstanding.

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

Re: [PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

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

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

> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
>
>> +#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) & 0xf))
>> +#define MBOX_CHAN(msg)			((msg) & 0xf)
>> +#define MBOX_DATA28(msg)		((msg) & ~0xf)
>
> Even the concept of storing channel IDs in the LSBs feels like it might
> be RPi-firmware-specific rather than HW-specific?

I guess?  If we found another firmware protocol, we could have that
device's dt just specify a different compatible string.  But in the
absence of another firmware to talk to, I'm not sure what you want here.

Note that Roku's kernel code dump doesn't even communicate through the
mailbox.  vcio.c exists, but is disconnected from the build.

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

[PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

Stephen Warren <swarren@wwwdotorg.org> writes:

> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
>
>> +#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) & 0xf))
>> +#define MBOX_CHAN(msg)			((msg) & 0xf)
>> +#define MBOX_DATA28(msg)		((msg) & ~0xf)
>
> Even the concept of storing channel IDs in the LSBs feels like it might
> be RPi-firmware-specific rather than HW-specific?

I guess?  If we found another firmware protocol, we could have that
device's dt just specify a different compatible string.  But in the
absence of another firmware to talk to, I'm not sure what you want here.

Note that Roku's kernel code dump doesn't even communicate through the
mailbox.  vcio.c exists, but is disconnected from the build.
-------------- 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/20150318/40d8a5ac/attachment.sig>

Re: [PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Lee Jones]

On Wed, 18 Mar 2015, Eric Anholt wrote:

> Lee Jones <lee-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> writes:
> 
> > On Thu, 12 Mar 2015, 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.
> >> 
> >> [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
> >> [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
> >> 
> >> Signed-off-by: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
> >> Signed-off-by: Craig McGeachie <slapdau-/E1597aS9LT0CCvOHzKKcA@public.gmane.org>
> >> Signed-off-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
> >> Signed-off-by: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> >> Cc: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> ---
> >> 
> >> 
> >> v2: Squashed Craig's work for review, carried over to new version of
> >>     Mailbox framework (changes by Lubomir)
> >> 
> >> v3: Fix multi-line comment style.  Refer to the documentation by
> >>     filename.  Only declare one MODULE_AUTHOR.  Alphabetize includes.
> >>     Drop some excessive dev_dbg()s (changes by anholt).
> >> 
> >> v4: Use the new bcm2835_peripheral_read_workaround(), drop the
> >
> > Can you explain to me why this is required (and don't just point me in
> > the direction of the other patch ;) ).  You appear to be using the
> > non-relaxed variants of readl and writel, which already do memory
> > barriers, so I'm a little perplexed as to how the problem can arise.
> 
> Hmm.
> 
> A shorter restatement of the architecture requirement would be, I think,
> "Don't let there be two outstanding reads of different peripherals on
> the AXI bus, or the CPU might mis-assign the read results.  Use rmb() to
> wait for the previous bus reads when you need to prevent this"
> 
> arch/arm/include/asm/io.h's readl() does __iormb() after each
> __raw_readl().  Imagine taking an interrupt for a new peripheral between
> the driver's __raw_readl() and the following __iormb(): Now you've got
> two __raw_readl()s in between iormb()s and you can theoretically get
> unordered reads.
> 
> We could hope that the architecture IRQ handler would happen to do an
> incidental rmb(), resolving the need to protect from interrupt handling
> inside of device drivers.  The interrupt controller's presence at
> 0x7e00b200 sounds like it's an AXI peripheral, so it would need to be
> ensuring ordering of reads.  However, it's doing readl_relaxed()s.  So
> my rmb() at the start of my irq handler is silly -- if somebody got
> interrupted between readl and rmb, we've already had a chance to get the
> wrong result inside of the IRQ chip's status read.
> 
> My new idea for handling this would be to:
> 
> 1) Assume drivers don't exit with reads outstanding.  This means they
> don't do a readl_relaxed() from an AXI peripheral at the end of a path
> without doing something with the result.
> 
> 2) Make bcm2835_handle_irq() do this rmb() at the top, with the big
> explanation, to avoid a race against the interrupted code device being
> inside a readl() before the __iormb().  We don't worry about the 1-2
> readl_relaxed()s inside of bcm2835_handle_irq(), because their return
> values get waited on before continuing on to calling the device driver,
> so the device driver knows its IRQ handler is being entered with no AXI
> reads outstanding.

That's a fantastic explanation.  Thanks for taking the time to
write this out so diligently.

Doing this at a sub-arch level sounds a little wrong to me.  I don't
think Broadcom are the only vendor who do not ensure correct read
order, and writing <vendor>_peripheral_read_workarounds() all over the
place sounds less than graceful.  Granted, if there were a greater
need and this can't be fixed another way we could knock off the
<vendor>_ part and make the call generic, but is there no way we can
deal with this at the architecture level?

The whole point of doing readl_relaxed()s is that you can be assured
that the architecture guarantee ordering.  If that's not the case,
then we need to be using readl()s in the IRQ handler instead. 
--
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 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/18/2015 04:39 PM, Eric Anholt wrote:
> Lee Jones <lee-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> writes:
> 
>> On Thu, 12 Mar 2015, 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.
>>> 
>>> [1]
>>> http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
>>>
>>> 
[2]
http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
>>> 
>>> Signed-off-by: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org> Signed-off-by:
>>> Craig McGeachie <slapdau-/E1597aS9LT0CCvOHzKKcA@public.gmane.org> Signed-off-by: Suman
>>> Anna <s-anna-l0cyMroinI0@public.gmane.org> Signed-off-by: Jassi Brar
>>> <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Signed-off-by: Eric Anholt
>>> <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> Cc: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 
>>> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> ---
>>> 
>>> 
>>> v2: Squashed Craig's work for review, carried over to new
>>> version of Mailbox framework (changes by Lubomir)
>>> 
>>> v3: Fix multi-line comment style.  Refer to the documentation
>>> by filename.  Only declare one MODULE_AUTHOR.  Alphabetize
>>> includes. Drop some excessive dev_dbg()s (changes by anholt).
>>> 
>>> v4: Use the new bcm2835_peripheral_read_workaround(), drop the
>> 
>> Can you explain to me why this is required (and don't just point
>> me in the direction of the other patch ;) ).  You appear to be
>> using the non-relaxed variants of readl and writel, which already
>> do memory barriers, so I'm a little perplexed as to how the
>> problem can arise.
> 
> Hmm.
> 
> A shorter restatement of the architecture requirement would be, I
> think, "Don't let there be two outstanding reads of different
> peripherals on the AXI bus, or the CPU might mis-assign the read
> results.  Use rmb() to wait for the previous bus reads when you
> need to prevent this"
> 
> arch/arm/include/asm/io.h's readl() does __iormb() after each 
> __raw_readl().  Imagine taking an interrupt for a new peripheral
> between the driver's __raw_readl() and the following __iormb(): Now
> you've got two __raw_readl()s in between iormb()s and you can
> theoretically get unordered reads.
> 
> We could hope that the architecture IRQ handler would happen to do
> an incidental rmb(), resolving the need to protect from interrupt
> handling inside of device drivers.  The interrupt controller's
> presence at 0x7e00b200 sounds like it's an AXI peripheral, so it
> would need to be ensuring ordering of reads.  However, it's doing
> readl_relaxed()s.  So my rmb() at the start of my irq handler is
> silly -- if somebody got interrupted between readl and rmb, we've
> already had a chance to get the wrong result inside of the IRQ
> chip's status read.
> 
> My new idea for handling this would be to:
> 
> 1) Assume drivers don't exit with reads outstanding.  This means
> they don't do a readl_relaxed() from an AXI peripheral at the end
> of a path without doing something with the result.
> 
> 2) Make bcm2835_handle_irq() do this rmb() at the top, with the
> big explanation, to avoid a race against the interrupted code
> device being inside a readl() before the __iormb().  We don't worry
> about the 1-2 readl_relaxed()s inside of bcm2835_handle_irq(),
> because their return values get waited on before continuing on to
> calling the device driver, so the device driver knows its IRQ
> handler is being entered with no AXI reads outstanding.

I /think/ that sounds reasonable. I suppose if we do find any code
that initiates a read but doesn't use the result before returning or
calling into some other code, we can always patch that up with an
extra barrier at that time.
--
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 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/18/2015 04:39 PM, Eric Anholt wrote:
> Lee Jones <lee@kernel.org> writes:
> 
>> On Thu, 12 Mar 2015, 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.
>>> 
>>> [1]
>>> http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
>>>
>>> 
[2]
http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
>>> 
>>> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> Signed-off-by:
>>> Craig McGeachie <slapdau@yahoo.com.au> Signed-off-by: Suman
>>> Anna <s-anna@ti.com> Signed-off-by: Jassi Brar
>>> <jassisinghbrar@gmail.com> Signed-off-by: Eric Anholt
>>> <eric@anholt.net> Cc: Jassi Brar <jassisinghbrar@gmail.com> 
>>> Acked-by: Lee Jones <lee.jones@linaro.org> ---
>>> 
>>> 
>>> v2: Squashed Craig's work for review, carried over to new
>>> version of Mailbox framework (changes by Lubomir)
>>> 
>>> v3: Fix multi-line comment style.  Refer to the documentation
>>> by filename.  Only declare one MODULE_AUTHOR.  Alphabetize
>>> includes. Drop some excessive dev_dbg()s (changes by anholt).
>>> 
>>> v4: Use the new bcm2835_peripheral_read_workaround(), drop the
>> 
>> Can you explain to me why this is required (and don't just point
>> me in the direction of the other patch ;) ).  You appear to be
>> using the non-relaxed variants of readl and writel, which already
>> do memory barriers, so I'm a little perplexed as to how the
>> problem can arise.
> 
> Hmm.
> 
> A shorter restatement of the architecture requirement would be, I
> think, "Don't let there be two outstanding reads of different
> peripherals on the AXI bus, or the CPU might mis-assign the read
> results.  Use rmb() to wait for the previous bus reads when you
> need to prevent this"
> 
> arch/arm/include/asm/io.h's readl() does __iormb() after each 
> __raw_readl().  Imagine taking an interrupt for a new peripheral
> between the driver's __raw_readl() and the following __iormb(): Now
> you've got two __raw_readl()s in between iormb()s and you can
> theoretically get unordered reads.
> 
> We could hope that the architecture IRQ handler would happen to do
> an incidental rmb(), resolving the need to protect from interrupt
> handling inside of device drivers.  The interrupt controller's
> presence at 0x7e00b200 sounds like it's an AXI peripheral, so it
> would need to be ensuring ordering of reads.  However, it's doing
> readl_relaxed()s.  So my rmb() at the start of my irq handler is
> silly -- if somebody got interrupted between readl and rmb, we've
> already had a chance to get the wrong result inside of the IRQ
> chip's status read.
> 
> My new idea for handling this would be to:
> 
> 1) Assume drivers don't exit with reads outstanding.  This means
> they don't do a readl_relaxed() from an AXI peripheral at the end
> of a path without doing something with the result.
> 
> 2) Make bcm2835_handle_irq() do this rmb() at the top, with the
> big explanation, to avoid a race against the interrupted code
> device being inside a readl() before the __iormb().  We don't worry
> about the 1-2 readl_relaxed()s inside of bcm2835_handle_irq(),
> because their return values get waited on before continuing on to
> calling the device driver, so the device driver knows its IRQ
> handler is being entered with no AXI reads outstanding.

I /think/ that sounds reasonable. I suppose if we do find any code
that initiates a read but doesn't use the result before returning or
calling into some other code, we can always patch that up with an
extra barrier at that time.

Re: [PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/19/2015 01:58 AM, Lee Jones wrote:
> On Wed, 18 Mar 2015, Eric Anholt wrote:
> 
>> Lee Jones <lee-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> writes:
>>
>>> On Thu, 12 Mar 2015, 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.
>>>>
>>>> [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
>>>> [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
>>>>
>>>> Signed-off-by: Lubomir Rintel <lkundrak-NGH9Lh4a5iE@public.gmane.org>
>>>> Signed-off-by: Craig McGeachie <slapdau-/E1597aS9LT0CCvOHzKKcA@public.gmane.org>
>>>> Signed-off-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
>>>> Signed-off-by: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
>>>> Cc: Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>> ---
>>>>
>>>>
>>>> v2: Squashed Craig's work for review, carried over to new version of
>>>>     Mailbox framework (changes by Lubomir)
>>>>
>>>> v3: Fix multi-line comment style.  Refer to the documentation by
>>>>     filename.  Only declare one MODULE_AUTHOR.  Alphabetize includes.
>>>>     Drop some excessive dev_dbg()s (changes by anholt).
>>>>
>>>> v4: Use the new bcm2835_peripheral_read_workaround(), drop the
>>>
>>> Can you explain to me why this is required (and don't just point me in
>>> the direction of the other patch ;) ).  You appear to be using the
>>> non-relaxed variants of readl and writel, which already do memory
>>> barriers, so I'm a little perplexed as to how the problem can arise.
>>
>> Hmm.
>>
>> A shorter restatement of the architecture requirement would be, I think,
>> "Don't let there be two outstanding reads of different peripherals on
>> the AXI bus, or the CPU might mis-assign the read results.  Use rmb() to
>> wait for the previous bus reads when you need to prevent this"
>>
>> arch/arm/include/asm/io.h's readl() does __iormb() after each
>> __raw_readl().  Imagine taking an interrupt for a new peripheral between
>> the driver's __raw_readl() and the following __iormb(): Now you've got
>> two __raw_readl()s in between iormb()s and you can theoretically get
>> unordered reads.
>>
>> We could hope that the architecture IRQ handler would happen to do an
>> incidental rmb(), resolving the need to protect from interrupt handling
>> inside of device drivers.  The interrupt controller's presence at
>> 0x7e00b200 sounds like it's an AXI peripheral, so it would need to be
>> ensuring ordering of reads.  However, it's doing readl_relaxed()s.  So
>> my rmb() at the start of my irq handler is silly -- if somebody got
>> interrupted between readl and rmb, we've already had a chance to get the
>> wrong result inside of the IRQ chip's status read.
>>
>> My new idea for handling this would be to:
>>
>> 1) Assume drivers don't exit with reads outstanding.  This means they
>> don't do a readl_relaxed() from an AXI peripheral at the end of a path
>> without doing something with the result.
>>
>> 2) Make bcm2835_handle_irq() do this rmb() at the top, with the big
>> explanation, to avoid a race against the interrupted code device being
>> inside a readl() before the __iormb().  We don't worry about the 1-2
>> readl_relaxed()s inside of bcm2835_handle_irq(), because their return
>> values get waited on before continuing on to calling the device driver,
>> so the device driver knows its IRQ handler is being entered with no AXI
>> reads outstanding.
> 
> That's a fantastic explanation.  Thanks for taking the time to
> write this out so diligently.
> 
> Doing this at a sub-arch level sounds a little wrong to me.  I don't
> think Broadcom are the only vendor who do not ensure correct read
> order,

It seems like quite a surprising bug to me, and AFAIK there's nothing
already upstream that requires a similar WAR.

> and writing <vendor>_peripheral_read_workarounds() all over the
> place sounds less than graceful. 

It sounds like Eric's latest plan is to put the one WAR into the
bcm2835-specific IRQ controller driver. Assuming we don't find any
unusual code elsewhere, we shouldn't need to put WARs all over the place.

> Granted, if there were a greater
> need and this can't be fixed another way we could knock off the
> <vendor>_ part and make the call generic, but is there no way we can
> deal with this at the architecture level?
> 
> The whole point of doing readl_relaxed()s is that you can be assured
> that the architecture guarantee ordering.  If that's not the case,
> then we need to be using readl()s in the IRQ handler instead. 

Doing a single dsb at the start of the IRQ handler should be enough,
assuming that the code consumes each read result in a control path
before issuing another readl_relaxed, there will never be multiple reads
outstanding.
--
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 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/19/2015 01:58 AM, Lee Jones wrote:
> On Wed, 18 Mar 2015, Eric Anholt wrote:
> 
>> Lee Jones <lee@kernel.org> writes:
>>
>>> On Thu, 12 Mar 2015, 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.
>>>>
>>>> [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
>>>> [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
>>>>
>>>> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>>>> Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>>> Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com>
>>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>>> Cc: Jassi Brar <jassisinghbrar@gmail.com>
>>>> Acked-by: Lee Jones <lee.jones@linaro.org>
>>>> ---
>>>>
>>>>
>>>> v2: Squashed Craig's work for review, carried over to new version of
>>>>     Mailbox framework (changes by Lubomir)
>>>>
>>>> v3: Fix multi-line comment style.  Refer to the documentation by
>>>>     filename.  Only declare one MODULE_AUTHOR.  Alphabetize includes.
>>>>     Drop some excessive dev_dbg()s (changes by anholt).
>>>>
>>>> v4: Use the new bcm2835_peripheral_read_workaround(), drop the
>>>
>>> Can you explain to me why this is required (and don't just point me in
>>> the direction of the other patch ;) ).  You appear to be using the
>>> non-relaxed variants of readl and writel, which already do memory
>>> barriers, so I'm a little perplexed as to how the problem can arise.
>>
>> Hmm.
>>
>> A shorter restatement of the architecture requirement would be, I think,
>> "Don't let there be two outstanding reads of different peripherals on
>> the AXI bus, or the CPU might mis-assign the read results.  Use rmb() to
>> wait for the previous bus reads when you need to prevent this"
>>
>> arch/arm/include/asm/io.h's readl() does __iormb() after each
>> __raw_readl().  Imagine taking an interrupt for a new peripheral between
>> the driver's __raw_readl() and the following __iormb(): Now you've got
>> two __raw_readl()s in between iormb()s and you can theoretically get
>> unordered reads.
>>
>> We could hope that the architecture IRQ handler would happen to do an
>> incidental rmb(), resolving the need to protect from interrupt handling
>> inside of device drivers.  The interrupt controller's presence at
>> 0x7e00b200 sounds like it's an AXI peripheral, so it would need to be
>> ensuring ordering of reads.  However, it's doing readl_relaxed()s.  So
>> my rmb() at the start of my irq handler is silly -- if somebody got
>> interrupted between readl and rmb, we've already had a chance to get the
>> wrong result inside of the IRQ chip's status read.
>>
>> My new idea for handling this would be to:
>>
>> 1) Assume drivers don't exit with reads outstanding.  This means they
>> don't do a readl_relaxed() from an AXI peripheral at the end of a path
>> without doing something with the result.
>>
>> 2) Make bcm2835_handle_irq() do this rmb() at the top, with the big
>> explanation, to avoid a race against the interrupted code device being
>> inside a readl() before the __iormb().  We don't worry about the 1-2
>> readl_relaxed()s inside of bcm2835_handle_irq(), because their return
>> values get waited on before continuing on to calling the device driver,
>> so the device driver knows its IRQ handler is being entered with no AXI
>> reads outstanding.
> 
> That's a fantastic explanation.  Thanks for taking the time to
> write this out so diligently.
> 
> Doing this at a sub-arch level sounds a little wrong to me.  I don't
> think Broadcom are the only vendor who do not ensure correct read
> order,

It seems like quite a surprising bug to me, and AFAIK there's nothing
already upstream that requires a similar WAR.

> and writing <vendor>_peripheral_read_workarounds() all over the
> place sounds less than graceful. 

It sounds like Eric's latest plan is to put the one WAR into the
bcm2835-specific IRQ controller driver. Assuming we don't find any
unusual code elsewhere, we shouldn't need to put WARs all over the place.

> Granted, if there were a greater
> need and this can't be fixed another way we could knock off the
> <vendor>_ part and make the call generic, but is there no way we can
> deal with this at the architecture level?
> 
> The whole point of doing readl_relaxed()s is that you can be assured
> that the architecture guarantee ordering.  If that's not the case,
> then we need to be using readl()s in the IRQ handler instead. 

Doing a single dsb at the start of the IRQ handler should be enough,
assuming that the code consumes each read result in a control path
before issuing another readl_relaxed, there will never be multiple reads
outstanding.

Re: [PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/18/2015 05:28 PM, Eric Anholt wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:
> 
>> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>> b/drivers/mailbox/bcm2835-mailbox.c
>> 
>>> +#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) &
>>> 0xf)) +#define MBOX_CHAN(msg)			((msg) & 0xf) +#define
>>> MBOX_DATA28(msg)		((msg) & ~0xf)
>> 
>> Even the concept of storing channel IDs in the LSBs feels like it
>> might be RPi-firmware-specific rather than HW-specific?
> 
> I guess?  If we found another firmware protocol, we could have
> that device's dt just specify a different compatible string.  But
> in the absence of another firmware to talk to, I'm not sure what
> you want here.

I would expect the mailbox driver to expose a single channel that just
transports 32-bit values, since the HW doesn't impose any kind of
structure on the values it transports AFAIK. Clients of the mailbox
driver would formulate the messages they send through the mailox using
the macros above.

I'm not sure whether the mailbox core allows multiple clients for the
same mailbox channel though? This HW appears to require it.

> Note that Roku's kernel code dump doesn't even communicate through
> the mailbox.  vcio.c exists, but is disconnected from the build.

--
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 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/18/2015 05:28 PM, Eric Anholt wrote:
> Stephen Warren <swarren@wwwdotorg.org> writes:
> 
>> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>> b/drivers/mailbox/bcm2835-mailbox.c
>> 
>>> +#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) &
>>> 0xf)) +#define MBOX_CHAN(msg)			((msg) & 0xf) +#define
>>> MBOX_DATA28(msg)		((msg) & ~0xf)
>> 
>> Even the concept of storing channel IDs in the LSBs feels like it
>> might be RPi-firmware-specific rather than HW-specific?
> 
> I guess?  If we found another firmware protocol, we could have
> that device's dt just specify a different compatible string.  But
> in the absence of another firmware to talk to, I'm not sure what
> you want here.

I would expect the mailbox driver to expose a single channel that just
transports 32-bit values, since the HW doesn't impose any kind of
structure on the values it transports AFAIK. Clients of the mailbox
driver would formulate the messages they send through the mailox using
the macros above.

I'm not sure whether the mailbox core allows multiple clients for the
same mailbox channel though? This HW appears to require it.

> Note that Roku's kernel code dump doesn't even communicate through
> the mailbox.  vcio.c exists, but is disconnected from the build.

Re: [PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Jassi Brar]

On Fri, Mar 20, 2015 at 10:18 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 03/18/2015 05:28 PM, Eric Anholt wrote:
>> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:
>>
>>> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>> b/drivers/mailbox/bcm2835-mailbox.c
>>>
>>>> +#define MBOX_MSG(chan, data28)             (((data28) & ~0xf) | ((chan) &
>>>> 0xf)) +#define MBOX_CHAN(msg)                       ((msg) & 0xf) +#define
>>>> MBOX_DATA28(msg)            ((msg) & ~0xf)
>>>
>>> Even the concept of storing channel IDs in the LSBs feels like it
>>> might be RPi-firmware-specific rather than HW-specific?
>>
>> I guess?  If we found another firmware protocol, we could have
>> that device's dt just specify a different compatible string.  But
>> in the absence of another firmware to talk to, I'm not sure what
>> you want here.
>
> I would expect the mailbox driver to expose a single channel that just
> transports 32-bit values, since the HW doesn't impose any kind of
> structure on the values it transports AFAIK. Clients of the mailbox
> driver would formulate the messages they send through the mailox using
> the macros above.
>
Yes, it should be so.

> I'm not sure whether the mailbox core allows multiple clients for the
> same mailbox channel though? This HW appears to require it.
>
There were tradeoffs to granting shared vs exclusive access, we chose
latter. The platform could have a global client that serializes
requests and dispatch received data to exact destination rather than
mbox core iterating over all users of the channel. Some platform may
require to execute 2 or more commands 'atomically', which would be
messy if channels are shared.
--
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 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Jassi Brar]

On Fri, Mar 20, 2015 at 10:18 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 03/18/2015 05:28 PM, Eric Anholt wrote:
>> Stephen Warren <swarren@wwwdotorg.org> writes:
>>
>>> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>> b/drivers/mailbox/bcm2835-mailbox.c
>>>
>>>> +#define MBOX_MSG(chan, data28)             (((data28) & ~0xf) | ((chan) &
>>>> 0xf)) +#define MBOX_CHAN(msg)                       ((msg) & 0xf) +#define
>>>> MBOX_DATA28(msg)            ((msg) & ~0xf)
>>>
>>> Even the concept of storing channel IDs in the LSBs feels like it
>>> might be RPi-firmware-specific rather than HW-specific?
>>
>> I guess?  If we found another firmware protocol, we could have
>> that device's dt just specify a different compatible string.  But
>> in the absence of another firmware to talk to, I'm not sure what
>> you want here.
>
> I would expect the mailbox driver to expose a single channel that just
> transports 32-bit values, since the HW doesn't impose any kind of
> structure on the values it transports AFAIK. Clients of the mailbox
> driver would formulate the messages they send through the mailox using
> the macros above.
>
Yes, it should be so.

> I'm not sure whether the mailbox core allows multiple clients for the
> same mailbox channel though? This HW appears to require it.
>
There were tradeoffs to granting shared vs exclusive access, we chose
latter. The platform could have a global client that serializes
requests and dispatch received data to exact destination rather than
mbox core iterating over all users of the channel. Some platform may
require to execute 2 or more commands 'atomically', which would be
messy if channels are shared.

Re: [PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

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

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

> On 03/18/2015 05:28 PM, Eric Anholt wrote:
>> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:
>> 
>>> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>> b/drivers/mailbox/bcm2835-mailbox.c
>>> 
>>>> +#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) &
>>>> 0xf)) +#define MBOX_CHAN(msg)			((msg) & 0xf) +#define
>>>> MBOX_DATA28(msg)		((msg) & ~0xf)
>>> 
>>> Even the concept of storing channel IDs in the LSBs feels like it
>>> might be RPi-firmware-specific rather than HW-specific?
>> 
>> I guess?  If we found another firmware protocol, we could have
>> that device's dt just specify a different compatible string.  But
>> in the absence of another firmware to talk to, I'm not sure what
>> you want here.
>
> I would expect the mailbox driver to expose a single channel that just
> transports 32-bit values, since the HW doesn't impose any kind of
> structure on the values it transports AFAIK. Clients of the mailbox
> driver would formulate the messages they send through the mailox using
> the macros above.
>
> I'm not sure whether the mailbox core allows multiple clients for the
> same mailbox channel though? This HW appears to require it.

Yeah, that's the problem.

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

[PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

Stephen Warren <swarren@wwwdotorg.org> writes:

> On 03/18/2015 05:28 PM, Eric Anholt wrote:
>> Stephen Warren <swarren@wwwdotorg.org> writes:
>> 
>>> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>> b/drivers/mailbox/bcm2835-mailbox.c
>>> 
>>>> +#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) &
>>>> 0xf)) +#define MBOX_CHAN(msg)			((msg) & 0xf) +#define
>>>> MBOX_DATA28(msg)		((msg) & ~0xf)
>>> 
>>> Even the concept of storing channel IDs in the LSBs feels like it
>>> might be RPi-firmware-specific rather than HW-specific?
>> 
>> I guess?  If we found another firmware protocol, we could have
>> that device's dt just specify a different compatible string.  But
>> in the absence of another firmware to talk to, I'm not sure what
>> you want here.
>
> I would expect the mailbox driver to expose a single channel that just
> transports 32-bit values, since the HW doesn't impose any kind of
> structure on the values it transports AFAIK. Clients of the mailbox
> driver would formulate the messages they send through the mailox using
> the macros above.
>
> I'm not sure whether the mailbox core allows multiple clients for the
> same mailbox channel though? This HW appears to require it.

Yeah, that's the problem.
-------------- 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/20150320/bb6c83cb/attachment-0001.sig>

Re: [PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

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

Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> On Fri, Mar 20, 2015 at 10:18 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 03/18/2015 05:28 PM, Eric Anholt wrote:
>>> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:
>>>
>>>> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>>> b/drivers/mailbox/bcm2835-mailbox.c
>>>>
>>>>> +#define MBOX_MSG(chan, data28)             (((data28) & ~0xf) | ((chan) &
>>>>> 0xf)) +#define MBOX_CHAN(msg)                       ((msg) & 0xf) +#define
>>>>> MBOX_DATA28(msg)            ((msg) & ~0xf)
>>>>
>>>> Even the concept of storing channel IDs in the LSBs feels like it
>>>> might be RPi-firmware-specific rather than HW-specific?
>>>
>>> I guess?  If we found another firmware protocol, we could have
>>> that device's dt just specify a different compatible string.  But
>>> in the absence of another firmware to talk to, I'm not sure what
>>> you want here.
>>
>> I would expect the mailbox driver to expose a single channel that just
>> transports 32-bit values, since the HW doesn't impose any kind of
>> structure on the values it transports AFAIK. Clients of the mailbox
>> driver would formulate the messages they send through the mailox using
>> the macros above.
>>
> Yes, it should be so.

I'm getting pretty frustrated here.

So, if I build a global client serializing requests and stop presenting
channels, what exactly is the generic mailbox infrastructure gaining me?
I don't need add_to_rbuf.  I don't need tpoll_txdone.  I don't need
tx_tick.  I don't need peek_data.  I don't need channels.  It doesn't
even handle waiting on requests for me, and I keep having to do it in
clients.  There's nothing left that the generic code is doing for me.

If I have to do any more changes ot this driver (we're at 27 hours of
just my work on it so far...), I'd rather go back to the trivial driver
we had before that simply, obviously did what we wanted.

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

[PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

Jassi Brar <jassisinghbrar@gmail.com> writes:

> On Fri, Mar 20, 2015 at 10:18 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 03/18/2015 05:28 PM, Eric Anholt wrote:
>>> Stephen Warren <swarren@wwwdotorg.org> writes:
>>>
>>>> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>>> b/drivers/mailbox/bcm2835-mailbox.c
>>>>
>>>>> +#define MBOX_MSG(chan, data28)             (((data28) & ~0xf) | ((chan) &
>>>>> 0xf)) +#define MBOX_CHAN(msg)                       ((msg) & 0xf) +#define
>>>>> MBOX_DATA28(msg)            ((msg) & ~0xf)
>>>>
>>>> Even the concept of storing channel IDs in the LSBs feels like it
>>>> might be RPi-firmware-specific rather than HW-specific?
>>>
>>> I guess?  If we found another firmware protocol, we could have
>>> that device's dt just specify a different compatible string.  But
>>> in the absence of another firmware to talk to, I'm not sure what
>>> you want here.
>>
>> I would expect the mailbox driver to expose a single channel that just
>> transports 32-bit values, since the HW doesn't impose any kind of
>> structure on the values it transports AFAIK. Clients of the mailbox
>> driver would formulate the messages they send through the mailox using
>> the macros above.
>>
> Yes, it should be so.

I'm getting pretty frustrated here.

So, if I build a global client serializing requests and stop presenting
channels, what exactly is the generic mailbox infrastructure gaining me?
I don't need add_to_rbuf.  I don't need tpoll_txdone.  I don't need
tx_tick.  I don't need peek_data.  I don't need channels.  It doesn't
even handle waiting on requests for me, and I keep having to do it in
clients.  There's nothing left that the generic code is doing for me.

If I have to do any more changes ot this driver (we're at 27 hours of
just my work on it so far...), I'd rather go back to the trivial driver
we had before that simply, obviously did what we wanted.
-------------- 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/20150320/c18c25a5/attachment.sig>

Re: [PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/20/2015 11:24 AM, Eric Anholt wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:
>
>> On 03/18/2015 05:28 PM, Eric Anholt wrote:
>>> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> writes:
>>>
>>>> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>>> b/drivers/mailbox/bcm2835-mailbox.c
>>>>
>>>>> +#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) &
>>>>> 0xf)) +#define MBOX_CHAN(msg)			((msg) & 0xf) +#define
>>>>> MBOX_DATA28(msg)		((msg) & ~0xf)
>>>>
>>>> Even the concept of storing channel IDs in the LSBs feels like it
>>>> might be RPi-firmware-specific rather than HW-specific?
>>>
>>> I guess?  If we found another firmware protocol, we could have
>>> that device's dt just specify a different compatible string.  But
>>> in the absence of another firmware to talk to, I'm not sure what
>>> you want here.
>>
>> I would expect the mailbox driver to expose a single channel that just
>> transports 32-bit values, since the HW doesn't impose any kind of
>> structure on the values it transports AFAIK. Clients of the mailbox
>> driver would formulate the messages they send through the mailox using
>> the macros above.
>>
>> I'm not sure whether the mailbox core allows multiple clients for the
>> same mailbox channel though? This HW appears to require it.
>
> Yeah, that's the problem.

I expect you'd end up representing the low-level mbox HW and the remote 
firmware as separate nodes in DT. There's certainly precedent for 
representing firmware in DT as a separate node. Perhaps something like:

// Exports just one channel, since there's 1 channel in HW
// Driver solely transports u32s through the registers and nothing else
// Driver allows a single client on the channel
mailbox: mailbox@7e00b800 {
	compatible = "brcm,bcm2835-mbox";
	reg = <0x7e00b880 0x40>;
	interrupts = <0 1>;
	#mbox-cells = <1>;
};

// implements all RPI- (rather than bcm2835-)specific aspects of the
// firmware interface, such as merging data and channel ID into the data
// sent into the mailbox registers, handling multiple clients on a
// channel, etc.
firmware {
	compatible = "raspberrypi,firmware";
	mboxes = <&mailbox 8>;
};

The driver for the FW node could either register itself with all the 
relevant subsystems (power domains, clocks, ...) or in turn exports its 
services to other nodes like:

   firmware {
   	compatible = "raspberrypi,firmware";
   	mboxes = <&mailbox 8>;
+ 	#fw-cells = <1>; # or whatever's needed
   };

clocks {
	compatible = "raspberrypi,clocks";
	fw = <&firmware 0>; // or whatever channel # clocks are on
};
--
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 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/20/2015 11:24 AM, Eric Anholt wrote:
> Stephen Warren <swarren@wwwdotorg.org> writes:
>
>> On 03/18/2015 05:28 PM, Eric Anholt wrote:
>>> Stephen Warren <swarren@wwwdotorg.org> writes:
>>>
>>>> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>>> b/drivers/mailbox/bcm2835-mailbox.c
>>>>
>>>>> +#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) &
>>>>> 0xf)) +#define MBOX_CHAN(msg)			((msg) & 0xf) +#define
>>>>> MBOX_DATA28(msg)		((msg) & ~0xf)
>>>>
>>>> Even the concept of storing channel IDs in the LSBs feels like it
>>>> might be RPi-firmware-specific rather than HW-specific?
>>>
>>> I guess?  If we found another firmware protocol, we could have
>>> that device's dt just specify a different compatible string.  But
>>> in the absence of another firmware to talk to, I'm not sure what
>>> you want here.
>>
>> I would expect the mailbox driver to expose a single channel that just
>> transports 32-bit values, since the HW doesn't impose any kind of
>> structure on the values it transports AFAIK. Clients of the mailbox
>> driver would formulate the messages they send through the mailox using
>> the macros above.
>>
>> I'm not sure whether the mailbox core allows multiple clients for the
>> same mailbox channel though? This HW appears to require it.
>
> Yeah, that's the problem.

I expect you'd end up representing the low-level mbox HW and the remote 
firmware as separate nodes in DT. There's certainly precedent for 
representing firmware in DT as a separate node. Perhaps something like:

// Exports just one channel, since there's 1 channel in HW
// Driver solely transports u32s through the registers and nothing else
// Driver allows a single client on the channel
mailbox: mailbox at 7e00b800 {
	compatible = "brcm,bcm2835-mbox";
	reg = <0x7e00b880 0x40>;
	interrupts = <0 1>;
	#mbox-cells = <1>;
};

// implements all RPI- (rather than bcm2835-)specific aspects of the
// firmware interface, such as merging data and channel ID into the data
// sent into the mailbox registers, handling multiple clients on a
// channel, etc.
firmware {
	compatible = "raspberrypi,firmware";
	mboxes = <&mailbox 8>;
};

The driver for the FW node could either register itself with all the 
relevant subsystems (power domains, clocks, ...) or in turn exports its 
services to other nodes like:

   firmware {
   	compatible = "raspberrypi,firmware";
   	mboxes = <&mailbox 8>;
+ 	#fw-cells = <1>; # or whatever's needed
   };

clocks {
	compatible = "raspberrypi,clocks";
	fw = <&firmware 0>; // or whatever channel # clocks are on
};