[PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[Ohad Ben-Cohen]

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.

[PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[Tivy, Robert]

Hi Ohad,

Glad to see you jump in the fray, please see responses below...

> -----Original Message-----
> From: Ohad Ben-Cohen [mailto:ohad at 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 at landley.net; linux-doc at 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.

[PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[Russell King - ARM Linux]

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?

[PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[Sekhar Nori]

On 1/12/2013 7:56 AM, Tivy, Robert wrote:

>> From: Ohad Ben-Cohen [mailto:ohad at 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

[PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[Ohad Ben-Cohen]

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.

[PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[Ohad Ben-Cohen]

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.

[PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[Sekhar Nori]

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

[PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[Ohad Ben-Cohen]

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.

[PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[Tivy, Robert]

> -----Original Message-----
> From: Ohad Ben-Cohen [mailto:ohad at 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 at landley.net; linux-doc at 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.

[PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[Ohad Ben-Cohen]

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.

[PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[Sekhar Nori]

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

[PATCH v5 1/9] ARM: davinci: da850 board: change pr_warning() to pr_warn()

[Sekhar Nori]

On 1/11/2013 5:53 AM, Robert Tivy wrote:
> Changed all pr_warning() calls to pr_warn(), as advised by checkpatch.pl.
> 
> Also, while modifying those pr_warning() calls I changed hardcoded
> function names to use '"%s:", __func__' instead, and converted acronym
> usage to upper case
> 
> Signed-off-by: Robert Tivy <rtivy@ti.com>

Applying this patch for v3.9. While committing I updated the commit
message to change past tense to present and also removed the first
person reference.

Thanks,
Sekhar

[PATCH v5 2/9] ARM: davinci: devices-da8xx.c: change pr_warning() to pr_warn()

[Sekhar Nori]

On 1/11/2013 5:53 AM, Robert Tivy wrote:
> Changed all pr_warning() calls to pr_warn(), as advised by checkpatch.pl.
> 
> Signed-off-by: Robert Tivy <rtivy@ti.com>
> ---
> Clean up files that will be otherwise modified in subsequent patch.
> 
>  arch/arm/mach-davinci/devices-da8xx.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
> index 2d5502d..fb2f51b 100644
> --- a/arch/arm/mach-davinci/devices-da8xx.c
> +++ b/arch/arm/mach-davinci/devices-da8xx.c
> @@ -751,7 +751,7 @@ void __iomem * __init da8xx_get_mem_ctlr(void)
>  
>  	da8xx_ddr2_ctlr_base = ioremap(DA8XX_DDR2_CTL_BASE, SZ_32K);
>  	if (!da8xx_ddr2_ctlr_base)
> -		pr_warning("%s: Unable to map DDR2 controller",	__func__);
> +		pr_warn("%s: Unable to map DDR2 controller", __func__);
>  
>  	return da8xx_ddr2_ctlr_base;
>  }

> @@ -876,8 +876,8 @@ int __init da8xx_register_spi(int instance, const struct spi_board_info *info,
>  
>  	ret = spi_register_board_info(info, len);
>  	if (ret)
> -		pr_warning("%s: failed to register board info for spi %d :"
> -			   " %d\n", __func__, instance, ret);
> +		pr_warn("%s: failed to register board info for spi %d : %d\n",
> +			__func__, instance, ret);

This part is not required anymore since this is already fixed by Vivien
Didelot as part of his SPI fixes. I applied rest of the patch for v3.9.
As with previous patch, I fixed the past tense to present in commit text
while applying.

Thanks,
Sekhar

[PATCH v5 4/9] ARM: davinci: da850: added pll0_sysclk1 for DSP usage

[Sekhar Nori]

On 1/11/2013 5:53 AM, Robert Tivy wrote:
> Added pll0_sysclk1 to allow clocking control for DSP
> 
> Signed-off-by: Robert Tivy <rtivy@ti.com>

Applied patches 3 and 4 as well.

Thanks,
Sekhar

[PATCH v5 5/9] ARM: davinci: New reset functionality/API provided for Davinci DSP

[Sekhar Nori]

On 1/11/2013 5:53 AM, Robert Tivy wrote:
> Since there is no general "reset" support for SoC devices, and since the
> remoteproc driver needs explicit control of the DSP's reset line, a new
> Davinci-specific API is added.
> 
> This private API will disappear with DT migration.  Some discussion
> regarding a proposed DT "reset" binding is here:
> https://patchwork.kernel.org/patch/1635051/
> 
> Modified davinci_clk_init() to set clk "reset" function for clocks
> that indicate PSC_LRST support.  Also fixed indentation issue with
> function opening curly brace.
> 
> Signed-off-by: Robert Tivy <rtivy@ti.com>

I applied this patch for v3.9. The subject seemed too long with repeated
references to davinci so I shortened it to:

ARM: davinci: psc: introduce reset API

> --- a/arch/arm/mach-davinci/include/mach/psc.h
> +++ b/arch/arm/mach-davinci/include/mach/psc.h
> @@ -246,6 +246,7 @@
>  
>  #define MDSTAT_STATE_MASK	0x3f
>  #define PDSTAT_STATE_MASK	0x1f
> +#define MDCTL_LRST		BIT(8)
>  #define MDCTL_FORCE		BIT(31)
>  #define PDCTL_NEXT		BIT(0)
>  #define PDCTL_EPCGOOD		BIT(8)
> @@ -253,6 +254,8 @@
>  #ifndef __ASSEMBLER__
>  
>  extern int davinci_psc_is_clk_active(unsigned int ctlr, unsigned int id);
> +extern void davinci_psc_reset_config(unsigned int ctlr, unsigned int id,
> +		bool reset);

I felt the word 'config' in the name is not really required since the
functionality is fixed (as opposed to the davinci_psc_config() function
which could do multiple configurations)

I updated the function name to davinci_psc_reset() when I committed the
patch locally.

Hope that's okay with you.

Thanks,
Sekhar

[PATCH v5 5/9] ARM: davinci: New reset functionality/API provided for Davinci DSP

[Tivy, Robert]

Thankyou for all your help Sekhar.  I'm fine with your fixups (both commentary and code naming).

Regards,

- Rob

> -----Original Message-----
> From: Nori, Sekhar
> Sent: Thursday, January 17, 2013 3:33 AM
> To: Tivy, Robert
> Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm-
> kernel at lists.infradead.org; Ring, Chris; Grosen, Mark; ohad at wizery.com;
> rob at landley.net; linux-doc at vger.kernel.org
> Subject: Re: [PATCH v5 5/9] ARM: davinci: New reset functionality/API
> provided for Davinci DSP
> 
> On 1/11/2013 5:53 AM, Robert Tivy wrote:
> > Since there is no general "reset" support for SoC devices, and since
> the
> > remoteproc driver needs explicit control of the DSP's reset line, a
> new
> > Davinci-specific API is added.
> >
> > This private API will disappear with DT migration.  Some discussion
> > regarding a proposed DT "reset" binding is here:
> > https://patchwork.kernel.org/patch/1635051/
> >
> > Modified davinci_clk_init() to set clk "reset" function for clocks
> > that indicate PSC_LRST support.  Also fixed indentation issue with
> > function opening curly brace.
> >
> > Signed-off-by: Robert Tivy <rtivy@ti.com>
> 
> I applied this patch for v3.9. The subject seemed too long with
> repeated
> references to davinci so I shortened it to:
> 
> ARM: davinci: psc: introduce reset API
> 
> > --- a/arch/arm/mach-davinci/include/mach/psc.h
> > +++ b/arch/arm/mach-davinci/include/mach/psc.h
> > @@ -246,6 +246,7 @@
> >
> >  #define MDSTAT_STATE_MASK	0x3f
> >  #define PDSTAT_STATE_MASK	0x1f
> > +#define MDCTL_LRST		BIT(8)
> >  #define MDCTL_FORCE		BIT(31)
> >  #define PDCTL_NEXT		BIT(0)
> >  #define PDCTL_EPCGOOD		BIT(8)
> > @@ -253,6 +254,8 @@
> >  #ifndef __ASSEMBLER__
> >
> >  extern int davinci_psc_is_clk_active(unsigned int ctlr, unsigned int
> id);
> > +extern void davinci_psc_reset_config(unsigned int ctlr, unsigned int
> id,
> > +		bool reset);
> 
> I felt the word 'config' in the name is not really required since the
> functionality is fixed (as opposed to the davinci_psc_config() function
> which could do multiple configurations)
> 
> I updated the function name to davinci_psc_reset() when I committed the
> patch locally.
> 
> Hope that's okay with you.
> 
> Thanks,
> Sekhar

[PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[Sekhar Nori]

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

[PATCH v5 7/9] ARM: davinci: Remoteproc platform device creation data/code

[Sekhar Nori]

On 1/11/2013 5:53 AM, Robert Tivy wrote:
> Added a new remoteproc platform device for DA8XX.  Contains CMA-based
> reservation of physical memory block.  A new kernel command-line
> parameter has been added to allow boot-time specification of the
> physical memory block.
> 
> Signed-off-by: Robert Tivy <rtivy@ti.com>
> ---
>  Documentation/kernel-parameters.txt        |    7 +++
>  arch/arm/mach-davinci/devices-da8xx.c      |   93 +++++++++++++++++++++++++++-
>  arch/arm/mach-davinci/include/mach/da8xx.h |    4 ++
>  3 files changed, 103 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 363e348..e95afb1 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -44,6 +44,7 @@ parameter is applicable:
>  	AVR32	AVR32 architecture is enabled.
>  	AX25	Appropriate AX.25 support is enabled.
>  	BLACKFIN Blackfin architecture is enabled.
> +	CMA	Contiguous Memory Area support is enabled.
>  	DRM	Direct Rendering Management support is enabled.
>  	DYNAMIC_DEBUG Build in debug messages and enable them at runtime
>  	EDD	BIOS Enhanced Disk Drive Services (EDD) is enabled
> @@ -2634,6 +2635,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			Useful for devices that are detected asynchronously
>  			(e.g. USB and MMC devices).
>  
> +	rproc_mem=nn[KMG][@address]
> +			[KNL,ARM,CMA] Remoteproc physical memory block.
> +			Memory area to be used by remote processor image,
> +			managed by CMA.  Suitable defaults exist if not
> +			specified.

There are no defaults now, right?

> +
>  	rw		[KNL] Mount root device read-write on boot
>  
>  	S		[KNL] Run init in single mode
> diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
> index fb2f51b..e8016ca 100644
> --- a/arch/arm/mach-davinci/devices-da8xx.c
> +++ b/arch/arm/mach-davinci/devices-da8xx.c
> @@ -12,10 +12,11 @@
>   */
>  #include <linux/init.h>
>  #include <linux/platform_device.h>
> -#include <linux/dma-mapping.h>
> +#include <linux/dma-contiguous.h>
>  #include <linux/serial_8250.h>
>  #include <linux/ahci_platform.h>
>  #include <linux/clk.h>
> +#include <linux/platform_data/da8xx-remoteproc.h>
>  
>  #include <mach/cputype.h>
>  #include <mach/common.h>
> @@ -706,6 +707,96 @@ int __init da850_register_mmcsd1(struct davinci_mmc_config *config)
>  }
>  #endif
>  
> +static struct resource da8xx_rproc_resources[] = {
> +	{ /* DSP boot address */
> +		.start		= DA8XX_SYSCFG0_BASE + DA8XX_HOST1CFG_REG,
> +		.end		= DA8XX_SYSCFG0_BASE + DA8XX_HOST1CFG_REG + 3,
> +		.flags		= IORESOURCE_MEM,
> +	},
> +	{ /* dsp irq */
> +		.start		= IRQ_DA8XX_CHIPINT0,
> +		.end		= IRQ_DA8XX_CHIPINT0,
> +		.flags		= IORESOURCE_IRQ,
> +	},
> +};
> +
> +static struct da8xx_rproc_pdata rproc_pdata = {
> +	.name		= "dsp",
> +};

Since the driver is only for da850 so the name of the remote processor
is fixed and can probably be hardcoded in the driver itself.

> +
> +static struct platform_device da8xx_dsp = {
> +	.name	= "davinci-rproc",
> +	.id	= 0,
> +	.dev	= {
> +		.platform_data		= &rproc_pdata,
> +		.coherent_dma_mask	= DMA_BIT_MASK(32),
> +	},
> +	.num_resources	= ARRAY_SIZE(da8xx_rproc_resources),
> +	.resource	= da8xx_rproc_resources,
> +};
> +
> +#if IS_ENABLED(CONFIG_DAVINCI_REMOTEPROC)
> +
> +static phys_addr_t rproc_base __initdata;
> +static unsigned long rproc_size __initdata;
> +
> +static int __init early_rproc_mem(char *p)
> +{
> +	char *endp;
> +
> +	if (p == NULL)
> +		return 0;
> +
> +	rproc_size = memparse(p, &endp);
> +	if (*endp == '@')
> +		rproc_base = memparse(endp + 1, NULL);
> +
> +	return 0;
> +}
> +early_param("rproc_mem", early_rproc_mem);
> +
> +void __init da8xx_rproc_reserve_cma(void)
> +{
> +	int ret;
> +
> +	if (!rproc_base || !rproc_size) {
> +		pr_err("%s: 'rproc_mem=nn at address' badly specified\n"
> +		       "    'nn' and 'address' must both be non-zero\n",
> +		       __func__);
> +
> +		return;
> +	}
> +
> +	pr_info("%s: reserving 0x%lx @ 0x%lx...\n",
> +		__func__, rproc_size, (unsigned long)rproc_base);
> +
> +	ret = dma_declare_contiguous(&da8xx_dsp.dev, rproc_size, rproc_base, 0);
> +	if (ret)
> +		pr_err("%s: dma_declare_contiguous failed %d\n", __func__, ret);
> +}
> +
> +#else
> +
> +void __init da8xx_rproc_reserve_cma(void)
> +{
> +}
> +
> +#endif
> +
> +int __init da8xx_register_rproc(void)
> +{
> +	int ret;
> +
> +	ret = platform_device_register(&da8xx_dsp);
> +	if (ret) {
> +		pr_err("%s: platform_device_register: %d\n", __func__, ret);
> +
> +		return ret;
> +	}
> +
> +	return 0;
> +};
> +
>  static struct resource da8xx_rtc_resources[] = {
>  	{
>  		.start		= DA8XX_RTC_BASE,
> diff --git a/arch/arm/mach-davinci/include/mach/da8xx.h b/arch/arm/mach-davinci/include/mach/da8xx.h
> index 700d311..e9295bd 100644
> --- a/arch/arm/mach-davinci/include/mach/da8xx.h
> +++ b/arch/arm/mach-davinci/include/mach/da8xx.h
> @@ -54,6 +54,7 @@ extern unsigned int da850_max_speed;
>  #define DA8XX_SYSCFG0_BASE	(IO_PHYS + 0x14000)
>  #define DA8XX_SYSCFG0_VIRT(x)	(da8xx_syscfg0_base + (x))
>  #define DA8XX_JTAG_ID_REG	0x18
> +#define DA8XX_HOST1CFG_REG	0x44
>  #define DA8XX_CFGCHIP0_REG	0x17c
>  #define DA8XX_CFGCHIP2_REG	0x184
>  #define DA8XX_CFGCHIP3_REG	0x188
> @@ -105,6 +106,9 @@ int __init da850_register_vpif_display
>  int __init da850_register_vpif_capture
>  			(struct vpif_capture_config *capture_config);
>  void da8xx_restart(char mode, const char *cmd);
> +void __init da8xx_rproc_reserve_cma(void);

No need of __init qualifier in function prototypes.

The patch looks good to me otherwise, but I will not apply it right away
and instead wait for the driver to get accepted first.

Thanks,
Sekhar

[PATCH v5 8/9] ARM: davinci: da850 board: Added .reserve function and rproc platform registration

[Sekhar Nori]

On 1/11/2013 5:53 AM, Robert Tivy wrote:
> Added .reserve function for reserving CMA memory block to MACHINE_START.
> 
> Added call to remoteproc platform device registration function during init.
> 
> Signed-off-by: Robert Tivy <rtivy@ti.com>

Looks good to me. I will apply after the driver is accepted.

Thanks,
Sekhar

[PATCH v5 9/9] ARM: davinci: da850: Added dsp clock definition

[Sekhar Nori]

On 1/11/2013 5:53 AM, Robert Tivy wrote:
> Added dsp clock definition, keyed to "davinci-rproc.0".
> 
> Signed-off-by: Robert Tivy <rtivy@ti.com>

I think this can be done along with 4/9 so I committed it by merging it
with 4/9

Thanks,
Sekhar

[PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[Russell King - ARM Linux]

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.

[PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[Tivy, Robert]

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 at arm.linux.org.uk]
> Sent: Monday, January 21, 2013 8:41 AM
> To: Nori, Sekhar
> Cc: Tivy, Robert; ohad at wizery.com; davinci-linux-open-
> source at linux.davincidsp.com; linux-doc at vger.kernel.org; Ring, Chris;
> rob at landley.net; Grosen, Mark; linux-arm-kernel at 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.

[PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[Tivy, Robert]

> -----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

[PATCH v5 7/9] ARM: davinci: Remoteproc platform device creation data/code

[Tivy, Robert]

> -----Original Message-----
> From: Nori, Sekhar
> Sent: Monday, January 21, 2013 12:34 AM
> To: Tivy, Robert
> Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm-
> kernel at lists.infradead.org; Ring, Chris; Grosen, Mark; ohad at wizery.com;
> rob at landley.net; linux-doc at vger.kernel.org
> Subject: Re: [PATCH v5 7/9] ARM: davinci: Remoteproc platform device
> creation data/code
> 
> 
> On 1/11/2013 5:53 AM, Robert Tivy wrote:
> > Added a new remoteproc platform device for DA8XX.  Contains CMA-based
> > reservation of physical memory block.  A new kernel command-line
> > parameter has been added to allow boot-time specification of the
> > physical memory block.
> >
> > Signed-off-by: Robert Tivy <rtivy@ti.com>
> > ---
> >  Documentation/kernel-parameters.txt        |    7 +++
> >  arch/arm/mach-davinci/devices-da8xx.c      |   93
> +++++++++++++++++++++++++++-
> >  arch/arm/mach-davinci/include/mach/da8xx.h |    4 ++
> >  3 files changed, 103 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/kernel-parameters.txt
> b/Documentation/kernel-parameters.txt
> > index 363e348..e95afb1 100644
> > --- a/Documentation/kernel-parameters.txt
> > +++ b/Documentation/kernel-parameters.txt
> > @@ -44,6 +44,7 @@ parameter is applicable:
> >  	AVR32	AVR32 architecture is enabled.
> >  	AX25	Appropriate AX.25 support is enabled.
> >  	BLACKFIN Blackfin architecture is enabled.
> > +	CMA	Contiguous Memory Area support is enabled.
> >  	DRM	Direct Rendering Management support is enabled.
> >  	DYNAMIC_DEBUG Build in debug messages and enable them at runtime
> >  	EDD	BIOS Enhanced Disk Drive Services (EDD) is enabled
> > @@ -2634,6 +2635,12 @@ bytes respectively. Such letter suffixes can
> also be entirely omitted.
> >  			Useful for devices that are detected asynchronously
> >  			(e.g. USB and MMC devices).
> >
> > +	rproc_mem=nn[KMG][@address]
> > +			[KNL,ARM,CMA] Remoteproc physical memory block.
> > +			Memory area to be used by remote processor image,
> > +			managed by CMA.  Suitable defaults exist if not
> > +			specified.
> 
> There are no defaults now, right?

That's right, booter must specify rproc_mem in u-boot bootargs.

> 
> > +
> >  	rw		[KNL] Mount root device read-write on boot
> >
> >  	S		[KNL] Run init in single mode
> > diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-
> davinci/devices-da8xx.c
> > index fb2f51b..e8016ca 100644
> > --- a/arch/arm/mach-davinci/devices-da8xx.c
> > +++ b/arch/arm/mach-davinci/devices-da8xx.c
> > @@ -12,10 +12,11 @@
> >   */
> >  #include <linux/init.h>
> >  #include <linux/platform_device.h>
> > -#include <linux/dma-mapping.h>
> > +#include <linux/dma-contiguous.h>
> >  #include <linux/serial_8250.h>
> >  #include <linux/ahci_platform.h>
> >  #include <linux/clk.h>
> > +#include <linux/platform_data/da8xx-remoteproc.h>
> >
> >  #include <mach/cputype.h>
> >  #include <mach/common.h>
> > @@ -706,6 +707,96 @@ int __init da850_register_mmcsd1(struct
> davinci_mmc_config *config)
> >  }
> >  #endif
> >
> > +static struct resource da8xx_rproc_resources[] = {
> > +	{ /* DSP boot address */
> > +		.start		= DA8XX_SYSCFG0_BASE +
> DA8XX_HOST1CFG_REG,
> > +		.end		= DA8XX_SYSCFG0_BASE + DA8XX_HOST1CFG_REG + 3,
> > +		.flags		= IORESOURCE_MEM,
> > +	},
> > +	{ /* dsp irq */
> > +		.start		= IRQ_DA8XX_CHIPINT0,
> > +		.end		= IRQ_DA8XX_CHIPINT0,
> > +		.flags		= IORESOURCE_IRQ,
> > +	},
> > +};
> > +
> > +static struct da8xx_rproc_pdata rproc_pdata = {
> > +	.name		= "dsp",
> > +};
> 
> Since the driver is only for da850 so the name of the remote processor
> is fixed and can probably be hardcoded in the driver itself.

In which case I wouldn't even need the da8xx_rproc_pdata at all, which would be nice.

> 
> > +
> > +static struct platform_device da8xx_dsp = {
> > +	.name	= "davinci-rproc",
> > +	.id	= 0,
> > +	.dev	= {
> > +		.platform_data		= &rproc_pdata,
> > +		.coherent_dma_mask	= DMA_BIT_MASK(32),
> > +	},
> > +	.num_resources	= ARRAY_SIZE(da8xx_rproc_resources),
> > +	.resource	= da8xx_rproc_resources,
> > +};
> > +
> > +#if IS_ENABLED(CONFIG_DAVINCI_REMOTEPROC)
> > +
> > +static phys_addr_t rproc_base __initdata;
> > +static unsigned long rproc_size __initdata;
> > +
> > +static int __init early_rproc_mem(char *p)
> > +{
> > +	char *endp;
> > +
> > +	if (p == NULL)
> > +		return 0;
> > +
> > +	rproc_size = memparse(p, &endp);
> > +	if (*endp == '@')
> > +		rproc_base = memparse(endp + 1, NULL);
> > +
> > +	return 0;
> > +}
> > +early_param("rproc_mem", early_rproc_mem);
> > +
> > +void __init da8xx_rproc_reserve_cma(void)
> > +{
> > +	int ret;
> > +
> > +	if (!rproc_base || !rproc_size) {
> > +		pr_err("%s: 'rproc_mem=nn at address' badly specified\n"
> > +		       "    'nn' and 'address' must both be non-zero\n",
> > +		       __func__);
> > +
> > +		return;
> > +	}
> > +
> > +	pr_info("%s: reserving 0x%lx @ 0x%lx...\n",
> > +		__func__, rproc_size, (unsigned long)rproc_base);
> > +
> > +	ret = dma_declare_contiguous(&da8xx_dsp.dev, rproc_size,
> rproc_base, 0);
> > +	if (ret)
> > +		pr_err("%s: dma_declare_contiguous failed %d\n", __func__,
> ret);
> > +}
> > +
> > +#else
> > +
> > +void __init da8xx_rproc_reserve_cma(void)
> > +{
> > +}
> > +
> > +#endif
> > +
> > +int __init da8xx_register_rproc(void)
> > +{
> > +	int ret;
> > +
> > +	ret = platform_device_register(&da8xx_dsp);
> > +	if (ret) {
> > +		pr_err("%s: platform_device_register: %d\n", __func__,
> ret);
> > +
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +};
> > +
> >  static struct resource da8xx_rtc_resources[] = {
> >  	{
> >  		.start		= DA8XX_RTC_BASE,
> > diff --git a/arch/arm/mach-davinci/include/mach/da8xx.h
> b/arch/arm/mach-davinci/include/mach/da8xx.h
> > index 700d311..e9295bd 100644
> > --- a/arch/arm/mach-davinci/include/mach/da8xx.h
> > +++ b/arch/arm/mach-davinci/include/mach/da8xx.h
> > @@ -54,6 +54,7 @@ extern unsigned int da850_max_speed;
> >  #define DA8XX_SYSCFG0_BASE	(IO_PHYS + 0x14000)
> >  #define DA8XX_SYSCFG0_VIRT(x)	(da8xx_syscfg0_base + (x))
> >  #define DA8XX_JTAG_ID_REG	0x18
> > +#define DA8XX_HOST1CFG_REG	0x44
> >  #define DA8XX_CFGCHIP0_REG	0x17c
> >  #define DA8XX_CFGCHIP2_REG	0x184
> >  #define DA8XX_CFGCHIP3_REG	0x188
> > @@ -105,6 +106,9 @@ int __init da850_register_vpif_display
> >  int __init da850_register_vpif_capture
> >  			(struct vpif_capture_config *capture_config);
> >  void da8xx_restart(char mode, const char *cmd);
> > +void __init da8xx_rproc_reserve_cma(void);
> 
> No need of __init qualifier in function prototypes.
> 
> The patch looks good to me otherwise, but I will not apply it right
> away
> and instead wait for the driver to get accepted first.

Sounds like a plan.

Thanks & Regards,

- Rob

> 
> Thanks,
> Sekhar

[PATCH v5 9/9] ARM: davinci: da850: Added dsp clock definition

[Sekhar Nori]

Rob,

On 1/11/2013 5:53 AM, Robert Tivy wrote:
> Added dsp clock definition, keyed to "davinci-rproc.0".
> 
> Signed-off-by: Robert Tivy <rtivy@ti.com>
> ---
>  arch/arm/mach-davinci/da850.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
> index 097fcb2..50107c5 100644
> --- a/arch/arm/mach-davinci/da850.c
> +++ b/arch/arm/mach-davinci/da850.c
> @@ -375,6 +375,14 @@ static struct clk sata_clk = {
>  	.flags		= PSC_FORCE,
>  };
>  
> +static struct clk dsp_clk = {
> +	.name		= "dsp",
> +	.parent		= &pll0_sysclk1,
> +	.domain		= DAVINCI_GPSC_DSPDOMAIN,
> +	.lpsc		= DA8XX_LPSC0_GEM,
> +	.flags		= PSC_LRST,
> +};
> +
>  static struct clk_lookup da850_clks[] = {
>  	CLK(NULL,		"ref",		&ref_clk),
>  	CLK(NULL,		"pll0",		&pll0_clk),
> @@ -421,6 +429,7 @@ static struct clk_lookup da850_clks[] = {
>  	CLK("spi_davinci.1",	NULL,		&spi1_clk),
>  	CLK("vpif",		NULL,		&vpif_clk),
>  	CLK("ahci",		NULL,		&sata_clk),
> +	CLK("davinci-rproc.0",	NULL,		&dsp_clk),

Adding this clock node without the having the driver probed leads to
kernel hang while booting. With CONFIG_DAVINCI_RESET_CLOCKS=n, the
kernel boot fine. It looks like there is some trouble disabling the
clock if it is not used. Can you please check this issue?

Thanks,
Sekhar

[PATCH v5 9/9] ARM: davinci: da850: Added dsp clock definition

[Tivy, Robert]

> -----Original Message-----
> From: Nori, Sekhar
> Sent: Tuesday, January 22, 2013 4:04 AM
> 
> Rob,
> 
> On 1/11/2013 5:53 AM, Robert Tivy wrote:
> > Added dsp clock definition, keyed to "davinci-rproc.0".
> >
> > Signed-off-by: Robert Tivy <rtivy@ti.com>
> > ---
> >  arch/arm/mach-davinci/da850.c |    9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-
> davinci/da850.c
> > index 097fcb2..50107c5 100644
> > --- a/arch/arm/mach-davinci/da850.c
> > +++ b/arch/arm/mach-davinci/da850.c
> > @@ -375,6 +375,14 @@ static struct clk sata_clk = {
> >  	.flags		= PSC_FORCE,
> >  };
> >
> > +static struct clk dsp_clk = {
> > +	.name		= "dsp",
> > +	.parent		= &pll0_sysclk1,
> > +	.domain		= DAVINCI_GPSC_DSPDOMAIN,
> > +	.lpsc		= DA8XX_LPSC0_GEM,
> > +	.flags		= PSC_LRST,
> > +};
> > +
> >  static struct clk_lookup da850_clks[] = {
> >  	CLK(NULL,		"ref",		&ref_clk),
> >  	CLK(NULL,		"pll0",		&pll0_clk),
> > @@ -421,6 +429,7 @@ static struct clk_lookup da850_clks[] = {
> >  	CLK("spi_davinci.1",	NULL,		&spi1_clk),
> >  	CLK("vpif",		NULL,		&vpif_clk),
> >  	CLK("ahci",		NULL,		&sata_clk),
> > +	CLK("davinci-rproc.0",	NULL,		&dsp_clk),
> 
> Adding this clock node without the having the driver probed leads to
> kernel hang while booting. With CONFIG_DAVINCI_RESET_CLOCKS=n, the
> kernel boot fine. It looks like there is some trouble disabling the
> clock if it is not used. Can you please check this issue?
> 
> Thanks,
> Sekhar

Sekhar,

Thankyou for trying that out.

I discovered that the kernel boot hang is caused when trying to disable this clk during init, from within the function davinci_clk_disable_unused().  That function calls davinci_psc_config() which attempts to disable the DSP clock.  Since this clk is enabled after reset, disabling it involves a state transition, and therefore the function must wait for GOSTAT[1] to be 0.  For most peripherals this happens automatically, but for the DSP (and ARM, too), the DSP must execute the IDLE instruction to allow a clock enable->disable transition.  Since this is bootup and there is no real program on the DSP yet, this doesn't happen.

I was able to overcome this by adding PSC_FORCE to the clk->flags for dsp_clk.  In fact, without PSC_FORCE I was not able to "rmmod davinci_remoteproc" since the firmware file that I'm loading did not execute IDLE.  It feels like for davinci we should have a way to control the clk->flags' PSC_FORCE setting at run-time.  The PSC_FORCE would be set initially (during boot) and the user (or system integrator) would have a way to unset it dynamically.  That way, when the DSP firmware does actually have a structured way to enter IDLE, the user can say "I'd like a structured shutdown" and unset PSC_FORCE (via a clean API).  But for now I'm just going to turn on PSC_FORCE:
	.flags		= PSC_LRST | PSC_FORCE,

Thanks & Regards,

- Rob

[PATCH v5 9/9] ARM: davinci: da850: Added dsp clock definition

[Sekhar Nori]

On 1/23/2013 7:07 AM, Tivy, Robert wrote:
>> -----Original Message-----
>> From: Nori, Sekhar
>> Sent: Tuesday, January 22, 2013 4:04 AM
>>
>> Rob,
>>
>> On 1/11/2013 5:53 AM, Robert Tivy wrote:
>>> Added dsp clock definition, keyed to "davinci-rproc.0".
>>>
>>> Signed-off-by: Robert Tivy <rtivy@ti.com>
>>> ---
>>>  arch/arm/mach-davinci/da850.c |    9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-
>> davinci/da850.c
>>> index 097fcb2..50107c5 100644
>>> --- a/arch/arm/mach-davinci/da850.c
>>> +++ b/arch/arm/mach-davinci/da850.c
>>> @@ -375,6 +375,14 @@ static struct clk sata_clk = {
>>>  	.flags		= PSC_FORCE,
>>>  };
>>>
>>> +static struct clk dsp_clk = {
>>> +	.name		= "dsp",
>>> +	.parent		= &pll0_sysclk1,
>>> +	.domain		= DAVINCI_GPSC_DSPDOMAIN,
>>> +	.lpsc		= DA8XX_LPSC0_GEM,
>>> +	.flags		= PSC_LRST,
>>> +};
>>> +
>>>  static struct clk_lookup da850_clks[] = {
>>>  	CLK(NULL,		"ref",		&ref_clk),
>>>  	CLK(NULL,		"pll0",		&pll0_clk),
>>> @@ -421,6 +429,7 @@ static struct clk_lookup da850_clks[] = {
>>>  	CLK("spi_davinci.1",	NULL,		&spi1_clk),
>>>  	CLK("vpif",		NULL,		&vpif_clk),
>>>  	CLK("ahci",		NULL,		&sata_clk),
>>> +	CLK("davinci-rproc.0",	NULL,		&dsp_clk),
>>
>> Adding this clock node without the having the driver probed leads to
>> kernel hang while booting. With CONFIG_DAVINCI_RESET_CLOCKS=n, the
>> kernel boot fine. It looks like there is some trouble disabling the
>> clock if it is not used. Can you please check this issue?
>>
>> Thanks,
>> Sekhar
> 
> Sekhar,
> 
> Thankyou for trying that out.
> 
> I discovered that the kernel boot hang is caused when trying to disable this clk during init, from within the function davinci_clk_disable_unused().  That function calls davinci_psc_config() which attempts to disable the DSP clock.  Since this clk is enabled after reset, disabling it involves a state transition, and therefore the function must wait for GOSTAT[1] to be 0.  For most peripherals this happens automatically, but for the DSP (and ARM, too), the DSP must execute the IDLE instruction to allow a clock enable->disable transition.  Since this is bootup and there is no real program on the DSP yet, this doesn't happen.
> 
> I was able to overcome this by adding PSC_FORCE to the clk->flags for dsp_clk.  In fact, without PSC_FORCE I was not able to "rmmod davinci_remoteproc" since the firmware file that I'm loading did not execute IDLE.  It feels like for davinci we should have a way to control the clk->flags' PSC_FORCE setting at run-time.  The PSC_FORCE would be set initially (during boot) and the user (or system integrator) would have a way to unset it dynamically.  That way, when the DSP firmware does actually have a structured way to enter IDLE, the user can say "I'd like a structured shutdown" and unset PSC_FORCE (via a clean API).  But for now I'm just going to turn on PSC_FORCE:
> 	.flags		= PSC_LRST | PSC_FORCE,

Thanks for the debug. It works for me after I added PSC_FORCE. Here is 
the patch I am finally committing.

Thanks,
Sekhar

commit 09810a853b9a7920ba8c250d18815ef236effc47
Author:     Robert Tivy <rtivy@ti.com>
AuthorDate: Thu Jan 10 16:23:22 2013 -0800
Commit:     Sekhar Nori <nsekhar@ti.com>
CommitDate: Thu Jan 24 10:54:08 2013 +0530

    ARM: davinci: da850: add dsp clock definition
    
    Added dsp clock definition, keyed to "davinci-rproc.0".
    DSP clocks is derived from pll0 sysclk1. Add a clock tree
    node for that too.
    
    Signed-off-by: Robert Tivy <rtivy@ti.com>
    [nsekhar at ti.com: merge addition of pll0 sysclk1 and dsp clock
    into one commit. Add PSC_FORCE to dsp clock node to handle the
    case where DSP does not go into IDLE and its clock needs to
    be disabled.]
    Signed-off-by: Sekhar Nori <nsekhar@ti.com>

diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
index 6b9154e..0c4a26d 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -76,6 +76,13 @@ static struct clk pll0_aux_clk = {
 	.flags		= CLK_PLL | PRE_PLL,
 };
 
+static struct clk pll0_sysclk1 = {
+	.name		= "pll0_sysclk1",
+	.parent		= &pll0_clk,
+	.flags		= CLK_PLL,
+	.div_reg	= PLLDIV1,
+};
+
 static struct clk pll0_sysclk2 = {
 	.name		= "pll0_sysclk2",
 	.parent		= &pll0_clk,
@@ -368,10 +375,19 @@ static struct clk sata_clk = {
 	.flags		= PSC_FORCE,
 };
 
+static struct clk dsp_clk = {
+	.name		= "dsp",
+	.parent		= &pll0_sysclk1,
+	.domain		= DAVINCI_GPSC_DSPDOMAIN,
+	.lpsc		= DA8XX_LPSC0_GEM,
+	.flags		= PSC_LRST | PSC_FORCE,
+};
+
 static struct clk_lookup da850_clks[] = {
 	CLK(NULL,		"ref",		&ref_clk),
 	CLK(NULL,		"pll0",		&pll0_clk),
 	CLK(NULL,		"pll0_aux",	&pll0_aux_clk),
+	CLK(NULL,		"pll0_sysclk1",	&pll0_sysclk1),
 	CLK(NULL,		"pll0_sysclk2",	&pll0_sysclk2),
 	CLK(NULL,		"pll0_sysclk3",	&pll0_sysclk3),
 	CLK(NULL,		"pll0_sysclk4",	&pll0_sysclk4),
@@ -413,6 +429,7 @@ static struct clk_lookup da850_clks[] = {
 	CLK("spi_davinci.1",	NULL,		&spi1_clk),
 	CLK("vpif",		NULL,		&vpif_clk),
 	CLK("ahci",		NULL,		&sata_clk),
+	CLK("davinci-rproc.0",	NULL,		&dsp_clk),
 	CLK(NULL,		NULL,		NULL),
 };