[PATCH v9 2/6] remoteproc: Change typo FW_CONFIG to FW_LOADER for REMOTEPROC

[PATCH v9 2/6] remoteproc: Change typo FW_CONFIG to FW_LOADER for REMOTEPROC

[Ohad Ben-Cohen]

On Fri, Mar 29, 2013 at 4:41 AM, Robert Tivy <rtivy@ti.com> wrote:
> Fix obvious typo introduced in commit e121aefa7d9f10eee5cf26ed47129237a05d940b
> ("remoteproc: fix missing CONFIG_FW_LOADER configurations").
>
> Signed-off-by: Robert Tivy <rtivy@ti.com>

Applied, thanks.

BTW - we don't usually want to directly cc stable at vger.kernel.org.
Instead we just add a Cc tag in the commit log (fixed before
applying).

[PATCH v9 3/6] remoteproc: Add support to rproc_alloc() for a default firmware name

[Ohad Ben-Cohen]

On Fri, Mar 29, 2013 at 4:41 AM, Robert Tivy <rtivy@ti.com> wrote:
> Signed-off-by: Robert Tivy <rtivy@ti.com>

Applied with the following changes:
- add commit log
- document change in function description
- use snprintf instead
- minor style change

Thanks!

[PATCH v9 1/6] rpmsg: Process all available messages in virtqueue callback

[Ohad Ben-Cohen]

On Fri, Mar 29, 2013 at 4:41 AM, Robert Tivy <rtivy@ti.com> wrote:
> Change virtqueue callback function rpmsg_recv_done() to process all
> available messages instead of just one message.
>
> Signed-off-by: Robert Tivy <rtivy@ti.com>

I'm thinking instead of adding an indentation level, let's split the
_recv function.

This is what I have in mind - let me know if it works for you - thanks:

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index a59684b..42b9872 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -776,22 +776,11 @@ out:
 }
 EXPORT_SYMBOL(rpmsg_send_offchannel_raw);

-/* called when an rx buffer is used, and it's time to digest a message */
-static void rpmsg_recv_done(struct virtqueue *rvq)
+static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
+                            struct rpmsg_hdr *msg, unsigned int len)
 {
-       struct rpmsg_hdr *msg;
-       unsigned int len;
        struct rpmsg_endpoint *ept;
        struct scatterlist sg;
-       struct virtproc_info *vrp = rvq->vdev->priv;
-       struct device *dev = &rvq->vdev->dev;
-       int err;
-
-       msg = virtqueue_get_buf(rvq, &len);
-       if (!msg) {
-               dev_err(dev, "uhm, incoming signal, but no used buffer ?\n");
-               return;
-       }

        dev_dbg(dev, "From: 0x%x, To: 0x%x, Len: %d, Flags: %d, Reserved: %d\n",
                                        msg->src, msg->dst, msg->len,
@@ -806,7 +795,7 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
        if (len > RPMSG_BUF_SIZE ||
                msg->len > (len - sizeof(struct rpmsg_hdr))) {
                dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg->len);
-               return;
+               return -EINVAL;
        }

        /* use the dst addr to fetch the callback of the appropriate user */
@@ -842,9 +831,39 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
        err = virtqueue_add_buf(vrp->rvq, &sg, 0, 1, msg, GFP_KERNEL);
        if (err < 0) {
                dev_err(dev, "failed to add a virtqueue buffer: %d\n", err);
+               return err;
+       }
+
+       return 0;
+}
+
+/* called when an rx buffer is used, and it's time to digest a message */
+static void rpmsg_recv_done(struct virtqueue *rvq)
+{
+       struct virtproc_info *vrp = rvq->vdev->priv;
+       struct device *dev = &rvq->vdev->dev;
+       struct rpmsg_hdr *msg;
+       unsigned int len, msgs_received = 0;
+       int err;
+
+       msg = virtqueue_get_buf(rvq, &len);
+       if (!msg) {
+               dev_err(dev, "uhm, incoming signal, but no used buffer ?\n");
                return;
        }

+       while (msg) {
+               err = rpmsg_recv_single(vrp, dev, msg, len);
+               if (err)
+                       break;
+
+               msgs_received++;
+
+               msg = virtqueue_get_buf(rvq, &len);
+       };
+
+       dev_dbg(dev, "Received %u messages\n", msgs_received);
+
        /* tell the remote processor we added another available rx buffer */
        virtqueue_kick(vrp->rvq);
 }

[PATCH v9 1/6] rpmsg: Process all available messages in virtqueue callback

[Tivy, Robert]

> -----Original Message-----
> From: Ohad Ben-Cohen [mailto:ohad at wizery.com]
> Sent: Sunday, April 07, 2013 5:51 AM
> 
> On Fri, Mar 29, 2013 at 4:41 AM, Robert Tivy <rtivy@ti.com> wrote:
> > Change virtqueue callback function rpmsg_recv_done() to process all
> > available messages instead of just one message.
> >
> > Signed-off-by: Robert Tivy <rtivy@ti.com>
> 
> I'm thinking instead of adding an indentation level, let's split the
> _recv function.
> 
> This is what I have in mind - let me know if it works for you - thanks:
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
> b/drivers/rpmsg/virtio_rpmsg_bus.c
> index a59684b..42b9872 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -776,22 +776,11 @@ out:
>  }
>  EXPORT_SYMBOL(rpmsg_send_offchannel_raw);
> 
> -/* called when an rx buffer is used, and it's time to digest a message
> */
> -static void rpmsg_recv_done(struct virtqueue *rvq)
> +static int rpmsg_recv_single(struct virtproc_info *vrp, struct device
> *dev,
> +                            struct rpmsg_hdr *msg, unsigned int len)
>  {
> -       struct rpmsg_hdr *msg;
> -       unsigned int len;
>         struct rpmsg_endpoint *ept;
>         struct scatterlist sg;
> -       struct virtproc_info *vrp = rvq->vdev->priv;
> -       struct device *dev = &rvq->vdev->dev;
> -       int err;
> -
> -       msg = virtqueue_get_buf(rvq, &len);
> -       if (!msg) {
> -               dev_err(dev, "uhm, incoming signal, but no used buffer
> ?\n");
> -               return;
> -       }
> 
>         dev_dbg(dev, "From: 0x%x, To: 0x%x, Len: %d, Flags: %d,
> Reserved: %d\n",
>                                         msg->src, msg->dst, msg->len,
> @@ -806,7 +795,7 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
>         if (len > RPMSG_BUF_SIZE ||
>                 msg->len > (len - sizeof(struct rpmsg_hdr))) {
>                 dev_warn(dev, "inbound msg too big: (%d, %d)\n", len,
> msg->len);
> -               return;
> +               return -EINVAL;
>         }
> 
>         /* use the dst addr to fetch the callback of the appropriate
> user */
> @@ -842,9 +831,39 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
>         err = virtqueue_add_buf(vrp->rvq, &sg, 0, 1, msg, GFP_KERNEL);
>         if (err < 0) {
>                 dev_err(dev, "failed to add a virtqueue buffer: %d\n",
> err);
> +               return err;
> +       }
> +
> +       return 0;
> +}
> +
> +/* called when an rx buffer is used, and it's time to digest a message
> */
> +static void rpmsg_recv_done(struct virtqueue *rvq)
> +{
> +       struct virtproc_info *vrp = rvq->vdev->priv;
> +       struct device *dev = &rvq->vdev->dev;
> +       struct rpmsg_hdr *msg;
> +       unsigned int len, msgs_received = 0;
> +       int err;
> +
> +       msg = virtqueue_get_buf(rvq, &len);
> +       if (!msg) {
> +               dev_err(dev, "uhm, incoming signal, but no used buffer
> ?\n");
>                 return;
>         }
> 
> +       while (msg) {
> +               err = rpmsg_recv_single(vrp, dev, msg, len);
> +               if (err)
> +                       break;
> +
> +               msgs_received++;
> +
> +               msg = virtqueue_get_buf(rvq, &len);
> +       };
> +
> +       dev_dbg(dev, "Received %u messages\n", msgs_received);
> +
>         /* tell the remote processor we added another available rx
> buffer */
>         virtqueue_kick(vrp->rvq);
>  }

Shouldn't the virtqueue_kick() be called only when we successfully added a buffer with virtqueue_add_buf()?

If so, we should make it:
	if (msgs_received)
		/* tell the remote processor we added another available rx buffer */
		virtqueue_kick(vrp->rvp);
}

Thanks for the rewrite, looks better.  I'll give it a try and let you know how it goes.

Regards,

- Rob

[PATCH v9 1/6] rpmsg: Process all available messages in virtqueue callback

[Ohad Ben-Cohen]

On Tue, Apr 9, 2013 at 1:56 AM, Tivy, Robert <rtivy@ti.com> wrote:
> Shouldn't the virtqueue_kick() be called only when we successfully added a buffer with virtqueue_add_buf()?

Definitely, thanks for noticing!

Take 2:

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index a59684b..4ade672 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -776,22 +776,11 @@ out:
 }
 EXPORT_SYMBOL(rpmsg_send_offchannel_raw);

-/* called when an rx buffer is used, and it's time to digest a message */
-static void rpmsg_recv_done(struct virtqueue *rvq)
+static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
+                            struct rpmsg_hdr *msg, unsigned int len)
 {
-       struct rpmsg_hdr *msg;
-       unsigned int len;
        struct rpmsg_endpoint *ept;
        struct scatterlist sg;
-       struct virtproc_info *vrp = rvq->vdev->priv;
-       struct device *dev = &rvq->vdev->dev;
-       int err;
-
-       msg = virtqueue_get_buf(rvq, &len);
-       if (!msg) {
-               dev_err(dev, "uhm, incoming signal, but no used buffer ?\n");
-               return;
-       }

        dev_dbg(dev, "From: 0x%x, To: 0x%x, Len: %d, Flags: %d, Reserved: %d\n",
                                        msg->src, msg->dst, msg->len,
@@ -806,7 +795,7 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
        if (len > RPMSG_BUF_SIZE ||
                msg->len > (len - sizeof(struct rpmsg_hdr))) {
                dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg->len);
-               return;
+               return -EINVAL;
        }

        /* use the dst addr to fetch the callback of the appropriate user */
@@ -842,11 +831,42 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
        err = virtqueue_add_buf(vrp->rvq, &sg, 0, 1, msg, GFP_KERNEL);
        if (err < 0) {
                dev_err(dev, "failed to add a virtqueue buffer: %d\n", err);
+               return err;
+       }
+
+       return 0;
+}
+
+/* called when an rx buffer is used, and it's time to digest a message */
+static void rpmsg_recv_done(struct virtqueue *rvq)
+{
+       struct virtproc_info *vrp = rvq->vdev->priv;
+       struct device *dev = &rvq->vdev->dev;
+       struct rpmsg_hdr *msg;
+       unsigned int len, msgs_received = 0;
+       int err;
+
+       msg = virtqueue_get_buf(rvq, &len);
+       if (!msg) {
+               dev_err(dev, "uhm, incoming signal, but no used buffer ?\n");
                return;
        }

+       while (msg) {
+               err = rpmsg_recv_single(vrp, dev, msg, len);
+               if (err)
+                       break;
+
+               msgs_received++;
+
+               msg = virtqueue_get_buf(rvq, &len);
+       };
+
+       dev_dbg(dev, "Received %u messages\n", msgs_received);
+
        /* tell the remote processor we added another available rx buffer */
-       virtqueue_kick(vrp->rvq);
+       if (msgs_received)
+               virtqueue_kick(vrp->rvq);
 }

 /*

> Thanks for the rewrite, looks better.  I'll give it a try and let you know how it goes.

Thanks!
Ohad.

[PATCH v9 1/6] rpmsg: Process all available messages in virtqueue callback

[Tivy, Robert]

Just one small fix needed (see below) and it's good-to-go.

> -----Original Message-----
> From: Ohad Ben-Cohen [mailto:ohad at wizery.com]
> Sent: Tuesday, April 09, 2013 1:26 AM
> 
> On Tue, Apr 9, 2013 at 1:56 AM, Tivy, Robert <rtivy@ti.com> wrote:
> > Shouldn't the virtqueue_kick() be called only when we successfully
> added a buffer with virtqueue_add_buf()?
> 
> Definitely, thanks for noticing!
> 
> Take 2:
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
> b/drivers/rpmsg/virtio_rpmsg_bus.c
> index a59684b..4ade672 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -776,22 +776,11 @@ out:
>  }
>  EXPORT_SYMBOL(rpmsg_send_offchannel_raw);
> 
> -/* called when an rx buffer is used, and it's time to digest a message
> */
> -static void rpmsg_recv_done(struct virtqueue *rvq)
> +static int rpmsg_recv_single(struct virtproc_info *vrp, struct device
> *dev,
> +                            struct rpmsg_hdr *msg, unsigned int len)
>  {
> -       struct rpmsg_hdr *msg;
> -       unsigned int len;
>         struct rpmsg_endpoint *ept;
>         struct scatterlist sg;
> -       struct virtproc_info *vrp = rvq->vdev->priv;
> -       struct device *dev = &rvq->vdev->dev;
> -       int err;

This new function also uses an 'int err;', so the above line should not be removed.

> -
> -       msg = virtqueue_get_buf(rvq, &len);
> -       if (!msg) {
> -               dev_err(dev, "uhm, incoming signal, but no used buffer
> ?\n");
> -               return;
> -       }
> 
>         dev_dbg(dev, "From: 0x%x, To: 0x%x, Len: %d, Flags: %d,
> Reserved: %d\n",
>                                         msg->src, msg->dst, msg->len,
> @@ -806,7 +795,7 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
>         if (len > RPMSG_BUF_SIZE ||
>                 msg->len > (len - sizeof(struct rpmsg_hdr))) {
>                 dev_warn(dev, "inbound msg too big: (%d, %d)\n", len,
> msg->len);
> -               return;
> +               return -EINVAL;
>         }
> 
>         /* use the dst addr to fetch the callback of the appropriate
> user */
> @@ -842,11 +831,42 @@ static void rpmsg_recv_done(struct virtqueue
> *rvq)
>         err = virtqueue_add_buf(vrp->rvq, &sg, 0, 1, msg, GFP_KERNEL);
>         if (err < 0) {
>                 dev_err(dev, "failed to add a virtqueue buffer: %d\n",
> err);
> +               return err;
> +       }
> +
> +       return 0;
> +}
> +
> +/* called when an rx buffer is used, and it's time to digest a message
> */
> +static void rpmsg_recv_done(struct virtqueue *rvq)
> +{
> +       struct virtproc_info *vrp = rvq->vdev->priv;
> +       struct device *dev = &rvq->vdev->dev;
> +       struct rpmsg_hdr *msg;
> +       unsigned int len, msgs_received = 0;
> +       int err;
> +
> +       msg = virtqueue_get_buf(rvq, &len);
> +       if (!msg) {
> +               dev_err(dev, "uhm, incoming signal, but no used buffer
> ?\n");
>                 return;
>         }
> 
> +       while (msg) {
> +               err = rpmsg_recv_single(vrp, dev, msg, len);
> +               if (err)
> +                       break;
> +
> +               msgs_received++;
> +
> +               msg = virtqueue_get_buf(rvq, &len);
> +       };
> +
> +       dev_dbg(dev, "Received %u messages\n", msgs_received);
> +
>         /* tell the remote processor we added another available rx
> buffer */
> -       virtqueue_kick(vrp->rvq);
> +       if (msgs_received)
> +               virtqueue_kick(vrp->rvq);
>  }
> 
>  /*
> 
> > Thanks for the rewrite, looks better.  I'll give it a try and let you
> know how it goes.
> 
> Thanks!
> Ohad.

I added the above-mentioned 'int err;' to my tree's rpmsg_recv_single() and tested it out, it worked well.  So once that is corrected, you can add:
Acked-by: Robert Tivy <rtivy@ti.com>

Regards,

- Rob

[PATCH v9 1/6] rpmsg: Process all available messages in virtqueue callback

[Ohad Ben-Cohen]

On Tue, Apr 9, 2013 at 11:56 PM, Tivy, Robert <rtivy@ti.com> wrote:
> This new function also uses an 'int err;', so the above line should not be removed.

Added, thanks!

> I added the above-mentioned 'int err;' to my tree's rpmsg_recv_single() and tested it out, it worked well.  So once that is corrected, you can add:
> Acked-by: Robert Tivy <rtivy@ti.com>

No need to add your Acked-by as you are the author :)

Applied - will show up in linux next soon, please check it out there.

Thanks,
Ohad.

[PATCH v9 4/6] ARM: davinci: Add a remoteproc driver implementation for OMAP-L13x DSP

[Ohad Ben-Cohen]

Hi Rob,

On Fri, Mar 29, 2013 at 4:41 AM, Robert Tivy <rtivy@ti.com> wrote:> +
> +static int da8xx_rproc_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct da8xx_rproc *drproc;
> +       struct rproc *rproc;
> +       struct irq_data *irq_data;
> +       struct resource *bootreg_res;
> +       struct resource *chipsig_res;
> +       struct clk *dsp_clk;
> +       void __iomem *chipsig;
> +       void __iomem *bootreg;
> +       int irq;
> +       int ret;
> +
> +       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 (!irq_data) {
> +               dev_err(dev, "irq_get_irq_data(%d): NULL\n", irq);
> +               return -EINVAL;
> +       }
> +
> +       bootreg_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!bootreg_res) {
> +               dev_err(dev,
> +                       "platform_get_resource(IORESOURCE_MEM, 0): NULL\n");
> +               return -EADDRNOTAVAIL;
> +       }
> +
> +       chipsig_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +       if (!chipsig_res) {
> +               dev_err(dev,
> +                       "platform_get_resource(IORESOURCE_MEM, 1): NULL\n");
> +               return -EADDRNOTAVAIL;
> +       }
> +
> +       bootreg = devm_request_and_ioremap(dev, bootreg_res);
> +       if (!bootreg) {
> +               dev_err(dev, "unable to map boot register\n");
> +               return -EADDRNOTAVAIL;
> +       }
> +
> +       chipsig = devm_request_and_ioremap(dev, chipsig_res);
> +       if (!chipsig) {
> +               dev_err(dev, "unable to map CHIPSIG register\n");
> +               return -EADDRNOTAVAIL;
> +       }
> +
> +       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);
> +       }
> +
> +       rproc = rproc_alloc(dev, "dsp", &da8xx_rproc_ops, da8xx_fw_name,
> +               sizeof(*drproc));
> +       if (!rproc)
> +               return -ENOMEM;
> +
> +       drproc = rproc->priv;
> +       drproc->rproc = rproc;
> +
> +       platform_set_drvdata(pdev, rproc);
> +
> +       /* 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);
> +               goto free_rproc;
> +       }
> +
> +       /*
> +        * rproc_add() can end up enabling the DSP's clk with the DSP
> +        * *not* in reset, but da8xx_rproc_start() needs the DSP to be
> +        * held in reset at the time it is called.
> +        */
> +       ret = reset_assert(dev);
> +       if (ret)
> +               goto free_rproc;
> +
> +       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;
> +       drproc->dsp_clk = dsp_clk;
> +
> +       return 0;
> +
> +free_rproc:
> +       rproc_put(rproc);
> +
> +       return ret;
> +}

Two small changes please:

- could you please fix the error path of probe? it seems that some
resources are taken but not freed in case a failure shows up
subsequently
- seems like it's safer to set drproc before calling rproc_add
otherwise you have a small race there

If you could respin this one quickly it'd be great, thanks!

Ohad.

[PATCH v9 4/6] ARM: davinci: Add a remoteproc driver implementation for OMAP-L13x DSP

[Sergei Shtylyov]

Hello.

On 04/07/2013 05:02 PM, Ohad Ben-Cohen wrote:

>
>> +static int da8xx_rproc_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct da8xx_rproc *drproc;
>> +       struct rproc *rproc;
>> +       struct irq_data *irq_data;
>> +       struct resource *bootreg_res;
>> +       struct resource *chipsig_res;
>> +       struct clk *dsp_clk;
>> +       void __iomem *chipsig;
>> +       void __iomem *bootreg;
>> +       int irq;
>> +       int ret;
>> +
[...]
>> +       bootreg = devm_request_and_ioremap(dev, bootreg_res);
>> +       if (!bootreg) {
>> +               dev_err(dev, "unable to map boot register\n");
>> +               return -EADDRNOTAVAIL;
>> +       }
>> +
>> +       chipsig = devm_request_and_ioremap(dev, chipsig_res);

    I suggest that you use more modern (yes, already a newer interface :-)
devm_ioremap_resource() instead -- it returns the error code (as a pointer)
in case of error, and it certainly doesn't require you to print error 
messages.

>> +       if (!chipsig) {
>> +               dev_err(dev, "unable to map CHIPSIG register\n");
>> +               return -EADDRNOTAVAIL;
>> +       }
>>

WBR, Sergei

[PATCH v9 4/6] ARM: davinci: Add a remoteproc driver implementation for OMAP-L13x DSP

[Tivy, Robert]

Hi Sergei,

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sergei.shtylyov at cogentembedded.com]
> Sent: Sunday, April 07, 2013 10:55 AM
> 
> Hello.
> 
> On 04/07/2013 05:02 PM, Ohad Ben-Cohen wrote:
> 
> >
> >> +static int da8xx_rproc_probe(struct platform_device *pdev)
> >> +{
> >> +       struct device *dev = &pdev->dev;
> >> +       struct da8xx_rproc *drproc;
> >> +       struct rproc *rproc;
> >> +       struct irq_data *irq_data;
> >> +       struct resource *bootreg_res;
> >> +       struct resource *chipsig_res;
> >> +       struct clk *dsp_clk;
> >> +       void __iomem *chipsig;
> >> +       void __iomem *bootreg;
> >> +       int irq;
> >> +       int ret;
> >> +
> [...]
> >> +       bootreg = devm_request_and_ioremap(dev, bootreg_res);
> >> +       if (!bootreg) {
> >> +               dev_err(dev, "unable to map boot register\n");
> >> +               return -EADDRNOTAVAIL;
> >> +       }
> >> +
> >> +       chipsig = devm_request_and_ioremap(dev, chipsig_res);
> 
>     I suggest that you use more modern (yes, already a newer interface
> :-)
> devm_ioremap_resource() instead -- it returns the error code (as a
> pointer)
> in case of error, and it certainly doesn't require you to print error
> messages.

Thanks, will do.

I appreciate the notice of a more modern function, it's really tough to keep up with the flurry of activity to the kernel.

Regarding this change, should the code use
	return PTR_ERR(bootreg);
or
	return PTR_RET(bootreg);
I ask because PTR_ERR() returns 'long' whereas PTR_RET() returns 'int' (and probe returns 'int'), but I see that the majority of existing code uses "return PTR_ERR()" in probe functions.

> 
> >> +       if (!chipsig) {
> >> +               dev_err(dev, "unable to map CHIPSIG register\n");
> >> +               return -EADDRNOTAVAIL;
> >> +       }
> >>
> 
> WBR, Sergei

Regards,

- Rob

[PATCH v9 4/6] ARM: davinci: Add a remoteproc driver implementation for OMAP-L13x DSP

[Sergei Shtylyov]

Hello.

On 09-04-2013 3:55, Tivy, Robert wrote:

>>>> +static int da8xx_rproc_probe(struct platform_device *pdev)
>>>> +{
>>>> +       struct device *dev = &pdev->dev;
>>>> +       struct da8xx_rproc *drproc;
>>>> +       struct rproc *rproc;
>>>> +       struct irq_data *irq_data;
>>>> +       struct resource *bootreg_res;
>>>> +       struct resource *chipsig_res;
>>>> +       struct clk *dsp_clk;
>>>> +       void __iomem *chipsig;
>>>> +       void __iomem *bootreg;
>>>> +       int irq;
>>>> +       int ret;
>>>> +
>> [...]
>>>> +       bootreg = devm_request_and_ioremap(dev, bootreg_res);
>>>> +       if (!bootreg) {
>>>> +               dev_err(dev, "unable to map boot register\n");
>>>> +               return -EADDRNOTAVAIL;
>>>> +       }
>>>> +
>>>> +       chipsig = devm_request_and_ioremap(dev, chipsig_res);

>>      I suggest that you use more modern (yes, already a newer interface
>> :-)
>> devm_ioremap_resource() instead -- it returns the error code (as a
>> pointer)
>> in case of error, and it certainly doesn't require you to print error
>> messages.

> Thanks, will do.

> I appreciate the notice of a more modern function, it's really tough to keep up with the flurry of activity to the kernel.

> Regarding this change, should the code use
> 	return PTR_ERR(bootreg);
> or
> 	return PTR_RET(bootreg);

    The former, to avoid duplicate IS_ERR() check.

> I ask because PTR_ERR() returns 'long' whereas PTR_RET() returns 'int' (and probe returns 'int'), but I see that the majority of existing code uses "return PTR_ERR()" in probe functions.

    But PTR_RET() uses PTR_ERR() internally anyway.

>>>> +       if (!chipsig) {
>>>> +               dev_err(dev, "unable to map CHIPSIG register\n");
>>>> +               return -EADDRNOTAVAIL;
>>>> +       }

WBR, Sergei

[PATCH v9 4/6] ARM: davinci: Add a remoteproc driver implementation for OMAP-L13x DSP

[Tivy, Robert]

Hi Ohad,

> -----Original Message-----
> From: Ohad Ben-Cohen [mailto:ohad at wizery.com]
> Sent: Sunday, April 07, 2013 6:03 AM
> To: Tivy, Robert
> Cc: davinci-linux-open-source; linux-arm; Nori, Sekhar; Rob Landley;
> linux-doc at vger.kernel.org; Grosen, Mark; Ring, Chris
> Subject: Re: [PATCH v9 4/6] ARM: davinci: Add a remoteproc driver
> implementation for OMAP-L13x DSP
> 
> Hi Rob,
> 
> On Fri, Mar 29, 2013 at 4:41 AM, Robert Tivy <rtivy@ti.com> wrote:> +
> > +static int da8xx_rproc_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct da8xx_rproc *drproc;
> > +       struct rproc *rproc;
> > +       struct irq_data *irq_data;
> > +       struct resource *bootreg_res;
> > +       struct resource *chipsig_res;
> > +       struct clk *dsp_clk;
> > +       void __iomem *chipsig;
> > +       void __iomem *bootreg;
> > +       int irq;
> > +       int ret;
> > +
> > +       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 (!irq_data) {
> > +               dev_err(dev, "irq_get_irq_data(%d): NULL\n", irq);
> > +               return -EINVAL;
> > +       }
> > +
> > +       bootreg_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       if (!bootreg_res) {
> > +               dev_err(dev,
> > +                       "platform_get_resource(IORESOURCE_MEM, 0):
> NULL\n");
> > +               return -EADDRNOTAVAIL;
> > +       }
> > +
> > +       chipsig_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +       if (!chipsig_res) {
> > +               dev_err(dev,
> > +                       "platform_get_resource(IORESOURCE_MEM, 1):
> NULL\n");
> > +               return -EADDRNOTAVAIL;
> > +       }
> > +
> > +       bootreg = devm_request_and_ioremap(dev, bootreg_res);
> > +       if (!bootreg) {
> > +               dev_err(dev, "unable to map boot register\n");
> > +               return -EADDRNOTAVAIL;
> > +       }
> > +
> > +       chipsig = devm_request_and_ioremap(dev, chipsig_res);
> > +       if (!chipsig) {
> > +               dev_err(dev, "unable to map CHIPSIG register\n");
> > +               return -EADDRNOTAVAIL;
> > +       }
> > +
> > +       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);
> > +       }
> > +
> > +       rproc = rproc_alloc(dev, "dsp", &da8xx_rproc_ops,
> da8xx_fw_name,
> > +               sizeof(*drproc));
> > +       if (!rproc)
> > +               return -ENOMEM;
> > +
> > +       drproc = rproc->priv;
> > +       drproc->rproc = rproc;
> > +
> > +       platform_set_drvdata(pdev, rproc);
> > +
> > +       /* 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);
> > +               goto free_rproc;
> > +       }
> > +
> > +       /*
> > +        * rproc_add() can end up enabling the DSP's clk with the DSP
> > +        * *not* in reset, but da8xx_rproc_start() needs the DSP to
> be
> > +        * held in reset at the time it is called.
> > +        */
> > +       ret = reset_assert(dev);
> > +       if (ret)
> > +               goto free_rproc;
> > +
> > +       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;
> > +       drproc->dsp_clk = dsp_clk;
> > +
> > +       return 0;
> > +
> > +free_rproc:
> > +       rproc_put(rproc);
> > +
> > +       return ret;
> > +}
> 
> Two small changes please:
> 
> - could you please fix the error path of probe? it seems that some
> resources are taken but not freed in case a failure shows up
> subsequently

I'm not sure what resources you mean?

The platform_get_irq()/irq_get_irq_data()/platform_get_resource() calls don't have "put" counterparts.  The "devm_*" calls get automatically cleaned up (and I wasn't able to find any existing example of some code that actually does the cleanup explicitly).

> - seems like it's safer to set drproc before calling rproc_add
> otherwise you have a small race there

Agreed, will change.

> 
> If you could respin this one quickly it'd be great, thanks!
> 
> Ohad.

Sergei had a comment to change devm_request_and_ioremap() to devm_ioremap_resource(), so I will submit a new patch with your and Sergei's comments applied.

Regards,

- Rob

[PATCH v9 4/6] ARM: davinci: Add a remoteproc driver implementation for OMAP-L13x DSP

[Ohad Ben-Cohen]

On Tue, Apr 9, 2013 at 1:02 AM, Tivy, Robert <rtivy@ti.com> wrote:
> The platform_get_irq()/irq_get_irq_data()/platform_get_resource() calls don't have "put" counterparts.  The "devm_*" calls get automatically cleaned up (and I wasn't able to find any existing example of some code that actually does the cleanup explicitly).

Fair enough, thanks for looking.

> Sergei had a comment to change devm_request_and_ioremap() to devm_ioremap_resource(), so I will submit a new patch with your and Sergei's comments applied.

Thanks!

[PATCH v9 0/6] ARM: davinci: remoteproc support

[Ohad Ben-Cohen]

Hi Rob,

On Fri, Mar 29, 2013 at 4:41 AM, Robert Tivy <rtivy@ti.com> wrote:
> This patch series adds remoteproc support for OMAP-L13x, along with needed
> supporting mach-davinci infrastructure.

Patches look good, thanks for pushing.

I'm applying 2 and 3 now. Please ack the change I've done for 1 and
I'll apply it too. No. 4 just needs a small change, if you could
please respin quickly I'll apply it immediately.

I prefer patches 5 and 6 go through Sekhar, guess after I'll have no.
4 applied in an immutable branch Sekhar could pull it and apply 5 and
6.

Thanks,
Ohad.

[PATCH v9 0/6] ARM: davinci: remoteproc support

[Sekhar Nori]

On 4/7/2013 6:37 PM, Ohad Ben-Cohen wrote:
> Hi Rob,
> 
> On Fri, Mar 29, 2013 at 4:41 AM, Robert Tivy <rtivy@ti.com> wrote:
>> This patch series adds remoteproc support for OMAP-L13x, along with needed
>> supporting mach-davinci infrastructure.
> 
> Patches look good, thanks for pushing.
> 
> I'm applying 2 and 3 now. Please ack the change I've done for 1 and
> I'll apply it too. No. 4 just needs a small change, if you could
> please respin quickly I'll apply it immediately.
> 
> I prefer patches 5 and 6 go through Sekhar, guess after I'll have no.
> 4 applied in an immutable branch Sekhar could pull it and apply 5 and
> 6.

That should be fine. Please let me know once you have the immutable branch.

Thanks,
Sekhar