RE: [PATCH v4 15/17] remoteproc: da8xx: declare reserved memory region for vdev device

[Loic PALLARDY]

> -----Original Message-----
> From: Suman Anna <s-anna@xxxxxx>
> Sent: mercredi 24 octobre 2018 04:58
> To: Loic PALLARDY <loic.pallardy@xxxxxx>; bjorn.andersson@xxxxxxxxxx;
> ohad@xxxxxxxxxx
> Cc: linux-remoteproc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> Arnaud POULIQUEN <arnaud.pouliquen@xxxxxx>;
> benjamin.gaignard@xxxxxxxxxx
> Subject: Re: [PATCH v4 15/17] remoteproc: da8xx: declare reserved memory
> region for vdev device
> 
> Hi Loic,
> 
> On 7/27/18 8:14 AM, Loic Pallardy wrote:
> > This patch introduces da8xx_rproc_parse_fw() to declare a
> > carveout region based on reserved memory for vdev buffer
> > allocation.
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@xxxxxx>
> > ---
> >  drivers/remoteproc/da8xx_remoteproc.c | 38
> +++++++++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> >
> > diff --git a/drivers/remoteproc/da8xx_remoteproc.c
> b/drivers/remoteproc/da8xx_remoteproc.c
> > index b668e32..679a076 100644
> > --- a/drivers/remoteproc/da8xx_remoteproc.c
> > +++ b/drivers/remoteproc/da8xx_remoteproc.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/irq.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > +#include <linux/of_address.h>
> >  #include <linux/of_reserved_mem.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/remoteproc.h>
> > @@ -179,10 +180,47 @@ static void da8xx_rproc_kick(struct rproc *rproc,
> int vqid)
> >  	writel(SYSCFG_CHIPSIG2, drproc->chipsig);
> >  }
> >
> > +static int da8xx_rproc_parse_fw(struct rproc *rproc, const struct firmware
> *fw)
> > +{
> > +	struct device *dev = rproc->dev.parent;
> > +	struct rproc_mem_entry *mem;
> > +	struct device_node *node;
> > +	struct resource res;
> > +	int err;
> > +
> > +	node = of_parse_phandle(dev->of_node, "memory-region", 0);
> > +	if (!node) {
> > +		dev_err(dev, "No memory-region specified\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	err = of_address_to_resource(node, 0, &res);
> > +	if (err) {
> > +		dev_err(dev, "Bad memory-region definition\n");
> > +		return err;
> > +	}
> > +
> > +	/* Register memory region for vdev buffer allocation */
> > +	mem = rproc_of_resm_mem_entry_init(dev, 0,
> resource_size(&res),
> > +					   res.start, "vdev0buffer");> +
> > +	if (!mem)
> > +		return -ENOMEM;
> > +
> > +	rproc_add_carveout(rproc, mem);
> > +
> > +	return rproc_elf_load_rsc_table(rproc, fw);
> > +}
> 
> Thanks for the patch, but this creates a kernel crash for me due to
> overlaps with manually created carveouts. I currently have a single
> memory-region and all allocations come from the same DMA pool, but the
> rproc_of_resm_mem_entry_init() creates an overall mem entry without the
> va being set (no alloc function plumbed in). In general, it is permitted
> to use the same reserved-memory node with multiple devices, so the index
> usage should have allowed it to do DMA allocations with vdev devices,
> but the loading is performed even before the vdev allocations and the
> da_to_va matches the first entry with no va set causing the crash.

Hummm, I didn't fall in this case, but clearly da_to_va should not crashed. 
Not allocated carveout should be bypassed in the loop. Thanks for pointing this. I need to fix it.

The rproc_of_resm_mem_entry_init() is simply registering the reserved memory to be attached to vdev device.
So that normal it won't be allocated by rproc core (there is no alloc/free function specificied in this helper). 

Regards,
Loic
> 
> Here's my debugfs output of the carveout_memories for reference,
> 
> Carveout memory entry:
>         Name: vdev0buffer
>         Virtual address: 00000000
>         DMA address: 0x00000000
>         Device address: 0xc3000000
>         Length: 0x1000000 Bytes
> 
> Carveout memory entry:
>         Name: vdev0vring0
>         Virtual address: c3000000
>         DMA address: 0xc3000000
>         Device address: 0xc3000000
>         Length: 0x3000 Bytes
> 
> Carveout memory entry:
>         Name: vdev0vring1
>         Virtual address: c3004000
>         DMA address: 0xc3004000
>         Device address: 0xc3004000
>         Length: 0x3000 Bytes
> 
> Carveout memory entry:
>         Name: DSP_MEM_DATA
>         Virtual address: c3100000
>         DMA address: 0xc3100000
>         Device address: 0xc3100000
>         Length: 0xf00000 Bytes
> 
> You can drop both this patch and the keystone_remoteproc patch from the
> series. I did not run into any issues there since I did not have any
> RSC_CARVEOUT entries there. Also, see my comments on the next patch
> (the
> changes in ST) in general regarding these API. Looks like this needs
> some more time in ironing out the issues.
> 
> regards
> Suman
> 
> 
> 
> > +
> >  static const struct rproc_ops da8xx_rproc_ops = {
> >  	.start = da8xx_rproc_start,
> >  	.stop = da8xx_rproc_stop,
> >  	.kick = da8xx_rproc_kick,
> > +	.parse_fw = da8xx_rproc_parse_fw,
> > +	.load = rproc_elf_load_segments,
> > +	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> > +	.sanity_check = rproc_elf_sanity_check,
> > +	.get_boot_addr = rproc_elf_get_boot_addr,
> >  };
> >
> >  static int da8xx_rproc_get_internal_memories(struct platform_device
> *pdev,
> >