| Message ID | 1430327374-6562-1-git-send-email-eric@anholt.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 29, 2015 at 10:39 PM, Eric Anholt <eric@anholt.net> wrote: > + > +struct bcm2835_mbox { > + struct device *dev; > + void __iomem *regs; > + spinlock_t lock; > + struct mbox_controller controller; > +}; > + > +static struct bcm2835_mbox *mbox; > + > +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) > +{ > + struct device *dev = mbox->dev; > + struct mbox_chan *link = &mbox->controller.chans[0]; > + I learn from Stephen's other post that the controller could have multiple channels. In which case this driver is poorly setup. Actually if the driver was designed properly there isn't anything special to be done. Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0 > + while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) { > + u32 msg = readl(mbox->regs + MAIL0_RD); > + dev_dbg(dev, "Reply 0x%08X\n", msg); > + mbox_chan_received_data(link, &msg); > + } > + return IRQ_HANDLED; > +} > + > +static int bcm2835_send_data(struct mbox_chan *link, void *data) > +{ > + int ret = 0; > + u32 msg = *(u32 *)data; > + > + spin_lock(&mbox->lock); > + writel(msg, mbox->regs + MAIL1_WRT); > + dev_dbg(mbox->dev, "Request 0x%08X\n", msg); > +end: > Did you compile check the driver for errors and warnings? > +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); > Why even pass 'mbox' when you insist on ignoring 'dev_id' and access the global variable again directly :D
On 04/29/2015 11:26 PM, Jassi Brar wrote: > On Wed, Apr 29, 2015 at 10:39 PM, Eric Anholt <eric@anholt.net> wrote: > >> + >> +struct bcm2835_mbox { >> + struct device *dev; >> + void __iomem *regs; >> + spinlock_t lock; >> + struct mbox_controller controller; >> +}; >> + >> +static struct bcm2835_mbox *mbox; >> + >> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) >> +{ >> + struct device *dev = mbox->dev; >> + struct mbox_chan *link = &mbox->controller.chans[0]; >> + > I learn from Stephen's other post that the controller could have > multiple channels. In which case this driver is poorly setup. Actually > if the driver was designed properly there isn't anything special to be > done. > Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0 Now that I look a bit harder at the registers, I think there are 2 mailbox register sets, but each is uni-directional, so a pair makes up the typical bi-directional mailbox. There are multiple "owner" areas (or sets of registers); I'm not quite sure what that implies. As such, limiting this driver to a single mailbox is probably correct. I would expect to see #mbox-cells=<0> in the DT, which would require a custom of_xlate though (or modifying the default to work with a 0 cell count; IIRC the default of_xlate for some other subsystems will work in that scenario). (As an aside, if we ever did find the need to expand the driver to cover more mailboxes in the future, extending it and the DT to support #mbox-cells=<0> or #mbox-cells=<1> at run-time should be trivial).
Jassi Brar <jassisinghbrar@gmail.com> writes: > On Wed, Apr 29, 2015 at 10:39 PM, Eric Anholt <eric@anholt.net> wrote: > >> + >> +struct bcm2835_mbox { >> + struct device *dev; >> + void __iomem *regs; >> + spinlock_t lock; >> + struct mbox_controller controller; >> +}; >> + >> +static struct bcm2835_mbox *mbox; >> + >> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) >> +{ >> + struct device *dev = mbox->dev; >> + struct mbox_chan *link = &mbox->controller.chans[0]; >> + > I learn from Stephen's other post that the controller could have > multiple channels. In which case this driver is poorly setup. Actually > if the driver was designed properly there isn't anything special to be > done. > Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0 There's only the one channel according to the docs. I wish we wouldn't get derailed by speculation on the list when the documentation is available. :( >> + while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) { >> + u32 msg = readl(mbox->regs + MAIL0_RD); >> + dev_dbg(dev, "Reply 0x%08X\n", msg); >> + mbox_chan_received_data(link, &msg); >> + } >> + return IRQ_HANDLED; >> +} >> + >> +static int bcm2835_send_data(struct mbox_chan *link, void *data) >> +{ >> + int ret = 0; >> + u32 msg = *(u32 *)data; >> + >> + spin_lock(&mbox->lock); >> + writel(msg, mbox->regs + MAIL1_WRT); >> + dev_dbg(mbox->dev, "Request 0x%08X\n", msg); >> +end: >> > Did you compile check the driver for errors and warnings? At this point I'm so burned out on repainting this bikeshed that I missed a spot of the current color.
On Fri, May 1, 2015 at 8:31 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/29/2015 11:26 PM, Jassi Brar wrote: >> On Wed, Apr 29, 2015 at 10:39 PM, Eric Anholt <eric@anholt.net> wrote: >> >>> + >>> +struct bcm2835_mbox { >>> + struct device *dev; >>> + void __iomem *regs; >>> + spinlock_t lock; >>> + struct mbox_controller controller; >>> +}; >>> + >>> +static struct bcm2835_mbox *mbox; >>> + >>> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) >>> +{ >>> + struct device *dev = mbox->dev; >>> + struct mbox_chan *link = &mbox->controller.chans[0]; >>> + >> I learn from Stephen's other post that the controller could have >> multiple channels. In which case this driver is poorly setup. Actually >> if the driver was designed properly there isn't anything special to be >> done. >> Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0 > > Now that I look a bit harder at the registers, I think there are 2 > mailbox register sets, but each is uni-directional, so a pair makes up > the typical bi-directional mailbox. There are multiple "owner" areas (or > sets of registers); I'm not quite sure what that implies. > Probably a mailbox is the send-recv pair, and the controller could have many such pairs. > As such, limiting this driver to a single mailbox is probably correct. > OK > I would expect to see #mbox-cells=<0> in the DT, which would require a > custom of_xlate though (or modifying the default to work with a 0 cell > count; IIRC the default of_xlate for some other subsystems will work in > that scenario). > Yeah the api could simply return the first free channel for 0-cell spec.
On Tue, May 5, 2015 at 6:27 AM, Eric Anholt <eric@anholt.net> wrote: > Jassi Brar <jassisinghbrar@gmail.com> writes: > >> On Wed, Apr 29, 2015 at 10:39 PM, Eric Anholt <eric@anholt.net> wrote: >> >>> + >>> +struct bcm2835_mbox { >>> + struct device *dev; >>> + void __iomem *regs; >>> + spinlock_t lock; >>> + struct mbox_controller controller; >>> +}; >>> + >>> +static struct bcm2835_mbox *mbox; >>> + >>> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) >>> +{ >>> + struct device *dev = mbox->dev; >>> + struct mbox_chan *link = &mbox->controller.chans[0]; >>> + >> I learn from Stephen's other post that the controller could have >> multiple channels. In which case this driver is poorly setup. Actually >> if the driver was designed properly there isn't anything special to be >> done. >> Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0 > > There's only the one channel according to the docs. I wish we wouldn't > get derailed by speculation on the list when the documentation is > available. :( > Can I have the pointer to the doc please, if its publicly available. >>> + while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) { >>> + u32 msg = readl(mbox->regs + MAIL0_RD); >>> + dev_dbg(dev, "Reply 0x%08X\n", msg); >>> + mbox_chan_received_data(link, &msg); >>> + } >>> + return IRQ_HANDLED; >>> +} >>> + >>> +static int bcm2835_send_data(struct mbox_chan *link, void *data) >>> +{ >>> + int ret = 0; >>> + u32 msg = *(u32 *)data; >>> + >>> + spin_lock(&mbox->lock); >>> + writel(msg, mbox->regs + MAIL1_WRT); >>> + dev_dbg(mbox->dev, "Request 0x%08X\n", msg); >>> +end: >>> >> Did you compile check the driver for errors and warnings? > > At this point I'm so burned out on repainting this bikeshed that I > missed a spot of the current color. > I have just rechecked the thread and I didn't find any revision that had only nits picked. Subsystems even dictate how the files are named and ordered in Makefile, and you have been insisting on hardcoding values.
On 05/04/2015 08:30 PM, Jassi Brar wrote: > On Tue, May 5, 2015 at 6:27 AM, Eric Anholt <eric@anholt.net> wrote: >> Jassi Brar <jassisinghbrar@gmail.com> writes: >> >>> On Wed, Apr 29, 2015 at 10:39 PM, Eric Anholt <eric@anholt.net> wrote: >>> >>>> + >>>> +struct bcm2835_mbox { >>>> + struct device *dev; >>>> + void __iomem *regs; >>>> + spinlock_t lock; >>>> + struct mbox_controller controller; >>>> +}; >>>> + >>>> +static struct bcm2835_mbox *mbox; >>>> + >>>> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) >>>> +{ >>>> + struct device *dev = mbox->dev; >>>> + struct mbox_chan *link = &mbox->controller.chans[0]; >>>> + >>> I learn from Stephen's other post that the controller could have >>> multiple channels. In which case this driver is poorly setup. Actually >>> if the driver was designed properly there isn't anything special to be >>> done. >>> Here you choose to waste 'dev_id' and hard-code dereferencing to channel-0 >> >> There's only the one channel according to the docs. I wish we wouldn't >> get derailed by speculation on the list when the documentation is >> available. :( >> > Can I have the pointer to the doc please, if its publicly available. https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf
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..f8b9c51 --- /dev/null +++ b/drivers/mailbox/bcm2835-mailbox.c @@ -0,0 +1,204 @@ +/* + * Copyright (C) 2010,2015 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 BIT(31) +#define ARM_MS_EMPTY BIT(30) + +/* Configuration register: Enable interrupts. */ +#define ARM_MC_IHAVEDATAIRQEN BIT(0) + +struct bcm2835_mbox { + struct device *dev; + void __iomem *regs; + spinlock_t lock; + struct mbox_controller controller; +}; + +static struct bcm2835_mbox *mbox; + +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) +{ + 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); + 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; + + spin_lock(&mbox->lock); + 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) +{ + /* Enable the interrupt on data reception */ + writel(ARM_MC_IHAVEDATAIRQEN, mbox->regs + MAIL0_CNF); + + return 0; +} + +static void bcm2835_shutdown(struct mbox_chan *link) +{ + writel(0, mbox->regs + MAIL0_CNF); +} + +static bool bcm2835_last_tx_done(struct mbox_chan *link) +{ + bool ret; + + 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; + } + + dev_info(dev, "mailbox enabled\n"); + + return ret; +} + +static int bcm2835_mbox_remove(struct platform_device *pdev) +{ + 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");