[PATCH v7 1/2] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[Ohad Ben-Cohen]

Hi Rob,

On Fri, Feb 1, 2013 at 4:24 AM, Robert Tivy <rtivy at ti.com> wrote:
>  drivers/remoteproc/Kconfig            |   29 ++-
>  drivers/remoteproc/Makefile           |    1 +
>  drivers/remoteproc/da8xx_remoteproc.c |  346 +++++++++++++++++++++++++++++++++
>  drivers/remoteproc/remoteproc_core.c  |   22 ++-

It looks like this patch squashes:
1. A fix to drivers/remoteproc/Kconfig
2. New functionality in remoteproc_core.c
3. A new da8xx driver

These are independent changes, so we better split them up to separate patches.

>  config OMAP_REMOTEPROC
>         tristate "OMAP remoteproc support"
> -       depends on EXPERIMENTAL
...
>  config STE_MODEM_RPROC
>         tristate "STE-Modem remoteproc support"
> -       depends on EXPERIMENTAL

These look unrelated to this patch, and it seems that Kees Cook
submitted these awhile ago so it should probably already be in
linux-next (haven't looked). I think we can drop these.

> +/**
> + * handle_event() - inbound virtqueue message workqueue function
> + *
> + * This function is registered as a kernel thread and is scheduled by the
> + * kernel handler.
> + */
> +static irqreturn_t handle_event(int irq, void *p)
> +{
> +       struct rproc *rproc = (struct rproc *)p;
> +
> +       /* Process incoming buffers on our vring */
> +       while (IRQ_HANDLED == rproc_vq_interrupt(rproc, 0))
> +               ;
> +
> +       /* Must allow wakeup of potenitally blocking senders */
> +       rproc_vq_interrupt(rproc, 1);

IIRC we agreed on removing this part, and instead adding it to the
generic remoteproc framework (i.e. Cyril's patch).

> +static int da8xx_rproc_start(struct rproc *rproc)
> +{
...
> +       dsp_clk = devm_clk_get(dev, NULL);
> +       if (IS_ERR(dsp_clk)) {
> +               dev_err(dev, "clk_get error: %ld\n", PTR_ERR(dsp_clk));
> +
> +               return PTR_RET(dsp_clk);
> +       }
> +       drproc->dsp_clk = dsp_clk;

Have you considered doing this in ->probe() instead? Do we need to
call get/put every time we start/stop the remote processor?

> +/* kick a virtqueue */
> +static void da8xx_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +       struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv;
> +       int timed_out;
> +       unsigned long timeout;
> +
> +       timed_out = 0;
> +       timeout = jiffies + HZ/100;
> +
> +       /* Poll for ack from other side first */
> +       while (readl(drproc->chipsig) & SYSCFG_CHIPSIG2)
> +               if (time_after(jiffies, timeout)) {
> +                       if (readl(drproc->chipsig) & SYSCFG_CHIPSIG2) {
> +                               dev_err(rproc->dev.parent,
> +                                       "failed to receive ack\n");
> +                               timed_out = 1;
> +                       }
> +
> +                       break;
> +               }

This still looks a bit out of place here.

Kicking should be a fast unilateral action, that doesn't depend on the
other side.

> +static int da8xx_rproc_probe(struct platform_device *pdev)
> +{
...
> +       ret = rproc_add(rproc);
> +       if (ret) {
> +               dev_err(dev, "rproc_add failed: %d\n", ret);
> +               goto free_rproc;
> +       }
> +
> +       drproc->chipsig = chipsig;
> +       drproc->bootreg = bootreg;
> +       drproc->ack_fxn = irq_data->chip->irq_ack;
> +       drproc->irq_data = irq_data;
> +       drproc->irq = irq;
> +
> +       /* everything the ISR needs is now setup, so hook it up */
> +       ret = devm_request_threaded_irq(dev, irq, da8xx_rproc_callback,
> +               handle_event, 0, "da8xx-remoteproc", rproc);
> +       if (ret) {
> +               dev_err(dev, "devm_request_threaded_irq error: %d\n", ret);
> +               rproc_del(rproc);
> +               goto free_rproc;
> +       }

Shouldn't we be requesting this before we rproc_add() ?

> +static int da8xx_rproc_remove(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct rproc *rproc = platform_get_drvdata(pdev);
> +       struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv;
> +       int ret;
...
> +       ret = rproc_del(rproc);

You can safely remove 'ret' altogether. We will just crash in
rproc_put if rproc is NULL.

> +       rproc_put(rproc);
> +
> +       return ret;
> +}

> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index dd3bfaf..e0f703e 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1222,19 +1222,39 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>                                 const char *firmware, int len)
>  {
>         struct rproc *rproc;
> +       char *template = "rproc-%s-fw";
> +       char *p;
>
>         if (!dev || !name || !ops)
>                 return NULL;
>
> +       if (!firmware)
> +               /*
> +                * Make room for default firmware name (minus %s plus '\0').
> +                * If the caller didn't pass in a firmware name then
> +                * construct a default name.  We're already glomming 'len'
> +                * bytes onto the end of the struct rproc allocation, so do
> +                * a few more for the default firmware name (but only if
> +                * the caller doesn't pass one).
> +                */
> +               len += strlen(name) + strlen(template) - 2 + 1;
> +
>         rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
>         if (!rproc) {
>                 dev_err(dev, "%s: kzalloc failed\n", __func__);
>                 return NULL;
>         }
>
> +       if (!firmware) {
> +               p = (char *)rproc + sizeof(struct rproc) + len;
> +               sprintf(p, template, name);

Looks like p you're writing to is outside of the rproc memory allocation.

Thanks,
Ohad.