| Message ID | 1414156789-30188-1-git-send-email-lkundrak@v3.sk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Lubomir, Am 24.10.2014 um 15:19 schrieb 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 > > [lkundrak@v3.sk: Tidied up Squashed Craig's work for review, carried over > to new version of Mailbox framework] > > Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au> > Signed-off-by: Suman Anna <s-anna@ti.com> > Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> > Cc: Stephen Warren <swarren@wwwdotorg.org> > Cc: Jassi Brar <jassisinghbrar@gmail.com> > Cc: Lee Jones <lee.jones@linaro.org> > Cc: linux-rpi-kernel@lists.infradead.org > Cc: linux-arm-kernel@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.c > > diff --git a/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt > new file mode 100644 > index 0000000..68d761a > --- /dev/null > +++ b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt > @@ -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@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. > [...] > > diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c > new file mode 100644 > index 0000000..5a37ac2 > --- /dev/null > +++ b/drivers/mailbox/bcm2835-mailbox.c > @@ -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> > +#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. > [...] > > + > +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
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. > 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@v3.sk: Tidied up Squashed Craig's work for review, carried over > to new version of Mailbox framework] > > Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au> > Signed-off-by: Suman Anna <s-anna@ti.com> > Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> 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? > Cc: Stephen Warren <swarren@wwwdotorg.org> > Cc: Jassi Brar <jassisinghbrar@gmail.com> > Cc: Lee Jones <lee.jones@linaro.org> > Cc: linux-rpi-kernel@lists.infradead.org > Cc: linux-arm-kernel@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. > --- > .../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 > 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 > > diff --git a/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt > new file mode 100644 > index 0000000..68d761a > --- /dev/null > +++ b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt > @@ -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. > +Example: > + > +mailbox: mailbox@0x7e00b800 { Drop the 0x > + compatible = "brcm,bcm2835-mbox"; > + reg = <0x7e00b880 0x40>; > + interrupts = <0 1>; > + #mbox-cells = <1>; > +}; > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig > index 9fd9c67..795d2fc 100644 > --- a/drivers/mailbox/Kconfig > +++ b/drivers/mailbox/Kconfig > @@ -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? > + help > + An implementation of the BCM2385 Mailbox. It is used to invoke > + the services of the Videocore. Say Y here if you want to use the > + BCM2835 Mailbox. > + > endif > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile > index 94ed7ce..e29e4d4 100644 > --- a/drivers/mailbox/Makefile > +++ b/drivers/mailbox/Makefile > @@ -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 > diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c > new file mode 100644 > index 0000000..5a37ac2 > --- /dev/null > +++ b/drivers/mailbox/bcm2835-mailbox.c > @@ -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? > + * 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. > + * obtained from branch "rpi-3.6.y" of git://github.com/raspberrypi/ > + * linux.git Drop all this please. > + * - 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. > + */ > + > +#include <linux/module.h> > +#include <linux/interrupt.h> > +#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. > +/* 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 > +#define MAIL0_RD (ARM_0_MAIL0 + 0x00) > +#define MAIL0_POL (ARM_0_MAIL0 + 0x10) > +#define MAIL0_STA (ARM_0_MAIL0 + 0x18) > +#define MAIL0_CNF (ARM_0_MAIL0 + 0x1C) > +#define MAIL1_WRT (ARM_0_MAIL1 + 0x00) > + > +#define MBOX_CHAN_COUNT 16 > + > +/* Status register: FIFO state. */ > +#define ARM_MS_FULL 0x80000000 > +#define ARM_MS_EMPTY 0x40000000 > + > +/* Configuration register: Enable interrupts. */ > +#define ARM_MC_IHAVEDATAIRQEN 0x00000001 > + > +#define MBOX_MSG(chan, data28) (((data28) & ~0xf) | ((chan) & 0xf)) > +#define MBOX_CHAN(msg) ((msg) & 0xf) > +#define MBOX_DATA28(msg) ((msg) & ~0xf) > + > +struct bcm2835_mbox; > + > +struct bcm2835_channel { > + struct bcm2835_mbox *mbox; Do you really want to put this in here? Can't you use conatiner_of() instead? > + 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. > + 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. > + 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(). > + 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. > + mbox_chan_received_data(mbox->channel[chan].link, > + (void *) MBOX_DATA28(msg)); > + } > + rmb(); /* Finished last mailbox read. */ > + return IRQ_HANDLED; > +} > + > +static int bcm2835_send_data(struct mbox_chan *link, void *data) > +{ > + struct bcm2835_channel *chan = to_channel(link); > + struct bcm2835_mbox *mbox = chan->mbox; > + int ret = 0; > + > + if (!chan->started) > + return -ENODEV; Nice to put a '\n' here. > + spin_lock(&mbox->lock); > + if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) { > + rmb(); /* Finished last mailbox read. */ > + ret = -EBUSY; > + goto end; > + } > + wmb(); /* About to write to the mail box. */ > + writel(MBOX_MSG(chan->chan_num, (u32) data), mbox->regs + MAIL1_WRT); > + dev_dbg(mbox->dev, "Request 0x%08X\n", MBOX_MSG(chan->chan_num, > + (u32) data)); > +end: > + spin_unlock(&mbox->lock); > + return ret; > +} > + > +static int bcm2835_startup(struct mbox_chan *link) > +{ > + struct bcm2835_channel *chan = to_channel(link); > + > + chan->started = true; > + return 0; > +} > + > +static void bcm2835_shutdown(struct mbox_chan *link) > +{ > + struct bcm2835_channel *chan = to_channel(link); > + > + chan->started = false; > +} > + > +static bool bcm2835_last_tx_done(struct mbox_chan *link) > +{ > + struct bcm2835_channel *chan = to_channel(link); > + struct bcm2835_mbox *mbox = chan->mbox; > + bool ret; > + > + if (!chan->started) > + return false; > + spin_lock(&mbox->lock); > + ret = !(readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL); > + rmb(); /* Finished last mailbox read. */ > + spin_unlock(&mbox->lock); > + return ret; > +} > + > +static struct mbox_chan_ops bcm2835_mbox_chan_ops = { > + .send_data = bcm2835_send_data, > + .startup = bcm2835_startup, > + .shutdown = bcm2835_shutdown, > + .last_tx_done = bcm2835_last_tx_done > +}; > + > +static int request_mailbox_iomem(struct bcm2835_mbox *mbox) > +{ It would be better to pass pdev instead of mbox and use platform_get_drvdata() here. > + struct platform_device *pdev = mbox->pdev; ... then you mitigate the requirement to save pdev in bcm2835_mbox. > + 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! > + if (IS_ERR(mbox->regs)) { > + dev_err(&pdev->dev, "Failed to remap mailbox regs\n"); Doesn't devm_ioremap_resource() already issue an error? > + 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. > + if (irq <= 0) { Then bring this line up so it's directly under irq_of_*. > + dev_err(dev, "Can't get IRQ number for mailbox\n"); > + return -ENODEV; Please return ret. > + } > + 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. > + } > + dev_dbg(dev, "Registered IRQ\n"); Blurh... > + 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. > + dev_dbg(dev, "Probing\n"); > + mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL); > + if (mbox == NULL) { if (!mbox) > + dev_err(dev, "Failed to allocate mailbox memory\n"); Remove all -ENOMEM prints please. I think checkpatch warns against this. > + return -ENOMEM; > + } > + platform_set_drvdata(pdev, mbox); > + mbox->pdev = pdev; Remove this. > + mbox->dev = dev; > + spin_lock_init(&mbox->lock); > + > + dev_dbg(dev, "Requesting IRQ\n"); > + ret = request_mailbox_irq(mbox); > + if (ret) > + return ret; '\n' > + dev_dbg(dev, "Requesting iomem\n"); > + ret = request_mailbox_iomem(mbox); > + if (ret) > + return ret; > + > + dev_dbg(dev, "Initializing mailbox controller\n"); > + 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. > + 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? > + if (ret) > + return ret; > + > + /* Enable the interrupt on data reception */ > + writel(ARM_MC_IHAVEDATAIRQEN, mbox->regs + MAIL0_CNF); > + dev_info(dev, "mailbox enabled\n"); > + > + return ret; > +} > + > +static int bcm2835_mbox_remove(struct platform_device *pdev) > +{ > + struct bcm2835_mbox *mbox = platform_get_drvdata(pdev); > + > + mbox_controller_unregister(&mbox->controller); > + return 0; > +} > + > +static const struct of_device_id bcm2835_mbox_of_match[] = { > + { .compatible = "brcm,bcm2835-mbox", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, bcm2835_mbox_of_match); If you don't depend on OF, you need to protect this. > +static struct platform_driver bcm2835_mbox_driver = { > + .driver = { > + .name = "bcm2835-mbox", > + .owner = THIS_MODULE, Remove this, it's taken care of elsewhere. > + .of_match_table = bcm2835_mbox_of_match, of_match_ptr() > + }, > + .probe = bcm2835_mbox_probe, > + .remove = bcm2835_mbox_remove, > +}; > +module_platform_driver(bcm2835_mbox_driver); > + > +MODULE_AUTHOR("Craig McGeachie"); > +MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>"); > +MODULE_DESCRIPTION("BCM2835 mailbox IPC driver"); > +MODULE_LICENSE("GPL v2");
On 10/24/2014 07:19 AM, Lubomir Rintel wrote: > 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@v3.sk: Tidied up Squashed Craig's work for review, carried over > to new version of Mailbox framework] > > Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au> > Signed-off-by: Suman Anna <s-anna@ti.com> > Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> 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 <slapdau@yahoo.com.au> ... either that, or say "based on work by ..." rather than s-o-b all those other people? > diff --git a/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt > +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. > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig > +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? > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile > obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o > +obj-$(CONFIG_BCM2835_MBOX) += bcm2835-mailbox.o Alphabetical order? > diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c > +/* 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. > +static int request_mailbox_irq(struct bcm2835_mbox *mbox) ... > + 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. > +MODULE_AUTHOR("Craig McGeachie"); > +MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>"); 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.
On 10/24/2014 09:51 AM, Lee Jones wrote: > 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. > >> 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 <swarren@wwwdotorg.org> >> Cc: Jassi Brar <jassisinghbrar@gmail.com> >> Cc: Lee Jones <lee.jones@linaro.org> >> Cc: linux-rpi-kernel@lists.infradead.org >> Cc: linux-arm-kernel@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. 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. > 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. >> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c >> new file mode 100644 >> index 0000000..5a37ac2 >> --- /dev/null >> +++ b/drivers/mailbox/bcm2835-mailbox.c >> @@ -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? Assuming Lubomir made any non-trival changes, he certainly can claim (c) (on parts of the file at least). >> + * 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. > >> + * obtained from branch "rpi-3.6.y" of git://github.com/raspberrypi/ >> + * linux.git > > Drop all this please. > >> + * - 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. >> + 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. 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? >> +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.
On Sat, 25 Oct 2014, Stephen Warren wrote: > On 10/24/2014 09:51 AM, Lee Jones wrote: > > 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. > > > >> 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 <swarren@wwwdotorg.org> > >> Cc: Jassi Brar <jassisinghbrar@gmail.com> > >> Cc: Lee Jones <lee.jones@linaro.org> > >> Cc: linux-rpi-kernel@lists.infradead.org > >> Cc: linux-arm-kernel@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. > > 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: > 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" > > 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. > 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. > > 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. > >> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c > >> new file mode 100644 > >> index 0000000..5a37ac2 > >> --- /dev/null > >> +++ b/drivers/mailbox/bcm2835-mailbox.c > >> @@ -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? > > 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. > >> + * 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. > > > >> + * obtained from branch "rpi-3.6.y" of git://github.com/raspberrypi/ > >> + * linux.git > > > > Drop all this please. > > > >> + * - 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. 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. > >> + 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. > > 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. > >> +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. :) > 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
diff --git a/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file mode 100644 index 0000000..68d761a --- /dev/null +++ b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt @@ -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@0x7e00b800 { + compatible = "brcm,bcm2835-mbox"; + reg = <0x7e00b880 0x40>; + interrupts = <0 1>; + #mbox-cells = <1>; +}; diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index 9fd9c67..795d2fc 100644 --- a/drivers/mailbox/Kconfig +++ b/drivers/mailbox/Kconfig @@ -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 diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index 94ed7ce..e29e4d4 100644 --- a/drivers/mailbox/Makefile +++ b/drivers/mailbox/Makefile @@ -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 diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c new file mode 100644 index 0000000..5a37ac2 --- /dev/null +++ b/drivers/mailbox/bcm2835-mailbox.c @@ -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> +#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> + +/* 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. */ +#define MAIL0_RD (ARM_0_MAIL0 + 0x00) +#define MAIL0_POL (ARM_0_MAIL0 + 0x10) +#define MAIL0_STA (ARM_0_MAIL0 + 0x18) +#define MAIL0_CNF (ARM_0_MAIL0 + 0x1C) +#define MAIL1_WRT (ARM_0_MAIL1 + 0x00) + +#define MBOX_CHAN_COUNT 16 + +/* Status register: FIFO state. */ +#define ARM_MS_FULL 0x80000000 +#define ARM_MS_EMPTY 0x40000000 + +/* Configuration register: Enable interrupts. */ +#define ARM_MC_IHAVEDATAIRQEN 0x00000001 + +#define MBOX_MSG(chan, data28) (((data28) & ~0xf) | ((chan) & 0xf)) +#define MBOX_CHAN(msg) ((msg) & 0xf) +#define MBOX_DATA28(msg) ((msg) & ~0xf) + +struct bcm2835_mbox; + +struct bcm2835_channel { + struct bcm2835_mbox *mbox; + struct mbox_chan *link; + u32 chan_num; + bool started; +}; + +struct bcm2835_mbox { + struct platform_device *pdev; + struct device *dev; + void __iomem *regs; + spinlock_t lock; + struct bcm2835_channel channel[MBOX_CHAN_COUNT]; + struct mbox_controller controller; +}; + +#define to_channel(link) ((struct bcm2835_channel *)link->con_priv) + +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) +{ + struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; + struct device *dev = mbox->dev; + + while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) { + u32 msg = readl(mbox->regs + MAIL0_RD); + unsigned int chan = MBOX_CHAN(msg); + + if (!mbox->channel[chan].started) { + dev_err(dev, "Reply on stopped channel %d\n", chan); + continue; + } + dev_dbg(dev, "Reply 0x%08X\n", msg); + mbox_chan_received_data(mbox->channel[chan].link, + (void *) MBOX_DATA28(msg)); + } + rmb(); /* Finished last mailbox read. */ + return IRQ_HANDLED; +} + +static int bcm2835_send_data(struct mbox_chan *link, void *data) +{ + struct bcm2835_channel *chan = to_channel(link); + struct bcm2835_mbox *mbox = chan->mbox; + int ret = 0; + + if (!chan->started) + return -ENODEV; + spin_lock(&mbox->lock); + if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) { + rmb(); /* Finished last mailbox read. */ + ret = -EBUSY; + goto end; + } + wmb(); /* About to write to the mail box. */ + writel(MBOX_MSG(chan->chan_num, (u32) data), mbox->regs + MAIL1_WRT); + dev_dbg(mbox->dev, "Request 0x%08X\n", MBOX_MSG(chan->chan_num, + (u32) data)); +end: + spin_unlock(&mbox->lock); + return ret; +} + +static int bcm2835_startup(struct mbox_chan *link) +{ + struct bcm2835_channel *chan = to_channel(link); + + chan->started = true; + return 0; +} + +static void bcm2835_shutdown(struct mbox_chan *link) +{ + struct bcm2835_channel *chan = to_channel(link); + + chan->started = false; +} + +static bool bcm2835_last_tx_done(struct mbox_chan *link) +{ + struct bcm2835_channel *chan = to_channel(link); + struct bcm2835_mbox *mbox = chan->mbox; + bool ret; + + if (!chan->started) + return false; + spin_lock(&mbox->lock); + ret = !(readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL); + rmb(); /* Finished last mailbox read. */ + spin_unlock(&mbox->lock); + return ret; +} + +static struct mbox_chan_ops bcm2835_mbox_chan_ops = { + .send_data = bcm2835_send_data, + .startup = bcm2835_startup, + .shutdown = bcm2835_shutdown, + .last_tx_done = bcm2835_last_tx_done +}; + +static int request_mailbox_iomem(struct bcm2835_mbox *mbox) +{ + struct platform_device *pdev = mbox->pdev; + struct resource *iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + + dev_dbg(&pdev->dev, "iomem 0x%08X-0x%08X\n", iomem->start, iomem->end); + mbox->regs = devm_ioremap_resource(&pdev->dev, iomem); + dev_dbg(&pdev->dev, "registers at [0x%p]\n", mbox->regs); + if (IS_ERR(mbox->regs)) { + dev_err(&pdev->dev, "Failed to remap mailbox regs\n"); + 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); + + if (irq <= 0) { + dev_err(dev, "Can't get IRQ number for mailbox\n"); + return -ENODEV; + } + ret = devm_request_irq(dev, irq, bcm2835_mbox_irq, 0, dev_name(dev), + mbox); + if (ret) { + dev_err(dev, "Failed to register a mailbox IRQ handler\n"); + return -ENODEV; + } + dev_dbg(dev, "Registered IRQ\n"); + 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; + + dev_dbg(dev, "Probing\n"); + mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL); + if (mbox == NULL) { + dev_err(dev, "Failed to allocate mailbox memory\n"); + return -ENOMEM; + } + platform_set_drvdata(pdev, mbox); + mbox->pdev = pdev; + mbox->dev = dev; + spin_lock_init(&mbox->lock); + + dev_dbg(dev, "Requesting IRQ\n"); + ret = request_mailbox_irq(mbox); + if (ret) + return ret; + dev_dbg(dev, "Requesting iomem\n"); + ret = request_mailbox_iomem(mbox); + if (ret) + return ret; + + dev_dbg(dev, "Initializing mailbox controller\n"); + mbox->controller.txdone_poll = true; + mbox->controller.txpoll_period = 5; + mbox->controller.ops = &bcm2835_mbox_chan_ops; + mbox->controller.dev = dev; + mbox->controller.num_chans = MBOX_CHAN_COUNT; + mbox->controller.chans = devm_kzalloc(dev, + sizeof(struct mbox_chan) * MBOX_CHAN_COUNT, + GFP_KERNEL); + if (!mbox->controller.chans) { + dev_err(dev, "Failed to alloc mbox_chans\n"); + return -ENOMEM; + } + + 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); + if (ret) + return ret; + + /* Enable the interrupt on data reception */ + writel(ARM_MC_IHAVEDATAIRQEN, mbox->regs + MAIL0_CNF); + dev_info(dev, "mailbox enabled\n"); + + return ret; +} + +static int bcm2835_mbox_remove(struct platform_device *pdev) +{ + struct bcm2835_mbox *mbox = platform_get_drvdata(pdev); + + mbox_controller_unregister(&mbox->controller); + return 0; +} + +static const struct of_device_id bcm2835_mbox_of_match[] = { + { .compatible = "brcm,bcm2835-mbox", }, + {}, +}; +MODULE_DEVICE_TABLE(of, bcm2835_mbox_of_match); + +static struct platform_driver bcm2835_mbox_driver = { + .driver = { + .name = "bcm2835-mbox", + .owner = THIS_MODULE, + .of_match_table = bcm2835_mbox_of_match, + }, + .probe = bcm2835_mbox_probe, + .remove = bcm2835_mbox_remove, +}; +module_platform_driver(bcm2835_mbox_driver); + +MODULE_AUTHOR("Craig McGeachie"); +MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>"); +MODULE_DESCRIPTION("BCM2835 mailbox IPC driver"); +MODULE_LICENSE("GPL v2");