Message ID | 1430173644-19099-3-git-send-email-eric@anholt.net (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, Apr 28, 2015 at 3:57 AM, Eric Anholt <eric@anholt.net> wrote: > From: Lubomir Rintel <lkundrak@v3.sk> > > Implement BCM2835 mailbox support as a device registered with the > general purpose mailbox framework. Implementation based on commits by > Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base the > implementation. > > [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html > [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html > We could probably drop the history from changelog. Just talk about what the controller is and any specifics of the driver. > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> > Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au> > Signed-off-by: Suman Anna <s-anna@ti.com> > Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com> > How come it has my s-o-b? > diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c > new file mode 100644 > index 0000000..6910c71 > --- /dev/null > +++ b/drivers/mailbox/bcm2835-mailbox.c > @@ -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 > + > +/* Status register: FIFO state. */ > +#define ARM_MS_FULL 0x80000000 > +#define ARM_MS_EMPTY 0x40000000 > nit: BIT(31) and BIT(30) > + > +/* Configuration register: Enable interrupts. */ > +#define ARM_MC_IHAVEDATAIRQEN 0x00000001 > nit: BIT(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. > +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' > + 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() > +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. > + if (!mbox->started) > + return -ENODEV; > This 'started' flag is unnecessary. API won't call send_data() before startup() or after shutdown(). > + 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.
On Tue, 2015-04-28 at 11:13 +0530, Jassi Brar wrote: > On Tue, Apr 28, 2015 at 3:57 AM, Eric Anholt <eric@anholt.net> wrote: > > From: Lubomir Rintel <lkundrak@v3.sk> > > > > Implement BCM2835 mailbox support as a device registered with the > > general purpose mailbox framework. Implementation based on commits > > by > > Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base > > the > > implementation. > > > > [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013 > > -April/000528.html > > [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013 > > -May/000546.html > > > We could probably drop the history from changelog. Just talk about > what the controller is and any specifics of the driver. > > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> > > Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au> > > Signed-off-by: Suman Anna <s-anna@ti.com> > > Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com> > > > > 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
Jassi Brar <jassisinghbrar@gmail.com> writes: > On Tue, Apr 28, 2015 at 3:57 AM, Eric Anholt <eric@anholt.net> wrote: >> From: Lubomir Rintel <lkundrak@v3.sk> >> >> Implement BCM2835 mailbox support as a device registered with the >> general purpose mailbox framework. Implementation based on commits by >> Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base the >> implementation. >> >> [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html >> [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html >> > We 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." > >> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> >> Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au> >> Signed-off-by: Suman Anna <s-anna@ti.com> >> Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com> >> > 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. >> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c >> new file mode 100644 >> index 0000000..6910c71 >> --- /dev/null >> +++ b/drivers/mailbox/bcm2835-mailbox.c >> @@ -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. >> +/* Status register: FIFO state. */ >> +#define ARM_MS_FULL 0x80000000 >> +#define ARM_MS_EMPTY 0x40000000 >> > nit: BIT(31) and BIT(30) > >> + >> +/* Configuration register: Enable interrupts. */ >> +#define ARM_MC_IHAVEDATAIRQEN 0x00000001 >> > nit: BIT(0) Works for me. >> + >> +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. 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. >> + 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() Done. >> +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. >> + if (!mbox->started) >> + return -ENODEV; >> > This 'started' flag is unnecessary. API won't call send_data() before > startup() or after shutdown(). Dropped. >> + 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.
On Wed, Apr 29, 2015 at 1:19 AM, Eric Anholt <eric@anholt.net> wrote: > Jassi Brar <jassisinghbrar@gmail.com> writes: > >>> + >>> +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. > > 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" >>> + 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.
Jassi Brar <jassisinghbrar@gmail.com> writes: > On Wed, Apr 29, 2015 at 1:19 AM, Eric Anholt <eric@anholt.net> wrote: >> Jassi Brar <jassisinghbrar@gmail.com> writes: >> >>>> + >>>> +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. >> >> 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. > 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. >>>> + 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.
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index 84b0a2d..ab574da 100644 --- a/drivers/mailbox/Kconfig +++ b/drivers/mailbox/Kconfig @@ -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 diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index b18201e..8e6d822 100644 --- a/drivers/mailbox/Makefile +++ b/drivers/mailbox/Makefile @@ -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 diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c new file mode 100644 index 0000000..6910c71 --- /dev/null +++ b/drivers/mailbox/bcm2835-mailbox.c @@ -0,0 +1,220 @@ +/* + * Copyright (C) 2010 Broadcom + * Copyright (C) 2013-2014 Lubomir Rintel + * Copyright (C) 2013 Craig McGeachie + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This device provides a mechanism for writing to the mailboxes, + * that are shared between the ARM and the VideoCore processor + * + * Parts of the driver are based on: + * - arch/arm/mach-bcm2708/vcio.c file written by Gray Girling that was + * obtained from branch "rpi-3.6.y" of git://github.com/raspberrypi/ + * linux.git + * - drivers/mailbox/bcm2835-ipc.c by Lubomir Rintel at + * https://github.com/hackerspace/rpi-linux/blob/lr-raspberry-pi/drivers/ + * mailbox/bcm2835-ipc.c + * - documentation available on the following web site: + * https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface + */ + +#include <linux/device.h> +#include <linux/dma-mapping.h> +#include <linux/err.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/kernel.h> +#include <linux/mailbox_controller.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> + +/* Mailboxes */ +#define ARM_0_MAIL0 0x00 +#define ARM_0_MAIL1 0x20 + +/* + * Mailbox registers. We basically only support mailbox 0 & 1. We + * deliver to the VC in mailbox 1, it delivers to us in mailbox 0. See + * BCM2835-ARM-Peripherals.pdf section 1.3 for an explanation about + * the placement of memory barriers. + */ +#define MAIL0_RD (ARM_0_MAIL0 + 0x00) +#define MAIL0_POL (ARM_0_MAIL0 + 0x10) +#define MAIL0_STA (ARM_0_MAIL0 + 0x18) +#define MAIL0_CNF (ARM_0_MAIL0 + 0x1C) +#define MAIL1_WRT (ARM_0_MAIL1 + 0x00) + +/* Status register: FIFO state. */ +#define ARM_MS_FULL 0x80000000 +#define ARM_MS_EMPTY 0x40000000 + +/* Configuration register: Enable interrupts. */ +#define ARM_MC_IHAVEDATAIRQEN 0x00000001 + +struct bcm2835_mbox { + struct device *dev; + void __iomem *regs; + spinlock_t lock; + bool started; + struct mbox_controller controller; +}; + +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]; + + 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); + continue; + } + 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; + u32 msg = *(u32 *)data; + + if (!mbox->started) + return -ENODEV; + spin_lock(&mbox->lock); + if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) { + ret = -EBUSY; + goto end; + } + writel(msg, mbox->regs + MAIL1_WRT); + dev_dbg(mbox->dev, "Request 0x%08X\n", msg); +end: + spin_unlock(&mbox->lock); + return ret; +} + +static int bcm2835_startup(struct mbox_chan *link) +{ + mbox->started = true; + return 0; +} + +static void bcm2835_shutdown(struct mbox_chan *link) +{ + mbox->started = false; +} + +static bool bcm2835_last_tx_done(struct mbox_chan *link) +{ + bool ret; + + if (!mbox->started) + return false; + spin_lock(&mbox->lock); + ret = !(readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL); + spin_unlock(&mbox->lock); + return ret; +} + +static struct mbox_chan_ops bcm2835_mbox_chan_ops = { + .send_data = bcm2835_send_data, + .startup = bcm2835_startup, + .shutdown = bcm2835_shutdown, + .last_tx_done = bcm2835_last_tx_done +}; + +static int bcm2835_mbox_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + int ret = 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); + if (ret) { + dev_err(dev, "Failed to register a mailbox IRQ handler: %d\n", + ret); + mbox = NULL; + return -ENODEV; + } + + iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + mbox->regs = devm_ioremap_resource(&pdev->dev, iomem); + if (IS_ERR(mbox->regs)) { + ret = PTR_ERR(mbox->regs); + dev_err(&pdev->dev, "Failed to remap mailbox regs: %d\n", ret); + mbox = NULL; + return ret; + } + + mbox->controller.txdone_poll = true; + mbox->controller.txpoll_period = 5; + mbox->controller.ops = &bcm2835_mbox_chan_ops; + mbox->controller.dev = dev; + mbox->controller.num_chans = 1; + mbox->controller.chans = devm_kzalloc(dev, + sizeof(*mbox->controller.chans), GFP_KERNEL); + if (!mbox->controller.chans) { + mbox = NULL; + return -ENOMEM; + } + + ret = mbox_controller_register(&mbox->controller); + if (ret) { + mbox = NULL; + 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) +{ + writel(0, mbox->regs + MAIL0_CNF); + mbox_controller_unregister(&mbox->controller); + mbox = NULL; + return 0; +} + +static const struct of_device_id bcm2835_mbox_of_match[] = { + { .compatible = "brcm,bcm2835-mbox", }, + {}, +}; +MODULE_DEVICE_TABLE(of, bcm2835_mbox_of_match); + +static struct platform_driver bcm2835_mbox_driver = { + .driver = { + .name = "bcm2835-mbox", + .owner = THIS_MODULE, + .of_match_table = bcm2835_mbox_of_match, + }, + .probe = bcm2835_mbox_probe, + .remove = bcm2835_mbox_remove, +}; +module_platform_driver(bcm2835_mbox_driver); + +MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>"); +MODULE_DESCRIPTION("BCM2835 mailbox IPC driver"); +MODULE_LICENSE("GPL v2");