Skip to contentSkip to conversation

ARM: BCM2835: Add a function for doing an rmb() between device reads.

PATCH In review Eric Anholt 8 participants 28 messages Mar 13, 2015 – Mar 20, 2015 6 messages not in the archive

Subsystems (4)
Device tree bindings Broadcom BCM2711/BCM2835 Arm architecture Broadcom BCM281XX/BCM11XXX/BCM216XX Arm architecture Mailbox API
lore
Share & tools
lore

Eric Anholt

author
#0 lore raw

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

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 <[email protected]>
---
 include/soc/bcm2835/peripheral-workaround.h | 75 +++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 include/soc/bcm2835/peripheral-workaround.h
include/soc/bcm2835/peripheral-workaround.h new file +75 -0
new file mode 100644
index 0000000..4541a13
@@ -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
+}
Signature · 6 lines
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1426213936-4139-1-git-send-email-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org

author
#2 lore raw

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

From: Lubomir Rintel <[email protected]>

Signed-off-by Lubomir Rintel <[email protected]>
Signed-off-by Craig McGeachie <slapdau-/[email protected]>
Signed-off-by Eric Anholt <[email protected]>

---

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
Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt new file +19 -0
new file mode 100644
index 0000000..0bb2b9d
@@ -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>;
+};
Signature · 6 lines
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1426213936-4139-2-git-send-email-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org

Acked-by
#4 lore raw

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

Quoted 8 lines · Eric Anholt

From: Lubomir Rintel <[email protected]>

Signed-off-by Lubomir Rintel <[email protected]>
Signed-off-by Craig McGeachie <slapdau-/[email protected]>
Signed-off-by Eric Anholt <[email protected]>

---

Quoted 7 lines

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.

Quoted 3 lines

.../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 <[email protected]>
Quoted 15 lines
Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt new file +9 -0
new file mode 100644
index 0000000..0bb2b9d
@@ -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

Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt line 19

Quoted 10 lines
Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt new file +10 -0
+		  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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw

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

Hi Lubomir,

Quoted 26 lines · Lee Jones
Quoted 8 lines · Eric Anholt

From: Lubomir Rintel <[email protected]>

Signed-off-by Lubomir Rintel <[email protected]>
Signed-off-by Craig McGeachie <slapdau-/[email protected]>
Signed-off-by Eric Anholt <[email protected]>

---

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 7 lines
Acked-by Lee Jones <[email protected]>

Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt

Quoted 5 lines
Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt new file +0 -0
new file mode 100644
index 0000000..0bb2b9d

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 5508A75A.4070203-dY08KVG/lbpWk0Htik3J/[email protected]

author
#7 lore raw

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

Quoted 13 lines · Scott Branden

Hi Lubomir,

Quoted 6 lines · Lee Jones

Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt

Quoted 5 lines · Eric Anholt
Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt new file +0 -0
new file mode 100644
index 0000000..0bb2b9d

I am trying to standarize all broadcom device tree bindings in the format "brcm,binding.txt".

Quoted 2 lines

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

#8 lore raw

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

In reply to a message not in the archive
Quoted 37 lines · Scott Branden
Quoted 26 lines · Lee Jones
Quoted 8 lines · Eric Anholt

From: Lubomir Rintel <[email protected]>

Signed-off-by Lubomir Rintel <[email protected]>
Signed-off-by Craig McGeachie <slapdau-/[email protected]>
Signed-off-by Eric Anholt <[email protected]>

---

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 7 lines
Acked-by Lee Jones <[email protected]>

Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt

Quoted 5 lines
Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt new file +0 -0
new file mode 100644
index 0000000..0bb2b9d

I am trying to standarize all broadcom device tree bindings in the format "brcm,binding.txt".

Quoted 2 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#9 lore raw

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

Quoted 41 lines · Lee Jones
Quoted 37 lines · Scott Branden
Quoted 26 lines · Lee Jones
Quoted 8 lines · Eric Anholt

From: Lubomir Rintel <[email protected]>

Signed-off-by Lubomir Rintel <[email protected]>
Signed-off-by Craig McGeachie <slapdau-/[email protected]>
Signed-off-by Eric Anholt <[email protected]>

---

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 7 lines
Acked-by Lee Jones <[email protected]>

Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt

Quoted 5 lines
Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt new file +0 -0
new file mode 100644
index 0000000..0bb2b9d

I am trying to standarize all broadcom device tree bindings in the format "brcm,binding.txt".

Quoted 2 lines

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

Quoted 7 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): CABb+yY0Gc+vJHksKc7ahYfRdh2vzj_VR_bFvjr+K+hiqiah5bg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#11 lore raw

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

Quoted 47 lines · Jassi Brar
Quoted 46 lines · Lee Jones
Quoted 37 lines · Scott Branden
Quoted 26 lines · Lee Jones
Quoted 8 lines · Eric Anholt

From: Lubomir Rintel <[email protected]>

Signed-off-by Lubomir Rintel <[email protected]>
Signed-off-by Craig McGeachie <slapdau-/[email protected]>
Signed-off-by Eric Anholt <[email protected]>

---

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 7 lines
Acked-by Lee Jones <[email protected]>

Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt

Quoted 5 lines
Documentation/devicetree/bindings/mailbox/bcm2835-mbox.txt new file +0 -0
new file mode 100644
index 0000000..0bb2b9d

I am trying to standarize all broadcom device tree bindings in the format "brcm,binding.txt".

Quoted 2 lines

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.

Quoted 4 lines
Quoted 2 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#12 lore raw

PATCH v4 3/4 mailbox: Enable BCM2835 mailbox support

In reply to a message not in the archive

From: Lubomir Rintel <[email protected]>

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 <[email protected]>
Signed-off-by Craig McGeachie <slapdau-/[email protected]>
Signed-off-by Suman Anna <[email protected]>
Signed-off-by Jassi Brar <[email protected]>
Signed-off-by Eric Anholt <[email protected]>
Cc Jassi Brar <[email protected]>
Acked-by Lee Jones <[email protected]>

---

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
drivers/mailbox/Kconfig +8 -0
index 84325f2..2873a03 100644
@@ -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
drivers/mailbox/Makefile +2 -0
index 2e79231..7feb8da 100644
@@ -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
drivers/mailbox/bcm2835-mailbox.c new file +259 -0

259 lines not on this page · raw message

Signature · 6 lines
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#13 lore raw

PATCH v4 3/4 mailbox: Enable BCM2835 mailbox support

Quoted 10 lines · Eric Anholt

From: Lubomir Rintel <[email protected]>

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

Quoted 7 lines
Signed-off-by Lubomir Rintel <[email protected]>
Signed-off-by Craig McGeachie <[email protected]>
Signed-off-by Suman Anna <[email protected]>
Signed-off-by Jassi Brar <[email protected]>
Signed-off-by Eric Anholt <[email protected]>
Cc Jassi Brar <[email protected]>
Acked-by Lee Jones <[email protected]>

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

drivers/mailbox/bcm2835-mailbox.c line 65

Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c +3 -0
 
+#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?

Quoted 7 lines
drivers/mailbox/bcm2835-mailbox.c +7 -0
+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();
+

drivers/mailbox/bcm2835-mailbox.c line 103

Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c +10 -0
+	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?

Quoted 6 lines
drivers/mailbox/bcm2835-mailbox.c +5 -0
+		mbox_chan_received_data(mbox->channel[chan].link, &data);
+	}
+	return IRQ_HANDLED;
+}
 
+static int bcm2835_mbox_probe(struct platform_device *pdev)

...

drivers/mailbox/bcm2835-mailbox.c line 225

Quoted 2 lines
drivers/mailbox/bcm2835-mailbox.c +2 -0
+	/* 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.

#14 lore raw

PATCH v4 3/4 mailbox: Enable BCM2835 mailbox support

Quoted 19 lines · Stephen Warren
Quoted 10 lines · Eric Anholt

From: Lubomir Rintel <[email protected]>

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

Quoted 7 lines
Signed-off-by Lubomir Rintel <[email protected]>
Signed-off-by Craig McGeachie <[email protected]>
Signed-off-by Suman Anna <[email protected]>
Signed-off-by Jassi Brar <[email protected]>
Signed-off-by Eric Anholt <[email protected]>
Cc Jassi Brar <[email protected]>
Acked-by Lee Jones <[email protected]>
Quoted 1 line

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

Did I even Ack this?

Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
#15 lore raw

PATCH v4 3/4 mailbox: Enable BCM2835 mailbox support

Quoted 21 lines · Lee Jones
Quoted 19 lines · Stephen Warren
Quoted 17 lines · Eric Anholt

From: Lubomir Rintel <[email protected]>

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 <[email protected]>
Signed-off-by Craig McGeachie <[email protected]>
Signed-off-by Suman Anna <[email protected]>
Signed-off-by Jassi Brar <[email protected]>
Signed-off-by Eric Anholt <[email protected]>
Cc Jassi Brar <[email protected]>
Acked-by Lee Jones <[email protected]>
Quoted 3 lines
Quoted 1 line

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>

#16 lore raw

PATCH v4 3/4 mailbox: Enable BCM2835 mailbox support

In reply to Stephen Warren
Quoted 9 lines · Stephen Warren

drivers/mailbox/bcm2835-mailbox.c line 65

Quoted 5 lines · Eric Anholt
drivers/mailbox/bcm2835-mailbox.c +3 -0
 
+#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>

#17 lore raw

PATCH v4 3/4 mailbox: Enable BCM2835 mailbox support

Quoted 10 lines · Eric Anholt
Quoted 8 lines · Stephen Warren
Quoted 6 lines · Eric Anholt
drivers/mailbox/bcm2835-mailbox.c +1 -0
 
+#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) &

0xf)) +#define MBOX_CHAN(msg) ((msg) & 0xf) +#define
MBOX_DATA28(msg) ((msg) & ~0xf)

Quoted 7 lines
Quoted 2 lines

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.

Quoted 2 lines

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

maintainer
#18 lore raw

PATCH v4 3/4 mailbox: Enable BCM2835 mailbox support

Quoted 19 lines · Stephen Warren
Quoted 10 lines · Eric Anholt
Quoted 8 lines · Stephen Warren
Quoted 6 lines · Eric Anholt
drivers/mailbox/bcm2835-mailbox.c +1 -0
 
+#define MBOX_MSG(chan, data28)             (((data28) & ~0xf) | ((chan) &

0xf)) +#define MBOX_CHAN(msg) ((msg) & 0xf) +#define
MBOX_DATA28(msg) ((msg) & ~0xf)

Quoted 7 lines
Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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.

#19 lore raw

PATCH v4 3/4 mailbox: Enable BCM2835 mailbox support

Quoted 20 lines · Jassi Brar
Quoted 19 lines · Stephen Warren
Quoted 17 lines · Eric Anholt
Quoted 10 lines · Stephen Warren
Quoted 6 lines · Eric Anholt
drivers/mailbox/bcm2835-mailbox.c +1 -0
 
+#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.

Quoted 7 lines
Quoted 6 lines

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>

#20 lore raw

PATCH v4 3/4 mailbox: Enable BCM2835 mailbox support

In reply to Stephen Warren
Quoted 19 lines · Stephen Warren
Quoted 10 lines · Eric Anholt
Quoted 8 lines · Stephen Warren
Quoted 6 lines · Eric Anholt
drivers/mailbox/bcm2835-mailbox.c +1 -0
 
+#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) &

0xf)) +#define MBOX_CHAN(msg) ((msg) & 0xf) +#define
MBOX_DATA28(msg) ((msg) & ~0xf)

Quoted 7 lines
Quoted 2 lines

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.

Quoted 8 lines

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>

#21 lore raw

PATCH v4 3/4 mailbox: Enable BCM2835 mailbox support

Quoted 27 lines · Eric Anholt
Quoted 25 lines · Stephen Warren
Quoted 10 lines · Eric Anholt
Quoted 8 lines · Stephen Warren
Quoted 6 lines · Eric Anholt
drivers/mailbox/bcm2835-mailbox.c +1 -0
 
+#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) &

0xf)) +#define MBOX_CHAN(msg) ((msg) & 0xf) +#define
MBOX_DATA28(msg) ((msg) & ~0xf)

Quoted 7 lines
Quoted 2 lines

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.

Quoted 4 lines
Quoted 2 lines

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

A message that is not in the archive (sent off-list or not yet received): 1426213936-4139-3-git-send-email-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org

#23 lore raw

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

Quoted 20 lines · Eric Anholt

From: Lubomir Rintel <[email protected]>

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 <[email protected]>
Signed-off-by Craig McGeachie <slapdau-/[email protected]>
Signed-off-by Suman Anna <[email protected]>
Signed-off-by Jassi Brar <[email protected]>
Signed-off-by Eric Anholt <[email protected]>
Cc Jassi Brar <[email protected]>
Acked-by Lee Jones <[email protected]>

---

Quoted 8 lines

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.

Quoted 10 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#24 lore raw

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

Quoted 29 lines · Lee Jones
Quoted 27 lines · Eric Anholt

From: Lubomir Rintel <[email protected]>

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 <[email protected]>
Signed-off-by Craig McGeachie <slapdau-/[email protected]>
Signed-off-by Suman Anna <[email protected]>
Signed-off-by Jassi Brar <[email protected]>
Signed-off-by Eric Anholt <[email protected]>
Cc Jassi Brar <[email protected]>
Acked-by Lee Jones <[email protected]>

---

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

Quoted 6 lines
Quoted 1 line

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.

A message that is not in the archive (sent off-list or not yet received): 87bnjqorpe.fsf-omZaPlIz5HhaEpDpdNBo/[email protected]

#26 lore raw

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

Quoted 66 lines · Eric Anholt
Quoted 29 lines · Lee Jones
Quoted 27 lines · Eric Anholt

From: Lubomir Rintel <[email protected]>

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 <[email protected]>
Signed-off-by Craig McGeachie <slapdau-/[email protected]>
Signed-off-by Suman Anna <[email protected]>
Signed-off-by Jassi Brar <[email protected]>
Signed-off-by Eric Anholt <[email protected]>
Cc Jassi Brar <[email protected]>
Acked-by Lee Jones <[email protected]>

---

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

Quoted 6 lines
Quoted 1 line

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.

Quoted 7 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#27 lore raw

PATCH v4 3/4 mailbox: Enable BCM2835 mailbox support

Quoted 76 lines · Lee Jones
Quoted 66 lines · Eric Anholt
Quoted 29 lines · Lee Jones
Quoted 27 lines · Eric Anholt

From: Lubomir Rintel <[email protected]>

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 <[email protected]>
Signed-off-by Craig McGeachie <[email protected]>
Signed-off-by Suman Anna <[email protected]>
Signed-off-by Jassi Brar <[email protected]>
Signed-off-by Eric Anholt <[email protected]>
Cc Jassi Brar <[email protected]>
Acked-by Lee Jones <[email protected]>

---

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

Quoted 6 lines
Quoted 1 line

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.

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

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.

#28 lore raw

PATCH v4 3/4 mailbox: Enable BCM2835 mailbox support

In reply to Eric Anholt
Quoted 11 lines · Eric Anholt
Quoted 9 lines · Lee Jones
Quoted 7 lines · Eric Anholt

From: Lubomir Rintel <[email protected]>

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.

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

[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

Quoted 54 lines
Quoted 16 lines
Quoted 16 lines
Signed-off-by Lubomir Rintel <[email protected]> Signed-off-by:

Craig McGeachie <[email protected]> Signed-off-by: Suman Anna <[email protected]> Signed-off-by: Jassi Brar <[email protected]> Signed-off-by: Eric Anholt <[email protected]> Cc: Jassi Brar <[email protected]>

Acked-by Lee Jones <[email protected]> ---

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

Quoted 7 lines
Quoted 1 line

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.

Quoted 7 lines

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.

author
#29 lore raw

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

In reply to a message not in the archive
Signed-off-by Eric Anholt <[email protected]>
Acked-by Lee Jones <[email protected]>
---
 arch/arm/boot/dts/bcm2835.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)
arch/arm/boot/dts/bcm2835.dtsi +7 -0
index 3342cb1..6b5c3a1 100644
@@ -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>;
Signature · 6 lines
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#31 lore raw
In reply to a message not in the archive
Quoted 85 lines · 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 <[email protected]>
---
 include/soc/bcm2835/peripheral-workaround.h | 75 +++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 include/soc/bcm2835/peripheral-workaround.h
include/soc/bcm2835/peripheral-workaround.h new file +65 -0
new file mode 100644
index 0000000..4541a13
@@ -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"
+	 *

include/soc/bcm2835/peripheral-workaround.h line 75

Quoted 10 lines
include/soc/bcm2835/peripheral-workaround.h new file +10 -0
+	 * 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#32 lore raw

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

In reply to Eric Anholt

include/soc/bcm2835/peripheral-workaround.h line 11

Quoted 13 lines · 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.

include/soc/bcm2835/peripheral-workaround.h +3 -0
 
+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...

include/soc/bcm2835/peripheral-workaround.h line 16

Quoted 5 lines
include/soc/bcm2835/peripheral-workaround.h +5 -0
+	/*
+	 * 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.

#33 lore raw

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

Quoted 9 lines · Stephen Warren
Quoted 7 lines · 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.

Quoted 8 lines

include/soc/bcm2835/peripheral-workaround.h line 11

Quoted 5 lines
include/soc/bcm2835/peripheral-workaround.h +3 -0
 
+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.

Quoted 12 lines

include/soc/bcm2835/peripheral-workaround.h line 16

Quoted 5 lines
include/soc/bcm2835/peripheral-workaround.h +5 -0
+	/*
+	 * 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.

Quoted 2 lines

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>