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

[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@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.

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

[Tivy, Robert]

Hi Ohad,

> -----Original Message-----
> From: Ohad Ben-Cohen [mailto:ohad at wizery.com]
> Sent: Friday, February 15, 2013 12:07 AM
> 
> Hi Rob,
> 
> On Fri, Feb 1, 2013 at 4:24 AM, Robert Tivy <rtivy@ti.com> wrote:
> >  drivers/remoteproc/Kconfig            |   29 ++-
> >  drivers/remoteproc/Makefile           |    1 +
> >  drivers/remoteproc/da8xx_remoteproc.c |  346
> +++++++++++++++++++++++++++++++++
> >  drivers/remoteproc/remoteproc_core.c  |   22 ++-
> 
> It looks like this patch squashes:
> 1. A fix to drivers/remoteproc/Kconfig
> 2. New functionality in remoteproc_core.c
> 3. A new da8xx driver
> 
> These are independent changes, so we better split them up to separate
> patches.

Ok, I will submit separate patches for these.  In doing so, I will have a stand-alone patch for the drivers/remoteproc/Kconfig fix, as well as the driver-related addition to that file in the driver patch.  PCMIIW.

> 
> >  config OMAP_REMOTEPROC
> >         tristate "OMAP remoteproc support"
> > -       depends on EXPERIMENTAL
> ...
> >  config STE_MODEM_RPROC
> >         tristate "STE-Modem remoteproc support"
> > -       depends on EXPERIMENTAL
> 
> These look unrelated to this patch, and it seems that Kees Cook
> submitted these awhile ago so it should probably already be in
> linux-next (haven't looked). I think we can drop these.

OK, will drop.

> 
> > +/**
> > + * handle_event() - inbound virtqueue message workqueue function
> > + *
> > + * This function is registered as a kernel thread and is scheduled
> by the
> > + * kernel handler.
> > + */
> > +static irqreturn_t handle_event(int irq, void *p)
> > +{
> > +       struct rproc *rproc = (struct rproc *)p;
> > +
> > +       /* Process incoming buffers on our vring */
> > +       while (IRQ_HANDLED == rproc_vq_interrupt(rproc, 0))
> > +               ;
> > +
> > +       /* Must allow wakeup of potenitally blocking senders */
> > +       rproc_vq_interrupt(rproc, 1);
> 
> IIRC we agreed on removing this part, and instead adding it to the
> generic remoteproc framework (i.e. Cyril's patch).

I didn't like the idea of having extra overhead when calling the "all virtqueues" version.

We know that we want to call rproc_vq_interrupt(rproc, 1) just once, and don't care about its return value.  If we did
      /* Process incoming buffers on our vring */
      while (IRQ_HANDLED == rproc_vq_interrupt(rproc, -1))
              ;
then that would essentially turn into:
	do {
		ret = IRQ_NONE;
		if (rproc_vq_interrupt(rproc, 0) == IRQ_HANDLED)
			ret = IRQ_HANDLED;
		if (rproc_vq_interrupt(rproc, 1) == IRQ_HANDLED)
			ret = IRQ_HANDLED;
	} while (ret == IRQ_HANDLED);
which will end up calling rproc_vq_interrupt(rproc, 1) too many times.

We have a benchmark goal to keep wrt/ the whole round-trip time of messages and we're currently not even achieving that, so more overhead than today can't be tolerated.

> 
> > +static int da8xx_rproc_start(struct rproc *rproc)
> > +{
> ...
> > +       dsp_clk = devm_clk_get(dev, NULL);
> > +       if (IS_ERR(dsp_clk)) {
> > +               dev_err(dev, "clk_get error: %ld\n",
> PTR_ERR(dsp_clk));
> > +
> > +               return PTR_RET(dsp_clk);
> > +       }
> > +       drproc->dsp_clk = dsp_clk;
> 
> Have you considered doing this in ->probe() instead? Do we need to
> call get/put every time we start/stop the remote processor?

I suppose we don't need to call it every time, I will try moving it to ->probe()

> 
> > +/* kick a virtqueue */
> > +static void da8xx_rproc_kick(struct rproc *rproc, int vqid)
> > +{
> > +       struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc-
> >priv;
> > +       int timed_out;
> > +       unsigned long timeout;
> > +
> > +       timed_out = 0;
> > +       timeout = jiffies + HZ/100;
> > +
> > +       /* Poll for ack from other side first */
> > +       while (readl(drproc->chipsig) & SYSCFG_CHIPSIG2)
> > +               if (time_after(jiffies, timeout)) {
> > +                       if (readl(drproc->chipsig) & SYSCFG_CHIPSIG2)
> {
> > +                               dev_err(rproc->dev.parent,
> > +                                       "failed to receive ack\n");
> > +                               timed_out = 1;
> > +                       }
> > +
> > +                       break;
> > +               }
> 
> This still looks a bit out of place here.
> 
> Kicking should be a fast unilateral action, that doesn't depend on the
> other side.

Ok, I'll drop this complexity.

While that code sorta looks like too much, in a smoothly working system it's just a register read and single "false" if-test, since SYSCFG_CHIPSIG2 will actually never be set there.  If SYSCFG_CHIPSIG2 *is* set then it will likely not unset and the timed_out situation will happen.

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

Yeah, that seems to be the more prudent ordering, will switch.
 
> 
> > +static int da8xx_rproc_remove(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct rproc *rproc = platform_get_drvdata(pdev);
> > +       struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc-
> >priv;
> > +       int ret;
> ...
> > +       ret = rproc_del(rproc);
> 
> You can safely remove 'ret' altogether. We will just crash in
> rproc_put if rproc is NULL.

Will do.

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

Yikes, must have been getting lucky when testing with the default name!

Thanks for the notice, will fix.

Regards,

- Rob

> 
> Thanks,
> Ohad.

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

[Ohad Ben-Cohen]

Hi Rob,

On Tue, Feb 19, 2013 at 1:02 AM, Tivy, Robert <rtivy@ti.com> wrote:
>> It looks like this patch squashes:
>> 1. A fix to drivers/remoteproc/Kconfig
>> 2. New functionality in remoteproc_core.c
>> 3. A new da8xx driver
>>
>> These are independent changes, so we better split them up to separate
>> patches.
>
> Ok, I will submit separate patches for these.  In doing so, I will have a stand-alone patch for the drivers/remoteproc/Kconfig fix, as well as the driver-related addition to that file in the driver patch.  PCMIIW.

We should have one patch for the Kconfig fix, another one for the new
firmware-name functionality in rproc_alloc() and then a patch adding
this new driver.

>> > +static irqreturn_t handle_event(int irq, void *p)
>> > +{
>> > +       struct rproc *rproc = (struct rproc *)p;
>> > +
>> > +       /* Process incoming buffers on our vring */
>> > +       while (IRQ_HANDLED == rproc_vq_interrupt(rproc, 0))
>> > +               ;
>> > +
>> > +       /* Must allow wakeup of potenitally blocking senders */
>> > +       rproc_vq_interrupt(rproc, 1);
>>
>> IIRC we agreed on removing this part, and instead adding it to the
>> generic remoteproc framework (i.e. Cyril's patch).
>
> I didn't like the idea of having extra overhead when calling the "all virtqueues" version.
>
> We know that we want to call rproc_vq_interrupt(rproc, 1) just once, and don't care about its return value.  If we did
>       /* Process incoming buffers on our vring */
>       while (IRQ_HANDLED == rproc_vq_interrupt(rproc, -1))
>               ;
> then that would essentially turn into:
>         do {
>                 ret = IRQ_NONE;
>                 if (rproc_vq_interrupt(rproc, 0) == IRQ_HANDLED)
>                         ret = IRQ_HANDLED;
>                 if (rproc_vq_interrupt(rproc, 1) == IRQ_HANDLED)
>                         ret = IRQ_HANDLED;
>         } while (ret == IRQ_HANDLED);
> which will end up calling rproc_vq_interrupt(rproc, 1) too many times.
>
> We have a benchmark goal to keep wrt/ the whole round-trip time of messages and we're currently not even achieving that, so more overhead than today can't be tolerated.

Have you checked whether it really affects performance? I'd expect
this to be very negligible. We can probably also maintain a bitmap
with bits set for every IRQ_HANDLED vring, and stop processing vrings
that has this bit cleared.

But it actually looks like there's a different problem here.
rproc_vq_interrupt() may also return IRQ_HANDLED on the unlikely event
that vq->broken is set. With the current approach taken by this patch,
this may lock the system in a busy loop.

I took a brief look at the other virtio drivers, and it seems they
process all pending messages available when kicked, so we should
probably add this to rpmsg_recv_done() too. This way we don't have to
manually do it in the remoteproc layer, and all platforms will benefit
from it.

We should then only have to allow the option of asking the core to
process all available vrings, as we discussed, and we're done.

Thanks,
Ohad.

[PATCH v7 0/2] ARM: davinci: remoteproc support

[Sekhar Nori]

On 2/1/2013 7:54 AM, Robert Tivy wrote:
> This patch series adds remoteproc support for OMAP-L138, along with needed
> supporting mach-davinci infrastructure.
> 
> These patches are based on Sekhar's commit id
> 601ac613155476701ff2155aa11853cad70d7ce5
> at git://gitorious.org/linux-davinci/linux-davinci.git
> 
> Some notes for reviewers...
> 
> DOCUMENTATION maintainers: patch 2/2 in this series contains a change to
> kernel-parameters.txt that adds a description for a new kernel command-line
> parameter, along with the code that defines the new kernel command-line
> parameter.  You are, of course, free to look at the whole series, but only
> patch 2/2 is of particular interest.
> 
> This is actually a subset of the complete patch series, since the other
> patches in this set have already been committed to a staging area by the
> tree maintainer.
> 
> Sekhar, once this is accepted, please remember to apply patch [PATCH v5 8/9].

Please add this patch to this series, next time you spin it.

Thanks,
Sekhar

[PATCH v7 2/2] ARM: davinci: Remoteproc platform device creation data/code

[Sekhar Nori]

Hi Rob,

On 2/1/2013 7:54 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>

This looks good to me. Except one minor nit below:

> +static struct platform_device da8xx_dsp = {
> +	.name	= "davinci-rproc",
> +	.id	= 0,

No need to zero initialize here.

> +	.dev	= {
> +		.coherent_dma_mask	= DMA_BIT_MASK(32),
> +	},
> +	.num_resources	= ARRAY_SIZE(da8xx_rproc_resources),
> +	.resource	= da8xx_rproc_resources,
> +};

You can fix that and add my:

Acked-by: Sekhar Nori <nsekhar@ti.com>

in case this goes from outside of DaVinci tree.

Thanks,
Sekhar