| Message ID | d9e1adbe6b12aab495f0292c4553c5c8f61c70b5.1359684780.git.rtivy@ti.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Rob, On Fri, Feb 1, 2013 at 4:24 AM, Robert Tivy <rtivy@ti.com> wrote: > drivers/remoteproc/Kconfig | 29 ++- > drivers/remoteproc/Makefile | 1 + > drivers/remoteproc/da8xx_remoteproc.c | 346 +++++++++++++++++++++++++++++++++ > drivers/remoteproc/remoteproc_core.c | 22 ++- It looks like this patch squashes: 1. A fix to drivers/remoteproc/Kconfig 2. New functionality in remoteproc_core.c 3. A new da8xx driver These are independent changes, so we better split them up to separate patches. > config OMAP_REMOTEPROC > tristate "OMAP remoteproc support" > - depends on EXPERIMENTAL ... > config STE_MODEM_RPROC > tristate "STE-Modem remoteproc support" > - depends on EXPERIMENTAL These look unrelated to this patch, and it seems that Kees Cook submitted these awhile ago so it should probably already be in linux-next (haven't looked). I think we can drop these. > +/** > + * handle_event() - inbound virtqueue message workqueue function > + * > + * This function is registered as a kernel thread and is scheduled by the > + * kernel handler. > + */ > +static irqreturn_t handle_event(int irq, void *p) > +{ > + struct rproc *rproc = (struct rproc *)p; > + > + /* Process incoming buffers on our vring */ > + while (IRQ_HANDLED == rproc_vq_interrupt(rproc, 0)) > + ; > + > + /* Must allow wakeup of potenitally blocking senders */ > + rproc_vq_interrupt(rproc, 1); IIRC we agreed on removing this part, and instead adding it to the generic remoteproc framework (i.e. Cyril's patch). > +static int da8xx_rproc_start(struct rproc *rproc) > +{ ... > + dsp_clk = devm_clk_get(dev, NULL); > + if (IS_ERR(dsp_clk)) { > + dev_err(dev, "clk_get error: %ld\n", PTR_ERR(dsp_clk)); > + > + return PTR_RET(dsp_clk); > + } > + drproc->dsp_clk = dsp_clk; Have you considered doing this in ->probe() instead? Do we need to call get/put every time we start/stop the remote processor? > +/* kick a virtqueue */ > +static void da8xx_rproc_kick(struct rproc *rproc, int vqid) > +{ > + struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv; > + int timed_out; > + unsigned long timeout; > + > + timed_out = 0; > + timeout = jiffies + HZ/100; > + > + /* Poll for ack from other side first */ > + while (readl(drproc->chipsig) & SYSCFG_CHIPSIG2) > + if (time_after(jiffies, timeout)) { > + if (readl(drproc->chipsig) & SYSCFG_CHIPSIG2) { > + dev_err(rproc->dev.parent, > + "failed to receive ack\n"); > + timed_out = 1; > + } > + > + break; > + } This still looks a bit out of place here. Kicking should be a fast unilateral action, that doesn't depend on the other side. > +static int da8xx_rproc_probe(struct platform_device *pdev) > +{ ... > + ret = rproc_add(rproc); > + if (ret) { > + dev_err(dev, "rproc_add failed: %d\n", ret); > + goto free_rproc; > + } > + > + drproc->chipsig = chipsig; > + drproc->bootreg = bootreg; > + drproc->ack_fxn = irq_data->chip->irq_ack; > + drproc->irq_data = irq_data; > + drproc->irq = irq; > + > + /* everything the ISR needs is now setup, so hook it up */ > + ret = devm_request_threaded_irq(dev, irq, da8xx_rproc_callback, > + handle_event, 0, "da8xx-remoteproc", rproc); > + if (ret) { > + dev_err(dev, "devm_request_threaded_irq error: %d\n", ret); > + rproc_del(rproc); > + goto free_rproc; > + } Shouldn't we be requesting this before we rproc_add() ? > +static int da8xx_rproc_remove(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct rproc *rproc = platform_get_drvdata(pdev); > + struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv; > + int ret; ... > + ret = rproc_del(rproc); You can safely remove 'ret' altogether. We will just crash in rproc_put if rproc is NULL. > + rproc_put(rproc); > + > + return ret; > +} > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index dd3bfaf..e0f703e 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1222,19 +1222,39 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > const char *firmware, int len) > { > struct rproc *rproc; > + char *template = "rproc-%s-fw"; > + char *p; > > if (!dev || !name || !ops) > return NULL; > > + if (!firmware) > + /* > + * Make room for default firmware name (minus %s plus '\0'). > + * If the caller didn't pass in a firmware name then > + * construct a default name. We're already glomming 'len' > + * bytes onto the end of the struct rproc allocation, so do > + * a few more for the default firmware name (but only if > + * the caller doesn't pass one). > + */ > + len += strlen(name) + strlen(template) - 2 + 1; > + > rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL); > if (!rproc) { > dev_err(dev, "%s: kzalloc failed\n", __func__); > return NULL; > } > > + if (!firmware) { > + p = (char *)rproc + sizeof(struct rproc) + len; > + sprintf(p, template, name); Looks like p you're writing to is outside of the rproc memory allocation. Thanks, Ohad.
Hi Ohad, > -----Original Message----- > From: Ohad Ben-Cohen [mailto:ohad@wizery.com] > Sent: Friday, February 15, 2013 12:07 AM > > Hi Rob, > > On Fri, Feb 1, 2013 at 4:24 AM, Robert Tivy <rtivy@ti.com> wrote: > > drivers/remoteproc/Kconfig | 29 ++- > > drivers/remoteproc/Makefile | 1 + > > drivers/remoteproc/da8xx_remoteproc.c | 346 > +++++++++++++++++++++++++++++++++ > > drivers/remoteproc/remoteproc_core.c | 22 ++- > > It looks like this patch squashes: > 1. A fix to drivers/remoteproc/Kconfig > 2. New functionality in remoteproc_core.c > 3. A new da8xx driver > > These are independent changes, so we better split them up to separate > patches. Ok, I will submit separate patches for these. In doing so, I will have a stand-alone patch for the drivers/remoteproc/Kconfig fix, as well as the driver-related addition to that file in the driver patch. PCMIIW. > > > config OMAP_REMOTEPROC > > tristate "OMAP remoteproc support" > > - depends on EXPERIMENTAL > ... > > config STE_MODEM_RPROC > > tristate "STE-Modem remoteproc support" > > - depends on EXPERIMENTAL > > These look unrelated to this patch, and it seems that Kees Cook > submitted these awhile ago so it should probably already be in > linux-next (haven't looked). I think we can drop these. OK, will drop. > > > +/** > > + * handle_event() - inbound virtqueue message workqueue function > > + * > > + * This function is registered as a kernel thread and is scheduled > by the > > + * kernel handler. > > + */ > > +static irqreturn_t handle_event(int irq, void *p) > > +{ > > + struct rproc *rproc = (struct rproc *)p; > > + > > + /* Process incoming buffers on our vring */ > > + while (IRQ_HANDLED == rproc_vq_interrupt(rproc, 0)) > > + ; > > + > > + /* Must allow wakeup of potenitally blocking senders */ > > + rproc_vq_interrupt(rproc, 1); > > IIRC we agreed on removing this part, and instead adding it to the > generic remoteproc framework (i.e. Cyril's patch). I didn't like the idea of having extra overhead when calling the "all virtqueues" version. We know that we want to call rproc_vq_interrupt(rproc, 1) just once, and don't care about its return value. If we did /* Process incoming buffers on our vring */ while (IRQ_HANDLED == rproc_vq_interrupt(rproc, -1)) ; then that would essentially turn into: do { ret = IRQ_NONE; if (rproc_vq_interrupt(rproc, 0) == IRQ_HANDLED) ret = IRQ_HANDLED; if (rproc_vq_interrupt(rproc, 1) == IRQ_HANDLED) ret = IRQ_HANDLED; } while (ret == IRQ_HANDLED); which will end up calling rproc_vq_interrupt(rproc, 1) too many times. We have a benchmark goal to keep wrt/ the whole round-trip time of messages and we're currently not even achieving that, so more overhead than today can't be tolerated. > > > +static int da8xx_rproc_start(struct rproc *rproc) > > +{ > ... > > + dsp_clk = devm_clk_get(dev, NULL); > > + if (IS_ERR(dsp_clk)) { > > + dev_err(dev, "clk_get error: %ld\n", > PTR_ERR(dsp_clk)); > > + > > + return PTR_RET(dsp_clk); > > + } > > + drproc->dsp_clk = dsp_clk; > > Have you considered doing this in ->probe() instead? Do we need to > call get/put every time we start/stop the remote processor? I suppose we don't need to call it every time, I will try moving it to ->probe() > > > +/* kick a virtqueue */ > > +static void da8xx_rproc_kick(struct rproc *rproc, int vqid) > > +{ > > + struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc- > >priv; > > + int timed_out; > > + unsigned long timeout; > > + > > + timed_out = 0; > > + timeout = jiffies + HZ/100; > > + > > + /* Poll for ack from other side first */ > > + while (readl(drproc->chipsig) & SYSCFG_CHIPSIG2) > > + if (time_after(jiffies, timeout)) { > > + if (readl(drproc->chipsig) & SYSCFG_CHIPSIG2) > { > > + dev_err(rproc->dev.parent, > > + "failed to receive ack\n"); > > + timed_out = 1; > > + } > > + > > + break; > > + } > > This still looks a bit out of place here. > > Kicking should be a fast unilateral action, that doesn't depend on the > other side. Ok, I'll drop this complexity. While that code sorta looks like too much, in a smoothly working system it's just a register read and single "false" if-test, since SYSCFG_CHIPSIG2 will actually never be set there. If SYSCFG_CHIPSIG2 *is* set then it will likely not unset and the timed_out situation will happen. > > > +static int da8xx_rproc_probe(struct platform_device *pdev) > > +{ > ... > > + ret = rproc_add(rproc); > > + if (ret) { > > + dev_err(dev, "rproc_add failed: %d\n", ret); > > + goto free_rproc; > > + } > > + > > + drproc->chipsig = chipsig; > > + drproc->bootreg = bootreg; > > + drproc->ack_fxn = irq_data->chip->irq_ack; > > + drproc->irq_data = irq_data; > > + drproc->irq = irq; > > + > > + /* everything the ISR needs is now setup, so hook it up */ > > + ret = devm_request_threaded_irq(dev, irq, > da8xx_rproc_callback, > > + handle_event, 0, "da8xx-remoteproc", rproc); > > + if (ret) { > > + dev_err(dev, "devm_request_threaded_irq error: %d\n", > ret); > > + rproc_del(rproc); > > + goto free_rproc; > > + } > > Shouldn't we be requesting this before we rproc_add() ? Yeah, that seems to be the more prudent ordering, will switch. > > > +static int da8xx_rproc_remove(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct rproc *rproc = platform_get_drvdata(pdev); > > + struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc- > >priv; > > + int ret; > ... > > + ret = rproc_del(rproc); > > You can safely remove 'ret' altogether. We will just crash in > rproc_put if rproc is NULL. Will do. > > > + rproc_put(rproc); > > + > > + return ret; > > +} > > > diff --git a/drivers/remoteproc/remoteproc_core.c > b/drivers/remoteproc/remoteproc_core.c > > index dd3bfaf..e0f703e 100644 > > --- a/drivers/remoteproc/remoteproc_core.c > > +++ b/drivers/remoteproc/remoteproc_core.c > > @@ -1222,19 +1222,39 @@ struct rproc *rproc_alloc(struct device *dev, > const char *name, > > const char *firmware, int len) > > { > > struct rproc *rproc; > > + char *template = "rproc-%s-fw"; > > + char *p; > > > > if (!dev || !name || !ops) > > return NULL; > > > > + if (!firmware) > > + /* > > + * Make room for default firmware name (minus %s plus > '\0'). > > + * If the caller didn't pass in a firmware name then > > + * construct a default name. We're already glomming > 'len' > > + * bytes onto the end of the struct rproc allocation, > so do > > + * a few more for the default firmware name (but only > if > > + * the caller doesn't pass one). > > + */ > > + len += strlen(name) + strlen(template) - 2 + 1; > > + > > rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL); > > if (!rproc) { > > dev_err(dev, "%s: kzalloc failed\n", __func__); > > return NULL; > > } > > > > + if (!firmware) { > > + p = (char *)rproc + sizeof(struct rproc) + len; > > + sprintf(p, template, name); > > Looks like p you're writing to is outside of the rproc memory > allocation. Yikes, must have been getting lucky when testing with the default name! Thanks for the notice, will fix. Regards, - Rob > > Thanks, > Ohad.
Hi Rob, On Tue, Feb 19, 2013 at 1:02 AM, Tivy, Robert <rtivy@ti.com> wrote: >> It looks like this patch squashes: >> 1. A fix to drivers/remoteproc/Kconfig >> 2. New functionality in remoteproc_core.c >> 3. A new da8xx driver >> >> These are independent changes, so we better split them up to separate >> patches. > > Ok, I will submit separate patches for these. In doing so, I will have a stand-alone patch for the drivers/remoteproc/Kconfig fix, as well as the driver-related addition to that file in the driver patch. PCMIIW. We should have one patch for the Kconfig fix, another one for the new firmware-name functionality in rproc_alloc() and then a patch adding this new driver. >> > +static irqreturn_t handle_event(int irq, void *p) >> > +{ >> > + struct rproc *rproc = (struct rproc *)p; >> > + >> > + /* Process incoming buffers on our vring */ >> > + while (IRQ_HANDLED == rproc_vq_interrupt(rproc, 0)) >> > + ; >> > + >> > + /* Must allow wakeup of potenitally blocking senders */ >> > + rproc_vq_interrupt(rproc, 1); >> >> IIRC we agreed on removing this part, and instead adding it to the >> generic remoteproc framework (i.e. Cyril's patch). > > I didn't like the idea of having extra overhead when calling the "all virtqueues" version. > > We know that we want to call rproc_vq_interrupt(rproc, 1) just once, and don't care about its return value. If we did > /* Process incoming buffers on our vring */ > while (IRQ_HANDLED == rproc_vq_interrupt(rproc, -1)) > ; > then that would essentially turn into: > do { > ret = IRQ_NONE; > if (rproc_vq_interrupt(rproc, 0) == IRQ_HANDLED) > ret = IRQ_HANDLED; > if (rproc_vq_interrupt(rproc, 1) == IRQ_HANDLED) > ret = IRQ_HANDLED; > } while (ret == IRQ_HANDLED); > which will end up calling rproc_vq_interrupt(rproc, 1) too many times. > > We have a benchmark goal to keep wrt/ the whole round-trip time of messages and we're currently not even achieving that, so more overhead than today can't be tolerated. Have you checked whether it really affects performance? I'd expect this to be very negligible. We can probably also maintain a bitmap with bits set for every IRQ_HANDLED vring, and stop processing vrings that has this bit cleared. But it actually looks like there's a different problem here. rproc_vq_interrupt() may also return IRQ_HANDLED on the unlikely event that vq->broken is set. With the current approach taken by this patch, this may lock the system in a busy loop. I took a brief look at the other virtio drivers, and it seems they process all pending messages available when kicked, so we should probably add this to rpmsg_recv_done() too. This way we don't have to manually do it in the remoteproc layer, and all platforms will benefit from it. We should then only have to allow the option of asking the core to process all available vrings, as we discussed, and we're done. Thanks, Ohad.
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig index 96ce101..8b82913 100644 --- a/drivers/remoteproc/Kconfig +++ b/drivers/remoteproc/Kconfig @@ -3,14 +3,12 @@ menu "Remoteproc drivers (EXPERIMENTAL)" # REMOTEPROC gets selected by whoever wants it config REMOTEPROC tristate - depends on EXPERIMENTAL depends on HAS_DMA - select FW_CONFIG + select FW_LOADER select VIRTIO config OMAP_REMOTEPROC tristate "OMAP remoteproc support" - depends on EXPERIMENTAL depends on HAS_DMA depends on ARCH_OMAP4 depends on OMAP_IOMMU @@ -32,7 +30,6 @@ config OMAP_REMOTEPROC config STE_MODEM_RPROC tristate "STE-Modem remoteproc support" - depends on EXPERIMENTAL depends on HAS_DMA select REMOTEPROC default n @@ -41,4 +38,28 @@ config STE_MODEM_RPROC This can be either built-in or a loadable module. If unsure say N. +config DA8XX_REMOTEPROC + tristate "DA830/OMAPL137 & DA850/OMAPL138 remoteproc support (EXPERIMENTAL)" + depends on ARCH_DAVINCI_DA8XX + select REMOTEPROC + select RPMSG + select CMA + default n + help + Say y here to support DA830/OMAPL137 & DA850/OMAPL138 remote + processors via the remote processor framework. + + You want to say y here in order to enable AMP + use-cases to run on your platform (multimedia codecs are + offloaded to remote DSP processors using this framework). + + This module controls the name of the firmware file that gets + loaded on the DSP. This file must reside in the /lib/firmware + directory. It can be specified via the module parameter + da8xx_fw_name=<filename>, and if not specified will default to + "rproc-dsp-fw". + + It's safe to say n here if you're not interested in multimedia + offloading or just want a bare minimum kernel. + endmenu diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile index 391b651..ac2ff75 100644 --- a/drivers/remoteproc/Makefile +++ b/drivers/remoteproc/Makefile @@ -9,3 +9,4 @@ remoteproc-y += remoteproc_virtio.o remoteproc-y += remoteproc_elf_loader.o obj-$(CONFIG_OMAP_REMOTEPROC) += omap_remoteproc.o obj-$(CONFIG_STE_MODEM_RPROC) += ste_modem_rproc.o +obj-$(CONFIG_DA8XX_REMOTEPROC) += da8xx_remoteproc.o diff --git a/drivers/remoteproc/da8xx_remoteproc.c b/drivers/remoteproc/da8xx_remoteproc.c new file mode 100644 index 0000000..7c3b41a --- /dev/null +++ b/drivers/remoteproc/da8xx_remoteproc.c @@ -0,0 +1,346 @@ +/* + * Remote processor machine-specific module for DA8XX + * + * Copyright (C) 2013 Texas Instruments, Inc. + * + * 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. + */ + +#include <linux/bitops.h> +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/irq.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/remoteproc.h> + +#include <mach/clock.h> /* for davinci_clk_reset_assert/deassert() */ + +#include "remoteproc_internal.h" + +static char *da8xx_fw_name; +module_param(da8xx_fw_name, charp, S_IRUGO); +MODULE_PARM_DESC(da8xx_fw_name, + "\n\t\tName of DSP firmware file in /lib/firmware"); + +/* + * OMAP-L138 Technical References: + * http://www.ti.com/product/omap-l138 + */ +#define SYSCFG_CHIPSIG0 BIT(0) +#define SYSCFG_CHIPSIG1 BIT(1) +#define SYSCFG_CHIPSIG2 BIT(2) +#define SYSCFG_CHIPSIG3 BIT(3) +#define SYSCFG_CHIPSIG4 BIT(4) + +/** + * struct da8xx_rproc - da8xx remote processor instance state + * @rproc: rproc handle + * @dsp_clk: placeholder for platform's DSP clk + * @ack_fxn: chip-specific ack function for ack'ing irq + * @irq_data: ack_fxn function parameter + * @chipsig: virt ptr to DSP interrupt registers (CHIPSIG & CHIPSIG_CLR) + * @bootreg: virt ptr to DSP boot address register (HOST1CFG) + * @irq: irq # used by this instance + */ +struct da8xx_rproc { + struct rproc *rproc; + struct clk *dsp_clk; + void (*ack_fxn)(struct irq_data *data); + struct irq_data *irq_data; + void __iomem *chipsig; + void __iomem *bootreg; + int irq; +}; + +/** + * handle_event() - inbound virtqueue message workqueue function + * + * This function is registered as a kernel thread and is scheduled by the + * kernel handler. + */ +static irqreturn_t handle_event(int irq, void *p) +{ + struct rproc *rproc = (struct rproc *)p; + + /* Process incoming buffers on our vring */ + while (IRQ_HANDLED == rproc_vq_interrupt(rproc, 0)) + ; + + /* Must allow wakeup of potenitally blocking senders */ + rproc_vq_interrupt(rproc, 1); + + return IRQ_HANDLED; +} + +/** + * da8xx_rproc_callback() - inbound virtqueue message handler + * + * This handler is invoked directly by the kernel whenever the remote + * core (DSP) has modified the state of a virtqueue. There is no + * "payload" message indicating the virtqueue index as is the case with + * mailbox-based implementations on OMAP4. As such, this handler "polls" + * each known virtqueue index for every invocation. + */ +static irqreturn_t da8xx_rproc_callback(int irq, void *p) +{ + struct rproc *rproc = (struct rproc *)p; + struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv; + u32 chipsig; + + chipsig = readl(drproc->chipsig); + if (chipsig & SYSCFG_CHIPSIG0) { + /* Clear interrupt level source */ + writel(SYSCFG_CHIPSIG0, drproc->chipsig + 4); + + /* + * ACK intr to AINTC. + * + * It has already been ack'ed by the kernel before calling + * this function, but since the ARM<->DSP interrupts in the + * CHIPSIG register are "level" instead of "pulse" variety, + * we need to ack it after taking down the level else we'll + * be called again immediately after returning. + */ + drproc->ack_fxn(drproc->irq_data); + + return IRQ_WAKE_THREAD; + } + + return IRQ_HANDLED; +} + +static int da8xx_rproc_start(struct rproc *rproc) +{ + struct device *dev = rproc->dev.parent; + struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv; + struct clk *dsp_clk; + + /* hw requires the start (boot) address be on 1KB boundary */ + if (rproc->bootaddr & 0x3ff) { + dev_err(dev, "invalid boot address: must be aligned to 1KB\n"); + + return -EINVAL; + } + writel(rproc->bootaddr, drproc->bootreg); + + dsp_clk = devm_clk_get(dev, NULL); + if (IS_ERR(dsp_clk)) { + dev_err(dev, "clk_get error: %ld\n", PTR_ERR(dsp_clk)); + + return PTR_RET(dsp_clk); + } + drproc->dsp_clk = dsp_clk; + + clk_enable(dsp_clk); + davinci_clk_reset_deassert(dsp_clk); + + return 0; +} + +static int da8xx_rproc_stop(struct rproc *rproc) +{ + struct da8xx_rproc *drproc = rproc->priv; + struct clk *dsp_clk = drproc->dsp_clk; + + clk_disable(dsp_clk); + devm_clk_put(rproc->dev.parent, dsp_clk); + + return 0; +} + +/* kick a virtqueue */ +static void da8xx_rproc_kick(struct rproc *rproc, int vqid) +{ + struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv; + int timed_out; + unsigned long timeout; + + timed_out = 0; + timeout = jiffies + HZ/100; + + /* Poll for ack from other side first */ + while (readl(drproc->chipsig) & SYSCFG_CHIPSIG2) + if (time_after(jiffies, timeout)) { + if (readl(drproc->chipsig) & SYSCFG_CHIPSIG2) { + dev_err(rproc->dev.parent, + "failed to receive ack\n"); + timed_out = 1; + } + + break; + } + + if (!timed_out) + /* Interupt remote proc */ + writel(SYSCFG_CHIPSIG2, drproc->chipsig); +} + +static struct rproc_ops da8xx_rproc_ops = { + .start = da8xx_rproc_start, + .stop = da8xx_rproc_stop, + .kick = da8xx_rproc_kick, +}; + +static int reset_assert(struct device *dev) +{ + struct clk *dsp_clk; + + dsp_clk = clk_get(dev, NULL); + if (IS_ERR(dsp_clk)) { + dev_err(dev, "clk_get error: %ld\n", PTR_ERR(dsp_clk)); + return PTR_RET(dsp_clk); + } + davinci_clk_reset_assert(dsp_clk); + clk_put(dsp_clk); + + return 0; +} + +static int da8xx_rproc_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct da8xx_rproc *drproc; + struct rproc *rproc; + struct irq_data *irq_data; + struct resource *bootreg_res; + struct resource *chipsig_res; + void __iomem *chipsig; + void __iomem *bootreg; + int irq; + int ret; + + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + dev_err(dev, "platform_get_irq(pdev, 0) error: %d\n", irq); + return irq; + } + + irq_data = irq_get_irq_data(irq); + if (!irq_data) { + dev_err(dev, "irq_get_irq_data(%d): NULL\n", irq); + return -EINVAL; + } + + bootreg_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!bootreg_res) { + dev_err(dev, + "platform_get_resource(IORESOURCE_MEM, 0): NULL\n"); + return -EADDRNOTAVAIL; + } + + chipsig_res = platform_get_resource(pdev, IORESOURCE_MEM, 1); + if (!chipsig_res) { + dev_err(dev, + "platform_get_resource(IORESOURCE_MEM, 1): NULL\n"); + return -EADDRNOTAVAIL; + } + + bootreg = devm_request_and_ioremap(dev, bootreg_res); + if (!bootreg) { + dev_err(dev, "unable to map boot register\n"); + return -EADDRNOTAVAIL; + } + + chipsig = devm_request_and_ioremap(dev, chipsig_res); + if (!chipsig) { + dev_err(dev, "unable to map CHIPSIG register\n"); + return -EADDRNOTAVAIL; + } + + rproc = rproc_alloc(dev, "dsp", &da8xx_rproc_ops, da8xx_fw_name, + sizeof(*drproc)); + if (!rproc) + return -ENOMEM; + + drproc = rproc->priv; + drproc->rproc = rproc; + + platform_set_drvdata(pdev, rproc); + + /* + * rproc_add() can end up enabling the DSP's clk with the DSP + * *not* in reset, but da8xx_rproc_start() needs the DSP to be + * held in reset at the time it is called. + */ + ret = reset_assert(dev); + if (ret) + goto free_rproc; + + ret = rproc_add(rproc); + if (ret) { + dev_err(dev, "rproc_add failed: %d\n", ret); + goto free_rproc; + } + + drproc->chipsig = chipsig; + drproc->bootreg = bootreg; + drproc->ack_fxn = irq_data->chip->irq_ack; + drproc->irq_data = irq_data; + drproc->irq = irq; + + /* everything the ISR needs is now setup, so hook it up */ + ret = devm_request_threaded_irq(dev, irq, da8xx_rproc_callback, + handle_event, 0, "da8xx-remoteproc", rproc); + if (ret) { + dev_err(dev, "devm_request_threaded_irq error: %d\n", ret); + rproc_del(rproc); + goto free_rproc; + } + + return 0; + +free_rproc: + rproc_put(rproc); + + return ret; +} + +static int da8xx_rproc_remove(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct rproc *rproc = platform_get_drvdata(pdev); + struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv; + int ret; + + /* + * It's important to place the DSP in reset before going away, + * since a subsequent insmod of this module may enable the DSP's + * clock before its program/boot-address has been loaded and + * before this module's probe has had a chance to reset the DSP. + * Without the reset, the DSP can lockup permanently when it + * begins executing garbage. + */ + reset_assert(dev); + + /* + * The devm subsystem might end up releasing things before + * freeing the irq, thus allowing an interrupt to sneak in while + * the device is being removed. This should prevent that. + */ + disable_irq(drproc->irq); + + ret = rproc_del(rproc); + rproc_put(rproc); + + return ret; +} + +static struct platform_driver da8xx_rproc_driver = { + .probe = da8xx_rproc_probe, + .remove = da8xx_rproc_remove, + .driver = { + .name = "davinci-rproc", + .owner = THIS_MODULE, + }, +}; + +module_platform_driver(da8xx_rproc_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("DA8XX Remote Processor control driver"); diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index dd3bfaf..e0f703e 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -1222,19 +1222,39 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, const char *firmware, int len) { struct rproc *rproc; + char *template = "rproc-%s-fw"; + char *p; if (!dev || !name || !ops) return NULL; + if (!firmware) + /* + * Make room for default firmware name (minus %s plus '\0'). + * If the caller didn't pass in a firmware name then + * construct a default name. We're already glomming 'len' + * bytes onto the end of the struct rproc allocation, so do + * a few more for the default firmware name (but only if + * the caller doesn't pass one). + */ + len += strlen(name) + strlen(template) - 2 + 1; + rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL); if (!rproc) { dev_err(dev, "%s: kzalloc failed\n", __func__); return NULL; } + if (!firmware) { + p = (char *)rproc + sizeof(struct rproc) + len; + sprintf(p, template, name); + } else { + p = (char *)firmware; + } + + rproc->firmware = p; rproc->name = name; rproc->ops = ops; - rproc->firmware = firmware; rproc->priv = &rproc[1]; device_initialize(&rproc->dev);
Adding a remoteproc driver implementation for OMAP-L138 DSP Adding support for a default firmware name Signed-off-by: Robert Tivy <rtivy@ti.com> --- drivers/remoteproc/Kconfig | 29 ++- drivers/remoteproc/Makefile | 1 + drivers/remoteproc/da8xx_remoteproc.c | 346 +++++++++++++++++++++++++++++++++ drivers/remoteproc/remoteproc_core.c | 22 ++- 4 files changed, 393 insertions(+), 5 deletions(-) create mode 100644 drivers/remoteproc/da8xx_remoteproc.c