BCM2835 core mailbox support
Subsystems (4)
cover BCM2835 core mailbox support
Following srwarren's suggestion, I've re-split the mailbox driver I had previously submitted so that it only exposes a single channel and leaves the previous "channel" notion up to a firmware-specific device driver.
I'll be submitting the firmware-specific driver in a separate mail thread -- hopefully this code is ready now, while I expect to get feeback on the other driver.
This code can also be found at https://github.com/anholt/linux/tree/rpi-mbox-only
PATCH v4 1/3 dt/bindings: Add binding for the BCM2835 mailbox driver
From: Lubomir Rintel <[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).
v4: Move file back by consensus from various Broadcom platform
maintainers (changes by anholt, acked by Lee Jones).
.../devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +19 -0
@@ -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 at 7e00b800 { + compatible = "brcm,bcm2835-mbox"; + reg = <0x7e00b880 0x40>; + interrupts = <0 1>; + #mbox-cells = <1>; +};
Signature · 1 line
2.1.4
Jassi Brar
maintainerPATCH v4 1/3 dt/bindings: Add binding for the BCM2835 mailbox driver
Quoted 31 lines · Eric Anholt
From: Lubomir Rintel <[email protected]>
Signed-off-by Lubomir Rintel <[email protected]>Signed-off-by Craig McGeachie <[email protected]>Signed-off-by Eric Anholt <[email protected]>Acked-by Lee Jones <[email protected]>Acked-by Stephen Warren <[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).v4: Move file back by consensus from various Broadcom platform
maintainers (changes by anholt, acked by Lee Jones)..../devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txtDocumentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +0
-0
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt line 10
Quoted 10 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +10
-0@@ -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
Probably also specify what that one-cell value would stand for.
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt line 19
Quoted 9 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +9
-0+ +Example: + +mailbox: mailbox at 7e00b800 { + compatible = "brcm,bcm2835-mbox"; + reg = <0x7e00b880 0x40>; + interrupts = <0 1>; + #mbox-cells = <1>; +};
Usually it comes with an example user node as well.
PATCH v5 1/3 dt/bindings: Add binding for the BCM2835 mailbox driver
In reply to Eric AnholtFrom: Lubomir Rintel <[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).
v4: Move file back by consensus from various Broadcom platform
maintainers (changes by anholt, acked by Lee Jones).
v5: Document that the mailbox cell should be 0 in clients, and add an
example of a client (changes by anholt, from review by Jassi).
.../bindings/mailbox/brcm,bcm2835-mbox.txt | 27 ++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +27 -0
@@ -0,0 +1,27 @@ +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. Clients should supply a value + of 0 for the mailbox channel, because there is only one + exposed + +Example: + +mailbox: mailbox at 7e00b800 { + compatible = "brcm,bcm2835-mbox"; + reg = <0x7e00b880 0x40>; + interrupts = <0 1>; + #mbox-cells = <1>; +}; + +firmware: firmware { + compatible = "raspberrypi,firmware"; + mboxes = <&mailbox 0>; + #power-domain-cells = <1>; +};
Signature · 1 line
2.1.4
PATCH v5 1/3 dt/bindings: Add binding for the BCM2835 mailbox driver
Quoted 14 lines · Eric Anholt
From: Lubomir Rintel <[email protected]>
Signed-off-by Lubomir Rintel <[email protected]>Signed-off-by Craig McGeachie <[email protected]>Signed-off-by Eric Anholt <[email protected]>Acked-by Lee Jones <[email protected]>Acked-by Stephen Warren <[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).
Quoted 8 lines
v3: Move the file to just bcm2835-mbox.txt, clean up formatting
(changes by anholt, from review by Lee Jones).v4: Move file back by consensus from various Broadcom platform
maintainers (changes by anholt, acked by Lee Jones).v5: Document that the mailbox cell should be 0 in clients, and add an
example of a client (changes by anholt, from review by Jassi).
Some mention of what you changed in the patch might be nice, since it was originally authored by someone else, and there's quite a changelog.
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt line 12
Quoted 6 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt +4
-0+- #mbox-cells: Specifies the number of cells needed to encode a mailbox + channel. The value shall be 1. Clients should supply a value + of 0 for the mailbox channel, because there is only one + exposed
Can't you use #mbox-cells = <0> if there's no data to provide?
PATCH v5 1/3 dt/bindings: Add binding for the BCM2835 mailbox driver
Quoted 18 lines · Stephen Warren
Quoted 17 lines · Eric Anholt
From: Lubomir Rintel <[email protected]>
Signed-off-by Lubomir Rintel <[email protected]>Signed-off-by Craig McGeachie <[email protected]>Signed-off-by Eric Anholt <[email protected]>Acked-by Lee Jones <[email protected]>Acked-by Stephen Warren <[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).
Quoted 8 lines
Quoted 5 lines
v4: Move file back by consensus from various Broadcom platform
maintainers (changes by anholt, acked by Lee Jones).v5: Document that the mailbox cell should be 0 in clients, and add an
example of a client (changes by anholt, from review by Jassi).Some mention of what you changed in the patch might be nice, since it was originally authored by someone else, and there's quite a changelog.
I've been taking this changelog stuff out of the patch and putting it below '---' because when I was submitting this series before, I got chided for putting it in the patch!
Quoted 8 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt line 12
Quoted 6 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt +4
-0+- #mbox-cells: Specifies the number of cells needed to encode a mailbox + channel. The value shall be 1. Clients should supply a value + of 0 for the mailbox channel, because there is only one + exposedCan't you use #mbox-cells = <0> if there's no data to provide?
of_mbox_index_xlate looks like it's always dereferencing the first arg.
-------------- 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/20150429/6c9852e1/attachment-0001.sig>
PATCH v5 1/3 dt/bindings: Add binding for the BCM2835 mailbox driver
Quoted 26 lines · Eric Anholt
Quoted 24 lines · Stephen Warren
Quoted 22 lines · Eric Anholt
From: Lubomir Rintel <[email protected]>
Signed-off-by Lubomir Rintel <[email protected]>Signed-off-by Craig McGeachie <[email protected]>Signed-off-by Eric Anholt <[email protected]>Acked-by Lee Jones <[email protected]>Acked-by Stephen Warren <[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).v4: Move file back by consensus from various Broadcom platform
maintainers (changes by anholt, acked by Lee Jones).v5: Document that the mailbox cell should be 0 in clients, and add an
example of a client (changes by anholt, from review by Jassi).
Quoted 6 lines
Quoted 2 lines
Some mention of what you changed in the patch might be nice, since it was originally authored by someone else, and there's quite a changelog.
I've been taking this changelog stuff out of the patch and putting it below '---' because when I was submitting this series before, I got chided for putting it in the patch!
A changelog and a description of the changes you've made aren't quite the same thing.
There should be some mention in the commit description of what you changed relative to the patch that someone else authored, so they don't get blaimed/praised for any of your changes. The format of that description wouldn't usually be a detailed patch revision changelog though, just a description of the overall changes that were left in the final version.
Quoted 10 lines
Quoted 8 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt line 12
Quoted 6 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt +4
-0+- #mbox-cells: Specifies the number of cells needed to encode a mailbox + channel. The value shall be 1. Clients should supply a value + of 0 for the mailbox channel, because there is only one + exposedCan't you use #mbox-cells = <0> if there's no data to provide?
of_mbox_index_xlatelooks like it's always dereferencing the first arg.
Is it possible to fix that?
That said, last I looked at the HW I thought there were actually multiple channels possible, so perhaps having a cell for future expansion is reasonable.
Jassi Brar
maintainerPATCH v5 1/3 dt/bindings: Add binding for the BCM2835 mailbox driver
Quoted 61 lines · Stephen Warren
Quoted 29 lines · Eric Anholt
Quoted 26 lines · Stephen Warren
Quoted 23 lines · Eric Anholt
From: Lubomir Rintel <[email protected]>
Signed-off-by Lubomir Rintel <[email protected]>Signed-off-by Craig McGeachie <[email protected]>Signed-off-by Eric Anholt <[email protected]>Acked-by Lee Jones <[email protected]>Acked-by Stephen Warren <[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).v4: Move file back by consensus from various Broadcom platform
maintainers (changes by anholt, acked by Lee Jones).v5: Document that the mailbox cell should be 0 in clients, and add an
example of a client (changes by anholt, from review by Jassi).Quoted 7 lines
Quoted 2 lines
Some mention of what you changed in the patch might be nice, since it was originally authored by someone else, and there's quite a changelog.
I've been taking this changelog stuff out of the patch and putting it below '---' because when I was submitting this series before, I got chided for putting it in the patch!
A changelog and a description of the changes you've made aren't quite the same thing.
There should be some mention in the commit description of what you changed relative to the patch that someone else authored, so they don't get blaimed/praised for any of your changes. The format of that description wouldn't usually be a detailed patch revision changelog though, just a description of the overall changes that were left in the final version.
Quoted 13 lines
Quoted 13 lines
Quoted 11 lines
diff --git
a/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txtb/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt+- #mbox-cells: Specifies the number of cells needed to encode a mailbox + channel. The value shall be 1. Clients should supplya value
+ of 0 for the mailbox channel, because there is only
one
+ exposed
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Can't you use #mbox-cells = <0> if there's no data to provide?
of_mbox_index_xlatelooks like it's always dereferencing the first arg.Is it possible to fix that?
The driver specific of_xlate() is meant to be provided during mbox_controller_register() in which case the default 1-cell xlate won't be used.
PATCH v5 2/3 mailbox: Enable BCM2835 mailbox support
In reply to Eric AnholtFrom: 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
---
v2: Squashed Craig's work for review, carried over to new version of
Mailbox framework (changes by Lubomir)
v3: Fix multi-line comment style. Refer to the documentation by
filename. Only declare one MODULE_AUTHOR. Alphabetize includes. Drop some excessive dev_dbg()s (changes by anholt).
v4: Use the new bcm2835_peripheral_read_workaround(), drop the
unnecessary wmb()s, make the messages be a pointer to u32, rather than u32-cast-as-pointer, fold in small static functions, drop extra error messages, clean up sizeof() arg for malloc, disable interrupts on unload (changes by anholt).
v5: Rewrite as a single-channel driver, where the old notion of
channels will now be interpreted by an rpi-specific driver. Minor style fix. Drop peripheral_read_workaround() since it's been noted that readl() covers the non-interrupt half of it, and I found that we would need to resolve read ordering in the interrupt handler anyway (changes by anholt).
drivers/mailbox/Kconfig | 8 ++ drivers/mailbox/Makefile | 2 + drivers/mailbox/bcm2835-mailbox.c | 220 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 230 insertions(+) create mode 100644 drivers/mailbox/bcm2835-mailbox.c
drivers/mailbox/Kconfig +8 -0
@@ -60,4 +60,12 @@ config ALTERA_MBOX An implementation of the Altera Mailbox soft core. It is used to send message between processors. Say Y here if you want to use the Altera mailbox support. + +config BCM2835_MBOX + tristate "BCM2835 Mailbox" + depends on ARCH_BCM2835 + help + An implementation of the BCM2385 Mailbox. It is used to invoke + the services of the Videocore. Say Y here if you want to use the + BCM2835 Mailbox. endif
drivers/mailbox/Makefile +2 -0
@@ -11,3 +11,5 @@ obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o obj-$(CONFIG_PCC) += pcc.o obj-$(CONFIG_ALTERA_MBOX) += mailbox-altera.o + +obj-$(CONFIG_BCM2835_MBOX) += bcm2835-mailbox.o
drivers/mailbox/bcm2835-mailbox.c new file +220 -0
220 lines not on this page · raw message
Signature · 1 line
2.1.4
Jassi Brar
maintainerPATCH v5 2/3 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
We could probably drop the history from changelog. Just talk about what the controller is and any specifics of the driver.
Quoted 5 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]>
How come it has my s-o-b?
drivers/mailbox/bcm2835-mailbox.c line 5
Quoted 11 lines
drivers/mailbox/bcm2835-mailbox.c new file +5
-0@@ -0,0 +1,220 @@ +/* +* Copyright (C) 2010 Broadcom +* Copyright (C) 2013-2014 Lubomir Rintel +* Copyright (C) 2013 Craig McGeachie +*
You may want to make it 2015
drivers/mailbox/bcm2835-mailbox.c line 55
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +4
-0+ +/* Status register: FIFO state. */ +#define ARM_MS_FULL 0x80000000 +#define ARM_MS_EMPTY 0x40000000
nit: BIT(31) and BIT(30)
drivers/mailbox/bcm2835-mailbox.c line 58
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox.c new file +3
-0+ +/* Configuration register: Enable interrupts. */ +#define ARM_MC_IHAVEDATAIRQEN 0x00000001
nit: BIT(0)
drivers/mailbox/bcm2835-mailbox.c line 69
Quoted 11 lines
drivers/mailbox/bcm2835-mailbox.c new file +11
-0+ +struct bcm2835_mbox { +struct device *dev; +void __iomem *regs; +spinlock_t lock; +bool started; +struct mbox_controller controller; +}; + +struct bcm2835_mbox *mbox; +
Bad omen. You assume any platform will ever have just one instance of the mailbox controller. Which may come true, but still is a taboo to think of.
drivers/mailbox/bcm2835-mailbox.c line 72
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox.c new file +3
-0+static irqreturn_t bcm2835_mbox_irq (int irq,void *dev_id) +{ +struct device *dev = mbox->dev;
.... and you are wasting 'dev_id' here, which could have been 'mbox'
drivers/mailbox/bcm2835-mailbox.c line 80
Quoted 9 lines
drivers/mailbox/bcm2835-mailbox.c new file +8
-0+struct mbox_chan *link = &mbox->controller.chans[0 ]; + +while (!(readl (mbox->regs +MAIL0_STA ) &ARM_MS_EMPTY )) { +u32 msg =readl (mbox->regs +MAIL0_RD ); + +if (!mbox->started) { +dev_err (dev,"Reply 0x%08x with no client attached\n" , + msg);
This shouldn't happen unless the remote could send asynchronous commands to Linux. And even if it does, we shouldn't be seeing them before we are ready. Please move the "Enable the interrupt on data reception" from probe to bcm2835_startup(), and disable interrupts in bcm2835_shutdown()
drivers/mailbox/bcm2835-mailbox.c line 93
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +5
-0+static int bcm2835_send_data (struct mbox_chan *link,void *data) +{ +int ret =0 ; +u32 msg = *(u32 *)data; +
Sorry, this is seen as an abuse. Please pass pointer to a u32 and not typecast the value.
drivers/mailbox/bcm2835-mailbox.c line 41
Quoted 3 lines
drivers/mailbox/bcm2835-mailbox.c new file +2
-0+if (!mbox->started) +return -ENODEV ;
This 'started' flag is unnecessary. API won't call send_data() before startup() or after shutdown().
drivers/mailbox/bcm2835-mailbox.c line 100
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +4
-0+if (readl (mbox->regs +MAIL0_STA ) &ARM_MS_FULL ) { + ret = -EBUSY ; + goto end; + }
This check is unnecessary too. API would have already called last_tx_done() already to make sure this never hits.
Cheers.
PATCH v5 2/3 mailbox: Enable BCM2835 mailbox support
Quoted 18 lines · Jassi Brar
Quoted 9 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 5 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.htmlWe could probably drop the history from changelog. Just talk about what the controller is and any specifics of the driver.
Quoted 7 lines
Quoted 5 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]>How come it has my s-o-b?
A good question. Seems like I've added it, but I can't really remember or explain that:
http://lists.infradead.org/pipermail/linux-rpi-kernel/2014
-October/001006.html
It doesn't seem like I've squashed any commit with S-o-b by Jassi Brar or Suman Anna in. Maybe I intended to add a Cc: and pasted in a S-o-b by accident? I'm sorry for that.
Please remove the two.
Thank you,
Lubo
PATCH v5 2/3 mailbox: Enable BCM2835 mailbox support
In reply to Jassi BrarQuoted 8 lines · Jassi Brar
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 5 lines
Quoted 3 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.htmlWe could probably drop the history from changelog. Just talk about what the controller is and any specifics of the driver.
How about:
"This mailbox driver provides a single mailbox channel to write 32-bit values to the VPU and get a 32-bit response. The Raspberry Pi firmware uses this mailbox channel to implement firmware calls, while Roku 2 (despite being derived from the same firmware source) doesn't."
Quoted 7 lines
Quoted 6 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]>How come it has my s-o-b?
The patch had your s-o-b on it when I started working on it:
http://lists.infradead.org/pipermail/linux-rpi-kernel/2014-October/001006.html
Presumably from:
http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-September/000692.html
I don't see your interaction in the cite for you, though, so if you'd like the s-o-b removed, I'm happy to do so. It's been an awfully long time in development for this driver, with enough revisions, I figured I just hadn't found where your involvement exactly was.
Quoted 4 lines
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox.c new file +0
-1--- /dev/null
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c line 5
Quoted 7 lines
+++
b/drivers/mailbox/bcm2835-mailbox.cdrivers/mailbox/bcm2835-mailbox.c new file +5
-0@@ -0,0 +1,220 @@ +/* +* Copyright (C) 2010 Broadcom +* Copyright (C) 2013-2014 Lubomir Rintel +* Copyright (C) 2013 Craig McGeachie +* You may want to make it 2015
At this point I've probably done enough to merit adding a 2015 for Broadcom. Done.
Quoted 11 lines
drivers/mailbox/bcm2835-mailbox.c line 55
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox.c new file +3
-0+/* Status register: FIFO state. */ +#define ARM_MS_FULL 0x80000000 +#define ARM_MS_EMPTY 0x40000000nit: BIT(31) and BIT(30)
drivers/mailbox/bcm2835-mailbox.c line 58
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox.c new file +3
-0+ +/* Configuration register: Enable interrupts. */ +#define ARM_MC_IHAVEDATAIRQEN 0x00000001nit: BIT(0)
Works for me.
Quoted 6 lines
Quoted 6 lines
drivers/mailbox/bcm2835-mailbox.c new file +6
-0+ +struct bcm2835_mbox { +struct device *dev; +void __iomem *regs; +spinlock_t lock; +bool started;
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c line 69
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +5
-0+struct mbox_controller controller; +}; + +struct bcm2835_mbox *mbox; +Bad omen. You assume any platform will ever have just one instance of the mailbox controller. Which may come true, but still is a taboo to think of.
On the other hand, when I've submitted to other subsystems people have complained about how I have these extra lookups/container_ofs, instead of just storing the obviously-only-one-of-them thing in a global.
I think a global makes definite sense for this driver. But if I have to go readd the code I had cleaned up, to act like there might be more than one, I could.
Quoted 6 lines
Quoted 6 lines
drivers/mailbox/bcm2835-mailbox.c new file +6
-0+struct mbox_chan *link = &mbox->controller.chans[0 ]; + +while (!(readl (mbox->regs +MAIL0_STA ) &ARM_MS_EMPTY )) { +u32 msg =readl (mbox->regs +MAIL0_RD ); + +if (!mbox->started) {
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c line 80
Quoted 3 lines
drivers/mailbox/bcm2835-mailbox.c new file +2
-0+dev_err (dev,"Reply 0x%08x with no client attached\n" , + msg);This shouldn't happen unless the remote could send asynchronous commands to Linux. And even if it does, we shouldn't be seeing them before we are ready. Please move the "Enable the interrupt on data reception" from probe to
bcm2835_startup(), and disable interrupts inbcm2835_shutdown()
Done.
Quoted 7 lines
drivers/mailbox/bcm2835-mailbox.c line 93
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +5
-0+static int bcm2835_send_data (struct mbox_chan *link,void *data) +{ +int ret =0 ; +u32 msg = *(u32 *)data; +Sorry, this is seen as an abuse. Please pass pointer to a u32 and not typecast the value.
This is a pointer to a u32 being passed in. I think you misread, or I'm misunderstanding you.
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c line 37
Quoted 3 lines
drivers/mailbox/bcm2835-mailbox.c new file +2
-0+if (!mbox->started) +return -ENODEV ;This 'started' flag is unnecessary. API won't call
send_data()beforestartup()or aftershutdown().
Dropped.
Quoted 7 lines
drivers/mailbox/bcm2835-mailbox.c line 100
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +4
-0+if (readl (mbox->regs +MAIL0_STA ) &ARM_MS_FULL ) { + ret = -EBUSY ; + goto end; + }This check is unnecessary too. API would have already called
last_tx_done()already to make sure this never hits.
Dropped.
-------------- 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/20150428/7a449b9e/attachment.sig>
Jassi Brar
maintainerPATCH v5 2/3 mailbox: Enable BCM2835 mailbox support
Quoted 17 lines · Eric Anholt
Quoted 6 lines · Jassi Brar
Quoted 6 lines
drivers/mailbox/bcm2835-mailbox.c new file +6
-0+ +struct bcm2835_mbox { +struct device *dev; +void __iomem *regs; +spinlock_t lock; +bool started;Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c line 69
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +5
-0+struct mbox_controller controller; +}; + +struct bcm2835_mbox *mbox; +Bad omen. You assume any platform will ever have just one instance of the mailbox controller. Which may come true, but still is a taboo to think of.
Quoted 6 lines
On the other hand, when I've submitted to other subsystems people have complained about how I have these extra lookups/container_ofs, instead of just storing the obviously-only-one-of-them thing in a global.
I think a global makes definite sense for this driver.
0) Why is 'mbox' not even static?
1) It makes sense for system wide entities like stack/protocol instance. But this is _one_ controller instance with _one_ mailbox instance. You think any platform will ever only need one mailbox or use two classes of controllers?
2) "Avoidable global statics are generally bad"
Quoted 10 lines
Quoted 7 lines
drivers/mailbox/bcm2835-mailbox.c line 100
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +4
-0+if (readl (mbox->regs +MAIL0_STA ) &ARM_MS_FULL ) { + ret = -EBUSY ; + goto end; + }This check is unnecessary too. API would have already called
last_tx_done()already to make sure this never hits.Dropped.
I see it's not yet.
PATCH v5 2/3 mailbox: Enable BCM2835 mailbox support
Quoted 18 lines · Jassi Brar
Quoted 17 lines · Eric Anholt
Quoted 14 lines · Jassi Brar
drivers/mailbox/bcm2835-mailbox.c line 69
Quoted 11 lines
drivers/mailbox/bcm2835-mailbox.c new file +11
-0+ +struct bcm2835_mbox { +struct device *dev; +void __iomem *regs; +spinlock_t lock; +bool started; +struct mbox_controller controller; +}; + +struct bcm2835_mbox *mbox; +Bad omen. You assume any platform will ever have just one instance of the mailbox controller. Which may come true, but still is a taboo to think of.
Quoted 7 lines
Quoted 6 lines
On the other hand, when I've submitted to other subsystems people have complained about how I have these extra lookups/container_ofs, instead of just storing the obviously-only-one-of-them thing in a global.
I think a global makes definite sense for this driver.
0) Why is 'mbox' not even static?
Typo.
Quoted 4 lines
1) It makes sense for system wide entities like stack/protocol instance. But this is _one_ controller instance with _one_ mailbox instance. You think any platform will ever only need one mailbox or use two classes of controllers?
Yes, I think there will only ever be one or zero instances of this driver. I give it 10:1 odds.
Quoted 11 lines
Quoted 10 lines
Quoted 7 lines
drivers/mailbox/bcm2835-mailbox.c line 100
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +4
-0+if (readl (mbox->regs +MAIL0_STA ) &ARM_MS_FULL ) { + ret = -EBUSY ; + goto end; + }This check is unnecessary too. API would have already called
last_tx_done()already to make sure this never hits.Dropped.
I see it's not yet.
Misread this while going through the ->stopped removals. Fixed now.
-------------- 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/20150429/bc4157e5/attachment.sig>
PATCH v6 2/3 mailbox: Enable BCM2835 mailbox support
In reply to Eric AnholtFrom: Lubomir Rintel <[email protected]>
This mailbox driver provides a single mailbox channel to write 32-bit values to the VPU and get a 32-bit response. The Raspberry Pi firmware uses this mailbox channel to implement firmware calls, while Roku 2 (despite being derived from the same firmware tree) doesn't.
---
v2: Squashed Craig's work for review, carried over to new version of
Mailbox framework (changes by Lubomir)
v3: Fix multi-line comment style. Refer to the documentation by
filename. Only declare one MODULE_AUTHOR. Alphabetize includes. Drop some excessive dev_dbg()s (changes by anholt).
v4: Use the new bcm2835_peripheral_read_workaround(), drop the
unnecessary wmb()s, make the messages be a pointer to u32, rather than u32-cast-as-pointer, fold in small static functions, drop extra error messages, clean up sizeof() arg for malloc, disable interrupts on unload (changes by anholt).
v5: Rewrite as a single-channel driver, where the old notion of
channels will now be interpreted by an rpi-specific driver. Minor style fix. Drop peripheral_read_workaround() since it's been noted that readl() covers the non-interrupt half of it, and I found that we would need to resolve read ordering in the interrupt handler anyway (changes by anholt).
v6: rewrite commit message, only turn on interrupts when a client is
present, drop the started flag, use BIT() macro, update Broadcom copyright (changes by anholt, from Jassi's review). Drop s-o-b of Jassi and Suman, who appear to have been accidentally added by Craig (noted by Lubomir).
drivers/mailbox/Kconfig | 8 ++ drivers/mailbox/Makefile | 2 + drivers/mailbox/bcm2835-mailbox.c | 208 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 218 insertions(+) create mode 100644 drivers/mailbox/bcm2835-mailbox.c
drivers/mailbox/Kconfig +8 -0
@@ -60,4 +60,12 @@ config ALTERA_MBOX An implementation of the Altera Mailbox soft core. It is used to send message between processors. Say Y here if you want to use the Altera mailbox support. + +config BCM2835_MBOX + tristate "BCM2835 Mailbox" + depends on ARCH_BCM2835 + help + An implementation of the BCM2385 Mailbox. It is used to invoke + the services of the Videocore. Say Y here if you want to use the + BCM2835 Mailbox. endif
drivers/mailbox/Makefile +2 -0
@@ -11,3 +11,5 @@ obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o obj-$(CONFIG_PCC) += pcc.o obj-$(CONFIG_ALTERA_MBOX) += mailbox-altera.o + +obj-$(CONFIG_BCM2835_MBOX) += bcm2835-mailbox.o
drivers/mailbox/bcm2835-mailbox.c new file +208 -0
208 lines not on this page · raw message
Signature · 1 line
2.1.4
PATCH v6 2/3 mailbox: Enable BCM2835 mailbox support
Quoted 6 lines · Eric Anholt
From: Lubomir Rintel <[email protected]>
This mailbox driver provides a single mailbox channel to write 32-bit values to the VPU and get a 32-bit response. The Raspberry Pi firmware uses this mailbox channel to implement firmware calls, while Roku 2 (despite being derived from the same firmware tree) doesn't.
PATCH v7 2/3 mailbox: Enable BCM2835 mailbox support
In reply to Eric AnholtFrom: Lubomir Rintel <[email protected]>
This mailbox driver provides a single mailbox channel to write 32-bit values to the VPU and get a 32-bit response. The Raspberry Pi firmware uses this mailbox channel to implement firmware calls, while Roku 2 (despite being derived from the same firmware tree) doesn't.
---
v2: Squashed Craig's work for review, carried over to new version of
Mailbox framework (changes by Lubomir)
v3: Fix multi-line comment style. Refer to the documentation by
filename. Only declare one MODULE_AUTHOR. Alphabetize includes. Drop some excessive dev_dbg()s (changes by anholt).
v4: Use the new bcm2835_peripheral_read_workaround(), drop the
unnecessary wmb()s, make the messages be a pointer to u32, rather than u32-cast-as-pointer, fold in small static functions, drop extra error messages, clean up sizeof() arg for malloc, disable interrupts on unload (changes by anholt).
v5: Rewrite as a single-channel driver, where the old notion of
channels will now be interpreted by an rpi-specific driver. Minor style fix. Drop peripheral_read_workaround() since it's been noted that readl() covers the non-interrupt half of it, and I found that we would need to resolve read ordering in the interrupt handler anyway (changes by anholt).
v6: rewrite commit message, only turn on interrupts when a client is
present, drop the started flag, use BIT() macro, update Broadcom copyright (changes by anholt, from Jassi's review). Drop s-o-b of Jassi and Suman, who appear to have been accidentally added by Craig (noted by Lubomir).
v7: Drop the txdone-style check in send_data, since the core will make
sure we're not called until the previous one is done. Fix "mbox" to be static (changes by anholt, from review by Jassi).
drivers/mailbox/Kconfig | 8 ++ drivers/mailbox/Makefile | 2 + drivers/mailbox/bcm2835-mailbox.c | 204 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 214 insertions(+) create mode 100644 drivers/mailbox/bcm2835-mailbox.c
drivers/mailbox/Kconfig +8 -0
@@ -60,4 +60,12 @@ config ALTERA_MBOX An implementation of the Altera Mailbox soft core. It is used to send message between processors. Say Y here if you want to use the Altera mailbox support. + +config BCM2835_MBOX + tristate "BCM2835 Mailbox" + depends on ARCH_BCM2835 + help + An implementation of the BCM2385 Mailbox. It is used to invoke + the services of the Videocore. Say Y here if you want to use the + BCM2835 Mailbox. endif
drivers/mailbox/Makefile +2 -0
@@ -11,3 +11,5 @@ obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o obj-$(CONFIG_PCC) += pcc.o obj-$(CONFIG_ALTERA_MBOX) += mailbox-altera.o + +obj-$(CONFIG_BCM2835_MBOX) += bcm2835-mailbox.o
drivers/mailbox/bcm2835-mailbox.c new file +204 -0
204 lines not on this page · raw message
Signature · 1 line
2.1.4
Jassi Brar
maintainerPATCH v7 2/3 mailbox: Enable BCM2835 mailbox support
Quoted 5 lines · Eric Anholt
drivers/mailbox/bcm2835-mailbox.c new file +5
-0+ +struct bcm2835_mbox { +struct device *dev; +void __iomem *regs; +spinlock_t lock;
drivers/mailbox/bcm2835-mailbox.c line 73
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c new file +10
-0+struct mbox_controller controller; +}; + +static struct bcm2835_mbox *mbox; + +static irqreturn_t bcm2835_mbox_irq (int irq,void *dev_id) +{ +struct device *dev = mbox->dev; +struct mbox_chan *link = &mbox->controller.chans[0 ]; +
I learn from Stephen's other post that the controller could have multiple channels. In which case this driver is poorly setup. Actually if the driver was designed properly there isn't anything special to be done.
Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c new file +7
-0+while (!(readl (mbox->regs +MAIL0_STA ) &ARM_MS_EMPTY )) { +u32 msg =readl (mbox->regs +MAIL0_RD ); +dev_dbg (dev,"Reply 0x%08X\n" , msg); +mbox_chan_received_data (link, &msg); + } +return IRQ_HANDLED ; +}
drivers/mailbox/bcm2835-mailbox.c line 90
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c new file +10
-0+ +static int bcm2835_send_data (struct mbox_chan *link,void *data) +{ +int ret =0 ; +u32 msg = *(u32 *)data; + +spin_lock (&mbox->lock); +writel (msg, mbox->regs +MAIL1_WRT ); +dev_dbg (mbox->dev,"Request 0x%08X\n" , msg); +end:
Did you compile check the driver for errors and warnings?
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +4
-0+static int bcm2835_mbox_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +int ret =0 ;
drivers/mailbox/bcm2835-mailbox.c line 138
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c new file +10
-0+struct resource *iomem; + + mbox =devm_kzalloc (dev,sizeof (*mbox),GFP_KERNEL ); +if (mbox ==NULL ) +return -ENOMEM ; + 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);
Why even pass 'mbox' when you insist on ignoring 'dev_id' and access the global variable again directly :D
PATCH v7 2/3 mailbox: Enable BCM2835 mailbox support
Quoted 14 lines · Jassi Brar
Quoted 12 lines · Eric Anholt
drivers/mailbox/bcm2835-mailbox.c new file +12
-0+ +struct bcm2835_mbox { +struct device *dev; +void __iomem *regs; +spinlock_t lock; +struct mbox_controller controller; +}; + +static struct bcm2835_mbox *mbox; + +static irqreturn_t bcm2835_mbox_irq (int irq,void *dev_id) +{
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c line 73
Quoted 3 lines
drivers/mailbox/bcm2835-mailbox.c new file +3
-0+struct device *dev = mbox->dev; +struct mbox_chan *link = &mbox->controller.chans[0 ]; +I learn from Stephen's other post that the controller could have multiple channels. In which case this driver is poorly setup. Actually if the driver was designed properly there isn't anything special to be done.
Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0
Now that I look a bit harder at the registers, I think there are 2 mailbox register sets, but each is uni-directional, so a pair makes up the typical bi-directional mailbox. There are multiple "owner" areas (or sets of registers); I'm not quite sure what that implies.
As such, limiting this driver to a single mailbox is probably correct.
I would expect to see #mbox-cells=<0> in the DT, which would require a custom of_xlate though (or modifying the default to work with a 0 cell count; IIRC the default of_xlate for some other subsystems will work in that scenario).
(As an aside, if we ever did find the need to expand the driver to cover more mailboxes in the future, extending it and the DT to support #mbox-cells=<0> or #mbox-cells=<1> at run-time should be trivial).
Jassi Brar
maintainerPATCH v7 2/3 mailbox: Enable BCM2835 mailbox support
Quoted 24 lines · Stephen Warren
Quoted 14 lines · Jassi Brar
Quoted 12 lines · Eric Anholt
drivers/mailbox/bcm2835-mailbox.c new file +12
-0+ +struct bcm2835_mbox { +struct device *dev; +void __iomem *regs; +spinlock_t lock; +struct mbox_controller controller; +}; + +static struct bcm2835_mbox *mbox; + +static irqreturn_t bcm2835_mbox_irq (int irq,void *dev_id) +{Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c line 73
Quoted 3 lines
drivers/mailbox/bcm2835-mailbox.c new file +3
-0+struct device *dev = mbox->dev; +struct mbox_chan *link = &mbox->controller.chans[0 ]; +I learn from Stephen's other post that the controller could have multiple channels. In which case this driver is poorly setup. Actually if the driver was designed properly there isn't anything special to be done.
Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0
Quoted 5 lines
Now that I look a bit harder at the registers, I think there are 2 mailbox register sets, but each is uni-directional, so a pair makes up the typical bi-directional mailbox. There are multiple "owner" areas (or sets of registers); I'm not quite sure what that implies.
Probably a mailbox is the send-recv pair, and the controller could have many such pairs.
Quoted 2 lines
As such, limiting this driver to a single mailbox is probably correct.
OK
Quoted 5 lines
I would expect to see #mbox-cells=<0> in the DT, which would require a custom
of_xlatethough (or modifying the default to work with a 0 cell count; IIRC the defaultof_xlatefor some other subsystems will work in that scenario).
Yeah the api could simply return the first free channel for 0-cell spec.
PATCH v7 2/3 mailbox: Enable BCM2835 mailbox support
In reply to Jassi BrarQuoted 14 lines · Jassi Brar
Quoted 12 lines · Eric Anholt
drivers/mailbox/bcm2835-mailbox.c new file +12
-0+ +struct bcm2835_mbox { +struct device *dev; +void __iomem *regs; +spinlock_t lock; +struct mbox_controller controller; +}; + +static struct bcm2835_mbox *mbox; + +static irqreturn_t bcm2835_mbox_irq (int irq,void *dev_id) +{
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c line 73
Quoted 3 lines
drivers/mailbox/bcm2835-mailbox.c new file +3
-0+struct device *dev = mbox->dev; +struct mbox_chan *link = &mbox->controller.chans[0 ]; +I learn from Stephen's other post that the controller could have multiple channels. In which case this driver is poorly setup. Actually if the driver was designed properly there isn't anything special to be done.
Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0
There's only the one channel according to the docs. I wish we wouldn't get derailed by speculation on the list when the documentation is available. :(
Quoted 11 lines
Quoted 11 lines
drivers/mailbox/bcm2835-mailbox.c new file +11
-0+while (!(readl (mbox->regs +MAIL0_STA ) &ARM_MS_EMPTY )) { +u32 msg =readl (mbox->regs +MAIL0_RD ); +dev_dbg (dev,"Reply 0x%08X\n" , msg); +mbox_chan_received_data (link, &msg); + } +return IRQ_HANDLED ; +} + +static int bcm2835_send_data (struct mbox_chan *link,void *data) +{ +int ret =0 ;
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c line 90
Quoted 7 lines
drivers/mailbox/bcm2835-mailbox.c new file +6
-0+u32 msg = *(u32 *)data; + +spin_lock (&mbox->lock); +writel (msg, mbox->regs +MAIL1_WRT ); +dev_dbg (mbox->dev,"Request 0x%08X\n" , msg); +end:Did you compile check the driver for errors and warnings?
At this point I'm so burned out on repainting this bikeshed that I
missed a spot of the current color.
-------------- 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/20150504/4ef0cb46/attachment-0001.sig>
Jassi Brar
maintainerPATCH v7 2/3 mailbox: Enable BCM2835 mailbox support
Quoted 25 lines · Eric Anholt
Quoted 14 lines · Jassi Brar
Quoted 12 lines · Eric Anholt
drivers/mailbox/bcm2835-mailbox.c new file +12
-0+ +struct bcm2835_mbox { +struct device *dev; +void __iomem *regs; +spinlock_t lock; +struct mbox_controller controller; +}; + +static struct bcm2835_mbox *mbox; + +static irqreturn_t bcm2835_mbox_irq (int irq,void *dev_id) +{Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c line 73
Quoted 3 lines
drivers/mailbox/bcm2835-mailbox.c new file +3
-0+struct device *dev = mbox->dev; +struct mbox_chan *link = &mbox->controller.chans[0 ]; +I learn from Stephen's other post that the controller could have multiple channels. In which case this driver is poorly setup. Actually if the driver was designed properly there isn't anything special to be done.
Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0
Quoted 4 lines
There's only the one channel according to the docs. I wish we wouldn't get derailed by speculation on the list when the documentation is available. :(
Can I have the pointer to the doc please, if its publicly available.
Quoted 20 lines
Quoted 11 lines
Quoted 11 lines
drivers/mailbox/bcm2835-mailbox.c new file +11
-0+while (!(readl (mbox->regs +MAIL0_STA ) &ARM_MS_EMPTY )) { +u32 msg =readl (mbox->regs +MAIL0_RD ); +dev_dbg (dev,"Reply 0x%08X\n" , msg); +mbox_chan_received_data (link, &msg); + } +return IRQ_HANDLED ; +} + +static int bcm2835_send_data (struct mbox_chan *link,void *data) +{ +int ret =0 ;Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c line 90
Quoted 7 lines
drivers/mailbox/bcm2835-mailbox.c new file +6
-0+u32 msg = *(u32 *)data; + +spin_lock (&mbox->lock); +writel (msg, mbox->regs +MAIL1_WRT ); +dev_dbg (mbox->dev,"Request 0x%08X\n" , msg); +end:Did you compile check the driver for errors and warnings?
Quoted 3 lines
At this point I'm so burned out on repainting this bikeshed that I missed a spot of the current color.
I have just rechecked the thread and I didn't find any revision that had only nits picked. Subsystems even dictate how the files are named and ordered in Makefile, and you have been insisting on hardcoding values.
PATCH v7 2/3 mailbox: Enable BCM2835 mailbox support
Quoted 26 lines · Jassi Brar
Quoted 25 lines · Eric Anholt
Quoted 22 lines · Jassi Brar
Quoted 5 lines · Eric Anholt
drivers/mailbox/bcm2835-mailbox.c new file +5
-0+ +struct bcm2835_mbox { +struct device *dev; +void __iomem *regs; +spinlock_t lock;drivers/mailbox/bcm2835-mailbox.c line 73
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c new file +10
-0+struct mbox_controller controller; +}; + +static struct bcm2835_mbox *mbox; + +static irqreturn_t bcm2835_mbox_irq (int irq,void *dev_id) +{ +struct device *dev = mbox->dev; +struct mbox_chan *link = &mbox->controller.chans[0 ]; +I learn from Stephen's other post that the controller could have multiple channels. In which case this driver is poorly setup. Actually if the driver was designed properly there isn't anything special to be done.
Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0
Quoted 5 lines
Quoted 4 lines
There's only the one channel according to the docs. I wish we wouldn't get derailed by speculation on the list when the documentation is available. :(
Can I have the pointer to the doc please, if its publicly available.
https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf
PATCH 3/3 ARM: bcm2835: Add the mailbox to the device tree
In reply to Eric Anholt--- arch/arm/boot/dts/bcm2835.dtsi | 7 +++++++ 1 file changed, 7 insertions(+)
arch/arm/boot/dts/bcm2835.dtsi +7 -0
@@ -61,6 +61,13 @@ reg = <0x7e104000 0x10>; }; + mailbox: mailbox at 7e00b800 { + compatible = "brcm,bcm2835-mbox"; + reg = <0x7e00b880 0x40>; + interrupts = <0 1>; + #mbox-cells = <1>; + }; + gpio: gpio at 7e200000 { compatible = "brcm,bcm2835-gpio"; reg = <0x7e200000 0xb4>;
Signature · 1 line
2.1.4