[v4,13/17] remoteproc: create vdev subdevice with specific dma memory pool

Commit Message

Loic PALLARDY July 27, 2018, 1:14 p.m. UTC

This patch creates a dedicated vdev subdevice for each vdev declared
in firmware resource table and associates carveout named "vdev%dbuffer"
(with %d vdev index in resource table) if any as dma coherent memory pool.

Then vdev subdevice is used as parent for virtio device.

Signed-off-by: Loic Pallardy <[e-mail]>
---
 drivers/remoteproc/remoteproc_core.c     | 35 +++++++++++++++++++++++---
 drivers/remoteproc/remoteproc_internal.h |  1 +
 drivers/remoteproc/remoteproc_virtio.c   | 42 +++++++++++++++++++++++++++++++-
 include/linux/remoteproc.h               |  1 +
 4 files changed, 75 insertions(+), 4 deletions(-)

Comments

Wendy Liang Sept. 27, 2018, 5:17 p.m. UTC | #1

On Fri, Jul 27, 2018 at 6:16 AM Loic Pallardy <[e-mail]> wrote:
>
> This patch creates a dedicated vdev subdevice for each vdev declared
> in firmware resource table and associates carveout named "vdev%dbuffer"
> (with %d vdev index in resource table) if any as dma coherent memory pool.
>
> Then vdev subdevice is used as parent for virtio device.
>
> Signed-off-by: Loic Pallardy <[e-mail]>
> ---
>  drivers/remoteproc/remoteproc_core.c     | 35 +++++++++++++++++++++++---
>  drivers/remoteproc/remoteproc_internal.h |  1 +
>  drivers/remoteproc/remoteproc_virtio.c   | 42 +++++++++++++++++++++++++++++++-
>  include/linux/remoteproc.h               |  1 +
>  4 files changed, 75 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 4edc6f0..adcc66e 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -39,6 +39,7 @@
>  #include <linux/idr.h>
>  #include <linux/elf.h>
>  #include <linux/crc32.h>
> +#include <linux/of_reserved_mem.h>
>  #include <linux/virtio_ids.h>
>  #include <linux/virtio_ring.h>
>  #include <asm/byteorder.h>
> @@ -145,7 +146,7 @@ static void rproc_disable_iommu(struct rproc *rproc)
>         iommu_domain_free(domain);
>  }
>
> -static phys_addr_t rproc_va_to_pa(void *cpu_addr)
> +phys_addr_t rproc_va_to_pa(void *cpu_addr)
>  {
>         /*
>          * Return physical address according to virtual address location
> @@ -160,6 +161,7 @@ static phys_addr_t rproc_va_to_pa(void *cpu_addr)
>         WARN_ON(!virt_addr_valid(cpu_addr));
>         return virt_to_phys(cpu_addr);
>  }
> +EXPORT_SYMBOL(rproc_va_to_pa);
>
>  /**
>   * rproc_da_to_va() - lookup the kernel virtual address for a remoteproc address
> @@ -423,6 +425,20 @@ static void rproc_vdev_do_stop(struct rproc_subdev *subdev, bool crashed)
>  }
>
>  /**
> + * rproc_rvdev_release() - release the existence of a rvdev
> + *
> + * @dev: the subdevice's dev
> + */
> +static void rproc_rvdev_release(struct device *dev)
> +{
> +       struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev, dev);
> +
> +       of_reserved_mem_device_release(dev);
> +
> +       kfree(rvdev);
> +}
> +
> +/**
>   * rproc_handle_vdev() - handle a vdev fw resource
>   * @rproc: the remote processor
>   * @rsc: the vring resource descriptor
> @@ -455,6 +471,7 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
>         struct device *dev = &rproc->dev;
>         struct rproc_vdev *rvdev;
>         int i, ret;
> +       char name[16];
>
>         /* make sure resource isn't truncated */
>         if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct fw_rsc_vdev_vring)
> @@ -488,6 +505,18 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
>         rvdev->rproc = rproc;
>         rvdev->index = rproc->nb_vdev++;
>
> +       /* Initialise vdev subdevice */
> +       snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
> +       rvdev->dev.parent = rproc->dev.parent;
> +       rvdev->dev.release = rproc_rvdev_release;
> +       dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev->dev.parent), name);
> +       dev_set_drvdata(&rvdev->dev, rvdev);
> +       dma_set_coherent_mask(&rvdev->dev, DMA_BIT_MASK(32));
I tried the latest kernel, this function will not set the DMA coherent mask as
dma_supported() of the &rvdev->dev will return false.
As this is a device created at run time, should it be force to support DMA?
should it directly set the dma_coherent_mask?

> +
> +       ret = device_register(&rvdev->dev);
> +       if (ret)
> +               goto free_rvdev;
> +
>         /* parse the vrings */
>         for (i = 0; i < rsc->num_of_vrings; i++) {
>                 ret = rproc_parse_vring(rvdev, rsc, i);
> @@ -518,7 +547,7 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
>         for (i--; i >= 0; i--)
>                 rproc_free_vring(&rvdev->vring[i]);
>  free_rvdev:
> -       kfree(rvdev);
> +       device_unregister(&rvdev->dev);
>         return ret;
>  }
>
> @@ -536,7 +565,7 @@ void rproc_vdev_release(struct kref *ref)
>
>         rproc_remove_subdev(rproc, &rvdev->subdev);
>         list_del(&rvdev->node);
> -       kfree(rvdev);
> +       device_unregister(&rvdev->dev);
>  }
>
>  /**
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index f6cad24..bfeacfd 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -52,6 +52,7 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
>  int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
>
>  void *rproc_da_to_va(struct rproc *rproc, u64 da, int len);
> +phys_addr_t rproc_va_to_pa(void *cpu_addr);
>  int rproc_trigger_recovery(struct rproc *rproc);
>
>  int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw);
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index de21f62..9ee63c0 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -17,7 +17,9 @@
>   * GNU General Public License for more details.
>   */
>
> +#include <linux/dma-mapping.h>
>  #include <linux/export.h>
> +#include <linux/of_reserved_mem.h>
>  #include <linux/remoteproc.h>
>  #include <linux/virtio.h>
>  #include <linux/virtio_config.h>
> @@ -315,10 +317,48 @@ static void rproc_virtio_dev_release(struct device *dev)
>  int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>  {
>         struct rproc *rproc = rvdev->rproc;
> -       struct device *dev = &rproc->dev;
> +       struct device *dev = &rvdev->dev;
>         struct virtio_device *vdev = &rvdev->vdev;
> +       struct rproc_mem_entry *mem;
>         int ret;
>
> +       /* Try to find dedicated vdev buffer carveout */
> +       mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer", rvdev->index);
> +       if (mem) {
> +               phys_addr_t pa;
> +
> +               if (mem->of_resm_idx != -1) {
> +                       struct device_node *np = rproc->dev.parent->of_node;
> +
> +                       /* Associate reserved memory to vdev device */
> +                       ret = of_reserved_mem_device_init_by_idx(dev, np,
> +                                                                mem->of_resm_idx);
> +                       if (ret) {
> +                               dev_err(dev, "Can't associate reserved memory\n");
> +                               goto out;
> +                       }
> +               } else {
> +                       if (mem->va) {
> +                               dev_warn(dev, "vdev %d buffer already mapped\n",
> +                                        rvdev->index);
> +                               pa = rproc_va_to_pa(mem->va);
> +                       } else {
> +                               /* Use dma address as carveout no memmapped yet */
> +                               pa = (phys_addr_t)mem->dma;
> +                       }
> +
> +                       /* Associate vdev buffer memory pool to vdev subdev */
> +                       ret = dmam_declare_coherent_memory(dev, pa,
> +                                                          mem->da,
> +                                                          mem->len,
> +                                                          DMA_MEMORY_EXCLUSIVE);
> +                       if (ret < 0) {
> +                               dev_err(dev, "Failed to associate buffer\n");
> +                               goto out;
> +                       }
> +               }
> +       }
> +
>         vdev->id.device = id,
>         vdev->config = &rproc_virtio_config_ops,
>         vdev->dev.parent = dev;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 6b3a234..2921dd2 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -547,6 +547,7 @@ struct rproc_vdev {
>         struct kref refcount;
>
>         struct rproc_subdev subdev;
> +       struct device dev;
>
>         unsigned int id;
>         struct list_head node;
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Loic PALLARDY Sept. 27, 2018, 7:22 p.m. UTC | #2

Hi Wendy

> -----Original Message-----
> From: Wendy Liang <[e-mail]>
> Sent: Thursday, September 27, 2018 7:17 PM
> To: Loic PALLARDY <[e-mail]>
> Cc: Bjorn Andersson <[e-mail]>; Ohad Ben-Cohen
> <[e-mail]>; [e-mail]; Linux Kernel
> Mailing List <[e-mail]>; Arnaud POULIQUEN
> <[e-mail]>; [e-mail]; Suman Anna
> <[e-mail]>
> Subject: Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with
> specific dma memory pool
> 
> On Fri, Jul 27, 2018 at 6:16 AM Loic Pallardy <[e-mail]> wrote:
> >
> > This patch creates a dedicated vdev subdevice for each vdev declared
> > in firmware resource table and associates carveout named "vdev%dbuffer"
> > (with %d vdev index in resource table) if any as dma coherent memory
> pool.
> >
> > Then vdev subdevice is used as parent for virtio device.
> >
> > Signed-off-by: Loic Pallardy <[e-mail]>
> > ---
> >  drivers/remoteproc/remoteproc_core.c     | 35
> +++++++++++++++++++++++---
> >  drivers/remoteproc/remoteproc_internal.h |  1 +
> >  drivers/remoteproc/remoteproc_virtio.c   | 42
> +++++++++++++++++++++++++++++++-
> >  include/linux/remoteproc.h               |  1 +
> >  4 files changed, 75 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> > index 4edc6f0..adcc66e 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -39,6 +39,7 @@
> >  #include <linux/idr.h>
> >  #include <linux/elf.h>
> >  #include <linux/crc32.h>
> > +#include <linux/of_reserved_mem.h>
> >  #include <linux/virtio_ids.h>
> >  #include <linux/virtio_ring.h>
> >  #include <asm/byteorder.h>
> > @@ -145,7 +146,7 @@ static void rproc_disable_iommu(struct rproc
> *rproc)
> >         iommu_domain_free(domain);
> >  }
> >
> > -static phys_addr_t rproc_va_to_pa(void *cpu_addr)
> > +phys_addr_t rproc_va_to_pa(void *cpu_addr)
> >  {
> >         /*
> >          * Return physical address according to virtual address location
> > @@ -160,6 +161,7 @@ static phys_addr_t rproc_va_to_pa(void
> *cpu_addr)
> >         WARN_ON(!virt_addr_valid(cpu_addr));
> >         return virt_to_phys(cpu_addr);
> >  }
> > +EXPORT_SYMBOL(rproc_va_to_pa);
> >
> >  /**
> >   * rproc_da_to_va() - lookup the kernel virtual address for a remoteproc
> address
> > @@ -423,6 +425,20 @@ static void rproc_vdev_do_stop(struct
> rproc_subdev *subdev, bool crashed)
> >  }
> >
> >  /**
> > + * rproc_rvdev_release() - release the existence of a rvdev
> > + *
> > + * @dev: the subdevice's dev
> > + */
> > +static void rproc_rvdev_release(struct device *dev)
> > +{
> > +       struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev, dev);
> > +
> > +       of_reserved_mem_device_release(dev);
> > +
> > +       kfree(rvdev);
> > +}
> > +
> > +/**
> >   * rproc_handle_vdev() - handle a vdev fw resource
> >   * @rproc: the remote processor
> >   * @rsc: the vring resource descriptor
> > @@ -455,6 +471,7 @@ static int rproc_handle_vdev(struct rproc *rproc,
> struct fw_rsc_vdev *rsc,
> >         struct device *dev = &rproc->dev;
> >         struct rproc_vdev *rvdev;
> >         int i, ret;
> > +       char name[16];
> >
> >         /* make sure resource isn't truncated */
> >         if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct
> fw_rsc_vdev_vring)
> > @@ -488,6 +505,18 @@ static int rproc_handle_vdev(struct rproc *rproc,
> struct fw_rsc_vdev *rsc,
> >         rvdev->rproc = rproc;
> >         rvdev->index = rproc->nb_vdev++;
> >
> > +       /* Initialise vdev subdevice */
> > +       snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
> > +       rvdev->dev.parent = rproc->dev.parent;
> > +       rvdev->dev.release = rproc_rvdev_release;
> > +       dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev-
> >dev.parent), name);
> > +       dev_set_drvdata(&rvdev->dev, rvdev);
> > +       dma_set_coherent_mask(&rvdev->dev, DMA_BIT_MASK(32));
> I tried the latest kernel, this function will not set the DMA coherent mask as
> dma_supported() of the &rvdev->dev will return false.
> As this is a device created at run time, should it be force to support DMA?
> should it directly set the dma_coherent_mask?

Thanks for pointing me this issue. I tested on top of 4.18-rc1 few months ago...
Could you please give me kernel version on which you are testing the series?
Is you platform 32bit or 64bit ?
I'll rebase and check on my side.

Regards,
Loic

> 
> > +
> > +       ret = device_register(&rvdev->dev);
> > +       if (ret)
> > +               goto free_rvdev;
> > +
> >         /* parse the vrings */
> >         for (i = 0; i < rsc->num_of_vrings; i++) {
> >                 ret = rproc_parse_vring(rvdev, rsc, i);
> > @@ -518,7 +547,7 @@ static int rproc_handle_vdev(struct rproc *rproc,
> struct fw_rsc_vdev *rsc,
> >         for (i--; i >= 0; i--)
> >                 rproc_free_vring(&rvdev->vring[i]);
> >  free_rvdev:
> > -       kfree(rvdev);
> > +       device_unregister(&rvdev->dev);
> >         return ret;
> >  }
> >
> > @@ -536,7 +565,7 @@ void rproc_vdev_release(struct kref *ref)
> >
> >         rproc_remove_subdev(rproc, &rvdev->subdev);
> >         list_del(&rvdev->node);
> > -       kfree(rvdev);
> > +       device_unregister(&rvdev->dev);
> >  }
> >
> >  /**
> > diff --git a/drivers/remoteproc/remoteproc_internal.h
> b/drivers/remoteproc/remoteproc_internal.h
> > index f6cad24..bfeacfd 100644
> > --- a/drivers/remoteproc/remoteproc_internal.h
> > +++ b/drivers/remoteproc/remoteproc_internal.h
> > @@ -52,6 +52,7 @@ struct dentry *rproc_create_trace_file(const char
> *name, struct rproc *rproc,
> >  int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
> >
> >  void *rproc_da_to_va(struct rproc *rproc, u64 da, int len);
> > +phys_addr_t rproc_va_to_pa(void *cpu_addr);
> >  int rproc_trigger_recovery(struct rproc *rproc);
> >
> >  int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw);
> > diff --git a/drivers/remoteproc/remoteproc_virtio.c
> b/drivers/remoteproc/remoteproc_virtio.c
> > index de21f62..9ee63c0 100644
> > --- a/drivers/remoteproc/remoteproc_virtio.c
> > +++ b/drivers/remoteproc/remoteproc_virtio.c
> > @@ -17,7 +17,9 @@
> >   * GNU General Public License for more details.
> >   */
> >
> > +#include <linux/dma-mapping.h>
> >  #include <linux/export.h>
> > +#include <linux/of_reserved_mem.h>
> >  #include <linux/remoteproc.h>
> >  #include <linux/virtio.h>
> >  #include <linux/virtio_config.h>
> > @@ -315,10 +317,48 @@ static void rproc_virtio_dev_release(struct device
> *dev)
> >  int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
> >  {
> >         struct rproc *rproc = rvdev->rproc;
> > -       struct device *dev = &rproc->dev;
> > +       struct device *dev = &rvdev->dev;
> >         struct virtio_device *vdev = &rvdev->vdev;
> > +       struct rproc_mem_entry *mem;
> >         int ret;
> >
> > +       /* Try to find dedicated vdev buffer carveout */
> > +       mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer", rvdev-
> >index);
> > +       if (mem) {
> > +               phys_addr_t pa;
> > +
> > +               if (mem->of_resm_idx != -1) {
> > +                       struct device_node *np = rproc->dev.parent->of_node;
> > +
> > +                       /* Associate reserved memory to vdev device */
> > +                       ret = of_reserved_mem_device_init_by_idx(dev, np,
> > +                                                                mem->of_resm_idx);
> > +                       if (ret) {
> > +                               dev_err(dev, "Can't associate reserved memory\n");
> > +                               goto out;
> > +                       }
> > +               } else {
> > +                       if (mem->va) {
> > +                               dev_warn(dev, "vdev %d buffer already mapped\n",
> > +                                        rvdev->index);
> > +                               pa = rproc_va_to_pa(mem->va);
> > +                       } else {
> > +                               /* Use dma address as carveout no memmapped yet */
> > +                               pa = (phys_addr_t)mem->dma;
> > +                       }
> > +
> > +                       /* Associate vdev buffer memory pool to vdev subdev */
> > +                       ret = dmam_declare_coherent_memory(dev, pa,
> > +                                                          mem->da,
> > +                                                          mem->len,
> > +                                                          DMA_MEMORY_EXCLUSIVE);
> > +                       if (ret < 0) {
> > +                               dev_err(dev, "Failed to associate buffer\n");
> > +                               goto out;
> > +                       }
> > +               }
> > +       }
> > +
> >         vdev->id.device = id,
> >         vdev->config = &rproc_virtio_config_ops,
> >         vdev->dev.parent = dev;
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index 6b3a234..2921dd2 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -547,6 +547,7 @@ struct rproc_vdev {
> >         struct kref refcount;
> >
> >         struct rproc_subdev subdev;
> > +       struct device dev;
> >
> >         unsigned int id;
> >         struct list_head node;
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-remoteproc"
> in
> > the body of a message to [e-mail]
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Wendy Liang Sept. 27, 2018, 8:18 p.m. UTC | #3

Hi Loic,


On Thu, Sep 27, 2018 at 12:22 PM Loic PALLARDY <[e-mail]> wrote:
>
> Hi Wendy
>
> > -----Original Message-----
> > From: Wendy Liang <[e-mail]>
> > Sent: Thursday, September 27, 2018 7:17 PM
> > To: Loic PALLARDY <[e-mail]>
> > Cc: Bjorn Andersson <[e-mail]>; Ohad Ben-Cohen
> > <[e-mail]>; [e-mail]; Linux Kernel
> > Mailing List <[e-mail]>; Arnaud POULIQUEN
> > <[e-mail]>; [e-mail]; Suman Anna
> > <[e-mail]>
> > Subject: Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with
> > specific dma memory pool
> >
> > On Fri, Jul 27, 2018 at 6:16 AM Loic Pallardy <[e-mail]> wrote:
> > >
> > > This patch creates a dedicated vdev subdevice for each vdev declared
> > > in firmware resource table and associates carveout named "vdev%dbuffer"
> > > (with %d vdev index in resource table) if any as dma coherent memory
> > pool.
> > >
> > > Then vdev subdevice is used as parent for virtio device.
> > >
> > > Signed-off-by: Loic Pallardy <[e-mail]>
> > > ---
> > >  drivers/remoteproc/remoteproc_core.c     | 35
> > +++++++++++++++++++++++---
> > >  drivers/remoteproc/remoteproc_internal.h |  1 +
> > >  drivers/remoteproc/remoteproc_virtio.c   | 42
> > +++++++++++++++++++++++++++++++-
> > >  include/linux/remoteproc.h               |  1 +
> > >  4 files changed, 75 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_core.c
> > b/drivers/remoteproc/remoteproc_core.c
> > > index 4edc6f0..adcc66e 100644
> > > --- a/drivers/remoteproc/remoteproc_core.c
> > > +++ b/drivers/remoteproc/remoteproc_core.c
> > > @@ -39,6 +39,7 @@
> > >  #include <linux/idr.h>
> > >  #include <linux/elf.h>
> > >  #include <linux/crc32.h>
> > > +#include <linux/of_reserved_mem.h>
> > >  #include <linux/virtio_ids.h>
> > >  #include <linux/virtio_ring.h>
> > >  #include <asm/byteorder.h>
> > > @@ -145,7 +146,7 @@ static void rproc_disable_iommu(struct rproc
> > *rproc)
> > >         iommu_domain_free(domain);
> > >  }
> > >
> > > -static phys_addr_t rproc_va_to_pa(void *cpu_addr)
> > > +phys_addr_t rproc_va_to_pa(void *cpu_addr)
> > >  {
> > >         /*
> > >          * Return physical address according to virtual address location
> > > @@ -160,6 +161,7 @@ static phys_addr_t rproc_va_to_pa(void
> > *cpu_addr)
> > >         WARN_ON(!virt_addr_valid(cpu_addr));
> > >         return virt_to_phys(cpu_addr);
> > >  }
> > > +EXPORT_SYMBOL(rproc_va_to_pa);
> > >
> > >  /**
> > >   * rproc_da_to_va() - lookup the kernel virtual address for a remoteproc
> > address
> > > @@ -423,6 +425,20 @@ static void rproc_vdev_do_stop(struct
> > rproc_subdev *subdev, bool crashed)
> > >  }
> > >
> > >  /**
> > > + * rproc_rvdev_release() - release the existence of a rvdev
> > > + *
> > > + * @dev: the subdevice's dev
> > > + */
> > > +static void rproc_rvdev_release(struct device *dev)
> > > +{
> > > +       struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev, dev);
> > > +
> > > +       of_reserved_mem_device_release(dev);
> > > +
> > > +       kfree(rvdev);
> > > +}
> > > +
> > > +/**
> > >   * rproc_handle_vdev() - handle a vdev fw resource
> > >   * @rproc: the remote processor
> > >   * @rsc: the vring resource descriptor
> > > @@ -455,6 +471,7 @@ static int rproc_handle_vdev(struct rproc *rproc,
> > struct fw_rsc_vdev *rsc,
> > >         struct device *dev = &rproc->dev;
> > >         struct rproc_vdev *rvdev;
> > >         int i, ret;
> > > +       char name[16];
> > >
> > >         /* make sure resource isn't truncated */
> > >         if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct
> > fw_rsc_vdev_vring)
> > > @@ -488,6 +505,18 @@ static int rproc_handle_vdev(struct rproc *rproc,
> > struct fw_rsc_vdev *rsc,
> > >         rvdev->rproc = rproc;
> > >         rvdev->index = rproc->nb_vdev++;
> > >
> > > +       /* Initialise vdev subdevice */
> > > +       snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
> > > +       rvdev->dev.parent = rproc->dev.parent;
> > > +       rvdev->dev.release = rproc_rvdev_release;
> > > +       dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev-
> > >dev.parent), name);
> > > +       dev_set_drvdata(&rvdev->dev, rvdev);
> > > +       dma_set_coherent_mask(&rvdev->dev, DMA_BIT_MASK(32));
> > I tried the latest kernel, this function will not set the DMA coherent mask as
> > dma_supported() of the &rvdev->dev will return false.
> > As this is a device created at run time, should it be force to support DMA?
> > should it directly set the dma_coherent_mask?
>
> Thanks for pointing me this issue. I tested on top of 4.18-rc1 few months ago...
> Could you please give me kernel version on which you are testing the series?
> Is you platform 32bit or 64bit ?
> I'll rebase and check on my side.

I am testing with 4.19-rc4 on aarch64 platform.

Best Regards,
Wendy
>
> Regards,
> Loic
>
> >
> > > +
> > > +       ret = device_register(&rvdev->dev);
> > > +       if (ret)
> > > +               goto free_rvdev;
> > > +
> > >         /* parse the vrings */
> > >         for (i = 0; i < rsc->num_of_vrings; i++) {
> > >                 ret = rproc_parse_vring(rvdev, rsc, i);
> > > @@ -518,7 +547,7 @@ static int rproc_handle_vdev(struct rproc *rproc,
> > struct fw_rsc_vdev *rsc,
> > >         for (i--; i >= 0; i--)
> > >                 rproc_free_vring(&rvdev->vring[i]);
> > >  free_rvdev:
> > > -       kfree(rvdev);
> > > +       device_unregister(&rvdev->dev);
> > >         return ret;
> > >  }
> > >
> > > @@ -536,7 +565,7 @@ void rproc_vdev_release(struct kref *ref)
> > >
> > >         rproc_remove_subdev(rproc, &rvdev->subdev);
> > >         list_del(&rvdev->node);
> > > -       kfree(rvdev);
> > > +       device_unregister(&rvdev->dev);
> > >  }
> > >
> > >  /**
> > > diff --git a/drivers/remoteproc/remoteproc_internal.h
> > b/drivers/remoteproc/remoteproc_internal.h
> > > index f6cad24..bfeacfd 100644
> > > --- a/drivers/remoteproc/remoteproc_internal.h
> > > +++ b/drivers/remoteproc/remoteproc_internal.h
> > > @@ -52,6 +52,7 @@ struct dentry *rproc_create_trace_file(const char
> > *name, struct rproc *rproc,
> > >  int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
> > >
> > >  void *rproc_da_to_va(struct rproc *rproc, u64 da, int len);
> > > +phys_addr_t rproc_va_to_pa(void *cpu_addr);
> > >  int rproc_trigger_recovery(struct rproc *rproc);
> > >
> > >  int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw);
> > > diff --git a/drivers/remoteproc/remoteproc_virtio.c
> > b/drivers/remoteproc/remoteproc_virtio.c
> > > index de21f62..9ee63c0 100644
> > > --- a/drivers/remoteproc/remoteproc_virtio.c
> > > +++ b/drivers/remoteproc/remoteproc_virtio.c
> > > @@ -17,7 +17,9 @@
> > >   * GNU General Public License for more details.
> > >   */
> > >
> > > +#include <linux/dma-mapping.h>
> > >  #include <linux/export.h>
> > > +#include <linux/of_reserved_mem.h>
> > >  #include <linux/remoteproc.h>
> > >  #include <linux/virtio.h>
> > >  #include <linux/virtio_config.h>
> > > @@ -315,10 +317,48 @@ static void rproc_virtio_dev_release(struct device
> > *dev)
> > >  int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
> > >  {
> > >         struct rproc *rproc = rvdev->rproc;
> > > -       struct device *dev = &rproc->dev;
> > > +       struct device *dev = &rvdev->dev;
> > >         struct virtio_device *vdev = &rvdev->vdev;
> > > +       struct rproc_mem_entry *mem;
> > >         int ret;
> > >
> > > +       /* Try to find dedicated vdev buffer carveout */
> > > +       mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer", rvdev-
> > >index);
> > > +       if (mem) {
> > > +               phys_addr_t pa;
> > > +
> > > +               if (mem->of_resm_idx != -1) {
> > > +                       struct device_node *np = rproc->dev.parent->of_node;
> > > +
> > > +                       /* Associate reserved memory to vdev device */
> > > +                       ret = of_reserved_mem_device_init_by_idx(dev, np,
> > > +                                                                mem->of_resm_idx);
> > > +                       if (ret) {
> > > +                               dev_err(dev, "Can't associate reserved memory\n");
> > > +                               goto out;
> > > +                       }
> > > +               } else {
> > > +                       if (mem->va) {
> > > +                               dev_warn(dev, "vdev %d buffer already mapped\n",
> > > +                                        rvdev->index);
> > > +                               pa = rproc_va_to_pa(mem->va);
> > > +                       } else {
> > > +                               /* Use dma address as carveout no memmapped yet */
> > > +                               pa = (phys_addr_t)mem->dma;
> > > +                       }
> > > +
> > > +                       /* Associate vdev buffer memory pool to vdev subdev */
> > > +                       ret = dmam_declare_coherent_memory(dev, pa,
> > > +                                                          mem->da,
> > > +                                                          mem->len,
> > > +                                                          DMA_MEMORY_EXCLUSIVE);
> > > +                       if (ret < 0) {
> > > +                               dev_err(dev, "Failed to associate buffer\n");
> > > +                               goto out;
> > > +                       }
> > > +               }
> > > +       }
> > > +
> > >         vdev->id.device = id,
> > >         vdev->config = &rproc_virtio_config_ops,
> > >         vdev->dev.parent = dev;
> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > > index 6b3a234..2921dd2 100644
> > > --- a/include/linux/remoteproc.h
> > > +++ b/include/linux/remoteproc.h
> > > @@ -547,6 +547,7 @@ struct rproc_vdev {
> > >         struct kref refcount;
> > >
> > >         struct rproc_subdev subdev;
> > > +       struct device dev;
> > >
> > >         unsigned int id;
> > >         struct list_head node;
> > > --
> > > 1.9.1
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-remoteproc"
> > in
> > > the body of a message to [e-mail]
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Andersson Oct. 10, 2018, 5:58 a.m. UTC | #4

On Fri 27 Jul 06:14 PDT 2018, Loic Pallardy wrote:

> This patch creates a dedicated vdev subdevice for each vdev declared
> in firmware resource table and associates carveout named "vdev%dbuffer"
> (with %d vdev index in resource table) if any as dma coherent memory pool.
> 
> Then vdev subdevice is used as parent for virtio device.
> 
> Signed-off-by: Loic Pallardy <[e-mail]>
> ---
>  drivers/remoteproc/remoteproc_core.c     | 35 +++++++++++++++++++++++---
>  drivers/remoteproc/remoteproc_internal.h |  1 +
>  drivers/remoteproc/remoteproc_virtio.c   | 42 +++++++++++++++++++++++++++++++-
>  include/linux/remoteproc.h               |  1 +
>  4 files changed, 75 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 4edc6f0..adcc66e 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -39,6 +39,7 @@
>  #include <linux/idr.h>
>  #include <linux/elf.h>
>  #include <linux/crc32.h>
> +#include <linux/of_reserved_mem.h>
>  #include <linux/virtio_ids.h>
>  #include <linux/virtio_ring.h>
>  #include <asm/byteorder.h>
> @@ -145,7 +146,7 @@ static void rproc_disable_iommu(struct rproc *rproc)
>  	iommu_domain_free(domain);
>  }
>  
> -static phys_addr_t rproc_va_to_pa(void *cpu_addr)
> +phys_addr_t rproc_va_to_pa(void *cpu_addr)
>  {
>  	/*
>  	 * Return physical address according to virtual address location
> @@ -160,6 +161,7 @@ static phys_addr_t rproc_va_to_pa(void *cpu_addr)
>  	WARN_ON(!virt_addr_valid(cpu_addr));
>  	return virt_to_phys(cpu_addr);
>  }
> +EXPORT_SYMBOL(rproc_va_to_pa);
>  
>  /**
>   * rproc_da_to_va() - lookup the kernel virtual address for a remoteproc address
> @@ -423,6 +425,20 @@ static void rproc_vdev_do_stop(struct rproc_subdev *subdev, bool crashed)
>  }
>  
>  /**
> + * rproc_rvdev_release() - release the existence of a rvdev
> + *
> + * @dev: the subdevice's dev
> + */
> +static void rproc_rvdev_release(struct device *dev)
> +{
> +	struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev, dev);
> +
> +	of_reserved_mem_device_release(dev);
> +
> +	kfree(rvdev);
> +}
> +
> +/**
>   * rproc_handle_vdev() - handle a vdev fw resource
>   * @rproc: the remote processor
>   * @rsc: the vring resource descriptor
> @@ -455,6 +471,7 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
>  	struct device *dev = &rproc->dev;
>  	struct rproc_vdev *rvdev;
>  	int i, ret;
> +	char name[16];
>  
>  	/* make sure resource isn't truncated */
>  	if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct fw_rsc_vdev_vring)
> @@ -488,6 +505,18 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
>  	rvdev->rproc = rproc;
>  	rvdev->index = rproc->nb_vdev++;
>  
> +	/* Initialise vdev subdevice */
> +	snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
> +	rvdev->dev.parent = rproc->dev.parent;
> +	rvdev->dev.release = rproc_rvdev_release;
> +	dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev->dev.parent), name);
> +	dev_set_drvdata(&rvdev->dev, rvdev);
> +	dma_set_coherent_mask(&rvdev->dev, DMA_BIT_MASK(32));
> +
> +	ret = device_register(&rvdev->dev);
> +	if (ret)
> +		goto free_rvdev;
> +
>  	/* parse the vrings */
>  	for (i = 0; i < rsc->num_of_vrings; i++) {
>  		ret = rproc_parse_vring(rvdev, rsc, i);
> @@ -518,7 +547,7 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
>  	for (i--; i >= 0; i--)
>  		rproc_free_vring(&rvdev->vring[i]);
>  free_rvdev:
> -	kfree(rvdev);
> +	device_unregister(&rvdev->dev);
>  	return ret;
>  }
>  
> @@ -536,7 +565,7 @@ void rproc_vdev_release(struct kref *ref)
>  
>  	rproc_remove_subdev(rproc, &rvdev->subdev);
>  	list_del(&rvdev->node);
> -	kfree(rvdev);
> +	device_unregister(&rvdev->dev);
>  }
>  
>  /**
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index f6cad24..bfeacfd 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -52,6 +52,7 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
>  int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
>  
>  void *rproc_da_to_va(struct rproc *rproc, u64 da, int len);
> +phys_addr_t rproc_va_to_pa(void *cpu_addr);
>  int rproc_trigger_recovery(struct rproc *rproc);
>  
>  int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw);
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index de21f62..9ee63c0 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -17,7 +17,9 @@
>   * GNU General Public License for more details.
>   */
>  
> +#include <linux/dma-mapping.h>
>  #include <linux/export.h>
> +#include <linux/of_reserved_mem.h>
>  #include <linux/remoteproc.h>
>  #include <linux/virtio.h>
>  #include <linux/virtio_config.h>
> @@ -315,10 +317,48 @@ static void rproc_virtio_dev_release(struct device *dev)
>  int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>  {
>  	struct rproc *rproc = rvdev->rproc;
> -	struct device *dev = &rproc->dev;
> +	struct device *dev = &rvdev->dev;
>  	struct virtio_device *vdev = &rvdev->vdev;
> +	struct rproc_mem_entry *mem;
>  	int ret;
>  
> +	/* Try to find dedicated vdev buffer carveout */
> +	mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer", rvdev->index);
> +	if (mem) {
> +		phys_addr_t pa;
> +
> +		if (mem->of_resm_idx != -1) {
> +			struct device_node *np = rproc->dev.parent->of_node;
> +
> +			/* Associate reserved memory to vdev device */
> +			ret = of_reserved_mem_device_init_by_idx(dev, np,
> +								 mem->of_resm_idx);
> +			if (ret) {
> +				dev_err(dev, "Can't associate reserved memory\n");
> +				goto out;
> +			}
> +		} else {
> +			if (mem->va) {
> +				dev_warn(dev, "vdev %d buffer already mapped\n",
> +					 rvdev->index);
> +				pa = rproc_va_to_pa(mem->va);
> +			} else {
> +				/* Use dma address as carveout no memmapped yet */
> +				pa = (phys_addr_t)mem->dma;
> +			}
> +
> +			/* Associate vdev buffer memory pool to vdev subdev */
> +			ret = dmam_declare_coherent_memory(dev, pa,
> +							   mem->da,
> +							   mem->len,
> +							   DMA_MEMORY_EXCLUSIVE);

Is it not possible to associate the dma_mem with vdev->dev directly,
instead of injecting yet another device in-between the remoteproc
platform device and the virtio device?

That way the resulting allocation in e.g. virtio_rpmsg would be on the
virtio device itself, not its parent.

Regards,
Bjorn

> +			if (ret < 0) {
> +				dev_err(dev, "Failed to associate buffer\n");
> +				goto out;
> +			}
> +		}
> +	}
> +
>  	vdev->id.device	= id,
>  	vdev->config = &rproc_virtio_config_ops,
>  	vdev->dev.parent = dev;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 6b3a234..2921dd2 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -547,6 +547,7 @@ struct rproc_vdev {
>  	struct kref refcount;
>  
>  	struct rproc_subdev subdev;
> +	struct device dev;
>  
>  	unsigned int id;
>  	struct list_head node;
> -- 
> 1.9.1
>

Loic PALLARDY Oct. 10, 2018, 7:17 p.m. UTC | #5

> -----Original Message-----
> From: Bjorn Andersson [mailto:[e-mail]]
> Sent: mercredi 10 octobre 2018 07:58
> To: Loic PALLARDY <[e-mail]>
> Cc: [e-mail]; [e-mail]; linux-
> [e-mail]; Arnaud POULIQUEN <[e-mail]>;
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with
> specific dma memory pool
> 
> On Fri 27 Jul 06:14 PDT 2018, Loic Pallardy wrote:
> 
> > This patch creates a dedicated vdev subdevice for each vdev declared
> > in firmware resource table and associates carveout named "vdev%dbuffer"
> > (with %d vdev index in resource table) if any as dma coherent memory
> pool.
> >
> > Then vdev subdevice is used as parent for virtio device.
> >
> > Signed-off-by: Loic Pallardy <[e-mail]>
> > ---
> >  drivers/remoteproc/remoteproc_core.c     | 35
> +++++++++++++++++++++++---
> >  drivers/remoteproc/remoteproc_internal.h |  1 +
> >  drivers/remoteproc/remoteproc_virtio.c   | 42
> +++++++++++++++++++++++++++++++-
> >  include/linux/remoteproc.h               |  1 +
> >  4 files changed, 75 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> > index 4edc6f0..adcc66e 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -39,6 +39,7 @@
> >  #include <linux/idr.h>
> >  #include <linux/elf.h>
> >  #include <linux/crc32.h>
> > +#include <linux/of_reserved_mem.h>
> >  #include <linux/virtio_ids.h>
> >  #include <linux/virtio_ring.h>
> >  #include <asm/byteorder.h>
> > @@ -145,7 +146,7 @@ static void rproc_disable_iommu(struct rproc
> *rproc)
> >  	iommu_domain_free(domain);
> >  }
> >
> > -static phys_addr_t rproc_va_to_pa(void *cpu_addr)
> > +phys_addr_t rproc_va_to_pa(void *cpu_addr)
> >  {
> >  	/*
> >  	 * Return physical address according to virtual address location
> > @@ -160,6 +161,7 @@ static phys_addr_t rproc_va_to_pa(void
> *cpu_addr)
> >  	WARN_ON(!virt_addr_valid(cpu_addr));
> >  	return virt_to_phys(cpu_addr);
> >  }
> > +EXPORT_SYMBOL(rproc_va_to_pa);
> >
> >  /**
> >   * rproc_da_to_va() - lookup the kernel virtual address for a remoteproc
> address
> > @@ -423,6 +425,20 @@ static void rproc_vdev_do_stop(struct
> rproc_subdev *subdev, bool crashed)
> >  }
> >
> >  /**
> > + * rproc_rvdev_release() - release the existence of a rvdev
> > + *
> > + * @dev: the subdevice's dev
> > + */
> > +static void rproc_rvdev_release(struct device *dev)
> > +{
> > +	struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev,
> dev);
> > +
> > +	of_reserved_mem_device_release(dev);
> > +
> > +	kfree(rvdev);
> > +}
> > +
> > +/**
> >   * rproc_handle_vdev() - handle a vdev fw resource
> >   * @rproc: the remote processor
> >   * @rsc: the vring resource descriptor
> > @@ -455,6 +471,7 @@ static int rproc_handle_vdev(struct rproc *rproc,
> struct fw_rsc_vdev *rsc,
> >  	struct device *dev = &rproc->dev;
> >  	struct rproc_vdev *rvdev;
> >  	int i, ret;
> > +	char name[16];
> >
> >  	/* make sure resource isn't truncated */
> >  	if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct
> fw_rsc_vdev_vring)
> > @@ -488,6 +505,18 @@ static int rproc_handle_vdev(struct rproc *rproc,
> struct fw_rsc_vdev *rsc,
> >  	rvdev->rproc = rproc;
> >  	rvdev->index = rproc->nb_vdev++;
> >
> > +	/* Initialise vdev subdevice */
> > +	snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
> > +	rvdev->dev.parent = rproc->dev.parent;
> > +	rvdev->dev.release = rproc_rvdev_release;
> > +	dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev-
> >dev.parent), name);
> > +	dev_set_drvdata(&rvdev->dev, rvdev);
> > +	dma_set_coherent_mask(&rvdev->dev, DMA_BIT_MASK(32));
> > +
> > +	ret = device_register(&rvdev->dev);
> > +	if (ret)
> > +		goto free_rvdev;
> > +
> >  	/* parse the vrings */
> >  	for (i = 0; i < rsc->num_of_vrings; i++) {
> >  		ret = rproc_parse_vring(rvdev, rsc, i);
> > @@ -518,7 +547,7 @@ static int rproc_handle_vdev(struct rproc *rproc,
> struct fw_rsc_vdev *rsc,
> >  	for (i--; i >= 0; i--)
> >  		rproc_free_vring(&rvdev->vring[i]);
> >  free_rvdev:
> > -	kfree(rvdev);
> > +	device_unregister(&rvdev->dev);
> >  	return ret;
> >  }
> >
> > @@ -536,7 +565,7 @@ void rproc_vdev_release(struct kref *ref)
> >
> >  	rproc_remove_subdev(rproc, &rvdev->subdev);
> >  	list_del(&rvdev->node);
> > -	kfree(rvdev);
> > +	device_unregister(&rvdev->dev);
> >  }
> >
> >  /**
> > diff --git a/drivers/remoteproc/remoteproc_internal.h
> b/drivers/remoteproc/remoteproc_internal.h
> > index f6cad24..bfeacfd 100644
> > --- a/drivers/remoteproc/remoteproc_internal.h
> > +++ b/drivers/remoteproc/remoteproc_internal.h
> > @@ -52,6 +52,7 @@ struct dentry *rproc_create_trace_file(const char
> *name, struct rproc *rproc,
> >  int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
> >
> >  void *rproc_da_to_va(struct rproc *rproc, u64 da, int len);
> > +phys_addr_t rproc_va_to_pa(void *cpu_addr);
> >  int rproc_trigger_recovery(struct rproc *rproc);
> >
> >  int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw);
> > diff --git a/drivers/remoteproc/remoteproc_virtio.c
> b/drivers/remoteproc/remoteproc_virtio.c
> > index de21f62..9ee63c0 100644
> > --- a/drivers/remoteproc/remoteproc_virtio.c
> > +++ b/drivers/remoteproc/remoteproc_virtio.c
> > @@ -17,7 +17,9 @@
> >   * GNU General Public License for more details.
> >   */
> >
> > +#include <linux/dma-mapping.h>
> >  #include <linux/export.h>
> > +#include <linux/of_reserved_mem.h>
> >  #include <linux/remoteproc.h>
> >  #include <linux/virtio.h>
> >  #include <linux/virtio_config.h>
> > @@ -315,10 +317,48 @@ static void rproc_virtio_dev_release(struct device
> *dev)
> >  int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
> >  {
> >  	struct rproc *rproc = rvdev->rproc;
> > -	struct device *dev = &rproc->dev;
> > +	struct device *dev = &rvdev->dev;
> >  	struct virtio_device *vdev = &rvdev->vdev;
> > +	struct rproc_mem_entry *mem;
> >  	int ret;
> >
> > +	/* Try to find dedicated vdev buffer carveout */
> > +	mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer",
> rvdev->index);
> > +	if (mem) {
> > +		phys_addr_t pa;
> > +
> > +		if (mem->of_resm_idx != -1) {
> > +			struct device_node *np = rproc->dev.parent-
> >of_node;
> > +
> > +			/* Associate reserved memory to vdev device */
> > +			ret = of_reserved_mem_device_init_by_idx(dev, np,
> > +								 mem-
> >of_resm_idx);
> > +			if (ret) {
> > +				dev_err(dev, "Can't associate reserved
> memory\n");
> > +				goto out;
> > +			}
> > +		} else {
> > +			if (mem->va) {
> > +				dev_warn(dev, "vdev %d buffer already
> mapped\n",
> > +					 rvdev->index);
> > +				pa = rproc_va_to_pa(mem->va);
> > +			} else {
> > +				/* Use dma address as carveout no
> memmapped yet */
> > +				pa = (phys_addr_t)mem->dma;
> > +			}
> > +
> > +			/* Associate vdev buffer memory pool to vdev
> subdev */
> > +			ret = dmam_declare_coherent_memory(dev, pa,
> > +							   mem->da,
> > +							   mem->len,
> > +
> DMA_MEMORY_EXCLUSIVE);
> 
> Is it not possible to associate the dma_mem with vdev->dev directly,
> instead of injecting yet another device in-between the remoteproc
> platform device and the virtio device?

Thank you for the review.
I'll check how to use virtio device. An additional ops for memory registration may be needed at virtio level. 

> 
> That way the resulting allocation in e.g. virtio_rpmsg would be on the
> virtio device itself, not its parent.
Yes agree, it will simplify allocation/inheritance...

Regards,
Loic
> 
> Regards,
> Bjorn
> 
> > +			if (ret < 0) {
> > +				dev_err(dev, "Failed to associate buffer\n");
> > +				goto out;
> > +			}
> > +		}
> > +	}
> > +
> >  	vdev->id.device	= id,
> >  	vdev->config = &rproc_virtio_config_ops,
> >  	vdev->dev.parent = dev;
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index 6b3a234..2921dd2 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -547,6 +547,7 @@ struct rproc_vdev {
> >  	struct kref refcount;
> >
> >  	struct rproc_subdev subdev;
> > +	struct device dev;
> >
> >  	unsigned int id;
> >  	struct list_head node;
> > --
> > 1.9.1
> >

Suman Anna Oct. 24, 2018, 1:22 a.m. UTC | #6

On 9/27/18 3:18 PM, Wendy Liang wrote:
> Hi Loic,
> 
> 
> On Thu, Sep 27, 2018 at 12:22 PM Loic PALLARDY <[e-mail]> wrote:
>>
>> Hi Wendy
>>
>>> -----Original Message-----
>>> From: Wendy Liang <[e-mail]>
>>> Sent: Thursday, September 27, 2018 7:17 PM
>>> To: Loic PALLARDY <[e-mail]>
>>> Cc: Bjorn Andersson <[e-mail]>; Ohad Ben-Cohen
>>> <[e-mail]>; [e-mail]; Linux Kernel
>>> Mailing List <[e-mail]>; Arnaud POULIQUEN
>>> <[e-mail]>; [e-mail]; Suman Anna
>>> <[e-mail]>
>>> Subject: Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with
>>> specific dma memory pool
>>>
>>> On Fri, Jul 27, 2018 at 6:16 AM Loic Pallardy <[e-mail]> wrote:
>>>>
>>>> This patch creates a dedicated vdev subdevice for each vdev declared
>>>> in firmware resource table and associates carveout named "vdev%dbuffer"
>>>> (with %d vdev index in resource table) if any as dma coherent memory
>>> pool.
>>>>
>>>> Then vdev subdevice is used as parent for virtio device.
>>>>
>>>> Signed-off-by: Loic Pallardy <[e-mail]>
>>>> ---
>>>>  drivers/remoteproc/remoteproc_core.c     | 35
>>> +++++++++++++++++++++++---
>>>>  drivers/remoteproc/remoteproc_internal.h |  1 +
>>>>  drivers/remoteproc/remoteproc_virtio.c   | 42
>>> +++++++++++++++++++++++++++++++-
>>>>  include/linux/remoteproc.h               |  1 +
>>>>  4 files changed, 75 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>>> b/drivers/remoteproc/remoteproc_core.c
>>>> index 4edc6f0..adcc66e 100644
>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>> @@ -39,6 +39,7 @@
>>>>  #include <linux/idr.h>
>>>>  #include <linux/elf.h>
>>>>  #include <linux/crc32.h>
>>>> +#include <linux/of_reserved_mem.h>
>>>>  #include <linux/virtio_ids.h>
>>>>  #include <linux/virtio_ring.h>
>>>>  #include <asm/byteorder.h>
>>>> @@ -145,7 +146,7 @@ static void rproc_disable_iommu(struct rproc
>>> *rproc)
>>>>         iommu_domain_free(domain);
>>>>  }
>>>>
>>>> -static phys_addr_t rproc_va_to_pa(void *cpu_addr)
>>>> +phys_addr_t rproc_va_to_pa(void *cpu_addr)
>>>>  {
>>>>         /*
>>>>          * Return physical address according to virtual address location
>>>> @@ -160,6 +161,7 @@ static phys_addr_t rproc_va_to_pa(void
>>> *cpu_addr)
>>>>         WARN_ON(!virt_addr_valid(cpu_addr));
>>>>         return virt_to_phys(cpu_addr);
>>>>  }
>>>> +EXPORT_SYMBOL(rproc_va_to_pa);
>>>>
>>>>  /**
>>>>   * rproc_da_to_va() - lookup the kernel virtual address for a remoteproc
>>> address
>>>> @@ -423,6 +425,20 @@ static void rproc_vdev_do_stop(struct
>>> rproc_subdev *subdev, bool crashed)
>>>>  }
>>>>
>>>>  /**
>>>> + * rproc_rvdev_release() - release the existence of a rvdev
>>>> + *
>>>> + * @dev: the subdevice's dev
>>>> + */
>>>> +static void rproc_rvdev_release(struct device *dev)
>>>> +{
>>>> +       struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev, dev);
>>>> +
>>>> +       of_reserved_mem_device_release(dev);
>>>> +
>>>> +       kfree(rvdev);
>>>> +}
>>>> +
>>>> +/**
>>>>   * rproc_handle_vdev() - handle a vdev fw resource
>>>>   * @rproc: the remote processor
>>>>   * @rsc: the vring resource descriptor
>>>> @@ -455,6 +471,7 @@ static int rproc_handle_vdev(struct rproc *rproc,
>>> struct fw_rsc_vdev *rsc,
>>>>         struct device *dev = &rproc->dev;
>>>>         struct rproc_vdev *rvdev;
>>>>         int i, ret;
>>>> +       char name[16];
>>>>
>>>>         /* make sure resource isn't truncated */
>>>>         if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct
>>> fw_rsc_vdev_vring)
>>>> @@ -488,6 +505,18 @@ static int rproc_handle_vdev(struct rproc *rproc,
>>> struct fw_rsc_vdev *rsc,
>>>>         rvdev->rproc = rproc;
>>>>         rvdev->index = rproc->nb_vdev++;
>>>>
>>>> +       /* Initialise vdev subdevice */
>>>> +       snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
>>>> +       rvdev->dev.parent = rproc->dev.parent;
>>>> +       rvdev->dev.release = rproc_rvdev_release;
>>>> +       dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev-
>>>> dev.parent), name);
>>>> +       dev_set_drvdata(&rvdev->dev, rvdev);
>>>> +       dma_set_coherent_mask(&rvdev->dev, DMA_BIT_MASK(32));
>>> I tried the latest kernel, this function will not set the DMA coherent mask as
>>> dma_supported() of the &rvdev->dev will return false.
>>> As this is a device created at run time, should it be force to support DMA?
>>> should it directly set the dma_coherent_mask?
>>
>> Thanks for pointing me this issue. I tested on top of 4.18-rc1 few months ago...
>> Could you please give me kernel version on which you are testing the series?
>> Is you platform 32bit or 64bit ?
>> I'll rebase and check on my side.
> 
> I am testing with 4.19-rc4 on aarch64 platform.

Btw, I ran into this on my v7 platform as well (4.19-rc6). The
dma_set_coherent_mask fails with error EIO. I did get my allocations
through though.

regards
Suman

> 
> Best Regards,
> Wendy
>>
>> Regards,
>> Loic
>>
>>>
>>>> +
>>>> +       ret = device_register(&rvdev->dev);
>>>> +       if (ret)
>>>> +               goto free_rvdev;
>>>> +
>>>>         /* parse the vrings */
>>>>         for (i = 0; i < rsc->num_of_vrings; i++) {
>>>>                 ret = rproc_parse_vring(rvdev, rsc, i);
>>>> @@ -518,7 +547,7 @@ static int rproc_handle_vdev(struct rproc *rproc,
>>> struct fw_rsc_vdev *rsc,
>>>>         for (i--; i >= 0; i--)
>>>>                 rproc_free_vring(&rvdev->vring[i]);
>>>>  free_rvdev:
>>>> -       kfree(rvdev);
>>>> +       device_unregister(&rvdev->dev);
>>>>         return ret;
>>>>  }
>>>>
>>>> @@ -536,7 +565,7 @@ void rproc_vdev_release(struct kref *ref)
>>>>
>>>>         rproc_remove_subdev(rproc, &rvdev->subdev);
>>>>         list_del(&rvdev->node);
>>>> -       kfree(rvdev);
>>>> +       device_unregister(&rvdev->dev);
>>>>  }
>>>>
>>>>  /**
>>>> diff --git a/drivers/remoteproc/remoteproc_internal.h
>>> b/drivers/remoteproc/remoteproc_internal.h
>>>> index f6cad24..bfeacfd 100644
>>>> --- a/drivers/remoteproc/remoteproc_internal.h
>>>> +++ b/drivers/remoteproc/remoteproc_internal.h
>>>> @@ -52,6 +52,7 @@ struct dentry *rproc_create_trace_file(const char
>>> *name, struct rproc *rproc,
>>>>  int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
>>>>
>>>>  void *rproc_da_to_va(struct rproc *rproc, u64 da, int len);
>>>> +phys_addr_t rproc_va_to_pa(void *cpu_addr);
>>>>  int rproc_trigger_recovery(struct rproc *rproc);
>>>>
>>>>  int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw);
>>>> diff --git a/drivers/remoteproc/remoteproc_virtio.c
>>> b/drivers/remoteproc/remoteproc_virtio.c
>>>> index de21f62..9ee63c0 100644
>>>> --- a/drivers/remoteproc/remoteproc_virtio.c
>>>> +++ b/drivers/remoteproc/remoteproc_virtio.c
>>>> @@ -17,7 +17,9 @@
>>>>   * GNU General Public License for more details.
>>>>   */
>>>>
>>>> +#include <linux/dma-mapping.h>
>>>>  #include <linux/export.h>
>>>> +#include <linux/of_reserved_mem.h>
>>>>  #include <linux/remoteproc.h>
>>>>  #include <linux/virtio.h>
>>>>  #include <linux/virtio_config.h>
>>>> @@ -315,10 +317,48 @@ static void rproc_virtio_dev_release(struct device
>>> *dev)
>>>>  int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>>>>  {
>>>>         struct rproc *rproc = rvdev->rproc;
>>>> -       struct device *dev = &rproc->dev;
>>>> +       struct device *dev = &rvdev->dev;
>>>>         struct virtio_device *vdev = &rvdev->vdev;
>>>> +       struct rproc_mem_entry *mem;
>>>>         int ret;
>>>>
>>>> +       /* Try to find dedicated vdev buffer carveout */
>>>> +       mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer", rvdev-
>>>> index);
>>>> +       if (mem) {
>>>> +               phys_addr_t pa;
>>>> +
>>>> +               if (mem->of_resm_idx != -1) {
>>>> +                       struct device_node *np = rproc->dev.parent->of_node;
>>>> +
>>>> +                       /* Associate reserved memory to vdev device */
>>>> +                       ret = of_reserved_mem_device_init_by_idx(dev, np,
>>>> +                                                                mem->of_resm_idx);
>>>> +                       if (ret) {
>>>> +                               dev_err(dev, "Can't associate reserved memory\n");
>>>> +                               goto out;
>>>> +                       }
>>>> +               } else {
>>>> +                       if (mem->va) {
>>>> +                               dev_warn(dev, "vdev %d buffer already mapped\n",
>>>> +                                        rvdev->index);
>>>> +                               pa = rproc_va_to_pa(mem->va);
>>>> +                       } else {
>>>> +                               /* Use dma address as carveout no memmapped yet */
>>>> +                               pa = (phys_addr_t)mem->dma;
>>>> +                       }
>>>> +
>>>> +                       /* Associate vdev buffer memory pool to vdev subdev */
>>>> +                       ret = dmam_declare_coherent_memory(dev, pa,
>>>> +                                                          mem->da,
>>>> +                                                          mem->len,
>>>> +                                                          DMA_MEMORY_EXCLUSIVE);
>>>> +                       if (ret < 0) {
>>>> +                               dev_err(dev, "Failed to associate buffer\n");
>>>> +                               goto out;
>>>> +                       }
>>>> +               }
>>>> +       }
>>>> +
>>>>         vdev->id.device = id,
>>>>         vdev->config = &rproc_virtio_config_ops,
>>>>         vdev->dev.parent = dev;
>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>> index 6b3a234..2921dd2 100644
>>>> --- a/include/linux/remoteproc.h
>>>> +++ b/include/linux/remoteproc.h
>>>> @@ -547,6 +547,7 @@ struct rproc_vdev {
>>>>         struct kref refcount;
>>>>
>>>>         struct rproc_subdev subdev;
>>>> +       struct device dev;
>>>>
>>>>         unsigned int id;
>>>>         struct list_head node;
>>>> --
>>>> 1.9.1
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc"
>>> in
>>>> the body of a message to [e-mail]
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Suman Anna Oct. 24, 2018, 1:27 a.m. UTC | #7

On 10/10/18 2:17 PM, Loic PALLARDY wrote:
> 
> 
>> -----Original Message-----
>> From: Bjorn Andersson [mailto:[e-mail]]
>> Sent: mercredi 10 octobre 2018 07:58
>> To: Loic PALLARDY <[e-mail]>
>> Cc: [e-mail]; [e-mail]; linux-
>> [e-mail]; Arnaud POULIQUEN <[e-mail]>;
>> [e-mail]; [e-mail]
>> Subject: Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with
>> specific dma memory pool
>>
>> On Fri 27 Jul 06:14 PDT 2018, Loic Pallardy wrote:
>>
>>> This patch creates a dedicated vdev subdevice for each vdev declared
>>> in firmware resource table and associates carveout named "vdev%dbuffer"
>>> (with %d vdev index in resource table) if any as dma coherent memory
>> pool.
>>>
>>> Then vdev subdevice is used as parent for virtio device.
>>>
>>> Signed-off-by: Loic Pallardy <[e-mail]>
>>> ---
>>>  drivers/remoteproc/remoteproc_core.c     | 35
>> +++++++++++++++++++++++---
>>>  drivers/remoteproc/remoteproc_internal.h |  1 +
>>>  drivers/remoteproc/remoteproc_virtio.c   | 42
>> +++++++++++++++++++++++++++++++-
>>>  include/linux/remoteproc.h               |  1 +
>>>  4 files changed, 75 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>> b/drivers/remoteproc/remoteproc_core.c
>>> index 4edc6f0..adcc66e 100644
>>> --- a/drivers/remoteproc/remoteproc_core.c
>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>> @@ -39,6 +39,7 @@
>>>  #include <linux/idr.h>
>>>  #include <linux/elf.h>
>>>  #include <linux/crc32.h>
>>> +#include <linux/of_reserved_mem.h>
>>>  #include <linux/virtio_ids.h>
>>>  #include <linux/virtio_ring.h>
>>>  #include <asm/byteorder.h>
>>> @@ -145,7 +146,7 @@ static void rproc_disable_iommu(struct rproc
>> *rproc)
>>>  	iommu_domain_free(domain);
>>>  }
>>>
>>> -static phys_addr_t rproc_va_to_pa(void *cpu_addr)
>>> +phys_addr_t rproc_va_to_pa(void *cpu_addr)
>>>  {
>>>  	/*
>>>  	 * Return physical address according to virtual address location
>>> @@ -160,6 +161,7 @@ static phys_addr_t rproc_va_to_pa(void
>> *cpu_addr)
>>>  	WARN_ON(!virt_addr_valid(cpu_addr));
>>>  	return virt_to_phys(cpu_addr);
>>>  }
>>> +EXPORT_SYMBOL(rproc_va_to_pa);
>>>
>>>  /**
>>>   * rproc_da_to_va() - lookup the kernel virtual address for a remoteproc
>> address
>>> @@ -423,6 +425,20 @@ static void rproc_vdev_do_stop(struct
>> rproc_subdev *subdev, bool crashed)
>>>  }
>>>
>>>  /**
>>> + * rproc_rvdev_release() - release the existence of a rvdev
>>> + *
>>> + * @dev: the subdevice's dev
>>> + */
>>> +static void rproc_rvdev_release(struct device *dev)
>>> +{
>>> +	struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev,
>> dev);
>>> +
>>> +	of_reserved_mem_device_release(dev);
>>> +
>>> +	kfree(rvdev);
>>> +}
>>> +
>>> +/**
>>>   * rproc_handle_vdev() - handle a vdev fw resource
>>>   * @rproc: the remote processor
>>>   * @rsc: the vring resource descriptor
>>> @@ -455,6 +471,7 @@ static int rproc_handle_vdev(struct rproc *rproc,
>> struct fw_rsc_vdev *rsc,
>>>  	struct device *dev = &rproc->dev;
>>>  	struct rproc_vdev *rvdev;
>>>  	int i, ret;
>>> +	char name[16];
>>>
>>>  	/* make sure resource isn't truncated */
>>>  	if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct
>> fw_rsc_vdev_vring)
>>> @@ -488,6 +505,18 @@ static int rproc_handle_vdev(struct rproc *rproc,
>> struct fw_rsc_vdev *rsc,
>>>  	rvdev->rproc = rproc;
>>>  	rvdev->index = rproc->nb_vdev++;
>>>
>>> +	/* Initialise vdev subdevice */
>>> +	snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
>>> +	rvdev->dev.parent = rproc->dev.parent;
>>> +	rvdev->dev.release = rproc_rvdev_release;
>>> +	dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev-
>>> dev.parent), name);
>>> +	dev_set_drvdata(&rvdev->dev, rvdev);
>>> +	dma_set_coherent_mask(&rvdev->dev, DMA_BIT_MASK(32));
>>> +
>>> +	ret = device_register(&rvdev->dev);
>>> +	if (ret)
>>> +		goto free_rvdev;
>>> +
>>>  	/* parse the vrings */
>>>  	for (i = 0; i < rsc->num_of_vrings; i++) {
>>>  		ret = rproc_parse_vring(rvdev, rsc, i);
>>> @@ -518,7 +547,7 @@ static int rproc_handle_vdev(struct rproc *rproc,
>> struct fw_rsc_vdev *rsc,
>>>  	for (i--; i >= 0; i--)
>>>  		rproc_free_vring(&rvdev->vring[i]);
>>>  free_rvdev:
>>> -	kfree(rvdev);
>>> +	device_unregister(&rvdev->dev);
>>>  	return ret;
>>>  }
>>>
>>> @@ -536,7 +565,7 @@ void rproc_vdev_release(struct kref *ref)
>>>
>>>  	rproc_remove_subdev(rproc, &rvdev->subdev);
>>>  	list_del(&rvdev->node);
>>> -	kfree(rvdev);
>>> +	device_unregister(&rvdev->dev);
>>>  }
>>>
>>>  /**
>>> diff --git a/drivers/remoteproc/remoteproc_internal.h
>> b/drivers/remoteproc/remoteproc_internal.h
>>> index f6cad24..bfeacfd 100644
>>> --- a/drivers/remoteproc/remoteproc_internal.h
>>> +++ b/drivers/remoteproc/remoteproc_internal.h
>>> @@ -52,6 +52,7 @@ struct dentry *rproc_create_trace_file(const char
>> *name, struct rproc *rproc,
>>>  int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
>>>
>>>  void *rproc_da_to_va(struct rproc *rproc, u64 da, int len);
>>> +phys_addr_t rproc_va_to_pa(void *cpu_addr);
>>>  int rproc_trigger_recovery(struct rproc *rproc);
>>>
>>>  int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw);
>>> diff --git a/drivers/remoteproc/remoteproc_virtio.c
>> b/drivers/remoteproc/remoteproc_virtio.c
>>> index de21f62..9ee63c0 100644
>>> --- a/drivers/remoteproc/remoteproc_virtio.c
>>> +++ b/drivers/remoteproc/remoteproc_virtio.c
>>> @@ -17,7 +17,9 @@
>>>   * GNU General Public License for more details.
>>>   */
>>>
>>> +#include <linux/dma-mapping.h>
>>>  #include <linux/export.h>
>>> +#include <linux/of_reserved_mem.h>
>>>  #include <linux/remoteproc.h>
>>>  #include <linux/virtio.h>
>>>  #include <linux/virtio_config.h>
>>> @@ -315,10 +317,48 @@ static void rproc_virtio_dev_release(struct device
>> *dev)
>>>  int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>>>  {
>>>  	struct rproc *rproc = rvdev->rproc;
>>> -	struct device *dev = &rproc->dev;
>>> +	struct device *dev = &rvdev->dev;
>>>  	struct virtio_device *vdev = &rvdev->vdev;
>>> +	struct rproc_mem_entry *mem;
>>>  	int ret;
>>>
>>> +	/* Try to find dedicated vdev buffer carveout */
>>> +	mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer",
>> rvdev->index);
>>> +	if (mem) {
>>> +		phys_addr_t pa;
>>> +
>>> +		if (mem->of_resm_idx != -1) {
>>> +			struct device_node *np = rproc->dev.parent-
>>> of_node;
>>> +
>>> +			/* Associate reserved memory to vdev device */
>>> +			ret = of_reserved_mem_device_init_by_idx(dev, np,
>>> +								 mem-
>>> of_resm_idx);
>>> +			if (ret) {
>>> +				dev_err(dev, "Can't associate reserved
>> memory\n");
>>> +				goto out;
>>> +			}
>>> +		} else {
>>> +			if (mem->va) {
>>> +				dev_warn(dev, "vdev %d buffer already
>> mapped\n",
>>> +					 rvdev->index);
>>> +				pa = rproc_va_to_pa(mem->va);
>>> +			} else {
>>> +				/* Use dma address as carveout no
>> memmapped yet */
>>> +				pa = (phys_addr_t)mem->dma;
>>> +			}
>>> +
>>> +			/* Associate vdev buffer memory pool to vdev
>> subdev */
>>> +			ret = dmam_declare_coherent_memory(dev, pa,
>>> +							   mem->da,
>>> +							   mem->len,
>>> +
>> DMA_MEMORY_EXCLUSIVE);
>>
>> Is it not possible to associate the dma_mem with vdev->dev directly,
>> instead of injecting yet another device in-between the remoteproc
>> platform device and the virtio device?
> 
> Thank you for the review.
> I'll check how to use virtio device. An additional ops for memory registration may be needed at virtio level. 
> 
>>
>> That way the resulting allocation in e.g. virtio_rpmsg would be on the
>> virtio device itself, not its parent.

> Yes agree, it will simplify allocation/inheritance...

+ Anup.

Anup,
Since you are currently interested in patch 17 (dma allocations from
vdev parent), may we get some information on how your device hierarchy
is looking like inside the VM/Guest?

regards
Suman

> 
> Regards,
> Loic
>>
>> Regards,
>> Bjorn
>>
>>> +			if (ret < 0) {
>>> +				dev_err(dev, "Failed to associate buffer\n");
>>> +				goto out;
>>> +			}
>>> +		}
>>> +	}
>>> +
>>>  	vdev->id.device	= id,
>>>  	vdev->config = &rproc_virtio_config_ops,
>>>  	vdev->dev.parent = dev;
>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>> index 6b3a234..2921dd2 100644
>>> --- a/include/linux/remoteproc.h
>>> +++ b/include/linux/remoteproc.h
>>> @@ -547,6 +547,7 @@ struct rproc_vdev {
>>>  	struct kref refcount;
>>>
>>>  	struct rproc_subdev subdev;
>>> +	struct device dev;
>>>
>>>  	unsigned int id;
>>>  	struct list_head node;
>>> --
>>> 1.9.1
>>>

Suman Anna Oct. 24, 2018, 1:48 a.m. UTC | #8

On 10/23/18 8:22 PM, Suman Anna wrote:
> On 9/27/18 3:18 PM, Wendy Liang wrote:
>> Hi Loic,
>>
>>
>> On Thu, Sep 27, 2018 at 12:22 PM Loic PALLARDY <[e-mail]> wrote:
>>>
>>> Hi Wendy
>>>
>>>> -----Original Message-----
>>>> From: Wendy Liang <[e-mail]>
>>>> Sent: Thursday, September 27, 2018 7:17 PM
>>>> To: Loic PALLARDY <[e-mail]>
>>>> Cc: Bjorn Andersson <[e-mail]>; Ohad Ben-Cohen
>>>> <[e-mail]>; [e-mail]; Linux Kernel
>>>> Mailing List <[e-mail]>; Arnaud POULIQUEN
>>>> <[e-mail]>; [e-mail]; Suman Anna
>>>> <[e-mail]>
>>>> Subject: Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with
>>>> specific dma memory pool
>>>>
>>>> On Fri, Jul 27, 2018 at 6:16 AM Loic Pallardy <[e-mail]> wrote:
>>>>>
>>>>> This patch creates a dedicated vdev subdevice for each vdev declared
>>>>> in firmware resource table and associates carveout named "vdev%dbuffer"
>>>>> (with %d vdev index in resource table) if any as dma coherent memory
>>>> pool.
>>>>>
>>>>> Then vdev subdevice is used as parent for virtio device.
>>>>>
>>>>> Signed-off-by: Loic Pallardy <[e-mail]>
>>>>> ---
>>>>>  drivers/remoteproc/remoteproc_core.c     | 35
>>>> +++++++++++++++++++++++---
>>>>>  drivers/remoteproc/remoteproc_internal.h |  1 +
>>>>>  drivers/remoteproc/remoteproc_virtio.c   | 42
>>>> +++++++++++++++++++++++++++++++-
>>>>>  include/linux/remoteproc.h               |  1 +
>>>>>  4 files changed, 75 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>>>> b/drivers/remoteproc/remoteproc_core.c
>>>>> index 4edc6f0..adcc66e 100644
>>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>>> @@ -39,6 +39,7 @@
>>>>>  #include <linux/idr.h>
>>>>>  #include <linux/elf.h>
>>>>>  #include <linux/crc32.h>
>>>>> +#include <linux/of_reserved_mem.h>
>>>>>  #include <linux/virtio_ids.h>
>>>>>  #include <linux/virtio_ring.h>
>>>>>  #include <asm/byteorder.h>
>>>>> @@ -145,7 +146,7 @@ static void rproc_disable_iommu(struct rproc
>>>> *rproc)
>>>>>         iommu_domain_free(domain);
>>>>>  }
>>>>>
>>>>> -static phys_addr_t rproc_va_to_pa(void *cpu_addr)
>>>>> +phys_addr_t rproc_va_to_pa(void *cpu_addr)
>>>>>  {
>>>>>         /*
>>>>>          * Return physical address according to virtual address location
>>>>> @@ -160,6 +161,7 @@ static phys_addr_t rproc_va_to_pa(void
>>>> *cpu_addr)
>>>>>         WARN_ON(!virt_addr_valid(cpu_addr));
>>>>>         return virt_to_phys(cpu_addr);
>>>>>  }
>>>>> +EXPORT_SYMBOL(rproc_va_to_pa);
>>>>>
>>>>>  /**
>>>>>   * rproc_da_to_va() - lookup the kernel virtual address for a remoteproc
>>>> address
>>>>> @@ -423,6 +425,20 @@ static void rproc_vdev_do_stop(struct
>>>> rproc_subdev *subdev, bool crashed)
>>>>>  }
>>>>>
>>>>>  /**
>>>>> + * rproc_rvdev_release() - release the existence of a rvdev
>>>>> + *
>>>>> + * @dev: the subdevice's dev
>>>>> + */
>>>>> +static void rproc_rvdev_release(struct device *dev)
>>>>> +{
>>>>> +       struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev, dev);
>>>>> +
>>>>> +       of_reserved_mem_device_release(dev);
>>>>> +
>>>>> +       kfree(rvdev);
>>>>> +}
>>>>> +
>>>>> +/**
>>>>>   * rproc_handle_vdev() - handle a vdev fw resource
>>>>>   * @rproc: the remote processor
>>>>>   * @rsc: the vring resource descriptor
>>>>> @@ -455,6 +471,7 @@ static int rproc_handle_vdev(struct rproc *rproc,
>>>> struct fw_rsc_vdev *rsc,
>>>>>         struct device *dev = &rproc->dev;
>>>>>         struct rproc_vdev *rvdev;
>>>>>         int i, ret;
>>>>> +       char name[16];
>>>>>
>>>>>         /* make sure resource isn't truncated */
>>>>>         if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct
>>>> fw_rsc_vdev_vring)
>>>>> @@ -488,6 +505,18 @@ static int rproc_handle_vdev(struct rproc *rproc,
>>>> struct fw_rsc_vdev *rsc,
>>>>>         rvdev->rproc = rproc;
>>>>>         rvdev->index = rproc->nb_vdev++;
>>>>>
>>>>> +       /* Initialise vdev subdevice */
>>>>> +       snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
>>>>> +       rvdev->dev.parent = rproc->dev.parent;
>>>>> +       rvdev->dev.release = rproc_rvdev_release;
>>>>> +       dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev-
>>>>> dev.parent), name);
>>>>> +       dev_set_drvdata(&rvdev->dev, rvdev);
>>>>> +       dma_set_coherent_mask(&rvdev->dev, DMA_BIT_MASK(32));
>>>> I tried the latest kernel, this function will not set the DMA coherent mask as
>>>> dma_supported() of the &rvdev->dev will return false.
>>>> As this is a device created at run time, should it be force to support DMA?
>>>> should it directly set the dma_coherent_mask?
>>>
>>> Thanks for pointing me this issue. I tested on top of 4.18-rc1 few months ago...
>>> Could you please give me kernel version on which you are testing the series?
>>> Is you platform 32bit or 64bit ?
>>> I'll rebase and check on my side.
>>
>> I am testing with 4.19-rc4 on aarch64 platform.
> 
> Btw, I ran into this on my v7 platform as well (4.19-rc6). The
> dma_set_coherent_mask fails with error EIO. I did get my allocations
> through though.

Correction, that was before Patch 17. With patch 17, this fails.

regards
Suman

> 
> regards
> Suman
> 
>>
>> Best Regards,
>> Wendy
>>>
>>> Regards,
>>> Loic
>>>
>>>>
>>>>> +
>>>>> +       ret = device_register(&rvdev->dev);
>>>>> +       if (ret)
>>>>> +               goto free_rvdev;
>>>>> +
>>>>>         /* parse the vrings */
>>>>>         for (i = 0; i < rsc->num_of_vrings; i++) {
>>>>>                 ret = rproc_parse_vring(rvdev, rsc, i);
>>>>> @@ -518,7 +547,7 @@ static int rproc_handle_vdev(struct rproc *rproc,
>>>> struct fw_rsc_vdev *rsc,
>>>>>         for (i--; i >= 0; i--)
>>>>>                 rproc_free_vring(&rvdev->vring[i]);
>>>>>  free_rvdev:
>>>>> -       kfree(rvdev);
>>>>> +       device_unregister(&rvdev->dev);
>>>>>         return ret;
>>>>>  }
>>>>>
>>>>> @@ -536,7 +565,7 @@ void rproc_vdev_release(struct kref *ref)
>>>>>
>>>>>         rproc_remove_subdev(rproc, &rvdev->subdev);
>>>>>         list_del(&rvdev->node);
>>>>> -       kfree(rvdev);
>>>>> +       device_unregister(&rvdev->dev);
>>>>>  }
>>>>>
>>>>>  /**
>>>>> diff --git a/drivers/remoteproc/remoteproc_internal.h
>>>> b/drivers/remoteproc/remoteproc_internal.h
>>>>> index f6cad24..bfeacfd 100644
>>>>> --- a/drivers/remoteproc/remoteproc_internal.h
>>>>> +++ b/drivers/remoteproc/remoteproc_internal.h
>>>>> @@ -52,6 +52,7 @@ struct dentry *rproc_create_trace_file(const char
>>>> *name, struct rproc *rproc,
>>>>>  int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
>>>>>
>>>>>  void *rproc_da_to_va(struct rproc *rproc, u64 da, int len);
>>>>> +phys_addr_t rproc_va_to_pa(void *cpu_addr);
>>>>>  int rproc_trigger_recovery(struct rproc *rproc);
>>>>>
>>>>>  int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw);
>>>>> diff --git a/drivers/remoteproc/remoteproc_virtio.c
>>>> b/drivers/remoteproc/remoteproc_virtio.c
>>>>> index de21f62..9ee63c0 100644
>>>>> --- a/drivers/remoteproc/remoteproc_virtio.c
>>>>> +++ b/drivers/remoteproc/remoteproc_virtio.c
>>>>> @@ -17,7 +17,9 @@
>>>>>   * GNU General Public License for more details.
>>>>>   */
>>>>>
>>>>> +#include <linux/dma-mapping.h>
>>>>>  #include <linux/export.h>
>>>>> +#include <linux/of_reserved_mem.h>
>>>>>  #include <linux/remoteproc.h>
>>>>>  #include <linux/virtio.h>
>>>>>  #include <linux/virtio_config.h>
>>>>> @@ -315,10 +317,48 @@ static void rproc_virtio_dev_release(struct device
>>>> *dev)
>>>>>  int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>>>>>  {
>>>>>         struct rproc *rproc = rvdev->rproc;
>>>>> -       struct device *dev = &rproc->dev;
>>>>> +       struct device *dev = &rvdev->dev;
>>>>>         struct virtio_device *vdev = &rvdev->vdev;
>>>>> +       struct rproc_mem_entry *mem;
>>>>>         int ret;
>>>>>
>>>>> +       /* Try to find dedicated vdev buffer carveout */
>>>>> +       mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer", rvdev-
>>>>> index);
>>>>> +       if (mem) {
>>>>> +               phys_addr_t pa;
>>>>> +
>>>>> +               if (mem->of_resm_idx != -1) {
>>>>> +                       struct device_node *np = rproc->dev.parent->of_node;
>>>>> +
>>>>> +                       /* Associate reserved memory to vdev device */
>>>>> +                       ret = of_reserved_mem_device_init_by_idx(dev, np,
>>>>> +                                                                mem->of_resm_idx);
>>>>> +                       if (ret) {
>>>>> +                               dev_err(dev, "Can't associate reserved memory\n");
>>>>> +                               goto out;
>>>>> +                       }
>>>>> +               } else {
>>>>> +                       if (mem->va) {
>>>>> +                               dev_warn(dev, "vdev %d buffer already mapped\n",
>>>>> +                                        rvdev->index);
>>>>> +                               pa = rproc_va_to_pa(mem->va);
>>>>> +                       } else {
>>>>> +                               /* Use dma address as carveout no memmapped yet */
>>>>> +                               pa = (phys_addr_t)mem->dma;
>>>>> +                       }
>>>>> +
>>>>> +                       /* Associate vdev buffer memory pool to vdev subdev */
>>>>> +                       ret = dmam_declare_coherent_memory(dev, pa,
>>>>> +                                                          mem->da,
>>>>> +                                                          mem->len,
>>>>> +                                                          DMA_MEMORY_EXCLUSIVE);
>>>>> +                       if (ret < 0) {
>>>>> +                               dev_err(dev, "Failed to associate buffer\n");
>>>>> +                               goto out;
>>>>> +                       }
>>>>> +               }
>>>>> +       }
>>>>> +
>>>>>         vdev->id.device = id,
>>>>>         vdev->config = &rproc_virtio_config_ops,
>>>>>         vdev->dev.parent = dev;
>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>>> index 6b3a234..2921dd2 100644
>>>>> --- a/include/linux/remoteproc.h
>>>>> +++ b/include/linux/remoteproc.h
>>>>> @@ -547,6 +547,7 @@ struct rproc_vdev {
>>>>>         struct kref refcount;
>>>>>
>>>>>         struct rproc_subdev subdev;
>>>>> +       struct device dev;
>>>>>
>>>>>         unsigned int id;
>>>>>         struct list_head node;
>>>>> --
>>>>> 1.9.1
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc"
>>>> in
>>>>> the body of a message to [e-mail]
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Loic PALLARDY Oct. 24, 2018, 12:40 p.m. UTC | #9

Hi Suman,

> -----Original Message-----
> From: Suman Anna <[e-mail]>
> Sent: mercredi 24 octobre 2018 03:22
> To: Wendy Liang <[e-mail]>; Loic PALLARDY
> <[e-mail]>
> Cc: Bjorn Andersson <[e-mail]>; Ohad Ben-Cohen
> <[e-mail]>; [e-mail]; Linux Kernel
> Mailing List <[e-mail]>; Arnaud POULIQUEN
> <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with
> specific dma memory pool
> 
> On 9/27/18 3:18 PM, Wendy Liang wrote:
> > Hi Loic,
> >
> >
> > On Thu, Sep 27, 2018 at 12:22 PM Loic PALLARDY <[e-mail]>
> wrote:
> >>
> >> Hi Wendy
> >>
> >>> -----Original Message-----
> >>> From: Wendy Liang <[e-mail]>
> >>> Sent: Thursday, September 27, 2018 7:17 PM
> >>> To: Loic PALLARDY <[e-mail]>
> >>> Cc: Bjorn Andersson <[e-mail]>; Ohad Ben-Cohen
> >>> <[e-mail]>; [e-mail]; Linux Kernel
> >>> Mailing List <[e-mail]>; Arnaud POULIQUEN
> >>> <[e-mail]>; [e-mail]; Suman
> Anna
> >>> <[e-mail]>
> >>> Subject: Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with
> >>> specific dma memory pool
> >>>
> >>> On Fri, Jul 27, 2018 at 6:16 AM Loic Pallardy <[e-mail]>
> wrote:
> >>>>
> >>>> This patch creates a dedicated vdev subdevice for each vdev declared
> >>>> in firmware resource table and associates carveout named
> "vdev%dbuffer"
> >>>> (with %d vdev index in resource table) if any as dma coherent memory
> >>> pool.
> >>>>
> >>>> Then vdev subdevice is used as parent for virtio device.
> >>>>
> >>>> Signed-off-by: Loic Pallardy <[e-mail]>
> >>>> ---
> >>>>  drivers/remoteproc/remoteproc_core.c     | 35
> >>> +++++++++++++++++++++++---
> >>>>  drivers/remoteproc/remoteproc_internal.h |  1 +
> >>>>  drivers/remoteproc/remoteproc_virtio.c   | 42
> >>> +++++++++++++++++++++++++++++++-
> >>>>  include/linux/remoteproc.h               |  1 +
> >>>>  4 files changed, 75 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/remoteproc/remoteproc_core.c
> >>> b/drivers/remoteproc/remoteproc_core.c
> >>>> index 4edc6f0..adcc66e 100644
> >>>> --- a/drivers/remoteproc/remoteproc_core.c
> >>>> +++ b/drivers/remoteproc/remoteproc_core.c
> >>>> @@ -39,6 +39,7 @@
> >>>>  #include <linux/idr.h>
> >>>>  #include <linux/elf.h>
> >>>>  #include <linux/crc32.h>
> >>>> +#include <linux/of_reserved_mem.h>
> >>>>  #include <linux/virtio_ids.h>
> >>>>  #include <linux/virtio_ring.h>
> >>>>  #include <asm/byteorder.h>
> >>>> @@ -145,7 +146,7 @@ static void rproc_disable_iommu(struct rproc
> >>> *rproc)
> >>>>         iommu_domain_free(domain);
> >>>>  }
> >>>>
> >>>> -static phys_addr_t rproc_va_to_pa(void *cpu_addr)
> >>>> +phys_addr_t rproc_va_to_pa(void *cpu_addr)
> >>>>  {
> >>>>         /*
> >>>>          * Return physical address according to virtual address location
> >>>> @@ -160,6 +161,7 @@ static phys_addr_t rproc_va_to_pa(void
> >>> *cpu_addr)
> >>>>         WARN_ON(!virt_addr_valid(cpu_addr));
> >>>>         return virt_to_phys(cpu_addr);
> >>>>  }
> >>>> +EXPORT_SYMBOL(rproc_va_to_pa);
> >>>>
> >>>>  /**
> >>>>   * rproc_da_to_va() - lookup the kernel virtual address for a
> remoteproc
> >>> address
> >>>> @@ -423,6 +425,20 @@ static void rproc_vdev_do_stop(struct
> >>> rproc_subdev *subdev, bool crashed)
> >>>>  }
> >>>>
> >>>>  /**
> >>>> + * rproc_rvdev_release() - release the existence of a rvdev
> >>>> + *
> >>>> + * @dev: the subdevice's dev
> >>>> + */
> >>>> +static void rproc_rvdev_release(struct device *dev)
> >>>> +{
> >>>> +       struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev,
> dev);
> >>>> +
> >>>> +       of_reserved_mem_device_release(dev);
> >>>> +
> >>>> +       kfree(rvdev);
> >>>> +}
> >>>> +
> >>>> +/**
> >>>>   * rproc_handle_vdev() - handle a vdev fw resource
> >>>>   * @rproc: the remote processor
> >>>>   * @rsc: the vring resource descriptor
> >>>> @@ -455,6 +471,7 @@ static int rproc_handle_vdev(struct rproc
> *rproc,
> >>> struct fw_rsc_vdev *rsc,
> >>>>         struct device *dev = &rproc->dev;
> >>>>         struct rproc_vdev *rvdev;
> >>>>         int i, ret;
> >>>> +       char name[16];
> >>>>
> >>>>         /* make sure resource isn't truncated */
> >>>>         if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct
> >>> fw_rsc_vdev_vring)
> >>>> @@ -488,6 +505,18 @@ static int rproc_handle_vdev(struct rproc
> *rproc,
> >>> struct fw_rsc_vdev *rsc,
> >>>>         rvdev->rproc = rproc;
> >>>>         rvdev->index = rproc->nb_vdev++;
> >>>>
> >>>> +       /* Initialise vdev subdevice */
> >>>> +       snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
> >>>> +       rvdev->dev.parent = rproc->dev.parent;
> >>>> +       rvdev->dev.release = rproc_rvdev_release;
> >>>> +       dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev-
> >>>> dev.parent), name);
> >>>> +       dev_set_drvdata(&rvdev->dev, rvdev);
> >>>> +       dma_set_coherent_mask(&rvdev->dev, DMA_BIT_MASK(32));
> >>> I tried the latest kernel, this function will not set the DMA coherent mask
> as
> >>> dma_supported() of the &rvdev->dev will return false.
> >>> As this is a device created at run time, should it be force to support
> DMA?
> >>> should it directly set the dma_coherent_mask?
> >>
> >> Thanks for pointing me this issue. I tested on top of 4.18-rc1 few months
> ago...
> >> Could you please give me kernel version on which you are testing the
> series?
> >> Is you platform 32bit or 64bit ?
> >> I'll rebase and check on my side.
> >
> > I am testing with 4.19-rc4 on aarch64 platform.
> 
> Btw, I ran into this on my v7 platform as well (4.19-rc6). The
> dma_set_coherent_mask fails with error EIO. I did get my allocations
> through though.

I don't have issue on v7 platform. Anyway I have now an Xilinx Ultra96 board running on my desk. It looks like vdev device doesn't have dma support, so not possible to set mask.
Need to continue the investigations...
Regards,
Loic
> 
> regards
> Suman
> 
> >
> > Best Regards,
> > Wendy
> >>
> >> Regards,
> >> Loic
> >>
> >>>
> >>>> +
> >>>> +       ret = device_register(&rvdev->dev);
> >>>> +       if (ret)
> >>>> +               goto free_rvdev;
> >>>> +
> >>>>         /* parse the vrings */
> >>>>         for (i = 0; i < rsc->num_of_vrings; i++) {
> >>>>                 ret = rproc_parse_vring(rvdev, rsc, i);
> >>>> @@ -518,7 +547,7 @@ static int rproc_handle_vdev(struct rproc
> *rproc,
> >>> struct fw_rsc_vdev *rsc,
> >>>>         for (i--; i >= 0; i--)
> >>>>                 rproc_free_vring(&rvdev->vring[i]);
> >>>>  free_rvdev:
> >>>> -       kfree(rvdev);
> >>>> +       device_unregister(&rvdev->dev);
> >>>>         return ret;
> >>>>  }
> >>>>
> >>>> @@ -536,7 +565,7 @@ void rproc_vdev_release(struct kref *ref)
> >>>>
> >>>>         rproc_remove_subdev(rproc, &rvdev->subdev);
> >>>>         list_del(&rvdev->node);
> >>>> -       kfree(rvdev);
> >>>> +       device_unregister(&rvdev->dev);
> >>>>  }
> >>>>
> >>>>  /**
> >>>> diff --git a/drivers/remoteproc/remoteproc_internal.h
> >>> b/drivers/remoteproc/remoteproc_internal.h
> >>>> index f6cad24..bfeacfd 100644
> >>>> --- a/drivers/remoteproc/remoteproc_internal.h
> >>>> +++ b/drivers/remoteproc/remoteproc_internal.h
> >>>> @@ -52,6 +52,7 @@ struct dentry *rproc_create_trace_file(const char
> >>> *name, struct rproc *rproc,
> >>>>  int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
> >>>>
> >>>>  void *rproc_da_to_va(struct rproc *rproc, u64 da, int len);
> >>>> +phys_addr_t rproc_va_to_pa(void *cpu_addr);
> >>>>  int rproc_trigger_recovery(struct rproc *rproc);
> >>>>
> >>>>  int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware
> *fw);
> >>>> diff --git a/drivers/remoteproc/remoteproc_virtio.c
> >>> b/drivers/remoteproc/remoteproc_virtio.c
> >>>> index de21f62..9ee63c0 100644
> >>>> --- a/drivers/remoteproc/remoteproc_virtio.c
> >>>> +++ b/drivers/remoteproc/remoteproc_virtio.c
> >>>> @@ -17,7 +17,9 @@
> >>>>   * GNU General Public License for more details.
> >>>>   */
> >>>>
> >>>> +#include <linux/dma-mapping.h>
> >>>>  #include <linux/export.h>
> >>>> +#include <linux/of_reserved_mem.h>
> >>>>  #include <linux/remoteproc.h>
> >>>>  #include <linux/virtio.h>
> >>>>  #include <linux/virtio_config.h>
> >>>> @@ -315,10 +317,48 @@ static void rproc_virtio_dev_release(struct
> device
> >>> *dev)
> >>>>  int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
> >>>>  {
> >>>>         struct rproc *rproc = rvdev->rproc;
> >>>> -       struct device *dev = &rproc->dev;
> >>>> +       struct device *dev = &rvdev->dev;
> >>>>         struct virtio_device *vdev = &rvdev->vdev;
> >>>> +       struct rproc_mem_entry *mem;
> >>>>         int ret;
> >>>>
> >>>> +       /* Try to find dedicated vdev buffer carveout */
> >>>> +       mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer",
> rvdev-
> >>>> index);
> >>>> +       if (mem) {
> >>>> +               phys_addr_t pa;
> >>>> +
> >>>> +               if (mem->of_resm_idx != -1) {
> >>>> +                       struct device_node *np = rproc->dev.parent->of_node;
> >>>> +
> >>>> +                       /* Associate reserved memory to vdev device */
> >>>> +                       ret = of_reserved_mem_device_init_by_idx(dev, np,
> >>>> +                                                                mem->of_resm_idx);
> >>>> +                       if (ret) {
> >>>> +                               dev_err(dev, "Can't associate reserved memory\n");
> >>>> +                               goto out;
> >>>> +                       }
> >>>> +               } else {
> >>>> +                       if (mem->va) {
> >>>> +                               dev_warn(dev, "vdev %d buffer already mapped\n",
> >>>> +                                        rvdev->index);
> >>>> +                               pa = rproc_va_to_pa(mem->va);
> >>>> +                       } else {
> >>>> +                               /* Use dma address as carveout no memmapped yet
> */
> >>>> +                               pa = (phys_addr_t)mem->dma;
> >>>> +                       }
> >>>> +
> >>>> +                       /* Associate vdev buffer memory pool to vdev subdev */
> >>>> +                       ret = dmam_declare_coherent_memory(dev, pa,
> >>>> +                                                          mem->da,
> >>>> +                                                          mem->len,
> >>>> +                                                          DMA_MEMORY_EXCLUSIVE);
> >>>> +                       if (ret < 0) {
> >>>> +                               dev_err(dev, "Failed to associate buffer\n");
> >>>> +                               goto out;
> >>>> +                       }
> >>>> +               }
> >>>> +       }
> >>>> +
> >>>>         vdev->id.device = id,
> >>>>         vdev->config = &rproc_virtio_config_ops,
> >>>>         vdev->dev.parent = dev;
> >>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >>>> index 6b3a234..2921dd2 100644
> >>>> --- a/include/linux/remoteproc.h
> >>>> +++ b/include/linux/remoteproc.h
> >>>> @@ -547,6 +547,7 @@ struct rproc_vdev {
> >>>>         struct kref refcount;
> >>>>
> >>>>         struct rproc_subdev subdev;
> >>>> +       struct device dev;
> >>>>
> >>>>         unsigned int id;
> >>>>         struct list_head node;
> >>>> --
> >>>> 1.9.1
> >>>>
> >>>> --
> >>>> To unsubscribe from this list: send the line "unsubscribe linux-
> remoteproc"
> >>> in
> >>>> the body of a message to [e-mail]
> >>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Loic PALLARDY Oct. 24, 2018, 12:42 p.m. UTC | #10

> -----Original Message-----
> From: Suman Anna <[e-mail]>
> Sent: mercredi 24 octobre 2018 03:49
> To: Wendy Liang <[e-mail]>; Loic PALLARDY
> <[e-mail]>
> Cc: Bjorn Andersson <[e-mail]>; Ohad Ben-Cohen
> <[e-mail]>; [e-mail]; Linux Kernel
> Mailing List <[e-mail]>; Arnaud POULIQUEN
> <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with
> specific dma memory pool
> 
> On 10/23/18 8:22 PM, Suman Anna wrote:
> > On 9/27/18 3:18 PM, Wendy Liang wrote:
> >> Hi Loic,
> >>
> >>
> >> On Thu, Sep 27, 2018 at 12:22 PM Loic PALLARDY <[e-mail]>
> wrote:
> >>>
> >>> Hi Wendy
> >>>
> >>>> -----Original Message-----
> >>>> From: Wendy Liang <[e-mail]>
> >>>> Sent: Thursday, September 27, 2018 7:17 PM
> >>>> To: Loic PALLARDY <[e-mail]>
> >>>> Cc: Bjorn Andersson <[e-mail]>; Ohad Ben-Cohen
> >>>> <[e-mail]>; [e-mail]; Linux Kernel
> >>>> Mailing List <[e-mail]>; Arnaud POULIQUEN
> >>>> <[e-mail]>; [e-mail]; Suman
> Anna
> >>>> <[e-mail]>
> >>>> Subject: Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with
> >>>> specific dma memory pool
> >>>>
> >>>> On Fri, Jul 27, 2018 at 6:16 AM Loic Pallardy <[e-mail]>
> wrote:
> >>>>>
> >>>>> This patch creates a dedicated vdev subdevice for each vdev declared
> >>>>> in firmware resource table and associates carveout named
> "vdev%dbuffer"
> >>>>> (with %d vdev index in resource table) if any as dma coherent
> memory
> >>>> pool.
> >>>>>
> >>>>> Then vdev subdevice is used as parent for virtio device.
> >>>>>
> >>>>> Signed-off-by: Loic Pallardy <[e-mail]>
> >>>>> ---
> >>>>>  drivers/remoteproc/remoteproc_core.c     | 35
> >>>> +++++++++++++++++++++++---
> >>>>>  drivers/remoteproc/remoteproc_internal.h |  1 +
> >>>>>  drivers/remoteproc/remoteproc_virtio.c   | 42
> >>>> +++++++++++++++++++++++++++++++-
> >>>>>  include/linux/remoteproc.h               |  1 +
> >>>>>  4 files changed, 75 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/remoteproc/remoteproc_core.c
> >>>> b/drivers/remoteproc/remoteproc_core.c
> >>>>> index 4edc6f0..adcc66e 100644
> >>>>> --- a/drivers/remoteproc/remoteproc_core.c
> >>>>> +++ b/drivers/remoteproc/remoteproc_core.c
> >>>>> @@ -39,6 +39,7 @@
> >>>>>  #include <linux/idr.h>
> >>>>>  #include <linux/elf.h>
> >>>>>  #include <linux/crc32.h>
> >>>>> +#include <linux/of_reserved_mem.h>
> >>>>>  #include <linux/virtio_ids.h>
> >>>>>  #include <linux/virtio_ring.h>
> >>>>>  #include <asm/byteorder.h>
> >>>>> @@ -145,7 +146,7 @@ static void rproc_disable_iommu(struct rproc
> >>>> *rproc)
> >>>>>         iommu_domain_free(domain);
> >>>>>  }
> >>>>>
> >>>>> -static phys_addr_t rproc_va_to_pa(void *cpu_addr)
> >>>>> +phys_addr_t rproc_va_to_pa(void *cpu_addr)
> >>>>>  {
> >>>>>         /*
> >>>>>          * Return physical address according to virtual address location
> >>>>> @@ -160,6 +161,7 @@ static phys_addr_t rproc_va_to_pa(void
> >>>> *cpu_addr)
> >>>>>         WARN_ON(!virt_addr_valid(cpu_addr));
> >>>>>         return virt_to_phys(cpu_addr);
> >>>>>  }
> >>>>> +EXPORT_SYMBOL(rproc_va_to_pa);
> >>>>>
> >>>>>  /**
> >>>>>   * rproc_da_to_va() - lookup the kernel virtual address for a
> remoteproc
> >>>> address
> >>>>> @@ -423,6 +425,20 @@ static void rproc_vdev_do_stop(struct
> >>>> rproc_subdev *subdev, bool crashed)
> >>>>>  }
> >>>>>
> >>>>>  /**
> >>>>> + * rproc_rvdev_release() - release the existence of a rvdev
> >>>>> + *
> >>>>> + * @dev: the subdevice's dev
> >>>>> + */
> >>>>> +static void rproc_rvdev_release(struct device *dev)
> >>>>> +{
> >>>>> +       struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev,
> dev);
> >>>>> +
> >>>>> +       of_reserved_mem_device_release(dev);
> >>>>> +
> >>>>> +       kfree(rvdev);
> >>>>> +}
> >>>>> +
> >>>>> +/**
> >>>>>   * rproc_handle_vdev() - handle a vdev fw resource
> >>>>>   * @rproc: the remote processor
> >>>>>   * @rsc: the vring resource descriptor
> >>>>> @@ -455,6 +471,7 @@ static int rproc_handle_vdev(struct rproc
> *rproc,
> >>>> struct fw_rsc_vdev *rsc,
> >>>>>         struct device *dev = &rproc->dev;
> >>>>>         struct rproc_vdev *rvdev;
> >>>>>         int i, ret;
> >>>>> +       char name[16];
> >>>>>
> >>>>>         /* make sure resource isn't truncated */
> >>>>>         if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct
> >>>> fw_rsc_vdev_vring)
> >>>>> @@ -488,6 +505,18 @@ static int rproc_handle_vdev(struct rproc
> *rproc,
> >>>> struct fw_rsc_vdev *rsc,
> >>>>>         rvdev->rproc = rproc;
> >>>>>         rvdev->index = rproc->nb_vdev++;
> >>>>>
> >>>>> +       /* Initialise vdev subdevice */
> >>>>> +       snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
> >>>>> +       rvdev->dev.parent = rproc->dev.parent;
> >>>>> +       rvdev->dev.release = rproc_rvdev_release;
> >>>>> +       dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev-
> >>>>> dev.parent), name);
> >>>>> +       dev_set_drvdata(&rvdev->dev, rvdev);
> >>>>> +       dma_set_coherent_mask(&rvdev->dev, DMA_BIT_MASK(32));
> >>>> I tried the latest kernel, this function will not set the DMA coherent
> mask as
> >>>> dma_supported() of the &rvdev->dev will return false.
> >>>> As this is a device created at run time, should it be force to support
> DMA?
> >>>> should it directly set the dma_coherent_mask?
> >>>
> >>> Thanks for pointing me this issue. I tested on top of 4.18-rc1 few months
> ago...
> >>> Could you please give me kernel version on which you are testing the
> series?
> >>> Is you platform 32bit or 64bit ?
> >>> I'll rebase and check on my side.
> >>
> >> I am testing with 4.19-rc4 on aarch64 platform.
> >
> > Btw, I ran into this on my v7 platform as well (4.19-rc6). The
> > dma_set_coherent_mask fails with error EIO. I did get my allocations
> > through though.
> 
> Correction, that was before Patch 17. With patch 17, this fails.

Yes normal as device for allocation is changed Patch17. 
Regards,
Loic
> 
> regards
> Suman
> 
> >
> > regards
> > Suman
> >
> >>
> >> Best Regards,
> >> Wendy
> >>>
> >>> Regards,
> >>> Loic
> >>>
> >>>>
> >>>>> +
> >>>>> +       ret = device_register(&rvdev->dev);
> >>>>> +       if (ret)
> >>>>> +               goto free_rvdev;
> >>>>> +
> >>>>>         /* parse the vrings */
> >>>>>         for (i = 0; i < rsc->num_of_vrings; i++) {
> >>>>>                 ret = rproc_parse_vring(rvdev, rsc, i);
> >>>>> @@ -518,7 +547,7 @@ static int rproc_handle_vdev(struct rproc
> *rproc,
> >>>> struct fw_rsc_vdev *rsc,
> >>>>>         for (i--; i >= 0; i--)
> >>>>>                 rproc_free_vring(&rvdev->vring[i]);
> >>>>>  free_rvdev:
> >>>>> -       kfree(rvdev);
> >>>>> +       device_unregister(&rvdev->dev);
> >>>>>         return ret;
> >>>>>  }
> >>>>>
> >>>>> @@ -536,7 +565,7 @@ void rproc_vdev_release(struct kref *ref)
> >>>>>
> >>>>>         rproc_remove_subdev(rproc, &rvdev->subdev);
> >>>>>         list_del(&rvdev->node);
> >>>>> -       kfree(rvdev);
> >>>>> +       device_unregister(&rvdev->dev);
> >>>>>  }
> >>>>>
> >>>>>  /**
> >>>>> diff --git a/drivers/remoteproc/remoteproc_internal.h
> >>>> b/drivers/remoteproc/remoteproc_internal.h
> >>>>> index f6cad24..bfeacfd 100644
> >>>>> --- a/drivers/remoteproc/remoteproc_internal.h
> >>>>> +++ b/drivers/remoteproc/remoteproc_internal.h
> >>>>> @@ -52,6 +52,7 @@ struct dentry *rproc_create_trace_file(const
> char
> >>>> *name, struct rproc *rproc,
> >>>>>  int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
> >>>>>
> >>>>>  void *rproc_da_to_va(struct rproc *rproc, u64 da, int len);
> >>>>> +phys_addr_t rproc_va_to_pa(void *cpu_addr);
> >>>>>  int rproc_trigger_recovery(struct rproc *rproc);
> >>>>>
> >>>>>  int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware
> *fw);
> >>>>> diff --git a/drivers/remoteproc/remoteproc_virtio.c
> >>>> b/drivers/remoteproc/remoteproc_virtio.c
> >>>>> index de21f62..9ee63c0 100644
> >>>>> --- a/drivers/remoteproc/remoteproc_virtio.c
> >>>>> +++ b/drivers/remoteproc/remoteproc_virtio.c
> >>>>> @@ -17,7 +17,9 @@
> >>>>>   * GNU General Public License for more details.
> >>>>>   */
> >>>>>
> >>>>> +#include <linux/dma-mapping.h>
> >>>>>  #include <linux/export.h>
> >>>>> +#include <linux/of_reserved_mem.h>
> >>>>>  #include <linux/remoteproc.h>
> >>>>>  #include <linux/virtio.h>
> >>>>>  #include <linux/virtio_config.h>
> >>>>> @@ -315,10 +317,48 @@ static void rproc_virtio_dev_release(struct
> device
> >>>> *dev)
> >>>>>  int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
> >>>>>  {
> >>>>>         struct rproc *rproc = rvdev->rproc;
> >>>>> -       struct device *dev = &rproc->dev;
> >>>>> +       struct device *dev = &rvdev->dev;
> >>>>>         struct virtio_device *vdev = &rvdev->vdev;
> >>>>> +       struct rproc_mem_entry *mem;
> >>>>>         int ret;
> >>>>>
> >>>>> +       /* Try to find dedicated vdev buffer carveout */
> >>>>> +       mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer",
> rvdev-
> >>>>> index);
> >>>>> +       if (mem) {
> >>>>> +               phys_addr_t pa;
> >>>>> +
> >>>>> +               if (mem->of_resm_idx != -1) {
> >>>>> +                       struct device_node *np = rproc->dev.parent->of_node;
> >>>>> +
> >>>>> +                       /* Associate reserved memory to vdev device */
> >>>>> +                       ret = of_reserved_mem_device_init_by_idx(dev, np,
> >>>>> +                                                                mem->of_resm_idx);
> >>>>> +                       if (ret) {
> >>>>> +                               dev_err(dev, "Can't associate reserved memory\n");
> >>>>> +                               goto out;
> >>>>> +                       }
> >>>>> +               } else {
> >>>>> +                       if (mem->va) {
> >>>>> +                               dev_warn(dev, "vdev %d buffer already mapped\n",
> >>>>> +                                        rvdev->index);
> >>>>> +                               pa = rproc_va_to_pa(mem->va);
> >>>>> +                       } else {
> >>>>> +                               /* Use dma address as carveout no memmapped yet
> */
> >>>>> +                               pa = (phys_addr_t)mem->dma;
> >>>>> +                       }
> >>>>> +
> >>>>> +                       /* Associate vdev buffer memory pool to vdev subdev */
> >>>>> +                       ret = dmam_declare_coherent_memory(dev, pa,
> >>>>> +                                                          mem->da,
> >>>>> +                                                          mem->len,
> >>>>> +                                                          DMA_MEMORY_EXCLUSIVE);
> >>>>> +                       if (ret < 0) {
> >>>>> +                               dev_err(dev, "Failed to associate buffer\n");
> >>>>> +                               goto out;
> >>>>> +                       }
> >>>>> +               }
> >>>>> +       }
> >>>>> +
> >>>>>         vdev->id.device = id,
> >>>>>         vdev->config = &rproc_virtio_config_ops,
> >>>>>         vdev->dev.parent = dev;
> >>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >>>>> index 6b3a234..2921dd2 100644
> >>>>> --- a/include/linux/remoteproc.h
> >>>>> +++ b/include/linux/remoteproc.h
> >>>>> @@ -547,6 +547,7 @@ struct rproc_vdev {
> >>>>>         struct kref refcount;
> >>>>>
> >>>>>         struct rproc_subdev subdev;
> >>>>> +       struct device dev;
> >>>>>
> >>>>>         unsigned int id;
> >>>>>         struct list_head node;
> >>>>> --
> >>>>> 1.9.1
> >>>>>
> >>>>> --
> >>>>> To unsubscribe from this list: send the line "unsubscribe linux-
> remoteproc"
> >>>> in
> >>>>> the body of a message to [e-mail]
> >>>>> More majordomo info at  http://vger.kernel.org/majordomo-
> info.html
> >

Suman Anna Oct. 25, 2018, 8:16 p.m. UTC | #11

On 10/24/18 7:40 AM, Loic PALLARDY wrote:
> Hi Suman,
> 
>> -----Original Message-----
>> From: Suman Anna <[e-mail]>
>> Sent: mercredi 24 octobre 2018 03:22
>> To: Wendy Liang <[e-mail]>; Loic PALLARDY
>> <[e-mail]>
>> Cc: Bjorn Andersson <[e-mail]>; Ohad Ben-Cohen
>> <[e-mail]>; [e-mail]; Linux Kernel
>> Mailing List <[e-mail]>; Arnaud POULIQUEN
>> <[e-mail]>; [e-mail]
>> Subject: Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with
>> specific dma memory pool
>>
>> On 9/27/18 3:18 PM, Wendy Liang wrote:
>>> Hi Loic,
>>>
>>>
>>> On Thu, Sep 27, 2018 at 12:22 PM Loic PALLARDY <[e-mail]>
>> wrote:
>>>>
>>>> Hi Wendy
>>>>
>>>>> -----Original Message-----
>>>>> From: Wendy Liang <[e-mail]>
>>>>> Sent: Thursday, September 27, 2018 7:17 PM
>>>>> To: Loic PALLARDY <[e-mail]>
>>>>> Cc: Bjorn Andersson <[e-mail]>; Ohad Ben-Cohen
>>>>> <[e-mail]>; [e-mail]; Linux Kernel
>>>>> Mailing List <[e-mail]>; Arnaud POULIQUEN
>>>>> <[e-mail]>; [e-mail]; Suman
>> Anna
>>>>> <[e-mail]>
>>>>> Subject: Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with
>>>>> specific dma memory pool
>>>>>
>>>>> On Fri, Jul 27, 2018 at 6:16 AM Loic Pallardy <[e-mail]>
>> wrote:
>>>>>>
>>>>>> This patch creates a dedicated vdev subdevice for each vdev declared
>>>>>> in firmware resource table and associates carveout named
>> "vdev%dbuffer"
>>>>>> (with %d vdev index in resource table) if any as dma coherent memory
>>>>> pool.
>>>>>>
>>>>>> Then vdev subdevice is used as parent for virtio device.
>>>>>>
>>>>>> Signed-off-by: Loic Pallardy <[e-mail]>
>>>>>> ---
>>>>>>  drivers/remoteproc/remoteproc_core.c     | 35
>>>>> +++++++++++++++++++++++---
>>>>>>  drivers/remoteproc/remoteproc_internal.h |  1 +
>>>>>>  drivers/remoteproc/remoteproc_virtio.c   | 42
>>>>> +++++++++++++++++++++++++++++++-
>>>>>>  include/linux/remoteproc.h               |  1 +
>>>>>>  4 files changed, 75 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>>>>> b/drivers/remoteproc/remoteproc_core.c
>>>>>> index 4edc6f0..adcc66e 100644
>>>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>>>> @@ -39,6 +39,7 @@
>>>>>>  #include <linux/idr.h>
>>>>>>  #include <linux/elf.h>
>>>>>>  #include <linux/crc32.h>
>>>>>> +#include <linux/of_reserved_mem.h>
>>>>>>  #include <linux/virtio_ids.h>
>>>>>>  #include <linux/virtio_ring.h>
>>>>>>  #include <asm/byteorder.h>
>>>>>> @@ -145,7 +146,7 @@ static void rproc_disable_iommu(struct rproc
>>>>> *rproc)
>>>>>>         iommu_domain_free(domain);
>>>>>>  }
>>>>>>
>>>>>> -static phys_addr_t rproc_va_to_pa(void *cpu_addr)
>>>>>> +phys_addr_t rproc_va_to_pa(void *cpu_addr)
>>>>>>  {
>>>>>>         /*
>>>>>>          * Return physical address according to virtual address location
>>>>>> @@ -160,6 +161,7 @@ static phys_addr_t rproc_va_to_pa(void
>>>>> *cpu_addr)
>>>>>>         WARN_ON(!virt_addr_valid(cpu_addr));
>>>>>>         return virt_to_phys(cpu_addr);
>>>>>>  }
>>>>>> +EXPORT_SYMBOL(rproc_va_to_pa);
>>>>>>
>>>>>>  /**
>>>>>>   * rproc_da_to_va() - lookup the kernel virtual address for a
>> remoteproc
>>>>> address
>>>>>> @@ -423,6 +425,20 @@ static void rproc_vdev_do_stop(struct
>>>>> rproc_subdev *subdev, bool crashed)
>>>>>>  }
>>>>>>
>>>>>>  /**
>>>>>> + * rproc_rvdev_release() - release the existence of a rvdev
>>>>>> + *
>>>>>> + * @dev: the subdevice's dev
>>>>>> + */
>>>>>> +static void rproc_rvdev_release(struct device *dev)
>>>>>> +{
>>>>>> +       struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev,
>> dev);
>>>>>> +
>>>>>> +       of_reserved_mem_device_release(dev);
>>>>>> +
>>>>>> +       kfree(rvdev);
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>>   * rproc_handle_vdev() - handle a vdev fw resource
>>>>>>   * @rproc: the remote processor
>>>>>>   * @rsc: the vring resource descriptor
>>>>>> @@ -455,6 +471,7 @@ static int rproc_handle_vdev(struct rproc
>> *rproc,
>>>>> struct fw_rsc_vdev *rsc,
>>>>>>         struct device *dev = &rproc->dev;
>>>>>>         struct rproc_vdev *rvdev;
>>>>>>         int i, ret;
>>>>>> +       char name[16];
>>>>>>
>>>>>>         /* make sure resource isn't truncated */
>>>>>>         if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct
>>>>> fw_rsc_vdev_vring)
>>>>>> @@ -488,6 +505,18 @@ static int rproc_handle_vdev(struct rproc
>> *rproc,
>>>>> struct fw_rsc_vdev *rsc,
>>>>>>         rvdev->rproc = rproc;
>>>>>>         rvdev->index = rproc->nb_vdev++;
>>>>>>
>>>>>> +       /* Initialise vdev subdevice */
>>>>>> +       snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
>>>>>> +       rvdev->dev.parent = rproc->dev.parent;
>>>>>> +       rvdev->dev.release = rproc_rvdev_release;
>>>>>> +       dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev-
>>>>>> dev.parent), name);
>>>>>> +       dev_set_drvdata(&rvdev->dev, rvdev);
>>>>>> +       dma_set_coherent_mask(&rvdev->dev, DMA_BIT_MASK(32));
>>>>> I tried the latest kernel, this function will not set the DMA coherent mask
>> as
>>>>> dma_supported() of the &rvdev->dev will return false.
>>>>> As this is a device created at run time, should it be force to support
>> DMA?
>>>>> should it directly set the dma_coherent_mask?
>>>>
>>>> Thanks for pointing me this issue. I tested on top of 4.18-rc1 few months
>> ago...
>>>> Could you please give me kernel version on which you are testing the
>> series?
>>>> Is you platform 32bit or 64bit ?
>>>> I'll rebase and check on my side.
>>>
>>> I am testing with 4.19-rc4 on aarch64 platform.
>>
>> Btw, I ran into this on my v7 platform as well (4.19-rc6). The
>> dma_set_coherent_mask fails with error EIO. I did get my allocations
>> through though.
> 
> I don't have issue on v7 platform. Anyway I have now an Xilinx Ultra96 board running on my desk. It looks like vdev device doesn't have dma support, so not possible to set mask.
> Need to continue the investigations...

Strange how it is working for you, the dma support on vdev device should
be the same for all of us right - vdevs are created during runtime.

Think somehow, dma_supported() function is returning non-zero value for you.

regards
Suman

> Regards,
> Loic
>>
>> regards
>> Suman
>>
>>>
>>> Best Regards,
>>> Wendy
>>>>
>>>> Regards,
>>>> Loic
>>>>
>>>>>
>>>>>> +
>>>>>> +       ret = device_register(&rvdev->dev);
>>>>>> +       if (ret)
>>>>>> +               goto free_rvdev;
>>>>>> +
>>>>>>         /* parse the vrings */
>>>>>>         for (i = 0; i < rsc->num_of_vrings; i++) {
>>>>>>                 ret = rproc_parse_vring(rvdev, rsc, i);
>>>>>> @@ -518,7 +547,7 @@ static int rproc_handle_vdev(struct rproc
>> *rproc,
>>>>> struct fw_rsc_vdev *rsc,
>>>>>>         for (i--; i >= 0; i--)
>>>>>>                 rproc_free_vring(&rvdev->vring[i]);
>>>>>>  free_rvdev:
>>>>>> -       kfree(rvdev);
>>>>>> +       device_unregister(&rvdev->dev);
>>>>>>         return ret;
>>>>>>  }
>>>>>>
>>>>>> @@ -536,7 +565,7 @@ void rproc_vdev_release(struct kref *ref)
>>>>>>
>>>>>>         rproc_remove_subdev(rproc, &rvdev->subdev);
>>>>>>         list_del(&rvdev->node);
>>>>>> -       kfree(rvdev);
>>>>>> +       device_unregister(&rvdev->dev);
>>>>>>  }
>>>>>>
>>>>>>  /**
>>>>>> diff --git a/drivers/remoteproc/remoteproc_internal.h
>>>>> b/drivers/remoteproc/remoteproc_internal.h
>>>>>> index f6cad24..bfeacfd 100644
>>>>>> --- a/drivers/remoteproc/remoteproc_internal.h
>>>>>> +++ b/drivers/remoteproc/remoteproc_internal.h
>>>>>> @@ -52,6 +52,7 @@ struct dentry *rproc_create_trace_file(const char
>>>>> *name, struct rproc *rproc,
>>>>>>  int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
>>>>>>
>>>>>>  void *rproc_da_to_va(struct rproc *rproc, u64 da, int len);
>>>>>> +phys_addr_t rproc_va_to_pa(void *cpu_addr);
>>>>>>  int rproc_trigger_recovery(struct rproc *rproc);
>>>>>>
>>>>>>  int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware
>> *fw);
>>>>>> diff --git a/drivers/remoteproc/remoteproc_virtio.c
>>>>> b/drivers/remoteproc/remoteproc_virtio.c
>>>>>> index de21f62..9ee63c0 100644
>>>>>> --- a/drivers/remoteproc/remoteproc_virtio.c
>>>>>> +++ b/drivers/remoteproc/remoteproc_virtio.c
>>>>>> @@ -17,7 +17,9 @@
>>>>>>   * GNU General Public License for more details.
>>>>>>   */
>>>>>>
>>>>>> +#include <linux/dma-mapping.h>
>>>>>>  #include <linux/export.h>
>>>>>> +#include <linux/of_reserved_mem.h>
>>>>>>  #include <linux/remoteproc.h>
>>>>>>  #include <linux/virtio.h>
>>>>>>  #include <linux/virtio_config.h>
>>>>>> @@ -315,10 +317,48 @@ static void rproc_virtio_dev_release(struct
>> device
>>>>> *dev)
>>>>>>  int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>>>>>>  {
>>>>>>         struct rproc *rproc = rvdev->rproc;
>>>>>> -       struct device *dev = &rproc->dev;
>>>>>> +       struct device *dev = &rvdev->dev;
>>>>>>         struct virtio_device *vdev = &rvdev->vdev;
>>>>>> +       struct rproc_mem_entry *mem;
>>>>>>         int ret;
>>>>>>
>>>>>> +       /* Try to find dedicated vdev buffer carveout */
>>>>>> +       mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer",
>> rvdev-
>>>>>> index);
>>>>>> +       if (mem) {
>>>>>> +               phys_addr_t pa;
>>>>>> +
>>>>>> +               if (mem->of_resm_idx != -1) {
>>>>>> +                       struct device_node *np = rproc->dev.parent->of_node;
>>>>>> +
>>>>>> +                       /* Associate reserved memory to vdev device */
>>>>>> +                       ret = of_reserved_mem_device_init_by_idx(dev, np,
>>>>>> +                                                                mem->of_resm_idx);
>>>>>> +                       if (ret) {
>>>>>> +                               dev_err(dev, "Can't associate reserved memory\n");
>>>>>> +                               goto out;
>>>>>> +                       }
>>>>>> +               } else {
>>>>>> +                       if (mem->va) {
>>>>>> +                               dev_warn(dev, "vdev %d buffer already mapped\n",
>>>>>> +                                        rvdev->index);
>>>>>> +                               pa = rproc_va_to_pa(mem->va);
>>>>>> +                       } else {
>>>>>> +                               /* Use dma address as carveout no memmapped yet
>> */
>>>>>> +                               pa = (phys_addr_t)mem->dma;
>>>>>> +                       }
>>>>>> +
>>>>>> +                       /* Associate vdev buffer memory pool to vdev subdev */
>>>>>> +                       ret = dmam_declare_coherent_memory(dev, pa,
>>>>>> +                                                          mem->da,
>>>>>> +                                                          mem->len,
>>>>>> +                                                          DMA_MEMORY_EXCLUSIVE);
>>>>>> +                       if (ret < 0) {
>>>>>> +                               dev_err(dev, "Failed to associate buffer\n");
>>>>>> +                               goto out;
>>>>>> +                       }
>>>>>> +               }
>>>>>> +       }
>>>>>> +
>>>>>>         vdev->id.device = id,
>>>>>>         vdev->config = &rproc_virtio_config_ops,
>>>>>>         vdev->dev.parent = dev;
>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>>>> index 6b3a234..2921dd2 100644
>>>>>> --- a/include/linux/remoteproc.h
>>>>>> +++ b/include/linux/remoteproc.h
>>>>>> @@ -547,6 +547,7 @@ struct rproc_vdev {
>>>>>>         struct kref refcount;
>>>>>>
>>>>>>         struct rproc_subdev subdev;
>>>>>> +       struct device dev;
>>>>>>
>>>>>>         unsigned int id;
>>>>>>         struct list_head node;
>>>>>> --
>>>>>> 1.9.1
>>>>>>
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-
>> remoteproc"
>>>>> in
>>>>>> the body of a message to [e-mail]
>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Suman Anna Oct. 25, 2018, 10:06 p.m. UTC | #12

On 10/24/18 7:42 AM, Loic PALLARDY wrote:
> 
> 
>> -----Original Message-----
>> From: Suman Anna <[e-mail]>
>> Sent: mercredi 24 octobre 2018 03:49
>> To: Wendy Liang <[e-mail]>; Loic PALLARDY
>> <[e-mail]>
>> Cc: Bjorn Andersson <[e-mail]>; Ohad Ben-Cohen
>> <[e-mail]>; [e-mail]; Linux Kernel
>> Mailing List <[e-mail]>; Arnaud POULIQUEN
>> <[e-mail]>; [e-mail]
>> Subject: Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with
>> specific dma memory pool
>>
>> On 10/23/18 8:22 PM, Suman Anna wrote:
>>> On 9/27/18 3:18 PM, Wendy Liang wrote:
>>>> Hi Loic,
>>>>
>>>>
>>>> On Thu, Sep 27, 2018 at 12:22 PM Loic PALLARDY <[e-mail]>
>> wrote:
>>>>>
>>>>> Hi Wendy
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Wendy Liang <[e-mail]>
>>>>>> Sent: Thursday, September 27, 2018 7:17 PM
>>>>>> To: Loic PALLARDY <[e-mail]>
>>>>>> Cc: Bjorn Andersson <[e-mail]>; Ohad Ben-Cohen
>>>>>> <[e-mail]>; [e-mail]; Linux Kernel
>>>>>> Mailing List <[e-mail]>; Arnaud POULIQUEN
>>>>>> <[e-mail]>; [e-mail]; Suman
>> Anna
>>>>>> <[e-mail]>
>>>>>> Subject: Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with
>>>>>> specific dma memory pool
>>>>>>
>>>>>> On Fri, Jul 27, 2018 at 6:16 AM Loic Pallardy <[e-mail]>
>> wrote:
>>>>>>>
>>>>>>> This patch creates a dedicated vdev subdevice for each vdev declared
>>>>>>> in firmware resource table and associates carveout named
>> "vdev%dbuffer"
>>>>>>> (with %d vdev index in resource table) if any as dma coherent
>> memory
>>>>>> pool.
>>>>>>>
>>>>>>> Then vdev subdevice is used as parent for virtio device.
>>>>>>>
>>>>>>> Signed-off-by: Loic Pallardy <[e-mail]>
>>>>>>> ---
>>>>>>>  drivers/remoteproc/remoteproc_core.c     | 35
>>>>>> +++++++++++++++++++++++---
>>>>>>>  drivers/remoteproc/remoteproc_internal.h |  1 +
>>>>>>>  drivers/remoteproc/remoteproc_virtio.c   | 42
>>>>>> +++++++++++++++++++++++++++++++-
>>>>>>>  include/linux/remoteproc.h               |  1 +
>>>>>>>  4 files changed, 75 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>>>>>> b/drivers/remoteproc/remoteproc_core.c
>>>>>>> index 4edc6f0..adcc66e 100644
>>>>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>>>>> @@ -39,6 +39,7 @@
>>>>>>>  #include <linux/idr.h>
>>>>>>>  #include <linux/elf.h>
>>>>>>>  #include <linux/crc32.h>
>>>>>>> +#include <linux/of_reserved_mem.h>
>>>>>>>  #include <linux/virtio_ids.h>
>>>>>>>  #include <linux/virtio_ring.h>
>>>>>>>  #include <asm/byteorder.h>
>>>>>>> @@ -145,7 +146,7 @@ static void rproc_disable_iommu(struct rproc
>>>>>> *rproc)
>>>>>>>         iommu_domain_free(domain);
>>>>>>>  }
>>>>>>>
>>>>>>> -static phys_addr_t rproc_va_to_pa(void *cpu_addr)
>>>>>>> +phys_addr_t rproc_va_to_pa(void *cpu_addr)
>>>>>>>  {
>>>>>>>         /*
>>>>>>>          * Return physical address according to virtual address location
>>>>>>> @@ -160,6 +161,7 @@ static phys_addr_t rproc_va_to_pa(void
>>>>>> *cpu_addr)
>>>>>>>         WARN_ON(!virt_addr_valid(cpu_addr));
>>>>>>>         return virt_to_phys(cpu_addr);
>>>>>>>  }
>>>>>>> +EXPORT_SYMBOL(rproc_va_to_pa);
>>>>>>>
>>>>>>>  /**
>>>>>>>   * rproc_da_to_va() - lookup the kernel virtual address for a
>> remoteproc
>>>>>> address
>>>>>>> @@ -423,6 +425,20 @@ static void rproc_vdev_do_stop(struct
>>>>>> rproc_subdev *subdev, bool crashed)
>>>>>>>  }
>>>>>>>
>>>>>>>  /**
>>>>>>> + * rproc_rvdev_release() - release the existence of a rvdev
>>>>>>> + *
>>>>>>> + * @dev: the subdevice's dev
>>>>>>> + */
>>>>>>> +static void rproc_rvdev_release(struct device *dev)
>>>>>>> +{
>>>>>>> +       struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev,
>> dev);
>>>>>>> +
>>>>>>> +       of_reserved_mem_device_release(dev);
>>>>>>> +
>>>>>>> +       kfree(rvdev);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/**
>>>>>>>   * rproc_handle_vdev() - handle a vdev fw resource
>>>>>>>   * @rproc: the remote processor
>>>>>>>   * @rsc: the vring resource descriptor
>>>>>>> @@ -455,6 +471,7 @@ static int rproc_handle_vdev(struct rproc
>> *rproc,
>>>>>> struct fw_rsc_vdev *rsc,
>>>>>>>         struct device *dev = &rproc->dev;
>>>>>>>         struct rproc_vdev *rvdev;
>>>>>>>         int i, ret;
>>>>>>> +       char name[16];
>>>>>>>
>>>>>>>         /* make sure resource isn't truncated */
>>>>>>>         if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct
>>>>>> fw_rsc_vdev_vring)
>>>>>>> @@ -488,6 +505,18 @@ static int rproc_handle_vdev(struct rproc
>> *rproc,
>>>>>> struct fw_rsc_vdev *rsc,
>>>>>>>         rvdev->rproc = rproc;
>>>>>>>         rvdev->index = rproc->nb_vdev++;
>>>>>>>
>>>>>>> +       /* Initialise vdev subdevice */
>>>>>>> +       snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
>>>>>>> +       rvdev->dev.parent = rproc->dev.parent;
>>>>>>> +       rvdev->dev.release = rproc_rvdev_release;
>>>>>>> +       dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev-
>>>>>>> dev.parent), name);
>>>>>>> +       dev_set_drvdata(&rvdev->dev, rvdev);
>>>>>>> +       dma_set_coherent_mask(&rvdev->dev, DMA_BIT_MASK(32));
>>>>>> I tried the latest kernel, this function will not set the DMA coherent
>> mask as
>>>>>> dma_supported() of the &rvdev->dev will return false.
>>>>>> As this is a device created at run time, should it be force to support
>> DMA?
>>>>>> should it directly set the dma_coherent_mask?
>>>>>
>>>>> Thanks for pointing me this issue. I tested on top of 4.18-rc1 few months
>> ago...
>>>>> Could you please give me kernel version on which you are testing the
>> series?
>>>>> Is you platform 32bit or 64bit ?
>>>>> I'll rebase and check on my side.
>>>>
>>>> I am testing with 4.19-rc4 on aarch64 platform.
>>>
>>> Btw, I ran into this on my v7 platform as well (4.19-rc6). The
>>> dma_set_coherent_mask fails with error EIO. I did get my allocations
>>> through though.
>>
>> Correction, that was before Patch 17. With patch 17, this fails.
> 
> Yes normal as device for allocation is changed Patch17. 

The failure was due to dma_alloc failure in rpmsg_probe, your patch13
should have registered the DMA pool for this, and I think the root-cause
is back to the dma_set_coherent_mask failure and unsupported dma_ops on
this device.

regards
Suman

> Regards,
> Loic
>>
>> regards
>> Suman
>>
>>>
>>> regards
>>> Suman
>>>
>>>>
>>>> Best Regards,
>>>> Wendy
>>>>>
>>>>> Regards,
>>>>> Loic
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +       ret = device_register(&rvdev->dev);
>>>>>>> +       if (ret)
>>>>>>> +               goto free_rvdev;
>>>>>>> +
>>>>>>>         /* parse the vrings */
>>>>>>>         for (i = 0; i < rsc->num_of_vrings; i++) {
>>>>>>>                 ret = rproc_parse_vring(rvdev, rsc, i);
>>>>>>> @@ -518,7 +547,7 @@ static int rproc_handle_vdev(struct rproc
>> *rproc,
>>>>>> struct fw_rsc_vdev *rsc,
>>>>>>>         for (i--; i >= 0; i--)
>>>>>>>                 rproc_free_vring(&rvdev->vring[i]);
>>>>>>>  free_rvdev:
>>>>>>> -       kfree(rvdev);
>>>>>>> +       device_unregister(&rvdev->dev);
>>>>>>>         return ret;
>>>>>>>  }
>>>>>>>
>>>>>>> @@ -536,7 +565,7 @@ void rproc_vdev_release(struct kref *ref)
>>>>>>>
>>>>>>>         rproc_remove_subdev(rproc, &rvdev->subdev);
>>>>>>>         list_del(&rvdev->node);
>>>>>>> -       kfree(rvdev);
>>>>>>> +       device_unregister(&rvdev->dev);
>>>>>>>  }
>>>>>>>
>>>>>>>  /**
>>>>>>> diff --git a/drivers/remoteproc/remoteproc_internal.h
>>>>>> b/drivers/remoteproc/remoteproc_internal.h
>>>>>>> index f6cad24..bfeacfd 100644
>>>>>>> --- a/drivers/remoteproc/remoteproc_internal.h
>>>>>>> +++ b/drivers/remoteproc/remoteproc_internal.h
>>>>>>> @@ -52,6 +52,7 @@ struct dentry *rproc_create_trace_file(const
>> char
>>>>>> *name, struct rproc *rproc,
>>>>>>>  int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
>>>>>>>
>>>>>>>  void *rproc_da_to_va(struct rproc *rproc, u64 da, int len);
>>>>>>> +phys_addr_t rproc_va_to_pa(void *cpu_addr);
>>>>>>>  int rproc_trigger_recovery(struct rproc *rproc);
>>>>>>>
>>>>>>>  int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware
>> *fw);
>>>>>>> diff --git a/drivers/remoteproc/remoteproc_virtio.c
>>>>>> b/drivers/remoteproc/remoteproc_virtio.c
>>>>>>> index de21f62..9ee63c0 100644
>>>>>>> --- a/drivers/remoteproc/remoteproc_virtio.c
>>>>>>> +++ b/drivers/remoteproc/remoteproc_virtio.c
>>>>>>> @@ -17,7 +17,9 @@
>>>>>>>   * GNU General Public License for more details.
>>>>>>>   */
>>>>>>>
>>>>>>> +#include <linux/dma-mapping.h>
>>>>>>>  #include <linux/export.h>
>>>>>>> +#include <linux/of_reserved_mem.h>
>>>>>>>  #include <linux/remoteproc.h>
>>>>>>>  #include <linux/virtio.h>
>>>>>>>  #include <linux/virtio_config.h>
>>>>>>> @@ -315,10 +317,48 @@ static void rproc_virtio_dev_release(struct
>> device
>>>>>> *dev)
>>>>>>>  int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>>>>>>>  {
>>>>>>>         struct rproc *rproc = rvdev->rproc;
>>>>>>> -       struct device *dev = &rproc->dev;
>>>>>>> +       struct device *dev = &rvdev->dev;
>>>>>>>         struct virtio_device *vdev = &rvdev->vdev;
>>>>>>> +       struct rproc_mem_entry *mem;
>>>>>>>         int ret;
>>>>>>>
>>>>>>> +       /* Try to find dedicated vdev buffer carveout */
>>>>>>> +       mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer",
>> rvdev-
>>>>>>> index);
>>>>>>> +       if (mem) {
>>>>>>> +               phys_addr_t pa;
>>>>>>> +
>>>>>>> +               if (mem->of_resm_idx != -1) {
>>>>>>> +                       struct device_node *np = rproc->dev.parent->of_node;
>>>>>>> +
>>>>>>> +                       /* Associate reserved memory to vdev device */
>>>>>>> +                       ret = of_reserved_mem_device_init_by_idx(dev, np,
>>>>>>> +                                                                mem->of_resm_idx);
>>>>>>> +                       if (ret) {
>>>>>>> +                               dev_err(dev, "Can't associate reserved memory\n");
>>>>>>> +                               goto out;
>>>>>>> +                       }
>>>>>>> +               } else {
>>>>>>> +                       if (mem->va) {
>>>>>>> +                               dev_warn(dev, "vdev %d buffer already mapped\n",
>>>>>>> +                                        rvdev->index);
>>>>>>> +                               pa = rproc_va_to_pa(mem->va);
>>>>>>> +                       } else {
>>>>>>> +                               /* Use dma address as carveout no memmapped yet
>> */
>>>>>>> +                               pa = (phys_addr_t)mem->dma;
>>>>>>> +                       }
>>>>>>> +
>>>>>>> +                       /* Associate vdev buffer memory pool to vdev subdev */
>>>>>>> +                       ret = dmam_declare_coherent_memory(dev, pa,
>>>>>>> +                                                          mem->da,
>>>>>>> +                                                          mem->len,
>>>>>>> +                                                          DMA_MEMORY_EXCLUSIVE);
>>>>>>> +                       if (ret < 0) {
>>>>>>> +                               dev_err(dev, "Failed to associate buffer\n");
>>>>>>> +                               goto out;
>>>>>>> +                       }
>>>>>>> +               }
>>>>>>> +       }
>>>>>>> +
>>>>>>>         vdev->id.device = id,
>>>>>>>         vdev->config = &rproc_virtio_config_ops,
>>>>>>>         vdev->dev.parent = dev;
>>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>>>>> index 6b3a234..2921dd2 100644
>>>>>>> --- a/include/linux/remoteproc.h
>>>>>>> +++ b/include/linux/remoteproc.h
>>>>>>> @@ -547,6 +547,7 @@ struct rproc_vdev {
>>>>>>>         struct kref refcount;
>>>>>>>
>>>>>>>         struct rproc_subdev subdev;
>>>>>>> +       struct device dev;
>>>>>>>
>>>>>>>         unsigned int id;
>>>>>>>         struct list_head node;
>>>>>>> --
>>>>>>> 1.9.1
>>>>>>>
>>>>>>> --
>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-
>> remoteproc"
>>>>>> in
>>>>>>> the body of a message to [e-mail]
>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-
>> info.html
>>>
>

Patch

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 4edc6f0..adcc66e 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -39,6 +39,7 @@ 
 #include <linux/idr.h>
 #include <linux/elf.h>
 #include <linux/crc32.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/virtio_ids.h>
 #include <linux/virtio_ring.h>
 #include <asm/byteorder.h>
@@ -145,7 +146,7 @@  static void rproc_disable_iommu(struct rproc *rproc)
 	iommu_domain_free(domain);
 }
 
-static phys_addr_t rproc_va_to_pa(void *cpu_addr)
+phys_addr_t rproc_va_to_pa(void *cpu_addr)
 {
 	/*
 	 * Return physical address according to virtual address location
@@ -160,6 +161,7 @@  static phys_addr_t rproc_va_to_pa(void *cpu_addr)
 	WARN_ON(!virt_addr_valid(cpu_addr));
 	return virt_to_phys(cpu_addr);
 }
+EXPORT_SYMBOL(rproc_va_to_pa);
 
 /**
  * rproc_da_to_va() - lookup the kernel virtual address for a remoteproc address
@@ -423,6 +425,20 @@  static void rproc_vdev_do_stop(struct rproc_subdev *subdev, bool crashed)
 }
 
 /**
+ * rproc_rvdev_release() - release the existence of a rvdev
+ *
+ * @dev: the subdevice's dev
+ */
+static void rproc_rvdev_release(struct device *dev)
+{
+	struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev, dev);
+
+	of_reserved_mem_device_release(dev);
+
+	kfree(rvdev);
+}
+
+/**
  * rproc_handle_vdev() - handle a vdev fw resource
  * @rproc: the remote processor
  * @rsc: the vring resource descriptor
@@ -455,6 +471,7 @@  static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 	struct device *dev = &rproc->dev;
 	struct rproc_vdev *rvdev;
 	int i, ret;
+	char name[16];
 
 	/* make sure resource isn't truncated */
 	if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct fw_rsc_vdev_vring)
@@ -488,6 +505,18 @@  static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 	rvdev->rproc = rproc;
 	rvdev->index = rproc->nb_vdev++;
 
+	/* Initialise vdev subdevice */
+	snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
+	rvdev->dev.parent = rproc->dev.parent;
+	rvdev->dev.release = rproc_rvdev_release;
+	dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev->dev.parent), name);
+	dev_set_drvdata(&rvdev->dev, rvdev);
+	dma_set_coherent_mask(&rvdev->dev, DMA_BIT_MASK(32));
+
+	ret = device_register(&rvdev->dev);
+	if (ret)
+		goto free_rvdev;
+
 	/* parse the vrings */
 	for (i = 0; i < rsc->num_of_vrings; i++) {
 		ret = rproc_parse_vring(rvdev, rsc, i);
@@ -518,7 +547,7 @@  static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 	for (i--; i >= 0; i--)
 		rproc_free_vring(&rvdev->vring[i]);
 free_rvdev:
-	kfree(rvdev);
+	device_unregister(&rvdev->dev);
 	return ret;
 }
 
@@ -536,7 +565,7 @@  void rproc_vdev_release(struct kref *ref)
 
 	rproc_remove_subdev(rproc, &rvdev->subdev);
 	list_del(&rvdev->node);
-	kfree(rvdev);
+	device_unregister(&rvdev->dev);
 }
 
 /**
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index f6cad24..bfeacfd 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -52,6 +52,7 @@  struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
 int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
 
 void *rproc_da_to_va(struct rproc *rproc, u64 da, int len);
+phys_addr_t rproc_va_to_pa(void *cpu_addr);
 int rproc_trigger_recovery(struct rproc *rproc);
 
 int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw);
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index de21f62..9ee63c0 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -17,7 +17,9 @@ 
  * GNU General Public License for more details.
  */
 
+#include <linux/dma-mapping.h>
 #include <linux/export.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/remoteproc.h>
 #include <linux/virtio.h>
 #include <linux/virtio_config.h>
@@ -315,10 +317,48 @@  static void rproc_virtio_dev_release(struct device *dev)
 int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
 {
 	struct rproc *rproc = rvdev->rproc;
-	struct device *dev = &rproc->dev;
+	struct device *dev = &rvdev->dev;
 	struct virtio_device *vdev = &rvdev->vdev;
+	struct rproc_mem_entry *mem;
 	int ret;
 
+	/* Try to find dedicated vdev buffer carveout */
+	mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer", rvdev->index);
+	if (mem) {
+		phys_addr_t pa;
+
+		if (mem->of_resm_idx != -1) {
+			struct device_node *np = rproc->dev.parent->of_node;
+
+			/* Associate reserved memory to vdev device */
+			ret = of_reserved_mem_device_init_by_idx(dev, np,
+								 mem->of_resm_idx);
+			if (ret) {
+				dev_err(dev, "Can't associate reserved memory\n");
+				goto out;
+			}
+		} else {
+			if (mem->va) {
+				dev_warn(dev, "vdev %d buffer already mapped\n",
+					 rvdev->index);
+				pa = rproc_va_to_pa(mem->va);
+			} else {
+				/* Use dma address as carveout no memmapped yet */
+				pa = (phys_addr_t)mem->dma;
+			}
+
+			/* Associate vdev buffer memory pool to vdev subdev */
+			ret = dmam_declare_coherent_memory(dev, pa,
+							   mem->da,
+							   mem->len,
+							   DMA_MEMORY_EXCLUSIVE);
+			if (ret < 0) {
+				dev_err(dev, "Failed to associate buffer\n");
+				goto out;
+			}
+		}
+	}
+
 	vdev->id.device	= id,
 	vdev->config = &rproc_virtio_config_ops,
 	vdev->dev.parent = dev;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 6b3a234..2921dd2 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -547,6 +547,7 @@  struct rproc_vdev {
 	struct kref refcount;
 
 	struct rproc_subdev subdev;
+	struct device dev;
 
 	unsigned int id;
 	struct list_head node;