> -----Original Message----- > From: Nori, Sekhar > Sent: Sunday, January 20, 2013 9:39 PM > > On 1/11/2013 5:53 AM, Robert Tivy wrote: > > Adding a remoteproc driver implementation for OMAP-L138 DSP > > > > Signed-off-by: Robert Tivy <rtivy@xxxxxx> > > --- > > drivers/remoteproc/Kconfig | 23 ++ > > drivers/remoteproc/Makefile | 1 + > > drivers/remoteproc/davinci_remoteproc.c | 307 > ++++++++++++++++++++++++ > > include/linux/platform_data/da8xx-remoteproc.h | 33 +++ > > naming the driver davinci_remoteproc.c and platform data as > da8xx-remoteproc.h is odd. The driver looks really specific to omap- > l138 > so may be call the driver da8xx-remoteproc.c? At first when I started working on this stuff we intended to unify this naming, to either "davinci"-based or "da8xx"-based, but after looking at it for a while we realized that it is already so hopelessly mixed in many areas that it wasn't worth the effort. But for new stuff that's directly tied to omap-l138 we should be consistent, so I agree. I'll change it to da8xx-remoteproc.c, at the same time changing all the code/data names to be prefixed with "da8xx_". > > > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig > > index 96ce101..7d3a5e0 100644 > > --- a/drivers/remoteproc/Kconfig > > +++ b/drivers/remoteproc/Kconfig > > @@ -41,4 +41,27 @@ config STE_MODEM_RPROC > > This can be either built-in or a loadable module. > > If unsure say N. > > > > +config DAVINCI_REMOTEPROC > > + tristate "DaVinci DA850/OMAPL138 remoteproc support > (EXPERIMENTAL)" > > + depends on ARCH_DAVINCI_DA850 > > + select REMOTEPROC > > + select RPMSG > > + select FW_LOADER > > + select CMA > > + default n > > + help > > + Say y here to support DaVinci 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). > > + > > + It's safe to say n here if you're not interested in multimedia > > + offloading or just want a bare minimum kernel. > > > + This feature is considered EXPERIMENTAL, due to it not having > > + any previous exposure to the general public and therefore > > + limited developer-based testing. > > This is probably true in general for remoteproc (I am being warned a > lot > by the framework when using it) so may be you can drop this specific > reference. OK, we'll let the overlying REMOTEPROC EXPERIMENTAL status preside. > > > diff --git a/drivers/remoteproc/davinci_remoteproc.c > b/drivers/remoteproc/davinci_remoteproc.c > > new file mode 100644 > > index 0000000..fc6fd72 > > --- /dev/null > > +++ b/drivers/remoteproc/davinci_remoteproc.c > > @@ -0,0 +1,307 @@ > > +/* > > + * Remote processor machine-specific module for Davinci > > + * > > + * Copyright (C) 2011-2012 Texas Instruments, Inc. > > 2013? Yep. > > > + * > > + * 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. > > + */ > > + > > +#define pr_fmt(fmt) "%s: " fmt, __func__ > > You dont seem to be using this anywhere. Will remove. > > > + > > +#include <linux/kernel.h> > > +#include <linux/err.h> > > +#include <linux/printk.h> > > +#include <linux/bitops.h> > > +#include <linux/platform_device.h> > > +#include <linux/remoteproc.h> > > +#include <linux/platform_data/da8xx-remoteproc.h> > > +#include <linux/clk.h> > > +#include <linux/io.h> > > +#include <linux/module.h> > > +#include <linux/interrupt.h> > > +#include <linux/irq.h> > > It will be nice to keep this sorted. It avoids duplicate includes > later. OK > > > +static char *fw_name; > > +module_param(fw_name, charp, S_IRUGO); > > +MODULE_PARM_DESC(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_CHIPSIG_OFFSET 0x174 > > +#define SYSCFG_CHIPSIG_CLR_OFFSET 0x178 > > +#define SYSCFG_CHIPINT0 (1 << 0) > > +#define SYSCFG_CHIPINT1 (1 << 1) > > +#define SYSCFG_CHIPINT2 (1 << 2) > > +#define SYSCFG_CHIPINT3 (1 << 3) > > You can use BIT(x) here. Will do. > > > + > > +/** > > + * struct davinci_rproc - davinci remote processor state > > + * @rproc: rproc handle > > + */ > > +struct davinci_rproc { > > + struct rproc *rproc; > > + struct clk *dsp_clk; > > +}; > > + > > +static void __iomem *syscfg0_base; > > +static struct platform_device *remoteprocdev; > > +static struct irq_data *irq_data; > > +static void (*ack_fxn)(struct irq_data *data); > > +static int irq; > > + > > +/** > > + * handle_event() - inbound virtqueue message workqueue function > > + * > > + * This funciton 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 = platform_get_drvdata(remoteprocdev); > > + > > + /* 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; > > +} > > + > > +/** > > + * davinci_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 davinci_rproc_callback(int irq, void *p) > > +{ > > + if (readl(syscfg0_base + SYSCFG_CHIPSIG_OFFSET) & > SYSCFG_CHIPINT0) { > > personally I think using a variable to read the register and then > testing its value inside if() is more readable. Agreed. > > > + /* Clear interrupt level source */ > > + writel(SYSCFG_CHIPINT0, > > + syscfg0_base + SYSCFG_CHIPSIG_CLR_OFFSET); > > + > > + /* > > + * 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. > > + */ > > + ack_fxn(irq_data); > > Don't really like interrupt controller functions being invoked like > this > but I don't understand the underlying issue well enough to suggest an > alternate. I've looked for an alternative with no success, but will start a discussion in earnest after tackling this initial commit. > > > + > > + return IRQ_WAKE_THREAD; > > + } > > + > > + return IRQ_HANDLED; > > +} > > + > > +static int davinci_rproc_start(struct rproc *rproc) > > +{ > > + struct platform_device *pdev = to_platform_device(rproc- > >dev.parent); > > + struct device *dev = rproc->dev.parent; > > + struct davinci_rproc *drproc = rproc->priv; > > + struct clk *dsp_clk; > > + struct resource *r; > > + unsigned long host1cfg_physaddr; > > + unsigned int host1cfg_offset; > > + int ret; > > + > > + remoteprocdev = pdev; > > + > > + /* 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; > > + } > > + > > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > Along with moving this to probe as Ohad requested, you can use > devm_request_and_ioremap() to simplify the error paths here. Have a > look > at: Documentation/driver-model/devres.txt So it seems that I should also use devm_request_threaded_irq()? In general, should I always use the "devm_*" APIs when available? > > > + if (IS_ERR_OR_NULL(r)) { > > + dev_err(dev, "platform_get_resource() error: %ld\n", > > + PTR_ERR(r)); > > + > > + return PTR_ERR(r); > > + } > > + host1cfg_physaddr = (unsigned long)r->start; > > + > > + 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 (IS_ERR_OR_NULL(irq_data)) { > > + dev_err(dev, "irq_get_irq_data(%d) error: %ld\n", > > + irq, PTR_ERR(irq_data)); > > + > > + return PTR_ERR(irq_data); > > + } > > + ack_fxn = irq_data->chip->irq_ack; > > + > > + ret = request_threaded_irq(irq, davinci_rproc_callback, > handle_event, > > + 0, "davinci-remoteproc", drproc); > > + if (ret) { > > + dev_err(dev, "request_threaded_irq error: %d\n", ret); > > + > > + return ret; > > + } > > + > > + syscfg0_base = ioremap(host1cfg_physaddr & PAGE_MASK, SZ_4K); > > + host1cfg_offset = offset_in_page(host1cfg_physaddr); > > + writel(rproc->bootaddr, syscfg0_base + host1cfg_offset); > > + > > + dsp_clk = clk_get(dev, NULL); > > devm_clk_get() here. OK. > > > + if (IS_ERR_OR_NULL(dsp_clk)) { > > + dev_err(dev, "clk_get error: %ld\n", PTR_ERR(dsp_clk)); > > + ret = PTR_ERR(dsp_clk); > > + goto fail; > > + } > > + clk_enable(dsp_clk); > > + davinci_clk_reset_deassert(dsp_clk); > > + > > + drproc->dsp_clk = dsp_clk; > > + > > + return 0; > > +fail: > > + free_irq(irq, drproc); > > + iounmap(syscfg0_base); > > + > > + return ret; > > +} > > + > > +static int davinci_rproc_stop(struct rproc *rproc) > > +{ > > + struct davinci_rproc *drproc = rproc->priv; > > + struct clk *dsp_clk = drproc->dsp_clk; > > + > > + clk_disable(dsp_clk); > > + clk_put(dsp_clk); > > + iounmap(syscfg0_base); > > + free_irq(irq, drproc); > > + > > + return 0; > > +} > > + > > +/* kick a virtqueue */ > > +static void davinci_rproc_kick(struct rproc *rproc, int vqid) > > +{ > > + int timed_out; > > + unsigned long timeout; > > + > > + timed_out = 0; > > + timeout = jiffies + HZ/100; > > + > > + /* Poll for ack from other side first */ > > + while (readl(syscfg0_base + SYSCFG_CHIPSIG_OFFSET) & > > + SYSCFG_CHIPINT2) > > If there is a context switch here long enough .. > > > + if (time_after(jiffies, timeout)) { > > > .. then time_after() might return true and you will erroneously report > a > timeout even though hardware is working perfectly fine. I will probably drop the whole check completely. Otherwise I'll add another check after detecting timeout. Thanks & Regards, - Rob > > Thanks, > Sekhar ��.n��������+%������w��{.n�����{����* jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
