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

From: Loic PALLARDY
Date: Wed Oct 24 2018 - 08:40:27 EST

Next message: Mrs Ruth Hamson: "Darowizna w wysokoÅci 6.500.000,00 dolarÃw od pani Ruth Hamson."
Previous message: Al Viro: "Re: [RFC][PATCH v3 01/10] fs: common implementation of file type"
In reply to: Suman Anna: "Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with specific dma memory pool"
Next in thread: Suman Anna: "Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with specific dma memory pool"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Suman,

> -----Original Message-----
> From: Suman Anna <s-anna@xxxxxx>
> Sent: mercredi 24 octobre 2018 03:22
> To: Wendy Liang <sunnyliangjy@xxxxxxxxx>; Loic PALLARDY
> <loic.pallardy@xxxxxx>
> Cc: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>; Ohad Ben-Cohen
> <ohad@xxxxxxxxxx>; linux-remoteproc@xxxxxxxxxxxxxxx; Linux Kernel
> Mailing List <linux-kernel@xxxxxxxxxxxxxxx>; Arnaud POULIQUEN
> <arnaud.pouliquen@xxxxxx>; benjamin.gaignard@xxxxxxxxxx
> 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 <loic.pallardy@xxxxxx>
> wrote:
> >>
> >> Hi Wendy
> >>
> >>> -----Original Message-----
> >>> From: Wendy Liang <sunnyliangjy@xxxxxxxxx>
> >>> Sent: Thursday, September 27, 2018 7:17 PM
> >>> To: Loic PALLARDY <loic.pallardy@xxxxxx>
> >>> Cc: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>; Ohad Ben-Cohen
> >>> <ohad@xxxxxxxxxx>; linux-remoteproc@xxxxxxxxxxxxxxx; Linux Kernel
> >>> Mailing List <linux-kernel@xxxxxxxxxxxxxxx>; Arnaud POULIQUEN
> >>> <arnaud.pouliquen@xxxxxx>; benjamin.gaignard@xxxxxxxxxx; Suman
> Anna
> >>> <s-anna@xxxxxx>
> >>> 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 <loic.pallardy@xxxxxx>
> 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 <loic.pallardy@xxxxxx>
> >>>> ---
> >>>> 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 majordomo@xxxxxxxxxxxxxxx
> >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html

Next message: Mrs Ruth Hamson: "Darowizna w wysokoÅci 6.500.000,00 dolarÃw od pani Ruth Hamson."
Previous message: Al Viro: "Re: [RFC][PATCH v3 01/10] fs: common implementation of file type"
In reply to: Suman Anna: "Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with specific dma memory pool"
Next in thread: Suman Anna: "Re: [PATCH v4 13/17] remoteproc: create vdev subdevice with specific dma memory pool"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]