| Message ID | 1357863807-380-7-git-send-email-rtivy@ti.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Robert, I'm happy to see this driver going upstream, thanks for pushing! Please see below my few comments. PS - sorry for the belated review. On Fri, Jan 11, 2013 at 2:23 AM, Robert Tivy <rtivy@ti.com> wrote: > +config DAVINCI_REMOTEPROC > + tristate "DaVinci DA850/OMAPL138 remoteproc support (EXPERIMENTAL)" > + depends on ARCH_DAVINCI_DA850 > + select REMOTEPROC > + select RPMSG > + select FW_LOADER This one gets selected by CONFIG_REMOTEPROC, so you don't need to do so too. > 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 > +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) > + > +/** > + * struct davinci_rproc - davinci remote processor state > + * @rproc: rproc handle Add @dsp_clk ? > + */ > +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; Is it safe to maintain these as globals (i.e. what if we have more than a single pdev instance) ? > + > +/** > + * handle_event() - inbound virtqueue message workqueue function > + * > + * This funciton is registered as a kernel thread and is scheduled by the typo > + * kernel handler. > + */ > +static irqreturn_t handle_event(int irq, void *p) > +{ > + struct rproc *rproc = platform_get_drvdata(remoteprocdev); It's probably better to pass this as an irq cookie instead of relying on global data > + > + /* Process incoming buffers on our vring */ > + while (IRQ_HANDLED == rproc_vq_interrupt(rproc, 0)) > + ; This is interesting. IIUC, you want a single irq event to trigger processing of all the available messages. It makes sense, but I'm not sure we need this to be platform-specific. > + > + /* Must allow wakeup of potenitally blocking senders: */ > + rproc_vq_interrupt(rproc, 1); IIUC, you do this is because you have a single irq for all the vrings (PCMIIW). We may want to add something in these lines to the generic remoteproc framework, as additional platforms require it (e.g. keystone). I remember a similar patch by Cyril was circulating internally but never hit the mailing lists - you may want to take it even though it would probably need to be refreshed. > +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); > + 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); > + 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; > + } There's a lot in this ->start() method that better move to ->probe() because it should be invoked only once throughout the life cycle of this driver (probably most of the code above). Can you please take a look? > +/* 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 (time_after(jiffies, timeout)) { > + dev_err(rproc->dev.parent, "failed to receive ack\n"); > + timed_out = 1; > + > + break; > + } Can you please explain what this ack is a bit ? Just out of curiosity. > +static int davinci_rproc_probe(struct platform_device *pdev) > +{ > + struct da8xx_rproc_pdata *pdata = pdev->dev.platform_data; > + struct davinci_rproc *drproc; > + struct rproc *rproc; > + struct clk *dsp_clk; > + int ret; > + > + if (!fw_name) { > + dev_err(&pdev->dev, "No firmware file specified\n"); > + > + return -EINVAL; > + } There are a few issues with this fw_name module param: 1. Usually we don't rely on users providing the firmware file name for drivers to work. Drivers should know the name beforehand, and if there may be several different instances of firmwares (for different cores you may have), then it's just better to get it from the platform data. 2. You may still want to have such a module param in order to be able to override the default firmware name (for debugging purposes?), but I'm not sure it should be davinci-specific. if we do want it to be then please prefix the name with 'davinci'. > + ret = rproc_add(rproc); > + if (ret) > + goto free_rproc; > + > + /* > + * rproc_add() can end up enabling the DSP's clk with the DSP > + * *not* in reset, but davinci_rproc_start() needs the DSP to be > + * held in reset at the time it is called. > + */ > + dsp_clk = clk_get(rproc->dev.parent, NULL); > + davinci_clk_reset_assert(dsp_clk); > + clk_put(dsp_clk); This looks racy. Don't you prefer to assert the reset line before you call rproc_add ? > +static int __devexit davinci_rproc_remove(struct platform_device *pdev) > +{ > + struct rproc *rproc = platform_get_drvdata(pdev); > + int ret; > + > + ret = rproc_del(rproc); > + if (ret) > + return ret; Personally I'd not test ret here. I know there's a nice check for !rproc inside rproc_del, but frankly I'd prefer the system to crash if the developer blew up so badly. It's nothing I'd want to be silently buried. > diff --git a/include/linux/platform_data/da8xx-remoteproc.h b/include/linux/platform_data/da8xx-remoteproc.h > new file mode 100644 > index 0000000..50e8c55 > --- /dev/null > +++ b/include/linux/platform_data/da8xx-remoteproc.h > @@ -0,0 +1,33 @@ > +/* > + * Remote Processor > + * > + * Copyright (C) 2011-2012 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. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#ifndef __DA8XX_REMOTEPROC_H__ > +#define __DA8XX_REMOTEPROC_H__ > + > +#include <linux/remoteproc.h> You should be able to avoid including this header by forward declaring struct rproc_ops. Thanks, Ohad.
Hi Ohad, Glad to see you jump in the fray, please see responses below... > -----Original Message----- > From: Ohad Ben-Cohen [mailto:ohad@wizery.com] > Sent: Friday, January 11, 2013 4:26 AM > To: Tivy, Robert > Cc: davinci-linux-open-source; linux-arm; Nori, Sekhar; Ring, Chris; > Grosen, Mark; rob@landley.net; linux-doc@vger.kernel.org; Chemparathy, > Cyril > Subject: Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for > OMAP-L138 DSP > > Hi Robert, > > I'm happy to see this driver going upstream, thanks for pushing! > > Please see below my few comments. PS - sorry for the belated review. > > On Fri, Jan 11, 2013 at 2:23 AM, Robert Tivy <rtivy@ti.com> wrote: > > +config DAVINCI_REMOTEPROC > > + tristate "DaVinci DA850/OMAPL138 remoteproc support > (EXPERIMENTAL)" > > + depends on ARCH_DAVINCI_DA850 > > + select REMOTEPROC > > + select RPMSG > > + select FW_LOADER > > This one gets selected by CONFIG_REMOTEPROC, so you don't need to do so > too. >From drivers/remoteproc/Kconfig: # REMOTEPROC gets selected by whoever wants it config REMOTEPROC tristate depends on EXPERIMENTAL depends on HAS_DMA select FW_CONFIG select VIRTIO Is FW_CONFIG above supposed to be FW_LOADER? I don't see any support for FW_CONFIG anywhere in the kernel. > > > 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 > > +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) > > + > > +/** > > + * struct davinci_rproc - davinci remote processor state > > + * @rproc: rproc handle > > Add @dsp_clk ? Yep. > > > + */ > > +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; > > Is it safe to maintain these as globals (i.e. what if we have more > than a single pdev instance) ? I think it makes sense for some of them to be globals, will review/change accordingly. > > > + > > +/** > > + * handle_event() - inbound virtqueue message workqueue function > > + * > > + * This funciton is registered as a kernel thread and is scheduled > by the > > typo Will fix. > > > + * kernel handler. > > + */ > > +static irqreturn_t handle_event(int irq, void *p) > > +{ > > + struct rproc *rproc = platform_get_drvdata(remoteprocdev); > > It's probably better to pass this as an irq cookie instead of relying > on global data Agreed, will change. > > > + > > + /* Process incoming buffers on our vring */ > > + while (IRQ_HANDLED == rproc_vq_interrupt(rproc, 0)) > > + ; > > This is interesting. IIUC, you want a single irq event to trigger > processing of all the available messages. It makes sense, but I'm not > sure we need this to be platform-specific. YUC :) > > > + > > + /* Must allow wakeup of potenitally blocking senders: */ > > + rproc_vq_interrupt(rproc, 1); > > IIUC, you do this is because you have a single irq for all the vrings > (PCMIIW). YUC again. > > We may want to add something in these lines to the generic remoteproc > framework, as additional platforms require it (e.g. keystone). I > remember a similar patch by Cyril was circulating internally but never > hit the mailing lists - you may want to take it even though it would > probably need to be refreshed. Ok, I got Cyril's patch (sent privately by you) and will incorporate it in the generic processing, and modify davinci_remoteproc.c to: while (IRQ_HANDLED == rproc_vq_interrupt(rproc, -1)) ; > > > +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); > > + 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); > > + 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; > > + } > > There's a lot in this ->start() method that better move to ->probe() > because it should be invoked only once throughout the life cycle of > this driver (probably most of the code above). Can you please take a > look? Will take a look. > > > +/* 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 (time_after(jiffies, timeout)) { > > + dev_err(rproc->dev.parent, "failed to receive > ack\n"); > > + timed_out = 1; > > + > > + break; > > + } > > Can you please explain what this ack is a bit ? Just out of curiosity. We're currently handling the CHIPINT lines as "level"s, since they're completely controlled by SW. The interruptor raises the line and the interruptee lowers (clears) it. In a situation where every interrupt is considered to be a signal of new data arrival we need to make sure that the other side has "seen" the previous interrupt before we raise another one. For the OMAP-L138 DSP VirtQueue implementation (in the sysbios-rpmsg repo) this is currently the case - each DSP interrupt indicates new vring availability. This behavior is due to the fact that the OMAP-L138 port is trying to emulate an omap mailbox. > > > +static int davinci_rproc_probe(struct platform_device *pdev) > > +{ > > + struct da8xx_rproc_pdata *pdata = pdev->dev.platform_data; > > + struct davinci_rproc *drproc; > > + struct rproc *rproc; > > + struct clk *dsp_clk; > > + int ret; > > + > > + if (!fw_name) { > > + dev_err(&pdev->dev, "No firmware file specified\n"); > > + > > + return -EINVAL; > > + } > > There are a few issues with this fw_name module param: > > 1. Usually we don't rely on users providing the firmware file name for > drivers to work. Drivers should know the name beforehand, and if there > may be several different instances of firmwares (for different cores > you may have), then it's just better to get it from the platform data. Is this suggesting that there be separate platform device instances for each different potential fw, and that each platform device instance hardcodes the fw filename? > > 2. You may still want to have such a module param in order to be able > to override the default firmware name (for debugging purposes?), but > I'm not sure it should be davinci-specific. if we do want it to be > then please prefix the name with 'davinci'. Sekhar asked that there not be a default fw name, so there's conflicting feedback on this point. I prefer to have a default name plus the module parameter override (but don't have much opinion on whether it should be davinci-specific (and passed with davinci_remoteproc.ko) or general (and passed with remoteproc.ko), please advise). Since the fw file (i.e., DSP program) is typically paired with a particular Linux app, I like the ability to specify the fw filename at runtime, depending on the Linux app I need to run. > > > + ret = rproc_add(rproc); > > + if (ret) > > + goto free_rproc; > > + > > + /* > > + * rproc_add() can end up enabling the DSP's clk with the DSP > > + * *not* in reset, but davinci_rproc_start() needs the DSP to > be > > + * held in reset at the time it is called. > > + */ > > + dsp_clk = clk_get(rproc->dev.parent, NULL); > > + davinci_clk_reset_assert(dsp_clk); > > + clk_put(dsp_clk); > > This looks racy. Don't you prefer to assert the reset line before you > call rproc_add ? I would prefer that, as long as the reset state is not changed by rproc_add(). I will look into it. > > > +static int __devexit davinci_rproc_remove(struct platform_device > *pdev) > > +{ > > + struct rproc *rproc = platform_get_drvdata(pdev); > > + int ret; > > + > > + ret = rproc_del(rproc); > > + if (ret) > > + return ret; > > Personally I'd not test ret here. > > I know there's a nice check for !rproc inside rproc_del, but frankly > I'd prefer the system to crash if the developer blew up so badly. It's > nothing I'd want to be silently buried. Ok, I'll trust you on this one :) > > > diff --git a/include/linux/platform_data/da8xx-remoteproc.h > b/include/linux/platform_data/da8xx-remoteproc.h > > new file mode 100644 > > index 0000000..50e8c55 > > --- /dev/null > > +++ b/include/linux/platform_data/da8xx-remoteproc.h > > @@ -0,0 +1,33 @@ > > +/* > > + * Remote Processor > > + * > > + * Copyright (C) 2011-2012 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. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#ifndef __DA8XX_REMOTEPROC_H__ > > +#define __DA8XX_REMOTEPROC_H__ > > + > > +#include <linux/remoteproc.h> > > You should be able to avoid including this header by forward declaring > struct rproc_ops. Will do. Thanks & Regards, - Rob > > Thanks, > Ohad.
On Fri, Jan 11, 2013 at 02:26:19PM +0200, Ohad Ben-Cohen wrote: > > +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); > > + if (IS_ERR_OR_NULL(r)) { No, this is buggy. Go and look up to see what the return ranges are for this function. > > + dev_err(dev, "platform_get_resource() error: %ld\n", > > + PTR_ERR(r)); > > + > > + return PTR_ERR(r); Which results in this being a bug. > > + } > > + 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)) { Again, bug. > > + dev_err(dev, "irq_get_irq_data(%d) error: %ld\n", > > + irq, PTR_ERR(irq_data)); > > + > > + return PTR_ERR(irq_data); Which results in this being a bug. > > + } > > + 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); > > + if (IS_ERR_OR_NULL(dsp_clk)) { And another bug. > > + dev_err(dev, "clk_get error: %ld\n", PTR_ERR(dsp_clk)); > > + ret = PTR_ERR(dsp_clk); And again, results in this being a bug. > > + goto fail; > > + } ... > > + ret = rproc_add(rproc); > > + if (ret) > > + goto free_rproc; > > + > > + /* > > + * rproc_add() can end up enabling the DSP's clk with the DSP > > + * *not* in reset, but davinci_rproc_start() needs the DSP to be > > + * held in reset at the time it is called. > > + */ > > + dsp_clk = clk_get(rproc->dev.parent, NULL); > > + davinci_clk_reset_assert(dsp_clk); > > + clk_put(dsp_clk); BUG: what if clk_get() fails here?
On 1/12/2013 7:56 AM, Tivy, Robert wrote: >> From: Ohad Ben-Cohen [mailto:ohad@wizery.com] >> Sent: Friday, January 11, 2013 4:26 AM >> On Fri, Jan 11, 2013 at 2:23 AM, Robert Tivy <rtivy@ti.com> wrote: >>> +static int davinci_rproc_probe(struct platform_device *pdev) >>> +{ >>> + struct da8xx_rproc_pdata *pdata = pdev->dev.platform_data; >>> + struct davinci_rproc *drproc; >>> + struct rproc *rproc; >>> + struct clk *dsp_clk; >>> + int ret; >>> + >>> + if (!fw_name) { >>> + dev_err(&pdev->dev, "No firmware file specified\n"); >>> + >>> + return -EINVAL; >>> + } >> There are a few issues with this fw_name module param: >> >> 1. Usually we don't rely on users providing the firmware file name for >> drivers to work. Drivers should know the name beforehand, and if there >> may be several different instances of firmwares (for different cores >> you may have), then it's just better to get it from the platform data. > > Is this suggesting that there be separate platform device instances for each different potential fw, and that each platform device instance hardcodes the fw filename? I am not convinced firmware name should be in platform data (or DT) since it is not hardware specific. User can choose multiple different firmwares to load on the DSP depending the application he is running all for the same platform (da850 evm). > >> >> 2. You may still want to have such a module param in order to be able >> to override the default firmware name (for debugging purposes?), but >> I'm not sure it should be davinci-specific. if we do want it to be >> then please prefix the name with 'davinci'. > > Sekhar asked that there not be a default fw name, so there's conflicting feedback on this point. I prefer to have a default name plus the module parameter override (but don't have much opinion on whether it should be davinci-specific (and passed with davinci_remoteproc.ko) or general (and passed with remoteproc.ko), please advise). Rob, I don't remember objecting to a default firmware name if module parameter is not passed. On 29th November 2012 you wrote: " Sounds OK. I propose then to have the above be the default firmware name, along with a module parameter that will override if specified. " and I wrote back: " Sounds good. " As you can see, there was no objection from me. > > Since the fw file (i.e., DSP program) is typically paired with a particular Linux app, I like the ability to specify the fw filename at runtime, depending on the Linux app I need to run. Right, and platform data is not the way to achieve this. Thanks, Sekhar
Hi Rob, On Sat, Jan 12, 2013 at 4:26 AM, Tivy, Robert <rtivy@ti.com> wrote: > Is FW_CONFIG above supposed to be FW_LOADER? That FW_CONFIG is completely bogus of course. care to fix it in your series? > We're currently handling the CHIPINT lines as "level"s, since they're completely controlled by SW. The interruptor raises the line and the interruptee lowers (clears) it. In a situation where every interrupt is considered to be a signal of new data arrival we need to make sure that the other side has "seen" the previous interrupt before we raise another one. What if we don't ? Can the DSP side just go over the vrings until no new msg is left (similar to what you do on the Linux side) ? > Is this suggesting that there be separate platform device instances for each different potential fw, and that each platform device instance hardcodes the fw filename? In principle this same driver can drive many instances of remote processors you may have on your board. E.g. in OMAP the same driver controls both the M3s and the DSP. pdata is then used to hold information specific to the hw instance. I'm not sure if there's (or will be) any DaVinci platform with several remote processors but it's a better practice to write the code as if there is/will be - it's usually cleaner and will just work in case a platform with multiple rprocs does show up one day. > Sekhar asked that there not be a default fw name, so there's conflicting feedback on this point. I prefer to have a default name plus the module parameter override (but don't have much opinion on whether it should be davinci-specific (and passed with davinci_remoteproc.ko) or general (and passed with remoteproc.ko), please advise). > > Since the fw file (i.e., DSP program) is typically paired with a particular Linux app, I like the ability to specify the fw filename at runtime, depending on the Linux app I need to run. Sure, no reason why not to allow users to override the default fw name. Thanks, Ohad.
On Tue, Jan 15, 2013 at 11:15 AM, Sekhar Nori <nsekhar@ti.com> wrote:
> Right, and platform data is not the way to achieve this.
How do you suggest to handle platforms with multiple different remote
processors (driven by the same driver) ?
Requiring the user to indicate the fw name for each processor is
somewhat error prone/cumbersome.
Thanks,
Ohad.
On 1/15/2013 3:33 PM, Ohad Ben-Cohen wrote: > On Tue, Jan 15, 2013 at 11:15 AM, Sekhar Nori <nsekhar@ti.com> wrote: >> Right, and platform data is not the way to achieve this. > > How do you suggest to handle platforms with multiple different remote > processors (driven by the same driver) ? > > Requiring the user to indicate the fw name for each processor is > somewhat error prone/cumbersome. May be rproc_alloc() could auto-assign the firmware name to something like 'rproc%d-fw' if firmware name passed to it is NULL? Thanks, Sekhar
On Tue, Jan 15, 2013 at 2:29 PM, Sekhar Nori <nsekhar@ti.com> wrote: > May be rproc_alloc() could auto-assign the firmware name to something > like 'rproc%d-fw' if firmware name passed to it is NULL? I prefer we use name-based filenames instead to make it easier for users (and us developers). We can probably do something like "rproc-%s-fw" with pdata->name assuming we/you do maintain a meaningful name in the latter. Thanks, Ohad.
> -----Original Message----- > From: Ohad Ben-Cohen [mailto:ohad@wizery.com] > Sent: Tuesday, January 15, 2013 4:49 AM > To: Nori, Sekhar > Cc: Tivy, Robert; davinci-linux-open-source; linux-arm; Ring, Chris; > Grosen, Mark; rob@landley.net; linux-doc@vger.kernel.org; Chemparathy, > Cyril > Subject: Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for > OMAP-L138 DSP > > On Tue, Jan 15, 2013 at 2:29 PM, Sekhar Nori <nsekhar@ti.com> wrote: > > May be rproc_alloc() could auto-assign the firmware name to something > > like 'rproc%d-fw' if firmware name passed to it is NULL? > > I prefer we use name-based filenames instead to make it easier for > users (and us developers). > > We can probably do something like "rproc-%s-fw" with pdata->name > assuming we/you do maintain a meaningful name in the latter. This sounds good, although it will introduce the need to handle dynamic storage for the generated name. I think I can jam that storage on the end of the already-dynamically-sized 'struct rproc + sizeof(pdata)' allocation in rproc_alloc(). Thanks & Regards, - Rob > > Thanks, > Ohad.
On Wed, Jan 16, 2013 at 1:06 AM, Tivy, Robert <rtivy@ti.com> wrote:
> This sounds good, although it will introduce the need to handle dynamic storage for the generated name. I think I can jam that storage on the end of the already-dynamically-sized 'struct rproc + sizeof(pdata)' allocation in rproc_alloc().
Or you can generate it when needed, without storing the result. This
rarely happens and anyway is negligible.
Thanks,
Ohad.
On 1/15/2013 6:19 PM, Ohad Ben-Cohen wrote: > On Tue, Jan 15, 2013 at 2:29 PM, Sekhar Nori <nsekhar@ti.com> wrote: >> May be rproc_alloc() could auto-assign the firmware name to something >> like 'rproc%d-fw' if firmware name passed to it is NULL? > > I prefer we use name-based filenames instead to make it easier for > users (and us developers). > > We can probably do something like "rproc-%s-fw" with pdata->name > assuming we/you do maintain a meaningful name in the latter. Are you thinking of passing name of the remote processor (like m3, dsp0, dsp1 etc) in pdata->name? That sounds OK since the processor name is actually tied to the platform. BTW, the current driver seems to be written for OMAP-L138 rather tightly so you could as well hardcode the firmware name to 'rproc-dsp-fw'. Thanks, Sekhar
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@ti.com> > --- > 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? > 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. > 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? > + * > + * 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. > + > +#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. > +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. > + > +/** > + * 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. > + /* 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. > + > + 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 > + 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. > + 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. Thanks, Sekhar
On Mon, Jan 21, 2013 at 11:08:43AM +0530, Sekhar Nori wrote: > > + if (IS_ERR_OR_NULL(r)) { > > + dev_err(dev, "platform_get_resource() error: %ld\n", > > + PTR_ERR(r)); > > + > > + return PTR_ERR(r); Sigh. Bug. > > + } > > + 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); Bug. > > + } > > + 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. > > > + if (IS_ERR_OR_NULL(dsp_clk)) { > > + dev_err(dev, "clk_get error: %ld\n", PTR_ERR(dsp_clk)); > > + ret = PTR_ERR(dsp_clk); Bug. See, yet again... almost every use of IS_ERR_OR_NULL() is a bug.
Russell, thankyou for the review and notice, all this will be straightened out in the next patch submission. Regards, - Rob > -----Original Message----- > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk] > Sent: Monday, January 21, 2013 8:41 AM > To: Nori, Sekhar > Cc: Tivy, Robert; ohad@wizery.com; davinci-linux-open- > source@linux.davincidsp.com; linux-doc@vger.kernel.org; Ring, Chris; > rob@landley.net; Grosen, Mark; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for > OMAP-L138 DSP > > On Mon, Jan 21, 2013 at 11:08:43AM +0530, Sekhar Nori wrote: > > > + if (IS_ERR_OR_NULL(r)) { > > > + dev_err(dev, "platform_get_resource() error: %ld\n", > > > + PTR_ERR(r)); > > > + > > > + return PTR_ERR(r); > > Sigh. Bug. > > > > + } > > > + 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); > > Bug. > > > > + } > > > + 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. > > > > > + if (IS_ERR_OR_NULL(dsp_clk)) { > > > + dev_err(dev, "clk_get error: %ld\n", PTR_ERR(dsp_clk)); > > > + ret = PTR_ERR(dsp_clk); > > Bug. > > See, yet again... almost every use of IS_ERR_OR_NULL() is a bug.
> -----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@ti.com> > > --- > > 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
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. + endmenu diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile index 391b651..da2dc0a 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_DAVINCI_REMOTEPROC) += davinci_remoteproc.o 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. + * + * 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__ + +#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> + +#include <mach/clock.h> /* for davinci_clk_reset_assert/deassert() */ + +#include "remoteproc_internal.h" + +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) + +/** + * 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) { + /* 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); + + 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); + 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); + 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 (time_after(jiffies, timeout)) { + dev_err(rproc->dev.parent, "failed to receive ack\n"); + timed_out = 1; + + break; + } + + if (!timed_out) + /* Interupt remote proc */ + writel(SYSCFG_CHIPINT2, syscfg0_base + SYSCFG_CHIPSIG_OFFSET); +} + +static struct rproc_ops davinci_rproc_ops = { + .start = davinci_rproc_start, + .stop = davinci_rproc_stop, + .kick = davinci_rproc_kick, +}; + +static int davinci_rproc_probe(struct platform_device *pdev) +{ + struct da8xx_rproc_pdata *pdata = pdev->dev.platform_data; + struct davinci_rproc *drproc; + struct rproc *rproc; + struct clk *dsp_clk; + int ret; + + if (!fw_name) { + dev_err(&pdev->dev, "No firmware file specified\n"); + + return -EINVAL; + } + + dev_dbg(&pdev->dev, "module param fw_name: '%s'\n", fw_name); + pdata->firmware = fw_name; + + rproc = rproc_alloc(&pdev->dev, pdata->name, &davinci_rproc_ops, + pdata->firmware, sizeof(*drproc)); + if (!rproc) + return -ENOMEM; + + drproc = rproc->priv; + drproc->rproc = rproc; + + platform_set_drvdata(pdev, rproc); + + ret = rproc_add(rproc); + if (ret) + goto free_rproc; + + /* + * rproc_add() can end up enabling the DSP's clk with the DSP + * *not* in reset, but davinci_rproc_start() needs the DSP to be + * held in reset at the time it is called. + */ + dsp_clk = clk_get(rproc->dev.parent, NULL); + davinci_clk_reset_assert(dsp_clk); + clk_put(dsp_clk); + + return 0; + +free_rproc: + dev_err(rproc->dev.parent, "rproc_add failed: %d\n", ret); + rproc_put(rproc); + + return ret; +} + +static int __devexit davinci_rproc_remove(struct platform_device *pdev) +{ + struct rproc *rproc = platform_get_drvdata(pdev); + int ret; + + ret = rproc_del(rproc); + if (ret) + return ret; + + rproc_put(rproc); + + return 0; +} + +static struct platform_driver davinci_rproc_driver = { + .probe = davinci_rproc_probe, + .remove = __devexit_p(davinci_rproc_remove), + .driver = { + .name = "davinci-rproc", + .owner = THIS_MODULE, + }, +}; + +module_platform_driver(davinci_rproc_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("Davinci DA850 Remote Processor control driver"); diff --git a/include/linux/platform_data/da8xx-remoteproc.h b/include/linux/platform_data/da8xx-remoteproc.h new file mode 100644 index 0000000..50e8c55 --- /dev/null +++ b/include/linux/platform_data/da8xx-remoteproc.h @@ -0,0 +1,33 @@ +/* + * Remote Processor + * + * Copyright (C) 2011-2012 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. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef __DA8XX_REMOTEPROC_H__ +#define __DA8XX_REMOTEPROC_H__ + +#include <linux/remoteproc.h> + +/** + * struct da8xx_rproc_pdata - da8xx remoteproc's platform data + * @name: the remoteproc's name + * @firmware: name of firmware file to load + * @ops: start/stop rproc handlers + */ +struct da8xx_rproc_pdata { + const char *name; + const char *firmware; + const struct rproc_ops *ops; +}; + +#endif /* __DA8XX_REMOTEPROC_H__ */
Adding a remoteproc driver implementation for OMAP-L138 DSP Signed-off-by: Robert Tivy <rtivy@ti.com> --- drivers/remoteproc/Kconfig | 23 ++ drivers/remoteproc/Makefile | 1 + drivers/remoteproc/davinci_remoteproc.c | 307 ++++++++++++++++++++++++ include/linux/platform_data/da8xx-remoteproc.h | 33 +++ 4 files changed, 364 insertions(+) create mode 100644 drivers/remoteproc/davinci_remoteproc.c create mode 100644 include/linux/platform_data/da8xx-remoteproc.h