mailbox: Enable BCM2835 mailbox support
Subsystems (4)
PATCH 1/1 mailbox: Enable BCM2835 mailbox support
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
[lkundrak at v3.sk: Tidied up Squashed Craig's work for review, carried over to new version of Mailbox framework]
--- .../bindings/mailbox/brcm,bcm2835-mbox.txt | 18 ++ drivers/mailbox/Kconfig | 9 + drivers/mailbox/Makefile | 1 + drivers/mailbox/bcm2835-mailbox.c | 291 +++++++++++++++++++++ 4 files changed, 319 insertions(+) create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt create mode 100644 drivers/mailbox/bcm2835-mailbox.c
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +18 -0
@@ -0,0 +1,18 @@ +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. Must be <0 1>, the mailbox interrupt. +- #mbox-cells : Specifies the number of cells needed to encode a + mailbox channel. The value shall be 1. + +Example: + +mailbox: mailbox at 0x7e00b800 { + compatible = "brcm,bcm2835-mbox"; + reg = <0x7e00b880 0x40>; + interrupts = <0 1>; + #mbox-cells = <1>; +};
drivers/mailbox/Kconfig +9 -0
@@ -33,4 +33,13 @@ config OMAP_MBOX_KFIFO_SIZE Specify the default size of mailbox's kfifo buffers (bytes). This can also be changed at runtime (via the mbox_kfifo_size module parameter). + +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 +1 -0
@@ -5,3 +5,4 @@ obj-$(CONFIG_MAILBOX) += mailbox.o obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o +obj-$(CONFIG_BCM2835_MBOX) += bcm2835-mailbox.o
drivers/mailbox/bcm2835-mailbox.c new file +291 -0
291 lines not on this page · raw message
Signature · 1 line
2.1.0
PATCH 1/1 mailbox: Enable BCM2835 mailbox support
Hi Lubomir,
Am 24.10.2014 um 15:19 schrieb Lubomir Rintel:
Quoted 43 lines
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[lkundrak at v3.sk: Tidied up Squashed Craig's work for review, carried over to new version of Mailbox framework]
Signed-off-by Craig McGeachie <[email protected]>Signed-off-by Suman Anna <[email protected]>Signed-off-by Jassi Brar <[email protected]>Signed-off-by Lubomir Rintel <[email protected]>Cc Stephen Warren <[email protected]>Cc Jassi Brar <[email protected]>Cc Lee Jones <[email protected]>Cc linux-rpi-kernel at lists.infradead.orgCc linux-arm-kernel at lists.infradead.org--- .../bindings/mailbox/brcm,bcm2835-mbox.txt | 18 ++ drivers/mailbox/Kconfig | 9 + drivers/mailbox/Makefile | 1 + drivers/mailbox/bcm2835-mailbox.c | 291 +++++++++++++++++++++ 4 files changed, 319 insertions(+) create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt create mode 100644 drivers/mailbox/bcm2835-mailbox.cDocumentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +8
-0@@ -0,0 +1,18 @@ +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. Must be <0 1>, the mailbox interrupt. +- #mbox-cells : Specifies the number of cells needed to encode a
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt line 18
Quoted 10 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +10
-0+ mailbox channel. The value shall be 1. + +Example: + +mailbox: mailbox at 0x7e00b800 { + compatible = "brcm,bcm2835-mbox"; + reg = <0x7e00b880 0x40>; + interrupts = <0 1>; + #mbox-cells = <1>; +};
i think you should add the devicetree maintainers to CC and the Documentation is a extra patch.
Quoted 33 lines
[...]
drivers/mailbox/bcm2835-mailbox.c new file +25
-0@@ -0,0 +1,291 @@ +/* +* Copyright (C) 2010 Broadcom +* Copyright (C) 2013-2014 Lubomir Rintel +* Copyright (C) 2013 Craig McGeachie +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +* +* This device provides a mechanism for writing to the mailboxes, +* that are shared between the ARM and the VideoCore processor +* +* Parts of the driver are based on: +* - arch/arm/mach-bcm2708/vcio.c file written by Gray Girling that was +* obtained from branch "rpi-3.6.y" of git://github.com/raspberrypi/ +* linux.git +* - drivers/mailbox/bcm2835-ipc.c by Lubomir Rintel at +* https://github.com/hackerspace/rpi-linux/blob/lr-raspberry-pi/drivers/ +* mailbox/bcm2835-ipc.c +* - documentation available on the following web site: +* https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface +*/ + +#include <linux/module.h> +#include <linux/interrupt.h>
drivers/mailbox/bcm2835-mailbox.c line 35
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c new file +10
-0+#include <linux/irq.h> +#include <linux/device.h> +#include <linux/dma-mapping.h> +#include <linux/platform_device.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/kernel.h> +#include <linux/mailbox_controller.h> +#include <linux/err.h> +#include <linux/spinlock.h>
Please reorder the includes alphabetical.
drivers/mailbox/bcm2835-mailbox.c line 89
Quoted 9 lines
[...]
drivers/mailbox/bcm2835-mailbox.c new file +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; + +while (!(readl (mbox->regs +MAIL0_STA ) &ARM_MS_EMPTY )) {
I think it would be good to have a timeout condition here.
In general i believe not all debug messages are necessary anymore.
Thanks
Stefan
PATCH 1/1 mailbox: Enable BCM2835 mailbox support
In reply to Lubomir RintelOn Fri, 24 Oct 2014, Lubomir Rintel wrote:
Thanks for sending this to list, as you are aware a lot of the other platform drivers depend on this.
FYI: I don't know much about the Mailbox FW, so I'll leave the functional review to the people who do. This is going to be a coding stylesque review instead.
Quoted 8 lines
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
[lkundrak at v3.sk: Tidied up Squashed Craig's work for review, carried over to new version of Mailbox framework]
Signed-off-by Craig McGeachie <[email protected]>Signed-off-by Suman Anna <[email protected]>Signed-off-by Jassi Brar <[email protected]>Signed-off-by Lubomir Rintel <[email protected]>
Did all of this people author this driver? Did they also review the final product and did they all sign it off? This list insinuates that Craig wrote the driver and was taken over by Suman etc, until it was finally finished off and send upstream by you. Is that really the case?
Quoted 5 lines
Cc Stephen Warren <[email protected]>Cc Jassi Brar <[email protected]>Cc Lee Jones <[email protected]>Cc linux-rpi-kernel at lists.infradead.orgCc linux-arm-kernel at lists.infradead.org
As you're only sending a small patch-set, it's better to address the emails to these guys directly, rather than doing so in here. Cc's in commit messages are good if there are only a few patches within a large set to be sent to different addresses.
FWIW, you can drop Stephen and myself from Cc completely as we're both subscribed to the RPi list and it's very low volume, thus we're not likely to miss it.
Quoted 2 lines
--- .../bindings/mailbox/brcm,bcm2835-mbox.txt | 18 ++
This should be a seperate patch and you should Cc the DT list on it.
See: Documentation/devicetree/bindings/submitting-patches.txt
Quoted 13 lines
drivers/mailbox/Kconfig | 9 + drivers/mailbox/Makefile | 1 + drivers/mailbox/bcm2835-mailbox.c | 291 +++++++++++++++++++++ 4 files changed, 319 insertions(+) create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt create mode 100644 drivers/mailbox/bcm2835-mailbox.cDocumentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +0
-0
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt line 9
Quoted 9 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +9
-0@@ -0,0 +1,18 @@ +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. Must be <0 1>, the mailbox interrupt. +- #mbox-cells : Specifies the number of cells needed to encode a + mailbox channel. The value shall be 1.
Either start the sentence with an uppercase letter, or don't.
Consistency aids in readability.
Links to where the generic properties are documented are helpful.
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt line 13
Quoted 3 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +3
-0+Example: + +mailbox: mailbox at 0x7e00b800 {
Drop the 0x
Quoted 10 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +5
-0+ compatible = "brcm,bcm2835-mbox"; + reg = <0x7e00b880 0x40>; + interrupts = <0 1>; + #mbox-cells = <1>; +};drivers/mailbox/Kconfig +0
-0
drivers/mailbox/Kconfig config OMAP_MBOX_KFIFO_SIZE · line 39
Quoted 7 lines
drivers/mailbox/Kconfig +4
-0@@ -33,4 +33,13 @@ config OMAP_MBOX_KFIFO_SIZE Specify the default size of mailbox's kfifo buffers (bytes). This can also be changed at runtime (via the mbox_kfifo_size module parameter). + +config BCM2835_MBOX + tristate "BCM2835 Mailbox" + depends on ARCH_BCM2835
depends on OF?
Quoted 21 lines
drivers/mailbox/Kconfig +5
-0+ 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. + endifdrivers/mailbox/Makefile +1
-0@@ -5,3 +5,4 @@ obj-$(CONFIG_MAILBOX) += mailbox.o obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o +obj-$(CONFIG_BCM2835_MBOX) += bcm2835-mailbox.odrivers/mailbox/bcm2835-mailbox.c new file +0
-0
drivers/mailbox/bcm2835-mailbox.c line 4
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox.c new file +4
-0@@ -0,0 +1,291 @@ +/* +* Copyright (C) 2010 Broadcom +* Copyright (C) 2013-2014 Lubomir Rintel +* Copyright (C) 2013 Craig McGeachie
Better to put these in chronological order.
Does the Copyright really belong to you?
drivers/mailbox/bcm2835-mailbox.c line 14
Quoted 9 lines
drivers/mailbox/bcm2835-mailbox.c new file +9
-0+* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +* +* This device provides a mechanism for writing to the mailboxes, +* that are shared between the ARM and the VideoCore processor +* +* Parts of the driver are based on: +* - arch/arm/mach-bcm2708/vcio.c file written by Gray Girling that was
This doesn't seem very future-proof.
drivers/mailbox/bcm2835-mailbox.c line 16
Quoted 2 lines
drivers/mailbox/bcm2835-mailbox.c new file +2
-0+* obtained from branch "rpi-3.6.y" of git://github.com/raspberrypi/ +* linux.git
Drop all this please.
drivers/mailbox/bcm2835-mailbox.c line 21
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +5
-0+* - drivers/mailbox/bcm2835-ipc.c by Lubomir Rintel at +* https://github.com/hackerspace/rpi-linux/blob/lr-raspberry-pi/drivers/ +* mailbox/bcm2835-ipc.c +* - documentation available on the following web site: +* https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
And this. Please don't put links to external Git repos in drivers.
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox.c new file +4
-0+*/ + +#include <linux/module.h> +#include <linux/interrupt.h>
drivers/mailbox/bcm2835-mailbox.c line 35
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c new file +10
-0+#include <linux/irq.h> +#include <linux/device.h> +#include <linux/dma-mapping.h> +#include <linux/platform_device.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/kernel.h> +#include <linux/mailbox_controller.h> +#include <linux/err.h> +#include <linux/spinlock.h>
Alphabetical.
drivers/mailbox/bcm2835-mailbox.c line 44
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c new file +8
-0+/* Mailboxes */ +#define ARM_0_MAIL0 0x00 +#define ARM_0_MAIL1 0x20 + +/* Mailbox registers. We basically only support mailbox 0 & 1. We deliver to +* the VC in mailbox 1, it delivers to us in mailbox 0. See BCM2835 ARM +* Peripherals section 1.3 for an explanation about the placement of memory +* barriers. */
Nonstandard multi-line comment.
Please see: Documentation/CodingStyle
Quoted 13 lines
drivers/mailbox/bcm2835-mailbox.c new file +13
-0+#define MAIL0_RD (ARM_0_MAIL0 + 0x00) +#define MAIL0_POL (ARM_0_MAIL0 + 0x10) +#define MAIL0_STA (ARM_0_MAIL0 + 0x18) +#define MAIL0_CNF (ARM_0_MAIL0 + 0x1C) +#define MAIL1_WRT (ARM_0_MAIL1 + 0x00) + +#define MBOX_CHAN_COUNT 16 + +/* Status register: FIFO state. */ +#define ARM_MS_FULL 0x80000000 +#define ARM_MS_EMPTY 0x40000000 + +/* Configuration register: Enable interrupts. */
drivers/mailbox/bcm2835-mailbox.c line 67
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c new file +10
-0+#define ARM_MC_IHAVEDATAIRQEN 0x00000001 + +#define MBOX_MSG (chan, data28) (((data28) & ~0xf) | ((chan) & 0xf)) +#define MBOX_CHAN (msg) ((msg) & 0xf) +#define MBOX_DATA28 (msg) ((msg) & ~0xf) + +struct bcm2835_mbox ; + +struct bcm2835_channel { +struct bcm2835_mbox *mbox;
Do you really want to put this in here?
Can't you use conatiner_of() instead?
drivers/mailbox/bcm2835-mailbox.c line 74
Quoted 7 lines
drivers/mailbox/bcm2835-mailbox.c new file +7
-0+struct mbox_chan *link; +u32 chan_num; +bool started; +}; + +struct bcm2835_mbox { +struct platform_device *pdev;
What are you using this for?
It's not standard to save the platform_device. Extract what you need out of it in probe(), then let it die.
drivers/mailbox/bcm2835-mailbox.c line 86
Quoted 12 lines
drivers/mailbox/bcm2835-mailbox.c new file +12
-0+struct device *dev; +void __iomem *regs; +spinlock_t lock; +struct bcm2835_channel channel[MBOX_CHAN_COUNT ]; +struct mbox_controller controller; +}; + +#define to_channel (link) ((struct bcm2835_channel *)link->con_priv) + +static irqreturn_t bcm2835_mbox_irq (int irq,void *dev_id) +{ +struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id;
No need to cast this.
drivers/mailbox/bcm2835-mailbox.c line 85
Quoted 1 line
drivers/mailbox/bcm2835-mailbox.c new file +1
-0+struct device *dev = mbox->dev;
Once the dev_dbg() has gone, you only use this once. Better to save a local variable and just use mbox->dev directly in dev_err().
drivers/mailbox/bcm2835-mailbox.c line 97
Quoted 9 lines
drivers/mailbox/bcm2835-mailbox.c new file +9
-0+while (!(readl (mbox->regs +MAIL0_STA ) &ARM_MS_EMPTY )) { +u32 msg =readl (mbox->regs +MAIL0_RD ); +unsigned int chan =MBOX_CHAN (msg); + +if (!mbox->channel[chan].started) { +dev_err (dev,"Reply on stopped channel %d\n" , chan); +continue ; + } +dev_dbg (dev,"Reply 0x%08X\n" , msg);
This file is riddled with dev_dbg() calls, which are great to have during development, but are of very little use once in production. They also make the place look untidy. Please remove them.
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +5
-0+mbox_chan_received_data (mbox->channel[chan].link, + (void *)MBOX_DATA28 (msg)); + } +rmb ();/* Finished last mailbox read. */ +return IRQ_HANDLED ;
drivers/mailbox/bcm2835-mailbox.c line 112
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c new file +10
-0+} + +static int bcm2835_send_data (struct mbox_chan *link,void *data) +{ +struct bcm2835_channel *chan =to_channel (link); +struct bcm2835_mbox *mbox = chan->mbox; +int ret =0 ; + +if (!chan->started) +return -ENODEV ;
Nice to put a '\n' here.
Quoted 44 lines
drivers/mailbox/bcm2835-mailbox.c new file +44
-0+spin_lock (&mbox->lock); +if (readl (mbox->regs +MAIL0_STA ) &ARM_MS_FULL ) { +rmb ();/* Finished last mailbox read. */ + ret = -EBUSY ; + goto end; + } +wmb ();/* About to write to the mail box. */ +writel (MBOX_MSG (chan->chan_num, (u32 ) data), mbox->regs +MAIL1_WRT ); +dev_dbg (mbox->dev,"Request 0x%08X\n" ,MBOX_MSG (chan->chan_num, + (u32 ) data)); +end: +spin_unlock (&mbox->lock); +return ret; +} + +static int bcm2835_startup (struct mbox_chan *link) +{ +struct bcm2835_channel *chan =to_channel (link); + + chan->started = true; +return 0 ; +} + +static void bcm2835_shutdown (struct mbox_chan *link) +{ +struct bcm2835_channel *chan =to_channel (link); + + chan->started = false; +} + +static bool bcm2835_last_tx_done (struct mbox_chan *link) +{ +struct bcm2835_channel *chan =to_channel (link); +struct bcm2835_mbox *mbox = chan->mbox; +bool ret; + +if (!chan->started) +return false; +spin_lock (&mbox->lock); + ret = !(readl (mbox->regs +MAIL0_STA ) &ARM_MS_FULL ); +rmb ();/* Finished last mailbox read. */ +spin_unlock (&mbox->lock); +return ret; +}
drivers/mailbox/bcm2835-mailbox.c line 166
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c new file +10
-0+ +static struct mbox_chan_ops bcm2835_mbox_chan_ops = { + .send_data = bcm2835_send_data, + .startup = bcm2835_startup, + .shutdown = bcm2835_shutdown, + .last_tx_done = bcm2835_last_tx_done +}; + +static int request_mailbox_iomem (struct bcm2835_mbox *mbox) +{
It would be better to pass pdev instead of mbox and use platform_get_drvdata() here.
drivers/mailbox/bcm2835-mailbox.c line 167
Quoted 1 line
drivers/mailbox/bcm2835-mailbox.c new file +1
-0+struct platform_device *pdev = mbox->pdev;
... then you mitigate the requirement to save pdev in bcm2835_mbox.
drivers/mailbox/bcm2835-mailbox.c line 172
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +5
-0+struct resource *iomem =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + +dev_dbg (&pdev->dev,"iomem 0x%08X-0x%08X\n" , iomem->start, iomem->end); + mbox->regs =devm_ioremap_resource (&pdev->dev, iomem); +dev_dbg (&pdev->dev,"registers at [0x%p]\n" , mbox->regs);
Remove all these, they're ugly!
drivers/mailbox/bcm2835-mailbox.c line 174
Quoted 2 lines
drivers/mailbox/bcm2835-mailbox.c new file +2
-0+if (IS_ERR (mbox->regs)) { +dev_err (&pdev->dev,"Failed to remap mailbox regs\n" );
Doesn't devm_ioremap_resource() already issue an error?
drivers/mailbox/bcm2835-mailbox.c line 186
Quoted 12 lines
drivers/mailbox/bcm2835-mailbox.c new file +12
-0+return PTR_ERR (mbox->regs); + } +dev_dbg (&pdev->dev,"iomem registered\n" ); +return 0 ; +} + +static int request_mailbox_irq (struct bcm2835_mbox *mbox) +{ +int ret; +struct device *dev = mbox->dev; +struct device_node *np = dev->of_node; +int irq =irq_of_parse_and_map (np,0 );
Separate the definition from the allocation here.
drivers/mailbox/bcm2835-mailbox.c line 188
Quoted 1 line
drivers/mailbox/bcm2835-mailbox.c new file +1
-0+if (irq <=0 ) {
Then bring this line up so it's directly under irq_of_*.
drivers/mailbox/bcm2835-mailbox.c line 186
Quoted 2 lines
drivers/mailbox/bcm2835-mailbox.c new file +2
-0+dev_err (dev,"Can't get IRQ number for mailbox\n" ); +return -ENODEV ;
Please return ret.
drivers/mailbox/bcm2835-mailbox.c line 196
Quoted 6 lines
drivers/mailbox/bcm2835-mailbox.c new file +6
-0+ } + ret =devm_request_irq (dev, irq, bcm2835_mbox_irq,0 ,dev_name (dev), + mbox); +if (ret) { +dev_err (dev,"Failed to register a mailbox IRQ handler\n" ); +return -ENODEV ;
You should be returning ret here.
drivers/mailbox/bcm2835-mailbox.c line 194
Quoted 2 lines
drivers/mailbox/bcm2835-mailbox.c new file +2
-0+ } +dev_dbg (dev,"Registered IRQ\n" );
Blurh...
drivers/mailbox/bcm2835-mailbox.c line 207
Quoted 9 lines
drivers/mailbox/bcm2835-mailbox.c new file +9
-0+return 0 ; +} + +static int bcm2835_mbox_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct bcm2835_mbox *mbox; +int i; +int ret =0 ;
No need to pre-allocate here.
drivers/mailbox/bcm2835-mailbox.c line 211
Quoted 3 lines
drivers/mailbox/bcm2835-mailbox.c new file +3
-0+dev_dbg (dev,"Probing\n" ); + mbox =devm_kzalloc (dev,sizeof (*mbox),GFP_KERNEL ); +if (mbox ==NULL ) {
if (!mbox)
drivers/mailbox/bcm2835-mailbox.c line 212
Quoted 1 line
drivers/mailbox/bcm2835-mailbox.c new file +1
-0+dev_err (dev,"Failed to allocate mailbox memory\n" );
Remove all -ENOMEM prints please.
I think checkpatch warns against this.
drivers/mailbox/bcm2835-mailbox.c line 216
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox.c new file +4
-0+return -ENOMEM ; + } +platform_set_drvdata (pdev, mbox); + mbox->pdev = pdev;
Remove this.
drivers/mailbox/bcm2835-mailbox.c line 223
Quoted 7 lines
drivers/mailbox/bcm2835-mailbox.c new file +7
-0+ mbox->dev = dev; +spin_lock_init (&mbox->lock); + +dev_dbg (dev,"Requesting IRQ\n" ); + ret =request_mailbox_irq (mbox); +if (ret) +return ret;
'\n'
Quoted 6 lines
drivers/mailbox/bcm2835-mailbox.c new file +6
-0+dev_dbg (dev,"Requesting iomem\n" ); + ret =request_mailbox_iomem (mbox); +if (ret) +return ret; + +dev_dbg (dev,"Initializing mailbox controller\n" );
drivers/mailbox/bcm2835-mailbox.c line 239
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c new file +10
-0+ mbox->controller.txdone_poll = true; + mbox->controller.txpoll_period =5 ; + mbox->controller.ops = &bcm2835_mbox_chan_ops; + mbox->controller.dev = dev; + mbox->controller.num_chans =MBOX_CHAN_COUNT ; + mbox->controller.chans =devm_kzalloc (dev, +sizeof (struct mbox_chan ) *MBOX_CHAN_COUNT , +GFP_KERNEL ); +if (!mbox->controller.chans) { +dev_err (dev,"Failed to alloc mbox_chans\n" );
Remove the print.
drivers/mailbox/bcm2835-mailbox.c line 252
Quoted 13 lines
drivers/mailbox/bcm2835-mailbox.c new file +13
-0+return -ENOMEM ; + } + +dev_dbg (dev,"Initializing mailbox channels\n" ); +for (i =0 ; i !=MBOX_CHAN_COUNT ; ++i) { + mbox->channel[i].mbox = mbox; + mbox->channel[i].link = &mbox->controller.chans[i]; + mbox->channel[i].chan_num = i; + mbox->controller.chans[i].con_priv = + (void *)&mbox->channel[i]; + } + + ret =mbox_controller_register (&mbox->controller);
Does this provide it's own print on error?
Quoted 13 lines
drivers/mailbox/bcm2835-mailbox.c new file +13
-0+if (ret) +return ret; + +/* Enable the interrupt on data reception */ +writel (ARM_MC_IHAVEDATAIRQEN , mbox->regs +MAIL0_CNF ); +dev_info (dev,"mailbox enabled\n" ); + +return ret; +} + +static int bcm2835_mbox_remove (struct platform_device *pdev) +{ +struct bcm2835_mbox *mbox =platform_get_drvdata (pdev);
drivers/mailbox/bcm2835-mailbox.c line 275
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c new file +10
-0+ +mbox_controller_unregister (&mbox->controller); +return 0 ; +} + +static const struct of_device_id bcm2835_mbox_of_match[] = { + { .compatible ="brcm,bcm2835-mbox" , }, + {}, +}; +MODULE_DEVICE_TABLE (of, bcm2835_mbox_of_match);
If you don't depend on OF, you need to protect this.
drivers/mailbox/bcm2835-mailbox.c line 280
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox.c new file +4
-0+static struct platform_driver bcm2835_mbox_driver = { + .driver = { + .name ="bcm2835-mbox" , + .owner =THIS_MODULE ,
Remove this, it's taken care of elsewhere.
drivers/mailbox/bcm2835-mailbox.c line 281
Quoted 1 line
drivers/mailbox/bcm2835-mailbox.c new file +1
-0+ .of_match_table = bcm2835_mbox_of_match,
of_match_ptr()
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c new file +10
-0+ }, + .probe = bcm2835_mbox_probe, + .remove = bcm2835_mbox_remove, +}; +module_platform_driver (bcm2835_mbox_driver); + +MODULE_AUTHOR ("Craig McGeachie" ); +MODULE_AUTHOR ("Lubomir Rintel <[email protected]>" ); +MODULE_DESCRIPTION ("BCM2835 mailbox IPC driver" ); +MODULE_LICENSE ("GPL v2" );
PATCH 1/1 mailbox: Enable BCM2835 mailbox support
Quoted 20 lines · Lee Jones
On Fri, 24 Oct 2014, Lubomir Rintel wrote:
Thanks for sending this to list, as you are aware a lot of the other platform drivers depend on this.
FYI: I don't know much about the Mailbox FW, so I'll leave the functional review to the people who do. This is going to be a coding stylesque review instead.
Quoted 10 lines
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.
Cc Stephen Warren <[email protected]>Cc Jassi Brar <[email protected]>Cc Lee Jones <[email protected]>Cc linux-rpi-kernel at lists.infradead.orgCc linux-arm-kernel at lists.infradead.org
Quoted 4 lines
As you're only sending a small patch-set, it's better to address the emails to these guys directly, rather than doing so in here. Cc's in commit messages are good if there are only a few patches within a large set to be sent to different addresses.
It's very useful for a submitter so they can work out the desired recipients once rather than repeating the process each time they run git send-email.
Perhaps the U-Boot patman tool might help; I believe it strips out such tags when sending email, so you get the benefit of storing metadata in the commit description locally, but it doesn't clutter the email that actually gets sent.
Quoted 3 lines
FWIW, you can drop Stephen and myself from Cc completely as we're both subscribed to the RPi list and it's very low volume, thus we're not likely to miss it.
Technically you're supposed to CC all the maintainers either way, to make sure the message shows up in their mailbox. You're right the list is low enough volume the message shouldn't be missed, but I de-dup message to multiple lists, so if I forgot to make the RPi list match higher than the ARM list match (which I admittedly haven't) I might miss it without a Cc.
Quoted 13 lines
drivers/mailbox/bcm2835-mailbox.c line 4
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c new file +4
-0@@ -0,0 +1,291 @@ +/* +* Copyright (C) 2010 Broadcom +* Copyright (C) 2013-2014 Lubomir Rintel +* Copyright (C) 2013 Craig McGeachie Better to put these in chronological order.
Quoted 1 line
Does the Copyright really belong to you?
Assuming Lubomir made any non-trival changes, he certainly can claim (c) (on parts of the file at least).
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c line 14
Quoted 2 lines
drivers/mailbox/bcm2835-mailbox.c new file +2
-0+* Parts of the driver are based on: +* - arch/arm/mach-bcm2708/vcio.c file written by Gray Girling that was This doesn't seem very future-proof.
drivers/mailbox/bcm2835-mailbox.c line 16
Quoted 2 lines
drivers/mailbox/bcm2835-mailbox.c new file +2
-0+* obtained from branch "rpi-3.6.y" of git://github.com/raspberrypi/ +* linux.git Drop all this please.
Quoted 7 lines
drivers/mailbox/bcm2835-mailbox.c line 21
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +5
-0+* - drivers/mailbox/bcm2835-ipc.c by Lubomir Rintel at +* https://github.com/hackerspace/rpi-linux/blob/lr-raspberry-pi/drivers/ +* mailbox/bcm2835-ipc.c +* - documentation available on the following web site: +* https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface And this. Please don't put links to external Git repos in drivers.
Hmmm. It can be quite useful to track down the history of the driver. If not mentioned in the comment, it'd be useful to mention this all in the commit description at least.
Quoted 5 lines
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +5
-0+while (!(readl (mbox->regs +MAIL0_STA ) &ARM_MS_EMPTY )) { +u32 msg =readl (mbox->regs +MAIL0_RD ); +unsigned int chan =MBOX_CHAN (msg); + +if (!mbox->channel[chan].started) {
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c line 97
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox.c new file +4
-0+dev_err (dev,"Reply on stopped channel %d\n" , chan); +continue ; + } +dev_dbg (dev,"Reply 0x%08X\n" , msg);This file is riddled with
dev_dbg()calls, which are great to have during development, but are of very little use once in production. They also make the place look untidy. Please remove them.
They don't print at run-time unless debugging is enabled for this driver. Admittedly some of the dev_dbg() aren't that useful, but I think tracing the sent/received messages could be quite useful outside of the initial mailbox driver development, e.g. so that every person developing a driver that uses the mailbox functionality doesn't have to implement their own mailbox tracing mechanism. Perhaps the mailbox core provides this already though?
Quoted 7 lines
drivers/mailbox/bcm2835-mailbox.c line 275
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +5
-0+static const struct of_device_id bcm2835_mbox_of_match[] = { + { .compatible ="brcm,bcm2835-mbox" , }, + {}, +}; +MODULE_DEVICE_TABLE (of, bcm2835_mbox_of_match);If you don't depend on OF, you need to protect this.
I've never seen that protected before, but perhaps because I've only worked on platforms where OF was unconditionally present. If this is an issue, I think it'd be better if the driver depended on OF for now, since there's no reason to expect the driver to work on a system without OF, since bcm2835 should always have OF.
PATCH 1/1 mailbox: Enable BCM2835 mailbox support
Quoted 21 lines · Stephen Warren
Quoted 20 lines · Lee Jones
On Fri, 24 Oct 2014, Lubomir Rintel wrote:
Thanks for sending this to list, as you are aware a lot of the other platform drivers depend on this.
FYI: I don't know much about the Mailbox FW, so I'll leave the functional review to the people who do. This is going to be a coding stylesque review instead.
Quoted 10 lines
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.
Cc Stephen Warren <[email protected]>Cc Jassi Brar <[email protected]>Cc Lee Jones <[email protected]>Cc linux-rpi-kernel at lists.infradead.orgCc linux-arm-kernel at lists.infradead.org
Quoted 8 lines
Quoted 4 lines
As you're only sending a small patch-set, it's better to address the emails to these guys directly, rather than doing so in here. Cc's in commit messages are good if there are only a few patches within a large set to be sent to different addresses.
It's very useful for a submitter so they can work out the desired recipients once rather than repeating the process each time they run git send-email.
I do understand the convienience, but IMHO it's an abuse of the function. The Cc: tag is designed to indicate that a person has been given the opotunity to comment on a patch, but has failed to do so:
Quoted 12 lines
Documentation/SubmittingPatches:"13) When to use Acked-by: and Cc:
[...]
If a person has had the opportunity to comment on a patch, but has not provided such comments, you may optionally add a "Cc:" tag to the patch. This is the only tag which might be added without an explicit action by the person it names. This tag documents that potentially interested parties have been included in the discussion"
Quoted 4 lines
Documentation/development-process/5.Posting:"- Cc: the named person received a copy of the patch and had the
opportunity to comment on it."
I also quite like 'extending' the feature to Cc people on a sub-set of a patch-set who might not necessarily want to be bombarded with the whole thing to lessen the burden on them; however, I do so sparingly and with forethought.
If I had to strip out Cc: tags on every patch I applied, it would drive me crazy.
Quoted 4 lines
Perhaps the U-Boot patman tool might help; I believe it strips out such tags when sending email, so you get the benefit of storing metadata in the commit description locally, but it doesn't clutter the email that actually gets sent.
Perfect.
Quoted 10 lines
Quoted 3 lines
FWIW, you can drop Stephen and myself from Cc completely as we're both subscribed to the RPi list and it's very low volume, thus we're not likely to miss it.
Technically you're supposed to CC all the maintainers either way, to make sure the message shows up in their mailbox. You're right the list is low enough volume the message shouldn't be missed, but I de-dup message to multiple lists, so if I forgot to make the RPi list match higher than the ARM list match (which I admittedly haven't) I might miss it without a Cc.
You're right of course. I was being a little selfish there and assumed everyone sets up their mail in the same way as I do, therefore it would not be missed. Please continue to Cc interested parties directly.
Quoted 13 lines
Quoted 13 lines
drivers/mailbox/bcm2835-mailbox.c line 4
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c new file +4
-0@@ -0,0 +1,291 @@ +/* +* Copyright (C) 2010 Broadcom +* Copyright (C) 2013-2014 Lubomir Rintel +* Copyright (C) 2013 Craig McGeachie Better to put these in chronological order.
Quoted 4 lines
Quoted 1 line
Does the Copyright really belong to you?
Assuming Lubomir made any non-trival changes, he certainly can claim (c) (on parts of the file at least).
Interesting, I didn't know it worked that way. Thanks for answering.
Quoted 16 lines
Quoted 16 lines
drivers/mailbox/bcm2835-mailbox.c line 14
Quoted 2 lines
drivers/mailbox/bcm2835-mailbox.c new file +2
-0+* Parts of the driver are based on: +* - arch/arm/mach-bcm2708/vcio.c file written by Gray Girling that was This doesn't seem very future-proof.
drivers/mailbox/bcm2835-mailbox.c line 16
Quoted 2 lines
drivers/mailbox/bcm2835-mailbox.c new file +2
-0+* obtained from branch "rpi-3.6.y" of git://github.com/raspberrypi/ +* linux.git Drop all this please.
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +5
-0+* - drivers/mailbox/bcm2835-ipc.c by Lubomir Rintel at +* https://github.com/hackerspace/rpi-linux/blob/lr-raspberry-pi/drivers/ +* mailbox/bcm2835-ipc.c +* - documentation available on the following web site: +* https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
Quoted 5 lines
Quoted 1 line
And this. Please don't put links to external Git repos in drivers.
Hmmm. It can be quite useful to track down the history of the driver. If not mentioned in the comment, it'd be useful to mention this all in the commit description at least.
I'll not disagree with the point that the history of the driver can be useful, and by all means give credit to original authors (although
Authors: and the (c) do a pretty good job of that already); however, doing so by providing URLs to personal volatile Git trees is not the ideal method for doing so.
Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +5
-0+while (!(readl (mbox->regs +MAIL0_STA ) &ARM_MS_EMPTY )) { +u32 msg =readl (mbox->regs +MAIL0_RD ); +unsigned int chan =MBOX_CHAN (msg); + +if (!mbox->channel[chan].started) {Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c line 97
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox.c new file +4
-0+dev_err (dev,"Reply on stopped channel %d\n" , chan); +continue ; + } +dev_dbg (dev,"Reply 0x%08X\n" , msg);This file is riddled with
dev_dbg()calls, which are great to have during development, but are of very little use once in production. They also make the place look untidy. Please remove them.
Quoted 7 lines
They don't print at run-time unless debugging is enabled for this driver. Admittedly some of the
dev_dbg()aren't that useful, but I think tracing the sent/received messages could be quite useful outside of the initial mailbox driver development, e.g. so that every person developing a driver that uses the mailbox functionality doesn't have to implement their own mailbox tracing mechanism. Perhaps the mailbox core provides this already though?
I'm not complaining that the bootlog will be filled, but I am picking up on the overuse of uninteresting debug prints littering the code. Of the 11 provided, probably 2 of them are useful, and as you say these should probably live in the framework.
Quoted 10 lines
Quoted 7 lines
drivers/mailbox/bcm2835-mailbox.c line 275
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c new file +5
-0+static const struct of_device_id bcm2835_mbox_of_match[] = { + { .compatible ="brcm,bcm2835-mbox" , }, + {}, +}; +MODULE_DEVICE_TABLE (of, bcm2835_mbox_of_match);If you don't depend on OF, you need to protect this.
I've never seen that protected before, but perhaps because I've only worked on platforms where OF was unconditionally present.
You have been blessed indeed. :)
Quoted 4 lines
If this is an
issue, I think it'd be better if the driver depended on OF for now, since there's no reason to expect the driver to work on a system without OF, since bcm2835 should always have OF.
So normally we use of_match_ptr() to NULLify the pointer to this struct if OF isn't enabled, so IMHO it stands to reason that it would be a good idea to not delcare/initialise the struct.
FYI:
$ git grep -C1 of_device_id | grep CONFIG_OF | wc -l 250
Kind regards,
Lee
PATCH 1/1 mailbox: Enable BCM2835 mailbox support
In reply to Lubomir RintelQuoted 8 lines · Lubomir Rintel
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
[lkundrak at v3.sk: Tidied up Squashed Craig's work for review, carried over to new version of Mailbox framework]
Signed-off-by Craig McGeachie <[email protected]>Signed-off-by Suman Anna <[email protected]>Signed-off-by Jassi Brar <[email protected]>Signed-off-by Lubomir Rintel <[email protected]>
It's a bit odd that the patch author is you if all those other people signed off the patch before you. Given that list of S-o-b, I'd have expected to see the following at the start of the email:
From: Craig McGeachie <[email protected]>
... either that, or say "based on work by ..." rather than s-o-b all those other people?
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt line 7
Quoted 7 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt +5
-0+Required properties: + +- compatible : should be "brcm,bcm2835-mbox" +- reg : Specifies base physical address and size of the registers. +- interrupts : the interrupt number. Must be <0 1>, the mailbox interrupt.
The DT binding should only specify exact values for properties that this HW module owns. The value of interrupts is owned by the IRQ controller that this module's IRQ output is sent to. This binding should say simple like:
- interrupts : The module's interrupt output.
See ../interrupt-controller/interrupts for details of the format.
Can you please make sure there aren't any other interrupts, clocks, resets, or other resources that should be listed here. DT works better if you list all the resources from the start, even if the current Linux driver doesn't make use of some of them yet.
drivers/mailbox/Kconfig config OMAP_MBOX_KFIFO_SIZE · line 42
Quoted 8 lines
drivers/mailbox/Kconfig +6
-0+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
"Y or M" since this is tristate?
drivers/mailbox/Makefile obj-$(CONFIG_MAILBOX) += mailbox.o · line 8
Quoted 4 lines
drivers/mailbox/Makefile +1
-0obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o +obj-$(CONFIG_BCM2835_MBOX) += bcm2835-mailbox.o
Alphabetical order?
drivers/mailbox/bcm2835-mailbox.c line 43
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c +3
-0+/* Mailbox registers. We basically only support mailbox 0 & 1. We deliver to +* the VC in mailbox 1, it delivers to us in mailbox 0. See BCM2835 ARM +* Peripherals section 1.3 for an explanation about the placement of memory
Rather than "BCM2835 ARM Peripherals", perhaps use the exact filename so it's obvious what the name refers to.
Quoted 1 line
+static int
request_mailbox_irq(struct bcm2835_mbox *mbox)
...
drivers/mailbox/bcm2835-mailbox.c line 193
Quoted 2 lines
drivers/mailbox/bcm2835-mailbox.c +2
-0+ ret =devm_request_irq (dev, irq, bcm2835_mbox_irq,0 ,dev_name (dev), + mbox);
Are you absolutely sure that using devm_request_irq() is safe? It's only safe if you can absolutely guarantee that the IRQ won't fire after remove() (or rather, whenever the resources required for the IRQ handler to run without crashing/failing) and before the devm core actually unregisters the IRQ handler. If this can't be guaranteed, explicit management of the IRQ is required.
drivers/mailbox/bcm2835-mailbox.c line 289
Quoted 2 lines
drivers/mailbox/bcm2835-mailbox.c +2
-0+MODULE_AUTHOR ("Craig McGeachie" ); +MODULE_AUTHOR ("Lubomir Rintel <[email protected]>" );
I have no idea if multiple MODULE_AUTHOR()s work. Can you make sure they do.
This driver doesn't seem to handle the property mailbox channel, which would involve additional handling of the memory buffers used to transfer the associated messages. Perhaps you're expecting some higher-level driver to sit on top of this driver for that purpose?
Having a separate device/driver might be problematic, since there's only the single mailbox HW module in the chip, and DT should mostly only nodes for actual physical HW modules. Perhaps we could make this driver register the device object for any separate driver though. Even then, you'd have to make sure that the #mbox-cells value was enough to refer to the property channel in the future.
Some functionality is only available through the property mailbox protocol, rather than the separate dedicated channels, so it's important to allow for that functionality.